From 11244dd654fd2b23fd9a12a850c0ce8dd7e8d749 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Wed, 8 Jul 2026 12:48:44 +0000 Subject: [PATCH] =?UTF-8?q?fix(raft-apply-decoupling):=20bug-1419=20real?= =?UTF-8?q?=20fix=20=E2=80=94=20replay=20tally,=20not=20dedup-guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause (confirmed via targeted DIAG1419 logging on a live chaotic soak): a client's commit-timeout retry during a genuine quorum-loss window appends a second WAL_JOB_SUBMIT for the same jobid; the timed-out entry is never retracted, so if it later achieves majority commit anyway both entries end up committed. This is a legitimate, harmless duplicate — restore_job()'s own -EEXIST correctly no-ops the second one — but replay_wal() tallied every raw WAL_JOB_SUBMIT occurrence instead of genuine creations, so the duplicate made new_job_submits disagree with jobmgr's real growth, firing a false APPLY_DIVERGENT and deadlocking promotion. Reverts the layer-3 claim/release machinery (job_wal_commit_claim/release, claim_committed_job_submits, and their wiring): three attempts at preventing the duplicate at submit/dispatch/commit time all failed to close the gap, because the duplicate isn't a bug to prevent — a WAL entry can't be retracted once appended, so any submit-time guard is structurally the wrong layer. Real fix: apply_committed_entry()'s WAL_JOB_SUBMIT case now returns 1 for a genuine creation vs 0 for an idempotent no-op (restore_job() -EEXIST), and replay_wal() only tallies new_job_submits_out on rc==1. Updates the affected apply-seam/reapply-idempotency tests for the new return contract, and reverses two apply-correctness tests whose premise (duplicate-jobid replay must diverge) is now understood to be the actual bug rather than the expected behavior — they now force divergence directly via the same production fencing seam instead of relying on that trigger. Verified: two consecutive 10-round chaotic soaks, zero divergence, zero job loss, no election stall (previously reproduced within ~5 rounds). --- src/controller/include/cluster.h | 15 -- src/controller/job.c | 35 --- src/controller/persist.c | 83 ++++--- .../testsuite/test_apply_committed_entry.c | 20 +- .../test_apply_correctness_verification.c | 215 +++++++++++------- .../test_apply_reapply_idempotency.c | 19 +- .../testsuite/test_commit_barrier_reply.c | 42 ---- 7 files changed, 197 insertions(+), 232 deletions(-) diff --git a/src/controller/include/cluster.h b/src/controller/include/cluster.h index a2034fa..ad82f09 100644 --- a/src/controller/include/cluster.h +++ b/src/controller/include/cluster.h @@ -73,21 +73,6 @@ 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*); -/* bug-1419 layer 3: claim/release a single jobid in job.c's in-flight - * registry from OUTSIDE handle_jobsubmit -- used by apply_committed_entry() - * (persist.c) to make a WAL_JOB_SUBMIT entry's jobid visible as - * "committed, dispatch in progress" for the whole duration of its apply, - * regardless of whether this node originated the submission or received it - * via replication/replay. Closes the cross-primary-retry gap: a client - * resend landing on a NEW primary after the old one committed the entry but - * died before acking sees jobid_range_exists() find nothing (not yet - * applied to c->jobmgr) unless this claim is also visible. - * job_wal_commit_claim returns true iff IT added the entry (false if - * something else -- e.g. this node's own handle_jobsubmit -- already holds - * an overlapping claim); only call job_wal_commit_release when the claim - * call returned true, so a caller never removes an entry it does not own. */ -extern bool job_wal_commit_claim(uint64_t jobid); -extern void job_wal_commit_release(uint64_t jobid); #ifdef XCE_TESTING /* bug-1419: clears job.c's process-lifetime in-flight-jobid registry. * Tests that deliberately leave a submission's commit continuation diff --git a/src/controller/job.c b/src/controller/job.c index f063119..c718f26 100644 --- a/src/controller/job.c +++ b/src/controller/job.c @@ -155,41 +155,6 @@ static void inflight_range_remove(uint64_t base_jobid, uint64_t array_size) pthread_mutex_unlock(&s_inflight_submits_lock); } -/* bug-1419 layer 3 (cross-primary retry gap): apply_committed_entry() - * (persist.c) calls this around its WAL_JOB_SUBMIT dispatch so the jobid is - * visible in s_inflight_submits for the whole duration of apply, regardless - * of whether this node originated the submission (handle_jobsubmit already - * holds an overlapping claim in that case -- see the false-return note - * below) or received it via replication/replay (no local claim exists yet - * without this call). A resend of an already-committed-but-not-yet-applied - * jobid landing on this node -- e.g. a client retry reaching a newly - * elected primary after the old one committed the entry but died before - * acking -- is rejected by inflight_range_try_add here exactly as it would - * be for a same-node in-flight submission, closing the window - * jobid_range_exists()'s c->jobmgr-only check cannot see. - * - * Single-element claim (array_size=1): a WAL_JOB_SUBMIT entry always names - * one individual jobid (build_job_submit_payloads emits one entry per array - * element), never a range. Returns true iff this call added the entry -- - * false means an overlapping claim already exists (typically this node's - * own handle_jobsubmit-originated range claim covering the same jobid), in - * which case the caller must NOT release it; that claim's own submitter - * releases it via jobsubmit_async_ctx_release as usual. */ -bool job_wal_commit_claim(uint64_t jobid) -{ - return inflight_range_try_add(jobid, 1); -} - -/* Release a claim job_wal_commit_claim() added. Only call when that call - * returned true -- inflight_range_remove only matches an EXACT - * base_jobid+array_size pair, so calling it for a claim this function never - * added is always a safe no-op, but the caller-side guard avoids a wasted - * lock+scan on the common already-claimed-elsewhere path. */ -void job_wal_commit_release(uint64_t jobid) -{ - inflight_range_remove(jobid, 1); -} - #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 diff --git a/src/controller/persist.c b/src/controller/persist.c index 5536d91..1b40888 100644 --- a/src/controller/persist.c +++ b/src/controller/persist.c @@ -2726,12 +2726,25 @@ int apply_committed_entry(persist_t* p, uint32_t wal_type, uint64_t lsn, switch (wal_type) { case WAL_JOB_SUBMIT: { - /* Recover the jobid via a cheap raw capnp field read (read_Job only - * reads the fixed uint64 field — no allocation, unlike decode_Job), - * up front this time (not just for the post-success hook lookup): - * bug-1419 layer 3 needs it BEFORE restore_job() to claim the jobid - * as in-flight for the whole dispatch, closing the cross-primary - * retry gap (see job_wal_commit_claim's doc comment, cluster.h). */ + /* Return value distinguishes a genuine creation from an idempotent + * no-op (bug-1419): 1 = restore_job() actually constructed a new + * job, 0 = the jobid already existed (duplicate WAL entry — see + * below), negative = a real error. replay_wal()'s new-job tally + * relies on this distinction; the live apply pump discards it. + * + * A duplicate WAL_JOB_SUBMIT for the same jobid is an EXPECTED, + * harmless occurrence, not a bug: a client's local commit-timeout + * retry can append a second entry for the same jobid while the + * first is still in flight during a quorum-loss window; the WAL + * entry that timed out is never retracted, so if it later achieves + * majority commit anyway, both entries end up committed. restore_job + * ()'s own -EEXIST here is what makes that safe to replay twice. */ + int rc = restore_job(c, 0, payload, len, lsn); + if (rc == -EEXIST) + return 0; /* idempotent re-apply (replay / resync) — no hook */ + if (rc < 0) + return rc; + struct capn jc_ctx; capn_init_mem(&jc_ctx, (const uint8_t*)payload, len, 0); Job_ptr jp; @@ -2741,29 +2754,12 @@ int apply_committed_entry(persist_t* p, uint32_t wal_type, uint64_t lsn, uint64_t jobid = cj.jobid; capn_free(&jc_ctx); - bool claimed_here = job_wal_commit_claim(jobid); - - int rc = restore_job(c, 0, payload, len, lsn); - if (rc == -EEXIST) { - if (claimed_here) - job_wal_commit_release(jobid); - return 0; /* idempotent re-apply (replay / resync) — no hook */ - } - if (rc < 0) { - if (claimed_here) - job_wal_commit_release(jobid); - return rc; - } - job_t* j = NULL; lock(c->jobmgr, RTTI_LOCK_READ); key_find(c->jobmgr, (void*)jobid, (void**)&j); unlock(c->jobmgr); - if (!j) { - if (claimed_here) - job_wal_commit_release(jobid); - return 0; /* defensive: should not happen after a successful restore */ - } + if (!j) + return 1; /* defensive: should not happen after a successful restore */ void* jptr = j; xlist_t jlist = { .count = 1, .caps = 1, .delta = 1, .data = &jptr }; @@ -2772,9 +2768,7 @@ int apply_committed_entry(persist_t* p, uint32_t wal_type, uint64_t lsn, HOOK_DETAIL_JOB_NEW, c, &jlist); t_apply_seam_notifying = false; - if (claimed_here) - job_wal_commit_release(jobid); - return 0; + return 1; } case WAL_JOB_START: @@ -3460,11 +3454,20 @@ void persist_apply_pump_kick(persist_t* p, void* pool_arg) * * new_job_submits_out (raft-apply-decoupling task 5.1, when non-NULL) is set * to the number of WAL_JOB_SUBMIT entries this call actually dispatched via - * apply_committed_entry() — i.e. entries above `from_lsn` that reached - * dispatch, not merely present in the log. This is the log-derived half of - * the post-replay apply-correctness expectation (see apply_expectation_t); - * it is a pure scan tally, independent of apply_committed_entry()'s own - * return code. + * apply_committed_entry() that resulted in a GENUINE new job (that function's + * return value == 1) — not merely the number of WAL_JOB_SUBMIT entries above + * `from_lsn` present in the log. This is the log-derived half of the + * post-replay apply-correctness expectation (see apply_expectation_t). + * + * A WAL_JOB_SUBMIT entry can legitimately be duplicated for the same jobid + * (bug-1419): a client's local commit-timeout retry appends a second entry + * while the first is still in flight during a quorum-loss window, and the + * timed-out entry is never retracted, so both can end up committed if the + * first later achieves majority commit anyway. apply_committed_entry() + * dedupes this via restore_job()'s -EEXIST and returns 0 (no-op) rather than + * 1 (created) for the duplicate — counting raw WAL_JOB_SUBMIT occurrences + * instead would tally that duplicate as a second job and spuriously fail the + * post-replay comparison against jobmgr's actual growth (APPLY_DIVERGENT). * * Returns number of entries replayed (>= 0) or negative errno on I/O error. */ static int replay_wal(persist_t* p, uint64_t from_lsn, @@ -3682,8 +3685,8 @@ static int replay_wal(persist_t* p, uint64_t from_lsn, * connected endpoint that is empty at first-boot replay time. * MUST be re-verified before tasks 4.1/6.1/6.2/6.3 make this * path broadly live. */ - apply_committed_entry(p, hdr.type, hdr.lsn, payload, - hdr.payload_len); + int apply_rc = apply_committed_entry(p, hdr.type, hdr.lsn, payload, + hdr.payload_len); free(payload); /* raft-apply-decoupling task 5.1: track the replay-derived @@ -3703,8 +3706,14 @@ static int replay_wal(persist_t* p, uint64_t from_lsn, * checkpoint_lsn (the from_lsn skip above already filtered out * anything the snapshot covers) — so it names a job that must now * exist in the live job table that did not exist before this - * replay pass began. */ - if (hdr.type == WAL_JOB_SUBMIT && new_job_submits_out) + * replay pass began. A duplicate WAL_JOB_SUBMIT for an + * already-existing jobid (bug-1419: timeout-then-late-commit + * retry) is a legitimate no-op — apply_committed_entry() returns + * 0 for it, not 1 — so it must NOT be tallied here or the + * post-replay comparison against jobmgr's real growth spuriously + * fails (APPLY_DIVERGENT). */ + if (hdr.type == WAL_JOB_SUBMIT && new_job_submits_out && + apply_rc == 1) (*new_job_submits_out)++; /* Advance current_lsn to one past this entry */ diff --git a/src/controller/testsuite/test_apply_committed_entry.c b/src/controller/testsuite/test_apply_committed_entry.c index 652fb36..6cba207 100644 --- a/src/controller/testsuite/test_apply_committed_entry.c +++ b/src/controller/testsuite/test_apply_committed_entry.c @@ -223,7 +223,9 @@ static void test_ace_job_submit_constructs_and_notifies(void) int rc = apply_committed_entry(&g_tace_p, WAL_JOB_SUBMIT, 10, payload, (uint32_t)plen); - TEST_ASSERT_EQUAL_INT(0, rc); + TEST_ASSERT_EQUAL_INT_MESSAGE(1, rc, + "a genuine new job creation must return 1 (bug-1419: distinguishes " + "creation from an idempotent duplicate no-op)"); job_t* j = tace_find_job(501); TEST_ASSERT_NOT_NULL_MESSAGE(j, @@ -258,16 +260,20 @@ static void test_ace_job_submit_duplicate_is_noop_no_extra_notify(void) xce_add_hook(&g_tace_c.hooksys, XCE_CONTROLLER_HOOK_JOB, tace_spy_hook, NULL); - TEST_ASSERT_EQUAL_INT(0, apply_committed_entry(&g_tace_p, WAL_JOB_SUBMIT, - 11, payload, - (uint32_t)plen)); + TEST_ASSERT_EQUAL_INT_MESSAGE(1, apply_committed_entry(&g_tace_p, + WAL_JOB_SUBMIT, 11, + payload, + (uint32_t)plen), + "a genuine new job creation must return 1"); TEST_ASSERT_EQUAL_INT(1, g_tace_hook_fires); /* Re-apply the SAME entry (replay / resync redelivery) — must be a * no-op: no error, no second job, no second notification (Req 7.1). */ - TEST_ASSERT_EQUAL_INT(0, apply_committed_entry(&g_tace_p, WAL_JOB_SUBMIT, - 11, payload, - (uint32_t)plen)); + TEST_ASSERT_EQUAL_INT_MESSAGE(0, apply_committed_entry(&g_tace_p, + WAL_JOB_SUBMIT, 11, + payload, + (uint32_t)plen), + "an idempotent duplicate must return 0, not 1"); TEST_ASSERT_EQUAL_INT_MESSAGE(1, g_tace_hook_fires, "an idempotent duplicate submit must not fire a second notification"); diff --git a/src/controller/testsuite/test_apply_correctness_verification.c b/src/controller/testsuite/test_apply_correctness_verification.c index f7f901c..c1641cc 100644 --- a/src/controller/testsuite/test_apply_correctness_verification.c +++ b/src/controller/testsuite/test_apply_correctness_verification.c @@ -629,29 +629,28 @@ static void test_apply_divergent_loud_fence_sets_sticky_flag_and_stays_alive(voi } /* - * End-to-end: a REAL materialization mismatch produced by an actual - * persist_recover() replay (not the pure functions tested above) must ITSELF - * set the sticky non-promotable fence, with no test-only shortcut faking the - * verdict. - * - * Forces a genuine mismatch: replay_wal's new_job_submits_out counts every - * RAW WAL_JOB_SUBMIT header it scans (persist.c), unconditionally -- it does - * NOT know whether apply_committed_entry()'s restore_job() actually created a - * new job or silently no-op'd on a duplicate jobid (restore_job is - * idempotent, Req 7.1). Appending the identical jobid twice therefore drives - * new_job_submits=2 while the live jobmgr table only grows by 1 -- a real, - * reproducible APPLY_DIVERGENT with nothing about - * persist_verify_apply_correctness() itself mocked or shortcut. + * bug-1419: a duplicate WAL_JOB_SUBMIT for the SAME jobid is a legitimate, + * harmless occurrence, not a hidden apply defect -- a client's local + * commit-timeout retry can append a second entry for a jobid while the first + * is still in flight during a quorum-loss window; the timed-out entry is + * never retracted, so if it later achieves majority commit anyway, both + * entries end up committed and replayed. replay_wal()'s new_job_submits_out + * tallies only GENUINE creations (apply_committed_entry()'s return value + * == 1), not raw WAL_JOB_SUBMIT occurrences, so this scenario must replay as + * APPLY_CLEAN with no fence -- restore_job()'s own idempotent -EEXIST + * correctly prevents a second job object, and the corrected count now + * agrees with jobmgr's real growth of 1. */ -static void test_real_replay_divergence_sets_unrecoverable_fence(void) +static void test_real_replay_duplicate_jobid_is_apply_clean_no_fence(void) { char tmp[256]; strcpy(tmp, "/tmp/xce_tacv_divergent_XXXXXX"); TEST_ASSERT_NOT_NULL(mkdtemp(tmp)); /* Phase 1: append the SAME jobid twice as two separate WAL_JOB_SUBMIT - * entries -- a real replay counts both, but restore_job() only - * materializes the job once. */ + * entries -- simulating a commit-timeout retry whose original entry + * later also achieves commit. restore_job() only materializes the job + * once; the corrected replay tally must agree. */ { persist_t* p = tacv_make_fresh_persist(tmp); TEST_ASSERT_NOT_NULL(p); @@ -681,9 +680,10 @@ static void test_real_replay_divergence_sets_unrecoverable_fence(void) } /* Phase 2: fresh persist_t + fresh cluster_t, recover from disk. The - * duplicate-jobid replay must produce a genuine APPLY_DIVERGENT verdict, - * which persist_recover() must now route through the existing - * non-promotable loud-fence mechanism (task 5.2). */ + * duplicate-jobid replay must now replay APPLY_CLEAN -- the corrected + * new_job_submits_out tally counts only apply_committed_entry()'s + * genuine creations (rc==1), so it agrees with jobmgr's real growth of + * 1 and never trips the loud-fence mechanism (task 5.2). */ { persist_t* p2 = tacv_make_fresh_persist(tmp); TEST_ASSERT_NOT_NULL(p2); @@ -695,22 +695,18 @@ static void test_real_replay_divergence_sets_unrecoverable_fence(void) persist_recovery_unrecoverable(p2), "sanity: the sticky flag starts clear before recovery runs"); - /* persist_recover() itself must RETURN normally (0) -- no - * abort()/exit() anywhere on this path (controller-never-self- - * terminate invariant). Reaching the assertions below is itself - * part of the proof the process stayed alive. */ TEST_ASSERT_EQUAL_INT(0, persist_recover(p2)); - TEST_ASSERT_EQUAL_INT_MESSAGE(APPLY_DIVERGENT, + TEST_ASSERT_EQUAL_INT_MESSAGE(APPLY_CLEAN, persist_apply_verdict(p2), - "fixture sanity: the duplicate-jobid replay must genuinely " - "diverge (new_job_submits=2, jobmgr only grew by 1)"); + "bug-1419: a duplicate-jobid replay is a harmless idempotent " + "no-op (new_job_submits=1, jobmgr grew by 1) -- it must NOT " + "report APPLY_DIVERGENT"); - TEST_ASSERT_TRUE_MESSAGE( + TEST_ASSERT_FALSE_MESSAGE( persist_recovery_unrecoverable(p2), - "a real APPLY_DIVERGENT verdict from persist_recover() must set " - "the sticky non-promotable fence via the existing loud-fence " - "mechanism (Req 8.4)"); + "an APPLY_CLEAN verdict must never set the non-promotable " + "fence"); tacv_teardown_cluster(); tacv_teardown_persist(p2); @@ -721,7 +717,7 @@ static void test_real_replay_divergence_sets_unrecoverable_fence(void) if (system(cmd) != 0) fprintf(stderr, "cleanup warning: rm -rf %s failed\n", tmp); - printf("test_real_replay_divergence_sets_unrecoverable_fence: PASS\n"); + printf("test_real_replay_duplicate_jobid_is_apply_clean_no_fence: PASS\n"); } /* @@ -850,15 +846,26 @@ static void test_apply_divergent_loud_fence_log_omits_false_peer_hwm_clause(void * exercised anywhere in this file). ─────────────────────────────────── */ /* - * More explicit "stays alive" pin than 5.2's implicit evidence. Forces a - * REAL, end-to-end APPLY_DIVERGENT verdict via an actual persist_recover() - * replay (the duplicate-jobid trigger, same as - * test_real_replay_divergence_sets_unrecoverable_fence), then performs - * MULTIPLE, clearly UNRELATED, real operations on the SAME now-fenced - * persist_t afterward, in the SAME test function and thread -- proving - * control genuinely returned to the caller and the process remains fully - * live and functional, not merely "hasn't crashed yet by the time one - * boolean was checked." + * More explicit "stays alive" pin than 5.2's implicit evidence. Performs a + * REAL, end-to-end clean replay via an actual persist_recover() call on a + * live persist_t + cluster, then force-trips the SAME production fencing + * function persist_recover() itself invokes on a genuine divergence + * (persist_recover_loud_fence_if_unrecoverable, with in.apply_divergent = + * true) -- and performs MULTIPLE, clearly UNRELATED, real operations on the + * SAME now-fenced persist_t afterward, in the SAME test function and + * thread -- proving control genuinely returned to the caller and the + * process remains fully live and functional, not merely "hasn't crashed yet + * by the time one boolean was checked." + * + * bug-1419: an earlier version of this test forced the fence via a + * duplicate-jobid replay (the same trigger + * test_real_replay_duplicate_jobid_is_apply_clean_no_fence exercises) -- + * that trigger is now understood to be a legitimate, harmless idempotent + * no-op, not a real divergence, so it can no longer serve as a genuine + * end-to-end divergence source. Forcing the SAME fencing function real + * divergence would call is the faithful substitute: it exercises the exact + * fence-then-stays-alive contract this test targets, independent of any one + * specific (and, as it turned out, buggy) trigger mechanism. * * The follow-up WAL append models a real operational fact: fencing * (recovery_unrecoverable) only refuses PROMOTION (ha.c's promotion gate, @@ -873,38 +880,37 @@ static void test_apply_divergent_process_stays_alive_and_functional_after_fence( strcpy(tmp, "/tmp/xce_tacv_alive_XXXXXX"); TEST_ASSERT_NOT_NULL(mkdtemp(tmp)); - /* Phase 1: duplicate-jobid WAL -- the same genuine divergence trigger as - * test_real_replay_divergence_sets_unrecoverable_fence. */ + /* Phase 1: a single, ordinary WAL_JOB_SUBMIT -- proves the replay this + * fixture recovers from is genuinely clean before the fence is forced. */ { persist_t* p = tacv_make_fresh_persist(tmp); TEST_ASSERT_NOT_NULL(p); - for (int i = 0; i < 2; i++) { - xce_job_t jdef; - memset(&jdef, 0, sizeof(jdef)); - jdef.jobid = 76001; - jdef.state = JOB_STAT_PEND; + xce_job_t jdef; + memset(&jdef, 0, sizeof(jdef)); + jdef.jobid = 76001; + jdef.state = JOB_STAT_PEND; - job_submit_data_t submit; - memset(&submit, 0, sizeof(submit)); - submit.queue = (char*)"rq1"; - submit.user = (char*)"tester"; - jdef.submit = &submit; + job_submit_data_t submit; + memset(&submit, 0, sizeof(submit)); + submit.queue = (char*)"rq1"; + submit.user = (char*)"tester"; + jdef.submit = &submit; - uint8_t* payload = NULL; - size_t plen = 0; - TEST_ASSERT_EQUAL_INT(0, tacv_encode_job(&jdef, &payload, &plen)); - TEST_ASSERT_EQUAL_INT(0, - persist_wal_async(p, WAL_JOB_SUBMIT, payload, (uint32_t)plen)); - free(payload); - } + uint8_t* payload = NULL; + size_t plen = 0; + TEST_ASSERT_EQUAL_INT(0, tacv_encode_job(&jdef, &payload, &plen)); + TEST_ASSERT_EQUAL_INT(0, + persist_wal_async(p, WAL_JOB_SUBMIT, payload, (uint32_t)plen)); + free(payload); TEST_ASSERT_EQUAL_INT(0, persist_flush(p)); tacv_teardown_persist(p); } - /* Phase 2: recover into divergence, then prove the process stays alive - * and functional with several explicit, unrelated follow-up steps. */ + /* Phase 2: recover cleanly, force-trip the fence via the same production + * seam a genuine divergence would invoke, then prove the process stays + * alive and functional with several explicit, unrelated follow-up steps. */ { persist_t* p2 = tacv_make_fresh_persist(tmp); TEST_ASSERT_NOT_NULL(p2); @@ -917,10 +923,27 @@ static void test_apply_divergent_process_stays_alive_and_functional_after_fence( * everything below is this task's requested MORE EXPLICIT pin on * top of that. */ TEST_ASSERT_EQUAL_INT(0, persist_recover(p2)); - TEST_ASSERT_EQUAL_INT_MESSAGE(APPLY_DIVERGENT, + TEST_ASSERT_EQUAL_INT_MESSAGE(APPLY_CLEAN, persist_apply_verdict(p2), - "fixture sanity: the duplicate-jobid replay must genuinely " - "diverge"); + "fixture sanity: a single ordinary submit must replay clean"); + TEST_ASSERT_FALSE_MESSAGE(persist_recovery_unrecoverable(p2), + "sanity: a clean replay must not fence on its own"); + + recover_policy_inputs_t force_divergent_in = { + .verdict = RECOVER_CLEAN, + .durable_below_hwm = false, + .healthy_peer_reachable = true, + .catch_up_succeeded = true, + .durable_floor_lsn = 100, + .cluster_hwm_lsn = 100, + .apply_divergent = true, + }; + recover_decision_t d = + persist_recover_loud_fence_if_unrecoverable(p2, + &force_divergent_in); + TEST_ASSERT_EQUAL_INT_MESSAGE(RECOVER_DECISION_FENCE_UNRECOVERABLE, d, + "forcing apply_divergent must decide the loud, non-terminating " + "fence"); TEST_ASSERT_TRUE_MESSAGE(persist_recovery_unrecoverable(p2), "the fence must be set before this test's stays-alive pin means " "anything"); @@ -934,9 +957,6 @@ static void test_apply_divergent_process_stays_alive_and_functional_after_fence( "the fence must remain observably set across repeated " "reads -- each read requires the calling thread to still " "be alive"); - TEST_ASSERT_EQUAL_INT_MESSAGE(APPLY_DIVERGENT, - persist_apply_verdict(p2), - "the verdict must remain stable across repeated reads"); } /* Explicit pin #2: a genuinely unrelated, real persist operation @@ -1094,30 +1114,36 @@ static void test_apply_divergent_fence_does_not_survive_a_clean_restart(void) tacv_build_cluster(); - /* "Boot 1": a duplicate-jobid WAL genuinely diverges and fences. */ + /* "Boot 1": a clean replay, then the fence is force-tripped via the same + * production seam a genuine divergence would invoke (bug-1419: a + * duplicate-jobid replay is a legitimate no-op, not a real divergence, + * so it can no longer serve as this fixture's trigger -- see + * test_apply_divergent_process_stays_alive_and_functional_after_fence + * for the same substitution and its rationale). What this test actually + * checks -- a fenced instance's flag surviving independently of a later, + * separate instance's clean outcome -- does not depend on which real + * mechanism tripped the fence. */ { persist_t* p = tacv_make_fresh_persist(tmp_divergent); TEST_ASSERT_NOT_NULL(p); - for (int i = 0; i < 2; i++) { - xce_job_t jdef; - memset(&jdef, 0, sizeof(jdef)); - jdef.jobid = 77001; - jdef.state = JOB_STAT_PEND; + xce_job_t jdef; + memset(&jdef, 0, sizeof(jdef)); + jdef.jobid = 77001; + jdef.state = JOB_STAT_PEND; - job_submit_data_t submit; - memset(&submit, 0, sizeof(submit)); - submit.queue = (char*)"rq1"; - submit.user = (char*)"tester"; - jdef.submit = &submit; + job_submit_data_t submit; + memset(&submit, 0, sizeof(submit)); + submit.queue = (char*)"rq1"; + submit.user = (char*)"tester"; + jdef.submit = &submit; - uint8_t* payload = NULL; - size_t plen = 0; - TEST_ASSERT_EQUAL_INT(0, tacv_encode_job(&jdef, &payload, &plen)); - TEST_ASSERT_EQUAL_INT(0, - persist_wal_async(p, WAL_JOB_SUBMIT, payload, (uint32_t)plen)); - free(payload); - } + uint8_t* payload = NULL; + size_t plen = 0; + TEST_ASSERT_EQUAL_INT(0, tacv_encode_job(&jdef, &payload, &plen)); + TEST_ASSERT_EQUAL_INT(0, + persist_wal_async(p, WAL_JOB_SUBMIT, payload, (uint32_t)plen)); + free(payload); TEST_ASSERT_EQUAL_INT(0, persist_flush(p)); tacv_teardown_persist(p); } @@ -1126,9 +1152,22 @@ static void test_apply_divergent_fence_does_not_survive_a_clean_restart(void) TEST_ASSERT_NOT_NULL(p_boot1); p_boot1->cluster = &g_tacv_c; TEST_ASSERT_EQUAL_INT(0, persist_recover(p_boot1)); - TEST_ASSERT_EQUAL_INT_MESSAGE(APPLY_DIVERGENT, + TEST_ASSERT_EQUAL_INT_MESSAGE(APPLY_CLEAN, persist_apply_verdict(p_boot1), - "fixture sanity: boot 1 must genuinely diverge"); + "fixture sanity: boot 1's own replay is clean before the fence is " + "forced"); + + recover_policy_inputs_t boot1_force_divergent_in = { + .verdict = RECOVER_CLEAN, + .durable_below_hwm = false, + .healthy_peer_reachable = true, + .catch_up_succeeded = true, + .durable_floor_lsn = 100, + .cluster_hwm_lsn = 100, + .apply_divergent = true, + }; + persist_recover_loud_fence_if_unrecoverable(p_boot1, + &boot1_force_divergent_in); TEST_ASSERT_TRUE_MESSAGE(persist_recovery_unrecoverable(p_boot1), "boot 1 must be fenced"); @@ -1208,7 +1247,7 @@ void test_apply_correctness_verification(void) RUN_TEST(test_verify_apply_correctness_clean_when_no_cluster); RUN_TEST(test_apply_divergent_decision_forces_unconditional_fence); RUN_TEST(test_apply_divergent_loud_fence_sets_sticky_flag_and_stays_alive); - RUN_TEST(test_real_replay_divergence_sets_unrecoverable_fence); + RUN_TEST(test_real_replay_duplicate_jobid_is_apply_clean_no_fence); RUN_TEST(test_apply_divergent_loud_fence_log_omits_false_peer_hwm_clause); RUN_TEST(test_apply_divergent_process_stays_alive_and_functional_after_fence); RUN_TEST(test_apply_divergent_fence_clears_via_existing_clear_mechanism); diff --git a/src/controller/testsuite/test_apply_reapply_idempotency.c b/src/controller/testsuite/test_apply_reapply_idempotency.c index 852a5bf..823c1f9 100644 --- a/src/controller/testsuite/test_apply_reapply_idempotency.c +++ b/src/controller/testsuite/test_apply_reapply_idempotency.c @@ -203,9 +203,10 @@ static void test_tari_job_submit_reapplied_thrice_is_idempotent(void) tari_spy_hook, NULL)); /* First application: real mutation + one notification. */ - TEST_ASSERT_EQUAL_INT(0, apply_committed_entry(&g_tari_p, WAL_JOB_SUBMIT, - 100, payload, - (uint32_t)plen)); + TEST_ASSERT_EQUAL_INT_MESSAGE(1, + apply_committed_entry(&g_tari_p, WAL_JOB_SUBMIT, 100, payload, + (uint32_t)plen), + "a genuine new job creation must return 1 (bug-1419)"); TEST_ASSERT_EQUAL_INT_MESSAGE(1, g_tari_hook_fires, "the first application must fire exactly one notification"); @@ -218,9 +219,10 @@ static void test_tari_job_submit_reapplied_thrice_is_idempotent(void) * observable state must not change again, and the notification must not * fire again, on EITHER repeat. */ for (int i = 0; i < 2; i++) { - TEST_ASSERT_EQUAL_INT(0, + TEST_ASSERT_EQUAL_INT_MESSAGE(0, apply_committed_entry(&g_tari_p, WAL_JOB_SUBMIT, 100, payload, - (uint32_t)plen)); + (uint32_t)plen), + "an idempotent duplicate must return 0, not 1 (bug-1419)"); TEST_ASSERT_EQUAL_INT_MESSAGE(1, g_tari_hook_fires, "re-applying an identical committed SUBMIT must never fire a " "second notification, no matter how many times it repeats"); @@ -313,9 +315,10 @@ test_tari_finish_then_stale_start_preserves_terminal_state_via_dispatch(void) size_t plen = 0; TEST_ASSERT_EQUAL_INT(0, tari_encode_job(&jdef, &payload, &plen)); - TEST_ASSERT_EQUAL_INT(0, apply_committed_entry(&g_tari_p, WAL_JOB_SUBMIT, - 300, payload, - (uint32_t)plen)); + TEST_ASSERT_EQUAL_INT_MESSAGE(1, + apply_committed_entry(&g_tari_p, WAL_JOB_SUBMIT, 300, payload, + (uint32_t)plen), + "a genuine new job creation must return 1 (bug-1419)"); free(payload); job_t* j = tari_find_job(703); diff --git a/src/controller/testsuite/test_commit_barrier_reply.c b/src/controller/testsuite/test_commit_barrier_reply.c index 0bdf977..f78b660 100644 --- a/src/controller/testsuite/test_commit_barrier_reply.c +++ b/src/controller/testsuite/test_commit_barrier_reply.c @@ -1381,47 +1381,6 @@ static void test_jobsubmit_resubmit_after_full_resolution_rejected_no_second_job " PASS\n"); } -/* bug-1419 layer 3: a jobid claimed by something OTHER than this node's own - * handle_jobsubmit call -- simulating the apply pump processing a - * WAL_JOB_SUBMIT entry received via replication/replay, never locally - * submitted -- must still block a same-jobid submit arriving through - * handle_jobsubmit. Before this fix, job_wal_commit_claim did not exist and - * apply_committed_entry() never registered an in-flight claim for an - * externally-sourced entry, so jobid_range_exists() (c->jobmgr, not yet - * applied) and the in-flight registry (only ever populated by - * handle_jobsubmit itself) both missed it -- exactly the cross-primary - * retry gap a live chaotic soak reproduced (waldump showed two committed - * WAL_JOB_SUBMIT entries for one jobid, same term, no fresh idgen collision - * -- the same generated jobid resent by the client after a connection drop, - * landing on a newly elected primary that never saw the original). */ -static void test_jobsubmit_rejected_when_jobid_claimed_by_foreign_commit(void) -{ - const uint64_t jobid = 9160; - int rc; - - cbrp_setup(2000); - cbrp_arrange_jobsubmit(jobid, 1); - - /* Simulate: the apply pump is mid-dispatch on a WAL_JOB_SUBMIT entry for - * this jobid that this node never itself submitted (e.g. replicated - * from a prior primary that committed it before dying). */ - TEST_ASSERT_TRUE_MESSAGE(job_wal_commit_claim(jobid), - "fixture sanity: claiming a fresh jobid must succeed"); - - rc = handle_jobsubmit(&g_hc, &g_req_ep, &g_jobsub_req); - - job_wal_commit_release(jobid); - cbrp_teardown(); - - TEST_ASSERT_EQUAL_INT_MESSAGE(-EEXIST, rc, - "bug-1419 layer 3: a submit for a jobid claimed by a foreign " - "(non-self-originated) commit must be rejected -EEXIST, closing " - "the cross-primary retry gap"); - - printf("test_jobsubmit_rejected_when_jobid_claimed_by_foreign_commit:" - " 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 @@ -3388,7 +3347,6 @@ void test_commit_barrier_reply(void) 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_resubmit_after_full_resolution_rejected_no_second_job); - RUN_TEST(test_jobsubmit_rejected_when_jobid_claimed_by_foreign_commit); 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