Repository navigation
Conversation
A connection the predecessor accepted in the last instant before it closed could be reset when its request arrived after teardown began. The drain tracked only requests whose headers had been parsed, and the Bun listener dropped idle connections as soon as it closed, so a connection that had connected but not yet sent its request was reset. Track connections the listener accepts and, after the existing session and request wait, keep serving them for a bounded linger (500 ms) before the process closes them, then wait for a request that arrived during the linger to finish. The Bun adapter no longer drops idle connections on close; the finalizer still closes everything at the end. Measured before the fix: 4 resets in 1851 requests across ten systemctl restarts (0.22%), and 1 in 7376 under the handover order. The new test opens a connection, lets the server accept it, starts the drain, and sends the request afterwards; it fails without this change.
The handover is verified under a real systemd user session (systemd 261, Linger=yes) with a socket unit and a Type=notify service that needs NotifyAccess=all because the server child, not the unit's main pid, sends READY=1. The teardown race is fixed with a fails-before test, so the process-behavior note comes out of TODO.md and the roadmap row is dropped per the forward-looking roadmap convention.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 25 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e484ca2360
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // connections cannot hold shutdown. | ||
| yield* waitForAccepted.pipe(Effect.timeoutOption(drainLinger)) | ||
| // A request that arrived during the linger is now in flight; finish it before closing. | ||
| yield* waitForRequests.pipe(Effect.timeoutOption(grace)) |
There was a problem hiding this comment.
If a non-SSE request already in flight never finishes (for example, the partial-body request used by heldSession), untilIdle consumes the entire grace period, but this unconditional wait then grants that same request another full grace period. With the default configured in packages/cli/src/server-process.ts, shutdown now takes approximately 60.5 seconds instead of 30 seconds, rather than adding only the documented 500 ms linger. Preserve a shared deadline, or distinguish requests admitted during the linger, so requests that already exhausted their grace do not receive it again.
Useful? React with 👍 / 👎.
| }) | ||
| const waitForRequests = Effect.gen(function* () { |
There was a problem hiding this comment.
If an otherwise idle server receives a prompt on an accepted connection during the new linger, the prompt handler in packages/server/src/handlers/session.ts returns after admission and SessionExecution.wake, not after model execution finishes. Once that response closes its connection, both waitForAccepted and waitForRequests finish even though the session is still running; the remaining-session check only logs before teardown interrupts it. Consequently, a prompt successfully answered during handover gets at most the linger duration to execute rather than the configured session grace. Make the post-linger wait observe active sessions as well as HTTP responses, within the shutdown deadline.
AGENTS.md reference: AGENTS.md:L179-L179
Useful? React with 👍 / 👎.
Event streams are open-ended and the drain does not wait for them, so a connection that has already asked for one must not hold the accepted-connection linger either.
|
The following comment was made by an LLM, it may be inaccurate: |
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 72 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
|
🤖 Agent note · triage evidence (issue-sweep-rotating, run 2026-10-29 21:24 CDT) — for the queue lane owning task 2687 Discrepancy found this run: issue #81 is CLOSED (status:done) but this PR — which carries the verified fix — is still OPEN and the fix is not on main.
Suggested resume for the owning lane: merge origin/main into |
|
Stale-PR warden: no activity for 3 days. Resolution task: #1300 — this PR should be merged, fixed, or closed. |
Summary
The serve drain now keeps serving connections the listener already accepted before closing them, so a request that arrives after teardown began is answered instead of reset.
The Bun adapter no longer calls
closeIdleConnections()on close; accepted connections are served through a bounded linger (500 ms) and the finalizer'scloseAllConnections()still ends them. A secondSIGTERMcuts the whole wait.Evidence
Before: ten
systemctl --user restarthandovers of the upstream binary, fresh connection every 5 ms: 4ECONNRESETin 1851 requests (0.22%). The handover order alone (successor ready beforeSIGTERM) saw 1 in 7376.After: ten
systemctl --user restarthandovers of the fixed source: 0 resets in 2065 requests. Handover-order repro: 0 in 7115.Fails before, passes after:
bun test test/serve-handover.test.ts -t "a drain answers a request on a connection the server accepted before it began"Full
test/serve-handover.test.ts: 9 pass, 0 fail on both runtimes. Rootbun run typecheck: 36/36.Real systemd user session (systemd 261,
Linger=yes): a socket unit (ListenStream=127.0.0.1:4319) and aType=notifyservice whoseExecStartruns the launcher, so the server child — not the unit's main pid — sendsREADY=1;NotifyAccess=allaccepts it and each generation logsStarted+opencode: ready. Two generations under a 10 ms loop: 0 refused, 0 failed. The unit and transcript are on #81.Merge Danger
Door: two-way
Blast Radius: serve shutdown
The drain can now take up to one 500 ms linger longer when a connection is open at shutdown; a second
SIGTERMstill ends it immediately.Closes #81
Need help on this PR? Tag
@codesmith-botwith what you need.