Skip to content

Keep borrowed packs open after cache eviction - #2349

Draft
bojanz wants to merge 3 commits into
jelmer:mainfrom
bojanz:pack-cache-release-on-eviction
Draft

Keep borrowed packs open after cache eviction#2349
bojanz wants to merge 3 commits into
jelmer:mainfrom
bojanz:pack-cache-release-on-eviction

Conversation

@bojanz

@bojanz bojanz commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Several code paths close a Pack at the moment it leaves the object-store cache: _add_cached_pack closes the pack it replaces, _enforce_packed_git_limit closes the least-recently-used pack, _remove_pack closes on removal, and _update_pack_cache closes packs whose files disappeared.

Closing at eviction time is unsafe when the store is shared: packs and get_raw hand out references to cached Pack objects, so another consumer — a concurrent request in a threaded/greenlet server, or an iterator like iter_unpacked that is still mid-pack — may be reading from the pack when eviction closes it under its feet. The reader then fails on a closed file even though the pack file itself is still perfectly valid on disk.

This changes eviction to release ownership instead of closing: the pack is dropped from the cache, and the underlying files are closed by Pack.__del__ once the last borrower is done with it. Since that hand-off is intentional, __del__ no longer emits the "unclosed Pack" ResourceWarning for packs released this way — explicit leaks still warn as before.

The new test exercises the LRU-limit path: a pack evicted by packed_git_limit while an iter_unpacked iterator is mid-stream stays readable until the iterator completes.

@bojanz
bojanz requested a review from jelmer as a code owner August 10, 2026 15:51
@bojanz

bojanz commented Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

The Windows failures were real: test_packed_git_limit_evicts_lru_packs started leaving an evicted pack's .idx open into teardown, so rmtree failed with WinError 32. Pushed a fix.

Unsure how useful test_no_reference_cycle truly is, considering that we already had a test that failed.

@bojanz
bojanz force-pushed the pack-cache-release-on-eviction branch from f73b999 to f9dfba2 Compare August 10, 2026 16:22
Turn resource-backed Pack iterators into generators that retain the owning pack.
Add regression coverage for cache-released packs across all iterator APIs.
@bojanz

bojanz commented Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

The last commit could be a followup PR as well if you prefer it that way.

One behavioral note: work and errors that previously happened at the call, most notably the full sort in sorted_entries() and the checksum/length validation on first self.data access in iterobjects(), now happen at the first next().

@bojanz
bojanz marked this pull request as draft August 27, 2026 21:35
@bojanz

bojanz commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

Drafted cause I want to take another look at this once we land #2334

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