[PLA-2184] Android - P5.1 Card-present session state machine and /config/{entry}

[Alex Arguello (alex-arguello)]

Tickets: PLA-2184, the card-present session state machine and the /config/{entry} call a session needs to reach a usable state.

:taptopay could attest a device and spend an activation code, and nothing sequenced those into a session: no state a host could observe, no entry point safe to call twice, and no way for a device attested on an earlier run to discover it still owes an activation code. This adds nine session states with a table of legal moves, one writer, and a coordinator whose three entry points never overlap. It also adds /config/{entry}, which returns the card reader's credentials and is the only route that answers the activation question on a warm start.

Structure

The session holds its own state rather than publishing into :core. SdkState says what the SDK can do at all, and it is written only by :core's own machine, which a capability module cannot reach. :payin holds its submission state the same way.

Exclusion and joining are two mechanisms. A mutex holds the region for a whole run; a claim per entry-point kind lets two callers of the same kind share one run. The claim is per kind rather than one slot, so a build still joins a build when a repair is queued between them. One queue doing both jobs would make them impossible to break independently, and each is worth its own failing test.

ReaderProvider has no implementation here. Without the seam, four of the nine states cannot be entered and cannot be tested. The card reader itself arrives with the charge work.

Notable changes

• advance(to) { work } moves the state and then runs the work under it, so a phase cannot run under a state that was never published. There is no boolean for a caller to drop.
• A failed session carries a reason, over a closed set of four remedies, so a host is not left assuming the most expensive repair.
• post in the device client splits into request assembly plus a shared response reader. /config is the first GET in the family, and the first route whose path is not its own template.
• A real HTTP 403 and an envelope 403 both reach pending activation. They arrive by different mechanisms, since the gateway's is classified before the envelope is read, so there are two catches and two tests.
• An envelope 401 on /config discards the stored binding and fails the attempt. Attesting again from inside the failing call would spend a challenge and hide the credential rotation that caused it.
• fromstate and tostate join the loggable field names, so a refused transition records both ends of the move.

Verification

253 unit tests in :taptopay, green, along with :core's suite, ktlintCheck and lintDebug. Nothing added here calls an Android API, so there is no instrumented tier for it.

Thirteen guarantees were each broken in turn and all but one was caught by a test; the exception is a diagnostic race no deterministic interleaving reaches, and the second read that caused it is gone rather than guarded. The harness deletes the results directory before each row, so a sabotage that fails to compile yields no verdict instead of a stale one; it refuses a patch whose anchor does not match exactly once; and it diffs the tree against a backup after every row. The row-by-row table is on the ticket.

Not included

The public facade and publishing the state type, the card reader and its two-tier failure classifier, and the first EventMulticaster emitter. Each belongs to the charge work that follows this one.

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds a Tap to Pay session state machine and coordinator flow, including a /config device-service call for reader credentials, with comprehensive tests around transitions, warm start behavior, and concurrency serialization.

Changes:

