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

fix(job-array-single-wal-record): wire dispatcher was silently dropping compact array pushes

Browse source 
extract_data() (src/lib/request.c), the shared reassembly step every
inbound push passes through inside vtask_entry() before any receiver's
handler runs, had no case for Data_data_jobArray -- it hit
default: rc=-EINVAL. Because vtask_entry()'s callnext check treats any
nonzero return (including a negative errno) as "reassembly complete,
proceed," this never surfaced as an error: the real payload was freed
and the receiver silently handed an empty, zeroed dataset_t, with no
log anywhere in the chain. Tasks 5.1-5.3 were each individually
correct and tested in isolation, but the feature was dead on the real
wire end-to-end.

Adds case Data_data_jobArray: passing the single, unchunked
descriptor straight through (no pagination needed, unlike the
existing LIST-shaped cases) and nulls the source pointer after
transfer -- verified this is not cosmetic, since free_Request()'s
pushmsg case would otherwise double-free it via free_Data()'s live
Data_data_jobArray branch.

Proven with a real xce_encode()/xce_decode() round-trip through the
fixed extract_data() -> sched_handle_dataset() -> sched_on_job_array()
(task 5.3, unmodified) chain, not just a unit-level check on
extract_data() alone. Also confirms task 5.4's future API-server
receive path shares this exact fix, since both go through the same
xcecli_ctx_init() -> vtask_entry() -> extract_data() pipeline.

One self-flagged, non-blocking residual gap (pre-existing since task
5.3, out of this task's request.c-only boundary): dataset->rows for
the jobArray case is never deep-freed downstream, leaking one bounded
xce_job_submit_t per successfully-delivered array push commit.

Task 5.3b of job-array-single-wal-record (Requirements 6.1, 6.2, 6.4, 8.4).
Inserted mid-run after independent review confirmed the gap; user
confirmed sequencing (fix now, before task 5.4).
This commit is contained in:
Rongsong Shen 2026-07-15 10:59:39 +00:00
parent d16847185e
commit 1cbd7df1a8
4 changed files with 216 additions and 1 deletions
Download patch file Download diff file Expand all files Collapse all files

3
.kiro/specs/job-array-single-wal-record/tasks.md
View file

@ -131,7 +131,7 @@
- _Boundary: Compact Scheduler/API-Server Push (scheduler daemon receive path)_
- _Depends: 5.1_
- [ ] 5.3b Fix the shared wire dispatcher to carry the compact array push
- [x] 5.3b Fix the shared wire dispatcher to carry the compact array push
- `extract_data()` (`src/lib/request.c`) is the shared reassembly step every inbound push (scheduler and API-server alike) passes through inside `vtask_entry()` (`src/lib/xcectx.c`) before a receiver's handler ever runs. Its dispatch switch has no case for the `Data_data_jobArray` variant (added task 1.2) and falls into `default: rc = -EINVAL`. Because `vtask_entry()`'s `if (callnext)` treats ANY nonzero return — including a negative errno — as "reassembly complete, proceed," this doesn't surface as an error: the real payload is freed and the receiver is silently handed an empty, zeroed `dataset_t` (`rows=NULL, nrows=0`), with no log anywhere in the chain (`vtask_entry()` discards the handler's own return value unconditionally)
- Add a `case Data_data_jobArray:` to `extract_data()` that passes the single, unchunked `JobSubmit` descriptor straight through to the caller's `dataset_t` — no pagination/accumulation loop is needed (unlike the existing LIST-shaped cases), since this push is always exactly one descriptor per commit, matching the "wire shape note" both `xcesched_push_jobarray()` (task 5.1) and `apiserver_push_jobarray()` (task 5.2) already establish
- Confirms both the scheduler (task 5.3, already landed) and the future API-server Go receiver (task 5.4) share this exact fix — this task only needs to land once
@ -191,3 +191,4 @@
- **Task 5.2**: `apiserver_push_jobarray()` (`apiserver.c`) mirrors task 5.1's `xcesched_push_jobarray()` exactly — same field derivation, same direct-`xce_request()` transport, same `l->count > 1` discriminator — and additionally, correctly replicates `apiserver_push_joblist()`'s existing per-connected-server fan-out loop (`N_APISERVER_MAX` iteration, same lock discipline), independently verified against 2 simulated connected servers each receiving exactly one push. Confirmed the same `apiserver.c`/`dataex.c` link-collision task 5.1 hit for `sched.c` (a fake `notify()` already exists in `test_commit_barrier_reply.c`) — new standalone `apiserver_push_unit_test` binary added, correctly omitting `egress.c` since `apiserver_push_joblist()`, unlike `sched.c`'s push path, never references `job_submit_committed()`/`collect_jobs_committed()`. One low-severity, non-blocking nit (not fixed): the new test file's comment justifying its `apiserver_init()` idempotency guard is factually inaccurate — it claims a second `apiserver_init()` call would double-register hooks, but `xce_add_hook()` has deduped by `(fn,data)` since bug-845, so that specific claim is wrong; the guard itself is still correct/necessary for a different, real reason (avoids re-`mylock_init()`-ing an already-initialized mutex, UB).
- **Task 5.3**: `sched_on_job_array()` (`handler.c`) decodes `Data_data_jobArray`, derives each element's jobid via `base_jobid + i`, and routes constructed elements through the unmodified per-job pipeline (`sched_on_jobs()`). Each element gets its OWN deep clone (`clone_job_submit_data`/`clone_job_startinfo`) of the shared submit spec — required because `sched_pend_update_killer()`'s per-element teardown (`free_Job_ptr`) frees each element's `submit` independently and a shared pointer across N elements would double-free; independently verified. Dedup reuses `sched_get_job()` against the same `sched->jobs` kvtab `sched_add_job()`'s `key_add` authoritatively guards — sound at the data-integrity level, but review found a narrow, non-corrupting TOCTOU: the dedup pre-check runs synchronously at receive time while the actual insert only happens later when `sched_update()` drains `pend_updates[0]`, so a redelivery landing before that drain can re-construct/re-enqueue a duplicate (harmless) object and double the confirmation log line — the same pre-existing, task-5.3-unrelated pattern as `sched_update_jobs()`'s already-ignored `key_add` `-EEXIST` return on the ordinary single-job path. Not fixed (pre-existing class of gap, narrow window, non-corrupting).
**CRITICAL discovery during this task's review (2026-07-15), NOT a defect in task 5.3 itself**: the shared wire dispatcher `extract_data()` (`src/lib/request.c`) has no case for `Data_data_jobArray` and silently drops every real array push before ANY receiver's handler runs (see new task 5.3b, inserted immediately after this task per explicit user decision). Confirmed to affect task 5.4's future API-server receive path identically (both share the same `xcecli_ctx_init()` → `vtask_entry()` → `extract_data()` pipeline). Task 5.3's own tests are unaffected by this gap since they use the same established white-box convention as every sibling `handler.c` test (calling `sched_handle_dataset()` directly, bypassing the transport layer) — this is not a new testing shortcut, it just means task 5.3's passing tests do not, and were never intended to, prove real-wire delivery.
- **Task 5.3b**: Added `case Data_data_jobArray:` to `extract_data()` (`request.c`) — sets `dataset->rows`/`nrows=1` directly (no pagination, matching the always-one-shot wire shape) and nulls `getdata->data.job_array` after transferring it. The null is NOT cosmetic — independently verified `free_Request()`'s `Request_body_pushmsg` case calls `free_Data()`, which has a live `Data_data_jobArray` branch that would double-free/UAF without it. Proven end-to-end with a real `xce_encode()`/`xce_decode()` round-trip through `extract_data()` → `sched_handle_dataset()` → `sched_on_job_array()` (task 5.3, unmodified), not just a unit-level check. Also independently confirmed the alternate hypothesis some earlier notes floated — that `scheduler_req_handler()` itself has a `pushmsg`-rejecting branch — is dead code on the live wire path, since `vtask_entry()` always converts `body_which` to `datamsg` before invoking any handler. **New residual gap, self-flagged, NOT fixed (pre-existing since task 5.3, not introduced here, genuinely out of this task's `request.c`-only boundary)**: `dataset->rows` for the jobArray case is never deep-freed downstream — `free_Dataset()` is an empty no-op for all table types, and `sched_handle_dataset()`'s jobArray case (task 5.3) doesn't null it after consuming it. Net effect: one bounded `xce_job_submit_t` + embedded spec leaks per successfully-delivered array push commit (not per element, not unbounded) — real but small; worth a dedicated follow-up before this spec is considered fully closed, alongside the other self-flagged residual gaps (task 3.1's bulk-insert orphan prefix, task 4.2's boot-replay fence gap).

21
src/lib/request.c
View file

@ -1778,6 +1778,27 @@ int extract_data(dataset_t* dataset, xce_data_t* getdata)
memset(getdata->data.resvals->vals, 0x0, count * sizeof(xce_resval_t*));
break;
case Data_data_jobArray:
/* job-array-single-wal-record task 5.3b: single, unchunked JobSubmit
* descriptor (base jobid, array_size, one shared spec) -- unlike the
* LIST-shaped cases above, no pagination/accumulation loop is needed:
* xcesched_push_jobarray() (sched.c) and apiserver_push_jobarray()
* (apiserver.c) always send exactly one complete descriptor per
* commit. Transfer ownership of the decoded pointer straight into
* dataset->rows (mirrors the LIST cases' ownership-transfer intent,
* without an array-of-pointers layer since there is only one row)
* and signal reassembly complete immediately -- returning here skips
* the shared ntotal/pos-based tail below, which does not apply to a
* single-shot payload. */
if (getdata->data.job_array == NULL) {
return -EINVAL;
}
dataset->rows = (void*)getdata->data.job_array;
dataset->nrows = 1;
getdata->data.job_array = NULL;
return 1;
default:
rc = -EINVAL;
break;

96
src/lib/testsuite/test_request.c
View file

@ -176,6 +176,99 @@ static void test_init_reply_by_req_zeroes_reply(void)
TEST_ASSERT_EQUAL_INT(0, reply.data_which);
}
/* ── job-array-single-wal-record task 5.3b: extract_data() wire dispatch for
* Data_data_jobArray ─────────────────────────────────────────────────────
* extract_data() is the shared inbound-push reassembly step every receiver
* (scheduler, API server) passes through inside vtask_entry() (xcectx.c)
* before its own handler ever runs. Its switch had no case for
* Data_data_jobArray -- it fell into `default: rc = -EINVAL`, and
* vtask_entry()'s `if (callnext)` treats ANY nonzero return (including a
* negative errno) as "reassembly complete, proceed" -- so the real payload
* was silently freed and the receiver was handed an empty, zeroed
* dataset_t{rows=NULL, nrows=0}. This test drives the REAL wire encode/
* decode path (mirrors xcesched_push_jobarray()'s exact construction,
* sched.c, and apiserver_push_jobarray()'s identical shape, apiserver.c) so
* the payload extract_data() sees here is genuinely wire-decoded, not a
* hand-built struct. */
static void test_extract_data_job_array_passes_through_intact(void)
{
job_submit_data_t submit;
xce_job_submit_t job_array;
xce_data_t data;
xce_request_t request;
xce_request_t decoded;
io_packet_t *pkt;
dataset_t dataset;
int rc;
xce_job_submit_t *got;
/* Arrange: build the exact wire shape xcesched_push_jobarray()/
* apiserver_push_jobarray() send: OPCode_push / Request_body_pushmsg
* carrying one Data.jobArray descriptor (base jobid, array_size, one
* shared submit spec) -- never chunked, exactly one per commit. */
memset(&submit, 0, sizeof(submit));
submit.queue = "normal";
submit.user = "tester";
submit.mintasks = 1;
submit.maxtasks = 1;
submit.resreq = "type == local";
memset(&job_array, 0, sizeof(job_array));
job_array.jobid = 70000;
job_array.array_size = 12;
job_array.submit = &submit;
memset(&data, 0, sizeof(data));
data.table = Data_data_jobArray;
data.data.job_array = &job_array;
memset(&request, 0, sizeof(request));
request.opcode = OPCode_push;
request.body_which = Request_body_pushmsg;
request.body.pushmsg = &data;
pkt = (io_packet_t *)calloc(1, sizeof(*pkt));
TEST_ASSERT_NOT_NULL(pkt);
TEST_ASSERT_EQUAL_INT(1, xce_encode(&request, pkt));
memset(&decoded, 0, sizeof(decoded));
TEST_ASSERT_EQUAL_INT(0, xce_decode(pkt, &decoded)); /* consumes/frees pkt */
TEST_ASSERT_EQUAL_INT(Request_body_pushmsg, decoded.body_which);
TEST_ASSERT_NOT_NULL(decoded.body.pushmsg);
TEST_ASSERT_EQUAL_INT(Data_data_jobArray, decoded.body.pushmsg->table);
/* Act: this is exactly what vtask_entry() calls on every inbound push. */
memset(&dataset, 0, sizeof(dataset));
dataset.table = decoded.body.pushmsg->table;
rc = extract_data(&dataset, decoded.body.pushmsg);
/* Assert: nonzero/"reassembly complete" (matches vtask_entry's
* `if (callnext)` truthy contract -- a single, unchunked descriptor never
* waits for more) AND the genuine payload, not an empty dataset. Before
* the fix, rc was -EINVAL (still truthy, so vtask_entry proceeded
* anyway) but dataset.rows stayed NULL/nrows stayed 0 -- exactly the
* silent-drop bug this test pins down. */
TEST_ASSERT_EQUAL_INT(1, rc);
TEST_ASSERT_NOT_NULL(dataset.rows);
TEST_ASSERT_EQUAL_INT(1, dataset.nrows);
got = (xce_job_submit_t *)dataset.rows;
TEST_ASSERT_EQUAL_UINT64(70000, got->jobid);
TEST_ASSERT_EQUAL_UINT64(12, got->array_size);
TEST_ASSERT_NOT_NULL(got->submit);
TEST_ASSERT_EQUAL_STRING("normal", got->submit->queue);
TEST_ASSERT_EQUAL_STRING("tester", got->submit->user);
/* Cleanup: extract_data() transfers ownership of the decoded
* xce_job_submit_t out of decoded.body.pushmsg->data.job_array into
* dataset.rows (nulling the source pointer) -- free the now-hollow
* container and the transferred descriptor separately. */
xce_free_request(&decoded);
free_JobSubmit_ptr((xce_job_submit_t **)&dataset.rows);
printf("test_extract_data_job_array_passes_through_intact: PASS\n");
}
void test_request(void)
{
printf("\n>> host control request validation tests\n\n");
@ -193,4 +286,7 @@ void test_request(void)
RUN_TEST(test_ha_heartbeat_req_null_ep);
RUN_TEST(test_ha_heartbeat_req_roundtrip_fields);
RUN_TEST(test_nop_req_remains_bodyless);
printf("\n>> extract_data() Data_data_jobArray wire dispatch tests\n\n");
RUN_TEST(test_extract_data_job_array_passes_through_intact);
}

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

@ -248,6 +248,102 @@ 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 ──────
* 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
* Implementation Notes call this out explicitly: those tests were never
* intended to prove real-wire delivery). This test drives the REAL
* xce_encode()/xce_decode()/extract_data() path (src/lib/request.c, task
* 5.3b's fix) with the exact wire shape xcesched_push_jobarray() (sched.c)
* 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. */
static void test_wire_dispatch_chain_delivers_n_tasks(void)
{
xce_scheduler_t sched;
job_submit_data_t submit;
xce_job_submit_t job_array;
xce_data_t data;
xce_request_t request;
xce_request_t decoded;
io_packet_t *pkt;
dataset_t dataset;
xce_job_t **jobs = NULL;
int njobs;
int i;
fixture_init(&sched);
/* Arrange: the exact wire message xcesched_push_jobarray() sends. */
memset(&submit, 0, sizeof(submit));
submit.queue = "normal";
submit.user = "tester";
submit.mintasks = 1;
submit.maxtasks = 1;
submit.resreq = "type == local";
memset(&job_array, 0, sizeof(job_array));
job_array.jobid = 8000;
job_array.array_size = 4;
job_array.submit = &submit;
memset(&data, 0, sizeof(data));
data.table = Data_data_jobArray;
data.data.job_array = &job_array;
memset(&request, 0, sizeof(request));
request.opcode = OPCode_push;
request.body_which = Request_body_pushmsg;
request.body.pushmsg = &data;
pkt = (io_packet_t *)calloc(1, sizeof(*pkt));
TEST_ASSERT_NOT_NULL(pkt);
TEST_ASSERT_EQUAL_INT(1, xce_encode(&request, pkt));
memset(&decoded, 0, sizeof(decoded));
TEST_ASSERT_EQUAL_INT(0, xce_decode(pkt, &decoded)); /* consumes/frees pkt */
/* Act 1: the REAL transport dispatcher (task 5.3b's fix) -- exactly what
* vtask_entry() (xcectx.c) calls on every inbound push. */
memset(&dataset, 0, sizeof(dataset));
dataset.table = decoded.body.pushmsg->table;
TEST_ASSERT_EQUAL_INT(1, extract_data(&dataset, decoded.body.pushmsg));
TEST_ASSERT_NOT_NULL(dataset.rows);
/* Act 2: hand the reassembled dataset straight to the receiver's own
* 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);
/* 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
* deliberately does not null dataset.rows out (every element is its own
* clone -- see that function's own comment), so this test, as the
* descriptor's owner, frees it directly. decoded.body.pushmsg is now a
* hollow container, safe to free via the normal request teardown. */
xce_free_request(&decoded);
free_JobSubmit_ptr((xce_job_submit_t **)&dataset.rows);
}
int main(int argc, char* argv[])
{
(void)argc; (void)argv;
@ -257,5 +353,6 @@ int main(int argc, char* argv[])
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);
return UNITY_END();
}