Found via the mandatory live HA chaotic soak for this session's controller
audit campaign -- unrelated to any of the 9 audit fixes (bug-1684/1687/
1689/1690/1691/1692/1693/1694), all in ha.c/persist.c/job.c/cluster.c/
sched.c/worker.c; this is in myapp.c, untouched by any of them.
myapp_run()'s bootstrap "is a leader already known" check had a fallback
`c.ha.raft.term > 0` meant to detect genuine new raft activity processed
during the startup-stagger nap. But ha_restore_term() loads ha->term from
disk BEFORE this check runs, on every boot -- and Raft terms only ever
increase, never reset. So `raft.term > 0` is trivially true on every
restart for the rest of the cluster's on-disk lifetime, once it has ever
held one election, regardless of whether any leader is currently
reachable. Live-reproduced: every node, on every restart after the
cluster's first election, unconditionally skipped ha_start_election_async
forever -- no node could ever re-assert leadership through a real
election again. bug-1477's existing cluster_init_phase2 retry loop kept
firing correctly (confirmed in the reproduction logs) but could never
succeed, since the deeper cause -- no election ever runs to produce a
live primary to push config from -- was never addressed by that fix.
Fixed by snapshotting `term_at_boot` right after ha_restore_term()/
ha_init_raft() seed the term, strictly before the stagger nap, and
comparing `raft.term > term_at_boot` instead of `raft.term > 0` -- only a
term that has genuinely ADVANCED during this boot cycle (real vote/
heartbeat traffic processed) counts as evidence a leader is known. The
decision was extracted into a new, unit-testable myapp_leader_already_known()
(startup_validation.c, since controller_unit_test deliberately excludes
myapp.c/main.c). voted_for_name/vft==cur and primary_ctrl_fd>=0 checks
are unchanged.
7 new tests in test_ha_startup.c; RED/GREEN independently verified.
Rigorous A/B live-soak proof (identical 6-round --chaotic run, fixed vs
git-stashed-back-to-original): baseline showed term frozen for the whole
run and zero election activity on any non-primary node; fixed run showed
term genuinely progressing and ctrl-2 achieving a real, log-verified
CTRL_PRIMARY transition via a won PreVote round -- proof a node other
than the original primary can now take over, which was structurally
impossible before. Full controller suite: 1191 tests, 2 failures -- both
the pre-existing, unrelated bug-1561 overlayfs sandbox flake.
Residual, NOT independently verified yet (flagged honestly, not fixed
here): the fixed run's chaotic verdict showed committed_job_loss=6
(baseline 0) -- traced via waldump to one jobid missing from every node's
WAL, at the cold-boot window before round 1. Resembles the already-fixed
bug-1487 false-positive class (apply-pump-lag racing verify_classify's
expect-job check) but the reported mechanism doesn't perfectly match
(bug-1487's job was present-in-WAL/not-yet-visible; this one is described
as absent from every WAL entirely) -- needs independent re-verification
before being dismissed, tracked as a follow-up.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AgYRU3zDTqNGV5Jk8aiKLg
Last of the 9 confirmed findings from the /xce-audit-invariants controller
pass. checkpoint_task_fn was registered directly as a delaymgr entry and
ran its full blocking body inline: checkpoint marker pwrite+fdatasync,
write_snapshot()'s genuinely unbounded per-entity walk (scales with
cluster state size) held under apply_pump_lock, WAL trim, and snapshot
rotation/unlink -- all on the same thread that drives heartbeats/election
timeouts. Unlike every other delaymgr operation (snapshot send,
config-push, reconnect, catch-up), checkpoint had no offload mechanism
at all -- the one gap left from the raft-drive-async hardening pass.
Design: full offload via the SlowLane (slowlane_submit), default ON --
NOT the vtask pool. persist_apply_pump_run() already holds apply_pump_lock
across its own per-entry walk while running ON the vtask pool
(VTASK_LIVE_BLOCK); putting checkpoint's unbounded, lock-held
write_snapshot() on that same pool risked starving it, exactly what
ha-async-io.md's own thread-role table warns against. The SlowLane is
the actual established pattern for this I/O class (push_config_transport_only,
the snapshot-send path, catchup_lane_fn, demotion_lane_fn all dispatch
there) -- and persist_checkpoint() already runs on the slow lane in
production TODAY via ha_snapshot_do_send() -> snapshot_lane_fn, so
apply_pump_lock's acquisition from a slow-lane thread is proven-safe,
exercised daily, not new surface. Unlike catchup/demotion (which stay
flag-gated OFF for an unrelated, still-not-root-caused hang, bug-1429),
checkpoint's new flag defaults ON given this reuses an already-proven
code path. Inline-fallback + WARN only on a genuine slowlane_submit
failure (not -EAGAIN/-EEXIST, both non-failures) -- never silently drops
a scheduled checkpoint.
New test_persist_checkpoint_offload.c (3 tests). RED-phase forced the
pre-fix always-inline path: the suite never printed a single result and
had to be killed, proving the test genuinely detects the violation, not
a tautology. GREEN: all 3 pass. Full suite: 1184 tests, 2 failures --
both the pre-existing, unrelated bug-1561 overlayfs sandbox flake
(exact signature match). No live HA soak was run for this specific fix
in isolation (explicitly scoped out, not hidden) -- the mandatory
live-soak proof for this whole audit campaign covers it next.
This closes the last of the 9 confirmed findings from the controller
audit (bug-1684, bug-1687/1689/1690/1691/1692, bug-1693, bug-1694).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AgYRU3zDTqNGV5Jk8aiKLg
Violates this project's own recorded Decision Log entry: "No client
reply-wait timeout as a fix for a hung request -- fail loud at the real
transport/source instead of masking with a deadline backstop."
OPCode_schedAdmin (cluster.c) and on_fetchfile (job.c, the worker proxy
-- cluster.c's own comment says schedAdmin "mirrors on_fetchfile's proxy
pattern", confirming one shared design mistake, not two independent
bugs) both blocked on a multi-second pthread_cond_timedwait, replying
"timeout" on expiry. handle_scheduler_leave()/handle_worker_leave() (the
actual disconnect handlers) only nulled the endpoint pointer -- neither
ever signaled the pending waiter, so a mid-wait disconnect was invisible
until the deadline lapsed. The timeout was the ONLY failure-detection
mechanism, exactly what the decision forbids.
Both sites already had a working signal function on their success path
(schedadmin_signal(), fetchfile_signal()) -- just never wired to the
disconnect handlers. Fixed by calling them from
handle_scheduler_leave()/handle_worker_leave() with a distinguishable
-ECONNRESET outcome, so the waiter wakes immediately on disconnect
instead of only on the deadline. Timeouts kept as defensive backstops
for a genuinely wedged peer that never disconnects cleanly either --
this fix is about not relying SOLELY on the timeout, not removing it.
schedadmin_signal_disconnect() must be called BEFORE handle_scheduler_leave()
acquires c->sched.lock -- the dispatch wait holds that same lock for its
entire duration, so signaling after acquiring it would deadlock.
Found by an /xce-audit-invariants controller pass (Decision Log fidelity
dimension). New test_fetchfile_disconnect.c plus extensions to
test_sched_admin_dispatch.c/test_sched_worker_change.c; RED/GREEN
independently verified against real elapsed-time assertions (pre-fix:
5001ms full-timeout wait; post-fix: <1s immediate wake). Full controller
suite: 1181 tests, 2 failures -- both the pre-existing, unrelated
bug-1561 overlayfs sandbox flake, confirmed via a clean meson-driven run.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AgYRU3zDTqNGV5Jk8aiKLg
Last of the five field-parity gaps from the original audit (bug-1678,
bug-1687, bug-1689, bug-1690, bug-1691). set_preason()/clear_preason()
set job.pend_reasons (host/resource-based pending-job explanations), but
persist_job_hook_fun's default: arm only ever captured {jobid, state}.
Neither apply seam restored pend_reasons after a restart.
Reuses the ALREADY-EXISTING JobPendReason/HostBasedReason/ResourceBasedReason
capnp structs -- no schema change needed, unlike bug-1690's WAL_JOB_RUSAGE.
Safety-checked against bug-1690's lesson (RTTI/makeString()-backed fields
need merge-not-replace, not free-and-replace): pend_reasons is plain
malloc/strdup-backed throughout (erase_job_preason is FREEUP()-only,
add_host_preason/add_res_preason use strdup()/calloc()), so a
straightforward free-and-replace is safe here, matching bug-1678/1687's
pattern instead.
New wrinkle beyond the four prior fixes: pend_reasons has a genuine CLEAR
operation, and "trailing bytes present or absent" can't distinguish a
legitimate clear from an old-format payload -- added an explicit presence
byte (0=cleared/adopt NULL, 1=capnp bytes follow), verified by two
dedicated "clear survives restart" tests.
11 new tests in test_wal_job_preason_fact.c; RED/GREEN independently
verified (7 genuine pre-fix failures, one test's own wrong premise about
capnp's garbage-byte behavior caught and corrected during RED/GREEN
iteration). Full controller suite: 1178 tests, 2 failures -- both the
pre-existing, unrelated bug-1561 overlayfs sandbox flake. No ASAN findings.
This closes the field-complete apply parity invariant class the
xce-audit-invariants controller dimension 7 was added for.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AgYRU3zDTqNGV5Jk8aiKLg
Same class as bug-1678/1687/1689/1690: on_job_ckpt() sets job.ckptinfo
(a plain char*, xstring-backed via makeString()) but is purely hook-driven
-- persist_job_hook_fun's default: arm only ever captured {jobid, state}
for WAL_JOB_CKPT. Neither apply seam restored ckptinfo after a restart.
Simplest of the five field-parity fixes: ckptinfo is a bare string, so no
capnp struct is needed -- mirrors WAL_QUEUE_CTRL's existing NUL-terminated-
string-after-header wire shape instead. Adoption is unconditional (not
gated on a state transition), matching bug-1690's precedent: a checkpoint
report never changes job.state. restore_job_ckpt_state() releases the
prior value via dereference() (the correct RTTI verb for an xstring), not
free().
Found by an /xce-audit-invariants controller pass (field-complete apply
parity dimension) -- the last of five sibling gaps found together
(bug-1678, bug-1687, bug-1689, bug-1690, this one). 7 new tests in
test_wal_job_ckpt_fact.c; RED/GREEN independently verified (5 genuine
pre-fix failures, 2 backward-compat tests correctly passed unchanged).
Full controller suite: 1167 tests, 2 failures -- both the pre-existing,
unrelated bug-1561 overlayfs sandbox flake. No ASAN findings.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AgYRU3zDTqNGV5Jk8aiKLg
Same class as bug-1678/1687/1689: the WAL fact for WAL_JOB_RUSAGE only
carried {jobid, state}, so job.run_usage (client-reported per-host
CPU/mem usage) was never restored on either apply seam after a restart.
Unlike the prior three, xce_job_runusage_t/xce_host_runusage_t had no
existing Cap'n Proto mapping -- a genuine new schema addition
(HostRunusage/JobRunusage in controller.capnp, mirroring JobPendReason's
List(NestedStruct)+$C.maplistcount style), verified against the real
struct definitions in types.h, codegen confirmed via nm on the built
binary rather than assumed.
Two deliberate deviations from the established precedent, found by
reading the real code rather than copying the pattern blindly:
1. Adoption is unconditional, not gated on a state transition -- a
rusage report almost never changes job.state (repeat reports keep
state at RUN), so gating on JOB_TRANSITION_APPLIED like START/FINISH/
DISPATCH do would make the fix dead code for the realistic case.
2. Merge, not free-and-replace. update_host_runusage() builds run_usage
with host_name sourced from makeString() -- an RTTI-managed pointer,
not malloc() -- and the primary's own live apply pump re-applies every
fact it just wrote. A naive free_JobRunusage_ptr()-then-adopt (matching
the prior three fixes exactly) would free() a non-malloc'd pointer on
the very first live rusage report for a new host -- guaranteed heap
corruption. Fixed with merge_job_runusage(): existing hosts get
scalar-only in-place updates (host_name pointer never touched), only
genuinely new hosts adopt their freshly-decoded (safe, malloc'd)
host_name. Proved via a real makeString()-backed test asserting
pointer identity survives, not just value equality. job_destroy()
(src/lib/jobj.c) was found to have this identical latent mismatch,
pre-existing and independent of this fix -- flagged, not fixed here.
Found by an /xce-audit-invariants controller pass (field-complete apply
parity dimension). 8 new tests in test_wal_job_rusage_fact.c; RED/GREEN
independently verified. Full controller suite: 1160 tests, 2 failures --
both the pre-existing, unrelated bug-1561 overlayfs sandbox flake. No
ASAN/heap-corruption findings. Backward compatibility confirmed for
old-format bare-payload entries.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AgYRU3zDTqNGV5Jk8aiKLg
Same class as bug-1678/bug-1687: record_job_finish_fact() wrote only
{jobid, state} for WAL_JOB_FINISH. Both apply seams either fabricated
finish_time via time(NULL) or touched nothing beyond state.
Worse than "lost on restart": apply_job_finish_transition() -- the only
function that ever set job.exit_info -- is dead code on the real
production apply path. The raft-apply-decoupling redesign routes actual
mutation through the leaner apply_job_update_transition() instead, so
exit_info was never populated at all, restart or not, before this fix.
job_exitinfo_t ({int exitcode;}) has no existing capnp mapping, so this
uses a small native (non-capnp) trailing payload -- wal_jobstate_t header
+ uint64_t finish_time + int32_t exit_code -- mirroring wal_jobstate_t's
own native-struct style rather than inventing a capnp wrapper for one
field. run_time is never carried on the wire, always re-derived as
finish_time - start_time at apply time. Both apply seams updated;
old-format payloads (exactly sizeof(wal_jobstate_t)) decode unchanged
(fabricated finish_time, NULL exit_info) -- full backward compatibility.
Found by an /xce-audit-invariants controller pass (field-complete apply
parity dimension). 6 new tests in test_wal_job_finish_fact.c; RED/GREEN
independently verified (4 genuine pre-fix failures with real
expected-vs-fabricated mismatches, 2 backward-compat tests correctly
passed both before and after). Full controller suite: 1152 tests, 2
failures -- both the pre-existing, unrelated bug-1561 overlayfs sandbox
flake (confirmed flaky across repeated re-runs of the same binary).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AgYRU3zDTqNGV5Jk8aiKLg
Same class as bug-1678: apply_job_start_transition() sets the real
job->job.pidinfo (worker-reported PIDs/PGIDs/exdata) and start_time, but
the WAL fact for WAL_JOB_START only carried the shared wal_jobstate_t
{jobid, state} header -- both apply seams (apply_committed_entry() for
primary boot replay, apply_wal_entry() for standby catch-up) restored
state only. pidinfo stayed permanently NULL after a restart; start_time
was fabricated with time(NULL) at replay instead of being carried,
drifting from the true value.
Mirrors bug-1678's exact fix shape: capnp_encode_pidinfo()/decode_pidinfo()
(persist.c, using the pre-existing JobPIDInfo capnp struct -- no schema
change needed) serialize job_pidinfo_t into the WAL payload, trailing a
new uint64_t start_time field appended after wal_jobstate_t.
persist_submit_job_start_fact() replaces the old state-only
persist_submit_job_fact() call for this type. Both apply seams updated;
a payload of exactly sizeof(wal_jobstate_t) (any entry written before
this fix) decodes with pidinfo left NULL and start_time fabricated via
time(NULL), exactly matching pre-fix behavior -- full backward
compatibility, verified by every pre-existing WAL_JOB_START test using
the old bare-struct shape.
Also fixed a genuine, unrelated deadlock this change's larger payload
exposed in test_commit_barrier_reply.c: cbrp_count_jobstate_entries_ex()
matched WAL entries by exact payload size (==), so once WAL_JOB_START
legitimately grew it silently undercounted to 0 and a downstream test
spun forever polling an uncounted target. Fixed the match to >= (jobid/
state always live at a fixed leading offset) -- bisected via /proc/*/wchan
futex inspection (no ptrace in this sandbox), confirmed as the actual
cause, not the teardown-ordering red herring first suspected.
Found by an /xce-audit-invariants controller pass (field-complete apply
parity dimension, added after bug-1678). 6 new tests in
test_wal_job_start_fact.c; RED/GREEN independently verified (4 tests
genuinely failed pre-fix with exact expected-vs-fabricated value
mismatches, 2 backward-compat tests correctly passed both before and
after). Full controller suite: 1146 tests, 2 failures -- both the
pre-existing, unrelated bug-1561 overlayfs sandbox flake.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AgYRU3zDTqNGV5Jk8aiKLg
User-reported live crash: xce_scheduler SIGSEGV in fcfs_cursor_enter_queue
(builtin_fcfs.c), called from builtin_fcfs_next_job <- sched_job_picker
(picker thread).
priv->cursor/priv->job_cursor (fcfs_plugin_data_t) are raw ordq_item_t*
pointers that persist across SEPARATE next_job() calls, each with its own
independent priv->lock acquisition/release. The existing mutual-exclusion
fix (task 2.4) correctly protects the cursor walk from corruption DURING
one locked call, but does nothing to stop a LATER call from dereferencing
a node a properly-locked, INTERVENING call already freed.
on_queue_update()'s re-sort (remove old ordq_item_t node, insert a fresh
one via ordq_put) is exactly such a free -- with only one queue in the
deployment, the picker's cursor is essentially always parked on it
between calls (sched_queue_is_ready/sched_job_is_ready are unconditional
true stubs today, so the cursor never advances away while jobs remain),
making a queue config update land in that window trivially reachable.
Fixed by invalidating (NULLing) whichever cursor points at a node right
before that node is freed, at all three sites that can free a node the
cursors might reference: on_queue_update's reuse-and-resort path
(priv->cursor + priv->job_cursor in lockstep), builtin_fcfs_on_job_state_change's
!JOB_IS_PEND removal, and fcfs_remove_job_everywhere's cross-queue removal
loop (both job_cursor only, via an ordq_peek-then-invalidate-then-delete
pattern). Always safe: next_job() treats a NULL cursor as "no more
candidates this round", and begin_session() resets both cursors fresh
every round regardless -- worst case is a bounded one-round delay, never
a crash or a permanently lost job.
New test_builtin_fcfs_cursor_uaf.c reproduces the bug fully single-threaded
(next_job() releases the lock between calls, so no real concurrency is
needed to hit the window). RED-phase independently confirmed a real ASAN
heap-use-after-free with the exact same call chain as the live crash
(freed by on_queue_update, read by next_job()); GREEN confirms 0 failures
and no job permanently lost across the invalidation.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AgYRU3zDTqNGV5Jk8aiKLg
User-reported live crash: xce_scheduler SIGABRT via glibc malloc_printerr
(heap corruption) inside kill_limit(), called from limit_update_config()
during a live config reload.
limit_update_config() called kill_limit(limit) in its error path after a
failed key_add(privdata->limits, ...). But privdata->limits is a kvmgr
created with .datakiller = kill_limit, so on a kvtab-level failure (e.g.
-EEXIST for a duplicate limit name -- exactly the rc=-17 captured in the
crash's own backtrace), xkvmgr_add() already rolls back via vclear() ->
xzarray_clear(), which fires kill_limit(limit) once automatically. The
caller's explicit second call was a double free.
Removed the redundant call; the kvmgr's own configured datakiller already
owns cleanup on this failure path (mirrors the ownership asymmetry
job.c's restore_job_array() already documents for the opposite case,
where its kvmgr has no datakiller configured and the caller must free
failed items itself). One accepted, documented residual gap: a vstore()-
level OOM failure (never reaching the kvtab step) would leak `limit`
instead -- vanishingly rare, and a leak is strictly better than the
guaranteed crash on the common duplicate-name case; xkvmgr_add's return
code doesn't distinguish which sub-case occurred, so there's no clean way
to free conditionally without a kvmgr API change (out of scope).
New test_limit_plugin.c (this plugin had zero test coverage before) --
RED-phase independently reproduced a genuine ASAN heap-use-after-free
matching the live crash's exact call chain; GREEN confirms 0 failures.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AgYRU3zDTqNGV5Jk8aiKLg
ha_persist_term() had a void return -- every open/write/fsync/rename
failure was silently swallowed ("best-effort; caller must not rely on
this for correctness"). Its sole vote-granting caller, raft_cb_persist(),
always returned 0 regardless, so the raft library always granted the
vote even when the term/voted_for record never reached disk -- a real
split-vote risk if the node then restarted and forgot it voted
(Election Safety requires durably-persisted votedFor).
ha_persist_term() now returns 0 on success (including the deliberate
NULL-workdir test-mode skip) or -errno on a genuine I/O failure
(fsync's previously-unchecked return is now checked too). raft_cb_persist()
propagates that failure to the raft library (denying the vote per its
own documented contract) and rolls back ha->voted_for_term/
ha->voted_for_name to their pre-call snapshot, so the adapter's
in-memory state stays consistent with the library's own "deny without
updating voted_for" behavior. ha->term is deliberately NOT rolled back:
the raft library adopts a higher observed term unconditionally at Step 2
of raft_on_vote_request, before and independent of the vote-grant/persist
outcome, so the adapter correctly mirrors that same semantics.
Also corrects a stale claim in controller/CLAUDE.md's Protocol
Verification Checklist (found by a sibling audit dimension the same
session): term+voted_for were never written "atomically under ha->lock"
-- term uses its own atomic store outside the lock.
Found by an /xce-audit-invariants controller pass (error-handling
dimension). 3 new tests in test_ha_durability.c drive the real
production path (ha_init_raft + raft_on_vote_request, the exact
function cluster.c's dispatch calls) against a forced ENOENT write
failure; RED/GREEN independently verified.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AgYRU3zDTqNGV5Jk8aiKLg
Open-ended dimension that walks .wolf/cerebrum.md's Decision Log section
fresh each audit (not a fixed list, since the log grows over sessions) --
seeded with two candidates: no-timeout-as-hang-fix, and commit-first-
then-reply ordering distinct from dimension 4's materialization-timing
check.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AgYRU3zDTqNGV5Jk8aiKLg
persistence.md's APPLY_CLEAN verdict only checks entity count and apply-
cursor position, not per-field completeness -- bug-1678 (job allocation
silently dropped on restart replay despite APPLY_CLEAN reporting healthy)
fell through exactly this gap. Documents the sharper invariant online-
apply and replay-apply must produce field-complete equivalent state, not
just matching counts/cursor, and adds it as a standing audit dimension
(#7) to xce-audit-invariants' controller dispatch list.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AgYRU3zDTqNGV5Jk8aiKLg
sim_loop.c never called plugin_begin_session()/plugin_end_session() at
its round boundary, unlike driver.c's normal-mode round -- a driver-
equivalence gap found by an /xce-audit-invariants scheduler pass.
Currently inert (sim mode's only reachable picker, builtin_fcfs, leaves
both hooks NULL) but structurally identical to bug-1613's class of bug.
Relocated plugin_begin_session()/plugin_end_session() from an implicit
xce_scheduler_t* to xce_sched_core_t* directly (mirroring
sched_account_jobs()'s prior relocation) -- their bodies never touched
anything beyond core->plugins, so driver.c's comment claiming they
"needed the full xce_scheduler_t*" was itself stale. sim_loop.c now
calls both, same relative position as driver.c. A full-tree grep (done
after the first build attempt failed on an untouched file) turned up a
third live call site in scheduler.c's legacy sched_begin_session()/
sched_end_session(), plus four testsuite files with their own stub
definitions of the same functions -- all updated to match the new
signature.
Extended test_sim_session_ordering.c's existing bug-1613 regression
tests with new plugin_begin_session()/plugin_end_session() spies
(call-count + session_active-window proof, same technique as the
existing sched_account_jobs spy). RED/GREEN independently verified:
temporarily disabled the two new call sites, confirmed both new
assertions fail (Expected 2 Was 0 / Expected 1 Was 0), restored,
reconfirmed green. Full scheduler suite: 51/52 green, the one failure
is the pre-existing unrelated bug-1560 sandbox flake.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AgYRU3zDTqNGV5Jk8aiKLg
FilterRows routed ANY condition on job_status/host_status through
MatchesBitmaskCondition based purely on the field name, regardless of
the condition's actual operator. MatchesBitmaskCondition only
implements OpEQ/OpNE (the only two shapes buildIntCondition's in/notin
bitmask branch ever produces) and silently returns false for anything
else -- so xcecli's `-a` flag, which builds `{job_status:notnull:}`
(Operator: OpNOTNULL), matched zero rows unconditionally. `xcecli jobs
list -a` showed "no job found" even when jobs genuinely existed and
were correctly queryable through every other view.
FilterRows now requires the operator to actually be OpEQ/OpNE before
taking the bitmask path; any other operator on these fields correctly
falls through to the generic MatchesCondition evaluator, which already
implements OpNOTNULL/OpISNULL correctly -- no changes needed to either
evaluator itself, only the dispatch condition.
New query_test.go (no test file existed for this package before):
TestFilterRows_JobStatusNotNull_MatchesRealRows reproduces the bug
(independently confirmed RED: 0 of 3 rows matched) and
TestFilterRows_JobStatusBitmaskEQ_StillWorks locks in that the actual
bitmask shape used by every other status flag is unaffected. Live-
verified: `xcecli jobs list -a` now correctly shows jobs across every
status, immediately, no restart needed.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AgYRU3zDTqNGV5Jk8aiKLg
New test_dc_success_notifies_job_update_hook_exactly_once
(test_dispatch_commit.c): a spy hook counting HOOK_AFTER_EVENT/
HOOK_DETAIL_JOB_UPDATE fires specifically -- the exact hook
apiserver_job_update_fun subscribes to in production. Asserts exactly
one fire per successful live dispatch.
Independently reproduced RED->GREEN: disabled the new notify call
with the test kept, rebuilt, confirmed FAIL (Expected 1 Was 0);
restored, rebuilt, confirmed PASS.
Also independently confirmed no double-WAL-append regression via two
existing tests whose fixtures genuinely wire persist.cluster with
persist_job_hook_fun registered: test_jobdispatch_receipt_ack_no_wal_append
(asserts exactly one WAL entry for a successful dispatch) and
test_handle_jobdispatch_matches_dispatch_a_job_notify_count -- both
pass unchanged. Full controller_unit_test: 1137/1137 (2 pre-existing
test_config_repl.c failures, confirmed identical on stock code via
git-stash -- the known overlayfs fdatasync errno=5 flake, bug-1561).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AgYRU3zDTqNGV5Jk8aiKLg
dispatch_a_job_() mutates job.state/job.allocs directly then persists,
firing no hooks at all. apply_committed_entry()'s later apply of that
same WAL_JOB_DISPATCH entry sees state already matches the value it
just set moments earlier (under the same lock) and reports
JOB_TRANSITION_NOOP_DUPLICATE, silently skipping its own would-be
notify. Result: the apiserver is never told a job reached RUN -- xcecli
jobs list shows it stuck at PEND forever, no restart involved.
New persist_notify_job_updated_already_persisted(cluster_t*, job_t*)
(persist.c/persist.h), called from dispatch_a_job_()'s success branch
right after unlock(j). Mirrors apply_committed_entry()'s own identical
Hazard-1-guarded hook fire exactly -- same t_apply_seam_notifying
guard, same hook (HOOK_DETAIL_JOB_UPDATE's AFTER_EVENT), same detail --
without which persist_job_hook_fun (also subscribed to this hook)
would redundantly re-append the transition as a second, duplicate
WAL_JOB_UPDATE entry. The flag stays file-private to persist.c; this
is job.c's only sanctioned way to reach it.
record_job_start_fact()/record_job_finish_fact() needed no change --
their durable-append-only pattern (defer mutation+notify to
apply_committed_entry entirely) is already correct; only
dispatch_a_job_()'s broken hybrid (direct mutation, no notify at all)
was ever the problem.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AgYRU3zDTqNGV5Jk8aiKLg
test_dispatch_commit.c's test_dc_success_appends_wal_job_dispatch_durably
(already drove the real dispatch_a_job_() production path through a
real WAL-write + persist_recover() restart cycle, previously only
checking recovered state) now also asserts the recovered job.allocs
matches what was dispatched -- host, resource name, value. This is
the primary regression test for the user's exact acceptance
criterion: "after recover the job should get the same as it's state
before shutdown."
Independently reproduced RED->GREEN (not just narrated): reverted the
production fix with the test extension kept, rebuilt, confirmed FAIL
at exactly this assertion; restored, rebuilt, confirmed PASS.
test_wal_job_dispatch_fact.c: added 3 new tests exercising the richer
payload directly on both apply_committed_entry and apply_wal_entry,
plus a full restart-replay variant. All 3 pre-existing tests (bare
wal_jobstate_t payloads, no trailing bytes) pass completely unchanged,
proving backward compatibility with any WAL_JOB_DISPATCH entry written
before this fix.
Full controller_unit_test suite independently re-run: 1136/1136 pass,
zero regressions.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AgYRU3zDTqNGV5Jk8aiKLg
dispatch_a_job_() set j->job.allocs in memory at dispatch-commit time
but only ever persisted {jobid, state} via persist_submit_job_fact() --
the allocation itself was never written to the WAL. On a full restart,
job.state correctly recovered to RUN (both WAL_JOB_DISPATCH and the
later WAL_JOB_START fact carry state=RUN) but job.allocs stayed
permanently NULL, since apply_job_update_transition() only ever
touches state. EXEC_HOST displayed blank forever for a job that was
genuinely running before shutdown.
No capnp schema change needed -- JobAlloc (xce_job_alloc_t) was
already a full top-level struct (the existing Job.allocs field) with
generated encode/decode functions already in the build. New:
capnp_encode_joballoc()/decode_joballoc() (persist.c, mirroring the
existing capnp_encode_worker()/restore_job() patterns) and
persist_submit_job_dispatch_fact() -- WAL_JOB_DISPATCH's payload is
now a wal_jobstate_t header plus trailing capnp-encoded JobAlloc
bytes. dispatch_a_job_() now persists j->job.allocs (the canonical,
freshly-constructed struct, read under the same write lock) instead
of just the state.
Both apply seams updated: apply_committed_entry() (primary's own boot
replay) gets one added branch in its existing WAL_JOB_START/FINISH
stamp-chain; apply_wal_entry() (standby catch-up / replicated-apply --
also required, a promoted standby would hit the identical bug) has
WAL_JOB_DISPATCH split into its own case, calling new
restore_job_dispatch_state(). A payload of exactly sizeof(wal_jobstate_t)
(any entry written before this fix) decodes no allocation and behaves
exactly as before -- full backward compatibility.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AgYRU3zDTqNGV5Jk8aiKLg
