diff --git a/backend/e2e/canonical-capability-behavior.spec.js b/backend/e2e/canonical-capability-behavior.spec.js index db406ff8..2922a136 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 3bacb61a..5b5ae1ec 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 35cd0f42..ebbd3e8c 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 49f25097..60355d58 100644 --- a/backend/src/routes/clientErrors.ts +++ b/backend/src/routes/clientErrors.ts @@ -62,5 +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), })); - return noStoreJson(204, {}); + // 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 926dd063..000388a1 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(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 827c2f1a..9bf452fa 100644 --- a/frontend/src/shell/api.js +++ b/frontend/src/shell/api.js @@ -1,3 +1,14 @@ +import { + 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"]') @@ -9,7 +20,12 @@ export function resolveApiBase({ documentRef, windowRef }) { return ""; } -export function createApiClient({ apiBase, fetchImpl, storage }) { +export function createApiClient({ + apiBase, + fetchImpl, + storage, + timeoutMs = READ_TIMEOUT_MS, +}) { function apiUrl(path) { return new URL(path, apiBase); } @@ -19,6 +35,17 @@ export function createApiClient({ apiBase, fetchImpl, storage }) { // 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 = @@ -53,10 +80,35 @@ 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, 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 + // 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"; + reportOnce({ surface: "app-shell", check: checkPath, status: null, message: status }); + 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 +134,15 @@ export function createApiClient({ apiBase, fetchImpl, storage }) { throw error; } + if (response.status >= 500) { + reportOnce({ + surface: "app-shell", + check: checkPath, + status: response.status, + message: String(payload?.error || ""), + }); + } + 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 b5394bf7..8a7b8d40 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"; @@ -67,3 +67,84 @@ 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)), + ); + }); + // 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/); + 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); + }); + + 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); + }); +}); diff --git a/infra/template.full.yaml b/infra/template.full.yaml index 7d79dad3..4b190ad7 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