Skip to content

fix(daemon): keep held runtime files fresh and exit when one is lost - #3

Open
halindrome wants to merge 8 commits into
mainfrom
fix/cohort-lock-tmp-cleaner
Open

halindrome wants to merge 8 commits into
mainfrom
fix/cohort-lock-tmp-cleaner

Conversation

@halindrome

@halindrome halindrome commented Sep 21, 2026 •

Copy link
Copy Markdown
Owner

Fork-internal QA PR. QA rounds for the upstream fix run here, so each fix commit starts fork CI rather than upstream CI. When a round comes back clean, an upstream draft PR against DeusData/codebase-memory-mcp:main will close DeusData#2178, with these QA reports carried over.

Problem

macOS com.apple.tmp_cleaner (launchd, daily at midnight, or on wake if the machine was asleep) deletes /tmp files whose atime, mtime and ctime are all older than 3 days. The daemon's long-held lock files are empty and never written, so under a daemon that lives longer than 3 days the cleaner unlinks them while they are held. The next participant then creates and locks a fresh inode:

  • Cohort daemon marker lost: a forked index worker's marker probe acquires OK, so it reads the live daemon as UNCOORDINATED and exits 1 on every reindex (the "pre-cohort/unknown" wedge).
  • IPC lifetime lock or cohort lifetime file lost: the daemon's reservation no longer protects the generation. A peer can claim a fresh lifetime file, and the install/update barrier could proceed while the daemon is alive.
  • .sock.identity lost: posix_listener_artifacts_remove_if_matches refuses to unlink anything, so even a clean daemon.stop leaves a markerless .sock/.anc pair behind.

Field evidence (2026-09-21): the daemon started Sep 16, and the daemon/admission cohort locks and the IPC lifetime lock all have a birth time of Sep 21 09:24:05, after the machine woke.

Fix

The host lifetime loop now runs a heartbeat every 60 s over every file the generation holds:

Artifact Entry point
Cohort daemon claim (marker EX) cbm_version_cohort_daemon_claim_touch
Cohort lease (lifetime SH) cbm_version_cohort_lease_touch
Participant guard cbm_daemon_ipc_participant_guard_touch
IPC lifetime reservation + identity marker cbm_daemon_ipc_listener_touch (through cbm_daemon_runtime_service_touch_listener)
  • Each file is refreshed with futimens through the held descriptor, and only after proving the path still names that descriptor (st_nlink == 1 / fd-vs-path inode). Refreshing by path would keep a replacement inode fresh and hide the loss.
  • On any loss the daemon logs daemon.lifetime_end reason=coordination_file_lost file=<artifact> and stops cleanly, permanent daemons included. The next client then starts a coordinated generation. Re-claiming in place would race that client.
  • Windows: there is no age-based cleaner, and held lock handles deny deletion, so the Windows entry points only validate the handles.

Tests

  • private_file_lock_touch_refreshes_held_file_and_detects_unlink: ages a held file, touches it (timestamps refreshed), unlinks it (touch → UNSAFE), and confirms that a peer EX then succeeds, which is the hazard itself.
  • daemon_ipc_listener_touch_detects_lost_runtime_files: a live listener passes touch; after unlinking the lifetime lock, or in a second case the identity marker, touch reports the loss.
  • End to end against an isolated CBM_RUNTIME_DIR with the built binary:
  • scripts/test.sh: 8109 passed, 1 failed, 10 skipped. The failure is daemon_runtime test_daemon_runtime.c:3938 (ASSERT(marker_published)), which also fails intermittently on unmodified upstream/main def38f3e (4 of 6 runs; this branch 6 of 7), and all three round-1 reviewers confirmed the diff cannot reach it.

Refs DeusData#2178 (the upstream PR will carry Closes #2178).

🤖 Generated with Claude Code

shanemccarron-maker and others added 2 commits September 21, 2026 11:48
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>
@halindrome

Copy link
Copy Markdown
Owner Author

QA Round 1

Contract: DeusData#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

