feat(worker-rejoin-resync): apply reported outcomes idempotently (task 3.2)

Add resync_apply_report()/resync_apply_running_entry()/
resync_apply_finished_entry()/resync_apply_unrecognized_entry() to
resync.c: routes each entry in a worker's job-state report to the
correct existing idempotent path (still-running -> clear from outstanding
set, no fact; finished-success/failure -> record_job_finish_fact_at/
on_job_fail; unrecognized -> record_job_reconcile_pending_fact;
unqueried extra -> record_job_reconcile_assign_fact or no-op), discards
reports for superseded rounds (Req 6.1/6.3), and resolves the round once
its outstanding set empties.

Also fixes bug-1747 (Req 4.2): adds record_job_finish_fact_at(),
additive alongside the unmodified record_job_finish_fact, so a
finished-success reconciliation entry records the WORKER-reported
finish time instead of reconciliation-processing time -- proven via a
real WAL-write + persist_recover() restart-replay test. The FAIL-side
half of the same gap (WAL_JOB_FAIL carries no fail-time wire slot at
all, for any caller) is confirmed pre-existing and systemic, correctly
left out of this task's scope.

Three review rounds surfaced and fixed two real, distinct bugs in the
unqueried-extra-job routing guard (see tasks.md's Implementation Notes
for the full account and the general lesson about re-deriving a full
truth table after a partial boolean-guard fix): round 1 caught the
wrong axis (terminal-vs-non-terminal instead of assignment-status,
Req 4.8); round 2 caught that the first fix's short-circuit clobbered
an already-terminal job's DONE/EXIT back to RUN (Req 4.4), including
confirming the implementer's own test had certified the bug as correct;
round 3 independently re-derived the final three-way guard from scratch
and found no further defect. bug-1748 logged for the guard fix.

Full controller suite: 1239 tests, 0 failures.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y4M5MJM2zK6nU43da267Ku
This commit is contained in:
Rongsong Shen 2026-09-01 15:24:29 +00:00
parent 442fcf4544
commit eec2596c5d
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
8 changed files with 1600 additions and 7 deletions

View file

@ -61,7 +61,7 @@
- _Boundary: job.c additions_
- _Depends: 2.1_
- [ ] 3. Core: controller reconciliation-round bookkeeping
- [x] 3. Core: controller reconciliation-round bookkeeping
- [x] 3.1 (P) Track one reconciliation round per worker with a bounded timeout
- Given a worker host with one or more non-terminal jobs currently assigned to it, construct the exact list of those jobs' unique IDs as the round's query set
- Track a round identifier that distinguishes the current reconciliation attempt from any prior one for the same worker
@ -70,7 +70,7 @@
- _Requirements: 1.1, 1.2, 1.3, 2.1, 2.2, 5.3, 6.1_
- _Boundary: resync.c_
- [ ] 3.2 Apply each reported job outcome through the correct idempotent path
- [x] 3.2 Apply each reported job outcome through the correct idempotent path
- For each entry in a worker's job-state report: a still-running job is cleared from the round's outstanding set with no fact recorded; a finished-success or finished-failure job is applied through the existing finish/fail recording used for an ordinary report; an unrecognized queried job is applied through task 2.1's pending-and-release path; a reported job outside the original query is applied through task 2.2's assignment path
- Discard a report that arrives for a round already superseded by a newer one for the same worker, rather than applying it
- When the round's outstanding set becomes empty, resolve the round so the worker becomes eligible for dispatch again
@ -157,3 +157,4 @@
- **Task 2.2 found and fixed bug-1746**, a real pre-existing defect in `persist.c` (part of bug-1678's original fix, predates this feature): `capnp_encode_joballoc()` unconditionally dereferenced a NULL `alloc` via `encode_JobAlloc()` instead of the already-existing NULL-safe `encode_JobAlloc_ptr()` wrapper — never triggered before since `dispatch_a_job_` (the only prior caller) always passes non-NULL. This directly contradicted design.md's claim that the NULL-alloc case was "tested, backward-compatible" on both apply seams — true for *decode*, never actually true for *encode* until this task exercised it. Fixed with a minimal guard producing a true zero-length buffer (not a capnp-null-pointer encoding — verified against `apply_committed_entry`'s `alloc_len > 0` decode gate, which a non-zero-length "null" encoding would have wrongly satisfied). design.md corrected in 4 places. **Any future task calling `persist_submit_job_dispatch_fact` with a NULL alloc can now trust this path — it wasn't safe before task 2.2, it is now.**
- Task 2.2 (self-correction): a coordinator-applied one-line comment fix to `job.c` accidentally closed the enclosing `/* */` comment block early, leaving the comment's remaining lines as bare (uncompilable) code — caught immediately by re-running the build before marking the task complete, not left for review to find. Reinforces why fresh-evidence re-verification applies even to "trivial, obviously-safe" doc-only edits made outside the TDD subagent loop.
- Task 3.1: `resync.c`'s `resync_timeout_ms()` applies its own private default-if-unset fallback (15000ms) for `workerResyncTimeoutMs` rather than following `myconf.c`'s established `SET_DEFAULT_IF_UNSET`-style canonicalization convention used for the sibling `heartbeat_interval` field (`myconf.c:74`) — `myconf.c` is outside task 3.1's boundary, so this was correctly left as-is, but if a SECOND consumer of `workerResyncTimeoutMs` (or `workerResyncWindowMs`) is ever added, it would have to either duplicate this private fallback or the two could silently disagree. Worth canonicalizing in `myconf.c` if/when that happens — not needed for this feature's own single consumer. delaymgr timer cancellation: `delay.c`/`delay.h` expose no dequeue/cancel primitive for an already-armed one-shot task; `resync.c` handles this correctly via a by-value round-token check inside the fired callback (verified safe against both the logical "resolves the wrong round" bug and use-after-free, since the callback's closure never holds a pointer into the freed `resync_state_t`) — any future delaymgr one-shot user in this codebase facing the same "need to cancel" problem should use this same by-value-token-check pattern rather than assuming a cancel API exists.
- **Task 3.2 needed 3 review rounds — a genuine cautionary tale about a boolean short-circuit hiding a second, adjacent bug.** Round 1 caught the unqueried-extra-"running" branch gating on terminal-vs-non-terminal instead of Req 4.8's real trigger (assignment status), silently leaving a job stuck PEND when its `WAL_JOB_DISPATCH` fact never reached durable commit before a failover — the feature's own core motivating scenario. The FIRST fix (`!terminal && assigned_to_w's_host`) solved that but introduced a NEW, distinct Req 4.4 violation via the same `&&`'s short-circuit: a TERMINAL job (DONE/EXIT) falls into the same `else`/assign branch and gets clobbered back to RUN — caught by round 2, and confirmed the implementer's own new test had been asserting the bug as correct behavior (a green suite is not proof of correctness when the test itself encodes the wrong expectation). The SECOND fix (a genuine three-way `if`/`else if`/`else`: terminal→noop, non-terminal-and-already-assigned-anywhere→noop, non-terminal-and-unassigned→assign) passed round 3's from-scratch re-derivation plus an explicit hunt for job==NULL reachability. **Lesson for future tasks in this codebase**: when a guard combines two orthogonal conditions with `&&`/`||` and only ONE of the two is corrected after a review finding, re-derive the FULL truth table before re-submitting — a targeted patch to the specific counterexample a reviewer names can leave an adjacent, differently-shaped counterexample in the same boolean untouched. bug-1747 (finish-time gap, split fixed) and bug-1748 (this guard defect) both logged.