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

fleetd#330: split reload class for health/coordinator + top-level coverage #331

Closed
agent wants to merge 0 commits from worker/fleetd-330-2770fb-17 into main
pull from: worker/fleetd-330-2770fb-17
merge into: fleet:main
Conversation 0 Commits - Files Changed -
agent commented 2026-09-04 10:17:42 +02:00
Member
Copy Link
Copy Source

Closes fleetd#330.

What changed

Unit 1 — the split reload class in ConfigRef. health: and coordinator: are each read
both off the startup snapshot (to build a long-lived object) and live off config.get() at a
different site, so neither the hot, deferred, nor cold class fit them as a whole. Added:

  • ConfigRef.SPLIT_KEYS = Set.of("health", "coordinator").
  • changedSplitKeys(old, fresh) — any change to health: or coordinator: (as a whole record,
    via Objects.equals) produces one fixed, descriptive message naming both halves — which part is
    frozen at startup and needs a restart, and which part is read live and already applied. No
    per-sub-field logic, per the issue's rejection of that as "more machinery than the problem
    needs."
  • Outcome gets a new split field, kept separate from deferred rather than folded into it
    with different wording. Decision + reason (also in the record's javadoc): every deferred entry
    means "this key's whole change waits for a restart"; every split entry means "part already
    applied, part waits, the message says which." A caller branching on the list (not just printing
    summary()) needs that distinction typed, not buried in a string it would have to re-parse.
  • Outcome.applied() stays true for a split change (invariant 3) and a split change never
    refuses the reload (invariant 2) — only changedColdKeys gates the refusal.
  • summary() extended to append "; partially live — <messages>" when split is non-empty,
    alongside the existing "; these changes need a restart to take effect: <keys>" for deferred.
    Both can appear together. The empty-deferred-and-split case is untouched — still exactly
    "config reloaded".
  • Class doc: "three classes" → "four classes"; new Split bullet with the exact facts for each
    key (call sites, selfId's cross-daemon-identity consequence, the URI env-var name that stays
    live for MemberEnvAllowList); denominator note no longer calls health/coordinator
    "undecided" — it now points at the Split bullet and at ConfigRefTopLevelCoverageTest.
  • COLD_KEYS visibility widened from private to package-private (matching
    LAUNCH_SETTINGS_EXCLUDED's existing visibility) so the new top-level coverage test can read it.

Unit 2 — ConfigRefTopLevelCoverageTest. Enumerates FleetConfig's 22 top-level record
components by reflection and requires each to sit in exactly one of four buckets:
ConfigRef.COLD_KEYS, a pinned DEFERRED_TOP_LEVEL_KEYS set (components changedDeferredKeys
actually reads — verified by reading that method), ConfigRef.SPLIT_KEYS, or a pinned
HOT_EXCLUDED_TOP_LEVEL_KEYS escape hatch (components read live off the config supplier, with a
citation per entry). It prints its own denominator on every run and pins the hot-exclusion set's
exact contents with assertEquals, the same shape ConfigRefProfileCoverageTest already uses for
LAUNCH_SETTINGS_EXCLUDED. ConfigRefProfileCoverageTest itself is untouched.

The test's javadoc states its limit up front: it proves the record's shape is fully triaged, it
cannot prove any citation is true — that a "compared in changedDeferredKeys" or "read live"
comment still matches the code it describes is a fact this reflection-only test has no way to
inspect.

A verified deviation from the issue's starting values

The issue lists profiles under Hot in its known-good buckets. I moved it to deferred
instead, because I checked the code rather than copying the list:

  • changedDeferredKeys (ConfigRef.java:295-386) demonstrably reads old.profiles() /
    fresh.profiles() and reports both added/removed profiles and an existing profile's changed
    launch settings.
  • The HOT_EXCLUDED_TOP_LEVEL_KEYS javadoc requires each entry's citation to be true — "read live
    off the config supplier." That is false for profiles as a whole: only weight, maxLoad and
    credentialId are read live (and that sub-field split is already covered, separately and more
    precisely, by ConfigRefProfileCoverageTest). Citing the whole key as hot-excluded would violate
    the very rule the escape hatch pin exists to enforce.

Net result: cold=5, deferred=11 (not 10), split=2, hot=4 (not 5), total=22. The total the issue
named (22) checked out exactly; only the deferred/hot split moved by one key, and I traced why
rather than trusting the list.

A caveat surfaced, not acted on (out of scope)

fleet: has the same shape of asymmetry as health/coordinator — fleet.leaders is read only
at startup (per the class doc's existing Hot bullet) and genuinely needs a restart, while the rest
of fleet: (role pools, charters, tabLabel) is read live. The issue's own fact-find explicitly
scoped split to exactly health/coordinator ("the seven readers... are the complete set... do
not re-derive"), so I kept fleet in the hot-exclusion set as directed and documented the caveat
in that set's javadoc rather than expanding scope. Flagging it here in case it's worth its own
ticket later.

Mutation proofs (each reverted, verified failing, restored, verified green — in that order)

Unit 1 — reverted changedSplitKeys(old, fresh) to List.of() in reload():

[ERROR] Tests run: 24, Failures: 4, Errors: 0, Skipped: 0
ConfigRefTest.aSplitChangeAndADeferredChangeCoexist:715 [] ==> expected: <1> but was: <0>
ConfigRefTest.changingBothSplitKeysReportsBoth:684 [] ==> expected: <2> but was: <0>
ConfigRefTest.changingCoordinatorIsReportedAsSplit:632 [] ==> expected: <1> but was: <0>
ConfigRefTest.changingHealthIsReportedAsSplit:600 [] ==> expected: <1> but was: <0>

Restored → Tests run: 24, Failures: 0, Errors: 0, Skipped: 0 / BUILD SUCCESS.

Unit 2, mutation A (untriaged component) — removed "guard" from DEFERRED_TOP_LEVEL_KEYS
without adding it anywhere else:

[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0
AssertionFailedError: these FleetConfig components are in none of COLD_KEYS,
DEFERRED_TOP_LEVEL_KEYS, SPLIT_KEYS or HOT_EXCLUDED_TOP_LEVEL_KEYS — triage each one
into whichever actually describes it: [guard] ==> expected: <[]> but was: <[guard]>

Restored → green.

Unit 2, mutation B (escape-hatch abuse, required by the issue) — moved "guard" (a genuinely
compared key) from DEFERRED_TOP_LEVEL_KEYS into HOT_EXCLUDED_TOP_LEVEL_KEYS:

[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0
AssertionFailedError: HOT_EXCLUDED_TOP_LEVEL_KEYS changed. A component belongs here ONLY
if it is read live off the config supplier, never because adding it makes this test pass.
... ==> expected: <[memberLoginShell, memberCredentials, placement, fleet]>
    but was: <[guard, placement, memberLoginShell, fleet, memberCredentials]>

Restored → green. The pinned assertion catches the escape hatch being grown to silence the test,
exactly as #323 asked for.

Build

cd fleetd && mvn clean install (unpiped, full output read):

[INFO] Tests run: 1348, Failures: 0, Errors: 0, Skipped: 0
...
[INFO] BUILD SUCCESS
[INFO] Total time:  51.151 s

Includes ConfigRefTest (24), ConfigRefTopLevelCoverageTest (1, prints:
FleetConfig top-level coverage — 22 components total: 5 cold [...], 11 deferred [...], 2 split [health, coordinator], 4 hot-excluded [...]), and the untouched ConfigRefProfileCoverageTest (1).
A handful of ERROR-level log lines appear mid-run from OpenCodeLauncherTest and
FleetdBackendErrorSinkTest — those are deliberately-simulated backend-error scenarios the tests
assert on, not real failures; the Tests run:/BUILD SUCCESS lines above are the real result.

Constraints honored

  • No key made to take effect live — this is reload reporting only.
  • MemberEnvAllowList untouched; no hole found in it beyond the one #326 already documented and
    this issue explicitly closed (the uriEnv name exposure caveat).
  • ConfigRefProfileCoverageTest's exclusion-set assertion (LAUNCH_SETTINGS_EXCLUDED) not touched.
  • No git stash, no git worktree remove/prune, no git add -A, no merge.
  • fleetd/fleetd.yaml not read or reported on (gitignored, absent from this worktree).

Files changed

  • fleetd/src/main/java/dev/ltms/fleet/config/ConfigRef.java
  • fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTest.java
  • fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTopLevelCoverageTest.java (new)
Closes fleetd#330. ## What changed **Unit 1 — the `split` reload class in `ConfigRef`.** `health:` and `coordinator:` are each read both off the startup snapshot (to build a long-lived object) and live off `config.get()` at a different site, so neither the hot, deferred, nor cold class fit them as a whole. Added: - `ConfigRef.SPLIT_KEYS = Set.of("health", "coordinator")`. - `changedSplitKeys(old, fresh)` — any change to `health:` or `coordinator:` (as a whole record, via `Objects.equals`) produces one fixed, descriptive message naming both halves — which part is frozen at startup and needs a restart, and which part is read live and already applied. No per-sub-field logic, per the issue's rejection of that as "more machinery than the problem needs." - `Outcome` gets a new `split` field, kept **separate from `deferred`** rather than folded into it with different wording. Decision + reason (also in the record's javadoc): every `deferred` entry means "this key's whole change waits for a restart"; every `split` entry means "part already applied, part waits, the message says which." A caller branching on the list (not just printing `summary()`) needs that distinction typed, not buried in a string it would have to re-parse. - `Outcome.applied()` stays `true` for a split change (invariant 3) and a split change never refuses the reload (invariant 2) — only `changedColdKeys` gates the refusal. - `summary()` extended to append `"; partially live — <messages>"` when `split` is non-empty, alongside the existing `"; these changes need a restart to take effect: <keys>"` for `deferred`. Both can appear together. The empty-deferred-and-split case is untouched — still exactly `"config reloaded"`. - Class doc: "three classes" → "four classes"; new **Split** bullet with the exact facts for each key (call sites, `selfId`'s cross-daemon-identity consequence, the URI env-var name that stays live for `MemberEnvAllowList`); denominator note no longer calls `health`/`coordinator` "undecided" — it now points at the Split bullet and at `ConfigRefTopLevelCoverageTest`. - `COLD_KEYS` visibility widened from `private` to package-private (matching `LAUNCH_SETTINGS_EXCLUDED`'s existing visibility) so the new top-level coverage test can read it. **Unit 2 — `ConfigRefTopLevelCoverageTest`.** Enumerates `FleetConfig`'s 22 top-level record components by reflection and requires each to sit in **exactly one** of four buckets: `ConfigRef.COLD_KEYS`, a pinned `DEFERRED_TOP_LEVEL_KEYS` set (components `changedDeferredKeys` actually reads — verified by reading that method), `ConfigRef.SPLIT_KEYS`, or a pinned `HOT_EXCLUDED_TOP_LEVEL_KEYS` escape hatch (components read live off the config supplier, with a citation per entry). It prints its own denominator on every run and pins the hot-exclusion set's exact contents with `assertEquals`, the same shape `ConfigRefProfileCoverageTest` already uses for `LAUNCH_SETTINGS_EXCLUDED`. `ConfigRefProfileCoverageTest` itself is untouched. The test's javadoc states its limit up front: it proves the record's *shape* is fully triaged, it cannot prove any citation is *true* — that a "compared in changedDeferredKeys" or "read live" comment still matches the code it describes is a fact this reflection-only test has no way to inspect. ### A verified deviation from the issue's starting values The issue lists `profiles` under **Hot** in its known-good buckets. I moved it to **deferred** instead, because I checked the code rather than copying the list: - `changedDeferredKeys` (`ConfigRef.java:295-386`) demonstrably reads `old.profiles()` / `fresh.profiles()` and reports both added/removed profiles and an existing profile's changed launch settings. - The `HOT_EXCLUDED_TOP_LEVEL_KEYS` javadoc requires each entry's citation to be true — "read live off the config supplier." That is false for `profiles` as a whole: only `weight`, `maxLoad` and `credentialId` are read live (and that sub-field split is already covered, separately and more precisely, by `ConfigRefProfileCoverageTest`). Citing the whole key as hot-excluded would violate the very rule the escape hatch pin exists to enforce. Net result: **cold=5, deferred=11 (not 10), split=2, hot=4 (not 5), total=22.** The total the issue named (22) checked out exactly; only the deferred/hot split moved by one key, and I traced why rather than trusting the list. ### A caveat surfaced, not acted on (out of scope) `fleet:` has the same *shape* of asymmetry as `health`/`coordinator` — `fleet.leaders` is read only at startup (per the class doc's existing Hot bullet) and genuinely needs a restart, while the rest of `fleet:` (role pools, `charters`, `tabLabel`) is read live. The issue's own fact-find explicitly scoped `split` to exactly `health`/`coordinator` ("the seven readers... are the complete set... do not re-derive"), so I kept `fleet` in the hot-exclusion set as directed and documented the caveat in that set's javadoc rather than expanding scope. Flagging it here in case it's worth its own ticket later. ## Mutation proofs (each reverted, verified failing, restored, verified green — in that order) **Unit 1** — reverted `changedSplitKeys(old, fresh)` to `List.of()` in `reload()`: ``` [ERROR] Tests run: 24, Failures: 4, Errors: 0, Skipped: 0 ConfigRefTest.aSplitChangeAndADeferredChangeCoexist:715 [] ==> expected: <1> but was: <0> ConfigRefTest.changingBothSplitKeysReportsBoth:684 [] ==> expected: <2> but was: <0> ConfigRefTest.changingCoordinatorIsReportedAsSplit:632 [] ==> expected: <1> but was: <0> ConfigRefTest.changingHealthIsReportedAsSplit:600 [] ==> expected: <1> but was: <0> ``` Restored → `Tests run: 24, Failures: 0, Errors: 0, Skipped: 0` / `BUILD SUCCESS`. **Unit 2, mutation A (untriaged component)** — removed `"guard"` from `DEFERRED_TOP_LEVEL_KEYS` without adding it anywhere else: ``` [ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0 AssertionFailedError: these FleetConfig components are in none of COLD_KEYS, DEFERRED_TOP_LEVEL_KEYS, SPLIT_KEYS or HOT_EXCLUDED_TOP_LEVEL_KEYS — triage each one into whichever actually describes it: [guard] ==> expected: <[]> but was: <[guard]> ``` Restored → green. **Unit 2, mutation B (escape-hatch abuse, required by the issue)** — moved `"guard"` (a genuinely compared key) from `DEFERRED_TOP_LEVEL_KEYS` into `HOT_EXCLUDED_TOP_LEVEL_KEYS`: ``` [ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0 AssertionFailedError: HOT_EXCLUDED_TOP_LEVEL_KEYS changed. A component belongs here ONLY if it is read live off the config supplier, never because adding it makes this test pass. ... ==> expected: <[memberLoginShell, memberCredentials, placement, fleet]> but was: <[guard, placement, memberLoginShell, fleet, memberCredentials]> ``` Restored → green. The pinned assertion catches the escape hatch being grown to silence the test, exactly as #323 asked for. ## Build `cd fleetd && mvn clean install` (unpiped, full output read): ``` [INFO] Tests run: 1348, Failures: 0, Errors: 0, Skipped: 0 ... [INFO] BUILD SUCCESS [INFO] Total time: 51.151 s ``` Includes `ConfigRefTest` (24), `ConfigRefTopLevelCoverageTest` (1, prints: `FleetConfig top-level coverage — 22 components total: 5 cold [...], 11 deferred [...], 2 split [health, coordinator], 4 hot-excluded [...]`), and the untouched `ConfigRefProfileCoverageTest` (1). A handful of `ERROR`-level log lines appear mid-run from `OpenCodeLauncherTest` and `FleetdBackendErrorSinkTest` — those are deliberately-simulated backend-error scenarios the tests assert on, not real failures; the `Tests run:`/`BUILD SUCCESS` lines above are the real result. ## Constraints honored - No key made to take effect live — this is reload *reporting* only. - `MemberEnvAllowList` untouched; no hole found in it beyond the one #326 already documented and this issue explicitly closed (the `uriEnv` name exposure caveat). - `ConfigRefProfileCoverageTest`'s exclusion-set assertion (`LAUNCH_SETTINGS_EXCLUDED`) not touched. - No `git stash`, no `git worktree remove`/`prune`, no `git add -A`, no merge. - `fleetd/fleetd.yaml` not read or reported on (gitignored, absent from this worktree). ## Files changed - `fleetd/src/main/java/dev/ltms/fleet/config/ConfigRef.java` - `fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTest.java` - `fleetd/src/test/java/dev/ltms/fleet/config/ConfigRefTopLevelCoverageTest.java` (new)
agent added 1 commit 2026-09-04 10:17:43 +02:00
fleetd#330: split reload class for health/coordinator + top-level coverage
CI / contract (pull_request) Successful in 53s
Details
CI / build (pull_request) Successful in 2m17s
Details
554395b104 
Unit 1: ConfigRef gets a fourth reload class, `split`, for keys read both
off the startup snapshot and live off config.get() at different sites
(health:, coordinator:). A split change is accepted (Outcome.applied()
stays true) and reported by name, naming which half is live and which
needs a restart, via a new Outcome.split() field kept separate from
deferred() since the two carry different guarantees for any caller that
branches on them, not just prose in summary(). Class doc updated: four
classes now, denominator note no longer calls health/coordinator
undecided.

Unit 2: ConfigRefTopLevelCoverageTest enumerates FleetConfig's 22
top-level record components and requires each to sit in exactly one of
COLD_KEYS, a pinned "compared in changedDeferredKeys" set, SPLIT_KEYS, or
a pinned hot-exclusion escape hatch — printing its own denominator and
pinning the escape hatch's exact contents the way #323 asked for.

Deviates from the issue's starting values by one key: `profiles` moves
from the suggested Hot bucket into the deferred bucket, because
changedDeferredKeys demonstrably compares it (add/remove and launch
settings), and citing "read live off the config supplier" for the whole
key would be false — most Profile fields are not read live, only
weight/maxLoad/credentialId are (and those are already covered by
ConfigRefProfileCoverageTest). Cold=5, split=2, deferred=11, hot=4,
total=22 — verified against the record and against ConfigRef's code, not
copied from the issue.
ltms closed this pull request 2026-09-04 10:22:55 +02:00
Some checks are pending
CI / contract (pull_request) Successful in 53s
Details
CI / build (pull_request) Successful in 2m17s
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#331
Delete Branch "worker/fleetd-330-2770fb-17"

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?