docs(kiro): reconcile spec metadata, promote 3 new specs, fix template path

- config-upgrade-snapshot, raft-adapter-conformance: correct stale
  phase metadata (both showed "tasks-generated" though every task was
  already complete) and record their validate-impl GO decisions with
  known-limitations notes (bug-1471/bug-1472-remainder, bug-1474
  angle-b) for future spec ownership.
- component-failover-adoption: catch up design/requirements/research
  docs to the Phase 6 fix (bug-1548/bug-1549, commit d479b707) that
  landed after this spec was already marked closed.
- Promote 3 roadmap backlog items to specs (2026-08-10 discovery):
  component-rejoin-resync (apiserver/scheduler rejoin resync),
  worker-rejoin-resync (worker job-fact resync, hard prereq for
  raft-apply-decoupling's facts half), scheduler-sim-diagnostics
  (open-ended scheduler REPL debug/troubleshoot capability).
- roadmap.md updated to reflect all of the above.
- kiro-steering/kiro-steering-custom SKILL.md: fix stale template path
  (templates/steering/ -> .kiro/settings/templates/steering/) plus
  minor doc corrections in the other kiro-spec-* skills.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Rongsong Shen 2026-08-16 11:02:06 +00:00
parent 3f58a3636b
commit ce0ee78b0f
Signed by: rshen
SSH key fingerprint: SHA256:8Vf2pZ3h6rJa7EhGK+WaAQ9Mta/apZvteW6g9U/WKKk
18 changed files with 467 additions and 114 deletions

View file

@ -19,7 +19,7 @@
### Does the library mechanism, once enabled, also cover runtime reconnect (Requirement 2) automatically?
- **Findings**: Yes for any component whose normal operation already calls `xcecli_*` functions periodically or on demand — every one of those calls is wrapped in `XCECLI_FAILOVER_CALL`/`XCECLI_FAILOVER_CALL_COUNT`, so a transport failure on any call transparently triggers rotate+reconnect+retry within that same call. Fire-and-forget calls (`xcecli_nop`, `xcecli_heartbeat`) use `XCECLI_FF_CALL` instead: a bounded single best-effort reconnect attempt, never the full retry budget — this is the library's own liveness-safety design for exactly the concern in Requirement 3.
- **Implications**: Scheduler's `xcecli_jobdispatch` calls (scheduler.c:1289, called from the `sched_allocator` thread) and apiserver's per-request backend calls (`gCtx.ctx` in clib.go) get transparent reconnect for free once `nmasters` is set correctly — no new polling/reconnect loop needs to be written for either component.
- **Implications**: Scheduler's `xcecli_jobdispatch` calls (scheduler.c:1259, called from `sched_publish_dispatch` on the main driver-loop thread — see "Thread-affinity" correction below, not `sched_allocator`) and apiserver's per-request backend calls (`gCtx.ctx` in clib.go) get transparent reconnect for free once `nmasters` is set correctly — no new polling/reconnect loop needs to be written for either component.
### Does anything need a NEW re-join after a transparent reconnect, beyond what the library does automatically?
- **Findings**: The library's rejoin hook exists precisely because reconnecting the transport does not re-establish application-level cluster membership — a new primary does not automatically know a worker/scheduler/apiserver rejoined unless it re-sends its join (role) message. `xcecli_reconnect_cb` "runs on the calling thread" and returns 0 to resume the interrupted op or <0 to treat the rejoin as failed (loop rotates/backs off again).
@ -27,13 +27,13 @@
### Thread-affinity of blocking retry calls (Requirement 3)
- **Findings**: Worker already isolates all controller-connection work (`controller_connector_start`, `xcecd_join_cluster`, and the periodic `checking_connection()` health-check) onto one dedicated thread (`start_controller_connector_thread` / `connector_to_controller`), separate from `hardware_monitor`/`system_monitor`/`process_manager` update calls in `xcecd_start()`. This isolation already satisfies Requirement 3 structurally for the worker — it is not something this feature needs to newly build, only preserve while removing the redundant candidate loop.
- Scheduler already runs `sched_job_picker` and `sched_allocator` as two separate `pthread_create`d threads (scheduler.c:511,516); `xcecli_jobdispatch` (the only call site found that could block on retry, scheduler.c:1289) executes from the allocator thread only. A blocked allocator during a controller outage is a benign, expected degradation (there is no work the allocator can do without a controller anyway) and does not stall the job picker or any liveness signal scheduler sends elsewhere — scheduler has no independent heartbeat-carrying thread to protect. No new threading is needed for the scheduler.
- **Corrected 2026-08-10**: `xcecli_jobdispatch` (`scheduler.c:1259`) does NOT execute from `sched_allocator`. It is called from `sched_publish_dispatch`, reached only via the decision-drain loop inside `xce_sched_driver_run_normal_mode_round` (`driver.c:765-786`), which runs on the single main-loop thread (`main.c`'s `while(1)`, `main.c:348-349`). `sched_allocator` (`scheduler.c:295-392`) only does `cbuf_get(&core.jbuf, ...)` then `cbuf_put(&core.decisions, ...)` — it never calls `xcecli_jobdispatch` itself. The scheduler's real pipeline is three stages joined by two `cbuf`s: `sched_job_picker` → `jbuf` (capacity 1, `core.c:161`, the scheduler-core determinism invariant) → `sched_allocator` → `decisions` (capacity `ndecisions`=5, `core.c:84`) → the main-loop thread → `xcecli_jobdispatch`. When that call blocks in retry/backoff, the main thread stops draining `decisions`; `sched_allocator` and `sched_job_picker` can still produce a few more jobs before their own `cbuf_put` calls block. So the two threads are **buffered for a bounded, occupancy-dependent grace window, not indefinitely isolated** — the earlier "benign, does not stall the job picker" claim was wrong; both threads do eventually stall for the remainder of an outage. **Corrected 2026-08-10 (second pass)**: the grace window's size depends on how full `decisions` was at the moment of the stall (guaranteed floor as low as 1-2 jobs, ceiling `ndecisions`+1=6) — an earlier version of this note claimed "~6-job" as if it were the guaranteed size; that was the best-case ceiling, not a floor. See design.md's SchedulerControllerClient section for the full derivation, the Req 3 AC1→AC2 scope-narrowing, and Requirement 7 (the actual load-bearing guarantee: no job lost regardless of how many the buffer absorbs).
- apiserver: unlike the C daemons, ALL request-handling goroutines that touch the backend share the same `gCtx.ctx`, serialized by the library's per-context `failover.lock`. Under a real outage this could queue every concurrent API request behind the full retry budget, which is a client-experience problem (Requirement 4), not a cluster-liveness problem. Needs a lightweight fail-fast guard in the Go layer — see Design Decisions.
- **Implications**: No new dedicated thread is required for worker or scheduler (existing isolation already suffices — flagged as a Validation Hook to confirm during implementation, not re-derived here). apiserver needs a new, small piece of state (not a thread) to short-circuit blocking calls when the backend is known-down.
- **Implications**: No new dedicated thread is required for worker (existing isolation already suffices). Scheduler needs no new thread either, but Req 3 AC1 needs its wording narrowed to match the bounded (not indefinite) buffering the existing `jbuf`/`decisions` cbufs actually provide — see design.md Critical Issue note. apiserver needs a new, small piece of state (not a thread) to short-circuit blocking calls when the backend is known-down.
### Configuration surface mismatch: `N_MAX_MASTERS` vs `MAX_HA_CANDIDATES`
- **Findings**: `src/include/client.h:28` defines `N_MAX_MASTERS (3)` — the hard cap on `xce_client_config_t.masters[]`. `src/include/config.h:78` defines `MAX_HA_CANDIDATES 4` — the cap on `xce_cluster_config_t.candidates[]` used by `xce_parse_peers`. These are different constants. `xce_test` (`test.c:360-367`) already handles the mismatch correctly: parse up to 4 candidates, then clamp to `N_MAX_MASTERS` (3) with a `WARN` if truncated.
- **Implications**: Any new shared conversion helper must replicate this exact clamp-and-warn behavior (not silently drop entries, not error out) so all components behave identically when 4 candidates are configured. Reconciling the two constants into one is out of scope (pre-existing, unrelated to this feature).
- **Implications**: Any new shared conversion helper must replicate this exact clamp-and-warn behavior (not silently drop entries, not error out) so all components behave identically when 4 candidates are configured. **Scope expanded 2026-08-10 (explicit user direction)**: reconciling the two constants is now IN scope — `N_MAX_MASTERS` is redefined to derive from `MAX_HA_CANDIDATES` (`config.h` becomes the single source of truth, Req 1 AC7), raising it 3→4. Source audit confirmed this is safe: no wire-protocol array assumes 3 (`masters[]` is never serialized), no header-include cycle (`config.h` depends only on `<stdint.h>`), and `xce_test`'s clamp already references `N_MAX_MASTERS` symbolically so it absorbs the raise automatically. Two latent bugs found by this audit that the raise would otherwise silently trip: `src/worker/src/main.c:414` and `src/scheduler/src/main.c:314` both hardcode a literal `> 3` check instead of referencing the macro — need fixing to `> N_MAX_MASTERS` alongside the raise. `src/lib/testsuite/test_libxce_retry.c`'s `fo_make_client_masters()` fixture also hardcodes exactly 3 literal host/port entries then copies `N_MAX_MASTERS` slots — needs a 4th literal added or it silently copies an uninitialized slot post-raise.
### TLS/CA-cert wiring gap (pre-existing, out of boundary)
- **Findings**: `xce_client_config_t` (the struct actually consumed by `xcecli_ctx_init`/`xcecli_start`, i.e. the real connection path) has no CA-cert field at all — only `nmasters`/`masters`/`master_ports`/`from`/`key`. The `ca_cert_path` that worker/scheduler's `--ca-cert` flag populates on `xce_cluster_config_t` is validated by `xce_cluster_config_validate()` but is only ever read by the orphaned `xce_connect_cluster()` stub, never by the real connect path.
@ -81,7 +81,7 @@
### Decision: rejoin callbacks must be lock-free flag-setters, never direct `xcecli_join` calls
- **Context**: A second `/kiro-validate-design` pass, cross-checking the library-invoked rejoin path against `client_failover.c` directly rather than only its header comments, found that `XCECLI_FAILOVER_CALL` holds `ac->failover.lock` — a plain, non-recursive `pthread_mutex_t` (`mylock_init` → `pthread_mutex_init(l, NULL)`) — for its entire retry loop, and `xcecli_invoke_rejoin()` (which calls the registered rejoin callback) runs synchronously *inside* that loop, lock held (`client_failover.c:397`). Every component's original design had its rejoin callback call `xcecli_join(...)` directly — itself `XCECLI_FAILOVER_CALL`-wrapped, so it would try to re-lock the same mutex on the same thread: a guaranteed self-deadlock on the very first real failover. This was never caught because `xcecli_set_reconnect_handler` has zero production callers and the existing unit test's callback never calls back into the client (it's a no-op probe).
- **Alternatives Considered**: (1) Have the callback call `xcecli_join` directly (original design — deadlocks). (2) Expose a new, lock-free/raw join primitive from libxce for rejoin callbacks to use (would require modifying `libxce-failover`, violating this spec's Allowed Dependencies boundary of consuming it read-only). (3) Callback only sets a lightweight flag and returns immediately (no I/O, no lock); the daemon's own calling code checks and clears the flag *after* the triggering call returns (lock free by then) and issues the real `xcecli_join` as a separate call.
- **Selected Approach**: (3), applied uniformly to worker (`checking_connection`), scheduler (the allocator, after `xcecli_jobdispatch`), and apiserver (each backend-call wrapper, after its `C.xcecli_*` call). apiserver's heartbeat-triggered path (Task 4.4) was already structured this way by construction (it acts after `xcecli_heartbeat` returns) and needed no change.
- **Selected Approach**: (3), applied uniformly to worker (`checking_connection`), scheduler (`sched_publish_dispatch`, the main-loop thread's actual `xcecli_jobdispatch` call site — not the allocator, corrected 2026-08-10), and apiserver (each backend-call wrapper, after its `C.xcecli_*` call). apiserver's heartbeat-triggered path (Task 4.4) was already structured this way by construction (it acts after `xcecli_heartbeat` returns) and needed no change.
- **Rationale**: Fixes the deadlock without touching `libxce` at all, staying inside this spec's stated boundary; the fix is a small, uniform pattern applied identically across all three components rather than a special case.
- **Trade-offs**: One extra check-and-clear step after each relevant call site, in exchange for correctness; negligible cost.
- **Follow-up**: Worth flagging upstream to `libxce-failover`'s owners that the rejoin-hook contract (`client.h`'s doc comment for `xcecli_reconnect_cb`) doesn't document this lock-held constraint — a future consumer without this spec's context could reintroduce the deadlock. Not fixed here (out of boundary), but the regression-guard unit test added in design.md's Testing Strategy exists specifically because of this gap in the upstream contract's documentation.
@ -108,6 +108,56 @@
- **Rationale**: Out of this feature's boundary — removing dead code is a separate hardening decision, not required to satisfy any requirement here, and touching it risks scope creep into an unrelated cleanup.
- **Follow-up**: Worth a future hardening backlog note (superseded/dead code) — not filed as part of this spec.
---
## Gap Re-Validation (2026-08-10)
Re-verified all code citations against the current `scheduler-framework` branch (post `scheduler-core` refactor, post worker CLI cleanup commits `9ffb1d5`–`0df1cf5`).
### Worker (xcecd)
**CLI changes orthogonal to spec.** The 6 worker commits removed `-c`/`-p`/`-m` flags, config-file loading, `xcecd_conf.c/h`, and `worker.yaml`. Added `-v` verbose logging, fixed `logdir` default and `-h` ordering. None touched `controller_connector.c`/`controller_connector.h` — the failover-relevant code is unchanged.
**Bug-1510 confirmed still present.** `cc_ctx->cluster_cfg = cluster_cfg` at current `main.c:537` still runs AFTER `xcecd_start()` at `main.c:531`. `controller_connector_start()` at `xcecd.c:515` performs the first join via `init_client_config()` (`controller_connector.c:129-160`, `nmasters=1` at line 144) before `cluster_cfg` is populated. Spec tasks 2.1–2.3 correctly target this.
**Line drift (minor):**
| spec cite | current |
|-----------|---------|
| `main.c:610` | `main.c:537` |
| `controller_connector.c:127-158` | `controller_connector.c:129-160` |
| `controller_connector.c:237-311` | `controller_connector.c:239-312` |
**Task 2.3 validation hook stale.** Task says to verify `hardware_monitor_update`/`system_monitor_update`/`process_manager_update` don't run on connector thread. These three are one-shot init calls at `xcecd.c:476-480` — never called again. The actual recurring liveness duties are `worker_process_update()` (main loop, line 577) and `resource_sync_thread` (dedicated thread, `resource_sync.c:293-330`). The architectural property (connector thread isolated) IS correct, but the named evidence is wrong. Recommend rewording to reference main loop/`resource_sync_thread`.
### Scheduler
**HA candidate loop untouched.** `scheduler.c:564-624` (was ~462-495 at spec-write). Loop logic unchanged; only line numbers shifted by `scheduler-core` refactor (doc comments, `core.c` extraction, `--sim` CLI additions in `main.c`).
**No-reconnect claim confirmed.** No `xcecli_set_reconnect_handler` call in any scheduler file. No reconnect path after `sched_init()`.
**Line drift (minor):**
| spec cite | current |
|-----------|---------|
| `scheduler.c:462-495` | `scheduler.c:564-624` |
| `scheduler.c:1289` (`xcecli_jobdispatch`) | `scheduler.c:1259` |
| `scheduler.c:511,516` (pthread_create) | `scheduler.c:626,632` |
| research.md L22: `scheduler.c:1289` | `scheduler.c:1259` |
| research.md L30: `scheduler.c:511,516` | `scheduler.c:626,632` |
**New structural note for design:** `main.c` now early-returns for `--sim` mode (line 296), inserted between peers-parsing block and `sched_init()`. Doesn't affect normal-mode path — peers block still runs before `sched_init()` for non-`--sim` — but design should note this guard exists.
### Apiserver
Not re-verified in depth (no flagged changes in this area). Light sanity check confirms `RegisterToBackend` still loops `config.Peers` with per-peer `tryConnect`.
### Requirements.md pre-existing issues
**Duplicate Background section.** Lines 10-22 and 24-34 are byte-identical — copy-paste artifact at spec-creation time. Should deduplicate.
### Summary
All architectural findings and target bugs in the spec remain substantively accurate. Only citation line numbers and task 2.3 wording need refreshing. No design decisions invalidated. Spec is ready for implementation.
## Risks & Mitigations
- Deleting worker's/scheduler's hand-rolled candidate loops could regress subtle existing behavior (e.g., worker's specific WARN logging on standby redirect) — mitigate by preserving equivalent log statements around the new single-call path, verified in code review.
- apiserver's circuit breaker could mask a fixed backend as still-degraded if the clearing signal (rejoin callback) doesn't fire in some edge case — mitigate with a bounded TTL on the degraded flag (re-probe after a short timeout) rather than relying solely on the rejoin callback.