Client suspension and reactivation#496
Open
parfeon wants to merge 5 commits into
Open
Conversation
Implement suspension mechanism that keeps the port listener alive when a client misses ping-pong,
removes it from subscription/heartbeat state, and reactivates on next message from the resumed tab.
fix(shared-worker): `squashedChanges` iterates wrong array
Fix inner loop in `squashedChanges` to iterate `sortedChanges` instead of `requestAddChange` so that
add+remove pair elimination actually executes for same-request entries within aggregation window.
fix(shared-worker): orphaned `SubscriptionState` on empty changes
Guard `handleDelayedAggregation` against creating subscription state when changes queue is empty, and
add self-invalidation in `processChanges` when post-squash changes are empty with no active clients.
fix(shared-worker): `BFCache` interaction with `pagehide` listener
Remove `{ once: true }` from `pagehide` listener so the handler fires correctly when the page truly
navigates away after a prior BFCache entry consumed the one-shot listener.
fix(shared-worker): version comparison and token handling
Replace independent semver component comparison with hierarchical check, fix `accessTokensMap` being
overwritten on each new token, and serialize `onTokenChange` with a promise chain for ordering.
fix(tests): shared worker test reliability
Replace fixed-delay sequential subscription with channel-list verification on status events, and fix
`makeSendable` overrides to always return a valid tuple after calling `done()`.
parfeon
added
priority: medium
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (6)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR implements client suspension and reactivation for PubNub's subscription worker, allowing clients to gracefully handle frozen or background-tab scenarios. When a client misses ping/pong, it is marked suspended; on the next message, it reactivates by clearing suspension state and reattaching event listeners. The middleware coordinates suspension detection and request cleanup via new worker events. ChangesClient Suspension and Reactivation
Sequence Diagram(s)sequenceDiagram
participant Client as PubNubClient
participant Manager as ClientsManager
participant Heartbeat as HeartbeatMgr
participant Subscribe as SubscribeMgr
participant Middleware as Middleware
Note over Client,Middleware: Client lifecycle: registration
Manager->>Client: create & register
Manager->>Heartbeat: ClientsManagerEvent.Registered
Heartbeat->>Heartbeat: attachClientListeners(client)
Manager->>Subscribe: ClientsManagerEvent.Registered
Subscribe->>Subscribe: attachClientListeners(client)
Note over Client,Middleware: Missed ping/pong: suspension
Manager->>Manager: timeout detected
Manager->>Client: suspendClient()
Client->>Client: _suspended = true
Client->>Client: cancelActiveRequests()
Manager->>Manager: dispatch UnregisterEvent
Note over Client,Middleware: Incoming message: reactivation
Client->>Client: message received while suspended
Client->>Client: handleReactivation()
Client->>Client: _suspended = false
Client->>Client: dispatch ReactivateEvent
Manager->>Client: reactivateClient()
Manager->>Heartbeat: ClientsManagerEvent.Reactivated
Heartbeat->>Heartbeat: attachClientListeners(client)
Manager->>Subscribe: ClientsManagerEvent.Reactivated
Subscribe->>Subscribe: attachClientListeners(client)
🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/transport/subscription-worker/subscription-worker-middleware.ts (1)
440-445:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope suspended events to the target client.
At Line 444,
shared-worker-client-suspendedbypassesclientIdentifierfiltering. Then Line 488 rejects all pending callbacks in this instance. A suspend event for another client can incorrectly fail this client’s in-flight requests.🔧 Proposed fix
if ( data.type !== 'shared-worker-ping' && data.type !== 'shared-worker-connected' && data.type !== 'shared-worker-console-log' && data.type !== 'shared-worker-console-dir' && - data.type !== 'shared-worker-client-suspended' && data.clientIdentifier !== this.configuration.clientIdentifier ) return;Also applies to: 481-498
🤖 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/transport/subscription-worker/subscription-worker-middleware.ts` around lines 440 - 445, The 'shared-worker-client-suspended' message is not being filtered by clientIdentifier, causing suspend events from other clients to reject this instance's in-flight requests; update the message handling so the condition that checks data.clientIdentifier !== this.configuration.clientIdentifier also applies to data.type === 'shared-worker-client-suspended', and modify the logic that rejects pending callbacks (the method that currently rejects all pending callbacks when handling 'shared-worker-client-suspended') to only reject callbacks associated with the suspended client's identifier (use the same clientIdentifier field on the pending callback entries) instead of rejecting every outstanding callback.src/transport/subscription-worker/components/pubnub-clients-manager.ts (1)
211-214:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPotential out-of-bounds splice when client not found in array.
If
clientIdxis-1(client not in array),splice(-1, 1)removes the last element, which would corrupt the client list.Proposed fix
const clientsBySubscribeKey = this.clientBySubscribeKey[client.subKey]; if (clientsBySubscribeKey) { const clientIdx = clientsBySubscribeKey.indexOf(client); - clientsBySubscribeKey.splice(clientIdx, 1); + if (clientIdx >= 0) clientsBySubscribeKey.splice(clientIdx, 1);Note:
suspendClientat line 155 already has this guard, butunregisterClientdoes not.🤖 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/transport/subscription-worker/components/pubnub-clients-manager.ts` around lines 211 - 214, In unregisterClient (pubnub-clients-manager.ts) avoid calling splice with -1 by checking that the client exists before removing: locate the code that accesses this.clientBySubscribeKey[client.subKey] and computing clientIdx = clientsBySubscribeKey.indexOf(client) and add a guard like clientIdx !== -1 (or use a filter to remove the exact reference) before calling clientsBySubscribeKey.splice; mirror the same presence check used in suspendClient to prevent accidentally removing the last array element when the client isn't found.
🤖 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/transport/subscription-worker/components/heartbeat-requests-manager.ts`:
- Around line 183-186: The attachClientListeners method currently overwrites
this.clientAbortControllers[client.identifier] without aborting any existing
controller, which allows duplicate heartbeat/presence handlers; update
attachClientListeners to check this.clientAbortControllers[client.identifier]
and if an existing AbortController exists, call abort() (and optionally remove
it) before creating and storing the new AbortController for the PubNubClient,
and ensure any listener cleanup tied to the AbortSignal runs so duplicate
handlers are not left registered.
In `@src/transport/subscription-worker/components/subscribe-requests-manager.ts`:
- Around line 372-375: The attachClientListeners function currently overwrites
this.clientAbortControllers[client.identifier] with a new AbortController
without aborting any prior controller, which allows duplicate handlers to
remain; before creating and assigning a new AbortController in
attachClientListeners, check for an existing controller at
this.clientAbortControllers[client.identifier], call .abort() on it (and
optionally delete it) to cancel / clean up previous listeners/work, then create
and assign the new AbortController so re-attaching won’t leave duplicate
handlers enqueued.
In `@src/transport/subscription-worker/subscription-worker-middleware.ts`:
- Around line 232-248: The tokenChangeChain currently becomes permanently
rejected if parsedAccessToken or scheduleEventPost rejects; wrap the promise
sequence attached to tokenChangeChain so that errors are caught and handled
(e.g., log via the worker logger) and the chain returns a resolved value to
allow future updates to run; specifically, update the code that chains
parsedAccessToken(...) -> then(...) -> scheduleEventPost(updateEvent) on
tokenChangeChain to append a .catch(err => { /* log with context about
updateEvent/clientIdentifier */ }) that swallows or converts the error into a
resolved outcome so tokenChangeChain remains recoverable while still recording
the failure.
In `@test/integration/shared-worker/shared-worker.test.ts`:
- Line 1637: The test uses it.only which will cause all other tests to be
skipped; remove the .only from the it.only call (the test named "should receive
timeout presence events when browser tabs are closed") so it becomes a normal
it(...) test, ensuring the full suite runs in CI and eliminating this debugging
artifact.
---
Outside diff comments:
In `@src/transport/subscription-worker/components/pubnub-clients-manager.ts`:
- Around line 211-214: In unregisterClient (pubnub-clients-manager.ts) avoid
calling splice with -1 by checking that the client exists before removing:
locate the code that accesses this.clientBySubscribeKey[client.subKey] and
computing clientIdx = clientsBySubscribeKey.indexOf(client) and add a guard like
clientIdx !== -1 (or use a filter to remove the exact reference) before calling
clientsBySubscribeKey.splice; mirror the same presence check used in
suspendClient to prevent accidentally removing the last array element when the
client isn't found.
In `@src/transport/subscription-worker/subscription-worker-middleware.ts`:
- Around line 440-445: The 'shared-worker-client-suspended' message is not being
filtered by clientIdentifier, causing suspend events from other clients to
reject this instance's in-flight requests; update the message handling so the
condition that checks data.clientIdentifier !==
this.configuration.clientIdentifier also applies to data.type ===
'shared-worker-client-suspended', and modify the logic that rejects pending
callbacks (the method that currently rejects all pending callbacks when handling
'shared-worker-client-suspended') to only reject callbacks associated with the
suspended client's identifier (use the same clientIdentifier field on the
pending callback entries) instead of rejecting every outstanding callback.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: af3072b3-e398-4591-a9aa-2e0658bb5f06
⛔ Files ignored due to path filters (4)
dist/web/pubnub.jsis excluded by!**/dist/**dist/web/pubnub.min.jsis excluded by!**/dist/**,!**/*.min.jsdist/web/pubnub.worker.jsis excluded by!**/dist/**dist/web/pubnub.worker.min.jsis excluded by!**/dist/**,!**/*.min.js
📒 Files selected for processing (12)
lib/types/index.d.tssrc/transport/subscription-worker/components/custom-events/client-event.tssrc/transport/subscription-worker/components/custom-events/client-manager-event.tssrc/transport/subscription-worker/components/heartbeat-requests-manager.tssrc/transport/subscription-worker/components/pubnub-client.tssrc/transport/subscription-worker/components/pubnub-clients-manager.tssrc/transport/subscription-worker/components/subscribe-requests-manager.tssrc/transport/subscription-worker/components/subscription-state.tssrc/transport/subscription-worker/subscription-worker-middleware.tssrc/transport/subscription-worker/subscription-worker-types.tssrc/web/index.tstest/integration/shared-worker/shared-worker.test.ts
Add defensive guards for splice index, abort controller reuse, token chain recovery, and remove `it.only` from tests.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/transport/subscription-worker/components/pubnub-clients-manager.ts (1)
178-194: 💤 Low valueConsider guarding against reactivating an already-active client.
reactivateClientdoesn't check if the client is already registered inthis.clients. If called for a non-suspended client (due to race conditions or programming error), it would overwrite the existing entry without aborting the previousAbortController, potentially leaking listeners.🛡️ Suggested defensive guard
private reactivateClient(client: PubNubClient) { + // Abort any existing controller to prevent duplicate listeners if called erroneously. + const existing = this.clients[client.identifier]; + if (existing?.abortController) existing.abortController.abort(); + this.clients[client.identifier] = { client, abortController: new AbortController() };🤖 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/transport/subscription-worker/components/pubnub-clients-manager.ts` around lines 178 - 194, The reactivateClient method can overwrite an existing this.clients[client.identifier] and leak the previous AbortController/listeners; update reactivateClient to first check if this.clients[client.identifier] already exists and either (a) if already active, return early (no-op) or (b) if you intend to replace, call abort() on the existing entry's abortController and clean up subscriptions before overwriting; ensure you also avoid duplicating entries in this.clientBySubscribeKey[client.subKey] when reactivating and only call subscribeOnClientEvents and dispatchEvent after safely replacing or confirming activation.
🤖 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.
Nitpick comments:
In `@src/transport/subscription-worker/components/pubnub-clients-manager.ts`:
- Around line 178-194: The reactivateClient method can overwrite an existing
this.clients[client.identifier] and leak the previous AbortController/listeners;
update reactivateClient to first check if this.clients[client.identifier]
already exists and either (a) if already active, return early (no-op) or (b) if
you intend to replace, call abort() on the existing entry's abortController and
clean up subscriptions before overwriting; ensure you also avoid duplicating
entries in this.clientBySubscribeKey[client.subKey] when reactivating and only
call subscribeOnClientEvents and dispatchEvent after safely replacing or
confirming activation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 62cc01f8-2c2e-46c4-9d68-59986c60ec51
⛔ Files ignored due to path filters (4)
dist/web/pubnub.jsis excluded by!**/dist/**dist/web/pubnub.min.jsis excluded by!**/dist/**,!**/*.min.jsdist/web/pubnub.worker.jsis excluded by!**/dist/**dist/web/pubnub.worker.min.jsis excluded by!**/dist/**,!**/*.min.js
📒 Files selected for processing (5)
src/transport/subscription-worker/components/heartbeat-requests-manager.tssrc/transport/subscription-worker/components/pubnub-clients-manager.tssrc/transport/subscription-worker/components/subscribe-requests-manager.tssrc/transport/subscription-worker/subscription-worker-middleware.tstest/integration/shared-worker/shared-worker.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/transport/subscription-worker/subscription-worker-middleware.ts
- src/transport/subscription-worker/components/subscribe-requests-manager.ts
mohitpubnub
reviewed
May 13, 2026
| { signal: abortController.signal }, | ||
| ); | ||
| client.addEventListener( | ||
| PubNubClientEvent.AuthChange, |
Contributor
There was a problem hiding this comment.
So during suspension also, If AuthChange happens, The token will get updated and when user reactivate the subscription(/heartbeat) there won't be 403.
Am i getting this right?
Contributor
Author
There was a problem hiding this comment.
Well, there probably will be no activity on the client, if tab deactivated. If it will "wake up" and would like to change PAM token, then it will reactivate first along with listeners attachments before configuration change handler will be called.
Next calls will pick on new access token.
Contributor
wondering about coderabbit's potential outdated/old abortController cleanup. |
Contributor
Author
Not an issue here — |
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
3 participants
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.
feat(shared-worker): client suspension and reactivation
Implement suspension mechanism that keeps the port listener alive when a client misses ping-pong, removes it from subscription/heartbeat state, and reactivates on next message from the resumed tab.
fix(shared-worker):
squashedChangesiterates wrong arrayFix inner loop in
squashedChangesto iteratesortedChangesinstead ofrequestAddChangeso that add+remove pair elimination actually executes for same-request entries within aggregation window.fix(shared-worker): orphaned
SubscriptionStateon empty changesGuard
handleDelayedAggregationagainst creating subscription state when changes queue is empty, and add self-invalidation inprocessChangeswhen post-squash changes are empty with no active clients.fix(shared-worker):
BFCacheinteraction withpagehidelistenerRemove
{ once: true }frompagehidelistener so the handler fires correctly when the page truly navigates away after a prior BFCache entry consumed the one-shot listener.fix(shared-worker): version comparison and token handling
Replace independent semver component comparison with hierarchical check, fix
accessTokensMapbeing overwritten on each new token, and serializeonTokenChangewith a promise chain for ordering.fix(tests): shared worker test reliability
Replace fixed-delay sequential subscription with channel-list verification on status events, and fix
makeSendableoverrides to always return a valid tuple after callingdone().