Skip to content

fix: complete network retry recovery and isolate failed attempts - #1395

Merged
EricSanchezok merged 2 commits into
devfrom
synergy/network-retry-recovery
Sep 15, 2026
Merged

EricSanchezok merged 2 commits into
devfrom
synergy/network-retry-recovery

Conversation

@EricSanchezok

@EricSanchezok EricSanchezok commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Recover consistently from transient DNS, connection, HTTP, and streamed provider failures across model providers and webpage reads. Preserve nested errors across Agent workers, honor bounded backoff and cancellation, and keep permanent failures terminal.

  • Give derived Agent calls one retry budget and total deadline for both startup and body failures, resetting partial output between attempts.
  • Withdraw failed-attempt text, reasoning, and unexecuted tool proposals before session recovery while retaining authoritative rollout evidence and accounting. Record a consumed stream error as failed when the consumer stops reading.
  • Coordinate recovery across sessions before worker admission, with shared cooldown, one recovery probe per connection, cancellation, bounded waiting/storage, and protection against stale successes clearing newer failures.
  • Limit webfetch GET to three attempts within its original whole-response deadline. Never replay the model after tool dispatch can produce effects.

Why?

A transient getaddrinfo ETIMEOUT was not consistently recognized by the runtime, SDK, or worker error path. The wider audit also found partial results treated as success, failed content leaking into recovered replies, independent concurrent retries, and post-tool failures restarting the model.

External sources and provenance

Provenance markers live in packages/util/src/network-error.ts, packages/util/src/retry.ts, packages/harness/src/provider/retry.ts, and packages/harness/src/provider/retry-coordinator.ts.

How was it tested?

bun run quality:quick
(cd packages/util && bun test && bun run build)
(cd packages/library && bun test test/experience-reencode.test.ts && bun run typecheck)
(cd packages/runtime-local && bun test test/tools/webfetch.test.ts)
(cd packages/harness && bun test test/provider/retry-coordinator.test.ts test/provider/retry.test.ts test/provider/auth-recovery.test.ts test/session/processor-retry.test.ts test/session/retry.test.ts test/session/agent-turn-protocol.test.ts test/session/agent-turn.test.ts test/agent/call.test.ts test/session/rollout-call.test.ts test/session/llm-stream-lifecycle.test.ts test/session/llm-variant.test.ts test/provider/sse-buffer.test.ts test/session/message-v2.test.ts test/session/message-cache.test.ts)
(cd packages/harness && bun test test/session/agent-worker-runtime-boundary.test.ts)

The Library integration checks the configured retry budget at AgentCall and asserts zero nested SDK retries. Regression tests use real message/rollout storage, real SDK retry admission, controlled concurrent recovery, and local HTTP failure injection. The worker import-graph test passed in isolation after exceeding its existing five-second limit while competing with the static-check cluster. No live provider outage or deployment was used as validation.

Checklist

  • Local static gates and relevant behavioral tests pass.
  • Tests live under their owning package's test/ directory.
  • Package, dependency, documentation, decision, localization, and secret checks pass.
  • Updated architecture, tool help/generated reference, owning skills, and the Chinese retry audit/decision.
  • External sources are attributed beside the authoritative implementation.
  • No secrets, private runtime data, unrelated cleanup, or global replay of writes.

Public API schemas and persisted message formats are unchanged; SDK regeneration and state migrations are not required. Worker error framing is an internal versioned protocol. Shared cooldown is process-local, and retries do not promise upstream exactly-once generation.

Unify transient failure classification, bound replay to model and read phases,
and coordinate provider recovery without multiplying request budgets.
Preserve rollout evidence while withdrawing failed stream content.

Co-authored-by: synergy-agent <299070056+synergy-agent@users.noreply.github.com>
@oryn-mini

oryn-mini Bot commented Sep 15, 2026

Copy link
Copy Markdown

🧹 Oryn Mini · ✅ Task complete

Code review · Attempt 1 · Cycle 1

This PR makes transient network failures recoverable end to end: a shared, bounded classifier recognizes DNS, connection, HTTP, and in-stream provider errors across providers and the worker boundary; failed model attempts are withdrawn from the visible reply before recovery; and a process-local coordinator shares one cooldown and recovery probe per provider connection. It matters because a single getaddrinfo ETIMEOUT can currently fail a turn terminally, half-written answers can leak into reco…

View the Actions run

Run details · operational receipt
{
  "version": 1,
  "workKey": "review:75b957287fb83384de0d609c",
  "sourceKey": "75b957287fb83384de0d609c",
  "intent": "review",
  "phase": "completed",
  "attempt": 1,
  "cycle": 1,
  "leaseID": "2a0d78fd-4fee-49a9-90e4-4248d5ab3a4c",
  "verdict": "actionable",
  "triage": {
    "category": "bug",
    "priority": "P1",
    "confidence": "high",
    "reproduction": "source_proven",
    "proof": "sufficient",
    "decision": "keep_open",
    "closeReason": "none",
    "implementationCandidate": false,
    "complexity": "medium",
    "evidence": [
      {
        "path": "packages/util/src/network-error.ts",
        "line": 45,
        "sha": "a024e0fff1ac9d8cb82e81a15455a1245b1c2127",
        "detail": "classifyNetworkError walks a bounded cause/errors graph; ETIMEOUT/ETIMEDOUT, EAI_AGAIN, Undici connect/body timeout codes are transient; cert/invalid-name/truncation permanent; abort wins over all."
      },
      {
        "path": "packages/util/src/retry.ts",
        "line": 47,
        "sha": "a024e0fff1ac9d8cb82e81a15455a1245b1c2127",
        "detail": "retryAfterMs strictly parses Retry-After and retry-after-ms (rejects -1, Infinity, trailing garbage); retry() adds equal jitter, a cancellable sleep, and the narrowed transient-only default retryIf."
      },
      {
        "path": "packages/harness/src/provider/retry.ts",
        "line": 32,
        "sha": "a024e0fff1ac9d8cb82e81a15455a1245b1c2127",
        "detail": "providerRetryable: recording errors and exhausted AI_RetryError stay terminal; terminal codes (auth/quota/input) beat transient; 408/429/5xx-except-501/505 retryable; 409 needs explicit isRetryable."
      },
      {
        "path": "packages/harness/src/provider/retry-coordinator.ts",
        "line": 47,
        "sha": "a024e0fff1ac9d8cb82e81a15455a1245b1c2127",
        "detail": "Process-local coordinator: hashed connection keys (endpoint/credential fields hashed, never stored), one recovery probe, revision guard so a stale success cannot clear a newer failure, five-minute admission bound, 1024-entry cap."
      },
      {
        "path": "packages/harness/src/session/agent-turn/index.ts",
        "line": 79,
        "sha": "a024e0fff1ac9d8cb82e81a15455a1245b1c2127",
        "detail": "AgentTurn.stream now admits worker runs through recovery.stream, so shared cooldown and a single probe precede worker-pool admission; admission also rejects while the pool is stopping."
      },
      {
        "path": "packages/harness/src/session/llm.ts",
        "line": 532,
        "sha": "a024e0fff1ac9d8cb82e81a15455a1245b1c2127",
        "detail": "wrapStream middleware normalizes stream-start failures into APICallError with accurate isRetryable before SDK retry admission, passing original errors through when the caller already aborted."
      },
      {
        "path": "packages/harness/src/session/agent-turn/protocol.ts",
        "line": 9,
        "sha": "a024e0fff1ac9d8cb82e81a15455a1245b1c2127",
        "detail": "Worker protocol VERSION bumped to 9; serializeCause now carries nested cause/errors graphs with depth/count/text bounds and an explicit ERR_CAUSE_TRUNCATED marker."
      },
      {
        "path": "packages/harness/src/session/processor.ts",
        "line": 1667,
        "sha": "a024e0fff1ac9d8cb82e81a15455a1245b1c2127",
        "detail": "retryEligible is set false once the provider stream has been consumed normally — before tool authorization and dispatch — so failures after tool dispatch can never replay the model (verified against the retry gate at line 1806)."
      },
      {
        "path": "packages/harness/src/session/processor.ts",
        "line": 1007,
        "sha": "a024e0fff1ac9d8cb82e81a15455a1245b1c2127",
        "detail": "Before a retry attempt: waits for tracked settlements, flushes part writes, removes failed-attempt parts not present in the pre-turn snapshot (keeping pre-existing parts, patches, step-finish), then resets message finish."
      },
      {
        "path": "packages/harness/src/agent/call.ts",
        "line": 197,
        "sha": "a024e0fff1ac9d8cb82e81a15455a1245b1c2127",
        "detail": "AgentCall passes retries: 0 so SDK retries are disabled; the outer loop (line 184) owns the caller budget for startup and body failures under the original abort deadline composed from input signal, timeout, and output controllers."
      },
      {
        "path": "packages/harness/src/session/retry.ts",
        "line": 26,
        "sha": "a024e0fff1ac9d8cb82e81a15455a1245b1c2127",
        "detail": "retryable narrowed to isRetryable-flagged APIErrors, worker-restart UnknownErrors, and providerRetryable-confirmed raw messages; delay now uses the shared strict Retry-After parser with capped jittered backoff."
      },
      {
        "path": "packages/harness/src/session/rollout/call.ts",
        "line": 172,
        "sha": "a024e0fff1ac9d8cb82e81a15455a1245b1c2127",
        "detail": "settle records 'failed' whenever a failure was observed, so a consumed stream error stays failed even when the consumer stops reading and disposes."
      },
      {
        "path": "packages/runtime-local/src/tools/webfetch.ts",
        "line": 116,
        "sha": "a024e0fff1ac9d8cb82e81a15455a1245b1c2127",
        "detail": "GET retried up to three attempts inside the single original timeout deadline (covering request, streaming body read, and backoff), one permission grant per invocation, caller cancellation reasons preserved."
      },
      {
        "path": "packages/harness/test/session/processor-retry.test.ts",
        "line": 77,
        "sha": "a024e0fff1ac9d8cb82e81a15455a1245b1c2127",
        "detail": "Real-storage tests: a failed stream executes no tools while recovery executes once, withdrawal leaves only valid text in history and model context, rollout keeps failed+completed attempts and accounting, cancel keeps the partial output."
      },
      {
        "path": "docs/decisions/implemented/bug-fix/2026-09-15-network-retry-boundaries.md",
        "line": 1,
        "sha": "a024e0fff1ac9d8cb82e81a15455a1245b1c2127",
        "detail": "Implemented decision record documenting shared classification, budget ownership per caller, failed-attempt withdrawal, coordination boundaries, rejected alternatives, and cost/exactly-once consequences."
      }
    ],
    "risks": [
      "ENOTFOUND, certificate failures, and truncated error graphs are deliberately terminal; a DNS outage that surfaces as ENOTFOUND will fail turns instead of retrying — a documented tradeoff that may generate support traffic during resolver incidents.",
      "The old 'assistant message prefill' retry special case was removed; providers relying on transient prefill errors will now surface them as terminal validation failures."
    ]
  },
  "run": "https://github.com/SII-Holos/synergy/actions/runs/34949411571",
  "summary": "This PR makes transient network failures recoverable end to end: a shared, bounded classifier recognizes DNS, connection, HTTP, and in-stream provider errors across providers and the worker boundary; failed model attempts are withdrawn from the visible reply before recovery; and a process-local coordinator shares one cooldown and recovery probe per provider connection. It matters because a single `getaddrinfo ETIMEOUT` can currently fail a turn terminally, half-written answers can leak into recovered replies, and independent per-session retries multiply provider load during an outage. Cancellation, certificate/quota/input errors, and post-tool-dispatch failures stay terminal, and the new tests exercise real message/rollout storage and real SDK retry admission rather than mocks."
}

Co-authored-by: synergy-agent <299070056+synergy-agent@users.noreply.github.com>
@oryn-mini

oryn-mini Bot commented Sep 15, 2026

Copy link
Copy Markdown

🧹 Oryn Mini · 🛠️ Changes requested

🟠 P1 · High · 🐛 Bug · High confidence

Why this PR

This PR makes transient network failures recoverable end to end: a shared, bounded classifier recognizes DNS, connection, HTTP, and in-stream provider errors across providers and the worker boundary; failed model attempts are withdrawn from the visible reply before recovery; and a process-local coordinator shares one cooldown and recovery probe per provider connection. It matters because a single getaddrinfo ETIMEOUT can currently fail a turn terminally, half-written answers can leak into recovered replies, and independent per-session retries multiply provider load during an outage. Cancellation, certificate/quota/input errors, and…

Full explanation

This PR makes transient network failures recoverable end to end: a shared, bounded classifier recognizes DNS, connection, HTTP, and in-stream provider errors across providers and the worker boundary; failed model attempts are withdrawn from the visible reply before recovery; and a process-local coordinator shares one cooldown and recovery probe per provider connection. It matters because a single getaddrinfo ETIMEOUT can currently fail a turn terminally, half-written answers can leak into recovered replies, and independent per-session retries multiply provider load during an outage. Cancellation, certificate/quota/input errors, and post-tool-dispatch failures stay terminal, and the new tests exercise real message/rollout storage and real SDK retry admission rather than mocks.


