Repository navigation
fix: add finalize API for handler cleanup - #2738
Merged
Merged
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRequestHandler adds a resolver-facing finalize(callback) API, schedules per-request or iterator-specific cleanup callbacks, wires abort listeners to trigger cleanup, and runs/awaits scheduled cleanups at resolver completion or iterator exhaustion; tests and typings were added to validate behavior and signature. ChangesRequestHandler Finalize Cleanup Feature
Sequence Diagram(s)sequenceDiagram
participant Client
participant Resolver
participant RequestHandler
participant Signal
participant Cleanup as runScheduledCleanups
Client->>RequestHandler: send request
RequestHandler->>Resolver: invoke resolver(info with finalize)
Resolver->>RequestHandler: calls finalize(cleanup_fn)
RequestHandler->>RequestHandler: scheduledCleanups.unshift(cleanup_fn)
Signal->>RequestHandler: abort event
RequestHandler->>Cleanup: runScheduledCleanups()
Cleanup-->>RequestHandler: await callbacks (may throw)
RequestHandler->>Client: respond / propagate error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/handlers/RequestHandler.ts (1)
197-205:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
reset()leaks pendingscheduledCleanups, undermining the core SSE use caseWhen
server.resetHandlers()is called (e.g. inafterEach),reset()clearsresolverIteratorbut leavesscheduledCleanupsuntouched. For a generator handler that has already executed at least one.next()(registering cleanups viafinalize), two bad outcomes follow:
- No further request arrives → cleanups never run → resource leak (e.g.
setIntervalkeeps firing across subsequent tests).- A new request arrives →
!this.resolverIteratoris true,wrapResolverre-runs setup, and thefinallyblock drains stale cleanups from the previous request's lifecycle during the new request's initialization — semantically wrong.This directly undercuts the primary motivation of this PR (SSE interval cleanup in tests).
🐛 Proposed fix — mirror the existing fire-and-forget pattern
protected reset(): void { const iterator = this.resolverIterator this.resolverIterator = undefined this.resolverIteratorResult = undefined if (typeof iterator?.return === 'function') { void Promise.resolve(iterator.return()) } + + void this.runScheduledCleanups() }🤖 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/core/handlers/RequestHandler.ts` around lines 197 - 205, reset() currently clears resolverIterator but leaves scheduledCleanups intact, causing leaked/incorrect cleanup runs; update reset() to drain and clear scheduledCleanups too: if this.scheduledCleanups exists, copy it, set this.scheduledCleanups = undefined, then iterate the copy and invoke each cleanup in a fire-and-forget manner (e.g. void Promise.resolve(cleanup())) so callbacks run asynchronously and are not reused by future wrapResolver/resolverIterator lifecycles; keep existing behavior for resolverIterator and resolverIteratorResult.
🧹 Nitpick comments (1)
test/node/msw-api/finalize.test.ts (1)
1-181: ⚡ Quick winMissing test for concurrent requests exposes the shared-
scheduledCleanupsbugAll tests exercise a single in-flight request at a time. The concurrency issue flagged in the source (two simultaneous requests sharing the handler's cleanup queue) would not be caught here. Consider adding a test like:
it('runs cleanups independently per request', async () => { const cleanupA = vi.fn() const cleanupB = vi.fn() server.use( http.get('http://localhost/resource', async ({ finalize, request }) => { const which = new URL(request.url).searchParams.get('id') finalize(which === 'a' ? cleanupA : cleanupB) await setTimeout(which === 'a' ? 300 : 150) return new Response(which ?? '') }), ) // Issue both requests concurrently; B resolves first const [, responseB] = await Promise.all([ fetch('http://localhost/resource?id=a'), fetch('http://localhost/resource?id=b'), ]) // cleanupB should run when B finishes; cleanupA must NOT run yet expect(cleanupB).toHaveBeenCalledOnce() expect(cleanupA).not.toHaveBeenCalled() })This test will currently fail (cleanupA would be called prematurely when B finishes) and would serve as a regression guard once the per-request queue fix is applied.
🤖 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 `@test/node/msw-api/finalize.test.ts` around lines 1 - 181, Add a concurrent-request test that ensures finalize callbacks are tracked per request rather than shared: add an it('runs cleanups independently per request', async () => { ... }) test which registers a handler via server.use(http.get(..., async ({ finalize, request }) => { ... })) that finalizes either cleanupA or cleanupB based on request URL param, awaits different setTimeout durations (e.g., 300 vs 150 ms) and returns a Response, then issues both fetches concurrently with Promise.all and asserts cleanupB has been calledOnce while cleanupA has not been called yet; this will reproduce and guard against the shared scheduledCleanups bug.
🤖 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/core/handlers/RequestHandler.ts`:
- Around line 463-467: The catch block in RequestHandler (where
errors.push(error) is executed) silently ignores non-Error throws; update the
catch to normalize any thrown value before pushing into the errors array: detect
if caught value is an instance of Error, otherwise wrap it in a new Error (e.g.,
include typeof and JSON.stringify/value as message) so strings, numbers, and
plain objects are converted into Error objects before calling errors.push(...);
keep the existing conditional branch for real Error instances and ensure the
normalization preserves useful context for debugging.
- Around line 350-363: scheduledCleanups is currently a handler-level array
shared across concurrent run() calls, so finalize callbacks from different
requests collide and a single abort drains all callbacks; change to a
per-request queue keyed by requestId: create a map (e.g.
requestScheduledCleanups) on the handler and update
runScheduledCleanups(requestId) and reset(requestId) to operate only on
requestScheduledCleanups.get(requestId); in run(), when adding the abort
listener call runScheduledCleanups(requestId) and when passing finalize into
executeResolver push callbacks into requestScheduledCleanups.get(requestId) (or
create the array if missing) instead of this.scheduledCleanups, and ensure
cleanup removes the map entry after draining.
---
Outside diff comments:
In `@src/core/handlers/RequestHandler.ts`:
- Around line 197-205: reset() currently clears resolverIterator but leaves
scheduledCleanups intact, causing leaked/incorrect cleanup runs; update reset()
to drain and clear scheduledCleanups too: if this.scheduledCleanups exists, copy
it, set this.scheduledCleanups = undefined, then iterate the copy and invoke
each cleanup in a fire-and-forget manner (e.g. void Promise.resolve(cleanup()))
so callbacks run asynchronously and are not reused by future
wrapResolver/resolverIterator lifecycles; keep existing behavior for
resolverIterator and resolverIteratorResult.
---
Nitpick comments:
In `@test/node/msw-api/finalize.test.ts`:
- Around line 1-181: Add a concurrent-request test that ensures finalize
callbacks are tracked per request rather than shared: add an it('runs cleanups
independently per request', async () => { ... }) test which registers a handler
via server.use(http.get(..., async ({ finalize, request }) => { ... })) that
finalizes either cleanupA or cleanupB based on request URL param, awaits
different setTimeout durations (e.g., 300 vs 150 ms) and returns a Response,
then issues both fetches concurrently with Promise.all and asserts cleanupB has
been calledOnce while cleanupA has not been called yet; this will reproduce and
guard against the shared scheduledCleanups bug.
🪄 Autofix (Beta)
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 31fb30d7-b80b-4cc8-a4cd-d45d003c5187
📒 Files selected for processing (2)
src/core/handlers/RequestHandler.tstest/node/msw-api/finalize.test.ts
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 `@src/core/handlers/RequestHandler.ts`:
- Around line 466-484: runScheduledCleanups can run concurrently and iterate the
same this.scheduledCleanups array, causing duplicate executions; to fix,
atomically drain the queue by capturing the array into a local variable and
clearing this.scheduledCleanups (set to []) before entering the for/await loop
so each cleanup is executed exactly once; update the runScheduledCleanups method
to: if length is zero return, otherwise const toRun = this.scheduledCleanups;
this.scheduledCleanups = []; then iterate await on toRun, preserving the
existing error collection logic; this prevents the abort listener and
wrapResolver finally from double-running the same cleanup.
🪄 Autofix (Beta)
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9888c236-fcd5-4b8b-b56d-a300f5dd4883
📒 Files selected for processing (4)
src/core/handlers/RequestHandler.tstest/typings/graphql.test-d.tstest/typings/http.test-d.tstest/typings/sse.test-d.ts
Member
Author
Released: v2.14.4 🎉This has been released in v2.14.4. Get these changes by running the following command: Predictable release automation by Release. |
3 tasks done
9 tasks done
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
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.
onCleanupcallback to schedule side effect cleanup when resolvers are finished #2630