fix(model cache): make_room honors the admission grace; grace owned by its LoadedModel handle

invokeai/app/api/routers/model_manager.py

@@ -1524,7 +1524,9 @@ def empty_model_cache(current_admin: AdminUserOrDefault) -> EmptyModelCacheRespo
         if id(cache) in seen_cache_ids:
             continue
         seen_cache_ids.add(id(cache))
-        result = cache.make_room(1000 * 2**30)
+        # spare_awaiting_first_use=False: the user asked for a full clear, which outranks the
+        # admission grace (an in-flight loader survives via the tolerated issue-7513 path).
+        result = cache.make_room(1000 * 2**30, spare_awaiting_first_use=False)
         models_cleared += result.models_cleared
         bytes_freed += result.bytes_freed
     return EmptyModelCacheResponse(models_cleared=models_cleared, bytes_freed=bytes_freed)

invokeai/backend/model_manager/load/load_base.py

@@ -4,6 +4,7 @@
 """
 
 import time
+import weakref
 from abc import ABC, abstractmethod
 from contextlib import contextmanager
 from logging import Logger
@@ -62,13 +63,17 @@ def __init__(self, cache_record: CacheRecord, cache: ModelCache):
         self._cache_record = cache_record
         self._cache = cache
         release_first_use_grace = getattr(cache, "release_first_use_grace", None)
-        self._first_use_finalizer = (
-            finalize(self, release_first_use_grace, cache_record)
-            if cache_record.awaiting_first_use and release_first_use_grace is not None
-            else None
-        )
-        if self._first_use_finalizer is not None:
+        if cache_record.awaiting_first_use and release_first_use_grace is not None:
+            # This handle owns the grace: put()'s sweep keeps a grace whose holder is alive
+            # (an in-flight multi-model load) and clears one whose holder is gone. The finalizer
+            # carries its own ref so the release can tell whether the grace has since been
+            # re-registered to a newer, still-live handle.
+            holder_ref = weakref.ref(self)
+            cache_record.grace_holder = holder_ref
+            self._first_use_finalizer = finalize(self, release_first_use_grace, cache_record, holder_ref)
             self._first_use_finalizer.atexit = False
+        else:
+            self._first_use_finalizer = None
 
     def _lock_paced(self, working_mem_bytes: Optional[int]) -> None:
         """Move the model into VRAM in bounded passes, yielding the global load lock between them.

invokeai/backend/model_manager/load/model_cache/cache_record.py

