From 17bd5ad0958c5c9db25e5bf0803564f033717b04 Mon Sep 17 00:00:00 2001 From: eliran goshen Date: Thu, 2 Jul 2026 10:16:05 +0200 Subject: [PATCH 1/5] Fix leader-election deadlock in ActiveClientManager A stale/ghost client GUID left in ACTIVE_CLIENTS (from an unclean shutdown) could hold leadership, leaving the only live tab unable to flush the SequentialQueue so all requests pile up indefinitely. - Re-append this client to ACTIVE_CLIENTS if a late hydration event drops it, so a ghost GUID can't strip the live tab out of the list on boot. - Add a self-promotion safety net: if the queue is stuck as a non-leader with queued work past a timeout, claim leadership and flush. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/CONST/index.ts | 4 + src/libs/ActiveClientManager/index.native.ts | 6 +- src/libs/ActiveClientManager/index.ts | 35 ++++++- src/libs/ActiveClientManager/types.ts | 4 +- src/libs/Network/SequentialQueue.ts | 50 +++++++++- tests/unit/ActiveClientManagerTest.ts | 66 +++++++++++++ .../unit/SequentialQueueLeaderRecoveryTest.ts | 97 +++++++++++++++++++ 7 files changed, 253 insertions(+), 9 deletions(-) create mode 100644 tests/unit/ActiveClientManagerTest.ts create mode 100644 tests/unit/SequentialQueueLeaderRecoveryTest.ts diff --git a/src/CONST/index.ts b/src/CONST/index.ts index 16405e6d2b39..1ffdb55e3703 100644 --- a/src/CONST/index.ts +++ b/src/CONST/index.ts @@ -2411,6 +2411,10 @@ const CONST = { SUSTAINED_FAILURE_THRESHOLD_COUNT: 3, SUSTAINED_FAILURE_WINDOW_MS: 10 * 1000, RECONNECT_STAMPEDE_JITTER_MS: 5000, + // How long the queue may sit with pending requests while this tab is not the leader before we + // assume the leader is a stale/ghost tab and self-promote to flush. Generous so a legitimately + // busy leader in another tab flushes well before this fires. + STUCK_QUEUE_LEADER_PROMOTION_TIMEOUT_MS: 30 * 1000, }, // The number of milliseconds for an idle session to expire SESSION_EXPIRATION_TIME_MS: 2 * 3600 * 1000, // 2 hours diff --git a/src/libs/ActiveClientManager/index.native.ts b/src/libs/ActiveClientManager/index.native.ts index 7de57a10e5f1..2080bb7c1b67 100644 --- a/src/libs/ActiveClientManager/index.native.ts +++ b/src/libs/ActiveClientManager/index.native.ts @@ -2,7 +2,7 @@ * For native devices, there will never be more than one * client running at a time, so this lib is a big no-op */ -import type {Init, IsClientTheLeader, IsReady} from './types'; +import type {Init, IsClientTheLeader, IsReady, PromoteToLeader} from './types'; const init: Init = () => {}; @@ -10,4 +10,6 @@ const isClientTheLeader: IsClientTheLeader = () => true; const isReady: IsReady = () => Promise.resolve(); -export {init, isClientTheLeader, isReady}; +const promoteToLeader: PromoteToLeader = () => {}; + +export {init, isClientTheLeader, isReady, promoteToLeader}; diff --git a/src/libs/ActiveClientManager/index.ts b/src/libs/ActiveClientManager/index.ts index 3776e2a53690..578b6e513d63 100644 --- a/src/libs/ActiveClientManager/index.ts +++ b/src/libs/ActiveClientManager/index.ts @@ -8,7 +8,7 @@ import Onyx from 'react-native-onyx'; import {setModalVisibility} from '@libs/actions/Modal'; import {setActiveClients} from '@userActions/ActiveClients'; import ONYXKEYS from '@src/ONYXKEYS'; -import type {Init, IsClientTheLeader, IsReady} from './types'; +import type {Init, IsClientTheLeader, IsReady, PromoteToLeader} from './types'; const clientID = Str.guid(); const maxClients = 20; @@ -18,6 +18,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 @@ -35,14 +39,22 @@ Onyx.connectWithoutView({ 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; + let changed = false; while (activeClients.length >= maxClients) { activeClients.shift(); - removed = true; + changed = true; + } + + // A late disk-hydration event can overwrite the list and drop this live client, handing + // leadership to a stale/ghost GUID and stalling the SequentialQueue. Re-append ourselves so + // we're never silently removed while still alive (unless we're intentionally leaving). + if (hasInitialized && !isLeavingTab && !activeClients.includes(clientID)) { + activeClients.push(clientID); + changed = true; } // Save the clients back to onyx, if they changed - if (removed) { + if (changed) { setActiveClients(activeClients); } }, @@ -70,7 +82,18 @@ const isClientTheLeader: IsClientTheLeader = () => { return lastActiveClient === clientID; }; +/** + * Force this client to become the leader by moving its GUID to the end of the list. Last-resort + * safety net for when the queue is stuck because a stale/ghost GUID is holding leadership. + */ +const promoteToLeader: PromoteToLeader = () => { + activeClients = activeClients.filter((id) => id !== clientID); + activeClients.push(clientID); + setActiveClients(activeClients); +}; + const cleanUpClientId = () => { + isLeavingTab = true; isPromotingNewLeader = isClientTheLeader(); activeClients = activeClients.filter((id) => id !== clientID); setActiveClients(activeClients); @@ -89,6 +112,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); @@ -101,4 +126,4 @@ const init: Init = () => { }); }; -export {init, isClientTheLeader, isReady}; +export {init, isClientTheLeader, isReady, promoteToLeader}; diff --git a/src/libs/ActiveClientManager/types.ts b/src/libs/ActiveClientManager/types.ts index a5c7d1e6d721..3733ad63ff85 100644 --- a/src/libs/ActiveClientManager/types.ts +++ b/src/libs/ActiveClientManager/types.ts @@ -4,4 +4,6 @@ type IsClientTheLeader = () => boolean; type IsReady = () => Promise; -export type {Init, IsClientTheLeader, IsReady}; +type PromoteToLeader = () => void; + +export type {Init, IsClientTheLeader, IsReady, PromoteToLeader}; diff --git a/src/libs/Network/SequentialQueue.ts b/src/libs/Network/SequentialQueue.ts index 9c4a1b366bf7..2750a1a8003e 100644 --- a/src/libs/Network/SequentialQueue.ts +++ b/src/libs/Network/SequentialQueue.ts @@ -15,7 +15,7 @@ import { update as updatePersistedRequest, } from '@libs/actions/PersistedRequests'; import {flushQueue, isEmpty} from '@libs/actions/QueuedOnyxUpdates'; -import {isClientTheLeader} from '@libs/ActiveClientManager'; +import {isClientTheLeader, promoteToLeader} from '@libs/ActiveClientManager'; import {WRITE_COMMANDS} from '@libs/API/types'; import Log from '@libs/Log'; import {getIsOffline as isOfflineNetwork} from '@libs/NetworkState'; @@ -47,6 +47,16 @@ type RequestError = Error & { let resolveIsReadyPromise: (() => void) | undefined; let isReadyPromise: Promise = Promise.resolve(); let isReadyPromisePending = false; +// Timer for the stuck-queue self-promotion safety net (see flush()). +let stuckQueueTimeoutId: ReturnType | null = null; + +function clearStuckQueueTimer() { + if (!stuckQueueTimeoutId) { + return; + } + clearTimeout(stuckQueueTimeoutId); + stuckQueueTimeoutId = null; +} /** * Marks isReadyPromise as pending so any READ that consults waitForIdle() parks behind us. @@ -298,6 +308,38 @@ function process(): Promise { return currentRequestPromise; } +/** + * Schedules a one-shot check that self-promotes this client to leader if the queue is still stuck + * (non-empty, no leader flushing it) after the timeout. This recovers from a leader-election + * deadlock where a stale/ghost GUID holds leadership and never flushes the shared queue. + */ +function scheduleStuckQueueCheck() { + if (stuckQueueTimeoutId) { + return; + } + stuckQueueTimeoutId = setTimeout(() => { + stuckQueueTimeoutId = null; + + // Conditions may have changed while we waited (we became leader, went offline, the queue + // paused, or another flush is already running) — in those cases there's nothing to recover. + if (isClientTheLeader() || isOfflineNetwork() || isQueuePaused || isSequentialQueueRunning) { + return; + } + + const hasWork = getAllPersistedRequests().length > 0 || !!getPersistedOngoingRequest(); + if (!hasWork) { + return; + } + + Log.alert('[SequentialQueue] Queue stuck with no leader flushing it; self-promoting to leader to recover.', { + persistedRequestsLength: getAllPersistedRequests().length, + hasOngoingRequest: !!getPersistedOngoingRequest(), + }); + promoteToLeader(); + flush(); + }, CONST.NETWORK.STUCK_QUEUE_LEADER_PROMOTION_TIMEOUT_MS); +} + /** * @param shouldResetPromise Determines whether the isReadyPromise should be reset. * A READ request will wait until all the WRITE requests are done, using the isReadyPromise promise. @@ -361,9 +403,15 @@ function flush(shouldResetPromise = true) { // process the queue, so resolve here — otherwise READs parked on waitForIdle() would // hang forever on this tab after any write. resolveIsReadyPromise?.(); + // Safety net: a stale/ghost leader GUID can leave the only live tab permanently unable to + // flush. If we're still not the leader after a delay and there's queued work, self-promote. + scheduleStuckQueueCheck(); return; } + // We are the leader, so any pending stuck-queue self-promotion check is no longer needed. + clearStuckQueueTimer(); + Log.info('[SequentialQueue] Starting queue processing', false, { persistedRequestsLength, hasOngoingRequest: !!currentOngoingRequest, diff --git a/tests/unit/ActiveClientManagerTest.ts b/tests/unit/ActiveClientManagerTest.ts new file mode 100644 index 000000000000..d26291680df1 --- /dev/null +++ b/tests/unit/ActiveClientManagerTest.ts @@ -0,0 +1,66 @@ +import Onyx from 'react-native-onyx'; +import type {Init, IsClientTheLeader, IsReady, PromoteToLeader} from '@libs/ActiveClientManager/types'; +import ONYXKEYS from '@src/ONYXKEYS'; +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; + promoteToLeader: PromoteToLeader; +}>('@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); + }); + + it('reclaims leadership via promoteToLeader when a ghost GUID holds the last slot', async () => { + // Our GUID is present but a ghost sits last, so we are not the leader and the callback won't re-add us. + await Onyx.set(ONYXKEYS.ACTIVE_CLIENTS, [clientID, GHOST_GUID]); + await waitForBatchedUpdates(); + expect(ActiveClientManager.isClientTheLeader()).toBe(false); + + ActiveClientManager.promoteToLeader(); + await waitForBatchedUpdates(); + + expect(latestActiveClients?.at(-1)).toBe(clientID); + expect(ActiveClientManager.isClientTheLeader()).toBe(true); + }); +}); diff --git a/tests/unit/SequentialQueueLeaderRecoveryTest.ts b/tests/unit/SequentialQueueLeaderRecoveryTest.ts new file mode 100644 index 000000000000..acf148f2a826 --- /dev/null +++ b/tests/unit/SequentialQueueLeaderRecoveryTest.ts @@ -0,0 +1,97 @@ +import Onyx from 'react-native-onyx'; +import * as NetworkState from '@libs/NetworkState'; +import {clear as clearPersistedRequests, save as savePersistedRequest} from '@userActions/PersistedRequests'; +import CONST from '@src/CONST'; +import ONYXKEYS from '@src/ONYXKEYS'; +import * as SequentialQueue from '../../src/libs/Network/SequentialQueue'; +import type Request from '../../src/types/onyx/Request'; +import type {MockFetch} from '../utils/TestHelper'; +import * as TestHelper from '../utils/TestHelper'; +import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; + +// Mock ActiveClientManager so we control leadership: start as a follower, and let promoteToLeader() flip us to leader. +const mockLeaderState = {isLeader: false}; +const mockIsClientTheLeader = jest.fn(() => mockLeaderState.isLeader); +const mockPromoteToLeader = jest.fn(() => { + mockLeaderState.isLeader = true; +}); +jest.mock('@libs/ActiveClientManager', () => ({ + init: jest.fn(), + isReady: jest.fn(() => Promise.resolve()), + isClientTheLeader: () => mockIsClientTheLeader(), + promoteToLeader: () => mockPromoteToLeader(), +})); + +const request: Request<'userMetadata'> = { + command: 'ReconnectApp', + successData: [{key: 'userMetadata', onyxMethod: 'set', value: {accountID: 1234}}], + failureData: [{key: 'userMetadata', onyxMethod: 'set', value: {}}], +}; + +let mockFetch: MockFetch; + +beforeAll(() => { + Onyx.init({keys: ONYXKEYS}); +}); + +beforeEach(() => { + jest.useFakeTimers(); + mockFetch = TestHelper.createGlobalFetchMock(); + global.fetch = mockFetch; + jest.spyOn(NetworkState, 'getIsOffline').mockReturnValue(false); + mockLeaderState.isLeader = false; + mockIsClientTheLeader.mockClear(); + mockPromoteToLeader.mockClear(); + return Onyx.clear().then(() => SequentialQueue.clearQueueFlushedData()); +}); + +afterEach(() => { + jest.clearAllTimers(); + jest.useRealTimers(); + jest.restoreAllMocks(); +}); + +describe('SequentialQueue leader-election recovery', () => { + it('self-promotes and flushes when stuck as a non-leader with queued work', async () => { + // A request is persisted but this tab is not the leader, so flush() bails and arms the safety-net timer. + savePersistedRequest(request); + await waitForBatchedUpdates(); + SequentialQueue.flush(); + + // Before the timeout elapses we must NOT have self-promoted. + expect(mockPromoteToLeader).not.toHaveBeenCalled(); + + // After the timeout, with the queue still stuck, we self-promote to recover. + jest.advanceTimersByTime(CONST.NETWORK.STUCK_QUEUE_LEADER_PROMOTION_TIMEOUT_MS); + + expect(mockPromoteToLeader).toHaveBeenCalledTimes(1); + expect(mockLeaderState.isLeader).toBe(true); + }); + + it('does not self-promote if it becomes the leader before the timeout', async () => { + savePersistedRequest(request); + await waitForBatchedUpdates(); + SequentialQueue.flush(); + + // Leadership is acquired through the normal path (e.g. ActiveClientManager re-adds us) before the timer fires. + mockLeaderState.isLeader = true; + + jest.advanceTimersByTime(CONST.NETWORK.STUCK_QUEUE_LEADER_PROMOTION_TIMEOUT_MS); + + expect(mockPromoteToLeader).not.toHaveBeenCalled(); + }); + + it('does not self-promote if the queue drained before the timeout', async () => { + savePersistedRequest(request); + await waitForBatchedUpdates(); + SequentialQueue.flush(); + + // The shared queue is emptied by the real leader in another tab before our timer fires. + clearPersistedRequests(); + await waitForBatchedUpdates(); + + jest.advanceTimersByTime(CONST.NETWORK.STUCK_QUEUE_LEADER_PROMOTION_TIMEOUT_MS); + + expect(mockPromoteToLeader).not.toHaveBeenCalled(); + }); +}); From c8c6c063cfa8e675eb1e749ad8efa165b2a919f5 Mon Sep 17 00:00:00 2001 From: eliran goshen Date: Thu, 2 Jul 2026 12:17:33 +0200 Subject: [PATCH 2/5] Address review: avoid duplicate writes and leadership thrash - Self-promote only when there are pending requests and no ongoing request, so a live leader's in-flight request is never re-issued as a duplicate. - Re-add this client to ACTIVE_CLIENTS only when a stale write dropped it, not when the max-client cap trim evicted it, avoiding leadership thrash with 20+ tabs. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/libs/ActiveClientManager/index.ts | 10 +++++++--- src/libs/Network/SequentialQueue.ts | 8 +++++--- tests/unit/SequentialQueueLeaderRecoveryTest.ts | 14 +++++++++++++- 3 files changed, 25 insertions(+), 7 deletions(-) diff --git a/src/libs/ActiveClientManager/index.ts b/src/libs/ActiveClientManager/index.ts index 578b6e513d63..9ac34f9cce39 100644 --- a/src/libs/ActiveClientManager/index.ts +++ b/src/libs/ActiveClientManager/index.ts @@ -36,6 +36,9 @@ 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 @@ -46,9 +49,10 @@ Onyx.connectWithoutView({ } // A late disk-hydration event can overwrite the list and drop this live client, handing - // leadership to a stale/ghost GUID and stalling the SequentialQueue. Re-append ourselves so - // we're never silently removed while still alive (unless we're intentionally leaving). - if (hasInitialized && !isLeavingTab && !activeClients.includes(clientID)) { + // leadership to a stale/ghost GUID and stalling the SequentialQueue. Re-append ourselves when a + // stale write dropped us (we were never in the incoming list) — but not when the cap trim above + // intentionally evicted us, which would otherwise thrash leadership with 20+ open tabs. + if (hasInitialized && !isLeavingTab && !wasClientInList) { activeClients.push(clientID); changed = true; } diff --git a/src/libs/Network/SequentialQueue.ts b/src/libs/Network/SequentialQueue.ts index 2750a1a8003e..d0848137232e 100644 --- a/src/libs/Network/SequentialQueue.ts +++ b/src/libs/Network/SequentialQueue.ts @@ -326,14 +326,16 @@ function scheduleStuckQueueCheck() { return; } - const hasWork = getAllPersistedRequests().length > 0 || !!getPersistedOngoingRequest(); - if (!hasWork) { + // Only recover the deadlock this targets: pending requests that never start because no leader + // flushes them. If a request is already in flight (ongoing), a live leader is/was processing it, + // so promoting could re-issue that same request as a duplicate write — don't. + const hasStalledWork = getAllPersistedRequests().length > 0 && !getPersistedOngoingRequest(); + if (!hasStalledWork) { return; } Log.alert('[SequentialQueue] Queue stuck with no leader flushing it; self-promoting to leader to recover.', { persistedRequestsLength: getAllPersistedRequests().length, - hasOngoingRequest: !!getPersistedOngoingRequest(), }); promoteToLeader(); flush(); diff --git a/tests/unit/SequentialQueueLeaderRecoveryTest.ts b/tests/unit/SequentialQueueLeaderRecoveryTest.ts index acf148f2a826..3c2ad3d1ad2c 100644 --- a/tests/unit/SequentialQueueLeaderRecoveryTest.ts +++ b/tests/unit/SequentialQueueLeaderRecoveryTest.ts @@ -1,6 +1,6 @@ import Onyx from 'react-native-onyx'; import * as NetworkState from '@libs/NetworkState'; -import {clear as clearPersistedRequests, save as savePersistedRequest} from '@userActions/PersistedRequests'; +import {clear as clearPersistedRequests, save as savePersistedRequest, updateOngoingRequest} from '@userActions/PersistedRequests'; import CONST from '@src/CONST'; import ONYXKEYS from '@src/ONYXKEYS'; import * as SequentialQueue from '../../src/libs/Network/SequentialQueue'; @@ -81,6 +81,18 @@ describe('SequentialQueue leader-election recovery', () => { expect(mockPromoteToLeader).not.toHaveBeenCalled(); }); + it('does not self-promote while a request is already in flight (avoids duplicate writes)', async () => { + // A live leader in another tab has a request in flight that outlasts the timeout (e.g. slow upload). + // Promoting here would re-issue the same ongoing request as a duplicate, so we must not. + updateOngoingRequest(request); + await waitForBatchedUpdates(); + SequentialQueue.flush(); + + jest.advanceTimersByTime(CONST.NETWORK.STUCK_QUEUE_LEADER_PROMOTION_TIMEOUT_MS); + + expect(mockPromoteToLeader).not.toHaveBeenCalled(); + }); + it('does not self-promote if the queue drained before the timeout', async () => { savePersistedRequest(request); await waitForBatchedUpdates(); From 1b651de6e87cec2256d7ccee057c895f855e4ee5 Mon Sep 17 00:00:00 2001 From: eliran goshen Date: Fri, 3 Jul 2026 10:25:55 +0200 Subject: [PATCH 3/5] style: apply oxfmt formatting to PR-changed files Co-Authored-By: Claude Opus 4.8 (1M context) --- src/libs/ActiveClientManager/index.ts | 10 +++++++--- tests/unit/ActiveClientManagerTest.ts | 5 ++++- tests/unit/SequentialQueueLeaderRecoveryTest.ts | 9 +++++++-- 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/src/libs/ActiveClientManager/index.ts b/src/libs/ActiveClientManager/index.ts index 9ac34f9cce39..a982d5d032e7 100644 --- a/src/libs/ActiveClientManager/index.ts +++ b/src/libs/ActiveClientManager/index.ts @@ -1,3 +1,9 @@ +import {setModalVisibility} from '@libs/actions/Modal'; + +import {setActiveClients} from '@userActions/ActiveClients'; + +import ONYXKEYS from '@src/ONYXKEYS'; + /** * When you have many tabs in one browser, the data of Onyx is shared between all of them. Since we persist write requests in Onyx, we need to ensure that * only one tab is processing those saved requests or we would be duplicating data (or creating errors). @@ -5,9 +11,7 @@ */ import {Str} from 'expensify-common'; import Onyx from 'react-native-onyx'; -import {setModalVisibility} from '@libs/actions/Modal'; -import {setActiveClients} from '@userActions/ActiveClients'; -import ONYXKEYS from '@src/ONYXKEYS'; + import type {Init, IsClientTheLeader, IsReady, PromoteToLeader} from './types'; const clientID = Str.guid(); diff --git a/tests/unit/ActiveClientManagerTest.ts b/tests/unit/ActiveClientManagerTest.ts index d26291680df1..d1992a3770c9 100644 --- a/tests/unit/ActiveClientManagerTest.ts +++ b/tests/unit/ActiveClientManagerTest.ts @@ -1,6 +1,9 @@ -import Onyx from 'react-native-onyx'; import type {Init, IsClientTheLeader, IsReady, PromoteToLeader} 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 diff --git a/tests/unit/SequentialQueueLeaderRecoveryTest.ts b/tests/unit/SequentialQueueLeaderRecoveryTest.ts index 3c2ad3d1ad2c..67fc659b05fa 100644 --- a/tests/unit/SequentialQueueLeaderRecoveryTest.ts +++ b/tests/unit/SequentialQueueLeaderRecoveryTest.ts @@ -1,11 +1,16 @@ -import Onyx from 'react-native-onyx'; import * as NetworkState from '@libs/NetworkState'; + import {clear as clearPersistedRequests, save as savePersistedRequest, updateOngoingRequest} from '@userActions/PersistedRequests'; + import CONST from '@src/CONST'; import ONYXKEYS from '@src/ONYXKEYS'; -import * as SequentialQueue from '../../src/libs/Network/SequentialQueue'; + +import Onyx from 'react-native-onyx'; + import type Request from '../../src/types/onyx/Request'; import type {MockFetch} from '../utils/TestHelper'; + +import * as SequentialQueue from '../../src/libs/Network/SequentialQueue'; import * as TestHelper from '../utils/TestHelper'; import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; From 84c8c7785a87ddc8fbb95c0d88ec557cba9ef926 Mon Sep 17 00:00:00 2001 From: eliran goshen Date: Fri, 3 Jul 2026 15:40:37 +0200 Subject: [PATCH 4/5] Remove stuck-queue self-promotion timer, keep deterministic live-client guard The 30s self-promotion safety net relied on a non-deterministic timer and risked duplicate writes. Guard #1 (re-appending the live client in the ACTIVE_CLIENTS connect callback) deterministically closes the leader-election deadlock, so drop the timer, promoteToLeader, its constant, and its test. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/CONST/index.ts | 4 - src/libs/ActiveClientManager/index.native.ts | 6 +- src/libs/ActiveClientManager/index.ts | 14 +-- src/libs/ActiveClientManager/types.ts | 4 +- src/libs/Network/SequentialQueue.ts | 52 +------- tests/unit/ActiveClientManagerTest.ts | 16 +-- .../unit/SequentialQueueLeaderRecoveryTest.ts | 114 ------------------ 7 files changed, 7 insertions(+), 203 deletions(-) delete mode 100644 tests/unit/SequentialQueueLeaderRecoveryTest.ts diff --git a/src/CONST/index.ts b/src/CONST/index.ts index 12a98c2a690c..084e4c5d6669 100644 --- a/src/CONST/index.ts +++ b/src/CONST/index.ts @@ -2442,10 +2442,6 @@ const CONST = { SUSTAINED_FAILURE_THRESHOLD_COUNT: 3, SUSTAINED_FAILURE_WINDOW_MS: 10 * 1000, RECONNECT_STAMPEDE_JITTER_MS: 5000, - // How long the queue may sit with pending requests while this tab is not the leader before we - // assume the leader is a stale/ghost tab and self-promote to flush. Generous so a legitimately - // busy leader in another tab flushes well before this fires. - STUCK_QUEUE_LEADER_PROMOTION_TIMEOUT_MS: 30 * 1000, }, // The number of milliseconds for an idle session to expire SESSION_EXPIRATION_TIME_MS: 2 * 3600 * 1000, // 2 hours diff --git a/src/libs/ActiveClientManager/index.native.ts b/src/libs/ActiveClientManager/index.native.ts index 2080bb7c1b67..7de57a10e5f1 100644 --- a/src/libs/ActiveClientManager/index.native.ts +++ b/src/libs/ActiveClientManager/index.native.ts @@ -2,7 +2,7 @@ * For native devices, there will never be more than one * client running at a time, so this lib is a big no-op */ -import type {Init, IsClientTheLeader, IsReady, PromoteToLeader} from './types'; +import type {Init, IsClientTheLeader, IsReady} from './types'; const init: Init = () => {}; @@ -10,6 +10,4 @@ const isClientTheLeader: IsClientTheLeader = () => true; const isReady: IsReady = () => Promise.resolve(); -const promoteToLeader: PromoteToLeader = () => {}; - -export {init, isClientTheLeader, isReady, promoteToLeader}; +export {init, isClientTheLeader, isReady}; diff --git a/src/libs/ActiveClientManager/index.ts b/src/libs/ActiveClientManager/index.ts index a982d5d032e7..2c4943725f28 100644 --- a/src/libs/ActiveClientManager/index.ts +++ b/src/libs/ActiveClientManager/index.ts @@ -12,7 +12,7 @@ import ONYXKEYS from '@src/ONYXKEYS'; import {Str} from 'expensify-common'; import Onyx from 'react-native-onyx'; -import type {Init, IsClientTheLeader, IsReady, PromoteToLeader} from './types'; +import type {Init, IsClientTheLeader, IsReady} from './types'; const clientID = Str.guid(); const maxClients = 20; @@ -90,16 +90,6 @@ const isClientTheLeader: IsClientTheLeader = () => { return lastActiveClient === clientID; }; -/** - * Force this client to become the leader by moving its GUID to the end of the list. Last-resort - * safety net for when the queue is stuck because a stale/ghost GUID is holding leadership. - */ -const promoteToLeader: PromoteToLeader = () => { - activeClients = activeClients.filter((id) => id !== clientID); - activeClients.push(clientID); - setActiveClients(activeClients); -}; - const cleanUpClientId = () => { isLeavingTab = true; isPromotingNewLeader = isClientTheLeader(); @@ -134,4 +124,4 @@ const init: Init = () => { }); }; -export {init, isClientTheLeader, isReady, promoteToLeader}; +export {init, isClientTheLeader, isReady}; diff --git a/src/libs/ActiveClientManager/types.ts b/src/libs/ActiveClientManager/types.ts index 3733ad63ff85..a5c7d1e6d721 100644 --- a/src/libs/ActiveClientManager/types.ts +++ b/src/libs/ActiveClientManager/types.ts @@ -4,6 +4,4 @@ type IsClientTheLeader = () => boolean; type IsReady = () => Promise; -type PromoteToLeader = () => void; - -export type {Init, IsClientTheLeader, IsReady, PromoteToLeader}; +export type {Init, IsClientTheLeader, IsReady}; diff --git a/src/libs/Network/SequentialQueue.ts b/src/libs/Network/SequentialQueue.ts index 4949e2dc7490..3a20f1883ea9 100644 --- a/src/libs/Network/SequentialQueue.ts +++ b/src/libs/Network/SequentialQueue.ts @@ -13,7 +13,7 @@ import { update as updatePersistedRequest, } from '@libs/actions/PersistedRequests'; import {flushQueue, isEmpty} from '@libs/actions/QueuedOnyxUpdates'; -import {isClientTheLeader, promoteToLeader} from '@libs/ActiveClientManager'; +import {isClientTheLeader} from '@libs/ActiveClientManager'; import {WRITE_COMMANDS} from '@libs/API/types'; import Log from '@libs/Log'; import {getIsOffline as isOfflineNetwork} from '@libs/NetworkState'; @@ -50,16 +50,6 @@ type RequestError = Error & { let resolveIsReadyPromise: (() => void) | undefined; let isReadyPromise: Promise = Promise.resolve(); let isReadyPromisePending = false; -// Timer for the stuck-queue self-promotion safety net (see flush()). -let stuckQueueTimeoutId: ReturnType | null = null; - -function clearStuckQueueTimer() { - if (!stuckQueueTimeoutId) { - return; - } - clearTimeout(stuckQueueTimeoutId); - stuckQueueTimeoutId = null; -} /** * Marks isReadyPromise as pending so any READ that consults waitForIdle() parks behind us. @@ -311,40 +301,6 @@ function process(): Promise { return currentRequestPromise; } -/** - * Schedules a one-shot check that self-promotes this client to leader if the queue is still stuck - * (non-empty, no leader flushing it) after the timeout. This recovers from a leader-election - * deadlock where a stale/ghost GUID holds leadership and never flushes the shared queue. - */ -function scheduleStuckQueueCheck() { - if (stuckQueueTimeoutId) { - return; - } - stuckQueueTimeoutId = setTimeout(() => { - stuckQueueTimeoutId = null; - - // Conditions may have changed while we waited (we became leader, went offline, the queue - // paused, or another flush is already running) — in those cases there's nothing to recover. - if (isClientTheLeader() || isOfflineNetwork() || isQueuePaused || isSequentialQueueRunning) { - return; - } - - // Only recover the deadlock this targets: pending requests that never start because no leader - // flushes them. If a request is already in flight (ongoing), a live leader is/was processing it, - // so promoting could re-issue that same request as a duplicate write — don't. - const hasStalledWork = getAllPersistedRequests().length > 0 && !getPersistedOngoingRequest(); - if (!hasStalledWork) { - return; - } - - Log.alert('[SequentialQueue] Queue stuck with no leader flushing it; self-promoting to leader to recover.', { - persistedRequestsLength: getAllPersistedRequests().length, - }); - promoteToLeader(); - flush(); - }, CONST.NETWORK.STUCK_QUEUE_LEADER_PROMOTION_TIMEOUT_MS); -} - /** * @param shouldResetPromise Determines whether the isReadyPromise should be reset. * A READ request will wait until all the WRITE requests are done, using the isReadyPromise promise. @@ -408,15 +364,9 @@ function flush(shouldResetPromise = true) { // process the queue, so resolve here — otherwise READs parked on waitForIdle() would // hang forever on this tab after any write. resolveIsReadyPromise?.(); - // Safety net: a stale/ghost leader GUID can leave the only live tab permanently unable to - // flush. If we're still not the leader after a delay and there's queued work, self-promote. - scheduleStuckQueueCheck(); return; } - // We are the leader, so any pending stuck-queue self-promotion check is no longer needed. - clearStuckQueueTimer(); - Log.info('[SequentialQueue] Starting queue processing', false, { persistedRequestsLength, hasOngoingRequest: !!currentOngoingRequest, diff --git a/tests/unit/ActiveClientManagerTest.ts b/tests/unit/ActiveClientManagerTest.ts index d1992a3770c9..3bde068bd4f7 100644 --- a/tests/unit/ActiveClientManagerTest.ts +++ b/tests/unit/ActiveClientManagerTest.ts @@ -1,4 +1,4 @@ -import type {Init, IsClientTheLeader, IsReady, PromoteToLeader} from '@libs/ActiveClientManager/types'; +import type {Init, IsClientTheLeader, IsReady} from '@libs/ActiveClientManager/types'; import ONYXKEYS from '@src/ONYXKEYS'; @@ -12,7 +12,6 @@ const ActiveClientManager = jest.requireActual<{ init: Init; isClientTheLeader: IsClientTheLeader; isReady: IsReady; - promoteToLeader: PromoteToLeader; }>('@libs/ActiveClientManager/index.ts'); const GHOST_GUID = 'ghost-guid-from-a-dead-tab'; @@ -53,17 +52,4 @@ describe('ActiveClientManager', () => { expect(latestActiveClients).toEqual([GHOST_GUID, clientID]); expect(ActiveClientManager.isClientTheLeader()).toBe(true); }); - - it('reclaims leadership via promoteToLeader when a ghost GUID holds the last slot', async () => { - // Our GUID is present but a ghost sits last, so we are not the leader and the callback won't re-add us. - await Onyx.set(ONYXKEYS.ACTIVE_CLIENTS, [clientID, GHOST_GUID]); - await waitForBatchedUpdates(); - expect(ActiveClientManager.isClientTheLeader()).toBe(false); - - ActiveClientManager.promoteToLeader(); - await waitForBatchedUpdates(); - - expect(latestActiveClients?.at(-1)).toBe(clientID); - expect(ActiveClientManager.isClientTheLeader()).toBe(true); - }); }); diff --git a/tests/unit/SequentialQueueLeaderRecoveryTest.ts b/tests/unit/SequentialQueueLeaderRecoveryTest.ts deleted file mode 100644 index 67fc659b05fa..000000000000 --- a/tests/unit/SequentialQueueLeaderRecoveryTest.ts +++ /dev/null @@ -1,114 +0,0 @@ -import * as NetworkState from '@libs/NetworkState'; - -import {clear as clearPersistedRequests, save as savePersistedRequest, updateOngoingRequest} from '@userActions/PersistedRequests'; - -import CONST from '@src/CONST'; -import ONYXKEYS from '@src/ONYXKEYS'; - -import Onyx from 'react-native-onyx'; - -import type Request from '../../src/types/onyx/Request'; -import type {MockFetch} from '../utils/TestHelper'; - -import * as SequentialQueue from '../../src/libs/Network/SequentialQueue'; -import * as TestHelper from '../utils/TestHelper'; -import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; - -// Mock ActiveClientManager so we control leadership: start as a follower, and let promoteToLeader() flip us to leader. -const mockLeaderState = {isLeader: false}; -const mockIsClientTheLeader = jest.fn(() => mockLeaderState.isLeader); -const mockPromoteToLeader = jest.fn(() => { - mockLeaderState.isLeader = true; -}); -jest.mock('@libs/ActiveClientManager', () => ({ - init: jest.fn(), - isReady: jest.fn(() => Promise.resolve()), - isClientTheLeader: () => mockIsClientTheLeader(), - promoteToLeader: () => mockPromoteToLeader(), -})); - -const request: Request<'userMetadata'> = { - command: 'ReconnectApp', - successData: [{key: 'userMetadata', onyxMethod: 'set', value: {accountID: 1234}}], - failureData: [{key: 'userMetadata', onyxMethod: 'set', value: {}}], -}; - -let mockFetch: MockFetch; - -beforeAll(() => { - Onyx.init({keys: ONYXKEYS}); -}); - -beforeEach(() => { - jest.useFakeTimers(); - mockFetch = TestHelper.createGlobalFetchMock(); - global.fetch = mockFetch; - jest.spyOn(NetworkState, 'getIsOffline').mockReturnValue(false); - mockLeaderState.isLeader = false; - mockIsClientTheLeader.mockClear(); - mockPromoteToLeader.mockClear(); - return Onyx.clear().then(() => SequentialQueue.clearQueueFlushedData()); -}); - -afterEach(() => { - jest.clearAllTimers(); - jest.useRealTimers(); - jest.restoreAllMocks(); -}); - -describe('SequentialQueue leader-election recovery', () => { - it('self-promotes and flushes when stuck as a non-leader with queued work', async () => { - // A request is persisted but this tab is not the leader, so flush() bails and arms the safety-net timer. - savePersistedRequest(request); - await waitForBatchedUpdates(); - SequentialQueue.flush(); - - // Before the timeout elapses we must NOT have self-promoted. - expect(mockPromoteToLeader).not.toHaveBeenCalled(); - - // After the timeout, with the queue still stuck, we self-promote to recover. - jest.advanceTimersByTime(CONST.NETWORK.STUCK_QUEUE_LEADER_PROMOTION_TIMEOUT_MS); - - expect(mockPromoteToLeader).toHaveBeenCalledTimes(1); - expect(mockLeaderState.isLeader).toBe(true); - }); - - it('does not self-promote if it becomes the leader before the timeout', async () => { - savePersistedRequest(request); - await waitForBatchedUpdates(); - SequentialQueue.flush(); - - // Leadership is acquired through the normal path (e.g. ActiveClientManager re-adds us) before the timer fires. - mockLeaderState.isLeader = true; - - jest.advanceTimersByTime(CONST.NETWORK.STUCK_QUEUE_LEADER_PROMOTION_TIMEOUT_MS); - - expect(mockPromoteToLeader).not.toHaveBeenCalled(); - }); - - it('does not self-promote while a request is already in flight (avoids duplicate writes)', async () => { - // A live leader in another tab has a request in flight that outlasts the timeout (e.g. slow upload). - // Promoting here would re-issue the same ongoing request as a duplicate, so we must not. - updateOngoingRequest(request); - await waitForBatchedUpdates(); - SequentialQueue.flush(); - - jest.advanceTimersByTime(CONST.NETWORK.STUCK_QUEUE_LEADER_PROMOTION_TIMEOUT_MS); - - expect(mockPromoteToLeader).not.toHaveBeenCalled(); - }); - - it('does not self-promote if the queue drained before the timeout', async () => { - savePersistedRequest(request); - await waitForBatchedUpdates(); - SequentialQueue.flush(); - - // The shared queue is emptied by the real leader in another tab before our timer fires. - clearPersistedRequests(); - await waitForBatchedUpdates(); - - jest.advanceTimersByTime(CONST.NETWORK.STUCK_QUEUE_LEADER_PROMOTION_TIMEOUT_MS); - - expect(mockPromoteToLeader).not.toHaveBeenCalled(); - }); -}); From 769b516032d40388c3e217e014b4aaf8694cfb4a Mon Sep 17 00:00:00 2001 From: eliran goshen Date: Tue, 7 Jul 2026 13:24:44 +0200 Subject: [PATCH 5/5] Address review feedback: clarify comments and document leader-election self-heal - Reword the cap-trim comment so it no longer implies `changed` only means "removed" - Simplify the re-add comment: drop disk-hydration jargon, describe any stale write - Document the active-clients self-heal in SEQUENTIAL_QUEUE.md leader-election section Co-Authored-By: Claude Opus 4.8 (1M context) --- contributingGuides/SEQUENTIAL_QUEUE.md | 2 ++ src/libs/ActiveClientManager/index.ts | 9 ++++----- 2 files changed, 6 insertions(+), 5 deletions(-) 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 2c4943725f28..a68699ecf21d 100644 --- a/src/libs/ActiveClientManager/index.ts +++ b/src/libs/ActiveClientManager/index.ts @@ -45,17 +45,16 @@ Onyx.connectWithoutView({ activeClients = val; - // Remove from the beginning of the list any clients that are past the limit, to avoid having thousands of them + // Trim clients past the limit from the front so the list can't grow unbounded let changed = false; while (activeClients.length >= maxClients) { activeClients.shift(); changed = true; } - // A late disk-hydration event can overwrite the list and drop this live client, handing - // leadership to a stale/ghost GUID and stalling the SequentialQueue. Re-append ourselves when a - // stale write dropped us (we were never in the incoming list) — but not when the cap trim above - // intentionally evicted us, which would otherwise thrash leadership with 20+ open tabs. + // 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;