Skip to content

feat: keep LSD caches warm across hot reloads with content-versioned preview hashes - #9819

Merged
dalkia merged 21 commits into
devfrom
feat/lsd-content-versioned-reload
Aug 31, 2026
Merged

feat: keep LSD caches warm across hot reloads with content-versioned preview hashes#9819
dalkia merged 21 commits into
devfrom
feat/lsd-content-versioned-reload

Conversation

@dalkia

@dalkia dalkia commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

What

Local Scene Development (LSD) hot reloads now only reload what actually changed. Three pieces:

  1. Skip the cache drain when the dev server versions its preview hashes by file mtime. An edited file arrives under a new hash and reloads through a natural cache miss, while every unchanged asset keeps its hash and stays warm.
  2. Re-enable the texture memory cache in LSD. The NoCache workaround from fix: Disable disk cache on LSD #3406 is removed — textures are cached, deduplicated, and refcount-managed like in every other launch mode. The disk cache stays disabled for localhost (unchanged), and the typed disk-cache layer no longer serializes when the underlying store discards writes.
  3. Revalidate cached raw-GLTF imports against the current content mapping. A GLB referencing an external texture keeps its own hash — the cache key — when only the texture changes, so both GLTF cache layers would serve the stale import forever. Each import now records the external files it fetched and the URL each resolved to; on a cache hit in LSD those are re-checked against the current mapping (in-memory string compares, no network) and stale entries are evicted from both layers so only the affected GLBs re-import.

Why

Draining + re-downloading/re-importing every asset is the slowest part of a hot reload. Until now the dev server derived hashes from the file path, so an edited file kept the same hash — cache hits would return stale assets, forcing a full drain (and no texture caching at all) to stay correct. With content-versioned hashes that constraint is gone: invalidation is declarative (changed file → new key → natural miss), the same content-addressing contract production catalysts already provide.

The texture NoCache was also actively harmful: every consumer of the same URL downloaded and decoded its own Texture2D, and nothing ever destroyed them. A memory snapshot of one editor LSD session showed ~600 MB of duplicated textures (21 live copies of a single 8 MB atlas) accumulating until app exit.

Depends on the dev-server side: decentraland/js-sdk-toolchain#1529 (embeds mtime in each per-file preview hash as b64-<base64(path\0mtimeMs-machineId)>).

