Harden specialist background lifecycle#116
Conversation
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a specialist.Runtime for lazy background-manager init and per-task prompt-file tracking; implements Manager.MarkExited and Manager.KillRunning; wires the runtime into tool registration and Executor; ensures CLI entry points defer runtime.Close() to kill running tasks and remove prompt files. ChangesSpecialist Runtime Lifecycle
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/specialist/registry.go (1)
5-19: 💤 Low valuePass-by-value executor: local mutations have no effect.
RegisterToolsreceivesExecutorby value (line 5), so assignments on lines 13–14 modify only the local copy and are discarded when the function returns. If the intent is to update the caller's executor, change the signature toexecutor *Executor. If the intent is to treatExecutoras immutable config (which matches current usage), remove lines 13–14 to avoid confusion.In practice this is harmless because the CLI always passes a fresh
Executor, but the dead assignments are misleading.♻️ Option 1: Remove dead assignments (preferred if Executor is immutable)
func RegisterTools(registry *tools.Registry, executor Executor) (*Runtime, error) { runtime := executor.BackgroundRuntime if runtime == nil { runtime = NewRuntime(RuntimeOptions{ Manager: executor.BackgroundManager, ManagerFunc: executor.BackgroundManagerFunc, }) } - executor.BackgroundRuntime = runtime - executor.BackgroundManagerFunc = runtime.Manager registry.Register(NewTaskTool(executor))♻️ Option 2: Change to pointer receiver (if mutation intended)
-func RegisterTools(registry *tools.Registry, executor Executor) (*Runtime, error) { +func RegisterTools(registry *tools.Registry, executor *Executor) (*Runtime, error) { runtime := executor.BackgroundRuntime(Would also require updating call sites.)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specialist/registry.go` around lines 5 - 19, RegisterTools currently takes Executor by value so the assignments to executor.BackgroundRuntime and executor.BackgroundManagerFunc inside RegisterTools (in function RegisterTools) modify only the local copy and have no effect; either remove those dead assignments (delete the lines that set executor.BackgroundRuntime and executor.BackgroundManagerFunc) if Executor is intended to be immutable, or change the function signature to accept a pointer (*Executor) and update all call sites so the mutations to BackgroundRuntime and BackgroundManagerFunc persist — adjust NewRuntime usage to set runtime on the caller's Executor when choosing the pointer option.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/specialist/registry.go`:
- Around line 5-19: RegisterTools currently takes Executor by value so the
assignments to executor.BackgroundRuntime and executor.BackgroundManagerFunc
inside RegisterTools (in function RegisterTools) modify only the local copy and
have no effect; either remove those dead assignments (delete the lines that set
executor.BackgroundRuntime and executor.BackgroundManagerFunc) if Executor is
intended to be immutable, or change the function signature to accept a pointer
(*Executor) and update all call sites so the mutations to BackgroundRuntime and
BackgroundManagerFunc persist — adjust NewRuntime usage to set runtime on the
caller's Executor when choosing the pointer option.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c18c297a-0191-41c5-834f-178374a308b6
📒 Files selected for processing (8)
internal/background/manager.gointernal/background/manager_test.gointernal/cli/app.gointernal/cli/exec.gointernal/specialist/exec.gointernal/specialist/registry.gointernal/specialist/runtime.gointernal/specialist/runtime_test.go
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Blockers
internal/specialist/exec.go:314/internal/specialist/runtime.go:91: shutdown cleanup kills running background tasks, but the child wait callback can still overwrite the terminalkilledstate witherrorafter the stopped process exits non-zero. That leavesTaskOutputreporting a runtime-close/TaskStop cleanup as an error instead of a deliberate kill. I reproduced this on the PR head with a focused runtime-close probe:Runtime.Close()marked the taskkilled, then the simulated child exit callback changed it toerror.
Non-Blocking
internal/specialist/output_tool.go: still reads the full task output file on every poll. A tail/limit can come later, but it would make long-running background tasks safer to inspect.
Looks Good
- Runtime ownership keeps the background manager lazy;
Runtime.Close()does not create storage when no background task was used. - Interactive and headless exec paths now defer specialist runtime cleanup, and prompt temp files are tracked for background children.
- Local validation passed:
go test -count=1 ./internal/background ./internal/specialist ./internal/cli, fullgo test -count=1 -p 1 ./...,go build ./cmd/zero,zero exec --list-tools --auto high, andgit diff --check. - CI is green at the reviewed head.
Verdict: Changes Requested — preserve the explicit killed terminal state after runtime shutdown/TaskStop before shipping this lifecycle cleanup.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Re-review — this resolves the #115 lifecycle gap, approving
Exactly the fix I asked for on #115, and cleanly done:
- Orphans on shutdown —
Runtime.Close()→manager.KillRunning()terminates still-running background tasks (reusing #108's TOCTOU-safeKill, andKillRunningonly touchesStatusRunningtasks). - Prompt-file leak — prompt files are tracked on launch, untracked+cleaned on child exit (normal/error paths), and cleaned by
Close()if the runtime exits while a child is still running.Closesnapshots+clears the map beforeKillRunning, so a killed child'sonExitcan't double-clean. No leak either way. - Shared-manager correctness preserved —
RegisterToolscreates oneRuntimeand wiresTask(via the executor) andOutput/Stop(viaruntime.Manager) to the same instance. - Lazy —
Manager()/Close()never create background storage if no task used it. - Wired via
defer closeSpecialistRuntime(...)on both the interactive TUI and headless exec paths — so it fires on normal return and on Bubble Tea's Ctrl-C (which returns fromp.Run). - Tests pin it:
TestRuntimeCloseKillsRunningTasksAndCleansPromptFiles(assertsStatusKilled+ prompt file removed) andTestManagerKillRunningStopsOnlyRunningTasks.go test ./...,-race, andGOOS=windows buildgreen.
Approving. 👍
Non-blocking observation
A headless run that receives an uncaught SIGINT (e.g. zero exec with a background task, terminal Ctrl-C) won't run the defer (Go doesn't run defers on uncaught signals), so Close is skipped there. In practice the background child shares the process group and dies with the parent anyway, so the only residual is a prompt-temp-file leak on hard SIGINT. A signal handler that calls Close would close that last gap, but it's a minor edge — fine to leave or fold into a later pass.
🤖 Verified re-review via Claude Code (vet/-race/windows/full-suite run locally).
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Correcting my earlier approve — @Vasanthdev2004's race is real
Good catch by @Vasanthdev2004, and I have to walk back my approve: I traced the Close/KillRunning/prompt-file paths carefully but missed the kill-vs-onExit status race. Confirmed on the head:
background.UpdateStatus sets task.Status unconditionally (no terminal-state guard). The background child's wait goroutine (launchBackgroundProcess → onExit) calls manager.UpdateStatus(taskID, StatusError, exitCode) when the process exits non-zero. So on shutdown:
Runtime.Close()→KillRunning()→Killsets the task tokilled.- The killed process exits non-zero → the still-running
onExitfires →UpdateStatus(StatusError)overwriteskilled→error.
So TaskOutput/status reports a deliberate runtime-close/TaskStop as a failure. Same class as the #108 Kill TOCTOU, but on the onExit side.
Suggested fix
Make onExit not clobber a terminal state — e.g. a UpdateStatusIfRunning that only transitions when the current status is still StatusRunning (mirrors markKilledIfStillRunning). Then a kill sets killed, and the subsequent non-zero onExit is a no-op. A focused test: register → Kill/KillRunning → invoke the exit callback with a non-zero code → assert status stays killed.
The rest of the PR is solid (the lifecycle cleanup itself, prompt-file tracking, lazy runtime, defer wiring) — this is the one correctness blocker to close.
Non-blocking (also noted by @Vasanthdev2004)
output_tool.go still os.ReadFiles the whole output on every poll — a tail/limit can come later.
Apologies for the premature approve — requesting changes to reflect the real blocker.
🤖 Verified review via Claude Code.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Re-review — race fixed, approving
Preserve killed background task state (56dbdc6) closes the kill-vs-onExit race cleanly:
- New
MarkExited(taskID, status, exitCode)only transitions fromStatusRunning(if task.Status != StatusRunning { return nil }) and only acceptsCompleted/Error.onExitnow calls it instead of the unconditionalUpdateStatus. - So a
Kill/KillRunningsetskilled, and the killed child's subsequent non-zero exit is a no-op —killed(ExitCode -1) is preserved, andTaskOutputno longer misreports a deliberate stop as an error. - Normal completion still works (Running → Completed/Error via
MarkExited). - Tests pin it:
TestManagerMarkExitedDoesNotClobberKilledTaskandTestRuntimeClosePreservesKilledTaskAfterChildExit(Close kills → child-exit callback → status stayskilled).go test ./...,-race, andGOOS=windows buildgreen.
That resolves the blocker @Vasanthdev2004 caught (and that I missed on my first pass). Combined with the lifecycle cleanup itself, this is a complete, well-tested fix for the #115 background-lifecycle gap. Approving. 👍
The earlier non-blocking notes stand for a later pass (headless uncaught-SIGINT bypasses the defer; TaskOutput reads the full file per poll).
🤖 Verified re-review via Claude Code (vet/-race/windows/full-suite run locally).
BlockersNone found. Non-Blocking
Looks Good
Verdict: Approve — clean lifecycle hardening; the prior killed-status overwrite is resolved. |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approved after rereview: prior killed-status overwrite blocker is fixed and validation passed on PR head plus latest-main test merge.
3 participants
Summary:
Self-review:
Tests:
Summary by CodeRabbit
New Features
Improvements
Tests