fix: implement soft-delete for ZipStore (issue #828)

src/zarr/storage/_zip.py

@@ -4,6 +4,7 @@
 import shutil
 import threading
 import time
+import warnings
 import zipfile
 from pathlib import Path
 from typing import TYPE_CHECKING, Any, Literal
@@ -22,6 +23,26 @@
 
 ZipStoreAccessModeLiteral = Literal["r", "w", "a"]
 
+# Marker used to flag a zip entry as soft-deleted.
+#
+# IMPORTANT: this is stored in the ZipInfo *comment* field (per-entry
+# central-directory metadata), not in the entry's actual data content.
+# An earlier version of this fix wrote a sentinel *value* as the entry's
+# data (first b"", later a long "unique" byte string). Both are unsafe:
+# Zarr payload bytes are arbitrary, so a legitimate value can always
+# equal whatever sentinel is chosen -- Hypothesis's stateful property
+# tests proved this by finding a falsifying example that set a key's
+# value to the literal sentinel bytes, which made list()/exists()/get()
+# incorrectly report a live key as deleted.
+#
+# Using ZipInfo.comment instead sidesteps the problem entirely: it is
+# metadata the store never exposes through set()/get(), so it cannot
+# collide with any byte string a caller stores as data, no matter what
+# that data is. It's also persisted to the zip's central directory, so
+# soft-deletes made in one process are still visible after the archive
+# is closed and reopened elsewhere.
+_SOFT_DELETE_MARKER = b"__zarr_zipstore_soft_delete__"
+
 
 class ZipStore(Store):
     """
@@ -52,10 +73,22 @@ class ZipStore(Store):
     path
     compression
     allowZip64
+
+    Notes
+    -----
+    Deletion is implemented as a soft-delete: since the ZIP format does not
+    support removing entries in place, `delete()` appends a new, empty entry
+    under the same name and flags it via the ZipInfo *comment* field (not its
+    data). All read, exists, and list operations check that flag and treat
+    matching entries as absent. Because deletions are recorded in the zip's
+    central directory rather than kept only in memory, they remain in effect
+    even if the store is closed and the same file reopened later. The
+    original, physical bytes for a soft-deleted entry stay on disk (the
+    archive is never rewritten), but are unreachable through the store API.
     """
 
     supports_writes: bool = True
-    supports_deletes: bool = False
+    supports_deletes: bool = True  # soft-delete via a flagged empty entry
     supports_listing: bool = True
 
     path: Path
@@ -145,6 +178,14 @@ def __repr__(self) -> str:
     def __eq__(self, other: object) -> bool:
         return isinstance(other, type(self)) and self.path == other.path
 
+    def _is_soft_deleted(self, key: str) -> bool:
+        """Return True if `key`'s most recent entry is a soft-delete marker."""
+        try:
+            info = self._zf.getinfo(key)
+        except KeyError:
+            return False
+        return info.comment == _SOFT_DELETE_MARKER
+
     def _get(
         self,
         key: str,
@@ -154,24 +195,25 @@ def _get(
         if not self._is_open:
             self._sync_open()
         # docstring inherited
+        if self._is_soft_deleted(key):
+            return None
         try:
-            with self._zf.open(key) as f:  # will raise KeyError
-                if byte_range is None:
-                    return prototype.buffer.from_bytes(f.read())
-                elif isinstance(byte_range, RangeByteRequest):
-                    f.seek(byte_range.start)
-                    return prototype.buffer.from_bytes(f.read(byte_range.end - f.tell()))
-                size = f.seek(0, os.SEEK_END)
-                if isinstance(byte_range, OffsetByteRequest):
-                    f.seek(byte_range.offset)
-                elif isinstance(byte_range, SuffixByteRequest):
-                    f.seek(max(0, size - byte_range.suffix))
-                else:
-                    raise TypeError(f"Unexpected byte_range, got {byte_range}.")
-                return prototype.buffer.from_bytes(f.read())
+            with self._zf.open(key) as f:
+                data = f.read()
         except KeyError:
             return None
 
+        if byte_range is None:
+            return prototype.buffer.from_bytes(data)
+        elif isinstance(byte_range, RangeByteRequest):
+            return prototype.buffer.from_bytes(data[byte_range.start : byte_range.end])
+        elif isinstance(byte_range, OffsetByteRequest):
+            return prototype.buffer.from_bytes(data[byte_range.offset :])
+        elif isinstance(byte_range, SuffixByteRequest):
+            return prototype.buffer.from_bytes(data[max(0, len(data) - byte_range.suffix) :])
+        else:
+            raise TypeError(f"Unexpected byte_range, got {byte_range}.")
+
     async def get(
         self,
         key: str,
@@ -207,6 +249,8 @@ 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--
+        # keyinfo.comment defaults to b"", so a fresh write is never
+        # mistaken for a soft-delete marker regardless of `value`.
         self._zf.writestr(keyinfo, value.to_bytes())
 
     async def set(self, key: str, value: Buffer) -> None:
@@ -229,41 +273,60 @@ async def set_if_not_exists(self, key: str, value: Buffer) -> None:
             if key not in members:
                 self._set(key, value)
 
-    async def delete_dir(self, prefix: str) -> None:
-        # only raise NotImplementedError if any keys are found
+    async def delete(self, key: str) -> None:
+        # docstring inherited
+        # Soft-delete: append an empty entry under this name, flagged via
+        # the ZipInfo comment field (see _SOFT_DELETE_MARKER above). The
+        # comment -- not the data -- is what read/list/exists check, so
+        # this can never be confused with a real value, including a real
+        # value that happens to be empty bytes.
         self._check_writable()
-        if prefix != "" and not prefix.endswith("/"):
-            prefix += "/"
-        async for _ in self.list_prefix(prefix):
-            raise NotImplementedError
+        with self._lock:
+            if key in self._zf.namelist():
+                keyinfo = zipfile.ZipInfo(filename=key, date_time=time.localtime(time.time())[:6])
+                keyinfo.compress_type = self.compression
+                keyinfo.external_attr = 0o644 << 16  # ?rw-r--r--
+                keyinfo.comment = _SOFT_DELETE_MARKER
+                # zipfile.writestr() warns "Duplicate name" whenever a name
+                # is written more than once, which is exactly what
+                # soft-delete does on purpose. That warning is expected and
+                # harmless here (unlike a real overwrite via set(), which
+                # intentionally keeps warning -- see test_api_integration),
+                # so it is suppressed rather than allowed to propagate under
+                # this project's filterwarnings = "error" pytest config.
+                with warnings.catch_warnings():
+                    warnings.simplefilter("ignore", UserWarning)
+                    self._zf.writestr(keyinfo, b"")
 
-    async def delete(self, key: str) -> None:
+    async def delete_dir(self, prefix: str) -> None:
         # docstring inherited
-        # we choose to only raise NotImplementedError here if the key exists
-        # this allows the array/group APIs to avoid the overhead of existence checks
+        # Collect all live keys under the prefix first, then soft-delete each.
         self._check_writable()
-        if await self.exists(key):
-            raise NotImplementedError
+        if prefix != "" and not prefix.endswith("/"):
+            prefix += "/"
+        keys_to_delete = [key async for key in self.list_prefix(prefix)]
+        for key in keys_to_delete:
+            await self.delete(key)
 
     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)
-            except KeyError:
-                return False
-            else:
-                return True
+            return not self._is_soft_deleted(key) and key in self._zf.namelist()
 
     async def list(self) -> AsyncIterator[str]:
         # docstring inherited
         if not self._is_open:
             self._sync_open()
         with self._lock:
+            seen: set[str] = set()
             for key in self._zf.namelist():
-                yield key
+                if key in seen:
+                    continue
+                seen.add(key)
+                if not self._is_soft_deleted(key):
+                    yield key
 
     async def list_prefix(self, prefix: str) -> AsyncIterator[str]:
         # docstring inherited
@@ -276,8 +339,7 @@ async def list_dir(self, prefix: str) -> AsyncIterator[str]:
         if not self._is_open:
             self._sync_open()
         prefix = prefix.rstrip("/")
-
-        keys = self._zf.namelist()
+        keys = [k async for k in self.list()]
         seen = set()
         if prefix == "":
             keys_unique = {k.split("/")[0] for k in keys}
@@ -294,9 +356,7 @@ async def list_dir(self, prefix: str) -> AsyncIterator[str]:
                         yield k
 
     async def move(self, path: Path | str) -> None:
-        """
-        Move the store to another path.
-        """
+        """Move the store to another path."""
         if isinstance(path, str):
             path = Path(path)
         self.close()

tests/test_store/test_stateful.py

@@ -8,7 +8,7 @@
 
 import zarr
 from zarr.abc.store import Store
-from zarr.storage import LocalStore, ZipStore
+from zarr.storage import LocalStore
 from zarr.testing.stateful import ZarrHierarchyStateMachine, ZarrStoreStateMachine
 
 pytestmark = [
@@ -26,26 +26,22 @@ def _enable_rectilinear_chunks() -> Generator[None, None, None]:
 
 
 @pytest.mark.filterwarnings("ignore::zarr.core.dtype.common.UnstableSpecificationWarning")
+@pytest.mark.filterwarnings("ignore:Duplicate name:UserWarning")
 def test_zarr_hierarchy(sync_store: Store) -> None:
     def mk_test_instance_sync() -> ZarrHierarchyStateMachine:
         return ZarrHierarchyStateMachine(sync_store)
 
-    if isinstance(sync_store, ZipStore):
-        pytest.skip(reason="ZipStore does not support delete")
-
+    # ZipStore now supports soft-delete — skip removed
     run_state_machine_as_test(mk_test_instance_sync)  # type: ignore[no-untyped-call]
 
 
+@pytest.mark.filterwarnings("ignore:Duplicate name:UserWarning")
 def test_zarr_store(sync_store: Store) -> None:
     def mk_test_instance_sync() -> ZarrStoreStateMachine:
         return ZarrStoreStateMachine(sync_store)
 
-    if isinstance(sync_store, ZipStore):
-        pytest.skip(reason="ZipStore does not support delete")
+    # ZipStore now supports soft-delete — skip removed
 
     if isinstance(sync_store, LocalStore):
-        # This test uses arbitrary keys, which are passed to `set` and `delete`.
-        # It assumes that `set` and `delete` are the only two operations that modify state.
-        # But LocalStore, directories can hang around even after a key is delete-d.
         pytest.skip(reason="Test isn't suitable for LocalStore.")
     run_state_machine_as_test(mk_test_instance_sync)  # type: ignore[no-untyped-call]