fix(cache): a failed disk-cache write no longer kills the writer thread - #669
Open
olesxg wants to merge 1 commit into
Open
fix(cache): a failed disk-cache write no longer kills the writer thread#669olesxg wants to merge 1 commit into
olesxg wants to merge 1 commit into
Conversation
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.
Contributor
Greptile SummaryThe 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.
Confidence Score: 5/5The 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.
|
| 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]
Reviews (1): Last reviewed commit: "fix(cache): a failed disk-cache write no..." | Re-trigger Greptile
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.
What
Cache._consume_queueruns the only consumer ofself._add_q. Theself._cache.add(...)call inside it is unguarded, so any exception raised by diskcache propagates out of
_consume_queueand terminates that thread. Because the thread was started withself._threadpool.submit(self._consume_queue)and the resultingFutureis neverretrieved, the exception is swallowed silently — nothing is logged.
From that moment on
_add_qhas no consumer at all. It is unbounded, andaget_completekeeps putting(sentence, embedding)pairs into it for every cachemiss, 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_queueexecuteson a worker of that same pool, so it calls
Thread.join()on the current thread andraises
RuntimeError: cannot join current thread— also swallowed by the unretrievedFuture. Since
ThreadPoolExecutor._threadsis a set with arbitrary iteration order,the other workers may or may not be joined first, so the
wait=Truecontract is nothonoured either way. Changed to
wait=Falsewith 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_writeto the existingtests/unit_test/inference/test_caching_layer.py. It monkeypatches_cache.addtoraise on the first item, then asserts the second item is still written — which fails
on
mainand passes here. It polls rather than sleeping a fixed interval, sincewindows-latestis in the CI matrix.Deliberately not in scope
No
maxsizeon_add_q, no new env var, noCache.close()wired intoBatchHandler.shutdown(). Under normal operation the producer only enqueues after acompleted forward pass, and one SQLite
add()is orders of magnitude cheaper than aninference, 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:The same new test, run against unmodified
main(fix reverted, test kept), fails as it should:ruff checkandcodespellpass;ruff format --checkdoes not flag either changed file;mypy ./infinity_embproduces output identical tomain(4 pre-existingimport-not-foundnotes for
uvloop/optimum, from installing without those extras).