[No QA] Fix leader-election deadlock in ActiveClientManager (SequentialQueue unable to flush)

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.

src/libs/ActiveClientManager/index.ts

@@ -22,6 +22,10 @@ const savedSelfPromise = new Promise<void>((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);

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);
+    });
+});