docs(controller-peers-env-fallback): design re-validation fixes

Iterated design review until clean (GO):

- Controller main() calls mylog_fini() before every early return, so
  log lines written by tlog's background thread survive the exit.
- cluster_init_phase1() keeps its candidates_count > 0 guard around the
  new xce_peers_find_identity() call (standalone stays my_priority 0).
- xceadmin adds the "(from $VAR)" source note only to network errors,
  not to local cluster-bin file errors (OSError with filename set).
- Regression script: scratch --workdir/--certdir/--logfile per daemon,
  refuses to run if a local cluster is up (port 9990, worker socket),
  --sim case asserts the sim branch ran, 2.3 patches follow_redirects,
  2.8 malformed-entry cases use a readable cluster-bin file, wrapper
  test expects PEERS_STR form.
- Two existing scripts that start xce_scheduler without --peers will
  unset XCE_CONTROLLER_PEERS; --interactive wording corrected.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Rongsong Shen 2026-10-03 04:46:00 +00:00
parent bd1f5175a1
commit 6160ca299f
Signed by untrusted user: rshen
GPG key ID: 80533536B0EB651C
3 changed files with 52 additions and 16 deletions

View file

@ -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 <file>` and `XCE_CONTROLLER_PEERS` set: assert the
process never logs an "HA cluster config"/"No --peers flag" line at all
- scheduler with `--sim <nonexistent file>` 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

View file

@ -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.

View file

@ -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": {