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

fix(controller): job.start_time never set -- dispatch/WAL_JOB_START RUN-state collision (bug-1697)

Browse source 
dispatch_a_job_() synchronously sets state=JOB_STAT_RUN at dispatch time
(scheduler-dispatch-commit task 3.1's deliberate no-worker-ack-wait
semantic). By the time the worker's later WAL_JOB_START fact commits,
apply_job_update_transition() sees state already == RUN and returns
NOOP_DUPLICATE instead of APPLIED -- so bug-1687's start_time/pidinfo
decode/adopt block, gated on result==APPLIED, could never run for any
job. start_time (and consequently run_time) stayed 0 forever. Same bug
class already fixed for WAL_JOB_RUSAGE/CKPT/PREASON in this identical
apply function; WAL_JOB_START was missed because bug-1687 predated the
dispatch-time RUN semantic change.

Moves start_time to dispatch-owned: dispatch_a_job_() now stamps
job.start_time alongside state=RUN, under the same write lock.
WAL_JOB_DISPATCH gains a flag-gated (WAL_JOB_DISPATCH_HAS_START_TIME_FLAG)
start_time field so it survives replay/restart; both apply seams
(apply_committed_entry, apply_wal_entry) and restore_job_dispatch_state
decode it when present, falling through to the old alloc-only shape
otherwise. WAL_JOB_START's own now-permanently-dead start_time write is
removed; pidinfo adoption made unconditional to match.

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-27 08:25:40 +00:00
parent faedc6fc49
commit ac1e312e2f
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
9 changed files with 859 additions and 184 deletions
Download patch file Download diff file Expand all files Collapse all files

252
src/controller/persist.c
View file

@ -40,19 +40,25 @@ extern int restore_job_state(cluster_t* c, uint64_t jobid, uint32_t state);
* an optional decoded xce_job_alloc_t so the dispatch-time resource
* allocation survives standby catch-up / replicated-apply the same way
* apply_committed_entry's WAL_JOB_DISPATCH case already restores it on the
* primary's own boot replay. */
* primary's own boot replay. bug-1697: also takes a wire-carried
* `start_time` (0 means "not carried -- leave job.start_time untouched",
* the pre-fix payload's behavior) -- job.start_time is now DISPATCH-owned
* (see wal_types.h's WAL_JOB_START doc comment for the full rationale), so
* this seam must restore it too, not just state/allocs. */
extern int restore_job_dispatch_state(cluster_t* c, uint64_t jobid,
uint32_t state, xce_job_alloc_t* alloc);
uint32_t state, uint64_t start_time,
xce_job_alloc_t* alloc);
/* restore_job_start_state defined in job.c (bug-1687): WAL_JOB_START's own
* sibling of restore_job_state() above, additionally taking ownership of an
* optional decoded job_pidinfo_t and a wire-carried start_time (0 means
* "not carried -- leave job.start_time untouched", the pre-fix payload's
* behavior) so the worker-confirmed start survives standby catch-up /
* replicated-apply the same way apply_committed_entry's WAL_JOB_START case
* already restores it on the primary's own boot replay. */
* optional decoded job_pidinfo_t so the worker-confirmed start survives
* standby catch-up / replicated-apply the same way apply_committed_entry's
* WAL_JOB_START case already restores it on the primary's own boot replay.
* bug-1697: no longer takes/touches start_time -- job.start_time is now
* exclusively DISPATCH-owned (restore_job_dispatch_state() above), so this
* seam must leave it alone entirely rather than risk clobbering the correct
* dispatch-time value with a stale worker-ack-time one. */
extern int restore_job_start_state(cluster_t* c, uint64_t jobid,
uint32_t state, uint64_t start_time,
job_pidinfo_t* pidinfo);
uint32_t state, 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
@ -4103,32 +4109,35 @@ int apply_wal_entry(persist_t* p, uint32_t type, uint64_t lsn,
* start_time PLUS trailing Cap'n Proto JobPIDInfo bytes
* (persist_submit_job_start_fact's shape) alongside the {jobid,
* state} header, so a promoted standby's catch-up replay recovers
* the worker-confirmed pids/pgids/exdata and the true start_time
* too, not just state. A payload of exactly sizeof(wal_jobstate_t)
* (no trailing bytes -- any WAL_JOB_START entry written before this
* fix) decodes neither, matching the pre-fix state-only behavior
* exactly. */
* the worker-confirmed pids/pgids/exdata too, not just state. A
* payload of exactly sizeof(wal_jobstate_t) (no trailing bytes --
* any WAL_JOB_START entry written before this fix) decodes no
* pidinfo, matching the pre-fix state-only behavior exactly.
*
* bug-1697: the wire-carried start_time field is decoded here only
* to locate where the trailing JobPIDInfo bytes start -- it is no
* longer passed to restore_job_start_state() at all. job.start_time
* is now exclusively DISPATCH-owned (restore_job_dispatch_state()
* below); leaving it untouched on this seam too prevents a stale
* worker-ack-time value from ever clobbering the correct
* dispatch-time one (see wal_types.h's WAL_JOB_START doc comment
* for the full rationale). */
if (payload_len >= sizeof(wal_jobstate_t)) {
const wal_jobstate_t* js = (const wal_jobstate_t*)payload;
uint64_t start_time = 0;
job_pidinfo_t* pidinfo = NULL;
job_pidinfo_t* pidinfo = NULL;
uint32_t extra = payload_len - (uint32_t)sizeof(wal_jobstate_t);
if (extra >= sizeof(start_time)) {
memcpy(&start_time,
(const uint8_t*)payload + sizeof(wal_jobstate_t),
sizeof(start_time));
uint32_t pidinfo_len = extra - (uint32_t)sizeof(start_time);
if (extra >= sizeof(uint64_t)) {
uint32_t pidinfo_len = extra - (uint32_t)sizeof(uint64_t);
if (pidinfo_len > 0) {
pidinfo = decode_pidinfo(
(const uint8_t*)payload + sizeof(wal_jobstate_t)
+ sizeof(start_time),
+ sizeof(uint64_t),
pidinfo_len);
}
}
restore_job_start_state(p->cluster, js->jobid, js->state,
start_time, pidinfo);
pidinfo);
}
break;
}
@ -4180,17 +4189,41 @@ int apply_wal_entry(persist_t* p, uint32_t type, uint64_t lsn,
* state. A payload of exactly sizeof(wal_jobstate_t) (no trailing
* bytes -- any WAL_JOB_DISPATCH entry written before this fix)
* decodes no allocation, matching the pre-fix state-only behavior
* exactly. */
* exactly.
*
* bug-1697: when WAL_JOB_DISPATCH_HAS_START_TIME_FLAG is set in
* js->_pad, a fixed uint64_t start_time immediately follows the
* header, BEFORE the JobAlloc bytes -- decode and pass it through to
* restore_job_dispatch_state() too (job.start_time is now
* DISPATCH-owned, see wal_types.h's WAL_JOB_START doc comment for
* the full rationale). The flag, not payload length, is the
* discriminator -- see that same doc comment for why a length-only
* test is ambiguous against JobAlloc's own variable length. An
* entry with the flag clear (any WAL_JOB_DISPATCH entry written
* before this fix) decodes the trailing bytes as JobAlloc only,
* leaving start_time at its "not carried" sentinel (0) -- full
* backward compatibility. */
if (payload_len >= sizeof(wal_jobstate_t)) {
const wal_jobstate_t* js = (const wal_jobstate_t*)payload;
xce_job_alloc_t* alloc = NULL;
if (payload_len > sizeof(wal_jobstate_t)) {
alloc = decode_joballoc(
(const uint8_t*)payload + sizeof(wal_jobstate_t),
payload_len - sizeof(wal_jobstate_t));
uint64_t start_time = 0;
const uint8_t* alloc_ptr =
(const uint8_t*)payload + sizeof(wal_jobstate_t);
uint32_t alloc_len =
payload_len - (uint32_t)sizeof(wal_jobstate_t);
if ((js->_pad & WAL_JOB_DISPATCH_HAS_START_TIME_FLAG) != 0
&& alloc_len >= sizeof(start_time)) {
memcpy(&start_time, alloc_ptr, sizeof(start_time));
alloc_ptr += sizeof(start_time);
alloc_len -= (uint32_t)sizeof(start_time);
}
if (alloc_len > 0) {
alloc = decode_joballoc(alloc_ptr, alloc_len);
}
restore_job_dispatch_state(p->cluster, js->jobid, js->state,
alloc);
start_time, alloc);
}
break;
}
@ -4591,51 +4624,55 @@ int apply_committed_entry(persist_t* p, uint32_t wal_type, uint64_t lsn,
}
}
/* bug-1697: WAL_JOB_START's pidinfo (worker-reported pids/pgids/
* exdata) must be decoded/adopted UNCONDITIONALLY, mirroring
* WAL_JOB_RUSAGE/WAL_JOB_CKPT/WAL_JOB_PREASON immediately above --
* dispatch_a_job_() now sets job.state = JOB_STAT_RUN synchronously
* at DISPATCH time (scheduler-dispatch-commit task 3.1), so by the
* time this WAL_JOB_START fact applies, apply_job_update_
* transition() above ALWAYS finds state already == RUN and returns
* JOB_TRANSITION_NOOP_DUPLICATE -- gating pidinfo adoption on
* result==JOB_TRANSITION_APPLIED (as the pre-fix code did, inside
* the block below) made it dead code for EVERY job, live, forever.
* job.start_time is DELIBERATELY not touched here at all any more:
* WAL_JOB_DISPATCH (further below) is now the sole owner of
* start_time, and this fact's own wire-carried start_time field
* (still written by persist_submit_job_start_fact() for wire-layout
* continuity) is intentionally skipped over -- never read, never
* adopted -- so it can never clobber the correct dispatch-time
* value once WAL_JOB_DISPATCH's own entry has already applied. A
* payload of exactly sizeof(wal_jobstate_t) (no trailing bytes at
* all -- any WAL_JOB_START entry written before bug-1687) decodes
* no pidinfo, matching the pre-1687 state-only behavior exactly. */
if (wal_type == WAL_JOB_START
&& len >= sizeof(wal_jobstate_t) + sizeof(uint64_t)) {
uint32_t pidinfo_len = (uint32_t)len
- (uint32_t)sizeof(wal_jobstate_t)
- (uint32_t)sizeof(uint64_t);
if (pidinfo_len > 0) {
job_pidinfo_t* decoded = decode_pidinfo(
(const uint8_t*)payload + sizeof(wal_jobstate_t)
+ sizeof(uint64_t),
pidinfo_len);
if (decoded != NULL) {
if (j->job.pidinfo != NULL)
free_JobPIDInfo_ptr(&(j->job.pidinfo));
j->job.pidinfo = 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
* start_time/finish_time/run_time itself — stamp them here,
* keyed on wal_type, mirroring what the richer
* apply_job_start_transition()/apply_job_finish_transition()
* cores do for their own (currently unreachable on this dispatch
* path) callers. */
if (wal_type == WAL_JOB_START) {
/* bug-1687: a WAL_JOB_START payload can carry a fixed
* start_time plus trailing Cap'n Proto JobPIDInfo bytes
* beyond the wal_jobstate_t header (persist_submit_job_
* start_fact's shape, mirroring bug-1678's WAL_JOB_DISPATCH
* trailing-JobAlloc-bytes handling below). Decode and adopt
* the wire-carried start_time/pidinfo when present; a
* payload of exactly sizeof(wal_jobstate_t) (no trailing
* bytes -- any WAL_JOB_START entry written before this fix)
* intentionally falls through to the old fabricated-
* time(NULL)/pidinfo-untouched behavior for full backward
* compatibility. */
if (len >= sizeof(wal_jobstate_t) + sizeof(uint64_t)) {
uint64_t wire_start_time;
memcpy(&wire_start_time,
(const uint8_t*)payload + sizeof(wal_jobstate_t),
sizeof(wire_start_time));
j->job.start_time = (time_t)wire_start_time;
uint32_t pidinfo_len = len
- (uint32_t)sizeof(wal_jobstate_t)
- (uint32_t)sizeof(uint64_t);
if (pidinfo_len > 0) {
job_pidinfo_t* decoded = decode_pidinfo(
(const uint8_t*)payload + sizeof(wal_jobstate_t)
+ sizeof(uint64_t),
pidinfo_len);
if (decoded != NULL) {
if (j->job.pidinfo != NULL)
free_JobPIDInfo_ptr(&(j->job.pidinfo));
j->job.pidinfo = decoded;
}
}
} else {
j->job.start_time = time(NULL);
}
} else if (wal_type == WAL_JOB_FINISH) {
* finish_time/run_time itself — stamp them here, keyed on
* wal_type, mirroring what the richer apply_job_finish_
* transition() core does for its own (currently unreachable on
* this dispatch path) callers. WAL_JOB_START no longer has a
* branch here (bug-1697) -- its only apply-time-relevant field,
* pidinfo, is decoded/adopted unconditionally above; start_time
* is DISPATCH-owned and is never touched by this fact type. */
if (wal_type == WAL_JOB_FINISH) {
/* 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
@ -4682,14 +4719,43 @@ int apply_committed_entry(persist_t* p, uint32_t wal_type, uint64_t lsn,
* payload of exactly sizeof(wal_jobstate_t) (no trailing
* bytes -- e.g. any WAL_JOB_DISPATCH entry written before
* this fix) intentionally falls through to the same
* state-only behavior every other type in this group gets. */
xce_job_alloc_t* decoded = decode_joballoc(
(const uint8_t*)payload + sizeof(wal_jobstate_t),
len - sizeof(wal_jobstate_t));
if (decoded != NULL) {
if (j->job.allocs != NULL)
free_JobAlloc_ptr(&(j->job.allocs));
j->job.allocs = decoded;
* state-only behavior every other type in this group gets.
*
* bug-1697: when WAL_JOB_DISPATCH_HAS_START_TIME_FLAG is set
* in js->_pad, a fixed uint64_t start_time immediately
* follows the header, BEFORE the JobAlloc bytes -- decode
* and adopt it here too (job.start_time is now DISPATCH-
* owned, see wal_types.h's WAL_JOB_START doc comment for the
* full rationale). The flag, not payload length, decides
* whether the field is present -- a length-only test cannot
* distinguish an old-format entry with a large alloc from a
* new-format entry with a small one. An entry with the flag
* clear (any WAL_JOB_DISPATCH entry written before this fix)
* skips straight to decoding the trailing bytes as JobAlloc
* only, leaving start_time untouched -- full backward
* compatibility. */
const uint8_t* alloc_ptr =
(const uint8_t*)payload + sizeof(wal_jobstate_t);
size_t alloc_len = len - sizeof(wal_jobstate_t);
if ((js->_pad & WAL_JOB_DISPATCH_HAS_START_TIME_FLAG) != 0
&& alloc_len >= sizeof(uint64_t)) {
uint64_t wire_start_time;
memcpy(&wire_start_time, alloc_ptr,
sizeof(wire_start_time));
j->job.start_time = (time_t)wire_start_time;
alloc_ptr += sizeof(uint64_t);
alloc_len -= sizeof(uint64_t);
}
if (alloc_len > 0) {
xce_job_alloc_t* decoded =
decode_joballoc(alloc_ptr, alloc_len);
if (decoded != NULL) {
if (j->job.allocs != NULL)
free_JobAlloc_ptr(&(j->job.allocs));
j->job.allocs = decoded;
}
}
}
}
@ -7655,17 +7721,24 @@ int persist_submit_job_fact_ex(persist_t* p, uint32_t wal_type,
* -- the ONLY job-state fact type that needs to carry a resource allocation
* alongside {jobid, state}, since it is the sole durable record of the
* dispatch-time PEND->RUN transition (design.md "WAL Dispatch Fact"). Payload
* shape: wal_jobstate_t header immediately followed by the Cap'n Proto-
* encoded JobAlloc bytes -- apply_committed_entry()'s and apply_wal_entry()'s
* WAL_JOB_DISPATCH cases decode exactly this shape, and treat a payload of
* exactly sizeof(wal_jobstate_t) (no trailing bytes) as "state-only, leave
* allocs untouched" for backward compatibility with any WAL_JOB_DISPATCH
* entry written before this fix. */
* shape (bug-1697 update): wal_jobstate_t header, with
* WAL_JOB_DISPATCH_HAS_START_TIME_FLAG set in _pad, immediately followed by
* a fixed uint64_t start_time (the dispatch-time clock, captured ONCE by the
* caller under the job's write lock -- job.start_time is now DISPATCH-owned,
* see wal_types.h's WAL_JOB_START doc comment for the full rationale),
* immediately followed by the Cap'n Proto-encoded JobAlloc bytes.
* apply_committed_entry()'s and apply_wal_entry()'s WAL_JOB_DISPATCH cases
* decode exactly this shape, keyed on the _pad flag (not payload length,
* which is ambiguous against JobAlloc's own variable length) -- an entry
* with the flag clear (any WAL_JOB_DISPATCH entry written before this fix)
* decodes the pre-fix shape (header + JobAlloc bytes only, start_time left
* untouched) for full backward compatibility. */
int persist_submit_job_dispatch_fact(persist_t* p, uint64_t jobid, int state,
uint64_t start_time,
const xce_job_alloc_t* alloc)
{
wal_jobstate_t js = { .jobid = jobid, .state = (uint32_t)state,
._pad = 0 };
._pad = WAL_JOB_DISPATCH_HAS_START_TIME_FLAG };
uint8_t* allocbuf = NULL;
size_t alloclen = 0;
uint8_t* payload;
@ -7676,11 +7749,12 @@ int persist_submit_job_dispatch_fact(persist_t* p, uint64_t jobid, int state,
if (rc != 0)
return rc;
total = sizeof(js) + alloclen;
total = sizeof(js) + sizeof(start_time) + alloclen;
payload = malloc(total);
if (!payload) { free(allocbuf); return -ENOMEM; }
memcpy(payload, &js, sizeof(js));
memcpy(payload + sizeof(js), allocbuf, alloclen);
memcpy(payload + sizeof(js), &start_time, sizeof(start_time));
memcpy(payload + sizeof(js) + sizeof(start_time), allocbuf, alloclen);
free(allocbuf);
rc = wal_hook_append_classified(p, WAL_JOB_DISPATCH, WAL_MODE_RETAIN,