diff --git a/.wolf/buglog.json b/.wolf/buglog.json index 32f782af..35ce2091 100644 --- a/.wolf/buglog.json +++ b/.wolf/buglog.json @@ -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" } ] } diff --git a/src/controller/persist.c b/src/controller/persist.c index 8877e24f..4119f65d 100644 --- a/src/controller/persist.c +++ b/src/controller/persist.c @@ -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. */ diff --git a/src/controller/resync.c b/src/controller/resync.c index f1ba05bd..ba946bd0 100644 --- a/src/controller/resync.c +++ b/src/controller/resync.c @@ -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) diff --git a/src/controller/testsuite/test_worker_resync.c b/src/controller/testsuite/test_worker_resync.c index 17c110f0..b143b78c 100644 --- a/src/controller/testsuite/test_worker_resync.c +++ b/src/controller/testsuite/test_worker_resync.c @@ -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); diff --git a/src/include/persist.h b/src/include/persist.h index f325015b..a93bb236 100644 --- a/src/include/persist.h +++ b/src/include/persist.h @@ -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);