diff --git a/mobile/src/cache/session-tab-strip-cache.test.ts b/mobile/src/cache/session-tab-strip-cache.test.ts index 241636c6401..4d6a4ae9acf 100644 --- a/mobile/src/cache/session-tab-strip-cache.test.ts +++ b/mobile/src/cache/session-tab-strip-cache.test.ts @@ -275,6 +275,26 @@ describe('session tab strip cache', () => { expect(asyncStorage.removeItem).toHaveBeenCalledWith(LEGACY_STORAGE_KEY) }) + it('retries a failed legacy removal on a later load, even with no write in between', async () => { + // Why: an offline session only ever loads; the memoized file read must not be the + // only place the retry lives. + const key = getSessionTabStripCacheKey('host-1', 'wt-1') + asyncStorage.removeItem.mockRejectedValueOnce(new Error('bridge down')) + await loadCachedSessionTabStrip(key) + expect(asyncStorage.removeItem).toHaveBeenCalledTimes(1) + await loadCachedSessionTabStrip(key) + expect(asyncStorage.removeItem).toHaveBeenCalledTimes(2) + await loadCachedSessionTabStrip(key) + expect(asyncStorage.removeItem).toHaveBeenCalledTimes(2) + }) + + it('does not report a host forgotten while its legacy blob is still on disk', async () => { + asyncStorage.removeItem.mockRejectedValue(new Error('bridge down')) + await expect(deleteCachedSessionTabStripForHost('host-1')).rejects.toThrow(/bridge down/) + asyncStorage.removeItem.mockResolvedValue(undefined) + await expect(deleteCachedSessionTabStripForHost('host-1')).resolves.toBeUndefined() + }) + it('retries a failed legacy removal on the next write, and stops once it lands', async () => { const key = getSessionTabStripCacheKey('host-1', 'wt-1') asyncStorage.removeItem.mockRejectedValueOnce(new Error('bridge down')) diff --git a/mobile/src/cache/session-tab-strip-cache.ts b/mobile/src/cache/session-tab-strip-cache.ts index d79c4388aa4..c6a97ba5542 100644 --- a/mobile/src/cache/session-tab-strip-cache.ts +++ b/mobile/src/cache/session-tab-strip-cache.ts @@ -79,6 +79,9 @@ export async function loadCachedSessionTabStrip( if (!key) { return null } + // Every load, not just the first file read: a removal that failed on launch must be + // retried by an offline session that only ever loads. + removeLegacyBlobsBestEffort() const cache = await loadFile() return cache.get(key) ?? null } @@ -136,6 +139,9 @@ export async function deleteCachedSessionTabStripForHost(hostId: string): Promis } // Queued, not raced: the purge is the last write, and its failure is the caller's. await enqueueWrite(cache) + // The forgotten host's titles may still sit in the blob an older build wrote. A deletion + // the user asked for is not done until that is gone too, so this one is awaited and thrown. + await removeLegacyBlobs() } export function resetSessionTabStripCacheForTests(): void { @@ -193,19 +199,24 @@ async function loadFile(): Promise> { return loadPromise } -// Not awaited: the plaintext left by an older build must go, but a failed removal is no reason -// to withhold the strip this build can draw. A failure keeps the key queued for the next try. -function removeLegacyBlobs(): void { - for (const key of pendingLegacyRemovals) { - void AsyncStorage.removeItem(key).then( - () => pendingLegacyRemovals.delete(key), - () => {} +// Loads and ordinary writes do not await this: the plaintext left by an older build must go, +// but a failed removal is no reason to withhold the strip this build can draw. A failure keeps +// the key queued for the next try. A host deletion does await it, and throws on failure. +function removeLegacyBlobs(): Promise { + return Promise.all( + [...pendingLegacyRemovals].map((key) => + AsyncStorage.removeItem(key).then(() => { + pendingLegacyRemovals.delete(key) + }) ) - } + ).then(() => {}) +} + +function removeLegacyBlobsBestEffort(): void { + void removeLegacyBlobs().catch(() => {}) } async function readStoredFile(): Promise { - removeLegacyBlobs() try { const raw = await AsyncStorage.getItem(STORAGE_KEY) if (!raw) { @@ -250,7 +261,7 @@ function enqueueWrite(cache: Map): Promise } async function writeFile(cache: Map): Promise { - removeLegacyBlobs() + removeLegacyBlobsBestEffort() const workspaces: StoredWorkspace[] = [...cache].map(([key, preview]) => ({ key, preview })) // Throws on purpose: a deletion that only removed the in-memory rows must not be // reported as a deletion, or the forgotten host's titles stay in plaintext on disk.