diff --git a/contributingGuides/SEQUENTIAL_QUEUE.md b/contributingGuides/SEQUENTIAL_QUEUE.md index ecb91b055174..d007244f2a67 100644 --- a/contributingGuides/SEQUENTIAL_QUEUE.md +++ b/contributingGuides/SEQUENTIAL_QUEUE.md @@ -373,6 +373,8 @@ Three concerns cut across every block. **How it works today.** `flush()` hard-gates on `isClientTheLeader()` (the last client GUID (globally unique identifier) in the shared active-clients list wins, with an `isPromotingNewLeader` carve-out: during the `beforeunload` handoff the departing leader keeps answering `true` even when its GUID is no longer last). Only the leader drains; followers may still **enqueue** (their genuinely-new requests merge cross-tab via `requestID` not in `knownRequestIDs`) but never send. Leadership is checked **once** per flush, before the running flag flips. +**Self-healing the active-clients list.** The leader is derived from `ACTIVE_CLIENTS`, which `ActiveClientManager`'s `connectWithoutView` callback keeps in sync. A late or stale write to that key can overwrite the list and drop a still-live client; if the dropped client was the leader, `flush()` would gate forever on a GUID belonging to a tab that no longer exists, stalling the queue. To prevent this deadlock, the callback re-appends the current client whenever an incoming list omits it while the tab is alive (`hasInitialized && !isLeavingTab`), so leadership settles back onto a real tab. The one exception is the cap-trim (`maxClients`, 20): a client the trim loop evicted on purpose is captured *before* the trim runs (`wasClientInList`) and is **not** re-added, or tabs past the cap would thrash leadership by continually re-adding themselves. + **Sharp edges.** - **Mid-flush leadership change.** Because leadership is checked once and the async `process()` recursion never re-checks, a leader tab closed/refreshed mid-request can hand leadership to a follower that flushes the same shared queue while the old request is still in flight. There is no cross-tab lock on the ongoing request. Duplicate-send avoidance relies on server-side `DUPLICATE_RECORD` / `ALREADY_CREATED` reconciliation, not a lock. - **`requestID` is per-tab, not globally unique.** It comes from a module-scoped counter initialized to a wall-clock timestamp at module load and incremented per request (stamped as `requestIndex`; `requestID` is the legacy fallback name); each tab has its own counter. Timestamp seeding *reduces* but does not *eliminate* cross-tab `requestID` collisions, which the `knownRequestIDs` cross-tab diff relies on. diff --git a/src/libs/ActiveClientManager/index.ts b/src/libs/ActiveClientManager/index.ts index d5f0ef815b0d..a68699ecf21d 100644 --- a/src/libs/ActiveClientManager/index.ts +++ b/src/libs/ActiveClientManager/index.ts @@ -22,6 +22,10 @@ const savedSelfPromise = new Promise((resolve) => { resolveSavedSelfPromise = resolve; }); let beforeunloadListenerAdded = false; +// Whether init() has run. Guards the connect callback from re-adding us before we've registered. +let hasInitialized = false; +// Set while this tab is unloading, so the connect callback doesn't fight our own cleanup by re-adding us. +let isLeavingTab = false; /** * Determines when the client is ready. We need to wait both till we saved our ID in onyx AND the init method was called @@ -36,17 +40,28 @@ Onyx.connectWithoutView({ return; } + // Whether this client was present in the incoming list, captured before the cap trim below mutates it. + const wasClientInList = val.includes(clientID); + activeClients = val; - // Remove from the beginning of the list any clients that are past the limit, to avoid having thousands of them - let removed = false; + // Trim clients past the limit from the front so the list can't grow unbounded + let changed = false; while (activeClients.length >= maxClients) { activeClients.shift(); - removed = true; + changed = true; + } + + // If another tab's write dropped us from the list while we're still alive, re-add ourselves so + // leadership doesn't get stuck on a dead tab. Skip this when the cap trim above evicted us on + // purpose — otherwise leadership thrashes with many open tabs. + if (hasInitialized && !isLeavingTab && !wasClientInList) { + activeClients.push(clientID); + changed = true; } // Save the clients back to onyx, if they changed - if (removed) { + if (changed) { setActiveClients(activeClients); } }, @@ -75,6 +90,7 @@ const isClientTheLeader: IsClientTheLeader = () => { }; const cleanUpClientId = () => { + isLeavingTab = true; isPromotingNewLeader = isClientTheLeader(); activeClients = activeClients.filter((id) => id !== clientID); setActiveClients(activeClients); @@ -93,6 +109,8 @@ const removeBeforeUnloadListener = () => { * We want to ensure we have no duplicates and that the activeClient gets added at the end of the array (see isClientTheLeader) */ const init: Init = () => { + hasInitialized = true; + isLeavingTab = false; removeBeforeUnloadListener(); activeClients = activeClients.filter((id) => id !== clientID); activeClients.push(clientID); diff --git a/tests/unit/ActiveClientManagerTest.ts b/tests/unit/ActiveClientManagerTest.ts new file mode 100644 index 000000000000..3bde068bd4f7 --- /dev/null +++ b/tests/unit/ActiveClientManagerTest.ts @@ -0,0 +1,55 @@ +import type {Init, IsClientTheLeader, IsReady} from '@libs/ActiveClientManager/types'; + +import ONYXKEYS from '@src/ONYXKEYS'; + +import Onyx from 'react-native-onyx'; + +import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; + +// Load the web implementation explicitly. Jest defaults to the native platform, and the native +// ActiveClientManager is a no-op, so we must pin the `.ts` (web) file to exercise leader election. +const ActiveClientManager = jest.requireActual<{ + init: Init; + isClientTheLeader: IsClientTheLeader; + isReady: IsReady; +}>('@libs/ActiveClientManager/index.ts'); + +const GHOST_GUID = 'ghost-guid-from-a-dead-tab'; + +// Persistent subscriber that always holds the latest ACTIVE_CLIENTS value (avoids connect/disconnect races). +let latestActiveClients: string[] | undefined; + +// Learned from the first init() (which pushes our GUID last) and reused across tests. +let clientID: string; + +beforeAll(async () => { + Onyx.init({keys: ONYXKEYS}); + Onyx.connect({ + key: ONYXKEYS.ACTIVE_CLIENTS, + callback: (value) => { + latestActiveClients = value; + }, + }); + ActiveClientManager.init(); + await ActiveClientManager.isReady(); + await waitForBatchedUpdates(); + clientID = latestActiveClients?.at(-1) ?? ''; +}); + +describe('ActiveClientManager', () => { + it('registers this client as the leader on init', () => { + expect(clientID).not.toBe(''); + expect(latestActiveClients).toEqual([clientID]); + expect(ActiveClientManager.isClientTheLeader()).toBe(true); + }); + + it('re-adds this client when a stale hydration event drops it (boot-race guard)', async () => { + // Simulate a late disk-hydration overwrite that omits the live client and leaves only a ghost GUID. + await Onyx.set(ONYXKEYS.ACTIVE_CLIENTS, [GHOST_GUID]); + await waitForBatchedUpdates(); + + // The connect callback must re-append our GUID so the ghost can't hold leadership. + expect(latestActiveClients).toEqual([GHOST_GUID, clientID]); + expect(ActiveClientManager.isClientTheLeader()).toBe(true); + }); +});