fix(net): run API calls off the caller's dispatcher (#326) - #354
Merged
Conversation
Contributor
Android debug APKArtifact:
|
Nothing in the repository layer moved HTTP work off the caller's thread, so whether a response was handled on Main was decided by whichever scope launched the request. Ktor's engine does its socket I/O on engine threads, but the continuation resumes in the caller's context -- so body deserialization, the auth plugin's 401 branch, the token refresh it drives and that refresh's keystore-backed EncryptedSharedPreferences reads all ran wherever the call started. Six cold-start call sites launch from viewModelScope, which put all of that on the UI thread while the app was drawing its first frame. The hop goes inside safeApiCall rather than into each repository. There are 148 call sites across the 28 repository impls that use it, and only 4 of those inject a dispatcher today -- so the per-repository version is 24 new constructor parameters plus the DI and test churn, and it stays forgettable for the next repository added. A Ktor plugin was the other candidate: it would have to cover two pipelines rather than one -- the send, and the response pipeline that body() drives -- and it still would not cover the Room and DataStore work sitting in the same block as the call. The iOS ioDispatcher actual moves from Dispatchers.Default to Dispatchers.IO. Routing every API call through it makes that matter: on Native, Default is a worker pool sized to the core count and shared with applicationScope, so a cold-start fan-out would park every CPU worker on a socket. Native's IO is a separate pool; Room's iOS query context already uses it. Nine API invocations across five repositories do not go through safeApiCall because they classify their own failures -- a server validation message read off the response, a fallback to a cached value, or a first attempt whose failure is expected on a server with no CDN. Routing those through safeApiCall would log an expected failure at error level, so they take the hop explicitly: withContext(dispatcher) where the repository already injects one, and onApiDispatcher (the same hop without the mapping) where it does not. The invariant to hold from here: every API invocation sits inside safeApiCall, inside onApiDispatcher, or inside a flow that ends in flowOn. Streaming already satisfied it.
garfiec
force-pushed
the
fix/io-dispatcher-http
branch
from
August 13, 2026 07:10
d420146 to
db0876b
Compare
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.
Summary
Nothing in the repository layer moved HTTP work off the caller's thread, so whether a response was
handled on the main thread was decided entirely by whichever scope launched the request. Ktor's
OkHttp engine does its socket I/O on engine threads, but the continuation resumes in the caller's
context — so body deserialization, the auth plugin's
401branch, the token refresh that branchdrives and that refresh's keystore-backed
EncryptedSharedPreferencesreads all ran wherever thecall started. Several cold-start fetches launch from
viewModelScope, which put all of that on theUI thread while the app was drawing its first frame —
/api/bannerand/api/configfromNavHostViewModel,/api/convosand/api/projectsfromDrawerViewModel.Closes #326.
Changes
The hop goes inside
safeApiCall(:core:common), not into each repository as the issueproposed. There are 148 call sites across the 28 repository impls that use it, and only 4 of those
inject a dispatcher today, so per-repository would mean 24 new constructor parameters plus the DI
and test churn — and it would stay forgettable for the next repository added. A Ktor plugin was the
other candidate: it would have to cover two pipelines rather than one — the send, and the response
pipeline that
body()drives — and it still would not cover the Room and DataStore work sitting inthe same block as the call.
iOS: the
ioqualifier moves fromDispatchers.DefaulttoDispatchers.IO. Routing every APIcall through
ioDispatcheris what makes that matter. On Native,Dispatchers.Defaultis a workerpool sized to the core count and shared with
applicationScope, so without this the change wouldpark every CPU worker on a socket during a cold-start fan-out — a regression on iOS in exchange for
the Android fix. Native's
Dispatchers.IOis a separate pool, and Room's iOS query context alreadyuses it.
The hop wraps the whole
safeApiCallblock rather than only its network call, so a block that alsotouches Room or DataStore now does those reads and writes on the IO dispatcher too. That is a
widening beyond the title, and it is the right direction — those were the other things running on
the caller's thread.
onApiDispatcheris the same hop without the error mapping. Nine API invocations across fiverepositories don't go through
safeApiCall— they read a server validation message off the responsebody, fall back to a cached value, make a first attempt whose failure is expected, or throw rather
than returning a
Result— and routing those throughsafeApiCallwould put aLogger.eon a paththat fails by design. They take the hop explicitly instead:
withContext(dispatcher)where therepository already injects one (
ChatRepositoryImpl.checkStreamStatus,ConfigRepositoryImpl.fetchAndValidateConfig),onApiDispatcherwhere it does not(
FileRepositoryImpl.downloadFile,RoleRepositoryImpl.fetchUserRole, and skill create / update /upload / import).
Documentation.
core/common/CLAUDE.md'ssafeApiCallsnippet was fictional — it showed catchesfor
ClientRequestException/ServerResponseException/IOExceptionwith hardcoded strings thatthe function has never had — and is now the real implementation plus the reason the hop lives there.
core/data/CLAUDE.mdgains the invariant: every:core:networkAPI-service invocation sits insidesafeApiCall, insideonApiDispatcher, or inside a flow that ends in.flowOn(dispatcher).Streaming already satisfied it. A stale comment in
CommonTokenDataStorethat still described theproactive-renewal path as running on the UI thread is corrected.
Testing
Three new tests in
SafeApiCallTest; two of them pin the hop on theContinuationInterceptorratherthan on a thread name, so they assert the same thing on both platforms, and
runTestinstalls adispatcher of its own, so removing the hop fails them rather than passing by coincidence.
Full unit suite: 2070 tests, 0 failures. No existing test needed a dispatcher override, so no
test-only seam was added to production code.
./gradlew testDebugUnitTest :app:assembleDebug detektMetadataCommonMainpasses.:core:common:compileKotlinIosSimulatorArm64passes for the iOS dispatcher change.:core:common'siOS test compile fails on a pre-existing illegal test-method name in
SafeErrorMessageTest.kt,untouched by this branch. No iOS device or simulator run.
Coverage was checked mechanically rather than by inspection: every
*Api.method(...)invocation in:core:datawas walked back to its enclosing function and required to sit insidesafeApiCall,inside
onApiDispatcher, or inside a flow ending in.flowOn(dispatcher). Zero uncovered remain.Device pass
Pixel 10 Pro Fold AVD (API 37) against a local LibreChat server, replaying one snapshotted
dead-session cold start so both builds see identical state. Each build fingerprinted by dex symbol
before its run, since the emulator is shared. StrictMode's thread policy was armed temporarily
(
detectDiskReads/detectDiskWrites/detectCustomSlowCalls/detectNetwork,penaltyLog) —instrumentation only, not committed.
Main-thread log lines, counted by TID == PID in
logcat -v threadtime:develop(5738299)401 receivedon mainrefresh_rejectedon mainrefresh_proactiveon mainThe baseline reproduces the issue's original measurement (it reported 11 of 16). Both criteria pass.
StrictMode reported 11 main-thread violations on both builds, and on both every one attributes to
TokenDataStore.createEncryptedPrefs— theMasterKey/EncryptedSharedPreferencesconstruction atKoin startup — plus one
cacheDirread inDataPlatformModule. None come from the auth or HTTPpath on either build, which is the criterion; the construction-time ones are the out-of-scope item
noted below, and this change neither fixes nor worsens them.
Outcome behaviour is unchanged: 19 refresh legs entered, 19 settled
session expired, exactly onesession_torn_down, and the sameNewChat → ServerUrlscreen sequence — so #323'sone-teardown-per-session behaviour is preserved. The one delta is that more refresh POSTs complete
inside the 22 s capture window (11 → 21) with the same number of refresh legs entered; the retry loops
now actually run instead of being starved behind a blocked main thread.
Notes
safeApiCallbodies no longer run on the test dispatcher, so a mockeddelay()inside one is nowreal time rather than virtual (two 50 ms sleeps in
RepositoryCacheMutexTest). The shape thatwould actually break —
launch { repo.call() },advanceUntilIdle(), assert — appears in none ofthe 14 test files that construct a real repository, so no test-only dispatcher override was added
to production code.
EncryptedSharedPreferences.createand the initial token decryptstill run on the main thread during
TokenDataStoreconstruction atstartKoin. That is thelargest remaining item in the 401 handling and token refresh run on the Main thread during cold start #326 family; it is already flagged in a comment in
NavHostViewModel, and it isn't reachable from this seam because it happens before any request.CommonTokenDataStore's token-refresh POST is the one HTTP call in:core:datathat isn't anAPI-service method and so isn't covered by the invariant above. It is always driven from inside a
request that already took the hop.