Skip to content

fix: catch canceled exceptions when Teleporting#8858

Open
alejandro-jimenez-dcl wants to merge 2 commits into
devfrom
fix/async-load-report-cancel-noise
Open

fix: catch canceled exceptions when Teleporting#8858
alejandro-jimenez-dcl wants to merge 2 commits into
devfrom
fix/async-load-report-cancel-noise

Conversation

@alejandro-jimenez-dcl
Copy link
Copy Markdown
Contributor

Pull Request Description

What does this PR change?

Fixes Sentry issue UNITY-EXPLORER-NHG: System.Exception: Cancelled: OperationCanceledException reported from AsyncLoadProcessReport.WaitUntilFinishedAsync (70 events / 47 users since 2026-03-07).

Root cause: when a scene load operation returned EnumResult.CancelledResult(TaskError.Cancelled), LoadingScreen.ExecuteOperationAsync called loadReport.SetResult(result.AsResult()). SetResult wraps any failure into new Exception(errorMessage), converting the cancellation into a plain Exception whose message
is "Cancelled: OperationCanceledException". WaitUntilFinishedAsync's generic catch (Exception) branch then reported it to Sentry via ReportHub.LogException.

Fix: in LoadingScreen.cs:46-56, route the Cancelled state directly to loadReport.SetCancelled(), which propagates OperationCanceledException through the completion source. WaitUntilFinishedAsync catches OCE separately and does not report it to Sentry. All other paths still go through SetResult.

Test Steps

Check that teleporting work as expected.

Reproduction (optional, dev-only)

Enable the Abort Scene Loading debug widget (DEBUG panel), set Type to CANCEL and coords to a scene currently loading. Before the fix: Sentry exception fires. After the fix: silent cancellation, no exception.

Quality Checklist

  • Changes have been tested locally
  • Documentation has been updated (if required)
  • Performance impact has been considered (none, single conditional branch)
  • For SDK features: Test scene is included (n/a)

@alejandro-jimenez-dcl alejandro-jimenez-dcl marked this pull request as ready for review May 22, 2026 11:30
@alejandro-jimenez-dcl alejandro-jimenez-dcl requested review from a team as code owners May 22, 2026 11:30
@github-actions github-actions Bot requested a review from DafGreco May 22, 2026 11:30
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 22, 2026

@claude

This comment has been minimized.

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 22, 2026

Claude finished @alejandro-jimenez-dcl's task in 3m 18s —— View job


Review Complete

  • Gather context (diff, changed files)
  • Load async-programming skill context
  • Review fix logic and root cause
  • Attempt test fix commit (blocked: repo requires signed commits)
  • Post final review with inline suggestion

Root-Cause Check

Problem: LoadingScreen.ExecuteOperationAsync called loadReport.SetResult(result.AsResult()) for all results. For a CancelledResult, AsResult() converts the cancellation into Result.ErrorResult("Cancelled: OperationCanceledException"), which SetResult wraps into new Exception(...). WaitUntilFinishedAsync's generic catch (Exception) branch then reported it to Sentry via ReportHub.LogException.

Fix: The PR correctly routes TaskError.Cancelled to loadReport.SetCancelled(), which propagates OperationCanceledException through the UniTaskCompletionSource. WaitUntilFinishedAsync catches OCE separately and suppresses it silently. This is a genuine root-cause fix, not a workaround.


Blocking Issue

Missing test update — FixUpLoadingReportAsync will fail for the CancelledResult case

Explorer/Assets/DCL/SceneLoadingScreens/Tests/LoadingScreenShould.cs:65

Before the fix, CancelledResult went through SetResult → SetException, producing UniTaskStatus.Faulted. After the fix it goes through SetCancelled → TrySetCanceled, producing UniTaskStatus.Canceled. The existing assertion only distinguishes between Succeeded and Faulted:

// current — fails for CancelledResult
Assert.That(outerReport!.GetStatus().TaskStatus,
    result.Success ? Is.EqualTo(UniTaskStatus.Succeeded) : Is.EqualTo(UniTaskStatus.Faulted));

Required fix (line 64–65):

// Report must be modified by the loading screen
UniTaskStatus expectedStatus = result.Success
    ? UniTaskStatus.Succeeded
    : result.Error?.State is TaskError.Cancelled
        ? UniTaskStatus.Canceled
        : UniTaskStatus.Faulted;
Assert.That(outerReport!.GetStatus().TaskStatus, Is.EqualTo(expectedStatus));

Note: I attempted to commit this fix directly but the repository requires signed commits. Please apply the change above to Tests/LoadingScreenShould.cs.

Fix this →


REVIEW_RESULT: FAIL ❌
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies async cancellation flow in the scene load pipeline, touching UniTaskCompletionSource state propagation.
QA_REQUIRED: YES

@m3taphysics
Copy link
Copy Markdown
Collaborator

PR #8858, run #26288527889

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

Framework 13 i7

Metric Change Baseline Delta Improvement
Samples 2701 2700
CPU average 33.3 ms 33.3 ms -0.0 ms 0.0%
CPU 1% worst 33.4 ms 33.7 ms -0.2 ms 0.7%
CPU 0.1% worst 33.8 ms 36.4 ms -2.7 ms 7.3% 🟢
GPU average 7.8 ms 8.2 ms -0.4 ms 4.7% 🟢
GPU 1% worst 19.4 ms 20.8 ms -1.4 ms 6.6% 🟢
GPU 0.1% worst 24.8 ms 25.8 ms -0.9 ms 3.6% 🟢

Copy link
Copy Markdown

@DafGreco DafGreco left a comment

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
  • [✔️ ] Settings

Checks made 🚀
Teleported through:

  1. Map ✔️
  2. Jump in button ✔️
  3. Commands on chat✔️
  4. Jump in from friends panel ✔️
  5. Teleport through jump in to genesis plaza button✔️

No new issues were found during this checkups

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