feat(worker-rejoin-resync): task 6.1 - trigger reconciliation on rejoin
Arm a reconciliation round in apply_join_state() the moment a worker host becomes connected (first join, reconnect, or failover-reconnect - all routed through the same shared call site, Req 1.1-1.3), then send the query from on_worker_join() strictly after the join reply and existing config push (Req 2.1-2.2), verified via a structural test that reads real wire order off the endpoint queue. Also closes Req 3.7's config-delivery seam end-to-end: worker_resync_ window_ms now rides the existing Conf4Worker push (clustername precedent, no new opcode) and the worker applies it to its retention window on receipt. Fixes bug-1752: myreq_handler had no OPCode_push dispatch case at all, so every config push - including the pre-existing clustername field - was silently discarded. Confirmed harmless until now (clustername was never read anywhere in src/worker/); closed narrowly for worker_resync_window_ms only. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y4M5MJM2zK6nU43da267Ku
This commit is contained in:
parent
48c6cfd58a
commit
a3d12d37d9
8 changed files with 1045 additions and 3 deletions
|
|
@ -113,7 +113,7 @@
|
|||
- _Boundary: scheduler candidate filter_
|
||||
|
||||
- [ ] 6. Integration: cross-daemon wiring
|
||||
- [ ] 6.1 Trigger reconciliation on worker rejoin and send the query
|
||||
- [x] 6.1 Trigger reconciliation on worker rejoin and send the query
|
||||
- At the moment a worker host becomes connected again (first join, network reconnect, or reconnect after controller failover, treated identically), check whether the controller currently holds non-terminal jobs for that host and, if so, arm a reconciliation round for it
|
||||
- Send the round's query to the worker as a follow-up exchange after the join reply and the existing configuration push have both already been sent, not as an extension of the join call itself
|
||||
- Also close a gap task 4.1 discovered and correctly left out of its own boundary: the worker daemon has no delivery path at all today for any `ClusterConfig` field (the existing `worker_push_config`/`on_worker_join` push carries a generic `confvars` key/value list plus a small number of directly-pulled fields — `clustername` is the existing precedent, `src/controller/worker.c` around the `collect_confvars`/`conf4worker` construction). Add `workerResyncWindowMs` to that same push, mirroring the `clustername` field precedent exactly (no new opcode needed — this reuses the existing `OPCode_push`/`WorkerConfig` wire struct this task already touches), and have the worker's existing config-receive handling call `xcecd_finished_job_retention_set_window_ms()` (task 4.1) with the received value so the retention window task 4.1 built is actually operator-tunable end-to-end, not stuck on its documented placeholder default
|
||||
|
|
@ -166,3 +166,4 @@
|
|||
- **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.
|
||||
- **Task 6.1 found and fixed bug-1752**: the worker daemon's `myreq_handler` had NO dispatch case for `OPCode_push` at all before this task — every prior config push (including the pre-existing `clustername` field, in production since before this spec started) was silently discarded via `default: break;`. Confirmed genuine (diffed `myreq_handler` pre/post, ruled out an alternate consumer — `xcectx.c`'s `vtask_entry` only special-cases the unrelated generic `pushmsg` union arm, not `configpushmsg`) and confirmed `clustername` has never been read anywhere in `src/worker/`, so this was harmless dead infrastructure until `workerResyncWindowMs` became its first real consumer. Fixed narrowly — `handle_pushconfig()` consumes only `worker_resync_window_ms`, leaving `clustername`/`confvars` still unconsumed and explicitly out of this task's boundary — rather than expanding scope to wire up every existing-but-never-consumed push field. Ordering (join reply → config push → conditional arm+query) and the live-configured-value plumbing were both verified via genuine structural tests (real packets popped off the wire, two distinct configured values round-tripped), not weaker behavioral proxies.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue