fix: avoid double-counting fetch failures

2026-05-17 14:39:46 +02:00
parent 8605d0baab
commit 08d24594c1
2 changed files with 87 additions and 11 deletions
--- a/apis/utils/fetch.mjs
+++ b/apis/utils/fetch.mjs
@@ -42,6 +42,7 @@ export async function safeFetch(url, opts = {}) {
  let lastError;
  for (let i = 0; i <= retries; i++) {
    const started = Date.now();
+    let metricRecorded = false;
    try {
      const controller = new AbortController();
      const timer = setTimeout(() => controller.abort(), timeout);
@@ -51,22 +52,29 @@ export async function safeFetch(url, opts = {}) {
      });
      clearTimeout(timer);
      const status = res.status;
-      if (!res.ok) {
-        const body = await res.text().catch(() => '');
-        recordFetchMetric({ url, source, ok: false, status, bytes: body.length, durationMs: Date.now() - started, error: `HTTP ${res.status}` });
-        throw new Error(`HTTP ${res.status}: ${body.slice(0, 200)}`);
-      }
      const text = await res.text();
-      recordFetchMetric({ url, source, ok: true, status, bytes: text.length, durationMs: Date.now() - started });
+      if (!res.ok) {
+        const error = `HTTP ${res.status}`;
+        recordFetchMetric({ url, source, ok: false, status, bytes: text.length, durationMs: Date.now() - started, error });
+        metricRecorded = true;
+        throw new Error(`${error}: ${text.slice(0, 200)}`);
+      }
      const trimmed = text.trim();
      const contentType = res.headers.get('content-type') || '';
      if (contentType.includes('text/html') || trimmed.startsWith('<!DOCTYPE html') || trimmed.startsWith('<html')) {
-        throw new Error(`Expected JSON but received HTML from ${new URL(url).host}`);
+        const error = `Expected JSON but received HTML from ${new URL(url).host}`;
+        recordFetchMetric({ url, source, ok: false, status, bytes: text.length, durationMs: Date.now() - started, error });
+        metricRecorded = true;
+        throw new Error(error);
      }
+      recordFetchMetric({ url, source, ok: true, status, bytes: text.length, durationMs: Date.now() - started });
+      metricRecorded = true;
      try { return JSON.parse(text); } catch { return { rawText: text.slice(0, 500) }; }
    } catch (e) {
      lastError = e;
-      recordFetchMetric({ url, source, ok: false, status: null, bytes: 0, durationMs: Date.now() - started, error: e.message });
+      if (!metricRecorded) {
+        recordFetchMetric({ url, source, ok: false, status: null, bytes: 0, durationMs: Date.now() - started, error: e.message });
+      }
      // GDELT needs 5s between requests, others are fine with shorter delays
      if (i < retries) await new Promise(r => setTimeout(r, 2000 * (i + 1)));
    }
@@ -79,6 +87,7 @@ export async function safeFetchText(url, opts = {}) {
  let lastError;
  for (let i = 0; i <= retries; i++) {
    const started = Date.now();
+    let metricRecorded = false;
    try {
      const controller = new AbortController();
      const timer = setTimeout(() => controller.abort(), timeout);
@@ -89,11 +98,14 @@ export async function safeFetchText(url, opts = {}) {
      clearTimeout(timer);
      const text = await res.text();
      recordFetchMetric({ url, source, ok: res.ok, status: res.status, bytes: text.length, durationMs: Date.now() - started, error: res.ok ? null : `HTTP ${res.status}` });
+      metricRecorded = true;
      if (!res.ok) throw new Error(`HTTP ${res.status}: ${text.slice(0, 200)}`);
      return { text, status: res.status, bytes: text.length };
    } catch (e) {
      lastError = e;
-      recordFetchMetric({ url, source, ok: false, status: null, bytes: 0, durationMs: Date.now() - started, error: e.message });
+      if (!metricRecorded) {
+        recordFetchMetric({ url, source, ok: false, status: null, bytes: 0, durationMs: Date.now() - started, error: e.message });
+      }
      if (i < retries) await new Promise(r => setTimeout(r, 2000 * (i + 1)));
    }
  }
--- a/test/fetch-utils.test.mjs
+++ b/test/fetch-utils.test.mjs
@@ -4,6 +4,7 @@ import { safeFetch, safeFetchText, getFetchMetrics } from '../apis/utils/fetch.m

 test('safeFetch reports HTML as degraded JSON response', async () => {
  const originalFetch = globalThis.fetch;
+  const source = 'unit-html-once';
  globalThis.fetch = async () => ({
    ok: true,
    status: 200,
@@ -11,9 +12,72 @@ test('safeFetch reports HTML as degraded JSON response', async () => {
    text: async () => '<html>not json</html>',
  });
  try {
-    const data = await safeFetch('https://example.test/json', { retries: 0, source: 'unit' });
+    const data = await safeFetch('https://example.test/json', { retries: 0, source });
    assert.match(data.error, /Expected JSON/);
-    assert.ok(getFetchMetrics().bySource.unit.requests >= 1);
+    const bucket = getFetchMetrics().bySource[source];
+    assert.equal(bucket.requests, 1);
+    assert.equal(bucket.ok, 0);
+    assert.equal(bucket.failed, 1);
+    assert.equal(bucket.lastStatus, 200);
+  } finally {
+    globalThis.fetch = originalFetch;
+  }
+});
+
+test('safeFetch records HTTP failure once with status and bytes', async () => {
+  const originalFetch = globalThis.fetch;
+  const source = 'unit-http-failure-once';
+  globalThis.fetch = async () => ({
+    ok: false,
+    status: 503,
+    headers: { get: () => 'application/json' },
+    text: async () => 'service unavailable',
+  });
+  try {
+    const data = await safeFetch('https://example.test/fail', { retries: 0, source });
+    assert.match(data.error, /HTTP 503/);
+    const bucket = getFetchMetrics().bySource[source];
+    assert.equal(bucket.requests, 1);
+    assert.equal(bucket.ok, 0);
+    assert.equal(bucket.failed, 1);
+    assert.equal(bucket.lastStatus, 503);
+    assert.equal(bucket.bytes, 'service unavailable'.length);
+    assert.match(bucket.lastError, /HTTP 503/);
+  } finally {
+    globalThis.fetch = originalFetch;
+  }
+});
+
+test('safeFetch retry metrics count one record per attempt', async () => {
+  const originalFetch = globalThis.fetch;
+  const source = 'unit-retry-attempts';
+  let calls = 0;
+  globalThis.fetch = async () => {
+    calls += 1;
+    if (calls === 1) {
+      return {
+        ok: false,
+        status: 502,
+        headers: { get: () => 'application/json' },
+        text: async () => 'bad gateway',
+      };
+    }
+    return {
+      ok: true,
+      status: 200,
+      headers: { get: () => 'application/json' },
+      text: async () => '{"ok":true}',
+    };
+  };
+  try {
+    const data = await safeFetch('https://example.test/retry', { retries: 1, source });
+    assert.equal(data.ok, true);
+    assert.equal(calls, 2);
+    const bucket = getFetchMetrics().bySource[source];
+    assert.equal(bucket.requests, 2);
+    assert.equal(bucket.ok, 1);
+    assert.equal(bucket.failed, 1);
+    assert.equal(bucket.lastStatus, 200);
  } finally {
    globalThis.fetch = originalFetch;
  }