fix(job-array-single-wal-record): close bug-1443's bulk-insert partial-visibility gap
restore_job_array()'s bulk-insert pass could fail partway through (key_add() on realloc growth exhaustion), leaving an already-inserted prefix permanently visible in c->jobmgr but excluded from out_new_jobs -- so those elements existed and were individually addressable but never reached HOOK_DETAIL_JOB_NEW, never scheduled or pushed, not on this attempt or any later successful retry. A literal violation of Requirements 2.3 and 9.4, tracked as bug-1443. Adds kvmgr_reserve()/xzarray_reserve(), new purely-additive primitives in shared kvmgr.c/zarray.c (following the existing kvmgr_capture_keys() template -- bare extern function, no RTTI vtable slot, caller-holds- write-lock contract), that pre-grow c->jobmgr's backing store before the bulk-insert pass begins, closing the dominant N-scaling failure trigger structurally. Found a genuine off-by-one in the existing, unmodified xzarray_ensure_items(): its strict less-than sufficiency check means reserving exactly array_size and then performing array_size real stores still triggers one more growth cycle on the last store. Reserving array_size + 1 is the exact minimal correct compensation, independently re-derived and confirmed by review with concrete arithmetic. For the now-much-narrower residual case (kvtab_add()'s own small per-entry allocation, or a content collision -- both non-N-scaling, honestly documented as still theoretically possible): restore_job_array() now populates out_new_jobs with whatever prefix was genuinely inserted before a failure, and apply_committed_entry() fires HOOK_DETAIL_JOB_NEW for that non-empty partial prefix even when the overall return is negative, while still propagating the negative rc so the LSN correctly halts and the Live Apply Resource Fence still engages for the remainder. A genuinely-inserted element is never left un-pushed again. Task 7.1 of job-array-single-wal-record (Requirements 2.3, 9.4, 9.6).
This commit is contained in:
parent
575f928bbc
commit
dcf9f39c22
13 changed files with 910 additions and 36 deletions
|
|
@ -188,7 +188,7 @@
|
|||
- _Boundary: Non-Regression & Testing_
|
||||
|
||||
- [ ] 7. Residual gap closure (post-GO follow-up, user-directed 2026-07-16)
|
||||
- [ ] 7.1 Close the bulk-insert-phase partial-visibility gap (bug-1443)
|
||||
- [x] 7.1 Close the bulk-insert-phase partial-visibility gap (bug-1443)
|
||||
- Add a new, purely-additive capacity-reservation primitive (`kvmgr_reserve(var kvmgr, int n)` in `kvmgr.c`, following the existing non-vtable `kvmgr_capture_keys()` template exactly — same "caller MUST hold the kvmgr write lock" contract, same bare-extern-function style rather than a new RTTI vtable slot) that pre-grows `c->jobmgr`'s backing `xzarray` for `n` additional entries via a new small non-static wrapper in `zarray.c` calling the existing, unmodified `xzarray_ensure_items(a, a->count + n)` — zero changes to any existing function in either file
|
||||
- Call `kvmgr_reserve(c->jobmgr, array_size)` in `restore_job_array()` immediately before the bulk-insert pass begins; on reservation failure, treat exactly like today's existing pre-mutation failure paths (discard the scratch list, `c->jobmgr` never touched, return negative) — this eliminates the dominant, N-scaling failure trigger (the large `realloc()` growing the value store) structurally, before any `key_add()` call in the bulk-insert loop can run
|
||||
- Document honestly (in the function's doc comment) that this narrows, not eliminates, the residual gap: `key_add()`'s own tiny per-entry hash-chain-node allocation and a content-based `-EEXIST` collision remain theoretically possible even after reservation, both far rarer and non-N-scaling than the closed failure mode
|
||||
|
|
@ -237,3 +237,4 @@
|
|||
- **Task 6.3**: Pure verification task, zero file changes. Ran the complete suite: `controller_unit_test` (299 tests, 0 failures, verified via raw-log `:FAIL:` grep, not the exit code/summary — per task 6.0a's nested-`UNITY_BEGIN`/`END` counter-reset finding), all scheduler binaries, `xcelib_test` (0 failures with the registered `detect_leaks=0` config; the one pre-existing task-1.3 rtti leak still reproduces under leak-detection-on, unchanged), Go apiserver tests under both `CGO_ENABLED=0` and `=1`, and every soak mode this spec added (`--selftest` 28/28, sequential baseline, `--chaotic`, `--array-soak`, `--array-chaos-retry`) — all clean. Designed and executed a genuine empirical byte-identical baseline comparison for the non-array path (Requirement 8.1): built commit `895a204` (immediately pre-dating this spec's first commit) in an isolated, cleanly-removed `git worktree add --detach`, deployed independent clusters from both the baseline and current-tree binaries, submitted one identical single-job request to each, and did a byte-for-byte `cmp -l` of the raw on-disk WAL entries. Independently reproduced end-to-end by the reviewer (separate build, separate clusters, separate comparison) with a consistent result: every differing byte mapped cleanly to `timestamp`/`crc`/`prev_crc` (the WAL header's inherently run-specific fields) or the payload's `jobid`/`sub_time`/`queue_time` fields — zero differences anywhere in `term`/`lsn`/`type`/`payload_len` or the remaining ~6485 payload bytes. This was further corroborated by a structural code-diff argument (the entire single-job WAL-encode path is untouched between `895a204` and HEAD, purely-additive diff only) and by the pre-existing task 2.3 unit test asserting the same invariant. Confirmed the three self-flagged residual gaps (task 3.1 bulk-insert orphan prefix, task 4.2 boot-replay fence, task 5.3b bounded leak) are the complete and only set of open items across all 24 completed tasks — no new regression, job loss, divergence, or liveness stall found anywhere. **Overall verdict: GO.**
|
||||
- **`/kiro-validate-impl` feature-level gate (2026-07-16)**: Ran 4 parallel validation dimensions. Requirements coverage: 38 ACs, 33 FULL, 5 PARTIAL (2.3, 8.3, 9.4, 9.5, 9.6 — all tracing to task 3.1's already-known bulk-insert partial-visibility gap plus 8.3's admitted commit-barrier-count inference), 0 outright unclaimed gaps. Design/boundary alignment: ALIGNED, no drift, no boundary violations, all 5 Out-of-Boundary items confirmed untouched, task 5.3b/6.0a's mid-run insertions confirmed surgically scoped to their own narrow fixes. Cross-task integration: SEAMS_SOUND — wire format, `base_jobid + i` arithmetic, `apply_rc` contract, and fence naming all independently verified consistent across every producer/consumer in the chain (5 wire consumers, 3 independent arithmetic implementations in C controller/C scheduler/Go apiserver, 3 tasks sharing the `apply_rc` contract). Mechanical checks: full suite clean, TBD/secrets grep clean, and a genuine live smoke boot (fresh single-node deploy, real `jobsub -a 5` via `xce_test`, exactly one `JOB_ARRAY_SUBMIT` WAL record, all 5 elements individually queryable) — PASS.
|
||||
The requirements-coverage pass sharpened task 3.1's known gap: it is a literal, code-proven violation of Requirement 2.3 and 9.4 (not just a soft nuance) — a bulk-insert-phase resource-exhaustion failure can permanently orphan array elements that exist and are individually addressable but never reach `HOOK_DETAIL_JOB_NEW`, so they are never scheduled or pushed, not on this attempt or any later successful retry of the same LSN. User decision on final GO/NO-GO: **GO, with the gap filed as a tracked bug (`bug-1443`, `.wolf/buglog.json`)** rather than blocking the spec on a fast-follow fix or holding NO-GO — the trigger condition is narrow (a resource-exhaustion failure at the exact moment of the bulk-insert pass) and every other validation dimension came back clean. Closing `bug-1443` requires either a new `key_del`/capacity-reservation primitive in `kv.c`/`kvmgr.c` (shared library, its own small design effort) or a re-derive-and-renotify recovery path — explicitly out of this spec's approved task list, not a quick follow-up task.
|
||||
- **Task 7.1**: Added `kvmgr_reserve()`/`xzarray_reserve()` (new, purely-additive primitives in shared `kvmgr.c`/`zarray.c`, following the existing `kvmgr_capture_keys()` template — bare extern function, no RTTI vtable slot, "caller must hold write lock" contract), wired a reservation call into `restore_job_array()`'s bulk-insert phase, and made a genuinely-inserted partial prefix always get notified (`out_new_jobs` populated + `apply_committed_entry()` fires `HOOK_DETAIL_JOB_NEW`) even when the function still fails overall. **Genuine off-by-one found in the existing, unmodified `xzarray_ensure_items()`**: its sufficiency check is strict less-than (`mincap < array->caps`), so reserving exactly `array_size` and then performing `array_size` real stores still triggers one more growth cycle on the LAST store (`mincap == caps` fails the strict check). Independently re-derived and confirmed by the reviewer with concrete arithmetic: reserving `array_size + 1` (not literally `array_size` as this task's text above says) is exactly the minimal correct compensation — not over- or under-provisioning. This is a deliberate, verified deviation from this task's literal wording, documented at the call site. Confirmed the fix narrows (does not fully eliminate) the residual gap as designed: `kvtab_add()`'s own small per-entry hash-chain-node allocation and a content-based `-EEXIST` collision remain theoretically possible even after reservation — both non-N-scaling and far rarer than the closed failure mode, honestly documented as such. Retry-double-notify risk (does a retry's own `out_new_jobs` ever re-include an already-notified prefix) independently traced and confirmed NOT present — the pre-existing per-index idempotent-skip logic excludes already-inserted indices from any subsequent attempt's `built` list. `apply_wal_entry()`'s dead-code standby path confirmed genuinely untouched (zero behavior change, still passes `NULL` for `out_new_jobs`). New build-architecture finding: `libxce.a` is compiled exactly once without `-DXCE_TESTING` and shared by every consumer, so the new test-support definitions in `zarray.c` had to be left unconditionally compiled (only their declarations in `zarray.h` stay `#ifdef XCE_TESTING`-gated) — confirmed zero production risk (force-fail flag defaults false/inert, no production translation unit can reach the ungated symbols without the gated declaration). One non-blocking coverage gap: no single test composes the full engage→notify-prefix→clear-seam→retry→no-double-notify flow end-to-end (the property is provably true by combining two already-passing tests, but not directly demonstrated in one).
|
||||
|
|
|
|||
|
|
@ -413,13 +413,19 @@ void job_array_apply_test_clear_bulk_insert_force_fail(void)
|
|||
* 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
|
||||
* cluster.c), so a meaningfully large array was near-certain to trigger a
|
||||
* growth cycle here. **Task 7.1 (bug-1443) update**: this specific,
|
||||
* DOMINANT, N-scaling failure trigger is now closed structurally --
|
||||
* kvmgr_reserve(c->jobmgr, array_size) (kvmgr.c/kvmgr.h) pre-grows
|
||||
* c->jobmgr's backing value store for the whole array BEFORE this loop's
|
||||
* first key_add() call, under this SAME write-lock acquisition. This does
|
||||
* NOT make key_add() unconditionally infallible, though: kvtab_add()'s own
|
||||
* separate, tiny per-entry hash-chain-node allocation (kv.c) and a
|
||||
* content-based -EEXIST collision remain theoretically possible -- both are
|
||||
* far rarer and non-N-scaling compared to the growth cycle just closed --
|
||||
* see the residual-atomicity-gap paragraph below for how that narrower case
|
||||
* 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
|
||||
|
|
@ -445,34 +451,43 @@ void job_array_apply_test_clear_bulk_insert_force_fail(void)
|
|||
* 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.
|
||||
* -EEXIST. **Task 7.1 (bug-1443) update**: on a BULK-INSERT-phase negative-
|
||||
* errno return (see below), out_new_jobs is now ALSO populated -- with
|
||||
* whatever genuinely-inserted prefix exists -- rather than left untouched;
|
||||
* it remains untouched only for a SCRATCH-BUILD-phase or reservation
|
||||
* failure, where nothing was ever inserted.
|
||||
*
|
||||
* 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.
|
||||
* the scratch-build-phase failure above; task 7.1/bug-1443 closed the
|
||||
* DOMINANT trigger via kvmgr_reserve() above, but did not, and structurally
|
||||
* cannot, make key_add() unconditionally infallible): if key_add() itself
|
||||
* still fails partway through the bulk-insert pass (kvtab_add()'s own tiny
|
||||
* per-entry allocation, kv.c, or a content-based -EEXIST collision -- both
|
||||
* far rarer and non-N-scaling) (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. **Task 7.1
|
||||
* (bug-1443) fix**: unlike before, the genuinely-inserted prefix [0, k) is
|
||||
* no longer silently un-notified -- it is transferred into *out_new_jobs
|
||||
* (when non-NULL) so the caller (apply_committed_entry()) can still fire
|
||||
* HOOK_DETAIL_JOB_NEW for it, even though the overall call still,
|
||||
* correctly, reports failure so persist_apply_pump_run() halts the LSN and
|
||||
* the Live Apply Resource Fence engages for the un-constructed remainder.
|
||||
*
|
||||
* 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. */
|
||||
* no hook should fire), or a negative errno on a genuine failure. For a
|
||||
* reservation or SCRATCH-BUILD-phase failure, *out_new_jobs is left
|
||||
* untouched -- the scratch list has already been discarded and c->jobmgr
|
||||
* was never mutated 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, and (task 7.1) *out_new_jobs is populated with exactly that
|
||||
* prefix so the caller can still notify for it. */
|
||||
int restore_job_array(cluster_t* c, const void* capnp_payload, size_t len,
|
||||
uint64_t entry_lsn, xlist_t* out_new_jobs)
|
||||
{
|
||||
|
|
@ -613,6 +628,56 @@ int restore_job_array(cluster_t* c, const void* capnp_payload, size_t len,
|
|||
* function doc comment's "Residual atomicity gap" paragraph). Checked
|
||||
* and reported below, never silently claimed as full success. */
|
||||
lock(c->jobmgr, RTTI_LOCK_WRITE);
|
||||
|
||||
/* job-array-single-wal-record task 7.1 (bug-1443): pre-grow c->jobmgr's
|
||||
* backing value store for `array_size` additional entries BEFORE the
|
||||
* first key_add() below can run -- this structurally closes the
|
||||
* DOMINANT, N-scaling failure trigger for this pass (the large
|
||||
* realloc() growing c->jobmgr's value store). array_size (rather than
|
||||
* the possibly-smaller built.count) is a deliberate, always-sufficient
|
||||
* upper bound: built.count <= array_size always (idempotent per-index
|
||||
* skips above only ever shrink it), so this never under-reserves, and
|
||||
* over-reserving by a few slots in the rare partial-skip case is
|
||||
* harmless.
|
||||
*
|
||||
* The `+ 1` headroom is deliberate, not padding: xzarray_ensure_items()
|
||||
* (zarray.c, unmodified) treats capacity as sufficient only when
|
||||
* `mincap < caps` (a STRICT less-than) -- verified directly by tracing
|
||||
* its logic -- so reserving EXACTLY `array_size` still leaves the very
|
||||
* LAST key_add() of a full `array_size`-element bulk-insert pass
|
||||
* hitting `mincap == caps`, which that strict check treats as
|
||||
* insufficient and grows again (by a small, delta-bounded amount, not
|
||||
* an N-scaling one, but still an avoidable realloc() under this write
|
||||
* lock). Reserving one extra slot closes that off-by-one so the
|
||||
* bulk-insert loop below never calls into growth at all when
|
||||
* reservation succeeds.
|
||||
*
|
||||
* Held under the SAME write-lock acquisition as the bulk-insert loop
|
||||
* itself -- not a redundant, separate lock/unlock cycle
|
||||
* (kvmgr_reserve()'s contract, kvmgr.h: caller must hold the kvmgr write
|
||||
* lock for the full duration of the call). This does NOT make
|
||||
* key_add() unconditionally infallible -- see the function doc
|
||||
* comment's "Residual atomicity gap" paragraph for what remains
|
||||
* theoretically possible after this call succeeds. */
|
||||
int reserve_rc = kvmgr_reserve(c->jobmgr, (int)array_size + 1);
|
||||
if (reserve_rc < 0) {
|
||||
unlock(c->jobmgr);
|
||||
|
||||
/* Reservation failure is a pre-mutation failure, exactly like a
|
||||
* scratch-build-phase failure above: no key_add() ever ran, so
|
||||
* c->jobmgr's key->index table was never touched -- there is
|
||||
* nothing to roll back there. Discard every scratch-built element
|
||||
* from THIS attempt and report the failure honestly (Req 9.4). */
|
||||
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 reserve_rc;
|
||||
}
|
||||
|
||||
int inserted = 0;
|
||||
int insert_rc = 0;
|
||||
for (int k = 0; k < built.count; k++) {
|
||||
|
|
@ -650,7 +715,26 @@ int restore_job_array(cluster_t* c, const void* capnp_payload, size_t len,
|
|||
if (j != NULL)
|
||||
dereference(j);
|
||||
}
|
||||
|
||||
/* job-array-single-wal-record task 7.1 (bug-1443 partial-prefix
|
||||
* fix): indices [0, inserted) are genuinely, permanently in
|
||||
* c->jobmgr -- they must never be silently un-notified. Transfer
|
||||
* exactly that prefix into *out_new_jobs (when the caller wants it)
|
||||
* so apply_committed_entry() can still fire HOOK_DETAIL_JOB_NEW for
|
||||
* it, even though this call still, correctly, reports failure via a
|
||||
* negative rc so the caller halts/fences for the un-constructed
|
||||
* remainder (Req 2.3, 9.4, 9.6). Truncating built.count to
|
||||
* `inserted` before the transfer is what excludes the already-
|
||||
* released [inserted, built.count) tail above from the transferred
|
||||
* list -- xlist_fini() only ever frees the list's own backing
|
||||
* array, never walks its elements, so this truncation is the only
|
||||
* step needed to make the transfer safe. */
|
||||
if (out_new_jobs != NULL && inserted > 0) {
|
||||
built.count = inserted;
|
||||
*out_new_jobs = built;
|
||||
} else {
|
||||
xlist_fini(&built);
|
||||
}
|
||||
destroy(scratch);
|
||||
return insert_rc;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -2820,16 +2820,44 @@ int apply_committed_entry(persist_t* p, uint32_t wal_type, uint64_t lsn,
|
|||
* and only, what the hook fire below carries — Req 3.4); -EEXIST =
|
||||
* every index in the record already existed (idempotent
|
||||
* whole-record no-op, Req 3.3) — no hook; a negative errno = a
|
||||
* genuine construction/resource-exhaustion failure (Req 9.4) — no
|
||||
* hook, propagated so persist_apply_pump_run() (task 4.1) can halt
|
||||
* and fence on it rather than silently skipping it. */
|
||||
* genuine construction/resource-exhaustion failure (Req 9.4) —
|
||||
* propagated so persist_apply_pump_run() (task 4.1) can halt and
|
||||
* fence on it rather than silently skipping it. **Task 7.1
|
||||
* (bug-1443) update**: a negative return no longer unconditionally
|
||||
* means "no hook" — restore_job_array()'s bulk-insert-phase residual
|
||||
* gap can leave a genuinely, permanently inserted prefix in
|
||||
* out_new_jobs even on failure (job.c's doc comment, "Residual
|
||||
* atomicity gap"); see below. */
|
||||
xlist_t out_new_jobs;
|
||||
memset(&out_new_jobs, 0, sizeof(out_new_jobs));
|
||||
int rc = restore_job_array(c, payload, len, lsn, &out_new_jobs);
|
||||
if (rc == -EEXIST)
|
||||
return 0; /* idempotent re-apply (replay / resync) — no hook */
|
||||
if (rc < 0)
|
||||
|
||||
if (rc < 0) {
|
||||
/* job-array-single-wal-record task 7.1 (bug-1443 partial-prefix
|
||||
* fix): out_new_jobs is untouched (count == 0, from the memset
|
||||
* above) for a reservation or scratch-build-phase failure —
|
||||
* nothing was ever inserted, so no hook fires, unchanged from
|
||||
* before this task. But for the narrower bulk-insert-phase
|
||||
* residual gap, out_new_jobs now carries a non-empty, genuinely
|
||||
* and permanently inserted prefix (job.c) that must still get
|
||||
* its one HOOK_DETAIL_JOB_NEW fire — a job that is really in
|
||||
* c->jobmgr must never be silently un-notified. The overall
|
||||
* apply is still, correctly, reported as failed via the
|
||||
* negative rc returned below, so persist_apply_pump_run() halts
|
||||
* the LSN and the Live Apply Resource Fence engages for the
|
||||
* un-constructed remainder (Req 2.3, 9.4, 9.6). */
|
||||
if (out_new_jobs.count > 0) {
|
||||
t_apply_seam_notifying = true;
|
||||
xce_run_hooks(&c->hooksys, XCE_CONTROLLER_HOOK_JOB,
|
||||
HOOK_AFTER_EVENT, HOOK_DETAIL_JOB_NEW, c,
|
||||
&out_new_jobs);
|
||||
t_apply_seam_notifying = false;
|
||||
}
|
||||
xlist_fini(&out_new_jobs);
|
||||
return rc;
|
||||
}
|
||||
|
||||
/* Exactly ONE fire carrying every newly created element in one
|
||||
* list — never once per element (Req 3.4). out_new_jobs is already
|
||||
|
|
|
|||
|
|
@ -662,6 +662,70 @@ static void test_ace_job_array_submit_forced_failure_fires_no_hook(void)
|
|||
tace_teardown();
|
||||
printf("test_ace_job_array_submit_forced_failure_fires_no_hook: PASS\n");
|
||||
}
|
||||
|
||||
/* ── job-array-single-wal-record task 7.1 (bug-1443 partial-prefix fix):
|
||||
* a bulk-insert-phase failure (the narrower residual gap, distinct from the
|
||||
* scratch-build-phase failure above) must STILL fire HOOK_DETAIL_JOB_NEW --
|
||||
* for exactly the genuinely-inserted prefix -- while still returning the
|
||||
* negative rc so persist_apply_pump_run() halts/fences for the remainder.
|
||||
* Before this task, this exact scenario fired NO hook at all (see
|
||||
* test_ace_job_array_submit_forced_failure_fires_no_hook above, which forces
|
||||
* the OTHER, scratch-build-phase seam and correctly still expects no hook --
|
||||
* that one never inserts anything real, so no hook is correct there; this
|
||||
* test's bulk-insert-phase seam DOES insert a real, permanent prefix, so a
|
||||
* hook is now required). */
|
||||
static void test_ace_job_array_submit_bulk_insert_failure_fires_hook_for_prefix(void)
|
||||
{
|
||||
tace_setup();
|
||||
|
||||
const uint64_t base_jobid = 5400;
|
||||
const uint64_t array_size = 5;
|
||||
job_submit_data_t spec = tace_array_make_spec();
|
||||
uint8_t* payload = NULL;
|
||||
size_t plen = 0;
|
||||
|
||||
TEST_ASSERT_EQUAL_INT(0, tace_encode_job_submit_array(base_jobid,
|
||||
array_size, &spec,
|
||||
&payload, &plen));
|
||||
|
||||
xce_add_hook(&g_tace_c.hooksys, XCE_CONTROLLER_HOOK_JOB,
|
||||
tace_array_spy_hook, NULL);
|
||||
|
||||
/* Force the bulk-insert pass (NOT the scratch-build pass) to fail at
|
||||
* index 2 -- indices 0 and 1 are genuinely, permanently inserted into
|
||||
* c->jobmgr via restore_job_array()'s real key_add() calls before this
|
||||
* fires. */
|
||||
job_array_apply_test_set_bulk_insert_force_fail_at(2);
|
||||
|
||||
int rc = apply_committed_entry(&g_tace_p, WAL_JOB_ARRAY_SUBMIT, 63,
|
||||
payload, (uint32_t)plen);
|
||||
TEST_ASSERT_TRUE_MESSAGE(rc < 0,
|
||||
"a bulk-insert-phase failure must still propagate as a negative"
|
||||
" errno so the caller halts/fences the LSN");
|
||||
|
||||
TEST_ASSERT_EQUAL_INT_MESSAGE(1, g_tace_hook_fires,
|
||||
"the genuinely-inserted prefix must still get exactly ONE"
|
||||
" HOOK_DETAIL_JOB_NEW fire -- task 7.1's bug-1443 partial-prefix fix"
|
||||
" -- even though the overall call reports failure");
|
||||
TEST_ASSERT_EQUAL_INT_MESSAGE(2, g_tace_last_new_list_count,
|
||||
"the fire must carry exactly the 2 genuinely-inserted elements, not"
|
||||
" the full array_size and not zero");
|
||||
|
||||
for (uint64_t i = 0; i < 2; i++) {
|
||||
TEST_ASSERT_NOT_NULL_MESSAGE(tace_find_job(base_jobid + i),
|
||||
"the genuinely-inserted prefix must be visible in c->jobmgr");
|
||||
}
|
||||
for (uint64_t i = 2; i < array_size; i++) {
|
||||
TEST_ASSERT_NULL_MESSAGE(tace_find_job(base_jobid + i),
|
||||
"indices never reached by key_add() must not be visible");
|
||||
}
|
||||
|
||||
job_array_apply_test_clear_bulk_insert_force_fail();
|
||||
free(payload);
|
||||
tace_teardown();
|
||||
printf("test_ace_job_array_submit_bulk_insert_failure_fires_hook_for_"
|
||||
"prefix: PASS\n");
|
||||
}
|
||||
#endif /* XCE_TESTING */
|
||||
|
||||
/* ── Defensive / boundary ─────────────────────────────────────────────── */
|
||||
|
|
@ -688,6 +752,7 @@ void test_apply_committed_entry(void)
|
|||
RUN_TEST(test_ace_job_array_submit_idempotent_replay_no_extra_notify);
|
||||
#ifdef XCE_TESTING
|
||||
RUN_TEST(test_ace_job_array_submit_forced_failure_fires_no_hook);
|
||||
RUN_TEST(test_ace_job_array_submit_bulk_insert_failure_fires_hook_for_prefix);
|
||||
#endif
|
||||
RUN_TEST(test_ace_null_persist_returns_error);
|
||||
UNITY_END();
|
||||
|
|
|
|||
|
|
@ -65,6 +65,7 @@
|
|||
#include "xce/types.h"
|
||||
#include "xlist.h"
|
||||
#include "xstring.h"
|
||||
#include "zarray.h"
|
||||
|
||||
static int g_taj_jobobj_ready;
|
||||
static int g_taj_xstring_ready;
|
||||
|
|
@ -92,6 +93,7 @@ static void taj_setup(void)
|
|||
#ifdef XCE_TESTING
|
||||
job_array_apply_test_clear_force_fail();
|
||||
job_array_apply_test_clear_bulk_insert_force_fail();
|
||||
xzarray_reserve_test_clear_force_fail();
|
||||
#endif
|
||||
}
|
||||
|
||||
|
|
@ -100,6 +102,7 @@ static void taj_teardown(void)
|
|||
#ifdef XCE_TESTING
|
||||
job_array_apply_test_clear_force_fail();
|
||||
job_array_apply_test_clear_bulk_insert_force_fail();
|
||||
xzarray_reserve_test_clear_force_fail();
|
||||
#endif
|
||||
/* Mirrors test_apply_committed_entry.c's tace_teardown: the jobmgr's own
|
||||
* datakiller is unset, so the job_t values it still holds are not
|
||||
|
|
@ -532,6 +535,142 @@ static void test_rja_bulk_insert_failure_reports_error_not_false_success(void)
|
|||
taj_teardown();
|
||||
printf("test_rja_bulk_insert_failure_reports_error_not_false_success: PASS\n");
|
||||
}
|
||||
|
||||
/* ── job-array-single-wal-record task 7.1 (bug-1443): restore_job_array()
|
||||
* now calls kvmgr_reserve() BEFORE the bulk-insert pass begins ──────────────
|
||||
* Proven by forcing the underlying xzarray_reserve() primitive itself to
|
||||
* fail (the same seam test_zarray_reserve.c/test_kvmgr_reserve.c use in
|
||||
* isolation, reused here at the job.c integration level): if
|
||||
* restore_job_array() genuinely calls kvmgr_reserve() and gates the
|
||||
* bulk-insert loop on its result, NO element of this attempt can ever reach
|
||||
* key_add() -- c->jobmgr must remain completely untouched, and the function
|
||||
* must report the reservation failure honestly (negative rc), exactly like
|
||||
* the OTHER existing pre-mutation failure paths in this function. If the
|
||||
* call were missing, or placed after the loop starts, at least the first
|
||||
* element(s) would have already been key_add()'d for real before this forced
|
||||
* failure could take effect -- this test would then observe a non-empty
|
||||
* partial prefix instead of nothing. */
|
||||
static void test_rja_kvmgr_reserve_gates_bulk_insert_pass(void)
|
||||
{
|
||||
taj_setup();
|
||||
|
||||
const uint64_t base_jobid = 77000;
|
||||
const uint64_t array_size = 5;
|
||||
job_submit_data_t spec = taj_make_spec();
|
||||
uint8_t* payload = NULL;
|
||||
size_t len = 0;
|
||||
|
||||
TEST_ASSERT_EQUAL_INT(0,
|
||||
taj_encode_job_submit(base_jobid, array_size, &spec, &payload, &len));
|
||||
|
||||
xzarray_reserve_test_set_force_fail();
|
||||
|
||||
xlist_t out;
|
||||
memset(&out, 0, sizeof(out));
|
||||
int rc = restore_job_array(&g_taj_c, payload, len, 70, &out);
|
||||
|
||||
TEST_ASSERT_TRUE_MESSAGE(rc < 0,
|
||||
"a forced reservation failure must be reported as a negative rc,"
|
||||
" treated exactly like the other pre-mutation failure paths");
|
||||
|
||||
for (uint64_t i = 0; i < array_size; i++) {
|
||||
TEST_ASSERT_NULL_MESSAGE(taj_find_job(base_jobid + i),
|
||||
"kvmgr_reserve() must be called and checked BEFORE the first"
|
||||
" key_add() of the bulk-insert pass -- a forced reservation"
|
||||
" failure must leave EVERY index of this attempt untouched, not"
|
||||
" just some");
|
||||
}
|
||||
|
||||
/* Clear the seam and retry: the identical record must now succeed
|
||||
* fully, proving the earlier reservation failure did not leave any
|
||||
* poisoned state behind (c->jobmgr really was never touched). */
|
||||
xzarray_reserve_test_clear_force_fail();
|
||||
|
||||
xlist_t out2;
|
||||
memset(&out2, 0, sizeof(out2));
|
||||
int rc2 = restore_job_array(&g_taj_c, payload, len, 71, &out2);
|
||||
TEST_ASSERT_EQUAL_INT_MESSAGE((int)array_size, rc2,
|
||||
"a retry after the reservation seam clears must construct every"
|
||||
" element, proving the earlier failure left c->jobmgr genuinely"
|
||||
" untouched");
|
||||
|
||||
xlist_fini(&out2);
|
||||
free(payload);
|
||||
taj_teardown();
|
||||
printf("test_rja_kvmgr_reserve_gates_bulk_insert_pass: PASS\n");
|
||||
}
|
||||
|
||||
/* ── job-array-single-wal-record task 7.1 (bug-1443 partial-prefix fix):
|
||||
* a bulk-insert-phase failure (despite a SUCCESSFUL reservation) must still
|
||||
* populate out_new_jobs with the genuinely-inserted prefix ─────────────────
|
||||
* Reuses task 3.1's existing bulk-insert-phase fault-injection seam
|
||||
* unmodified -- job_array_apply_test_set_bulk_insert_force_fail_at()
|
||||
* simulates key_add() itself failing at a chosen index REGARDLESS of
|
||||
* capacity state, so it exercises the exact narrow residual case task 7.1
|
||||
* targets (key_add() still fails even though reservation succeeded), with
|
||||
* no seam extension needed. Before task 7.1, out_new_jobs was left
|
||||
* completely untouched (empty) on this failure path -- see
|
||||
* test_rja_bulk_insert_failure_reports_error_not_false_success above, which
|
||||
* never inspects `out` after a failed call for exactly that reason. This
|
||||
* test proves out_new_jobs now carries indices [0, k) -- the genuinely,
|
||||
* permanently inserted prefix -- so a caller (apply_committed_entry(), see
|
||||
* test_apply_committed_entry.c) can still notify for it. */
|
||||
static void test_rja_bulk_insert_failure_populates_out_new_jobs_with_prefix(void)
|
||||
{
|
||||
taj_setup();
|
||||
|
||||
const uint64_t base_jobid = 76500;
|
||||
const uint64_t array_size = 5;
|
||||
job_submit_data_t spec = taj_make_spec();
|
||||
uint8_t* payload = NULL;
|
||||
size_t len = 0;
|
||||
|
||||
TEST_ASSERT_EQUAL_INT(0,
|
||||
taj_encode_job_submit(base_jobid, array_size, &spec, &payload, &len));
|
||||
|
||||
/* Force the bulk-insert pass to fail inserting the 3rd element (index 2
|
||||
* of built.count) -- indices 0 and 1 are genuinely, permanently
|
||||
* key_add()'d into c->jobmgr before this fires (reservation, task 7.1,
|
||||
* already succeeded and is irrelevant to this simulated failure). */
|
||||
job_array_apply_test_set_bulk_insert_force_fail_at(2);
|
||||
|
||||
xlist_t out;
|
||||
memset(&out, 0, sizeof(out));
|
||||
int rc = restore_job_array(&g_taj_c, payload, len, 55, &out);
|
||||
|
||||
TEST_ASSERT_TRUE_MESSAGE(rc < 0,
|
||||
"a bulk-insert-phase failure must still be reported as a negative"
|
||||
" rc, even though a genuine prefix was inserted");
|
||||
|
||||
TEST_ASSERT_EQUAL_INT_MESSAGE(2, out.count,
|
||||
"out_new_jobs must now carry exactly the genuinely-inserted prefix"
|
||||
" [0, k) -- task 7.1's bug-1443 partial-prefix fix -- instead of"
|
||||
" being left empty/untouched as before this task");
|
||||
|
||||
for (int k = 0; k < out.count; k++) {
|
||||
job_t* j = (job_t*)xlist_get(&out, k);
|
||||
TEST_ASSERT_NOT_NULL(j);
|
||||
TEST_ASSERT_EQUAL_UINT64_MESSAGE(base_jobid + (uint64_t)k, j->job.jobid,
|
||||
"the transferred prefix must be in index order, matching the"
|
||||
" genuinely-inserted indices");
|
||||
}
|
||||
|
||||
/* Cross-check against direct jobmgr lookups, mirroring the existing
|
||||
* bulk-insert-failure test's own residual-gap assertions. */
|
||||
for (uint64_t i = 0; i < 2; i++) {
|
||||
TEST_ASSERT_NOT_NULL(taj_find_job(base_jobid + i));
|
||||
}
|
||||
for (uint64_t i = 2; i < array_size; i++) {
|
||||
TEST_ASSERT_NULL(taj_find_job(base_jobid + i));
|
||||
}
|
||||
|
||||
xlist_fini(&out);
|
||||
free(payload);
|
||||
job_array_apply_test_clear_bulk_insert_force_fail();
|
||||
taj_teardown();
|
||||
printf("test_rja_bulk_insert_failure_populates_out_new_jobs_with_prefix: "
|
||||
"PASS\n");
|
||||
}
|
||||
#endif /* XCE_TESTING */
|
||||
|
||||
/* ── Defensive / boundary ─────────────────────────────────────────────── */
|
||||
|
|
@ -868,6 +1007,8 @@ void test_job_array_apply(void)
|
|||
#ifdef XCE_TESTING
|
||||
RUN_TEST(test_rja_forced_failure_leaves_nothing_visible_then_retry_succeeds);
|
||||
RUN_TEST(test_rja_bulk_insert_failure_reports_error_not_false_success);
|
||||
RUN_TEST(test_rja_kvmgr_reserve_gates_bulk_insert_pass);
|
||||
RUN_TEST(test_rja_bulk_insert_failure_populates_out_new_jobs_with_prefix);
|
||||
#endif
|
||||
RUN_TEST(test_rja_null_or_empty_payload_returns_error);
|
||||
RUN_TEST(test_rja_out_new_jobs_is_optional);
|
||||
|
|
|
|||
|
|
@ -41,4 +41,30 @@ int kvmgr_capture_keys(var kvmgr,
|
|||
slot_key_t** keys_out,
|
||||
size_t* n_keys_out);
|
||||
|
||||
/*
|
||||
* kvmgr_reserve — job-array-single-wal-record task 7.1 (bug-1443).
|
||||
*
|
||||
* Pre-grows kvmgr's backing value store (the internal xzarray_type
|
||||
* instance) for `n` additional entries, via the new xzarray_reserve()
|
||||
* primitive (zarray.h/zarray.c) -- zero changes to any existing function in
|
||||
* either file. Closes the DOMINANT, N-scaling failure mode for a bulk
|
||||
* key_add() loop (e.g. restore_job_array()'s bulk-insert pass, job.c): the
|
||||
* large realloc() reachable through key_add -> xkvmgr_add -> vstore ->
|
||||
* xzarray_ensure_items when the value store must grow.
|
||||
*
|
||||
* This narrows, but does NOT eliminate, that failure surface: key_add()'s
|
||||
* own kvtab_add() call (kv.c) still has its own separate, tiny per-entry
|
||||
* hash-chain-node allocation, and a content-based -EEXIST collision remains
|
||||
* theoretically possible -- both are far rarer and non-N-scaling compared
|
||||
* to the growth cycle this call structurally avoids.
|
||||
*
|
||||
* Caller MUST hold the kvmgr write lock for the full duration of this call
|
||||
* -- identical contract to kvmgr_capture_keys() above.
|
||||
*
|
||||
* Returns 1 on success (capacity already sufficient, or successfully
|
||||
* grown). Returns -EINVAL for a NULL kvmgr or negative n. Returns -ENOMEM
|
||||
* if the underlying growth allocation fails.
|
||||
*/
|
||||
int kvmgr_reserve(var kvmgr, int n);
|
||||
|
||||
#endif
|
||||
|
|
|
|||
|
|
@ -43,4 +43,62 @@ typedef struct {
|
|||
|
||||
extern typeinfo_t xzarray_type;
|
||||
|
||||
/* job-array-single-wal-record task 7.1 (bug-1443): pre-grows an xzarray_type
|
||||
* instance's backing value store for `n` additional entries, by calling the
|
||||
* existing, unmodified xzarray_ensure_items(a, a->count + n) internally --
|
||||
* zero changes to xzarray_ensure_items or any other existing function in
|
||||
* this file. This is the primitive kvmgr_reserve() (kvmgr.c/kvmgr.h) uses to
|
||||
* pre-grow c->jobmgr's backing xzarray_type instance (kvm->data) before a
|
||||
* bulk-insert pass, closing the DOMINANT, N-scaling failure mode for that
|
||||
* pass (the large realloc() reachable through vstore ->
|
||||
* xzarray_ensure_items). It does NOT make every subsequent vstore() call
|
||||
* unconditionally infallible: this only covers the value-store side --
|
||||
* xkvmgr_type's separate kvtab key->index table (kv.c) has its own,
|
||||
* independent, much smaller, non-N-scaling per-entry allocation that this
|
||||
* function does not and cannot pre-reserve.
|
||||
*
|
||||
* Caller MUST hold the same write lock the subsequent bulk vstore()/
|
||||
* key_add() calls will be made under, for the full duration of this call --
|
||||
* identical contract to kvmgr_capture_keys() (kvmgr.h).
|
||||
*
|
||||
* Returns 1 on success (capacity already sufficient, or successfully
|
||||
* grown). Returns -EINVAL for a NULL array or negative n. Returns -ENOMEM if
|
||||
* the underlying growth allocation fails. */
|
||||
extern int xzarray_reserve(var array, int n);
|
||||
|
||||
#ifdef XCE_TESTING
|
||||
/* Test-only fault injection for xzarray_reserve() (job-array-single-wal-
|
||||
* record task 7.1 / bug-1443). When engaged, EVERY subsequent
|
||||
* xzarray_reserve() call -- across any xzarray_type instance, including one
|
||||
* wrapped inside an xkvmgr_type via kvmgr_reserve() -- reports a simulated
|
||||
* allocation failure (-ENOMEM) without touching the real array. File-scope,
|
||||
* not per-instance: mirrors job.c's own
|
||||
* job_array_apply_test_set_bulk_insert_force_fail_at() convention (this
|
||||
* feature's own established #ifdef XCE_TESTING test-only-symbol style).
|
||||
*
|
||||
* NOTE on why these declarations, unlike job.c's own #ifdef XCE_TESTING
|
||||
* seams, are guarded here but their DEFINITIONS (zarray.c) are NOT: zarray.c
|
||||
* is compiled exactly once, as part of the shared `libxce` static_library()
|
||||
* target (src/lib/meson.build), without -DXCE_TESTING -- unlike job.c, which
|
||||
* every consumer (including test binaries) recompiles from source with its
|
||||
* own c_args. Guarding the .c definitions the same way would silently drop
|
||||
* them from libxce.a in every build, including test builds that need them
|
||||
* (confirmed empirically). So the underlying symbols are unconditionally
|
||||
* present in the compiled library; only these DECLARATIONS are gated, which
|
||||
* keeps them un-prototyped (hence uncallable without a compiler error) from
|
||||
* any translation unit that does not itself define XCE_TESTING -- i.e. every
|
||||
* production source file. */
|
||||
extern void xzarray_reserve_test_set_force_fail(void);
|
||||
extern void xzarray_reserve_test_clear_force_fail(void);
|
||||
|
||||
/* Test-only introspection: returns the CURRENT backing-store capacity (item
|
||||
* slots, not bytes) of an xzarray_type instance. xzarray_t is fully opaque
|
||||
* outside zarray.c, so this is the only way a test can observe whether a
|
||||
* reservation actually grew capacity ahead of need, as opposed to deferring
|
||||
* growth to the next vstore() call. Returns -EINVAL for a NULL array. See
|
||||
* the NOTE above -- declaration-gated, not definition-gated, for the same
|
||||
* shared-library build reason. */
|
||||
extern int xzarray_test_get_caps(var array);
|
||||
#endif
|
||||
|
||||
#endif
|
||||
|
|
|
|||
|
|
@ -410,3 +410,27 @@ int kvmgr_capture_keys(var kvmgr_var,
|
|||
*n_keys_out = ctx.filled;
|
||||
return 0;
|
||||
}
|
||||
|
||||
/* ------------------------------------------------------------------ */
|
||||
/* kvmgr_reserve — job-array-single-wal-record task 7.1 (bug-1443) */
|
||||
/* ------------------------------------------------------------------ */
|
||||
|
||||
int kvmgr_reserve(var kvmgr_var, int n)
|
||||
{
|
||||
if ((kvmgr_var == NULL) || (n < 0)) {
|
||||
return -EINVAL;
|
||||
}
|
||||
|
||||
/* Same public-pointer -> meta_obj_t -> xkvmgr_t unwrap as
|
||||
* kvmgr_capture_keys() above. */
|
||||
meta_obj_t* obj = RTTI_DATA_TO_OBJ(kvmgr_var);
|
||||
xkvmgr_t* kvm = RTTI_OBJ_TO_DATA(xkvmgr_t, obj);
|
||||
|
||||
/* Pre-grow the value store ONLY -- this is the piece xkvmgr_add()
|
||||
* mutates first (vstore(kvm->data, ...), see xkvmgr_add() above) and
|
||||
* the only piece with an N-scaling realloc() reachable from a bulk
|
||||
* key_add() loop. kvm->kvtab's own per-entry hash-chain-node allocation
|
||||
* (kv.c) is deliberately NOT covered here -- see this function's doc
|
||||
* comment (kvmgr.h) for why. */
|
||||
return xzarray_reserve(kvm->data, n);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -169,6 +169,8 @@ libxce_test_src = ['testsuite/test.c',
|
|||
'testsuite/test_request.c',
|
||||
'testsuite/test_zarray_cow.c',
|
||||
'testsuite/test_kvmgr_capture.c',
|
||||
'testsuite/test_zarray_reserve.c',
|
||||
'testsuite/test_kvmgr_reserve.c',
|
||||
'testsuite/test_hooks.c',
|
||||
'testsuite/test_cluster.c',
|
||||
'testsuite/test_cluster_parse.c',
|
||||
|
|
|
|||
|
|
@ -32,6 +32,8 @@ extern void test_wobj(void);
|
|||
extern void test_request(void);
|
||||
extern void test_zarray_cow(void);
|
||||
extern void test_kvmgr_capture(void);
|
||||
extern void test_zarray_reserve(void);
|
||||
extern void test_kvmgr_reserve(void);
|
||||
extern void test_hooks(void);
|
||||
extern void test_cluster(void);
|
||||
extern void test_cluster_parse(void);
|
||||
|
|
@ -147,6 +149,8 @@ int main(int argc, char* argv[])
|
|||
TEST(test_request);
|
||||
TEST(test_zarray_cow);
|
||||
TEST(test_kvmgr_capture);
|
||||
TEST(test_zarray_reserve);
|
||||
TEST(test_kvmgr_reserve);
|
||||
TEST(test_hooks);
|
||||
TEST(test_cluster);
|
||||
TEST(test_cluster_parse);
|
||||
|
|
|
|||
136
src/lib/testsuite/test_kvmgr_reserve.c
Normal file
136
src/lib/testsuite/test_kvmgr_reserve.c
Normal file
|
|
@ -0,0 +1,136 @@
|
|||
/* test_kvmgr_reserve.c — job-array-single-wal-record task 7.1 (bug-1443)
|
||||
*
|
||||
* Unit tests for kvmgr_reserve() (kvmgr.c/kvmgr.h) -- the thin, kvmgr-level
|
||||
* forwarding wrapper around xzarray_reserve() (zarray.c, see
|
||||
* test_zarray_reserve.c for the rigorous capacity-growth proof at that
|
||||
* lower level). This file proves kvmgr_reserve() itself:
|
||||
* - correctly unwraps an xkvmgr_type instance and forwards to its
|
||||
* internal value store, allowing a subsequent bulk key_add() sequence
|
||||
* up to the reserved count to succeed;
|
||||
* - propagates a simulated allocation failure (via the SAME
|
||||
* xzarray_reserve_test_set_force_fail() seam kvmgr_reserve() forwards
|
||||
* into -- there is no separate kvmgr-level seam, deliberately, since
|
||||
* kvmgr_reserve() has no logic of its own beyond the forward);
|
||||
* - rejects a NULL kvmgr / negative n defensively.
|
||||
*/
|
||||
|
||||
#include <errno.h>
|
||||
#include <rtti.h>
|
||||
#include <stdio.h>
|
||||
#include <stdlib.h>
|
||||
|
||||
#include <unity/unity.h>
|
||||
|
||||
#include "kv.h"
|
||||
#include "kvmgr.h"
|
||||
#include "zarray.h"
|
||||
|
||||
static var make_test_kvmgr(uint32_t nslots, uint32_t delta)
|
||||
{
|
||||
kvmgr_params_t params = { .key_type = KVMGR_KEY_INT,
|
||||
.nslots = nslots,
|
||||
.delta = delta,
|
||||
.datakiller = NULL };
|
||||
return makeInstance(&xkvmgr_type, ¶ms);
|
||||
}
|
||||
|
||||
/* ── kvmgr_reserve() allows a subsequent bulk key_add() sequence up to the
|
||||
* reserved count to succeed (functional forwarding proof) ──────────────── */
|
||||
|
||||
static void test_kvmgr_reserve_allows_subsequent_bulk_key_add(void)
|
||||
{
|
||||
const int n = 30;
|
||||
|
||||
/* Tiny initial capacity/delta -- without reservation this would force
|
||||
* many incremental growth cycles across the loop below. */
|
||||
var kvmgr = make_test_kvmgr(2, 1);
|
||||
TEST_ASSERT_NOT_NULL(kvmgr);
|
||||
|
||||
lock(kvmgr, RTTI_LOCK_WRITE);
|
||||
int reserve_rc = kvmgr_reserve(kvmgr, n + 1);
|
||||
TEST_ASSERT_TRUE_MESSAGE(reserve_rc >= 0,
|
||||
"kvmgr_reserve must succeed for a valid kvmgr and count");
|
||||
|
||||
for (int i = 0; i < n; i++) {
|
||||
int idx = key_add(kvmgr, (void*)(long)(1000 + i), (void*)(long)i);
|
||||
TEST_ASSERT_TRUE_MESSAGE(idx >= 0,
|
||||
"every key_add within the reserved count must succeed");
|
||||
}
|
||||
unlock(kvmgr);
|
||||
|
||||
/* Spot-check a few entries resolve correctly. */
|
||||
void* found = NULL;
|
||||
TEST_ASSERT_TRUE(key_find(kvmgr, (void*)(long)1000, &found) >= 0);
|
||||
TEST_ASSERT_EQUAL_INT(0, (int)(long)found);
|
||||
TEST_ASSERT_TRUE(key_find(kvmgr, (void*)(long)1029, &found) >= 0);
|
||||
TEST_ASSERT_EQUAL_INT(29, (int)(long)found);
|
||||
|
||||
destroy(kvmgr);
|
||||
printf("test_kvmgr_reserve_allows_subsequent_bulk_key_add: PASS\n");
|
||||
}
|
||||
|
||||
/* ── Simulated allocation failure propagates through kvmgr_reserve() too --
|
||||
* reusing the SAME xzarray_reserve()-level seam, since kvmgr_reserve() is a
|
||||
* pure forward with no logic of its own. ───────────────────────────────── */
|
||||
|
||||
static void test_kvmgr_reserve_propagates_simulated_allocation_failure(void)
|
||||
{
|
||||
var kvmgr = make_test_kvmgr(4, 4);
|
||||
TEST_ASSERT_NOT_NULL(kvmgr);
|
||||
|
||||
xzarray_reserve_test_set_force_fail();
|
||||
|
||||
lock(kvmgr, RTTI_LOCK_WRITE);
|
||||
int rc = kvmgr_reserve(kvmgr, 10);
|
||||
unlock(kvmgr);
|
||||
|
||||
TEST_ASSERT_EQUAL_INT_MESSAGE(-ENOMEM, rc,
|
||||
"a forced allocation failure must propagate as -ENOMEM through"
|
||||
" kvmgr_reserve()");
|
||||
|
||||
xzarray_reserve_test_clear_force_fail();
|
||||
|
||||
lock(kvmgr, RTTI_LOCK_WRITE);
|
||||
int rc2 = kvmgr_reserve(kvmgr, 10);
|
||||
unlock(kvmgr);
|
||||
TEST_ASSERT_TRUE_MESSAGE(rc2 >= 0,
|
||||
"clearing the force-fail seam must restore normal success");
|
||||
|
||||
destroy(kvmgr);
|
||||
printf("test_kvmgr_reserve_propagates_simulated_allocation_failure: "
|
||||
"PASS\n");
|
||||
}
|
||||
|
||||
/* ── Defensive / boundary ─────────────────────────────────────────────── */
|
||||
|
||||
static void test_kvmgr_reserve_rejects_null_kvmgr(void)
|
||||
{
|
||||
int rc = kvmgr_reserve(NULL, 5);
|
||||
TEST_ASSERT_TRUE(rc < 0);
|
||||
printf("test_kvmgr_reserve_rejects_null_kvmgr: PASS\n");
|
||||
}
|
||||
|
||||
static void test_kvmgr_reserve_rejects_negative_n(void)
|
||||
{
|
||||
var kvmgr = make_test_kvmgr(4, 4);
|
||||
TEST_ASSERT_NOT_NULL(kvmgr);
|
||||
|
||||
lock(kvmgr, RTTI_LOCK_WRITE);
|
||||
int rc = kvmgr_reserve(kvmgr, -1);
|
||||
unlock(kvmgr);
|
||||
TEST_ASSERT_TRUE(rc < 0);
|
||||
|
||||
destroy(kvmgr);
|
||||
printf("test_kvmgr_reserve_rejects_negative_n: PASS\n");
|
||||
}
|
||||
|
||||
/* ── Test group entry point ───────────────────────────────────────────── */
|
||||
|
||||
void test_kvmgr_reserve(void)
|
||||
{
|
||||
printf("\n>> kvmgr_reserve test\n");
|
||||
RUN_TEST(test_kvmgr_reserve_allows_subsequent_bulk_key_add);
|
||||
RUN_TEST(test_kvmgr_reserve_propagates_simulated_allocation_failure);
|
||||
RUN_TEST(test_kvmgr_reserve_rejects_null_kvmgr);
|
||||
RUN_TEST(test_kvmgr_reserve_rejects_negative_n);
|
||||
}
|
||||
236
src/lib/testsuite/test_zarray_reserve.c
Normal file
236
src/lib/testsuite/test_zarray_reserve.c
Normal file
|
|
@ -0,0 +1,236 @@
|
|||
/* test_zarray_reserve.c — job-array-single-wal-record task 7.1 (bug-1443)
|
||||
*
|
||||
* Unit tests for xzarray_reserve() (zarray.c), the new capacity-reservation
|
||||
* primitive kvmgr_reserve() (kvmgr.c/kvmgr.h) uses to pre-grow an
|
||||
* xkvmgr_type instance's backing value store before a bulk-insert pass
|
||||
* (e.g. restore_job_array()'s bulk-insert loop, job.c) -- closing the
|
||||
* DOMINANT, N-scaling failure trigger for that pass (the large realloc()
|
||||
* reachable through vstore -> xzarray_ensure_items).
|
||||
*
|
||||
* xzarray_t is fully opaque outside zarray.c, so these tests use the two
|
||||
* new XCE_TESTING-guarded introspection/fault-injection primitives added
|
||||
* alongside xzarray_reserve() itself:
|
||||
* - xzarray_test_get_caps() -- read current backing capacity
|
||||
* - xzarray_reserve_test_set/clear_force_fail() -- simulate an allocation
|
||||
* failure without needing genuine OOM
|
||||
*
|
||||
* Covers:
|
||||
* - xzarray_reserve() genuinely pre-grows capacity: reserving N+1 items
|
||||
* up front means a subsequent sequence of N real vstore() calls causes
|
||||
* NO further capacity growth at all (proven via xzarray_test_get_caps()
|
||||
* staying constant across every store).
|
||||
*
|
||||
* The "+1" is not incidental -- it is required by xzarray_ensure_items()
|
||||
* (zarray.c, unmodified)'s own strict `mincap < caps` sufficiency check:
|
||||
* reserving EXACTLY N and then performing N stores still hits
|
||||
* `mincap == caps` on the very last store, which that strict check
|
||||
* treats as insufficient and grows again (by a small, delta-bounded
|
||||
* amount). This is verified directly below (see
|
||||
* test_xzarray_reserve_off_by_one_without_extra_headroom) rather than
|
||||
* assumed.
|
||||
* - Without any reservation, the identical N-store sequence DOES grow
|
||||
* capacity partway through (the behavior task 7.1 improves on).
|
||||
* - A simulated allocation failure is correctly propagated as -ENOMEM.
|
||||
* - Defensive/boundary: NULL array, negative n.
|
||||
*/
|
||||
|
||||
#include <errno.h>
|
||||
#include <rtti.h>
|
||||
#include <stdio.h>
|
||||
#include <stdlib.h>
|
||||
|
||||
#include <unity/unity.h>
|
||||
|
||||
#include "zarray.h"
|
||||
|
||||
static var make_test_xzarray(int delta)
|
||||
{
|
||||
xzarray_params_t params = { .delta = delta, .datakiller = NULL };
|
||||
return makeInstance(&xzarray_type, ¶ms);
|
||||
}
|
||||
|
||||
/* ── xzarray_reserve() genuinely pre-grows capacity (with the verified +1
|
||||
* headroom) -- zero further growth during N subsequent real stores ──────── */
|
||||
|
||||
static void
|
||||
test_xzarray_reserve_pregrows_capacity_no_growth_during_subsequent_stores(void)
|
||||
{
|
||||
const int delta = 1; /* small delta -- growth would otherwise be
|
||||
* triggered on almost every store, making this the
|
||||
* worst case (and the one bug-1443 cares about:
|
||||
* arrays larger than the natural delta margin). */
|
||||
const int n = 20;
|
||||
|
||||
var array = make_test_xzarray(delta);
|
||||
TEST_ASSERT_NOT_NULL(array);
|
||||
|
||||
/* Reserve n+1 -- see the file doc comment for why +1 is required. */
|
||||
int reserve_rc = xzarray_reserve(array, n + 1);
|
||||
TEST_ASSERT_TRUE_MESSAGE(reserve_rc >= 0,
|
||||
"xzarray_reserve must succeed for a valid array and count");
|
||||
|
||||
int caps_after_reserve = xzarray_test_get_caps(array);
|
||||
TEST_ASSERT_TRUE_MESSAGE(caps_after_reserve >= n + 1,
|
||||
"reservation must genuinely grow capacity to cover n+1 items");
|
||||
|
||||
for (int i = 0; i < n; i++) {
|
||||
char label[32];
|
||||
snprintf(label, sizeof(label), "item-%d", i);
|
||||
int idx = vstore(array, (void*)"x");
|
||||
TEST_ASSERT_TRUE(idx >= 0);
|
||||
|
||||
int caps_now = xzarray_test_get_caps(array);
|
||||
TEST_ASSERT_EQUAL_INT_MESSAGE(caps_after_reserve, caps_now,
|
||||
"capacity must NOT change during any of the n reserved stores --"
|
||||
" a change here means a realloc() happened despite a successful"
|
||||
" reservation, defeating the whole point of task 7.1");
|
||||
}
|
||||
|
||||
destroy(array);
|
||||
printf("test_xzarray_reserve_pregrows_capacity_no_growth_during_"
|
||||
"subsequent_stores: PASS\n");
|
||||
}
|
||||
|
||||
/* ── The verified off-by-one: reserving EXACTLY n and then storing n items
|
||||
* still triggers exactly one (small, delta-bounded) growth on the LAST
|
||||
* store -- documents/proves why job.c's caller reserves array_size + 1, not
|
||||
* array_size, to fully close growth during its bulk-insert pass. ────────── */
|
||||
|
||||
static void test_xzarray_reserve_off_by_one_without_extra_headroom(void)
|
||||
{
|
||||
const int delta = 1;
|
||||
const int n = 10;
|
||||
|
||||
var array = make_test_xzarray(delta);
|
||||
TEST_ASSERT_NOT_NULL(array);
|
||||
|
||||
int reserve_rc = xzarray_reserve(array, n); /* exactly n, no +1 */
|
||||
TEST_ASSERT_TRUE(reserve_rc >= 0);
|
||||
|
||||
int caps_after_reserve = xzarray_test_get_caps(array);
|
||||
|
||||
int grew_during_stores = 0;
|
||||
for (int i = 0; i < n; i++) {
|
||||
vstore(array, (void*)"x");
|
||||
if (xzarray_test_get_caps(array) != caps_after_reserve) {
|
||||
grew_during_stores = 1;
|
||||
}
|
||||
}
|
||||
|
||||
TEST_ASSERT_TRUE_MESSAGE(grew_during_stores,
|
||||
"reserving EXACTLY n and then storing n items is expected to still"
|
||||
" trigger one growth cycle on the last store (xzarray_ensure_items's"
|
||||
" strict mincap < caps check) -- this is the documented reason"
|
||||
" job.c's kvmgr_reserve() call reserves array_size + 1, not"
|
||||
" array_size");
|
||||
|
||||
destroy(array);
|
||||
printf("test_xzarray_reserve_off_by_one_without_extra_headroom: PASS\n");
|
||||
}
|
||||
|
||||
/* ── Baseline contrast: WITHOUT any reservation, the same N-store sequence
|
||||
* genuinely does grow capacity (the pre-task-7.1 behavior) ─────────────── */
|
||||
|
||||
static void test_xzarray_without_reservation_growth_occurs_during_stores(void)
|
||||
{
|
||||
const int delta = 1;
|
||||
const int n = 20;
|
||||
|
||||
var array = make_test_xzarray(delta);
|
||||
TEST_ASSERT_NOT_NULL(array);
|
||||
|
||||
int caps_initial = xzarray_test_get_caps(array);
|
||||
TEST_ASSERT_EQUAL_INT(0, caps_initial);
|
||||
|
||||
int grew_during_stores = 0;
|
||||
for (int i = 0; i < n; i++) {
|
||||
int before = xzarray_test_get_caps(array);
|
||||
vstore(array, (void*)"x");
|
||||
int after = xzarray_test_get_caps(array);
|
||||
if (after != before) {
|
||||
grew_during_stores = 1;
|
||||
}
|
||||
}
|
||||
|
||||
TEST_ASSERT_TRUE_MESSAGE(grew_during_stores,
|
||||
"without a reservation, growth during the store sequence is"
|
||||
" expected -- this is the pre-existing baseline task 7.1 improves"
|
||||
" on, not a new claim");
|
||||
|
||||
destroy(array);
|
||||
printf("test_xzarray_without_reservation_growth_occurs_during_stores: "
|
||||
"PASS\n");
|
||||
}
|
||||
|
||||
/* ── Simulated allocation failure propagates as -ENOMEM ─────────────────── */
|
||||
|
||||
static void test_xzarray_reserve_propagates_simulated_allocation_failure(void)
|
||||
{
|
||||
var array = make_test_xzarray(4);
|
||||
TEST_ASSERT_NOT_NULL(array);
|
||||
|
||||
xzarray_reserve_test_set_force_fail();
|
||||
|
||||
int rc = xzarray_reserve(array, 10);
|
||||
TEST_ASSERT_EQUAL_INT_MESSAGE(-ENOMEM, rc,
|
||||
"a forced allocation failure must propagate as -ENOMEM");
|
||||
|
||||
xzarray_reserve_test_clear_force_fail();
|
||||
|
||||
/* Clearing the seam must restore normal behavior. */
|
||||
int rc2 = xzarray_reserve(array, 10);
|
||||
TEST_ASSERT_TRUE_MESSAGE(rc2 >= 0,
|
||||
"clearing the force-fail seam must restore normal success");
|
||||
|
||||
destroy(array);
|
||||
printf("test_xzarray_reserve_propagates_simulated_allocation_failure: "
|
||||
"PASS\n");
|
||||
}
|
||||
|
||||
/* ── Defensive / boundary ─────────────────────────────────────────────── */
|
||||
|
||||
static void test_xzarray_reserve_rejects_null_array(void)
|
||||
{
|
||||
int rc = xzarray_reserve(NULL, 5);
|
||||
TEST_ASSERT_TRUE(rc < 0);
|
||||
printf("test_xzarray_reserve_rejects_null_array: PASS\n");
|
||||
}
|
||||
|
||||
static void test_xzarray_reserve_rejects_negative_n(void)
|
||||
{
|
||||
var array = make_test_xzarray(4);
|
||||
TEST_ASSERT_NOT_NULL(array);
|
||||
|
||||
int rc = xzarray_reserve(array, -1);
|
||||
TEST_ASSERT_TRUE(rc < 0);
|
||||
|
||||
destroy(array);
|
||||
printf("test_xzarray_reserve_rejects_negative_n: PASS\n");
|
||||
}
|
||||
|
||||
static void test_xzarray_reserve_zero_is_noop_success(void)
|
||||
{
|
||||
var array = make_test_xzarray(4);
|
||||
TEST_ASSERT_NOT_NULL(array);
|
||||
|
||||
int rc = xzarray_reserve(array, 0);
|
||||
TEST_ASSERT_TRUE(rc >= 0);
|
||||
|
||||
destroy(array);
|
||||
printf("test_xzarray_reserve_zero_is_noop_success: PASS\n");
|
||||
}
|
||||
|
||||
/* ── Test group entry point ───────────────────────────────────────────── */
|
||||
|
||||
void test_zarray_reserve(void)
|
||||
{
|
||||
printf("\n>> xzarray_reserve test\n");
|
||||
RUN_TEST(
|
||||
test_xzarray_reserve_pregrows_capacity_no_growth_during_subsequent_stores);
|
||||
RUN_TEST(test_xzarray_reserve_off_by_one_without_extra_headroom);
|
||||
RUN_TEST(test_xzarray_without_reservation_growth_occurs_during_stores);
|
||||
RUN_TEST(test_xzarray_reserve_propagates_simulated_allocation_failure);
|
||||
RUN_TEST(test_xzarray_reserve_rejects_null_array);
|
||||
RUN_TEST(test_xzarray_reserve_rejects_negative_n);
|
||||
RUN_TEST(test_xzarray_reserve_zero_is_noop_success);
|
||||
}
|
||||
|
|
@ -780,3 +780,72 @@ typeinfo_t xzarray_type = {
|
|||
.lock = &(intf_lock_t) { .lock = (lock_fn_t)xzarray_lock,
|
||||
.unlock = (unlock_fn_t)xzarray_unlock } }
|
||||
};
|
||||
|
||||
/* ------------------------------------------------------------------ */
|
||||
/* xzarray_reserve -- job-array-single-wal-record task 7.1 (bug-1443) */
|
||||
/* ------------------------------------------------------------------ */
|
||||
|
||||
/* NOTE on why these test-support symbols are NOT wrapped in
|
||||
* `#ifdef XCE_TESTING` here, unlike job.c's analogous seams: job.c is
|
||||
* recompiled from source directly into each test binary's own source list
|
||||
* (controller_unit_test_src, meson.build) with `-DXCE_TESTING` supplied as
|
||||
* that binary's own c_args, so an `#ifdef XCE_TESTING` guard in job.c is
|
||||
* evaluated per-binary. zarray.c, by contrast, is compiled EXACTLY ONCE, as
|
||||
* part of the shared `libxce` static_library() target (src/lib/meson.build)
|
||||
* -- WITHOUT `-DXCE_TESTING` -- and that single libxce.a is then linked,
|
||||
* unmodified, into every consumer, test binaries included. Guarding these
|
||||
* definitions behind `#ifdef XCE_TESTING` here would compile them out of
|
||||
* libxce.a entirely, in every build, making them permanently unresolvable
|
||||
* link errors for the test binaries that need them (confirmed empirically:
|
||||
* this was the very shape of the link failure this fix corrects). The
|
||||
* header-side declarations (zarray.h) remain guarded, so these symbols stay
|
||||
* un-prototyped -- and therefore uncallable without a compile error -- from
|
||||
* any production translation unit; only the symbol bytes are unconditionally
|
||||
* present in libxce.a, not a callable, declared API. The force-fail flag
|
||||
* below defaults to false and only ever affects xzarray_reserve() itself
|
||||
* (never any other function), so its unconditional presence is inert unless
|
||||
* a test explicitly engages it. File-scope, not per-instance -- mirrors
|
||||
* job.c's own s_job_array_bulk_insert_force_fail_at convention otherwise. */
|
||||
static _Atomic _Bool s_xzarray_reserve_force_fail = false;
|
||||
|
||||
void xzarray_reserve_test_set_force_fail(void)
|
||||
{
|
||||
atomic_store_explicit(&s_xzarray_reserve_force_fail, true,
|
||||
memory_order_relaxed);
|
||||
}
|
||||
|
||||
void xzarray_reserve_test_clear_force_fail(void)
|
||||
{
|
||||
atomic_store_explicit(&s_xzarray_reserve_force_fail, false,
|
||||
memory_order_relaxed);
|
||||
}
|
||||
|
||||
int xzarray_test_get_caps(var array)
|
||||
{
|
||||
if (array == NULL) {
|
||||
return -EINVAL;
|
||||
}
|
||||
|
||||
meta_obj_t* obj = RTTI_DATA_TO_OBJ(array);
|
||||
xzarray_t* a = RTTI_OBJ_TO_DATA(xzarray_t, obj);
|
||||
|
||||
return (int)a->caps;
|
||||
}
|
||||
|
||||
int xzarray_reserve(var array, int n)
|
||||
{
|
||||
if ((array == NULL) || (n < 0)) {
|
||||
return -EINVAL;
|
||||
}
|
||||
|
||||
if (atomic_load_explicit(&s_xzarray_reserve_force_fail,
|
||||
memory_order_relaxed)) {
|
||||
errno = ENOMEM; /* simulated resource-exhaustion failure */
|
||||
return -ENOMEM;
|
||||
}
|
||||
|
||||
meta_obj_t* obj = RTTI_DATA_TO_OBJ(array);
|
||||
xzarray_t* a = RTTI_OBJ_TO_DATA(xzarray_t, obj);
|
||||
|
||||
return xzarray_ensure_items(a, a->count + n);
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue