lei.chen/myxce
1
0
Fork 1
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(raft-apply-decoupling): bug-1419 real fix — replay tally, not dedup-guard

Browse source 
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).
This commit is contained in:
Rongsong Shen 2026-07-08 12:48:44 +00:00
parent f28ce57993
commit 11244dd654
7 changed files with 197 additions and 232 deletions
Download patch file Download diff file Expand all files Collapse all files

83
src/controller/persist.c
View file

@ -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 */