Repository navigation
fix(model-install): preserve install tmpdir when a single multi-file part fails - #9591
lorenzozanee wants to merge 8 commits into
Conversation
JPPhoto
left a comment
There was a problem hiding this comment.
@lorenzozanee Thanks for this PR. I'd like you to address the following findings/issues and then re-request review from me:
-
invokeai/app/services/model_install/model_install_default.py:1525: Auto-pausing writes no model-install event; frontend only logsdownload_error, so UI remainsdownloadingwith no recovery action. Effect: stale UI. Likelihood: normal multi-file 5xx failure. Recovery: manual refresh/reconnect. Test: assert client state changes topausedwithout refetch. -
invokeai/app/services/model_install/model_install_default.py:1525: After restart, an early resumed-part failure can overwrite the marker with emptyfilesbecauseinstall_job.download_partsis empty. ETag/Last-Modified are lost; the nextRangerequest lacksIf-Rangeand can silently append bytes from changed remote content. Effect: possible corrupt model. Likelihood: plausible after restart with an existing partial. Recovery: delete partial and reinstall. Test: restore paused marker with partial plus ETag, fail before_download_started_callback, then verify metadata survives andIf-Rangeis sent. -
docs/src/content/docs/development/Architecture/model-manager.mdx:650: Docs still claim model installs never preserve partial downloads, now false. Effect: inaccurate recovery/storage guidance. Likelihood: normal recoverable multi-file failure. Recovery: update docs. Test: docs check must remove that claim.
Suggestions:
- Consider copying parent download parts into the install job before writing the paused marker.
- Consider emitting and handling a dedicated model-install paused event.
|
@lorenzozanee Are you still active and interested in making this contribution? |
|
@lstein Did you address this in v7? |
|
I did a re-review as things have been moving along in Fixed in
Other findings/issues:
Suggestions:
|
Preserve partial-download validators across resumed failures.\nEmit paused events for frontend recovery and cover restart and stale-UI cases.
|
@lstein I completed the implementation. Can you review and merge if all is good? |
Merge origin/main into restore/pr-9488 while preserving its existing PR history.
lstein
left a comment
There was a problem hiding this comment.
Thanks for picking this back up. The new model_install_paused event, the resume-metadata snapshot, and the docs update address the three items from the previous review. Reviewed at 8e34bb995d; the existing model-install suite passes. The main problem from #9488 is still here, though: an automatic failure leaves the job in the non-terminal PAUSED state.
Blocker
1. Once any part has completed, every later part error is parked as PAUSED, including permanent ones (model_install_default.py:2510-2521)
The new branch only checks whether recoverable data is on disk. It never looks at the exception. HF repos download small files like config.json first, so in practice almost any later failure now pauses: 401/403/404, the out-of-disk RuntimeError (download_default.py:619), the unsafe-filename ValueError, the "File exists" OSError.
Reproduced with a probe: part A (aaa_config.json) returns 200, part B returns 403 (and separately 404). Result:
job.status == PAUSED,job.error/job.error_typeareNone- no
ModelInstallErrorEvent, oneModelInstallPausedEvent wait_for_job(job, timeout=3)raisesTimeoutError- a second
import_modelwith the same source returns the same paused job (dedupe atmodel_install_default.py:1190)
Consequences:
flux/flux_redux.py:159andsd/ip_adapter.py:223auto-install withwait_for_job(job, timeout=600). These used to fail fast. Now each invocation blocks 10 minutes and raisesTimeoutError, and every later invocation dedupes onto the same paused job and blocks another 10 minutes, until a user cancels or resumes it in the install queue.- The failure cause is lost. A user who resumes a 403 or disk-full install loops straight back to
PAUSEDwith no explanation.
Suggested fix (as recommended on #9488): keep the job terminal (_set_error + error event), skip only the _safe_rmtree, and write the install marker with status PAUSED. _find_reusable_tmpdir and _remove_dangling_install_dirs only skip/sweep COMPLETED/ERROR/CANCELLED markers, so the next retry of the same source reuses the preserved parts. If you want to keep the paused UX, then at minimum: (a) pause only on transient errors (5xx, 429, connection/chunked errors, the rename race), (b) still record error/error_type and emit the error information, and (c) make the wait_for_job callers stop waiting on PAUSED.
Medium
2. Both frontends update a paused row in place and never refetch it, so an auto-pause looks like a user pause
- webv2
installsStore.ts:398-413, webv1setEventListeners.tsx:421-439
The event carries only {id, source}. The handlers set status = 'paused' on the cached row but don't reload download_parts. So the failed part never appears: there's no failed-file line and no "Restart this file", and the row offers Resume instead of "Retry failed download". In the resume_required path, webv1's stale parts make Resume visible; clicking it re-pauses.
InstallQueueBar.browser.test.tsx:220-257 masks this. Its pre-pause REST snapshot already contains an errored part, which can't exist before the failure that triggers the pause, and the test asserts listModelInstalls is called only once. Keep the instant status projection, but also scheduleRefresh() (webv2) or invalidate ModelInstalls (webv1) when the job is known.
Low
3. The resume-metadata snapshot lets one part's error delete a sibling's .downloading partial (model_install_default.py:634-644 with download_default.py:796-805, 895-916)
In-process resume_job parts now carry download_path. When the active part errors, _mfd_error cancels the waiting siblings and _cleanup_cancelled_job unlinks download_path + ".downloading" for each one. Before this PR those parts had download_path=None, so cleanup did nothing. Sequence: a job holds partials for B and C → resume_job → B returns 503 → C's partial is deleted and C restarts from zero. That works against the goal of the PR. (The restore-from-marker path already had this behavior.)
4. No test for a permanent error. test_multifile_install_part_error_preserves_tmpdir only uses a 500. Please add cases asserting that a 4xx and a local RuntimeError/ValueError end in ERROR, with the waiter returning. Also note the multifile test doesn't exercise the .downloading half of _tmpdir_has_recoverable_data, because its good part is already complete.
5. Nits
- webv1
setEventListeners.tsx:403,412:clearModelInstallPause(id)runs just before!isModelInstallPaused(id), so that guard is always true. - The snapshot also gives completed parts their ETag, so on resume
_do_downloadmarks themresume_from_scratchwith "Partial file missing. Restarted download…" (download_default.py:500-504). Cosmetic, since nothing surfaces it today.
Checked and fine
Siblings still writing after an error (parts are sequential and _mfd_error terminates the parent first), pause/cancel/shutdown racing the error callback, duplicate pause events, stale-ETag corruption (a 200 response takes the existing resume_required path), restart_failed/resume_job guards, generated OpenAPI/schema contracts, paused handling in status switches and bulk actions, id-based event matching, and fence clearing on reconnect and account switch.
…ed data Permanent part errors fail the install again; transient ones pause it when staged bytes exist on disk, and wait_for_job returns on a settled pause. Auto-install callers resume a paused install; both UIs refetch on pause, show the failed file's cause, and offer Resume unless a range was refused. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
@lstein The latest commit addresses the earlier permanent-error and pause-refresh issues, but creates/exposes a bunch of other issues. It's your call whether these get fixed or not, as some seem pretty unlikely to me:
-
invokeai/app/services/model_install/model_install_default.py:708andinvokeai/app/services/download/download_default.py:604: Resume fetches current Hugging Face metadata, but an existing completed file is accepted when its size matches. If the repo’smainchanges while the job is paused, a same-size replacement can be combined with files from the new revision. Effect: a mixed-version model. Likelihood: plausible after a delayed retry. Recovery: reinstall from one revision. Fix: pin the install to its initial commit. Test: change mockedmainbetween attempts while keeping a completed file’s size constant; verify resume does not mix revisions. -
invokeai/frontend/webv1/src/services/events/setEventListeners.tsx:445andinvokeai/frontend/webv1/src/services/api/endpoints/models.ts:332: A late list response can overwrite the UI’s completed status. The pause fence is cleared on resume or completion, so an older in-flight response can merge its stale status without triggering a follow-up refresh. Effect: the row can show downloading or paused until refresh. Likelihood: a slow list request and fast resume. Recovery: refresh the page. Fix: keep a status generation fence through terminal events. Test: delay a list response across pause, resume, and completion. -
invokeai/app/services/model_install/model_install_default.py:765andinvokeai/app/invocations/flux/flux_redux.py:163: If a user pauses an install while its download callback is still pending, an auto-install invocation gets a resume conflict, assumes another invocation is resuming it, thenwait_for_job()returns because it checks resume operations but not the pending callback. The invocation fails and does not retry after the callback settles. Effect: generation fails despite the auto-resume path. Likelihood: overlap with a slow pause. Recovery: retry generation. Fix: distinguish callback teardown from a competing resume, then retry resume if the job is still paused. Test: hold the callback pending during an auto-install resume attempt. -
docs/src/content/docs/development/Architecture/model-manager.mdx:426: The event table updated by this PR still lists nonexistent names (model_install_downloading,model_install_running,model_install_completed) and documents the parts field assource; the backend uses different event names andurl. Effect: consumers following the reference can miss events or read the wrong field. Likelihood: normal for integrations using the docs. Fix: align the table with the registered event schemas. Test: check documented names and fields against those schemas.
Corner/impossible cases to ignore/not fix:
invokeai/app/services/model_install/model_install_default.py:612: After a server refuses a range request and the app restarts,resume_requiredremains only in private marker metadata; the restored job has nodownload_parts. The UI therefore offers Resume instead of Retry, requiring one more refused request before Retry appears. Likelihood: restart after a refused range. Recovery: resume once, then retry the part.
Suggested follow-up work (pre-existing, create a new issue to track):
invokeai/app/services/model_install/model_install_common.py:245andinvokeai/app/api/routers/model_manager.py:1217:source.access_tokenis included in existing install events and the admin-only/installresponse. Redact it across client serializers if admin clients should not share these credentials.
Fence stale install statuses, align event docs, and bound callback waits with actionable timeout handling.
Summary
A single part error in a multi-file install deletes the entire install tmpdir. A transient failure on one file (an HTTP 5xx, or the sidecar rename race from #9432) discards parts that already completed and
.downloadingpartials holding resumable progress, so the next attempt restarts the whole install from zero.The error path now preserves the tmpdir when it contains anything worth keeping (a completed part file or a non-empty partial): the install is marked paused and the install marker is persisted, so restarting failed files and resume can pick up where the download left off. When nothing on disk is worth keeping, the tmpdir is still removed as before.
Related Issues / Discussions
Closes #9481
QA Instructions
New regression test (fails on the previous HEAD, passes with this change):
Surrounding suites and lint:
Result: 52 passed (35 model install, 17 download queue), ruff clean.
Merge Plan
Standard merge.
Checklist
What's Newcopy (if doing a release after this PR)Supersedes #9488(原PR分支已删,按原提交重建)