fix(controller): resync_resolve_round never told scheduler RECONCILING cleared (bug-1841)
Worker-rejoin-resync sets XCE_WORKER_RECONCILING and pushes it to the scheduler, which correctly freezes that worker's resInUse mid-reconciliation (bug-1799/1801). But resync_resolve_round() only cleared the bit on the controller's own worker_t, never notifying the scheduler -- so once a rejoin round resolved, the scheduler's copy stayed RECONCILING forever, permanently freezing that worker's resource accounting even after every job on it finished. Live-observed via a replay capture: a 22-slot worker stuck at used=22.00 for the rest of the run with zero RUN jobs left. Added persist_notify_worker_status_updated() (mirrors bug-1679's job-side persist_notify_job_updated_already_persisted()) and call it from resync_resolve_round() so HOOK_DETAIL_WORKER_UPDATE fires without a spurious WAL append. TDD: new spy-hook regression test, RED confirmed against the unfixed code, GREEN after the fix. Full controller_unit_test suite: 1220 Tests, 0 Failures, 0 Ignored. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
parent
e1da9f1e9b
commit
1f6f219da6
5 changed files with 147 additions and 5 deletions
|
|
@ -24928,6 +24928,35 @@
|
|||
],
|
||||
"occurrences": 1,
|
||||
"last_seen": "2026-09-22T02:35:00Z"
|
||||
},
|
||||
{
|
||||
"id": "bug-1841",
|
||||
"timestamp": "2026-09-22T09:00:00Z",
|
||||
"error_message": "User-reported (xce_scheduler -i -s /tmp/scheduler-replay-20260922-071101.replay, worker pc13v, 22 real slots), also reported to occur in normal (non-sim) mode: after worker pc13v rejoined mid-replay (session~162, multiple WORKERS_PUSH+JOB_PUSH bursts, several `JOBS_PUSH: refusing resync state 0x1 for jobid ... existing job is RUN with a real allocation on record` WARNs -- bug-1812's guard correctly refusing a stale PEND resync), every one of that worker's jobs eventually legitimately finished (`info jobs` at session=423 shows all of jobids [0-22] DONE, [23-59] PEND, zero RUN), yet `info worker pc13v` still showed `resource slots: value=22.00 used=22.00` for the rest of the run (session 162 through 423, ~18 simulated minutes) -- the worker was permanently starved even though nothing was actually running on it.",
|
||||
"file": "src/controller/resync.c (resync_resolve_round() -- the fix), src/controller/persist.c/src/include/persist.h (new persist_notify_worker_status_updated(), mirroring persist_notify_job_updated_already_persisted()/bug-1679), src/controller/testsuite/test_worker_resync.c (new regression test)",
|
||||
"root_cause": "The worker-rejoin-resync feature's resync_arm_round() (resync.c) sets XCE_WORKER_RECONCILING on the controller's local worker_t.host.status and (via the HOOK_DETAIL_WORKER_JOIN hook already firing on rejoin) pushes it to the scheduler over WORKERS_PUSH. On the scheduler side, sched_reset_worker_()/sched_update_account() (bug-1799/bug-1801, 2026-09-11) deliberately FREEZE that worker's resInUse -- skip both the per-round zero-reset and the recharge -- while IS_WORKER_RECONCILING is set, by design, so in-flight reconciliation bookkeeping isn't clobbered mid-flight; bug-1799's own fix comment documents that normal reset+resum 'resumes and self-corrects on the very next round' once the status bit clears. But resync_resolve_round() (called from both the report-completion path, resync_apply_report -> resync_maybe_resolve, and the Req-5.3 timeout path, resync_on_timeout) only cleared XCE_WORKER_RECONCILING on the CONTROLLER's own local worker_t -- it never fired HOOK_DETAIL_WORKER_UPDATE or otherwise pushed the corrected status to the scheduler (contrast mark_worker_back()/worker.c:878-888, which does exactly that for the sibling XCE_WORKER_UNAVAIL bit). core_upsert_worker() (scheduler/src/core_events.c) only ever learns a worker's status from whatever WORKERS_PUSH it last received (`worker->status = w->status`, a bare unconditional copy, no other refresh path) -- so the scheduler's copy of the worker's status kept XCE_WORKER_RECONCILING set forever after the controller-side round resolved, permanently freezing resInUse at whatever it was when reconciliation started, even though every job on that worker had long since finished DONE. Reproduces identically in both sim/replay and live/normal mode since both drive the identical xce_core_begin_session() -> sched_reset_worker_()/sched_update_account() freeze logic; sim just replays whatever WORKERS_PUSH events the live controller actually sent, and the live controller never sent the correcting one either. test_worker_resync.c thoroughly tested that RECONCILING clears on the controller's OWN worker_t after resync_resolve_round(), but nothing tested that the scheduler-facing notification actually fires -- a gap noted separately during controller-peers-env-fallback design research (Sep 15 observation: 'full resync path lacks test coverage').",
|
||||
"fix": "FIXED (2026-09-22). Added persist_notify_worker_status_updated(cluster_t* c, worker_t* w) (persist.c, declared in persist.h next to persist_worker_hook_fun) -- the worker-side sibling of persist_notify_job_updated_already_persisted() (bug-1679): fires XCE_CONTROLLER_HOOK_WORKER's HOOK_DETAIL_WORKER_UPDATE AFTER_EVENT under the existing t_apply_seam_notifying guard so persist_worker_hook_fun (also subscribed to this hook, unconditionally WAL-appends on AFTER_EVENT) does NOT durably persist a WAL_WORKER_CTRL fact for this fire -- correct, since XCE_WORKER_RECONCILING/resync_state_t is intentionally ephemeral, re-derived fresh per rejoin, never replayed from WAL. resync_resolve_round() (resync.c) now calls this immediately after clearing XCE_WORKER_RECONCILING and detaching w->resync, guarded so it only fires when a round was genuinely outstanding (never on the already-documented idempotent no-op case). Verified safe under all three call sites' shared precondition (caller already holds w's write lock): confirmed neither HOOK_DETAIL_WORKER_UPDATE subscriber (sched_worker_change/apiserver_worker_change) nor persist_worker_hook_fun (guarded out entirely by the flag before touching w) locks w itself, so no deadlock risk. TDD: added test_resync_resolve_round_notifies_worker_update (test_worker_resync.c) -- a HOOK_DETAIL_WORKER_UPDATE spy hook proves resync_resolve_round() fires it exactly once per genuine resolve and not at all for the idempotent already-resolved no-op. RED confirmed first (Expected 1 Was 0) against the unfixed code, then GREEN after the fix. Full controller_unit_test suite: 1220 Tests, 0 Failures, 0 Ignored, OK (debug profile) -- no regressions. Note: the user's original .replay file is a frozen recording of the pre-fix controller's actual live traffic and will still show the frozen state when replayed, since sim mode only replays already-recorded events; the fix takes effect for new live runs/recordings going forward, not retroactively on old captures.",
|
||||
"tags": [
|
||||
"auto-detected",
|
||||
"resource-leak",
|
||||
"resync",
|
||||
"worker-rejoin-resync",
|
||||
"resInUse",
|
||||
"scheduler",
|
||||
"controller",
|
||||
"hook-notification",
|
||||
"fixed"
|
||||
],
|
||||
"related_bugs": [
|
||||
"bug-1679",
|
||||
"bug-1799",
|
||||
"bug-1801",
|
||||
"bug-1815",
|
||||
"bug-1812",
|
||||
"bug-1751"
|
||||
],
|
||||
"occurrences": 1,
|
||||
"last_seen": "2026-09-22T09:00:00Z"
|
||||
}
|
||||
]
|
||||
}
|
||||
|
|
|
|||
|
|
@ -7983,6 +7983,22 @@ void persist_notify_job_updated_already_persisted(cluster_t* c, job_t* j)
|
|||
t_apply_seam_notifying = false;
|
||||
}
|
||||
|
||||
/* bug-1841: see persist.h's doc comment on this declaration for the full
|
||||
* rationale. Same t_apply_seam_notifying-guarded shape as persist_notify_
|
||||
* job_updated_already_persisted() above, applied to worker status instead
|
||||
* of job state -- lets resync.c notify HOOK_DETAIL_WORKER_UPDATE's live
|
||||
* subscribers (sched_worker_change/apiserver_worker_change) that `w`'s
|
||||
* status changed WITHOUT persist_worker_hook_fun appending a WAL_WORKER_CTRL
|
||||
* fact for it, since resync.c's XCE_WORKER_RECONCILING coordination bit is
|
||||
* intentionally never durable. */
|
||||
void persist_notify_worker_status_updated(cluster_t* c, worker_t* w)
|
||||
{
|
||||
t_apply_seam_notifying = true;
|
||||
xce_run_hooks(&c->hooksys, XCE_CONTROLLER_HOOK_WORKER, HOOK_AFTER_EVENT,
|
||||
HOOK_DETAIL_WORKER_UPDATE, c, w);
|
||||
t_apply_seam_notifying = false;
|
||||
}
|
||||
|
||||
/* P0 payload: just the detail value (4 bytes).
|
||||
* Full Cap'n Proto serialization is wired in Task 15. */
|
||||
|
||||
|
|
|
|||
|
|
@ -393,11 +393,6 @@ int resync_arm_round(cluster_t* c, worker_t* w)
|
|||
|
||||
void resync_resolve_round(cluster_t* c, worker_t* w)
|
||||
{
|
||||
(void)c; /* accepted to match this file's other functions' shape and
|
||||
* what a later caller (task 3.2) already has in hand
|
||||
* alongside `w` -- this function's own body has nothing to do
|
||||
* with `c` today. */
|
||||
|
||||
if (w == NULL || w->resync == NULL)
|
||||
return;
|
||||
|
||||
|
|
@ -406,6 +401,22 @@ void resync_resolve_round(cluster_t* c, worker_t* w)
|
|||
free(w->resync->queried_jobids);
|
||||
free(w->resync);
|
||||
w->resync = NULL;
|
||||
|
||||
/* bug-1841: every call site here (resync_on_timeout, resync_maybe_
|
||||
* resolve via resync_settle_entry, and resync_arm_round's own "discard
|
||||
* a still-outstanding prior round" branch) already holds w's write
|
||||
* lock, matching this function's documented precondition -- and
|
||||
* persist_notify_worker_status_updated()'s only two live subscribers
|
||||
* (sched_worker_change/apiserver_worker_change) never lock `w`
|
||||
* themselves, so firing it here is safe. Without this, the scheduler's
|
||||
* own copy of `w`'s status (fed purely from whatever WORKERS_PUSH it
|
||||
* last received) keeps XCE_WORKER_RECONCILING set forever once this
|
||||
* clears it only in the controller's local worker_t -- permanently
|
||||
* freezing that worker's resInUse (bug-1799/1801's by-design freeze)
|
||||
* even after every job on it has finished. */
|
||||
if (c != NULL) {
|
||||
persist_notify_worker_status_updated(c, w);
|
||||
}
|
||||
}
|
||||
|
||||
int resync_round_is_current(worker_t* w, uint64_t token)
|
||||
|
|
|
|||
|
|
@ -1019,6 +1019,74 @@ static void test_resync_reconciling_flag_set_after_arm_cleared_after_resolve(
|
|||
"resolve: PASS\n");
|
||||
}
|
||||
|
||||
/* bug-1841 (new): resync_resolve_round() must notify HOOK_DETAIL_WORKER_
|
||||
* UPDATE subscribers once RECONCILING actually clears -- otherwise the
|
||||
* scheduler's own copy of this worker's status (fed purely from whatever
|
||||
* WORKERS_PUSH it last received; core_events.c's core_upsert_worker does a
|
||||
* bare `worker->status = w->status` with no other refresh path) stays
|
||||
* RECONCILING forever, permanently freezing that worker's resInUse
|
||||
* (sched_reset_worker_/sched_update_account's by-design bug-1799/1801
|
||||
* freeze) even after every job on it has long since finished DONE --
|
||||
* live-observed via a real scheduler-replay capture: a 22-slot worker
|
||||
* stuck at used=22.00 for the rest of the run with zero RUN jobs left.
|
||||
* Mirrors bug-1679's identical job-side gap and its
|
||||
* test_reconcile_pending_applies_releases_allocs_and_notifies spy-hook
|
||||
* proof above. */
|
||||
static int g_twr_worker_update_hook_fires;
|
||||
|
||||
static int twr_worker_update_spy_hook(int type, int situation, int detail,
|
||||
void* data, va_list ap)
|
||||
{
|
||||
(void)type; (void)data; (void)ap;
|
||||
if (situation == HOOK_AFTER_EVENT && detail == HOOK_DETAIL_WORKER_UPDATE)
|
||||
g_twr_worker_update_hook_fires++;
|
||||
return 0;
|
||||
}
|
||||
|
||||
static void test_resync_resolve_round_notifies_worker_update(void)
|
||||
{
|
||||
job_t* j;
|
||||
worker_t w;
|
||||
int rc;
|
||||
|
||||
twr_setup();
|
||||
g_twr_worker_update_hook_fires = 0;
|
||||
TEST_ASSERT_EQUAL_INT(0,
|
||||
xce_add_hook(&g_twr.hooksys, XCE_CONTROLLER_HOOK_WORKER,
|
||||
twr_worker_update_spy_hook, NULL));
|
||||
|
||||
j = twr_seed_job(TWR_JOBID_RESYNC_SOLO, JOB_STAT_RUN);
|
||||
TEST_ASSERT_NOT_NULL(j);
|
||||
j->job.allocs = twr_make_test_alloc_for_host(TWR_JOBID_RESYNC_SOLO,
|
||||
"resync-host-notify");
|
||||
|
||||
memset(&w, 0, sizeof(w));
|
||||
w.host.name = (char*)"resync-host-notify";
|
||||
|
||||
rc = resync_arm_round(&g_twr, &w);
|
||||
TEST_ASSERT_EQUAL_INT(1, rc);
|
||||
TEST_ASSERT_TRUE(IS_WORKER_RECONCILING(w.host.status));
|
||||
TEST_ASSERT_EQUAL_INT_MESSAGE(0, g_twr_worker_update_hook_fires,
|
||||
"arming a round must not itself fire HOOK_DETAIL_WORKER_UPDATE -- "
|
||||
"only resolving one does");
|
||||
|
||||
resync_resolve_round(&g_twr, &w);
|
||||
TEST_ASSERT_FALSE(IS_WORKER_RECONCILING(w.host.status));
|
||||
TEST_ASSERT_EQUAL_INT_MESSAGE(1, g_twr_worker_update_hook_fires,
|
||||
"resync_resolve_round must notify HOOK_DETAIL_WORKER_UPDATE "
|
||||
"subscribers (scheduler/apiserver) once RECONCILING actually "
|
||||
"clears -- otherwise the scheduler never learns and stays frozen");
|
||||
|
||||
/* Idempotent no-op: resolving an already-resolved round must NOT fire a
|
||||
* second, spurious notification. */
|
||||
resync_resolve_round(&g_twr, &w);
|
||||
TEST_ASSERT_EQUAL_INT_MESSAGE(1, g_twr_worker_update_hook_fires,
|
||||
"resolving an already-resolved (NULL) round must not re-notify");
|
||||
|
||||
twr_teardown();
|
||||
printf("test_resync_resolve_round_notifies_worker_update: PASS\n");
|
||||
}
|
||||
|
||||
/* Req 5.3: the timeout resolves an outstanding round -- and does so via a
|
||||
* plain, synchronous function call with no sleep/wait of any kind, proving
|
||||
* the resolution itself never blocks (ha-async-io.md's delaymgr
|
||||
|
|
@ -4759,6 +4827,7 @@ void test_worker_resync(void)
|
|||
RUN_TEST(test_resync_arm_round_no_jobs_is_noop);
|
||||
RUN_TEST(test_resync_arm_round_fresh_token_supersedes_prior);
|
||||
RUN_TEST(test_resync_reconciling_flag_set_after_arm_cleared_after_resolve);
|
||||
RUN_TEST(test_resync_resolve_round_notifies_worker_update);
|
||||
RUN_TEST(test_resync_timeout_resolves_round_without_blocking);
|
||||
RUN_TEST(test_apply_report_all_five_outcomes_updates_job_table);
|
||||
RUN_TEST(test_apply_report_duplicate_reapplication_is_noop);
|
||||
|
|
|
|||
|
|
@ -1527,6 +1527,23 @@ int persist_submit_job_finish_fact(persist_t* p, uint64_t jobid, int state,
|
|||
* function is job.c's only sanctioned way to reach it. */
|
||||
void persist_notify_job_updated_already_persisted(cluster_t* c, job_t* j);
|
||||
|
||||
/* bug-1841 (worker-side sibling of bug-1679/persist_notify_job_updated_
|
||||
* already_persisted above): fires XCE_CONTROLLER_HOOK_WORKER's
|
||||
* HOOK_DETAIL_WORKER_UPDATE AFTER_EVENT for `w`, guarded by the SAME
|
||||
* t_apply_seam_notifying flag so persist_worker_hook_fun (also subscribed
|
||||
* to this hook) does NOT append a fresh WAL_WORKER_CTRL fact for this fire.
|
||||
* Unlike the job version, nothing here was "already persisted" -- this is
|
||||
* for a worker mutation that is intentionally never durable at all (e.g.
|
||||
* resync.c's XCE_WORKER_RECONCILING coordination bit, which is armed and
|
||||
* cleared per rejoin and re-derived fresh every time, never replayed from
|
||||
* WAL) but still needs HOOK_DETAIL_WORKER_UPDATE's live subscribers
|
||||
* (sched_worker_change/apiserver_worker_change) to see `w`'s current
|
||||
* status. Without this, a subscriber's only view of `w->host.status` is
|
||||
* whatever the last WORKERS_PUSH-triggering hook fire carried -- resync.c's
|
||||
* own resync_resolve_round() clearing the bit in memory is invisible to
|
||||
* them until some unrelated worker event happens to re-fire the hook. */
|
||||
void persist_notify_worker_status_updated(cluster_t* c, worker_t* w);
|
||||
|
||||
int persist_worker_hook_fun(int type, int situation, int detail,
|
||||
void* cls, va_list ap);
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue