Rongsong Shen
d26ff8870d
The buglog was one 27K-line JSON file whose fields covered only symptom,
root cause and fix, as free text: 2% of entries named the code version a
bug was found on, 12% the build profile, 36% a way to reproduce, and 58% of
the file was OpenWolf hook auto-detections ("Missing guard clause",
"Function not marked async" on .md files).
- .claude/skills/buglog/bug-record.md: the record standard. Eight
sections (Summary as the title, Description, Environment, Reproduction;
Root cause, Fix, Verification, References once fixed), each with the
content it must contain -- e.g. Environment needs the OS, the libc the
failing binary loads (from its ELF interpreter: glibc 2.42 in the nix
shell vs 2.40 on the host), toolchain, build profile and commit.
- .wolf/bugs/<id>.md: 500 records migrated from buglog.json; every field
verified to round-trip. They carry legacy: true and an explicit "not
recorded" where the old log had no such field; component derived from
file paths, severity from keywords (else unknown), fix_commits from
hashes in status_note/fix that git resolves (91 of 463 fixed).
- .wolf/buglog-archive.json: the 710 hook auto-detections, verbatim.
- .wolf/buglog.json is now an index generated from the records, keeping
the fields OpenWolf's hooks and `openwolf bug search` read.
- scripts/buglog.py rewritten: new (template + Environment collected from
the machine and --binary), check, status (refuses a status the record
doesn't support; fixed needs Root cause, Fix, Verification and
--commit hashes git resolves), note, recur, set, show, list, triage,
env, index. 29 unittest cases in scripts/tests/test_buglog.py.
- scripts/git-hooks/pre-commit runs `check --staged`: incomplete staged
records, a legacy record hand-changed to fixed without the fix-time
sections, a new record claiming legacy: true, or a stale index refuse
the commit.
- scripts/gitea-sync.py reads the records: issue body = the record's
sections plus linked fix commits (origin remote, GITEA_COMMIT_REPO to
override), and a body hash so an issue's text is refreshed when the
record changes.
- bug-1866 rewritten to the full standard as the worked example.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
12 KiB
| id | status | status_updated | status_note | component | severity | timestamp | last_seen | occurrences | files | tags | related | legacy | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| bug-1691 | fixed | 2026-09-24 | controller | unknown | 2026-08-25T12:00:00Z | 2026-09-19T09:00:00Z | 1 |
|
|
|
true |
bug-1691: xce-audit-invariants finding: WAL_JOB_CKPT silently drops job.ckptinfo (the…
Description
xce-audit-invariants finding: WAL_JOB_CKPT silently drops job.ckptinfo (the worker/scheduler-reported checkpoint info string) on a controller restart replay -- same bug class as bug-1678 (WAL_JOB_DISPATCH/job.allocs), bug-1687 (WAL_JOB_START/job.pidinfo+start_time), bug-1689 (WAL_JOB_FINISH/job.exit_info+finish_time), and bug-1690 (WAL_JOB_RUSAGE/job.run_usage). on_job_ckpt() (job.c) sets job->job.ckptinfo via makeString(), but the WAL_JOB_CKPT fact -- purely hook-driven via persist_job_hook_fun's HOOK_DETAIL_JOB_CKPT arm (on_job_ckpt() itself never calls a persist_submit_job__fact() function directly, unlike DISPATCH/START/FINISH) -- only ever carried the shared wal_jobstate_t{jobid, state} payload. Neither apply seam restored ckptinfo -- it stayed NULL after restart. job->job.ckptinfo (xce/types.h) is a plain NUL-terminated char, NOT the richer xce_ckptinfo_t/CheckpointInfo capnp struct used elsewhere for a different purpose (that struct is on_job_ckpt()'s OWN incoming-report parameter type).
Environment
Not recorded (migrated from buglog.json).
Reproduction
Not recorded (migrated from buglog.json).
Root cause
persist_job_hook_fun()'s job-fact switch (persist.c) routed HOOK_DETAIL_JOB_CKPT through its generic default: arm (shared with STOP/RESUME/UPDATE/PREASON), which captures only {jobid, state} from the job_t* -- j->job.ckptinfo, freshly set by on_job_ckpt() moments earlier under the same write lock this AFTER_EVENT fires inside, was never read or encoded. Both apply seams -- apply_committed_entry()'s shared WAL_JOB_START/FINISH/.../DISPATCH fall-through group (primary boot replay AND live commit-advance apply) and apply_wal_entry()'s identical shared group -> restore_job_state() (job.c, state-only) for standby catch-up -- never restored ckptinfo, matching the exact same class of gap bug-1678/1687/1689/1690 already fixed for allocs/pidinfo+start_time/exit_info+finish_time/run_usage. Confirmed via CodeGraph source read that on_job_ckpt() is hook-driven (fires xce_run_hooks only, no direct persist_submit_job_*_fact() call site) -- a genuine divergence from DISPATCH/START/FINISH's direct-call-site pattern, closer to bug-1690's RUSAGE precedent (also purely hook-driven) -- verified against the real code before implementing, per this task's explicit instruction. Also confirmed on_job_ckpt() never changes job.state (only job.ckptinfo), so a naive copy of the START/FINISH/DISPATCH pattern (gating adoption on a forward JOB_TRANSITION_APPLIED) would have made the fix dead code on every real invocation, mirroring bug-1690's exact same discovery for WAL_JOB_RUSAGE.
Fix
FIXED (2026-08-25). This is the simplest of the five field-parity fixes -- job.ckptinfo is a plain string, so NO capnp struct was needed at all (unlike bug-1678/1687/1690's capnp wrappers, though it does share bug-1689's 'native, non-capnp trailer' style). Mirrors WAL_QUEUE_CTRL's own established [header][name NUL-terminated] wire shape (persist_queue_hook_fun / apply_wal_entry's WAL_QUEUE_CTRL case, read as the reference implementation before designing this fix) exactly: the ckptinfo string is appended as a NUL-terminated string directly after the wal_jobstate_t header, bounded by the WAL entry's own payload_len (no separate length prefix). persist.c gains decode_ckptinfo() (a native memcpy-with-NUL-bound-check decode, mirroring decode_jobexitinfo()'s bug-1689 native-trailer style) and a new HOOK_DETAIL_JOB_CKPT case in persist_job_hook_fun() -- split out of the generic default: arm -- that encodes j->job.ckptinfo (when non-NULL) as trailing bytes appended after the wal_jobstate_t header, mirroring the rusage_buf/rusage_len construction bug-1690 already added to this same function for WAL_JOB_RUSAGE (both are hook-driven, unlike DISPATCH/START/FINISH's direct persist_submit_job_fact() call sites). src/include/wal_types.h's WAL_JOB_CKPT enum value gains a doc comment documenting the new payload shape and backward-compat contract, mirroring every other WAL_JOB entry's existing convention. Both apply seams updated: apply_wal_entry() (standby catch-up) splits WAL_JOB_CKPT out of its shared state-only group into its own case (mirroring bug-1690's WAL_JOB_RUSAGE split), decoding trailing ckptinfo bytes via decode_ckptinfo() and calling new restore_job_ckpt_state() (job.c, restore_job_rusage_state()'s sibling). apply_committed_entry() stays in its existing shared job-fact group but -- mirroring bug-1690's WAL_JOB_RUSAGE design exactly, for the identical reason -- decodes/adopts ckptinfo UNCONDITIONALLY, before the JOB_TRANSITION_APPLIED gate: a checkpoint report never changes job.state, so gating adoption on a forward state transition would make it dead code for the realistic, only-ever-occurring case (every real WAL_JOB_CKPT fact hits JOB_TRANSITION_NOOP_DUPLICATE, since js.state is captured verbatim from the job's own unchanged current state). A payload of exactly sizeof(wal_jobstate_t) (any WAL_JOB_CKPT entry written before this fix, or one fired while job.ckptinfo was still NULL at write time) decodes no ckptinfo on both seams, matching pre-fix behavior exactly -- full backward compatibility.
Adoption is a plain free-and-replace (dereference() the prior xstring value, adopt a fresh makeString()'d copy), NOT a merge -- this DIVERGES from bug-1690's WAL_JOB_RUSAGE fix, which required a merge specifically because a naive free-and-replace via the WRONG release function (the capnp-generated free_JobRunusage_ptr(), written for plain malloc'd fields) would free a non-malloc'd, RTTI-managed xstring pointer. job.ckptinfo IS xstring-backed (makeString(), xstring.c) exactly like RUSAGE's host_name was, and CAN be the job's own live value when the primary's own live apply pump re-applies a fact moments after on_job_ckpt() already set it -- but dereference() (the correct RTTI release verb, used here instead of free()/FREEUP) is safe regardless of whether the pointer is 'old' or coincidentally 'the current live one', since it decrements a refcount rather than raw free()ing a non-owned pointer. This distinction was verified by reading xstring.c's actual makeString()/dereference() implementation before choosing the release strategy, not assumed from the RUSAGE precedent.
Regression tests: new src/controller/testsuite/test_wal_job_ckpt_fact.c (7 tests, mirroring test_wal_job_rusage_fact.c's 'unconditional decode independent of transition result' structure plus test_wal_job_finish_fact.c's simpler real-production-call pattern, wired into src/controller/meson.build and testsuite/controller_test.c): (1) apply_committed_entry sets ckptinfo when state does NOT change (JOB_TRANSITION_NOOP_DUPLICATE) -- this suite's most important test, proving the unconditional-decode design choice, hook fires 0 times; (2) apply_committed_entry sets ckptinfo on a genuine forward transition, hook still fires once; (3) apply_committed_entry backward-compat with an old-format payload; (4) apply_wal_entry sets ckptinfo, fires no hook; (5) apply_wal_entry backward-compat; (6) direct-payload restart-replay (WAL_JOB_SUBMIT + new-format WAL_JOB_CKPT via persist_wal_async + persist_recover()); (7) test_wjc_real_on_job_ckpt_survives_restart -- drives the REAL production on_job_ckpt() path (cluster.c's NotifyData_data_ckptinfo dispatch's only callee) with a real xce_ckptinfo_t, through a real WAL-write + persist_recover() restart cycle, asserting ckptinfo survives the restart intact -- the audit's exact confirmed root cause. Test 7 required passing a real cluster_t* (not NULL) as persist_init()'s 3rd argument -- unlike test_wjf_real_on_job_finish_survives_restart's identical-looking fixture (a direct-call-site fix needing no hook registration), on_job_ckpt() is hook-driven and depends entirely on persist_start() registering persist_job_hook_fun on cluster.hooksys (gated on p->cluster != NULL) to reach the WAL at all -- caught via a genuine RED-signal debugging pass (the real-path test initially failed even with the fix applied, isolating to this exact fixture gap, not a fix defect).
RED->GREEN independently verified live (not just narrated): git stash push -- src/controller/job.c src/controller/persist.c (keeping the new test file, wal_types.h, meson.build/controller_test.c wiring), incremental rebuild via the synchronous xce-build.sh script, ran the full controller_unit_test suite -- 1167 Tests 7 Failures (the 2 pre-existing bug-1561 test_config_repl.c failures plus all 5 new-behavior WAL_JOB_CKPT tests failing with the exact expected 'must set job.ckptinfo' mismatch; the 2 backward-compat tests correctly PASS unchanged). git stash pop, incremental rebuild, ran the isolated new test group directly against the binary (ASAN_OPTIONS=detect_leaks=0 ./controller_unit_test -t test_wal_job_ckpt_fact, needed because LeakSanitizer's atexit-based leak check bypasses libc's normal exit() stdio-flush path on this binary, silently discarding all buffered PASS/FAIL output when leaks are present -- a pre-existing, suite-wide fixture characteristic unrelated to this fix, confirmed identical on the RUSAGE suite: 78 leaked allocations in isolation, same fixture-leak pattern, not a regression) -- initially 6/7 passed, with test_wjc_real_on_job_ckpt_survives_restart failing; root-caused to the persist_init() NULL-vs-&c fixture gap above, fixed the test, reran -- 7 Tests 0 Failures. Full controller_unit_test suite re-run via xce-build.sh after all fixes: 1167 Tests, 2 Failures -- exact bug-1561 signature match ('Expected 2 Was 3' / test_conf_trim_keeps_only_n_versions, 'Expected 0 Was ' / test_conf_get_saved_tag_no_config in test_config_repl.c), zero new failures, zero ASAN heap-corruption findings (grepped the full test log for heap-buffer-overflow/invalid-free/use-after-free/SEGV/Aborted/double-free -- none found).
ENVIRONMENT NOTE: this session's build/test runs were repeatedly disrupted by the /tmp filesystem filling to 100% (0 bytes free), traced to ~15,437 orphaned mkdtemp() scratch directories (xce_ckpt_/xce_hook_test_/xce_e2e_wal_/etc., ~658GB total) accumulated across many prior sessions' test runs whose cleanup never ran (each interrupted mid-flight by the same ENOSPC condition in a compounding cycle). Cleared via rm -rf /tmp/xce_* (verified via /proc//fd that no live process held any of them open) before the final, valid GREEN run -- unrelated to this fix's correctness, but a genuine environment hazard worth flagging for other sessions sharing this machine.
Verification
Not recorded (migrated from buglog.json).
References
- Related: bug-1678, bug-1687, bug-1689, bug-1690, bug-1561