Repository navigation
fix(model-manager): prevent stale in-flight model loads from reentering the cache - #9694
Conversation
Cache-miss construction re-checks the model record under MODEL_LOAD_LOCK and raises StaleModelConfigError if a load-affecting edit superseded the caller's config; invocation loads re-read and retry once. The invalidation predicate moves to model_load_common. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
If 9669 merges first, this PR will need conflict resolution. |
JPPhoto
left a comment
There was a problem hiding this comment.
Merge blockers:
- None.
Other findings/issues:
-
invokeai/app/services/shared/invocation_context.py:642: In the settings-edit race, the first load emitsmodel_load_startedbut raises beforemodel_load_complete; the retry emits another start. Webv2 appends both and removes only one matching label, leaving the queue progress rail showing a load after the invocation succeeds (invokeai/frontend/webv2/src/features/models/data/modelLoadStore.ts:27-38,QueueProgressRail.tsx:20-23). Effect: stale loading indicator. Likelihood: the exact invocation race this PR handles. Recovery: a connection or account reset clears it. Test: extendtests/app/services/model_load/test_stale_model_load_fence.pyto assert no active load remains after the retry. -
invokeai/backend/model_manager/load/load_default.py:383: The new stale-config error reaches the direct/expand-promptand/image-to-promptcallers, which return HTTP 500 through generic exception handlers (invokeai/app/api/routers/utilities.py:543-547,659-663) instead of retrying with the current record. Effect: a valid prompt request fails and must be resubmitted. Likelihood: a settings edit overlaps a cache-miss load. Recovery: manual retry. Test: race each endpoint against acpu_onlyorfp8_storageedit; assert retry succeeds or the endpoint returns an intentional non-500 conflict. -
invokeai/app/services/model_load/model_load_default.py:120:_config_is_current()reads SQLite while the cache-miss path holdsMODEL_LOAD_LOCK.write_lock()(invokeai/backend/model_manager/load/load_default.py:370-383).get_model()takes the database lock, which/database/vacuumholds for the fullVACUUM(invokeai/app/services/shared/sqlite/sqlite_database.py:112-130, 241-250). If vacuum begins after a caller reads the record, this load holds the global model lock while waiting; unrelated model builds and VRAM moves stall until vacuum finishes. Effect: unrelated generation/model operations pause. Likelihood: supported admin vacuum overlapping a cache miss. Recovery: automatic when vacuum completes. Test: hold the database lock after the caller reads its config, start a cold load, and verify anotherMODEL_LOAD_LOCKreader remains blocked until release.
Suggested follow-up work (pre-existing):
invokeai/app/api/routers/model_manager.py:643: The record update commits before takingMODEL_LOAD_LOCK, as it did on the base. A load that passes the fence just before that commit can still admit the old config before invalidation; the PR body notes held stale entries may remain available. Suggested issue: close the commit/fence race and prevent held stale entries from serving later requests.
Move the one-shot re-read-and-retry on StaleModelConfigError from ModelsInterface into ModelLoadService.load_model, so direct callers (prompt utilities, image index) recover too and each load emits a single started/complete pair.
|
Thanks for the review. Addressed in e3ae73a. 1. Stale loading indicator (confirmed, fixed). The one-shot retry has moved out of 2. 3. Record read under 4. Commit-before-lock race / held stale entries (pre-existing). Agreed. I'll open a follow-up issue rather than widen this PR. |
JPPhoto
left a comment
There was a problem hiding this comment.
Other findings/issues:
invokeai/app/services/model_load/model_load_default.py:127-132:_config_is_current()reads SQLite while holdingMODEL_LOAD_LOCK’s write lock. If a cold load reaches this check during adminVACUUM, unrelated model builds and VRAM moves wait for the database lock. Effect: unrelated model operations pause; Likelihood: rare overlap; Recovery: automatic whenVACUUMends. Test: no current test covers this lock overlap.
Corner/impossible cases to note (but don't spend time on fixing):
invokeai/app/services/model_load/model_load_default.py:104-109: if a second settings edit supersedes the retry, the error propagates beforemodel_load_complete; webv2 can retain the initial loading indicator. The added second-supersession test confirms the error path. Effect: stale progress indicator; Likelihood: rare, requiring another edit during retry; Recovery: connection or account reset. The missing terminal event on load errors is a pre-existing limitation.
Suggested follow-up work (pre-existing, make a new issue):
invokeai/app/api/routers/model_manager.py:643-653: the record update still commits before acquiringMODEL_LOAD_LOCK, as it does in base. A load can pass its freshness check before the commit and race with invalidation. Suggested issue: close the commit/invalidation race and prevent held stale entries from serving later requests.
Cold loads check their record before taking the construction lock; record edits (update, reidentify) are bracketed with record_edit(), and a load re-reads under the lock only if an edit was open at its check or started since, so a long DB transaction no longer stalls unrelated model-lock users.
|
Thanks. Addressed in 615c3b9. Record read under Simply moving the read early would reopen the race this PR closes: an edit could commit after the early read but invalidate only after the load holds the lock. To prevent that, writers of load-affecting settings ( New tests:
Removing either part of the check, or either route's bracket, makes a test fail. Second supersession during the retry. Acknowledged and not changed, as you suggested. It is the existing behaviour of every failed load, which sends no terminal load event. It is listed under the PR's remaining limitations. Commit/invalidation race and held stale entries. Filed as #9711. |
Move #9669's load fingerprint into model_load_common.load_settings_changed (path resolved against models_path for the fence) and keep reidentify's cache drop inside its record_edit bracket.
JPPhoto
left a comment
There was a problem hiding this comment.
Merge blockers:
invokeai/app/services/model_load/model_load_default.py:170-175: When an edit is active or its generation changes,still_current()rereads the database.invokeai/backend/model_manager/load/load_default.py:374-385calls it while holdingMODEL_LOAD_LOCK. If a same-key edit is waiting on SQLite during a long transaction, the cold load holds the global lock while waiting too; unrelated model builds and VRAM moves stall. Effect: model loading pauses until the transaction ends. Likelihood: rare overlap of a queued cold load, same-key edit, and long transaction. Recovery: automatic when the transaction finishes. Test: the existing database-wait test has no activerecord_edit; add a barrier test for that overlap and verify unrelated lock users can proceed.
Other findings/issues:
-
invokeai/backend/model_manager/load/load_default.py:281-286: The path-existence check runs before the new stale-config fence. If an in-flight request holds the old config after files are moved and the record is updated to a valid new path, the missing old path raisesFileNotFoundErrorbefore the service can retry. This check predates the PR, but the new retry promise does not cover path edits. Effect: that request fails despite the valid updated record. Likelihood: plausible during a concurrent path update. Recovery: a fresh request uses the new path. Test: move the model, update its record, then load with the stale config and verify it retries successfully. -
invokeai/app/services/model_load/model_load_default.py:129-140: If a second edit supersedes the retry,_load()raises aftermodel_load_started;model_load_completeis skipped. Effect: webv2 can keep showing the load until a connection or account reset. Likelihood: rare second edit during the retry. Recovery: reconnect or reset the account. Test: extendtest_record_superseded_again_during_retry_raisesto verify a terminal event clears the loading state.
…MODEL_LOAD_LOCK Cold loads wait for in-progress edits of their model before reading its record, and the check under the lock only consults in-memory edit state; edits report whether they changed how the model loads, so renames no longer reject loads. Also retry a load whose files moved with its record, and always emit model_load_complete.
|
Thanks. All three addressed in 26f58ea. Blocker: record re-read under
Tests:
Path edits and the existence check (fixed). If a load fails with
Missing terminal event after a second supersession (fixed).
The PR description is updated, including the remaining limitation: a cold load waits while that same model is being edited. |
JPPhoto
left a comment
There was a problem hiding this comment.
Other findings/issues:
invokeai/app/services/model_load/model_load_default.py:39-41-_generationsretains an entry for every key that has had a load-affecting edit, even after the model is deleted. Effect: gradual memory growth with high unique-key churn. Likelihood: low for typical use; it grows with the number of distinct edited keys. Recovery: restart; consider safe entry retirement. Test: edit and delete many distinct keys, then verify the registry stays bounded.
…ad needs it The load check holds a watcher on its model from before MODEL_LOAD_LOCK until after it, and a model's entry is dropped once it has no edits in progress and no watchers, so deleted models no longer accumulate. The fence tests hold a write transaction through the new database layer instead of the removed store._db.
|
Thanks. Addressed in 774aa8e. Unbounded edit tracking (confirmed, fixed). A model's edit count only matters to a load that has read the record but not yet done its check under
Tests:
The same commit also fixes the two fence tests that broke when main's #9702 removed |
JPPhoto
left a comment
There was a problem hiding this comment.
Other findings/issues:
invokeai/app/services/model_load/model_load_default.py:196-198- The service installs the stale-load check only onModelLoader. The registry contract accepts subclasses ofModelLoaderBase(model_loader_registry.py:43-47), so a custom loader registered directly from that base gets no fence. If a load-affecting edit overlaps its cold load, it can still admit a model built from the superseded record. Likelihood is low for documented integrations, which inheritModelLoader; impact is stale loads for directModelLoaderBaseextensions. Either enforce the narrower loader contract or expose the fence through the base contract. Add a regression test for a directly registeredModelLoaderBaseimplementation.
… check Only ModelLoader checks a cold load against edits of its record before caching it. The registry now rejects any other class at registration, and the load service refuses one supplied by a custom registry.
|
Thanks. Addressed in f588938. Loaders built directly on
Tests:
Removing either check makes its test fail. Separately, the |
## Summary `test_workflow_last_run_at.py::test_update_last_run_at_sets_timestamp` fails intermittently, mostly on Windows CI. - The fixture stores the 43 bundled workflows, then the test creates one workflow and expects it in the first 10 results of a listing sorted newest first by `created_at`. - `created_at` has millisecond resolution. On Windows' ~15 ms clock the new workflow usually gets the same timestamp as the bundled ones. - Ties sort by `workflow_id` descending. A random UUID sorts above the bundled `default_...` ids only about 1 time in 7 or 8, so the new workflow usually falls off the page and the test's `next(...)` raises `StopIteration`. The listing now includes only user workflows, which is what the check is about. Nothing else changes. ## Related Issues / Discussions Seen as the `windows-cpu` failure on #9694. The test came in with #9702. ## QA Instructions - Forced tie (a throwaway test setting every row's `created_at` to the same value), 20 runs each: the old query lost the workflow in 19, the new one in none. - `uv run --no-sync pytest tests/app/services/workflow_records`: 137 passed. - `ruff check` and `ruff format --check` are clean. ## Review No material findings. This is a one-line test change. ## Checklist - [x] _The PR has a short but descriptive title, suitable for a changelog_ - [x] _Meaningful regression coverage added / updated where needed; obsolete tests/code removed_ - [ ] _Persisted-state and API changes include required migrations / compatibility validation_ - [ ] _Relevant performance/efficiency opportunities considered; material claims have evidence_ - [x] _Material review findings resolved and relevant checks rerun_ - [ ] _Documentation added / updated (if applicable)_ - [ ] _Updated `What's New` copy (if doing a release after this PR)_
Summary
A model load could read its record and then wait for the model-load lock. Meanwhile a load-affecting edit (e.g.
cpu_only,fp8_storage) could commit and invalidate the cache. The load then built and admitted the pre-edit model, and later requests reused its stale weights.What a cold load does now:
StaleModelConfigErrorinstead of building. Nothing is reserved or admitted.ModelLoadService.load_modelthen waits for the edit to finish, re-reads the record and retries once. Every caller gets this: invocations, the prompt utility routes, the image index and convert.model_load_completeis sent however the load ends, so UIs never keep showing a load that has finished or failed.No database access under
MODEL_LOAD_LOCK: a long DB transaction (e.g. admin VACUUM) never stalls unrelated model builds or VRAM moves.PATCH /i/{key}and reidentify) wrap their write and any cache invalidation inModelLoadService.record_edit(key). They report on the yieldedRecordEditwhether the write changed anything that loads.Other details:
services/model_load/model_load_common.py, so cache invalidation and the load check share one definition. The load check resolvespathagainst the models directory, because the loader rewritesconfig.pathto absolute in place.ModelLoader.config_checkis an optional instance attribute thatModelLoadServicesets. It is not a constructor argument, so registered loaders that override__init__are unaffected.ModelLoader, as the docs already required, because that is where the check runs.ModelLoaderRegistry.registerrejects any other class with aTypeError, andModelLoadServicerefuses one supplied by a custom registry.Related Issues / Discussions
Closes #9674. Follow-up to #9669. Remaining pre-existing race tracked in #9711.
QA Instructions
uv run --no-sync pytest tests/app/services/model_load/test_stale_model_load_fence.pypasses 14 tests. A fixture records, for every record read made by a loading thread, whether that thread heldMODEL_LOAD_LOCK; the overlap tests assert none did. The tests cover:MODEL_LOAD_LOCK, so another thread can take it, and then loads the edit;MODEL_LOAD_LOCK;model_load_complete;ModelLoaderBaseis refused before it can cache anything, with the started/complete pair still sent.tests/backend/model_manager/load/test_model_loader_registry_entries.pychecks thatregisterrejects a function and a directModelLoaderBasesubclass without changing the registry.tests/app/routers/test_model_manager.pyandtest_update_model_record_cache_invalidation.pycheck thatPATCH /i/{key}commits and drops insiderecord_edit, that reidentify replaces inside it, and that the edit is flagged load-affecting exactly for fields in the fingerprint.OMP_NUM_THREADS=4 uv run --no-sync pytest -n 4 tests/app/services/model_load tests/backend/model_manager tests/app/routers tests/app/services/model_records tests/app/services/model_installgave 3424 passed and 113 skipped, plus one failure intest_model_install.py::test_restore_metadata_timeout_preserves_partial_download_for_retry. That test fails intermittently on its own as well and exercises code this PR doesn't touch.ruff checkandruff format --checkare clean.Review
Findings resolved during review:
context.models.load()into the load service. Direct callers such as/expand-promptand/image-to-promptno longer return 500, and the loading indicator no longer sticks after a retry.MODEL_LOAD_LOCK, including when a load overlaps an edit of the same model. Reidentify writes are bracketed like edits.model_load_complete.Remaining limitations, which predate this change:
ModelCache.getkeeps serving it to same-cache requests until the hold is released. Tracked in Model settings edit can leave pre-edit cache entries serving requests (commit-before-invalidate race) #9711.Compatibility / Rollout
Any writer of load-affecting model-record fields must use
record_editand reportload_affecting(see theModelLoadServiceBase.record_editdocstring).model_load_completenow also marks a failed load's end; both web UIs only use it to clear the loading indicator, and webv2 also refreshes its model list.Checklist
What's Newcopy (if doing a release after this PR)🤖 Generated with Claude Code