mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-04 14:57:42 +00:00
fix(import): survive transient poll failures and refresh after partial runs
A single dropped request or 5xx put the job query into an error state that permanently disabled its interval, with retry, reconnect, focus and mount refetching all off - so the panel declared a still-running import lost and never saw it finish. Only a 404 now ends the poll; everything else retries and keeps polling. Two related gaps alongside it: a /start whose response is lost left the client on the confirmation screen for an import that had already begun, so the job is refetched on error to find out which happened; and the sidebar was only refreshed for a completed job, though a failed or cancelled run keeps every conversation it flushed before it stopped.
This commit is contained in:
parent
a16a4a57d9
commit
a4bd2ed2ae
4 changed files with 119 additions and 25 deletions
|
|
@ -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();
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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), {
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
},
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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<TImportJob['phase']>([
|
||||
'awaiting_confirmation',
|
||||
|
|
@ -15,6 +18,17 @@ const TERMINAL_PHASES = new Set<TImportJob['phase']>([
|
|||
'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<TImportJob['phase']>(['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<TImportJob['phase']>([
|
|||
* 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<TImportJob> => {
|
||||
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
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue