From 3cf2a6663f36a6a366806bc78e4cc0678e1f1b55 Mon Sep 17 00:00:00 2001 From: Shane McCarron Date: Mon, 21 Sep 2026 13:02:07 -0500 Subject: [PATCH] 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 | 107 ++++++++++++-------- src/daemon/ipc.h | 26 +++-- src/daemon/runtime.c | 6 +- src/daemon/runtime.h | 10 +- src/daemon/version_cohort.c | 40 +++++--- 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, 329 insertions(+), 129 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"; +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; } - 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 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); -} - -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; +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; } - 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. */ + 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); - 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); + if (marker_fd < 0) { + return errno == ENOENT ? 0 : -1; } - return lifetime && participant && marker; + 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; +} + +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 0; + } + 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 *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 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; -} - -bool cbm_version_cohort_lease_touch(cbm_version_cohort_lease_t *lease) { - if (!lease) { - return false; +/* 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; } - /* 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_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_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_lease_touch(cbm_version_cohort_lease_t *lease) { + if (!lease || !lease->lifetime) { + return CBM_VERSION_COHORT_UNSAFE; + } + 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); +} + +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);