Skip to content

Commit 6944ea4

Browse files
adhorodyskiclaude
andcommitted
SequentialQueue: don't block the queue when a persist write fails
The conflict-resolution persist paths (update / deleteRequestsByIndices) returned a raw Onyx.set that could reject. Awaiting that in push() before flush() stranded isReadyPromise on a disk-write failure, hanging every READ parked on waitForIdle() and never flushing the request. These paths now swallow the rejection like save() already did — the in-memory queue stays the source of truth for retry — and surface it via Log.alert so a disk-write failure is observable rather than silent. push() keeps a defensive try/catch so a persist rejection can never wedge the queue; on failure it flushes best-effort, matching pre-async behavior. Adds a regression test that fails on the pre-fix code. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent d3b7692 commit 6944ea4

3 files changed

Lines changed: 70 additions & 7 deletions

File tree

src/libs/Network/SequentialQueue.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -584,7 +584,13 @@ async function push<TKey extends OnyxKey>(newRequest: OnyxRequest<TKey>): Promis
584584

585585
// Block until the Onyx disk commit lands so flush() → XHR cannot race the disk write —
586586
// a process kill in that window would lose the request on next launch.
587-
await persistencePromise;
587+
try {
588+
await persistencePromise;
589+
} catch {
590+
// Backstop: persistence alerts+swallows on failure, so this shouldn't reject. If it ever does,
591+
// flush anyway (the request is already in the in-memory queue) rather than stranding isReadyPromise.
592+
Log.info('[SequentialQueue] Persist rejected — flushing anyway', false, {command: newRequest.command});
593+
}
588594

589595
// The network may have flipped offline while we awaited the disk write. flush() would
590596
// early-return on its offline check without resolving isReadyPromise, leaving READs parked

src/libs/actions/PersistedRequests.ts

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -260,10 +260,12 @@ function save<TKey extends OnyxKey>(requestToPersist: Request<TKey>): Promise<vo
260260
queueLength: getLength(),
261261
});
262262
})
263-
.catch(() => {
264-
Log.info('[PersistedRequests] ERROR: Failed to persist request to disk', false, {
263+
.catch((error) => {
264+
// Disk-write failure risks losing this request on a crash — alert as a storage emergency.
265+
Log.alert('[PersistedRequests] ERROR: Failed to persist request to disk', {
265266
command: requestToPersist.command,
266267
queueLength: getLength(),
268+
error,
267269
});
268270
});
269271
}
@@ -334,9 +336,18 @@ function deleteRequestsByIndices(indices: number[]): Promise<void> {
334336
persistedRequests = persistedRequests.filter((_, index) => !indicesSet.has(index));
335337

336338
// Update the persisted requests in storage or state as necessary
337-
return trackOnyxWrite(Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequests)).then(() => {
338-
Log.info(`Multiple (${indices.length}) requests removed from the queue. Queue length is ${persistedRequests.length}`);
339-
});
339+
return trackOnyxWrite(Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, persistedRequests))
340+
.then(() => {
341+
Log.info(`Multiple (${indices.length}) requests removed from the queue. Queue length is ${persistedRequests.length}`);
342+
})
343+
.catch((error) => {
344+
// Swallow so the conflict promise resolves (in-memory queue is the source of truth); alert as a storage emergency.
345+
Log.alert('[PersistedRequests] ERROR: Failed to persist request deletion to disk', {
346+
indicesCount: indices.length,
347+
queueLength: getLength(),
348+
error,
349+
});
350+
});
340351
}
341352

342353
function update<TKey extends OnyxKey>(oldRequestIndex: number, newRequest: Request<TKey>): Promise<void> {
@@ -349,7 +360,14 @@ function update<TKey extends OnyxKey>(oldRequestIndex: number, newRequest: Reque
349360
if (requestIndex != null) {
350361
knownRequestIDs.add(requestIndex);
351362
}
352-
return trackOnyxWrite(Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, requests));
363+
return trackOnyxWrite(Onyx.set(ONYXKEYS.PERSISTED_REQUESTS, requests)).catch((error) => {
364+
// Swallow so the conflict promise resolves (in-memory queue is the source of truth); alert as a storage emergency.
365+
Log.alert('[PersistedRequests] ERROR: Failed to persist updated request to disk', {
366+
command: newRequest.command,
367+
queueLength: getLength(),
368+
error,
369+
});
370+
});
353371
}
354372

355373
function shouldPersistOngoingRequest(request: AnyRequest | null): boolean {

tests/unit/SequentialQueueTest.ts

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,45 @@ describe('SequentialQueue', () => {
7474
}
7575
});
7676

77+
it('should not block the queue when a disk write fails during persist', async () => {
78+
// If a conflict-resolution disk write rejects (storage full / corruption), push() must not throw
79+
// or strand isReadyPromise — the request should still flush and waitForIdle() should resolve.
80+
const mockedFetch = global.fetch as ReturnType<typeof TestHelper.getGlobalFetchMock> & {pause: () => void; resume: () => Promise<void>};
81+
const originalSet = Onyx.set.bind(Onyx);
82+
83+
mockedFetch.pause();
84+
try {
85+
await SequentialQueue.push({command: 'OpenReport'}); // occupies ongoingRequest
86+
await waitForBatchedUpdates();
87+
await SequentialQueue.push(request); // ReconnectApp stacks in the queue
88+
89+
// Fail the conflict-resolution persist (a raw Onyx.set on the persisted-requests key).
90+
const setMock = jest
91+
.spyOn(Onyx, 'set')
92+
.mockImplementation((key, value) => (key === ONYXKEYS.PERSISTED_REQUESTS ? Promise.reject(new Error('simulated disk-write failure')) : originalSet(key, value)));
93+
try {
94+
const replacing: Request<never> = {
95+
command: 'ReconnectApp',
96+
data: {accountID: 56789},
97+
checkAndFixConflictingRequest: (persistedRequests) => {
98+
const index = persistedRequests.findIndex((r) => r.command === 'ReconnectApp');
99+
return {conflictAction: index === -1 ? {type: 'push'} : {type: 'replace', index}};
100+
},
101+
};
102+
// The failed disk write must not reject the caller.
103+
await expect(SequentialQueue.push(replacing)).resolves.toBeUndefined();
104+
} finally {
105+
setMock.mockRestore();
106+
}
107+
} finally {
108+
await mockedFetch.resume();
109+
}
110+
111+
// The queue still drains and READs unblock — a hang here would fail the test by timing out.
112+
await SequentialQueue.waitForIdle();
113+
expect(getLength()).toBe(0);
114+
});
115+
77116
it('should push two requests with conflict resolution and replace', async () => {
78117
// Pause the queue so `process()` does not consume the first request before
79118
// the conflict resolver runs. Under persist-before-fire `push()` is async,

0 commit comments

Comments
 (0)