From dd82e39dc9784d59f8237e143771a68707083e2b Mon Sep 17 00:00:00 2001 From: eliran goshen Date: Thu, 14 May 2026 16:23:23 +0200 Subject: [PATCH 1/2] fix --- lib/storage/index.ts | 32 ++----- .../storage/tryOrDegradePerformanceTest.ts | 89 +++++++++++++++++++ 2 files changed, 98 insertions(+), 23 deletions(-) create mode 100644 tests/unit/storage/tryOrDegradePerformanceTest.ts diff --git a/lib/storage/index.ts b/lib/storage/index.ts index c143c8e85..b68af3a0f 100644 --- a/lib/storage/index.ts +++ b/lib/storage/index.ts @@ -29,30 +29,16 @@ function degradePerformance(error: Error) { * Runs a piece of code and degrades performance if certain errors are thrown */ function tryOrDegradePerformance(fn: () => Promise | T, waitForInitialization = true): Promise { - return new Promise((resolve, reject) => { - const promise = waitForInitialization ? initPromise : Promise.resolve(); - - promise.then(() => { - try { - resolve(fn()); - } catch (error) { - // Test for known critical errors that the storage provider throws, e.g. when storage is full - if (error instanceof Error) { - // IndexedDB error when storage is full (https://github.com/Expensify/App/issues/29403) - if (error.message.includes('Internal error opening backing store for indexedDB.open')) { - degradePerformance(error); - } - - // catch the error if DB connection can not be established/DB can not be created - if (error.message.includes('IDBKeyVal store could not be created')) { - degradePerformance(error); - } - } - - reject(error); + const initialization = waitForInitialization ? initPromise : Promise.resolve(); + return initialization.then(() => + Promise.resolve(fn()).catch((error: unknown) => { + // catch the error if DB connection can not be established/DB can not be created + if (error instanceof Error && error.message.includes('IDBKeyVal store could not be created')) { + degradePerformance(error); } - }); - }); + return Promise.reject(error); + }), + ); } const storage: Storage = { diff --git a/tests/unit/storage/tryOrDegradePerformanceTest.ts b/tests/unit/storage/tryOrDegradePerformanceTest.ts new file mode 100644 index 000000000..444003aa4 --- /dev/null +++ b/tests/unit/storage/tryOrDegradePerformanceTest.ts @@ -0,0 +1,89 @@ +import type * as LoggerModule from '../../../lib/Logger'; +import type storageModule from '../../../lib/storage'; + +// `jestSetup.js` globally mocks `lib/storage`; this suite tests the real implementation. +jest.unmock('../../../lib/storage'); + +type Storage = typeof storageModule; +type Logger = typeof LoggerModule; +type LoggerCallback = Parameters[0]; +type LogData = Parameters[0]; + +type IsolatedModules = { + storage: Storage; + Logger: Logger; +}; + +type CapturedLog = {level: string; message: string}; + +/** + * Load a fresh copy of `lib/storage` (and its `Logger` dependency) in an isolated + * module registry so the module-private `provider` state in `lib/storage/index.ts` + * does not leak between tests. + */ +function loadIsolatedStorage(): IsolatedModules { + let storage!: Storage; + let Logger!: Logger; + + jest.isolateModules(() => { + Logger = require('../../../lib/Logger'); + storage = require('../../../lib/storage').default; + }); + + return {storage, Logger}; +} + +function noop() { + // intentionally empty +} + +describe('storage/tryOrDegradePerformance', () => { + // Fake timers cause the init promise chain to hang. + beforeAll(() => jest.useRealTimers()); + + let consoleErrorSpy: jest.SpyInstance; + + beforeEach(() => { + // `degradePerformance` calls `console.error` — silence it to keep test output clean. + consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(noop); + }); + + afterEach(() => { + consoleErrorSpy.mockRestore(); + }); + + it('falls back to MemoryOnlyProvider when a storage op rejects asynchronously with "IDBKeyVal store could not be created"', async () => { + const {storage, Logger} = loadIsolatedStorage(); + const capturedLogs: CapturedLog[] = []; + Logger.registerLogger((data: LogData) => capturedLogs.push({level: data.level, message: data.message})); + + storage.init(); + + const originalProvider = storage.getStorageProvider(); + const targetError = new Error('IDBKeyVal store could not be created'); + originalProvider.getAllKeys = jest.fn().mockReturnValue(Promise.reject(targetError)); + + await expect(storage.getAllKeys()).rejects.toBe(targetError); + + expect(capturedLogs.some((log) => log.level === 'hmmm' && log.message.includes('Falling back to only using cache'))).toBe(true); + expect(storage.getStorageProvider().name).toBe('MemoryOnlyProvider'); + }); + + it('propagates async rejections with unrelated messages without falling back', async () => { + const {storage, Logger} = loadIsolatedStorage(); + const capturedLogs: CapturedLog[] = []; + Logger.registerLogger((data: LogData) => capturedLogs.push({level: data.level, message: data.message})); + + storage.init(); + + const originalProvider = storage.getStorageProvider(); + const originalProviderName = originalProvider.name; + const unrelatedError = new Error('Some unrelated storage failure'); + originalProvider.getAllKeys = jest.fn().mockReturnValue(Promise.reject(unrelatedError)); + + await expect(storage.getAllKeys()).rejects.toBe(unrelatedError); + + expect(capturedLogs.some((log) => log.level === 'hmmm' && log.message.includes('Falling back to only using cache'))).toBe(false); + expect(storage.getStorageProvider().name).toBe(originalProviderName); + }); +}); From b131e24a4ae126fbd74d7cb2f39ef255a9ed64c4 Mon Sep 17 00:00:00 2001 From: eliran goshen Date: Mon, 18 May 2026 10:38:37 +0200 Subject: [PATCH 2/2] Address review: handle sync throws in tryOrDegradePerformance Per https://github.com/Expensify/react-native-onyx/pull/784#discussion_r3249353752, switch from `Promise.resolve(fn()).catch(...)` to `.then(() => fn()).catch(...)` so synchronous throws from `fn` are also routed through the degrade-detection catch handler. Add two test cases covering the sync-throw paths. --- lib/storage/index.ts | 8 ++-- .../storage/tryOrDegradePerformanceTest.ts | 39 +++++++++++++++++++ 2 files changed, 43 insertions(+), 4 deletions(-) diff --git a/lib/storage/index.ts b/lib/storage/index.ts index b68af3a0f..6dcd8e0bd 100644 --- a/lib/storage/index.ts +++ b/lib/storage/index.ts @@ -30,15 +30,15 @@ function degradePerformance(error: Error) { */ function tryOrDegradePerformance(fn: () => Promise | T, waitForInitialization = true): Promise { const initialization = waitForInitialization ? initPromise : Promise.resolve(); - return initialization.then(() => - Promise.resolve(fn()).catch((error: unknown) => { + return initialization + .then(() => fn()) + .catch((error: unknown) => { // catch the error if DB connection can not be established/DB can not be created if (error instanceof Error && error.message.includes('IDBKeyVal store could not be created')) { degradePerformance(error); } return Promise.reject(error); - }), - ); + }); } const storage: Storage = { diff --git a/tests/unit/storage/tryOrDegradePerformanceTest.ts b/tests/unit/storage/tryOrDegradePerformanceTest.ts index 444003aa4..b160b5903 100644 --- a/tests/unit/storage/tryOrDegradePerformanceTest.ts +++ b/tests/unit/storage/tryOrDegradePerformanceTest.ts @@ -86,4 +86,43 @@ describe('storage/tryOrDegradePerformance', () => { expect(capturedLogs.some((log) => log.level === 'hmmm' && log.message.includes('Falling back to only using cache'))).toBe(false); expect(storage.getStorageProvider().name).toBe(originalProviderName); }); + + it('falls back to MemoryOnlyProvider when a storage op throws synchronously with "IDBKeyVal store could not be created"', async () => { + const {storage, Logger} = loadIsolatedStorage(); + const capturedLogs: CapturedLog[] = []; + Logger.registerLogger((data: LogData) => capturedLogs.push({level: data.level, message: data.message})); + + storage.init(); + + const originalProvider = storage.getStorageProvider(); + const targetError = new Error('IDBKeyVal store could not be created'); + originalProvider.getAllKeys = jest.fn().mockImplementation(() => { + throw targetError; + }); + + await expect(storage.getAllKeys()).rejects.toBe(targetError); + + expect(capturedLogs.some((log) => log.level === 'hmmm' && log.message.includes('Falling back to only using cache'))).toBe(true); + expect(storage.getStorageProvider().name).toBe('MemoryOnlyProvider'); + }); + + it('propagates sync throws with unrelated messages without falling back', async () => { + const {storage, Logger} = loadIsolatedStorage(); + const capturedLogs: CapturedLog[] = []; + Logger.registerLogger((data: LogData) => capturedLogs.push({level: data.level, message: data.message})); + + storage.init(); + + const originalProvider = storage.getStorageProvider(); + const originalProviderName = originalProvider.name; + const unrelatedError = new Error('Some unrelated storage failure'); + originalProvider.getAllKeys = jest.fn().mockImplementation(() => { + throw unrelatedError; + }); + + await expect(storage.getAllKeys()).rejects.toBe(unrelatedError); + + expect(capturedLogs.some((log) => log.level === 'hmmm' && log.message.includes('Falling back to only using cache'))).toBe(false); + expect(storage.getStorageProvider().name).toBe(originalProviderName); + }); });