From 47c5e0a1c85f2451d68005dc8f3ed4c7b70df598 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Fri, 7 Aug 2026 16:50:47 +1000 Subject: [PATCH 01/28] Pro: flag the false grace premise in the display mapper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comment here asserts the backend judges status against expiry + grace_period_duration. It does not โ€” it folds grace into the stored expiry before sending it, so the wire expiry is coverage end, not paid-through. Two consequences below it: inGracePeriod sits in a branch that requires now <= expiry so it is unreachable, and the renewal date renders one grace period late (an hour on Apple, operator-configured days on Google Play). Comment only. Desktop and iOS encode the same premise in different wordings, so correcting the logic is a three-client change and is with the architect. The superseded text is kept verbatim because the other clients' comments echo it and it needs to stay greppable. --- .../securesms/pro/ProDataMapper.kt | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProDataMapper.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProDataMapper.kt index d73aced4f4..a9acb557a5 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProDataMapper.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProDataMapper.kt @@ -30,6 +30,30 @@ fun GetProStatusResponse.toProStatus(nowMs: Long, context: Context, refundInProg return when (userStatus) { ProUserStatus.ACTIVE -> { val paymentItem = latestPayment ?: return ProStatus.NeverSubscribed + // ๐Ÿ”ด THE COMMENT BELOW IS KNOWN TO BE FALSE โ€” held under F8, unresolved as of 2026-08-07. + // Read this first; the logic still implements the superseded premise on purpose. + // + // The backend does NOT judge status against `expiry + grace_period_duration`. It folds + // grace into the stored expiry BEFORE sending it โ€” `Session-Pro-Backend` + // `backend.py` `_lookup_user_expiry`: `payment_expiry_at = expiry_at + grace if + // auto_renewing else expiry_at` -> `users.expiry_at` -> the wire `expiry_ts` + // (`server.py:317`), with the status boundary using that same value (`server.py:322`). + // Its own test subtracts grace from the wire value to recover the store's paid-through + // date (`tests/test_google.py:556-560`). + // + // So `expiry` is COVERAGE END, not paid-through. The real grace window is + // `expiry - gracePeriod <= now < expiry`, and two things below are therefore wrong: + // * `inGracePeriod = nowMs >= expiryMs` sits in a branch that requires `now <= expiry`, + // so the two are satisfiable only at a single instant โ€” the indicator is dead code. + // * `renewingAt = expiry` renders the renewal date one whole grace period late. + // + // Not fixed here because it is a user-visible date on all three clients, which encode the + // same premise in three different wordings โ€” which is why it read as corroboration rather + // than one mistake copied three times, and why it must be corrected on all three together + // rather than by whoever gets there first. + // + // --- SUPERSEDED (kept verbatim: it explains the existing diff, and the other two clients' + // --- comments echo this text, so it stays greppable for whoever does the three-client fix) // `expiry` is the paid-through end. user_status stays `active` through the grace window โ€” // the backend judges status against coverage_end = expiry + grace_period_duration (both gated // on auto_renewing) โ€” so being in this ACTIVE branch already means we are still covered. The From 913244734b2642c682fa0132f3702e4f3d61c019 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Fri, 7 Aug 2026 16:51:03 +1000 Subject: [PATCH 02/28] Pro: stop the proof worker depending on a status fetch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two couplings removed. It refreshed get_pro_status after minting a proof, which is redundant โ€” the access-expiry write it already makes is a config change that fires the status trigger on its own. And it gated on the in-memory load state holding an ACTIVE status, which is wrong in a way that only shows after a restart: WorkManager persists the proof schedule across process death but that state restarts empty, so a renewal falling due while the app was dead read as "not Pro, no purchase" and returned without renewing. It now asks libsession the same question the scheduler asks โ€” pro_renewal_target, from config. --- .../securesms/pro/ProProofGenerationWorker.kt | 61 +++++++++++-------- 1 file changed, 37 insertions(+), 24 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt index 17112b2324..6c1a58a74d 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt @@ -39,8 +39,9 @@ import javax.inject.Provider * A worker that generates a new [network.loki.messenger.libsession_util.pro.ProProof] and stores it * locally. * - * Normally you don't need to interact with this worker directly, as it is scheduled - * automatically when needed based on the Pro status state, by the [FetchProStatusWorker]. + * Normally you don't need to interact with this worker directly: + * [ProStatusManager.manageProofRenewalScheduling] schedules it off libsession's + * `pro_renewal_target`, recomputed whenever the config inputs to that target change. */ @HiltWorker class ProProofGenerationWorker @AssistedInject constructor( @@ -49,7 +50,6 @@ class ProProofGenerationWorker @AssistedInject constructor( private val apiExecutor: ServerApiExecutor, private val proBackendConfig: Provider, private val generateProProofApi: GenerateProProofApi.Factory, - private val proStatusRepository: ProStatusRepository, private val loginStateRepository: LoginStateRepository, private val configFactory: ConfigFactoryProtocol, private val snodeClock: SnodeClock, @@ -59,28 +59,41 @@ class ProProofGenerationWorker @AssistedInject constructor( "User must be logged to generate proof" } - // Run when we're already Pro (proof renewal) OR when a purchase is in flight (redemption): in the - // latter case get_pro_status may not be ACTIVE yet, and calling generate_pro_proof is what pulls - // the entitlement through once the backend has ingested the payment. If neither holds, nothing to do. - val isActive = proStatusRepository.loadState.value.lastUpdated?.first?.userStatus == ProUserStatus.ACTIVE - val purchasePending = configFactory.withUserConfigs { it.userProfile.getProPrepaid() } != null - if (!isActive && !purchasePending) { - Log.d(WORK_NAME, "Not Pro and no purchase in flight; nothing to generate") + // Ask libsession, from config, whether a proof is wanted at all โ€” the same question + // ProStatusManager asked to schedule us. It covers proof renewal, a purchase in flight + // (redemption, where get_pro_status is not ACTIVE yet and minting the proof is what pulls + // the entitlement through), and an entitlement held with no proof to attach. + // + // This used to read `proStatusRepository.loadState` for an ACTIVE status instead. That was + // the last status->proof dependency, and it was wrong in a way that only showed after a + // process restart: WorkManager persists our schedule, but `loadState` starts at Init, so a + // renewal that came due while the app was dead saw "not active, no purchase" and returned + // without renewing. + val now = snodeClock.currentTime() + val (renewalTarget, purchasePending, proof) = configFactory.withUserConfigs { configs -> + Triple( + configs.userProfile.getProRenewalTarget(now.epochSecond), + configs.userProfile.getProPrepaid() != null, + configs.userProfile.getProConfig()?.proProof, + ) + } + + if (renewalTarget == null) { + Log.d(WORK_NAME, "No Pro proof renewal is due; nothing to generate") return Result.success() } - // Pace acquisition. Without a floor this path is a closed loop: a successful generate - // force-refreshes get_pro_status (below), the fetch asks libsession for a renewal target, and - // `target = proofExpiry - PRO_RENEWAL_LEAD` is permanently in the past whenever a proof lives - // for less than the 60-minute lead, so it reschedules us immediately, forever. + // Pace acquisition. Without a floor this path is a closed loop: a successful generate writes + // the new proof to config, ProStatusManager.manageProofRenewalScheduling re-reads the renewal + // target off that write, and `target = proofExpiry - PRO_RENEWAL_LEAD` is permanently in the + // past whenever a proof lives for less than the 60-minute lead โ€” so it reschedules us + // immediately, forever. (Before the status/proof split the loop ran through a forced + // get_pro_status instead; same shape, and the floor is what bounds it either way.) // // Mirrors iOS `SessionProManager.reconcileProofRenewal` and Desktop `ducks/proBackendData.ts`, // constants included. Note it RE-ARMS rather than skipping: `target <= now` is the normal // "renewal due" signal, so dropping the work would break real renewals. - val now = snodeClock.currentTime() - val covered = configFactory.withUserConfigs { configs -> - configs.userProfile.getProConfig()?.proProof - }?.let { it.expirySeconds > now.epochSecond } == true + val covered = proof?.let { it.expirySeconds > now.epochSecond } == true if (covered) darkAttempt = 0 val intervalSeconds = if (covered) { @@ -144,12 +157,12 @@ class ProProofGenerationWorker @AssistedInject constructor( } Log.d(WORK_NAME, "Successfully generated a new pro proof expiring at ${Instant.ofEpochSecond(proof.expirySeconds)}") - // Minting the proof is what makes the backend validate the payment and mark the - // account active, so a get_pro_status fetched before now (e.g. the one behind the - // Pro settings screen right after a purchase) is stale "expired". Refresh the - // display-only status so the UI flips to active on its own, instead of the user - // having to hit "Check Pro Status" manually. - proStatusRepository.requestRefresh(force = true) + // No status refresh is requested here. Minting the proof does flip the account + // active at the backend, so the display status does need re-reading โ€” but the + // `setProAccessExpiry` write above already fires the (E, prepaid) config-change + // trigger in ProStatusManager, which schedules exactly that fetch. Asking for + // one here as well made the proof loop a *source* of status fetches, which is + // the coupling this rework removes. Result.success() } From e4db9f9876e6a4f726574e4ae19d7da6f17c7f44 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Fri, 7 Aug 2026 16:51:03 +1000 Subject: [PATCH 03/28] Pro: add attempt-stamped pro_state keys for the status floor and the startup gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two new name/value rows, no migration. Both stamp the ATTEMPT rather than the success: a request that reached the server costs it the same whether or not it succeeded, and stamping on success makes a client retry hardest exactly when the server is least able to take it. They are separate keys on purpose. A routine refresh must not consume the gate's 24h budget, and a startup fetch from twenty hours ago must not satisfy the 60s floor โ€” one value cannot answer both. Neither can reuse pro_status_updated_at, which is written in the same statement as the response blob and is unreadable without it, so a failed fetch has nothing to record there. --- .../securesms/pro/db/ProDatabase.kt | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/db/ProDatabase.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/db/ProDatabase.kt index f2e98336d6..cace7aae34 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/db/ProDatabase.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/db/ProDatabase.kt @@ -185,6 +185,65 @@ class ProDatabase @Inject constructor( } } + /** + * When a `get_pro_status` fetch was last **attempted**, successful or not. + * + * Deliberately separate from [getProStatusAndLastUpdated]'s timestamp, which cannot serve this + * purpose: that value is written as a pair with the response blob and is only readable when both + * are present, so a failed fetch has nothing to record there. Using it as the freshness floor + * therefore meant a failing network was never throttled at all โ€” every trigger and every cold + * launch re-attempted, which is the load the floor exists to prevent. + */ + fun getProStatusLastAttemptAt(): Instant? { + return readableDatabase.query( + "SELECT value FROM pro_state WHERE name = ?", + arrayOf(STATE_PRO_STATUS_LAST_ATTEMPT_AT) + ).use { cursor -> + if (cursor.moveToFirst()) Instant.ofEpochMilli(cursor.getString(0).toLong()) else null + } + } + + /** + * When the STARTUP GATE last attempted a fetch. Separate from [getProStatusLastAttemptAt] on + * purpose: the gate's 24h interval must not be consumed or reset by a routine refresh, and the + * 60s floor must not be satisfied by a startup fetch from twenty hours ago. One value cannot + * answer both questions. + */ + fun getProStatusLastStartupFetchAttemptAt(): Instant? { + return readableDatabase.query( + "SELECT value FROM pro_state WHERE name = ?", + arrayOf(STATE_PRO_STATUS_LAST_STARTUP_FETCH_ATTEMPT_AT) + ).use { cursor -> + if (cursor.moveToFirst()) Instant.ofEpochMilli(cursor.getString(0).toLong()) else null + } + } + + /** See [getProStatusLastStartupFetchAttemptAt]. Attempt-stamped, like the floor's key. */ + fun setProStatusLastStartupFetchAttemptAt(attemptedAt: Instant) { + writableDatabase.compileStatement(""" + INSERT OR REPLACE INTO pro_state (name, value) + VALUES (?, ?) + """).use { stmt -> + stmt.bindString(1, STATE_PRO_STATUS_LAST_STARTUP_FETCH_ATTEMPT_AT) + stmt.bindString(2, attemptedAt.toEpochMilli().toString()) + // Must be executed โ€” binding alone writes nothing, and the failure is silent. + stmt.executeInsert() + } + } + + /** Records a fetch attempt. Deliberately no change notification โ€” this is not display state. */ + fun setProStatusLastAttemptAt(attemptedAt: Instant) { + writableDatabase.compileStatement(""" + INSERT OR REPLACE INTO pro_state (name, value) + VALUES (?, ?) + """).use { stmt -> + stmt.bindString(1, STATE_PRO_STATUS_LAST_ATTEMPT_AT) + stmt.bindString(2, attemptedAt.toEpochMilli().toString()) + // Must be executed โ€” binding alone writes nothing, and the failure is silent. + stmt.executeInsert() + } + } + fun updateProStatus(proStatus: GetProStatusResponse, updatedAt: Instant) { val changes = writableDatabase.compileStatement(""" INSERT INTO pro_state (name, value) @@ -214,6 +273,15 @@ class ProDatabase @Inject constructor( private const val STATE_PRO_STATUS = "pro_status" private const val STATE_PRO_STATUS_UPDATED_AT = "pro_status_updated_at" + // Written on every fetch ATTEMPT, unlike STATE_PRO_STATUS_UPDATED_AT which is written only + // alongside a successful response. No migration needed: pro_state is a name/value table. + private const val STATE_PRO_STATUS_LAST_ATTEMPT_AT = "pro_status_last_attempt_at" + + // The startup gate's own 24h interval. Attempt-stamped like the above, and deliberately a + // SEPARATE key โ€” see getProStatusLastStartupFetchAttemptAt. + private const val STATE_PRO_STATUS_LAST_STARTUP_FETCH_ATTEMPT_AT = + "pro_status_last_startup_fetch_attempt_at" + private const val ROTATING_KEY_VALIDITY_DAYS = 15 fun createTable(db: SupportSQLiteDatabase) { From 3a096cbe6485101abf7e7259cee3a513734e62d0 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Fri, 7 Aug 2026 16:51:23 +1000 Subject: [PATCH 04/28] Pro: gate the startup fetch, make the floor real, and stop reporting an unconfirmed status as confirmed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One commit because force -> immediate renames a parameter every one of these files calls, so they cannot compile apart. - Gate the startup get_pro_status fetch on whether a home CTA could plausibly fire, from synced config, with a persisted 24h minimum. Every cold start used to fetch, including for users who have never subscribed, and mobile cold-starts constantly. - Make the 60s floor apply. It asked whether the load state was Loading or Loaded; a cold start begins at Init, which is neither, so it was skipped on exactly the path it exists for โ€” and every background caller passed force=true anyway. It now reads the persisted attempt timestamp, and a process that has never fetched is exempt so the floor cannot become the "is one already running" mutex. - force -> immediate, reserved to three callers named in the KDoc. - Only report a confirmed status when THIS process confirmed it. The state mapping ended in a catch-all that swept up both Init and a Loaded restored from WorkManager's persisted work state, so at launch the refresh state read Success against a cached response and a renewal that happened while the app was closed splashed the Expired CTA off stale data. Reported in the wild. The catch-all is gone: the mapping is exhaustive, so the next state added has to declare which it is. - Pro settings: refresh on open, and while open poll once a minute from when the renewal falls due until it lands, coverage ends or the screen closes. The grace label now needs a completed fetch at or after the crossing โ€” a snapshot predating it cannot have seen the failure it warns about. - Persist auto_renewing into config alongside the access expiry, unconditionally: libsession short-circuits a no-change write on a clean config, and a presence-based guard would be wrong because the key is erased rather than stored when false. Two gate rows are deliberately built wrong and pinned by tests that will fail when the decisions land: row 1 cannot see the real grace window (the expiry is grace-inclusive), and the not-auto-renewing-but-active row declines to fetch while auto_renewing is presence-only. Both match Desktop and iOS; the PR description carries them. --- .../prosettings/ProSettingsViewModel.kt | 124 ++++++++++- .../securesms/pro/FetchProStatusWorker.kt | 71 +++--- .../securesms/pro/ProStatusManager.kt | 209 ++++++++++++++++-- .../securesms/pro/ProStatusRepository.kt | 176 +++++++++++++-- .../securesms/pro/ProStartupGateTest.kt | 107 +++++++++ .../pro/ProStatusFreshnessFloorTest.kt | 107 +++++++++ 6 files changed, 707 insertions(+), 87 deletions(-) create mode 100644 app/src/test/java/org/thoughtcrime/securesms/pro/ProStartupGateTest.kt create mode 100644 app/src/test/java/org/thoughtcrime/securesms/pro/ProStatusFreshnessFloorTest.kt diff --git a/app/src/main/java/org/thoughtcrime/securesms/preferences/prosettings/ProSettingsViewModel.kt b/app/src/main/java/org/thoughtcrime/securesms/preferences/prosettings/ProSettingsViewModel.kt index 426012bcc0..51544672da 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/preferences/prosettings/ProSettingsViewModel.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/preferences/prosettings/ProSettingsViewModel.kt @@ -22,10 +22,13 @@ import kotlinx.coroutines.delay import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.collectLatest +import kotlinx.coroutines.flow.distinctUntilChanged +import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.update import kotlinx.coroutines.launch import kotlinx.coroutines.withContext import network.loki.messenger.R +import network.loki.messenger.libsession_util.pro.GetProStatusResponse import org.session.libsession.database.StorageProtocol import org.session.libsession.network.SnodeClock import org.session.libsession.utilities.ConfigFactoryProtocol @@ -64,6 +67,7 @@ import org.thoughtcrime.securesms.util.DateUtils import org.thoughtcrime.securesms.util.State import java.math.BigDecimal import java.time.Duration +import java.time.Instant import kotlin.time.Duration.Companion.minutes import kotlin.time.Duration.Companion.seconds @@ -106,6 +110,22 @@ class ProSettingsViewModel @AssistedInject constructor( private var recovering: Boolean = false init { + // Trigger #3 โ€” refresh on entering Pro settings. Floored, not `immediate`: this screen is + // the one place the status is actually read, so it should not be showing a value from an + // arbitrarily old background fetch, but arriving here is not on its own a reason to bypass + // the 60s floor. + // + // Deliberately NOT via refreshProStatus(): that early-returns while `refreshState` is + // Loading, and since F15 a process that hasn't confirmed a fetch of its own reports Loading + // from launch. Routing on-enter through that guard would mean the one trigger able to + // resolve the state is the one the state suppresses โ€” a spinner that never clears, which is + // the failure iOS hit from the same direction. The repository single-flights anyway + // (WorkManager REPLACE), so the guard buys nothing here. + proStatusRepository.requestRefresh(immediate = false) + + // Trigger #4 โ€” the bounded grace poll, for as long as this screen is open. + pollProStatusDuringGraceWhileOpen() + // observe subscription status viewModelScope.launch { proStatusManager @@ -196,17 +216,35 @@ class ProSettingsViewModel @AssistedInject constructor( recovering = false } + // Addendum item 2 debounce, extended from the home splash gate to the in-settings label. + // A status snapshot taken BEFORE the renewal fell due cannot tell us the renewal failed โ€” + // it predates the event. Showing the grace warning off one turns an ordinary boundary + // crossing into a "renewal unsuccessful" alarm the backend never reported. + // + // The condition is a COMPLETED fetch at or after the crossing, not "a fetch succeeded at + // some point": `lastUpdated` is stamped by ProDatabase.updateProStatus once the response + // has landed, so a request still in flight when the threshold passed doesn't satisfy it. + // Cross-client contract, matching iOS's `lastStatusFetch >= E`. + val confirmedSinceRenewalDue = proStatusRepository.loadState.value.lastUpdated + ?.let { (status, fetchedAt) -> + status.renewalDueAt()?.let { !fetchedAt.isBefore(it) } + } == true + while (true) { val now = clock.currentTime() _proSettingsUIState.update { it.copy( proDataState = proDataState, - inGracePeriod = (subType as? ProStatus.Active.AutoRenewing)?.inGracePeriod ?: false, + inGracePeriod = confirmedSinceRenewalDue && + (subType as? ProStatus.Active.AutoRenewing)?.inGracePeriod == true, subscriptionExpiryLabel = when(subType){ is ProStatus.Active.AutoRenewing -> { - // in grace period - if(subType.inGracePeriod) { + // in grace period โ€” debounced, same condition as the `inGracePeriod` + // flag above. Reading `subType.inGracePeriod` directly here would + // reintroduce the unconfirmed alarm through the label while the flag + // driving the warning colour stayed correctly suppressed. + if(confirmedSinceRenewalDue && subType.inGracePeriod) { Phrase.from(context, R.string.proRenewalUnsuccessful) .format() } else { @@ -696,12 +734,74 @@ class ProSettingsViewModel @AssistedInject constructor( } } - private fun refreshProStatus(force: Boolean){ + /** + * Trigger #4 โ€” poll `get_pro_status` while the renewal is overdue and this screen is open. + * + * Deliberately NOT a 60s timer on the screen. It sleeps until the renewal falls due and only + * then polls, once a minute, while the renewal still hasn't landed. Three things stop it: the + * renewal arrives (`expiry` advances, which restarts this from the new date), the account runs + * past coverage, or the screen closes and cancels `viewModelScope`. + * + * Note it needs no `auto_renewing` check. The wire zeroes `grace_period_duration` when the + * subscription isn't auto-renewing, so for those accounts coverage ends exactly when the + * renewal falls due and the loop below never runs a single iteration โ€” there is no renewal in + * flight to poll for, and the arithmetic already says so. + * + * Exempt from the freshness floor (spec ยง4: bounded polls carry their own cadence and their own + * termination). At 60s this poll sits exactly on the 60s floor, so leaving it floored would drop + * ticks to timing jitter alone. + */ + private fun pollProStatusDuringGraceWhileOpen() { + viewModelScope.launch { + proStatusRepository.loadState + .map { it.lastUpdated?.first } + .distinctUntilChanged { old, new -> old?.expiry == new?.expiry } + .collectLatest { status -> + val renewalDue = status?.renewalDueAt() ?: return@collectLatest + val coverageEnd = status.coverageEndsAt() ?: return@collectLatest + + // Returns immediately when already past it โ€” i.e. the screen was opened + // mid-grace โ€” which is exactly when we want the first poll to be now. + clock.delayUntil(renewalDue) + + while (clock.currentTime().isBefore(coverageEnd)) { + proStatusRepository.requestRefresh(immediate = true) + delay(GRACE_POLL_INTERVAL_MS) + } + } + } + } + + /** + * The instant the renewal falls due โ€” the account's paid-through end. + * + * โš ๏ธ CONTESTED, and this pair of functions is the whole of the disagreement. The spec (ยง0, ยง6) + * reads `expiry` as the paid-through end, so the renewal is due at `expiry` and grace runs from + * there to `expiry + gracePeriod`. The Pro backend disagrees: it folds grace into the stored + * expiry before sending it (`backend.py` `_lookup_user_expiry`), so `expiry` is already the end + * of coverage and the paid-through end is `expiry - gracePeriod`. + * + * That is with Morgan and the architect. Until it is ruled on these follow the SPEC, so Android + * does not quietly disagree with Desktop and iOS about a user-visible date. If the backend + * reading wins, swap the two bodies โ€” the poll's structure holds under either, and nothing else + * in this file reads the boundary. + */ + private fun GetProStatusResponse.renewalDueAt(): Instant? = expiry + + /** The instant coverage ends. See [renewalDueAt] โ€” same caveat, same seam. */ + private fun GetProStatusResponse.coverageEndsAt(): Instant? = expiry?.plus(gracePeriod) + + /** + * [immediate] bypasses the repository's freshness floor. Every caller here is a user-initiated + * refresh (a retry button, recover, returning from cancellation) โ€” trigger #5 โ€” so they pass + * true: the user is looking at the screen waiting for the answer. + */ + private fun refreshProStatus(immediate: Boolean){ // stop early if we are already refreshing if(_proSettingsUIState.value.proDataState.refreshState is State.Loading) return // refreshes the pro status data - proStatusRepository.requestRefresh(force = force) + proStatusRepository.requestRefresh(immediate = immediate) } private fun getSelectedPlan(): ProPlan? { @@ -988,4 +1088,18 @@ class ProSettingsViewModel @AssistedInject constructor( val showTCPolicyDialog: Boolean = false, val showSimpleDialog: SimpleDialogData? = null, ) + + companion object { + /** + * Cadence of the #4 grace poll. Shared cross-client contract (spec ยง9.3) โ€” the same 60s + * Desktop and iOS use for their while-open poll; keep them in step. + * + * **This equals `ProStatusRepository.MIN_UPDATE_INTERVAL_SECONDS`, and that equality is the + * reason #4 bypasses the freshness floor** rather than being floored like the other routine + * triggers. A poll running at exactly the floor would have roughly every other tick dropped + * by timing jitter alone, silently halving the rate. Two files apart the two constants look + * coincidentally equal; they are not, so change neither without the other. + */ + private const val GRACE_POLL_INTERVAL_MS = 60_000L + } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/FetchProStatusWorker.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/FetchProStatusWorker.kt index ae81b2c296..08d3d1e00c 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/FetchProStatusWorker.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/FetchProStatusWorker.kt @@ -27,7 +27,6 @@ import org.session.libsignal.utilities.Log import org.thoughtcrime.securesms.api.server.ServerApiExecutor import org.thoughtcrime.securesms.api.server.execute import org.thoughtcrime.securesms.auth.LoginStateRepository -import network.loki.messenger.libsession_util.pro.GetProStatusResponse import org.thoughtcrime.securesms.pro.api.GetProStatusApi import org.thoughtcrime.securesms.pro.api.ServerApiRequest import org.thoughtcrime.securesms.pro.api.successOrThrow @@ -39,13 +38,13 @@ import javax.inject.Provider * A worker that fetches the user's Pro status from the server and updates the local database. * * This worker doesn't do any business logic in terms of when to schedule itself, it simply performs - * the fetch and update operation regardlessly. It, however, does schedule the [ProProofGenerationWorker] - * if needed based on the fetched Pro status, this is because the proof generation logic - * is tightly coupled to the fetched Pro status state. + * the fetch and update operation regardlessly โ€” and that is now the whole of its job. It used to + * also schedule [ProProofGenerationWorker], which made proof renewal a downstream effect of a + * status fetch; that scheduling lives in [ProStatusManager] and keys off config instead. */ @HiltWorker class FetchProStatusWorker @AssistedInject constructor( - @Assisted private val context: Context, + @Assisted context: Context, @Assisted params: WorkerParameters, private val proBackendConfig: Provider, private val serverApiExecutor: ServerApiExecutor, @@ -61,6 +60,11 @@ class FetchProStatusWorker @AssistedInject constructor( "User must be logged in to fetch pro status" } + // Record the attempt before making it, so one that fails still spaces out the next. The + // success timestamp can't do this job: it is stored as a pair with the response blob, so a + // failed fetch leaves nothing behind and a failing network was never throttled at all. + proDatabase.setProStatusLastAttemptAt(snodeClock.currentTime()) + return try { Log.d(TAG, "Fetching Pro status from server") val details = serverApiExecutor.execute( @@ -86,12 +90,24 @@ class FetchProStatusWorker @AssistedInject constructor( configs.userProfile.removeProAccessExpiry() } + // Persist auto-renewing into synced config alongside E, so a linked device has the + // account state without its own fetch. Written unconditionally and deliberately so: + // libsession's set_nonzero_int short-circuits a no-change write on a clean config + // (`assign_if_changed`), exactly as the E write above already relies on, so a + // client-side "only if changed" guard would add nothing. A presence-based guard + // would be actively wrong โ€” the key is erased rather than stored when false, so + // presence flips on every transition and would read as churn that isn't there. + // + // No `t`/`T` bump either: this is backend-derived state like E and I, not a user + // profile edit, and libsession omits the bump for it on purpose. + configs.userProfile.setProAutoRenewing(details.autoRenewing) + // Remove the pro config only when the backend authoritatively says we are no longer // pro (expired) or never were (never). An unknown/future status is NOT a basis to // delete it: removeProConfig() writes the SYNCED user profile, so clearing on an // unrecognised status would erase a valid proof across all the user's devices. Leave // it โ€” the proof's own expiry governs, and the backend won't refresh (or will revoke) - // a genuinely-lapsed account. We schedule proof generation below if we are still pro. + // a genuinely-lapsed account. if (details.userStatus == ProUserStatus.EXPIRED || details.userStatus == ProUserStatus.NEVER ) { @@ -107,8 +123,10 @@ class FetchProStatusWorker @AssistedInject constructor( } proDatabase.updateProStatus(proStatus = details, updatedAt = snodeClock.currentTime()) - scheduleProofGenerationIfNeeded(details) - + // Proof generation is NOT scheduled from here. It runs off libsession's + // `pro_renewal_target`, watched from config by + // ProStatusManager.manageProofRenewalScheduling โ€” so the config writes above reach it + // anyway, and a renewal no longer depends on a status fetch having happened. Result.success() } catch (e: CancellationException) { Log.d(TAG, "Work cancelled") @@ -123,43 +141,6 @@ class FetchProStatusWorker @AssistedInject constructor( } - private suspend fun scheduleProofGenerationIfNeeded(details: GetProStatusResponse) { - if (details.userStatus != ProUserStatus.ACTIVE) { - // Not (yet) Pro โ€” but if a purchase is in flight (possibly synced from another device that - // bought and set pro_prepaid), keep driving the redemption poll so any device can pull the - // entitlement through. Otherwise there's nothing to generate. - val purchasePending = configFactory.withUserConfigs { it.userProfile.getProPrepaid() } != null - if (purchasePending) { - Log.d(TAG, "Not active but a purchase is in flight; scheduling proof redemption") - ProProofGenerationWorker.schedule(context) - } else { - Log.d(TAG, "Pro is not active, cancelling any existing proof generation work") - ProProofGenerationWorker.cancel(context) - } - return - } - - // libsession owns the renewal schedule now โ€” no more client-side autoRenewing/expiry logic (which - // was inconsistent and skipped non-auto-renewing but still-valid entitlements). getProRenewalTarget - // returns null (valid proof, no renewal needed), a target <= now (renew now), or a future target - // (~1h before proof expiry, nudged off the rotation-period boundary so all devices converge). - val nowSeconds = snodeClock.currentTime().epochSecond - val target = configFactory.withUserConfigs { it.userProfile.getProRenewalTarget(nowSeconds) } - if (target == null) { - Log.d(TAG, "Pro proof is still valid; no renewal needed") - return - } - - val delay = Duration.ofSeconds((target - nowSeconds).coerceAtLeast(0L)) - if (delay.isZero) { - Log.d(TAG, "Pro proof needs (re)generation now, scheduling immediately") - ProProofGenerationWorker.schedule(context) - } else { - Log.d(TAG, "Pro proof renewal due in $delay, scheduling") - ProProofGenerationWorker.schedule(context, delay) - } - } - companion object { private const val TAG = "FetchProStatusWorker" diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt index 144b573eff..42f1e59abb 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt @@ -19,9 +19,8 @@ import kotlinx.coroutines.flow.debounce import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.filter import kotlinx.coroutines.flow.filterNotNull -import kotlinx.coroutines.flow.flowOf +import kotlinx.coroutines.flow.flow import kotlinx.coroutines.flow.map -import kotlinx.coroutines.flow.mapLatest import kotlinx.coroutines.flow.mapNotNull import kotlinx.coroutines.flow.merge import kotlinx.coroutines.flow.onStart @@ -127,14 +126,29 @@ class ProStatusManager @Inject constructor( DebugMenuViewModel.DebugProPlanStatus.LOADING -> State.Loading DebugMenuViewModel.DebugProPlanStatus.ERROR -> State.Error(Exception()) else -> { - // calculate the real refresh state here + // The real refresh state. `Success` means THIS PROCESS has had a fetch confirmed + // by the backend โ€” nothing weaker. Consumers gate on it to avoid acting on stale + // data (`HomeViewModel`'s Expired CTA above all), so anything else must not + // report success. + // + // Exhaustive on purpose, with no `else`. It previously ended in + // `else -> State.Success(Unit)`, which quietly swept up two states that are not + // successes: `Init` (nothing has happened yet) and โ€” the one that caused a live + // bug โ€” a `Loaded` restored from WorkManager's PERSISTED work state, i.e. a fetch + // some earlier process made. A renewal that happened while the app was closed + // then showed the Expired CTA off the stale cache on the next launch. Listing + // every case means the next state added here has to declare which it is. when(proStatusState){ is ProStatusRepository.LoadState.Loading -> { if(proStatusState.waitingForNetwork) State.Error(Exception()) else State.Loading } is ProStatusRepository.LoadState.Error -> State.Error(Exception()) - else -> State.Success(Unit) + is ProStatusRepository.LoadState.Loaded -> { + if (proStatusState.confirmedInThisProcess) State.Success(Unit) + else State.Loading + } + ProStatusRepository.LoadState.Init -> State.Loading } } } @@ -274,6 +288,7 @@ class ProStatusManager @Inject constructor( launch { manageOtherPeoplePro() } launch { manageProStatusRefreshScheduling() } + launch { manageProofRenewalScheduling() } launch { manageCurrentProProofRevocation() } } @@ -346,23 +361,12 @@ class ProStatusManager @Inject constructor( } }, - configFactory.get() - .watchUserProConfig() - .filterNotNull() - .distinctUntilChanged() - .mapLatest { proConfig -> - val expiry = Instant.ofEpochSecond(proConfig.proProof.expirySeconds) - // Wake ~1h before proof expiry so the renewal path runs. Deterministic - // (no client-side jitter): per-device random offsets leak device count - // via the landed-renewal order statistic; libsession owns the timing - // (renewal_target), and config resolution settles concurrent renewals. - val refreshTime = expiry.minus(Duration.ofMinutes(60)) - - snodeClock.delayUntil(refreshTime) - "Pro proof expiry reached" - }, + // NOTE: there is deliberately no trigger keyed to PROOF expiry here. Proof timing + // drives proof renewal (see manageProofRenewalScheduling) and nothing else; a status + // fetch scheduled off the proof's clock coupled the two loops together, so a proof + // that renewed early or late dragged the status fetch with it. - flowOf("App starting up") + startupGate() ).debounce(500.milliseconds) .collect { refreshReason -> Log.d( @@ -370,7 +374,98 @@ class ProStatusManager @Inject constructor( "Scheduling ProStatus fetch due to: $refreshReason" ) - proStatusRepository.get().requestRefresh(force = true) + // Background triggers respect the freshness floor. `immediate` is for the two + // paths where the user is waiting: the post-purchase poll and manual/recover. + proStatusRepository.get().requestRefresh() + } + } + + /** + * Trigger #1 โ€” the startup fetch, gated. + * + * Every client used to fetch `get_pro_status` on every cold start, including users who have never + * subscribed and users who are comfortably paid up. "Cold start" is something mobile does + * constantly, and none of those fetches had a consumer: entitlement runs off the proof, the + * settings screen refreshes when opened, and account-expiry awareness is the `E+30s` wake. The + * only real consumer is the home CTAs, so the gate asks whether a CTA could plausibly fire, from + * synced config alone, and otherwise stays off the network entirely. + * + * Two independent brakes: the CTA-worthiness test below, and a persisted 24h minimum between + * startup fetches. The interval has its own key โ€” a routine refresh must not consume the gate's + * budget, and a startup fetch from twenty hours ago must not satisfy the 60s floor. + */ + private fun startupGate(): Flow = flow { + val now = snodeClock.currentTime() + + val lastStartupFetch = proDatabase.getProStatusLastStartupFetchAttemptAt() + if (lastStartupFetch != null && lastStartupFetch.plus(STARTUP_MIN_INTERVAL).isAfter(now)) { + Log.d(DebugLogGroup.PRO_SUBSCRIPTION.label, "Startup gate: fetched within the last $STARTUP_MIN_INTERVAL, skipping") + return@flow + } + + val (accessExpiry, autoRenewing) = configFactory.get().withUserConfigs { configs -> + configs.userProfile.getProAccessExpiry()?.let(Instant::ofEpochSecond) to + configs.userProfile.getProAutoRenewing() + } + + val reason = startupFetchReason(accessExpiry, autoRenewing, now) + if (reason == null) { + Log.d(DebugLogGroup.PRO_SUBSCRIPTION.label, "Startup gate: no CTA could fire, skipping the startup fetch") + return@flow + } + + // Stamped on ATTEMPT, matching the floor's key: a fetch that fails still costs the backend, + // and stamping on success would retry hardest exactly when the server is least able to take it. + proDatabase.setProStatusLastStartupFetchAttemptAt(now) + emit("App starting up โ€” $reason") + } + + /** + * Drives proof acquisition/renewal purely from config, off libsession's `pro_renewal_target`. + * + * This used to be kicked by [FetchProStatusWorker] off the get_pro_status response, which made + * the proof loop a downstream effect of a status fetch: no fetch, no renewal. The inputs + * libsession actually needs โ€” the stored proof, the access expiry (E) and the prepaid marker + * (I) โ€” all live in the user profile, so watching them directly is both sufficient and honest + * about the dependency. + * + * The loop closes without a status fetch anywhere in it: the proof worker's own config writes + * (a new proof, a refreshed or cleared E) re-enter here and schedule the next attempt, and a + * `null` target โ€” no proof and no entitlement signalled โ€” cancels the work outright. + */ + @OptIn(FlowPreview::class) + private suspend fun manageProofRenewalScheduling() { + configFactory.get() + .userConfigsChanged(EnumSet.of(UserConfigType.USER_PROFILE)) + .castAwayType() + .onStart { emit(Unit) } + .map { + configFactory.get().withUserConfigs { configs -> + // Only the three inputs to pro_renewal_target, so an unrelated profile edit + // (name, avatar) doesn't take the config lock again to recompute the same answer. + Triple( + configs.userProfile.getProConfig()?.proProof?.expirySeconds, + configs.userProfile.getProAccessExpiry(), + configs.userProfile.getProPrepaid(), + ) + } + } + .distinctUntilChanged() + .debounce(500.milliseconds) + .collectLatest { + val nowSeconds = snodeClock.currentTime().epochSecond + val target = configFactory.get() + .withUserConfigs { it.userProfile.getProRenewalTarget(nowSeconds) } + + if (target == null) { + Log.d(DebugLogGroup.PRO_SUBSCRIPTION.label, "No Pro proof renewal needed; cancelling any scheduled work") + ProProofGenerationWorker.cancel(application) + return@collectLatest + } + + val delay = Duration.ofSeconds((target - nowSeconds).coerceAtLeast(0L)) + Log.d(DebugLogGroup.PRO_SUBSCRIPTION.label, "Pro proof renewal due in $delay; scheduling") + ProProofGenerationWorker.schedule(application, delay.takeIf { !it.isZero }) } } @@ -511,7 +606,7 @@ class ProStatusManager @Inject constructor( // Fire immediately (over onion routing the request often reaches the backend after // the store's async notification already did). val before = repo.loadState.value.lastUpdated?.second - repo.requestRefresh(force = true) + repo.requestRefresh(immediate = true) // Pace off COMPLETION, not request-start: requestRefresh enqueues with REPLACE, so // re-firing while a slow request is in flight would just cancel and restart it forever. // Wait for THIS fetch to settle โ€” a newer Loaded, or an Error (failure/timeout). @@ -534,6 +629,76 @@ class ProStatusManager @Inject constructor( } companion object { + /** + * Startup-gate constants. **Shared cross-client contract** (spec ยง9.3) โ€” Desktop and iOS use + * the same values; keep them in step, and say why in the commit if they ever diverge. + */ + private val STARTUP_MIN_INTERVAL: Duration = Duration.ofHours(24) + private val EXPIRING_CTA_WINDOW: Duration = Duration.ofDays(7) + private val EXPIRED_CTA_WINDOW: Duration = Duration.ofDays(30) + + /** + * Whether a cold start should fetch `get_pro_status`, and why โ€” or null to stay off the + * network. Pure over plain values so it can be tested without a clock, a database or config. + * + * The architect's four rows, which REPLACE the spec's `E + grace โ‰ค now` expired test (grace + * is not in config, so that test is unimplementable): + * + * | config state | action | + * |---------------------------------------------|-----------------------------------------| + * | `auto_renewing && now < E` | no fetch โ€” comfortably active | + * | `auto_renewing && now โ‰ฅ E` | fetch; grace is unknowable from config | + * | `!auto_renewing && E` within the CTA window | fetch โ€” the Expiring CTA may fire | + * | `!auto_renewing && now โ‰ฅ E` | confirm-fetch before the Expired CTA | + * + * โš ๏ธ **Row 1 is known to be wrong, and is built this way deliberately โ€” see F8.** `E` is + * grace-INCLUSIVE (the backend folds grace in before sending it), so the real grace window + * `E โˆ’ grace โ‰ค now < E` lies entirely inside `now < E` โ€” the row that declines to fetch. The + * state this redesign exists to surface is therefore the one the gate is currently blind to. + * Desktop and iOS ship the identical row; correcting it is a three-client change and all + * three PRs document it. Do not fix it here alone. + * + * โš ๏ธ **The `!auto_renewing && now < E && outside the CTA window` case is HELD โ€” see F2.** It + * returns null (the spec's letter: comfortably-active users never fetch on startup), but that + * is not a settled answer. `A` is presence-only, so `false` also means "never written" โ€” and + * every existing subscriber lands here on their first run after this ships, where declining + * to fetch means `A` is never written and the gate never changes its mind. Whether that needs + * a bootstrap fetch is Morgan's, and it is one branch of this `when`. + */ + internal fun startupFetchReason( + accessExpiry: Instant?, + autoRenewing: Boolean, + now: Instant, + ): String? { + // No access expiry at all: never subscribed, so no CTA can fire and nothing to confirm. + if (accessExpiry == null) return null + + val pastExpiry = !now.isBefore(accessExpiry) + + return when { + autoRenewing && !pastExpiry -> null + + autoRenewing -> "auto-renewing and past the access expiry; grace is not knowable from config" + + !pastExpiry -> + if (accessExpiry.isBefore(now.plus(EXPIRING_CTA_WINDOW))) { + "not auto-renewing and expiring within $EXPIRING_CTA_WINDOW" + } else { + // HELD (F2) โ€” the unnamed row. + null + } + + // Past expiry and not auto-renewing. Confirm with the backend before showing the + // Expired CTA: config can read expired while a renewal landed on another device and + // hasn't synced. Bounded by the CTA's own window โ€” once the CTA can no longer fire + // there is nothing for the fetch to serve. + accessExpiry.plus(EXPIRED_CTA_WINDOW).isAfter(now) -> + "not auto-renewing and expired within $EXPIRED_CTA_WINDOW; confirming before the Expired CTA" + + else -> null + } + } + // Bounded post-purchase get_pro_status poll (the backend learns of the payment out-of-band via // an async store notification): after each fetch settles, wait 5s and retry until it's been ~2 // minutes since the first request, so slow/timing-out onion requests still get a few attempts. diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusRepository.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusRepository.kt index f7d06dae71..48409c3412 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusRepository.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusRepository.kt @@ -11,6 +11,7 @@ import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.onStart import kotlinx.coroutines.flow.stateIn +import kotlinx.coroutines.launch import org.session.libsession.network.SnodeClock import org.session.libsignal.utilities.Log import org.thoughtcrime.securesms.auth.LoginStateRepository @@ -28,7 +29,7 @@ class ProStatusRepository @Inject constructor( private val application: Application, private val db: ProDatabase, private val snodeClock: SnodeClock, - @ManagerScope scope: CoroutineScope, + @param:ManagerScope private val scope: CoroutineScope, loginStateRepository: LoginStateRepository, private val networkConnectivity: NetworkConnectivity, ) { @@ -45,7 +46,18 @@ class ProStatusRepository @Inject constructor( val waitingForNetwork: Boolean ) : LoadState - data class Loaded(override val lastUpdated: Pair) : LoadState + /** + * A status fetch has succeeded โ€” but [confirmedInThisProcess] says whether it was OURS. + * + * WorkManager persists a unique work's terminal state, so at process start `watch()` replays + * the PREVIOUS run's `SUCCEEDED` and this state is reached before we have asked anyone + * anything. Callers that mean "the backend has confirmed this for us" must check the flag; + * `Loaded` alone does not mean that and never did. + */ + data class Loaded( + override val lastUpdated: Pair, + val confirmedInThisProcess: Boolean, + ) : LoadState data class Error(override val lastUpdated: Pair?) : LoadState } @@ -68,7 +80,12 @@ class ProStatusRepository @Inject constructor( WorkInfo.State.SUCCEEDED -> { if (last != null) { Log.d(DebugLogGroup.PRO_DATA.label, "Successfully fetched Pro status from backend") - LoadState.Loaded(last) + LoadState.Loaded( + lastUpdated = last, + // The success timestamp is stamped on completion, so "at or after this + // process started observing" is exactly "this process confirmed it". + confirmedInThisProcess = !last.second.isBefore(processStartedAt), + ) } else { // This should never happen, but just in case... LoadState.Error(null) @@ -82,25 +99,154 @@ class ProStatusRepository @Inject constructor( /** - * Requests a fresh of current user's pro status. By default, if last update is recent enough, - * no network request will be made. If [force] is true, a network request will be - * made regardless of the freshness of the last update. + * When this process started observing status. + * + * Used to tell a fetch WE completed from one restored out of WorkManager's persisted state. Both + * failure directions are safe: a clock that runs backwards, or a singleton constructed late, make + * a genuine confirmation read as unconfirmed, which suppresses rather than asserts. + */ + private val processStartedAt: Instant = snodeClock.currentTime() + + /** + * Requests a refresh of the current user's Pro status. By default the request is dropped when + * the last successful fetch is recent enough. + * + * [immediate] bypasses that floor. It is ONE mechanism with a closed list of sanctioned + * callers โ€” adding a fourth is a cross-client decision, not a local one: + * + * - **#5 manual refresh / recover** (`ProSettingsViewModel`) โ€” the user is watching. + * - **#7 post-purchase poll** (`ProStatusManager.pollProStatusAfterPurchase`) โ€” bounded, and + * the user is waiting on the entitlement. + * - **#4 while-open grace poll** (`ProSettingsViewModel`) โ€” bounded and self-terminating. It + * bypasses for a mechanical reason rather than an urgency one: its cadence + * (`GRACE_POLL_INTERVAL_MS`) is *exactly* [MIN_UPDATE_INTERVAL_SECONDS], so leaving it + * floored would drop roughly every other tick to timing jitter and silently halve the poll + * rate. + * + * Everything else โ€” startup, config-change, the `E+30s` wake, on-enter โ€” goes through the + * floor. That is what stops several triggers coinciding (a config change and a timer, say) + * from each costing a fetch. + */ + /** + * Whether THIS process has asked for a status fetch yet. Deliberately in-memory and deliberately + * not the same thing as the persisted timestamp: see [shouldFetch]. + * + * โš ๏ธ **LOAD-BEARING โ€” do not delete this as redundant with the persisted timestamp.** It looks + * redundant, and it is not: the persisted value answers *"was the last fetch recent?"*, this + * answers *"has this process asked at all?"*. + * + * Its remaining job is to guarantee the FIRST request of a process reaches the network โ€” and to + * be precise about why that job exists: **this is what stops the floor becoming a mutex.** + * + * Three separate things can refuse to start a status fetch, and they are easy to conflate: + * + * 1. **in flight** โ€” a request is already running, + * 2. **unconfirmed** โ€” we have no confirmed status (now [LoadState.Loaded.confirmedInThisProcess]), + * 3. **too soon** โ€” this floor. + * + * Only (3) is **persisted**, so it is the only one that can refuse in a process that has never + * fetched at all: it reads a timestamp that outlived the process that wrote it. That is the same + * shape as the `Loaded(stale)` bug โ€” durable state answering a per-process question โ€” arriving + * through a different mechanism. Without this exemption, a relaunch inside the interval would be + * refused by a decision no part of this process ever took, and after the startup gate nothing + * else would ask. + * + * That reasoning survives someone fixing the confirmed-status problem another way, which the + * previous "restores the Loading transition" justification did not. + * + * (It used to have a second job โ€” restoring the `Loading` transition that suppressed the home + * Expired CTA. That is now done properly by [LoadState.Loaded.confirmedInThisProcess], so this + * field is no longer what stands between a stale cache and a false CTA. The retirement condition + * is therefore no longer "when the predicate is fixed" โ€” the predicate IS fixed and this is still + * needed.) + * + * The tempting cleanup is a demonstrated failure, not a hypothetical one: the Desktop client + * removed its equivalent per-run value during the same rework and would have shipped a + * permanently-spinning Pro screen. */ - fun requestRefresh(force: Boolean = false) { - val currentState = loadState.value - if (!force && (currentState is LoadState.Loading || currentState is LoadState.Loaded) && - currentState.lastUpdated?.second?.plusSeconds(MIN_UPDATE_INTERVAL_SECONDS) - ?.isAfter(snodeClock.currentTime()) == true) { - Log.d(DebugLogGroup.PRO_DATA.label, "Pro status are fresh enough, skipping refresh") + @Volatile + private var fetchedInThisProcess = false + + fun requestRefresh(immediate: Boolean = false) { + if (immediate) { + Log.d(DebugLogGroup.PRO_DATA.label, "Scheduling immediate fetch of Pro status from server") + fetchedInThisProcess = true + FetchProStatusWorker.schedule(application, ExistingWorkPolicy.REPLACE) return } - Log.d(DebugLogGroup.PRO_DATA.label, "Scheduling fetch of Pro status from server") - FetchProStatusWorker.schedule(application, ExistingWorkPolicy.REPLACE) + // The floor reads the persisted timestamp rather than `loadState`. `loadState` is a + // StateFlow starting at LoadState.Init, and Init is neither Loading nor Loaded, so the + // old check was skipped outright until the database combine had emitted โ€” which the + // startup trigger beats. The result was that the floor never applied to the one fetch it + // most needed to cover. `pro_status_updated_at` is already written by every successful + // fetch (ProDatabase.updateProStatus), so there is nothing new to persist. + // + // The scope is GlobalScope (Dispatchers.Default), so the read is off the main thread; the + // callers are all fire-and-forget. + scope.launch { + // The ATTEMPT timestamp, not the success one (F10, pending Morgan): a failed fetch costs + // the backend the same as a successful one, and gating on success meant a failing + // network re-attempted on every trigger and every cold launch. + val lastFetchedAt = db.getProStatusLastAttemptAt() + if (!shouldFetch( + immediate = false, + fetchedInThisProcess = fetchedInThisProcess, + lastFetchedAt = lastFetchedAt, + now = snodeClock.currentTime(), + ) + ) { + Log.d(DebugLogGroup.PRO_DATA.label, "Pro status are fresh enough, skipping refresh") + return@launch + } + + Log.d(DebugLogGroup.PRO_DATA.label, "Scheduling fetch of Pro status from server") + fetchedInThisProcess = true + FetchProStatusWorker.schedule(application, ExistingWorkPolicy.REPLACE) + } } companion object { - private const val MIN_UPDATE_INTERVAL_SECONDS = 60L + /** + * The status freshness floor. **Shared cross-client contract** โ€” Desktop and iOS use the + * same 60s; keep them in step, and say why in the commit if they ever have to diverge. + */ + const val MIN_UPDATE_INTERVAL_SECONDS = 60L + + /** + * The whole of the floor decision, over plain values so it can be tested without a + * database, a clock or WorkManager. + * + * Expressed over the **timestamp**, never over a load-state enum. That distinction is the + * bug this replaced: the old check asked whether the in-memory state was `Loading`/`Loaded`, + * and the state a cold start begins in โ€” `Init` โ€” is neither, so the floor was skipped + * outright on exactly the path it exists to cover. An absent timestamp means "no successful + * fetch on record", which is a genuine reason to fetch; an initial enum value is not. + * + * Drop-on-fresh, not re-arm (spec ยง4): a caller whose request is dropped here does not get + * a later one scheduled on its behalf. The proof loop is deliberately the opposite. + * + * [fetchedInThisProcess] is the second exemption, and it needs the persisted timestamp to + * exist before it makes sense โ€” the two are not redundant. A process that has never fetched + * always fetches once, however fresh the stored value is, because several things downstream + * key off *this process* having confirmed the status rather than off the status being + * recent. On Android that is the false-expired protection: the home Expired CTA is gated on + * `refreshState is State.Success` (`HomeViewModel`), and `Init` maps to `Success` โ€” so what + * actually suppressed the CTA until confirmation was the `Loading` transition a real fetch + * produces. Floor the very first request of a process and that transition never happens, + * and the CTA fires off whatever the cache last held. Cold-start load stays bounded by the + * 24h startup gate, which is the stronger limit anyway. + */ + fun shouldFetch( + immediate: Boolean, + fetchedInThisProcess: Boolean, + lastFetchedAt: Instant?, + now: Instant, + ): Boolean = + immediate || + !fetchedInThisProcess || + lastFetchedAt == null || + !lastFetchedAt.plusSeconds(MIN_UPDATE_INTERVAL_SECONDS).isAfter(now) } } \ No newline at end of file diff --git a/app/src/test/java/org/thoughtcrime/securesms/pro/ProStartupGateTest.kt b/app/src/test/java/org/thoughtcrime/securesms/pro/ProStartupGateTest.kt new file mode 100644 index 0000000000..805eb2d519 --- /dev/null +++ b/app/src/test/java/org/thoughtcrime/securesms/pro/ProStartupGateTest.kt @@ -0,0 +1,107 @@ +package org.thoughtcrime.securesms.pro + +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +import org.junit.Test +import org.thoughtcrime.securesms.pro.ProStatusManager.Companion.startupFetchReason +import java.time.Duration +import java.time.Instant + +/** + * The startup gate's decision (spec ยง2 trigger #1 / ยง4), as a pure function of + * (access expiry, auto-renewing, now). + * + * Scope: these cover the decision only. They do NOT cover the 24h persisted interval or the config + * read โ€” both need a database, and the interval is checked before this function is reached. + * + * **Two of these tests pin behaviour we believe is WRONG**, deliberately: the F8 row-1 blind spot and + * the F2 held row. They are written so that the day either ruling lands, the test fails and points at + * the decision rather than silently accepting a change. Read their comments before "fixing" them. + */ +class ProStartupGateTest { + + private val now: Instant = Instant.parse("2026-08-07T00:00:00Z") + + private fun inDays(days: Long): Instant = now.plus(Duration.ofDays(days)) + private fun daysAgo(days: Long): Instant = now.minus(Duration.ofDays(days)) + + // --- never subscribed ----------------------------------------------------------------------- + + @Test + fun `no access expiry means no fetch`() { + // The population the gate exists for: users who have never subscribed were fetching on every + // cold start and could never see a CTA. + assertNull(startupFetchReason(accessExpiry = null, autoRenewing = false, now = now)) + assertNull(startupFetchReason(accessExpiry = null, autoRenewing = true, now = now)) + } + + // --- rows 1 and 2: auto-renewing ------------------------------------------------------------ + + @Test + fun `row 1 - auto-renewing and comfortably active does not fetch`() { + assertNull(startupFetchReason(inDays(20), autoRenewing = true, now = now)) + } + + @Test + fun `row 2 - auto-renewing and past the access expiry fetches`() { + // Grace is not derivable from config, so the only way to learn where we stand is to ask. + assertNotNull(startupFetchReason(daysAgo(1), autoRenewing = true, now = now)) + } + + @Test + fun `row 2 boundary - exactly at the access expiry fetches`() { + assertNotNull(startupFetchReason(now, autoRenewing = true, now = now)) + } + + // --- rows 3 and 4: not auto-renewing -------------------------------------------------------- + + @Test + fun `row 3 - expiring inside the CTA window fetches`() { + assertNotNull(startupFetchReason(inDays(3), autoRenewing = false, now = now)) + } + + @Test + fun `row 3 boundary - just outside the 7 day window does not fetch`() { + assertNull(startupFetchReason(inDays(8), autoRenewing = false, now = now)) + } + + @Test + fun `row 4 - recently expired fetches to confirm before the Expired CTA`() { + // Config can read expired while a renewal landed on another device and hasn't synced, so the + // Expired CTA must never fire off config alone. + assertNotNull(startupFetchReason(daysAgo(5), autoRenewing = false, now = now)) + } + + @Test + fun `row 4 boundary - expired longer ago than the CTA window does not fetch`() { + // Past 30 days the Expired CTA can no longer fire, so a confirming fetch has no consumer. + assertNull(startupFetchReason(daysAgo(31), autoRenewing = false, now = now)) + } + + // --- the two seams: pinned as-built, both believed wrong ------------------------------------- + + @Test + fun `F8 SEAM - row 1 does not fetch during the real grace window, and that is the known flaw`() { + // `E` is grace-INCLUSIVE, so the real grace window (E - grace <= now < E) lies entirely + // inside row 1's `now < E`. An auto-renewing user whose renewal is overdue but who is still + // covered is exactly the state this redesign exists to surface โ€” and the gate declines to + // fetch for them. + // + // Pinned so the blind spot is visible rather than incidental. When F8 is ruled on and the + // boundary moves to `E - grace`, this assertion SHOULD fail โ€” at which point the fix is to + // change the gate and invert this test, not to delete it. + val graceWindow = startupFetchReason(inDays(1), autoRenewing = true, now = now) + assertNull(graceWindow) + } + + @Test + fun `F2 SEAM - not auto-renewing, active, outside the CTA window does not fetch`() { + // The held row. Returns null per the spec's letter ("comfortably-active users never fetch on + // startup"), but `A` is presence-only, so `autoRenewing = false` here also covers "never + // written" โ€” which is where every existing subscriber lands on their first run after this + // ships. Declining means `A` is never written and the gate never revises its answer. + // + // If F2 rules for bootstrap-on-unknown this becomes a fetch and the test must change with it. + assertNull(startupFetchReason(inDays(60), autoRenewing = false, now = now)) + } +} diff --git a/app/src/test/java/org/thoughtcrime/securesms/pro/ProStatusFreshnessFloorTest.kt b/app/src/test/java/org/thoughtcrime/securesms/pro/ProStatusFreshnessFloorTest.kt new file mode 100644 index 0000000000..2757725348 --- /dev/null +++ b/app/src/test/java/org/thoughtcrime/securesms/pro/ProStatusFreshnessFloorTest.kt @@ -0,0 +1,107 @@ +package org.thoughtcrime.securesms.pro + +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test +import org.thoughtcrime.securesms.pro.ProStatusRepository.Companion.MIN_UPDATE_INTERVAL_SECONDS +import org.thoughtcrime.securesms.pro.ProStatusRepository.Companion.shouldFetch +import java.time.Instant + +/** + * The `get_pro_status` freshness floor. + * + * Scope, stated plainly: these cover the floor's **decision**, which is a pure function of + * (immediate, last-fetch timestamp, now). They do NOT cover the wiring โ€” that `requestRefresh` + * reads the timestamp from `pro_state` rather than from `loadState`, and that a dropped request + * really does skip `FetchProStatusWorker`. Both of those need WorkManager and a real database, so + * they are not reachable from a JVM unit test here. + * + * That boundary matters because the wiring is where the original bug was: the floor asked whether + * the in-memory load state was `Loading`/`Loaded`, and a cold start begins at `Init`, which is + * neither โ€” so the floor was skipped on precisely the path it existed for. What these tests pin is + * the shape that prevents it recurring: the decision is expressed over the timestamp, and "no + * timestamp" is a distinct, deliberate answer rather than a state that falls between the cases. + */ +class ProStatusFreshnessFloorTest { + + private val now: Instant = Instant.parse("2026-08-07T00:00:00Z") + + @Test + fun `no recorded fetch means fetch`() { + // A cold start with an empty pro_state, or a user who has never fetched. Distinct from the + // old failure: this is "no evidence of a recent fetch", not "the state enum hasn't settled". + assertTrue(shouldFetch(immediate = false, fetchedInThisProcess = true, lastFetchedAt = null, now = now)) + } + + @Test + fun `a fetch inside the interval is dropped`() { + val recent = now.minusSeconds(MIN_UPDATE_INTERVAL_SECONDS / 2) + assertFalse(shouldFetch(immediate = false, fetchedInThisProcess = true, lastFetchedAt = recent, now = now)) + } + + @Test + fun `a fetch older than the interval is allowed`() { + val old = now.minusSeconds(MIN_UPDATE_INTERVAL_SECONDS + 1) + assertTrue(shouldFetch(immediate = false, fetchedInThisProcess = true, lastFetchedAt = old, now = now)) + } + + @Test + fun `the interval boundary is inclusive - exactly the interval ago is allowed`() { + // Pinned deliberately: at exactly the floor the request goes through. The #4 grace poll + // runs at exactly this cadence, so an exclusive boundary here would drop alternate ticks + // to nothing but timing jitter. + val exactly = now.minusSeconds(MIN_UPDATE_INTERVAL_SECONDS) + assertTrue(shouldFetch(immediate = false, fetchedInThisProcess = true, lastFetchedAt = exactly, now = now)) + } + + @Test + fun `immediate bypasses the floor even on a fetch that just happened`() { + assertTrue(shouldFetch(immediate = true, fetchedInThisProcess = true, lastFetchedAt = now, now = now)) + } + + @Test + fun `immediate is the only bypass - a just-completed fetch is otherwise dropped`() { + // The negative control for the test above: same inputs, immediate off. Without this pair, + // the immediate test passes just as well against a function that always returns true. + assertFalse(shouldFetch(immediate = false, fetchedInThisProcess = true, lastFetchedAt = now, now = now)) + } + + @Test + fun `the first request of a process is never floored`() { + // Second exemption. A relaunch inside 60s of the last fetch would otherwise be dropped, and + // several things downstream key off THIS process having confirmed the status rather than + // off the stored value being recent โ€” on Android, the home Expired CTA, which is gated on a + // `Loading` transition that only a real fetch produces. + assertTrue( + shouldFetch( + immediate = false, + fetchedInThisProcess = false, + lastFetchedAt = now.minusSeconds(1), + now = now, + ) + ) + } + + @Test + fun `once this process has fetched the floor applies again`() { + // Negative control for the exemption: same inputs, flag flipped. Without this the test + // above passes against an exemption that never turns off. + assertFalse( + shouldFetch( + immediate = false, + fetchedInThisProcess = true, + lastFetchedAt = now.minusSeconds(1), + now = now, + ) + ) + } + + @Test + fun `a future timestamp still floors`() { + // Clock skew, or a fetch recorded against network time while we compare against a slightly + // behind reading. Treat it as fresh rather than fetching: the alternative reads a skewed + // clock as a licence to bypass the floor entirely. + val future = now.plusSeconds(MIN_UPDATE_INTERVAL_SECONDS) + assertFalse(shouldFetch(immediate = false, fetchedInThisProcess = true, lastFetchedAt = future, now = now)) + } +} From 24c1667fb067eafb2ca8604a1c55d6f29459eae7 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Mon, 10 Aug 2026 12:33:43 +1000 Subject: [PATCH 05/28] Pro: subtract grace to get the paid-through date, and gate on when the renewal is actually due MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The access expiry get_pro_status sends is grace-INCLUSIVE โ€” the backend folds grace in before sending it and judges "active" against that same value, and its own test subtracts grace from the wire value to recover the store's date. So the expiry is coverage end, and the renewal falls due a grace period earlier. Three things were wrong as a result: - The renewal date rendered a whole grace period late. - The grace indicator was unreachable: it tested now >= expiry inside a branch that requires now <= expiry, so only a single instant satisfied both. - The startup gate's first row declined to fetch for the whole grace window, because that window lies inside "now < coverage end" โ€” the gate was blind to exactly the state this rework exists to surface. Subtracting is unconditional and needs no provider branching: the wire sends grace = 0 whenever the subscription is not auto-renewing. Not cosmetic where it mattered. Apple configures no grace, so the backend's ~1h stand-in is the whole of it. Google's is the operator-configured base-plan value in DAYS, fetched exactly when the subscriber enters grace โ€” so the date was days wrong on the screen whose purpose is that date, and most wrong precisely when someone was looking at it. Also moves the grace warning's "a completed fetch at or after the renewal fell due" condition into toProStatus, where inGracePeriod is produced, instead of applying it at each consumer. Five call sites read that flag; a sixth would have inherited no protection. Needs the grace key in synced config, so the wrapper's submodule pin moves with it. --- .../prosettings/ProSettingsViewModel.kt | 51 +++----- .../securesms/pro/ProDataMapper.kt | 79 +++++++------ .../securesms/pro/ProStatusManager.kt | 79 +++++++------ .../securesms/pro/ProStatusRepository.kt | 7 +- .../securesms/pro/ProStartupGateTest.kt | 109 ++++++++++-------- 5 files changed, 167 insertions(+), 158 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/preferences/prosettings/ProSettingsViewModel.kt b/app/src/main/java/org/thoughtcrime/securesms/preferences/prosettings/ProSettingsViewModel.kt index 51544672da..83c35f293a 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/preferences/prosettings/ProSettingsViewModel.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/preferences/prosettings/ProSettingsViewModel.kt @@ -116,8 +116,8 @@ class ProSettingsViewModel @AssistedInject constructor( // the 60s floor. // // Deliberately NOT via refreshProStatus(): that early-returns while `refreshState` is - // Loading, and since F15 a process that hasn't confirmed a fetch of its own reports Loading - // from launch. Routing on-enter through that guard would mean the one trigger able to + // Loading, and a process that hasn't confirmed a fetch of its own reports Loading from + // launch. Routing on-enter through that guard would mean the one trigger able to // resolve the state is the one the state suppresses โ€” a spinner that never clears, which is // the failure iOS hit from the same direction. The repository single-flights anyway // (WorkManager REPLACE), so the guard buys nothing here. @@ -216,35 +216,21 @@ class ProSettingsViewModel @AssistedInject constructor( recovering = false } - // Addendum item 2 debounce, extended from the home splash gate to the in-settings label. - // A status snapshot taken BEFORE the renewal fell due cannot tell us the renewal failed โ€” - // it predates the event. Showing the grace warning off one turns an ordinary boundary - // crossing into a "renewal unsuccessful" alarm the backend never reported. - // - // The condition is a COMPLETED fetch at or after the crossing, not "a fetch succeeded at - // some point": `lastUpdated` is stamped by ProDatabase.updateProStatus once the response - // has landed, so a request still in flight when the threshold passed doesn't satisfy it. - // Cross-client contract, matching iOS's `lastStatusFetch >= E`. - val confirmedSinceRenewalDue = proStatusRepository.loadState.value.lastUpdated - ?.let { (status, fetchedAt) -> - status.renewalDueAt()?.let { !fetchedAt.isBefore(it) } - } == true - + // The grace warning's "a completed fetch at or after the crossing" condition is applied + // inside `toProStatus`, where inGracePeriod is produced โ€” so `subType.inGracePeriod` is + // already safe to read directly here and at the label below. It used to be gated at each + // consumer instead, which meant a new reader inherited no protection. while (true) { val now = clock.currentTime() _proSettingsUIState.update { it.copy( proDataState = proDataState, - inGracePeriod = confirmedSinceRenewalDue && - (subType as? ProStatus.Active.AutoRenewing)?.inGracePeriod == true, + inGracePeriod = (subType as? ProStatus.Active.AutoRenewing)?.inGracePeriod == true, subscriptionExpiryLabel = when(subType){ is ProStatus.Active.AutoRenewing -> { - // in grace period โ€” debounced, same condition as the `inGracePeriod` - // flag above. Reading `subType.inGracePeriod` directly here would - // reintroduce the unconfirmed alarm through the label while the flag - // driving the warning colour stayed correctly suppressed. - if(confirmedSinceRenewalDue && subType.inGracePeriod) { + // in grace period โ€” already debounced at construction (see toProStatus) + if(subType.inGracePeriod) { Phrase.from(context, R.string.proRenewalUnsuccessful) .format() } else { @@ -775,21 +761,14 @@ class ProSettingsViewModel @AssistedInject constructor( /** * The instant the renewal falls due โ€” the account's paid-through end. * - * โš ๏ธ CONTESTED, and this pair of functions is the whole of the disagreement. The spec (ยง0, ยง6) - * reads `expiry` as the paid-through end, so the renewal is due at `expiry` and grace runs from - * there to `expiry + gracePeriod`. The Pro backend disagrees: it folds grace into the stored - * expiry before sending it (`backend.py` `_lookup_user_expiry`), so `expiry` is already the end - * of coverage and the paid-through end is `expiry - gracePeriod`. - * - * That is with Morgan and the architect. Until it is ruled on these follow the SPEC, so Android - * does not quietly disagree with Desktop and iOS about a user-visible date. If the backend - * reading wins, swap the two bodies โ€” the poll's structure holds under either, and nothing else - * in this file reads the boundary. + * `expiry` is COVERAGE END, not paid-through: the backend folds grace into it before sending it, + * so the renewal was due a grace period earlier. Subtracting is unconditional โ€” the wire sends + * grace = 0 when the subscription is not auto-renewing, so this is a no-op for those accounts. */ - private fun GetProStatusResponse.renewalDueAt(): Instant? = expiry + private fun GetProStatusResponse.renewalDueAt(): Instant? = expiry?.minus(gracePeriod) - /** The instant coverage ends. See [renewalDueAt] โ€” same caveat, same seam. */ - private fun GetProStatusResponse.coverageEndsAt(): Instant? = expiry?.plus(gracePeriod) + /** The instant coverage really ends โ€” the wire value as sent. See [renewalDueAt]. */ + private fun GetProStatusResponse.coverageEndsAt(): Instant? = expiry /** * [immediate] bypasses the repository's freshness floor. Every caller here is a user-initiated diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProDataMapper.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProDataMapper.kt index a9acb557a5..8dff6feef3 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProDataMapper.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProDataMapper.kt @@ -26,43 +26,48 @@ object ProUserStatus { * Map a libsession-parsed get-pro-status response to the app's [ProStatus] domain model. Needs a [Context] * to resolve the (client-owned) provider display strings. */ -fun GetProStatusResponse.toProStatus(nowMs: Long, context: Context, refundInProgress: Boolean): ProStatus { +/** + * @param confirmedAt when the fetch behind this response COMPLETED, or null if nothing has been + * confirmed. Used to gate [ProStatus.Active.AutoRenewing.inGracePeriod]: a snapshot taken before the + * renewal fell due cannot have observed it failing, so raising the "renewal unsuccessful" warning off + * one turns an ordinary boundary crossing into an alarm the backend never reported. + * + * Gated **here**, where the flag is produced, rather than at each consumer. Five call sites read it + * today; a sixth would inherit no protection if the check lived in front of the value instead of + * inside it โ€” and there would be nothing to tell whoever added it. + */ +fun GetProStatusResponse.toProStatus( + nowMs: Long, + context: Context, + refundInProgress: Boolean, + confirmedAt: Instant?, +): ProStatus { return when (userStatus) { ProUserStatus.ACTIVE -> { val paymentItem = latestPayment ?: return ProStatus.NeverSubscribed - // ๐Ÿ”ด THE COMMENT BELOW IS KNOWN TO BE FALSE โ€” held under F8, unresolved as of 2026-08-07. - // Read this first; the logic still implements the superseded premise on purpose. - // - // The backend does NOT judge status against `expiry + grace_period_duration`. It folds - // grace into the stored expiry BEFORE sending it โ€” `Session-Pro-Backend` - // `backend.py` `_lookup_user_expiry`: `payment_expiry_at = expiry_at + grace if - // auto_renewing else expiry_at` -> `users.expiry_at` -> the wire `expiry_ts` - // (`server.py:317`), with the status boundary using that same value (`server.py:322`). - // Its own test subtracts grace from the wire value to recover the store's paid-through - // date (`tests/test_google.py:556-560`). - // - // So `expiry` is COVERAGE END, not paid-through. The real grace window is - // `expiry - gracePeriod <= now < expiry`, and two things below are therefore wrong: - // * `inGracePeriod = nowMs >= expiryMs` sits in a branch that requires `now <= expiry`, - // so the two are satisfiable only at a single instant โ€” the indicator is dead code. - // * `renewingAt = expiry` renders the renewal date one whole grace period late. + // `expiry` is COVERAGE END, not the paid-through date: the backend folds grace into it + // before sending it (`Session-Pro-Backend` `backend.py` `_lookup_user_expiry`: + // `payment_expiry_at = expiry_at + grace if auto_renewing else expiry_at` -> + // `users.expiry_at` -> the wire `expiry_ts`, `server.py:317`), and judges `active` against + // that same value (`:322`). Its own test subtracts grace from the wire value to recover + // the store's date (`tests/test_google.py:556-560`). // - // Not fixed here because it is a user-visible date on all three clients, which encode the - // same premise in three different wordings โ€” which is why it read as corroboration rather - // than one mistake copied three times, and why it must be corrected on all three together - // rather than by whoever gets there first. + // So the renewal is due at `expiry - gracePeriod`, and the grace window is + // `expiry - gracePeriod <= now < expiry`. Subtracting is unconditional and needs no + // provider branching: the wire sends grace = 0 whenever the subscription is not + // auto-renewing (`server.py:335`), so `expiry - 0 == expiry` for those accounts. // - // --- SUPERSEDED (kept verbatim: it explains the existing diff, and the other two clients' - // --- comments echo this text, so it stays greppable for whoever does the three-client fix) - // `expiry` is the paid-through end. user_status stays `active` through the grace window โ€” - // the backend judges status against coverage_end = expiry + grace_period_duration (both gated - // on auto_renewing) โ€” so being in this ACTIVE branch already means we are still covered. The - // renewal is due AT the paid-through end; being past it while still active IS the grace period. - // Never subtract grace here: that put "renew due" a whole grace period in the past, which - // (in sandbox, where grace โ‰ซ the compressed period) made inGracePeriod perpetually true. - val accountExpiry = expiry ?: return ProStatus.NeverSubscribed - val expiryMs = accountExpiry.toEpochMilli() - val renewingAt = accountExpiry + // This was wrong in the opposite direction until 2026-08-10 โ€” it treated `expiry` as + // paid-through and never subtracted, which made `inGracePeriod` unreachable (it sits in a + // branch requiring `now <= expiry`) and rendered the renewal date a whole grace period + // late. Not cosmetic where it mattered: Apple's grace is the backend's own ~1h stand-in, + // but Google's is the operator-configured base-plan value in DAYS, fetched exactly when + // the subscriber enters grace โ€” so the date was days wrong on the screen whose purpose is + // that date, and most wrong precisely when someone was looking at it. + val coverageEnd = expiry ?: return ProStatus.NeverSubscribed + // The paid-through end โ€” when the renewal actually falls due. + val renewingAt = coverageEnd.minus(gracePeriod) + val renewingAtMs = renewingAt.toEpochMilli() val providerData = providerMetadata(paymentItem.paymentProvider, context) val duration = paymentItem.toProPlanPeriod() @@ -82,8 +87,14 @@ fun GetProStatusResponse.toProStatus(nowMs: Long, context: Context, refundInProg providerData = providerData, quickRefundExpiry = paymentItem.platformRefundExpiry, refundInProgress = refundInProgress, - // In this ACTIVE branch we're covered; past the paid-through end (renewingAt) = grace. - inGracePeriod = nowMs >= expiryMs, + // Covered but past the paid-through end = the renewal is overdue and grace is + // running. Reachable now that renewingAt is `coverageEnd - grace` rather than + // `coverageEnd`, which no `now` in this branch could ever be at or past. + // + // Requires a fetch that COMPLETED at or after the renewal fell due โ€” see + // `confirmedAt`. Never constructed true on an unconfirmed snapshot. + inGracePeriod = nowMs >= renewingAtMs && + confirmedAt != null && !confirmedAt.isBefore(renewingAt), ) } else { ProStatus.Active.Expiring( diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt index 42f1e59abb..8de592010a 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt @@ -162,7 +162,9 @@ class ProStatusManager @Inject constructor( val refundInProgress = configFactory.get() .withUserConfigs { it.userProfile.getRefundRequested() != null } ProDataState( - type = proStatusState.lastUpdated?.first?.toProStatus(nowMs, application, refundInProgress) ?: ProStatus.NeverSubscribed, + type = proStatusState.lastUpdated?.let { (response, confirmedAt) -> + response.toProStatus(nowMs, application, refundInProgress, confirmedAt) + } ?: ProStatus.NeverSubscribed, showProBadge = showProBadgePreference, refreshState = proDataRefreshState ) @@ -403,12 +405,15 @@ class ProStatusManager @Inject constructor( return@flow } - val (accessExpiry, autoRenewing) = configFactory.get().withUserConfigs { configs -> - configs.userProfile.getProAccessExpiry()?.let(Instant::ofEpochSecond) to - configs.userProfile.getProAutoRenewing() + val (coverageEnd, autoRenewing, grace) = configFactory.get().withUserConfigs { configs -> + Triple( + configs.userProfile.getProAccessExpiry()?.let(Instant::ofEpochSecond), + configs.userProfile.getProAutoRenewing(), + configs.userProfile.getProGracePeriod(), + ) } - val reason = startupFetchReason(accessExpiry, autoRenewing, now) + val reason = startupFetchReason(coverageEnd, autoRenewing, grace, now) if (reason == null) { Log.d(DebugLogGroup.PRO_SUBSCRIPTION.label, "Startup gate: no CTA could fire, skipping the startup fetch") return@flow @@ -641,58 +646,56 @@ class ProStatusManager @Inject constructor( * Whether a cold start should fetch `get_pro_status`, and why โ€” or null to stay off the * network. Pure over plain values so it can be tested without a clock, a database or config. * - * The architect's four rows, which REPLACE the spec's `E + grace โ‰ค now` expired test (grace - * is not in config, so that test is unimplementable): + * [coverageEnd] is the access expiry as the backend sends it, which is **grace-inclusive**; + * the renewal falls due at `coverageEnd - grace`. Subtracting is unconditional and needs no + * provider branching, because the wire sends grace = 0 whenever the subscription is not + * auto-renewing. * - * | config state | action | - * |---------------------------------------------|-----------------------------------------| - * | `auto_renewing && now < E` | no fetch โ€” comfortably active | - * | `auto_renewing && now โ‰ฅ E` | fetch; grace is unknowable from config | - * | `!auto_renewing && E` within the CTA window | fetch โ€” the Expiring CTA may fire | - * | `!auto_renewing && now โ‰ฅ E` | confirm-fetch before the Expired CTA | + * The four rows, which replace the spec's `E + grace <= now` expired test (that test both + * double-counted grace and was unimplementable when written, because grace was not in config): * - * โš ๏ธ **Row 1 is known to be wrong, and is built this way deliberately โ€” see F8.** `E` is - * grace-INCLUSIVE (the backend folds grace in before sending it), so the real grace window - * `E โˆ’ grace โ‰ค now < E` lies entirely inside `now < E` โ€” the row that declines to fetch. The - * state this redesign exists to surface is therefore the one the gate is currently blind to. - * Desktop and iOS ship the identical row; correcting it is a three-client change and all - * three PRs document it. Do not fix it here alone. + * | config state | action | + * |----------------------------------------------------|-----------------------------------| + * | `auto_renewing && now < renewalDue` | no fetch โ€” comfortably active | + * | `auto_renewing && now >= renewalDue` | fetch โ€” the renewal is overdue | + * | `!auto_renewing && renewalDue` in the CTA window | fetch โ€” the Expiring CTA may fire | + * | `!auto_renewing && now >= renewalDue` | confirm before the Expired CTA | * - * โš ๏ธ **The `!auto_renewing && now < E && outside the CTA window` case is HELD โ€” see F2.** It - * returns null (the spec's letter: comfortably-active users never fetch on startup), but that - * is not a settled answer. `A` is presence-only, so `false` also means "never written" โ€” and - * every existing subscriber lands here on their first run after this ships, where declining - * to fetch means `A` is never written and the gate never changes its mind. Whether that needs - * a bootstrap fetch is Morgan's, and it is one branch of this `when`. + * Row 1 keying off `renewalDue` rather than `coverageEnd` is the point: the grace window is + * `renewalDue <= now < coverageEnd`, so keying off coverage end would have declined to fetch + * for exactly the state this exists to surface. */ internal fun startupFetchReason( - accessExpiry: Instant?, + coverageEnd: Instant?, autoRenewing: Boolean, + grace: Duration, now: Instant, ): String? { // No access expiry at all: never subscribed, so no CTA can fire and nothing to confirm. - if (accessExpiry == null) return null + if (coverageEnd == null) return null - val pastExpiry = !now.isBefore(accessExpiry) + val renewalDue = coverageEnd.minus(grace) + val overdue = !now.isBefore(renewalDue) return when { - autoRenewing && !pastExpiry -> null + autoRenewing && !overdue -> null - autoRenewing -> "auto-renewing and past the access expiry; grace is not knowable from config" + autoRenewing -> "auto-renewing and past the renewal date; grace is running" - !pastExpiry -> - if (accessExpiry.isBefore(now.plus(EXPIRING_CTA_WINDOW))) { + !overdue -> + if (renewalDue.isBefore(now.plus(EXPIRING_CTA_WINDOW))) { "not auto-renewing and expiring within $EXPIRING_CTA_WINDOW" } else { - // HELD (F2) โ€” the unnamed row. + // Comfortably active and not renewing โ€” a prepaid or long non-renewing + // subscription. No CTA can fire, so nothing to fetch for. null } - // Past expiry and not auto-renewing. Confirm with the backend before showing the - // Expired CTA: config can read expired while a renewal landed on another device and - // hasn't synced. Bounded by the CTA's own window โ€” once the CTA can no longer fire - // there is nothing for the fetch to serve. - accessExpiry.plus(EXPIRED_CTA_WINDOW).isAfter(now) -> + // Past the renewal date and not auto-renewing. Confirm with the backend before + // showing the Expired CTA: config can read expired while a renewal landed on another + // device and hasn't synced. Bounded by the CTA's own window โ€” once the CTA can no + // longer fire there is nothing for the fetch to serve. + renewalDue.plus(EXPIRED_CTA_WINDOW).isAfter(now) -> "not auto-renewing and expired within $EXPIRED_CTA_WINDOW; confirming before the Expired CTA" else -> null diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusRepository.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusRepository.kt index 48409c3412..73a8f86682 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusRepository.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusRepository.kt @@ -185,9 +185,10 @@ class ProStatusRepository @Inject constructor( // The scope is GlobalScope (Dispatchers.Default), so the read is off the main thread; the // callers are all fire-and-forget. scope.launch { - // The ATTEMPT timestamp, not the success one (F10, pending Morgan): a failed fetch costs - // the backend the same as a successful one, and gating on success meant a failing - // network re-attempted on every trigger and every cold launch. + // The ATTEMPT timestamp, not the success one: a failed request that reached the server + // costs it the same as a successful one, and gating on success made a failing network + // re-attempt on every trigger and every cold launch โ€” hardest exactly when the server is + // least able to take it. val lastFetchedAt = db.getProStatusLastAttemptAt() if (!shouldFetch( immediate = false, diff --git a/app/src/test/java/org/thoughtcrime/securesms/pro/ProStartupGateTest.kt b/app/src/test/java/org/thoughtcrime/securesms/pro/ProStartupGateTest.kt index 805eb2d519..426b129235 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/pro/ProStartupGateTest.kt +++ b/app/src/test/java/org/thoughtcrime/securesms/pro/ProStartupGateTest.kt @@ -8,20 +8,24 @@ import java.time.Duration import java.time.Instant /** - * The startup gate's decision (spec ยง2 trigger #1 / ยง4), as a pure function of - * (access expiry, auto-renewing, now). + * The startup gate's decision, as a pure function of + * (coverage end, auto-renewing, grace, now). * - * Scope: these cover the decision only. They do NOT cover the 24h persisted interval or the config - * read โ€” both need a database, and the interval is checked before this function is reached. + * Scope: the decision only. These do NOT cover the persisted 24h interval or the config read โ€” both + * need a database, and the interval is checked before this function is reached. * - * **Two of these tests pin behaviour we believe is WRONG**, deliberately: the F8 row-1 blind spot and - * the F2 held row. They are written so that the day either ruling lands, the test fails and points at - * the decision rather than silently accepting a change. Read their comments before "fixing" them. + * The distinction every case below turns on: **the access expiry the backend sends is grace-inclusive**, + * so it is coverage END, and the renewal falls due a grace period earlier. Anything keyed to coverage + * end rather than to `coverageEnd - grace` is blind to the entire grace window. */ class ProStartupGateTest { private val now: Instant = Instant.parse("2026-08-07T00:00:00Z") + /** A realistic Google Play base-plan grace period. Apple's is ~1h; Google's is days. */ + private val grace: Duration = Duration.ofDays(14) + private val noGrace: Duration = Duration.ZERO + private fun inDays(days: Long): Instant = now.plus(Duration.ofDays(days)) private fun daysAgo(days: Long): Instant = now.minus(Duration.ofDays(days)) @@ -31,77 +35,88 @@ class ProStartupGateTest { fun `no access expiry means no fetch`() { // The population the gate exists for: users who have never subscribed were fetching on every // cold start and could never see a CTA. - assertNull(startupFetchReason(accessExpiry = null, autoRenewing = false, now = now)) - assertNull(startupFetchReason(accessExpiry = null, autoRenewing = true, now = now)) + assertNull(startupFetchReason(null, autoRenewing = false, grace = noGrace, now = now)) + assertNull(startupFetchReason(null, autoRenewing = true, grace = grace, now = now)) } - // --- rows 1 and 2: auto-renewing ------------------------------------------------------------ + // --- auto-renewing -------------------------------------------------------------------------- @Test - fun `row 1 - auto-renewing and comfortably active does not fetch`() { - assertNull(startupFetchReason(inDays(20), autoRenewing = true, now = now)) + fun `auto-renewing and comfortably active does not fetch`() { + // Renewal due in 6 days (coverage end 20 days out, less 14 days of grace). + assertNull(startupFetchReason(inDays(20), autoRenewing = true, grace = grace, now = now)) } @Test - fun `row 2 - auto-renewing and past the access expiry fetches`() { - // Grace is not derivable from config, so the only way to learn where we stand is to ask. - assertNotNull(startupFetchReason(daysAgo(1), autoRenewing = true, now = now)) + fun `auto-renewing and inside the grace window DOES fetch`() { + // Coverage end is 7 days away, so `now` is comfortably before it โ€” but the renewal fell due 7 + // days ago and grace is running. This is the state the whole rework exists to surface, and a + // gate keyed to coverage end would sleep straight through it. + assertNotNull(startupFetchReason(inDays(7), autoRenewing = true, grace = grace, now = now)) } @Test - fun `row 2 boundary - exactly at the access expiry fetches`() { - assertNotNull(startupFetchReason(now, autoRenewing = true, now = now)) + fun `auto-renewing boundary - exactly at the renewal date fetches`() { + assertNotNull(startupFetchReason(now.plus(grace), autoRenewing = true, grace = grace, now = now)) } - // --- rows 3 and 4: not auto-renewing -------------------------------------------------------- + @Test + fun `auto-renewing boundary - one second before the renewal date does not fetch`() { + // The negative control for the pair above. Without it, "inside grace fetches" also passes + // against a gate that fetches unconditionally whenever auto-renewing. + val justBefore = now.plus(grace).plusSeconds(1) + assertNull(startupFetchReason(justBefore, autoRenewing = true, grace = grace, now = now)) + } @Test - fun `row 3 - expiring inside the CTA window fetches`() { - assertNotNull(startupFetchReason(inDays(3), autoRenewing = false, now = now)) + fun `auto-renewing past coverage end still fetches`() { + assertNotNull(startupFetchReason(daysAgo(1), autoRenewing = true, grace = grace, now = now)) } + // --- not auto-renewing ---------------------------------------------------------------------- + @Test - fun `row 3 boundary - just outside the 7 day window does not fetch`() { - assertNull(startupFetchReason(inDays(8), autoRenewing = false, now = now)) + fun `not auto-renewing and expiring inside the CTA window fetches`() { + assertNotNull(startupFetchReason(inDays(3), autoRenewing = false, grace = noGrace, now = now)) } @Test - fun `row 4 - recently expired fetches to confirm before the Expired CTA`() { + fun `not auto-renewing boundary - just outside the 7 day window does not fetch`() { + assertNull(startupFetchReason(inDays(8), autoRenewing = false, grace = noGrace, now = now)) + } + + @Test + fun `not auto-renewing and recently expired fetches to confirm before the Expired CTA`() { // Config can read expired while a renewal landed on another device and hasn't synced, so the // Expired CTA must never fire off config alone. - assertNotNull(startupFetchReason(daysAgo(5), autoRenewing = false, now = now)) + assertNotNull(startupFetchReason(daysAgo(5), autoRenewing = false, grace = noGrace, now = now)) } @Test - fun `row 4 boundary - expired longer ago than the CTA window does not fetch`() { + fun `not auto-renewing boundary - expired longer ago than the CTA window does not fetch`() { // Past 30 days the Expired CTA can no longer fire, so a confirming fetch has no consumer. - assertNull(startupFetchReason(daysAgo(31), autoRenewing = false, now = now)) + assertNull(startupFetchReason(daysAgo(31), autoRenewing = false, grace = noGrace, now = now)) } - // --- the two seams: pinned as-built, both believed wrong ------------------------------------- - @Test - fun `F8 SEAM - row 1 does not fetch during the real grace window, and that is the known flaw`() { - // `E` is grace-INCLUSIVE, so the real grace window (E - grace <= now < E) lies entirely - // inside row 1's `now < E`. An auto-renewing user whose renewal is overdue but who is still - // covered is exactly the state this redesign exists to surface โ€” and the gate declines to - // fetch for them. - // - // Pinned so the blind spot is visible rather than incidental. When F8 is ruled on and the - // boundary moves to `E - grace`, this assertion SHOULD fail โ€” at which point the fix is to - // change the gate and invert this test, not to delete it. - val graceWindow = startupFetchReason(inDays(1), autoRenewing = true, now = now) - assertNull(graceWindow) + fun `not auto-renewing, active, outside the CTA window does not fetch`() { + // A prepaid or long non-renewing subscription. No CTA can fire, so there is nothing to fetch + // for. Note this is now unambiguous: the proof-success path writes the renewing flag beside + // the expiry it sets, so an absent flag genuinely means not-renewing rather than "never + // recorded". + assertNull(startupFetchReason(inDays(60), autoRenewing = false, grace = noGrace, now = now)) } + // --- the subtraction is unconditional ------------------------------------------------------- + @Test - fun `F2 SEAM - not auto-renewing, active, outside the CTA window does not fetch`() { - // The held row. Returns null per the spec's letter ("comfortably-active users never fetch on - // startup"), but `A` is presence-only, so `autoRenewing = false` here also covers "never - // written" โ€” which is where every existing subscriber lands on their first run after this - // ships. Declining means `A` is never written and the gate never revises its answer. - // - // If F2 rules for bootstrap-on-unknown this becomes a fetch and the test must change with it. - assertNull(startupFetchReason(inDays(60), autoRenewing = false, now = now)) + fun `zero grace makes coverage end and the renewal date the same instant`() { + // The wire sends grace = 0 whenever the subscription is not auto-renewing, so subtracting + // needs no provider branching and no null handling โ€” it is a no-op for those accounts. Pinned + // because a future reader may be tempted to guard the subtraction. + val expiring = startupFetchReason(inDays(3), autoRenewing = false, grace = noGrace, now = now) + val alsoExpiring = startupFetchReason(inDays(3), autoRenewing = false, grace = Duration.ZERO, now = now) + assertNotNull(expiring) + assertNotNull(alsoExpiring) } } From fa763674dd84c5353e6fdc77679a24caf703fd3f Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Mon, 10 Aug 2026 12:40:17 +1000 Subject: [PATCH 06/28] Pro: record the blocked proof-path writes for auto-renewing and grace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The proof response refreshes the cached access expiry, and the two fields that must travel with it cannot be written yet. The intended lines are in the source, commented, with the blocker named at the site. Both incoherences are live today rather than introduced here: - auto-renewing goes stale: an account whose expiry only came from a proof reads back as not-renewing while it is renewing, because the key is presence-only and nothing on this path sets it. - grace pairs with the wrong expiry: paid-through is derived as E - G everywhere, so a fresh E beside a G from an older response is wrong by the difference. Worst where it matters, since Google reports its real multi-day grace only once the subscriber enters grace โ€” a proof landing first pairs a new coverage end with the old stand-in. The hole is in the middle of the chain: the backend sends both account_auto_renewing and account_grace_period_duration, but parse_pro_proof reads only account_expiry_ts, so neither reaches the response type or the wrapper. A core commit lands first. No client-side workaround: reading the raw response behind libsession, or sourcing the flag from anywhere other than this outcome, are both worse than the staleness. --- .../securesms/pro/ProProofGenerationWorker.kt | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt index 6c1a58a74d..104143c7ed 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt @@ -154,6 +154,34 @@ class ProProofGenerationWorker @AssistedInject constructor( // Refresh the cached access-expiry from the advisory account_expiry that rides the // proof response, so the renewal path keeps E fresh without a separate get_pro_status. response.accountExpiry?.let { configs.userProfile.setProAccessExpiry(it.epochSecond) } + + // โš ๏ธ BLOCKED โ€” the two lines below are the intended write and cannot compile yet. + // + // configs.userProfile.setProAutoRenewing(response.accountAutoRenewing) + // configs.userProfile.setProGracePeriod(response.accountGracePeriod) + // + // Writing the access expiry here WITHOUT them leaves two incoherences, both of + // which are live today: + // + // * auto-renewing goes stale. An account whose expiry only ever came from a + // proof reads back as not-renewing while it is in fact renewing, because + // the key is presence-only and nothing on this path sets it. + // * the grace period pairs with the wrong expiry. Everything derives the + // paid-through instant as `E - G`; writing a fresh E beside a G from an + // older response makes that subtraction wrong by the difference between + // them. Worst at the transition that matters: Google reports its real, + // multi-day grace only once the subscriber ENTERS grace, so a proof landing + // first pairs a new coverage end with the old ~1h stand-in. + // + // The backend sends both fields (`account_auto_renewing`, + // `account_grace_period_duration`). The hole is in the middle: libsession's + // `parse_pro_proof` reads only `account_expiry_ts` + // (`src/pro_backend.cpp`), so neither field reaches + // `GenerateProProofResponse` and neither is exposed by the glue. Uncomment + // once core parses them and the wrapper surfaces them; no client-side + // workaround is appropriate โ€” reading the raw response behind libsession, or + // sourcing the flag from anywhere other than this outcome, would both be worse + // than the staleness. } Log.d(WORK_NAME, "Successfully generated a new pro proof expiring at ${Instant.ofEpochSecond(proof.expirySeconds)}") From 911a0305a6ac0128f5b9d3c9aae972df0a8412a0 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Mon, 10 Aug 2026 12:45:29 +1000 Subject: [PATCH 07/28] Pro: keep the renewing flag and grace coherent when a proof refreshes the access expiry A proof response refreshes the cached access expiry, and the two fields that must travel with it are now written alongside: the renewing flag, and the grace period the expiry has folded into it. Without them an account whose expiry only ever came from a proof read back as terminal while it was renewing, and `E - G` -- which everything now uses for the paid-through instant -- was wrong by the difference between two responses. Worst where it matters: Google reports its real multi-day grace only once the subscriber enters grace, so a proof landing first paired a new coverage end with the ~1h stand-in. Written ONLY when the backend actually sent them. Absent means "did not say", not false or zero, and collapsing that is destructive rather than merely lossy: both config keys are presence-only, so writing false or zero erases them, and a backend predating these fields sends neither. A `?: false` in this path would make every proof fetch wipe a value correctly learned from get_pro_status. Needs the wrapper's matching commit, which re-pins libsession to the parse. --- .../securesms/pro/ProProofGenerationWorker.kt | 40 +++++++------------ 1 file changed, 14 insertions(+), 26 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt index 104143c7ed..1e1324fe5f 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt @@ -155,33 +155,21 @@ class ProProofGenerationWorker @AssistedInject constructor( // proof response, so the renewal path keeps E fresh without a separate get_pro_status. response.accountExpiry?.let { configs.userProfile.setProAccessExpiry(it.epochSecond) } - // โš ๏ธ BLOCKED โ€” the two lines below are the intended write and cannot compile yet. + // Keep the renewing flag and the grace period coherent with the expiry above. + // Both must travel with it: everything derives the paid-through instant as + // `E - G`, so a fresh E beside a G from an older response is wrong by the + // difference between them โ€” worst exactly where it matters, since Google + // reports its real multi-day grace only once the subscriber ENTERS grace, so a + // proof landing first would pair a new coverage end with the ~1h stand-in. // - // configs.userProfile.setProAutoRenewing(response.accountAutoRenewing) - // configs.userProfile.setProGracePeriod(response.accountGracePeriod) - // - // Writing the access expiry here WITHOUT them leaves two incoherences, both of - // which are live today: - // - // * auto-renewing goes stale. An account whose expiry only ever came from a - // proof reads back as not-renewing while it is in fact renewing, because - // the key is presence-only and nothing on this path sets it. - // * the grace period pairs with the wrong expiry. Everything derives the - // paid-through instant as `E - G`; writing a fresh E beside a G from an - // older response makes that subtraction wrong by the difference between - // them. Worst at the transition that matters: Google reports its real, - // multi-day grace only once the subscriber ENTERS grace, so a proof landing - // first pairs a new coverage end with the old ~1h stand-in. - // - // The backend sends both fields (`account_auto_renewing`, - // `account_grace_period_duration`). The hole is in the middle: libsession's - // `parse_pro_proof` reads only `account_expiry_ts` - // (`src/pro_backend.cpp`), so neither field reaches - // `GenerateProProofResponse` and neither is exposed by the glue. Uncomment - // once core parses them and the wrapper surfaces them; no client-side - // workaround is appropriate โ€” reading the raw response behind libsession, or - // sourcing the flag from anywhere other than this outcome, would both be worse - // than the staleness. + // โš ๏ธ Written ONLY when the backend actually said. `null` means "did not say", + // NOT false or zero, and the distinction is destructive to lose: both config + // keys are presence-only, so writing false/zero ERASES them. A backend that + // predates these fields sends neither, and collapsing absent โ€” a `?: false` + // anywhere in this path โ€” would make every proof fetch wipe a value correctly + // learned from get_pro_status. That is worse than the staleness it would fix. + response.accountAutoRenewing?.let { configs.userProfile.setProAutoRenewing(it) } + response.accountGracePeriod?.let { configs.userProfile.setProGracePeriod(it) } } Log.d(WORK_NAME, "Successfully generated a new pro proof expiring at ${Instant.ofEpochSecond(proof.expirySeconds)}") From b0e6067549989c8859edbf53fa50d2fdfc794ae2 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Mon, 10 Aug 2026 12:57:25 +1000 Subject: [PATCH 08/28] Pro: wake at the renewal date, not at the end of coverage The single user_expiry wake fired 30s after the access expiry the backend sends. That value is grace-inclusive, so it is the instant coverage ENDS -- the renewal became overdue a whole grace period earlier, and the wake fired after the window it exists to catch had already closed. It was the last trigger still keyed to the pre-correction reading of that field. Now wakes 30s after the renewal falls due. Subtracting grace is a no-op for a non-auto-renewing account, where the wire sends zero. Note what this does NOT add: nothing wakes at the end of coverage once grace has been entered. A failed renewal is learned at the renewal date, and after that the account is covered by the startup gate -- which now fetches for exactly that state -- plus on-enter and the while-open poll. Whether a second wake at coverage end is wanted is an open cross-client question. --- .../securesms/pro/ProStatusManager.kt | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt index 8de592010a..802c578ce7 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt @@ -354,12 +354,19 @@ class ProStatusManager @Inject constructor( .map { "ProAccessExpiry/prepaid in config changes" }, proStatusRepository.get().loadState - .mapNotNull { it.lastUpdated?.first?.expiry } + .mapNotNull { state -> + // The instant the renewal falls DUE, not when coverage ends. The expiry the + // backend sends is grace-inclusive, so waking at it would fire after the window + // this trigger exists to catch has already closed โ€” the renewal became overdue a + // whole grace period earlier. Subtracting is a no-op for a non-auto-renewing + // account, where the wire sends grace = 0. + state.lastUpdated?.first?.let { it.expiry?.minus(it.gracePeriod) } + } .distinctUntilChanged() - .transformLatest { expiry -> - // Schedule a refresh for 30 seconds after access expiry - if (snodeClock.delayUntil(expiry.plusSeconds(30))) { - emit("30 seconds after Access expiry reached") + .transformLatest { renewalDue -> + // Schedule a refresh for 30 seconds after the renewal fell due. + if (snodeClock.delayUntil(renewalDue.plusSeconds(30))) { + emit("30 seconds after the renewal fell due") } }, From 8a393122d670bf46ba3aed2476f91c8cb3cb851f Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Mon, 10 Aug 2026 13:04:18 +1000 Subject: [PATCH 09/28] Pro: add a second wake at coverage end, and correct the grace severity comment Trigger #6 becomes two instants: 30s after the renewal falls due, and 30s after coverage ends. The second is guarded on the two coinciding, which is the case for every non-auto-renewing account, where grace is zero. It looks redundant with the proof loop and isn't. The backend issues a proof good until roughly an hour past coverage end and the renewal target sits ~1h before that, so a proof attempt lands near coverage end and its config write fires the config-change trigger -- but only when E actually MOVES. A succeeded renewal advances E and is covered; a FAILED one leaves E unchanged and nothing fires. The uncovered branch is the one the grace warning exists for. No wake handles to leak: transformLatest cancels the whole body when E moves, so a wake armed against a superseded expiry cannot outlive it. A scheduler holding ids would need a collection for two instants -- holding one and scheduling two orphans a timer every period, while both wakes still fire correctly. Also corrects the severity claim in the grace comment. The multi-day operator-configured value is written only on Google's IN_GRACE_PERIOD notification and reset to the ~1h stand-in on RECOVERED/RENEWED, so it exists only during the grace window. A healthy auto-renewing subscriber has grace of about an hour on both stores, so the renewal date was an hour late, not days. The unreachable grace state was the real defect, and the gate's blindness to the window is the consequence that genuinely spans days. --- .../securesms/pro/ProDataMapper.kt | 17 ++++--- .../securesms/pro/ProStatusManager.kt | 44 +++++++++++++++---- 2 files changed, 47 insertions(+), 14 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProDataMapper.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProDataMapper.kt index 8dff6feef3..86bc5598c7 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProDataMapper.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProDataMapper.kt @@ -58,12 +58,17 @@ fun GetProStatusResponse.toProStatus( // auto-renewing (`server.py:335`), so `expiry - 0 == expiry` for those accounts. // // This was wrong in the opposite direction until 2026-08-10 โ€” it treated `expiry` as - // paid-through and never subtracted, which made `inGracePeriod` unreachable (it sits in a - // branch requiring `now <= expiry`) and rendered the renewal date a whole grace period - // late. Not cosmetic where it mattered: Apple's grace is the backend's own ~1h stand-in, - // but Google's is the operator-configured base-plan value in DAYS, fetched exactly when - // the subscriber enters grace โ€” so the date was days wrong on the screen whose purpose is - // that date, and most wrong precisely when someone was looking at it. + // paid-through and never subtracted, so `inGracePeriod` was unreachable (it sits in a + // branch requiring `now <= expiry`) and the renewal date rendered a grace period late. + // + // Magnitude, stated carefully because it is easy to get wrong in both directions: a + // healthy auto-renewing subscriber has grace โ‰ˆ 1 HOUR on both stores. Google's + // operator-configured multi-day value is written only on the IN_GRACE_PERIOD notification + // and reset to the 1h stand-in on RECOVERED/RENEWED, so its lifetime is exactly the grace + // window. The renewal date a subscriber actually reads was therefore an hour late โ€” + // invisible at day granularity โ€” and the real defect was the unreachable grace state, not + // the date. (The startup gate's blindness WAS multi-day, because during grace the value + // is the real one; that is a different consequence of the same bug.) val coverageEnd = expiry ?: return ProStatus.NeverSubscribed // The paid-through end โ€” when the renewal actually falls due. val renewingAt = coverageEnd.minus(gracePeriod) diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt index 802c578ce7..c0ab81c13c 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt @@ -355,19 +355,47 @@ class ProStatusManager @Inject constructor( proStatusRepository.get().loadState .mapNotNull { state -> - // The instant the renewal falls DUE, not when coverage ends. The expiry the - // backend sends is grace-inclusive, so waking at it would fire after the window - // this trigger exists to catch has already closed โ€” the renewal became overdue a - // whole grace period earlier. Subtracting is a no-op for a non-auto-renewing - // account, where the wire sends grace = 0. - state.lastUpdated?.first?.let { it.expiry?.minus(it.gracePeriod) } + state.lastUpdated?.first?.let { status -> + status.expiry?.let { coverageEnd -> + // The renewal falls due a grace period BEFORE coverage ends, because the + // expiry the backend sends is grace-inclusive. Waking only at coverage end + // would fire after the window this trigger exists to catch had closed. + coverageEnd.minus(status.gracePeriod) to coverageEnd + } + } } .distinctUntilChanged() - .transformLatest { renewalDue -> - // Schedule a refresh for 30 seconds after the renewal fell due. + .transformLatest { (renewalDue, coverageEnd) -> + // TWO wakes, and the second is not redundant with the first. + // + // 1. renewal due โ€” did the charge succeed or fail? + // 2. coverage end โ€” did grace run out without recovery? + // + // (2) looks covered by the proof loop, and isn't. The backend issues a proof + // good until roughly an hour past coverage end and the renewal target sits ~1h + // before that, so a proof attempt lands near coverage end and its config write + // fires the config-change trigger. But that chain needs `E` to MOVE: + // + // renewal succeeded -> E advances -> config change -> refresh โœ… + // renewal failed -> E unchanged -> no change -> nothing โŒ + // + // The uncovered branch is the one the grace warning exists for. + // + // No wake handles to leak here: `transformLatest` cancels this whole body when + // `E` moves, so a second wake armed against a superseded expiry cannot outlive + // it. A scheduler holding ids would need a COLLECTION for two instants โ€” holding + // one and scheduling two orphans a timer every period, and both wakes still fire + // correctly while it accumulates. if (snodeClock.delayUntil(renewalDue.plusSeconds(30))) { emit("30 seconds after the renewal fell due") } + + // Guarded on the two instants COINCIDING rather than on grace being zero: same + // condition today, but it says what actually matters, so it survives a change to + // how the instants are derived. + if (coverageEnd != renewalDue && snodeClock.delayUntil(coverageEnd.plusSeconds(30))) { + emit("30 seconds after coverage ended") + } }, // NOTE: there is deliberately no trigger keyed to PROOF expiry here. Proof timing From 7e7d1bf8798ca00db061cb5796a2395c69aee201 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Mon, 10 Aug 2026 13:36:15 +1000 Subject: [PATCH 10/28] Pro: document why the coverage-end wake doesn't fetch on a compressed test backend Both user_expiry wake emits reach the network through the floored path, so when the grace period is shorter than the 60s freshness floor the second wake lands inside the floor the first one just armed and its fetch is dropped. The wake fires; the fetch does not, which looks exactly like the wake was never scheduled. Only reachable on a compressed testing backend -- google_play/mule.py overrides grace with testing_grace_period_duration_ms = 10s. Production grace is either the backend's ~1h stand-in or an operator-configured value in days, both far outside the floor. Documented in two places because the two readers arrive from opposite directions and neither has reason to open the other's file: at the floor constant, for someone tuning or removing it, and at the wake, for someone debugging why it "didn't fire". Left alone deliberately. If Pro UI-test work needs to exercise the coverage-end wake, the sanctioned escape hatch is an env-var override of the floor, owned by that work. Not an immediate fetch for a scheduled trigger -- that is what force -> immediate was introduced to stop. --- .../securesms/pro/ProStatusManager.kt | 16 ++++++++++++++++ .../securesms/pro/ProStatusRepository.kt | 19 ++++++++++++++++++- 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt index c0ab81c13c..5fbb167eca 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt @@ -393,6 +393,22 @@ class ProStatusManager @Inject constructor( // Guarded on the two instants COINCIDING rather than on grace being zero: same // condition today, but it says what actually matters, so it survives a change to // how the instants are derived. + // + // โš ๏ธ If you are here because this wake "didn't fire" on a QA backend: it fired. + // The FETCH was dropped. Both emits go through the floored path + // (`requestRefresh()`, not `immediate`), so when grace is shorter than + // `MIN_UPDATE_INTERVAL_SECONDS` = 60s the second wake lands inside the floor that + // the first one just armed. The symptom is indistinguishable from the wake never + // having been scheduled, which is why this comment exists. + // + // Only reachable on a compressed testing backend โ€” `google_play/mule.py` + // overrides grace with `testing_grace_period_duration_ms` = 10s. Production grace + // is the ~1h stand-in or an operator value in days, both far outside the floor. + // + // Left alone deliberately (ruled 2026-08-10): the escape hatch is an env-var + // override of the floor, owned by the Pro UI-test work. Do not make this wake + // `immediate` โ€” a scheduled trigger bypassing the floor is what `force` -> + // `immediate` was introduced to stop. if (coverageEnd != renewalDue && snodeClock.delayUntil(coverageEnd.plusSeconds(30))) { emit("30 seconds after coverage ended") } diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusRepository.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusRepository.kt index 73a8f86682..beaeb91b9e 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusRepository.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusRepository.kt @@ -211,7 +211,24 @@ class ProStatusRepository @Inject constructor( companion object { /** * The status freshness floor. **Shared cross-client contract** โ€” Desktop and iOS use the - * same 60s; keep them in step, and say why in the commit if they ever have to diverge. + * same 60s (`SessionPro.StatusRefresh.floorSeconds`, `STATUS_FLOOR_MS`); keep them in step, + * and say why in the commit if they ever have to diverge. + * + * โš ๏ธ **A scheduled wake depends on this not being crossed.** `ProStatusManager`'s + * `user_expiry` trigger arms two wakes โ€” at the renewal date and at coverage end โ€” and both + * reach the network through the FLOORED path here, not through `immediate`. When the grace + * period is shorter than this floor the two wakes land inside it and **the second one's fetch + * is dropped.** + * + * In production that cannot happen: grace is either the backend's ~1h stand-in or an + * operator-configured value in days. It happens on a **compressed testing backend**, where + * `providers/google_play/mule.py` overrides grace with + * `api.testing_grace_period_duration_ms` = 10 seconds. + * + * Deliberately not worked around here (ruled 2026-08-10). If UI-test work needs to exercise + * the coverage-end wake, the sanctioned escape hatch is an **env-var override of this + * constant**, owned by that work โ€” not an `immediate` fetch for a scheduled trigger, which + * would reopen exactly what `force` -> `immediate` closed. */ const val MIN_UPDATE_INTERVAL_SECONDS = 60L From 4929f1f47eaec5aadd5d65830c33e0cf6c4288d5 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Mon, 10 Aug 2026 14:36:57 +1000 Subject: [PATCH 11/28] Pro: write the renewal flag and grace unconditionally on a proof success libsession now REQUIRES both fields on a successful proof rather than treating them as optional, so there is no "the backend did not say" state left to guard against and the presence checks come out. That is safe for a specific reason rather than by simplification: a missing or malformed field fails the parse, so we never reach this write. Writing false to a presence-only config key ERASES it, so a defaulted false would have been destructive rather than inert -- requiring the field is what makes an unconditional write correct here. Inside the success branch both values are truthful. Needs the wrapper's matching commit, which re-pins libsession to 799f1972. --- .../securesms/pro/ProProofGenerationWorker.kt | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt index 1e1324fe5f..3dc420460a 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt @@ -162,14 +162,14 @@ class ProProofGenerationWorker @AssistedInject constructor( // reports its real multi-day grace only once the subscriber ENTERS grace, so a // proof landing first would pair a new coverage end with the ~1h stand-in. // - // โš ๏ธ Written ONLY when the backend actually said. `null` means "did not say", - // NOT false or zero, and the distinction is destructive to lose: both config - // keys are presence-only, so writing false/zero ERASES them. A backend that - // predates these fields sends neither, and collapsing absent โ€” a `?: false` - // anywhere in this path โ€” would make every proof fetch wipe a value correctly - // learned from get_pro_status. That is worse than the staleness it would fix. - response.accountAutoRenewing?.let { configs.userProfile.setProAutoRenewing(it) } - response.accountGracePeriod?.let { configs.userProfile.setProGracePeriod(it) } + // Unconditional, and that is safe for a specific reason: libsession REQUIRES + // both fields on a successful proof, so there is no "the backend did not say" + // state to guard against. A missing or malformed field fails the parse and we + // never reach here, which is deliberate โ€” writing `false` to a presence-only + // config key ERASES it, so a defaulted false would be destructive rather than + // inert. We are inside the success branch, so both values are truthful. + configs.userProfile.setProAutoRenewing(response.accountAutoRenewing) + configs.userProfile.setProGracePeriod(response.accountGracePeriod) } Log.d(WORK_NAME, "Successfully generated a new pro proof expiring at ${Instant.ofEpochSecond(proof.expirySeconds)}") From a76aa9be299bb8095839e9fc08e330837416cf76 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Mon, 10 Aug 2026 14:41:29 +1000 Subject: [PATCH 12/28] Pro: name placement as what makes the proof-path config write safe The write is already inside the success branch and its behaviour is unchanged. The comment justifying it was wrong: it said a missing or malformed field fails the parse so the write is never reached. That is not what protects it. libsession's parse_pro_proof returns on the failure path BEFORE filling the renewal flag and the grace period, so on every non-OK outcome they hold struct defaults of false and zero. There is no presence flag on the C struct and the Kotlin type is non-nullable, so a read outside the success branch gets false and cannot tell it from a backend that genuinely said "not renewing" -- and writing false to a presence-only config key ERASES it. Erasing is truthful for subscription_expired, not_subscribed and revoked. For a protocol error, a stale request or a transport failure it would wipe a flag get_pro_status had correctly learned. So the comment now says the protection is PLACEMENT, not the parse and not the type, and not to hoist the two writes out of the success branch. Needs the wrapper's matching commit. --- .../securesms/pro/ProProofGenerationWorker.kt | 23 ++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt index 3dc420460a..e0e67be2fa 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt @@ -162,12 +162,23 @@ class ProProofGenerationWorker @AssistedInject constructor( // reports its real multi-day grace only once the subscriber ENTERS grace, so a // proof landing first would pair a new coverage end with the ~1h stand-in. // - // Unconditional, and that is safe for a specific reason: libsession REQUIRES - // both fields on a successful proof, so there is no "the backend did not say" - // state to guard against. A missing or malformed field fails the parse and we - // never reach here, which is deliberate โ€” writing `false` to a presence-only - // config key ERASES it, so a defaulted false would be destructive rather than - // inert. We are inside the success branch, so both values are truthful. + // โš ๏ธ These two are only meaningful HERE, inside the success branch, and nothing + // in the type system says so โ€” that is the hazard. + // + // libsession's `parse_pro_proof` returns on the failure path BEFORE it fills + // them, so on every non-OK outcome they hold struct defaults: grace 0, + // renewing false. The C struct carries no presence flag and the Kotlin type is + // non-nullable, so a read outside this branch gets `false` and cannot tell it + // from a backend that genuinely said "not renewing". + // + // That matters because writing `false` to a presence-only config key ERASES + // it. On `subscription_expired`/`not_subscribed`/`revoked` erasing is truthful; + // on a protocol error, a stale request or a transport failure it would wipe a + // flag `get_pro_status` had correctly learned, on the strength of a response + // that said nothing about the account. + // + // So the protection is PLACEMENT โ€” not the parse, and not the type. Do not + // hoist these two out of the success branch. configs.userProfile.setProAutoRenewing(response.accountAutoRenewing) configs.userProfile.setProGracePeriod(response.accountGracePeriod) } From 43479169cf334e88b6a3a941bd44834bd237250b Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Tue, 11 Aug 2026 10:05:20 +1000 Subject: [PATCH 13/28] Pro: read the access expiry as the payment date, not as coverage end MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The backend no longer folds grace into the stored expiry โ€” that fold was removed upstream, and `grace_period_duration` now reports how much longer we serve PAST the expiry sent. The backend states it directly: "expiry_ts + grace_period_duration is exactly when we stop serving". So every subtraction goes away and the display gets simpler: - the renewal date is `expiry`, rendered as sent - the grace window is `[expiry, expiry + grace)` - coverage ends at `expiry + grace` - trigger #6 wakes at `expiry`, then again at `expiry + grace` Grace now enters the startup gate in exactly one row, and earns its place there: the auto-renewing row is bounded from COVERAGE end rather than the payment date, so an account inside a multi-day grace is never mistaken for a long-dead one, and an account whose renewing flag was never cleared stops fetching on every cold start forever. The earlier reading was traced against a checkout 70 commits behind `origin/dev`, which is the whole lesson: the staleness trap already documented for the client repos was never applied to the backend. --- .../prosettings/ProSettingsViewModel.kt | 12 +-- .../securesms/pro/ProDataMapper.kt | 49 +++++------- .../securesms/pro/ProStatusManager.kt | 59 +++++++++------ .../securesms/pro/ProStartupGateTest.kt | 74 +++++++++++++------ 4 files changed, 113 insertions(+), 81 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/preferences/prosettings/ProSettingsViewModel.kt b/app/src/main/java/org/thoughtcrime/securesms/preferences/prosettings/ProSettingsViewModel.kt index 83c35f293a..791ff8e6df 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/preferences/prosettings/ProSettingsViewModel.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/preferences/prosettings/ProSettingsViewModel.kt @@ -761,14 +761,14 @@ class ProSettingsViewModel @AssistedInject constructor( /** * The instant the renewal falls due โ€” the account's paid-through end. * - * `expiry` is COVERAGE END, not paid-through: the backend folds grace into it before sending it, - * so the renewal was due a grace period earlier. Subtracting is unconditional โ€” the wire sends - * grace = 0 when the subscription is not auto-renewing, so this is a no-op for those accounts. + * `expiry` IS the payment-due date. Coverage runs a further `gracePeriod` past it โ€” the backend's + * contract is "`expiry_ts` + `grace_period_duration` is exactly when we stop serving" โ€” so do not + * subtract to get this instant. */ - private fun GetProStatusResponse.renewalDueAt(): Instant? = expiry?.minus(gracePeriod) + private fun GetProStatusResponse.renewalDueAt(): Instant? = expiry - /** The instant coverage really ends โ€” the wire value as sent. See [renewalDueAt]. */ - private fun GetProStatusResponse.coverageEndsAt(): Instant? = expiry + /** The instant coverage really ends. See [renewalDueAt]. */ + private fun GetProStatusResponse.coverageEndsAt(): Instant? = expiry?.plus(gracePeriod) /** * [immediate] bypasses the repository's freshness floor. Every caller here is a user-initiated diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProDataMapper.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProDataMapper.kt index 86bc5598c7..8508514050 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProDataMapper.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProDataMapper.kt @@ -45,33 +45,21 @@ fun GetProStatusResponse.toProStatus( return when (userStatus) { ProUserStatus.ACTIVE -> { val paymentItem = latestPayment ?: return ProStatus.NeverSubscribed - // `expiry` is COVERAGE END, not the paid-through date: the backend folds grace into it - // before sending it (`Session-Pro-Backend` `backend.py` `_lookup_user_expiry`: - // `payment_expiry_at = expiry_at + grace if auto_renewing else expiry_at` -> - // `users.expiry_at` -> the wire `expiry_ts`, `server.py:317`), and judges `active` against - // that same value (`:322`). Its own test subtracts grace from the wire value to recover - // the store's date (`tests/test_google.py:556-560`). + // `expiry` is the PAYMENT-DUE date โ€” the renewal is due then, and coverage runs a + // further `gracePeriod` past it. The backend states the contract directly: "`expiry_ts` + + // `grace_period_duration` is exactly when we stop serving", derived from the same instant + // the status is judged against (`server.py`, `account_coverage_end`). So being in this + // ACTIVE branch past `expiry` IS the grace period. // - // So the renewal is due at `expiry - gracePeriod`, and the grace window is - // `expiry - gracePeriod <= now < expiry`. Subtracting is unconditional and needs no - // provider branching: the wire sends grace = 0 whenever the subscription is not - // auto-renewing (`server.py:335`), so `expiry - 0 == expiry` for those accounts. - // - // This was wrong in the opposite direction until 2026-08-10 โ€” it treated `expiry` as - // paid-through and never subtracted, so `inGracePeriod` was unreachable (it sits in a - // branch requiring `now <= expiry`) and the renewal date rendered a grace period late. - // - // Magnitude, stated carefully because it is easy to get wrong in both directions: a - // healthy auto-renewing subscriber has grace โ‰ˆ 1 HOUR on both stores. Google's - // operator-configured multi-day value is written only on the IN_GRACE_PERIOD notification - // and reset to the 1h stand-in on RECOVERED/RENEWED, so its lifetime is exactly the grace - // window. The renewal date a subscriber actually reads was therefore an hour late โ€” - // invisible at day granularity โ€” and the real defect was the unreachable grace state, not - // the date. (The startup gate's blindness WAS multi-day, because during grace the value - // is the real one; that is a different consequence of the same bug.) - val coverageEnd = expiry ?: return ProStatus.NeverSubscribed - // The paid-through end โ€” when the renewal actually falls due. - val renewingAt = coverageEnd.minus(gracePeriod) + // Two traps here, both of which have caught someone: + // * Do NOT subtract grace to get the renewal date. `expiry` already IS that date. An + // earlier reading of the backend had grace folded into the stored expiry; that fold was + // removed and the field now reports how much longer we serve PAST the shown expiry. + // * `gracePeriod` on THIS type is the account-level field โ€” "how much longer we serve" โ€” + // and is not the same quantity as `ProPaymentItem.gracePeriod`, which reports what a + // store declared about one transaction. They share a name and answer different + // questions. + val renewingAt = expiry ?: return ProStatus.NeverSubscribed val renewingAtMs = renewingAt.toEpochMilli() val providerData = providerMetadata(paymentItem.paymentProvider, context) val duration = paymentItem.toProPlanPeriod() @@ -92,9 +80,8 @@ fun GetProStatusResponse.toProStatus( providerData = providerData, quickRefundExpiry = paymentItem.platformRefundExpiry, refundInProgress = refundInProgress, - // Covered but past the paid-through end = the renewal is overdue and grace is - // running. Reachable now that renewingAt is `coverageEnd - grace` rather than - // `coverageEnd`, which no `now` in this branch could ever be at or past. + // Covered but past the payment-due date = the renewal is overdue and grace is + // running. Reachable because this ACTIVE branch extends to `expiry + gracePeriod`. // // Requires a fetch that COMPLETED at or after the renewal fell due โ€” see // `confirmedAt`. Never constructed true on an unconfirmed snapshot. @@ -103,7 +90,9 @@ fun GetProStatusResponse.toProStatus( ) } else { ProStatus.Active.Expiring( - renewingAt = renewingAt, // the paid-through end (not auto-renewing โ†’ it just expires then) + // Not auto-renewing, so there is nothing to renew and grace is 0: this instant + // is both the payment-due date and the end of coverage. It just expires then. + renewingAt = renewingAt, duration = duration, providerData = providerData, quickRefundExpiry = paymentItem.platformRefundExpiry, diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt index 5fbb167eca..2ea7e3528a 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt @@ -356,11 +356,11 @@ class ProStatusManager @Inject constructor( proStatusRepository.get().loadState .mapNotNull { state -> state.lastUpdated?.first?.let { status -> - status.expiry?.let { coverageEnd -> - // The renewal falls due a grace period BEFORE coverage ends, because the - // expiry the backend sends is grace-inclusive. Waking only at coverage end - // would fire after the window this trigger exists to catch had closed. - coverageEnd.minus(status.gracePeriod) to coverageEnd + status.expiry?.let { renewalDue -> + // `expiry` is the payment-due date; coverage runs a further grace period + // past it. So the renewal falls due at `expiry`, and grace ends at + // `expiry + gracePeriod`. + renewalDue to renewalDue.plus(status.gracePeriod) } } } @@ -371,10 +371,9 @@ class ProStatusManager @Inject constructor( // 1. renewal due โ€” did the charge succeed or fail? // 2. coverage end โ€” did grace run out without recovery? // - // (2) looks covered by the proof loop, and isn't. The backend issues a proof - // good until roughly an hour past coverage end and the renewal target sits ~1h - // before that, so a proof attempt lands near coverage end and its config write - // fires the config-change trigger. But that chain needs `E` to MOVE: + // (2) looks covered by the proof loop, and isn't. A proof attempt near the end + // of coverage writes config, and that write fires the config-change trigger. But + // that chain needs `E` to MOVE: // // renewal succeeded -> E advances -> config change -> refresh โœ… // renewal failed -> E unchanged -> no change -> nothing โŒ @@ -456,7 +455,7 @@ class ProStatusManager @Inject constructor( return@flow } - val (coverageEnd, autoRenewing, grace) = configFactory.get().withUserConfigs { configs -> + val (renewalDue, autoRenewing, grace) = configFactory.get().withUserConfigs { configs -> Triple( configs.userProfile.getProAccessExpiry()?.let(Instant::ofEpochSecond), configs.userProfile.getProAutoRenewing(), @@ -464,7 +463,7 @@ class ProStatusManager @Inject constructor( ) } - val reason = startupFetchReason(coverageEnd, autoRenewing, grace, now) + val reason = startupFetchReason(renewalDue, autoRenewing, grace, now) if (reason == null) { Log.d(DebugLogGroup.PRO_SUBSCRIPTION.label, "Startup gate: no CTA could fire, skipping the startup fetch") return@flow @@ -697,10 +696,10 @@ class ProStatusManager @Inject constructor( * Whether a cold start should fetch `get_pro_status`, and why โ€” or null to stay off the * network. Pure over plain values so it can be tested without a clock, a database or config. * - * [coverageEnd] is the access expiry as the backend sends it, which is **grace-inclusive**; - * the renewal falls due at `coverageEnd - grace`. Subtracting is unconditional and needs no - * provider branching, because the wire sends grace = 0 whenever the subscription is not - * auto-renewing. + * [renewalDue] is the access expiry as the backend sends it โ€” the **payment-due date**. Coverage + * runs a further [grace] past it, so the grace window is `[renewalDue, renewalDue + grace)`. + * The backend states the contract as "`expiry_ts` + `grace_period_duration` is exactly when we + * stop serving". * * The four rows, which replace the spec's `E + grace <= now` expired test (that test both * double-counted grace and was unimplementable when written, because grace was not in config): @@ -712,26 +711,42 @@ class ProStatusManager @Inject constructor( * | `!auto_renewing && renewalDue` in the CTA window | fetch โ€” the Expiring CTA may fire | * | `!auto_renewing && now >= renewalDue` | confirm before the Expired CTA | * - * Row 1 keying off `renewalDue` rather than `coverageEnd` is the point: the grace window is - * `renewalDue <= now < coverageEnd`, so keying off coverage end would have declined to fetch - * for exactly the state this exists to surface. + * Row 2 is what surfaces grace: past the payment-due date while still covered is exactly the + * state the grace warning exists for, and it is reachable because coverage extends to + * `renewalDue + grace`. It is also the only row [grace] enters โ€” see the comment there. */ internal fun startupFetchReason( - coverageEnd: Instant?, + renewalDue: Instant?, autoRenewing: Boolean, grace: Duration, now: Instant, ): String? { // No access expiry at all: never subscribed, so no CTA can fire and nothing to confirm. - if (coverageEnd == null) return null + if (renewalDue == null) return null - val renewalDue = coverageEnd.minus(grace) val overdue = !now.isBefore(renewalDue) return when { autoRenewing && !overdue -> null - autoRenewing -> "auto-renewing and past the renewal date; grace is running" + // Past the payment date while auto-renewing: either the charge is retrying (grace + // is running) or it ultimately failed and coverage has since ended. Both want a + // fetch โ€” the first to raise the grace warning, the second to confirm before the + // Expired CTA. + // + // Bounded from COVERAGE end, not from the payment date, and this is the one place + // `grace` does any work. Every other row here needs only "is the renewal overdue", + // which is why grace no longer appears in them: under the corrected model the + // payment date arrives as `expiry` directly and nothing has to be reconstructed + // from it. Keeping the bound measured from `renewalDue + grace` means an account + // still inside a multi-day grace is never mistaken for a long-dead one, and an + // account dead for a year stops fetching on every cold start. + autoRenewing -> + if (renewalDue.plus(grace).plus(EXPIRED_CTA_WINDOW).isAfter(now)) { + "auto-renewing and past the payment date; grace or a failed renewal" + } else { + null + } !overdue -> if (renewalDue.isBefore(now.plus(EXPIRING_CTA_WINDOW))) { diff --git a/app/src/test/java/org/thoughtcrime/securesms/pro/ProStartupGateTest.kt b/app/src/test/java/org/thoughtcrime/securesms/pro/ProStartupGateTest.kt index 426b129235..34f96c0c7d 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/pro/ProStartupGateTest.kt +++ b/app/src/test/java/org/thoughtcrime/securesms/pro/ProStartupGateTest.kt @@ -8,15 +8,19 @@ import java.time.Duration import java.time.Instant /** - * The startup gate's decision, as a pure function of - * (coverage end, auto-renewing, grace, now). + * The startup gate's decision, as a pure function of (renewal due, auto-renewing, grace, now). * * Scope: the decision only. These do NOT cover the persisted 24h interval or the config read โ€” both * need a database, and the interval is checked before this function is reached. * - * The distinction every case below turns on: **the access expiry the backend sends is grace-inclusive**, - * so it is coverage END, and the renewal falls due a grace period earlier. Anything keyed to coverage - * end rather than to `coverageEnd - grace` is blind to the entire grace window. + * The model every case below turns on: **the access expiry the backend sends IS the payment-due date**, + * and coverage runs a further grace period past it โ€” the backend's own words are "`expiry_ts` + + * `grace_period_duration` is exactly when we stop serving". So `expiry` needs no adjustment to get the + * renewal date, and the grace window is `[expiry, expiry + grace)`. + * + * A note for anyone tempted to "fix" this by subtracting grace: that WAS the shape here, built against a + * backend that folded grace into the stored expiry. The fold was removed upstream and the field now + * reports how much longer we serve PAST the shown expiry. Subtracting now double-counts. */ class ProStartupGateTest { @@ -43,34 +47,55 @@ class ProStartupGateTest { @Test fun `auto-renewing and comfortably active does not fetch`() { - // Renewal due in 6 days (coverage end 20 days out, less 14 days of grace). + // The renewal is 20 days out, so nothing can have gone wrong with it yet. assertNull(startupFetchReason(inDays(20), autoRenewing = true, grace = grace, now = now)) } @Test fun `auto-renewing and inside the grace window DOES fetch`() { - // Coverage end is 7 days away, so `now` is comfortably before it โ€” but the renewal fell due 7 - // days ago and grace is running. This is the state the whole rework exists to surface, and a - // gate keyed to coverage end would sleep straight through it. - assertNotNull(startupFetchReason(inDays(7), autoRenewing = true, grace = grace, now = now)) + // The renewal fell due 7 days ago and grace runs 14, so coverage is still live but the charge + // has not landed. This is the state the whole rework exists to surface. + assertNotNull(startupFetchReason(daysAgo(7), autoRenewing = true, grace = grace, now = now)) } @Test fun `auto-renewing boundary - exactly at the renewal date fetches`() { - assertNotNull(startupFetchReason(now.plus(grace), autoRenewing = true, grace = grace, now = now)) + assertNotNull(startupFetchReason(now, autoRenewing = true, grace = grace, now = now)) } @Test fun `auto-renewing boundary - one second before the renewal date does not fetch`() { // The negative control for the pair above. Without it, "inside grace fetches" also passes // against a gate that fetches unconditionally whenever auto-renewing. - val justBefore = now.plus(grace).plusSeconds(1) - assertNull(startupFetchReason(justBefore, autoRenewing = true, grace = grace, now = now)) + assertNull(startupFetchReason(now.plusSeconds(1), autoRenewing = true, grace = grace, now = now)) } @Test fun `auto-renewing past coverage end still fetches`() { - assertNotNull(startupFetchReason(daysAgo(1), autoRenewing = true, grace = grace, now = now)) + // Renewal due 20 days ago, grace 14, so coverage ended 6 days ago: the renewal ultimately + // failed. Still worth a fetch โ€” this is the account about to be shown the Expired CTA, and + // config alone must never be the basis for that. + assertNotNull(startupFetchReason(daysAgo(20), autoRenewing = true, grace = grace, now = now)) + } + + @Test + fun `auto-renewing and long dead does not fetch`() { + // Renewal due 60 days ago, coverage ended 46 days ago, past the Expired CTA window. Without + // this bound an account whose renewing flag was never cleared fetches on every cold start + // forever. + assertNull(startupFetchReason(daysAgo(60), autoRenewing = true, grace = grace, now = now)) + } + + @Test + fun `the auto-renewing bound is measured from coverage end, not the payment date`() { + // 40 days past the payment date with 14 days of grace: coverage ended 26 days ago, so the + // Expired CTA can still fire and this must fetch. + // + // The discriminating case for where that bound is anchored. Measured from the payment date the + // account reads as 40 days gone โ€” past the 30 day window โ€” and this returns null. Only the + // coverage-end anchor gets it right, and only a grace period longer than the gap between the + // two anchors can tell them apart, which is why this uses the multi-day Google value. + assertNotNull(startupFetchReason(daysAgo(40), autoRenewing = true, grace = grace, now = now)) } // --- not auto-renewing ---------------------------------------------------------------------- @@ -107,16 +132,19 @@ class ProStartupGateTest { assertNull(startupFetchReason(inDays(60), autoRenewing = false, grace = noGrace, now = now)) } - // --- the subtraction is unconditional ------------------------------------------------------- + // --- grace's blast radius ------------------------------------------------------------------- @Test - fun `zero grace makes coverage end and the renewal date the same instant`() { - // The wire sends grace = 0 whenever the subscription is not auto-renewing, so subtracting - // needs no provider branching and no null handling โ€” it is a no-op for those accounts. Pinned - // because a future reader may be tempted to guard the subtraction. - val expiring = startupFetchReason(inDays(3), autoRenewing = false, grace = noGrace, now = now) - val alsoExpiring = startupFetchReason(inDays(3), autoRenewing = false, grace = Duration.ZERO, now = now) - assertNotNull(expiring) - assertNotNull(alsoExpiring) + fun `grace does not widen the non-renewing rows`() { + // Grace belongs to ONE row โ€” the auto-renewing one โ€” and these pin that. A non-renewing + // account 31 days past its expiry is out of CTA range whatever grace says, and one expiring in + // 3 days is in range whatever grace says. + // + // This is the guard against grace being reintroduced into the other rows by someone who + // remembers it mattering more. It cannot matter here: the wire sends grace = 0 when the + // subscription is not auto-renewing, so any behaviour keyed to a non-zero grace on this path + // is behaviour that never runs in production and only ever fires on a test fixture. + assertNull(startupFetchReason(daysAgo(31), autoRenewing = false, grace = grace, now = now)) + assertNotNull(startupFetchReason(inDays(3), autoRenewing = false, grace = grace, now = now)) } } From 3af519d9c8b94d29821ddd2bb2db54d03eeb76fa Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Tue, 11 Aug 2026 10:05:20 +1000 Subject: [PATCH 14/28] Pro: write the grace period on the status path, beside the expiry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The status fetch wrote E and A but never G, so the expiry it stored paired with whatever grace a proof outcome last left โ€” or with nothing at all. Everything downstream reads coverage as `E + G`, so the second wake at coverage end and the settings grace poll's termination bound were both computed against a grace this path never supplied. Takes the ACCOUNT-level field, not `latestPayment.gracePeriod`: they share a name and answer different questions โ€” how much longer we serve, versus what a store declared about one transaction. --- .../securesms/pro/FetchProStatusWorker.kt | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/FetchProStatusWorker.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/FetchProStatusWorker.kt index 08d3d1e00c..5d1057c8ae 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/FetchProStatusWorker.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/FetchProStatusWorker.kt @@ -102,6 +102,17 @@ class FetchProStatusWorker @AssistedInject constructor( // profile edit, and libsession omits the bump for it on purpose. configs.userProfile.setProAutoRenewing(details.autoRenewing) + // And the grace period, from the SAME response as the expiry above. Everything + // downstream reads coverage as `E + G`, so an E written without its G pairs with + // whatever G happens to be sitting there โ€” which, before this, was whatever a proof + // outcome last left, or nothing at all. Writing E and A here but not G made the + // gate's arithmetic a no-op. + // + // This is the ACCOUNT-level grace ("how much longer we serve past the expiry shown"), + // not `latestPayment.gracePeriod`, which reports what a store declared about one + // transaction. Same field name, different question. + configs.userProfile.setProGracePeriod(details.gracePeriod) + // Remove the pro config only when the backend authoritatively says we are no longer // pro (expired) or never were (never). An unknown/future status is NOT a basis to // delete it: removeProConfig() writes the SYNCED user profile, so clearing on an From c1dba22de12e1cbd9b3e35a262e53af85b1d119b Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Tue, 11 Aug 2026 10:10:17 +1000 Subject: [PATCH 15/28] Pro: rewrite the comments that explained the withdrawn grace model The arithmetic changed in 120e846dde; these are the prose explanations of why it used to be the other way. A confidently-worded wrong explanation is worse than none, so they are rewritten rather than patched around. --- .../preferences/prosettings/ProSettingsViewModel.kt | 2 +- .../securesms/pro/ProProofGenerationWorker.kt | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/preferences/prosettings/ProSettingsViewModel.kt b/app/src/main/java/org/thoughtcrime/securesms/preferences/prosettings/ProSettingsViewModel.kt index 791ff8e6df..fa532a79a8 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/preferences/prosettings/ProSettingsViewModel.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/preferences/prosettings/ProSettingsViewModel.kt @@ -759,7 +759,7 @@ class ProSettingsViewModel @AssistedInject constructor( } /** - * The instant the renewal falls due โ€” the account's paid-through end. + * The instant the renewal falls due. * * `expiry` IS the payment-due date. Coverage runs a further `gracePeriod` past it โ€” the backend's * contract is "`expiry_ts` + `grace_period_duration` is exactly when we stop serving" โ€” so do not diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt index e0e67be2fa..ee78fc8135 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt @@ -156,11 +156,11 @@ class ProProofGenerationWorker @AssistedInject constructor( response.accountExpiry?.let { configs.userProfile.setProAccessExpiry(it.epochSecond) } // Keep the renewing flag and the grace period coherent with the expiry above. - // Both must travel with it: everything derives the paid-through instant as - // `E - G`, so a fresh E beside a G from an older response is wrong by the - // difference between them โ€” worst exactly where it matters, since Google - // reports its real multi-day grace only once the subscriber ENTERS grace, so a - // proof landing first would pair a new coverage end with the ~1h stand-in. + // Both must travel with it: everything derives coverage end as `E + G`, so a + // fresh E beside a G from an older response is wrong by the difference between + // them โ€” worst exactly where it matters, since Google reports its real + // multi-day grace only once the subscriber ENTERS grace, so a proof landing + // first would pair a new payment date with the ~1h stand-in. // // โš ๏ธ These two are only meaningful HERE, inside the success branch, and nothing // in the type system says so โ€” that is the hazard. From 3499745f59d75faf34ab78f18fa66d3a002c6d50 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Tue, 11 Aug 2026 10:48:44 +1000 Subject: [PATCH 16/28] Pro: state the durable contract instead of another repo's current text MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four comments described what the backend or its test harness currently says rather than what the protocol guarantees: the grace direction was explained via "an earlier reading had the fold, it was removed upstream", and the floor gap cited a QA mule's specific override value. Nothing in this repo can notice when a claim like that goes stale โ€” CI never builds that file and no test touches it โ€” and it documents a state that will not exist once both sides have moved on. Each now states the durable fact: grace runs forward from the expiry, and the floor gap needs only "grace shorter than the floor", which production grace never is. The citations that support durable contract facts stay: those are claims about the wire, and if the wire changes this code changes with it. --- .../java/org/thoughtcrime/securesms/pro/ProDataMapper.kt | 5 ++--- .../org/thoughtcrime/securesms/pro/ProStatusManager.kt | 7 ++++--- .../org/thoughtcrime/securesms/pro/ProStatusRepository.kt | 8 ++++---- .../org/thoughtcrime/securesms/pro/ProStartupGateTest.kt | 6 +++--- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProDataMapper.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProDataMapper.kt index 8508514050..9dc4910eb8 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProDataMapper.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProDataMapper.kt @@ -52,9 +52,8 @@ fun GetProStatusResponse.toProStatus( // ACTIVE branch past `expiry` IS the grace period. // // Two traps here, both of which have caught someone: - // * Do NOT subtract grace to get the renewal date. `expiry` already IS that date. An - // earlier reading of the backend had grace folded into the stored expiry; that fold was - // removed and the field now reports how much longer we serve PAST the shown expiry. + // * Do NOT subtract grace to get the renewal date. `expiry` already IS that date, and + // grace runs forward from it โ€” subtracting double-counts. // * `gracePeriod` on THIS type is the account-level field โ€” "how much longer we serve" โ€” // and is not the same quantity as `ProPaymentItem.gracePeriod`, which reports what a // store declared about one transaction. They share a name and answer different diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt index 2ea7e3528a..2cf691c1f3 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt @@ -400,9 +400,10 @@ class ProStatusManager @Inject constructor( // the first one just armed. The symptom is indistinguishable from the wake never // having been scheduled, which is why this comment exists. // - // Only reachable on a compressed testing backend โ€” `google_play/mule.py` - // overrides grace with `testing_grace_period_duration_ms` = 10s. Production grace - // is the ~1h stand-in or an operator value in days, both far outside the floor. + // Only reachable where grace is shorter than the floor, which production grace + // never is โ€” it is at least the ~1h latency allowance, and an operator value in + // days once a subscriber is actually in grace. Compressed QA backends do set grace + // to seconds, which is where this shows up. // // Left alone deliberately (ruled 2026-08-10): the escape hatch is an env-var // override of the floor, owned by the Pro UI-test work. Do not make this wake diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusRepository.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusRepository.kt index beaeb91b9e..494597cc36 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusRepository.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusRepository.kt @@ -220,10 +220,10 @@ class ProStatusRepository @Inject constructor( * period is shorter than this floor the two wakes land inside it and **the second one's fetch * is dropped.** * - * In production that cannot happen: grace is either the backend's ~1h stand-in or an - * operator-configured value in days. It happens on a **compressed testing backend**, where - * `providers/google_play/mule.py` overrides grace with - * `api.testing_grace_period_duration_ms` = 10 seconds. + * In production that cannot happen: grace is at least the ~1h latency allowance, and an + * operator-configured value in days once a subscriber is actually in grace. It happens on + * **compressed QA backends**, which set grace to seconds so the window can be exercised in a + * test run. * * Deliberately not worked around here (ruled 2026-08-10). If UI-test work needs to exercise * the coverage-end wake, the sanctioned escape hatch is an **env-var override of this diff --git a/app/src/test/java/org/thoughtcrime/securesms/pro/ProStartupGateTest.kt b/app/src/test/java/org/thoughtcrime/securesms/pro/ProStartupGateTest.kt index 34f96c0c7d..2b6929934a 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/pro/ProStartupGateTest.kt +++ b/app/src/test/java/org/thoughtcrime/securesms/pro/ProStartupGateTest.kt @@ -18,9 +18,9 @@ import java.time.Instant * `grace_period_duration` is exactly when we stop serving". So `expiry` needs no adjustment to get the * renewal date, and the grace window is `[expiry, expiry + grace)`. * - * A note for anyone tempted to "fix" this by subtracting grace: that WAS the shape here, built against a - * backend that folded grace into the stored expiry. The fold was removed upstream and the field now - * reports how much longer we serve PAST the shown expiry. Subtracting now double-counts. + * If you are here because you expected a subtraction: there isn't one, and adding it double-counts. + * Grace runs FORWARD from the expiry. `the auto-renewing bound is measured from coverage end, not the + * payment date` is the case that pins the direction. */ class ProStartupGateTest { From 855d6f75de3a987dd231e707cbd5ce3c856a8f90 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Tue, 11 Aug 2026 13:42:47 +1000 Subject: [PATCH 17/28] Pro: refresh the status after clearing a revoked own-proof MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clearing the proof leaves the account asserting a future access expiry with nothing behind it, and nothing else corrects that: the config-change trigger watches E and the prepaid marker, and this path touches neither by design. The user sat on a stale expiry until some unrelated trigger fired. Asks the server rather than deciding locally. A revocation says this proof is void, not what the subscription is now โ€” the account may be fine and re-provable, or genuinely gone, and only one party knows which. So E is not cleared here. Gated on the clear having actually happened. The collector can fire for a hash that is no longer the stored proof, and a refresh triggered by someone else's revocation is a request with no reason behind it. Floored, not immediate โ€” nobody is waiting on a screen โ€” and requested after the config mutation closes rather than inside it. --- .../securesms/pro/ProStatusManager.kt | 25 ++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt index 2cf691c1f3..3790268b98 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt @@ -540,15 +540,38 @@ class ProStatusManager @Inject constructor( ) .filterNotNull() .collectLatest { revokedHash -> - configFactory.get().withMutableUserConfigs { configs -> + val cleared = configFactory.get().withMutableUserConfigs { configs -> if (configs.userProfile.getProConfig()?.proProof?.revocationTagHex == revokedHash) { Log.w( DebugLogGroup.PRO_SUBSCRIPTION.label, "Current Pro proof has been revoked, clearing Pro config" ) configs.userProfile.removeProConfig() + true + } else { + false } } + + // Ask the server what the state actually is now. Clearing the proof leaves the account + // asserting a future access expiry with nothing to back it, and NOTHING ELSE HERE WILL + // CORRECT THAT: the config-change trigger watches `E` and the prepaid marker, and this + // path deliberately touches neither. Without this the user sits on a stale expiry with + // no proof until some unrelated trigger happens to fire. + // + // `E` is deliberately NOT cleared locally. A revocation says this proof is void, not + // what the subscription is now โ€” the account may be fine and re-provable, or genuinely + // gone. Deciding that here would be the client overruling the only party that knows. + // + // Floored, not `immediate`: nobody is waiting on a screen. `immediate` is for the + // post-purchase poll and manual/recover. + // + // Outside the mutation block on purpose. It must also be gated on the clear actually + // happening โ€” this collector can fire for a hash that is no longer the stored proof, + // and a refresh for someone else's revocation is a request with no reason. + if (cleared) { + proStatusRepository.get().requestRefresh() + } } } From 0112690fae9e7669ba9f8a45a0a49c8522b60fc9 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Tue, 11 Aug 2026 13:52:35 +1000 Subject: [PATCH 18/28] Pro: state what the code guarantees, not what it used to do MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nine comments in this branch narrated the change rather than the contract: "it previously ended in", "this used to read", "every client used to fetch", "it used to have a second job". A reader in a year never saw the change, so none of that is actionable โ€” and where the narration carried the real warning, the warning was only implicit in it. Each now states the invariant directly and imperatively, which is both shorter and stronger: - the exhaustive `when` says do not add an `else`, and which two states a catch-all would sweep up - the proof worker says ask config, never loadState, because WorkManager outlives the process and loadState does not - the load-bearing per-process flag says what deleting it costs: the settings screen spins until the floor expires Also drops the claim that Desktop nearly shipped the spinning screen. Whether that stayed true is not observable from this repo, and the mechanism it was evidence for is stated here directly instead. --- .../prosettings/ProSettingsViewModel.kt | 5 ++- .../securesms/pro/FetchProStatusWorker.kt | 11 +++--- .../securesms/pro/ProProofGenerationWorker.kt | 10 ++--- .../securesms/pro/ProStatusManager.kt | 38 +++++++++---------- .../securesms/pro/ProStatusRepository.kt | 21 ++++------ 5 files changed, 39 insertions(+), 46 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/preferences/prosettings/ProSettingsViewModel.kt b/app/src/main/java/org/thoughtcrime/securesms/preferences/prosettings/ProSettingsViewModel.kt index fa532a79a8..f5f82d1c02 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/preferences/prosettings/ProSettingsViewModel.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/preferences/prosettings/ProSettingsViewModel.kt @@ -218,8 +218,9 @@ class ProSettingsViewModel @AssistedInject constructor( // The grace warning's "a completed fetch at or after the crossing" condition is applied // inside `toProStatus`, where inGracePeriod is produced โ€” so `subType.inGracePeriod` is - // already safe to read directly here and at the label below. It used to be gated at each - // consumer instead, which meant a new reader inherited no protection. + // already safe to read directly here and at the label below. Gating where the flag is + // produced rather than at each consumer means a new reader inherits the protection instead of + // having to know about it. while (true) { val now = clock.currentTime() diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/FetchProStatusWorker.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/FetchProStatusWorker.kt index 5d1057c8ae..9f480eed60 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/FetchProStatusWorker.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/FetchProStatusWorker.kt @@ -37,10 +37,9 @@ import javax.inject.Provider /** * A worker that fetches the user's Pro status from the server and updates the local database. * - * This worker doesn't do any business logic in terms of when to schedule itself, it simply performs - * the fetch and update operation regardlessly โ€” and that is now the whole of its job. It used to - * also schedule [ProProofGenerationWorker], which made proof renewal a downstream effect of a - * status fetch; that scheduling lives in [ProStatusManager] and keys off config instead. + * Performs the fetch and the update, and makes no scheduling decisions โ€” not even its own. Proof + * renewal is scheduled by [ProStatusManager]'s config watcher, deliberately not from here: keying it + * to a status response would make renewal a downstream effect of a display fetch. */ @HiltWorker class FetchProStatusWorker @AssistedInject constructor( @@ -136,8 +135,8 @@ class FetchProStatusWorker @AssistedInject constructor( // Proof generation is NOT scheduled from here. It runs off libsession's // `pro_renewal_target`, watched from config by - // ProStatusManager.manageProofRenewalScheduling โ€” so the config writes above reach it - // anyway, and a renewal no longer depends on a status fetch having happened. + // ProStatusManager.manageProofRenewalScheduling, which the config writes above reach on + // their own โ€” so renewal does not depend on a status fetch having happened. Result.success() } catch (e: CancellationException) { Log.d(TAG, "Work cancelled") diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt index ee78fc8135..6cb8467bd6 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt @@ -64,11 +64,11 @@ class ProProofGenerationWorker @AssistedInject constructor( // (redemption, where get_pro_status is not ACTIVE yet and minting the proof is what pulls // the entitlement through), and an entitlement held with no proof to attach. // - // This used to read `proStatusRepository.loadState` for an ACTIVE status instead. That was - // the last status->proof dependency, and it was wrong in a way that only showed after a - // process restart: WorkManager persists our schedule, but `loadState` starts at Init, so a - // renewal that came due while the app was dead saw "not active, no purchase" and returned - // without renewing. + // Ask CONFIG, never `proStatusRepository.loadState`. WorkManager persists this worker's + // schedule across process death; `loadState` does not survive it and restarts at `Init`. An + // in-memory status check therefore reads "not active, no purchase" and returns without + // renewing, for exactly the renewal that came due while the app was dead โ€” the case the + // persisted schedule exists to serve. val now = snodeClock.currentTime() val (renewalTarget, purchasePending, proof) = configFactory.withUserConfigs { configs -> Triple( diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt index 3790268b98..6b99969228 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt @@ -131,13 +131,13 @@ class ProStatusManager @Inject constructor( // data (`HomeViewModel`'s Expired CTA above all), so anything else must not // report success. // - // Exhaustive on purpose, with no `else`. It previously ended in - // `else -> State.Success(Unit)`, which quietly swept up two states that are not - // successes: `Init` (nothing has happened yet) and โ€” the one that caused a live - // bug โ€” a `Loaded` restored from WorkManager's PERSISTED work state, i.e. a fetch - // some earlier process made. A renewal that happened while the app was closed - // then showed the Expired CTA off the stale cache on the next launch. Listing - // every case means the next state added here has to declare which it is. + // Exhaustive on purpose โ€” no `else`, and do not add one. Two states are not + // successes and a catch-all sweeps up both: `Init`, where nothing has been asked + // yet, and a `Loaded` restored from WorkManager's PERSISTED work state, which is + // a fetch some EARLIER process made. The second is the dangerous one โ€” a renewal + // landing while the app is closed leaves a stale cache that reads as confirmed, + // and the home Expired CTA fires off it. Listing every case forces the next state + // added here to declare which it is. when(proStatusState){ is ProStatusRepository.LoadState.Loading -> { if(proStatusState.waitingForNetwork) State.Error(Exception()) @@ -436,12 +436,12 @@ class ProStatusManager @Inject constructor( /** * Trigger #1 โ€” the startup fetch, gated. * - * Every client used to fetch `get_pro_status` on every cold start, including users who have never - * subscribed and users who are comfortably paid up. "Cold start" is something mobile does - * constantly, and none of those fetches had a consumer: entitlement runs off the proof, the + * An ungated cold-start fetch has no consumer for most users: entitlement runs off the proof, the * settings screen refreshes when opened, and account-expiry awareness is the `E+30s` wake. The - * only real consumer is the home CTAs, so the gate asks whether a CTA could plausibly fire, from - * synced config alone, and otherwise stays off the network entirely. + * only real consumer is the home CTAs โ€” so the gate asks whether a CTA could plausibly fire, from + * synced config alone, and otherwise stays off the network entirely. Users who never subscribed + * and users comfortably paid up therefore make no request at all, which matters because "cold + * start" is something mobile does constantly. * * Two independent brakes: the CTA-worthiness test below, and a persisted 24h minimum between * startup fetches. The interval has its own key โ€” a routine refresh must not consume the gate's @@ -479,11 +479,10 @@ class ProStatusManager @Inject constructor( /** * Drives proof acquisition/renewal purely from config, off libsession's `pro_renewal_target`. * - * This used to be kicked by [FetchProStatusWorker] off the get_pro_status response, which made - * the proof loop a downstream effect of a status fetch: no fetch, no renewal. The inputs - * libsession actually needs โ€” the stored proof, the access expiry (E) and the prepaid marker - * (I) โ€” all live in the user profile, so watching them directly is both sufficient and honest - * about the dependency. + * The inputs libsession needs for `pro_renewal_target` โ€” the stored proof, the access expiry (E) + * and the prepaid marker (I) โ€” all live in the user profile, so watching them directly is + * sufficient. Do not drive this off a `get_pro_status` response instead: that makes the proof + * loop a downstream effect of a display fetch, so no fetch means no renewal. * * The loop closes without a status fetch anywhere in it: the proof worker's own config writes * (a new proof, a refreshed or cleared E) re-enter here and schedule the next attempt, and a @@ -760,9 +759,8 @@ class ProStatusManager @Inject constructor( // // Bounded from COVERAGE end, not from the payment date, and this is the one place // `grace` does any work. Every other row here needs only "is the renewal overdue", - // which is why grace no longer appears in them: under the corrected model the - // payment date arrives as `expiry` directly and nothing has to be reconstructed - // from it. Keeping the bound measured from `renewalDue + grace` means an account + // which the payment date answers on its own โ€” grace has no part in them. Keeping + // this bound measured from `renewalDue + grace` means an account // still inside a multi-day grace is never mistaken for a long-dead one, and an // account dead for a year stops fetching on every cold start. autoRenewing -> diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusRepository.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusRepository.kt index 494597cc36..c717b54146 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusRepository.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusRepository.kt @@ -135,8 +135,8 @@ class ProStatusRepository @Inject constructor( * redundant, and it is not: the persisted value answers *"was the last fetch recent?"*, this * answers *"has this process asked at all?"*. * - * Its remaining job is to guarantee the FIRST request of a process reaches the network โ€” and to - * be precise about why that job exists: **this is what stops the floor becoming a mutex.** + * Its job is to guarantee the FIRST request of a process reaches the network. Precisely: **this is + * what stops the floor becoming a mutex.** * * Three separate things can refuse to start a status fetch, and they are easy to conflate: * @@ -151,18 +151,13 @@ class ProStatusRepository @Inject constructor( * refused by a decision no part of this process ever took, and after the startup gate nothing * else would ask. * - * That reasoning survives someone fixing the confirmed-status problem another way, which the - * previous "restores the Loading transition" justification did not. + * Note that (2) being handled properly by [LoadState.Loaded.confirmedInThisProcess] does **not** + * make this removable โ€” the two guard different refusals, and only this one is reachable in a + * process that has not fetched. * - * (It used to have a second job โ€” restoring the `Loading` transition that suppressed the home - * Expired CTA. That is now done properly by [LoadState.Loaded.confirmedInThisProcess], so this - * field is no longer what stands between a stale cache and a false CTA. The retirement condition - * is therefore no longer "when the predicate is fixed" โ€” the predicate IS fixed and this is still - * needed.) - * - * The tempting cleanup is a demonstrated failure, not a hypothetical one: the Desktop client - * removed its equivalent per-run value during the same rework and would have shipped a - * permanently-spinning Pro screen. + * What deleting it costs, concretely: the Pro settings screen refreshes on entry through the + * floored path, so on a relaunch inside the interval that refresh is refused, the screen has no + * confirmed status to render, and nothing remaining will ask. It spins until the interval expires. */ @Volatile private var fetchedInThisProcess = false From c930ec10eae4ab1f8baf8402df67d872736d7e4e Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Tue, 11 Aug 2026 13:55:05 +1000 Subject: [PATCH 19/28] Pro: say which test the startup gate omits, not which one it replaced The row table was introduced as replacing a spec test that "double-counted grace and was unimplementable when written". A reader of this function has no access to that document and nothing to do with its history. The durable half is that no row keys off coverage end, and why: a cold start does not need to know when coverage ends, and testing it would double-count grace against the payment date the rows already turn on. That is now stated as a property of the rows. --- .../java/org/thoughtcrime/securesms/pro/ProStatusManager.kt | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt index 6b99969228..ce5ecb049b 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt @@ -724,8 +724,9 @@ class ProStatusManager @Inject constructor( * The backend states the contract as "`expiry_ts` + `grace_period_duration` is exactly when we * stop serving". * - * The four rows, which replace the spec's `E + grace <= now` expired test (that test both - * double-counted grace and was unimplementable when written, because grace was not in config): + * The decision is four rows over config state. Note that none of them tests + * `renewalDue + grace <= now`: coverage end is not what a cold start needs to know, and keying + * any row to it double-counts grace against the payment date the rows already turn on. * * | config state | action | * |----------------------------------------------------|-----------------------------------| From 97b1a4e534fb1c47fd104a4ed85a037f43bb7f38 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Tue, 11 Aug 2026 14:16:02 +1000 Subject: [PATCH 20/28] Pro: anchor the Expired CTA at coverage end MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The backend only reports EXPIRED once coverage has ended, and coverage ends a grace period after the payment-due date it sends. Measuring the 30-day CTA window from the payment date therefore yields a window of 30d - G, and none at all once G reaches 30 days. Google Play grace is operator- configurable up to 30 days, so the empty case is reachable in production; an account with 16 days of store grace got 14 days of CTA. Expired CTA fires while E + G <= now < E + G + 30d Expiring CTA fires while E - 7d <= now < E The displayed date stays E everywhere. The Expiring window is deliberately untouched: "your payment is due soon" is a statement about the payment date, not about coverage. `expiredAt` keeps meaning the payment-due date, which is the value the display model says we show, and the grace period travels beside it. Coverage end is derived on the type rather than at the consumer, so a second reader cannot pick the other anchor. Naming it after what it is also makes the CTA deadline visibly the same instant as the startup gate's bound; the two disagreed by G. Pre-existing, not from this rework โ€” 44ab07f2eac. --- .../securesms/home/HomeViewModel.kt | 11 ++- .../securesms/pro/ProDataMapper.kt | 3 + .../thoughtcrime/securesms/pro/ProStatus.kt | 20 ++++- .../securesms/pro/ProStatusManager.kt | 6 ++ .../pro/ProExpiredCoverageEndTest.kt | 87 +++++++++++++++++++ 5 files changed, 122 insertions(+), 5 deletions(-) create mode 100644 app/src/test/java/org/thoughtcrime/securesms/pro/ProExpiredCoverageEndTest.kt diff --git a/app/src/main/java/org/thoughtcrime/securesms/home/HomeViewModel.kt b/app/src/main/java/org/thoughtcrime/securesms/home/HomeViewModel.kt index 392244538e..a52bf1ad42 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/home/HomeViewModel.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/home/HomeViewModel.kt @@ -255,12 +255,15 @@ class HomeViewModel @Inject constructor( // network failure must not surface a false "expired". Consistent with the iOS fix. && subscription.refreshState is org.thoughtcrime.securesms.util.State.Success && !prefs.hasSeenProExpired()) { - val validUntil = subscription.type.expiredAt - showExpired = now.isBefore(validUntil.plus(30, ChronoUnit.DAYS)) + // Anchored at coverage end, not at the payment date: the backend only reports + // EXPIRED once coverage has ended, so measuring from the payment date shortens + // this window by exactly the grace period and empties it entirely when grace is + // 30 days or more. + val coverageEnded = subscription.type.coverageEndedAt + showExpired = now.isBefore(coverageEnded.plus(30, ChronoUnit.DAYS)) - Log.d(DebugLogGroup.PRO_DATA.label, "Home: Pro expired. Expired at: $validUntil - Should show Expired CTA? $showExpired") + Log.d(DebugLogGroup.PRO_DATA.label, "Home: Pro expired. Coverage ended: $coverageEnded - Should show Expired CTA? $showExpired") - // Check if now is within 30 days after expiry if (showExpired) { _dialogsState.update { state -> state.copy(proExpiredCTA = true) diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProDataMapper.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProDataMapper.kt index 9dc4910eb8..728de84258 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProDataMapper.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProDataMapper.kt @@ -102,6 +102,7 @@ fun GetProStatusResponse.toProStatus( ProUserStatus.EXPIRED -> ProStatus.Expired( expiredAt = expiry ?: Instant.EPOCH, + gracePeriod = gracePeriod, providerData = providerMetadata( latestPayment?.paymentProvider ?: PAYMENT_PROVIDER_GOOGLE_PLAY, context, @@ -174,5 +175,7 @@ val previewAutoRenewingApple = ProStatus.Active.AutoRenewing( val previewExpiredApple = ProStatus.Expired( expiredAt = Instant.now() - Duration.ofDays(14), + // Zero grace, so expiredAt and coverage end coincide: the fixture means what it reads as. + gracePeriod = Duration.ZERO, providerData = previewAppleMetaData ) diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatus.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatus.kt index c10960edf8..24e95f24f8 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatus.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatus.kt @@ -4,6 +4,7 @@ import network.loki.messenger.BuildConfig import org.thoughtcrime.securesms.pro.subscription.ProPlanPeriod import org.thoughtcrime.securesms.util.DateUtils import org.thoughtcrime.securesms.util.State +import java.time.Duration import java.time.Instant sealed interface ProStatus{ @@ -56,9 +57,26 @@ sealed interface ProStatus{ } data class Expired( + /** + * The payment-due date, as the backend sends it. This is the date to display; coverage ran a + * further [gracePeriod] past it. + */ val expiredAt: Instant, + val gracePeriod: Duration, val providerData: PaymentProviderMetadata - ): ProStatus + ): ProStatus { + /** + * When access actually ended, and the anchor for anything measuring how long ago that was. + * + * The backend only reports EXPIRED once coverage has ended, so measuring an "expired + * recently" window from [expiredAt] instead shortens it by exactly [gracePeriod] โ€” and + * empties it when the grace period is at least as long as the window. Google Play grace is + * operator-configurable up to 30 days, so that is reachable and not a corner case. + * + * Derived here rather than at the consumer so a second reader cannot pick the other anchor. + */ + val coverageEndedAt: Instant get() = expiredAt.plus(gracePeriod) + } } data class ProDataState( diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt index ce5ecb049b..a19399c2d0 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt @@ -243,14 +243,20 @@ class ProStatusManager @Inject constructor( DebugMenuViewModel.DebugSubscriptionStatus.EXPIRED -> ProStatus.Expired( expiredAt = now - Duration.ofDays(14), + // Zero grace: these fixtures mean "coverage ended N days ago". + gracePeriod = Duration.ZERO, providerData = providerMetadata(PAYMENT_PROVIDER_GOOGLE_PLAY, application) ) DebugMenuViewModel.DebugSubscriptionStatus.EXPIRED_EARLIER -> ProStatus.Expired( expiredAt = now - Duration.ofDays(60), + // Zero grace: these fixtures mean "coverage ended N days ago". + gracePeriod = Duration.ZERO, providerData = providerMetadata(PAYMENT_PROVIDER_GOOGLE_PLAY, application) ) DebugMenuViewModel.DebugSubscriptionStatus.EXPIRED_APPLE -> ProStatus.Expired( expiredAt = now - Duration.ofDays(14), + // Zero grace: these fixtures mean "coverage ended N days ago". + gracePeriod = Duration.ZERO, providerData = providerMetadata(PAYMENT_PROVIDER_APP_STORE, application) ) }.withMockedExpiry(debugAccessExpiry), diff --git a/app/src/test/java/org/thoughtcrime/securesms/pro/ProExpiredCoverageEndTest.kt b/app/src/test/java/org/thoughtcrime/securesms/pro/ProExpiredCoverageEndTest.kt new file mode 100644 index 0000000000..8bd47e21e9 --- /dev/null +++ b/app/src/test/java/org/thoughtcrime/securesms/pro/ProExpiredCoverageEndTest.kt @@ -0,0 +1,87 @@ +package org.thoughtcrime.securesms.pro + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test +import java.time.Duration +import java.time.Instant + +/** + * [ProStatus.Expired.coverageEndedAt] and the 30-day Expired-CTA window measured from it. + * + * The backend only reports EXPIRED once coverage has ended, and coverage ends a grace period after the + * payment-due date it sends. So a window measured from the payment date is short by exactly the grace + * period, and empty once grace reaches the window length โ€” which Google Play grace can, being + * operator-configurable up to 30 days. + * + * These pin the anchor. The CTA condition itself lives in `HomeViewModel`, which needs Android; what + * is testable here is the instant it keys off, and that is the part that was wrong. + */ +class ProExpiredCoverageEndTest { + + private val ctaWindow: Duration = Duration.ofDays(30) + private val paymentDue: Instant = Instant.parse("2026-08-01T00:00:00Z") + + private fun expired(grace: Duration) = ProStatus.Expired( + expiredAt = paymentDue, + gracePeriod = grace, + providerData = previewAppleMetaData, + ) + + /** The CTA condition from `HomeViewModel`, over the anchor this type exposes. */ + private fun ctaShows(grace: Duration, now: Instant): Boolean = + now.isBefore(expired(grace).coverageEndedAt.plus(ctaWindow)) + + @Test + fun `coverage ends a grace period after the payment date`() { + assertEquals( + paymentDue.plus(Duration.ofDays(16)), + expired(Duration.ofDays(16)).coverageEndedAt, + ) + } + + @Test + fun `zero grace leaves the payment date as coverage end`() { + // The wire sends grace = 0 for an account that is not renewing, so the two anchors coincide and + // this fix is a no-op for those accounts rather than a change. + assertEquals(paymentDue, expired(Duration.ZERO).coverageEndedAt) + } + + @Test + fun `the window is a full 30 days of coverage having ended, whatever grace was`() { + // The property that matters: window length is independent of grace. A 16-day store grace used to + // cost 16 of the 30 days. + for (graceDays in listOf(0L, 1L, 16L, 29L, 45L)) { + val grace = Duration.ofDays(graceDays) + val coverageEnd = paymentDue.plus(grace) + assertTrue( + "grace=$graceDays: should show one day after coverage ended", + ctaShows(grace, coverageEnd.plus(Duration.ofDays(1))), + ) + assertTrue( + "grace=$graceDays: should show at 29 days after coverage ended", + ctaShows(grace, coverageEnd.plus(Duration.ofDays(29))), + ) + assertFalse( + "grace=$graceDays: should not show at 30 days after coverage ended", + ctaShows(grace, coverageEnd.plus(ctaWindow)), + ) + } + } + + @Test + fun `a grace period as long as the window does not empty it`() { + // The reachable failure of the payment-date anchor, not a corner case: with 30 days of grace, + // `now` is already past `paymentDue + 30d` the moment EXPIRED can first be reported, so that + // anchor yields no window at all and the CTA could never fire. + val grace = ctaWindow + val firstMomentExpiredIsReportable = paymentDue.plus(grace).plusSeconds(1) + + assertTrue(ctaShows(grace, firstMomentExpiredIsReportable)) + assertFalse( + "the payment-date anchor is what this test exists to rule out", + firstMomentExpiredIsReportable.isBefore(paymentDue.plus(ctaWindow)), + ) + } +} From 1ab05474ac3719e0086c58633618ae4792ebb0aa Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Tue, 11 Aug 2026 14:19:10 +1000 Subject: [PATCH 21/28] Pro: say why the expiry and grace both come off the response MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The backend derives the grace it sends from the same coverage-end instant it judged the status against, so the two describe one moment together and none apart. Config is not an equivalent source for either: not every status branch writes G there, and the branches that clear E cascade G away with it, so a config read would pair this response's expiry with a grace period from a different one. Comment only โ€” both values already came off the response. --- .../java/org/thoughtcrime/securesms/pro/ProDataMapper.kt | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProDataMapper.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProDataMapper.kt index 728de84258..00fb33f9f7 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProDataMapper.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProDataMapper.kt @@ -101,6 +101,15 @@ fun GetProStatusResponse.toProStatus( } ProUserStatus.EXPIRED -> ProStatus.Expired( + // `expiry` and `gracePeriod` are only meaningful as a PAIR, from one response. The backend + // derives the grace it sends from the same coverage-end instant it judged this status + // against, so together they describe one moment; separately they describe none. + // + // That is why both come off the response and neither is read from config. Config is not an + // equivalent source: not every status branch writes `G` there, and the branches that clear + // `E` cascade `G` away with it โ€” so a config read here would pair THIS response's expiry + // with a grace period from a different response, or with nothing at all, and the resulting + // coverage end would be short by however much they disagreed. expiredAt = expiry ?: Instant.EPOCH, gracePeriod = gracePeriod, providerData = providerMetadata( From 0e8133bbce3a6f125a7bdb1926b5e714bccb3fef Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Tue, 11 Aug 2026 14:54:14 +1000 Subject: [PATCH 22/28] Pro: attribute the multi-day grace to Apple, which is where it comes from Five comments had the two stores the wrong way round, saying Apple's grace is ~1h and Google's is the operator-configured multi-day value. It is the other way. Play applies grace by extending the expiry it reports, so the backend deliberately stores no separate number for it and a Play account's grace is just the ~1h renewal-latency allowance. Apple states its dunning window separately, so that is the one arriving as a multi-day grace. Consequences for the reader, which is why this is worth correcting rather than leaving as trivia: the Expired-CTA re-anchor changes behaviour for Apple accounts whose dunning ran out, and is legitimately inert for Play, where the expiry already contains the grace. A reader told the opposite would look for the effect on the wrong population and conclude the anchor was broken. The window-length tests sweep grace as a parameter, so they were asserting the invariant rather than either store's number and did not change. --- .../main/java/org/thoughtcrime/securesms/pro/ProStatus.kt | 8 ++++++-- .../org/thoughtcrime/securesms/pro/ProStatusManager.kt | 6 +++--- .../org/thoughtcrime/securesms/pro/ProStatusRepository.kt | 5 +++-- .../securesms/pro/ProExpiredCoverageEndTest.kt | 8 ++++++-- .../org/thoughtcrime/securesms/pro/ProStartupGateTest.kt | 6 +++++- 5 files changed, 23 insertions(+), 10 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatus.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatus.kt index 24e95f24f8..f64c5f174f 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatus.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatus.kt @@ -70,8 +70,12 @@ sealed interface ProStatus{ * * The backend only reports EXPIRED once coverage has ended, so measuring an "expired * recently" window from [expiredAt] instead shortens it by exactly [gracePeriod] โ€” and - * empties it when the grace period is at least as long as the window. Google Play grace is - * operator-configurable up to 30 days, so that is reachable and not a corner case. + * empties it when the grace period is at least as long as the window. + * + * [gracePeriod] is multi-day for an Apple account whose dunning window ran out, because Apple + * states its retry window separately and it arrives here as grace. It is only the ~1h + * renewal-latency allowance on Play, which folds grace into the expiry it reports, and zero for + * an account that is not renewing at all โ€” for those two the anchors effectively coincide. * * Derived here rather than at the consumer so a second reader cannot pick the other anchor. */ diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt index a19399c2d0..d70a265677 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt @@ -407,9 +407,9 @@ class ProStatusManager @Inject constructor( // having been scheduled, which is why this comment exists. // // Only reachable where grace is shorter than the floor, which production grace - // never is โ€” it is at least the ~1h latency allowance, and an operator value in - // days once a subscriber is actually in grace. Compressed QA backends do set grace - // to seconds, which is where this shows up. + // never is: while a renewal is still going to be attempted the backend always adds + // a ~1h renewal-latency allowance on top of any window the store stated. + // Compressed QA backends do set grace to seconds, which is where this shows up. // // Left alone deliberately (ruled 2026-08-10): the escape hatch is an env-var // override of the floor, owned by the Pro UI-test work. Do not make this wake diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusRepository.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusRepository.kt index c717b54146..67a9383f89 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusRepository.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusRepository.kt @@ -215,8 +215,9 @@ class ProStatusRepository @Inject constructor( * period is shorter than this floor the two wakes land inside it and **the second one's fetch * is dropped.** * - * In production that cannot happen: grace is at least the ~1h latency allowance, and an - * operator-configured value in days once a subscriber is actually in grace. It happens on + * In production that cannot happen: while a renewal is still going to be attempted the backend + * always adds a ~1h renewal-latency allowance, on top of any window the store stated. It + * happens on * **compressed QA backends**, which set grace to seconds so the window can be exercised in a * test run. * diff --git a/app/src/test/java/org/thoughtcrime/securesms/pro/ProExpiredCoverageEndTest.kt b/app/src/test/java/org/thoughtcrime/securesms/pro/ProExpiredCoverageEndTest.kt index 8bd47e21e9..09fcf1453c 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/pro/ProExpiredCoverageEndTest.kt +++ b/app/src/test/java/org/thoughtcrime/securesms/pro/ProExpiredCoverageEndTest.kt @@ -12,8 +12,12 @@ import java.time.Instant * * The backend only reports EXPIRED once coverage has ended, and coverage ends a grace period after the * payment-due date it sends. So a window measured from the payment date is short by exactly the grace - * period, and empty once grace reaches the window length โ€” which Google Play grace can, being - * operator-configurable up to 30 days. + * period, and empty once grace reaches the window length. + * + * A multi-day grace on the wire means an Apple account whose dunning window ran out: Apple states its + * retry window separately, so it arrives as grace. Play folds grace into the expiry it reports, so a + * Play account's grace is only the ~1h renewal-latency allowance and the two anchors all but coincide. + * The cases below therefore sweep grace as a parameter rather than asserting one store's number. * * These pin the anchor. The CTA condition itself lives in `HomeViewModel`, which needs Android; what * is testable here is the instant it keys off, and that is the part that was wrong. diff --git a/app/src/test/java/org/thoughtcrime/securesms/pro/ProStartupGateTest.kt b/app/src/test/java/org/thoughtcrime/securesms/pro/ProStartupGateTest.kt index 2b6929934a..36ab5eb80c 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/pro/ProStartupGateTest.kt +++ b/app/src/test/java/org/thoughtcrime/securesms/pro/ProStartupGateTest.kt @@ -26,7 +26,11 @@ class ProStartupGateTest { private val now: Instant = Instant.parse("2026-08-07T00:00:00Z") - /** A realistic Google Play base-plan grace period. Apple's is ~1h; Google's is days. */ + /** + * A realistic multi-day grace period, which on the wire means an Apple account mid-dunning: Apple + * states its retry window separately, so it arrives as grace. Play folds its grace into the expiry + * instead, so a Play account's grace is just the ~1h renewal-latency allowance. + */ private val grace: Duration = Duration.ofDays(14) private val noGrace: Duration = Duration.ZERO From 78f5337149a57cbd07f0f40915314b0b013d8833 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Tue, 11 Aug 2026 15:40:30 +1000 Subject: [PATCH 23/28] Pro: share the CTA/gate windows, and state invariants once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two changes, in one commit because they touch the same lines. Q6 โ€” the 24h, 7d and 30d windows move to `ProRefreshWindows`, and the wake slack becomes `WAKE_SLACK`. They were named in the gate's companion and bare literals in HomeViewModel, so tuning the CTA window left the gate behind and the two then disagreed about the same instant โ€” the one property they exist to share. A shared object rather than widened visibility because the gate is not their only consumer and they should not read as its internals. Comments โ€” cuts the argument around the invariants rather than the invariants: how a conclusion was reached, what was believed before, rhetorical emphasis, and the same rule restated at three sites. The floor/poll-cadence equality now lives only at GRACE_POLL_INTERVAL_MS, which owns it; the QA-backend interaction only at MIN_UPDATE_INTERVAL_SECONDS. Their former call-site copies point instead. Two more leftovers of the withdrawn Google grace story went with them. Kept in full: the E/G model where it is the contract, the presence-only erase trap on the proof path, the ordering dependency between the two wakes and the floor, and the cross-client constants preamble. 618 -> 439 comment lines of 1001 added (52% -> 44%). --- .../securesms/home/HomeViewModel.kt | 6 +- .../prosettings/ProSettingsViewModel.kt | 55 ++--- .../securesms/pro/FetchProStatusWorker.kt | 35 ++-- .../securesms/pro/ProDataMapper.kt | 49 ++--- .../securesms/pro/ProProofGenerationWorker.kt | 63 +++--- .../securesms/pro/ProRefreshWindows.kt | 25 +++ .../thoughtcrime/securesms/pro/ProStatus.kt | 15 +- .../securesms/pro/ProStatusManager.kt | 189 +++++++----------- .../securesms/pro/ProStatusRepository.kt | 110 +++------- .../securesms/pro/db/ProDatabase.kt | 22 +- .../pro/ProExpiredCoverageEndTest.kt | 22 +- .../securesms/pro/ProStartupGateTest.kt | 68 +++---- .../pro/ProStatusFreshnessFloorTest.kt | 40 ++-- 13 files changed, 271 insertions(+), 428 deletions(-) create mode 100644 app/src/main/java/org/thoughtcrime/securesms/pro/ProRefreshWindows.kt diff --git a/app/src/main/java/org/thoughtcrime/securesms/home/HomeViewModel.kt b/app/src/main/java/org/thoughtcrime/securesms/home/HomeViewModel.kt index a52bf1ad42..ec1594eed7 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/home/HomeViewModel.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/home/HomeViewModel.kt @@ -52,6 +52,7 @@ import org.thoughtcrime.securesms.onboarding.OnBoardingPreferences.HAS_VIEWED_SE import org.thoughtcrime.securesms.preferences.AppPreferences import org.thoughtcrime.securesms.preferences.PreferenceStorage import org.thoughtcrime.securesms.preferences.prosettings.ProSettingsDestination +import org.thoughtcrime.securesms.pro.ProRefreshWindows import org.thoughtcrime.securesms.pro.ProStatus import org.thoughtcrime.securesms.pro.ProStatusManager import org.thoughtcrime.securesms.repository.ConversationRepository @@ -67,7 +68,6 @@ import org.thoughtcrime.securesms.util.UserProfileUtils import org.thoughtcrime.securesms.webrtc.CallManager import org.thoughtcrime.securesms.webrtc.data.State import java.time.Instant -import java.time.temporal.ChronoUnit import javax.inject.Inject @HiltViewModel @@ -236,7 +236,7 @@ class HomeViewModel @Inject constructor( && !prefs.hasSeenProExpiring() ){ val validUntil = subscription.type.renewingAt - showExpiring = validUntil.isBefore(now.plus(7, ChronoUnit.DAYS)) + showExpiring = validUntil.isBefore(now.plus(ProRefreshWindows.EXPIRING_CTA)) Log.d(DebugLogGroup.PRO_DATA.label, "Home: Pro active but not auto renewing (expiring). Valid until: $validUntil - Should show Expiring CTA? $showExpiring") if (showExpiring) { _dialogsState.update { state -> @@ -260,7 +260,7 @@ class HomeViewModel @Inject constructor( // this window by exactly the grace period and empties it entirely when grace is // 30 days or more. val coverageEnded = subscription.type.coverageEndedAt - showExpired = now.isBefore(coverageEnded.plus(30, ChronoUnit.DAYS)) + showExpired = now.isBefore(coverageEnded.plus(ProRefreshWindows.EXPIRED_CTA)) Log.d(DebugLogGroup.PRO_DATA.label, "Home: Pro expired. Coverage ended: $coverageEnded - Should show Expired CTA? $showExpired") diff --git a/app/src/main/java/org/thoughtcrime/securesms/preferences/prosettings/ProSettingsViewModel.kt b/app/src/main/java/org/thoughtcrime/securesms/preferences/prosettings/ProSettingsViewModel.kt index f5f82d1c02..1b07ffab09 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/preferences/prosettings/ProSettingsViewModel.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/preferences/prosettings/ProSettingsViewModel.kt @@ -110,16 +110,12 @@ class ProSettingsViewModel @AssistedInject constructor( private var recovering: Boolean = false init { - // Trigger #3 โ€” refresh on entering Pro settings. Floored, not `immediate`: this screen is - // the one place the status is actually read, so it should not be showing a value from an - // arbitrarily old background fetch, but arriving here is not on its own a reason to bypass - // the 60s floor. + // Trigger #3 โ€” refresh on entering Pro settings. Floored: arriving here is not on its own a + // reason to bypass the floor. // - // Deliberately NOT via refreshProStatus(): that early-returns while `refreshState` is - // Loading, and a process that hasn't confirmed a fetch of its own reports Loading from - // launch. Routing on-enter through that guard would mean the one trigger able to - // resolve the state is the one the state suppresses โ€” a spinner that never clears, which is - // the failure iOS hit from the same direction. The repository single-flights anyway + // Not via refreshProStatus(), which early-returns while `refreshState` is Loading โ€” and a + // process that has not confirmed a fetch reports Loading from launch, so that guard would + // suppress the one trigger able to clear it. The repository single-flights anyway // (WorkManager REPLACE), so the guard buys nothing here. proStatusRepository.requestRefresh(immediate = false) @@ -216,11 +212,8 @@ class ProSettingsViewModel @AssistedInject constructor( recovering = false } - // The grace warning's "a completed fetch at or after the crossing" condition is applied - // inside `toProStatus`, where inGracePeriod is produced โ€” so `subType.inGracePeriod` is - // already safe to read directly here and at the label below. Gating where the flag is - // produced rather than at each consumer means a new reader inherits the protection instead of - // having to know about it. + // `inGracePeriod` is safe to read directly here and at the label below: the "completed fetch at + // or after the crossing" condition is applied in `toProStatus`, where the flag is produced. while (true) { val now = clock.currentTime() @@ -724,19 +717,16 @@ class ProSettingsViewModel @AssistedInject constructor( /** * Trigger #4 โ€” poll `get_pro_status` while the renewal is overdue and this screen is open. * - * Deliberately NOT a 60s timer on the screen. It sleeps until the renewal falls due and only - * then polls, once a minute, while the renewal still hasn't landed. Three things stop it: the - * renewal arrives (`expiry` advances, which restarts this from the new date), the account runs - * past coverage, or the screen closes and cancels `viewModelScope`. + * Not a 60s timer on the screen: it sleeps until the renewal falls due, then polls once a minute + * while the renewal still hasn't landed. Three things stop it โ€” the renewal arrives (`expiry` + * advances, restarting this from the new date), the account runs past coverage, or the screen + * closes and cancels `viewModelScope`. * - * Note it needs no `auto_renewing` check. The wire zeroes `grace_period_duration` when the - * subscription isn't auto-renewing, so for those accounts coverage ends exactly when the - * renewal falls due and the loop below never runs a single iteration โ€” there is no renewal in - * flight to poll for, and the arithmetic already says so. + * No `auto_renewing` check needed: the wire zeroes `grace_period_duration` when the subscription + * is not auto-renewing, so coverage ends exactly when the renewal falls due and the loop runs no + * iterations at all. * - * Exempt from the freshness floor (spec ยง4: bounded polls carry their own cadence and their own - * termination). At 60s this poll sits exactly on the 60s floor, so leaving it floored would drop - * ticks to timing jitter alone. + * Exempt from the freshness floor โ€” see [GRACE_POLL_INTERVAL_MS] for why. */ private fun pollProStatusDuringGraceWhileOpen() { viewModelScope.launch { @@ -760,11 +750,8 @@ class ProSettingsViewModel @AssistedInject constructor( } /** - * The instant the renewal falls due. - * - * `expiry` IS the payment-due date. Coverage runs a further `gracePeriod` past it โ€” the backend's - * contract is "`expiry_ts` + `grace_period_duration` is exactly when we stop serving" โ€” so do not - * subtract to get this instant. + * The instant the renewal falls due. `expiry` IS that date โ€” do not subtract grace, which runs + * forward from it. */ private fun GetProStatusResponse.renewalDueAt(): Instant? = expiry @@ -1074,11 +1061,9 @@ class ProSettingsViewModel @AssistedInject constructor( * Cadence of the #4 grace poll. Shared cross-client contract (spec ยง9.3) โ€” the same 60s * Desktop and iOS use for their while-open poll; keep them in step. * - * **This equals `ProStatusRepository.MIN_UPDATE_INTERVAL_SECONDS`, and that equality is the - * reason #4 bypasses the freshness floor** rather than being floored like the other routine - * triggers. A poll running at exactly the floor would have roughly every other tick dropped - * by timing jitter alone, silently halving the rate. Two files apart the two constants look - * coincidentally equal; they are not, so change neither without the other. + * This equals `ProStatusRepository.MIN_UPDATE_INTERVAL_SECONDS`, and that equality is why #4 + * bypasses the floor: a poll running exactly at the floor loses every other tick to timing + * jitter. The two constants are not coincidentally equal โ€” change neither without the other. */ private const val GRACE_POLL_INTERVAL_MS = 60_000L } diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/FetchProStatusWorker.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/FetchProStatusWorker.kt index 9f480eed60..6d91a51b6e 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/FetchProStatusWorker.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/FetchProStatusWorker.kt @@ -89,27 +89,20 @@ class FetchProStatusWorker @AssistedInject constructor( configs.userProfile.removeProAccessExpiry() } - // Persist auto-renewing into synced config alongside E, so a linked device has the - // account state without its own fetch. Written unconditionally and deliberately so: - // libsession's set_nonzero_int short-circuits a no-change write on a clean config - // (`assign_if_changed`), exactly as the E write above already relies on, so a - // client-side "only if changed" guard would add nothing. A presence-based guard - // would be actively wrong โ€” the key is erased rather than stored when false, so - // presence flips on every transition and would read as churn that isn't there. + // A and G go into synced config beside E, so a linked device has the account state + // without its own fetch. All three must come from ONE response: coverage is read as + // `E + G` downstream, so an E stored without its G pairs with whatever G was already + // there. // - // No `t`/`T` bump either: this is backend-derived state like E and I, not a user - // profile edit, and libsession omits the bump for it on purpose. - configs.userProfile.setProAutoRenewing(details.autoRenewing) - - // And the grace period, from the SAME response as the expiry above. Everything - // downstream reads coverage as `E + G`, so an E written without its G pairs with - // whatever G happens to be sitting there โ€” which, before this, was whatever a proof - // outcome last left, or nothing at all. Writing E and A here but not G made the - // gate's arithmetic a no-op. + // Written unconditionally. `set_nonzero_int` short-circuits a no-change write on a + // clean config, so a client-side "only if changed" guard adds nothing, and a + // presence-based guard would be wrong โ€” the key is erased rather than stored when + // false, so presence flips on every transition. No `t`/`T` bump either: this is + // backend-derived state like E and I, not a user profile edit. // - // This is the ACCOUNT-level grace ("how much longer we serve past the expiry shown"), - // not `latestPayment.gracePeriod`, which reports what a store declared about one - // transaction. Same field name, different question. + // `details.gracePeriod` is the ACCOUNT-level field, not `latestPayment.gracePeriod`, + // which reports one store transaction and is not gated on auto-renewing. + configs.userProfile.setProAutoRenewing(details.autoRenewing) configs.userProfile.setProGracePeriod(details.gracePeriod) // Remove the pro config only when the backend authoritatively says we are no longer @@ -133,10 +126,6 @@ class FetchProStatusWorker @AssistedInject constructor( } proDatabase.updateProStatus(proStatus = details, updatedAt = snodeClock.currentTime()) - // Proof generation is NOT scheduled from here. It runs off libsession's - // `pro_renewal_target`, watched from config by - // ProStatusManager.manageProofRenewalScheduling, which the config writes above reach on - // their own โ€” so renewal does not depend on a status fetch having happened. Result.success() } catch (e: CancellationException) { Log.d(TAG, "Work cancelled") diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProDataMapper.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProDataMapper.kt index 00fb33f9f7..39f458c2e4 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProDataMapper.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProDataMapper.kt @@ -25,16 +25,14 @@ object ProUserStatus { /** * Map a libsession-parsed get-pro-status response to the app's [ProStatus] domain model. Needs a [Context] * to resolve the (client-owned) provider display strings. - */ -/** + * * @param confirmedAt when the fetch behind this response COMPLETED, or null if nothing has been - * confirmed. Used to gate [ProStatus.Active.AutoRenewing.inGracePeriod]: a snapshot taken before the - * renewal fell due cannot have observed it failing, so raising the "renewal unsuccessful" warning off - * one turns an ordinary boundary crossing into an alarm the backend never reported. + * confirmed. Gates [ProStatus.Active.AutoRenewing.inGracePeriod]: a snapshot taken before the renewal + * fell due cannot have observed it failing, so raising the "renewal unsuccessful" warning off one + * turns an ordinary boundary crossing into an alarm the backend never reported. * - * Gated **here**, where the flag is produced, rather than at each consumer. Five call sites read it - * today; a sixth would inherit no protection if the check lived in front of the value instead of - * inside it โ€” and there would be nothing to tell whoever added it. + * Applied here, where the flag is produced, rather than at each consumer โ€” so a new reader inherits + * the protection instead of having to know about it. */ fun GetProStatusResponse.toProStatus( nowMs: Long, @@ -45,19 +43,17 @@ fun GetProStatusResponse.toProStatus( return when (userStatus) { ProUserStatus.ACTIVE -> { val paymentItem = latestPayment ?: return ProStatus.NeverSubscribed - // `expiry` is the PAYMENT-DUE date โ€” the renewal is due then, and coverage runs a - // further `gracePeriod` past it. The backend states the contract directly: "`expiry_ts` + - // `grace_period_duration` is exactly when we stop serving", derived from the same instant - // the status is judged against (`server.py`, `account_coverage_end`). So being in this - // ACTIVE branch past `expiry` IS the grace period. + // `expiry` is the PAYMENT-DUE date and coverage runs a further `gracePeriod` past it: + // `expiry_ts + grace_period_duration` is when the backend stops serving, derived from the + // same instant it judged this status against. So being in the ACTIVE branch past `expiry` + // IS the grace period. // - // Two traps here, both of which have caught someone: - // * Do NOT subtract grace to get the renewal date. `expiry` already IS that date, and - // grace runs forward from it โ€” subtracting double-counts. - // * `gracePeriod` on THIS type is the account-level field โ€” "how much longer we serve" โ€” - // and is not the same quantity as `ProPaymentItem.gracePeriod`, which reports what a - // store declared about one transaction. They share a name and answer different - // questions. + // Two traps: + // * Do not subtract grace to get the renewal date. `expiry` already is that date and + // grace runs forward from it, so subtracting double-counts. + // * `gracePeriod` here is the ACCOUNT-level field โ€” how much longer we serve โ€” and not + // `ProPaymentItem.gracePeriod`, which reports what a store declared about one + // transaction and is not gated on auto-renewing. val renewingAt = expiry ?: return ProStatus.NeverSubscribed val renewingAtMs = renewingAt.toEpochMilli() val providerData = providerMetadata(paymentItem.paymentProvider, context) @@ -101,15 +97,10 @@ fun GetProStatusResponse.toProStatus( } ProUserStatus.EXPIRED -> ProStatus.Expired( - // `expiry` and `gracePeriod` are only meaningful as a PAIR, from one response. The backend - // derives the grace it sends from the same coverage-end instant it judged this status - // against, so together they describe one moment; separately they describe none. - // - // That is why both come off the response and neither is read from config. Config is not an - // equivalent source: not every status branch writes `G` there, and the branches that clear - // `E` cascade `G` away with it โ€” so a config read here would pair THIS response's expiry - // with a grace period from a different response, or with nothing at all, and the resulting - // coverage end would be short by however much they disagreed. + // Both values come off the response, and neither may be read from config: they are only + // meaningful as a PAIR from one response. Not every status branch writes `G` to config, and + // the branches that clear `E` cascade `G` away with it, so a config read would pair this + // response's expiry with a grace period from a different one. expiredAt = expiry ?: Instant.EPOCH, gracePeriod = gracePeriod, providerData = providerMetadata( diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt index 6cb8467bd6..51c73b012f 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt @@ -59,16 +59,14 @@ class ProProofGenerationWorker @AssistedInject constructor( "User must be logged to generate proof" } - // Ask libsession, from config, whether a proof is wanted at all โ€” the same question - // ProStatusManager asked to schedule us. It covers proof renewal, a purchase in flight - // (redemption, where get_pro_status is not ACTIVE yet and minting the proof is what pulls - // the entitlement through), and an entitlement held with no proof to attach. + // Whether a proof is wanted at all โ€” proof renewal, a purchase in flight (redemption, where + // get_pro_status is not ACTIVE yet and minting the proof is what pulls the entitlement + // through), or an entitlement held with no proof to attach. // // Ask CONFIG, never `proStatusRepository.loadState`. WorkManager persists this worker's - // schedule across process death; `loadState` does not survive it and restarts at `Init`. An - // in-memory status check therefore reads "not active, no purchase" and returns without - // renewing, for exactly the renewal that came due while the app was dead โ€” the case the - // persisted schedule exists to serve. + // schedule across process death and `loadState` does not, restarting at `Init` โ€” so an + // in-memory check reads "not active, no purchase" and returns without renewing, for exactly + // the renewal that came due while the app was dead. val now = snodeClock.currentTime() val (renewalTarget, purchasePending, proof) = configFactory.withUserConfigs { configs -> Triple( @@ -87,8 +85,7 @@ class ProProofGenerationWorker @AssistedInject constructor( // the new proof to config, ProStatusManager.manageProofRenewalScheduling re-reads the renewal // target off that write, and `target = proofExpiry - PRO_RENEWAL_LEAD` is permanently in the // past whenever a proof lives for less than the 60-minute lead โ€” so it reschedules us - // immediately, forever. (Before the status/proof split the loop ran through a forced - // get_pro_status instead; same shape, and the floor is what bounds it either way.) + // immediately, forever. // // Mirrors iOS `SessionProManager.reconcileProofRenewal` and Desktop `ducks/proBackendData.ts`, // constants included. Note it RE-ARMS rather than skipping: `target <= now` is the normal @@ -155,41 +152,31 @@ class ProProofGenerationWorker @AssistedInject constructor( // proof response, so the renewal path keeps E fresh without a separate get_pro_status. response.accountExpiry?.let { configs.userProfile.setProAccessExpiry(it.epochSecond) } - // Keep the renewing flag and the grace period coherent with the expiry above. - // Both must travel with it: everything derives coverage end as `E + G`, so a - // fresh E beside a G from an older response is wrong by the difference between - // them โ€” worst exactly where it matters, since Google reports its real - // multi-day grace only once the subscriber ENTERS grace, so a proof landing - // first would pair a new payment date with the ~1h stand-in. + // The renewing flag and the grace period must travel with the expiry above: + // coverage end is derived as `E + G`, so a fresh E beside a G from an older + // response is wrong by the difference between them. // - // โš ๏ธ These two are only meaningful HERE, inside the success branch, and nothing - // in the type system says so โ€” that is the hazard. + // Do not hoist these two out of the success branch. Their protection is + // PLACEMENT โ€” not the parse, and not the type. libsession's `parse_pro_proof` + // returns on the failure path before filling them, so every non-OK outcome + // leaves struct defaults of grace 0 and renewing false; the C struct has no + // presence flag and the Kotlin type is non-nullable, so a read outside this + // branch cannot tell that from a backend that said "not renewing". // - // libsession's `parse_pro_proof` returns on the failure path BEFORE it fills - // them, so on every non-OK outcome they hold struct defaults: grace 0, - // renewing false. The C struct carries no presence flag and the Kotlin type is - // non-nullable, so a read outside this branch gets `false` and cannot tell it - // from a backend that genuinely said "not renewing". - // - // That matters because writing `false` to a presence-only config key ERASES - // it. On `subscription_expired`/`not_subscribed`/`revoked` erasing is truthful; - // on a protocol error, a stale request or a transport failure it would wipe a - // flag `get_pro_status` had correctly learned, on the strength of a response - // that said nothing about the account. - // - // So the protection is PLACEMENT โ€” not the parse, and not the type. Do not - // hoist these two out of the success branch. + // Writing `false` to a presence-only config key ERASES it. On + // `subscription_expired`/`not_subscribed`/`revoked` erasing is truthful; on a + // protocol error or transport failure it would wipe a flag `get_pro_status` + // had correctly learned, on the strength of a response that said nothing. configs.userProfile.setProAutoRenewing(response.accountAutoRenewing) configs.userProfile.setProGracePeriod(response.accountGracePeriod) } Log.d(WORK_NAME, "Successfully generated a new pro proof expiring at ${Instant.ofEpochSecond(proof.expirySeconds)}") - // No status refresh is requested here. Minting the proof does flip the account - // active at the backend, so the display status does need re-reading โ€” but the - // `setProAccessExpiry` write above already fires the (E, prepaid) config-change - // trigger in ProStatusManager, which schedules exactly that fetch. Asking for - // one here as well made the proof loop a *source* of status fetches, which is - // the coupling this rework removes. + // No status refresh requested here, deliberately. Minting the proof does flip the + // account active at the backend, but the `setProAccessExpiry` write above already + // fires the (E, prepaid) config-change trigger, which schedules that fetch. + // Requesting one here as well would make the proof loop a SOURCE of status + // fetches, which is the coupling this design keeps out. Result.success() } diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProRefreshWindows.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProRefreshWindows.kt new file mode 100644 index 0000000000..0a290ff69c --- /dev/null +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProRefreshWindows.kt @@ -0,0 +1,25 @@ +package org.thoughtcrime.securesms.pro + +import java.time.Duration + +/** + * The time windows the Pro refresh and the home CTAs share. + * + * **Cross-client contract (spec ยง9.3).** Desktop and iOS use the same values; they move by agreement + * across clients or not at all, and a divergence needs saying why in the commit. + * + * These live here rather than inside the startup gate because the gate is not their only consumer. The + * gate decides whether to fetch on a cold start, and the home CTAs decide whether to show โ€” off the same + * two windows. Tuning one of a duplicated pair leaves the gate and the CTA disagreeing about the same + * instant, which is the one property they exist to share. + */ +object ProRefreshWindows { + /** Minimum spacing between startup-gate fetches, persisted across processes. */ + val STARTUP_MIN_INTERVAL: Duration = Duration.ofHours(24) + + /** How long before coverage lapses the Expiring CTA may fire. Anchored at the payment date. */ + val EXPIRING_CTA: Duration = Duration.ofDays(7) + + /** How long after coverage has ended the Expired CTA may fire. Anchored at coverage end. */ + val EXPIRED_CTA: Duration = Duration.ofDays(30) +} diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatus.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatus.kt index f64c5f174f..080f428215 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatus.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatus.kt @@ -66,16 +66,13 @@ sealed interface ProStatus{ val providerData: PaymentProviderMetadata ): ProStatus { /** - * When access actually ended, and the anchor for anything measuring how long ago that was. + * When access actually ended, and the anchor for any window measuring how long ago that was. * - * The backend only reports EXPIRED once coverage has ended, so measuring an "expired - * recently" window from [expiredAt] instead shortens it by exactly [gracePeriod] โ€” and - * empties it when the grace period is at least as long as the window. - * - * [gracePeriod] is multi-day for an Apple account whose dunning window ran out, because Apple - * states its retry window separately and it arrives here as grace. It is only the ~1h - * renewal-latency allowance on Play, which folds grace into the expiry it reports, and zero for - * an account that is not renewing at all โ€” for those two the anchors effectively coincide. + * The backend only reports EXPIRED once coverage has ended, so a window measured from + * [expiredAt] instead is short by exactly [gracePeriod], and empty once grace reaches the + * window length. [gracePeriod] is multi-day for an Apple account whose dunning ran out, since + * Apple states its retry window separately; on Play it is only the ~1h renewal-latency + * allowance, because Play folds grace into the expiry it reports. * * Derived here rather than at the consumer so a second reader cannot pick the other anchor. */ diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt index d70a265677..0971c2157f 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt @@ -126,18 +126,15 @@ class ProStatusManager @Inject constructor( DebugMenuViewModel.DebugProPlanStatus.LOADING -> State.Loading DebugMenuViewModel.DebugProPlanStatus.ERROR -> State.Error(Exception()) else -> { - // The real refresh state. `Success` means THIS PROCESS has had a fetch confirmed - // by the backend โ€” nothing weaker. Consumers gate on it to avoid acting on stale - // data (`HomeViewModel`'s Expired CTA above all), so anything else must not - // report success. + // `Success` means THIS PROCESS has had a fetch confirmed by the backend, nothing + // weaker: consumers gate on it to avoid acting on stale data, `HomeViewModel`'s + // Expired CTA above all. // - // Exhaustive on purpose โ€” no `else`, and do not add one. Two states are not - // successes and a catch-all sweeps up both: `Init`, where nothing has been asked - // yet, and a `Loaded` restored from WorkManager's PERSISTED work state, which is - // a fetch some EARLIER process made. The second is the dangerous one โ€” a renewal - // landing while the app is closed leaves a stale cache that reads as confirmed, - // and the home Expired CTA fires off it. Listing every case forces the next state - // added here to declare which it is. + // Exhaustive on purpose โ€” do not add an `else`. It would sweep up two states that + // are not successes: `Init`, where nothing has been asked yet, and a `Loaded` + // restored from WorkManager's PERSISTED work state, i.e. a fetch from an EARLIER + // process. The second is the dangerous one โ€” a renewal landing while the app is + // closed leaves a stale cache reading as confirmed, and the CTA fires off it. when(proStatusState){ is ProStatusRepository.LoadState.Loading -> { if(proStatusState.waitingForNetwork) State.Error(Exception()) @@ -372,58 +369,32 @@ class ProStatusManager @Inject constructor( } .distinctUntilChanged() .transformLatest { (renewalDue, coverageEnd) -> - // TWO wakes, and the second is not redundant with the first. + // Two wakes: the renewal date (did the charge land?) and coverage end (did grace + // run out?). The second is not redundant, because the proof loop only covers it + // when `E` MOVES โ€” a successful renewal advances `E` and fires the config-change + // trigger, a failed one leaves `E` alone and fires nothing. The failed branch is + // the one the grace warning exists for. // - // 1. renewal due โ€” did the charge succeed or fail? - // 2. coverage end โ€” did grace run out without recovery? - // - // (2) looks covered by the proof loop, and isn't. A proof attempt near the end - // of coverage writes config, and that write fires the config-change trigger. But - // that chain needs `E` to MOVE: - // - // renewal succeeded -> E advances -> config change -> refresh โœ… - // renewal failed -> E unchanged -> no change -> nothing โŒ - // - // The uncovered branch is the one the grace warning exists for. - // - // No wake handles to leak here: `transformLatest` cancels this whole body when - // `E` moves, so a second wake armed against a superseded expiry cannot outlive - // it. A scheduler holding ids would need a COLLECTION for two instants โ€” holding - // one and scheduling two orphans a timer every period, and both wakes still fire - // correctly while it accumulates. - if (snodeClock.delayUntil(renewalDue.plusSeconds(30))) { - emit("30 seconds after the renewal fell due") + // `transformLatest` cancels this body when `E` moves, so a wake armed against a + // superseded expiry cannot outlive it and there are no handles to track. + if (snodeClock.delayUntil(renewalDue.plus(WAKE_SLACK))) { + emit("$WAKE_SLACK after the renewal fell due") } - // Guarded on the two instants COINCIDING rather than on grace being zero: same - // condition today, but it says what actually matters, so it survives a change to - // how the instants are derived. + // Guarded on the two instants coinciding rather than on grace being zero: the same + // condition today, but it survives a change to how the instants are derived. // - // โš ๏ธ If you are here because this wake "didn't fire" on a QA backend: it fired. - // The FETCH was dropped. Both emits go through the floored path - // (`requestRefresh()`, not `immediate`), so when grace is shorter than - // `MIN_UPDATE_INTERVAL_SECONDS` = 60s the second wake lands inside the floor that - // the first one just armed. The symptom is indistinguishable from the wake never - // having been scheduled, which is why this comment exists. - // - // Only reachable where grace is shorter than the floor, which production grace - // never is: while a renewal is still going to be attempted the backend always adds - // a ~1h renewal-latency allowance on top of any window the store stated. - // Compressed QA backends do set grace to seconds, which is where this shows up. - // - // Left alone deliberately (ruled 2026-08-10): the escape hatch is an env-var - // override of the floor, owned by the Pro UI-test work. Do not make this wake - // `immediate` โ€” a scheduled trigger bypassing the floor is what `force` -> - // `immediate` was introduced to stop. - if (coverageEnd != renewalDue && snodeClock.delayUntil(coverageEnd.plusSeconds(30))) { - emit("30 seconds after coverage ended") + // If this wake looks like it never fired on a QA backend, it fired and the FETCH + // was dropped โ€” see [ProStatusRepository.MIN_UPDATE_INTERVAL_SECONDS], which owns + // that interaction. Do not make this wake `immediate` to work around it. + if (coverageEnd != renewalDue && snodeClock.delayUntil(coverageEnd.plus(WAKE_SLACK))) { + emit("$WAKE_SLACK after coverage ended") } }, - // NOTE: there is deliberately no trigger keyed to PROOF expiry here. Proof timing - // drives proof renewal (see manageProofRenewalScheduling) and nothing else; a status - // fetch scheduled off the proof's clock coupled the two loops together, so a proof - // that renewed early or late dragged the status fetch with it. + // No trigger keyed to PROOF expiry, deliberately: proof timing drives proof renewal + // (see manageProofRenewalScheduling) and nothing else. A status fetch on the proof's clock + // couples the two loops, so a proof renewing early or late drags the status fetch with it. startupGate() ).debounce(500.milliseconds) @@ -443,22 +414,20 @@ class ProStatusManager @Inject constructor( * Trigger #1 โ€” the startup fetch, gated. * * An ungated cold-start fetch has no consumer for most users: entitlement runs off the proof, the - * settings screen refreshes when opened, and account-expiry awareness is the `E+30s` wake. The - * only real consumer is the home CTAs โ€” so the gate asks whether a CTA could plausibly fire, from - * synced config alone, and otherwise stays off the network entirely. Users who never subscribed - * and users comfortably paid up therefore make no request at all, which matters because "cold - * start" is something mobile does constantly. + * settings screen refreshes when opened, and account-expiry awareness is the wake. The only real + * consumer is the home CTAs, so the gate asks whether one could plausibly fire, from synced config + * alone. Never-subscribed and comfortably-paid-up accounts make no request at all. * - * Two independent brakes: the CTA-worthiness test below, and a persisted 24h minimum between - * startup fetches. The interval has its own key โ€” a routine refresh must not consume the gate's - * budget, and a startup fetch from twenty hours ago must not satisfy the 60s floor. + * Two independent brakes: the CTA-worthiness test, and a persisted 24h minimum. The interval needs + * its own key โ€” a routine refresh must not consume the gate's budget, and a startup fetch from + * twenty hours ago must not satisfy the 60s floor. */ private fun startupGate(): Flow = flow { val now = snodeClock.currentTime() val lastStartupFetch = proDatabase.getProStatusLastStartupFetchAttemptAt() - if (lastStartupFetch != null && lastStartupFetch.plus(STARTUP_MIN_INTERVAL).isAfter(now)) { - Log.d(DebugLogGroup.PRO_SUBSCRIPTION.label, "Startup gate: fetched within the last $STARTUP_MIN_INTERVAL, skipping") + if (lastStartupFetch != null && lastStartupFetch.plus(ProRefreshWindows.STARTUP_MIN_INTERVAL).isAfter(now)) { + Log.d(DebugLogGroup.PRO_SUBSCRIPTION.label, "Startup gate: fetched within the last ${ProRefreshWindows.STARTUP_MIN_INTERVAL}, skipping") return@flow } @@ -485,14 +454,13 @@ class ProStatusManager @Inject constructor( /** * Drives proof acquisition/renewal purely from config, off libsession's `pro_renewal_target`. * - * The inputs libsession needs for `pro_renewal_target` โ€” the stored proof, the access expiry (E) - * and the prepaid marker (I) โ€” all live in the user profile, so watching them directly is - * sufficient. Do not drive this off a `get_pro_status` response instead: that makes the proof - * loop a downstream effect of a display fetch, so no fetch means no renewal. + * The inputs to `pro_renewal_target` โ€” the stored proof, the access expiry (E) and the prepaid + * marker (I) โ€” all live in the user profile, so watching them directly is sufficient. Do not drive + * this off a `get_pro_status` response: that makes the proof loop a downstream effect of a display + * fetch, so no fetch means no renewal. * - * The loop closes without a status fetch anywhere in it: the proof worker's own config writes - * (a new proof, a refreshed or cleared E) re-enter here and schedule the next attempt, and a - * `null` target โ€” no proof and no entitlement signalled โ€” cancels the work outright. + * The loop closes with no status fetch in it. The proof worker's own config writes re-enter here + * and schedule the next attempt; a `null` target cancels the work outright. */ @OptIn(FlowPreview::class) private suspend fun manageProofRenewalScheduling() { @@ -558,22 +526,20 @@ class ProStatusManager @Inject constructor( } } - // Ask the server what the state actually is now. Clearing the proof leaves the account - // asserting a future access expiry with nothing to back it, and NOTHING ELSE HERE WILL - // CORRECT THAT: the config-change trigger watches `E` and the prepaid marker, and this - // path deliberately touches neither. Without this the user sits on a stale expiry with - // no proof until some unrelated trigger happens to fire. + // Ask the server what the state is now. Clearing the proof leaves the account + // asserting a future access expiry with nothing behind it, and nothing else corrects + // that: the config-change trigger watches `E` and the prepaid marker, and this path + // touches neither, so the stale expiry stands until some unrelated trigger fires. // - // `E` is deliberately NOT cleared locally. A revocation says this proof is void, not - // what the subscription is now โ€” the account may be fine and re-provable, or genuinely - // gone. Deciding that here would be the client overruling the only party that knows. + // `E` is not cleared locally. A revocation says this proof is void, not what the + // subscription is now โ€” the account may be fine and re-provable, or genuinely gone, + // and only the server knows which. // - // Floored, not `immediate`: nobody is waiting on a screen. `immediate` is for the - // post-purchase poll and manual/recover. + // Floored, not `immediate`: nobody is waiting on a screen. // - // Outside the mutation block on purpose. It must also be gated on the clear actually - // happening โ€” this collector can fire for a hash that is no longer the stored proof, - // and a refresh for someone else's revocation is a request with no reason. + // Requested outside the mutation block, and gated on the clear having happened: this + // collector can fire for a hash that is no longer the stored proof, and a refresh + // triggered by someone else's revocation has no reason behind it. if (cleared) { proStatusRepository.get().requestRefresh() } @@ -714,25 +680,18 @@ class ProStatusManager @Inject constructor( companion object { /** - * Startup-gate constants. **Shared cross-client contract** (spec ยง9.3) โ€” Desktop and iOS use - * the same values; keep them in step, and say why in the commit if they ever diverge. + * How long after a wake instant to fetch. The backend judges against its own clock, so a wake + * landing exactly on the boundary can read the pre-crossing state. */ - private val STARTUP_MIN_INTERVAL: Duration = Duration.ofHours(24) - private val EXPIRING_CTA_WINDOW: Duration = Duration.ofDays(7) - private val EXPIRED_CTA_WINDOW: Duration = Duration.ofDays(30) + private val WAKE_SLACK: Duration = Duration.ofSeconds(30) /** * Whether a cold start should fetch `get_pro_status`, and why โ€” or null to stay off the * network. Pure over plain values so it can be tested without a clock, a database or config. * - * [renewalDue] is the access expiry as the backend sends it โ€” the **payment-due date**. Coverage - * runs a further [grace] past it, so the grace window is `[renewalDue, renewalDue + grace)`. - * The backend states the contract as "`expiry_ts` + `grace_period_duration` is exactly when we - * stop serving". - * - * The decision is four rows over config state. Note that none of them tests - * `renewalDue + grace <= now`: coverage end is not what a cold start needs to know, and keying - * any row to it double-counts grace against the payment date the rows already turn on. + * [renewalDue] is the access expiry as the backend sends it โ€” the payment-due date. Coverage + * runs a further [grace] past it: `expiry + grace_period_duration` is when the backend stops + * serving. * * | config state | action | * |----------------------------------------------------|-----------------------------------| @@ -741,9 +700,8 @@ class ProStatusManager @Inject constructor( * | `!auto_renewing && renewalDue` in the CTA window | fetch โ€” the Expiring CTA may fire | * | `!auto_renewing && now >= renewalDue` | confirm before the Expired CTA | * - * Row 2 is what surfaces grace: past the payment-due date while still covered is exactly the - * state the grace warning exists for, and it is reachable because coverage extends to - * `renewalDue + grace`. It is also the only row [grace] enters โ€” see the comment there. + * No row tests `renewalDue + grace <= now`: a cold start does not need to know when coverage + * ends, and testing it would double-count grace against the payment date the rows turn on. */ internal fun startupFetchReason( renewalDue: Instant?, @@ -759,27 +717,24 @@ class ProStatusManager @Inject constructor( return when { autoRenewing && !overdue -> null - // Past the payment date while auto-renewing: either the charge is retrying (grace - // is running) or it ultimately failed and coverage has since ended. Both want a - // fetch โ€” the first to raise the grace warning, the second to confirm before the - // Expired CTA. + // Past the payment date while auto-renewing: the charge is either still retrying or + // it failed and coverage has since ended. Both want a fetch โ€” to raise the grace + // warning, or to confirm before the Expired CTA. // - // Bounded from COVERAGE end, not from the payment date, and this is the one place - // `grace` does any work. Every other row here needs only "is the renewal overdue", - // which the payment date answers on its own โ€” grace has no part in them. Keeping - // this bound measured from `renewalDue + grace` means an account - // still inside a multi-day grace is never mistaken for a long-dead one, and an - // account dead for a year stops fetching on every cold start. + // The bound is measured from COVERAGE end, which is the only place [grace] does any + // work here. Anchored at the payment date instead, an account still inside a multi-day + // grace reads as long-dead; unbounded, an account whose renewing flag was never + // cleared fetches on every cold start forever. autoRenewing -> - if (renewalDue.plus(grace).plus(EXPIRED_CTA_WINDOW).isAfter(now)) { + if (renewalDue.plus(grace).plus(ProRefreshWindows.EXPIRED_CTA).isAfter(now)) { "auto-renewing and past the payment date; grace or a failed renewal" } else { null } !overdue -> - if (renewalDue.isBefore(now.plus(EXPIRING_CTA_WINDOW))) { - "not auto-renewing and expiring within $EXPIRING_CTA_WINDOW" + if (renewalDue.isBefore(now.plus(ProRefreshWindows.EXPIRING_CTA))) { + "not auto-renewing and expiring within ${ProRefreshWindows.EXPIRING_CTA}" } else { // Comfortably active and not renewing โ€” a prepaid or long non-renewing // subscription. No CTA can fire, so nothing to fetch for. @@ -790,8 +745,8 @@ class ProStatusManager @Inject constructor( // showing the Expired CTA: config can read expired while a renewal landed on another // device and hasn't synced. Bounded by the CTA's own window โ€” once the CTA can no // longer fire there is nothing for the fetch to serve. - renewalDue.plus(EXPIRED_CTA_WINDOW).isAfter(now) -> - "not auto-renewing and expired within $EXPIRED_CTA_WINDOW; confirming before the Expired CTA" + renewalDue.plus(ProRefreshWindows.EXPIRED_CTA).isAfter(now) -> + "not auto-renewing and expired within ${ProRefreshWindows.EXPIRED_CTA}; confirming before the Expired CTA" else -> null } diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusRepository.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusRepository.kt index 67a9383f89..01d815fb39 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusRepository.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusRepository.kt @@ -111,53 +111,30 @@ class ProStatusRepository @Inject constructor( * Requests a refresh of the current user's Pro status. By default the request is dropped when * the last successful fetch is recent enough. * - * [immediate] bypasses that floor. It is ONE mechanism with a closed list of sanctioned - * callers โ€” adding a fourth is a cross-client decision, not a local one: + * [immediate] bypasses that floor, and its caller list is closed โ€” adding a fourth is a + * cross-client decision: * - * - **#5 manual refresh / recover** (`ProSettingsViewModel`) โ€” the user is watching. - * - **#7 post-purchase poll** (`ProStatusManager.pollProStatusAfterPurchase`) โ€” bounded, and - * the user is waiting on the entitlement. - * - **#4 while-open grace poll** (`ProSettingsViewModel`) โ€” bounded and self-terminating. It - * bypasses for a mechanical reason rather than an urgency one: its cadence - * (`GRACE_POLL_INTERVAL_MS`) is *exactly* [MIN_UPDATE_INTERVAL_SECONDS], so leaving it - * floored would drop roughly every other tick to timing jitter and silently halve the poll - * rate. + * - #5 manual refresh / recover (`ProSettingsViewModel`) โ€” the user is watching. + * - #7 post-purchase poll (`ProStatusManager`) โ€” bounded, the user is waiting on the entitlement. + * - #4 while-open grace poll (`ProSettingsViewModel`) โ€” bounded, and mechanical rather than + * urgent: its cadence is exactly [MIN_UPDATE_INTERVAL_SECONDS], so leaving it floored would + * drop every other tick to timing jitter. * - * Everything else โ€” startup, config-change, the `E+30s` wake, on-enter โ€” goes through the - * floor. That is what stops several triggers coinciding (a config change and a timer, say) - * from each costing a fetch. + * Everything else goes through the floor, which is what stops coinciding triggers each costing a + * fetch. */ /** - * Whether THIS process has asked for a status fetch yet. Deliberately in-memory and deliberately - * not the same thing as the persisted timestamp: see [shouldFetch]. + * Whether THIS process has asked for a status fetch yet โ€” in memory, and not the question the + * persisted timestamp answers. That one is "was the last fetch recent?"; this one is "has this + * process asked at all?". * - * โš ๏ธ **LOAD-BEARING โ€” do not delete this as redundant with the persisted timestamp.** It looks - * redundant, and it is not: the persisted value answers *"was the last fetch recent?"*, this - * answers *"has this process asked at all?"*. + * Do not delete as redundant with the timestamp. The floor is the only refusal that survives a + * restart, because it reads a value that outlived the process that wrote it, so this is what + * guarantees a process's first request reaches the network. Without it the Pro settings screen's + * on-enter refresh is refused on a relaunch inside the interval, leaving the screen with no + * confirmed status to render and nothing left that would ask: it spins until the interval expires. * - * Its job is to guarantee the FIRST request of a process reaches the network. Precisely: **this is - * what stops the floor becoming a mutex.** - * - * Three separate things can refuse to start a status fetch, and they are easy to conflate: - * - * 1. **in flight** โ€” a request is already running, - * 2. **unconfirmed** โ€” we have no confirmed status (now [LoadState.Loaded.confirmedInThisProcess]), - * 3. **too soon** โ€” this floor. - * - * Only (3) is **persisted**, so it is the only one that can refuse in a process that has never - * fetched at all: it reads a timestamp that outlived the process that wrote it. That is the same - * shape as the `Loaded(stale)` bug โ€” durable state answering a per-process question โ€” arriving - * through a different mechanism. Without this exemption, a relaunch inside the interval would be - * refused by a decision no part of this process ever took, and after the startup gate nothing - * else would ask. - * - * Note that (2) being handled properly by [LoadState.Loaded.confirmedInThisProcess] does **not** - * make this removable โ€” the two guard different refusals, and only this one is reachable in a - * process that has not fetched. - * - * What deleting it costs, concretely: the Pro settings screen refreshes on entry through the - * floored path, so on a relaunch inside the interval that refresh is refused, the screen has no - * confirmed status to render, and nothing remaining will ask. It spins until the interval expires. + * Cold-start load stays bounded by the 24h startup gate. */ @Volatile private var fetchedInThisProcess = false @@ -205,26 +182,15 @@ class ProStatusRepository @Inject constructor( companion object { /** - * The status freshness floor. **Shared cross-client contract** โ€” Desktop and iOS use the - * same 60s (`SessionPro.StatusRefresh.floorSeconds`, `STATUS_FLOOR_MS`); keep them in step, - * and say why in the commit if they ever have to diverge. - * - * โš ๏ธ **A scheduled wake depends on this not being crossed.** `ProStatusManager`'s - * `user_expiry` trigger arms two wakes โ€” at the renewal date and at coverage end โ€” and both - * reach the network through the FLOORED path here, not through `immediate`. When the grace - * period is shorter than this floor the two wakes land inside it and **the second one's fetch - * is dropped.** - * - * In production that cannot happen: while a renewal is still going to be attempted the backend - * always adds a ~1h renewal-latency allowance, on top of any window the store stated. It - * happens on - * **compressed QA backends**, which set grace to seconds so the window can be exercised in a - * test run. + * The status freshness floor. Shared cross-client contract: Desktop and iOS use the same 60s, + * and it moves by agreement across clients or not at all. * - * Deliberately not worked around here (ruled 2026-08-10). If UI-test work needs to exercise - * the coverage-end wake, the sanctioned escape hatch is an **env-var override of this - * constant**, owned by that work โ€” not an `immediate` fetch for a scheduled trigger, which - * would reopen exactly what `force` -> `immediate` closed. + * A scheduled wake depends on this not being crossed. `ProStatusManager`'s `user_expiry` + * trigger arms two wakes, at the renewal date and at coverage end, and both reach the network + * through the floored path. A grace period shorter than this floor puts both inside it and the + * second one's fetch is dropped โ€” reachable only on compressed QA backends, since production + * grace always includes a ~1h renewal-latency allowance. To exercise the coverage-end wake, + * override this constant; do not make a scheduled trigger `immediate`. */ const val MIN_UPDATE_INTERVAL_SECONDS = 60L @@ -232,25 +198,15 @@ class ProStatusRepository @Inject constructor( * The whole of the floor decision, over plain values so it can be tested without a * database, a clock or WorkManager. * - * Expressed over the **timestamp**, never over a load-state enum. That distinction is the - * bug this replaced: the old check asked whether the in-memory state was `Loading`/`Loaded`, - * and the state a cold start begins in โ€” `Init` โ€” is neither, so the floor was skipped - * outright on exactly the path it exists to cover. An absent timestamp means "no successful - * fetch on record", which is a genuine reason to fetch; an initial enum value is not. + * Keyed off the timestamp, never off a load-state enum: an absent timestamp means "no + * successful fetch on record", which is a reason to fetch, whereas a cold start's initial enum + * value is neither `Loading` nor `Loaded` and so falls outside any test against those two. * - * Drop-on-fresh, not re-arm (spec ยง4): a caller whose request is dropped here does not get - * a later one scheduled on its behalf. The proof loop is deliberately the opposite. + * Drop-on-fresh, not re-arm (spec ยง4): a caller whose request is dropped here does not get a + * later one scheduled on its behalf. The proof loop is deliberately the opposite. * - * [fetchedInThisProcess] is the second exemption, and it needs the persisted timestamp to - * exist before it makes sense โ€” the two are not redundant. A process that has never fetched - * always fetches once, however fresh the stored value is, because several things downstream - * key off *this process* having confirmed the status rather than off the status being - * recent. On Android that is the false-expired protection: the home Expired CTA is gated on - * `refreshState is State.Success` (`HomeViewModel`), and `Init` maps to `Success` โ€” so what - * actually suppressed the CTA until confirmation was the `Loading` transition a real fetch - * produces. Floor the very first request of a process and that transition never happens, - * and the CTA fires off whatever the cache last held. Cold-start load stays bounded by the - * 24h startup gate, which is the stronger limit anyway. + * [fetchedInThisProcess] is the second exemption โ€” see its own doc for why the two are not + * redundant. */ fun shouldFetch( immediate: Boolean, diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/db/ProDatabase.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/db/ProDatabase.kt index cace7aae34..7ca3c4a4da 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/db/ProDatabase.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/db/ProDatabase.kt @@ -188,11 +188,9 @@ class ProDatabase @Inject constructor( /** * When a `get_pro_status` fetch was last **attempted**, successful or not. * - * Deliberately separate from [getProStatusAndLastUpdated]'s timestamp, which cannot serve this - * purpose: that value is written as a pair with the response blob and is only readable when both - * are present, so a failed fetch has nothing to record there. Using it as the freshness floor - * therefore meant a failing network was never throttled at all โ€” every trigger and every cold - * launch re-attempted, which is the load the floor exists to prevent. + * Separate from [getProStatusAndLastUpdated]'s timestamp, which cannot serve this purpose: that + * value is written as a pair with the response blob and is unreadable without it, so a failed fetch + * records nothing and a failing network goes unthrottled entirely. */ fun getProStatusLastAttemptAt(): Instant? { return readableDatabase.query( @@ -204,10 +202,9 @@ class ProDatabase @Inject constructor( } /** - * When the STARTUP GATE last attempted a fetch. Separate from [getProStatusLastAttemptAt] on - * purpose: the gate's 24h interval must not be consumed or reset by a routine refresh, and the - * 60s floor must not be satisfied by a startup fetch from twenty hours ago. One value cannot - * answer both questions. + * When the STARTUP GATE last attempted a fetch. Separate from [getProStatusLastAttemptAt]: the + * gate's 24h interval must not be consumed by a routine refresh, and the 60s floor must not be + * satisfied by a startup fetch from twenty hours ago. */ fun getProStatusLastStartupFetchAttemptAt(): Instant? { return readableDatabase.query( @@ -273,12 +270,11 @@ class ProDatabase @Inject constructor( private const val STATE_PRO_STATUS = "pro_status" private const val STATE_PRO_STATUS_UPDATED_AT = "pro_status_updated_at" - // Written on every fetch ATTEMPT, unlike STATE_PRO_STATUS_UPDATED_AT which is written only - // alongside a successful response. No migration needed: pro_state is a name/value table. + // Written on every ATTEMPT, unlike STATE_PRO_STATUS_UPDATED_AT which is written only alongside + // a successful response. No migration needed: pro_state is a name/value table. private const val STATE_PRO_STATUS_LAST_ATTEMPT_AT = "pro_status_last_attempt_at" - // The startup gate's own 24h interval. Attempt-stamped like the above, and deliberately a - // SEPARATE key โ€” see getProStatusLastStartupFetchAttemptAt. + // The startup gate's 24h interval โ€” a separate key, see getProStatusLastStartupFetchAttemptAt. private const val STATE_PRO_STATUS_LAST_STARTUP_FETCH_ATTEMPT_AT = "pro_status_last_startup_fetch_attempt_at" diff --git a/app/src/test/java/org/thoughtcrime/securesms/pro/ProExpiredCoverageEndTest.kt b/app/src/test/java/org/thoughtcrime/securesms/pro/ProExpiredCoverageEndTest.kt index 09fcf1453c..647cc304e9 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/pro/ProExpiredCoverageEndTest.kt +++ b/app/src/test/java/org/thoughtcrime/securesms/pro/ProExpiredCoverageEndTest.kt @@ -14,13 +14,12 @@ import java.time.Instant * payment-due date it sends. So a window measured from the payment date is short by exactly the grace * period, and empty once grace reaches the window length. * - * A multi-day grace on the wire means an Apple account whose dunning window ran out: Apple states its - * retry window separately, so it arrives as grace. Play folds grace into the expiry it reports, so a - * Play account's grace is only the ~1h renewal-latency allowance and the two anchors all but coincide. - * The cases below therefore sweep grace as a parameter rather than asserting one store's number. + * A multi-day grace means an Apple account whose dunning ran out, since Apple states its retry window + * separately; on Play it is only the ~1h latency allowance. The cases sweep grace as a parameter rather + * than asserting either store's number. * - * These pin the anchor. The CTA condition itself lives in `HomeViewModel`, which needs Android; what - * is testable here is the instant it keys off, and that is the part that was wrong. + * The CTA condition itself lives in `HomeViewModel`, which needs Android. What is testable here is the + * instant it keys off. */ class ProExpiredCoverageEndTest { @@ -47,15 +46,13 @@ class ProExpiredCoverageEndTest { @Test fun `zero grace leaves the payment date as coverage end`() { - // The wire sends grace = 0 for an account that is not renewing, so the two anchors coincide and - // this fix is a no-op for those accounts rather than a change. + // Grace is 0 when not renewing, so the anchors coincide and this is a no-op for those accounts. assertEquals(paymentDue, expired(Duration.ZERO).coverageEndedAt) } @Test fun `the window is a full 30 days of coverage having ended, whatever grace was`() { - // The property that matters: window length is independent of grace. A 16-day store grace used to - // cost 16 of the 30 days. + // The property: window length is independent of grace. for (graceDays in listOf(0L, 1L, 16L, 29L, 45L)) { val grace = Duration.ofDays(graceDays) val coverageEnd = paymentDue.plus(grace) @@ -76,9 +73,8 @@ class ProExpiredCoverageEndTest { @Test fun `a grace period as long as the window does not empty it`() { - // The reachable failure of the payment-date anchor, not a corner case: with 30 days of grace, - // `now` is already past `paymentDue + 30d` the moment EXPIRED can first be reported, so that - // anchor yields no window at all and the CTA could never fire. + // With 30 days of grace, `now` is already past `paymentDue + 30d` the moment EXPIRED can first + // be reported, so the payment-date anchor yields no window at all. val grace = ctaWindow val firstMomentExpiredIsReportable = paymentDue.plus(grace).plusSeconds(1) diff --git a/app/src/test/java/org/thoughtcrime/securesms/pro/ProStartupGateTest.kt b/app/src/test/java/org/thoughtcrime/securesms/pro/ProStartupGateTest.kt index 36ab5eb80c..b8b8eb00ac 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/pro/ProStartupGateTest.kt +++ b/app/src/test/java/org/thoughtcrime/securesms/pro/ProStartupGateTest.kt @@ -13,24 +13,17 @@ import java.time.Instant * Scope: the decision only. These do NOT cover the persisted 24h interval or the config read โ€” both * need a database, and the interval is checked before this function is reached. * - * The model every case below turns on: **the access expiry the backend sends IS the payment-due date**, - * and coverage runs a further grace period past it โ€” the backend's own words are "`expiry_ts` + - * `grace_period_duration` is exactly when we stop serving". So `expiry` needs no adjustment to get the - * renewal date, and the grace window is `[expiry, expiry + grace)`. - * - * If you are here because you expected a subtraction: there isn't one, and adding it double-counts. - * Grace runs FORWARD from the expiry. `the auto-renewing bound is measured from coverage end, not the - * payment date` is the case that pins the direction. + * The model every case turns on: the access expiry the backend sends IS the payment-due date, and + * coverage runs a further grace period past it โ€” `expiry_ts + grace_period_duration` is when it stops + * serving. So the grace window is `[expiry, expiry + grace)` and there is no subtraction anywhere; + * adding one double-counts. `the auto-renewing bound is measured from coverage end, not the payment + * date` pins the direction. */ class ProStartupGateTest { private val now: Instant = Instant.parse("2026-08-07T00:00:00Z") - /** - * A realistic multi-day grace period, which on the wire means an Apple account mid-dunning: Apple - * states its retry window separately, so it arrives as grace. Play folds its grace into the expiry - * instead, so a Play account's grace is just the ~1h renewal-latency allowance. - */ + /** Multi-day grace means an Apple account mid-dunning; Play's is only the ~1h latency allowance. */ private val grace: Duration = Duration.ofDays(14) private val noGrace: Duration = Duration.ZERO @@ -41,8 +34,7 @@ class ProStartupGateTest { @Test fun `no access expiry means no fetch`() { - // The population the gate exists for: users who have never subscribed were fetching on every - // cold start and could never see a CTA. + // The population the gate exists for: no CTA can fire, so no fetch has a consumer. assertNull(startupFetchReason(null, autoRenewing = false, grace = noGrace, now = now)) assertNull(startupFetchReason(null, autoRenewing = true, grace = grace, now = now)) } @@ -51,14 +43,12 @@ class ProStartupGateTest { @Test fun `auto-renewing and comfortably active does not fetch`() { - // The renewal is 20 days out, so nothing can have gone wrong with it yet. assertNull(startupFetchReason(inDays(20), autoRenewing = true, grace = grace, now = now)) } @Test fun `auto-renewing and inside the grace window DOES fetch`() { - // The renewal fell due 7 days ago and grace runs 14, so coverage is still live but the charge - // has not landed. This is the state the whole rework exists to surface. + // Renewal due 7 days ago, grace 14: still covered, charge not landed. The state this exists for. assertNotNull(startupFetchReason(daysAgo(7), autoRenewing = true, grace = grace, now = now)) } @@ -69,36 +59,30 @@ class ProStartupGateTest { @Test fun `auto-renewing boundary - one second before the renewal date does not fetch`() { - // The negative control for the pair above. Without it, "inside grace fetches" also passes - // against a gate that fetches unconditionally whenever auto-renewing. + // Negative control: without it, "inside grace fetches" also passes against a gate that always + // fetches when auto-renewing. assertNull(startupFetchReason(now.plusSeconds(1), autoRenewing = true, grace = grace, now = now)) } @Test fun `auto-renewing past coverage end still fetches`() { - // Renewal due 20 days ago, grace 14, so coverage ended 6 days ago: the renewal ultimately - // failed. Still worth a fetch โ€” this is the account about to be shown the Expired CTA, and - // config alone must never be the basis for that. + // Coverage ended 6 days ago: the renewal failed. Still fetches โ€” this account is about to be + // shown the Expired CTA, and config alone must never be the basis for that. assertNotNull(startupFetchReason(daysAgo(20), autoRenewing = true, grace = grace, now = now)) } @Test fun `auto-renewing and long dead does not fetch`() { - // Renewal due 60 days ago, coverage ended 46 days ago, past the Expired CTA window. Without - // this bound an account whose renewing flag was never cleared fetches on every cold start - // forever. + // Coverage ended 46 days ago, past the CTA window. Unbounded, an account whose renewing flag + // was never cleared fetches on every cold start forever. assertNull(startupFetchReason(daysAgo(60), autoRenewing = true, grace = grace, now = now)) } @Test fun `the auto-renewing bound is measured from coverage end, not the payment date`() { - // 40 days past the payment date with 14 days of grace: coverage ended 26 days ago, so the - // Expired CTA can still fire and this must fetch. - // - // The discriminating case for where that bound is anchored. Measured from the payment date the - // account reads as 40 days gone โ€” past the 30 day window โ€” and this returns null. Only the - // coverage-end anchor gets it right, and only a grace period longer than the gap between the - // two anchors can tell them apart, which is why this uses the multi-day Google value. + // Coverage ended 26 days ago, so the CTA can still fire. Discriminates the anchor: from the + // payment date this reads as 40 days gone, past the window, and returns null. Only a grace + // longer than the gap between the anchors tells them apart, hence the multi-day value. assertNotNull(startupFetchReason(daysAgo(40), autoRenewing = true, grace = grace, now = now)) } @@ -129,10 +113,9 @@ class ProStartupGateTest { @Test fun `not auto-renewing, active, outside the CTA window does not fetch`() { - // A prepaid or long non-renewing subscription. No CTA can fire, so there is nothing to fetch - // for. Note this is now unambiguous: the proof-success path writes the renewing flag beside - // the expiry it sets, so an absent flag genuinely means not-renewing rather than "never - // recorded". + // A prepaid or long non-renewing subscription: no CTA can fire. Unambiguous because the + // proof-success path writes the renewing flag beside the expiry, so absent means not-renewing + // rather than never-recorded. assertNull(startupFetchReason(inDays(60), autoRenewing = false, grace = noGrace, now = now)) } @@ -140,14 +123,9 @@ class ProStartupGateTest { @Test fun `grace does not widen the non-renewing rows`() { - // Grace belongs to ONE row โ€” the auto-renewing one โ€” and these pin that. A non-renewing - // account 31 days past its expiry is out of CTA range whatever grace says, and one expiring in - // 3 days is in range whatever grace says. - // - // This is the guard against grace being reintroduced into the other rows by someone who - // remembers it mattering more. It cannot matter here: the wire sends grace = 0 when the - // subscription is not auto-renewing, so any behaviour keyed to a non-zero grace on this path - // is behaviour that never runs in production and only ever fires on a test fixture. + // Grace belongs to one row, the auto-renewing one. The guard against it being reintroduced + // into the others: the wire sends grace = 0 when not auto-renewing, so anything keyed to a + // non-zero grace on this path only ever fires on a fixture. assertNull(startupFetchReason(daysAgo(31), autoRenewing = false, grace = grace, now = now)) assertNotNull(startupFetchReason(inDays(3), autoRenewing = false, grace = grace, now = now)) } diff --git a/app/src/test/java/org/thoughtcrime/securesms/pro/ProStatusFreshnessFloorTest.kt b/app/src/test/java/org/thoughtcrime/securesms/pro/ProStatusFreshnessFloorTest.kt index 2757725348..517ec38102 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/pro/ProStatusFreshnessFloorTest.kt +++ b/app/src/test/java/org/thoughtcrime/securesms/pro/ProStatusFreshnessFloorTest.kt @@ -10,17 +10,12 @@ import java.time.Instant /** * The `get_pro_status` freshness floor. * - * Scope, stated plainly: these cover the floor's **decision**, which is a pure function of - * (immediate, last-fetch timestamp, now). They do NOT cover the wiring โ€” that `requestRefresh` - * reads the timestamp from `pro_state` rather than from `loadState`, and that a dropped request - * really does skip `FetchProStatusWorker`. Both of those need WorkManager and a real database, so - * they are not reachable from a JVM unit test here. + * Scope: the floor's decision, a pure function of (immediate, last-fetch timestamp, now). NOT the + * wiring โ€” that `requestRefresh` reads the timestamp from `pro_state` rather than `loadState`, and that + * a dropped request really skips `FetchProStatusWorker`. Both need WorkManager and a database. * - * That boundary matters because the wiring is where the original bug was: the floor asked whether - * the in-memory load state was `Loading`/`Loaded`, and a cold start begins at `Init`, which is - * neither โ€” so the floor was skipped on precisely the path it existed for. What these tests pin is - * the shape that prevents it recurring: the decision is expressed over the timestamp, and "no - * timestamp" is a distinct, deliberate answer rather than a state that falls between the cases. + * What these pin is the shape: the decision is keyed off the timestamp, and "no timestamp" is a + * deliberate answer rather than a value that falls between the cases. */ class ProStatusFreshnessFloorTest { @@ -28,8 +23,7 @@ class ProStatusFreshnessFloorTest { @Test fun `no recorded fetch means fetch`() { - // A cold start with an empty pro_state, or a user who has never fetched. Distinct from the - // old failure: this is "no evidence of a recent fetch", not "the state enum hasn't settled". + // A cold start with an empty pro_state: no evidence of a recent fetch, which is a reason to go. assertTrue(shouldFetch(immediate = false, fetchedInThisProcess = true, lastFetchedAt = null, now = now)) } @@ -47,9 +41,8 @@ class ProStatusFreshnessFloorTest { @Test fun `the interval boundary is inclusive - exactly the interval ago is allowed`() { - // Pinned deliberately: at exactly the floor the request goes through. The #4 grace poll - // runs at exactly this cadence, so an exclusive boundary here would drop alternate ticks - // to nothing but timing jitter. + // Inclusive on purpose: the #4 grace poll runs at exactly this cadence, so an exclusive + // boundary would drop alternate ticks to timing jitter. val exactly = now.minusSeconds(MIN_UPDATE_INTERVAL_SECONDS) assertTrue(shouldFetch(immediate = false, fetchedInThisProcess = true, lastFetchedAt = exactly, now = now)) } @@ -61,17 +54,14 @@ class ProStatusFreshnessFloorTest { @Test fun `immediate is the only bypass - a just-completed fetch is otherwise dropped`() { - // The negative control for the test above: same inputs, immediate off. Without this pair, - // the immediate test passes just as well against a function that always returns true. + // Negative control: without it the immediate test passes against a function returning true. assertFalse(shouldFetch(immediate = false, fetchedInThisProcess = true, lastFetchedAt = now, now = now)) } @Test fun `the first request of a process is never floored`() { - // Second exemption. A relaunch inside 60s of the last fetch would otherwise be dropped, and - // several things downstream key off THIS process having confirmed the status rather than - // off the stored value being recent โ€” on Android, the home Expired CTA, which is gated on a - // `Loading` transition that only a real fetch produces. + // Second exemption: downstream consumers key off THIS process having confirmed the status, not + // off the stored value being recent. See `ProStatusRepository.fetchedInThisProcess`. assertTrue( shouldFetch( immediate = false, @@ -84,8 +74,7 @@ class ProStatusFreshnessFloorTest { @Test fun `once this process has fetched the floor applies again`() { - // Negative control for the exemption: same inputs, flag flipped. Without this the test - // above passes against an exemption that never turns off. + // Negative control: without it the test above passes against an exemption that never turns off. assertFalse( shouldFetch( immediate = false, @@ -98,9 +87,8 @@ class ProStatusFreshnessFloorTest { @Test fun `a future timestamp still floors`() { - // Clock skew, or a fetch recorded against network time while we compare against a slightly - // behind reading. Treat it as fresh rather than fetching: the alternative reads a skewed - // clock as a licence to bypass the floor entirely. + // Clock skew: a fetch recorded against network time compared with a slightly behind reading. + // Treat as fresh โ€” the alternative makes a skewed clock a licence to bypass the floor. val future = now.plusSeconds(MIN_UPDATE_INTERVAL_SECONDS) assertFalse(shouldFetch(immediate = false, fetchedInThisProcess = true, lastFetchedAt = future, now = now)) } From e6f3b2242ff2b7316abb144e71e5479e483b25df Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Tue, 11 Aug 2026 15:49:27 +1000 Subject: [PATCH 24/28] Pro: keep the wire model at the mapper and point to it elsewhere The expiry/grace contract was restated at eight sites. It belongs at `toProStatus`, which is where the response is read and where the don't-subtract trap has to live; the wake derivation, the gate's parameter doc and two test headers only use it, so they now name what they use and point. The contract itself is unchanged and still stated in full where it is the contract. --- .../securesms/pro/ProProofGenerationWorker.kt | 2 +- .../org/thoughtcrime/securesms/pro/ProStatusManager.kt | 9 +++------ .../securesms/pro/ProExpiredCoverageEndTest.kt | 10 ++++------ .../thoughtcrime/securesms/pro/ProStartupGateTest.kt | 8 +++----- 4 files changed, 11 insertions(+), 18 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt index 51c73b012f..16d75a40fd 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt @@ -176,7 +176,7 @@ class ProProofGenerationWorker @AssistedInject constructor( // account active at the backend, but the `setProAccessExpiry` write above already // fires the (E, prepaid) config-change trigger, which schedules that fetch. // Requesting one here as well would make the proof loop a SOURCE of status - // fetches, which is the coupling this design keeps out. + // fetches, coupling the two loops. Result.success() } diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt index 0971c2157f..31fa69702e 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusManager.kt @@ -360,9 +360,7 @@ class ProStatusManager @Inject constructor( .mapNotNull { state -> state.lastUpdated?.first?.let { status -> status.expiry?.let { renewalDue -> - // `expiry` is the payment-due date; coverage runs a further grace period - // past it. So the renewal falls due at `expiry`, and grace ends at - // `expiry + gracePeriod`. + // Renewal falls due at `expiry`, grace ends at `expiry + gracePeriod`. renewalDue to renewalDue.plus(status.gracePeriod) } } @@ -689,9 +687,8 @@ class ProStatusManager @Inject constructor( * Whether a cold start should fetch `get_pro_status`, and why โ€” or null to stay off the * network. Pure over plain values so it can be tested without a clock, a database or config. * - * [renewalDue] is the access expiry as the backend sends it โ€” the payment-due date. Coverage - * runs a further [grace] past it: `expiry + grace_period_duration` is when the backend stops - * serving. + * [renewalDue] is the access expiry as sent โ€” the payment-due date, with coverage running a + * further [grace] past it (see `toProStatus`). * * | config state | action | * |----------------------------------------------------|-----------------------------------| diff --git a/app/src/test/java/org/thoughtcrime/securesms/pro/ProExpiredCoverageEndTest.kt b/app/src/test/java/org/thoughtcrime/securesms/pro/ProExpiredCoverageEndTest.kt index 647cc304e9..bd9a5dcc35 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/pro/ProExpiredCoverageEndTest.kt +++ b/app/src/test/java/org/thoughtcrime/securesms/pro/ProExpiredCoverageEndTest.kt @@ -10,13 +10,11 @@ import java.time.Instant /** * [ProStatus.Expired.coverageEndedAt] and the 30-day Expired-CTA window measured from it. * - * The backend only reports EXPIRED once coverage has ended, and coverage ends a grace period after the - * payment-due date it sends. So a window measured from the payment date is short by exactly the grace - * period, and empty once grace reaches the window length. + * The backend only reports EXPIRED once coverage has ended, so a window measured from the payment date + * is short by exactly the grace period and empty once grace reaches the window length. * - * A multi-day grace means an Apple account whose dunning ran out, since Apple states its retry window - * separately; on Play it is only the ~1h latency allowance. The cases sweep grace as a parameter rather - * than asserting either store's number. + * A multi-day grace means an Apple account whose dunning ran out; on Play it is only the ~1h latency + * allowance. The cases sweep grace as a parameter rather than asserting either store's number. * * The CTA condition itself lives in `HomeViewModel`, which needs Android. What is testable here is the * instant it keys off. diff --git a/app/src/test/java/org/thoughtcrime/securesms/pro/ProStartupGateTest.kt b/app/src/test/java/org/thoughtcrime/securesms/pro/ProStartupGateTest.kt index b8b8eb00ac..c37b1e2216 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/pro/ProStartupGateTest.kt +++ b/app/src/test/java/org/thoughtcrime/securesms/pro/ProStartupGateTest.kt @@ -13,11 +13,9 @@ import java.time.Instant * Scope: the decision only. These do NOT cover the persisted 24h interval or the config read โ€” both * need a database, and the interval is checked before this function is reached. * - * The model every case turns on: the access expiry the backend sends IS the payment-due date, and - * coverage runs a further grace period past it โ€” `expiry_ts + grace_period_duration` is when it stops - * serving. So the grace window is `[expiry, expiry + grace)` and there is no subtraction anywhere; - * adding one double-counts. `the auto-renewing bound is measured from coverage end, not the payment - * date` pins the direction. + * Every case turns on the wire model stated in `toProStatus`: the expiry IS the payment-due date and + * grace runs forward from it, so there is no subtraction anywhere and adding one double-counts. `the + * auto-renewing bound is measured from coverage end, not the payment date` pins the direction. */ class ProStartupGateTest { From fa70642656e595a93295a55e317ece62c231eb65 Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Tue, 11 Aug 2026 14:24:20 -0300 Subject: [PATCH 25/28] =?UTF-8?q?Pro:=20fix=20stale=20comments=20left=20by?= =?UTF-8?q?=20the=20E=C2=B1G=20polarity=20correction?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit renewingAt is now E (the payment/renewal-due date), not E adjusted for grace; Expired.gracePeriod is coverage-past-expiry for both stores (the backend keeps the reported expiry at paid-through and carries Play's extension as grace); and the freshness floor persists pro_status_last_attempt_at, not the old pro_status_updated_at 'nothing to persist' claim. --- .../java/org/thoughtcrime/securesms/pro/ProStatus.kt | 10 ++++++---- .../thoughtcrime/securesms/pro/ProStatusRepository.kt | 6 +++--- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatus.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatus.kt index 080f428215..ccea0c1ae0 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatus.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatus.kt @@ -11,7 +11,7 @@ sealed interface ProStatus{ data object NeverSubscribed: ProStatus sealed interface Active: ProStatus{ - val renewingAt: Instant //this takes into account the expiry and the grace period + val renewingAt: Instant // the payment/renewal-due date (E), as the backend sends it val duration: ProPlanPeriod // the backend's raw (count, unit) โ€” rendered generically, never bucketed val providerData: PaymentProviderMetadata val quickRefundExpiry: Instant? @@ -70,9 +70,11 @@ sealed interface ProStatus{ * * The backend only reports EXPIRED once coverage has ended, so a window measured from * [expiredAt] instead is short by exactly [gracePeriod], and empty once grace reaches the - * window length. [gracePeriod] is multi-day for an Apple account whose dunning ran out, since - * Apple states its retry window separately; on Play it is only the ~1h renewal-latency - * allowance, because Play folds grace into the expiry it reports. + * window length. [gracePeriod] is coverage-past-expiry: the provider's dunning window plus the + * backend's ~1h renewal-latency allowance. It is multi-day once a real dunning window is known + * (Apple states its retry window directly; for Play the backend keeps the reported expiry at + * the paid-through date and carries Play's expiry extension as the grace instead), and ~1h + * before then. * * Derived here rather than at the consumer so a second reader cannot pick the other anchor. */ diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusRepository.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusRepository.kt index 01d815fb39..e8d6c35237 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusRepository.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProStatusRepository.kt @@ -147,12 +147,12 @@ class ProStatusRepository @Inject constructor( return } - // The floor reads the persisted timestamp rather than `loadState`. `loadState` is a + // The floor reads a persisted timestamp rather than `loadState`. `loadState` is a // StateFlow starting at LoadState.Init, and Init is neither Loading nor Loaded, so the // old check was skipped outright until the database combine had emitted โ€” which the // startup trigger beats. The result was that the floor never applied to the one fetch it - // most needed to cover. `pro_status_updated_at` is already written by every successful - // fetch (ProDatabase.updateProStatus), so there is nothing new to persist. + // most needed to cover. So it instead reads `pro_status_last_attempt_at`, stamped on every + // fetch attempt (see below), which survives process death. // // The scope is GlobalScope (Dispatchers.Default), so the read is off the main thread; the // callers are all fire-and-forget. From 8a7acb520018f685c98c178bce26e8c06c618d32 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Wed, 12 Aug 2026 07:08:38 +1000 Subject: [PATCH 26/28] Pro: grace is multi-day on both providers, not just Apple The test docs said Play's grace is only the ~1h latency allowance because Play folds its grace into the expiry. It does extend `expiryTime`, but the backend keeps the paid term as E and stores that extension as G, so G is multi-day on either provider once a real dunning window is known. Completes ba96c4e03b, which corrected the same claim on the production side. --- .../thoughtcrime/securesms/pro/ProExpiredCoverageEndTest.kt | 4 ++-- .../java/org/thoughtcrime/securesms/pro/ProStartupGateTest.kt | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/src/test/java/org/thoughtcrime/securesms/pro/ProExpiredCoverageEndTest.kt b/app/src/test/java/org/thoughtcrime/securesms/pro/ProExpiredCoverageEndTest.kt index bd9a5dcc35..74c05002d3 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/pro/ProExpiredCoverageEndTest.kt +++ b/app/src/test/java/org/thoughtcrime/securesms/pro/ProExpiredCoverageEndTest.kt @@ -13,8 +13,8 @@ import java.time.Instant * The backend only reports EXPIRED once coverage has ended, so a window measured from the payment date * is short by exactly the grace period and empty once grace reaches the window length. * - * A multi-day grace means an Apple account whose dunning ran out; on Play it is only the ~1h latency - * allowance. The cases sweep grace as a parameter rather than asserting either store's number. + * A multi-day grace means a real dunning window is known, which happens on both providers. The cases + * sweep grace as a parameter rather than asserting any store's number. * * The CTA condition itself lives in `HomeViewModel`, which needs Android. What is testable here is the * instant it keys off. diff --git a/app/src/test/java/org/thoughtcrime/securesms/pro/ProStartupGateTest.kt b/app/src/test/java/org/thoughtcrime/securesms/pro/ProStartupGateTest.kt index c37b1e2216..9c73e0a89a 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/pro/ProStartupGateTest.kt +++ b/app/src/test/java/org/thoughtcrime/securesms/pro/ProStartupGateTest.kt @@ -21,7 +21,7 @@ class ProStartupGateTest { private val now: Instant = Instant.parse("2026-08-07T00:00:00Z") - /** Multi-day grace means an Apple account mid-dunning; Play's is only the ~1h latency allowance. */ + /** Multi-day grace means a real dunning window is known โ€” on either provider. See `ProStatus`. */ private val grace: Duration = Duration.ofDays(14) private val noGrace: Duration = Duration.ZERO From 847bcd5dc412605d9e8c850cc2f22caf88c7abf9 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Wed, 12 Aug 2026 14:32:27 +1000 Subject: [PATCH 27/28] Pro: re-derive why the proof-path writes stay inside the success branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit libSession's parse now reads the grace and renewal fields on the `subscription_expired` path too, and treats an absent field on success as "not applicable" rather than throwing. So the old justification โ€” that the parser returns before filling them, making every non-OK outcome a struct default โ€” is no longer true. The conclusion survives on narrower grounds. Absent-means-not-applicable is safe on success, because 0/false are then genuine values. On a transport or protocol failure the same defaults arrive parsed from nothing, and neither the C struct nor the Kotlin type can distinguish that from a backend saying "not renewing" โ€” which is what makes a hoisted write erase a flag get_pro_status had correctly learned. Also records why the denied path needs no such write: it clears E, and libsession erases G and A with it. --- .../securesms/pro/ProProofGenerationWorker.kt | 23 +++++++++++-------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt index 16d75a40fd..86b9bedc68 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt @@ -157,16 +157,21 @@ class ProProofGenerationWorker @AssistedInject constructor( // response is wrong by the difference between them. // // Do not hoist these two out of the success branch. Their protection is - // PLACEMENT โ€” not the parse, and not the type. libsession's `parse_pro_proof` - // returns on the failure path before filling them, so every non-OK outcome - // leaves struct defaults of grace 0 and renewing false; the C struct has no - // presence flag and the Kotlin type is non-nullable, so a read outside this - // branch cannot tell that from a backend that said "not renewing". + // PLACEMENT โ€” not the parse, and not the type. // - // Writing `false` to a presence-only config key ERASES it. On - // `subscription_expired`/`not_subscribed`/`revoked` erasing is truthful; on a - // protocol error or transport failure it would wipe a flag `get_pro_status` - // had correctly learned, on the strength of a response that said nothing. + // Absent fields on a parsed response mean "not applicable" and arrive as grace + // 0 / renewing false, which are genuine values. On a transport or protocol + // failure the same defaults arrive having been parsed from nothing, and the C + // struct has no presence flag and the Kotlin type is non-nullable, so a read + // outside this branch cannot tell the two apart. + // + // That matters because writing `false` to a presence-only config key ERASES + // it: it would wipe a flag `get_pro_status` had correctly learned, on the + // strength of a response that said nothing about the account. + // + // The entitlement-denied path needs no equivalent write. It clears `E`, and + // libsession erases `G` and `A` with it โ€” a grace that outlived its expiry + // would pair with whatever wrote `E` next. configs.userProfile.setProAutoRenewing(response.accountAutoRenewing) configs.userProfile.setProGracePeriod(response.accountGracePeriod) } From 1945149119bfe52ca627366b75dd5e84dbc95c29 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Wed, 12 Aug 2026 16:13:11 +1000 Subject: [PATCH 28/28] Bump libsession-util-android to 1.1.0-49-g5dbfffc The pin still pointed at 1.1.0-37-g5f85e5d, whose Config.kt has none of the auto-renewing or grace-period accessors this branch calls, so a build resolving the published AAR could not have linked. Nothing local could have caught it: settings.gradle.kts builds the glue from source whenever session.libsession_util.project.path is set, and that path bypasses artifact resolution entirely. Production and CI leave it unset. Verified against the published artifact rather than the derived string: resolution succeeds, and the AAR's own classes.jar declares get/setProAutoRenewing and get/setProGracePeriod with the expected signatures, with all four JNI symbols defined in each of its four ABI libraries. --- gradle/libs.versions.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 03855d9d83..aec0e97d5c 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -30,7 +30,7 @@ kotlinxImmutableVersion = "0.5.1" kryoVersion = "5.6.2" kspVersion = "2.3.6" legacySupportV13Version = "1.0.0" -libsessionUtilAndroidVersion = "1.1.0-37-g5f85e5d" +libsessionUtilAndroidVersion = "1.1.0-49-g5dbfffc" media3ExoplayerVersion = "1.10.0" mockitoCoreVersion = "5.23.0" navVersion = "2.9.8"