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

fleetd#323: close the reload-classifier drift with a reflection coverage test #325

Closed
agent wants to merge 0 commits from worker/fix-323-b8287d-13 into main
pull from: worker/fix-323-b8287d-13
merge into: fleet:main
Conversation 0 Commits - Files Changed -
agent commented 2026-09-04 09:26:54 +02:00
Member
Copy Link
Copy Source

Fixes fleetd#323.

What was wrong

ConfigRef.sameLaunchSettings's javadoc claimed it "compares every component the launcher reads at
spawn". It missed ideProjectDir, ideOpenCommand and autoCompactWindow (all three baked in at
daemon startup — ClaudeCodeLauncher.java:267/269/926, OpenCodeLauncher.java:474/480/486/650). Separately, changedDeferredKeysand the class doc's deferred list checkedworktreeRootbut not its siblingworktreeGroup, though both are baked into the same GitWorktreesatFleetd.java:251andGitWorktreesis never rebuilt. A reload that changed only one of those four keys used to report a bareconfig reloaded` and the running daemon kept the old value.

The mechanism (not just the four fields)

Added ConfigRefProfileCoverageTest. It enumerates every record component of
FleetConfig.Profile by reflection, builds a base profile and, for each component not in the new
ConfigRef.LAUNCH_SETTINGS_EXCLUDED set, constructs a copy that differs in exactly that one field
and asserts sameLaunchSettings actually reports a difference (a real behavioural check, not a
hand-maintained list pretending to be one). It prints its own denominator, as the ticket required:

ConfigRef.sameLaunchSettings coverage — 26 Profile components total, 23 compared, 3 excluded ([credentialId, maxLoad, weight])

