Skip to content

Commit bfb3c28

Browse files
committed
Merge remote-tracking branch 'origin/main' into elirangoshen/fix/retry-side-effects-idempotency
# Conflicts: # tests/unit/onyxUtilsTest.ts
2 parents 8b18535 + 14be5b8 commit bfb3c28

16 files changed

Lines changed: 1796 additions & 105 deletions

jestSetup.js

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ jest.mock('./lib/storage/platforms/index', () => require('./lib/storage/__mocks_
55
jest.mock('react-native-device-info', () => ({getFreeDiskStorage: () => {}}));
66
jest.mock('react-native-nitro-sqlite', () => ({
77
open: () => ({execute: () => {}}),
8-
enableSimpleNullHandling: () => undefined,
98
}));
109

1110
jest.useRealTimers();

lib/OnyxSnapshotCache.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,9 +62,9 @@ class OnyxSnapshotCache {
6262
* Other options like `reuseConnection` don't affect the data transformation
6363
* or timing behavior of getSnapshot, so they're excluded from the cache key for better cache hit rates.
6464
*/
65-
registerConsumer<TKey extends OnyxKey, TReturnValue>(options: Pick<UseOnyxOptions<TKey, TReturnValue>, 'selector'>): string {
65+
registerConsumer<TKey extends OnyxKey, TReturnValue>(key: TKey, options: Pick<UseOnyxOptions<TKey, TReturnValue>, 'selector'>): string {
6666
const selectorID = options?.selector ? this.getSelectorID(options.selector) : 'no_selector';
67-
const cacheKey = String(selectorID);
67+
const cacheKey = `${key}_${selectorID}`;
6868

6969
// Increment reference count for this cache key
7070
const currentCount = this.cacheKeyRefCounts.get(cacheKey) || 0;

lib/OnyxUtils.ts

Lines changed: 28 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -347,9 +347,10 @@ function multiGet<TKey extends OnyxKey>(keys: CollectionKeyBase[]): Promise<Map<
347347
continue;
348348
}
349349

350-
const cacheValue = cache.get(key) as OnyxValue<TKey>;
351-
if (cacheValue) {
352-
dataMap.set(key, cacheValue);
350+
// hasCacheForKey catches cached falsy values (0, '', false, null) as cache hits, which
351+
// a truthy check on the value would miss.
352+
if (cache.hasCacheForKey(key)) {
353+
dataMap.set(key, cache.get(key) as OnyxValue<TKey>);
353354
continue;
354355
}
355356

@@ -401,6 +402,13 @@ function multiGet<TKey extends OnyxKey>(keys: CollectionKeyBase[]): Promise<Map<
401402
}
402403
}
403404

405+
// Prefer cache over stale storage if a concurrent write populated it during
406+
// the read — otherwise cache.merge(temp) below would resurrect dropped fields.
407+
if (cache.hasCacheForKey(key)) {
408+
dataMap.set(key, cache.get(key) as OnyxValue<TKey>);
409+
continue;
410+
}
411+
404412
dataMap.set(key, value as OnyxValue<TKey>);
405413
temp[key] = value as OnyxValue<TKey>;
406414
}
@@ -787,8 +795,12 @@ function remove<TKey extends OnyxKey>(key: TKey, isProcessingCollectionUpdate?:
787795

788796
function reportStorageQuota(error?: Error): Promise<void> {
789797
return Storage.getDatabaseSize()
790-
.then(({bytesUsed, bytesRemaining}) => {
791-
Logger.logInfo(`Storage Quota Check -- bytesUsed: ${bytesUsed} bytesRemaining: ${bytesRemaining}. Original error: ${error}`);
798+
.then(({bytesUsed, bytesRemaining, usageDetails}) => {
799+
Logger.logInfo(
800+
`Storage Quota Check -- bytesUsed: ${bytesUsed} bytesRemaining: ${bytesRemaining}${
801+
usageDetails ? ` usageDetails: ${JSON.stringify(usageDetails)}` : ''
802+
}. Original error: ${error}`,
803+
);
792804
})
793805
.catch((dbSizeError) => {
794806
Logger.logAlert(`Unable to get database size. getDatabaseSize error: ${dbSizeError}. Original error: ${error}`);
@@ -1627,12 +1639,17 @@ function mergeCollectionWithPatches<TKey extends CollectionKeyBase>(
16271639
// finalMergedCollection contains all the keys that were merged, without the keys of incompatible updates
16281640
const finalMergedCollection = {...existingKeyCollection, ...newCollection};
16291641

1630-
// Pre-warm cache for any existing storage keys that aren't yet in cache. get() is a no-op
1631-
// (sync-resolved) for cache hits, and on a cache miss it reads from storage and writes the
1632-
// value back to cache. This is required so the subsequent cache.merge() merges the new delta
1633-
// into the real previous storage value (rather than starting from `undefined` and dropping
1634-
// the existing keys).
1635-
return Promise.all(existingKeys.map((key) => get(key))).then(() => {
1642+
// Pre-warm cache for cache-miss existingKeys so cache.merge() merges the new delta into
1643+
// the real previous storage value. Fast path (all warm) skips the pre-warm to preserve
1644+
// promise-chain depth; slow path batches the misses into one Storage.multiGet.
1645+
const hasColdExistingKey = existingKeys.some((key) => !cache.hasCacheForKey(key));
1646+
// Swallow pre-warm read failures so a transient Storage.multiGet rejection doesn't
1647+
// skip the cache.merge() + keysChanged() below. Subscribers still see the merge even
1648+
// when storage reads fail.
1649+
const prewarmPromise = hasColdExistingKey
1650+
? multiGet(existingKeys).catch((err) => Logger.logInfo(`mergeCollectionWithPatches pre-warm failed; proceeding with cache-only merge. Error: ${err}`))
1651+
: Promise.resolve();
1652+
return prewarmPromise.then(() => {
16361653
// Snapshot previous values from the (now-warm) cache for keysChanged's diff, then update
16371654
// cache and notify subscribers synchronously BEFORE issuing storage writes. This matches
16381655
// the cache-first / storage-second invariant followed by every other Onyx write method

lib/storage/providers/IDBKeyValProvider/createStore.ts

Lines changed: 113 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,50 @@ import * as IDB from 'idb-keyval';
22
import type {UseStore} from 'idb-keyval';
33
import * as Logger from '../../../Logger';
44

5+
const HEAL_ATTEMPTS_MAX = 3;
6+
7+
/**
8+
* Detects the Chromium-specific IDB backing store corruption error.
9+
* Fires when LevelDB files backing IndexedDB are corrupted and Chrome's
10+
* internal recovery (RepairDB -> delete -> recreate) also fails.
11+
*/
12+
function isBackingStoreError(error: unknown): boolean {
13+
return (error instanceof Error || error instanceof DOMException) && (error as Error).message.includes('Internal error opening backing store');
14+
}
15+
16+
/**
17+
* Detects Safari/WebKit IDB connection termination errors.
18+
* Fires when Safari kills the IDB server process for backgrounded tabs.
19+
* WebKit bugs: https://bugs.webkit.org/show_bug.cgi?id=197050, https://bugs.webkit.org/show_bug.cgi?id=201483
20+
*/
21+
function isConnectionLostError(error: unknown): boolean {
22+
if (!(error instanceof Error || error instanceof DOMException)) return false;
23+
const msg = (error as Error).message.toLowerCase();
24+
return msg.includes('connection to indexed database server lost') || msg.includes('connection is closing');
25+
}
26+
27+
function isInvalidStateError(error: unknown): boolean {
28+
return (error instanceof Error || error instanceof DOMException) && (error as Error).name === 'InvalidStateError';
29+
}
30+
31+
/** Errors that trigger a budgeted heal-and-retry in store(). */
32+
function isBudgetedHealError(error: unknown): boolean {
33+
return isBackingStoreError(error) || isConnectionLostError(error);
34+
}
35+
36+
function getBudgetedHealErrorLabel(error: unknown): string {
37+
if (isBackingStoreError(error)) return 'backing store';
38+
if (isConnectionLostError(error)) return 'connection lost';
39+
return 'unknown';
40+
}
41+
542
// This is a copy of the createStore function from idb-keyval, we need a custom implementation
643
// because we need to create the database manually in order to ensure that the store exists before we use it.
744
// If the store does not exist, idb-keyval will throw an error
845
// source: https://github.com/jakearchibald/idb-keyval/blob/9d19315b4a83897df1e0193dccdc29f78466a0f3/src/index.ts#L12
946
function createStore(dbName: string, storeName: string): UseStore {
1047
let dbp: Promise<IDBDatabase> | undefined;
48+
let healAttemptsRemaining = HEAL_ATTEMPTS_MAX;
1149

1250
const attachHandlers = (db: IDBDatabase) => {
1351
// Browsers may close idle IDB connections at any time, especially Safari.
@@ -30,18 +68,27 @@ function createStore(dbName: string, storeName: string): UseStore {
3068
};
3169
};
3270

71+
// Cache the open promise and attach handlers + rejection cleanup.
72+
// On rejection, clears dbp so the next operation retries with a fresh indexedDB.open()
73+
// instead of returning the same rejected promise.
74+
// Guard: only clear if dbp hasn't been replaced by a concurrent heal/retry.
75+
function cacheOpenPromise(openPromise: Promise<IDBDatabase>) {
76+
dbp = openPromise;
77+
const currentPromise = openPromise;
78+
openPromise.then(attachHandlers, () => {
79+
if (dbp !== currentPromise) {
80+
return;
81+
}
82+
dbp = undefined;
83+
});
84+
return openPromise;
85+
}
86+
3387
const getDB = () => {
3488
if (dbp) return dbp;
3589
const request = indexedDB.open(dbName);
3690
request.onupgradeneeded = () => request.result.createObjectStore(storeName);
37-
dbp = IDB.promisifyRequest(request);
38-
39-
dbp.then(
40-
attachHandlers,
41-
// eslint-disable-next-line @typescript-eslint/no-empty-function
42-
() => {},
43-
);
44-
return dbp;
91+
return cacheOpenPromise(IDB.promisifyRequest(request));
4592
};
4693

4794
// Ensures the store exists in the DB. If missing, bumps the version to trigger
@@ -66,10 +113,7 @@ function createStore(dbName: string, storeName: string): UseStore {
66113
updatedDatabase.createObjectStore(storeName);
67114
};
68115

69-
dbp = IDB.promisifyRequest(request);
70-
// eslint-disable-next-line @typescript-eslint/no-empty-function
71-
dbp.then(attachHandlers, () => {});
72-
return dbp;
116+
return cacheOpenPromise(IDB.promisifyRequest(request));
73117
};
74118

75119
function executeTransaction<T>(txMode: IDBTransactionMode, callback: (store: IDBObjectStore) => T | PromiseLike<T>): Promise<T> {
@@ -78,23 +122,64 @@ function createStore(dbName: string, storeName: string): UseStore {
78122
.then((db) => callback(db.transaction(storeName, txMode).objectStore(storeName)));
79123
}
80124

81-
// If the connection was closed between getDB() resolving and db.transaction() executing,
82-
// the transaction throws InvalidStateError. We catch it and retry once with a fresh connection.
125+
function resetHealBudget<T>(result: T): T {
126+
healAttemptsRemaining = HEAL_ATTEMPTS_MAX;
127+
return result;
128+
}
129+
130+
// Handles three recoverable error classes:
131+
// 1. InvalidStateError — connection closed between getDB() resolving and db.transaction().
132+
// Retry once with a fresh connection. No budget limit (transient, always worth one reopen).
133+
// 2. Backing store corruption (Chromium UnknownError) — drop cached connection and reopen.
134+
// 3. Connection lost (Safari UnknownError) — IDB server terminated for backgrounded tabs.
135+
// Both 2 and 3 share a heal budget (3 attempts, reset on success).
136+
// Mirrors Dexie's PR1398_maxLoop pattern: https://github.com/dexie/Dexie.js/blob/master/src/functions/temp-transaction.ts
137+
// Note: concurrent store() calls share the budget. Under overlapping failures each caller
138+
// decrements independently, so the budget may drain faster than one-per-incident. This is
139+
// acceptable — same as Dexie's approach — and the budget resets on any success.
83140
return (txMode, callback) =>
84-
executeTransaction(txMode, callback).catch((error) => {
85-
if (error instanceof DOMException && error.name === 'InvalidStateError') {
86-
Logger.logAlert('IDB InvalidStateError, retrying with fresh connection', {
87-
dbName,
88-
storeName,
89-
txMode,
90-
errorMessage: error.message,
91-
});
92-
dbp = undefined;
93-
// Retry only once — this call is not wrapped, so if it also fails the error propagates normally.
94-
return executeTransaction(txMode, callback);
95-
}
96-
throw error;
97-
});
141+
executeTransaction(txMode, callback)
142+
.then(resetHealBudget)
143+
.catch((error) => {
144+
if (isInvalidStateError(error)) {
145+
Logger.logInfo('IDB InvalidStateError — dropping cached connection and retrying', {
146+
dbName,
147+
storeName,
148+
txMode,
149+
errorMessage: error instanceof Error ? error.message : String(error),
150+
});
151+
dbp = undefined;
152+
return executeTransaction(txMode, callback).then(resetHealBudget);
153+
}
154+
155+
if (isBudgetedHealError(error) && healAttemptsRemaining > 0) {
156+
healAttemptsRemaining--;
157+
const label = getBudgetedHealErrorLabel(error);
158+
Logger.logInfo(`IDB heal: ${label} error detected — dropping cached connection and reopening (${healAttemptsRemaining} attempts left)`, {
159+
dbName,
160+
storeName,
161+
});
162+
dbp = undefined;
163+
return executeTransaction(txMode, callback).then((result) => {
164+
Logger.logInfo(`IDB heal: successfully recovered after ${label} error`, {dbName, storeName});
165+
return resetHealBudget(result);
166+
});
167+
}
168+
169+
if (isBudgetedHealError(error)) {
170+
Logger.logAlert(`IDB heal: ${getBudgetedHealErrorLabel(error)} error — heal budget exhausted, giving up`, {
171+
dbName,
172+
storeName,
173+
});
174+
} else {
175+
Logger.logAlert('IDB error is not recoverable, giving up', {
176+
dbName,
177+
storeName,
178+
errorMessage: error instanceof Error ? error.message : String(error),
179+
});
180+
}
181+
throw error;
182+
});
98183
}
99184

100185
export default createStore;

lib/storage/providers/IDBKeyValProvider/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,7 @@ const provider: StorageProvider<UseStore | undefined> = {
156156
.then((value) => ({
157157
bytesUsed: value.usage ?? 0,
158158
bytesRemaining: (value.quota ?? 0) - (value.usage ?? 0),
159+
usageDetails: (value as StorageEstimate & {usageDetails?: Record<string, number>}).usageDetails,
159160
}))
160161
.catch((error) => {
161162
throw new Error(`Unable to estimate web storage quota. Original error: ${error}`);

lib/storage/providers/types.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ type StorageKeyList = OnyxKey[];
77
type DatabaseSize = {
88
bytesUsed: number;
99
bytesRemaining: number;
10+
usageDetails?: Record<string, number>;
1011
};
1112

1213
type OnStorageKeyChanged = <TKey extends OnyxKey>(key: TKey, value: OnyxValue<TKey>) => void;

lib/useOnyx.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -124,10 +124,10 @@ function useOnyx<TKey extends OnyxKey, TReturnValue = OnyxValue<TKey>>(
124124
// Cache the options key to avoid regenerating it every getSnapshot call
125125
const cacheKey = useMemo(
126126
() =>
127-
onyxSnapshotCache.registerConsumer({
127+
onyxSnapshotCache.registerConsumer(key, {
128128
selector: options?.selector,
129129
}),
130-
[options?.selector],
130+
[key, options?.selector],
131131
);
132132

133133
useEffect(() => () => onyxSnapshotCache.deregisterConsumer(key, cacheKey), [key, cacheKey]);

0 commit comments

Comments
 (0)