fix(admin-config-tool): unwrap Request reply envelope before decoding as RequestReply

config push crashed on every real controller with a capnp KjException --
xce_reply() wraps every reply in an outer Request{body.replymsg} envelope,
but discovery.py/push.py decoded raw wire bytes directly as a bare
RequestReply. Adds a shared decode_reply() unwrap point used by both call
sites, and fixes the same wrong-shape assumption baked into task 5.6's
and test_cli.py's mock-server fixtures.
This commit is contained in:
Rongsong Shen 2026-07-30 09:43:56 +00:00
parent 46bbdaa9a6
commit 915e6fdf64
7 changed files with 231 additions and 26 deletions

View file

@ -141,6 +141,24 @@
- _Boundary: xceadmin.push, xceadmin.discovery_
- _Depends: 3.3_
- [ ] 6. Fix reply envelope decode and add live E2E push coverage
- [x] 6.1 Unwrap the `Request` reply envelope before decoding as `RequestReply`
- Live-verified 2026-07-30 against a real deployed single-node controller: `config push` crashes every time with `capnp.lib.capnp.KjException: ... non-list pointer where text was expected`. Root cause: `xce_reply()` (`src/lib/request.c`) wraps every reply — including `config push`'s — in an outer `Request{opcode: OPCode_reply, body: union{replymsg @4: RequestReply}}` envelope (`src/schema/controller.capnp:645-651`) before writing it to the wire. Two call sites decode the raw wire bytes directly as a bare `RequestReply`, skipping the outer envelope, which misaligns every pointer offset: `xceadmin.discovery._redirect_target()` (crashes first, on every reply) and `xceadmin.push.push_config()`'s own `mod.RequestReply.from_bytes(reply_bytes)` (same latent bug, currently unreached only because discovery crashes first — fixing discovery alone is not sufficient). The controller side is not at fault: server logs confirm the same request's dispatch, commit, and activation all succeed for the exact bytes that then crash the client on decode.
- Both decode sites unwrap `Request.body.replymsg` before treating the bytes as a `RequestReply` — introduce one shared decode helper (e.g. in `xceadmin.discovery`, which `xceadmin.push` already imports from) rather than duplicating the unwrap logic at each call site
- `config push` against a real controller (mock or live) succeeds end to end: exits 0, prints `versionTag`/`ackedCount`, no `KjException`
- Redirect-following (standby → primary) still decodes correctly through the fixed unwrap — existing task 5.6 mock-based redirect tests continue to pass unmodified after their mock reply fixtures are updated to the real envelope shape
- _Requirements: 2.1, 2.5, 3.1, 3.2, 7.2, 7.3_
- _Boundary: xceadmin.discovery, xceadmin.push_
- _Depends: 3.2, 3.3_
- [ ] 6.2 Add a live single-node E2E test for `config push`
- New test deploys one real `xce_controller` (no HA peers needed — reachability/decode correctness is what's under test, not consensus), runs `xceadmin config build` against `src/configs/`, then a real `xceadmin config push` against it over the actual TLS+capnp wire protocol — no mock server
- Test asserts exit 0, a parsed `versionTag`/`ackedCount` in the output, and confirms the controller's own log shows the config committed and activated
- Skips cleanly (not a hard failure) when no controller binary is available in the current build profile, matching this suite's existing cert-dependent skip convention (see task 5.3's Implementation Notes)
- _Requirements: 2.1, 2.5, 2.6, 3.1_
- _Boundary: xceadmin (test-only, cross-cutting with the built controller binary)_
- _Depends: 6.1_
## Implementation Notes
- **1.3**: nix's bare `python3` does not put `pip` on PATH — `python3Packages.pip` had to be added to `nativeBuildInputs` alongside the packages design.md's Technology Stack table lists; not a spec gap, just an operational dependency the table didn't need to spell out.
@ -165,4 +183,6 @@
- **Post-implementation, out-of-band (2026-07-22, second pass)**: `meson-build.sh`/`meson-install.sh` no longer resolve `python3` via a bare PATH lookup — `meson.build` now pins it once via `import('python').find_installation('python3')` (same mechanism as `find_program('go')`/capnp/packcc elsewhere in this build, visible at `meson setup` time as `Program python3 found: YES (/nix/store/...)`) and threads the resolved path into both scripts explicitly. Verified: `meson setup` prints the pinned interpreter, rebuild stays 165/165 green, and the built venv's own `python3` resolves through to that exact store path.
- **Post-implementation, out-of-band (2026-07-22, third pass — removes `ha/data/` entirely)**: `deploy-ha.sh` no longer takes a `<data-dir>` argument at all (`--build-config` flag also removed) — every invocation now generates a fresh self-signed TLS cert/key (`openssl req -x509 -newkey rsa:2048 -nodes -days 3650`, 10y validity) directly into the deploy dir's `certs/`, and builds `cluster.bin` directly into `controller/ctrl-1/conf/` via `xceadmin`, with no intermediate staging directory. `run-ha-soak.sh` (`/xce-test-ha`) updated to match: dropped its own `XCE_HA_DATADIR`-based precondition entirely, since `deploy-ha.sh` now handles both cert and config generation itself. Updated 4 callers (`run-ha-soak.sh`, `test-heartbeat-tolerance.sh`, `test-commit-durability.sh`, plus both `xce-deploy-ha`/`xce-test-ha` SKILL.md docs) to the new 3-arg signature. `ha/data/{xce.crt,cluster.bin}` removed from git tracking (`cluster.bin` was already gitignored from the prior pass; the certs were a real checked-in TLS private key, now gone from history going forward).
- **6.1**: `decode_reply()` (new, `discovery.py`) is the single unwrap point for the `Request{opcode: reply, body.replymsg: RequestReply}` envelope every real controller reply arrives in (`xce_reply()`, `src/lib/request.c:384-418`) — both `discovery._redirect_target()` and `push.push_config()` go through it; `probe_config()` is untouched (`ProbeConfigTagReply` is never envelope-wrapped). Fixing the two documented call sites alone was insufficient — task 5.6's own mock-server test fixtures (`test_discovery.py`, `test_config_push_mock.py`) AND `test_cli.py`'s two `config push` mock handlers all constructed bare, unwrapped `RequestReply` bytes, meaning the entire existing test suite had been silently validating the wrong wire shape since task 5.6/task 4.1. All fixed via one shared `wrap_reply` fixture in `conftest.py`. Any future test that hand-builds a controller reply for a mock server must wrap it the same way, or it will pass against a mock while still being broken against a real controller.
Investigating this surfaced a real, independent finding: `xcecommon.wire.connect()` was loading a **client-side** TLS cert (`context.load_cert_chain(...)` on the `PROTOCOL_TLS_CLIENT` context) — verified against the actual reference implementation (`netio.c`'s `create_ssl_context()`) that the real client presents **no** certificate at all when `cert_file`/`key_file` are `NULL` (bare `TLS_client_method()`, no `SSL_CTX_use_certificate_chain_file`/`SSL_CTX_use_PrivateKey_file` call). `xceadmin`'s client diverged from the reference client it's meant to mirror. Fixed: `connect()` no longer loads a client cert or takes a `cert_dir` parameter at all. Since nothing else consumed `cert_dir` (no CA verification happens either — `verify_mode=CERT_NONE` throughout), removed it end-to-end: `xceadmin.discovery.follow_redirects()`'s `cert_dir` param (already dead/`noqa`'d — signature-parity leftover), `xceadmin.push.push_config()`/`probe_config()`, and the `--cert-dir` CLI flag on `config push`. Server-side cert loading (the mock TLS servers in `test_wire.py`/`test_cli.py`/`test_config_push_mock.py`) is unaffected and still needed — moved into a shared session-scoped `tls_certs` fixture in a new `tests/conftest.py` (openssl-generated, same approach as `deploy-ha.sh`), replacing 3 duplicated module-level `REPO_HA_DATA_DIR`/`pytest.mark.skipif(not ha/data/xce.crt...)` blocks. Net result: those 3 files' TLS-dependent tests no longer skip under a hermetic `nix build` sandbox (which never had `ha/data/` in its git-tracked-only source) — verified 165/165 passed, 0 skipped, both via `meson compile` and via a full `nix build --option sandbox false .#xcecore_debug` rebuild (previously 128 passed / 37 skipped there). Requirements 2.3/2.4 and 8.3/8.4/8.5 marked superseded in `requirements.md` with dated correction notes rather than silently rewritten.