fix(controller): WAL_JOB_START loses pidinfo/start_time on restart replay (bug-1687)

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
This commit is contained in:
Rongsong Shen 2026-08-25 09:23:00 +00:00
parent bc3f76b42e
commit 955efecd19
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
9 changed files with 992 additions and 7 deletions

View file

@ -475,6 +475,22 @@ static void cbrp_teardown(void)
* range in job.c's process-lifetime in-flight registry. Clear it here
* so it never outlives this fixture. */
inflight_submits_reset_for_test();
/* bug-1687: persist_stop() must run BEFORE commit_engine_fini() --
* exactly the ordering bug-1452 already fixed in test_persist_truncate.c
* (that fix's own note flagged this file as a same-pattern, not-yet-
* observed-to-crash follow-up). commit_engine_fini() tears down
* g_hce.lock/g_hce.cond and, as its final step, nulls g_hce.p while the
* fsync_worker background thread (started by persist_start(), stopped
* only by persist_stop()'s pthread_join) can still be mid-flight inside
* its unlocked p->on_durable_advance(p->durable_cb_ctx) callback
* (persist.c's fsync_worker) -- a genuine TOCTOU data race with no
* synchronization between the two threads. Calling persist_stop() here,
* first, guarantees the flusher thread cannot be in-flight at all by the
* time commit_engine_fini() runs. The later persist_stop() call below
* becomes a safe, idempotent no-op on this second invocation (mirrors
* bug-1452's fix exactly -- fsync_thread_running is already 0 and wal_fd
* is already -1, so both of persist_stop()'s internal branches skip). */
persist_stop(&g_hc.persist);
commit_engine_fini(&g_hce);
persist_stop(&g_hc.persist);
persist_fini(&g_hc.persist);
@ -2410,7 +2426,14 @@ static int cbrp_count_jobstate_entries_ex(persist_t *p, uint32_t wal_type,
(int)pread(fd, &hdr, sizeof(hdr), (off_t)off),
"WAL segment scan: short header read");
if (hdr.type == wal_type
&& hdr.payload_len == sizeof(wal_jobstate_t)) {
&& hdr.payload_len >= sizeof(wal_jobstate_t)) {
/* bug-1687: >= not == -- WAL_JOB_START (like bug-1678's
* WAL_JOB_DISPATCH before it) can now carry trailing bytes
* (start_time + Cap'n Proto JobPIDInfo) beyond the fixed
* wal_jobstate_t header. jobid/state always live at the same
* fixed leading offset regardless of trailing bytes, so reading
* just sizeof(wal_jobstate_t) here remains correct for any
* payload_len >= that size. */
wal_jobstate_t js;
TEST_ASSERT_EQUAL_INT_MESSAGE(