How

  • ECSReloadScene.IsContentVersioned(definition) inspects the first content entry's hash, base64-decodes the b64- payload, and returns true iff it contains a NUL byte — the unambiguous marker of the versioned format ({path}\0{mtimeMs}-{machineId}). The dev server hashes every file the same way, so the first entry decides for the whole scene.
  • On reload: versioned → no eviction at all (caches self-invalidate per changed hash); otherwise fall back to the existing behavior (scoped GLTF eviction by src, else full drain).
  • TexturesLoadingPlugin always uses the real TexturesCache registered with the CacheCleaner. Old path-only servers stay fresh through the existing full drain on reload (fix: raw GLTF memory problems #8419), which now covers the textures cache too.
  • IDiskCache gains a typed PutAsync(data, serializer): serialization is owned by the concrete store, so the LSD Fake (which discards writes) never invokes the serializer — previously every cache hit's re-put read the texture's pixels and threw is not readable once glTFast dropped the CPU mirror.
  • GltFastSceneDownloadProvider records (file → resolved content URL) for every external file an import fetches; the list is stored on GLTFData. PrepareGltfAssetLoadingSystem re-resolves those on a container-cache hit in LSD and, on mismatch, evicts the popped instance, its pool, and the import layer (GltfLoadCache.RemoveByHash) — mirroring CacheCleaner.EvictGltfModel. The import eviction is required so the re-import can occupy the same (Name, Hash) cache key.

Compatibility

  • Backward compatible / ships independently. Against today's path-only dev server, IsContentVersioned returns false and the current drain/scoped behavior is unchanged (now including textures). It lights up automatically once #1529 ships — no lockstep release.
  • Broader than the old scoped path. Covers every file type (textures, .crdt, --local-ab bundles), not just raw GLTF, since all those caches key off the now-versioned content hash.
  • Production paths untouched. GLTF dependency validation is gated to LSD; global/realm content is content-addressed and immutable, so no revalidation runs there. The real disk store serializes exactly as before.

Testing

Unit tests

  • ECSReloadSceneShould: versioned hash (has NUL) → true; path-only hash → false; null/empty content and non-b64- (production CID) → false.
  • PrepareGltfAssetLoadingSystemShould: stale dependency → evicts both cache layers and falls through to a fresh raw-GLTF load; unchanged dependencies → served from cache with no eviction.

Manual test plan (Genesis Plaza central-plaza + the #1529 dev server)

Environment setup (once)

  1. Explorer — download this PR's build for your platform from the CI artifacts.

  2. Test scene — clone Genesis-Plaza-2025; the tests run against central-plaza.

  3. Content-versioned dev server — install the feat: content-versioned preview ids for local scene development js-sdk-toolchain#1529 prerelease of @dcl/sdk-commands into the scene (grab the current .tgz URL from the auto-generated "Test this pull request" comment on that PR; example at time of writing):

    cd Genesis-Plaza-2025/central-plaza
    npm install
    npm install "@dcl/sdk@latest" "@dcl/sdk-commands@latest"
    npm run start   # dev server on http://127.0.0.1:8000

    Sanity-check the server is minting versioned hashes (they contain a NUL between path and mtime — any content[].hash should change after touching its file, while the entity id stays stable):

    curl -s -X POST http://127.0.0.1:8000/content/entities/active -d '{"pointers":["0,0"]}' -H 'Content-Type: application/json'
  4. Launch the build in Local Scene Development mode against the preview server.

    macOS:

    open Decentraland.app --args --realm http://127.0.0.1:8000 --local-scene true

    Windows (cmd/PowerShell):

    .\Decentraland.exe --realm http://127.0.0.1:8000 --local-scene true

    Both --realm and --local-scene true are required (--local-scene without true is ignored). Optionally add --position "x,y" to spawn on a specific parcel. Once in, head to the clock tower / pool / fishing game — all test targets are there.

  5. Create the solid-magenta paint stub (a 64×64 PNG, embedded here as base64):

base64 --decode > ~/magenta.png <<< 'iVBORw0KGgoAAAANSUhEUgAAAEAAAABACAIAAAAlC+aJAAAAdklEQVR4nO3PQQkAMAzAwPo33Yno4xgEIuAyO/t1wwUNaEEDWtCAFjSgBQ1oQQNa0IAWNKAFDWhBA1rQgBY0oAUNaEEDWtCAFjSgBQ1oQQNa0IAWNKAFDWhBA1rQgBY0oAUNaEEDWtCAFjSgBQ1oQQNa0IAWHHtBp+HSthEAJgAAAABJRU5ErkJggg=='

⚠️ Edit the files the scene actually serves. Genesis Plaza consolidates assets into the generated central-plaza/assets/models/pool/ folder, which shadows the source folders (images/…, assets/scene/…) — editing a source copy changes nothing visible. When in doubt, find which GLB references a texture with strings model.glb | grep uri, or check the decoded content URL in the explorer log.

All texture/model steps below run from:

cd Genesis-Plaza-2025/central-plaza/assets/models/pool

1. GLB-referenced texture change (the new dependency-revalidation path — these textures are external URIs of clocktower_main.glb):

cp stylizedBricks_v03_basecolor.png stylizedBricks_v03_basecolor.png.bak
cp clockTower_wall_01_basecolor.png clockTower_wall_01_basecolor.png.bak
cp ~/magenta.png stylizedBricks_v03_basecolor.png
cp ~/magenta.png clockTower_wall_01_basecolor.png

✅ On the reload the clock tower's brick and wall surfaces turn magenta; only the clocktower GLBs re-import, everything else stays warm.

Rollback (itself a positive test — the original art must come back on the next reload):

mv stylizedBricks_v03_basecolor.png.bak stylizedBricks_v03_basecolor.png
mv clockTower_wall_01_basecolor.png.bak clockTower_wall_01_basecolor.png

2. Code-only change — add as the first line of main() in central-plaza/src/index.ts:

console.log('[reload-test] code-only change, assets should stay warm')

✅ Fast reload: the log appears in the console; zero [TextureSystem] processing request lines and zero GLTF re-imports — everything served from cache. This is the headline speedup.

Rollback: delete the console.log line and save (one more warm reload).

3. Standalone (non-GLB) texture — UI textures loaded directly by scene code:

cp fish_game_atlas_1024.png fish_game_atlas_1024.png.bak
cp ~/magenta.png fish_game_atlas_1024.png

✅ Open the fishing game: its UI sprites turn magenta(-tinted — UI multiplies the texture by each element's color, so shades of magenta are the pass signal). Exactly one texture request for the changed file regardless of how many elements consume it (dedup check), and no GLB re-imports.

Rollback:

mv fish_game_atlas_1024.png.bak fish_game_atlas_1024.png

4. Model change — swap two models that are easy to spot (street benches and bush pots are all over the plaza, and both GLBs sit at the root of the pool folder):

cp BenchStreet.glb BenchStreet.glb.bak
cp BushPot.glb BenchStreet.glb

✅ Every street bench renders as a bush pot after the reload; only that model re-imports, everything else stays warm.

Rollback:

mv BenchStreet.glb.bak BenchStreet.glb

5. No error spam — throughout all reloads: ✅ zero Error putting cache content: Texture 'image_N' is not readable.

6. Memory stays flat-ish — after repeated reloads, the debug panel's Textures cache count/size should return to roughly its baseline instead of climbing by the scene's full texture set on every reload. Some growth over a session is expected and fine (observed: ~6 GB → ~7.1 GB across a long reload session); the pass criterion is that growth is bounded and the plaza keeps reloading — before this PR, LSD leaked every downloaded texture until app exit.

7. Old-server fallback — against a path-only (pre-#1529) dev server, repeat steps 1–3: fresh content must still appear after reload (served by the full drain instead of hash misses).

🤖 Generated with Claude Code

The dev server can now embed each file's mtime in its preview hash
(b64-<base64(path\0mtimeMs-machineId)>), so an edited file arrives under a
new hash and reloads through a natural cache miss while unchanged assets keep
their hash and stay warm. Detect this per scene from the first content hash
(a NUL byte marks the versioned format) and skip eviction entirely on reload;
fall back to the existing scoped/drain behavior for path-only (older) servers.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@dalkia
dalkia requested review from a team as code owners August 20, 2026 12:44
@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

🚦 CI Status

Build

Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below.

Name Link
Commit fd52378
Logs https://github.com/decentraland/unity-explorer/actions/runs/33190397940
Download Windows https://github.com/decentraland/unity-explorer/suites/89951248081/artifacts/9694679203
Download Windows S3 https://explorer-artifacts.decentraland.org/@dcl/unity-explorer/branch/feat/lsd-content-versioned-reload/pr-25653-fd52378/Decentraland_windows64.zip
Download Mac https://github.com/decentraland/unity-explorer/suites/89951248081/artifacts/9694368067
Download Mac S3 https://explorer-artifacts.decentraland.org/@dcl/unity-explorer/branch/feat/lsd-content-versioned-reload/pr-25653-fd52378/Decentraland_macos.zip
Built on 2026-08-28T17:10:33Z

Lint

Lint did not finish (cancelled) — the warning ratchet could not be evaluated. See logs.

Tests

All Unity tests passed ✅

TESTS SUITE Result Passed Failed Skipped
EditMode ✅ Passed 25517 0 13
PlayMode ✅ Passed 248 0 37

@decentraland-bot decentraland-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review — #9819 feat: skip LSD cache drain when preview hashes are content-versioned

STEP 2 — Root-cause check

PASS. The PR addresses the actual cause: when the dev server derives hashes from the file path rather than content+mtime, an edited file keeps the same hash and cache hits return stale assets — forcing a full drain. With content-versioned hashes (mtime embedded), an edit produces a new hash, so cache self-invalidates via natural miss. The diff gates eviction on this distinction rather than working around a symptom.

STEP 3 — Design & integration

PASS. No new lifecycle units introduced. IsContentVersioned and HashIsContentVersioned are pure static methods with no persistent state, placed in ECSReloadScene — the same class that already owns the eviction decision in DisposeAndRestartAsync. This is the correct home; the eviction decision and its inputs stay together.

Owner search: ECSReloadScene is not an ECS system — it is a plain class instantiated with dependencies (IScenesCache, World, Entity, bool, ICacheCleaner). It manages the reload lifecycle for local scene development. The cache eviction logic already lived here (lines 113–129 post-change). The new methods add a detection layer before the existing eviction decision without altering ownership.

Teardown/consumption trace: No new subscriptions, event hookups, connections, buffers, or measurements. ArrayPool<byte>.Shared.Rent is properly returned in a finally block (line 205). No leaks.

STEP 4 — Member audit

  • IsContentVersioned (internal static, line 173): Used at the call site (line 112) and by 5 test assertions. internal visibility is justified for testability. Single production consumer within the class — this is fine for a predicate that gates a single decision point.
  • HashIsContentVersioned (private static, line 183): Single caller (IsContentVersioned). The split separates "which entry to check" from "how to decode and inspect a hash" — reasonable factoring for a method that does base64 decoding + byte scanning. Not a bridge/wrapper anti-pattern; each method has a distinct responsibility.

STEP 5 — Line-level findings

See inline comment below.

Checked and clean:

  • Naming: PascalCase for methods/properties, camelCase for locals — all correct. PREFIX uses SCREAMING_CASE for a const, consistent with codebase convention (DEFAULT_VERSION, PORTABLE_EXPERIENCE_MAX_VALUES, LOD_REDUCTION, etc.).
  • Nullable handling: IsContentVersioned accepts SceneEntityDefinition? and safely propagates via definition?.content. HashIsContentVersioned accepts string? and guards with IsNullOrEmpty. ContentDefinition.hash is declared non-nullable but is a DTO struct — the null guard is legitimate.
  • Performance: ArrayPool<byte>.Shared avoids heap allocation, consistent with project standards (CLAUDE.md: "Minimize GC pressure: reuse objects, use object pooling"). No LINQ. StringComparison.Ordinal used for prefix check.
  • Resource lifecycle: ArrayPool buffer returned in finally — no leak path.
  • No anti-patterns detected: No bridge/wrapper classes, no extracting-when-should-merge, no per-frame logic in presenter, no defensive null-checks against non-null declarations (DTO fields are legitimately nullable at runtime), no debug/mock code in production hot paths.
  • Security: Input validation is thorough (IsNullOrEmpty, prefix check, TryFromBase64Chars safe failure). Only runs in local dev mode (localSceneDevelopment flag). NUL-byte detection is reliable (cannot appear in file paths or hostnames). Array.IndexOf correctly bounded to written bytes — no false positives from stale pool data.

STEP 6 — Complexity

SIMPLE. Touches 2 files with ~110 lines of changes. Adds static helper methods and wraps existing eviction logic in a guard. Does not modify ECS systems, components, queries, async patterns, plugin registration, or dependency injection.

STEP 7 — QA assessment

QA_REQUIRED: YES. Modifies runtime code that affects scene reload behavior during local development. Cache eviction is skipped entirely for content-versioned dev servers — this changes observable hot-reload behavior.

STEP 8 — Warnings

None. Main scene file not modified.

Tests

Three new tests cover the key cases:

  • Versioned hash (NUL present) → true
  • Path-only hash (no NUL) → false
  • Null/empty/non-b64 (production CID) → false

Good coverage of edge cases. Test helpers (PathOnlyHash, VersionedHash) correctly construct the expected hash formats.


REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Adds static helper methods to ECSReloadScene for content-versioned hash detection and wraps existing cache eviction logic in a guard — no ECS, async, or architectural changes.
QA_REQUIRED: YES


Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub

Comment thread Explorer/Assets/DCL/Infrastructure/SceneLifeCycle/ECSReloadScene.cs Outdated
@decentraland-bot

This comment has been minimized.

LSD has used NoCache for standalone textures since #3406, when preview
hashes were path-only and any cache hit after a hot reload served stale
content. That workaround is now pure downside: every consumer of the same
texture URL downloads and decodes its own Texture2D (Genesis Plaza holds
21 live copies of one 8 MB atlas), and because NoCache retains nothing the
CacheCleaner never disposes them, so every texture ever downloaded leaks
until the app closes.

Freshness no longer needs it: content-versioned dev servers mint a new
hash for an edited file (natural cache miss), and for path-only servers
ECSReloadScene already force-drains all registered caches on reload
(#8419), which now covers the textures cache too.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@decentraland-bot

This comment has been minimized.

…ing in LSD

A GLB that references an external texture keeps serving the old texture
after a hot reload on a content-versioned dev server: the edited texture
gets a new content hash, but the GLB's own hash — the cache key for both
the import cache (GltfLoadCache) and the built-asset cache
(GltfContainerAssetsCache) — is unchanged, so both layers hit and the
texture pipeline never runs.

Fix by recording, at import time, every external file the GLB fetched
together with the content URL it resolved to (GltFastSceneDownloadProvider
already resolves each URI through the scene's content mapping). In local
scene development a cache hit in either layer is then only served while
every recorded file still resolves to the same URL; otherwise the entry is
evicted and the GLTF re-imports, fetching the edited file under its new
hash while every other asset stays warm.

Production paths are untouched: validation is gated to LSD, and the
global/realm strategies short-circuit to valid since content-addressed
hashes are immutable. Path-only (older) dev servers keep working through
ECSReloadScene's full drain on reload, as before.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@decentraland-bot

This comment has been minimized.

…etLoadingSystem

Drop the generic LoadSystemBase revalidation hook, the strategy interface
method, and the LoadGLTFSystem override. The dependency check now runs in
one place — PrepareGltfAssetLoadingSystem, which already owns the LSD cache
interaction — and on a stale hit evicts both layers (container pool and
GltfLoadCache.RemoveByHash), mirroring CacheCleaner.EvictGltfModel.

Eviction of the import layer stays: the fresh import must occupy the same
(Name, Hash) cache key the stale entry holds — Add is a TryAdd, so leaving
the stale entry would force a re-import on every subsequent request.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@dalkia dalkia changed the title feat: skip LSD cache drain when preview hashes are content-versioned feat: keep LSD caches warm across hot reloads with content-versioned preview hashes Aug 21, 2026
Co-authored-by: Muna <44584806+decentraland-bot@users.noreply.github.com>
Signed-off-by: Juan Ignacio Molteni <juanignaciomolteni@gmail.com>
@decentraland-bot

This comment has been minimized.

@decentraland-bot

This comment has been minimized.

@decentraland-bot

This comment has been minimized.

@decentraland-bot

This comment has been minimized.

@decentraland-bot

This comment has been minimized.

CacheableFlowAsync called genericCache.PutAsync for every successful
result, including ones that came FROM the cache. A memory hit is already
stored and a disk hit is promoted to memory by GenericCache.ContentAsync,
so the re-put was pure waste — and actively harmful for textures shared
with glTFast imports: DropTexturesCpuMirror makes them non-readable after
upload, so the disk serializer's GetRawTextureData throws on every
subsequent hit ("Error putting cache content: Texture 'image_N' is not
readable"), spamming the log on each LSD reload.

Only freshly downloaded results are put now.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@vercel

vercel Bot commented Aug 24, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
unity-explorer Error Error Aug 24, 2026 5:19pm

Request Review

@decentraland-bot

This comment has been minimized.

dalkia and others added 2 commits August 24, 2026 15:58
In LSD the root IDiskCache is a Fake that discards writes — but the typed
DiskCache<T> wrapper serialized the asset BEFORE delegating, so every
texture put still read the texture's pixels. For textures shared with
glTFast imports, DropTexturesCpuMirror makes them non-readable after GPU
upload, so GetRawTextureData threw "is not readable" on every cache hit's
re-put, spamming the log on each LSD reload.

Expose the store's ability to persist via IDiskCache.Enabled (default
interface member, only the Fake reports false) and early-out in the typed
wrapper before serializing. No behavior change outside LSD.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…flag

Replace IDiskCache.Enabled with a typed PutAsync(data, serializer)
overload: the real store serializes (on the caller's thread, since
serializers may touch main-thread-only Unity APIs) and streams; the Fake
never invokes the serializer, so discarded writes structurally cannot
serialize. The typed DiskCache<T> wrapper becomes a pure forward.

Same LSD outcome as before — no texture pixel reads for writes that are
thrown away — expressed through polymorphism rather than a capability
check.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@decentraland-bot

This comment has been minimized.

@decentraland-bot decentraland-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review — feat: keep LSD caches warm across hot reloads with content-versioned preview hashes

STEP 2 — Root-cause check: PASS

The PR addresses the root cause of slow LSD hot reloads: path-based hashing made cache invalidation impossible without draining everything. Content-versioned hashes make invalidation declarative (changed file → new hash → natural cache miss), and the texture NoCache workaround — which was leaking ~600 MB of duplicated textures — is correctly removed in favor of the real ref-counted cache that CacheCleaner can manage.

STEP 3 — Design & integration: PASS

Lifecycle owner search (mandatory):

New unit Entity/resource managed Existing owners found Should it live there?
IsContentVersioned (static method on ECSReloadScene) Reload-time eviction decision ECSReloadScene.DisposeAndRestartAsync — already owns the reload flow ✅ Lives in the owner
IsStaleRawGltf + eviction in PrepareGltfAssetLoadingSystem.Prepare() Per-asset GLTF dependency freshness CacheCleaner.EvictGltfModel (reload-time, hash-exact) See below
GltfExternalDependency struct Immutable import-time data carrier N/A (new data, not a lifecycle) ✅ Correct

Why the staleness check belongs in Prepare(), not in CacheCleaner:

The git history confirms the author tried and reverted a LoadSystemBase.IsCachedResultValid approach (commits 39b13e1cf → reverted in 4e821f33b) because PrepareGltfAssetLoadingSystem.Prepare() checks the container cache (IGltfContainerAssetsCache) before a GetGLTFIntention is ever created — if that check hits, LoadGLTFSystem (and therefore any IsCachedResultValid hook) never runs at all.

CacheCleaner.EvictGltfModel can't detect this staleness either: a GLTF container whose own hash is unchanged (the cache key) while only an external texture was republished under a new content hash is invisible to hash-exact eviction. Detecting it requires resolving each dependency's current URL through sceneData.SceneContent, which is scene-scoped and per-asset — only meaningful at the moment the asset is next requested.

This is not a per-frame reconciliation scan (CLAUDE.md design anti-pattern). It's a cache-hit validation that runs only when a specific entity requests a specific asset and the container cache returns a hit. No persistent state is held by the system.

Data flow: GltFastSceneDownloadProvider (records file→url during import) → GLTFData (immutable carrier, ExternalDependencies is IReadOnlyList?) → PrepareGltfAssetLoadingSystem (consumer, calls static GltfExternalDependency.AreUpToDate). Unidirectional, no back-references — correct coupling direction.

Assembly boundary: GltfLoadCache is in the same ECS.Unity assembly as PrepareGltfAssetLoadingSystem. The same instance is already wired by GltfContainerPlugin to LoadGLTFSystem and CacheCleaner.Register. Passing it to one more system is minimal, correct wiring.

TexturesLoadingPlugin simplification: The removed NoCache branch dates to 2023 (#3406). TexturesCache<TIntention> is ref-counted and CacheCleaner-registered, so LSD textures are now properly deduplicated and managed. Freshness for old path-only servers is covered by the existing full drain in ECSReloadScene (which now sweeps the texture cache too since it's registered). Consistent.

Teardown trace:

  • GltfExternalDependency list: allocated by GltFastSceneDownloadProvider (non-persistent, scoped to the import via using), stored on GLTFData (ref-counted, destroyed by DestroyObjectGltfImport.Dispose() + SafeDestroy(Root)). No leak.
  • ArrayPool<byte>.Shared buffer in HashIsContentVersioned: rented and returned in finally block. No leak.
  • Eviction path asset!.Dispose() + cache.Remove() + gltfLoadCache.RemoveByHash(): mirrors CacheCleaner.EvictGltfModel. TryGet pops the asset from the pool list before returning it, so asset!.Dispose() and cache.Remove() (which disposes remaining pooled instances) operate on disjoint sets — no double-dispose.

STEP 4 — Member audit: PASS

Member Consumers Assessment
GltfExternalDependency.AreUpToDate IsStaleRawGltf in PrepareGltfAssetLoadingSystem Single consumer, but logic is correctly co-located with the data type — encapsulates ISceneContent resolution
GLTFData.ExternalDependencies IsStaleRawGltf Appropriate public readonly field on the data class
IGLTFastDisposableDownloadProvider.ExternalDependencies LoadGLTFSystem (passes to GLTFData ctor) Appropriate interface surface
IDiskCache.PutAsync<T,Ts> DiskCache<T,Ts>.PutAsync Solves a real bug (texture serialization crash in Fake) — not YAGNI
IsStaleRawGltf (private) Prepare() Named intent, cleaner than inlining the pattern match + call

No "absent ≠ false" or "single-use re-validates" issues.

STEP 5 — Line-level findings

One P2 finding — see inline comment below.

Allocation-freedom check (hot path):

  • IsStaleRawGltf: pattern match (is GLTFData) — no allocation. AreUpToDate: plain for loop over IReadOnlyList indexer + string.Equals — no allocation. TryGetContentUrl: dictionary lookup + out param — no allocation. Gated behind options.LocalSceneDevelopment (false in production), so zero added cost outside LSD. ✅
  • HashIsContentVersioned: ArrayPool<byte>.Shared.Rent/Return — no GC allocation. Only runs once per reload, not per frame. ✅
  • Structural change ordering in Prepare(): all ref intention reads (CacheKey, Hash, Name) complete before the first World.Add structural change. ✅

Security review: No issues found.

  • ArrayPool<byte> buffer correctly bounded by written in Array.IndexOf — no over-read of stale pooled memory.
  • URL comparison uses StringComparison.OrdinalIgnoreCase between two deterministically-generated content URLs from the same ISceneContent dictionary — not a security boundary.
  • Base64 decode fails safely (TryFromBase64Chars returns false → falls back to full drain).
  • No secrets, no auth changes, no user input handling changes.
  • GltFastSceneDownloadProvider.GetDownloadUri path resolution still gated by manifest-based dictionary lookup — no new traversal/SSRF surface.

STEP 6 — Complexity: COMPLEX

Touches asset loading pipeline, cache management, GLTF import systems, disk cache interface, plugin wiring, and download provider across 16 files with significant logic changes.

STEP 7 — QA: YES

Changes affect runtime behavior — cache invalidation strategy, texture loading, GLTF import revalidation. All changes execute in the Unity player at runtime.

STEP 8 — Non-blocking warnings

⚠️ CI Lint failure is infrastructure, not code. The Lint job failed because the CI runner ran out of disk space (Docker data-root has only <30GB free). The linter never actually ran. Tests (editmode + playmode) and builds (Windows + macOS) all pass. A re-run should resolve the lint check.

REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches GLTF asset loading pipeline, cache invalidation strategy, disk cache interface, texture loading plugin, and download provider across 16 files
QA_REQUIRED: YES


Reviewed by Jarvis 🤖 · Requested by Juan Ignacio Molteni (<@U03JSUQ5Z7U>) via Slack

@pravusjif pravusjif left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

dalkia and others added 3 commits August 26, 2026 12:33
…et/Systems/PrepareGltfAssetLoadingSystem.cs

Co-authored-by: Muna <44584806+decentraland-bot@users.noreply.github.com>
Signed-off-by: Juan Ignacio Molteni <juanignaciomolteni@gmail.com>
@dalkia
dalkia enabled auto-merge (squash) August 26, 2026 17:35
@decentraland-bot

This comment has been minimized.

@decentraland-bot

This comment has been minimized.

@decentraland-bot

This comment has been minimized.

@decentraland-bot

This comment has been minimized.

Re-enabling the texture memory cache in LSD exposed a refcount-0
window: a texture enters the cache via PutAsync before its consumer's
AddReference runs, and the unbudgeted drain ECSReloadScene performs for
path-only (no-mtime) dev servers reaps every such entry, handing a
destroyed Texture2D to GLTF imports in flight across the reload
(EnsureTexture2D -> NRE via Unity fake-null).

- Move the b64 hash classification out of ECSReloadScene into
  LocalSceneDevHashes (DCL.Ipfs), adding IsPathOnly alongside
  IsContentVersioned.
- TexturesLoadingPlugin now decides per scene world at InjectToWorld:
  path-only LSD scenes get NoCache (the pre-existing behavior for old
  dev servers), while versioned scenes, production scenes and the
  global world keep the shared TexturesCache and its dedup win.
- Remove the EnsureTexture2D debug log whose `Asset.Texture?.name`
  turned the residual race into a hard crash (`?.` does not guard
  against Unity fake-null).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@decentraland-bot

This comment has been minimized.

dalkia and others added 2 commits August 28, 2026 11:04
Follow-up to aac733f: restore the ILaunchMode guard so hash shape
alone can never opt a world out of caching outside local scene
development, and keep the shared TexturesCache always created and
registered in the ctor — the global world consumes it unconditionally,
while path-only LSD scene worlds get a per-world NoCache at inject
time.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
NoCache inherited the interface's no-op AddReference while consumers
Dereference directly on the asset, driving every NoCache-served
texture's count negative on release and spamming the negative-count
guard. Expose AddReference through IStreamableRefCountData (explicit
implementation keeps the internal method as the only direct entry
point) and have NoCache forward it to ref-counted assets.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@decentraland-bot

This comment has been minimized.

@decentraland-bot

Copy link
Copy Markdown
Contributor

PR #9819, run #33193518181

Overall: ✅ no significant changes

Builds: Windows change, Windows baseline, macOS change, macOS baseline

How to read this table
  • Each build is measured 3 times, interleaved with the other build (change, baseline, change, baseline, ...) in the same session, so both see the same world content and machine state. The values are the median, and (min–max) is the lowest and highest of those runs.
  • Δ is Change minus Baseline (a negative Δ means Change is faster).
  • 🟢 faster / 🔴 slower — a difference that passed every check: the runs are fully separated (every run of one build faster than every run of the other), and the median difference is at least 3% and at least 0.5 ms.
  • ⚪ within noise — the builds' runs overlap, or the difference is tiny; it cannot be told apart from random variation. Treat it as no change.
  • — informational — the 0.1% worst metrics average only the few worst frames of a run, so a single OS hiccup swings them by a lot; they are shown for context and never earn a verdict.
  • ⚠️ no verdict — the two builds' sessions were not comparable (very different sample counts, or too few usable runs), so no conclusion is drawn from them.
  • Exceptions per run — the average number of exceptions in a run's log, not counting teardown ones logged while the app quits. Flagged only on a difference of at least 2 per run and 1.5× the other build; exception kinds the baseline never threw are called out under the table. The Exception breakdown groups all of them by the explorer's report category and exception type (as totals across the runs).
  • A run that logged unusually many exceptions (at least 10 and 5× the median of its build's runs — e.g. a service was down during it) is excluded from all numbers and called out under the table.
  • The Overall line at the top only reacts to a metric that moved on two or more machines, or by 10% or more on one — a single modest 🟢/🔴 cell can still be a statistical fluke.

Intel Core i5

Metric Baseline Change Δ Result
Samples 2399 (×3) 2341 (×3)
CPU average 37.4 ms (36.0–37.5) 38.3 ms (37.6–38.3) 0.9 ms ⚪ within noise
CPU 1% worst 312.9 ms (300.7–326.1) 316.0 ms (286.1–361.6) 3.1 ms ⚪ within noise
CPU 0.1% worst 336.8 ms (319.8–344.4) 344.9 ms (295.6–421.9) 8.0 ms — informational
GPU average 22.8 ms (21.9–23.1) 23.8 ms (22.9–23.9) 1.0 ms ⚪ within noise
GPU 1% worst 307.1 ms (291.3–332.2) 322.1 ms (292.5–368.7) 15.0 ms ⚪ within noise
GPU 0.1% worst 343.4 ms (327.4–349.9) 348.4 ms (301.1–423.7) 5.0 ms — informational
Exceptions per run 0 0 0 ⚪ no significant change

Apple M1

Metric Baseline Change Δ Result
Samples 3358 (×3) 3341 (×3)
CPU average 26.6 ms (26.5–27.1) 26.8 ms (25.2–27.0) 0.2 ms ⚪ within noise
CPU 1% worst 237.8 ms (237.7–238.1) 236.3 ms (234.6–238.6) -1.6 ms ⚪ within noise
CPU 0.1% worst 245.0 ms (244.7–247.4) 244.5 ms (243.9–249.5) -0.4 ms — informational
GPU average 19.4 ms (19.0–19.9) 19.1 ms (18.4–21.0) -0.3 ms ⚪ within noise
GPU 1% worst 51.1 ms (46.6–56.0) 49.7 ms (48.2–57.1) -1.4 ms ⚪ within noise
GPU 0.1% worst 57.5 ms (54.7–62.5) 56.0 ms (55.5–62.3) -1.5 ms — informational
Exceptions per run 0 0 0 ⚪ no significant change

@DafGreco DafGreco left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✔️ PR reviewed and approved by QA on both platforms following instructions playing both happy and un-happy path

Regressions for this ticket had been performed in order to verify that the normal flow is working as expected:

  • [✔️ ] Backpack and wearables in world
  • [ ✔️] Emotes in world and in backpack
  • [ ✔️] Teleport with map/coordinates/Jump In
  • [ ✔️] Chat and multiplayer
  • [✔️ ] Profile card

No new issues were detected

@dalkia
dalkia merged commit 32958b1 into dev Aug 31, 2026
29 of 31 checks passed
@dalkia
dalkia deleted the feat/lsd-content-versioned-reload branch August 31, 2026 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants