myxce/.kiro/specs/clusterconfig-schema-split/research.md

# Research & Design Decisions

## Summary
- **Feature**: `clusterconfig-schema-split`
- **Discovery Scope**: Extension (existing system) — light discovery per `design-discovery-light.md`
- **Key Findings**:
  - `candidates`, `vcpus`, and the five perf-tuning fields are genuinely dead in `ClusterConfig` at runtime (verified across every `.c` file that touches `cluster_config_t`).
  - `port` is NOT dead — its removal (Requirement 1.1) needs a replacement mechanism (Requirement 1.7), which this design resolves as a new `--port` CLI flag, matching the existing `--vcpus` pattern exactly.
  - No tooling in this repo (`deploy-ha.sh`, test scripts) currently starts a controller with zero peer-related flags in a way that would be affected either way — but the codebase's own `main.c:294` message ("No --peers flag; running as single-node primary") confirms the zero-`--peers` invocation is an intentional, first-class supported mode, not incidental. The design preserves it exactly rather than forcing a CLI-contract change.

## Research Log

### Requirement 1.7's port-sourcing replacement mechanism
- **Context**: `requirements.md` Requirement 1.7 deliberately left the replacement mechanism as an open design question after the requirements-phase review found `cluster_open_listener` (`cluster.c:1216-1258`) depends on `cluster_config->port` as the sole port source for single-node mode (no `--peers` flag passed at all, `main.c:293-294`).
- **Sources Consulted**: `src/controller/main.c` (CLI parsing, `--vcpus` precedent at lines 65-66/114-118/252, single-node branch at 293-295), `src/controller/cluster.c:1216-1258` (port resolution), `src/include/myconf.h` (existing CLI-derived field conventions), `src/controller/ha.h` (hardcoded-default naming convention, `HA_DEFAULT_*`), prior memory of this codebase (`--peers` with a single self-referencing candidate is a confirmed, already-used alternative single-node invocation — but no repo tooling actually relies on the zero-flag form being removed).
- **Findings**:
  - No standalone `--port` CLI flag exists today — confirmed via grep (zero hits for `OPT_PORT`/`"port"` in `main.c`).
  - `--vcpus` establishes the exact pattern to follow: `OPT_VCPUS` argp option → `struct arguments.vcpus` → validated → copied into `myconf.vcpus` (0 = unset sentinel, documented inline in `myconf.h:37`) → consumed at the point of use with a hardcoded fallback (`cluster.c:1320`, `myapp.c:129`: `(myconf->vcpus > 0) ? myconf->vcpus : 8`).
  - `deploy-ha.sh` (the only repo-internal controller launcher found) always passes `--peers`; no repo script exercises the true zero-flag single-node path. This means changing that path's invocation contract would carry no *repo-internal* tooling risk — but it's still a documented, INFO-logged, intentionally-supported mode (`main.c:294`), not dead code, so removing it outright is a real behavior change to a supported feature, not cleanup.
  - `xce_parse_peers`'s side effects (CA cert path population, identity string, `main.c:278-292`) only run inside the `if (arguments.peers_str)` branch — the zero-flag path currently skips them. Preserving the zero-flag path (rather than forcing a synthetic self-referencing `--peers` entry) avoids introducing any change to that unrelated cert/identity setup.
- **Implications**: Adding `--port` as a new CLI flag (rather than mandating `--peers` for all invocations) requires slightly more new code but changes zero existing invocation behavior and directly reuses a pattern already reviewed and shipped in this exact file. Given this spec's own history of an overconfident "zero blast radius" claim being wrong once already, the lower-risk option is preferred even at the cost of a few more lines.

## Architecture Pattern Evaluation

| Option | Description | Strengths | Risks / Limitations | Notes |
|--------|-------------|-----------|---------------------|-------|
| A: `--port` CLI flag (chosen) | New `OPT_PORT` flag, `myconf.port` field, hardcoded fallback constant when unset and not found in `--peers` | Zero invocation-UX change; exact precedent (`--vcpus`) to copy; isolated, low-risk diff | Slightly more new code than B | Selected — see Decision below |
| B: Mandate `--peers` everywhere (self-referencing entry for single-node) | Remove the zero-flag path; single-node must pass `--peers ctrl-1=host:port` | Single source of truth for port; reuses an already-proven 1-peer self-quorum path; less code | Removes a documented, intentionally-supported invocation mode; unverifiable whether anything outside this repo (docs, operator muscle memory) depends on the zero-flag form | Rejected — the removed mode is intentional/documented, not incidental, and this spec's mandate is schema cleanup, not an invocation-contract change |

