From da7813bfdb72b92274f4919633871bd5b84c4a24 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Tue, 25 Aug 2026 03:44:15 +0000 Subject: [PATCH] fix(controller): notify apiserver synchronously on live dispatch (bug-1679) dispatch_a_job_() mutates job.state/job.allocs directly then persists, firing no hooks at all. apply_committed_entry()'s later apply of that same WAL_JOB_DISPATCH entry sees state already matches the value it just set moments earlier (under the same lock) and reports JOB_TRANSITION_NOOP_DUPLICATE, silently skipping its own would-be notify. Result: the apiserver is never told a job reached RUN -- xcecli jobs list shows it stuck at PEND forever, no restart involved. New persist_notify_job_updated_already_persisted(cluster_t*, job_t*) (persist.c/persist.h), called from dispatch_a_job_()'s success branch right after unlock(j). Mirrors apply_committed_entry()'s own identical Hazard-1-guarded hook fire exactly -- same t_apply_seam_notifying guard, same hook (HOOK_DETAIL_JOB_UPDATE's AFTER_EVENT), same detail -- without which persist_job_hook_fun (also subscribed to this hook) would redundantly re-append the transition as a second, duplicate WAL_JOB_UPDATE entry. The flag stays file-private to persist.c; this is job.c's only sanctioned way to reach it. record_job_start_fact()/record_job_finish_fact() needed no change -- their durable-append-only pattern (defer mutation+notify to apply_committed_entry entirely) is already correct; only dispatch_a_job_()'s broken hybrid (direct mutation, no notify at all) was ever the problem. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01AgYRU3zDTqNGV5Jk8aiKLg --- src/controller/job.c | 12 ++++++++++++ src/controller/persist.c | 16 ++++++++++++++++ src/include/persist.h | 19 +++++++++++++++++++ 3 files changed, 47 insertions(+) diff --git a/src/controller/job.c b/src/controller/job.c index 98c052ad..5134dbee 100644 --- a/src/controller/job.c +++ b/src/controller/job.c @@ -1404,6 +1404,18 @@ static int dispatch_a_job_(cluster_t* c, endpoint_t* ep, xce_job_alloc_t* alloc) JOB_STAT_RUN, j->job.allocs); unlock(j); + + /* bug-1679: the mutation above is live and synchronous -- unlike + * record_job_start_fact()'s durable-append-only sibling, this + * function never defers to apply_committed_entry() for its own + * notification, and by the time that WAL_JOB_DISPATCH entry does + * apply, state already matches (a correct, harmless no-op) so its + * own hook-fire is silently skipped. Without this explicit call, + * the apiserver is never told this job reached RUN. persist.h's + * doc comment on persist_notify_job_updated_already_persisted has + * the full rationale and the double-WAL-append hazard it guards + * against. */ + persist_notify_job_updated_already_persisted(c, j); } else { xce_dispatch_fail_class_t dfclass; job_failinfo_t failinfo = { .jobid = alloc->jobid }; diff --git a/src/controller/persist.c b/src/controller/persist.c index 6e39c76e..b4ae59a0 100644 --- a/src/controller/persist.c +++ b/src/controller/persist.c @@ -6855,6 +6855,22 @@ int persist_submit_job_dispatch_fact(persist_t* p, uint64_t jobid, int state, return rc; } +/* 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 + * `if (result != JOB_TRANSITION_APPLIED) return 0;` line) exactly -- + * same guard, same hook, same detail. The only difference is WHO is + * calling: there it is the apply seam notifying after a REPLAYED + * transition; here it is a live caller (dispatch_a_job_()) notifying + * after a transition it already durably persisted itself. */ +void persist_notify_job_updated_already_persisted(cluster_t* c, job_t* j) +{ + t_apply_seam_notifying = true; + xce_run_hooks(&c->hooksys, XCE_CONTROLLER_HOOK_JOB, HOOK_AFTER_EVENT, + HOOK_DETAIL_JOB_UPDATE, c, j); + t_apply_seam_notifying = false; +} + /* P0 payload: just the detail value (4 bytes). * Full Cap'n Proto serialization is wired in Task 15. */ diff --git a/src/include/persist.h b/src/include/persist.h index 45dcc6d9..40fe0d01 100644 --- a/src/include/persist.h +++ b/src/include/persist.h @@ -1421,6 +1421,25 @@ int persist_submit_job_fact_ex(persist_t* p, uint32_t wal_type, int persist_submit_job_dispatch_fact(persist_t* p, uint64_t jobid, int state, const xce_job_alloc_t* alloc); +/* bug-1679: fire XCE_CONTROLLER_HOOK_JOB's HOOK_DETAIL_JOB_UPDATE AFTER_EVENT + * for a job whose durable WAL fact the caller has ALREADY appended itself + * (e.g. dispatch_a_job_()'s own persist_submit_job_dispatch_fact() call, + * made just before this). Without this call, dispatch_a_job_()'s live, + * synchronous state/allocs mutation is never notified: it fires no hooks at + * all, and apply_committed_entry()'s later apply of that same WAL_JOB_DISPATCH + * entry sees the state already matches (JOB_TRANSITION_NOOP_DUPLICATE) and + * skips its own would-be notify -- the job is stuck at PEND from the + * apiserver's perspective forever, restart or not. + * + * Internally guards with the SAME t_apply_seam_notifying flag + * apply_committed_entry()'s own shared WAL_JOB_START/.../DISPATCH case uses + * around its identical hook fire (mirrors that exact bracketing) -- without + * it, persist_job_hook_fun (also subscribed to this hook) would redundantly + * re-append this same transition as a second, duplicate WAL_JOB_UPDATE + * entry. The flag stays file-local (static) to persist.c on purpose; this + * function is job.c's only sanctioned way to reach it. */ +void persist_notify_job_updated_already_persisted(cluster_t* c, job_t* j); + int persist_worker_hook_fun(int type, int situation, int detail, void* cls, va_list ap);