LAUNCH_SETTINGS_EXCLUDED = {weight, maxLoad, credentialId} — the three fields the existing
javadoc already named as genuinely hot (read live by the placement policy / CompositePeerLauncher
and the CB-578 stage B exhaustion sink). Everything else on the record must be compared or the test
fails by name. This is how the mechanism found a 5th gap on its own: the profile field itself
(the profile's own identity string) was neither compared nor excluded — added it to the comparison.

I chose reflection-over-record + an explicit exclusion set (the issue's candidate mechanism)
because it wasn't awkward here: Profile's canonical constructor is the declared 26-arg one, so
RecordComponent[] order matches constructor parameter order exactly, and building a one-field
mutant via getDeclaredConstructor(...).newInstance(...) is straightforward. No simpler
alternative seemed necessary.

The two open questions

1. Is worktreeGroup deferred or cold? Deferred — same reasoning the code already applies to
its sibling worktreeRoot. Cold is for keys where accepting the new value would leave the daemon
in an inconsistent state relative to an already-open resource (bound socket, open broker
connection, decided auth mode) — the class doc's own definition. worktreeGroup has no such
property: the new value lands safely in the snapshot, it just doesn't retroactively re-chgrp
worktrees GitWorktrees already built. That is exactly the deferred definition, and worktreeRoot
— baked into the very same GitWorktrees at the very same constructor call — already sets the
precedent. Fixed by adding a worktreeGroup check right next to worktreeRoot's in
changedDeferredKeys, plus a class-doc update.

2. Does the same drift exist for the top-level FleetConfig keys changedDeferredKeys compares
by hand? Yes, but it is not the same one-line shape, so per the ticket's instruction I fixed only
the one key it explicitly named (worktreeGroup) and left the rest — reporting them here instead:

  • primary — missing entirely. Every read (Fleetd.java:506 cfg.primary().terminal(),
    Fleetd.java:519-520 remindersOrDefault()/backoffMsOrDefault()) is off the startup snapshot,
    baked into PrimaryRegistry / ReplyPushLoop at construction. This one is one-line-shaped
    (single reader, unambiguously deferred) — I left it unfixed only because the ticket asked me to
    fix the shape only when it's a single gap, and by the time I'd found primary I'd also found the
    three below, making the total shape bigger. Worth its own follow-up ticket.
  • configReload — missing entirely, and cold rather than deferred: Fleetd.java:679-680 reads it
    only from the startup snapshot to decide whether to construct ConfigWatcher at all — the
    component that would apply a reload is itself only built once, so this is definitionally cold.
  • health — mixed, not one-line. Fleetd.java:556-563 reads the startup snapshot to decide
    whether to build FleetHealthMonitor and its interval (deferred), but
    Fleetd.java:648 (HealthCoverageSource) reads config.get().health() live for
    fleet_profiles's reported coverage string (hot). Same key, two different reload behaviours
    depending on which part changed — needs real design, not a one-line Objects.equals add.
  • coordinator — mixed, and the issue's own "Also noted" section already flags this key's javadoc
    as possibly stale and explicitly says "do not fix it here." Confirmed the mixed shape:
    Fleetd.java:502 builds the LeadMailbox once from the startup snapshot (cold-ish, matches the
    javadoc), but HerdrPeerLauncher.java:1530 → MemberEnvAllowList.brokerUriEnvNames reads
    config.get().coordinator() live on every spawn to exclude its URI env var name (hot). Left
    untouched per the issue's own instruction.
  • memberCredentials and memberLoginShell — both correctly hot already (live Supplier reads
    at Fleetd.java:198/205/729 and HerdrPeerLauncher.java:1454-1455/1468 respectively); no gap.

So: real drift exists beyond worktreeGroup, but it's primary + configReload (simple) and
health + coordinator (each genuinely mixed hot/deferred/cold within one key) — bigger than the
single-sibling-key shape of instance 2. Per the ticket's rule I did not touch these; the coordinator
one is explicitly out of scope by the issue text itself.

Rules followed

  • Did not make any key take effect live — this PR only changes what reload reports.
  • Mutation proof: reverted the ideProjectDir/ideOpenCommand/autoCompactWindow comparisons and
    the worktreeGroup check, ran ConfigRefTest+ConfigRefProfileCoverageTest, got 3 real failures
    naming exactly the missing fields, then restored the fix and reran — Tests run: 1333, Failures: 0, Errors: 0 again. See the fleet_reply / PR description below for the exact failure output.
  • sameLaunchSettings's javadoc is corrected to describe what ConfigRefProfileCoverageTest
    actually guarantees, not an unchecked claim.

Build

mvn clean install
...
Tests run: 1333, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS
Fixes fleetd#323. ## What was wrong `ConfigRef.sameLaunchSettings`'s javadoc claimed it "compares every component the launcher reads at spawn". It missed `ideProjectDir`, `ideOpenCommand` and `autoCompactWindow` (all three baked in at daemon startup — `ClaudeCodeLauncher.java:267/269/926`, `OpenCodeLauncher.java:474/480/486/650). Separately, `changedDeferredKeys` and the class doc's deferred list checked `worktreeRoot` but not its sibling `worktreeGroup`, though both are baked into the same `GitWorktrees` at `Fleetd.java:251` and `GitWorktrees` is never rebuilt. A reload that changed only one of those four keys used to report a bare `config reloaded` and the running daemon kept the old value. ## The mechanism (not just the four fields) Added `ConfigRefProfileCoverageTest`. It enumerates every record component of `FleetConfig.Profile` by reflection, builds a base profile and, for each component not in the new `ConfigRef.LAUNCH_SETTINGS_EXCLUDED` set, constructs a copy that differs in exactly that one field and asserts `sameLaunchSettings` actually reports a difference (a real behavioural check, not a hand-maintained list pretending to be one). It prints its own denominator, as the ticket required: ``` ConfigRef.sameLaunchSettings coverage — 26 Profile components total, 23 compared, 3 excluded ([credentialId, maxLoad, weight]) ``` `LAUNCH_SETTINGS_EXCLUDED = {weight, maxLoad, credentialId}` — the three fields the existing javadoc already named as genuinely hot (read live by the placement policy / `CompositePeerLauncher` and the CB-578 stage B exhaustion sink). Everything else on the record must be compared or the test fails by name. This is how the mechanism found a 5th gap on its own: the `profile` field itself (the profile's own identity string) was neither compared nor excluded — added it to the comparison. I chose reflection-over-record + an explicit exclusion set (the issue's candidate mechanism) because it wasn't awkward here: `Profile`'s canonical constructor is the declared 26-arg one, so `RecordComponent[]` order matches constructor parameter order exactly, and building a one-field mutant via `getDeclaredConstructor(...).newInstance(...)` is straightforward. No simpler alternative seemed necessary. ## The two open questions **1. Is `worktreeGroup` deferred or cold?** Deferred — same reasoning the code already applies to its sibling `worktreeRoot`. Cold is for keys where accepting the new value would leave the daemon in an inconsistent state relative to an already-open resource (bound socket, open broker connection, decided auth mode) — the class doc's own definition. `worktreeGroup` has no such property: the new value lands safely in the snapshot, it just doesn't retroactively re-`chgrp` worktrees `GitWorktrees` already built. That is exactly the deferred definition, and `worktreeRoot` — baked into the very same `GitWorktrees` at the very same constructor call — already sets the precedent. Fixed by adding a `worktreeGroup` check right next to `worktreeRoot`'s in `changedDeferredKeys`, plus a class-doc update. **2. Does the same drift exist for the top-level `FleetConfig` keys `changedDeferredKeys` compares by hand?** Yes, but it is not the same one-line shape, so per the ticket's instruction I fixed only the one key it explicitly named (`worktreeGroup`) and left the rest — reporting them here instead: - `primary` — missing entirely. Every read (`Fleetd.java:506` `cfg.primary().terminal()`, `Fleetd.java:519-520` `remindersOrDefault()`/`backoffMsOrDefault()`) is off the startup snapshot, baked into `PrimaryRegistry` / `ReplyPushLoop` at construction. This one *is* one-line-shaped (single reader, unambiguously deferred) — I left it unfixed only because the ticket asked me to fix the shape only when it's a single gap, and by the time I'd found `primary` I'd also found the three below, making the total shape bigger. Worth its own follow-up ticket. - `configReload` — missing entirely, and cold rather than deferred: `Fleetd.java:679-680` reads it only from the startup snapshot to decide whether to construct `ConfigWatcher` at all — the component that would apply a reload is itself only built once, so this is definitionally cold. - `health` — mixed, not one-line. `Fleetd.java:556-563` reads the startup snapshot to decide whether to build `FleetHealthMonitor` and its interval (deferred), but `Fleetd.java:648` (`HealthCoverageSource`) reads `config.get().health()` live for `fleet_profiles`'s reported coverage string (hot). Same key, two different reload behaviours depending on which part changed — needs real design, not a one-line `Objects.equals` add. - `coordinator` — mixed, and the issue's own "Also noted" section already flags this key's javadoc as possibly stale and explicitly says "do not fix it here." Confirmed the mixed shape: `Fleetd.java:502` builds the `LeadMailbox` once from the startup snapshot (cold-ish, matches the javadoc), but `HerdrPeerLauncher.java:1530` → `MemberEnvAllowList.brokerUriEnvNames` reads `config.get().coordinator()` live on every spawn to exclude its URI env var name (hot). Left untouched per the issue's own instruction. - `memberCredentials` and `memberLoginShell` — both correctly hot already (live `Supplier` reads at `Fleetd.java:198/205/729` and `HerdrPeerLauncher.java:1454-1455/1468` respectively); no gap. So: real drift exists beyond `worktreeGroup`, but it's `primary` + `configReload` (simple) and `health` + `coordinator` (each genuinely mixed hot/deferred/cold within one key) — bigger than the single-sibling-key shape of instance 2. Per the ticket's rule I did not touch these; the `coordinator` one is explicitly out of scope by the issue text itself. ## Rules followed - Did not make any key take effect live — this PR only changes what reload *reports*. - Mutation proof: reverted the `ideProjectDir`/`ideOpenCommand`/`autoCompactWindow` comparisons and the `worktreeGroup` check, ran `ConfigRefTest`+`ConfigRefProfileCoverageTest`, got 3 real failures naming exactly the missing fields, then restored the fix and reran — `Tests run: 1333, Failures: 0, Errors: 0` again. See the fleet_reply / PR description below for the exact failure output. - `sameLaunchSettings`'s javadoc is corrected to describe what `ConfigRefProfileCoverageTest` actually guarantees, not an unchecked claim. ## Build ``` mvn clean install ... Tests run: 1333, Failures: 0, Errors: 0, Skipped: 0 BUILD SUCCESS ```
agent added 1 commit 2026-09-04 09:26:54 +02:00
fleetd#323: close the reload-classifier drift with a reflection coverage test
CI / contract (pull_request) Successful in 1m11s
Details
CI / build (pull_request) Successful in 2m6s
Details
ca47e90c01 
ConfigRef.sameLaunchSettings' javadoc claimed it compares every component
the launcher reads at spawn. It missed ideProjectDir, ideOpenCommand and
autoCompactWindow, and changedDeferredKeys separately missed worktreeGroup
(baked into the same GitWorktrees as worktreeRoot, Fleetd.java:251). A
reload that changed only one of those keys reported "config reloaded" with
nothing deferred, and the running daemon kept the old value.

Fix the four instances, and add ConfigRefProfileCoverageTest: it enumerates
every FleetConfig.Profile record component by reflection, mutates each one
not in the new ConfigRef.LAUNCH_SETTINGS_EXCLUDED set on a base profile,
and asserts sameLaunchSettings actually notices — so a fifth missed field
fails the build by name instead of drifting silently. It also prints its
own denominator (26 components, 23 compared, 3 excluded) per the ticket's
requirement that a checker must be able to state what it checked.

Also add the `profile` field itself to the comparison (it was neither
compared nor excluded before this fix — the coverage test surfaced it).

Rewrote the sameLaunchSettings javadoc to describe what the coverage test
actually guarantees instead of repeating the unchecked claim.
ltms closed this pull request 2026-09-04 09:32:54 +02:00
Some checks are pending
CI / contract (pull_request) Successful in 1m11s
Details
CI / build (pull_request) Successful in 2m6s
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#325
Delete Branch "worker/fix-323-b8287d-13"

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?