docs(admin-config-tool): clear stale OP_upgradeConfig dispatch blocker (bug-1468 already fixed)

admin-config-tool's spec.json blocked_on and tasks.md both still described
the OP_upgradeConfig dispatch-gate defect as an open blocker. It was fixed
2026-07-25 (bug-1468, commit 959b0cda) by config-upgrade-snapshot task 8.6,
which independently hit the same wall building a live OP_upgradeConfig
sender -- the fix's own comment in cluster.c already cross-references this
spec. The controller-side path has since been exercised live and
successfully many times via config-upgrade-snapshot's --config-upgrade
soak. All 23 admin-config-tool tasks are already [x]; this was pure
tracking-metadata debt, not a reopened implementation gap.

Also appends a cross-spec gap analysis (admin-config-tool x
config-upgrade-snapshot x clusterconfig-schema-split) to all three specs'
research.md, confirming clean schema integration between the latter two
and flagging the one remaining real gap: a new commit-timeout issue in
config-upgrade-snapshot task 8.6's force-elect step, surfaced now that
bug-1477 is fixed, not yet root-caused.
This commit is contained in:
Rongsong Shen 2026-07-28 14:13:06 +00:00
parent f1e54f3ba0
commit aecd8766f4
5 changed files with 225 additions and 3 deletions

View file

@ -152,7 +152,7 @@
- **2.4**: `errors.py`'s exception subclasses (task 1.2) all take `message` as an explicit first constructor argument in addition to their structured fields (e.g. `ConnectionTimeoutError(message, host, port)`, `ReplyDecodeError(message, reason)`) — design.md's abbreviated `Service Interface` type stubs (`class ConnectionTimeoutError(XceAdminError): host: str; port: int`) read as if only the structured fields were needed; always check the real constructor in `errors.py`, not the design doc's shorthand. `xcecommon.wire` is deliberately opcode-agnostic — `send_request`/`send_frame`/`recv_frame` move raw `bytes` only and never import a capnp type; callers own `.to_bytes()`/`.from_bytes()`. Reused `ha/data/xce.crt`/`xce.key` (the repo's existing dev cert pair, same one `deploy-ha.sh` already copies for controller TLS) as the test fixture for both the TLS test server and the client `cert_dir` — no synthetic cert generation needed.
- **3.1**: Requirement 1.7's "unrecognized-but-schema-valid field" warning is real and fires on essentially every real build using `src/configs/resources.yaml` — `resources.yaml`'s `type` key is *required* by `resources.schema.json` (`required: [name, type]`) but has no `ClusterConfig` destination (per `xcecommon.mapping`'s documented decision not to guess the `valtype`/`ordtype`/`flags` integer encoding), so every valid resource entry triggers a "field 'type' ... ignored" warning unconditionally, not just for unusual input. This is a disclosed spec tradeoff, not a bug — flagged here so a future pass over `xceadmin.build`'s warning UX (e.g. only warning once per field-name per run instead of once per item) knows the noise is expected, not a regression. `xceadmin.build`'s unmapped-field table (`_UNMAPPED_BUT_VALID_ITEM_KEYS`) was derived by cross-referencing `xcecommon.mapping`'s per-item builder functions against the real `src/configs/schema/*.schema.json` property lists directly (not from prose alone) to avoid drift.
- **3.2**: design.md's Dependencies list for `xceadmin.discovery` names only `xcecommon.wire`, but decoding a reply to inspect `RequestReply.redirectHost`/`.redirectPort` genuinely requires `xcecommon.schema` too (verified real field names against `controller.capnp:594-595`, which matched design.md exactly this time — unlike task 2.2's `Reply`/`RequestReply` struct-name slip). Treated as a design-doc omission, not license to hand-roll capnp decoding. `follow_redirects`'s `cert_dir` parameter is accepted for signature parity with design.md but unused internally — all I/O, including TLS setup, is delegated to the caller's `send_fn` closure, which is expected to already carry its own `cert_dir`. A connection timeout to any hop (including a non-initial one reached via redirect) raises `NoPrimaryError` naming that specific hop, not the original target.
- **bug-1461 (out-of-band, between 3.2 and 3.3)**: `controller.capnp` had a real, confirmed schema bug — a locally-declared `struct Data` (an `OPCode_get` query-result tagged union) shadowed Cap'n Proto's builtin `Data` blob type for every later `: Data` field in the file, miscompiling `UpgradeConfigRequest.sha256`/`.bytes` (and 5 other fields across `ReplConfigReply`/`ReplConfigPushRequest`/`ReplEnvelope`/`SnapshotTransfer`/`WalCatchupEntry`/`CtrlHelloRequest`, several HA-critical) as a struct-pointer type instead of a byte blob — blocking `push_config()` from constructing a valid request via pycapnp. Fixed at the root (renamed the local struct to `XceData`, `$C.mapname("xce_data_t")` unchanged so the C typedef is stable) across 20 files spanning controller/lib/scheduler/Go — see commit `e53c086` and `.wolf/buglog.json` bug-1461 for the full investigation and fix. This was NOT an admin-config-tool-boundary change; it's a pre-existing, cross-component schema defect that happened to block this task. `push_config()`'s real controller counterpart remains untested live — a *separate*, already-documented dispatch-gate defect (`ha_dispatch_gate()`/`cluster.c`, spec.json `blocked_on`) still blocks that independently of bug-1461.
- **bug-1461 (out-of-band, between 3.2 and 3.3)**: `controller.capnp` had a real, confirmed schema bug — a locally-declared `struct Data` (an `OPCode_get` query-result tagged union) shadowed Cap'n Proto's builtin `Data` blob type for every later `: Data` field in the file, miscompiling `UpgradeConfigRequest.sha256`/`.bytes` (and 5 other fields across `ReplConfigReply`/`ReplConfigPushRequest`/`ReplEnvelope`/`SnapshotTransfer`/`WalCatchupEntry`/`CtrlHelloRequest`, several HA-critical) as a struct-pointer type instead of a byte blob — blocking `push_config()` from constructing a valid request via pycapnp. Fixed at the root (renamed the local struct to `XceData`, `$C.mapname("xce_data_t")` unchanged so the C typedef is stable) across 20 files spanning controller/lib/scheduler/Go — see commit `e53c086` and `.wolf/buglog.json` bug-1461 for the full investigation and fix. This was NOT an admin-config-tool-boundary change; it's a pre-existing, cross-component schema defect that happened to block this task. `push_config()`'s real controller counterpart was, separately, blocked by a dispatch-gate defect (`ha_dispatch_gate()`/`cluster.c`) — **RESOLVED 2026-07-25 (bug-1468, commit `959b0cda`)**, fixed by `config-upgrade-snapshot` task 8.6 hitting the same wall building a live `OP_upgradeConfig` sender; that path has since been exercised live and successfully many times via that spec's `--config-upgrade` soak. Nothing blocks a live `push_config()` E2E run anymore, though `xceadmin`'s own TLS/redirect/framing code has not yet had one specifically.
- **3.3**: `probe_config()` does NOT go through `xceadmin.discovery.follow_redirects` — `ProbeConfigTagReply` is a direct struct with only `configTag`, no `redirectHost`/`.Port` fields exist on it at all, so there is no redirect signal for `follow_redirects` (which unconditionally decodes replies as `RequestReply`) to read; only `push_config()`'s `Request`/`RequestReply`-wrapped exchange supports redirects. `ProbeConfigTagReply` also carries no accept/reject signal — `DryRunResult.accepted` is a client-side inference (true on any successful round-trip), matching the real C reference client (`config_repl.c`'s `xce_probe_config_tag`, which also always sends an empty `name` and returns only a bare tag). pycapnp union field assignment for Void/Text-typed variants (e.g. `reply.data.nodata = None`, `reply.data.debugmsg = "..."`) must use direct assignment, not `.init(name)` — `.init()` without a size is only valid for struct/list/data-typed variants, confirmed via `KjException: init() without a size is only valid for struct and object fields`.
- **4.1**: `xcecommon.wire.connect()` deliberately lets `ConnectionRefusedError`/other bare `OSError`s propagate unwrapped (documented in `wire.py`'s own module docstring) — `xceadmin.cli` is the layer that catches `OSError` and formats it per Requirement 7.1, alongside its `except XceAdminError` dispatch surface; a task 4.1 implementer must not assume `XceAdminError` alone covers every network failure mode. click 8.3.1's `CliRunner` no longer accepts `mix_stderr=`; `result.stdout`/`.stderr` are separated by default, but `CliRunner`'s output capture decodes as text and is **lossy for arbitrary binary** (confirmed: `bytes(range(256))` does not round-trip through the captured `.output` string) — do not assert exact byte content of a `-o -` binary write through `CliRunner`; that's already covered at the `write_output()` unit level (task 3.1's `test_config_build.py`). A CLI test that omits `-o` falls through to the real default (`cluster.bin`, relative to CWD) — always pass an explicit `-o <tmp_path>` in tests that reach the write step, or the test will write a stray artifact into the actual repo working directory (caught via `git status` before commit, not by the test itself).
- **4.2**: `deploy-ha.sh` resolves both `xceadmin` (`$SCRIPT_DIR/../xceadmin/.venv/bin/xceadmin`) and the YAML configs source (`$SCRIPT_DIR/../configs`) relative to the script's own location (`$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)`), not the caller's CWD — matches the project's other relative-path scripts and keeps the deploy working regardless of where it's invoked from. `--build-config` is parsed out of `"$@"` before positional-arg assignment so it can appear anywhere in the argument list, not just a fixed position. Verified live against the real `xceadmin` build and `src/configs/`: missing `cluster.bin` auto-builds (8.4), an existing one with no flag is left byte-for-byte untouched — same mtime, same SHA (8.5), and `--build-config` against an existing `cluster.bin` forces a rebuild that reproduces byte-identical content (8.3, confirming 9.1's determinism guarantee holds through this path too).