diff --git a/client/src/data-provider/Import/__tests__/mutations.spec.tsx b/client/src/data-provider/Import/__tests__/mutations.spec.tsx index 30c3452f72..df5fab7d51 100644 --- a/client/src/data-provider/Import/__tests__/mutations.spec.tsx +++ b/client/src/data-provider/Import/__tests__/mutations.spec.tsx @@ -156,7 +156,10 @@ describe('useStartImportMutation', () => { unmount(); }); - it('surfaces start failures through onError', async () => { + /** The server responds before it launches the run, so a failed `/start` does + * not mean the job did not start — and `awaiting_confirmation` is a phase the + * poller sits idle on, so nothing else would ever correct the panel. */ + it('surfaces start failures through onError and refetches the job to find out what happened', async () => { const mockStart = dataService.startImportJob as jest.MockedFunction< typeof dataService.startImportJob >; @@ -177,7 +180,8 @@ describe('useStartImportMutation', () => { await waitFor(() => { expect(onError).toHaveBeenCalledWith(error); }); - expect(invalidateSpy).not.toHaveBeenCalled(); + expect(invalidateSpy).toHaveBeenCalledWith([QueryKeys.importJob, 'job-1']); + expect(invalidateSpy).not.toHaveBeenCalledWith([QueryKeys.allConversations]); unmount(); }); diff --git a/client/src/data-provider/Import/__tests__/queries.spec.tsx b/client/src/data-provider/Import/__tests__/queries.spec.tsx index dbae5c1006..e5ed777aa4 100644 --- a/client/src/data-provider/Import/__tests__/queries.spec.tsx +++ b/client/src/data-provider/Import/__tests__/queries.spec.tsx @@ -36,6 +36,11 @@ function job(phase: TImportJob['phase']): TImportJob { }; } +/** The axios error shape the panel reads: only a 404 means the job itself is + * gone, and everything else is a request that failed on the way there. */ +const notFound = () => ({ response: { status: 404 } }); +const serverError = () => ({ response: { status: 503 } }); + function createWrapper(queryClient: QueryClient) { return function Wrapper({ children }: { children: ReactNode }) { return createElement(QueryClientProvider, { client: queryClient }, children); @@ -70,8 +75,24 @@ describe('importJobRefetchInterval', () => { expect(importJobRefetchInterval(undefined)).toBe(2000); }); - it('stops once the query has errored, so a 404 does not poll forever', () => { - expect(importJobRefetchInterval(undefined, { state: { status: 'error' } })).toBe(false); + it('stops once the job is gone, so a 404 does not poll forever', () => { + expect( + importJobRefetchInterval(undefined, { state: { status: 'error', error: notFound() } }), + ).toBe(false); + }); + + /** The import runs server-side; a request that failed on the way there says + * nothing about it. Giving up here would strand the panel on a job it would + * otherwise have watched finish. */ + it('keeps polling through a transient failure', () => { + expect( + importJobRefetchInterval(undefined, { state: { status: 'error', error: serverError() } }), + ).toBe(2000); + expect( + importJobRefetchInterval(undefined, { + state: { status: 'error', error: new Error('Network Error') }, + }), + ).toBe(2000); }); }); @@ -90,31 +111,48 @@ describe('fetchImportJob', () => { expect(invalidateSpy).toHaveBeenCalledWith([QueryKeys.allConversations]); }); - it('does not invalidate the conversation list for non-completed phases, including other terminal ones', async () => { + /** Neither is a rollback: a run that throws or is cancelled after a batch + * flush leaves every conversation up to that flush permanently saved, so the + * sidebar has to be refreshed even though the import did not succeed. */ + it.each(['failed', 'cancelled'] as const)( + 'invalidates the conversation list for a %s job, which may have written before stopping', + async (phase) => { + const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + const invalidateSpy = jest.spyOn(queryClient, 'invalidateQueries'); + const mockGetImportJob = dataService.getImportJob as jest.MockedFunction< + typeof dataService.getImportJob + >; + mockGetImportJob.mockResolvedValue(job(phase)); + + await fetchImportJob('job-1', queryClient); + + expect(invalidateSpy).toHaveBeenCalledWith([QueryKeys.allConversations]); + }, + ); + + it('does not invalidate the conversation list while the job is still running', async () => { const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }); const invalidateSpy = jest.spyOn(queryClient, 'invalidateQueries'); const mockGetImportJob = dataService.getImportJob as jest.MockedFunction< typeof dataService.getImportJob >; - mockGetImportJob.mockResolvedValue(job('failed')); - await fetchImportJob('job-1', queryClient); - mockGetImportJob.mockResolvedValue(job('cancelled')); - await fetchImportJob('job-1', queryClient); mockGetImportJob.mockResolvedValue(job('assets')); await fetchImportJob('job-1', queryClient); + mockGetImportJob.mockResolvedValue(job('awaiting_confirmation')); + await fetchImportJob('job-1', queryClient); expect(invalidateSpy).not.toHaveBeenCalled(); }); }); describe('useImportJobQuery', () => { - it('surfaces the error and stops polling when the job cannot be fetched', async () => { - const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + it('surfaces the error without retrying when the job is gone', async () => { + const queryClient = new QueryClient({ defaultOptions: { queries: { retryDelay: 0 } } }); const mockGetImportJob = dataService.getImportJob as jest.MockedFunction< typeof dataService.getImportJob >; - mockGetImportJob.mockRejectedValue(new Error('Request failed with status code 404')); + mockGetImportJob.mockRejectedValue(notFound()); const { result, unmount } = renderHook(() => useImportJobQuery('job-1'), { wrapper: createWrapper(queryClient), @@ -128,6 +166,26 @@ describe('useImportJobQuery', () => { unmount(); }); + it('rides out a transient failure instead of declaring the job lost', async () => { + const queryClient = new QueryClient({ defaultOptions: { queries: { retryDelay: 0 } } }); + const mockGetImportJob = dataService.getImportJob as jest.MockedFunction< + typeof dataService.getImportJob + >; + mockGetImportJob.mockRejectedValueOnce(serverError()).mockResolvedValue(job('conversations')); + + const { result, unmount } = renderHook(() => useImportJobQuery('job-1'), { + wrapper: createWrapper(queryClient), + }); + + await waitFor(() => { + expect(result.current.data?.phase).toBe('conversations'); + }); + expect(result.current.isError).toBe(false); + expect(mockGetImportJob).toHaveBeenCalledTimes(2); + + unmount(); + }); + it('stays disabled and never calls the API when jobId is null', () => { const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }); const { result, unmount } = renderHook(() => useImportJobQuery(null), { diff --git a/client/src/data-provider/Import/mutations.ts b/client/src/data-provider/Import/mutations.ts index 724ee73529..fecbbbefe1 100644 --- a/client/src/data-provider/Import/mutations.ts +++ b/client/src/data-provider/Import/mutations.ts @@ -35,7 +35,16 @@ export const useStartImportMutation = (options?: { onSuccess: (_data, jobId) => { queryClient.invalidateQueries([QueryKeys.importJob, jobId]); }, - onError: (error) => { + /** + * A failed `/start` does not mean the job did not start. The server + * responds before launching the run, so a timeout or a dropped connection + * leaves the client holding an error for an import that is underway — and + * `awaiting_confirmation` is a phase the poller deliberately sits idle on, + * so nothing would ever correct it. Refetching the job asks the server + * which of the two happened instead of assuming. + */ + onError: (error, jobId) => { + queryClient.invalidateQueries([QueryKeys.importJob, jobId]); options?.onError?.(error); }, }); diff --git a/client/src/data-provider/Import/queries.ts b/client/src/data-provider/Import/queries.ts index 11ff07dbc4..d2be81f988 100644 --- a/client/src/data-provider/Import/queries.ts +++ b/client/src/data-provider/Import/queries.ts @@ -4,9 +4,12 @@ import type { QueryClient, QueryObserverResult } from '@tanstack/react-query'; import type { TImportJob } from 'librechat-data-provider'; const POLL_MS = 2000; +/** Transient failures are retried this many times before the panel gives up + * and shows the "job lost" screen. */ +const TRANSIENT_RETRIES = 3; /** The subset of React Query's `Query` that the interval callback reads. */ -type ImportJobQueryState = { state: { status: string } }; +type ImportJobQueryState = { state: { status: string; error?: unknown } }; const TERMINAL_PHASES = new Set([ 'awaiting_confirmation', @@ -15,6 +18,17 @@ const TERMINAL_PHASES = new Set([ 'cancelled', ]); +/** Phases whose job may have written conversations to the database. `failed` + * is included deliberately: a run that throws after a batch flush leaves every + * conversation up to that flush permanently saved, so the sidebar has to be + * refreshed even though the import as a whole did not succeed. */ +const WROTE_CONVERSATIONS = new Set(['completed', 'failed', 'cancelled']); + +/** A job the server will never return again — as opposed to a request that + * failed on the way there. Only the former is worth giving up on. */ +export const isJobGone = (error: unknown): boolean => + (error as { response?: { status?: number } })?.response?.status === 404; + /** * Determines the polling cadence for an import job. * @@ -22,16 +36,19 @@ const TERMINAL_PHASES = new Set([ * conversations/assets, and while no data has arrived yet. Stops on every * terminal phase, including `awaiting_confirmation`: the job is idle there, * waiting on the user to confirm before it starts, so continuing to poll - * would burn requests for nothing. Also stops once the query has errored — - * a job that 404s (TTL expiry, cache eviction, a server restart on the - * default in-memory backend) will never appear, so retrying every two - * seconds forever only wastes requests. + * would burn requests for nothing. + * + * A 404 also stops it — a job lost to TTL expiry, cache eviction, or a server + * restart on the default in-memory backend will never reappear. Any other + * failure keeps polling: a dropped connection or a single 5xx says nothing + * about the import, which is still running server-side, and abandoning the + * poll there strands the panel on a job it would have seen finish. */ export const importJobRefetchInterval = ( data: TImportJob | undefined, query?: ImportJobQueryState, ): number | false => { - if (query?.state.status === 'error') { + if (query?.state.status === 'error' && isJobGone(query.state.error)) { return false; } if (!data) { @@ -41,9 +58,10 @@ export const importJobRefetchInterval = ( }; /** - * Fetches a single import job and, the moment a fetch reveals the job just - * finished, invalidates the conversation list so the sidebar reflects what - * the background job wrote to the database. + * Fetches a single import job and, the moment a fetch reveals the job has + * stopped running, invalidates the conversation list so the sidebar reflects + * what the background job wrote to the database — succeeded or not, since a + * run that stops partway leaves everything it already flushed behind. * * This lives in the fetcher itself rather than in `useQuery`'s `onSuccess` * config on purpose: `onSuccess` is a per-observer callback tied to React @@ -59,7 +77,7 @@ export const fetchImportJob = async ( queryClient: QueryClient, ): Promise => { const data = await dataService.getImportJob(jobId); - if (data.phase === 'completed') { + if (WROTE_CONVERSATIONS.has(data.phase)) { queryClient.invalidateQueries([QueryKeys.allConversations]); } return data; @@ -75,9 +93,14 @@ export const useImportJobQuery = ( () => fetchImportJob(jobId ?? '', queryClient), { enabled: jobId != null, - retry: false, + /** A 404 is the job's final answer and is surfaced immediately as the + * "job lost" screen; everything else gets a few attempts before the + * panel concludes anything, so one dropped request mid-import does not + * replace a running job with an error state. */ + retry: (failureCount: number, error: unknown) => + !isJobGone(error) && failureCount < TRANSIENT_RETRIES, refetchOnWindowFocus: false, - refetchOnReconnect: false, + refetchOnReconnect: true, /** * Once mounted, `refetchInterval` alone keeps an active job fresh; * a stale-but-cached job needs no forced refetch on remount. This