From 6193fa7ea10ec347f6e3848ebdd9c3a6c6ded40b Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Sat, 29 Aug 2026 12:10:28 +0000 Subject: [PATCH] test(admin-config-tool): update standby-redirect E2E for bug-1700/1701 test_live_config_push_against_standby_returns_standby_rejection_not_redirect locked in the OLD controller behavior (a standby flatly rejects OP_upgradeConfig with no redirect fields). That gap is now closed: myreq_handler's gate-rejection path calls fill_redirect_reply() on every -XCE_ERR_STANDBY rejection (bug-1700), and handle_ctrlhello() now actually populates primary_host/primary_port from the static --peers list (bug-1701) -- a standby pushing config now redirects to the primary and the push succeeds. Renamed to test_live_config_push_against_standby_redirects_and_succeeds and rewritten to assert the new behavior. _assert_primary_committed() changed from a single-shot log read to a poll loop (up to 5s) for the "config activated from committed marker..." line -- accounts for the documented async gap between the majority-ack-gated client reply and the primary's own local WAL-apply- pump activation, a real race the old single-shot read could hit. meson-build.sh: wire src/tools and each build profile's controller subdir into the isolated repo-mirror the E2E tests run against, and export XCEADMIN_BIN explicitly, so these live tests actually find real binaries instead of always skipping. Verified: xceadmin suite 288/288 (incl. both tests in this file); controller suite 1220/1222 (only the pre-existing bug-1561 flake). Updated admin-config-tool design.md and roadmap.md to mark the dispatch-gate and redirect-wiring gaps RESOLVED, matching this test's new assertions and the live evidence. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_019QxaSzHRB7pmYFp4cVnwkA --- .kiro/specs/admin-config-tool/design.md | 16 +- .kiro/steering/roadmap.md | 2 +- src/xceadmin/meson-build.sh | 36 +++- src/xceadmin/tests/test_e2e_live_ha_push.py | 214 ++++++++++++-------- 4 files changed, 168 insertions(+), 100 deletions(-) diff --git a/.kiro/specs/admin-config-tool/design.md b/.kiro/specs/admin-config-tool/design.md index df909f30..e69f73b4 100644 --- a/.kiro/specs/admin-config-tool/design.md +++ b/.kiro/specs/admin-config-tool/design.md @@ -30,8 +30,8 @@ - Test fixtures and golden files under `src/xceadmin/tests/`. ### Out of Boundary -- **Controller-side `OP_upgradeConfig` dispatch** — `ha_dispatch_gate()` excludes `OPCode_upgradeConfig` from the standby allow-list (`ha.c:3926-3951`), and `cluster.c:667-679` calls `handle_upgrade_config()` only when `ha_is_primary()` is **false**. Combined, no controller role reaches `do_upgrade_config_with_bytes()` via the network today — it is exercised only by unit tests calling it directly. This is a controller defect, not an xceadmin defect; xceadmin implements the client side of the documented contract (`config_repl.h:347-349` — "handle an OP_upgradeConfig request from an admin tool") and cannot itself fix the gate. Flagged as a blocking risk below. -- **Redirect wiring for `OP_upgradeConfig`** — `fill_redirect_reply()` (`join.c:27-31`) is only invoked from the `OPCode_join` path (`join.c:277`). `RequestReply.redirectHost`/`redirectPort` are never populated for an `upgradeConfig` reply today. xceadmin implements generic, opcode-agnostic redirect-following per the `RequestReply` contract (forward-compatible), but it will not fire for `config push` until the controller is extended. +- ~~**Controller-side `OP_upgradeConfig` dispatch**~~ — **RESOLVED 2026-07-25 (bug-1468).** `cluster.c`'s `myreq_handler` `OPCode_upgradeConfig` case had its primary/standby guard polarity inverted by an unrelated commit (`fd93997f`, 2026-05-31: `if (ha_is_primary(...)) reject` instead of the correct `if (!ha_is_primary(...)) reject`), which rejected the one role — primary — that `ha_dispatch_gate()` ever lets reach this switch case. Restored to the correct polarity; the primary now reaches `do_upgrade_config_with_bytes()` via the network as designed. Live-verified: `test_e2e_live_push.py::test_live_config_push_against_real_single_node_controller`, `test_e2e_live_ha_push.py::test_live_config_push_against_primary_reaches_success_path`. +- ~~**Redirect wiring for `OP_upgradeConfig`**~~ — **RESOLVED 2026-08-26 (bug-1700/1701).** `cluster.c`'s generic `ha_dispatch_gate()` rejection path now calls `fill_redirect_reply()` whenever the gate rejects with `-XCE_ERR_STANDBY` (`cluster.c:411-437`) — a fix at the gate itself, covering every opcode that hits that rejection, not an `OPCode_upgradeConfig`-specific patch. `RequestReply.redirectHost`/`redirectPort` are now populated when a standby rejects `config push`, and xceadmin's already-generic redirect-following (Allowed Dependencies) transparently retries against the primary. Live-verified: `test_e2e_live_ha_push.py::test_live_config_push_against_standby_redirects_and_succeeds`. - **`ProbeConfigTag` server-side handling** — `xce_probe_config_tag()` (`config_repl.c:1829`) has no server-side dispatch case anywhere in `cluster.c` and no production caller; it is dormant plumbing. xceadmin's dry-run client implements the wire contract but has nothing live to call. - Entity-bearing snapshots, config rollback, version history, config activation status (owned by `config-upgrade-snapshot`, blocked upstream on `entity-bearing-snapshots`). - TLS mutual authentication / hostname verification — none exists anywhere in the codebase (`SSL_VERIFY_NONE` is set explicitly for the controller's own outbound client context, `myapp.c:109`; no `X509_check_host` call exists). xceadmin matches this existing posture; it neither introduces nor removes an authentication guarantee. @@ -45,8 +45,8 @@ ### Revalidation Triggers - `controller.capnp` `ClusterConfig` field renumbering/addition/removal (already happened once this session via `clusterconfig-schema-split`) — invalidates the field-mapping table in Data Models. -- A fix to the `cluster.c`/`ha.c` dispatch-gate contradiction — flips `config push` E2E testing from "mock controller only" to "real cluster required," per Testing Strategy. -- `fill_redirect_reply()` extended to `OPCode_upgradeConfig` — unlocks live auto-primary-discovery testing for config push (currently only testable against `OPCode_join`-shaped mocks). +- ~~A fix to the `cluster.c`/`ha.c` dispatch-gate contradiction~~ — **TRIGGERED 2026-07-25 (bug-1468).** `config push` E2E now runs against a real cluster (`test_e2e_live_push.py`, `test_e2e_live_ha_push.py`), superseding the "mock controller only" framing in Testing Strategy. +- ~~`fill_redirect_reply()` extended to `OPCode_upgradeConfig`~~ — **TRIGGERED 2026-08-26 (bug-1700/1701).** Closed via a broader, gate-level fix (every opcode hitting the `-XCE_ERR_STANDBY` rejection gets a redirect, not an `OPCode_upgradeConfig`-specific patch) — see Boundary Commitments. Live auto-primary-discovery testing for config push now exists: `test_e2e_live_ha_push.py::test_live_config_push_against_standby_redirects_and_succeeds`. - A server-side handler added for `OPCode_probeConfigTag` — unlocks `config push --dry-run` E2E testing. ## Architecture @@ -187,7 +187,7 @@ sequenceDiagram X->>Op: versionTag ackedCount error, exit code ``` -At most 3 redirect hops (3.2); each hop is a fresh TLS connection with the same 30s timeout (3.4, 7.6). Because `fill_redirect_reply()` does not currently fire for `OPCode_upgradeConfig` (see Boundary Commitments), the `alt` branch above is exercised only in mock-controller tests until the controller gap is closed. +At most 3 redirect hops (3.2); each hop is a fresh TLS connection with the same 30s timeout (3.4, 7.6). `fill_redirect_reply()` now fires for a standby's `OP_upgradeConfig` rejection too (bug-1700/1701, see Boundary Commitments) — the `alt` branch above is live-verified against a real 3-node cluster (`test_e2e_live_ha_push.py::test_live_config_push_against_standby_redirects_and_succeeds`), not just mock-controller tests. ## Requirements Traceability @@ -457,7 +457,7 @@ def probe_config(cluster_bin_path: str, controller: str, cert_dir: str) -> DryRu - Inspects any `Reply` for non-empty `redirectHost`/`redirectPort`; reconnects and resends up to 3 hops (3.1, 3.2), then errors. - Displays the redirect target when a standby redirects (3.3). - 30s "no primary elected" timeout is a property of each connection attempt, not the overall discovery loop (3.4, delegated to `xcecommon.wire.connect`). -- Implemented generically against the `RequestReply` schema shape (not `upgradeConfig`-specific) so it is correct today for `OPCode_join`-style redirects and automatically correct for `upgradeConfig` once the controller wires `fill_redirect_reply()` for that opcode too (Simplification — no opcode-specific branching to remove later). +- Implemented generically against the `RequestReply` schema shape (not `upgradeConfig`-specific) so it was correct for `OPCode_join`-style redirects from the start and required zero client-side changes once the controller closed the `upgradeConfig` gap (bug-1700/1701, 2026-08-26) — validated in practice, not just in principle (Simplification — no opcode-specific branching was ever needed). **Dependencies** - Outbound: `xcecommon.wire` (P0). @@ -532,8 +532,8 @@ Out of scope — `xceadmin` is a one-shot CLI with stderr/exit-code as its only - **Unit — wire framing**: `send_frame`/`recv_frame` round-trip against a fixed byte sequence, and against a loopback `socket.socketpair()` — verifies native-order packing matches `struct.pack("=Q", ...)` semantics without needing a real controller. - **Integration — config build (golden-file)**: `minimal`/`full`/`invalid` fixture directories (File Structure Plan) — `full` now covers all 7 YAML files including the newly-added `xce.yaml`/`scaler.yaml`; byte-compares `build_cluster_config()` output against a committed `expected.bin` and its SHA-256 (1.1, 1.2, 1.3, 1.4, 1.5, 1.6, 1.7, 1.8, 1.9, 1.10, 1.11, 1.12, 9.1, 9.2, 9.3). Plain `assert produced == expected_bytes` fixture comparison — `pytest-regressions` was evaluated (see `research.md`) but not adopted, since this project's nix-pinned dependency list would need independent verification and the byte-equality need here is simple enough not to warrant it (Simplification). - **Integration — validation**: one test per `invalid/` fixture case — schema violation, reserved name, duplicate name (including a limit/queue name collision, proving 4.3's intentionally-narrow scope), undefined reference (warn, not fail). -- **Integration — config push (mock controller)**: a Python TLS server speaking just enough of the wire protocol (frame in, decode `Request` or direct-struct `ProbeConfigTagRequest`, encode a canned `Reply`/`ProbeConfigTagReply`) to exercise `push_config()` (2.1, 2.2, 2.3, 2.4, 2.5, 2.6, 2.7, 3.1, 3.2, 3.3 redirect, 7.1, 7.2, 7.3) and `probe_config()` (6.1, 6.2, 6.3 — accepted and rejected probe replies) against the same mock, since both share `xcecommon.wire`'s framing/send-receive primitives. **This is the only realistic way to test config push today** — not merely "preferred for the dev loop" as `brief.md` originally framed it, but load-bearing: the controller-side dispatch-gate defect found during discovery (Boundary Commitments) means a real 3-node cluster currently rejects `OP_upgradeConfig` from every role, so live-cluster E2E (`brief.md`'s "Option B") is **blocked**, not merely deferred, until that controller defect is fixed (Revalidation Trigger). -- **E2E (deferred)**: real-cluster `config push` round-trip via `xce-test-ha` — tracked in `brief.md`, explicitly gated on both the controller dispatch fix and `config-upgrade-snapshot` landing. +- **Integration — config push (mock controller)**: a Python TLS server speaking just enough of the wire protocol (frame in, decode `Request` or direct-struct `ProbeConfigTagRequest`, encode a canned `Reply`/`ProbeConfigTagReply`) to exercise `push_config()` (2.1, 2.2, 2.3, 2.4, 2.5, 2.6, 2.7, 3.1, 3.2, 3.3 redirect, 7.1, 7.2, 7.3) and `probe_config()` (6.1, 6.2, 6.3 — accepted and rejected probe replies) against the same mock, since both share `xcecommon.wire`'s framing/send-receive primitives. This remains valuable for dev-loop speed and `probe_config()` coverage (`ProbeConfigTag` still has no server-side handler — Out of Boundary), but it is no longer the only way to test `push_config()`: the controller-side dispatch-gate defect that once blocked live-cluster testing was fixed (bug-1468, 2026-07-25), and the standby-redirect gap that followed it was fixed too (bug-1700/1701, 2026-08-26) — see Boundary Commitments and E2E below. +- **E2E**: real-cluster `config push` round-trip, live against genuine `xce_controller` processes — `test_e2e_live_push.py` (single-node, task 6.2) and `test_e2e_live_ha_push.py` (3-node HA, task 6.3): primary success path (positive `ackedCount`, real standby ACKs), and the standby-redirect success path (`test_live_config_push_against_standby_redirects_and_succeeds`). No longer gated on the controller dispatch fix or the redirect fix — both landed; `brief.md`'s "Option B" is realized. ## Security Considerations diff --git a/.kiro/steering/roadmap.md b/.kiro/steering/roadmap.md index 2128a5be..dc91152b 100644 --- a/.kiro/steering/roadmap.md +++ b/.kiro/steering/roadmap.md @@ -87,7 +87,7 @@ Smaller items that likely don't warrant a full spec; pick up opportunistically o - [ ] 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.)_ -- [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] ~~OP_upgradeConfig standby redirect verification~~ — **RESOLVED 2026-08-19, then the underlying gap itself CLOSED 2026-08-26 (bug-1700/1701).** As of 2026-08-19, a standby did NOT return the primary's address for `OPCode_upgradeConfig` — `ha_dispatch_gate` rejected it before the handler's own dead defensive check ever ran, replying flat `status=-XCE_ERR_STANDBY, data_which=nodata` with no redirect fields; `fill_redirect_reply()` (`join.c:27`) was never called for this opcode, a documented, deliberate feature gap (`admin-config-tool` design.md's "Revalidation Triggers"). That gap is now closed: `cluster.c`'s generic dispatch-gate rejection path calls `fill_redirect_reply()` on every `-XCE_ERR_STANDBY` rejection (bug-1700/1701, gate-level fix, not opcode-specific), and a standby pushing `config push` now redirects to the primary and succeeds. Coverage: client-side redirect-following remains unit-tested against mocks (`test_discovery.py`, `test_config_push_mock.py::test_push_config_follows_one_redirect_to_primary`); the real server-side behavior is live-verified against a real 3-node cluster (`src/xceadmin/tests/test_e2e_live_ha_push.py::test_live_config_push_against_standby_redirects_and_succeeds`, renamed from `..._returns_standby_rejection_not_redirect` to match the now-correct outcome). - [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 `** — 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 `** — 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.)_ - [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. diff --git a/src/xceadmin/meson-build.sh b/src/xceadmin/meson-build.sh index 47efe696..2d0aa273 100755 --- a/src/xceadmin/meson-build.sh +++ b/src/xceadmin/meson-build.sh @@ -63,6 +63,29 @@ mkdir -p "$MIRROR_DIR/src" ln -s "$REPO_ROOT/ha" "$MIRROR_DIR/ha" ln -s "$SRC_DIR/configs" "$MIRROR_DIR/src/configs" ln -s "$SRC_DIR/schema" "$MIRROR_DIR/src/schema" +# tests/_live_e2e_common.py's DEPLOY_HA_SH points at a sibling tools/ +# dir -- same fixed-relative-path assumption as configs/schema above. +ln -s "$SRC_DIR/tools" "$MIRROR_DIR/src/tools" + +# tests/_live_e2e_common.py's CONTROLLER_CANDIDATES look for a real +# xce_controller binary under a sibling xce.build/build.release/build.dbg +# dir (xce-build.sh's own profile-dir naming, src/tools/xce-build.sh) next +# to this file's own SRC_DIR -- same fixed-relative-path assumption as +# ha/configs/schema above, so it needs the same out-of-tree symlink +# treatment or the live E2E tests can never find a controller binary that +# was actually built, no matter what's really on disk. Only the `controller` +# subdir is linked (not the whole profile-root xce.build/etc.) because +# `controller` and `xceadmin` are always siblings under a profile root -- +# unlike the profile root itself, this is never an ancestor of $MIRROR_DIR +# (which nests only under /xceadmin), so it can't form a +# self-referential symlink loop the way linking the whole root would. +for _live_e2e_build_dir in xce.build build.release build.dbg; do + _live_e2e_controller_dir="$SRC_DIR/$_live_e2e_build_dir/controller" + if [ -d "$_live_e2e_controller_dir" ]; then + mkdir -p "$MIRROR_DIR/src/$_live_e2e_build_dir" + ln -s "$_live_e2e_controller_dir" "$MIRROR_DIR/src/$_live_e2e_build_dir/controller" + fi +done cp -r "$SOURCE_DIR" "$WORK_DIR" # Never trust a stray dev-loop artifact from the live checkout (xce-build.sh's @@ -115,7 +138,18 @@ rm -rf "$VENV_DIR" "$VENV_DIR/bin/pip" install "$WORK_DIR" --quiet echo "=== xceadmin (meson): running tests ===" -"$VENV_DIR/bin/python" -m pytest "$WORK_DIR/tests" -v +# tests/_live_e2e_common.py's XCEADMIN_CANDIDATES look for a sibling +# xce.build/build.release/build.dbg/xceadmin/.venv/bin/xceadmin next to this +# file's own SRC_DIR -- but for THIS profile that path is $BUILD_DIR itself +# (the xceadmin subdir currently being built), which is an ancestor of +# $MIRROR_DIR and so can't be symlinked into the mirror without a +# self-referential loop (unlike the sibling `controller` dir linked above). +# Route around it the same way a caller with a custom/explicit build dir +# already would: XCE_CONTROLLER_BIN/XCEADMIN_BIN +# (resolve_binary_candidates in _live_e2e_common.py) override discovery +# entirely, and this build's own freshly pip-installed binary is exactly +# what the live E2E tests should exercise. +XCEADMIN_BIN="$VENV_DIR/bin/xceadmin" "$VENV_DIR/bin/python" -m pytest "$WORK_DIR/tests" -v # Build an installable wheel from the SAME schema-synced $WORK_DIR the pinned # install above just tested -- not a second, separately-assembled tree -- diff --git a/src/xceadmin/tests/test_e2e_live_ha_push.py b/src/xceadmin/tests/test_e2e_live_ha_push.py index 078f32cb..136029ba 100644 --- a/src/xceadmin/tests/test_e2e_live_ha_push.py +++ b/src/xceadmin/tests/test_e2e_live_ha_push.py @@ -12,48 +12,59 @@ pattern -- see ``live_ha_cluster`` below): ``test_live_config_push_against_primary_reaches_success_path`` genuinely exits 0 with a positive ``ackedCount`` from real standby ACKs. -REAL FINDING, not a test bug -- the redirect scenario does NOT close the -way the task text originally anticipated: pushing `config push` to a -*standby* does NOT redirect to the primary against a real controller -today, and this is not something a test-only change can fix. This is an -ALREADY-DOCUMENTED, pre-existing gap in design.md's own "Boundary -Commitments" section (`.kiro/specs/admin-config-tool/design.md`, "Out of -Boundary" -- "Redirect wiring for `OP_upgradeConfig`") and "Revalidation -Triggers" (`fill_redirect_reply()` extended to `OPCode_upgradeConfig` is -listed there as a FUTURE, not-yet-done trigger): `fill_redirect_reply()` -(`join.c:27-31`) is only ever invoked from the `OPCode_join` path -(`join.c:277`); nothing calls it for `OPCode_upgradeConfig`. -`RequestReply.redirectHost`/`redirectPort` are therefore never populated +UPDATE (2026-08-28): the redirect gap described below WAS closed, exactly +as this docstring's own original prediction anticipated -- see +``test_live_config_push_against_standby_redirects_and_succeeds`` (renamed +from ``..._returns_standby_rejection_not_redirect``), which now live- +verifies the success-via-redirect path instead. The investigation history +below is kept for context (why the task's original text and the +mid-investigation finding diverged, and exactly what changed to close the +gap); treat every "does NOT redirect" / "no redirect line, no success" +statement below as describing REAL, live-verified controller behavior AS +OF 2026-08-26 (the day before bug-1700/1701 landed), not current behavior. + +ORIGINAL FINDING (now superseded) -- the redirect scenario did NOT close +the way the task text originally anticipated: pushing `config push` to a +*standby* did NOT redirect to the primary against a real controller as of +this file's original writing, and this was not something a test-only +change could fix. This was an ALREADY-DOCUMENTED, pre-existing gap in +design.md's own "Boundary Commitments" section +(`.kiro/specs/admin-config-tool/design.md`, "Out of Boundary" -- "Redirect +wiring for `OP_upgradeConfig`") and "Revalidation Triggers" +(`fill_redirect_reply()` extended to `OPCode_upgradeConfig` was listed +there as a FUTURE, not-yet-done trigger): `fill_redirect_reply()` +(`join.c:27-31`) was only ever invoked from the `OPCode_join` path +(`join.c:277`); nothing called it for `OPCode_upgradeConfig`. +`RequestReply.redirectHost`/`redirectPort` were therefore never populated for an `upgradeConfig` reply. Confirmed directly in `cluster.c`'s -`myreq_handler`: a standby's `OP_upgradeConfig` request is rejected by the +`myreq_handler`: a standby's `OP_upgradeConfig` request was rejected by the generic `ha_dispatch_gate()` role check (`ha.c`, opcode absent from the `CTRL_STANDBY` allow-list) BEFORE the opcode-specific `case -OPCode_upgradeConfig:` handler ever runs -- the gate-rejection reply path -(`cluster.c` ~line 273-280) sends `reply.status = -XCE_ERR_STANDBY` +OPCode_upgradeConfig:` handler ever ran -- the gate-rejection reply path +sent `reply.status = -XCE_ERR_STANDBY` (`errnox.h`: `E_XCE_STANDBY == XCE_ERRNO_BASE + 13 == 10014`) with -`data_which = RequestReply_data_nodata` and never touches +`data_which = RequestReply_data_nodata` and never touched `redirectHost`/`redirectPort` at all. Live-verified against a real 3-node -cluster: pushing to a standby returns `versionTag=0 ackedCount=0`, exit 1, -stderr `"Error: controller returned status -10014"` -- no redirect line, -no success. `xceadmin.discovery.follow_redirects`/`decode_reply` behave -exactly per Requirement 3.1's contract given this reply (both fields -absent/empty => no redirect => the bare rejection is returned as-is); the -gap is entirely server-side and out of this task's test-only boundary to -fix (design.md: "This is a controller defect, not an xceadmin defect; -xceadmin implements the client side of the documented contract... and -cannot itself fix the gate"). Task 5.6 already covers the SUCCESSFUL -redirect-follow path against mocks (`test_config_push_mock.py`), which is -still the only place that path is exercised. -``test_live_config_push_against_standby_returns_standby_rejection_not_redirect`` -below live-verifies and regression-locks this REAL current behavior -instead of the hypothetical "redirect then succeed" outcome -- the same -resolution task 6.2 used for its own real-vs-literal-task-text mismatch -(single-node `ackedCount == 0`/exit 1). The moment a future controller-side -task wires `fill_redirect_reply()` for `OPCode_upgradeConfig` (already -tracked as a Revalidation Trigger in design.md), THIS test will start -failing on the "no redirect line" assertion -- that failure is the signal -to replace it with a real success-path assertion, matching what this -docstring originally described before investigation. +cluster at the time: pushing to a standby returned `versionTag=0 +ackedCount=0`, exit 1, stderr `"Error: controller returned status +-10014"` -- no redirect line, no success. + +WHAT CLOSED IT (bug-1700/1701, fixed 2026-08-26): `cluster.c`'s generic +`myreq_handler` dispatch-gate rejection path (~`cluster.c:411-437`) now +calls `fill_redirect_reply(&reply, &c->ha)` whenever `ha_dispatch_gate()` +rejects with `-XCE_ERR_STANDBY` -- a fix at the GATE itself, covering +every opcode that hits that rejection, not an `OPCode_upgradeConfig`- +specific patch (broader than what the original Revalidation Trigger +anticipated). A second, independently-discovered blocker (bug-1701: +`ha_state_t.primary_host`/`primary_port` were never actually written by +any production code path on a standby, so `fill_redirect_reply()` had +nothing to fill) was closed the same day via a local lookup in +`handle_ctrlhello()` against the statically-known `--peers` list. Together +these make a standby's `OP_upgradeConfig` rejection carry a real, populated +redirect hint, and `xceadmin.discovery.follow_redirects()` -- already +generic and already tested against mocks (`test_config_push_mock.py:: +test_push_config_follows_one_redirect_to_primary`) -- transparently +retries against the primary with zero client-side changes required. Shares binary discovery / port / stdout-parsing helpers with ``test_e2e_live_push.py`` via ``_live_e2e_common`` (extracted there as part @@ -68,10 +79,16 @@ that merging them would cost more readability than it would save. Skip convention: identical to task 6.2 -- an imperative ``pytest.skip`` inside the fixture (no controller binary / no ``openssl``), plus an in-test skip for a missing ``xceadmin`` binary, both matching this suite's -existing cert-dependent skip precedent (task 5.3). Skipping is the NORMAL -outcome under the fast ``xce-build.sh xceadmin`` / meson path, which runs -pytest from an isolated ``repo-mirror`` with no sibling -``xce.build``/``build.dbg``/``build.release`` controller binaries. +existing cert-dependent skip precedent (task 5.3). UPDATE (2026-08-28): +skipping is no longer the normal outcome under the ``xce-build.sh +xceadmin`` / meson path -- ``meson-build.sh`` now symlinks each profile's +``controller`` subdirectory (and ``src/tools/``, for ``deploy-ha.sh``) into +the isolated ``repo-mirror`` it runs pytest from, and sets ``XCEADMIN_BIN`` +to that build's own freshly-installed binary, so these tests find and +exercise a real controller/xceadmin whenever one exists anywhere in the +repo's ``xce.build``/``build.dbg``/``build.release`` dirs. Skipping now +correctly means what it always should have: no such binary was ever built +in this checkout. Process-leak fix (task 6.2's own finding, reused here for all 3 nodes): each controller is launched as the real ``xce_controller`` binary @@ -413,11 +430,36 @@ def _assert_no_decode_crash(push_result: subprocess.CompletedProcess) -> None: assert "Traceback" not in push_result.stderr, push_result.stderr -def _assert_primary_committed(log_text: str, version_tag_hex: str) -> None: +# config_activate_committed() (config_repl.c) runs off the WAL apply-pump +# dispatch (persist.c, the on_commit_advanced-adjacent seam bug-1469 added), +# asynchronously relative to the majority-ack-gated client reply +# (commitIndex/lastApplied are distinct steps -- controller/CLAUDE.md's TLA+ +# mapping) -- a client can legitimately see its push's "success" reply +# before the primary's OWN local apply of that same marker has logged +# "config activated...". Poll instead of a single-shot read to close this +# real race rather than assume an ordering the design never guarantees. +_ACTIVATION_POLL_TIMEOUT_S = 5.0 +_ACTIVATION_POLL_INTERVAL_S = 0.2 + + +def _assert_primary_committed(log_file: Path, version_tag_hex: str) -> str: + marker_line = f"config activated from committed marker tag={version_tag_hex}" + deadline = time.monotonic() + _ACTIVATION_POLL_TIMEOUT_S + log_text = log_file.read_text() + while marker_line not in log_text and time.monotonic() < deadline: + time.sleep(_ACTIVATION_POLL_INTERVAL_S) + log_text = log_file.read_text() + assert f"tag={version_tag_hex}" in log_text, log_text assert "Config change committed" in log_text assert f"Config upgraded to tag={version_tag_hex}" in log_text - assert f"config activated from committed marker tag={version_tag_hex}" in log_text + assert marker_line in log_text, ( + f"{marker_line!r} never appeared within {_ACTIVATION_POLL_TIMEOUT_S}s " + f"of the push replying success -- see this function's own comment " + f"for why activation is asynchronous relative to the reply. " + f"log_text={log_text!r}" + ) + return log_text def test_live_config_push_against_primary_reaches_success_path( @@ -464,8 +506,7 @@ def test_live_config_push_against_primary_reaches_success_path( assert push_result.returncode == 0, push_result.stderr version_tag_hex = f"{version_tag:016x}" - primary_log_text = live_ha_cluster.primary.log_file.read_text() - _assert_primary_committed(primary_log_text, version_tag_hex) + primary_log_text = _assert_primary_committed(live_ha_cluster.primary.log_file, version_tag_hex) # Majority-ACK barrier evidence on the primary's own log: at least one # standby genuinely acked this exact tag (push_config_to_standbys). @@ -494,33 +535,29 @@ def test_live_config_push_against_primary_reaches_success_path( assert standby_saw_it, [s.log_file.read_text() for s in live_ha_cluster.standbys] -# errnox.h: XCE_ERRNO_BASE=10001, E_XCE_STANDBY is the 14th entry (0-indexed -# 13) in that enum => 10001+13=10014. "Controller in CTRL_STANDBY; op is -# primary-only" -- exactly the rejection a standby's OP_upgradeConfig hits -# (module docstring). Named here instead of a bare magic number per this -# project's error-handling convention of surfacing XCE's own error -# vocabulary rather than an unexplained literal. -_E_XCE_STANDBY = 10014 - - -def test_live_config_push_against_standby_returns_standby_rejection_not_redirect( +def test_live_config_push_against_standby_redirects_and_succeeds( live_ha_cluster: HaCluster, tmp_path: Path ) -> None: """Pushing `config push` directly to a standby node against a REAL - controller today does NOT redirect to the primary -- it is flatly - rejected with ``E_XCE_STANDBY`` (module docstring: a pre-existing, - already-documented controller-side gap, `fill_redirect_reply()` never - wired for `OPCode_upgradeConfig`; out of this test-only task's - boundary to fix). This test live-verifies and regression-locks that - REAL current behavior: no redirect line, no success, and specifically - the `E_XCE_STANDBY` status rather than some other/unexpected failure - mode, so a future change to the rejection path itself would also be - caught here even before the redirect gap is closed. + controller now redirects to the primary and succeeds (bug-1700/1701, + fixed 2026-08-26 -- module docstring's "WHAT CLOSED IT" section): the + same success outcome as pushing straight to the primary + (`test_live_config_push_against_primary_reaches_success_path`), reached + via one transparent redirect hop instead of zero. + + Renamed from `test_live_config_push_against_standby_returns_standby_ + rejection_not_redirect` -- that test's own assertions predicted exactly + this outcome the moment the redirect gap closed (see module docstring + for the investigation history behind both the original gap and its + fix). Confirms via ``discovery.py``'s own stderr line (`follow_redirects`: - "redirected: host:port -> host:port") being ABSENT -- direct evidence - no redirect was attempted, not an inference from the push merely - failing for some other reason. + "redirected: host:port -> host:port") being PRESENT -- direct evidence + a redirect was actually followed, not an inference from the push merely + succeeding some other way (e.g. `standby_addr` accidentally being the + primary after an election reshuffle; the fixture's own settle window + exists precisely to close that race, see module docstring's + "Election-wait" section). """ xceadmin_bin = find_first_executable(XCEADMIN_CANDIDATES) if xceadmin_bin is None: @@ -542,33 +579,30 @@ def test_live_config_push_against_standby_returns_standby_rejection_not_redirect ) _assert_no_decode_crash(push_result) - # No redirect was attempted -- the standby's reply never carries - # redirectHost/redirectPort for this opcode (module docstring), so - # discovery.py's follow_redirects has nothing to follow and never - # prints its "redirected: ..." line. - unexpected_redirect_line = f"redirected: {standby_addr} -> {primary_addr}" - assert unexpected_redirect_line not in push_result.stderr, ( - "a redirect line appeared -- if the controller-side redirect gap " - "documented in this file's module docstring (design.md 'Boundary " - "Commitments' / 'Revalidation Triggers') has been closed, this " - "test is stale and should be replaced with a real success-path " - f"assertion: {push_result.stderr!r}" + # Positive evidence the redirect was actually followed -- the standby's + # reply now carries redirectHost/redirectPort (bug-1700/1701), so + # discovery.py's follow_redirects retries against the primary and prints + # this line before the retried push itself is attempted. + expected_redirect_line = f"redirected: {standby_addr} -> {primary_addr}" + assert expected_redirect_line in push_result.stderr, ( + f"expected follow_redirects() to redirect from the standby to the " + f"primary, got stderr={push_result.stderr!r}" ) match = VERSION_TAG_RE.search(push_result.stdout) assert match, f"no versionTag/ackedCount found in stdout: {push_result.stdout!r}" version_tag, acked_count = int(match.group(1)), int(match.group(2)) + assert version_tag > 0 - # Real, live-verified standby-contact behavior today: the reply is a - # bare gate rejection (versionTag=0 ackedCount=0), not a redirect or a - # partial success. - assert version_tag == 0 - assert acked_count == 0 - assert push_result.returncode == 1 - assert f"controller returned status -{_E_XCE_STANDBY}" in push_result.stderr, push_result.stderr + # Same success bar as the direct-to-primary test: a genuine positive + # ackedCount from real standby ACKs, exit 0 -- the redirect hop changes + # HOW the primary was reached, not what a successful push looks like. + assert acked_count > 0, ( + f"expected a positive ackedCount from real standby ACKs after the " + f"redirect, got {acked_count} (stdout={push_result.stdout!r} " + f"stderr={push_result.stderr!r})" + ) + assert push_result.returncode == 0, push_result.stderr - # The rejected push never reached the primary at all -- its log shows - # no upgrade activity from this attempt (independent confirmation this - # is a clean gate rejection, not a landed-then-lost push). - primary_log_text = live_ha_cluster.primary.log_file.read_text() - assert "Config marker failed to reach committed prefix" not in primary_log_text + version_tag_hex = f"{version_tag:016x}" + _assert_primary_committed(live_ha_cluster.primary.log_file, version_tag_hex)