init #12
init #12
Conversation
WalkthroughThis pull request enhances the run execution functionality. The Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant EC as Exec Command
participant R as Exec Runner
participant S as Signal Handler
U->>EC: Invoke run command
EC->>EC: Create cancellation context and setup flags
EC->>EC: Start polling loop with ticker for job status
EC->>R: Initiate asynchronous job execution
R->>R: Track job with RunningJob struct
R->>S: Register for termination signals
S-->>R: Signal received for graceful shutdown
R->>R: Call ExitAll to terminate all running jobs
R->>EC: Update job status and exit gracefully
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
cmd/ctrlc/root/run/exec/exec.go (2)
19-23: Flag definitions provide flexible configuration for name, workspace ID, and interval.Exposing these flags allows users to tailor the polling loop and identify the runner. Consider documenting any limits or validation for the interval flag (e.g., a minimum acceptable duration).
Also applies to: 105-109
76-101: Polling loop for queued and running jobs is clear, but consider edge cases.The ticker-based loop effectively checks job status at the specified interval. Potential improvements:
- Allow immediate shutdown without waiting for the next poll if a job becomes stuck.
- Optionally skip or adjust polling intervals if performance demands it.
Overall, this approach is straightforward and maintains the system in near-real-time sync.
cmd/ctrlc/root/run/exec/runner.go (3)
110-150:ExitAllforcibly kills processes but doesn’t wait for graceful exit.You send
SIGTERMbut immediately continue, and if the caller also callsos.Exit(0), partial cleanup could happen. For a fully graceful termination, consider a short wait or a mechanism to ensure processes have time to shut down before continuing.
153-178: Deferred file cleanup is useful, but be mindful of abrupt process termination.The script file is removed after
r.wg.Wait(). If the entire process is killed before jobs finish, these files remain. This might not be a critical concern, but keep in mind potential stale files in unusual shutdown scenarios.
226-314: Concurrent command execution logic is robust, but consider a final check for errors postWait().
- Adds the job to
runningJobsunder lock.- Spawns a new goroutine that updates job status and handles potential failures.
- Removes the job from
runningJobsif startup fails.Everything is well-coordinated, particularly with
wgusage, ensuring background tasks finish. If you want to confirm the final job outcomes at shutdown, you might incorporate a final wait or status check.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/ctrlc/root/run/exec/exec.go(3 hunks)cmd/ctrlc/root/run/exec/runner.go(3 hunks)cmd/ctrlc/root/run/run.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
cmd/ctrlc/root/run/run.go (1)
cmd/ctrlc/root/run/exec/exec.go (1)
NewRunExecCmd(18-110)
cmd/ctrlc/root/run/exec/exec.go (3)
cmd/ctrlc/root/run/exec/runner.go (2)
cmd(215-215)NewExecRunner(45-62)internal/api/client.gen.go (17)
UpsertJobAgentJSONRequestBody(538-538)err(686-686)err(1738-1738)err(1783-1783)err(1812-1812)err(1846-1846)err(1891-1891)err(1927-1927)err(1979-1979)err(2008-2008)err(2042-2042)err(2087-2087)err(2116-2116)err(2150-2150)err(2184-2184)err(2218-2218)err(2263-2263)pkg/jobagent/runner.go (1)
NewJobAgent(17-39)
cmd/ctrlc/root/run/exec/runner.go (2)
pkg/jobagent/runner.go (3)
Runner(12-15)wg(66-66)wg(119-119)internal/api/client.gen.go (55)
ClientWithResponses(3319-3321)c(1064-1074)c(1076-1086)c(1088-1098)c(1100-1110)c(1112-1122)c(1124-1134)c(1136-1146)c(1148-1158)c(1160-1170)c(1172-1182)c(1184-1194)c(1196-1206)c(1208-1218)c(1220-1230)c(1232-1242)c(1244-1254)r(3511-3516)r(3519-3524)r(3540-3545)r(3548-3553)r(3568-3573)r(3576-3581)r(3593-3598)r(3601-3606)r(3621-3626)r(3629-3634)r(3654-3659)r(3662-3667)r(3683-3688)r(3691-3696)r(3704-3709)r(3712-3717)Job(128-144)JobStatus(147-147)JobStatusSuccessful(36-36)JobStatusCancelled(28-28)err(686-686)err(1738-1738)err(1783-1783)err(1812-1812)err(1846-1846)err(1891-1891)err(1927-1927)err(1979-1979)err(2008-2008)err(2042-2042)err(2087-2087)err(2116-2116)err(2150-2150)err(2184-2184)err(2218-2218)err(2263-2263)JobStatusInProgress(31-31)Wait(55-55)
🔇 Additional comments (7)
cmd/ctrlc/root/run/run.go (1)
17-17: Simplify command registration by removing thecliutildependency.This replacement cleanly delegates interval and other configuration to the
NewRunExecCmd()function. No issues found here; looks good!cmd/ctrlc/root/run/exec/exec.go (3)
4-9: Imports added for context, signal handling, and interval logic.These imports (
context,os/signal,syscall, andtime) are consistent with the new polling and graceful shutdown approach. No concerns here.
36-60: Job agent creation and runner instantiation look solid.The logic correctly:
- Creates an API client using provided credentials.
- Initializes an ExecRunner to handle job execution.
- Constructs a job agent config object populated with name/type/workspaceID.
- Registers the agent and logs relevant info.
Everything aligns well with the intended design.
62-75: Graceful shutdown with signal handling is well-structured.Capturing OS signals into a channel, calling
runner.ExitAll(true), and canceling the context is a clean approach. This ensures existing jobs can be finalized properly on interrupt/termination.cmd/ctrlc/root/run/exec/runner.go (3)
29-35: New structsRunningJobandExecRunnereffectively capture job metadata and manage concurrency.·
RunningJobneatly tracks command, job ID, exit code, etc.
·ExecRunnermaintains a map of running jobs and an API client reference.This setup is a good foundation for flexible job management.
Also applies to: 37-42
64-108:Statusmethod concurrency is handled carefully with mutex usage.
- Reads
runningJobsunder a lock.- Removes the job from
runningJobswhen completed.- Correctly reports final status (cancelled, failed, success).
No issues observed. The approach to handle an “unusual case” (i.e., process not captured by
ProcessState) is a good safety net.
215-219: Platform-specific script execution is handled well.Using
powershell -Fileon Windows andbashon other systems is standard. No immediate concerns here.
| // NewExecRunner creates a new ExecRunner | ||
| func NewExecRunner(client *api.ClientWithResponses) *ExecRunner { | ||
| runner := &ExecRunner{ | ||
| runningJobs: make(map[string]*RunningJob), | ||
| client: client, | ||
| } | ||
|
|
||
| // Set up signal handling for graceful shutdown | ||
| c := make(chan os.Signal, 1) | ||
| signal.Notify(c, os.Interrupt, syscall.SIGTERM) | ||
| go func() { | ||
| <-c | ||
| log.Info("Received shutdown signal, terminating all jobs") | ||
| runner.ExitAll(true) | ||
| os.Exit(0) | ||
| }() | ||
|
|
||
| return runner | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
NewExecRunner: Potential duplication of signal handling.
This constructor also sets up signal handling to call ExitAll(true) and os.Exit(0), but the upper layer (exec.go) configures signals too. This may lead to multiple goroutines responding to the same signals. Consider either removing one signal handler or clarifying the chain of shutdown responsibilities to avoid redundancy or conflicting exits.
Summary by CodeRabbit
New Features
Refactor