Client suspension and reactivation by parfeon · Pull Request #496 · pubnub/javascript · GitHub
Skip to content

Client suspension and reactivation#496

Open
parfeon wants to merge 5 commits into
masterfrom
fix/shared-worker-subscription-reliability
Open

Client suspension and reactivation#496
parfeon wants to merge 5 commits into
masterfrom
fix/shared-worker-subscription-reliability

Conversation

@parfeon

@parfeon parfeon commented May 7, 2026

Copy link
Copy Markdown
Contributor

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): 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 2 commits May 7, 2026 19:44
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 parfeon self-assigned this May 7, 2026
@parfeon parfeon added the status: done This issue is considered resolved. label May 7, 2026
@parfeon parfeon requested a review from mohitpubnub as a code owner May 7, 2026 16:51
@parfeon parfeon added priority: medium This PR should be reviewed after all high priority PRs. type: fix This PR contains fixes to existing features. labels May 7, 2026
@coderabbitai

coderabbitai Bot commented May 7, 2026

Copy link
Copy Markdown

@pubnub-ops-terraform

pubnub-ops-terraform commented May 7, 2026

Copy link
Copy Markdown

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Scope suspended events to the target client.

At Line 444, shared-worker-client-suspended bypasses clientIdentifier filtering. 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 win

Potential out-of-bounds splice when client not found in array.

If clientIdx is -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: suspendClient at line 155 already has this guard, but unregisterClient does 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

📥 Commits

Reviewing files that changed from the base of the PR and between 1d2ee43 and 4b07c09.

⛔ Files ignored due to path filters (4)
  • dist/web/pubnub.js is excluded by !**/dist/**
  • dist/web/pubnub.min.js is excluded by !**/dist/**, !**/*.min.js
  • dist/web/pubnub.worker.js is excluded by !**/dist/**
  • dist/web/pubnub.worker.min.js is excluded by !**/dist/**, !**/*.min.js
📒 Files selected for processing (12)
  • lib/types/index.d.ts
  • src/transport/subscription-worker/components/custom-events/client-event.ts
  • src/transport/subscription-worker/components/custom-events/client-manager-event.ts
  • src/transport/subscription-worker/components/heartbeat-requests-manager.ts
  • src/transport/subscription-worker/components/pubnub-client.ts
  • src/transport/subscription-worker/components/pubnub-clients-manager.ts
  • src/transport/subscription-worker/components/subscribe-requests-manager.ts
  • src/transport/subscription-worker/components/subscription-state.ts
  • src/transport/subscription-worker/subscription-worker-middleware.ts
  • src/transport/subscription-worker/subscription-worker-types.ts
  • src/web/index.ts
  • test/integration/shared-worker/shared-worker.test.ts

Comment thread src/transport/subscription-worker/subscription-worker-middleware.ts Outdated
Comment thread test/integration/shared-worker/shared-worker.test.ts Outdated
Add defensive guards for splice index, abort controller reuse, token chain recovery, and remove
`it.only` from tests.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/transport/subscription-worker/components/pubnub-clients-manager.ts (1)

178-194: 💤 Low value

Consider guarding against reactivating an already-active client.

reactivateClient doesn't check if the client is already registered in this.clients. If called for a non-suspended client (due to race conditions or programming error), it would overwrite the existing entry without aborting the previous AbortController, 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

📥 Commits

Reviewing files that changed from the base of the PR and between 4b07c09 and 8d58d7c.

⛔ Files ignored due to path filters (4)
  • dist/web/pubnub.js is excluded by !**/dist/**
  • dist/web/pubnub.min.js is excluded by !**/dist/**, !**/*.min.js
  • dist/web/pubnub.worker.js is excluded by !**/dist/**
  • dist/web/pubnub.worker.min.js is excluded by !**/dist/**, !**/*.min.js
📒 Files selected for processing (5)
  • src/transport/subscription-worker/components/heartbeat-requests-manager.ts
  • src/transport/subscription-worker/components/pubnub-clients-manager.ts
  • src/transport/subscription-worker/components/subscribe-requests-manager.ts
  • src/transport/subscription-worker/subscription-worker-middleware.ts
  • test/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

Copy link
Copy Markdown
Contributor

The reactivateClient method can overwrite an existing
this.clients[client.identifier] and leak the previous AbortController/listeners

wondering about coderabbit's potential outdated/old abortController cleanup.

@parfeon

parfeon commented May 14, 2026

Copy link
Copy Markdown
Contributor Author

@parfeon parfeon requested a review from mohitpubnub May 14, 2026 15:06

@mohitpubnub mohitpubnub left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority: medium This PR should be reviewed after all high priority PRs. status: done This issue is considered resolved. type: fix This PR contains fixes to existing features.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants