Forbid zero-length chunk references in ChunkManifest

conftest.py

@@ -171,6 +171,8 @@ def _generate_chunk_entries(
     )
 
     if chunk_grid_shape == ():
+        # a scalar array has a single chunk holding a single element, so passing
+        # chunks=() gives it a length of one itemsize rather than zero
         return {"0": entry_generator((0,), (), itemsize)}
 
     all_possible_combos = itertools.product(

docs/about/releases.md

@@ -6,6 +6,19 @@
 
 ### Breaking changes
 
+- Creating a `ChunkManifest` (and so a `ManifestArray`) containing a zero-length chunk reference
+  now raises `ValueError`. A chunk must decode to the full chunk shape, so no valid chunk is ever
+  zero bytes long, and such a reference can only fail at read time - after it has been written to
+  a store. Applies to virtual and inlined references alike, via every constructor
+  (`ChunkManifest`, `ChunkManifest.from_arrays`, `ChunkEntry.with_validation`).
+  A chunk which simply isn't stored - as in a sparse array - should be given the empty path
+  instead, which reads back as the array's `fill_value` and fetches nothing.
+  This catches bugs like [virtual-tiff#108](https://github.com/virtual-zarr/virtual-tiff/issues/108),
+  where sparse GeoTIFF tiles (`offset = 0, byteCount = 0`) became references that committed
+  successfully to Icechunk and only failed on read. Closes
+  [#1088](https://github.com/zarr-developers/VirtualiZarr/issues/1088).
+  By [Tom Nicholas](https://github.com/TomNicholas).
+
 ### Bug fixes
 
 ### Documentation

virtualizarr/manifests/manifest.py

@@ -64,6 +64,8 @@ def with_validation(
 
         # note: we can't just use `__init__` or a dataclass' `__post_init__` because we need `fs_root` to be an optional kwarg
         if inlined_data is not None:
+            if len(inlined_data) == 0:
+                raise ValueError(zero_length_chunk_error_message())
             return ChunkEntry(
                 path=INLINED_CHUNK_PATH,
                 offset=0,
@@ -72,6 +74,8 @@ def with_validation(
             )
         if path != MISSING_CHUNK_PATH:
             path = validate_and_normalize_path_to_uri(path, fs_root=fs_root)
+            if length == 0:
+                raise ValueError(zero_length_chunk_error_message(path=path))
         validate_byte_range(offset=offset, length=length)
         return ChunkEntry(path=path, offset=offset, length=length)
 
@@ -128,6 +132,21 @@ def convert_relative_path_to_absolute(path: PosixPath, fs_root: str) -> str:
     return (_fs_root / path).resolve().as_uri()
 
 
+def zero_length_chunk_error_message(
+    *, path: str | None = None, index: tuple[int, ...] | None = None
+) -> str:
+    """Build the error message raised when a chunk reference has zero length."""
+    subject = "inlined chunk" if path is None else f"chunk reference to {path!r}"
+    at = "" if index is None else f" at index {index}"
+    return (
+        f"Found a zero-length {subject}{at}. A chunk must decode to the full chunk "
+        "shape, so no valid chunk is ever zero bytes long. To record that a chunk is "
+        "not stored at all (as in a sparse array), give it the empty path "
+        f"{MISSING_CHUNK_PATH!r} instead, which reads back as the array's fill_value "
+        "without fetching anything."
+    )
+
+
 def validate_byte_range(*, offset: Any, length: Any) -> None:
     """Raise if byte offset or length has invalid type or value"""
 
@@ -260,6 +279,8 @@ def __init__(
 
             if "data" in entry:
                 # Inlined chunk: store bytes in the sparse dict
+                if len(entry["data"]) == 0:
+                    raise ValueError(zero_length_chunk_error_message(index=split_key))
                 inlined[split_key] = entry["data"]
                 paths[split_key] = INLINED_CHUNK_PATH
                 offsets[split_key] = 0
@@ -348,6 +369,29 @@ def from_arrays(
                 f"Shapes of the arrays must be consistent, but shapes of paths array and lengths array do not match: {paths.shape} vs {lengths.shape}"
             )
 
+        # A zero length is only meaningful for chunks which aren't stored at all.
+        # Comparing the paths array is ~30x more expensive than scanning lengths, so only
+        # do it for manifests which actually contain zero lengths.
+        zero_length = lengths == 0
+        if zero_length.any():
+            invalid = zero_length & (paths != MISSING_CHUNK_PATH)
+            if invalid.any():
+                # argmax gives the first offending chunk without materializing the
+                # position of every other one, which may be most of the manifest
+                first = tuple(
+                    int(i)
+                    for i in np.unravel_index(int(np.argmax(invalid)), invalid.shape)
+                )
+                offending_path = str(paths[first])
+                raise ValueError(
+                    zero_length_chunk_error_message(
+                        path=None
+                        if offending_path == INLINED_CHUNK_PATH
+                        else offending_path,
+                        index=first,
+                    )
+                )
+
         if validate_paths:
             vectorized_validation_fn = np.vectorize(
                 validate_and_normalize_path_to_uri, otypes=[np.dtypes.StringDType()]

virtualizarr/tests/test_manifests/test_manifest.py

@@ -3,7 +3,7 @@
 import numpy as np
 import pytest
 
-from virtualizarr.manifests import ChunkEntry, ChunkManifest
+from virtualizarr.manifests import ChunkEntry, ChunkManifest, ManifestArray
 
 
 class TestPathValidation:
@@ -263,6 +263,76 @@ def test_validate_paths(self):
         )
 
 
+class TestZeroLengthChunks:
+    """Zero-length chunk references are always invalid - a chunk can never decode to
+    the full chunk shape from zero bytes. See VirtualiZarr#1088, virtual-tiff#108."""
+
+    @pytest.mark.parametrize("validate_paths", [True, False])
+    def test_from_arrays_raises(self, validate_paths):
+        """The offending chunk is named, and skipping path validation is no escape."""
+        paths = np.asarray([["/foo.nc", "/foo.nc"]] * 2, dtype=np.dtypes.StringDType)
+        offsets = np.zeros((2, 2), dtype=np.uint64)
+        lengths = np.asarray([[100, 100], [100, 0]], dtype=np.uint64)
+
+        with pytest.raises(
+            ValueError,
+            match=r"zero-length chunk reference to '/foo.nc' at index \(1, 1\)",
+        ):
+            ChunkManifest.from_arrays(
+                paths=paths,
+                offsets=offsets,
+                lengths=lengths,
+                validate_paths=validate_paths,
+            )
+
+    def test_missing_chunks_may_be_zero_length(self):
+        """The empty path is how a not-stored chunk is spelled, so it keeps length 0."""
+        paths = np.asarray(["/foo1.nc", ""], dtype=np.dtypes.StringDType)
+        offsets = np.asarray([100, 0], dtype=np.uint64)
+        lengths = np.asarray([100, 0], dtype=np.uint64)
+
+        manifest = ChunkManifest.from_arrays(
+            paths=paths, offsets=offsets, lengths=lengths, validate_paths=True
+        )
+        assert manifest.dict() == {
+            "0": {"path": "file:///foo1.nc", "offset": 100, "length": 100},
+        }
+
+    def test_dict_constructor_raises(self):
+        with pytest.raises(ValueError, match="zero-length chunk reference"):
+            ChunkManifest(
+                entries={
+                    "0.0": {"path": "s3://bucket/foo.nc", "offset": 0, "length": 0}
+                }
+            )
+
+    def test_chunk_entry_raises(self):
+        with pytest.raises(ValueError, match="zero-length chunk reference"):
+            ChunkEntry.with_validation(path="s3://bucket/foo.nc", offset=0, length=0)
+
+    def test_inlined_raises(self):
+        with pytest.raises(ValueError, match="zero-length inlined chunk"):
+            ChunkEntry.with_validation(path="", offset=0, length=0, inlined_data=b"")
+
+    def test_inlined_dict_constructor_raises(self):
+        with pytest.raises(ValueError, match="zero-length inlined chunk"):
+            ChunkManifest(
+                entries={"0.0": {"path": "", "offset": 0, "length": 0, "data": b""}}
+            )
+
+    def test_manifestarray_raises(self, array_v3_metadata):
+        """The check covers ManifestArray however its manifest is spelled."""
+        metadata = array_v3_metadata(shape=(2,), chunks=(1,))
+        with pytest.raises(ValueError, match="zero-length chunk reference"):
+            ManifestArray(
+                metadata=metadata,
+                chunkmanifest={
+                    "0": {"path": "s3://bucket/foo.nc", "offset": 0, "length": 100},
+                    "1": {"path": "s3://bucket/foo.nc", "offset": 100, "length": 0},
+                },
+            )
+
+
 class TestEquals:
     def test_equals(self):
         manifest1 = ChunkManifest(