Memory accounting before memory management: ADR 0005 rungs 1-3 - #132
Open
xinaesthete wants to merge 5 commits into
Open
Memory accounting before memory management: ADR 0005 rungs 1-3#132xinaesthete wants to merge 5 commits into
xinaesthete wants to merge 5 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR adds shared memory accounting and byte-bounded LRU caches. It applies the caches to Parquet data in ChangesCache infrastructure and integrations
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SpatialDataTableSource
participant ByteLruCache
participant BackingStore
participant ParquetDecoder
SpatialDataTableSource->>ByteLruCache: Check encoded and decoded caches
ByteLruCache-->>SpatialDataTableSource: Return resident entry or miss
SpatialDataTableSource->>BackingStore: Read missing parquet bytes
BackingStore-->>SpatialDataTableSource: Return parquet bytes
SpatialDataTableSource->>ParquetDecoder: Decode and cache in-flight promise
ParquetDecoder-->>SpatialDataTableSource: Return Arrow table
SpatialDataTableSource->>ByteLruCache: Recount decoded table bytes
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
packages/core/tests/rasterElementStore.spec.ts (2)
56-58: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse one backing store for the distinct-view test.
The two elements currently use different root stores. Pass the same
fakeStore()result to both elements. This verifies thatRasterElementcreates separate prefixed views even when both elements share one backing store.Proposed test change
it('gives different elements different views', () => { - expect(createImageElement().getStore()).not.toBe(createImageElement().getStore()); + const store = fakeStore(); + expect(createImageElement(store).getStore()).not.toBe(createImageElement(store).getStore()); });🤖 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 `@packages/core/tests/rasterElementStore.spec.ts` around lines 56 - 58, Update the “gives different elements different views” test to create one shared fakeStore instance and pass it to both RasterElement instances, then compare their getStore() results. Keep the assertion that the resulting views are distinct while ensuring both elements use the same backing store.
34-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the test
as anyassertions.
ImageElementexpectssource: StoreReferenceonsdata, so line 39 bypasses theSDataProps.sourcecontract. For line 65, use a local absolute-path helper/narrower instead of bypassing Zarrita’szarr.AbsolutePathcontract.🤖 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 `@packages/core/tests/rasterElementStore.spec.ts` around lines 34 - 42, Remove the `as any` assertions from the raster element tests. Update `createImageElement` to provide a valid `source: StoreReference` on `sdata`, and replace the line 65 cast with a local absolute-path helper or type-narrowing approach that satisfies Zarrita’s `zarr.AbsolutePath` contract.Source: Coding guidelines
packages/vis/tests/codecWorkers.spec.ts (1)
18-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the fixture type assertion.
as Chunk<DataType>suppresses validation of the mock chunk shape. Make the object satisfyChunk<DataType>directly so changes to zarrita’s chunk contract fail this test at compile time.Proposed change
-function chunkOf(bytes: number): Chunk<DataType> { +function chunkOf(bytes: number) { return { data: new Uint8Array(bytes), shape: [bytes], stride: [1], - } as Chunk<DataType>; + } satisfies Chunk<DataType>; }As per coding guidelines, “Avoid type assertions (
as)” when a narrower contract can express the same fact.🤖 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 `@packages/vis/tests/codecWorkers.spec.ts` around lines 18 - 24, Update the chunkOf fixture to satisfy the Chunk<DataType> return type without using a type assertion, preserving its existing data, shape, and stride fields so the compiler validates the mock against the chunk contract.Source: Coding guidelines
🤖 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 @.changeset/bound-parquet-caches.md:
- Around line 1-3: Update the changeset release level for `@spatialdata/core` from
minor to major to reflect the breaking parquetTableBytes access change from
bracket indexing to get(path). Do not retain the minor classification unless the
implementation preserves a compatible record-style API.
In `@packages/core/src/memory/byteLruCache.ts`:
- Around line 126-129: Update the eviction and clear flows in the byte LRU
cache, including the replacement path around onDispose and the logic at the
referenced clear-related sections, to complete removals and residentBytes
bookkeeping before invoking any disposal callbacks. Capture the first callback
error, continue invoking all required callbacks, and rethrow the captured error
only after eviction or clearing has fully completed.
- Around line 69-73: Update the ByteLruCache constructor and each sizeOf call to
validate that maxBytes and computed entry sizes are finite, non-negative
numbers, rejecting invalid values before they affect eviction or byte
accounting. Ensure NaN, infinities, and negative counts cannot enter cache state
or allow unbounded growth.
In `@packages/vis/src/codecWorkers.ts`:
- Around line 85-87: Validate options?.chunkCacheMaxBytes before constructing
ByteLruCache: accept only finite, non-negative values, while preserving
DEFAULT_CHUNK_CACHE_MAX_BYTES when the option is absent. Reject Infinity, NaN,
and negative limits before the maxBytes value reaches the cache configuration.
- Around line 15-16: Update the contract for chunkCacheMaxBytes and the
initialization flow in ensureCodecWorkers so the option is read on the first
call that enables workers, rather than the first invocation when Worker is
unavailable. Ensure a later worker-enabled call can apply its value, or persist
the value from the first call and use it when workers become available.
- Around line 23-31: Update chunkByteLength to calculate the actual encoded byte
size of each string payload rather than using data.length, while preserving
typed-array byteLength handling for numeric chunks. Use the existing chunk data
flow and add a test covering variable-length strings to verify the reported size
and maxBytes eviction behavior.
---
Nitpick comments:
In `@packages/core/tests/rasterElementStore.spec.ts`:
- Around line 56-58: Update the “gives different elements different views” test
to create one shared fakeStore instance and pass it to both RasterElement
instances, then compare their getStore() results. Keep the assertion that the
resulting views are distinct while ensuring both elements use the same backing
store.
- Around line 34-42: Remove the `as any` assertions from the raster element
tests. Update `createImageElement` to provide a valid `source: StoreReference`
on `sdata`, and replace the line 65 cast with a local absolute-path helper or
type-narrowing approach that satisfies Zarrita’s `zarr.AbsolutePath` contract.
In `@packages/vis/tests/codecWorkers.spec.ts`:
- Around line 18-24: Update the chunkOf fixture to satisfy the Chunk<DataType>
return type without using a type assertion, preserving its existing data, shape,
and stride fields so the compiler validates the mock against the chunk contract.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ee834d9d-cefd-4c1a-9a41-f42d472b850d
📒 Files selected for processing (17)
.changeset/bound-parquet-caches.md.changeset/fill-chunk-cache-seam.md.changeset/memory-reporting-scalar.md.changeset/parquet-table-cache-rejection-cleanup.mdpackages/core/src/Vutils.tspackages/core/src/index.tspackages/core/src/memory/byteLruCache.tspackages/core/src/memory/index.tspackages/core/src/memory/memoryReporting.tspackages/core/src/models/VTableSource.tspackages/core/src/models/index.tspackages/core/tests/byteLruCache.spec.tspackages/core/tests/parquetTableCache.spec.tspackages/core/tests/rasterElementStore.spec.tspackages/vis/src/codecWorkers.tspackages/vis/src/index.tspackages/vis/tests/codecWorkers.spec.ts
Comment on lines
+126
to
+129
| if (previous) { | ||
| this.onDispose?.(previous.value, key); | ||
| } | ||
| this.evictToBudget(); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Complete cache bookkeeping before disposal callbacks run.
onDispose can throw before evictToBudget() completes. For example, replacing a 50-byte entry with a 200-byte entry under a 100-byte limit leaves the new entry resident when the replacement disposer throws. A throwing disposer in clear() also prevents disposal of later entries.
Remove all required entries and update residentBytes before invoking callbacks. Invoke every callback even if one fails. Propagate a captured callback error only after cache bookkeeping completes.
Also applies to: 170-174, 181-187
🤖 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 `@packages/core/src/memory/byteLruCache.ts` around lines 126 - 129, Update the
eviction and clear flows in the byte LRU cache, including the replacement path
around onDispose and the logic at the referenced clear-related sections, to
complete removals and residentBytes bookkeeping before invoking any disposal
callbacks. Capture the first callback error, continue invoking all required
callbacks, and rethrow the captured error only after eviction or clearing has
fully completed.
Comment on lines
+23
to
+31
| function chunkByteLength(chunk: Chunk<DataType>): number { | ||
| // Numeric dtypes give a typed array, which reports `byteLength` for free — | ||
| // that structural match is the whole point of `MemoryReporting`. String dtypes | ||
| // give a plain array instead, and `length` is the conservative floor there: one | ||
| // byte per element at minimum. Reporting zero would make such entries invisible | ||
| // to the budget and so unevictable by size, which is the one way a bounded | ||
| // cache quietly goes back to being unbounded. | ||
| const data: { byteLength?: number; length: number } = chunk.data; | ||
| return typeof data.byteLength === 'number' ? data.byteLength : data.length; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Account for string payload bytes.
For string chunks, data.length counts elements, not bytes. Long string values can retain substantially more memory than maxBytes reports, so eviction does not enforce the configured byte limit. Size each string payload before inserting it, and add a variable-length string test.
🤖 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 `@packages/vis/src/codecWorkers.ts` around lines 23 - 31, Update
chunkByteLength to calculate the actual encoded byte size of each string payload
rather than using data.length, while preserving typed-array byteLength handling
for numeric chunks. Use the existing chunk data flow and add a test covering
variable-length strings to verify the reported size and maxBytes eviction
behavior.
Comment on lines
+85
to
+87
| chunkCache = new ByteLruCache<Chunk<DataType>>({ | ||
| maxBytes: options?.chunkCacheMaxBytes ?? DEFAULT_CHUNK_CACHE_MAX_BYTES, | ||
| sizeOf: chunkByteLength, |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject non-finite cache limits.
Infinity and NaN are valid number values. residentBytes > Infinity and residentBytes > NaN never evict entries, so a host can disable the cache bound accidentally. Validate that chunkCacheMaxBytes is finite and non-negative before constructing ByteLruCache.
🤖 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 `@packages/vis/src/codecWorkers.ts` around lines 85 - 87, Validate
options?.chunkCacheMaxBytes before constructing ByteLruCache: accept only
finite, non-negative values, while preserving DEFAULT_CHUNK_CACHE_MAX_BYTES when
the option is absent. Reject Infinity, NaN, and negative limits before the
maxBytes value reaches the cache configuration.
This was referenced Aug 5, 2026
`parquetTableCache` caches the table promise before it settles — genuine in-flight dedup — but never cleaned up a rejection, so one transient fetch failure pinned a rejected promise at that path for the lifetime of the source and every later read replayed a network error that had cleared long ago. The entry now evicts itself on rejection, and only if it is still current, so a retry that superseded it is not clobbered by the earlier promise's late rejection — the same discipline `evictIfCurrent` already applies to the dataset metadata and part-path caches. The rejection still propagates unchanged to the caller that provoked it, so the skip-vs-fail policy in `docs/plans/parquet-io-error-handling.md` is untouched. Tests cover the retry as well as the two behaviours that must survive it: concurrent dedup, and successful tables staying cached. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`{ readonly byteLength: number }`, exported from `@spatialdata/core`. One number,
named so that every `TypedArray`, `ArrayBuffer` and `DataView` satisfies it
structurally — no wrapper, no import — which is what makes it cheap enough to put
on every cache rather than a chosen few.
No policy, no eviction, no tiers: the ADR gates rungs 2 and 3 on this existing,
and is emphatic that this rung cannot be over-engineered. The tiered
`ResidencyReport` and the global ceiling stay deferred.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`parquetTableBytes` and `parquetTableCache` were plain `Record`s with no eviction: a source held both the compressed and the decoded tier of every parquet file any caller had ever touched, simultaneously, until the source was discarded. Double memory, zero eviction benefit. Adds `ByteLruCache` — framework-free, byte-bounded, `MemoryReporting`, with a dispose hook — and puts both tiers behind it. Two semantics the tests pin: - An oversized value is admitted and left sole resident rather than refused. `loadParquetBytes` runs ~20 times per points load, so a value that can never be admitted becomes ~20 refetches of the file too big to fetch once. - Sizes need not be known at insertion. The decoded cache holds the in-flight promise (that is the dedup), so it enters at zero bytes and is recounted when the table lands. Arrow's `Data.byteLength` walks the child tree, so it is asked once per table and the total is incremental from there. Ceilings default to 128 MB encoded / 256 MB decoded, overridable per source via `DataSourceParams.parquetCacheLimits`. The numbers bound a leak; they are not a measured working set, which is why they are an option and not a constant. Breaking for direct readers of those two fields — `Record` index access becomes `.get()`. Nothing outside `VTableSource` touched either. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`ensureCodecWorkers()` called `enableWorkerChunkDecode()` with no options, so
`cache` was undefined and fizarrita fell back to its no-op — the seam was
exported, documented, typed end to end, and empty. There was no chunk cache at
all, so every tile paid a network round-trip and a re-decode each time it came
back into view. Filling it is a pure win: nothing to trade against a cache that
did not exist.
It now takes a `ByteLruCache`, which satisfies fizarrita's `ChunkCache`
structurally — `get` is the lookup and the recency update in one, which is
exactly what that interface asks for. Default 256 MB, overridable on the first
call; `getChunkCache()` exposes it for inspection and `clear()`.
`RasterElement.getStore()` is memoized, and that is the load-bearing half.
fizarrita keys chunks `store_N:{path}:{chunkKey}` with `N` from a `WeakMap` on
the store instance, while `createPrefixedStore` returns a fresh object literal
per call — so a view per caller would key one chunk differently per view and
fill the cache with duplicates that never hit.
Two upstream gaps left standing and documented: absent chunks are cached as
full zero-filled arrays (bounded now, but still real bytes), and the chunk path
still has no in-flight dedup.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`ByteLruCache` trusted every number handed to it. A `NaN` ceiling or a `sizeOf` that returned `NaN` made every `resident > max` comparison false, silently disabling eviction for the life of the cache — a bounded cache quietly becoming an unbounded one, which is the exact failure it exists to prevent. Both are now refused at the one place a bad number can enter, along with Infinity and negatives. Disposal errors no longer abandon the operation that triggered them. Removal and byte bookkeeping already completed before `onDispose` ran, but a throwing callback could abort an eviction pass halfway — leaving the cache over its ceiling — or skip the rest of a `clear`. Eviction and clearing now run to completion and rethrow the first error afterwards. Test-only, from the same review: the "different elements different views" case now shares one backing store, so it asserts something about the per-element view rather than about two elements holding different stores; and the `as any` casts are gone from both new specs, verified with tsc rather than assumed (the specs are outside the package tsconfig, so the suite alone would not have caught it). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
xinaesthete
force-pushed
the
claude/memory-accounting-handoff-adr-5a4377
branch
from
August 10, 2026 10:54
95fad4b to
bfbf793
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/tests/byteLruCache.spec.ts (1)
154-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a rejected-size case for an existing key.
This test covers only an empty cache. The replacement path is untested: when
sizeOfthrows for a key that is already resident,setcurrently drops the previous entry without disposal. See the finding onpackages/core/src/memory/byteLruCache.tsLine 167-176 for the root cause.🧪 Proposed test
it('refuses a size that would corrupt the running total', () => { const cache = new ByteLruCache<string>({ maxBytes: 100, sizeOf: () => Number.NaN }); expect(() => cache.set('a', 'x')).toThrow(RangeError); expect(cache.byteLength).toBe(0); }); + + it('keeps the previous entry when a replacement size is refused', () => { + let bytes = 10; + const onDispose = vi.fn(); + const cache = new ByteLruCache<string>({ maxBytes: 100, sizeOf: () => bytes, onDispose }); + cache.set('a', 'first'); + + bytes = Number.NaN; + expect(() => cache.set('a', 'second')).toThrow(RangeError); + + expect(cache.peek('a')).toBe('first'); + expect(cache.byteLength).toBe(10); + expect(onDispose).not.toHaveBeenCalled(); + });🤖 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 `@packages/core/tests/byteLruCache.spec.ts` around lines 154 - 159, Add a test alongside the existing rejected-size case that first inserts a key, then makes its replacement `sizeOf` throw or return an invalid size, and verifies `ByteLruCache.set` preserves the resident entry, byteLength, and disposal behavior. Anchor the test to the existing `ByteLruCache` test cases and ensure it specifically exercises replacement of an already-present key.
🤖 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 `@packages/core/src/memory/byteLruCache.ts`:
- Around line 167-176: Validate the replacement value with this.measure(value)
before mutating state in ByteLruCache.set, then remove the previous entry and
adjust residentBytes only after measurement succeeds. In
packages/core/src/memory/byteLruCache.ts lines 167-176, preserve the existing
entry and avoid dispose calls when measurement throws. In
packages/core/tests/byteLruCache.spec.ts lines 154-159, add coverage for a
resident key whose sizeOf throws, asserting the old value remains cached and
onDispose is not called.
---
Nitpick comments:
In `@packages/core/tests/byteLruCache.spec.ts`:
- Around line 154-159: Add a test alongside the existing rejected-size case that
first inserts a key, then makes its replacement `sizeOf` throw or return an
invalid size, and verifies `ByteLruCache.set` preserves the resident entry,
byteLength, and disposal behavior. Anchor the test to the existing
`ByteLruCache` test cases and ensure it specifically exercises replacement of an
already-present key.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 858699d7-7707-406d-a9a9-0c7d6bef11bd
📒 Files selected for processing (5)
packages/core/src/memory/byteLruCache.tspackages/core/tests/byteLruCache.spec.tspackages/core/tests/rasterElementStore.spec.tspackages/vis/src/codecWorkers.tspackages/vis/tests/codecWorkers.spec.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/tests/rasterElementStore.spec.ts
- packages/vis/tests/codecWorkers.spec.ts
- packages/vis/src/codecWorkers.ts
Comment on lines
+167
to
+176
| set(key: string, value: V): void { | ||
| const previous = this.entries.get(key); | ||
| if (previous) { | ||
| this.entries.delete(key); | ||
| this.residentBytes -= previous.bytes; | ||
| } | ||
| const bytes = this.measure(value); | ||
| this.entries.set(key, { value, bytes }); | ||
| this.residentBytes += bytes; | ||
| const disposeError = previous ? this.disposeQuietly(previous.value, key) : undefined; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
set mutates cache state before it validates the new size. measure runs after the previous entry is deleted and its bytes are subtracted, so a rejected size drops the displaced value without calling onDispose. The test suite covers the rejected-size case only on an empty cache, so the replacement path is not exercised.
packages/core/src/memory/byteLruCache.ts#L167-L176: callthis.measure(value)first, then delete the previous entry and adjustresidentBytes.packages/core/tests/byteLruCache.spec.ts#L154-L159: add a case wheresizeOfthrows for an already-resident key, and assert that the previous value stays resident andonDisposeis not called.
📍 Affects 2 files
packages/core/src/memory/byteLruCache.ts#L167-L176(this comment)packages/core/tests/byteLruCache.spec.ts#L154-L159
🤖 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 `@packages/core/src/memory/byteLruCache.ts` around lines 167 - 176, Validate
the replacement value with this.measure(value) before mutating state in
ByteLruCache.set, then remove the previous entry and adjust residentBytes only
after measurement succeeds. In packages/core/src/memory/byteLruCache.ts lines
167-176, preserve the existing entry and avoid dispose calls when measurement
throws. In packages/core/tests/byteLruCache.spec.ts lines 154-159, add coverage
for a resident key whose sizeOf throws, asserting the old value remains cached
and onDispose is not called.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements rungs 1–3 of ADR 0005. Rungs 4–5 are deliberately not built — the ADR defers them until measurement justifies them, and this leaves that intact.
The repo had a memory policy and no memory accounting:
DEFAULT_POINTS_MEMORY_CAPis a row count applied to one element kind, and nothing anywhere could answer "how many bytes are resident?". These four commits fix the three things that are broken today and stop there.Each commit stands alone and is separately reviewable.
06dd876— rung 2b: rejected parquet promises evict themselvesparquetTableCachecaches the table promise before it settles — genuine in-flight dedup — but never cleaned up a rejection, so one transient fetch failure pinned a rejected promise at that path for the lifetime of the source, and every later read replayed a network error that had cleared long ago. The only recovery was a new source.Reproduced first, then fixed with the
evictIfCurrentdiscipline already used by the dataset-metadata and part-path caches. The rejection still propagates unchanged to the caller that provoked it, so the deliberate skip-vs-fail policy indocs/plans/parquet-io-error-handling.mdis untouched — it just stops being the answer for the next caller. Tests cover the retry plus the two behaviours that had to survive it: concurrent dedup, and successful tables staying cached.7d8592f— rung 1: theMemoryReportingscalar{ readonly byteLength: number }. No policy, no eviction, no tiers.The name is the design:
byteLengthis whatTypedArray,ArrayBufferandDataViewalready call this, so every payload we hold satisfies it structurally — no wrapper, no import. That is what makes it cheap enough to put on every cache rather than a chosen few. Implementors take on one obligation: keep the number cheap to read (running total on insert/evict, never a scan per read).5118184— rung 2: both parquet tiers boundedparquetTableBytes(compressed bytes) andparquetTableCache(decoded Arrow) were plainRecords with no eviction of any kind. A source held both tiers of every parquet file any caller had ever touched, simultaneously, until it was discarded — double memory for zero eviction benefit. Fixing a leak, not building an architecture.Adds
ByteLruCache(framework-free, byte-bounded,MemoryReporting, dispose hook) and puts both tiers behind it. Two semantics the tests pin:loadParquetBytesruns ~20 times per points load, so a file that can never be admitted becomes ~20 refetches of the file that was already too big to fetch once.Data.byteLengthwalks the whole child tree, so it is asked exactly once per table.Ceilings default to 128 MB encoded / 256 MB decoded, overridable per source via
DataSourceParams.parquetCacheLimits. The numbers bound a leak; they are not a measured working set, which is why they are an option rather than a constant you would have to fork to change.Breaking for direct readers of those two public fields —
source.parquetTableBytes[path]becomes.get(path). Nothing outsideVTableSourcetouched either.7f3c3d9— rung 3: the chunk-cache seam, filledfizarrita has always accepted a
{ get, set }cache andzarrextrahas always plumbed it through, butensureCodecWorkers()calledenableWorkerChunkDecode()with no options — socachewasundefinedand fizarrita fell back to its no-op. There was no chunk cache at all, not an undersized one. Every tile paid a network round-trip and a re-decode on every pan back over ground already covered. Pure win: nothing to trade against a cache that did not exist.ByteLruCachesatisfies fizarrita'sChunkCachestructurally, so it is passed as-is — itsgetis the lookup and the recency update, exactly what that interface wants. Default 256 MB, overridable on the first call;getChunkCache()exposes it for inspection andclear().RasterElement.getStore()is now memoized, and that is load-bearing rather than tidiness: fizarrita keys chunksstore_N:{path}:{chunkKey}withNfrom aWeakMapon the store instance, whilecreatePrefixedStorereturns a fresh object literal per call. A view per caller would key one chunk differently per view and fill the cache with duplicates that never hit.Two upstream gaps left standing, and documented at the call site rather than silently inherited: absent chunks are cached as full zero-filled arrays (bounded now, but still real bytes), and the chunk path still has no in-flight dedup. Both are filed upstream.
Verification
biome ciandlint:reactclean. All packages build.store_0:/0:0/0/0. Before this branch there was no cache object to inspect at all.One thing I could not demonstrate in-app: the duplicate-key counterfactual for the
getStore()memoization. Removing the memoization and toggling the layer still produced a singlestore_0entry, because the app caches the image loader sogetStore()is not called again on a toggle. The memoization is correct and is what the ADR asks for, but the only direct evidence is the unit test that it returns a stable instance — there is no in-app repro of the failure it prevents.Not in scope
Rungs 4 (encoded tier / evict-decoded-keep-encoded) and 5 (tiered
ResidencyReport, global ceiling, degrade-to-fit) are untouched by design. The ADR's own rationale —tgpu-htj2kbuilt and unit-tested a budget solver it then never called in production — is the reason.The ADR still reads
Status: proposed. Rungs 1–3 landing does not by itself settle 4–5, so that felt like a call to make separately.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes