diff --git a/README.md b/README.md index f236a35..bb1e308 100644 --- a/README.md +++ b/README.md @@ -72,6 +72,52 @@ function App() { Hooks that communicate with the map server will wait for `getMapServerBaseUrl()` to resolve before making requests, so the provider can be mounted before the server is ready. You can also provide an optional `fetch` prop to use a custom fetch implementation. +### Recovering from a backend restart + +On platforms where the backend can restart underneath a running app — Android, where it lives in a separate OS process that the system may kill under memory pressure — pass a `subscribeToBackendRestart` function. It receives a listener and returns a function that removes it. Call the listener once the transport has reconnected, i.e. once requests will reach the new backend: + +```tsx +// Defined outside the component (or wrapped in `useCallback`) so that its +// identity is stable: a new function on every render makes the provider +// unsubscribe and resubscribe on every render. +function subscribeToBackendRestart(listener: () => void) { + return subscribeToRestarts(listener) +} + +function App() { + return ( + + + + ) +} +``` + +Platforms whose backend cannot outlive the app, such as desktop, should omit the prop. + +#### What a notification does + +The client API and every project reference survive a restart: under `@comapeo/ipc` v10 a project reference is permanent, and its channel re-opens transparently against the new backend. What does not survive is the data read through them — the media server comes back on a different port, and everything the backend held in memory (invites, sync state) is gone. On each notification the provider resets its own queries (never the consuming app's) in four steps: + +1. **Removes** every query read through a project instance — project settings, members, documents, and the cached media server origin. Removal, rather than invalidation, is what reaches the media server origin, which is cached with `staleTime: 'static'` and so cannot be invalidated at all. It also guarantees that a fetcher which failed as the old backend went away cannot stay latched in `status: 'error'`, which a suspense query with retries disabled never recovers from. +2. **Resets** the cached per-project API instances. This is what makes mounted screens react: removal on its own is invisible to a mounted component, which keeps rendering its last result, whereas a reset suspends it. On resume it re-runs the queries removed in step 1. +3. **Invalidates** what is left — device info, invites, the project list. These are read through the client API, so a background refetch is enough and screens showing them do not flash a loading state. +4. **Refreshes** the sync state, which is an external store rather than a query. Because the project reference is permanent, step 2 hands back the same store, so the error it latched when the backend went away has to be cleared and its state re-read explicitly. + +#### Projects this device has left + +A restart is recoverable; leaving a project is not. Every call on a project this device has left rejects with an error carrying `code: 'PROJECT_LEFT'`, as does `useSingleProject` for a project left before it was ever fetched. That error is deliberately neither retried nor reset — it surfaces at the nearest error boundary, and the project only becomes usable again by re-joining through an invite. + +#### What it does not cover + +- **In-memory map share state.** The received and sent map share stores are plain in-memory stores, not queries, and are not reset, so a share that was pending when the backend restarted stays pending. A share that was _downloading_ keeps waiting while its progress stream reconnects, and ends up in `error` with code `EVENT_STREAM_ERROR` only once the stream has been down for a minute; downloading it again starts a fresh download. Resuming an interrupted download where it left off is a separate piece of work. +- **Events emitted during the disconnect window.** Invites, sync state and `map-share` events raised while the app was disconnected from the backend are lost. Refetching the queries above is the compensation for this: the state they carry is re-read from the new backend, but one-off notifications are not replayed. + ## API Documentation Still a work in progress. Currently lives in [`docs/API.md`](./docs/API.md). diff --git a/docs/API.md b/docs/API.md index f55b560..2a784b1 100644 --- a/docs/API.md +++ b/docs/API.md @@ -79,12 +79,13 @@ Create a context provider that holds a CoMapeo API client instance. | Function | Type | | ---------- | ---------- | -| `ClientApiProvider` | `({ children, clientApi, }: ClientApiProviderProps) => Element` | +| `ClientApiProvider` | `({ children, clientApi, subscribeToBackendRestart, }: ClientApiProviderProps) => Element` | Parameters: * `opts.children`: React children node * `opts.clientApi`: Client API instance +* `opts.subscribeToBackendRestart`: Optional, referentially stable subscribe function for backend-restart notifications ### useClientApi @@ -287,7 +288,7 @@ Throws if used outside of MapServerProvider. | Function | Type | | ---------- | ---------- | -| `ComapeoCoreProvider` | `({ children, clientApi, getMapServerBaseUrl, fetch, queryClient, }: ComapeoCoreProviderProps) => Element` | +| `ComapeoCoreProvider` | `({ children, clientApi, getMapServerBaseUrl, fetch, queryClient, subscribeToBackendRestart, }: ComapeoCoreProviderProps) => Element` | ### useProjectSettings @@ -319,6 +320,15 @@ Retrieve a project API instance for a project. This is mostly used internally by the other hooks and should only be used if certain project APIs are not exposed via the hooks. +Project instances are permanent references (`@comapeo/ipc` v10): the same +instance is handed back for the lifetime of the client, and it keeps working +across a backend restart. The one case where it stops working is a project +this device has left — every call on it, and this hook itself for a project +left before it was ever fetched, then rejects with an error carrying +`code: 'PROJECT_LEFT'`. That error is not retried and is not recovered from: +it surfaces at the nearest error boundary, and the project only becomes +usable again by re-joining through an invite. + | Function | Type | | ---------- | ---------- | | `useSingleProject` | `({ projectId, }: { projectId: string; }) => Pick>, "data" or "error" or "isRefetching">` | @@ -1432,7 +1442,7 @@ function SentShareStatus({ shareId }: { shareId: string }) { | Constant | Type | | ---------- | ---------- | -| `ReceivedMapSharesContext` | `Context<{ subscribe: (listener: () => void) => () => boolean; getSnapshot: () => ReceivedMapShareState[]; actions: { download({ shareId }: DownloadMapShareOptions): Promise<...>; decline({ shareId, reason }: DeclineMapShareOptions): Promise<...>; abort({ shareId }: AbortMapShareOptions): Promise<...>; }; } or null>` | +| `ReceivedMapSharesContext` | `Context<{ subscribe: (listener: () => void) => () => boolean; getSnapshot: () => ReceivedMapShareState[]; actions: { download({ shareId }: DownloadMapShareOptions): Promise<...>; decline({ shareId, reason }: DeclineMapShareOptions): Promise<...>; abort({ shareId }: AbortMapShareOptions): Promise<...>; }; listen(): (...` | ### SentMapSharesContext @@ -1450,6 +1460,7 @@ function SentShareStatus({ shareId }: { shareId: string }) { ## Types +- [SubscribeToBackendRestart](#subscribetobackendrestart) - [ClientApiProviderProps](#clientapiproviderprops) - [CompatFile](#compatfile) - [ExpoFileDuckType](#expofileducktype) @@ -1457,11 +1468,45 @@ function SentShareStatus({ shareId }: { shareId: string }) { - [MapServerApi](#mapserverapi) - [MapServerProviderProps](#mapserverproviderprops) +### SubscribeToBackendRestart + +Subscribe to notifications that the CoMapeo backend restarted, so that cached +data pointing at the previous backend instance can be discarded. + +A "restart" means the backend lost all of its in-memory state and came back +as a fresh instance — for example on Android, where the backend runs in its +own OS process that the system can kill under memory pressure and later +restart while the app keeps running. It does *not* mean a dropped and +re-established transport connection to a backend that is still alive. + +The listener should be called after the RPC transport has reconnected to the +new backend, i.e. once requests made on it will reach the new instance. + +Pass a referentially stable function — a module-scope function, or one +wrapped in `useCallback` — because a new identity on every render makes the +provider unsubscribe and resubscribe on every render. + +Platforms whose backend cannot outlive the app (desktop, where the backend +dying exits the app) should omit this prop. + +| Type | Type | +| ---------- | ---------- | +| `SubscribeToBackendRestart` | `(listener: () => void) => () => void` | + +Parameters: + +* `listener`: Called each time the backend has restarted + + +Returns: + +A function that removes the listener + ### ClientApiProviderProps | Type | Type | | ---------- | ---------- | -| `ClientApiProviderProps` | `PropsWithChildren<{ clientApi: ComapeoCoreClientApi }>` | +| `ClientApiProviderProps` | `PropsWithChildren<{ clientApi: ComapeoCoreClientApi /** * Subscribe function for backend-restart notifications. See * {@link SubscribeToBackendRestart}. */ subscribeToBackendRestart?: SubscribeToBackendRestart }>` | ### CompatFile diff --git a/package.json b/package.json index e877efb..68c0ee0 100644 --- a/package.json +++ b/package.json @@ -71,7 +71,7 @@ }, "peerDependencies": { "@comapeo/core": "^7.2.0", - "@comapeo/ipc": "^9.0.0", + "@comapeo/ipc": "^9.0.0 || ^10.0.0", "@tanstack/react-query": "^5", "react": "^18 || ^19" }, diff --git a/src/contexts/ClientApi.ts b/src/contexts/ClientApi.ts index 8fcdffb..90655f0 100644 --- a/src/contexts/ClientApi.ts +++ b/src/contexts/ClientApi.ts @@ -4,18 +4,52 @@ import { createContext, createElement, useEffect, + useRef, type Context, type JSX, type PropsWithChildren, } from 'react' -import { getInvitesQueryKey } from '../lib/react-query.js' +import { + getInvitesQueryKey, + resetQueriesAfterBackendRestart, +} from '../lib/react-query.js' export const ClientApiContext: Context = createContext(null) +/** + * Subscribe to notifications that the CoMapeo backend restarted, so that cached + * data pointing at the previous backend instance can be discarded. + * + * A "restart" means the backend lost all of its in-memory state and came back + * as a fresh instance — for example on Android, where the backend runs in its + * own OS process that the system can kill under memory pressure and later + * restart while the app keeps running. It does *not* mean a dropped and + * re-established transport connection to a backend that is still alive. + * + * The listener should be called after the RPC transport has reconnected to the + * new backend, i.e. once requests made on it will reach the new instance. + * + * Pass a referentially stable function — a module-scope function, or one + * wrapped in `useCallback` — because a new identity on every render makes the + * provider unsubscribe and resubscribe on every render. + * + * Platforms whose backend cannot outlive the app (desktop, where the backend + * dying exits the app) should omit this prop. + * + * @param listener Called each time the backend has restarted + * @returns A function that removes the listener + */ +export type SubscribeToBackendRestart = (listener: () => void) => () => void + export type ClientApiProviderProps = PropsWithChildren<{ clientApi: ComapeoCoreClientApi + /** + * Subscribe function for backend-restart notifications. See + * {@link SubscribeToBackendRestart}. + */ + subscribeToBackendRestart?: SubscribeToBackendRestart }> /** @@ -23,14 +57,31 @@ export type ClientApiProviderProps = PropsWithChildren<{ * * @param opts.children React children node * @param opts.clientApi Client API instance + * @param opts.subscribeToBackendRestart Optional, referentially stable subscribe function for backend-restart notifications * */ export function ClientApiProvider({ children, clientApi, + subscribeToBackendRestart, }: ClientApiProviderProps): JSX.Element { const queryClient = useQueryClient() + // Parked in a ref so that the subscribe effect only depends on the subscribe + // function, and a new `queryClient` identity cannot churn the subscription. + const queryClientRef = useRef(queryClient) + useEffect(() => { + queryClientRef.current = queryClient + }, [queryClient]) + + useEffect(() => { + if (!subscribeToBackendRestart) return + + return subscribeToBackendRestart(() => { + resetQueriesAfterBackendRestart(queryClientRef.current) + }) + }, [subscribeToBackendRestart]) + useEffect(() => { function invalidateInviteCache() { queryClient.invalidateQueries({ queryKey: getInvitesQueryKey() }) diff --git a/src/contexts/ComapeoCore.ts b/src/contexts/ComapeoCore.ts index 493436b..3ae92f6 100644 --- a/src/contexts/ComapeoCore.ts +++ b/src/contexts/ComapeoCore.ts @@ -14,10 +14,11 @@ export function ComapeoCoreProvider({ getMapServerBaseUrl, fetch, queryClient, + subscribeToBackendRestart, }: ComapeoCoreProviderProps): JSX.Element { return createElement( ClientApiProvider, - { clientApi }, + { clientApi, subscribeToBackendRestart }, createElement( MapServerProvider, { getBaseUrl: getMapServerBaseUrl, fetch, queryClient }, diff --git a/src/contexts/MapShares.ts b/src/contexts/MapShares.ts index 8bcde6d..435c25c 100644 --- a/src/contexts/MapShares.ts +++ b/src/contexts/MapShares.ts @@ -5,6 +5,7 @@ import { createElement, useCallback, useContext, + useEffect, useMemo, useSyncExternalStore, type Context, @@ -55,6 +56,9 @@ export function ReceivedMapSharesProvider({ createReceivedMapSharesStore({ clientApi, mapServerApi, queryClient }), [clientApi, mapServerApi, queryClient], ) + + useEffect(() => mapSharesStore.listen(), [mapSharesStore]) + return createElement( ReceivedMapSharesContext.Provider, { value: mapSharesStore }, diff --git a/src/hooks/projects.ts b/src/hooks/projects.ts index 1a45ed4..b65d6d2 100644 --- a/src/hooks/projects.ts +++ b/src/hooks/projects.ts @@ -79,6 +79,15 @@ Pick< * * This is mostly used internally by the other hooks and should only be used if certain project APIs are not exposed via the hooks. * + * Project instances are permanent references (`@comapeo/ipc` v10): the same + * instance is handed back for the lifetime of the client, and it keeps working + * across a backend restart. The one case where it stops working is a project + * this device has left — every call on it, and this hook itself for a project + * left before it was ever fetched, then rejects with an error carrying + * `code: 'PROJECT_LEFT'`. That error is not retried and is not recovered from: + * it surfaces at the nearest error boundary, and the project only becomes + * usable again by re-joining through an invite. + * * @param opts.projectId Project public ID * * @example @@ -375,6 +384,17 @@ const FAKE_BLOB_ID: BlobApi.BlobId = { /** * @internal * Hack to retrieve the media server origin (protocol + host). + * + * The probe is why `resetQueriesAfterBackendRestart` has to special-case this + * key: it is read from a project instance but lives outside the + * `projects/` namespace, and `staleTime: 'static'` makes + * invalidation a structural no-op, so removal is the only thing that reaches + * it. The port changes on every restart, so missing it means every image URL + * points at a dead port for the life of the app. + * + * TODO: replace the FAKE_BLOB_ID probe with a direct origin API, which would + * make both the odd query key and that special case unnecessary. See + * digidem/comapeo-core-react#96. */ function useMediaServerOrigin({ projectApi, diff --git a/src/index.ts b/src/index.ts index d1325fa..e527370 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1,4 +1,5 @@ export { ComapeoCoreProvider } from './contexts/ComapeoCore.js' +export type { SubscribeToBackendRestart } from './contexts/ClientApi.js' export { useClientApi, useIsArchiveDevice, @@ -51,6 +52,7 @@ export { MapShareErrorCode, getErrorCode, MapShareCanceledError, + MapShareStreamError, InvalidStatusTransitionError, } from './lib/map-shares-stores.js' export { diff --git a/src/lib/map-shares-stores.ts b/src/lib/map-shares-stores.ts index 35e5cad..275987a 100644 --- a/src/lib/map-shares-stores.ts +++ b/src/lib/map-shares-stores.ts @@ -72,6 +72,10 @@ export type SentMapSharesStore = ReturnType * * ## Common * + * - `EVENT_STREAM_ERROR` — the server-sent event stream reporting the share's + * progress stayed down long enough that its final status will never arrive. + * Appears as a share state error for either sender or receiver. A received + * share in this state can be downloaded again. * - `UNKNOWN_ERROR` — fallback when the original error has no specific code. * Can appear as both a mutation error and a share state error for either * sender or receiver. @@ -125,6 +129,8 @@ export const MapShareErrorCode = { // --- Common --- + /** Receiver/Sender: the progress event stream could not be re-established */ + EVENT_STREAM_ERROR: 'EVENT_STREAM_ERROR', /** Receiver/Sender: fallback code when the original error has no specific code */ UNKNOWN_ERROR: 'UNKNOWN_ERROR', } as const @@ -226,6 +232,32 @@ export class InvalidStatusTransitionError extends Error { } } +/** + * Thrown when the server-sent event stream that reports a map share's progress + * stays down long enough that its final status will never arrive. Has + * `code: 'EVENT_STREAM_ERROR'`. The share can be downloaded again. + */ +export class MapShareStreamError extends Error { + code = 'EVENT_STREAM_ERROR' as const + constructor(shareId: string) { + super(`Lost the event stream for map share ${shareId}`) + this.name = 'MapShareStreamError' + } +} + +/** + * How long the progress stream may stay down before the share is failed. + * + * The event source reconnects on its own and reports the delay it will wait + * before each attempt, so summing those delays measures the outage. There is no + * option to set the delay: eventsource-client uses 2s with no backoff unless the + * server sends a `retry:` field, which it honours — either way the budget below + * follows it rather than assuming a value. Sized for an Android backend restart, + * which routinely takes tens of seconds; a threshold measured in attempts (3 + * attempts is ~6s) would fail exactly the shares this recovery path exists for. + */ +const MAX_EVENT_STREAM_OUTAGE_MS = 60_000 + /** * This is like a mini zustand store. Keeping the map shares in an external * store avoids unnecessary re-renders of the entire app when map shares are @@ -307,9 +339,26 @@ function createMapSharesStore< async function monitor(mapShareId: string, path: string) { // TODO: add a timeout in case the download stalls and never completes return new Promise((resolve, reject) => { + // The event source reconnects on its own, so a dropped connection is + // not a failure until the server stays unreachable for the whole + // budget. Without this the share sits in `downloading` for the life of + // the app when the map server is gone for good, killed with the + // backend on Android. Any delivered message resets the budget: the map + // server sends the current state as the first message on a stream, so + // one event means the stream is genuinely live again. + let outageMs = 0 const es = mapServerApi.createEventSource({ url: path, + onScheduleReconnect({ delay }) { + outageMs += delay + if (outageMs <= MAX_EVENT_STREAM_OUTAGE_MS) return + es.close() + const error = new MapShareStreamError(mapShareId) + handleError(mapShareId, error) + reject(error) + }, onMessage({ data }) { + outageMs = 0 try { const stateUpdate = JSON.parse(data) update(mapShareId, stateUpdate) @@ -369,9 +418,11 @@ export function createReceivedMapSharesStore({ // cleanup, but this is unlikely to be an issue in practice. const downloads = new Map>() - clientApi.on('map-share', (mapShare: MapShare) => { + function handleMapShare(mapShare: MapShare) { add({ ...mapShare, status: 'pending' }) - }) + } + + let stopListening: (() => void) | undefined const actions = { async download({ shareId }: DownloadMapShareOptions) { @@ -405,8 +456,13 @@ export function createReceivedMapSharesStore({ downloads.set(shareId, downloadIdPromise) const downloadId = await downloadIdPromise monitor(shareId, `downloads/${downloadId}/events`) - .then((stateUpdate) => { + .finally(() => { + // However the monitor ends, not just on success: a share that + // errored can be downloaded again, and a leftover entry would + // point the retry's abort at a download id that is long gone. downloads.delete(shareId) + }) + .then((stateUpdate) => { // Invalidate map queries when download completes to trigger reload of map if (stateUpdate.status === 'completed') { return invalidateMapQueries(queryClient, { @@ -477,6 +533,26 @@ export function createReceivedMapSharesStore({ subscribe, getSnapshot, actions, + /** + * Start receiving `map-share` events from the client API. Attaching from + * an effect rather than at creation keeps store creation free of side + * effects, so a render that React discards cannot orphan a listener. + * + * Calling this while already listening is a no-op that returns the + * existing teardown, so a double-invoked effect cannot register the + * listener twice and have every share added to the store twice. + * + * @returns A teardown function that removes the listener + */ + listen() { + if (stopListening) return stopListening + clientApi.on('map-share', handleMapShare) + stopListening = () => { + stopListening = undefined + clientApi.off('map-share', handleMapShare) + } + return stopListening + }, } } @@ -544,7 +620,11 @@ const allowedStatusTransitions: Record< canceled: ['error'], aborted: ['error'], declined: ['error'], - error: ['error'], + // The only status the user did not choose, so the only one worth offering a + // way out of. `declined` and `aborted` are decisions; `error` is a failure, + // and the store is never reset, so without this a share that lost its + // progress stream is a dead row for the life of the app. + error: ['error', 'downloading'], } /** diff --git a/src/lib/react-query.ts b/src/lib/react-query.ts index beefed2..7e794dd 100644 --- a/src/lib/react-query.ts +++ b/src/lib/react-query.ts @@ -8,21 +8,76 @@ import type { } from '@tanstack/react-query' import { DistributedPick } from 'type-fest' +import { refreshActiveSyncStores } from './sync.js' import type { WriteableDocumentType } from './types.js' // #region Shared const ROOT_QUERY_KEY = '@comapeo/core-react' +/** + * Prefix shared by every query key owned by this package. Matching on it + * targets all of our caches without touching queries owned by the consuming + * app, which may share the same `QueryClient`. + */ +export function getRootQueryKey() { + return [ROOT_QUERY_KEY] as const +} + +/** + * A call that was in flight when the RPC transport to the backend dropped + * rejects with this code — rpc-reflector's `ChannelClosedError`, re-exported by + * `@comapeo/ipc` as `RpcChannelClosedError` and used both when the client is + * closed and when the transport owner reports a reset. The response will never + * arrive, but the call was a read, so re-issuing it is safe — and on platforms + * where the backend restarts in place (Android), the retried call waits in the + * transport's send queue until the new backend is up, turning an error flash + * into continued loading. Matched by `code` rather than `instanceof` so a + * duplicated copy of `@comapeo/ipc` in the dependency tree cannot break the + * check. + */ +const CHANNEL_CLOSED_CODE = 'RPC_CHANNEL_CLOSED' + +/** + * A project this device has left rejects every call with this code until it is + * re-joined via an invite, so retrying only re-rejects. + */ +const PROJECT_LEFT_CODE = 'PROJECT_LEFT' + +const CHANNEL_CLOSED_RETRY_LIMIT = 3 +const CHANNEL_CLOSED_RETRY_DELAY_MS = 1_000 + +function getErrorCode(error: unknown): unknown { + return typeof error === 'object' && error !== null && 'code' in error + ? error.code + : undefined +} + +function shouldRetryQuery(failureCount: number, error: unknown): boolean { + if (failureCount >= CHANNEL_CLOSED_RETRY_LIMIT) return false + const code = getErrorCode(error) + if (code === PROJECT_LEFT_CODE) return false + return code === CHANNEL_CLOSED_CODE +} + // Since the API is running locally, queries should run regardless of network -// status, and should not be retried. In React Native the API consumer would -// have to manually set the network mode, but we still should keep these options -// to avoid surprises. Not using the queryClient `defaultOptions` because the API -// consumer might also use the same queryClient for network queries +// status, and should not be retried — with one exception: a channel-closed +// rejection (the transport to the backend dropped under the call) is retried a +// bounded number of times, see `shouldRetryQuery`. Mutations are never retried: +// re-issuing a write whose response was lost is not safe. In React Native the +// API consumer would have to manually set the network mode, but we still +// should keep these options to avoid surprises. Not using the queryClient +// `defaultOptions` because the API consumer might also use the same +// queryClient for network queries — and because these per-hook options would +// override a client-level default anyway. export function baseQueryOptions() { return { networkMode: 'always', - retry: false, + // Param typed as the registered `Error` default so TError inference in + // the hooks is unaffected; the guard itself narrows from unknown. + retry: (failureCount: number, error: Error) => + shouldRetryQuery(failureCount, error), + retryDelay: CHANNEL_CLOSED_RETRY_DELAY_MS, } satisfies QueryOptions } @@ -305,3 +360,94 @@ export function getDocumentByVersionIdQueryKey< } // #endregion + +// #region Backend restart + +const PROJECT_INSTANCE_QUERY_KEY_LENGTH = getProjectByIdQueryKey({ + projectId: '', +}).length + +function hasPrefix( + queryKey: ReadonlyArray, + prefix: ReadonlyArray, +) { + return prefix.every((segment, index) => queryKey[index] === segment) +} + +function isProjectScopedQueryKey(queryKey: ReadonlyArray) { + return hasPrefix(queryKey, getProjectsQueryKey()) +} + +/** + * Queries whose data was read through a `ComapeoProjectClientApi` instance, and + * so describes the backend that has gone: everything nested below + * `projects/`, plus the media server origin, which is keyed outside + * the project namespace but is read from a project instance. + * + * `document_created_by` is content-addressed, so its data survives a restart, + * but it is dropped with the rest rather than carved out — re-reading an + * immutable mapping is cheaper than the exception. + */ +function isBoundToProjectInstance(queryKey: ReadonlyArray) { + return ( + hasPrefix(queryKey, getMediaServerOriginQueryKey()) || + (isProjectScopedQueryKey(queryKey) && + queryKey.length > PROJECT_INSTANCE_QUERY_KEY_LENGTH) + ) +} + +function isProjectInstanceQueryKey(queryKey: ReadonlyArray) { + return ( + isProjectScopedQueryKey(queryKey) && + queryKey.length === PROJECT_INSTANCE_QUERY_KEY_LENGTH + ) +} + +/** + * Drop what a backend restart made stale and get mounted components reading + * from the new backend. + * + * The references themselves survive: the client API is unaffected, and a + * project reference is permanent (`@comapeo/ipc` v10) — its channel re-opens + * against the restarted backend on the next call. What does not survive is what + * was read through them. The media server comes back on a different port, and + * everything the backend held in memory is gone. + * + * Each of the four steps does something the others cannot: + * + * 1. `removeQueries` for everything read through a project instance. Removal + * rather than invalidation, because the media server origin is cached with + * `staleTime: 'static'`, which invalidation skips structurally — so it would + * otherwise hand out URLs for the dead port for the life of the app. Removal + * also guarantees that a query which failed as the old backend went away + * cannot stay latched in `status: 'error'`, which `shouldFetchOptionally` in + * query-core never retries for a `useSuspenseQuery`. + * 2. `resetQueries` for the project instance queries. Removal is invisible to a + * mounted observer — it keeps rendering its last result indefinitely because + * nothing dispatches a state change — whereas resetting does dispatch, so + * components suspend on `useSingleProject` and rebuild and refetch the + * queries removed in step 1. The `queryFn` here calls + * `clientApi.getProject()`, which is safe to re-run: it resolves the same + * permanent reference without a wire round trip. + * 3. `invalidateQueries` for the remainder: manager-level data such as device + * info, invites and the project list, all read through the client API. A + * background refetch is enough, and avoids a loading state. + * 4. `refreshActiveSyncStores` for the sync state, which is an external store + * rather than a query, keyed on the project reference. Because that + * reference is permanent, step 2 hands back the same store, so nothing else + * would drop the state and error it holds from the previous backend. + */ +export function resetQueriesAfterBackendRestart(queryClient: QueryClient) { + queryClient.removeQueries({ + queryKey: getRootQueryKey(), + predicate: (query) => isBoundToProjectInstance(query.queryKey), + }) + queryClient.resetQueries({ + queryKey: getRootQueryKey(), + predicate: (query) => isProjectInstanceQueryKey(query.queryKey), + }) + queryClient.invalidateQueries({ queryKey: getRootQueryKey() }) + refreshActiveSyncStores() +} + +// #endregion diff --git a/src/lib/sync.ts b/src/lib/sync.ts index 9d30362..d7d6308 100644 --- a/src/lib/sync.ts +++ b/src/lib/sync.ts @@ -1,4 +1,5 @@ import type { ComapeoProjectClientApi } from '@comapeo/ipc' +import ensureError from 'ensure-error' export type SyncState = Awaited< ReturnType @@ -11,11 +12,43 @@ function getDataSyncCountForDevice( return data.want + data.wanted } +/** + * Every store that currently has at least one listener. `useSyncStore` caches + * one store per project client in a `WeakMap`, and with a permanent project + * client reference (`@comapeo/ipc` v10) a backend restart never produces a + * fresh store — so the existing ones have to be reachable, and a `WeakMap` + * cannot be iterated. Stores join on their first listener and leave on their + * last, so nothing the app has stopped using is retained. + */ +const ACTIVE_SYNC_STORES = new Set() + +/** + * @internal + * Re-read sync state on every store that is currently being listened to, + * discarding the state, the progress baselines and any error left over from the + * previous backend. + * + * Only subscribed stores need this. A store whose last listener has gone drops + * its error as it unsubscribes and reads from scratch when something subscribes + * again, so it recovers without being tracked here. + * + * Caller contract: the transport owner must have re-sent its event + * subscriptions to the restarted backend before calling this. Each store + * re-reads state over the wire, but it does not re-send its own `sync-state` + * subscription unless attaching the listener failed in the first place. + */ +export function refreshActiveSyncStores() { + for (const store of ACTIVE_SYNC_STORES) { + store.refreshAfterBackendRestart() + } +} + export class SyncStore { #project: ComapeoProjectClientApi #listeners = new Set<() => void>() #isSubscribedInternal = false + #isListening = false #error: Error | null = null #state: SyncState | null = null @@ -121,21 +154,71 @@ export class SyncStore { this.#notifyListeners() } + #onError = (e: unknown) => { + this.#error = ensureError(e) + this.#notifyListeners() + } + + #connect = () => { + // A fresh read attempt supersedes whatever the last one failed with + this.#error = null + try { + if (!this.#isListening) { + this.#project.$sync.on('sync-state', this.#onSyncState) + this.#isListening = true + } + this.#project.$sync + .getState() + .then(this.#onSyncState) + .catch(this.#onError) + } catch (e) { + // `on`/`off` throw on a closed project wrapper in @comapeo/ipc v9; + // v10 references are permanent, so there this only fires if the whole + // client has been closed. + this.#onError(e) + } + } + #startSubscription = () => { - this.#project.$sync.on('sync-state', this.#onSyncState) this.#isSubscribedInternal = true - this.#project.$sync - .getState() - .then(this.#onSyncState) - .catch((e) => { - this.#error = e - this.#notifyListeners() - }) + ACTIVE_SYNC_STORES.add(this) + this.#connect() } #stopSubscription = () => { this.#isSubscribedInternal = false - this.#project.$sync.off('sync-state', this.#onSyncState) + this.#isListening = false + ACTIVE_SYNC_STORES.delete(this) + // The error belongs to the subscription that produced it. Holding it + // past the last listener wedges the store permanently: `getStateSnapshot` + // throws during render, so an error boundary that remounts the subtree + // hits the same stale error before `subscribe` can run — and with a + // permanent project reference the remount is handed back this very store. + this.#error = null + try { + this.#project.$sync.off('sync-state', this.#onSyncState) + } catch { + // Runs in React effect cleanup, where a throw from a closed project + // wrapper (@comapeo/ipc v9) would take down the tree. + } + } + + /** + * @internal + * Discard everything held from the previous backend and read the current + * state again. See {@link refreshActiveSyncStores} for the caller contract. + */ + refreshAfterBackendRestart = () => { + // The state goes too, not just the baselines it is measured against: + // `getDataProgressSnapshot` divides by the largest sync count seen so + // far, so old state over cleared baselines reads as 1 — "sync complete" — + // on every restart. With no state it reports `null`, i.e. not known yet. + this.#state = null + this.#perDeviceMaxSyncCount.clear() + // Connect first: it clears the error, so the notification below cannot + // hand a listener a snapshot that still throws the previous backend's. + this.#connect() + this.#notifyListeners() } } diff --git a/test/contexts/backend-restart.test.tsx b/test/contexts/backend-restart.test.tsx new file mode 100644 index 0000000..f7fead5 --- /dev/null +++ b/test/contexts/backend-restart.test.tsx @@ -0,0 +1,626 @@ +import type { ComapeoCoreClientApi } from '@comapeo/ipc' +import { + QueryClient, + QueryClientProvider, + useQuery, +} from '@tanstack/react-query' +import { act, render, waitFor, within } from '@testing-library/react' +import { Component, Suspense, type ReactNode } from 'react' +import { describe, expect, test, vi } from 'vitest' + +import { + ComapeoCoreProvider, + useAttachmentUrl, + useManyInvites, + useOwnDeviceInfo, + useProjectSettings, + useSingleProject, + type SubscribeToBackendRestart, +} from '../../src/index.js' +import { createMockClientApi } from '../helpers/client-api-mock.js' + +// Kept as a literal so the test fails if the shared query key prefix changes +// without the reset in `src/lib/react-query.ts` being updated. +const ROOT_QUERY_KEY = '@comapeo/core-react' +const PROJECT_ID = 'project-id' + +function createBackendRestartSource() { + const listeners = new Set<() => void>() + const unsubscribe = vi.fn() + + const subscribe = vi.fn((listener) => { + listeners.add(listener) + return () => { + listeners.delete(listener) + unsubscribe() + } + }) + + return { + subscribe, + unsubscribe, + listenerCount: () => listeners.size, + restart() { + act(() => { + for (const listener of [...listeners]) { + listener() + } + }) + }, + } +} + +/** + * A client API whose `getProject()` hands out generation-tagged project + * instances. Bumping the generation stands in for a backend restart, and is + * deliberately the harshest shape the reset has to cope with: every instance + * handed out before the bump starts rejecting, as a project wrapper does under + * `@comapeo/ipc` v9. A v10 reference stays usable instead, which only makes the + * same reset easier — so passing here covers both. + */ +function createGenerationalClientApi() { + let generation = 0 + + const getProject = vi.fn(async (projectId: string) => { + const instanceGeneration = generation + function assertLive() { + if (instanceGeneration !== generation) { + throw new Error( + `ProjectClosed: instance from generation ${instanceGeneration}`, + ) + } + } + return { + generation: instanceGeneration, + projectId, + $getProjectSettings: async () => { + assertLive() + return { name: `settings-gen-${instanceGeneration}` } + }, + $blobs: { + // The media server port is ephemeral and changes on every restart + getUrl: async () => { + assertLive() + return `http://127.0.0.1:${5000 + instanceGeneration}/blob` + }, + }, + } + }) + + const clientApi = Object.assign(createMockClientApi(), { getProject }) + + return { + clientApi: clientApi as unknown as ComapeoCoreClientApi, + getProject, + bumpGeneration() { + generation += 1 + }, + } +} + +function ProjectScreen() { + const { data: projectApi } = useSingleProject({ projectId: PROJECT_ID }) + const { data: settings } = useProjectSettings({ projectId: PROJECT_ID }) + const { data: attachmentUrl } = useAttachmentUrl({ + projectId: PROJECT_ID, + blobId: { + type: 'photo', + variant: 'thumbnail', + name: 'name', + driveId: 'drive-id', + }, + }) + + return ( +
+ + {String((projectApi as unknown as { generation: number }).generation)} + + {settings.name} + {attachmentUrl} +
+ ) +} + +function renderProvider({ + queryClient, + clientApi = createMockClientApi() as unknown as ComapeoCoreClientApi, + subscribeToBackendRestart, + children, +}: { + queryClient: QueryClient + clientApi?: ComapeoCoreClientApi + subscribeToBackendRestart?: SubscribeToBackendRestart + children?: ReactNode +}) { + function Tree(props: { + subscribeToBackendRestart?: SubscribeToBackendRestart + }) { + return ( + + new URL('http://localhost:3000')} + subscribeToBackendRestart={props.subscribeToBackendRestart} + > + loading}> + {children} + + + + ) + } + + const utils = render( + , + ) + + return { + ...utils, + screen: within(utils.container), + rerenderTree() { + utils.rerender( + , + ) + }, + setSubscribeToBackendRestart(next?: SubscribeToBackendRestart) { + utils.rerender() + }, + } +} + +describe('subscribeToBackendRestart', () => { + test('is optional', () => { + const queryClient = new QueryClient() + + expect(() => { + renderProvider({ queryClient }) + }).not.toThrow() + }) + + // Call counts are not asserted anywhere in this file: the test setup enables + // `reactStrictMode`, so effects are mounted, cleaned up and re-mounted. The + // number of live listeners is what actually matters. + test('subscribes on mount', () => { + const queryClient = new QueryClient() + const backend = createBackendRestartSource() + + renderProvider({ + queryClient, + subscribeToBackendRestart: backend.subscribe, + }) + + expect(backend.subscribe).toHaveBeenCalled() + expect(backend.listenerCount()).toBe(1) + }) + + test('unsubscribes on unmount', () => { + const queryClient = new QueryClient() + const backend = createBackendRestartSource() + + const { unmount } = renderProvider({ + queryClient, + subscribeToBackendRestart: backend.subscribe, + }) + + expect(backend.listenerCount()).toBe(1) + + unmount() + + expect(backend.unsubscribe).toHaveBeenCalled() + expect(backend.listenerCount()).toBe(0) + }) + + test('unsubscribes from the previous function when the prop changes', () => { + const queryClient = new QueryClient() + const first = createBackendRestartSource() + const second = createBackendRestartSource() + + const { setSubscribeToBackendRestart } = renderProvider({ + queryClient, + subscribeToBackendRestart: first.subscribe, + }) + + setSubscribeToBackendRestart(second.subscribe) + + expect(first.unsubscribe).toHaveBeenCalled() + expect(first.listenerCount()).toBe(0) + expect(second.listenerCount()).toBe(1) + }) + + test('does not resubscribe when the tree re-renders with the same function', () => { + const queryClient = new QueryClient() + const backend = createBackendRestartSource() + + const { rerenderTree } = renderProvider({ + queryClient, + subscribeToBackendRestart: backend.subscribe, + }) + + const subscribeCalls = backend.subscribe.mock.calls.length + + rerenderTree() + rerenderTree() + + expect(backend.subscribe.mock.calls.length).toBe(subscribeCalls) + expect(backend.listenerCount()).toBe(1) + }) +}) + +describe('recovery after a restart', () => { + test('a mounted project-derived query ends up with data from the new backend', async () => { + const queryClient = new QueryClient() + const backend = createGenerationalClientApi() + const restarts = createBackendRestartSource() + + const { screen } = renderProvider({ + queryClient, + clientApi: backend.clientApi, + subscribeToBackendRestart: restarts.subscribe, + children: , + }) + + await waitFor(() => { + expect(screen.getByTestId('settings-name').textContent).toBe( + 'settings-gen-0', + ) + }) + + backend.bumpGeneration() + restarts.restart() + + await waitFor(() => { + expect(screen.queryByTestId('loading')).toBeNull() + expect(screen.getByTestId('generation').textContent).toBe('1') + expect(screen.getByTestId('settings-name').textContent).toBe( + 'settings-gen-1', + ) + }) + + // A `useSuspenseQuery` with `retry: false` that fails once never refetches + // again, so the derived query must never have run against the closed + // instance in the first place + expect( + queryClient.getQueryState([ + ROOT_QUERY_KEY, + 'projects', + PROJECT_ID, + 'project_settings', + ])?.status, + ).toBe('success') + }) + + test('a static-staleTime query re-runs after a restart', async () => { + const queryClient = new QueryClient() + const backend = createGenerationalClientApi() + const restarts = createBackendRestartSource() + + const { screen } = renderProvider({ + queryClient, + clientApi: backend.clientApi, + subscribeToBackendRestart: restarts.subscribe, + children: , + }) + + await waitFor(() => { + expect(screen.getByTestId('attachment-url').textContent).toContain( + '127.0.0.1:5000', + ) + }) + + backend.bumpGeneration() + restarts.restart() + + // The media server origin is cached with `staleTime: 'static'`, so + // invalidation cannot reach it: without being removed, every image URL + // would point at the dead port for the life of the app + await waitFor(() => { + expect(screen.getByTestId('attachment-url').textContent).toContain( + '127.0.0.1:5001', + ) + }) + }) + + test('manager-level queries are refetched without dropping their data', async () => { + const queryClient = new QueryClient() + const restarts = createBackendRestartSource() + const queryFn = vi.fn(async () => 'device-info') + + function ManagerLevelProbe() { + const { data } = useQuery({ + queryKey: [ROOT_QUERY_KEY, 'client', 'device_info'], + queryFn, + networkMode: 'always', + retry: false, + }) + return {data ?? 'none'} + } + + const { screen } = renderProvider({ + queryClient, + subscribeToBackendRestart: restarts.subscribe, + children: , + }) + + await waitFor(() => { + expect(queryFn).toHaveBeenCalledTimes(1) + }) + + restarts.restart() + + await waitFor(() => { + expect(queryFn).toHaveBeenCalledTimes(2) + }) + // Invalidated, not removed: the data stays put while it refetches + expect(screen.getByTestId('device-info').textContent).toBe('device-info') + }) + + // Invites are in-memory actors on the backend: a restart drops every one of + // them, and `invite.getMany()` is the only way to find out what the new + // backend has. Nothing invalidates the invites query on its own - the + // `invite-received` / `invite-updated` events that normally do were raised + // (if at all) while the app was disconnected - so the root invalidation is + // what has to reach it. + test('the invites query is refetched', async () => { + const queryClient = new QueryClient() + const restarts = createBackendRestartSource() + const clientApi = createMockClientApi() + const getMany = vi.fn(async () => []) + Object.assign(clientApi.invite, { getMany }) + + function InvitesProbe() { + const { data } = useManyInvites() + return {data.length} + } + + const { screen } = renderProvider({ + queryClient, + clientApi: clientApi as unknown as ComapeoCoreClientApi, + subscribeToBackendRestart: restarts.subscribe, + children: , + }) + + await waitFor(() => { + expect(screen.getByTestId('invite-count').textContent).toBe('0') + }) + const callsBeforeRestart = getMany.mock.calls.length + + restarts.restart() + + await waitFor(() => { + expect(getMany.mock.calls.length).toBeGreaterThan(callsBeforeRestart) + }) + }) + + test('queries owned by the consuming app are left alone', () => { + const queryClient = new QueryClient() + const appQueryKey = ['app-owned-query'] + queryClient.setQueryData(appQueryKey, 'not ours') + + const restarts = createBackendRestartSource() + + renderProvider({ + queryClient, + subscribeToBackendRestart: restarts.subscribe, + }) + + restarts.restart() + + expect(queryClient.getQueryData(appQueryKey)).toBe('not ours') + expect(queryClient.getQueryState(appQueryKey)?.isInvalidated).toBe(false) + }) + + test('a restart no longer resets the cache after unmount', () => { + const queryClient = new QueryClient() + const projectQueryKey = [ROOT_QUERY_KEY, 'projects', PROJECT_ID] + queryClient.setQueryData(projectQueryKey, 'from the previous backend') + + const restarts = createBackendRestartSource() + + const { unmount } = renderProvider({ + queryClient, + subscribeToBackendRestart: restarts.subscribe, + }) + + unmount() + restarts.restart() + + expect(queryClient.getQueryData(projectQueryKey)).toBe( + 'from the previous backend', + ) + }) +}) + +class TestErrorBoundary extends Component< + { children: ReactNode }, + { error: Error | null } +> { + override state: { error: Error | null } = { error: null } + static getDerivedStateFromError(error: Error) { + return { error } + } + override render() { + if (this.state.error) { + return ( + + {this.state.error.message} + + ) + } + return this.props.children + } +} + +function DeviceInfoScreen() { + const { data } = useOwnDeviceInfo() + return ( + + {(data as unknown as { name: string }).name} + + ) +} + +describe('channel-closed retry', () => { + // A query in flight when the backend's RPC transport drops rejects with + // code RPC_CHANNEL_CLOSED (a read whose response will never arrive). + // `baseQueryOptions` retries only that code, so the query keeps loading + // through the restart instead of latching into an error state. + test('a channel-closed rejection is retried and resolves', async () => { + const queryClient = new QueryClient() + const clientApi = + createMockClientApi() as unknown as ComapeoCoreClientApi & { + getDeviceInfo: ReturnType + } + let failuresLeft = 1 + clientApi.getDeviceInfo = vi.fn(async () => { + if (failuresLeft > 0) { + failuresLeft -= 1 + throw Object.assign(new Error('Channel closed'), { + code: 'RPC_CHANNEL_CLOSED', + }) + } + return { + deviceId: 'device-id', + name: 'gecko', + deviceType: 'mobile' as const, + } + }) + + const { screen } = renderProvider({ + queryClient, + clientApi, + children: ( + + + + ), + }) + + await waitFor( + () => { + expect(screen.getByTestId('device-name').textContent).toBe('gecko') + }, + { timeout: 5_000 }, + ) + expect(screen.queryByTestId('boundary-error')).toBeNull() + expect(clientApi.getDeviceInfo.mock.calls.length).toBeGreaterThanOrEqual(2) + }, 10_000) + + test('other errors are not retried', async () => { + const queryClient = new QueryClient() + const clientApi = + createMockClientApi() as unknown as ComapeoCoreClientApi & { + getDeviceInfo: ReturnType + } + clientApi.getDeviceInfo = vi.fn(async () => { + throw new Error('genuinely broken') + }) + + const { screen } = renderProvider({ + queryClient, + clientApi, + children: ( + + + + ), + }) + + await waitFor(() => { + expect(screen.getByTestId('boundary-error').textContent).toBe( + 'genuinely broken', + ) + }) + }) +}) + +// @comapeo/ipc v10 hands out permanent project references. Leaving a project +// does not invalidate the reference the app is holding: every call on it - +// and `getProject()` itself, for a project left before it was ever acquired - +// rejects with `ProjectLeftError` until the project is re-joined via an +// invite. There is nothing to recover from, so the error has to reach the +// consuming app's error boundary with its code intact and without being +// retried first. +describe('a project this device has left', () => { + function createLeftProjectError() { + return Object.assign(new Error('This device has left the project'), { + code: 'PROJECT_LEFT', + }) + } + + function createLeftProjectClientApi({ leftBefore }: { leftBefore: boolean }) { + const getProject = vi.fn(async () => { + if (leftBefore) throw createLeftProjectError() + return { + $getProjectSettings: async () => { + throw createLeftProjectError() + }, + } + }) + const clientApi = Object.assign(createMockClientApi(), { getProject }) + return { + clientApi: clientApi as unknown as ComapeoCoreClientApi, + getProject, + } + } + + function LeftProjectScreen() { + const { data } = useProjectSettings({ projectId: PROJECT_ID }) + return {data.name} + } + + test('a call on a held reference reaches the error boundary', async () => { + const { clientApi } = createLeftProjectClientApi({ leftBefore: false }) + + const { screen } = renderProvider({ + queryClient: new QueryClient(), + clientApi, + children: ( + + + + ), + }) + + await waitFor(() => { + expect(screen.getByTestId('boundary-error').textContent).toBe( + 'This device has left the project', + ) + }) + expect(screen.getByTestId('boundary-error').dataset.code).toBe( + 'PROJECT_LEFT', + ) + }) + + test('a first acquisition reaches the error boundary', async () => { + const { clientApi, getProject } = createLeftProjectClientApi({ + leftBefore: true, + }) + + const { screen } = renderProvider({ + queryClient: new QueryClient(), + clientApi, + children: ( + + + + ), + }) + + await waitFor(() => { + expect(screen.getByTestId('boundary-error').dataset.code).toBe( + 'PROJECT_LEFT', + ) + }) + // Not retried: a retry would push the failure past the 1s retry delay + const callsWhenSettled = getProject.mock.calls.length + await new Promise((resolve) => setTimeout(resolve, 1_200)) + expect(getProject.mock.calls.length).toBe(callsWhenSettled) + }, 10_000) +}) diff --git a/test/hooks/invite-rejoin.test.ts b/test/hooks/invite-rejoin.test.ts index c3557c9..ac29cb0 100644 --- a/test/hooks/invite-rejoin.test.ts +++ b/test/hooks/invite-rejoin.test.ts @@ -62,11 +62,10 @@ async function waitForPeers(managers: Managers) { // Regression test for digidem/comapeo-mobile#2042 and #2041: a member is // removed from a project, leaves it, and is re-invited. Accepting the new -// invite closes the old project instance on the manager -// (`MapeoManager.addProject`) and opens a fresh one. The project client -// wrapper is cached with `staleTime: Infinity`, so without invalidation the -// hooks keep using the closed instance and every project call rejects with -// ProjectClosed until app restart. +// invite re-adds the project on the manager (`MapeoManager.addProject`), +// which closes the instance left behind by leaving and opens a fresh one. +// The project must be usable again from the hooks straight afterwards, +// rather than every call rejecting until the app restarts. test( 're-joining a project after leaving yields a working project instance', { timeout: 60_000 }, @@ -198,11 +197,18 @@ test( ) assert.strictEqual(settingsHook.result.current.data.name, 'mapeo') - // The re-joined project must be a fresh instance — calls on the wrapper - // cached before the re-join reject because that instance is closed. - assert.notStrictEqual( - rejoinedProjectHook.result.current.data, - originalWrapper, - ) + // The regression itself: a call made directly through the reference the + // hooks hand out has to reach a live instance, not the closed one that + // was cached before the leave. Asserted through the reference rather + // than only through the settings query, because it is the cached + // reference that #199 was about. + // + // What the reference *is* differs by major and is deliberately not + // asserted: @comapeo/ipc v9 hands out a fresh wrapper and leaves the + // pre-leave one permanently closed, while v10 hands back the same + // permanent reference, re-routed to a re-opened instance. + const rejoinedProjectApi = rejoinedProjectHook.result.current.data + const settingsFromReference = await rejoinedProjectApi.$getProjectSettings() + assert.strictEqual(settingsFromReference.name, 'mapeo') }, ) diff --git a/test/lib/react-query.test.ts b/test/lib/react-query.test.ts new file mode 100644 index 0000000..af96df4 --- /dev/null +++ b/test/lib/react-query.test.ts @@ -0,0 +1,47 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' + +import { + baseMutationOptions, + baseQueryOptions, +} from '../../src/lib/react-query.js' + +function errorWithCode(code: string) { + return Object.assign(new Error(code), { code }) +} + +describe('baseQueryOptions() retry', () => { + const { retry } = baseQueryOptions() + + // `RPC_CHANNEL_CLOSED` is what rpc-reflector rejects in-flight calls with + // when the transport to the backend drops. There is no other transport-level + // code - `RPC_TRANSPORT_CLOSED` has never existed. + it('retries a channel-closed rejection a bounded number of times', () => { + const error = errorWithCode('RPC_CHANNEL_CLOSED') + + expect(retry(0, error)).toBe(true) + expect(retry(2, error)).toBe(true) + expect(retry(3, error)).toBe(false) + }) + + // A left project is only usable again after re-joining via an invite, which + // no amount of retrying brings about + it('does not retry a left project', () => { + expect(retry(0, errorWithCode('PROJECT_LEFT'))).toBe(false) + }) + + it('does not retry anything else', () => { + expect(retry(0, new Error('genuinely broken'))).toBe(false) + expect(retry(0, errorWithCode('CLIENT_CLOSED'))).toBe(false) + expect(retry(0, errorWithCode('RPC_TIMEOUT'))).toBe(false) + }) +}) + +describe('baseMutationOptions()', () => { + // Re-issuing a write whose response was lost is never safe + it('never retries', () => { + expect(baseMutationOptions().retry).toBe(false) + }) +}) diff --git a/test/lib/received-map-shares-store.test.ts b/test/lib/received-map-shares-store.test.ts index f4b3e38..0438fdd 100644 --- a/test/lib/received-map-shares-store.test.ts +++ b/test/lib/received-map-shares-store.test.ts @@ -1,6 +1,7 @@ /** * @vitest-environment node */ +import { QueryClient } from '@tanstack/react-query' import { beforeEach, describe, expect, it, vi } from 'vitest' import { createMapServerApi } from '../../src/contexts/MapServer.js' @@ -40,6 +41,88 @@ describe('ReceivedMapSharesStore', () => { getBaseUrl: async () => new URL(receiver.localBaseUrl), }), }) + t.onTestFinished(store.listen()) + }) + + describe('client api listener', () => { + function createStore() { + return createReceivedMapSharesStore({ + // @ts-expect-error - We're only mocking what we need + clientApi: mockClientApi, + mapServerApi: createMapServerApi({ + getBaseUrl: async () => new URL(receiver.localBaseUrl), + }), + }) + } + + it('should not attach a listener until listen() is called', () => { + const before = mockClientApi.listeners.get('map-share')?.length ?? 0 + + const teardown = createStore().listen() + + expect(mockClientApi.listeners.get('map-share')).toHaveLength(before + 1) + + teardown() + }) + + it('should remove its listener on teardown', () => { + const before = mockClientApi.listeners.get('map-share')?.length ?? 0 + + const teardown = createStore().listen() + teardown() + + expect(mockClientApi.listeners.get('map-share')).toHaveLength(before) + }) + + it('should not register a second listener when listen() is called twice', async () => { + const before = mockClientApi.listeners.get('map-share')?.length ?? 0 + + const doubleListeningStore = createStore() + const teardown = doubleListeningStore.listen() + const secondTeardown = doubleListeningStore.listen() + + expect(mockClientApi.listeners.get('map-share')).toHaveLength(before + 1) + expect(secondTeardown).toBe(teardown) + + const serverShare = await createShare(sender, receiver) + mockClientApi.emit( + 'map-share', + createMapShareFromServerShare(sender.deviceId, serverShare), + ) + expect(doubleListeningStore.getSnapshot()).toHaveLength(1) + + teardown() + expect(mockClientApi.listeners.get('map-share')).toHaveLength(before) + }) + + it('should re-attach when listen() is called again after teardown', () => { + const before = mockClientApi.listeners.get('map-share')?.length ?? 0 + + const relisteningStore = createStore() + relisteningStore.listen()() + + const teardown = relisteningStore.listen() + expect(mockClientApi.listeners.get('map-share')).toHaveLength(before + 1) + + teardown() + expect(mockClientApi.listeners.get('map-share')).toHaveLength(before) + }) + + it('should ignore map-share events after teardown', async () => { + const serverShare = await createShare(sender, receiver) + const mapShare = createMapShareFromServerShare( + sender.deviceId, + serverShare, + ) + + const torndownStore = createStore() + const teardown = torndownStore.listen() + teardown() + + mockClientApi.emit('map-share', mapShare) + + expect(torndownStore.getSnapshot()).toHaveLength(0) + }) }) describe('subscription', () => { @@ -139,3 +222,156 @@ describe('ReceivedMapSharesStore', () => { }) }) }) + +// The event stream is the only thing that ever moves a share out of +// `downloading`. If the map server goes away for good — it dies with the +// backend on Android — the stream never re-establishes, and without an error +// path the share would sit in `downloading` for the life of the app. +describe('map share event stream failures', () => { + const MAP_SHARE = { + shareId: 'share-id', + senderDeviceId: 'sender-device-id', + senderDeviceName: 'Sender', + mapShareReceivedAt: Date.now(), + mapId: 'custom', + estimatedSizeBytes: 100, + mapShareUrls: ['http://127.0.0.1:1/'], + } + + // eventsource-client's own default, and what the map server gets unless it + // sends a `retry:` field + const DEFAULT_RECONNECT_DELAY_MS = 2_000 + + function createStoreWithFakeEventSource() { + const close = vi.fn() + const createEventSource = vi.fn() + let onScheduleReconnect: ((info: { delay: number }) => void) | undefined + let onMessage: ((event: { data: string }) => void) | undefined + + const mapServerApi = { + post: () => ({ json: async () => ({ downloadId: 'download-id' }) }), + createEventSource: (options: { + onScheduleReconnect?: (info: { delay: number }) => void + onMessage?: (event: { data: string }) => void + }) => { + createEventSource(options) + onScheduleReconnect = options.onScheduleReconnect + onMessage = options.onMessage + return { close } + }, + } + + const mockClientApi = createMockClientApi() + const store = createReceivedMapSharesStore({ + // @ts-expect-error - We're only mocking what we need + clientApi: mockClientApi, + // @ts-expect-error - We're only mocking what we need + mapServerApi, + queryClient: new QueryClient(), + }) + + return { + store, + close, + createEventSource, + async startDownload() { + await store.actions.download({ shareId: MAP_SHARE.shareId }) + }, + receiveShare() { + mockClientApi.emit('map-share', MAP_SHARE) + }, + dropStream: (delay = DEFAULT_RECONNECT_DELAY_MS) => + onScheduleReconnect?.({ delay }), + emitProgress: (bytesDownloaded: number) => + onMessage?.({ + data: JSON.stringify({ status: 'downloading', bytesDownloaded }), + }), + } + } + + // The budget is measured in seconds of outage rather than reconnect + // attempts: at the 2s default with no backoff, a handful of attempts is only + // a few seconds, which an Android backend restart routinely exceeds. + it('tolerates a stream outage far longer than a few reconnects', async (t) => { + const fake = createStoreWithFakeEventSource() + t.onTestFinished(fake.store.listen()) + fake.receiveShare() + + await fake.startDownload() + + for (let i = 0; i < 15; i++) fake.dropStream() + + expect(fake.store.getSnapshot()[0]).toHaveProperty('status', 'downloading') + }) + + it('moves the share to error once the outage budget is spent', async (t) => { + const fake = createStoreWithFakeEventSource() + t.onTestFinished(fake.store.listen()) + fake.receiveShare() + + await fake.startDownload() + expect(fake.store.getSnapshot()[0]).toHaveProperty('status', 'downloading') + + // 30s and 60s of accumulated downtime are both still within budget + fake.dropStream(30_000) + fake.dropStream(30_000) + expect(fake.store.getSnapshot()[0]).toHaveProperty('status', 'downloading') + + fake.dropStream(30_000) + + expect(fake.store.getSnapshot()[0]).toMatchObject({ + status: 'error', + error: { code: 'EVENT_STREAM_ERROR' }, + }) + expect(fake.close).toHaveBeenCalled() + }) + + it('keeps monitoring a stream that recovers between drops', async (t) => { + const fake = createStoreWithFakeEventSource() + t.onTestFinished(fake.store.listen()) + fake.receiveShare() + + await fake.startDownload() + + // The map server sends the current state as the first message on a new + // stream, so a delivered event means the outage is over + for (let i = 1; i <= 10; i++) { + fake.dropStream(30_000) + fake.dropStream(30_000) + fake.emitProgress(i) + } + + expect(fake.store.getSnapshot()[0]).toMatchObject({ + status: 'downloading', + bytesDownloaded: 10, + }) + }) + + // The stores are plain in-memory state and are never reset, so a share left + // in `error` would be a dead row for the life of the app + it('lets a share that lost its stream be downloaded again', async (t) => { + const fake = createStoreWithFakeEventSource() + t.onTestFinished(fake.store.listen()) + fake.receiveShare() + + await fake.startDownload() + fake.dropStream(90_000) + expect(fake.store.getSnapshot()[0]).toHaveProperty('status', 'error') + + await fake.startDownload() + + expect(fake.store.getSnapshot()[0]).toMatchObject({ + status: 'downloading', + bytesDownloaded: 0, + }) + // A second monitor, so the retry is watching a live stream rather than + // riding on the closed one + expect(fake.createEventSource).toHaveBeenCalledTimes(2) + + fake.emitProgress(42) + expect(fake.store.getSnapshot()[0]).toMatchObject({ + status: 'downloading', + bytesDownloaded: 42, + }) + }) +}) diff --git a/test/lib/sent-map-shares-store.test.ts b/test/lib/sent-map-shares-store.test.ts index 0a09171..e888e69 100644 --- a/test/lib/sent-map-shares-store.test.ts +++ b/test/lib/sent-map-shares-store.test.ts @@ -95,7 +95,7 @@ describe('SentMapSharesStore', () => { let receiverMockClientApi: MockClientApi let receivedStore: ReceivedMapSharesStore - beforeEach(() => { + beforeEach((t) => { receiverMockClientApi = createMockClientApi() receivedStore = createReceivedMapSharesStore({ // @ts-expect-error - We're only mocking what we need @@ -104,6 +104,7 @@ describe('SentMapSharesStore', () => { getBaseUrl: async () => new URL(receiver.localBaseUrl), }), }) + t.onTestFinished(receivedStore.listen()) }) it('should update status when receiver declines the share', async () => { @@ -348,7 +349,7 @@ describe('SentMapSharesStore', () => { let receiverMockClientApi: MockClientApi let receivedStore: ReceivedMapSharesStore - beforeEach(() => { + beforeEach((t) => { receiverMockClientApi = createMockClientApi() receivedStore = createReceivedMapSharesStore({ // @ts-expect-error - We're only mocking what we need @@ -357,6 +358,7 @@ describe('SentMapSharesStore', () => { getBaseUrl: async () => new URL(receiver.localBaseUrl), }), }) + t.onTestFinished(receivedStore.listen()) }) it('should not allow cancel after decline', async () => { diff --git a/test/lib/sync.test.ts b/test/lib/sync.test.ts new file mode 100644 index 0000000..1bb5757 --- /dev/null +++ b/test/lib/sync.test.ts @@ -0,0 +1,286 @@ +/** + * @vitest-environment node + */ +import type { ComapeoProjectClientApi } from '@comapeo/ipc' +import { describe, expect, it, vi } from 'vitest' + +import { + refreshActiveSyncStores, + SyncStore, + type SyncState, +} from '../../src/lib/sync.js' + +function createSyncState({ + isSyncEnabled = false, + devices = {}, +}: { + isSyncEnabled?: boolean + /** Sync-enabled remote devices, by outstanding `want` count */ + devices?: Record +} = {}): SyncState { + return { + data: { isSyncEnabled }, + remoteDeviceSyncState: Object.fromEntries( + Object.entries(devices).map(([deviceId, want]) => [ + deviceId, + { data: { isSyncEnabled: true, want, wanted: 0 } }, + ]), + ), + } as unknown as SyncState +} + +function createProject({ + throwOnOff = false, + throwOnOn = false, +}: { throwOnOff?: boolean; throwOnOn?: boolean } = {}) { + const on = vi.fn(() => { + if (throwOnOn) throw new Error('ProjectClosed') + }) + const off = vi.fn(() => { + if (throwOnOff) throw new Error('ProjectClosed') + }) + const getState = vi.fn(async () => { + throw new Error('ProjectClosed') + }) + + return { + project: { + $sync: { on, off, getState }, + } as unknown as ComapeoProjectClientApi, + on, + off, + } +} + +// `on`/`off` throw on a project wrapper closed by a backend restart in +// @comapeo/ipc v9, and on any reference after the whole client is closed. The +// unsubscribe runs in React effect cleanup, where a throw would take down the +// tree, so both emitter calls have to tolerate it. +describe('SyncStore with a closed project wrapper', () => { + it('does not throw from the unsubscribe returned by subscribe()', () => { + const { project, off } = createProject({ throwOnOff: true }) + const store = new SyncStore(project) + + const unsubscribe = store.subscribe(() => {}) + + expect(() => unsubscribe()).not.toThrow() + expect(off).toHaveBeenCalled() + }) + + it('does not throw from subscribe() when adding the listener fails', () => { + const { project, on } = createProject({ throwOnOn: true }) + const store = new SyncStore(project) + + expect(() => store.subscribe(() => {})).not.toThrow() + expect(on).toHaveBeenCalled() + // The failure is surfaced through the snapshot, the same way a rejected + // `getState()` is + expect(() => store.getStateSnapshot()).toThrow(/ProjectClosed/) + }) +}) + +function createLiveProject() { + const listeners = new Set<(state: SyncState) => void>() + let nextState = createSyncState() + + const on = vi.fn((_event: string, listener: (state: SyncState) => void) => { + listeners.add(listener) + }) + const off = vi.fn((_event: string, listener: (state: SyncState) => void) => { + listeners.delete(listener) + }) + const getState = vi.fn(async () => nextState) + + return { + project: { + $sync: { on, off, getState }, + } as unknown as ComapeoProjectClientApi, + on, + off, + getState, + listenerCount: () => listeners.size, + emit(state: SyncState) { + nextState = state + for (const listener of listeners) listener(state) + }, + setNextState(state: SyncState) { + nextState = state + }, + } +} + +// With a permanent project client reference (@comapeo/ipc v10) the per-project +// store cache hands back the same `SyncStore` after a backend restart, so +// anything it latched from the previous backend has to be cleared explicitly. +describe('recovering a SyncStore after a backend restart', () => { + it('clears a sticky error on a store that is still listened to', async (t) => { + const project = createLiveProject() + project.getState.mockRejectedValueOnce(new Error('ProjectClosed')) + + const store = new SyncStore(project.project) + t.onTestFinished(store.subscribe(() => {})) + + await vi.waitFor(() => { + expect(() => store.getStateSnapshot()).toThrow(/ProjectClosed/) + }) + + const fresh = createSyncState({ isSyncEnabled: true }) + project.setNextState(fresh) + refreshActiveSyncStores() + + expect(() => store.getStateSnapshot()).not.toThrow() + await vi.waitFor(() => { + expect(store.getStateSnapshot()).toBe(fresh) + }) + }) + + // The real sequence when the error reaches a boundary: the snapshot throws + // during render, the boundary unmounts the subtree, and that removes the + // last listener. The remount is handed back this same store and reads the + // snapshot during render, before `subscribe` can run — so the error must + // not still be there. + it('recovers a store whose error boundary unmounted every listener', async () => { + const project = createLiveProject() + project.getState.mockRejectedValueOnce(new Error('ProjectClosed')) + + const store = new SyncStore(project.project) + const unsubscribe = store.subscribe(() => {}) + + await vi.waitFor(() => { + expect(() => store.getStateSnapshot()).toThrow(/ProjectClosed/) + }) + + unsubscribe() + const fresh = createSyncState({ isSyncEnabled: true }) + project.setNextState(fresh) + + refreshActiveSyncStores() + + expect(() => store.getStateSnapshot()).not.toThrow() + + const unsubscribeAfterRemount = store.subscribe(() => {}) + await vi.waitFor(() => { + expect(store.getStateSnapshot()).toBe(fresh) + }) + unsubscribeAfterRemount() + }) + + // Same wedge, without a restart: a boundary that offers a retry button has + // to be able to get somewhere on its own. + it('recovers a remount with no restart notification at all', async () => { + const project = createLiveProject() + project.getState.mockRejectedValueOnce(new Error('ProjectClosed')) + + const store = new SyncStore(project.project) + const unsubscribe = store.subscribe(() => {}) + + await vi.waitFor(() => { + expect(() => store.getStateSnapshot()).toThrow(/ProjectClosed/) + }) + + unsubscribe() + + expect(() => store.getStateSnapshot()).not.toThrow() + + const fresh = createSyncState({ isSyncEnabled: true }) + project.setNextState(fresh) + const unsubscribeAfterRemount = store.subscribe(() => {}) + await vi.waitFor(() => { + expect(store.getStateSnapshot()).toBe(fresh) + }) + unsubscribeAfterRemount() + }) + + it('notifies listeners with the state from the new backend', async (t) => { + const project = createLiveProject() + const store = new SyncStore(project.project) + const listener = vi.fn() + t.onTestFinished(store.subscribe(listener)) + + await vi.waitFor(() => { + expect(store.getStateSnapshot()).not.toBeNull() + }) + listener.mockClear() + + const fresh = createSyncState({ isSyncEnabled: true }) + project.setNextState(fresh) + refreshActiveSyncStores() + + await vi.waitFor(() => { + expect(store.getStateSnapshot()).toBe(fresh) + }) + expect(listener).toHaveBeenCalled() + // The transport owner re-sends its subscriptions before announcing the + // restart, so the store must not attach a second listener + expect(project.listenerCount()).toBe(1) + }) + + // Progress is a ratio against the largest sync count seen so far. Clearing + // those baselines while keeping the previous backend's state makes the + // ratio read 1, which the UI shows as "sync complete". + it('never reports sync as complete on the way through a restart', async (t) => { + const project = createLiveProject() + const store = new SyncStore(project.project) + const seen: Array = [] + t.onTestFinished( + store.subscribe(() => { + seen.push(store.getDataProgressSnapshot()) + }), + ) + + await vi.waitFor(() => { + expect(store.getStateSnapshot()).not.toBeNull() + }) + project.emit(createSyncState({ devices: { 'device-a': 10 } })) + project.emit(createSyncState({ devices: { 'device-a': 5 } })) + expect(store.getDataProgressSnapshot()).toBe(0.5) + + project.setNextState(createSyncState({ devices: { 'device-a': 8 } })) + refreshActiveSyncStores() + + // Not known yet, rather than a number derived from the dead backend + expect(store.getDataProgressSnapshot()).toBeNull() + await vi.waitFor(() => { + expect(store.getStateSnapshot()).not.toBeNull() + }) + expect(store.getDataProgressSnapshot()).toBe(0) + expect(seen).not.toContain(1) + }) + + it('does not touch a store with no listeners', async () => { + const project = createLiveProject() + const store = new SyncStore(project.project) + const unsubscribe = store.subscribe(() => {}) + + await vi.waitFor(() => { + expect(store.getStateSnapshot()).not.toBeNull() + }) + + unsubscribe() + project.getState.mockClear() + + refreshActiveSyncStores() + + expect(project.getState).not.toHaveBeenCalled() + }) + + it('re-attaches the listener when the first attach failed', async (t) => { + const project = createLiveProject() + project.on.mockImplementationOnce(() => { + throw new Error('ProjectClosed') + }) + + const store = new SyncStore(project.project) + t.onTestFinished(store.subscribe(() => {})) + + expect(() => store.getStateSnapshot()).toThrow(/ProjectClosed/) + expect(project.listenerCount()).toBe(0) + + refreshActiveSyncStores() + + expect(project.listenerCount()).toBe(1) + await vi.waitFor(() => { + expect(store.getStateSnapshot()).not.toBeNull() + }) + }) +})