Repository navigation
fix(agent-runtime): rebuild the shared provider transport after unanswered failures - #486
Merged
Merged
Conversation
…wered failures Issue #234: one Codex turn retried ten times and every attempt reported a bare `NETWORK_ERROR: fetch failed` while a new turn on the same provider, model, and network recovered immediately. The recorded fingerprint (`phase=stream`, `streamMs=1~2`) reads like a failure after the response headers, but pi-agent-core emits `message_start` for a stream that ended without ever emitting `start`, so it is the opposite: all ten attempts died before a response, and each spent seconds in the `phase=request` retries. Two provider-side gaps follow from that: - The real cause never reached the log. `classifyAgentError` walks the cause chain only when it is handed the Error, and pi-ai flattens a rejected request into `errorMessage` first, so `fetch failed` collapsed to `networkCategory: unknown` with no errno. The fetch wrapper in `provider-retry.ts` is the last layer that still holds the object, so it describes the cause there and merges the same validated fields the classifier produces (`networkCategory`, `networkCode`, `networkSyscall`, `networkHost`) plus `networkRoute` into the retried and terminal errors. - The shared transport was never invalidated. `node-proxy.ts` installs one process-wide undici dispatcher, and a pooled connection that died unnoticed stayed in use for the whole retry budget. `createProviderTransportHealth` counts consecutive unanswered failures per origin and rebuilds the transport after the second one, once per streak, at most once every 30 seconds process-wide, and never for `dns`. The replacement is installed before the previous dispatcher is closed, the previous one is closed gracefully (`close()`, never `destroy()`), and the configured route — including an environment-proxy bootstrap — is reproduced, so another session's in-flight request finishes on the pool it started on. A captured cause also reports `phase: request`, which is what the attempt actually did, instead of the synthetic start that surfaced it. The retry budget, backoff curve, classification, and user-visible codes are unchanged. New tests cover the capture, its redaction (no token, Authorization value, or URL query secret can reach a field), the per-origin threshold, the throttle, graceful replacement, route preservation, and an in-flight request surviving a rebuild; reverting the source leaves them red.
Sync the runtime specs (EN and zh-CN) with the new network diagnosis: the captured cause and `networkRoute`, the honest `phase: request` for a request that never answered, and the bounded, conditional rebuild of the shared transport. Add ADR 0271 for the rebuild policy — the thresholds, the graceful replacement, and why the pattern is "same origin twice" rather than every failure — and list it in the ADR index.
This branch was successfully deployed
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.
Refs #234. This continues the diagnosis from #435 and acts on it.
What the previous evidence actually said
#435 deliberately did not touch the shared transport, reading
phase=stream+streamMs=1~2as proof that "the response headers were already received". Thatreading is wrong, and the code says so: pi-agent-core emits
message_startfor astream that ended without ever emitting
start(
@earendil-works/pi-agent-core/dist/agent-loop.js:236-249), sostreamMsmeasures that synthetic start/end pair, not a started stream. Combined with
providerWaitMs=112442covering the whole retry loop, the reporter's recordmeans the opposite: every one of the ten attempts died before any response,
each spending seconds in
phase=requestretries, and the shared pool was reusedten times without ever being invalidated.
Two defects follow.
1. The real cause never reached the log
classifyAgentErrorwalks the cause chain only when it is handed the Error, andpi-ai flattens a rejected request into
errorMessagefirst. The reporter's shapetherefore degraded to
networkCategory: unknownwith no errno, which is why#435's new fields would not have helped their report at all:
The fetch wrapper in
provider-retry.tsis the last layer that still holds theobject, so it now describes the cause there and merges the same validated fields
the classifier produces —
networkCategory,networkCode,networkSyscall,networkHost— plusnetworkRoute(direct,environment-proxy,http-proxy,socks5-proxy, which names the hop and answers the reporter's"is it the proxy?" question) into the retried error and the terminal error. The
retry indicator and the assistant error card already render
networkCode, so theUI becomes diagnosable for this exact case without a new code or locale string.
Redaction and bounds stay owned by
agent-errors.ts: errno-shaped codes, alowercase syscall, a bare hostname, and a fixed route enum — never a URL, port,
path, query, token, or header value. A captured cause also reports
phase: request, because that is what the attempt did.2. The shared transport was never invalidated
node-proxy.tsinstalls one process-wide undici dispatcher;close()only ranwhen settings changed.
createProviderTransportHealthnow counts consecutiveunanswered failures per origin and asks for a rebuild after the second one:
once per streak, at most once every 30 seconds process-wide, and never for
dns(a fresh pool cannot change a name lookup). One failure is a blip and a replay
over the same pool is what a retry is for; the same origin failing twice without
ever answering means the pool's view of that origin is not recovering on its own,
and rebuilding there still leaves eight of the ten attempts to prove it helped.
Why this is safe for concurrent requests: the replacement dispatcher is
installed before the previous one is closed, so no request can be dispatched
into a dispatcher that is already closing; undici resolves the global dispatcher
per dispatch, so a request already in flight keeps the pool it started on; and
the previous pool is closed with
close()— neverdestroy()— which drainsin-flight requests and closes only idle sockets. The closed pool's module
reference is repointed when it was the captured original, and the configured route
(including a
NODE_USE_ENV_PROXYbootstrap, detected at startup) is reproduced,so a rebuild can never silently downgrade a proxy to a direct connection. A
process-wide throttle bounds the cost other sessions pay for it (one new TCP/TLS
handshake on their next request, never a broken request).
Verification
On the rebased branch (base
d51de445), all run in the worktree:packages/agent-runtime(vitest run): 36 files / 494 tests passpnpm -r --if-present test: docs 11, plugin-sdk 322, i18n 25, shared 678,plugin-devkit 48, agent-host 47, agent-runtime 494, desktop 1954 — 0 failures
pnpm build:js,pnpm --filter @pi-desktop/desktop typecheck,pnpm --filter @pi-desktop/agent-runtime typecheck,pnpm lint,pnpm docs:check(450 pages),node scripts/check-architecture.mjs: passNew tests, and what they prove:
provider-transport-recovery.test.ts— the cause chain behind a barefetch failedyields the errno and the category; an abort does not count as abroken connection; a credential/URL/query in the cause message or the request
URL cannot reach any reported field; the streak is per origin, cleared by a
response, never triggered by
dns, and spent once.node-proxy.test.ts— a rebuild swaps the process-wide dispatcher, keeps arequest that is already in flight (
close()is called,destroy()is not),is throttled for 30 seconds, and reproduces a custom proxy route.
provider-retry.test.ts— the rejection is described without replacing theoriginal error, and the retried error carries the errno while the provider
message stays untouched.
runtime.test.ts— the reporter's exact fingerprint now reports the capturedreset/ECONNRESET,phase: request, and the shared dispatcher is left aloneafter one failure but replaced after the second.
Counter-evidence: reverting the four source files to the base commit (keeping the
tests) leaves 4 test files red — the three that import the new module fail to
load and all four rebuild tests in
node-proxy.test.tsfail; with the change theyare green.
Risks and open items
received a response and that the pool was never rebuilt, which makes a stale
pooled connection the best-supported hypothesis, but the actual failing layer
can only be named by the next occurrence's
networkCategory/networkCode(DNS, TLS, refused, reset, timeout, proxy). This PR makes that reading possible
instead of guessing.
the threshold (2) and throttle (30s) are the tunable parts if review prefers a
different balance.
error.cause.nameand the raw cause message. Theywould put free-form provider text into logs and the transcript for little gain
over the errno, the category, and the route; say the word if you want them
behind a stricter redaction boundary.
timinglog stream is reintroduced.