From fc2d645f7c7f7b7666ca5159110bde81c298b0f5 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Fri, 28 Aug 2026 10:22:01 +1000 Subject: [PATCH 1/2] Pro: a revoked proof no longer clears the synced access expiry The proof worker treated revoked, not_subscribed and subscription_expired as one answer and cleared the access expiry for all three. Revoked is not the same answer as the other two. Revoked says this PROOF is void. It says nothing about the subscription, and carries no expiry to say it with -- a revocation that does not revoke payments is a rotation, leaving the account paid and re-provable, and locally we cannot tell that from a refund. Clearing the expiry answers a question the backend did not answer. It also propagates. The expiry is synced config, so one device's clear reaches every other device and erases the shared record that the user ever subscribed: with no expiry and no proof, the seeded display status reads "never subscribed" -- a confident claim rather than an absence -- and a refunded subscriber is offered "Upgrade" where they should see "Renew". This is what ProStatusManager already does on the revocation-list path, and its comment there says why; the worker was undoing it three files away. not_subscribed still clears, because no account row exists and there is genuinely nothing to record. The defunct credential is still dropped either way, guarded so a proof another device just landed survives. Keeping the expiry leaves the acquire loop running, since libsession's renewal target fires on a future expiry with no proof. That is bounded, not unbounded -- the dark backoff widens to a 15-minute floor -- and it ends when a status fetch writes a past expiry. No extra limiter added: a third guard would suppress the symptom of a loop that already terminates. --- .../securesms/pro/ProProofGenerationWorker.kt | 47 +++++++++++++++---- 1 file changed, 39 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 86b9bedc68..1f42a45deb 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt @@ -197,21 +197,52 @@ class ProProofGenerationWorker @AssistedInject constructor( notEntitled && purchasePending -> Result.retry() notEntitled -> { - // Backend authoritatively says we're not (or no longer) entitled. Clear the - // synced access-expiry (E) so the renewal loop terminates: libsession's renewal - // target now fires on "future E but no proof", so a stale future E left here - // would spin. (subscription_expired's past account_expiry is redundant with the - // get_pro_status horizon, so we clear E rather than re-set it.) Also drop a now- - // defunct credential, guarded so a proof another device just landed survives. + // The backend says this device is not (or no longer) entitled, and the defunct + // credential goes either way — guarded, so a proof another device just landed + // survives. + // + // Whether the synced access-expiry (E) goes with it depends on WHICH answer + // this is, and REVOKED is not the same answer as the other two: + // + // * REVOKED says this PROOF is void. It says nothing about the subscription, + // and it carries no expiry to say it with — a revocation with + // revoke_payments=false is a rotation, leaving the account paid and + // re-provable, and locally we cannot tell that from a refund. Clearing E + // here would answer a question the backend did not answer. E is SYNCED, so + // that answer would propagate to every other device and erase the shared + // record that the user ever subscribed: with no E and no proof, the seeded + // display status reads "never subscribed" — a confident claim, not an + // absence — and a refunded subscriber gets offered "Upgrade" instead of + // "Renew". Matches the reasoning in `ProStatusManager`, which keeps E on the + // revocation-LIST path for the same reason. + // + // * NOT_SUBSCRIBED means no account row exists, so there is genuinely nothing + // to record and clearing is right. + // + // Keeping E on REVOKED leaves libsession's renewal target firing on + // "future E but no proof", so the acquire loop keeps running. That is bounded + // rather than unbounded — the dark backoff widens to DARK_CAP_SECONDS spacing — + // and it ends when a status fetch writes a past E. Deliberately no extra + // limiter here: a third guard would suppress the symptom of a loop that already + // terminates. + val keepAccessExpiry = code == ProErrorCode.REVOKED + configFactory.withMutableUserConfigs { configs -> - configs.userProfile.removeProAccessExpiry() + if (!keepAccessExpiry) { + configs.userProfile.removeProAccessExpiry() + } val nowSeconds = snodeClock.currentTime().epochSecond val existing = configs.userProfile.getProConfig()?.proProof if (existing == null || existing.expirySeconds <= nowSeconds) { configs.userProfile.removeProConfig() } } - Log.w(WORK_NAME, "Pro proof denied (code=$code); cleared access-expiry, ending the acquire loop") + Log.w( + WORK_NAME, + "Pro proof denied (code=$code); " + + if (keepAccessExpiry) "kept access-expiry (the proof is void, the plan may not be)" + else "cleared access-expiry, ending the acquire loop" + ) Result.failure() } From ff669440bfbbf2fd7b20cc3a50f2539b2c9458cb Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Fri, 28 Aug 2026 10:36:30 +1000 Subject: [PATCH 2/2] Pro: store the dates a lapsed subscription returns, and fetch status after every denial Two changes on the proof worker's entitlement-denied path, on top of revoked no longer clearing the access expiry. First, subscription_expired now stores what it returns. The backend sends the account expiry, grace period and renewing flag with that slug precisely so a client can persist them and read them back later -- offline, at cold start, on another device. We were discarding them and clearing the expiry instead, which left a lapsed subscriber indistinguishable from one who never subscribed. All three, not the expiry alone. On a cancellation the expiry is typically the value that did NOT change while the grace and the flag did, so writing only the expiry leaves a stale grace claiming coverage the backend has stopped honouring; coverage end is derived as expiry + grace. The write is gated on the slug, and that gate does real work: libsession populates those fields for this slug only and otherwise leaves them at 0/false, which are indistinguishable from genuine zeroes -- and writing false to a presence-only key erases it, which would wipe a flag get_pro_status had correctly learned. Carrying those values required the API layer to stop discarding the parsed body on failure. Some error slugs are informative rather than merely negative, so Failure now holds the parsed response alongside the error. Reaching that branch means the envelope parsed, which is what makes reading it safe at all. Second, every denial now fetches the account status immediately. Change the local record, then ask the server. Immediate rather than floored because a floored request is dropped whenever a fetch already ran inside the floor -- the ordinary case, since launch fetches -- so flooring drops exactly the fetch that matters: it is the acquire loop's terminator, and only a response writing a past expiry stops the loop. This deliberately makes the proof loop a source of status fetches, which the success path avoids. The difference is that the success path has another trigger in its own expiry write, whereas revoked writes nothing -- so without this the terminator would arrive only via the revocation-list path, a dependency between two paths that neither of them states. --- .../securesms/pro/ProProofGenerationWorker.kt | 103 ++++++++++++------ .../thoughtcrime/securesms/pro/api/ProApi.kt | 19 +++- 2 files changed, 86 insertions(+), 36 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 1f42a45deb..0957955b28 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/ProProofGenerationWorker.kt @@ -53,6 +53,7 @@ class ProProofGenerationWorker @AssistedInject constructor( private val loginStateRepository: LoginStateRepository, private val configFactory: ConfigFactoryProtocol, private val snodeClock: SnodeClock, + private val proStatusRepository: Provider, ) : CoroutineWorker(context, params) { override suspend fun doWork(): Result { val proMasterKey = requireNotNull(loginStateRepository.peekLoginState()?.seeded?.proMasterPrivateKey) { @@ -169,9 +170,8 @@ class ProProofGenerationWorker @AssistedInject constructor( // 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. + // The entitlement-denied path writes the same three, but only for + // `subscription_expired` — the one denial that returns them. See that branch. configs.userProfile.setProAutoRenewing(response.accountAutoRenewing) configs.userProfile.setProGracePeriod(response.accountGracePeriod) } @@ -197,51 +197,88 @@ class ProProofGenerationWorker @AssistedInject constructor( notEntitled && purchasePending -> Result.retry() notEntitled -> { - // The backend says this device is not (or no longer) entitled, and the defunct - // credential goes either way — guarded, so a proof another device just landed - // survives. + // The backend says this device is not (or no longer) entitled. The defunct + // credential goes in every case — guarded, so a proof another device just + // landed survives. What happens to the synced access expiry (E) does NOT + // generalise, because the three slugs are three different answers: // - // Whether the synced access-expiry (E) goes with it depends on WHICH answer - // this is, and REVOKED is not the same answer as the other two: + // * REVOKED — this PROOF is void, and that is all it says. It returns no + // dates to say more with. A revocation that does not revoke payments is a + // rotation, leaving the account paid and re-provable, and locally we cannot + // tell that from a refund. So KEEP E: clearing it would answer a question + // the backend did not answer, and E is SYNCED, so that answer would reach + // every other device and erase the shared record that the user ever + // subscribed. With no E and no proof the seeded display reads "never + // subscribed" — a confident claim rather than an absence — and a refunded + // subscriber is offered "Upgrade" where they should see "Renew". + // `ProStatusManager` keeps E on the revocation-LIST path for this reason. // - // * REVOKED says this PROOF is void. It says nothing about the subscription, - // and it carries no expiry to say it with — a revocation with - // revoke_payments=false is a rotation, leaving the account paid and - // re-provable, and locally we cannot tell that from a refund. Clearing E - // here would answer a question the backend did not answer. E is SYNCED, so - // that answer would propagate to every other device and erase the shared - // record that the user ever subscribed: with no E and no proof, the seeded - // display status reads "never subscribed" — a confident claim, not an - // absence — and a refunded subscriber gets offered "Upgrade" instead of - // "Renew". Matches the reasoning in `ProStatusManager`, which keeps E on the - // revocation-LIST path for the same reason. + // * SUBSCRIPTION_EXPIRED — a lapse or cancellation, and the response carries + // the expiry, grace period and renewing flag so a client can persist them. + // SET all three. Not the expiry alone: on a cancellation the expiry is + // typically the value that did NOT change while the grace and the flag did, + // so writing only the expiry leaves a stale grace claiming coverage the + // backend has stopped honouring. Coverage end is derived as E + G. // - // * NOT_SUBSCRIBED means no account row exists, so there is genuinely nothing - // to record and clearing is right. + // * NOT_SUBSCRIBED — no account row exists, so there is genuinely nothing to + // record. CLEAR E. // - // Keeping E on REVOKED leaves libsession's renewal target firing on - // "future E but no proof", so the acquire loop keeps running. That is bounded - // rather than unbounded — the dark backoff widens to DARK_CAP_SECONDS spacing — - // and it ends when a status fetch writes a past E. Deliberately no extra - // limiter here: a third guard would suppress the symptom of a loop that already - // terminates. - val keepAccessExpiry = code == ProErrorCode.REVOKED + // The three-value write is gated on the slug, and that gate is doing real + // work: libsession only populates those fields for SUBSCRIPTION_EXPIRED and + // otherwise leaves them at 0/false, which are indistinguishable from genuine + // zeroes. Writing `false` to a presence-only key ERASES it, so an ungated + // write would wipe a flag `get_pro_status` had correctly learned. + val expiredWithDates = code == ProErrorCode.SUBSCRIPTION_EXPIRED configFactory.withMutableUserConfigs { configs -> - if (!keepAccessExpiry) { - configs.userProfile.removeProAccessExpiry() + when { + expiredWithDates -> { + result.parsed.accountExpiry?.let { + configs.userProfile.setProAccessExpiry(it.epochSecond) + } + configs.userProfile.setProGracePeriod(result.parsed.accountGracePeriod) + configs.userProfile.setProAutoRenewing(result.parsed.accountAutoRenewing) + } + + // REVOKED keeps whatever the backend last said; only NOT_SUBSCRIBED + // clears. + code == ProErrorCode.NOT_SUBSCRIBED -> { + configs.userProfile.removeProAccessExpiry() + } } + val nowSeconds = snodeClock.currentTime().epochSecond val existing = configs.userProfile.getProConfig()?.proProof if (existing == null || existing.expirySeconds <= nowSeconds) { configs.userProfile.removeProConfig() } } + + // Change the local record, then ask the server — on every one of the three, + // not just the ones that cleared something. + // + // IMMEDIATE, bypassing the routine freshness floor. A floored request is + // dropped whenever a status fetch already ran inside the floor, which is the + // ordinary case because launch fetches, so flooring drops exactly the fetch + // that matters. It matters because it is the acquire loop's TERMINATOR: + // libsession's renewal target fires while E is in the future with no proof, and + // only a status response writing a past E stops it. + // + // This does make the proof loop a source of status fetches, which the success + // branch above deliberately avoids. The difference is that the success branch + // has another trigger — its own E write fires the config-change trigger — + // whereas REVOKED writes nothing, so without this the terminator would have to + // arrive via the revocation-LIST path. That would be a dependency between two + // paths that neither of them states. + proStatusRepository.get().requestRefresh(immediate = true) + Log.w( WORK_NAME, - "Pro proof denied (code=$code); " + - if (keepAccessExpiry) "kept access-expiry (the proof is void, the plan may not be)" - else "cleared access-expiry, ending the acquire loop" + "Pro proof denied (code=$code); " + when { + expiredWithDates -> "stored the returned expiry, grace and renewing flag" + code == ProErrorCode.NOT_SUBSCRIBED -> "cleared access-expiry" + else -> "kept access-expiry (the proof is void, the plan may not be)" + } + "; fetching status immediately" ) Result.failure() } diff --git a/app/src/main/java/org/thoughtcrime/securesms/pro/api/ProApi.kt b/app/src/main/java/org/thoughtcrime/securesms/pro/api/ProApi.kt index 55814e03c4..a291979815 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/pro/api/ProApi.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/pro/api/ProApi.kt @@ -60,11 +60,12 @@ abstract class ProApi(private val deps: ProApiDependencies) ProApiResponse.Success(parsed) } else { ProApiResponse.Failure( - ProApiError( + error = ProApiError( status = parsed.header.status, errorCode = parsed.header.errorCode, error = parsed.header.error, - ) + ), + parsed = parsed, ) } } @@ -113,7 +114,19 @@ object ProErrorCode { */ sealed interface ProApiResponse { data class Success(val data: T) : ProApiResponse - data class Failure(val error: ProApiError) : ProApiResponse + + /** + * A failure still carries the [parsed] body, because some error slugs are informative rather than + * merely negative: `subscription_expired` returns the account expiry, grace period and renewing flag + * precisely so a client can persist them. + * + * Reaching here means the ENVELOPE parsed — a body that could not be parsed throws before this — so + * [parsed] is a real struct rather than defaults built from nothing. That distinction is what makes it + * safe to write config from a failure at all, and it is only safe **per slug**: libsession populates + * those fields for `subscription_expired` and leaves them at their defaults otherwise, and the defaults + * are indistinguishable from genuine zeroes. Gate any write on the slug. + */ + data class Failure(val error: ProApiError, val parsed: T) : ProApiResponse } fun ProApiResponse.successOrThrow(): T {