docs(job-array-single-wal-record): record /kiro-validate-impl GO decision
Feature-level validation ran 4 parallel dimensions: requirements coverage (38 ACs, 33 full, 5 partial -- all tracing to the already-known bulk-insert partial-visibility gap plus an admitted commit-barrier-count inference), design/boundary alignment (ALIGNED, no drift), cross-task integration (SEAMS_SOUND, wire format/arithmetic/contracts verified consistent end to end), and mechanical checks including a live smoke boot (fresh single-node deploy, real array submission, one WAL record, all elements queryable) -- PASS. The coverage pass sharpened restore_job_array()'s known bulk-insert gap: it's a literal, code-proven violation of Requirements 2.3 and 9.4, not just a soft nuance -- a resource-exhaustion failure during the bulk-insert pass can permanently orphan array elements that exist and are addressable but never get scheduled. Filed as bug-1443 (.wolf/buglog.json) for tracking. User decision: GO, ship with the gap tracked rather than blocking on a fast-follow fix -- the trigger condition is narrow and every other validation dimension is clean. Closing bug-1443 needs a new kv.c/kvmgr.c primitive or a re-derive- and-renotify path, explicitly out of this spec's approved scope. spec.json phase -> implementation-complete.
This commit is contained in:
parent
1cfdac47b6
commit
2b0a37acc7
3 changed files with 175 additions and 9 deletions
|
|
@ -13116,5 +13116,162 @@
|
|||
],
|
||||
"occurrences": 2,
|
||||
"last_seen": "2026-07-12T16:12:54Z"
|
||||
},
|
||||
{
|
||||
"id": "bug-1437",
|
||||
"timestamp": "2026-07-13T00:55:00Z",
|
||||
"error_message": "Verification run of lib build + xcelib_test --run test_jobj_hardening (ASAN_OPTIONS=detect_leaks=1) after user changed 4x .datakiller=destroy -> .datakiller=dereference in src/lib/testsuite/test_jobj_hardening.c (to fix a leak in the kvmgr-of-job_t test fixture, since job_t.destroy is NULL in RTTI typeinfo and teardown only runs via job_dereference): Unity reports 5/5 PASS, 0 Failures, 0 Ignored, but LeakSanitizer STILL reports two leaks totaling 247 bytes and the process exits 1 (no halt_on_error/abort_on_error set, so no SIGABRT with plain detect_leaks=1 -- differs from the meson full-suite run which sets abort_on_error=1 and gets killed by signal 6).",
|
||||
"file": "src/lib/testsuite/test_jobj_hardening.c",
|
||||
"root_cause": "TWO leaks detected, only one already known. (1) Direct leak of 240 bytes via calloc -> makeInstance -> makeJob_ (../lib/jobj.c:768) -> makeJob (../lib/jobj.c:789) -> test_job_init_propagates_makeJobsub_failure (test_jobj_hardening.c:133) -- this IS the already-diagnosed, out-of-repo makeInstance-does-not-free-on-negative-init-rc characteristic (vendored rtti library), unrelated to the datakiller fix. (2) Direct leak of 7 bytes via strdup -> hard_build_accounts (test_jobj_hardening.c:269) -> test_job_destroy_does_not_leak_accounts (test_jobj_hardening.c:328) -- this is a SEPARATE, NEW leak signature not previously logged, and it is attributed to the exact test (test_job_destroy_does_not_leak_accounts) that the datakiller=dereference fix was meant to make leak-free. NOT YET DIAGNOSED WHY: could be an unrelated small leak in the hard_build_accounts helper itself (e.g. a strdup()d account string never freed regardless of datakiller correctness), or could indicate the datakiller fix is incomplete. Not investigated further per explicit user instruction to build/run/report only, no fixing.",
|
||||
"fix": "NOT FIXED IN THIS SESSION -- user explicitly instructed build+run+report only, no code changes. Logging the raw finding so the next session does not have to rediscover it: the accounts-leak test still leaks (7 bytes, strdup, hard_build_accounts) even after the .datakiller=dereference change, though it is a much smaller leak than the pre-existing 240-byte makeInstance one and does not cause any Unity test FAILURE (LSAN leak detection is a separate pass/fail axis from Unity PASS/FAIL in this harness).",
|
||||
"tags": [
|
||||
"jobj",
|
||||
"test_jobj_hardening",
|
||||
"leak",
|
||||
"asan",
|
||||
"lsan",
|
||||
"datakiller",
|
||||
"dereference",
|
||||
"hard_build_accounts",
|
||||
"test_job_destroy_does_not_leak_accounts",
|
||||
"unverified-fix",
|
||||
"kvmgr"
|
||||
],
|
||||
"related_bugs": [],
|
||||
"occurrences": 1,
|
||||
"last_seen": "2026-07-13T00:55:00Z"
|
||||
},
|
||||
{
|
||||
"id": "bug-1438",
|
||||
"timestamp": "2026-07-14T09:36:00Z",
|
||||
"error_message": "test_job_array_submit_appends_exactly_one_wal_entry_and_one_continuation: Expected 11 Was 5 (WAL type mismatch); subsequent test hung all 3 threads on futex_do_wait",
|
||||
"file": "src/controller/testsuite/test_commit_barrier_rollout.c",
|
||||
"root_cause": "jap_read_last_wal_hdr()/jap_read_last_wal_payload() were copied from read_last_wal_hdr() in test_ha_role_lifecycle.c, which computes the header offset as seg_offset - sizeof(hdr). That arithmetic is only correct for zero-payload entries (its only prior caller was a WAL_NOOP test). Every WAL_JOB_ARRAY_SUBMIT entry carries a nonzero capnp payload, so seg_offset - sizeof(hdr) landed inside the payload bytes, not the header -- read garbage decoded as a bogus type (5 == WAL_JOB_STOP). The first array test's TEST_ASSERT_EQUAL_UINT32 failure then longjmp'd out of the test body BEFORE reaching jap_teardown(), leaking that test's persist_t (background WAL-sync thread still running) into the next test's jap_setup() memset -- corrupting a live mutex mid-flight and deadlocking all 3 threads on futex_do_wait in the very next test.",
|
||||
"fix": "Replaced the back-computed-offset helpers with jap_read_wal_hdr_at()/jap_read_wal_entry_at(), which take the entry's start offset EXPLICITLY. Callers now snapshot g_jap.seg_offset immediately before each persist_submit_job_array_async() call and pass that snapshot in, instead of trying to infer the header position from the post-append offset.",
|
||||
"tags": [
|
||||
"job-array-single-wal-record",
|
||||
"wal",
|
||||
"test-helper",
|
||||
"offset-arithmetic",
|
||||
"deadlock",
|
||||
"futex"
|
||||
],
|
||||
"related_bugs": [],
|
||||
"occurrences": 1,
|
||||
"last_seen": "2026-07-14T09:36:00Z"
|
||||
},
|
||||
{
|
||||
"id": "bug-1439",
|
||||
"timestamp": "2026-07-14T13:15:00Z",
|
||||
"error_message": "lib/controller.capnp.h:4170:43: error: unknown type name 'LimitCon'; lib/controller.capnp.h:1: error: unterminated #ifndef \u2014 during ninja compile of lib/libxce.a.p/utils.c.o, cascading into ~440 bogus errors (glibc math.h storage-class errors, utils.h ectx_t redefinition) for the rest of that one translation unit",
|
||||
"file": "src/xce.build/lib/controller.capnp.h (generated by [5/190] Generating lib/controllercapnp.[ch] custom command)",
|
||||
"root_cause": "SUSPECTED (not confirmed): parallel-build race \u2014 gcc appears to have read lib/controller.capnp.h while the custom-command generation step was still writing it (non-atomic write, no write-to-tmp+rename). Evidence: re-inspecting the SAME on-disk file immediately after the failed build shows correct, well-formed content at line 4170 (get_LimitConfig(struct LimitConfig*, LimitConfig_list, int i);) and balanced #ifndef/#endif (9/9) \u2014 i.e. the finished file is NOT actually corrupt, only what gcc read mid-write was. Not yet reproduced a second time to confirm.",
|
||||
"fix": "NONE APPLIED \u2014 this run was build+report only (nix develop ./src -- xce-build.sh controller), not a fix task. Build failed via set -euo pipefail before any tests ran; no controller/xce_controller, controller_unit_test, or xce_test binaries were produced. Next step if it recurs: check meson.build custom_target() for lib/controller.capnp.[ch] has correct output declarations / add explicit depends on all consumers, or file upstream against capnpc-c invocation for atomic output.",
|
||||
"tags": [
|
||||
"build-failure",
|
||||
"capnp",
|
||||
"ninja",
|
||||
"parallel-build-race",
|
||||
"controller",
|
||||
"transient-suspected",
|
||||
"unresolved"
|
||||
],
|
||||
"related_bugs": [],
|
||||
"occurrences": 1,
|
||||
"last_seen": "2026-07-14T13:15:00Z"
|
||||
},
|
||||
{
|
||||
"id": "bug-1440",
|
||||
"timestamp": "2026-07-15T09:35:00Z",
|
||||
"error_message": "use of cgo in test wire_dispatch_test.go not supported / FAIL apiserver/backend [setup failed]",
|
||||
"file": "src/frontend/apiserver/backend/wire_dispatch_test.go",
|
||||
"root_cause": "Go toolchain hard restriction, not an environment/CGO_CFLAGS/nix issue: cmd/go's module indexer (GOROOT share/go/src/cmd/go/internal/modindex/read.go, ~line 589, inside the per-file import scan) explicitly checks `if imp.path == \"C\" { if isTest { badGoFile(name, fmt.Errorf(\"use of cgo in test %s not supported\", name)) } }`. Any `_test.go` file that directly does `import \"C\"` is rejected outright the moment CGO is active for that file (CGO_ENABLED=1 and the file's build tag, here `//go:build cgo`, is satisfied) -- this is golang/go#13938, a long-standing toolchain limitation, confirmed by reading the actual installed Go 1.25.5 source in the nix shell. Because wire_dispatch_test.go and clib.go/convert.go/peers_test.go/handler_test.go/convert_test.go/heartbeat_test.go (the last four carry no build tag, so they build under BOTH cgo and !cgo) all live in the same package, the ENTIRE apiserver/backend package fails as `[setup failed]` under `make test-backend` (CGO_ENABLED=1) -- this also silently prevented the pre-existing cgo-buildable tests (handler_test.go, convert_test.go, peers_test.go, heartbeat_test.go) from running, even though none of them regressed on their own. The `!cgo`-tagged files (integration_test.go, control_host_test.go, stub_funcs_test.go) are unaffected since they're excluded from the CGO_ENABLED=1 file set entirely; `make test` (CGO_ENABLED=0) passed clean (apiserver/backend ok, 1.510s) confirming no regression on the non-cgo path.",
|
||||
"fix": "NOT FIXED -- user explicitly instructed build/run/report only, no source edits. Reported fix path for the user: remove `import \"C\"` (and any direct `C.xxx` calls) from wire_dispatch_test.go itself; move the cgo-dependent helpers (buildEncodedJobArrayPush, deliverViaRealDispatchChain, and the `/* ... */ import \"C\"` preamble with the wire_test_* static C helpers) into a new non-test `.go` file in the same package (e.g. clib_wire_test_helpers.go, `//go:build cgo`), exposing plain-Go-signature functions the _test.go file can call without itself using cgo directly. This mirrors the established pattern already used for other CGO test-only helpers in this package. Because the file was rejected before type-checking, it is unknown whether wire_dispatch_test.go has any further errors (undefined C symbols, type mismatches) beyond this first, blocking one -- those can only be discovered after this structural fix.",
|
||||
"tags": [
|
||||
"cgo",
|
||||
"go-test",
|
||||
"toolchain-limitation",
|
||||
"wire_dispatch_test",
|
||||
"job-array-single-wal-record",
|
||||
"backend-package",
|
||||
"build-error",
|
||||
"golang-go-13938"
|
||||
],
|
||||
"related_bugs": [],
|
||||
"occurrences": 1,
|
||||
"last_seen": "2026-07-15T09:35:00Z"
|
||||
},
|
||||
{
|
||||
"id": "bug-1441",
|
||||
"timestamp": "2026-07-15T09:40:00Z",
|
||||
"error_message": "backend/wire_dispatch_support.go:154:8: could not determine what C.go_push_handler refers to / FAIL apiserver/backend [build failed]",
|
||||
"file": "src/frontend/apiserver/backend/wire_dispatch_support.go",
|
||||
"root_cause": "cgo declaration visibility is per-file, not per-package: go_push_handler is exported to C only via clib.go's own preamble comment (\"extern int go_push_handler(xce_request_t*, void*);\" immediately followed by \"//export go_push_handler\" on the Go func), and that extern declaration lives nowhere in any shared header -- it is a plain comment inside clib.go's /* ... */ block, not client.h/netio.h/request.h/controller.capnp.h. cgo processes each file's own preamble independently to build that file's C.xxx symbol table, so wire_dispatch_support.go (which #includes client.h/netio.h/request.h/controller.capnp.h but does NOT itself declare go_push_handler) has no visibility into that extern, even though both files are in the same Go package and both carry //go:build cgo. This surfaced only after fixing bug-1440's toolchain-level rejection (moving import \"C\" out of the _test.go file) and after separately discovering make test-backend also needs CGO_CFLAGS/CGO_LDFLAGS manually derived to match src/frontend/apiserver/meson.build (pkg-config --cflags/--libs for librtti, c-capnproto, openssl, tinylog, plus -I/src/include -I/src/build/lib -L/src/build/lib -lxce -pthread -lm) since the nix devshell itself does not export them and make test-backend invokes go test directly rather than through the meson/go_build path that normally injects them.",
|
||||
"fix": "NOT FIXED -- user explicitly instructed build/run/report only, no source edits. Reported fix path for the user: add the same \"extern int go_push_handler(xce_request_t*, void*);\" declaration to wire_dispatch_support.go's own cgo preamble (matching clib.go's exact signature), OR move that extern declaration into a shared header both files already include. Because the toolchain stops at first error, it is unknown whether wire_dispatch_support.go/wire_dispatch_test.go have any FURTHER errors (e.g. C.free_JobSubmit_ptr signature, C.extract_data, C.xce_encode/xce_decode field access) beyond this first blocking one -- those can only be discovered after this fix. Separately (env-only, no source change): make test-backend also requires CGO_CFLAGS/CGO_LDFLAGS to be exported manually before invoking make, mirroring meson.build's pkg-config-derived flags -- the bare Makefile command alone (as documented) does not produce a working CGO_ENABLED=1 build in this worktree.",
|
||||
"tags": [
|
||||
"cgo",
|
||||
"wire_dispatch",
|
||||
"go_push_handler",
|
||||
"cross-file-declaration",
|
||||
"job-array-single-wal-record",
|
||||
"test-backend",
|
||||
"CGO_CFLAGS"
|
||||
],
|
||||
"related_bugs": [
|
||||
"bug-1440"
|
||||
],
|
||||
"occurrences": 1,
|
||||
"last_seen": "2026-07-15T09:40:00Z"
|
||||
},
|
||||
{
|
||||
"id": "bug-1442",
|
||||
"timestamp": "2026-07-15T09:50:00Z",
|
||||
"error_message": "AddressSanitizer: attempting double-free on 0x...1b0 in thread T0 -- fires immediately on TestWireDispatch_OnePushProducesNRecordsAndLogsOnce, aborts the whole backend.test binary before it can print PASS/FAIL, so TestWireDispatch_RedeliveryCreatesNoNewRecordsAndLogsNothing never runs at all",
|
||||
"file": "src/frontend/apiserver/backend/wire_dispatch_support.go",
|
||||
"root_cause": "NOT ROOT-CAUSED (build/run/report only, no fix attempted per user instruction). bug-1441 (C.go_push_handler cross-file cgo resolution) is now fixed, and a separate missing `import \"apiserver/store\"` in wire_dispatch_test.go was fixed as a trivial compile error, so the build now succeeds and the test actually runs -- and immediately hits a real ASan double-free at runtime, not a build failure. Leading hypothesis (unverified): wireDispatchDeliver() (lines 122-180) calls go_push_handler(&decoded, nil) at line 164, whose defer C.free_Request(req) (clib.go) frees *decoded including its Request_body_datamsg payload -- the comment at lines 166-172 asserts free_Dataset() is 'a documented no-op for every table' (citing task 5.3b's residual-gap note) and therefore safe for wireDispatchDeliver to ALSO free ds.rows via free_JobSubmit_ptr() and ds itself at lines 173-177. A 16-byte double-free immediately after go_push_handler returns is consistent with that no-op assumption being wrong for the Data_data_jobArray-derived dataset table specifically (i.e. free_Dataset() actually does free ds->rows or ds itself for this table, and wire_dispatch_support.go's explicit cleanup then frees the same memory again). NOT CONFIRMED -- needs a symbolized ASan backtrace (frames are bare hex addresses in this build) or manual trace of free_Dataset()/free_Request() to confirm which free is the duplicate.",
|
||||
"fix": "FIXED and VERIFIED 2026-07-15T09:55:00Z. Root cause confirmed: go_push_handler's internal `defer C.free_Request(req)` (clib.go) calls free_Request(&decoded), whose Request_body_datamsg case calls free_Dataset_ptr(&decoded.body.datamsg) -- free_Dataset_ptr() (controller.capnp.c, generated) unconditionally free()s the dataset_t container pointer it is given, even though free_Dataset() itself is an empty no-op (never touches ->rows). So `ds` becomes a dangling pointer the instant go_push_handler returns -- the earlier NOT-ROOT-CAUSED hypothesis was correct in spirit but had the wrong culprit function (free_Dataset_ptr, not free_Dataset). wireDispatchDeliver() (wire_dispatch_support.go) had two bugs: (1) read ds.rows AFTER calling go_push_handler = use-after-free, (2) separately called C.free(ds) again = double-free on the same 16-byte region ASan caught. Fix: capture `rows := ds.rows` BEFORE calling go_push_handler (line 175), use that captured value afterward instead of re-reading ds.rows, and removed the redundant C.free(unsafe.Pointer(ds)) call entirely since go_push_handler's internal free_Request already frees it. Verified via full ASan CGO rebuild (make test-backend under nix develop ./src with CGO_CFLAGS/CGO_LDFLAGS = -fsanitize=address -g -DWITH_DEBUG=1 + pkg-config librtti/c-capnproto/openssl/tinylog): TestWireDispatch_OnePushProducesNRecordsAndLogsOnce and TestWireDispatch_RedeliveryCreatesNoNewRecordsAndLogsNothing both PASS with zero ASan errors; full ./backend/... suite (91 subtests, convert/handler/heartbeat/peers/wire_dispatch/auth) all PASS, zero regressions; make test (CGO_ENABLED=0) also fully green.",
|
||||
"tags": [
|
||||
"cgo",
|
||||
"wire_dispatch",
|
||||
"asan",
|
||||
"double-free",
|
||||
"use-after-free",
|
||||
"job-array-single-wal-record",
|
||||
"test-backend",
|
||||
"go_push_handler",
|
||||
"free_Dataset",
|
||||
"free_Dataset_ptr"
|
||||
],
|
||||
"related_bugs": [
|
||||
"bug-1441"
|
||||
],
|
||||
"occurrences": 1,
|
||||
"last_seen": "2026-07-15T09:55:00Z"
|
||||
},
|
||||
{
|
||||
"id": "bug-1443",
|
||||
"timestamp": "2026-07-16T02:05:00Z",
|
||||
"error_message": "OPEN, NOT A CRASH -- discovered as a design-time/test-proven gap, not a runtime failure report. restore_job_array()'s final bulk-insert pass into c->jobmgr can itself fail partway through (e.g. realloc() exhaustion growing c->jobmgr's backing table). Because the underlying xkvmgr_type store has no key_del, an already-inserted prefix [0, k) cannot be rolled back and is left permanently, genuinely visible in c->jobmgr -- individually addressable (status/cancel/completion all work via direct jobid lookup) -- while the function honestly returns a negative errno (never silently claims success). Critically, that prefix is never included in out_new_jobs, so HOOK_DETAIL_JOB_NEW never fires for it, not on this attempt or on any later successful retry of the same LSN (the per-index idempotent-skip logic on retry treats the already-inserted prefix as 'already exists', excluding it from the retry's own out_new_jobs too). Practical consequence: these array elements exist and are queryable forever, but never reach the scheduler's or API-server's push and so are never actually scheduled to run -- a silent, permanent no-op job, not merely a bookkeeping gap. requirements-coverage validation (kiro-validate-impl, 2026-07-16) confirmed this is a literal violation of Requirement 2.3 ('a standby shall never observe a single array WAL record in a partially-applied form') and Requirement 9.4 ('shall not construct a partial/divergent set of elements'), and its shadow extends to 9.5 (self-heal, since the orphaned prefix never gets a fresh chance to hook-fire even once the retry succeeds) and 9.6 (the capstone fault-injection test deliberately targets the fully-atomic construction-phase seam instead of this bulk-insert seam, precisely because asserting 'zero partial elements' against this seam would be dishonest).",
|
||||
"file": "src/controller/job.c (restore_job_array(), bulk-insert phase)",
|
||||
"root_cause": "No capacity-reservation or rollback primitive exists anywhere in this codebase's kv.c/kvmgr.c/zarray.c stack -- c->jobmgr (xkvmgr_type) grows its backing table via realloc() with no pre-check, and once key_add() has inserted an element there is no key_del to undo it. restore_job_array()'s design (scratch-build every element first, then one bulk-insert pass under a single write-lock) makes construction-phase failures fully atomic (nothing ever touches c->jobmgr until every element is ready), but the bulk-insert pass itself is NOT covered by that atomicity guarantee -- it is the one place a late-stage resource-exhaustion failure can leave real, permanent state behind that the function's own success/failure contract cannot fully describe. Discovered and documented during task 3.1's original implementation review (job-array-single-wal-record spec), re-confirmed live-testable via test_rja_bulk_insert_failure_reports_error_not_false_success (test_job_array_apply.c), and re-surfaced by name during the spec's final /kiro-validate-impl requirements-coverage pass.",
|
||||
"fix": "NOT FIXED -- explicitly out of job-array-single-wal-record's scope by design; closing it requires new capability this spec's task list was never scoped to build. Two known remediation paths, either of which is its own small design effort, not a quick task-list append: (1) add a real key_del/capacity-reservation primitive to kv.c/kvmgr.c (shared library code used well beyond this feature) so the bulk-insert pass can either pre-reserve capacity before mutating, or cleanly roll back an already-inserted prefix on a later element's failure; (2) add a re-derive-and-renotify recovery path that periodically scans c->jobmgr for jobs tagged with an array-submit LSN that never got a HOOK_DETAIL_JOB_NEW fire, and re-fires the hook for them once discovered. User decision (2026-07-16, via kiro-validate-impl's final GO/NO-GO gate): ship job-array-single-wal-record as GO with this bug filed for tracking, rather than blocking the spec on a fast-follow fix or holding it NO-GO -- trigger condition is narrow (a resource-exhaustion failure at the exact moment of the bulk-insert pass, not ordinary construction-phase OOM) and every other validation dimension (mechanical, design/boundary, cross-task integration) came back clean.",
|
||||
"tags": [
|
||||
"job-array-single-wal-record",
|
||||
"restore_job_array",
|
||||
"bulk-insert",
|
||||
"partial-visibility",
|
||||
"requirement-2.3",
|
||||
"requirement-9.4",
|
||||
"requirement-9.5",
|
||||
"requirement-9.6",
|
||||
"key_del",
|
||||
"kv.c",
|
||||
"kvmgr.c",
|
||||
"open-gap",
|
||||
"kiro-validate-impl"
|
||||
],
|
||||
"related_bugs": [],
|
||||
"occurrences": 1,
|
||||
"last_seen": "2026-07-16T02:05:00Z"
|
||||
}
|
||||
]
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue