fix(daemon): keep held runtime files fresh and exit when one is lost - #2289
halindrome wants to merge 8 commits into
Conversation
macOS com.apple.tmp_cleaner deletes /tmp files whose atime, mtime and ctime are all older than three days. The daemon's long-held lock files are empty and never written, so under a daemon that lives past three days the cleaner unlinks them while they are held. The next participant then creates and locks a fresh inode: forked index workers see the cohort daemon marker as free, read the live daemon as uncoordinated and exit 1 on every reindex, and a lost IPC lifetime lock or cohort lifetime file stops protecting the generation at all. A lost socket identity marker additionally makes listener close leave the socket pair behind. The host lifetime loop now runs a heartbeat every 60 s over every file the generation holds (cohort daemon claim and lease, participant guard, IPC lifetime reservation, identity marker). Each file is refreshed with futimens through its held descriptor, only after proving the path still names that descriptor, so the cleaner never selects it. If any file is already lost, the daemon logs daemon.lifetime_end reason=coordination_file_lost and stops cleanly; the next client starts a coordinated generation. Re-claiming in place would race that client. Windows needs no refresh (no age-based cleaner; held lock handles deny deletion), so its entry points only validate the handles. Closes DeusData#2178 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Shane McCarron <shane.mccarron@corvexconnect.com>
- Separate loss from transient failure. Touch helpers now report OK / lost / transient (version_cohort: OK/UNSAFE/IO; ipc: 1/0/-1). Only a proven unlink or replacement stops the daemon. A refresh failure on a still-valid file (EROFS, EPERM, EMFILE on the marker reopen) logs daemon.coordination_touch_failed once, retries every tick, and logs daemon.coordination_touch_recovered when it clears. - Cohort and claim locks now prove the path still names the held inode (private_file_revalidate against the stored directory + name), not just st_nlink, so rename-away, hard-link+unlink and a replaced directory are caught like unlink. - The runtime service's self-owned participant guard is refreshed by the listener heartbeat too. - Tests: rename-away detection; cohort lease/claim touch (lease with only lifetime held is OK, each unlinked file reported independently); IPC listener touch now checks timestamps are actually refreshed and covers the participant guard and identity-marker replacement. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Shane McCarron <shane.mccarron@corvexconnect.com>
- Classify touch failures explicitly. A file is lost only on proof that the path no longer names it: ENOENT, st_nlink 0, another inode, a symlink or other non-regular entry, or a replaced lock directory. Any other syscall failure (EIO, ESTALE, ENOMEM, EACCES on reopen) is transient, so a flaky runtime volume no longer stops the daemon. The POSIX private-lock touch and the ipc lock/marker touches share this rule; the Windows touch reports a failed handle query as transient. - The identity marker is classified by path (fstatat, no follow) before it is reopened, so replacement by a symlink is now loss, not a transient reopen failure. - Tests: symlink replacement of the identity marker (lost); an unreadable marker is transient (-1) and recovers (1) once readable; the cohort touch test ages both held files and asserts they were refreshed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Shane McCarron <shane.mccarron@corvexconnect.com>
QA Round 1Contract: #2178. Acceptance criteria are root-cause fixes 1 and 2: keep held files fresh, and detect loss then exit cleanly. The sentinel wording and the promotion of the worker failure reason into the daemon log (issue items 3a and 3b) are out of scope. Review panel: 3 Claude lenses (contract-security, regression-edges, schema-tests) plus a synthesizer. None failed. Contract Verification
After the round 1 fixes: AC3 is addressed by finding 1 and AC7 by findings 4–6. AC6 is still partial, by design (see finding 7). Finding 1: [claude] Cohort and claim lock loss is detected only through st_nlink on the held inode, with no check that the path still names that inode
Finding 2: [claude] Transient or IO errors during the heartbeat are reported as a lost coordination file and stop the daemon
Finding 3: [claude] No automated test covers the host wiring: host_coordination_touch, stop on loss, the log event, permanent generations, and no false positive at the t=0 check
Finding 4: [claude] The cohort and participant-guard touch helpers are untested, including the NULL-lock semantics that keep a running daemon alive
Finding 5: [claude] The IPC listener touch test checks only return values and never that timestamps were refreshed
Finding 6: [claude] Replacement (unlink then recreate at the same name) is not tested, so neither the AC2 hazard nor the dev/ino check is protected against regression
Finding 7: [claude] The Windows touch paths have no test, including the platform-neutral half that checks an intact handle is not reported lost
Finding 8: [claude] The socket and its anchor are deliberately not refreshed, and loss of the socket is not detected
Finding 9: [claude] After a long sleep there is a blind window: the monotonic clock does not advance during macOS sleep, so after more than 3 days asleep tmp_cleaner can run before the next touch
Finding 10: [claude] The runtime-service start path's owned_participant_guard is not refreshed by the listener heartbeat
Summary
Fixed: 1, 2, 4, 5, 6, 10. Accepted with rationale: 3, 7. No change needed: 8, 9. Schema ChangeNone. No SQL, migrations or DDL; all three lenses agree. Other notes
NEW SAST findings (GitHub code scanning, open alerts on
|
QA Round 2Contract: #2178 (AC1–AC7, issue items 3a and 3b out of scope). Review panel: 3 Claude lenses plus a synthesizer, none failed. CMM graph tools were available this round. The branch was re-synced with upstream Contract Verification
After the round 2 fixes, AC3's partial verdict (the symlink case) is addressed by finding 2, and the false-loss routes behind AC5 and AC6 are addressed by finding 1. AC6 stays partial only because the Windows touch tests are SKIP_PLATFORM, which was accepted in round 1. Finding 1: [claude] POSIX lock touch reports any validation error as 'lost', which stops the daemon instead of treating the error as transient. The Windows touch maps a GetFileInformationByHandle failure to loss.
Finding 2: [claude] Identity-marker touch treats replacement by a symlink (or by an unreadable same-name file) as transient instead of lost
Finding 3: [claude] The production host participant-guard touch, cbm_daemon_runtime_service_touch_listener, and the round-1 external_guard wiring are never called by a test
Finding 4: [claude] The transient (-1 / IO) result added in round 1 has no test
Finding 5: [claude] The version_cohort touch test checks only status codes, not that timestamps were refreshed
Summary
Fixed: 1, 2, 4, 5. Accepted with rationale: 3. Schema ChangeNone. All three lenses agree. Pre-existing issues discovered
Verification
NEW SAST findings (GitHub code scanning, open alerts on
|
QA Round 3 — cleanContract: #2178 (AC1–AC7). Panel: 3 Claude lenses plus a synthesizer, none failed. Branch re-synced with upstream All three lenses declare diminishing returns. No lens found a functional defect. One lens returned zero findings. The 6 merged findings are all minor: five test-coverage or test-robustness notes, and one code-hardening asymmetry that no lens could turn into a reachable failure. Contract Verification
AC1–AC5 and AC7 pass. AC6 remains partial for the reason accepted in round 1: the Windows touch entry points are compile- and CI-verified but their behaviour is not exercised by a test (the POSIX-only tests are Finding 1: [claude] Loss-detection-to-stop wiring in host_wait_for_lifetime has no automated test; two new public touch entry points have zero test references
Finding 2: [claude] ipc.c posix_path_names_inode is never exercised for held lock files - only plain unlink (st_nlink==0) is tested for lifetime.lock and the participant guard
Finding 3: [claude] private_held_identity directory-replacement and undetermined (-1) branches have no test
Finding 4: [claude] ipc.c touch path omits the runtime-directory identity check that every other ipc.c lock path (and private_file_lock.c) performs
Finding 5: [claude] Identity-marker LOSS_REPLACE_FILE case can false-pass on filesystems that reuse inode numbers
Finding 6: [claude] Windows #2178 heartbeat is unverified beyond compilation - all three new tests SKIP_PLATFORM on _WIN32
Summary
Nothing fixed this round: every finding is advisory, and no acceptance criterion fails. Round 1 raised 10 findings (6 fixed), round 2 raised 5 (4 fixed), round 3 raised 0 functional defects. Schema ChangeNone. Verification
NEW SAST findings (GitHub code scanning, open alerts on
|
|
Thanks for opening this — it has been seen, and it is queued. This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence. Current review status: working through a backlog. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
If this fixes a bug, a reproduction we can run is worth more than a description of the symptom. Thanks for contributing, and sorry in advance for the wait. |
DeusData
left a comment
There was a problem hiding this comment.
Thank you — this is careful work on #2178. Refreshing each held file through its descriptor only after proving the path still names the same file, and stopping the daemon when a coordination file is provably gone, is the behaviour we want: a daemon that has lost its claim can no longer prove it is the only one, and stopping is the safe move. The tests are deterministic, using timestamps they set themselves with no sleeps and no clock checks, and the fcntl close-drops-lock trap is avoided.
Two small things before it merges:
- A test for the host-loop wiring. The per-file loss paths are covered, but nothing covers the loop itself: the refresh on its tick leading to a stop on loss. A seam that drives one tick with a lost file and asserts
daemon.lifetime_end reason=coordination_file_lostwould close that. - Drop the unrelated reflow hunks in
test_daemon_ipc.c(the SID test),test_version_cohort.candtest_private_file_lock.c, so the diff says only what the PR does.
For context, #2207 fixes a different cause of lifetime-lock loss, the probe's fcntl close, and the two complement each other. Thank you again.
Add cbm_daemon_host_wait_for_lifetime_for_test, which runs the production lifetime loop over caller-held coordination handles, and a runtime test that ages the claim marker, unlinks the cohort lifetime lock, and asserts the tick refreshes the marker, logs daemon.lifetime_end reason=coordination_file_lost file=cohort_lease, and stops the runtime. The seam runs an ephemeral generation, so a missed loss ends at the initial client window instead of hanging. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NbYKGdU7pqG6AEuMQuJxiF Signed-off-by: Shane McCarron <shane.mccarron@corvexconnect.com>
Restore main's formatting on pre-existing lines that a whole-file clang-format pass had reflowed, so the diff carries only the DeusData#2178 change. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NbYKGdU7pqG6AEuMQuJxiF Signed-off-by: Shane McCarron <shane.mccarron@corvexconnect.com>
Reflow the host-loop seam call to clang-format and note that the fixture's convenience start joins its own participant guard, unlike the host's start_reserved. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NbYKGdU7pqG6AEuMQuJxiF Signed-off-by: Shane McCarron <shane.mccarron@corvexconnect.com>
QA Round 4Round: 4 · PR: #2289 · Model: claude-opus-5-5 · Contract source: the maintainer's CHANGES_REQUESTED review · Scope: Contract Verification
Does the test catch real breakage?Yes.
I found no way for it to pass vacuously:
Other checks
Findings
Verdict: needs fixes (1 should-fix) → addressed in 8cd2ee5. This QA round was run by Claude Opus 5.5 (Anthropic) in Claude Code. |
QA Round 5 — cleanPR: #2289 (closes #2178)
Findings: none. Verdict: clean. This QA round was run by Claude Opus 5.5 (Anthropic) in Claude Code. |
|
Thanks for the review. Both items are addressed:
The new commits are fee73ce (test), b15e7f9 (reflow revert) and 8cd2ee5 (QA round 4 fix). QA rounds 4 and 5 are posted above, and round 5 is clean. On macOS arm64, These changes were written by Claude Opus 5.5 (Anthropic) in Claude Code and reviewed by me before pushing. |
Problem
macOS
com.apple.tmp_cleaner(launchd; daily at midnight, or on the next wake if the machine was asleep) runs:The daemon's coordination files are zero-byte, created once and never written, and
flocktouches no timestamp. After three days they qualify and are unlinked while the daemon still holds them. The next participant'sO_CREAT|O_EXCLthen creates a fresh inode nobody holds:version_cohort_active_daemon_presence()sees markerOK+ lifetime probe1=UNCOORDINATED, so every index worker the daemon forks refuses at startup and exits 1, forever. That is A live daemon that is not holding the version-cohort marker poisons every index worker it forks, for its entire lifetime — no self-heal, and the conflict log names a build that does not exist #2178: 370 conflict records over 427 minutes on one machine, ending at the exact second the daemon was killed..sock.identitylost →posix_listener_artifacts_remove_if_matches()refuses to unlink anything, so even a cleandaemon.stopleaves a markerless.sock/.ancpair behind (the shape reported in macOS: markerless stale socket/anchor pair permanently blocks daemon startup #1757; fix: recover markerless stale rendezvous sockets during activation #1894 recovers from it at startup, but nothing stops it being created).Field evidence (2026-09-21, the fifth occurrence on this machine): daemon started 09-16;
cbm-version-cohort-{daemon,admission}-v1.lockandcbm-<key>.lifetime.lockall have birth time 09-21 09:24:05, minutes after a lid-open wake. Every onset has been in the morning, which is when a missed midnight job runs on wake.Fix
The host lifetime loop runs a heartbeat every 60 s (
HOST_COORDINATION_TOUCH_MS) over every file the generation holds:cbm_version_cohort_daemon_claim_touchcbm_version_cohort_lease_touchcbm_daemon_ipc_participant_guard_touch.sock.identitycbm_daemon_ipc_listener_touch(viacbm_daemon_runtime_service_touch_listener)futimensthrough the held descriptor, and only after proving the path still names that descriptor. Refreshing by path would keep a replacement inode fresh and hide the loss. The identity marker, which is not held open, is classified byfstatat(AT_SYMLINK_NOFOLLOW)before it is reopened.ENOENT,st_nlink == 0, another inode, a non-regular entry (symlink), or a replaced lock directory. Anything else (EIO,ESTALE,ENOMEM,EACCESon reopen) is transient: the daemon logsdaemon.coordination_touch_failedonce, keeps serving, retries next tick, and logsdaemon.coordination_touch_recoveredwhen it clears. A flaky runtime volume must not end a healthy generation.daemon.lifetime_end reason=coordination_file_lost file=<artifact>and stops cleanly, permanent generations included. The next client then starts a coordinated generation. Re-claiming in place would race that client.This is the fix direction discussed in the issue thread (points 1 and 2 of the root-cause comment). Issue items 3a (the
pre-cohort/unknownsentinel) and 3b (promoting the worker's failure sentence into the daemon log) are deliberately not in this PR — they are independent and small, and I am happy to send them separately.Tests
private_file_lock_touch_refreshes_held_file_and_detects_unlink— ages a held file, touches it, and proves the timestamps moved; unlink →UNSAFE; and a peerEXthen succeeds, which is the hazard itself.private_file_lock_touch_detects_rename_away— the renamed file keepsst_nlink == 1, so this is the case a link-count check alone misses.version_cohort_touch_reports_each_lost_file— a lease holding only its lifetime file isOK; marker unlink → claimUNSAFEwhile the lease staysOK; lifetime unlink → leaseUNSAFE; both files proven refreshed.daemon_ipc_listener_touch_detects_lost_runtime_files— five loss cases (lifetime lock, participant guard, identity marker unlinked, replaced by a fresh inode, replaced by a symlink), each with a proven refresh first, plus an unreadable marker returning transient and recovering once readable.No sleeps and no wall-clock assertions: each timestamp assertion compares against the stale value the test itself set.
Verification
scripts/test.sh: 8111 passed, 10 skipped. One failure,test_daemon_runtime.c:3938ASSERT(marker_published)(section_cold_storm's sibling in the runtime suite), which also fails on unmodifiedmain— measured 4 of 6 runs on plainmainversus 6 of 7 on this branch, and all three review passes confirmed this diff cannot reach that fixture (the new code is reachable only fromhost.c, which the fixture never calls). Integration steps 5–6 pass when run directly (the script exits at the flake before reaching them).CBM_RUNTIME_DIR, with the real binary: marker unlink → clean stop in 59 s withfile=cohort_daemon_claim;.sock.identityunlink → clean stop withfile=listener; cohort lifetime renamed away → clean stop withfile=cohort_lease; identity marker replaced by a symlink → clean stop. A freshdaemon startsucceeded after each.coordination_touch_failed; restoring it loggedcoordination_touch_recovered.Note on one pre-existing behaviour found while testing
A stray symlink at the
.sock.identitypath blocks daemon startup (CBM daemon could not start within 30000 ms). I reproduced this on a runtime directory where no daemon had ever run with the symlink present, so it predates this change; it needs same-uid tampering inside the 0700 directory, andtmp_cleanernever creates symlinks. With this PR a live daemon in that state now exits at the next tick with a named reason instead of serving on and hitting the same refusal at its next restart. Happy to file it separately if you would like it tracked.Closes #2178
🤖 Generated with Claude Code