Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request introduces an HTTP-based schedule trigger server component. A new scheduleserver package provides a Server type that manages scheduled triggers for services via REST endpoints. The Server allocates an available port, exposes GET /schedules/{serviceId}/{scheduleIndex} to trigger specific schedules with optional asynchronous execution, and includes lifecycle management through Start and Stop methods. The ServiceSimulation type gains a TriggerSchedule method that validates schedule indices and sends HTTP POST requests to trigger scheduled paths. SimulationServer integrates the schedule trigger server, starting it after services initialize and stopping it during shutdown. The changes enable external triggering of service schedules through HTTP requests. Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/internal/simulation/service/service.go (1)
129-176: Well ackchyually… the cron callback closes over loop variables, so all jobs will hit the last schedule’s path.Inside
startSchedules, the function passed tocron.AddFunccapturesschedule,cronExpression, andurlfrom theforloop. Because these are reused each iteration, all scheduled callbacks will end up using the values from the final loop iteration (last schedule) when they actually fire, which is a functional bug.You can fix this by copying the per-iteration values into new locals before registering the cron func and closing over those instead:
- for _, schedule := range s.intent.Schedules { - cronExpression := strings.TrimSpace(schedule.Cron) + for _, schedule := range s.intent.Schedules { + sched := schedule + cronExpression := strings.TrimSpace(sched.Cron) if cronExpression == "" { return nil, fmt.Errorf("service %s has a schedule with an empty cron expression", style.Teal(fmt.Sprintf("[%s]", s.name))) } - url := url.URL{ + schedURL := url.URL{ Scheme: "http", Host: fmt.Sprintf("localhost:%d", s.port), - Path: schedule.Path, + Path: sched.Path, } + urlStr := schedURL.String() - _, err := cron.AddFunc(cronExpression, func() { - fmt.Printf("%s -> %s POST %s\n", style.Purple(fmt.Sprintf("[schedule.%s] (%s)", s.name, cronExpression)), style.Teal(fmt.Sprintf("[%s]", s.name)), schedule.Path) - req, err := http.NewRequest(http.MethodPost, url.String(), nil) + _, err := cron.AddFunc(cronExpression, func() { + fmt.Printf("%s -> %s POST %s\n", + style.Purple(fmt.Sprintf("[schedule.%s] (%s)", s.name, cronExpression)), + style.Teal(fmt.Sprintf("[%s]", s.name)), + sched.Path, + ) + req, err := http.NewRequest(http.MethodPost, urlStr, nil) if err != nil { // log the error fmt.Fprint(stderrorWriter, "error creating request for schedule", err) return }This ensures each cron entry consistently targets the intended path and logs the correct cron expression.
Also applies to: 247-257
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cli/internal/scheduleserver/server.go(1 hunks)cli/internal/simulation/service/service.go(3 hunks)cli/internal/simulation/simulation.go(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-23T23:36:19.429Z
Learnt from: davemooreuws
Repo: nitrictech/suga PR: 74
File: cli/internal/simulation/simulation.go:85-85
Timestamp: 2025-09-23T23:36:19.429Z
Learning: In the Suga CLI, there are two different contexts for SugaIntro Dashboard output: 1) Simulation server (simulation.go) where Dashboard was correctly removed because it was non-functional, and 2) Development WebSocket server (suga.go) where Dashboard URL is valid and needed for syncing with the dashboard using team slug and port parameters.
Applied to files:
cli/internal/simulation/simulation.go
🧬 Code graph analysis (3)
cli/internal/scheduleserver/server.go (1)
cli/internal/netx/net.go (4)
ReservedPort(52-52)GetNextPort(72-93)MinPort(46-50)MaxPort(40-44)
cli/internal/simulation/service/service.go (1)
cli/internal/simulation/service/status.go (1)
Status_Running(9-9)
cli/internal/simulation/simulation.go (3)
cli/internal/netx/net.go (1)
ReservedPort(52-52)cli/internal/simulation/service/service.go (1)
ServiceSimulation(21-38)cli/internal/scheduleserver/server.go (3)
Server(13-19)ServiceWithSchedules(22-25)NewServer(28-44)
Comment on lines
+1
to
+150
| package scheduleserver | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "net" | ||
| "net/http" | ||
| "strconv" | ||
|
|
||
| "github.com/nitrictech/suga/cli/internal/netx" | ||
| ) | ||
|
|
||
| // Server provides HTTP endpoints to manually trigger schedules | ||
| type Server struct { | ||
| services map[string]ServiceWithSchedules | ||
| mux *http.ServeMux | ||
| listener net.Listener | ||
| port netx.ReservedPort | ||
| server *http.Server | ||
| } | ||
|
|
||
| // ServiceWithSchedules interface for services that support schedule triggering | ||
| type ServiceWithSchedules interface { | ||
| GetName() string | ||
| TriggerSchedule(index int, async bool) error | ||
| } | ||
|
|
||
| // NewServer creates a new schedule trigger server | ||
| func NewServer(services map[string]ServiceWithSchedules) (*Server, error) { | ||
| // Get an available port | ||
| port, err := netx.GetNextPort(netx.MinPort(8000), netx.MaxPort(8999)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to find open port: %w", err) | ||
| } | ||
|
|
||
| s := &Server{ | ||
| services: services, | ||
| mux: http.NewServeMux(), | ||
| port: port, | ||
| } | ||
|
|
||
| s.setupRoutes() | ||
|
|
||
| return s, nil | ||
| } | ||
|
|
||
| func (s *Server) setupRoutes() { | ||
| // Schedule trigger endpoint: GET /schedules/{serviceId}/{scheduleIndex}?async=true | ||
| s.mux.HandleFunc("/schedules/{serviceId}/{scheduleIndex}", s.handleTriggerSchedule) | ||
| } | ||
|
|
||
| func (s *Server) handleTriggerSchedule(w http.ResponseWriter, r *http.Request) { | ||
| // Only accept GET requests | ||
| if r.Method != http.MethodGet { | ||
| w.WriteHeader(http.StatusMethodNotAllowed) | ||
| fmt.Fprintf(w, "✗ Method not allowed. Use GET to trigger schedules.\n") | ||
| return | ||
| } | ||
|
|
||
| // Extract path parameters | ||
| serviceId := r.PathValue("serviceId") | ||
| scheduleIndexStr := r.PathValue("scheduleIndex") | ||
|
|
||
| // Parse schedule index | ||
| scheduleIndex, err := strconv.Atoi(scheduleIndexStr) | ||
| if err != nil { | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| fmt.Fprintf(w, "✗ Invalid schedule index: %s\n", scheduleIndexStr) | ||
| return | ||
| } | ||
|
|
||
| // Check if schedule index is valid (non-negative) | ||
| if scheduleIndex < 0 { | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| fmt.Fprintf(w, "✗ Schedule index must be non-negative\n") | ||
| return | ||
| } | ||
|
|
||
| // Find the service | ||
| svc, ok := s.services[serviceId] | ||
| if !ok { | ||
| w.WriteHeader(http.StatusNotFound) | ||
| fmt.Fprintf(w, "✗ Service '%s' not found\n", serviceId) | ||
| return | ||
| } | ||
|
|
||
| // Parse async query parameter (defaults to false for synchronous execution) | ||
| asyncStr := r.URL.Query().Get("async") | ||
| async := false | ||
| if asyncStr == "true" || asyncStr == "1" { | ||
| async = true | ||
| } | ||
|
|
||
| // Trigger the schedule | ||
| err = svc.TriggerSchedule(scheduleIndex, async) | ||
| if err != nil { | ||
| w.WriteHeader(http.StatusInternalServerError) | ||
| fmt.Fprintf(w, "✗ Failed to trigger schedule %d on service '%s': %v\n", scheduleIndex, serviceId, err) | ||
| return | ||
| } | ||
|
|
||
| // Success response | ||
| w.WriteHeader(http.StatusOK) | ||
| if async { | ||
| fmt.Fprintf(w, "✓ Schedule %d on service '%s' triggered asynchronously\n", scheduleIndex, serviceId) | ||
| } else { | ||
| fmt.Fprintf(w, "✓ Schedule %d on service '%s' executed successfully\n", scheduleIndex, serviceId) | ||
| } | ||
| } | ||
|
|
||
| // Start starts the HTTP server | ||
| func (s *Server) Start() error { | ||
| addr := fmt.Sprintf("localhost:%d", s.port) | ||
|
|
||
| listener, err := net.Listen("tcp", addr) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to listen on %s: %w", addr, err) | ||
| } | ||
|
|
||
| s.listener = listener | ||
| s.server = &http.Server{ | ||
| Handler: s.mux, | ||
| } | ||
|
|
||
| // Start server in goroutine | ||
| go func() { | ||
| if err := s.server.Serve(listener); err != nil && err != http.ErrServerClosed { | ||
| fmt.Printf("Schedule trigger server error: %v\n", err) | ||
| } | ||
| }() | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // Stop stops the HTTP server | ||
| func (s *Server) Stop() error { | ||
| if s.server != nil { | ||
| return s.server.Close() | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // GetPort returns the port the server is listening on | ||
| func (s *Server) GetPort() int { | ||
| return int(s.port) | ||
| } | ||
|
|
||
| // GetURL returns the base URL of the schedule trigger server | ||
| func (s *Server) GetURL() string { | ||
| return fmt.Sprintf("http://localhost:%d", s.port) | ||
| } |
Contributor
There was a problem hiding this comment.
Well ackchyually… the schedule trigger HTTP API treats all TriggerSchedule failures as 500, even for client mistakes.
Right now handleTriggerSchedule turns any TriggerSchedule error into http.StatusInternalServerError, which means user errors like “schedule index out of range” or “service not running” will look like server faults.
If you’re willing to standardize a couple of sentinel error values or types from your ServiceWithSchedules implementations (e.g. ErrScheduleNotFound, ErrServiceNotRunning), you could map them to more appropriate responses:
- 400 for invalid schedule index.
- 404 for unknown service or schedule.
- 409 or 503 for “service not running”.
Sketch:
// In scheduleserver or a shared package:
var (
ErrScheduleNotFound = errors.New("schedule not found")
ErrServiceNotRunning = errors.New("service not running")
)and then:
- err = svc.TriggerSchedule(scheduleIndex, async)
- if err != nil {
- w.WriteHeader(http.StatusInternalServerError)
- fmt.Fprintf(w, "✗ Failed to trigger schedule %d on service '%s': %v\n", scheduleIndex, serviceId, err)
- return
- }
+ if err := svc.TriggerSchedule(scheduleIndex, async); err != nil {
+ switch {
+ case errors.Is(err, ErrScheduleNotFound):
+ w.WriteHeader(http.StatusNotFound)
+ case errors.Is(err, ErrServiceNotRunning):
+ w.WriteHeader(http.StatusConflict)
+ default:
+ w.WriteHeader(http.StatusInternalServerError)
+ }
+ fmt.Fprintf(w, "✗ Failed to trigger schedule %d on service '%s': %v\n", scheduleIndex, serviceId, err)
+ return
+ }That keeps the core wiring intact while making the HTTP surface a bit more self-explanatory for callers.
🤖 Prompt for AI Agents
In cli/internal/scheduleserver/server.go lines 1-150, the handler currently
treats every TriggerSchedule error as a 500; add shared sentinel errors (e.g.
ErrScheduleNotFound, ErrServiceNotRunning) in this package or a small shared
package, have ServiceWithSchedules implementations return those sentinel errors,
and update handleTriggerSchedule to inspect errors using errors.Is and map them
to appropriate HTTP statuses (400 for invalid index, 404 for ErrScheduleNotFound
or unknown service, 409/503 for ErrServiceNotRunning, and keep 500 for
unknown/internal errors); ensure comparisons use errors.Is and update imports
accordingly.
Comment on lines
+280
to
+333
| // TriggerSchedule manually triggers a schedule by index | ||
| // If async is true, the schedule runs in a goroutine and returns immediately | ||
| // If async is false, waits for the HTTP response | ||
| func (s *ServiceSimulation) TriggerSchedule(index int, async bool) error { | ||
| // Validate schedule index | ||
| if index < 0 || index >= len(s.intent.Schedules) { | ||
| return fmt.Errorf("schedule index %d out of range (service has %d schedules)", index, len(s.intent.Schedules)) | ||
| } | ||
|
|
||
| // Check if service is running | ||
| if s.currentStatus != Status_Running { | ||
| return fmt.Errorf("service is not running (current status: %v)", s.currentStatus) | ||
| } | ||
|
|
||
| schedule := s.intent.Schedules[index] | ||
|
|
||
| // Build the URL | ||
| url := url.URL{ | ||
| Scheme: "http", | ||
| Host: fmt.Sprintf("localhost:%d", s.port), | ||
| Path: schedule.Path, | ||
| } | ||
|
|
||
| // Function to execute the schedule | ||
| executeSchedule := func() error { | ||
| req, err := http.NewRequest(http.MethodPost, url.String(), nil) | ||
| if err != nil { | ||
| return fmt.Errorf("error creating request: %w", err) | ||
| } | ||
|
|
||
| resp, err := http.DefaultClient.Do(req) | ||
| if err != nil { | ||
| return fmt.Errorf("error sending request: %w", err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode < 200 || resp.StatusCode >= 300 { | ||
| return fmt.Errorf("request returned status %d", resp.StatusCode) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| if async { | ||
| // Asynchronous execution | ||
| go func() { | ||
| _ = executeSchedule() | ||
| }() | ||
| return nil | ||
| } | ||
|
|
||
| // Synchronous execution | ||
| return executeSchedule() | ||
| } |
Contributor
There was a problem hiding this comment.
Well ackchyually… TriggerSchedule works but drops async errors and has no HTTP timeout.
The synchronous path is fine, but for async == true you completely ignore executeSchedule errors, and both sync/async paths use http.DefaultClient with no timeout, so a hung handler can block the HTTP handler (sync) or leak a goroutine (async).
If you care about observability and avoiding indefinite hangs, consider:
- Using a client with a reasonable timeout.
- Logging async failures somewhere (even to stderr) instead of silently discarding them.
For example:
+ httpClient := &http.Client{Timeout: 30 * time.Second}
+
// Function to execute the schedule
executeSchedule := func() error {
- req, err := http.NewRequest(http.MethodPost, url.String(), nil)
+ req, err := http.NewRequest(http.MethodPost, url.String(), nil)
if err != nil {
return fmt.Errorf("error creating request: %w", err)
}
- resp, err := http.DefaultClient.Do(req)
+ resp, err := httpClient.Do(req)
@@
if async {
// Asynchronous execution
go func() {
- _ = executeSchedule()
+ if err := executeSchedule(); err != nil {
+ // TODO: route this through the service’s logging if desired
+ fmt.Fprintf(os.Stderr, "schedule trigger failed for %s[%d]: %v\n", s.name, index, err)
+ }
}()
return nil
}That keeps the current API while making async failures visible and bounded in time.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cli/internal/simulation/service/service.go around lines 280 to 333, the async
branch of TriggerSchedule currently discards executeSchedule errors and both
paths use http.DefaultClient without a timeout; update the function to create
and use an http.Client with a reasonable Timeout (e.g. a few seconds) for both
sync and async calls, and ensure async execution logs any returned error (to the
service logger or stderr) instead of silently ignoring it — e.g., call
executeSchedule in the goroutine and log non-nil errors so async failures are
observable and time-bounded.
Comment on lines
+413
to
+469
| func (s *SimulationServer) startScheduleTriggerServer(output io.Writer) error { | ||
| // Check if any service has schedules | ||
| hasSchedules := false | ||
| for _, serviceIntent := range s.appSpec.ServiceIntents { | ||
| if len(serviceIntent.Schedules) > 0 { | ||
| hasSchedules = true | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if !hasSchedules { | ||
| return nil | ||
| } | ||
|
|
||
| // Convert services map to interface map for schedule trigger server | ||
| servicesWithSchedules := make(map[string]scheduleserver.ServiceWithSchedules) | ||
| for name, svc := range s.services { | ||
| servicesWithSchedules[name] = svc | ||
| } | ||
|
|
||
| // Create and start the schedule trigger server | ||
| triggerServer, err := scheduleserver.NewServer(servicesWithSchedules) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create schedule trigger server: %w", err) | ||
| } | ||
|
|
||
| err = triggerServer.Start() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to start schedule trigger server: %w", err) | ||
| } | ||
|
|
||
| s.scheduleTriggerSv = triggerServer | ||
|
|
||
| fmt.Fprintf(output, "%s\n\n", style.Purple("Schedule Triggers")) | ||
|
|
||
| // Print clickable trigger URLs for each service's schedules | ||
| for serviceName, serviceIntent := range s.appSpec.ServiceIntents { | ||
| if len(serviceIntent.Schedules) == 0 { | ||
| continue | ||
| } | ||
|
|
||
| for i, schedule := range serviceIntent.Schedules { | ||
| triggerURL := fmt.Sprintf("%s/schedules/%s/%d", triggerServer.GetURL(), serviceName, i) | ||
| fmt.Fprintf(output, "%s %s schedule %d (%s -> %s)\n Trigger: %s\n", | ||
| greenCheck, | ||
| styledName(serviceName, style.Teal), | ||
| i, | ||
| style.Gray(schedule.Cron), | ||
| style.Gray(fmt.Sprintf("POST %s", schedule.Path)), | ||
| style.Cyan(triggerURL)) | ||
| } | ||
| } | ||
|
|
||
| fmt.Fprint(output, "\n") | ||
|
|
||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
Well ackchyually… schedule trigger wiring looks good, but you may want to tighten service selection and shutdown behavior.
A couple of small nits:
servicesWithSchedulescurrently includes all services, even ones without schedules. That’s harmless but means/schedules/foo/0can 500 purely because the service has no schedules. You could restrict the map to only services that actually havelen(serviceIntent.Schedules) > 0to better match the printed URLs and avoid odd 500s.- In
Stop, ifscheduleTriggerSv.Stop()fails you return immediately and skip shutting down services and databases. For a CLI tool it might be preferable to log the error and continue best-effort cleanup instead of bailing out early.
Something like:
- servicesWithSchedules := make(map[string]scheduleserver.ServiceWithSchedules)
- for name, svc := range s.services {
- servicesWithSchedules[name] = svc
- }
+ servicesWithSchedules := make(map[string]scheduleserver.ServiceWithSchedules)
+ for name, svc := range s.services {
+ if intent, ok := s.appSpec.ServiceIntents[name]; ok && len(intent.Schedules) > 0 {
+ servicesWithSchedules[name] = svc
+ }
+ }and:
- if s.scheduleTriggerSv != nil {
- fmt.Println("Stopping schedule trigger server...")
- if err := s.scheduleTriggerSv.Stop(); err != nil {
- return fmt.Errorf("failed to stop schedule trigger server: %w", err)
- }
- }
+ if s.scheduleTriggerSv != nil {
+ fmt.Println("Stopping schedule trigger server...")
+ if err := s.scheduleTriggerSv.Stop(); err != nil {
+ fmt.Printf("Warning: failed to stop schedule trigger server: %v\n", err)
+ }
+ }would keep behavior robust with minimal extra complexity.
Also applies to: 512-518, 530-536
🤖 Prompt for AI Agents
In cli/internal/simulation/simulation.go around lines 413-469 (and similarly
adjust at 512-518 and 530-536), tighten the schedule trigger wiring by only
adding services that actually have schedules to servicesWithSchedules (iterate
s.appSpec.ServiceIntents and for each with len(Schedules)>0, copy the
corresponding s.services entry into the map) so the schedule server only exposes
valid endpoints; and change Stop behavior so that when scheduleTriggerSv.Stop()
returns an error you log or record the error and continue to stop remaining
services/databases (do not return immediately) to ensure best-effort cleanup in
the CLI.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.