Skip to content

fix(daemon-client): wait for an acknowledged quiesce instead of killing the daemon - #3780

Merged
thymikee merged 3 commits into
mainfrom
fix/ensure-daemon-drain-wait
Oct 17, 2026
Merged

thymikee merged 3 commits into
mainfrom
fix/ensure-daemon-drain-wait

Conversation

@thymikee

@thymikee thymikee commented Oct 17, 2026 •

Copy link
Copy Markdown
Member

Part of #3747, references #3754.

After a daemon answered the authenticated quiesce with draining, ensureDaemon still called retireDaemonForTakeover: SIGTERM, then SIGKILL after 3 s. On Windows SIGTERM ends the process outright, skipping plugin disposal, lease finalization and registration cleanup; on POSIX a healthy drain over 3 s was killed.

Now the client signals nothing after draining:

  • It waits up to 60 s (polling every 250 ms) for the fenced daemon (pid + start time) to exit, then clears the registration it left.
  • At the deadline, a daemon that fails 3 liveness probes is force-retired. stopDaemonProcess still signals only a process whose command line is a daemon entry, so an embedded host is never signaled (feat(host): startAgentDevice() for embedder-owned daemons #3757 will register embedded ownership explicitly).
  • A daemon that still answers is left alone; the call fails with daemon-drain-timeout and a hint naming agent-device daemon stop.

Tests use a virtual clock: a 5 s drain (past the old 3 s kill window) sees no signal, no signal arrives before the deadline, and a responsive daemon at the deadline is left running. Existing replace tests now let the stand-in daemon exit by itself after it answers.

ADR 0006 and the changelog are updated.

🤖 Generated with Claude Code

View guided diff Turn on auto-fix

…d of killing the daemon

After the daemon answers draining, ensureDaemon waits up to 60 s for it to exit and
release its registration. Signals are sent only to a daemon still running at the deadline
that also fails its liveness probes; one that still answers fails with daemon-drain-timeout.

Part of #3747, refs #3754

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.34 MB 5.34 MB +927 B
Package (unpacked) 5.34 MB 5.34 MB +927 B
Package (download) 1.61 MB 1.61 MB +217 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 27.7 ms 28.3 ms +0.5 ms
CLI --help 77.0 ms 79.3 ms +2.3 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 96% 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 src/daemon-client/daemon-client-ensure.ts Outdated
Comment thread src/daemon-client/daemon-client-ensure.ts Outdated
Comment thread src/daemon-client/daemon-lifecycle-errors.ts
…hten drain tests

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

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

You’re at about 96% 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 src/daemon-client/daemon-client-ensure.ts Outdated
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@thymikee

Copy link
Copy Markdown
Member Author

This PR is ready at 65b5043. The acknowledged-quiesce flow reads correctly, and all 17 checks pass at this commit.

Not blocking, and you can take or leave these: (1) client-api.md should list the new daemon-drain-timeout reason and the post-drain force-retire beside the other two cases, and the cli-help daemon topic needs one clause for it (https://github.com/callstack/agent-device/blob/65b5043/website/docs/docs/client-api.md#L114); (2) isRefusalToSignalNonDaemon casts error.details.retirement out of an AppError built in the same package, so calling stopAndRetireDaemon directly, or using a non-throwing variant, would remove the cast (https://github.com/callstack/agent-device/blob/65b5043/src/daemon-client/daemon-client-ensure.ts#L179); (3) the stand-in in the "answers a probe" test keeps serving /health after it reports draining, which a real daemon cannot do because shutdown() closes its servers first, so the probe branch looks unreachable today and the test or the docs could say which host it guards (https://github.com/callstack/agent-device/blob/65b5043/src/daemon-client/__tests__/daemon-client-ensure-drain.test.ts#L196); (4) the !startTime?.trim() guard now reports daemon-incompatible with mismatch no-quiesce where it used to end in daemon-lifecycle-contended, so a distinct label or a changelog line would help, the result of recoverAbandonedDaemonRegistration is dropped at line 160, and daemonDrainTimeoutError is async with no await.

I did not run the tests. The drain tests use a stand-in child process and a scripted HTTP server, so nothing here drains a real daemon of another version through shutdown() and checks the successor start, and the Windows SIGTERM claim has no Windows test. The PR body also has no pnpm check:affected receipt.

Five bot threads are fixed at this head, so please resolve them: #3780 (comment) (leaver signals now asserted), #3780 (comment) (start-time guard), #3780 (comment) (identity-unverified mapped to drain timeout), #3780 (comment) (comment says four ways), #3780 (comment) (doc comments say identity unproven).

Nothing blocks human review. The best next step is the docs update in note (1).

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 17, 2026
@thymikee
thymikee merged commit 28b9789 into main Oct 17, 2026
17 checks passed
@thymikee
thymikee deleted the fix/ensure-daemon-drain-wait branch October 17, 2026 10:42
@github-actions

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

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

* origin/main:
  refactor(daemon): publish a single package entrypoint (#3781)
  fix(daemon-client): wait for an acknowledged quiesce instead of killing the daemon (#3780)
  fix(daemon): refuse leases for providers without a registered runtime (#3773)

# Conflicts:
#	packages/daemon/src/sessions/handlers/__tests__/lease.test.ts
#	src/daemon-client/daemon-client-ensure.ts
thymikee added a commit that referenced this pull request Oct 17, 2026
…refactor/daemon-runtime-no-process-globals

* origin/refactor/host-environment:
  refactor(cli): resolve one transport per run and one host per MCP tool call; default the lease provider cwd to the bound host
  refactor(daemon): publish a single package entrypoint (#3781)
  fix(daemon-client): wait for an acknowledged quiesce instead of killing the daemon (#3780)
  fix(daemon): refuse leases for providers without a registered runtime (#3773)
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