Skip to content

fix(model cache): keep patch models alive while they are patched in - #9397

Closed
lstein wants to merge 76 commits into
invoke-ai:mainfrom
lstein:lstein/fix/lora-unlocked-use
Closed

fix(model cache): keep patch models alive while they are patched in#9397
lstein wants to merge 76 commits into
invoke-ai:mainfrom
lstein:lstein/fix/lora-unlocked-use

Conversation

@lstein

@lstein lstein commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Stacked on #9263 — the branch sits on top of lstein/feat/multi-gpu, so the diff below includes that PR's commits. Base is main and it stays a draft until #9263 merges, at which point it is rebased and marked ready — same pattern as #9387#9389. Review only the last commit; everything else belongs to #9263.

The problem

Patch models are used without ever locking their cache record. A patch iterator calls context.models.load(), reads .model, yields the ModelPatchRaw and lets the LoadedModel wrapper go — and LayerPatcher.apply_smart_model_patches materializes the iterator before applying anything, so every wrapper is already gone before the first patch lands, while the patches stay applied for the whole denoise.

The LoadedModel wrapper is the cache's only signal that such a record is in use: the lock count is zero, so ModelCache.release_first_use_grace (added in 7ffee4d on #9263) treats the record as abandoned as soon as the wrapper is collected, and a peer's budget reconcile is then free to evict it. Because ModelPatchRaw is not an nn.Module, _delete_cache_entry takes the remove_non_shared path and drops the bytes from the shared RamBudget while the RAM is still resident — the cap under-counts and over-admits, which is the exact failure RamBudget exists to prevent — plus a disk reload on the LoRA's next use.

This is a regression introduced by that commit: with a single LoRA there is no later put() on that cache, so the post-admission grace previously survived the whole generation.

The fix

Make the wrapper outlive the patch. LayerPatcher gains a PatchSpec type alias:

PatchSpec = Union[Tuple[ModelPatchRaw, float], Tuple[ModelPatchRaw, float, object]]

Producers pass the LoadedModel as an optional third element instead of del-ing it, and apply_smart_model_patches — which already materializes the iterable and holds it for the entire patched region — pins it for exactly as long as the patches are applied. When the patched region exits, patches is released, the wrappers die, and the grace is released normally.

Two-element specs remain valid, so callers whose patches are not cache-backed (the patcher tests, and anything owning the model's lifetime itself) are unaffected.

Why it touches so many files

There is no central place to do this: only the producer holds the LoadedModel. The change is 16 patch iterators across 15 invocation files — every model family's denoise and text-encoder path (FLUX, FLUX2, Z-Image, Qwen, Anima, WAN, SD3, SDXL/compel, tiled MD) — plus layer_patcher.py. Each edit is identical: append lora_info to the yielded tuple, drop the del, retype the iterator to Iterator[PatchSpec].

The WAN expert-swapper path works unchanged: LoRAIteratorFactory is retyped and produces a fresh iterator per swap, so each re-entry pins its own wrappers.

Tests

tests/backend/patches/test_layer_patcher_shared_weights.py:

  • test_patch_spec_cache_handle_is_held_for_the_patched_region — the spec's handle must stay alive for the whole with block and be released after it. Verified to fail (the patch's cache handle was released while its patch was still applied) when the pinning is reverted by dropping the third element before materializing.
  • test_two_element_patch_specs_are_still_accepted — back-compat for non-cache-backed callers.

Full suite green; ruff check and format clean.

🤖 Generated with Claude Code

lstein and others added 30 commits May 31, 2026 23:26
Run one generation session per configured GPU concurrently, with a tiled
progress preview. Multi-user isolation is unchanged. Backed by five seams:

- Per-thread device context (TorchDevice.set/get/clear_session_device);
  choose_torch_device() consults it first, so all device-selecting call sites
  resolve to the calling worker's GPU with no per-node changes.
- Per-device model caches: build_model_manager builds one ModelCache per
  generation device; ModelLoadService.ram_cache resolves by current thread
  device; ram_caches fans out clear/drop/shutdown.
- Atomic concurrent dequeue: a dequeue lock makes select+claim atomic so
  concurrent workers never claim the same item (works on FIFO; round-robin
  from invoke-ai#9086 slots in later).
- Worker pool: one _SessionWorker per device, each pinning torch.cuda.set_device
  and its session device, with its own runner and cancel event; cancellation
  routes via an {item_id -> worker} lookup. Single-device installs keep the
  exact legacy single-worker behavior. Profiling disabled when >1 worker.
- New config `generation_devices`; unset = legacy single-worker mode.

Frontend: the canvas staging area already tiles per queue item; the main
ImageViewer now tracks progress per session and renders a tile grid
(ProgressImageTiles) when more than one session is active.

Also adds a lock to ObjectSerializerForwardCache for concurrent access.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
test_model_load_device_routing mutated the process-wide get_config()
singleton (device = "cuda:0") to exercise the per-thread cache routing,
but never restored it. The leaked CUDA device was then picked up by a
later test (test_model_load::test_loading) via choose_torch_device(),
which crashed with "Torch not compiled with CUDA enabled" on the
CUDA-less CI runner. Add an autouse fixture to save/restore device and
clear any pinned session device.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…n_devices

Regenerate openapi.json (make frontend-openapi) and the frontend
schema.ts types (make frontend-typegen) so they include the new
generation_devices config field, fixing the openapi-checks and
typegen-checks CI jobs.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`make frontend-openapi` used a bare `python` from a different environment
that emitted the CacheStats @DataClass docstring as a schema description.
CI generates the schema via `uv run`, which does not, so openapi-checks
failed on the diff. Regenerate with the uv-locked environment to drop the
stray description while keeping the generation_devices field.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…o prevent meta-device corruption

Parallel multi-GPU session workers could intermittently crash with "unrecognized
device meta" (denoise) or "Cannot copy out of meta tensor; no data!" (l2i), because
model loading relies on process-global, non-thread-safe monkey-patches.

accelerate.init_empty_weights() (used directly by the loaders and implicitly by
diffusers' default low_cpu_mem_usage=True in from_pretrained) swaps
torch.nn.Module.register_parameter globally for the duration of a load, routing every
newly-registered parameter to the meta device. The model cache's VRAM load/unload runs
nn.Module.load_state_dict(assign=True), whose assign path does setattr -> __setattr__ ->
register_parameter. When one worker's VRAM move overlapped another worker's from_pretrained,
the move's real weights got hijacked onto meta and blew up on the next .to(device).

Introduce MODEL_LOAD_LOCK, a write-preferring readers-writer lock:
- write lock = model construction (_load_and_cache, load_model_from_path), exclusive.
- read lock  = VRAM load/unload (ModelCache.lock(), repair_required_tensors_on_device).

VRAM transfers across GPUs still overlap each other; they only block while a construction
holds the write lock. The lock is always acquired before any per-cache lock to keep a
consistent order and avoid an AB-BA deadlock with the writer's make_room/put.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ions

Image.open() is lazy: it reads the header but defers pixel decoding (and
holds the file handle open) until the first .load()/.copy()/.convert(). The
opened object was cached and the same object handed to every caller, so in
multi-GPU parallel mode two session-processor worker threads could call
.copy() on it concurrently and race on the shared file handle and decoder
state. This surfaced as "broken data stream when reading image file" and
"AssertionError: self.png is not None" during inpainting with batch >1.

Force the decode (image.load()) before the object enters the cache so the
cached object is safe for concurrent reads, and guard the cache structures
(__cache / __cache_ids) with a lock since they are now mutated from multiple
threads.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The generation progress bars (under the Invoke button and the Viewer tab)
both read a single global $lastProgressEvent atom, which every session
overwrites. With parallel multi-GPU sessions this made the bar jump back
and forth between sessions.

Track progress per queue item id and render one bar per in-flight session,
stacked vertically, each removed as its session reaches a terminal state.

- stores.ts: add $progressEvents (map keyed by item_id),
  $activeProgressEvents (sorted), and set/clear helpers.
- setEventListeners.tsx: populate per-item progress on invocation_progress;
  clear per item on terminal status; clear all on connect/disconnect/queue
  cleared.
- ProgressBar.tsx: render a vertical stack of bars (one per active session)
  with a single-bar fallback for the idle / model-loading window; add
  containerProps so dockview tabs can position the stack.
- Dockview tab call sites: move positioning into containerProps.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
$progressEvents is only referenced within stores.ts (via the
$activeProgressEvents computed and the set/clear helpers), so exporting
it tripped knip's unused-exports check.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
With 4 GPUs the stacked per-session progress bars grew past the bottom
strip of the dockview tab and overlapped the "Viewer" label.

Add a fitHeightPx prop: in fit mode the stack is capped to the available
strip (10px below the ~40px tab's centered label) and the bars flex to
share it, shrinking below their natural height only once they no longer
fit. With 1-2 sessions the bars keep their familiar thin height; with 3+
they scale down to stay within the strip. The sidebar bar is unaffected
and continues to stack at natural height (it has the vertical room).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…fault

generation_devices now accepts "auto" (the new default), which expands to
every visible CUDA device — so multi-GPU parallel generation works out of
the box without manually listing devices. On GPU-less systems "auto"
resolves to the single cpu/mps device, preserving serial behavior.

- config_default.py: type is now Union[Literal["auto"], list[str]],
  default "auto"; validator accepts "auto" or a list of device strings.
- devices.py: add TorchDevice.get_generation_devices(), the single resolver
  that expands "auto", normalizes, and deduplicates.
- session_processor / model_manager: both consumers use the resolver
  instead of iterating the raw config value (which would have iterated the
  characters of the "auto" string).
- Regenerated docs/src/generated/settings.json.
- Tests for the resolver (auto-with/without-CUDA, dedup, empty).

An explicit single-device list (e.g. [cuda:0]) or an empty list opts out
of parallelism.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add a badges UI in the Generation section of the Settings dialog for
choosing which devices `generation_devices` should use, modeled on the
Log Namespaces toggle UI.

Backend:
- New `GET /api/v1/app/generation_device_options` endpoint listing the
  selectable devices (cuda:N with GPU names, or the sole mps/cpu fallback).
- Add `generation_devices` to the runtime-config update allowlist with
  validation rejecting invalid device strings and explicit nulls.

Frontend:
- New SettingsGenerationDevices component with active/inactive badges.
  "Auto (all GPUs)" is exclusive; removing the last explicit device
  reverts to auto. Admin/multiuser gated; notes restart requirement.
- Wire into the Generation section; regenerate schema; add en strings.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Split the restart sentence into its own string and render it bold so
users notice that device changes require restarting InvokeAI.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Render device badges as "cuda:0 (RTX 3090 #1)" so identical cards can be
told apart. Strips the "NVIDIA GeForce" vendor prefix and adds a 1-based
"#N" suffix only when multiple cards share a name. The full device name
remains available as the badge tooltip.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Help users track which CUDA device is processing each session:

- Model-load log: "Loaded model ... onto cuda device #N in ..s"
- Denoise progress bars: "Denoising (#N)" across all architectures
  (SD1.5/SDXL, FLUX, FLUX2, Z-Image, Anima, SD3, CogView4)
- Progress preview circle: GPU number centered in the ring, via a new
  `device` field on InvocationProgressEvent (resolved from the worker's
  thread-local session device)
- Session Queue: new "GPU #" column between STATUS and TIME, backed by a
  `device` column on session_queue (migration_32) recorded when a worker
  claims an item

Adds TorchDevice.get_session_device_label()/get_session_device_index()
helpers and a frontend getCudaDeviceIndex() parser (with tests). Shows the
number on CUDA only; CPU/MPS show nothing.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
# Conflicts:
#	invokeai/frontend/web/src/features/gallery/components/ImageViewer/CurrentImagePreview.tsx
#	invokeai/frontend/web/src/features/gallery/components/ImageViewer/context.tsx
#	invokeai/frontend/web/src/services/events/setEventListeners.tsx
Resolve migration_32 conflict: main's migration_32 (model_relationships FK
repair) is kept, and the multi-gpu device-column migration is moved to
migration_33 and registered after it.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rcles

- Startup log lists each generation device with its GPU number and id,
  e.g. "Using torch device: [AMD Radeon PRO W7900 #1 (cuda:0), ...]".
  Single-device setups keep the bare device name.
- Canvas progress circles now show the CUDA device index in the center,
  matching the viewer panel.
- Progress-circle tooltips show the device name and number on hover.
- Both are hidden when only a single GPU is available.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… caches

In multi-GPU mode the model manager builds one ModelCache per generation device,
each with storage_device="cpu" and its own RAM-resident copy of every model. A model
loaded on N GPUs therefore occupied N copies in RAM, and each cache sized itself
against max_cache_ram_gb independently, so RAM use during the text/reference-image
encoding phases skyrocketed and the system swapped — worst when two images rendered
at once.

This deduplicates the CPU-resident weights and makes RAM accounting global.

- SharedCpuWeightsStore: process-/manager-global, refcounted store of one canonical
  CPU state_dict per model key. The first device to load a key registers its weights;
  subsequent devices adopt the canonical tensors and re-point their module's params at
  them (load_state_dict(assign=True)), freeing the duplicate. Weights live once in RAM
  regardless of GPU count; freed only when the last device releases. Per-device modules
  are kept (params are device-shuffled in place, so two GPUs need two modules), but
  their CPU-resident params alias the shared tensors.

- RamBudget: single system-wide RAM authority. Splits RAM into shared (counted once via
  the store) and non-shared (per-instance). ModelCache eviction now runs against the
  global, deduplicated total and re-checks availability each iteration, since evicting a
  model another device still holds frees no RAM. build_model_manager wires one store +
  one budget into all device caches; the cap is max_cache_ram_gb as a true system-wide
  limit, else the sum of per-cache heuristics. Passing ram_budget=None preserves the
  prior local accounting.

- LoRA/patch safety: direct LoRA patching did an in-place copy_ on the weight, which
  would corrupt the now-shared canonical tensor (and taint keep_ram_copy even with one
  GPU) when patching a CPU-resident weight. Switched to an out-of-place add (memory-
  equivalent) so the canonical tensor is never mutated; fixed the FluxControlLoRA
  expansion path to target the module's live parameter. Sidecar patching and
  FreeU/Seamless (which patch forward methods) were already safe.

Validated on 2x AMD W7900 / ROCm: correct inference on both GPUs from one shared copy
(full + partial load + Q8_0 GGUF quantized), concurrent load/unload without corruption,
and LoRA isolation across devices. ~40 new tests; existing suites unchanged.

Adds scripts/multigpu_ram_driver.py to drive concurrent dual-GPU generations via the
queue API and measure peak RSS / leak drift.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…(multi-GPU)

With one session-processor worker per device, multiple queue items can be in_progress
at once. cancel_by_batch_ids(), cancel_by_destination() and cancel_by_queue_id() excluded
in_progress rows from their bulk UPDATE and then canceled only the single get_current()
item (LIMIT 1), so on multi-GPU the other running items kept consuming a GPU and could
still produce output after the user requested cancellation.

Each running item must be canceled via _set_queue_item_status(), which emits the
QueueItemStatusChangedEvent that the processor maps to the worker running that item_id and
uses to set its cancel event. Add _cancel_in_progress_matching() to cancel every in-progress
item matching the same filter (with user-id scoping preserved) and call it from all three
bulk-cancel methods. The returned `canceled` count now includes canceled in-progress items.

Adds regression tests that dequeue two items onto separate devices and assert every bulk
cancel API moves all matching in_progress items to canceled and emits a cancel event for
each (and that user-scoped cancel leaves another user's in-progress item running).

Reported by JPPhoto in review of invoke-ai#9263.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…vice guards, refcount leak)

Fixes from the code review of PR invoke-ai#9263:

- Cancellation could be silently lost around dequeue: the per-iteration
  worker.cancel_event.clear() ran AFTER dequeue + gc.collect() + logging, so a cancel
  arriving in that window was set by the status handler and then wiped. Move the clear to
  before dequeue, and after claiming an item re-check (cancel_event + a fresh DB status read
  via _is_queue_item_terminal) and skip running if it is already terminal, closing both race
  windows. The runner's stale queue_item.status check could not catch this.

- delete_by_destination only stopped one in-progress item (get_current) before deleting all
  matching rows, leaving other GPU workers running (and then failing to update a deleted row).
  Cancel every matching in-progress item via _cancel_in_progress_matching first.

- generation_devices validation: a bare non-"auto" string (e.g. "cuda:0") was iterated
  character-by-character; an empty list silently fell back to one device. Reject both with a
  clear message.

- get_generation_devices now fails fast on a CUDA device that does not exist (index past
  device_count, or CUDA unavailable) instead of starting a worker that errors cryptically at
  first allocation.

- Shared-weights wrappers: if the canonical re-point (load_state_dict assign=True) threw after
  acquire(), the reference was leaked (the wrapper never entered the cache). Compute size
  metadata first, make acquire the last step, and release on failure.

Adds tests for each: post-dequeue terminal guard, delete_by_destination cancellation,
generation_devices validation, absent-device rejection, and acquire-released-on-repoint-failure.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Apply ruff 0.11.2 formatting to the files flagged by `ruff format --check`.
- The new fail-fast guard in get_generation_devices() (reject a CUDA device that
  doesn't exist) made the pre-existing test_get_generation_devices_explicit_list_is_deduplicated
  fail on CPU-only CI runners, since it passes a cuda list with no CUDA present. Mock
  torch.cuda.is_available/device_count in that test (matching the existing pattern in this
  file) so it validates dedup on any runner.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
lstein and others added 10 commits July 24, 2026 18:53
…cile

The deferred reconcile request was recorded pre-admission and honored only
by the peer's next lock release. Two interleavings could strand the shared
RAM budget above its cap indefinitely:

- Lost wakeup: the busy peer releases its lock (running its reconcile hook
  while the flag is still unset) before request_budget_reconcile() sets the
  flag; if the peer then stays idle, no future release honors the request.
- Pre-admission clearing: a peer's reconcile could run between the request
  and the new model being counted, see the budget as satisfied, and clear
  the flag before the admission pushed usage over the cap.

Fix both by (1) moving the reconcile request to the end of put(), after the
new model is counted, so peers always evaluate the true budget state, and
(2) having request_budget_reconcile() attempt the reconcile inline with a
non-blocking lock acquire: either the peer's lock is free now and the
reconcile runs immediately, or it is still held and the eventual release
hook — which runs strictly after the flag is set — performs it.

The prior regression test masked the race by touching cache_b.stats after
the request; it now emulates the production release hook in the holder
thread and asserts reconciliation with no subsequent cache access, and a
new test forces the lost-wakeup interleaving by delaying the request until
the peer's operation has fully finished.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Addresses the three lingering issues from review of the deferred
budget-reconcile mechanism:

1. Manual lock releases bypass the reconcile hook. cached_model_keys()
   and evict_unlocked_for_peer() acquire/release _lock without the
   synchronized decorator, so a reconcile request whose inline attempt
   failed on their held lock was stranded when they released. Both now
   run the same reconcile hook after their manual release (non-blocking,
   preserving cached_model_keys' no-stall guarantee and avoiding the
   hold-A-block-on-B deadlock shape in evict_unlocked_for_peer).

2. clear() can wipe a concurrent request. A reconciler observing a
   satisfied budget could clear the pending flag just after a peer's
   admission (already counted, budget negative) set it, and the peer's
   inline attempt then saw the flag unset and returned — leaving the
   budget exceeded with no pending request. The reconcile now runs as a
   loop with a single guarded clear site: because admissions are counted
   before the flag is set, a negative budget re-check immediately after
   the clear proves a request may have been wiped; the flag is restored
   and reconciliation continues. This covers both former clear sites
   (satisfied early-out and post-eviction).

3. No reconcile trigger when the admitting cache itself holds the
   overshoot. put() requests reconciles from peers only, so when the
   exceeded budget was held by the admitting cache's own locked entry,
   no pending request existed anywhere and the eventual unlock ran its
   hook with the flag unset. unlock() now records a reconcile request on
   its own cache whenever it completes with the shared budget exceeded,
   so the entry that just became evictable triggers the reconcile.

Supporting change: put() admitting a model while a peer's reconcile
request is already pending must not let its own release hook evict the
just-admitted entry before the loader's immediately-following get()
(that would break the in-flight load with an IndexError). CacheRecord
gains an awaiting_first_use grace flag, set on admission and cleared on
first get()/lock(), which the asynchronous eviction paths (budget
reconcile, peer-requested eviction) skip. The local make_room path
ignores it: cold loads are serialized under MODEL_LOAD_LOCK, so it can
never see another loader's entry inside the put()->get() window, and
this bounds the flag's lifetime if a load errors out in between.

Each new regression test was verified to fail against the previous
implementation.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s stall-free

Addresses the three issues from JPPhoto's 2026-07-27 review:

1. Prefetched submodels can no longer shield the budget forever. The SD
   single-file loader's proactive submodel put()s are now admitted with
   prefetch=True (no post-admission grace), since nothing ever get()s or
   lock()s them. As a backstop, put() sweeps stale grace flags from prior
   loads — cold loads are serialized under MODEL_LOAD_LOCK, so any flag
   still standing at the next admission belongs to a dead load (errored
   before get(), or LoadedModel dropped before lock()) and is cleared.

2. The grace now survives get() and ends at lock(). get() is synchronized,
   so clearing the flag inside it let get()'s own release hook run a
   pending reconcile and evict the very record it had just selected —
   detaching a live model from the cache and its RAM accounting before the
   caller could lock it. load_default also retrieves immediately after
   put() so no failure in between can orphan a graced record.

3. cached_model_keys()'s manual-release hook hands a pending reconcile to
   a short-lived background thread instead of running it inline:
   reconciliation evicts models and calls gc.collect(), which would break
   the method's no-stall contract and pause session dequeue.

Each new regression test verified to fail with its mechanism reverted.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Conflict in image_files_disk.py: main's staged-delete protocol (invoke-ai#9163)
supersedes the old delete() body; its cache eviction in stage_delete()
now takes this branch's __cache_lock, preserving the multi-worker
thread-safety the locked removal in the old delete() provided.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
release_first_use_grace() is invoked from a weakref.finalize callback, so it
runs at an arbitrary decref/garbage-collection point in an arbitrary thread.
Making it @synchronized therefore made ModelCache._lock — and, through the
decorator's release hook, a full budget reconcile — reachable from anywhere.

That inverts the lock order RamBudget documents as impossible. The hook's
_reconcile_budget_if_pending reads RamBudget.available() ->
SharedCpuWeightsStore.total_bytes_in_use(), both plain non-reentrant locks. A
thread inside SharedCpuWeightsStore.acquire() holds the store lock while summing
tensor sizes, an allocation loop that trips generational GC; if that collection
reclaims an abandoned wrapper belonging to another device's cache, the release
hook re-enters the store lock the thread is already holding and the thread
deadlocks against itself, still holding it. Every other cache then blocks on its
next _delete_cache_entry -> release_shared_weights(). Reproduced on a two-cache
budget: the collecting thread wedges in total_bytes_in_use() and never returns.

The same hook also ran evictions, gc.collect() and empty_cache() inline in
whatever unrelated thread happened to drop the reference — including the API
event loop — undoing the no-stall contract cached_model_keys() was just given.

Do no locking work in the callback: hand the release to a short-lived background
thread, exactly as cached_model_keys() does with its own pending reconcile. The
thread may wait on the cache lock and do the slow work; the collecting thread
returns immediately.

The existing abandoned-wrapper test now polls for the (asynchronous) release.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-ups from an adversarial review of 55a6fc4:

- Thread.start() can raise RuntimeError under thread/process limits. A
  weakref.finalize callback gets no retry (weakref retires it before invoking
  it) and its exceptions go to sys.unraisablehook, so the release was silently
  lost and the record kept shielding an idle cache. Fall back to clearing the
  flag inline under a non-blocking acquire, which takes no store or budget lock
  and so still cannot deadlock the collecting thread. No reconcile on that path
  by design: a pending request stays set for the next cache operation.

- The regression test's outcome was a pure function of the ambient allocation
  count: nothing pinned the cycle between its creation and the collector thread,
  so an automatic gen-0 pass landing in the setup reclaimed it on the main
  thread and the test passed vacuously (or tripped its own setup assertions).
  Under an allocation-shifting plugin it failed at 6 of 12 offsets. Disable
  automatic gc across the setup so only the explicit collect reclaims the cycle;
  the same sweep is now 12 of 12 passing, and the test still fails against
  7ffee4d with the expected re-entrancy report.

- Correct the docstring: Thread.start() waits for the child to bootstrap, so the
  guarantee is "no lock waits", not "returns immediately".

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added CI-CD Continuous integration / Continuous delivery docker api python PRs that change python files Root invocations PRs that change invocations backend PRs that change backend files services PRs that change app services frontend-deps PRs that change frontend dependencies frontend PRs that change frontend files installer PRs that change the installer docs PRs that change docs labels Jul 28, 2026
@lstein
lstein changed the base branch from lstein/feat/multi-gpu to main July 28, 2026 17:40
Patch models are used without ever locking their cache record: an iterator
calls context.models.load(), reads `.model`, yields the ModelPatchRaw and lets
the LoadedModel wrapper go. LayerPatcher.apply_smart_model_patches materializes
that iterator before applying anything, so every wrapper is already gone before
the first patch lands, while the patches stay applied for the whole denoise.

The wrapper is the cache's only signal that such a record is in use — the lock
count is zero — so ModelCache.release_first_use_grace treats the record as
abandoned as soon as it is collected and a peer's budget reconcile may evict it.
ModelPatchRaw is not an nn.Module, so _delete_cache_entry takes the
remove_non_shared path and drops the bytes from the shared RamBudget while the
RAM is still resident: the cap under-counts and over-admits, the exact failure
RamBudget exists to prevent, plus a disk reload on next use.

Make the wrapper outlive the patch. LayerPatcher gains a PatchSpec alias whose
optional third element is the cache handle that must be pinned; producers pass
the LoadedModel there instead of deleting it, and apply_smart_model_patches —
which already materializes the iterable and holds it for the entire patched
region — keeps the record live for exactly as long as its patch is applied.
Two-element specs stay valid for callers whose patches are not cache-backed.

There is no central place to do this: only the producer holds the wrapper, so
all 16 iterators across the 15 invocation files change identically.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@lstein
lstein force-pushed the lstein/fix/lora-unlocked-use branch from 73d500a to 2bded03 Compare July 28, 2026 17:42
@github-actions github-actions Bot added the python-tests PRs that change python tests label Jul 28, 2026
@lstein

lstein commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator Author

Superseded by JPPhoto's 1142430 on the #9263 branch, which lands a strictly stronger version of this fix: instead of keeping the LoadedModel wrapper alive (which only preserved the post-admission grace), the patch specs now carry a model_in_ram() context manager that takes a real cache lock for the patched region — protecting warm records from peer eviction too, across the same producers plus the model families added since (anima, krea2, flux2). Reviewed and hardened in d63c1f3 / 272a2ef.

@lstein lstein closed this Jul 29, 2026
@lstein
lstein deleted the lstein/fix/lora-unlocked-use branch July 29, 2026 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api backend PRs that change backend files CI-CD Continuous integration / Continuous delivery docker docs PRs that change docs frontend PRs that change frontend files frontend-deps PRs that change frontend dependencies installer PRs that change the installer invocations PRs that change invocations python PRs that change python files python-tests PRs that change python tests Root services PRs that change app services

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants