Skip to content

fix: store failed result for emotes to prevent infinite 404 retry loop#8619

Merged
dalkia merged 8 commits into
devfrom
fix/emote-infinite-404-retry-loop
May 11, 2026
Merged

fix: store failed result for emotes to prevent infinite 404 retry loop#8619
dalkia merged 8 commits into
devfrom
fix/emote-infinite-404-retry-loop

Conversation

@dalkia

@dalkia dalkia commented May 5, 2026

Copy link
Copy Markdown
Collaborator

Pull Request Description

What does this PR change?

When emote asset bundles (or GLTFs) fail to load — for example, because the emote does not exist in the zone environment and returns a 404 — FinalizeEmoteLoadingSystem was only logging a warning and leaving AssetResults[bodyShape] as null. This caused CreateAssetBundlePromiseIfRequired to create a new promise every frame, producing an infinite loop of 404 requests and, in some cases, preventing the avatar from ever loading.

This PR cherry-picks the emote-loading portion of #8504 into dev. That PR was originally merged onto a WebGL branch, so the fix never reached dev and the issue is still present.

The fix stores a failed StreamableLoadingResult on the failure branch in both FinalizeAssetBundleLoading and FinalizeGltfLoading, mirroring SetAsFailed in FinalizeWearableLoadingSystemBase. The non-null result stops CreateAssetBundlePromiseIfRequired from re-creating the promise every frame, which lets the avatar finish loading.

Test Instructions

Prerequisites

  • Use the zone environment

Test Steps

  1. Launch the explorer connected to zone.
  2. Check that your avatar loads in the authentication screen
  3. Observe the network tab / logs: there should be no repeated 404 requests for the same emote asset bundle hash.
  4. The avatar should appear and finish loading. Without this fix, there is a chance the avatar never loads at all because the emote 404 retry loop keeps the asset-promise pipeline busy indefinitely.

Additional Testing Notes

  • Failing emotes will not play (expected) — only the infinite retry loop and the avatar-never-loads symptom are addressed.
  • The fix mirrors the wearable failure-handling pattern (SetAsFailed in FinalizeWearableLoadingSystemBase), so behavior should be consistent with how missing wearables are handled today.
  • Catalyst (non-zone) environments where all emotes resolve successfully should be unaffected.

Quality Checklist

  • Changes have been tested locally
  • Documentation has been updated (if required)
  • Performance impact has been considered
  • For SDK features: Test scene is included

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.

When emote asset bundles fail to load (404), AssetResults[bodyShape]
was left as null, causing CreateAssetBundlePromiseIfRequired to
create a new promise every frame — looping indefinitely.
Store a failed StreamableLoadingResult on failure, matching the
pattern already used by wearables (SetAsFailed).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@dalkia
dalkia requested review from a team as code owners May 5, 2026 15:34
@github-actions

github-actions Bot commented May 5, 2026

Copy link
Copy Markdown
Contributor

@github-actions

github-actions Bot commented May 5, 2026

Copy link
Copy Markdown
Contributor

Thanks for opening this PR! Here's how the review process works:

Auto-review is running — this is a fix:/chore: PR, so Claude will review it automatically. A QA reviewer and a DEV reviewer have been auto-assigned.

For the full review process, labels, and merge requirements, see Branch & PR Standards.

@github-actions
github-actions Bot requested a review from anicalbano May 5, 2026 15:34
@claude

claude Bot commented May 5, 2026

Copy link
Copy Markdown
Contributor

Claude finished @dalkia's task in 2m 17s —— View job


Review

  • Gather context
  • Read changed file and related systems
  • Review against CLAUDE.md standards
  • Post final review

Root-Cause Analysis

Problem: When an emote asset bundle (or GLTF) returns a 404, FinalizeAssetBundleLoading/FinalizeGltfLoading logged a warning but left emote.AssetResults[bodyShape] as null. CreateAssetBundlePromiseIfRequired (line 233 in LoadEmotesByPointersSystem.cs) guards promise creation with component.AssetResults[intention.BodyShape] == null, so on every update tick it found null, created a fresh promise, got another 404, and looped indefinitely.

Does the diff fix the cause or a symptom? ✅ Root cause. The fix stores a non-null StreamableLoadingResult<AttachmentRegularAsset> with a failure exception (via the new AssignFailedEmoteResult helper). This satisfies the != null guard in CreateAssetBundlePromiseIfRequired, halts re-queuing, and — because line 149 of the loading system counts any non-null result toward completion — lets the overall emote promise resolve, unblocking avatar loading.


No Blocking Issues Found

The implementation is correct and consistent:

  • Pattern parity with wearables: AssignFailedEmoteResult mirrors SetAsFailed in FinalizeWearableLoadingSystemBase (same GetReportData() + fresh exception + unisex both-body-shape handling). ✅
  • IsInitialized semantics preserved: The failed StreamableLoadingResult has Exception != null, so IsInitialized returns true. ResetEmoteResultOnCancellation (line 201) checks is { IsInitialized: false } before clearing — meaning a previously-failed result will not be erased by a subsequent cancellation. ✅
  • Allocation concern (CLAUDE.md §4): new Exception(...) only executes on the failure branch, not on every frame. Once stored, the non-null result prevents re-entry. One-time allocation per failed emote is acceptable. ✅
  • Both loading paths covered: Both FinalizeAssetBundleLoading and FinalizeGltfLoading call AssignFailedEmoteResult, matching the two places a per-body-shape asset promise can be created. ✅
  • Audio clip re-retry implicitly fixed: TryCreateAudioClipPromises is only called inside CreateAssetBundlePromiseIfRequired, so preventing AB promise re-creation also prevents audio re-queuing. ✅

REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies the emote asset promise lifecycle (avatar rendering pipeline) — specifically how failed StreamableLoadingResult values are stored to prevent promise re-creation.
QA_REQUIRED: YES

@github-actions

github-actions Bot commented May 5, 2026

Copy link
Copy Markdown
Contributor

🔍 Claude reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging.

@dalkia dalkia self-assigned this May 6, 2026
@claude

claude Bot commented May 6, 2026

Copy link
Copy Markdown
Contributor

Claude finished @dalkia's task in 1m 16s —— View job


Claude finished reviewing PR #8619

Review

  • Check latest diff vs origin/dev
  • Inspect changed file in full context
  • Verify against CLAUDE.md standards
  • Post final review

Root-Cause Analysis

Problem: When an emote asset bundle (or GLTF) returns 404, FinalizeAssetBundleLoading/FinalizeGltfLoading only logged a warning and left emote.AssetResults[bodyShape] as null. The guard at LoadEmotesByPointersSystem.cs:233 (component.AssetResults[intention.BodyShape] == null) then re-created a fresh AB promise every frame, producing an infinite 404 retry loop and stalling the avatar pipeline.

Does the diff fix the cause or a symptom? ✅ Root cause. The new AssignFailedEmoteResult helper writes a non-null StreamableLoadingResult<AttachmentRegularAsset> carrying a fresh Exception on the failure branch in both FinalizeAssetBundleLoading (FinalizeEmoteLoadingSystem.cs:87) and FinalizeGltfLoading (FinalizeEmoteLoadingSystem.cs:112). The non-null result satisfies the guard and prevents promise re-queuing; the overall emote promise can then resolve, unblocking avatar loading.

(Note: the only diff vs my previous review is the merge commit bc22821b from dev. The substantive change in FinalizeEmoteLoadingSystem.cs is unchanged.)


No Blocking Issues Found

  • Pattern parity with wearables: AssignFailedEmoteResult mirrors SetAsFailed in FinalizeWearableLoadingSystemBase (same GetReportData() + fresh exception + unisex both-body-shape handling). ✅
  • IsInitialized semantics preserved: The failed StreamableLoadingResult has Exception != null, so IsInitialized returns true. ResetEmoteResultOnCancellation (FinalizeEmoteLoadingSystem.cs:201) only clears results where IsInitialized: false, so a failed result will not be erased on a subsequent cancellation. ✅
  • Allocation concern (CLAUDE.md §4): new Exception(...) is allocated only once per failed emote (the non-null result then prevents re-entry), not on every frame — acceptable. ✅
  • Both loading paths covered: Both AB and GLTF finalize paths call the helper, matching the two places per-body-shape promises can be created. ✅
  • Audio retry implicitly fixed: TryCreateAudioClipPromises is only invoked inside CreateAssetBundlePromiseIfRequired, so blocking AB promise re-creation also blocks audio re-queuing. ✅

REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies the emote asset-promise lifecycle in the avatar rendering pipeline — specifically how failed StreamableLoadingResult values are stored to prevent promise re-creation.
QA_REQUIRED: YES

@github-actions

github-actions Bot commented May 6, 2026

Copy link
Copy Markdown
Contributor

🔍 Claude reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging.

@dalkia
dalkia enabled auto-merge (squash) May 6, 2026 18:23
@claude

claude Bot commented May 6, 2026

Copy link
Copy Markdown
Contributor

Claude finished @dalkia's task in 55s —— View job


Claude finished reviewing PR #8619

Review

  • Check latest diff vs origin/dev
  • Inspect changed file in full context
  • Verify against CLAUDE.md standards
  • Post final review

Root-Cause Analysis

Problem: When an emote asset bundle (or GLTF) returned 404, FinalizeAssetBundleLoading/FinalizeGltfLoading only logged a warning and left emote.AssetResults[bodyShape] as null. The guard at LoadEmotesByPointersSystem.cs:233 (component.AssetResults[intention.BodyShape] == null) then re-created a fresh AB promise every frame — an infinite 404 retry loop that stalled avatar loading.

Does the diff fix the cause or a symptom? ✅ Root cause. The new AssignFailedEmoteResult helper writes a non-null StreamableLoadingResult<AttachmentRegularAsset> carrying a fresh Exception on the failure branch in both FinalizeAssetBundleLoading (FinalizeEmoteLoadingSystem.cs:87) and FinalizeGltfLoading (FinalizeEmoteLoadingSystem.cs:112). The non-null result satisfies the guard and prevents promise re-queuing; the overall emote promise can then resolve, unblocking avatar loading.

(Note: the only diff vs my previous review is the new merge commit 577e6189 from dev. The substantive change in FinalizeEmoteLoadingSystem.cs is unchanged.)


No Blocking Issues Found

  • Pattern parity with wearables: AssignFailedEmoteResult mirrors SetAsFailed in FinalizeWearableLoadingSystemBase (same GetReportData() + fresh exception + unisex both-body-shape handling). ✅
  • IsInitialized semantics preserved: The failed StreamableLoadingResult has Exception != null, so IsInitialized returns true. ResetEmoteResultOnCancellation only clears results where IsInitialized: false, so a failed result will not be erased on a subsequent cancellation. ✅
  • Allocation concern (CLAUDE.md §4): new Exception(...) is allocated only once per failed emote (the non-null result then prevents re-entry), not on every frame — acceptable. ✅
  • Both loading paths covered: Both AB and GLTF finalize paths call the helper, matching the two places per-body-shape promises can be created. ✅
  • Audio retry implicitly fixed: TryCreateAudioClipPromises is only invoked inside CreateAssetBundlePromiseIfRequired, so blocking AB promise re-creation also blocks audio re-queuing. ✅

REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies the emote asset-promise lifecycle in the avatar rendering pipeline — specifically how failed StreamableLoadingResult values are stored to prevent promise re-creation.
QA_REQUIRED: YES
• Branch: fix/emote-infinite-404-retry-loop

@github-actions

github-actions Bot commented May 6, 2026

Copy link
Copy Markdown
Contributor

🔍 Claude reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging.

@Ludmilafantaniella Ludmilafantaniella 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.

Tested on Windows and macOS ✅

8619-evi2.mp4
8619-evi.mp4

✅Smoke test:

  • ✔️ Backpack and wearables in world
  • ✔️ Emotes in world and in backpack
  • ✔️ Teleport with map/coordinates/Jump In
  • Genesis Plaza (scene emotes)
  • The Inn (autoEmotes)
  • ✔️ Chat and multiplayer
  • ✔️ Profile card
  • ✔️ Camera
  • ✔️ Skybox

@claude

claude Bot commented May 6, 2026

Copy link
Copy Markdown
Contributor

Claude finished @dalkia's task in 48s —— View job


Claude finished reviewing PR #8619

Review

  • Check latest diff vs origin/dev
  • Inspect changed file in full context
  • Verify against CLAUDE.md standards
  • Post final review

Root-Cause Analysis

Problem: When an emote asset bundle (or GLTF) returned 404, FinalizeAssetBundleLoading/FinalizeGltfLoading only logged a warning and left emote.AssetResults[bodyShape] as null. The guard at LoadEmotesByPointersSystem.cs:233 (component.AssetResults[intention.BodyShape] == null) then re-created a fresh AB promise every frame — an infinite 404 retry loop that stalled the avatar pipeline.

Does the diff fix the cause or a symptom? ✅ Root cause. The new AssignFailedEmoteResult helper writes a non-null StreamableLoadingResult<AttachmentRegularAsset> carrying a fresh Exception on the failure branch in both FinalizeAssetBundleLoading (FinalizeEmoteLoadingSystem.cs:87) and FinalizeGltfLoading (FinalizeEmoteLoadingSystem.cs:112). The non-null result satisfies the guard and prevents promise re-queuing; the overall emote promise can then resolve, unblocking avatar loading.

(Note: substantive change in FinalizeEmoteLoadingSystem.cs is unchanged from prior reviews — the only delta vs my last pass is the merge commit 4d6279f0 from dev.)


No Blocking Issues Found

  • Pattern parity with wearables: AssignFailedEmoteResult mirrors SetAsFailed in FinalizeWearableLoadingSystemBase (same GetReportData() + fresh exception + unisex both-body-shape handling). ✅
  • IsInitialized semantics preserved: The failed StreamableLoadingResult has Exception != null, so IsInitialized returns true. ResetEmoteResultOnCancellation (FinalizeEmoteLoadingSystem.cs:201) only clears results where IsInitialized: false, so a failed result will not be erased on a subsequent cancellation. ✅
  • Allocation concern (CLAUDE.md §4): new Exception(...) is allocated only once per failed emote (the non-null result then prevents re-entry), not on every frame — acceptable. ✅
  • Both loading paths covered: Both AB and GLTF finalize paths call the helper, matching the two places per-body-shape promises can be created. ✅
  • Audio retry implicitly fixed: TryCreateAudioClipPromises is only invoked inside CreateAssetBundlePromiseIfRequired, so blocking AB promise re-creation also blocks audio re-queuing. ✅

REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies the emote asset-promise lifecycle in the avatar rendering pipeline — specifically how failed StreamableLoadingResult values are stored to prevent promise re-creation.
QA_REQUIRED: YES

• Branch: fix/emote-infinite-404-retry-loop

@github-actions

github-actions Bot commented May 6, 2026

Copy link
Copy Markdown
Contributor

🔍 Claude reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging.

@claude

claude Bot commented May 8, 2026

Copy link
Copy Markdown
Contributor

Claude finished @dalkia's task in 53s —— View job


Claude finished reviewing PR #8619

Review

  • Check latest diff vs origin/dev
  • Inspect changed file in full context
  • Verify against CLAUDE.md standards
  • Post final review

Root-Cause Analysis

Problem: When an emote asset bundle (or GLTF) returned 404, FinalizeAssetBundleLoading/FinalizeGltfLoading only logged a warning and left emote.AssetResults[bodyShape] as null. The guard at LoadEmotesByPointersSystem.cs:233 (component.AssetResults[intention.BodyShape] == null) then re-created a fresh AB promise every frame — an infinite 404 retry loop that stalled the avatar pipeline.

Does the diff fix the cause or a symptom? ✅ Root cause. The new AssignFailedEmoteResult helper writes a non-null StreamableLoadingResult<AttachmentRegularAsset> carrying a fresh Exception on the failure branch in both FinalizeAssetBundleLoading (FinalizeEmoteLoadingSystem.cs:87) and FinalizeGltfLoading (FinalizeEmoteLoadingSystem.cs:112). The non-null result satisfies the guard and prevents promise re-queuing; the overall emote promise can then resolve, unblocking avatar loading.

(Note: substantive change in FinalizeEmoteLoadingSystem.cs is unchanged from prior reviews — the only delta vs my last pass is the new merge commit e06ce5b5 from dev.)


No Blocking Issues Found

  • Pattern parity with wearables: AssignFailedEmoteResult mirrors SetAsFailed in FinalizeWearableLoadingSystemBase (same GetReportData() + fresh exception + unisex both-body-shape handling). ✅
  • IsInitialized semantics preserved: The failed StreamableLoadingResult has Exception != null, so IsInitialized returns true. ResetEmoteResultOnCancellation (FinalizeEmoteLoadingSystem.cs:201) only clears results where IsInitialized: false, so a failed result will not be erased on a subsequent cancellation. ✅
  • Allocation concern (CLAUDE.md §4): new Exception(...) is allocated only once per failed emote (the non-null result then prevents re-entry), not on every frame — acceptable. ✅
  • Both loading paths covered: Both AB and GLTF finalize paths call the helper, matching the two places per-body-shape promises can be created. ✅
  • Audio retry implicitly fixed: TryCreateAudioClipPromises is only invoked inside CreateAssetBundlePromiseIfRequired, so blocking AB promise re-creation also blocks audio re-queuing. ✅

REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies the emote asset-promise lifecycle in the avatar rendering pipeline — specifically how failed StreamableLoadingResult values are stored to prevent promise re-creation.
QA_REQUIRED: YES

• Branch: fix/emote-infinite-404-retry-loop

@github-actions

github-actions Bot commented May 8, 2026

Copy link
Copy Markdown
Contributor

🔍 Claude reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging.

Tests previously asserted AssetResults[bodyShape] was null on failure,
but the retry-loop fix now stores a failed StreamableLoadingResult.
Update assertions to verify the result is set and marked as failed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented May 8, 2026

Copy link
Copy Markdown
Contributor

Claude finished @dalkia's task in 1m 8s —— View job


Claude finished reviewing PR #8619

Review

  • Check latest diff vs origin/dev
  • Inspect changed files in full context
  • Verify against CLAUDE.md standards
  • Post final review

Root-Cause Analysis

Problem: When an emote asset bundle (or GLTF) returned 404, FinalizeAssetBundleLoading/FinalizeGltfLoading only logged a warning and left emote.AssetResults[bodyShape] as null. The guard at LoadEmotesByPointersSystem.cs:233 (component.AssetResults[intention.BodyShape] == null) then re-created a fresh AB promise every frame — an infinite 404 retry loop that stalled the avatar pipeline.

Does the diff fix the cause or a symptom? ✅ Root cause. The new AssignFailedEmoteResult helper writes a non-null StreamableLoadingResult<AttachmentRegularAsset> carrying a fresh Exception on the failure branch in both FinalizeAssetBundleLoading (FinalizeEmoteLoadingSystem.cs:87) and FinalizeGltfLoading (FinalizeEmoteLoadingSystem.cs:112). The non-null result satisfies the guard and prevents promise re-queuing; the overall emote promise can then resolve, unblocking avatar loading.

(Delta vs prior reviews: the new commit f044c5d7 updates two previously-failing unit tests in FinalizeEmoteLoadingSystemShould.cs to assert the new behavior — AssetResults[bodyShape] is now a non-null failed StreamableLoadingResult rather than null. Test changes are correct and consistent with the production change.)


No Blocking Issues Found

  • Pattern parity with wearables: AssignFailedEmoteResult mirrors SetAsFailed in FinalizeWearableLoadingSystemBase (same GetReportData() + fresh exception + unisex both-body-shape handling). ✅
  • IsInitialized semantics preserved: The failed StreamableLoadingResult has Exception != null, so IsInitialized returns true. ResetEmoteResultOnCancellation (FinalizeEmoteLoadingSystem.cs:201) only clears results where IsInitialized: false, so a failed result will not be erased on a subsequent cancellation. ✅
  • Allocation concern (CLAUDE.md §4): new Exception(...) is allocated only once per failed emote (the non-null result then prevents re-entry), not on every frame — acceptable. ✅
  • Both loading paths covered: Both AB and GLTF finalize paths call the helper, matching the two places per-body-shape promises can be created. ✅
  • Audio retry implicitly fixed: TryCreateAudioClipPromises is only invoked inside CreateAssetBundlePromiseIfRequired, so blocking AB promise re-creation also blocks audio re-queuing. ✅
  • Tests updated to match new behavior: FinalizeEmoteLoadingSystemShould.cs:228-229,349-350 — the two failure-path tests now assert HasValue == true and Succeeded == false, which is the correct expectation given the fix. ✅

REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies the emote asset-promise lifecycle in the avatar rendering pipeline — specifically how failed StreamableLoadingResult values are stored to prevent promise re-creation.
QA_REQUIRED: YES

• Branch: fix/emote-infinite-404-retry-loop

@claude

claude Bot commented May 8, 2026

Copy link
Copy Markdown
Contributor

Claude finished @dalkia's task in 1m 4s —— View job


Claude finished reviewing PR #8619

Review

  • Check latest diff vs origin/dev
  • Inspect changed files in full context
  • Verify against CLAUDE.md standards
  • Post final review

Root-Cause Analysis

Problem: When an emote asset bundle (or GLTF) returned 404, FinalizeAssetBundleLoading/FinalizeGltfLoading only logged a warning and left emote.AssetResults[bodyShape] as null. The guard at LoadEmotesByPointersSystem.cs:233 (component.AssetResults[intention.BodyShape] == null) then re-created a fresh AB promise every frame — an infinite 404 retry loop that stalled the avatar pipeline.

Does the diff fix the cause or a symptom? ✅ Root cause. The new AssignFailedEmoteResult helper writes a non-null StreamableLoadingResult<AttachmentRegularAsset> carrying a fresh Exception on the failure branch in both FinalizeAssetBundleLoading (FinalizeEmoteLoadingSystem.cs:87) and FinalizeGltfLoading (FinalizeEmoteLoadingSystem.cs:112). The non-null result satisfies the guard and prevents promise re-queuing; the overall emote promise can then resolve, unblocking avatar loading.

(Delta vs prior review: a new merge commit fb598d6f from dev plus test-expectation updates in FinalizeEmoteLoadingSystemShould.cs — the failure-path tests now assert that AssetResults[bodyShape] holds a non-null, non-Succeeded result, matching the new behavior. The substantive change in FinalizeEmoteLoadingSystem.cs is unchanged.)


No Blocking Issues Found

  • Pattern parity with wearables: AssignFailedEmoteResult mirrors SetAsFailed in FinalizeWearableLoadingSystemBase (same GetReportData() + fresh exception + unisex both-body-shape handling). ✅
  • IsInitialized semantics preserved: The failed StreamableLoadingResult has Exception != null, so IsInitialized returns true. ResetEmoteResultOnCancellation (FinalizeEmoteLoadingSystem.cs:201) only clears results where IsInitialized: false, so a failed result will not be erased on a subsequent cancellation. ✅
  • Allocation concern (CLAUDE.md §4): new Exception(...) is allocated only once per failed emote (the non-null result then prevents re-entry), not on every frame — acceptable. ✅
  • Both loading paths covered: Both AB and GLTF finalize paths call the helper, matching the two places per-body-shape promises can be created. ✅
  • Audio retry implicitly fixed: TryCreateAudioClipPromises is only invoked inside CreateAssetBundlePromiseIfRequired, so blocking AB promise re-creation also blocks audio re-queuing. ✅
  • Tests updated to match new contract: FinalizeEmoteLoadingSystemShould.cs assertions now expect a failed result rather than null, locking in the regression fix. ✅

REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies the emote asset-promise lifecycle in the avatar rendering pipeline — specifically how failed StreamableLoadingResult values are stored to prevent promise re-creation.
QA_REQUIRED: YES
• Branch: fix/emote-infinite-404-retry-loop

@github-actions

github-actions Bot commented May 8, 2026

Copy link
Copy Markdown
Contributor

🔍 Claude reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging.

@github-actions

github-actions Bot commented May 8, 2026

Copy link
Copy Markdown
Contributor

🔍 Claude reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging.

@claude

claude Bot commented May 11, 2026

Copy link
Copy Markdown
Contributor

Claude finished @dalkia's task in 51s —— View job


Claude finished reviewing PR #8619

Review

  • Check latest diff vs origin/dev
  • Inspect changed files in full context
  • Verify against CLAUDE.md standards
  • Post final review

Root-Cause Analysis

Problem: When an emote asset bundle (or GLTF) returned 404, FinalizeAssetBundleLoading/FinalizeGltfLoading only logged a warning and left emote.AssetResults[bodyShape] as null. The guard at LoadEmotesByPointersSystem.cs:233 (component.AssetResults[intention.BodyShape] == null) then re-created a fresh AB promise every frame — an infinite 404 retry loop that stalled the avatar pipeline.

Does the diff fix the cause or a symptom? ✅ Root cause. The new AssignFailedEmoteResult helper writes a non-null StreamableLoadingResult<AttachmentRegularAsset> carrying a fresh Exception on the failure branch in both FinalizeAssetBundleLoading (FinalizeEmoteLoadingSystem.cs:87) and FinalizeGltfLoading (FinalizeEmoteLoadingSystem.cs:112). The non-null result satisfies the guard and prevents promise re-queuing; the overall emote promise can then resolve, unblocking avatar loading.

(Delta vs prior review: new merge commit 0637fe26 from dev. The substantive change in FinalizeEmoteLoadingSystem.cs and the test-expectation updates in FinalizeEmoteLoadingSystemShould.cs are unchanged.)


No Blocking Issues Found

  • Pattern parity with wearables: AssignFailedEmoteResult mirrors SetAsFailed in FinalizeWearableLoadingSystemBase (same GetReportData() + fresh exception + unisex both-body-shape handling). ✅
  • IsInitialized semantics preserved: The failed StreamableLoadingResult has Exception != null, so IsInitialized returns true. ResetEmoteResultOnCancellation (FinalizeEmoteLoadingSystem.cs:201) only clears results where IsInitialized: false, so a failed result will not be erased on a subsequent cancellation. ✅
  • Allocation concern (CLAUDE.md §4): new Exception(...) is allocated only once per failed emote (the non-null result then prevents re-entry), not on every frame — acceptable. ✅
  • Both loading paths covered: Both AB and GLTF finalize paths call the helper, matching the two places per-body-shape promises can be created. ✅
  • Audio retry implicitly fixed: TryCreateAudioClipPromises is only invoked inside CreateAssetBundlePromiseIfRequired, so blocking AB promise re-creation also blocks audio re-queuing. ✅
  • Tests updated to match new contract: FinalizeEmoteLoadingSystemShould.cs:228-229,349-350 — the failure-path tests now assert HasValue == true and Succeeded == false, locking in the regression fix. ✅

REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies the emote asset-promise lifecycle in the avatar rendering pipeline — specifically how failed StreamableLoadingResult values are stored to prevent promise re-creation.
QA_REQUIRED: YES

• Branch: fix/emote-infinite-404-retry-loop

@github-actions

Copy link
Copy Markdown
Contributor

🔍 Claude reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging.

@dalkia
dalkia merged commit d49c13c into dev May 11, 2026
14 checks passed
@dalkia
dalkia deleted the fix/emote-infinite-404-retry-loop branch May 11, 2026 04:53
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