fix(storage): open ZipStore archive on first use from every method and reopen it without truncating

[d-v-b]

This is the result of a very tedious dive into problems at the boundary between our store API and the realities of zip archives. There are many ways in which an object storage API can fail to uphold the expectations of an archive that is unreadable until a finalization process has occurred, and this PR discovered and addressed many of them.

tl;dr is that these changes make our zip storage safer, by adding locks and checks in various places to prevent zip archive corruption, but the work done here represents a warning sign that zip storage is not in fact like S3, and we probably need to represent that formally with API design. I had claude write up a post-mortem / design recommendation here, in case anyone wants to read it. The good news is that many of the proposed fixes were already suggested in the zarr-python-planning storage API proposal

claude wrote the fix and the long-winded robot PR description that follows.

🤖 AI text below 🤖

ZipStore now opens its archive on first use from every method, so calling get, get_partial_values, set_if_not_exists, or clear on a store that was never opened no longer raises AttributeError. A store created with mode="w" or mode="x" now keeps its entries when it is used again after close(), moved with move(), or unpickled, instead of truncating the archive or refusing to open it. Previously, even a read on a closed mode="w" store erased the archive, and such a store backed by a file object dropped its earlier entries when written to again after close(). A store backed by a readable, seekable file object now keeps them, and one backed by any other file object raises io.UnsupportedOperation instead, because it cannot read the archive back. Threads that use a store for the first time at the same moment no longer open the archive twice and lose writes, and neither does a thread that uses a store while another thread closes or moves it. If closing the archive fails, for example because the disk is full, every later use of the store that would reopen the archive now raises RuntimeError, instead of reopening an archive that may lack its central directory and silently starting a new one; for a writable store backed by a path, clear() replaces the archive and makes the store usable again. A clear() that fails partway now leaves the store closed instead of open on a closed archive. An unpickled store now opens its archive on first use, so a copy that is never used, such as those dask makes while building a graph, no longer touches the file; before, such a copy reopened the archive and corrupted it, so writing a dask array to a ZipStore produced an unreadable file. A store that reopens its archive and finds it is not a zip, as happens while another copy is still in its first writing session, raises zipfile.BadZipFile instead of starting a new archive; other concurrent writers are not detected. On a store that was never opened, move() now applies the first-open mode at the original path, so mode "x" refuses an existing file instead of moving it. The type of the mode parameter now includes "x", which the store already accepted at runtime.

This replaces #3593 and keeps its five commits by Othman El Hammouchi. The first fix commit also credits them as co-author. Refs #3588. Refs #3516: its reproducer, which writes a dask array to a ZipStore, reads back correctly with this change and raises BadZipFile on main.

Root cause

Two design flaws caused these bugs:

1. _lock and _zf were created in _sync_open(), so each method had to check _is_open before touching them. That check was copied into each method, and it ran outside any lock. On main, get, get_partial_values and set_if_not_exists take _lock before any check runs, and clear touches _zf directly, so all four raise AttributeError on a store that was never opened. fix: ensure ZipStore is open before acquiring lock #3593 moved the checks around but dropped the one in list_dir. Two threads could both see a closed store and both open it; with "w", the second open truncated the first one's writes.
2. _zmode was used both for the first open and for every reopen, and unpickling reopened the archive right away. With "w", each reopen truncated the archive. On main, a "w" store has no keys after close() followed by any use, after move(), or after a pickle round trip.

Change

• The lock is created in __init__ and again in __setstate__.
• A new _zipfile() accessor opens the archive under the lock on first use and returns it. Every read and write gets the archive through it. _open() opens under the lock, and _ensure_open() is overridden to call _zipfile(), because the base version checked _is_open outside the lock.
• After the first open, _sync_open switches mode "w" or "x" to "a".
• close() holds the lock until the archive is closed and the store is marked closed. Before, it marked the store closed first, so a thread that used the store in that gap reopened an archive whose central directory was not written yet.
• If closing the archive raises, the store records it and later opens raise RuntimeError. The archive may lack its central directory, and reopening it in append mode would make zipfile start a new archive and silently drop the earlier entries. clear() resets this, because it replaces the archive.
• move() holds the lock from closing the store until it reopens it at the new path, and opens a never-opened store first, so mode "x" refuses an existing file instead of moving it.
• clear() opens a never-opened "x" store before deleting the file, for the same reason. It then closes the store, removes the file if it is still there, and creates the new archive with "w", so it also replaces a file that is damaged or gone, and a failure partway leaves the store closed rather than open on a closed archive. It lifts the failed-close guard only once the old file is removed.
• Reusing a store backed by a file object after close() requires it to be readable and seekable, and raises io.UnsupportedOperation otherwise. On a write-only file, zipfile started a new archive after the old one and the earlier entries were lost. The first open is unchanged.
• Unpickling no longer opens the archive; an unpickled store opens it on first use. On main, every copy dask made of a store that was still writing reopened the file with "w" and corrupted it. A store that has already opened its archive for writing refuses to reopen a file that is not a zip archive and raises zipfile.BadZipFile, because appending to it would start a new archive after the entries already written. It opens the file itself rather than relying on zipfile.is_zipfile, which swallows OSError, so a directory or an unreadable file at the path surfaces as that error. This catches a copy used while the original is still in its first writing session, when the file has no central directory yet.
• list() iterates a copy of the names without holding the lock, so other threads are not blocked while a caller iterates.
• __getstate__ copies the state under the lock.
• ZipStoreAccessModeLiteral includes "x", and the docstring says "w" and "x" apply to the first open only.

Tests

• test_methods_open_store_on_first_use and test_listing_opens_store_on_first_use call each public method on a store that was constructed but never opened, and check return values; the first also checks the archive's entries. They replace test_lock_present from fix: ensure ZipStore is open before acquiring lock #3593, which used an already-open fixture and could not catch the missing check in get.
• test_reopen_keeps_entries covers modes w and x with a read after close(), move(), and a pickle round trip.
• test_close_blocks_concurrent_reopen, test_move_blocks_concurrent_reopen, and test_first_use_waits_for_lock start work on another thread while this thread is writing the central directory in close(), is inside move(), or holds the lock, and check that the other thread waits.
• test_list_does_not_hold_lock_while_iterating calls list_dir() from another thread partway through list().
• test_failed_close_blocks_reopen makes writing the central directory fail, checks that the next write raises, and that clear() recovers the store.
• test_failed_clear_leaves_store_closed, test_clear_exclusive_mode_keeps_existing_file, and test_move_exclusive_mode_keeps_existing_file cover clear() and move().
• test_write_after_close_keeps_entries, test_write_only_reuse_after_close_raises, and test_unseekable_reuse_after_close_raises cover file objects.
• test_unused_copy_of_open_writer_does_not_write and test_used_copy_of_open_writer_raises pickle a store that is still writing: an unused copy collected after the original closes leaves the archive intact, and a used copy raises.
• test_failed_clear_keeps_failed_close_guard and test_clear_read_mode_writable_store cover clear() after a failed close and on a mode="r", read_only=False store.
• test_clear_replaces_damaged_file, test_reopen_recreates_deleted_file, and test_first_open_append_on_empty_file cover a file that was replaced, removed, or empty between uses, and check that the reopen guard applies only to archives the store wrote. test_reopen_reports_directory_at_path checks that a directory at the path is not reported as an unfinished archive.
• test_unpickle_state_from_older_release unpickles state without the attributes this PR adds.
• The new tests fail 32 times against main's _zip.py, with no errors leaking into other tests. With this change, tests/test_store and tests/test_api.py pass.

Known leftovers, not changed here, and each also present on main:

• After a reopen in "a" mode, writing a key again adds a second entry with that name, as overwriting within one session already did.
• A file object opened in append mode ("a+b") is readable and seekable, but its writes always land at the end of the file, so reopening it after close() can corrupt the archive.
• A copy of a "w" or "x" store that was never opened applies the first-open mode when it is first used, so each copy that is used truncates the file or refuses it.
• Several stores writing to one archive at once, in threads or processes, is not supported. A copy that reopens the archive while the original is still in its first writing session raises zipfile.BadZipFile; other concurrent writers are not detected and can lose writes.

Bookkeeping

• The branch starts at the fix: ensure ZipStore is open before acquiring lock #3593 head (8688953ea) and merges upstream main (0ba2ca24a). The merge conflicts in _zip.py were resolved by taking main's version, and the fix is in its own commit on top. The test-file conflict was resolved by keeping both sides.
• The commits after adbed1886 came out of several review rounds.
• The changelog fragment is named after this PR.

🤖 Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.45%. Comparing base (319bfa4) to head (5b340cd).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4450      +/-   ##
==========================================
+ Coverage   94.43%   94.45%   +0.01%     
==========================================
  Files          93       93              
  Lines       13219    13244      +25     
==========================================
+ Hits        12483    12509      +26     
+ Misses        736      735       -1     

Files with missing lines	Coverage Δ
src/zarr/storage/_zip.py	98.76% <100.00%> (+0.60%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.