From d2c495441e5fe283b3c55f1414019b37d849bc71 Mon Sep 17 00:00:00 2001 From: Alexey Grigorev Date: Mon, 9 Nov 2026 11:42:16 +0100 Subject: [PATCH 1/5] Give app-shell API requests a timeout, visible failure message and server report --- frontend/src/shell/api.js | 61 ++++++++++++++++++++++++++++--- frontend/test/api-client.test.mjs | 59 ++++++++++++++++++++++++++++++ 2 files changed, 115 insertions(+), 5 deletions(-) diff --git a/frontend/src/shell/api.js b/frontend/src/shell/api.js index 827c2f1ab..2217076da 100644 --- a/frontend/src/shell/api.js +++ b/frontend/src/shell/api.js @@ -1,3 +1,9 @@ +import { + REQUEST_TIMEOUT_MS, + failureMessage, + reportClientFailure, +} from "../core/guarded-fetch.js"; + export function resolveApiBase({ documentRef, windowRef }) { const meta = documentRef .querySelector('meta[name="api-base"]') @@ -9,7 +15,12 @@ export function resolveApiBase({ documentRef, windowRef }) { return ""; } -export function createApiClient({ apiBase, fetchImpl, storage }) { +export function createApiClient({ + apiBase, + fetchImpl, + storage, + timeoutMs = REQUEST_TIMEOUT_MS, +}) { function apiUrl(path) { return new URL(path, apiBase); } @@ -53,10 +64,38 @@ export function createApiClient({ apiBase, fetchImpl, storage }) { headers.Authorization = `Bearer ${token}`; } const { responseType, ...fetchOptions } = options; - const response = await fetchImpl(url, { - ...fetchOptions, - headers, - }); + // Reads fail fast; writes and downloads may legitimately run as long as the + // Lambda (120s) before the visitor is told it failed. + const deadlineMs = + String(fetchOptions.method || "GET").toUpperCase() === "GET" && + responseType !== "blob" + ? timeoutMs + : Math.max(timeoutMs, 120000); + const checkPath = new URL(String(url), apiBase || "http://localhost").pathname; + const what = `load ${checkPath}`; + // Every shell request has a deadline and a visible failure: a hung API call + // must reject with a message the surface renders, never spin forever. + let response; + try { + response = await fetchImpl(url, { + ...fetchOptions, + headers, + signal: fetchOptions.signal || AbortSignal.timeout(deadlineMs), + }); + } catch (cause) { + if (fetchOptions.signal?.aborted && cause?.name === "AbortError") throw cause; + const status = + cause?.name === "TimeoutError" || cause?.name === "AbortError" + ? "timeout" + : "network"; + reportClientFailure( + { surface: "app-shell", check: checkPath, status: null, message: status }, + { fetchImpl }, + ); + const error = new Error(failureMessage(what, status)); + error.status = status; + throw error; + } if (response.ok && responseType === "blob") return response.blob(); const text = await response.text(); @@ -82,6 +121,18 @@ export function createApiClient({ apiBase, fetchImpl, storage }) { throw error; } + if (response.status >= 500) { + reportClientFailure( + { + surface: "app-shell", + check: checkPath, + status: response.status, + message: String(payload?.error || ""), + }, + { fetchImpl }, + ); + } + if (!response.ok) { const fallback = `HTTP ${response.status}${ response.statusText ? ` ${response.statusText}` : "" diff --git a/frontend/test/api-client.test.mjs b/frontend/test/api-client.test.mjs index b5394bf7e..5963ae4b1 100644 --- a/frontend/test/api-client.test.mjs +++ b/frontend/test/api-client.test.mjs @@ -67,3 +67,62 @@ describe("createApiClient request coalescing", () => { assert.equal(calls.length, 2); }); }); + +describe("createApiClient fail-loud", () => { + function client(fetchImpl, timeoutMs = 20) { + return createApiClient({ + apiBase: "https://ops.example.invalid", + storage: { getItem: () => null }, + fetchImpl, + timeoutMs, + }); + } + + test("a hung request times out with a visible message and is reported", async () => { + const reports = []; + const { request } = client((url, options) => { + if (String(url).endsWith("/api/client-errors")) { + reports.push(JSON.parse(options.body)); + return Promise.resolve({}); + } + return new Promise((_, reject) => + options.signal.addEventListener("abort", () => reject(options.signal.reason)), + ); + }); + await assert.rejects(request(new URL("https://ops.example.invalid/api/me?x=1")), (error) => { + assert.equal(error.status, "timeout"); + assert.match(error.message, /Could not load \/api\/me: it took too long/); + return true; + }); + assert.equal(reports[0].surface, "app-shell"); + assert.equal(reports[0].check, "/api/me"); + }); + + test("a 5xx is reported and still rejects with the server error", async () => { + const reports = []; + const { request } = client((url, options) => { + if (String(url).endsWith("/api/client-errors")) { + reports.push(JSON.parse(options.body)); + return Promise.resolve({}); + } + return Promise.resolve({ + ok: false, + status: 503, + statusText: "x", + text: async () => JSON.stringify({ error: "down" }), + }); + }); + await assert.rejects(request(new URL("https://ops.example.invalid/api/users")), /down/); + assert.equal(reports[0].status, 503); + }); + + test("a 4xx is not reported", async () => { + const reports = []; + const { request } = client((url, options) => { + if (String(url).endsWith("/api/client-errors")) reports.push(options.body); + return Promise.resolve({ ok: false, status: 403, statusText: "", text: async () => "{}" }); + }); + await assert.rejects(request(new URL("https://ops.example.invalid/api/users"))); + assert.equal(reports.length, 0); + }); +}); From 6515f7486f92573ba10766d625eeb853b5077844 Mon Sep 17 00:00:00 2001 From: Alexey Grigorev Date: Mon, 9 Nov 2026 11:42:16 +0100 Subject: [PATCH 2/5] Alarm on browser-reported client_check_failed via the shared operator topic --- infra/template.full.yaml | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/infra/template.full.yaml b/infra/template.full.yaml index 7d79dad35..4b190ad7c 100644 --- a/infra/template.full.yaml +++ b/infra/template.full.yaml @@ -1645,6 +1645,36 @@ Resources: TreatMissingData: notBreaching AlarmActions: [!Ref ConversationalRolloutAlarmTopic] + # Browser-reported failures (anonymous pages and the app shell post to + # /api/client-errors, which logs one ERROR client_check_failed line). Paged + # on the shared operator topic: this stack's own alarm topic has no + # subscribers, so alarms on it reach nobody. + ClientCheckFailedMetricFilter: + Type: AWS::Logs::MetricFilter + Properties: + LogGroupName: !Ref ConversationalBackendLogGroup + FilterPattern: '{ $.event = "client_check_failed" }' + MetricTransformations: + - MetricNamespace: DataOps/Client + MetricName: ClientCheckFailed + MetricValue: "1" + DefaultValue: 0 + + ClientCheckFailedAlarm: + Type: AWS::CloudWatch::Alarm + Properties: + AlarmName: !Sub ${AWS::StackName}-client-check-failed + AlarmDescription: A browser reported a failed or timed-out page/API check (client_check_failed). See the backend log group. + Namespace: DataOps/Client + MetricName: ClientCheckFailed + Statistic: Sum + Period: 300 + EvaluationPeriods: 1 + Threshold: 1 + ComparisonOperator: GreaterThanOrEqualToThreshold + TreatMissingData: notBreaching + AlarmActions: [!Ref DynamoPitrAlertTopicArn] + Outputs: ConversationalRolloutAlarmTopicArn: Value: !Ref ConversationalRolloutAlarmTopic From c89504731dde6bcf0e0b09d56198fc5f6709bc5f Mon Sep 17 00:00:00 2001 From: Alexey Grigorev Date: Mon, 9 Nov 2026 11:58:14 +0100 Subject: [PATCH 3/5] Keep the event loop alive in the timeout test so CI does not cancel it --- frontend/test/api-client.test.mjs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/frontend/test/api-client.test.mjs b/frontend/test/api-client.test.mjs index 5963ae4b1..81a37651d 100644 --- a/frontend/test/api-client.test.mjs +++ b/frontend/test/api-client.test.mjs @@ -1,5 +1,5 @@ import assert from "node:assert/strict"; -import { describe, test } from "node:test"; +import { after, describe, test } from "node:test"; import { createApiClient } from "../src/shell/api.js"; @@ -89,6 +89,9 @@ describe("createApiClient fail-loud", () => { options.signal.addEventListener("abort", () => reject(options.signal.reason)), ); }); + // AbortSignal.timeout timers are unref'd in Node; hold the loop open until it fires. + const keepAlive = setTimeout(() => {}, 5000); + after(() => clearTimeout(keepAlive)); await assert.rejects(request(new URL("https://ops.example.invalid/api/me?x=1")), (error) => { assert.equal(error.status, "timeout"); assert.match(error.message, /Could not load \/api\/me: it took too long/); From 785c248c7b15096297683fc7d99e5f84e736920c Mon Sep 17 00:00:00 2001 From: Alexey Grigorev Date: Mon, 9 Nov 2026 12:17:36 +0100 Subject: [PATCH 4/5] Answer client error reports with an empty 204 so browsers do not abort them --- backend/src/routes/clientErrors.ts | 5 +++-- backend/tests/client-errors.test.ts | 6 +++++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/backend/src/routes/clientErrors.ts b/backend/src/routes/clientErrors.ts index 49f250971..be495c924 100644 --- a/backend/src/routes/clientErrors.ts +++ b/backend/src/routes/clientErrors.ts @@ -1,5 +1,5 @@ import { headerValue } from '../httpHeaders'; -import { noStoreJson } from '../http'; +import { NO_STORE_JSON_HEADERS, noStoreJson } from '../http'; import type { LambdaEvent, LambdaResponse } from '../types'; /** @@ -62,5 +62,6 @@ export function handleClientErrorRoute(event: LambdaEvent): LambdaResponse { page: field(body.page).split(/[?#]/)[0], userAgent: headerValue(event.headers, 'user-agent').slice(0, MAX_FIELD_LENGTH), })); - return noStoreJson(204, {}); + // A 204 must have no body; a `{}` payload makes browsers abort the response. + return { statusCode: 204, headers: NO_STORE_JSON_HEADERS, body: '' }; } diff --git a/backend/tests/client-errors.test.ts b/backend/tests/client-errors.test.ts index 926dd0632..34a31870c 100644 --- a/backend/tests/client-errors.test.ts +++ b/backend/tests/client-errors.test.ts @@ -24,10 +24,14 @@ describe('client error reports', () => { it('logs one structured ERROR line without query strings', () => { let status = 0; + let body: string | undefined; const lines = captureErrors(() => { - status = handleClientErrorRoute(event({ surface: 'sponsor-doc', check: 'GET /api/sponsor-docs', status: 503, message: 'x', page: '/sponsor-docs/sdoc-1?code=SECRET' }, '198.51.100.1')).statusCode; + const response = handleClientErrorRoute(event({ surface: 'sponsor-doc', check: 'GET /api/sponsor-docs', status: 503, message: 'x', page: '/sponsor-docs/sdoc-1?code=SECRET' }, '198.51.100.1')); + status = response.statusCode; + body = response.body; }); assert.equal(status, 204); + assert.equal(body, '', 'a 204 must carry no body or browsers abort the response'); assert.equal(lines.length, 1); const entry = JSON.parse(lines[0]); assert.equal(entry.event, 'client_check_failed'); From 14a744ffbe0ecbba216d748df80588faf7cb4495 Mon Sep 17 00:00:00 2001 From: Alexey Grigorev Date: Mon, 9 Nov 2026 12:50:17 +0100 Subject: [PATCH 5/5] Dedupe and cap app-shell failure reports, answer them with 200, widen shell read timeout, and declare report telemetry in outage e2e specs --- .../e2e/canonical-capability-behavior.spec.js | 4 +- backend/e2e/crm-startup.spec.js | 2 +- backend/e2e/docs-outage.spec.js | 9 +++- backend/src/routes/clientErrors.ts | 7 ++-- backend/tests/client-errors.test.ts | 4 +- frontend/src/shell/api.js | 42 ++++++++++++------- frontend/test/api-client.test.mjs | 19 +++++++++ 7 files changed, 63 insertions(+), 24 deletions(-) diff --git a/backend/e2e/canonical-capability-behavior.spec.js b/backend/e2e/canonical-capability-behavior.spec.js index db406ff82..2922a136e 100644 --- a/backend/e2e/canonical-capability-behavior.spec.js +++ b/backend/e2e/canonical-capability-behavior.spec.js @@ -179,7 +179,9 @@ function observeBrowserErrors(page) { message: message.text(), }); }); - page.on('requestfailed', (request) => entries.push({ + // The shell reports deliberate failures to /api/client-errors best-effort; a + // report cut off by navigation is telemetry, not a page failure. + page.on('requestfailed', (request) => request.url().endsWith('/api/client-errors') || entries.push({ kind: 'requestfailed', url: request.url(), failure: request.failure()?.errorText || 'unknown', diff --git a/backend/e2e/crm-startup.spec.js b/backend/e2e/crm-startup.spec.js index 3bacb61ae..5b5ae1ec6 100644 --- a/backend/e2e/crm-startup.spec.js +++ b/backend/e2e/crm-startup.spec.js @@ -103,7 +103,7 @@ function watchRequests(page) { }); }); page.on('requestfailed', (request) => { - if (!started.has(request)) return; + if (!started.has(request) || request.url().endsWith('/api/client-errors')) return; failed.push({ method: request.method(), path: shortUrl(request.url()), diff --git a/backend/e2e/docs-outage.spec.js b/backend/e2e/docs-outage.spec.js index 35cd0f42d..ebbd3e8cc 100644 --- a/backend/e2e/docs-outage.spec.js +++ b/backend/e2e/docs-outage.spec.js @@ -162,6 +162,12 @@ function observeErrors(page) { * @param {Array} entries collected observer entries * @param {(entry: object) => boolean} [alsoAttributable] extra declared responses */ +// The shell reports each deliberate failure to /api/client-errors on a +// best-effort basis. Those reports are a declared consequence of the outage: a +// report may be cut off by navigation or throttled by the per-IP budget that +// every test in a file shares. +const isClientErrorReport = (entry) => new URL(entry.url).pathname === '/api/client-errors'; + function expectOnlyDeliberateDocsOutage(entries, alsoAttributable = () => false) { const docs503s = entries.filter( (entry) => entry.kind === 'response' && entry.status === 503 && isDocsUrl(entry.url), @@ -174,12 +180,13 @@ function expectOnlyDeliberateDocsOutage(entries, alsoAttributable = () => false) const attributable = new Set([ ...outageResponses, ...entries.filter((entry) => entry.kind === 'console' && outageUrls.has(entry.url)), + ...entries.filter(isClientErrorReport), ]); const unexpected = entries.filter((entry) => !attributable.has(entry)).map(describeEntry); expect(unexpected).toEqual([]); expect(docs503s.length).toBeGreaterThan(0); expect(entries.filter((entry) => entry.kind === 'pageerror').map(describeEntry)).toEqual([]); - expect(entries.filter((entry) => entry.kind === 'requestfailed').map(describeEntry)).toEqual([]); + expect(entries.filter((entry) => entry.kind === 'requestfailed' && !isClientErrorReport(entry)).map(describeEntry)).toEqual([]); } async function portalContext(browser, server, options = {}) { diff --git a/backend/src/routes/clientErrors.ts b/backend/src/routes/clientErrors.ts index be495c924..60355d582 100644 --- a/backend/src/routes/clientErrors.ts +++ b/backend/src/routes/clientErrors.ts @@ -1,5 +1,5 @@ import { headerValue } from '../httpHeaders'; -import { NO_STORE_JSON_HEADERS, noStoreJson } from '../http'; +import { noStoreJson } from '../http'; import type { LambdaEvent, LambdaResponse } from '../types'; /** @@ -62,6 +62,7 @@ export function handleClientErrorRoute(event: LambdaEvent): LambdaResponse { page: field(body.page).split(/[?#]/)[0], userAgent: headerValue(event.headers, 'user-agent').slice(0, MAX_FIELD_LENGTH), })); - // A 204 must have no body; a `{}` payload makes browsers abort the response. - return { statusCode: 204, headers: NO_STORE_JSON_HEADERS, body: '' }; + // 200, not 204: Chromium reports a POST answered with 204 as net::ERR_ABORTED, + // which surfaces as a failed request in the browser and in e2e observers. + return noStoreJson(200, {}); } diff --git a/backend/tests/client-errors.test.ts b/backend/tests/client-errors.test.ts index 34a31870c..000388a16 100644 --- a/backend/tests/client-errors.test.ts +++ b/backend/tests/client-errors.test.ts @@ -30,8 +30,8 @@ describe('client error reports', () => { status = response.statusCode; body = response.body; }); - assert.equal(status, 204); - assert.equal(body, '', 'a 204 must carry no body or browsers abort the response'); + assert.equal(status, 200, 'not 204: Chromium reports a 204 POST as ERR_ABORTED'); + assert.equal(body, '{}'); assert.equal(lines.length, 1); const entry = JSON.parse(lines[0]); assert.equal(entry.event, 'client_check_failed'); diff --git a/frontend/src/shell/api.js b/frontend/src/shell/api.js index 2217076da..9bf452fa6 100644 --- a/frontend/src/shell/api.js +++ b/frontend/src/shell/api.js @@ -1,9 +1,14 @@ import { - REQUEST_TIMEOUT_MS, failureMessage, reportClientFailure, } from "../core/guarded-fetch.js"; +// Shell reads can sit behind a cold Lambda start and a large listing, so they +// get longer than the anonymous pages' 20s; writes get the full Lambda budget. +const READ_TIMEOUT_MS = 30000; +const WRITE_TIMEOUT_MS = 120000; +const MAX_REPORTS_PER_PAGE = 5; + export function resolveApiBase({ documentRef, windowRef }) { const meta = documentRef .querySelector('meta[name="api-base"]') @@ -19,7 +24,7 @@ export function createApiClient({ apiBase, fetchImpl, storage, - timeoutMs = REQUEST_TIMEOUT_MS, + timeoutMs = READ_TIMEOUT_MS, }) { function apiUrl(path) { return new URL(path, apiBase); @@ -30,6 +35,17 @@ export function createApiClient({ // and blob downloads never coalesce. const inflightGets = new Map(); + // One failing page load fires many failing requests. Report each distinct + // (check, status) once, capped per page, so the server's per-IP budget is + // not spent on duplicates and no 429s show up as browser errors. + const reported = new Set(); + function reportOnce(report) { + const key = `${report.check} ${report.status ?? report.message}`; + if (reported.has(key) || reported.size >= MAX_REPORTS_PER_PAGE) return; + reported.add(key); + reportClientFailure(report, { fetchImpl }); + } + async function request(url, options = {}) { const method = String(options.method || "GET").toUpperCase(); const coalesceKey = @@ -70,7 +86,7 @@ export function createApiClient({ String(fetchOptions.method || "GET").toUpperCase() === "GET" && responseType !== "blob" ? timeoutMs - : Math.max(timeoutMs, 120000); + : Math.max(timeoutMs, WRITE_TIMEOUT_MS); const checkPath = new URL(String(url), apiBase || "http://localhost").pathname; const what = `load ${checkPath}`; // Every shell request has a deadline and a visible failure: a hung API call @@ -88,10 +104,7 @@ export function createApiClient({ cause?.name === "TimeoutError" || cause?.name === "AbortError" ? "timeout" : "network"; - reportClientFailure( - { surface: "app-shell", check: checkPath, status: null, message: status }, - { fetchImpl }, - ); + reportOnce({ surface: "app-shell", check: checkPath, status: null, message: status }); const error = new Error(failureMessage(what, status)); error.status = status; throw error; @@ -122,15 +135,12 @@ export function createApiClient({ } if (response.status >= 500) { - reportClientFailure( - { - surface: "app-shell", - check: checkPath, - status: response.status, - message: String(payload?.error || ""), - }, - { fetchImpl }, - ); + reportOnce({ + surface: "app-shell", + check: checkPath, + status: response.status, + message: String(payload?.error || ""), + }); } if (!response.ok) { diff --git a/frontend/test/api-client.test.mjs b/frontend/test/api-client.test.mjs index 81a37651d..8a7b8d408 100644 --- a/frontend/test/api-client.test.mjs +++ b/frontend/test/api-client.test.mjs @@ -128,4 +128,23 @@ describe("createApiClient fail-loud", () => { await assert.rejects(request(new URL("https://ops.example.invalid/api/users"))); assert.equal(reports.length, 0); }); + + test("repeated failures are reported once per check and capped per page", async () => { + const reports = []; + const { request } = client((url, options) => { + if (String(url).endsWith("/api/client-errors")) { + reports.push(JSON.parse(options.body)); + return Promise.resolve({}); + } + return Promise.resolve({ ok: false, status: 503, statusText: "", text: async () => "{}" }); + }); + for (let i = 0; i < 4; i += 1) { + await assert.rejects(request(new URL("https://ops.example.invalid/api/same"), { method: "POST" })); + } + for (let i = 0; i < 10; i += 1) { + await assert.rejects(request(new URL(`https://ops.example.invalid/api/many-${i}`))); + } + assert.equal(reports.filter((report) => report.check === "/api/same").length, 1); + assert.equal(reports.length, 5); + }); });