diff --git a/__tests__/rntl/screens/SignInScreen.test.tsx b/__tests__/rntl/screens/SignInScreen.test.tsx index f05821ee3..28322ce19 100644 --- a/__tests__/rntl/screens/SignInScreen.test.tsx +++ b/__tests__/rntl/screens/SignInScreen.test.tsx @@ -133,6 +133,48 @@ describe('SignInScreen', () => { expect(mockGoBack).not.toHaveBeenCalled(); }); + // The spinner used to be cleared on each exit path rather than in a + // `finally`. A profile call that never settled -- ganeshaApiClient had no + // request timeout -- left the button spinning forever, with no error and no + // way to retry. In the field that looked like the app had simply frozen. + + it('clears the spinner after a failed profile lookup so sign-in can be retried', async () => { + jest.spyOn(Alert, 'alert').mockImplementation(() => {}); + mockSignIn.mockResolvedValue({ accessToken: 'a', refreshToken: 'r', idToken: 'i', accessTokenExpirationDate: '' }); + mockGetUserProfile.mockResolvedValue({ ok: false, code: 'timeout', message: 'Request timed out after 30s' }); + + const { getByTestId, queryByText } = render(); + fireEvent.press(getByTestId('sign-in-button')); + + await waitFor(() => expect(Alert.alert).toHaveBeenCalled()); + // The mocked Button renders " (loading)" while its spinner shows. + expect(queryByText(/\(loading\)/)).toBeNull(); + }); + + it('clears the spinner after a cancelled sign-in', async () => { + mockSignIn.mockRejectedValue(new Error('User cancelled flow')); + + const { getByTestId, queryByText } = render(<SignInScreen />); + fireEvent.press(getByTestId('sign-in-button')); + + await waitFor(() => expect(mockSignIn).toHaveBeenCalled()); + await waitFor(() => expect(queryByText(/\(loading\)/)).toBeNull()); + }); + + it('says the session was saved when only the profile lookup failed', async () => { + const alertSpy = jest.spyOn(Alert, 'alert').mockImplementation(() => {}); + mockSignIn.mockResolvedValue({ accessToken: 'a', refreshToken: 'r', idToken: 'i', accessTokenExpirationDate: '' }); + mockGetUserProfile.mockResolvedValue({ ok: false, code: 'timeout', message: 'Request timed out after 30s' }); + + const { getByTestId } = render(<SignInScreen />); + fireEvent.press(getByTestId('sign-in-button')); + + await waitFor(() => expect(alertSpy).toHaveBeenCalled()); + const [title, body] = alertSpy.mock.calls[0]; + expect(title).not.toMatch(/sign-in failed/i); + expect(body).toMatch(/session is saved/i); + }); + it('alerts on a real sign-in failure but not on a cancelled sign-in', async () => { const alertSpy = jest.spyOn(Alert, 'alert').mockImplementation(() => {}); mockSignIn.mockRejectedValue(new Error('User cancelled flow')); diff --git a/__tests__/unit/configureApp.test.ts b/__tests__/unit/configureApp.test.ts index 811245b00..83a6f4e4a 100644 --- a/__tests__/unit/configureApp.test.ts +++ b/__tests__/unit/configureApp.test.ts @@ -122,4 +122,27 @@ describe('deployment configuration', () => { configureApp(options); expect(fs.statSync(generated).mtimeMs).toBe(0); }); -}); \ No newline at end of file +}); + +describe('configureApp redirect normalisation', () => { + // Entra returns a custom-scheme redirect with a trailing slash appended. + // AppAuth-iOS compares the callback path against the configured redirect, so + // storing it without the slash makes iOS reject its own callback and hang. + // Android matches on scheme alone and is unaffected either way. + it('stores the redirect exactly as Entra returns it, with the trailing slash', () => { + const config = validateConfig({ ...example, redirectUrl: 'org.ganesha.elebook://oauthredirect/' }); + + expect(config.redirectUrl).toBe('org.ganesha.elebook://oauthredirect/'); + }); + + it('normalises the pre-fix spelling so existing configs self-heal', () => { + const config = validateConfig({ ...example, redirectUrl: 'org.ganesha.elebook://oauthredirect' }); + + expect(config.redirectUrl).toBe('org.ganesha.elebook://oauthredirect/'); + }); + + it('still rejects a redirect belonging to a different app', () => { + expect(() => validateConfig({ ...example, redirectUrl: 'different.app://oauthredirect/' })) + .toThrow(/redirectUrl/); + }); +}); diff --git a/__tests__/unit/deploymentConfig.test.ts b/__tests__/unit/deploymentConfig.test.ts index 215fc887c..5aea88b9d 100644 --- a/__tests__/unit/deploymentConfig.test.ts +++ b/__tests__/unit/deploymentConfig.test.ts @@ -22,7 +22,7 @@ const LONG_DEPLOYMENT = { tenantId: '77777777-7777-4777-8777-777777777777', mobileClientId: '88888888-8888-4888-8888-888888888888', apiClientId: '99999999-9999-4999-8999-999999999999', - redirectUrl: 'org.ganesha.elebook://oauthredirect', + redirectUrl: 'org.ganesha.elebook://oauthredirect/', }; describe('deployment token storage', () => { @@ -40,8 +40,14 @@ describe('deployment token storage', () => { it('derives the documented service name for the example deployment', () => { // Pinned on purpose: changing the derivation orphans tokens stored by earlier builds. + // + // This value moved once, when redirectUrl gained the trailing slash that + // iOS needs to accept its own OAuth callback. Anyone already signed in is + // signed out by that change and has to sign in again; observations, packs + // and the local database are untouched. iOS loses nothing, never having + // been able to sign in at all. expect(getTokenStorageService(example)).toBe( - 'org.ganesha.elebook.entra.92eda79296364b345b9cb882a37983eb15de4e4ac4d7c6aa511b6d2d5ed543ed', + 'org.ganesha.elebook.entra.df2facb18aac60dd0a72437d48a060371a0843065ae7fa0383d7a47f1d1a355b', ); }); diff --git a/__tests__/unit/services/entraAuthService.test.ts b/__tests__/unit/services/entraAuthService.test.ts index 0ae930a59..5782b7191 100644 --- a/__tests__/unit/services/entraAuthService.test.ts +++ b/__tests__/unit/services/entraAuthService.test.ts @@ -13,7 +13,11 @@ jest.mock('react-native-keychain', () => ({ import { authorize, refresh, revoke } from 'react-native-app-auth'; import * as Keychain from 'react-native-keychain'; -import { entraAuthService } from '../../../src/services/entraAuthService'; +import { + entraAuthService, + ENTRA_INTERACTIVE_TIMEOUT_MS, + ENTRA_REFRESH_TIMEOUT_MS, +} from '../../../src/services/entraAuthService'; import { ENTRA_ISSUER, ENTRA_MOBILE_CLIENT_ID, ENTRA_REDIRECT_URL, ENTRA_SCOPES } from '../../../src/config/entraAuth'; import { deploymentConfig, getTokenStorageService } from '../../../src/config/deployment'; @@ -234,3 +238,56 @@ describe('entraAuthService.getValidAccessToken', () => { expect(mockResetGenericPassword).toHaveBeenCalledWith({ service: TOKEN_SERVICE }); }); }); + +describe('entraAuthService deadlines', () => { + afterEach(() => { + jest.useRealTimers(); + }); + + // AppAuth bounds neither leg. On a marginal link the token exchange stalls + // after the browser has already closed, so authorize() never settles and the + // sign-in screen spins forever with nothing to report. + it('gives up on an interactive sign-in that never settles', async () => { + jest.useFakeTimers(); + mockAuthorize.mockImplementation(() => new Promise(() => {})); + + const pending = entraAuthService.signIn(); + const assertion = (async () => { + await expect(pending).rejects.toThrow(/timed out after 180s/); + })(); + await jest.advanceTimersByTimeAsync(ENTRA_INTERACTIVE_TIMEOUT_MS); + await assertion; + }); + + it('gives up on a token refresh that never settles', async () => { + jest.useFakeTimers(); + mockGetGenericPassword.mockResolvedValue({ + username: 'entra-tokens', + password: JSON.stringify({ + accessToken: 'stale', + refreshToken: 'refresh-me', + idToken: 'id', + accessTokenExpirationDate: new Date(Date.now() - 1000).toISOString(), + }), + }); + mockRefresh.mockImplementation(() => new Promise(() => {})); + + const pending = entraAuthService.getValidAccessToken(); + await jest.advanceTimersByTimeAsync(ENTRA_REFRESH_TIMEOUT_MS); + + // A refresh that cannot complete is reported as "no valid session" rather + // than thrown, matching how an expired refresh token is already handled. + await expect(pending).resolves.toBeNull(); + }); + + it('does not interfere with a sign-in that completes normally', async () => { + mockAuthorize.mockResolvedValue({ + accessToken: 'a', + refreshToken: 'r', + idToken: 'i', + accessTokenExpirationDate: new Date(Date.now() + 3_600_000).toISOString(), + }); + + await expect(entraAuthService.signIn()).resolves.toMatchObject({ accessToken: 'a' }); + }); +}); diff --git a/__tests__/unit/services/fileDownloadService.test.ts b/__tests__/unit/services/fileDownloadService.test.ts new file mode 100644 index 000000000..5de5d6fcd --- /dev/null +++ b/__tests__/unit/services/fileDownloadService.test.ts @@ -0,0 +1,138 @@ +jest.mock('react-native-fs', () => ({ + mkdir: jest.fn(() => Promise.resolve()), + unlink: jest.fn(() => Promise.resolve()), + stat: jest.fn(() => Promise.resolve({ size: 1000 })), + hash: jest.fn(() => Promise.resolve('a'.repeat(64))), + moveFile: jest.fn(() => Promise.resolve()), + downloadFile: jest.fn(), + stopDownload: jest.fn(), +})); + +import RNFS from 'react-native-fs'; +import { downloadFileWithIntegrityCheck } from '../../../src/services/fileDownloadService'; + +const mockDownloadFile = RNFS.downloadFile as jest.Mock; +const mockStopDownload = RNFS.stopDownload as jest.Mock; + +const target = { + source: { + url: 'https://example.org/model.onnx', + expectedSha256: 'a'.repeat(64), + expectedSizeBytes: 1000, + }, + stagingPath: '/mock/staging/model.onnx.part', + finalPath: '/mock/models/model.onnx', +}; + +describe('fileDownloadService inactivity timeout', () => { + beforeEach(() => { + jest.clearAllMocks(); + jest.useFakeTimers(); + }); + + afterEach(() => { + jest.useRealTimers(); + }); + + it('stops and reports a download that receives no data before the deadline', async () => { + mockDownloadFile.mockReturnValue({ + jobId: 42, + promise: new Promise(() => {}), + }); + + const pending = downloadFileWithIntegrityCheck(target, { + maxAttempts: 1, + inactivityTimeoutMs: 1000, + }); + await jest.advanceTimersByTimeAsync(1000); + + await expect(pending).resolves.toMatchObject({ + ok: false, + code: 'timeout', + message: 'download received no data for 1s', + }); + expect(mockStopDownload).toHaveBeenCalledWith(42); + expect(mockDownloadFile).toHaveBeenCalledWith( + expect.objectContaining({ + progressInterval: 1000, + readTimeout: 1000, + }), + ); + }); + + it('resets the deadline when download progress arrives', async () => { + let reportProgress: (() => void) | undefined; + let resolveDownload: + | ((result: { statusCode: number; bytesWritten: number }) => void) + | undefined; + mockDownloadFile.mockImplementation( + (options: { + progress?: (result: { + bytesWritten: number; + contentLength: number; + }) => void; + }) => { + reportProgress = () => + options.progress?.({ bytesWritten: 500, contentLength: 1000 }); + return { + jobId: 42, + promise: new Promise(resolve => { + resolveDownload = resolve; + }), + }; + }, + ); + + const pending = downloadFileWithIntegrityCheck(target, { + maxAttempts: 1, + inactivityTimeoutMs: 1000, + }); + await jest.advanceTimersByTimeAsync(750); + reportProgress?.(); + await jest.advanceTimersByTimeAsync(750); + expect(mockStopDownload).not.toHaveBeenCalled(); + + resolveDownload?.({ statusCode: 200, bytesWritten: 1000 }); + await expect(pending).resolves.toMatchObject({ ok: true }); + }); +}); + +describe('background transfer', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + // A foreground URLSession stops when iOS suspends the app, so locking the + // screen part-way through an 80MB model killed the transfer. Upstream had + // already removed the foreground path ("use background downloads + // exclusively"); this service was rewritten without the flag while + // AppDelegate kept servicing handleEventsForBackgroundURLSession. + it('asks for a background session so a locked screen does not kill the transfer', async () => { + mockDownloadFile.mockReturnValue({ + jobId: 1, + promise: Promise.resolve({ statusCode: 200, bytesWritten: 1000 }), + }); + + await downloadFileWithIntegrityCheck(target); + + expect(mockDownloadFile).toHaveBeenCalledWith( + expect.objectContaining({ background: true }), + ); + }); + + it('re-arms the inactivity deadline when the app returns to the foreground', async () => { + const { AppState } = require('react-native'); + const addEventListener = jest.spyOn(AppState, 'addEventListener'); + mockDownloadFile.mockReturnValue({ + jobId: 1, + promise: Promise.resolve({ statusCode: 200, bytesWritten: 1000 }), + }); + + await downloadFileWithIntegrityCheck(target); + + // Suspended JS timers fire late on wake; without this the watchdog would + // trip against a transfer that progressed fine while backgrounded. + expect(addEventListener).toHaveBeenCalledWith('change', expect.any(Function)); + addEventListener.mockRestore(); + }); +}); diff --git a/__tests__/unit/services/ganeshaApiClient.test.ts b/__tests__/unit/services/ganeshaApiClient.test.ts index 7e10314cb..babbbddb4 100644 --- a/__tests__/unit/services/ganeshaApiClient.test.ts +++ b/__tests__/unit/services/ganeshaApiClient.test.ts @@ -4,7 +4,7 @@ jest.mock('../../../src/services/entraAuthService', () => ({ entraAuthService: { getValidAccessToken: jest.fn() }, })); -import { ganeshaApiClient } from '../../../src/services/ganeshaApiClient'; +import { ganeshaApiClient, GANESHA_REQUEST_TIMEOUT_MS } from '../../../src/services/ganeshaApiClient'; import { entraAuthService } from '../../../src/services/entraAuthService'; const mockGetValidAccessToken = entraAuthService.getValidAccessToken as jest.Mock; @@ -310,3 +310,51 @@ describe('ganeshaApiClient.createUserProfile', () => { ); }); }); + +describe('ganeshaApiClient request deadline', () => { + afterEach(() => { + jest.useRealTimers(); + }); + + // Without a deadline a connection that is accepted but never answered leaves + // the promise pending forever, which the screens render as a permanent + // spinner with no error -- the iOS field symptom this guards against. + it('aborts and reports a timeout when the server never answers', async () => { + jest.useFakeTimers(); + mockFetch.mockImplementation( + (_url: string, init: { signal: AbortSignal }) => + new Promise((_resolve, reject) => { + init.signal.addEventListener('abort', () => reject(new Error('Aborted')), { once: true }); + }), + ); + + const pending = ganeshaApiClient.getUserProfile(); + // Async advance: the request awaits getValidAccessToken() before it ever + // arms the deadline, so a synchronous advance would fire against a timer + // that does not exist yet and the promise would hang. + await jest.advanceTimersByTimeAsync(GANESHA_REQUEST_TIMEOUT_MS); + const result = await pending; + + expect(result.ok).toBe(false); + expect(result).toMatchObject({ code: 'timeout' }); + }); + + it('passes an abort signal on every request', async () => { + mockFetch.mockResolvedValueOnce(jsonResponse(200, {})); + + await ganeshaApiClient.getUserProfile(); + + expect(mockFetch).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ signal: expect.anything() }), + ); + }); + + it('still reports a plain network error as network-error, not a timeout', async () => { + mockFetch.mockRejectedValueOnce(new Error('Network request failed')); + + const result = await ganeshaApiClient.getUserProfile(); + + expect(result).toMatchObject({ code: 'network-error', message: 'Network request failed' }); + }); +}); diff --git a/__tests__/unit/services/miewidModelManager.test.ts b/__tests__/unit/services/miewidModelManager.test.ts index 9f01ef98d..af90b414d 100644 --- a/__tests__/unit/services/miewidModelManager.test.ts +++ b/__tests__/unit/services/miewidModelManager.test.ts @@ -313,3 +313,62 @@ describe('checkEmbeddingModelCompatibility', () => { ); }); }); + +describe('prepareMiewidModel failure reporting', () => { + const SOURCE = { + name: 'miewid', + version: '4.2.0', + url: 'https://example.org/miewid-4.2.onnx', + expectedSha256: 'abc', + expectedSizeBytes: 2000, + format: 'onnx' as const, + }; + + beforeEach(() => { + jest.clearAllMocks(); + }); + + // 'missing' covers a stalled transfer, an unreachable host, a 404 and a + // cancellation alike. Collapsing them lost the only detail the person in the + // field could act on -- the screen said "status: missing" for an 80MB model + // download that had repeatedly stalled on a weak link. + it('keeps the underlying reason when a download fails', async () => { + mockDownloadModel.mockResolvedValue({ + ok: false, + code: 'timeout', + message: 'download received no data for 60s', + }); + + const candidate = await prepareMiewidModel(SOURCE); + + expect(candidate.status).toBe('missing'); + expect(candidate.failureReason).toBe('timeout: download received no data for 60s'); + }); + + it('keeps the verification detail when a download is corrupt', async () => { + mockDownloadModel.mockResolvedValue({ + ok: false, + code: 'checksum-mismatch', + message: 'expected abc, got def', + }); + + const candidate = await prepareMiewidModel(SOURCE); + + expect(candidate.status).toBe('corrupt'); + expect(candidate.failureReason).toBe('expected abc, got def'); + }); + + it('leaves the reason null on a model that prepared cleanly', async () => { + mockDownloadModel.mockResolvedValue({ + ok: true, + path: '/models/miewid.onnx', + sha256: 'abc', + sizeBytes: 1024, + }); + + const candidate = await prepareMiewidModel(SOURCE); + + expect(candidate.status).toBe('ready'); + expect(candidate.failureReason).toBeNull(); + }); +}); diff --git a/deployment.example.json b/deployment.example.json index c1055ecd1..21abc816b 100644 --- a/deployment.example.json +++ b/deployment.example.json @@ -4,5 +4,5 @@ "tenantId": "11111111-1111-4111-8111-111111111111", "mobileClientId": "22222222-2222-4222-8222-222222222222", "apiClientId": "33333333-3333-4333-8333-333333333333", - "redirectUrl": "org.ganesha.elebook://oauthredirect" -} \ No newline at end of file + "redirectUrl": "org.ganesha.elebook://oauthredirect/" +} diff --git a/docs/setup.md b/docs/setup.md index 98bde0925..8a1dab347 100644 --- a/docs/setup.md +++ b/docs/setup.md @@ -13,7 +13,7 @@ The app needs exactly six public strings. Start with [deployment.example.json](. "tenantId": "11111111-1111-4111-8111-111111111111", "mobileClientId": "22222222-2222-4222-8222-222222222222", "apiClientId": "33333333-3333-4333-8333-333333333333", - "redirectUrl": "org.ganesha.elebook://oauthredirect" + "redirectUrl": "org.ganesha.elebook://oauthredirect/" } ``` @@ -24,7 +24,7 @@ The app needs exactly six public strings. Start with [deployment.example.json](. | `tenantId` | Tenant UUID. | | `mobileClientId` | Native public-client registration UUID. | | `apiClientId` | API registration UUID. | -| `redirectUrl` | Exactly `org.ganesha.elebook://oauthredirect`, matching the native registrations. | +| `redirectUrl` | Exactly `org.ganesha.elebook://oauthredirect/`, matching the native registrations. The trailing slash is required: Entra appends it to a custom-scheme redirect that has no path, and AppAuth-iOS compares the callback path against this value, so omitting it makes iOS reject its own callback and hang. The pre-fix spelling without the slash is accepted and normalised. | Pack acquisition has a narrower limit: project IDs and pack version strings must fit within 40 UTF-16 code units. For live setup, keep `projectId` within that limit even though configuration accepts up to 128 characters; see [pack candidate paths](../src/services/packDownloadService/candidate.ts). Identities are digested into short directory names, so their length does not affect on-device paths. diff --git a/scripts/configure-app.js b/scripts/configure-app.js index 1066cfc9f..7183a6e22 100644 --- a/scripts/configure-app.js +++ b/scripts/configure-app.js @@ -10,7 +10,29 @@ const CONFIG_FIELDS = [ 'redirectUrl', ]; const UUID = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; -const NATIVE_REDIRECT = 'org.ganesha.elebook://oauthredirect'; +/** + * The trailing slash is load-bearing on iOS, and is not cosmetic. + * + * Entra normalises a custom-scheme redirect that has no path, so a request + * sent as `org.ganesha.elebook://oauthredirect` comes back as + * `org.ganesha.elebook://oauthredirect/`. AppAuth-iOS compares the callback + * against the configured redirect component by component, including path, so + * '' != '/' and it rejects the callback. It then discards the rejection -- + * OIDExternalUserAgentIOS ignores the BOOL from + * resumeExternalUserAgentFlowWithURL: -- and the authorization session waits + * forever. Sign-in spins with no error and no way to retry. + * + * Android never noticed: its intent filter matches on the scheme alone + * (appAuthRedirectScheme in android/app/build.gradle), so the extra slash is + * irrelevant there. Storing the redirect exactly as Entra returns it makes the + * comparison succeed on iOS and changes nothing on Android. + * + * Confirmed on an iPhone 15 Pro: the callback arrives as + * `org.ganesha.elebook://oauthredirect/?code=...` with path '/'. + */ +const NATIVE_REDIRECT = 'org.ganesha.elebook://oauthredirect/'; +/** Pre-fix spelling. Accepted and normalised so existing configs self-heal. */ +const NATIVE_REDIRECT_LEGACY = 'org.ganesha.elebook://oauthredirect'; function validateApiUrl(value) { let url; @@ -49,6 +71,9 @@ function validateConfig(value) { if (!/^[a-z0-9][a-z0-9_-]{0,127}$/i.test(config.projectId)) { throw new Error('projectId must contain only letters, digits, underscores, and hyphens.'); } + if (config.redirectUrl === NATIVE_REDIRECT_LEGACY) { + config.redirectUrl = NATIVE_REDIRECT; + } if (config.redirectUrl !== NATIVE_REDIRECT) { throw new Error('redirectUrl must match the native redirect registered by this app.'); } diff --git a/src/screens/PacksScreen.tsx b/src/screens/PacksScreen.tsx index 78c8561c4..264ab4af5 100644 --- a/src/screens/PacksScreen.tsx +++ b/src/screens/PacksScreen.tsx @@ -158,7 +158,9 @@ export const PacksScreen: React.FC = () => { if (modelForPack.status !== 'ready') { Alert.alert( 'Download failed', - `The MiewID model could not be prepared (status: ${modelForPack.status}).`, + modelForPack.failureReason + ? `The MiewID model could not be prepared: ${modelForPack.failureReason}` + : `The MiewID model could not be prepared (status: ${modelForPack.status}).`, ); return; } diff --git a/src/screens/SignInScreen.tsx b/src/screens/SignInScreen.tsx index 9ab24758e..1c579f978 100644 --- a/src/screens/SignInScreen.tsx +++ b/src/screens/SignInScreen.tsx @@ -29,42 +29,52 @@ export const SignInScreen: React.FC = () => { const handleSignIn = useCallback(async () => { setIsSigningIn(true); + // The spinner is cleared in `finally`, not on each exit path. Clearing it + // per-branch meant any early return added later -- or a profile call that + // never settled -- left it spinning with no error and no way to retry. try { - await entraAuthService.signIn(); - } catch (error) { - const message = error instanceof Error ? error.message : String(error); - logger.warn('[SignInScreen] Sign-in failed or was cancelled:', message); - // A cancelled sign-in (user backed out of the browser) is not an - // error worth alerting about -- only surface a message for real - // failures the person might be able to act on. - if (!/cancel/i.test(message)) { - Alert.alert('Sign-in failed', message); + try { + await entraAuthService.signIn(); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + logger.warn('[SignInScreen] Sign-in failed or was cancelled:', message); + // A cancelled sign-in (user backed out of the browser) is not an + // error worth alerting about -- only surface a message for real + // failures the person might be able to act on. + if (!/cancel/i.test(message)) { + Alert.alert('Sign-in failed', message); + } + return; } - setIsSigningIn(false); - return; - } - const profileResult = await ganeshaApiClient.getUserProfile(); - setIsSigningIn(false); + const profileResult = await ganeshaApiClient.getUserProfile(); - if (profileResult.ok) { - if (navigation.canGoBack()) { - navigation.goBack(); - } else { - navigation.replace('Main'); + if (profileResult.ok) { + if (navigation.canGoBack()) { + navigation.goBack(); + } else { + navigation.replace('Main'); + } + return; } - return; - } - if (profileResult.code === 'not-found') { - // First sign-in for this identity -- mirrors the web app's - // select-role step. Replace, not push, so a later "back" from - // SelectRole doesn't return to a completed SignIn screen. - navigation.replace('SelectRole'); - return; - } + if (profileResult.code === 'not-found') { + // First sign-in for this identity -- mirrors the web app's + // select-role step. Replace, not push, so a later "back" from + // SelectRole doesn't return to a completed SignIn screen. + navigation.replace('SelectRole'); + return; + } - Alert.alert('Sign-in failed', `Could not load your profile: ${profileResult.message}`); + // The session itself is already stored at this point, so say so -- + // otherwise "Sign-in failed" invites a pointless second sign-in. + Alert.alert( + 'Signed in, but your profile could not be loaded', + `${profileResult.message}. Your session is saved -- try again from Settings.`, + ); + } finally { + setIsSigningIn(false); + } }, [navigation]); return ( diff --git a/src/services/entraAuthService.ts b/src/services/entraAuthService.ts index d4d0ec3fe..fabe0a39d 100644 --- a/src/services/entraAuthService.ts +++ b/src/services/entraAuthService.ts @@ -22,6 +22,37 @@ export interface StoredEntraTokens { accessTokenExpirationDate: string; } +/** + * AppAuth has no deadline of its own on either leg of the flow, and on a + * marginal link the token exchange is where it stalls: the browser closes, the + * authorization code comes back, and the POST that trades it for tokens never + * completes. `authorize()` then never settles, so the screen spins forever with + * nothing to report. Measured in the field on a link at -74dBm with 40% beacon + * loss, which is an ordinary reserve connection, not an edge case. + * + * The interactive budget is deliberately long: it has to cover reading a + * password manager and approving an MFA push on another device, so cutting it + * short would fail people who were succeeding. It exists to bound the hang, not + * to be hit in normal use. Refresh is non-interactive and gets far less. + */ +export const ENTRA_INTERACTIVE_TIMEOUT_MS = 180_000; +export const ENTRA_REFRESH_TIMEOUT_MS = 30_000; + +async function withDeadline<T>(operation: Promise<T>, timeoutMs: number, label: string): Promise<T> { + let timer: ReturnType<typeof setTimeout> | undefined; + const deadline = new Promise<never>((_resolve, reject) => { + timer = setTimeout( + () => reject(new Error(`${label} timed out after ${timeoutMs / 1000}s`)), + timeoutMs, + ); + }); + try { + return await Promise.race([operation, deadline]); + } finally { + clearTimeout(timer); + } +} + const authConfig: AuthConfiguration = { issuer: ENTRA_ISSUER, clientId: ENTRA_MOBILE_CLIENT_ID, @@ -43,7 +74,11 @@ const authConfig: AuthConfiguration = { class EntraAuthService { /** Runs the interactive sign-in flow (opens the system browser) and stores the resulting tokens. */ async signIn(): Promise<StoredEntraTokens> { - const result = await authorize(authConfig); + const result = await withDeadline( + authorize(authConfig), + ENTRA_INTERACTIVE_TIMEOUT_MS, + 'Sign-in', + ); const tokens: StoredEntraTokens = { accessToken: result.accessToken, refreshToken: result.refreshToken || null, @@ -104,7 +139,11 @@ class EntraAuthService { } try { - const refreshed = await refresh(authConfig, { refreshToken: tokens.refreshToken }); + const refreshed = await withDeadline( + refresh(authConfig, { refreshToken: tokens.refreshToken }), + ENTRA_REFRESH_TIMEOUT_MS, + 'Token refresh', + ); const newTokens: StoredEntraTokens = { accessToken: refreshed.accessToken, // Entra does not always return a new refresh token on a refresh diff --git a/src/services/fileDownloadService/index.ts b/src/services/fileDownloadService/index.ts index 6165b6505..4836aec26 100644 --- a/src/services/fileDownloadService/index.ts +++ b/src/services/fileDownloadService/index.ts @@ -1,3 +1,4 @@ +import { AppState } from 'react-native'; import RNFS from 'react-native-fs'; import type { DownloadErrorCode, @@ -21,9 +22,11 @@ import logger from '../../utils/logger'; const DEFAULT_MAX_ATTEMPTS = 3; const DEFAULT_BASE_BACKOFF_MS = 1000; +export const DEFAULT_DOWNLOAD_INACTIVITY_TIMEOUT_MS = 60_000; const RETRYABLE_CODES: ReadonlySet<DownloadErrorCode> = new Set([ 'network-error', + 'timeout', 'length-mismatch', 'checksum-mismatch', ]); @@ -69,6 +72,53 @@ async function cleanupStaging(stagingPath: string): Promise<void> { } } +interface DownloadInactivityWatchdog { + promise: Promise<never>; + reset: () => void; + didTimeout: () => boolean; + clear: () => void; +} + +function createDownloadInactivityWatchdog( + timeoutMs: number, + onTimeout: () => void, +): DownloadInactivityWatchdog { + let timer: ReturnType<typeof setTimeout> | undefined; + let timedOut = false; + let rejectTimeout: (error: Error) => void = () => {}; + const promise = new Promise<never>((_resolve, reject) => { + rejectTimeout = reject; + }); + const reset = () => { + clearTimeout(timer); + timer = setTimeout(() => { + timedOut = true; + onTimeout(); + rejectTimeout( + new Error(`download received no data for ${timeoutMs / 1000}s`), + ); + }, timeoutMs); + }; + return { + promise, + reset, + didTimeout: () => timedOut, + clear: () => clearTimeout(timer), + }; +} + +function downloadFailure( + error: unknown, + signal: AbortSignal | undefined, + timedOut: boolean, +): DownloadOutcome { + if (signal?.aborted) { + return failure('cancelled', 'download cancelled'); + } + const message = error instanceof Error ? error.message : String(error); + return failure(timedOut ? 'timeout' : 'network-error', message); +} + /** Where a download comes from and where it lands, staged and final. */ export interface DownloadTarget { source: DownloadSource; @@ -86,20 +136,57 @@ async function attemptDownload( await cleanupStaging(stagingPath); let contentLengthFromServer = 0; - const { jobId, promise } = RNFS.downloadFile({ + const inactivityTimeoutMs = + opts.inactivityTimeoutMs ?? DEFAULT_DOWNLOAD_INACTIVITY_TIMEOUT_MS; + let jobId = -1; + const stopDownload = () => { + if (jobId >= 0) { + RNFS.stopDownload(jobId); + } + }; + const watchdog = createDownloadInactivityWatchdog( + inactivityTimeoutMs, + stopDownload, + ); + const download = RNFS.downloadFile({ fromUrl: source.url, toFile: stagingPath, headers: source.headers, - progressDivider: 5, + progressInterval: 1000, + // A foreground session stops the moment iOS suspends the app, so the screen + // locking part-way through an 80MB model was enough to kill the transfer -- + // no data would arrive, and the watchdog below would correctly but uselessly + // report a stall. Upstream removed the foreground path for this reason + // ("use background downloads exclusively"); the rewrite of this service lost + // the flag while AppDelegate kept handling + // handleEventsForBackgroundURLSession for a session nothing was asking for. + // Ignored on Android, which has its own long-running download path. + background: true, begin: (res: { contentLength: number }) => { contentLengthFromServer = res.contentLength; + watchdog.reset(); }, progress: (res: { bytesWritten: number; contentLength: number }) => { + watchdog.reset(); opts.onProgress?.(res.bytesWritten, res.contentLength); }, + readTimeout: inactivityTimeoutMs, + }); + jobId = download.jobId; + const { promise } = download; + watchdog.reset(); + + // JS timers do not run while iOS has the app suspended, so a watchdog armed + // before suspension fires the instant the app wakes -- against a background + // transfer that may have been progressing the whole time. Re-arm on wake and + // judge inactivity from then, not from whenever the app went away. + const appStateSubscription = AppState.addEventListener('change', nextState => { + if (nextState === 'active') { + watchdog.reset(); + } }); - const onAbort = () => RNFS.stopDownload(jobId); + const onAbort = stopDownload; if (opts.signal?.aborted) { // The signal aborted before we could listen — an aborted signal never // fires 'abort' again, so stop the job directly. @@ -110,17 +197,13 @@ async function attemptDownload( let statusCode: number; try { - const result = await promise; + const result = await Promise.race([promise, watchdog.promise]); statusCode = result.statusCode; } catch (error) { - if (opts.signal?.aborted) { - return failure('cancelled', 'download cancelled'); - } - return failure( - 'network-error', - error instanceof Error ? error.message : String(error), - ); + return downloadFailure(error, opts.signal, watchdog.didTimeout()); } finally { + watchdog.clear(); + appStateSubscription.remove(); opts.signal?.removeEventListener('abort', onAbort); } diff --git a/src/services/fileDownloadService/types.ts b/src/services/fileDownloadService/types.ts index 2a5a77a37..af3f8d181 100644 --- a/src/services/fileDownloadService/types.ts +++ b/src/services/fileDownloadService/types.ts @@ -1,6 +1,7 @@ export type DownloadErrorCode = | 'http-error' | 'network-error' + | 'timeout' | 'length-mismatch' | 'checksum-mismatch' | 'cancelled' @@ -18,6 +19,8 @@ export type DownloadOutcome = export interface DownloadOptions { onProgress?: (bytesWritten: number, contentLength: number) => void; signal?: AbortSignal; + /** Maximum time without download activity before stopping the native job. */ + inactivityTimeoutMs?: number; /** Total attempts including the first (default 3). */ maxAttempts?: number; /** Base for exponential backoff with full jitter (default 1000 ms). */ diff --git a/src/services/ganeshaApiClient/index.ts b/src/services/ganeshaApiClient/index.ts index fd4dfa94c..e595c33c0 100644 --- a/src/services/ganeshaApiClient/index.ts +++ b/src/services/ganeshaApiClient/index.ts @@ -32,6 +32,18 @@ import logger from '../../utils/logger'; * needs different headers (`x-ms-blob-type`) than any call here. See * `services/syncEngine` for that step. */ +/** + * Every request gets a deadline. Without one, 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. + * Callers render that as a spinner with no way out and no error to report, + * which is exactly what it looked like in the field on iOS. + * + * 30s is deliberately generous: these calls are small JSON reads, but they + * can be the first request after an Azure Function cold start. + */ +export const GANESHA_REQUEST_TIMEOUT_MS = 30_000; + class GaneshaApiClient { async getLatestModel(modelName: string): Promise<GaneshaApiResult<LatestModelInfo>> { return this.request<LatestModelInfo>(`/models/${encodeURIComponent(modelName)}/latest`); @@ -81,6 +93,8 @@ class GaneshaApiClient { } let response: Response; + const controller = new AbortController(); + const deadline = setTimeout(() => controller.abort(), GANESHA_REQUEST_TIMEOUT_MS); try { response = await fetch(`${GANESHA_API_BASE_URL}${path}`, { method: init.method ?? 'GET', @@ -89,11 +103,23 @@ class GaneshaApiClient { ...(init.body !== undefined ? { 'Content-Type': 'application/json' } : {}), }, ...(init.body !== undefined ? { body: JSON.stringify(init.body) } : {}), + signal: controller.signal, }); } catch (error) { + // A timeout surfaces as an AbortError from the signal above. Report it + // separately from a refused connection: one means "no network", the + // other means "the server took the call and went quiet", and the person + // in the field can act on that difference. + if (controller.signal.aborted) { + const message = `Request timed out after ${GANESHA_REQUEST_TIMEOUT_MS / 1000}s`; + logger.warn(`[GaneshaApiClient] ${path} timed out`); + return { ok: false, code: 'timeout', message }; + } const message = error instanceof Error ? error.message : String(error); logger.warn(`[GaneshaApiClient] Network error for ${path}: ${message}`); return { ok: false, code: 'network-error', message }; + } finally { + clearTimeout(deadline); } const errorCode = this.errorCodeFor(response.status); diff --git a/src/services/ganeshaApiClient/types.ts b/src/services/ganeshaApiClient/types.ts index 3d7daa1f1..bf7cc7ee8 100644 --- a/src/services/ganeshaApiClient/types.ts +++ b/src/services/ganeshaApiClient/types.ts @@ -93,6 +93,7 @@ export interface CreateUserProfilePayload { */ export type GaneshaApiErrorCode = | 'network-error' + | 'timeout' | 'unauthenticated' | 'unauthorized' | 'not-found' diff --git a/src/services/miewidModelManager/index.ts b/src/services/miewidModelManager/index.ts index fd8259204..f46c9afd5 100644 --- a/src/services/miewidModelManager/index.ts +++ b/src/services/miewidModelManager/index.ts @@ -51,6 +51,7 @@ const candidateRecord = ( status: 'downloading', verifiedAt: null, format: source.format, + failureReason: null, ...overrides, }); @@ -63,8 +64,9 @@ export async function prepareMiewidModel( try { outcome = await modelDownloadService.downloadModel(source, opts); } catch (error) { + const message = error instanceof Error ? error.message : String(error); logger.error('[MiewIDModelManager] Model preparation threw:', error); - return candidateRecord(source, { status: 'missing' }); + return candidateRecord(source, { status: 'missing', failureReason: message }); } if (outcome.ok) { @@ -80,12 +82,18 @@ export async function prepareMiewidModel( logger.error( `[MiewIDModelManager] Downloaded model failed verification: ${outcome.message}`, ); - return candidateRecord(source, { status: 'corrupt' }); + return candidateRecord(source, { + status: 'corrupt', + failureReason: outcome.message, + }); } logger.warn( `[MiewIDModelManager] Model preparation failed (${outcome.code}): ${outcome.message}`, ); - return candidateRecord(source, { status: 'missing' }); + return candidateRecord(source, { + status: 'missing', + failureReason: `${outcome.code}: ${outcome.message}`, + }); } /** diff --git a/src/types/wildlife.ts b/src/types/wildlife.ts index ac2cce840..966ed8749 100644 --- a/src/types/wildlife.ts +++ b/src/types/wildlife.ts @@ -30,6 +30,15 @@ export interface MiewIDModelRecord { status: MiewIDModelStatus; verifiedAt: string | null; format: ModelFormat; + /** + * Why a non-ready record ended up that way, for display. 'missing' covers + * every failure that is not an integrity mismatch -- a stalled transfer, an + * unreachable host, a 404, a cancellation -- and collapsing those into one + * word leaves the person in the field with nothing to act on. Null when the + * model is ready or still downloading, and absent on records written by + * builds from before it existed. + */ + failureReason?: string | null; } // === Embedding Pack Types ===