Repository navigation
fix: ensure ZipStore is open before acquiring lock - #3593
Closed
oelhammouchi wants to merge 11 commits into
Closed
oelhammouchi wants to merge 11 commits into
oelhammouchi wants to merge 11 commits into
Conversation
github-actions
Bot
added
the
needs release notes
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3593 +/- ##
==========================================
+ Coverage 60.89% 60.91% +0.02%
==========================================
Files 86 86
Lines 10182 10188 +6
==========================================
+ Hits 6200 6206 +6
Misses 3982 3982
🚀 New features to boost your workflow:
|
Contributor
|
how did the bug you are fixing here get past the store tests? Maybe we need to make those tests stronger? |
Contributor
Author
|
You're right, I didn't want to appear presumptuous but the code could do with some refactoring as well, having to repeat all those |
Contributor
|
if you can find a clean way to reduce the repetition (e.g., a decorator) we'd appreciate it! |
Contributor
Author
|
@d-v-b just for info, I'm currently on holiday but I'm planning to implement the proposed improvements shortly |
Contributor
Author
|
@d-v-b I've finally had time to revisit this! I've added tests and cleanup up the open check, the proposed decorator approach turned out not very clean imo because you'd need to test for the various types of functions (async, iterator, etc.) |
Contributor
|
looks good @oelhammouchi, would you mind writing a release note for these changes? |
github-actions
Bot
removed
the
needs release notes
d-v-b
approved these changes
Jan 13, 2026
Contributor
|
🤖 AI text below 🤖 The changelog fragment git mv changes/3588.bugfix.md changes/3593.bugfix.mdThis PR is planned for the 3.4.1 patch release. |
d-v-b
added a commit
to d-v-b/zarr-python
that referenced
this pull request
Oct 3, 2026
Assisted-by: ClaudeCode:claude-opus-5-5
d-v-b
added a commit
that referenced
this pull request
Oct 26, 2026
…d reopen it without truncating (#4450) * fix: ensure `ZipStore` is open before acquiring lock * refactor: Cleanup open check in ZipStore * test: Add test to check that lock is present when required * refactor: Add return type to `_sync_ensure_open` * docs: Add release note * fix(storage): open ZipStore archive through one accessor and reopen in append mode Create the lock in `__init__` and route every archive access through `_zipfile()`, which opens the archive on first use. This covers `get`, `get_partial_values`, `set_if_not_exists` and `clear`, which still raised `AttributeError` on a never-opened store. After the first open, switch mode "w" or "x" to "a" so that `move()` and unpickling reopen the archive without truncating it or refusing to open it. Builds on the fix proposed by Othman El Hammouchi in #3593. Refs #3588 Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Othman El Hammouchi <othman.el.hammouchi@protonmail.com> * docs: rename changelog fragment to PR number Assisted-by: ClaudeCode:claude-opus-5-5 * test(storage): make ZipStore reopen tests exercise the reopen Replace the close-twice case, which never reopened the archive and passed on main, with a read after close(), which erased a "w" archive on main. Assert return values and final archive contents in the first-use test instead of only checking that nothing raises. Cover reopening a file-object-backed store after close(). List the reopen after close(), the file-object case, and the first-use thread race in the changelog fragment. Assisted-by: ClaudeCode:claude-opus-5-5 * fix(storage): hold the ZipStore lock for all of close() close() marked the store closed before taking the lock, so a thread that used the store in that gap reopened the archive in append mode before its central directory was written. Its handle then wrote a directory that listed only its own entries, and the earlier entries were lost. Assisted-by: ClaudeCode:claude-opus-5-5 * fix(storage): hold the ZipStore lock across move() and every open move() closed the store, moved the file, and reopened it without the lock, so a thread that used the store in between reopened the old path and the move then failed with "store is already open". Hold the lock for the whole move. _open() also opens under the lock, and unpickling opens through _zipfile(). clear() keeps opening a never-opened store before deleting the file, so a store in mode "x" refuses an existing file instead of deleting it. A test pins that behaviour. Assisted-by: ClaudeCode:claude-opus-5-5 * docs: note move() in the ZipStore changelog fragment Assisted-by: ClaudeCode:claude-opus-5-5 * fix(storage): accept mode "x" in ZipStoreAccessModeLiteral ZipStore documents and accepts mode "x" at runtime, but the type of its mode parameter left it out, so type-checked callers needed an ignore. Assisted-by: ClaudeCode:claude-opus-5-5 * fix(storage): refuse to reopen a ZipStore on a write-only file object Reopening after close() uses append mode, which reads the archive back. A write-only file object cannot be read, so zipfile started a new archive after the old one and the earlier entries were silently lost. Raise io.UnsupportedOperation instead. The first open is unchanged. The thread-race tests now join the writer with a timeout, so a deadlock fails the test instead of hanging the run. Assisted-by: ClaudeCode:claude-opus-5-5 * fix(storage): make ZipStore close() exception-safe and check first use under the lock close() now marks the store closed even when closing the archive raises, so the next use reopens the archive instead of failing on a closed handle. ZipStore overrides _ensure_open, whose base version checked _is_open outside the lock, so it could race a concurrent first use and fail with "store is already open". Reuse after close() now also requires a seekable file object, since append mode seeks. The docstring now says that "w" and "x" apply to the first open only. A new test holds the lock while another thread makes its first use, which also catches a _zipfile() that opens without the lock. Assisted-by: ClaudeCode:claude-opus-5-5 * fix(storage): refuse to reopen a ZipStore whose close() failed If closing the archive raises, its central directory may be missing, and reopening it in append mode makes zipfile start a new archive and silently drop the earlier entries. The store now records the failure and raises RuntimeError on its next use. list() now iterates a copy of the names without holding the lock, so other threads are not blocked while a caller iterates. move() opens a never-opened store before closing it, so mode "x" refuses an existing file instead of moving it. The mode docstring no longer says every reopen appends. The failed-close test replaces one that patched ZipFile.close with a closure over the same ZipFile. That cycle raised from ZipFile.__del__ during a later test's garbage collection, which the warnings-as-errors config reported as an error in an unrelated test. Assisted-by: ClaudeCode:claude-opus-5-5 * fix(storage): leave a ZipStore closed when clear() fails partway clear() closed the archive, removed the file, and opened a new one. If a later step raised, the store stayed marked open on a closed handle and every use failed. It now closes the store through close() and reopens with "w", which truncates, so a failure leaves the store closed and the next use tries the truncating open again. The file is no longer removed first. Assisted-by: ClaudeCode:claude-opus-5-5 * fix(storage): let clear() recover a ZipStore whose close() failed clear() replaces the archive, so it cannot drop the entries that the failed-close guard protects; it now resets the guard instead of raising. clear() removes the file again before creating the new archive, as main did, so links, permissions, and other open readers behave as before. __getstate__ copies the state under the lock, so a pickle cannot catch _sync_open between the "w" open and the switch to "a". A test covers unpickling state from before _was_opened and _close_failed existed. Assisted-by: ClaudeCode:claude-opus-5-5 * test(storage): close ZipStores over file objects when a reuse test fails If the reuse-after-close tests fail, the store is still open when its file object goes away, and ZipFile.__del__ then raises during a later test's garbage collection. The warnings-as-errors config reported that as an error in an unrelated test. Close the store in a finally block. Assisted-by: ClaudeCode:claude-opus-5-5 * fix(storage): open unpickled ZipStores lazily and refuse to reopen an unfinished archive Unpickling opened the archive right away. A copy of a store that was still writing, like those dask makes while building a graph, found no central directory and opened in append mode, which zipfile marks as modified; when the copy was garbage-collected after the original closed, it wrote an empty central directory at the end of the file and the archive read as empty. Unpickled stores now open on first use, and a store that has already written refuses to reopen a file with no central directory, raising zipfile.BadZipFile. clear() lifts the failed-close guard only after the old file is removed, and the docstring and error message no longer suggest clear() for stores it cannot clear. A test covers clear() on a mode "r" store with read_only=False. Assisted-by: ClaudeCode:claude-opus-5-5 * docs: describe the unpickling fix from main's side in the ZipStore fragment Assisted-by: ClaudeCode:claude-opus-5-5 * fix(storage): let ZipStore.clear() replace a damaged or missing archive clear() opened the store before removing the file, so a store whose file had become a non-zip hit the reopen guard, and one whose file was gone after a failed close() raised FileNotFoundError forever. It now opens first only for a store that was never opened, which is what keeps mode "x" from deleting an unclaimed file, and removes the file with missing_ok. The guard's message no longer blames another writer alone, and the changelog says which concurrent writers it detects. Tests cover clear() on a damaged or missing file, reopening after the file was deleted, and a first open with "a" on an empty file. Assisted-by: ClaudeCode:claude-opus-5-5 * fix(storage): narrow ZipStore.clear()'s first open to mode "x" and report OS errors on reopen clear() opened a never-opened store in every mode, but only "x" needs it, and for mode "r" with read_only=False over a missing or damaged file that open failed and left the store unusable. The reopen guard used zipfile.is_zipfile, which swallows OSError, so a directory or an unreadable file at the path was reported as an unfinished archive; it now opens the file itself and lets such errors surface. The close() race test now starts the concurrent write while the central directory is being written, so removing the lock from close() fails it. New cases cover set_if_not_exists on an existing key, an old pickle in mode "a" over an empty file, KeyboardInterrupt during close(), clear() on a mode "r" store whose file is missing, and a directory at the archive path. Assisted-by: ClaudeCode:claude-opus-5-5 * refactor(storage): drop a redundant condition from ZipStore.clear() Mode "x" is switched to "a" by the first open, so checking _was_opened alongside it changed nothing. Assisted-by: ClaudeCode:claude-opus-5-5 * docs: credit #3593 in the ZipStore changelog fragment Assisted-by: ClaudeCode:claude-opus-5-5 * docs(zipstore): shorten the changelog fragment and give the new tests docstrings Assisted-by: ClaudeCode:claude-fable-5.1 --------- Co-authored-by: Othman El Hammouchi <othman.elhammouchi@protonmail.com> Co-authored-by: Othman El Hammouchi <78906075+oelhammouchi@users.noreply.github.com> Co-authored-by: Othman El Hammouchi <othman.el.hammouchi@protonmail.com>
Contributor
|
@oelhammouchi I folded your work into #4450 and credited you + this PR, so I'm closing this as superseded, thanks for this work! |
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.
Fix a bug where
ZipStorecomplains that it has no attribute_lockwhen some methods are called. Any method using the lock should make sure the store is opened because that's when the_lockattribute gets set.