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

exhaustionDetectionArmed reads the LIVE config while detection reads the STARTUP snapshot — after a reload it reports armed when detection is still off #404

New Issue
Closed
opened 2026-09-10 03:50:18 +02:00 by ltms · 1 comment
ltms commented 2026-09-10 03:50:18 +02:00
Owner
Copy Link
Copy Source

Regression introduced by #395, merged as 7180b1a. I merged it, and I missed this in review — I checked what the field means and read the report method in full, but I did not check which config snapshot the lookup reads. Found by the fleet01 lead pointing out that exhaustedPattern is deferred, not hot.

The two halves disagree

Detection is built once at startup from the startup snapshot (Fleetd.java:372-377):

// Compiled once at startup, keyed by profile name; a profile with no exhaustedPattern is
Map<String, Pattern> exhaustedPatternsByProfile = new LinkedHashMap<>();
...
        exhaustedPatternsByProfile.put(name, Pattern.compile(profile.exhaustedPattern()));

ConfigRef.java:599-602 says the same thing and is explicit that this is why the key is deferred:

CB-578 stage B: exhaustedPattern is compiled once into Fleetd.main's pattern map at startup (see ExhaustedPatternLookup wiring) — a reload never re-reads it, so a changed pattern must be reported as deferred, exactly like model/baseUrl/argv.

The new report field reads the live config instead (Fleetd.java:668-677):

}, quarantine, profile -> {
    // fleetd #395: read live off the current config, like credentialIdFor above — an
    // exhaustedPattern edit takes effect on the next fleet_profiles/fleet_list call, no
    // restart needed, same as the credential-id lookup it sits beside.
    var configured = config.get().profiles().get(profile);
    return configured != null && configured.hasExhaustedPattern();
});

The failure

  1. A profile has no exhaustedPattern. fleet_profiles correctly reports exhaustionDetectionArmed: false.
  2. The operator adds a pattern to fleetd.yaml and reloads.
  3. fleet_profiles now reports exhaustionDetectionArmed: true.
  4. CompletionResolver still has no pattern for that profile. A usage-limit refusal is still not classified, ExhaustionSink is still never called, quarantine() still never runs — until the daemon restarts.

So the field claims the profile is covered while it is not. That is the unsafe direction, and it defeats the entire point of the field: #395 exists so that free: N can be read correctly, and this makes it misreadable again in the one situation where an operator is actively trying to fix the gap.

The opposite edit (removing a pattern, then reloading) reports false while detection is still armed. That understates, so it is the harmless direction.

Why the comment's reasoning is wrong

The comment copies the neighbouring credentialIdFor lambda and argues "same as the credential-id lookup it sits beside". credentialId really is hot, so reading live is right for it. exhaustedPattern is not, so reading live is wrong here. Sitting next to a hot lookup does not make a deferred key hot. The two lambdas are adjacent and are the same shape, which is exactly what made the copy look safe.

There is also a second door that already gets this right: a reload that changes exhaustedPattern reports profiles as a changed deferred key, meaning "restart needed". So after step 2 above the daemon tells the operator "restart needed" through the reload report and "armed: true" through fleet_profiles. Two doors, opposite answers, same fact.

Fix

The field's name is a claim about detection, so it must answer from the same data detection uses: the startup snapshot, not config.get().

Read the profile off the startup snapshot cfg in that third lambda, the way exhaustedPatternsByProfile does. Better, derive it from exhaustedPatternsByProfile itself, so the field and the detection map cannot drift at all — one source, not two agreeing ones.

Do not make detection hot to match the field. That is a much larger change (recompiling patterns on reload, and ConfigRef's deferred triage would have to change with it), and it is not what this ticket is for.

Acceptance

  • After a reload that adds an exhaustedPattern to a profile, fleet_profiles and GET /profiles still report exhaustionDetectionArmed: false for it, because detection genuinely is still off.
  • After a reload that removes one, the field keeps reporting true until a restart, for the same reason.
  • A test pins the distinction: it must fail if the lookup is switched back to config.get(). A test that only checks a fresh daemon cannot catch this — the two snapshots are identical before any reload, which is why the whole suite passed.
  • The comment at Fleetd.java:669-671 is corrected, since it currently states the wrong rule as the justification.

Note for whoever fixes this

The reason this shipped green is worth keeping: at startup cfg and config.get() are the same object, so every existing test agrees under both readings. The bug needs a reload to become observable, and no test performed one. Same family as fleetd #398's unpinned startup calls.

Regression introduced by #395, merged as `7180b1a`. I merged it, and I missed this in review — I checked what the field means and read the report method in full, but I did not check which config snapshot the lookup reads. Found by the fleet01 lead pointing out that `exhaustedPattern` is deferred, not hot. ## The two halves disagree **Detection** is built once at startup from the startup snapshot (`Fleetd.java:372-377`): ```java // Compiled once at startup, keyed by profile name; a profile with no exhaustedPattern is Map<String, Pattern> exhaustedPatternsByProfile = new LinkedHashMap<>(); ... exhaustedPatternsByProfile.put(name, Pattern.compile(profile.exhaustedPattern())); ``` `ConfigRef.java:599-602` says the same thing and is explicit that this is why the key is deferred: > CB-578 stage B: exhaustedPattern is compiled once into Fleetd.main's pattern map at startup (see ExhaustedPatternLookup wiring) — a reload never re-reads it, so a changed pattern must be reported as deferred, exactly like model/baseUrl/argv. **The new report field** reads the live config instead (`Fleetd.java:668-677`): ```java }, quarantine, profile -> { // fleetd #395: read live off the current config, like credentialIdFor above — an // exhaustedPattern edit takes effect on the next fleet_profiles/fleet_list call, no // restart needed, same as the credential-id lookup it sits beside. var configured = config.get().profiles().get(profile); return configured != null && configured.hasExhaustedPattern(); }); ``` ## The failure 1. A profile has no `exhaustedPattern`. `fleet_profiles` correctly reports `exhaustionDetectionArmed: false`. 2. The operator adds a pattern to `fleetd.yaml` and reloads. 3. `fleet_profiles` now reports `exhaustionDetectionArmed: true`. 4. `CompletionResolver` still has **no pattern** for that profile. A usage-limit refusal is still not classified, `ExhaustionSink` is still never called, `quarantine()` still never runs — until the daemon restarts. So the field claims the profile is covered while it is not. That is the unsafe direction, and it defeats the entire point of the field: #395 exists so that `free: N` can be read correctly, and this makes it misreadable again in the one situation where an operator is actively trying to fix the gap. The opposite edit (removing a pattern, then reloading) reports `false` while detection is still armed. That understates, so it is the harmless direction. ## Why the comment's reasoning is wrong The comment copies the neighbouring `credentialIdFor` lambda and argues "same as the credential-id lookup it sits beside". `credentialId` really is hot, so reading live is right for it. `exhaustedPattern` is not, so reading live is wrong here. **Sitting next to a hot lookup does not make a deferred key hot.** The two lambdas are adjacent and are the same shape, which is exactly what made the copy look safe. There is also a second door that already gets this right: a reload that changes `exhaustedPattern` reports `profiles` as a changed **deferred** key, meaning "restart needed". So after step 2 above the daemon tells the operator "restart needed" through the reload report and "armed: true" through `fleet_profiles`. Two doors, opposite answers, same fact. ## Fix The field's name is a claim about **detection**, so it must answer from the same data detection uses: the startup snapshot, not `config.get()`. Read the profile off the startup snapshot `cfg` in that third lambda, the way `exhaustedPatternsByProfile` does. Better, derive it from `exhaustedPatternsByProfile` itself, so the field and the detection map cannot drift at all — one source, not two agreeing ones. Do **not** make detection hot to match the field. That is a much larger change (recompiling patterns on reload, and `ConfigRef`'s deferred triage would have to change with it), and it is not what this ticket is for. ## Acceptance - After a reload that adds an `exhaustedPattern` to a profile, `fleet_profiles` and `GET /profiles` still report `exhaustionDetectionArmed: false` for it, because detection genuinely is still off. - After a reload that removes one, the field keeps reporting `true` until a restart, for the same reason. - A test pins the distinction: it must fail if the lookup is switched back to `config.get()`. A test that only checks a fresh daemon cannot catch this — the two snapshots are identical before any reload, which is why the whole suite passed. - The comment at `Fleetd.java:669-671` is corrected, since it currently states the wrong rule as the justification. ## Note for whoever fixes this The reason this shipped green is worth keeping: at startup `cfg` and `config.get()` are the same object, so every existing test agrees under both readings. The bug needs a reload to become observable, and no test performed one. Same family as fleetd #398's unpinned startup calls.
ltms commented 2026-09-10 04:21:30 +02:00
Author
Owner
Copy Link
Copy Source

Merged as PR #406, with one test I added during review.

The fix

The armed lambda now reads the startup pattern map — exhaustedPatternsByProfile, the same map
CompletionResolver detection reads at Fleetd.java:380:

}, quarantine, profile -> startupExhaustedPatterns.containsKey(profile));

One source, not two that agree today. The wiring also moved into a package-private
Fleetd.quarantineSource(...) factory so a test can call the production code instead of
reimplementing it.

The first version of the test was sent back

It asserted on the text of Fleetd.java:

assertTrue(fleetdSource().contains("return exhaustedPatternsByProfile.containsKey(profile);"), ...);

A source-text assertion is not a behavioural pin. It fails on a reformat that changes nothing and
passes if the string survives inside a comment. It also cannot be the fix for a ticket about a
report that does not measure the real thing — it is the same mistake one layer up. The rework calls
Fleetd.quarantineSource(...) after a real config.reload(), which is the only shape that can
catch this: at startup cfg and config.get() are the same object, so a fresh-daemon test agrees
under both readings.

The mutation I ran, and the gap it found

The worker verified their own direction — switching the lookup back to live config fails the new
test. I checked the other direction:

}, quarantine, profile -> false); /* MUTANT F: armed permanently off */
→ Tests run: 1475, Failures: 0   BUILD SUCCESS

Not caught. The reason: the test only ever passed Map.of() — an empty startup map — so
containsKey returning false was indistinguishable from a lambda that always returns false. The
only other test touching this field, FleetProfilesArmedFieldTest, asserts that the string
"exhaustionDetectionArmed" appears in the JSON. It never looks at the value.

So the suite would have stayed green with #395's visibility feature permanently dead. And note
which way that fails: an operator who adds an exhaustedPattern to fix a detection gap, restarts,
and checks fleet_profiles would be told the gap is still open — forever. The field added to make
the gap visible would have started hiding the fix.

I added aProfileInTheStartupMapIsArmed, which passes a non-empty map and asserts both directions —
terra present ⇒ armed, sonnet absent ⇒ not armed. Re-running mutant F:

→ Tests run: 1476, Failures: 1   BUILD FAILURE
   FleetdExhaustionDetectionArmedWiringTest.aProfileInTheStartupMapIsArmed:89
     a profile whose pattern was compiled at startup must report armed
     ==> expected: <true> but was: <false>

One failure, and it is the new test. That is the proof it was the only thing covering this
direction. Clean build after restoring: Tests run: 1476, Failures: 0.

What I take from this one

A one-directional test on a boolean is half a test, and which half you write is decided by the
fixture you reach for. Both tests here are honest tests of real behaviour; the empty-map fixture was
just the convenient one for the bug in hand, and it happened to make the other direction
unobservable.

The original defect has its own lesson, already recorded: the wrong lambda sat directly beside a
correct one of identical shape and copied its comment verbatim. credentialId is genuinely hot, so
live is right there. Sitting next to a hot lookup does not make a deferred key hot. I merged
#395 with this defect in it — I checked what the field means and read the reporter in full, and
never asked which snapshot the third lambda read.

Still open, filed separately

Fleetd.main has other local-capturing lambdas the worker left alone and named:
exhaustedPatterns, exhaustionSink, outageSource. I have not audited those for the same
hot/deferred mismatch. The five unpinned log-only startup reporters are #407 and the five
exit-code-trusted-as-effect sites are #408.

Merged as PR #406, with one test I added during review. ## The fix The armed lambda now reads the **startup pattern map** — `exhaustedPatternsByProfile`, the same map `CompletionResolver` detection reads at `Fleetd.java:380`: ```java }, quarantine, profile -> startupExhaustedPatterns.containsKey(profile)); ``` One source, not two that agree today. The wiring also moved into a package-private `Fleetd.quarantineSource(...)` factory so a test can call the production code instead of reimplementing it. ## The first version of the test was sent back It asserted on the **text of `Fleetd.java`**: ```java assertTrue(fleetdSource().contains("return exhaustedPatternsByProfile.containsKey(profile);"), ...); ``` A source-text assertion is not a behavioural pin. It fails on a reformat that changes nothing and passes if the string survives inside a comment. It also cannot be the fix for a ticket about a report that does not measure the real thing — it is the same mistake one layer up. The rework calls `Fleetd.quarantineSource(...)` after a real `config.reload()`, which is the only shape that can catch this: at startup `cfg` and `config.get()` are the same object, so a fresh-daemon test agrees under both readings. ## The mutation I ran, and the gap it found The worker verified their own direction — switching the lookup back to live config fails the new test. I checked the other direction: ``` }, quarantine, profile -> false); /* MUTANT F: armed permanently off */ → Tests run: 1475, Failures: 0 BUILD SUCCESS ``` **Not caught.** The reason: the test only ever passed `Map.of()` — an empty startup map — so `containsKey` returning false was indistinguishable from a lambda that always returns false. The only other test touching this field, `FleetProfilesArmedFieldTest`, asserts that the string `"exhaustionDetectionArmed"` appears in the JSON. It never looks at the value. So the suite would have stayed green with #395's visibility feature **permanently dead**. And note which way that fails: an operator who adds an `exhaustedPattern` to fix a detection gap, restarts, and checks `fleet_profiles` would be told the gap is still open — forever. The field added to make the gap visible would have started hiding the fix. I added `aProfileInTheStartupMapIsArmed`, which passes a non-empty map and asserts both directions — `terra` present ⇒ armed, `sonnet` absent ⇒ not armed. Re-running mutant F: ``` → Tests run: 1476, Failures: 1 BUILD FAILURE FleetdExhaustionDetectionArmedWiringTest.aProfileInTheStartupMapIsArmed:89 a profile whose pattern was compiled at startup must report armed ==> expected: <true> but was: <false> ``` One failure, and it is the new test. That is the proof it was the only thing covering this direction. Clean build after restoring: `Tests run: 1476, Failures: 0`. ## What I take from this one **A one-directional test on a boolean is half a test**, and which half you write is decided by the fixture you reach for. Both tests here are honest tests of real behaviour; the empty-map fixture was just the convenient one for the bug in hand, and it happened to make the other direction unobservable. The original defect has its own lesson, already recorded: the wrong lambda sat directly beside a correct one of identical shape and copied its comment verbatim. `credentialId` is genuinely hot, so live is right there. **Sitting next to a hot lookup does not make a deferred key hot.** I merged #395 with this defect in it — I checked what the field means and read the reporter in full, and never asked which snapshot the third lambda read. ## Still open, filed separately `Fleetd.main` has other local-capturing lambdas the worker left alone and named: `exhaustedPatterns`, `exhaustionSink`, `outageSource`. I have not audited those for the same hot/deferred mismatch. The five unpinned log-only startup reporters are #407 and the five exit-code-trusted-as-effect sites are #408.
ltms closed this issue 2026-09-10 04:21:58 +02:00
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#404
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?