PLA-2686 | feat(android): name the transaction a failed payment belongs to - #77
Conversation
a928ce8 to
5cd698a
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Cancellation handling and published-constructor binary compatibility must be corrected.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds transaction identifiers to failed payment outcomes and recovery for captured card-present payments.
Changes:
- Exposes transaction IDs on card-present and card-not-present failures.
- Adds captured-payment recovery without another tap.
- Adds focused unit coverage for failure propagation and recovery.
File summaries
| File | Description |
|---|---|
TapToPayChargeRunnerTest.kt |
Tests transaction attribution and captured-payment recovery. |
PayabliTapToPayTest.kt |
Tests failure propagation through the public facade. |
TapToPayException.kt |
Adds transaction ID and captured status. |
TapToPayChargeRunner.kt |
Tracks failures and supports retrying captured closes. |
PayabliTTP.kt |
Exposes captured-charge recovery. |
MoneyInClientTest.kt |
Tests transaction IDs on refusal responses. |
PayInResults.kt |
Adds transaction ID to PayInFailure. |
MoneyInClient.kt |
Preserves response transaction IDs in failures. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
5cd698a to
f25039d
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Captured-payment recovery retains sensitive data unsafely, is lost with the runner lifecycle, and lacks operational instrumentation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
taptopay/src/main/java/com/payabli/sdk/taptopay/TapToPayChargeRunner.kt:55
PendingClosenow keepsCardReadResult.providerResponse—which contains the card expiry and processor token—in an immutableStringfor potentially the lifetime of the terminal. That value cannot be wiped after close and materially extends the lifetime of sensitive payment data. Refactor the retained response to encrypted/zeroizable storage and explicitly overwrite the in-memory buffer when it is closed or discarded.
private class PendingClose(
val paymentTransId: String,
val read: CardReadResult,
)
- Files reviewed: 8/8 changed files
- Comments generated: 3
- Review effort level: Balanced
Suppressed comment:
|
There was a problem hiding this comment.
🟡 Changes recommended
Public documentation overstates close certainty, repeats an incorrect payload claim, and close telemetry lacks focused coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
taptopay/src/main/java/com/payabli/sdk/taptopay/PayabliTTP.kt:60
- A failed update can have an unknown outcome: the backend may have closed the payment before the response was lost. This public method is the safe retry path for an unconfirmed close, rather than proof that the payment remains open; align this contract with that distinction.
taptopay/src/main/java/com/payabli/sdk/taptopay/TapToPayException.kt:27 capturedis set before the PATCH and remains true when every attempt loses its response, even if the service already applied the first PATCH. The SDK therefore knows that the card was charged and that closing was not confirmed, not that the payment is definitely still open. State that uncertainty so callers do not treat “open” as authoritative.
taptopay/src/test/java/com/payabli/sdk/taptopay/TapToPayChargeRunnerTest.kt:518- The reader answer retained by this runner does not contain an expiry or processor-minted token: the adapter explicitly omits those fields before constructing
CardReadResult(ChargeRecord.kt:24-25,FiservCardReaderGateway.kt:114-152). Keep this test explanation consistent with the actual redaction boundary.
- Files reviewed: 11/11 changed files
- Comments generated: 4
- Review effort level: Balanced
50f06c5 to
2b88fa1
Compare
Correction to my earlier answer on the suppressed commentI answered the retained-data comment by accepting its premise and deferring the fix. The premise was wrong, and so was my answer.
What I took the claim from is The deferral stands only for the narrower point that the contract lets an implementation forward whatever the processor answered, so the type cannot promise otherwise. Nothing about the shipped path needs a zeroizable buffer for the reason I gave. |
2b88fa1 to
ddbaa64
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Successful recovery retains the settled idempotency key, and capture status cannot represent an unknown post-reader outcome.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 3
- Review effort level: Balanced
Alex Arguello (alex-arguello)
changed the title
ddbaa64 to
a0a69fb
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Cancellation can return normally after protected closes, and several public contracts provide unsafe or inaccurate reconciliation guidance.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
taptopay/src/main/java/com/payabli/sdk/taptopay/TapToPayChargeRunner.kt:233
- The recovery path has the same cancellation hole: replacing the caller job with
NonCancellablecompletes the PATCH but can then return normally from a cancelledcloseCapturedChargecall. Explicitly check the restored caller context after the atomic close/settle/clear block so the public promise that cancellation unwinds remains true.
withContext(NonCancellable) {
client.update(pending.paymentTransId, pending.read)
// The same three as the charge's own close, for the same reason: a close that landed
// with its attempt still reserved leaves the next charge reusing a key the service has
// already seen, and one still held would be offered for closing again.
keys.settle(entry, pending.idempotencyKey)
pendingClose = null
}
- Files reviewed: 15/15 changed files
- Comments generated: 5
- Review effort level: Balanced
a0a69fb to
0a5588d
Compare
814b7b7 to
8ddeae2
Compare
…made MoneyInClient built its failure from the envelope's scalar half and dropped the payload, so a refusal that named a transaction lost it at that line. A refused transaction still exists at the paypoint, and the identifier is the only handle a caller has for reconciling, voiding or logging it. PayInFailure carries it, which reaches a caller through both PayInException subclasses and through PayInSubmissionState.Failed.cause, so no second field is needed on the published state. PayabliDeclineException drops it too and is left alone: it is only ever built from a 402, and a card-not-present decline arrives as a 2xx carrying a D code.
The close runs uncancellable now, for the reason the close after a failed tap already does: past the tap the money has moved, so a caller withdrawing mid-close left the payment open with nobody holding the answer. That is the state this path exists to avoid, and it was reachable on the charge's own close as well as on the later one. Cancellation still unwinds as cancellation. Wrapping it would report a withdrawn caller as a failed payment, and would hide it from the facade, which reads the type to decide what to rethrow. The fixture's transport takes a responder that cannot suspend, so the test for the first half brings its own, gated on the close.
Each argued for the line below it or restated it. What is left is the part that would be undone without it.
Closing a payment the card was charged for is money settling, and it ran unmeasured: the attempt that failed was recorded as a failed charge and nothing said whether it was closed afterwards or kept failing. Three events beside the charge's own, on the same shapes. The failure is immediate, because a payment left open with the card charged is the one an operator has to see now rather than at the next flush. The recovery also had no test for staying held after a failed attempt, so moving the clear into a finally would have made it one-shot and left the suite green.
…t is open A close that fails may have landed with its response lost, so nothing here can assert the payment is still open. Three places claimed it did. CardReadResult's own documentation was the fourth and the one that misled the others: it says the response carries the card's expiry and any token the processor minted, which the shipped adapter stopped putting in. That adapter forwards a named list of processor identifiers and approval fields, and the card, its holder, its expiry and its security code are not among them. The type still cannot promise that, because the contract permits an implementation to forward whatever the processor answered, so it says which half is the contract and which is this adapter.
…declared The catalog test checks that every name and key a site sends is allowed, so a site emitting the wrong event stays green. These name the events and the keys the close reports. Seen red by pointing closeSucceeded at the charge's event. One assertion is missing rather than written weakly: whether the close failure leaves immediately cannot be told apart here, because `forcesSend` already answers true from the failed outcome and `immediateEvents` is internal to the core module. A test through `forcesSend` would pass with the row deleted.
`a cancellation after the card is taken still closes the transaction` covers the same window on the charge path and covers it better: it cancels from inside the read, which lands in that window deterministically, where this one held the transport open and cancelled from outside. What is left of the gated transport is the recovery path, which nothing else reaches: a cancellation raised inside a later close has to unwind as one.
…that it did not A flag had two values for three states. The processor takes the sale before the reader hands the answer back, so a tap that ends without one may still have moved money, and that arrived as a definite not-charged. A host reading it retries a payment that may already have been taken. TapToPayCapture is NOT_CHARGED, UNKNOWN and CHARGED, and only the first means a retry cannot take it twice. The recovery also left its attempt reserved. The close resolves the transaction whenever it lands, so the attempt is over then too, and holding the key made the next charge reuse one the service had already seen. The key now rides on the held payment and is settled with it, inside the same uncancellable step as the close.
Four sentences claimed more than the code establishes, three of them mine from renaming the flag. A null identifier is not proof that nothing was opened: the call that opens a payment may have succeeded with its answer lost, which is the case the attempt is kept for. A close that failed may have landed with its response lost, so what is unknown is whether the service was told rather than whether the card was charged. And the class still told a host to retry any tap that did not complete, which for the unknown case is the retry that charges twice. One was a dangling link: the flag was renamed and its documentation was not.
A charge now refuses a customer that identifies nobody before the reader is touched, so eleven of these passed an empty one and were refused earlier than the line under test. None of them is about the customer, so each takes the payer the rest of the file already uses.
…r charged Asking to close a payment this terminal no longer holds went through the facade's default failure, which names no payment and reports not-charged. A host reaches that by persisting an identifier and coming back after a restart, and it is told a second charge is safe for a payment that may have taken money. It now carries the identifier it was given and unknown. The region moves off the instance and onto the entry point, which is what it protects. A terminal is built per call, so two exist for one paypoint whenever a screen is rebuilt, and they share the charge key by design; an instance mutex let one settle that key while the other was mid-charge. The concurrency test asserts the order the two payments reach the wire. Its first form asserted a mid-flight state and passed with the fix removed, because a second terminal that had not yet reached the opening satisfies it either way.
…made stale The regions were a map keyed on the entry point with nothing removed, and the entry point is a caller-supplied string that nothing bounds, so the map grew with whatever a host passed. A fixed set of striped locks cannot. Two unrelated entry points sharing a stripe now wait for each other, and one entry point still always resolves to one lock, which is the property the region exists for. The index is masked rather than negated because Int.MIN_VALUE has no positive counterpart and negating it returns itself. ChargeKeyStore's rationale for checking the key rather than assuming it said two terminals for one entry point hold separate charge locks. That stopped being true when the region moved off the instance. The check is still needed, for the reason that survives: this store is reached from paths the region does not cover, and a caller building its own is serialized by nothing this class owns.
…per terminal A terminal is built per call, so two exist for one paypoint whenever a screen is rebuilt, and they share the charge key. Held on the instance, the second terminal's payment hid the first's and both believed they held one; whichever settled the attempt first left the other to mint a fresh key and charge again. Sharing the region does not reach it, because the two never have to overlap. Keyed exactly rather than striped, since two entry points sharing a stripe share a lock and must not share a payment. It does not grow the way the regions would have: an entry appears only once a card has been charged for that paypoint, and it goes when the close lands or the next payment opens. The concurrency test starts the second terminal only once the first is inside its close. Its previous form released the gate before either child had run, so nothing was ever held and the ordering it asserted came from the dispatcher rather than from the region.
A close runs only after the reader has answered, so it is always on the far side of the window `cardWasAsked` marks on a charge. Reported with the default classification, a 402 arriving there recorded `outcome=declined` for a sale the processor had already taken. The rule needs no parameter here, unlike the charge, which spans both sides of the tap.
…int alone A session that has reached `ReinitializeRequired` admits any configuration next, and a capability can be pointed at an entry point the session was not configured with. Keyed on the entry point alone, a terminal built against one environment found a payment opened against another and offered to close it, sending an identifier and a processor answer to a service that never opened it. The retained payment and the lock protecting it are now keyed on the environment together with the entry point. The idempotency key store is unchanged and still keyed per entry point.
The runner's `environment` parameter carries no default, so the live charge test stopped compiling when it arrived. Neither `test` nor `lint` compiles `src/androidTest`, so all three of the gates run before a push stayed green while every instrumented variant was uncompilable.
The retained payment is keyed on the environment together with the entry point and shared across every terminal built for that pair, which is what lets a screen that has been rebuilt finish a close the screen before it started. The comment said only the terminal that took the payment could close it, which is the instance scope the record deliberately does not have.
The comment justified it with a configuration this file rules out forty lines above, where the entry point is taken from the session precisely so a terminal cannot be pointed somewhere the session was never configured for. The invariant is the transport: the transaction client is built over `session.transport`, so a retained payment has to be scoped to the service that transport reaches.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…d environment Scoping the retained payment by environment took the region lock with it, and the region does not protect the retained payment alone. What it protects is the idempotency record, and the store holds one of those per entry point. So the lock became narrower than the record it guards: two terminals for one paypoint on different environments stopped serializing, and one could settle a key the other was charging under. An ambiguous tap after that mints a fresh key and the payer is charged twice, which is the outcome the key exists to prevent. The lock is keyed on the entry point again. The retained payment stays keyed on the environment as well, which is correct and is narrower than the lock rather than wider, so nothing it guards is left unprotected. The test drives two terminals for one paypoint on different environments and was seen fail with the lock keyed on both, where the two openings interleave before either close.
…refusal Three sites said a reused idempotency key is one the service has already seen and is refused as a duplicate, stated without bound. What was measured on hardware is narrower: a repeat inside the service's duplicate window is refused and answers nothing at all, and what happens outside that window was never measured. The reason the key is settled where it is does not rest on either answer. An attempt that has reached an outcome is over, so holding its key past that leaves the next charge sending one that names a payment already resolved, whatever the service then does with it. The comments say that instead. Comment only. The base branch removed the same premise from four of its own sites; these three are this branch's.
…rom it answering The capture state was set to charged as soon as the reader returned, on the reasoning that the processor takes the sale before the answer is delivered. That held while any answer meant a sale. The base branch establishes that the reader answers for a refusal too, and for an outcome that is neither, so the reasoning no longer covers the cases it is applied to. Measured on the base's own two tests once this branch replayed over it: a refused card reported charged, and an indeterminate outcome reported charged. Both are the reading the capture state exists to prevent - a host reads it before offering a retry, and charged sends it to finish a payment that may never have been taken. The state now follows the outcome. An approval charged, a refusal not charged, anything else unknown, which is what it already was before the card was asked for. NOT_CHARGED's documentation said the card was never asked for, which was true of the only way it could be reached before. It now says no money moved and names both ways that happens, since a refusal is an answer and that is what separates it from unknown. The two base tests asserted the vendor exception type directly; this branch wraps every failure after the opening so the payment can be named, so they now assert the wrapped failure, its cause and its capture state. Both were seen fail on the capture assertion before the fix.
31c3385 to
543639e
Compare
Rebased onto seven more base commits, and the replay found a defect on this branchThe base moved from The base now tells a refusal apart from an approval. Its What that exposed. This branch set the capture state to charged as soon as the reader returned, reasoning that the processor takes the sale before the answer is delivered. That held while any answer meant a sale. Once the reader answers for a refusal too, it means a refused card was reported to a host as a payment whose money had moved, and an indeterminate outcome as the same. The capture state is what a host reads before offering a retry, and charged is what sends it to finish a payment that may never have been taken. Fixed in The base's two tests for that behaviour are the ones that caught it, and they needed changing for a second reason: they asserted the vendor exception type directly, and this branch wraps every failure after the opening so the payment can be named. They now assert the wrapped failure, its cause and its capture state. Both were seen fail on the capture assertion before the fix, with Current hashes for every commit named in a reply above:
|
There was a problem hiding this comment.
🟡 Changes recommended
Recovery can discard an indeterminate attempt key and misreport non-approved outcomes as charged.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
taptopay/src/main/java/com/payabli/sdk/taptopay/TapToPayChargeRunner.kt:354
- A held record can also contain a declined or indeterminate reader result, but every failed retry is relabeled
CHARGEDhere. Calling recovery with the ID from aNOT_CHARGEDorUNKNOWNfailure therefore changes the reported financial outcome. Derive the capture state frompending.read.outcome, or validate that only approved records enter this method.
throw failed(failure, pending.paymentTransId, TapToPayCapture.CHARGED)
- Files reviewed: 16/16 changed files
- Comments generated: 2
- Review effort level: Balanced
The charge takes its capture state and its settle decision from the reader's outcome. The recovery took neither: it settled the attempt whatever the outcome was and reported every failure as charged. A held payment is kept for every outcome, deliberately, because the transaction is open at the service whatever the card did. So a recovery can be closing a payment whose outcome was never definite, and settling that drops the only handle on an attempt that may have taken money, after which the next charge mints a fresh key. It also told a host a refused card had been charged, which is the reading the capture state exists to prevent. Both now come from the held answer, through one mapping the charge and the recovery share so the two cannot disagree about a payment they both saw. The facade's contract said the recovery was for the charged case. It is reachable from any capture state, because a transaction left open is worth closing whatever the card did, so it says that instead. Two tests, each seen fail with its own guard reverted and no other test moving: an indeterminate recovery keeps its attempt, and a failed recovery of a refused card reports not-charged.
The property described the transaction a refusal belongs to. It is set from the envelope's payload for every non-approved answer, so a service error carries one as a decline does, and the code's own documentation already says an E prefix is an error rather than a refusal. Read as written, it implied a caller handling a service error had no transaction to reconcile.
There was a problem hiding this comment.
🟡 Changes recommended
The recovery contract overstates which UNKNOWN failures can be closed, and related telemetry documentation assumes every close represents charged money.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
core/src/main/java/com/payabli/sdk/core/telemetry/TelemetryEvents.kt:75
- These public event descriptions assume every recovery concerns charged money, but
closeCapturedChargeexplicitly accepts heldDECLINEDandINDETERMINATEresults and emits these same events. Telemetry consumers would therefore interpret some successful closes as settled charges when no charge is known to exist; describe confirmation of the close independently of capture state.
- Files reviewed: 16/16 changed files
- Comments generated: 1
- Review effort level: Balanced
| * **Reachable from any [TapToPayCapture], not only [TapToPayCapture.CHARGED].** The transaction is open | ||
| * at the service whatever the card did, so a payment whose card was refused or whose outcome was never | ||
| * definite is worth closing too. The failure this raises carries the capture state the reader's answer | ||
| * supports rather than assuming the money moved. |
|
Stacked on
alexarguello/pla-2186-fiserv-card-reader-adapter, which carries the charge flow this changes. Rebase ontomainwhen #73 merges.A charge reported its transaction identifier on success and on no other outcome, so any failure after the payment was opened left the caller holding a transaction they could not name, reconcile, void or log. The closing call is the sharper half: once the reader returns, the processor has taken the card, and a caller told only that the charge failed retries it and takes the money twice. Failures now carry the identifier and say whether the card was charged, and a payment whose close did not land can be finished without a second tap.
Structure
The capture state is three values rather than a flag.
TapToPayCaptureisNOT_CHARGED,UNKNOWNandCHARGED. The middle one is reachable and is the one that matters: the processor takes the sale before the reader hands the answer back, so a tap that ends without one may still have moved money. A Boolean reports that as a definite not-charged and invites the retry that charges twice.Which value is reported comes from what the reader answered, not from the fact that it answered. An approval is charged, a refusal is not charged, and anything else stays unknown. Reading it from the answer arriving was correct while any answer meant a sale, and stopped being correct once the base branch established that the reader answers for a refusal too.
The close, the idempotency settle and the drop of the held payment are one uncancellable step. Splitting them leaves a charged payment open, or an attempt still reserved so the next charge reuses a key the service has already seen, or a payment offered for closing after it closed. That holds on the charge's own close and on the later recovery.
The held payment is keyed on the environment together with the entry point; the lock and the idempotency key store are keyed on the entry point alone. A terminal is built per call, so a screen that has been rebuilt finishes a close the one before it started, which is why the payment is shared across every terminal built for that pair rather than held per instance. Keyed on the entry point alone it went too far the other way: a terminal built against one environment found a payment opened against another and offered to close it.
The lock is broader than everything it guards, and that is the constraint rather than a preference. The key store holds one record per entry point, so a lock narrowed to the environment as well is narrower than that record and leaves two terminals for one paypoint on different environments unserialized over it. The key store's own identity is unchanged by this branch.
The reader's answer is held in memory, not persisted. Nothing survives the close landing, a later payment opening, or the process ending.
Notable changes
TapToPayExceptioncarriespaymentTransIdandcapture. The identifier is non-null exactly when a payment was opened.PayabliTTP.closeCapturedCharge(paymentTransId)finishes a payment whose card was charged and whose close did not land. No second tap, no second open.MoneyInClient.readwas building its failure from the envelope's scalar half and dropping the payload that holds the identifier, so a card-not-present refusal lost it.PayInFailurecarries it now, reaching callers through both exception types and through the form's published failure state.CardReadResult's documentation claimed the response carries the card's expiry and a processor token. The shipped adapter forwards a named subset holding neither. It now states what the contract permits separately from what the adapter does.Verification
Unit tests: 21 added — 14 on the charge runner, 3 on the card-not-present client, 3 on the telemetry reporters, 1 on the facade.
:taptopay,:coreand:payinsuites green, plusktlintCheck,lintandassembleAndroidTest. The last is what compiles the instrumented source sets, which neithertestnorlintreaches: an instrumented call site of the charge runner was uncompilable while all three of the others were green.Each new test was seen red before it was trusted, by breaking the guarantee it names and watching that test alone fail. Among them: dropping the identifier, collapsing the unknown capture state, skipping the attempt settle after a recovery, clearing the held payment in a
finally, emitting the charge event in place of the close event, keying the charge lock on the environment as well as the paypoint, which lets two terminals interleave their openings, and reporting the capture state from the reader having answered, which called a refusal and an unknown outcome both charged.Card-not-present live on a Pixel 7a against qa and sandbox: five of five on each, covering save and capture for card and bank account plus capture-and-void. An SM-A136U1 could not answer on either environment, its Compose harness losing the app mid-run with no crash or kill in logcat, on the base build as well; that is a stated skip rather than a pass.
Card-present has a live tier and this change has not been through it.
ChargeWithoutTapLiveTestsits in the manual tier and drives both service calls against a real paypoint on a wired handset, with only the tap stubbed; this branch fixes its call site so it compiles again, and it has not been run for this change.An earlier revision of this description said the reader had never armed on a handset. That is no longer true: readers have armed on two handsets and real transactions have been opened and closed against qa. What no tier reaches is a real tap driven through the recovery, which needs a person and a card, so the close-and-recover path is reasoned and unit-covered rather than observed end to end.
Not included
Preventing a second transaction outright. The client makes the double charge avoidable rather than impossible, and enforcement is the backend dedupe across the two calls, which is unstarted.
The sample app does not yet show either field. It needs a structured failure on its terminal seam, and the cheap version puts a transaction identifier into an exception message.
:taptopay's Lint is red onAndroidManifest.xml:26,MissingClassfor the card reader's activity. It arrives with the base branch and reproduces there with none of this change present.