• Introduces TapToPaySessionState, transition rules (TapToPaySessionTransitions), and a single-writer TapToPaySessionManager.
• Adds TapToPaySessionCoordinator to serialize/merge concurrent session entry points (initialize / repair / activate) and standardize failure landings.
• Extends DeviceServiceClient with /config/{entry} GET + wire models (ConfigResponse, ReaderCredentials) and updated fixtures/tests.

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
taptopay/src/main/java/com/payabli/sdk/taptopay/session/TapToPaySessionCoordinator.kt	Orchestrates session lifecycle with mutual exclusion and “join same kind” semantics
taptopay/src/main/java/com/payabli/sdk/taptopay/session/TapToPaySessionManager.kt	Single-writer state publisher enforcing transition table
taptopay/src/main/java/com/payabli/sdk/taptopay/session/TapToPaySessionTransitions.kt	Centralized legality table for state transitions
taptopay/src/main/java/com/payabli/sdk/taptopay/session/TapToPaySessionState.kt	Defines the session states and diagnostic names
taptopay/src/main/java/com/payabli/sdk/taptopay/session/TapToPaySessionFailures.kt	Maps failures to end states/recovery hints
taptopay/src/main/java/com/payabli/sdk/taptopay/session/TapToPaySessionException.kt	Defines session-level exception vocabulary
taptopay/src/main/java/com/payabli/sdk/taptopay/session/TapToPayFailureReason.kt	Defines host-facing failure remedies
taptopay/src/main/java/com/payabli/sdk/taptopay/session/ReaderProvider.kt	Introduces abstraction for reader configuration/preparation
taptopay/src/main/java/com/payabli/sdk/taptopay/attestation/device/DeviceServiceClient.kt	Adds /config/{entry} GET route and path-segment validation
taptopay/src/main/java/com/payabli/sdk/taptopay/attestation/device/DeviceWireFormat.kt	Adds config response + strongly typed, redacted reader credentials
taptopay/src/main/java/com/payabli/sdk/taptopay/enrollment/DeviceEnrollment.kt	Adds assertion() helper for warm-start credential fetch
taptopay/src/main/java/com/payabli/sdk/taptopay/attestation/device/DeviceServiceException.kt	Updates documentation for forbidden cases and decode failures
core/src/main/java/com/payabli/sdk/core/logging/impl/LoggableFieldNames.kt	Whitelists fromstate/tostate log fields
taptopay/src/test/java/com/payabli/sdk/taptopay/session/TapToPayTransitionMatrixTest.kt	Exhaustive transition-matrix test for the legality table
taptopay/src/test/java/com/payabli/sdk/taptopay/session/TapToPaySessionManagerTest.kt	Tests manager publishing/refusal semantics
taptopay/src/test/java/com/payabli/sdk/taptopay/session/SessionWarmStartTest.kt	Verifies warm-start sequences don’t re-attest and behave correctly
taptopay/src/test/java/com/payabli/sdk/taptopay/session/SessionSerializationTest.kt	Tests exclusion/join behavior for concurrent entry points
taptopay/src/test/java/com/payabli/sdk/taptopay/session/SessionFixture.kt	Shared session fixture wiring enrollment/client/reader fakes
taptopay/src/test/java/com/payabli/sdk/taptopay/enrollment/EnrollmentFixture.kt	Adds config response body + config route constant + shared client
taptopay/src/test/java/com/payabli/sdk/taptopay/attestation/device/DeviceServiceConfigTest.kt	Tests /config request shape, decoding, redaction, and refusal mapping

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.

Suppressed comments (2)

taptopay/src/main/java/com/payabli/sdk/taptopay/session/TapToPaySessionManager.kt:64

• The failure message uses state.value after write(to) returns false. Under concurrent callers (this class explicitly doesn’t serialize phases), another successful transition could update state between the refusal decision and message construction, producing an incorrect from state in the exception message. Consider returning the decided from state from write (e.g., a small result object { from, permitted, published }) or providing a writeOrThrow(to) helper that captures from under the same lock and uses it in the thrown message.

        check(write(to)) {
            // A defect in this SDK's own sequence rather than anything a host did, so it is not part of the
            // failure vocabulary a caller handles. Both names are from the fixed state vocabulary.
            "a session cannot move to ${to.diagnosticName} from ${state.value.diagnosticName}"
        }

taptopay/src/main/java/com/payabli/sdk/taptopay/session/TapToPaySessionCoordinator.kt:202

• TapToPaySessionState.AttestingDevice is documented as “Skipped by a warm start” (in TapToPaySessionState.kt), but runInitialize() always advances into AttestingDevice before calling enrollment.enroll(), including warm-start cases where no attestation runs. Either update the state documentation to match the observed behavior, or adjust the coordinator/enrollment API so the session only enters AttestingDevice when attestation is actually performed (e.g., split enrollment into a warm-start check phase vs attestation phase, or introduce a separate state that accurately represents ‘enrollment’ even when attestation is skipped).

    private suspend fun runInitialize() {
        manager.reset()
        val outcome = manager.advance(TapToPaySessionState.AttestingDevice) { enrollment.enroll() }
        if (outcome is EnrollmentOutcome.Attested && outcome.activationRequired) {
            // Registration already said so, so there is nothing to learn from asking for the credentials.
            throw TapToPaySessionException.PendingActivation()
        }
        bringReaderUp()
    }

[Copilot]

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.

Suppressed comments (2)

taptopay/src/test/java/com/payabli/sdk/taptopay/session/TapToPaySessionManagerTest.kt:134

