Skip to content

fix(daemon): keep idle reap closed during shutdown - #4167

Merged
thymikee merged 1 commit into
mainfrom
fix/idle-reap-closed-state
Nov 2, 2026
Merged

thymikee merged 1 commit into
mainfrom
fix/idle-reap-closed-state

Conversation

@thymikee

@thymikee thymikee commented Nov 2, 2026 •

Copy link
Copy Markdown
Member

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 draining during a SIGTERM drain.

Three files; no scope expansion. FU-REAP audit task; no separate implementation issue.

Validation

  • Base: a0e7c7dc239922bf93adaceff20a2f7e78579146; head: b4e74e66f6ce37de044b1ac26965c6207de0ac57.
  • Regression command: 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 format passed. pnpm check:affected --run --head HEAD failed. Receipt .tmp/validation/run-H4U4HJ/receipt.json: exactCommit=true, checkoutChanged=false; format, lint, typecheck, layering, Fallow, build passed.
  • Base blocker #4060: identical related-test command on unchanged base and head yielded 303 passed / 1 failed, apple-platform-output-guard.test.ts:385, record exceeded 20 seconds. Cause remains unproven.
  • Live at 1def13f07b (production identical to head): private-state daemon, inventory request canceled by SIGTERM, exit 0. Temporary timer-observer log: SIGTERM 09:22:01.427, exit 09:22:06.447 UTC; 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.

View guided diff Turn on auto-fix

@vercel

vercel Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
agent-device-website Ready Ready Preview Nov 2, 2026 10:02am UTC

Request Review

@github-actions

github-actions Bot commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.69 MB 5.69 MB +86 B
Package (unpacked) 5.69 MB 5.69 MB +86 B
Package (download) 1.73 MB 1.73 MB +28 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 36.0 ms 35.5 ms -0.5 ms
CLI --help 75.4 ms 76.0 ms +0.7 ms

@thymikee thymikee left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 terminal open/closed phase. Runtime close and startup rollback use close(); ordinary request admission retains reversible cancel(). 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; removing shuttingDown removes 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 typed draining outcome 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.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-sentinel-staging

Copy link
Copy Markdown

Apex Sentinel

Verdict: approve

2 finding(s): 2 nit. Verdict: approve. reached max rounds (1).

🔵 Nit (2)
Where Finding
packages/daemon/src/sessions/server/daemon-idle-reap.ts:168 Inline close breaks the file's named-const pattern for controller methods
packages/daemon/src/sessions/server/daemon-idle-reap.ts:184 Type-level doc's close() is now ambiguous next to the new close member

Reply under an inline comment with @apex-sentinel-staging note ... or @apex-sentinel-staging pivot ... to re-check that finding.
Comment @apex-sentinel-staging review on this conversation to run a full review.

@apex-sentinel-staging apex-sentinel-staging Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apex Sentinel: 2 new findings on b4e74e6. The verdict and the full list are in the summary comment.

@thymikee

thymikee commented Nov 2, 2026

Copy link
Copy Markdown
Member Author

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).

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Nov 2, 2026
@thymikee
thymikee merged commit ec7e37b into main Nov 2, 2026
19 of 20 checks passed
@thymikee
thymikee deleted the fix/idle-reap-closed-state branch November 2, 2026 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant