feat(worker-rejoin-resync): task 5 - exclude reconciling host from dispatch

Extend the IS_WORKER_UNAVAIL-style candidate-host filter to also skip
a host with XCE_WORKER_RECONCILING set (Req 5.1), naturally
re-including it once the bit clears with no separate mechanism (Req
5.2). Leaves the scheduler's decision-publish code and its
symbol-guard regression test untouched.

Fixes bug-1751: design.md's cited filter site (scheduler.c's
sched_reset_worker_(), via sched_begin_session()) is dead code on both
live drivers - driver.c/sim_loop.c actually call
xce_core_begin_session() -> core_session.c's core_reset_workers(), an
independently-maintained mirror not listed in design.md's File
Structure Plan. Applied the identical disjunct to both sites so the
task's own observable-completion criterion holds in production, not
just on the literally-cited dead path.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y4M5MJM2zK6nU43da267Ku

.kiro/specs/worker-rejoin-resync/design.md

@@ -146,7 +146,7 @@ graph TB
 - `src/controller/join.c` — **no change**. `handle_worker_join` itself is untouched; the reconciliation query is sent from the (extended) `on_worker_join`, which `handle_worker_join` already calls unconditionally after the join reply.
 - `src/controller/job.c` — **Correction (2026-09-01, bug-1747, task 3.2): three new functions, not two — see below.** `record_job_reconcile_pending_fact()` and `record_job_reconcile_assign_fact()` both follow `dispatch_a_job_`'s eager-mutation-plus-explicit-notify pattern (not `record_job_finish_fact`'s deferred one — see Components for why): `record_job_reconcile_pending_fact()` (Requirement 4.6, eagerly calls `apply_job_update_transition(job, JOB_STAT_PEND)` under the write lock, releases `job->job.allocs` — `free_JobAlloc_ptr(&j->job.allocs); j->job.allocs = NULL;` — on an applied (non-no-op) transition, durably appends via the existing generic `persist_submit_job_fact(&c->persist, WAL_JOB_UPDATE, jobid, JOB_STAT_PEND)`, then calls `persist_notify_job_updated_already_persisted(c, job)` after unlocking; this is bug-1631's own documented candidate fix, applied here because Requirement 4.6 is exactly the "RUN job re-admitted to the PEND pool" precondition bug-1631's fix was deferred pending), `record_job_reconcile_assign_fact()` (Requirement 4.8, eagerly sets `job->job.state`/host assignment under the write lock, calls the existing `persist_submit_job_dispatch_fact(c->persist, jobid, state, ts, alloc=NULL)` — reusing bug-1678's already-tested NULL-alloc decode path on both apply seams verbatim, not a new fact shape — then calls `persist_notify_job_updated_already_persisted(c, job)` after unlocking, same as above and for the same reason). Existing `record_job_finish_fact`/`record_job_fail_fact` were originally scoped to be called as-is for the finished-success/finished-failure cases; **found live during task 3.2 implementation that `record_job_finish_fact`'s specific call site has no way to carry the worker-reported finish time (Requirement 4.2) — its own primitive, `persist_submit_job_finish_fact`, accepts one, but `record_job_finish_fact` always derives `time(NULL)` instead.** Fixed with a THIRD new function, `record_job_finish_fact_at(cluster_t*, job_t*, xce_jobfinish_t*, uint64_t finish_time_ms)` — `record_job_finish_fact`'s own worker-reported-finish-time sibling, following `record_job_finish_fact`'s own DEFERRED pattern (durable append only, not eager like the other two new functions above), differing only in passing the caller-supplied (ms→s converted) time to `persist_submit_job_finish_fact` instead of `time(NULL)`; `record_job_finish_fact` itself stays unmodified, still used as-is for `on_job_finish`'s ordinary (non-reconciliation) receipt path. `record_job_fail_fact` is still called as-is for the finished-failure case — Requirement 4.2 remains genuinely unsatisfiable there via any existing path (see the Persistence Field-Completeness Review section's correction note; tracked as the FAIL half of bug-1747, deliberately deferred). Also: split `dispatch_a_job_`'s `w->ep == NULL || w->host.status != 0` check (job.c:1511-1512) into two branches, checked in order — `w->ep == NULL` is resolved directly via `record_job_reconcile_pending_fact()` with **no retry-counter increment and no `mark_job_fail()`**, *before* the code ever reaches `xce_classify_dispatch_fail()` (a deliberate bypass, not a third value on the shared `xce_dispatch_fail_class_t` enum — see Components); `w->host.status != 0` keeps its existing `XCE_DISPATCH_FAIL_SITE_WORKER_NOT_READY` behavior unchanged (now rarely hit for `XCE_WORKER_RECONCILING` specifically, since the scheduler-side filter below keeps it from being selected in the first place).
 - `src/controller/cluster.c` — new `case OPCode_jobStateReport:` in `myreq_handler`'s dispatch switch (runs after `ha_dispatch_gate`); route to the new reconciliation-apply logic.
-- `src/scheduler/src/scheduler.c` — extend the existing candidate-host filter (`scheduler.c:1211`, today `if (IS_WORKER_UNAVAIL(w->status)) { ... }`) to also skip a host with the new reconciling bit set — same shape, one added disjunct, no new call site.
+- `src/scheduler/src/scheduler.c` — extend the existing candidate-host filter (`scheduler.c:1211`, today `if (IS_WORKER_UNAVAIL(w->status)) { ... }`) to also skip a host with the new reconciling bit set — same shape, one added disjunct, no new call site. **Correction (2026-09-01, bug-1751, task 5): `scheduler.c:1211`'s `sched_reset_worker_()` is reached only via `sched_begin_session()`, which has zero live callers in `driver.c`/`sim_loop.c` today — both call `xce_core_begin_session()` (`src/scheduler/src/core_session.c`) directly, which runs an independently-maintained mirror, `core_reset_workers()` (`core_session.c:102`, its own header comment: "mirrors scheduler.c's sched_reset_workers/sched_reset_worker_"), not listed below. The identical disjunct is required in BOTH files for Requirement 5.1/5.2 to hold on the actual live path; `scheduler.c:1211`'s copy is applied for parity/dead-code-hygiene but is not itself exercised in production today.
 - `src/include/xce/types.h` — add `XCE_WORKER_RECONCILING` to the worker status bitmask plus an `IS_WORKER_RECONCILING(status)` macro (mirroring `IS_WORKER_UNAVAIL`'s existing shape) for the scheduler-side filter above; add a `resync_state_t` (queried job-ID set, round token, deadline) referenced from `worker_t`.
 - `src/lib/request.c` — `xce_job_state_query_req()`/`xce_job_state_report_req()` encode/decode helpers, following `xce_reply_get`/`xce_push_data_req`'s pagination loop shape.
 

.kiro/specs/worker-rejoin-resync/tasks.md

@@ -104,7 +104,7 @@
   - _Boundary: xcecd_resync.c_
   - _Depends: 4.2_
 
-- [ ] 5. (P) Core: exclude a reconciling host from scheduler dispatch candidacy
+- [x] 5. (P) Core: exclude a reconciling host from scheduler dispatch candidacy
   - Extend the scheduler's existing candidate-host filter so a host currently undergoing job-state reconciliation is excluded from dispatch candidacy the same way an already-unavailable host is
   - When the reconciling status clears, the same filter must stop excluding that host without any separate action
   - Introduces no new reconciliation-protocol name or concept into the scheduler's outbound decision-publish code — this is an ordinary status-bit read alongside the existing unavailable-host check, not a new symbol; the standing regression test guarding that publish path against ever referencing reconciliation concepts must stay green, unmodified
@@ -165,3 +165,4 @@
 - Task 4.2: implemented `xcecd_resync_classify_job`/`xcecd_resync_build_report`/`xcecd_resync_answer_query` (classify RUNNING/FINISHED_OK/FINISHED_FAIL/UNRECOGNIZED, compute extras, paginate via task 1.4's `xce_job_state_report_req()`) as directly-callable, fully-tested functions (20/20 tests) — necessarily added one small, purely-additive companion function to task 4.1's API, `xcecd_finished_job_retention_snapshot()`, since 4.1 only exposed single-jobid lookup and 4.2 needs to enumerate ALL retained jobs for the extras set. Confirmed by inspection that nothing wires the worker's actual receipt of `OPCode_jobStateQuery` to call into this code yet (`controller_connector.c`'s `myreq_handler` has no case for it) — folded that wiring explicitly into task 4.3's scope (see its entry above), since 4.3 already needs the real receive path to implement its post-join gate correctly and is the same file/boundary.
 - **Task 4.3 needed 2 review rounds — a genuine deadlock, not a style nitpick.** Round 1 placed the `resync_pending` gate check as `checking_connection()`'s entry guard (returning early before `xcecli_hello()`/the `needs_rejoin` check could run), so a worker whose connection dropped after a non-final page of a multi-page `OPCode_jobStateQuery` (no further page ever arriving to clear the gate) would have its ONLY connection-recovery mechanism permanently disabled — a direct Requirement 6.1 violation the existing suite didn't catch because no test drove the multi-page-then-disconnect sequence. Round 2 fixed it two ways: (a) narrowed the gate to guard only the `rejoin_call()` step inside the `needs_rejoin` branch, so `xcecli_hello()`/reconnect-detection always run every tick regardless of gate state; (b) added `xcecd_resync_reset()`, called from `xcecd_on_reconnect()` (the same signal that sets `needs_rejoin`), which clears the gate and discards any stalled partial multi-page accumulation the moment a real reconnect completes — bounding the deadlock window to at most one reconnect cycle instead of forever. Round 2's reviewer independently confirmed the fix by reintroducing the round-1 bug and rebuilding, and found the new *behavioral* regression test alone did NOT catch the reintroduced bug — only a new *structural* source-order test (asserting `xcecli_hello()` precedes the `needs_rejoin` check precedes the gate check, textually) actually distinguished correct from broken placement. **Lesson for future tasks in this codebase**: when a fix's correctness hinges on WHERE a check is placed relative to other statements (not just what value it reads), a pure behavioral/outcome test can pass even with the wrong placement if the test's specific stimulus happens not to exercise the broken ordering — add a structural/ordering assertion alongside the behavioral one for placement-sensitive gates.
 - Task 4.3 (advisory, non-blocking, not fixed in this pass): `resync_pending`/`query_accum` in `xcecd_resync.c` are plain unsynchronized statics touched from two different OS threads — the worker's dedicated connector thread (`checking_connection()`/`xcecd_on_reconnect()`) and a `vtaskmgr` pool thread (`xcecd_resync_handle_query_page()`, reached via the request-dispatch path). This mirrors the pre-existing, design-directed unsynchronized `needs_rejoin` pattern (same codebase convention, deliberately reused per design.md), but is riskier since `xcecd_resync_reset()` calls `free()` on the accumulator rather than just staling a bool read — a narrow, non-deterministic window exists where an independent reconnect could free the buffer while a pool thread is mid-read of the same connection's in-flight query page. Does not reproduce task 4.3's own violated requirement (Req 6.1) and isn't required by any Requirement/AC as written; this project's ASAN/UBSAN test harness has no TSAN, so it also isn't mechanically testable the way the deadlock fix was. Worth a small follow-up (a mutex around the gate+accumulator, or moving accumulation to the connector thread only) if this ever needs hardening beyond what `needs_rejoin`'s own accepted risk profile already tolerates.
+- **Task 5 found and fixed bug-1751**: design.md's cited candidate-filter site (`scheduler.c:1211`'s `sched_reset_worker_()`, reached only via `sched_begin_session()`) is dead code on both live drivers — `driver.c`/`sim_loop.c` actually call `xce_core_begin_session()` (`core_session.c`), which runs an independently-maintained mirror, `core_reset_workers()`, not listed in design.md's File Structure Plan. Fixed by applying the identical `IS_WORKER_UNAVAIL || IS_WORKER_RECONCILING` disjunct to both sites (parity/dead-code-hygiene on the design-cited one, correctness on the actually-live one) rather than patching only the literally-cited site and leaving the task's own Observable Completion Criterion false in production — same category as bug-1746/bug-1747's design-vs-reality corrections. design.md's File Structure Plan corrected. Also confirmed design.md's Testing Strategy referenced an "existing IS_WORKER_UNAVAIL candidate-filter test" that does not actually exist; new coverage added to `test_core_session.c` instead (white-box `#include`s `core_session.c` already; `scheduler.c`'s own dependency closure is too large to white-box per established precedent, `test_snapshot_load_allocator_regression.c`) — `scheduler.c`'s copy of the fix is therefore verified only by compilation + the identical, tested logic pattern in its mirror, not by a direct test; judged acceptable since it's confirmed dead code today, not a live gap.

src/scheduler/src/core_session.c

@@ -99,7 +99,16 @@ static void core_reset_workers(xce_sched_core_t* core)
             break;
         }
 
-        if ((w == NULL) || IS_WORKER_UNAVAIL(w->status)) {
+        /* worker-rejoin-resync (Req 5.1/5.2): mirrors scheduler.c's
+         * sched_reset_worker_() disjunct exactly -- this function is that
+         * one's live, actually-called-every-round counterpart (see
+         * xce_core_begin_session()/driver.c/sim_loop.c), so the
+         * reconciling-host skip must be kept here too for the exclusion to
+         * be observable on either real driver, not just in the dormant
+         * sched_begin_session() path. Ordinary status-bit read, same shape
+         * as the existing disjunct, no new call site. */
+        if ((w == NULL) || IS_WORKER_UNAVAIL(w->status)
+            || IS_WORKER_RECONCILING(w->status)) {
             continue;
         }
 

src/scheduler/src/scheduler.c

@@ -1208,7 +1208,11 @@ static int sched_reset_worker_(xce_scheduler_t* sched, xce_sched_worker_t* w)
         return 0;
     }
 
-    if (IS_WORKER_UNAVAIL(w->status)) {
+    /* A host with job-state reconciliation outstanding gets the same
+     * per-session reset skip an unavailable host already does -- ordinary
+     * status-bit read, same shape as the existing disjunct, no new call
+     * site. */
+    if (IS_WORKER_UNAVAIL(w->status) || IS_WORKER_RECONCILING(w->status)) {
         return 0;
     }
 

src/scheduler/testsuite/test_core_session.c

@@ -340,6 +340,183 @@ static void test_end_session_unblocks_next_core_go(void)
         1, atomic_load_explicit(&(core.session_active), memory_order_acquire));
 }
 
