fix: centralize confirmed daemon retirement - #3126
Conversation
Size Report
Startup median (7 runs, lower is better):
|
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
The code in df38a36 looks correct to me, and all 14 checks pass at that commit. Evidence is still pending: I did not run the focused unit or smoke tests locally, and I did not confirm that the smoke-daemon-clean real-daemon test ran unskipped in CI, since it depends on loopback availability. Please confirm that test ran, or show its output from a run on a machine with loopback. I also did not read tryAcquireProcessLock's EACCES branch or how it handles a legacy daemon.lock file that is not a directory. I inferred the 'retirement-unconfirmed' mapping from the chmod test and from host-kit tagging only the timeout path, so a quick check of that mapping would help. I cannot verify the PR body's line about an independent read-only review, so I did not count it as evidence. Not blocking, and you can take or leave these: when release fails after a result is already retained, releaseAfterRetirement overwrites the typed reason ('registration-replaced', 'metadata-unreadable', 'ownership-unproven') with 'retirement-unconfirmed', so a release failure should add a secondary error and only convert 'retired' or 'absent'; the live-owner test uses a vitest worker that isAgentDeviceDaemonProcess rejects, so it cannot fail on a signaling regression and its lock half returns 'absent' either way, and it should be renamed to what it asserts or check that the lock is acquired and released; no retirement test asserts 'daemon_retirement_release_failed' in the log, though the PR says these are recorded, so a match on paths.logPath like the registration test would cover it; and pruneStaleDevStateDirs prints only 'retired' results, so retained directories, including ones with an EACCES error, are now silent where the old rmSync would have thrown. Is there anything smaller? I looked and found nothing meaningful. #3116 names these two functions as the owner of client-side retirement, and the production change is +224/-79. One option is to let retireDaemonRegistration short-circuit a null observation itself, instead of widening the owner type in readRegisteredDaemonOwnership to No conflicts. The only thing before merge is that base PR #3125 lands first. |
df38a36 to
e72c8e1
Compare
|
The earlier evidence gap on df38a36 is now closed. At e72c8e1 I found no problems in the changed code. CI is still pending. All 11 non-passing jobs were cancelled by a superseded run and have no failed steps, so there is no failure to attribute. The Integration Tests job runs test/integration/smoke-daemon-clean.test.ts, which this change touches, so it needs a completed rerun on e72c8e1. CI also does not set AGENT_DEVICE_REQUIRE_LOOPBACK_TESTS, so a pass alone will not show that the real-daemon tests ran unskipped. I did not run the smoke or unit tests. Zero skips under that flag is the author's report. I only confirmed that the flag turns a skip into a failure. This PR does not make stopAndRetireDaemon the only retirement owner. daemon-client-lifecycle.ts:214 and :453-457, and daemon-client-timeout.ts:197-198, still remove daemon.json and daemon.lock without confirmed termination. The PR body defers these to #3116, so they are outside this slice. I did not re-review code outside the delta. Before merge, CI needs a completed rerun on e72c8e1. #3116 then needs to move the daemon-client lifecycle and timeout metadata removal onto stopAndRetireDaemon. |
e72c8e1 to
9b6b44a
Compare
9b6b44a to
d495c5c
Compare
d495c5c to
b45777a
Compare
…token-attach-c41aff * commit 'd396f3b509d9ed7cddaf170351ea6cf34e02ac16': fix(provider-webdriver): harden BrowserStack app references and endpoints (#3169) fix(android): back off a timed-out snapshot helper session and bound content re-captures (#3160) fix: centralize confirmed daemon retirement (#3126) fix: bind daemon registration writes to the acquired owner (#3125) fix: return confirmed daemon termination outcomes (#3124) refactor(move): share daemon registration and shutdown report modules (#3123) fix: preserve process lock exclusion across publication and reclaim (#3122) fix(cli): refuse a non-URL install-from-source source up front (#3166) fix(daemon): start a lease's TTL when its allocation completes (#3165) fix(android): fail doctor when adb is the Windows binary on a POSIX host (#3157) 0.21.20 0.21.19 # Conflicts: # src/daemon/server/daemon-runtime-metadata-ownership.test.ts
Summary
Client cleanup can call one job that confirms the captured process exited, acquires the startup lock, re-reads registration, removes only matching metadata, and confirms release. Abandoned recovery uses the same core without signaling live processes.
Replaced, unknown, unreadable, busy, and partially cleaned state is retained with typed outcomes. I/O errors preserve normalized primary failures; secondary release failures are recorded in daemon diagnostics. Acquisition permission failures are distinguished from contention.
The development cleanup script is the first consumer.
--prune-devselects directories by their newest mtime and retains shared state directories and session artifacts. Client lifecycle/timeout paths and private replay ownership follow in #3116.6 files changed. Builds on #3125.
Validation
Tested
e72c8e1a82:AGENT_DEVICE_REQUIRE_LOOPBACK_TESTS=1 pnpm check:affected --base fix/daemon-registration-owner --runpassed; all runnable checks completed.