Repository navigation
fix(cli): bound server shutdown with a 10 s deadline - #58817
Open
kitlangton wants to merge 2 commits into
Open
kitlangton wants to merge 2 commits into
kitlangton wants to merge 2 commits into
Conversation
After SIGTERM or SIGINT, teardown had no deadline: a finalizer that never finished kept the process alive until it was SIGKILLed, with nothing in the log naming the step it hung in. - ShutdownDeadline gives teardown 10s, then logs and prints the layers still releasing and the child processes still running, and exits 1. - LayerNode records when each node starts and finishes releasing; the spawner records running children. - Closing a child's scope escalates to SIGKILL after 3s unless the command sets its own forceKillAfter, so a child that ignores SIGTERM or whose descendants hold its pipes can no longer block release forever.
…ed children - A second SIGTERM or SIGINT during teardown prints what is pending and exits 1 at once instead of being swallowed until the deadline. - Releasing a child's scope settles its pending entry, so a descendant that keeps the pipes open can't leave a released child listed as running.
Contributor
There was a problem hiding this comment.
Looks good. I reproduced #58816 on the base and confirmed it's fixed here:
- Child that ignores SIGTERM: the base was still alive 20 s after SIGTERM. With the PR, the new 3 s release force-kill ends it in about 4 s (exit 130).
- Finalizer that never finishes: the base was still alive after 20 s. With the PR it exits 1 after the 10 s deadline plus the log flush, and names
layer … releasingon stderr and in the error log. - Normal shutdown:
opencode servefrom source still exits within about 30 ms of SIGTERM.runMaincallsprocess.exitafter a signal, so the deadline timer doesn't hold the process open.
The new cli tests and the cross-spawn/layer-node tests in core pass.
Public API: unchanged. There are no changes to the SDK, HTTP API, config, CLI flags or env vars. RELEASE_FORCE_KILL_AFTER and @opencode/util/teardown are internal. The only behaviour change is the 3 s default SIGKILL escalation when a child's scope is released, which the description already covers.
No blocking concerns.
Related: #54267
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #58816
What it does
After SIGTERM or SIGINT, the server could hang in teardown forever, and the logs never named the step it was stuck on. Teardown now has a 10 s deadline. When the deadline passes, the process logs what is still pending and exits 1. A second signal during teardown does the same immediately.
How it works
ShutdownDeadline.install()in the CLI entry starts a timer on the first SIGTERM or SIGINT.runMainstill interrupts the main fiber as before. Startup order and listener shutdown are unchanged.shutdown deadline exceeded, waits 1.5 s so the file logger can flush, then exits 1. A second signal prints the same report and exits at once.Teardown.track/pendingin@opencode/util/teardown:LayerNode.compilebrackets each node's release, so a stuck layer shows up aslayer <name> releasing.child <pid> running: <command>.RELEASE_FORCE_KILL_AFTER) unless the command sets its ownforceKillAfter. Before, release sent SIGTERM and waited forever.Example report:
Risk
Proof
Reviewed by Argon before push (verdict: ship). Its two follow-ups are in the second commit: settle released children, and handle a second signal.
bun test --timeout 30000 test/shutdown-deadline.test.ts test/service-lifecycle.test.ts test/standalone.test.ts(packages/cli): pass. The new tests spawn a real server-shaped process with a hung layer and a child, then check the report, exit code 1, and that a second signal exits without waiting for the deadline.bun run script/test.ts test/effect/cross-spawn-spawner.test.ts test/effect/layer-node test/process/process.test.ts test/mcp.test.ts(packages/core): pass. The new spawner test checks that a released child whose grandchild holds its pipes is no longer listed as pending. It fails without the fix.bun typecheckin util, cli and core: pass.oxlinton the changed files: pass.Added by the opencode bot (AI-generated).
Related: #54267