## Design Decisions

### Decision: Add `--port` CLI flag as the replacement for `ClusterConfig.port`
- **Context**: Requirement 1.7 requires a non-`ClusterConfig` port-sourcing mechanism before `port` can be removed from the schema, without breaking single-node controller startup.
- **Alternatives Considered**: (A) new `--port` flag, hardcoded fallback default; (B) mandate `--peers` for all invocations including single-node.
- **Selected Approach**: (A). New `OPT_PORT` argp option → `struct arguments.port` (uint32_t, default 0) → copied to a new `myconf_t.port` field → `cluster_open_listener()`'s fallback branch reads `myconf->port` instead of `cluster_config->port`; if still 0 (flag not passed), falls back to a new hardcoded constant `XCE_CONTROLLER_DEFAULT_PORT = 9990` (matching today's exact `SET_DEFAULT_IF_UNSET(c->port, 0, 9990)` value byte-for-byte, so the default behavior is unchanged for anyone relying on it).
- **Rationale**: Exact structural precedent already exists and was already reviewed/shipped for `--vcpus`; zero changes to any existing invocation (scripted or manual); avoids introducing a new port-precedence edge case between two different flags (`--port` only matters when `--peers` doesn't supply a match, so precedence is unambiguous: `--peers` match > `--port` > hardcoded default).
- **Trade-offs**: A few more lines of new code than mandating `--peers` everywhere; two flags (`--peers`, `--port`) both nominally relate to "port" now, though only one (`--peers`) is meaningful once any peer is configured.
- **Follow-up**: None — this is fully specified for the tasks phase.

## Risks & Mitigations
- **Risk**: A future contributor reintroduces a `ClusterConfig`-sourced default for `port`/`candidates`/hash-tuning "for convenience" during an unrelated change, recreating the original problem. **Mitigation**: `.kiro/steering/config-design.md` now documents the general rule; this spec's removal is the concrete enforcement.
- **Risk**: The checked-in `ha/data/cluster.bin` might carry non-default values for the removed fields, silently masking a real backward-compat issue if the live-soak test (Requirement 5.3) doesn't actually inspect them first. **Mitigation**: Requirement 5.3 (and Testing Strategy below) mandates inspecting or synthesizing non-default legacy field values before relying on that test as proof.
- **Risk**: `test_commit_barrier_rollout.c:1359` / `test_config_repl.c:983` are the only *known* struct-literal hits, but a broader grep pattern might surface more during implementation. **Mitigation**: Requirement 5.1's catch-all clause; the build itself will fail loudly on any missed site (C struct field removal is a compile error, not a silent runtime gap).

## References
- `src/docs/cluster-config-split-analysis.md` — prior team analysis (2026-06-08), primary design input for the dead-field classification.
- `.kiro/steering/config-design.md` — general shared-vs-local config classification rule this spec enforces.
- `.kiro/specs/admin-config-tool/requirements.md` — downstream consumer whose "per-instance field preservation" risk this spec closes structurally.

## Design Review (`/kiro-validate-design`, 2026-07-20)

Initial GO/NO-GO: NO-GO, 3 issues found on a fresh critical pass of the first design draft. All fixed in `design.md` directly:

1. **Flow diagram dropped the zero-value fallback trigger.** Original diagram branched on "name found" (boolean); actual pre-spec code branches on `my_port == 0`, which also covers "found but that candidate's port field is malformed/zero" (`main.c:289-291` already WARNs on this input). As drafted, a matched-but-zero-port entry would have bound to literal port 0. Fixed: diagram and Service Interface contract now check the port value, not just match presence.
2. **Removing the `-EINVAL` path conflated two different scenarios.** Original design treated "`--peers` never passed" (true single-node) and "`--peers` passed but local name absent" (likely operator typo) identically — both silently falling to `--port`/hardcoded-default. User decision (interactive, during review): keep today's hard error for the second case; only the first gets the new fallback. Rationale: the project's fail-loud convention (`src/controller/CLAUDE.md` process-lifecycle invariant) — a silent wrong-default here would surface later as a confusing "peer unreachable" symptom instead of a clear startup error now. Fixed: System Flows, Service Interface, and Error Handling sections all updated to reflect the split.
3. **Requirement 6 missing from Boundary Commitments.** Present in File Structure Plan/Components/Traceability but not in "This Spec Owns" — mechanical inconsistency. Fixed: one bullet added.

Re-affirmed GO after fixes — no further review pass required (fixes were direct, not requiring a requirements-phase return).

## Design Revision (`/kiro-impl` kickoff, 2026-07-20)

**Reversed the Architecture Pattern Evaluation decision** (Option A → Option B) before implementation started, per direct user instruction at the start of `/kiro-impl`.

- **Original choice**: Option A, a new `--port` CLI flag + `myconf_t.port` field, mirroring `--vcpus`.
- **Revised choice**: Option B (previously rejected) — no new CLI flag. `cluster_open_listener`'s fallback becomes two-tier: `--peers` candidate match (unchanged) → hardcoded `XCE_CONTROLLER_DEFAULT_PORT` (9990) when `--peers` was never passed at all. Operators who want a non-default single-node port pass a single self-referencing `--peers <name>=<host>:<port>` entry (matching `-n`/`--myname`) — an **already-existing, already-supported** invocation mode (confirmed working: single-node primary via `--peers` omission OR a 1-peer `--peers` self-quorum entry are both live paths today, verified by grepping `cluster_open_listener`'s existing candidate-match loop against `c->myconf->name`).
- **Why the reversal holds up**: the original rejection of Option B was "removes a documented, intentionally-supported invocation mode" — but Option B as now scoped does NOT remove the zero-`--peers` mode; it only asks operators who need a *non-default port* in that mode to use the already-supported 1-peer `--peers` form instead of a brand-new flag. Zero new surface area, zero new plumbing (`main.c` argp wiring, `myconf_t.port` field, `myconf.vcpus`-style wiring all become unnecessary), and it reuses code that already exists and is already exercised.
- **Requirements impact**: none — Requirement 1.7 already listed "requiring single-node mode to pass a self-referencing `--peers` entry" as an explicit example alternative and left mechanism selection to the design phase.
- **Design impact**: `design.md`'s Overview, Goals, Boundary Commitments, Architecture diagram, File Structure Plan, Requirements Traceability, Components and Interfaces, Service Interface contract, and System Flows are all updated in place to reflect the two-tier fallback with no CLI Component for Requirement 1.7 (the CLI Component now exists only for the optional Requirement 6 tunables, reusing the pre-existing `--vcpus` pattern directly rather than a `--port` precedent).
- **Tasks impact**: `tasks.md`'s former Task 2 (`--port` CLI flag) is removed; all subsequent major tasks renumbered down by one.
- **Note**: `xce_parse_peers` also accepts a bare `host:port` candidate (no `name=` prefix), matched via `hostname` when no name is given — but `design.md`/`tasks.md` document the explicit `name=host:port` form as the canonical example for this replacement mechanism (clearer about intent than relying on hostname matching implicitly).

