Skip to content

fix: hold the client through long preflight compression (#568) - #571

Merged
ranxianglei merged 3 commits into
masterfrom
2026-09-06_preflight-hold
Sep 7, 2026
Merged

ranxianglei merged 3 commits into
masterfrom
2026-09-06_preflight-hold

Conversation

@ranxianglei

Copy link
Copy Markdown
Owner

What

Preflight compression sent zero bytes to the client until it finished. On big sessions it takes minutes (summarization round-trips to the same upstream — the issue logs show 48s–407s per run, with one finished run at 265,882ms), so any client whose header deadline expires mid-compression aborts. With undici's default headersTimeout of 300s the proxy then detects the disconnect, drops the in-flight compression, never forwards, and the client retries into the same wall — the repeating ~5-minute death loop from #568.

Fix

When preflight outlives a grace period (default 30s, override BILI_PREFLIGHT_HOLD_MS), the proxy commits the response early and holds the client with keep-alive bytes:

  • streams: 200 + text/event-stream + SSE comment keep-alives (: bili-preflight, every 15s — spec-mandated no-op for every SSE consumer, same pattern as OpenAI's pings)
  • non-stream: 200 + application/json + whitespace padding (valid JSON padding)
  • early commits carry an x-bili-preflight: compressing header for observability

Each byte resets undici's inactivity-based bodyTimeout, so the client is held for the whole compression regardless of duration.

Trade-off (documented): once the status is committed, late failures can no longer change it. They are delivered in-band instead — a protocol-native error event (preflight_compress_failed) for streams (per-protocol shapes: OpenAI data-frame error + [DONE], Anthropic/Responses event: error), and the identical structured error body under the already-sent 200 for JSON. Upstream non-2xx after an early commit is likewise delivered in-band (protocol error event / verbatim error body). Fast paths (preflight finishes within the grace) are unchanged and keep full status-code fidelity — all existing #301 fail-fast tests pass untouched.

Changes

  • src/server.ts: beginPreflightHold() + grace timer around preflightCompress; in-band fail-fast branch in handle(); headersSent guards in forward() (codex forge response, non-2xx upstream branch, 2xx commit)
  • src/stream-error.ts: emitPreflightError() — per-protocol in-band error emission, never throws
  • tests/preflight-hold.test.ts: 4 e2e cases (stream success, stream 429 fail-fast, Responses 429 fail-fast, non-stream success) with timing assertions proving headers arrive before the slow summarization completes and the response still waits for it
  • README.md: "Preflight hold" section

Both compression modes (AGENTS.md §6)

The hold is wire-level: plugin-mode responses pass through verbatim, where SSE comment lines are inert no-ops for ACP clients and a leading whitespace byte is inert JSON padding; the in-band error events use the same protocol shapes plugin mode already emits on stream errors. No mode-specific state is touched.

Pre-flight

  • npm run typecheck — clean
  • npm test — 1051/1052 pass; the single failure (launcher.test.ts: resolveClientCommand codex/claude resolve to themselves) also fails on the unmodified tree in this environment (it asserts command === 'codex' but this sandbox has a codex binary on PATH that resolves to an absolute path) — pre-existing, unrelated
  • npm run build — success
  • E2E: E2E_CHECK=1 node --import tsx --test tests/e2e/e2e-codex.test.ts passes (codex CLI + dist verified); the full ACP_TEST_E2E=1 run needs a reachable Responses-compatible upstream (http://127.0.0.1:8199/v1 default), which is unreachable from this environment — please run it (or the CI dispatch) before merging

@github-actions

github-actions Bot commented Sep 6, 2026 •

Copy link
Copy Markdown
Contributor

📦 Built Package Artifact

Branch: 2026-09-06_preflight-hold (e152a66)

Option A — Install from npm PR tag (recommended)

npm install -g billion-context@pr-571

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

Option B — Download artifact

  1. Download the artifact from the Actions run
  2. Extract the tarball and install:
tar xzf billion-context-pr571.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] 🏷 Reviewing PR #571 now. Plan: (1) re-run pre-flight on head a9b98a7 (typecheck / test / build / E2E_CHECK), (2) line-by-line review of the server.ts hold logic + stream-error.ts in-band emission, (3) verify both compression modes per AGENTS.md §6, (4) confirm the #301 fail-fast contract stays intact for fast paths. Will follow up with findings.

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 ework agent · qwen3.8-27b

[bot] 🏷 Review complete — LGTM with minor (non-blocking) notes. Full pre-flight re-run on head a9b98a7, line-by-line review of all 4 files, both compression modes checked per AGENTS.md §6.

Pre-flight re-verification (all claims confirmed):

  • npm run typecheck — clean
  • npm run build — clean (self-contained dist/index.js)
  • npm test — 1051/1052 pass, identical to the PR's claim. I isolated the single failure: tests/launcher.test.ts → resolveClientCommand: codex/claude resolve to themselves, and reproduced it on unmodified master (2cbbeab) in this environment too — it asserts command === 'codex' while a codex binary on PATH resolves to an absolute path. Pre-existing, environment-dependent, unrelated to this change (PR touches none of the launcher code).
  • E2E_CHECK=1 node --import tsx --test tests/e2e/e2e-codex.test.ts — passes. The full ACP_TEST_E2E=1 run still needs a reachable Responses-compatible upstream, which this environment can't reach — please run it (or the CI dispatch) before merging, per AGENTS.md.

Code review — key correctness points I verified:

  • Timer race is safe. If the grace timer fires first, stopHold is set and cleared in the finally; if preflightCompress settles first, its continuation runs as a microtask before any pending timer macrotask, so clearTimeout always wins. No double-commit, no leaked interval either way.
  • Fast path is byte-identical. Within the 30s grace the timer never fires → zero behavior change → the fix: honor anthropic-beta context-1m to size the context window #301 fail-fast contract (status-code fidelity, Retry-After on 503) is intact; all existing tests pass untouched, as claimed.
  • prepared! at src/server.ts:1523 is sound — prepared is assigned at src/server.ts:1497 before preflightCompressIfNeeded runs, and PreflightFailFast outcomes originate only inside it. Consistent with the existing prepared!.body style at :1559.
  • Abort path unchanged in semantics: client disconnect → res "close" → clientAbort.abort() → aborted fail-fast with respond: false → finally clears timer + keep-alive. Writes to the dead socket are all swallowed. Detection now simply happens after an early commit instead of after the full header budget.
  • Keep-alive byte choice is right: SSE comment lines (: bili-preflight) and leading JSON whitespace are spec-mandated no-ops — safer than synthetic ping data frames, which strict parsers could reject.
  • Both modes (§6): the hold is purely wire-level — framing decided by prepared.stream/prepared.protocol, inert bytes written, no mode-specific state touched. Plugin-mode responses pass through verbatim where comments/whitespace are inert for ACP clients; the in-band error shapes reuse the same protocol forms plugin mode already emits on stream errors. Confirmed against the code paths.
  • Duplicate screen: only related items are 上游长时间 prefill(首 token 前无输出)被 undici 默认 bodyTimeout=300s 掐断,UPSTREAM_TIMEOUT_MS=10min 配置形同虚设,大上下文请求必超时 #551/fix: align undici transport timeouts with idle watchdog; add BILI_UPSTREAM_TIMEOUT_MS (#551) #556 (undici bodyTimeout on the proxy→upstream leg during long prefills, fixed via BILI_UPSTREAM_TIMEOUT_MS). This is the client→proxy leg (headersTimeout while preflight holds) — distinct layer, distinct fix, not a duplicate.

Minor findings (report-only, none block merge):

  1. emitPreflightError drops the retryable field in the anthropic/responses error shapes (it's kept in the openai frame and in both JSON paths) — small inconsistency; easy to align if you want it.
  2. Non-stream in-band upstream failure when errBody === null (error body unreadable, after an early commit): the client receives 200 + whitespace-only padding → invalid JSON rather than a structured envelope. Extreme corner case (needs upstream error + broken body stream + >30s preflight); consider falling back to the structured error body there.
  3. Test coverage gaps: no direct case for the non-stream in-band failure (JSON error under the committed 200) nor for client-disconnect-during-hold. Fast-path fidelity is covered indirectly by the untouched fix: honor anthropic-beta context-1m to size the context window #301 suite; the two missing ones would round out the matrix.
  4. Nit: the keep-alive setInterval isn't .unref()'d (the grace timer is). Harmless — it's always cleared in finally or self-clears within 15s on a dead socket — but aligning them would be tidy.

Bottom line: root cause (zero-byte silence racing undici's 300s headersTimeout) is correctly addressed at the right layer — commit-early-and-hold is the standard remedy, the trade-off (lost status fidelity → in-band delivery) is explicitly documented in README + code comments, and the fast path keeps the #301 contract byte-for-byte. My recommendation: ship it once the full ACP_TEST_E2E run is green. Merging stays with you — I can't merge PRs (AGENTS.md), so please merge yourself: #571

@ranxianglei
ranxianglei force-pushed the 2026-09-06_preflight-hold branch from a9b98a7 to 5783e03 Compare September 6, 2026 16:21
@ranxianglei

Copy link
Copy Markdown
Owner Author

已在 5783e03 将本 PR 语义合并到当前 master 并重写为线性单提交(修 #575 同样的 diff-爆炸问题)。全量 1146/1146 通过、typecheck 0 错、build 成功。语义合并要点:#568 hold:hold 包装套在 master 现版 preflight 调用上(wireOverhead 保留),forward/fail 判定保持 master 的重建后实测(#330/#488/#496),PR 的 payloadEstimate 门弃用;in-band 错误投递(caller 层)原样保留。CI 绿后请人工合并。

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 ework agent · qwen3.8-27b

[bot] 🏷 Got it — re-reviewing the linearized head 5783e03 now. Plan: (1) confirm it's a single commit on top of current master and inspect the semantic-merge diff (hold wiring over master's newer preflight flow, master's post-rebuild measured fit decisions kept, PR's payloadEstimate gate dropped, in-band delivery preserved), (2) re-run typecheck / full test / build / E2E_CHECK on the new head, (3) re-check the earlier minor notes. Will follow up with results.

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 ework agent · qwen3.8-27b

[bot] 🏷 Re-review of linearized head 5783e03 complete — all claims verified, ready for manual merge once CI is green.

Linearity & mergeability (confirmed):

  • 5783e03 is exactly one commit, parent 3f57ce4; that parent is an ancestor of current master (2b510af) — the only commits between are the v0.1.88 release pair (01fd5de + merge #592), which touch only package.json/package-lock.json, no overlap with this PR's 4 files. git merge-tree simulation against latest master: clean, zero conflicts.

Semantic-merge fidelity (checked hunk-by-hunk vs the round-1 head a9b98a7):

  • Hold mechanism: added lines are byte-identical (30s grace / BILI_PREFLIGHT_HOLD_MS, 15s keep-alives, SSE-comment/whitespace padding, x-bili-preflight header, unref()'d grace timer + try/finally cleanup).
  • In-band delivery (caller layer) kept 原样: handle() headersSent fail-fast branch, emitPreflightError (src/stream-error.ts diff byte-identical), and all three forward() guards (codexForge, non-2xx upstream, 2xx commit) — byte-identical additions.
  • wireOverhead: overheadEstimate correctly carried over from master's preflight call (now context, not PR-added — the only substantive delta vs the old head, plus the import line picking up master's estimateRawBodyTokens).
  • Forward/fail decision: master's post-rebuild measured logic at src/server.ts:2934-2959 (#330/#488 image-cost/#496 comments intact) is untouched by the PR; the old payloadEstimate gate is fully gone — preflightCompress has exactly one caller (src/server.ts:2912, inside the hold wiring), and fit is decided on estimateCoreMessages(...) + overheadEstimate + imageTokens < limit.
  • tests/preflight-hold.test.ts: byte-identical to round 1, and passes against master's evolved preflight (all-viable-ranges etc.) — timing assertions still enforce headers-before-completion.
  • README section: identical content, cleanly anchored in the current doc.

Pre-flight re-run on 5783e03:

  • npm run typecheck — clean
  • npm run build — success
  • npm test — 1145/1146. The single failure is the known pre-existing environment-dependent one: tests/launcher.test.ts → resolveClientCommand: codex/claude resolve to themselves (expects command === 'codex'; here a codex binary on PATH resolves to an absolute path). I proved in round 1 that it fails identically on unmodified master, and this PR touches no launcher code — so your 1146/1146 stands for environments without that binary; nothing new here.
  • E2E_CHECK=1 … e2e-codex.test.ts — passes. Full ACP_TEST_E2E=1 still needs a reachable Responses-compatible upstream — please run it (or the CI dispatch) before merging, same caveat as before.

Earlier minor notes — status: all four still stand (expected, since the PR content was preserved verbatim): retryable dropped in anthropic/responses in-band shapes; non-stream errBody === null corner (200 + whitespace-only body); missing direct tests for non-stream in-band failure and disconnect-during-hold; keep-alive interval not .unref()'d. All optional polish — none block this merge; happy to see them as a small follow-up if you want.

Bottom line: the semantic merge is faithful (hold over master's newer preflight, master's measured fit decisions kept, payloadEstimate gate dropped, in-band delivery intact), the tree is green, and it merges cleanly onto today's master. Gate remaining: full E2E run, then human merge — #571 (I can't merge per AGENTS.md).

@ranxianglei

Copy link
Copy Markdown
Owner Author

已解决与 master(d0c941c,含 #593/#550/#558)的冲突,force-push 到本分支(5783e03 → 7bdcc73,保持单提交)。

冲突点(#558 同改 preflight 区):

  1. src/server.ts import 行 — 并集保留:fix: gate fork sessions on char-count upper bound in preflight (#553) #558 的 estimateCoreMessagesUpper + 本 PR 的 type PreflightResult。
  2. preflightCompress 调用点 — 保留本 PR 的 hold 包装(grace timer + beginPreflightHold + try/finally),deps 对象补回 fix: gate fork sessions on char-count upper bound in preflight (#553) #558 新增的 unknownBaseline(fork/未知基线会话走字符上界判体积)。

语义交互已核:#571 的早提交(200 + keep-alive)与 #558 的 unknownBaseline fail-fast 共存 — grace 内失败仍走结构化 5xx(#301 全过),grace 外失败走 in-band 错误事件;fits 判定沿用 #558 的 result.fitsWindow(未知基线)/乐观重估(已知基线)二分,未被本 PR 触碰。

Pre-flight:typecheck ✓;定向 preflight-hold 5 + preflight-fork-estimate 2 + preflight-fail-fast 4 = 11/11 ✓;全量 1155/1157(2 个失败为 plugin-agent.test.ts 已知环境失败,与改动无关);build ✓。

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 ework agent · qwen3.8-27b

[bot] 🏷 Got it — re-reviewing 7bdcc73. Since #558 touched the same preflight region, I'll focus on: (1) linearity vs d0c941c + clean-merge simulation, (2) the two conflict resolutions you listed (import union with estimateCoreMessagesUpper; hold wrapper kept at the call site with unknownBaseline restored), (3) confirming the fits decision (result.fitsWindow unknown-baseline / optimistic re-estimate known-baseline split) is untouched by this PR, (4) that the rest of the PR content survived the rebase byte-identical, (5) full pre-flight re-run incl. identifying the 2 plugin-agent.test.ts failures and proving they're pre-existing on the parent. Will follow up with results.

@ranxianglei
ranxianglei force-pushed the 2026-09-06_preflight-hold branch from 7bdcc73 to dce7502 Compare September 7, 2026 00:47
@ranxianglei

Copy link
Copy Markdown
Owner Author

补上了:README.zh-CN.md 的「工作原理」末尾新增「预压缩挂起(#568)」小节,与英文 README 的 Preflight hold 段逐句对应(宽限期 30s / BILI_PREFLIGHT_HOLD_MS / SSE 保活与空白填充 / 带内错误投递 / 快路径不变)。amend 进同一提交(7bdcc73 → dce7502)。

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 ework agent · qwen3.8-27b

[bot] 🏷 Got it — verifying dce7502. Plan: confirm the amend is docs-only (git diff 7bdcc73..dce7502 = README.zh-CN.md alone, code untouched), check the zh section against the English one, then finish the interrupted full pre-flight (typecheck ✓ already on 7bdcc73; re-confirming on dce7502 plus full test / build / E2E_CHECK), and inspect the two #558 conflict resolutions (import union, unknownBaseline restored at the call site) and the fits-decision split. Results to follow.

Semantic merge onto current master: the hold wrapper (grace timer +
beginPreflightHold early-commit + try/finally stop) now wraps the
master-side preflight call including wireOverhead, and the forward/fail
gate keeps master's rebuilt-payload check (#330/#488/#496) instead of
the older payloadEstimate gate. In-band error delivery after an early
commit (#568) unchanged.
@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 ework agent · qwen3.8-27b

[bot] 🏷 Re-review of dce7502 complete — everything verifies, ready for manual merge once CI/full E2E is green.

Linearity: single commit whose parent is current master tip d0c941c (= origin/master after my ref refresh — it had advanced past 2b510af with exactly the #593/#550/#558 merges you cited). Direct child of the tip → fast-forwardable, zero conflict surface. The 7bdcc73 → dce7502 amend is docs-only: git diff shows README.zh-CN.md +6 lines alone; I checked the「预压缩挂起(#568)」section against the English one — sentence-by-sentence correspondence with every detail intact (30s grace / BILI_PREFLIGHT_HOLD_MS / 15s : bili-preflight SSE comments / whitespace padding / x-bili-preflight header / in-band preflight_compress_failed / fast path unchanged). ✓

Conflict resolutions (hunk-by-hunk):

  1. Import line: union confirmed — fix: gate fork sessions on char-count upper bound in preflight (#553) #558's estimateCoreMessagesUpper + this PR's type PreflightResult, nothing dropped. ✓
  2. Call site: hold wrapper (grace timer + beginPreflightHold + try/finally) wraps the call; comparing the removed vs added call expressions, the deps field sets are identical — unknownBaseline and wireOverhead both carried over verbatim. Only delta is the wrapper itself. ✓
  3. Fits decision untouched: src/server.ts:2989-3004 shows the fix: gate fork sessions on char-count upper bound in preflight (#553) #558/fork 会话 lastInputTokens=0 时 preflight 退化为字符估算,大载荷被低估后不压缩直接裸转上游 #553 split — unknownBaseline ? result.fitsWindow : estimateCoreMessages(...) + overheadEstimate + imageTokens < limit — appearing purely as diff context (not modified by this PR), with the explanatory comment at :2986-2988 intact. ✓
  4. Semantic interaction holds in code: within-grace failures keep headersSent === false → original structured 5xx path (fix: honor anthropic-beta context-1m to size the context window #301) byte-identical; post-commit failures take the in-band branch. All preflight-fail-fast + preflight-multi-range + fork-estimate tests pass in my full run.

Pre-flight re-run on dce7502: typecheck ✓ · build ✓ · E2E_CHECK=1 ✓ · npm test 1156/1157. The single failure here is the known pre-existing environment-dependent one (launcher.test.ts: resolveClientCommand: codex/claude resolve to themselves — asserts 'codex' while a codex binary on PATH resolves to an absolute path; proven failing on unmodified master back in round 1, unrelated files). Notably the two plugin-agent.test.ts failures you saw did not reproduce in this environment — those pass here — so your 1155/1157 and my 1156/1157 are consistent: both are env-dependent, none touch this PR's code paths.

Content fidelity: src/stream-error.ts and tests/preflight-hold.test.ts (4 e2e cases) remain byte-identical to the round-1-reviewed versions; src/server.ts additions differ from round 1 only by the three expected integration deltas above. The four minor polish notes from earlier still stand (retryable in anthropic/responses shapes, errBody===null corner, two missing test cases, interval .unref() nit) — optional follow-up, non-blocking.

Bottom line: conflict resolution is correct, the semantic interaction with #558 is sound, tree is green, merge is a clean fast-forward onto today's master. Remaining gate: full ACP_TEST_E2E=1 run (or CI dispatch), then human merge — #571 (I can't merge per AGENTS.md).

@ranxianglei
ranxianglei force-pushed the 2026-09-06_preflight-hold branch from dce7502 to 19f0fbb Compare September 7, 2026 00:52
@ranxianglei

Copy link
Copy Markdown
Owner Author

文档位置修正 + 补 env 参考:README.zh-CN.md 的「预压缩挂起(#568)」小节从「工作原理」(概览节,插深潜内容头重脚轻)移到「运行代理 → 远程 agent 之后、调试之前」——这段的读者是已在运行、遇到请求挂住的人;另外 补进 CONFIGURATION.md / CONFIGURATION.zh-CN.md 的环境变量参考表(此前只出现在 README 英文档)。单提交 amend 到 19f0fbb。

@ranxianglei

Copy link
Copy Markdown
Owner Author

(更正上条:反引号被 shell 吃了,补一句)补进 CONFIGURATION.md / CONFIGURATION.zh-CN.md 环境变量参考表的是 BILI_PREFLIGHT_HOLD_MS(预压缩挂起宽限期,默认 30000)。

Removed section on preflight hold and its implications for response handling.
@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 ework agent · qwen3.8-27b

[bot] 🏷 Amend 19f0fbb verified — docs-only, accurate, and the PR still merges green onto today's master.

The amend (dce7502 → 19f0fbb) touches exactly 3 doc files:

Master moved past the PR parent again: d0c941c..ba55fea = #583/#585 (second-chance developer→user compat learn), which edits src/server.ts — including inside forward(), the same function this PR guards. So I didn't stop at git merge-tree (clean) — I materialized the actual merge result (PR head + current master tip) in a throwaway worktree and ran the full pre-flight there:

So the semantic interaction with #583's role-rejection retry ladder is safe: with an early-committed 200, the PR's headersSent branch delivers the upstream failure in-band instead of re-attempting status-code fidelity, and nothing double-terminates the response.

State: single commit, parent d0c941c (ancestor of master tip ba55fea). Either let GitHub land it as a merge commit (verified green above) or re-linearize onto ba55fea per your usual pattern — my verification carries over since the amend was docs-only. Remaining gate: full ACP_TEST_E2E=1 run or CI dispatch, then human merge — #571 (I can't merge per AGENTS.md).

@ranxianglei
ranxianglei merged commit eb0f532 into master Sep 7, 2026
6 checks passed
ranxianglei pushed a commit that referenced this pull request Sep 26, 2026
…OW_MS (#1397)

Env-var mirror rule (#571): the two shadow-mode tuning knobs introduced by step 2 were only in the PR description.
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