@@ -1,3 +1,4 @@
+import weakref
 from dataclasses import dataclass
 
 from invokeai.backend.model_manager.load.model_cache.cached_model.cached_model_only_full_load import (
@@ -24,20 +25,32 @@ class CacheRecord:
     is_stale: bool = False
     # Post-admission grace: set by ModelCache.put() (unless the admission is a prefetch of a
     # model nothing will come back for) and cleared on the entry's first lock(). A freshly
-    # admitted model is about to be used — its loader calls get() as soon as put() returns, then
-    # locks it for inference — so the asynchronous eviction paths (shared-budget reconcile,
-    # peer-requested eviction) must not treat it as idle: they would evict the record out from
-    # under the in-flight load, breaking the loader's get() or detaching a live model from the
-    # cache's RAM accounting. The grace deliberately survives get(): get() is synchronized, and
-    # its own lock-release hook may run a pending reconcile before the caller can lock the record
-    # it was just handed. The flag cannot shield a record forever: the cache's local make_room
-    # path ignores it (cold loads are serialized under MODEL_LOAD_LOCK, so make_room can never
-    # see another loader's entry inside the put()->lock() window), and the next admission on the
-    # same cache clears any flag still standing (see the sweep in ModelCache.put()), so a load
-    # that errors out — or a LoadedModel dropped without ever locking — cannot dodge budget
-    # reconciles indefinitely.
+    # admitted model is about to be used — its loader calls get() as soon as put() returns,
+    # constructs a LoadedModel handle, and locks it for inference — so no eviction path
+    # (make_room, shared-budget reconcile, peer-requested eviction) may treat it as idle:
+    # evicting it frees nothing (the handle keeps the model alive) while detaching the record
+    # from the cache's RAM accounting. The window is NOT confined to a single load: multi-model
+    # invocations load their whole set (e.g. text encoder, then tokenizer, then processor)
+    # before locking any of it, so a sibling's cold load legitimately runs make_room — and
+    # put() — while earlier entries sit graced with live handles. The grace deliberately
+    # survives get(): get() is synchronized, and its own lock-release hook may run a pending
+    # reconcile before the caller can lock the record it was just handed.
+    #
+    # The flag cannot shield a record forever. Its owner is the LoadedModel handle
+    # (`grace_holder` below): a dropped handle releases the grace through its finalizer, and
+    # ModelCache.put()'s sweep clears any grace that is provably orphaned — no handle was ever
+    # registered (the load raised between put() and LoadedModel construction), or the handle
+    # died without its finalizer running (deferred worker lost) — so an orphaned record cannot
+    # dodge budget reconciles indefinitely. The keep-alive timeout clear also ignores the grace:
+    # after an idle period it is abandoned by definition.
     awaiting_first_use: bool = False
 
+    # Weak reference to the LoadedModel handle that owns `awaiting_first_use`, registered at
+    # handle construction (see LoadedModelWithoutConfig.__init__). None until then — which is
+    # exactly what put()'s sweep uses to tell an in-flight sibling (live holder: keep the grace)
+    # from an orphaned admission (no holder, or a dead one: clear it).
+    grace_holder: "weakref.ref[object] | None" = None
+
     def lock(self) -> None:
         """Lock this record."""
         self._locks += 1

invokeai/backend/model_manager/load/model_cache/model_cache.py

@@ -87,6 +87,7 @@ def _run_deferred_work(cache_ref: "weakref.ReferenceType[ModelCache]", work_queu
     while True:
         work = work_queue.get()
         cache = None
+        record = None
         try:
             if work is _DEFERRED_STOP:
                 return
@@ -98,6 +99,11 @@ def _run_deferred_work(cache_ref: "weakref.ReferenceType[ModelCache]", work_queu
                 continue
             if work is _DEFERRED_RECONCILE:
                 cache._reconcile_budget_if_pending()
+            elif isinstance(work, tuple):
+                record, holder_ref = work
+                assert isinstance(record, CacheRecord)
+                cache._release_first_use_grace(record, holder_ref)
+                holder_ref = None
             else:
                 assert isinstance(work, CacheRecord)
                 cache._release_first_use_grace(work)
@@ -106,13 +112,14 @@ def _run_deferred_work(cache_ref: "weakref.ReferenceType[ModelCache]", work_queu
                 cache._logger.exception("Error processing deferred model-cache work")
         finally:
             # Drop both references before blocking on the next get(): locals stay bound for as long
-            # as this frame lives. `work` may be a CacheRecord, which transitively holds its model's
+            # as this frame lives. `work` may be (or hold) a CacheRecord, which transitively holds its model's
             # CPU weights — and _release_first_use_grace's release hook can evict that very record,
             # removing it from the cache AND subtracting its bytes from the RamBudget, so holding it
             # would leave the budget under-reporting a model that is still resident. `cache` must go
             # for the same reason this function takes a weakref at all.
             work = None
             cache = None
+            record = None
 
 
 class _ModelLoadReadWriteLock:
@@ -593,8 +600,10 @@ def _on_timeout(self) -> None:
             )
             # Clear the cache by requesting a very large amount of space.
             # This is the same logic used by the "Clear Model Cache" button.
-            # Using 1000 GB ensures all unlocked models are removed.
-            self._make_room_internal(1000 * GB)
+            # Using 1000 GB ensures all unlocked models are removed. A surviving admission grace
+            # is abandoned by definition here — a healthy loader locks within seconds and every
+            # lock resets the keep-alive timer — so the timeout clear ignores it.
+            self._make_room_internal(1000 * GB, spare_awaiting_first_use=False)
         elif len(self._cached_models) > 0:
             # All models are locked, don't log at info level
             self._logger.debug(
@@ -650,16 +659,23 @@ def put(
         # unused cache.
         self._ensure_deferred_worker()
 
-        # Any entry still carrying the post-admission grace belongs to an earlier load: cold loads
-        # are serialized under MODEL_LOAD_LOCK's write lock and each load's only graced put() is
-        # its final one, so a flag that survives to the next admission is stale — its loader
-        # either errored out before retrieving the model or dropped the LoadedModel without ever
-        # locking it. Clear such flags so an orphaned record cannot dodge budget reconciles
-        # indefinitely. (An entry retrieved but not yet locked loses its shield here; if a
-        # reconcile then evicts it, lock() falls back to the tolerated issue-7513 path and
-        # proceeds on the detached record.)
+        # A grace that survives to the next admission is NOT necessarily stale: multi-model
+        # invocations load their whole set (text encoder, then tokenizer, then processor) before
+        # locking any of it, so earlier siblings' graces legitimately outlive later puts — and
+        # sweeping them let a sibling's make_room evict a just-admitted multi-GB model to fit a
+        # tiny one, freeing nothing (the loader's handle keeps it alive) while detaching the
+        # record from all accounting. The grace's owner is the LoadedModel handle: clear only
+        # provably orphaned graces — no handle was ever registered (the load raised between
+        # put() and LoadedModel construction), or the handle died without its finalizer running
+        # (deferred worker lost). A live holder means lock() or the finalizer will release the
+        # grace. (Residual race, tolerated via the issue-7513 path: a sibling's put() landing in
+        # the instructions between a load's MODEL_LOAD_LOCK release and its LoadedModel
+        # construction sees a grace with no holder yet and sweeps it.)
         for stale_entry in self._cached_models.values():
-            stale_entry.awaiting_first_use = False
+            if not stale_entry.awaiting_first_use:
+                continue
+            if stale_entry.grace_holder is None or stale_entry.grace_holder() is None:
+                stale_entry.awaiting_first_use = False
 
         size = calc_model_size_by_data(self._logger, model)
         self._make_room_internal(size)
@@ -819,7 +835,9 @@ def cached_model_keys(self) -> set[str]:
             if self._ram_budget is not None and self._budget_reconcile_pending.is_set() and not self._lock._is_owned():
                 self._dispatch_deferred(_DEFERRED_RECONCILE)
 
-    def release_first_use_grace(self, cache_entry: CacheRecord) -> None:
+    def release_first_use_grace(
+        self, cache_entry: CacheRecord, holder_ref: "weakref.ref[object] | None" = None
+    ) -> None:
         """Make an abandoned, never-locked record available for budget eviction.
 
         Called from a `weakref.finalize` callback (see LoadedModelWithoutConfig), which runs at an
@@ -845,7 +863,7 @@ def release_first_use_grace(self, cache_entry: CacheRecord) -> None:
         # release. Losing a race here at worst queues work that no-ops under the lock.
         if not cache_entry.awaiting_first_use:
             return
-        self._dispatch_deferred(cache_entry)
+        self._dispatch_deferred((cache_entry, holder_ref))
 
     def _ensure_deferred_worker(self) -> None:
         """Start the background worker if it is not currently running. Caller must hold the lock.
@@ -920,10 +938,25 @@ def _dispatch_deferred(self, work: object) -> None:
         self._deferred_work_queue.put(work)
 
     @synchronized
-    def _release_first_use_grace(self, cache_entry: CacheRecord) -> None:
-        """Clear an abandoned record's grace, then let the release hook reconcile the budget."""
-        if self._cached_models.get(cache_entry.key) is cache_entry and not cache_entry.is_locked:
-            cache_entry.awaiting_first_use = False
+    def _release_first_use_grace(
+        self, cache_entry: CacheRecord, holder_ref: "weakref.ref[object] | None" = None
+    ) -> None:
+        """Clear an abandoned record's grace, then let the release hook reconcile the budget.
+
+        `holder_ref` identifies the handle whose death triggered this release. When the record's
+        current `grace_holder` is a DIFFERENT, still-live handle (a newer handle was constructed
+        for the same graced record and re-registered itself), the grace belongs to that handle
+        now — leave it. (Single-slot limitation, documented: if the NEWER handle dies first while
+        an older one lives, the slot points at the dead ref and the grace clears anyway. No
+        current code path constructs two pre-lock handles for one record — one session worker per
+        device cache — so this stays a contract note, not a reachable bug.)
+        """
+        if self._cached_models.get(cache_entry.key) is not cache_entry or cache_entry.is_locked:
+            return
+        current = cache_entry.grace_holder
+        if holder_ref is not None and current is not None and current is not holder_ref and current() is not None:
+            return
+        cache_entry.awaiting_first_use = False
 
     @synchronized
     def _get_cache_snapshot(self) -> dict[str, CacheEntrySnapshot]:
@@ -1100,9 +1133,10 @@ def continue_lock(
         the last pass's.
         """
         if cache_entry.key not in self._cached_models:
-            # Same diagnostic as lock()/unlock() (issue 7513): a detached record's continuation
-            # passes should not run silently.
-            self._logger.info(
+            # Same diagnostic as lock()/unlock() (issue 7513) — but at DEBUG: lock() already said
+            # it once at INFO, and a paced stream repeats this method dozens of times, which turned
+            # one detached record into a page of identical log lines.
+            self._logger.debug(
                 f"Continuing paced lock of model cache entry {cache_entry.key} "
                 f"(Type: {cache_entry.cached_model.model.__class__.__name__}), but it has already been dropped from "
                 "the RAM cache. This is a sign that the model loading order is non-optimal in the invocation code "
@@ -1726,17 +1760,25 @@ def _log_cache_state(self, title: str = "Model cache state:", include_entry_deta
         self._logger.debug(log)
 
     @synchronized
-    def make_room(self, bytes_needed: int) -> CacheClearResult:
+    def make_room(self, bytes_needed: int, spare_awaiting_first_use: bool = True) -> CacheClearResult:
         """Make enough room in the cache to accommodate a new model of indicated size.
 
         Note: This function deletes all of the cache's internal references to a model in order to free it. If there are
         external references to the model, there's nothing that the cache can do about it, and those models will not be
         garbage-collected.
+
+        `spare_awaiting_first_use=False` (the explicit clear-cache paths) also evicts entries
+        still inside their admission grace — a user asking for a full clear outranks the grace,
+        and the tolerated issue-7513 path covers an in-flight loader.
         """
-        return self._make_room_internal(bytes_needed)
+        return self._make_room_internal(bytes_needed, spare_awaiting_first_use=spare_awaiting_first_use)
+
+    def _make_room_internal(self, bytes_needed: int, spare_awaiting_first_use: bool = True) -> CacheClearResult:
+        """Internal implementation of make_room(). Assumes the lock is already held.
 
-    def _make_room_internal(self, bytes_needed: int) -> CacheClearResult:
-        """Internal implementation of make_room(). Assumes the lock is already held."""
+        `spare_awaiting_first_use=False` (the keep-alive timeout clear) also evicts entries whose
+        admission grace was never released — abandoned by definition after an idle period.
+        """
         self._logger.debug(f"Making room for {bytes_needed / MB:.2f}MB of RAM.")
         self._log_cache_state(title="Before dropping models:")
 
@@ -1761,7 +1803,16 @@ def _make_room_internal(self, bytes_needed: int) -> CacheClearResult:
             model_key = self._cache_stack[pos]
             cache_entry = self._cached_models[model_key]
 
-            if not cache_entry.is_locked:
+            # awaiting_first_use marks the window between put() and the loader's first lock() —
+            # a window a SINGLE worker thread can re-enter this method inside, because multi-model
+            # invocations load their whole set before locking any of it (e.g. the H3 text encoder
+            # loads text_encoder, then tokenizer, then processor; the tokenizer's cold-load
+            # make_room runs with the 27GB text encoder admitted but not yet locked). Evicting
+            # such an entry frees NOTHING — the loader's handle keeps the model alive — while
+            # detaching the record from all cache accounting and turning every subsequent lock
+            # pass into an issue-7513 diagnostic. The asynchronous eviction paths (budget
+            # reconcile, peer eviction) already honor the grace; the synchronous path must too.
+            if not cache_entry.is_locked and not (spare_awaiting_first_use and cache_entry.awaiting_first_use):
                 ram_bytes_freed += cache_entry.cached_model.total_bytes()
                 self._logger.debug(
                     f"Dropping {model_key} from RAM cache to free {(cache_entry.cached_model.total_bytes() / MB):.2f}MB."

tests/app/routers/test_model_manager.py

@@ -283,9 +283,11 @@ class _Cache:
         def __init__(self, models_cleared: int, bytes_freed: int) -> None:
             self._result = CacheClearResult(models_cleared=models_cleared, bytes_freed=bytes_freed)
             self.requested: list[int] = []
+            self.spared_grace: list[bool] = []
 
-        def make_room(self, bytes_needed: int) -> CacheClearResult:
+        def make_room(self, bytes_needed: int, spare_awaiting_first_use: bool = True) -> CacheClearResult:
             self.requested.append(bytes_needed)
+            self.spared_grace.append(spare_awaiting_first_use)
             return self._result
 
     cache_0 = _Cache(models_cleared=2, bytes_freed=100)
@@ -302,6 +304,9 @@ def make_room(self, bytes_needed: int) -> CacheClearResult:
     # Both devices were actually asked to clear, not just the API thread's default cache.
     assert len(cache_0.requested) == 1
     assert len(cache_1.requested) == 1
+    # A user-requested full clear outranks the admission grace.
+    assert cache_0.spared_grace == [False]
+    assert cache_1.spared_grace == [False]
 
 
 def test_empty_model_cache_clears_duplicate_cache_objects_once(monkeypatch: Any, client: TestClient) -> None:
@@ -313,7 +318,7 @@ class _Cache:
         def __init__(self) -> None:
             self.calls = 0
 
-        def make_room(self, bytes_needed: int) -> CacheClearResult:
+        def make_room(self, bytes_needed: int, spare_awaiting_first_use: bool = True) -> CacheClearResult:
             self.calls += 1
             return CacheClearResult(models_cleared=2, bytes_freed=100)