fix(agent-runtime): classify the network cause behind NETWORK_ERROR - #435
Merged
Merged
Conversation
Issue #234: one turn retried ten times with `NETWORK_ERROR: fetch failed` (phase=stream, streamMs=1-2) while a new turn in the same session recovered immediately, and nothing in the logs identified the failing layer because no `error.cause` was kept. `classifyAgentError` already walked the cause chain to detect a network failure, but then returned a bare `result("NETWORK_ERROR", true)` with no cause detail. - Summarize the transport failure in bounded `details`: `networkCategory` (dns | tls | timeout | refused | unreachable | reset | proxy | unknown), `networkCode` (the errno from the cause chain, including undici's happy-eyeballs `AggregateError.errors`), `networkSyscall`, and `networkHost`. - Keep the existing user-visible code. Per-layer codes (DNS_ERROR, TLS_ERROR, SOCKET_RESET, PROXY_ERROR, STREAM_OPEN_FAILED) would each need a spec entry and strings in all eight shipped locales; the category splits the layers inside the already-spec'd, non-user-visible `details` channel instead. - Redaction: only errno-shaped codes, a lowercase syscall, and a bare hostname (never a URL, port, path, query, or credential) can reach the details, and `providerCode` is omitted when it would repeat `networkCode`. - Correlate a failure that produced no response: `requestMessages`, `requestBytes` (measured at the fetch wrapper - byte size only, the body is never read) and `compactionGeneration`. - Surface the errno where the user already is: the retry-reason popover and the assistant error card render `NETWORK_ERROR - ENOTFOUND` in the existing code chip, so no new locale strings are needed. Suggestion 3 of the issue (write the sanitized cause chain to `agent/timing.log`) is obsolete and is deliberately not implemented: ADR 0212 (Accepted 2026-09-10) removed the sidecar `[timing]` lines, the `timing` log category, and `PI_DESKTOP_TIMING`. Failures stay diagnosable through the channels ADR 0212 keeps - the `agent/session.log` error record (its `details` pass through unwhitelisted), the stable error code, and the transcript. No timing record is reintroduced. Verified against the code and deliberately unchanged: every retry already builds a fresh AbortController, stream, and SDK client (`retryPendingProviderFailure` -> `agent.continue()` -> `createProviderRetryStream` -> pi-ai), so suggestion 1 needed no change. A process-global undici dispatcher does exist (`node-proxy.ts`), but rebuilding it after N failures is a behavior change for every session and needs its own ADR; the reported evidence does not prove that retries reused a dead pooled socket, so it is left alone. Specs updated: 03-runtime/08-error-codes.md (details enumeration and the new network diagnosis), 03-runtime/01-ipc-protocol.md, 03-runtime/02-agent-runtime.md and their zh-CN pairs. Refs #234
Follow-up to the review of the previous commit (issue #234): - Validate message-derived candidates before they can be reported: the error message is untrusted provider text, so an errno-shaped run inside it (`EDNS` + 3000 characters) can no longer become an unbounded `networkCode`, and the message scan is capped at 16 candidates. - Anchor the proxy code pattern on the code itself instead of any occurrence of "proxy" inside a body word, and pick a proxy-classified code from anywhere in the cause chain: undici reports the proxy's own socket errno as a deeper cause, so traversal order was deciding the category the comment claimed was decided by specificity. - A hostname read out of the message now has to be dotted, so a credential-shaped token, or the `user` left over from a truncated `getaddrinfo ENOTFOUND user:pass@host`, is never reported as `networkHost`. A Node-supplied `err.hostname` keeps its single-label case (`localhost`). - Repair the spec sentences the previous commit's insertions broke, in both the English and the zh-CN spec pairs. - Cover both renderer surfaces in the existing source-contract test, and cover the unmeasurable-body and bounded-token cases in the unit tests. Refs #234
…-cause-diagnostics
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reported in #234: one Turn in a long Codex session failed 10 consecutive retries over ~112 s, every attempt surfacing only
NETWORK_ERROR: fetch failedwithphase=stream,streamMs=1~2and noerror.cause, while a new Desktop session and a new Turn in the same session both worked immediately. The reporter could not tell which layer failed, so the report's own root-cause hypothesis could not be tested.This PR makes the failing layer identifiable and correlates it with request size, without changing the retry policy.
One suggestion in the report is obsolete
The report asks to write the sanitized
causechain intoagent/timing.log.docs/adr/0212-remove-diagnostic-timing-log-streams.md(Accepted 2026-09-10) deleted the sidecar[timing]lines, thetiminglog category andPI_DESKTOP_TIMING, and the report's logs predate that on 0.14.6. This PR therefore does not reintroduce any timing output. Per ADR 0212, failures stay diagnosable through lifecycle records, error codes and the transcript — which is the channel used here. The issue thread should be corrected on this point.Changes
packages/agent-runtime/src/agent-errors.ts— the network branch ofclassifyAgentErrornow walks thecausechain for a low-cardinality classification:networkCategory(dns|tls|timeout|refused|unreachable|reset|proxy|unknown), plusnetworkCode(errno-style),networkSyscallandnetworkHost.DNS_ERROR/TLS_ERROR. A new code needs an i18n string in every shipped locale and a registry entry (error-code-registry.test.mjsenforces it) for no additional diagnostic power;detailsis the already-specified channel for this (docs/spec/03-runtime/08-error-codes.md§5).STREAM_OPEN_FAILEDis already expressed by the existingphase=stream+streamMs.user:pass@host,host:8080, a URL, a path, a query or an IPv6 literal. A hostname taken from message text must contain a dot, so a credential-shaped token cannot be reported as a host. No raw provider text is retained.packages/agent-runtime/src/provider-retry.ts—captureProviderResponsereports the outgoing request's byte length only (never the body content), including on a request that dies before headers, which is exactly the failure worth correlating.packages/agent-runtime/src/runtime.ts— the errordetailsnow carryrequestBytes,requestMessagesandcompactionGeneration; the retry activity error carriesnetworkCode.apps/desktop/.../transcript/ActivityGroup.tsx+shared.tsx— the retry bubble and the assistant error card show the errno next to the code (NETWORK_ERROR · ENOTFOUND). This is a bare technical token in the existing code-chip style, so it adds no i18n key.packages/shared/src/types/sessions.ts— optionalAgentActivityError.networkCode.docs/spec/03-runtime/08-error-codes.md,01-ipc-protocol.md,02-agent-runtime.mdand theirdocs/zh-CNmirrors.What is deliberately NOT done
Suggestion 2 — "after several consecutive pre-stream failures, rebuild the host-proxy transport / HTTP dispatcher / keep-alive connection pool". A process-wide undici dispatcher does exist (
node-proxy.tssetGlobalDispatcher+globalThis.fetch = undiciFetch;closeActiveDispatchers()runs only at boot and on a settings change), so this is technically reachable. But: rebuilding it on a failure counter is a global behavior change affecting every session, it needs its own ADR, and the evidence does not actually show retries reusing a dead connection —phase=streamwithstreamMs=1~2means response headers had already been received. So it is reported, not implemented.Suggestion 1 (a fresh request per attempt) was verified rather than changed:
runtime.tscreates a newAbortControllerper retry, the retry re-drivesagent.continue(), each round builds a new inner stream, and pi-ai constructs a new SDK client per request.Validation
On the integrated local
main(merge commit93abcf0e, after merging this branch):pnpm build:jspnpm --filter @pi-desktop/desktop typecheckpnpm --filter @pi-desktop/agent-runtime typecheckpnpm lint:biomepnpm -r --if-present testpnpm docs:checkpnpm test:e2e:transcriptpnpm test:e2e:bootpnpm test:e2e:subagentsReverting the four source files to
origin/mainwhile keeping the new tests gives 14 failed / 195 passed, so the coverage is real.Remaining risk (stated plainly)
The root cause is not fixed — only made diagnosable. Whether retries reused dead transport state is neither proven nor addressed. The next occurrence can now be classified (DNS vs TLS vs timeout vs refused vs reset vs proxy) and correlated with
requestMessages/requestBytes/compactionGeneration, which is what is needed to decide whether the ADR-gated transport rebuild is warranted.Known limitation: if the failure is in the proxy hop but the error text never says "proxy", it classifies as
refusedrather thanproxy— the classifier has no access to the proxy configuration.Refs #234