[PR] fix: honor backpressure and completion when dumping SSE streams - #6052

🤖 Powered by ework · qwen3.8-27b

Dedup: not a duplicate. Searched open+closed issues/PRs by failure signature (SSE dump ignores backpressure / resolves before file completion / sink error leaves source readers active) — only #6042 (the source bug this fixes) and #6052 (this PR) match. The related items cited (#113/#787/#2351/#2356) provide no sink coordination; none overlaps.

Labels applied to #6042: bug + scope/wiring + p2 — an opt-in diagnostic-dump resource-hygiene defect (ignored write backpressure → bounded memory growth; incomplete dumps; lingering source reader on sink error); only triggers when ACP_DUMP_SSE/dumpSse is enabled, never affects the live path.

Independent review (external-contributor PR — judged against the diff, not the issue's claims):

Direction / root cause. The three reported defects share one root cause: the hand-rolled pump loop in src/server/stream-io.ts:dumpStreamToFile moves bytes but never coordinates the three things a pipe must — backpressure (ws.write() return ignored, source drained unconditionally), completion (the promise resolves when the read loop ends, not when ws.end()'s callback fires → resolves before the file is flushed), and error cancellation (ws.on("error") only logs; the reader keeps running after a sink failure). The fix swaps the loop for Node's stream/promises.pipeline, the canonical primitive giving exactly these three guarantees. Fixing the mechanism, not masking a symptom. Correct direction.

Verified locally on the PR head (105b84d; base is current master, stream-io.ts unchanged across the range):

• Targeted test tests/sse-dump-backpressure.test.ts: 2/2 pass.
• Teeth check: swapped in the pre-fix stream-io.ts → both tests fail ("a blocked sink must not drain the entire source"; cancelled === true) → genuine regression coverage, not tautologies. Restored cleanly.
• npm run typecheck: clean. npm run build: success (commit 105b84d, dist-import-annotations OK, dist-external OK).
• Extra probe (not covered by the PR's test, which uses a direct source rather than the production tee shape): raw upstream tee() → [primary, dump]; slow dump sink (5 ms/write) → primary still drained 200 KB in ~45 ms while the dump side stayed pending. Confirms the new sink backpressure does not throttle the client-facing response — no live-path regression from the tee interaction.

Regression audit. The dumped stream is always an independent tee() branch (src/loop/core.ts:644, src/server/relay.ts:1903); the primary response uses the sibling branch, so pipeline's cancel-on-error cannot disturb the client path. Error semantics preserved (old = swallow-and-log; new = pipeline rejects → same outer try/catch → logDumpFailure → resolve; still not fail-fast). gcDumpDirIfConfigured timing equivalent-or-better (now reliably after full flush). No config-schema / persistence-version / wire-message-shape / kernel/ / msg-ref-id / security changes. Mode-agnostic (relay egress infra; both plugin and proxy modes stream through it).

Intended behavior change (disclosed): on sink error the source dump branch was previously drained to natural completion; it is now cancelled early. That is literally the fix for defect #3, and since the branch is throwaway tee output it frees resources sooner with zero client-path impact.

CI: at last mirror sync (~15:15 UTC) the suite is still running — several shards completed/success, remainder in_progress/queued, no failures yet. Final merge gate = full remote CI green on the pushed head.

Quality bar (external contributor, strict list): clean naming, no dead code, no duplication, complete error handling, no stray constants outside self-documenting tests, no new deps (built-in module only), consistent style. Meets standard — no restructure required.

审核八问

1. 是什么问题：SSE 诊断落盘（opt-in）用手写泵循环写文件，同时漏掉背压、完成等待、错误取消三件事（同一根因：循环只搬字节、不做 sink 协调）。
2. 什么原因：ws.write() 不看返回值/不 drain → 背压被忽略、源被抽干进缓冲；promise 绑在读循环结束而非 ws.end() 回调 → 提前 resolve；ws.on("error") 只记日志不取消 reader → sink 失败源残留。修法换官方 pipeline 原语，一个原语补齐三保证——修机制，非堵症状。
3. 影响范围与严重程度：仅启用 ACP_DUMP_SSE/dumpSse 时触发，未启用者零影响。启用后：慢盘时 Writable 缓冲随单条响应增长（有界、非跨轮无界，故非 p1 缓存泄露类）、dump 可能不完整、sink 出错后分支读到自然结束才停。主链路实测不受影响。定级 p2（个别功能异常）。
4. 怎么修复：手写循环 → await pipeline(stream, ws)，统一背压暂停 / final-flush 等待 / 任一侧出错销毁对侧；gcDumpDirIfConfigured 移到 pipeline 成功后（≈原 ws.end 回调时机且更可靠）；外层吞错记日志语义不变。
5. 是否回归：主响应路径无回归（dump 恒为独立 tee 分支，实测慢盘下主分支 44.8ms 抽干 200KB 不受拖）；错误处理仍吞错、不 fail-fast，无漂移；gc 时机等价或更好；无 config/persistence/wire/kernel/msg-ref/security 改动。唯一有意变更：sink 出错时源分支从"抽干到自然结束"改为"提前 cancel"（即修 Use kernel renderTags:"text-only" + forbid tag echoing in prompt #3 本身，丢弃式分支，客户端零影响）。
6. 是否破坏缓存：不涉及（纯诊断旁路，不碰 prefix cache / cache ref / 重试游标 / 去重键）。
7. 是否改设计方向：不涉及，仅把有缺陷的手写循环换成标准原语，边界/数据流/模块不变。
8. 是否引入新配置：不涉及，无新 env/config/默认值；仅新增内建 import（node:stream/promises），零运行时依赖。

判定：可合（mergeable）。 本地定向验证全绿（回归测试 2/2 且对旧代码可复现失败、typecheck、build）；远端 CI 最近一次同步仍在跑、暂无失败。建议 CI 转绿后由 owner 合并。质量达标，无需重构对齐。

Reviewed by opencode agent (qwen3.8-27b).

🤖 Powered by ework · qwen3.8-27b

[bot] 🏷 Status confirm — the full review is already posted above (floor 2); this is a short cross-reference, not a repeat of it.

What I did and outcome:

• Dedup: not a duplicate — only [Bug] Honor backpressure and completion when dumping SSE streams #6042 (source bug) + [PR] fix: honor backpressure and completion when dumping SSE streams #6052 match the failure signature; cited related items (fix: novel review bugs (registry ESM, logger rotation, persist drain, dump error, tarball cap) #113/refactor: split server.ts phase 1 leaf utilities #787/fix: dumpSse covers plugin lane and non-injected streaming turns (#2347) #2351/[PR] feat: plugin-lane stream-termination observability (#2328) #2356) provide no sink coordination.
• Labels: applied bug / scope/wiring / p2 to [Bug] Honor backpressure and completion when dumping SSE streams #6042.
• Local verification on PR head 105b84d (all green): targeted test tests/sse-dump-backpressure.test.ts 2/2 pass, and both tests fail against the pre-fix code (teeth confirmed); npm run typecheck clean; npm run build OK.
• Extra probe beyond the PR's own test: a slow dump sink does not throttle the live tee() branch (primary drained 200 KB in ~45 ms) → no live-path regression.
• Intended behavior change disclosed: on sink error the dump branch now cancels early instead of draining to completion (that is the fix; throwaway branch, zero client impact).

Verdict: mergeable. Remote CI was still running at last mirror sync (~15:15 UTC) with no failures; the remaining gate is full remote CI green before owner merges. Quality meets the external-contributor bar — no restructure needed.