From e029a7db6442ad16488e4a2150662bd0e6d3054c Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 2 Sep 2026 14:59:41 -0400 Subject: [PATCH] fix(frontend): every request carries a deadline, and expiry arrives as an error callers already handle (#3412) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rule 156, across the whole client. `apiGet`, `apiPost`, `apiPut`, `apiPatch` and `apiDelete` each called bare `fetch`, whose default is to wait as long as the browser will — not a long timeout but the absence of one. The only AbortController in the frontend belonged to the SSE stream and was for cancellation. So every request in the app could hang forever, and there is no state a surface can render for "pending forever" that is not a lie: the spinner that never resolves looks exactly like work still in progress. Found while building the version readout (#3329), which had to tell "the fetch failed" apart from "still loading" and could not. ONE REQUEST PATH. The five verbs were near-identical bodies; they now delegate to a single `request()` that owns the deadline, so a sixth verb cannot be added without one. 30s by default — long enough to clear a cold embedding call and a list view under pool contention (#2384), so tripping it means something is wrong rather than merely busy. Overridable per call via `timeoutMs`. EXPIRY IS AN ApiError, which is the half of rule 156 that is easy to skip. A raw `DOMException: TimeoutError` reaches `apiErrorMessage(e, fallback)` as an object with no `body`, so all ~330 existing catch sites would have printed their generic fallback and the timeout would have been invisible in exactly the situation it exists to expose. Rethrown as `ApiError` with a 408 — a status no Scribe route returns, so it unambiguously means the client gave up — every one of those call sites now reports it correctly, untouched. Only TimeoutError is converted. A deliberate cancellation aborts with AbortError and passes through: a caller that cancelled its own request does not want that surfaced as a server failure. Pinned by a test, because collapsing the two is the obvious "simplification". STREAMS RELOCATE THE DEADLINE RATHER THAN ESCAPING IT. A wall-clock timeout would kill a long-lived SSE connection mid-flight, but two different waits are involved and only one of them is the stream: the CONNECT can fail to answer and now carries a 15s deadline, cleared the moment headers arrive; the BODY stays unbounded on purpose, since its failure mode is going quiet, which a timeout cannot distinguish from being idle — that is what reconnection and Last-Event-ID are for. Reading the connect as exempt because "the stream is long-lived" leaves an unreachable server looking like a quiet one. BULK TRANSFERS get their own value, not the default. Backup, notes export and admin restore walk the whole store and 30s would cut them off mid-work; they carry 10 minutes. Bounded, not unbounded — rule 156 asks for a deadline, not a short one, and no ceiling at all is what leaves a restore that died server-side spinning forever. Four source-inspection guards in the unit lane (no frontend test runner): no bare fetch anywhere; the default is actually applied — pinning the specific regression, since #3329's opt-in shape would pass every other check while leaving 330 callers unbounded; expiry converts to ApiError; and cancellation does not. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01TcCs1CcQ1ormdnzSshKqvN --- frontend/src/api/client.ts | 166 +++++++++++++++++------ frontend/src/api/version.ts | 16 ++- frontend/src/views/SettingsView.vue | 15 +- tests/test_frontend_request_deadlines.py | 128 +++++++++++++++++ 4 files changed, 272 insertions(+), 53 deletions(-) create mode 100644 tests/test_frontend_request_deadlines.py diff --git a/frontend/src/api/client.ts b/frontend/src/api/client.ts index ad91d7a..2f4c110 100644 --- a/frontend/src/api/client.ts +++ b/frontend/src/api/client.ts @@ -53,57 +53,120 @@ export function apiErrorMessage(e: unknown, fallback: string): string { } /** - * A GET, optionally with a deadline. + * How long an ordinary JSON call may wait before it is declared failed. * - * `timeoutMs` is OPT-IN rather than defaulted, deliberately. Every existing - * caller was written against a `fetch` that waits as long as the browser will, - * and handing them all a deadline in one change would alter behaviour at every - * call site at once, including ones nobody has looked at. New callers should - * pass one. + * Rule 156: a wait with no deadline is a bug. `fetch`'s own default is to wait + * as long as the browser will, which is not a deadline — it is the absence of + * one, and it renders as a spinner that never resolves. There is no state a + * surface can show for "pending forever" that is not a lie. * - * Why a caller should want it: a wait with no deadline cannot report that it - * failed. It can only stay pending — which is indistinguishable, to anything - * rendering it, from "still loading". A surface that has to tell those two - * apart needs the request to give up on its own. + * 30s is chosen to be longer than anything healthy: it has to clear a cold + * embedding call and a list view under connection-pool contention (#2384 had + * /api/projects fanning 25 concurrent sessions at a 15-connection pool), so + * tripping it means something is genuinely wrong rather than merely busy. Slow + * BY DESIGN is a different case and passes its own value — see the callers in + * SettingsView that do. */ -export async function apiGet(path: string, opts?: { timeoutMs?: number }): Promise { - const res = await fetch( - path, - opts?.timeoutMs ? { signal: AbortSignal.timeout(opts.timeoutMs) } : undefined, +const DEFAULT_TIMEOUT_MS = 30_000; + +/** HTTP 408. Not a status any Scribe route returns, so it unambiguously means + * "the client gave up" rather than anything the server said. */ +const CLIENT_TIMEOUT_STATUS = 408; + +/** + * How long a STREAM may take to answer with its headers. + * + * Streams are the one case a wall-clock deadline would break: a long-lived SSE + * connection is *supposed* to stay open, and `AbortSignal.timeout` would kill + * it mid-flight along with the body. But that does not exempt them from rule + * 156 — it relocates the deadline. Two different waits are involved: + * + * connect — the server answering with headers. CAN fail to answer, so it + * carries this deadline, cleared the moment headers arrive. + * stream — the body, open indefinitely on purpose. Its failure mode is + * going quiet, which a timeout cannot tell from being idle; that + * is what reconnection and Last-Event-ID are for, not this. + * + * Reading the connect as exempt because "the stream is long-lived" is the easy + * mistake here, and it leaves an unreachable server looking like a quiet one. + */ +const STREAM_CONNECT_TIMEOUT_MS = 15_000; + +/** + * A signal that aborts if headers do not arrive in time, plus the `settle` to + * call once they do. After `settle()` the returned signal never fires, so the + * stream body runs unbounded — which is the intent. + */ +function connectDeadline(base: AbortSignal): { signal: AbortSignal; settle: () => void } { + const gate = new AbortController(); + const timer = setTimeout( + () => gate.abort(new DOMException("stream did not connect in time", "TimeoutError")), + STREAM_CONNECT_TIMEOUT_MS, ); + return { + signal: AbortSignal.any([base, gate.signal]), + settle: () => clearTimeout(timer), + }; +} + +export interface RequestOpts { + /** Override the deadline. Pass one when the call is slow BY DESIGN. */ + timeoutMs?: number; +} + +/** + * The one place a request is actually made — every verb below goes through + * here, so the deadline cannot be forgotten by adding a sixth. + * + * EXPIRY SURFACES AS AN `ApiError`, which is rule 156's second half: the + * failure has to arrive in the shape the caller already handles. A bare + * `DOMException: TimeoutError` would reach `apiErrorMessage(e, fallback)` as + * an object with no `body`, so every catch site in the app would report its + * generic fallback and the timeout would be invisible in the very situation it + * exists to expose. Rethrowing as `ApiError` means ~330 existing call sites + * report it correctly without being touched. + * + * Only a TIMEOUT is converted. A deliberate cancellation aborts with + * `AbortError` and is left alone — a caller that cancelled its own request + * does not want it reported as a server failure. + */ +async function request(path: string, init: RequestInit, opts?: RequestOpts): Promise { + const timeoutMs = opts?.timeoutMs ?? DEFAULT_TIMEOUT_MS; + let res: Response; + try { + res = await fetch(path, { ...init, signal: AbortSignal.timeout(timeoutMs) }); + } catch (e) { + if (e instanceof DOMException && e.name === "TimeoutError") { + throw new ApiError(CLIENT_TIMEOUT_STATUS, { + error: `The server did not answer within ${Math.round(timeoutMs / 1000)}s.`, + }); + } + throw e; + } return handleResponse(res, path); } -export async function apiPost(path: string, body: unknown): Promise { - const res = await fetch(path, { - method: "POST", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify(body), - }); - return handleResponse(res, path); +/** JSON body headers — the three write verbs sent an identical literal each. */ +const JSON_HEADERS = { "Content-Type": "application/json" }; + +export function apiGet(path: string, opts?: RequestOpts): Promise { + return request(path, {}, opts); } -export async function apiPut(path: string, body: unknown): Promise { - const res = await fetch(path, { - method: "PUT", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify(body), - }); - return handleResponse(res, path); +export function apiPost(path: string, body: unknown, opts?: RequestOpts): Promise { + return request(path, { method: "POST", headers: JSON_HEADERS, body: JSON.stringify(body) }, opts); } -export async function apiPatch(path: string, body: unknown): Promise { - const res = await fetch(path, { - method: "PATCH", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify(body), - }); - return handleResponse(res, path); +export function apiPut(path: string, body: unknown, opts?: RequestOpts): Promise { + return request(path, { method: "PUT", headers: JSON_HEADERS, body: JSON.stringify(body) }, opts); } -export async function apiDelete(path: string): Promise { - const res = await fetch(path, { method: "DELETE" }); - return handleResponse(res, path); +export function apiPatch(path: string, body: unknown, opts?: RequestOpts): Promise { + return request(path, { method: "PATCH", headers: JSON_HEADERS, body: JSON.stringify(body) }, opts); +} + +export function apiDelete(path: string, opts?: RequestOpts): Promise { + return request(path, { method: "DELETE" }, opts); } // --------------------------------------------------------------------------- @@ -238,7 +301,14 @@ export function apiSSEStream( } const done = (async () => { - const res = await fetch(path, { headers, signal: combinedSignal }); + // Bounded connect, unbounded stream — see STREAM_CONNECT_TIMEOUT_MS. + const connect = connectDeadline(combinedSignal); + let res: Response; + try { + res = await fetch(path, { headers, signal: connect.signal }); + } finally { + connect.settle(); + } if (!res.ok) { let body: Record = {}; try { @@ -335,11 +405,19 @@ export async function apiStreamPost( body: unknown, onChunk: (data: Record) => void ): Promise { - const res = await fetch(path, { - method: "POST", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify(body), - }); + // Bounded connect, unbounded stream — see STREAM_CONNECT_TIMEOUT_MS. + const connect = connectDeadline(new AbortController().signal); + let res: Response; + try { + res = await fetch(path, { + method: "POST", + headers: JSON_HEADERS, + body: JSON.stringify(body), + signal: connect.signal, + }); + } finally { + connect.settle(); + } if (!res.ok) { let errBody: Record = {}; try { diff --git a/frontend/src/api/version.ts b/frontend/src/api/version.ts index 1d239c0..79e89f0 100644 --- a/frontend/src/api/version.ts +++ b/frontend/src/api/version.ts @@ -27,13 +27,15 @@ export interface VersionPayload { } /** - * The readout exists to answer "what is running?" during an incident, which is - * exactly when the server may be the thing that is unwell. Without a deadline - * a failing instance leaves the request pending forever and the surface sits - * on "still loading" — a blank standing in for `unknown`, which is the failure - * mode #3127 checklist 12 names by hand. Eight seconds is long enough for a - * slow-but-alive instance and short enough that a person watching it learns - * something. + * SHORTER than the client's 30s default, deliberately. + * + * This readout answers "what is running?" during an incident, which is exactly + * when the server may be the thing that is unwell — and it is one static field + * off a route that does no work, so a healthy instance answers it immediately. + * Waiting the full default before saying so would leave a person staring at + * "still loading" for half a minute in the moment they are trying to find out + * whether the instance is alive at all. Eight seconds clears a slow-but-alive + * instance and tells them something quickly when it is not. */ const VERSION_TIMEOUT_MS = 8000; diff --git a/frontend/src/views/SettingsView.vue b/frontend/src/views/SettingsView.vue index 1a7e7cc..715c222 100644 --- a/frontend/src/views/SettingsView.vue +++ b/frontend/src/views/SettingsView.vue @@ -188,6 +188,16 @@ const changingPassword = ref(false); const invalidatingSessions = ref(false); const exporting = ref(false); const restoring = ref(false); +// Backup, export and restore walk the whole store, so they are slow BY DESIGN +// and the client's ordinary 30s default would cut them off mid-work. They are +// still bounded: rule 156 asks for a deadline, not a short one, and "no ceiling +// at all" is what leaves a restore that died server-side spinning forever. +const BULK_TRANSFER_TIMEOUT_MS = 10 * 60 * 1000; + +function bulkDeadline(): AbortSignal { + return AbortSignal.timeout(BULK_TRANSFER_TIMEOUT_MS); +} + // ── What's running (#3127 checklist 12) ───────────────────────────────── // Three states kept apart, because collapsing any two of them is the defect // this readout exists to remove: `null` + no error = not asked yet (the Config @@ -755,7 +765,7 @@ async function exportData(scope: "user" | "full") { exporting.value = true; try { const url = scope === "full" ? "/api/admin/backup" : "/api/admin/backup?scope=user"; - const res = await fetch(url); + const res = await fetch(url, { signal: bulkDeadline() }); if (!res.ok) { const body = await res.json().catch(() => ({ error: `Error ${res.status}` })); throw new Error((body as Record).error || `Error ${res.status}`); @@ -780,7 +790,7 @@ const exportingNotes = ref(false); async function exportNotes(format: "markdown" | "json") { exportingNotes.value = true; try { - const res = await fetch(`/api/export?format=${format}`); + const res = await fetch(`/api/export?format=${format}`, { signal: bulkDeadline() }); if (!res.ok) throw new Error(`Error ${res.status}`); const blob = await res.blob(); const ext = format === "json" ? "json" : "zip"; @@ -1009,6 +1019,7 @@ async function handleRestoreFile(event: Event) { method: "POST", headers: { "Content-Type": "application/json" }, body: JSON.stringify(data), + signal: bulkDeadline(), }); if (!res.ok) { const body = await res.json().catch(() => ({ error: `Error ${res.status}` })); diff --git a/tests/test_frontend_request_deadlines.py b/tests/test_frontend_request_deadlines.py new file mode 100644 index 0000000..65abd0d --- /dev/null +++ b/tests/test_frontend_request_deadlines.py @@ -0,0 +1,128 @@ +"""Every request the web UI makes has a deadline (rule 156). + +A source-inspection guard in the unit lane — there is no frontend test runner, +and this is a property of the source rather than of a rendered result, so +reading the source is the honest way to check it. + +WHY. `fetch`'s default is to wait as long as the browser will. That is not a +long timeout, it is the absence of one, and there is no state a surface can +render for "pending forever" that is not a lie — the spinner that never +resolves is indistinguishable from work still in progress. Rule 156 names +`fetch` specifically: + + When a library's default is "wait indefinitely" — `fetch`, most HTTP + clients, a bare `await` on a stream — supplying the deadline is part of + using it, not a hardening pass for later. + +Before this guard, no request in the app carried one. +""" +from __future__ import annotations + +import re +from pathlib import Path + +FRONTEND = Path(__file__).resolve().parents[1] / "frontend" / "src" +CLIENT = FRONTEND / "api" / "client.ts" + + +def _call_text(src: str, start: int) -> str: + """The source of one `fetch(...)` call, from its open paren to its close. + + Naive paren balancing. Adequate because every call site here passes an + object literal, and a construct complex enough to defeat it is one worth + looking at by hand anyway. + """ + depth = 0 + for i in range(start, len(src)): + if src[i] == "(": + depth += 1 + elif src[i] == ")": + depth -= 1 + if depth == 0: + return src[start:i + 1] + return src[start:] + + +def _fetch_calls() -> list[tuple[Path, str]]: + calls: list[tuple[Path, str]] = [] + for path in list(FRONTEND.rglob("*.ts")) + list(FRONTEND.rglob("*.vue")): + src = path.read_text() + for m in re.finditer(r"\bfetch\(", src): + calls.append((path, _call_text(src, m.end() - 1))) + return calls + + +def test_every_fetch_passes_a_signal(): + """No bare `fetch` anywhere in the frontend. + + Stated on the SIGNAL rather than on a timeout value, because the two + legitimate shapes here produce different values and only share this: an + ordinary call takes the client's default, a stream bounds its CONNECT and + then deliberately runs unbounded, and a bulk transfer passes minutes. What + they must all do is pass something. + """ + naked = [ + f"{path.relative_to(FRONTEND)}: {call[:70]}" + for path, call in _fetch_calls() + if "signal:" not in call + ] + assert not naked, ( + "these fetch calls carry no AbortSignal, so they wait forever " + "(rule 156):\n " + "\n ".join(naked) + ) + + +def test_the_client_applies_its_deadline_by_default(): + """The specific regression that would silently undo this. + + An earlier pass (#3329) made `timeoutMs` OPT-IN and used it at exactly one + call site, which left ~330 others waiting forever while the mechanism + looked present. Reverting to that shape would not fail the guard above — + every call would still reach `fetch` through `request()` — so the default + is pinned here separately. + + `??` is the operative character: `opts?.timeoutMs || DEFAULT` would treat + an explicit 0 as "use the default", and `opts?.timeoutMs` alone would + reinstate the opt-in bug. + """ + src = CLIENT.read_text() + assert "opts?.timeoutMs ?? DEFAULT_TIMEOUT_MS" in src, ( + "request() must fall back to DEFAULT_TIMEOUT_MS — without it the " + "deadline is opt-in again and almost nothing opts in" + ) + + +def test_a_timeout_arrives_as_the_error_shape_callers_already_handle(): + """Rule 156's second half: expiry surfaces as a NAMED failure. + + A raw `DOMException: TimeoutError` reaches `apiErrorMessage(e, fallback)` + as an object with no `body`, so every catch site in the app would print its + generic fallback and the timeout would be invisible in exactly the + situation it exists to expose. Rethrowing as `ApiError` is what makes the + other ~330 call sites report it without being edited. + """ + src = CLIENT.read_text() + assert 'e.name === "TimeoutError"' in src, ( + "request() must recognise a timeout specifically" + ) + assert "new ApiError(CLIENT_TIMEOUT_STATUS" in src, ( + "a timeout must be rethrown as ApiError so apiErrorMessage can read it" + ) + + +def test_a_deliberate_cancellation_is_not_reported_as_a_timeout(): + """Only `TimeoutError` is converted, never `AbortError`. + + A caller that cancelled its own request — a superseded search, a closed + stream — must not have that surfaced to the user as a server failure. The + guard is that the conversion is gated on the name, which the assertion + above already pins; this states the intent so the gate is not "simplified" + into catching every abort. + """ + src = CLIENT.read_text() + convert = src[src.index("async function request<"):] + convert = convert[:convert.index("\n}")] + assert "AbortError" not in convert, ( + "request() must not convert AbortError — a deliberate cancellation is " + "not a timeout" + )