diff --git a/.kiro/specs/controller-peers-env-fallback/design.md b/.kiro/specs/controller-peers-env-fallback/design.md index e741fbd2..b6c8fd24 100644 --- a/.kiro/specs/controller-peers-env-fallback/design.md +++ b/.kiro/specs/controller-peers-env-fallback/design.md @@ -79,8 +79,9 @@ Walked against `xce-audit-invariants`' controller and scheduler dimensions: startup code (`cluster_init_phase1()`, `cluster_open_listener()`), with no blocking added. - **Driver/simulation state-equivalence (scheduler) — not affected.** - `--sim`/`--interactive` return before the `--peers` block, so the - variable can never reach replay mode. + `--sim` (with or without `--interactive`) returns before the `--peers` + block, so the variable can never reach replay mode; `--interactive` + without `--sim` is a live run and takes the normal `--peers` path. ### Revalidation Triggers - `xce_peers_find_identity()` is now the only "which candidate is me" rule for the controller (the 1.6 check, `my_priority`, listen port). Any future identity rule (e.g. matching on host:port) changes there, never in a re-added inline loop. @@ -130,15 +131,16 @@ One new file (the regression script below); every other change is to an existing - `src/include/xce/cluster.h` — declare `const char *xce_controller_peers_env(void);` and `int xce_peers_find_identity(const xce_cluster_config_t *cfg, const char *name);` alongside the existing `xce_parse_peers()` declaration. - `src/lib/cluster.c` — implement `xce_controller_peers_env()`: read `getenv("XCE_CONTROLLER_PEERS")`, return it if non-empty, else `NULL`. Implement `xce_peers_find_identity()` with the matching rule moved verbatim from `src/controller/cluster.c` (identical at ~L1784-1791 and ~L1890-1900). - `src/lib/testsuite/test_cluster.c` — new unit tests for `xce_controller_peers_env()` (unset, empty, non-empty) and `xce_peers_find_identity()` (named match, bare-hostname match, named entry not matched by its hostname, no match → -1, first of duplicate names wins, NULL name → -1, empty name matching an unnamed empty-hostname entry exactly as today). -- `src/controller/main.c` — at the existing `if (arguments.peers_str)` site (~L455), resolve `arguments.peers_str` against `xce_controller_peers_env()` before the check, remembering in a local `peers_from_env` whether the env var supplied it; on a parse or even-count failure, name the source (flag vs. environment variable) in the existing error message; after parsing, when `peers_from_env` is true and `xce_peers_find_identity(&myconf.peers_cfg, arguments.myname) < 0`, print the 1.6 error and `return 1`; extend the "No --peers flag" `INFO` (~L486) per 1.8; add `XCE_CONTROLLER_PEERS` and the "unset it to run standalone" note (1.7) to the `--peers` option's help text (~L85-87). -- `src/controller/cluster.c` — replace both inline identity loops with `xce_peers_find_identity(pcfg, c->myconf->name)`: in `cluster_init_phase1()` (~L1784-1791), `my_priority = idx >= 0 ? idx : pcfg->candidates_count - 1;`; in `cluster_open_listener()` (~L1890-1906), `matched = idx >= 0; my_port = matched ? pcfg->candidates[idx].port : 0;` followed by the existing refusal and default-port code. Same result for every input. +- `src/controller/main.c` — at the existing `if (arguments.peers_str)` site (~L455), resolve `arguments.peers_str` against `xce_controller_peers_env()` before the check, remembering in a local `peers_from_env` whether the env var supplied it; on a parse or even-count failure, name the source (flag vs. environment variable) in the existing error message; after parsing, when `peers_from_env` is true and `xce_peers_find_identity(&myconf.peers_cfg, arguments.myname) < 0`, print the 1.6 error and `return 1`; extend the "No --peers flag" `INFO` (~L486) per 1.8; call `mylog_fini()` before every early `return` after `mylog_init()` (~L393) — the `get_myname()` failure (~L403), the `mylock_init()` failure (~L452), the `--peers` parse and even-count errors, the new 1.6 check, and the cert pre-flight (~L489-496); it is a no-op before `mylog_init()` and safe to call twice — because tlog writes the log file from a background thread with no `atexit` flush, so today those returns can lose already-logged lines (the scheduler already drains explicitly for the same reason); add `XCE_CONTROLLER_PEERS` and the "unset it to run standalone" note (1.7) to the `--peers` option's help text (~L85-87). +- `src/controller/cluster.c` — replace both inline identity loops with `xce_peers_find_identity(pcfg, c->myconf->name)`: in `cluster_init_phase1()` (~L1784-1791), keep the existing `if (pcfg->candidates_count > 0)` guard (~L1782; standalone mode leaves `my_priority = 0`) and replace only the loop inside it with `my_priority = idx >= 0 ? idx : pcfg->candidates_count - 1;`; in `cluster_open_listener()` (~L1890-1906), `matched = idx >= 0; my_port = matched ? pcfg->candidates[idx].port : 0;` followed by the existing refusal and default-port code. Same result for every input. - `src/tools/deploy-ha.sh` — in the generated `scripts/xceadmin` wrapper heredoc (~L689-694), add `export XCE_CONTROLLER_PEERS="$PEERS_STR"` next to the existing `XCEADMIN_CONTROLLER` export, and extend the comment above it to say why (Requirement 4). -- `src/scheduler/src/main.c` — identical shape at the existing `if (arguments.peers)` site (~L411), without the identity check; name the source in both the parse-error and the `xce_cluster_config_validate()` failure messages (~L412-424, 1.4); "No --peers flag" `INFO` (~L435) per 1.8; help text update with 1.7's note (~L88). No change anywhere in the `--sim`/`--interactive` path, which already returns before this point. +- `src/scheduler/src/main.c` — identical shape at the existing `if (arguments.peers)` site (~L411), without the identity check; name the source in both the parse-error and the `xce_cluster_config_validate()` failure messages (~L412-424, 1.4); "No --peers flag" `INFO` (~L435) per 1.8; help text update with 1.7's note (~L88). No change anywhere in the `--sim` path, which already returns before this point. - `src/worker/src/main.c` — identical shape at the existing `if (peers_str)` site (~L470), without the identity check; name the source in both the parse-error and the `xce_cluster_config_validate()` failure messages (~L472-485, 1.4); "No --peers flag" log line (~L501) per 1.8; update the `-h`/usage `printf` (~L443-446) to mention `XCE_CONTROLLER_PEERS`, 1.7's note, and that `sudo` drops it unless preserved. - `src/harness/test.c` — at startup, before the REPL loop and before rc-file loading, compute the `"masters"` entry's `defval` from `xce_controller_peers_env()` if set, else leave it at today's literal `"localhost:9990"`; extend the `USAGE`/`set`-command help text (~L403-415) to mention the new variable and 1.7's note. - `src/xceadmin/src/xceadmin/discovery.py` — extend `resolve_controllers()`'s per-entry parsing to strip an optional `name=` prefix (mirrors `xce_parse_peers()`) before its existing `rsplit(":", 1)`; turn today's uncaught `ValueError` from `int(port_str)` (~L78) into a new `ControllerValueError` naming the bad entry (2.8). - `src/xceadmin/src/xcecommon/errors.py` — add `ControllerValueError(XceAdminError)` next to the existing controller-reachability errors (`ConnectionTimeoutError`, `TooManyRedirectsError`, `NoPrimaryError`). -- `src/xceadmin/src/xceadmin/cli.py` — change `_CONTROLLER_ENV_VAR` (~L109), which all 4 `--controller` options use, to the list `["XCE_CONTROLLER_PEERS", "XCEADMIN_CONTROLLER"]`; rewrite each option's help string, which today interpolates `_CONTROLLER_ENV_VAR` with an f-string and would print a raw Python list once it becomes one, and the precedence comment above it (~L102-108). Add a helper `_controller_source_note()` that returns `" (from $NAME)"` when `click.get_current_context().get_parameter_source("controller")` is `ParameterSource.ENVIRONMENT` (NAME = the first non-empty variable in the list, the same rule click applies) and `""` otherwise; append it to the two existing `cannot connect to {controller}` messages (~L172, ~L265), and, in the existing `except XceAdminError` handlers of the `--controller`-accepting commands, to the message when the error is one of `_CONTROLLER_ERRORS = (ControllerValueError, ConnectionTimeoutError, TooManyRedirectsError, NoPrimaryError)` (2.8). Errors unrelated to the controller value (e.g. `ClusterBinFileError`) keep their text unchanged. +- `src/xceadmin/src/xceadmin/cli.py` — change `_CONTROLLER_ENV_VAR` (~L109), which all 4 `--controller` options use, to the list `["XCE_CONTROLLER_PEERS", "XCEADMIN_CONTROLLER"]`; rewrite each option's help string, which today interpolates `_CONTROLLER_ENV_VAR` with an f-string and would print a raw Python list once it becomes one, and the precedence comment above it (~L102-108). Add a helper `_controller_source_note()` that returns `" (from $NAME)"` when `click.get_current_context().get_parameter_source("controller")` is `ParameterSource.ENVIRONMENT` (NAME = the first non-empty variable in the list, the same rule click applies) and `""` otherwise; append it to the two existing `cannot connect to {controller}` messages (~L172, ~L265) only when the `OSError` is not a local file error (`e.filename is None`: socket errors carry no filename, while `read_bytes()` of a missing or unreadable cluster-bin file in `push_config()`/`probe_config()` (`push.py` ~L89, ~L143) raises a `FileNotFoundError`/`PermissionError` with `filename` set, which reaches the same `except OSError` branch today and must not be blamed on the variable), and, in the existing `except XceAdminError` handlers of the `--controller`-accepting commands, to the message when the error is one of `_CONTROLLER_ERRORS = (ControllerValueError, ConnectionTimeoutError, TooManyRedirectsError, NoPrimaryError)` (2.8). Errors unrelated to the controller value (e.g. a missing cluster-bin file, or `ClusterBinFileError` from `config build`) keep their text unchanged. +- `src/tools/test-version-startup-consistency.sh`, `src/tools/test-dirty-fingerprint-startup-log-unchanged.sh` — add `unset XCE_CONTROLLER_PEERS` near the top, so their scheduler run keeps taking the no-peers path they were written for (see Testing Strategy, ambient-variable exposure). - `src/xceadmin/tests/test_discovery.py` — new unit tests for `resolve_controllers()`'s `name=`-stripping and its `ControllerValueError` on a non-numeric port. - `src/xceadmin/tests/test_cli.py` — new unit tests for the new env-var precedence (mirrors the existing `test_controller_option_falls_back_to_xceadmin_controller_env_var`/`test_controller_flag_overrides_xceadmin_controller_env_var` tests) and for 2.8's messages. - `.kiro/steering/build-deployment.md` — document `XCE_CONTROLLER_PEERS`: components, value format, precedence, the "unset it to run standalone" opt-out, the controller's own-name rule (1.6), and that `sudo` (default `env_reset`) and systemd services do not inherit it (`sudo --preserve-env=XCE_CONTROLLER_PEERS`, or `Environment=` in the unit). @@ -263,7 +265,7 @@ int xce_peers_find_identity(const xce_cluster_config_t *cfg, const char *name); ``` - Preconditions: `cfg` non-NULL. - Postconditions: returns the index (0..`candidates_count`-1) of the first matching candidate, or -1 when `name` is NULL or nothing matches. An empty `name` is compared like any other (today's loops do the same: it matches an unnamed entry with an empty hostname, e.g. from `:5`), so the invariant below holds for every input. -- Invariants: for every input, `idx >= 0 ? idx : candidates_count - 1` equals what `cluster.c` computes for `my_priority` today, and `idx >= 0` equals the listener's `matched` today. +- Invariants: for every input with `candidates_count > 0`, `idx >= 0 ? idx : candidates_count - 1` equals what `cluster.c` computes for `my_priority` today (with no candidates, the call site's existing guard keeps `my_priority = 0`, and the helper returns -1); and for every input, `idx >= 0` equals the listener's `matched` today. ### xceadmin @@ -323,10 +325,11 @@ No new logging/metrics infrastructure; each component's existing startup log lin - `resolve_controllers()` extension (`src/xceadmin/tests/test_discovery.py`): single `name=host:port`; mixed bare/named multi-candidate list; existing bare-host cases still pass unchanged. (2.2) - xceadmin `--controller` option precedence (`src/xceadmin/tests/test_cli.py`): `XCE_CONTROLLER_PEERS` set alone; both `XCE_CONTROLLER_PEERS` and `XCEADMIN_CONTROLLER` set (new wins); explicit `--controller` overrides both; neither set (existing default). (2.1, 2.4-2.6) For 2.3: with `XCE_CONTROLLER_PEERS="a=127.0.0.1:1,b=127.0.0.1:2,127.0.0.1:3"` and no -`--controller`, mock the send function under `follow_redirects_any`, make -the first candidate raise a connection error, and assert the candidates -arrive in env order with `name=` stripped and that the second one is -tried next. (2.3) For 2.8: an unreachable value from each of `XCE_CONTROLLER_PEERS`, `XCEADMIN_CONTROLLER` and an explicit `--controller` (message names the variable in the first two only), and a non-numeric port giving a clean error rather than a traceback. Run the unreachable cases through `config push` with a real, readable cluster-bin file (`push_config()` reads it before resolving controllers, so a missing file raises `ClusterBinFileError` first), or patch out the retry sleep, since the `scheduler record` commands retry `NoPrimaryError` 8 times with a 2 s pause (`scheduler.py` ~L131-132). All existing `test_cli.py` cases that assert `XCEADMIN_CONTROLLER`-only or no-env-var behavior (e.g. `test_controller_option_falls_back_to_xceadmin_controller_env_var`) must explicitly `monkeypatch.delenv("XCE_CONTROLLER_PEERS", raising=False)`, so the new variable's introduction cannot make them order-dependent on ambient environment state. +`--controller`, patch `follow_redirects` (the per-candidate call inside +`follow_redirects_any`) so the first candidate raises a connection error +and the second returns a canned reply, and assert the candidates arrive +in env order with `name=` stripped and that the second one is tried next. +(2.3) For 2.8: an unreachable value from each of `XCE_CONTROLLER_PEERS`, `XCEADMIN_CONTROLLER` and an explicit `--controller` (message names the variable in the first two only), and a non-numeric port giving a clean error rather than a traceback — run with a readable cluster-bin file too, since `config push` reads the file before resolving controllers: once as an explicit `--controller a=h:x` (message quotes the entry, no source note) and once as `XCE_CONTROLLER_PEERS=a=h:x` with no `--controller` (message quotes the entry and ends `(from $XCE_CONTROLLER_PEERS)`). Run the unreachable cases through `config push` with a real, readable cluster-bin file (`push_config()` reads it before resolving controllers, so a missing file raises `FileNotFoundError` first — add one case asserting that error gets no source note even with `XCE_CONTROLLER_PEERS` set), or patch out the retry sleep, since the `scheduler record` commands retry `NoPrimaryError` 8 times with a 2 s pause (`scheduler.py` ~L131-132). All existing `test_cli.py` cases that assert `XCEADMIN_CONTROLLER`-only or no-env-var behavior (e.g. `test_controller_option_falls_back_to_xceadmin_controller_env_var`) must explicitly `monkeypatch.delenv("XCE_CONTROLLER_PEERS", raising=False)`, so the new variable's introduction cannot make them order-dependent on ambient environment state. ### Scripted Regression Verification None of controller/scheduler/worker/harness's `main()`-level argument or @@ -351,6 +354,23 @@ controller and scheduler set up their log files earlier (controller candidate count and pass `--myname` naming a candidate unless the case is about the 1.6 check, so the even-count rule and 1.6 do not mask what the case is testing. +Every daemon run is isolated in a fresh scratch directory: the controller +always gets `--workdir`, `--certdir` and `--logfile` there (otherwise it +defaults to `/tmp/xce` and `/tmp/xce_controller.log`, `src/include/config.h:22`, +`main.c:312`, and could touch a local cluster's state), the scheduler +`--logfile`, and the worker `-l`. With an empty certificate directory the +controller exits at the certificate pre-flight (`main.c` ~L489), right +after logging the line under test (~L476/~L486) and after the 1.6 check, +so it never reaches `myapp_run()`; the `mylog_fini()` added before that +return (File Structure Plan, `main.c`) is what guarantees the line is in +the log file when the script reads it. Scheduler and worker runs are killed +after a short timeout once the asserted line has appeared. Two things a +scratch directory cannot isolate: with no peers list the scheduler and +worker still try their default controller (`localhost:9990`), and the +worker always uses the hardcoded `/var/run/.xcecd.sock`. The script +therefore checks first that nothing listens on port 9990 and that the +socket does not exist, and fails with a message (rather than running) +when a local cluster is up. - controller/scheduler/worker: with `XCE_CONTROLLER_PEERS` set (e.g. `a=localhost:1,b=localhost:2,c=localhost:3`; controller also `--myname a`) and no `--peers`, start briefly and grep the log (worker: @@ -367,8 +387,11 @@ the case is testing. worker's "single-controller path", on stdout) log line now also says `XCE_CONTROLLER_PEERS` was unset or empty, and that behavior is otherwise unchanged. (1.3, 1.8) -- scheduler with `--sim ` and `XCE_CONTROLLER_PEERS` set: assert the - process never logs an "HA cluster config"/"No --peers flag" line at all +- scheduler with `--sim ` and `XCE_CONTROLLER_PEERS` + set: assert stderr shows `xce_scheduler: --sim startup failed`, which only + the `--sim` branch prints (proving that branch ran, so the case cannot + pass by failing earlier), and that the process never logs an + "HA cluster config"/"No --peers flag" line at all (the `--sim` path returns before that code is reached — this is a by-construction guarantee already, not new logic; the script asserts it stays true). (Non-Goals cross-check) @@ -416,10 +439,23 @@ the case is testing. `$TOP/scripts/xceadmin` with its final `exec` line replaced by `printenv XCE_CONTROLLER_PEERS XCEADMIN_CONTROLLER`, and run the copy with `XCE_CONTROLLER_PEERS=bogus:1` in the caller's environment; assert - it prints the deployment's own `--peers` list and first candidate. If + it prints the deployment's own list in `deploy-ha.sh`'s `PEERS_STR` form (`ctrl-1=host:port,ctrl-2=...`, not the raw peers argument) and its first candidate (`PEER_ARRAY[0]`, bare `host:port`). If the xceadmin wheel is missing (so no wrapper is generated), this case fails with a message naming `/xce-build xceadmin`, rather than being skipped. (4.1, 4.2) +- Ambient-variable exposure of existing tests: the live xceadmin tests + and the `xce-test-*` skill scripts pass `--peers`, `--controller` or + `set masters` explicitly (e.g. `test_e2e_live_ha_push.py` ~L361/~L488), + so a developer's `XCE_CONTROLLER_PEERS` cannot change them (1.1). Two + existing scripts start `xce_scheduler` with only `--logfile`: + `src/tools/test-version-startup-consistency.sh` (~L302) and + `src/tools/test-dirty-fingerprint-startup-log-unchanged.sh`. With the + variable set, their scheduler takes the HA path instead of the + single-controller path; they still pass, because they match the + startup line by its version text, which the "HA cluster config" line + also carries. Implementation adds `unset XCE_CONTROLLER_PEERS` at the top + of both so they keep testing the path they were written for. Any new + test that relies on the no-value default must do the same. - xceadmin (`test_cli.py`): with `XCE_CONTROLLER_PEERS` and `XCEADMIN_CONTROLLER` both set and an explicit `--controller`, the flag wins. (4.3; this runs the CLI directly rather than through the diff --git a/.kiro/specs/controller-peers-env-fallback/research.md b/.kiro/specs/controller-peers-env-fallback/research.md index ea076134..e963dfd1 100644 --- a/.kiro/specs/controller-peers-env-fallback/research.md +++ b/.kiro/specs/controller-peers-env-fallback/research.md @@ -15,7 +15,7 @@ - **Sources Consulted**: `src/controller/main.c` (~L403-429), `src/scheduler/src/main.c` (~L380-429), `src/worker/src/main.c` (~L389-475), buglog.json bug-1457. - **Findings**: - controller: `if (arguments.peers_str) {...}` — omitting it is controller's own supported single-node/standalone primary path (bug-1457 fixed a startup-ordering defect in this exact path in 2026-07; the path itself is intentional, not an error). - - scheduler: `if (arguments.peers) {...} else { INFO("No --peers flag; using single-controller path, ...") }` — same shape, explicitly logged as intentional. Critically, scheduler's `--sim` mode `return`s **before** this block is ever reached (`main.c` ~L380-400) — an env-var-supplied peers default therefore cannot affect `--sim`/`--interactive` replay mode at all, by construction, with no special-casing needed. + - scheduler: `if (arguments.peers) {...} else { INFO("No --peers flag; using single-controller path, ...") }` — same shape, explicitly logged as intentional. Critically, scheduler's `--sim` mode `return`s **before** this block is ever reached (`main.c` ~L380-400) — an env-var-supplied peers default therefore cannot affect `--sim` replay mode (with or without `--interactive`) at all, by construction, with no special-casing needed. - worker: `if (peers_str) {...}` — identical shape. - All three: an invalid `--peers` value is a hard parse-error (`xce_parse_peers() != 0` → `fprintf(stderr, ...); return 1;`), never a silent fallback. - **Implications**: The env-var fallback only needs to be consulted at the single point each component already gates on `peers_str`/`peers` being non-NULL — no new mode-interaction risk exists, and `--sim` is unaffected without any extra guard. diff --git a/.kiro/specs/controller-peers-env-fallback/spec.json b/.kiro/specs/controller-peers-env-fallback/spec.json index 5f153105..fddc2acf 100644 --- a/.kiro/specs/controller-peers-env-fallback/spec.json +++ b/.kiro/specs/controller-peers-env-fallback/spec.json @@ -1,7 +1,7 @@ { "feature_name": "controller-peers-env-fallback", "created_at": "2026-09-22T07:37:10Z", - "updated_at": "2026-10-03T04:05:28Z", + "updated_at": "2026-10-03T04:45:21Z", "language": "en", "phase": "design-generated", "approvals": {