## Design Revision 2: Ordinal-Gap Discovery (`/kiro-impl` task 1.1, 2026-07-20)

The task 1.1 implementer, before touching the schema, found and empirically proved a contradiction between Requirement 3.1 (retained fields keep their literal original `@N` field number) and Requirement 4.2 (direct removal, no field-number reservation): Cap'n Proto requires struct ordinals to be a gapless `0..N-1` sequence — verified independently (`capnp compile` on a schema with a gap fails: `error: Skipped ordinal @1. Ordinals must be sequential with no holes.`). Deleting 8 fields from the middle of `ClusterConfig`'s 21 while freezing the 13 survivors' literal ordinals is not something any compiling schema can satisfy — you either renumber the survivors (breaks their wire byte-offsets for anything reading an old blob) or leave inert renamed placeholders at the old ordinals (which is field-number reservation, exactly what 4.2 forbids).

Neither `requirements.md` nor `design.md` had anticipated this; both were written assuming Cap'n Proto allows field deletion independent of ordinal freezing, which is not the case.

- **Decision (user, interactive)**: renumber the 13 survivors to a contiguous `0..12` range — literal direct removal, matching Requirement 4.2's exact text and the project's "still pre-release, no wire-compat obligation" posture. Requirement 3.1 amended to drop "field number" from its invariance clause (type/semantics only); Requirement 3.2 and 4.1 amended to state plainly that a `cluster.bin` built before this spec is not expected to decode correctly (not just the removed fields — every retained field after the first removed one shifts byte-offset).
- **Consequence discovered next**: the checked-in `ha/data/cluster.bin` (`deploy-ha.sh:33-36` copies it verbatim as every cluster's seed config) is itself a pre-spec artifact. Post-renumbering, its retained fields (`resources`, `queues`, `hostConfigs`, `hproviders`, `limits`, etc.) would silently decode to wrong bytes — breaking `/xce-deploy-ha` and `/xce-test-ha` for every future spec that depends on that fixture, not just this one's verification task. No existing tool rebuilds this blob (`xceadmin config build` isn't implemented yet — separate, unstarted spec).
- **Decision (user, interactive)**: add a task to regenerate `ha/data/cluster.bin` — decode its current retained-field values under the pre-spec schema, re-encode under the new schema with a small throwaway tool (written and discarded, not shipped), overwrite the fixture in place. New Requirement 5.3; old 5.3 (3-node soak) → 5.4, old 5.4 (single-node) → 5.5. `tasks.md` gained a new task 5.1 (regenerate fixture) ahead of the renumbered 3-node-soak task.
- **Rationale**: given the renumbering choice, the fixture WILL be silently wrong if left untouched — this is not optional cleanup, it's a repo-wide landmine for every HA spec's live verification, self-inflicted by this spec's own schema edit. Regenerating it once, in-scope, is far cheaper than every future spec independently rediscovering "why is deploy-ha producing garbage config."
- **Scope boundary held**: the throwaway encoding tool is explicitly NOT a permanent addition — building a real `cluster.bin` authoring tool is `admin-config-tool`'s job (`xceadmin config build`), not this spec's. This task only needs to reproduce ONE known blob's values under the new layout, not build general tooling.

## Design Revision 3: Orphaned HashZArrayConfig/ResmgrConfig Structs (`/kiro-impl` task 2.1, 2026-07-20)

The task 1.1 implementer deliberately left `HashZArrayConfig` and `ResmgrConfig` (the capnp struct types used by the now-removed `jobmgr`/`workermgr`/`qmgr`/`xstrings`/`resmgr` fields) in `controller.capnp`, correctly noting no requirement or task text asked for their removal — they compile fine as unreferenced-but-valid struct definitions on their own. Task 1.2 then removed their corresponding hand-written C typedefs (`hash_zarray_config_t`, `resmgr_config_t`) from `clusterconfig.h`, since nothing in `cluster_config_t` referenced them anymore. The task 2.1 implementer's partial syntax check surfaced the resulting break: capnp's C codegen still emits `HashZArrayConfig`/`ResmgrConfig` encode/decode prototypes referencing `hash_zarray_config_t`/`resmgr_config_t` via their `$C.mapname(...)` bindings — types that no longer exist anywhere in the tree.

- **Root cause**: this is the same class of gap Requirement 1.6 already closed for `MasterCandidate` (used only by the removed `candidates` field) — just missed for the tunables side during the original requirements pass, since `MasterCandidate` was called out explicitly by name while the tunables' supporting struct types were not.
- **Decision**: extend the same treatment — remove `HashZArrayConfig`/`ResmgrConfig` from `controller.capnp`. Verified via grep (same method as the original `MasterCandidate` verification) that neither struct is referenced anywhere else in the schema. New Requirement 2.5 added (mirrors 1.6's phrasing); design.md's File Structure Plan and Requirements Traceability updated; `tasks.md` gained task 1.3 (Schema Component, same boundary as 1.1) to close this before task 4's full build.
- **Why this wasn't caught in the requirements-phase discovery pass**: the original `cluster-config-split-analysis.md` and this spec's requirements-phase grep swept for the FIELDS being removed, not for supporting/nested struct TYPES those fields' capnp types depended on. A useful pattern for future field-removal specs: after identifying fields to remove, also check whether any of their types are struct types used nowhere else in the schema.