Skip to content

Model settings edit can leave pre-edit cache entries serving requests (commit-before-invalidate race) #9711

Open

Description

@lstein

Summary

A load-affecting model edit (cpu_only, fp8_storage) commits its record before it takes MODEL_LOAD_LOCK to invalidate the cache. A cache entry built from the pre-edit record can therefore be admitted after the commit and then kept in service, because invalidation only marks held entries stale. This predates #9694, which fences loads that start from a superseded record but leaves this window open.

Details

_update_model_record (invokeai/app/api/routers/model_manager.py, around lines 641-653):

  1. record_store.update_model(...) commits the new record.
  2. Only then does it take MODEL_LOAD_LOCK.write_lock() and call drop_model(key) on every per-device cache.

A cold load's freshness check (ModelLoadService._config_is_current, added in #9694) runs under that same write lock. A load that passes the check before step 1 commits builds and admits the pre-edit model. The edit's later drop_model then handles that entry in one of two ways:

  • Not held: it is evicted. Correct.
  • Held (locked, in its first-use window, or with an admission in flight): drop_model only sets is_stale (model_cache.py, drop_model). ModelCache.get returns a stale entry as a cache hit while it is held (model_cache.py, get: the stale-drop branch requires not is_locked and not in_first_use_window). Other requests on the same device cache keep getting the pre-edit weights until the last hold is released.

The second case applies to any held entry when a settings edit lands, not only to entries that won the race above. For example, editing cpu_only while a generation holds the model leaves the old build serving same-cache requests, such as /expand-prompt or another session on that device, until the generation releases it.

Expected

Once PATCH /api/v2/models/i/{key} returns, no new request is served a model built from the superseded load-affecting settings.

Possible directions

  • Make the commit and the invalidation one step with respect to construction. For example, take MODEL_LOAD_LOCK.write_lock() around both update_model and drop_model, after checking that holding the DB lock under the model-load lock cannot invert lock order with any existing path.
  • Have ModelCache.get treat a stale-marked entry as a miss for new requests even while it is held. Current holders keep their reference, and the next request builds a fresh entry. This needs a cache-key scheme, or a side slot, that lets the old held record and the new one coexist until the old one is released.

Tests

  • Interleave a cold load with an edit so that the load passes its freshness check, then the edit commits and invalidates. Assert that the next get for that key misses.
  • Hold an entry (lock it), apply a cpu_only edit, and assert that a new same-cache request does not receive the held, pre-edit record.

Follow-up from the review of #9694.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions