Fix concurrent ModelManager mutations and LRU cache synchronization

[KushagraKanaujia]

Summary

This PR fixes critical concurrency bugs in ModelManager and WithFixedSizeCache identified in issue #2819.

Problem Statement

The current implementation has several race conditions that can cause:

• Split lock generations: Multiple locks for the same model_id, losing mutual exclusion
• Duplicate LRU queue entries: Concurrent cold adds creating duplicate entries
• Queue/manager desynchronization: Failed removals leaving inconsistent state
• Decorator mutation bypass: Decorators bypassing in-progress mutations

Impact

These bugs can lead to:

• Duplicate model loading (memory/VRAM spikes)
• Concurrent model initialization (potential crashes)
• GPU OOM errors
• Inconsistent cache state

Root Causes

1. Lock Generation Splitting

ModelManager.remove() called _dispose_model_lock() while still holding the per-model lock:

# OLD CODE (buggy)
with acquire_with_timeout(lock=model_lock):
    # ... removal logic ...
    self._dispose_model_lock(model_id=model_id)  # ❌ Deletes lock while holding it!

This allowed new add_model() calls to create a new lock generation while the old one was still held.

2. Decorator Bypass

ModelManagerDecorator.add_model() checked existence without synchronization:

# OLD CODE (buggy)  
if model_id in self:  # ❌ Unlocked check
    return  # Bypasses in-progress removal!

3. LRU Rollback Failures

WithFixedSizeCache removed queue entries before attempting removal:

# OLD CODE (buggy)
self._safe_remove_model_from_queue(model_id=model_id)  # ❌ Removed first!
return super().remove(model_id, delete_from_disk=delete_from_disk)  # If this fails, state is inconsistent

4. Duplicate Queue Entries

Concurrent adds could both pass the existence check before either acquired the queue lock.

Solution

Changes to inference/core/managers/base.py

1. Added lifecycle lock tracking: New _models_lifecycle_locks dict tracks ongoing mutations
2. Fixed lock disposal ordering: remove() now disposes lock AFTER releasing it
3. Added _get_lifecycle_lock() method: Allows decorators to detect in-progress mutations

# NEW CODE (fixed)
with acquire_with_timeout(lock=model_lock):
    # ... removal logic ...
    removal_succeeded = True
# Dispose lock AFTER releasing it ✅
if removal_succeeded:
    self._dispose_model_lock(model_id=model_id)

Changes to inference/core/managers/decorators/base.py

1. Check for ongoing mutations: Use lifecycle lock to detect in-progress operations
2. Preserve mutation ordering: Defer to wrapped manager when mutation is ongoing

# NEW CODE (fixed)
lifecycle_lock = self.model_manager._get_lifecycle_lock(model_id)
if lifecycle_lock is not None:
    # Mutation in progress - defer to wrapped manager ✅
    self.model_manager.add_model(...)
    return

Changes to inference/core/managers/decorators/fixed_size_cache.py

1. Remove queue entry AFTER successful removal: Prevents desynchronization on failures
2. Rollback eviction on failure: Restore queue entry if eviction fails
3. Double-check under lock: Prevent duplicate entries from concurrent adds

# NEW CODE (fixed)
result = super().remove(model_id, delete_from_disk=delete_from_disk)  # Remove first ✅
# Only update queue if removal succeeded
with acquire_with_timeout(lock=self._queue_lock):
    self._safe_remove_model_from_queue(model_id=model_id)

Testing

Added comprehensive test suite in test_concurrent_mutations.py:

• ✅ Lock generation stability tests
• ✅ Decorator mutation ordering tests
• ✅ LRU queue consistency tests
• ✅ Duplicate entry prevention tests
• ✅ Alias handling tests

All tests use threading primitives (Events, Threads) to deterministically reproduce race conditions without timing dependencies.

Backward Compatibility

✅ No breaking changes - All changes are internal implementation improvements. Public API remains unchanged.

Related Issues

Fixes #2819

Acknowledgments

Thanks to the issue author for the excellent bug report and detailed analysis!

---

🤖 Generated with Claude Code

Co-Authored-By: Claude noreply@anthropic.com

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

---

Kushagra Kanaujia seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[voropaevv]

I’m the author of #2821, which addresses the same issue.

At 738cf26a, the constructor-error path in add_model() still calls _dispose_model_lock() before releasing the per-model lock. A targeted component reproduction with real locks and threads reaches this sequence:

1. H holds L1 while constructing the model.
2. W obtains a reference to L1 and waits.
3. H's constructor fails and removes the registry entry.
4. N creates L2 and starts loading.
5. H releases L1; W acquires it and also starts loading.

Two constructors for the same resolved identifier are then active simultaneously. External dependencies were stubbed; this was not a full-suite or production-server test.

Moving disposal after remove() releases its lock also does not account for callers that already hold references to that generation. The required invariant is that an old generation cannot remain usable for mutations while new callers use a replacement. Reservation counting is one way to enforce it, but the implementation does not need to match #2821.

Two additional component cases expose bookkeeping/composition problems:

• In decorators/base.py, _get_lifecycle_lock() can be inherited by a wrapped ModelManagerDecorator, whose initializer does not create _state_lock. WithFixedSizeCache(ModelManagerDecorator(ModelManager(...))) then raises AttributeError.
• In fixed_size_cache.py, new is appended before eviction. If old.clear_cache() raises, restoring old and re-raising leaves [old, new] in the queue while only old is registered; the later add-error cleanup is not reached.

Could we add the load-failure and remove/reload schedules, nested-decorator case, and failed-eviction postconditions as regressions before considering the split-generation and bookkeeping issues resolved?

[PawelPeczek-Roboflow]

TMP impediment - CLA