From 9e793df87b51f07d3741c55ea6e27367ee1a7b7d Mon Sep 17 00:00:00 2001 From: Shane McCarron Date: Mon, 21 Sep 2026 11:48:06 -0500 Subject: [PATCH 1/6] fix(daemon): keep held runtime files fresh and exit when one is lost 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 #2178 Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Shane McCarron --- src/daemon/host.c | 57 ++++++++++++++++++- src/daemon/ipc.c | 57 +++++++++++++++++++ src/daemon/ipc.h | 13 +++++ src/daemon/runtime.c | 4 ++ src/daemon/runtime.h | 4 ++ src/daemon/version_cohort.c | 19 +++++++ src/daemon/version_cohort.h | 9 +++ src/foundation/private_file_lock.c | 32 +++++++++++ src/foundation/private_file_lock_internal.h | 7 +++ tests/test_daemon_ipc.c | 56 ++++++++++++++++-- tests/test_private_file_lock.c | 63 ++++++++++++++++++++- 11 files changed, 314 insertions(+), 7 deletions(-) diff --git a/src/daemon/host.c b/src/daemon/host.c index 3e509e207..856af3ee4 100644 --- a/src/daemon/host.c +++ b/src/daemon/host.c @@ -52,6 +52,9 @@ enum { HOST_HTTP_RETRY_INITIAL_MS = 1000, HOST_HTTP_RETRY_MAX_MS = 30000, HOST_WATCH_INTERVAL_MS = 5000, + /* Age-based temp cleaners need days of inactivity (macOS tmp_cleaner: 3); + * a minute bounds the blind window after a long sleep (#2178). */ + HOST_COORDINATION_TOUCH_MS = 60000, HOST_CONFLICT_LOG_CAP = 1024 * 1024, HOST_OPERATION_LOG_CAP = 5 * 1024 * 1024, HOST_PATH_CAP = 4096, @@ -868,10 +871,42 @@ static bool host_background_start(host_state_t *host) { return true; } +/* Coordination handles the host retains for the generation's whole lifetime. */ +typedef struct { + const cbm_daemon_ipc_endpoint_t *endpoint; + cbm_version_cohort_lease_t *cohort_lease; + cbm_version_cohort_daemon_claim_t *daemon_claim; + cbm_daemon_ipc_participant_guard_t *participant_guard; +} host_coordination_t; + +/* Refresh every long-held runtime file and name the first one found lost, or + * return NULL. A held lock whose path now names another inode coordinates + * nothing: forked index workers read the daemon as uncoordinated and a peer + * can start a second generation (#2178). */ +static const char *host_coordination_touch(const host_coordination_t *coordination, + cbm_daemon_runtime_service_t *service) { + if (!cbm_version_cohort_daemon_claim_touch(coordination->daemon_claim)) { + return "cohort_daemon_claim"; + } + if (!cbm_version_cohort_lease_touch(coordination->cohort_lease)) { + return "cohort_lease"; + } + if (!cbm_daemon_ipc_participant_guard_touch(coordination->endpoint, + coordination->participant_guard)) { + return "participant_guard"; + } + if (!cbm_daemon_runtime_service_touch_listener(service)) { + return "listener"; + } + return NULL; +} + static bool host_wait_for_lifetime(cbm_daemon_runtime_service_t *service, - atomic_int *stop_requested, host_state_t *host, bool permanent) { + atomic_int *stop_requested, host_state_t *host, bool permanent, + const host_coordination_t *coordination) { uint64_t initial_deadline = cbm_now_ms() + HOST_INITIAL_CLIENT_TIMEOUT_MS; uint64_t stopping_deadline = 0; + uint64_t next_touch = 0; for (;;) { cbm_daemon_runtime_service_state_t state = cbm_daemon_runtime_service_state(service); if (state == CBM_DAEMON_RUNTIME_SERVICE_EXITED) { @@ -903,6 +938,17 @@ static bool host_wait_for_lifetime(cbm_daemon_runtime_service_t *service, stop ? "stop_requested" : "initial_window_expired"); return cbm_daemon_runtime_service_stop(service, HOST_RUNTIME_SHUTDOWN_MS); } + /* Permanent generations too: exiting lets the next client start a + * coordinated generation, while re-claiming would race that client. */ + if (cbm_now_ms() >= next_touch) { + next_touch = cbm_now_ms() + HOST_COORDINATION_TOUCH_MS; + const char *lost = host_coordination_touch(coordination, service); + if (lost) { + cbm_log_warn("daemon.lifetime_end", "reason", "coordination_file_lost", "file", + lost); + return cbm_daemon_runtime_service_stop(service, HOST_RUNTIME_SHUTDOWN_MS); + } + } host_http_reconcile_at(host, cbm_now_ms(), false); /* Retire an ephemeral generation that lingered for cold-storm cohort * participants once they drain, or once its bounded linger elapses. @@ -1123,7 +1169,14 @@ int cbm_daemon_host_run(const cbm_daemon_host_config_t *config) { : "unavailable", "memory_budget_bytes", memory_budget, "physical_job_limit", physical_job_limit, "worker_memory_budget_bytes", worker_memory_budget); - if (!host_wait_for_lifetime(service, config->stop_requested, &host, config->permanent)) { + host_coordination_t coordination = { + .endpoint = config->endpoint, + .cohort_lease = cohort_lease, + .daemon_claim = daemon_claim, + .participant_guard = participant_guard, + }; + if (!host_wait_for_lifetime(service, config->stop_requested, &host, config->permanent, + &coordination)) { host_force_terminate("runtime"); } diff --git a/src/daemon/ipc.c b/src/daemon/ipc.c index a92befb08..53da43542 100644 --- a/src/daemon/ipc.c +++ b/src/daemon/ipc.c @@ -3220,6 +3220,46 @@ void cbm_daemon_ipc_listener_close(cbm_daemon_ipc_listener_t *listener) { free(listener); } +/* Touch through the held descriptor only after proving the path still names + * it: refreshing by path would keep a replacement inode fresh and hide the + * loss (#2178). */ +static bool posix_held_file_touch(int directory_fd, const char *base_name, int fd) { + return private_regular_file_snapshot(directory_fd, base_name, fd, 1, NULL) && + futimens(fd, NULL) == 0; +} + +static bool posix_held_lock_touch(int directory_fd, const process_lock_entry_t *entry, int fd) { + return entry && posix_held_file_touch(directory_fd, entry->lock_name, fd); +} + +bool cbm_daemon_ipc_listener_touch(cbm_daemon_ipc_listener_t *listener) { + if (!listener || listener->dir_fd < 0 || listener->owner_pid != getpid() || + !listener->lifetime_reservation) { + return false; + } + bool lifetime = + posix_held_lock_touch(listener->dir_fd, listener->lifetime_reservation->process_entry, + listener->lifetime_reservation->fd); + const cbm_daemon_ipc_participant_guard_t *guard = listener->participant_guard; + bool participant = + !guard || + posix_held_lock_touch(listener->dir_fd, guard->legacy_process_entry, guard->legacy_fd); + /* The marker is not held open; reopen it and require the published inode. */ + int marker_fd = openat(listener->dir_fd, listener->socket_identity_name, + O_RDONLY | O_CLOEXEC | O_NOFOLLOW | O_NONBLOCK); + struct stat marker_status; + bool marker = marker_fd >= 0 && + private_regular_file_snapshot(listener->dir_fd, listener->socket_identity_name, + marker_fd, 1, &marker_status) && + marker_status.st_dev == listener->identity_device && + marker_status.st_ino == listener->identity_inode && + futimens(marker_fd, NULL) == 0; + if (marker_fd >= 0) { + (void)close(marker_fd); + } + return lifetime && participant && marker; +} + int cbm_daemon_ipc_accept(cbm_daemon_ipc_listener_t *listener, uint32_t timeout_ms, cbm_daemon_ipc_connection_t **connection_out) { if (connection_out) { @@ -3603,6 +3643,12 @@ bool cbm_daemon_ipc_participant_guard_release(cbm_daemon_ipc_participant_guard_t return true; } +bool cbm_daemon_ipc_participant_guard_touch(const cbm_daemon_ipc_endpoint_t *endpoint, + cbm_daemon_ipc_participant_guard_t *guard) { + return endpoint && guard && guard->owner_pid == getpid() && + posix_held_lock_touch(endpoint->dir_fd, guard->legacy_process_entry, guard->legacy_fd); +} + int cbm_daemon_ipc_local_transition_try_acquire( const cbm_daemon_ipc_endpoint_t *endpoint, cbm_daemon_ipc_local_transition_t **transition_out) { if (transition_out) { @@ -6746,6 +6792,17 @@ bool cbm_daemon_ipc_participant_guard_release(cbm_daemon_ipc_participant_guard_t return true; } +/* Windows has no age-based cleaner of the private runtime directory, and its + * held handles deny deletion; the #2178 heartbeat has nothing to refresh. */ +bool cbm_daemon_ipc_listener_touch(cbm_daemon_ipc_listener_t *listener) { + return listener != NULL; +} + +bool cbm_daemon_ipc_participant_guard_touch(const cbm_daemon_ipc_endpoint_t *endpoint, + cbm_daemon_ipc_participant_guard_t *guard) { + return endpoint && guard; +} + int cbm_daemon_ipc_local_transition_try_acquire( const cbm_daemon_ipc_endpoint_t *endpoint, cbm_daemon_ipc_local_transition_t **transition_out) { if (transition_out) { diff --git a/src/daemon/ipc.h b/src/daemon/ipc.h index 545ecb357..ed214ea79 100644 --- a/src/daemon/ipc.h +++ b/src/daemon/ipc.h @@ -76,6 +76,15 @@ cbm_daemon_ipc_listener_t *cbm_daemon_ipc_listen_reserved( cbm_daemon_ipc_lifetime_reservation_t **reservation_io); void cbm_daemon_ipc_listener_close(cbm_daemon_ipc_listener_t *listener); +/* Heartbeat for the daemon's long-lived runtime artifacts: refresh the held + * lifetime reservation, the listener's participant guard (if it owns one) and + * the published identity marker so an age-based temp cleaner never deletes + * them (#2178). Returns false once any of them no longer names the file this + * listener owns: a lost lifetime file lets a peer start a second generation, + * and a lost identity marker makes listener close leave the socket pair + * behind. Only the owning thread may call it while the listener is open. */ +bool cbm_daemon_ipc_listener_touch(cbm_daemon_ipc_listener_t *listener); + /* Create or validate one private current-user directory at an already * canonical local path. Ancestors are handle-validated without mutation: * POSIX permits only root/current-user owners and no group/other write (apart @@ -270,6 +279,10 @@ bool cbm_daemon_ipc_startup_lock_release(cbm_daemon_ipc_startup_lock_t **lock_io int cbm_daemon_ipc_participant_guard_try_join(const cbm_daemon_ipc_endpoint_t *endpoint, cbm_daemon_ipc_participant_guard_t **guard_out); bool cbm_daemon_ipc_participant_guard_release(cbm_daemon_ipc_participant_guard_t **guard_io); +/* Same heartbeat contract as cbm_daemon_ipc_listener_touch, for a guard the + * caller retains itself. */ +bool cbm_daemon_ipc_participant_guard_touch(const cbm_daemon_ipc_endpoint_t *endpoint, + cbm_daemon_ipc_participant_guard_t *guard); /* Standalone CLI work joins the legacy-compatible current participant group * without becoming a daemon client. Acquisition retains startup-v2 only for diff --git a/src/daemon/runtime.c b/src/daemon/runtime.c index efa41a020..7fb7d32a9 100644 --- a/src/daemon/runtime.c +++ b/src/daemon/runtime.c @@ -2578,6 +2578,10 @@ size_t cbm_daemon_runtime_service_active_connections(cbm_daemon_runtime_service_ return count; } +bool cbm_daemon_runtime_service_touch_listener(cbm_daemon_runtime_service_t *service) { + return service && service->listener && cbm_daemon_ipc_listener_touch(service->listener); +} + void cbm_daemon_runtime_service_reconcile_lifetime(cbm_daemon_runtime_service_t *service) { if (!service) { return; diff --git a/src/daemon/runtime.h b/src/daemon/runtime.h index 7a31dab61..304d7c898 100644 --- a/src/daemon/runtime.h +++ b/src/daemon/runtime.h @@ -336,6 +336,10 @@ size_t cbm_daemon_runtime_service_active_connections(cbm_daemon_runtime_service_ * an unbounded idle hang. The host lifetime loop calls this every tick; it is a * no-op unless a last-committed-client linger is armed. */ void cbm_daemon_runtime_service_reconcile_lifetime(cbm_daemon_runtime_service_t *service); +/* #2178 heartbeat over the listener's runtime artifacts; see + * cbm_daemon_ipc_listener_touch. Call only from the thread that stops and + * frees the service, which is the only thread that closes the listener. */ +bool cbm_daemon_runtime_service_touch_listener(cbm_daemon_runtime_service_t *service); size_t cbm_daemon_runtime_service_job_subscribers(cbm_daemon_runtime_service_t *service, const char *project_key); uint64_t cbm_daemon_runtime_service_client_process_id(cbm_daemon_runtime_service_t *service, diff --git a/src/daemon/version_cohort.c b/src/daemon/version_cohort.c index 3742a1330..82cc63b32 100644 --- a/src/daemon/version_cohort.c +++ b/src/daemon/version_cohort.c @@ -363,6 +363,25 @@ cbm_private_file_lock_status_t cbm_version_cohort_lease_release( return result; } +static bool version_cohort_lock_touch(cbm_private_file_lock_t *lock) { + return !lock || cbm_private_file_lock_touch(lock) == CBM_PRIVATE_FILE_LOCK_OK; +} + +bool cbm_version_cohort_lease_touch(cbm_version_cohort_lease_t *lease) { + if (!lease) { + return false; + } + /* Evaluate all three so one lost file does not leave the others to age. */ + bool lifetime = version_cohort_lock_touch(lease->lifetime); + bool admission = version_cohort_lock_touch(lease->admission); + bool maintenance = version_cohort_lock_touch(lease->maintenance); + return lifetime && admission && maintenance; +} + +bool cbm_version_cohort_daemon_claim_touch(cbm_version_cohort_daemon_claim_t *claim) { + return claim && claim->marker && version_cohort_lock_touch(claim->marker); +} + static cbm_version_cohort_status_t version_cohort_failed(cbm_version_cohort_lease_t *lease, cbm_version_cohort_status_t status, cbm_version_cohort_lease_t **lease_out) { diff --git a/src/daemon/version_cohort.h b/src/daemon/version_cohort.h index 51304cc95..ac79c8b86 100644 --- a/src/daemon/version_cohort.h +++ b/src/daemon/version_cohort.h @@ -145,6 +145,15 @@ cbm_version_cohort_daemon_presence_t cbm_version_cohort_daemon_presence_under_tr cbm_private_file_lock_status_t cbm_version_cohort_lease_release( cbm_version_cohort_lease_t **lease_io); +/* Heartbeat for a long-lived holder: refresh every lock file the lease or + * claim holds and report false once any held file was unlinked or replaced. + * An age-based temp cleaner deleting a held file silently ends coordination, + * because the next participant creates and locks a fresh inode (#2178). The + * only safe response to false is an orderly exit; re-claiming would race + * that participant. */ +bool cbm_version_cohort_lease_touch(cbm_version_cohort_lease_t *lease); +bool cbm_version_cohort_daemon_claim_touch(cbm_version_cohort_daemon_claim_t *claim); + /* Refuses teardown while a lease or retryable cleanup handle remains. */ cbm_private_file_lock_status_t cbm_version_cohort_manager_free( cbm_version_cohort_manager_t **manager_io); diff --git a/src/foundation/private_file_lock.c b/src/foundation/private_file_lock.c index 80df0d17c..a7d83172e 100644 --- a/src/foundation/private_file_lock.c +++ b/src/foundation/private_file_lock.c @@ -482,6 +482,24 @@ cbm_private_file_lock_status_t cbm_private_file_lock_payload_write(cbm_private_f return valid ? CBM_PRIVATE_FILE_LOCK_OK : CBM_PRIVATE_FILE_LOCK_IO; } +cbm_private_file_lock_status_t cbm_private_file_lock_touch(cbm_private_file_lock_t *lock) { + if (!lock) { + return CBM_PRIVATE_FILE_LOCK_UNSAFE; + } + if (!cbm_private_file_lock_fork_guard_enter()) { + return CBM_PRIVATE_FILE_LOCK_IO; + } + /* An unlinked file keeps its inode alive for this handle with st_nlink 0, + * which the payload validity check already rejects. */ + bool linked = private_payload_fd_valid(lock, NULL); + bool touched = linked && futimens(lock->fd, NULL) == 0; + cbm_private_file_lock_fork_guard_leave(); + if (!linked) { + return CBM_PRIVATE_FILE_LOCK_UNSAFE; + } + return touched ? CBM_PRIVATE_FILE_LOCK_OK : CBM_PRIVATE_FILE_LOCK_IO; +} + cbm_private_file_lock_status_t cbm_private_file_lock_release(cbm_private_file_lock_t **lock_io) { if (!lock_io || !*lock_io) { return CBM_PRIVATE_FILE_LOCK_IO; @@ -1450,6 +1468,20 @@ cbm_private_file_lock_status_t cbm_private_file_lock_payload_write(cbm_private_f return valid ? CBM_PRIVATE_FILE_LOCK_OK : CBM_PRIVATE_FILE_LOCK_IO; } +cbm_private_file_lock_status_t cbm_private_file_lock_touch(cbm_private_file_lock_t *lock) { + /* Windows has no age-based cleaner of the private lock directory; only + * confirm the held handle still names a linked file. */ + if (!lock) { + return CBM_PRIVATE_FILE_LOCK_UNSAFE; + } + if (!cbm_private_file_lock_fork_guard_enter()) { + return CBM_PRIVATE_FILE_LOCK_IO; + } + bool linked = private_win_payload_handle_valid(lock); + cbm_private_file_lock_fork_guard_leave(); + return linked ? CBM_PRIVATE_FILE_LOCK_OK : CBM_PRIVATE_FILE_LOCK_UNSAFE; +} + static bool private_win_release_unlock(cbm_private_file_lock_t *lock) { lock->test_unlock_attempts++; if (lock->test_fail_unlock_once) { diff --git a/src/foundation/private_file_lock_internal.h b/src/foundation/private_file_lock_internal.h index 8801d9b1a..b8ec34ac7 100644 --- a/src/foundation/private_file_lock_internal.h +++ b/src/foundation/private_file_lock_internal.h @@ -61,6 +61,13 @@ cbm_private_file_lock_status_t cbm_private_file_lock_payload_write(cbm_private_f const void *buffer, size_t length); +/* Long-lived holders call this periodically. It refreshes the held file's + * timestamps through the validated handle so age-based temp cleaners (macOS + * tmp_cleaner, systemd-tmpfiles) never select it, and returns UNSAFE once the + * handle no longer names a linked file: a peer opening the path would then + * lock a different inode and the lock coordinates nothing (#2178). */ +cbm_private_file_lock_status_t cbm_private_file_lock_touch(cbm_private_file_lock_t *lock); + /* Forces the next successfully acquired native lock down the post-lock * validation cleanup path and injects pre-call release failures there. */ bool cbm_private_lock_directory_fail_post_acquire_cleanup_for_test( diff --git a/tests/test_daemon_ipc.c b/tests/test_daemon_ipc.c index b451f9bb3..d29a1e6c0 100644 --- a/tests/test_daemon_ipc.c +++ b/tests/test_daemon_ipc.c @@ -1004,8 +1004,8 @@ TEST(daemon_ipc_windows_sid_trust_accepts_local_admin_rejects_foreign_500) { (void)CreateWellKnownSid(WinBuiltinAdministratorsSid, NULL, NULL, &builtin_needed); if (builtin_needed > 0) { builtin_admins = malloc(builtin_needed); - if (builtin_admins && - !CreateWellKnownSid(WinBuiltinAdministratorsSid, NULL, builtin_admins, &builtin_needed)) { + if (builtin_admins && !CreateWellKnownSid(WinBuiltinAdministratorsSid, NULL, builtin_admins, + &builtin_needed)) { free(builtin_admins); builtin_admins = NULL; } @@ -2063,6 +2063,54 @@ TEST(daemon_ipc_lifetime_reservation_survives_saturated_second_listen) { PASS(); } +/* #2178: an age-based temp cleaner unlinked the live daemon's lifetime lock + * and identity marker. The heartbeat must report either loss, so the daemon + * exits instead of serving under a lifetime file that coordinates nothing or + * leaving an unremovable socket pair behind at close. */ +TEST(daemon_ipc_listener_touch_detects_lost_runtime_files) { +#ifdef _WIN32 + SKIP_PLATFORM("unlink-while-held of runtime files is POSIX behavior"); +#else + static const char key[] = "2178a2178a2178a0"; + static const char *const lost_files[] = {"lifetime.lock", "sock.identity"}; + char parent[TEST_PATH_CAP] = {0}; + char runtime_dir[TEST_PATH_CAP] = {0}; + cbm_daemon_ipc_endpoint_t *endpoint = NULL; + bool started[2] = {false, false}; + bool touched_intact[2] = {false, false}; + bool unlinked[2] = {false, false}; + bool touched_after_loss[2] = {true, true}; + + if (ipc_test_parent_new(parent, "listener-touch")) { + endpoint = cbm_daemon_ipc_endpoint_new(key, parent); + } + if (endpoint) { + ipc_test_copy_path(runtime_dir, cbm_daemon_ipc_endpoint_runtime_dir(endpoint)); + } + for (size_t i = 0; endpoint && i < 2; i++) { + cbm_daemon_ipc_listener_t *listener = cbm_daemon_ipc_listen(endpoint); + started[i] = listener != NULL; + touched_intact[i] = started[i] && cbm_daemon_ipc_listener_touch(listener); + char path[TEST_PATH_CAP]; + int written = snprintf(path, sizeof(path), "%s/cbm-%s.%s", runtime_dir, key, lost_files[i]); + unlinked[i] = + touched_intact[i] && written > 0 && written < (int)sizeof(path) && unlink(path) == 0; + touched_after_loss[i] = unlinked[i] && cbm_daemon_ipc_listener_touch(listener); + cbm_daemon_ipc_listener_close(listener); + } + cbm_daemon_ipc_endpoint_free(endpoint); + ipc_test_remove_tree(runtime_dir, parent); + + for (size_t i = 0; i < 2; i++) { + ASSERT_TRUE(started[i]); + ASSERT_TRUE(touched_intact[i]); + ASSERT_TRUE(unlinked[i]); + ASSERT_FALSE(touched_after_loss[i]); + } + PASS(); +#endif +} + TEST(daemon_ipc_lifetime_reservation_transfers_without_unlock_window) { static const char key[] = "1029384756abcdef"; char parent[TEST_PATH_CAP] = {0}; @@ -5260,8 +5308,7 @@ TEST(daemon_ipc_posix_single_uid_userns_real_smoke_issue1830) { if (WEXITSTATUS(status) != 0 && WEXITSTATUS(status) != 1) { char unexpected[128]; (void)snprintf(unexpected, sizeof(unexpected), - "userns probe exited %d -- not a security verdict", - WEXITSTATUS(status)); + "userns probe exited %d -- not a security verdict", WEXITSTATUS(status)); FAIL(unexpected); } ASSERT_EQ(0, WEXITSTATUS(status)); @@ -5376,6 +5423,7 @@ SUITE(daemon_ipc) { RUN_TEST(daemon_ipc_rejects_uppercase_instance_key); RUN_TEST(daemon_ipc_no_spawn_probe_distinguishes_absent_active_and_busy); RUN_TEST(daemon_ipc_lifetime_reservation_survives_saturated_second_listen); + RUN_TEST(daemon_ipc_listener_touch_detects_lost_runtime_files); RUN_TEST(daemon_ipc_lifetime_reservation_transfers_without_unlock_window); RUN_TEST(daemon_ipc_local_frame_roundtrip); RUN_TEST(daemon_ipc_bounded_receive_rejects_oversize_before_payload); diff --git a/tests/test_private_file_lock.c b/tests/test_private_file_lock.c index 2760bf55b..b795f9176 100644 --- a/tests/test_private_file_lock.c +++ b/tests/test_private_file_lock.c @@ -166,7 +166,8 @@ static void private_lock_fixture_finish(private_lock_fixture_t *fixture) { "lock-attempt-cleanup.lock", "acl-directory.lock", "acl-file.lock", - "fork.lock"}; + "fork.lock", + "touch.lock"}; if (fixture->root[0]) { for (size_t index = 0; index < sizeof(files) / sizeof(files[0]); index++) { if (!private_lock_path(path, fixture, files[index])) { @@ -480,6 +481,65 @@ TEST(private_file_lock_consumed_close_error_never_retries_recycled_fd) { #endif } +/* #2178: macOS tmp_cleaner unlinks /tmp files whose atime, mtime and ctime are + * all older than three days, including empty lock files a daemon holds for its + * whole life. The next participant then creates and locks a fresh inode, so + * the old lock coordinates nothing. Touch must keep a held file young and + * report the loss once the path stops naming it. */ +TEST(private_file_lock_touch_refreshes_held_file_and_detects_unlink) { +#ifdef _WIN32 + SKIP_PLATFORM("age-based temp cleaning and unlink-while-held are POSIX behaviors"); +#else + private_lock_fixture_t fixture; + bool started = private_lock_fixture_start(&fixture); + char path[PRIVATE_LOCK_TEST_PATH_CAP]; + bool path_ok = started && private_lock_path(path, &fixture, "touch.lock"); + cbm_private_file_lock_t *held = NULL; + cbm_private_file_lock_t *successor = NULL; + cbm_private_file_lock_status_t acquired = + path_ok ? cbm_private_file_lock_try_acquire(fixture.directory, "touch.lock", + CBM_PRIVATE_FILE_LOCK_EX, &held) + : CBM_PRIVATE_FILE_LOCK_IO; + const time_t stale = 1000000000; + struct timespec stale_times[2] = {{.tv_sec = stale}, {.tv_sec = stale}}; + bool aged = acquired == CBM_PRIVATE_FILE_LOCK_OK && + futimens(cbm_private_file_lock_native_fd_for_test(held), stale_times) == 0; + cbm_private_file_lock_status_t linked_touch = + aged ? cbm_private_file_lock_touch(held) : CBM_PRIVATE_FILE_LOCK_IO; + struct stat refreshed = {0}; + bool refreshed_ok = linked_touch == CBM_PRIVATE_FILE_LOCK_OK && stat(path, &refreshed) == 0; + + bool unlinked = refreshed_ok && unlink(path) == 0; + cbm_private_file_lock_status_t unlinked_touch = + unlinked ? cbm_private_file_lock_touch(held) : CBM_PRIVATE_FILE_LOCK_IO; + /* The hazard itself: with the path gone, a peer's EX succeeds while the + * original holder still believes it is exclusive. */ + cbm_private_file_lock_status_t successor_status = + unlinked ? cbm_private_file_lock_try_acquire(fixture.directory, "touch.lock", + CBM_PRIVATE_FILE_LOCK_EX, &successor) + : CBM_PRIVATE_FILE_LOCK_IO; + if (successor) { + (void)cbm_private_file_lock_release(&successor); + } + if (held) { + (void)cbm_private_file_lock_release(&held); + } + private_lock_fixture_finish(&fixture); + + ASSERT_TRUE(started); + ASSERT_EQ(acquired, CBM_PRIVATE_FILE_LOCK_OK); + ASSERT_TRUE(aged); + ASSERT_EQ(linked_touch, CBM_PRIVATE_FILE_LOCK_OK); + ASSERT_TRUE(refreshed_ok); + ASSERT_TRUE(refreshed.st_atime > stale); + ASSERT_TRUE(refreshed.st_mtime > stale); + ASSERT_TRUE(unlinked); + ASSERT_EQ(unlinked_touch, CBM_PRIVATE_FILE_LOCK_UNSAFE); + ASSERT_EQ(successor_status, CBM_PRIVATE_FILE_LOCK_OK); + PASS(); +#endif +} + TEST(private_file_lock_post_acquire_failure_returns_cleanup_owner) { private_lock_fixture_t fixture; bool started = private_lock_fixture_start(&fixture); @@ -863,6 +923,7 @@ SUITE(private_file_lock) { RUN_TEST(private_file_lock_unlock_failure_retains_retryable_lock); RUN_TEST(private_file_lock_close_failure_retries_without_duplicate_unlock); RUN_TEST(private_file_lock_consumed_close_error_never_retries_recycled_fd); + RUN_TEST(private_file_lock_touch_refreshes_held_file_and_detects_unlink); RUN_TEST(private_file_lock_post_acquire_failure_returns_cleanup_owner); RUN_TEST(private_file_lock_windows_lock_attempt_failure_returns_cleanup_owner); RUN_TEST(private_file_lock_rejects_unsafe_entries_and_replaced_root); From 3cf2a6663f36a6a366806bc78e4cc0678e1f1b55 Mon Sep 17 00:00:00 2001 From: Shane McCarron Date: Mon, 21 Sep 2026 13:02:07 -0500 Subject: [PATCH 2/6] fix(daemon): address QA round 1 - 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) Signed-off-by: Shane McCarron --- src/daemon/host.c | 71 ++++++++++----- src/daemon/ipc.c | 99 +++++++++++++-------- src/daemon/ipc.h | 26 +++--- src/daemon/runtime.c | 6 +- src/daemon/runtime.h | 10 ++- src/daemon/version_cohort.c | 36 +++++--- src/daemon/version_cohort.h | 16 ++-- src/foundation/private_file_lock.c | 13 ++- src/foundation/private_file_lock_internal.h | 9 +- tests/test_daemon_ipc.c | 59 ++++++++---- tests/test_private_file_lock.c | 47 +++++++++- tests/test_version_cohort.c | 54 ++++++++++- 12 files changed, 323 insertions(+), 123 deletions(-) diff --git a/src/daemon/host.c b/src/daemon/host.c index 856af3ee4..3dea464cf 100644 --- a/src/daemon/host.c +++ b/src/daemon/host.c @@ -879,26 +879,41 @@ typedef struct { cbm_daemon_ipc_participant_guard_t *participant_guard; } host_coordination_t; -/* Refresh every long-held runtime file and name the first one found lost, or - * return NULL. A held lock whose path now names another inode coordinates - * nothing: forked index workers read the daemon as uncoordinated and a peer - * can start a second generation (#2178). */ -static const char *host_coordination_touch(const host_coordination_t *coordination, - cbm_daemon_runtime_service_t *service) { - if (!cbm_version_cohort_daemon_claim_touch(coordination->daemon_claim)) { - return "cohort_daemon_claim"; - } - if (!cbm_version_cohort_lease_touch(coordination->cohort_lease)) { - return "cohort_lease"; - } - if (!cbm_daemon_ipc_participant_guard_touch(coordination->endpoint, - coordination->participant_guard)) { - return "participant_guard"; - } - if (!cbm_daemon_runtime_service_touch_listener(service)) { - return "listener"; +typedef struct { + const char *lost; /* first held file whose path no longer names it */ + const char *transient; /* first still-valid file that could not be refreshed */ +} host_touch_result_t; + +static void host_touch_note(host_touch_result_t *result, const char *file, int status) { + if (status == 0 && !result->lost) { + result->lost = file; + } else if (status < 0 && !result->transient) { + result->transient = file; } - return NULL; +} + +static int host_cohort_touch_status(cbm_version_cohort_status_t status) { + return status == CBM_VERSION_COHORT_OK ? 1 : (status == CBM_VERSION_COHORT_IO ? -1 : 0); +} + +/* Refresh every long-held runtime file. A held lock whose path now names + * another inode coordinates nothing: forked index workers read the daemon as + * uncoordinated and a peer can start a second generation (#2178). Every file + * is visited even after one fails so the rest keep being refreshed. */ +static host_touch_result_t host_coordination_touch(const host_coordination_t *coordination, + cbm_daemon_runtime_service_t *service) { + host_touch_result_t result = {0}; + host_touch_note(&result, "cohort_daemon_claim", + host_cohort_touch_status( + cbm_version_cohort_daemon_claim_touch(coordination->daemon_claim))); + host_touch_note( + &result, "cohort_lease", + host_cohort_touch_status(cbm_version_cohort_lease_touch(coordination->cohort_lease))); + host_touch_note(&result, "participant_guard", + cbm_daemon_ipc_participant_guard_touch(coordination->endpoint, + coordination->participant_guard)); + host_touch_note(&result, "listener", cbm_daemon_runtime_service_touch_listener(service)); + return result; } static bool host_wait_for_lifetime(cbm_daemon_runtime_service_t *service, @@ -907,6 +922,7 @@ static bool host_wait_for_lifetime(cbm_daemon_runtime_service_t *service, uint64_t initial_deadline = cbm_now_ms() + HOST_INITIAL_CLIENT_TIMEOUT_MS; uint64_t stopping_deadline = 0; uint64_t next_touch = 0; + bool touch_degraded = false; for (;;) { cbm_daemon_runtime_service_state_t state = cbm_daemon_runtime_service_state(service); if (state == CBM_DAEMON_RUNTIME_SERVICE_EXITED) { @@ -942,12 +958,23 @@ static bool host_wait_for_lifetime(cbm_daemon_runtime_service_t *service, * coordinated generation, while re-claiming would race that client. */ if (cbm_now_ms() >= next_touch) { next_touch = cbm_now_ms() + HOST_COORDINATION_TOUCH_MS; - const char *lost = host_coordination_touch(coordination, service); - if (lost) { + host_touch_result_t touched = host_coordination_touch(coordination, service); + if (touched.lost) { cbm_log_warn("daemon.lifetime_end", "reason", "coordination_file_lost", "file", - lost); + touched.lost); return cbm_daemon_runtime_service_stop(service, HOST_RUNTIME_SHUTDOWN_MS); } + /* A refresh failure on a still-valid file (read-only remount, + * EMFILE) is not loss: keep serving, retry next tick, and log + * only transitions so a persistent condition cannot flood. */ + if ((touched.transient != NULL) != touch_degraded) { + touch_degraded = touched.transient != NULL; + if (touch_degraded) { + cbm_log_warn("daemon.coordination_touch_failed", "file", touched.transient); + } else { + cbm_log_info("daemon.coordination_touch_recovered"); + } + } } host_http_reconcile_at(host, cbm_now_ms(), false); /* Retire an ephemeral generation that lingered for cold-storm cohort diff --git a/src/daemon/ipc.c b/src/daemon/ipc.c index 53da43542..bedc5884d 100644 --- a/src/daemon/ipc.c +++ b/src/daemon/ipc.c @@ -3220,44 +3220,65 @@ void cbm_daemon_ipc_listener_close(cbm_daemon_ipc_listener_t *listener) { free(listener); } -/* Touch through the held descriptor only after proving the path still names - * it: refreshing by path would keep a replacement inode fresh and hide the - * loss (#2178). */ -static bool posix_held_file_touch(int directory_fd, const char *base_name, int fd) { - return private_regular_file_snapshot(directory_fd, base_name, fd, 1, NULL) && - futimens(fd, NULL) == 0; +/* Touch results: 1 refreshed, 0 lost (the path no longer names the owned + * file), -1 transient (a still-valid file could not be refreshed). Touch + * through the held descriptor only after proving the path still names it: + * refreshing by path would keep a replacement inode fresh and hide the loss + * (#2178). */ +static int posix_touch_fold(int result, int next) { + return result == 0 || next == 0 ? 0 : (result < 0 || next < 0 ? -1 : 1); } -static bool posix_held_lock_touch(int directory_fd, const process_lock_entry_t *entry, int fd) { - return entry && posix_held_file_touch(directory_fd, entry->lock_name, fd); +static int posix_held_file_touch(int directory_fd, const char *base_name, int fd) { + if (!private_regular_file_snapshot(directory_fd, base_name, fd, 1, NULL)) { + return 0; + } + return futimens(fd, NULL) == 0 ? 1 : -1; +} + +static int posix_held_lock_touch(int directory_fd, const process_lock_entry_t *entry, int fd) { + return entry ? posix_held_file_touch(directory_fd, entry->lock_name, fd) : 0; +} + +static int posix_identity_marker_touch(const cbm_daemon_ipc_listener_t *listener) { + /* The marker is not held open; reopen it and require the published inode. + * Only ENOENT proves loss: EMFILE and friends say nothing about the file. */ + int marker_fd = openat(listener->dir_fd, listener->socket_identity_name, + O_RDONLY | O_CLOEXEC | O_NOFOLLOW | O_NONBLOCK); + if (marker_fd < 0) { + return errno == ENOENT ? 0 : -1; + } + struct stat marker_status; + int result = 0; + if (private_regular_file_snapshot(listener->dir_fd, listener->socket_identity_name, marker_fd, + 1, &marker_status) && + marker_status.st_dev == listener->identity_device && + marker_status.st_ino == listener->identity_inode) { + result = futimens(marker_fd, NULL) == 0 ? 1 : -1; + } + (void)close(marker_fd); + return result; } -bool cbm_daemon_ipc_listener_touch(cbm_daemon_ipc_listener_t *listener) { +int cbm_daemon_ipc_listener_touch(cbm_daemon_ipc_listener_t *listener, + cbm_daemon_ipc_participant_guard_t *external_guard) { if (!listener || listener->dir_fd < 0 || listener->owner_pid != getpid() || !listener->lifetime_reservation) { - return false; + return 0; } - bool lifetime = + int result = posix_held_lock_touch(listener->dir_fd, listener->lifetime_reservation->process_entry, listener->lifetime_reservation->fd); - const cbm_daemon_ipc_participant_guard_t *guard = listener->participant_guard; - bool participant = - !guard || - posix_held_lock_touch(listener->dir_fd, guard->legacy_process_entry, guard->legacy_fd); - /* The marker is not held open; reopen it and require the published inode. */ - int marker_fd = openat(listener->dir_fd, listener->socket_identity_name, - O_RDONLY | O_CLOEXEC | O_NOFOLLOW | O_NONBLOCK); - struct stat marker_status; - bool marker = marker_fd >= 0 && - private_regular_file_snapshot(listener->dir_fd, listener->socket_identity_name, - marker_fd, 1, &marker_status) && - marker_status.st_dev == listener->identity_device && - marker_status.st_ino == listener->identity_inode && - futimens(marker_fd, NULL) == 0; - if (marker_fd >= 0) { - (void)close(marker_fd); + const cbm_daemon_ipc_participant_guard_t *guards[] = {listener->participant_guard, + external_guard}; + for (size_t i = 0; i < sizeof(guards) / sizeof(guards[0]); i++) { + if (guards[i]) { + result = posix_touch_fold(result, posix_held_lock_touch(listener->dir_fd, + guards[i]->legacy_process_entry, + guards[i]->legacy_fd)); + } } - return lifetime && participant && marker; + return posix_touch_fold(result, posix_identity_marker_touch(listener)); } int cbm_daemon_ipc_accept(cbm_daemon_ipc_listener_t *listener, uint32_t timeout_ms, @@ -3643,10 +3664,12 @@ bool cbm_daemon_ipc_participant_guard_release(cbm_daemon_ipc_participant_guard_t return true; } -bool cbm_daemon_ipc_participant_guard_touch(const cbm_daemon_ipc_endpoint_t *endpoint, - cbm_daemon_ipc_participant_guard_t *guard) { - return endpoint && guard && guard->owner_pid == getpid() && - posix_held_lock_touch(endpoint->dir_fd, guard->legacy_process_entry, guard->legacy_fd); +int cbm_daemon_ipc_participant_guard_touch(const cbm_daemon_ipc_endpoint_t *endpoint, + cbm_daemon_ipc_participant_guard_t *guard) { + if (!endpoint || !guard || guard->owner_pid != getpid()) { + return 0; + } + return posix_held_lock_touch(endpoint->dir_fd, guard->legacy_process_entry, guard->legacy_fd); } int cbm_daemon_ipc_local_transition_try_acquire( @@ -6794,13 +6817,15 @@ bool cbm_daemon_ipc_participant_guard_release(cbm_daemon_ipc_participant_guard_t /* Windows has no age-based cleaner of the private runtime directory, and its * held handles deny deletion; the #2178 heartbeat has nothing to refresh. */ -bool cbm_daemon_ipc_listener_touch(cbm_daemon_ipc_listener_t *listener) { - return listener != NULL; +int cbm_daemon_ipc_listener_touch(cbm_daemon_ipc_listener_t *listener, + cbm_daemon_ipc_participant_guard_t *external_guard) { + (void)external_guard; + return listener ? 1 : 0; } -bool cbm_daemon_ipc_participant_guard_touch(const cbm_daemon_ipc_endpoint_t *endpoint, - cbm_daemon_ipc_participant_guard_t *guard) { - return endpoint && guard; +int cbm_daemon_ipc_participant_guard_touch(const cbm_daemon_ipc_endpoint_t *endpoint, + cbm_daemon_ipc_participant_guard_t *guard) { + return endpoint && guard ? 1 : 0; } int cbm_daemon_ipc_local_transition_try_acquire( diff --git a/src/daemon/ipc.h b/src/daemon/ipc.h index ed214ea79..9a1e7b044 100644 --- a/src/daemon/ipc.h +++ b/src/daemon/ipc.h @@ -77,13 +77,17 @@ cbm_daemon_ipc_listener_t *cbm_daemon_ipc_listen_reserved( void cbm_daemon_ipc_listener_close(cbm_daemon_ipc_listener_t *listener); /* Heartbeat for the daemon's long-lived runtime artifacts: refresh the held - * lifetime reservation, the listener's participant guard (if it owns one) and - * the published identity marker so an age-based temp cleaner never deletes - * them (#2178). Returns false once any of them no longer names the file this - * listener owns: a lost lifetime file lets a peer start a second generation, - * and a lost identity marker makes listener close leave the socket pair - * behind. Only the owning thread may call it while the listener is open. */ -bool cbm_daemon_ipc_listener_touch(cbm_daemon_ipc_listener_t *listener); + * lifetime reservation, the listener's participant guard (if it owns one), + * external_guard (optional, a guard the caller retains beside this listener) + * and the published identity marker so an age-based temp cleaner never + * deletes them (#2178). Returns 1 when all were refreshed, 0 once any no + * longer names the file this listener owns (a lost lifetime file lets a peer + * start a second generation; a lost identity marker makes listener close + * leave the socket pair behind), and -1 when a still-valid file could not be + * refreshed (transient). Only the thread that closes the listener may call it + * while the listener is open. */ +int cbm_daemon_ipc_listener_touch(cbm_daemon_ipc_listener_t *listener, + cbm_daemon_ipc_participant_guard_t *external_guard); /* Create or validate one private current-user directory at an already * canonical local path. Ancestors are handle-validated without mutation: @@ -279,10 +283,10 @@ bool cbm_daemon_ipc_startup_lock_release(cbm_daemon_ipc_startup_lock_t **lock_io int cbm_daemon_ipc_participant_guard_try_join(const cbm_daemon_ipc_endpoint_t *endpoint, cbm_daemon_ipc_participant_guard_t **guard_out); bool cbm_daemon_ipc_participant_guard_release(cbm_daemon_ipc_participant_guard_t **guard_io); -/* Same heartbeat contract as cbm_daemon_ipc_listener_touch, for a guard the - * caller retains itself. */ -bool cbm_daemon_ipc_participant_guard_touch(const cbm_daemon_ipc_endpoint_t *endpoint, - cbm_daemon_ipc_participant_guard_t *guard); +/* Same heartbeat contract and 1/0/-1 result as cbm_daemon_ipc_listener_touch, + * for a guard the caller retains itself. */ +int cbm_daemon_ipc_participant_guard_touch(const cbm_daemon_ipc_endpoint_t *endpoint, + cbm_daemon_ipc_participant_guard_t *guard); /* Standalone CLI work joins the legacy-compatible current participant group * without becoming a daemon client. Acquisition retains startup-v2 only for diff --git a/src/daemon/runtime.c b/src/daemon/runtime.c index 7fb7d32a9..bce7118f4 100644 --- a/src/daemon/runtime.c +++ b/src/daemon/runtime.c @@ -2578,8 +2578,10 @@ size_t cbm_daemon_runtime_service_active_connections(cbm_daemon_runtime_service_ return count; } -bool cbm_daemon_runtime_service_touch_listener(cbm_daemon_runtime_service_t *service) { - return service && service->listener && cbm_daemon_ipc_listener_touch(service->listener); +int cbm_daemon_runtime_service_touch_listener(cbm_daemon_runtime_service_t *service) { + return service && service->listener + ? cbm_daemon_ipc_listener_touch(service->listener, service->owned_participant_guard) + : 0; } void cbm_daemon_runtime_service_reconcile_lifetime(cbm_daemon_runtime_service_t *service) { diff --git a/src/daemon/runtime.h b/src/daemon/runtime.h index 304d7c898..5bcd10a34 100644 --- a/src/daemon/runtime.h +++ b/src/daemon/runtime.h @@ -336,10 +336,12 @@ size_t cbm_daemon_runtime_service_active_connections(cbm_daemon_runtime_service_ * an unbounded idle hang. The host lifetime loop calls this every tick; it is a * no-op unless a last-committed-client linger is armed. */ void cbm_daemon_runtime_service_reconcile_lifetime(cbm_daemon_runtime_service_t *service); -/* #2178 heartbeat over the listener's runtime artifacts; see - * cbm_daemon_ipc_listener_touch. Call only from the thread that stops and - * frees the service, which is the only thread that closes the listener. */ -bool cbm_daemon_runtime_service_touch_listener(cbm_daemon_runtime_service_t *service); +/* #2178 heartbeat over the listener's runtime artifacts, plus the participant + * guard the service owns when it started itself; see + * cbm_daemon_ipc_listener_touch for the 1/0/-1 result. Call only from the + * thread that stops and frees the service, which is the only thread that + * closes the listener. */ +int cbm_daemon_runtime_service_touch_listener(cbm_daemon_runtime_service_t *service); size_t cbm_daemon_runtime_service_job_subscribers(cbm_daemon_runtime_service_t *service, const char *project_key); uint64_t cbm_daemon_runtime_service_client_process_id(cbm_daemon_runtime_service_t *service, diff --git a/src/daemon/version_cohort.c b/src/daemon/version_cohort.c index 82cc63b32..5acaa6d5e 100644 --- a/src/daemon/version_cohort.c +++ b/src/daemon/version_cohort.c @@ -363,23 +363,35 @@ cbm_private_file_lock_status_t cbm_version_cohort_lease_release( return result; } -static bool version_cohort_lock_touch(cbm_private_file_lock_t *lock) { - return !lock || cbm_private_file_lock_touch(lock) == CBM_PRIVATE_FILE_LOCK_OK; +/* Folds one lock's touch into an accumulated result: a lost file (UNSAFE) + * outranks a transient refresh failure (IO), which outranks OK. A lease holds + * only lifetime after admission, so absent locks are skipped. */ +static cbm_version_cohort_status_t version_cohort_lock_touch(cbm_private_file_lock_t *lock, + cbm_version_cohort_status_t result) { + if (!lock || result == CBM_VERSION_COHORT_UNSAFE) { + return result; + } + cbm_private_file_lock_status_t status = cbm_private_file_lock_touch(lock); + if (status == CBM_PRIVATE_FILE_LOCK_UNSAFE) { + return CBM_VERSION_COHORT_UNSAFE; + } + return status == CBM_PRIVATE_FILE_LOCK_OK ? result : CBM_VERSION_COHORT_IO; } -bool cbm_version_cohort_lease_touch(cbm_version_cohort_lease_t *lease) { - if (!lease) { - return false; +cbm_version_cohort_status_t cbm_version_cohort_lease_touch(cbm_version_cohort_lease_t *lease) { + if (!lease || !lease->lifetime) { + return CBM_VERSION_COHORT_UNSAFE; } - /* Evaluate all three so one lost file does not leave the others to age. */ - bool lifetime = version_cohort_lock_touch(lease->lifetime); - bool admission = version_cohort_lock_touch(lease->admission); - bool maintenance = version_cohort_lock_touch(lease->maintenance); - return lifetime && admission && maintenance; + cbm_version_cohort_status_t result = + version_cohort_lock_touch(lease->lifetime, CBM_VERSION_COHORT_OK); + result = version_cohort_lock_touch(lease->admission, result); + return version_cohort_lock_touch(lease->maintenance, result); } -bool cbm_version_cohort_daemon_claim_touch(cbm_version_cohort_daemon_claim_t *claim) { - return claim && claim->marker && version_cohort_lock_touch(claim->marker); +cbm_version_cohort_status_t cbm_version_cohort_daemon_claim_touch( + cbm_version_cohort_daemon_claim_t *claim) { + return claim && claim->marker ? version_cohort_lock_touch(claim->marker, CBM_VERSION_COHORT_OK) + : CBM_VERSION_COHORT_UNSAFE; } static cbm_version_cohort_status_t version_cohort_failed(cbm_version_cohort_lease_t *lease, diff --git a/src/daemon/version_cohort.h b/src/daemon/version_cohort.h index ac79c8b86..0a6be9ce9 100644 --- a/src/daemon/version_cohort.h +++ b/src/daemon/version_cohort.h @@ -146,13 +146,15 @@ cbm_private_file_lock_status_t cbm_version_cohort_lease_release( cbm_version_cohort_lease_t **lease_io); /* Heartbeat for a long-lived holder: refresh every lock file the lease or - * claim holds and report false once any held file was unlinked or replaced. - * An age-based temp cleaner deleting a held file silently ends coordination, - * because the next participant creates and locks a fresh inode (#2178). The - * only safe response to false is an orderly exit; re-claiming would race - * that participant. */ -bool cbm_version_cohort_lease_touch(cbm_version_cohort_lease_t *lease); -bool cbm_version_cohort_daemon_claim_touch(cbm_version_cohort_daemon_claim_t *claim); + * claim holds. An age-based temp cleaner deleting a held file silently ends + * coordination, because the next participant creates and locks a fresh inode + * (#2178). Returns OK when all held files were refreshed, UNSAFE once any is + * lost (its path no longer names the held file), and IO when a still-valid + * file could not be refreshed. The only safe response to UNSAFE is an orderly + * exit; re-claiming would race that participant. IO is transient. */ +cbm_version_cohort_status_t cbm_version_cohort_lease_touch(cbm_version_cohort_lease_t *lease); +cbm_version_cohort_status_t cbm_version_cohort_daemon_claim_touch( + cbm_version_cohort_daemon_claim_t *claim); /* Refuses teardown while a lease or retryable cleanup handle remains. */ cbm_private_file_lock_status_t cbm_version_cohort_manager_free( diff --git a/src/foundation/private_file_lock.c b/src/foundation/private_file_lock.c index a7d83172e..a5fdf797d 100644 --- a/src/foundation/private_file_lock.c +++ b/src/foundation/private_file_lock.c @@ -38,6 +38,9 @@ struct cbm_private_file_lock { int fd; pid_t owner_pid; cbm_private_file_lock_mode_t mode; + /* Borrowed; the directory outlives every lock acquired through it. */ + const cbm_private_lock_directory_t *directory; + char base_name[NAME_MAX + 1]; struct cbm_private_file_lock *next_tracked; bool unlocked; bool test_fail_unlock_once; @@ -345,6 +348,8 @@ cbm_private_file_lock_status_t cbm_private_file_lock_try_acquire( lock->fd = fd; lock->owner_pid = getpid(); lock->mode = mode; + lock->directory = directory; + memcpy(lock->base_name, base_name, strlen(base_name) + 1); int operation = mode == CBM_PRIVATE_FILE_LOCK_SH ? LOCK_SH : LOCK_EX; if (private_flock_set(fd, operation | LOCK_NB) != 0) { @@ -489,9 +494,11 @@ cbm_private_file_lock_status_t cbm_private_file_lock_touch(cbm_private_file_lock if (!cbm_private_file_lock_fork_guard_enter()) { return CBM_PRIVATE_FILE_LOCK_IO; } - /* An unlinked file keeps its inode alive for this handle with st_nlink 0, - * which the payload validity check already rejects. */ - bool linked = private_payload_fd_valid(lock, NULL); + /* Unlink leaves st_nlink 0 on the held inode, but a rename-away or a + * replaced directory keeps it linked elsewhere; only the path-vs-handle + * identity check proves the canonical path still names this lock. */ + bool linked = private_payload_fd_valid(lock, NULL) && lock->directory && + private_file_revalidate(lock->directory, lock->base_name, lock->fd, NULL); bool touched = linked && futimens(lock->fd, NULL) == 0; cbm_private_file_lock_fork_guard_leave(); if (!linked) { diff --git a/src/foundation/private_file_lock_internal.h b/src/foundation/private_file_lock_internal.h index b8ec34ac7..d39fe7d5f 100644 --- a/src/foundation/private_file_lock_internal.h +++ b/src/foundation/private_file_lock_internal.h @@ -63,9 +63,12 @@ cbm_private_file_lock_status_t cbm_private_file_lock_payload_write(cbm_private_f /* Long-lived holders call this periodically. It refreshes the held file's * timestamps through the validated handle so age-based temp cleaners (macOS - * tmp_cleaner, systemd-tmpfiles) never select it, and returns UNSAFE once the - * handle no longer names a linked file: a peer opening the path would then - * lock a different inode and the lock coordinates nothing (#2178). */ + * tmp_cleaner, systemd-tmpfiles) never select it. Returns UNSAFE once the + * lock's path in its directory no longer names the held file (unlinked, + * replaced, renamed away, or the directory itself replaced): a peer opening + * the path would then lock a different inode and the lock coordinates nothing + * (#2178). IO reports a refresh failure on a still-valid lock, which callers + * should treat as transient. The directory must outlive the lock. */ cbm_private_file_lock_status_t cbm_private_file_lock_touch(cbm_private_file_lock_t *lock); /* Forces the next successfully acquired native lock down the post-lock diff --git a/tests/test_daemon_ipc.c b/tests/test_daemon_ipc.c index d29a1e6c0..8107a0ef5 100644 --- a/tests/test_daemon_ipc.c +++ b/tests/test_daemon_ipc.c @@ -2064,22 +2064,34 @@ TEST(daemon_ipc_lifetime_reservation_survives_saturated_second_listen) { } /* #2178: an age-based temp cleaner unlinked the live daemon's lifetime lock - * and identity marker. The heartbeat must report either loss, so the daemon - * exits instead of serving under a lifetime file that coordinates nothing or - * leaving an unremovable socket pair behind at close. */ + * and identity marker. The heartbeat must keep every listener-owned file + * young, and report loss (unlink, or replacement by a fresh file at the same + * name) so the daemon exits instead of serving under a lifetime file that + * coordinates nothing or leaving an unremovable socket pair behind at close. */ TEST(daemon_ipc_listener_touch_detects_lost_runtime_files) { #ifdef _WIN32 SKIP_PLATFORM("unlink-while-held of runtime files is POSIX behavior"); #else + enum { TOUCH_CASES = 4 }; static const char key[] = "2178a2178a2178a0"; - static const char *const lost_files[] = {"lifetime.lock", "sock.identity"}; + static const struct { + const char *suffix; + bool replace; + } cases[TOUCH_CASES] = { + {"lifetime.lock", false}, /* lifetime reservation */ + {"lock", false}, /* listener-owned participant guard */ + {"sock.identity", false}, /* identity marker, unlinked */ + {"sock.identity", true}, /* identity marker, replaced by a fresh inode */ + }; + const time_t stale = 1000000000; char parent[TEST_PATH_CAP] = {0}; char runtime_dir[TEST_PATH_CAP] = {0}; cbm_daemon_ipc_endpoint_t *endpoint = NULL; - bool started[2] = {false, false}; - bool touched_intact[2] = {false, false}; - bool unlinked[2] = {false, false}; - bool touched_after_loss[2] = {true, true}; + bool started[TOUCH_CASES] = {false}; + int touched_intact[TOUCH_CASES] = {0}; + bool refreshed[TOUCH_CASES] = {false}; + bool lost[TOUCH_CASES] = {false}; + int touched_after_loss[TOUCH_CASES] = {1, 1, 1, 1}; if (ipc_test_parent_new(parent, "listener-touch")) { endpoint = cbm_daemon_ipc_endpoint_new(key, parent); @@ -2087,25 +2099,36 @@ TEST(daemon_ipc_listener_touch_detects_lost_runtime_files) { if (endpoint) { ipc_test_copy_path(runtime_dir, cbm_daemon_ipc_endpoint_runtime_dir(endpoint)); } - for (size_t i = 0; endpoint && i < 2; i++) { + for (size_t i = 0; endpoint && i < TOUCH_CASES; i++) { cbm_daemon_ipc_listener_t *listener = cbm_daemon_ipc_listen(endpoint); started[i] = listener != NULL; - touched_intact[i] = started[i] && cbm_daemon_ipc_listener_touch(listener); char path[TEST_PATH_CAP]; - int written = snprintf(path, sizeof(path), "%s/cbm-%s.%s", runtime_dir, key, lost_files[i]); - unlinked[i] = - touched_intact[i] && written > 0 && written < (int)sizeof(path) && unlink(path) == 0; - touched_after_loss[i] = unlinked[i] && cbm_daemon_ipc_listener_touch(listener); + int written = + snprintf(path, sizeof(path), "%s/cbm-%s.%s", runtime_dir, key, cases[i].suffix); + bool path_ok = started[i] && written > 0 && written < (int)sizeof(path); + struct timespec stale_times[2] = {{.tv_sec = stale}, {.tv_sec = stale}}; + bool aged = path_ok && utimensat(AT_FDCWD, path, stale_times, AT_SYMLINK_NOFOLLOW) == 0; + touched_intact[i] = aged ? cbm_daemon_ipc_listener_touch(listener, NULL) : -2; + struct stat after; + refreshed[i] = touched_intact[i] == 1 && stat(path, &after) == 0 && + after.st_atime > stale && after.st_mtime > stale; + lost[i] = refreshed[i] && unlink(path) == 0; + if (lost[i] && cases[i].replace) { + int fd = open(path, O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC | O_NOFOLLOW, 0600); + lost[i] = fd >= 0 && close(fd) == 0; + } + touched_after_loss[i] = lost[i] ? cbm_daemon_ipc_listener_touch(listener, NULL) : -2; cbm_daemon_ipc_listener_close(listener); } cbm_daemon_ipc_endpoint_free(endpoint); ipc_test_remove_tree(runtime_dir, parent); - for (size_t i = 0; i < 2; i++) { + for (size_t i = 0; i < TOUCH_CASES; i++) { ASSERT_TRUE(started[i]); - ASSERT_TRUE(touched_intact[i]); - ASSERT_TRUE(unlinked[i]); - ASSERT_FALSE(touched_after_loss[i]); + ASSERT_EQ(touched_intact[i], 1); + ASSERT_TRUE(refreshed[i]); + ASSERT_TRUE(lost[i]); + ASSERT_EQ(touched_after_loss[i], 0); } PASS(); #endif diff --git a/tests/test_private_file_lock.c b/tests/test_private_file_lock.c index b795f9176..988b73e71 100644 --- a/tests/test_private_file_lock.c +++ b/tests/test_private_file_lock.c @@ -167,7 +167,9 @@ static void private_lock_fixture_finish(private_lock_fixture_t *fixture) { "acl-directory.lock", "acl-file.lock", "fork.lock", - "touch.lock"}; + "touch.lock", + "touch-rename.lock", + "touch-renamed.lock"}; if (fixture->root[0]) { for (size_t index = 0; index < sizeof(files) / sizeof(files[0]); index++) { if (!private_lock_path(path, fixture, files[index])) { @@ -540,6 +542,48 @@ TEST(private_file_lock_touch_refreshes_held_file_and_detects_unlink) { #endif } +/* A rename-away keeps the held inode linked (st_nlink stays 1) while the + * canonical path no longer names it, so a peer would create and lock a fresh + * file there. Touch must compare the path with the handle, not just count + * links. */ +TEST(private_file_lock_touch_detects_rename_away) { +#ifdef _WIN32 + SKIP_PLATFORM("renaming a held lock file away is POSIX behavior"); +#else + private_lock_fixture_t fixture; + bool started = private_lock_fixture_start(&fixture); + char path[PRIVATE_LOCK_TEST_PATH_CAP]; + char moved[PRIVATE_LOCK_TEST_PATH_CAP]; + bool paths_ok = started && private_lock_path(path, &fixture, "touch-rename.lock") && + private_lock_path(moved, &fixture, "touch-renamed.lock"); + cbm_private_file_lock_t *held = NULL; + cbm_private_file_lock_status_t acquired = + paths_ok ? cbm_private_file_lock_try_acquire(fixture.directory, "touch-rename.lock", + CBM_PRIVATE_FILE_LOCK_EX, &held) + : CBM_PRIVATE_FILE_LOCK_IO; + cbm_private_file_lock_status_t intact_touch = acquired == CBM_PRIVATE_FILE_LOCK_OK + ? cbm_private_file_lock_touch(held) + : CBM_PRIVATE_FILE_LOCK_IO; + bool renamed = intact_touch == CBM_PRIVATE_FILE_LOCK_OK && rename(path, moved) == 0; + struct stat moved_status = {0}; + bool still_linked = renamed && stat(moved, &moved_status) == 0 && moved_status.st_nlink == 1; + cbm_private_file_lock_status_t renamed_touch = + renamed ? cbm_private_file_lock_touch(held) : CBM_PRIVATE_FILE_LOCK_IO; + if (held) { + (void)cbm_private_file_lock_release(&held); + } + private_lock_fixture_finish(&fixture); + + ASSERT_TRUE(started); + ASSERT_EQ(acquired, CBM_PRIVATE_FILE_LOCK_OK); + ASSERT_EQ(intact_touch, CBM_PRIVATE_FILE_LOCK_OK); + ASSERT_TRUE(renamed); + ASSERT_TRUE(still_linked); + ASSERT_EQ(renamed_touch, CBM_PRIVATE_FILE_LOCK_UNSAFE); + PASS(); +#endif +} + TEST(private_file_lock_post_acquire_failure_returns_cleanup_owner) { private_lock_fixture_t fixture; bool started = private_lock_fixture_start(&fixture); @@ -924,6 +968,7 @@ SUITE(private_file_lock) { RUN_TEST(private_file_lock_close_failure_retries_without_duplicate_unlock); RUN_TEST(private_file_lock_consumed_close_error_never_retries_recycled_fd); RUN_TEST(private_file_lock_touch_refreshes_held_file_and_detects_unlink); + RUN_TEST(private_file_lock_touch_detects_rename_away); RUN_TEST(private_file_lock_post_acquire_failure_returns_cleanup_owner); RUN_TEST(private_file_lock_windows_lock_attempt_failure_returns_cleanup_owner); RUN_TEST(private_file_lock_rejects_unsafe_entries_and_replaced_root); diff --git a/tests/test_version_cohort.c b/tests/test_version_cohort.c index 13e0f8065..f40e05c5e 100644 --- a/tests/test_version_cohort.c +++ b/tests/test_version_cohort.c @@ -330,9 +330,9 @@ TEST(version_cohort_conflict_is_retried_until_the_deadline) { /* An indefinite deadline never waits on a conflicting holder. */ retries_before = cbm_version_cohort_conflict_retries_for_testing(); - ASSERT_EQ(cbm_version_cohort_acquire(second, &requested, UINT64_MAX, &requested_lease, - &conflict), - CBM_VERSION_COHORT_CONFLICT); + ASSERT_EQ( + cbm_version_cohort_acquire(second, &requested, UINT64_MAX, &requested_lease, &conflict), + CBM_VERSION_COHORT_CONFLICT); ASSERT_NULL(requested_lease); ASSERT_EQ(cbm_version_cohort_conflict_retries_for_testing(), retries_before); @@ -669,6 +669,53 @@ TEST(version_cohort_does_not_repurpose_daemon_startup_lock_for_lifetime) { PASS(); } +/* #2178: a live daemon's lease holds only the lifetime file after admission + * (admission and maintenance are released), and its claim holds the marker. + * Touch must report both intact, then name each as lost once an age-based + * temp cleaner unlinks it, independently of the other. */ +TEST(version_cohort_touch_reports_each_lost_file) { +#ifdef _WIN32 + SKIP_PLATFORM("unlinking a held lock file is POSIX behavior"); +#else + version_cohort_fixture_t fixture; + ASSERT_TRUE(version_cohort_fixture_start(&fixture, "touch")); + cbm_version_cohort_manager_t *manager = cbm_version_cohort_manager_new(fixture.endpoint); + ASSERT_NOT_NULL(manager); + cbm_daemon_build_identity_t identity = version_cohort_identity("1.0.0", VERSION_COHORT_BUILD_A); + cbm_daemon_conflict_t conflict; + cbm_version_cohort_lease_t *lease = NULL; + cbm_version_cohort_daemon_claim_t *claim = NULL; + ASSERT_EQ( + cbm_version_cohort_acquire(manager, &identity, cbm_now_ms() + 5000, &lease, &conflict), + CBM_VERSION_COHORT_OK); + ASSERT_EQ(cbm_version_cohort_daemon_claim_acquire(manager, &claim), CBM_VERSION_COHORT_OK); + + ASSERT_EQ(cbm_version_cohort_lease_touch(lease), CBM_VERSION_COHORT_OK); + ASSERT_EQ(cbm_version_cohort_daemon_claim_touch(claim), CBM_VERSION_COHORT_OK); + + const char *runtime_dir = cbm_daemon_ipc_endpoint_runtime_dir(fixture.endpoint); + char marker[VERSION_COHORT_TEST_PATH_CAP]; + char lifetime[VERSION_COHORT_TEST_PATH_CAP]; + ASSERT_TRUE(snprintf(marker, sizeof(marker), "%s/cbm-version-cohort-daemon-v1.lock", + runtime_dir) < (int)sizeof(marker)); + ASSERT_TRUE(snprintf(lifetime, sizeof(lifetime), "%s/cbm-version-cohort-lifetime-v1.lock", + runtime_dir) < (int)sizeof(lifetime)); + + ASSERT_EQ(unlink(marker), 0); + ASSERT_EQ(cbm_version_cohort_daemon_claim_touch(claim), CBM_VERSION_COHORT_UNSAFE); + ASSERT_EQ(cbm_version_cohort_lease_touch(lease), CBM_VERSION_COHORT_OK); + + ASSERT_EQ(unlink(lifetime), 0); + ASSERT_EQ(cbm_version_cohort_lease_touch(lease), CBM_VERSION_COHORT_UNSAFE); + + ASSERT_EQ(cbm_version_cohort_daemon_claim_release(&claim), CBM_PRIVATE_FILE_LOCK_OK); + version_cohort_release(&lease); + version_cohort_manager_close(&manager); + version_cohort_fixture_finish(&fixture); + PASS(); +#endif +} + TEST(version_cohort_distinguishes_coordinated_daemon_without_connecting) { version_cohort_fixture_t fixture; ASSERT_TRUE(version_cohort_fixture_start(&fixture, "daemon-marker")); @@ -1034,6 +1081,7 @@ SUITE(version_cohort) { RUN_TEST(version_cohort_mutation_waits_for_every_lifetime_participant); RUN_TEST(version_cohort_mutation_timeout_releases_all_guards); RUN_TEST(version_cohort_does_not_repurpose_daemon_startup_lock_for_lifetime); + RUN_TEST(version_cohort_touch_reports_each_lost_file); RUN_TEST(version_cohort_distinguishes_coordinated_daemon_without_connecting); RUN_TEST(version_cohort_transition_presence_is_authoritative_and_marker_checked); RUN_TEST(version_cohort_transition_shutdown_order_has_no_false_conflict); From 1133ba324656042f8c73578a788ebf5c77ec8172 Mon Sep 17 00:00:00 2001 From: Shane McCarron Date: Tue, 22 Sep 2026 11:27:26 -0500 Subject: [PATCH 3/6] fix(daemon): address QA round 2 - 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) Signed-off-by: Shane McCarron --- src/daemon/ipc.c | 47 ++++++++++++++++----- src/foundation/private_file_lock.c | 65 ++++++++++++++++++++++++------ tests/test_daemon_ipc.c | 47 +++++++++++++++++---- tests/test_version_cohort.c | 19 +++++++-- 4 files changed, 144 insertions(+), 34 deletions(-) diff --git a/src/daemon/ipc.c b/src/daemon/ipc.c index bedc5884d..a6259e1dd 100644 --- a/src/daemon/ipc.c +++ b/src/daemon/ipc.c @@ -3229,9 +3229,29 @@ static int posix_touch_fold(int result, int next) { return result == 0 || next == 0 ? 0 : (result < 0 || next < 0 ? -1 : 1); } +/* Whether base_name still names the regular file (device, inode): 1 yes, + * 0 proven not (absent, another inode, a symlink or other non-regular + * entry), -1 undetermined (any other lookup failure, e.g. EIO or ESTALE). */ +static int posix_path_names_inode(int directory_fd, const char *base_name, dev_t device, + ino_t inode) { + struct stat by_path; + if (fstatat(directory_fd, base_name, &by_path, AT_SYMLINK_NOFOLLOW) != 0) { + return errno == ENOENT ? 0 : -1; + } + return S_ISREG(by_path.st_mode) && by_path.st_dev == device && by_path.st_ino == inode ? 1 : 0; +} + static int posix_held_file_touch(int directory_fd, const char *base_name, int fd) { - if (!private_regular_file_snapshot(directory_fd, base_name, fd, 1, NULL)) { - return 0; + struct stat by_handle; + if (fstat(fd, &by_handle) != 0) { + return -1; + } + int identity = + by_handle.st_nlink == 0 + ? 0 + : posix_path_names_inode(directory_fd, base_name, by_handle.st_dev, by_handle.st_ino); + if (identity <= 0) { + return identity; } return futimens(fd, NULL) == 0 ? 1 : -1; } @@ -3241,20 +3261,27 @@ static int posix_held_lock_touch(int directory_fd, const process_lock_entry_t *e } static int posix_identity_marker_touch(const cbm_daemon_ipc_listener_t *listener) { - /* The marker is not held open; reopen it and require the published inode. - * Only ENOENT proves loss: EMFILE and friends say nothing about the file. */ + /* The marker is not held open. Classify by path first, so a symlink or + * other replacement is loss; only then reopen it to refresh, where a + * failure (EMFILE, EACCES) says nothing about the file and is transient. */ + int identity = posix_path_names_inode(listener->dir_fd, listener->socket_identity_name, + listener->identity_device, listener->identity_inode); + if (identity <= 0) { + return identity; + } int marker_fd = openat(listener->dir_fd, listener->socket_identity_name, O_RDONLY | O_CLOEXEC | O_NOFOLLOW | O_NONBLOCK); if (marker_fd < 0) { return errno == ENOENT ? 0 : -1; } struct stat marker_status; - int result = 0; - if (private_regular_file_snapshot(listener->dir_fd, listener->socket_identity_name, marker_fd, - 1, &marker_status) && - marker_status.st_dev == listener->identity_device && - marker_status.st_ino == listener->identity_inode) { - result = futimens(marker_fd, NULL) == 0 ? 1 : -1; + int result = -1; + if (fstat(marker_fd, &marker_status) == 0) { + /* Replaced between the lookup and the open. */ + result = marker_status.st_dev != listener->identity_device || + marker_status.st_ino != listener->identity_inode + ? 0 + : (futimens(marker_fd, NULL) == 0 ? 1 : -1); } (void)close(marker_fd); return result; diff --git a/src/foundation/private_file_lock.c b/src/foundation/private_file_lock.c index a5fdf797d..46111cc1a 100644 --- a/src/foundation/private_file_lock.c +++ b/src/foundation/private_file_lock.c @@ -487,6 +487,39 @@ cbm_private_file_lock_status_t cbm_private_file_lock_payload_write(cbm_private_f return valid ? CBM_PRIVATE_FILE_LOCK_OK : CBM_PRIVATE_FILE_LOCK_IO; } +/* Whether the lock's canonical path still names the held inode: 1 yes, 0 no + * (proven lost), -1 undetermined. Only proof counts as loss: unlink leaves + * st_nlink 0, a rename-away or replacement makes the path name another inode + * or nothing, and a replaced directory changes the directory's identity. Any + * other syscall failure (EIO, ESTALE, ENOMEM) says nothing about the file and + * is reported as undetermined so the caller retries instead of exiting. */ +static int private_held_identity(const cbm_private_file_lock_t *lock) { + const cbm_private_lock_directory_t *directory = lock->directory; + struct stat by_handle; + struct stat directory_by_path; + struct stat by_path; + if (fstat(lock->fd, &by_handle) != 0) { + return -1; + } + if (by_handle.st_nlink == 0) { + return 0; + } + if (lstat(directory->path, &directory_by_path) != 0) { + return errno == ENOENT || errno == ENOTDIR ? 0 : -1; + } + if (!S_ISDIR(directory_by_path.st_mode) || directory_by_path.st_dev != directory->device || + directory_by_path.st_ino != directory->inode) { + return 0; + } + if (fstatat(directory->fd, lock->base_name, &by_path, AT_SYMLINK_NOFOLLOW) != 0) { + return errno == ENOENT ? 0 : -1; + } + return S_ISREG(by_path.st_mode) && by_path.st_dev == by_handle.st_dev && + by_path.st_ino == by_handle.st_ino + ? 1 + : 0; +} + cbm_private_file_lock_status_t cbm_private_file_lock_touch(cbm_private_file_lock_t *lock) { if (!lock) { return CBM_PRIVATE_FILE_LOCK_UNSAFE; @@ -494,17 +527,17 @@ cbm_private_file_lock_status_t cbm_private_file_lock_touch(cbm_private_file_lock if (!cbm_private_file_lock_fork_guard_enter()) { return CBM_PRIVATE_FILE_LOCK_IO; } - /* Unlink leaves st_nlink 0 on the held inode, but a rename-away or a - * replaced directory keeps it linked elsewhere; only the path-vs-handle - * identity check proves the canonical path still names this lock. */ - bool linked = private_payload_fd_valid(lock, NULL) && lock->directory && - private_file_revalidate(lock->directory, lock->base_name, lock->fd, NULL); - bool touched = linked && futimens(lock->fd, NULL) == 0; - cbm_private_file_lock_fork_guard_leave(); - if (!linked) { - return CBM_PRIVATE_FILE_LOCK_UNSAFE; + cbm_private_file_lock_status_t status = CBM_PRIVATE_FILE_LOCK_UNSAFE; + if (lock->fd >= 0 && !lock->unlocked && lock->owner_pid == getpid() && lock->directory && + private_lock_is_tracked(lock)) { + int identity = private_held_identity(lock); + status = identity > 0 ? (futimens(lock->fd, NULL) == 0 ? CBM_PRIVATE_FILE_LOCK_OK + : CBM_PRIVATE_FILE_LOCK_IO) + : identity < 0 ? CBM_PRIVATE_FILE_LOCK_IO + : CBM_PRIVATE_FILE_LOCK_UNSAFE; } - return touched ? CBM_PRIVATE_FILE_LOCK_OK : CBM_PRIVATE_FILE_LOCK_IO; + cbm_private_file_lock_fork_guard_leave(); + return status; } cbm_private_file_lock_status_t cbm_private_file_lock_release(cbm_private_file_lock_t **lock_io) { @@ -1478,15 +1511,21 @@ cbm_private_file_lock_status_t cbm_private_file_lock_payload_write(cbm_private_f cbm_private_file_lock_status_t cbm_private_file_lock_touch(cbm_private_file_lock_t *lock) { /* Windows has no age-based cleaner of the private lock directory; only * confirm the held handle still names a linked file. */ - if (!lock) { + if (!lock || lock->handle == INVALID_HANDLE_VALUE || lock->unlocked) { return CBM_PRIVATE_FILE_LOCK_UNSAFE; } if (!cbm_private_file_lock_fork_guard_enter()) { return CBM_PRIVATE_FILE_LOCK_IO; } - bool linked = private_win_payload_handle_valid(lock); + BY_HANDLE_FILE_INFORMATION information; + bool queried = GetFileInformationByHandle(lock->handle, &information) != 0; cbm_private_file_lock_fork_guard_leave(); - return linked ? CBM_PRIVATE_FILE_LOCK_OK : CBM_PRIVATE_FILE_LOCK_UNSAFE; + /* A failed query says nothing about the file; only a proven unlink is loss. */ + if (!queried) { + return CBM_PRIVATE_FILE_LOCK_IO; + } + return information.nNumberOfLinks == 0 ? CBM_PRIVATE_FILE_LOCK_UNSAFE + : CBM_PRIVATE_FILE_LOCK_OK; } static bool private_win_release_unlock(cbm_private_file_lock_t *lock) { diff --git a/tests/test_daemon_ipc.c b/tests/test_daemon_ipc.c index 8107a0ef5..5fcf8c1fc 100644 --- a/tests/test_daemon_ipc.c +++ b/tests/test_daemon_ipc.c @@ -2072,16 +2072,18 @@ TEST(daemon_ipc_listener_touch_detects_lost_runtime_files) { #ifdef _WIN32 SKIP_PLATFORM("unlink-while-held of runtime files is POSIX behavior"); #else - enum { TOUCH_CASES = 4 }; + enum { TOUCH_CASES = 5 }; + enum { LOSS_UNLINK, LOSS_REPLACE_FILE, LOSS_REPLACE_SYMLINK }; static const char key[] = "2178a2178a2178a0"; static const struct { const char *suffix; - bool replace; + int loss; } cases[TOUCH_CASES] = { - {"lifetime.lock", false}, /* lifetime reservation */ - {"lock", false}, /* listener-owned participant guard */ - {"sock.identity", false}, /* identity marker, unlinked */ - {"sock.identity", true}, /* identity marker, replaced by a fresh inode */ + {"lifetime.lock", LOSS_UNLINK}, /* lifetime reservation */ + {"lock", LOSS_UNLINK}, /* listener-owned participant guard */ + {"sock.identity", LOSS_UNLINK}, /* identity marker, unlinked */ + {"sock.identity", LOSS_REPLACE_FILE}, /* ... replaced by a fresh inode */ + {"sock.identity", LOSS_REPLACE_SYMLINK} /* ... replaced by a symlink */, }; const time_t stale = 1000000000; char parent[TEST_PATH_CAP] = {0}; @@ -2091,7 +2093,9 @@ TEST(daemon_ipc_listener_touch_detects_lost_runtime_files) { int touched_intact[TOUCH_CASES] = {0}; bool refreshed[TOUCH_CASES] = {false}; bool lost[TOUCH_CASES] = {false}; - int touched_after_loss[TOUCH_CASES] = {1, 1, 1, 1}; + int touched_after_loss[TOUCH_CASES] = {1, 1, 1, 1, 1}; + int unreadable_touch = 0; + int readable_again_touch = 0; if (ipc_test_parent_new(parent, "listener-touch")) { endpoint = cbm_daemon_ipc_endpoint_new(key, parent); @@ -2099,6 +2103,19 @@ TEST(daemon_ipc_listener_touch_detects_lost_runtime_files) { if (endpoint) { ipc_test_copy_path(runtime_dir, cbm_daemon_ipc_endpoint_runtime_dir(endpoint)); } + /* A still-valid marker that merely cannot be reopened (EACCES) is a + * transient refresh failure, not loss: the daemon must keep serving. */ + if (endpoint) { + cbm_daemon_ipc_listener_t *listener = cbm_daemon_ipc_listen(endpoint); + char marker[TEST_PATH_CAP]; + int written = snprintf(marker, sizeof(marker), "%s/cbm-%s.sock.identity", runtime_dir, key); + if (listener && written > 0 && written < (int)sizeof(marker) && chmod(marker, 0) == 0) { + unreadable_touch = cbm_daemon_ipc_listener_touch(listener, NULL); + readable_again_touch = + chmod(marker, 0600) == 0 ? cbm_daemon_ipc_listener_touch(listener, NULL) : -2; + } + cbm_daemon_ipc_listener_close(listener); + } for (size_t i = 0; endpoint && i < TOUCH_CASES; i++) { cbm_daemon_ipc_listener_t *listener = cbm_daemon_ipc_listen(endpoint); started[i] = listener != NULL; @@ -2113,16 +2130,30 @@ TEST(daemon_ipc_listener_touch_detects_lost_runtime_files) { refreshed[i] = touched_intact[i] == 1 && stat(path, &after) == 0 && after.st_atime > stale && after.st_mtime > stale; lost[i] = refreshed[i] && unlink(path) == 0; - if (lost[i] && cases[i].replace) { + if (lost[i] && cases[i].loss == LOSS_REPLACE_FILE) { int fd = open(path, O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC | O_NOFOLLOW, 0600); lost[i] = fd >= 0 && close(fd) == 0; + } else if (lost[i] && cases[i].loss == LOSS_REPLACE_SYMLINK) { + lost[i] = symlink("/dev/null", path) == 0; } touched_after_loss[i] = lost[i] ? cbm_daemon_ipc_listener_touch(listener, NULL) : -2; cbm_daemon_ipc_listener_close(listener); + /* A tampered marker makes close (correctly) keep the socket pair; clear + * this case's simulated leftovers so the next case can listen. */ + static const char *const leftovers[] = {"sock.identity", "sock", "anc"}; + for (size_t j = 0; j < sizeof(leftovers) / sizeof(leftovers[0]); j++) { + written = snprintf(path, sizeof(path), "%s/cbm-%s.%s", runtime_dir, key, leftovers[j]); + if (written > 0 && written < (int)sizeof(path)) { + (void)unlink(path); + } + } } cbm_daemon_ipc_endpoint_free(endpoint); ipc_test_remove_tree(runtime_dir, parent); + /* Root bypasses mode 0, so the reopen succeeds there and the touch is 1. */ + ASSERT_EQ(unreadable_touch, geteuid() == 0 ? 1 : -1); + ASSERT_EQ(readable_again_touch, 1); for (size_t i = 0; i < TOUCH_CASES; i++) { ASSERT_TRUE(started[i]); ASSERT_EQ(touched_intact[i], 1); diff --git a/tests/test_version_cohort.c b/tests/test_version_cohort.c index f40e05c5e..d5b6d4c5a 100644 --- a/tests/test_version_cohort.c +++ b/tests/test_version_cohort.c @@ -22,6 +22,8 @@ #endif #include #else +#include +#include #include #include #endif @@ -690,9 +692,6 @@ TEST(version_cohort_touch_reports_each_lost_file) { CBM_VERSION_COHORT_OK); ASSERT_EQ(cbm_version_cohort_daemon_claim_acquire(manager, &claim), CBM_VERSION_COHORT_OK); - ASSERT_EQ(cbm_version_cohort_lease_touch(lease), CBM_VERSION_COHORT_OK); - ASSERT_EQ(cbm_version_cohort_daemon_claim_touch(claim), CBM_VERSION_COHORT_OK); - const char *runtime_dir = cbm_daemon_ipc_endpoint_runtime_dir(fixture.endpoint); char marker[VERSION_COHORT_TEST_PATH_CAP]; char lifetime[VERSION_COHORT_TEST_PATH_CAP]; @@ -701,6 +700,20 @@ TEST(version_cohort_touch_reports_each_lost_file) { ASSERT_TRUE(snprintf(lifetime, sizeof(lifetime), "%s/cbm-version-cohort-lifetime-v1.lock", runtime_dir) < (int)sizeof(lifetime)); + /* Age both held files the way a cleaner would see them, then prove the + * touch refreshed them through the held handles. */ + const time_t stale = 1000000000; + struct timespec stale_times[2] = {{.tv_sec = stale}, {.tv_sec = stale}}; + ASSERT_EQ(utimensat(AT_FDCWD, marker, stale_times, 0), 0); + ASSERT_EQ(utimensat(AT_FDCWD, lifetime, stale_times, 0), 0); + ASSERT_EQ(cbm_version_cohort_lease_touch(lease), CBM_VERSION_COHORT_OK); + ASSERT_EQ(cbm_version_cohort_daemon_claim_touch(claim), CBM_VERSION_COHORT_OK); + struct stat refreshed; + ASSERT_EQ(stat(marker, &refreshed), 0); + ASSERT_TRUE(refreshed.st_mtime > stale && refreshed.st_atime > stale); + ASSERT_EQ(stat(lifetime, &refreshed), 0); + ASSERT_TRUE(refreshed.st_mtime > stale && refreshed.st_atime > stale); + ASSERT_EQ(unlink(marker), 0); ASSERT_EQ(cbm_version_cohort_daemon_claim_touch(claim), CBM_VERSION_COHORT_UNSAFE); ASSERT_EQ(cbm_version_cohort_lease_touch(lease), CBM_VERSION_COHORT_OK); From fee73ced23e4f0753523bc2157f1fbb853f51fdd Mon Sep 17 00:00:00 2001 From: Shane McCarron Date: Fri, 25 Sep 2026 14:09:27 -0500 Subject: [PATCH 4/6] test(daemon): cover host loop stop on coordination file loss 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) Claude-Session: https://claude.ai/code/session_01NbYKGdU7pqG6AEuMQuJxiF Signed-off-by: Shane McCarron --- src/daemon/host.c | 16 +++++ src/daemon/host_internal.h | 14 ++++ tests/test_daemon_runtime.c | 131 ++++++++++++++++++++++++++++++++++++ 3 files changed, 161 insertions(+) diff --git a/src/daemon/host.c b/src/daemon/host.c index 3dea464cf..fb44acdd3 100644 --- a/src/daemon/host.c +++ b/src/daemon/host.c @@ -985,6 +985,22 @@ static bool host_wait_for_lifetime(cbm_daemon_runtime_service_t *service, } } +bool cbm_daemon_host_wait_for_lifetime_for_test(cbm_daemon_runtime_service_t *service, + const cbm_daemon_ipc_endpoint_t *endpoint, + cbm_version_cohort_lease_t *cohort_lease, + cbm_version_cohort_daemon_claim_t *daemon_claim, + cbm_daemon_ipc_participant_guard_t *guard) { + host_state_t host = {0}; + atomic_int stop_requested = 0; + host_coordination_t coordination = { + .endpoint = endpoint, + .cohort_lease = cohort_lease, + .daemon_claim = daemon_claim, + .participant_guard = guard, + }; + return host_wait_for_lifetime(service, &stop_requested, &host, false, &coordination); +} + static bool host_application_shutdown(host_state_t *host) { if (cbm_daemon_application_shutdown(host->application, HOST_APPLICATION_SHUTDOWN_MS)) { return true; diff --git a/src/daemon/host_internal.h b/src/daemon/host_internal.h index dd8856161..8573c0913 100644 --- a/src/daemon/host_internal.h +++ b/src/daemon/host_internal.h @@ -9,6 +9,10 @@ #include struct cbm_daemon_ipc_endpoint; +struct cbm_daemon_ipc_participant_guard; +struct cbm_daemon_runtime_service; +struct cbm_version_cohort_daemon_claim; +struct cbm_version_cohort_lease; typedef bool (*cbm_daemon_host_cleanup_release_for_test_fn)(void *context); @@ -68,4 +72,14 @@ bool cbm_daemon_host_http_reconcile_free_refusal_for_test( * the failure, the adapter must cancel so the final free succeeds. */ bool cbm_daemon_host_http_thread_create_failure_lifecycle_for_test(void); +/* Run the production lifetime loop over caller-held coordination handles with + * no HTTP host state and no stop request. The generation is ephemeral, so a + * loop that misses a loss still ends at the initial client window instead of + * hanging the test. Returns the loop's own result. */ +bool cbm_daemon_host_wait_for_lifetime_for_test( + struct cbm_daemon_runtime_service *service, const struct cbm_daemon_ipc_endpoint *endpoint, + struct cbm_version_cohort_lease *cohort_lease, + struct cbm_version_cohort_daemon_claim *daemon_claim, + struct cbm_daemon_ipc_participant_guard *guard); + #endif /* CBM_DAEMON_HOST_INTERNAL_H */ diff --git a/tests/test_daemon_runtime.c b/tests/test_daemon_runtime.c index 827a8cf34..5ca4c72dd 100644 --- a/tests/test_daemon_runtime.c +++ b/tests/test_daemon_runtime.c @@ -1262,6 +1262,19 @@ static bool runtime_test_fixture_start_failed(const char *tag, const char *stage static bool runtime_test_fixture_permanent = false; +typedef struct { + cbm_version_cohort_manager_t *manager; + cbm_version_cohort_lease_t *lease; + cbm_daemon_ipc_participant_guard_t *guard; + cbm_version_cohort_daemon_claim_t *claim; + bool held; +} runtime_test_coordination_t; + +/* When set, the fixture takes the host's coordination handles in the host's + * order before starting the service: cohort admission refuses once a + * generation is already serving. */ +static runtime_test_coordination_t *runtime_test_fixture_coordination = NULL; + static bool runtime_test_fixture_start_configured( runtime_test_fixture_t *fixture, const char *tag, const cbm_daemon_build_identity_t *identity, uint32_t max_clients, uint64_t lease_timeout_ms, @@ -1317,6 +1330,19 @@ static bool runtime_test_fixture_start_configured( if (application) { config.application = *application; } + runtime_test_coordination_t *coordination = runtime_test_fixture_coordination; + if (coordination) { + cbm_daemon_conflict_t conflict; + coordination->manager = cbm_version_cohort_manager_new(fixture->endpoint); + coordination->held = + coordination->manager && + cbm_version_cohort_acquire(coordination->manager, identity, cbm_now_ms() + 5000, + &coordination->lease, &conflict) == CBM_VERSION_COHORT_OK && + cbm_daemon_ipc_participant_guard_try_join(fixture->endpoint, &coordination->guard) == + 1 && + cbm_version_cohort_daemon_claim_acquire(coordination->manager, &coordination->claim) == + CBM_VERSION_COHORT_OK; + } fixture->service = cbm_daemon_runtime_service_start(&config); if (!fixture->service) { return runtime_test_fixture_start_failed(tag, "service-start", @@ -1370,6 +1396,110 @@ TEST(daemon_runtime_convenience_service_owns_participant_guard) { PASS(); } +#ifndef _WIN32 +/* The runtime's threads log concurrently while the loop stops the service, so + * the sink only sets flags. */ +static atomic_bool runtime_test_coordination_lost_logged; +static atomic_bool runtime_test_window_expired_logged; + +static void runtime_test_lifetime_log_sink(const char *line) { + /* Substrings, not key=value spelling, so text and JSON formats both match. */ + if (line && strstr(line, "daemon.lifetime_end") && strstr(line, "coordination_file_lost") && + strstr(line, "cohort_lease")) { + atomic_store(&runtime_test_coordination_lost_logged, true); + } + if (line && strstr(line, "initial_window_expired")) { + atomic_store(&runtime_test_window_expired_logged, true); + } +} +#endif + +/* #2178: the host lifetime loop must refresh every held coordination file on + * its tick and stop the generation once one is lost. The claim marker is aged + * and the cohort lifetime lock unlinked: the tick must refresh the marker + * through its held handle, name the lease (touched after the claim) as lost, + * and stop the runtime. */ +TEST(daemon_runtime_host_loop_stops_on_coordination_file_loss) { +#ifdef _WIN32 + SKIP_PLATFORM("unlinking a held lock file is POSIX behavior"); +#else + cbm_daemon_build_identity_t identity = + runtime_test_identity("2.4.0", runtime_test_self_build()); + identity.cache_fingerprint = RUNTIME_CACHE_A; /* cohort admission requires one */ + runtime_test_fixture_t fixture; + runtime_test_coordination_t coordination = {0}; + runtime_test_fixture_coordination = &coordination; + bool started = runtime_test_fixture_start(&fixture, "coordination-loss", &identity); + runtime_test_fixture_coordination = NULL; + bool held = coordination.held; + + char marker[RUNTIME_TEST_PATH_CAP]; + char lifetime[RUNTIME_TEST_PATH_CAP]; + int marker_written = snprintf(marker, sizeof(marker), "%s/cbm-version-cohort-daemon-v1.lock", + fixture.runtime_dir); + int lifetime_written = snprintf(lifetime, sizeof(lifetime), + "%s/cbm-version-cohort-lifetime-v1.lock", fixture.runtime_dir); + const time_t stale = 1000000000; + struct timespec stale_times[2] = {{.tv_sec = stale}, {.tv_sec = stale}}; + bool staged = held && marker_written > 0 && marker_written < (int)sizeof(marker) && + lifetime_written > 0 && lifetime_written < (int)sizeof(lifetime) && + utimensat(AT_FDCWD, marker, stale_times, 0) == 0 && unlink(lifetime) == 0; + + bool loop_ok = false; + if (staged) { + atomic_store(&runtime_test_coordination_lost_logged, false); + atomic_store(&runtime_test_window_expired_logged, false); + CBMLogLevel previous_log_level = cbm_log_get_level(); + cbm_log_set_level(CBM_LOG_INFO); + cbm_log_set_sink(runtime_test_lifetime_log_sink); + loop_ok = cbm_daemon_host_wait_for_lifetime_for_test( + fixture.service, fixture.endpoint, coordination.lease, coordination.claim, + coordination.guard); + cbm_log_set_sink(NULL); + cbm_log_set_level(previous_log_level); + } + cbm_daemon_runtime_service_state_t state = + started ? cbm_daemon_runtime_service_state(fixture.service) + : CBM_DAEMON_RUNTIME_SERVICE_RUNNING; + struct stat marker_status = {0}; + bool marker_refreshed = staged && stat(marker, &marker_status) == 0 && + marker_status.st_mtime > stale && marker_status.st_atime > stale; + + /* Host teardown order: service first, then guard, claim and lease. Every + * handle references the endpoint, which the fixture frees. */ + if (started && state != CBM_DAEMON_RUNTIME_SERVICE_EXITED) { + (void)cbm_daemon_runtime_service_stop(fixture.service, RUNTIME_TEST_TIMEOUT_MS); + } + if (fixture.service && cbm_daemon_runtime_service_free(fixture.service)) { + fixture.service = NULL; + } + (void)cbm_daemon_ipc_participant_guard_release(&coordination.guard); + while (coordination.claim && cbm_version_cohort_daemon_claim_release(&coordination.claim) != + CBM_PRIVATE_FILE_LOCK_OK) { + cbm_usleep(1000); + } + while (coordination.lease && + cbm_version_cohort_lease_release(&coordination.lease) != CBM_PRIVATE_FILE_LOCK_OK) { + cbm_usleep(1000); + } + while (coordination.manager && + cbm_version_cohort_manager_free(&coordination.manager) != CBM_PRIVATE_FILE_LOCK_OK) { + cbm_usleep(1000); + } + runtime_test_fixture_finish(&fixture); + + ASSERT_TRUE(started); + ASSERT_TRUE(held); + ASSERT_TRUE(staged); + ASSERT_TRUE(loop_ok); + ASSERT_EQ(state, CBM_DAEMON_RUNTIME_SERVICE_EXITED); + ASSERT_TRUE(marker_refreshed); + ASSERT_TRUE(atomic_load(&runtime_test_coordination_lost_logged)); + ASSERT_FALSE(atomic_load(&runtime_test_window_expired_logged)); + PASS(); +#endif +} + static bool runtime_test_fixture_start_application(runtime_test_fixture_t *fixture, const char *tag, const cbm_daemon_build_identity_t *identity, runtime_application_context_t *context) { @@ -5106,6 +5236,7 @@ SUITE(daemon_runtime) { RUN_TEST(daemon_runtime_process_fingerprint_never_hashes_replacement_path); #endif RUN_TEST(daemon_runtime_convenience_service_owns_participant_guard); + RUN_TEST(daemon_runtime_host_loop_stops_on_coordination_file_loss); RUN_TEST(daemon_runtime_rendezvous_layout_is_frozen_and_detailed_abi_independent); RUN_TEST(daemon_runtime_exact_hello_issues_connection_bound_identity); RUN_TEST(daemon_runtime_image_rejection_reaches_client_issue1383); From b15e7f91b95958555ac23a575b0bd1ebbfec7ddf Mon Sep 17 00:00:00 2001 From: Shane McCarron Date: Fri, 25 Sep 2026 14:09:27 -0500 Subject: [PATCH 5/6] style(tests): drop unrelated reflow hunks Restore main's formatting on pre-existing lines that a whole-file clang-format pass had reflowed, so the diff carries only the #2178 change. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01NbYKGdU7pqG6AEuMQuJxiF Signed-off-by: Shane McCarron --- tests/test_daemon_ipc.c | 7 ++++--- tests/test_version_cohort.c | 6 +++--- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/tests/test_daemon_ipc.c b/tests/test_daemon_ipc.c index 5fcf8c1fc..5a4e0e386 100644 --- a/tests/test_daemon_ipc.c +++ b/tests/test_daemon_ipc.c @@ -1004,8 +1004,8 @@ TEST(daemon_ipc_windows_sid_trust_accepts_local_admin_rejects_foreign_500) { (void)CreateWellKnownSid(WinBuiltinAdministratorsSid, NULL, NULL, &builtin_needed); if (builtin_needed > 0) { builtin_admins = malloc(builtin_needed); - if (builtin_admins && !CreateWellKnownSid(WinBuiltinAdministratorsSid, NULL, builtin_admins, - &builtin_needed)) { + if (builtin_admins && + !CreateWellKnownSid(WinBuiltinAdministratorsSid, NULL, builtin_admins, &builtin_needed)) { free(builtin_admins); builtin_admins = NULL; } @@ -5362,7 +5362,8 @@ TEST(daemon_ipc_posix_single_uid_userns_real_smoke_issue1830) { if (WEXITSTATUS(status) != 0 && WEXITSTATUS(status) != 1) { char unexpected[128]; (void)snprintf(unexpected, sizeof(unexpected), - "userns probe exited %d -- not a security verdict", WEXITSTATUS(status)); + "userns probe exited %d -- not a security verdict", + WEXITSTATUS(status)); FAIL(unexpected); } ASSERT_EQ(0, WEXITSTATUS(status)); diff --git a/tests/test_version_cohort.c b/tests/test_version_cohort.c index d5b6d4c5a..f48bd7225 100644 --- a/tests/test_version_cohort.c +++ b/tests/test_version_cohort.c @@ -332,9 +332,9 @@ TEST(version_cohort_conflict_is_retried_until_the_deadline) { /* An indefinite deadline never waits on a conflicting holder. */ retries_before = cbm_version_cohort_conflict_retries_for_testing(); - ASSERT_EQ( - cbm_version_cohort_acquire(second, &requested, UINT64_MAX, &requested_lease, &conflict), - CBM_VERSION_COHORT_CONFLICT); + ASSERT_EQ(cbm_version_cohort_acquire(second, &requested, UINT64_MAX, &requested_lease, + &conflict), + CBM_VERSION_COHORT_CONFLICT); ASSERT_NULL(requested_lease); ASSERT_EQ(cbm_version_cohort_conflict_retries_for_testing(), retries_before); From 8cd2ee5f421d0b7c06022091d59dbe71002de30d Mon Sep 17 00:00:00 2001 From: Shane McCarron Date: Fri, 25 Sep 2026 14:39:51 -0500 Subject: [PATCH 6/6] fix(daemon): address QA round 4 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) Claude-Session: https://claude.ai/code/session_01NbYKGdU7pqG6AEuMQuJxiF Signed-off-by: Shane McCarron --- tests/test_daemon_runtime.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/test_daemon_runtime.c b/tests/test_daemon_runtime.c index 5ca4c72dd..5187bb7a4 100644 --- a/tests/test_daemon_runtime.c +++ b/tests/test_daemon_runtime.c @@ -1272,7 +1272,8 @@ typedef struct { /* When set, the fixture takes the host's coordination handles in the host's * order before starting the service: cohort admission refuses once a - * generation is already serving. */ + * generation is already serving. Unlike the host's start_reserved, the + * fixture's convenience start also joins a service-owned participant guard. */ static runtime_test_coordination_t *runtime_test_fixture_coordination = NULL; static bool runtime_test_fixture_start_configured( @@ -1452,9 +1453,9 @@ TEST(daemon_runtime_host_loop_stops_on_coordination_file_loss) { CBMLogLevel previous_log_level = cbm_log_get_level(); cbm_log_set_level(CBM_LOG_INFO); cbm_log_set_sink(runtime_test_lifetime_log_sink); - loop_ok = cbm_daemon_host_wait_for_lifetime_for_test( - fixture.service, fixture.endpoint, coordination.lease, coordination.claim, - coordination.guard); + loop_ok = cbm_daemon_host_wait_for_lifetime_for_test(fixture.service, fixture.endpoint, + coordination.lease, coordination.claim, + coordination.guard); cbm_log_set_sink(NULL); cbm_log_set_level(previous_log_level); }