From 7e2bf897a27ff9b15ffa7dcb0ff4a05a347b7064 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Wed, 7 Oct 2026 01:22:17 -0400 Subject: [PATCH] Validate the job envelope and make Retry poll the batch's job useJob now takes the job kind and checks each GET /api/v1/jobs/{id} answer: the requested id, the expected kind, a known status, a result and a text or null error. An answer that is not the job is exposed as the error and stops polling, so an unknown status no longer leaves the Discover skeleton cards running with no error or Retry. useJob also returns poll, which asks for the job at once and restarts the backoff; the Discover page's "Couldn't check on new side quests" Retry uses it instead of posting another batch. Co-Authored-By: Claude Opus 5.5 --- web/src/api/useJob.test.tsx | 85 +++++++++++++++++++-- web/src/api/useJob.ts | 101 ++++++++++++++++++------- web/src/discover/DiscoverPage.test.tsx | 46 ++++++++++- web/src/discover/DiscoverPage.tsx | 8 +- web/src/discover/api.ts | 3 + 5 files changed, 203 insertions(+), 40 deletions(-) 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 }