From 7154260222243935a5524313206abad762269556 Mon Sep 17 00:00:00 2001 From: Othman El Hammouchi Date: Fri, 21 Nov 2025 00:51:44 +0100 Subject: [PATCH 1/7] 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 2/7] 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 3/7] 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 4/7] 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 5/7] 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 6/7] 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 7/7] 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