fix: don't cache transient GLTFContainer asset-load failures as irrec… - #9774
fix: don't cache transient GLTFContainer asset-load failures as irrec…#9774alejandro-jimenez-dcl wants to merge 2 commits into
Conversation
…overable All rod meshes load from the same GLTF asset-bundle hash. A single transient failure during a load burst (an aborted request, a corrupt partial download, or any general exception) gets cached forever as an "irrecoverable" failure, so every subsequent load for that hash fails instantly with no retry — every rod goes permanently meshless until the client restarts. Separately (tracked as #9469), releasing a `GltfContainer` doesn't clear its cached promise result, so two release paths can dereference and re-pool the same live asset, causing a rod to disappear mid-use or get stuck loading forever when its shared mesh is destroyed by cache eviction. Memory pressure is what triggers cache eviction and thus both failure paths. Fixes #9511 Related: #9469 Includes a regression test that fails without this fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🚦 CI Status
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. Warnings not reduced: 13116 => 13118 — remove at least 3 warnings to merge. Warnings/errors in files changed by this PR (8)All Unity tests passed ✅
|
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
STEP 1 — Context & Scope
Files reviewed: 4 production files + 1 new test file (295 lines). Subsystem: StreamableLoading (asset loading pipeline) and GLTFContainer (GLTF asset lifecycle).
Surrounding context read: LoadSystemBase.cs (full), AssetsLoadingUtility.cs (full), WebRequestUtils.cs (full — IsIrrecoverableError, IsIrrecoverableResponseCode, IsAborted), AssetPromise.cs (full — NULL, ForgetLoading, TryGetResult), CleanUpGltfContainerSystem.cs (full), ResetGltfContainerSystem.cs (full), IStreamableCache.cs (full — interface contract for IrrecoverableFailures), StreamableLoadingResult.cs (full — Exception property, IsInitialized).
STEP 2 — Root-Cause Check
Problem: A single transient failure during a GLTF load burst (aborted request, corrupt download, general exception) is cached permanently in IrrecoverableFailures, blocking all future loads for that hash for the rest of the session. Separately, releasing a GltfContainer doesn't clear its cached promise result, enabling double-dereference of the same live asset.
Does the diff fix the cause? Yes — both fixes. The caching change in LoadSystemBase.RepeatLoopAsync (line 402) restricts what gets stored in IrrecoverableFailures from "any concluded failure" to "only definitive 4xx HTTP client errors." The Promise = NULL assignment prevents the double-dereference by clearing the cached result after cleanup. Neither fix is a symptom-level workaround. PASS.
STEP 3 — Design & Integration
No new long-lived units introduced. The diff adds one private static helper method (IsIrrecoverableFailure) and two single-line assignments. No new systems, managers, or state-holding types.
IsIrrecoverableFailure placement: Correct. LoadSystemBase is the sole gatekeeper for cache.IrrecoverableFailures (line 428, SetIrrecoverableFailure). The filtering logic belongs adjacent to RepeatLoopAsync where it is consumed. Placing it in WebRequestUtils would conflate the HTTP-level classification (retry policy) with the asset-loading-level decision (permanent cache poisoning).
Semantic layering with WebRequestUtils.IsIrrecoverableError: Intentional narrowing, not redundant overlap. IsIrrecoverableError() serves the retry policy — it returns true for aborted requests (IsAborted()), permanent 5xx codes (501, 505, 507, 508, 511 via IsIrrecoverableResponseCode default case), and non-DNS/non-SSL errors. The new IsIrrecoverableFailure() serves the caching policy — it adds the >= 400 and < 500 gate to ensure only definitive client errors are cached for the session. Aborted requests with no 4xx code, server errors (even permanent ones like 501), and non-HTTP exceptions are excluded from caching. This is the correct tightening: the retry policy says "don't retry this request," while the caching policy says "don't cache this as permanently failed."
Promise = NULL pattern consistency: This pattern appears in three locations in GLTF container systems, all added or already present in this PR. Other systems (e.g. AvatarLoaderSystem) call ForgetLoading without NULL assignment because they either immediately reassign the promise or the entity is about to be destroyed. The GLTF cleanup path is different: the component can linger after ForgetLoading (per-frame delete intention, scene teardown), making the NULL guard specifically necessary here.
Teardown trace: ForgetLoading (AssetPromise.cs:129-135) cancels the CTS and destroys the entity. The subsequent Promise = NULL clears the cached Result field. TryGetResult on a NULL promise returns false immediately (Entity == Entity.Null check, line 70). No subscription/event/callback is left dangling.
PASS.
STEP 4 — Member Audit
IsIrrecoverableFailure (private static, 1 consumer): Called only from RepeatLoopAsync (line 402). Single-use is appropriate — it's a predicate extracted for readability of the ternary condition. It classifies the exception chain rather than re-checking state. No merge-or-inline concern.
STEP 5 — Line-Level Review
No P0 or P1 issues found. One P2 finding below (posted as inline comment).
STEP 6 — Complexity
COMPLEX — Modifies the asset loading pipeline's caching policy (LoadSystemBase), changes resource cleanup paths in GLTF container systems, and involves async flow interaction between RepeatLoopAsync, CacheableFlowAsync, and FlowAsync.
STEP 7 — QA Assessment
QA_REQUIRED: YES — Changes affect runtime asset loading behavior. The fishing-rod regression (all rods going permanently meshless) is the primary scenario to verify. Testing should cover: asset loading under memory pressure, GLTF container cleanup during scene teardown, and repeated load/release cycles.
STEP 8 — Non-Blocking Warnings
No Main Scene modifications detected. No warnings.
Security Review
No security issues found. The test HttpListener binds to loopback only (IPAddress.Loopback). No secrets, injection vectors, or auth concerns.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies the StreamableLoading asset-caching pipeline policy and GLTF container resource cleanup lifecycle
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
|
🔍 Jarvis reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging. |
alejandro-jimenez-dcl
left a comment
alejandro-jimenez-dcl
left a comment
There was a problem hiding this comment.
Approved
ac96d63 to
6ca5343
Compare
|
PR #9774, run #32172184339 Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Intel Core i5
Exception breakdown
Apple M1
|
|
Superseded by #9784, which combines this and the other bugsweep fixes into a single PR (one squashed commit per original, rebased onto updated dev). Closing in favor of that one. |
3 participants
…overable
All rod meshes load from the same GLTF asset-bundle hash. A single transient failure during a load burst (an aborted request, a corrupt partial download, or any general exception) gets cached forever as an "irrecoverable" failure, so every subsequent load for that hash fails instantly with no retry — every rod goes permanently meshless until the client restarts. Separately (tracked as #9469), releasing a
GltfContainerdoesn't clear its cached promise result, so two release paths can dereference and re-pool the same live asset, causing a rod to disappear mid-use or get stuck loading forever when its shared mesh is destroyed by cache eviction. Memory pressure is what triggers cache eviction and thus both failure paths.Fixes #9511
Related: #9469
Includes a regression test that fails without this fix.
Pull Request Description
What does this PR change?
Test Instructions
Steps (standard run):
metaforge explorer run XXXX # ← replace with this PR numberExpected result:
Steps (fresh account):
metaforge account create --clear metaforge explorer run XXXX # ← replace with this PR numberExpected result:
Automation (if applicable):
metaforge explorer test XXXXPrerequisites
Test Steps
Additional Testing Notes
Quality Checklist
Code Review Reference
Please review our Branch & PR Standards before submitting. It explains the automated review flow, QA/DEV approval requirements, and what each label does — especially useful for first-time contributors.