Pool the sidecar read seam's per-chunk fetches (read_workers)

[espg]

What

Pools the per-chunk ranged fetches inside the sidecar backend's read seam (_read_fn_for): the covering chunks of one planned read are independent GETs, now fetched through a bounded, order-preserving ThreadPoolExecutor (pool.map keeps read_plan order, so read_from_buffers semantics are untouched). Width comes from data_source.read_workers (the same knob zagg 0.15.0 uses for its dataset-level pool — one knob, two fan-out levels), default 8, 1 = the exact pre-pool serial behavior. A driver None return (h5coro drivers swallow exceptions) now raises OSError at the fetch instead of failing inside the decoder with a buffer-type error (zagg PR #173 review lesson).

Also in scope (espg call on the session thread): flat-source support. read_group now serves read-plan-less data sources through zagg ≥ 0.15's compiled full-read seam (_read_group_full(read_fn=...)), mirroring inline's dual route — most non-ATL03 products are flat. Against an older zagg (no seam), a flat source fails loudly with the upgrade path instead of silently degrading; the spatial_index validation requirement is dropped (chunk_boundaries mutual exclusion stays), and read_workers is now also gated at backend resolution (submission time) rather than only inside a group read.

Why

Fleet evidence from zagg 0.15.0 (englacial/zagg#170, the 2026-07-07 manual benchmark): the sidecar read of the densest NEON o10 shard is fetch-bound, not decode-bound — its read phase (195 s) is indistinguishable from the pure-h5coro baseline because the compiled decode win is given back to serial per-chunk round trips. zagg's dataset-level pool can't reach them (ATL03 groups expose only 2–3 datasets per planned read); this seam is where the latency actually serializes. With ~10–20 covering chunks per (granule, dataset) at in-region S3 latencies, an 8-wide pool should recover most of the PR-150-measured decode advantage.

Sizing note (documented in the helper): worst-case in-flight GETs = zagg's dataset pool × this width; boto3's default 10-connection client pool soft-caps realized concurrency — excess fetches queue, they don't error.

Testing

tests/test_backend_hermetic.py: pooled-vs-serial byte-identity under adversarially scrambled completion order (jittered fake driver; multi-chunk hyperslab span included per review); None-buffer → OSError on both the pooled (full-read, mid-stream pool.map re-raise) and serial branches; read_workers validation incl. the submission-time gate; flat-route dispatch to the 0.15 seam with a working read_fn (routing pinned against a seam-stub), and the actionable old-zagg error. Full suite: 64 passed, 1 skipped (network tier), against a maturin develop --locked build. A fresh-context adversarial review pass ran on the first commit; all five findings folded (sizing-note correction — S3Driver provisions max_pool_connections=100, so 8×8 concurrency is realized, not queued; HTTPDriver shared-Session caveat documented; pooled error path + multi-chunk jitter spans now exercised; read_workers gated at validation).

Release

The version bump to 0.3.0 is included (Cargo.toml + Cargo.lock; maturin derives the wheel version from it — verified maturin develop --locked installs 0.3.0). Post-merge: git tag 0.3.0 && git push origin 0.3.0 triggers wheels.yml (tag push) → trusted-publishing wheels; then zagg bumps its h5coro-hidefix>=0.3.0 pin and redeploys the layer (the englacial/zagg#154 pattern).

Not in this PR (follow-ups)

• Rust-side rayon buffer decode still needs upstream hidefix (parallel CacheReader or public decode_chunk) — additive, tracked on Compiled (hidefix) read path is never exercised by default, and its fetch/decode is serial englacial/zagg#170.

[espg]

🤖 from Claude (review)

Fresh-context adversarial review of the chunk-fetch pool. Four findings below (one doc-factual, one driver flag, two test-coverage); the core mechanics all verified clean:

• Exception semantics: pool.map's result iterator cancels pending futures in its finally when list() re-raises (CPython 3.12 Executor.map), so a mid-stream failure wastes at most workers already-running GETs — bounded, matches the docstring. The OSError passes straight through read_fn (its try catches only KeyError → _ManifestMiss; OSError is not a KeyError subclass), and upstream zagg's worker counts it as a per-group read error and continues the shard (worker.py per-group except Exception → read_errors) — the intended transient-I/O semantics.
• S3Driver thread safety: one boto3 client per driver instance, get_object only, no shared mutable read state (_closed is read-only in read()); boto3 clients are documented thread-safe. FileDriver serializes on its lock (pool is a harmless no-op locally).
• numpy under threads: int(addrs[i])/int(sizes[i]) on read-only arrays under the GIL — fine.
• Memory: no new regression — the serial version also accumulated all buffers before read_from_buffers; the pool only adds in-flight fetch buffers (≤ workers).
• Jitter test math: verified against the fixture — a full-dataset read covers 10 chunks (N=10_000 / CHUNK=1_000), ranks 1–5 sleep 25→5 ms while ranks 6+ return immediately, so completion order on the first pooled read is genuinely scrambled with robust 5 ms rank gaps. The assertion is real.

Full suite: 62 passed, 1 skipped, reproduced locally.

[espg]

🤖 from Claude (review)

The sizing note's premise is wrong for the driver zagg actually uses. h5coro's S3Driver does not run on boto3's default 10-connection pool — it explicitly configures max_pool_connections=self.max_connections, and since H5Coro.__init__ instantiates the driver as driverClass(resource, credentials) (no max_connections kwarg), that resolves to S3Driver._global_max_connections = 100 (s3driver.py in the pinned h5coro). So there is no 10-connection soft-cap: worst case, zagg's dataset pool (8) × this width (8) = up to 64 concurrent GETs are realized on real sockets, not queued.

Correctness is unaffected (S3Driver's own retries={'max_attempts': 2, 'mode': 'adaptive'} + read_timeout=5 degrade a throttle to None → your OSError → one read error), but the "safe — excess fetches queue" rationale here and in the PR body doesn't hold as written. Suggest rewording the note to the actual bound (dataset pool × this width, ~64 concurrent GETs against a 100-connection client) — and if the 10-cap was load-bearing in the width choice, reconsidering the default. The https driver's HTTPAdapter (pool_maxsize=50, non-blocking) similarly creates-and-discards connections beyond 50 rather than queueing.

[espg]

🤖 from Claude (review)

Two driver-specific behaviors under the pool worth a caveat in this docstring (verified against the pinned h5coro):

1. driver: https shares one requests.Session across the pool threads. zagg's https arm (worker.py) builds webdriver.HTTPDriver, whose read() issues self.session.get(..., allow_redirects=True) on a single shared Session. requests does not guarantee Session thread safety; the murky part is cookie-jar mutation during concurrent redirect chains — exactly the EDL flow this driver exists for. Practical risk in CPython is low (CookieJar locks internally, urllib3's pool is thread-safe), but it's a documented-unsupported combination now exercised 8-wide. Worth a sentence here; if EDL cookie races ever surface, driver.copy() gives a cheap per-thread session.

2. The None-return contract is S3Driver-shaped, not universal. HTTPDriver.read never returns None — it raises FatalError (a RuntimeError) after retries. That still propagates correctly (it isn't a KeyError, so it can't be misdiagnosed as a _ManifestMiss), so no bug — but "a driver that swallows an error and returns None" is only the S3/file behavior. FileDriver additionally serializes all reads on its instance lock, so pooling is a no-op for local files (harmless).

[espg]

🤖 from Claude (review)

"fails loudly" overstates what actually happens in a fleet worker. This ValueError is raised inside read_group, and zagg's worker wraps each group read in except Exception → read_errors += 1, warning, continue — so a bad read_workers from an older-zagg/hand-rolled payload yields a shard that completes with empty output and a read_errors count: the exact zagg PR #173 finding-2 pattern this docstring's neighbor cites.

Raising is still the right call (silently running serial would be worse, and it exactly mirrors zagg's own _read_workers in processing/read.py — same message, same in-read-path raise; zagg ≥0.15.0 additionally validates at submission). But there's a cheap way to make the claim true here too: SidecarIndex.validate_index_config already receives data_source and runs at submission time under the pinned protocol (zagg config.py → validate_index_config(index_cfg, data_source)) — adding the same check there fails the submission loudly even for callers that predate zagg's config-side validation. Suggest that plus softening this sentence to "…still surfaces as a per-group read error instead of silently running serial."

[espg]

🤖 from Claude (review)

This never exercises the pooled error path. The span [(0, 10)] covers exactly 1 chunk (verified against the fixture: read_plan returns n=1), so _fetch_chunks takes the workers <= 1 or n <= 1 serial branch and the OSError is raised from the list comprehension — pool.map's re-raise/pending-cancel machinery is never crossed, despite workers=4.

A full read makes it real: read_fn(DSETS[0]) covers 10 chunks (N=10_000 / CHUNK=1_000), so the OSError must propagate out of list(pool.map(...)) through the executor's __exit__. One-line change, and it pins the exception contract on the branch this PR actually adds.

[espg]

🤖 from Claude (review)

Minor coverage note: both hyperslab spans cover a single chunk each ((5, 800) → chunk 0, (2500, 2501) → chunk 2; verified via read_plan), so the hyperslab arm of this test runs the serial branch on both sides — only the two full-dataset reads (10 chunks each) exercise the pool. Also, the jitter budget 0.03 - 0.005 * rank hits zero at rank 6 and calls is shared across all reads in the test, so only the first pooled full read sees scrambled completion; the second dataset's pooled read runs unjittered. That first read alone is a genuine 10-chunk scramble with 5 ms rank gaps, so the assertion stands — but if you want the hyperslab path pooled too, a multi-chunk span like (500, 3500) (4 covering chunks) would do it.

[espg]

🤖 from Claude

All five review findings folded in 259a6fa, plus the flat-source route espg pulled into scope: (1) sizing note corrected — h5coro's S3Driver provisions max_pool_connections=100, so worst-case 8×8 concurrency is realized, not queued (docstring + PR body fixed); (2) HTTPDriver shared-Session caveat documented (S3 is the deployed path; read_workers: 1 is the strict-isolation escape hatch); (3) the None→OSError test now exercises the pooled full-read branch (mid-stream pool.map re-raise) as well as the serial one; (4) read_workers is gated in validate_index_config at submission time, and the 'fails loudly' wording softened to describe the actual worker-side swallow; (5) jitter test gained a multi-chunk hyperslab span. Flat sources now route through zagg ≥0.15's _read_group_full seam (routing pinned against a stub; actionable error on older zagg). 64 passed, 1 skipped.