coalesce chunk-range fetches in the sidecar read seam

[espg]

What

Coalesces the sidecar backend's per-chunk ranged GETs into merged spans in _fetch_chunks (python/h5coro_hidefix/zagg_backend.py), the compiled read path's fetch seam added in #3.

Why

Fleet evidence in the englacial/zagg#170 final table: the compiled path loses to plain h5coro 1.0.5 on wall time because h5coro coalesces reads into ~4 MiB cache lines (few round trips) while this seam issued one GET per covering chunk. HDF5 chunk addresses for consecutive chunks of one dataset are usually adjacent or near-adjacent in the file, so most of a planned read's chunks merge into a handful of large ranged GETs.

How

A new _merge_ranges(addrs, sizes) plans the spans; _fetch_chunks fetches the spans through the existing pool and slices each original chunk back out:

• Merge rule: two ranges merge when the file gap between them is ≤ _COALESCE_GAP (1 MiB) and the merged span stays ≤ _COALESCE_MAX_SPAN (16 MiB). Module constants for now; promotable to config tunables if fleet evidence asks for it.
• Tradeoff: gap bytes inside a merged span are fetched and discarded — wasted bandwidth is bounded by GAP per merge, and at S3 latency round trips beat bandwidth. h5coro's own 4 MiB cache lines make the same trade.
• Ordering: inputs may be unsorted (chunk-index order ≠ file order); merging sorts by address, but the returned buffer list is in the original input order, so read_from_buffers' contract is unchanged. Overlapping/duplicate ranges are served from the already-merged span.
• Pool semantics unchanged: same workers width over spans, ThreadPoolExecutor.map order-preserving, a driver None on any span surfaces as OSError (never misdiagnosed as a decode failure — the zagg PR #173 lesson). workers <= 1 stays serial (now serial over merged spans).

memoryview finding

Chunks sliced out of a multi-chunk span are returned as zero-copy memoryview slices — verified working: extract_buffers in src/lib.rs normalizes any bytes-like via the buffer protocol (bytes(memoryview(item))), so read_from_buffers accepts them (byte-identical results, pinned by test_memoryview_slices_accepted_by_decoder). Note the Rust seam copies a memoryview once at extraction (tobytes()); only plain bytes passes zero-copy end-to-end — so total copies are the same as bytes slices, the memoryview just defers the copy to the seam. A span serving exactly one chunk hands the driver buffer through untouched, preserving the pre-coalescing zero-copy path for isolated chunks.

Tests (tests/test_backend_hermetic.py)

• test_coalesced_reads_byte_identical — full reads and multi-chunk hyperslabs byte-identical to direct Index.read, at workers 1 and 8.
• test_coalescing_reduces_get_count — counting fake driver: the 10-chunk gzip dataset's full read now costs ≤ 2 GETs (was 10), every GET ≤ _COALESCE_MAX_SPAN.
• TestFetchChunkCoalescing.test_unsorted_duplicate_overlapping_ranges — synthetic direct _fetch_chunks call: unsorted + duplicate + overlapping ranges come back correct and in input order, one GET total.
• test_gap_and_span_caps_split_spans — gap over GAP and merges over MAX_SPAN split; exactly-at-cap merges.
• test_none_span_surfaces_as_os_error — None from a merged span raises OSError, serial and pooled.
• test_memoryview_slices_accepted_by_decoder — pins the emitted slice type against read_from_buffers.

Gates: pytest tests/ -q → 70 passed, 1 skipped (zagg-integration skip, zagg not installed here) after maturin develop --locked rebuild; cargo fmt --check and cargo clippy --locked -- -D warnings clean (no Rust changes).

Release

Version 0.3.0 → 0.3.1 (behavior-only, no API change). Post-merge: tag 0.3.1 → wheels.yml publishes; zagg's Lambda layer pin bumps to consume (the englacial/zagg#178 pattern).

🤖 Generated with Claude Code

https://claude.ai/code/session_017w2BCjStojyfaTkw77BfT2

[espg]

🤖 from Claude (review)

Fresh-context adversarial review of the range coalescing. One behavioral finding (short driver returns) and two test-coverage findings inline; everything else probed clean, verified empirically against the branch build:

• _merge_ranges edges: a single range larger than _COALESCE_MAX_SPAN is still fetched whole ([0], [MAX+5MB] → one span); contained duplicates — same address with different sizes, in either size order — are served from the covering span; a partial overlap extending past the span's end extends the span ([0,50],[100,100] → one (0,150) span), and slicing can never run off the end: inputs are processed in ascending start order, only the last span mutates and only grows, so every range's assigned span covers it at assignment time and forever after. An overlap whose extension would exceed MAX yields two overlapping spans — correct results, bounded refetch. Exact boundaries: gap == _COALESCE_GAP merges (tested), and span == _COALESCE_MAX_SPAN exactly merges too ([0, MAX-100],[MAX-100,100] → one 16 MiB span; +1 splits) — verified, though untested (see inline). Zero-size ranges degenerate to size-0 GETs, but read_plan can't emit them and the pre-coalescing code had the identical behavior.
• Slicing arithmetic: byte-identity verified across a real 2-span split (5 unsorted chunks split by a >1 MiB gap, workers 1 and 4: 2 GETs, every slice byte-equal), including numpy uint64 addr/size inputs.
• Order restoration: input order confirmed under the pool with unsorted addresses (probe + test_unsorted_duplicate_overlapping_ranges).
• memoryview claim: matches the code — extract_buffers (src/lib.rs) passes PyBytes through and copies anything else once via memoryview(item).tobytes(), so multi-chunk-span slices cost one copy at the seam and single-chunk spans stay zero-copy bytes. And read_from_buffers enforces exact per-chunk buffer lengths, which is what keeps the short-return finding below a diagnosis problem rather than a corruption one.
• None mid-span parity: OSError type/message pattern preserved, pool.map pending-cancel semantics unchanged from #3, and test_none_span_surfaces_as_os_error covers the pooled 2-span case. The GET-counting test counts driver calls, i.e. spans — the right quantity — and pins the span-size cap.

Suite reproduced: 70 passed, 1 skipped.

[espg]

🤖 from Claude (review)

Short driver returns now surface as a decode-shaped error — the exact misdiagnosis this seam's None guard exists to prevent. fetch checks buf is None but not len(buf). A truncated ranged GET (dropped connection mid-body: boto3's StreamingBody.read() and requests' .content can both hand back fewer bytes than the Range asked for without raising) used to shortchange exactly one chunk; now it shortchanges a span, and the memoryview slices silently truncate — verified on this branch: a span cut 13 bytes short yields full-length slices for every chunk except the last, and the operator sees

ValueError: buffer 9 has 7421 bytes but the chunk at dataspace offset [9000] stores 7434 bytes

from read_from_buffers — a decoder-side diagnosis pointing at a chunk, for what was transient I/O. Under zagg's worker that lands in read_errors as a decode-looking warning, which is precisely the zagg PR #173 lesson the adjacent None → OSError line encodes. (Credit where due: the Rust length gate means this can never silently wrong-decode — it's a diagnosis-quality bug, not corruption.)

