Skip to content

[Feat]: Configurable per-server network request timeout for remote models#778

Merged
a-ghorbani merged 13 commits into
mainfrom
feature/TASK-20260614-1334
Jun 15, 2026
Merged

[Feat]: Configurable per-server network request timeout for remote models#778
a-ghorbani merged 13 commits into
mainfrom
feature/TASK-20260614-1334

Conversation

@a-ghorbani

@a-ghorbani a-ghorbani commented Jun 14, 2026

Copy link
Copy Markdown
Owner

Summary

Adds a user-configurable network request timeout for remote / OpenAI-compatible model servers. Some self-hosted or remote LLMs have slow cold starts (>180s) or slow generation; the previously hard-coded 30s connection / 60s idle guards would abort those conversations prematurely. Users can now set a per-server timeout (e.g. 600s).

Closes #776.

What changed

  • ServerConfig.requestTimeoutMs — new optional per-server field. Absent on older persisted configs → falls back to existing defaults; no migration.
  • src/api/openai.ts — all four request helpers (streamChatCompletion, fetchModels, fetchModelsWithHeaders, testConnection) accept an optional timeoutMs. A single resolveTimeout helper is the only normalizer: undefined / ≤0 / NaN / non-finite → the existing default (30s connection, 60s idle). Stores, engine, and UI forward the raw stored value untouched.
  • Two-phase structure preserved — a supplied value overrides both the connection-phase and idle-phase guards; the idle timer still resets per chunk, so a healthy stream is never killed by a total wall-clock cap.
  • UI — a "Request timeout (seconds)" input on both the edit path (ServerDetailsSheet) and the add path (RemoteModelSheet); seconds↔ms conversion at the save boundary. Live connection probes honor the timeout: the in-edit field on add/edit, and the saved value on a known-server chip press.
  • ServerStore remains the sole writer of the field (addServer / updateServer).

Tests

  • Per-server timeout normalization (≤0/NaN/non-finite/undefined → default; positive passes through), raised-timeout success, default-on-unset, idle abort, and idle-reset-per-chunk (fake timers).
  • Engine forwards raw timeoutMs and rebuilds on reselect; ServerStore forwards undefined without crashing.
  • Both sheets: input render + s↔ms conversion; the two distinct probe feeds (in-edit field vs saved value) verified not cross-wired.
  • 301 affected tests pass; tsc --noEmit and lint clean. No native changes (no package.json / ios/ / android/).

Visual evidence

Both new "Request timeout (seconds)" inputs were captured on the iOS simulator (iPhone 17 Pro) and attached as a PR comment:

  • Add path (RemoteModelSheet, remote-timeout-input) — field appears after a probe attempt, with helper text.
  • Edit path (ServerDetailsSheet, server-details-timeout-input) — empty (default) and populated (600) states.

A local OpenAI-compatible mock server (GET /v1/models) was used so the probe succeeds and a server can be saved to reach the edit sheet. Screenshots are hosted as PR-comment images (no screenshots are committed to the source tree).

The "slow server succeeds with a raised timeout" runtime outcome still needs a real reachable slow remote server and remains a manual check.

Generated by PocketPal Dev Team

Add tests for the configurable network timeout across the remote-server
stack:
- openai.ts: timeout normalization (undefined/NaN/<=0/non-finite fall back
  to defaults; positive finite passes through), raised connection timeout,
  default-30s connection abort, configured idle abort, idle-timer-resets-
  per-chunk, and timeout forwarding through fetchModels/testConnection.
- OpenAICompletionEngine: forwards the constructed timeoutMs raw.
- ServerStore: forwards a saved requestTimeoutMs (and undefined) to
  fetchModels/testConnection without crashing.
- ModelStore.setRemoteModel: builds and rebuilds the engine carrying the
  server's requestTimeoutMs.
- ServerDetailsSheet: timeout input render/prefill, edit-time probe uses the
  in-edit field with fallback to the saved value, save converts seconds to ms
  and clears to undefined when empty.
- RemoteModelSheet: distinct probe feeds (in-edit field via
  fetchModelsWithHeaders for manual add vs saved requestTimeoutMs via
  fetchModels on chip press), plus add-server persistence.
Wrap the long json mock line; use advance-then-await for the two
fake-timer connection-timeout probes so the rejection settles after
the timer fires (satisfies jest/valid-expect without a deferred await).
@a-ghorbani a-ghorbani added the enhancement New feature or request label Jun 14, 2026
Adds Selectors.remoteModel.timeoutInput (remote-timeout-input) and
Selectors.serverDetails.timeoutInput (server-details-timeout-input) for
the per-server request timeout fields.
@a-ghorbani

a-ghorbani commented Jun 14, 2026

Copy link
Copy Markdown
Owner Author

Visual evidence — Request timeout (seconds) fields

Captured on iOS simulator (iPhone 17 Pro, iOS 26.0) for issue #776. Both new timeout inputs render with label and helper text.

Add path — RemoteModelSheet (testID remote-timeout-input)

Shown in the server-fields block after a connection probe.

Add path request timeout input

Edit path — ServerDetailsSheet (testID server-details-timeout-input)

Empty / default state:

Edit path request timeout input — default

Populated (600 seconds):

Edit path request timeout input — populated

The "slow server succeeds with a raised timeout" runtime behaviour still needs a real reachable slow server and remains a manual check.

Generated by PocketPal Dev Team

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a configurable, per-server network request timeout for remote/OpenAI-compatible model servers, allowing slow cold-start or slow-streaming servers to complete requests without being cut off by the existing fixed timeouts.

Changes:

  • Introduces ServerConfig.requestTimeoutMs and forwards it through stores/engine into OpenAI request helpers.
  • Adds a single normalization point (resolveTimeout) in src/api/openai.ts and applies the configured timeout to both connection and idle guards (while keeping per-chunk idle resets).
  • Adds UI inputs (add/edit flows) for “Request timeout (seconds)” with seconds↔ms conversion, plus unit/e2e test coverage for wiring and behavior.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/utils/types.ts Adds requestTimeoutMs to persisted server config shape.
src/store/ServerStore.ts Forwards per-server timeout to model fetch and connection test calls.
src/store/ModelStore.ts Passes server timeout into the remote OpenAI engine construction.
src/store/tests/ServerStore.test.ts Verifies timeout forwarding (and undefined passthrough) for store calls.
src/store/tests/ModelStore.test.ts Verifies engine rebuild/selection carries updated timeout values.
src/locales/en.json Adds i18n strings for the new timeout inputs.
src/components/ServerDetailsSheet/ServerDetailsSheet.tsx Adds edit-flow timeout input, prefill, save, and probe wiring.
src/components/ServerDetailsSheet/tests/ServerDetailsSheet.test.tsx Tests edit-sheet rendering, ms↔s conversion, and probe wiring.
src/components/RemoteModelSheet/RemoteModelSheet.tsx Adds add-flow timeout input, probe wiring, and persistence via addServer.
src/components/RemoteModelSheet/tests/RemoteModelSheet.test.tsx Tests add-flow probe feed vs chip feed and persistence conversions.
src/api/openai.ts Adds resolveTimeout and threads timeout through fetch + streaming helpers.
src/api/completionEngines.ts Extends OpenAICompletionEngine to carry/forward timeout to streaming.
src/api/tests/openai.test.ts Adds coverage for timeout normalization and stream/fetch timeout behavior.
src/api/tests/completionEngines.test.ts Ensures engine forwards constructed timeout argument.
e2e/helpers/selectors.ts Adds e2e selectors for the new timeout inputs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/components/ServerDetailsSheet/ServerDetailsSheet.tsx
Comment thread src/locales/en.json
Comment thread src/components/ServerDetailsSheet/ServerDetailsSheet.tsx Outdated
@a-ghorbani

Copy link
Copy Markdown
Owner Author

E2E run — all feature specs + quick-smoke (iOS sim + Android emulator)

Ran the 12 feature/smoke specs on iPhone 17 Pro simulator (iOS 26.0) and Android emulator-5554 (API 16) against this branch. App built fresh for both (iOS Release-iphonesimulator .app, Android app-e2e-releaseE2e.apk).

Spec iOS Android
quick-smoke
remote-server (this PR's surface)
draft-autosave
context-banner
download-cancel
hub-run
language
pal-greeting ⚠️ env
talent-tool-use ⚠️ env
thinking ⚠️ env ⚠️ env
thinking-pal-override ⚠️ env ⚠️ env
purchase-flow ⚠️ env ⚠️ env

The spec this PR actually touches — remote-server — passes on both platforms, as does quick-smoke. None of the ⚠️ failures are in code this PR changes.

Why the ⚠️ specs failed (environmental, not regressions)

  • thinking / thinking-pal-override / pal-greeting / talent-tool-use — these download a model (Qwen3-0.6B / 1.7B) on a fresh app install and assert on inference output (thinking bubble, greeting chip, tool-call HTML). The failures are NoSuchElement waiting on model download/load/inference to settle in time. pal-greeting and talent-tool-use pass on Android, confirming the feature works — the iOS misses are model-load/inference timing on a cold install.
  • purchase-flow — fails in the beforeEach hook with TypeError: fetch failed: it needs a reachable PalsHub backend + authenticated session, neither of which is wired in this local run.

Screenshots

Generated screenshots are hosted as release assets (same pattern as the timeout-field captures) and embedded below.

language — 11 locales, iOS (passing)

en
fa
he
id
ja
ko
ms
ru
uk
zh
zh_Hant

language — 11 locales, Android (passing)

en
fa
he
id
ja
ko
ms
ru
uk
zh
zh_Hant

pal-greeting / talent-tool-use — Android passing result + iOS failure state

pal-greeting (Android, passing):
pal-greeting android
pal-greeting (iOS, failure):
pal-greeting ios
talent-tool-use (Android, passing):
talent android
talent-tool-use (iOS, failure):
talent ios

thinking — failure state (iOS + Android)

thinking ios
thinking android

Generated by PocketPal Dev Team

The palsTab selector was switched to the drawer-item-pals testID so the
drawer open/close indicator survives a language switch, but navigateToPals
also taps it — and a Paper Drawer.Item does not reliably respond to a testID
tap on iOS, so the tap became a no-op and the drawer never closed. Tap the
visible Pals label instead (matching the other tabs); keep the testID for the
open/close indicator.
tapThinkingToggle offsets the tap to avoid the TTS VoiceChip, but the
offset could compute an x just past the toggle's edge (measured x=326 for a
257-322 button), so the synthetic tap missed and the toggle never flipped
(thinking.spec 'toggled off' assertion got true). Clamp x to the toggle
bounds so the avoidance offset can never land outside the button.
@a-ghorbani a-ghorbani marked this pull request as ready for review June 15, 2026 13:06
@a-ghorbani

Copy link
Copy Markdown
Owner Author

Update — earlier ⚠️ failures root-caused and fixed; suite now green

Following up on the E2E run above. That comment chalked the ⚠️ specs up to "model-load timing / environment" — that diagnosis was wrong. They were two E2E-harness tap bugs (not product or feature issues), now fixed on this branch, plus one backend that was simply down at the time.

Root causes

  1. Drawer tap no-op on iOS (pal-greeting, talent-tool-use, thinking-pal-override). The palsTab selector had been switched to the drawer-item-pals testID so the drawer open/close indicator survives a language switch — but navigateToPals also taps that selector, and a Paper Drawer.Item doesn't reliably respond to a testID tap on iOS, so the tap silently missed and the drawer never closed. Fix: tap the visible label (like the other tabs); keep the testID for the indicator. → fix(e2e): tap Pals drawer label so navigateToPals works on iOS
  2. Thinking-toggle tap miss (thinking, both platforms). The toggle tap offsets to avoid the adjacent TTS VoiceChip, but the offset could compute an x just past the toggle's edge, so the tap missed and the toggle never flipped. Fix: clamp the tap inside the button bounds. → fix(e2e): clamp thinking-toggle tap inside the button bounds
  3. purchase-flow — was failing only because the PalsHub e2e test backend was unreachable at the time. With it up, the authenticated checkout flow passes on iOS. (This spec is iOS-only — it uses -ios predicate string locators + ASWebAuthenticationSession.)

Re-run — all feature specs + quick-smoke (iPhone 17 Pro sim + emulator-5554)

Spec iOS Android
quick-smoke
remote-server (this PR's surface)
pal-greeting
talent-tool-use
thinking
thinking-pal-override
draft-autosave
context-banner
download-cancel
hub-run
language
purchase-flow n/a (iOS-only spec)

Every spec now passes on every platform it applies to. The two fixes are test-harness only (no src/ change), so the timeout feature itself is unchanged from the approved review.

Generated by PocketPal Dev Team

- placeholder said 'default 30s' but an empty field keeps two defaults
  (30s connection / 60s idle); reword to 'leave empty for default'
- extract the duplicated parseTimeoutMs into src/utils/timeout.ts, shared
  by ServerDetailsSheet and RemoteModelSheet
@a-ghorbani

Copy link
Copy Markdown
Owner Author

Thanks @copilot — addressed in 4979d336:

  • parseTimeoutMs duplication → extracted into src/utils/timeout.ts, now imported by both ServerDetailsSheet and RemoteModelSheet (byte-equivalent, no behavior change).
  • requestTimeoutPlaceholder "default 30s" → reworded to "Optional - leave empty for default", since an empty field keeps two defaults (30s connection / 60s idle), so a single number was misleading.

On the uncontrolled defaultValue timeout input in ServerDetailsSheet: left as-is intentionally. It mirrors the existing defaultValue={url} field directly above it, so it's consistent with the sheet's established pattern. The save-desync concern doesn't apply here — onChangeText={setTimeoutSeconds} keeps state in sync on every keystroke and handleSave reads that state. Switching only the timeout field to a controlled value would make it inconsistent with the URL field in the same sheet; converting both is out of scope for this change.

Generated by PocketPal Dev Team

@a-ghorbani a-ghorbani merged commit 65c846d into main Jun 15, 2026
5 checks passed
@a-ghorbani a-ghorbani deleted the feature/TASK-20260614-1334 branch June 15, 2026 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feat]: Configurable network request timeout for remote models

2 participants