If SQLite clearAll fails mid-panic, in-memory state was already cleared
but a process restart could reload encrypted history from disk (#699).
Fall back to deleting the database files before reporting failure.
Two problems Codex found in the first push.
"SOCKS connection error" is not a circuit failure. arti-build logs that
wrapper for every error out of handle_socks_connection, including local
protocol faults raised before a circuit is attempted — "Invalid SOCKS
handshake", "Unsupported SOCKS version", "Unsupported SOCKS command".
Anything on the device that speaks SOCKS badly to the local port would
have read as a dead Tor. Drop it. "Failed to connect through Tor" is
logged only on client.connect() failure, and "Failed to obtain exit
circuit" is arti's own detail for that error, which is what #610 shows —
so the reported lines are still matched.
The restart path cancels itself. restartArti stops Arti first, and
stopArtiInternal cancels retryJob and inactivityJob. Both existing
recovery paths call restartArti from inside one of those jobs, so the
coroutine is cancelled between the stop and the start and Arti never
comes back. That is already true on main for the start-failure and
bootstrap-inactivity paths; routing a post-bootstrap outage into it would
have turned a wrong indicator into a stopped Tor, which is worse.
Run restarts from a separate restartJob that the stop path leaves alone,
and cancel it explicitly when the user turns Tor off so a pending
recovery cannot resurrect it.
Uses the log lines from the report verbatim. Pins both directions: a
same-instant burst and occasional failures far apart leave the indicator
alone, while four failures still going after twenty seconds downgrade it.
Also covers reset and a backwards clock jump.
Arti's bootstrap milestones only fire on the way up. "We have found that
guard [scrubbed] is usable." latches the session to RUNNING at 100% and
calls stopInactivityMonitoring(), and from there nothing downgrades the
status except a deliberate stop or a state-file lock conflict.
So when the network goes away after bootstrap, Arti logs a continuous
stream of "Failed to connect through Tor" / "SOCKS connection error"
while the header keeps showing the connected colour, which is gated on
running && bootstrapPercent >= 100. isProxyEnabled() agrees, so requests
keep being routed into a SOCKS port that cannot reach an exit.
Track circuit failures after bootstrap and drop the published status
below 100% once they both accumulate and persist. Individual failures are
a normal part of Tor, so a burst inside one instant — which is what the
report shows, three threads failing in the same millisecond — is ignored;
it takes four failures still going twenty seconds later.
Downgrading to BOOTSTRAPPING rather than ERROR keeps running = true, so
callers fail closed through awaitSelectedRoute instead of leaking to
clearnet, the inactivity watchdog re-arms now that bootstrapPercent is
below 100, and the existing backoff retry gets to re-establish the
session. A successful re-bootstrap clears the tally and restores RUNNING.
The policy is kept free of Android and coroutine dependencies so the
thresholds are directly testable.
Refs #610
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>
