[PR #62] [MERGED] fix: surface billing failures instead of burying them #65

New issue

Closed

opened 2026-10-26 22:34:34 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:34:34 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-swift-sdk/pull/62
Author: @mattmillerai
Created: 8/25/2026
Status: ✅ Merged
Merged: 8/25/2026
Merged by: @mattmillerai

Base: main ← Head: fix/surface-payment-and-execution-errors

📝 Commits (1)

75d4c66 fix: surface billing failures instead of burying them

📊 Changes

5 files changed (+169 additions, -5 deletions)

View changed files

📝 Sources/ComfySwiftSDK/Internal/PollingFallback.swift (+24 -2)
📝 Sources/ComfySwiftSDK/Internal/ReattachCoordinator.swift (+1 -2)
📝 Sources/ComfySwiftSDK/Internal/Transport.swift (+21 -1)
📝 Sources/ComfySwiftSDK/Public/ComfyError.swift (+6 -0)
➕ Tests/ComfySwiftSDKTests/InsufficientCreditsTests.swift (+117 -0)

📄 Description

ELI5

Someone ran a generation on an account with no credits. What Comfy Cloud actually said was:

Payment Required: Please add credits to your account to use this node.

What the app could show was a generic failure — so the user had no idea the problem was billing, and went looking for a bug instead.

The message was there the whole time. This SDK threw it away, in three different places.

The three paths

1. checkStatus has no 402 branch. A 402 fell through to default: and became ComfyError.network(URLError(.badServerResponse)). So a billing failure was reported as a connectivity failure — the app tells the user to check their internet connection when the real problem is an empty balance. That's not just vague, it's actively wrong advice.

2. checkBody only inspected 400 and 422. A 402's body was never parsed, so even when the server explained itself in plain English the message was discarded before anything could match on it.

3. A failed job discarded execution_error entirely. PollingFallback and ReattachCoordinator both did:

case .failed:
    let phase = derivePhase(from: dto)
    continuation.yield(.failed(.jobFailed(phase: phase)))

JobDetailResponse.executionError.exceptionMessage — the field holding the quote above — was never read. This is the path the real incident took: a partner node refused mid-run, and the consumer received nothing but "the job failed".

The fix

Adds ServerRejectionReason.insufficientCredits, and classifies to it from all three paths: 402 by status, a payment message by body, and a failed job by its execution error.

It's deliberately distinct from quotaExceeded, because the advice differs. A quota resets when the billing period rolls over — the user waits. Credits clear only by topping up — the user acts. Collapsing them would send half of these users to wait for something that is never going to happen.

The credit match also runs before the quota match, for two reasons:

The real message contains none of quota / limit exceeded / billing, so it previously fell all the way through to .other and rendered as generic copy.
Wording like "add credits to your billing account" contains billing, so quota-first would silently miscategorise it.

Unrelated execution failures keep the existing .jobFailed(phase:) shape — this only promotes failures it can positively identify.

Testing

245 tests pass (6 new, in InsufficientCreditsTests), covering each path: 402 status, payment message in the body, credit-wording-beats-quota, a partner node refusing mid-run, and the two negative cases — an unrelated execution error and a failure with no execution_error at all, both of which must still report .jobFailed.

Breaking-change note

ServerRejectionReason isn't frozen, but adding a case still breaks exhaustive switches over it in consumers. That's deliberate: the whole point is to make each consumer decide how to present a billing failure rather than let it fall into a generic bucket. In Comfy-iOS this surfaces as one compile error in ErrorPresentation, which is the file that should be handling it.

🤖 Generated with Claude Code

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/comfy-swift-sdk/pull/62 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/25/2026 **Status:** ✅ Merged **Merged:** 8/25/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `fix/surface-payment-and-execution-errors` --- ### 📝 Commits (1) - [`75d4c66`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/75d4c66da3470599cdae22e0200040eb1534c76c) fix: surface billing failures instead of burying them ### 📊 Changes **5 files changed** (+169 additions, -5 deletions) <details> <summary>View changed files</summary> 📝 `Sources/ComfySwiftSDK/Internal/PollingFallback.swift` (+24 -2) 📝 `Sources/ComfySwiftSDK/Internal/ReattachCoordinator.swift` (+1 -2) 📝 `Sources/ComfySwiftSDK/Internal/Transport.swift` (+21 -1) 📝 `Sources/ComfySwiftSDK/Public/ComfyError.swift` (+6 -0) ➕ `Tests/ComfySwiftSDKTests/InsufficientCreditsTests.swift` (+117 -0) </details> ### 📄 Description ## ELI5 Someone ran a generation on an account with no credits. What Comfy Cloud actually said was: > `Payment Required: Please add credits to your account to use this node.` What the app could show was a generic failure — so the user had no idea the problem was billing, and went looking for a bug instead. The message was there the whole time. This SDK threw it away, in three different places. ## The three paths **1. `checkStatus` has no 402 branch.** A 402 fell through to `default:` and became `ComfyError.network(URLError(.badServerResponse))`. So a billing failure was reported as a *connectivity* failure — the app tells the user to check their internet connection when the real problem is an empty balance. That's not just vague, it's actively wrong advice. **2. `checkBody` only inspected 400 and 422.** A 402's body was never parsed, so even when the server explained itself in plain English the message was discarded before anything could match on it. **3. A failed job discarded `execution_error` entirely.** `PollingFallback` and `ReattachCoordinator` both did: ```swift case .failed: let phase = derivePhase(from: dto) continuation.yield(.failed(.jobFailed(phase: phase))) ``` `JobDetailResponse.executionError.exceptionMessage` — the field holding the quote above — was never read. This is the path the real incident took: a partner node refused mid-run, and the consumer received nothing but "the job failed". ## The fix Adds `ServerRejectionReason.insufficientCredits`, and classifies to it from all three paths: 402 by status, a payment message by body, and a failed job by its execution error. It's deliberately **distinct from `quotaExceeded`**, because the advice differs. A quota resets when the billing period rolls over — the user waits. Credits clear only by topping up — the user acts. Collapsing them would send half of these users to wait for something that is never going to happen. The credit match also runs **before** the quota match, for two reasons: - The real message contains none of `quota` / `limit exceeded` / `billing`, so it previously fell all the way through to `.other` and rendered as generic copy. - Wording like "add credits to your billing account" contains `billing`, so quota-first would silently miscategorise it. Unrelated execution failures keep the existing `.jobFailed(phase:)` shape — this only promotes failures it can positively identify. ## Testing 245 tests pass (6 new, in `InsufficientCreditsTests`), covering each path: 402 status, payment message in the body, credit-wording-beats-quota, a partner node refusing mid-run, and the two negative cases — an unrelated execution error and a failure with no `execution_error` at all, both of which must still report `.jobFailed`. ## Breaking-change note `ServerRejectionReason` isn't frozen, but adding a case still breaks exhaustive switches over it in consumers. That's deliberate: the whole point is to make each consumer decide how to present a billing failure rather than let it fall into a generic bucket. In Comfy-iOS this surfaces as one compile error in `ErrorPresentation`, which is the file that should be handling it. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:34:34 +00:00