Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
167 changes: 158 additions & 9 deletions src/platform/remote/comfyui/jobs/fetchJobs.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { describe, expect, it, vi } from 'vitest'
import { beforeEach, describe, expect, it, vi } from 'vitest'

import {
extractWorkflow,
Expand All @@ -15,6 +15,15 @@ import type { z } from 'zod'

type JobsListResponse = z.infer<typeof zJobsListResponse>

function dispatchPageTransition(
type: 'pagehide' | 'pageshow',
{ persisted }: { persisted: boolean }
) {
const event = new Event(type)
Object.defineProperty(event, 'persisted', { value: persisted })
window.dispatchEvent(event)
}

function createMockJob(
id: string,
status: 'pending' | 'in_progress' | 'completed' | 'failed' = 'completed',
Expand Down Expand Up @@ -45,6 +54,10 @@ function createMockResponse(
}

describe('fetchJobs', () => {
beforeEach(() => {
dispatchPageTransition('pageshow', { persisted: true })
})

describe('fetchHistory', () => {
it('fetches completed jobs', async () => {
const mockFetch = vi.fn().mockResolvedValue({
Expand All @@ -61,7 +74,8 @@ describe('fetchJobs', () => {
const result = await fetchHistory(mockFetch)

expect(mockFetch).toHaveBeenCalledWith(
'/jobs?status=completed,failed,cancelled&limit=200&offset=0'
'/jobs?status=completed,failed,cancelled&limit=200&offset=0',
{ signal: expect.any(AbortSignal) }
)
expect(result).toHaveLength(2)
expect(result[0].id).toBe('job1')
Expand Down Expand Up @@ -112,7 +126,8 @@ describe('fetchJobs', () => {
const result = await fetchHistory(mockFetch, 200, 5)

expect(mockFetch).toHaveBeenCalledWith(
'/jobs?status=completed,failed,cancelled&limit=200&offset=5'
'/jobs?status=completed,failed,cancelled&limit=200&offset=5',
{ signal: expect.any(AbortSignal) }
)
// Priority base is total - offset = 10 - 5 = 5
expect(result[0].priority).toBe(5) // (total - offset) - 0
Expand All @@ -135,15 +150,22 @@ describe('fetchJobs', () => {
expect(result[0].priority).toBe(999)
})

it('returns empty array on error', async () => {
it('reports a network error and returns an empty array', async () => {
const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
const mockFetch = vi.fn().mockRejectedValue(new Error('Network error'))

const result = await fetchHistory(mockFetch)

expect(result).toEqual([])
expect(errorSpy).toHaveBeenCalledWith(
'[Jobs API] Error fetching jobs:',
expect.any(Error)
)
errorSpy.mockRestore()
})

it('returns empty array on non-ok response', async () => {
it('reports a server error and returns an empty array', async () => {
const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
const mockFetch = vi.fn().mockResolvedValue({
ok: false,
status: 500
Expand All @@ -152,6 +174,109 @@ describe('fetchJobs', () => {
const result = await fetchHistory(mockFetch)

expect(result).toEqual([])
expect(errorSpy).toHaveBeenCalledWith(
'[Jobs API] Failed to fetch jobs: 500'
)
errorSpy.mockRestore()
})

it('reports a malformed response body as an error', async () => {
const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
const mockFetch = vi.fn().mockResolvedValue({
ok: true,
json: () => Promise.resolve({ jobs: 'not-an-array' })
})

const result = await fetchHistory(mockFetch)

expect(result).toEqual([])
expect(errorSpy).toHaveBeenCalledWith(
'[Jobs API] Error fetching jobs:',
expect.anything()
)
errorSpy.mockRestore()
})

it('warns instead of erroring on an unauthenticated response', async () => {
const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {})
const mockFetch = vi.fn().mockResolvedValue({
ok: false,
status: 401
})

const result = await fetchHistory(mockFetch)

expect(result).toEqual([])
expect(warnSpy).toHaveBeenCalledWith(
'[Jobs API] Failed to fetch jobs: 401'
)
expect(errorSpy).not.toHaveBeenCalled()
errorSpy.mockRestore()
warnSpy.mockRestore()
})

it('does not report an aborted request', async () => {
const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
const mockFetch = vi
.fn()
.mockRejectedValue(new DOMException('Aborted', 'AbortError'))

const result = await fetchHistory(mockFetch)

expect(result).toEqual([])
expect(errorSpy).not.toHaveBeenCalled()
errorSpy.mockRestore()
})

it('does not report a request cancelled by page teardown', async () => {
const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
const mockFetch = vi.fn().mockImplementation(() => {
dispatchPageTransition('pagehide', { persisted: false })
return Promise.reject(new TypeError('Failed to fetch'))
})

const result = await fetchHistory(mockFetch)

expect(result).toEqual([])
expect(errorSpy).not.toHaveBeenCalled()
errorSpy.mockRestore()
})

it('leaves requests alone when the page is only frozen', async () => {
const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
const mockFetch = vi.fn().mockImplementation(() => {
dispatchPageTransition('pagehide', { persisted: true })
return Promise.reject(new TypeError('Failed to fetch'))
})

const result = await fetchHistory(mockFetch)

expect(mockFetch.mock.calls[0][1].signal.aborted).toBe(false)
expect(result).toEqual([])
expect(errorSpy).toHaveBeenCalledWith(
'[Jobs API] Error fetching jobs:',
expect.any(TypeError)
)
errorSpy.mockRestore()
})

it('still reports network errors after the page is restored', async () => {
const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
dispatchPageTransition('pagehide', { persisted: false })
dispatchPageTransition('pageshow', { persisted: true })
const mockFetch = vi
.fn()
.mockRejectedValue(new TypeError('Failed to fetch'))

const result = await fetchHistory(mockFetch)

expect(result).toEqual([])
expect(errorSpy).toHaveBeenCalledWith(
'[Jobs API] Error fetching jobs:',
expect.any(TypeError)
)
errorSpy.mockRestore()
})

it('parses batch containing text-only preview outputs', async () => {
Expand Down Expand Up @@ -208,7 +333,8 @@ describe('fetchJobs', () => {
const result = await fetchHistoryPage(mockFetch, 2, 5)

expect(mockFetch).toHaveBeenCalledWith(
'/jobs?status=completed,failed,cancelled&limit=2&offset=5'
'/jobs?status=completed,failed,cancelled&limit=2&offset=5',
{ signal: expect.any(AbortSignal) }
)
expect(result.jobs).toHaveLength(2)
expect(result.offset).toBe(5)
Expand Down Expand Up @@ -237,7 +363,8 @@ describe('fetchJobs', () => {
const result = await fetchQueue(mockFetch)

expect(mockFetch).toHaveBeenCalledWith(
'/jobs?status=in_progress,pending&limit=200&offset=0'
'/jobs?status=in_progress,pending&limit=200&offset=0',
{ signal: expect.any(AbortSignal) }
)
expect(result.Running).toHaveLength(1)
expect(result.Pending).toHaveLength(2)
Expand Down Expand Up @@ -295,7 +422,9 @@ describe('fetchJobs', () => {

const result = await fetchJobDetail(mockFetch, 'job1')

expect(mockFetch).toHaveBeenCalledWith('/jobs/job1')
expect(mockFetch).toHaveBeenCalledWith('/jobs/job1', {
signal: expect.any(AbortSignal)
})
expect(result?.id).toBe('job1')
expect(result?.outputs).toBeDefined()
})
Expand All @@ -311,12 +440,32 @@ describe('fetchJobs', () => {
expect(result).toBeUndefined()
})

it('returns undefined on error', async () => {
it('reports an error and returns undefined on a network fault', async () => {
const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
const mockFetch = vi.fn().mockRejectedValue(new Error('Network error'))

const result = await fetchJobDetail(mockFetch, 'job1')

expect(result).toBeUndefined()
expect(errorSpy).toHaveBeenCalledWith(
'Failed to fetch job detail for job job1:',
expect.any(Error)
)
errorSpy.mockRestore()
})

it('does not report a request cancelled by page teardown', async () => {
const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
const mockFetch = vi.fn().mockImplementation(() => {
dispatchPageTransition('pagehide', { persisted: false })
return Promise.reject(new TypeError('Failed to fetch'))
})

const result = await fetchJobDetail(mockFetch, 'job1')

expect(result).toBeUndefined()
expect(errorSpy).not.toHaveBeenCalled()
errorSpy.mockRestore()
})
})

Expand Down
47 changes: 31 additions & 16 deletions src/platform/remote/comfyui/jobs/fetchJobs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
import type { ComfyWorkflowJSON } from '@/platform/workflow/validation/schemas/workflowSchema'
import { validateComfyWorkflow } from '@/platform/workflow/validation/schemas/workflowSchema'
import type { JobId } from '@/schemas/apiSchema'
import { pageTeardownSignal } from '@/utils/pageTeardownUtil'
import { isAbortError } from '@/utils/typeGuardUtil'

import type {
JobDetail,
Expand All @@ -18,6 +20,8 @@ import type {
} from './jobTypes'
import { zJobDetail, zJobsListResponse, zWorkflowContainer } from './jobTypes'

type JobsApiFetcher = (url: string, options?: RequestInit) => Promise<Response>

interface FetchJobsRawResult {
jobs: RawJobListItem[]
total: number
Expand All @@ -39,24 +43,30 @@ export interface FetchHistoryPageResult {
* @internal
*/
async function fetchJobsRaw(
fetchApi: (url: string) => Promise<Response>,
fetchApi: JobsApiFetcher,
statuses: JobStatus[],
maxItems: number = 200,
offset: number = 0
): Promise<FetchJobsRawResult> {
const statusParam = statuses.join(',')
const url = `/jobs?status=${statusParam}&limit=${maxItems}&offset=${offset}`
const noJobs: FetchJobsRawResult = {
jobs: [],
total: 0,
offset,
limit: maxItems,
hasMore: false
}
const signal = pageTeardownSignal()
try {
const res = await fetchApi(url)
const res = await fetchApi(url, { signal })
Comment thread
mattmillerai marked this conversation as resolved.
if (!res.ok) {
console.error(`[Jobs API] Failed to fetch jobs: ${res.status}`)
return {
jobs: [],
total: 0,
offset,
limit: maxItems,
hasMore: false
}
const message = `[Jobs API] Failed to fetch jobs: ${res.status}`
// A poll that outran the session is a lifecycle state, not a fault: the
// request seam already waits for auth to resolve and re-mints once on 401.
if (res.status === 401) console.warn(message)
else console.error(message)
return noJobs
}
const data = zJobsListResponse.parse(await res.json())
return {
Expand All @@ -67,8 +77,11 @@ async function fetchJobsRaw(
hasMore: data.pagination.has_more
}
} catch (error) {
// `signal.aborted` rather than the error shape: a request cancelled while
// the page unloads can surface as a bare `TypeError: Failed to fetch`.
if (signal.aborted || isAbortError(error)) return noJobs
Comment thread
mattmillerai marked this conversation as resolved.
console.error('[Jobs API] Error fetching jobs:', error)
return { jobs: [], total: 0, offset, limit: maxItems, hasMore: false }
return noJobs
}
}

Expand All @@ -94,7 +107,7 @@ function assignPriority(
* Assigns synthetic priority starting from total (lower than queue jobs).
*/
export async function fetchHistory(
fetchApi: (url: string) => Promise<Response>,
fetchApi: JobsApiFetcher,
maxItems: number = 200,
offset: number = 0
): Promise<JobListItem[]> {
Expand All @@ -106,7 +119,7 @@ export async function fetchHistory(
* Fetches one page of history with server-provided pagination metadata.
*/
export async function fetchHistoryPage(
fetchApi: (url: string) => Promise<Response>,
fetchApi: JobsApiFetcher,
maxItems: number = 200,
offset: number = 0
): Promise<FetchHistoryPageResult> {
Expand All @@ -132,7 +145,7 @@ export async function fetchHistoryPage(
* Pending jobs get highest priority, then running jobs.
*/
export async function fetchQueue(
fetchApi: (url: string) => Promise<Response>
fetchApi: JobsApiFetcher
): Promise<{ Running: JobListItem[]; Pending: JobListItem[] }> {
const { jobs } = await fetchJobsRaw(
fetchApi,
Expand All @@ -159,11 +172,12 @@ export async function fetchQueue(
* Fetches full job details from /jobs/{job_id}
*/
export async function fetchJobDetail(
fetchApi: (url: string) => Promise<Response>,
fetchApi: JobsApiFetcher,
Comment thread
mattmillerai marked this conversation as resolved.
jobId: JobId
): Promise<JobDetail | undefined> {
const signal = pageTeardownSignal()
try {
const res = await fetchApi(`/jobs/${encodeURIComponent(jobId)}`)
const res = await fetchApi(`/jobs/${encodeURIComponent(jobId)}`, { signal })

if (!res.ok) {
console.warn(`Job not found for job ${jobId}`)
Expand All @@ -172,6 +186,7 @@ export async function fetchJobDetail(

return zJobDetail.parse(await res.json())
} catch (error) {
if (signal.aborted || isAbortError(error)) return undefined
console.error(`Failed to fetch job detail for job ${jobId}:`, error)
return undefined
}
Expand Down
4 changes: 3 additions & 1 deletion src/platform/workflow/cloud/getWorkflowFromHistory.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,9 @@ describe('fetchJobDetail', () => {

await fetchJobDetail(mockFetchApi, 'test-job-id')

expect(mockFetchApi).toHaveBeenCalledWith('/jobs/test-job-id')
expect(mockFetchApi).toHaveBeenCalledWith('/jobs/test-job-id', {
signal: expect.any(AbortSignal)
})
})

it('should return job detail with workflow and outputs', async () => {
Expand Down
Loading
Loading