core, tests: warn about a spent code only when one was sent

plans/badges-codes/2026-08-21-badges-web-checkout.md

@@ -1451,7 +1451,9 @@ Append here when a step contradicts this plan: the step id, what was wrong, and
 - **C5 fix round — three comments corrected, one plan cross-reference added.** The happy-path example claimed that verifying the stored credential proved the client had checked it before storing; it does not (the real service only signs valid credentials), and the comment now points at `testC5UnverifiableCredentialWritesNothing` and its mutation, which is the actual proof. The catalog example now names the six-line comparison as what catches `catalogRequest` starting to sign. And H2's **Do** now carries the instruction to re-point `testC5SameCodeAgainThenUnredeemed` at `codes unredeem` when that command lands, instead of that depending on H2's author reading §9.
 
 - **Phase C review — `purchaseBadge{code}`'s replay path is unreachable from the shipped client, and a timeout during redemption therefore burns a paid code.** RPC §Idempotency and §10's second invariant both read as if a client could safely repeat a redemption after a lost reply. It cannot. `redeemBadgeCode` mints a fresh purchase keypair and master key on **every** call and persists nothing before the send (C1's "nothing is persisted before the send" rule, which `initial_badge_type NOT NULL` forces), and the service's replay path keys on the purchase key: `classifyRow` answers `RedeemAlreadyRedeemedBySameKey` only when `redeemerKey == Just purchaseKey`, and `planTxn`'s own comment says "C4 mints a fresh key per redemption, so there is no purchase row". So a retry arrives as a **different signer** and can only ever be answered `code_used` — on a code the user paid for. **Consequence:** a redemption whose reply is lost consumes the code with nothing written on the client, and neither the app nor the user can recover it. The only recovery is the operator's `codes unredeem` (H2), which is why H2's **Do** now takes that command first and G2's and G3's steps now forbid offering a retry of the same code after a timeout. Documentation only: persisting the purchase key before the send would overturn C1's rule and is a design change, not a fix. Note what is **not** affected: `issueBadge`'s `IssueCached` path is reachable and exercised — the worker signs with the purchase key stored on the row, so its retry after a lost reply is the same signer and is answered with the cached credential (B10's `testBadgeServiceIssueBadgeCachedInLastFundedMonth`). It is `purchaseBadge{code}`'s replay alone that no shipped client can reach.
-- **Phase C review — `sendBadgeRequest` told the user a timeout had not been delivered. Fixed.** `tryAllErrors (sendRequest target)` collapsed four outcomes into one sentence — "could not be reached, and the request was not delivered" — for an unconfigured address, a pre-delivery transport failure, an **agent timeout** and an undecodable response. For the timeout that claim is affirmatively false: the request may have reached the service and consumed the code. G2 renders this exact `message` for an `internal` code, so combined with the entry above it steered the user into the retry that reports `code_used`. The timeout now has its own branch, matched on the constructor path (`ChatErrorAgent {agentError = AGENT (A_SERVICE ASETimeout)}`, which is what `withAgent`'s `chatErrorAgent` wraps `serviceRequest`'s timeout in) rather than on a rendered string, and its own sentence: the request may still have been delivered, a code presented with it may already have been used, entering it again will not help, contact support. No catch-all was added — every other error still reports non-delivery, which for them is true. `testC5ServiceStoppedIsInternal` was asserting the false sentence (nothing is listening, so it times out); it now asserts the timeout line in full, spelled out rather than derived from the function under test.
+- **Phase C review — `sendBadgeRequest` told the user a timeout had not been delivered. Fixed.** `tryAllErrors (sendRequest target)` collapsed four outcomes into one sentence — "could not be reached, and the request was not delivered" — for an unconfigured address, a pre-delivery transport failure, an **agent timeout** and an undecodable response. For the timeout that claim is affirmatively false: the request may have reached the service and consumed the code. G2 renders this exact `message` for an `internal` code, so combined with the entry above it steered the user into the retry that reports `code_used`. The timeout now has its own branch, matched on the constructor path (`ChatErrorAgent {agentError = AGENT (A_SERVICE ASETimeout)}`, which is what `withAgent`'s `chatErrorAgent` wraps `serviceRequest`'s timeout in) rather than on a rendered string, and its own sentence. No catch-all was added — every other error still reports non-delivery, which for them is true. `testC5ServiceStoppedIsInternal` was asserting the false sentence (nothing is listening, so it times out); it now asserts the timeout lines in full, spelled out rather than derived from the function under test.
+
+  **The timeout has two sentences, not one, and only `purchaseBadge` warns about the code.** The first version of this fix warned about a spent code on *every* timeout, which the re-review caught: `sendBadgeRequest` serves three call sites and two of them carry no code — `APIGetBadgeCatalog`, and the worker's `issueBadge`, whose `BSPError` reaches the UI through `storeBadgeIssueResponse`'s `eToView`. Telling a user on the catalog screen that their code may already have been used is false and alarming, and the right advice there is to try again. `requestFailed` now branches on the request being `BSCPurchaseBadge`: that one keeps the code warning and the support contact, and every other command gets "nothing was spent, so you can try again". G2/G3 render whichever they are given verbatim, so the distinction has to be made here rather than in the apps.
 - **Phase C review — the `internal` code is overloaded, and G2/G3 cannot tell the two kinds apart from the code alone.** `docs/protocol/badges-rpc.md:64` makes every code other than the marked transient ones "terminal for the attempted command", and `internal` is one of them. But the client emits `BSEInternal` for its **own** local failures too (`localBadgeError`, `sendBadgeRequest`): an unconfigured service address, a transport failure, an undecodable response, a credential that fails verification, a store payment method. Their retryability differs per command and none of them is the service's `internal`. No new error code was added — that would be a protocol change (§6, and it is explicitly out of this round's scope). What distinguishes them is the `message`: the service sends `internal` with no message, and every client-local failure carries a non-empty one. **G2 and G3 must treat a non-empty `message` on an `internal` as the local kind** and render it, which they already do for the blank-screen reason; this entry is the record of *why* that rule exists rather than being a fallback.
 - **Phase C review — `APIGetBadgeState`'s worker signal could hang the command thread forever. Fixed.** The second branch of the race the C3 entry above records, and the worse one. `getAgentWorker'` reads the map and calls `runWorker` in two separate `atomically` blocks (`Agent/Client.hs:408`); `cancelWorker` (`:1005-1007`) empties the worker's `action` TMVar without replacing it; `runWorkerAsync` (`:463-465`) opens with a blocking `takeTMVar action`. So `APIGetBadgeState` could read worker *W* from the map, `stopChatController` could swap the map and cancel *W*, and the command thread would then block on `runWorkerAsync W` with nothing left to put the TMVar back. Swapping the map before cancelling — which `stopChatController`'s comment says fixes the next caller — does not close the in-flight window. The shape is pre-existing in the agent (`disconnectAgentClient` does the same over `smpDeliveryWorkers`) and **simplexmq is deliberately not changed**; what is new is the exposure, since every other `getAgentWorker` caller runs from a start or a background loop while this one runs synchronously on a user-facing command thread that G1 and G3 hit on every badge-screen open — on mobile, a hung `chatSendCmd`. The signal is now detached (`forkIO`), which turns the hang into a dropped signal: harmless, because the pass is idempotent and the worker's own timer re-fires.
 - **Phase C review — "unreachable via C1's `createPurchase`, which always inserts an opening credit" named a reason that does not exist. Corrected in place.** `Store/Badges.hs`'s `createPurchase` inserts the `badge_purchases` row and nothing else, and nothing in this milestone writes an opening credit at all (§9, above). The `issueDueBadgePeriod` case *is* unreachable, for a different reason: `redeemBadgeCode` is the only writer of a purchase row and of `users.shown_badge_id`, it rejects a response whose statement carries no issued period (`issuedBadgePeriod`) **before** it writes anything, and it inserts that statement's entries in the same transaction as the purchase row — so a shown purchase always has ledger rows. The comment now says that. A reason that does not hold is worse than no reason: the next reader would have looked for the opening credit.
@@ -1466,7 +1468,7 @@ Append here when a step contradicts this plan: the step id, what was wrong, and
 - **Phase C review — the generated clients carry no badge types, and §9's "belongs with C4" was not honoured.** `APIGetBadgeCatalog`, `APIGetBadgeState` and `APIPurchaseBadge` are in `undocumentedCommands`, `CRBadgeState` and `CRBadgeCatalog` in `undocumentedResponses`, and `CEvtBadgeChanged` in `undocumentedEvents` (`bots/src/API/Docs/{Commands,Responses,Events}.hs`) — all exemptions from `tests/APIDocs.hs`'s completeness check. So `bots/api/TYPES.md`, `types.ts` and `_types.py` carry only `BadgeServiceErrorCode` and `CEBadgeServiceError`, and `UserBadge`, `UserBadgeState`, `CRBadgeState`, `CRBadgeCatalog` and `CEvtBadgeChanged` reach no generated client. The C2 entry above said documenting the three commands "belongs with C4, when they do something"; C4 made them do something and did not document them. **Assigned to a G step** — G4 is the natural one, since it is the first step that reads `CRBadgeCatalog` and `CRBadgeState` field by field and would notice a wrong shape. Moving a constructor out of an `undocumented*` list regenerates `COMMANDS.md`, `EVENTS.md`, `TYPES.md`, `types.ts` and `_types.py`, and `describe "Bot API docs"` must pass afterwards.
 - **Phase C review — mobile cannot configure the badge service at all, which makes G2's and G3's Verify lines impossible as written. Assigned to G0.** `defaultChatConfig` sets `badgeServiceAddress = Nothing` and `badgeWebBaseUrl = ""`; `defaultMobileConfig` (`Mobile.hs`) overrides three unrelated fields and none of these; and `mobileChatOpts` hardcodes `optBadgeServiceAddress = Nothing`, `optBadgeWebUrl = Nothing` and `optBadgeIssuerKeys = []`. The three overrides exist only on the terminal CLI (`--badge-service-address`, `--badge-web-url`, `--badge-issuer-key`), so "verify manually against a locally run service" cannot be done on iOS or Android by any means the client offers. G0 is the assignment: it is the first mobile step, it lands before G2 and G3 need it, and until it does every mobile Verify line in Phase G is unrunnable. Whatever shape it takes — build-time constants, a debug-only setting, `mobileChatOpts` parameters — the three values must be reachable from a mobile build.
 - **Phase C review — `redeemBadgeCode` reads the reported badge state outside the badge lock. Accepted, not fixed.** The lock is released after `storeRedeemedBadge` and before `presentUserBadgeToContacts` (it must be: `presentUserBadgeToContacts` takes `chatLock`), and `getUserBadgeState` for the response runs after that. A concurrent redemption on the same profile could supersede this one in between, so the response would name the *other* purchase as shown. Cosmetic: both purchases are stored correctly, the rows are right, and the next `APIGetBadgeState` reports the truth. Fixing it would mean either computing the response inside the lock — which is a different value from the one the app will read next — or widening the lock over `chatLock`, which is the inversion C3 exists to avoid.
-- **Phase C review — two one-line inconsistencies fixed.** `hasIssuanceForPeriod` (`Store/Badges.hs`) used a bare string literal for its query where every other query in that module uses `[sql| |]`; it was the only one, and it is now the same as the rest. And `runBadgeWorker`'s haddock did not mention the `waitChatStartedAndActivated` gate the C3 review round added to the top of its loop, which is the one thing about that loop a reader most needs to know after a suspend; it now names it and the three sibling loops that gate the same way.
+- **Phase C review — a haddock gap fixed, and a style "fix" reverted as wrong.** The review reported `hasIssuanceForPeriod` (`Store/Badges.hs`) as the module's only query not using `[sql| |]`, and it was reformatted to match. That premise was false and the re-review caught it: the module carries bare-string queries at four other sites against six `[sql| |]` blocks, and the convention is by query length — `[sql| |]` for multi-line or multi-column statements, a plain string for a one-liner. Under the real convention the original was already consistent, and the reformat made a one-line `SELECT EXISTS` span five lines, so it was reverted. Recorded because two successive reviews asserted the convention without reading the module for it. And `runBadgeWorker`'s haddock did not mention the `waitChatStartedAndActivated` gate the C3 review round added to the top of its loop, which is the one thing about that loop a reader most needs to know after a suspend; it now names it and the three sibling loops that gate the same way.
 
 ## 10. End-to-end verification
 

src/Simplex/Chat/Library/Commands.hs

@@ -5491,16 +5491,26 @@ sendBadgeRequest nm user signKey req =
     -- message (G2), and a bare 'Show' of a 'ChatError' is not something to put in front of a user
     --
     -- the agent's request timeout gets its own sentence, because it is the ONE outcome here that
-    -- may have been DELIVERED: the service can have received the request and consumed the code,
-    -- with only the reply lost. Saying "not delivered" would steer the user into re-entering the
-    -- same code, and 'redeemBadgeCode' mints a FRESH purchase key per call, so the retry reaches
-    -- the service as a different signer and is answered @code_used@ (plan §9) — burning a code
-    -- they paid for. Everything else this collapses is a genuine non-delivery: an unresolvable
-    -- target, a transport failure before the send, or an agent error other than the timeout.
+    -- may have been DELIVERED: the service can have received the request, with only the reply
+    -- lost. Saying "not delivered" would be a claim this cannot make.
+    --
+    -- and only a timed-out 'BSCPurchaseBadge' warns about the code. That is the one request
+    -- carrying something spendable, and re-entering the code cannot help: 'redeemBadgeCode' mints
+    -- a FRESH purchase key per call, so the retry reaches the service as a different signer and is
+    -- answered @code_used@ (plan §9) — burning a code they paid for. A timed-out catalog fetch or
+    -- worker re-issue spends nothing, and both are safe to repeat, so telling a user on the
+    -- catalog screen that their code may be gone would be false and alarming.
+    --
+    -- everything else collapses to non-delivery: an unresolvable target, a transport failure
+    -- before the send, or an agent error other than the timeout.
     requestFailed = \case
-      ChatErrorAgent {agentError = AGENT (A_SERVICE ASETimeout)} ->
-        "The badge service did not answer in time. The request may still have been delivered, so a code presented with it may already have been used, and entering it again will not help. Please contact support."
+      ChatErrorAgent {agentError = AGENT (A_SERVICE ASETimeout)}
+        | isPurchase -> "The badge service did not answer in time. The request may still have been delivered, so the code may already have been used, and entering it again will not help. Please contact support."
+        | otherwise -> "The badge service did not answer in time. Nothing was spent, so you can try again."
       e -> "The badge service could not be reached, and the request was not delivered. Details: " <> tshow e
+    isPurchase = case req of
+      BadgeServiceRequest {request = BSCPurchaseBadge {}} -> True
+      _ -> False
     responseFailed e = "The badge service answered with something this app version cannot read. Details: " <> T.pack e
     badgeRequestFailed e = BSPError {code = BSEInternal, message = Just e, retryAfter = Nothing}
 

src/Simplex/Chat/Store/Badges.hs

@@ -386,9 +386,7 @@ hasIssuanceForPeriod db badgePurchaseId periodStart =
   fromOnly . head
     <$> DB.query
       db
-      [sql|
-        SELECT EXISTS (SELECT 1 FROM badge_issuances WHERE badge_purchase_id = ? AND period_start = ?)
-      |]
+      "SELECT EXISTS (SELECT 1 FROM badge_issuances WHERE badge_purchase_id = ? AND period_start = ?)"
       (badgePurchaseId, periodStart)
 
 -- Ledger ----------------------------------------------------------------------

tests/Bots/BadgeServiceTests.hs

@@ -2876,20 +2876,27 @@ testC5ServiceStoppedIsInternal ps =
     pk <- readTestIssuerPublicKey (tmpPath ps)
     let cfg = (badgeClientCfg sLink pk) {agentConfig = testAgentCfg {serviceRequestTimeout = 2}}
     withNewTestChatCfg ps cfg badgeClientDbPrefix aliceProfile $ \alice -> do
+      -- the catalog fetch spends nothing, so its timeout must NOT warn about a code
       alice ##> "/_badge catalog 1"
-      alice <## badgeTimeoutError
+      alice <## badgeRetryableTimeoutError
       alice ##> purchaseCodeCmd "C5-SERVICE-DOWN"
-      alice <## badgeTimeoutError
+      alice <## badgePurchaseTimeoutError
       noClientBadgeRows "with the service stopped" alice
 
--- | 'sendBadgeRequest''s timeout sentence, spelled out rather than derived from the function under
--- test: it carries no @Details:@ tail, so the whole line is compared.
-badgeTimeoutError :: String
-badgeTimeoutError =
+-- | 'sendBadgeRequest''s two timeout sentences, spelled out rather than derived from the function
+-- under test: neither carries a @Details:@ tail, so the whole line is compared. Only a timed-out
+-- @purchaseBadge@ warns that the code may be gone; every other command is safe to repeat.
+badgePurchaseTimeoutError :: String
+badgePurchaseTimeoutError =
   "badge service error: internal, The badge service did not answer in time."
-    <> " The request may still have been delivered, so a code presented with it may already have been used,"
+    <> " The request may still have been delivered, so the code may already have been used,"
     <> " and entering it again will not help. Please contact support."
 
+badgeRetryableTimeoutError :: String
+badgeRetryableTimeoutError =
+  "badge service error: internal, The badge service did not answer in time."
+    <> " Nothing was spent, so you can try again."
+
 -- Supersession and the slot -----------------------------------------------------
 
 -- | A second code redeemed on the same profile takes the slot: the first purchase becomes