docs(roadmap): close out four low-risk backlog cleanup items
- xce_test shell command support: was already implemented (xce-test-e2e-
primitives task 2.1, closed 2026-06-19) -- stale "not started" entry
corrected.
- scheduler-core tasks 2.1-2.6 _Verified: backfill: was already done
(commit d7b49690, 2026-08-08) -- corrects a status line I myself wrote
incorrectly earlier today by trusting the original 2026-08-07 finding's
wording without re-checking the file.
- job_ctrl direct-mutate-path investigation: completed. Real risk
confirmed (pre-commit state pushed live to scheduler + apiserver/clients
with no rollback on commit failure), scope wider than originally framed
(on_job_stop/on_job_resume/on_job_ckpt share the same pattern, fed by
ordinary traffic not just rare admin commands), plus a separate
read-lock-during-mutation bug found along the way. Not promoted to a
spec yet -- scope/framing corrected first per this session's decision.
- OP_upgradeConfig standby redirect verification: investigated. The
item's own premise doesn't match production (a standby never actually
returns a redirect for this opcode -- a documented, deliberate feature
gap, not a missing test); coverage confirmed already complete on both
the client mock side and a live 3-node regression test.
This commit is contained in:
parent
0519d24d64
commit
a16d32e3fe
2 changed files with 4 additions and 4 deletions
|
|
@ -428,4 +428,4 @@
|
|||
1. **Req 5 AC 3's tlog logging-integration bullet is built but never activated.** `core_clock.c`'s `xce_core_register_tlog_format()`/`xce_core_set_active_for_logging()` (task 4.5) are unit-tested but have zero call sites in `main.c`/`sim.c`/`driver.c` — task 4.5's own header comment explicitly deferred this ("this task does not wire it into mylog_init()/main.c; that remains a later task's job"), but no later task in this spec ever picked it up. Root-caused where it *should* wire in: NOT `main.c`'s normal-mode path — normal mode seeds `virtual_clock_base=0, real_clock_base=0` (`core.c:139-140`), so `xce_core_now()` there returns raw `CLOCK_MONOTONIC` nanoseconds-since-boot, not epoch time; activating it for normal-mode logging would make log timestamps *worse*, not better. The correct site is `sim.c`'s `sched_sim_driver_start()`, right after `sim_set_virtual_clock()` (line 394) — `virtual_clock_base` is seeded to `snapshot_ts` there (a real epoch-ns value from the `.replay` file), so `xce_core_now()` becomes a meaningful simulated timestamp. Left unfixed here rather than patched under validation-gate time pressure because activation needs a matching deactivation: `g_active_core_for_logging` is a single process-wide pointer, `sched_sim_driver_run()`'s own header comment already documents "no sim-driver stop/fini function exists" (deliberate, accepted gap) with multiple early-return paths, and several test binaries (e.g. `test_repl_equivalence.c`) construct more than one stack-allocated `xce_sched_sim_driver_t` per process — activating without a paired deactivation risks a real ASAN stack-use-after-scope hazard the very next time an unrelated log call fires in that process. Needs a small dedicated follow-up task: wire activation into `sched_sim_driver_start()` AND deactivation into every `sched_sim_driver_run()` return path (or add the fini function its own comment says doesn't exist yet).
|
||||
2. **`plugins/fcfs.c` vs `plugins/builtin_fcfs.c` — functionally correct, undocumented.** design.md (component description) says `fcfs.c` is "rewritten (Phase 1) to `xce_job_picker_iface_t`, drops `job_elect_fn`." The actual implementation instead added a new parallel file, `plugins/builtin_fcfs.c`, and left `fcfs.c` untouched still registering the legacy `job_elect_fn` (`fcfs.c:353`) — confirmed deliberate, not an oversight: `scheduler.c:466`'s own comment says the new picker is registered "alongside the untouched legacy `xce_sched_fcfs`", and task 1.2's compatibility-wrapper requirement needs exactly this kind of legacy `job_elect_fn`-only plugin to exist as its live exercise target. `builtin_fcfs` is confirmed the registered default (`schedobj.c:364`). Functionally sound; the only gap is that tasks 2.1-2.6 carry no `_Verified:` annotations (unlike every other task group) and design.md's literal text was never updated to record this file-layout decision — a documentation/traceability gap, not a defect. Worth a follow-up: add `_Verified:` notes to 2.1-2.6 and correct design.md's component description.
|
||||
- Both items are cosmetic/traceability, not correctness or determinism defects — the soak, equivalence, and integration evidence for this validation pass (15-round sequential + 15-round chaotic HA soak, both zero-loss/zero-divergence; full rebuild 41/41 meson + 15/15 manual scheduler + all Go/xceadmin tests green; independent requirements-coverage/design-alignment/cross-task-integration subagent passes) is unaffected by either.
|
||||
- **Status update (2026-08-19)**: Follow-up 1 (tlog activation) is now **FIXED** — see design.md's Open Questions/Risks #6. Follow-up 2 is **partially closed**: design.md's component description was corrected the same day (2026-08-07, see design.md's own Open Questions/Risks #7), but tasks 2.1-2.6 still carry no `_Verified:` annotations — that backfill remains open.
|
||||
- **Status update (2026-08-19)**: Follow-up 1 (tlog activation) is now **FIXED** — see design.md's Open Questions/Risks #6. Follow-up 2 is **FULLY CLOSED**: design.md's component description was corrected the same day (2026-08-07, see design.md's own Open Questions/Risks #7), and tasks 2.1-2.6 were backfilled with `_Verified:` lines the next day (commit `d7b49690`, 2026-08-08) — an earlier revision of this status-update line incorrectly claimed the backfill was still open without checking; corrected after fresh verification confirmed both the `_Verified:` lines and the underlying `builtin_fcfs.c` implementation they describe are present and accurate (60/60 picker/fcfs-related tests green on a fresh rebuild, 2026-08-19).
|
||||
|
|
|
|||
|
|
@ -79,17 +79,17 @@ The protocol **drive** is NOT fully event-driven. Root cause: `peer_ctrl_msg_han
|
|||
|
||||
Smaller items that likely don't warrant a full spec; pick up opportunistically or bundle into adjacent spec work.
|
||||
|
||||
- [ ] **investigate: job_ctrl direct-mutate path (admin suspend/resume) — migrate to commit-then-apply?** — **recorded 2026-07-13, surfaced during job-array-single-wal-record's `persist_job_hook_fun` investigation.** `raft-apply-decoupling` deliberately scoped `job_ctrl`'s `mark_job_update`/`mark_job_fail` (admin SIGCONT/SIGSTOP) OUT-OF-BOUNDARY (task 4.1/6.1 Implementation Notes) — confirmed the ONLY remaining live call site that still mutates state directly, THEN fires the `AFTER_EVENT` hook, with `persist_job_hook_fun` doing the real WAL append + async commit from inside the hook (not through `apply_committed_entry()`). This is the same mutate-before-commit shape as the bug-795/828/851/1060 divergence family raft-apply-decoupling was built to eliminate for job submit + worker facts, narrowly scoped to admin suspend/resume. Not yet known to have caused a live incident — rare, synchronous, admin-only traffic. **Investigate before committing to a fix**: (a) actual blast radius if a commit fails/times out post-mutation (does it ever get observed/notified before commit — violates criterion 2.4-style provisional-state-export concerns?); (b) live-apply vs replay-apply asymmetry for this one WAL entry type — does replay actually diverge from live in any reachable scenario, or is it benign in practice; (c) cost of migrating vs. accepted-residual status quo. Promote to a spec (likely a small `raft-apply-decoupling` follow-up, not standalone) only if the investigation finds real risk, not proactively.
|
||||
- [ ] **job_ctrl direct-mutate path — migrate to commit-then-apply** — **recorded 2026-07-13, investigated 2026-08-19 (finding: real risk, wider scope than originally framed).** `raft-apply-decoupling` deliberately scoped `job_ctrl`'s `mark_job_update`/`mark_job_fail` (admin SIGCONT/SIGSTOP) OUT-OF-BOUNDARY (task 4.1/6.1 Implementation Notes). The 2026-08-19 investigation found the original framing wrong in two ways: (1) **NOT the only remaining call site** — `on_job_stop`/`on_job_resume`/`on_job_ckpt` (`job.c:2067-2222`) share the identical unconverted mutate-then-notify-then-WAL-append pattern, fed by ordinary worker/scheduler traffic, not "rare, admin-only" as originally described; (2) **blast radius (a) is confirmed real, not theoretical** — the pre-commit mutation is synchronously pushed live to BOTH the scheduler (`sched_job_update_fun`) AND every connected apiserver/client (`apiserver_job_update_fun`) before the commit barrier resolves; on barrier timeout the admin gets a failure reply but `job->job.state` is never rolled back and no retraction is ever sent — matching the bug-795/828/851/1060 shape. Live-apply vs replay-apply (b) is narrower than feared for `job_ctrl` specifically (the `allocs != NULL` gate mutually excludes it from the converted deferred-apply fact types), but the investigation surfaced a SEPARATE bug underpinning that safety argument: `job_ctrl` (`job.c:999`) holds only a read lock while mutating job state — every other mutator in this file takes a write lock — so two concurrent admin commands on the same job aren't mutually excluded, undermining the "same lock, no staleness window" claim the replay-safety argument depends on (already flagged by a `raft-apply-decoupling` task 2.1 reviewer, never fixed). **Recommendation**: promote to a small follow-up spec covering all four call sites together (bounded, well-precedented fix — same shape as the fact-receipt conversion `raft-apply-decoupling` already built for job submit); the lock-type bug is independently fixable and low-risk regardless of when/whether the larger migration happens. Not yet promoted — user decision 2026-08-19 to correct scope/framing here first, promote later.
|
||||
- [x] **WAL-trim cursor-bound guard (bug-1147; surfaced 2026-06-25 by raft-drive-async design review)** — **IMPLEMENTED** as part of `raft-drive-async` task 3.2 (`raft-drive-async` is IMPLEMENTED, validate-impl GO 2026-07-18; see above). `trim_wal_segments` (`persist.c` ~2597-2644) now computes `floor = repl_trim_lsn(&p->repl, active_seg_lsn, p->config.snap_interval_s)` — folding in active-standby `acked_lsn` and disconnected-slot `last_disconnected_lsn` — and only unlinks segments below that floor; the code comment at `persist.c:2634` explicitly labels this the "(bug-1147 guard)". Far-behind/empty join (cursor=0) is additionally covered now that `entity-bearing snapshots` (strategy B) has landed as `ha-entity-snapshots` (see above), with a live decision path (`ha.c:4413`, "< oldest_wal_lsn → requesting snapshot transfer"). Related: bug-795, bug-828, bug-1060.
|
||||
- [ ] OP_heartbeat peers cross-validation — include a hash of the local `--peers` string in heartbeat requests; log ERROR on mismatch between candidates. Detects misconfigured nodes at election time instead of silently risking dual-primary split-brain. _(FUTURE_WORK.md §3.)_
|
||||
- [ ] **peer-authentication-hardening** — **NEW 2026-07-06 from admin-config-tool discovery** (`/kiro-discovery`). XCE currently operates under a **trusted-LAN model** — any CA-signed SSL peer can connect to the controller and perform ANY operation. Three gaps identified: **(a) Admin→Controller**: `validate_host()` is a stub returning success for all peers (roadmap § below). `OP_upgradeConfig` has no real authorization gate today. **Same gap confirmed 2026-08-02 for `OPCode_schedAdmin`** (`scheduler-core` design, `cluster.c`'s `case OPCode_upgradeConfig` was checked as the closest precedent and has only a primary/standby routing gate, no RBAC/auth check) — `scheduler-core` ships `OPCode_schedAdmin` at parity with that existing gap (project owner decision: not a `scheduler-core` blocker), so this item now covers both opcodes; when `validate_host()` is implemented, wire it to both `OP_upgradeConfig` and `OPCode_schedAdmin` in the same pass. **(b) Worker/Scheduler→Controller**: workers and schedulers connect via `controller_connector.c` using the same TLS context as controller peers, with no role differentiation — a compromised worker could send `OP_upgradeConfig` or read full config via `OP_replConfig`. **(c) Cross-component trust**: no distinction between controller peers, admin tools, workers, and schedulers — they share the same X.509 identity pool. **(d) xceadmin login (future)**: xceadmin currently uses zero-config TLS (same as xce_test). Production deployments likely need an admin login requirement — token-based session (`xceadmin login → token → config push`) or password-per-command. X.509 CN alone proves identity but not intent; a login step provides audit trail, session expiry, and operator accountability. **Needs design**: role-based X.509 CN validation (controller/admin/worker/scheduler roles), per-role OP code allowlists (now including `OPCode_schedAdmin`), admin login/session model, audit logging for admin operations. Separate from the existing `validate_host()` hardening (below) which is the minimal gate for `OP_upgradeConfig`/`OPCode_schedAdmin` specifically. Discuss + design before implementing; likely promotes to a spec via `/kiro-discovery`.
|
||||
- [ ] validate_host() real authorization — currently a stub returning success for all peers (`join.c:13-16`, `return 1;` unconditionally). **`CONF_ENABLE_UPGRADE_CONFIG` is already `1` in the standard build** (`config_repl.h:27-28`, no build-system override found) — the precondition this item was originally framed to guard against has already been crossed; `OP_upgradeConfig` (and `OPCode_schedAdmin`, added 2026-08-02, `scheduler-core`) are live and unauthenticated TODAY, not a future risk to prevent. Minimum fix: verify peer X.509 CN against a configured allowlist (`--admin-cn` flag or `admin_cn_list` config), reject non-allowlisted `OP_upgradeConfig` and `OPCode_schedAdmin` with clear error, WARN-log rejected attempts with CN+IP. Future: IP allowlist, role-based access, admin-specific CA. _(FUTURE_WORK.md §6; security-critical.)_ **Part of the broader `peer-authentication-hardening` item above.**
|
||||
- [ ] OP_replConfig access control hardening — currently serves the full config blob to any CA-signed SSL peer (trusted-LAN model). Consider: verify requester is a known `--peers` controller, rate-limit per peer, `minimum_role` check (controllers only). _(FUTURE_WORK.md §7.)_ **Part of the broader `peer-authentication-hardening` item above.**
|
||||
- [ ] lost-and-found drain command — `xcecli lost-and-found drain`: reassign or cancel sidelined jobs, reset `lost_and_found_job_count`. Automates operator cleanup after config upgrade. _(FUTURE_WORK.md §4.)_
|
||||
- [ ] OP_upgradeConfig standby redirect verification — test that a standby returns the primary's address to an admin tool and the tool retries against the primary. Note: redirect semantics change under ha-consistency-invariants Req 1 (no transitional-state redirect); verify against the new behavior. _(FUTURE_WORK.md §5.)_
|
||||
- [x] ~~OP_upgradeConfig standby redirect verification~~ — **RESOLVED 2026-08-19 (finding: item's own premise doesn't match production behavior, but coverage is already complete).** A standby does NOT return the primary's address for `OPCode_upgradeConfig` — `ha_dispatch_gate` rejects it before the handler's own dead defensive check ever runs, replying flat `status=-XCE_ERR_STANDBY, data_which=nodata` with no redirect fields; `fill_redirect_reply()` (`join.c:27`) is never called for this opcode. This is a documented, deliberate feature gap (see `admin-config-tool` design.md's "Revalidation Triggers"), not a missing test. Coverage is already complete from both directions: the client-side redirect-following mechanism is unit-tested against mocks (`src/xceadmin/tests/test_discovery.py`, `test_config_push_mock.py::test_push_config_follows_one_redirect_to_primary`), and the real server-side current behavior (no redirect, flat rejection) is live-verified against a real 3-node cluster and regression-locked (`src/xceadmin/tests/test_e2e_live_ha_push.py::test_live_config_push_against_standby_returns_standby_rejection_not_redirect`). The remaining real work — wiring `fill_redirect_reply()` for `OPCode_upgradeConfig` — is a feature task already tracked under `admin-config-tool`'s Revalidation Triggers, not this item.
|
||||
- [x] steering sync — `ha-async-io.md` synchronized to the three-state model + own-term gate (transitional-state row/role removed; heartbeat re-arm PRIMARY-only; PRIMARY no-quorum fact allowlist noted). Done as part of ha-single-leader-role task 6.1.
|
||||
- [ ] xce_test WAL/snapshot inspection tooling — extend the offline inspector family in `xce_test` (sibling of the shipped `waldump`, `src/controller/testsuite/waldump.{c,h}` + `cmd_waldump`). Two remaining commands: **(a) `snapdump <snap_dir|snap_file>`** — decode the `XCESNAP\0` header (8-byte LSN; `persist.c:1266` `load_snapshot`) and report whether the snapshot carries job/queue entities or is an LSN-only stub. This directly settles Open Fork #3 of the failover job-loss investigation (`.kiro/specs/ha-commit-durability/INVESTIGATION-failover-job-loss.md`): if snapshots are entity-less, any empty-rejoin/WAL-prune below committed is unrecoverable BY DESIGN. **(b) `walcompare <dirA> <dirB>`** — diff two nodes' WALs by `lsn → crc/jobid` to flag divergence/prefix-gaps between members instantly. Reuse the `waldump.c` reader/decoder; same zero-alloc capnp jobid decode + `c_capnproto_dep`/`zlib_dep` wiring already added to `xce_test_deps`. Nonzero exit on entity-less snapshot / divergence so both double as E2E assertions. _(recorded 2026-06-20; `waldump` already done.)_
|
||||
- [ ] xce_test shell command support — add `!<command>` prefix to `xce_test` test scripts to execute arbitrary shell commands inline during test runs. Enables test scripts to drive environment setup/teardown (e.g., kill a node, write a file, run a helper) without calling out to a separate harness layer. Scope: parser change in the xce_test runner to detect `!`-prefixed lines, fork/exec the remainder as a shell command, fail the test on non-zero exit, capture stdout/stderr to the test log.
|
||||
- [x] ~~xce_test shell command support~~ — **IMPLEMENTED** (task 2.1, spec `xce-test-e2e-primitives`, closed 2026-06-19; stale entry corrected 2026-08-19). `preprocess_and_run()` (`src/controller/testsuite/test.c:950`) detects a leading `!` and runs the remainder via `system()`; the shell command's exit status becomes `g_last_status` (the `$?` auto-variable) and a non-zero status sets `g_session_failed`, aborting the script the same way any other failing command does — `system()`'s child inherits the parent's stdout/stderr fds, so output lands in whatever the invoking `xce_test` session's own output is already redirected to. Works in every REPL context (no `leave_ctx()`/disconnect), including mid-session inside `worker`/`client` contexts — used this session to fix bug-1579 (resource-value-verification's nested controller-check pattern). No parser/harness-layer change remains to do.
|
||||
- [x] scheduler framework enhancement — **PROMOTED to spec 2026-07-06, then DECOMPOSED 2026-07-27** into `scheduler-core` (**IMPLEMENTATION-COMPLETE**), `scheduler-allocation`, and `scheduler-preemption` — see **In Flight** above for current status. Scope notes from the original brief still apply: commit barrier wiring deferred (separate hardening item); GPU/affinity allocation deferred (pluggable via new allocator interface); new job selection policies deferred (framework enables them, ships separately). No longer tracked here.
|
||||
- [→] **perf-tuning-extraction** — **PROMOTED to spec 2026-07-20** (`clusterconfig-schema-split`, `.kiro/specs/clusterconfig-schema-split/`). Originally recorded 2026-07-06 from admin-config-tool discovery as a documentation/audit item; the audit (2026-07-20) found the fields are load-bearing enough to need real code changes, not just a doc pass — `candidates`/`vcpus`/hash-tuning fields (`jobmgr`/`workermgr`/`qmgr`/`resmgr`/`xstrings`) are already dead weight in the `ClusterConfig` capnp blob (superseded by `--peers`/`--vcpus` CLI flags or never read into runtime state — see `src/docs/cluster-config-split-analysis.md`). **`port` is NOT dead** — a second review pass (2026-07-20) found `cluster_open_listener` (`cluster.c:1216-1258`) falls back to the blob's `port` field, and single-node mode (no `--peers` flag at all, `main.c:293-294`) depends on that fallback entirely; the spec now requires a replacement port-sourcing mechanism before the field can be removed (`clusterconfig-schema-split` Requirement 1.7), not a bare deletion. `heartbeatInterval`/`inactiveHeartbeatCount` (controller→worker liveness, NOT the HA/Raft election heartbeat) were investigated and deliberately kept IN `ClusterConfig` for operational uniformity despite passing the independence rule — see `.kiro/steering/config-design.md`. `fetchJobInterval`/`netioTimeout` left unresolved, flagged out-of-scope in the new spec for a future pass.
|
||||
- [ ] **Raft/HA heartbeat interval+timeout as CLI parameters** — **NEW 2026-07-20 from clusterconfig-schema-split discovery.** The controller-to-controller HA/Raft election heartbeat (`ha->config.heartbeat_interval_ms`/`heartbeat_timeout_ms` — the one where a cross-node mismatch risks spurious failover) is 100% hardcoded today (`HA_DEFAULT_HEARTBEAT_INTERVAL_MS`/`HA_DEFAULT_HEARTBEAT_TIMEOUT_MS`, `cluster.c:1131`) — there is no `ClusterConfig` field for it, and per user decision (2026-07-20) there should never be one: these values must be available to a controller BEFORE it has validated/received cluster configuration (a controller needs its own election-timing parameters to even participate in the election that delivers config in the first place — a bootstrap/chicken-egg constraint, not an independence/frequency judgment call). Target: `--heartbeat-interval-ms`/`--heartbeat-timeout-ms` CLI flags following the exact `--vcpus` pattern (`OPT_*` + `struct arguments` + `myconf_t` field, `0` sentinel = use hardcoded default). Kept hardcoded for now; this is a future item, not blocking `clusterconfig-schema-split`.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue