diff --git a/web/src/api/useJob.test.tsx b/web/src/api/useJob.test.tsx index 21d1359..6c114f8 100644 --- a/web/src/api/useJob.test.tsx +++ b/web/src/api/useJob.test.tsx @@ -14,7 +14,7 @@ function job(status: string, result: Draft | null, error: string | null = null) function renderJob(jobId: string | null) { const api = createApiClient(fakeLocation(), createNetworkStatus(window), createVerifiedSession(localStorage)) const wrapper = ({ children }: { children: ReactNode }) => {children} - return renderHook(({ id }) => useJob(id), { wrapper, initialProps: { id: jobId } }) + return renderHook(({ id }) => useJob('plan.draft', id), { wrapper, initialProps: { id: jobId } }) } // advance runs the timers due in ms and lets the polls they start answer. @@ -42,7 +42,7 @@ describe('useJob', () => { job('succeeded', { dinners: ['Tacos', 'Chili', 'Soup'] }), ) const { result } = renderJob('j1') - expect(result.current).toEqual({ status: undefined, result: null, error: null }) + expect(result.current).toMatchObject({ status: undefined, result: null, error: null }) await advance(0) expect(result.current.status).toBe('queued') @@ -51,7 +51,7 @@ describe('useJob', () => { await advance(499) expect(fetchMock).toHaveBeenCalledTimes(1) await advance(1) - expect(result.current).toEqual({ status: 'running', result: { dinners: ['Tacos'] }, error: null }) + expect(result.current).toMatchObject({ status: 'running', result: { dinners: ['Tacos'] }, error: null }) await advance(999) expect(fetchMock).toHaveBeenCalledTimes(2) @@ -62,7 +62,7 @@ describe('useJob', () => { expect(fetchMock).toHaveBeenCalledTimes(4) await advance(2000) expect(fetchMock).toHaveBeenCalledTimes(5) - expect(result.current).toEqual({ + expect(result.current).toMatchObject({ status: 'succeeded', result: { dinners: ['Tacos', 'Chili', 'Soup'] }, error: null, @@ -106,10 +106,76 @@ describe('useJob', () => { expect(fetchMock).toHaveBeenCalledTimes(2) await advance(1) expect(fetchMock).toHaveBeenCalledTimes(3) - expect(result.current).toEqual({ status: 'succeeded', result: { dinners: ['Tacos', 'Chili'] }, error: null }) + expect(result.current).toMatchObject({ status: 'succeeded', result: { dinners: ['Tacos', 'Chili'] }, error: null }) expect(vi.getTimerCount()).toBe(0) }) + it.each([ + ['an unknown status', { id: 'j1', kind: 'plan.draft', status: 'stuck', result: null, error: null }], + ['another job', { id: 'j2', kind: 'plan.draft', status: 'running', result: null, error: null }], + ['another kind', { id: 'j1', kind: 'suggestion_batch', status: 'running', result: null, error: null }], + ['no result', { id: 'j1', kind: 'plan.draft', status: 'running', error: null }], + ['an error that is not text', { id: 'j1', kind: 'plan.draft', status: 'failed', result: null, error: 500 }], + ['not a job', ['j1']], + ])('exposes an answer with %s as the error and stops, keeping the last status', async (_, body) => { + const fetchMock = stubFetch(job('running', { dinners: ['Tacos'] }), jsonResponse(200, body)) + const { result } = renderJob('j1') + + await advance(0) + await advance(500) + expect(fetchMock).toHaveBeenCalledTimes(2) + expect(result.current).toMatchObject({ status: 'running', result: { dinners: ['Tacos'] } }) + expect(result.current.error?.message).toBe('GET /api/v1/jobs/j1: response is not the plan.draft job j1') + expect(vi.getTimerCount()).toBe(0) + await advance(10_000) + expect(fetchMock).toHaveBeenCalledTimes(2) + }) + + it('polls again at once on poll after stopping on an answer that is not the job, then keeps polling', async () => { + const fetchMock = stubFetch( + jsonResponse(200, { id: 'j1', kind: 'plan.draft', status: 'stuck', result: null, error: null }), + job('running', { dinners: ['Tacos'] }), + job('succeeded', { dinners: ['Tacos', 'Chili'] }), + ) + const { result } = renderJob('j1') + await advance(0) + expect(result.current.error).not.toBeNull() + expect(vi.getTimerCount()).toBe(0) + + act(() => result.current.poll()) + await advance(0) + expect(fetchMock).toHaveBeenCalledTimes(2) + expect(result.current).toMatchObject({ status: 'running', result: { dinners: ['Tacos'] }, error: null }) + await advance(500) + expect(fetchMock).toHaveBeenCalledTimes(3) + expect(result.current).toMatchObject({ status: 'succeeded', error: null }) + }) + + it('polls at once on poll during the backoff after a failed poll, starting the backoff again', async () => { + const fetchMock = stubFetch( + job('running', null), + errorResponse(503, 'unavailable', true), + errorResponse(503, 'unavailable', true), + job('running', { dinners: ['Tacos'] }), + job('succeeded', { dinners: ['Tacos'] }), + ) + const { result } = renderJob('j1') + await advance(0) + await advance(500) + await advance(1000) + expect(fetchMock).toHaveBeenCalledTimes(3) + + // The next poll was due 2 s later; poll asks now. + act(() => result.current.poll()) + await advance(0) + expect(fetchMock).toHaveBeenCalledTimes(4) + expect(result.current).toMatchObject({ status: 'running', result: { dinners: ['Tacos'] }, error: null }) + expect(vi.getTimerCount()).toBe(1) + await advance(500) + expect(fetchMock).toHaveBeenCalledTimes(5) + expect(result.current.status).toBe('succeeded') + }) + it('stops polling on unmount after a failed poll', async () => { const fetchMock = stubFetch(errorResponse(503, 'unavailable', true)) const { result, unmount } = renderJob('j1') @@ -125,12 +191,15 @@ describe('useJob', () => { }) it('stops polling the old job when the job id changes', async () => { - const fetchMock = stubFetch(job('running', null), job('running', null)) + const fetchMock = stubFetch( + job('running', null), + jsonResponse(200, { id: 'j2', kind: 'plan.draft', status: 'running', result: null, error: null }), + ) const { result, rerender } = renderJob('j1') await advance(0) rerender({ id: 'j2' }) - expect(result.current).toEqual({ status: undefined, result: null, error: null }) + expect(result.current).toMatchObject({ status: undefined, result: null, error: null }) await advance(0) expect(request(fetchMock, 1).path).toBe('/api/v1/jobs/j2') expect(vi.getTimerCount()).toBe(1) @@ -165,6 +234,6 @@ describe('useJob', () => { const { result } = renderJob(null) await advance(5000) expect(fetchMock).not.toHaveBeenCalled() - expect(result.current).toEqual({ status: undefined, result: null, error: null }) + expect(result.current).toMatchObject({ status: undefined, result: null, error: null }) }) }) diff --git a/web/src/api/useJob.ts b/web/src/api/useJob.ts index 2a26c2c..b918742 100644 --- a/web/src/api/useJob.ts +++ b/web/src/api/useJob.ts @@ -1,8 +1,11 @@ -import { useEffect, useState } from 'react' +import { useCallback, useEffect, useState } from 'react' import { useApi } from './ApiContext.tsx' +import { isRecord } from './errors.ts' export type JobStatus = 'queued' | 'running' | 'succeeded' | 'failed' +const STATUSES: readonly string[] = ['queued', 'running', 'succeeded', 'failed'] satisfies JobStatus[] + // Job is the GET /api/v1/jobs/{id} response. result may be set while the job // is running (a partial result); error is set only when it failed. export type Job = { @@ -18,28 +21,56 @@ export type JobState = { status: JobStatus | undefined // The latest result, partial while the job runs. result: R | null - // The job's failure, or the latest polling request's own failure (cleared - // by the next poll that answers). + // The job's failure, the latest polling request's own failure (cleared + // by the next poll that answers), or an answer that is not this job. error: Error | null + // poll asks for the job now, starting the backoff again — also after + // polling stopped on an answer that is not this job. + poll: () => void } // Polling starts at FIRST_DELAY_MS and doubles up to MAX_DELAY_MS. export const FIRST_DELAY_MS = 500 export const MAX_DELAY_MS = 2000 +type JobData = Omit, 'poll'> + const idle = { status: undefined, result: null, error: null } +// parseJob checks a GET /api/v1/jobs/{id} answer is the kind of job asked +// for, with a known status. result is passed on unread for the caller to +// check. +function parseJob(where: string, kind: string, jobId: string, body: unknown): Job { + if ( + !isRecord(body) || + body.id !== jobId || + body.kind !== kind || + typeof body.status !== 'string' || + !STATUSES.includes(body.status) || + !('result' in body) || + (body.error !== null && typeof body.error !== 'string') + ) { + throw new Error(`${where}: response is not the ${kind} job ${jobId}`) + } + return { id: jobId, kind, status: body.status as JobStatus, result: body.result as R | null, error: body.error as string | null } +} + function isTerminal(status: JobStatus): boolean { return status === 'succeeded' || status === 'failed' } -// useJob polls GET /api/v1/jobs/{jobId} until the job succeeds or fails, -// backing off from 500 ms to 2 s. A failed poll is exposed as the error and -// polling continues. Polling stops only at a terminal status, when jobId -// changes and on unmount. A null jobId polls nothing. -export function useJob(jobId: string | null): JobState { +// useJob polls GET /api/v1/jobs/{jobId}, a job of the given kind, until it +// succeeds or fails, backing off from 500 ms to 2 s. A failed poll is +// exposed as the error and polling continues. An answer that is not this +// job is exposed as the error and stops polling until poll is called. +// Polling also stops at a terminal status, when jobId changes and on +// unmount. A null jobId polls nothing. +export function useJob(kind: string, jobId: string | null): JobState { const { apiFetch } = useApi() - const [state, setState] = useState<{ jobId: string | null; job: JobState }>({ jobId, job: idle }) + const [state, setState] = useState<{ jobId: string | null; job: JobData }>({ jobId, job: idle }) + // Each poll call bumps round, which restarts the polling effect. + const [round, setRound] = useState(0) + const poll = useCallback(() => setRound((r) => r + 1), []) useEffect(() => { if (jobId === null) return @@ -47,37 +78,51 @@ export function useJob(jobId: string | null): JobState { let timer: ReturnType | undefined let delay = FIRST_DELAY_MS - function next() { - timer = setTimeout(poll, delay) - delay = Math.min(delay * 2, MAX_DELAY_MS) - } - - async function poll() { + async function fetchJob() { + const path = `/api/v1/jobs/${encodeURIComponent(jobId!)}` + let body: unknown try { - const job = await apiFetch>(`/api/v1/jobs/${encodeURIComponent(jobId!)}`) - if (!active) return - const error = job.status === 'failed' ? new Error(job.error ?? `job ${job.id} failed without an error message`) : null - setState({ jobId, job: { status: job.status, result: job.result, error } }) - if (isTerminal(job.status)) return + body = await apiFetch(path) } catch (error) { if (!active) return // A failed poll is not the job's end: keep the last known status and // result, expose the request's error, and poll again. - setState((prev) => ({ - jobId, - job: { ...(prev.jobId === jobId ? prev.job : idle), error: error instanceof Error ? error : new Error(String(error)) }, - })) + fail(error instanceof Error ? error : new Error(String(error))) + next() + return } - next() + if (!active) return + let job: Job + try { + job = parseJob(`GET ${path}`, kind, jobId!, body) + } catch (error) { + // An answer that is not this job will not become one by asking + // again: expose it and stop. + fail(error as Error) + return + } + const error = job.status === 'failed' ? new Error(job.error ?? `job ${job.id} failed without an error message`) : null + setState({ jobId, job: { status: job.status, result: job.result, error } }) + if (!isTerminal(job.status)) next() } - void poll() + // fail keeps the last known status and result, with error. + function fail(error: Error) { + setState((prev) => ({ jobId, job: { ...(prev.jobId === jobId ? prev.job : idle), error } })) + } + + function next() { + timer = setTimeout(fetchJob, delay) + delay = Math.min(delay * 2, MAX_DELAY_MS) + } + + void fetchJob() return () => { active = false clearTimeout(timer) } - }, [apiFetch, jobId]) + }, [apiFetch, kind, jobId, round]) // State left from a previous jobId is not this job's. - return state.jobId === jobId ? state.job : idle + return { ...(state.jobId === jobId ? state.job : idle), poll } } diff --git a/web/src/discover/DiscoverPage.test.tsx b/web/src/discover/DiscoverPage.test.tsx index 3589aa1..a147537 100644 --- a/web/src/discover/DiscoverPage.test.tsx +++ b/web/src/discover/DiscoverPage.test.tsx @@ -68,6 +68,11 @@ function sent(fetchMock: Mock) { .map(([input, init]) => `${init?.method} ${String(input)}`) } +// jobPolls counts the GET /api/v1/jobs/j1 requests. +function jobPolls(fetchMock: Mock) { + return fetchMock.mock.calls.filter(([input, init]) => (init?.method ?? 'GET') === 'GET' && String(input) === '/api/v1/jobs/j1').length +} + afterEach(() => { vi.unstubAllGlobals() }) @@ -419,11 +424,50 @@ describe('DiscoverPage', () => { expect(alert).toHaveTextContent("Couldn't check on new side quests.") expect(screen.getAllByTestId('skeleton-card')).toHaveLength(3) + // Retry polls the job at once, well before the next poll is due; it + // does not ask for another batch. failPoll = false + const polls = jobPolls(server.fetchMock) fireEvent.click(within(alert).getByRole('button', { name: 'Retry' })) + await waitFor(() => expect(jobPolls(server.fetchMock)).toBe(polls + 1), { timeout: 200 }) await screen.findByRole('article', { name: 'Korean beef bowls' }) expect(screen.queryByRole('alert')).not.toBeInTheDocument() - expect(sent(server.fetchMock)).toEqual(['POST /api/v1/suggestions/batches', 'POST /api/v1/suggestions/batches']) + expect(sent(server.fetchMock)).toEqual(['POST /api/v1/suggestions/batches']) + }) + + it('shows a batch whose job answers with an unknown status as an error, and Retry polls the job again', async () => { + let status = 'stuck' + const server = stubServer([], { + 'POST /api/v1/suggestions/batches': () => + jsonResponse(202, { batch_id: 'b1', job_id: 'j1', trigger: 'manual', status: 'pending' }), + 'GET /api/v1/jobs/j1': () => { + if (status === 'succeeded') server.deck = [BOWLS] + return jsonResponse(200, { + id: 'j1', + kind: 'suggestion_batch', + status, + result: { batch_id: 'b1', stage: 'done', suggestion_ids: ['s1'], dropped: 0 }, + error: null, + }) + }, + }) + await renderPage() + fireEvent.click(await screen.findByRole('button', { name: 'Get more now' })) + + const alert = await screen.findByRole('alert') + expect(alert).toHaveTextContent( + "Couldn't check on new side quests. GET /api/v1/jobs/j1: response is not the suggestion_batch job j1", + ) + // Polling has stopped: nothing more is asked until Retry. + await new Promise((resolve) => setTimeout(resolve, 700)) + expect(jobPolls(server.fetchMock)).toBe(1) + + status = 'succeeded' + fireEvent.click(within(alert).getByRole('button', { name: 'Retry' })) + await screen.findByRole('article', { name: 'Korean beef bowls' }) + expect(jobPolls(server.fetchMock)).toBe(2) + expect(screen.queryByRole('alert')).not.toBeInTheDocument() + expect(sent(server.fetchMock)).toEqual(['POST /api/v1/suggestions/batches']) }) it('shows a batch whose job result is unreadable with a retry', async () => { diff --git a/web/src/discover/DiscoverPage.tsx b/web/src/discover/DiscoverPage.tsx index 45f8a4e..77a4337 100644 --- a/web/src/discover/DiscoverPage.tsx +++ b/web/src/discover/DiscoverPage.tsx @@ -9,6 +9,7 @@ import { Icon } from '../ui/Icon.tsx' import { IconButton } from '../ui/IconButton.tsx' import { Skeleton } from '../ui/Skeleton.tsx' import { + BATCH_JOB_KIND, DECK_KEY, parseBatchResult, useDeck, @@ -73,7 +74,7 @@ export function DiscoverPage() { const questLog = useQuestLog() const decision = useDecide() const startBatch = useStartBatch() - const job = useJob(startBatch.data?.job_id ?? null) + const job = useJob(BATCH_JOB_KIND, startBatch.data?.job_id ?? null) const top = deck.data?.[0] const busy = decision.isPending @@ -89,7 +90,8 @@ export function DiscoverPage() { return { stored: 0, error: error as Error } } }, [job.result, startBatch.data?.job_id]) - // The job's failure, the latest poll's failure, or an unreadable result. + // The job's failure, the latest poll's failure, an answer that is not the + // batch's job, or an unreadable result. const batchError = job.error ?? batch.error const { mutate: mutateDecision } = decision @@ -147,7 +149,7 @@ export function DiscoverPage() { {finding &&

Finding new side quests…

} {jobRunning && batchError !== null && ( - startBatch.mutate()}>Couldn't check on new side quests. {batchError.message} + Couldn't check on new side quests. {batchError.message} )} ) diff --git a/web/src/discover/api.ts b/web/src/discover/api.ts index b0a0a77..9767eb7 100644 --- a/web/src/discover/api.ts +++ b/web/src/discover/api.ts @@ -29,6 +29,9 @@ export type Choice = 'try' | 'pass' // running it, which GET /api/v1/jobs/{id} reports on. export type Batch = { batch_id: string; job_id: string } +// BATCH_JOB_KIND is the kind of the job running a suggestion batch. +export const BATCH_JOB_KIND = 'suggestion_batch' + // BatchResult is the suggestion batch job's result, published as it goes. export type BatchResult = { batch_id: string; stage: string; suggestion_ids: string[]; dropped: number }