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

fix(bug-1731): fire HOOK_DETAIL_JOB_START/JOB_EXIT on WAL apply

Browse source 
apply_committed_entry() (persist.c), the single seam that applies every
committed WAL entry, only ever fired the generic HOOK_DETAIL_JOB_UPDATE
for job-state changes -- HOOK_DETAIL_JOB_START/JOB_EXIT were
permanently dead code, since job.c only ever fires them at
HOOK_BEFORE_EVENT (for WAL-append purposes). This left sched_job_start_
fun/sched_job_exit_fun (sched.c) and apiserver_job_start_fun/
apiserver_job_exit_fun (apiserver.c) unreachable, and the Go apiserver's
already-wired OnJobFinished handler permanently starved of data -- a
finished job's state could stay stuck (e.g. showing RUN) in the
scheduler's own view and the apiserver never learned a job finished.

The wire format was already enriched by bug-1697/bug-1689 with the
pidinfo/exit_info data these hooks need, so this closes the gap rather
than requiring new wire work. WAL_JOB_FINISH now fires
HOOK_DETAIL_JOB_EXIT; WAL_JOB_START now fires HOOK_DETAIL_JOB_START,
gated on a call-scoped decode flag rather than the transition result
(WAL_JOB_START applies as a no-op duplicate for every real job in
production per bug-1697, so gating on the transition result would make
it dead code again). WAL_JOB_FAIL -> HOOK_DETAIL_JOB_FAIL has the same
gap and is left as a follow-up.

Also includes this investigation's supporting changes: a DEBUG3 log-
retention bump (main.c, both controller and scheduler) so race-condition
evidence survives a live burst without rotating out, and DEBUG3-gated
diagnostic trace logging at the two points a job-update notification
can fire or silently drop (job.c, sched.c).

Live-verified end to end on a real deployed cluster: scheduler-side
sched_job_state_change now fires for RUN and EXIT states, and the
apiserver's OnJobFinished -- previously never invoked in production --
now correctly updates job status on completion.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019QxaSzHRB7pmYFp4cVnwkA
This commit is contained in:
Rongsong Shen 2026-08-29 11:29:10 +00:00
parent 42e1eeaa29
commit ebe691c3ed
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
9 changed files with 521 additions and 22 deletions
Download patch file Download diff file Expand all files Collapse all files

91
src/controller/persist.c
View file

