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 <noreply@anthropic.com>
This commit is contained in:
@@ -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 }) => <ApiProvider client={api}>{children}</ApiProvider>
|
||||
return renderHook(({ id }) => useJob<Draft>(id), { wrapper, initialProps: { id: jobId } })
|
||||
return renderHook(({ id }) => useJob<Draft>('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 })
|
||||
})
|
||||
})
|
||||
|
||||
+73
-28
@@ -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<R> = {
|
||||
@@ -18,28 +21,56 @@ export type JobState<R> = {
|
||||
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<R> = Omit<JobState<R>, '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<R>(where: string, kind: string, jobId: string, body: unknown): Job<R> {
|
||||
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<R>(jobId: string | null): JobState<R> {
|
||||
// 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<R>(kind: string, jobId: string | null): JobState<R> {
|
||||
const { apiFetch } = useApi()
|
||||
const [state, setState] = useState<{ jobId: string | null; job: JobState<R> }>({ jobId, job: idle })
|
||||
const [state, setState] = useState<{ jobId: string | null; job: JobData<R> }>({ 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<R>(jobId: string | null): JobState<R> {
|
||||
let timer: ReturnType<typeof setTimeout> | 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<Job<R>>(`/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<unknown>(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<R>
|
||||
try {
|
||||
job = parseJob<R>(`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 }
|
||||
}
|
||||
|
||||
@@ -68,6 +68,11 @@ function sent(fetchMock: Mock<typeof fetch>) {
|
||||
.map(([input, init]) => `${init?.method} ${String(input)}`)
|
||||
}
|
||||
|
||||
// jobPolls counts the GET /api/v1/jobs/j1 requests.
|
||||
function jobPolls(fetchMock: Mock<typeof fetch>) {
|
||||
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 () => {
|
||||
|
||||
@@ -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<unknown>(startBatch.data?.job_id ?? null)
|
||||
const job = useJob<unknown>(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() {
|
||||
</div>
|
||||
{finding && <p className={styles.finding}>Finding new side quests…</p>}
|
||||
{jobRunning && batchError !== null && (
|
||||
<ErrorBanner onRetry={() => startBatch.mutate()}>Couldn't check on new side quests. {batchError.message}</ErrorBanner>
|
||||
<ErrorBanner onRetry={job.poll}>Couldn't check on new side quests. {batchError.message}</ErrorBanner>
|
||||
)}
|
||||
</section>
|
||||
)
|
||||
|
||||
@@ -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 }
|
||||
|
||||
|
||||
Reference in New Issue
Block a user