fix(gateway): bound fragmented health response retention - #641
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough헬스 스트리밍 응답은 청크 배열 대신 사전 할당 버퍼에 기록됩니다. 실제 수신 바이트만 검증에 사용됩니다. 테스트는 Changes스트리밍 헬스 응답
Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix ✨ Finishing Touches📝 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 |
seonghobae
left a comment
There was a problem hiding this comment.
Current-head review on 73f13b3a3826c5d6f7aafc89d1ff23747f8cf8e9: the repair is causal and confined to Noema's contextual-orchestrator health preflight. The stream path now has fixed retained capacity (65,536 bytes), rejects an overflowing chunk before copy, avoids per-chunk retention plus Buffer.concat, preserves reader cancellation/lock cleanup and all existing UTF-8/JSON/orchestrator-identity checks, and does not alter provider/model/retry/failover authority. Hosted application CI, reviewer-ci, Security Scan, and patch-validator-image are all terminal success on this exact head; unresolved inline thread count is zero. No additional valid finding from this review.
Finding
verifyOrchestratorHealthz()enforces a 65,536-byte payload ceiling while streaming, but protected main retained every receivedBufferinchunks[]until EOF and then performedBuffer.concat. A legal health response fragmented into many tiny chunks therefore created allocation/object-retention pressure proportional to chunk count even though payload bytes were bounded. This is the same bounded-bytes-but-fragment-cardinality class already removed from the Cloudflare control-plane reader, while authority here remains Noema's contextual-orchestrator health preflight only.RED
Test-only exact
ac063326717b67144e8bae628920e9e64708bb07fed a valid orchestrator health document one byte per stream chunk and required the streaming path to finish without a secondBuffer.concatmaterialization. Hosted application CI run34626575434checked out that exact SHA, passed live-base/toolchain/install/typecheck, then failed only atrelease tests: 675 existing test files / 4,691 existing tests passed and the new fragmented-response assertion failed becauseBuffer.concatreceived the retained one-byte chunks.Repair
Current exact
73f13b3a3826c5d6f7aafc89d1ff23747f8cf8e9replaceschunks[]plus final concatenation with one fixed 65,536-byte buffer. Each admitted stream chunk is rejected before copy if it would cross the existing byte ceiling, otherwise copied directly into the fixed buffer, and only the received subarray is decoded after EOF. The existing cleanup, fatal UTF-8/duplicate-key admission, optional explicit preflight timeout, and fail-closed orchestrator identity check remain unchanged.Scope
No provider/model/group routing, retry/failover policy, model timeout, credentials, quarantine/security/outbound authority, or contextual-orchestrator server behavior moves into Noema. Keep Draft until this final exact head reaches all required hosted gates and receives a clean current-head review.
Summary by CodeRabbit
버그 수정
테스트