test: catch daemon lifecycle and durable-state leaks (#1781 B1) - #1859
test: catch daemon lifecycle and durable-state leaks (#1781 B1)#1859thymikee wants to merge 8 commits into
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks: no changes in the largest emitted chunks. |
|
CI note: every job is green except Coverage, which fails on |
|
Not ready at exact
The Coverage failure is inherited main-side ratchet skew, not caused here. The PR remains draft and unready. |
84c81b6 to
dac78ed
Compare
|
CI note: rebased onto current |
…ules Review of #1859 found the oracle reported "clean" when the daemon itself outlived shutdown: daemon pids were excluded from ownership, liveDaemonPids never reached hasDaemonLeaks, and a live daemon even flipped daemon.json/lock from stray to expected. stopProcessForTakeover is best-effort void, so only smoke-daemon-clean independently asserted death. - a live daemon pid at phase 'after-shutdown' is now itself a leak, and its metadata files stay stray; a live daemon remains legitimate at 'after-close' - split the pure ownership/residue rules into daemon-leak-model.ts and pin them with daemon-leak-model.test.ts, using the real ps rows captured during the #1109 and #1324 red-proofs (the lanes' daemons own no children, so the fixture test is what guards those shapes in CI) - exempt the managed tools/ install tree before the .tmp rule, so agent-browser's own download temporaries are no longer a false LEAK - report empty directories as residue (an unswept session scaffold leaves no file) - reuse src/utils/host-process.ts (expandProcessTree, uniquePositivePids) and its /bin/ps convention instead of re-deriving them - assert in smoke-daemon-http on the success path, not in finally, so the settle window cannot replace a primary assertion's diagnostic Refs #1781 #1431
|
Review round addressed — pushed as Blocker 1 — surviving daemon reported clean: fixed. A live daemon pid at Blocker 2 — red-proof does not transfer to the lanes: stated, not papered over. The PR body now carries a per-arm table: the surviving-daemon and state-dir-residue arms are genuinely guarded by the three lanes in CI; the owned-process arm asserts Should-fix, all done. (3) Nits. Recorded as you asked: both #1109 arms pinned |
|
Re-reviewed exact P1 remains: no real lane performs an |
…closed Re-review of #1859: `after-close` accepted every capture descriptor and legacy marker, so the closed session's unfinalized handle was indistinguishable from another session's legitimately live one — the phase could not fail, the same shape as the surviving-daemon blocker, and it left half of B1's stated scope undelivered. - the observation carries the session directories closed at the checkpoint; a capture handle must be finalized once its owning session is gone (every session after shutdown, only the closed ones after close), while another session's live handle stays expected and a legacy pid marker is never a finish record - the oracle accepts `closedSessions` and a `sessionsDir` override, normalizing entries to the canonical `sessions/<name>/…` shape so an in-process harness rooted directly at its own sessions dir is classified the same way - add a real route regression: a provider-backed session with a live screen recording is closed through the daemon route, and the oracle refuses a descriptor left `lifecycle: "open"`. Reverting the close-route finalization (session-close-lifecycle-teardown.ts, the #1325 fix) turns it red naming sessions/default/screen-recording.resource.json; with the pre-fix model it stays green, which is the P1 in one line Refs #1781 #1431
|
P1 fixed in The rule. The observation carries the session directories closed at the checkpoint. A capture handle must be finalized once its owning session is gone: every session at The route regression. Red proof, and the vacuity check you warned about:
Both reverts were temporary ( Both phases now have a lane that can fail: Also added four model cases for the new rule (24 total), and re-ran the whole |
|
Re-reviewed exact One blocker remains: |
… interface Re-review of #1859: `closedSessions` was optional and defaulted to empty, so any caller — or the standalone CLI invoked with `--phase after-close` and no `--closed-session` — silently restored the vacuous checkpoint that accepts every unfinalized capture handle and reports clean. - phase and the identity that phase needs are now one discriminated shape: { phase: 'after-close'; closedSessions: NonEmpty<string> } | { phase: 'after-shutdown' }, so the empty case is not expressible and 'after-shutdown' is unchanged - the CLI refuses the same invocation with a typed INVALID_ARGS error and a hint naming what the missing identity would have cost, instead of degrading - daemon-leak-oracle-cli.test.ts drives the real CLI: the unnamed after-close invocation must fail without reporting, the named one finds the planted unfinalized handle, and after-shutdown is unaffected. Reverting the guard makes it print 'clean (after-close)' over that same handle and turns the test red Refs #1781 #1431
|
Done in Type shape. Phase and the identity that phase needs are one discriminated shape: export type NonEmpty<T> = readonly [T, ...T[]];
export type DaemonLeakPhaseSelection =
| { phase: 'after-shutdown' }
| { phase: 'after-close'; closedSessions: NonEmpty<string> };
CLI refusal. Refusal regression ( And the refusal claim is itself red-proven — reverting the guard to the old optional/empty-tolerated behaviour: That failure message is the exact vacuous mode the guard exists to prevent: All four node lanes pass (4/4), the model test 24/24, the provider-backed close regression 1/1, |
|
Re-review of |
thymikee
added
the
ready-for-human
|
Trimmed in Per file, before → after:
What moved, and what was deleted. Moved into the probe: the four ownership rules and their Nothing was dropped. The three ownership fixture cases did not die with the arm — they moved to Re-run #1324 red-proof through the moved code — pre-fix Every retained rule still reds under revert. Model (21 cases): surviving-daemon, empty-directory, tools-before- Follow-up filed: #1882 — the daemon should record owned child pids the way it records captures. Then the arm becomes "read the record, assert they are dead": one rule instead of four, assertable in every lane including the device-free ones, immune to pid reuse and to macOS hiding Apple binaries' environment, and it doubles as the startup reaper's input. Linked from the PR body; #1431 wants it for the same reason. Four node lanes 4/4, provider scenario 1/1, unit fixtures 24/24, |
thymikee
removed
the
ready-for-human
|
Two blockers at exact head
|
d793687 to
ddc4307
Compare
|
Both blockers fixed in P1 — resolution (b), and it can really fail. I took the reviewer's second option: exercise an existing route that actually creates a child. That route already exists in CI — I chose (b) over (a) because (a) is the wrong size for this PR, not because it is wrong: recording owned pids means touching the daemon's process-launching interface — production surface, in a test-only PR, on a Hard-Rule module ( The arm is back in the shipped oracle — The checkpoint is #1109's acceptance criterion. After the normal smoke the lane reopens a session, stops the daemon with that session still open, and requires that nothing owned outlives the browser idle window. The unclosed session is the whole point: my first attempt put the checkpoint after Proof it goes red on a leaked child, same lane, browser idle window raised past the settle budget: Unmodified: green in 59 s. Cost: ~35 s added to a 30-minute job. The checkpoint also refuses to skip silently — missing P2 — fixed. LOC: 989 in the harness (852 shipped + 137 in the three regression tests), against 977 at |
|
The prior P1 and P2 fixes are sound at |
ddc4307 to
888a479
Compare
…ules Review of #1859 found the oracle reported "clean" when the daemon itself outlived shutdown: daemon pids were excluded from ownership, liveDaemonPids never reached hasDaemonLeaks, and a live daemon even flipped daemon.json/lock from stray to expected. stopProcessForTakeover is best-effort void, so only smoke-daemon-clean independently asserted death. - a live daemon pid at phase 'after-shutdown' is now itself a leak, and its metadata files stay stray; a live daemon remains legitimate at 'after-close' - split the pure ownership/residue rules into daemon-leak-model.ts and pin them with daemon-leak-model.test.ts, using the real ps rows captured during the #1109 and #1324 red-proofs (the lanes' daemons own no children, so the fixture test is what guards those shapes in CI) - exempt the managed tools/ install tree before the .tmp rule, so agent-browser's own download temporaries are no longer a false LEAK - report empty directories as residue (an unswept session scaffold leaves no file) - reuse src/utils/host-process.ts (expandProcessTree, uniquePositivePids) and its /bin/ps convention instead of re-deriving them - assert in smoke-daemon-http on the success path, not in finally, so the settle window cannot replace a primary assertion's diagnostic Refs #1781 #1431
…closed Re-review of #1859: `after-close` accepted every capture descriptor and legacy marker, so the closed session's unfinalized handle was indistinguishable from another session's legitimately live one — the phase could not fail, the same shape as the surviving-daemon blocker, and it left half of B1's stated scope undelivered. - the observation carries the session directories closed at the checkpoint; a capture handle must be finalized once its owning session is gone (every session after shutdown, only the closed ones after close), while another session's live handle stays expected and a legacy pid marker is never a finish record - the oracle accepts `closedSessions` and a `sessionsDir` override, normalizing entries to the canonical `sessions/<name>/…` shape so an in-process harness rooted directly at its own sessions dir is classified the same way - add a real route regression: a provider-backed session with a live screen recording is closed through the daemon route, and the oracle refuses a descriptor left `lifecycle: "open"`. Reverting the close-route finalization (session-close-lifecycle-teardown.ts, the #1325 fix) turns it red naming sessions/default/screen-recording.resource.json; with the pre-fix model it stays green, which is the P1 in one line Refs #1781 #1431
… interface Re-review of #1859: `closedSessions` was optional and defaulted to empty, so any caller — or the standalone CLI invoked with `--phase after-close` and no `--closed-session` — silently restored the vacuous checkpoint that accepts every unfinalized capture handle and reports clean. - phase and the identity that phase needs are now one discriminated shape: { phase: 'after-close'; closedSessions: NonEmpty<string> } | { phase: 'after-shutdown' }, so the empty case is not expressible and 'after-shutdown' is unchanged - the CLI refuses the same invocation with a typed INVALID_ARGS error and a hint naming what the missing identity would have cost, instead of degrading - daemon-leak-oracle-cli.test.ts drives the real CLI: the unnamed after-close invocation must fail without reporting, the named one finds the planted unfinalized handle, and after-shutdown is unaffected. Reverting the guard makes it print 'clean (after-close)' over that same handle and turns the test red Refs #1781 #1431
|
Rebased onto current Two files conflicted, both kept from both sides: Checked #1880 against this PR's subject matter, since stricter device selection could plausibly change what a session owns: it is admission-layer only — identity-conflict policy evaluated before dispatch — and adds no new state-dir artifact, so the allowlist needs no new classification. The device-backed paths still behave: the provider-backed close regression passes (its The enforceability decision stands and is unchanged by the rebase — it landed before this message: resolution (b), the web smoke lane, already proven red on a real stranded fleet. #1880 neither helps nor hurts it: that lane's sessions are web, so device-identity selection is not on its path. |
|
Re-reviewed exact head Two readiness cleanups remain: update |
… B1) Adds test/integration/support/daemon-leak-oracle.ts and calls it at the end of smoke-daemon-clean, smoke-daemon-http and daemon-replace-exit-flush. After shutdown the oracle asserts that no daemon-owned process survives (ownership: PPID descendant, PGID = daemon pid, AGENT_DEVICE_STATE_DIR env, state-dir argv — never global counts) and that the isolated state dir holds only classified artifacts (no *.tmp, no daemon.json/lock without a live daemon, no open capture descriptor). Red-proofs: pre-fix #1324 (a84caa8) leaves simctl recordVideo with ppid 1 / pgid = dead daemon; pre-fix #1109 (be4bd09) leaves the agent-browser daemon plus its Chrome fleet. Both are clean on main. Refs #1781 #1431
…ules Review of #1859 found the oracle reported "clean" when the daemon itself outlived shutdown: daemon pids were excluded from ownership, liveDaemonPids never reached hasDaemonLeaks, and a live daemon even flipped daemon.json/lock from stray to expected. stopProcessForTakeover is best-effort void, so only smoke-daemon-clean independently asserted death. - a live daemon pid at phase 'after-shutdown' is now itself a leak, and its metadata files stay stray; a live daemon remains legitimate at 'after-close' - split the pure ownership/residue rules into daemon-leak-model.ts and pin them with daemon-leak-model.test.ts, using the real ps rows captured during the #1109 and #1324 red-proofs (the lanes' daemons own no children, so the fixture test is what guards those shapes in CI) - exempt the managed tools/ install tree before the .tmp rule, so agent-browser's own download temporaries are no longer a false LEAK - report empty directories as residue (an unswept session scaffold leaves no file) - reuse src/utils/host-process.ts (expandProcessTree, uniquePositivePids) and its /bin/ps convention instead of re-deriving them - assert in smoke-daemon-http on the success path, not in finally, so the settle window cannot replace a primary assertion's diagnostic Refs #1781 #1431
…closed Re-review of #1859: `after-close` accepted every capture descriptor and legacy marker, so the closed session's unfinalized handle was indistinguishable from another session's legitimately live one — the phase could not fail, the same shape as the surviving-daemon blocker, and it left half of B1's stated scope undelivered. - the observation carries the session directories closed at the checkpoint; a capture handle must be finalized once its owning session is gone (every session after shutdown, only the closed ones after close), while another session's live handle stays expected and a legacy pid marker is never a finish record - the oracle accepts `closedSessions` and a `sessionsDir` override, normalizing entries to the canonical `sessions/<name>/…` shape so an in-process harness rooted directly at its own sessions dir is classified the same way - add a real route regression: a provider-backed session with a live screen recording is closed through the daemon route, and the oracle refuses a descriptor left `lifecycle: "open"`. Reverting the close-route finalization (session-close-lifecycle-teardown.ts, the #1325 fix) turns it red naming sessions/default/screen-recording.resource.json; with the pre-fix model it stays green, which is the P1 in one line Refs #1781 #1431
… interface Re-review of #1859: `closedSessions` was optional and defaulted to empty, so any caller — or the standalone CLI invoked with `--phase after-close` and no `--closed-session` — silently restored the vacuous checkpoint that accepts every unfinalized capture handle and reports clean. - phase and the identity that phase needs are now one discriminated shape: { phase: 'after-close'; closedSessions: NonEmpty<string> } | { phase: 'after-shutdown' }, so the empty case is not expressible and 'after-shutdown' is unchanged - the CLI refuses the same invocation with a typed INVALID_ARGS error and a hint naming what the missing identity would have cost, instead of degrading - daemon-leak-oracle-cli.test.ts drives the real CLI: the unnamed after-close invocation must fail without reporting, the named one finds the planted unfinalized handle, and after-shutdown is unaffected. Reverting the guard makes it print 'clean (after-close)' over that same handle and turns the test red Refs #1781 #1431
…path Maintainer asked whether the harness can be trimmed. The two arms differ in kind: the daemon already records captures and state-dir artifacts, so those rules are short and fire on every lane run; nothing records what a daemon spawned, so the process arm reconstructs ownership from the OS four ways — and the three CLI daemon lanes are device-free, so it only ever compared an empty set to an empty set. Move that arm to test/integration/support/daemon-owned-process-probe.ts, the manual script that produced the #1109/#1324 evidence, and keep its rules pinned by a fixture test (no lane can run the probe, so its fixtures are the only CI guard on those shapes). The shipped oracle keeps every guarantee it had: surviving daemon at after-shutdown, the closed-session finalization rule, the state-dir allowlist, the CLI refusal, and the phase-discriminated types. Lane-path harness 852 → 548 LOC (-36%); repo total roughly flat, because the ownership reconstruction can be relocated but not deleted. That is the argument for the follow-up: once the daemon records owned child pids the way it records captures, the arm collapses to "read the record, assert they are dead" and moves back into the oracle. Re-proved after the move: #1324 against pre-fix a84caa8 on an iPhone 16 simulator still goes red through the probe (simctl recordVideo, ppid 1, pgid = the dead daemon, 0-byte mp4), and clean once the orphan is reaped. Refs #1781 #1431
Re-review of ce9f0ee: moving that arm to a manual probe left the two failures B1 exists to prevent detectable only by hand, and a fixture test proves regexes recognize synthetic rows, not that the shipped route reaps what it spawned. Restore the arm to the shipped oracle (the three files return byte-identical to 3a5b9be) and give it a lane that can execute it: smoke-web-platform is the one CI route whose daemon owns real children — the managed agent-browser daemon and its Chrome fleet. After the normal smoke it reopens a session, stops the daemon with that session still open (the #1109 shape: an ordinary close reaps the fleet, so only an unclosed session can strand it), and requires that nothing owned outlives the browser idle window. Proven both ways locally: green in 59s, and red when the fleet is stranded (browser idle window raised past the settle budget) with the oracle naming 15 owned processes — the #1109 signature, in a lane that runs on every PR. Also: daemon-replace-exit-flush cleared `info` before the oracle ran, so a failed stop would skip the `finally` retry and remove the state dir while the daemon was still alive. Clear it only once the checkpoint passes. Refs #1781 #1431 #1882
888a479 to
c717e56
Compare
|
Not ready at |
thymikee
changed the title
|
Addressed at
The PR title/body now make the boundary explicit: this change does not claim #1109/#1324 process coverage. #1882 owns the cohesive durable process-identity design; the trace here confirmed Apple recording already persists exact identities in its capture descriptor, while agent-browser creates its daemon indirectly, so a quick central spawn hook would still be incomplete or duplicative. Local exact-head evidence: 21 model cases, four real-subprocess daemon/CLI regressions, the provider-backed close-route regression, and the full runnable |
|
Re-reviewed exact |
1 participant
Summary
Adds a daemon lifecycle and durable-state leak oracle around the three existing real-subprocess daemon lanes.
After session close or daemon shutdown it detects:
.tmppublishes, empty unswept directories, stale daemon metadata, and unknown state-dir artifacts.The phase and its required identity are one discriminated shape: an
after-closecheckpoint must name at least one closed session, so the vacuous form that accepts every open capture handle is not expressible. The standalone CLI enforces the same rule at runtime.This PR deliberately no longer infers daemon-owned processes from a host-wide
pssnapshot. #1882 owns that work at the correct boundary: persist exact process identity at the real owners (including the already-durable capture descriptor and agent-browser's indirect daemon lifecycle), then assert the record during shutdown/recovery. Until that lands, this PR makes no claim to catch #1109/#1324 process leaks.Refs #1781 B1, #1431, #1882.
Validation
The surviving-daemon rule is red-proven against this branch's pre-fix behavior: a live daemon after shutdown was previously reported clean and now fails.
The provider-backed close regression drives a live screen recording through the real daemon
closeroute. Reverting production finalization leavesscreen-recording.resource.jsonopen and makes the oracle fail; the pre-fix classifier stayed green on the same fault.The standalone CLI regression proves
--phase after-closewithout--closed-sessionis refused before a report can certify the state dir.Local evidence on
919da440a:pnpm check:affected --runrunnable set.Current scope: 9 files, 728 additions / 16 deletions. No production, workflow, user-facing docs, or skills files changed. The Web smoke is no longer modified.