One-line fix in the guard:

if buf is None or len(buf) != size:
    got = "None" if buf is None else f"{len(buf)} bytes"
    raise OSError(f"ranged read failed at {addr}+{size} (driver returned {got})")

len() on the driver's bytes is free, and it also covers the span==chunk passthrough (probe: a short passthrough buffer currently sails through _fetch_chunks unchecked too).

[espg]

🤖 from Claude (review)

No byte-identity test ever crosses a span split. This test's ranges all merge into one GET (asserted on this line), the fixture datasets are ~10 contiguous gzip chunks (every real read in test_coalesced_reads_byte_identical coalesces to a single span — I checked the plans), test_gap_and_span_caps_split_spans only inspects the merge plan, and the multi-span case in test_none_span_surfaces_as_os_error never returns bytes. So the code this PR most needs pinned — assign/span_bufs[j] indexing and slice offsets across two or more fetched spans, under the pool — is byte-verified by nothing in the suite; a regression that scrambles cross-span assignment would pass today.

I verified it correct empirically, and the case is cheap to add here, e.g.:

gap = _COALESCE_GAP
addrs = [2 * gap + 100, 0, 500, 2 * gap + 900, 200]  # unsorted, split by a > GAP gap
sizes = [300, 100, 400, 50, 250]
for workers in (1, 4):
    calls.clear()
    bufs = _fetch_chunks(self._fake(blob, calls), addrs, sizes, workers)
    assert [bytes(b) for b in bufs] == [blob[a : a + s] for a, s in zip(addrs, sizes)]
    assert len(calls) == 2

(needs a blob of ~2 * gap + 4096 bytes; passes on this branch).

[espg]

🤖 from Claude (review)

Minor: the comment says "at exactly the caps" (plural) but only the gap cap is exercised — there's no case where merged_end - s0 == _COALESCE_MAX_SPAN exactly (the earlier [0, big], [big, 100] case lands at MAX + 50, over the cap). The <= does merge at exactly MAX — verified: _merge_ranges([0, MAX - 100], [MAX - 100, 100]) → one 16 MiB span, and sizes=[MAX - 100, 101] splits — but that boundary is one changed comparison away from silently becoming <, so worth the two-line assertion for symmetry with the gap case.

[espg]

🤖 from Claude

All three review findings folded: the fetch guard now raises OSError on short returns as well as None (covering the span==chunk passthrough — your truncation repro now dies at the fetch with the actionable message instead of the decode-shaped length error); the cross-span byte-identity case you supplied is in (5 unsorted ranges, >1 MiB gap, 2 spans, workers 1 vs 4, gated against file bytes); and the exact-MAX_SPAN boundary assertion is pinned (== merges, +1 splits). 73 passed, 1 skipped after maturin rebuild.