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

refactor(scheduler): defer job-array expansion off the vtask thread pool

Browse source 
sched_on_job_array() expanded a compact array-job push (up to
hundreds of elements: a per-element dedup lookup plus a full deep
clone) inline, synchronously, on whatever vtaskmgr thread handled the
incoming request -- the same shared pool the scheduler's own
controller connection traffic depends on, violating ha-async-io.md's
"bounded handler work only" contract for that pool (a project-wide
design invariant). A prior finding already showed a burst of
6x50-element array pushes (300 job events landing within ~2ms of each
other) causing the scheduler to lose its controller connection within
245ms and enter a reconnect/failover loop -- directly consistent with
this mechanism, and the same reconnect storm whose resync races
motivated the companion JOBS_PUSH fix in this branch.

The request handler now does O(1) work only: validate the descriptor
and enqueue it unexpanded (XCE_SCHED_OP_UPDATE_JOB_ARRAY, mirroring
the existing XCE_SCHED_OP_ADMIN_RECORD deferred-dispatch pattern).
The actual per-element expansion and dedup check moved to
translate_one_pending_update(), which runs on the round-processing
thread (main.c's xce_sched_driver_run_normal_mode_round(), before
xce_core_begin_session() each round) -- a thread with no connection-
liveness responsibility. As a side effect this also closes the
dedup check's own pre-existing cross-thread TOCTOU race (previously
documented as bug-1714 follow-up "Bug B"): only one thread ever runs
it now.

test_job_array_expand.c and test_job_array_wire_e2e.c, which asserted
the old inline-expansion behavior, are updated to assert the new
deferred-enqueue behavior instead; expansion, dedup, and full
drain-push-release ownership coverage moved to new tests in
test_driver_translate.c.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RLSNJhNoULvVcUhXEFafQC
This commit is contained in:
Rongsong Shen 2026-09-15 09:01:47 +00:00
parent 6c201bb3e8
commit d5ec1eb3ae
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
10 changed files with 838 additions and 517 deletions
Download patch file Download diff file Expand all files Collapse all files

182
src/scheduler/testsuite/test_job_array_expand.c
View file

@ -3,12 +3,22 @@
* job-array push receive path (job-array-single-wal-record task 5.3, Req
* 6.3, 6.4, 6.5, 8.4).
*
* REVISED (connection-loss-under-burst fix, see XCE_SCHED_OP_UPDATE_JOB_
* ARRAY's own comment, scheduler.h): sched_on_job_array() no longer expands
* the compact descriptor into N tasks itself -- it just takes ownership and
* enqueues it as a single XCE_SCHED_OP_UPDATE_JOB_ARRAY request. The actual
* per-element expansion (and its dedup check) now runs in driver.c's
* translate_one_pending_update(), covered by test_driver_translate.c's own
* UPDATE_JOB_ARRAY tests. This file now covers only: (a) sched_on_job_array()
* /sched_handle_dataset()'s own input validation and ownership-transfer
* bookkeeping, and (b) that the wire-decode -> handler -> queue chain
* delivers the descriptor's fields correctly, unmodified.
*
* Strategy: pull the real (static) sched_handle_dataset / sched_on_job_array
* into this TU via #include "../src/handler.c" (same white-box pattern as
* test_handlers.c), driven against a REAL sched->jobs kvmgr so the
* per-element dedup check (sched_get_job) is exercised for real rather than
* faked. sched_add_job/sched_get_job below are the SAME kvmgr key_add/
* key_find path production uses (mirrors test_job_update.c's precedent).
* test_handlers.c). sched_add_job/sched_get_job stubs below are unused by
* anything this file still exercises (kept, harmless, for parity with other
* handler.c white-box test files that DO need them).
*/
#define UNIT_TEST 1
#include <errno.h>
@ -94,13 +104,6 @@ static void fixture_init(xce_scheduler_t* sched)
sched->core.jobs = makeInstance(&xkvmgr_type, &jobs_params);
}
static void seed_existing_job(xce_scheduler_t* sched, uint64_t jobid)
{
xce_sched_job_t* job = (xce_sched_job_t*)calloc(1, sizeof(*job));
job->jobid = jobid;
sched_add_job(&(sched->core), job);
}
/* Builds a heap-allocated, heap-owned-strings (jobArray, submit) pair -- the
* same shape decode_JobSubmit_ptr() produces on the real wire-decode path
* (controller.capnp.c), not a stack/string-literal stand-in. Task 7.3 made
@ -128,41 +131,42 @@ static xce_job_submit_t* init_job_array(uint64_t base_jobid,
return ja;
}
/* Pop one pending update request, asserting it is an UPDATE_JOB op; hands
* back njobs/jobs (caller now owns them -- free via free_Job_ptr + free() to
* mirror sched_pend_update_killer). Returns -1 if the queue is empty. */
static int pop_job_update(xce_scheduler_t* sched, xce_job_t*** jobs_out)
/* Pop one pending update request, asserting it is an UPDATE_JOB_ARRAY op;
* hands back the raw job_array descriptor (caller now owns it -- free via
* free_JobSubmit_ptr to mirror release_translated_payload()'s own cleanup on
* the failure/never-translated path). Returns NULL if the queue is empty. */
static xce_job_submit_t* pop_job_array_update(xce_scheduler_t* sched)
{
data_item_t* it = dataq_deq(sched->core.pend_updates[0], NULL);
xce_sched_update_request_t* req;
int njobs;
xce_job_submit_t* job_array;
if (it == NULL) {
return -1;
return NULL;
}
req = ITPTR_TO_UPDATE_REQ(it);
TEST_ASSERT_EQUAL_INT(XCE_SCHED_OP_UPDATE_JOB, req->op);
TEST_ASSERT_EQUAL_INT(XCE_SCHED_OP_UPDATE_JOB_ARRAY, req->op);
njobs = req->u.jobupdate.njobs;
if (jobs_out != NULL) {
*jobs_out = req->u.jobupdate.jobs;
}
job_array = req->u.jobarrayupdate.job_array;
TEST_ASSERT_EQUAL_INT(0, req->u.jobarrayupdate.expanded_njobs);
TEST_ASSERT_NULL(req->u.jobarrayupdate.expanded_jobs);
free(req);
return njobs;
return job_array;
}
/* ── Req 6.3/6.4: a compact Data.jobArray push expands locally into N
* ordinary schedulable tasks via base_jobid + i arithmetic ── */
/* ── Connection-loss-under-burst fix: a compact Data.jobArray push is now
* queued UNEXPANDED -- ownership of the raw descriptor transfers to the
* XCE_SCHED_OP_UPDATE_JOB_ARRAY request as-is, no per-element work happens
* on this (request-handling) thread. Expansion into N tasks is
* test_driver_translate.c's concern now (translate_one_pending_update()). ── */
static void test_job_array_push_expands_into_n_tasks(void)
static void test_job_array_push_enqueues_raw_descriptor_unexpanded(void)
{
xce_scheduler_t sched;
xce_job_submit_t* ja;
dataset_t dataset;
xce_job_t** jobs = NULL;
int njobs;
int i;
xce_job_submit_t* popped;
fixture_init(&sched);
ja = init_job_array(5000, 3);
@ -171,71 +175,19 @@ static void test_job_array_push_expands_into_n_tasks(void)
dataset.table = XceData_data_jobArray;
dataset.rows = (void*)ja;
/* sched_handle_dataset() owns and frees dataset.rows on return (task
* 7.3) -- do not touch ja/dataset.rows again after this call. */
TEST_ASSERT_TRUE(sched_handle_dataset(&sched, &dataset) >= 0);
njobs = pop_job_update(&sched, &jobs);
TEST_ASSERT_EQUAL_INT(3, njobs);
TEST_ASSERT_EQUAL_UINT64(5000, jobs[0]->jobid);
TEST_ASSERT_EQUAL_UINT64(5001, jobs[1]->jobid);
TEST_ASSERT_EQUAL_UINT64(5002, jobs[2]->jobid);
TEST_ASSERT_EQUAL_STRING("normal", jobs[0]->submit->queue);
TEST_ASSERT_EQUAL_STRING("normal", jobs[2]->submit->queue);
/* every element gets its OWN independent clone, not a shared pointer --
* required so each is independently free_Job_ptr()-able downstream
* (sched_pend_update_killer) without a double-free. */
TEST_ASSERT_TRUE(jobs[0]->submit != jobs[1]->submit);
TEST_ASSERT_TRUE(jobs[1]->submit != jobs[2]->submit);
/* Ownership transferred to the queued request -- sched_handle_dataset()
* must not have freed it and must have nulled dataset.rows out. */
TEST_ASSERT_NULL(dataset.rows);
for (i = 0; i < njobs; i++) {
free_Job_ptr(&jobs[i]);
}
free(jobs);
}
popped = pop_job_array_update(&sched);
TEST_ASSERT_EQUAL_PTR(ja, popped); /* same descriptor, not a clone */
TEST_ASSERT_EQUAL_UINT64(5000, popped->jobid);
TEST_ASSERT_EQUAL_UINT64(3, popped->array_size);
TEST_ASSERT_EQUAL_STRING("normal", popped->submit->queue);
/* ── Req 6.5/8.4: a redelivered push (e.g. after a scheduler reconnect)
* must not create duplicate tasks -- reuse the jobid-keyed exists-check
* (sched_get_job) the ordinary single-job path relies on ── */
static void test_job_array_redelivery_does_not_duplicate(void)
{
xce_scheduler_t sched;
dataset_t dataset;
xce_job_t** jobs = NULL;
int njobs;
int i;
fixture_init(&sched);
memset(&dataset, 0, sizeof(dataset));
dataset.table = XceData_data_jobArray;
dataset.rows = (void*)init_job_array(6000, 2);
/* first delivery: 2 new tasks enqueued. sched_handle_dataset() owns and
* frees dataset.rows on return (task 7.3). */
TEST_ASSERT_TRUE(sched_handle_dataset(&sched, &dataset) >= 0);
njobs = pop_job_update(&sched, &jobs);
TEST_ASSERT_EQUAL_INT(2, njobs);
for (i = 0; i < njobs; i++) {
/* simulate the async consumer (scheduler.c's sched_update_jobs ->
* sched_add_job, not linked into this white-box TU) materializing
* the element into sched->jobs, so the redelivery's dedup check
* sees them as already existing -- exactly what a real reconnect
* scenario looks like from the handler's point of view. */
seed_existing_job(&sched, jobs[i]->jobid);
free_Job_ptr(&jobs[i]);
}
free(jobs);
/* redelivery of the IDENTICAL push: a genuinely new decode of the
* retransmitted wire message (same jobid/array_size/submit content, but
* its own freshly-allocated descriptor -- exactly what a real reconnect
* redelivers; the first descriptor is already freed by the call above).
* Every index already exists. */
dataset.rows = (void*)init_job_array(6000, 2);
TEST_ASSERT_EQUAL_INT(0, sched_handle_dataset(&sched, &dataset));
TEST_ASSERT_EQUAL_INT(-1, pop_job_update(&sched, NULL)); /* nothing new */
free_JobSubmit_ptr(&popped);
}
/* ── Guard-clause coverage on the decode step itself ── */
@ -279,7 +231,8 @@ static void test_job_array_null_descriptor_returns_einval(void)
TEST_ASSERT_EQUAL_INT(-EINVAL, sched_on_job_array(&sched, NULL));
}
/* ── job-array-single-wal-record task 5.3b: full wire-dispatch chain ──────
/* ── job-array-single-wal-record task 5.3b: full wire-dispatch chain
* (REVISED for the connection-loss-under-burst fix) ──────────────────────
* Every test above feeds a stack-built xce_job_submit_t straight into
* sched_handle_dataset(), bypassing the transport layer entirely -- the same
* white-box convention every sibling handler.c test uses (task 5.3's own
@ -290,10 +243,11 @@ static void test_job_array_null_descriptor_returns_einval(void)
* sends -- OPCode_push / Request_body_pushmsg carrying one Data.jobArray
* descriptor -- then hands extract_data()'s reassembled dataset_t to
* sched_handle_dataset(), proving the sender-payload -> wire-dispatch ->
* receiver-expand chain is intact end to end, not just sched_on_job_array()
* in isolation. */
* receiver-enqueue chain delivers the descriptor's fields correctly, intact
* end to end -- expansion itself is no longer this chain's job (driver.c's
* concern now), so this only proves the queued descriptor. */
static void test_wire_dispatch_chain_delivers_n_tasks(void)
static void test_wire_dispatch_chain_delivers_job_array_descriptor(void)
{
xce_scheduler_t sched;
job_submit_data_t submit;
@ -303,9 +257,7 @@ static void test_wire_dispatch_chain_delivers_n_tasks(void)
xce_request_t decoded;
io_packet_t* pkt;
dataset_t dataset;
xce_job_t** jobs = NULL;
int njobs;
int i;
xce_job_submit_t* popped;
fixture_init(&sched);
@ -350,27 +302,22 @@ static void test_wire_dispatch_chain_delivers_n_tasks(void)
* handler (task 5.3, unmodified) -- proving the two layers compose. */
TEST_ASSERT_TRUE(sched_handle_dataset(&sched, &dataset) >= 0);
/* Assert: N schedulable tasks, correctly addressed via base_jobid + i. */
njobs = pop_job_update(&sched, &jobs);
TEST_ASSERT_EQUAL_INT(4, njobs);
TEST_ASSERT_EQUAL_UINT64(8000, jobs[0]->jobid);
TEST_ASSERT_EQUAL_UINT64(8001, jobs[1]->jobid);
TEST_ASSERT_EQUAL_UINT64(8002, jobs[2]->jobid);
TEST_ASSERT_EQUAL_UINT64(8003, jobs[3]->jobid);
TEST_ASSERT_EQUAL_STRING("normal", jobs[0]->submit->queue);
TEST_ASSERT_EQUAL_STRING("normal", jobs[3]->submit->queue);
for (i = 0; i < njobs; i++) {
free_Job_ptr(&jobs[i]);
}
free(jobs);
/* Assert: the decoded descriptor was queued unexpanded, fields intact,
* correctly addressed via base_jobid + array_size. */
popped = pop_job_array_update(&sched);
TEST_ASSERT_NOT_NULL(popped);
TEST_ASSERT_EQUAL_UINT64(8000, popped->jobid);
TEST_ASSERT_EQUAL_UINT64(4, popped->array_size);
TEST_ASSERT_EQUAL_STRING("normal", popped->submit->queue);
free_JobSubmit_ptr(&popped);
/* Cleanup: extract_data() transferred ownership of the decoded
* xce_job_submit_t into dataset.rows (nulling the source pointer inside
* decoded.body.pushmsg); sched_handle_dataset()'s jobArray case now
* frees dataset.rows itself and nulls it out (task 7.3), so there is
* nothing left for this test to free there. decoded.body.pushmsg is a
* hollow container, safe to free via the normal request teardown. */
* decoded.body.pushmsg); sched_handle_dataset()'s jobArray case
* transfers that ownership again, into the queued request, and nulls
* dataset.rows out -- nothing left for this test to free there.
* decoded.body.pushmsg is a hollow container, safe to free via the
* normal request teardown. */
TEST_ASSERT_NULL(dataset.rows);
xce_free_request(&decoded);
}
@ -380,11 +327,10 @@ int main(int argc, char* argv[])
(void)argc;
(void)argv;
UNITY_BEGIN();
RUN_TEST(test_job_array_push_expands_into_n_tasks, 0);
RUN_TEST(test_job_array_redelivery_does_not_duplicate, 0);
RUN_TEST(test_job_array_push_enqueues_raw_descriptor_unexpanded, 0);
RUN_TEST(test_job_array_null_submit_returns_einval, 0);
RUN_TEST(test_job_array_zero_size_returns_einval, 0);
RUN_TEST(test_job_array_null_descriptor_returns_einval, 0);
RUN_TEST(test_wire_dispatch_chain_delivers_n_tasks, 0);
RUN_TEST(test_wire_dispatch_chain_delivers_job_array_descriptor, 0);
return UNITY_END();
}