@ -4644,6 +4644,17 @@ int apply_committed_entry(persist_t* p, uint32_t wal_type, uint64_t lsn,
* 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. */
/* bug-1731: pidinfo_just_decoded is a call-scoped flag, set ONLY
* where the decode-and-adopt block immediately below genuinely
* decodes a new job_pidinfo_t THIS call -- deliberately NOT the
* same thing as `j->job.pidinfo != NULL` (which could already be
* true from an earlier apply of this same job, and would wrongly
* re-trigger the HOOK_DETAIL_JOB_START fire below for an unrelated
* later transition). This flag gates that fire; see the fire site
* near the end of this case for why it must NOT be gated on
* `result == JOB_TRANSITION_APPLIED` the way HOOK_DETAIL_JOB_UPDATE
* is. */
bool pidinfo_just_decoded = false;
if (wal_type == WAL_JOB_START
&& len >= sizeof(wal_jobstate_t) + sizeof(uint64_t)) {
uint32_t pidinfo_len = (uint32_t)len
@ -4658,6 +4669,17 @@ int apply_committed_entry(persist_t* p, uint32_t wal_type, uint64_t lsn,
if (j->job.pidinfo != NULL)
free_JobPIDInfo_ptr(&(j->job.pidinfo));
j->job.pidinfo = decoded;
/* bug-1731 belt-and-suspenders: explicitly stamp jobid
* on the decoded struct rather than trusting the capnp
* decode alone -- cheap and always correct, and this is
* exactly the field sched_job_start_fun (sched.c)
* unconditionally dereferences (info->jobid, no NULL
* guard) the moment HOOK_DETAIL_JOB_START fires below;
* see persist.h's apply_committed_entry doc comment for
* the full NULL-safety history that made this detail
* permanently dead code until now. */
decoded->jobid = js->jobid;
pidinfo_just_decoded = true;
}
}
}
@ -4761,8 +4783,24 @@ int apply_committed_entry(persist_t* p, uint32_t wal_type, uint64_t lsn,
}
unlock(j);
if (result != JOB_TRANSITION_APPLIED)
return 0; /* rejected-backward / no-op-duplicate — no hook */
/* bug-1731: this gate used to be a flat `result != APPLIED` early
* return (no hook at all on a rejected-backward/no-op-duplicate
* transition). It now also lets a genuinely-just-decoded
* WAL_JOB_START pidinfo through even when `result` is a confirmed
* NOOP_DUPLICATE -- which, per bug-1697, is what EVERY real job's
* WAL_JOB_START fact applies as in production (dispatch_a_job_()
* already set state=RUN synchronously before WAL_JOB_START ever
* applies). Gating the new HOOK_DETAIL_JOB_START fire on
* result==JOB_TRANSITION_APPLIED, like HOOK_DETAIL_JOB_UPDATE below,
* would make it dead code for every real job -- exactly the bug
* being fixed here for HOOK_DETAIL_JOB_EXIT. A genuine idempotent
* re-apply of an already-applied WAL_JOB_FINISH still hits this same
* early return and fires NOTHING (pidinfo_just_decoded is only ever
* set inside the WAL_JOB_START branch above, so it stays false for
* every other wal_type in this group). */
if (result != JOB_TRANSITION_APPLIED && !pidinfo_just_decoded)
return 0; /* rejected-backward / no-op-duplicate, and no pidinfo
* just decoded either — nothing to notify */
/* Hazard 1 guard (see t_apply_seam_notifying's doc comment above):
* HOOK_DETAIL_JOB_UPDATE is dual-origin — job_ctrl's mark_job_update
@ -4770,10 +4808,53 @@ int apply_committed_entry(persist_t* p, uint32_t wal_type, uint64_t lsn,
* still-legitimate append via persist_job_hook_fun. Without this
* guard, EVERY fact this apply pump applies would re-append itself
* as a duplicate, mis-typed WAL_JOB_UPDATE entry the moment the live
* pump (task 6.1) starts calling this function. */
* pump (task 6.1) starts calling this function. Single bracket
* wraps ALL THREE possible fires below (bug-1731 adds JOB_START/
* JOB_EXIT alongside the pre-existing JOB_UPDATE) — do not narrow or
* duplicate this guard per-fire. */
t_apply_seam_notifying = true;
xce_run_hooks(&c->hooksys, XCE_CONTROLLER_HOOK_JOB, HOOK_AFTER_EVENT,
HOOK_DETAIL_JOB_UPDATE, c, j);
/* bug-1731: HOOK_DETAIL_JOB_START -- fires independent of `result`;
* see pidinfo_just_decoded's own doc comment at its declaration
* above, and the gate comment immediately above this bracket. Args
* mirror job.c's own HOOK_BEFORE_EVENT fire site for this exact
* detail (record_job_start_fact, job.c) — (c, info, job) — so every
* subscriber's va_arg reads stay consistent regardless of which
* situation/call site fired it. */
if (pidinfo_just_decoded) {
xce_run_hooks(&c->hooksys, XCE_CONTROLLER_HOOK_JOB,
HOOK_AFTER_EVENT, HOOK_DETAIL_JOB_START, c,
j->job.pidinfo, j);
}
if (result == JOB_TRANSITION_APPLIED) {
/* bug-1731: HOOK_DETAIL_JOB_EXIT for WAL_JOB_FINISH -- this is
* the PRIMARY fix for bug-1731 (apiserver's OnJobFinished was
* permanently starved of data; see persist.h's
* apply_committed_entry doc comment for the full history of why
* this detail was dead). exit_code is read from j->job.exit_info
* (decoded/adopted just above when the wire payload carried
* trailing bytes, bug-1689) — an old-format payload with no
* trailing bytes leaves exit_info NULL, so exit_code falls back
* to 0 rather than reading a dangling/garbage value. Args mirror
* job.c's own HOOK_BEFORE_EVENT fire site (record_job_finish_
* fact, job.c) — (c, finish, job). */
if (wal_type == WAL_JOB_FINISH) {
xce_jobfinish_t finish_info = {
.jobid = js->jobid,
.exit_code = (j->job.exit_info != NULL)
? j->job.exit_info->exitcode
: 0,
};
xce_run_hooks(&c->hooksys, XCE_CONTROLLER_HOOK_JOB,
HOOK_AFTER_EVENT, HOOK_DETAIL_JOB_EXIT, c,
&finish_info, j);
}
xce_run_hooks(&c->hooksys, XCE_CONTROLLER_HOOK_JOB,
HOOK_AFTER_EVENT, HOOK_DETAIL_JOB_UPDATE, c, j);
}
t_apply_seam_notifying = false;
return 0;
}