Repository navigation
fix: bounded transparent retry for fail-fast transport failures on the main model-request path (#1688) - #1693
Conversation
📦 Built Package ArtifactBranch: Option A — Install this exact build (recommended)npm install -g billion-context@0.0.1679-pr.1693.0f81aa9This exact version is immutable and reproducible. The rolling Option B — Download artifact
tar xzf billion-context-pr1693.tgz
npm install -g packageInstall instructions are refreshed on each push. |
[bot] 🏷 独立复核完成(未重做工作:拉取分支逐行审代码 + 本地全量验证 + CI 状态核对)。结论:可以合并(human-gate,等你合并)。 独立验证(全部在本机对 head 61bb12d 执行,base = 当前 master c0e9e65,无落后)
审查记录(按规范逐项)① 原 issue 方向(#1688): 根因定位正确(主转发 hop 裸调单次 ② 回归审查(diff 触碰路径的既有行为清点):
③ 代码审查: 新函数是 ④ 测试有效性: 真 loopback http/net server、零 mock;断言全部有牙——精确尝试计数(retries.length、relay.streamingCalls()===2/3/1)、精确状态码与响应体、错误链文本(ENOTFOUND/ECONNREFUSED)、指数退避 ⑤ diff 整洁度: 单 commit、7 文件、432+/6-,每行都服务于本 PR 目的;无空行/重排噪音、无 lockfile churn、无版本号改动、无跑题文件。CONFIGURATION.md 中英两版同步更新(仅澄清现有旋钮的作用域扩展,未新增任何配置字段——符合配置面纪律)。 行为变更确认(old → new,与 PR 描述一致,核实无误)
一句话中文摘要:独立复核了主模型请求路径 fail-fast 传输有界重试(仅重放响应前网络死亡、复用现有两个旋钮、HTTP verdict 一律透传不改写)的代码、回归面与测试,本地 typecheck/3238 项测试/build 全绿且 CI 13 项检查(含真实 codex e2e)全绿,未发现回归,可以合并。 (合并操作按规范由你执行:#1693 ) |
…e main model-request path (#1688) The main forward hop was the only request path still doing a single attempt: one millisecond DNS/reset/refused/connect-timeout blip killed the whole round (502 'acp-proxy failure') while acp-loop/preflight/compress loops already replayed under fetchWithRetry (#1263/#189). - fetch-util: new fetchWithTransportRetry — replays ONLY pre-response fail-fast network deaths (isFailFastUpstreamKind) under the existing BILI_REPLAY_RETRY_MAX / BILI_REPLAY_RETRY_BASE_MS budget; any HTTP verdict (ok/4xx/5xx) passes through untouched. fetchWithRetry itself is unsuitable here: it also retries transient HTTP responses and throws UpstreamHttpError on non-ok, which would convert verbatim upstream error bodies into proxy 502s. - server.ts: main forward hop now uses it; each transparent replay logs a warn line with kind/delay/attempt. - tests: new tests/main-path-transport-retry.test.ts (unit: refused/reset/DNS recovery, budget exhaustion, backoff shape, max=1 legacy switch, abort-during-backoff; server-level: reset mid-round -> one 200, persistent failure -> 502 after exactly 3 attempts, 4xx verbatim passthrough). - e2e-relay-5xx-selfheal / issue1493-arm-outbound-payload pin BILI_REPLAY_RETRY_MAX=1: they assert the single-attempt failure shape. - CONFIGURATION.md/.zh-CN: document the extended scope of the two knobs. Behavior change (old -> new): main model-request transport failures of fail-fast kinds previously failed the round after 1 attempt; now up to 3 attempts total (default) with 1500ms->3000ms backoff before surfacing the same 502. BILI_REPLAY_RETRY_MAX=1 restores the old behavior exactly. No new config surface; HTTP-level responses are never retried on this path.
61bb12d to
a3fcf21
Compare
Closes #1688
What & why
The main forward hop (
src/server.tsforward()) was the only request path still doing a single attempt: one millisecond DNS/reset/refused/connect-timeout blip killed the whole round (502{"error":"acp-proxy failure"}) — while acp-loop re-requests (src/loop/core.ts), compress loops and preflight already replayed underfetchWithRetry(#1263/#189). This makes the main path symmetric with them.Design note (one refinement over the issue's literal proposal): the issue suggested switching the main hop to the existing
fetchWithRetry. It cannot be used directly for two reasons:fetchWithRetryalso retries transient HTTP responses (429/5xx/risk-control 4xx) — the issue explicitly scopes this fix to fail-fast kinds only (pre-response failures, no double-delivery risk).UpstreamHttpErrorfor any non-ok response. On the main path that would convert today's verbatim upstream 4xx/5xx passthrough into proxy 502s — clients would never see upstream error bodies again. A regression, not a fix.So this adds a thin sibling,
fetchWithTransportRetryinsrc/fetch-util.ts, implementing exactly the issue-specified semantics:isFailFastUpstreamKind: proxy-reset | upstream-reset | connect-refused | connect-timeout | dns);BILI_REPLAY_RETRY_MAX/BILI_REPLAY_RETRY_BASE_MSknobs (same budget, same exponential backoff as [Enhancement] 代理上游连接异常缺乏区分性诊断（代理 reset vs 超时 vs 客户端 abort）+ 握手层健壮性评估 #1263) — no new config surface;fetchWithTimeoutshape (response/clearTimer/stopIdleTimer), so the existing rebuild blocks and finally-cleanup inforward()are unchanged.Each transparent replay logs a warn line with kind/delay/attempt (e.g.
[acp-secondary] upstream dns (pre-response network failure): getaddrinfo ENOTFOUND ...; retrying in 1500ms (attempt 1/3)).Behavior change (old → new)
{"error":"acp-proxy failure"}→ round deadZero-drift switch:
BILI_REPLAY_RETRY_MAX=1restores the old single-attempt behavior exactly (used by the two tests that pin the old shape). Intended per issue: users on weak networks get transparent recovery; anyone who wants the legacy behavior sets the env var.Existing tests that assert the single-attempt failure shape (
tests/e2e-relay-5xx-selfheal.test.ts,tests/issue1493-arm-outbound-payload.test.ts) now pinBILI_REPLAY_RETRY_MAX=1— their assertions (first socket drop → immediate 502 → emergency-shrink arm) are preserved byte-for-byte; without the pin their attempt 2 would succeed and they'd observe a 200 instead of the asserted 502.Tests
New
tests/main-path-transport-retry.test.ts(10 tests):*.invalid) → rejects after exactly 3 attempts with ENOTFOUND in the cause chain, retry details classifieddns; 400 verdict returned untouched with exactly one upstream call (guards the UpstreamHttpError-conversion regression);BILI_REPLAY_RETRY_MAX=1→ legacy single attempt; backoff delays are[50, 100]at BASE_MS=50 (exponential); client abort during backoff exits immediately.startServer(): destroy first streaming response mid-round → client gets one 200 with full SSE (the issue's exact repro) and the relay saw exactly 2 calls; always-destroy relay → 502/upstream request failed/after exactly 3 calls (budget respected, no storm); 400 verdict → verbatim passthrough, 1 call.Pre-flight on the rebased head (base = current master c0e9e65, v0.1.172):
npm run typecheck— cleannpm test— 3238 pass / 0 fail / 12 skipped (first run had one unrelated timing flake intests/fix-437-idle-timeout.test.ts, which imports onlyfetchWithTimeoutdirectly and passes 3/3 in isolation; second run fully green)npm run build— success, dist-import-annotations OKE2E_CHECK=1) — green locally (codex binary + dist present; the sandbox has no reachable Responses-compatible upstream, so the token-costing full run executes in CI, which auto-triggers because this diff touchessrc/server.ts)No config surface added. No version bump. No lockfile churn.