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" + )