Skip to content

feat(daemon): quiesce request that retires an idle daemon atomically - #3764

Merged
thymikee merged 1 commit into
mainfrom
feat/ensure-daemon
Oct 17, 2026
Merged

thymikee merged 1 commit into
mainfrom
feat/ensure-daemon

Conversation

@thymikee

@thymikee thymikee commented Oct 16, 2026 •

Copy link
Copy Markdown
Member

Summary

Layer 1 of 3 for #3754 (umbrella #3747): the daemon half of replacing an incompatible daemon safely.

  • A daemon now answers daemon_quiesce, authenticated with its token and fenced to its pid and start time. It closes admission and counts in-flight requests, sessions, repair sessions and live leases in one synchronous step. Any of them refuses with daemon-busy and reopens admission; otherwise it answers draining and exits.
  • A remote-origin request is refused. The request is answered before dispatch, so it works on the socket and HTTP transports.
  • The daemon publishes capabilities: ['quiesce'] in daemon.json; a daemon that predates it omits the field.
  • Shared vocabulary lives in daemon-resolution.ts, and the daemon side in daemon-idle-reap.ts, because both are already in the eager closures. ADR 0006 gains a short section, including what the census does not count (uploads, lease-less holds).

No client behavior changes here. 10 files. The last PR in the stack closes #3754.

Validation

pnpm check:affected --run --head ad938223f9: passed (format, lint, typecheck, layering, fallow, build, vitest-related). New runtime tests boot the real daemon and quiesce it over its socket and HTTP: idle drain, busy refusal per cause with admission reopened, wrong token, pid and start-time fences, remote-origin refusal.

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.32 MB 5.32 MB +1.9 kB
Package (unpacked) 5.32 MB 5.32 MB +1.9 kB
Package (download) 1.60 MB 1.60 MB +640 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 26.0 ms 27.7 ms +1.7 ms
CLI --help 76.3 ms 79.2 ms +3.0 ms

@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 8 files

You’re at about 92% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.

Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic

Comment thread packages/daemon/src/server/http/daemon-runtime.ts
Comment thread docs/adr/0006-daemon-rpc-protocol-version.md Outdated
Comment thread packages/daemon-contracts/src/daemon-resolution.ts
Comment thread packages/daemon/src/server/http/daemon-runtime-quiesce.test.ts Outdated
Comment thread packages/daemon-contracts/src/daemon-resolution.ts Outdated
@thymikee

Copy link
Copy Markdown
Member Author

The code needs one fix before merge: a quiesce that arrives during daemon startup can crash the daemon. Reviewed commit a972b47. All 17 checks pass. I did not run the tests. I found this by reading the code, not by reproducing it.

In daemon-runtime.ts, drain closes over const shutdown, which is declared near line 852. The servers start listening near line 805, and daemon.json is published with the token and pid before that. Startup then still awaits the diagnostics flush and device-claim reconciliation. If an idle quiesce lands in that window, the daemon answers draining. Then setImmediate(params.drain) reads shutdown before it is initialized and throws a ReferenceError from a timer. The SIGTERM and uncaughtException handlers are registered only near line 932, so nothing catches it. This is the mixed-version replace race #3754 targets: a client that reads daemon.json just after another process starts the daemon gets draining, and the daemon exits with code 1. It skips registration cleanup, lock cleanup and lease finalization, so daemon.json and the lock stay behind. The earlier "Addressed" reply does not hold, because deferring the call moved the throw out of the request and made it uncaught. The rule to meet: the daemon answers draining only when it can run its drain path. Either create the shutdown operation before the servers listen, or count "startup not complete" as a held census entry until idleReap.noteActivity() is armed, as idle reap already does. Please add a runtime test that sends a quiesce while reconcile is held and asserts a busy refusal, or a clean exit 0 with daemon.json removed.

Not blocking, take or leave: the ADR at docs/adr/0006-daemon-rpc-protocol-version.md line 104 says a null start time is rejected, but the code accepts it when the daemon cannot read its own, so either reject null or state that the token is the instance fence and the start time is checked only when known; and SessionStore.assertAdmissionOpen in session-store.ts still hardcodes 'daemon_shutting_down' instead of using the new DAEMON_SHUTTING_DOWN_REASON.

Could the runtime own one admission state, open | quiesced | shutting-down, that replaces shuttingDown and is the only thing handleRequest checks? daemon-idle-reap.ts would then export only a pure readQuiesceCensus(sessionStore, leaseRegistry, inFlight) next to isDaemonIdle. The runtime would handle the quiesce command itself: check the token and fence, move open to quiesced, read the census, then revert or start the drain. With the shutdown operation built before the servers listen, or "starting" counted as held, a draining answer always has a live drain path. That removes the admit(handler) wrapper and the third admission flag, and the TDZ goes away by construction. The pid and start-time fence could also go, since the random per-instance token already identifies the instance, though the issue asks for the fence, so that part is optional. The first step would be to hoist the shutdown construction, or add a startup-complete flag, so it exists before listen. No ADR change is needed beyond the existing 0006 section.

