Fix stale serializer cache IDs after deletion

[longligooo]

Summary

• Remove deleted object identifiers from the forward cache's eviction queue.
• Prevent a later cache insertion from attempting to evict an identifier that no longer exists.
• Add regression coverage for deleting a cached object and filling the freed cache slot.

Related Issues / Discussions

Closes #9370

QA Instructions

Run the focused regression test:

pytest -q tests/test_object_serializer_disk.py::test_obj_serializer_fwd_cache_removes_deleted_ids_from_eviction_queue

Run the configured lint check on the changed files:

ruff check invokeai/app/services/object_serializer/object_serializer_forward_cache.py tests/test_object_serializer_disk.py

Merge Plan

No special merge steps are required.

Checklist

• The PR has a short but descriptive title, suitable for a changelog
• Tests added / updated (if applicable)
• Changes to a redux slice have a corresponding migration (not applicable)
• Documentation added / updated (if applicable) (not applicable)
• Updated What's New copy (if doing a release after this PR) (not applicable)

[Pfannkuchensack]

Findings

The reviewed change is a single commit touching
invokeai/app/services/object_serializer/object_serializer_forward_cache.py plus one test.
The core fix is correct: the pre-patch code provably raises KeyError on the next eviction
after a delete, and the patch removes that.

Low - invokeai/app/services/object_serializer/object_serializer_forward_cache.py:51-56

The new consistency cleanup is skipped whenever the underlying delete raises, leaving a
"deleted" object still readable from the forward cache.

Chain:

1. delete() calls self._underlying_storage.delete(name) on line 52 before touching
   cache state.
2. invokeai/app/services/object_serializer/object_serializer_disk.py:66-68 implements
   delete as file_path.unlink() with no missing_ok=True, so it raises FileNotFoundError
   when the backing file is already gone.
3. The base contract at invokeai/app/services/object_serializer/object_serializer_base.py:29-35
   documents delete as "Deletes the object, if it exists", so a missing file is a
   contract-permitted input, not a programming error.
4. Lines 53-55 (del self._cache[name] and the new _remove_cache_id(name)) and line 56
   (self._on_deleted(name)) are all skipped.

Consequence: the object remains in _cache, so load(name) still returns it after the
caller was told to delete it, and MemoryInvocationCache._delete_by_match, registered at
invokeai/app/services/invocation_cache/invocation_cache_memory.py:40-41, never purges the
invocation-cache entry referencing that name.

Trigger: any out-of-band removal of the tempdir contents while entries are still cached -
e.g. ObjectSerializerDisk.stop() calling _tempdir_cleanup(), the dangling-tempdir sweep in
the disk serializer constructor, or a delete retried after a partial failure.

Evidence: reproduced directly. Removing the backing file and then calling delete()
produced FileNotFoundError with n1 still in _cache: True. Note the cache and queue stay
mutually consistent (cache 2 / queue 2), so this does not resurrect the KeyError; the
defect is stale-data visibility and a missed on_deleted notification, not a crash.

This ordering is pre-existing, but the commit's stated purpose is keeping _cache and
_cache_ids consistent across delete, and it leaves this path inconsistent. Moving the
cache/queue/callback cleanup ahead of the storage call, or wrapping line 52 in try/finally,
closes it.

To expose this issue, add a test that saves an object through the forward cache, deletes the
underlying file directly via fwd_cache._underlying_storage._get_path(name).unlink(), then
asserts that fwd_cache.delete(name) leaves the name absent from _cache and fires the
on_deleted callback.

Low - tests/test_object_serializer_disk.py:191-207

The new test covers only the positive path and cannot distinguish a correct queue rebuild from
an order-scrambling one.

• The drain-and-re-put loop at
  invokeai/app/services/object_serializer/object_serializer_forward_cache.py:58-69 is
  precisely where a FIFO-ordering regression would live, yet the test uses only two entries
  with max_cache_size=2, where every re-put ordering yields the same observable result.
  Order preservation was verified manually with a 3-entry probe (max_cache_size=3, save
  a/b/d, delete b, save e/f, evicts a first as expected), but nothing in the suite pins it.
• The negative path is also unpinned: delete() of a name that is not in _cache (already
  evicted, or never saved) currently short-circuits at the line 53 guard - verified a no-op
  today. If a future refactor drops that guard or calls _remove_cache_id unconditionally,
  every delete would drain and rebuild the whole queue with no test failing.

To expose this issue, add a test that uses max_cache_size=3, saves three objects, deletes
the middle one, saves two more, and asserts the oldest surviving entry is the one evicted;
and add a test that deletes an already-evicted name and asserts _cache_ids.qsize() and
_cache are unchanged and no exception is raised.

Minor: the test hand-rolls a serializer instead of reusing the existing fwd_cache fixture at
tests/test_object_serializer_disk.py:27-31, which is already max_cache_size=2. Duplicating
it means fixture drift will not be reflected here.

This is a good fix.

For what it's worth: the underlying cause is that the eviction order is tracked in a
second structure that has to be kept in sync with _cache by hand. I'd like to follow
up separately with a rework that drops the queue in favour of an OrderedDict — that
removes the bookkeeping entirely and lets the class actually be the LRU cache its
docstring claims (right now load() doesn't refresh position, so it's FIFO).

That's a larger change than this bug deserves though, so I don't think it should block
this PR.

[longligooo]

Thanks for the detailed review. Addressed in 6f36882: ObjectSerializerDisk.delete() is now idempotent for missing files per the base contract; forward-cache cleanup and on_deleted callbacks run in a finally block so cache state remains consistent when underlying deletion raises; regression coverage now includes a missing backing file, FIFO order after deleting a middle entry, and deletion of an already-evicted entry. The original regression test also reuses the existing fwd_cache fixture. Local verification: 20 object serializer tests passed, Ruff lint and format checks passed, and git diff --check passed.

[JPPhoto]

Note: Accepting this PR should result in closing PR #9371, and vice versa. This PR seems more robust to me.

[JPPhoto]

@longligooo Also adding my notes on this PR here:

• invokeai/app/services/object_serializer/object_serializer_forward_cache.py:60: _remove_cache_id() drains and rebuilds _cache_ids without serializing the full cache/queue transition. Two deletes can interleave: first drains IDs A/B, second deletes B against empty queue, then first requeues stale B. Two later saves reproduce issue 9370, raising KeyError at line 81 after disk write. Merged main added no _cache_lock; head 61f4bb5c03 remains unsafe. Test: pause first delete after queue drain, complete second delete, resume first, then save twice; reproduced exact KeyError.

Attaching a standalone test that illustrates this that could be integrated with the regular tests: test_pr9390_concurrency.py

[longligooo]

Thanks for the deterministic reproducer. Addressed in 2e9a9a1, with current main merged in ebbdf67. The forward cache now uses a single RLock to serialize complete transitions across both _cache and _cache_ids, including the queue drain and rebuild performed by concurrent deletes. Reentrancy is required because the transition methods call cache helpers that share the same lock, and deletion callbacks remain outside the critical section. I added a deterministic regression test that pauses the first delete after draining the queue, starts a second delete, verifies that it cannot complete until the first transition is released, then saves two new entries and checks that no stale eviction ID or KeyError remains. Local verification after resolving the main-branch conflict: 31 focused serializer/cache tests passed; full-repository Ruff 0.11.2 lint passed; full-repository Ruff format check passed (1169 files); and git diff origin/main --check passed.

[Pfannkuchensack]

Findings

1. Medium-High -- load() now serializes every cache-miss read across all session-processor workers

invokeai/app/services/object_serializer/object_serializer_forward_cache.py:41-49

def load(self, name: str) -> T:
    with self._cache_lock:
        cache_item = self._get_cache(name)
        if cache_item is not None:
            return cache_item
        obj = self._underlying_storage.load(name)   # <-- disk I/O + torch deserialization under the global lock
        self._set_cache(name, obj)
        return obj

Before this PR, load() took the lock only inside _get_cache / _set_cache; the underlying read ran lock-free. The PR widens the critical section to cover the entire read, and this widening is not mentioned anywhere in the PR description or the QA instructions.

Evidence chain:

1. invokeai/app/services/object_serializer/object_serializer_forward_cache.py:47 calls self._underlying_storage.load(name) while holding self._cache_lock.
2. invokeai/app/services/object_serializer/object_serializer_disk.py:53-58 implements that as torch.load(file_path) -- a full file read plus tensor deserialization.
3. invokeai/app/api/dependencies.py:150 and invokeai/app/api/dependencies.py:157 construct exactly one shared ObjectSerializerForwardCache for tensors and one for conditioning, for the whole process.
4. invokeai/app/services/session_processor/session_processor_default.py:505-529 starts one session_processor_<index> thread per device, all sharing those two serializer instances.
5. Therefore, on a multi-GPU box, a cache-miss context.tensors.load(...) on GPU0 blocks the identical call on GPU1 for the full duration of the disk read.

Measured, not inferred. Four concurrent cache-miss loads with a 0.25s underlying read:

4 concurrent cache-miss loads of 0.25s each took 1.03s

Fully serialized (4 x 0.25s), not overlapped. The lock's own comment cites "concurrent session-processor workers (multi-GPU)" as the motivation, so the regression lands precisely on the configuration the comment names. Callers affected include invokeai/app/invocations/denoise_latents.py:798-801, invokeai/app/invocations/flux_denoise.py:531, and invokeai/app/invocations/flux2_denoise.py:548.

The only behavior this widening buys is de-duplicating two concurrent misses on the same name -- a benign redundancy in the old code. That is not worth serializing all reads, and it is orthogonal to issue #9370, which is a delete() bug.

To expose this issue, add a test that patches the underlying storage load to sleep, issues N concurrent load() calls for N distinct uncached names from a thread pool, and asserts wall-clock time is closer to one read than to N reads.

2. Medium -- delete() fires on_deleted and drops the cache entry even when the underlying delete raises

invokeai/app/services/object_serializer/object_serializer_forward_cache.py:56-66

def delete(self, name: str) -> None:
    try:
        with self._cache_lock:
            try:
                self._underlying_storage.delete(name)
            finally:                                  # <-- runs on failure
                if name in self._cache:
                    del self._cache[name]
                    self._remove_cache_id(name)
    finally:
        self._on_deleted(name)                        # <-- runs on failure

Both cleanup blocks are finally, not else. Pre-PR, self._underlying_storage.delete(name) ran first and unguarded, so a raised exception skipped both the cache removal and the notification.

Confirmed by probe. With the underlying delete raising PermissionError (the realistic Windows case: file still open/mapped by another handle):

on_deleted callbacks fired = ['MockDataclass_a79ffe8d-...']
still cached = False
file still on disk = True

Consequence chain:

1. invokeai/app/services/object_serializer/object_serializer_forward_cache.py:66 invokes _on_deleted(name) despite the failure.
2. invokeai/app/services/object_serializer/object_serializer_base.py:42-44 fans out to registered callbacks.
3. invokeai/app/services/invocation_cache/invocation_cache_memory.py:41-42 registers MemoryInvocationCache._delete_by_match for both tensors and conditioning.
4. invokeai/app/services/invocation_cache/invocation_cache_memory.py:117-131 purges every cached invocation output whose JSON contains that name, and logs Deleted N cached invocation outputs.

So listeners are told the object was deleted, and the invocation cache is purged, while the exception propagates to the caller saying the delete failed and the file is still on disk. The base contract at invokeai/app/services/object_serializer/object_serializer_base.py:38-39 says the callback is "for when an object is deleted".

The PR's own new test test_obj_serializer_fwd_cache_cleans_up_when_storage_object_is_missing asserts called_names == [obj_name], but that case never raises -- finding 3 below made it a silent no-op. No test in the PR covers a raising underlying delete.

To expose this issue, add a test that monkeypatches the underlying storage's delete to raise PermissionError, calls ObjectSerializerForwardCache.delete inside pytest.raises, and asserts no on_deleted callback fired.

3. Low-Medium -- missing_ok=True is unrelated to the fix and silently swallows a real error class

invokeai/app/services/object_serializer/object_serializer_disk.py:66-68

def delete(self, name: str) -> None:
    file_path = self._get_path(name)
    file_path.unlink(missing_ok=True)

The base docstring does say "Deletes the object, if it exists", so the direction is defensible -- but the change is not mentioned in the PR summary and is not required by #9370. _get_path at invokeai/app/services/object_serializer/object_serializer_disk.py:77-78 is self._output_dir / name, and _output_dir differs between the ephemeral tempdir and the base dir (invokeai/app/services/object_serializer/object_serializer_disk.py:39-48). Production wires ephemeral=True at invokeai/app/api/dependencies.py:150-156. A delete issued against a stale or wrong _output_dir previously surfaced as FileNotFoundError; it now returns success. Note also that load() on the same class deliberately raises ObjectNotFoundError for a missing file (invokeai/app/services/object_serializer/object_serializer_disk.py:57-58), so load and delete now disagree on how they treat a missing object.

The PR's test_obj_serializer_disk_delete_is_noop_when_object_is_missing pins the new behavior but does not distinguish "object legitimately already gone" from "we computed the wrong path".

4. Low -- the fixed code path is unreachable from the running application, so the PR's net production effect is finding 1

invokeai/app/services/shared/invocation_context.py:437-487 exposes only save and load on TensorsInterface and ConditioningInterface. A repo-wide search for callers of services.tensors.delete / services.conditioning.delete returns nothing outside invokeai/app/services/object_serializer/object_serializer_forward_cache.py:60 itself and the test file. The KeyError in #9370 is reachable only from tests or out-of-tree code.

This is not an argument against fixing it -- the fix is correct -- but it does mean the PR trades a hot-path throughput regression (finding 1) and a notification-semantics change (finding 2) for a latent-only defect. The load() and delete() locking changes should be reverted to the pre-PR shape; only _remove_cache_id and its invocation are needed to close #9370.