Backend-error classification needs a real mechanism, not hard-coded strings (#164 points 3 and 4) #201

Closed
opened 2026-08-31 05:57:59 +02:00 by ltms · 3 comments
Owner

Follow-up to #164, which closed with points 1 and 2 done (3bfa828, 4ac688b). Points 3 and 4 stay open and would otherwise be lost with that close.

Where things stand

CompletionResolver now classifies a completed-but-unreplied turn through a fixed chain:

  1. too fast (MIN_TURN_NANOS, 2s) → FAILED
  2. empty or unreadable scrape → FAILED
  3. misattribution guard (scrape identical to the delivery baseline) → suppress
  4. profile's configured exhaustedPattern → BACKEND_EXHAUSTED (+ credential quarantine)
  5. hard-coded BACKEND_ERROR = (?i)\bAPI Error\s*: → FAILED
  6. otherwise → resolve as a completion

Step 5 is the part that needs replacing.

Point 3 — surface backend errors properly

The current pattern is one hard-coded string, and that was a deliberate stopgap, not a design. It fails in the silent direction: when a backend rewords its error, the pattern stops matching and the scrape goes back to resolving as a success. That is the exact defect #164 was filed about, reintroduced by drift, with nothing to signal it.

Step 4 already shows the shape this should take. exhaustedPattern is per profile and configured, so a backend's wording lives next to that backend's definition, and CompletionResolver.coverage(...) logs at startup which profiles have one and which do not. Step 5 has neither.

Suggested direction:

  • Add a per-profile errorPattern beside the existing exhaustedPattern, resolved through the same ExhaustedPatternLookup-style seam.
  • Extend the startup coverage line to report both, so an operator can see that classification is off for a profile instead of discovering it from a lost turn.
  • Keep the built-in (?i)\bAPI Error\s*: as the default when a profile configures nothing, so removing the hard-coded string never silently weakens an unconfigured profile.

Note the ordering constraint: exhaustion must keep winning over a generic error, because only BACKEND_EXHAUSTED quarantines the credential. A merged pattern would lose that.

The dead-code question that belongs here

The rescued branch (851ebca) carried a visibleTurn raw-screen fallback, so a TUI-hidden error line could still classify when lastAssistantBlock parses blank. On the current chain the step-2 empty fail fires first, so that fallback can never run.

Making it live means reordering the chain — moving pattern checks ahead of the empty check. That changes which classification wins for a whole class of turns, so it needs deciding here rather than being slipped into an unrelated change. It was correctly left out of #164.

Point 4 — quarantine a profile whose first turn fails

From #164: a profile whose first turn 400s stays ready in /members and keeps accepting sends. Every send burns a spawn and returns a failure.

The machinery already exists — ExhaustionSink quarantines a credential on BACKEND_EXHAUSTED, and fleet_list reports free: 0 with credentialId and quarantinedForSeconds. This is about deciding whether a hard backend rejection should feed the same sink, and if so with what backoff.

It is not obviously a yes. Exhaustion is a known-temporary refusal with a natural retry window; a 400 on a malformed request body is a permanent config error that will fail identically on every retry, so a timed quarantine is the wrong shape for it. A profile marked unusable until its config changes may fit better than one that silently comes back and fails again.

Why this is not urgent

The costly half of #164 is fixed: a lost turn can no longer reach a caller labelled as a successful empty reply. What remains is coverage and rot-resistance — real, but it degrades gracefully, and the failure is now a wrong classification rather than a silent data loss.

Follow-up to #164, which closed with points 1 and 2 done (`3bfa828`, `4ac688b`). Points 3 and 4 stay open and would otherwise be lost with that close. ## Where things stand `CompletionResolver` now classifies a completed-but-unreplied turn through a fixed chain: 1. too fast (`MIN_TURN_NANOS`, 2s) → `FAILED` 2. empty or unreadable scrape → `FAILED` 3. misattribution guard (scrape identical to the delivery baseline) → suppress 4. profile's configured `exhaustedPattern` → `BACKEND_EXHAUSTED` (+ credential quarantine) 5. **hard-coded `BACKEND_ERROR` = `(?i)\bAPI Error\s*:`** → `FAILED` 6. otherwise → resolve as a completion Step 5 is the part that needs replacing. ## Point 3 — surface backend errors properly The current pattern is one hard-coded string, and that was a deliberate stopgap, not a design. It fails in the **silent** direction: when a backend rewords its error, the pattern stops matching and the scrape goes back to resolving as a success. That is the exact defect #164 was filed about, reintroduced by drift, with nothing to signal it. Step 4 already shows the shape this should take. `exhaustedPattern` is **per profile and configured**, so a backend's wording lives next to that backend's definition, and `CompletionResolver.coverage(...)` logs at startup which profiles have one and which do not. Step 5 has neither. Suggested direction: - Add a per-profile `errorPattern` beside the existing `exhaustedPattern`, resolved through the same `ExhaustedPatternLookup`-style seam. - Extend the startup coverage line to report both, so an operator can see that classification is **off** for a profile instead of discovering it from a lost turn. - Keep the built-in `(?i)\bAPI Error\s*:` as the default when a profile configures nothing, so removing the hard-coded string never silently weakens an unconfigured profile. Note the ordering constraint: exhaustion must keep winning over a generic error, because only `BACKEND_EXHAUSTED` quarantines the credential. A merged pattern would lose that. ### The dead-code question that belongs here The rescued branch (`851ebca`) carried a `visibleTurn` raw-screen fallback, so a TUI-hidden error line could still classify when `lastAssistantBlock` parses blank. On the current chain the step-2 empty fail fires first, so that fallback can never run. Making it live means reordering the chain — moving pattern checks ahead of the empty check. That changes which classification wins for a whole class of turns, so it needs deciding here rather than being slipped into an unrelated change. It was correctly left out of #164. ## Point 4 — quarantine a profile whose first turn fails From #164: a profile whose first turn 400s stays `ready` in `/members` and keeps accepting sends. Every send burns a spawn and returns a failure. The machinery already exists — `ExhaustionSink` quarantines a credential on `BACKEND_EXHAUSTED`, and `fleet_list` reports `free: 0` with `credentialId` and `quarantinedForSeconds`. This is about deciding whether a **hard backend rejection** should feed the same sink, and if so with what backoff. It is not obviously a yes. Exhaustion is a known-temporary refusal with a natural retry window; a 400 on a malformed request body is a **permanent** config error that will fail identically on every retry, so a timed quarantine is the wrong shape for it. A profile marked unusable until its config changes may fit better than one that silently comes back and fails again. ## Why this is not urgent The costly half of #164 is fixed: a lost turn can no longer reach a caller labelled as a successful empty reply. What remains is coverage and rot-resistance — real, but it degrades gracefully, and the failure is now a wrong classification rather than a silent data loss.
Author
Owner

Refined into five units — design is on main

An architect member read this issue, #227, and the named source, and split the work. Full design: docs/CB-201-227-Refinement.md (merged to main as 7662e2d).

#201 and #227 are one delivery program, but not one implementation unit. CompletionResolver can publish a typed backend-error event once its captured waiter resolution wins; #227 consumes that event without knowing any pane text. The classifier must land before the final wiring, but the policy, the roster state and the lead nudge can be built beside it.

Unit 1: typed classification -----------+
Unit 2: credential outage policy -------+
Unit 3: lead outage nudge --------------+--> Unit 5: production wiring and views
Unit 4: durable member outcome ---------+
#234 defect 2 fix ----------------------+

Units 1 to 4 have disjoint file ownership. Unit 5 owns every composition file, including Fleetd.java, and starts only after Units 1 to 4 and #234 land.

Policy — first values

Knob Value
Threshold 2 classified backend errors
Window 60 seconds, inclusive
Cool-off 60 seconds from threshold crossing
Correlation key credentialId — never profile name, never error text

One active incident per credential. Errors during cool-off neither extend it nor raise another notice. After expiry, two fresh errors are needed to rearm. A single error still fails its send and marks that member backend_error; it does not cool the credential.

Two is the smallest threshold that protects an honest one-turn failure. 60 seconds fits the measured two-member outage. A 60-second cool-off is deliberately not the 1800-second exhaustion quarantine — a short fault is not a spent credential.

Findings that changed the plan

  1. The "dead-code question" in this issue is stale. #211 already added raw-screen classification (CompletionResolver.classifyRawScrapeFallback, :260-276, :332-367). Do not rebuild or reorder it.
  2. The two-second floor still has a real gap. CompletionResolver.resolve fails at :229-237 before the normal pattern checks, so a fast backend failure is only a generic failure today. The classifier must cover that path.
  3. Exhaustion already wins over generic backend error at :288-317. That order stays.
  4. The code already admits the error match is a heuristic (:311-316) — a valid member report can quote an API Error: line. This is the main policy risk once two matches remove capacity.
  5. CB-588 nudges for every terminal async ticket (MessageService.java:922-940), but would not say those failures are one outage.
  6. ReplyPushLoop already combines replies, terminal tickets and questions on one per-lead schedule (:20-48, :305-395). A new direct injector would race it.
  7. BackendQuarantine restarts a long cooldown per exhaustion (:60-87) — wrong store and wrong name for a short outage.
  8. Placement knows only quarantined (PlacementContext.java:10-22) — reusing it would make refusal text say "backend exhausted".
  9. MemberSession has DONE and generic FAILED, no reason (:51-59); rosterView cannot preserve the cause after the ticket is gone (:648-687).
  10. The async resolver races SessionManager.onTurnComplete (:715-755), so a backend-error update must handle both BUSY and DONE.
  11. FleetMcp.capacityView already shows the right shape for exhaustion (:996-1025); outage needs parallel fields with different names.
  12. FleetHealthMonitor.healthCoverage (:206-208) must not change to full because of this.

Riskiest assumption

That a configured regex means the backend failed this turn. Two false matches would cool a healthy credential. Cheapest experiment: replay the saved 2026-09-01 pane, one deliberate disposable failure per adapter, and one valid member report quoting each error line, through the real CompletionResolver fixture. Outage panes must match; quoted reports must not. If quoted reports still match, narrow the patterns — do not raise the threshold to hide weak classification.

Explicitly not building

Reuse of BackendQuarantine; merged exhaustion/error patterns; grouping by error text; marking a profile permanently unusable (the classifier cannot yet separate malformed input from a transient fault); cooling on one generic error; putting the detector in FleetHealthMonitor; a second lead injector; persisting incident history across restart; work recovery; the old visibleTurn direction (#211 superseded it); and removing the legacy API Error: fallback in the first release.

Units 1–4 are now delegated in parallel.

## Refined into five units — design is on main An architect member read this issue, #227, and the named source, and split the work. Full design: `docs/CB-201-227-Refinement.md` (merged to main as `7662e2d`). **#201 and #227 are one delivery program, but not one implementation unit.** `CompletionResolver` can publish a typed backend-error event once its captured waiter resolution wins; #227 consumes that event without knowing any pane text. The classifier must land before the final wiring, but the policy, the roster state and the lead nudge can be built beside it. ``` Unit 1: typed classification -----------+ Unit 2: credential outage policy -------+ Unit 3: lead outage nudge --------------+--> Unit 5: production wiring and views Unit 4: durable member outcome ---------+ #234 defect 2 fix ----------------------+ ``` Units 1 to 4 have disjoint file ownership. Unit 5 owns every composition file, including `Fleetd.java`, and starts only after Units 1 to 4 and #234 land. ### Policy — first values | Knob | Value | |---|---| | Threshold | 2 classified backend errors | | Window | 60 seconds, inclusive | | Cool-off | 60 seconds from threshold crossing | | Correlation key | `credentialId` — never profile name, never error text | One active incident per credential. Errors during cool-off neither extend it nor raise another notice. After expiry, two fresh errors are needed to rearm. A single error still fails its send and marks that member `backend_error`; it does not cool the credential. Two is the smallest threshold that protects an honest one-turn failure. 60 seconds fits the measured two-member outage. A 60-second cool-off is deliberately **not** the 1800-second exhaustion quarantine — a short fault is not a spent credential. ### Findings that changed the plan 1. **The "dead-code question" in this issue is stale.** #211 already added raw-screen classification (`CompletionResolver.classifyRawScrapeFallback`, `:260-276`, `:332-367`). Do not rebuild or reorder it. 2. **The two-second floor still has a real gap.** `CompletionResolver.resolve` fails at `:229-237` before the normal pattern checks, so a fast backend failure is only a generic failure today. The classifier must cover that path. 3. Exhaustion already wins over generic backend error at `:288-317`. That order stays. 4. The code already admits the error match is a heuristic (`:311-316`) — a valid member report can quote an `API Error:` line. This is the main policy risk once two matches remove capacity. 5. CB-588 nudges for every terminal async ticket (`MessageService.java:922-940`), but would not say those failures are **one** outage. 6. `ReplyPushLoop` already combines replies, terminal tickets and questions on one per-lead schedule (`:20-48`, `:305-395`). A new direct injector would race it. 7. `BackendQuarantine` restarts a long cooldown per exhaustion (`:60-87`) — wrong store and wrong name for a short outage. 8. Placement knows only `quarantined` (`PlacementContext.java:10-22`) — reusing it would make refusal text say "backend exhausted". 9. `MemberSession` has `DONE` and generic `FAILED`, no reason (`:51-59`); `rosterView` cannot preserve the cause after the ticket is gone (`:648-687`). 10. The async resolver races `SessionManager.onTurnComplete` (`:715-755`), so a backend-error update must handle both `BUSY` and `DONE`. 11. `FleetMcp.capacityView` already shows the right shape for exhaustion (`:996-1025`); outage needs parallel fields with **different** names. 12. `FleetHealthMonitor.healthCoverage` (`:206-208`) must not change to `full` because of this. ### Riskiest assumption That a configured regex means the backend failed *this turn*. Two false matches would cool a healthy credential. Cheapest experiment: replay the saved 2026-09-01 pane, one deliberate disposable failure per adapter, and one valid member report quoting each error line, through the real `CompletionResolver` fixture. Outage panes must match; quoted reports must not. **If quoted reports still match, narrow the patterns — do not raise the threshold to hide weak classification.** ### Explicitly not building Reuse of `BackendQuarantine`; merged exhaustion/error patterns; grouping by error text; marking a profile permanently unusable (the classifier cannot yet separate malformed input from a transient fault); cooling on one generic error; putting the detector in `FleetHealthMonitor`; a second lead injector; persisting incident history across restart; work recovery; the old `visibleTurn` direction (#211 superseded it); and removing the legacy `API Error:` fallback in the first release. Units 1–4 are now delegated in parallel.
Author
Owner

Done and merged. All five units are on main.

Unit Commit What
1 26bafe8 the classifier: per-target backend-error pattern lookup + sink
2 959c835 BackendOutagePolicy — two distinct targets inside 60s cools the credential for 60s
3 e5eb353 the nudge to the lead when an incident starts
4 c3672f5 the outcome a member ends in
5 ac47498 wiring: the errorPattern config key, the spawn gate, and the fleet_list/fleet_profiles views

Prerequisite #234 (the ExhaustionSink inversion) landed first as 838a701, on purpose: it narrows a type, so it turned every silent collision in the parallel units into a loud compile error at merge time instead of a green build that ships a dead feature.

Build on main after the last merge: 1215 tests, 0 failures, 0 compile errors, BUILD SUCCESS.

The hard-coded API Error: string is gone as the only mechanism. A profile now declares errorPattern: in fleetd.yaml; a profile that declares none falls back to the built-in compatibility pattern, so this is never silently "off". A malformed pattern is rejected at startup by name.

Three things worth recording, because each was nearly missed:

  1. Unit 5's mutation M7 did not kill on the first pass. The test asserted only .contains("quarantined") — true of both the correct message and the fallback message the mutation produced, so it passed under correct and broken code alike. The worker found this, strengthened the assertion to an exact assertEquals, re-ran with the mutation still applied to confirm it now fails, and kept the stronger assertion. That is the right handling and it was reported rather than buried.
  2. A clean auto-merge is not a compiling merge. Merging Unit 1 produced no conflict markers at all and then failed to compile: #234 had made the 3-argument ExhaustionSink method the single abstract one, and a 2-argument lambda in a shared test file was now illegal. Git compares text; a lambda's arity is a type fact. Only the compiler finds it.
  3. The wiring itself is untested. Dropping Unit 5's patterns and sink at the Fleetd.java call site leaves all 1215 tests green with 0 compile errors. BackendOutageFlowTest is a good test, but it copies Fleetd.main's sink lambda — so it proves the copy, and cannot notice the original being deleted. Filed as #248 and already delegated.

Closing this and #227 together. A wiki/11-Features.md entry for the errorPattern knob is still owed and is mine to write.

Done and merged. All five units are on `main`. | Unit | Commit | What | |---|---|---| | 1 | `26bafe8` | the classifier: per-target backend-error pattern lookup + sink | | 2 | `959c835` | `BackendOutagePolicy` — two distinct targets inside 60s cools the credential for 60s | | 3 | `e5eb353` | the nudge to the lead when an incident starts | | 4 | `c3672f5` | the outcome a member ends in | | 5 | `ac47498` | wiring: the `errorPattern` config key, the spawn gate, and the `fleet_list`/`fleet_profiles` views | Prerequisite `#234` (the `ExhaustionSink` inversion) landed first as `838a701`, on purpose: it narrows a type, so it turned every silent collision in the parallel units into a loud compile error at merge time instead of a green build that ships a dead feature. Build on `main` after the last merge: **1215 tests, 0 failures, 0 compile errors, BUILD SUCCESS.** The hard-coded `API Error:` string is gone as the only mechanism. A profile now declares `errorPattern:` in `fleetd.yaml`; a profile that declares none falls back to the built-in compatibility pattern, so this is never silently "off". A malformed pattern is rejected at startup by name. **Three things worth recording, because each was nearly missed:** 1. **Unit 5's mutation M7 did not kill on the first pass.** The test asserted only `.contains("quarantined")` — true of both the correct message *and* the fallback message the mutation produced, so it passed under correct and broken code alike. The worker found this, strengthened the assertion to an exact `assertEquals`, re-ran with the mutation still applied to confirm it now fails, and kept the stronger assertion. That is the right handling and it was reported rather than buried. 2. **A clean auto-merge is not a compiling merge.** Merging Unit 1 produced no conflict markers at all and then failed to compile: `#234` had made the 3-argument `ExhaustionSink` method the single abstract one, and a 2-argument lambda in a shared *test* file was now illegal. Git compares text; a lambda's arity is a type fact. Only the compiler finds it. 3. **The wiring itself is untested.** Dropping Unit 5's patterns and sink at the `Fleetd.java` call site leaves all 1215 tests green with 0 compile errors. `BackendOutageFlowTest` is a good test, but it *copies* `Fleetd.main`'s sink lambda — so it proves the copy, and cannot notice the original being deleted. Filed as #248 and already delegated. Closing this and #227 together. A `wiki/11-Features.md` entry for the `errorPattern` knob is still owed and is mine to write.
ltms closed this issue 2026-09-03 06:58:04 +02:00
Author
Owner

Follow-up fix after this closed — coverage() named the wrong config key

Merged as 9d37f3a on main (1229 tests). Recording it here because the ticket has the context.

I found this by reading the boot log after redeploying this feature, not from a test. Unit 5 added a second classification line for the new errorPattern knob, and both lines came out of the same helper. The new line said:

backend-error classification: off (no profile has an exhaustedPattern configured; ...)

It is the errorPattern line. It named the other knob. Anyone acting on that message would have set the wrong key in fleetd.yaml, seen no change, and had no way to work out why — the message would still have been wrong after the fix.

The cause was a shared helper that hardcoded one key name while serving two callers:

public static String coverage(Set<String> allProfiles, Set<String> configuredProfiles) {
    if (configuredProfiles.isEmpty()) {
        return "off (no profile has an exhaustedPattern configured; profiles: " + ...

How I fixed it matters more than the fix

The tempting fix is an overload — add a three-argument coverage(patternKey, ...) and leave the old two-argument one alone. That is exactly the trap recorded in this repo already: a new method that existing callers silently ignore, shipping dead with a green suite.

So I changed the signature instead of adding to it:

public static String coverage(String patternKey, Set<String> allProfiles, Set<String> configuredProfiles)

The compiler then found all three call sites and made me pass "exhaustedPattern" and "errorPattern" explicitly. A caller cannot get the old wrong behaviour by doing nothing, because doing nothing no longer compiles. That is the property worth having, and it is only available before the overload exists.

Test that pins it, in CompletionResolverTest:

@Test
void coverageNamesTheConfigKeyItsCallerMeansRatherThanAlwaysSayingExhaustedPattern() {
    assertEquals("off (no profile has an errorPattern configured; profiles: [gx10, terra])",
            CompletionResolver.coverage("errorPattern", Set.of("terra", "gx10"), Set.of()));
}

Mutation-checked: reverting the production fix turns it red at :800, with 0 compile errors — so it is a real kill, not a broken build reporting zero failures.

Verified live. Redeployed to pid 36650, jar b8af736f44fc; both classification lines now name their own key, and the same log file holds the before (12:26) and the after (12:30) for comparison.

The general point

This is the second defect in this ticket's area that no test could have caught, because both were about a message being wrong, not a behaviour being wrong. A log line is a user interface for the operator. It is worth reading your own log after deploying, once, as a deliberate step — the two minutes it costs found both of these.

## Follow-up fix after this closed — `coverage()` named the wrong config key Merged as `9d37f3a` on `main` (1229 tests). Recording it here because the ticket has the context. I found this by **reading the boot log after redeploying this feature**, not from a test. Unit 5 added a second classification line for the new `errorPattern` knob, and both lines came out of the same helper. The new line said: ``` backend-error classification: off (no profile has an exhaustedPattern configured; ...) ``` It is the `errorPattern` line. It named the *other* knob. Anyone acting on that message would have set the wrong key in `fleetd.yaml`, seen no change, and had no way to work out why — the message would still have been wrong after the fix. The cause was a shared helper that hardcoded one key name while serving two callers: ```java public static String coverage(Set<String> allProfiles, Set<String> configuredProfiles) { if (configuredProfiles.isEmpty()) { return "off (no profile has an exhaustedPattern configured; profiles: " + ... ``` ## How I fixed it matters more than the fix The tempting fix is an **overload** — add a three-argument `coverage(patternKey, ...)` and leave the old two-argument one alone. That is exactly the trap recorded in this repo already: a new method that existing callers silently ignore, shipping dead with a green suite. So I **changed the signature instead of adding to it**: ```java public static String coverage(String patternKey, Set<String> allProfiles, Set<String> configuredProfiles) ``` The compiler then found all three call sites and made me pass `"exhaustedPattern"` and `"errorPattern"` explicitly. A caller cannot get the old wrong behaviour by doing nothing, because doing nothing no longer compiles. That is the property worth having, and it is only available before the overload exists. Test that pins it, in `CompletionResolverTest`: ```java @Test void coverageNamesTheConfigKeyItsCallerMeansRatherThanAlwaysSayingExhaustedPattern() { assertEquals("off (no profile has an errorPattern configured; profiles: [gx10, terra])", CompletionResolver.coverage("errorPattern", Set.of("terra", "gx10"), Set.of())); } ``` Mutation-checked: reverting the production fix turns it red at `:800`, with **0 compile errors** — so it is a real kill, not a broken build reporting zero failures. Verified live. Redeployed to pid 36650, jar `b8af736f44fc`; both classification lines now name their own key, and the same log file holds the before (12:26) and the after (12:30) for comparison. ## The general point This is the second defect in this ticket's area that no test could have caught, because both were about **a message being wrong, not a behaviour being wrong**. A log line is a user interface for the operator. It is worth reading your own log after deploying, once, as a deliberate step — the two minutes it costs found both of these.
Sign in to join this conversation.
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#201