diff --git a/.wolf/buglog.json b/.wolf/buglog.json index e619b82..ebe66ed 100644 --- a/.wolf/buglog.json +++ b/.wolf/buglog.json @@ -12640,7 +12640,7 @@ "error_message": "[xce-test-ha --chaotic] genuine, reproducible election-liveness stall (not the bug-1418 harness process-stacking artifact, which was a separate, now-fixed issue): after 2+ overlapping kills, the surviving 2-node set can permanently deadlock -- neither node can ever become primary again for the rest of the run.", "file": "src/controller/persist.c (apply-correctness verification) + src/controller/ha.c (PreVote freshness gate) + WAL duplicate-submit mechanism (unlocated)", "root_cause": "Direct log/WAL analysis (waldump on the affected node's data dir) during a live repro superseded two prior, less-precise theories filed as bug-1418 (a shell-script process-stacking artifact, unrelated and separately fixed; and an unverified ha->term/ha->raft.term \"term desync\" claim that does not match the code -- raft_begin_pre_vote/ha_start_prevote_async both read s->term == ha->raft.term directly, there is no separate stale-field read on the PreVote path).\n\nACTUAL mechanism observed: waldump on ctrl-3's WAL (the node with the longest/freshest log after the chaotic kill sequence) showed \"duplicate submits: 3 (byte-identical: 0)\" -- the SAME jobid appended to the WAL twice with DIFFERING payloads (different subtime, different CRC), e.g. lsn=13 and lsn=14 both jobid=0-0-1783418222-0 but distinct subtime/PCRC. This is NOT bug-845's mechanism (that fix, landed 2026-06-23, was for byte-IDENTICAL double-hook-registration dupes and is confirmed still working -- these are non-identical). count_jobmgr_entries() legitimately sees these as 2 distinct job-table mutations (or the entity count otherwise fails to match the WAL-replay-derived expectation), so persist_recover()'s post-replay apply-correctness check (task 5.1, Req 8.1/8.2) correctly fires APPLY_DIVERGENT -> the loud, sticky, non-promotable fence (raft-apply-decoupling Req 8.3/8.4) -- this fencing behavior is WORKING AS DESIGNED, not itself a bug.\n\nThe resulting LIVENESS bug: with the fenced node holding the only sufficiently-fresh (longest) log, the other surviving node can never win the PreVote freshness gate against it (correctly, per Raft/PreVote semantics -- it really is behind), while the fenced node itself can win Raft elections (quorum_reached/became_leader observed at terms 27/30/32 in logs) but its own raft_cb_become_leader promotion gate refuses CTRL_PRIMARY every time (apply_divergent=1). With the third node dead (killed by the chaotic driver) and never restarted long enough to catch up, NO node can ever become primary again -- a genuine, reproducible liveness deadlock, distinct from (and downstream of) the WAL duplicate-submit root cause. Confirmed via direct log grep across ctrl-1.log/ctrl-3.log at the exact stall timestamps (09:57:16 divergence detected, repeating \"PreVote DENIED ... reason=freshness\" and \"promotion REFUSED ... apply_divergent=1\" through 10:05+); NOT root-caused via speculation.", - "fix": "NOT FIXED. Two separate, layered problems, neither resolved here: (1) the WAL duplicate-non-identical-submit mechanism itself is unlocated -- needs its own investigation (likely a distinct submit-path race from bug-845's already-fixed double-hook-registration, since these pairs are NOT byte-identical; check for a retry/resubmit path that re-stamps subtime on the same client-generated jobid, or a race in the commit-continuation/apply-pump interaction from raft-apply-decoupling that could double-materialize a submit under a specific kill-timing window). (2) Even with (1) fixed, the raft-apply-decoupling promotion-refusal design has a known, currently-unaddressed liveness gap when the ONLY sufficiently-fresh candidate is apply-divergent and the other members cannot catch up (no reconciliation/truncation-then-promote recovery path exists today) -- this needs its own design discussion, not a quick patch, since forcing promotion of a divergent node or truncating its uncommitted-but-locally-applied entries both have real correctness implications.", + "fix": "PARTIALLY FIXED (2026-07-07). Root cause of layer (1), the WAL duplicate-non-identical-submit mechanism, is now located and fixed: raft-apply-decoupling task 3.1 deferred job/entity construction in c->jobmgr until AFTER commit (apply_committed_entry, post-commit apply pump), but jobid_range_exists() -- the sole dup guard in handle_jobsubmit -- only checks c->jobmgr. A submission that is durably WAL-appended and awaiting its commit outcome does not yet exist in c->jobmgr, so a client-side retry for the SAME jobid (XCECLI_FAILOVER_CALL/xce_failover_classify's idempotent-replay contract resends on a transport error or failover-mid-flight) sailed straight past the guard and appended a second, non-identical WAL_JOB_SUBMIT entry (different subtime/CRC) for the same jobid -- exactly the 'duplicate submits: N (byte-identical: 0)' signature waldump showed. Fix: src/controller/job.c adds a process-lifetime in-flight-jobid-range registry (s_inflight_submits, guarded by s_inflight_submits_lock) that handle_jobsubmit atomically check-and-claims (inflight_range_try_add) immediately after the existing jobid_range_exists check, and releases (inflight_range_remove) from jobsubmit_async_ctx_release -- the same release point already shared by the commit-continuation and apply-complete paths, so the range stays claimed for exactly the window a duplicate could land in. A resubmit for a still-in-flight jobid now gets -EEXIST immediately, with no second WAL append. Regression test test_jobsubmit_resubmit_before_resolution_rejected_no_dup_wal_entry added to test_commit_barrier_reply.c (RED confirmed -EEXIST not returned pre-fix; GREEN after). Verified via the full 78-group controller_unit_test sweep (782 tests, 0 failures) -- zero regressions, including the 6 apply-pump/commit-barrier groups most adjacent to this change.\n\nLayer (2), the PreVote-freshness/apply-divergence liveness deadlock itself (when the ONLY sufficiently-fresh candidate is apply-divergent-fenced and no reconciliation path exists), remains UNFIXED and is now the sole open part of this bug -- it needs its own design discussion (forcing promotion of a divergent node, or truncating its uncommitted-but-locally-applied entries, both have real correctness implications), not a quick patch. With layer (1) fixed, this deadlock should no longer be reachable via the duplicate-submit path specifically, but the underlying design gap in raft-apply-decoupling's promotion-refusal logic is independent and could still be triggered by any other cause of a genuine, permanent APPLY_DIVERGENT fence on the only fresh-enough node.", "tags": [ "ha", "election", @@ -12650,7 +12650,8 @@ "chaotic-failover", "liveness", "wal-duplicate-submit", - "unresolved" + "partially-fixed", + "liveness-gap-open" ], "related_bugs": [ "bug-1417", @@ -12658,7 +12659,7 @@ "bug-845", "bug-1418" ], - "occurrences": 1, - "last_seen": "2026-07-07T14:00:00.000Z" + "occurrences": 2, + "last_seen": "2026-07-07T14:30:00.000Z" } -] \ No newline at end of file +] diff --git a/src/controller/include/cluster.h b/src/controller/include/cluster.h index 0b3f780..ad82f09 100644 --- a/src/controller/include/cluster.h +++ b/src/controller/include/cluster.h @@ -73,6 +73,15 @@ extern int handle_join(cluster_t*, endpoint_t*, xce_request_t*); extern int handle_reply(endpoint_t*, struct Request*, cluster_t*); extern int handle_jobdispatch(cluster_t*, endpoint_t*, xce_request_t*); extern int handle_jobsubmit(cluster_t*, endpoint_t*, xce_request_t*); +#ifdef XCE_TESTING +/* bug-1419: clears job.c's process-lifetime in-flight-jobid registry. + * Tests that deliberately leave a submission's commit continuation + * unresolved through teardown (no ack, no timeout, no truncation) have no + * other event to release that registry entry -- call this from fixture + * teardown so it doesn't outlive the test and spuriously reject an + * unrelated later test reusing the same jobid range. */ +extern void inflight_submits_reset_for_test(void); +#endif extern int handle_worker_leave(cluster_t*, int); extern int mark_worker_unavail(cluster_t*, worker_t*); extern int mark_worker_back(cluster_t*, worker_t*); diff --git a/src/controller/job.c b/src/controller/job.c index a2cb0ee..c718f26 100644 --- a/src/controller/job.c +++ b/src/controller/job.c @@ -51,6 +51,135 @@ static job_t* create_job(cluster_t* c, xce_job_t* j) return job; } +/* bug-1419: in-flight submission registry, closing the resubmit-dedup gap + * task 3.1's apply-after-commit deferral opened. + * + * Before task 3.1, jobid_range_exists()'s c->jobmgr check was airtight + * against a client's own retried submit (xcecli_jobsubmit wrapped in + * XCECLI_FAILOVER_CALL, client_failover.c) because job construction ran + * synchronously in this same request: by the time a retry could possibly + * reach the controller again, the first attempt's job already existed in + * c->jobmgr. client_failover.c's own doc comment names this "the + * controller's client-jobid replay guard" and relies on it to reclassify a + * retried write's EEXIST as success (xce_failover_classify's + * "Idempotent-replay rule"). + * + * Task 3.1 deliberately deferred job construction to AFTER commit (Req 2.1, + * 2.3) — correct for its own goal (no compensating rollback needed on a + * failed commit) — but this reopened exactly the window the replay guard + * depended on: a submit that is durably appended and will commit, but has + * not yet run through apply_committed_entry() and landed in c->jobmgr, is + * invisible to jobid_range_exists(). A client retry (e.g. after a + * transport error or failover mid-flight, before it ever saw the first + * attempt's reply) landing in that window sails through the check and + * appends a SECOND WAL_JOB_SUBMIT for the same jobid with a fresh subtime — + * confirmed via waldump on a live chaotic-soak node (non-byte-identical + * duplicate submits, same jobid, differing subtime/CRC), which can + * ultimately trip persist_recover()'s post-replay apply-correctness check + * (task 5.1) into a genuine APPLY_DIVERGENT verdict on restart. + * + * Fix: track jobids that have been accepted for WAL append but have not + * yet had their outcome fully resolved (failed, or applied) in a small + * process-local registry, and treat a hit there identically to a + * c->jobmgr hit — both mean "this exact write already happened or is + * already in flight," which is precisely what a retried write must see as + * a duplicate. Populated in handle_jobsubmit() right after this check + * passes (before any WAL work); cleared in jobsubmit_async_ctx_release() + * (the single choke point already reached on every resolution path: + * commit failure, apply completion, and OOM/alloc-failure early-outs). */ +typedef struct inflight_submit { + uint64_t base_jobid; + uint64_t array_size; + struct inflight_submit* next; +} inflight_submit_t; + +static pthread_mutex_t s_inflight_submits_lock = PTHREAD_MUTEX_INITIALIZER; +static inflight_submit_t* s_inflight_submits = NULL; + +static bool inflight_range_overlaps_locked(uint64_t base_jobid, + uint64_t array_size) +{ + for (inflight_submit_t* n = s_inflight_submits; n != NULL; n = n->next) { + uint64_t lo = base_jobid, hi = base_jobid + array_size; + uint64_t nlo = n->base_jobid, nhi = n->base_jobid + n->array_size; + if (lo < nhi && nlo < hi) + return true; + } + return false; +} + +/* Atomically check-then-register under one lock hold, closing the TOCTOU a + * separate check + separate add would leave between two concurrent + * handle_jobsubmit calls for the same jobid (a retry's new connection can + * race the still-in-flight original request under this codebase's + * vtask-dispatched, not single-threaded-per-jobid, request handling). + * Returns true (registered) if no overlap existed; false (already in + * flight, NOT registered) if one did — the caller must reject on false + * exactly as it already does for a c->jobmgr hit. */ +static bool inflight_range_try_add(uint64_t base_jobid, uint64_t array_size) +{ + bool ok = false; + + pthread_mutex_lock(&s_inflight_submits_lock); + if (!inflight_range_overlaps_locked(base_jobid, array_size)) { + inflight_submit_t* n = calloc(1, sizeof(*n)); + if (n != NULL) { + n->base_jobid = base_jobid; + n->array_size = array_size; + n->next = s_inflight_submits; + s_inflight_submits = n; + ok = true; + } + /* calloc failure: fall through with ok=false -- rejecting an + * accepted-looking submit on OOM is safe (the client's own retry + * logic already handles a rejected attempt); silently admitting an + * untracked in-flight submission on OOM is not. */ + } + pthread_mutex_unlock(&s_inflight_submits_lock); + return ok; +} + +static void inflight_range_remove(uint64_t base_jobid, uint64_t array_size) +{ + pthread_mutex_lock(&s_inflight_submits_lock); + inflight_submit_t** pp = &s_inflight_submits; + while (*pp != NULL) { + if ((*pp)->base_jobid == base_jobid && (*pp)->array_size == array_size) { + inflight_submit_t* dead = *pp; + *pp = dead->next; + free(dead); + break; + } + pp = &(*pp)->next; + } + pthread_mutex_unlock(&s_inflight_submits_lock); +} + +#ifdef XCE_TESTING +/* Test-only seam: s_inflight_submits is process-lifetime, not tied to any + * cluster_t fixture. In production that's fine -- a restart wipes it along + * with every other in-memory registry, and persist_recover() rebuilds + * c->jobmgr fresh from the WAL. But a unit-test binary reuses the same + * process across many fixtures, and a test that deliberately leaves a + * submission's commit continuation unresolved through teardown (bug-1419's + * own regression test) has no other event left to release its entry -- + * commit_engine_fini() frees pending continuations without invoking their + * callback. Without this, that entry outlives the fixture and can spuriously + * reject an unrelated later test that reuses the same jobid range. */ +void inflight_submits_reset_for_test(void) +{ + pthread_mutex_lock(&s_inflight_submits_lock); + inflight_submit_t* n = s_inflight_submits; + while (n != NULL) { + inflight_submit_t* dead = n; + n = n->next; + free(dead); + } + s_inflight_submits = NULL; + pthread_mutex_unlock(&s_inflight_submits_lock); +} +#endif + /* True if any jobid in [base_jobid, base_jobid + array_size) already exists * in c->jobmgr (raft-apply-decoupling task 3.1). Checked BEFORE any WAL work * so a colliding submission is still rejected immediately with no side @@ -58,7 +187,12 @@ static job_t* create_job(cluster_t* c, xce_job_t* j) * guard used to provide when job construction ran before the commit * barrier — now that construction is deferred to after commit, a collision * detected only post-commit could no longer be cleanly reported as "never - * happened". */ + * happened". + * + * bug-1419: this check alone is no longer sufficient (see + * inflight_range_try_add's doc comment) — handle_jobsubmit additionally + * calls inflight_range_try_add() after this check passes, which atomically + * closes the remaining not-yet-applied window this function cannot see. */ static bool jobid_range_exists(cluster_t* c, uint64_t base_jobid, uint64_t array_size) { @@ -628,6 +762,10 @@ typedef struct { * matches pre-3.1 semantics (git history commit * 6f1fa97), which always reported the BASE id, * never a per-element id */ + uint64_t array_size; /* bug-1419: paired with jobid to remove the + * matching inflight_range_add() entry on + * release -- see jobid_range_exists' doc + * comment for the full contract */ pthread_mutex_t lock; /* guards pending/replied below */ int pending; /* entries not yet FULLY resolved (see above) */ int replied; /* 1 once a reply has been sent for this batch */ @@ -635,6 +773,13 @@ typedef struct { static void jobsubmit_async_ctx_release(jobsubmit_async_ctx_t* actx) { + /* bug-1419: this is the single choke point every resolution path + * (commit failure, apply completion, and the OOM/alloc-failure + * early-outs in handle_jobsubmit below) already reaches -- clearing + * the in-flight registration here means a retry is only ever + * dedup-blocked while this exact submission's outcome is genuinely + * still unresolved, never after. */ + inflight_range_remove(actx->jobid, actx->array_size); endpoint_unref(actx->ep); pthread_mutex_destroy(&actx->lock); free(actx); @@ -816,6 +961,25 @@ int handle_jobsubmit(cluster_t* c, endpoint_t* ep, xce_request_t* request) return -errno; } + /* bug-1419: atomically claim this jobid range as in-flight, closing the + * window jobid_range_exists' c->jobmgr check alone cannot see (see its + * doc comment). A false return means another submission (a genuine + * concurrent collision, or -- the actual motivating case -- this exact + * request's own retried resend racing its still-in-flight original + * attempt) already holds this range; reject exactly as the + * jobid_range_exists branch above does. Released in + * jobsubmit_async_ctx_release() once this submission's outcome is + * fully resolved; the two error returns below (which occur BEFORE + * actx exists to carry that release) remove it directly. */ + if (!inflight_range_try_add(jobid, submit->array_size)) { + errno = EEXIST; + reply.status = -errno; + reply.data_which = RequestReply_data_nodata; + + xce_reply(ep, c->myhost, &reply); + return -errno; + } + /* Build only the WAL payload data for each array element — no job * object is constructed or inserted into c->jobmgr until the commit * below succeeds (raft-apply-decoupling task 3.1 / Req 2.1, 2.3): a @@ -830,6 +994,7 @@ int handle_jobsubmit(cluster_t* c, endpoint_t* ep, xce_request_t* request) } if (rc < 0) { free_job_submit_payloads(&plist); + inflight_range_remove(jobid, submit->array_size); reply.status = rc; reply.data_which = RequestReply_data_nodata; @@ -868,6 +1033,7 @@ int handle_jobsubmit(cluster_t* c, endpoint_t* ep, xce_request_t* request) if (actx == NULL) { free_job_submit_payloads(&plist); free(lsns); + inflight_range_remove(jobid, submit->array_size); reply.status = -ENOMEM; reply.data_which = RequestReply_data_nodata; @@ -875,13 +1041,14 @@ int handle_jobsubmit(cluster_t* c, endpoint_t* ep, xce_request_t* request) xce_reply(ep, c->myhost, &reply); return -ENOMEM; } - actx->c = c; - actx->ep = ep; - actx->seq = request->seq; - actx->opcode = request->opcode; - actx->jobid = jobid; - actx->pending = plist.count; - actx->replied = 0; + actx->c = c; + actx->ep = ep; + actx->seq = request->seq; + actx->opcode = request->opcode; + actx->jobid = jobid; + actx->array_size = submit->array_size; + actx->pending = plist.count; + actx->replied = 0; pthread_mutex_init(&actx->lock, NULL); endpoint_ref(ep); diff --git a/src/controller/testsuite/test_commit_barrier_reply.c b/src/controller/testsuite/test_commit_barrier_reply.c index 754fe31..52f7292 100644 --- a/src/controller/testsuite/test_commit_barrier_reply.c +++ b/src/controller/testsuite/test_commit_barrier_reply.c @@ -431,6 +431,12 @@ static void cbrp_teardown(void) cbrp_scan_replies(&g_req_ep, &drain); /* free any leftover packets */ repl_broadcast_send_hook = NULL; + /* bug-1419: commit_engine_fini frees any still-pending continuation + * without invoking its callback, so a test that deliberately leaves a + * submission unresolved (no ack/timeout/truncation) leaks its jobid + * range in job.c's process-lifetime in-flight registry. Clear it here + * so it never outlives this fixture. */ + inflight_submits_reset_for_test(); commit_engine_fini(&g_hce); persist_stop(&g_hc.persist); persist_fini(&g_hc.persist); @@ -1224,6 +1230,81 @@ static void test_jobsubmit_repeated_timeout_failures_no_phantom_jobs(void) printf("test_jobsubmit_repeated_timeout_failures_no_phantom_jobs: PASS\n"); } +/* bug-1419: a client retry that resends the SAME jobid before it has seen + * the first attempt's outcome must be rejected as a duplicate, not append + * a second WAL_JOB_SUBMIT entry. + * + * Root cause this proves fixed: jobid_range_exists()'s c->jobmgr check is + * airtight against a genuinely completed prior submission (the job is + * there), but raft-apply-decoupling task 3.1 deliberately defers job + * construction until AFTER commit -- so a submission that is durably + * appended and awaiting its commit outcome does NOT yet exist in + * c->jobmgr, and jobid_range_exists() alone cannot see it. libxce's + * client-side failover retry wrapper (XCECLI_FAILOVER_CALL, + * client_failover.c) resends the identical jobid on a transport error or + * failover-mid-flight, exactly the shape this test drives directly against + * the handler. + * + * Withholds ACKs (mirrors cbrp_drive_withheld) so the first call's commit + * stays genuinely unresolved when the second call arrives -- the real + * race window, not a timing approximation. */ +static void test_jobsubmit_resubmit_before_resolution_rejected_no_dup_wal_entry(void) +{ + const uint64_t jobid = 9139; + uint64_t lsn_before, lsn_before_second, watermark_first, watermark_second; + int rc_second; + + cbrp_setup(2000); /* generous: must never fire -- this test resolves + * via explicit teardown, not the timeout sweep */ + + cbrp_arrange_jobsubmit(jobid, 1); + + pthread_mutex_lock(&g_hc.persist.lock); + lsn_before = g_hc.persist.current_lsn; + pthread_mutex_unlock(&g_hc.persist.lock); + + /* First attempt: durably appends and registers its commit continuation, + * then returns immediately (task 3.2 -- handle_jobsubmit never blocks). + * No ACK is ever delivered, so its outcome stays unresolved for the + * rest of this test. */ + g_op_arg.fn = handle_jobsubmit; + g_op_arg.req = &g_jobsub_req; + g_op_arg.rc = -9999; + TEST_ASSERT_EQUAL_INT(0, pthread_create(&g_op_tid, NULL, + cbrp_op_thread_fn, &g_op_arg)); + TEST_ASSERT_EQUAL_INT(0, pthread_join(g_op_tid, NULL)); + + watermark_first = cbrp_poll_append(&g_hc.persist, lsn_before, 1000); + TEST_ASSERT_TRUE_MESSAGE(watermark_first > 0, + "the first attempt must append its WAL entry"); + + /* Second attempt: the retry, same jobid, while the first is still + * in flight (no ACK delivered, no timeout elapsed). Called directly -- + * handle_jobsubmit never blocks, so no second thread is needed. */ + cbrp_arrange_jobsubmit(jobid, 1); /* fresh xce_request_t (same seq/jobid) */ + + pthread_mutex_lock(&g_hc.persist.lock); + lsn_before_second = g_hc.persist.current_lsn; + pthread_mutex_unlock(&g_hc.persist.lock); + + rc_second = handle_jobsubmit(&g_hc, &g_req_ep, &g_jobsub_req); + + watermark_second = cbrp_poll_append(&g_hc.persist, lsn_before_second, 300); + + cbrp_teardown(); + + TEST_ASSERT_EQUAL_INT_MESSAGE(-EEXIST, rc_second, + "bug-1419: a resubmit for a jobid still in flight (durably " + "appended, commit outcome unresolved) must be rejected -EEXIST, " + "identically to a resubmit for a jobid already visible in " + "c->jobmgr"); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, (int)watermark_second, + "bug-1419: a rejected resubmit must NOT append a second " + "WAL_JOB_SUBMIT entry for the same jobid"); + + printf("test_jobsubmit_resubmit_before_resolution_rejected_no_dup_wal_entry: PASS\n"); +} + /* The explicit-FAILED (non-timeout) sibling of the repeated-timeout test * above: repeats the truncation-forced FAILED path * CBRP_JOBSUBMIT_REPEAT_N times in the same fixture, proving the @@ -3188,6 +3269,7 @@ void test_commit_barrier_reply(void) * FAILED failure-path coverage beyond the single-timeout tests above */ RUN_TEST(test_jobsubmit_truncation_forces_failed_no_phantom_job); RUN_TEST(test_jobsubmit_repeated_timeout_failures_no_phantom_jobs); + RUN_TEST(test_jobsubmit_resubmit_before_resolution_rejected_no_dup_wal_entry); RUN_TEST(test_jobsubmit_repeated_truncation_forced_failures_no_phantom_jobs); RUN_TEST(test_jobsubmit_array_mixed_outcome_no_phantom_job_for_failed_elements); /* handle_jobsubmit success path via the ordered apply pump