Skip to content

fix(opencode): bound shell cleanup on interruption - #49

Closed
goleary wants to merge 3 commits into
devfrom
codex/bash-timeout
Closed

goleary wants to merge 3 commits into
devfrom
codex/bash-timeout

Conversation

@goleary

@goleary goleary commented Sep 17, 2026 •

Copy link
Copy Markdown

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.

ShellTool passed forceKillAfter to explicit handle.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

  • A real-process regression waits for a SIGTERM-resistant command to announce readiness, then interrupts the tool fiber. The original regression exceeded its 10-second deadline without the fix. With the requested five-second grace period, it finishes in about 5.3 seconds; its deadline is now 15 seconds, below the child’s 20-second natural lifetime.
  • All 24 shell tests pass on macOS with Bun 1.3.14 (the fork's pinned runtime) after the five-second adjustment. The earlier three-second version also passed on Bun 1.4.0.
  • bun typecheck passes in packages/opencode.
  • Prettier passes; targeted oxlint reports zero errors and 10 warnings on existing code.

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.

@github-actions

Copy link
Copy Markdown

Thanks for your contribution!

This PR doesn't have a linked issue. All PRs must reference an existing issue.

Please:

  1. Open an issue describing the bug/feature (if one doesn't exist)
  2. Add Fixes #<number> or Closes #<number> to this PR description

See CONTRIBUTING.md for details.

@github-actions

Copy link
Copy Markdown

The following comment was made by an LLM, it may be inaccurate:

@goleary

goleary commented Sep 17, 2026

Copy link
Copy Markdown
Author

/devin review

@devin-ai-integration

Copy link
Copy Markdown

Starting Devin Review.

Devin Review

@goleary
goleary marked this pull request as ready for review September 17, 2026 21:49

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 potential issues.

Devin Review

Comment thread packages/opencode/src/tool/shell.ts Resolved

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread packages/opencode/src/tool/shell.ts Resolved

@goleary goleary left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@goleary

goleary commented Sep 17, 2026

Copy link
Copy Markdown
Author

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.

@goleary

goleary commented Sep 17, 2026

Copy link
Copy Markdown
Author

pausing this pr pending production evidence. i am not recommending merge or release based on the local reproduction alone.

@goleary

goleary commented Sep 17, 2026

Copy link
Copy Markdown
Author

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.

@goleary goleary closed this Sep 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant