Skip to content

fix(reliability): a dead socket is a transport failure, not a tool bug - #262

Merged
plombeer31 merged 1 commit into
mainfrom
fix/classify-network-failures
Aug 28, 2026
Merged

fix(reliability): a dead socket is a transport failure, not a tool bug#262
plombeer31 merged 1 commit into
mainfrom
fix/classify-network-failures

Conversation

@plombeer31

Copy link
Copy Markdown
Collaborator

What Sentry shows

Three of the top tool-error issues in cli are not tool errors:

Issue Events Users Tags
CLI-4N 108 13 category=tool, tool=unknown, cause_type=TypeError, zero stack frames
CLI-4T 44 20 category=tool, tool=unknown, cause_type=TypeError, frames end in undici … Object.errorRequest ← Socket.onHttpSocketClose
CLI-4A 173 31 category=tool, tool=unknown, cause_type=Error

Plus CLI-G, CLI-F, CLI-2K, CLI-4F, CLI-2S, CLI-W in the same shape — ~700 events filed as "our tool broke" for what the stack says is a connection that died.

Root cause

classifyFailure ends in a catch-all return "tool", and toLlmFailure wraps the remainder into ToolExecutionError("unknown", …). Everything that is not a typed LlamaServerError / OpenAiHttpError lands there — i.e. every raw undici failure from a surface that does not wrap its own errors (MCP streamable-http, embeddings, vendor SDKs with their own fetch). Those arrive as a bare TypeError: fetch failed with the real errno buried in cause.

The misfiling costs twice over:

  1. the user is told Turn failed [tool] for someone else's dead socket;
  2. shouldAdvance refuses to advance on tool (by definition our own bug, not the provider's), so a genuine network failure never falls over to the next provider — precisely what the fallback chain exists for.

The change

src/llm/reliability/network-error.tsisNetworkError / readNetworkErrorCode walk the cause chain (bounded, cycle-safe) for connection-level errnos (ECONNREFUSED, ECONNRESET, ENOTFOUND, EAI_AGAIN, UND_ERR_*, …) or undici's stock socket messages (fetch failed, terminated, socket hang up, other side closed). classifyFailure returns transport for those; toLlmFailure wraps them as TransportError carrying the original as cause.

Two deliberate boundaries:

  • EPIPE / EIO are excluded. Those are overwhelmingly stdio — a closed host pipe, a vanished tty — and reading one as transport would send the chain hunting for a different provider over a broken local stream. (They are their own defect; separate PR.)
  • The check sits after the cancellation branch. An aborted request can surface as ECONNRESET, and a user pressing Esc is not a fallover. Pinned by a test.

Tests

network-error.test.ts (10 new), plus classify-failure.test.ts and step-executor.test.ts cases proving a fetch failed from the completion path surfaces as TransportError while TypeError: x.map is not a function still surfaces as ToolExecutionError.

npm run lint clean; npm test 6330 passed / 1 failed — the failure is src/sidecar/send-message-concurrency.test.ts, which fails identically on untouched main in this environment.

**Sentry's biggest tool-error bucket is not a tool error at all.**

`CLI-4N` (108 events), `CLI-4T` (44), `CLI-4A` (173) and their siblings
all report the same thing: `ToolExecutionError`, `tool=unknown`,
`cause_type=TypeError`. `CLI-4T`'s stack ends in
`undici … Socket.onHttpSocketClose` — a connection that died, filed as
"our tool broke".

`classifyFailure` ends in a catch-all `return "tool"`, and `toLlmFailure`
wraps whatever is left into `ToolExecutionError("unknown", …)`. Anything
that is not a typed `LlamaServerError` / `OpenAiHttpError` lands there —
which is every raw `undici` failure from a surface that does not wrap its
own errors: MCP streamable-http transports, embedding calls, vendor SDKs
carrying their own `fetch`. They arrive as a bare `TypeError: fetch
failed` with the real errno buried in `cause`.

That misfiling costs twice. The user is told `Turn failed [tool]` for
someone else's dead socket. And `shouldAdvance` refuses to advance on a
`tool` category — by definition our own bug, not the provider's — so a
genuine network failure never falls over to the next link in the chain,
which is exactly what the fallback chain exists to do.

`isNetworkError` walks the `cause` chain looking for connection-level
errnos (`ECONNREFUSED`, `ECONNRESET`, `ENOTFOUND`, `UND_ERR_*`, …) or
undici's stock socket messages (`fetch failed`, `terminated`, `socket
hang up`, `other side closed`). Those classify as `transport` and wrap as
`TransportError`, so the chain advances and the turn reads honestly.

Two deliberate exclusions:

- `EPIPE` / `EIO` are **not** in the set. Those are overwhelmingly stdio
  — a closed host pipe, a vanished tty — and reading one as `transport`
  would send the fallback chain hunting for a different provider over a
  broken *local* stream.
- The check sits *after* the cancellation branch. An aborted request can
  surface as `ECONNRESET`, and a user pressing Esc is not a fallover.
plombeer31 added a commit that referenced this pull request Aug 28, 2026
Steer context in tasks, not tokens — plus #262#266 and six defect fixes
@plombeer31
plombeer31 merged commit dc4e562 into main Aug 28, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant