Conversation
github-actions
Bot
added
the
maintainer:e2e:ok
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe change adds OCR transport scenario lifecycle helpers, request and response tracking, failure-safe monitor cleanup, and comprehensive lifecycle tests. Existing concurrency canary tests now use scoped resource management. ChangesOCR transport lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
WalkthroughBefore this PR, the OCR transport test surface could leave child processes, temp directories, and monitor state behind when an assertion failed or a test exited early. The existing concurrency canary coverage exercised successful transport paths, but a failed startup, timeout, mid-stream error, or client abort could still leak resources or mask cleanup problems. After this PR, the transport lifecycle is covered by a new failure-safe test suite that forces invalid targets, readiness timeouts, upstream crashes, and client aborts, while the concurrency tests reuse scoped cleanup helpers that aggregate cleanup errors and reap children deterministically. The scripts TypeScript configuration is also updated so these new lifecycle tests are compiled and discoverable by the test runner. Release NotesTests
Refactor
Changes
Magnitude🎯 1 (S) RelatedNo related items found. Pre-merge Checks
Walkthrough generated by LLxprt PR Review. Planner issue: #2256 |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/tests/ocr-concurrency-canary-2673-helpers.ts (1)
264-276: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle callback exceptions inside
proxyRequest.If
onResponsethrows, the exception escapes the asynchronous response listener, leaving the Promise pending and potentially terminating the process. IfonRequestthrows, the Promise rejects while the request remains undestroyed. Register request handlers beforeonRequest, catch both callback exceptions, destroy the request or response, and reject the Promise.🤖 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 `@scripts/tests/ocr-concurrency-canary-2673-helpers.ts` around lines 264 - 276, Update proxyRequest to register request and response error handlers before invoking onRequest, then wrap both onRequest and onResponse callbacks in try/catch blocks. On callback failure, destroy the associated request or response and reject the Promise, ensuring no asynchronous exception escapes and no request remains open.
🧹 Nitpick comments (1)
scripts/tests/ocr-concurrency-canary-2673.test.ts (1)
495-521: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSettle the streaming promise on abnormal response termination.
The promise resolves only on the response
endevent. It rejects only on a request-levelerror. If the proxy or upstream destroys the response after headers, neither event fires. The scenario then hangs until the test timeout instead of failing fast. The sibling test at lines 602-618 already handles this case withcloseanderrorlisteners.♻️ Proposed fix to bound the streaming wait
(response) => { const chunks: Buffer[] = []; response.on('data', (chunk) => { if (chunks.length === 0) firstChunkBeforeCompletion = !upstreamCompleted; chunks.push(chunk); }); response.on('end', () => resolve(Buffer.concat(chunks).toString()), ); + response.on('error', reject); + response.once('close', () => { + if (!response.complete) { + reject(new Error('streamed response closed before end')); + } + }); },🤖 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 `@scripts/tests/ocr-concurrency-canary-2673.test.ts` around lines 495 - 521, Update the response handling in the streaming promise around the request callback to reject on abnormal response termination by adding response error and close handling, while resolving only on end. Ensure the close listener does not reject after normal completion, and preserve the existing chunk collection and request-level error handling.
🤖 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 `@scripts/tests/ocr-transport-lifecycle-2744-helpers.ts`:
- Around line 87-90: In the onRequest error handler, remove the explicit
clientRequests.delete call after createdRequest.destroy(). Keep the failed
request tracked until the trackRequest close listener handles its removal, while
preserving the destroy and rethrow behavior.
- Around line 70-76: Update registerUpstream and registerMonitor to preserve
every registered resource instead of overwriting the existing upstream or
monitor. Store each registration in a collection and ensure the corresponding
cleanup path iterates over and cleans all resources, or explicitly reject
duplicate registrations while retaining the existing cleanup behavior.
In `@scripts/tests/ocr-transport-lifecycle-2744.test.ts`:
- Around line 105-112: Add timeout: 30_000 and killSignal: 'SIGKILL' to the
execFileSync options in the probe helper, ensuring the child process is forcibly
terminated if it hangs while preserving the existing Bun ESM execution setup.
---
Outside diff comments:
In `@scripts/tests/ocr-concurrency-canary-2673-helpers.ts`:
- Around line 264-276: Update proxyRequest to register request and response
error handlers before invoking onRequest, then wrap both onRequest and
onResponse callbacks in try/catch blocks. On callback failure, destroy the
associated request or response and reject the Promise, ensuring no asynchronous
exception escapes and no request remains open.
---
Nitpick comments:
In `@scripts/tests/ocr-concurrency-canary-2673.test.ts`:
- Around line 495-521: Update the response handling in the streaming promise
around the request callback to reject on abnormal response termination by adding
response error and close handling, while resolving only on end. Ensure the close
listener does not reject after normal completion, and preserve the existing
chunk collection and request-level error handling.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 810cba01-c963-40ec-b895-3e4c4e5eb684
⛔ Files ignored due to path filters (1)
project-plans/issue2744.mdis excluded by!project-plans/**
📒 Files selected for processing (5)
scripts/tests/ocr-concurrency-canary-2673-helpers.tsscripts/tests/ocr-concurrency-canary-2673.test.tsscripts/tests/ocr-transport-lifecycle-2744-helpers.tsscripts/tests/ocr-transport-lifecycle-2744.test.tstsconfig.scripts.json
![github-actions[bot]](https://avatars.github.com/in/15368?s=60&v=4){kind=small}
OpenCodeReview — automatic reviews suspendedAutomatic OCR reviews are suspended for this PR after 2 of 2 automatic reviews. To get more reviews you can:
OpenCodeReview — PR #3166
Findings without a resolvable position
|
|
PR OCR overflow disposition — Reject: the base proxy helper invokes its request callback immediately after request construction, but the only scenario callback supplied here first registers ownership, and its user-callback catch destroys and synchronously deregisters the real Bun request before rethrowing. The Promise executor preserves that exact callback failure. The dedicated pre-traffic callback regression proves the request is destroyed with no cleanup timeout or leak. Moving the callback before request construction is impossible because the callback receives that request; adding speculative handling for an asynchronous event within the same synchronous stack is not warranted. No source change was made for this finding. The changed-file coverage notice is a review-tool coverage warning rather than a source defect; CI independently ran and passed the scripts test shard. |
|
Second PR OCR overflow dispositions — Reject for both destroy-throw variants. A request or response is removed from ownership only after it is already destroyed/complete or after its real close event. If destroy itself throws, deleting it from the set would falsely claim release of a resource that is not proven closed and would suppress the bounded ownership failure. The current path retains the original destroy failure plus the subsequent observable cleanup timeout, then continues with monitor and upstream cleanup. The tracking sets are local to the scenario and become collectible after rejection; they do not create a process-lifetime registry leak. Triggering these findings would also require replacing the controlled Bun destroy methods. No source change was made. The 3/5 review coverage warning and one tool file-read failure are review diagnostics, not source findings; final CI scripts tests and lint passed. |
1 participant
TLDR
Makes the OCR real-loopback transport integration tests failure-safe and event-driven. Each scenario now owns its client requests and active responses, embedded monitor process and temporary directory, and upstream server/connections so failures cannot strand resources. The abort regression destroys the client only after the complete request body and first streamed response chunk are observed, then waits for bounded request/response close events instead of sleeping.
Dive Deeper
Reviewer Test Plan
Run the focused lifecycle and existing transport suites independently:
Then validate scripts typing and targeted lint:
Local evidence:
Testing Matrix
Linked issues / bugs
Fixes #2744
Summary by CodeRabbit
Bug Fixes
Tests