Codex is right about this one. The downloader observer builds Downloading
out of whatever status it replaces, reading shareableFallback from an
existing Downloading or a current Ready. A ViewModel restored onto work
that is already active starts from Loading, so neither cast matches and
the fallback is null. WorkManager keeps a download running across process
death, so this is the ordinary case: background the app during a 42 MB
transfer over Tor, come back, and the row drops to "Prepare App for
Sharing" while Share via Hotspot and Share via Quick Share disappear
entirely. The installed APK never moved - it was on disk and shareable a
moment earlier - and it stays hidden until the download ends.
checkStatus() could not repair it because its guard conflated two things:
not letting a resolved status overwrite active work, which is right, and
not looking at local state at all during a download, which is not. Both
orderings lost. If the observer arrived first the guard returned early. If
checkStatus() arrived first it suspended on disk IO, the observer flipped
the state underneath it, and the re-check discarded the status it had just
resolved.
Resolves the local artifact either way and decides inside the same state
update, where the active download is visible: the download keeps the
status it owns, and adopts the artifact only when it is carrying none.
Metadata is still skipped while work is active, so this costs no extra API
budget.
Verified on a Pixel 9a by starting a download, force-stopping mid-transfer
and reopening: the row holds "App Ready for Offline Sharing" with both
sharing rows present, where it previously showed neither.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GitHub sends X-RateLimit-Reset on every REST response, an ordinary 403
included, and it always points at the current window. Feeding it through
retryAtMillis() therefore produced a non-null deadline for any 403, and
the classifier accepted that as proof of a limit. A permissions failure
with the quota untouched came back as RateLimited, so the caller persisted
a cooldown on that route and served stale metadata until a reset window
the failure had nothing to do with.
Classification now looks only at signals that actually mean this request
was the one refused: a spent quota, or an explicit Retry-After. Nothing
real is lost, because GitHub marks a primary limit with
X-RateLimit-Remaining: 0 and a secondary limit with Retry-After. The reset
header keeps its job of supplying the deadline once a limit is
established some other way.
ApkDownloadSourceTest already claimed this contract - its name is "403 is
only treated as a limit when response headers say so" - but its
permissions case passed no reset header at all, which is the one input
that hides the bug. Adds the case it was missing, which fails without this
change, and pins the secondary-limit path so tightening the reset header
cannot blind the client to a Retry-After.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The cooldown was tracked in two places. ApkRateLimitStore persisted it per
scope and per route, and the ViewModel kept a second copy in
downloadRetryAtMillis plus a retryAtMillis on Resumable and Error, frozen
into WorkManager output data on the way through. The copy was the weaker
of the two: it lived in memory, so a cold start lost it, and the deadline
it froze belonged to whichever route earned it, which is exactly the drift
the per-route store exists to prevent.
It also could not expire on its own. downloadRetryBlocked was computed
with System.currentTimeMillis() during composition, so nothing recomposed
when the deadline passed; scheduleRetryUnlock papered over that with a
viewModelScope delay that died with the process. Meanwhile the disabled
row and icon gave the user no countdown to read, so a tap simply did
nothing.
Drops the copy. The store is consulted where the request is actually made
and the UI stays enabled, which costs a worker that fails in well under a
tenth of a second without touching the network.
RateLimitedWithWait goes with it. Its "try again in %2$s min" was computed
at failure time and baked into static text that never ticked down, so it
was wrong within a minute; RateLimited says "try again later" and stays
true. Removing it leaves nothing pre-formatted, so Resumable and Error now
carry an ApkFailureMessage of string id plus arguments and the row
resolves it during composition. Failure text follows the device locale
rather than the locale the worker happened to run under.
Anchors the GitHub cooldown at the moment it is judged. now was sampled
before awaitRoute(), which can hold a request for the full 60-second route
timeout, so a relative Retry-After interpreted against it could land in
the past and let the very next check reach GitHub - the loop this branch
set out to close. Reads the clock again once the route is ready and once
the response arrives, and uses each where it applies.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
routedHttpClient() and webSocketClient() both did a plain check-then-set
on their AtomicReference: read, and if empty build a client and store it.
Two threads arriving together each saw an empty reference, each built a
full OkHttpClient, and the loser's client was dropped on the floor with
its connection pool and dispatcher threads already allocated. Nothing
closed it, so the leak lasted until the process died.
Moves construction inside a lock and re-checks the reference there, so
the second thread returns the first thread's client rather than building
its own. reset() takes the same lock, which is what makes the pairing
airtight: a build can no longer interleave with a reset and store a
client for the route that was just discarded. The fast path stays outside
the lock, so a warm client still costs a single volatile read.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The alpha bump was reverted in 2c19d00d, but its verification metadata
stayed behind: 45 components for compose 1.12.0-beta01 and material3
1.5.0-alpha25 that no lockfile resolves. Regenerated from upstream's file
so only artifacts this branch actually pulls are trusted.
Trusting artifacts nothing resolves is the opposite of what this file is
for, and it made the branch look like a Compose beta upgrade in review.
gradle/verification-metadata.xml: +322 lines -> +8, one component
(mockwebserver3, the dependency 2c19d00d genuinely added).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI failed at ':wear:compileDebugUnitTestKotlin' with okhttp, okio and
mockwebserver3 "not part of the dependency lock state". No test ran.
Adding okhttp-mockwebserver to the shared test bundle put okhttp and okio
on :wear's unit-test classpath as well, but only :app's lock state was
regenerated, so :wear/gradle.lockfile had no entry for any of them.
Regenerated lock state and verification metadata for debug and both
release variants per docs/reproducible-builds.md. No new components
needed trusting: the checksums already existed from the :app side, so
this is lockfile scope only.
Worth noting the local command that missed it was :app-scoped;
CI runs testDebugUnitTest at the root, which includes :wear.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Self and grouped voice-note bubbles had no long-press target: the sender
label is hidden and VoiceNotePlayer controls consume touches, so the
message action sheet was unreachable. combinedClickable on the shell
restores long-press for every media bubble.
Codex is right about this one. Cancellation in Kotlin is cooperative, and
everything from validateDownloadedApk() through saveMetadata() is plain
blocking code with no suspension point. Stopping during the signature
check was therefore not observed until after the temp file had been
renamed and its metadata written, so the worker committed the APK while
WorkManager reported the work cancelled.
That also raced onCancelDownload(): its checkStatus() could read the cache
before the commit and settle on NotDownloaded, after which the cancelled
work maps to Idle and the observer ignores it. The row then advertised
"Not ready" with a verified universal APK already in the cache, and
tapping it downloaded the same bytes again.
One checkpoint after validation, which is the slow step and so the most
likely moment to press Stop. The verified temp file is left in place, so
the next attempt resumes rather than starting over.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codex caught this and it is correct. WorkManager returns a retried
request to ENQUEUED for the duration of its backoff whether or not the
device is online, and the mapping sent every ENQUEUED record to
AwaitingConnectivity. With exponential backoff from 15s over three
attempts, a fully connected device claimed "Waiting for network…" in both
the row and the notification for roughly 45 seconds.
ENQUEUED covers two different waits and the state alone cannot separate
them; a non-zero runAttemptCount means the work already ran, so it is the
backoff. Adds a Retrying phase for that case, extracted as queuedPhase()
so the distinction is testable without a WorkInfo.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codex flagged this on the string-extraction change and it is right.
WorkManager keeps failed records in its own database across app updates,
and AAPT2 reassigns R.string ids on every build. A failure written by one
build and read by the next would resolve its persisted int against a
different resource table: wrong string, or NotFoundException, or
IllegalFormatException when the placeholder arity no longer matches. The
existing zero-check only caught an absent key, not a stale valid one.
ApkDownloadFailureReason now names each failure and owns its string, and
the boundary carries the enum name. This is the same treatment
DownloadPhase.fromKey already gives the phase across the same boundary,
including tolerating a name this build no longer has.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Progress moves out of the trailing slot and under the subtitle, so every
status now shows exactly one 48dp control there instead of a spinner and
a button competing for the same space. The trailing slot had three
different widths across states, which made the text column re-wrap on
every status change; it is now one width throughout.
"Get universal" and "Retry" become icon buttons. That removes the labels
they were leaning on, so both gain tooltips and real content
descriptions, and prepareRowTapAction() now drives the row's enabled flag
and its tap handler from one mapping. Previously the row rendered as
clickable in the ready state but onPrepareRowClicked ignored it, leaving
the icon as the only way to reach the universal download.
Resumable downloads get a progress bar for the first time, drawn flat via
amplitude 0 so a stalled download does not look like a live one.
Strings: every user-facing literal now lives in strings.xml. Download
failures were assembled as English sentences in the util layer, which has
no Context by design, so they crossed the WorkManager boundary already
formatted and could never be translated. ApkDownloadException now carries
a string resource and its arguments, and the ViewModel resolves them
against the device locale. util/ stays Context-free and its tests stay
plain JUnit.
Also drops translatable="false" from seven strings that were visible
prose, and stops showing raw exception text when the APK status cannot be
read.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The expressive wavy progress indicators are Compose-only in the 1.5.0
line, which has no stable release yet, so this overrides the BOM's 1.4.0
for that one artifact.
The override sits outside the BOM, so material3's own requirements win
and pull ui, runtime, foundation and animation from 1.11.4 to
1.12.0-beta01. That is the real cost of this change and the reason the
lock diff is 46 components rather than one.
Lock state and verification metadata regenerated for debug and both
release variants per docs/reproducible-builds.md.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses review on #812.
Each retry resampled the route but never rechecked the gate against it,
so a route change during a request or its backoff walked straight past a
cooldown. A Tor attempt failing with a 500, then the user switching to a
direct connection that is already rate limited, and the next attempt
contacts it regardless.
The check moves inside the loop, immediately after the route is sampled,
which makes it cover the first attempt too -- the separate post-wait
check it replaces was only ever that first iteration. The check before
the route wait stays: knowing the selected route is blocked is worth
avoiding a sixty-second Tor bootstrap for.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses review on #812. Both findings are the same mistake: the route
was sampled at a moment that need not match the request it governs.
The gate was checked once, before a route wait that can last a minute.
Start with Tor selected, disable it during the wait, and the request
goes direct having consulted only the Tor deadline -- contacting a
direct IP whose own cooldown is still running. The gate is now
re-evaluated after the wait, when the route the request will take is
finally known.
The cooldown was likewise recorded against the mode selected when the
response arrived, not the one the call was made on. Changing the setting
mid-flight filed it in the wrong bucket, freeing the limited route and
suppressing the newly selected one. The route is now sampled
immediately before each attempt and reused for that attempt's response
and its retry decision.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses review on #812.
Scoping the gate to the route was right, but it was implemented as one
deadline that moved with the route, so a switch deleted the cooldown
rather than setting it aside. Rate-limited on a Tor exit, switch to
direct, switch back before the reset, and the app contacts that same
limited exit again with nothing left to stop it.
Tor and direct now carry their own deadlines. Switching route selects
the other one rather than forgetting this one, a success clears only the
route that succeeded, and when the route cannot be determined the
stricter of the two applies -- failing to identify a route must not
release a cooldown that is still running.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses review on #812.
isProxyEnabled() reports readiness, not route selection: it is false
while Tor is bootstrapping or restarting, even though requests will
still go through Tor. Using it as the route identity cleared a
Tor-earned gate mid-bootstrap and applied a direct-earned one to the
first Tor request -- the opposite of what scoping the gate was for. The
identity is now the selected mode from statusFlow.
The force flag turned out to be both unnecessary and harmful. Leaving
Downloading synchronously before the check already clears the entry
guard, so force only reached the completion guard -- which must stay
armed. A cancellation check can take a minute on the route timeout, and
WorkManager can surface Resumable meanwhile, so the user may start a new
download before it returns; force let the stale result overwrite work
that was running and strip the progress and stop controls. Removing it
restores that protection and needs no generation counter.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses review on #812.
Cancelling left the spinner up for as long as the status check took.
Forcing checkStatus() past its guard was not enough: resolveApkStatus()
calls checkForUpdate(), which reaches the network and can sit on the
60-second route timeout while Tor bootstraps. Nothing clears the state
in the meantime -- the cancelled job maps to Idle, which the observer
ignores -- so the stop button looked broken for the whole wait. The
state now leaves Downloading immediately and the check resolves it
afterwards.
The rate-limit gate was process-wide. GitHub counts unauthenticated
requests per IP, so a cooldown earned through a shared Tor exit was
being applied to a direct connection with an entirely different quota,
and vice versa -- potentially suppressing a usable route for an hour.
The gate now records which route earned it and is dropped when the
current route differs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses review on #812.
Pressing the new stop button before a partial file exists -- while
resolving the release, or waiting for Tor -- left the row disabled and
spinning for the lifetime of the ViewModel. Two guards conspired:
checkStatus() returns early while the state is Downloading, and the
downloader observer deliberately ignores the Idle that WorkManager
reports for a cancelled job. Both exist to stop a running job being
second-guessed from cache contents, and neither anticipated a job that
is no longer running.
checkStatus() takes a force flag, used only by cancellation, and clears
the stale progress along with the status.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
