fix(friends): OperationCanceledException-safe friends prewarm on reconnect#9422
fix(friends): OperationCanceledException-safe friends prewarm on reconnect#9422eordano wants to merge 1 commit into
Conversation
…nnect Production evidence (decentraland Sentry, archive snapshot 2026-07-17): - UNITY-EXPLORER-PFR: 1 events / 1 users, last seen 2026-07-10 - UNITY-EXPLORER-PF9: 1 events / 1 users, last seen 2026-07-06 - UNITY-EXPLORER-PE6: 1 events / 1 users, last seen 2026-07-01 - UNITY-TEST-ENVIRONMENT-1CK: 1 events / 1 users, last seen 2026-07-08 - UNITY-TEST-ENVIRONMENT-1F2: 1 events / 1 users, last seen 2026-06-30 - UNITY-TEST-ENVIRONMENT-1EE: 1 events / 1 users, last seen 2026-06-19
|
Slack notification sent to #explorer-ext-contributions for external review. |
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — fix(friends): OperationCanceledException-safe friends prewarm on reconnect
STEP 2 — Root-cause check: PASS ✅
The PR fixes unhandled OperationCanceledException in a detached UniTaskVoid flow (PrewarmAsync). The token (prewarmFriendsCancellationToken) is SafeRestart()ed on reconnect, which cancels any in-flight prewarm. Without the try-catch, the cancellation exception propagates unhandled from the fire-and-forget call, causing the Sentry crashes listed in the PR description. This is the correct fix — the root cause is the missing exception handling in a detached async flow, not a symptom workaround.
STEP 3 — Design & integration: PASS ✅
No new long-lived units introduced. The change adds a try-catch inside an existing local function (PrewarmAsync). No lifecycle duplication, no per-frame reconciliation, no persistent state outside ECS. The cancellation-on-reconnect flow is correct: SafeRestart() cancels any prior prewarm, the catch swallows the expected OperationCanceledException, and the Unsubscribe is correctly skipped on cancellation (so the next loading-complete event re-triggers the prewarm).
Teardown trace: The prewarmFriendsCancellationToken CTS is SafeCancelAndDispose()d in Dispose() (line ~156) — no leak.
STEP 4 — Member audit: N/A
No public properties or accessors added or changed.
STEP 5 — Line-level findings
| # | Severity | File | Line | Issue |
|---|---|---|---|---|
| 1 | P2 | FriendsContainer.cs |
249 | Missing general exception catch per CLAUDE.md §9 |
See inline comment with suggestion below.
Security review
No security issues found.
CI status
semantic / title-matches-convention— ❌ failingBuild (windows64),Build (macos)— ⏳ pendingTest (editmode),Test (playmode),Lint— ⏳ pendingQA and DEV Approvals— ❌ waiting on approvals
STEP 6–9 — Verdict
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Single-file try-catch addition in an existing async local function; no ECS, system, or architectural changes.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by unknown via Slack
| // TODO should not unsubscribe as the user can re-login with another account, and thus, pre-warming will be skipped | ||
| loadingStatus.CurrentStage.Unsubscribe(PreWarmFriends); | ||
| } | ||
| catch (OperationCanceledException) { } |
There was a problem hiding this comment.
[P2] Missing general exception handler (CLAUDE.md §9).
The convention for detached UniTaskVoid flows requires: "Always catch exceptions: Ignore OperationCanceledException, Log/report all others via ReportHub.LogException."
The current catch handles cancellation correctly, but friendsPanelController.InitAsync(ct) could throw non-cancellation exceptions (network errors, deserialization failures, null references) that would still propagate unhandled — the same class of unhandled crash this PR aims to prevent.
Compare with the neighboring SyncBlockingStatusAsync which uses .SuppressToResultAsync(ReportCategory.FRIENDS) to handle both cases.
| catch (OperationCanceledException) { } | |
| catch (OperationCanceledException) { } | |
| catch (Exception e) { ReportHub.LogException(e, ReportCategory.FRIENDS); } |
Production evidence (decentraland Sentry, archive snapshot 2026-07-17):
Supersedes #9408: moved from the fork into the org repo so CI workflows receive repository secrets (fork PRs do not).