feat(raft-apply-decoupling): arm timeout for async commit continuations (task 1.3)
Converting COMMIT_MODE_WAIT away from a blocking wait removed the only mechanism that ever detected a commit timing out (the caller's own pthread_cond_timedwait deadline). Adds a dedicated per-commit-engine sweep thread that arms an equivalent timeout for async-registered continuations, resolving FAILED/-ETIMEDOUT within the same DEFAULT_SYNC_TIMEOUT_MS window the old blocking path used, with no thread parked to detect it. Chose a private sweep thread over the shared delaymgr: wiring delaymgr would touch cluster.c/ha.c (out of boundary) and its 200ms poll granularity vs the 500ms default timeout would add jitter, breaking the "identical failure timing" no-regression bar. Round 1 review rejected a lost-wakeup race: the sweep loop computed its next-deadline snapshot under lock, unlocked to dispatch, then slept on the stale snapshot -- a registration landing mid-dispatch had its broadcast lost (no thread parked on the condvar), so the sweep thread could sleep on an absent/late deadline indefinitely. Fixed by holding the lock continuously across scan-then-wait decisions and always re-scanning immediately before ever waiting (recheck-then-wait idiom). Round 2 review independently reproduced the round-1 bug shape against the fix to confirm it actually closes the race, not just narrows it.
This commit is contained in:
parent
fd7a0746eb
commit
530a02bda3
4 changed files with 461 additions and 4 deletions
|
|
@ -26,6 +26,29 @@
|
|||
* §Commit Engine). n_peers <= 1 self-quorum advances committed_lsn
|
||||
* through the single advancement path and completes immediately.
|
||||
*
|
||||
* raft-apply-decoupling task 1.3: commit_submit_async() registers a
|
||||
* continuation instead of parking a caller thread, which removes the ONLY
|
||||
* mechanism (the caller's own pthread_cond_timedwait deadline) that ever
|
||||
* detected a commit timing out. commit_engine_t therefore owns a dedicated
|
||||
* timeout-sweep thread (commit_sweep_worker, started in commit_engine_init /
|
||||
* stopped+joined in commit_engine_fini) that sleeps on the SAME ce->cond
|
||||
* used everywhere else in this file, waking either on a broadcast (new
|
||||
* registration, majority advance, membership change, gate change) or at the
|
||||
* earliest pending continuation's deadline, whichever comes first.
|
||||
*
|
||||
* WHY A DEDICATED THREAD, NOT THE SHARED CONTROLLER delaymgr: this task's
|
||||
* boundary is commit.c/commit.h only (no cluster.c/ha.c wiring changes), and
|
||||
* the shared delaymgr's production polling precision (200ms, cluster.c) is
|
||||
* coarse relative to DEFAULT_SYNC_TIMEOUT_MS (500ms) — routing through it
|
||||
* would measurably loosen the "identical failure timing" bar (Req 11.1).
|
||||
* Reusing ce->lock/ce->cond and the exact wait_deadline() computation
|
||||
* commit_submit's blocking path already uses gives the async path the same
|
||||
* precision as the blocking path it replaces, with a single thread
|
||||
* regardless of how many continuations are pending (unlike the old
|
||||
* one-parked-thread-per-waiter model). This thread is private to the commit
|
||||
* engine and never touches iomgr or delaymgr, so it does not interact with
|
||||
* either thread's non-blocking contract (.kiro/steering/ha-async-io.md).
|
||||
*
|
||||
* Requirements: 1.1, 1.2, 2.1, 2.2, 2.3, 4.3, 4.4, 5.1, 6.3, 7.1, 7.2, 7.3
|
||||
*/
|
||||
#include <errno.h>
|
||||
|
|
@ -50,6 +73,12 @@
|
|||
* that section for the full definition and contract. */
|
||||
static void commit_resolve_pending(commit_engine_t* ce);
|
||||
|
||||
/* Forward declaration: raft-apply-decoupling task 1.3. commit_engine_init
|
||||
* (below) starts the timeout-sweep thread; its entry function is defined in
|
||||
* the Timeout Sweep section further down this file (alongside
|
||||
* commit_sweep_scan_locked, the locked-scan helper it drives). */
|
||||
static void* commit_sweep_worker(void* arg);
|
||||
|
||||
/* ============================================================
|
||||
* Internal helpers
|
||||
* ============================================================ */
|
||||
|
|
@ -267,6 +296,21 @@ static void wait_deadline(struct timespec* dl, uint32_t timeout_ms)
|
|||
}
|
||||
}
|
||||
|
||||
/* raft-apply-decoupling task 1.3: three-way compare of two CLOCK_REALTIME
|
||||
* timespecs (the same clock wait_deadline() uses). Used by the timeout-sweep
|
||||
* thread to detect an expired deadline and to track the earliest of several
|
||||
* pending ones. */
|
||||
static int ts_cmp(const struct timespec* a, const struct timespec* b)
|
||||
{
|
||||
if (a->tv_sec != b->tv_sec) {
|
||||
return (a->tv_sec < b->tv_sec) ? -1 : 1;
|
||||
}
|
||||
if (a->tv_nsec != b->tv_nsec) {
|
||||
return (a->tv_nsec < b->tv_nsec) ? -1 : 1;
|
||||
}
|
||||
return 0;
|
||||
}
|
||||
|
||||
/* Durable-advance wake (ha-commit-durability task 2.2): invoked by the persist
|
||||
* group-commit flusher AFTER it advances durable_lsn and AFTER it releases
|
||||
* p->lock. Now that the commit majority caps self at durable_lsn, an advance of
|
||||
|
|
@ -342,6 +386,30 @@ int commit_engine_init(commit_engine_t* ce, persist_t* p, int n_peers, int n_vcp
|
|||
* we only register a function + ctx, preserving the commit→persist (never
|
||||
* the reverse) dependency direction. */
|
||||
persist_set_durable_cb(p, commit_on_durable_advance, ce);
|
||||
|
||||
/* raft-apply-decoupling task 1.3: start the dedicated timeout-sweep
|
||||
* thread (see file header for why this is a private thread rather than
|
||||
* the shared controller delaymgr). A creation failure is treated as a
|
||||
* hard init failure -- like the calloc failure above -- because a
|
||||
* commit engine that cannot detect async timeouts cannot honor its
|
||||
* documented contract (Req 9.4: every registered continuation must
|
||||
* eventually resolve). */
|
||||
ce->sweep_stop = 0;
|
||||
ce->sweep_thread = 0; /* explicit sentinel: pthread_create leaves this
|
||||
* unspecified on failure (POSIX), and ce may be
|
||||
* caller-reused memory from a prior lifecycle. */
|
||||
{
|
||||
int prc = pthread_create(&ce->sweep_thread, NULL, commit_sweep_worker, ce);
|
||||
if (prc != 0) {
|
||||
persist_set_durable_cb(p, NULL, NULL);
|
||||
free(ce->continuations);
|
||||
ce->continuations = NULL;
|
||||
ce->n_continuations = 0;
|
||||
pthread_cond_destroy(&ce->cond);
|
||||
pthread_mutex_destroy(&ce->lock);
|
||||
return -prc;
|
||||
}
|
||||
}
|
||||
return 0;
|
||||
}
|
||||
|
||||
|
|
@ -364,6 +432,20 @@ void commit_engine_fini(commit_engine_t* ce)
|
|||
if (ce == NULL) {
|
||||
return;
|
||||
}
|
||||
/* raft-apply-decoupling task 1.3: stop + join the timeout-sweep thread
|
||||
* BEFORE tearing down lock/cond/continuations -- it reads all three.
|
||||
* Signal under the lock and broadcast to wake it from either
|
||||
* pthread_cond_wait (no pending continuations) or pthread_cond_timedwait
|
||||
* (a pending deadline); the join guarantees it has fully exited before
|
||||
* anything it touches is freed or destroyed. */
|
||||
if (ce->sweep_thread != 0) {
|
||||
pthread_mutex_lock(&ce->lock);
|
||||
ce->sweep_stop = 1;
|
||||
pthread_cond_broadcast(&ce->cond);
|
||||
pthread_mutex_unlock(&ce->lock);
|
||||
pthread_join(ce->sweep_thread, NULL);
|
||||
ce->sweep_thread = 0;
|
||||
}
|
||||
/* Unregister the durable-advance wake BEFORE tearing down the mutex/cond:
|
||||
* otherwise the flusher could fire commit_on_durable_advance against a
|
||||
* destroyed engine (use-after-free / ASAN). Callers are expected to have
|
||||
|
|
@ -530,7 +612,18 @@ int commit_submit_async(commit_engine_t* ce, uint64_t lsn,
|
|||
ce->continuations[slot].lsn = lsn;
|
||||
ce->continuations[slot].fn = fn;
|
||||
ce->continuations[slot].ctx = ctx;
|
||||
/* raft-apply-decoupling task 1.3: arm this registration's timeout with
|
||||
* the SAME deadline computation and SAME default constant
|
||||
* commit_submit's blocking WAIT path uses, so an async registration
|
||||
* observes identical failure timing to today's blocking path (Req
|
||||
* 11.1). The dedicated timeout-sweep thread (commit_sweep_worker)
|
||||
* resolves this continuation FAILED/-ETIMEDOUT if it is still pending
|
||||
* once this deadline passes. */
|
||||
wait_deadline(&ce->continuations[slot].deadline, DEFAULT_SYNC_TIMEOUT_MS);
|
||||
|
||||
/* Wake the sweep thread: a freshly armed deadline may be sooner than
|
||||
* whatever it is currently sleeping toward. */
|
||||
pthread_cond_broadcast(&ce->cond);
|
||||
pthread_mutex_unlock(&ce->lock);
|
||||
return 0;
|
||||
}
|
||||
|
|
@ -708,6 +801,130 @@ static void commit_resolve_pending(commit_engine_t* ce)
|
|||
}
|
||||
}
|
||||
|
||||
/* ============================================================
|
||||
* Timeout sweep (raft-apply-decoupling task 1.3)
|
||||
* ============================================================ */
|
||||
|
||||
/* One bounded, read-only classification pass over the continuation
|
||||
* registry: for every still-registered lsn, majority-satisfied ALWAYS wins
|
||||
* over an expired deadline when both are true in this same lock hold --
|
||||
* mirroring commit_submit's own last-look-before-timeout tie-break (a
|
||||
* majority ack landing exactly at the deadline is never spuriously reported
|
||||
* as a timeout). Populates committed_ready[] and failed_ready[] (via nc and
|
||||
* nf) and reports the earliest deadline among whatever remains pending via
|
||||
* next_deadline and have_next. Caller MUST hold ce->lock and does its own
|
||||
* unlocking -- this function never blocks and never dispatches, so it is
|
||||
* safe to call as the last thing done before committing to a
|
||||
* pthread_cond_wait/pthread_cond_timedwait in the SAME lock hold (see
|
||||
* commit_sweep_worker). */
|
||||
static void commit_sweep_scan_locked(commit_engine_t* ce,
|
||||
uint64_t* committed_ready, int* nc,
|
||||
uint64_t* failed_ready, int* nf,
|
||||
struct timespec* next_deadline,
|
||||
int* have_next)
|
||||
{
|
||||
struct timespec now;
|
||||
|
||||
clock_gettime(CLOCK_REALTIME, &now); /* same clock as wait_deadline() */
|
||||
*nc = 0;
|
||||
*nf = 0;
|
||||
*have_next = 0;
|
||||
|
||||
for (int i = 0; i < ce->n_continuations; i++) {
|
||||
uint64_t lsn = ce->continuations[i].lsn;
|
||||
|
||||
if (lsn == 0) {
|
||||
continue;
|
||||
}
|
||||
if (wait_done_locked(ce, lsn)) {
|
||||
committed_ready[(*nc)++] = lsn;
|
||||
} else if (ts_cmp(&now, &ce->continuations[i].deadline) >= 0) {
|
||||
failed_ready[(*nf)++] = lsn;
|
||||
} else if (!*have_next ||
|
||||
ts_cmp(&ce->continuations[i].deadline, next_deadline) < 0) {
|
||||
*next_deadline = ce->continuations[i].deadline;
|
||||
*have_next = 1;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/* Dedicated timeout-sweep thread, private to this commit_engine_t (see the
|
||||
* file header for why this is not routed through the shared controller
|
||||
* delaymgr). Sleeps on ce->cond to the earliest pending continuation
|
||||
* deadline, or indefinitely when none are pending; a broadcast (new
|
||||
* registration, majority advance, membership change, gate change, or
|
||||
* commit_engine_fini's shutdown signal) wakes it early to re-evaluate.
|
||||
* Exits once ce->sweep_stop is observed under ce->lock.
|
||||
*
|
||||
* LOST-WAKEUP FIX: commit_sweep_scan_locked() only classifies -- it never
|
||||
* dispatches -- so every dispatch (commit_resolve(), which must run
|
||||
* UNLOCKED per the lock-ordering rule) happens between an unlock and the
|
||||
* next lock in this loop. A commit_submit_async() registration (or any
|
||||
* other broadcaster; all of them broadcast while HOLDING ce->lock) landing
|
||||
* during that unlocked window would find no thread parked in cond_wait to
|
||||
* receive it -- POSIX condvars do not latch a signal sent to no waiter.
|
||||
* The fix follows the standard "recheck the wait condition in the SAME
|
||||
* lock hold that leads directly into the wait call" idiom: whenever this
|
||||
* loop has something to dispatch, it unlocks, dispatches, RE-LOCKS, and
|
||||
* loops back to scan again -- it never commits to a wait based on a
|
||||
* have_next/next_deadline computed before an unlocked dispatch step. Only
|
||||
* a scan that found NOTHING to dispatch (so no unlock is needed before the
|
||||
* wait) is trusted to size the wait. */
|
||||
static void* commit_sweep_worker(void* arg)
|
||||
{
|
||||
commit_engine_t* ce = (commit_engine_t*)arg;
|
||||
|
||||
pthread_mutex_lock(&ce->lock);
|
||||
for (;;) {
|
||||
uint64_t committed_ready[ce->n_continuations]; /* VLA: bounded
|
||||
* by n_vcpus,
|
||||
* see the
|
||||
* commit.h
|
||||
* sizing
|
||||
* invariant */
|
||||
uint64_t failed_ready[ce->n_continuations];
|
||||
int nc, nf, have_next;
|
||||
struct timespec next_deadline;
|
||||
|
||||
if (ce->sweep_stop) {
|
||||
break;
|
||||
}
|
||||
|
||||
commit_sweep_scan_locked(ce, committed_ready, &nc, failed_ready, &nf,
|
||||
&next_deadline, &have_next);
|
||||
|
||||
if (nc > 0 || nf > 0) {
|
||||
pthread_mutex_unlock(&ce->lock);
|
||||
for (int i = 0; i < nc; i++) {
|
||||
commit_resolve(ce, committed_ready[i], COMMIT_OUTCOME_COMMITTED, 0);
|
||||
}
|
||||
for (int i = 0; i < nf; i++) {
|
||||
WARN("commit: async continuation deadline expired without "
|
||||
"commit (lsn=%lu committed_lsn=%lu writable_lsn=%lu "
|
||||
"timeout_ms=%u)",
|
||||
failed_ready[i], atomic_load(&ce->p->committed_lsn),
|
||||
ce->writable_lsn, (unsigned)DEFAULT_SYNC_TIMEOUT_MS);
|
||||
commit_resolve(ce, failed_ready[i], COMMIT_OUTCOME_FAILED,
|
||||
-ETIMEDOUT);
|
||||
}
|
||||
pthread_mutex_lock(&ce->lock);
|
||||
continue; /* re-scan under the lock before ever waiting */
|
||||
}
|
||||
|
||||
/* Nothing to dispatch: this scan just ran in the exact lock hold we
|
||||
* are about to wait under, so no broadcast between "now" and the
|
||||
* wait call below can be missed. */
|
||||
if (have_next) {
|
||||
pthread_cond_timedwait(&ce->cond, &ce->lock, &next_deadline);
|
||||
} else {
|
||||
pthread_cond_wait(&ce->cond, &ce->lock);
|
||||
}
|
||||
}
|
||||
pthread_mutex_unlock(&ce->lock);
|
||||
|
||||
return NULL;
|
||||
}
|
||||
|
||||
/* ============================================================
|
||||
* ACK and membership events
|
||||
* ============================================================ */
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue