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

The top-level deferred-key list has the same drift as #323, and two keys are read both from the startup snapshot and live — no single classification is right for them #326

New Issue
Closed
opened 2026-09-04 09:33:58 +02:00 by ltms · 2 comments
ltms commented 2026-09-04 09:33:58 +02:00
Owner
Copy Link
Copy Source

Follow-up to #323, which fixed the profile side of the reload classifier and left the top-level side reported but unfixed. I verified every claim below myself.

The list, measured

ConfigRef.changedDeferredKeys compares exactly seven top-level keys:

guard  leadHeartbeat  lifecycle  quarantineCooldownSeconds  spawnReady*  worktreeGroup  worktreeRoot

(worktreeGroup is new, from #323.) Everything else on FleetConfig is unclassified. A reload that changes only an unclassified key reports a bare config reloaded and the daemon keeps the old value.

Two simple gaps

primary — every read is off the startup snapshot, so it is deferred and simply missing:

Fleetd.java:506   cfg.primary() != null ? cfg.primary().terminal() : null
Fleetd.java:519   cfg.primary() != null ? cfg.primary().remindersOrDefault() : 5
Fleetd.java:520   cfg.primary() != null ? cfg.primary().backoffMsOrDefault() : 15_000L

These are baked into PrimaryRegistry / ReplyPushLoop at construction. Changing the pinned primary terminal in a reload silently does nothing, which matters: a lead whose tab no longer matches is demoted to worker.

configReload — read only at startup, and cold rather than deferred:

Fleetd.java:679   if (cfg.configReload() != null && cfg.configReload().isEnabled())
Fleetd.java:680   configWatcher = new ConfigWatcher(config, cfg.configReload().intervalSeconds());

The component that would apply a reload is itself built once. Turning reload off through a reload is the obvious joke here, and it currently reports success.

Two keys that no single classification fits

This is the part that needs a decision rather than a line.

health is read both ways:

Fleetd.java:556-563   cfg.health()...            // startup snapshot → deferred
Fleetd.java:648       config.get().health()      // live supplier    → hot

Line 556 decides whether FleetHealthMonitor is built at all and with what interval — that is frozen. Line 648 is HealthCoverageSource, feeding the coverage string fleet_profiles reports — that is live. So changing health.enabled needs a restart while changing something the coverage string reads does not, and one classification will be wrong for one of them.

coordinator is the same shape:

Fleetd.java:502                    openLeadMailbox(cfg.coordinator(), ...)   // startup → cold-ish
MemberEnvAllowList.java:165        addUriEnvIfPresent(names, config.coordinator())
HerdrPeerLauncher.java:1530        MemberEnvAllowList.brokerUriEnvNames(config.get())   // live → hot

The LeadMailbox is opened once. The broker URI env-var name is re-read from the live config on every spawn, to keep that variable out of a member's environment. That second one is a credential-adjacent path, which is why I am not guessing at it: if an operator changes coordinator.uriEnv in a reload, the exclusion list follows immediately while the mailbox does not, and I want someone to say out loud whether that split is safe or a hole.

What I want

Goal: every top-level FleetConfig key is classified, and a key that cannot be classified as one thing is described honestly rather than forced onto a list.

Do these two:

  1. Add primary to changedDeferredKeys and to the class doc's deferred list.
  2. Add configReload where it belongs. Decide deferred versus cold and say why — the class doc defines cold as a key whose new value would leave the daemon inconsistent with an already-open resource.

Do not change health or coordinator. Instead, write up the options and what each costs, and stop. I will decide. Cover at least:

  • Report the key as deferred whenever anything under it changes. Safe, but it tells the operator a restart is needed when often it is not.
  • Split the classification by sub-field, so health.enabled is deferred and the parts only the live reader sees are hot. More honest, more machinery, and it needs a rule for what happens when both change at once.
  • Make the frozen reader live so the key becomes wholly hot. Biggest change, and for coordinator it means rebuilding a LeadMailbox on a running daemon — say whether that is even safe.

For coordinator specifically, answer this factual question: if coordinator.uriEnv changes in a reload, can the broker URI variable end up visible to a member spawned after that reload? Read MemberEnvAllowList and the spawn path and say what you find. If the answer is no, say what stops it. Do not write an exploit; describe the mechanism.

Invariants:

  1. Do not make any key take effect live in this ticket. Same rule as #323 — this is about what a reload reports.
  2. Do not weaken MemberEnvAllowList. If your reading suggests a hole, report it; do not "fix" it here.
  3. The #323 mechanism stays green. ConfigRefProfileCoverageTest pins the profile side including its exclusion set; do not edit that assertion.

A note on the mechanism, since it is the obvious thing to reach for. #323's reflection coverage test works because a Profile is a flat record of scalars. FleetConfig's top level is not — its components are nested records with their own fields and defaulting methods, and several are genuinely hot. Do not force the same reflection test onto it without saying why it works. If you think a checker is possible here, propose it and say exactly what it would and would not catch. If you think it is not worth it, say that instead. Either answer is fine; an unexamined copy of the previous mechanism is not.

Rules

  • Prove each of the two fixes with a test that fails without it, next to ConfigRefTest's existing deferred-key tests.
  • Mutation proof required: revert each fix, quote the real failure output, restore it.
  • fleetd/fleetd.yaml is gitignored and absent from your worktree. Do not report on its contents.
  • Never git stash — the stash is shared across every worktree here.
  • Never run git worktree remove or git worktree prune — other workers are live in those directories.
  • Stage files explicitly; never git add -A. Never merge.
  • Run cd fleetd && mvn clean install unpiped; quote the real Tests run: and BUILD lines. Never read $? after a pipe.
  • Put your full report in the PR body as well as in your fleet_reply.

Already established, do not re-derive

memberCredentials and memberLoginShell were checked during #323 and are correctly hot — live supplier reads at Fleetd.java:198/205/729 and HerdrPeerLauncher.java:1454-1455/1468. No gap there.

Follow-up to #323, which fixed the *profile* side of the reload classifier and left the top-level side reported but unfixed. I verified every claim below myself. ## The list, measured `ConfigRef.changedDeferredKeys` compares exactly seven top-level keys: ``` guard leadHeartbeat lifecycle quarantineCooldownSeconds spawnReady* worktreeGroup worktreeRoot ``` (`worktreeGroup` is new, from #323.) Everything else on `FleetConfig` is unclassified. A reload that changes only an unclassified key reports a bare `config reloaded` and the daemon keeps the old value. ## Two simple gaps **`primary`** — every read is off the startup snapshot, so it is deferred and simply missing: ``` Fleetd.java:506 cfg.primary() != null ? cfg.primary().terminal() : null Fleetd.java:519 cfg.primary() != null ? cfg.primary().remindersOrDefault() : 5 Fleetd.java:520 cfg.primary() != null ? cfg.primary().backoffMsOrDefault() : 15_000L ``` These are baked into `PrimaryRegistry` / `ReplyPushLoop` at construction. Changing the pinned primary terminal in a reload silently does nothing, which matters: a lead whose tab no longer matches is demoted to worker. **`configReload`** — read only at startup, and cold rather than deferred: ``` Fleetd.java:679 if (cfg.configReload() != null && cfg.configReload().isEnabled()) Fleetd.java:680 configWatcher = new ConfigWatcher(config, cfg.configReload().intervalSeconds()); ``` The component that would apply a reload is itself built once. Turning reload off through a reload is the obvious joke here, and it currently reports success. ## Two keys that no single classification fits This is the part that needs a decision rather than a line. **`health`** is read both ways: ``` Fleetd.java:556-563 cfg.health()... // startup snapshot → deferred Fleetd.java:648 config.get().health() // live supplier → hot ``` Line 556 decides whether `FleetHealthMonitor` is built at all and with what interval — that is frozen. Line 648 is `HealthCoverageSource`, feeding the coverage string `fleet_profiles` reports — that is live. So changing `health.enabled` needs a restart while changing something the coverage string reads does not, and one classification will be wrong for one of them. **`coordinator`** is the same shape: ``` Fleetd.java:502 openLeadMailbox(cfg.coordinator(), ...) // startup → cold-ish MemberEnvAllowList.java:165 addUriEnvIfPresent(names, config.coordinator()) HerdrPeerLauncher.java:1530 MemberEnvAllowList.brokerUriEnvNames(config.get()) // live → hot ``` The `LeadMailbox` is opened once. The broker URI env-var name is re-read from the live config on every spawn, to keep that variable out of a member's environment. **That second one is a credential-adjacent path**, which is why I am not guessing at it: if an operator changes `coordinator.uriEnv` in a reload, the exclusion list follows immediately while the mailbox does not, and I want someone to say out loud whether that split is safe or a hole. ## What I want **Goal:** every top-level `FleetConfig` key is classified, and a key that cannot be classified as one thing is described honestly rather than forced onto a list. **Do these two:** 1. Add `primary` to `changedDeferredKeys` and to the class doc's deferred list. 2. Add `configReload` where it belongs. Decide deferred versus cold and say why — the class doc defines cold as a key whose new value would leave the daemon inconsistent with an already-open resource. **Do not change `health` or `coordinator`.** Instead, write up the options and what each costs, and stop. I will decide. Cover at least: - Report the key as deferred whenever *anything* under it changes. Safe, but it tells the operator a restart is needed when often it is not. - Split the classification by sub-field, so `health.enabled` is deferred and the parts only the live reader sees are hot. More honest, more machinery, and it needs a rule for what happens when both change at once. - Make the frozen reader live so the key becomes wholly hot. Biggest change, and for `coordinator` it means rebuilding a `LeadMailbox` on a running daemon — say whether that is even safe. For `coordinator` specifically, answer this factual question: **if `coordinator.uriEnv` changes in a reload, can the broker URI variable end up visible to a member spawned after that reload?** Read `MemberEnvAllowList` and the spawn path and say what you find. If the answer is no, say what stops it. Do not write an exploit; describe the mechanism. **Invariants:** 1. **Do not make any key take effect live in this ticket.** Same rule as #323 — this is about what a reload *reports*. 2. **Do not weaken `MemberEnvAllowList`.** If your reading suggests a hole, report it; do not "fix" it here. 3. **The #323 mechanism stays green.** `ConfigRefProfileCoverageTest` pins the profile side including its exclusion set; do not edit that assertion. **A note on the mechanism, since it is the obvious thing to reach for.** #323's reflection coverage test works because a `Profile` is a flat record of scalars. `FleetConfig`'s top level is not — its components are nested records with their own fields and defaulting methods, and several are genuinely hot. **Do not force the same reflection test onto it without saying why it works.** If you think a checker is possible here, propose it and say exactly what it would and would not catch. If you think it is not worth it, say that instead. Either answer is fine; an unexamined copy of the previous mechanism is not. ## Rules - Prove each of the two fixes with a test that fails without it, next to `ConfigRefTest`'s existing deferred-key tests. - Mutation proof required: revert each fix, quote the real failure output, restore it. - `fleetd/fleetd.yaml` is gitignored and absent from your worktree. Do not report on its contents. - Never `git stash` — the stash is shared across every worktree here. - Never run `git worktree remove` or `git worktree prune` — other workers are live in those directories. - Stage files explicitly; never `git add -A`. Never merge. - Run `cd fleetd && mvn clean install` **unpiped**; quote the real `Tests run:` and `BUILD` lines. Never read `$?` after a pipe. - Put your full report in the PR body as well as in your `fleet_reply`. ## Already established, do not re-derive `memberCredentials` and `memberLoginShell` were checked during #323 and are correctly hot — live supplier reads at `Fleetd.java:198/205/729` and `HerdrPeerLauncher.java:1454-1455/1468`. No gap there.
ltms commented 2026-09-04 09:58:44 +02:00
Author
Owner
Copy Link
Copy Source

Merged as 2fd4a9e..823976c (PR #328), with one correction of my own.

What I checked myself

Branch was behind main, so --no-ff merge and a rebuild. Full build, unpiped:

Tests run: 1342, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS

I re-read the cited lines rather than trusting them. Both citations are right:

Fleetd.java:506  String pinnedPrimaryTerminal = cfg.primary() != null ? cfg.primary().terminal() : null;
Fleetd.java:519  int maxReminders = cfg.primary() != null ? cfg.primary().remindersOrDefault() : 5;
Fleetd.java:520  long backoffMs  = cfg.primary() != null ? cfg.primary().backoffMsOrDefault() : 15_000L;
Fleetd.java:679  if (cfg.configReload() != null && cfg.configReload().isEnabled()) {
Fleetd.java:680      configWatcher = new ConfigWatcher(config, cfg.configReload().intervalSeconds());

Those are the only readers of either key outside FleetConfig itself, and each is a one-shot startup read. deferred is the right class for both, and the configReload-is-deferred-not-cold argument holds: nothing already open goes inconsistent, a running watcher just keeps its old settings.

Mutation R — remove both new checks, full suite

Tests run: 1342, Failures: 2
ConfigRefTest.changingPrimaryIsReportedAsDeferred:532      expected: <[primary]>      but was: <[]>
ConfigRefTest.changingConfigReloadIsReportedAsDeferred:566 expected: <[configReload]> but was: <[]>

Exactly one failure per key, and nothing else in the suite covered either. Both halves are separately pinned, and the tests drive ref.reload() — the real entry point, not a seam beside it.

Correction — the primary consequence was over-claimed

The merged javadoc said a changed pin leaves "a lead whose pinned terminal changed under a running daemon unresolved as primary until a restart". That over-claims. Fleetd.java:508-515 says primary.terminal is deprecated (CB-532) and warns about it at startup: a lead's identity comes from leaders:/leadScan:. Changing this pin does not demote or promote a lead that uses those.

What a changed pin genuinely does not take effect on until a restart: the fallback nudge destination the pin still is, the deprecated identity path for an operator who still relies on it, and pushReminders/pushBackoffMs. Fixed in 823976c. The classification is unchanged and correct; only the stated harm was wrong.

The denominator — I measured it, and it changes the follow-up

The ticket asked about health and coordinator. It did not ask "what else is missing", so I checked. FleetConfig has 22 top-level record components. Four are named nowhere in ConfigRef.java:

key verdict
memberCredentials hot, correctly absent — read live off config.get() at Fleetd.java:198, 205, 729
memberLoginShell hot, correctly absent — read live via HerdrPeerLauncher#configuredMemberLoginShell
health undecided
coordinator undecided

So no new defect: #326's scope is complete. But the measurement is the point. "Not mentioned in ConfigRef" looks identical for a key that is correctly hot and for a key nobody triaged, and twice now the second kind hid among the first — worktreeGroup in #323, primary/configReload here. I wrote the count and both verdicts into the class doc so the next person does not redo this.

On the two open questions

The top-level coverage checker. I accept the argument for not building it now. It is not the #323 shape: reflection over the config record can assert "named somewhere" but cannot verify the citation is true, and health/coordinator need a fourth bucket the three-class model does not have. Holding it until their classification is decided is right.

coordinator.uriEnv. The analysis is careful and I agree with the conclusion — a leak needs the old variable name to be separately allow-listed for an unrelated reason, since the exclusion only ever removes a name from an allow set. Noting explicitly that nothing was run against that path.

health/coordinator classification stays mine. The three options and their costs are what I asked for and what I got. Deciding it needs a fourth class ("split — some sub-fields live, some frozen") and that is a design change to this file, not a bug fix. It goes to its own ticket.

Closing.

Merged as `2fd4a9e`..`823976c` (PR #328), with one correction of my own. ## What I checked myself Branch was **behind main**, so `--no-ff` merge and a rebuild. Full build, unpiped: ``` Tests run: 1342, Failures: 0, Errors: 0, Skipped: 0 BUILD SUCCESS ``` I re-read the cited lines rather than trusting them. Both citations are right: ``` Fleetd.java:506 String pinnedPrimaryTerminal = cfg.primary() != null ? cfg.primary().terminal() : null; Fleetd.java:519 int maxReminders = cfg.primary() != null ? cfg.primary().remindersOrDefault() : 5; Fleetd.java:520 long backoffMs = cfg.primary() != null ? cfg.primary().backoffMsOrDefault() : 15_000L; Fleetd.java:679 if (cfg.configReload() != null && cfg.configReload().isEnabled()) { Fleetd.java:680 configWatcher = new ConfigWatcher(config, cfg.configReload().intervalSeconds()); ``` Those are the only readers of either key outside `FleetConfig` itself, and each is a one-shot startup read. `deferred` is the right class for both, and the `configReload`-is-deferred-not-cold argument holds: nothing already open goes inconsistent, a running watcher just keeps its old settings. ### Mutation R — remove both new checks, full suite ``` Tests run: 1342, Failures: 2 ConfigRefTest.changingPrimaryIsReportedAsDeferred:532 expected: <[primary]> but was: <[]> ConfigRefTest.changingConfigReloadIsReportedAsDeferred:566 expected: <[configReload]> but was: <[]> ``` Exactly one failure per key, and nothing else in the suite covered either. Both halves are separately pinned, and the tests drive `ref.reload()` — the real entry point, not a seam beside it. ## Correction — the `primary` consequence was over-claimed The merged javadoc said a changed pin leaves "a lead whose pinned terminal changed under a running daemon **unresolved as primary** until a restart". That over-claims. `Fleetd.java:508-515` says `primary.terminal` is **deprecated** (CB-532) and warns about it at startup: a lead's identity comes from `leaders:`/`leadScan:`. Changing this pin does not demote or promote a lead that uses those. What a changed pin genuinely does not take effect on until a restart: the fallback nudge destination the pin still is, the deprecated identity path for an operator who still relies on it, and `pushReminders`/`pushBackoffMs`. Fixed in `823976c`. The classification is unchanged and correct; only the stated harm was wrong. ## The denominator — I measured it, and it changes the follow-up The ticket asked about `health` and `coordinator`. It did not ask "what else is missing", so I checked. `FleetConfig` has **22** top-level record components. **Four** are named nowhere in `ConfigRef.java`: | key | verdict | |---|---| | `memberCredentials` | **hot, correctly absent** — read live off `config.get()` at `Fleetd.java:198, 205, 729` | | `memberLoginShell` | **hot, correctly absent** — read live via `HerdrPeerLauncher#configuredMemberLoginShell` | | `health` | undecided | | `coordinator` | undecided | So no new defect: #326's scope is complete. But the measurement is the point. **"Not mentioned in `ConfigRef`" looks identical for a key that is correctly hot and for a key nobody triaged**, and twice now the second kind hid among the first — `worktreeGroup` in #323, `primary`/`configReload` here. I wrote the count and both verdicts into the class doc so the next person does not redo this. ## On the two open questions **The top-level coverage checker.** I accept the argument for not building it now. It is not the #323 shape: reflection over the config record can assert "named somewhere" but cannot verify the citation is true, and `health`/`coordinator` need a fourth bucket the three-class model does not have. Holding it until their classification is decided is right. **`coordinator.uriEnv`.** The analysis is careful and I agree with the conclusion — a leak needs the old variable name to be *separately* allow-listed for an unrelated reason, since the exclusion only ever removes a name from an allow set. Noting explicitly that nothing was run against that path. **`health`/`coordinator` classification stays mine.** The three options and their costs are what I asked for and what I got. Deciding it needs a fourth class ("split — some sub-fields live, some frozen") and that is a design change to this file, not a bug fix. It goes to its own ticket. Closing.
ltms closed this issue 2026-09-04 09:58:49 +02:00
ltms commented 2026-09-04 09:59:01 +02:00
Author
Owner
Copy Link
Copy Source

Correcting myself on one point in the comment above. I presented the memberCredentials / memberLoginShell verdict as something I went and found. It was already written in this ticket's own "Already established, do not re-derive" tail, from #323. So my check confirmed it against today's code; it did not discover it.

The count of 22 and the four-key gap list are new, and the reason for writing them into the class doc stands. But the two hot verdicts were not.

Correcting myself on one point in the comment above. I presented the `memberCredentials` / `memberLoginShell` verdict as something I went and found. It was already written in this ticket's own "Already established, do not re-derive" tail, from #323. So my check **confirmed** it against today's code; it did not discover it. The count of 22 and the four-key gap list are new, and the reason for writing them into the class doc stands. But the two hot verdicts were not.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
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#326
Delete Branch "%!s()"

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?