fix(launcher): a supervised stop exits 0 — force lingering streams instead of failing - #277
Merged
vsits-proxy-builder[bot] merged 2 commits intoJul 31, 2026
Conversation
…stead of failing server.close() waits for in-flight requests, and a live Claude Code session always has one (the streaming /v1/messages response), so the graceful path alone never resolves — the 5s watchdog is the NORMAL exit under systemd, not the exception. Exiting 1 there made every `systemctl stop` log "status=1/FAILURE", which makes a crash and a clean stop indistinguishable in the journal and trips Restart=on-failure on a deliberate stop. The watchdog now force-closes lingering connections (closeAllConnections, Node >=18.2; older engines keep the prior behavior), reports the forcing on stderr, and exits 0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e stream exits 0 Spawns the real server, opens a request that never completes, sends SIGTERM, and asserts exit code 0 within the watchdog window. Against the pre-fix launcher this hangs on close and exits 1 — the exact journal FAILURE the fix removes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
vsits-codex-review-agent Bot
pushed a commit
to Gunther-Schulz/claude-code-cache-fix
that referenced
this pull request
Jul 31, 2026
![vsits-codex-review-agent[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
![vsits-codex-review-agent[bot]](https://avatars.githubusercontent.com/u/115404?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review: PR #277 graceful supervised stop
Date: 2026-07-31
Reviewed: PR #277 at 94363a1a7aaad43f2f28f56c1af1c4bdb7f73ca7
Round: 1
Label applied: approved-by-codex-agent
What Is Correct
- The shutdown premise is correct:
proxy/server.mjsroutes CLI shutdown throughactive.close(), which delegates toserver.close(), and Node keeps connections open while they are still sending a request or waiting for a response. That covers both an unfinished request body and the live/v1/messagesSSE response. - Calling
closeAllConnections()only afterserver.close()is the documented safe ordering. - The repo currently declares Node
>=18, documentsNode.js 18+, and tests majors18,20, and22. On 18.0/18.1 the feature-detect branch is reachable, but the supervisor-facing fix still lands because the watchdog logs and exits0; only the explicit pre-exit reap is unavailable there. - Exiting
0on a supervised stop is the right contract. A deliberateSIGTERMshould not look like a crash tosystemd, and stderr preserves visibility when the stop required a forced close. - The
handleMessages()abort path remains coherent: forced connection teardown will either fireclientReqclose and abort upstream, or the immediate process exit will tear the upstream down with the process. test/shutdown-exit-code.test.mjsmeaningfully exercises the watchdog path and locks the externally visible contract: delayed stop, stderr notice, and exit code0.
Blockers
None.
What Needs Attention
- The new regression test uses an unfinished request body rather than a live SSE response. That still proves the watchdog path and the
server.close()behavior this fix depends on, but it leaves the exact mid-stream response shape covered by reasoning rather than by a dedicated test.
Bloat / Non-Functional
None.
Recommendations
- If this area changes again, keep the test contract black-box and supervisor-facing.
- If maintainers later need a machine-readable distinction between "drained" and "forced after 5s", add telemetry rather than reusing process exit codes.
Bottom Line
This fixes a real supervisor contract bug without disturbing the embeddable API or the request pipeline. The reasoning is sound, the fallback behavior is acceptable on the repo's declared Node floor, and the regression test locks the intended stop semantics in place. Approved. — Codex review
vsits-codex-review-agent
Bot
added
reviewed-by-codex-agent
vsits-proxy-builder
Bot
dismissed
vsits-codex-review-agent[bot]’s stale review
via
July 31, 2026 15:47
c814fd2
vsits-proxy-builder
Bot
force-pushed
the
pr/graceful-stop
branch
from
July 31, 2026 15:47
94363a1 to
c814fd2
Compare
Contributor
There was a problem hiding this comment.
Codex review: re-submitting approval after the fork branch was reverted to remove my docs-only review artifact commit.
I previously reviewed and approved this PR at 94363a16. I confirmed the current head c814fd2f differs only by removal of docs/code-reviews/pr-277-round-1-codex.md; there is no code change.
vsits-codex-review-agent
Bot
added
reviewed-by-codex-agent
vsits-proxy-builder Bot
added a commit
that referenced
this pull request
Jul 31, 2026
…286) The artifact rule said "PR review -> on the PR branch" without qualifying who owns it, so reviewing a community PR meant pushing to the contributor's branch. That happened on #274 and #277 this week; both were reverted. maintainerCanModify makes the push possible; it is not permission. It also backfires mechanically: this repo dismisses stale reviews on push, so the review-doc commit dismisses the approval it documents and staleness-flags the approval label against the new head. Fork PRs now have no committed artifact — the formal review body is the artifact. Closes #286
cnighswonger
pushed a commit
that referenced
this pull request
Jul 31, 2026
The global reviewer bar is "larger than the directive's requirements justify." Community PRs have no directive, so the bar had nothing to anchor to: across eight open community PRs every review reported "Bloat: None", including one on 6,630 lines of new production code. AGENTS.md now anchors no-directive PRs to the defect being fixed, and requires the size numbers be stated in every review — a number is checkable, "None" is not. Calibrated against #274/#277/#261, all merged and all proportionate, so the reference is real work rather than a guess. Explicitly excludes test volume and why-comments from the finding, since both are high here by design. CONTRIBUTING.md is new: AGENTS.md and CLAUDE.md were already committed but are addressed to our own agents, so contributors had no file that spoke to them. Points them at both, and asks PRs over ~300 production LOC to carry the non-functional checklist. Claude-Session: https://claude.ai/code/session_01GvZKP1JeXgHFCovTaAPT5B Co-authored-by: vsits-proxy-builder[bot] <223447982+vsits-proxy-builder[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
server.close()waits for in-flight requests — and a live Claude Code session always has one (the streaming/v1/messagesresponse). So the graceful path never resolves on its own and the 5s watchdog is the normal exit under a supervisor, not the exception. Exiting 1 there means everysystemctl stop:status=1/FAILURE, making a crash and a clean stop indistinguishable in the journal, andRestart=on-failureon a deliberate stop.The watchdog now force-closes lingering connections (
closeAllConnections, Node ≥18.2; on the 18.0/18.1 floor the prior behavior is the only option), reports the forcing on stderr, and exits 0. Observed across two days of restarts under systemd on our machines: every stop previously logged FAILURE; with this patch, cleanstatus=0.Independent of the #272–#276 series (found while operating them, touches only the shutdown path).
🤖 Generated with Claude Code