wdgzyjx/myxce
1
0
Fork 0
forked from lei.chen/myxce
Code Pull requests Activity

fix(raft-apply-decoupling): bug-1419 close resubmit-dedup gap left by apply-after-commit deferral

Browse source 
jobid_range_exists() only checks c->jobmgr, but task 3.1 deliberately
defers job construction until after commit -- so a submission that is
durably WAL-appended and awaiting its commit outcome is invisible to
that check. A client-side retry (XCECLI_FAILOVER_CALL resending the
same jobid after a transport error or failover mid-flight) sails past
the guard and appends a second, non-identical WAL_JOB_SUBMIT entry for
the same jobid -- confirmed via waldump on a live chaotic-soak node
(duplicate submits, same jobid, differing subtime/CRC) -- which can
trip persist_recover()'s post-replay apply-correctness check into a
genuine APPLY_DIVERGENT verdict.

Adds a process-lifetime in-flight-jobid-range registry in job.c that
handle_jobsubmit atomically check-and-claims immediately after the
existing jobid_range_exists check, releasing from
jobsubmit_async_ctx_release (the single choke point already shared by
the commit-continuation, apply-complete, and OOM early-out paths). A
resubmit for a still-in-flight jobid now gets -EEXIST with no second
WAL append.

Adds a matching XCE_TESTING reset seam (declared in cluster.h) so a
test that deliberately leaves a submission's commit continuation
unresolved through teardown doesn't leak its jobid range into a later
test reusing the same range -- commit_engine_fini frees pending
continuations without invoking their callback.

Regression test added to test_commit_barrier_reply.c (RED->GREEN
confirmed). Full 78-group controller_unit_test sweep: 782 tests, 0
failures.

This fixes only the WAL duplicate-submit mechanism (layer 1 of
bug-1419). The deeper PreVote-freshness/apply-divergence liveness
deadlock (layer 2 -- no recovery path when the only fresh-enough node
is apply-divergent-fenced) remains open and needs its own design
discussion; buglog.json updated accordingly.
This commit is contained in:
Rongsong Shen 2026-07-07 14:28:53 +00:00
parent 9b173c9a5e
commit fe4352fb36
4 changed files with 272 additions and 13 deletions
Download patch file Download diff file Expand all files Collapse all files

9
src/controller/include/cluster.h
View file

@ -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*);

183
src/controller/job.c
View file

@ -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);

82
src/controller/testsuite/test_commit_barrier_reply.c
View file

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