What changed

  • Before: a raw getaddrinfo ETIMEOUT fell outside the session's known error codes and the SDK never retried it. After: classifyNetworkError walks a bounded cause/errors graph shared by webfetch, provider policy, and worker frames.
  • Before: worker error frames dropped nested causes and aggregate members, so recovery decisions were made on incomplete evidence. After: protocol v9 serializes nested cause/errors graphs with explicit truncation that stops auto-retry.
  • Before: AgentCall.text ignored stream error parts (partial text looked like success) and nested hidden SDK retries. After: startup and body failures share one caller-owned budget and original deadline, SDK retries are zero, and the output buffer resets each attempt.
  • Before: failed-attempt text, reasoning, and unexecuted tool proposals stayed mixed into the recovered message. After: the processor withdraws them via ordinary part removal before retrying, while rollout retains every attempt, status, and accounting.
  • Before: each session backed off independently and a post-tool-dispatch error could restart the model. After: a hashed-connection coordinator shares one cooldown and recovery probe before worker admission, and model retries end once the provider stream completes normally.
  • Before: webfetch gave up after one failed GET and caller cancellation could surface as a timeout. After: three attempts share the original whole-response deadline with one permission grant and preserved cancellation reasons.

How it fits together

How a provider failure flows from raw transport error to coordinated session recovery while rollout records keep every attempt.

flowchart TD
  accTitle: Network retry recovery path
  accDescr: How a provider failure flows from raw transport error to coordinated session recovery while rollout records keep every attempt.
  n_0["Raw provider stream failure"]
  n_1["Shared network classifier"]
  n_2["Provider retry policy"]
  n_3["Recovery coordinator gate"]
  n_4["Agent worker stream"]
  n_5["Processor retry window"]
  n_6["Rollout call records"]
  n_0 -->|"classify cause graph"| n_1
  n_1 -->|"transient or terminal"| n_2
  n_2 -->|"shared cooldown"| n_3
  n_3 -->|"admit one probe"| n_4
  n_4 -->|"protocol v9 error"| n_5
  n_5 -->|"next attempt queues"| n_3
  n_5 -->|"all attempts retained"| n_6
Loading

Review findings

🔵 Low · ENOTFOUND and truncated error graphs are terminal by designpackages/util/src/network-error.ts:24

PERMANENT_CODES classifies ENOTFOUND/ENONAME as permanent, and worker-protocol truncation surfaces as ERR_CAUSE_TRUNCATED, which also stops auto-retry. A resolver outage that presents as ENOTFOUND will fail turns instead of retrying. The audit and decision record document this tradeoff explicitly, so it is a policy choice rather than a defect — keep it in mind when triaging user DNS reports.

🔵 Low · Provider prefill validation failures lost their retry pathpackages/harness/src/session/retry.ts:33

SessionRetry.retryable no longer special-cases 'assistant message prefill' messages; only APIErrors flagged isRetryable by providerRetryable now retry. Providers that previously surfaced transient prefill errors will now persist them as terminal validation failures. The narrowing matches the audit's 'input errors are terminal' stance, but it is a user-visible behavior change worth a second opinion from someone covering those providers.

🔵 Low · Abandoned recovery streams hold the probe until the five-minute boundpackages/harness/src/provider/retry-coordinator.ts:93

In coordinator.stream, the probe token is only released on dispose (or lease release), so a future caller that obtains a recovery stream and neither reads it to completion nor disposes it blocks new admissions for that connection until the bounded admission wait expires. The processor always disposes in a finally block, so no current path leaks, but new AgentTurn callers must keep the dispose contract in mind.

Before merge

  • Three CI checks on head a024e0f were still in progress at review time (Benchmark Docker Lifecycle; Long Rollout Contracts cancelled/failed) — let them finish before merging.
  • The PR carries a changes-requested label but no review threads were visible in my evidence; confirm any maintainer feedback is already addressed in the head commits.
  • Confirm the deliberate narrowing is acceptable for your providers: ENOTFOUND, certificate, truncated-error, and unknown errors no longer retry, and the old 'assistant message prefill' retry special case is gone.
  • Failed attempts still consume provider tokens and retries make no upstream exactly-once promise — worth a release note alongside the updated Chinese audit/decision docs.

Things to watch

  • ENOTFOUND, certificate failures, and truncated error graphs are deliberately terminal; a DNS outage that surfaces as ENOTFOUND will fail turns instead of retrying — a documented tradeoff that may generate support traffic during resolver incidents.
  • The old 'assistant message prefill' retry special case was removed; providers relying on transient prefill errors will now surface them as terminal validation failures.

Verification

🔎 Evidence supports the conclusion. Confirmed in source; not reproduced in a live run.

No host validation commands were run for this report.

