fix(model cache): release shared weights when a cache goes away - #9403
Open
lstein wants to merge 2 commits into
Open
fix(model cache): release shared weights when a cache goes away#9403lstein wants to merge 2 commits into
lstein wants to merge 2 commits into
Conversation
5 tasks
Nothing released a cache's SharedCpuWeightsStore references except
_delete_cache_entry(): shutdown() left every resident record's refcount
held, and a cache dropped without shutdown() (test teardown; any future
wiring that rebuilds caches at runtime) stranded the canonical tensors
and their accounting forever. Today's production wiring tears the store
down together with its caches, so the live exposure is cross-test
pollution of the process-global store and RAM pinned past
ModelManagerService.stop() — but the refcount invariant ('every acquire
is paired with exactly one release') was simply not upheld, and this
makes it self-healing before any wiring change turns it into a real
peer-accounting bug.
Two mechanisms, for the two ways a cache goes away:
- shutdown() now releases its resident records' shared references
synchronously — it runs in a normal thread context, so the direct
(locking) release is safe there, and teardown does not depend on a
later store operation happening.
- Each wrapper registers a weakref.finalize fallback for the
dropped-without-shutdown case. The finalizer runs in GC context,
where taking the store's non-reentrant lock could self-deadlock (a
collection can fire inside acquire()'s critical section on the same
thread — the rule ModelCache.release_first_use_grace documents), so
it only ENQUEUES into a SimpleQueue; every public store method drains
the queue under the lock. The finalizer is registered inside the
acquire's try (a registration failure must release too), its args
carry the key and canonical dict rather than the wrapper (finalize
holds args strongly — referencing self would make the wrapper
immortal), and release_shared_weights() detaches it before releasing
synchronously so eviction-then-collection releases exactly once. The
state-dict identity keeps releases correct across invalidate()'s
retired entries.
RamBudget.total_in_use() now documents why its store read must stay
outside the budget lock: the drain allocates under the store lock, so
GC can run _on_cache_collected (store→budget) there, and a
budget→store order anywhere would complete the deadlock cycle.
Six regression tests, verified to fail before the fix, covering:
shutdown releases synchronously with an empty queue; collection returns
refcount/bytes/budget to zero; the collection-time release is
enqueue-only (never applied inline by GC); eviction + collection
release exactly once across two caches; a retired (invalidated) entry
is freed by a collected holder; and the partial-load wrapper behaves
like the full-load one. One existing test relied on an abandoned
wrapper leaking its reference and now binds it.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
lstein
force-pushed
the
lstein/fix/multigpu-shared-weights-collect
branch
from
July 30, 2026 01:10
b295ec7 to
f959cef
Compare
lstein
marked this pull request as ready for review
July 30, 2026 01:14
lstein
requested review from
JPPhoto,
Pfannkuchensack,
blessedcoolant and
dunkeroni
as code owners
July 30, 2026 01:14
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-on to #9263, addressing the first of the non-blocking issues deferred from review there: a
ModelCachedropped withoutshutdown()/clear()never routes its resident records through_delete_cache_entry()— the only caller ofrelease_shared_weights()— so the process-globalSharedCpuWeightsStorekept the entry's refcount and canonical tensors forever, and every surviving peer cache saw phantom RAM in the shared budget (evicting or refusing capacity for bytes no live cache held).Design
Each cached-model wrapper now registers a
weakref.finalizefallback when it acquires shared weights. Two constraints shape the implementation:acquire()'s critical section on the same thread — the same ruleModelCache.release_first_use_gracedocuments. So the finalizer only enqueues the release into aSimpleQueue(lock-free, reentrant-safe); every public store method drains the queue under the lock, so the bytes disappear from the accounting no later than the next store operation — in particular the next budget query.release_shared_weights()detaches the finalizer before releasing synchronously, so a wrapper that was evicted and later collected cannot decrement a peer's reference. The finalizer's args carry the key and the canonical dict — not the wrapper (finalizeholds args strongly; referencingselfwould make the wrapper immortal) — and the state-dict identity check keeps the release correct acrossinvalidate()'s retired entries.Tests
Five regression tests, each verified to fail before the fix:
RamBudget.total_in_use()to zero (the test JPPhoto specified);invalidate()d while referenced) entry is freed by a collected holder via state-dict identity;CachedModelWithPartialLoadbehaves identically toCachedModelOnlyFullLoad.One existing test constructed a wrapper without binding it and relied on the abandoned wrapper leaking its reference; it now binds the wrapper.
Status
Stacked on #9263 (
lstein/feat/multi-gpu); the diff shows that branch's commits until it merges. Marked draft until then — rebase ontomainand un-draft after #9263 lands.🤖 Generated with Claude Code