fix(proxy): bound Replay disconnect memory retention (#1408) - #1414
Merged
Conversation
AptS-1547
added
bug
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough本次变更限制 ReplaySpool 的本地内存和 Redis 写入积压。失效 Replay 的客户端断线排水窗口缩短为 60 秒。会话响应体新增默认 5 MiB 的可配置 Redis 存储上限。新增 Issue ChangesReplay 与会话响应体内存控制
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/v1/_lib/proxy/replay/replay-spool.ts (1)
98-101: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win按所有输入字节计算
pendingBytes。当 UTF-8 字符跨多个 chunk 时，
decoder.decode()可以返回空字符串。当前代码会在 Line 99 提前返回，因此不会计入这些 chunk 的字节数。后续 chunk 解码出文本时，queuedWriteBytes只包含最后一个片段的大小。这会低估 Redis 写入积压，并允许实际写入数据超过 1 MiB 上限。请在调用
decoder.decode()前累计chunk.byteLength。请增加一个跨 chunk 的多字节 UTF-8 测试。建议修改
if (this.totalBytes > env.REPLAY_MAX_PAYLOAD_BYTES) { this.disable("payload_too_large"); return; } + this.pendingBytes += chunk.byteLength; const text = this.decoder.decode(chunk, { stream: true }); if (text.length === 0) return; this.pending.push(text); - this.pendingBytes += chunk.byteLength;🤖 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` around lines 98 - 101, 在负责处理 chunk 的方法中，先将每个输入 chunk 的 chunk.byteLength 累加到 pendingBytes，再调用 decoder.decode()，移除空文本时提前返回导致字节未计数的问题；仅在解码产生文本时追加到 pending。新增跨多个 chunk 的多字节 UTF-8 测试，验证 queuedWriteBytes/pendingBytes 按所有输入字节累计并正确遵守 1 MiB 上限。
🤖 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 `@docs/troubleshooting/issue-1408-replay-oom.md`:
- Around line 283-285: 在 issue-1408 调查结论段落中修改行首的 “#1408”，避免其被 Markdown
解析为标题；优先将该文本接回上一行，或对井号进行转义，同时保持原有结论内容不变。
In `@src/app/v1/_lib/proxy/replay/replay-spool.ts`:
- Around line 262-266: 在 completeAfterBilling 中不要直接增加 queuedWriteBytes；复用
enqueueFlush 的 MAX_QUEUED_WRITE_BYTES 预留检查。预留失败时清空尾批、禁用
spool，并等待现有清理链完成；同时增加覆盖“阻塞写入加尾批”超过上限的测试。
In `@tests/load/issue-1408-replay-oom/drive-disconnect-waves.cjs`:
- Around line 86-103: Update the request promise in waitForMock to also reject
when the response emits "aborted" or "error" after the response has started.
Attach these handlers to the response alongside the existing "data" and "end"
listeners, preserving the current status and JSON parsing behavior for normally
completed responses.
In `@tests/load/issue-1408-replay-oom/start-mock-container.sh`:
- Around line 32-44: 在 start-mock-container.sh 的 readiness 重试结束后、现有 docker logs
输出之后，调用 docker rm -f "$container" 清理新建容器，再保持失败退出行为不变。
---
Outside diff comments:
In `@src/app/v1/_lib/proxy/replay/replay-spool.ts`:
- Around line 98-101: 在负责处理 chunk 的方法中，先将每个输入 chunk 的 chunk.byteLength 累加到
pendingBytes，再调用 decoder.decode()，移除空文本时提前返回导致字节未计数的问题；仅在解码产生文本时追加到
pending。新增跨多个 chunk 的多字节 UTF-8 测试，验证 queuedWriteBytes/pendingBytes
按所有输入字节累计并正确遵守 1 MiB 上限。
🪄 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: 012e4a98-5520-414c-919c-f483d4c6028b
📒 Files selected for processing (20)
.env.exampleCHANGELOG.mddocs/troubleshooting/issue-1408-replay-oom.mdsrc/app/v1/_lib/proxy/replay/replay-spool.tssrc/app/v1/_lib/proxy/response-handler.tssrc/lib/config/env.schema.tssrc/lib/session-manager-detail-snapshots.test.tssrc/lib/session-manager.tstests/load/issue-1408-replay-oom/README.mdtests/load/issue-1408-replay-oom/drive-disconnect-waves.cjstests/load/issue-1408-replay-oom/memory-probe.cjstests/load/issue-1408-replay-oom/mock-upstream.cjstests/load/issue-1408-replay-oom/run-wave.shtests/load/issue-1408-replay-oom/sample-container.shtests/load/issue-1408-replay-oom/start-mock-container.shtests/unit/lib/env-store-session-response-body.test.tstests/unit/lib/session-manager-redaction.test.tstests/unit/proxy/issue-1408-load-fixture.test.tstests/unit/proxy/replay-spool.test.tstests/unit/proxy/response-handler-stream-terminal.test.ts
Contributor
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Summary
This PR correctly eliminates the Replay disconnect memory-retention chain described in #1408: it removes the in-process parts[] full-stream retention, bounds the Redis write-behind backlog at 1 MiB, downgrades the detached drain window to 60 s when the spool becomes inactive, and caps persisted session response bodies. The core proxy logic, the drain-window refactor, and the session-body bounding are all sound, and the change is backed by 104+ focused tests covering boundary, race, and failure conditions.
PR Size: XL
- Lines changed: 2009 (1884 additions, 125 deletions)
- Files changed: 20
Optional split (non-blocking) — the PR bundles three logical groups that could be reviewed/landed independently if desired:
- Core fix + unit tests (~500 lines):
replay-spool.ts,response-handler.ts,env.schema.ts,session-manager.ts+ their unit tests. This is the essential behavioral fix. - Load fixture (~700 lines):
tests/load/issue-1408-replay-oom/*+ contract test. Reproducible reproduction tooling. - Documentation (~400 lines):
docs/troubleshooting/issue-1408-replay-oom.md+CHANGELOG.md. Root-cause evidence report.
Keeping them together is defensible since the fixture and doc provide the verification evidence for the fix.
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 met the reporting confidence threshold (>= 80).
Notable Validation Performed
completeAfterBillingchunk-count invariant:readChunks(0)is called afterthis.chunkCountis updated from the terminalwriteOwned; fenced ownership guarantees no concurrent writer can mutate the LIST between the terminal write and the read-back, so thechunks.length !== this.chunkCountguard is a true consistency check, not a race.serializeDurablePersistencechain safety: the module-level singleton swallows rejection in the chain-advancement handler (.then(id, id)) while propagating it to the caller, so a failing persist cannot poison the chain for subsequent spools. The serialization is intentional (bounds heap peak to one reconstructed payload) per the PR description.capInactiveReplayDrainWindowcorrectness: theclientAbortDrainTimeoutMs <= CLIENT_ABORT_DRAIN_MAX_MSguard makes it a no-op in non-Replay mode and whenREPLAY_MAX_DETACHED_MSis configured below 60 s (never increases the window).Math.max(0, ...)inscheduleClientAbortDrainTimeouthandles the elapsed-exceeds-cap case. All three timing orderings (inactive-after-detach, active-spool-keeps-300 s, inactive-before-detach) are covered by tests.- Session-body double check:
storeSessionResponsechecks both the raw input and the post-redactionresponseString, which is necessary because[REDACTED]substitution can grow a small body past the limit. At most one warn log is emitted per call. The snapshot path correctly handles null/string/object bodies and deletes stale keys on oversize. notifyInactiveidempotency: theinactiveNotifiedguard plus therelease()idempotency guard correctly handle the disable-then-abort and abort-then-disable orderings without double-firing the callback or double-decrementingactiveSpoolCount.- CLAUDE.md compliance: no emoji in code/comments/strings; no new user-facing dashboard strings requiring i18n (all changes are backend proxy/server-log/internal-doc); extensive test coverage added.
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Clean
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Strong (104+ focused tests)
- Code clarity - Good (helper extraction improves readability)
Automated review by Claude AI
AptS-1547
force-pushed
the
fix/issue-1408-replay-oom
branch
from
August 11, 2026 10:23
05291ac to
b100aff
Compare
Collaborator
Author
|
CodeRabbit 第一轮 review 已处理（b100aff7）：
|
Collaborator
Author
|
@coderabbitai review |
|
Contributor
There was a problem hiding this comment.
Code Review Summary
The PR closes the main Replay retention path, but one remaining gap in ReplaySpool.completeAfterBilling() still lets a detached stream exceed the new write-backlog ceiling on the terminal path. After validating the modified production code and the new regression tests in full-file context, I did not find other high-confidence issues on changed lines.
PR Size: XL
- Lines changed: 2009
- Files changed: 20
- Split suggestions: land the core
ReplaySpool/ProxyResponseHandlermemory-retention fix separately from the load-fixture and troubleshooting-doc additions so the runtime behavior can be reviewed in isolation. - Split suggestions: keep the
SESSION_RESPONSE_BODY_MAX_BYTESsession-debug-artifact limit as a separate PR when possible, since it changes a different subsystem and test surface from the Replay drain fix.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 1 | 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 |
Critical Issues (Must Fix)
- None.
High Priority Issues (Should Fix)
src/app/v1/_lib/proxy/replay/replay-spool.ts:266—completeAfterBilling()appends the terminalpendingBytesbatch without re-checkingMAX_QUEUED_WRITE_BYTES, so a stalled Redis write can still leave a detached spool above the new 1 MiB backlog ceiling until the blocked write drains.
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
Contributor
There was a problem hiding this comment.
Review Posted
- Reviewed PR
#1414, applied thesize/XLlabel, and submitted the summary review on GitHub. - Posted 1 inline
HIGHfinding onsrc/app/v1/_lib/proxy/replay/replay-spool.ts:266. - Main issue:
completeAfterBilling()appends the terminal tail batch without re-checkingMAX_QUEUED_WRITE_BYTES, so a stalled Redis write can still push a detached spool above the new 1 MiB backlog ceiling. - I did not find other high-confidence issues on modified lines after checking the surrounding code and the new tests in context.
- I could not execute the added tests in this runner because
bunis not installed andnode_modulesis absent.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/unit/proxy/replay-spool.test.ts (1)
99-99: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value建议重命名
resetOwnedChunkCount。该函数现在清空按 replay ID 保存的 chunks Map，不再重置计数。名称与行为不一致。改为
resetOwnedChunks更准确。♻️ 建议的改名
- resetOwnedChunkCount: () => { + resetOwnedChunks: () => { ownedChunksByReplayId.clear(); },调用处同步更新。
🤖 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` at line 99, Rename the function resetOwnedChunkCount to resetOwnedChunks to reflect that it clears the owned chunks map, and update every call site and reference accordingly.
🤖 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/load/issue-1408-replay-oom/start-mock-container.sh`:
- Around line 17-20: Update the existence check in start-mock-container.sh to
use docker container inspect instead of docker inspect, so only containers with
the specified name prevent startup; preserve the existing error message and exit
behavior.
- Around line 30-44: Add a shared container-cleanup function immediately after
the successful docker run, register POSIX traps for SIGINT, SIGTERM, and EXIT,
and have it remove the tracked container safely. Clear the traps before the
readiness-success exit so the ready container is preserved, and reuse the same
cleanup function in the timeout path. Add CI coverage for signal interruption
and early command failure cleanup.
In `@tests/unit/proxy/issue-1408-load-fixture.test.ts`:
- Around line 75-81: 隔离 fixture 的环境变量，避免继承父进程中的配置。更新
tests/unit/proxy/issue-1408-load-fixture.test.ts:75-81 的 startMock，在设置默认值和
overrides 前删除所有 fixture 配置键；同时在
tests/unit/proxy/issue-1408-load-fixture.test.ts:252-257 复用同一份已清理的 mock
环境，确保配置验证不受外部环境影响。
In `@tests/unit/proxy/replay-spool.test.ts`:
- Around line 676-723: 在测试套件的 beforeEach 中重置 storeControl.store.persistCompleted
的 mock，并重新设置其默认实现，确保不会保留并发测试中针对 identity.replayId 返回未完成 Promise 的实现；保留
envControl.maxPayloadBytes 现有的恢复逻辑。
---
Nitpick comments:
In `@tests/unit/proxy/replay-spool.test.ts`:
- Line 99: Rename the function resetOwnedChunkCount to resetOwnedChunks to
reflect that it clears the owned chunks map, and update every call site and
reference accordingly.
🪄 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: 108a6680-6bc1-49bc-8863-9e7de873dbd1
📒 Files selected for processing (6)
docs/troubleshooting/issue-1408-replay-oom.mdsrc/app/v1/_lib/proxy/replay/replay-spool.tstests/load/issue-1408-replay-oom/drive-disconnect-waves.cjstests/load/issue-1408-replay-oom/start-mock-container.shtests/unit/proxy/issue-1408-load-fixture.test.tstests/unit/proxy/replay-spool.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/load/issue-1408-replay-oom/drive-disconnect-waves.cjs
- src/app/v1/_lib/proxy/replay/replay-spool.ts
- docs/troubleshooting/issue-1408-replay-oom.md
Contributor
🧪 测试结果
总体结果: ✅ 所有测试通过 |
Collaborator
Author
|
CodeRabbit 第二轮 review 已处理（9f5890df，普通提交，未改写远端历史）：
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/load/issue-1408-replay-oom/start-mock-container.sh`:
- Line 42: 为健康检查中的 curl 调用添加 --connect-timeout 2 和 --max-time
5，确保连接或响应无期限阻塞；保留现有重试逻辑，并使用不返回响应的端点验证重试次数及 cleanup_container 清理行为。
🪄 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: 158ace4d-f899-4293-8e46-61ed905aa29e
📒 Files selected for processing (3)
tests/load/issue-1408-replay-oom/start-mock-container.shtests/unit/proxy/issue-1408-load-fixture.test.tstests/unit/proxy/replay-spool.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/proxy/replay-spool.test.ts
Contributor
🧪 测试结果
总体结果: ✅ 所有测试通过 |
Collaborator
Author
|
CodeRabbit 第三轮（最终 remediation round）已处理（5825a5b4，普通提交，未改写远端历史）：
已达到约定的最多三轮真实反馈/修复上限，本 PR 不再触发额外 CodeRabbit review round。 |
Contributor
🧪 测试结果
总体结果: ✅ 所有测试通过 |
6 tasks
Collaborator
Author
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
Closed
Contributor
🧪 测试结果
总体结果: ✅ 所有测试通过 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Root Cause
On v0.9.2, a Replay owner retained the complete upstream stream in
ReplaySpool.partswhile also queuing Redis writes. After the downstream client disconnected, an active spool selected the 300-second detached drain window. Slow or blocked persistence therefore kept large external buffers, upstream sockets, and async tasks alive for substantially longer than the ordinary 60-second drain.The fix removes the redundant process-local body, bounds queued writes, releases queued batches on abort/disable, and makes the response handler observe when Replay becomes inactive so it can restore the shorter drain deadline.
Related Work
devstill retained the full in-processReplaySpool.parts[]and did not downgrade the detached drain window when the spool became inactive. This PR closes that remaining trigger chain.SESSION_RESPONSE_BODY_MAX_BYTESto bound the legacy + before + after Redis body amplification. Physical deduplication of those three values is tracked in perf(session): 去重 Redis 中重复存储的 response body #1415. Reviewers should compare the two approaches.Review Follow-up
CCH_MOCK_*values and reset stateful Replay store mocks between tests.Verification
bunx vitest run tests/unit/proxy/issue-1408-load-fixture.test.ts tests/unit/proxy/replay-spool.test.ts tests/unit/proxy/response-handler-stream-terminal.test.ts --configLoader bundle: 90 tests passed./v1/responsessmoke: mock receipt confirmed, client disconnected after 252 ms, the stream hit the 60-second drain timeout, and both stream tasks cleaned up toremainingTasks: 0.bun run build: passed.bun run lint: passed.bun run lint:fix: passed with no additional changes.bun run typecheck: passed.git diff --check: passed.bun run test: completed with one unrelated existing failure atsrc/components/ui/__tests__/language-switcher.test.tsx:153; the component, test, package manifest, and lockfile have no diff in this branch.Operational Notes
CCH_API_KEYorCCH_API_KEY_FILE.CHANGELOG.mddocument the final behavior and evidence boundary.Closes #1408
Description enhanced by Claude AI
Greptile Summary
The PR bounds memory and Redis retention for disconnected Replay streams while preserving replay persistence and streaming finalization.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Sequence Diagram
sequenceDiagram participant Client participant Handler as ResponseHandler participant Spool as ReplaySpool participant Redis participant PG Client->>Handler: Start streaming request Handler->>Spool: Create owner spool with onInactive Spool->>Redis: Fenced write-behind chunks Client--xHandler: Disconnect Handler->>Handler: Start bounded detached drain alt Replay remains active Handler->>Handler: Retain Replay drain window Spool->>Redis: Flush terminal batch Spool->>Redis: Read ordered chunks Spool->>PG: Persist durable replay else Replay becomes inactive Spool-->>Handler: onInactive() Handler->>Handler: Cap deadline at 60s from disconnect Handler--xHandler: Abort upstream at deadline endReviews (5): Last reviewed commit: "fix(session): default stored responses t..." | Re-trigger Greptile
Context used: