Skip to content

Commit 9104feb

Browse files
committed
fix(daemon): retry a mismatched cohort holder until the caller's deadline
A participant admitted to the version cohort holds the cohort lifetime lock SH until its lease is released at exit. An internal daemon that has just lost its last stdio client keeps that lock through its teardown, so a local CLI (`config get`, `index_status`, ...) arriving in that few-hundred-ms window met a holder whose cache root differed and was refused immediately with "active account daemon uses a different cache directory" — a pure lifecycle race, previously masked by the slow scalar self-hash and exposed as soon as the hash got fast (#1767 pr-smoke on macOS). cbm_version_cohort_acquire now retries a CONFLICT until the caller's finite deadline_ms, holding no guard between attempts, exactly as host.c already waits out the same handoff for the daemon claim marker. Every production caller passes a finite deadline; UINT64_MAX keeps failing fast so a genuine conflict against a long-lived peer is never waited on indefinitely. Tests (deterministic, no timing assertions on transient windows): one proves the retry runs until the deadline and a UINT64_MAX caller still fails immediately; one proves a waiter that met the mismatched holder is admitted the moment the holder releases. Both fail with the retry removed. Fixes #2046 Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
1 parent 2f9828d commit 9104feb

3 files changed

Lines changed: 224 additions & 9 deletions

File tree

‎src/daemon/version_cohort.c‎

Lines changed: 76 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
#include "foundation/platform.h"
88
#include "foundation/private_file_lock_internal.h"
99

10+
#include <stdatomic.h>
1011
#include <stdbool.h>
1112
#include <stdint.h>
1213
#include <stdio.h>
@@ -414,11 +415,10 @@ static cbm_version_cohort_status_t version_cohort_claim_new(
414415
CBM_PRIVATE_FILE_LOCK_SH, deadline_ms, &lease->lifetime));
415416
}
416417

