Repository navigation
fix(daemon): keep idle reap closed during shutdown - #4167
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Size Report
Startup median (7 runs, lower is better):
|
thymikee
left a comment
There was a problem hiding this comment.
Independent review: no actionable findings in the requested scope.
Reviewed head b4e74e66f6ce37de044b1ac26965c6207de0ac57; merge-base a0e7c7dc239922bf93adaceff20a2f7e78579146.
- Traced all four production
noteActivity()sites: startup, request completion (including explicit session close), lease-end completion, and idle session expiry. The controller owns its terminalopen/closedphase. Runtime close and startup rollback useclose(); ordinary request admission retains reversiblecancel(). Late completions cannot re-arm after close. - Checked SIGTERM, accepted quiesce, and the reap's own callback against both event orders. They converge on the existing shared close promise. Admission and reap close synchronously before the first shutdown await. A queued timer observes the closed phase; a timer already executing reaches close synchronously, so it cannot overlap another event-loop close between its idle check and close. Retained-lease expiry can schedule asynchronous lease-end completion, which encounters the same terminal guard. Quiesce's deferred drain can race the timer without creating a second shutdown.
lifecycleAdmission.close()now owns ordinary-request refusal for every close source; removingshuttingDownremoves the competing admission flag. A correctly authenticated and fenced detached-daemon quiesce during SIGTERM drain returns the existing{ outcome: 'draining' }success even while requests remain. Authentication, instance fencing, remote-origin refusal, and embedded ownership checks remain ahead of that answer. The daemon client maps success to its typeddrainingoutcome and waits for exit; the CLI uses that ensuring-client path. Busy refusals remain classified by typed reasons rather than message text.- Design: the two-state reap controller plus the existing admission owner are sufficient. The runtime changes are limited to wiring those owners and removing the duplicate flag; no broader lifecycle state machine is needed.
Validation: setup/build passed. Ran the four affected daemon files once with pnpm exec vitest run --project unit-core packages/daemon/src/sessions/server/daemon-idle-reap.test.ts packages/daemon/src/sessions/server/daemon-idle-reap-lifecycle.test.ts packages/daemon/src/server/http/daemon-runtime-idle-reap.test.ts packages/daemon/src/server/http/daemon-runtime-quiesce.test.ts: 43 passed, 4 files passed. Reviewed checkout remains clean at the pinned SHA. No CI wait or live-device run.
Base sensitivity: source inspection confirms the first new assertion detects request-finally re-arming after base's reversible idleReap.cancel(), and the second detects base leaving admitting open during external close and therefore returning daemon-busy for the held request. The PR reports both failing on a tests-only base patch; I did not independently execute that base comparison under the one-run limit. The PR's reported broader validation blocker #4060 remains separate from this focused pass.
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Apex SentinelVerdict: 2 finding(s): 2 nit. Verdict: approve. reached max rounds (1). 🔵 Nit (2)
Reply under an inline comment with |
There was a problem hiding this comment.
Apex Sentinel: 2 new findings on b4e74e6. The verdict and the full list are in the summary comment.
|
The code looks good at b4e74e6. Idle reap now stays closed during shutdown, and I found no correctness problems. Not blocking, and you can take or leave these: ADR 0006 (Daemon capabilities and quiesce) says a quiesce that finds work refuses with daemon-busy and reopens admission, but a daemon whose runtime close has already started now answers draining without a census, so one sentence in the ADR (and optionally website/docs/docs/commands.md:91) would match the code; and in daemon-idle-reap.ts the phase check at :151 and the early return at :163 cannot fire, because close() sets the phase and clears the timer synchronously and schedule() already refuses at :145, so keeping only the :145 check would be simpler. The Smoke Tests check is still queued, not failed. The diff touches its route (admission, idle-reap cancel and noteActivity in handleRequest, and closeSteps), so it needs to finish green on b4e74e6 before merge. The #4060 failure in apple-platform-output-guard.test.ts:385 looks unrelated, since the diff does not touch recording, and it still stops a passing exact-head check:affected. I did not run the two regression tests. I judged the base failure and head pass by reading the pre-change path, including the fake-timer ordering in the first test. The gate counts, the check:affected receipt, the live SIGTERM run and the local smoke result (76 passed) come from the PR body, and I did not reproduce them. On the existing threads: the cubic P3 about the admit doc at daemon-idle-reap.ts:187-191 still applies, because close() at :273 now also closes admission, and the author accepted it as a doc follow-up (#4167 (comment)). The two style and wording threads do not apply, since they have no behavior effect, and they are already resolved: #4167 (comment) and #4167 (comment). |
Summary
Prevent request completion and lease-end activity during shutdown from restarting idle reap. The timer owns a terminal closed phase. Lifecycle admission now closes for every shutdown, so quiesce reports
drainingduring a SIGTERM drain.Three files; no scope expansion. FU-REAP audit task; no separate implementation issue.
Validation
a0e7c7dc239922bf93adaceff20a2f7e78579146; head:b4e74e66f6ce37de044b1ac26965c6207de0ac57.pnpm exec vitest run --project unit-core packages/daemon/src/server/http/daemon-runtime-quiesce.test.ts -t 'request completion after close|quiesce during an external close'. Tests-only patch on base: 2 failed (one reap;daemon-busy). Same command on head: 2 passed. Four focused files: 43 passed.pnpm gate unit: 16,967 passed, 1 skipped; smoke 76 passed, 6 skipped; temporary-directory guard passed.pnpm formatpassed.pnpm check:affected --run --head HEADfailed. Receipt.tmp/validation/run-H4U4HJ/receipt.json:exactCommit=true,checkoutChanged=false; format, lint, typecheck, layering, Fallow, build passed.apple-platform-output-guard.test.ts:385,recordexceeded 20 seconds. Cause remains unproven.1def13f07b(production identical to head): private-state daemon, inventory request canceled by SIGTERM, exit 0. Temporary timer-observer log: SIGTERM09:22:01.427, exit09:22:06.447UTC; zero re-arms/fires during the 25 ms idle window's drain. Scratch removed.Cold self-review only. CI Integration Tests/Coverage remain authoritative. Ready for review as requested; not merge-ready while #4060 blocks validation.