Pro: separate the status and proof refresh loops, and make the freshness floor real - #2155
Draft
mpretty-cyro wants to merge 3 commits into
Draft
Conversation
Lets a QA Pro backend be targeted without rebuilding, matching the iOS customProBackendUrl/customProBackendPubkey launch variables. Both values are required together: a QA URL paired with the production signing key reads every QA-signed proof as invalid and silently strips Pro content.
…ation
Reading a SessionProtocol constant runs System.loadLibrary("session_util"), so
doing it from the companion's initialiser made the class impossible to
initialise wherever the native library is absent -- every JVM unit test. Mockito
could not instrument it, and the nine tests constructing a ConversationViewModel
failed with NoClassDefFoundError.
The constants stay single-sourced from libsession; they are just read on first
use rather than on class load.
A successful generate force-refreshes get_pro_status, which asks libsession for a renewal target, and `proofExpiry - PRO_RENEWAL_LEAD` is permanently in the past for any proof living less than the 60-minute lead -- so the worker rescheduled itself immediately and looped. Mirrors iOS SessionProManager.reconcileProofRenewal and Desktop, constants included: 60s while covered, 15s * attempt capped at 900s while dark, and re-arming rather than dropping the work, since `target <= now` is also the normal renewal-due signal. The state is in-memory as it is on the other two platforms; a process restart costs one extra request rather than a loop.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Android's half of the cross-client Pro status-refresh unification. Desktop and iOS carry the same
spec; where a decision was cross-client it was routed rather than settled here.
🔴 Known flaw — do not merge without reading this
This branch ships code that encodes a premise we have proved false. It does so deliberately, so
that all three clients stay consistent until the correction lands on all three together — but a
reviewer must not merge it believing the grace handling is correct.
get_pro_status.expiry_tsis grace-inclusive. The backend folds the grace period into the storedexpiry before sending it (
Session-Pro-Backendbackend.py_lookup_user_expiry→users.expiry_at→server.py:317), and judgesactiveagainst that same value (:322). Its owntest subtracts grace from the wire value to recover the store's paid-through date
(
tests/test_google.py:556-560). SoEis coverage end, not paid-through, andnow ≥ Eisgrace's exit, not its entry.
What that means for this branch:
inGracePeriod = now >= expiry(pre-existing,ProDataMapper) sits inside a branch requiringnow <= expiry— the grace indicator is effectively dead code.renewingAt = expiryrenders the renewal date one whole grace period late.currently use the superseded reading.
Why it is not fixed here: it is a user-visible date, and Desktop and iOS encode the identical
premise — in three different wordings, which is why it read as corroboration rather than one mistake
copied three times. Fixing one client alone makes the three disagree about what "in grace" means.
What has been done instead: the contested instant is isolated behind
renewalDueAt()/coverageEndsAt()inProSettingsViewModel, so the correction is two one-line bodies; and thefalse premise is marked in place at its source in
ProDataMapper, with the superseded text keptverbatim so it stays greppable across the three clients.
This is tracked as F8, with the architect. Do not merge until it is ruled on.
The startup gate IS in this branch, so row 1 of the gate is affected too. The real grace window
(
E − grace ≤ now < E) lies entirely inside row 1'snow < E, the row that declines to fetch — so thegate is blind to precisely the state this rework exists to surface. Built that way deliberately:
Desktop and iOS ship the identical row, and correcting it is a three-client change. Two tests pin the
blind spot and will fail when F8 lands, which is the intended signal, not a regression.
Magnitude, per provider — this is not uniformly cosmetic. Apple configures no grace, so the
backend's own ~1h stand-in is the whole of it and the renewal date is an hour out. Google's real
grace is an operator-configured base-plan setting measured in days, fetched and stored at the moment
the user enters grace (
providers/google_play/notifications.py:704-718). So on Android — a Google Playclient — the renewal date can be days wrong, on the screen whose purpose is that date, and it is
largest exactly when someone is looking at it.
feature/pro-backend-override-qa) — this branch is cut from its head445b74cc89, which carries the proof-acquisition floorJasonFork/pro-auto-renewing-config, head8e5634b8) — adds theA/auto_renewinguser-profile keyLibSession-Util-Androidrelease exposing #121, plus thelibsessionUtilAndroidVersionbump ingradle/libs.versions.tomlMerge order is 2 → 3 → 1 → this. Until 3 exists the
auto_renewingread switch cannot be built,which is why only the write side is in this PR (see Deliberately not included).
Not a dependency of this PR — conditional, and listed only so the link isn't lost
user_profile_has_pro_auto_renewing()— a libSession accessor answering "hasAever beenwritten?", built and tested on
feature/pro-auto-renewing-tristate(off8e5634b8), unpushed andunreviewed.
This branch does not call it and compiles and behaves correctly without it.
Ais presence-only,so a stored
falseand a never-written one are indistinguishable through the plain accessor — whichis all this branch uses. The new accessor becomes a dependency only if the architect rules that
the startup gate must bootstrap a fetch when
Ahas never been written (tracked as F2). If thatruling lands, this PR additionally needs the accessor plus an Android JNI mirror of it.
Note what it does and does not make possible: "has anyone ever written
A?" becomes answerable, sobootstrap-once-then-trust is implementable. "Is this user explicitly not auto-renewing?" remains
unanswerable — that would need
Ato store0rather than erase, i.e. a wire-encoding change.Not the
-pfsbranch.JasonFork/pro-auto-renewing-config-pfshas an identical commit subject anda different sha, and is not a descendant — it rebases the change onto the PFS track.
gh pr view 121 --json headRefOidconfirms8e5634b8.What this changes
Entitlement is the proof; display is the status. They no longer drive each other.
proof − 1h → status fetchtrigger (source Add "is_deleted" column to sms/mms #3 ofmanageProStatusRefreshScheduling).A status fetch keyed to proof timing meant a proof renewing early or late dragged the status fetch
with it.
Efrom the proofresponse, and that write fires the config-change trigger on its own.
FetchProStatusWorkerintoProStatusManager.manageProofRenewalScheduling,which watches the three config inputs to libsession's
pro_renewal_target— the stored proof,E,and the prepaid marker. Proof renewal no longer depends on a status fetch having happened.
force→immediate, with a closed, documented caller list: manual/recover, the post-purchasepoll, and the bounded grace poll. Every routine trigger now goes through the floor.
sleeps until the renewal falls due, then polls once a minute until it lands, coverage ends, or the
screen closes.
fetch at or after the renewal became due. Previously a status snapshot predating the crossing could
raise an alarm about an event it could not have observed.
a persisted 24h minimum in its own attempt-stamped key. Non-subscribers and comfortably-active users
no longer fetch on every cold start.
Bugs found and fixed along the way
Four defects in this path had one cause: a state meaning "we have not confirmed anything" being
compared against an enum that does not enumerate it, so it fell into whichever branch happened to
catch it. Twice it fell outside the compared cases and disabled a check; twice it fell inside a
success-shaped default and satisfied one.
Loadingor
Loaded;Initis neither, so the check was skipped on exactly the path it exists for. It nowreads the persisted timestamp. Covered by
ProStatusFreshnessFloorTest.ProProofGenerationWorkergatedon the in-memory load state holding an ACTIVE status, but WorkManager persists its schedule across
process death and
loadStaterestarts atInit. It now asks libsession the same question thescheduler does.
HomeViewModel's Expired-CTA gate(
refreshState is State.Success) never enforced what its comment claims, because the mapping endedin a catch-all that swept up
Init. What actually suppressed the CTA was the unconditional startupfetch driving a
Loadingtransition — so gating startup would have removed it.ProStatusRepository.fetchedInThisProcessguarantees the first request of a process still reachesthe network; its comment records that it is load-bearing and why.
same catch-all also swept up a
Loadedrestored from WorkManager's persisted work state, so atprocess start, before asking anyone anything, the refresh state read as
Successagainst a cachedresponse. A renewal that happened while the app was closed therefore splashed the Expired CTA off
the stale cache on the next launch — reported in the wild by the architect.
LoadState.Loadednowcarries whether the fetch landed in this process, and the catch-all is gone: the mapping is
exhaustive, so the next state added has to declare which it is. That last part is the durable fix —
the
elseis what made two different non-successes indistinguishable.The floor's timestamp is stamped on attempt, not success, and needed a new
pro_statekey:pro_status_updated_atis written in the same statement as the response blob and is unreadablewithout it, so a failed fetch had nothing to record and a failing network was never throttled.
Deliberately not included
!auto_renewing && now < E && outside the CTA windowrow — held (F2). It returns "nofetch", the spec's letter, but
Ais presence-only so that also covers "never written", which iswhere every existing subscriber lands on their first run. Whether it needs a bootstrap fetch is a
decision, not an implementation; a test pins the current answer.
auto_renewingfrom config (ProDataMapper). The write side IS included — everyget_pro_statusnow persists it alongsideE. The read switch is held becauseautoRenewingisthe branch selector for the construction F8 rewrites, under the comment F8 invalidates: landing it
first would produce a diff whose justification the next diff deletes.
get_pro_status.expiry_tsis grace-inclusive (the backendfolds grace into the stored expiry before sending it; verified in
Session-Pro-Backendand by itsown test), so
now ≥ Eis grace's exit rather than its entry. All three clients currently encode theopposite, including in merged code. This PR keeps the existing (spec) reading and isolates the
contested instant behind
renewalDueAt()/coverageEndsAt()inProSettingsViewModel, so theruling is a two-line change. Do not fix this on one client alone — it is a user-visible date.
Testing
:app:testPlayDebugUnitTest— 207 tests, 0 failures, verified against result-file mtimes ratherthan counts alone (
TEST-*.xmlpersists across runs that never happened, so counts alone are notevidence of a run).
ProStatusFreshnessFloorTestcovers the floor decision as a pure function, including both exemptionsand their negative controls. Mutation-checked: breaking the boundary or the absent-timestamp answer
turns exactly the intended cases red.
Note for reviewers: this branch also retires a stale baseline —
ConversationViewModelTest's 9known failures on
devare fixed by59060641d2, so the suite is fully green here.Note: This is based on https://github.com/mpretty-cyro/libsession-util/pull/2153