• The tests cancel the collector but don’t wait for it to actually stop, which can make seen nondeterministic/flaky (especially with Dispatchers.Unconfined). Prefer collector.cancelAndJoin() (or cancel(); join()) before asserting, and consider using UnconfinedTestDispatcher(testScheduler) instead of Dispatchers.Unconfined to keep execution tied to the test scheduler while retaining 'immediate' behavior.

    fun `a refused move is never briefly published`() =
        runTest(timeout = TEST_TIMEOUT) {
            val seen = mutableListOf<TapToPaySessionState>()
            // An immediate dispatcher, so a collector would resume inside the write if one were made.
            val collector = launch(Dispatchers.Unconfined) { manager.state.collect { seen += it } }

            manager.advance(TapToPaySessionState.FetchingConfig)
            runCatching { manager.advance(TapToPaySessionState.Ready) }
            manager.invalidate()

            collector.cancel()
            assertEquals(
                listOf(TapToPaySessionState.Idle, TapToPaySessionState.FetchingConfig),
                seen,
            )
        }

    @Test
    fun `re-entering a state publishes nothing`() =
        runTest(timeout = TEST_TIMEOUT) {
            val seen = mutableListOf<TapToPaySessionState>()
            val collector = launch(Dispatchers.Unconfined) { manager.state.collect { seen += it } }

            manager.advance(TapToPaySessionState.FetchingConfig)
            manager.advance(TapToPaySessionState.FetchingConfig)

            collector.cancel()
            assertEquals(listOf(TapToPaySessionState.Idle, TapToPaySessionState.FetchingConfig), seen)
        }

taptopay/src/test/java/com/payabli/sdk/taptopay/session/SessionSerializationTest.kt:135

• After owner.cancel(), the test immediately proceeds without ensuring the owner coroutine has fully completed its cancellation path (including releasing claims/locks and settling state). Using owner.cancelAndJoin() (or owner.cancel(); owner.join()) before asserting/joining the joiner will reduce timing sensitivity and potential flakiness in this serialization test.

            val owner = launch(UnconfinedTestDispatcher(testScheduler)) { fixture.coordinator.initialize() }
            var joinerFailure: Throwable? = null
            val joiner =
                launch(UnconfinedTestDispatcher(testScheduler)) {
                    try {
                        fixture.coordinator.initialize()
                    } catch (failure: TapToPaySessionException) {
                        joinerFailure = failure
                    }
                }

            owner.cancel()
            completing("the joining build") { joiner.join() }

[Alex Arguello (alex-arguello)]

Suppressed comment · session/TapToPaySessionManager.kt:64 · stale from in the refusal message

The failure message uses state.value after write(to) returns false. Under concurrent callers (this class explicitly doesn't serialize phases), another successful transition could update state between the refusal decision and message construction, producing an incorrect from state in the exception message.

Fixed · ba9457f

Change write returns the state it decided against, and one writeOrThrow builds the message from it. No caller reads the state a second time.
Test None. The window is between a returned value and a lambda evaluation, and no deterministic interleaving reaches it. Reintroducing the second read as a sabotage failed nothing, which is the same finding from the other side. The read is gone rather than guarded.

Suppressed comment · session/TapToPaySessionState.kt · AttestingDevice and warm starts

TapToPaySessionState.AttestingDevice is documented as "Skipped by a warm start", but runInitialize() always advances into AttestingDevice before calling enrollment.enroll(), including warm-start cases where no attestation runs.

Fixed · 41d6f17

The documentation was wrong, not the code. Enrollment reads the stored record and decides for itself whether the cold sequence is needed, so the state covers asking the question as well as answering it. Splitting it would add a state whose only content is that a local read happened.

Change The state says a repair never enters it, and a warm start enters and leaves without a round trip. Swept the other three warm-start sentences in the package; each describes where the device learns it owes an activation code, which is unchanged.
Test None. SessionWarmStartTest already asserts the route trace is exactly /config on a warm start, which is the behaviour the sentence now describes.

Suppressed comment · session/TapToPaySessionManagerTest.kt:134 · collector cancelled without joining

The tests cancel the collector but don't wait for it to actually stop, which can make seen nondeterministic/flaky (especially with Dispatchers.Unconfined). Prefer collector.cancelAndJoin() ... and consider using UnconfinedTestDispatcher(testScheduler).

Fixed · caa0d69

Change cancelAndJoin, and UnconfinedTestDispatcher(testScheduler) in place of Dispatchers.Unconfined. The immediate resumption the assertion depends on is what makes a briefly-published value observable, and the test dispatcher keeps it while tying execution to the scheduler.
Test The existing assertions, unchanged. Both still fail when an illegal transition is allowed to publish.

Suppressed comment · session/SessionSerializationTest.kt:135 · owner cancelled without joining

After owner.cancel(), the test immediately proceeds without ensuring the owner coroutine has fully completed its cancellation path (including releasing claims/locks and settling state).

Fixed · caa0d69

The cancellation path is exactly what this test asserts on: it releases the claim and settles the state under NonCancellable, and both were read before that had necessarily run.

Change owner.cancelAndJoin().
Test The existing assertions. Removing the claim release still fails them.

---

SonarCloud

The gate failed on one condition, new coverage at 79.7% against a threshold of 80. Every other condition passed.

TapToPaySessionFailures carried 69 of the 91 uncovered new lines and branches and had no direct test. c0d3ab5 adds a table with one row per branch, plus two invariants read back from the classifier rather than from the table. That file now reports no missed lines and no missed branches.

[Copilot]

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.

Suppressed comments (3)

taptopay/src/test/java/com/payabli/sdk/taptopay/session/TapToPayTransitionMatrixTest.kt:37

• In Kotlin, ALL_CAPS names are conventionally reserved for const val compile-time constants. Since this is a runtime val (and a Failed(...) instance), rename it to something like failedInternal / internalFailureState to reduce confusion and keep naming conventions consistent.

private val FAILED = Failed(TapToPayFailureReason.INTERNAL)

taptopay/src/main/java/com/payabli/sdk/taptopay/session/TapToPaySessionManager.kt:90

• reset() is described as 'the first act of building a session' and depends on Idle being universally reachable. Calling write() here silently logs-and-drops if the transition table ever regresses, which can make failures harder to diagnose (the build would proceed from the wrong state). Consider using writeOrThrow(TapToPaySessionState.Idle) here so a broken invariant fails fast and points directly at the defect.

    fun reset() {
        write(TapToPaySessionState.Idle)
    }

taptopay/src/main/java/com/payabli/sdk/taptopay/attestation/device/DeviceServiceClient.kt:257

• The require(...) message is a bit underspecified for debugging (it doesn’t indicate what character set is allowed). Since the entry value itself must not be echoed, consider expanding the message to include the allowed shape (e.g., 'unreserved RFC3986 characters: [A-Za-z0-9._~-]') or otherwise clarify what inputs are acceptable without printing the merchant identifier.

    private fun pathSegment(entry: String): String {
        require(PATH_SEGMENT.matches(entry)) { "entry must be usable as a single path segment" }
        return entry
    }

[Alex Arguello (alex-arguello)]

Suppressed comment · session/TapToPaySessionManager.kt:90 · reset() drops its write result

reset() is described as 'the first act of building a session' and depends on Idle being universally reachable. Calling write() here silently logs-and-drops if the transition table ever regresses, which can make failures harder to diagnose (the build would proceed from the wrong state). Consider using writeOrThrow(TapToPaySessionState.Idle) here so a broken invariant fails fast and points directly at the defect.

Fixed · 7c685e5

The file states its own rule as throw where a refusal is deterministic, log where it can lose a race. reset runs inside the serialized region and starting over is reachable from every state, so it belongs on the first side and was on the second. A build continuing from a state nobody expects while reporting success is the defect this type exists to prevent, and it was open at the one call site that opens every build.

Change reset throws, like both advance overloads. settle and invalidate still log and return: one is reached from a failure path where a throw replaces the caller's failure, and the other can lose a race legitimately.
Test None added. TapToPayTransitionMatrixTest.starting over is reachable from every state already guards the invariant; this makes the call site say so rather than continue past it.

Suppressed comment · attestation/device/DeviceServiceClient.kt:257 · path-segment refusal names no shape

The require(...) message is a bit underspecified for debugging (it doesn't indicate what character set is allowed) ... consider expanding the message to include the allowed shape ... without printing the merchant identifier.

Fixed · c3a09bf

Change The message names the character set. The value stays out of it, since an entry point identifies a merchant. ActivateRequest states its own shape the same way.
Test DeviceServiceConfigTest.an entry that is not one path segment is refused before anything is sent, unchanged: it asserts the refusal and that nothing was sent, not the wording.

Suppressed comment · session/TapToPayTransitionMatrixTest.kt:37 · FAILED naming

In Kotlin, ALL_CAPS names are conventionally reserved for const val compile-time constants. Since this is a runtime val (and a Failed(...) instance), rename it to something like failedInternal / internalFailureState to reduce confusion and keep naming conventions consistent.

Declined

The Kotlin coding conventions scope uppercase underscore names wider than const:

Names of constants (properties marked with const, or top-level or object val properties with no custom get function holding deeply immutable data) should use uppercase underscore-separated names.

FAILED is a top-level val with no custom getter holding a data class of an enum, so it is inside that definition rather than outside it.

The repository is consistent with the same reading. Non-const uppercase vals in this module include SIX_DIGITS and ACTIVATION_CODE (both Regex), PATH_SEGMENT, DEFAULT_WINDOW, DEFAULT_PLATFORM_DEADLINE, and TEST_TIMEOUT in a dozen test files. Renaming this one would make it the exception.

Commit none. Nothing in this PR changes.
Test None. No behaviour is involved.

[Copilot]

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.

Suppressed comments (2)

taptopay/src/test/java/com/payabli/sdk/taptopay/session/TapToPayTransitionMatrixTest.kt:37

• FAILED is easy to confuse with the Failed state type and also hides that it’s specifically the INTERNAL failure instance used as a representative in the table. Rename to something explicit like FAILED_INTERNAL (or INTERNAL_FAILURE_STATE) to make the intent and equality semantics (data-class reason) clearer.

private val FAILED = Failed(TapToPayFailureReason.INTERNAL)

taptopay/src/main/java/com/payabli/sdk/taptopay/session/TapToPaySessionTransitions.kt:55

• Each call allocates a new Set via setOf(...). If permits() is called frequently (e.g., on every state write and in logging paths), this creates avoidable allocations. Consider returning precomputed constant sets (e.g., private val FROM_IDLE = setOf(...)) while keeping the exhaustive when for compile-time coverage.

    private fun reachableFrom(from: TapToPaySessionState): Set<TapToPaySessionState> =
        when (from) {
            Idle -> setOf(AttestingDevice, FetchingConfig)
            AttestingDevice -> setOf(FetchingConfig, PendingActivation)
            FetchingConfig -> setOf(InitializingReader, PendingActivation)
            InitializingReader -> setOf(Ready)
            Ready -> setOf(SessionExpired)
            // Only into a re-initialization. Reaching config directly from here would skip the state that
            // says a repair is under way, and that state is what a host shows.
            SessionExpired -> setOf(Reinitializing)
            Reinitializing -> setOf(FetchingConfig)
            // The device owes a code. Confirming it puts the session back through attestation, since the
            // service issues the credentials only to an active device.
            PendingActivation -> setOf(AttestingDevice)
            is Failed -> setOf(AttestingDevice, FetchingConfig)
        }

[Alex Arguello (alex-arguello)]

Suppressed comment · session/TapToPayTransitionMatrixTest.kt:37 · FAILED naming

FAILED is easy to confuse with the Failed state type and also hides that it's specifically the INTERNAL failure instance used as a representative in the table. Rename to something explicit like FAILED_INTERNAL (or INTERNAL_FAILURE_STATE) to make the intent and equality semantics (data-class reason) clearer.

Fixed · 6e05ec0

A different finding from the casing one declined on the previous round, and this one holds. The two names differ only in case and met on one line of the table, is Failed -> setOf(Idle, AttestingDevice, FetchingConfig, FAILED), where one is the state type and the other is the single instance every row uses. The naming rule here forbids two names a reader can confuse.

The second half is also right and is what the table depends on: the state is a data class, so equality includes the reason, and the name now says which instance it is.

Change FAILED_INTERNAL, eleven references. The casing is unchanged, for the reason given last round.
Test The matrix tests, unchanged. All 81 ordered pairs still assert.

Suppressed comment · session/TapToPaySessionTransitions.kt:55 · a set allocated per call

Each call allocates a new Set via setOf(...). If permits() is called frequently (e.g., on every state write and in logging paths), this creates avoidable allocations. Consider returning precomputed constant sets (e.g., private val FROM_IDLE = setOf(...)) while keeping the exhaustive when for compile-time coverage.

Declined

The premise is the call frequency, and it does not hold. permits has one production caller, TapToPaySessionManager.write, and nothing in the logging path calls it: the two log records read diagnosticName on states already in hand.

reachableFrom is reached less often than permits is called, because three rules short-circuit ahead of it:

when {
    from == to -> true
    to is Idle -> true
    to is Failed -> true
    else -> to in reachableFrom(from)
}

So a build allocates four of these sets over a whole session: AttestingDevice, FetchingConfig, InitializingReader, Ready. The reset that opens it, and every landing on a failure, return before reaching the table.

Nine precomputed vals would trade a table that reads top to bottom for one spread across nine declarations plus the when that indexes them, to save four small allocations per session.

Commit none. Nothing in this PR changes.
Test None. No behaviour is involved.

[Copilot]

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

taptopay/src/main/java/com/payabli/sdk/taptopay/session/TapToPaySessionManager.kt:148

• TapToPaySessionManager logs a new field name errorkind, but LoggableFieldNames only adds fromstate/tostate in this PR. If the logging layer filters/validates field names against LoggableFieldNames, this field may be dropped or could cause validation failures. Consider adding errorkind to LoggableFieldNames (or reusing an existing allowed field name) so failure-reason logging behaves consistently.

            logger.info(
                LogField.safe("event", "ttp_session_state"),
                LogField.safe("state", to.diagnosticName),
                LogField.safe("errorkind", (to as? TapToPaySessionState.Failed)?.reason?.name),
            ) { "session state changed" }

taptopay/src/main/java/com/payabli/sdk/taptopay/session/TapToPaySessionCoordinator.kt:150

• On a Throwable that is not an Exception (e.g., OutOfMemoryError), the claim is released but the session state is not settled back to Idle (unlike the CancellationException branch). That can leave the system in a mid-phase state indefinitely and change the behavior of subsequent calls (e.g., repairs/refusals based on a stale state). Consider mirroring the CancellationException behavior by settling to Idle under NonCancellable (or otherwise ensuring state is left in a safe baseline) before releasing the claim.

        } catch (fatal: Throwable) {
            // An OutOfMemoryError reaches the caller unchanged. The claim is still released, or every later
            // caller of this kind waits for something that will never complete.
            release(claim, TapToPaySessionException.SetupAbandoned())
            throw fatal
        }

[Copilot]

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.

Suppressed comments (2)

taptopay/src/main/java/com/payabli/sdk/taptopay/session/TapToPaySessionCoordinator.kt:151

• In the fatal: Throwable path, joiners will observe SetupAbandoned even though the actual failure was a fatal error (e.g., OutOfMemoryError). That makes the outcome misleading and can encourage callers to retry when the underlying failure is non-recoverable. Consider completing the claim with the same fatal throwable (or a dedicated wrapper that preserves the fatal as cause) so all waiters receive an accurate outcome while still ensuring the claim is always released.

        } catch (fatal: Throwable) {
            // An OutOfMemoryError reaches the caller unchanged. The claim is still released, or every later
            // caller of this kind waits for something that will never complete.
            release(claim, TapToPaySessionException.SetupAbandoned())
            throw fatal
        }

taptopay/src/test/java/com/payabli/sdk/taptopay/session/TapToPayTransitionMatrixTest.kt:37

• Using a single FAILED_INTERNAL instance in the matrix can read like identity is relevant, even though Failed is a data class and equality is by reason. Consider inlining Failed(TapToPayFailureReason.SDK_INTERNAL_ERROR) (or introducing a tiny helper like failed(reason)) in the sets to make it obvious the test is about value equality rather than a shared singleton instance.

private fun legalTargetsFrom(from: TapToPaySessionState): Set<TapToPaySessionState> =
    when (from) {
        Idle -> setOf(Idle, AttestingDevice, FetchingConfig, FAILED_INTERNAL)
        AttestingDevice -> setOf(Idle, AttestingDevice, FetchingConfig, PendingActivation, FAILED_INTERNAL)
        FetchingConfig -> setOf(Idle, FetchingConfig, InitializingReader, PendingActivation, FAILED_INTERNAL)
        InitializingReader -> setOf(Idle, InitializingReader, Ready, FAILED_INTERNAL)
        Ready -> setOf(Idle, Ready, SessionExpired, FAILED_INTERNAL)
        SessionExpired -> setOf(Idle, SessionExpired, Reinitializing, FAILED_INTERNAL)
        Reinitializing -> setOf(Idle, Reinitializing, FetchingConfig, FAILED_INTERNAL)
        PendingActivation -> setOf(Idle, PendingActivation, AttestingDevice, FAILED_INTERNAL)
        is Failed -> setOf(Idle, AttestingDevice, FetchingConfig, FAILED_INTERNAL)
    }

private val FAILED_INTERNAL = Failed(TapToPayFailureReason.SDK_INTERNAL_ERROR)

[Alex Arguello (alex-arguello)]

Suppressed comment · session/TapToPaySessionCoordinator.kt:151 · joiners told a fatal was a withdrawal

In the fatal: Throwable path, joiners will observe SetupAbandoned even though the actual failure was a fatal error (e.g., OutOfMemoryError). That makes the outcome misleading and can encourage callers to retry when the underlying failure is non-recoverable. Consider completing the claim with the same fatal throwable (or a dedicated wrapper that preserves the fatal as cause) so all waiters receive an accurate outcome while still ensuring the claim is always released.

Fixed, recommendation corrected · 33139ef

The defect is real. SetupAbandoned says nothing happened and asking again is safe, and after an OutOfMemoryError neither is true, so a host following that answer retries into a process that is already going down.

The remedy is not, on both halves. Completing the claim with the fatal, or wrapping it as a cause, hands every waiter a reference to whatever died. The token refresh in the core module reached this exact question and answered it at PayabliAuth.kt:257-261:

} catch (fatal: Throwable) {
    // It still reaches the caller unchanged, but the claim cannot outlive it or every later
    // reader waits on a deferred nobody owns. No cause attached: it would pin whatever died.
    finish(shared, PayabliGenericException(PayabliErrorCode.TOKEN_EXPIRED, REASON_REFRESH_FAILED))
    throw fatal
}

That is the shape taken here: a failure rather than a withdrawal, and no cause. The owner keeps the original and it reaches that caller unchanged.

Change TapToPaySessionException.SetupFailed, landing on the SDK-internal reason. SetupAbandoned keeps its meaning for a caller that withdrew, where asking again genuinely is safe.
Test SessionSerializationTest.a joiner is told the owner failed when the owner died on something unclassified, which also asserts the joiner is handed no cause. Handing the withdrawal outcome back fails it and nothing else.

Suppressed comment · session/TapToPayTransitionMatrixTest.kt:37 · one FAILED_INTERNAL instance in the matrix

Using a single FAILED_INTERNAL instance in the matrix can read like identity is relevant, even though Failed is a data class and equality is by reason. Consider inlining Failed(TapToPayFailureReason.SDK_INTERNAL_ERROR) (or introducing a tiny helper like failed(reason)) in the sets to make it obvious the test is about value equality rather than a shared singleton instance.

Declined

The file already demonstrates what the change would signal. a failure may change its reason asserts that a transition between two separately constructed Failed instances carrying different reasons is permitted, so value semantics are shown by a passing test rather than implied by a spelling.

The named value also carries something inlining removes: eight of the nine rows use one representative failure, and the name says which reason it holds, which matters because equality includes it.

Commit none. Nothing in this PR changes.
Test None. No behaviour is involved.

[Copilot]

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

taptopay/src/main/java/com/payabli/sdk/taptopay/session/TapToPaySessionManager.kt:148

• This adds a new log field name errorkind. If the SDK enforces an allowlist of loggable field names (as suggested by LoggableFieldNames), errorkind needs to be present there or it may be dropped/blocked, reducing observability of failure reasons. Consider adding errorkind to the allowlist (or reusing an existing standardized field name if one already exists in the logging vocabulary).

            logger.info(
                LogField.safe("event", "ttp_session_state"),
                LogField.safe("state", to.diagnosticName),
                LogField.safe("errorkind", (to as? TapToPaySessionState.Failed)?.reason?.name),
            ) { "session state changed" }

[Copilot]

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
96.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud