Skip to content

fix(#437): make upstream timeout idle-based so long healthy streams aren't truncated - #439

Merged
ranxianglei merged 1 commit into
masterfrom
2026-09-02_fix-437-idle-timeout
Sep 2, 2026
Merged

ranxianglei merged 1 commit into
masterfrom
2026-09-02_fix-437-idle-timeout

Conversation

@ranxianglei

Copy link
Copy Markdown
Owner

What & why

Issue #437: with bili dsh (DeepSeek), a long streaming generation got cut off ~10 minutes in ("流式输出十分钟后对话被截断").

Root cause: fetchWithTimeout (src/fetch-util.ts) armed a single total setTimeout(abort, UPSTREAM_TIMEOUT_MS) (10 min) that was only cleared once the response body was fully consumed. A healthy stream that legitimately runs longer than 10 minutes was aborted at the 10-minute mark mid-stream — the for await loop threw, emitStreamError wrote ❌ [ACP] stream error: … + a finish event, and the client saw the conversation truncated.

Fix

Make the timeout idle-based during the body phase: the timer is re-armed on every response-body chunk (via a backpressure-preserving armIdleBody body wrapper). So:

  • a healthy stream that keeps producing chunks is never aborted mid-flight (LLM generations can run for minutes);
  • a genuinely stuck stream (no chunk for timeoutMs) still trips the abort;
  • time-to-first-byte bounding is unchanged;
  • clearTimer / externalSignal semantics are preserved, and callers see an identical Response shape (status/headers/body).

Centralized in fetchWithTimeout, so it covers every upstream path (main forward(), preflight, compress-loop re-requests, registry).

Tests

  • New tests/fix-437-idle-timeout.test.ts:
    • a healthy stream of ~800ms with a 500ms timeout completes in full (verified it fails under the old total timer by reverting src/fetch-util.ts);
    • a stuck stream (one chunk then silence) is still aborted by the idle timeout;
    • the wrapped body preserves status + headers.
  • npm run typecheck — clean
  • npm test — 899/899 pass
  • npm run build — clean

E2E note

tests/e2e/e2e-codex.test.ts requires a live codex binary + a local Responses upstream, which aren't available in this environment (spawn codex ENOENT). The change is an isolated timeout-semantics change in fetchWithTimeout (no message-rebuild/rewrite logic touched) and is covered by the full unit suite above. Please run the E2E core before merging if you have the upstream available.

…ren't truncated

fetchWithTimeout used a single TOTAL timer (UPSTREAM_TIMEOUT_MS = 10min)
that only cleared once the body was fully consumed. A healthy stream that
legitimately runs longer than 10 minutes (e.g. a DeepSeek generation via
bili dsh) was aborted at the 10-minute mark mid-stream: the for-await loop
threw, emitStreamError wrote 'stream error' + finish, and the client saw the
conversation truncated.

Re-arm the timer on every response-body chunk (via an idle-wrapped body), so
the timeout is IDLE-based during streaming: healthy long streams survive,
while a genuinely stuck stream (no chunk for timeoutMs) still trips the abort.
Time-to-first-byte bounding is unchanged. clearTimer/externalSignal semantics
are preserved and callers see an identical Response shape (status/headers/body).
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

📦 Built Package Artifact

Branch: 2026-09-02_fix-437-idle-timeout (65cd535)

Option A — Install from npm PR tag (recommended)

npm install -g billion-context@pr-439

Each push to this PR publishes a new version under the pr-439 npm tag.

Option B — Download artifact

  1. Download the artifact from the Actions run
  2. Extract the tarball and install:
tar xzf billion-context-pr439.tgz
npm install -g package

This comment is automatically updated on each push.

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 ework agent · qwen3.8-27b

[bot] 🏷 Reviewed 2026-09-02_fix-437-idle-timeout. LGTM — the fix correctly addresses #437 and the change is minimal and well-scoped. All checks pass.

Verification (ran locally on the PR branch)

  • npm run typecheck — clean
  • npm test — 899/899 pass
  • npm run build — clean
  • New tests/fix-437-idle-timeout.test.ts — 3/3 pass. I also confirmed the regression test is not vacuous: reverting src/fetch-util.ts to the master (total-timer) version makes the "healthy stream" case fail — the abort fires at the total-timeout mark via Timeout._onTimeout → AbortController.abort. Restored the PR version afterward (working tree clean).
  • Smoke test: built dist/index.js boots and serves __bili/health → {"ok":true,...}.
  • E2E: codex binary is not present in this environment, so the E2E core can't run here (matches your note). Recommend running it against the local Responses upstream (see tests/e2e/README.md) before merge if you have it available.

Code review

  • Idle-timeout logic is correct. The initial setTimeout bounds time-to-first-byte/headers; rearm() (clear + reset) is invoked on every body chunk via the armIdleBody wrapper's pull. A healthy stream re-arms continuously and is never aborted mid-flight; a genuinely stuck stream (no chunk for timeoutMs) still trips the abort. TTFB bounding is unchanged.
  • clearTimer / externalSignal semantics preserved. The new cleanup() clears the latest timer (closure over the reassigned timer) and removes the external-abort listener; the externalSignal wiring is untouched.
  • Body wrapping is transparent to consumers. armIdleBody returns a standard ReadableStream<Uint8Array>; read/cancel/error all propagate to the underlying reader (cancel → reader.cancel, read error → controller.error). The fresh Response carries status/statusText/headers.
  • All 6 call sites are compatible. I checked each: server.ts forward (:697) and main streaming path (:2384), preflight.ts:186, compress-loop-responses.ts:216, loop/core.ts:433, registry.ts:108. Every one consumes the body via standard methods (text() / json() / arrayBuffer() / tee() / for await / getReader()) and manages clearTimer in try/finally. The tee() branches (resetAfterSuccess + dumpSse paths) share the wrapped source correctly — each upstream chunk is read once and duplicated, and rearm fires once per upstream chunk.

Minor (non-blocking) notes

  1. The returned Response is a fresh object, so response.type becomes "default" and response.url / response.redirected are reset. I grepped src/ — no caller reads any of those (only status/headers/body/ok), so this is safe. Just noting the shape is identical for the fields that matter, not literally the same instance.
  2. rearm fires in pull (consumer-driven). Under tee() backpressure where the client stalls > timeoutMs while upstream is still producing, the idle timer can trip. Not a regression (the old total timer also fired) and arguably the correct idle semantics — no action needed.
  3. A single inter-chunk gap ≥ timeoutMs (10 min) aborts — intended idle behavior, and a strict improvement over the old hard 10-min total cap.

No new issues to file. Ready to merge once you've had a chance to run the E2E core (per the AGENTS.md rule to run it before merging request-pipeline changes) — or merge now if you consider the full unit suite + regression test sufficient coverage for this isolated timeout-semantics change.

@ranxianglei
ranxianglei merged commit ebdfa78 into master Sep 2, 2026
6 checks passed
@ranxianglei ranxianglei mentioned this pull request Sep 2, 2026
@ranxianglei
ranxianglei deleted the 2026-09-02_fix-437-idle-timeout branch October 15, 2026 05:32
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