paykit: finishPayment can re-queue a just-paid request, enabling a second payment

[jvsena42]

Found during review of #722. The two implicated files are byte-identical between origin/master and that PR's head, so this is master's, not #722's — it only showed up in that diff because the PR's base branch lags master. All line numbers below are at master 5ea3cf3d.

Gated behind PaykitFeatureFlags.isUIEnabled (default false, Dev Settings only), so this is dev/QA-facing today. It becomes user-facing when the flag flips.

What happens

finishPayment re-adds a request to pendingRequests by looking for an .accepted entry in local history. That is deliberate — it is how "user opened the Send sheet and closed it without paying" keeps the request visible, and PaykitPaymentRequestServiceTests.swift:1554-1556 pins exactly that behaviour.

The defect is that it cannot tell that case apart from "user paid, and the proof has already been deleted". Both look identical by the time the guard runs.

The chain

PaykitPaymentRequestService.swift:1252-1271:

1256        let acceptedRequest = historyRequests.first(where: {
1257            $0.id == request.id && $0.direction == .incoming && $0.lifecycleState == .accepted
1258        })
...
1261        async let completed = completedPaymentProofKinds(activeIdentity)
1262        async let inFlight = inFlightPaymentRequestIds(activeIdentity)
1263        let (completedProofKinds, inFlightRequestIds) = await (completed, inFlight)
1264        let protectedRequestIds = Set(completedProofKinds.keys).union(inFlightRequestIds)
1265        guard !protectedRequestIds.contains(request.id),
1266              !pendingRequests.contains(where: { $0.id == request.id })
1267        else { return }
1268
1269        pendingRequests.append(acceptedRequest)

Two things make the guard ineffective after a successful payment:

1. The .accepted entry is one the app wrote itself. perform() at :1502-1505 does invalidateRefresh(), then removes and re-inserts the request into historyRequests with the locally-computed resultingState. Nothing updates that entry until the next completed refresh. The SDK itself moves the record to ProofSubmitted as soon as the proof is enqueued, so after any completed refresh the lookup at :1256 correctly finds nothing — the bug only lives inside the window.

2. Both protection sources are backed by local proofs that submit() has already deleted. The default closures at :796-801 delegate to PaykitPaymentProofService.completedRequestProofKindsAwaitingSubmission (:516-526, keyed on proof.proofData != nil) and .inFlightRequestIds (:528-534, keyed on proof.paymentStarted). Both read loadProofs(). submit() calls await removeRequestProofs(pendingProof) at PaykitPaymentProofService.swift:621, so by the time :1263 awaits, there is no proof left to protect the request and :1265 passes.

Trigger is SendSheet.swift:338-344, cleanup() on .onDisappear:

342        if let request = incomingPaymentRequest ?? app.contactPaymentContext?.incomingPaymentRequest {
343            Task { await paykitPaymentRequestManager.finishPayment(request) }

The race

Proof deletion at :621 fires proofStateChangedSubject (:718), which starts a refresh via AppScene.swift:258-259. Two interleavings leave the stale append standing:

• finishPayment passes :1256, and the in-flight refresh assigns pendingRequests (:1407) during finishPayment's await at :1263. The append then lands after the refresh and survives it.
• A poll refresh whose synchronize() snapshot predates the proof but whose proof lookups (:1345-1347) run after the :621 deletion. refresh() at :951-953 coalesces onto that in-flight task, so the proof-triggered refresh does not take a fresh snapshot.

If the refresh lands cleanly before or after the whole of finishPayment, :1407 wipes the entry and nothing goes wrong. The window spans several network calls, so it is reachable in normal use, but reproducing it on demand needs instrumentation.

Why it can lead to a second payment

The re-queued request is presented again (:1507 clears presentedRequestIds, so it re-auto-presents if it went .proposed → .accepted this session). A second payment does not fire straight away: re-presentation goes through beginContactPayment with afterPrivatePaymentListVersion: consumedVersion, and consumePrivatePaymentList (PrivatePaykitService+Payments.swift:148-163) recorded the version at the first pay, so it defers with .waitingForUpdatedPaymentList until the payee publishes a strictly newer list.

But given payee republish plus a fresh invoice plus the user confirming a second Send sheet, it is a real second payment on a new invoice — no LDK duplicate-hash protection applies. perform() at :1476-1482 only checks expiry (isExpired is .proposed-only) and pendingRequests membership, and prepareForPayment skips service.accept because requiresAcceptance == false for .accepted. The second proof then wedges locally, because the SDK's require_state(&[Accepted, ActiveRecurring]) rejects a proof for a record already in ProofSubmitted.

Suggested fix

Gate the re-queue on the SDK record state rather than the locally mutated history entry — for example only append when service.synchronize().incoming still contains request.id. Alternatively keep a proof tombstone until the post-submit refresh completes, so the :1265 guard still has something to match on.

Either way the "closed the sheet without paying" case that PaykitPaymentRequestServiceTests.swift:1553-1556 covers needs to keep working — that is the behaviour this code exists for.

Notes

• Not reproduced on device; found by reading and traced through master. The window is narrow enough that a device repro would need instrumentation.
• Android may or may not share this. Its refresh does not have the equivalent await between list replacement and the requested-id check, so the specific interleaving above does not port directly — worth a separate look rather than assuming parity.