fix: asset-bundle-budget-null-safe#9399
Conversation
The pooled StreamableLoadingState can be recycled (budget disposed and nulled via the pool actionOnRelease) if the promise is forgotten or cancelled while the detached flow is mid-await. Releasing the budget with !. would NRE in that window, so guard with ?. to match how the budget is treated everywhere else in the flow.
Pass envelope.Ct to UniTask.Delay so the artificial delay is cancelled along with the request instead of running to completion after the request is already gone.
alejandro-jimenez-dcl
changed the title
|
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — fix: asset-bundle-budget-null-safe
STEP 1 — Context & Scope
Files changed: 2 (+4 −2)
Explorer/Assets/DCL/Infrastructure/ECS/Unity/StreamableLoading/AssetBundles/LoadAssetBundleSystem.csExplorer/Assets/DCL/WebRequests/ArtificialDelayWebRequestController.cs
Loaded: CLAUDE.md, LoadSystemBase.cs, StreamableLoadingState.cs, IAcquiredBudget.cs, AcquiredBudget.cs. Searched codebase for all AcquiredBudget usage (20+ files) and all ?.Release() call sites in StreamableLoading/.
STEP 2 — Root-cause check
Change 1 — AcquiredBudget!.Release() → AcquiredBudget?.Release()
Problem: AcquiredBudget is declared IAcquiredBudget? (nullable, StreamableLoadingState.cs:74). The old code used the null-forgiving operator !, which suppressed the compiler warning but could NRE at runtime.
Root cause: FlowInternalAsync runs inside a detached async flow (.Forget() at LoadSystemBase.Execute). The StreamableLoadingState is pooled — when the promise is forgotten/cancelled externally, POOL.actionOnRelease calls DisposeBudgetIfExists(), which calls AcquiredBudget.Dispose() (internally calls Release()) then sets AcquiredBudget = null. If the detached flow is mid-await at that point, it would NRE on the !.Release() line.
Fixes cause, not symptom? ✅ Yes. The property IS nullable by declaration and by runtime lifecycle. Using ?. correctly matches the declaration. If null, the budget was already released via Dispose() → Release() internally (verified in AcquiredBudget.cs:24-28), so no budget leak occurs. The base class LoadSystemBase.cs:146 already uses state.AcquiredBudget?.Release() for the identical scenario.
Change 2 — Missing cancellationToken on UniTask.Delay
Problem: Without the token, UniTask.Delay runs to completion even after the request is cancelled.
Root cause: The parameter was simply omitted. The method already uses envelope.Ct for GetOptionsAsync() and passes it through to origin.SendAsync(), so the token is available and appropriate.
Fixes cause, not symptom? ✅ Yes — directly addresses the missing cancellation propagation.
STEP 2 VERDICT: PASS ✅
STEP 3 — Design & integration
No new types, systems, managers, or persistent state introduced. Both changes are single-line fixes to existing methods in existing classes.
- No lifecycle duplication.
- No per-frame reconciliation added.
- No new subscriptions, events, or resources.
Owner search: N/A — no new long-lived units.
Teardown/consumption trace: No new subscriptions or resources opened by this diff.
STEP 3 VERDICT: PASS ✅
STEP 4 — Member audit
No public properties or accessors added or changed.
STEP 5 — Line-level review
Pass A (Blocking issues): None found.
- ✅ No CLAUDE.md violations —
?.on nullable is correct (CLAUDE.md §11 anti-pattern: "Defensive null-checks against non-null declarations" —AcquiredBudgetIS nullable, so?.is appropriate) - ✅ No bugs or runtime errors introduced
- ✅ No security vulnerabilities
- ✅ No performance issues (both changes are on existing async paths, no new allocations)
- ✅ No missing error handling
- ✅ No resource leaks —
Release()is idempotent (AcquiredBudget.cs:36-40), and null means already disposed+released - ✅ No nullability-contract violations —
?.matches theIAcquiredBudget?declaration
Pass B (Design, encapsulation & resource smells): None found.
- Comment on line 98-99 is justified — explains why null is possible at this point (pooled state recycling during detached flow), not narrating external behavior.
Parallel review agents (security, architecture, patterns): All returned clean — no findings.
STEP 6 — Complexity assessment
SIMPLE — 2 files, 4 lines changed, straightforward null-safety and cancellation-token fixes. No ECS system changes, no async pattern changes, no architectural modifications.
STEP 7 — QA assessment
QA_REQUIRED: YES — Both files are runtime code in the asset loading pipeline and web request layer. The null-safety fix affects asset bundle loading under cancellation edge cases; the cancellation token fix affects artificial delay behavior during request teardown.
STEP 8 — Non-blocking warnings
None. Main scene not modified.
STEP 9 — Verdict
Both changes are correct, minimal, and aligned with existing codebase patterns. No issues found across manual review and parallel agent analysis (security, architecture, patterns).
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Null-safety fix in asset bundle loading and cancellation token fix in web request delay — no architectural subsystems affected.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Warnings not reduced: 19087 => 19156 — remove at least one warning to merge. |
DafGreco
left a comment
DafGreco
left a comment
There was a problem hiding this comment.
✔️ 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
- [ ✔️] Camera
- [ ✔️] Settings
4 participants
Pull Request Description
What does this PR change?
Two small robustness fixes in the streamable loading / web request path:
Null-safe budget release in
LoadAssetBundleSystem. The pooledStreamableLoadingStatecan be recycled (its budget disposed and nulled by the poolactionOnRelease) if the promise is forgotten or cancelled while the detached load flow is mid-await. Releasing the budget with!.would throw aNullReferenceExceptionin that window. Switched to?.to match how the budget is treated everywhere else in the flow (FlowAsync's ongoing-request path,FinalizeLoading,IsWorldInvalidall already null-check it).Honor cancellation token in
ArtificialDelayWebRequestController.UniTask.Delaynow receivesenvelope.Ct, so the artificial delay is cancelled together with the request instead of running to completion after the request is already gone.Test Instructions
Smoke test, nothing in particular. Run the app and confirm scenes/asset bundles load normally with no regressions or new errors in the console.
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.