From cd5949b81bf6778c05c2aede116fdc26dcd3b785 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Tue, 13 Oct 2026 08:10:05 +0000 Subject: [PATCH] fix(ui): bound the wait for a queued model import The model editor polled a queued import forever: every status error was swallowed and retried, and nothing stopped the loop when the editor closed. A job the server had already dropped left the editor stuck on "saving". Move the job-status reading into a shared helper used by the import page and the editor. It classifies cancelled, failed (error field or an "error:" message) and finished jobs once, gives up after 10 consecutive status failures or 2 hours of waiting, and stops on unmount. The editor now shows the failure instead of looping. Assisted-by: Claude:claude-sonnet-5-5 Signed-off-by: Ettore Di Giacinto --- core/http/react-ui/src/pages/ImportModel.jsx | 22 ++--- core/http/react-ui/src/pages/ModelEditor.jsx | 29 +++--- core/http/react-ui/src/utils/importJob.js | 91 +++++++++++++++++++ .../http/react-ui/src/utils/importJob.test.js | 82 +++++++++++++++++ 4 files changed, 197 insertions(+), 27 deletions(-) create mode 100644 core/http/react-ui/src/utils/importJob.js create mode 100644 core/http/react-ui/src/utils/importJob.test.js diff --git a/core/http/react-ui/src/pages/ImportModel.jsx b/core/http/react-ui/src/pages/ImportModel.jsx index 882b19075..be079939d 100644 --- a/core/http/react-ui/src/pages/ImportModel.jsx +++ b/core/http/react-ui/src/pages/ImportModel.jsx @@ -12,6 +12,7 @@ import CodeEditor from '../components/CodeEditor' import SearchableSelect from '../components/SearchableSelect' import AmbiguityAlert from '../components/AmbiguityAlert' import ModalityChips from '../components/ModalityChips' +import { classifyImportJob } from '../utils/importJob' import Icon from '../components/Icon' import ToolSteps from '../components/tools/ToolSteps' import Checks from '../components/tools/Checks' @@ -280,32 +281,25 @@ export default function ImportModel() { pollRef.current = setInterval(async () => { try { const data = await modelsApi.getJobStatus(jobId) - if (data.cancelled || data.error || (data.message && data.message.startsWith('error:'))) { + // Failure is classified before completion: a failed or cancelled + // job also sets `processed`. Shared with the model editor. + const status = classifyImportJob(data) + if (status.state === 'failed') { transferRateRef.current.reset(jobId) clearInterval(pollRef.current) pollRef.current = null setIsSubmitting(false) setJob(null) - let msg = 'Unknown error' - if (data.cancelled) msg = 'Import cancelled' - else if (typeof data.error === 'string') msg = data.error - else if (data.error?.message) msg = data.error.message - else if (data.message) msg = data.message - if (msg.startsWith('error: ')) msg = msg.substring(7) - addToast(t('toasts.importFailed', { message: msg }), 'error') + addToast(t('toasts.importFailed', { message: status.message }), 'error') return } - // The raw /models/jobs endpoint reports `processed`, not the - // `completed` the /api/models/job shim synthesizes — checking - // only `completed` left the poll spinning after the job ended. - // Errors are handled above: failed jobs also set `processed`. - if (data.completed || data.processed) { + if (status.state === 'done') { transferRateRef.current.reset(jobId) clearInterval(pollRef.current) pollRef.current = null setIsSubmitting(false) setJob(null) - setDone({ name: data.gallery_element_name || '' }) + setDone({ name: status.name }) addToast(t('toasts.imported'), 'success') return } diff --git a/core/http/react-ui/src/pages/ModelEditor.jsx b/core/http/react-ui/src/pages/ModelEditor.jsx index 5c16f59fb..05d724e49 100644 --- a/core/http/react-ui/src/pages/ModelEditor.jsx +++ b/core/http/react-ui/src/pages/ModelEditor.jsx @@ -2,6 +2,7 @@ import { useState, useEffect, useRef, useMemo, useCallback } from 'react' import { useParams, useNavigate, useOutletContext, useSearchParams, useLocation } from 'react-router-dom' import YAML from 'yaml' import { modelsApi } from '../utils/api' +import { waitForImportJob as waitForJob } from '../utils/importJob' import { apiUrl } from '../utils/basePath' import { useConfigMetadata } from '../hooks/useConfigMetadata' import { useVramEstimate } from '../hooks/useVramEstimate' @@ -119,6 +120,14 @@ export default function ModelEditor() { const sectionRefs = useRef({}) + // Stops the wait for a queued import when the editor unmounts. Created in + // the effect as well so a StrictMode remount gets a live controller. + const importAbortRef = useRef(new AbortController()) + useEffect(() => { + importAbortRef.current = new AbortController() + return () => importAbortRef.current.abort() + }, []) + const vramEstimate = useVramEstimate({ model: name, contextSize: values['context_size'], @@ -297,19 +306,11 @@ export default function ModelEditor() { const jobId = result?.uuid || result?.ID if (!jobId) return false addToast('Import queued — downloading model assets…', 'info') - for (;;) { - await new Promise(r => setTimeout(r, 1000)) - let data - try { data = await modelsApi.getJobStatus(jobId) } catch { continue } - if (data?.cancelled) throw new Error('Import cancelled') - if (data?.error) { - throw new Error(typeof data.error === 'string' ? data.error : 'model asset download failed') - } - // The raw /models/jobs endpoint reports `processed`, not the - // `completed` the /api/models/job shim synthesizes; failed jobs - // also set `processed`, but the error branch above runs first. - if (data?.completed || data?.processed) return true - } + await waitForJob(jobId, { + getStatus: modelsApi.getJobStatus, + signal: importAbortRef.current.signal, + }) + return true } // Interactive save — uses PATCH (edit mode) or importConfig (create mode) @@ -356,6 +357,7 @@ export default function ModelEditor() { addToast('Configuration saved', 'success') } } catch (err) { + if (err?.name === 'AbortError') return addToast(`Save failed: ${err.message}`, 'error') } finally { setSaving(false) @@ -407,6 +409,7 @@ export default function ModelEditor() { } } } catch (err) { + if (err?.name === 'AbortError') return addToast(`Save failed: ${err.message}`, 'error') } finally { setSaving(false) diff --git a/core/http/react-ui/src/utils/importJob.js b/core/http/react-ui/src/utils/importJob.js new file mode 100644 index 000000000..26daa8be6 --- /dev/null +++ b/core/http/react-ui/src/utils/importJob.js @@ -0,0 +1,91 @@ +// Shared reading of a gallery job status for manual model imports. +// +// The raw /models/jobs endpoint reports `processed` (set for finished, +// failed and cancelled jobs alike), while the /api/models/job shim +// synthesizes `completed`. Failures arrive as `error` (string or object) +// or as a `message` that starts with "error:". Every import screen must +// read them the same way, so the rules live here. + +export const IMPORT_POLL_INTERVAL_MS = 1000 +// Give up on a status endpoint that keeps failing: a job the server has +// reaped or never knew answers with an error every time. +export const IMPORT_MAX_CONSECUTIVE_FAILURES = 10 +// Upper bound on one wait. The download itself keeps running server side. +export const IMPORT_MAX_WAIT_MS = 2 * 60 * 60 * 1000 + +// classifyImportJob maps one status payload to +// { state: 'running' | 'done' | 'failed', message }. +// Failure is checked before completion because a failed job also sets +// `processed`. +export function classifyImportJob(data) { + if (data?.cancelled) return { state: 'failed', message: 'Import cancelled' } + const hasErrorMessage = typeof data?.message === 'string' && data.message.startsWith('error:') + if (data?.error || hasErrorMessage) { + let msg = 'Unknown error' + if (typeof data.error === 'string') msg = data.error + else if (data.error?.message) msg = data.error.message + else if (data.message) msg = String(data.message) + if (msg.startsWith('error: ')) msg = msg.substring(7) + return { state: 'failed', message: msg } + } + if (data?.completed || data?.processed) { + return { state: 'done', message: '', name: data.gallery_element_name || '' } + } + return { state: 'running', message: '' } +} + +function sleep(ms, signal) { + return new Promise((resolve, reject) => { + if (signal?.aborted) { reject(new DOMException('Aborted', 'AbortError')); return } + const timer = setTimeout(() => { + signal?.removeEventListener('abort', onAbort) + resolve() + }, ms) + const onAbort = () => { + clearTimeout(timer) + reject(new DOMException('Aborted', 'AbortError')) + } + signal?.addEventListener('abort', onAbort, { once: true }) + }) +} + +// waitForImportJob polls getStatus(jobId) until the job ends. It resolves +// with the classified status on success and rejects with an Error when the +// job failed, was cancelled, the status endpoint kept failing, or the wait +// limit passed. An aborted signal rejects with an AbortError, which callers +// should treat as "the screen went away" and not show. +export async function waitForImportJob(jobId, { + getStatus, + signal, + intervalMs = IMPORT_POLL_INTERVAL_MS, + maxConsecutiveFailures = IMPORT_MAX_CONSECUTIVE_FAILURES, + maxWaitMs = IMPORT_MAX_WAIT_MS, + now = Date.now, +} = {}) { + const startedAt = now() + let failures = 0 + for (;;) { + await sleep(intervalMs, signal) + let data + try { + data = await getStatus(jobId) + failures = 0 + } catch (err) { + if (signal?.aborted) throw new DOMException('Aborted', 'AbortError') + failures += 1 + if (failures >= maxConsecutiveFailures) { + throw new Error(`Lost track of the import job: ${err?.message || 'status check failed'}`) + } + if (now() - startedAt >= maxWaitMs) { + throw new Error('Import is still running after waiting too long; check the models list later') + } + continue + } + const status = classifyImportJob(data) + if (status.state === 'failed') throw new Error(status.message) + if (status.state === 'done') return status + if (now() - startedAt >= maxWaitMs) { + throw new Error('Import is still running after waiting too long; check the models list later') + } + } +} diff --git a/core/http/react-ui/src/utils/importJob.test.js b/core/http/react-ui/src/utils/importJob.test.js new file mode 100644 index 000000000..c8b598117 --- /dev/null +++ b/core/http/react-ui/src/utils/importJob.test.js @@ -0,0 +1,82 @@ +import assert from 'node:assert/strict' +import test from 'node:test' + +import { classifyImportJob, waitForImportJob } from './importJob.js' + +test('reads a running job', () => { + assert.equal(classifyImportJob({ processed: false, message: 'downloading' }).state, 'running') + assert.equal(classifyImportJob(null).state, 'running') +}) + +test('reads completion from either endpoint shape', () => { + assert.equal(classifyImportJob({ completed: true }).state, 'done') + assert.deepEqual( + classifyImportJob({ processed: true, gallery_element_name: 'm' }), + { state: 'done', message: '', name: 'm' }, + ) +}) + +test('failure wins over processed', () => { + assert.deepEqual(classifyImportJob({ processed: true, error: 'boom' }), { state: 'failed', message: 'boom' }) + assert.deepEqual(classifyImportJob({ processed: true, error: { message: 'bad' } }), { state: 'failed', message: 'bad' }) + assert.deepEqual(classifyImportJob({ processed: true, message: 'error: no space' }), { state: 'failed', message: 'no space' }) + assert.deepEqual(classifyImportJob({ processed: true, cancelled: true }), { state: 'failed', message: 'Import cancelled' }) +}) + +const fast = { intervalMs: 0 } + +test('wait resolves when the job finishes', async () => { + const answers = [{ processed: false }, { processed: true }] + const status = await waitForImportJob('j', { ...fast, getStatus: async () => answers.shift() }) + assert.equal(status.state, 'done') +}) + +test('wait rejects with the job error', async () => { + await assert.rejects( + waitForImportJob('j', { ...fast, getStatus: async () => ({ processed: true, error: 'download failed' }) }), + /download failed/, + ) +}) + +test('wait gives up when the status endpoint keeps failing', async () => { + let calls = 0 + await assert.rejects( + waitForImportJob('j', { + ...fast, + maxConsecutiveFailures: 3, + getStatus: async () => { calls++; throw new Error('404 not found') }, + }), + /Lost track of the import job: 404 not found/, + ) + assert.equal(calls, 3) +}) + +test('a successful read resets the failure count', async () => { + const answers = [new Error('x'), new Error('x'), { processed: false }, new Error('x'), new Error('x'), { processed: true }] + const status = await waitForImportJob('j', { + ...fast, + maxConsecutiveFailures: 3, + getStatus: async () => { const a = answers.shift(); if (a instanceof Error) throw a; return a }, + }) + assert.equal(status.state, 'done') +}) + +test('wait stops at the time limit', async () => { + let t = 0 + await assert.rejects( + waitForImportJob('j', { + ...fast, + maxWaitMs: 5, + now: () => (t += 3), + getStatus: async () => ({ processed: false }), + }), + /still running/, + ) +}) + +test('wait rejects with AbortError when the signal fires', async () => { + const ac = new AbortController() + const p = waitForImportJob('j', { intervalMs: 10000, signal: ac.signal, getStatus: async () => ({}) }) + ac.abort() + await assert.rejects(p, err => err.name === 'AbortError') +})