fix: await in-flight requests from closing clients#2724
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR converts the worker stop operation to be asynchronous, enabling proper synchronization of in-flight requests before shutdown. Changes span type definitions, the setup worker API, service worker implementation, worker channel events, and message handling to resolve requests being processed after Changes
Sequence DiagramsequenceDiagram
participant Client
participant SetupWorker
participant ServiceWorkerSource
participant ServiceWorker
Client->>SetupWorker: stop()
activate SetupWorker
SetupWorker->>ServiceWorkerSource: disable()
activate ServiceWorkerSource
ServiceWorkerSource->>ServiceWorker: postMessage({type: 'msw/worker:stop'})
activate ServiceWorker
ServiceWorker->>ServiceWorker: await Promise.allSettled(pendingRequests)
deactivate ServiceWorker
ServiceWorker->>ServiceWorkerSource: postMessage({type: 'CLIENT_CLOSED'})
ServiceWorkerSource->>ServiceWorkerSource: DeferredPromise resolves
deactivate ServiceWorkerSource
SetupWorker->>Client: Promise resolves
deactivate SetupWorker
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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 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: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/browser/sources/service-worker-source.ts (1)
186-199: Refresh the shutdown notes to match the new close protocol.The implementation no longer has a
#stoppedAtpassthrough guard in#handleRequest, so these comments now describe behavior that was removed.📝 Proposed comment update
/** * `@note` Tell the Service Worker to drop this client from its active set - * so it stops forwarding REQUEST events here. `stoppedAt` still guards - * any requests the SW already forwarded before this message arrived. + * so it stops forwarding REQUEST events here. The worker acknowledges + * this only after any pending responses for this client have settled. */ this.#channel.postMessage('CLIENT_CLOSE') await closedPromise /** * `@note` Do NOT reset `workerPromise` here. The channel must continue to - * resolve the currently registered SW so that any in-flight requests the - * worker forwards after `stop()` can be answered with `PASSTHROUGH` by - * `#handleRequest`. `#startWorker` swaps in a fresh deferred on re-enable. + * resolve the currently registered SW until the close handshake completes. + * `#startWorker` swaps in a fresh deferred on re-enable. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/browser/sources/service-worker-source.ts` around lines 186 - 199, Update the shutdown comments to remove references to the removed `#stoppedAt` passthrough guard and instead describe the current close protocol: when `this.#channel.postMessage('CLIENT_CLOSE')` is sent and we await `closedPromise`, note that we still keep the existing `workerPromise`/resolved service worker alive so `#handleRequest` can process any in-flight requests forwarded by the SW, and that `#startWorker` will create a fresh deferred only when re-enabling; also remove any mention of a `stoppedAt` guard and ensure the comment accurately ties the behavior to `#channel.postMessage('CLIENT_CLOSE')`, `closedPromise`, `workerPromise`, `#handleRequest`, and `#startWorker`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/browser/sources/service-worker-source.ts`:
- Around line 186-199: Update the shutdown comments to remove references to the
removed `#stoppedAt` passthrough guard and instead describe the current close
protocol: when `this.#channel.postMessage('CLIENT_CLOSE')` is sent and we await
`closedPromise`, note that we still keep the existing `workerPromise`/resolved
service worker alive so `#handleRequest` can process any in-flight requests
forwarded by the SW, and that `#startWorker` will create a fresh deferred only
when re-enabling; also remove any mention of a `stoppedAt` guard and ensure the
comment accurately ties the behavior to `#channel.postMessage('CLIENT_CLOSE')`,
`closedPromise`, `workerPromise`, `#handleRequest`, and `#startWorker`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e59a331e-cb73-4277-8d83-25d4a30fc735
📒 Files selected for processing (6)
src/browser/sources/service-worker-source.tssrc/mockServiceWorker.jstest/browser/msw-api/setup-worker/start/handlers-reset.test.tstest/browser/msw-api/setup-worker/start/start-after-stop.test.tstest/browser/msw-api/setup-worker/stop/removes-all-listeners.test.tstest/browser/msw-api/unregister.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/mockServiceWorker.js
Warning
This is a breaking change as it includes breaking changes to the worker.
Changes
The worker now tracks all pending response promises by
client.id. Then, whenever a client closes, it awaits its pending responses to settle before sending back a close confirmation message. This fixes the race condition between client closure and in-flight requests that might still be pending.Verification