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

fix(controller): WAL_JOB_CKPT loses job.ckptinfo on replay (bug-1691)

Browse source 
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
This commit is contained in:
Rongsong Shen 2026-08-25 12:29:10 +00:00
parent b2a8fe4ce9
commit 977607615f
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
7 changed files with 955 additions and 3 deletions
Download patch file Download diff file Expand all files Collapse all files

173
src/controller/persist.c
View file

@ -11,6 +11,7 @@
#include "xlist.h"
#include "cluster.h"
#include "commit.h" /* commit_submit RETAIN registration (barrier-rollout 1.2) */
#include "xstring.h" /* makeString() -- bug-1691 WAL_JOB_CKPT ckptinfo adopt */
#include <assert.h>
#include <stdbool.h>
@ -74,6 +75,15 @@ extern int restore_job_finish_state(cluster_t* c, uint64_t jobid,
extern int restore_job_rusage_state(cluster_t* c, uint64_t jobid,
uint32_t state,
xce_job_runusage_t* runusage);
/* restore_job_ckpt_state defined in job.c (bug-1691): WAL_JOB_CKPT's own
* sibling of restore_job_state() above, additionally taking ownership of an
* optional decoded ckptinfo string (NULL for a state-only WAL_JOB_CKPT entry
* -- either one written before this fix, or one fired while job.ckptinfo was
* still NULL) so the checkpoint info survives standby catch-up / replicated-
* apply the same way apply_committed_entry's WAL_JOB_CKPT branch already
* restores it on the primary's own boot replay. */
extern int restore_job_ckpt_state(cluster_t* c, uint64_t jobid,
uint32_t state, char* ckptinfo);
/* 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
@ -324,6 +334,39 @@ static job_exitinfo_t* decode_jobexitinfo(const uint8_t* payload, size_t len)
return info;
}
/* Decode a native (non-capnp) WAL_JOB_CKPT trailing string payload (bug-1691)
* into a freshly heap-allocated, NUL-terminated char*. job.ckptinfo (xce/
* types.h) is a plain char* -- NOT the richer xce_ckptinfo_t/CheckpointInfo
* capnp struct used elsewhere for a different purpose -- so this mirrors
* WAL_QUEUE_CTRL's own [header][name NUL-terminated] wire shape
* (persist_queue_hook_fun / apply_wal_entry's WAL_QUEUE_CTRL case) rather
* than inventing a capnp wrapper for a bare string. Bounds-checked against
* `len` (the WAL entry's own payload_len, per the WAL_QUEUE_CTRL precedent)
* rather than trusting the bytes are NUL-terminated -- returns NULL if no
* NUL exists within the bounded region (a defensively-rejected malformed/
* truncated payload), mirroring decode_jobexitinfo()'s short-payload guard
* above. Returns NULL on a NULL/empty payload too; caller owns the result
* (release via free()). The returned buffer is a TRANSIENT decode copy, not
* itself adopted into job.ckptinfo -- makeString() (xstring.c) copies its
* input into its own refcounted allocation, so callers pass this result to
* makeString() and then free it (see restore_job_ckpt_state()/job.c and
* apply_committed_entry()'s own WAL_JOB_CKPT branch below). */
static char* decode_ckptinfo(const uint8_t* payload, size_t len)
{
char* copy;
if (!payload || len == 0)
return NULL;
if (memchr(payload, '\0', len) == NULL)
return NULL; /* not NUL-terminated within the declared bound -- reject */
copy = (char*)malloc(len);
if (!copy)
return NULL;
memcpy(copy, payload, len);
return copy;
}
/* 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
@ -3703,7 +3746,6 @@ int apply_wal_entry(persist_t* p, uint32_t type, uint64_t lsn,
case WAL_JOB_STOP:
case WAL_JOB_RESUME:
case WAL_JOB_UPDATE:
case WAL_JOB_CKPT:
case WAL_JOB_PREASON:
/* Payload: wal_jobstate_t {jobid, state} */
if (payload_len >= sizeof(wal_jobstate_t)) {
@ -3712,6 +3754,32 @@ int apply_wal_entry(persist_t* p, uint32_t type, uint64_t lsn,
}
break;
case WAL_JOB_CKPT: {
/* bug-1691: split out of the shared group above -- mirrors
* WAL_JOB_RUSAGE's split just below (bug-1690). This is the one
* fact type in that (former) group whose payload can carry a
* trailing NUL-terminated ckptinfo string (persist_job_hook_fun's
* HOOK_DETAIL_JOB_CKPT payload shape) alongside the {jobid, state}
* header, so a promoted standby's catch-up replay recovers the
* checkpoint info too, not just state. A payload of exactly
* sizeof(wal_jobstate_t) (no trailing bytes -- any WAL_JOB_CKPT
* entry written before this fix, or fired while job.ckptinfo was
* still NULL at write time) decodes no ckptinfo, 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;
char* ckptinfo = NULL;
if (payload_len > sizeof(wal_jobstate_t)) {
ckptinfo = decode_ckptinfo(
(const uint8_t*)payload + sizeof(wal_jobstate_t),
payload_len - sizeof(wal_jobstate_t));
}
restore_job_ckpt_state(p->cluster, js->jobid, js->state,
ckptinfo);
}
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/
@ -4147,6 +4215,49 @@ int apply_committed_entry(persist_t* p, uint32_t wal_type, uint64_t lsn,
merge_job_runusage(&(j->job.run_usage), decoded);
}
/* bug-1691: WAL_JOB_CKPT mirrors WAL_JOB_RUSAGE's unconditional-
* adopt pattern immediately above -- see that block's comment for
* the full rationale. A checkpoint report never changes job.state
* (on_job_ckpt(), job.c, only ever sets j->job.ckptinfo -- js->state
* is simply the job's UNCHANGED current state, captured verbatim by
* persist_job_hook_fun's HOOK_DETAIL_JOB_CKPT case), so gating this
* adoption on result==JOB_TRANSITION_APPLIED the way START/FINISH/
* DISPATCH do below would make it dead code: apply_job_update_
* transition() sees the wire state == the job's current state and
* reports JOB_TRANSITION_NOOP_DUPLICATE every time, so the gated
* block below would never run. Decode/adopt unconditionally, before
* the transition-guarded block. A payload of exactly
* sizeof(wal_jobstate_t) (no trailing bytes -- any WAL_JOB_CKPT
* entry written before this fix, or one fired while job.ckptinfo
* was still NULL at write time) decodes no ckptinfo, matching the
* pre-fix behavior exactly.
*
* j->job.ckptinfo is xstring-backed (makeString(), xstring.c), NOT
* a plain malloc'd/capnp-decoded string -- release the prior value
* via dereference() (the RTTI verb), never free()/FREEUP, mirroring
* on_job_ckpt()'s own dereference(j->job.ckptinfo) call. Unlike
* WAL_JOB_RUSAGE's run_usage, a free-and-replace here is safe (no
* merge needed): dereference() is the correct RTTI release call
* regardless of whether j->job.ckptinfo happens to be the SAME live
* xstring the primary's own live apply pump re-applies moments
* after on_job_ckpt() already set it -- it decrements a refcount
* rather than raw free()ing a non-owned pointer, so re-applying the
* identical fact is a harmless extra alloc/dereference cycle, not a
* hazard. decode_ckptinfo()'s result is a transient decode buffer
* (makeString() copies its input into its own allocation), so it is
* always freed here after use, whether or not adoption happened. */
if (wal_type == WAL_JOB_CKPT && len > sizeof(wal_jobstate_t)) {
char* decoded = decode_ckptinfo(
(const uint8_t*)payload + sizeof(wal_jobstate_t),
len - sizeof(wal_jobstate_t));
if (decoded != NULL) {
if (j->job.ckptinfo != NULL)
dereference(j->job.ckptinfo);
j->job.ckptinfo = makeString(decoded);
free(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
@ -7390,6 +7501,13 @@ int persist_job_hook_fun(int type, int situation, int detail,
* them. */
uint8_t* rusage_buf = NULL;
size_t rusage_len = 0;
/* bug-1691: WAL_JOB_CKPT's own optional trailing NUL-terminated
* ckptinfo string -- populated only by the HOOK_DETAIL_JOB_CKPT 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* ckpt_buf = NULL;
size_t ckpt_len = 0;
switch (detail) {
case HOOK_DETAIL_JOB_START: {
/* args: cluster_t*, job_pidinfo_t*, job_t* */
@ -7458,8 +7576,41 @@ int persist_job_hook_fun(int type, int situation, int detail,
&rusage_len);
break;
}
case HOOK_DETAIL_JOB_CKPT: {
/* args: cluster_t*, job_t* -- same shape as the default arm below,
* split out to also capture j->job.ckptinfo (bug-1691: same
* silent-field-drop bug class as bug-1678/1687/1689/1690 --
* WAL_JOB_CKPT previously carried only {jobid, state}, so a restart
* replay left job.ckptinfo permanently NULL even though
* on_job_ckpt() (job.c) had just set it, under the same write lock
* this AFTER_EVENT fires inside). j->job.ckptinfo is a plain
* NUL-terminated char* -- xstring-backed (makeString(), xstring.c),
* NOT the richer xce_ckptinfo_t/CheckpointInfo capnp struct used
* elsewhere for a different purpose -- so no capnp wrapper is
* needed; append it directly as a NUL-terminated string trailing
* the header, mirroring WAL_QUEUE_CTRL's own [header][name
* NUL-terminated] wire shape (persist_queue_hook_fun below)
* exactly. NULL only in test doubles that never set job.ckptinfo --
* the real production caller (on_job_ckpt) always sets it via
* makeString() before this hook fires; a NULL ckptinfo falls back
* to the legacy state-only payload at this function's shared tail,
* unchanged. */
(void)va_arg(ap, cluster_t*);
job_t* j = va_arg(ap, job_t*);
js.jobid = j->job.jobid; js.state = (uint32_t)j->job.state;
if (j->job.ckptinfo != NULL) {
ckpt_len = strlen(j->job.ckptinfo) + 1;
ckpt_buf = malloc(ckpt_len);
if (ckpt_buf != NULL)
memcpy(ckpt_buf, j->job.ckptinfo, ckpt_len);
else
ckpt_len = 0; /* OOM -- fall back to the state-only payload */
}
break;
}
default: {
/* STOP, RESUME, UPDATE, CKPT, PREASON — args: cluster_t*, job_t* */
/* STOP, RESUME, UPDATE, PREASON — args: cluster_t*, job_t* */
(void)va_arg(ap, cluster_t*);
job_t* j = va_arg(ap, job_t*);
js.jobid = j->job.jobid; js.state = (uint32_t)j->job.state;
@ -7487,6 +7638,24 @@ int persist_job_hook_fun(int type, int situation, int detail,
free(payload);
return rc;
}
if (ckpt_buf != NULL) {
/* bug-1691: append the header + trailing ckptinfo bytes as one
* contiguous payload -- mirrors the rusage_buf construction just
* above (bug-1690) and persist_queue_hook_fun's own header+trailer
* construction for WAL_QUEUE_CTRL (this function's own file, just
* below). */
size_t total = sizeof(js) + ckpt_len;
uint8_t* payload = malloc(total);
int rc;
if (!payload) { free(ckpt_buf); return -ENOMEM; }
memcpy(payload, &js, sizeof(js));
memcpy(payload + sizeof(js), ckpt_buf, ckpt_len);
free(ckpt_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));
}