Skip to content

fix: stop a stalled request from becoming a permanent spinner - #42

Open
blclo wants to merge 1 commit into
wildlife-reidfrom
fix/network-timeout-and-signin-spinner
Open

blclo wants to merge 1 commit into
wildlife-reidfrom
fix/network-timeout-and-signin-spinner

Conversation

@blclo

@blclo blclo commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

Summary

Found while testing the field workflow on a physical iPhone: tapping Download on the embedding pack showed a loading wheel that never stopped — no error, no alert, no way to retry, and no way to tell whether sign-in had even succeeded. Two independent defects combine to produce it.

ganeshaApiClient called fetch with no deadline. A connection that is accepted but never answered — a captive portal, a dropped cellular handover, a backend that stalls mid-response — leaves the promise pending forever, and every screen awaiting it spins indefinitely. Requests now abort after 30s and report a distinct timeout code.

timeout is deliberately separate from network-error: one means "no network", the other means "the server took the call and went quiet". Those want different reactions from someone standing in a reserve, and collapsing them hides which is happening. No call site switches exhaustively on GaneshaApiErrorCode, so the added variant is non-breaking.

SignInScreen.handleSignIn cleared its spinner on each exit path rather than in a finally, and the path after getUserProfile() had no protection at all. Combined with the missing timeout, a stalled profile lookup left the button spinning with the session already storedentraAuthService.signIn() writes tokens to the Keychain before the profile call runs. So the person is signed in and cannot tell. The spinner now clears in a finally, and a profile failure says the session was saved instead of "Sign-in failed", which was inviting a pointless second sign-in.

handleDownloadPack already had a correct try/finally — it was spinning because the awaited API call never settled, not because it leaked state. No change needed there.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)

Screenshots / Screen Recordings

No layout change. The visible difference is that a stalled request now ends in an alert naming the failure instead of an indefinite spinner.

Checklist

General

  • My code follows the project's coding style and conventions
  • I have performed a self-review of my code
  • I have added/updated comments where the logic isn't self-evident
  • My changes generate no new warnings or errors

Testing

  • Existing tests pass locally (npm test)
  • I have added tests that prove my fix is effective or my feature works
  • I have tested on Android (physical device or emulator)
  • I have tested on iOS (physical device or simulator)
  • I have tested in light mode and dark mode

6 new tests, 1037 passing overall, plus tsc --noEmit. The timeout test drives a fetch that never resolves and asserts the request aborts — it also documents a trap: the deadline is armed only after getValidAccessToken() resolves, so a synchronous jest.advanceTimersByTime fires against a timer that does not exist yet and the test hangs for the full 60s Jest limit. It uses advanceTimersByTimeAsync.

Device boxes are unchecked because the fix has not yet been confirmed against the original stalling iPhone — that is the next step, and it is what will reveal the underlying cause, since the app can finally report it.

React Native Specific

  • No new native module without corresponding platform implementation (Android + iOS)
  • No unnecessary re-renders introduced

Remaining items in this section are N/A — no native modules, no styling, no lists touched.

Security

  • No secrets, API keys, or credentials are included in the code
  • User input is validated/sanitized where applicable

Additional Notes

This does not identify the root cause, and does not claim to. It removes the failure mode where the cause is unknowable — a silent hang — and replaces it with a reported error. On the device that prompted this, nothing had reached disk: no staging file, no partial download, Documents/embedding_packs empty. So the stall was upstream of the transfer, in an API call, which is exactly what now gets a deadline. Whether the trigger is the network, the backend, or a Keychain read blocked while the device was locked, the app will now say so.

Diagnosis was harder than it should have been. utils/logger is __DEV__-gated, so a release build emits nothing at all. Establishing even this much required building a custom diagnostic IPA with the gate removed. Worth considering whether a release build should retain a minimal breadcrumb trail for field debugging — the whole point of this app is running where no debugger can follow.

Not fixed here

  • RNFS.downloadFile in fileDownloadService sets no connectionTimeout or readTimeout either. It is a different failure mode — it reports progress and honours an abort signal — and it was not implicated in this symptom, so it belongs in its own change.
  • getValidAccessToken() awaits react-native-app-auth's refresh() with no deadline. Same hazard class, on the auth path rather than the API path.

Found while testing the field workflow on a physical iPhone: tapping
Download on the embedding pack showed a loading wheel that never stopped,
with no error and no way to retry. Two independent defects combine to
produce that.

ganeshaApiClient called fetch with no deadline. A connection that is
accepted but never answered -- a captive portal, a dropped cellular
handover, a backend that stalls mid-response -- leaves the promise pending
forever. Every screen that awaits it spins indefinitely. Requests now abort
after 30s (generous, since these can be the first call after an Azure
Function cold start) and report a distinct `timeout` code, kept separate
from `network-error`: one means "no network", the other means "the server
took the call and went quiet", and those want different responses from
someone standing in a reserve.

SignInScreen.handleSignIn cleared its spinner on each exit path rather than
in a finally, and the path after getUserProfile() had no protection at all.
Combined with the missing timeout, a stalled profile lookup left the button
spinning with the session already stored -- so there was no way to tell
whether sign-in had succeeded. The spinner now clears in a finally, and a
profile failure says the session was saved rather than "Sign-in failed",
which was inviting a pointless second sign-in.

handleDownloadPack already had a correct try/finally; it was spinning
because the awaited API call never settled, not because it leaked state.

Not fixed here: RNFS.downloadFile sets no connection/read timeout either.
It is a different failure mode -- it reports progress and honours an abort
signal -- and on the reported symptom nothing had reached disk, no staging
file, so the stall was upstream of the transfer.

Co-Authored-By: Claude Opus 5 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant