Skip to content

fix(cache): a failed disk-cache write no longer kills the writer thread - #669

Open
olesxg wants to merge 1 commit into
michaelfeil:mainfrom
olesxg:fix/cache-writer-survives-failures
Open

fix(cache): a failed disk-cache write no longer kills the writer thread#669
olesxg wants to merge 1 commit into
michaelfeil:mainfrom
olesxg:fix/cache-writer-survives-failures

Conversation

@olesxg

@olesxg olesxg commented Aug 24, 2026

Copy link
Copy Markdown

What

Cache._consume_queue runs the only consumer of self._add_q. The self._cache.add(...)
call inside it is unguarded, so any exception raised by diskcache propagates out of
_consume_queue and terminates that thread. Because the thread was started with
self._threadpool.submit(self._consume_queue) and the resulting Future is never
retrieved, the exception is swallowed silently — nothing is logged.

From that moment on _add_q has no consumer at all. It is unbounded, and
aget_complete keeps putting (sentence, embedding) pairs into it for every cache
miss, so it grows for the lifetime of the process while the cache stores nothing.
There is no external symptom until memory runs out.

A best-effort vector cache should never be able to take down its own writer.

Also in this PR

self._threadpool.shutdown(wait=True) on the last line of _consume_queue executes
on a worker of that same pool, so it calls Thread.join() on the current thread and
raises RuntimeError: cannot join current thread — also swallowed by the unretrieved
Future. Since ThreadPoolExecutor._threads is a set with arbitrary iteration order,
the other workers may or may not be joined first, so the wait=True contract is not
honoured either way. Changed to wait=False with a comment explaining why.

Committed as two separate commits so the shutdown change can be dropped independently
if you would rather handle it another way.

Test

Added test_consumer_survives_failed_write to the existing
tests/unit_test/inference/test_caching_layer.py. It monkeypatches _cache.add to
raise on the first item, then asserts the second item is still written — which fails
on main and passes here. It polls rather than sleeping a fixed interval, since
windows-latest is in the CI matrix.

Deliberately not in scope

No maxsize on _add_q, no new env var, no Cache.close() wired into
BatchHandler.shutdown(). Under normal operation the producer only enqueues after a
completed forward pass, and one SQLite add() is orders of magnitude cheaper than an
inference, so the queue cannot outrun the writer while the writer is alive. Bounding it
would be treating the symptom. Happy to add any of those separately if you want them.

Verified locally

Windows, Python 3.11.9, poetry install -E cache -E torch -E server --with test,lint,codespell:

$ poetry run pytest tests/unit_test/inference/test_caching_layer.py -v
tests/unit_test/inference/test_caching_layer.py::test_cache PASSED                        [ 50%]
tests/unit_test/inference/test_caching_layer.py::test_consumer_survives_failed_write PASSED [100%]
2 passed in 1.93s

The same new test, run against unmodified main (fix reverted, test kept), fails as it should:

FAILED tests/unit_test/inference/test_caching_layer.py::test_consumer_survives_failed_write
1 failed in 8.04s

ruff check and codespell pass; ruff format --check does not flag either changed file;
mypy ./infinity_emb produces output identical to main (4 pre-existing import-not-found
notes for uvloop / optimum, from installing without those extras).

An exception from diskcache escaped _consume_queue, terminated the only
consumer thread, and was swallowed by the never-retrieved Future from
_threadpool.submit(). From that point _add_q had no consumer and grew one
entry per embedded item for the lifetime of the process, while the cache
stored nothing.

Also fixes _threadpool.shutdown(wait=True) being called from a worker of
that same pool, which raises RuntimeError: cannot join current thread.
@greptile-apps

greptile-apps Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR makes the best-effort disk-cache writer resilient to individual write failures and avoids waiting for its executor from inside the executor’s own worker.

  • Catches and logs exceptions from diskcache.Cache.add while always balancing Queue.task_done().
  • Uses non-waiting executor shutdown to avoid attempting to join the current worker thread.
  • Adds a regression test confirming that a failed write does not prevent processing the next queued item.

Confidence Score: 5/5

The PR appears safe to merge, with no actionable changed-code defects identified.

The writer now contains individual disk-cache failures, continues consuming queued entries, and shuts down without attempting to join its current executor worker.

Important Files Changed

Filename Overview
libs/infinity_emb/infinity_emb/inference/caching_layer.py Guards each queued disk-cache write, guarantees queue accounting, and avoids self-joining the cache executor during shutdown.
libs/infinity_emb/tests/unit_test/inference/test_caching_layer.py Adds a focused asynchronous regression test proving the writer continues after one cache-add exception.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Queued cache item] --> B[Writer consumes item]
    B --> C{Disk-cache write succeeds?}
    C -- Yes --> D[Mark item done]
    C -- No --> E[Log warning]
    E --> D
    D --> F{Shutdown requested?}
    F -- No --> A
    F -- Yes --> G[Shutdown executor without self-join]
Loading

Reviews (1): Last reviewed commit: "fix(cache): a failed disk-cache write no..." | Re-trigger Greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant