Harden OpenCode probe teardown to avoid leaked processes#2657
Harden OpenCode probe teardown to avoid leaked processes#2657juliusmarminge wants to merge 1 commit into
Conversation
- keep successful provider refreshes when probe scope teardown fails - terminate stray matching `opencode serve` processes alongside the child
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Failable
killDirectChildshort-circuits SIGKILL cleanup phase- Changed
Effect.asVoidtoEffect.ignoreinkillDirectChild(and the Windows path ofkillOpenCodeProcessGroup) so that errors fromchild.killon already-exited processes are suppressed, allowing the full SIGTERM-then-SIGKILL kill chain to complete.
- Changed
Or push these changes by commenting:
@cursor push b3ce952ec9
Preview (b3ce952ec9)
diff --git a/apps/server/src/provider/opencodeRuntime.ts b/apps/server/src/provider/opencodeRuntime.ts
--- a/apps/server/src/provider/opencodeRuntime.ts
+++ b/apps/server/src/provider/opencodeRuntime.ts
@@ -450,7 +450,7 @@
const killOpenCodeProcessGroup = (signal: NodeJS.Signals) =>
process.platform === "win32"
- ? child.kill({ killSignal: signal, forceKillAfter: "1 second" }).pipe(Effect.asVoid)
+ ? child.kill({ killSignal: signal, forceKillAfter: "1 second" }).pipe(Effect.ignore)
: Effect.sync(() => {
try {
process.kill(-Number(child.pid), signal);
@@ -461,7 +461,7 @@
}
});
const killDirectChild = (signal: NodeJS.Signals) =>
- child.kill({ killSignal: signal, forceKillAfter: "1 second" }).pipe(Effect.asVoid);
+ child.kill({ killSignal: signal, forceKillAfter: "1 second" }).pipe(Effect.ignore);
const killMatchingServeProcesses = (signal: NodeJS.Signals) =>
terminateMatchingOpenCodeServeProcesses({
binaryPath: input.binaryPath,You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit b538723. Configure here.
| }); | ||
| const terminateChild = killOpenCodeProcessGroup("SIGTERM").pipe( | ||
| const killDirectChild = (signal: NodeJS.Signals) => | ||
| child.kill({ killSignal: signal, forceKillAfter: "1 second" }).pipe(Effect.asVoid); |
There was a problem hiding this comment.
Failable killDirectChild short-circuits SIGKILL cleanup phase
High Severity
killDirectChild uses Effect.asVoid (which only discards the success value) instead of Effect.ignore (which also suppresses errors). Since child.kill from @effect/platform is a failable effect, it can produce a PlatformError (e.g., when the child already exited). When it fails inside Effect.all, the entire SIGTERM phase fails, which short-circuits Effect.andThen so the SIGKILL phase never runs. The trailing Effect.ignore on the pipeline only suppresses the overall error — it doesn't undo the short-circuit. This means killOpenCodeProcessGroup("SIGKILL") and killMatchingServeProcesses("SIGKILL") are skipped, potentially leaving orphaned serve processes alive — the exact problem this PR aims to fix.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b538723. Configure here.
ApprovabilityVerdict: Needs human review This PR introduces significant changes to process termination logic with new system-level operations. An unresolved high-severity review comment identifies a bug where improper error handling in You can customize Macroscope's approvability policy. Learn more. |



Summary
serveprocesses, in addition to the existing process-group cleanup.Testing
bun fmt)bun lint)bun typecheck)bun run test)Note
Medium Risk
Touches OpenCode probe lifecycle and subprocess termination logic; mistakes could cause lingering processes or over-aggressive kills on Unix platforms.
Overview
Wraps the OpenCode provider inventory probe with
scopedSafeTeardownso finalizer/cleanup errors during scope close no longer mask a successful refresh.Hardens OpenCode runtime shutdown by, on non-Windows platforms, additionally killing the direct spawned child and any matching
opencode serve --hostname=... --port=...processes (viapsscan) alongside the existing process-group kill.Adds a regression test ensuring
checkOpenCodeProviderStatusstill returns areadysnapshot even if probe teardown throws after inventory loads.Reviewed by Cursor Bugbot for commit b538723. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Harden OpenCode probe teardown to avoid leaked processes on scope finalization
scopedSafeTeardownso that teardown/finalizer errors are suppressed and a successful provider refresh result is preserved even if scope cleanup throws.listProcessCommands,terminateMatchingOpenCodeServeProcesses) to opencodeRuntime.ts that identify and signal stray OpenCodeserveprocesses by matching binary path, hostname, and port.runOpenCodeCommandfinalizer to concurrently send SIGTERM to the process group, the direct child, and any matching serve processes; waits 1 second; then sends SIGKILL to all three.Macroscope summarized b538723.