Repository navigation
feat(lifecycle): allow an explicit restart drain grace (carry #6703) - #6724
Conversation
Separate the proposed two-second active/scoped-drain grace from the existing sixty-second cleanup watchdog and seventy-second replacement-readiness budget. Keep API and dashboard timing aligned, cover lifecycle timing and ownership with synthetic clocks, and document interrupted-execution uncertainty. Proposal for owner review of the shared lifecycle default; does not authorize automatic replay or guarantee a two-second recovery. Refs #6643
Signed-off-by: BigHulk <happyhls@gmail.com>
Signed-off-by: BigHulk <happyhls@gmail.com>
Signed-off-by: BigHulk <happyhls@gmail.com>
…ning the flush window The restart drain always started 200ms after acceptance so the 202 response could flush, which meant an explicit drainGraceMs below 200 cancelled active turns late. Arm a separate early-cut timer at acceptance + grace for those values; cleanup, listener stop and process exit still wait for the 200ms flush window, and a pending veto cancels the timer. Default and >=200ms grace arm nothing new. Co-authored-by: Hulk <happyhls@gmail.com>
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. |
|
✅ Deterministic PR hygiene checks passed. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (15)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe restart endpoint now accepts an optional ChangesRestart drain grace
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant ManagementClient
participant SystemRestartRoute
participant acceptSystemRestart
participant drainAndShutdown
participant ActiveTurns
ManagementClient->>SystemRestartRoute: POST /api/system/restart with optional drainGraceMs
SystemRestartRoute->>acceptSystemRestart: validated restart options
acceptSystemRestart->>drainAndShutdown: accepted grace and remaining drain time
drainAndShutdown->>ActiveTurns: wait until completion or grace expiry
drainAndShutdown->>ActiveTurns: abort active turns if short grace expires
Merge Risk: ⚪ Minimal · up to The restart endpoint gains an optional, validated drain-grace setting while the 60-second default is kept for everything else. No merge-blocking risk was identified. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation Issue Resolution Apply the Full details: Docstring CoverageExplanation Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 4 files. (11 skipped: 11 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
![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: c7ebc4f142
ℹ️ 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".
| if (!body || typeof body !== "object" || Array.isArray(body)) { | ||
| return jsonResponse({ success: false, error: "Invalid restart JSON body." }, 400, req, config); | ||
| } | ||
| const drainGraceMs = (body as Record<string, unknown>).drainGraceMs; |
There was a problem hiding this comment.
Reject keys other than drainGraceMs before accepting the restart. Currently a typo such as {"drainGraceMS":2000} passes the object check, is treated like {}, and immediately starts a restart with the 60-second default instead of returning the documented 400; because this is a mutating endpoint, silently ignoring the requested option can leave the caller waiting through an unintended drain. Add an explicit key whitelist and cover this case in the route tests so the implementation matches the public API contract.
AGENTS.md reference: docs-site/AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
…atchet (#6733) #6721 and #6724 each added one explicit registration and together took scripts/test-layout/layout.json to 2000 lines, over the 1999-line NEW_OVERSIZED ratchet, so file-size ratchet: repository fails on dev and on every PR's merge ref. Fold forty single-entry lines into four-per-line rows (the file's existing compact form). The parsed mapping is unchanged; the file drops to 1970 lines.
2 participants
Summary
Carries #6703 by @hulkbig with the one fix its review asked for. Closes #6643.
POST /api/system/restartaccepts an optional JSON body such as{"drainGraceMs":2000}to shorten how long the restart waits for active turns. Omitting the body or sending{}keeps the existing 60-second default. The value must be an integer from 1 to 60000; malformed JSON and invalid values are rejected with 400 before any drain starts. Only a management session or the admin token may set it — the target-bound local restart capability gets 403. The first accepted grace is fixed for repeated requests, the 60 s cleanup watchdog and 70 s replacement readiness budget are unchanged, and an interrupted turn is never replayed automatically. Dashboard, CLI, tray, join and automatic restarts keep their default; there is nogui/change. The English and seven localized API pages document the option.The fix on top of #6703: the drain always started 200 ms after acceptance so the 202 response could flush, so a grace below 200 ms (for example
{"drainGraceMs":1}) cancelled active turns at about 200 ms instead of at its accepted deadline. Shortening that delay would have letdrainAndShutdown's listener stop (server.stop(true)) or a supervised exit cut the 202 before it flushed. Instead, a grace below 200 ms now arms a separate timer at acceptance + grace that cancels and releases admitted turns only. Cleanup, listener stop and process exit still wait the full 200 ms, and a pending restart veto cancels the timer. Grace of 200 ms or more, including the default, arms nothing new.Co-authored-by: Hulk happyhls@gmail.com
Verification
On this head (
c7ebc4f142), macOS arm64, Bun 1.4.0:tests/server/system-restart-admission.test.ts: grace 1 ms and 199 ms cancel the active turn by the accepted deadline on both the standalone and supervised paths, with no drain, listener stop or exit before 200 ms; a veto cancels the early cut; grace 200, 60000 and the default arm no early cut. On the unmodified feat(lifecycle): allow an explicit restart drain grace #6703 headcc7cae71b5these fail (29 pass / 5 fail); with the fix, 34 pass / 0 fail.bun test tests/server/system-restart-admission.test.ts tests/server/system-restart.test.ts tests/service/shutdown-drain.test.ts tests/lib/system-restart-contract-security.test.ts— 87 pass.bun run typecheck, layout tests (18 pass),tests/ci-workflows/file-size-ratchet.test.ts(9 pass),tests/lab/core-lab-boundary.test.ts(25 pass),bun run structure:checkandgit diff --check— pass.cc7cae71b5; it is that head's evidence, not a rerun here. The full suite was not run locally; required hosted CI covers this head.Checklist
Summary by CodeRabbit