fix: enhance caching logic to differentiate between transient failures and permanent not found results

Author: jakejarvis

lib/cache.test.ts

@@ -97,11 +97,11 @@ describe("cached assets", () => {
     expect(stored?.url).toBe("https://cdn/new.webp");
   });
 
-  it("retries when null is cached (treats null as miss)", async () => {
+  it("retries transient failures (null without notFound flag)", async () => {
     const indexKey = ns("test", "asset4");
     const lockKey = ns("lock", "test", "asset4");
 
-    // Pre-seed cache with null result (simulating previous failure)
+    // Pre-seed cache with null result WITHOUT notFound (simulating transient failure)
     const { redis } = await import("@/lib/redis");
     await redis.set(indexKey, {
       url: null,
@@ -124,25 +124,55 @@ describe("cached assets", () => {
     expect(produceCalled).toBe(true);
   });
 
-  it("writes null to cache to prevent concurrent retries", async () => {
+  it("does NOT retry permanent not found (null with notFound=true)", async () => {
     const indexKey = ns("test", "asset5");
     const lockKey = ns("lock", "test", "asset5");
 
+    // Pre-seed cache with permanent not found
+    const { redis } = await import("@/lib/redis");
+    await redis.set(indexKey, {
+      url: null,
+      notFound: true,
+      expiresAtMs: Date.now() + 1000,
+    });
+
+    let produceCalled = false;
+    const result = await getOrCreateCachedAsset<{ source: string }>({
+      indexKey,
+      lockKey,
+      ttlSeconds: 60,
+      produceAndUpload: async () => {
+        produceCalled = true;
+        return { url: "https://cdn/should-not-call.webp" };
+      },
+    });
+
+    // Should NOT retry and return null immediately
+    expect(result).toEqual({ url: null });
+    expect(produceCalled).toBe(false);
+  });
+
+  it("caches notFound flag when returned by producer", async () => {
+    const indexKey = ns("test", "asset6");
+    const lockKey = ns("lock", "test", "asset6");
+
     const result = await getOrCreateCachedAsset<{ source: string }>({
       indexKey,
       lockKey,
       ttlSeconds: 60,
-      produceAndUpload: async () => ({ url: null }),
+      produceAndUpload: async () => ({ url: null, notFound: true }),
     });
 
     expect(result).toEqual({ url: null });
 
-    // Null should still be written to cache
+    // notFound flag should be stored in cache
     const { redis } = await import("@/lib/redis");
     const stored = (await redis.get(indexKey)) as {
       url?: string | null;
+      notFound?: boolean;
     } | null;
     expect(stored?.url).toBe(null);
+    expect(stored?.notFound).toBe(true);
   });
 
   it("schedules blob deletion with grace period beyond Redis TTL", async () => {

lib/cache.ts

@@ -143,6 +143,7 @@ type CachedAssetOptions<TProduceMeta extends Record<string, unknown>> = {
   produceAndUpload: () => Promise<{
     url: string | null;
     key?: string;
+    notFound?: boolean; // true if asset permanently doesn't exist (don't retry)
     metrics?: TProduceMeta;
   }>;
   /**
@@ -166,15 +167,24 @@ export async function getOrCreateCachedAsset<T extends Record<string, unknown>>(
 
   // 1) Check index
   try {
-    const raw = (await redis.get(indexKey)) as { url?: unknown } | null;
+    const raw = (await redis.get(indexKey)) as {
+      url?: unknown;
+      notFound?: unknown;
+    } | null;
     if (raw && typeof raw === "object") {
       const cachedUrl = (raw as { url?: unknown }).url;
+      const cachedNotFound = (raw as { notFound?: unknown }).notFound;
+
       if (typeof cachedUrl === "string") {
         return { url: cachedUrl };
       }
-      // Treat null as cache miss - allows retry on transient failures
-      // Null will still be written to cache with lock to prevent concurrent retries
+      // Only retry null if it's NOT marked as permanently not found
       if (cachedUrl === null) {
+        if (cachedNotFound === true) {
+          // Permanent not found - don't retry
+          return { url: null };
+        }
+        // Transient failure or legacy null - retry
         console.debug(
           `[cache] null result in cache ${indexKey}, treating as miss for retry`,
         );
@@ -244,7 +254,12 @@ export async function getOrCreateCachedAsset<T extends Record<string, unknown>>(
       const pipeline = redis.pipeline();
       pipeline.set(
         indexKey,
-        { url: produced.url, key: produced.key, expiresAtMs },
+        {
+          url: produced.url,
+          key: produced.key,
+          notFound: produced.notFound ?? undefined, // Store notFound flag if present
+          expiresAtMs,
+        },
         { ex: ttlSeconds },
       );
       if (purgeQueue && produced.url) {

server/services/favicon.ts

@@ -32,6 +32,8 @@ export async function getOrCreateFaviconBlobUrl(
     purgeQueue: "favicon",
     produceAndUpload: async () => {
       const sources = buildSources(domain);
+      let allNotFound = true; // Track if all sources returned 404/not found
+
       for (const src of sources) {
         try {
           const res = await fetchWithTimeout(
@@ -46,7 +48,13 @@ export async function getOrCreateFaviconBlobUrl(
             },
             { timeoutMs: REQUEST_TIMEOUT_MS },
           );
-          if (!res.ok) continue;
+          if (!res.ok) {
+            // Track if this was a 404 (not found) vs other error
+            if (res.status !== 404) {
+              allNotFound = false; // Server error, timeout, etc. - not a true "not found"
+            }
+            continue;
+          }
           const contentType = res.headers.get("content-type");
           const ab = await res.arrayBuffer();
           const buf = Buffer.from(ab);
@@ -81,10 +89,13 @@ export async function getOrCreateFaviconBlobUrl(
             },
           };
         } catch {
+          // Network error, timeout, etc. - not a true "not found"
+          allNotFound = false;
           // try next source
         }
       }
-      return { url: null };
+      // Return null with notFound flag if ALL sources returned 404
+      return { url: null, notFound: allNotFound };
     },
   });
 }