On the open review threads: the drain/TDZ thread is marked resolved but still applies, and it is the finding above. The null start time thread and the hardcoded reason thread still apply and match the non-blocking notes. Three lower-priority threads also still apply, and I found none that should be resolved as stale.

No conflicts. Live device validation is not needed here, since the change is daemon lifecycle only and the tests boot the real runtime. Before merge, make draining possible only once the drain path exists, and add the startup-window test.

@thymikee
thymikee force-pushed the feat/ensure-daemon branch 2 times, most recently from d80f096 to 415e080 Compare October 17, 2026 00:05

@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 2 files (changes from recent commits).

You’re at about 94% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.

Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic

Comment thread packages/daemon/src/server/http/daemon-runtime-quiesce.test.ts Outdated
@thymikee
thymikee force-pushed the feat/ensure-daemon branch 2 times, most recently from 1bacb95 to edd4335 Compare October 17, 2026 01:33
@thymikee

Copy link
Copy Markdown
Member Author

Thanks, you were right: the first TDZ fix only moved the throw. Fixed in edd4335.

  • Startup window: startup now counts as work in flight, so a quiesce that arrives before the drain path exists is refused daemon-busy (inFlightRequests: 1) and admission stays open. draining is only ever answered once shutdown exists. The deferred-drain promise is gone.
  • Test: daemon-runtime-quiesce.test.ts holds startup after daemon.json is published, asserts the busy refusal, then quiesces after startup and asserts a clean exit 0 with daemon.json removed. It fails with the TDZ ReferenceError if startup is not counted.
  • Null start time: a daemon that cannot prove its start time now refuses every quiesce (daemon-instance-mismatch), and ADR 0006 says so. Unit test in daemon-idle-reap.test.ts.
  • Hardcoded reason: session-store.ts keeps the literal because importing the constant there grows nine eager closures (the closure-budget test fails). session-store-lifetime.test.ts now asserts its reason against DAEMON_SHUTTING_DOWN_REASON, so they cannot drift.
  • Single admission state: I did not take it. Inlining the quiesce handling in startDaemonRuntime breaks its fallow complexity ceiling, and the admit wrapper keeps the verdict unit-testable beside isDaemonIdle. With startup counted as held, the TDZ is gone either way. Happy to revisit if you want the admission enum as a follow-up.

@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 2 files (changes from recent commits).

You’re at about 95% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.

Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic

Comment thread packages/daemon/src/server/http/daemon-runtime-quiesce.test.ts Outdated
@thymikee

Copy link
Copy Markdown
Member Author

The PR is ready. I reviewed 364af8c, and the startup-window crash from the earlier review is fixed and covered by a runtime regression test. CI is green: 17 of 17 checks pass at 364af8c, with no failing job. There are no conflicts. Nothing else must happen before a maintainer merges.

