fix(proxy): prevent shared mutation and replay memory retention - #1405
Conversation
Rectifiers and cache TTL overrides mutated shared nested arrays in place, leaking attempt-specific edits across concurrent shadow sessions and the original request. Switch to copy-on-write: filtered arrays are assigned through the top-level message object, TTL override rebuilds only changed message entries, and shadow sessions shallow-copy the request message while sharing the readonly buffer instead of deep-cloning multi-MB request bodies per shadow. ReplaySpool retained full payload while Redis or PG writes blocked and abort/disable raced with in-flight flushes. Payload is now snapshotted and cleared before persistence; abort immediately releases accumulated parts and queued batches, deduplicates concurrent calls through a shared barrier, and fences store cleanup through the writeChain so concurrency quota is freed only after cleanup completes. Streaming detection now reads the stream flag directly from the outgoing message instead of re-parsing the serialized body.
📝 WalkthroughWalkthrough本次变更为代理消息处理增加写时复制,并隔离 streaming shadow session 的顶层状态。ReplaySpool 跟踪异步批次并串行完成 abort 清理。JSON 请求解析器改用 Zod 输出类型推导。 Changes代理状态与生命周期
JSON 请求类型推导
Estimated code review effort: 4 (Complex) | ~45 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/unit/proxy/replay-spool.test.ts (1)
577-582: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win避免用 4 MiB 字符串字面量做断言。
"x".repeat(payloadBytes)会额外构造一个 4 MiB 字符串。断言失败时,Vitest 会为这两个 4 MiB 字符串生成 diff,输出体积巨大,报告可能被撑爆或明显变慢。改为断言长度和内容特征即可,覆盖度不变。
♻️ 建议改动
expect(storeControl.store.persistCompleted).toHaveBeenCalledTimes(1); - expect(storeControl.store.persistCompleted).toHaveBeenCalledWith( - expect.objectContaining({ - payload: "x".repeat(payloadBytes), - byteSize: payloadBytes, - }) - ); + const persistArg = storeControl.store.persistCompleted.mock.calls[0][0] as { + payload: string; + byteSize: number; + }; + expect(persistArg.payload).toHaveLength(payloadBytes); + expect(persistArg.payload.startsWith("xxx")).toBe(true); + expect(persistArg.payload.endsWith("xxx")).toBe(true); + expect(persistArg.byteSize).toBe(payloadBytes);🤖 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 577 - 582, Update the persistCompleted assertion in the replay spool test to avoid constructing or diffing the full 4 MiB payload string. Assert the payload’s length and representative content characteristics instead, while retaining the existing byteSize assertion and equivalent coverage.src/app/v1/_lib/proxy/replay/replay-spool.ts (1)
386-386: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value参数名
deleteEntry与实际行为不一致,字段声明位置建议上移。两点可读性问题:
teardown的参数名为deleteEntry,但为 true 时调用的是store.abortOwned,不是store.deleteEntry。名称会误导读者。建议改为fenceEntry或abortEntry。released、aborting、abortPromise三个实例字段声明在teardown与release之间。其余字段都集中在类顶部。建议移到第 54 行附近,与metaWritten放在一起。♻️ 建议改动
- private teardown(reason: string, deleteEntry: boolean): void { + private teardown(reason: string, fenceEntry: boolean): void {- if (deleteEntry) { + if (fenceEntry) {- - private released = false; - private aborting = false; - private abortPromise: Promise<void> | null = null; -在类顶部字段区补充:
private writeChain: Promise<void> = Promise.resolve(); private metaWritten = false; + private released = false; + private aborting = false; + private abortPromise: Promise<void> | null = null;Also applies to: 410-412
🤖 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 386, 将 teardown 及其调用链中的 deleteEntry 参数重命名为能反映 store.abortOwned 行为的 fenceEntry 或 abortEntry,并同步更新所有引用;同时将 released、aborting、abortPromise 三个实例字段从 teardown 与 release 之间移到类顶部字段声明区,靠近 metaWritten。
🤖 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/forwarder.ts`:
- Around line 1175-1179: Remove the full structuredClone before
descriptor.rectify in the request-forwarding flow. Pass a shallow top-level copy
to the rectifier, make descriptor.rectify copy only branches it mutates, and
assign the rectified message back to requestSession.request.message only when
rectified.applied is true; add a large-request test verifying unchanged branches
are not fully copied.
In `@tests/unit/proxy/replay-spool.test.ts`:
- Around line 686-696: Update the abort test around spool.abort to snapshot the
queued batch references before aborting, then assert those batches are emptied
after abort and separately assert queuedBatches.size is zero. Replace the
vacuous every check over the post-abort set while preserving the existing batch
and abort behavior.
- Around line 609-610: Update the timer advancement in the test around
renewOwnerLease to exceed OWNER_HEARTBEAT_INTERVAL_MS, using 15_001 or the
shared constant, so the assertion verifies no heartbeat occurs after cleanup
rather than at the interval boundary.
---
Nitpick comments:
In `@src/app/v1/_lib/proxy/replay/replay-spool.ts`:
- Line 386: 将 teardown 及其调用链中的 deleteEntry 参数重命名为能反映 store.abortOwned 行为的
fenceEntry 或 abortEntry,并同步更新所有引用;同时将 released、aborting、abortPromise 三个实例字段从
teardown 与 release 之间移到类顶部字段声明区,靠近 metaWritten。
In `@tests/unit/proxy/replay-spool.test.ts`:
- Around line 577-582: Update the persistCompleted assertion in the replay spool
test to avoid constructing or diffing the full 4 MiB payload string. Assert the
payload’s length and representative content characteristics instead, while
retaining the existing byteSize assertion and equivalent coverage.
🪄 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: f71c339c-fd45-4fa0-a528-38f53d123ae7
📒 Files selected for processing (7)
src/app/v1/_lib/proxy/billing-header-rectifier.tssrc/app/v1/_lib/proxy/forwarder.tssrc/app/v1/_lib/proxy/replay/replay-spool.tstests/unit/proxy/billing-header-rectifier.test.tstests/unit/proxy/cache-ttl-override.test.tstests/unit/proxy/proxy-forwarder-hedge-first-byte.test.tstests/unit/proxy/replay-spool.test.ts
| const mutableMessage = structuredClone( | ||
| requestSession.request.message as Record<string, unknown> | ||
| ); | ||
| requestSession.request.message = mutableMessage; | ||
| const rectified = descriptor.rectify(mutableMessage); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
移除整流路径中的完整深拷贝。
Line [1175] 在调用 descriptor.rectify 前对整个 requestSession.request.message 执行 structuredClone。这会复制所有 messages、内嵌媒体和未修改分支。若整流器返回 applied: false,该副本还会立即丢弃。
该实现仍保留请求整流的内存放大路径,与本 PR 的 copy-on-write 目标冲突。请让整流器按需复制:先复制顶层对象,只复制实际写入的分支,并在 rectified.applied 为 true 后写回 session。请增加大请求测试,确认未修改分支不会被完整复制。
🤖 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/forwarder.ts` around lines 1175 - 1179, Remove the full
structuredClone before descriptor.rectify in the request-forwarding flow. Pass a
shallow top-level copy to the rectifier, make descriptor.rectify copy only
branches it mutates, and assign the rectified message back to
requestSession.request.message only when rectified.applied is true; add a
large-request test verifying unchanged branches are not fully copied.
| await vi.advanceTimersByTimeAsync(15_000); | ||
| expect(storeControl.store.renewOwnerLease).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 查找 OWNER_HEARTBEAT_INTERVAL_MS 的定义值
rg -n -C 2 'OWNER_HEARTBEAT_INTERVAL_MS\s*=' --glob '**/*.ts'Repository: ding113/claude-code-hub
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -u
echo "Relevant files containing heartbeat interval terms:"
rg -n -i -C 2 'heartbeat|owner.*lease|renewOwnerLease|OWNER_HEARTBEAT|interval' --glob '**/*.{ts,tsx,js,jsx,json,yml,yaml,md}' || true
echo
echo "Test file excerpt:"
if [ -f tests/unit/proxy/replay-spool.test.ts ]; then
sed -n '570,625p' tests/unit/proxy/replay-spool.test.ts | cat -n
else
echo "tests/unit/proxy/replay-spool.test.ts not found"
fi
echo
echo "Definitions in candidate proxy files:"
fd -a 'replay-spool|spool|proxy' tests src | sed 's#^\./##' | head -80Repository: ding113/claude-code-hub
Length of output: 213
🏁 Script executed:
#!/bin/bash
set -u
echo "Git tracked TypeScript files count:"
git ls-files '*.ts' '*.tsx' '*.js' '*.jsx' | wc -l
echo
echo "Search for heartbeat/lease/renew in git-tracked text files:"
git ls-files | xargs rg -n -C 2 -i 'heartbeat|renewOwnerLease|owner.*lease|renew.*lease|interval' || true
echo
echo "Search for 15_000 / 15000:"
git ls-files | xargs rg -n -C 2 '15_?[0-9]+' || true
echo
echo "Search for replay-spool test locations:"
git ls-files | rg 'replay-spool|spool|proxy' || trueRepository: ding113/claude-code-hub
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -u
echo "Current status of replay-spool test file:"
git ls-files tests/unit/proxy/replay-spool.test.ts
wc -l tests/unit/proxy/replay-spool.test.ts 2>/dev/null || true
echo
echo "Precise exact matches in tracked source/config docs (non-generated JSON snapshots):"
rg -n -C 2 -I 'OWNER_HEARTBEAT_INTERVAL_MS|renewOwnerLease|heartbeat|owner.*lease' tests src docs --glob '!drizzle/meta/*.json' --glob '!node_modules/**' || true
echo
echo "Precise exact matches in exact-relevant terms:"
rg -n -C 2 'renewOwnerLease|OwnerLease|owner lease|OWNER_HEARTBEAT|HEARTBEAT_INTERVAL' --glob '!drizzle/meta/*.json' --glob '!node_modules/**' || trueRepository: ding113/claude-code-hub
Length of output: 13956
🏁 Script executed:
#!/bin/bash
set -u
echo "ReplayStore source constants and scheduling/release excerpts:"
ast-grep outline src --view compact --match 'class ReplayStore' 2>/dev/null | head -80 || true
ast-grep outline src --view compact --match 'function ReplayReplayStore' 2>/dev/null | head -80 || true
echo
sed -n '1,220p' src --glob '*replay*.ts' 2>/dev/null | rg -n -C 3 'OWNER_HEARTBEAT_INTERVAL_MS|ownerHeartbeatTimer|startOwnerHeartbeat|stopOwnerHeartbeat|releaseOwner|clearTimeout|setInterval|setInterval' || true
echo
# List source files matching replay-store-like names if the above path did not show enough.
fd -a '.*replay.*store.*|.*spool.*|.*proxy.*' src | sed 's#^\./##' | head -100
echo
echo "Relevant constants definitions across tracked source-like TypeScript files only:"
git ls-files '*.ts' '*.tsx' | rg '(^|/)(src|tests)/' | xargs rg -n -C 2 'OWNER_HEARTBYEAT_INTERVAL_MS|ownerHeartbeat|clearTimeout\(.*ownerHeartbeat|setInterval\(\).*OWNER_HEARTBEAT_INTERVAL_MS|15_000|15000' || trueRepository: ding113/claude-code-hub
Length of output: 20937
将断言时延改为超过 OWNER_HEARTBEAT_INTERVAL_MS。
OWNER_HEARTBEAT_INTERVAL_MS 为 15_000,本用例推进 15s 尚不够下一个心跳周期;即使定时器未被清除,renewOwnerLease 仍不会刚好调用,所以该断言不能保护心跳已清理。改为 15_001 或引用常量。
🤖 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 609 - 610, Update the
timer advancement in the test around renewOwnerLease to exceed
OWNER_HEARTBEAT_INTERVAL_MS, using 15_001 or the shared constant, so the
assertion verifies no heartbeat occurs after cleanup rather than at the interval
boundary.
| const queuedBatches = (spool as unknown as { queuedBatches: Set<string[]> }).queuedBatches; | ||
| expect(queuedBatches.size).toBe(2); | ||
|
|
||
| let abortSettled = false; | ||
| const abortPromise = spool.abort("client_disconnect").then(() => { | ||
| abortSettled = true; | ||
| }); | ||
| await vi.advanceTimersByTimeAsync(0); | ||
|
|
||
| expect(batch).toEqual([]); | ||
| expect([...queuedBatches].every((queuedBatch) => queuedBatch.length === 0)).toBe(true); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
第 696 行的断言恒为真,没有验证能力。
abort() 内部的 clearQueuedBatches() 会先清空每个批次,再执行 this.queuedBatches.clear()。执行到第 696 行时,queuedBatches 已经是空集合,[...queuedBatches] 得到空数组,Array.prototype.every 对空数组返回 true。因此该断言在实现回退时也不会失败。
请在 abort 之前先取出批次引用,再对这些引用断言内容已释放,并单独断言集合已清空。
💚 建议改动
const queuedBatches = (spool as unknown as { queuedBatches: Set<string[]> }).queuedBatches;
expect(queuedBatches.size).toBe(2);
+ const trackedBatches = [...queuedBatches];
let abortSettled = false;
const abortPromise = spool.abort("client_disconnect").then(() => {
abortSettled = true;
});
await vi.advanceTimersByTimeAsync(0);
expect(batch).toEqual([]);
- expect([...queuedBatches].every((queuedBatch) => queuedBatch.length === 0)).toBe(true);
+ expect(trackedBatches.every((queuedBatch) => queuedBatch.length === 0)).toBe(true);
+ expect(queuedBatches.size).toBe(0);📝 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.
| const queuedBatches = (spool as unknown as { queuedBatches: Set<string[]> }).queuedBatches; | |
| expect(queuedBatches.size).toBe(2); | |
| let abortSettled = false; | |
| const abortPromise = spool.abort("client_disconnect").then(() => { | |
| abortSettled = true; | |
| }); | |
| await vi.advanceTimersByTimeAsync(0); | |
| expect(batch).toEqual([]); | |
| expect([...queuedBatches].every((queuedBatch) => queuedBatch.length === 0)).toBe(true); | |
| const queuedBatches = (spool as unknown as { queuedBatches: Set<string[]> }).queuedBatches; | |
| expect(queuedBatches.size).toBe(2); | |
| const trackedBatches = [...queuedBatches]; | |
| let abortSettled = false; | |
| const abortPromise = spool.abort("client_disconnect").then(() => { | |
| abortSettled = true; | |
| }); | |
| await vi.advanceTimersByTimeAsync(0); | |
| expect(batch).toEqual([]); | |
| expect(trackedBatches.every((queuedBatch) => queuedBatch.length === 0)).toBe(true); | |
| expect(queuedBatches.size).toBe(0); |
🤖 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 686 - 696, Update the
abort test around spool.abort to snapshot the queued batch references before
aborting, then assert those batches are emptied after abort and separately
assert queuedBatches.size is zero. Replace the vacuous every check over the
post-abort set while preserving the existing batch and abort behavior.
tsgo (TypeScript native preview) could not infer the generic output type from JsonBodySchema<T>'s structural safeParse signature because zod 4 uses this-type polymorphism (core.output<this>) instead of a plain generic. This left body.data as unknown across all v1 API handlers, producing 36 TS18046/TS2698/TS2345 errors. Switch parseHonoJsonBody/parseJsonBody/parseJson to infer the schema type directly (<S extends z.ZodType>) and extract the output via z.output<S>, which tsgo resolves correctly. Add explicit parameter annotations to four zod .transform()/.refine() callbacks where tsgo also failed to infer the this-dependent input type. CI Run: https://github.com/ding113/claude-code-hub/actions/runs/31245382874 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review Summary
I reviewed the three implementation files and four unit-test updates with emphasis on the new copy-on-write boundaries in the proxy forwarder and the replay spool cleanup/ownership changes.
No significant issues identified in this PR.
PR Size: M
- Lines changed: 400
- Files changed: 7
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.
Review Done
- Reviewed PR
#1405across all7changed files, including full-file context and related call sites around the forwarder shadow-session changes andReplaySpoolcleanup flow. - No diff-line issues cleared the reporting threshold after validation, so I did not post inline comments.
- Applied the
size/Mlabel and submitted the required no-findings review summary on the PR:https://github.com/ding113/claude-code-hub/pull/1405
Summary
This fixes two memory-amplification paths in the proxy runtime:
ArrayBufferpayloads.ReplaySpoolretained accumulated and queued payload batches while Redis or PostgreSQL writes were blocked, and abort/disable races could release the concurrency quota before fenced store cleanup completed.The patch switches shadow attempts and request rectifiers to copy-on-write, shares readonly request buffers, and tightens
ReplaySpoolpayload ownership, cleanup ordering, and abort barriers.Changes
ArrayBufferdata across Discovery/Hedge shadow sessions instead of copying multi-MiB buffers per attempt.streamflag directly instead of serializing and parsing the request again.abort()calls through one shared cleanup barrier.disabled || abortingafter the asynchronous Replay bootstrap write. This closes the reviewed race whereabort()could resolve before the realabortOwnedcleanup.Review Closure
Independent review found a real bootstrap/abort race after the initial implementation:
ReplaySpool.bootstrap()checked abort state beforeawait store.writeOwned(), but not after it. A concurrent bootstrap failure could queue teardown behindabortPromise, letabort()skip store cleanup, and release quota before the actual fenced cleanup completed.The final patch adds the post-await guard and a deterministic regression test for bootstrap-pending -> abort -> bootstrap null -> fenced cleanup. Two independent post-fix reviews returned
No findings.Validation
4 files / 141 tests passed.860 files / 8444 tests passed;2 files / 13 tests skipped.bun run build: passed. The existing 17 Edge Runtime warnings remain non-fatal.bun run lint: passed.bun run lint:fix: passed; 2054 files checked, no fixes applied.bun run typecheck: passed.git diff --check origin/dev...HEAD: passed.Current CI Baseline Drift
The PR is currently red in clean-install typecheck/build jobs for a repository-wide dependency resolution change outside this 7-file diff:
devrun, https://github.com/ding113/claude-code-hub/actions/runs/31227727835, resolved@hono/zod-openapi@1.5.1.^1.5.1, the current branch and PR workflows resolve@hono/zod-openapi@1.5.2.tsgoreportsunknown, spread-type, and implicit-anyerrors across existing management API handlers and schemas that are not changed by this PR.This PR does not pin or otherwise change the repository-wide dependency policy because the delivery scope is intentionally restricted to the memory fix and its tests. The clean-install drift needs a separate dependency compatibility decision.
Acceptance
5/5criteria, all required text evidence uploaded.Both rounds intentionally retain an overall
partialconclusion because the runtime matrix used relaxed host admission.Diagnostic Runtime Matrix
All 9 samples are explicitly
relaxed-admission diagnostic-only. Every sample hadcompleted == successful > 0, with zero dropped, offered, response, transport, protocol, drain, cancellation, telemetry-invalid, or OOM outcomes.Directional allocation observations:
127.28 -> 111.97 MiB; arrayBuffers123.31 -> 108.12 MiB.270.17 -> 213.23 MiB; arrayBuffers266.20 -> 209.39 MiB.Claude baseline Redis retained growth normalized by completed requests is approximately
1.06 MiB/requestacross the three images. Static attribution points to mode-off Session debug/observability persistence of multiple large request, tools-schema, and response snapshots. WithReplay=false, Replay identity returns early, so this structural Redis retention is not aReplaySpoolleak.Benchmark Limitations
verdict=passmeans internal assertions passed; it is not formal host admission.cooldown.classification=formalonly means the cooldown duration was within 20-30 seconds.cchp-node-memory:current-fix(sha256:5175937dcf9c129bb0ccccf1cf1d972e3eb74d03618480723c26d24313c02085) lacks OCI source/revision labels.a5f51d1cand cannot validate that final guard at runtime.Scope
The commit contains exactly 3 implementation files and 4 unit-test files. Local
cch-benchmarkharness changes and runtime artifacts remain uncommitted and are not part of this PR.This PR is ready for human review and is not being merged automatically.
Greptile Summary
This PR reduces proxy memory amplification by introducing copy-on-write request mutation, sharing read-only shadow-attempt buffers, and strengthening ReplaySpool ownership and cleanup barriers.
Confidence Score: 5/5
The PR appears safe to merge because the previously reported ReplaySpool bootstrap/abort cleanup race is closed and no blocking failure remains.
No blocking failure remains.
Important Files Changed
Sequence Diagram
sequenceDiagram participant Stream as Response stream participant Spool as ReplaySpool participant Redis as Redis replay store participant PG as PostgreSQL replay store Stream->>Spool: observe(chunk) Spool->>Spool: retain payload and queue batch Spool->>Redis: writeOwned(batch) alt successful completion after billing Spool->>Redis: writeOwned(tail) Spool->>Spool: takePayload() Spool->>PG: persistCompleted(payload) Spool->>Redis: completeOwned() Spool->>Spool: clear payload and release quota else abort or disable Spool->>Spool: mark aborting and clear local payloads Spool->>Redis: abortOwned() after queued writes Spool->>Spool: release quota after cleanup endReviews (2): Last reviewed commit: "fix(types): resolve tsgo type inference ..." | Re-trigger Greptile
Context used: