From 7154260222243935a5524313206abad762269556 Mon Sep 17 00:00:00 2001 From: Othman El Hammouchi Date: Fri, 21 Nov 2025 00:51:44 +0100 Subject: [PATCH 01/23] fix: ensure `ZipStore` is open before acquiring lock --- src/zarr/storage/_zip.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/zarr/storage/_zip.py b/src/zarr/storage/_zip.py index 72bf9e335a..bb32fb7ead 100644 --- a/src/zarr/storage/_zip.py +++ b/src/zarr/storage/_zip.py @@ -120,12 +120,18 @@ def __setstate__(self, state: dict[str, Any]) -> None: def close(self) -> None: # docstring inherited + if not self._is_open: + self._sync_open() + super().close() with self._lock: self._zf.close() async def clear(self) -> None: # docstring inherited + if not self._is_open: + self._sync_open() + with self._lock: self._check_writable() self._zf.close() @@ -188,6 +194,8 @@ async def get_partial_values( key_ranges: Iterable[tuple[str, ByteRequest | None]], ) -> list[Buffer | None]: # docstring inherited + if not self._is_open: + self._sync_open() out = [] with self._lock: for key, byte_range in key_ranges: @@ -222,6 +230,9 @@ async def set(self, key: str, value: Buffer) -> None: async def set_if_not_exists(self, key: str, value: Buffer) -> None: self._check_writable() + if not self._is_open: + self._sync_open() + with self._lock: members = self._zf.namelist() if key not in members: @@ -245,6 +256,9 @@ async def delete(self, key: str) -> None: async def exists(self, key: str) -> bool: # docstring inherited + if not self._is_open: + self._sync_open() + with self._lock: try: self._zf.getinfo(key) @@ -255,6 +269,9 @@ async def exists(self, key: str) -> bool: async def list(self) -> AsyncIterator[str]: # docstring inherited + if not self._is_open: + self._sync_open() + with self._lock: for key in self._zf.namelist(): yield key From 35f801963042cf4cfd5fc56003a5b640144d4f08 Mon Sep 17 00:00:00 2001 From: Othman El Hammouchi Date: Thu, 25 Dec 2025 12:18:08 +0100 Subject: [PATCH 02/23] refactor: Cleanup open check in ZipStore --- src/zarr/storage/_zip.py | 33 ++++++++++++++------------------- 1 file changed, 14 insertions(+), 19 deletions(-) diff --git a/src/zarr/storage/_zip.py b/src/zarr/storage/_zip.py index bb32fb7ead..776f07e39f 100644 --- a/src/zarr/storage/_zip.py +++ b/src/zarr/storage/_zip.py @@ -103,6 +103,10 @@ def _sync_open(self) -> None: self._is_open = True + def _sync_ensure_open(self): + if not self._is_open: + self._sync_open() + async def _open(self) -> None: self._sync_open() @@ -120,17 +124,15 @@ def __setstate__(self, state: dict[str, Any]) -> None: def close(self) -> None: # docstring inherited - if not self._is_open: - self._sync_open() - + self._sync_ensure_open() + super().close() with self._lock: self._zf.close() async def clear(self) -> None: # docstring inherited - if not self._is_open: - self._sync_open() + self._sync_ensure_open() with self._lock: self._check_writable() @@ -155,8 +157,7 @@ def _get( prototype: BufferPrototype, byte_range: ByteRequest | None = None, ) -> Buffer | None: - if not self._is_open: - self._sync_open() + self._sync_ensure_open() # docstring inherited try: with self._zf.open(key) as f: # will raise KeyError @@ -194,8 +195,7 @@ async def get_partial_values( key_ranges: Iterable[tuple[str, ByteRequest | None]], ) -> list[Buffer | None]: # docstring inherited - if not self._is_open: - self._sync_open() + self._sync_ensure_open() out = [] with self._lock: for key, byte_range in key_ranges: @@ -203,8 +203,7 @@ async def get_partial_values( return out def _set(self, key: str, value: Buffer) -> None: - if not self._is_open: - self._sync_open() + self._sync_ensure_open() # generally, this should be called inside a lock keyinfo = zipfile.ZipInfo(filename=key, date_time=time.localtime(time.time())[:6]) keyinfo.compress_type = self.compression @@ -218,8 +217,7 @@ def _set(self, key: str, value: Buffer) -> None: async def set(self, key: str, value: Buffer) -> None: # docstring inherited self._check_writable() - if not self._is_open: - self._sync_open() + self._sync_ensure_open() assert isinstance(key, str) if not isinstance(value, Buffer): raise TypeError( @@ -230,8 +228,7 @@ async def set(self, key: str, value: Buffer) -> None: async def set_if_not_exists(self, key: str, value: Buffer) -> None: self._check_writable() - if not self._is_open: - self._sync_open() + self._sync_ensure_open() with self._lock: members = self._zf.namelist() @@ -256,8 +253,7 @@ async def delete(self, key: str) -> None: async def exists(self, key: str) -> bool: # docstring inherited - if not self._is_open: - self._sync_open() + self._sync_ensure_open() with self._lock: try: @@ -269,8 +265,7 @@ async def exists(self, key: str) -> bool: async def list(self) -> AsyncIterator[str]: # docstring inherited - if not self._is_open: - self._sync_open() + self._sync_ensure_open() with self._lock: for key in self._zf.namelist(): From 05970a2a528e332edfdedfc7601becfe83039e9a Mon Sep 17 00:00:00 2001 From: Othman El Hammouchi Date: Thu, 25 Dec 2025 12:43:06 +0100 Subject: [PATCH 03/23] test: Add test to check that lock is present when required --- tests/test_store/test_zip.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/test_store/test_zip.py b/tests/test_store/test_zip.py index 744ee82945..7bfc4cfb0c 100644 --- a/tests/test_store/test_zip.py +++ b/tests/test_store/test_zip.py @@ -152,3 +152,17 @@ async def test_move(self, tmp_path: Path) -> None: assert destination.exists() assert not origin.exists() assert np.array_equal(array[...], np.arange(10)) + + async def test_lock_present(self, store: ZipStore) -> None: + buf = cpu.Buffer.from_bytes(b"bar") + await store.set("foo", buf) + await store.set_if_not_exists("foo", buf) + await store.exists("foo") + await store.get("foo", default_buffer_prototype()) + + async for _ in store.list(): + pass + + await store.clear() + + store.close() From 864e445aa733593b25fb559f34365ebcc1725e3e Mon Sep 17 00:00:00 2001 From: Othman El Hammouchi Date: Thu, 25 Dec 2025 12:56:17 +0100 Subject: [PATCH 04/23] refactor: Add return type to `_sync_ensure_open` --- src/zarr/storage/_zip.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/zarr/storage/_zip.py b/src/zarr/storage/_zip.py index 776f07e39f..bc6677043f 100644 --- a/src/zarr/storage/_zip.py +++ b/src/zarr/storage/_zip.py @@ -103,7 +103,7 @@ def _sync_open(self) -> None: self._is_open = True - def _sync_ensure_open(self): + def _sync_ensure_open(self) -> None: if not self._is_open: self._sync_open() From f85b29c9b4a44643bcb8b993ae03e6360f144dfd Mon Sep 17 00:00:00 2001 From: Othman El Hammouchi Date: Tue, 13 Jan 2026 10:04:51 +0100 Subject: [PATCH 05/23] docs: Add release note --- changes/3588.bugfix.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 changes/3588.bugfix.md diff --git a/changes/3588.bugfix.md b/changes/3588.bugfix.md new file mode 100644 index 0000000000..865a2cd075 --- /dev/null +++ b/changes/3588.bugfix.md @@ -0,0 +1 @@ +Fix missing `_lock` attribute error in ZipStore when calling some methods of before opening it From adbed1886ce4d7202984b1d62eb9dd3149edeaa8 Mon Sep 17 00:00:00 2001 From: Davis Vann Bennett Date: Tue, 29 Sep 2026 17:36:13 +0200 Subject: [PATCH 06/23] 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 zarr-developers/zarr-python#3593. Refs zarr-developers/zarr-python#3588 Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Othman El Hammouchi --- changes/3588.bugfix.md | 2 +- src/zarr/storage/_zip.py | 42 ++++++++++---------- tests/test_store/test_zip.py | 77 ++++++++++++++++++++++++++++++------ 3 files changed, 88 insertions(+), 33 deletions(-) diff --git a/changes/3588.bugfix.md b/changes/3588.bugfix.md index 865a2cd075..ea68727f33 100644 --- a/changes/3588.bugfix.md +++ b/changes/3588.bugfix.md @@ -1 +1 @@ -Fix missing `_lock` attribute error in ZipStore when calling some methods of before opening it +`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 reopened by `move()` or by unpickling, instead of truncating the archive or refusing to open it. diff --git a/src/zarr/storage/_zip.py b/src/zarr/storage/_zip.py index 6b9ebca561..9e773ae9fe 100644 --- a/src/zarr/storage/_zip.py +++ b/src/zarr/storage/_zip.py @@ -162,22 +162,33 @@ def __init__( self._zmode = mode self.compression = compression self.allowZip64 = allowZip64 + self._lock = threading.RLock() def _sync_open(self) -> None: if self._is_open: raise ValueError("store is already open") - self._lock = threading.RLock() - self._zf = zipfile.ZipFile( self.path if self.path is not None else self._fileobj, # type: ignore[arg-type] mode=self._zmode, compression=self.compression, allowZip64=self.allowZip64, ) + # "w" truncates and "x" refuses an existing file. Both apply only to the + # first open: reopening after close(), move(), or unpickling must keep + # the entries already written. + if self._zmode in ("w", "x"): + self._zmode = "a" self._is_open = True + def _zipfile(self) -> zipfile.ZipFile: + """Return the archive, opening it on first use.""" + with self._lock: + if not self._is_open: + self._sync_open() + return self._zf + async def _open(self) -> None: self._sync_open() @@ -197,6 +208,7 @@ def __getstate__(self) -> dict[str, Any]: def __setstate__(self, state: dict[str, Any]) -> None: self.__dict__ = state + self._lock = threading.RLock() self._is_open = False self._sync_open() @@ -216,7 +228,7 @@ async def clear(self) -> None: raise NotImplementedError( "clear() is not supported for a ZipStore backed by a file-like object" ) - self._zf.close() + self._zipfile().close() os.remove(self.path) self._zf = zipfile.ZipFile( self.path, mode="w", compression=self.compression, allowZip64=self.allowZip64 @@ -243,11 +255,9 @@ def _get( prototype: BufferPrototype, byte_range: ByteRequest | None = None, ) -> Buffer | None: - if not self._is_open: - self._sync_open() # docstring inherited try: - with self._zf.open(key) as f: # will raise KeyError + with self._zipfile().open(key) as f: # will raise KeyError if byte_range is None: return prototype.buffer.from_bytes(f.read()) elif isinstance(byte_range, RangeByteRequest): @@ -288,8 +298,6 @@ async def get_partial_values( return out def _set(self, key: str, value: Buffer) -> None: - if not self._is_open: - self._sync_open() # generally, this should be called inside a lock keyinfo = zipfile.ZipInfo(filename=key, date_time=time.localtime(time.time())[:6]) keyinfo.compress_type = self.compression @@ -298,13 +306,11 @@ def _set(self, key: str, value: Buffer) -> None: keyinfo.external_attr |= 0x10 # MS-DOS directory flag else: keyinfo.external_attr = 0o644 << 16 # ?rw-r--r-- - self._zf.writestr(keyinfo, value.to_bytes()) + self._zipfile().writestr(keyinfo, value.to_bytes()) async def set(self, key: str, value: Buffer) -> None: # docstring inherited self._check_writable() - if not self._is_open: - self._sync_open() if not isinstance(value, Buffer): raise TypeError( f"ZipStore.set(): `value` must be a Buffer instance. Got an instance of {type(value)} instead." @@ -315,7 +321,7 @@ async def set(self, key: str, value: Buffer) -> None: async def set_if_not_exists(self, key: str, value: Buffer) -> None: self._check_writable() with self._lock: - members = self._zf.namelist() + members = self._zipfile().namelist() if key not in members: self._set(key, value) @@ -337,11 +343,9 @@ async def delete(self, key: str) -> None: async def exists(self, key: str) -> bool: # docstring inherited - if not self._is_open: - self._sync_open() with self._lock: try: - self._zf.getinfo(key) + self._zipfile().getinfo(key) except KeyError: return False else: @@ -349,10 +353,8 @@ async def exists(self, key: str) -> bool: async def list(self) -> AsyncIterator[str]: # docstring inherited - if not self._is_open: - self._sync_open() with self._lock: - for key in self._zf.namelist(): + for key in self._zipfile().namelist(): yield key async def list_prefix(self, prefix: str) -> AsyncIterator[str]: @@ -363,11 +365,9 @@ async def list_prefix(self, prefix: str) -> AsyncIterator[str]: async def list_dir(self, prefix: str) -> AsyncIterator[str]: # docstring inherited - if not self._is_open: - self._sync_open() prefix = prefix.rstrip("/") - keys = self._zf.namelist() + keys = self._zipfile().namelist() seen = set() if prefix == "": keys_unique = {k.split("/")[0] for k in keys} diff --git a/tests/test_store/test_zip.py b/tests/test_store/test_zip.py index 31d0a7cbba..1189fbe6d2 100644 --- a/tests/test_store/test_zip.py +++ b/tests/test_store/test_zip.py @@ -188,20 +188,76 @@ async def test_move(self, tmp_path: Path) -> None: assert not origin.exists() assert np.array_equal(array[...], np.arange(10)) - async def test_lock_present(self, store: ZipStore) -> None: - buf = cpu.Buffer.from_bytes(b"bar") - await store.set("foo", buf) - await store.set_if_not_exists("foo", buf) - await store.exists("foo") - await store.get("foo", default_buffer_prototype()) + @pytest.mark.parametrize( + "call", + [ + lambda s: s.get("foo", default_buffer_prototype()), + lambda s: s.get_partial_values(default_buffer_prototype(), [("foo", None)]), + lambda s: s.exists("foo"), + lambda s: s.set("bar", cpu.Buffer.from_bytes(b"x")), + lambda s: s.set_if_not_exists("bar", cpu.Buffer.from_bytes(b"x")), + lambda s: s.clear(), + lambda s: s.delete("bar"), + lambda s: s.delete_dir("bar"), + lambda s: s.is_empty(""), + ], + ids=[ + "get", + "get_partial_values", + "exists", + "set", + "set_if_not_exists", + "clear", + "delete", + "delete_dir", + "is_empty", + ], + ) + async def test_methods_open_store_on_first_use( + self, store_kwargs: dict[str, Any], call: Any + ) -> None: + # every method works on a store that was constructed but never opened + seed = await self.store_cls.open(**store_kwargs) + await seed.set("foo", cpu.Buffer.from_bytes(b"bar")) + seed.close() - async for _ in store.list(): - pass + store = self.store_cls(**{**store_kwargs, "mode": "a"}) + assert not store._is_open + await call(store) + store.close() - await store.clear() + @pytest.mark.parametrize("method", ["list", "list_dir"]) + async def test_listing_opens_store_on_first_use( + self, store_kwargs: dict[str, Any], method: str + ) -> None: + seed = await self.store_cls.open(**store_kwargs) + await seed.set("foo", cpu.Buffer.from_bytes(b"bar")) + seed.close() + store = self.store_cls(**{**store_kwargs, "mode": "r"}) + args = () if method == "list" else ("",) + assert [k async for k in getattr(store, method)(*args)] == ["foo"] store.close() + @pytest.mark.parametrize("mode", ["w", "x"]) + @pytest.mark.parametrize("reopen", ["close_twice", "move", "pickle"]) + async def test_reopen_keeps_entries(self, tmp_path: Path, mode: str, reopen: str) -> None: + # "w" truncates and "x" refuses an existing file; neither may apply + # when a store that already wrote entries is opened again + store = ZipStore(tmp_path / "data.zip", mode=mode) # type: ignore[arg-type] + await store.set("foo", cpu.Buffer.from_bytes(b"bar")) + if reopen == "close_twice": + store.close() + store.close() + elif reopen == "move": + await store.move(tmp_path / "moved" / "data.zip") + store.close() + else: + store.close() + pickle.loads(pickle.dumps(store)).close() + with zipfile.ZipFile(store.path) as zf: # type: ignore[arg-type] + assert zf.namelist() == ["foo"] + class TestZipStoreFileObj: """ZipStore backed by an open binary file-like object instead of a path.""" @@ -366,8 +422,7 @@ class ZipStoreLifecycleMachine(RuleBasedStateMachine): Invariant under test: a constructed ZipStore can always be closed without raising, regardless of whether it was ever opened or did any I/O. This is a property-based generalization of the former example-based regression tests - for ZipStore.close() being called on a never-opened store (which raised - AttributeError because ``_lock`` is created lazily in ``_sync_open``). + for ZipStore.close() being called on a never-opened store. """ def __init__(self, tmp_path: Path) -> None: From 119e7a9be3b7bc2ac2993b39f4474a3318314054 Mon Sep 17 00:00:00 2001 From: Davis Vann Bennett Date: Tue, 29 Sep 2026 17:39:35 +0200 Subject: [PATCH 07/23] docs: rename changelog fragment to PR number Assisted-by: ClaudeCode:claude-opus-5-5 --- changes/{3588.bugfix.md => 4450.bugfix.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changes/{3588.bugfix.md => 4450.bugfix.md} (100%) diff --git a/changes/3588.bugfix.md b/changes/4450.bugfix.md similarity index 100% rename from changes/3588.bugfix.md rename to changes/4450.bugfix.md From d24126c50630f821b3b98bad3902f026cb2b159e Mon Sep 17 00:00:00 2001 From: Davis Vann Bennett Date: Tue, 29 Sep 2026 17:45:37 +0200 Subject: [PATCH 08/23] 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 --- changes/4450.bugfix.md | 2 +- tests/test_store/test_zip.py | 82 ++++++++++++++++++++++++------------ 2 files changed, 57 insertions(+), 27 deletions(-) diff --git a/changes/4450.bugfix.md b/changes/4450.bugfix.md index ea68727f33..f922c5a315 100644 --- a/changes/4450.bugfix.md +++ b/changes/4450.bugfix.md @@ -1 +1 @@ -`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 reopened by `move()` or by unpickling, instead of truncating the archive or refusing to open it. +`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 a store backed by a file object dropped its earlier entries when written to again after `close()`. Threads that use a store for the first time at the same moment no longer open the archive twice and lose writes. diff --git a/tests/test_store/test_zip.py b/tests/test_store/test_zip.py index 1189fbe6d2..3ed99af56c 100644 --- a/tests/test_store/test_zip.py +++ b/tests/test_store/test_zip.py @@ -189,42 +189,56 @@ async def test_move(self, tmp_path: Path) -> None: assert np.array_equal(array[...], np.arange(10)) @pytest.mark.parametrize( - "call", + ("call", "result", "keys"), [ - lambda s: s.get("foo", default_buffer_prototype()), - lambda s: s.get_partial_values(default_buffer_prototype(), [("foo", None)]), - lambda s: s.exists("foo"), - lambda s: s.set("bar", cpu.Buffer.from_bytes(b"x")), - lambda s: s.set_if_not_exists("bar", cpu.Buffer.from_bytes(b"x")), - lambda s: s.clear(), - lambda s: s.delete("bar"), - lambda s: s.delete_dir("bar"), - lambda s: s.is_empty(""), - ], - ids=[ - "get", - "get_partial_values", - "exists", - "set", - "set_if_not_exists", - "clear", - "delete", - "delete_dir", - "is_empty", + pytest.param( + lambda s: s.get("foo", default_buffer_prototype()), b"bar", ["foo"], id="get" + ), + pytest.param( + lambda s: s.get_partial_values(default_buffer_prototype(), [("foo", None)]), + [b"bar"], + ["foo"], + id="get_partial_values", + ), + pytest.param(lambda s: s.exists("foo"), True, ["foo"], id="exists"), + pytest.param( + lambda s: s.set("bar", cpu.Buffer.from_bytes(b"x")), + None, + ["foo", "bar"], + id="set", + ), + pytest.param( + lambda s: s.set_if_not_exists("bar", cpu.Buffer.from_bytes(b"x")), + None, + ["foo", "bar"], + id="set_if_not_exists", + ), + pytest.param(lambda s: s.clear(), None, [], id="clear"), + pytest.param(lambda s: s.delete("bar"), None, ["foo"], id="delete"), + pytest.param(lambda s: s.delete_dir("bar"), None, ["foo"], id="delete_dir"), + pytest.param(lambda s: s.is_empty(""), False, ["foo"], id="is_empty"), ], ) async def test_methods_open_store_on_first_use( - self, store_kwargs: dict[str, Any], call: Any + self, store_kwargs: dict[str, Any], call: Any, result: Any, keys: list[str] ) -> None: - # every method works on a store that was constructed but never opened + # every method works on a store that was constructed but never opened, + # and sees the entries already in the archive seed = await self.store_cls.open(**store_kwargs) await seed.set("foo", cpu.Buffer.from_bytes(b"bar")) seed.close() store = self.store_cls(**{**store_kwargs, "mode": "a"}) assert not store._is_open - await call(store) + out = await call(store) + if isinstance(out, list): + out = [b.to_bytes() for b in out] + elif isinstance(out, Buffer): + out = out.to_bytes() + assert out == result store.close() + with zipfile.ZipFile(store_kwargs["path"]) as zf: + assert zf.namelist() == keys @pytest.mark.parametrize("method", ["list", "list_dir"]) async def test_listing_opens_store_on_first_use( @@ -240,14 +254,17 @@ async def test_listing_opens_store_on_first_use( store.close() @pytest.mark.parametrize("mode", ["w", "x"]) - @pytest.mark.parametrize("reopen", ["close_twice", "move", "pickle"]) + @pytest.mark.parametrize("reopen", ["use_after_close", "move", "pickle"]) async def test_reopen_keeps_entries(self, tmp_path: Path, mode: str, reopen: str) -> None: # "w" truncates and "x" refuses an existing file; neither may apply # when a store that already wrote entries is opened again store = ZipStore(tmp_path / "data.zip", mode=mode) # type: ignore[arg-type] await store.set("foo", cpu.Buffer.from_bytes(b"bar")) - if reopen == "close_twice": + if reopen == "use_after_close": store.close() + value = await store.get("foo", default_buffer_prototype()) + assert value is not None + assert value.to_bytes() == b"bar" store.close() elif reopen == "move": await store.move(tmp_path / "moved" / "data.zip") @@ -289,6 +306,19 @@ def test_write_to_fileobj(self) -> None: array = zarr.open_array(roundtrip, mode="r") assert np.array_equal(array[...], np.arange(4)) + async def test_write_after_close_keeps_entries(self) -> None: + # reopening a file object after close() appends to the archive it + # already holds instead of starting a new one after it + buffer = io.BytesIO() + store = ZipStore(buffer, mode="w", read_only=False) + await store.set("foo", cpu.Buffer.from_bytes(b"1")) + store.close() + await store.set("bar", cpu.Buffer.from_bytes(b"2")) + store.close() + + with zipfile.ZipFile(io.BytesIO(buffer.getvalue())) as zf: + assert zf.namelist() == ["foo", "bar"] + async def test_clear_unsupported(self, zip_bytes: bytes) -> None: # clear() requires a filesystem location, so it raises a clear error # for file-object-backed stores From 4f166dc312ca0556eac4ed06599ecca398d114c4 Mon Sep 17 00:00:00 2001 From: Davis Vann Bennett Date: Tue, 29 Sep 2026 17:47:34 +0200 Subject: [PATCH 09/23] 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 --- changes/4450.bugfix.md | 2 +- src/zarr/storage/_zip.py | 8 +++++--- tests/test_store/test_zip.py | 30 ++++++++++++++++++++++++++++++ 3 files changed, 36 insertions(+), 4 deletions(-) diff --git a/changes/4450.bugfix.md b/changes/4450.bugfix.md index f922c5a315..61b65a5453 100644 --- a/changes/4450.bugfix.md +++ b/changes/4450.bugfix.md @@ -1 +1 @@ -`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 a store backed by a file object dropped its earlier entries when written to again after `close()`. Threads that use a store for the first time at the same moment no longer open the archive twice and lose writes. +`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 a store backed by a file object dropped its earlier entries when written to again after `close()`. 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 it. diff --git a/src/zarr/storage/_zip.py b/src/zarr/storage/_zip.py index 9e773ae9fe..bca97e86a7 100644 --- a/src/zarr/storage/_zip.py +++ b/src/zarr/storage/_zip.py @@ -214,11 +214,13 @@ def __setstate__(self, state: dict[str, Any]) -> None: def close(self) -> None: # docstring inherited - if not self._is_open: - return - super().close() + # hold the lock until the archive is closed: a thread that reopened it + # before its central directory was written would lose the entries with self._lock: + if not self._is_open: + return self._zf.close() + super().close() async def clear(self) -> None: # docstring inherited diff --git a/tests/test_store/test_zip.py b/tests/test_store/test_zip.py index 3ed99af56c..40bfc6d407 100644 --- a/tests/test_store/test_zip.py +++ b/tests/test_store/test_zip.py @@ -5,6 +5,7 @@ import pickle import shutil import tempfile +import threading import zipfile from typing import TYPE_CHECKING @@ -21,6 +22,7 @@ import zarr from zarr import create_array +from zarr.abc.store import Store from zarr.core.buffer import Buffer, cpu, default_buffer_prototype from zarr.core.sync import sync from zarr.storage import ZipStore @@ -275,6 +277,34 @@ async def test_reopen_keeps_entries(self, tmp_path: Path, mode: str, reopen: str with zipfile.ZipFile(store.path) as zf: # type: ignore[arg-type] assert zf.namelist() == ["foo"] + async def test_close_blocks_concurrent_reopen( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + # a thread that uses the store while close() runs must wait until the + # archive is closed, or it reopens a file with no central directory + store = ZipStore(tmp_path / "data.zip", mode="w") + await store.set("foo", cpu.Buffer.from_bytes(b"1")) + + writer = threading.Thread( + target=lambda: sync(store.set("bar", cpu.Buffer.from_bytes(b"2"))) + ) + mark_closed = Store.close + + def mark_closed_then_write(self: Store) -> None: + # the store now reports closed; start a write before close() returns + mark_closed(self) + writer.start() + writer.join(timeout=0.2) + + monkeypatch.setattr(Store, "close", mark_closed_then_write) + store.close() + monkeypatch.undo() + writer.join() + store.close() + + with zipfile.ZipFile(store.path) as zf: # type: ignore[arg-type] + assert zf.namelist() == ["foo", "bar"] + class TestZipStoreFileObj: """ZipStore backed by an open binary file-like object instead of a path.""" From fdf16c7b81895cbcf5673fea8cdd1527dfebadf2 Mon Sep 17 00:00:00 2001 From: Davis Vann Bennett Date: Tue, 29 Sep 2026 17:54:24 +0200 Subject: [PATCH 10/23] 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 --- src/zarr/storage/_zip.py | 18 +++++++++------ tests/test_store/test_zip.py | 44 ++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 7 deletions(-) diff --git a/src/zarr/storage/_zip.py b/src/zarr/storage/_zip.py index bca97e86a7..b2f4cff2f1 100644 --- a/src/zarr/storage/_zip.py +++ b/src/zarr/storage/_zip.py @@ -190,7 +190,8 @@ def _zipfile(self) -> zipfile.ZipFile: return self._zf async def _open(self) -> None: - self._sync_open() + with self._lock: + self._sync_open() def __getstate__(self) -> dict[str, Any]: if self.path is None: @@ -210,7 +211,7 @@ def __setstate__(self, state: dict[str, Any]) -> None: self.__dict__ = state self._lock = threading.RLock() self._is_open = False - self._sync_open() + self._zipfile() def close(self) -> None: # docstring inherited @@ -230,6 +231,7 @@ async def clear(self) -> None: raise NotImplementedError( "clear() is not supported for a ZipStore backed by a file-like object" ) + # opening first keeps mode "x" from deleting a file it may not claim self._zipfile().close() os.remove(self.path) self._zf = zipfile.ZipFile( @@ -395,8 +397,10 @@ async def move(self, path: Path | str) -> None: ) if isinstance(path, str): path = Path(path) - self.close() - os.makedirs(path.parent, exist_ok=True) - shutil.move(self.path, path) - self.path = path - await self._open() + # hold the lock so that no thread reopens the old path mid-move + with self._lock: + self.close() + os.makedirs(path.parent, exist_ok=True) + shutil.move(self.path, path) + self.path = path + self._sync_open() diff --git a/tests/test_store/test_zip.py b/tests/test_store/test_zip.py index 40bfc6d407..2f631c8552 100644 --- a/tests/test_store/test_zip.py +++ b/tests/test_store/test_zip.py @@ -305,6 +305,50 @@ def mark_closed_then_write(self: Store) -> None: with zipfile.ZipFile(store.path) as zf: # type: ignore[arg-type] assert zf.namelist() == ["foo", "bar"] + async def test_move_blocks_concurrent_reopen( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + # a thread that uses the store while move() runs must wait for the + # move, or it reopens the old path between close and reopen + origin = tmp_path / "data.zip" + destination = tmp_path / "moved" / "data.zip" + store = ZipStore(origin, mode="w") + await store.set("foo", cpu.Buffer.from_bytes(b"1")) + + writer = threading.Thread( + target=lambda: sync(store.set("bar", cpu.Buffer.from_bytes(b"2"))) + ) + move_file = shutil.move + + def write_then_move(src: Any, dst: Any) -> Any: + # the store is closed here; start a write before move() reopens it + writer.start() + writer.join(timeout=0.2) + return move_file(src, dst) + + monkeypatch.setattr(shutil, "move", write_then_move) + await store.move(destination) + monkeypatch.undo() + writer.join() + store.close() + + assert not origin.exists() + with zipfile.ZipFile(destination) as zf: + assert zf.namelist() == ["foo", "bar"] + + async def test_clear_exclusive_mode_keeps_existing_file(self, tmp_path: Path) -> None: + # a never-opened "x" store has not claimed the file, so clear() must + # refuse it the way the first open would instead of deleting it + path = tmp_path / "data.zip" + with zipfile.ZipFile(path, mode="w") as zf: + zf.writestr("foo", b"1") + + store = ZipStore(path, mode="x") # type: ignore[arg-type] + with pytest.raises(FileExistsError): + await store.clear() + with zipfile.ZipFile(path) as zf: + assert zf.namelist() == ["foo"] + class TestZipStoreFileObj: """ZipStore backed by an open binary file-like object instead of a path.""" From 62cb610912f26aac9bd98418e937836574bd9881 Mon Sep 17 00:00:00 2001 From: Davis Vann Bennett Date: Tue, 29 Sep 2026 17:54:36 +0200 Subject: [PATCH 11/23] docs: note move() in the ZipStore changelog fragment Assisted-by: ClaudeCode:claude-opus-5-5 --- changes/4450.bugfix.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changes/4450.bugfix.md b/changes/4450.bugfix.md index 61b65a5453..b5f2c11b09 100644 --- a/changes/4450.bugfix.md +++ b/changes/4450.bugfix.md @@ -1 +1 @@ -`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 a store backed by a file object dropped its earlier entries when written to again after `close()`. 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 it. +`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 a store backed by a file object dropped its earlier entries when written to again after `close()`. 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. From edf2a0c4c01c0651402f4be28f0b09d2a0e43c4b Mon Sep 17 00:00:00 2001 From: Davis Vann Bennett Date: Tue, 29 Sep 2026 17:56:07 +0200 Subject: [PATCH 12/23] 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 --- changes/4450.bugfix.md | 2 +- src/zarr/storage/_zip.py | 2 +- tests/test_store/test_zip.py | 10 +++++++--- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/changes/4450.bugfix.md b/changes/4450.bugfix.md index b5f2c11b09..e2c5402496 100644 --- a/changes/4450.bugfix.md +++ b/changes/4450.bugfix.md @@ -1 +1 @@ -`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 a store backed by a file object dropped its earlier entries when written to again after `close()`. 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. +`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 a store backed by a file object dropped its earlier entries when written to again after `close()`. 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. The type of the `mode` parameter now includes `"x"`, which the store already accepted at runtime. diff --git a/src/zarr/storage/_zip.py b/src/zarr/storage/_zip.py index b2f4cff2f1..9d89ea8933 100644 --- a/src/zarr/storage/_zip.py +++ b/src/zarr/storage/_zip.py @@ -21,7 +21,7 @@ if TYPE_CHECKING: from collections.abc import AsyncIterator, Iterable -ZipStoreAccessModeLiteral = Literal["r", "w", "a"] +ZipStoreAccessModeLiteral = Literal["r", "w", "a", "x"] class _RawReaderAdapter(io.RawIOBase): diff --git a/tests/test_store/test_zip.py b/tests/test_store/test_zip.py index 2f631c8552..7c531cb6a3 100644 --- a/tests/test_store/test_zip.py +++ b/tests/test_store/test_zip.py @@ -32,6 +32,8 @@ from pathlib import Path from typing import Any + from zarr.storage._zip import ZipStoreAccessModeLiteral + # TODO: work out where this is coming from and fix pytestmark = [ @@ -257,10 +259,12 @@ async def test_listing_opens_store_on_first_use( @pytest.mark.parametrize("mode", ["w", "x"]) @pytest.mark.parametrize("reopen", ["use_after_close", "move", "pickle"]) - async def test_reopen_keeps_entries(self, tmp_path: Path, mode: str, reopen: str) -> None: + async def test_reopen_keeps_entries( + self, tmp_path: Path, mode: ZipStoreAccessModeLiteral, reopen: str + ) -> None: # "w" truncates and "x" refuses an existing file; neither may apply # when a store that already wrote entries is opened again - store = ZipStore(tmp_path / "data.zip", mode=mode) # type: ignore[arg-type] + store = ZipStore(tmp_path / "data.zip", mode=mode) await store.set("foo", cpu.Buffer.from_bytes(b"bar")) if reopen == "use_after_close": store.close() @@ -343,7 +347,7 @@ async def test_clear_exclusive_mode_keeps_existing_file(self, tmp_path: Path) -> with zipfile.ZipFile(path, mode="w") as zf: zf.writestr("foo", b"1") - store = ZipStore(path, mode="x") # type: ignore[arg-type] + store = ZipStore(path, mode="x") with pytest.raises(FileExistsError): await store.clear() with zipfile.ZipFile(path) as zf: From 698c2c09e225e1754937f532a27759605fd31df6 Mon Sep 17 00:00:00 2001 From: Davis Vann Bennett Date: Tue, 29 Sep 2026 18:02:15 +0200 Subject: [PATCH 13/23] 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 --- changes/4450.bugfix.md | 2 +- src/zarr/storage/_zip.py | 14 ++++++++++++++ tests/test_store/test_zip.py | 20 ++++++++++++++++++-- 3 files changed, 33 insertions(+), 3 deletions(-) diff --git a/changes/4450.bugfix.md b/changes/4450.bugfix.md index e2c5402496..82f86de9cb 100644 --- a/changes/4450.bugfix.md +++ b/changes/4450.bugfix.md @@ -1 +1 @@ -`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 a store backed by a file object dropped its earlier entries when written to again after `close()`. 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. The type of the `mode` parameter now includes `"x"`, which the store already accepted at runtime. +`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 a store backed by a file object dropped its earlier entries when written to again after `close()`. A store backed by a readable file object now keeps them, and one backed by a write-only 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. The type of the `mode` parameter now includes `"x"`, which the store already accepted at runtime. diff --git a/src/zarr/storage/_zip.py b/src/zarr/storage/_zip.py index 9d89ea8933..60b90f62c4 100644 --- a/src/zarr/storage/_zip.py +++ b/src/zarr/storage/_zip.py @@ -163,10 +163,23 @@ def __init__( self.compression = compression self.allowZip64 = allowZip64 self._lock = threading.RLock() + self._was_opened = False def _sync_open(self) -> None: if self._is_open: raise ValueError("store is already open") + if ( + self.path is None + and self._fileobj is not None + and self._was_opened + and not self._fileobj.readable() + ): + # reopening appends, which needs to read the archive back; zipfile + # would instead start a new archive and drop the earlier entries + raise io.UnsupportedOperation( + "a ZipStore backed by a write-only file object cannot be used " + "again after close(), because the archive it wrote cannot be read back" + ) self._zf = zipfile.ZipFile( self.path if self.path is not None else self._fileobj, # type: ignore[arg-type] @@ -180,6 +193,7 @@ def _sync_open(self) -> None: if self._zmode in ("w", "x"): self._zmode = "a" + self._was_opened = True self._is_open = True def _zipfile(self) -> zipfile.ZipFile: diff --git a/tests/test_store/test_zip.py b/tests/test_store/test_zip.py index 7c531cb6a3..8185cec4fd 100644 --- a/tests/test_store/test_zip.py +++ b/tests/test_store/test_zip.py @@ -303,7 +303,8 @@ def mark_closed_then_write(self: Store) -> None: monkeypatch.setattr(Store, "close", mark_closed_then_write) store.close() monkeypatch.undo() - writer.join() + writer.join(timeout=5) + assert not writer.is_alive() store.close() with zipfile.ZipFile(store.path) as zf: # type: ignore[arg-type] @@ -333,7 +334,8 @@ def write_then_move(src: Any, dst: Any) -> Any: monkeypatch.setattr(shutil, "move", write_then_move) await store.move(destination) monkeypatch.undo() - writer.join() + writer.join(timeout=5) + assert not writer.is_alive() store.close() assert not origin.exists() @@ -397,6 +399,20 @@ async def test_write_after_close_keeps_entries(self) -> None: with zipfile.ZipFile(io.BytesIO(buffer.getvalue())) as zf: assert zf.namelist() == ["foo", "bar"] + async def test_write_only_reuse_after_close_raises(self, tmp_path: Path) -> None: + # a write-only file object cannot be read back, so reopening it would + # start a new archive and drop the entries already written + path = tmp_path / "data.zip" + with path.open("wb") as f: + store = ZipStore(f, mode="w", read_only=False) + await store.set("foo", cpu.Buffer.from_bytes(b"1")) + store.close() + with pytest.raises(io.UnsupportedOperation, match="write-only"): + await store.set("bar", cpu.Buffer.from_bytes(b"2")) + + with zipfile.ZipFile(path) as zf: + assert zf.namelist() == ["foo"] + async def test_clear_unsupported(self, zip_bytes: bytes) -> None: # clear() requires a filesystem location, so it raises a clear error # for file-object-backed stores From 45cd41fbc4c9c3873369ed984cf2f83e931b4c84 Mon Sep 17 00:00:00 2001 From: Davis Vann Bennett Date: Tue, 29 Sep 2026 18:12:13 +0200 Subject: [PATCH 14/23] 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 --- changes/4450.bugfix.md | 2 +- src/zarr/storage/_zip.py | 31 ++++++++++++++----- tests/test_store/test_zip.py | 60 +++++++++++++++++++++++++++++++++++- 3 files changed, 83 insertions(+), 10 deletions(-) diff --git a/changes/4450.bugfix.md b/changes/4450.bugfix.md index 82f86de9cb..bd06204608 100644 --- a/changes/4450.bugfix.md +++ b/changes/4450.bugfix.md @@ -1 +1 @@ -`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 a store backed by a file object dropped its earlier entries when written to again after `close()`. A store backed by a readable file object now keeps them, and one backed by a write-only 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. The type of the `mode` parameter now includes `"x"`, which the store already accepted at runtime. +`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 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, the store is still marked closed, so the next use reopens the archive instead of failing on the closed handle. The type of the `mode` parameter now includes `"x"`, which the store already accepted at runtime. diff --git a/src/zarr/storage/_zip.py b/src/zarr/storage/_zip.py index 60b90f62c4..bbcf687b0d 100644 --- a/src/zarr/storage/_zip.py +++ b/src/zarr/storage/_zip.py @@ -85,10 +85,15 @@ class ZipStore(Store): can only be used for reading (`mode="r"`). The file object must stay open for the lifetime of the store, and operations that require a filesystem location (`clear`, `move`, pickling) are not supported. + Using the store again after `close()` reopens the archive to append + to it, which requires a file object that is readable and seekable; + otherwise it raises `io.UnsupportedOperation`. mode : str, optional One of 'r' to read an existing file, 'w' to truncate and write a new file, 'a' to append to an existing file, or 'x' to exclusively create - and write a new file. + and write a new file. 'w' and 'x' apply to the first open only; the + store reopens its archive with 'a' after `close()`, `move()`, or + unpickling, so the entries it already wrote are kept. compression : int, optional Compression method to use when writing to the archive. allowZip64 : bool, optional @@ -172,13 +177,15 @@ def _sync_open(self) -> None: self.path is None and self._fileobj is not None and self._was_opened - and not self._fileobj.readable() + and not (self._fileobj.readable() and self._fileobj.seekable()) ): - # reopening appends, which needs to read the archive back; zipfile - # would instead start a new archive and drop the earlier entries + # reopening appends, which needs to read the archive back; on a + # write-only file zipfile would start a new archive and drop the + # earlier entries raise io.UnsupportedOperation( - "a ZipStore backed by a write-only file object cannot be used " - "again after close(), because the archive it wrote cannot be read back" + "a ZipStore backed by a file object that is not readable and " + "seekable cannot be used again after close(), because the " + "archive it wrote cannot be read back" ) self._zf = zipfile.ZipFile( @@ -207,6 +214,10 @@ async def _open(self) -> None: with self._lock: self._sync_open() + async def _ensure_open(self) -> None: + # the base class checks _is_open outside the lock + self._zipfile() + def __getstate__(self) -> dict[str, Any]: if self.path is None: # A path-backed store pickles its path and reopens the file on @@ -234,8 +245,12 @@ def close(self) -> None: with self._lock: if not self._is_open: return - self._zf.close() - super().close() + try: + self._zf.close() + finally: + # a failed close still leaves the handle unusable; mark the + # store closed so the next use reopens the archive + super().close() async def clear(self) -> None: # docstring inherited diff --git a/tests/test_store/test_zip.py b/tests/test_store/test_zip.py index 8185cec4fd..0dfa5c7338 100644 --- a/tests/test_store/test_zip.py +++ b/tests/test_store/test_zip.py @@ -7,6 +7,7 @@ import tempfile import threading import zipfile +from concurrent.futures import ThreadPoolExecutor, wait from typing import TYPE_CHECKING import numpy as np @@ -29,6 +30,7 @@ from zarr.testing.store import StoreTests if TYPE_CHECKING: + from collections.abc import AsyncIterator from pathlib import Path from typing import Any @@ -43,6 +45,10 @@ ] +async def _drain(keys: AsyncIterator[str]) -> list[str]: + return [k async for k in keys] + + class TestZipStore(StoreTests[ZipStore, cpu.Buffer]): store_cls = ZipStore buffer_cls = cpu.Buffer @@ -355,6 +361,47 @@ async def test_clear_exclusive_mode_keeps_existing_file(self, tmp_path: Path) -> with zipfile.ZipFile(path) as zf: assert zf.namelist() == ["foo"] + @pytest.mark.parametrize( + "use", + [ + pytest.param(lambda s: s._ensure_open(), id="ensure_open"), + pytest.param(lambda s: _drain(s.list_dir("")), id="list_dir"), + ], + ) + async def test_first_use_waits_for_lock(self, tmp_path: Path, use: Any) -> None: + # a thread's first use must wait while another thread holds the lock, + # then find the archive that thread opened instead of opening it again + store = ZipStore(tmp_path / "data.zip", mode="w") + with ThreadPoolExecutor(max_workers=1) as pool: + with store._lock: + first_use = pool.submit(sync, use(store)) + wait([first_use], timeout=0.2) + assert not store._is_open + store._zipfile() + first_use.result(timeout=5) + store.close() + + async def test_failed_close_leaves_store_reusable(self, tmp_path: Path) -> None: + # if closing the archive raises, the store is still marked closed so + # the next use reopens it instead of hitting a dead handle + store = ZipStore(tmp_path / "data.zip", mode="w") + await store.set("foo", cpu.Buffer.from_bytes(b"1")) + close_archive = store._zf.close + + def close_then_fail() -> None: + close_archive() + raise OSError("disk full") + + store._zf.close = close_then_fail # type: ignore[method-assign] + with pytest.raises(OSError, match="disk full"): + store.close() + assert not store._is_open + + await store.set("bar", cpu.Buffer.from_bytes(b"2")) + store.close() + with zipfile.ZipFile(store.path) as zf: # type: ignore[arg-type] + assert zf.namelist() == ["foo", "bar"] + class TestZipStoreFileObj: """ZipStore backed by an open binary file-like object instead of a path.""" @@ -407,12 +454,23 @@ async def test_write_only_reuse_after_close_raises(self, tmp_path: Path) -> None store = ZipStore(f, mode="w", read_only=False) await store.set("foo", cpu.Buffer.from_bytes(b"1")) store.close() - with pytest.raises(io.UnsupportedOperation, match="write-only"): + with pytest.raises(io.UnsupportedOperation, match="readable and seekable"): await store.set("bar", cpu.Buffer.from_bytes(b"2")) with zipfile.ZipFile(path) as zf: assert zf.namelist() == ["foo"] + async def test_unseekable_reuse_after_close_raises(self) -> None: + # a file object that cannot seek cannot be read back either, so reuse + # after close() raises the same error as a write-only one + buffer = io.BytesIO() + pipe = io.BufferedRWPair(buffer, buffer) # readable, not seekable + store = ZipStore(pipe, mode="w", read_only=False) # type: ignore[arg-type] + await store.set("foo", cpu.Buffer.from_bytes(b"1")) + store.close() + with pytest.raises(io.UnsupportedOperation, match="readable and seekable"): + await store.set("bar", cpu.Buffer.from_bytes(b"2")) + async def test_clear_unsupported(self, zip_bytes: bytes) -> None: # clear() requires a filesystem location, so it raises a clear error # for file-object-backed stores From ee6033fe63ab9119f97b99bdd4d3738a06543d82 Mon Sep 17 00:00:00 2001 From: Davis Vann Bennett Date: Tue, 29 Sep 2026 18:28:55 +0200 Subject: [PATCH 15/23] 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 --- changes/4450.bugfix.md | 2 +- src/zarr/storage/_zip.py | 33 ++++++++++++++++------ tests/test_store/test_zip.py | 53 ++++++++++++++++++++++++++++-------- 3 files changed, 67 insertions(+), 21 deletions(-) diff --git a/changes/4450.bugfix.md b/changes/4450.bugfix.md index bd06204608..530f3e2d69 100644 --- a/changes/4450.bugfix.md +++ b/changes/4450.bugfix.md @@ -1 +1 @@ -`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 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, the store is still marked closed, so the next use reopens the archive instead of failing on the closed handle. The type of the `mode` parameter now includes `"x"`, which the store already accepted at runtime. +`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 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, the store now raises `RuntimeError` on its next use, instead of reopening an archive that may lack its central directory and silently starting a new one. 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. diff --git a/src/zarr/storage/_zip.py b/src/zarr/storage/_zip.py index bbcf687b0d..3479a1bf32 100644 --- a/src/zarr/storage/_zip.py +++ b/src/zarr/storage/_zip.py @@ -85,15 +85,17 @@ class ZipStore(Store): can only be used for reading (`mode="r"`). The file object must stay open for the lifetime of the store, and operations that require a filesystem location (`clear`, `move`, pickling) are not supported. - Using the store again after `close()` reopens the archive to append - to it, which requires a file object that is readable and seekable; - otherwise it raises `io.UnsupportedOperation`. + Using the store again after `close()` reopens the archive, which + requires a file object that is readable and seekable; otherwise it + raises `io.UnsupportedOperation`. mode : str, optional One of 'r' to read an existing file, 'w' to truncate and write a new file, 'a' to append to an existing file, or 'x' to exclusively create and write a new file. 'w' and 'x' apply to the first open only; the store reopens its archive with 'a' after `close()`, `move()`, or - unpickling, so the entries it already wrote are kept. + unpickling, so the entries it already wrote are kept. If `close()` + raises, the archive may be incomplete, and every later use of the + store raises `RuntimeError` instead of reopening it. compression : int, optional Compression method to use when writing to the archive. allowZip64 : bool, optional @@ -169,10 +171,18 @@ def __init__( self.allowZip64 = allowZip64 self._lock = threading.RLock() self._was_opened = False + self._close_failed = False def _sync_open(self) -> None: if self._is_open: raise ValueError("store is already open") + if self._close_failed: + # the central directory may be missing; appending to such a file + # makes zipfile start a new archive and drop the earlier entries + raise RuntimeError( + f"closing the archive of {self!r} failed, so it may be incomplete; " + "the store will not reopen it" + ) if ( self.path is None and self._fileobj is not None @@ -234,6 +244,7 @@ def __getstate__(self) -> dict[str, Any]: def __setstate__(self, state: dict[str, Any]) -> None: self.__dict__ = state + self.__dict__.setdefault("_close_failed", False) self._lock = threading.RLock() self._is_open = False self._zipfile() @@ -247,9 +258,10 @@ def close(self) -> None: return try: self._zf.close() + except BaseException: + self._close_failed = True + raise finally: - # a failed close still leaves the handle unusable; mark the - # store closed so the next use reopens the archive super().close() async def clear(self) -> None: @@ -386,9 +398,10 @@ async def exists(self, key: str) -> bool: async def list(self) -> AsyncIterator[str]: # docstring inherited - with self._lock: - for key in self._zipfile().namelist(): - yield key + # namelist() is a copy; holding the lock across yield would block + # other threads for as long as the caller iterates + for key in self._zipfile().namelist(): + yield key async def list_prefix(self, prefix: str) -> AsyncIterator[str]: # docstring inherited @@ -428,6 +441,8 @@ async def move(self, path: Path | str) -> None: path = Path(path) # hold the lock so that no thread reopens the old path mid-move with self._lock: + # opening first keeps mode "x" from moving a file it may not claim + self._zipfile() self.close() os.makedirs(path.parent, exist_ok=True) shutil.move(self.path, path) diff --git a/tests/test_store/test_zip.py b/tests/test_store/test_zip.py index 0dfa5c7338..960fe65d9e 100644 --- a/tests/test_store/test_zip.py +++ b/tests/test_store/test_zip.py @@ -8,7 +8,7 @@ import threading import zipfile from concurrent.futures import ThreadPoolExecutor, wait -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, cast import numpy as np import pytest @@ -30,7 +30,7 @@ from zarr.testing.store import StoreTests if TYPE_CHECKING: - from collections.abc import AsyncIterator + from collections.abc import AsyncGenerator, AsyncIterator from pathlib import Path from typing import Any @@ -381,26 +381,57 @@ async def test_first_use_waits_for_lock(self, tmp_path: Path, use: Any) -> None: first_use.result(timeout=5) store.close() - async def test_failed_close_leaves_store_reusable(self, tmp_path: Path) -> None: - # if closing the archive raises, the store is still marked closed so - # the next use reopens it instead of hitting a dead handle + async def test_failed_close_blocks_reopen( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + # if the central directory could not be written, reopening in append + # mode would start a new archive and drop the entries, so later use + # raises instead store = ZipStore(tmp_path / "data.zip", mode="w") await store.set("foo", cpu.Buffer.from_bytes(b"1")) - close_archive = store._zf.close - def close_then_fail() -> None: - close_archive() + def disk_full() -> None: raise OSError("disk full") - store._zf.close = close_then_fail # type: ignore[method-assign] + monkeypatch.setattr(store._zf, "_write_end_record", disk_full) with pytest.raises(OSError, match="disk full"): store.close() assert not store._is_open + with pytest.raises(RuntimeError, match="closing the archive"): + await store.set("bar", cpu.Buffer.from_bytes(b"2")) + async def test_list_does_not_hold_lock_while_iterating(self, tmp_path: Path) -> None: + # another thread can use the store while a caller is partway + # through iterating list() + store = ZipStore(tmp_path / "data.zip", mode="w") + await store.set("foo", cpu.Buffer.from_bytes(b"1")) await store.set("bar", cpu.Buffer.from_bytes(b"2")) + # list() is an async generator; the annotation hides aclose() + keys = cast("AsyncGenerator[str, None]", store.list()) + pool = ThreadPoolExecutor(max_workers=1) + try: + assert await anext(keys) == "foo" + other = pool.submit(sync, _drain(store.list_dir(""))) + assert sorted(other.result(timeout=5)) == ["bar", "foo"] + finally: + await keys.aclose() + pool.shutdown() store.close() - with zipfile.ZipFile(store.path) as zf: # type: ignore[arg-type] - assert zf.namelist() == ["foo", "bar"] + + async def test_move_exclusive_mode_keeps_existing_file(self, tmp_path: Path) -> None: + # a never-opened "x" store has not claimed the file, so move() must + # refuse it the way the first open would instead of moving it + origin = tmp_path / "data.zip" + destination = tmp_path / "moved" / "data.zip" + with zipfile.ZipFile(origin, mode="w") as zf: + zf.writestr("foo", b"1") + + store = ZipStore(origin, mode="x") + with pytest.raises(FileExistsError): + await store.move(destination) + assert not destination.exists() + with zipfile.ZipFile(origin) as zf: + assert zf.namelist() == ["foo"] class TestZipStoreFileObj: From c93bfdadebb4c79665458ef948df9f94ae6eaf3a Mon Sep 17 00:00:00 2001 From: Davis Vann Bennett Date: Tue, 29 Sep 2026 18:32:47 +0200 Subject: [PATCH 16/23] 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 --- src/zarr/storage/_zip.py | 13 +++++++------ tests/test_store/test_zip.py | 22 ++++++++++++++++++++++ 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/src/zarr/storage/_zip.py b/src/zarr/storage/_zip.py index 3479a1bf32..4132bca5d6 100644 --- a/src/zarr/storage/_zip.py +++ b/src/zarr/storage/_zip.py @@ -272,12 +272,13 @@ async def clear(self) -> None: raise NotImplementedError( "clear() is not supported for a ZipStore backed by a file-like object" ) - # opening first keeps mode "x" from deleting a file it may not claim - self._zipfile().close() - os.remove(self.path) - self._zf = zipfile.ZipFile( - self.path, mode="w", compression=self.compression, allowZip64=self.allowZip64 - ) + # opening first keeps mode "x" from truncating a file it may not claim + self._zipfile() + self.close() + # "w" truncates; if the open fails the store stays closed, and the + # next use tries the truncating open again + self._zmode = "w" + self._sync_open() def __str__(self) -> str: if self.path is None: diff --git a/tests/test_store/test_zip.py b/tests/test_store/test_zip.py index 960fe65d9e..068899caad 100644 --- a/tests/test_store/test_zip.py +++ b/tests/test_store/test_zip.py @@ -348,6 +348,28 @@ def write_then_move(src: Any, dst: Any) -> Any: with zipfile.ZipFile(destination) as zf: assert zf.namelist() == ["foo", "bar"] + async def test_failed_clear_leaves_store_closed( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + # if clear() fails after closing the archive, the store is closed + # rather than left open on a closed handle, and the next use clears + store = ZipStore(tmp_path / "data.zip", mode="w") + await store.set("foo", cpu.Buffer.from_bytes(b"1")) + + def unavailable(*args: Any, **kwargs: Any) -> None: + raise OSError("unavailable") + + with monkeypatch.context() as m: + m.setattr(zipfile, "ZipFile", unavailable) + with pytest.raises(OSError, match="unavailable"): + await store.clear() + assert not store._is_open + + await store.set("bar", cpu.Buffer.from_bytes(b"2")) + store.close() + with zipfile.ZipFile(store.path) as zf: # type: ignore[arg-type] + assert zf.namelist() == ["bar"] + async def test_clear_exclusive_mode_keeps_existing_file(self, tmp_path: Path) -> None: # a never-opened "x" store has not claimed the file, so clear() must # refuse it the way the first open would instead of deleting it From c3c34209ed4108b08ed09409a42f8088a630ec76 Mon Sep 17 00:00:00 2001 From: Davis Vann Bennett Date: Tue, 29 Sep 2026 18:41:53 +0200 Subject: [PATCH 17/23] 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 --- changes/4450.bugfix.md | 2 +- src/zarr/storage/_zip.py | 30 ++++++++++++++++++++---------- tests/test_store/test_zip.py | 22 ++++++++++++++++++++++ 3 files changed, 43 insertions(+), 11 deletions(-) diff --git a/changes/4450.bugfix.md b/changes/4450.bugfix.md index 530f3e2d69..a643bd93f1 100644 --- a/changes/4450.bugfix.md +++ b/changes/4450.bugfix.md @@ -1 +1 @@ -`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 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, the store now raises `RuntimeError` on its next use, instead of reopening an archive that may lack its central directory and silently starting a new one. 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. +`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 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 read or write now raises `RuntimeError`, instead of reopening an archive that may lack its central directory and silently starting a new one; `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. 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. diff --git a/src/zarr/storage/_zip.py b/src/zarr/storage/_zip.py index 4132bca5d6..27d4849357 100644 --- a/src/zarr/storage/_zip.py +++ b/src/zarr/storage/_zip.py @@ -94,8 +94,9 @@ class ZipStore(Store): and write a new file. 'w' and 'x' apply to the first open only; the store reopens its archive with 'a' after `close()`, `move()`, or unpickling, so the entries it already wrote are kept. If `close()` - raises, the archive may be incomplete, and every later use of the - store raises `RuntimeError` instead of reopening it. + raises, the archive may be incomplete, and every later read or write + raises `RuntimeError` instead of reopening it; `clear()` replaces the + archive and makes the store usable again. compression : int, optional Compression method to use when writing to the archive. allowZip64 : bool, optional @@ -181,7 +182,7 @@ def _sync_open(self) -> None: # makes zipfile start a new archive and drop the earlier entries raise RuntimeError( f"closing the archive of {self!r} failed, so it may be incomplete; " - "the store will not reopen it" + "the store will not reopen it, but clear() replaces it" ) if ( self.path is None @@ -236,8 +237,11 @@ def __getstate__(self) -> dict[str, Any]: "cannot pickle a ZipStore backed by a file-like object; " "construct the store from a path instead" ) - # We need a copy to not modify the state of the original store - state = self.__dict__.copy() + # We need a copy to not modify the state of the original store. The + # lock keeps it from catching _sync_open between opening with "w" and + # switching to "a", which would make unpickling truncate the archive. + with self._lock: + state = self.__dict__.copy() for attr in ["_zf", "_lock"]: state.pop(attr, None) return state @@ -272,11 +276,17 @@ async def clear(self) -> None: raise NotImplementedError( "clear() is not supported for a ZipStore backed by a file-like object" ) - # opening first keeps mode "x" from truncating a file it may not claim - self._zipfile() - self.close() - # "w" truncates; if the open fails the store stays closed, and the - # next use tries the truncating open again + if self._close_failed: + # replacing the file cannot drop entries, so clear() is the one + # way to recover a store whose close() failed + self._close_failed = False + else: + # opening first keeps mode "x" from deleting a file it may not claim + self._zipfile() + self.close() + os.remove(self.path) + # if this open fails the store stays closed, and the next use + # creates the archive again self._zmode = "w" self._sync_open() diff --git a/tests/test_store/test_zip.py b/tests/test_store/test_zip.py index 068899caad..f05797eba6 100644 --- a/tests/test_store/test_zip.py +++ b/tests/test_store/test_zip.py @@ -422,6 +422,28 @@ def disk_full() -> None: with pytest.raises(RuntimeError, match="closing the archive"): await store.set("bar", cpu.Buffer.from_bytes(b"2")) + # clear() replaces the archive, so it cannot drop entries, and it + # makes the store usable again + await store.clear() + await store.set("baz", cpu.Buffer.from_bytes(b"3")) + store.close() + with zipfile.ZipFile(store.path) as zf: # type: ignore[arg-type] + assert zf.namelist() == ["baz"] + + async def test_unpickle_state_from_older_release(self, tmp_path: Path) -> None: + # a pickle made before _was_opened and _close_failed existed still + # reopens the archive it names + path = tmp_path / "data.zip" + with zipfile.ZipFile(path, mode="w") as zf: + zf.writestr("foo", b"1") + state = ZipStore(path, mode="r").__getstate__() + del state["_was_opened"], state["_close_failed"] + + store = ZipStore.__new__(ZipStore) + store.__setstate__(state) + assert [k async for k in store.list()] == ["foo"] + store.close() + async def test_list_does_not_hold_lock_while_iterating(self, tmp_path: Path) -> None: # another thread can use the store while a caller is partway # through iterating list() From 9cb2c534bb49c908a36a94f085c217f47754bbfc Mon Sep 17 00:00:00 2001 From: Davis Vann Bennett Date: Tue, 29 Sep 2026 18:43:46 +0200 Subject: [PATCH 18/23] 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 --- tests/test_store/test_zip.py | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/tests/test_store/test_zip.py b/tests/test_store/test_zip.py index f05797eba6..8621782ce3 100644 --- a/tests/test_store/test_zip.py +++ b/tests/test_store/test_zip.py @@ -529,8 +529,12 @@ async def test_write_only_reuse_after_close_raises(self, tmp_path: Path) -> None store = ZipStore(f, mode="w", read_only=False) await store.set("foo", cpu.Buffer.from_bytes(b"1")) store.close() - with pytest.raises(io.UnsupportedOperation, match="readable and seekable"): - await store.set("bar", cpu.Buffer.from_bytes(b"2")) + try: + with pytest.raises(io.UnsupportedOperation, match="readable and seekable"): + await store.set("bar", cpu.Buffer.from_bytes(b"2")) + finally: + # close before the file does, even if the write went through + store.close() with zipfile.ZipFile(path) as zf: assert zf.namelist() == ["foo"] @@ -543,8 +547,11 @@ async def test_unseekable_reuse_after_close_raises(self) -> None: store = ZipStore(pipe, mode="w", read_only=False) # type: ignore[arg-type] await store.set("foo", cpu.Buffer.from_bytes(b"1")) store.close() - with pytest.raises(io.UnsupportedOperation, match="readable and seekable"): - await store.set("bar", cpu.Buffer.from_bytes(b"2")) + try: + with pytest.raises(io.UnsupportedOperation, match="readable and seekable"): + await store.set("bar", cpu.Buffer.from_bytes(b"2")) + finally: + store.close() async def test_clear_unsupported(self, zip_bytes: bytes) -> None: # clear() requires a filesystem location, so it raises a clear error From a93ee73006baf89c36c4c03956ac19d78ab0ddb0 Mon Sep 17 00:00:00 2001 From: Davis Vann Bennett Date: Tue, 29 Sep 2026 19:02:48 +0200 Subject: [PATCH 19/23] 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 --- changes/4450.bugfix.md | 2 +- src/zarr/storage/_zip.py | 40 +++++++++++++------ tests/test_store/test_zip.py | 74 +++++++++++++++++++++++++++++++++++- 3 files changed, 103 insertions(+), 13 deletions(-) diff --git a/changes/4450.bugfix.md b/changes/4450.bugfix.md index a643bd93f1..93fb447cc7 100644 --- a/changes/4450.bugfix.md +++ b/changes/4450.bugfix.md @@ -1 +1 @@ -`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 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 read or write now raises `RuntimeError`, instead of reopening an archive that may lack its central directory and silently starting a new one; `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. 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. +`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 read or write 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 writes to the file, which could leave an empty archive when the copy was garbage-collected after the original closed. A copy that is used while another store still has the archive open for writing raises `zipfile.BadZipFile` instead of starting a new archive. 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. diff --git a/src/zarr/storage/_zip.py b/src/zarr/storage/_zip.py index 27d4849357..6d97ceae6e 100644 --- a/src/zarr/storage/_zip.py +++ b/src/zarr/storage/_zip.py @@ -93,10 +93,12 @@ class ZipStore(Store): file, 'a' to append to an existing file, or 'x' to exclusively create and write a new file. 'w' and 'x' apply to the first open only; the store reopens its archive with 'a' after `close()`, `move()`, or - unpickling, so the entries it already wrote are kept. If `close()` - raises, the archive may be incomplete, and every later read or write - raises `RuntimeError` instead of reopening it; `clear()` replaces the - archive and makes the store usable again. + unpickling, so the entries it already wrote are kept. An unpickled + store opens its archive on first use. If `close()` raises, the + archive may be incomplete, and every later read or write raises + `RuntimeError` instead of reopening it; for a writable store backed + by a path, `clear()` replaces the archive and makes the store usable + again. compression : int, optional Compression method to use when writing to the archive. allowZip64 : bool, optional @@ -182,7 +184,21 @@ def _sync_open(self) -> None: # makes zipfile start a new archive and drop the earlier entries raise RuntimeError( f"closing the archive of {self!r} failed, so it may be incomplete; " - "the store will not reopen it, but clear() replaces it" + "the store will not reopen it" + ) + if ( + self.path is not None + and self._was_opened + and self._zmode == "a" + and self.path.exists() + and not zipfile.is_zipfile(self.path) + ): + # the file lacks a central directory, e.g. because another copy of + # this store still has it open for writing; appending would start + # a new archive and drop the entries already written + raise zipfile.BadZipFile( + f"{self!r} cannot reopen {self.path}: the file has no zip central " + "directory, so another store may still have it open for writing" ) if ( self.path is None @@ -249,9 +265,12 @@ def __getstate__(self) -> dict[str, Any]: def __setstate__(self, state: dict[str, Any]) -> None: self.__dict__ = state self.__dict__.setdefault("_close_failed", False) + self.__dict__.setdefault("_was_opened", False) self._lock = threading.RLock() + # open on first use: a copy that is never used, like those dask makes + # while building a graph, must not open the file, because zipfile + # writes a central directory when it closes an archive it could not read self._is_open = False - self._zipfile() def close(self) -> None: # docstring inherited @@ -276,15 +295,14 @@ async def clear(self) -> None: raise NotImplementedError( "clear() is not supported for a ZipStore backed by a file-like object" ) - if self._close_failed: - # replacing the file cannot drop entries, so clear() is the one - # way to recover a store whose close() failed - self._close_failed = False - else: + if not self._close_failed: # opening first keeps mode "x" from deleting a file it may not claim self._zipfile() self.close() os.remove(self.path) + # replacing the file cannot drop entries, so clear() is the one way + # to recover a store whose close() failed + self._close_failed = False # if this open fails the store stays closed, and the next use # creates the archive again self._zmode = "w" diff --git a/tests/test_store/test_zip.py b/tests/test_store/test_zip.py index 8621782ce3..0c6b01c3ae 100644 --- a/tests/test_store/test_zip.py +++ b/tests/test_store/test_zip.py @@ -1,5 +1,6 @@ from __future__ import annotations +import gc import io import os import pickle @@ -283,7 +284,11 @@ async def test_reopen_keeps_entries( store.close() else: store.close() - pickle.loads(pickle.dumps(store)).close() + copy = pickle.loads(pickle.dumps(store)) + value = await copy.get("foo", default_buffer_prototype()) + assert value is not None + assert value.to_bytes() == b"bar" + copy.close() with zipfile.ZipFile(store.path) as zf: # type: ignore[arg-type] assert zf.namelist() == ["foo"] @@ -430,6 +435,73 @@ def disk_full() -> None: with zipfile.ZipFile(store.path) as zf: # type: ignore[arg-type] assert zf.namelist() == ["baz"] + async def test_unused_copy_of_open_writer_does_not_write(self, tmp_path: Path) -> None: + # a copy of a store that is still writing, like those dask makes while + # building a graph, leaves the file alone if it is never used, even + # when it is collected after the original closes + store = ZipStore(tmp_path / "data.zip", mode="w") + await store.set("foo", cpu.Buffer.from_bytes(b"1")) + copy = pickle.loads(pickle.dumps(store)) + await store.set("bar", cpu.Buffer.from_bytes(b"2")) + store.close() + del copy + gc.collect() + + with zipfile.ZipFile(store.path) as zf: # type: ignore[arg-type] + assert zf.namelist() == ["foo", "bar"] + assert zf.testzip() is None + + async def test_used_copy_of_open_writer_raises(self, tmp_path: Path) -> None: + # the file has no central directory until the original closes, so a + # copy that uses it refuses to reopen it instead of starting a new + # archive after the entries already written + store = ZipStore(tmp_path / "data.zip", mode="w") + await store.set("foo", cpu.Buffer.from_bytes(b"1")) + copy = pickle.loads(pickle.dumps(store)) + with pytest.raises(zipfile.BadZipFile, match="no zip central directory"): + await copy.get("foo", default_buffer_prototype()) + store.close() + + with zipfile.ZipFile(store.path) as zf: # type: ignore[arg-type] + assert zf.namelist() == ["foo"] + + async def test_failed_clear_keeps_failed_close_guard( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + # clear() lifts the failed-close guard only once the old file is gone + store = ZipStore(tmp_path / "data.zip", mode="w") + await store.set("foo", cpu.Buffer.from_bytes(b"1")) + + def disk_full() -> None: + raise OSError("disk full") + + def locked(path: Any) -> None: + raise PermissionError("locked") + + monkeypatch.setattr(store._zf, "_write_end_record", disk_full) + with pytest.raises(OSError, match="disk full"): + store.close() + with monkeypatch.context() as m: + m.setattr(os, "remove", locked) + with pytest.raises(PermissionError, match="locked"): + await store.clear() + with pytest.raises(RuntimeError, match="closing the archive"): + await store.set("bar", cpu.Buffer.from_bytes(b"2")) + + async def test_clear_read_mode_writable_store(self, tmp_path: Path) -> None: + # a store opened with mode "r" but read_only=False creates the new + # archive with "w", since "r" cannot open the file clear() removed + path = tmp_path / "data.zip" + with zipfile.ZipFile(path, mode="w") as zf: + zf.writestr("foo", b"1") + + store = ZipStore(path, mode="r", read_only=False) + await store.clear() + await store.set("bar", cpu.Buffer.from_bytes(b"2")) + store.close() + with zipfile.ZipFile(path) as zf: + assert zf.namelist() == ["bar"] + async def test_unpickle_state_from_older_release(self, tmp_path: Path) -> None: # a pickle made before _was_opened and _close_failed existed still # reopens the archive it names From badf49d26319032fd04efb28630021bd63181385 Mon Sep 17 00:00:00 2001 From: Davis Vann Bennett Date: Tue, 29 Sep 2026 19:03:04 +0200 Subject: [PATCH 20/23] docs: describe the unpickling fix from main's side in the ZipStore fragment Assisted-by: ClaudeCode:claude-opus-5-5 --- changes/4450.bugfix.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changes/4450.bugfix.md b/changes/4450.bugfix.md index 93fb447cc7..bbf3da9d8e 100644 --- a/changes/4450.bugfix.md +++ b/changes/4450.bugfix.md @@ -1 +1 @@ -`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 read or write 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 writes to the file, which could leave an empty archive when the copy was garbage-collected after the original closed. A copy that is used while another store still has the archive open for writing raises `zipfile.BadZipFile` instead of starting a new archive. 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. +`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 read or write 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 copy that is used while another store still has the archive open for writing raises `zipfile.BadZipFile` instead of starting a new archive. 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. From 19f118d3270c81c1ed58b823d7717ce3ebd44712 Mon Sep 17 00:00:00 2001 From: Davis Vann Bennett Date: Tue, 29 Sep 2026 19:19:35 +0200 Subject: [PATCH 21/23] 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 --- changes/4450.bugfix.md | 2 +- src/zarr/storage/_zip.py | 12 ++++--- tests/test_store/test_zip.py | 62 ++++++++++++++++++++++++++++++++++-- 3 files changed, 67 insertions(+), 9 deletions(-) diff --git a/changes/4450.bugfix.md b/changes/4450.bugfix.md index bbf3da9d8e..159f6f0e1b 100644 --- a/changes/4450.bugfix.md +++ b/changes/4450.bugfix.md @@ -1 +1 @@ -`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 read or write 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 copy that is used while another store still has the archive open for writing raises `zipfile.BadZipFile` instead of starting a new archive. 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. +`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 read or write 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. diff --git a/src/zarr/storage/_zip.py b/src/zarr/storage/_zip.py index 6d97ceae6e..24ea6fde59 100644 --- a/src/zarr/storage/_zip.py +++ b/src/zarr/storage/_zip.py @@ -197,8 +197,9 @@ def _sync_open(self) -> None: # this store still has it open for writing; appending would start # a new archive and drop the entries already written raise zipfile.BadZipFile( - f"{self!r} cannot reopen {self.path}: the file has no zip central " - "directory, so another store may still have it open for writing" + f"{self!r} cannot reopen {self.path}: it is not a zip archive. It " + "may be incomplete, for example because another store still has it " + "open for writing." ) if ( self.path is None @@ -295,11 +296,12 @@ async def clear(self) -> None: raise NotImplementedError( "clear() is not supported for a ZipStore backed by a file-like object" ) - if not self._close_failed: + if not self._was_opened: # opening first keeps mode "x" from deleting a file it may not claim self._zipfile() - self.close() - os.remove(self.path) + self.close() + # the file may be damaged or gone; clear() replaces it either way + self.path.unlink(missing_ok=True) # replacing the file cannot drop entries, so clear() is the one way # to recover a store whose close() failed self._close_failed = False diff --git a/tests/test_store/test_zip.py b/tests/test_store/test_zip.py index 0c6b01c3ae..0492f90906 100644 --- a/tests/test_store/test_zip.py +++ b/tests/test_store/test_zip.py @@ -3,6 +3,7 @@ import gc import io import os +import pathlib import pickle import shutil import tempfile @@ -458,7 +459,7 @@ async def test_used_copy_of_open_writer_raises(self, tmp_path: Path) -> None: store = ZipStore(tmp_path / "data.zip", mode="w") await store.set("foo", cpu.Buffer.from_bytes(b"1")) copy = pickle.loads(pickle.dumps(store)) - with pytest.raises(zipfile.BadZipFile, match="no zip central directory"): + with pytest.raises(zipfile.BadZipFile, match="not a zip archive"): await copy.get("foo", default_buffer_prototype()) store.close() @@ -475,19 +476,74 @@ async def test_failed_clear_keeps_failed_close_guard( def disk_full() -> None: raise OSError("disk full") - def locked(path: Any) -> None: + def locked(path: Any, missing_ok: bool = False) -> None: raise PermissionError("locked") monkeypatch.setattr(store._zf, "_write_end_record", disk_full) with pytest.raises(OSError, match="disk full"): store.close() with monkeypatch.context() as m: - m.setattr(os, "remove", locked) + m.setattr(pathlib.Path, "unlink", locked) with pytest.raises(PermissionError, match="locked"): await store.clear() with pytest.raises(RuntimeError, match="closing the archive"): await store.set("bar", cpu.Buffer.from_bytes(b"2")) + @pytest.mark.parametrize("damage", ["garbage", "missing"]) + @pytest.mark.parametrize("close", ["closed", "close_failed"]) + async def test_clear_replaces_damaged_file( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch, damage: str, close: str + ) -> None: + # clear() replaces the archive even when the file it wrote is no longer + # a zip or is gone, including after a failed close() + path = tmp_path / "data.zip" + store = ZipStore(path, mode="w") + await store.set("foo", cpu.Buffer.from_bytes(b"1")) + if close == "closed": + store.close() + else: + + def disk_full() -> None: + raise OSError("disk full") + + monkeypatch.setattr(store._zf, "_write_end_record", disk_full) + with pytest.raises(OSError, match="disk full"): + store.close() + if damage == "garbage": + path.write_bytes(b"not a zip") + else: + path.unlink() + + await store.clear() + await store.set("bar", cpu.Buffer.from_bytes(b"2")) + store.close() + with zipfile.ZipFile(path) as zf: + assert zf.namelist() == ["bar"] + + async def test_reopen_recreates_deleted_file(self, tmp_path: Path) -> None: + # a file removed between uses is created again, not refused + path = tmp_path / "data.zip" + store = ZipStore(path, mode="w") + await store.set("foo", cpu.Buffer.from_bytes(b"1")) + store.close() + path.unlink() + + await store.set("bar", cpu.Buffer.from_bytes(b"2")) + store.close() + with zipfile.ZipFile(path) as zf: + assert zf.namelist() == ["bar"] + + async def test_first_open_append_on_empty_file(self, tmp_path: Path) -> None: + # mode "a" on an empty file, e.g. from tempfile.mkstemp, starts an + # archive in it; the reopen guard applies only to archives this store wrote + path = tmp_path / "data.zip" + path.touch() + store = ZipStore(path, mode="a") + await store.set("foo", cpu.Buffer.from_bytes(b"1")) + store.close() + with zipfile.ZipFile(path) as zf: + assert zf.namelist() == ["foo"] + async def test_clear_read_mode_writable_store(self, tmp_path: Path) -> None: # a store opened with mode "r" but read_only=False creates the new # archive with "w", since "r" cannot open the file clear() removed From a0d2b423ac066253cc00e3f9dc4d554d615f1667 Mon Sep 17 00:00:00 2001 From: Davis Vann Bennett Date: Tue, 29 Sep 2026 19:36:43 +0200 Subject: [PATCH 22/23] 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 --- changes/4450.bugfix.md | 2 +- src/zarr/storage/_zip.py | 20 ++++++--- tests/test_store/test_zip.py | 87 +++++++++++++++++++++++------------- 3 files changed, 72 insertions(+), 37 deletions(-) diff --git a/changes/4450.bugfix.md b/changes/4450.bugfix.md index 159f6f0e1b..8760b7fb4e 100644 --- a/changes/4450.bugfix.md +++ b/changes/4450.bugfix.md @@ -1 +1 @@ -`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 read or write 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. +`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. diff --git a/src/zarr/storage/_zip.py b/src/zarr/storage/_zip.py index 24ea6fde59..921ff50a23 100644 --- a/src/zarr/storage/_zip.py +++ b/src/zarr/storage/_zip.py @@ -72,6 +72,17 @@ def readinto(self, b: Any) -> int: return n +def _is_unfinished_archive(path: Path) -> bool: + """Whether the file at `path` exists but is not a zip archive.""" + try: + with path.open("rb") as f: + # is_zipfile swallows OSError, so open the file here to let a + # directory or a permission error surface as itself + return not zipfile.is_zipfile(f) + except FileNotFoundError: + return False + + class ZipStore(Store): """ Store using a ZIP file. @@ -95,8 +106,8 @@ class ZipStore(Store): store reopens its archive with 'a' after `close()`, `move()`, or unpickling, so the entries it already wrote are kept. An unpickled store opens its archive on first use. If `close()` raises, the - archive may be incomplete, and every later read or write raises - `RuntimeError` instead of reopening it; for a writable store backed + archive may be incomplete, and every later use that would reopen it + raises `RuntimeError` instead; for a writable store backed by a path, `clear()` replaces the archive and makes the store usable again. compression : int, optional @@ -190,8 +201,7 @@ def _sync_open(self) -> None: self.path is not None and self._was_opened and self._zmode == "a" - and self.path.exists() - and not zipfile.is_zipfile(self.path) + and _is_unfinished_archive(self.path) ): # the file lacks a central directory, e.g. because another copy of # this store still has it open for writing; appending would start @@ -296,7 +306,7 @@ async def clear(self) -> None: raise NotImplementedError( "clear() is not supported for a ZipStore backed by a file-like object" ) - if not self._was_opened: + if not self._was_opened and self._zmode == "x": # opening first keeps mode "x" from deleting a file it may not claim self._zipfile() self.close() diff --git a/tests/test_store/test_zip.py b/tests/test_store/test_zip.py index 0492f90906..7c2715ad4d 100644 --- a/tests/test_store/test_zip.py +++ b/tests/test_store/test_zip.py @@ -9,7 +9,7 @@ import tempfile import threading import zipfile -from concurrent.futures import ThreadPoolExecutor, wait +from concurrent.futures import Future, ThreadPoolExecutor, wait from typing import TYPE_CHECKING, cast import numpy as np @@ -25,7 +25,6 @@ import zarr from zarr import create_array -from zarr.abc.store import Store from zarr.core.buffer import Buffer, cpu, default_buffer_prototype from zarr.core.sync import sync from zarr.storage import ZipStore @@ -225,6 +224,12 @@ async def test_move(self, tmp_path: Path) -> None: ["foo", "bar"], id="set_if_not_exists", ), + pytest.param( + lambda s: s.set_if_not_exists("foo", cpu.Buffer.from_bytes(b"x")), + None, + ["foo"], + id="set_if_not_exists_existing", + ), pytest.param(lambda s: s.clear(), None, [], id="clear"), pytest.param(lambda s: s.delete("bar"), None, ["foo"], id="delete"), pytest.param(lambda s: s.delete_dir("bar"), None, ["foo"], id="delete_dir"), @@ -296,27 +301,23 @@ async def test_reopen_keeps_entries( async def test_close_blocks_concurrent_reopen( self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: - # a thread that uses the store while close() runs must wait until the - # archive is closed, or it reopens a file with no central directory + # a thread that uses the store while close() is writing the central + # directory must wait until the archive is closed store = ZipStore(tmp_path / "data.zip", mode="w") await store.set("foo", cpu.Buffer.from_bytes(b"1")) + write_end_record = store._zf._write_end_record # type: ignore[attr-defined] + writes: list[Future[None]] = [] - writer = threading.Thread( - target=lambda: sync(store.set("bar", cpu.Buffer.from_bytes(b"2"))) - ) - mark_closed = Store.close + with ThreadPoolExecutor(max_workers=1) as pool: - def mark_closed_then_write(self: Store) -> None: - # the store now reports closed; start a write before close() returns - mark_closed(self) - writer.start() - writer.join(timeout=0.2) + def write_during_close() -> None: + writes.append(pool.submit(sync, store.set("bar", cpu.Buffer.from_bytes(b"2")))) + wait(writes, timeout=0.2) + write_end_record() - monkeypatch.setattr(Store, "close", mark_closed_then_write) - store.close() - monkeypatch.undo() - writer.join(timeout=5) - assert not writer.is_alive() + monkeypatch.setattr(store._zf, "_write_end_record", write_during_close) + store.close() + writes[0].result(timeout=5) store.close() with zipfile.ZipFile(store.path) as zf: # type: ignore[arg-type] @@ -409,8 +410,9 @@ async def test_first_use_waits_for_lock(self, tmp_path: Path, use: Any) -> None: first_use.result(timeout=5) store.close() + @pytest.mark.parametrize("error", [OSError, KeyboardInterrupt]) async def test_failed_close_blocks_reopen( - self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch, error: type[BaseException] ) -> None: # if the central directory could not be written, reopening in append # mode would start a new archive and drop the entries, so later use @@ -419,10 +421,10 @@ async def test_failed_close_blocks_reopen( await store.set("foo", cpu.Buffer.from_bytes(b"1")) def disk_full() -> None: - raise OSError("disk full") + raise error("disk full") monkeypatch.setattr(store._zf, "_write_end_record", disk_full) - with pytest.raises(OSError, match="disk full"): + with pytest.raises(error, match="disk full"): store.close() assert not store._is_open with pytest.raises(RuntimeError, match="closing the archive"): @@ -544,12 +546,15 @@ async def test_first_open_append_on_empty_file(self, tmp_path: Path) -> None: with zipfile.ZipFile(path) as zf: assert zf.namelist() == ["foo"] - async def test_clear_read_mode_writable_store(self, tmp_path: Path) -> None: + @pytest.mark.parametrize("file", ["archive", "missing"]) + async def test_clear_read_mode_writable_store(self, tmp_path: Path, file: str) -> None: # a store opened with mode "r" but read_only=False creates the new - # archive with "w", since "r" cannot open the file clear() removed + # archive with "w", since "r" cannot open the file clear() removed, + # and it does not need to open the old file first path = tmp_path / "data.zip" - with zipfile.ZipFile(path, mode="w") as zf: - zf.writestr("foo", b"1") + if file == "archive": + with zipfile.ZipFile(path, mode="w") as zf: + zf.writestr("foo", b"1") store = ZipStore(path, mode="r", read_only=False) await store.clear() @@ -558,17 +563,37 @@ async def test_clear_read_mode_writable_store(self, tmp_path: Path) -> None: with zipfile.ZipFile(path) as zf: assert zf.namelist() == ["bar"] - async def test_unpickle_state_from_older_release(self, tmp_path: Path) -> None: - # a pickle made before _was_opened and _close_failed existed still - # reopens the archive it names + async def test_reopen_reports_directory_at_path(self, tmp_path: Path) -> None: + # a directory where the archive was is reported as an OS error, not as + # an unfinished archive path = tmp_path / "data.zip" - with zipfile.ZipFile(path, mode="w") as zf: - zf.writestr("foo", b"1") - state = ZipStore(path, mode="r").__getstate__() + store = ZipStore(path, mode="w") + await store.set("foo", cpu.Buffer.from_bytes(b"1")) + store.close() + path.unlink() + path.mkdir() + + with pytest.raises(OSError): + await store.get("foo", default_buffer_prototype()) + + @pytest.mark.parametrize("mode", ["r", "a"]) + async def test_unpickle_state_from_older_release(self, tmp_path: Path, mode: str) -> None: + # a pickle made before _was_opened and _close_failed existed opens the + # file as a first open would; for "a" on an empty file that starts an + # archive rather than tripping the reopen guard + path = tmp_path / "data.zip" + if mode == "r": + with zipfile.ZipFile(path, mode="w") as zf: + zf.writestr("foo", b"1") + else: + path.touch() + state = ZipStore(path, mode=mode).__getstate__() # type: ignore[arg-type] del state["_was_opened"], state["_close_failed"] store = ZipStore.__new__(ZipStore) store.__setstate__(state) + if mode == "a": + await store.set("foo", cpu.Buffer.from_bytes(b"1")) assert [k async for k in store.list()] == ["foo"] store.close() From 0d25f133b784266b378a9b14ab1754da6679ae06 Mon Sep 17 00:00:00 2001 From: Davis Vann Bennett Date: Tue, 29 Sep 2026 19:43:33 +0200 Subject: [PATCH 23/23] 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 --- src/zarr/storage/_zip.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/zarr/storage/_zip.py b/src/zarr/storage/_zip.py index 921ff50a23..e29d2ce496 100644 --- a/src/zarr/storage/_zip.py +++ b/src/zarr/storage/_zip.py @@ -306,8 +306,9 @@ async def clear(self) -> None: raise NotImplementedError( "clear() is not supported for a ZipStore backed by a file-like object" ) - if not self._was_opened and self._zmode == "x": - # opening first keeps mode "x" from deleting a file it may not claim + if self._zmode == "x": + # "x" lasts only until the first open; opening now keeps it from + # deleting a file this store never claimed self._zipfile() self.close() # the file may be damaged or gone; clear() replaces it either way