From c100b4881168380ea93b0878896c7c0869bbd89f Mon Sep 17 00:00:00 2001 From: Davis Vann Bennett Date: Sat, 3 Oct 2026 17:06:44 +0200 Subject: [PATCH 1/4] refactor(sharding): test shard readers and byte slices with `is None`, not truthiness `_ShardReader` defines `__len__`, so `shard_reader or create_empty(...)` replaced a loaded zero-chunk shard with an empty reader; the outcome was equivalent but the test meant "was a shard loaded". Likewise `get_chunk_slice` returns `tuple[int, int] | None`, and `_load_full_shard_maybe` relied on `Buffer.__len__` to treat a missing and a zero-length value alike; that conflation is now spelled out. No behaviour changes. Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- src/zarr/codecs/sharding.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/zarr/codecs/sharding.py b/src/zarr/codecs/sharding.py index 85d445752d..a127bddb2f 100644 --- a/src/zarr/codecs/sharding.py +++ b/src/zarr/codecs/sharding.py @@ -355,7 +355,7 @@ def create_empty( def __getitem__(self, chunk_coords: tuple[int, ...]) -> Buffer: chunk_byte_slice = self.index.get_chunk_slice(chunk_coords) - if chunk_byte_slice: + if chunk_byte_slice is not None: return self.buf[chunk_byte_slice[0] : chunk_byte_slice[1]] raise KeyError @@ -1403,7 +1403,8 @@ async def _encode_partial_single( prototype=chunk_spec.prototype, chunks_per_shard=chunks_per_shard, ) - shard_reader = shard_reader or _ShardReader.create_empty(chunks_per_shard) + if shard_reader is None: + shard_reader = _ShardReader.create_empty(chunks_per_shard) # Use vectorized lookup for better performance. The lexicographic # coordinate array and keys are cached, so neither is rebuilt on # every write. @@ -1669,7 +1670,7 @@ async def _load_full_shard_maybe( return ( await _ShardReader.from_bytes(shard_bytes, self, chunks_per_shard) - if shard_bytes + if shard_bytes is not None and len(shard_bytes) > 0 else None ) From 69aa38d5c16a56169a591235cd801b2e202d03f5 Mon Sep 17 00:00:00 2001 From: Davis Vann Bennett Date: Sat, 3 Oct 2026 18:18:13 +0200 Subject: [PATCH 2/4] fix(sharding): reject a stored shard too short to hold its index on every path A zero-length shard was read as missing by the async full-shard read and partial write, and failed the index checksum on every other path. The index decoder now checks the index length first, so every read and partial-write path, under either pipeline, raises the same ValueError. zarrs and tensorstore also reject such a shard: https://github.com/zarrs/zarrs/blob/b8ad3ab45082b7d55ac0dd653594953a9d9e6e7d/zarrs/src/array/codec/array_to_bytes/sharding/sharding_codec.rs#L1273 https://github.com/google/tensorstore/blob/f47412a006d7b3da888f3dbb7fc001d1ac6553e1/tensorstore/kvstore/zarr3_sharding_indexed/shard_format.cc#L113 Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- changes/4463.bugfix.md | 1 + src/zarr/codecs/sharding.py | 21 ++++++++++++++- tests/test_codecs/test_sharding.py | 43 +++++++++++++++++++++++++++++- 3 files changed, 63 insertions(+), 2 deletions(-) create mode 100644 changes/4463.bugfix.md diff --git a/changes/4463.bugfix.md b/changes/4463.bugfix.md new file mode 100644 index 0000000000..01e0570bf8 --- /dev/null +++ b/changes/4463.bugfix.md @@ -0,0 +1 @@ +A stored shard too short to hold its shard index, including a zero-length value, now raises a `ValueError` saying the shard is truncated or corrupt, on every read and partial-write path under either codec pipeline. Previously the asynchronous full-shard read and partial write treated a zero-length shard as missing (reading the fill value, or overwriting it), while every other path failed with a checksum mismatch or a decode error. zarrs and tensorstore also reject such a shard. diff --git a/src/zarr/codecs/sharding.py b/src/zarr/codecs/sharding.py index a127bddb2f..cb972118ce 100644 --- a/src/zarr/codecs/sharding.py +++ b/src/zarr/codecs/sharding.py @@ -667,6 +667,7 @@ def _decode_shard_index_sync( self, index_bytes: Buffer, chunks_per_shard: tuple[int, ...] ) -> _ShardIndex: """Decode shard index synchronously using ChunkTransform.""" + self._check_shard_index_size(index_bytes, chunks_per_shard) index_transform = self._get_index_chunk_transform(chunks_per_shard) index_spec = self._get_index_chunk_spec(chunks_per_shard) index_array = index_transform.decode_chunk(index_bytes, index_spec) @@ -1548,6 +1549,7 @@ async def _decode_shard_index( # the synchronous read paths cannot use but this async path still can. if self._index_codecs_sync_capable(): return self._decode_shard_index_sync(index_bytes, chunks_per_shard) + self._check_shard_index_size(index_bytes, chunks_per_shard) index_array = next( iter( await get_pipeline_class() @@ -1582,6 +1584,23 @@ async def _encode_shard_index(self, index: _ShardIndex) -> Buffer: raise RuntimeError("Encoding the shard index produced no bytes.") return index_bytes + def _check_shard_index_size( + self, index_bytes: Buffer, chunks_per_shard: tuple[int, ...] + ) -> None: + """Raise if `index_bytes` is shorter than the encoded shard index. + + A stored shard always holds its index, so a shorter read means the + stored value is truncated (a zero-length value included). Every read + and write path decodes the index through here, so they all reject it + alike. zarrs and tensorstore also reject such a shard. + """ + expected = self._shard_index_size(chunks_per_shard) + if len(index_bytes) < expected: + raise ValueError( + f"The stored shard is too short to hold its index: read {len(index_bytes)} " + f"bytes of index, expected {expected}. The shard is truncated or corrupt." + ) + def _shard_index_size(self, chunks_per_shard: tuple[int, ...]) -> int: return ( get_pipeline_class() @@ -1670,7 +1689,7 @@ async def _load_full_shard_maybe( return ( await _ShardReader.from_bytes(shard_bytes, self, chunks_per_shard) - if shard_bytes is not None and len(shard_bytes) > 0 + if shard_bytes is not None else None ) diff --git a/tests/test_codecs/test_sharding.py b/tests/test_codecs/test_sharding.py index 188d6d85c1..3cd8044952 100644 --- a/tests/test_codecs/test_sharding.py +++ b/tests/test_codecs/test_sharding.py @@ -2,7 +2,7 @@ import math import pickle import warnings -from typing import Any, cast, get_args +from typing import TYPE_CHECKING, Any, cast, get_args from unittest.mock import AsyncMock import numpy as np @@ -40,6 +40,9 @@ from ..conftest import ArrayRequest from .test_codecs import _AsyncArrayProxy, order_from_dim +if TYPE_CHECKING: + from collections.abc import Callable + def _reads_are_sync(store_mock: AsyncMock) -> bool: """True when the partial-shard read for this store+pipeline goes through the @@ -1395,3 +1398,41 @@ def test_sharding_orthogonal_set_multiple_array_dims( expected[ix] = value.reshape(expected[ix].shape) assert np.array_equal(a[:], expected) assert np.array_equal(a.oindex[selection], value) + + +@pytest.mark.parametrize( + "pipeline_path", + [ + "zarr.core.codec_pipeline.FusedCodecPipeline", + "zarr.core.codec_pipeline.BatchedCodecPipeline", + ], +) +@pytest.mark.parametrize("store", ["memory", "local"], indirect=True) +@pytest.mark.parametrize("index_location", ["start", "end"]) +@pytest.mark.parametrize("stored", [b"", b"abc"]) +@pytest.mark.parametrize("op", ["full read", "partial read", "partial write"]) +def test_sharding_truncated_shard_raises( + pipeline_path: str, store: Store, index_location: IndexLocation, stored: bytes, op: str +) -> None: + """A stored shard shorter than its index (zero-length included) is + rejected by every read and partial-write path, under either pipeline, + rather than read as missing by some paths and failing the checksum in + others.""" + with zarr.config.set({"codec_pipeline.path": pipeline_path}): + arr = zarr.create_array( + store, + shape=(8,), + chunks=(2,), + shards={"shape": (4,), "index_location": index_location}, + dtype="i4", + compressors=None, + fill_value=-1, + ) + zarr.core.sync.sync(store.set("c/0", default_buffer_prototype().buffer.from_bytes(stored))) + ops: dict[str, Callable[[], object]] = { + "full read": lambda: arr[0:4], + "partial read": lambda: arr[0:2], + "partial write": lambda: arr.__setitem__(slice(0, 2), 7), + } + with pytest.raises(ValueError, match="too short to hold its index"): + ops[op]() From ae8ff76e1b87cf9a0fa3b1f7a28e8c5d0f7f4dfa Mon Sep 17 00:00:00 2001 From: Davis Vann Bennett Date: Sat, 3 Oct 2026 18:25:38 +0200 Subject: [PATCH 3/4] docs(changes): name the fragment after PR 388 Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- changes/{4463.bugfix.md => 388.bugfix.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changes/{4463.bugfix.md => 388.bugfix.md} (100%) diff --git a/changes/4463.bugfix.md b/changes/388.bugfix.md similarity index 100% rename from changes/4463.bugfix.md rename to changes/388.bugfix.md From bd7544ceaa356a0de89f6dd59d5794aa5343dae2 Mon Sep 17 00:00:00 2001 From: Davis Vann Bennett Date: Sat, 3 Oct 2026 18:38:17 +0200 Subject: [PATCH 4/4] docs(changes): name the fragment after upstream PR 4475 Assisted-by: ClaudeCode:claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- changes/{388.bugfix.md => 4475.bugfix.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changes/{388.bugfix.md => 4475.bugfix.md} (100%) diff --git a/changes/388.bugfix.md b/changes/4475.bugfix.md similarity index 100% rename from changes/388.bugfix.md rename to changes/4475.bugfix.md