diff --git a/README.md b/README.md index 1182c64..9e9d1d7 100644 --- a/README.md +++ b/README.md @@ -477,6 +477,7 @@ doc({ autosave: 1000, readOnly: false, retryOnError: false, + maxWriteRetries: 3, }) col({ @@ -506,6 +507,7 @@ const projectDoc = defineDocument({ readOnly: false, // Optional: prevent updates retryOnError: false, // Optional: retry on listener errors retryInterval: 5000, // Optional: retry interval (ms) + maxWriteRetries: 3, // Optional: max retries on transient write failures schema: ProjectSchema, // Optional: Zod schema (validates set/add) }) ``` diff --git a/src/collection.ts b/src/collection.ts index eac9e5d..3bad1b9 100644 --- a/src/collection.ts +++ b/src/collection.ts @@ -149,6 +149,7 @@ export const createCollectionSubscription = ( queryConstraints: definitionConstraints, retryOnError = false, retryInterval = 5000, + maxWriteRetries = 3, schema, } = definition @@ -182,6 +183,8 @@ export const createCollectionSubscription = ( let autosaveTimeout: ReturnType | null = null let minLoadTimeout: ReturnType | null = null let retryTimeout: ReturnType | null = null + let writeRetryCount = 0 + let writeRetryTimeout: ReturnType | null = null let minLoadTimeElapsed = false let loaded = false // Cached handle — returns the same reference until notify() invalidates @@ -279,7 +282,7 @@ export const createCollectionSubscription = ( state.localState = newLocalState notify() - scheduleAutosave() + scheduleAutosave(true) } // Overloaded: callers can pass (id, data, opts) or (data, opts). The @@ -354,7 +357,7 @@ export const createCollectionSubscription = ( state.localState = newLocalState notify() - scheduleAutosave() + scheduleAutosave(true) return id } @@ -392,13 +395,26 @@ export const createCollectionSubscription = ( state.localState = newLocalState notify() - scheduleAutosave() + scheduleAutosave(true) } - const scheduleAutosave = () => { + // `fromMutation` is true when called from a user edit (updateState, + // addDocument, removeDocument). Only in that case do we cancel the + // pending write-retry timer and reset the counter — a new user edit + // supersedes the previous attempt and starts with a fresh retry budget. + // Snapshot-driven calls (handleSnapshot) must NOT reset the budget so a + // stray read from the listener doesn't silently swallow a retry. + const scheduleAutosave = (fromMutation = false) => { if (autosaveTimeout) { clearTimeout(autosaveTimeout) } + if (fromMutation) { + if (writeRetryTimeout) { + clearTimeout(writeRetryTimeout) + writeRetryTimeout = null + } + writeRetryCount = 0 + } if (autosave > 0) { autosaveTimeout = setTimeout(() => { sync() @@ -459,13 +475,22 @@ export const createCollectionSubscription = ( await batch.commit() } catch (error) { - console.error('Collection sync failed:', error) state.waitingForUpdate = false state.inflightLocalState = undefined - // Surface to React: handle.error reflects the failure and the - // listener will keep state.localState so consumers can retry by - // calling sync(). Autosave is not automatically rescheduled to - // avoid retry loops on permission errors. + if (writeRetryCount < maxWriteRetries) { + writeRetryCount++ + console.warn(`[firestate] Write failed (attempt ${writeRetryCount}/${maxWriteRetries}), retrying in ${retryInterval}ms:`, error) + writeRetryTimeout = setTimeout(() => { + writeRetryTimeout = null + sync() + }, retryInterval) + return + } + // All retries exhausted — surface the error to React and the + // onError handler. localState is preserved so the caller can + // inspect the pending change or call sync() to retry manually. + writeRetryCount = 0 + console.error('Collection sync failed:', error) state.error = error as Error store.reportError(error as Error, { type: 'collection', @@ -488,6 +513,11 @@ export const createCollectionSubscription = ( if (state.waitingForUpdate) { state.waitingForUpdate = false + writeRetryCount = 0 + if (writeRetryTimeout) { + clearTimeout(writeRetryTimeout) + writeRetryTimeout = null + } const inflightState = state.inflightLocalState state.inflightLocalState = undefined const currentLocal = state.localState @@ -605,6 +635,10 @@ export const createCollectionSubscription = ( clearTimeout(retryTimeout) retryTimeout = null } + if (writeRetryTimeout) { + clearTimeout(writeRetryTimeout) + writeRetryTimeout = null + } if (unsubscribeListener) { unsubscribeListener() unsubscribeListener = null diff --git a/src/document.ts b/src/document.ts index 03e2ad1..d2ec093 100644 --- a/src/document.ts +++ b/src/document.ts @@ -138,6 +138,7 @@ export const createDocumentSubscription = ( readOnly: definitionReadOnly, retryOnError = false, retryInterval = 5000, + maxWriteRetries = 3, schema, } = definition @@ -184,6 +185,8 @@ export const createDocumentSubscription = ( let autosaveTimeout: ReturnType | null = null let retryTimeout: ReturnType | null = null let minLoadTimeout: ReturnType | null = null + let writeRetryCount = 0 + let writeRetryTimeout: ReturnType | null = null let minLoadTimeElapsed = false let loaded = false // Cached handle — returns the same reference until notify() invalidates @@ -276,7 +279,7 @@ export const createDocumentSubscription = ( state.isSetOperation = false notify() - scheduleAutosave() + scheduleAutosave(true) } const setData = (data: TData, undoOptions: UpdateOptions = {}) => { @@ -324,7 +327,7 @@ export const createDocumentSubscription = ( state.isSetOperation = true notify() - scheduleAutosave() + scheduleAutosave(true) } const deleteDocument = (undoOptions: UpdateOptions = {}) => { @@ -352,13 +355,26 @@ export const createDocumentSubscription = ( state.isSetOperation = false notify() - scheduleAutosave() + scheduleAutosave(true) } - const scheduleAutosave = () => { + // `fromMutation` is true when called from a user edit (updateState, + // setData, deleteDocument). Only in that case do we cancel the pending + // write-retry timer and reset the counter — a new user edit supersedes + // the previous attempt and starts with a fresh retry budget. + // Snapshot-driven calls (handleSnapshot) must NOT reset the budget so a + // stray read from the listener doesn't silently swallow a retry. + const scheduleAutosave = (fromMutation = false) => { if (autosaveTimeout) { clearTimeout(autosaveTimeout) } + if (fromMutation) { + if (writeRetryTimeout) { + clearTimeout(writeRetryTimeout) + writeRetryTimeout = null + } + writeRetryCount = 0 + } if (autosave > 0) { autosaveTimeout = setTimeout(() => { sync() @@ -378,9 +394,19 @@ export const createDocumentSubscription = ( try { await deleteDoc(docRef) } catch (error) { - console.error('Sync failed:', error) state.waitingForUpdate = false state.inflightLocalState = undefined + if (writeRetryCount < maxWriteRetries) { + writeRetryCount++ + console.warn(`[firestate] Write failed (attempt ${writeRetryCount}/${maxWriteRetries}), retrying in ${retryInterval}ms:`, error) + writeRetryTimeout = setTimeout(() => { + writeRetryTimeout = null + sync() + }, retryInterval) + return + } + writeRetryCount = 0 + console.error('Sync failed:', error) state.error = error as Error store.reportError(error as Error, { type: 'document', @@ -433,14 +459,22 @@ export const createDocumentSubscription = ( await updateDoc(docRef, flatDiff) } } catch (error) { - console.error('Sync failed:', error) state.waitingForUpdate = false state.inflightLocalState = undefined - // Surface to React: handle.error reflects the failure and the - // listener will keep state.localState so consumers can retry by - // calling sync() or by issuing another update. Autosave is not - // automatically rescheduled to avoid retry loops on permission - // errors — that policy is left to the consumer. + if (writeRetryCount < maxWriteRetries) { + writeRetryCount++ + console.warn(`[firestate] Write failed (attempt ${writeRetryCount}/${maxWriteRetries}), retrying in ${retryInterval}ms:`, error) + writeRetryTimeout = setTimeout(() => { + writeRetryTimeout = null + sync() + }, retryInterval) + return + } + // All retries exhausted — surface the error to React and the + // onError handler. localState is preserved so the caller can + // inspect the pending change or call sync() to retry manually. + writeRetryCount = 0 + console.error('Sync failed:', error) state.error = error as Error store.reportError(error as Error, { type: 'document', @@ -458,6 +492,11 @@ export const createDocumentSubscription = ( if (state.waitingForUpdate) { state.waitingForUpdate = false + writeRetryCount = 0 + if (writeRetryTimeout) { + clearTimeout(writeRetryTimeout) + writeRetryTimeout = null + } const inflightState = state.inflightLocalState state.inflightLocalState = undefined const currentLocal = state.localState @@ -526,6 +565,11 @@ export const createDocumentSubscription = ( if (state.waitingForUpdate) { state.waitingForUpdate = false state.inflightLocalState = undefined + writeRetryCount = 0 + if (writeRetryTimeout) { + clearTimeout(writeRetryTimeout) + writeRetryTimeout = null + } } if (minLoadTimeElapsed) { state.isLoading = false @@ -598,6 +642,10 @@ export const createDocumentSubscription = ( clearTimeout(retryTimeout) retryTimeout = null } + if (writeRetryTimeout) { + clearTimeout(writeRetryTimeout) + writeRetryTimeout = null + } if (minLoadTimeout) { clearTimeout(minLoadTimeout) minLoadTimeout = null diff --git a/src/firestate.integration.test.ts b/src/firestate.integration.test.ts index 8c12c8e..cbb2e71 100644 --- a/src/firestate.integration.test.ts +++ b/src/firestate.integration.test.ts @@ -8,7 +8,7 @@ * is that those strings reach Firestore in `collection(firestore, path)`. * This file pins that contract directly. */ -import { vi, describe, it, expect, beforeEach } from 'vitest' +import { vi, describe, it, expect, beforeEach, afterEach } from 'vitest' vi.mock('firebase/firestore', async () => { const actual = @@ -26,6 +26,15 @@ vi.mock('firebase/firestore', async () => { onSnapshot: vi.fn(() => () => { /* noop unsubscribe */ }), + setDoc: vi.fn(), + updateDoc: vi.fn(), + deleteDoc: vi.fn(), + writeBatch: vi.fn(() => ({ + set: vi.fn(), + update: vi.fn(), + delete: vi.fn(), + commit: vi.fn(), + })), } }) @@ -39,6 +48,7 @@ import { buildCollectionDefinition, } from './firestate' import { createDocumentSubscription } from './document' +import { createCollectionSubscription } from './collection' import { createStore, type FirestateStore } from './store' const revisionSchema = z.object({ title: z.string() }) @@ -326,3 +336,207 @@ describe('Document subscription: serverTimestamp display overrides', () => { expect(sub.getState().data!.updatedAt).toBeInstanceOf(Timestamp) }) }) + +// Pins the fix for issue #25: write failures should be retried up to +// `maxWriteRetries` times before the error is surfaced to React. +describe('Write retry on transient failures', () => { + let store: FirestateStore + let snapshotCallback: ((snap: unknown) => void) | undefined + const schema = z.object({ title: z.string() }) + + beforeEach(() => { + vi.clearAllMocks() + vi.useFakeTimers() + store = createStore({ firestore: {} as any, autosave: 0 }) + + snapshotCallback = undefined + vi.mocked(firestore.onSnapshot).mockImplementation( + ((_ref: unknown, onNext: (snap: unknown) => void) => { + snapshotCallback = onNext + return () => { + snapshotCallback = undefined + } + }) as never + ) + }) + + afterEach(() => { + vi.useRealTimers() + }) + + const fireDocSnapshot = (data: Record | null) => { + snapshotCallback!({ + exists: () => data !== null, + data: () => data, + metadata: { fromCache: false, hasPendingWrites: false }, + }) + } + + it('does not surface an error until all retries are exhausted (document set)', async () => { + vi.mocked(firestore.setDoc).mockRejectedValue(new Error('transient')) + const onError = vi.fn() + store = createStore({ firestore: {} as any, autosave: 0, onError }) + + const sub = createDocumentSubscription({ + store, + definition: buildDocumentDefinition( + doc({ path: 'items/{id}', schema, maxWriteRetries: 2, retryInterval: 100 }) + ), + docId: 'i1', + collectionPath: 'items', + }) + sub.load() + + sub.getHandle().set({ title: 'new' }) + await sub.sync() + + // Attempt 1 failed — 2 retries remaining, no error yet + expect(sub.getState().error).toBeUndefined() + expect(onError).not.toHaveBeenCalled() + expect(sub.getState().isSynced).toBe(false) + + await vi.advanceTimersByTimeAsync(101) + // Attempt 2 (retry 1) failed — 1 retry remaining + expect(sub.getState().error).toBeUndefined() + expect(onError).not.toHaveBeenCalled() + + await vi.advanceTimersByTimeAsync(101) + // Attempt 3 (retry 2) failed — retries exhausted, error surfaced + expect(sub.getState().error).toBeInstanceOf(Error) + expect(onError).toHaveBeenCalledOnce() + // localState preserved so the pending change is still visible + expect(sub.getState().isSynced).toBe(false) + }) + + it('maxWriteRetries: 0 surfaces error immediately on the first failure', async () => { + vi.mocked(firestore.setDoc).mockRejectedValue(new Error('perm denied')) + const onError = vi.fn() + store = createStore({ firestore: {} as any, autosave: 0, onError }) + + const sub = createDocumentSubscription({ + store, + definition: buildDocumentDefinition( + doc({ path: 'items/{id}', schema, maxWriteRetries: 0 }) + ), + docId: 'i1', + collectionPath: 'items', + }) + sub.load() + + sub.getHandle().set({ title: 'new' }) + await sub.sync() + + expect(sub.getState().error).toBeInstanceOf(Error) + expect(onError).toHaveBeenCalledOnce() + }) + + it('a new user edit resets the retry budget', async () => { + vi.mocked(firestore.updateDoc).mockRejectedValue(new Error('fail')) + const onError = vi.fn() + store = createStore({ firestore: {} as any, autosave: 0, onError }) + + const sub = createDocumentSubscription({ + store, + definition: buildDocumentDefinition( + doc({ path: 'items/{id}', schema, maxWriteRetries: 1, retryInterval: 100 }) + ), + docId: 'i1', + collectionPath: 'items', + }) + sub.load() + fireDocSnapshot({ title: 'original' }) + + // First edit: consumes 1 of 1 retry slot + sub.getHandle().update({ title: 'v1' }) + await sub.sync() + expect(onError).not.toHaveBeenCalled() + + // New edit before the retry fires — cancels the old timer and resets the budget + sub.getHandle().update({ title: 'v2' }) + await sub.sync() + // Budget was reset to 0, so this counts as attempt 1 of a fresh budget + expect(sub.getState().error).toBeUndefined() + expect(onError).not.toHaveBeenCalled() + + // One retry of the fresh budget exhausted — error surfaces + await vi.advanceTimersByTimeAsync(101) + expect(sub.getState().error).toBeInstanceOf(Error) + expect(onError).toHaveBeenCalledOnce() + }) + + it('collection batch commit is also retried before surfacing the error', async () => { + const mockBatch = { + set: vi.fn(), + update: vi.fn(), + delete: vi.fn(), + commit: vi.fn().mockRejectedValue(new Error('batch fail')), + } + vi.mocked(firestore.writeBatch).mockReturnValue(mockBatch as any) + + const onError = vi.fn() + store = createStore({ firestore: {} as any, autosave: 0, onError }) + const spaceSchema = z.object({ label: z.string() }) + + const sub = createCollectionSubscription({ + store, + collectionPath: 'spaces', + definition: buildCollectionDefinition( + col({ path: 'spaces', schema: spaceSchema, maxWriteRetries: 1, retryInterval: 100 }) + ), + }) + sub.load() + // Fire an empty snapshot so syncState is defined and add() is allowed + snapshotCallback!({ docs: [] }) + + sub.getHandle().add({ label: 'room' }) + await sub.sync() + + // Attempt 1 failed — no error yet, 1 retry scheduled + expect(sub.getState().error).toBeUndefined() + expect(onError).not.toHaveBeenCalled() + + await vi.advanceTimersByTimeAsync(101) + + // Retry 1 failed — retries exhausted, error surfaced + expect(sub.getState().error).toBeInstanceOf(Error) + expect(onError).toHaveBeenCalledOnce() + }) + + it('an intervening snapshot does not cancel the write-retry timer (autosave: 0 regression)', async () => { + // Regression for the bug reported in the Codex review: handleSnapshot() + // calls scheduleAutosave() when localState is pending, which previously + // cleared writeRetryTimeout and reset writeRetryCount. With autosave: 0 + // no replacement timer was scheduled, leaving the edit stuck indefinitely. + vi.mocked(firestore.updateDoc).mockRejectedValue(new Error('transient')) + const onError = vi.fn() + store = createStore({ firestore: {} as any, autosave: 0, onError }) + + const sub = createDocumentSubscription({ + store, + definition: buildDocumentDefinition( + doc({ path: 'items/{id}', schema, maxWriteRetries: 1, retryInterval: 100 }) + ), + docId: 'i1', + collectionPath: 'items', + }) + sub.load() + fireDocSnapshot({ title: 'original' }) + + sub.getHandle().update({ title: 'v1' }) + await sub.sync() + + // Retry is scheduled (1 of 1 retry budget used) + expect(sub.getState().error).toBeUndefined() + expect(onError).not.toHaveBeenCalled() + + // An unrelated snapshot arrives before the retry fires — must NOT + // cancel the retry timer or reset the budget. + fireDocSnapshot({ title: 'original' }) + + // Retry still fires on schedule + await vi.advanceTimersByTimeAsync(101) + // Budget exhausted — error surfaced + expect(sub.getState().error).toBeInstanceOf(Error) + expect(onError).toHaveBeenCalledOnce() + }) +}) diff --git a/src/firestate.ts b/src/firestate.ts index d622ccf..5aef2c8 100644 --- a/src/firestate.ts +++ b/src/firestate.ts @@ -69,6 +69,8 @@ interface CommonEntryOptions { retryOnError?: boolean; /** Retry interval (ms). */ retryInterval?: number; + /** Maximum automatic retries on transient write failures (default 3). */ + maxWriteRetries?: number; } /** @@ -323,6 +325,7 @@ export function buildDocumentDefinition( readOnly: entry.readOnly, retryOnError: entry.retryOnError, retryInterval: entry.retryInterval, + maxWriteRetries: entry.maxWriteRetries, } as DocumentDefinition); } @@ -344,6 +347,7 @@ export function buildCollectionDefinition( queryConstraints: entry.queryConstraints, retryOnError: entry.retryOnError, retryInterval: entry.retryInterval, + maxWriteRetries: entry.maxWriteRetries, } as CollectionDefinition); } diff --git a/src/types.ts b/src/types.ts index ef4e45b..33fd909 100644 --- a/src/types.ts +++ b/src/types.ts @@ -220,6 +220,13 @@ export interface DocumentDefinition { retryOnError?: boolean; /** Retry interval (ms), default 5000 */ retryInterval?: number; + /** + * Maximum number of automatic retries on transient write failures before + * surfacing the error. On each failure firestate reschedules the write after + * `retryInterval` ms. Set to `0` to disable automatic write retries. + * Default: 3. + */ + maxWriteRetries?: number; } /** @@ -252,6 +259,13 @@ export interface CollectionDefinition { retryOnError?: boolean; /** Retry interval (ms), default 5000 */ retryInterval?: number; + /** + * Maximum number of automatic retries on transient write failures before + * surfacing the error. On each failure firestate reschedules the write after + * `retryInterval` ms. Set to `0` to disable automatic write retries. + * Default: 3. + */ + maxWriteRetries?: number; } /**