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>
14 KiB
| id | status | status_updated | status_note | component | severity | timestamp | last_seen | occurrences | files | tags | related | legacy | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| bug-1692 | fixed | 2026-09-24 | controller | unknown | 2026-08-25T13:10:00Z | 2026-09-19T09:00:00Z | 1 |
|
|
|
true |
bug-1692: xce-audit-invariants finding: WAL_JOB_PREASON silently drops job.pend_reasons (the…
Description
xce-audit-invariants finding: WAL_JOB_PREASON silently drops job.pend_reasons (the scheduler-reported host/resource-based pending-job explanation) 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), bug-1690 (WAL_JOB_RUSAGE/job.run_usage), and bug-1691 (WAL_JOB_CKPT/job.ckptinfo). set_preason()/do_clr_preason() (job.c) set/clear job->pend_reasons (xce_job_preason_t*: main_reason plus xce_host_reason_t**/xce_res_reason_t** nested arrays), but the WAL_JOB_PREASON fact -- purely hook-driven via persist_job_hook_fun's default: arm (shared with STOP/RESUME/UPDATE, same class as bug-1690/1691's hook-driven precedent, NOT a direct persist_submit_job_*_fact() call site like DISPATCH/START/FINISH) -- only ever carried the shared wal_jobstate_t{jobid, state} payload. Neither apply seam restored pend_reasons after a restart. This was the LAST of the five field-parity gaps from the original xce-audit-invariants pass -- closes that invariant class.
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_SETPREASON/HOOK_DETAIL_CLRPREASON through its generic default: arm (shared with STOP/RESUME/UPDATE), which captures only {jobid, state} from the job_t* -- j->job.pend_reasons, freshly set/cleared by set_preason()/do_clr_preason() 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 pend_reasons, matching the exact same class of gap bug-1678/1687/1689/1690/1691 already fixed for allocs/pidinfo+start_time/exit_info+finish_time/run_usage/ckptinfo. Confirmed via CodeGraph source read that set_preason()/do_clr_preason() are hook-driven (fire xce_run_hooks only, no direct persist_submit_job_*_fact() call site) -- closer to bug-1690/1691's RUSAGE/CKPT precedent than DISPATCH/START/FINISH's direct-call-site pattern -- verified against the real code before implementing. Also confirmed set_preason()/do_clr_preason() never change job.state (only job.pend_reasons), 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/1691's exact same discovery. UNIQUE to this bug (verified, not assumed from precedent): job.pend_reasons has a genuine CLEAR operation (do_clr_preason/HOOK_DETAIL_CLRPREASON) that legitimately resets it to NULL and must survive replay just as durably as the SET side -- bug-1690/1691's own wire convention ('trailing bytes present or absent' == 'field has a new value or is old-format, leave untouched') cannot distinguish that legitimate NULL from a pre-fix payload, so a blind copy of that convention would have silently reintroduced a stale-pend_reasons-after-clear bug on restart.
Fix
FIXED (2026-08-25). controller.capnp ALREADY had a matching struct -- struct JobPendReason $C.mapname("xce_job_preason_t") with mainReason/hostReasons: List(HostBasedReason) $C.maplistcount("nhost_reasons")/resReasons: List(ResourceBasedReason) $C.maplistcount("nres_reasons"), plus sibling HostBasedReason $C.mapname("xce_host_reason_t") and ResourceBasedReason $C.mapname("xce_res_reason_t") -- verified field-for-field against types.h's real struct layouts, and ALREADY wired into the full Job struct's own encode/decode/free (Job.pendreason field, used by the existing checkpoint/snapshot path) -- confirmed via grep that encode_JobPendReason/decode_JobPendReason/free_JobPendReason/free_JobPendReason_ptr/new_JobPendReason/read_JobPendReason/write_JobPendReason all already exist in the generated controller.capnp.c/.h. NO schema change needed (like bug-1678/1687's JobAlloc/JobPIDInfo reuse, unlike bug-1690's WAL_JOB_RUSAGE which needed a brand-new schema struct).
SAFETY CHECK (bug-1690's RUSAGE lesson, explicitly re-verified rather than assumed): read erase_job_preason() (src/lib/preason.c) -- releases every field via FREEUP() (plain free()), never dereference()/RTTI. Read add_host_preason()/add_res_preason() (same file) -- build host/resource reason entries via strdup()/calloc() throughout, never makeString(). Read the generated decode_HostBasedReason/decode_ResourceBasedReason (controller.capnp.c) -- use STRING_DUP (plain strdup) + calloc(), and free_HostBasedReason/free_ResourceBasedReason/free_JobPendReason/free_JobPendReason_ptr all use plain free(). Also confirmed jobj.c's job_init() ALREADY does a plain MOVE_PTR(dst->job.pend_reasons, src->pend_reasons) when constructing a job from a decoded Job (no merge, no makeString) and job_destroy() releases it via erase_job_preason() (FREEUP-only) -- an existing, working precedent for exactly this adopt pattern elsewhere in the codebase. CONCLUSION: unlike WAL_JOB_RUSAGE's host_name (xstring/RTTI-backed, required a merge to avoid corrupting the codebase's own live in-place-mutated structure), job.pend_reasons and its entire nested tree are plain calloc()/strdup()-backed with NO RTTI involvement -- a straightforward free-old-then-adopt-new via the capnp-generated free_JobPendReason_ptr() is safe, matching bug-1678/1687's JobAlloc/JobPIDInfo pattern, NOT bug-1690's merge-required pattern. set_preason() itself corroborates this: it does erase_job_preason()+FREEUP() then a plain MOVE_PTR of the caller's already-built xce_job_preason_t*, never mutating the SAME live structure in place the way update_host_runusage() does for run_usage -- so there is no live-object-reapplied-via-apply-pump hazard analogous to RUSAGE's here either.
persist.c gains capnp_encode_jobpreason()/decode_jobpreason() (mirroring capnp_encode_joballoc()/decode_joballoc() exactly). persist_job_hook_fun() gains a new HOOK_DETAIL_SETPREASON/HOOK_DETAIL_CLRPREASON case (split out of the generic default arm, mirroring bug-1691's HOOK_DETAIL_JOB_CKPT split) that encodes j->job.pend_reasons when non-NULL.
KEY DESIGN DIVERGENCE from bug-1690/1691's wire convention (the CLEAR-semantics gap identified above): the payload appends an explicit PRESENCE BYTE after the wal_jobstate_t header whenever the fact is SETPREASON or CLRPREASON (not just when a value exists to encode) -- 0 = pend_reasons was genuinely cleared (adopt NULL on replay), 1 = trailing capnp JobPendReason bytes follow (adopt the decoded value). A payload of exactly sizeof(wal_jobstate_t) (no presence byte at all -- any entry written before this fix) leaves pend_reasons untouched on both apply seams, preserving full backward compatibility. Both apply seams updated: apply_wal_entry() splits WAL_JOB_PREASON out of its shared state-only group into its own case, decoding the presence byte (and trailing capnp bytes when present=1) via decode_jobpreason() and calling new restore_job_preason_state() (job.c) -- careful to treat a present-but-failed-decode as 'leave untouched', never as an implicit clear. apply_committed_entry() stays in its shared job-fact group but -- mirroring bug-1690/1691's unconditional-adopt design for the identical reason (set_preason()/do_clr_preason() never change job.state, so gating on JOB_TRANSITION_APPLIED would make it dead code) -- decodes/adopts pend_reasons UNCONDITIONALLY, before the transition gate, using the same presence-byte + decode-failure-safe logic.
Regression tests: new src/controller/testsuite/test_wal_job_preason_fact.c (11 tests, mirroring test_wal_job_ckpt_fact.c's 'unconditional decode independent of transition result' structure, extended with PREASON-specific clear-semantics coverage no CKPT/RUSAGE precedent needed): (1) apply_committed_entry sets pend_reasons (BOTH a host-based AND a resource-based reason, exercising both nested list types) when state does NOT change -- most important test; (2) apply_committed_entry sets pend_reasons on a genuine forward transition; (3) apply_committed_entry backward-compat with an old bare payload; (4) apply_committed_entry CLEARS an existing pend_reasons on a presence-byte=0 payload -- the test that would have caught a naive copy of bug-1690/1691's convention; (5) apply_committed_entry leaves pend_reasons untouched when present=1 but zero trailing bytes follow (decode_jobpreason never even called) -- NOTE: an earlier draft of this test tried to exercise 'corrupt capnp bytes' instead and got a genuine RED signal for the wrong reason (Expected 4 Was 0) -- capnp-c's read path does not validate content, so garbage-but-nonempty bytes decode into a zero-valued struct rather than returning NULL (matching decode_joballoc/decode_jobrunusage's identical pre-existing leniency); corrected to the actually-reachable zero-trailing-bytes case; (6-8) the apply_wal_entry mirrors of (1)/(3)/(4); (9) direct-payload restart replay (WAL_JOB_SUBMIT + WAL_JOB_PREASON with host+resource reasons via persist_wal_async + persist_recover()); (10) test_wjp_real_do_set_preason_survives_restart -- drives the REAL, unmodified do_set_preason() (set_preason() itself is file-static to job.c; do_set_preason() is job.c's only exported entrypoint, matching the actual OPCode_setpreason wire path) through a real WAL-write + persist_recover() restart cycle, requiring a minimal zeroed (fd=-1, no iomgr) endpoint_t since do_set_preason() calls xce_reply() before processing -- mirrors test_commit_barrier_reply.c's established fixture pattern, packets drained via endpoint_deq()/free_packet() to avoid a reachable-leak false positive; (11) test_wjp_real_clear_survives_restart -- PREASON-specific, no CKPT/RUSAGE analog: drives real do_set_preason() THEN real do_clr_preason() before restarting, proving the CLEAR itself durably survives replay (not just the earlier SET) -- the audit's exact confirmed correctness requirement unique to this fact type.
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 doc comment, meson.build/controller_test.c wiring), incremental rebuild via the synchronous xce-build.sh script (nix develop ./src --command bash src/tools/xce-build.sh controller asan build-asan --incremental) -- 1178 Tests 9 Failures: the 2 pre-existing bug-1561 test_config_repl.c failures plus exactly 7 of the 11 new tests failing with the exact expected 'must set job.pend_reasons' / 'must reset pend_reasons to NULL' mismatches (tests 1,2,4,6,8,9,10 by suite order); the other 4 new tests (backward-compat x2, the decode-edge-case test, and the clear-survives-restart test) correctly PASS unchanged under the reverted code -- verified this is expected, not a test-quality defect: those 4 scenarios are either genuinely unaffected by the fix (backward-compat) or, for the two restart-replay tests, happen to trivially satisfy their own assertion under BOTH old and new code (old code never encodes pend_reasons at all regardless of set-vs-clear, so a 'must be NULL after clear' check passes vacuously pre-fix too -- acceptable, since the SET-side restart test (10) and the direct-payload test (9) already independently prove the encode/decode mechanism itself, and test (11)'s value is in proving the CLEAR case specifically once mechanism 10 is proven, not in isolating it alone). git stash pop, incremental rebuild -- 1178 Tests 2 Failures (only the pre-existing bug-1561 pair, exact signature match 'Expected 2 Was 3' / 'Expected 0 Was 11954236611186853435'), all 11 new tests pass, zero new failures, zero ASAN heap-corruption findings (grepped the full test log for AddressSanitizer/heap-buffer-overflow/invalid-free/double-free/use-after-free/SEGV/Aborted -- none found). Verified no new compiler warnings in persist.c/job.c/the new test file (isolated rebuild + warning grep; the handful of pre-existing format-truncation/comment warnings in persist.c are all at unrelated line numbers, predating this change).
This was the LAST of the five field-parity gaps identified by the original xce-audit-invariants pass (bug-1678/1687/1689/1690/1691/1692) -- that invariant class (APPLY_CLEAN's entity-count+cursor-position check not proving per-field completeness, persistence.md) is now closed for job.allocs/pidinfo+start_time/exit_info+finish_time/run_usage/ckptinfo/pend_reasons.
Verification
Not recorded (migrated from buglog.json).
References
- Related: bug-1678, bug-1687, bug-1689, bug-1690, bug-1691, bug-1561