diff --git a/.kiro/specs/job-array-single-wal-record/tasks.md b/.kiro/specs/job-array-single-wal-record/tasks.md index 5ebfbefa..ff256706 100644 --- a/.kiro/specs/job-array-single-wal-record/tasks.md +++ b/.kiro/specs/job-array-single-wal-record/tasks.md @@ -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). diff --git a/src/controller/job.c b/src/controller/job.c index 046ca367..e2a3bde7 100644 --- a/src/controller/job.c +++ b/src/controller/job.c @@ -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); } - xlist_fini(&built); + + /* 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; } diff --git a/src/controller/persist.c b/src/controller/persist.c index 6cd5d663..64666904 100644 --- a/src/controller/persist.c +++ b/src/controller/persist.c @@ -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 diff --git a/src/controller/testsuite/test_apply_committed_entry.c b/src/controller/testsuite/test_apply_committed_entry.c index 864f1c6f..e22936aa 100644 --- a/src/controller/testsuite/test_apply_committed_entry.c +++ b/src/controller/testsuite/test_apply_committed_entry.c @@ -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(); diff --git a/src/controller/testsuite/test_job_array_apply.c b/src/controller/testsuite/test_job_array_apply.c index 0505446a..fa9edf4a 100644 --- a/src/controller/testsuite/test_job_array_apply.c +++ b/src/controller/testsuite/test_job_array_apply.c @@ -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); diff --git a/src/include/kvmgr.h b/src/include/kvmgr.h index 9e21adf7..7b270b57 100644 --- a/src/include/kvmgr.h +++ b/src/include/kvmgr.h @@ -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 diff --git a/src/include/zarray.h b/src/include/zarray.h index cfce4883..f8f715d0 100644 --- a/src/include/zarray.h +++ b/src/include/zarray.h @@ -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 diff --git a/src/lib/kvmgr.c b/src/lib/kvmgr.c index 5891035e..854a3c75 100644 --- a/src/lib/kvmgr.c +++ b/src/lib/kvmgr.c @@ -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); +} diff --git a/src/lib/meson.build b/src/lib/meson.build index 507e2e07..a99ab5a7 100644 --- a/src/lib/meson.build +++ b/src/lib/meson.build @@ -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', diff --git a/src/lib/testsuite/test.c b/src/lib/testsuite/test.c index b149087b..1ec5b869 100644 --- a/src/lib/testsuite/test.c +++ b/src/lib/testsuite/test.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); diff --git a/src/lib/testsuite/test_kvmgr_reserve.c b/src/lib/testsuite/test_kvmgr_reserve.c new file mode 100644 index 00000000..ad113086 --- /dev/null +++ b/src/lib/testsuite/test_kvmgr_reserve.c @@ -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 +#include +#include +#include + +#include + +#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); +} diff --git a/src/lib/testsuite/test_zarray_reserve.c b/src/lib/testsuite/test_zarray_reserve.c new file mode 100644 index 00000000..c87e5adb --- /dev/null +++ b/src/lib/testsuite/test_zarray_reserve.c @@ -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 +#include +#include +#include + +#include + +#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); +} diff --git a/src/lib/zarray.c b/src/lib/zarray.c index 88df02fd..42113e52 100644 --- a/src/lib/zarray.c +++ b/src/lib/zarray.c @@ -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); +}