Comfy-Org / Comfy-Org/ComfyUI_frontend

Downgrade reactivation emits 2 started + 3 phantom failed per successful attempt

Open
#14,921 1 comment 1 reaction 1 assignee Claimed by @dante01yoon View on GitHub
Potential Bug
Dominant language
TypeScript
Stars
2k
Forks
699
Avg merge
1d 7h
Merged PRs (30d)
490

Description

Follow-up from the re-review of https://github.com/Comfy-Org/ComfyUI_frontend/pull/14216. Approved that PR rather than holding the rollout, so tracking the remainder here.

## Problem / Goal

`useDowngradeToPersonal.ts` emits phantom failure telemetry for a continuation state, which the [Telemetry Lifecycle SSOT](https://app.notion.com/p/3b56d73d365081158a55c2e16bc81353) explicitly forbids: "`REACTIVATION_CONFIRMATION_REQUIRED` is a continuation state, not a failure. It must not emit a phantom failed event before confirmation continues."

Chain:

1. `:292-299` catches `REACTIVATION_CONFIRMATION_REQUIRED`, sets `telemetryFailure`, and throws `ReactivationConfirmationRequiredError`.
2. That propagates to the outer catch at `:371`, which emits `downgrade_to_personal.failed` at `:375`.
3. Because `checkoutStartedAt` was already set at `:263`, it also emits `subscription_checkout.failed` at `:386` and `operation.failed` at `:397`.
4. `dialogService.ts:768-778` re-prompts and retries, producing a second `downgrade_to_personal.started`.

Net: **two `started` and three phantom `failed` events per successful reactivating downgrade.** Downgrade and team-to-personal conversion will read far worse than reality, and the failure mix will be dominated by a category that is not a failure.

The same defect was already fixed for workspace checkout at `useSubscriptionCheckout.ts:719-722` by moving the continuation test ahead of `trackSubscriptionFailure`. The downgrade path needs equivalent treatment.

## Proposed Solution

Let the continuation error bypass the terminal-emitting catch rather than flow through it — either rethrow it in a form the outer catch recognises and skips, or hoist the continuation check above the block that owns terminal emission, mirroring the checkout fix.

Also close the two coverage gaps that let this through:

- `useDowngradeToPersonal.test.ts:341` asserts only the thrown error and never the absence of `failed`. The checkout equivalents (`useSubscriptionCheckout.test.ts:1774`, `:2597`) do assert it.
- The `poll_timeout` specialized terminals added at `billingOperationStore.ts:558-599` have no unit coverage; only the generic `stage: 'timeout'` is asserted (`billingOperationStore.test.ts:1018`).

## Acceptance Criteria

- [ ] A reactivating downgrade that ultimately succeeds emits exactly one `downgrade_to_personal.started` and exactly one terminal, and no `failed` for any of the three families.
- [ ] A test asserts the absence of `stage: 'failed'` across the reactivation continuation on the downgrade path, and fails against current `fix/incident-88-telemetry-gaps`.
- [ ] Poll timeout emits the specialized `failed` with `failure_category: 'poll_timeout'` under test, for `subscription_checkout`, `topup`, and `downgrade_to_personal`.
- [ ] SSOT updated in the same PR if any lifecycle semantics change.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.