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

fix(bug-1731): fire HOOK_DETAIL_JOB_FAIL on WAL_JOB_FAIL apply

Browse source 
Follow-up to ebe691c3 (HOOK_DETAIL_JOB_START/JOB_EXIT), same gap for job
failures: apply_committed_entry()'s WAL_JOB_FAIL case only ever fired
the generic HOOK_DETAIL_JOB_UPDATE, leaving sched_job_fail_fun (sched.c)
and apiserver_job_fail_fun (apiserver.c) dead -- the apiserver's
OnJobFinished was starved of data for job failures specifically (the
prior fix only closed this for clean finishes).

Fires HOOK_DETAIL_JOB_FAIL only for the WAL_JOB_FAIL_STAGE_FLAG branch,
which already constructs a real job_failinfo_t -- the legacy _pad==0
absolute-overwrite branch has no fail-specific data and must not
fabricate one, mirroring how the WAL_JOB_START fix only fires when
pidinfo was genuinely decoded. Unlike WAL_JOB_START, no gate-widening
was needed: apply_job_fail_transition() always returns
JOB_TRANSITION_APPLIED for a genuine new fail transition (confirmed by
reading it), so the existing result-gated early return already covers
this correctly.

TDD: RED confirmed (fix reverted, test failed "Expected 2 Was 1"),
GREEN after restoring it. Full regression: 1220 tests, only the known
bug-1561 flake.

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 12:13:01 +00:00
parent 6193fa7ea1
commit 670098150d
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
3 changed files with 173 additions and 4 deletions
Download patch file Download diff file Expand all files Collapse all files

4
.wolf/buglog.json
View file

File diff suppressed because one or more lines are too long

31
src/controller/persist.c
View file

@ -4888,13 +4888,16 @@ int apply_committed_entry(persist_t* p, uint32_t wal_type, uint64_t lsn,
lock(j, RTTI_LOCK_WRITE);
job_transition_result_t result;
job_failinfo_t failinfo;
bool have_failinfo = false;
if (js->_pad & WAL_JOB_FAIL_STAGE_FLAG) {
job_failinfo_t failinfo = {
failinfo = (job_failinfo_t){
.jobid = js->jobid,
.stage = js->_pad & ~WAL_JOB_FAIL_STAGE_FLAG,
.code = 0,
.detail = NULL,
};
have_failinfo = true;
result = apply_job_fail_transition(j, &failinfo);
} else {
result = apply_job_update_transition(j, (int)js->state);
@ -4904,8 +4907,32 @@ int apply_committed_entry(persist_t* p, uint32_t wal_type, uint64_t lsn,
if (result != JOB_TRANSITION_APPLIED)
return 0; /* rejected-backward / no-op-duplicate — no hook */
/* Hazard 1 guard — see the identical guard above. */
/* Hazard 1 guard — see the identical guard above. Single bracket
* wraps both possible fires (bug-1731 follow-up adds
* HOOK_DETAIL_JOB_FAIL alongside the pre-existing JOB_UPDATE). */
t_apply_seam_notifying = true;
/* bug-1731 follow-up: HOOK_DETAIL_JOB_FAIL -- only for the
* WAL_JOB_FAIL_STAGE_FLAG branch, which is the only one that
* constructs a real job_failinfo_t; the legacy _pad==0 absolute-
* overwrite branch has no fail-specific data to report and must not
* fabricate one (mirrors bug-1731's own WAL_JOB_START fix, which
* only fires HOOK_DETAIL_JOB_START when pidinfo was genuinely
* decoded). Args mirror job.c's own HOOK_BEFORE_EVENT fire sites for
* this detail — (c, failinfo, job). Revives sched_job_fail_fun
* (sched.c) and apiserver_job_fail_fun (apiserver.c), previously
* permanently dead for the exact same reason HOOK_DETAIL_JOB_START/
* JOB_EXIT were (see persist.h's apply_committed_entry doc comment
* and bug-1731) -- the apiserver's OnJobFinished (Go side,
* NotifyData_data_jobFailInfo case) was starved of data for job
* FAILURES specifically (bug-1731's own fix only closed this for
* clean finishes, via WAL_JOB_FINISH/HOOK_DETAIL_JOB_EXIT). */
if (have_failinfo) {
xce_run_hooks(&c->hooksys, XCE_CONTROLLER_HOOK_JOB,
HOOK_AFTER_EVENT, HOOK_DETAIL_JOB_FAIL, c,
&failinfo, j);
}
xce_run_hooks(&c->hooksys, XCE_CONTROLLER_HOOK_JOB, HOOK_AFTER_EVENT,
HOOK_DETAIL_JOB_UPDATE, c, j);
t_apply_seam_notifying = false;

142
src/controller/testsuite/test_apply_committed_entry.c
View file

@ -92,6 +92,29 @@ static int tace_array_spy_hook(int type, int situation, int detail,
return 0;
}
/* ── Fail-detail-aware spy hook: like tace_spy_hook (counts every
* AFTER_EVENT firing into the same g_tace_hook_fires counter), additionally
* recording the job_failinfo_t.jobid seen on the most recent
* HOOK_DETAIL_JOB_FAIL firing (bug-1731 follow-up: HOOK_DETAIL_JOB_FAIL on
* WAL_JOB_FAIL apply). va_arg order mirrors job.c's own HOOK_BEFORE_EVENT
* fire sites for this detail -- (cluster_t*, job_failinfo_t*, job_t*). ──── */
static int g_tace_last_fail_jobid;
static int tace_faildetail_spy_hook(int type, int situation, int detail,
void* data, va_list ap)
{
(void)type; (void)data;
if (situation != HOOK_AFTER_EVENT)
return 0;
g_tace_hook_fires++;
if (detail == HOOK_DETAIL_JOB_FAIL) {
(void)va_arg(ap, cluster_t*);
job_failinfo_t* info = va_arg(ap, job_failinfo_t*);
g_tace_last_fail_jobid = info ? (int)info->jobid : -1;
}
return 0;
}
static void tace_setup(void)
{
kvmgr_params_t int_params = { .key_type = KVMGR_KEY_INT,
@ -131,6 +154,7 @@ static void tace_setup(void)
g_tace_p.cluster = &g_tace_c;
g_tace_hook_fires = 0;
g_tace_last_fail_jobid = -1;
}
static void tace_teardown(void)
@ -465,6 +489,121 @@ static void test_ace_job_finish_sets_finish_time_and_run_time(void)
printf("test_ace_job_finish_sets_finish_time_and_run_time: PASS\n");
}
/* ── WAL_JOB_FAIL (bug-1731 follow-up: HOOK_DETAIL_JOB_FAIL) ─────────────
* The identical dead-hook gap bug-1731 closed for WAL_JOB_START/FINISH
* existed for WAL_JOB_FAIL's deferred-fail-fact (WAL_JOB_FAIL_STAGE_FLAG)
* branch too -- sched_job_fail_fun (sched.c) and apiserver_job_fail_fun
* (apiserver.c) were permanently dead because HOOK_DETAIL_JOB_FAIL was never
* fired at HOOK_AFTER_EVENT. These three tests prove: (1) the STAGE_FLAG
* branch fires BOTH HOOK_DETAIL_JOB_FAIL and HOOK_DETAIL_JOB_UPDATE with the
* correct jobid, (2) the legacy _pad==0 absolute-overwrite branch fires ONLY
* HOOK_DETAIL_JOB_UPDATE -- it has no real job_failinfo_t to report and must
* not fabricate one, and (3) a guard-rejected transition still fires
* NEITHER hook (idempotency of this shared WAL-apply seam is unchanged). */
static void test_ace_job_fail_stage_flag_fires_faildetail_and_update(void)
{
tace_setup();
/* apply_job_fail_transition() (job.c) has no internal guard -- it
* unconditionally mutates and returns JOB_TRANSITION_APPLIED for any
* stage, so any seeded starting state genuinely applies. Seed a job
* already RUNNING with EXIT set, so the POST_EXEC branch merges
* JOB_STAT_PERR without the defensive "exit status not set" WARN path. */
job_t* j = tace_seed_job(605, JOB_STAT_RUN | JOB_STAT_EXIT);
TEST_ASSERT_NOT_NULL(j);
wal_jobstate_t js = { .jobid = 605,
.state = 0,
._pad = WAL_JOB_FAIL_STAGE_FLAG |
JOB_EXEC_STAGE_POST_EXEC };
TEST_ASSERT_EQUAL_INT(0,
xce_add_hook(&g_tace_c.hooksys, XCE_CONTROLLER_HOOK_JOB,
tace_faildetail_spy_hook, NULL));
int rc = apply_committed_entry(&g_tace_p, WAL_JOB_FAIL, 24, &js,
sizeof(js));
TEST_ASSERT_EQUAL_INT(0, rc);
TEST_ASSERT_TRUE_MESSAGE((j->job.state & JOB_STAT_PERR) != 0,
"a genuine STAGE_FLAG fail transition must merge JOB_STAT_PERR");
TEST_ASSERT_EQUAL_INT_MESSAGE(2, g_tace_hook_fires,
"a genuine STAGE_FLAG fail must fire BOTH HOOK_DETAIL_JOB_FAIL and "
"HOOK_DETAIL_JOB_UPDATE");
TEST_ASSERT_EQUAL_INT_MESSAGE(605, g_tace_last_fail_jobid,
"HOOK_DETAIL_JOB_FAIL must carry a job_failinfo_t with the correct "
"jobid");
tace_teardown();
printf("test_ace_job_fail_stage_flag_fires_faildetail_and_update: "
"PASS\n");
}
static void test_ace_job_fail_legacy_pad_zero_fires_update_only(void)
{
tace_setup();
/* Legacy live path (job_ctrl's mark_job_fail): _pad == 0, `state` is
* already the correct absolute value -- an absolute overwrite via
* apply_job_update_transition(), with no real job_failinfo_t to report.
* Regression guard: must NOT fabricate one for HOOK_DETAIL_JOB_FAIL. */
job_t* j = tace_seed_job(606, JOB_STAT_RUN);
TEST_ASSERT_NOT_NULL(j);
wal_jobstate_t js = { .jobid = 606, .state = JOB_STAT_EXIT, ._pad = 0 };
xce_add_hook(&g_tace_c.hooksys, XCE_CONTROLLER_HOOK_JOB,
tace_faildetail_spy_hook, NULL);
int rc = apply_committed_entry(&g_tace_p, WAL_JOB_FAIL, 25, &js,
sizeof(js));
TEST_ASSERT_EQUAL_INT(0, rc);
TEST_ASSERT_EQUAL_UINT32(JOB_STAT_EXIT, j->job.state);
TEST_ASSERT_EQUAL_INT_MESSAGE(1, g_tace_hook_fires,
"the legacy _pad==0 branch must fire exactly ONE notification "
"(HOOK_DETAIL_JOB_UPDATE only)");
TEST_ASSERT_EQUAL_INT_MESSAGE(-1, g_tace_last_fail_jobid,
"the legacy _pad==0 branch has no real job_failinfo_t and must NOT "
"fabricate one for HOOK_DETAIL_JOB_FAIL");
tace_teardown();
printf("test_ace_job_fail_legacy_pad_zero_fires_update_only: PASS\n");
}
static void test_ace_job_fail_guard_rejected_fires_no_hook(void)
{
tace_setup();
/* apply_job_fail_transition() (the STAGE_FLAG branch) has no guard and
* always applies -- a rejectable transition can only come through the
* legacy _pad==0 branch's apply_job_update_transition() guard. Job
* already terminal (DONE); a stale backward fail-derived UPDATE must be
* rejected, exactly mirroring
* test_ace_job_state_transition_rejects_backward_suppresses_notify. */
job_t* j = tace_seed_job(607, JOB_STAT_DONE);
TEST_ASSERT_NOT_NULL(j);
wal_jobstate_t js = { .jobid = 607, .state = JOB_STAT_RUN, ._pad = 0 };
xce_add_hook(&g_tace_c.hooksys, XCE_CONTROLLER_HOOK_JOB,
tace_faildetail_spy_hook, NULL);
int rc = apply_committed_entry(&g_tace_p, WAL_JOB_FAIL, 26, &js,
sizeof(js));
TEST_ASSERT_EQUAL_INT_MESSAGE(0, rc,
"a guard-rejected backward transition is a success no-op, not an "
"apply failure");
TEST_ASSERT_EQUAL_UINT32_MESSAGE(JOB_STAT_DONE, j->job.state,
"a rejected backward transition must preserve the terminal state");
TEST_ASSERT_EQUAL_INT_MESSAGE(0, g_tace_hook_fires,
"a guard-rejected WAL_JOB_FAIL transition must fire NEITHER hook");
TEST_ASSERT_EQUAL_INT_MESSAGE(-1, g_tace_last_fail_jobid,
"no HOOK_DETAIL_JOB_FAIL fire means no job_failinfo_t captured");
tace_teardown();
printf("test_ace_job_fail_guard_rejected_fires_no_hook: PASS\n");
}
/* ── WAL_QUEUE_CTRL ───────────────────────────────────────────────────── */
static void test_ace_queue_ctrl_updates_and_notifies(void)
@ -1106,6 +1245,9 @@ void test_apply_committed_entry(void)
RUN_TEST(test_ace_job_state_transition_rejects_backward_suppresses_notify);
RUN_TEST(test_ace_job_start_leaves_start_time_untouched);
RUN_TEST(test_ace_job_finish_sets_finish_time_and_run_time);
RUN_TEST(test_ace_job_fail_stage_flag_fires_faildetail_and_update);
RUN_TEST(test_ace_job_fail_legacy_pad_zero_fires_update_only);
RUN_TEST(test_ace_job_fail_guard_rejected_fires_no_hook);
RUN_TEST(test_ace_queue_ctrl_updates_and_notifies);
RUN_TEST(test_ace_worker_ctrl_join_restores_and_notifies);
RUN_TEST(test_ace_worker_ctrl_update_applies_to_existing_worker);