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

feat(raft-apply-decoupling): defer job construction past the commit barrier (task 3.1)

Browse source 
Reorder handle_jobsubmit() so job objects are constructed via
create_job() only after the WAL commit succeeds, not before. Adds
jobid_range_exists() for a fast-fail pre-check with no WAL work, and
persist_submit_job_payloads_and_commit() (persist.c) operating on raw
xce_job_t* payloads instead of pre-existing job_t*. Commit remains
synchronous -- converting to the continuation-based commit API is
task 3.2.

Fixes bug-1408 (found during review): the post-commit array-
construction loop suppressed notification for an entire array batch
if any one sibling element failed to construct, stranding other
durably-committed and successfully-constructed jobs invisible until
the next restart's replay. Notification now fires unconditionally
for every successfully-constructed element, independent of sibling
failures; the client reply still separately reports the batch's
construction status.
This commit is contained in:
Rongsong Shen 2026-07-04 04:44:43 +00:00
parent 8febd663d5
commit 6f1fa97fab
6 changed files with 871 additions and 399 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -834,6 +834,264 @@ static void test_worker_join_no_hostconfig_error_reply_immediate(void)
printf("test_worker_join_no_hostconfig_error_reply_immediate: PASS\n");
}
/* ── handle_jobsubmit (raft-apply-decoupling task 3.1: apply-after-commit) ──
*
* Req 2.1/2.3: a job submission whose commit fails or times out must leave
* NO job object in c->jobmgr — construction is deferred until after the
* commit barrier succeeds. Req 2.2: a successful submission still replies
* synchronously with the jobid, and by the time that reply is observable the
* job is already visible in c->jobmgr. */
static job_startinfo_t g_jobsub_startinfo;
static job_submit_data_t g_jobsub_data;
static xce_job_submit_t g_jobsub_msg;
static xce_request_t g_jobsub_req;
static xce_peerinfo_t g_jobsub_from;
static void cbrp_arrange_jobsubmit(uint64_t jobid, uint64_t array_size)
{
memset(&g_jobsub_startinfo, 0, sizeof(g_jobsub_startinfo));
g_jobsub_startinfo.cmdline = (char *)"/bin/true";
memset(&g_jobsub_data, 0, sizeof(g_jobsub_data));
g_jobsub_data.queue = (char *)"normal";
g_jobsub_data.user = (char *)"tester";
g_jobsub_data.start_info = &g_jobsub_startinfo;
memset(&g_jobsub_msg, 0, sizeof(g_jobsub_msg));
g_jobsub_msg.jobid = jobid;
g_jobsub_msg.array_size = array_size;
g_jobsub_msg.submit = &g_jobsub_data;
g_jobsub_from.name = (char *)"requester";
g_jobsub_from.key = (char *)"";
memset(&g_jobsub_req, 0, sizeof(g_jobsub_req));
g_jobsub_req.opcode = OPCode_jobsub;
g_jobsub_req.seq = 15;
g_jobsub_req.from = &g_jobsub_from;
g_jobsub_req.body_which = Request_body_jobsubmsg;
g_jobsub_req.body.jobsubmsg = &g_jobsub_msg;
}
/* Direct c->jobmgr lookup — the task 3.1 observable is about the in-memory
* job TABLE, not the wire reply, so it is checked independently of
* cbrp_scan_replies/cbrp_reply_obs_t (which do not carry a jobid). */
static bool cbrp_job_exists(uint64_t jobid)
{
job_t *j = NULL;
int rc;
lock(g_hc.jobmgr, RTTI_LOCK_READ);
rc = key_find(g_hc.jobmgr, (void *)jobid, (void **)&j);
unlock(g_hc.jobmgr);
return (rc > 0 && j != NULL);
}
static void test_jobsubmit_withheld_acks_no_phantom_job(void)
{
cbrp_setup(300);
cbrp_arrange_jobsubmit(9100, 1);
cbrp_drive_withheld(handle_jobsubmit, &g_jobsub_req, 300);
TEST_ASSERT_FALSE_MESSAGE(cbrp_job_exists(9100),
"a job submission whose commit times out must leave no job object "
"in the in-memory job table (raft-apply-decoupling task 3.1, "
"Req 2.1/2.3)");
cbrp_teardown();
printf("test_jobsubmit_withheld_acks_no_phantom_job: PASS\n");
}
static void test_jobsubmit_success_reply_and_job_visible_after_commit(void)
{
cbrp_setup(1500);
cbrp_arrange_jobsubmit(9101, 1);
cbrp_drive_acked(handle_jobsubmit, &g_jobsub_req);
TEST_ASSERT_TRUE_MESSAGE(cbrp_job_exists(9101),
"a successful job submission must construct the job object once "
"its commit succeeds, and still reply synchronously with the "
"jobid (raft-apply-decoupling task 3.1, Req 2.2)");
cbrp_teardown();
printf("test_jobsubmit_success_reply_and_job_visible_after_commit: PASS\n");
}
/* Captures every jobid delivered via the AFTER_EVENT/HOOK_DETAIL_JOB_NEW
* notification hook (job.c's post-commit notify list). Needed by the
* array-partial-construction-failure remediation test below to observe
* that a durably committed AND successfully constructed sibling is still
* notified even when another array element in the same submission fails to
* construct (raft-apply-decoupling task 3.1 remediation). xce_run_hooks
* also fires this same hook TYPE for HOOK_BEFORE_EVENT with a different
* vararg shape (xce_job_submit_t*, not xlist_t*) — filtered out below
* before touching ap. */
#define CBRP_MAX_JOBNEW_NOTIFY 8
static int g_jobnew_notify_count;
static uint64_t g_jobnew_notify_jobids[CBRP_MAX_JOBNEW_NOTIFY];
static void cbrp_jobnew_capture_reset(void)
{
g_jobnew_notify_count = 0;
memset(g_jobnew_notify_jobids, 0, sizeof(g_jobnew_notify_jobids));
}
static int cbrp_jobnew_capture_fn(int type, int situation, int detail,
void *data, va_list ap)
{
(void)type; (void)data;
if (detail != HOOK_DETAIL_JOB_NEW || situation != HOOK_AFTER_EVENT)
return 0;
{
xlist_t *jlist;
int i;
(void)va_arg(ap, cluster_t *);
jlist = va_arg(ap, xlist_t *);
if (jlist == NULL)
return 0;
for (i = 0; i < jlist->count
&& g_jobnew_notify_count < CBRP_MAX_JOBNEW_NOTIFY; i++) {
job_t *j = (job_t *)xlist_get(jlist, i);
if (j != NULL)
g_jobnew_notify_jobids[g_jobnew_notify_count++] = j->job.jobid;
}
}
return 0;
}
static bool cbrp_jobnew_notified(uint64_t jobid)
{
int i;
for (i = 0; i < g_jobnew_notify_count; i++) {
if (g_jobnew_notify_jobids[i] == jobid)
return true;
}
return false;
}
/* Array-partial-construction-failure remediation (round-2 fix, task 3.1):
* when ONE array element's post-commit create_job() fails (a jobid
* collision that slipped past jobid_range_exists()'s pre-check, or any
* other construct-time failure), the OTHER array elements that constructed
* successfully — and are already durably committed — must still fire the
* AFTER_EVENT/JOB_NEW notification hook. Both scheduler and apiserver are
* purely event-driven off this hook with no periodic reconciliation, so a
* withheld notification strands an otherwise-healthy, already-durable job
* until the next controller restart replays the WAL.
*
* The colliding element's failure is reproduced deterministically (no real
* race) by exploiting the actual TOCTOU window between
* jobid_range_exists()'s pre-check (a READ-locked snapshot taken BEFORE any
* WAL work) and makeJob_'s own key_find (a WRITE-locked check made fresh at
* construction time, AFTER the commit barrier): a stand-in job for the
* second array element's jobid is inserted directly into c->jobmgr from the
* test thread strictly after the pre-check has already run (its own WAL
* entry is visible, which cannot happen until after the pre-check passes)
* and strictly before the construction loop can run (its commit is still
* pending on a withheld ACK). */
static void test_jobsubmit_array_partial_construct_failure_notifies_survivors(void)
{
cbrp_reply_obs_t obs = { 0 };
uint64_t lsn_before, wm0, wm1;
job_submit_data_t collide_submit;
bool elem0_wal_appeared, elem1_wal_appeared;
bool elem0_exists, elem0_notified;
/* Every TEST_ASSERT_* below is deferred until after cbrp_teardown().
* A failing Unity assertion longjmps out of this function (TEST_ABORT),
* which would otherwise skip cbrp_teardown() and leak live
* persist/commit-engine background threads into the NEXT test's setup,
* deadlocking the whole group (see bug-1408's round-2 correction: this
* exact failure mode was originally reproduced as a hang, not a clean
* FAIL, under the standard --run test_commit_barrier_reply invocation).
* Capture every observable into a local first, teardown unconditionally,
* then assert on the captured locals. */
cbrp_setup(2000);
cbrp_jobnew_capture_reset();
TEST_ASSERT_EQUAL_INT(0, xce_add_hook(&g_hc.hooksys, XCE_CONTROLLER_HOOK_JOB,
cbrp_jobnew_capture_fn, NULL));
cbrp_arrange_jobsubmit(9200, 2); /* 9200 survives, 9201 collides */
pthread_mutex_lock(&g_hc.persist.lock);
lsn_before = g_hc.persist.current_lsn;
pthread_mutex_unlock(&g_hc.persist.lock);
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));
/* Element 0's (jobid 9200) WAL entry appears first; ACK it so the
* sequential payload loop advances to element 1. The settling sleep
* before each ACK mirrors cbrp_drive_acked's proven pattern — it gives
* the handler thread time to actually register its commit wait for the
* just-appended LSN before the ACK is delivered. */
wm0 = cbrp_poll_append(&g_hc.persist, lsn_before, 2000);
elem0_wal_appeared = (wm0 > 0);
cbrp_sleep_ms(150);
if (elem0_wal_appeared)
repl_on_ack(&g_hc.persist.repl, &g_hep_a, wm0);
/* Element 1's (jobid 9201) WAL entry appears once element 0 commits.
* Its own ACK is deliberately withheld until the collision stand-in
* below is in place. cbrp_poll_append's lsn_before is a current_lsn
* SNAPSHOT (see its first use above, seeded from persist.current_lsn),
* not an entry's assigned LSN — element 0 already advanced current_lsn
* to wm0 + 1, so that (not wm0 itself) is the correct baseline to wait
* past for element 1's append. */
wm1 = cbrp_poll_append(&g_hc.persist, wm0 + 1, 2000);
elem1_wal_appeared = (wm1 > wm0);
cbrp_sleep_ms(150);
/* Deterministic TOCTOU-outcome injection (see block comment above). */
memset(&collide_submit, 0, sizeof(collide_submit));
cbrp_make_job(9201, &collide_submit);
if (elem1_wal_appeared)
repl_on_ack(&g_hc.persist.repl, &g_hep_a, wm1);
cbrp_poll_reply(&g_req_ep, 2000, &obs);
pthread_join(g_op_tid, NULL);
elem0_exists = cbrp_job_exists(9200);
elem0_notified = cbrp_jobnew_notified(9200);
cbrp_teardown();
TEST_ASSERT_TRUE_MESSAGE(elem0_wal_appeared,
"array element 0's WAL entry must appear");
TEST_ASSERT_TRUE_MESSAGE(elem1_wal_appeared,
"array element 1's WAL entry must appear");
TEST_ASSERT_TRUE_MESSAGE(obs.n_replies > 0,
"the requester must still receive a reply when one array element "
"fails post-commit construction");
TEST_ASSERT_TRUE_MESSAGE(obs.last_status < 0,
"a partially-failed array submission must report the construction "
"failure to the client");
TEST_ASSERT_TRUE_MESSAGE(elem0_exists,
"the non-colliding array element must still be constructed and "
"present in c->jobmgr despite its sibling failing to construct");
TEST_ASSERT_TRUE_MESSAGE(elem0_notified,
"the successfully constructed sibling must fire the AFTER_EVENT/"
"JOB_NEW notification hook even though array element 9201 failed "
"to construct post-commit (raft-apply-decoupling task 3.1 "
"remediation) — otherwise it is durably committed and constructed "
"but invisible to the purely event-driven scheduler/apiserver "
"until the next controller restart replays the WAL");
printf("test_jobsubmit_array_partial_construct_failure_notifies_survivors:"
" PASS\n");
}
/* ════════════════════════════════════════════════════════════════════════
* Receipt-ACK verdict pins (ha-commit-barrier-rollout task 3.3, Req 2.3)
*
@ -1873,6 +2131,10 @@ void test_commit_barrier_reply(void)
RUN_TEST(test_worker_join_withheld_acks_replies_error);
RUN_TEST(test_worker_join_success_reply_only_after_commit);
RUN_TEST(test_worker_join_no_hostconfig_error_reply_immediate);
/* handle_jobsubmit (raft-apply-decoupling task 3.1, Req 2.1/2.2/2.3) */
RUN_TEST(test_jobsubmit_withheld_acks_no_phantom_job);
RUN_TEST(test_jobsubmit_success_reply_and_job_visible_after_commit);
RUN_TEST(test_jobsubmit_array_partial_construct_failure_notifies_survivors);
/* Receipt-ACK verdict pins (task 3.3, Req 2.3) — green on landing */
RUN_TEST(test_jobdispatch_receipt_ack_no_wal_append);
RUN_TEST(test_set_preason_receipt_reply_fact_retained);