SidecarIndex: lock the on_miss:build delegate init; process-global chunk-fetch budget

[espg]

Closes #5 (both items: the inline-delegate init race, and the process-global chunk-fetch budget)

Item 1: inline-delegate init race (on_miss: build)

The race

SidecarIndex._miss (0.3.0, zagg_backend.py:260-264) did an unguarded check-then-act on self._inline under on_miss: build. zagg PR englacial/zagg#183 moves the worker to a bounded thread pool with K granules in flight, calling read_group/finish_granule from pool threads — two concurrently-missing granules could both observe self._inline is None and construct two InlineIndex(write_back=True) delegates. One assignment wins; the loser granule keeps reading through the orphaned instance for the rest of its read_group call, accumulating its pending chunk maps there. finish_granule forwards only to self._inline — the winner — so the loser's pending maps are never drained and its written-back manifest is silently sparse (missing datasets, no error raised).

The fix

Double-checked locking: self._inline_lock = threading.Lock() in __init__ next to _inline; _miss checks self._inline is None, takes the lock, re-checks, constructs once. The zagg.index.inline import stays lazy inside the locked branch, so module load still never pulls it (no hard/circular zagg import).

Contract restatement + version note

• The module docstring and finish_granule docs previously described the old contract (worker "reads granules serially and calls finish_granule after each"). Restated per Granule-level read concurrency (K in flight, ordered fold) englacial/zagg#183: granule reads may interleave across threads; per-granule state must be keyed by granule id (full resource URL); finish_granule(h5obj, granule_url) is called exactly once per granule, from the thread that read it.
• Version note (docstring): on_miss: build + concurrent granules requires zagg >= the release carrying Granule-level read concurrency (K in flight, ordered fold) englacial/zagg#183 (its _pending re-key to the full resource URL); older zagg's path-keyed _pending would interleave chunk maps across granules in the shared delegate.
• self._cache (granule-id-keyed, single-key get/pop per granule) is safe under the GIL as-is; its comment now says so.

Test

tests/test_backend_hermetic.py::TestProtocolShape::test_build_delegate_constructed_once_under_concurrent_misses — hermetic (zagg-stub seam via sidecar_cls, plus a stubbed zagg.index.inline module in sys.modules): two threads read two granules neither covered by the store under on_miss: build, with a threading.Barrier(2) inside the stub InlineIndex.__init__. Under the unguarded code both threads land inside __init__ (the barrier releases them, deterministically recording two constructions); under the lock exactly one thread enters, its barrier times out and breaks, and construction proceeds alone. Asserts exactly one delegate is constructed, both threads read through that same instance, and finish_granule forwards to it for both granules. Verified the test fails against the unguarded 0.3.0 code (2 constructions) and passes with the lock.

Item 2: process-global chunk-fetch budget (shared semaphore)

The problem

Each granule's read_fn pools its chunk fetches (_fetch_workers(data_source) wide), zagg's dataset-level read_workers pool multiplies it, and zagg PR englacial/zagg#183 multiplies again by K granules in flight — total in-flight S3 range-GETs become K × read_workers × fetch-width (~384 at K=6 with defaults) against the S3 driver's ~100-connection urllib3 pool. The overflow queues inside urllib3 where the 5 s timeout can fire, so pool exhaustion masquerades as read_errors (zagg PR #183 review finding 4).

The fix

Module-level threading.BoundedSemaphore acquired around the ioRequest call in _fetch_chunks' fetch worker (both the pooled and serial branches go through the same closure). Width from module constant _FETCH_BUDGET (default 64 ≈ the S3 driver's max_pool_connections=100 with headroom — a single number, deliberately not auto-tuned; zagg's fleet A/B at K∈{1,2,4,6} produces the evidence for a better default if one exists). Queueing now happens in our code — orderly, no timeout artifacts — and budget idled by a draining granule flows to granules still reading (the right behavior for tail-granule skew). Env-overridable via ZAGG_HIDEFIX_FETCH_BUDGET (parsed defensively: positive ints win, anything else logs a warning and falls back) so ops can tune without a release; documented in the _FETCH_BUDGET comment and the _fetch_chunks sizing note. The semaphore is held only around the GET itself, never while holding another lock, so contention cannot deadlock.

Note: zagg-side #183 ships a static warn/clamp guard on the K×workers product; this is the dynamic fix that makes the static guard unnecessary.

Tests

• test_fetch_budget_caps_concurrent_gets — concurrency-tracking fake driver, budget monkeypatched to 4, workers=32 over a >4-chunk full read: peak concurrent ioRequest never exceeds 4 (and ≥2, proving the pool actually ran concurrently), bytes identical to direct reads. Verified this test fails with the semaphore removed (peak 32 > 4).
• test_fetch_budget_contention_across_read_fns — budget 2, two threads reading different datasets through two read_fns at workers=8: both complete byte-identical within a join timeout (no deadlock).
• test_fetch_budget_env_override — defensive env parsing (128 wins; 0, -3, eight, 2.5, "" fall back to the default).

Interaction with PR #4

This branch is off main (0.3.0), whose _fetch_chunks predates PR #4's range coalescing, so the semaphore wraps main's per-chunk fetch. PR #4 touches the same function; whichever merges second rebases the fetch site — the semaphore wraps the span fetch there, one line either way.

Gates

• maturin develop --locked: clean
• .venv/bin/python -m pytest tests/ -q: 68 passed, 1 skipped (main baseline is 64 passed, 1 skipped — this PR adds exactly the four new tests)

Versioning

No version bump here — these fixes ride the 0.3.1 release together with PR #4 (which carries the 0.3.1 bump on claude/range-coalescing).

Credit

Both findings surfaced by the adversarial review on zagg PR englacial/zagg#183: item 1 from englacial/zagg#183 (comment), item 2 from englacial/zagg#183 (comment)

🤖 Generated with Claude Code

https://claude.ai/code/session_017w2BCjStojyfaTkw77BfT2

[espg]

🤖 from Claude

Conflicts with merged PR #4 resolved (merge commit, no history rewrite): the fetch semaphore now wraps the coalesced span fetches (one acquire per span GET — the docstring's sizing note explains the composition: coalescing collapses most chunk fan-out, the budget backstops many-granule × many-dataset pileups under zagg#183); PR #4's short-return OSError guard is preserved inside the budgeted fetch; both PRs' test blocks kept. One test needed a real rework, not just stitching: the budget-cap test's full read now coalesces to a single span (which would have made it vacuous), so it exercises _fetch_chunks directly with 12 synthetic ranges spaced >1 MiB apart (unmergeable) via a mapping driver — peak concurrency ≤ budget still empirically pinned. Full suite: 77 passed, 1 skipped.