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

A member's normal report that quotes "API Error:" is recorded as a real backend outage #339

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

Found by a hunt over inject/. I have re-read the code and the pattern myself and confirmed the
shape.

What happens

CompletionResolver.resolve scans the assistant block for a backend-error pattern:

// CompletionResolver.java:393
String backendError = firstMatchingLine(assistantBlock, backendErrorPatternOrFallback(target));
if (backendError != null) {
    ...
    if (rendezvous.resolveFailure(waiter, reason)) {
        inFlight.remove(target, turn);
        log.warn("failing send to {} via turn-stall fallback: {}", target, reason);
        backendErrorSink.onBackendError(target, backendError, reason);   // <- the problem
    }
    return;
}

The built-in fallback pattern is:

// CompletionResolver.java:84
private static final Pattern BACKEND_ERROR = Pattern.compile("(?i)\\bAPI Error\\s*:");

Case-insensitive, unanchored, matched against every line of the assistant block. Any member that
ends a turn without fleet_reply and whose final text merely mentions an API error matches it.
Writing "the retry path handles the API error: it backs off" is enough.

onBackendError is not cosmetic. It feeds the credential outage machinery: two distinct backend
errors on one credential within 60 seconds put that credential into cooling-off, and fleet_spawn
naming a cooling profile is refused before it reaches the backend.

Direction of harm

Two costs, and they are not the same size.

  • Failing the send on a text match is defensible, and the code says why: the caller must not read
    a scrape as an answer.
  • Recording a credential outage on the same text match is not. It takes a member that was
    working fine and blocks new spawns on its credential. Profiles share credentials here, so this can
    refuse spawns on a profile that never had a problem.

The comment excuses the first thing and not the second

CompletionResolver.java:394-398 already acknowledges the false match:

The pattern is a heuristic: a member that forgot fleet_reply while reporting about a backend
error matches it too. Failing is still right — the caller must not read a scrape as an answer —
but dropping the rest of the pane would destroy the report.

That argument is sound, and it covers resolveFailure. It says nothing at all about
backendErrorSink.onBackendError. The sink call sits inside the block the comment excuses and
inherits an excuse that was never written for it.

Goal and invariants

Goal: a member's own prose about an error must not be recorded as that error happening.

Invariants:

  1. Keep failing the send. The comment's argument holds — a scrape is not an answer. Do not turn
    this into a success.
  2. Keep carrying the whole pane tail in the failure reason. Dropping it destroys the member's
    report, which is what #164 is about.
  3. A genuine backend error must still reach the sink. Do not fix a false positive by creating a
    false negative — that direction is worse, because a real outage would then go unrecorded and the
    fleet would keep spawning into a dead credential.

Candidate mechanisms, as candidates only — pick one and justify it:

  1. Require corroboration before calling the sink: the backend's own failure state, an exit status,
    or the error appearing outside the member's own prose — rather than a text match alone.
  2. Split the two decisions. Let a text match fail the send, and let only stronger evidence reach the
    sink. This keeps invariant 1 exactly as it is today.
  3. Anchor the pattern so it matches only a line that is the error, not a line that mentions one.
    Cheapest, but still a heuristic — say what it still lets through if you choose it.

Option 2 looks smallest to me, but I have not measured it. Decide it yourself.

Not verified

I confirmed the code path and the pattern by reading them. I have not produced a live false
cooling-off. Before fixing, write a test that drives a normal member report containing
API Error: through resolve and assert what reaches the sink — that turns this from a reading
into a fact, and it is the test the fix needs anyway.

The profile-configured patterns (BackendErrorPatternLookup) may be narrower or wider than the
built-in one. fleetd.yaml is not in a worker's worktree, so do not guess at the live values —
reason about the built-in fallback, which is always reachable for a target with no configured
pattern.

Found by a hunt over `inject/`. I have re-read the code and the pattern myself and confirmed the shape. ## What happens `CompletionResolver.resolve` scans the assistant block for a backend-error pattern: ```java // CompletionResolver.java:393 String backendError = firstMatchingLine(assistantBlock, backendErrorPatternOrFallback(target)); if (backendError != null) { ... if (rendezvous.resolveFailure(waiter, reason)) { inFlight.remove(target, turn); log.warn("failing send to {} via turn-stall fallback: {}", target, reason); backendErrorSink.onBackendError(target, backendError, reason); // <- the problem } return; } ``` The built-in fallback pattern is: ```java // CompletionResolver.java:84 private static final Pattern BACKEND_ERROR = Pattern.compile("(?i)\\bAPI Error\\s*:"); ``` Case-insensitive, unanchored, matched against **every line of the assistant block**. Any member that ends a turn without `fleet_reply` and whose final text merely *mentions* an API error matches it. Writing "the retry path handles the API error: it backs off" is enough. `onBackendError` is not cosmetic. It feeds the credential outage machinery: two distinct backend errors on one credential within 60 seconds put that credential into cooling-off, and `fleet_spawn` naming a cooling profile is refused before it reaches the backend. ## Direction of harm Two costs, and they are not the same size. - **Failing the send** on a text match is defensible, and the code says why: the caller must not read a scrape as an answer. - **Recording a credential outage** on the same text match is not. It takes a member that was working fine and blocks new spawns on its credential. Profiles share credentials here, so this can refuse spawns on a profile that never had a problem. ## The comment excuses the first thing and not the second `CompletionResolver.java:394-398` already acknowledges the false match: > The pattern is a heuristic: a member that forgot fleet_reply while reporting *about* a backend > error matches it too. Failing is still right — the caller must not read a scrape as an answer — > but dropping the rest of the pane would destroy the report. That argument is sound, and it covers `resolveFailure`. It says nothing at all about `backendErrorSink.onBackendError`. The sink call sits inside the block the comment excuses and inherits an excuse that was never written for it. ## Goal and invariants **Goal:** a member's own prose about an error must not be recorded as that error happening. **Invariants:** 1. **Keep failing the send.** The comment's argument holds — a scrape is not an answer. Do not turn this into a success. 2. **Keep carrying the whole pane tail** in the failure reason. Dropping it destroys the member's report, which is what #164 is about. 3. A genuine backend error must still reach the sink. Do not fix a false positive by creating a false negative — that direction is worse, because a real outage would then go unrecorded and the fleet would keep spawning into a dead credential. **Candidate mechanisms, as candidates only — pick one and justify it:** 1. Require corroboration before calling the sink: the backend's own failure state, an exit status, or the error appearing outside the member's own prose — rather than a text match alone. 2. Split the two decisions. Let a text match fail the send, and let only stronger evidence reach the sink. This keeps invariant 1 exactly as it is today. 3. Anchor the pattern so it matches only a line that *is* the error, not a line that mentions one. Cheapest, but still a heuristic — say what it still lets through if you choose it. Option 2 looks smallest to me, but I have not measured it. Decide it yourself. ## Not verified I confirmed the code path and the pattern by reading them. I have **not** produced a live false cooling-off. Before fixing, write a test that drives a normal member report containing `API Error:` through `resolve` and assert what reaches the sink — that turns this from a reading into a fact, and it is the test the fix needs anyway. The profile-configured patterns (`BackendErrorPatternLookup`) may be narrower or wider than the built-in one. `fleetd.yaml` is not in a worker's worktree, so do not guess at the live values — reason about the built-in fallback, which is always reachable for a target with no configured pattern.
ltms commented 2026-09-04 11:14:11 +02:00
Author
Owner
Copy Link
Copy Source

Merged to main as d11d1d1 (--no-ff; the branch was behind main). Follow-up commit 6a81417
corrects a regression the fix introduced — read that part.

Build after both: Tests run: 1362, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS, unpiped.

What the implementer got right

It did the measurement first, as asked, and the false positive is real: a member's prose
("I checked the retry path. An API Error: makes it back off.") failed the send and recorded a
credential failure. Its new test pins that, and invariants 1 and 2 are intact — the send still fails
and the whole pane tail is still carried.

Splitting the too-fast crash path out was a good call: there the crash signature is corroboration,
so a bare match is enough evidence.

What it got wrong, and I caught on merge

The check shipped as a bare lookingAt():

private static boolean startsWithBackendError(String line, Pattern pattern) {
    return pattern.matcher(line).lookingAt();
}

firstMatchingLine returns line.strip(), and strip() removes whitespace only — not terminal
chrome. So a genuine error line rendered with an ordinary gutter bar fails the check.

I probed it on the raw-scrape path, which is exactly the path whose own comment says to expect
leading chrome:

PROBE-CHROME kind=FAILED sinkNotified=0

Input was │ 503 Service Unavailable: upstream credential rejected against the configured pattern.
The send fails, and the outage is never recorded. That is the false negative this ticket's own
invariant 3 named as the worse direction — an unrecorded outage leaves the fleet spawning into a
dead credential.

The existing tests missed it because every one of them puts the error line with no chrome in front
of it. The raw-scrape test even sets up box-drawing chrome on the line above the error, and leaves
the error line itself clean.

The correction (6a81417)

startsWithBackendError now skips a leading run of non-letter, non-digit characters before the
check. That keeps this ticket's intent exactly: prose still does not match, because there the
pattern sits after words, not after chrome. The implementer's own prose test still passes
unchanged.

Mutation Y — restoring the bare lookingAt():

Tests run: 1362, Failures: 1, Errors: 0
CompletionResolverTest.aRealErrorBehindTerminalChromeStillNotifiesTheSink:1002
a real error line behind box chrome is still a real outage — it must reach the sink, or the fleet
keeps spawning into a dead credential ==> expected: <1> but was: <0>

Pinned now, in both directions.

The remaining caveat is honest and stays

The check is still a heuristic: prose that begins with API Error: will still notify the sink. The
implementer said so plainly rather than claiming the problem solved, which is the right way to leave
it.

Same shape, reported and not fixed here

The implementer flagged that exhaustion-pattern matches notify ExhaustionSink on the same
free-text basis, and left it alone as out of scope — correctly. That one is worse: a quarantine runs
1800s by default against this cooldown's fixed 60s. Filed as #348, marked unproven, with the chrome
lesson written into it so it is not repeated.

Closing. One fix in, one regression caught and corrected on merge, one candidate out (#348).

Merged to `main` as `d11d1d1` (`--no-ff`; the branch was behind main). **Follow-up commit `6a81417` corrects a regression the fix introduced — read that part.** Build after both: `Tests run: 1362, Failures: 0, Errors: 0, Skipped: 0`, `BUILD SUCCESS`, unpiped. ## What the implementer got right It did the measurement first, as asked, and the false positive is real: a member's prose (`"I checked the retry path. An API Error: makes it back off."`) failed the send **and** recorded a credential failure. Its new test pins that, and invariants 1 and 2 are intact — the send still fails and the whole pane tail is still carried. Splitting the too-fast crash path out was a good call: there the crash signature is corroboration, so a bare match is enough evidence. ## What it got wrong, and I caught on merge The check shipped as a bare `lookingAt()`: ```java private static boolean startsWithBackendError(String line, Pattern pattern) { return pattern.matcher(line).lookingAt(); } ``` `firstMatchingLine` returns `line.strip()`, and `strip()` removes whitespace only — not terminal chrome. So a **genuine** error line rendered with an ordinary gutter bar fails the check. I probed it on the raw-scrape path, which is exactly the path whose own comment says to expect leading chrome: ``` PROBE-CHROME kind=FAILED sinkNotified=0 ``` Input was `│ 503 Service Unavailable: upstream credential rejected` against the configured pattern. The send fails, and the outage is **never recorded**. That is the false negative this ticket's own invariant 3 named as the worse direction — an unrecorded outage leaves the fleet spawning into a dead credential. The existing tests missed it because every one of them puts the error line with no chrome in front of it. The raw-scrape test even sets up box-drawing chrome on the line *above* the error, and leaves the error line itself clean. ## The correction (`6a81417`) `startsWithBackendError` now skips a leading run of non-letter, non-digit characters before the check. That keeps this ticket's intent exactly: prose still does not match, because there the pattern sits after *words*, not after chrome. The implementer's own prose test still passes unchanged. **Mutation Y** — restoring the bare `lookingAt()`: ``` Tests run: 1362, Failures: 1, Errors: 0 CompletionResolverTest.aRealErrorBehindTerminalChromeStillNotifiesTheSink:1002 a real error line behind box chrome is still a real outage — it must reach the sink, or the fleet keeps spawning into a dead credential ==> expected: <1> but was: <0> ``` Pinned now, in both directions. ## The remaining caveat is honest and stays The check is still a heuristic: prose that *begins* with `API Error:` will still notify the sink. The implementer said so plainly rather than claiming the problem solved, which is the right way to leave it. ## Same shape, reported and not fixed here The implementer flagged that exhaustion-pattern matches notify `ExhaustionSink` on the same free-text basis, and left it alone as out of scope — correctly. That one is worse: a quarantine runs 1800s by default against this cooldown's fixed 60s. Filed as #348, marked unproven, with the chrome lesson written into it so it is not repeated. Closing. One fix in, one regression caught and corrected on merge, one candidate out (#348).
ltms closed this issue 2026-09-04 11:14:36 +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#339
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?