417-
cbm_version_cohort_status_t cbm_version_cohort_acquire(cbm_version_cohort_manager_t *manager,
418-
const cbm_daemon_build_identity_t *identity,
419-
uint64_t deadline_ms,
420-
cbm_version_cohort_lease_t **lease_out,
421-
cbm_daemon_conflict_t *conflict_out) {
418+
static cbm_version_cohort_status_t version_cohort_acquire_once(
419+
cbm_version_cohort_manager_t *manager, const cbm_daemon_build_identity_t *identity,
420+
uint64_t deadline_ms, cbm_version_cohort_lease_t **lease_out,
421+
cbm_daemon_conflict_t *conflict_out) {
422422
if (lease_out) {
423423
*lease_out = NULL;
424424
}
@@ -569,6 +569,77 @@ cbm_version_cohort_status_t cbm_version_cohort_acquire(cbm_version_cohort_manage
569569
return CBM_VERSION_COHORT_OK;
570570
}
571571

572+
#ifdef CBM_ENABLE_TEST_SEAMS
573+
/* #2046 test seam: counts conflict retries so a test can prove the waiting
574+
* participant actually met the mismatched holder before that holder left —
575+
* without it, "admitted after release" is indistinguishable from "arrived
576+
* after release" and the test passes vacuously. */
577+
static atomic_uint_fast64_t version_cohort_conflict_retry_seam;
578+
uint64_t cbm_version_cohort_conflict_retries_for_testing(void) {
579+
return atomic_load_explicit(&version_cohort_conflict_retry_seam, memory_order_acquire);
580+
}
581+
static void version_cohort_note_conflict_retry(void) {
582+
(void)atomic_fetch_add_explicit(&version_cohort_conflict_retry_seam, 1, memory_order_release);
583+
}
584+
#else
585+
static void version_cohort_note_conflict_retry(void) {}
586+
#endif
587+
588+
static const char *version_cohort_conflict_kind(const cbm_daemon_conflict_t *conflict) {
589+
switch (conflict->status) {
590+
case CBM_DAEMON_HELLO_VERSION_CONFLICT:
591+
return "version";
592+
case CBM_DAEMON_HELLO_BUILD_CONFLICT:
593+
return "build";
594+
case CBM_DAEMON_HELLO_PROTOCOL_ABI_CONFLICT:
595+
return "protocol_abi";
596+
case CBM_DAEMON_HELLO_STORE_ABI_CONFLICT:
597+
return "store_abi";
598+
case CBM_DAEMON_HELLO_FEATURE_ABI_CONFLICT:
599+
return "feature_abi";
600+
case CBM_DAEMON_HELLO_CACHE_CONFLICT:
601+
return "cache_root";
602+
default:
603+
return "unknown";
604+
}
605+
}
606+
607+
/* A mismatched live holder is not necessarily a peer that will stay: an
608+
* internal daemon keeps its lifetime SH lock for the few hundred milliseconds
609+
* between its last client leaving and its teardown finishing, and a CLI under
610+
* another CBM_CACHE_DIR arriving inside that window used to be refused with a
611+
* cache-root conflict although nothing was running any more (#2046; the
612+
* refusal was masked only by the local CLI's own slow startup). host.c already
613+
* waits out the same handoff for the daemon claim marker. Admission therefore
614+
* retries a conflict until the caller's deadline, holding NO guard between
615+
* attempts so compatible peers and activations are never queued behind the
616+
* wait. On deadline the last observed conflict is reported exactly as before;
617+
* an indefinite deadline keeps failing fast because a mismatched holder may
618+
* never leave. */
619+
cbm_version_cohort_status_t cbm_version_cohort_acquire(cbm_version_cohort_manager_t *manager,
620+
const cbm_daemon_build_identity_t *identity,
621+
uint64_t deadline_ms,
622+
cbm_version_cohort_lease_t **lease_out,
623+
cbm_daemon_conflict_t *conflict_out) {
624+
bool retry_logged = false;
625+
for (;;) {
626+
cbm_version_cohort_status_t status =
627+
version_cohort_acquire_once(manager, identity, deadline_ms, lease_out, conflict_out);
628+
if (status != CBM_VERSION_COHORT_CONFLICT || deadline_ms == UINT64_MAX ||
629+
cbm_now_ms() >= deadline_ms) {
630+
return status;
631+
}
632+
if (!retry_logged) {
633+
retry_logged = true;
634+
cbm_log_info("version_cohort.conflict_retry", "reason",
635+
version_cohort_conflict_kind(conflict_out), "active_build",
636+
conflict_out->active_build_fingerprint);
637+
}
638+
version_cohort_note_conflict_retry();
639+
version_cohort_retry_sleep();
640+
}
641+
}
642+
572643
static cbm_version_cohort_status_t version_cohort_reserve_for_mutation_internal(
573644
cbm_version_cohort_manager_t *manager, uint64_t deadline_ms,
574645
cbm_version_cohort_quiesce_fn quiesce, void *quiesce_context,

‎src/daemon/version_cohort.h‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -58,10 +58,14 @@ cbm_version_cohort_manager_t *cbm_version_cohort_manager_new(
5858
/* Admission first takes the maintenance gate SH without waiting, holds it
5959
* across the short admission EX transition, then retains SH on the cohort
6060
* lifetime file. Active maintenance therefore fails fast with BUSY. Exact
61-
* identity peers share SH; a different version, build, or ABI returns
62-
* CONFLICT with conflict_out populated. deadline_ms is an absolute
63-
* cbm_now_ms() deadline; UINT64_MAX waits indefinitely. Every non-NULL
64-
* lease_out, including cleanup-only IO state, must be released. */
61+
* identity peers share SH; a different version, build, ABI, or cache root
62+
* is a CONFLICT — retried, with every guard released between attempts, until
63+
* deadline_ms, because the mismatched holder is often a draining daemon or a
64+
* short-lived CLI that is about to leave (#2046). Only when the deadline
65+
* passes is CONFLICT returned with conflict_out populated. deadline_ms is an
66+
* absolute cbm_now_ms() deadline; UINT64_MAX waits indefinitely for locks but
67+
* never for a conflicting holder. Every non-NULL lease_out, including
68+
* cleanup-only IO state, must be released. */
6569
cbm_version_cohort_status_t cbm_version_cohort_acquire(cbm_version_cohort_manager_t *manager,
6670
const cbm_daemon_build_identity_t *identity,
6771
uint64_t deadline_ms,
@@ -153,4 +157,11 @@ bool cbm_version_cohort_log_conflict(const cbm_daemon_conflict_t *conflict);
153157
* is active but no current-generation coordination marker can be verified. */
154158
bool cbm_version_cohort_log_uncoordinated_daemon(const cbm_daemon_build_identity_t *requested);
155159

160+
#ifdef CBM_ENABLE_TEST_SEAMS
161+
/* Monotonic count of admission attempts that met a mismatched live holder and
162+
* retried (#2046). Lets a test prove the waiter observed the conflict before
163+
* the holder left, instead of passing vacuously by arriving late. */
164+
uint64_t cbm_version_cohort_conflict_retries_for_testing(void);
165+
#endif
166+
156167
#endif /* CBM_DAEMON_VERSION_COHORT_H */

‎tests/test_version_cohort.c‎

Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -265,6 +265,137 @@ TEST(version_cohort_rejects_exact_build_with_different_cache_root) {
265265
PASS();
266266
}
267267

268+
typedef struct {
269+
cbm_version_cohort_manager_t *manager;
270+
cbm_daemon_build_identity_t identity;
271+
uint64_t deadline_ms;
272+
cbm_version_cohort_status_t status;
273+
cbm_version_cohort_lease_t *lease;
274+
cbm_daemon_conflict_t conflict;
275+
atomic_bool finished;
276+
} version_cohort_acquire_wait_t;
277+
278+
static void *version_cohort_acquire_wait_thread(void *context) {
279+
version_cohort_acquire_wait_t *wait = context;
280+
wait->status = cbm_version_cohort_acquire(wait->manager, &wait->identity, wait->deadline_ms,
281+
&wait->lease, &wait->conflict);
282+
atomic_store_explicit(&wait->finished, true, memory_order_release);
283+
return NULL;
284+
}
285+
286+
static bool version_cohort_wait_for_retries_above(uint64_t baseline, uint64_t deadline_ms) {
287+
while (cbm_version_cohort_conflict_retries_for_testing() <= baseline &&
288+
cbm_now_ms() < deadline_ms) {
289+
cbm_usleep(1000);
290+
}
291+
return cbm_version_cohort_conflict_retries_for_testing() > baseline;
292+
}
293+
294+
/* #2046: a mismatched holder that never leaves is still a conflict — but the
295+
* caller's deadline is the budget for waiting it out, so the refusal may only
296+
* arrive once that budget is spent, and must carry the same detail as before.
297+
* The lower bound is the contract under test, not a timing guess: the holder
298+
* is released only after the call returns. */
299+
TEST(version_cohort_conflict_is_retried_until_the_deadline) {
300+
version_cohort_fixture_t fixture;
301+
ASSERT_TRUE(version_cohort_fixture_start(&fixture, "conflict-deadline"));
302+
cbm_version_cohort_manager_t *first = cbm_version_cohort_manager_new(fixture.endpoint);
303+
cbm_version_cohort_manager_t *second = cbm_version_cohort_manager_new(fixture.endpoint);
304+
ASSERT_NOT_NULL(first);
305+
ASSERT_NOT_NULL(second);
306+
307+
cbm_daemon_build_identity_t active = version_cohort_identity("2.4.0", VERSION_COHORT_BUILD_A);
308+
cbm_daemon_build_identity_t requested = active;
309+
active.cache_fingerprint = VERSION_COHORT_CACHE_A;
310+
requested.cache_fingerprint = VERSION_COHORT_CACHE_B;
311+
cbm_version_cohort_lease_t *active_lease = NULL;
312+
cbm_version_cohort_lease_t *requested_lease = NULL;
313+
cbm_daemon_conflict_t conflict;
314+
ASSERT_EQ(cbm_version_cohort_acquire(first, &active, UINT64_MAX, &active_lease, &conflict),
315+
CBM_VERSION_COHORT_OK);
316+
317+
uint64_t retries_before = cbm_version_cohort_conflict_retries_for_testing();
318+
uint64_t deadline = cbm_now_ms() + 300U;
319+
cbm_version_cohort_status_t status =
320+
cbm_version_cohort_acquire(second, &requested, deadline, &requested_lease, &conflict);
321+
uint64_t returned_at = cbm_now_ms();
322+
323+
ASSERT_EQ(status, CBM_VERSION_COHORT_CONFLICT);
324+
ASSERT_NULL(requested_lease);
325+
ASSERT_TRUE(returned_at >= deadline);
326+
ASSERT_TRUE(cbm_version_cohort_conflict_retries_for_testing() > retries_before);
327+
ASSERT_EQ(conflict.status, CBM_DAEMON_HELLO_CACHE_CONFLICT);
328+
ASSERT_STR_EQ(conflict.active_cache_fingerprint, VERSION_COHORT_CACHE_A);
329+
ASSERT_STR_EQ(conflict.requested_cache_fingerprint, VERSION_COHORT_CACHE_B);
330+
331+
/* An indefinite deadline never waits on a conflicting holder. */
332+
retries_before = cbm_version_cohort_conflict_retries_for_testing();
333+
ASSERT_EQ(cbm_version_cohort_acquire(second, &requested, UINT64_MAX, &requested_lease,
334+
&conflict),
335+
CBM_VERSION_COHORT_CONFLICT);
336+
ASSERT_NULL(requested_lease);
337+
ASSERT_EQ(cbm_version_cohort_conflict_retries_for_testing(), retries_before);
338+
339+
version_cohort_release(&active_lease);
340+
version_cohort_manager_close(&second);
341+
version_cohort_manager_close(&first);
342+
version_cohort_fixture_finish(&fixture);
343+
PASS();
344+
}
345+
346+
/* #2046: the draining-daemon handoff. The waiter is admitted once the
347+
* mismatched holder leaves — and the test releases that holder only after the
348+
* seam proves the waiter already met it, so "admitted" cannot mean "arrived
349+
* late". The waiter's deadline is a backstop against a hang, never the
350+
* decider: the asserted state is OK-with-lease after the release. */
351+
TEST(version_cohort_conflict_waiter_is_admitted_when_the_holder_leaves) {
352+
version_cohort_fixture_t fixture;
353+
ASSERT_TRUE(version_cohort_fixture_start(&fixture, "conflict-handoff"));
354+
cbm_version_cohort_manager_t *first = cbm_version_cohort_manager_new(fixture.endpoint);
355+
cbm_version_cohort_manager_t *second = cbm_version_cohort_manager_new(fixture.endpoint);
356+
ASSERT_NOT_NULL(first);
357+
ASSERT_NOT_NULL(second);
358+
359+
cbm_daemon_build_identity_t active = version_cohort_identity("2.4.0", VERSION_COHORT_BUILD_A);
360+
active.cache_fingerprint = VERSION_COHORT_CACHE_A;
361+
cbm_version_cohort_lease_t *active_lease = NULL;
362+
cbm_daemon_conflict_t conflict;
363+
ASSERT_EQ(cbm_version_cohort_acquire(first, &active, UINT64_MAX, &active_lease, &conflict),
364+
CBM_VERSION_COHORT_OK);
365+
366+
version_cohort_acquire_wait_t wait;
367+
memset(&wait, 0, sizeof(wait));
368+
wait.manager = second;
369+
wait.identity = active;
370+
wait.identity.cache_fingerprint = VERSION_COHORT_CACHE_B;
371+
wait.deadline_ms = cbm_now_ms() + 10000U;
372+
wait.status = CBM_VERSION_COHORT_IO;
373+
atomic_init(&wait.finished, false);
374+
375+
uint64_t retries_before = cbm_version_cohort_conflict_retries_for_testing();
376+
cbm_thread_t thread;
377+
bool started = cbm_thread_create(&thread, 0, version_cohort_acquire_wait_thread, &wait) == 0;
378+
bool met_holder =
379+
started && version_cohort_wait_for_retries_above(retries_before, cbm_now_ms() + 5000U);
380+
bool still_waiting = started && !atomic_load_explicit(&wait.finished, memory_order_acquire);
381+
version_cohort_release(&active_lease);
382+
bool finished = started && version_cohort_wait_for_atomic(&wait.finished, cbm_now_ms() + 5000U);
383+
bool joined = started && cbm_thread_join(&thread) == 0;
384+
385+
version_cohort_release(&wait.lease);
386+
version_cohort_manager_close(&second);
387+
version_cohort_manager_close(&first);
388+
version_cohort_fixture_finish(&fixture);
389+
390+
ASSERT_TRUE(started);
391+
ASSERT_TRUE(met_holder);
392+
ASSERT_TRUE(still_waiting);
393+
ASSERT_TRUE(finished);
394+
ASSERT_TRUE(joined);
395+
ASSERT_EQ(wait.status, CBM_VERSION_COHORT_OK);
396+
PASS();
397+
}
398+
268399
TEST(version_cohort_exclusive_activation_blocks_and_is_blocked_by_participants) {
269400
version_cohort_fixture_t fixture;
270401
ASSERT_TRUE(version_cohort_fixture_start(&fixture, "activation"));
@@ -896,6 +1027,8 @@ SUITE(version_cohort) {
8961027
RUN_TEST(version_cohort_rejects_same_hash_with_different_abi);
8971028
RUN_TEST(version_cohort_rejects_missing_cache_fingerprint);
8981029
RUN_TEST(version_cohort_rejects_exact_build_with_different_cache_root);
1030+
RUN_TEST(version_cohort_conflict_is_retried_until_the_deadline);
1031+
RUN_TEST(version_cohort_conflict_waiter_is_admitted_when_the_holder_leaves);
8991032
RUN_TEST(version_cohort_exclusive_activation_blocks_and_is_blocked_by_participants);
9001033
RUN_TEST(version_cohort_mutation_intent_fails_new_admission_and_spans_lease);
9011034
RUN_TEST(version_cohort_mutation_waits_for_every_lifetime_participant);

0 commit comments

Comments
 (0)