fix: stop waiting when a credits checkout is cancelled - #9767
fix: stop waiting when a credits checkout is cancelled#9767juanmahidalgo wants to merge 1 commit into
Conversation
🚦 CI StatusBuild skipped — no changes detected under Warnings not reduced: 13156 => 13165 — remove at least 10 warnings to merge. Warnings/errors in files changed by this PR (4)
|
|
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: stop waiting when a credits checkout is cancelled
STEP 2 — Root-cause check
PASS. The PR correctly identifies the root cause: the polling loop in PollOrderAsync only broke on credited or failed, so a server-side abandoned status (checkout expired or buyer clicked back on Stripe) was indistinguishable from processing. The poll would spin for the entire 60s foreground + 10min background window before giving up. Adding STATUS_ABANDONED to the wire contract and mapping it through the state machine is the right fix.
STEP 3 — Design & integration
PASS. The design decisions are well-justified:
AbandonedvsFailed: correct separation.Abandonedmeans nobody was charged and nothing broke;Failedmeans something went wrong (e.g. card declined). Different semantics → different stage → different UI copy. The factory correctly sets no error and skips the balance re-read.AbandonedvsCancelled: correct separation.Cancelledmeans the local user closed the modal (CancelTopUpowns the state transition);Abandonedmeans the server retired the checkout session. ReusingCancelledwould have conflated server-side status with client-side intent.- Reusing
ModalState.Failedpanel: pragmatic. Avoids a prefab change for what amounts to different copy in the same layout. The STAGE stays distinct downstream.
No new long-lived units, systems, or controllers are introduced. The changes extend existing types with a new enum value and its handling — no lifecycle or owner-search concerns.
STEP 4 — Member audit
New members introduced:
STATUS_ABANDONED(const) — consumed byPollOrderAsyncswitch. ✅PollOutcome.Abandoned(enum) — consumed byRunTopUpAsyncswitch. ✅CreditsTopUpStage.Abandoned(enum) — consumed byMapStage,ApplyStatus. ✅CreditsTopUpStatus.Abandoned(pack, orderId)(factory) — consumed byRunTopUpAsync. ✅
All new members have exactly the consumers they need. No single-use or redundant predicates.
STEP 5 — Line-level review
[P1] Bug: AcknowledgeTerminalState() does not handle the new Abandoned stage
📍 CreditsTopUpService.cs line 98
Abandoned is a terminal state (the checkout is permanently retired — no payment can ever arrive). But AcknowledgeTerminalState() only resets for Credited or Failed:
// Current (line 98):
if (CurrentStatus.Stage is CreditsTopUpStage.Credited or CreditsTopUpStage.Failed)So the service gets stuck in Abandoned forever.
Consequences:
- Retry breaks. The modal maps
Abandoned→ModalState.Failed, soOnRetryClickedpasses its guard and callsAcknowledgeTerminalState()— which silently does nothing. The UI stays on the Failed panel; the user is stuck. - Close-and-reopen breaks.
OnViewClosecallsAcknowledgeTerminalState()forModalState.Failed, which again does nothing. Next open re-shows the Abandoned panel.
Fix:
if (CurrentStatus.Stage is CreditsTopUpStage.Credited or CreditsTopUpStage.Failed or CreditsTopUpStage.Abandoned)The existing ResetTerminalStateToIdleOnAcknowledge test covers Failed; a parallel test for Abandoned should be added to lock this down.
No other issues found. No security vulnerabilities, no resource leaks, no nullability violations, no naming or style issues. The test is well-structured (AAA pattern, NUnit + NSubstitute), and the comments are informative without being excessive.
STEP 6 — Complexity
SIMPLE — 5 files, +54 −0 lines, pure additions of a new enum value and its handling. No ECS, async, or architectural changes.
STEP 7 — QA
QA_REQUIRED: YES — changes affect runtime UI behavior (modal state machine, user-visible copy).
STEP 8 — Non-blocking warnings
None. Main scene not modified.
Security review
No security issues found. The abandoned status string comes from the existing authenticated API. No injection vectors (compared against a const, not interpolated). No auth/authz changes. No sensitive data exposure.
REVIEW_RESULT: FAIL ❌
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Pure addition of a new status enum value through the existing credits top-up state machine, no structural or architectural changes.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by unknown via Slack
|
PR #9767, run #32021753857 Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Intel Core i5
Exception breakdown
Apple M1
|
|
Closing in favor of #9768 |
Closes #9737 (the client half).
What was wrong
CreditsOrderStatusResponsedeclared three statuses —processing,credited,failed— and the poll only breaks on the last two. The server has always been able to answerabandonedfor a checkout retired without payment; its own comment says "a poll that saw it as 'processing' would wait forever".So a cancelled purchase was indistinguishable from one still in flight, and the modal span for the full 60s foreground + 10min background window before giving up.
The change
STATUS_ABANDONEDadded to the wire contract.PollOutcome.Abandoned, kept separate from the existingCancelled— that one means this local operation was aborted (the user closed the modal) and carries the rule that "whoever cancelled owns the status". Reusing it would have silently swallowed the new case.CreditsTopUpStage.Abandonedwith its own factory, kept apart fromFailed: nobody was charged and nothing broke, so it must not be reported as an error.Needs the server change to be useful
This reads a status that, today, nothing sets in time: the order only becomes
abandonedwhen the timed sweep retires it ~26h later. decentraland/credits-server#587 makes Stripe'scancel_urlrecord it immediately, so clicking back flips the status within one poll (~1.5s).Merging this alone is harmless but changes nothing observable.
Deliberately not done
BuyCreditsFailedwould file a cancellation as a failure and skew the funnel; a cancelled step deserves its own event, which is a separate decision.Test plan
abandonedpoll lands on the Abandoned stage, re-reads no balance and reports no error