feat(fizarrita): share in-flight chunk fetches and decodes - #10
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
In-flight deduplication is keyed only by the cache key, which can cause callers using different cache instances (or cache vs no-cache) to share a promise and silently skip populating the caller-provided cache.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR improves @fideus-labs/fizarrita’s getWorker chunk-reading path by deduplicating concurrent fetch+decode work for the same chunk (so overlapping reads share one in-flight operation rather than duplicating network + decode), and updates/extends browser tests to validate the new behavior.
Changes:
- Add a module-level in-flight map to share concurrent chunk fetch+decode promises keyed like the chunk cache.
- Re-check the cache when each task starts (not only when building the task list) to pick up chunks completed by concurrent calls.
- Add tests covering concurrent dedup (fetch/decode once) and ensuring failed fetches don’t poison future attempts.
File summaries
| File | Description |
|---|---|
| fizarrita/src/get-worker.ts | Introduces in-flight promise sharing and an additional cache re-check at task start; adjusts caching behavior to avoid redundant writes. |
| test/browser/zarrita-worker.spec.ts | Updates cache op-count expectations and adds concurrency/failure regression tests for in-flight sharing. |
Review details
Suppressed comments (1)
fizarrita/src/get-worker.ts:887
- In-flight dedup currently uses
cacheKeyalone. If you scope pending entries by cache identity (see helper above), update the key passed toshareInFlightChunkso calls with different caches (or cache vs no-cache) don’t share the same promise and silently skip populating the caller’s cache.
const chunk = await shareInFlightChunk<D>(cacheKey, async () => {
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| const pendingChunks = new Map<string, Promise<Chunk<DataType>>>() | ||
|
|
| // Let all three pile up on the gate, then let them through together. | ||
| await new Promise((resolve) => setTimeout(resolve, 100)) | ||
| release() | ||
| const chunks = await Promise.all(reads) |
|
@coderabbitai review |
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe worker shares in-flight chunk fetch/decode promises by cache key. It rechecks caches, conditionally stores decoded chunks, preserves the direct no-cache path, and removes rejected promises so later reads can retry. ChangesChunk sharing and cache coordination
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CallerA
participant CallerB
participant ChunkTask
participant shareInFlightChunk
participant Cache
CallerA->>ChunkTask: request chunk
CallerB->>ChunkTask: request same chunk
ChunkTask->>Cache: recheck chunk
ChunkTask->>shareInFlightChunk: request missing chunk
shareInFlightChunk-->>CallerA: share pending result
shareInFlightChunk-->>CallerB: share pending result
shareInFlightChunk->>Cache: conditionally store decoded chunk
Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
|
|
Both addressed in f85ee2d — thanks, the first one was a real hole. Caches not being populated for sharers. Confirmed: I went a different way than scoping the pending key by cache identity, and I'd rather flag that than have it look like I missed the suggestion. Scoping the key fixes the contract by removing the dedup whenever two readers hold different caches: they'd each do a store round-trip and each run a decode, to arrive at the same chunk. But neither the round-trip nor the decode belongs to a cache — they're properties of the chunk. So the key stays on the chunk, and each caller writes its own cache once the chunk arrives. The write is guarded by a lookup rather than unconditional. With a shared chunk object a second write isn't just redundant: it displaces a live entry with itself, which a cache that disposes on overwrite would act on. New test, Cost is one more cache lookup per chunk on a cold read, so the op counts in Fixed sleep in the concurrency test. Fair, and it was worse than it looked. The obvious fix — wait until three gated requests are observed — deadlocks on a passing run, because dedup working means only one request ever reaches the gate. It now waits on the shape probe instead: that runs once per call, outside the task path, so it survives dedup, and three c/0 reads means all three callers are past probing and into their task phase. A task's first act is the Full suite: 113/113. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fizarrita/src/get-worker.ts (1)
882-904: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument or prevent divergent store options in chunk sharing.
cacheKeyandpendingChunksusestore:path:chunkKeyonly, whileshareInFlightChunkfetches with the producer’sopts.opts. If one caller provides anAbortSignal, all followers share that aborted fetch; if two callers provide different store options, followers receive the producer’s request result. If this is intentional, state the contract in the sharing comment block; if store options affect the result, include them in cache/in-flight keys or reject divergent shared calls.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fizarrita/src/get-worker.ts` around lines 882 - 904, Document the sharing contract in the comment above shareInFlightChunk: clarify that callers sharing the same cacheKey also share the producer’s arr.store.get options, including AbortSignal behavior, and that divergent store options are not supported. If options can affect the fetched result, instead incorporate the relevant options into cacheKey/pendingChunks key generation or reject mismatched calls.
🧹 Nitpick comments (3)
test/browser/zarrita-worker.spec.ts (1)
2440-2447: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the shared object for
c/0as well, or state why it is not asserted.
c1Keyis resolved fromcacheA. IfcacheAis empty,c1KeyisundefinedandsameChunkObjectisfalse, which fails for the right reason. The assertion is sound.Only
c/1is gated, soc/0may be fetched twice and produce two distinct objects. That is expected. A short comment stating thatc/0identity is deliberately not asserted prevents a future reader from adding a failing assertion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/browser/zarrita-worker.spec.ts` around lines 2440 - 2447, Add a brief comment near sameChunkObject clarifying that identity is intentionally asserted only for c/1, because c/0 may be fetched twice and can produce distinct objects; do not add a c/0 identity assertion.fizarrita/src/get-worker.ts (2)
906-923: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument that the decoded chunk is now aliased across caches.
The guarded write is correct and avoids dispose-on-overwrite. One new property is worth stating in the comment: the same
Chunkobject is now stored in several independent caches and copied into several outputs. Any consumer that mutateschunk.datain place affects every other caller. The current code only reads the chunk, so this is a documentation point rather than a defect.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fizarrita/src/get-worker.ts` around lines 906 - 923, Update the comment above the guarded cache write in the read flow to document that the same Chunk object may be aliased across independent caches and copied into multiple outputs. State that mutating chunk.data in place would affect other callers, while the current path only reads it.
118-140: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider closing the settle-to-cleanup window and tightening the casts.
forgetruns in a microtask after the promise settles. A caller that callsshareInFlightChunkin that window receives the already-rejected promise and fails without attempting a fetch. The next attempt succeeds, so the impact is limited, but a settled flag removes the window.The double cast through
unknownalso erases the relationship between the map value andChunk<D>. A single storage type plus one cast at read keeps the same behavior with less type erasure.♻️ Optional: track settlement explicitly
const promise = produce() - pendingChunks.set(key, promise as unknown as Promise<Chunk<DataType>>) + let settled = false + pendingChunks.set(key, promise as Promise<Chunk<DataType>>) const forget = () => { - if (pendingChunks.get(key) === (promise as unknown)) { + settled = true + if (pendingChunks.get(key) === (promise as unknown)) { pendingChunks.delete(key) } }with the lookup guarded by
if (inFlight && !settledFor(key)), or simply keep the current form if the one-tick window is acceptable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fizarrita/src/get-worker.ts` around lines 118 - 140, Update shareInFlightChunk to track whether the produced promise has settled and reject or remove settled entries during lookup, so callers cannot receive an already-settled failed promise during cleanup. Simplify pendingChunks storage to one consistent promise type and retain only the necessary single cast when returning the cached promise, removing the unknown-based double casts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/browser/zarrita-worker.spec.ts`:
- Around line 2303-2307: Replace the total /c/0 read count with a dedicated
probe-only counter incremented before the task phase. At
test/browser/zarrita-worker.spec.ts lines 2303-2307, wait for probesSeen to
reach 3; apply the same change at lines 2430-2434 and wait for 2, ensuring task
fetches cannot satisfy the readiness signal.
---
Outside diff comments:
In `@fizarrita/src/get-worker.ts`:
- Around line 882-904: Document the sharing contract in the comment above
shareInFlightChunk: clarify that callers sharing the same cacheKey also share
the producer’s arr.store.get options, including AbortSignal behavior, and that
divergent store options are not supported. If options can affect the fetched
result, instead incorporate the relevant options into cacheKey/pendingChunks key
generation or reject mismatched calls.
---
Nitpick comments:
In `@fizarrita/src/get-worker.ts`:
- Around line 906-923: Update the comment above the guarded cache write in the
read flow to document that the same Chunk object may be aliased across
independent caches and copied into multiple outputs. State that mutating
chunk.data in place would affect other callers, while the current path only
reads it.
- Around line 118-140: Update shareInFlightChunk to track whether the produced
promise has settled and reject or remove settled entries during lookup, so
callers cannot receive an already-settled failed promise during cleanup.
Simplify pendingChunks storage to one consistent promise type and retain only
the necessary single cast when returning the cached promise, removing the
unknown-based double casts.
In `@test/browser/zarrita-worker.spec.ts`:
- Around line 2440-2447: Add a brief comment near sameChunkObject clarifying
that identity is intentionally asserted only for c/1, because c/0 may be fetched
twice and can produce distinct objects; do not add a c/0 identity assertion.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 32b99082-fcc7-4a8b-8272-eae920d8e8f4
📒 Files selected for processing (2)
fizarrita/src/get-worker.tstest/browser/zarrita-worker.spec.ts
| await waitFor( | ||
| () => chunkPaths.filter((path) => path.endsWith('/c/0')).length >= 3, | ||
| 'all three callers to finish probing', | ||
| ) | ||
| await waitFor(() => gatedRequests >= 1, 'the shared c/1 fetch to begin') |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
The readiness signal counts task fetches of /c/0, not only probe reads.
Both tests wait until chunkPaths holds N reads of /c/0, where N is the caller count. The interceptor pushes every /c/0 read, including the task fetch that follows probing. The threshold can therefore be reached from fewer callers than intended. Example for the three-caller test: caller A probes, caller A's task fetches /c/0, caller B probes. The count is 3 while caller C has not probed. release() runs, the shared /c/1 fetch completes, and caller C then performs a second /c/1 fetch. expect(result.c1Reads).toBe(1) fails intermittently.
Count probe reads separately instead of relying on the total. A dedicated counter that is incremented only before the task phase makes the signal exact.
test/browser/zarrita-worker.spec.ts#L2303-L2307: replace the/c/0total with a probe-only counter and wait for 3.test/browser/zarrita-worker.spec.ts#L2430-L2434: apply the same change and wait for 2.
💚 One way to make the signal exact
- const originalGet = arr.store.get.bind(arr.store)
- const chunkPaths: string[] = []
+ const originalGet = arr.store.get.bind(arr.store)
+ const chunkPaths: string[] = []
+ // The probe is the only `/c/0` read that happens before any task runs.
+ // Count callers, not raw reads, so a task fetch of `/c/0` cannot
+ // satisfy the wait on behalf of a caller that has not probed yet.
+ let probesSeen = 0
+ let tasksStarted = false
;(arr.store as any).get = async (path: string, ...rest: any[]) => {
if (path.includes('/c/')) {
chunkPaths.push(path)
+ if (path.endsWith('/c/0') && !tasksStarted) probesSeen += 1
if (path.endsWith('/c/1')) {
+ tasksStarted = true
gatedRequests += 1
await gate
}
}
return originalGet(path, ...rest)
}Then wait on probesSeen. If that heuristic is still too coupled to ordering, wrap each getWorker call so the test itself signals when the call is created and when its first chunk read occurs.
📍 Affects 1 file
test/browser/zarrita-worker.spec.ts#L2303-L2307(this comment)test/browser/zarrita-worker.spec.ts#L2430-L2434
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/browser/zarrita-worker.spec.ts` around lines 2303 - 2307, Replace the
total /c/0 read count with a dedicated probe-only counter incremented before the
task phase. At test/browser/zarrita-worker.spec.ts lines 2303-2307, wait for
probesSeen to reach 3; apply the same change at lines 2430-2434 and wait for 2,
ensuring task fetches cannot satisfy the readiness signal.
|
Heads up: the red What happens. Why now. Fix. Move the setting into allowBuilds:
esbuild: trueThe Verified, in clean clones with
Worth pinning too. The config fix unblocks today, but CI still tracks Happy to send either or both as a separate PR if useful. |
|
@xinaesthete thanks for the updates 👏 pnpm 11 support added in #11 -- please rebase on |
`getWorker` consulted the cache while building its task list and wrote back only after the worker returned, so nothing existed to join between "someone started fetching this chunk" and "the result is cacheable". Two overlapping calls — two viewports, a re-render arriving mid-flight — both missed, both fetched the same bytes, and both decoded them. A cache cannot close that window on its own: `ChunkCache` is synchronous and holds decoded chunks, so an entry appears only once a decode has finished. Adds a module-level map of in-flight chunk promises, keyed exactly like the cache, so concurrent readers of one chunk share one fetch and one decode. Two smaller changes fall out of the same window: - The cache is consulted again when a task starts, not only when the task list was built. A chunk another call finished in between is now picked up instead of being refetched and redecoded. This costs one extra `cache.get` per chunk that reaches the task stage, which is why the op counts in "custom cache implementation receives get/set calls" move from 4/6 to 6/8. - Only the producer writes to the cache. Letting every sharer re-`set` the same object would be a redundant write, and a cache with dispose semantics would see its own live entry displaced by itself. The SAB-without-cache path is untouched: it decodes straight into the calling read's SharedArrayBuffer using that read's mapping, so there is no standalone chunk to hand to anyone else. In-flight entries are dropped as soon as they settle, on both paths. Keeping a rejection would make one transient fetch failure permanent for that chunk; keeping a fulfilment would shadow the cache and pin chunks it had since evicted. A sharer does hold its worker slot while waiting, costing some parallelism — but that slot would otherwise have gone to a duplicate round-trip and a duplicate decompression of bytes already in flight, so no useful work is displaced. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review catch: sharing was keyed on the chunk while the `cache.set` lived inside
the producer, so a read that shared someone else's in-flight chunk never had its
own cache filled. Worst shape is a cache-holding read sharing with a no-cache
one — it asked for caching and silently got none for that chunk, which is the
documented `cache` contract ("on a cache miss the decoded chunk is stored for
future use") quietly not holding.
Each caller now writes its own cache once the chunk arrives. The write is
guarded by a lookup rather than unconditional: with a shared chunk object, a
second write does not merely repeat itself, it displaces a live entry with
itself, which a cache that disposes on overwrite would act on.
Keeps the pending map keyed on the chunk rather than scoping it per cache. The
expensive half is the store round-trip and the decode, and neither belongs to a
particular cache — scoping the key would make two readers holding different
caches fetch and decode the same bytes twice to arrive at the same chunk.
Adds "every concurrent caller gets its own cache populated" — two overlapping
reads with different caches, asserting one fetch, both caches filled, and the
same chunk object in each. Verified to fail before this commit.
The op counts in "custom cache implementation receives get/set calls" move again,
to 8/10, for the third lookup this adds.
Also replaces the fixed 100 ms sleep in the concurrency test with condition-based
waits. The gated-request count cannot be the signal — dedup working means only
one request reaches the gate — so it waits on the shape probe instead, which runs
once per call outside the task path and so survives dedup.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
f85ee2d to
ebe8bd1
Compare
|
Rebased onto One conflict, in the task body: One interaction worth naming, since Sharing is keyed on the chunk, so the producer's decode runs on its worker, from its pool. If two reads of the same array use different pools and the producer's pool is terminated mid-flight, the shared decode now rejects — and every sharer rejects with it, including one whose own pool is perfectly healthy. Before terminate rejected in-flight work this could only hang; before this PR the sharer would have decoded on its own worker and succeeded. I have not tried to fix it. Recovering means having sharers retry on their own worker when the shared promise rejects, which is a meaningful amount of machinery and invites retry storms when the rejection is a genuine store failure rather than a teardown. It also needs a rejection taxonomy — "your pool died" vs "the bytes are not there" — which doesn't exist today. It seems narrow to me: same store, same array, different pools, terminate racing an in-flight decode. Most callers pass one pool, and terminating it fails those reads regardless of this PR. But it is a real behaviour change and your call whether it wants handling before this lands, or a note in the docs, or nothing. Happy to take it either way. |
Yes, this is not a primary use case. I check the benchmark against |
Follow-up to #9, independent of it — this branch is off
mainand the two don't touch the same code. Same context: we usegetWorkeras codec offload for OME-Zarr imagery in SpatialData.ts, and this is the gap that most limits what a chunk cache can do for us.The gap
getWorkerreads the cache while building its task list, and writes back only after the worker returns. Nothing exists in between. So for the whole span of a fetch plus a decode, a chunk is invisible: a secondgetWorkerthat overlaps the first misses the cache, fetches the same bytes, and decodes them again.That is not an edge case for tiled imagery — it is the normal case. Two viewports over one array, a scale-level change, or a re-render arriving while the previous read is still in flight all produce overlapping selections over the same chunks.
A cache can't close this by itself:
ChunkCacheis synchronous and stores decoded chunks, so an entry can only appear once a decode has already finished. Deduplication has to key on the operation, not the result.The change
A module-level
Map<string, Promise<Chunk>>of in-flight fetch+decode operations, keyed exactly like the cache (store_N:/path:chunkKey). The first caller for a chunk produces it; concurrent callers await the same promise and copy from the same decoded chunk into their own outputs.Entries are removed as soon as they settle, on both paths. Keeping a rejection would make one transient fetch failure permanent for that chunk; keeping a fulfilment would shadow the cache and pin chunks it had since evicted. Removal is conditional on the entry still being the current one, so a later attempt that already replaced it isn't dropped by an earlier settlement.
Two smaller things fall out of the same window:
custom cache implementation receives get/set callsfrom 4/6 to 6/8 — one extracache.getper chunk that reaches the task stage. I updated that test's expectations and its comment; shout if you'd rather I dropped this half and left the counts alone.setting the same object would be a redundant write, and a cache with dispose semantics would watch its own live entry be displaced by itself.The SAB-without-cache path is deliberately untouched. It decodes straight into the calling read's
SharedArrayBufferusing that read's mapping, so there is no standalone chunk to hand anyone else. It keeps its original behaviour exactly.Cost
A sharer holds its worker slot while it waits, which costs some parallelism. That slot would otherwise have been spent on a duplicate network round-trip and a duplicate decompression of bytes already in flight, so nothing useful is displaced — but it is a real property and I'd rather state it than have it discovered.
There is no deadlock risk: an entry is registered only once its task is running, so a sharer can only ever be waiting on a producer that already holds a slot and is making progress.
Tests
Two added, both verified to fail without the change:
concurrent reads of the same chunk fetch and decode it once— three concurrentgetWorkercalls, with reads ofc/1gated so they genuinely overlap. Asserts one fetch (3 without the change) and that all three callers still receive complete, correct data — sharing one decode must not mean sharing a half-filled output.Two details worth knowing if you tweak it: the pool is deliberately wider than the total task count, since otherwise the first caller's tasks occupy every slot, settle, and never overlap with the others; and only
c/1is gated, because the shape probe readsc/0before any task runs, so gating that stalls all three callers before the task phase and nothing overlaps.a failed chunk fetch is not remembered by the in-flight map— failsc/1once, then retries and expects correct data. Guards the rejection-cleanup half, which is silent when it regresses.Full suite: 121/121 on the branch (112 on
mainplus these; the count differs from #9's 119 because that branch adds 9 of its own).🤖 Generated with Claude Code
Summary by CodeRabbit
Performance
Reliability