Source evidence (15)
  • packages/util/src/network-error.ts:45 — classifyNetworkError walks a bounded cause/errors graph; ETIMEOUT/ETIMEDOUT, EAI_AGAIN, Undici connect/body timeout codes are transient; cert/invalid-name/truncation permanent; abort wins over all.

  • packages/util/src/retry.ts:47 — retryAfterMs strictly parses Retry-After and retry-after-ms (rejects -1, Infinity, trailing garbage); retry() adds equal jitter, a cancellable sleep, and the narrowed transient-only default retryIf.

  • packages/harness/src/provider/retry.ts:32 — providerRetryable: recording errors and exhausted AI_RetryError stay terminal; terminal codes (auth/quota/input) beat transient; 408/429/5xx-except-501/505 retryable; 409 needs explicit isRetryable.

  • packages/harness/src/provider/retry-coordinator.ts:47 — Process-local coordinator: hashed connection keys (endpoint/credential fields hashed, never stored), one recovery probe, revision guard so a stale success cannot clear a newer failure, five-minute admission bound, 1024-entry cap.

  • packages/harness/src/session/agent-turn/index.ts:79 — AgentTurn.stream now admits worker runs through recovery.stream, so shared cooldown and a single probe precede worker-pool admission; admission also rejects while the pool is stopping.

  • packages/harness/src/session/llm.ts:532 — wrapStream middleware normalizes stream-start failures into APICallError with accurate isRetryable before SDK retry admission, passing original errors through when the caller already aborted.

  • packages/harness/src/session/agent-turn/protocol.ts:9 — Worker protocol VERSION bumped to 9; serializeCause now carries nested cause/errors graphs with depth/count/text bounds and an explicit ERR_CAUSE_TRUNCATED marker.

  • packages/harness/src/session/processor.ts:1667 — retryEligible is set false once the provider stream has been consumed normally — before tool authorization and dispatch — so failures after tool dispatch can never replay the model (verified against the retry gate at line 1806).

  • packages/harness/src/session/processor.ts:1007 — Before a retry attempt: waits for tracked settlements, flushes part writes, removes failed-attempt parts not present in the pre-turn snapshot (keeping pre-existing parts, patches, step-finish), then resets message finish.

  • packages/harness/src/agent/call.ts:197 — AgentCall passes retries: 0 so SDK retries are disabled; the outer loop (line 184) owns the caller budget for startup and body failures under the original abort deadline composed from input signal, timeout, and output controllers.

  • packages/harness/src/session/retry.ts:26 — retryable narrowed to isRetryable-flagged APIErrors, worker-restart UnknownErrors, and providerRetryable-confirmed raw messages; delay now uses the shared strict Retry-After parser with capped jittered backoff.

  • packages/harness/src/session/rollout/call.ts:172 — settle records 'failed' whenever a failure was observed, so a consumed stream error stays failed even when the consumer stops reading and disposes.

  • packages/runtime-local/src/tools/webfetch.ts:116 — GET retried up to three attempts inside the single original timeout deadline (covering request, streaming body read, and backoff), one permission grant per invocation, caller cancellation reasons preserved.

  • packages/harness/test/session/processor-retry.test.ts:77 — Real-storage tests: a failed stream executes no tools while recovery executes once, withdrawal leaves only valid text in history and model context, rollout keeps failed+completed attempts and accounting, cancel keeps the partial output.

  • docs/decisions/implemented/bug-fix/2026-09-15-network-retry-boundaries.md:1 — Implemented decision record documenting shared classification, budget ownership per caller, failed-attempt withdrawal, coordination boundaries, rejected alternatives, and cost/exactly-once consequences.


Discussion and CI reflect the snapshot read for this review. Current checks and approval are verified separately before merging.

🧹 Oryn Mini · Source a024e0f · Built with Synergy core

@oryn-mini oryn-mini Bot added oryn:kind: 🐛 bug Behavior differs from the intended result. oryn:priority: 🟠 P1 High-priority impact supported by the review. oryn:proof: 🔎 supported Evidence supports this conclusion; see the report for source vs live proof. oryn:status: 🛠️ changes requested The review found concrete work that remains. labels Sep 15, 2026
@EricSanchezok
EricSanchezok merged commit ce08ef3 into dev Sep 15, 2026
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

oryn:kind: 🐛 bug Behavior differs from the intended result. oryn:priority: 🟠 P1 High-priority impact supported by the review. oryn:proof: 🔎 supported Evidence supports this conclusion; see the report for source vs live proof. oryn:status: 🛠️ changes requested The review found concrete work that remains.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant