forked from lei.chen/myxce
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.
This commit is contained in:
parent
85ce5da61c
commit
575f928bbc
2 changed files with 32 additions and 3 deletions
|
|
@ -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."
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue