chore(wolf): sync anatomy/cerebrum/memory/hooks/token-ledger bookkeeping
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AgYRU3zDTqNGV5Jk8aiKLg
This commit is contained in:
parent
ac46bfa649
commit
a16776bdbd
5 changed files with 3228 additions and 18 deletions
|
|
@ -128,6 +128,11 @@
|
|||
- **WAL field-parity fix family (bug-1678/1687/1689/1690/1691/1692, controller persist.c/job.c)**: when a hook-driven WAL fact (payload built inside `persist_job_hook_fun`, not a direct `persist_submit_job_*_fact()` call site) needs a new field added to survive replay: (1) check whether the field's own release function uses `FREEUP()`/`free()` (plain malloc/strdup/calloc — safe to free-and-replace, e.g. `JobAlloc`/`JobPIDInfo`/`JobPendReason`) vs `dereference()` (xstring/RTTI-backed, e.g. `ckptinfo`) vs "can be the SAME live object the apply pump re-applies moments later" (needs a MERGE, not free-and-replace — only `WAL_JOB_RUSAGE`'s `run_usage` needed this, because `update_host_runusage()` mutates it in place before the WAL fact is even appended); (2) check whether adoption must be gated on `JOB_TRANSITION_APPLIED` or unconditional — if the setter never changes `job.state` (true for CKPT/RUSAGE/PREASON, false for START/FINISH/DISPATCH), gating makes the fix dead code since every real replay hits `JOB_TRANSITION_NOOP_DUPLICATE`; (3) **check whether the field has a genuine CLEAR-to-NULL operation** (only `pend_reasons`/`do_clr_preason` in this family so far) — if so, "trailing bytes present or absent" (bug-1690/1691's convention) cannot distinguish a legitimate clear from an old-format payload; needs an explicit presence byte instead. Always re-verify (2) and (3) from the real setter code, never assume the newest precedent's shape applies unchanged.
|
||||
- **capnp-c's generated decode path does not validate content, only bounds via `capn_getp`/`capn_resolve`**: passing well-formed-length-but-semantically-garbage bytes to a `decode_X()`/`read_X()` pair (e.g. `decode_JobPendReason`) does NOT return NULL or error — it silently produces a zero-valued struct. A "decode failure returns NULL" defensive branch (`decode_X() == NULL`) is only reachable via allocation failure or an upstream length/bounds guard (e.g. `payload_len` too short to even attempt the read) — never via corrupt-but-present bytes. Don't write a regression test assuming garbadge bytes will fail decode; it will silently pass through and the test either false-passes or fails for a confusing, misleading reason (`Expected 4 Was 0` — the "corrupted" value decoded as an all-zero struct, not a NULL).
|
||||
|
||||
- **(2026-08-25, bug-1695) `meson.build`'s `controller_unit_test_src` deliberately does NOT link `myapp.c` (or `main.c`) — only `startup_validation.c`, `ha.c`, `cluster.c`, `persist.c`, etc.** `myapp_run()` is documented as an "untestable integration entry point" (test_ha_startup.c's own Task 5.1 comment) requiring a live network socket, so it's excluded from the unit-test binary entirely. If you need to unit-test logic that currently lives inline inside `myapp_run()`, do NOT just add a non-static function to `myapp.c` and forward-declare it in a test file — it will compile but fail to LINK (`undefined reference`) because `myapp.c` itself never gets compiled into `controller_unit_test`. Instead, extract the logic into `startup_validation.c` (already linked into both `xce_controller`/`main.c` AND `controller_unit_test`, and explicitly documented as existing "for use in main.c and the controller unit test suite") and have `myapp.c` forward-declare + call it — exactly mirroring the pre-existing `validate_startup_config()` pattern (defined in `startup_validation.c`, forward-declared at the top of `myapp.c`, called from `myapp_run()`). Confirmed via a real RED/GREEN round-trip: extracting into `myapp.c` first produced a clean compile + link FAILURE across all of `controller_unit_test`'s ~90 test files; moving the same function body to `startup_validation.c` fixed it with zero other changes.
|
||||
- **(2026-08-25, bug-1695) `ha_state_t` has TWO separate term fields that look similar but are written by different code paths and must not be conflated**: `ha->term` (top-level, `_Atomic uint64_t`, `ha.h:157`) is the adapter's own copy, written by `ha_restore_term()` at boot and by `raft_cb_persist()` on every `on_persist` callback; `ha->raft.term` (nested inside the embedded `raft_state_t`, `ha.h:198`/`raft.h:29`) is the raft LIBRARY's own internal term, seeded ONCE at `ha_init_raft()` time from `ha->term` (`rcfg.initial_term = atomic_load(&ha->term)`) and thereafter updated only by the library itself (`raft_start_election`/`raft_on_vote_request`/`raft_on_append_entries_request`, all via the SAME `on_persist` callback that also updates `ha->term`) — so they stay in sync after boot, but the ONLY correct place to read "the term as of right after this boot's restore" is `ha->raft.term` captured immediately after `ha_init_raft()` returns (not `ha->term`, and not before `ha_init_raft()` runs). myapp_run's bootstrap check reads `ha->raft.term` specifically (not `ha->term`) — a fix that snapshotted the wrong field would silently do nothing.
|
||||
- **(2026-08-25, bug-1695) A `--chaotic` HA soak's per-round "wait_for_new_primary window" log line is a WEAK signal on its own — always cross-check the raw controller logs (`grep -n "CTRL_PRIMARY\|CTRL_STANDBY\|PreVote round" logs/ctrl-*.log`) before trusting it.** The chaotic driver's own per-round polling window (~90s, 30×3s probes) can time out ("NO new primary within window") even when a genuinely NEW node DID win a real election shortly after — the log line only reflects whether the driver's OWN bounded poll happened to catch it, not whether the underlying HA mechanism worked. In this session's soak, the fixed run showed "NO new primary within window" on 5/6 rounds (superficially similar to the unfixed baseline's 6/6) while the RAW logs proved something structurally different was happening: term genuinely progressing 1→2→3→4→7 and a different node (`ctrl-2`) achieving a real, log-verified `CTRL_PRIMARY` transition via a won PreVote — vs. the baseline where term was frozen at a single value for the entire run and NEITHER peer ever showed any PreVote/election activity at all, anywhere. **The A/B technique that resolved this ambiguity**: `git stash push -- <the exact files you changed>` (reverts just those files, keeps everything else including the same freshly-built binaries' surrounding tree state), rebuild, re-run the IDENTICAL soak invocation against a fresh scratch dir, compare raw logs side by side, then `git stash pop` to restore the fix. Far more conclusive than trusting either run's summary line alone.
|
||||
- **(2026-08-25, bug-1695) A chaotic-soak `expect-job`/`verify_classify` "COMMITTED JOB LOSS" verdict is NOT self-verifying — cross-check the raw WAL with `xce_test`'s `waldump <data-dir>` REPL command (built into `xce_test`, not a separate binary) before treating it as a real controller data-loss bug.** `verify_classify` reads through the apply-pump/materialized-jobmgr layer (an RPC-level `expect-job` query), which per already-documented bug-1487 can legitimately lag behind the raw committed WAL for several seconds after a genuinely-new election (the new leader's OWN-term commit-barrier gate, `ha_arm_own_term_gate`, requires a fresh majority ack for its own-term no-op before folding in prior-term entries) — a RETRY-EXHAUSTED "not found" there does not by itself prove the entry is actually missing from the durable log. `walcompare`'s `committed_divergence` count (also built into `xce_test`) is the AUTHORITATIVE data-safety signal — it stayed 0 in this session's soak even while `verify_classify` counted 6 losses, and a direct `waldump` of all 3 nodes' `data/wal/seg-*.wal` files confirmed the ONE specific jobid in question genuinely does not appear on ANY node's WAL at all (not divergent between nodes — universally absent), tracing it to the cluster's very first cold-boot config-bootstrap window rather than anything this specific fix touched.
|
||||
|
||||
## Do-Not-Repeat
|
||||
- **When translating a `fp_ok`-tracked, per-step-gated failure-detection pattern (like `xce-build.sh`'s dirty-fingerprint computation) into a Make `$(shell sh -c '...')` one-liner, do NOT collapse two commands into `{ cmd1; cmd2; } >"$$fp_tmp"` and gate the whole group with one `if`** — a POSIX sequential group's exit status reflects only its LAST command, so `cmd1`'s failure is silently discarded whenever `cmd2` still succeeds (2026-08-21, bug-1612: `git diff HEAD` failing was swallowed because the trailing `git ls-files | sort | xargs cat` pipe still exited 0, producing a fabricated fingerprint instead of degrading to empty). Give each step its own explicit `if ! step; then fp_ok=0; fi` (or `&&`-chain), exactly mirroring the reference script's discipline, even inside a cramped one-liner. Caught by adversarial code review with a targeted failure-injection test (wrapper `git` on `PATH` that fails only the targeted subcommand) — that test pattern is the reusable way to verify this class of fix.
|
||||
- Don't run `clang-format -i` on whole existing files here — the repo isn't clang-format-clean at baseline, so a full reformat buries the real diff; recovering via `git checkout HEAD -- <file>` on a file with other uncommitted work destroys that work irrecoverably.
|
||||
|
|
@ -191,3 +196,4 @@
|
|||
- **When offloading a newly-discovered delaymgr/iomgr blocking-I/O violation, "vtask_enq_classified(pool, task, VTASK_LIVE_BLOCK)" is NOT automatically the right destination even though it's this codebase's most commonly cited fix-pattern name — check whether the SAME pool already carries a lock the new work also needs, via a DIFFERENT already-offloaded caller** (bug-1694, checkpoint_task_fn offload, 2026-08-25). This codebase actually has TWO distinct offload destinations, easy to conflate: the vtaskmgr N-thread pool (`vtask_enq_classified`, VTASK_LIVE_BLOCK/VTASK_REQ_BLOCK — used for *bounded* handler work, e.g. `persist_apply_pump_kick`'s apply-pump drain and `commit.c`'s `dispatch_continuation`) vs. the dedicated SlowLane (`slowlane_submit`, `c->slowlane`, `n_workers` separate pthreads — used for genuinely *long/unbounded* blocking I/O: snapshot send, config-push, reconnect, catch-up-streaming, demotion-truncate). `ha-async-io.md`'s own table already states this split ("vtaskmgr must NOT do: long/unbounded blocking I/O ... starves the pool") but it's easy to miss which category new work falls into. Concretely: `persist_apply_pump_run()` (the live WAL-apply walk) holds `apply_pump_lock` across its ENTIRE walk and is ITSELF dispatched onto the vtask pool via `persist_apply_pump_kick()`'s `vtask_enq_classified(..., VTASK_LIVE_BLOCK)` — so any OTHER unbounded, lock-holding work (checkpoint's `write_snapshot()`, also under `apply_pump_lock`) put on that SAME pool would risk starving it (a worker parked on the lock for the whole snapshot-write duration, stealing capacity from commit-continuation/apply-pump liveness work) — not a classic deadlock, but the exact "starves the pool" failure mode the split exists to prevent. The fix used the slow lane instead, exactly mirroring `ha_snapshot_do_send()`'s pre-existing (already-in-production, default-ON) call to the SAME function (`persist_checkpoint()`) from a slow-lane thread — i.e. the safest new offload destination is often "wherever an already-proven-safe sibling call already runs the identical function from," not a fresh pattern match on a fix-recipe's literal wording. **General technique: before picking an offload destination, grep every existing caller of the SAME blocking function/lock for its current thread context — if a sibling caller already runs it safely from candidate destination X, reuse X; if a DIFFERENT already-offloaded consumer of the same lock lives on candidate destination Y, that's evidence AGAINST Y.**
|
||||
- **A buglog entry's own "root cause: NOT YET ROOT-CAUSED" (or similarly hedged) status is a real signal to re-derive the causal claim from primary evidence before citing it as settled, especially when later re-occurrences of the SAME symptom show it firing on completely unrelated diffs.** bug-1429 (cited by `config_repl.c`/`ha_convergence.c`'s catchup/demotion offload-flag comments as "the vtask pool that deadlocked controller_unit_test on 2026-07-10") is itself logged as never root-caused, and its OWN later occurrences (2026-08-20, 2026-08-23) reproduced the identical TIMEOUT/hang signature on diffs that touched neither the vtask offload code nor persist.c's checkpoint path at all — "strongly corroborates this is a pre-existing, systemic, timing/order-dependent hang in the controller_unit_test harness," per the buglog's own later entries, not proven caused by VTASK_LIVE_BLOCK dispatch specifically. Don't repeat a downstream comment's causal claim as fact in a new design doc/comment without checking the buglog entry it cites — the comment can be more confident than the evidence it's citing.
|
||||
- **`controller_unit_test` (the main shared meson.build `controller_unit_test_src` binary) does NOT link the real `sched.c`/`dataex.c` at all** (bug-1693, 2026-08-25) — `sched.c`'s own `on_scheduler_join()`/`collect_confvars()` and `dataex.c`'s `notify()` collide at link time with fake local definitions several OTHER files in that same binary already provide (`test_commit_barrier_reply.c`, `test_worker_join.c`), so `test_config_repl.c` instead ships a trivial no-op `handle_scheduler_leave(cluster_t *c) { (void)c; return 0; }` stub purely to satisfy the linker. **Any new/changed behavior added to a real `sched.c` function is therefore SILENTLY UNTESTED by any `controller_unit_test`-based test that calls that function by name** — the call resolves to the stub, not the real implementation, with no compile/link error to flag the mismatch (confirmed empirically: a new regression test asserting sub-second wake timing after calling `handle_scheduler_leave()` passed compilation and linking fine, then failed by taking the full 5s deadline — the stub had silently swallowed the call). `nm '<obj>.o' | grep ' T <symbol>$'` across every `.o` in the target's build dir is the fast way to confirm which TU actually provides a given symbol in a given binary before trusting a test's result. The two standalone binaries that DO link the real `sched.c` (`sched_push_unit_test`, `sched_worker_change_unit_test` — see their own `meson.build` comments for the collision rationale) don't link `cluster.c`, so neither binary alone can exercise a fix that spans both files for real; bug-1693's fix split verification across both (a direct-call test against `cluster.c`'s real new function in `controller_unit_test`, plus a small counting-spy test in `test_sched_worker_change.c` proving `sched.c`'s real caller invokes it) rather than inventing a third binary that links both (would need to duplicate most of `controller_unit_test_src`'s already-large dependency list to satisfy `cluster.c`'s own huge fan-out). General lesson: when a fix spans a function in `sched.c`/`dataex.c` AND a function in any of the ~15 other files real-linked into `controller_unit_test_src`, check with `nm`/grep for a same-name fake stub in a sibling test file in that binary BEFORE trusting a test that calls the `sched.c`/`dataex.c` side by name — it may be exercising a stub, not your change.
|
||||
- **(2026-08-25, bug-1695) A bootstrap "is a leader already known" fallback check must compare a term against a snapshot taken at THIS boot's start, never against a bare constant like `> 0`, whenever the compared value is restored from durable on-disk state that only ever increases (Raft terms, monotonic counters, etc.).** `myapp_run()`'s `raft.term > 0` fallback was trivially true forever after the cluster's first-ever election because `ha_restore_term()` restores a persisted, never-decreasing term at every boot. Fixed by capturing `term_at_boot = ha->raft.term` immediately after `ha_init_raft()` seeds it (right after restore, strictly before any new activity could legitimately advance it) and comparing `> term_at_boot` instead of `> 0` — the general pattern: "is X known/true" checks against monotonic restored state should ask "did X change SINCE I booted," not "is X nonzero," whenever a nonzero-forever steady state is a normal, expected outcome of prior successful operation (not an error state).
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue