fix: [#838] webfetch 404 cache — longer TTL + URL normalization

packages/opencode/src/tool/webfetch.ts

@@ -15,16 +15,143 @@ const BROWSER_UA =
 // Status codes that warrant a retry with a different User-Agent
 const RETRYABLE_STATUSES = new Set([403, 406])
 
-// altimate_change start — session-level URL failure cache (#471)
-// Prevents repeated fetches to URLs that already returned 404/410 in this session.
+// altimate_change start — process-level URL failure cache (#471)
+// Prevents repeated fetches to URLs that already returned 404/410 in this
+// process. Cache is module-scope so in daemon mode (`altimate serve`) and
+// long-running MCP servers it persists across logical sessions — fine for
+// 404/410 (resource-permanent) but worth flagging for 451 (geo/legal block,
+// observer-conditional). See cacheUrlFailure comment for TTL rationale.
+//
+// 2026-05-22 follow-up to telemetry-2026-05-21 (486 residual webfetch 404s,
+// down from March's 2,222 but still meaningful):
+//   - Extended TTL from 5 min to 30 min. URLs that 404 rarely self-heal
+//     within a session, and many sessions run longer than 5 min so the
+//     short window was letting agents re-hit the same dead URL multiple
+//     times in one session.
+//   - Cache key is the URL with tracking-only params (utm_*, fbclid,
+//     gclid, mc_*, _ga, _gl, igshid, mibextid, __cf_chl_*, plus HubSpot/
+//     Marketo/Adobe/Matomo variants) stripped, so LLM-generated tracking
+//     variations of a known-bad URL all hit cache. Functional query params
+//     (page, id, q, ref, etc.) are preserved — see exclusions block below
+//     for the rationale on `ref` specifically.
 const failedUrls = new Map<string, { status: number; timestamp: number }>()
-const FAILURE_CACHE_TTL = 5 * 60 * 1000 // 5 minutes
+const FAILURE_CACHE_TTL = 30 * 60 * 1000 // 30 minutes — 404/410 (resource-permanent)
+const FAILURE_CACHE_TTL_451 = 5 * 60 * 1000 // 5 minutes — 451 is observer-conditional (VPN swap, travel)
+
+/**
+ * Param names that are universally tracking-only — stripping them never
+ * changes the document the URL points to. Conservative list; favoured over
+ * a regex sweep because false positives (e.g. stripping `?page=2`) would
+ * cause real fetches to incorrectly hit the cache.
+ */
+// Notable exclusions (do NOT add these to TRACKING_PARAMS):
+//   - `ref` / `ref_src` / `ref_url`: functional on GitHub raw URLs and
+//     git-hosting APIs (`?ref=main` vs `?ref=v2.0` selects a different
+//     branch/tag). Stripping `ref` would suppress legitimate fetches of
+//     different refs against the same path — a coding agent fetches GitHub
+//     heavily, so this is a real false-positive risk.
+//   - `referrer` (full word) is tracking-only and IS in the list; the
+//     abbreviated `ref` is not.
+//
+// The utm_* family is enumerated explicitly here AND covered by the
+// `isTrackingParamPrefix` check below. The duplication is intentional —
+// the explicit list documents the named params we know about, the prefix
+// check catches any future utm_* variant we haven't enumerated.
+// Cloudflare challenge tokens (`__cf_chl_*`) are handled by the prefix
+// check exclusively; no explicit entries needed.
+const TRACKING_PARAMS = new Set([
+  "utm_source",
+  "utm_medium",
+  "utm_campaign",
+  "utm_term",
+  "utm_content",
+  "utm_id",
+  "referrer",
+  "fbclid",
+  "gclid",
+  "dclid",
+  "msclkid",
+  "yclid",
+  "twclid",
+  "li_fat_id",
+  "mc_cid",
+  "mc_eid",
+  "_ga",
+  "_gl",
+  "igshid",
+  "mibextid",
+  "vero_conv",
+  "vero_id",
+  // HubSpot
+  "_hsenc",
+  "_hsmi",
+  "hsCtaTracking",
+  // Marketo
+  "mkt_tok",
+  // Adobe Analytics / SiteCatalyst
+  "s_cid",
+  "s_kwcid",
+  // Piwik / Matomo
+  "pk_campaign",
+  "pk_kwd",
+  "pk_source",
+  "pk_medium",
+  "pk_content",
+  "piwik_campaign",
+  "piwik_keyword",
+])
+
+function isTrackingParamPrefix(name: string): boolean {
+  return name.startsWith("__cf_chl_") || name.startsWith("utm_")
+}
+
+/**
+ * Normalize a URL for cache lookup. Strips tracking-only query params, sorts
+ * the remaining params for stable ordering, and lowercases the host. Returns
+ * the input unchanged if URL parsing fails.
+ */
+export function normalizeUrlForCache(url: string): string {
+  try {
+    const parsed = new URL(url)
+    parsed.hostname = parsed.hostname.toLowerCase()
+    // Strip userinfo (basic-auth `user:pass@`) — it doesn't change the
+    // resource the URL points to, and leaving it in lets the same logical
+    // URL with/without credentials occupy two cache slots. Also a minor
+    // hygiene win for cache keys that get logged in telemetry.
+    parsed.username = ""
+    parsed.password = ""
+    const kept: [string, string][] = []
+    for (const [k, v] of parsed.searchParams) {
+      if (TRACKING_PARAMS.has(k) || isTrackingParamPrefix(k)) continue
+      kept.push([k, v])
+    }
+    // Sort by (key, value) so duplicate-key params produce a stable order:
+    // `?a=1&a=2` and `?a=2&a=1` normalize to the same string. Sorting by key
+    // alone leaked the original insertion order between same-key entries.
+    kept.sort(([ak, av], [bk, bv]) => {
+      if (ak !== bk) return ak < bk ? -1 : 1
+      return av < bv ? -1 : av > bv ? 1 : 0
+    })
+    parsed.search = ""
+    for (const [k, v] of kept) parsed.searchParams.append(k, v)
+    // Also strip fragment — fragments don't change the fetched resource.
+    parsed.hash = ""
+    return parsed.toString()
+  } catch {
+    return url
+  }
+}
+
+function ttlForStatus(status: number): number {
+  return status === 451 ? FAILURE_CACHE_TTL_451 : FAILURE_CACHE_TTL
+}
 
 function isUrlCachedFailure(url: string): { status: number } | null {
-  const entry = failedUrls.get(url)
+  const key = normalizeUrlForCache(url)
+  const entry = failedUrls.get(key)
   if (!entry) return null
-  if (Date.now() - entry.timestamp > FAILURE_CACHE_TTL) {
-    failedUrls.delete(url)
+  if (Date.now() - entry.timestamp > ttlForStatus(entry.status)) {
+    failedUrls.delete(key)
     return null
   }
   return { status: entry.status }
@@ -33,16 +160,72 @@ function isUrlCachedFailure(url: string): { status: number } | null {
 const MAX_CACHED_URLS = 500
 
 function cacheUrlFailure(url: string, status: number): void {
+  // TTL by status:
+  //   404 (resource not found) + 410 (gone) — 30 min. Both are
+  //   resource-permanent; 410 is spec-permanent, 404 is overwhelmingly
+  //   permanent in practice (typos, removed docs).
+  //   451 (unavailable for legal reasons) — 5 min. Observer-conditional:
+  //   if the agent swaps networks (VPN, travel, mobile hotspot) the URL
+  //   may become reachable from the new vantage point, so a long TTL is
+  //   wrong. 5 min keeps the immediate-retry suppression but lets a
+  //   network change unblock the URL.
   if (status === 404 || status === 410 || status === 451) {
+    const key = normalizeUrlForCache(url)
+    // Re-touch semantics: if the key is already in the cache, delete first
+    // so the .set() reinserts at the tail of the FIFO. Otherwise a
+    // frequently-failing URL would stay at the head and be the first
+    // evicted under pressure — wrong order for an LRU-flavoured cache.
+    if (failedUrls.has(key)) failedUrls.delete(key)
     if (failedUrls.size >= MAX_CACHED_URLS) {
       // Evict oldest entry (Map preserves insertion order)
       const oldest = failedUrls.keys().next().value
       if (oldest) failedUrls.delete(oldest)
     }
-    failedUrls.set(url, { status, timestamp: Date.now() })
+    failedUrls.set(key, { status, timestamp: Date.now() })
   }
 }
 
+/**
+ * Reset the failure cache.
+ * @internal — only used by tests; do not call from production code.
+ */
+export function _resetFailureCache(): void {
+  failedUrls.clear()
+}
+
+/**
+ * Read the current failure cache size.
+ * @internal — only used by tests; do not call from production code.
+ */
+export function _failureCacheSize(): number {
+  return failedUrls.size
+}
+
+/**
+ * Drive the cache state machine from tests — same code path as production.
+ * @internal — only used by tests; do not call from production code.
+ */
+export function _cacheUrlFailureForTest(url: string, status: number): void {
+  cacheUrlFailure(url, status)
+}
+
+/**
+ * Read cache key insertion order — the order entries would be evicted under
+ * FIFO/LRU pressure. Returns normalized cache keys, not raw URLs.
+ * @internal — only used by tests; do not call from production code.
+ */
+export function _failureCacheKeysInOrder(): string[] {
+  return [...failedUrls.keys()]
+}
+
+/**
+ * Probe the cache for a URL — same code path as production lookups.
+ * @internal — only used by tests; do not call from production code.
+ */
+export function _isUrlCachedFailureForTest(url: string): { status: number } | null {
+  return isUrlCachedFailure(url)
+}
+
 /** Strip query string from URL to avoid leaking auth tokens in error messages. */
 function sanitizeUrl(url: string): string {
   try {