fix(test): settle the daemon's exit on its own deadline in the web shutdown lane - #2083
Conversation
Size Report
npm unpacked components
Startup median (7 runs, lower is better):
Top changed chunks: no changes in the largest emitted chunks. Top changed packed files
|
|
[P1] Put identity-aware daemon exit waiting at the daemon-process owner and reuse it here. The new test-local 45s poll correctly treats the original daemon identity disappearing as success, but duplicates lifecycle policy beside the owner it already imports. More importantly, |
Review feedback on #2083: the smoke lane had grown its own identity-aware poll beside the owner it already imported, and the owner itself was weaker than the copy. Add `waitForDaemonExit(identity, { timeoutMs, pollMs })` to daemon-process, the module that owns daemon lifecycle policy, and consume it from the web shutdown smoke, `stopProcessForTakeover` and `stopDaemon`. The test-local helper and its paragraph of rationale are gone; the invariant now lives in the interface. This closes a real defect in `stopProcessForTakeover`. It verified identity only before SIGTERM, then waited on the bare pid: a daemon that exited and had its pid reused read as "still running", and the takeover escalated SIGKILL onto whatever now held that number. Escalation is no longer a rule each call site remembers — `signalDaemonIdentity` re-reads identity immediately before every signal, so a recycled pid cannot be signaled at all. `stopDaemon` already re-verified before SIGKILL; it now also stops misreporting a recycled pid as "did not exit". `classifyDaemonPid` names the four states a pid can be in, including the one a bare liveness read cannot express: a process that has died but has not been reaped answers kill(pid, 0) and still reports its original start time, while ps shows its command as `<defunct>`. Reading that as a recycled pid would hand the number back while it is still taken, so it counts as neither ending and the wait keeps polling — preserving the guarantee callers had before. Deterministic coverage in daemon-exit-wait.test.ts: identity changing mid-wait, a zombie waited through to its reap, no SIGKILL after a recycled grace wait, and escalation still reached for a daemon that genuinely survives SIGTERM. Verified planted-red — reverting stopProcessForTakeover to the bare-pid wait fails the recycled-pid test alone, on its assertion, inside the unit lane's clock budget.
82aee9b to
029a10d
Compare
|
Addressed in
The takeover defect is fixed. Rather than re-verifying at each call site, One thing your review surfaced that I had wrong too. Waiting on identity alone regressed Coverage ( Branch rebased onto 🤖 Addressed by Claude Code |
|
[P2] Remove the new implementation-history/control-flow comments. Current main’s AGENTS.md rejects comments that narrate a workaround or preserve review history, yet the daemon-process helper/type blocks, daemon-exit-wait test comments, and edited smoke comment still do exactly that. The identity type, classifier, one owning wait helper, and planted-red assertions now make the invariant evident in code; retain only necessary public API documentation. The implementation and exact-head CI/live-web evidence are otherwise clean. |
…utdown lane The daemon-exit assertion in the #1868 shutdown lane was an instantaneous liveness read placed right after settleManagedBrowserProcesses, so it silently borrowed the Chrome fleet's poll deadline for a condition it never polled. It passed only while the fleet outlasted the daemon. Nothing enforces that order. The daemon awaits the `agent-browser close` CLI's return, never the disappearance of the Chrome pids that call reaps, and awaits it under a teardown-budget race it walks away from on expiry — so either can finish first, and both have been observed: macOS reaps the fleet ~13s after the daemon, while CI once had the daemon still alive ~4.8s after the fleet and failed the lane. A re-run of the same commit passed. Settle both endings concurrently from the same SIGTERM, each against its own deadline, and assert no ordering between them. The guarantee #1868 is actually about — the fleet closed because the daemon closed it, not because agent-browser's idle timer beat us — is held by WEB_SHUTDOWN_IDLE_TIMEOUT_MS, not by any ordering here. settleDaemonExit waits out the daemon's identity rather than the bare pid: a pid the host recycles inside the 45s window is a different process, and reading it as "the daemon is still alive" would fail this lane for the one reason it must not — a false red. It reuses isAgentDeviceDaemonProcess and both existing settle constants, and reports elapsed time so a deadline failure says how long the daemon outlived its SIGTERM.
Review feedback on #2083: the smoke lane had grown its own identity-aware poll beside the owner it already imported, and the owner itself was weaker than the copy. Add `waitForDaemonExit(identity, { timeoutMs, pollMs })` to daemon-process, the module that owns daemon lifecycle policy, and consume it from the web shutdown smoke, `stopProcessForTakeover` and `stopDaemon`. The test-local helper and its paragraph of rationale are gone; the invariant now lives in the interface. This closes a real defect in `stopProcessForTakeover`. It verified identity only before SIGTERM, then waited on the bare pid: a daemon that exited and had its pid reused read as "still running", and the takeover escalated SIGKILL onto whatever now held that number. Escalation is no longer a rule each call site remembers — `signalDaemonIdentity` re-reads identity immediately before every signal, so a recycled pid cannot be signaled at all. `stopDaemon` already re-verified before SIGKILL; it now also stops misreporting a recycled pid as "did not exit". `classifyDaemonPid` names the four states a pid can be in, including the one a bare liveness read cannot express: a process that has died but has not been reaped answers kill(pid, 0) and still reports its original start time, while ps shows its command as `<defunct>`. Reading that as a recycled pid would hand the number back while it is still taken, so it counts as neither ending and the wait keeps polling — preserving the guarantee callers had before. Deterministic coverage in daemon-exit-wait.test.ts: identity changing mid-wait, a zombie waited through to its reap, no SIGKILL after a recycled grace wait, and escalation still reached for a daemon that genuinely survives SIGTERM. Verified planted-red — reverting stopProcessForTakeover to the bare-pid wait fails the recycled-pid test alone, on its assertion, inside the unit lane's clock budget.
…change AGENTS.md allows only public API docs, tool directives, and a brief citation to an external constraint. The identity type, the pid-state union, the single wait helper and the planted-red assertions carry the invariant; the prose restating them, and the review history behind them, belong in the PR. Kept: the exported identity/result types and the wait helper's contract, plus a two-line citation for the one thing code cannot encode — a terminated pid awaiting reap answers kill(pid, 0), keeps its start time, and reports `<defunct>`.
029a10d to
f6bf3e2
Compare
|
Removed in Every comment this PR added is gone except four, all of which the rule permits:
Dropped: the Verified at 🤖 Addressed by Claude Code |
|
Re-reviewed exact head |
thymikee
added
the
ready-for-human
|
1 participant
What
The daemon-exit assertion in the #1868 web shutdown lane (
live web platform e2e daemon-shutdown browser cleanup) was an instantaneous liveness read placed right aftersettleManagedBrowserProcesses:It silently borrowed the Chrome fleet's poll deadline for a condition it never polled, and passed only while the fleet happened to outlast the daemon.
Both endings now settle concurrently from the same SIGTERM, each against its own deadline, with no ordering asserted between them.
Why
Nothing enforces that order. The daemon awaits the
agent-browser closeCLI's return, never the disappearance of the Chrome pids that call reaps, and awaits it under a teardown-budget race (WEB_BROWSER_SESSION_TEARDOWN_BUDGET_MS+DAEMON_SESSION_TEARDOWN_TIMEOUT_MS) it walks away from on expiry. Either can finish first, and both orders have been observed:expected the daemon process itself to have exited after SIGTERM. Seen once on refactor(commands): one audience table for common input fields #2074; re-running the same job on the same commit passed.The guarantee #1868 is actually about — the fleet closed because the daemon closed it, not because agent-browser's idle timer beat the test — is held by
WEB_SHUTDOWN_IDLE_TIMEOUT_MS, not by any ordering between these two. So the ordering is asserted nowhere, on purpose, and the comment records why.Notes for the reviewer
settleDaemonExitwaits out the daemon's identity, not the bare pid. A pid the host recycles inside the 45s window is a different process, and reading it as "the daemon is still alive" would fail this lane for the one reason it must not — a false red. It pollsisAgentDeviceDaemonProcess(pid, startTime)(already exported; this file already imports from that module, and the sibling test at the top of the file exists for the same hazard). That also makesWEB_SHUTDOWN_SETTLE_POLL_MSthe right interval, since the identity probe shells out topslike the fleet sweep. No new constant, no new machinery, no new module edge — a structural sibling ofsettleManagedBrowserProcessessharing both existing constants.No fixed sleep. The 45s is a failure bound, not a wait; the happy path returns as soon as the daemon is gone. There is no event-based alternative: the daemon is not a child of the test process (the CLI spawns it detached and exits), Node exposes neither kqueue
NOTE_EXITnorpidfd, and every daemon artifact (lock file,daemon.json, shutdown report) is written beforeexit()— so watching one would prove the daemon decided to exit, which is the weaker proxy claim this bug came from in the first place.Residual limits, stated rather than hidden: the 45s is derived from the daemon's 35s web-session teardown budget and must grow with it; a zombie daemon would still read as alive (documented at
src/utils/host-process.ts:64), unreachable here because the reparented daemon is reaped by launchd/init.Verification — planted red, then green
Plant:
await sleep(N)immediately beforeexit()insrc/daemon/server/daemon-runtime.ts(reverted; not in this diff).sleep(25s)expected the daemon process itself to have exited after SIGTERM, the exact CI messagesleep(25s)sleep(120s)still alive 45128ms laterRun with
AGENT_DEVICE_WEB_E2E=1 pnpm test:smoke:web. Healthy-path wall clock is unchanged because the two settles run concurrently.pnpm lintandpnpm typecheckboth clean.Test-only change; #2074 was not touched.