decentraland / decentraland/unity-explorer

[TECH DEBT] StreamableLoading | Refcount-0 window lets cache eviction destroy assets handed to in-flight loads

Open
#9,900 1 comment 0 reactions 1 assignee Claimed by @dalkia View on GitHub
1-high bug tech debt
Dominant language
C#
Stars
23
Forks
17
Avg merge
2d 16h
Merged PRs (30d)
101

Description

### ๐Ÿ“Š **Priority Level:**
Medium

### ๐Ÿ“ **Area/Component:**
`ECS.StreamableLoading` โ€” `LoadSystemBase.CacheableFlowAsync` + `RefCountStreamableCacheBase` (affects every registered ref-counted cache: textures, GLTFs, asset bundles, audio clipsโ€ฆ)

### ๐Ÿ“ **Description:**
There is a window between an asset entering a memory cache and its first consumer taking a reference, during which the asset is a legitimate eviction target even though a load flow is about to hand it to a consumer. An eviction landing inside that window destroys the underlying Unity object, and the in-flight flow then returns a destroyed asset (Unity fake-null) to its consumer.

### ๐Ÿ” **Current State:**
In `LoadSystemBase.CacheableFlowAsync`:
1. A successful download is added to the memory cache via `PutAsync` with **refcount 0**.
2. Only on a later system tick does `ApplyLoadedResult` run `cache.AddReference` for the consumer.

Between (1) and (2), `CanBeDisposed()` is true. Worse, `StreamableRefCountData.LastUsedFrame` is only stamped by `AddReference`/`Dereference`, so a fresh un-referenced entry has `LastUsedFrame = 0` and `RefCountStreamableCacheBase.Unload` sorts it as the **oldest** entry โ€” freshly downloaded assets are the *first* eviction candidates during budgeted memory-pressure unloads.

This surfaced as a hard crash in local scene development (path-only hashes), where `ECSReloadScene` runs an unbudgeted `UnloadCache(budgeted: false)` on every reload and reaps every refcount-0 entry, corpsing textures for GLTF imports in flight across the reload (`GltFastDownloadProviderBase.RequestTextureAsync` โ†’ `TextureData.EnsureTexture2D` โ†’ NRE). That specific vector is mitigated by keeping path-only LSD scenes on `NoCache`, but the underlying window still exists everywhere the budgeted cleaner runs โ€” production included; it is just rare there.

### ๐Ÿ’ก **Proposed Solution:**
Options, roughly in order of preference:
1. Hand the consumer a pre-acquired reference: `AddReference` inside `CacheableFlowAsync` before publishing the result, and have `ApplyLoadedResult` adopt (not re-add) it, dereferencing on abandonment โ€” closes the window structurally.
2. At minimum, stamp `LastUsedFrame` in `RefCountStreamableCacheBase.Add` so fresh entries are not the top eviction candidates (narrows but does not close the window for unbudgeted drains).

### โš–๏ธ **Impact Assessment:**
#### Performance Impact:
None expected; refcount bookkeeping only.

#### Maintainability Impact:
Clarifies asset ownership during the load flow; removes a class of hard-to-reproduce fake-null crashes.

#### Risk of Refactoring:
`LoadSystemBase` is shared by all streamable loaders, so an ownership change touches every asset type. Needs care around abandonment paths (`DisposeAbandonedResult`) and `OngoingRequests` dedup to avoid leaks or double-dereference.

### ๐Ÿ“ˆ **Effort Estimate:**
M

### ๐Ÿ”— **Dependencies:**
Branch `feat/lsd-content-versioned-reload` contains the LSD-specific mitigation (path-only scenes use `NoCache`; `TextureData.EnsureTexture2D` fake-null log removed).

### ๐Ÿ–ฅ๏ธ **Additional Notes:**
Repro evidence: LSD reload of a path-only (no-mtime dev server) scene while Genesis Plaza GLTFs were still streaming โ€” `NullReferenceException` in `UnityEngine.Object.GetName` via `TextureData.EnsureTexture2D` ~6s after the reload drain.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.