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

feat(job-array-single-wal-record): implement restore_job_array primitive

Browse source 
Adds the shared N-way array expansion primitive: decodes a committed
WAL_JOB_ARRAY_SUBMIT payload, scratch-builds every element via a
throwaway kv-table (never touching c->jobmgr during construction),
then makes the whole set visible in one bulk pass under a single
write-lock acquisition -- no window where a concurrent reader can
observe a partially-constructed array. Idempotent per index (skips
already-existing jobids; whole-record no-op if all exist). Includes
an XCE_TESTING-guarded fault-injection seam. Not yet wired into any
live dispatch path (tasks 3.2/3.3).

Two review rounds: round 1 found the bulk-insert pass silently
claimed success even when key_add() itself could fail on allocation
exhaustion (c->jobmgr grows via realloc, no reservation primitive
exists anywhere in kv.c/kvmgr.c/zarray.c to pre-check this). Fixed
by checking key_add()'s return and reporting failure honestly
instead of a false success. The underlying table has no key_del, so
an already-inserted prefix on this specific failure mode can remain
genuinely visible while the call reports failure -- documented as a
residual gap in design.md and tasks.md for tasks 3.2/4.x to account
for (that prefix is never pushed to scheduler/API server, even on a
later successful retry, since it's never included in out_new_jobs).

Task 3.1 of job-array-single-wal-record.
This commit is contained in:
Rongsong Shen 2026-07-14 14:16:29 +00:00
parent 78eb983a74
commit 7c755dc4d7
7 changed files with 1346 additions and 1 deletions
Download patch file Download diff file Expand all files Collapse all files

334
src/controller/job.c
View file

@ -1,5 +1,6 @@
#include <errno.h>
#include <signal.h>
#include <stdatomic.h>
#include <stdbool.h>
#include <string.h>
#include <time.h>
@ -333,6 +334,339 @@ int restore_job(cluster_t* c, uint64_t jobid_unused,
return 0;
}
#ifdef XCE_TESTING
/* job-array-single-wal-record task 3.1: forceable, per-index construction
* failure for restore_job_array()'s scratch-build loop -- lets a test prove
* the all-or-nothing atomicity contract (a mid-array failure discards every
* element built THIS attempt, without ever touching c->jobmgr) without
* needing a real OOM. -1 = disabled (default); a value >= 0 forces the
* construction attempt for that 0-based element index within the CURRENT
* array to fail, simulating a resource-exhaustion apply failure (Req 9.4,
* 9.6). File-scope, not per-cluster_t: this task's own test only ever
* drives one cluster_t fixture at a time; a later task needing independent
* multi-node fault injection can revisit this. Compiled out of production
* (xce_controller is built without -DXCE_TESTING). */
static _Atomic int s_job_array_apply_force_fail_at = -1;
void job_array_apply_test_set_force_fail_at(int index)
{
atomic_store_explicit(&s_job_array_apply_force_fail_at, index,
memory_order_relaxed);
}
void job_array_apply_test_clear_force_fail(void)
{
atomic_store_explicit(&s_job_array_apply_force_fail_at, -1,
memory_order_relaxed);
}
/* job-array-single-wal-record task 3.1 remediation (review finding: the
* bulk-insert pass into c->jobmgr was previously unchecked and unfailable
* by test): forces the BULK-INSERT phase's key_add(c->jobmgr, ...) call --
* as opposed to the scratch-build phase's makeJob() above -- to fail at a
* given 0-based index WITHIN THE BULK-INSERT PASS, simulating the one real
* failure this pass can hit: c->jobmgr's backing xzarray hitting a genuine
* realloc() OOM while growing under the write lock (Req 9.4). -1 = disabled
* (default). Like a real allocation failure, the forced index is never
* actually inserted -- indices before it were already inserted for real and
* cannot be un-inserted (see the function doc comment's residual-gap
* paragraph). Compiled out of production. */
static _Atomic int s_job_array_bulk_insert_force_fail_at = -1;
void job_array_apply_test_set_bulk_insert_force_fail_at(int index)
{
atomic_store_explicit(&s_job_array_bulk_insert_force_fail_at, index,
memory_order_relaxed);
}
void job_array_apply_test_clear_bulk_insert_force_fail(void)
{
atomic_store_explicit(&s_job_array_bulk_insert_force_fail_at, -1,
memory_order_relaxed);
}
#endif /* XCE_TESTING */
/* job-array-single-wal-record task 3.1 (design.md Array Apply Expansion,
* Service Interface): expands one committed WAL_JOB_ARRAY_SUBMIT payload
* (base jobid, array_size, one shared job_submit_data_t) into `array_size`
* job_t entries, atomically -- shared by apply_committed_entry() (live
* apply / boot replay) and apply_wal_entry() (standby catch-up), task 3.2/
* 3.3, so the two dispatchers can never diverge in how they expand the same
* committed array record (Req 3.1, 3.2).
*
* Atomicity (Req 2.1, 2.2, 2.3, 9.3, 9.4 -- scratch-build-then-bulk-insert,
* NOT construct-then-rollback): every element is constructed via makeJob()
* -- the same construction create_job() wraps -- against a throwaway,
* function-local kv table (`scratch`, an xkvmgr_type instance identical in
* kind to c->jobmgr itself, so makeJob()'s internal locking/key_find/
* key_add calls behave exactly as they do in production). c->jobmgr is
* NEVER touched while this loop runs -- scratch is never visible to any
* other thread, so there is no window, success path or failure path, in
* which a concurrent reader of c->jobmgr (a status query, a cancel request)
* can observe some array indices present and others absent. Only after
* EVERY element has successfully constructed does one final pass insert
* them all into c->jobmgr under a single held write-lock acquisition. Every
* object here already exists, fully constructed, and no other writer can be
* racing these exact jobids, so key_add() cannot fail on EEXIST or
* content grounds -- but it CAN still fail on resource exhaustion: growing
* c->jobmgr's own backing store (a realloc() reachable through key_add ->
* xkvmgr_add -> vstore -> xzarray_store -> xzarray_ensure_items, kvmgr.c/
* zarray.c) happens WHILE this write lock is held, and c->jobmgr is sized
* far below MAX_ARRAY_IDX in production (jobmgr_nhash/jobmgr_delta,
* cluster.c), so a meaningfully large array is near-certain to trigger a
* growth cycle here. No capacity-reservation primitive exists anywhere in
* c->jobmgr's stack (xkvmgr_type/xzarray/kvtab -- kv.c, kvmgr.c, zarray.c
* all checked) to pre-size it before this loop starts mutating it, so this
* bulk-insert pass is a second, narrower, LATER possible failure point --
* see the residual-atomicity-gap paragraph below for how it differs from a
* scratch-build-phase failure. On a construction failure at index k during
* the SCRATCH-BUILD phase (resource exhaustion only -- Req 9.1's pre-commit
* validation already made a content-derived failure here impossible, Req
* 9.3), the 0..k-1 scratch-built elements are destroyed directly via
* job_dereference (job_t's teardown is wired through its reference
* interface, not the generic destroy() -- job_type.destroy is NULL, jobj.c)
* -- c->jobmgr was never mutated, so there is no key_del/rollback race to
* reason about. A retry re-enters this function from index 0 against a
* fresh scratch list, not from a saved offset.
*
* Idempotent per index (Req 3.3): if jobid already exists in c->jobmgr,
* that index is skipped (not an error) -- handles re-applying an
* already-fully-applied record (replay, or a resolved-but-retried commit).
* If every index already exists, returns -EEXIST (mirrors restore_job()'s
* own convention) and constructs nothing -- the caller must fire no hook.
*
* Every newly constructed element is tagged with submit_lsn = entry_lsn
* (mirrors restore_job(), feeds the existing egress-watermark predicate
* unchanged -- Req 5).
*
* out_new_jobs (optional): on a genuine (non-EEXIST) success, populated
* with every newly created job_t* in index order -- structurally identical
* to the ad hoc xlist_t apply_committed_entry() already builds for a single
* WAL_JOB_SUBMIT's HOOK_DETAIL_JOB_NEW fire, so task 3.2 can feed it
* directly to xce_run_hooks() as the array's one-shot, all-N-elements
* notification (Req 3.4). Left untouched (caller must not read it) on
* -EEXIST or a negative-errno return.
*
* Residual atomicity gap -- BULK-INSERT-phase failure only (narrower than
* the scratch-build-phase failure above, and NOT structurally preventable:
* no capacity-reservation primitive exists to pre-size c->jobmgr, see
* above): if key_add() itself fails partway through the bulk-insert pass
* (at index k of built.count), indices [0, k) are, by that point, REAL and
* PERMANENTLY visible in c->jobmgr -- xkvmgr_type exposes no key_del
* (kvmgr.c), so a partial bulk insert can never be rolled back after the
* fact. This function still reports the failure honestly (a negative
* errno, never a false built.count success) and releases the
* never-inserted [k, built.count) elements so they are not leaked, but it
* CANNOT restore the true "invisible-then-visible, nothing in between"
* guarantee for this specific failure point. Callers -- and task 4.x's Live
* Apply Resource Fence in particular -- must be aware that a negative
* return from this function may still have left a real, non-empty prefix
* of this attempt's elements live and findable in c->jobmgr, with no
* HOOK_DETAIL_JOB_NEW ever fired for them.
*
* Returns the count of newly created elements (1..array_size) on success,
* -EEXIST if every index already existed (idempotent whole-record no-op,
* no hook should fire), or a negative errno on a genuine failure -- in
* which case *out_new_jobs is left untouched and the caller must fire no
* hook. For a SCRATCH-BUILD-phase failure, the scratch list has already
* been discarded and c->jobmgr was never touched at all (Req 9.4: no
* partial set ever observable). For a BULK-INSERT-phase failure, see the
* residual atomicity gap paragraph above -- c->jobmgr may already hold a
* real, permanent prefix of this attempt's elements. */
int restore_job_array(cluster_t* c, const void* capnp_payload, size_t len,
uint64_t entry_lsn, xlist_t* out_new_jobs)
{
if (!c || !capnp_payload || len == 0)
return -EINVAL;
struct capn c_ctx;
capn_init_mem(&c_ctx, (const uint8_t*)capnp_payload, len, 0);
JobSubmit_ptr jp;
jp.p = capn_getp(capn_root(&c_ctx), 0, 1);
struct JobSubmit wire;
memset(&wire, 0, sizeof(wire));
read_JobSubmit(&wire, jp);
xce_job_submit_t decoded;
memset(&decoded, 0, sizeof(decoded));
decode_JobSubmit(&decoded, &wire);
capn_free(&c_ctx);
if (decoded.submit == NULL || decoded.array_size == 0) {
free_JobSubmit(&decoded);
return -EINVAL;
}
uint64_t base_jobid = decoded.jobid;
uint64_t array_size = decoded.array_size;
/* Throwaway construction target -- see the function doc comment above.
* datakiller is deliberately NULL: this scratch table's job_t values are
* always released explicitly (via `built`, below), never through the
* table's own teardown -- xkvmgr_type has no key_del, so relying on a
* datakiller here would make the success path (where ownership of every
* element moves to c->jobmgr) indistinguishable from the failure path
* (where every element must be released) purely from the table's own
* perspective. */
kvmgr_params_t scratch_params
= { .key_type = KVMGR_KEY_INT,
.nslots = (uint32_t)((array_size < 2) ? 2 : array_size),
.delta = (uint32_t)((array_size < 1) ? 1 : array_size),
.datakiller = NULL };
var scratch = makeInstance(&xkvmgr_type, &scratch_params);
if (scratch == NULL) {
int saved_rc = -errno;
free_JobSubmit(&decoded);
return saved_rc;
}
xlist_t built;
xlist_init(&built, (int)array_size);
time_t now = time(NULL);
int rc = 0;
for (uint64_t i = 0; i < array_size; i++) {
uint64_t jobid = base_jobid + i;
job_t* existing = NULL;
lock(c->jobmgr, RTTI_LOCK_READ);
int found = key_find(c->jobmgr, (void*)jobid, (void**)&existing);
unlock(c->jobmgr);
if (found > 0 && existing != NULL)
continue; /* idempotent per-index skip (Req 3.3) */
xce_job_t ej;
memset(&ej, 0, sizeof(ej));
ej.submit = decoded.submit; /* shared/borrowed across every
* element -- job_init's makeJobsub()
* interns/refcounts it, never takes
* ownership; freed once, below. */
ej.jobid = jobid;
ej.sub_time = now;
ej.queue_time = now;
ej.state = JOB_STAT_PEND;
#ifdef XCE_TESTING
int forced = atomic_load_explicit(&s_job_array_apply_force_fail_at,
memory_order_relaxed);
if (forced >= 0 && (uint64_t)forced == i) {
errno = ENOMEM; /* simulated resource-exhaustion failure */
rc = -ENOMEM;
break;
}
#endif
job_t* job = makeJob(scratch, &ej);
if (job == NULL) {
rc = -errno;
break;
}
job->submit_lsn = entry_lsn;
if (xlist_store(&built, job) < 0) {
dereference(job); /* never touched c->jobmgr -- safe to release
* directly (job_type.destroy is NULL; the
* reference interface is the only teardown
* path, jobj.c). */
rc = -ENOMEM;
break;
}
}
/* The one shared spec is always released here, exactly once, whether
* every element succeeded or not -- mirrors restore_job()'s
* unconditional free_Job() discipline. */
free_JobSubmit(&decoded);
if (rc < 0) {
/* All-or-nothing: discard everything built THIS attempt. c->jobmgr
* was never mutated, so there is nothing to roll back there -- a
* retry starts over from index 0 against a fresh scratch list. */
for (int k = 0; k < built.count; k++) {
job_t* j = (job_t*)xlist_get(&built, k);
if (j != NULL)
dereference(j);
}
xlist_fini(&built);
destroy(scratch);
return rc;
}
if (built.count == 0) {
/* Every index already existed -- idempotent whole-record no-op
* (Req 3.3). Nothing was constructed; *out_new_jobs is left
* untouched and no hook should fire. */
xlist_fini(&built);
destroy(scratch);
return -EEXIST;
}
/* One bulk pass, held under a single write-lock acquisition so no
* concurrent reader of c->jobmgr can ever observe a partial insert --
* this is what makes the array atomically invisible-then-visible on the
* SUCCESS path too, not only recoverable on the failure path (Req 2.3,
* 9.4). Every object already exists, fully constructed, and no other
* writer can be racing these exact jobids (only this apply path ever
* constructs them), so key_add() cannot fail on EEXIST or content
* grounds -- but it CAN still fail on resource exhaustion (c->jobmgr's
* backing store growing via realloc() under this write lock; see the
* function doc comment's "Residual atomicity gap" paragraph). Checked
* and reported below, never silently claimed as full success. */
lock(c->jobmgr, RTTI_LOCK_WRITE);
int inserted = 0;
int insert_rc = 0;
for (int k = 0; k < built.count; k++) {
job_t* j = (job_t*)xlist_get(&built, k);
int add_rc;
#ifdef XCE_TESTING
int forced_bulk = atomic_load_explicit(
&s_job_array_bulk_insert_force_fail_at, memory_order_relaxed);
if (forced_bulk >= 0 && forced_bulk == k) {
errno = ENOMEM; /* simulated resource-exhaustion failure */
add_rc = -ENOMEM;
} else
#endif
{
add_rc = key_add(c->jobmgr, (void*)j->job.jobid, (void*)j);
}
if (add_rc < 0) {
insert_rc = add_rc;
break;
}
inserted++;
}
unlock(c->jobmgr);
if (insert_rc < 0) {
/* See the function doc comment's "Residual atomicity gap" paragraph:
* indices [0, inserted) are already real and permanently visible in
* c->jobmgr (no key_del exists to undo them) -- only the
* never-inserted [inserted, built.count) tail is still ours to
* release here. */
for (int k = inserted; k < built.count; k++) {
job_t* j = (job_t*)xlist_get(&built, k);
if (j != NULL)
dereference(j);
}
xlist_fini(&built);
destroy(scratch);
return insert_rc;
}
int created = built.count;
if (out_new_jobs != NULL)
*out_new_jobs = built; /* transfer ownership of built's data array */
else
xlist_fini(&built);
destroy(scratch);
return created;
}
/* Apply a recorded job state transition during WAL recovery.
* Looks up the job (which must already exist from WAL_JOB_SUBMIT replay)
* and overwrites its state with the value stored in the WAL entry. */