Criterion Verdict Evidence
AC1 (keep fresh): every private lock/identity file held or owned for the daemon lifetime is refreshed on a timer well under 3 days pass Timer: host.c:57 HOST_COORDINATION_TOUCH_MS=60000. First check runs immediately (next_touch=0, host.c:941-944). The refresh is futimens(fd,NULL), which updates atime and mtime and bumps ctime. File-by-file (host path uses start_reserved, host.c:1122): (1) cohort lease lifetime file: refreshed, version_cohort.c:370-379. The admission and maintenance locks are released after acquire (version_cohort.c:574-583), so they are NULL and correctly skipped. (2) cohort daemon claim marker: refreshed, version_cohort.c:381-383. (3) host participant guard (legacy lock): refreshed, ipc.c:3646-3650. (4) IPC lifetime reservation, moved into the listener by listen_reserved (ipc.c:3089): refreshed, ipc.c:3240-3242. (5) listener participant guard: NULL on the host path (only cbm_daemon_ipc_listen sets it, ipc.c:3110); refreshed when present, ipc.c:3243-3246. (6) .sock.identity: reopened, verified by dev/ino, refreshed, ipc.c:3248-3257. (7) runtime service owned_participant_guard: NULL on the host path (set only by cbm_daemon_runtime_service_start, runtime.c:2535, which src never calls outside tests), so nothing to refresh. (8) startup lock (startup-v2 and legacy): not held for the lifetime on the host path. (9) pending record: removed after publish (ipc.c:3076-3081). (10) socket and anchor: S_ISSOCK, not refreshed on purpose because ctime is part of socket identity; outside -type f. Daemon logs live under the cache dir, not the runtime dir (host.c host_log_open). Project locks (project_lock.c) are taken per operation, not for the lifetime. Caveat: after the machine sleeps longer than 3 days, the cleaner may run before the next tick. AC3/AC4 cover that window.
AC2 (refresh through the held object, never blindly by path) pass private_file_lock.c:495 futimens(lock->fd) runs after fd validation under the fork guard. ipc.c:3226-3228 posix_held_file_touch compares path and fd with private_regular_file_snapshot and then calls futimens(fd). The identity marker is opened with O_NOFOLLOW|O_CLOEXEC|O_NONBLOCK, and the fd's st_dev/st_ino must equal listener->identity_device/inode before futimens(marker_fd) (ipc.c:3248-3257). Because the touch is applied to the verified fd, a path swap between open and check can only touch the inode that was checked. There is no TOCTOU exposure, and marker_fd is closed on every path.
AC3 (detect loss: path no longer names the held inode) partial IPC files: unlink and replacement are detected by the path-vs-fd and dev/ino comparison (ipc.c:803-822, 3253-3254). Cohort files: only st_nlink==1 is checked (private_file_lock.c:393-404). That catches unlink and rename-over, but not rename-away. See the minor contract finding. The tmp_cleaner scenario is fully detected.
AC4 (fail safe on loss: named event, orderly stop incl. permanent, no re-claim) pass host.c:941-950 logs warn daemon.lifetime_end reason=coordination_file_lost file= and returns cbm_daemon_runtime_service_stop(). The check is outside the permanent gate, so permanent generations stop too. Teardown then follows the normal ordered path (host.c:1182-1206), the same one stop_requested uses; force-terminate happens only if the stop times out, as in the existing path. Nothing re-claims. posix_listener_artifacts_remove_if_matches (ipc.c:3115-3200) refuses to unlink artifacts it does not own, and cbm_private_file_lock_release does not unlink by path, so the old generation cannot delete a replacement's files.
AC5 (no regressions; heartbeat cheap, no race with listener/lease teardown) pass The touch runs only in the RUNNING branch of the host thread. service->listener is closed only in cbm_daemon_runtime_service_free (runtime.c:2724), which the same host thread calls after the loop returns. runtime_service_destroy_unstarted (runtime.c:2378) runs only on the startup failure path. Leases, the claim and the guard are released after the loop (host.c:1203-1205). Cost is a few fstat/fstatat/openat/futimens calls per 60s. Nothing else in the code reads timestamps of lock or marker files (grep of st_mtim/st_ctim hits only socket identity and executable-image checks). Flaky test_daemon_runtime.c:3938 ASSERT(marker_published): this diff cannot plausibly affect it. That test drives the runtime service through runtime_test_fixture, not cbm_daemon_host_run, and the only callers of the new touch functions are host.c:888-898 plus the two new unit tests. The runtime.c change only adds a function. See the transient-error minor finding.
AC6 (portability: Windows entry points exist and don't falsely report loss; Linux) partial Windows: cbm_private_file_lock_touch (private_file_lock.c:1471-1483) checks handle validity via private_win_payload_handle_valid. Listener and guard stubs return non-NULL (ipc.c:6797-6804). The host guard on Windows is non-NULL on success (ipc.c:6730+). Linux: futimens is covered by -D_GNU_SOURCE (Makefile.cbm:50). Not verified: no Windows or Linux build or test ran in this review, and both new tests SKIP_PLATFORM on _WIN32.
AC7 (tests cover refresh-through-fd and loss detection, no sleeps/wall-clock flakiness) partial test_private_file_lock.c:489-541 ages timestamps with futimens, then asserts the touch refreshed them and that unlink returns UNSAFE. It uses fixed old times and no sleeps. test_daemon_ipc.c:2066-2112 covers unlink of the lifetime lock and the identity marker. Not covered: the host loop and stop path, the cohort wrappers, participant-guard loss, marker replacement (the dev/ino branch), and whether listener touch refreshes timestamps.

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

  • Area: src/foundation/private_file_lock.c (lines 485–501)
  • What was tested: Read cbm_private_file_lock_touch (485-501) and private_payload_fd_valid (393-404). Validity comes from fstat alone: fd>=0, tracked, S_ISREG, uid, st_nlink==1, mode 0600, empty ACL. Nothing looks up the lock's name in its directory or compares it with the fd's dev/ino. The IPC counterpart posix_held_file_touch (ipc.c:3226-3229) does make that comparison through private_regular_file_snapshot (ipc.c:803-822).
  • Expected: AC3: detect that a held file was 'unlinked or replaced (path no longer names the held inode)'.
  • Actual / Risk: Unlink and rename-over drop the held inode to nlink 0, so both are caught. That covers the tmp_cleaner trigger in 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 DeusData/codebase-memory-mcp#2178. Three cases leave nlink==1 on an inode the path no longer names, so the touch still returns OK: a rename-away inside the same filesystem, a hard-link followed by unlink of the original name, and a rename or replacement of the lock directory. A peer can then create or lock a fresh inode at the canonical path while the old daemon keeps refreshing the orphan and coordinating nothing. tmp_cleaner does none of these, so the real-world risk is low. Still, the cohort and claim files get weaker detection than the IPC files and than AC3 asks for.
  • Severity: minor | Status: confirmed | Relevance: contract
  • Resolution: Fixed in 3cf2a66: cohort and claim touches now run private_file_revalidate (path inode vs fd inode, directory identity) against the stored directory and name. New test private_file_lock_touch_detects_rename_away. Rename-away of the cohort lifetime file checked end to end: clean stop in 57s with file=cohort_lease.

Finding 2: [claude] Transient or IO errors during the heartbeat are reported as a lost coordination file and stop the daemon

  • Area: src/daemon/ipc.c (lines 3226–3261)
  • What was tested: Traced every failure path of the heartbeat. cbm_daemon_ipc_listener_touch reopens the identity marker with openat, and any failure there returns false, including EMFILE/ENFILE on a busy daemon. posix_held_file_touch also returns false when the snapshot fails or futimens fails. In private_file_lock_touch, IO results (futimens failure, fork guard not entered) come back as CBM_PRIVATE_FILE_LOCK_IO. version_cohort_lock_touch (version_cohort.c:366-368) collapses both IO and UNSAFE to false. host_coordination_touch and host_wait_for_lifetime (host.c:886-903, 941-950) then log daemon.lifetime_end reason=coordination_file_lost file=, with no errno, and stop the service.
  • Expected: Only a real unlink or replacement should end the generation (AC3/AC4). A resource-exhaustion or IO error should either retry on the next tick or get its own log reason and errno.
  • Actual / Risk: A long-lived daemon start generation that hits RLIMIT_NOFILE at a 60s tick shuts down and interrupts every connected client. On a filesystem where futimens always fails, every new generation passes acquisition and then fails its first touch at t=0, so each client start produces another start/stop cycle. Examples are a custom CBM_RUNTIME_DIR remounted read-only, or NFS with uid squash (EROFS/EPERM/EIO). In both cases the log says 'coordination_file_lost', which points diagnosis the wrong way. The name 'listener' also does not say which of the three listener files failed: the lifetime lock, the participant guard or the identity marker.
  • Severity: minor | Status: confirmed | Relevance: regression
  • Resolution: Fixed in 3cf2a66: touch results are now OK / lost / transient. Only loss stops the daemon. A transient failure logs daemon.coordination_touch_failed file=<name> once, retries every tick, and logs daemon.coordination_touch_recovered when it clears. On the identity-marker reopen, only ENOENT counts as loss.

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

  • Area: src/daemon/host.c (lines 874–951)
  • What was tested: Searched src/ and tests/ for callers of the new functions. Tests call only cbm_private_file_lock_touch (tests/test_private_file_lock.c 508/514) and cbm_daemon_ipc_listener_touch (tests/test_daemon_ipc.c 2093/2098). host_coordination_touch, cbm_daemon_runtime_service_touch_listener and the loss-to-stop branch of host_wait_for_lifetime are reached only from host.c and have no test seam; host_wait_for_lifetime is static.
  • Expected: At least one test showing that a lost coordination file makes the lifetime loop return through cbm_daemon_runtime_service_stop (a clean stop, not host_force_terminate), including with permanent=true, and that intact handles do not stop the daemon at the first check (next_touch=0).
  • Actual / Risk: AC4 behaviour is verified only by the author's manual e2e runs. That covers the named warn event, the orderly stop, and permanent generations. Several regressions would pass the unit suites: moving the touch before the stop branch, reporting a NULL participant_guard as loss, swapping cohort_lease and daemon_claim, or breaking the file-name mapping. Because the first check runs at t=0, any change that makes a touch helper return false for a legitimate state would stop every daemon right after start.
  • Severity: minor | Status: confirmed | Relevance: regression
  • Resolution: Not fixed (accepted): host_wait_for_lifetime is static and has no seam. The wiring (named event, clean stop, permanent generation) is covered by e2e runs against an isolated CBM_RUNTIME_DIR: marker unlink, identity unlink, and cohort-lifetime rename-away all stop cleanly within one tick, and the next daemon start succeeds.

Finding 4: [claude] The cohort and participant-guard touch helpers are untested, including the NULL-lock semantics that keep a running daemon alive

  • Area: src/daemon/version_cohort.c (lines 366–383)
  • What was tested: version_cohort.c:578/583 releases lease->admission and lease->maintenance after acquisition, so they are NULL in a running daemon. version_cohort_lock_touch returns true for NULL (!lock ||), which is what keeps cbm_version_cohort_lease_touch returning true. No test calls cbm_version_cohort_lease_touch, cbm_version_cohort_daemon_claim_touch or cbm_daemon_ipc_participant_guard_touch (ipc.c:3646-3650). No test reaches the listener-owned guard branch of cbm_daemon_ipc_listener_touch (ipc.c:3243-3246). The IPC test never unlinks the participant or legacy lock (endpoint lock_name).
  • Expected: Tests for: lease touch returning true with only lifetime held and false after that file is unlinked; claim touch returning false after the marker is unlinked; participant guard touch returning false after the lock_name file is unlinked.
  • Actual / Risk: These are the files named in the e2e evidence (file=cohort_daemon_claim). A regression in the NULL handling, such as tightening it to lock && ..., would make every daemon report cohort_lease lost at t=0 and stop at once, and the suite would still pass.
  • Severity: minor | Status: confirmed | Relevance: regression
  • Resolution: Fixed in 3cf2a66: new test version_cohort_touch_reports_each_lost_file checks that a lease holding only its lifetime file is OK, that an unlinked marker gives claim UNSAFE while the lease stays OK, and that an unlinked lifetime file gives lease UNSAFE.

Finding 5: [claude] The IPC listener touch test checks only return values and never that timestamps were refreshed

  • Area: tests/test_daemon_ipc.c (lines 2066–2112)
  • What was tested: Read daemon_ipc_listener_touch_detects_lost_runtime_files. It asserts only the touched_intact and touched_after_loss booleans. It never ages the lifetime reservation, participant guard or .sock.identity, and never stats them after the touch.
  • Expected: AC7: tests cover refresh through the fd for every held file class. At least one IPC artifact should be aged with futimens, as the private_file_lock test does, and then shown to be fresh after cbm_daemon_ipc_listener_touch.
  • Actual / Risk: If cbm_daemon_ipc_listener_touch (ipc.c:3235-3261) or posix_held_file_touch (3226-3229) dropped the futimens call and kept only the snapshot checks, this test would still pass. The IPC half of AC1 is protected only by the manual e2e run.
  • Severity: minor | Status: confirmed | Relevance: contract
  • Resolution: Fixed in 3cf2a66: the IPC test now ages each target file with utimensat and asserts that atime and mtime are newer after the touch.

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

  • Area: tests/test_private_file_lock.c (lines 489–541)
  • What was tested: Traced the order of operations in both new tests: touch (intact), unlink, touch (expects UNSAFE/false). The second touch runs while the path is absent, never while it names a replacement inode. The IPC test also only unlinks. So the st_dev/st_ino comparison at ipc.c:3253-3254, which exists to catch replacement of the identity marker, never runs in a test.
  • Expected: A test where a successor first recreates or locks the path, after which the old holder's touch returns UNSAFE/false and leaves the replacement inode's timestamps unchanged.
  • Actual / Risk: A path-based implementation (utimensat on the name, with ENOENT mapped to UNSAFE) passes both new tests. After a peer recreated the file, that implementation would keep the replacement inode fresh and report OK, which is exactly what AC2 forbids. A broken dev/ino check would also go unnoticed. The current code is correct on reading.
  • Severity: minor | Status: confirmed | Relevance: contract
  • Resolution: Fixed in 3cf2a66: the IPC test covers identity-marker replacement (unlink, then a fresh inode at the same name, which gives 0). The private-lock rename-away test covers a path that no longer names the held inode.

Finding 7: [claude] The Windows touch paths have no test, including the platform-neutral half that checks an intact handle is not reported lost

  • Area: tests/test_private_file_lock.c (lines 489–493)
  • What was tested: Both new tests call SKIP_PLATFORM on _WIN32 for the whole body. On Windows, cbm_private_file_lock_touch (private_file_lock.c:1471-1483) returns UNSAFE unless private_win_payload_handle_valid holds. The Windows IPC stubs (ipc.c:6797-6804) return listener!=NULL and endpoint&&guard.
  • Expected: AC6: the entry points exist and do not falsely report loss. The half that acquires, touches and expects OK does not need an unlink while the file is held, so it could run on Windows.
  • Actual / Risk: The host checks at t=0, so a false UNSAFE from the Windows handle-validity check would stop every Windows daemon at startup with no CI signal. On reading the risk is low, because the same handle check already gates payload read and write.
  • Severity: minor | Status: confirmed | Relevance: contract
  • Resolution: Not fixed (accepted): the Windows entry points only validate handles, CI compiles them, and the existing tests use the repo's SKIP_PLATFORM pattern for POSIX-only unlink behaviour.

Finding 8: [claude] The socket and its anchor are deliberately not refreshed, and loss of the socket is not detected

  • Area: src/daemon/ipc.c (lines 3235–3261)
  • What was tested: listen_reserved (ipc.c:3028-3053) publishes the bound socket (socket_name) and its hard-linked anchor (socket_anchor_name, nlink 2), and the heartbeat does not touch them. That is correct: touching would change ctime, and ctime is part of posix_socket_identity_t (posix_socket_identity_equal, ipc.c:~2004).
  • Expected: If an age-based cleaner ever removed files that are not regular files, the daemon would notice.
  • Actual / Risk: This does not affect the find -type f cleaner named in AC1. If some cleaner did unlink sockets, clients could not reach a daemon that still holds every lock, and the heartbeat, which checks only the identity marker, would not notice. Not verified against com.apple.tmp_cleaner's actual file-type filter.
  • Severity: minor | Status: hypothetical | Relevance: observation
  • Resolution: No change: tmp_cleaner deletes only -type f, so sockets and anchors survive. The identity marker, whose loss orphans them, is refreshed and checked.

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

  • Area: src/daemon/host.c (lines 55–57)
  • What was tested: On macOS, cbm_now_ms uses mach_absolute_time (src/foundation/platform.c 270-274), which stops during system sleep. next_touch = now + 60000 (host.c 943-944). launchd runs a missed periodic tmp_cleaner soon after wake.
  • Expected: AC1 says the files 'never' match the age criteria while the daemon is alive.
  • Actual / Risk: That cannot hold across a sleep of more than 3 days. The result is the designed fail-safe: the loss is detected within 60s and the daemon stops cleanly. So this is a known limit, not a defect. A laptop closed for 3 days or more loses its permanent daemon on wake.
  • Severity: minor | Status: hypothetical | Relevance: observation
  • Resolution: No change: unavoidable without wake notifications. The window is bounded to one 60s tick after wake. (On macOS, CLOCK_MONOTONIC keeps advancing during sleep, so the first tick after a long sleep falls due at once.)

Finding 10: [claude] The runtime-service start path's owned_participant_guard is not refreshed by the listener heartbeat

  • Area: src/daemon/runtime.c (lines 2581–2583)
  • What was tested: cbm_daemon_runtime_service_start (runtime.c ~2506-2537) stores its guard in service->owned_participant_guard, not in listener->participant_guard. cbm_daemon_runtime_service_touch_listener touches only service->listener. The host uses start_reserved and keeps its own guard, which host_coordination_touch does touch.
  • Expected: The runtime.h comment implies the heartbeat covers the service's long-lived artifacts.
  • Actual / Risk: This matters only if a long-lived runtime service outside the host is ever given this heartbeat. There is no effect on the current production path.
  • Severity: minor | Status: confirmed | Relevance: observation
  • Resolution: Fixed in 3cf2a66: cbm_daemon_runtime_service_touch_listener passes owned_participant_guard into cbm_daemon_ipc_listener_touch as external_guard.

Summary

Severity Count
Critical 0
Major 0
Minor 10
Total 10

Fixed: 1, 2, 4, 5, 6, 10. Accepted with rationale: 3, 7. No change needed: 8, 9.

Schema Change

None. No SQL, migrations or DDL; all three lenses agree.

Other notes

  • All three lenses agree that the test_daemon_runtime.c:3938 ASSERT(marker_published) failure cannot be caused by this diff: the new code is reachable only from host.c, and that fixture never calls cbm_daemon_host_run. Measured separately, it also fails on unmodified upstream/main def38f3e in 4 of 6 runs; this branch failed it in 6 of 7.
  • After the fixes, the affected suites pass under ASan/UBSan (private_file_lock, version_cohort, daemon_ipc: 83 passed, 3 skipped).

NEW SAST findings (GitHub code scanning, open alerts on fix/cohort-lock-tmp-cleaner)

Open code-scanning alerts on the PR head ref, filtered at severity floor high (total open alerts at any severity: 0). A finding here is present on the PR branch — the reviewer should weigh each as either intentional (e.g. a deliberate new dependency with a tracked CVE) or a regression to fix. GitHub does not expose a per-job baseline the way GitLab artifacts do, so this is an absolute snapshot of the head ref, not a strict delta vs the base branch.

No open code-scanning alerts at or above severity high on fix/cohort-lock-tmp-cleaner.

Dependabot (SCA) — advisory, repo-wide

Dependabot alerts unavailable (not enabled, or token lacks security_events scope). Skipped — non-blocking.


Note: GitHub code scanning is an absolute snapshot of open alerts on the head ref, not a strict NEW-vs-baseline delta. The reviewer should treat each finding as either intentional (and recommend dismissing the alert with a reason) or a regression (and recommend a fix).


QA performed by Claude Code (claude-opus-5)

shanemccarron-maker and others added 2 commits September 21, 2026 14:58
- 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>
@halindrome

Copy link
Copy Markdown
Owner Author

QA Round 2

Contract: DeusData#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 d615088a beforehand; that pulled in 27 upstream commits, none of which touch this PR's files.

Contract Verification

Criterion Verdict Evidence
AC1 (keep fresh): every private lock/identity file held or owned for the daemon lifetime is refreshed on a timer well under 3 days pass Timer: HOST_COORDINATION_TOUCH_MS=60000 (host.c:57). The first check runs immediately (next_touch=0, host.c:924,959). Held-file inventory for the host path (cbm_daemon_host_run, host.c:1038-1235): (1) cohort daemon marker cbm-version-cohort-daemon-v1.lock: refreshed (claim_touch, version_cohort.c:391-395). (2) Cohort lifetime lock cbm-version-cohort-lifetime-v1.lock: refreshed (lease_touch). Admission/maintenance are released after admission (version_cohort.c:589-598) and touched if still present. (3) Host participant guard (legacy cbm-.lock, flock SH): refreshed via cbm_daemon_ipc_participant_guard_touch (host.c:913, ipc.c:3667). (4) IPC lifetime reservation cbm-.lifetime.lock (fcntl F_WRLCK, handed to the listener via start_reserved, runtime.c:2477 / ipc.c:3089): refreshed via listener_touch. The touch uses the held fd only and never opens or closes a second fd on that file, so the fcntl lock is not dropped. (5) .sock.identity marker: refreshed by reopen plus inode check (ipc.c:3244-3261). (6) listener->participant_guard and runtime owned_participant_guard: NULL on the host path (start_reserved; only cbm_daemon_ipc_listen / service_start set them) and refreshed when set (ipc.c:3272-3280, runtime.c:2581-2585). Not held for the lifetime: startup/startup-v2 locks (released after listen), the pending record (unlinked during publish, ipc.c:3079), and project locks (per mutation, application.c:922-944). Not refreshed and not -type f: the socket and its anchor (a hard link of the socket, nlink 2) and the runtime directory (accepted round-1 item 8). The daemon log lives in the cache dir, not the runtime dir (host.c:128-142).
AC2 (refresh through the held object, never blindly by path) pass Locks: private_file_revalidate (path inode == fd inode, plus the directory's path-vs-fd identity), then futimens(lock->fd) under the fork guard (private_file_lock.c:490-508). IPC locks: private_regular_file_snapshot(dir_fd, name, fd) then futimens(fd) (ipc.c:3232-3237). Marker: openat O_NOFOLLOW|O_NONBLOCK|O_CLOEXEC, snapshot path-vs-fd, then a comparison with the published identity_device/inode before futimens on that fd (ipc.c:3244-3261). If the path is swapped after the check, only the owned inode is touched, and the next tick catches the swap. No path-based utimes anywhere.
AC3 (detect loss: path no longer names the held inode) partial Unlink, replacement by a fresh regular file, and rename-away are detected for locks and the marker. Tests: private_file_lock_touch_refreshes_held_file_and_detects_unlink, private_file_lock_touch_detects_rename_away, version_cohort_touch_reports_each_lost_file, and daemon_ipc_listener_touch_detects_lost_runtime_files (4 cases incl. marker replacement). A rename-away of the directory is caught by the cohort touch (private_directory_revalidate; the cohort directory is the same runtime dir, ipc.c:1056-1077). Gap: a marker replaced by a symlink or an unreadable file comes back transient (F-01).
AC4 (fail safe on loss: named event + clean orderly stop incl. permanent; no re-claim) pass host.c:962-965 logs warn daemon.lifetime_end reason=coordination_file_lost file= and returns cbm_daemon_runtime_service_stop. This check sits before any permanent-only branch, so it applies to permanent generations. Teardown then follows the normal path. Every release function is unconditional with respect to path validity: cbm_private_file_lock_release (private_file_lock.c:510-550), lease/claim release (version_cohort.c:317-362,851-873), participant_guard_release and lifetime_reservation_release (ipc.c:3650-3664,1313-1320). So an unlinked file cannot make host_cleanup_release_until_complete fall through to force-terminate. Nothing re-claims in place.
AC5 (no regressions; cheap heartbeat; no race with listener/lease teardown) pass The touch runs on the host thread inside host_wait_for_lifetime, before any teardown. The listener is closed only by cbm_daemon_runtime_service_free / destroy_unstarted (runtime.c:2378,2726), also on the host thread after the loop. Lease, claim and guard are released after the loop (host.c:1231-1233). Touching stops once the state is STOPPING (host.c:932-941). Cost: a few fstat/fstatat/futimens calls per minute plus one openat/close for the marker. The fork mutex is held only briefly. The only change to shared acquisition code is storing directory and base_name in try_acquire (length already bounded by private_base_name_valid ≤ NAME_MAX). The test_daemon_runtime.c:3938 marker_published failure uses the runtime fixture directly, not cbm_daemon_host_run, and never reaches any touch function (the only callers are in host.c:908-915). This diff cannot plausibly cause it; that matches the reported baseline flake.
AC6 (portability: Windows entry points exist, do not falsely report loss; Linux) partial Windows stubs exist: ipc.c:6818-6829 (listener touch returns 1, guard touch returns 1) and private_file_lock.c:1478-1490 (handle validity only). Fork CI on 3cf2a66 is green for windows/macos/linux plus the sanitizers, per the orchestrator. Caveat: on Windows a transient GetFileInformationByHandle failure maps to UNSAFE, i.e. a false loss (F-02, hypothetical).
AC7 (tests cover refresh-through-fd and loss detection, no sleeps / wall-clock flakiness) pass Files are aged with futimens/utimensat to a fixed 2001 timestamp and the tests assert atime/mtime > stale. There are no sleeps; the only cbm_now_ms use is an acquire deadline (test_version_cohort.c:283). Coverage spans private_file_lock (unlink, rename-away), version_cohort (claim then lease loss) and ipc listener (lifetime lock, listener guard, marker unlink/replace). Gap: participant_guard_touch, the external_guard argument and the transient path are untested (F-03). The host loop has no unit seam (accepted in round 1).

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.

  • Area: src/foundation/private_file_lock.c (lines 490–508)
  • What was tested: Read cbm_private_file_lock_touch (private_file_lock.c:490-508) with private_file_revalidate (206-219), private_directory_revalidate (174-184) and private_payload_fd_valid (398-409). Read posix_held_file_touch and private_regular_file_snapshot (ipc.c:3232-3237, 803-821). Read cbm_macos_extended_acl_fd_is_empty (macos_acl.c:12-35). Compared against the round-1 tri-state design. Only posix_identity_marker_touch (ipc.c:3243-3262) separates ENOENT (lost) from other errno values (transient). Windows twin: private_file_lock.c:1478-1490 / private_win_payload_handle_valid 1401-1409.
  • Expected: Round-1 fix (2) set a tri-state rule: only proof that the path no longer names the held inode counts as loss. Everything else (EIO, ENOMEM, ESTALE, a failed ACL read, a GetFileInformationByHandle failure) should return IO/-1. The host then logs daemon.coordination_touch_failed and keeps serving, the same way the identity-marker path already does. AC6 says the Windows entry points 'must not falsely report loss'.
  • Actual / Risk: linked is a single bool built from a chain of checks: fstat(fd), fstatat(dir, name), lstat(directory->path), acl_get_fd_np on both fds, the mode/uid/nlink checks and private_lock_is_tracked. Any syscall error in that chain is treated like an unlink. Examples: acl_get_fd_np failing with ENOMEM (it fails closed unless errno is ENOENT), ESTALE/EIO on an NFS CBM_RUNTIME_DIR, or an I/O error on the runtime volume. Any of these returns UNSAFE, and posix_held_file_touch returns 0. host_wait_for_lifetime (host.c:962-965) then logs daemon.lifetime_end reason=coordination_file_lost and stops the generation, including permanent ones. On Windows, a GetFileInformationByHandle/GetHandleInformation failure also returns UNSAFE instead of IO. The stop is clean and clients re-spawn, so the impact is a spurious restart, not corruption. It is still the false-loss route the tri-state change was meant to close. Not observed in practice.
  • Severity: minor | Status: confirmed | Relevance: regression
  • Resolution: Fixed in 1133ba3. cbm_private_file_lock_touch now classifies explicitly through private_held_identity. Lost means st_nlink 0, a replaced or missing directory, ENOENT, or a path naming another inode or a non-regular entry. Every other syscall failure is IO (transient). The ipc lock touch uses the same rule through posix_path_names_inode. On Windows, a failed GetFileInformationByHandle is now IO.

Finding 2: [claude] Identity-marker touch treats replacement by a symlink (or by an unreadable same-name file) as transient instead of lost

  • Area: src/daemon/ipc.c (lines 3244–3261)
  • What was tested: Read posix_identity_marker_touch. It opens the marker with O_NOFOLLOW and counts only ENOENT as loss. Every other openat errno returns -1 (transient). If .sock.identity is replaced by a symlink, openat fails with ELOOP. If it is replaced by a same-uid file the daemon cannot read, openat fails with EACCES.
  • Expected: AC3 asks for detection when the file is 'unlinked or replaced'. A marker whose path names something other than the published inode should return 0 (lost), matching lock-file handling: posix_held_file_touch uses fstatat plus private_regular_file_snapshot, so a non-regular, symlink or mode-mismatch replacement counts as lost there.
  • Actual / Risk: When the marker is replaced by a symlink, the host logs daemon.coordination_touch_failed once and keeps serving indefinitely. posix_listener_artifacts_remove_if_matches then leaves the socket pair behind at close. This requires same-uid tampering inside a 0700 directory. tmp_cleaner never creates symlinks, and replacement by a fresh regular file IS detected (covered by a test). Low impact, but here the lost/transient classification is the reverse of the lock-file path.
  • Severity: minor | Status: confirmed | Relevance: contract
  • Resolution: Fixed in 1133ba3. The marker is classified by fstatat(AT_SYMLINK_NOFOLLOW) before it is reopened, so a symlink or non-regular replacement is lost (0). Reopen failures such as EMFILE or EACCES stay transient. New symlink case in daemon_ipc_listener_touch_detects_lost_runtime_files.

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

  • Area: tests/test_daemon_ipc.c (lines 2066–2135)
  • What was tested: Searched tests/ for cbm_daemon_ipc_participant_guard_touch, cbm_daemon_runtime_service_touch_listener, and any cbm_daemon_ipc_listener_touch call with a non-NULL external_guard. There are no hits: both symbols appear only in src/daemon/{ipc.c,ipc.h,runtime.c,runtime.h,host.c}. The IPC test always passes external_guard=NULL, so it only exercises the listener-owned participant guard ('lock' case). Production call sites: host.c:913 (host_coordination_touch) calls cbm_daemon_ipc_participant_guard_touch (ipc.c:3664-3673). Round-1 fix (10) passes runtime->owned_participant_guard as external_guard (runtime.c:2581-2585).
  • Expected: AC7: automated tests cover refresh-through-fd and loss detection for every held file. The guard and runtime entry points should each have a direct refresh-and-detect-loss test, like the other touch functions.
  • Actual / Risk: The wrappers are thin and delegate to posix_held_lock_touch, which the listener-guard case covers, so regression risk is low. Still, none of the following would fail a test: a regression in the guard's owner_pid/NULL checks (which return 0 = lost), passing the wrong dir_fd, or dropping the external_guard argument. The first two would make every host tick report participant_guard lost and stop the daemon in production. Today only the author's manual e2e runs catch this.
  • Severity: minor | Status: confirmed | Relevance: contract
  • Resolution: Accepted. cbm_daemon_ipc_participant_guard_touch and the external_guard wiring are thin delegates to posix_held_lock_touch, which the listener-guard (lock) case covers directly. A standalone guard can only be built through the prepared startup-lock handoff. Production wiring was checked e2e.

Finding 4: [claude] The transient (-1 / IO) result added in round 1 has no test

  • Area: src/daemon/ipc.c, src/foundation/private_file_lock.c, src/daemon/version_cohort.c, src/daemon/host.c (lines 3220–3290)
  • What was tested: Went through every new or changed test: tests/test_private_file_lock.c (touch_refreshes_held_file_and_detects_unlink, touch_detects_rename_away), tests/test_daemon_ipc.c (daemon_ipc_listener_touch_detects_lost_runtime_files) and tests/test_version_cohort.c (version_cohort_touch_reports_each_lost_file). Every assertion checks only OK/1 or UNSAFE/0. None of these transient branches runs: posix_identity_marker_touch returning -1 on a non-ENOENT openat failure, a futimens failure giving -1/IO, private_file_lock_touch returning IO when the fork guard is refused, the IO case in version_cohort_lock_touch, or posix_touch_fold combining -1 with 1.
  • Expected: Round 1 fix (2) made the result three-way (refreshed / lost / transient) so that EMFILE or a read-only remount does not stop the daemon. At least one test should confirm that a non-loss failure returns -1/IO, not 0/UNSAFE.
  • Actual / Risk: A later change that folds transient back into lost would pass every test. That is the exact round-1 bug: for example, return 0 on any openat failure, or treating IO as UNSAFE in version_cohort_lock_touch. The reverse mistake (classing a real loss as transient) would also pass. The host-side handling has no unit seam, which round 1 accepted; here the lower layers are untested too. The contract-security and regression-edges lenses both noted this gap.
  • Severity: minor | Status: confirmed | Relevance: regression
  • Resolution: Fixed in 1133ba3. The ipc test makes the identity marker unreadable (mode 0) and asserts -1, then restores it and asserts 1. The root guard expects 1 because root bypasses mode 0. Checked e2e: with the marker unreadable the daemon kept serving and logged daemon.coordination_touch_failed file=listener; after restoring it logged daemon.coordination_touch_recovered.

Finding 5: [claude] The version_cohort touch test checks only status codes, not that timestamps were refreshed

  • Area: tests/test_version_cohort.c (lines 672–716)
  • What was tested: Read version_cohort_touch_reports_each_lost_file. It asserts OK, then UNSAFE after unlinking the marker, then UNSAFE after unlinking the lifetime file. It never ages either file or checks mtime/atime after cbm_version_cohort_lease_touch or cbm_version_cohort_daemon_claim_touch.
  • Expected: AC1 names the cohort daemon marker and the lease files specifically. The IPC test ages files and asserts they were refreshed; the cohort test should do the same.
  • Actual / Risk: Low. The cohort functions are thin wrappers over cbm_private_file_lock_touch, whose refresh has its own test. A wrapper that returned OK without calling touch would go unnoticed on the refresh side.
  • Severity: minor | Status: confirmed | Relevance: regression
  • Resolution: Fixed in 1133ba3. version_cohort_touch_reports_each_lost_file ages the marker and lifetime files, then asserts atime and mtime were refreshed after the touch.

Summary

Severity Count
Critical 0
Major 0
Minor 5
Total 5

Fixed: 1, 2, 4, 5. Accepted with rationale: 3.

Schema Change

None. All three lenses agree.

Pre-existing issues discovered

  • A stray symlink at the identity-marker path blocks daemon startup ("could not start within 30000 ms"). This was reproduced with no daemon ever having run on the symlink, and removing it restores startup, so it is existing upstream fail-closed behaviour and not something this diff introduced. It needs same-uid tampering inside the 0700 runtime directory; tmp_cleaner never creates symlinks. With this PR, a live daemon in that state now exits at the next tick with coordination_file_lost file=listener. Before, it would have served indefinitely and hit the same startup refusal at its next restart.

Verification

  • Affected suites under ASan/UBSan: private_file_lock, version_cohort, daemon_ipc gave 83 passed and 3 skipped, three runs in a row.
  • End to end against an isolated CBM_RUNTIME_DIR: an unreadable marker kept the daemon serving, a restored marker logged recovery, and a symlink replacement produced a clean stop. All three happened within one tick.
  • Fork CI on round 1 (3cf2a66): 31 jobs green, including msan, tsan, lsan, Windows and macOS. The only red was codeql-gate, which counts every open alert in the fork. The fork had 21 stale alerts (vendored grammars and Scorecard); they have now been dismissed.
  • Every lens again confirms that the test_daemon_runtime.c:3938 flake is unrelated.

NEW SAST findings (GitHub code scanning, open alerts on fix/cohort-lock-tmp-cleaner)

Open code-scanning alerts on the PR head ref, filtered at severity floor high (total open alerts at any severity: 0). A finding here is present on the PR branch — the reviewer should weigh each as either intentional (e.g. a deliberate new dependency with a tracked CVE) or a regression to fix. GitHub does not expose a per-job baseline the way GitLab artifacts do, so this is an absolute snapshot of the head ref, not a strict delta vs the base branch.

No open code-scanning alerts at or above severity high on fix/cohort-lock-tmp-cleaner.

Dependabot (SCA) — advisory, repo-wide

Dependabot alerts unavailable (not enabled, or token lacks security_events scope). Skipped — non-blocking.


Note: GitHub code scanning is an absolute snapshot of open alerts on the head ref, not a strict NEW-vs-baseline delta. The reviewer should treat each finding as either intentional (and recommend dismissing the alert with a reason) or a regression (and recommend a fix).


QA performed by Claude Code (claude-opus-5)

@halindrome

Copy link
Copy Markdown
Owner Author

QA Round 3 — clean

Contract: DeusData#2178 (AC1–AC7). Panel: 3 Claude lenses plus a synthesizer, none failed. Branch re-synced with upstream e783f73d first; those 43 upstream commits touch none of this PR's 12 files.

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

Criterion Verdict Evidence
AC1 (keep fresh): On a timer well under 3 days, the daemon refreshes the timestamps of every private lock / identity file it holds or owns for its lifetime (cohort daemon marker, cohort lease files, participant guard, IPC lifetime reservation, .sock.identity), so find -type f -atime +3 -mtime +3 -ctime +3 never matches them while the daemon lives. pass Timer: HOST_COORDINATION_TOUCH_MS=60000 (host.c:55-57); host_wait_for_lifetime touches when cbm_now_ms() >= next_touch, next_touch initialised to 0 so the first check is immediate (host.c:924, 959-960); the loop ticks every HOST_WAIT_TICK_MS via cbm_daemon_runtime_service_wait_exited (host.c:984). Complete enumeration of regular files the daemon holds/owns in the runtime dir, each checked against host_coordination_touch (host.c:900-917): (1) cbm-.lifetime.lock — listener->lifetime_reservation->fd/process_entry, ipc.c:3296-3298 REFRESHED; (2) cbm-.lock (POSIX participant guard, endpoint->lock_name, ipc.c:969, guard_try_join 3649-3672) — refreshed in all three places it can be held: host-retained guard via cbm_daemon_ipc_participant_guard_touch (host.c:912-914 -> ipc.c:3694-3700), listener-owned guard and service->owned_participant_guard via the guards[] loop (ipc.c:3299-3308, runtime.c:2581-2585); (3) cbm-.sock.identity — posix_identity_marker_touch, ipc.c:3258-3284 REFRESHED; (4)-(7) cbm-version-cohort-{daemon,lifetime,admission,maintenance}-v1.lock (version_cohort.c:47-50) — cbm_version_cohort_daemon_claim_touch + lease_touch walk claim->marker and lease->lifetime/admission/maintenance (version_cohort.c:381-395). Files deliberately NOT refreshed, verified not lifetime-held: cbm-.startup-v2.lock (released at end of startup, ipc.c:3115 / 3630-3647), cbm-.sock.pending (unlinked at publish, ipc.c:3079-3088). Not regular files and so outside AC1's own -type f scope: cbm-.sock and cbm-.anc (S_ISSOCK, ipc.c:2971-3046) — see contract_notes. Not in the runtime dir: the daemon/conflict logs (cache dir, host.c:127-142). Opt-in and non-coordinating: the CBM_DIAGNOSTICS mkdtemp dir (diagnostics.c:657-680, 264-276). futimens(fd, NULL) sets atime+mtime and updates ctime as an inode-metadata side effect, so all three clocks move.
AC2 (refresh through the held object): Refreshing must go through the held descriptor (or verify path inode == owned inode first), never blindly by path. pass private_file_lock.c:520-538 — cbm_private_file_lock_touch calls private_held_identity() (492-517: fstat st_nlink, lstat(directory->path) dev/ino, fstatat(dir_fd, base_name, AT_SYMLINK_NOFOLLOW) S_ISREG+dev/ino) and only then futimens(lock->fd, NULL). ipc.c:3243-3255 — posix_held_file_touch fstat(fd) then posix_path_names_inode() then futimens(fd, NULL). The only reopen-by-name is the identity marker, which is not held open: ipc.c:3258-3284 classifies by path FIRST (so a symlink or foreign inode is loss, not a refresh), reopens with O_RDONLY|O_CLOEXEC|O_NOFOLLOW|O_NONBLOCK, re-verifies st_dev/st_ino against listener->identity_device/identity_inode after the open (closing the open-race window), and only then futimens. marker_fd is closed on every return path (3282). No path-based utimensat anywhere in the diff.
AC3 (detect loss): On the same timer, the daemon detects that a held/owned file was unlinked or replaced (path no longer names the held inode). pass Loss is proven, never inferred: unlink -> st_nlink==0 (ipc.c:3247-3250, private_file_lock.c:503-505); rename-away/replacement -> fstatat ENOENT or dev/ino mismatch or non-S_ISREG (ipc.c:3236-3240, private_file_lock.c:513-517); directory replaced -> lstat dev/ino mismatch (private_file_lock.c:508-512, foundation path only — see finding F-01); every other errno -> -1/IO transient, not loss. Aggregation preserves precedence: posix_touch_fold (ipc.c:3228-3230) and version_cohort_lock_touch (version_cohort.c:369-378) both rank lost > transient > ok, and host_touch_note/host_cohort_touch_status (host.c:884-895) map OK->1, IO->-1, anything else->0 while visiting every file so one loss does not stop the others being refreshed.
AC4 (fail safe on loss): On detected loss the daemon logs a named event and performs a clean, orderly stop (not a crash / forced terminate), including permanent (daemon start) generations, so the next client starts a properly coordinated generation. It must not re-claim in place. pass host.c:962-966 logs cbm_log_warn("daemon.lifetime_end", "reason", "coordination_file_lost", "file", ) and returns cbm_daemon_runtime_service_stop(service, HOST_RUNTIME_SHUTDOWN_MS) — the same cooperative stop used by the stop_requested path at 955, with no dependence on permanent (the permanent short-circuit lives only in the unrelated initial-window condition at 952). No re-acquire/re-claim call exists anywhere in the new code. Verified the post-stop teardown cannot degrade into host_force_terminate: host_cohort_close/host_daemon_claim_close/host_participant_guard_close (host.c:1231-1233) loop through host_cleanup_release_until_complete, which force-terminates on timeout (host.c:212-216), so each release had to be checked against an already-unlinked file — cbm_private_file_lock_release (private_file_lock.c:563-582) does flock(LOCK_UN)+close with NO path revalidation; posix_named_lock_release (ipc.c:1084-1090) and posix_lifetime_lock_release (ipc.c:1277-1283) likewise. tests/test_version_cohort.c confirms this end to end: after unlink(marker) AND unlink(lifetime) it asserts daemon_claim_release == CBM_PRIVATE_FILE_LOCK_OK and then runs version_cohort_release/manager_close/fixture_finish. Author's e2e (orchestrator prompt) observed a clean stop in 59 s for the cohort marker and for .sock.identity, with the next daemon start succeeding.
AC5 (no regressions): Normal daemon lifetime, shutdown, idle/linger retirement, and startup handoff are unchanged when no file is lost; the heartbeat is cheap and does not race listener/lease teardown. pass Placement: the touch block sits after the EXITED/STOPPING/stop_requested branches (host.c:928-956, the STOPPING branch continues so no touch runs during shutdown) and before host_http_reconcile_at / reconcile_lifetime (979-983) — those two and the wait tick are byte-identical to origin/main. Cost per minute: ~10 fstat/fstatat plus futimens, one openat/close. No teardown race: service->listener is set once at runtime.c:2477 and cleared only at runtime.c:2726-2727 inside cbm_daemon_runtime_service_free, which runs on the host thread after the loop returns and after the accept thread is joined (2710-2714); the only other listener_close is runtime_service_destroy_unstarted (2378), before any thread starts. The accept thread reads service->listener concurrently (runtime.c:2292) but cbm_daemon_ipc_accept mutates no POSIX listener field, and all fields the touch reads (dir_fd, identity_device/inode, lifetime_reservation, participant_guard) are immutable after publish. Host-retained cohort_lease/daemon_claim/participant_guard pointers are host-thread-local (host.c:1050/1075/1084, released at 1231-1233). No behavioural coupling to timestamps: grep -rn 'st_mtim|st_atim|st_ctim' over src/ shows no age- or timestamp-based logic in ipc.c or version_cohort.c at all; runtime.c:694-751 compares mtime/ctime only for the process executable image, never for runtime-dir files, so refreshing cannot perturb stale-generation classification. New signature host_wait_for_lifetime(..., const host_coordination_t *) has exactly one caller (host.c:1205); cbm_daemon_runtime_service_touch_listener and the two ipc/cohort touch entry points have no other callers.
AC6 (portability): Builds and behaves correctly on Windows (no age-based cleaner there; entry points must exist and not falsely report loss) and Linux. partial Entry points exist on all three platforms and none falsely reports loss by construction: Windows cbm_daemon_ipc_listener_touch returns listener ? 1 : 0 and cbm_daemon_ipc_participant_guard_touch returns endpoint && guard ? 1 : 0 (ipc.c:6845-6857); Windows cbm_private_file_lock_touch (private_file_lock.c:1511-1529) treats a failed GetFileInformationByHandle as IO/transient and only nNumberOfLinks==0 as UNSAFE. The POSIX code uses no macOS-only APIs (fstatat/futimens/openat are POSIX.1-2008), and the two new tests guard with #ifdef _WIN32 SKIP_PLATFORM. I marked this partial rather than pass because I can only verify the source here — I did not build for Windows or Linux. The orchestrator reports fork CI on 1133ba3 fully green including windows-guards (flake, green on rerun); that is the evidence for the build half of this criterion, not a local verification.
AC7 (tests): Automated tests cover refresh-through-fd and loss detection, without sleeps or wall-clock-flaky assertions. pass tests/test_private_file_lock.c: private_file_lock_touch_refreshes_held_file_and_detects_unlink (touch==OK, atime/mtime > stale, then unlink -> UNSAFE, then a successor acquire succeeds) and private_file_lock_touch_detects_rename_away (still-linked but renamed -> UNSAFE, the case a bare st_nlink check would miss). tests/test_daemon_ipc.c daemon_ipc_listener_touch_detects_lost_runtime_files: a 5-case table over lifetime.lock (unlink), .lock (unlink, listener-owned guard), .sock.identity (unlink / replaced by fresh inode / replaced by symlink), each asserting touch==1 + timestamps refreshed while intact and touch==0 after loss, plus the chmod-0 marker case asserting -1 (transient, root-guarded with geteuid()==0 ? 1 : -1). tests/test_version_cohort.c version_cohort_touch_reports_each_lost_file ages the marker and lifetime locks via utimensat and asserts per-file UNSAFE isolation. No sleep()/usleep in any of them; aging uses an explicit epoch constant (1000000000, i.e. 2001) so refreshed.st_mtime > stale cannot be wall-clock-flaky. Not covered (accepted in round 2 as item 3): the external_guard argument of cbm_daemon_ipc_listener_touch and the standalone cbm_daemon_ipc_participant_guard_touch, both thin delegates to posix_held_lock_touch which the 'lock' case does exercise.

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 SKIP_PLATFORM).

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

  • Area: src/daemon/host.c (lines 898–978)
  • What was tested: grep for cbm_daemon_runtime_service_touch_listener, cbm_daemon_ipc_participant_guard_touch, host_coordination_touch across tests/ and src/ returns hits only in src/ (host.c, runtime.c/.h, ipc.c/.h). The only tests reaching the host loop are the three cbm_daemon_host_run call sites: tests/test_daemon_runtime.c:1470, tests/test_daemon_runtime.c:1638, tests/test_daemon_bootstrap.c:928.
  • Expected: An automated test that removes a held runtime file under a running host loop and asserts the daemon logs daemon.lifetime_end reason=coordination_file_lost and stops cleanly (AC4), plus coverage of the degraded/recovered log transition (host.c:971-977) and the 60s cadence.
  • Actual / Risk: Only the healthy first-tick path is smoke-covered: because next_touch starts at 0, a touch that spuriously returned 0 would stop those host_run daemons immediately and break the tests, so a false-positive regression is caught. But a false-negative regression (loss no longer detected, or host_touch_note's fold at host.c:888-894 picking the wrong branch, or the lost->stop return being dropped) would pass the entire suite. The stop-on-loss behaviour - the whole point of 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 DeusData/codebase-memory-mcp#2178 - is verified only by the author's manual e2e. Additionally the production listener has participant_guard == NULL (host -> cbm_daemon_runtime_service_start_reserved -> cbm_daemon_ipc_listen_reserved; owned_participant_guard is set only in cbm_daemon_runtime_service_start, runtime.c:2535), so the IPC test's 'lock' case exercises a listener-owned guard the daemon never owns; the daemon's real guard goes through cbm_daemon_ipc_participant_guard_touch(endpoint->dir_fd, ...) (ipc.c:3691-3698), which is the same posix_held_lock_touch logic against a different directory fd and is untested. No acceptance criterion functionally fails.
  • Severity: minor | Status: confirmed | Relevance: contract
  • Resolution: Accepted. host_wait_for_lifetime is static with no seam; stop-on-loss is covered by e2e (marker unlink, identity unlink, cohort-lifetime rename-away, identity symlink — each a clean stop within one tick, with the next daemon start succeeding). The lens notes the healthy first-tick path IS smoke-covered by the three cbm_daemon_host_run tests, so a false-positive regression (spurious stop) would fail the suite.

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

  • Area: src/daemon/ipc.c (lines 3232–3268)
  • What was tested: Read the case table in tests/test_daemon_ipc.c daemon_ipc_listener_touch_detects_lost_runtime_files: LOSS_REPLACE_FILE and LOSS_REPLACE_SYMLINK are applied only to the 'sock.identity' suffix; 'lifetime.lock' and 'lock' use LOSS_UNLINK only. Unlink of a held file sets st_nlink==0, which posix_held_file_touch short-circuits on at ipc.c:3256 before ever calling posix_path_names_inode.
  • Expected: At least one held-lock case where the file is renamed away or replaced while the fd is still open, so the path-vs-inode comparison in posix_held_file_touch is what produces the 0 - mirroring tests/test_private_file_lock.c private_file_lock_touch_detects_rename_away, which does exactly this for the sibling implementation in private_file_lock.c.
  • Actual / Risk: Deleting the posix_path_names_inode call from posix_held_file_touch (ipc.c:3253-3258) and returning 1 whenever st_nlink != 0 passes the whole suite unchanged. That defence-in-depth branch (rename-away / replacement of a held lifetime reservation or participant guard, which AC2 and AC3 call out explicitly) would silently rot. macOS tmp_cleaner itself unlinks, so the tested path covers the reported failure mode; the risk is regression in the broader loss detection the PR advertises.
  • Severity: minor | Status: confirmed | Relevance: contract
  • Resolution: Accepted (advisory). posix_path_names_inode is reached for held locks only on rename/replace, which tmp_cleaner never does (it unlinks, and unlink is covered). The sibling implementation's path-vs-inode branch is covered by private_file_lock_touch_detects_rename_away.

Finding 3: [claude] private_held_identity directory-replacement and undetermined (-1) branches have no test

  • Area: src/foundation/private_file_lock.c (lines 496–518)
  • What was tested: Read both new tests in tests/test_private_file_lock.c. They cover: intact -> OK, unlink -> UNSAFE, rename-away -> UNSAFE. Nothing replaces or removes the lock directory itself, and nothing drives fstat/lstat/fstatat to a non-ENOENT errno.
  • Expected: A case that replaces fixture.directory's path with a different directory (or removes it) and asserts UNSAFE, exercising the lstat(directory->path) ENOENT/ENOTDIR -> lost and the S_ISDIR / dev / ino mismatch -> lost branches added in round 2; and, ideally, one case proving an undetermined failure maps to IO rather than UNSAFE (the distinction round 2 was specifically about, since UNSAFE stops the daemon and IO does not).
  • Actual / Risk: The round-2 lost-vs-transient classification is the thing most likely to regress into a spurious daemon stop, and its directory-level half plus every -1 path is untested. A mutation turning any 'return -1' into 'return 0' in private_held_identity would pass the suite while making a transient fstat/lstat failure (EIO, ESTALE, ENOMEM) terminate a healthy daemon. Same shape applies to the corresponding -1 returns in ipc.c posix_path_names_inode/posix_held_file_touch; only the identity marker's EACCES-on-reopen transient case is covered (the chmod-0 preamble in the IPC test).
  • Severity: minor | Status: confirmed | Relevance: contract
  • Resolution: Accepted (advisory). Directory replacement and the undetermined (-1) branches are untested. The EACCES-on-reopen transient path IS covered (the chmod-0 case), and directory replacement is covered in practice because the cohort locks live in the same directory and private_held_identity classifies it as loss.

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

  • Area: src/daemon/ipc.c (lines 3233–3313)
  • What was tested: Compared the loss-detection logic of the two new touch implementations. private_held_identity() (private_file_lock.c:492-517) proves identity in three steps: fstat(fd).st_nlink, lstat(directory->path) + S_ISDIR + dev/ino compare against the adopted directory identity, then fstatat(directory->fd, base_name, NOFOLLOW). The ipc.c equivalents — posix_path_names_inode (ipc.c:3233-3241), posix_held_file_touch (3243-3255), posix_identity_marker_touch (3258-3284), cbm_daemon_ipc_listener_touch (3286-3313) and cbm_daemon_ipc_participant_guard_touch (3694-3700) — resolve only relative to the cached listener->dir_fd / endpoint->dir_fd and never call endpoint_runtime_still_valid() nor compare listener->dir_device/dir_inode, although both are stored (struct at ipc.c:484-497) and endpoint_runtime_still_valid() is used in every other lock path in this file (e.g. 1190, 1209, 1229, 1237, 1251, 1266, 1335).
  • Expected: If the runtime directory itself is replaced (the daemon's dir_fd still resolves to the now-unreachable old inode), the lifetime reservation, participant guard and identity marker coordinate nothing — the published path resolves into a fresh directory where a peer creates fresh locks. Per AC3 that is 'replaced', and the foundation implementation classifies it as loss.
  • Actual / Risk: These entry points return 1 (refreshed) in that case. No daemon-level failure scenario exists today: host_coordination_touch (host.c:900-917) also calls cbm_version_cohort_daemon_claim_touch/lease_touch, whose locks live in the SAME directory (ipc.c:1077 adopts endpoint->runtime_dir as the cohort private lock directory), so private_held_identity's lstat(directory->path) check catches directory replacement and returns UNSAFE -> the daemon stops. The gap is a hardening asymmetry that bites only a caller who uses cbm_daemon_ipc_participant_guard_touch or cbm_daemon_ipc_listener_touch standalone, without a cohort touch alongside. Reported because the PR introduced this code, not demoted to observation.
  • Severity: minor | Status: hypothetical | Relevance: regression
  • Resolution: Accepted (hypothetical). The ipc touch resolves against the cached dir_fd and does not re-check runtime-directory identity. The lens confirms no reachable daemon-level failure: host_coordination_touch always also calls the cohort touches, whose locks are in the same directory, so a replaced directory is caught there and stops the daemon. Recorded as a hardening asymmetry for a future standalone caller.

Finding 5: [claude] Identity-marker LOSS_REPLACE_FILE case can false-pass on filesystems that reuse inode numbers

  • Area: tests/test_daemon_ipc.c (lines 2143–2152)
  • What was tested: Read the loss simulation: for case index 3 the test unlinks the marker and immediately re-creates it with open(O_CREAT|O_EXCL), then asserts cbm_daemon_ipc_listener_touch == 0. Unlike every other case, the identity marker is not held open by the listener (ipc.c:3269-3271 comment: 'The marker is not held open'), so the unlink frees its inode immediately. posix_path_names_inode compares only (st_dev, st_ino) against listener->identity_device/identity_inode.
  • Expected: Loss detection that does not depend on the filesystem's inode-allocation policy - e.g. creating the replacement with distinct content/size, or creating a second file first so the freed inode number is consumed, or asserting the replacement's st_ino differs before calling touch.
  • Actual / Risk: On a filesystem that recycles freed inode numbers (ext4, xfs - GitHub-hosted Linux runners put /tmp on the ext4 root), the replacement can land on the same inode number, posix_path_names_inode returns 1, the marker is refreshed and touched_after_loss[3] becomes 1 instead of 0 - an intermittent CI failure with no code change. Immune on tmpfs, APFS and btrfs, which is consistent with the green runs so far. Not reproduced; reported as a flakiness vector because AC7 names flaky assertions explicitly. Note this is a test-only artifact: in the real 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 DeusData/codebase-memory-mcp#2178 sequence the replacement is created by a live peer that holds it, so a same-inode collision cannot silently hide the loss in production.
  • Severity: minor | Status: hypothetical | Relevance: contract
  • Resolution: Accepted (hypothetical). Inode reuse could in principle let the replace-file case pass for the wrong reason. The symlink case, which cannot be affected by inode reuse, covers the same classification.

Finding 6: [claude] Windows DeusData#2178 heartbeat is unverified beyond compilation - all three new tests SKIP_PLATFORM on _WIN32

  • Area: src/foundation/private_file_lock.c (lines 1511–1528)
  • What was tested: All three new tests open with '#ifdef _WIN32 SKIP_PLATFORM(...)'. The Windows cbm_private_file_lock_touch is not a stub - it has real logic (GetFileInformationByHandle, nNumberOfLinks == 0 -> UNSAFE, query failure -> IO) and is reachable on Windows through cbm_version_cohort_lease_touch / cbm_version_cohort_daemon_claim_touch, which the host loop calls every 60s on Windows too. Cross-lens: the contract-security lens recorded AC6 as 'partial' for the same reason (source-only verification, no local Windows/Linux build); regression-edges recorded AC6 as pass on the strength of the Windows stubs not being able to report false loss.
  • Expected: At least one Windows-executed assertion that cbm_private_file_lock_touch on a freshly acquired lock returns OK (AC6's 'must not falsely report loss'), rather than relying solely on the code compiling.
  • Actual / Risk: Low: the lock handle is opened with GENERIC_READ|GENERIC_WRITE|READ_CONTROL (private_file_lock.c:1346), which implies FILE_READ_ATTRIBUTES, so GetFileInformationByHandle should succeed and nNumberOfLinks should be 1. But if that ever regressed, the symptom on Windows is either a permanent daemon.coordination_touch_failed log (query failure -> IO) or, in the worst case, a healthy daemon stopping itself every generation - and no test would catch either. Also note the Windows touch, unlike POSIX, does not check owner_pid, so a forked/inherited handle is not guarded; there is no fork on Windows, so this is not reachable.
  • Severity: minor | Status: hypothetical | Relevance: contract
  • Resolution: Accepted. Same as AC6 above; unchanged since round 1.

Summary

Severity Count
Critical 0
Major 0
Minor 6
Total 6

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 Change

None.

Verification

  • Local: affected suites 83 passed / 3 skipped under ASan/UBSan; full suite 8111 passed with the one known test_daemon_runtime.c:3938 flake (fails ~4 of 6 runs on plain upstream too); integration steps 5–6 pass when run directly.
  • Fork CI on 1133ba3: fully green, including codeql-gate after the fork's 21 stale alerts were dismissed. test-windows-guards (section_cold_storm) and test-lsan-macos (subprocess_cancel_grace_is_hard_capped) each failed once and passed on rerun — both are known-flaky and neither is reachable from this diff.
  • e2e: unreadable marker → keeps serving + coordination_touch_failed; restore → coordination_touch_recovered; marker unlink / cohort-lifetime rename-away / identity symlink → clean stop with coordination_file_lost, next daemon start OK.

NEW SAST findings (GitHub code scanning, open alerts on fix/cohort-lock-tmp-cleaner)

Open code-scanning alerts on the PR head ref, filtered at severity floor high (total open alerts at any severity: 0). A finding here is present on the PR branch — the reviewer should weigh each as either intentional (e.g. a deliberate new dependency with a tracked CVE) or a regression to fix. GitHub does not expose a per-job baseline the way GitLab artifacts do, so this is an absolute snapshot of the head ref, not a strict delta vs the base branch.

No open code-scanning alerts at or above severity high on fix/cohort-lock-tmp-cleaner.

Dependabot (SCA) — advisory, repo-wide

Dependabot alerts unavailable (not enabled, or token lacks security_events scope). Skipped — non-blocking.

Security-related PR checks

CodeQL	pass	2s	https://github.com/halindrome/codebase-memory-mcp/runs/106851289103	
security / codeql-gate	pass	43m35s	https://github.com/halindrome/codebase-memory-mcp/actions/runs/35754244910/job/106865790746	
security / license-gate	pass	3m57s	https://github.com/halindrome/codebase-memory-mcp/actions/runs/35754244910/job/106865834542	
security / security-static	pass	29s	https://github.com/halindrome/codebase-memory-mcp/actions/runs/35754244910/job/106865790526	

Note: GitHub code scanning is an absolute snapshot of open alerts on the head ref, not a strict NEW-vs-baseline delta. The reviewer should treat each finding as either intentional (and recommend dismissing the alert with a reason) or a regression (and recommend a fix).


QA performed by Claude Code (claude-opus-5)

@halindrome

Copy link
Copy Markdown
Owner Author

QA complete (3 rounds, round 3 clean). Upstream draft PR opened: DeusData#2289, with these three round reports carried over. This fork PR stays open as the QA trail and is not for merging.

shanemccarron-maker and others added 3 commits September 25, 2026 14:24
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>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants