fix(core): release shader value cache & subtexture listeners on destroy#56
Merged
Conversation
…troy Two lifecycle leaks that accumulate over a long-lived session (embedded device, page never reloaded) as nodes/textures are created and destroyed during navigation: 1. Shader value cache never released on node destroy. CoreShaderManager.valuesCache/valuesCacheUsage are keyed by resolved shader props + node size. Usage is incremented when values are computed and only decremented in the shader node's update() when the key changes, so the idle cleanup() (which evicts entries at usage <= 0) could never reclaim a destroyed node's last entry — it stayed pinned at usage >= 1 forever. This affects nearly every built-in shader (RoundedRectangle, Border, Shadow, gradients, ...). Fix: move the held `valueKey` onto the base CoreShaderNode and add detachNode(), which decrements its usage and detaches from the node. CoreNode.destroy() calls it for non-default (per-node) shaders. 2. SubTexture never unsubscribed from its parent atlas. SubTexture attaches 4 listeners (loading/loaded/failed/freed) to the parent texture, which is typically a long-lived shared atlas (preventCleanup), but had no destroy() override — so every destroyed SubTexture (and everything its handlers captured) was retained by the parent's listener lists. Fix: SubTexture.destroy() off()s the four handlers; Texture.destroy() now also removeAllListeners() defensively. Found via a memory-leak audit prompted by the SDF text-cache fix (#55). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
chiefcll
added a commit
that referenced
this pull request
Jun 9, 2026
…place (#87) * fix(textures): reversibly free textures on cleanup so they reload in-place LRU/idle texture cleanup destroyed textures (Texture.destroy() -> removeAllListeners() + cache eviction) instead of reversibly freeing them. A CoreNode that kept its texture reference would lose its 'loaded'/'freed' subscription, so when it scrolled back into the viewport the texture reloaded all the way to 'loaded' but the node was never re-notified -- leaving the node blank ("texture is loaded but nothing is displayed"). The latent design issue (cleanup using the terminal destroy path) dates to the initial release, but it only became an observable regression in #56, which added removeAllListeners() to Texture.destroy() -- correct for genuine teardown, but it severed the last surviving link for nodes still holding the reference. Fix: add TextureMemoryManager.freeTexture(), a reversible reclamation that releases the GPU resource and transitions the source to 'freed' while keeping the Texture object, its listeners, and its cache entry intact. cleanup() now uses freeTexture() instead of destroyTexture(); the 'freed' -> reload machinery already existed and now correctly re-notifies the node. Frees are kept in cache so re-requests reuse them (and avoid duplicate downloads). destroyTexture() is retained for genuine teardown. - Unit tests: cleanup takes the free() path (not destroy()) and reclaims memory. - Visual regression: texture-free-reload exercises load -> offscreen -> forced cleanup -> back onscreen -> assert reload + re-display. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * add missing vrt --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Background
While fixing the unbounded SDF text layout cache (#55), I audited the codebase for the same class of leak — things that grow over a long-lived session (embedded/set-top, page never reloaded) as nodes and textures are created/destroyed during navigation. This PR fixes the two HIGH-severity leaks found that occur per-node during normal navigation. (Two MEDIUM findings —
colorCacheandnormalizedMetrics— are noted below as follow-ups.)Leak 1 — shader value cache never released on node destroy
CoreShaderManager.valuesCache/valuesCacheUsageare keyed by resolved shader props + node width/height. Usage is incremented when values are first computed and only decremented in the shader node'supdate()when the key changes. The idlecleanup()only evicts entries at usage<= 0, so a destroyed node's last value-cache entry stayed pinned at usage>= 1forever and could never be reclaimed.This affects nearly every built-in shader that defines an
update(RoundedRectangle, Border, Shadow, the gradients, HolePunch, RadialProgress…), so any shader-styled node leaks an entry on destroy.Fix: move the held
valueKeyonto the baseCoreShaderNodeand adddetachNode(), which decrements that key's usage and detaches from the node.CoreNode.destroy()calls it for non-default (per-node) shaders — the shared default shader is skipped. This mirrors the existingprevKeyrelease done on every key change inupdate().Leak 2 — SubTexture never unsubscribed from its parent atlas
SubTextureattaches 4 listeners (loading/loaded/failed/freed) to its parent texture, which is typically a long-lived shared atlas (preventCleanup). It had nodestroy()override, so every destroyed SubTexture — and everything its handlers captured — was retained by the parent's listener lists for the rest of the session.Fix:
SubTexture.destroy()nowoff()s the four handlers beforesuper.destroy();Texture.destroy()additionally callsremoveAllListeners()defensively so any texture destroyed while something still holds a listener doesn't retain it.Testing
pnpm buildclean; full unit suite green; lint clean (lint-staged on commit).CoreShaderNode.test.ts—detachNode()decrements the held key, clears it, is a no-op when no key is held, and doesn't double-decrement.SubTexture.test.ts—destroy()detaches all four parent listeners.Follow-ups (not in this PR)
colorCache(src/core/lib/colorCache.ts) — unboundedMap<number,string>, Canvas2D path only. Bound with the same idle-LRU pattern as feat(text): bound SDF text layout cache with idle LRU eviction #55 if the Canvas backend is used.normalizedMetrics(font handlers) — not cleared onunloadFont; bounded for a fixed font set but grows with fractional/scaled sizes.🤖 Generated with Claude Code