docs(worker-rejoin-resync): add requirements, design, and research docs
Requirements approved; design generated (not yet approved -- pending /kiro-validate-design fixes). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y4M5MJM2zK6nU43da267Ku
This commit is contained in:
parent
76511d356c
commit
f2ff45f18e
4 changed files with 669 additions and 6 deletions
113
.kiro/specs/worker-rejoin-resync/research.md
Normal file
113
.kiro/specs/worker-rejoin-resync/research.md
Normal file
|
|
@ -0,0 +1,113 @@
|
|||
# Research & Design Decisions: worker-rejoin-resync
|
||||
|
||||
## Summary
|
||||
- **Feature**: `worker-rejoin-resync`
|
||||
- **Discovery Scope**: Extension (existing system) — integration-focused, cross-cutting across controller, worker, and scheduler around an already-implemented join/rejoin path and an already-implemented commit-then-apply seam.
|
||||
- **Key Findings**:
|
||||
- `rejoin_worker`/`apply_join_state` (worker.c, from `worker-join-refactor`) is the exact moment a host becomes "connected" again — the natural trigger point for Requirement 1, and a new `worker_t` status bit here gates new dispatch "for free" through the *existing* `dispatch_a_job_` connected-state check with zero changes to that function.
|
||||
- The job table's idempotent commit-then-apply split (`record_job_*_fact` / `apply_job_*_transition`, with `job_state_is_terminal`/`job_state_is_backward` guards) is the exact dedup mechanism the requirements' Boundary Context says this feature must reuse, not duplicate. Three of four reconciliation outcomes (finished-success, finished-failure, still-running) already have a fully-idempotent home; only "unrecognized → return to pending" and "unqueried job → assign" are genuinely new fact shapes.
|
||||
- The controller→scheduler notification path (Requirement 7) is a hook → notify → wire → dequeue pipeline (`XCE_CONTROLLER_HOOK_JOB` → `sched.c` hook callbacks → `notify()` → scheduler's `handler.c` inbound switch → `sched_job_state_change()`), and it is **strictly separate** from the scheduler's own outbound decision-publish path (`scheduler.c`/`driver.c`), which a standing text-scan test (`test_no_worker_resync_symbols_in_scheduler_publish_path`) guards against ever learning about "resync" concepts. This fixes the correct integration seam: `handler.c`'s inbound switch only.
|
||||
- The worker has **no existing retention** of finished-job state — `cleanup_worker_list()` reaps and deletes an exited job's record (memory + sqlite row) on the very next cleanup pass, with no `finish_time` field anywhere in the worker's job structs today. New short-lived retention is required, not a rewiring of an existing cache.
|
||||
- `xce_reply_get()`/`xce_push_data_req()` (`{pos, count, n_total}` chunking, used today for joblist/workerlist/reslist replies) is the established bounded/paginated multi-message idiom in this codebase and is the direct template for Requirement 8, not the heavier WAL catch-up/replication protocol and not `XCECLI_FF_CALL` (whose whole point is "loss is fine" — the opposite of what this feature needs).
|
||||
- `config-design.md`'s rule set classifies the new "recently finished" window (Requirement 3.7) the same way `heartbeatInterval`/`inactiveHeartbeatCount` were classified: passes the independence rule (no cross-node correctness dependency) but is kept in shared config anyway via the documented operational-uniformity override, for the same "predictable cluster-wide behavior" reason.
|
||||
|
||||
## Research Log
|
||||
|
||||
### Controller-side join/rejoin flow and job state machine
|
||||
- **Context**: Where does reconciliation trigger, and how does it reuse the existing idempotent fact/apply path?
|
||||
- **Sources Consulted**: `src/controller/worker.c` (`rejoin_worker:403`, `apply_join_state:321`, `on_worker_join:623`), `src/controller/join.c` (`handle_worker_join:48`), `src/controller/job.c` (`record_job_finish_fact:2626`, `record_job_fail_fact:2781`, `record_job_postdone_fact:2921`, `apply_job_update_transition:2808`, `dispatch_a_job_:1278`), `src/controller/persist.c` (`apply_committed_entry:4333`), `src/controller/commit.c` (`commit_submit:485`), `src/include/xce/types.h` (`worker_t:284`, status bits `170-184`), `.kiro/specs/worker-join-refactor/design.md`.
|
||||
- **Findings**:
|
||||
- `handle_worker_join` durably commits the join (`persist_commit_barrier`) before `on_worker_join` runs — a second, later plug-in point after the join is durable.
|
||||
- `worker_t.host.status` is a bitmask; `dispatch_a_job_` (job.c:1511-1512) already refuses dispatch when `w->ep == NULL || w->host.status != 0`. A new `XCE_WORKER_RECONCILING`-style bit therefore gates Requirement 5.1 with no change to `dispatch_a_job_` itself.
|
||||
- `record_job_finish_fact`/`apply_job_finish_transition` already sets `finish_time` at receipt time (bug-1689), matching Requirement 3.5/4.2 exactly — the finished-job reconciliation case is a straight reuse.
|
||||
- `JOB_STAT_PEND` is otherwise only ever set at job creation — there is no existing "return a RUN job to PEND" transition. Since RUN→PEND is non-terminal→non-terminal, the existing `job_state_is_backward` guard does not block it; a thin new fact wrapper routed through the same `WAL_JOB_UPDATE`/`apply_job_update_transition` seam is the correct reuse (not a new state-machine core).
|
||||
- No existing "assign a job to a worker the controller didn't previously have a record for" fact exists; the closest analog is `dispatch_a_job_`'s own assignment logic (job.c:1686-1734).
|
||||
- The pending-facts queue is not a separate structure — it *is* the commit engine (`commit_submit`, `COMMIT_MODE_RETAIN`) plus `apply_committed_entry`'s guards. Reconciliation must submit through this exact seam, never a parallel one (this is also stated explicitly in the requirements' own Boundary Context).
|
||||
- **Implications**: Reconciliation needs exactly two new fact-recording functions in `job.c` (reconcile-to-pending, reconcile-assign) plus straight reuse of the three existing ones (finish, fail, no-op-for-running).
|
||||
|
||||
### Worker-side job bookkeeping and wire protocol
|
||||
- **Context**: What can the worker actually report, and how should the query/report exchange ride the wire?
|
||||
- **Sources Consulted**: `src/worker/include/xcecd_worker_utils.h`, `src/worker/src/xcecd_worker_utils.c` (`cleanup_worker_list:385-404`), `src/worker/include/xcecd_utils.h`, `src/lib/request.c` (`xce_join_req:1398-1429`, `xce_reply_get:1627-1784`, `xce_push_data_req:1491-1610`), `src/schema/controller.capnp` (`JoinData:155-161`, `HostAttrs:126-135`), `src/lib/client_internal.h` (`XCECLI_FF_CALL:290-302`), `src/lib/client.c`.
|
||||
- **Findings**:
|
||||
- `xcecd_worker_t` (PID-keyed) and `xcecd_job_t` carry no timestamp field at all; `cleanup_worker_list()` deletes both the in-memory entry and its sqlite row essentially immediately after exit — there is no retention window today.
|
||||
- `JoinData`'s capnp union has exactly two arms (`nodata`, `attrs`) with no job-state content, confirming the brief's own claim. Cramming a paginated exchange inline into `JoinData` would force it through `xce_join()`'s single-shot synchronous `cbuf_timedget` call shape, which cannot paginate — a new follow-up opcode/message pair after the join reply is the right shape, not a `JoinData` union extension.
|
||||
- `xce_reply_get`/`xce_push_data_req`'s `{pos, count, n_total}` chunking (already used for joblist/workerlist/reslist) is the established, directly-reusable bounded-exchange template for Requirement 8.
|
||||
- `XCECLI_FF_CALL`'s own header comment explicitly names "worker-rejoin-resync" as the intended fix for the loss it tolerates — confirming this feature's problem statement directly in code, not just in the bug report.
|
||||
- **Implications**: New capnp opcode pair (controller→worker query, worker→controller report), both using the existing pagination shape; new worker-side finished-job retention structure with a timestamp field, bounded by the Requirement 3.7 window.
|
||||
|
||||
### Scheduler notification path and scope-fence test
|
||||
- **Context**: Requirement 7 requires reconciliation-driven job-state changes to reach the scheduler through "the same path" as an ordinary change — what is that path, concretely, and are there any standing tests that constrain where new code may live?
|
||||
- **Sources Consulted**: `src/controller/job.c` (hook call sites `HOOK_DETAIL_JOB_EXIT:2641`, `HOOK_DETAIL_JOB_FAIL:2745`), `src/controller/sched.c` (`sched_job_hook_fun:547`, `sched_job_finish_fun:392`, `sched_job_fail_fun:438`, `sched_job_update_fun:468`), `src/controller/dataex.c` (`notify:399`), `src/controller/job.c` (`notify_dispatch:1971`), `src/scheduler/src/handler.c` (`sched_handle_notify:525`, `sched_job_state_change:407`), `src/scheduler/testsuite/test_scheduler_decision_redelivery.c:424-456`.
|
||||
- **Findings**:
|
||||
- Job-state-change code fires `XCE_CONTROLLER_HOOK_JOB` (not `HOOK_WORKER` — that category is for worker join/dead events); `sched.c`'s registered hook callback dispatches by `HOOK_DETAIL_*` to one of three functions, each of which calls the shared `notify()` with a distinct `NotifyData_data_sched*` event.
|
||||
- `notify_dispatch()`/`NotifyData_data_jobDispatchFail` is an existing, direct precedent for "tell the scheduler this job is back to pending" — its handler in `handler.c` already calls `sched_job_state_change(sched, jobid, JOB_STAT_PEND)`.
|
||||
- `test_no_worker_resync_symbols_in_scheduler_publish_path` is a static text-scan (`strstr` for `"resync"`) over `scheduler.c` and `driver.c` — the scheduler's own **outbound** decision-publish direction. It is a deliberate scope-fence: that direction must never learn about worker-resync concepts. It does not cover `handler.c` (the **inbound** Controller→Scheduler direction), which is unambiguously the correct, and only permitted, integration seam.
|
||||
- The async-I/O steering doc's thread-role contract applies identically to the scheduler; a bounded reconciliation wait must follow the vtask + admission-reservation pattern (`commit.c`'s `COMMIT_WAIT_RESERVED`, `wait_cap`/`wait_admit_locked`) rather than any raw blocking wait on iomgr/delaymgr, with the timeout itself fired by a delaymgr trigger.
|
||||
- **Implications**: Reconciliation's finish/fail cases need zero new scheduler-facing code (they already drive `schedExitJob`/`schedFailJob`). The reconcile-to-pending case should reuse the existing `jobDispatchFail`-style `sched_job_state_change(..., JOB_STAT_PEND)` call shape (either by literally reusing that notify event or adding a same-shaped one) — never by adding anything to `scheduler.c`/`driver.c`.
|
||||
|
||||
## Architecture Pattern Evaluation
|
||||
|
||||
| Option | Description | Strengths | Risks / Limitations | Notes |
|
||||
|--------|-------------|-----------|---------------------|-------|
|
||||
| Inline extension of `JoinData` union | Add job-state content as a third `JoinData` union arm | Minimal new opcode surface | Forces the exchange through `xce_join()`'s single-shot synchronous reply — cannot paginate (Requirement 8) | Rejected |
|
||||
| New opcode pair, ad hoc payload shape | New `OPCode_jobStateQuery`/`OPCode_jobStateReport`, custom framing | Full control over shape | Reinvents pagination the codebase already solved | Rejected |
|
||||
| New opcode pair reusing `{pos,count,n_total}` pagination | New opcode pair, payload shape mirrors `JobData`/`WorkerData`/`ResourceVals` | Directly reuses an established, tested idiom (`xce_reply_get`/`xce_push_data_req`); minimal new protocol concepts | None significant | **Selected** |
|
||||
| Reuse WAL catch-up/replication streaming protocol | Model the exchange on `replEntry`/`walCatchupEntry` | Already resumable/ack-per-entry | Heavier machinery designed for log replication, wrong semantic fit for a bounded one-time query/report | Rejected |
|
||||
|
||||
## Design Decisions
|
||||
|
||||
### Decision: New opcode pair for the job-state query/report exchange, following the `xce_reply_get`/`xce_push_data_req` pagination template
|
||||
- **Context**: Requirements 2, 3, and 8 need a controller-initiated, bounded/paginated, bidirectional exchange that today's join message cannot carry.
|
||||
- **Alternatives Considered**: inline `JoinData` extension; ad hoc new framing; WAL-catch-up-style streaming (see table above).
|
||||
- **Selected Approach**: Two new opcodes — `OPCode_jobStateQuery` (controller→worker) and `OPCode_jobStateReport` (worker→controller) — both paginated with the existing `{pos, count, n_total}` shape, issued as a follow-up exchange after the join reply (not inline in `JoinData`).
|
||||
- **Rationale**: Reuses a proven, already-tested idiom instead of inventing a new one; keeps the join message itself unchanged (no regression risk to `worker-join-refactor`'s existing behavior, per its own Boundary Commitments).
|
||||
- **Trade-offs**: One more opcode pair to add to `ha_dispatch_gate`'s role tables and `myreq_handler`'s switch, versus a cleaner, template-following protocol.
|
||||
- **Follow-up**: Confirm at implementation time whether `OPCode_jobStateReport` needs an explicit redirect-reply path if ever received by a non-primary (per `api-standards.md`'s "Adding a New Opcode" step 6 / bug-1700 precedent) — expected answer is no, since workers only ever connect to the current primary, but this must be verified, not assumed.
|
||||
|
||||
### Decision: Reconciliation-outstanding state lives on `worker_t` as a status bit + a small tracking record, not a new global registry
|
||||
- **Context**: Requirement 5 needs to gate new dispatch per-worker while reconciliation is outstanding, and Requirement 6 needs "discard on disconnect, re-trigger on next rejoin" semantics.
|
||||
- **Alternatives Considered**: a separate global map keyed by worker; per-worker state (selected).
|
||||
- **Selected Approach**: A new `XCE_WORKER_RECONCILING` status bit (reusing the existing `w->host.status != 0` dispatch check "for free") plus a small struct on `worker_t` holding the queried job-ID set, a round/generation token (for Requirement 6.3's late-report discard), and the timeout deadline.
|
||||
- **Rationale**: `worker_t` is already the natural per-host session object (confirmed by `worker-join-refactor`'s own design); a new global structure would duplicate lifecycle management the existing object already handles (creation, teardown, locking).
|
||||
- **Trade-offs**: None significant — this is the minimal-surface option.
|
||||
- **Follow-up**: None.
|
||||
|
||||
### Decision: Reconcile-to-pending and reconcile-assign are new, narrow fact-recording functions in `job.c`, not a new state-machine core
|
||||
- **Context**: Requirement 4.6 (unrecognized → pending) and Requirement 4.8 (unqueried job → assign) have no existing idempotent fact path.
|
||||
- **Alternatives Considered**: a new, separate reconciliation-specific mutation path; thin wrappers over the existing `WAL_JOB_UPDATE`/dispatch-assignment seams (selected).
|
||||
- **Selected Approach**: `record_job_reconcile_pending_fact()` submits through the existing `WAL_JOB_UPDATE`/`apply_job_update_transition` seam with target state `PEND` (same shape as any other job-state-update fact, so it drives the existing `HOOK_DETAIL_JOB_UPDATE` → scheduler-notify path for free). `record_job_reconcile_assign_fact()` mirrors `dispatch_a_job_`'s own assignment logic for host/state but does **not** attempt to reconstruct the job's original resource allocation (see Risks & Mitigations).
|
||||
- **Rationale**: The requirements' Boundary Context and Constraints explicitly forbid a second, competing idempotency mechanism; both new functions are additive call sites into the existing guarded seam, not new guards.
|
||||
- **Trade-offs**: The reconcile-assign case inherits a real, pre-existing limitation (below) rather than solving it.
|
||||
- **Follow-up**: None — the limitation is out of this spec's boundary by design.
|
||||
|
||||
### Decision: The "recently finished" reporting window (Requirement 3.7) is shared cluster config, via the operational-uniformity override
|
||||
- **Context**: Requirement 3.7 asks for a bounded, cluster-configurable window; `config-design.md` governs where any such field belongs.
|
||||
- **Alternatives Considered**: per-node local config (passes the independence rule cleanly — no cross-node correctness dependency); shared config via override (selected).
|
||||
- **Selected Approach**: Add the window (and, for the same reason, the Requirement 5.3 reconciliation timeout) as new fields on the existing `ClusterConfig` capnp blob, alongside `heartbeatInterval`/`inactiveHeartbeatCount`.
|
||||
- **Rationale**: `config-design.md`'s own worked example classifies the structurally identical `heartbeatInterval`/`inactiveHeartbeatCount` fields (controller→worker liveness, not HA/Raft election heartbeat) as shared-via-override for exactly this reason: no correctness invariant demands cluster-wide agreement, but operators want uniform, predictable behavior rather than per-node drift. This feature's window is the same shape of parameter in the same domain (worker liveness/reconnection).
|
||||
- **Trade-offs**: A config push is required to change it (heavier than a per-node flag), accepted for the same reason the precedent field accepted it.
|
||||
- **Follow-up**: None.
|
||||
|
||||
### Decision: Config push and the job-state query are sent in a fixed order (config first) with no new ack — ordering is guaranteed by the single connection, not invented
|
||||
- **Context**: `on_worker_join` (worker.c:623) must trigger both the existing config push and the new job-state query for a rejoining worker. Does the worker need to fully apply config before it can correctly answer the query, and if so, how is that ordering enforced?
|
||||
- **Alternatives Considered**: (1) have the controller wait for an explicit worker-sent config-applied ack before sending the query (initially adopted, then retracted); (2) rely on the connection's existing ordering guarantees (selected).
|
||||
- **Selected Approach**: `on_worker_join` sends config push (unchanged `worker_push_config` call), then immediately sends the new job-state query — both fire-and-forget, no new ack/reply added.
|
||||
- **Rationale**: Initially assumed the worker might dispatch incoming controller requests concurrently across its `thread_pool_t`, which would make arrival order insufficient to guarantee handling order — this assumption was wrong. Reading `controller_connector.c:1563`'s `myreq_handler` directly confirmed it is a plain synchronous `switch`, invoked one message at a time off the worker's single long-lived client connection (`xcecli_ctx`); the worker's `thread_pool_t` is scoped to job-forking/job-control work only, not request dispatch. On one connection, TCP ordering + this synchronous single-message dispatch together guarantee config push is *handled to completion* before the job-state-query handler even begins — not just sent first. No config content is actually needed to answer the query correctly either way (job IDs are globally unique, independent of host config), so this was a belt-and-suspenders concern, not a load-bearing correctness dependency — but the ordering guarantee turned out to be free, so it's stated explicitly rather than left implicit.
|
||||
- **Trade-offs**: None — this is strictly simpler than the retracted ack-based alternative, with no loss of guarantee.
|
||||
- **Follow-up**: If the worker's request-dispatch model is ever changed to fan out across a thread pool (unlike today), this ordering guarantee must be re-derived — it is not inherent to the wire protocol, only to the current single-threaded-dispatch implementation.
|
||||
|
||||
## Risks & Mitigations
|
||||
- **Risk**: A reconciliation-assigned job (Requirement 4.8) has no recoverable original resource allocation (`job->allocs`) — the controller never held a dispatch record for it, so there is nothing to read the allocation back from. — **Mitigation**: Document as a deliberate, bounded scope limitation (see `design.md`'s Data Models persistence-completeness note): the job's *state* and *host* are corrected; its resource *reservation* accounting starts from zero/unknown, converging via the existing `resource-accounting.md` "charged = max(reserved, actual)" policy on the job's next rusage tick — identical to how any legitimately-unreserved job is already handled today, not a new class of gap.
|
||||
- **Risk**: A new WAL fact type (reconcile-assign) must be decoded/adopted identically by both apply seams (`apply_committed_entry` primary-boot-replay and the standby catch-up apply path) or it reproduces the exact bug-1678 field-completeness failure mode. — **Mitigation**: `design.md` calls this out explicitly as a persistence-completeness checklist item, per `design-principles.md`'s Persistence Review requirement.
|
||||
- **Risk**: A blocking wait for the worker's report, implemented carelessly, could stall a liveness-critical thread (iomgr/delaymgr). — **Mitigation**: Reuse `commit.c`'s `COMMIT_WAIT_RESERVED` admission-reservation pattern for any bounded wait; drive the timeout via a delaymgr-scheduled trigger, never a blocking sleep.
|
||||
- **Risk**: The requirements' Boundary Context explicitly disclaims coupling the Requirement 3.7 window to `commit-batch-window`'s (separately parked) commit-urgency tuning. — **Mitigation**: `design.md` does not reference or depend on `commit-batch-window` in any component contract.
|
||||
|
||||
## References
|
||||
- `.kiro/specs/worker-rejoin-resync/requirements.md` — approved-for-design requirements (8 requirement groups, 30 acceptance criteria)
|
||||
- `.kiro/specs/worker-rejoin-resync/brief.md` — original discovery brief with file:line evidence
|
||||
- `.kiro/specs/worker-join-refactor/design.md` — `rejoin_worker`/`apply_join_state` contract this feature extends
|
||||
- `.kiro/steering/config-design.md` — shared-vs-local config classification rule, applied to Requirement 3.7
|
||||
- `.kiro/steering/resource-accounting.md` — reserved/used accounting model the reconcile-assign limitation converges into
|
||||
- `.kiro/steering/persistence.md` — `APPLY_DIVERGENT`/bug-1678 per-field completeness precedent
|
||||
- `.kiro/steering/api-standards.md` — "Adding a New Opcode" checklist (bug-1700 redirect-reply precedent)
|
||||
- `.kiro/steering/ha-async-io.md` — thread-role contract for the bounded reconciliation wait
|
||||
Loading…
Add table
Add a link
Reference in a new issue