+/* ---------------------------------------------------------------------- */
+/* xce_core_begin_session() / core_reset_workers() -- per-session worker  */
+/* reset skip for excluded hosts (worker-rejoin-resync spec, task 5,      */
+/* Req 5.1/5.2). core_reset_workers() (core_session.c) is the live,       */
+/* actually-called-every-round mirror of scheduler.c's sched_reset_       */
+/* worker_() -- xce_sched_driver_run_normal_mode_round() (driver.c) and   */
+/* sched_sim_driver_run() (sim_loop.c) both call xce_core_begin_session() */
+/* directly, not the legacy sched_begin_session()/sched_reset_workers()   */
+/* path, so this is where the RECONCILING exclusion must actually take   */
+/* effect to be observable on either real driver.                        */
+/*                                                                        */
+/* Observable signal: core_reset_workers() skips (does not zero) a       */
+/* worker's resInUse entries for a host it excludes, and resets them to  */
+/* 0 for a host it doesn't -- sched_worker_release_host_ctx() itself is  */
+/* stubbed to a no-op above (this TU's existing convention), so resInUse */
+/* is the one real, unstubbed effect this file can observe directly.     */
+/* ---------------------------------------------------------------------- */
+
+/* fixture_worker - builds a minimal xce_sched_worker_t with one resInUse
+ * entry pre-set to a known non-zero value (42.0), registered into
+ * core->workers under `name` via the same low-level key_add() primitive
+ * test_core_events.c's fixture_init() already uses directly for
+ * core->plugins (core->workers is populated by core_build_worker()/
+ * sched_add_worker() in production, both in core_events.c, a TU this file
+ * deliberately does not pull in -- see this file's own header comment). */
+static xce_sched_worker_t* fixture_worker(xce_sched_core_t* core,
+                                          const char* name, uint32_t status)
+{
+    xce_sched_worker_t* w = (xce_sched_worker_t*)calloc(1, sizeof(*w));
+    xce_val_t*          v = (xce_val_t*)calloc(1, sizeof(*v));
+    int                 rc;
+
+    TEST_ASSERT_NOT_NULL(w);
+    TEST_ASSERT_NOT_NULL(v);
+
+    w->name   = strdup(name);
+    w->status = status;
+    zarray_init(&(w->resInUse), 4, NULL);
+    zarray_init(&(w->resvals), 4, NULL);
+
+    v->which    = Value_u_fvalue;
+    v->u.fvalue = 42.0f;
+    zarray_put(&(w->resInUse), 0, v);
+
+    lock(core->workers, RTTI_LOCK_WRITE);
+    rc = key_add(core->workers, (void*)w->name, (void*)w);
+    unlock(core->workers);
+    TEST_ASSERT_GREATER_OR_EQUAL(0, rc);
+
+    return w;
+}
+
+static float worker_resinuse_0(xce_sched_worker_t* w)
+{
+    xce_val_t* v = (xce_val_t*)zarray_get(&(w->resInUse), 0);
+
+    TEST_ASSERT_NOT_NULL(v);
+    return v->u.fvalue;
+}
+
+static void test_begin_session_resets_available_host_resinuse(void)
+{
+    xce_sched_core_t    core;
+    xce_sched_worker_t* w;
+    int                 rc;
+
+    fixture_init(&core);
+    w = fixture_worker(&core, "avail-host", 0);
+
+    rc = xce_core_begin_session(&core);
+    TEST_ASSERT_EQUAL_INT(0, rc);
+
+    TEST_ASSERT_EQUAL_FLOAT_MESSAGE(
+        0.0f, worker_resinuse_0(w),
+        "baseline: an ordinary available host's resInUse IS reset every "
+        "session -- proves the fixture/harness itself is sound before the "
+        "exclusion-path assertions below rely on it");
+}
+
+static void test_begin_session_skips_unavailable_host_resinuse(void)
+{
+    xce_sched_core_t    core;
+    xce_sched_worker_t* w;
+    int                 rc;
+
+    fixture_init(&core);
+    w = fixture_worker(&core, "unavail-host", XCE_WORKER_UNAVAIL);
+
+    rc = xce_core_begin_session(&core);
+    TEST_ASSERT_EQUAL_INT(0, rc);
+
+    TEST_ASSERT_EQUAL_FLOAT_MESSAGE(
+        42.0f, worker_resinuse_0(w),
+        "baseline: the pre-existing IS_WORKER_UNAVAIL exclusion must keep "
+        "behaving exactly as before -- unchanged by this task's addition");
+}
+
+static void test_begin_session_skips_reconciling_host_resinuse(void)
+{
+    xce_sched_core_t    core;
+    xce_sched_worker_t* w;
+    int                 rc;
+
+    fixture_init(&core);
+    w = fixture_worker(&core, "reconciling-host", XCE_WORKER_RECONCILING);
+
+    rc = xce_core_begin_session(&core);
+    TEST_ASSERT_EQUAL_INT(0, rc);
+
+    TEST_ASSERT_EQUAL_FLOAT_MESSAGE(
+        42.0f, worker_resinuse_0(w),
+        "Req 5.1: a RECONCILING host must be excluded from the same "
+        "per-session reset an UNAVAIL host already is -- its stale "
+        "resInUse must NOT be zeroed while reconciliation is outstanding");
+}
+
+static void
+test_begin_session_skips_host_with_both_unavail_and_reconciling(void)
+{
+    /* Truth-table completeness: a host that is simultaneously UNAVAIL and
+     * RECONCILING must still be excluded (the two conditions are ORed,
+     * never ANDed) -- guards against exactly the class of orthogonal-
+     * condition mistake flagged for this feature's task 3.2. */
+    xce_sched_core_t    core;
+    xce_sched_worker_t* w;
+    int                 rc;
+
+    fixture_init(&core);
+    w = fixture_worker(&core, "both-host",
+                       XCE_WORKER_UNAVAIL | XCE_WORKER_RECONCILING);
+
+    rc = xce_core_begin_session(&core);
+    TEST_ASSERT_EQUAL_INT(0, rc);
+
+    TEST_ASSERT_EQUAL_FLOAT(42.0f, worker_resinuse_0(w));
+}
+
+static void test_begin_session_reincludes_host_once_reconciling_clears(void)
+{
+    /* Req 5.2: once XCE_WORKER_RECONCILING clears, the SAME filter must
+     * stop excluding the host on its very next evaluation -- no separate
+     * or new mechanism, just the live status-bit read falling through to
+     * the normal reset path. */
+    xce_sched_core_t    core;
+    xce_sched_worker_t* w;
+    int                 rc;
+    xce_val_t*          v;
+
+    fixture_init(&core);
+    w = fixture_worker(&core, "clears-host", XCE_WORKER_RECONCILING);
+
+    rc = xce_core_begin_session(&core);
+    TEST_ASSERT_EQUAL_INT(0, rc);
+    TEST_ASSERT_EQUAL_FLOAT(42.0f, worker_resinuse_0(w));
+
+    rc = xce_core_end_session(&core);
+    TEST_ASSERT_EQUAL_INT(0, rc);
+
+    /* Simulate a fresh non-zero in-use value still standing at the next
+     * session boundary (as it would if a job were still genuinely running
+     * there), then clear the reconciling bit -- the only state change
+     * Req 5.2 requires. */
+    v = (xce_val_t*)zarray_get(&(w->resInUse), 0);
+    TEST_ASSERT_NOT_NULL(v);
+    v->u.fvalue = 42.0f;
+    w->status   = 0;
+
+    rc = xce_core_begin_session(&core);
+    TEST_ASSERT_EQUAL_INT(0, rc);
+
+    TEST_ASSERT_EQUAL_FLOAT_MESSAGE(
+        0.0f, worker_resinuse_0(w),
+        "Req 5.2: clearing XCE_WORKER_RECONCILING alone must be enough to "
+        "make the host eligible again on the very next session -- no "
+        "separate re-enable action");
+}
+
 int main(void)
 {
     UNITY_BEGIN();
@@ -351,5 +528,10 @@ int main(void)
     RUN_TEST(test_core_set_decision_cb_registers_cb_and_driver_ctx);
     RUN_TEST(test_core_end_session_null_core_returns_einval);
     RUN_TEST(test_end_session_unblocks_next_core_go);
+    RUN_TEST(test_begin_session_resets_available_host_resinuse);
+    RUN_TEST(test_begin_session_skips_unavailable_host_resinuse);
+    RUN_TEST(test_begin_session_skips_reconciling_host_resinuse);
+    RUN_TEST(test_begin_session_skips_host_with_both_unavail_and_reconciling);
+    RUN_TEST(test_begin_session_reincludes_host_once_reconciling_clears);
     return UNITY_END();
 }