chore(wolf): update session tracking state

anatomy.md/cerebrum.md/memory.md/token-ledger.json/hooks state covering
the design-review fix pass (tlog activation, core-isolation audit,
schedAdmin admission gate, resource-value-verification doc/comment gaps)
and task 12.5's fresh completion verification.
This commit is contained in:
Rongsong Shen 2026-08-19 11:01:16 +00:00
parent aa42752d35
commit ffa8bc110f
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
5 changed files with 5156 additions and 29 deletions

View file

@ -27,6 +27,7 @@
- **Fixing a busy-spin/livelock can unmask a latent notification gap (bug-1564)**: throttling an unbounded retry loop (bug-1563's fix) removed an *accidental* fast-repoll that had been quietly covering for `persist_flush()` never firing `on_durable_advance` (only the async `fsync_worker` did). A deterministic (3/3) new test failure appeared immediately after — always suspect "the old code accidentally covered for a real gap via sheer retry speed" when a correctness fix (removing a spin/race) makes a previously-green test start failing 100% of the time; that's a signal to look for a missing notification/wakeup, not to revert the fix.
- **nix devShell Python packages are `PYTHONPATH` entries, not merged into `python3`'s own site-packages** (found building a standalone-installable xceadmin wheel): `python3 -m venv --system-site-packages` only inherits the base interpreter's OWN site-packages dir, which is empty in this project's nix shell (`python3Packages.click`/`pycapnp`/etc. are each a separate `/nix/store/...` path added via `$PYTHONPATH`, confirmed via `nix develop … --command sh -c 'echo $PYTHONPATH'`). A `--system-site-packages` venv therefore silently CANNOT see them (`ModuleNotFoundError` at runtime, discovered live). Fix: skip venv/pip entirely — extract the wheel directly (`python3 -m zipfile -e`) and bake the *resolved* `$PYTHONPATH` string into a wrapper script's `export PYTHONPATH=...` at generation time (expand now, not `${PYTHONPATH:-}` at runtime) — mirrors `src/xceadmin/meson-install.sh`'s existing nix-packaging wrapper, which already solved this correctly; I should have grepped for that pattern before reaching for venv the first time.
- **`src/tools/xce-build.sh`'s `setup_build()` does `rm -rf "$BUILD_DIR"` for every single-component invocation** (bug-1565, FIXED): `/xce-build xceadmin` after `/xce-build controller` in the same dir used to silently delete `xce_controller`/`xce_test`/etc. Fixed by an opt-in `--incremental` flag (`meson setup --reconfigure` in place instead of wiping) — deliberately NOT the default, since this project's capnp-generation `custom_target` has known missing-dependency-edge races that a full wipe papers over as a loud, retry-fixable failure (skipping the wipe could turn the same gap into silent stale-generated-code instead). `/xce-build all` (build everything together) remains the simplest option when you don't specifically need incremental speed.
- **A never-joined background thread reading a stack-local test fixture is a latent ASAN stack-use-after-return, exposed only by test ORDERING, not by the thread's own logic** (2026-08-19, scheduler-core design-review follow-up). `sched_sim_driver_run()` (sim_loop.c) spawns `sim_picker_thread_fn` on a real pthread that is, by this codebase's own documented design, "never joined or cancelled... no sim-driver teardown/fini function exists yet" — it loops forever on `cbuf_get(&sim->core.wbuf, ...)`. Every existing white-box test in `test_sim_event_loop.c` already called this with a stack-local `xce_sched_sim_driver_t sim;`, which was ALWAYS a latent stack-use-after-return once the test function returned — it just hadn't been caught, because no test's stack frame had yet been reused by a later frame in a way ASAN's shadow-memory detector caught. Simply INSERTING one new test between two existing ones (no logic change to the existing tests) shifted the stack-reuse pattern enough to make it fire deterministically (reproduced twice). Fix: change the fixture from stack-local to `static` storage — safe here specifically because (a) `sched_sim_driver_start()` unconditionally `memset()`s the whole struct as its first action so no stale-data leak across runs, and (b) each affected test function is called exactly once per process (verified via grep, no re-entrancy) so a second call's memset can never stomp a still-live zombie thread's cbuf mutex/condvar state from a first call — `static` would be UNSAFE to apply to a helper called more than once while its first zombie thread might still be alive. General lesson: when a background thread spawned by code-under-test is documented as "never joined," any test giving that code a stack-local fixture is a ticking ASAN time bomb regardless of whether it has fired yet — check for exactly this "never joined/cancelled" phrasing in the target function's own comments before adding a new test that changes stack layout/ordering in the same file.
- **A pipeline into `head -N` can SIGPIPE-kill the upstream script early** under `set -o pipefail` (bit twice this session: once with `timeout N cmd | head -N` leaving a stale background process running for hours after the harness reported the task "complete", once with `xce-build.sh ... | head -3` silently truncating a build before it finished writing its wheel). If you need to sanity-check just the first few lines of a long-running command's output, redirect to a file and `head` the file afterward instead of piping directly into `head`.
- **`sim_loop.c` (and its `*_isolation`-audited siblings) genuinely has zero line-budget headroom** — its own file header says so explicitly. When a fix needs new logic there, split a new sibling file (bug-1567's `sim_picker.c`, following the exact precedent chain `sim_loop.c`→`sim_record.c`→`repl.c`→`repl_break.c`/`repl_info.c`→`repl_params.c`) rather than growing it even slightly. This requires wiring the new file into: `scheduler_srcs` AND `sched_sim_isolation_lib` (both `src/scheduler/meson.build`), a new `XCE_SIM_*_PATH` macro (`testsuite/meson.build`) consumed by `test_sim_isolation.c`'s 3 per-file text-scan functions, AND every white-box test TU that `#include`s `sim_loop.c` directly (9 files this session — grep `#include "../src/sim_loop.c"` in `testsuite/` to find them all) needs the new file `#include`d too or the link fails on the new extern. Mechanical but real work — budget for it.
- **`controller_unit_test`'s intermittent SIGSEGV (bug-1553, ~1-in-3 full runs) is a known pre-existing flake**, not a new regression — reproduce standalone (`./src/build.dbg/controller/controller_unit_test`, no `meson test` wrapper) a few times before suspecting your own change; `meson test`'s `MALLOC_PERTURB_` randomization per invocation is what makes it flaky, not deterministic per-build.
@ -37,6 +38,7 @@
- **apiserver's `main.go` only calls `RegisterToBackend` when `BackendYamlConfig.Address != ""` — checked BEFORE `Peers` is ever consulted** (`cmd/main.go:58`). Setting `app.yaml`'s `backend.peers` to a fully-populated HA candidate list while leaving `backend.address`/`backend.port` blank (reasoning "Peers takes priority inside RegisterToBackend anyway, per `backend/config.go`'s own doc comment") is NOT sufficient — the apiserver silently boots in permanent "stub mode" (`logger.Info("Backend not configured")`) and never even attempts a connection, `peers` or not. Caught live while building `xce-deploy-ha`'s new apiserver step (resource-value-verification task 3.1): always populate `address`/`port` too (e.g. the first `--peers` candidate's host:port) even when also setting `peers` — belt-and-suspenders, not redundant, because the two fields gate two different things (whether to connect at all, vs. which candidate(s) to try).
- **resource-value-verification task 4.1's scheduler trace-log exact format** (`xce_core_push_events()`'s `XCE_CORE_EVENT_RESVALS_DELTA` case, `src/scheduler/src/core_events.c`, right after `zarray_put(&(w->resvals), res->idx, rv->val)`): `TRACE("resval applied host=%s resource=%s value=%f", ...)` for numeric (`Value_u_fvalue`) resources, or `value=%s` for `Value_u_svalue` ones — live-confirmed via `xce-deploy-ha` + `xce_test` worker-context `join`/`resupdate -n r15m -v 12345.5` producing exactly `resval applied host=task41-worker resource=r15m value=12345.500000` in `scheduler.log` at `-vvv`. Task 6.3 (scheduler-side log-tail check) MUST grep for `resval applied host=<H> resource=<R> value=<V>` — labeled `key=value` fields, no other fixed text to depend on. `%f` on a normal resource value produces trailing zeros (e.g. `12345.500000`), not the input's original decimal precision — task 6.3's comparison should parse-and-compare as float, not do a literal string match against the injected value string.
- **bug-1572 (NOT FIXED, pre-existing, discovered live task 4.1 round-2): `xce_scheduler` double-frees on a worker's SECOND periodic resource report for the same resource(s), silently dying with no ASAN output** (backgrounded via `setsid nohup ... >/dev/null 2>&1`, so the crash report is lost -- only visible as scheduler.log stopping mid-stream + `handle_scheduler_leave` in the controller log ~1s later). Root cause: `core_events.c`'s `XCE_CORE_EVENT_RESVALS_DELTA` case does `zarray_put(&(w->resvals), res->idx, rv->val)` with NO `MOVE_PTR` (unlike its sibling `core_apply_config_resvals()`, which does `MOVE_PTR(val, nv->val)` first) -- the same `xce_val_t*` stays aliased in the decoded request. `driver.c`'s `release_translated_payload()` then unconditionally frees that request's resvals list even when `applied=1` (its `XCE_SCHED_OP_UPDATE_RESVALS` case has no `if (!applied)` guard, unlike the sibling `XCE_SCHED_OP_UPDATE_CONFIG` case which explicitly skips the free when applied). Net effect: `w->resvals[idx]` is left dangling immediately after every successful RESVALS_DELTA batch; the SECOND update to the same index triggers zarray's killer to free the already-freed pointer. Confirms identically on round-1's code (unrelated to the enum-branch bug fixed in that round) -- a live smoke test that only runs one resupdate quickly after deploy will NOT hit this; anything that waits past one real worker's report interval (xcecd_virtual reports ~every 15s) will. When live-testing anything touching RESVALS_DELTA, either run fast (single update, no wait) or expect this crash and don't misattribute it to whatever fix is actually under test.
- **`src/frontend/Makefile`'s `build-server` target is missing CGO_CFLAGS/CGO_LDFLAGS (bug-1597, NOT FIXED)**: unlike `test-backend` (bug-1558) and `apiserver/meson.build`, `build-server`'s recipe is a bare `go build` with no `-I`/`-L` for `client.h`/`controller.capnp.h`/`libxce.a`, so `nix develop ../ --command make build-server` fails with `fatal error: client.h: No such file or directory` regardless of source changes. Confirmed via git-stash A/B that this reproduces identically with or without an unrelated diff, so don't misattribute it to whatever change is under test. Workaround to actually verify compilation: reconstruct CGO_CFLAGS/CGO_LDFLAGS the same way `test-backend` does (pkg-config for librtti/c-capnproto/openssl/tinylog + `-I$BUILD_DIR/lib`/`-L$BUILD_DIR/lib -lxce`, `BUILD_DIR` defaulting to `src/xce.build`) and run `CGO_ENABLED=1 CGO_CFLAGS=... CGO_LDFLAGS=... go build -o /tmp/apiserver ./cmd/main.go` directly inside `nix develop ../` — see bug-1597 for the exact one-liner.
- **`test.c`'s `set_resval()` leaves `xce_resval_t.name` (and `.val->which`) uninitialized for the `Value_u_svalue` fallback path** (`-n type`/`-n model`/explicit `-t string` on an unknown name) — its switch statement has no `case Value_u_svalue:` arm, and `vals[]` is a non-zeroed stack array reused across REPL invocations, so `.name` can be garbage, not NULL. Pre-existing (predates resource-value-verification's task 2.1/2.2), explicitly flagged as out-of-scope-to-fix in both tasks' code comments (Requirement 1.4's "preserve byte-for-byte" constraint). When building task 2.2's per-name verification loop on top of this, the safe pattern was to track the submitted name in a separate parallel array (`subnames[]`) captured directly from the getopt loop's own `resname` variable (always valid — that pointer is deliberately leaked/never freed, so it stays alive for the whole REPL call) rather than trusting `vals[i].name`.
- **Trap-based cleanup that must survive an async signal (SIGTERM/SIGINT) should combine multiple `pkill -9 -f` calls into ONE alternation pattern, not several sequential invocations, plus a retry-until-verified (`pgrep`) loop** (bug-1575, resource-value-verification task 5.1's `run-resource-verify.sh`). Four separate sequential `pkill -9 -f -- "pattern"` calls inside `cleanup()` worked reliably every time when reached via normal control flow (a direct `fail()`/`exit` call), but non-deterministically left 1-2 of the four target processes running when `cleanup()` instead ran from the EXIT trap fired by an externally-delivered SIGTERM translated via a separate `trap 'exit N' TERM` handler -- the exact failure point (which of the 4 pkill calls was "last to complete") varied run to run, consistent with a timing race in the trap-handler code path rather than a pattern-matching bug (each pattern was independently verified correct). Fix: one combined `pkill -9 -f -- "pat1|pat2|pat3|pat4"` call (fewer fork/exec round trips through the trap handler) wrapped in a loop that re-checks via `pgrep -f` and retries (up to ~2s total) until genuinely clean. Verified reliable across 5+ repeated live SIGTERM-mid-deployment interrupts after the fix (zero leftover processes every time), vs. non-deterministic leftovers before it. General lesson: don't trust a cleanup routine's reliability from its NORMAL-completion path alone -- explicitly live-test the signal-interrupted path too, since bash's async-signal-trap execution context can behave differently from the same code reached via ordinary top-to-bottom flow or a plain function call/exit.
@ -72,6 +74,8 @@
- **A dead `#if 0 /*FIXME*/ ... #endif` block is a signal the disabled code is unsafe as written, not merely "not yet wired in" — don't just uncomment it.** bug-1578: `sched_worker_change()`'s disabled block called `free_name_val_list(&w->host.resvals_to_sched, &w->host.nresvals_to_sched)` right before `xlist_fini(&resvalist)` — but `resvalist.data` and `w->host.resvals_to_sched` are the SAME pointer (the earlier line aliased one to the other's backing array), and `free_name_val_list()` already frees that array itself; uncommenting as-is would double-free. The safe fix null-and-reset the aliased fields directly (`w->host.resvals_to_sched = NULL; w->host.nresvals_to_sched = 0;`) and let `xlist_fini()` be the sole owner of the free — the pattern already proven correct in the sibling `apiserver_worker_change()` (apiserver.c). General lesson: when re-enabling dead code, first work out WHY it was disabled (git blame + read what it actually does to shared/aliased memory), don't assume `#if 0` means "harmless, just needs a flip." Other call sites still doing `free_name_val_list(&(w->resvals_to_sched), ...)` on this same field (dataex.c:435, opget.c:128, reply.c:66, apiserver.c:51) were NOT audited for the identical aliasing hazard as part of this fix — flagged, not yet checked.
- **`xce_test`'s `!<shell-command>` escape (`preprocess_and_run()`, single call site in `main()`'s top-level REPL loop, `test.c` ~line 950-998, backed by a plain blocking `system()`) works IDENTICALLY in every REPL context, including `worker` — no `leave_ctx()`/disconnect happens, unlike moving between contexts via `..`/`cmd_up`.** This is the general pattern for "run an out-of-band check while a stateful REPL session must stay connected": bake a `!`-prefixed line into the script fed to the session (before its `quit`) that shells out to ANOTHER invocation of your own driver script in a hidden internal mode (e.g. `! bash "$0" --internal-<mode> <args>`), intercepted via a dispatch check placed BEFORE the normal arg-parsing entry point at the very bottom of the file. Used to fix bug-1579 (resource-value-verification): the controller-side resval check now runs nested inside the injecting worker session's own `!` escape instead of as a separate xce_test process launched after `quit` already disconnected the worker (which reset host-attrs resources to baseline first, an always-false MISMATCH). Gotchas: (1) the nested process must generally return exit 0 even on partial/expected failure conditions — a nonzero exit becomes the outer session's `g_last_status`/`$?`, and in non-interactive/batch mode `preprocess_and_run()` ABORTS the REPL loop before any later command (including `quit`) runs; surface real failures via a sentinel/marker file the OUTER script checks AFTER the whole session completes, not via the nested process's own exit code. (2) `system()`'s child inherits the invoking process's environment but NOT its parent bash's local (non-exported) shell variables — bake every value the nested invocation needs (paths, host lists, names) as literal, `printf '%q'`-quoted arguments directly into the `!` line's command string, don't rely on env-var passthrough through the `nix develop`/ASAN-wrapped intermediate hops. (3) since accept/reject-style bookkeeping computed by the REPL session itself is only visible as TEXT PRINTED to that session's own stdout — not accessible to a freshly-spawned nested process — have the nested step collect RAW, unfiltered data for every candidate (not just ones later known to be "accepted") and defer accept/reject filtering to the outer script, after it has parsed the whole session's captured output post-`quit`. (4) live-verified no orphan/zombie processes result even when a SIGTERM interrupts the outer driver script WHILE the nested `!`-escape's own sub-session is actively running — bash defers a pending trap until the current foreground command (the whole piped xce_test invocation, nested check included) returns, so the child process tree either completes and exits cleanly on its own or the trap fires right after; confirmed via a full `/proc/[0-9]*/cmdline` scan, not just `pgrep`.
- **Piping scripted commands into `xce_test` (the controller REPL/testsuite binary) with default `ASAN_OPTIONS` loses ALL stdout output when redirected to a file/pipe.** `xce_test` is an ASAN build; `detect_leaks=1` is the default, and this binary has small pre-existing intentional leaks (e.g. `set_resval`'s `calloc`, `cmd_resupdate`'s `strdup` for `resname` — never freed by design, REPL-lifetime scratch data). LeakSanitizer's exit-time report appears to bypass/precede the normal stdio flush when stdout is non-interactive (fully block-buffered), so a redirected-output smoke test showed ONLY the ASAN leak report and none of the actual REPL command output (join/resupdate/resval results), even though the commands genuinely executed (confirmed via the leak stack trace itself). Fix: always run `xce_test` with `ASAN_OPTIONS=detect_leaks=0` (matching the meson test harness's own invocation, which already sets this) whenever piping/redirecting its stdout for a live smoke test — e.g. `nix develop ./src --command env ASAN_OPTIONS=detect_leaks=0 xce_test < cmds.txt`.
- **A fork/subagent dispatched in a prior session that claimed "build/tests in progress, will report back" is NOT reachable in a new session — `ListAgents` will not show it, and its self-report (if logged to memory before the session ended) describes an *intent*, not a completed result.** 2026-08-19: a fork agent from the previous session was recorded as "actively writing tests... then building via xce-build skill and reporting final metrics," but the session ended before it reported. On resuming ("continue"), the scheduler test binaries in `src/xce.build` were still dated Aug 18 14:27 while the source edits were Aug 19 09:18-09:27 — proof the claimed build never actually happened in this environment. Always verify via binary/source `stat` mtime comparison (or just re-run the build) before trusting a cross-session "in progress"/"dispatched" claim, same discipline as the existing "stale/uncommitted tree GO claim" entry above but for cross-session fork continuity specifically, not just tree staleness.
- **A REPL/replay test fixture that reuses the SAME jobid for both a "real" event (job push) and a "cheap padding" event (job state change, added purely to exercise a second event type) accidentally mutates the real job's state, not just decorates the log.** bug-1595 test regression (task 12.5, 2026-08-19): `test_step_echoes_events_before_session_decisions` pushed job 6161 then sent a JOBSTATECHANGE event for the SAME jobid 6161 (PEND->RUN) purely to get a second event type echoed — this genuinely transitioned the real job to RUNNING before the session ran, so the picker's `next_job()` never offered it and the session's own PEND decision (which the test also asserted on) was never produced. The sibling test `test_event_breakpoint_pauses_repl_at_matching_event` already documents the correct pattern in its own comment (`append_jobstatechange_event() establishes: core_events.c WARN+skips an unknown jobid rather than fail, so ... below needs no real job to exist`) — it deliberately used unrelated jobids (111/222) precisely to keep the padding event inert. When adding a "just need any event of type X to be logged" filler event to a fixture that also has a real job in flight, always target an unrelated/nonexistent jobid unless the state transition itself is the thing under test.
- **`[ -n "${arr[key]+x}" ]` (associative-array key-EXISTENCE) is NOT a safe proxy for "the value is meaningful/reachable" when the producer unconditionally sets that key even on failure.** bug-1582 (resource-value-verification task 7.1, round-1 REJECTED, round-2 fixed): `main()`'s record-building loop derived `sched_ok`/`api_ok` from whether `SCHEDULER_OBSERVED[$vname]`/`APISERVER_OBSERVED[$vname]` existed — but `query_scheduler_resvals()`/`query_apiserver_resvals()` (already-approved tasks 6.3/6.4) always set that key, setting it to `""` on a genuine UNREACHABLE/timeout rather than leaving it unset. Key-existence was therefore always true, silently masking every real UNREACHABLE as a MISMATCH (comparing the injected value against an empty string). Fix: derive the "is this layer reachable" signal from the layer's own pre-classified status string (`SCHEDULER_CHECK_RESULTS[$vname]`/`APISERVER_CHECK_RESULTS[$vname]`, which DOES distinguish `UNREACHABLE:*` from `MATCH`/`MISMATCH:*`) instead of from a sibling data array's key presence. General lesson: before using `${arr[k]+x}`/`-v arr[k]` as a "was this checked / did this succeed" signal, verify the producer actually leaves the key UNSET on failure — if it sets it to an empty/placeholder value instead (a common pattern for "always populate every requested key so downstream code doesn't need existence-checks"), key-existence collapses to "was this key in the requested set" a wholly different, weaker signal than "did this succeed". Also: a synthetic test that only calls the pure aggregation function directly with hand-built records (task 8.2's original approach) cannot catch a bug living in the data-marshaling code that BUILDS those records — this class of bug needs the marshaling code itself extracted into a directly-callable function (`build_verdict_records_from_tsv()`) and exercised by its own test.
## Decision Log