fleet/fleetd
3
0
Fork 0
Code Issues 73 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

fleetd#326: classify primary and configReload as deferred top-level keys #328

Closed
agent wants to merge 0 commits from worker/fix-326-50506e-15 into main
pull from: worker/fix-326-50506e-15
merge into: fleet:main
Conversation 0 Commits - Files Changed -
agent commented 2026-09-04 09:43:57 +02:00
Member
Copy Link
Copy Source

What changed

Two top-level FleetConfig keys were missing from ConfigRef.changedDeferredKeys, so a reload that
changed only one of them reported a bare config reloaded while the daemon kept the old value:

  • primary — Fleetd.java:506, 519, 520 read cfg.primary() only off the startup snapshot to
    build PrimaryRegistry (pinned terminal) and size ReplyPushLoop's reminder cap/backoff. Neither
    is rebuilt on reload.
  • configReload — Fleetd.java:679-680 read it only at startup to decide whether to build a
    ConfigWatcher at all, and with what interval. The watcher that would apply a later change is
    itself built once.

Both are now added to changedDeferredKeys and to the class doc's deferred list, each proven with a
failing-first test in ConfigRefTest (changingPrimaryIsReportedAsDeferred,
changingConfigReloadIsReportedAsDeferred) and a revert/restore mutation check.

health and coordinator are deliberately left unclassified — see the options writeup below.
ConfigRefProfileCoverageTest's exclusion-set assertion (Set.of("weight", "maxLoad", "credentialId")) was not touched.

configReload: deferred, not cold — reasoning

The class doc defines cold as a key whose new value would leave the daemon inconsistent with an
already-open resource (bind, herdrSocket, broker, auth — a bound socket, an open connection,
an already-listening port). configReload has no such resource. Fleetd.java:679-680 only decides,
once, whether to construct a ConfigWatcher and what interval to give it:

if (cfg.configReload() != null && cfg.configReload().isEnabled()) {
    configWatcher = new ConfigWatcher(config, cfg.configReload().intervalSeconds());
    configWatcher.start();
} else {
    configWatcher = null;
}

If a reload changes enabled or intervalSeconds, nothing goes inconsistent — a running watcher (if
one exists) just keeps polling at its original interval and ignores the new enabled flag, and a
daemon with no watcher stays without one. That is exactly the deferred shape already used for
lifecycle, guard, etc.: "accepted into the new snapshot, but the wiring built at startup keeps
the old value until a restart." So configReload → deferred.

(The "obvious joke" the issue names — turning reload off through a reload — is specifically why this
needed a decision rather than a one-line copy: refusing the reload as cold would be wrong, since
nothing breaks; reporting nothing at all is the bug being fixed.)

health / coordinator: options, not a fix (per the issue — I decide nothing here)

Both are read twice, off two different things, and no single bucket is correct for either.

health

  • Fleetd.java:556-563 — startup snapshot. Decides whether FleetHealthMonitor is built at all and
    its intervalOrDefault()/workingSuspectAfterOrDefault() — frozen at startup.
  • Fleetd.java:648-650 (FleetMcp.HealthCoverageSource) — live, via config.get().health(), on
    every fleet_profiles call. Feeds the reported coverage string (detection-only vs the full
    string), including whether notifications is configured.

coordinator

  • Fleetd.java:502 (openLeadMailbox) — startup snapshot. Resolves coordinator.effectiveUri()
    (honoring uriEnv over uri) and opens the LeadMailbox once with that URI, plus selfId and
    prefetch. All four sub-fields (uri, uriEnv, selfId, prefetch) are consumed only here for
    this purpose — frozen.
  • MemberEnvAllowList.java:165, reached from HerdrPeerLauncher.java:1529-1530 — live, via
    config.get() on every spawn. Reads coordinator.uriEnv() only (not uri/selfId/
    prefetch) to build the exclusion set that keeps the broker URI env var name out of a member's
    allow-list/scrub.

So coordinator.uriEnv specifically is the split field (like health.enabled); coordinator.uri,
selfId, prefetch are cleanly deferred-only (only the startup snapshot ever reads them).

Options, with costs

  1. Report the whole key as deferred whenever anything under it changes. Cheapest: one
    Objects.equals(old.health(), fresh.health()) / ...coordinator()... check, same shape as
    lifecycle/guard. Cost: honest for the frozen half, wrong for the live half — changing only
    health.notifications.mode (read live at :648-650) or only coordinator.prefetch would tell the
    operator a restart is needed when nothing needs to restart. This is exactly the kind of
    over-claim the ticket is trying to get away from, just in the safer direction.

  2. Split by sub-field. health.enabled / intervalSeconds / workingSuspectAfterSeconds →
    deferred (frozen into the monitor); health.notifications → also touches the live coverage
    string, so it's arguably hot-for-reporting-purposes but its value is still read live either way
    — no restart needed. coordinator.uri / selfId / prefetch → deferred; coordinator.uriEnv →
    split (deferred for the mailbox's actual connection, hot for the spawn-time exclusion list).
    Cost: more machinery — changedDeferredKeys currently compares whole nested records with one
    Objects.equals; sub-field comparison means unpacking each nested record's fields by hand (or
    reflection) and picking a bucket per field, including one field (coordinator.uriEnv) that is
    correctly in both buckets. The issue's own text names the open question: what happens when both
    the deferred half and the hot half of the same key change in one reload? Does Outcome.deferred()
    name "coordinator.uri" while staying silent on "coordinator.uriEnv" even though uriEnv
    changing genuinely needs a restart for the mailbox to move? That needs an explicit rule, and I
    don't think there's a "safe default" here — it has to be decided, not defaulted.

  3. Make the frozen reader live. For health: reconstruct/reconfigure FleetHealthMonitor
    (interval, enabled) on a running daemon — means starting/stopping its own scheduler and thread
    without racing the ordered shutdown hook or an in-flight health check. For coordinator: rebuild
    a running LeadMailbox — close the existing AMQP consumer/connection and open a new one with the
    new URI/selfId/prefetch on a live daemon. This is the one I want to flag as possibly unsafe, not
    just expensive:     selfId names this daemon's own inbox queue (lead.<selfId>.inbox) — a peer
    lead that already learned the old coord-id would not automatically discover the new one, so
    changing coordinator.selfId live is a distributed-identity change, not just a reconnect. I did
    not find anything in LeadMailbox/LeadCoordLoop that handles a self-id changing under a running
    daemon (nor should this ticket add it — invariant 1 forbids making anything take effect live
    here). Biggest change, and for coordinator specifically I'd want that identity question answered
    before calling it safe.

I'm not picking one — that's the point of this section.

The coordinator.uriEnv question — what I actually found

Question: if coordinator.uriEnv changes in a reload, can the broker URI variable end up visible
to a member spawned after that reload?

What I read: MemberEnvAllowList.brokerUriEnvNames(FleetConfig) (MemberEnvAllowList.java:159- 167) reads config.coordinator() (and config.broker()) directly off whatever FleetConfig it's
handed. Both call sites hand it the live config:

  • HerdrPeerLauncher.brokerUriEnvNames() (HerdrPeerLauncher.java:1529-1530) calls
    MemberEnvAllowList.brokerUriEnvNames(config.get()) — config is the Supplier<FleetConfig>
    (ConfigRef), so this re-reads on every spawn.
  • That set is used two ways, one per memberCredentials.policy:
    • allow-list (derivedAllowedNames, HerdrPeerLauncher.java:1517-1527): the set is
      subtracted from the derived allow-list twice — once inside MemberEnvAllowList.derive(..., excludedNames) (removes it from the profile/allow:-derived union) and again explicitly
      (allowed.removeAll(brokerUriEnvNames), line 1525) after launch.env()'s own keys are unioned
      in. The scrub then blanks anything not in this final allowed set, running after the
      pane's login shell has sourced everything (EnvAllowListScrub, post-shell).
    • deny-by-default (overlayBlockedCredentials, HerdrPeerLauncher.java:1317-1325): the set
      is unioned into blocked and a sentinel value is written over each blocked name in the
      pane-creation env map only — a pre-shell overlay, per MemberEnvAllowList's own class doc
      (lines 149-157): "Under the deny-list policy there is no scrub: the name is only removed from
      the pre-shell env map, and a login shell that sources the operator's secret store re-exports it.
      ... deny-list deployments do NOT get this guarantee."

So: after a reload changes coordinator.uriEnv from (say) OLD_NAME to NEW_NAME, the very next
spawn's exclusion set is {NEW_NAME, ...} — OLD_NAME drops out of it immediately, while the
already-open LeadMailbox keeps using whatever host variable OLD_NAME named (it was never
rebuilt). Whether that makes the value visible to a newly-spawned member depends on the policy:

  • Under deny-by-default: the protection this exclusion buys was already documented as weak
    regardless of reload — a login shell that re-sources the operator's secret store (which is where
    OLD_NAME's value would live in the first place, per "Central secret store" / "Login shell beats
    launcher env") overwrites the pre-shell sentinel outright. So under this policy the reload-vs-
    mailbox desync doesn't introduce a new hole on top of the one the class doc already names — the
    exclusion's protection for either name was never guaranteed to survive a login shell under this
    policy.
  • Under allow-list: the post-shell scrub is the real control, and it defaults to deny
    everything not explicitly derived. OLD_NAME was never added to the allow set by the exclusion
    mechanism — the exclusion only ever removes it from an allow set some other source put it in
    (a profile's env: map, or the operator's own memberCredentials.allow: list — see
    MemberEnvAllowList's class doc, "even when the operator lists them under allow:, they are
    excluded here"). So in the default case (OLD_NAME is not independently allow-listed elsewhere),
    losing the exclusion changes nothing: OLD_NAME still isn't in the derived allow set, and the
    scrub still blanks it.
    The narrow case where it does matter: if OLD_NAME is also present in a profile's env:
    map (frozen at startup, from HerdrPeerLauncher's own Map.copyOf(profiles)) or in the operator's
    memberCredentials.allow: list (read live, so this can even be added in the same reload) — i.e.
    exactly the coincidence the exclusion mechanism exists to guard against — then after the reload,
    OLD_NAME re-enters the derived allow set with nothing left to strip it back out, the scrub keeps
    it, and if the operator's shell also still exports OLD_NAME with the mailbox's real (still-live)
    AMQP URI, a member spawned after the reload keeps that value in its environment.

So: yes, it can, but only in that narrow, config-dependent case — an operator who has (or adds,
in the same reload) OLD_NAME to memberCredentials.allow: or a profile's env: map. In the
default configuration (nothing else references that variable name), no — the general deny-by-default
allow-set logic already excludes it independent of the coordinator-specific safety net. I read
MemberEnvAllowList.java in full and the two call sites named above; I did not write or run anything
that exercises the exposure path, per the issue's instruction, and I didn't touch
MemberEnvAllowList — invariant 2.

Is a top-level reflection coverage test (the #323 mechanism) worth building here?

No, not now — and not a blind copy. ConfigRefProfileCoverageTest works because Profile is a
flat record of scalars: it mutates one component at a time via reflection and asserts a single
boolean method (sameLaunchSettings) notices, then pins the exclusion set with a reason. Two things
break that shape at the top level:

  1. FleetConfig's components are nested records with their own defaulting, several deeply
    (Fleet alone holds leaders: Map<String, Leader>, charters, developers/reviewers/
    architects, tabLabel). Building valid "base" and "alt" instances for every nested record by
    reflection (as baseValues()/altValues() do for Profile) is real authoring work, not a
    mechanical extension — Fleet and MemberCredentials are not "one more scalar row" the way
    Profile's fields are.
  2. The correctness claim a checker would need to make is external to the record's shape. For
    Profile, "is this component compared or excluded" is a closed question the method itself
    answers — reflection can observe it directly. For FleetConfig, "is this key genuinely hot" means
    "is it read through config.get() at every point of use in Fleetd.java/HerdrPeerLauncher.java
    /etc.", which is a fact about other files, invisible to reflection over the config record. A
    checker built this way could only assert "this key is explicitly named somewhere (cold, deferred,
    or a hot-exclusion set with a citation)" — it cannot verify the citation is true, the same gap
    LAUNCH_SETTINGS_EXCLUDED already has (a human still has to read the wiring to trust the excuse).
  3. health/coordinator don't fit a three-way partition at all. A reflection mutate-and-assert
    loop needs each component to land in exactly one bucket (cold / deferred / hot-excluded). These
    two need a fourth "split — see doc" bucket the #323 mechanism never needed, and building that
    bucket well is exactly the open design question in the options section above, not something a
    generic coverage test can settle.

What such a checker WOULD still catch, if built: a new top-level FleetConfig component added
later with no explicit triage anywhere — the literal shape of both #323 and this issue. That's real,
recurring value (two real instances so far). What it would NOT catch: a wrongly-justified
exclusion (an "it's read live" comment that isn't true), or a wrong verdict on a split key — both
still require a human reading the wiring, same as today.

Given the authoring cost (especially Fleet) and that the two genuinely dangerous gaps here
(health, coordinator) can't be resolved by this mechanism until their split classification is
decided, I'd hold off building it now. If/when health/coordinator get a final classification, a
narrower version — enumerate top-level components, require each to be named in COLD_KEYS, an
explicit deferred-key name list (changedDeferredKeys doesn't currently have one — it's all inline
ifs), or a documented hot/split exclusion set — becomes a much smaller, well-defined change. Both
answers were explicitly fine per the issue; this is mine, and I'm open to being told to build it
anyway.

Build

cd fleetd && mvn clean install

Full run: Tests run: 1341, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS (45.575 s).
ConfigRefTest alone: Tests run: 19, Failures: 0, Errors: 0, Skipped: 0.

Mutation proof

primary — removed the primary check from changedDeferredKeys, ran only the new test:

[ERROR] dev.ltms.fleet.config.ConfigRefTest.changingPrimaryIsReportedAsDeferred(Path) -- Time elapsed: 0.281 s <<< FAILURE!
org.opentest4j.AssertionFailedError: expected: <[primary]> but was: <[]>
	at dev.ltms.fleet.config.ConfigRefTest.changingPrimaryIsReportedAsDeferred(ConfigRefTest.java:532)
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0
[INFO] BUILD FAILURE

Restored, re-ran full ConfigRefTest green.

configReload — removed the configReload check, ran only the new test:

[ERROR] dev.ltms.fleet.config.ConfigRefTest.changingConfigReloadIsReportedAsDeferred(Path) -- Time elapsed: 0.282 s <<< FAILURE!
org.opentest4j.AssertionFailedError: expected: <[configReload]> but was: <[]>
	at dev.ltms.fleet.config.ConfigRefTest.changingConfigReloadIsReportedAsDeferred(ConfigRefTest.java:566)
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0
[INFO] BUILD FAILURE

Restored, re-ran full ConfigRefTest green (19/19), then full mvn clean install (1341/1341) as
quoted above.

Caveats for review

  • health and coordinator are untouched by design — this PR does not resolve them, per the issue.
  • No top-level reflection coverage test was added — see the verdict section above.
  • fleetd/fleetd.yaml is gitignored and absent from my worktree; nothing here depends on or reports
    its contents.
  • Out of scope, not investigated: whether deny-by-default's pre-shell-only protection for
    coordinator/broker URI env names (already documented as bypassable by a login shell) should be
    strengthened — that's the existing, separately-documented deny-list weakness, not something this
    ticket's reload-classification scope covers.
## What changed Two top-level `FleetConfig` keys were missing from `ConfigRef.changedDeferredKeys`, so a reload that changed only one of them reported a bare `config reloaded` while the daemon kept the old value: - **`primary`** — `Fleetd.java:506, 519, 520` read `cfg.primary()` only off the startup snapshot to build `PrimaryRegistry` (pinned terminal) and size `ReplyPushLoop`'s reminder cap/backoff. Neither is rebuilt on reload. - **`configReload`** — `Fleetd.java:679-680` read it only at startup to decide whether to build a `ConfigWatcher` at all, and with what interval. The watcher that would apply a later change is itself built once. Both are now added to `changedDeferredKeys` and to the class doc's deferred list, each proven with a failing-first test in `ConfigRefTest` (`changingPrimaryIsReportedAsDeferred`, `changingConfigReloadIsReportedAsDeferred`) and a revert/restore mutation check. `health` and `coordinator` are **deliberately left unclassified** — see the options writeup below. `ConfigRefProfileCoverageTest`'s exclusion-set assertion (`Set.of("weight", "maxLoad", "credentialId")`) was not touched. ## `configReload`: deferred, not cold — reasoning The class doc defines **cold** as a key whose new value would leave the daemon inconsistent with an already-open resource (`bind`, `herdrSocket`, `broker`, `auth` — a bound socket, an open connection, an already-listening port). `configReload` has no such resource. `Fleetd.java:679-680` only decides, once, whether to construct a `ConfigWatcher` and what interval to give it: ```java if (cfg.configReload() != null && cfg.configReload().isEnabled()) { configWatcher = new ConfigWatcher(config, cfg.configReload().intervalSeconds()); configWatcher.start(); } else { configWatcher = null; } ``` If a reload changes `enabled` or `intervalSeconds`, nothing goes inconsistent — a running watcher (if one exists) just keeps polling at its original interval and ignores the new `enabled` flag, and a daemon with no watcher stays without one. That is exactly the **deferred** shape already used for `lifecycle`, `guard`, etc.: "accepted into the new snapshot, but the wiring built at startup keeps the old value until a restart." So `configReload` → deferred. (The "obvious joke" the issue names — turning reload off through a reload — is specifically why this needed a decision rather than a one-line copy: refusing the reload as cold would be wrong, since nothing breaks; reporting nothing at all is the bug being fixed.) ## `health` / `coordinator`: options, not a fix (per the issue — I decide nothing here) Both are read **twice**, off two different things, and no single bucket is correct for either. **`health`** - `Fleetd.java:556-563` — startup snapshot. Decides whether `FleetHealthMonitor` is built at all and its `intervalOrDefault()`/`workingSuspectAfterOrDefault()` — frozen at startup. - `Fleetd.java:648-650` (`FleetMcp.HealthCoverageSource`) — live, via `config.get().health()`, on every `fleet_profiles` call. Feeds the reported coverage string (`detection-only` vs the full string), including whether `notifications` is configured. **`coordinator`** - `Fleetd.java:502` (`openLeadMailbox`) — startup snapshot. Resolves `coordinator.effectiveUri()` (honoring `uriEnv` over `uri`) and opens the `LeadMailbox` once with that URI, plus `selfId` and `prefetch`. All four sub-fields (`uri`, `uriEnv`, `selfId`, `prefetch`) are consumed only here for this purpose — frozen. - `MemberEnvAllowList.java:165`, reached from `HerdrPeerLauncher.java:1529-1530` — live, via `config.get()` on every spawn. Reads `coordinator.uriEnv()` **only** (not `uri`/`selfId`/ `prefetch`) to build the exclusion set that keeps the broker URI env var name out of a member's allow-list/scrub. So `coordinator.uriEnv` specifically is the split field (like `health.enabled`); `coordinator.uri`, `selfId`, `prefetch` are cleanly deferred-only (only the startup snapshot ever reads them). ### Options, with costs 1. **Report the whole key as deferred whenever anything under it changes.** Cheapest: one `Objects.equals(old.health(), fresh.health())` / `...coordinator()...` check, same shape as `lifecycle`/`guard`. **Cost:** honest for the frozen half, wrong for the live half — changing only `health.notifications.mode` (read live at :648-650) or only `coordinator.prefetch` would tell the operator a restart is needed when nothing needs to restart. This is exactly the kind of over-claim the ticket is trying to get away from, just in the safer direction. 2. **Split by sub-field.** `health.enabled` / `intervalSeconds` / `workingSuspectAfterSeconds` → deferred (frozen into the monitor); `health.notifications` → also touches the live coverage string, so it's arguably hot-for-reporting-purposes but its *value* is still read live either way — no restart needed. `coordinator.uri` / `selfId` / `prefetch` → deferred; `coordinator.uriEnv` → split (deferred for the mailbox's actual connection, hot for the spawn-time exclusion list). **Cost:** more machinery — `changedDeferredKeys` currently compares whole nested records with one `Objects.equals`; sub-field comparison means unpacking each nested record's fields by hand (or reflection) and picking a bucket per field, including one field (`coordinator.uriEnv`) that is correctly in *both* buckets. The issue's own text names the open question: what happens when both the deferred half and the hot half of the same key change in one reload? Does `Outcome.deferred()` name `"coordinator.uri"` while staying silent on `"coordinator.uriEnv"` even though `uriEnv` changing genuinely needs a restart for the mailbox to move? That needs an explicit rule, and I don't think there's a "safe default" here — it has to be decided, not defaulted. 3. **Make the frozen reader live.** For `health`: reconstruct/reconfigure `FleetHealthMonitor` (interval, enabled) on a running daemon — means starting/stopping its own scheduler and thread without racing the ordered shutdown hook or an in-flight health check. For `coordinator`: rebuild a running `LeadMailbox` — close the existing AMQP consumer/connection and open a new one with the new URI/selfId/prefetch on a live daemon. **This is the one I want to flag as possibly unsafe, not just expensive:** `selfId` names this daemon's own inbox queue (`lead.<selfId>.inbox`) — a peer lead that already learned the old coord-id would not automatically discover the new one, so changing `coordinator.selfId` live is a distributed-identity change, not just a reconnect. I did not find anything in `LeadMailbox`/`LeadCoordLoop` that handles a self-id changing under a running daemon (nor should this ticket add it — invariant 1 forbids making anything take effect live here). Biggest change, and for `coordinator` specifically I'd want that identity question answered before calling it safe. I'm not picking one — that's the point of this section. ## The `coordinator.uriEnv` question — what I actually found **Question:** if `coordinator.uriEnv` changes in a reload, can the broker URI variable end up visible to a member spawned after that reload? **What I read:** `MemberEnvAllowList.brokerUriEnvNames(FleetConfig)` (`MemberEnvAllowList.java:159- 167`) reads `config.coordinator()` (and `config.broker()`) directly off whatever `FleetConfig` it's handed. Both call sites hand it the **live** config: - `HerdrPeerLauncher.brokerUriEnvNames()` (`HerdrPeerLauncher.java:1529-1530`) calls `MemberEnvAllowList.brokerUriEnvNames(config.get())` — `config` is the `Supplier<FleetConfig>` (`ConfigRef`), so this re-reads on every spawn. - That set is used two ways, one per `memberCredentials.policy`: - **`allow-list`** (`derivedAllowedNames`, `HerdrPeerLauncher.java:1517-1527`): the set is subtracted from the derived allow-list *twice* — once inside `MemberEnvAllowList.derive(..., excludedNames)` (removes it from the profile/`allow:`-derived union) and again explicitly (`allowed.removeAll(brokerUriEnvNames)`, line 1525) after `launch.env()`'s own keys are unioned in. The scrub then blanks anything **not** in this final `allowed` set, running *after* the pane's login shell has sourced everything (`EnvAllowListScrub`, post-shell). - **`deny-by-default`** (`overlayBlockedCredentials`, `HerdrPeerLauncher.java:1317-1325`): the set is unioned into `blocked` and a sentinel value is written over each blocked name in the **pane-creation env map only** — a pre-shell overlay, per `MemberEnvAllowList`'s own class doc (lines 149-157): "Under the deny-list policy there is no scrub: the name is only removed from the pre-shell env map, and a login shell that sources the operator's secret store re-exports it. ... deny-list deployments do NOT get this guarantee." So: after a reload changes `coordinator.uriEnv` from (say) `OLD_NAME` to `NEW_NAME`, the very next spawn's exclusion set is `{NEW_NAME, ...}` — `OLD_NAME` drops out of it immediately, while the already-open `LeadMailbox` keeps using whatever host variable `OLD_NAME` named (it was never rebuilt). Whether that makes the value **visible** to a newly-spawned member depends on the policy: - **Under `deny-by-default`:** the protection this exclusion buys was already documented as weak *regardless of reload* — a login shell that re-sources the operator's secret store (which is where `OLD_NAME`'s value would live in the first place, per "Central secret store" / "Login shell beats launcher env") overwrites the pre-shell sentinel outright. So under this policy the reload-vs- mailbox desync doesn't introduce a *new* hole on top of the one the class doc already names — the exclusion's protection for either name was never guaranteed to survive a login shell under this policy. - **Under `allow-list`:** the post-shell scrub is the real control, and it defaults to **deny** everything not explicitly derived. `OLD_NAME` was never added to the *allow* set by the exclusion mechanism — the exclusion only ever *removes* it from an allow set some other source put it in (a profile's `env:` map, or the operator's own `memberCredentials.allow:` list — see `MemberEnvAllowList`'s class doc, "even when the operator lists them under `allow:`, they are excluded here"). So in the default case (`OLD_NAME` is not independently allow-listed elsewhere), losing the exclusion changes nothing: `OLD_NAME` still isn't in the derived allow set, and the scrub still blanks it. **The narrow case where it does matter:** if `OLD_NAME` is *also* present in a profile's `env:` map (frozen at startup, from `HerdrPeerLauncher`'s own `Map.copyOf(profiles)`) or in the operator's `memberCredentials.allow:` list (read live, so this can even be added in the *same* reload) — i.e. exactly the coincidence the exclusion mechanism exists to guard against — then after the reload, `OLD_NAME` re-enters the derived allow set with nothing left to strip it back out, the scrub keeps it, and if the operator's shell also still exports `OLD_NAME` with the mailbox's real (still-live) AMQP URI, a member spawned after the reload keeps that value in its environment. **So: yes, it can, but only in that narrow, config-dependent case** — an operator who has (or adds, in the same reload) `OLD_NAME` to `memberCredentials.allow:` or a profile's `env:` map. In the default configuration (nothing else references that variable name), no — the general deny-by-default allow-set logic already excludes it independent of the coordinator-specific safety net. I read `MemberEnvAllowList.java` in full and the two call sites named above; I did not write or run anything that exercises the exposure path, per the issue's instruction, and I didn't touch `MemberEnvAllowList` — invariant 2. ## Is a top-level reflection coverage test (the #323 mechanism) worth building here? **No, not now — and not a blind copy.** `ConfigRefProfileCoverageTest` works because `Profile` is a flat record of scalars: it mutates one component at a time via reflection and asserts a single boolean method (`sameLaunchSettings`) notices, then pins the exclusion set with a reason. Two things break that shape at the top level: 1. **`FleetConfig`'s components are nested records with their own defaulting**, several deeply (`Fleet` alone holds `leaders: Map<String, Leader>`, `charters`, `developers`/`reviewers`/ `architects`, `tabLabel`). Building valid "base" and "alt" instances for every nested record by reflection (as `baseValues()`/`altValues()` do for `Profile`) is real authoring work, not a mechanical extension — `Fleet` and `MemberCredentials` are not "one more scalar row" the way `Profile`'s fields are. 2. **The correctness claim a checker would need to make is external to the record's shape.** For `Profile`, "is this component compared or excluded" is a closed question the method itself answers — reflection can observe it directly. For `FleetConfig`, "is this key genuinely hot" means "is it read through `config.get()` at every point of use in `Fleetd.java`/`HerdrPeerLauncher.java` /etc.", which is a fact about *other files*, invisible to reflection over the config record. A checker built this way could only assert "this key is *explicitly named* somewhere (cold, deferred, or a hot-exclusion set with a citation)" — it cannot verify the citation is true, the same gap `LAUNCH_SETTINGS_EXCLUDED` already has (a human still has to read the wiring to trust the excuse). 3. **`health`/`coordinator` don't fit a three-way partition at all.** A reflection mutate-and-assert loop needs each component to land in exactly one bucket (cold / deferred / hot-excluded). These two need a fourth "split — see doc" bucket the #323 mechanism never needed, and building that bucket well is exactly the open design question in the options section above, not something a generic coverage test can settle. **What such a checker WOULD still catch, if built:** a new top-level `FleetConfig` component added later with no explicit triage anywhere — the literal shape of both #323 and this issue. That's real, recurring value (two real instances so far). **What it would NOT catch:** a wrongly-justified exclusion (an "it's read live" comment that isn't true), or a wrong verdict on a split key — both still require a human reading the wiring, same as today. Given the authoring cost (especially `Fleet`) and that the two genuinely dangerous gaps here (`health`, `coordinator`) can't be resolved by this mechanism until their split classification is decided, I'd hold off building it now. If/when `health`/`coordinator` get a final classification, a narrower version — enumerate top-level components, require each to be named in `COLD_KEYS`, an explicit deferred-key name list (`changedDeferredKeys` doesn't currently have one — it's all inline `if`s), or a documented hot/split exclusion set — becomes a much smaller, well-defined change. Both answers were explicitly fine per the issue; this is mine, and I'm open to being told to build it anyway. ## Build ``` cd fleetd && mvn clean install ``` Full run: `Tests run: 1341, Failures: 0, Errors: 0, Skipped: 0` — `BUILD SUCCESS` (45.575 s). `ConfigRefTest` alone: `Tests run: 19, Failures: 0, Errors: 0, Skipped: 0`. ## Mutation proof **`primary`** — removed the `primary` check from `changedDeferredKeys`, ran only the new test: ``` [ERROR] dev.ltms.fleet.config.ConfigRefTest.changingPrimaryIsReportedAsDeferred(Path) -- Time elapsed: 0.281 s <<< FAILURE! org.opentest4j.AssertionFailedError: expected: <[primary]> but was: <[]> at dev.ltms.fleet.config.ConfigRefTest.changingPrimaryIsReportedAsDeferred(ConfigRefTest.java:532) [ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0 [INFO] BUILD FAILURE ``` Restored, re-ran full `ConfigRefTest` green. **`configReload`** — removed the `configReload` check, ran only the new test: ``` [ERROR] dev.ltms.fleet.config.ConfigRefTest.changingConfigReloadIsReportedAsDeferred(Path) -- Time elapsed: 0.282 s <<< FAILURE! org.opentest4j.AssertionFailedError: expected: <[configReload]> but was: <[]> at dev.ltms.fleet.config.ConfigRefTest.changingConfigReloadIsReportedAsDeferred(ConfigRefTest.java:566) [ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0 [INFO] BUILD FAILURE ``` Restored, re-ran full `ConfigRefTest` green (19/19), then full `mvn clean install` (1341/1341) as quoted above. ## Caveats for review - `health` and `coordinator` are untouched by design — this PR does not resolve them, per the issue. - No top-level reflection coverage test was added — see the verdict section above. - `fleetd/fleetd.yaml` is gitignored and absent from my worktree; nothing here depends on or reports its contents. - Out of scope, not investigated: whether `deny-by-default`'s pre-shell-only protection for `coordinator`/`broker` URI env names (already documented as bypassable by a login shell) should be strengthened — that's the existing, separately-documented deny-list weakness, not something this ticket's reload-classification scope covers.
agent added 1 commit 2026-09-04 09:43:57 +02:00
fleetd#326: classify primary and configReload as deferred top-level keys
CI / contract (pull_request) Successful in 1m3s
Details
CI / build (pull_request) Failing after 1m44s
Details
6d493bc7bb 
ConfigRef.changedDeferredKeys only classified seven top-level FleetConfig
keys (#323 fixed the profile side). Two more keys are read only off the
startup snapshot and were missing:

- primary: Fleetd.java:506/519/520 feed PrimaryRegistry and ReplyPushLoop
  at construction; neither is rebuilt on reload.
- configReload: Fleetd.java:679-680 decide once at startup whether to
  build a ConfigWatcher at all, and with what interval; the watcher that
  would apply a later change is itself built once, so it is deferred
  (not cold — no already-open resource goes inconsistent, a running
  watcher just keeps its original settings).

health and coordinator are deliberately left unclassified: both are read
both off the startup snapshot AND live off the config supplier at a
second call site, so no single bucket is correct for either — see the
PR body for the options writeup and the coordinator.uriEnv exposure
question the issue asked to be answered.

Each fix is proven with a failing-first test in ConfigRefTest and a
revert-quote-restore mutation check (see PR body for the transcripts).
ltms closed this pull request 2026-09-04 09:59:07 +02:00
Some checks are pending
CI / contract (pull_request) Successful in 1m3s
Details
CI / build (pull_request) Failing after 1m44s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
blocked
Cannot start until something else lands. The body says what.
 needs-live-proof
Merged and green, but never shown working on the running daemon. Not the same as done.
 ready-to-delegate
Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.
 silent-default
A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent kevin ltms
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#328
Delete Branch "worker/fix-326-50506e-15"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?