fix(replay): bound detached stream memory (#1408) - #1413
Conversation
github-actions
Bot
added
bug
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c36678adb7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private trackQueuedBatch(batch: QueuedReplayBatch): void { | ||
| this.queuedBatches.add(batch); | ||
| if (this.activeWriteBatch) { | ||
| this.queuedBytes += batch.byteSize; | ||
| } else { | ||
| this.activeWriteBatch = batch; | ||
| } |
There was a problem hiding this comment.
In the normal owner path, bootstrap() first places an untracked Redis write on writeChain, but the first response batch is still marked as activeWriteBatch here and excluded from queuedBytes. If the bootstrap Redis call stalls, that batch is actually waiting behind it and can retain up to REPLAY_MAX_PAYLOAD_BYTES because the write-behind check exempts it; with the configured concurrent-spool limit, slow Redis can therefore recreate the large detached-stream heap growth this change is intended to prevent. Only exempt the batch once its write callback actually starts, or account for it while any preceding chain operation is in flight.
Useful? React with 👍 / 👎.
📝 WalkthroughWalkthroughChangesReplay 生命周期
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/app/v1/_lib/proxy/replay/replay-spool.ts (1)
132-132: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win热路径上为计算字节数分配了完整副本。
this.encoder.encode(text)会为每个 chunk 分配一份与文本等长的Uint8Array，随后立即丢弃。该调用位于流式热路径，且本 PR 的目标是降低堆压力。改用Buffer.byteLength(text, "utf8")可以只计算长度，不分配缓冲区。同样的替换也适用于 Line 302 的尾部计量。请确认该模块只在 Node.js runtime 执行（
Buffer在 edge runtime 不可用）。♻️ 建议改动
- this.pendingBytes += this.encoder.encode(text).byteLength; + this.pendingBytes += Buffer.byteLength(text, "utf8");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/v1/_lib/proxy/replay/replay-spool.ts` at line 132, Replace the per-chunk encoder allocation used to update pendingBytes with Buffer.byteLength(text, "utf8"), and apply the same change to the trailing byte accounting near the second measurement site. Confirm replay-spool.ts runs only in the Node.js runtime before relying on Buffer, preserving UTF-8 byte counts without creating Uint8Array copies.src/app/v1/_lib/proxy/replay/replay-store.ts (1)
84-97: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win可选：避免在 Lua 中复制整个 chunks 表。
当前实现先
LRANGE得到chunks，再逐项复制到result。对大 payload，这会在 Redis 的 Lua 栈内短暂占用两份完整正文。使用table.insert(chunks, 1, 1)可以原地加前缀，减少一份副本。该改动不改变返回结构。♻️ 建议改动
local chunks = redis.call('LRANGE', KEYS[2], 0, -1) -local result = {1} -for i = 1, `#chunks` do - result[`#result` + 1] = chunks[i] -end -return result`; +table.insert(chunks, 1, 1) +return chunks`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/v1/_lib/proxy/replay/replay-store.ts` around lines 84 - 97, Update LUA_READ_OWNED_CHUNKS to prepend the success marker directly to the table returned by LRANGE using an in-place insertion, then return that table. Remove the separate result table and chunk-copying loop while preserving the existing ownership and length checks and the {1, ...chunks} response structure.tests/unit/proxy/replay-spool.test.ts (1)
691-692: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value可选：补充
readOwnedChunks返回null的用例。当前测试覆盖了 reject（异常）与
false（token 失效或数量不完整）两条分支。实现中还有chunks === null分支，对应 Redis 不可用，错误信息为"final replay payload read failed"。该分支决定了 Redis 短暂不可用时不会误写 PG，值得单独覆盖。💚 建议补充用例
+ it("Redis 不可用导致 payload 读取返回 null 时不持久化 PG", async () => { + storeControl.store.readOwnedChunks.mockResolvedValueOnce(null); + const spool = makeSpool(); + spool.observe(encoder.encode("data: partial\n\n")); + + await spool.completeAfterBilling(12); + + expect(storeControl.store.persistCompleted).not.toHaveBeenCalled(); + expect(storeControl.store.abortOwned).toHaveBeenCalled(); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/proxy/replay-spool.test.ts` around lines 691 - 692, 在 replay-spool 测试中补充 `readOwnedChunks` 返回 `null` 的独立用例，覆盖实现中的 `chunks === null` 分支。参考现有 fenced payload 读取失败测试，验证 Redis 暂时不可用时记录 `"final replay payload read failed"`，不会误写 PG，并保持 heartbeat 与并发配额按预期释放。
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/v1/_lib/proxy/replay/replay-spool.ts`:
- Around line 45-67: 为 withPayloadRebuildSlot 增加有界等待，避免
largePayloadRebuildWaiters 中的请求无限期排队；等待超过配置的超时时间后应放弃重建，并按现有 disable/abort 的
fail-open 路径继续处理。确保超时、操作失败或正常完成时都不会错误增加 activeLargePayloadRebuilds，也不会遗留等待队列项。
In `@src/app/v1/_lib/proxy/response-handler.ts`:
- Around line 4930-4931: 将 replay spool 终态监听器的清理从正常完成路径移至 finally 块，确保
activeResponsePump.completion 抛错或进入 catch 时也会执行
cleanupReplaySpoolTerminalListener()；同时保持与 cleanupClientAbortListener 一致，并移除原
try 块中的重复清理。
---
Nitpick comments:
In `@src/app/v1/_lib/proxy/replay/replay-spool.ts`:
- Line 132: Replace the per-chunk encoder allocation used to update pendingBytes
with Buffer.byteLength(text, "utf8"), and apply the same change to the trailing
byte accounting near the second measurement site. Confirm replay-spool.ts runs
only in the Node.js runtime before relying on Buffer, preserving UTF-8 byte
counts without creating Uint8Array copies.
In `@src/app/v1/_lib/proxy/replay/replay-store.ts`:
- Around line 84-97: Update LUA_READ_OWNED_CHUNKS to prepend the success marker
directly to the table returned by LRANGE using an in-place insertion, then
return that table. Remove the separate result table and chunk-copying loop while
preserving the existing ownership and length checks and the {1, ...chunks}
response structure.
In `@tests/unit/proxy/replay-spool.test.ts`:
- Around line 691-692: 在 replay-spool 测试中补充 `readOwnedChunks` 返回 `null`
的独立用例，覆盖实现中的 `chunks === null` 分支。参考现有 fenced payload 读取失败测试，验证 Redis 暂时不可用时记录
`"final replay payload read failed"`，不会误写 PG，并保持 heartbeat 与并发配额按预期释放。
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 391ad142-08b5-4f98-91a2-a5216f3f155a
📒 Files selected for processing (7)
src/app/v1/_lib/proxy/replay/replay-spool.tssrc/app/v1/_lib/proxy/replay/replay-store.tssrc/app/v1/_lib/proxy/response-handler.tstests/unit/proxy/replay-spool.test.tstests/unit/proxy/replay-store.test.tstests/unit/proxy/response-handler-client-abort-drain.test.tstests/unit/proxy/response-handler-stream-terminal.test.ts
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
This PR eliminates the replay/streaming memory amplification that caused the ~30 GiB OOM in #1408 by removing the in-process parts[] body copy (Redis LIST becomes the sole long-term body), bounding the write-behind backlog to 512 KiB with fail-open, limiting concurrent large-payload rebuilds to 2 slots, and tightening the detached drain window to 60 s once the owning spool reaches a terminal state. The concurrency reasoning (write-chain serialization, owner-fenced Lua with LIST-generation checks, slot-transfer accounting in the rebuild limiter, idempotent batch release) is internally consistent and every error path is logged or surfaced.
PR Size: L
- Lines changed: 874 (713 additions, 161 deletions)
- Files changed: 7 (3 source, 4 test)
Split considerations (required for size L): The change is cohesive around a single incident. The only cleanly separable unit is the detached-drain tightening in response-handler.ts (the onTerminal listener + armClientAbortDrainTimer refactor, ~40 lines), which is independent of the replay-spool memory work and could land separately. The remaining replay-spool/store changes (Redis-as-source-of-truth, write-behind limit, rebuild limiter, fenced read) are mutually dependent and do not split further without breaking invariants.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
No issues at or above the 80% confidence threshold.
Notes (below threshold, FYI only — not action items)
onTerminalinvokes the listener synchronously without thetry/catchguard thatnotifyTerminaluses, but the sole registered listener (Math.min+armClientAbortDrainTimer) cannot throw in practice.observe()now callsthis.encoder.encode(text).byteLengthper chunk to count retained UTF-8 bytes correctly across split multi-byte sequences; the transient allocation is immediately GC-eligible and does not undermine the PR's memory-bounding goal.
Verification
- Ran the 4 changed test files locally: 173 tests passed (83 replay-spool/store + 90 response-handler), matching the PR description.
- Confirmed
withPayloadRebuildSlotslot-transfer accounting holds under error, bypass, and wait-queue interleavings. - Confirmed
releaseQueuedBatchis idempotent (early clear incompleteAfterBilling+ finally no-op) and thatclearQueuedBatchescannot corrupt the in-flight write-chain callback's already-sent args. - Confirmed every terminal-state transition (
completeAfterBilling,abort,teardown) callsnotifyTerminalexactly once, and the drain re-arm clears the prior timer before setting a new one.
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Clean
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Adequate
- Code clarity - Good
Automated review by Claude AI
There was a problem hiding this comment.
Code Review Summary
No significant issues identified in this PR.
PR Size: L
- Lines changed: 874
- Files changed: 7
- Split suggestion: If this area changes again, consider separating replay storage/spool invariants from response-handler drain-window behavior to keep future review surfaces smaller.
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Clean
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Adequate
- Code clarity - Good
Automated review by Codex AI
There was a problem hiding this comment.
- Reviewed PR
#1413end-to-end across logic, security, error handling, types, docs, tests, and simplification, with validation against full file context for candidate concerns. - Applied the
size/Llabel and posted the required GitHub review summary. - No diff-line issues survived the false-positive and confidence filters, so I did not submit any inline review comments.
- I also tried to run the focused Vitest files, but this runner does not have
bun/bunxinstalled, so the final review is based on code/context inspection rather than local test execution.
Track pending write operations so the write-behind byte limit also applies while the bootstrap owning-meta write is in flight. Add a 30s timeout that removes stalled large-payload rebuild waiters without consuming a future permit. Move replay terminal listener cleanup into the stream processing finally block, replace per-chunk TextEncoder allocations with Buffer.byteLength, and simplify the fenced Redis chunk-read Lua result.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/proxy/response-handler-client-abort-drain.test.ts`:
- Around line 1253-1255: 在该测试中补充对 session.releaseAgent 的调用断言，确保异常处理路径确实执行了该
hook。使用现有的 session.releaseAgent mock，在触发生产代码后断言其已被调用。
- Line 1262: Update the async task cleanup at the Promise.allSettled call to
avoid discarding rejected background-task results: use Promise.all so any
unexpected rejection fails the test, or explicitly inspect and assert every
settled result, separately documenting any intentionally expected rejection.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: de758c3b-6b7e-499a-a1f4-cb48df850ad7
📒 Files selected for processing (5)
src/app/v1/_lib/proxy/replay/replay-spool.tssrc/app/v1/_lib/proxy/replay/replay-store.tssrc/app/v1/_lib/proxy/response-handler.tstests/unit/proxy/replay-spool.test.tstests/unit/proxy/response-handler-client-abort-drain.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/app/v1/_lib/proxy/response-handler.ts
- src/app/v1/_lib/proxy/replay/replay-store.ts
- src/app/v1/_lib/proxy/replay/replay-spool.ts
| vi.mocked(session.releaseAgent).mockImplementationOnce(() => { | ||
| throw new Error("release agent failed"); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
断言 session.releaseAgent 确实被调用。
当前测试只配置了 mockImplementationOnce，但没有验证该 hook 被执行。如果生产代码未调用 session.releaseAgent，异常路径不会执行，测试仍可能通过。请补充 expect(session.releaseAgent).toHaveBeenCalled()。
建议补充调用断言
await Promise.allSettled(asyncTasks.splice(0, asyncTasks.length));
+ expect(session.releaseAgent).toHaveBeenCalled();
expect(replayControl.state.unsubscribeCalls).toBe(1);Also applies to: 1264-1264
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/proxy/response-handler-client-abort-drain.test.ts` around lines
1253 - 1255, 在该测试中补充对 session.releaseAgent 的调用断言，确保异常处理路径确实执行了该 hook。使用现有的
session.releaseAgent mock，在触发生产代码后断言其已被调用。
| const processingTask = getRegisteredTask("stream-processing"); | ||
| expect(processingTask).toBeDefined(); | ||
| await expect(processingTask).resolves.toBeUndefined(); | ||
| await Promise.allSettled(asyncTasks.splice(0, asyncTasks.length)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
不要丢弃后台任务的 rejected 结果。
Promise.allSettled(...) 的结果未被检查。任一后台任务以 rejected 结束时，测试仍可能通过并隐藏回归。请改用 Promise.all(...)，或断言每个结果的状态；如果某个拒绝是预期行为，请单独断言该结果。
建议检查后台任务结果
- await Promise.allSettled(asyncTasks.splice(0, asyncTasks.length));
+ await Promise.all(asyncTasks.splice(0, asyncTasks.length));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await Promise.allSettled(asyncTasks.splice(0, asyncTasks.length)); | |
| await Promise.all(asyncTasks.splice(0, asyncTasks.length)); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/proxy/response-handler-client-abort-drain.test.ts` at line 1262,
Update the async task cleanup at the Promise.allSettled call to avoid discarding
rejected background-task results: use Promise.all so any unexpected rejection
fails the test, or explicitly inspect and assert every settled result,
separately documenting any intentionally expected rejection.
🧪 测试结果
总体结果: ✅ 所有测试通过 |
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
1 participant
背景
Replay owner 在客户端断线后可能同时保留完整的本地
parts[]payload, Redis write-behind batch 和长达 300 秒的 detached transport. 持续断线流会在最早请求回收前叠加并触发 Node heap OOM.修复
ReplaySpool.parts[], Redis LIST 成为流块的唯一长期正文副本.验证
parts[],pendingBytes和queuedBytes回到 0, 进程存活.bun run lint:fix,bun run lint,bun run typecheck,bun run build和git diff --check通过.运行时回归边界
调查报告列出的
/private/tmp/cch1408-*容器压测夹具不在当前 Linux 主机, 因此本 PR 不声明新的正式 heap/RSS 对比数据. 合并前建议在保留原 PostgreSQL, Redis, mock upstream 和 1 GiB app 容器夹具的隔离环境, 对最终镜像重跑同一 40+ 请求波次.关联
parts[]; 本 PR 进一步移除本地副本, 以 Redis LIST 为唯一长期正文, 并约束 write-behind backlog (512 KiB) 与大 payload 并发重建 (2 槽).Greptile Summary
The PR bounds memory retained by detached replay streams while preserving fenced replay completion.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Sequence Diagram
Reviews (2): Last reviewed commit: "fix(replay): bound bootstrap backlog and..." | Re-trigger Greptile
Context used: