From ffb2b884dcf2139e7a05f19cc1ad065d76e84460 Mon Sep 17 00:00:00 2001 From: Static Date: Sat, 22 Aug 2026 00:17:53 -0400 Subject: [PATCH 1/4] fix: keep the price lookup alive when the connect race is lost A free user could open the unlock screen and find a disabled button spinning on "Checking price" forever, with no in-app way to clear it. Restore did not help, because Restore re-asks what you own and not what things cost. On the device tonight this reproduced on 10 of 10 cold starts. WHAT BREAKS. connect() calls startConnection while enableAutoServiceReconnection already has a connection in flight. The library rejects the second caller with DEVELOPER_ERROR and says so plainly: "Client is already in the process of connecting to billing service." Our setup listener treats any non-OK code as "Play is unavailable" and returns, so the refreshPurchases + loadProductDetails pair behind it never runs. Play is fine throughout. Finsky resolves the billing account normally in the same log window, and the connection the other caller opened serves queries happily. The entitlement restore proved it: it worked on every one of those same failed launches, including after a full pm clear. WHY THAT ASYMMETRY IS THE REAL BUG. refreshPurchases had three callers, so it healed itself and tested clean. loadProductDetails had exactly one, sitting inside the branch the race kills, so it failed silently and only for people who had not bought yet. A tester who owns the product cannot see this failure at all, which is why a passing purchase pass sat right next to it. So the fix is a second trigger, not a cleverer connect. Suppressing the double connect would mean timing our call against the library's internal reconnection schedule, which we do not control and which the next library update is free to change. Instead loadProductDetails now runs from the same foreground hook that already rescues refreshPurchases: a path observed working 10 times tonight. It returns immediately once a price is in hand, and a failed lookup leaves the product null so the next foreground retries. Comments at both sites record what was measured, so nobody restores the old single-trigger shape believing the setup callback is dependable. NOT UNIT TESTED, deliberately. BillingManager builds its BillingClient in its own constructor and no test in this repo touches either class. The honest options were a refactor I am not doing unattended, or a tautological policy object invented to look like coverage. Neither earns its keep. 416 existing tests still pass and the behavioural proof is named in the receipt. Co-Authored-By: Claude Opus 5 --- .../app/entitlement/BillingManager.kt | 60 ++++++++++++++++++- 1 file changed, 58 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/com/ninelivesaudio/app/entitlement/BillingManager.kt b/app/src/main/java/com/ninelivesaudio/app/entitlement/BillingManager.kt index aafbd09..a8d15e1 100644 --- a/app/src/main/java/com/ninelivesaudio/app/entitlement/BillingManager.kt +++ b/app/src/main/java/com/ninelivesaudio/app/entitlement/BillingManager.kt @@ -71,6 +71,8 @@ class BillingManager @Inject constructor( */ private val refreshMutex = Mutex() + private val productMutex = Mutex() + private val _unlockProduct = MutableStateFlow(null) /** @@ -112,12 +114,18 @@ class BillingManager @Inject constructor( * Also registers the foreground hook, because an out-of-app promo redemption * produces no purchase-update callback in this process. Without a query on * resume, redeeming a code and coming back leaves the app still locked. + * + * The foreground hook loads the price as well as the purchases, and that is + * the whole point rather than a convenience. See [loadProductDetails]. */ fun start() { ProcessLifecycleOwner.get().lifecycle.addObserver( object : DefaultLifecycleObserver { override fun onStart(owner: LifecycleOwner) { - scope.launch { refreshPurchases() } + scope.launch { + refreshPurchases() + loadProductDetails() + } } } ) @@ -133,6 +141,20 @@ class BillingManager @Inject constructor( // Not an error worth surfacing. A device with no Play Store, // or a Play Store mid-update, is a legitimate state. The user // keeps whatever entitlement they already had. + // + // Nor is it rare. [enableAutoServiceReconnection] can already + // have a connection in flight when this call reaches the + // library, and the library rejects the loser with + // DEVELOPER_ERROR: "Client is already in the process of + // connecting to billing service." That happened on 10 of 10 + // cold starts on a real device on 2026-08-22. The connection + // the other caller opened is live and serves queries fine, + // so this branch does not mean Play is unavailable. It means + // this particular listener will never hear back. + // + // Nothing may hang off this callback alone for that reason. + // Both calls below are also reachable from the foreground + // hook in [start], which is what makes them survive. Log.d(TAG, "billing setup finished: ${result.responseCode}") return } @@ -210,8 +232,42 @@ class BillingManager @Inject constructor( if (responseOk) acknowledgeIfNeeded(result.purchasesList) } - /** Load `nine_lives_unlock` so the unlock screen can show a real price. */ + /** + * Load `nine_lives_unlock` so the unlock screen can show a real price. + * + * Runs on launch, on every successful connect, and on every foreground. + * + * The foreground trigger is not belt and braces. Until 2026-08-22 the only + * caller was the success branch of [connect]'s setup listener, and that + * listener loses a startup race with the library's own auto-reconnection + * often enough to have failed 10 of 10 cold starts on a real device. When it + * lost, this never ran, `productLookupSettled` stayed false for the life of + * the process, and the unlock screen showed a disabled button spinning on + * "Checking price" that no in-app action could clear. The Restore button did + * not help: it re-asks what you own, not what things cost. + * + * The entitlement refresh survived the same race only because it had other + * callers. This one did not. That asymmetry was the actual defect, so the + * fix is a second trigger rather than a cleverer connect. + * + * Cheap to call repeatedly. It returns immediately once a price is in hand, + * and a failed lookup deliberately leaves the product null so the next + * foreground retries it. + */ suspend fun loadProductDetails() { + if (_unlockProduct.value != null) return + // Two foregrounds inside one slow query would ask Play the same question + // twice and write the same answer twice. Skip rather than queue, exactly + // as [refreshPurchases] does. + if (!productMutex.tryLock()) return + try { + loadProductDetailsLocked() + } finally { + productMutex.unlock() + } + } + + private suspend fun loadProductDetailsLocked() { val params = QueryProductDetailsParams.newBuilder() .setProductList( listOf( From c68479d152f5d7d2024efc8aad6fb872ee0e4566 Mon Sep 17 00:00:00 2001 From: Static Date: Sat, 22 Aug 2026 00:29:06 -0400 Subject: [PATCH 2/4] fix: re-query the price instead of pinning the first answer, and bump to 212 Review finding (P2) on the previous commit, and it was right. The early return on a cached ProductDetails meant that once a price landed it was never asked for again, where the code before this branch re-queried on every successful reconnect. That trades one bug for a quieter one. ProductDetails carries the offer token the purchase flow actually spends, prices vary by region, and this file already notes the ladder moves with each feature drop. An audiobook player process can live for days. A price pinned on first launch is a price that can be wrong by the time somebody presses the button, and the failure lands on the same button this branch exists to unbreak. Dropping the guard also makes the thing symmetric, which was the whole argument. refreshPurchases re-asks on every foreground. Now so does the price. The mutex still skips overlapping calls rather than queueing them, and a failed lookup leaves a previously good product in place, so one bad query cannot cost a working price. versionCode 212 rides along. 212 is the build that carries both the claim-prompt removal and this fix, and it is the one a Play reviewer should open. Folded in here rather than split into its own PR so the build needs one review cycle instead of two. 416 tests, 0 failures. assembleDebug clean. Co-Authored-By: Claude Opus 5 --- app/build.gradle.kts | 2 +- .../ninelivesaudio/app/entitlement/BillingManager.kt | 12 ++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/app/build.gradle.kts b/app/build.gradle.kts index c2af377..9385608 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -21,7 +21,7 @@ android { applicationId = "com.ninelivesaudio.app" minSdk = 30 targetSdk = 36 - versionCode = 211 + versionCode = 212 versionName = "2.1.0" testInstrumentationRunner = "androidx.test.runner.AndroidJUnitRunner" diff --git a/app/src/main/java/com/ninelivesaudio/app/entitlement/BillingManager.kt b/app/src/main/java/com/ninelivesaudio/app/entitlement/BillingManager.kt index a8d15e1..c68fe68 100644 --- a/app/src/main/java/com/ninelivesaudio/app/entitlement/BillingManager.kt +++ b/app/src/main/java/com/ninelivesaudio/app/entitlement/BillingManager.kt @@ -250,12 +250,16 @@ class BillingManager @Inject constructor( * callers. This one did not. That asymmetry was the actual defect, so the * fix is a second trigger rather than a cleverer connect. * - * Cheap to call repeatedly. It returns immediately once a price is in hand, - * and a failed lookup deliberately leaves the product null so the next - * foreground retries it. + * Re-queries every time rather than caching the first answer. Prices vary by + * region, the ladder moves with each feature drop, and the offer token + * inside [ProductDetails] is what the purchase flow actually spends. A + * player process can live for days, so a price pinned on first launch is a + * price that can be wrong by the time somebody presses the button. + * + * A failed lookup leaves any previously good product in place rather than + * clearing it, so one bad query never costs a working price. */ suspend fun loadProductDetails() { - if (_unlockProduct.value != null) return // Two foregrounds inside one slow query would ask Play the same question // twice and write the same answer twice. Skip rather than queue, exactly // as [refreshPurchases] does. From 90f7703e37e69247eb0587dc636932e65500be35 Mon Sep 17 00:00:00 2001 From: Static Date: Sat, 22 Aug 2026 00:40:12 -0400 Subject: [PATCH 3/4] fix: never let an empty product response erase a price we already have Review finding (P1) on the previous commit, introduced by that commit. Dropping the cache guard was right, but the guard had been doing two jobs and only one of them got credited. It also stopped the unconditional assignment below from ever running twice, which hid the fact that the assignment overwrites on every path. An OK response is not a promise that our product came back with it. Play reports unfetched products separately from the overall response code, so a successful query returning an empty or non-matching list is routine. That null was being written straight over a good price, leaving priceLookupSettled true with nothing to show, which renders the button as "Unavailable" and disables it. That is the same dead purchase surface this branch exists to remove, reached through a different door. Now the assignment happens only on a hit. Keeping the last known price carries the opposite risk, and a much smaller one: if the product really were deactivated, the user taps buy and Play declines. A recoverable error beats a button nobody can press. A device that never had a price still keeps null and still shows the unavailable copy, because there that is the honest answer. 416 tests, 0 failures. assembleDebug clean. Co-Authored-By: Claude Opus 5 --- .../app/entitlement/BillingManager.kt | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/com/ninelivesaudio/app/entitlement/BillingManager.kt b/app/src/main/java/com/ninelivesaudio/app/entitlement/BillingManager.kt index c68fe68..1b54401 100644 --- a/app/src/main/java/com/ninelivesaudio/app/entitlement/BillingManager.kt +++ b/app/src/main/java/com/ninelivesaudio/app/entitlement/BillingManager.kt @@ -299,8 +299,28 @@ class BillingManager @Inject constructor( return } - _unlockProduct.value = result.productDetailsList + val found = result.productDetailsList ?.firstOrNull { it.productId == PurchaseEvaluator.UNLOCK_PRODUCT_ID } + + // Only overwrite on a hit. An OK response is not a promise that our + // product came back with it: Play reports unfetched products separately + // from the overall response code, so an empty or non-matching list is a + // routine outcome rather than proof the product is gone. + // + // Assigning that null unconditionally would clear a price we already had + // and leave the button reading "Unavailable" with nothing the user can + // press, which is the same dead purchase surface this whole change + // exists to remove. Keeping the last known price risks the opposite and + // much smaller failure: if the product really were deactivated, the user + // taps buy and Play says no. A recoverable error beats a dead button. + // + // A device that never had a price keeps null and correctly shows the + // unavailable copy, because that is the honest answer there. + if (found != null) { + _unlockProduct.value = found + } else { + Log.d(TAG, "product lookup found no matching product, keeping last known price") + } } /** From 75057c1f06a8a7d1f3721da7c75dda70457bf69f Mon Sep 17 00:00:00 2001 From: Static Date: Sat, 22 Aug 2026 00:50:56 -0400 Subject: [PATCH 4/4] fix: treat a product with no purchasable offer as a miss Review finding, first half. oneTimePurchaseOfferDetails is nullable in the Billing API, and it is both the price the unlock screen displays and the object launchPurchase hands to Play. A product that matches the id but carries no offer prices as null, which disables the button exactly as thoroughly as having no product at all. Worse, it looked like a cache hit, so it would sit there and block the next lookup from replacing it with a good one. Matching the id is not enough. Match the id AND a usable offer, or treat the response as a miss and keep whatever last worked. Second half of that finding DECLINED for this branch, deliberately: re-querying product details inside the purchase path before launching the billing flow. Reasoning, so the next person does not have to guess. The staleness window is now at most one foreground, because this branch already makes the price re-query on every foreground rather than caching it. For the retained object to be genuinely unusable, Play has to have invalidated it inside that window. If it did, launchBillingFlow returns an error and the next foreground replaces the object. That is a recoverable failure. The proposed fix puts a network round trip in front of the single most important button in the app, adding latency and a brand new failure mode to the one interaction that must not break, and there is no device on this machine tonight to watch it behave. Trading a rare recoverable error for an untested change to the purchase interaction is the wrong direction at 1am. Filed as follow-up rather than merged blind. 416 tests, 0 failures. assembleDebug clean. Co-Authored-By: Claude Opus 5 --- .../ninelivesaudio/app/entitlement/BillingManager.kt | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/com/ninelivesaudio/app/entitlement/BillingManager.kt b/app/src/main/java/com/ninelivesaudio/app/entitlement/BillingManager.kt index 1b54401..bc5450e 100644 --- a/app/src/main/java/com/ninelivesaudio/app/entitlement/BillingManager.kt +++ b/app/src/main/java/com/ninelivesaudio/app/entitlement/BillingManager.kt @@ -299,8 +299,17 @@ class BillingManager @Inject constructor( return } + // Matching the id is not enough. oneTimePurchaseOfferDetails is nullable + // in the Billing API, and that field is both the price the screen shows + // and the thing [launchPurchase] hands to Play. A matching product with + // no offer prices as null, which disables the button just as thoroughly + // as having no product at all, except it also looks like a cache hit and + // so blocks the next lookup from replacing it. Treat it as a miss. val found = result.productDetailsList - ?.firstOrNull { it.productId == PurchaseEvaluator.UNLOCK_PRODUCT_ID } + ?.firstOrNull { + it.productId == PurchaseEvaluator.UNLOCK_PRODUCT_ID && + it.oneTimePurchaseOfferDetails != null + } // Only overwrite on a hit. An OK response is not a promise that our // product came back with it: Play reports unfetched products separately