From 575f928bbc83a6e379e1e1bfe20d48931145799e Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Thu, 16 Jul 2026 13:14:03 +0000 Subject: [PATCH] plan(job-array-single-wal-record): reopen spec for residual gap closure User directed (2026-07-16) that this feature's own open residual gaps be fixed: bug-1443 (restore_job_array()'s bulk-insert partial-visibility gap), task 4.2's boot-replay fence gap, and task 5.3b's bounded leak. Unrelated pre-existing bugs 1437/1438 explicitly excluded from this reopening per user's scope decision. Adds Phase 7 (tasks 7.1-7.3) to tasks.md: - 7.1: add a purely-additive kvmgr_reserve() capacity-reservation primitive (mirroring the existing kvmgr_capture_keys() template) so restore_job_array()'s bulk-insert pass can pre-grow its backing store before mutating, closing the dominant N-scaling failure trigger; plus make a genuinely-inserted partial prefix always get notified even in the now-much-narrower residual failure case - 7.2: extend the Live Apply Resource Fence's halt-and-fence protection to replay_wal() (boot/restart replay), reusing the same primitives task 4.1/4.2 already built for the live path - 7.3: free dataset->rows in sched_handle_dataset()'s jobArray case spec.json phase reverted to tasks-generated to reflect the reopening. --- .../job-array-single-wal-record/spec.json | 8 +++--- .../job-array-single-wal-record/tasks.md | 27 +++++++++++++++++++ 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/.kiro/specs/job-array-single-wal-record/spec.json b/.kiro/specs/job-array-single-wal-record/spec.json index 1efde7a..318cae2 100644 --- a/.kiro/specs/job-array-single-wal-record/spec.json +++ b/.kiro/specs/job-array-single-wal-record/spec.json @@ -1,9 +1,9 @@ { "feature_name": "job-array-single-wal-record", "created_at": "2026-07-09T17:20:00Z", - "updated_at": "2026-07-16T02:10:00Z", + "updated_at": "2026-07-16T06:15:00Z", "language": "en", - "phase": "implementation-complete", + "phase": "tasks-generated", "approvals": { "requirements": { "generated": true, @@ -25,6 +25,8 @@ "validated_at": "2026-07-16T02:10:00Z", "known_limitations": [ "bug-1443: restore_job_array()'s bulk-insert-phase failure can permanently orphan array elements (violates Req 2.3/9.4 literally); tracked in .wolf/buglog.json, not fixed in this spec" - ] + ], + "reopened_at": "2026-07-16T06:15:00Z", + "reopen_reason": "User directed all of this feature's own open residual gaps be fixed (2026-07-16): bug-1443 (bulk-insert partial-visibility), task 4.2's boot-replay fence gap, and task 5.3b's bounded leak. Added as Phase 7 (tasks 7.1-7.3) in tasks.md. Unrelated pre-existing bugs 1437/1438 explicitly excluded from this reopening per user's scope decision." } } diff --git a/.kiro/specs/job-array-single-wal-record/tasks.md b/.kiro/specs/job-array-single-wal-record/tasks.md index a0d2994..5ebfbef 100644 --- a/.kiro/specs/job-array-single-wal-record/tasks.md +++ b/.kiro/specs/job-array-single-wal-record/tasks.md @@ -187,6 +187,33 @@ - _Requirements: 8.1, 8.2, 8.4_ - _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) + - 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 + - For that now-much-narrower residual case: make `restore_job_array()` populate `out_new_jobs` with whatever prefix was genuinely, successfully inserted before a mid-pass failure, even though the function's overall return is still negative — and make `apply_committed_entry()`'s `WAL_JOB_ARRAY_SUBMIT` case fire `HOOK_DETAIL_JOB_NEW` for that non-empty partial prefix even when `restore_job_array()`'s return is negative, while still propagating the negative return so the LSN correctly halts and the Live Apply Resource Fence still engages for the remaining un-constructed indices. Net effect: a genuinely-inserted element is never left un-pushed/un-scheduled again, even in the narrow residual failure case — closing Requirement 2.3/9.4's literal text, not just narrowing the trigger condition + - Observable: a fault-injection test forcing the bulk-insert-phase failure (reusing task 3.1's existing bulk-insert-phase seam) shows the already-inserted prefix IS included in a fired `HOOK_DETAIL_JOB_NEW` (not silently orphaned), while the overall LSN still correctly halts/fences for the remainder; a separate test proves `kvmgr_reserve()` genuinely pre-grows capacity (no realloc occurs during the subsequent bulk-insert loop when reservation succeeded) + - _Requirements: 2.3, 9.4, 9.6_ + - _Boundary: Array Apply Expansion (`src/lib/kvmgr.c`, `src/lib/zarray.c`, `src/controller/job.c`'s `restore_job_array()`, `src/controller/persist.c`'s `apply_committed_entry()` `WAL_JOB_ARRAY_SUBMIT` case) — touches shared library code (`kvmgr.c`/`zarray.c`) used far beyond this feature; new functions only, zero changes to existing function bodies_ + +- [ ] 7.2 Extend the Live Apply Resource Fence's protection across a boot/restart replay (task 4.2's self-flagged gap) + - `replay_wal()` (boot/restart replay path) currently captures `apply_committed_entry()`'s return value only for the job-count tally (task 6.0a) — it does not halt or engage the Live Apply Resource Fence on a negative return the way `persist_apply_pump_run()` (the live path, task 4.1) already does + - Add the same halt-and-fence behavior to `replay_wal()`: on a negative `apply_committed_entry()` return during replay, stop the replay walk at that LSN without advancing past it, log loud, and call the SAME `persist_mark_apply_resource_fenced()` primitive the live path uses — reuse, do not reimplement + - Verify (and document, not just assume) that this engages BEFORE the node could ever be considered for promotion during its own boot sequence — trace the actual boot ordering to confirm `persist_recover()`/`replay_wal()` completes before `ha.c`'s promotion-gate logic could evaluate this node as a candidate + - `persist_init()`'s existing unconditional fence-reset on every fresh process construction remains correct and unchanged: if the same resource-exhaustion condition recurs during a later boot replay, this task's new check will immediately re-engage the fence before promotion is possible, so starting from a clear state on each fresh process is safe by construction, not a gap + - Observable: a test simulating a node that crashes while fenced and restarts into a boot replay hitting the identical resource-exhaustion condition (reuse task 3.1's fault-injection seam) shows the node re-engages the fence during replay and is refused promotion, rather than silently booting clean and looking promotable while actually behind + - _Requirements: 9.4, 9.5_ + - _Boundary: Live Apply Resource Fence (`src/controller/persist.c`'s `replay_wal()`) — reuses existing task 4.1/4.2 primitives verbatim, no new fence mechanism_ + - _Depends: 7.1 is not a prerequisite (independent root cause), but should land first since both touch `persist.c`_ + +- [ ] 7.3 Fix the bounded per-commit leak on the scheduler receive path (task 5.3b's self-flagged gap) + - `sched_handle_dataset()`'s `Data_data_jobArray` case never frees `dataset->rows` (the decoded `xce_job_submit_t*`) after `sched_on_job_array()` consumes it — `free_Dataset()` is an empty no-op for every table type, so this is the one call site responsible for freeing this specific variant's payload + - Free `dataset->rows` (and its embedded submit spec, following whatever the existing `xce_job_submit_t`/`JobSubmitData` teardown convention already is elsewhere in this file) after `sched_on_job_array()` returns, in both the success and any early-return/error path through that case + - Observable: a leak-detection test run (ASan) around a real array push shows zero growth attributable to this path, where it previously showed one bounded leak per delivered array push + - _Requirements: 8.4_ + - _Boundary: Compact Scheduler/API-Server Push (`src/scheduler/src/handler.c`'s `sched_handle_dataset()`)_ + ## Implementation Notes - **Task 1.3**: `meson test --suite xce`'s `libxce test` target SIGABRTs under ASAN leak detection whenever any code path exercises a `job_init()` failure — root cause is the vendored `rtti` library's `makeInstance()` not freeing its allocation when a type's `.init()` returns negative (no `.c` source available, headers-only in the nix store). This is pre-existing and independently reproduced against an unmodified pre-task `jobj.c`, not introduced by this feature. It will fire again on any future task whose tests exercise a `create_job()`/`makeJob()` failure path (e.g. task 3.1/3.4's fault-injection tests for `restore_job_array()`) — expect the same SIGABRT-not-assertion-failure signature (`grep ":FAIL"` on the Unity log will be empty; the failure is LeakSanitizer-only) and do not treat it as a new regression without the same stash-and-reproduce check task 1.3's review used.