lei.chen/myxce
1
0
Fork 1
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(controller): WAL_JOB_FINISH loses exit_info/finish_time on replay -- and exit_info was never set at all in production (bug-1689)

Browse source 
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
This commit is contained in:
Rongsong Shen 2026-08-25 09:58:55 +00:00
parent 44bc50202a
commit c2690cbf95
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
8 changed files with 1381 additions and 425 deletions
Download patch file Download diff file Expand all files Collapse all files

145
src/controller/persist.c
View file

@ -52,6 +52,17 @@ extern int restore_job_dispatch_state(cluster_t* c, uint64_t jobid,
extern int restore_job_start_state(cluster_t* c, uint64_t jobid,
uint32_t state, uint64_t start_time,
job_pidinfo_t* pidinfo);
/* restore_job_finish_state defined in job.c (bug-1689): WAL_JOB_FINISH's own
* sibling of restore_job_state() above, additionally taking ownership of an
* optional decoded job_exitinfo_t and a wire-carried finish_time (0 means
* "not carried -- leave job.finish_time/run_time untouched", the pre-fix
* payload's behavior) so the worker-confirmed finish survives standby
* catch-up / replicated-apply the same way apply_committed_entry's
* WAL_JOB_FINISH case already restores it on the primary's own boot
* replay. */
extern int restore_job_finish_state(cluster_t* c, uint64_t jobid,
uint32_t state, uint64_t finish_time,
job_exitinfo_t* exit_info);
extern int worker_from_capnp(cluster_t* c,
const void* capnp_payload, size_t len);
extern int restore_queue_state(cluster_t* c,
@ -264,6 +275,31 @@ static job_pidinfo_t* decode_pidinfo(const uint8_t* payload, size_t len)
return info;
}
/* Decode a native (non-capnp) WAL_JOB_FINISH exit_code trailer (bug-1689)
* into a freshly heap-allocated job_exitinfo_t*. job_exitinfo_t (xce/types.h)
* is a single int with NO existing Cap'n Proto mapping (confirmed -- no
* JobExitInfo struct in controller.capnp), so this mirrors wal_jobstate_t's
* own native-struct style rather than inventing a capnp wrapper for one int
* -- unlike decode_joballoc()/decode_pidinfo() above, which decode genuine
* capnp payloads. Returns NULL on a short/absent payload; caller owns the
* result (release via free()/FREEUP -- no nested pointers). */
static job_exitinfo_t* decode_jobexitinfo(const uint8_t* payload, size_t len)
{
job_exitinfo_t* info;
int32_t exitcode;
if (!payload || len < sizeof(exitcode))
return NULL;
memcpy(&exitcode, payload, sizeof(exitcode));
info = (job_exitinfo_t*)calloc(1, sizeof(*info));
if (!info)
return NULL;
info->exitcode = (int)exitcode;
return info;
}
/* Serialize a host admin-state frame (config-upgrade-snapshot task 2.1, Req
* 1.6/1.7/1.9) to a malloc'd Cap'n Proto buffer. Mirrors capnp_encode_job
* above exactly -- capnp_host_admin_state_t (typemap.h) is a frame-only
@ -3579,7 +3615,6 @@ int apply_wal_entry(persist_t* p, uint32_t type, uint64_t lsn,
restore_job_array(p->cluster, payload, payload_len, lsn, NULL);
break;
case WAL_JOB_FINISH:
case WAL_JOB_FAIL:
case WAL_JOB_STOP:
case WAL_JOB_RESUME:
@ -3631,6 +3666,44 @@ int apply_wal_entry(persist_t* p, uint32_t type, uint64_t lsn,
break;
}
case WAL_JOB_FINISH: {
/* bug-1689: split out of the shared group above -- mirrors
* WAL_JOB_START's split just above (bug-1687) / WAL_JOB_DISPATCH's
* split just below (bug-1678). This is the one fact type in that
* group whose payload can carry a fixed finish_time PLUS a fixed
* exit_code (persist_submit_job_finish_fact's shape) alongside the
* {jobid, state} header, so a promoted standby's catch-up replay
* recovers the worker-confirmed exit_code and the true finish_time
* too, not just state. A payload of exactly sizeof(wal_jobstate_t)
* (no trailing bytes -- any WAL_JOB_FINISH entry written before this
* fix) decodes neither, matching the pre-fix state-only behavior
* exactly (finish_time/run_time/exit_info all left untouched on
* this seam). */
if (payload_len >= sizeof(wal_jobstate_t)) {
const wal_jobstate_t* js = (const wal_jobstate_t*)payload;
uint64_t finish_time = 0;
job_exitinfo_t* exit_info = NULL;
uint32_t extra = payload_len - (uint32_t)sizeof(wal_jobstate_t);
if (extra >= sizeof(finish_time)) {
memcpy(&finish_time,
(const uint8_t*)payload + sizeof(wal_jobstate_t),
sizeof(finish_time));
uint32_t exitinfo_len = extra - (uint32_t)sizeof(finish_time);
if (exitinfo_len > 0) {
exit_info = decode_jobexitinfo(
(const uint8_t*)payload + sizeof(wal_jobstate_t)
+ sizeof(finish_time),
exitinfo_len);
}
}
restore_job_finish_state(p->cluster, js->jobid, js->state,
finish_time, exit_info);
}
break;
}
case WAL_JOB_DISPATCH: {
/* bug-1678: split out of the shared group above -- this is the one
* fact type whose payload can carry a trailing Cap'n Proto JobAlloc
@ -3973,7 +4046,42 @@ int apply_committed_entry(persist_t* p, uint32_t wal_type, uint64_t lsn,
j->job.start_time = time(NULL);
}
} else if (wal_type == WAL_JOB_FINISH) {
j->job.finish_time = time(NULL);
/* bug-1689: a WAL_JOB_FINISH payload can carry a fixed
* finish_time plus a fixed exit_code beyond the
* wal_jobstate_t header (persist_submit_job_finish_fact's
* shape, mirroring bug-1687's WAL_JOB_START trailing-fields
* handling above). Decode and adopt the wire-carried
* finish_time/exit_code when present; a payload of exactly
* sizeof(wal_jobstate_t) (no trailing bytes -- any
* WAL_JOB_FINISH entry written before this fix)
* intentionally falls through to the old fabricated-
* time(NULL)/exit_info-untouched behavior for full backward
* compatibility. run_time is always re-derived from
* finish_time - start_time either way. */
if (len >= sizeof(wal_jobstate_t) + sizeof(uint64_t)) {
uint64_t wire_finish_time;
memcpy(&wire_finish_time,
(const uint8_t*)payload + sizeof(wal_jobstate_t),
sizeof(wire_finish_time));
j->job.finish_time = (time_t)wire_finish_time;
uint32_t exitinfo_len = len
- (uint32_t)sizeof(wal_jobstate_t)
- (uint32_t)sizeof(uint64_t);
if (exitinfo_len > 0) {
job_exitinfo_t* decoded = decode_jobexitinfo(
(const uint8_t*)payload + sizeof(wal_jobstate_t)
+ sizeof(uint64_t),
exitinfo_len);
if (decoded != NULL) {
if (j->job.exit_info != NULL)
free(j->job.exit_info);
j->job.exit_info = decoded;
}
}
} else {
j->job.finish_time = time(NULL);
}
j->job.run_time = (int)(j->job.finish_time - j->job.start_time);
} else if (wal_type == WAL_JOB_DISPATCH
&& len > sizeof(wal_jobstate_t)) {
@ -7034,6 +7142,39 @@ int persist_submit_job_start_fact(persist_t* p, uint64_t jobid, int state,
return rc;
}
/* bug-1689: WAL_JOB_FINISH's own richer sibling of persist_submit_job_fact()
* -- mirrors persist_submit_job_start_fact() (bug-1687) exactly, but for the
* worker-confirmed-finish fact: the sole durable record of the job's real
* exit_code and its true finish_time. Payload shape: wal_jobstate_t header,
* immediately followed by a fixed uint64_t finish_time (the receipt-time
* clock, captured ONCE by the caller rather than re-derived at each apply
* seam's own apply-time moment), immediately followed by a fixed int32_t
* exit_code -- a small NATIVE trailer, not capnp (job_exitinfo_t is a single
* int with no existing capnp mapping, unlike WAL_JOB_START's variable-length
* JobPIDInfo, so no malloc is needed here -- the whole payload is a fixed
* size). apply_committed_entry()'s and apply_wal_entry()'s WAL_JOB_FINISH
* cases decode exactly this shape, and treat a payload of exactly
* sizeof(wal_jobstate_t) (no trailing bytes) as "state-only, fabricate
* finish_time and leave exit_info untouched" for backward compatibility
* with any WAL_JOB_FINISH entry written before this fix. run_time is NOT
* carried -- it is re-derived (finish_time - start_time) at each apply seam
* once both are known. */
int persist_submit_job_finish_fact(persist_t* p, uint64_t jobid, int state,
uint64_t finish_time, int exit_code)
{
wal_jobstate_t js = { .jobid = jobid, .state = (uint32_t)state,
._pad = 0 };
int32_t ec = (int32_t)exit_code;
uint8_t payload[sizeof(js) + sizeof(finish_time) + sizeof(ec)];
memcpy(payload, &js, sizeof(js));
memcpy(payload + sizeof(js), &finish_time, sizeof(finish_time));
memcpy(payload + sizeof(js) + sizeof(finish_time), &ec, sizeof(ec));
return wal_hook_append_classified(p, WAL_JOB_FINISH, WAL_MODE_RETAIN,
payload, (uint32_t)sizeof(payload));
}
/* bug-1679: see persist.h's doc comment for the full rationale. Mirrors
* apply_committed_entry()'s own identical Hazard-1-guarded hook fire
* (its shared WAL_JOB_START/.../DISPATCH case, right after its