lrucache: allow replacing an entry, make it thread-safe - #10041
Merged
ThomasWaldmann merged 2 commits intoAug 5, 2026
Conversation
LRUCache refused that with an assertion, so callers had to write
if key in cache:
cache.replace(key, value)
else:
cache[key] = value
and a caller that just assigned crashed - which is a nasty failure mode for a
cache: it is a correctness-neutral operation that suddenly is not. It bites
especially when several threads use one cache (see the FUSE data cache), where
two threads can miss on the same key and both insert the value they fetched.
Assignment now replaces the value, disposing the old one (it left the cache,
just like a deleted one would) unless it is the identical object, and counts as
a use, so the entry moves to the most-recently-used end.
replace() stays for the one case that really needs it: updating an entry while
keeping its old value alive - the legacy repository's fd cache re-timestamps its
entries that way and must not have the open file closed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Several threads share one LRUCache in borg: mfusepy serves the FUSE requests of one mount from a thread pool, and "borg webdav" from one thread per connection. Until now every caller had to serialize the access itself - a rule that is easy to miss, and missing it does not fail loudly at the cache, it fails somewhere far away (mfusepy turns any exception from a FUSE operation into EINVAL, so a mount just reports "Invalid argument" for a healthy file). Getting, setting, deleting, popping, replacing and clearing now each hold a lock, so they are atomic. A *sequence* of them still is not - two threads can miss on the same key and both store a value, which for a cache is wasteful, not wrong. pop() is implemented here now instead of inheriting it: MutableMapping builds it from __getitem__ + __delitem__, which is not atomic (two threads popping the same key raise KeyError, see the new test), and it would dispose the value it hands to the caller - for e.g. a cache of open files it returned a closed one. Cost, measured with timeit: about 75 ns per operation (get 70 -> 148 ns, set 148 -> 222 ns). That is nothing next to what these caches guard - a chunk decrypt is in the milliseconds, an item unpack in the microseconds - and a borg mount read benchmark (1 GiB sequential, and stat+read of 200 small files, mfusepy) shows no difference at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10041 +/- ##
==========================================
+ Coverage 86.18% 86.20% +0.02%
==========================================
Files 96 96
Lines 17434 17455 +21
Branches 2665 2667 +2
==========================================
+ Hits 15025 15047 +22
+ Misses 1668 1667 -1
Partials 741 741 ☔ View full report in Codecov by Harness. |
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.
Follow-up to #10023, where a
LRUCacheblew up in the FUSE mount. Two commits, both onhelpers/lrucache.pyand its tests only - no call site changes, so this does not conflict with #10023.1. Assigning to a key that is present replaces its value
__setitem__assertedkey not in self._cache, so callers had to writeand a caller that just assigned crashed. That is a nasty failure mode for a cache: an operation that is correctness-neutral for the caller suddenly is not. It bites especially when several threads share one cache - two of them can miss on the same key and both store the value they fetched, which is how the mount in #10023 crashed (
hlfuse.py:self.data_cache[id] = data).Assignment now replaces the value, disposes the old one (it left the cache, exactly like a deleted one would) unless it is the identical object, and counts as a use.
replace()stays for the one case that needs it: updating an entry while keeping its old value alive - the legacy repository's fd cache re-timestamps its entries that way and must not have the open file closed.Why the first commit can not break a working caller
The old behaviour for that case was to raise, and that was the only reachable one: borg refuses to
run with assertions disabled (
archiver/__init__.pydoesassert Falseat import and exits with rc2), so there is no
-Omode in which the assignment quietly did something else. Only code that wascrashing changes behaviour.
The invariant the assertion protected - "dispose() is always called when necessary", the rationale in
9ba7daa that introduced it - is now enforced rather than forbidden: the replaced value is disposed.
Auditing every assignment into an LRUCache in the tree: the legacy repository's fd cache (the only
disposeuser) assigns only insideopen_fd(), i.e. on the KeyError path, and re-timestamps viareplace(); thefuse.pycaches andparsed_cache/_pack_cacheare all guarded by a precedinglookup and reached single-threaded or under the repository lock. The sites that can reach the new
path are exactly the concurrent ones:
hlfuse.py's andwebdav.py'sdata_cache, and the globalzero_chunk_idsinarchive.py- i.e. the bugs this fixes. Nothing catches the AssertionError, andno test depended on it.
2. Thread safety
Several threads share one
LRUCachein borg: mfusepy serves the FUSE requests of one mount from a thread pool, andborg webdavfrom one thread per connection. Until now every caller had to serialize access itself - a rule that is easy to miss, and missing it does not fail at the cache but far away from it (mfusepy turns any exception from a FUSE operation into EINVAL, so the mount reports "Invalid argument" for a perfectly healthy file).Get, set, delete, pop, replace and clear now each hold a lock. A sequence of them still is not atomic - two threads can miss on the same key and both compute a value, which for a cache is wasteful, not wrong - and that is documented, together with the fact that iteration yields a live view and that
disposeruns under the lock.pop()is implemented here now rather than inherited:MutableMappingbuilds it from__getitem__+__delitem__, which is not atomic and disposed the value it hands to the caller (a cache of open files returned a closed file).Testing
test_threaded_accessis a real regression test: it fails without these changes and passes with them. It needssys.setswitchinterval(1e-6)- at the default 5 ms these loops hardly ever switch inside one cache operation, which is why this class of bug stays hidden until it hits a mount.popmypy --ignore-missing-importsreports exactly the same pre-existing errors as masterCost
About 75 ns per operation (timeit: get 70 -> 148 ns, set 148 -> 222 ns). Nothing next to what these caches guard - a chunk decrypt is in the milliseconds, an item unpack in the microseconds - and a borg mount read benchmark (1 GiB sequential, plus stat+read of 200 small files, mfusepy) shows no difference. It also makes the caches robust for free-threaded Python, where the GIL no longer papers over the check-then-act sequences these caches are full of.
🤖 Generated with Claude Code