fix(proxy): prevent large session requests from cascading into OOM - #1457
Conversation
Introduce SESSION_REQUEST_ARTIFACT_MAX_BYTES (default 5 MiB, range 64 KiB to 64 MiB) and skip persisting requestBody, messages, and snapshot body or messages whose serialized size exceeds the limit. Lightweight snapshot headers and meta are still written, oversized request payloads are no longer structured-cloned before proxy mutations run, and the SessionManager deletes any pre-existing key when an oversized write is rejected so a previous oversized body cannot linger in Redis. The guard runs at every relevant SessionManager write site (storeSessionRequestBody, storeSessionMessages, storeSessionRequestPhaseSnapshot) and at ProxySessionGuard, which now consults a new ProxySession.shouldPersistSessionRequestArtifacts predicate before cloning the message and before invoking storeSessionRequestBody or storeSessionMessages.
getSessionDetails used to fetch the legacy requestBody, messages, and response keys on every detail load, even when the modern phase snapshots already contain those fields. Compute candidate body, messages, and response values from the request/response snapshots first and only fall back to the SessionManager.getSessionRequestBody/getSessionMessages/getSessionResponse calls when a snapshot field is missing. The common-path session detail load now avoids three extra large Redis reads, removing a major contributor to memory pressure during dashboard refresh.
The streaming hedge launchAlternative path previously kept spawning new provider attempts as each first-byte deadline expired, so a string of slow or non-responsive upstreams could fan out without bound and accumulate concurrent request bodies in memory. Refuse to launch another attempt when two are already in flight; the scheduler will retry as soon as an in-flight attempt settles. This caps peak concurrency for the legacy hedge schedule while preserving the replacement-on-failure behavior the forwarder relies on.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthrough本次变更为会话请求工件增加可配置的字节上限，限制超限内容写入 Redis，并让详情读取优先使用阶段快照。同时，旧版流式竞速路径将并发 attempt 限制为 2。 Changes会话请求工件持久化与读取
旧版流式竞速并发控制
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to The PR limits hedge concurrency and suppresses oversized debug artifacts, but large request data can still be transformed before the persistence cap, an aborted request can briefly create a replacement upstream attempt, and terminated sessions may retain request artifacts until expiration. These bounded availability and data-lifecycle risks require explicit owner follow-up before merge. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 11 files. (1 skipped: 1 unsupported.)
✨ 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}
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| }, | ||
| }; | ||
|
|
||
| const snapshotRequestBody = snapshots.request.after?.body ?? snapshots.request.before?.body; |
There was a problem hiding this comment.
[MEDIUM] [LOGIC-BUG] Legacy requestBody field now prefers the upstream-forwarded body over the client body
Why this is a problem: The legacy key this field mirrors (storeSessionRequestBody / getSessionRequestBody) is documented as "客户端原始请求体" - the client's request body captured at guard time, before any format conversion or request filtering. This derivation prefers snapshots.request.after.body, which is session.forwardedRequestBody: the post-conversion body actually sent to the upstream provider. For cross-format routing (e.g. an OpenAI client served by a Claude provider) or model-redirected requests, data.requestBody returned by GET /api/v1/sessions/{id} now contains a completely different payload than it did before this PR, while both snapshots still exist (the normal case for completed requests). The adjacent snapshotMessages derivation two lines below prefers before first, correctly preserving the client-side semantics of its legacy counterpart - both legacy fields were captured at the same guard-time point, so requestBody should match that ordering.
Suggested fix:
const snapshotRequestBody = snapshots.request.before?.body ?? snapshots.request.after?.body;The forwarded body remains available to consumers via data.snapshots.request.after, which is what the dashboard UI reads (session-messages-client.tsx only consumes snapshots).
| return !this.highConcurrencyModeEnabled; | ||
| } | ||
|
|
||
| shouldPersistSessionRequestArtifacts(): boolean { |
There was a problem hiding this comment.
[MEDIUM] [TEST-MISSING-CRITICAL] No unit test covers the artifact-size gate's measurement logic
Why this is a problem: This method contains the boundary logic of the OOM fix itself - buffer-hint precedence (avoiding re-serialization of the body), the multipart exception (measuring the small logical body instead of the raw multipart buffer, which would falsely reject image requests), and the byteSize <= maxBytes cutoff. Every test in this PR bypasses the real implementation: session-guard-warmup-intercept.test.ts stubs shouldPersistSessionRequestArtifacts to a constant (lines 97 and 307), and the session-manager tests exercise only the string path of getSessionRequestArtifactByteSize. A regression here - for example dropping the multipart exception so every image request is measured via JSON.stringify, or inverting the hint precedence so the buffer is ignored - would silently re-introduce the serialization spike this PR eliminates, and the suite would still pass. CLAUDE.md requires "All new features must have unit test coverage of at least 80%", and the sibling gate shouldPersistSessionDebugArtifacts is already directly tested in tests/unit/proxy/session.test.ts ("ProxySession high-concurrency policy").
Suggested fix (add to tests/unit/proxy/session.test.ts, reusing its createSession helper):
describe("ProxySession request artifact gate", () => {
it("buffer 提示超过上限时跳过 request artifacts", () => {
const session = createSession({ redirectedModel: null, requestMessage: { messages: [] } });
session.request.buffer = new Uint8Array(6 * 1024 * 1024);
expect(session.shouldPersistSessionRequestArtifacts()).toBe(false);
});
it("multipart 请求按逻辑 body 而非原始 buffer 判定大小", () => {
const session = createSession({
redirectedModel: null,
requestMessage: { model: "gpt-image-1", prompt: "cat" },
});
session.request.buffer = new Uint8Array(6 * 1024 * 1024);
vi.spyOn(session, "isOpenAIImageMultipartRequest").mockReturnValue(true);
expect(session.shouldPersistSessionRequestArtifacts()).toBe(true);
});
it("恰好等于上限时仍保留 artifacts", () => {
const session = createSession({ redirectedModel: null, requestMessage: { messages: [] } });
session.request.buffer = new Uint8Array(5 * 1024 * 1024);
expect(session.shouldPersistSessionRequestArtifacts()).toBe(true);
});
});
There was a problem hiding this comment.
Code Review Summary
This PR bounds three distinct memory amplifiers (hedge fan-out width, oversized request debug artifacts, redundant Redis reads) with a clean layering: the session guard gates expensive clone/serialize work before it happens, SessionManager re-enforces the limit at every Redis write boundary (including stale-key cleanup), and session details now read legacy large payloads only when snapshots are absent. The hedge cap is correctly placed - failure handling deletes the attempt before re-invoking launchAlternative, so replacement semantics survive while width stays at 2. Two medium issues were found: a semantic drift in the legacy requestBody field, and missing direct test coverage for the new artifact-size gate.
PR Size: M
- Lines changed: 346 (277 additions, 69 deletions)
- Files changed: 12
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 1 | 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 | 1 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
None.
High Priority Issues (Should Fix)
-
[MEDIUM] Legacy
requestBodysemantic drift (src/actions/active-sessions.ts:1111, confidence 80): The field now preferssnapshots.request.after.body(the post-conversion body forwarded upstream) over the before-snapshot. The legacy key it replaces was documented as the client's original request body ("客户端原始请求体"), and the adjacentsnapshotMessagesderivation correctly prefersbeforefirst. For cross-format routing,data.requestBodyfromGET /api/v1/sessions/{id}now returns a different payload than before. Suggested fix: swap the preference tobefore?.body ?? after?.body. -
[MEDIUM] Missing tests for the artifact-size gate (
src/app/v1/_lib/proxy/session.ts:588, confidence 80):shouldPersistSessionRequestArtifactsholds the boundary logic of this fix (buffer-hint precedence, multipart exception, size cutoff) but every test in the PR stubs it; a regression would silently re-introduce the serialization spike and still pass CI. Suggested tests provided inline, following the existingshouldPersistSessionDebugArtifactspattern intests/unit/proxy/session.test.ts.
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Validated as non-issues during review: hedge cap placement (failure replacement verified against handleAttemptFailure ordering at forwarder.ts:5394-5441), multipart measurement design (logical body excludes primary image file content, so the buffer must not be used as the hint), stale-key deletion paths (all inside logged try/catch), after-snapshot gating (store boundary rejects oversized forwarded bodies while preserving headers/meta), env schema consistency with SESSION_RESPONSE_BODY_MAX_BYTES, and nullish-coalescing correctness in the legacy fallback conditions.
Automated review by Claude AI
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
1 participant
Summary
Problem
A roughly 19.36 MB request could fan out across 10-24 legacy hedge attempts. Every attempt cloned, serialized, and transported the full request body, while session debugging persisted several redundant Redis artifacts. Anonymous RSS then reached the 10 GiB pod cgroup limit and the kernel killed Node.
Related Issues:
Fix-chain context: continues the v0.9.4+ OOM hardening series #1439 / #1440 / #1441 / #1453.
Solution
Three complementary bounds, each targeting a different amplifier:
forwarder.ts): legacy streaming hedge now launches at most 2 concurrent attempts (LEGACY_STREAMING_HEDGE_MAX_CONCURRENCY). When an in-flight attempt fails, a replacement is still launched, so failover semantics are preserved - only the fan-out width is bounded, capping the per-request memory multiplier on large bodies.src/lib/session-request-artifact-limit.ts):SESSION_REQUEST_ARTIFACT_MAX_BYTES(default 5 MiB, range 64 KiB-64 MiB) bounds each request debug artifact (requestBody / messages / before-after snapshot bodies). Oversized artifacts are rejected beforestructuredClonein the session guard (avoiding the clone/serialize spike entirely) and deleted rather than written inSessionManager(including stale keys from a previous smaller payload). Headers and meta are always preserved so session debugging still works.active-sessions.ts):getSessionDetailsnow derives requestBody/messages/response from the phase snapshots it already loads, and only falls back to the legacy large-payload Redis keys when snapshot content is missing (compatibility with historical data).Changes
Core Changes
src/app/v1/_lib/proxy/forwarder.ts: cap legacy streaming hedge at 2 in-flight attempts, keep failure replacementsrc/lib/session-request-artifact-limit.ts(new): shared max-bytes helper plus byte-size computation with a buffer-length hint to avoid re-serializing the body just to measure itsrc/lib/session-manager.ts:canStoreSessionRequestArtifactguard instoreSessionRequestBody/storeSessionMessages/ request phase-snapshot writes; oversized values skip the write and delete any stale keysrc/app/v1/_lib/proxy/session-guard.ts+session.ts: newshouldPersistSessionRequestArtifacts()gate - oversized requests skipstructuredClone,getMessages(), and body/messages stores, while the lightweight before-snapshot (headers/meta) is still persistedsrc/actions/active-sessions.ts: snapshot-first reads with legacy fallback only when snapshot body/messages are absentSupporting Changes
src/lib/config/env.schema.ts+.env.example: newSESSION_REQUEST_ARTIFACT_MAX_BYTES(default 5 MiB, range 64 KiB-64 MiB) with documentationBehavioral Notes
No migrations or API changes. Two intentional behavior changes reviewers should be aware of:
SESSION_REQUEST_ARTIFACT_MAX_BYTES.Testing
Automated Tests
bun run typecheck,bun run lint,bun run buildLC_ALL=Cand a 70-second timeout respectivelyManual Testing
Skipped oversized session request artifactsfires, and Session details still show headers/meta with bodies omitted.Checklist
.env.example)Description enhanced by Claude AI
Greptile Summary
This PR limits the memory amplification caused by large session requests while retaining routing recovery and lightweight diagnostics.
Confidence Score: 5/5
The PR appears safe to merge, with no concrete correctness or security failures identified in the changed paths.
The new limits are applied before the principal cloning path and again at Redis write boundaries, hedge failures continue launching replacement providers, and session-detail fallback remains field-specific when snapshots are absent.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Incoming proxy request] --> B{Request artifact size within limit?} B -- Yes --> C[Clone and persist request body/messages] B -- No --> D[Persist headers and metadata only] C --> E[Forward upstream] D --> E E --> F[Run at most two legacy hedge attempts] F --> G{Attempt succeeds?} G -- Yes --> H[Commit winning response] G -- No --> I{Another provider available?} I -- Yes --> F I -- No --> J[Return terminal failure] H --> K[Read phase snapshots for session details] J --> K K --> L{Snapshot field available?} L -- Yes --> M[Return snapshot value] L -- No --> N[Fetch corresponding legacy artifact]Reviews (1): Last reviewed commit: "fix(forwarder): cap legacy streaming hed..." | Re-trigger Greptile
Context used: