bvandeusenandClaude Opus 5
e029a7db64
CI & Build / Plugin hooks (push) Successful in 11s
CI & Build / Python lint (push) Successful in 4s
CI & Build / TypeScript typecheck (push) Successful in 39s
CI & Build / integration (push) Successful in 34s
CI & Build / Python tests (push) Successful in 1m10s
CI & Build / Build & push image (push) Successful in 34s
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TcCs1CcQ1ormdnzSshKqvN
45 lines
2.0 KiB
TypeScript
45 lines
2.0 KiB
TypeScript
import { apiGet } from "./client";
|
|
|
|
/**
|
|
* What `/api/version` answers — the client's half of `build_version_payload`
|
|
* (`src/scribe/routes/api.py`), which is where the reasoning for the shape is
|
|
* written down.
|
|
*
|
|
* EVERY FIELD BUT `version` IS OPTIONAL, and an absent one means "this build
|
|
* does not know", not "empty". A local build has no ordering key and no
|
|
* channel, and the server says so by omitting the keys rather than sending
|
|
* `""` — emitting a placeholder would let it claim a position in an update
|
|
* order it is not part of.
|
|
*
|
|
* So a renderer must read ABSENCE, never falsiness. `build` is a number and
|
|
* `0` is a legitimate ordering key, so `v.build || "unknown"` would report a
|
|
* real value as unknown; `v.build ?? "unknown"` is the correct form.
|
|
*/
|
|
export interface VersionPayload {
|
|
/** The NAME — `YYYY.MM.DD.HHMM` from commit time. Answers "is this the same code?" */
|
|
version: string;
|
|
/** The ORDERING KEY — minutes since 2020-01-01, from build time. Absent on a local build. */
|
|
build?: number;
|
|
/** `dev` / `main` / a tag. Its own field, never folded into the name. */
|
|
channel?: string;
|
|
/** The commit the artifact was published under, so its claim can be checked against the registry. */
|
|
commit?: string;
|
|
}
|
|
|
|
/**
|
|
* 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;
|
|
|
|
export function fetchVersion(): Promise<VersionPayload> {
|
|
return apiGet<VersionPayload>("/api/version", { timeoutMs: VERSION_TIMEOUT_MS });
|
|
}
|