fix(bindx): evict has-many item handles for ids that left the list - #79
Closed
matej21 wants to merge 1 commit into
Closed
fix(bindx): evict has-many item handles for ids that left the list#79matej21 wants to merge 1 commit into
matej21 wants to merge 1 commit into
Conversation
HasManyListHandle cached an EntityHandle and a proxy per item id and released neither: the whole file had no delete, no clear and no dispose. A paginated, filtered or repeatedly refetched has-many therefore grew for as long as the parent handle lived. The items getter now prunes keys missing from the presented id list, before the handles are resolved, so a still-listed item is never touched and keeps its identity by construction. Two guards go with it. Pruning is skipped while the parent persists, because the presented list hides planned additions and pruning against it would evict a just-added item and re-mint it seconds later. And lookups canonicalise a temp id through its persisted id, so a rekeyed item reuses one handle instead of minting a duplicate for the same entity. A rekeyed entry is evicted rather than migrated. EntityHandle.id returns the id the handle was constructed with, so a carried-over handle would keep reporting a dead temp id while its own $fields.id.value reported the real one. itemHandleCacheRaw goes in the same pass: it was written and never read anywhere in the repo. The proxy is the reference that keeps the handle alive, so caching the raw handle separately bought nothing.
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
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.
The leak
HasManyListHandlecached anEntityHandleand a proxy per item id:Across all 544 lines of that file there was no
.delete(), no.clear()and no dispose. Every id the relation had ever shown stayed cached for as long as the parent handle lived. A paginated, filtered or repeatedly refetched has-many inside one mounted entity grows without bound.The fix
The
itemsgetter prunes keys that are not in the presented id list, before the handles are resolved — so the pass provably never touches a key that is about to be reused, and a still-listed item keeps the identical proxy (and with it the identical handle). Identity stability is the entire reason the cache exists; a fix that churned it would be worse than the leak.Two guards go with it:
getPresentationHasManyOrderedIdsreturns onlyserverIdsduring a pessimistic in-flight persist, i.e. it hides planned additions. Pruning against that list would evict a just-added item's handle and re-mint it when the persist settles.resolveItemKeymaps a temp id throughstore.getPersistedId, so a lookup by a now-dead temp id reuses the persisted item's handle instead of minting a second handle for the same entity.Why a rekeyed entry is evicted, not migrated
The obvious approach — migrate the cache entry from
__temp_xto the persisted key so identity survives the persist — is wrong.EntityHandle.id(EntityHandle.ts:130) returnsthis.entityId, the id the handle was constructed with. The store redirects the data reads, but not the handle's ownid/[FIELD_REF_META].entityId. A carried-over handle would report a dead temp id while its own$fields.id.valuereported the real one.So the temp key is treated as dead and evicted, and the lookup is canonicalised instead. Identity across the persist boundary still changes exactly once — unchanged from today's behaviour, where the rekeyed list already mints a fresh handle.
itemHandleCacheRawis removedWhile working on this it turned out the raw map is written and never read —
grepover the whole repo finds the declaration and a.set, and no.getanywhere. The proxy is what holds the handle alive, so caching the raw handle separately bought nothing. Removing it halves what the leak was retaining.Testing
10 new tests in
tests/unit/handles/hasManyItemCache.test.ts, covering eviction, identity stability, leave-and-re-enter, temp-id rekey, and the persist guard.Negative control, re-run after the final refactor rather than carried over: commenting out the single
syncItemHandleCachecall fails 6 of the 10, on cache occupancy (e.g.stays bounded across many pagesexpects 2, gets 40). The other 4 are guard tests that cannot fail from absent eviction — they exist to catch a too-eager eviction, which is the opposite failure mode.Gates:
bun test tests/unit/handles/243 pass / 0 fail;bun test tests/react/relations/hasMany/34 pass / 0 fail; typecheck clean.Not done
The file is 634 lines against the repo's ~300 guideline (it was already 544 before this). Extracting the cache into a
HasManyItemCachemodule would converge it withbindx-react'sItemAccessorCache, which solves the same problem at the hook layer — worth a follow-up, but it is a refactor of its own.