Not blocking, take or leave: at startup the census reports inFlightRequests: 1 when no request is in flight (https://github.com/callstack/agent-device/blob/364af8c/packages/daemon/src/server/http/daemon-runtime.ts#L640), so a separate census field or reason detail would be clearer. Also, the new ADR sentence says the request "names it with the pid", but the code also requires the start time to match (https://github.com/callstack/agent-device/blob/364af8c/docs/adr/0006-daemon-rpc-protocol-version.md#L104), so "the pid and that start time" would be accurate, and the line needs the same wrapping as the rest of the ADR.

I did not run the new runtime test or the eager-closure-budget test. I judged the regression coverage from the diff.

These earlier inline threads are fixed at this head, so you can resolve them: startup counted in the census (#3764 (comment)), refusal when the start time is null (#3764 (comment)), in-flight test cleanup (#3764 (comment)), reworded doc comment (#3764 (comment)), bounded daemon.json poll (#3764 (comment)), and the corrected test comment (#3764 (comment)). The literal in session-store.ts is intentional, because the lifetime test asserts it equals DAEMON_SHUTTING_DOWN_REASON, so this thread can be resolved too (#3764 (comment)).

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 17, 2026
A daemon now answers an authenticated, instance-fenced daemon_quiesce request: it closes admission and
counts requests in flight, sessions, repair sessions and live leases in one synchronous step, refuses
with daemon-busy (reopening admission) if it holds any, and otherwise drains and exits. It publishes
capabilities in daemon.json so a client can tell a daemon that can quiesce from one that predates it.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@thymikee
thymikee added this pull request to stack #3774 October 17, 2026 05:48
@thymikee

Copy link
Copy Markdown
Member Author

Thanks. Both non-blocking notes are taken in ad93822:

  • Startup census: the census now has its own startingUp field instead of counting startup as an in-flight request. A quiesce during startup is refused daemon-busy with startingUp: true, inFlightRequests: 0, the client reads the field (and the hint says the daemon is still starting), and the runtime test asserts it.
  • ADR 0006: the sentence now says the request names both the pid and that start time, and is wrapped like the rest. It also lists startingUp among what the daemon counts.

pnpm check:affected --run passes at ad93822 (it needed one rerun: the first run hit three process-timing unit tests that time out under host load and pass alone). CI is re-running on this head.

@thymikee

Copy link
Copy Markdown
Member Author

The delta at ad93822 fixes what the earlier review at 364af8c left open. The close-count-reopen step stays atomic, the daemon still refuses while it starts up, and the startingUp: true / inFlightRequests: 0 assertion would fail on 364af8c, so the new test guards the fix. I did not run the tests; I read the diff. All 17 checks pass at ad93822, and there are no conflicts.

Not blocking, and you can take or leave it: the comment at src/daemon-client/daemon-client-metadata.ts#L55 says the client reads startingUp and that the hint says the daemon is still starting. No client code reads that census at this head, and the busy refusal at https://github.com/callstack/agent-device/blob/ad93822/packages/daemon/src/core/daemon-idle-reap.ts#L210-L214 has no startup hint. Please fix the comment, or add the client read and the hint in the client PR of the stack that closes #3754.

On the open threads: the cubic-dev-ai thread about the single-source reason string in session-store.ts (#3764 (comment)) still applies as a note, not a defect, since the test pins it to DAEMON_SHUTTING_DOWN_REASON. These threads no longer apply, so please resolve them: startup refusal before shutdown exists, now fixed in isHeld (#3764 (comment)); null own start time, now refused (#3764 (comment)); try/finally around held sections, now in place (#3764 (comment)); doc comment on the four counts, now updated (#3764 (comment)); unbounded daemon.json poll, now capped at 500 with a named error (#3764 (comment)); test comment on the busy refusal, now corrected (#3764 (comment)).

Nothing else stands between this PR and merge.

@thymikee
thymikee merged commit 75ea87a into main Oct 17, 2026
17 checks passed
@thymikee
thymikee deleted the feat/ensure-daemon branch October 17, 2026 08:44
@github-actions

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-10-17 08:46 UTC

thymikee added a commit that referenced this pull request Oct 17, 2026
…ment

* origin/main:
  feat(cli): ensure the daemon explicitly before attaching in the CLI and MCP (#3766)
  feat(client): daemonLifecycle attach mode and ensureDaemon() (#3765)
  feat(daemon): quiesce request that retires an idle daemon atomically (#3764)
  fix(daemon): answer a named missing session on the session, not the device (#3701)
  test: route test/ and scripts/ off the daemon's testing exports (#3733)
  refactor(plugins): make the plugin framework provider-independent (#3722)
  fix(apple): name an Xcode too old for its host macOS as a runner startup failure (#3739)

# Conflicts:
#	packages/daemon/src/server/http/daemon-runtime.ts
#	packages/daemon/src/sessions/handlers/session-appstate-device-runtime.ts
#	packages/daemon/test/nightly/concurrency-torture/real-scope-serialization.ts
#	src/__tests__/daemon-registration-owner.test.ts
#	src/agent-device-client.ts
#	src/cli.ts
#	src/cli/commands/proxy.ts
#	src/daemon-client/daemon-client-lifecycle.ts
#	src/daemon-client/daemon-client-metadata.ts
#	src/daemon-client/daemon-client.ts
#	test/integration/provider-scenarios/harness.ts
thymikee added a commit that referenced this pull request Oct 17, 2026
…refactor/daemon-runtime-no-process-globals

* origin/refactor/host-environment:
  refactor(daemon): read configuration from the request host instead of the ambient scope
  test: merge the duplicate test-kit import in the stale provider admission scenarios
  feat(cli): ensure the daemon explicitly before attaching in the CLI and MCP (#3766)
  feat(client): daemonLifecycle attach mode and ensureDaemon() (#3765)
  feat(daemon): quiesce request that retires an idle daemon atomically (#3764)
  fix(daemon): answer a named missing session on the session, not the device (#3701)
  test: route test/ and scripts/ off the daemon's testing exports (#3733)
  refactor(plugins): make the plugin framework provider-independent (#3722)
  fix(apple): name an Xcode too old for its host macOS as a runner startup failure (#3739)

# Conflicts:
#	packages/daemon/src/server/http/daemon-runtime.ts
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.

feat(client): explicit daemon lifecycle with ensureDaemon() and attach clients

1 participant