Repository navigation
Conversation
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
|
The following comment was made by an LLM, it may be inaccurate: |
|
/devin review |
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
goleary
left a comment
There was a problem hiding this comment.
independent review: clean, 94/100 for 6510616.
three fresh reviewers covered interface/naming, process lifecycle, and tests/operations on 34f7644; none found an actionable defect in the changed production path. the final commit only replaces the test's external node dependency with its existing process.execPath helper. parent verification: all 24 shell tests pass on pinned bun 1.3.14; the revised regression fails at 10 seconds with the production option removed. pre-push typechecks passed across 30 tasks.
| path | before | after |
|---|---|---|
| scope interruption | term-resistant process can hold cleanup indefinitely | uses the existing three-second escalation grace period |
| explicit abort/timeout | bounded kill | same behavior |
no new abstraction, caller contract, permission change, or speculative handling was needed. security and ux lanes were omitted because this diff changes internal process termination only.
devin's test-portability finding is fixed. the descendant-survival finding reproduces both before and after this change and is documented in the description and review thread; full process-group cleanup is outside this fix. the original customer stall's root cause remains unproven.
verification limits: windows was inspected, not executed. latest linux checks are pending; the prior fork-ci suite passed, while the broader unit job failed in a separate pty environment test. additional independent validators could not start because of a usage limit, so the two devin findings were checked empirically by the parent. this is a code-review verdict, not a declaration that ci is green or approval to merge.
|
the linked team issue is REPL-31317: https://linear.app/replo/issue/REPL-31317. keeping it open because this fixes a separately reproduced cleanup hang, not the entire reported watchdog issue; a github closes/fixes reference would misstate that scope. |
|
pausing this pr pending production evidence. i am not recommending merge or release based on the local reproduction alone. |
|
closing this without merging. the local regression demonstrates a real cancellation/cleanup edge case, but we have not established that customers are encountering it or that it caused the stalled turn this investigation started from. passing tests and code review show that the patch addresses the reproduction; they do not establish that this is the right fix for the reported issue. that is not enough justification for adding another behavior change to our opencode fork and increasing the work needed to stay aligned with upstream. the original investigation remains open in REPL-31317. further work should identify the failure in an affected session and establish its cause before changing replopencode. this patch can be reconsidered if that evidence points to this cleanup path. no release or downstream version pin was changed. the branch and regression test remain available as investigation material, and the pr babysitting automation is paused. |
Cancelling an agent shell command can hang if the command ignores termination. This gives shell cleanup a five-second grace period before forced termination, shared with explicit timeout and abort handling.
Found while investigating REPL-31317. The September 16 customer session ran OpenCode 1.17.14-15 and left a browser-test tool running after producing output. There is no confirmed customer incident attributed to this cleanup bug. This PR fixes a separately reproduced interruption/cleanup hang; it does not establish the cause of that customer's 19-minute stall or close the ticket.
ShellToolpassedforceKillAfterto explicithandle.kill()calls but omitted it from the command options used by the spawner's scope finalizer. Interrupting the tool fiber bypasses those explicit handlers, so a process ignoring SIGTERM could hold cleanup indefinitely. Both shell command constructors now supply the grace period, allowing cleanup to escalate to SIGKILL. The shared grace period increases from three to five seconds for scope cleanup, explicit abort, and timeout. The default two-minute command timeout is unchanged.Testing Done
bun typecheckpasses inpackages/opencode.The original fork CI passed on Linux; CI is rerunning after the five-second grace-period adjustment. The new POSIX signal test skips Windows. This is an internal process-lifecycle change with no canary contract changes. Shipping requires a fork release and fleet pin update after review; this PR does not change fleet pins.
Known limitation: if the wrapper shell exits after SIGTERM while a descendant ignores it and redirects output, the existing spawner can leave that descendant alive. A direct spawner probe reproduces this both without
forceKillAfter(37 ms cleanup) and with the earlier three-second option (24 ms). This PR bounds waiting for cleanup; it does not guarantee that every descendant is gone. Review discussion.