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

fix(controller): WAL_JOB_RUSAGE loses job.run_usage on replay (bug-1690)

Browse source 
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
This commit is contained in:
Rongsong Shen 2026-08-25 10:53:32 +00:00
parent c3d6cfa4cc
commit f7b732a300
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
8 changed files with 1328 additions and 2 deletions
Download patch file Download diff file Expand all files Collapse all files

191
src/controller/persist.c
View file

@ -63,6 +63,30 @@ extern int restore_job_start_state(cluster_t* c, uint64_t jobid,
extern int restore_job_finish_state(cluster_t* c, uint64_t jobid,
uint32_t state, uint64_t finish_time,
job_exitinfo_t* exit_info);
/* restore_job_rusage_state defined in job.c (bug-1690): WAL_JOB_RUSAGE's own
* sibling of restore_job_state() above, additionally taking ownership of an
* optional decoded xce_job_runusage_t (NULL for a state-only WAL_JOB_RUSAGE
* entry -- either one written before this fix, or one fired while
* job.run_usage was still NULL) so the client-reported per-host CPU/mem
* usage survives standby catch-up / replicated-apply the same way
* apply_committed_entry's WAL_JOB_RUSAGE case already restores it on the
* primary's own boot replay. */
extern int restore_job_rusage_state(cluster_t* c, uint64_t jobid,
uint32_t state,
xce_job_runusage_t* runusage);
/* merge_job_runusage defined in job.c (bug-1690): merges a freshly-decoded
* xce_job_runusage_t into *dst rather than freeing-and-replacing it. Needed
* here too (apply_committed_entry's own WAL_JOB_RUSAGE branch below), not
* just by restore_job_rusage_state() above -- *dst can be the job's LIVE
* run_usage (update_job_rusages()/update_host_runusage(), misc.c, mutate it
* synchronously and in place, with an xstring-backed host_name, BEFORE the
* WAL fact is even appended), and persist_apply_pump_run() re-applies that
* SAME fact on the SAME live job moments later via apply_committed_entry --
* a plain free()-and-replace (e.g. the capnp-generated free_JobRunusage_ptr)
* would free a non-malloc'd, RTTI-managed pointer there. See
* merge_job_runusage()'s own doc comment (job.c) for the full rationale. */
extern void merge_job_runusage(xce_job_runusage_t** dst,
xce_job_runusage_t* src);
extern int worker_from_capnp(cluster_t* c,
const void* capnp_payload, size_t len);
extern int restore_queue_state(cluster_t* c,
@ -300,6 +324,66 @@ static job_exitinfo_t* decode_jobexitinfo(const uint8_t* payload, size_t len)
return info;
}
/* Serialize xce_job_runusage_t to a malloc'd Cap'n Proto buffer (bug-1690):
* lets WAL_JOB_RUSAGE carry the client-reported, accumulated per-host
* CPU/mem usage (job->job.run_usage) alongside the {jobid, state} header, so
* a restart/catch-up replay can reconstruct it instead of leaving it
* permanently NULL. Mirrors capnp_encode_joballoc()/capnp_encode_pidinfo()
* above exactly, using the NEW JobRunusage struct (controller.capnp) --
* unlike JobAlloc/JobPIDInfo, xce_job_runusage_t had NO existing Cap'n Proto
* mapping at all, so this is a genuine schema addition (see the schema's own
* doc comment on JobRunusage for why it is not the same type as
* xce_job_rusages_t/JobRusages, a similar-looking but unrelated struct). */
static int capnp_encode_jobrunusage(const xce_job_runusage_t* r, uint8_t** out,
size_t* out_len)
{
struct capn c;
struct capn_segment* cs;
struct JobRunusage cjr;
JobRunusage_ptr jrp;
capn_init_malloc(&c);
cs = capn_root(&c).seg;
encode_JobRunusage(cs, &cjr, (xce_job_runusage_t*)r);
jrp = new_JobRunusage(cs);
write_JobRunusage(&cjr, jrp);
capn_setp(capn_root(&c), 0, jrp.p);
*out_len = capn_size(&c);
*out = malloc(*out_len);
if (!*out) { capn_free(&c); return -ENOMEM; }
capn_write_mem(&c, *out, *out_len, 0);
capn_free(&c);
return 0;
}
/* Decode a Cap'n Proto JobRunusage buffer (bug-1690) into a freshly heap-
* allocated xce_job_runusage_t*, mirroring decode_joballoc()/decode_pidinfo()'s
* pattern above. Returns NULL on a NULL/empty payload; caller owns the
* result (release via free_JobRunusage_ptr). */
static xce_job_runusage_t* decode_jobrunusage(const uint8_t* payload,
size_t len)
{
struct capn c_ctx;
JobRunusage_ptr jrp;
struct JobRunusage cjr;
xce_job_runusage_t* r;
if (!payload || len == 0)
return NULL;
capn_init_mem(&c_ctx, payload, len, 0);
jrp.p = capn_getp(capn_root(&c_ctx), 0, 1);
memset(&cjr, 0, sizeof(cjr));
read_JobRunusage(&cjr, jrp);
r = calloc(1, sizeof(*r));
if (!r) { capn_free(&c_ctx); return NULL; }
decode_JobRunusage(r, &cjr);
capn_free(&c_ctx);
return r;
}
/* 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
@ -3621,7 +3705,6 @@ int apply_wal_entry(persist_t* p, uint32_t type, uint64_t lsn,
case WAL_JOB_UPDATE:
case WAL_JOB_CKPT:
case WAL_JOB_PREASON:
case WAL_JOB_RUSAGE:
/* Payload: wal_jobstate_t {jobid, state} */
if (payload_len >= sizeof(wal_jobstate_t)) {
const wal_jobstate_t* js = (const wal_jobstate_t*)payload;
@ -3629,6 +3712,34 @@ int apply_wal_entry(persist_t* p, uint32_t type, uint64_t lsn,
}
break;
case WAL_JOB_RUSAGE: {
/* bug-1690: split out of the shared group above -- mirrors
* WAL_JOB_START's/WAL_JOB_FINISH's splits just below (bug-1687/
* bug-1689) and WAL_JOB_DISPATCH's split further below (bug-1678).
* This is the one fact type in that group whose payload can carry
* trailing Cap'n Proto JobRunusage bytes (persist_job_hook_fun's
* WAL_JOB_RUSAGE payload shape, appended only when job.run_usage
* was non-NULL at write time) alongside the {jobid, state} header,
* so a promoted standby's catch-up replay recovers the
* client-reported per-host CPU/mem usage too, not just state. A
* payload of exactly sizeof(wal_jobstate_t) (no trailing bytes --
* any WAL_JOB_RUSAGE entry written before this fix, or one fired
* while job.run_usage was still NULL) decodes no run_usage,
* matching the pre-fix state-only behavior exactly. */
if (payload_len >= sizeof(wal_jobstate_t)) {
const wal_jobstate_t* js = (const wal_jobstate_t*)payload;
xce_job_runusage_t* runusage = NULL;
if (payload_len > sizeof(wal_jobstate_t)) {
runusage = decode_jobrunusage(
(const uint8_t*)payload + sizeof(wal_jobstate_t),
payload_len - sizeof(wal_jobstate_t));
}
restore_job_rusage_state(p->cluster, js->jobid, js->state,
runusage);
}
break;
}
case WAL_JOB_START: {
/* bug-1687: split out of the shared group above -- mirrors
* WAL_JOB_DISPATCH's split just below (bug-1678). This is the one
@ -4001,6 +4112,41 @@ int apply_committed_entry(persist_t* p, uint32_t wal_type, uint64_t lsn,
lock(j, RTTI_LOCK_WRITE);
job_transition_result_t result =
apply_job_update_transition(j, (int)js->state);
/* bug-1690: WAL_JOB_RUSAGE carries the client-reported run_usage
* data independent of the job's state transition -- a rusage
* report almost never changes state (the job stays RUN across
* every repeat report), so gating this adoption on
* result==JOB_TRANSITION_APPLIED the way START/FINISH/DISPATCH do
* below would make it dead code for that realistic, common case:
* every report after the first would see JOB_TRANSITION_NOOP_
* DUPLICATE and the decode would never run. Decode/adopt
* unconditionally, before the transition-guarded block below,
* whether or not the state actually changed. A payload of exactly
* sizeof(wal_jobstate_t) (no trailing bytes -- any WAL_JOB_RUSAGE
* entry written before this fix, or one fired while job.run_usage
* was still NULL at write time) decodes no run_usage, leaving it
* untouched, matching the pre-fix behavior exactly.
*
* MERGE, not free-and-replace: on the primary, this exact branch
* re-runs via persist_apply_pump_run()'s live pump moments after
* update_job_rusages() already live-mutated j->job.run_usage in
* place (with an xstring-backed host_name, makeString(),
* src/lib/xstring.c) -- j->job.run_usage here can genuinely be that
* SAME live structure, not just a replay artifact. Freeing it via
* the capnp-generated free_JobRunusage_ptr() (written for plain
* malloc'd, capnp-decoded Text fields) would free a non-malloc'd,
* RTTI-managed pointer -- heap corruption. merge_job_runusage()
* (job.c) never frees an existing host_name; see its own doc
* comment for the full rationale. */
if (wal_type == WAL_JOB_RUSAGE && len > sizeof(wal_jobstate_t)) {
xce_job_runusage_t* decoded = decode_jobrunusage(
(const uint8_t*)payload + sizeof(wal_jobstate_t),
len - sizeof(wal_jobstate_t));
if (decoded != NULL)
merge_job_runusage(&(j->job.run_usage), decoded);
}
if (result == JOB_TRANSITION_APPLIED) {
/* apply_job_update_transition() only carries {jobid, state} on
* this wire shape (see the doc comment above) and never touches
@ -7236,6 +7382,14 @@ int persist_job_hook_fun(int type, int situation, int detail,
/* All other job events: payload = wal_jobstate_t {jobid, state}.
* State is read from the job_t after the AFTER_EVENT hook updates it. */
wal_jobstate_t js = {0};
/* bug-1690: WAL_JOB_RUSAGE's own optional trailing Cap'n Proto
* JobRunusage bytes -- populated only by the HOOK_DETAIL_JOB_RUSAGE_UPDATE
* arm below, consumed by the shared append at the bottom of this
* function. Every other detail leaves this NULL, so their payload stays
* the plain wal_jobstate_t it always was -- zero behavior change for
* them. */
uint8_t* rusage_buf = NULL;
size_t rusage_len = 0;
switch (detail) {
case HOOK_DETAIL_JOB_START: {
/* args: cluster_t*, job_pidinfo_t*, job_t* */
@ -7286,6 +7440,22 @@ int persist_job_hook_fun(int type, int situation, int detail,
if (j == NULL)
return 0; /* job gone by the time this fired -- nothing to log */
js.jobid = rusages->jobid; js.state = (uint32_t)j->job.state;
/* bug-1690: WAL_JOB_RUSAGE previously carried only {jobid, state}
* -- the same silent-field-drop bug class as bug-1678/1687/1689 --
* the client-reported per-host CPU/mem usage (j->job.run_usage,
* just mutated in-place by update_job_rusages() moments ago, under
* the same write lock this AFTER_EVENT fires inside) was never in
* the log at all, so a restart replay left job.run_usage
* permanently NULL. Encode it as a trailing Cap'n Proto JobRunusage
* blob when present -- it is NULL only in test doubles that stub
* out update_host_runusage (e.g. test_commit_barrier_reply.c); the
* real production caller always has one by the time this fires. A
* NULL run_usage falls back to the legacy state-only payload at
* this function's shared tail, unchanged. */
if (j->job.run_usage != NULL)
(void)capnp_encode_jobrunusage(j->job.run_usage, &rusage_buf,
&rusage_len);
break;
}
default: {
@ -7298,6 +7468,25 @@ int persist_job_hook_fun(int type, int situation, int detail,
}
/* mode == WAL_MODE_RETAIN — fact-shaped job state transition */
if (rusage_buf != NULL) {
/* bug-1690: append the header + trailing JobRunusage bytes as one
* contiguous payload -- persist_submit_job_dispatch_fact()'s/
* persist_submit_job_start_fact()'s own header+trailer construction
* (bug-1678/1687), inlined here since WAL_JOB_RUSAGE is hook-driven
* (from the raw rusages payload update_job_rusages() fires, not a
* direct persist_submit_job_*_fact() call site job.c could own). */
size_t total = sizeof(js) + rusage_len;
uint8_t* payload = malloc(total);
int rc;
if (!payload) { free(rusage_buf); return -ENOMEM; }
memcpy(payload, &js, sizeof(js));
memcpy(payload + sizeof(js), rusage_buf, rusage_len);
free(rusage_buf);
rc = wal_hook_append_classified(p, wal_type, mode, payload,
(uint32_t)total);
free(payload);
return rc;
}
return wal_hook_append_classified(p, wal_type, mode, &js, sizeof(js));
}