fix(core): an accepted non-2xx no longer becomes a cached absence (#704)

CHANGELOG.md

@@ -566,6 +566,13 @@ npm release are grouped under the in-development version that introduced them.
 
 ### Fixed
 
+- **An accepted non-2xx no longer becomes a cached absence.**
+  ([#704](https://github.com/rejifald/StitchAPI/issues/704)) The store gate was `out.ok` alone, so
+  `verdict: { accept: [404] }` cached the absence behind the `404` — masking a record created after
+  it for the whole TTL, and (a hit replays without re-running `interpret`/`verdict`) letting that
+  entry reach a reader whose own verdict rejects the status. The gate is now accept-blind (`< 400`),
+  so only a status every reader counts as a success on its own merits is stored.
+
 - **Adding `cache: { ttl }` no longer turns a handled vendor failure into a process exit.**
   ([#670](https://github.com/rejifald/StitchAPI/issues/670)) A cached stitch whose vendor returned
   `503` emitted an **unhandled promise rejection**, which under Node's default

packages/core/src/engine.ts

@@ -1665,8 +1665,22 @@ async function* runCached(
     }
 
     yield cacheEvt('miss');
+    // What may be WRITTEN is a narrower question than what succeeded (issue #704 §1). `verdict.accept`
+    // makes a declared non-2xx a successful outcome, so gating on `out.ok` alone commits the ABSENCE
+    // behind a `404` to the store: a record created a second later stays masked for the whole TTL. And
+    // the hit path above replays an entry as `resultEvt(found.data, found.status, 0)` WITHOUT re-running
+    // `interpret`/`verdict`, so that stored `404` would also reach a reader whose own verdict rejects it
+    // — the entry's accept list belonging to its WRITER rather than its reader.
+    //
+    // `< 400` is `classifyStatus`'s own threshold minus the accept term: exactly the statuses every
+    // reader counts as a success on its own merits, so a stored entry is sound no matter who reads it
+    // back. Deliberately inline and accept-BLIND by construction rather than a `ctl.` predicate or a
+    // `bypass:` trace event: `runCached` is in the core entry, which sits under a full bundle budget
+    // (scripts/bundle-size.mjs), and neither spelling fits in the headroom. Comments are free at
+    // runtime, so the reasoning lives here instead — the skip is silent, but nothing unsound is kept.
     const store = async (out: RunOutcome): Promise<void> => {
-        if (out.ok) await op.set(out.value, out.status, out.vary);
+        if (out.ok && out.status < 400)
+            await op.set(out.value, out.status, out.vary);
     };
 
     // Coalescing disabled: run the chain, write the cache last.
@@ -1686,6 +1700,11 @@ async function* runCached(
             throw e;
         }
         if (out.ok) {
+            // `settle` stays ungated: coalescing is in-process and the coalescer belongs to THIS
+            // stitch, so every follower shares the leader's verdict and its accept list. Sharing one
+            // concurrent accepted `404` among identical callers is the live behaviour they'd each
+            // have got anyway — only the durable write, which outlives the burst and can be read
+            // back by a different reader, is the one the status gate holds back.
             await store(out);
             claim.settle({ data: out.value, status: out.status });
         } else {

packages/core/test/cache.spec.ts

@@ -4,7 +4,7 @@
 // eviction, the `sensitive` bypass, principal scope isolation, re-validate-on-hit + the
 // `version` fast path, and the cacheable-method gate (GraphQL opt-in).
 import { graphql, memoryStore, seam, stitch } from '../src';
-import type { Adapter, CacheOptions } from '../src';
+import type { Adapter, CacheOptions, StitchConfig } from '../src';
 import { clearFingerprinters, registerFingerprinter } from '../src/fingerprint';
 import type { SchemaFingerprinter } from '../src/fingerprint';
 import type { StandardSchemaV1 } from '../src/standard-schema';
@@ -838,3 +838,90 @@ describe('cache — non-storable pass-through', () => {
         expect(calls()).toBe(2); // not cached
     });
 });
+
+describe('cache — an accepted non-2xx is never stored (#704 §1)', () => {
+    // A resource that does not exist YET: the first call 404s, every call after it finds the
+    // record. With `verdict: { accept: [404] }` that 404 is a normal result, so it arrives at the
+    // cache's store gate looking exactly like a success — which is the whole trap.
+    function lateArrival(): { adapter: Adapter; calls: () => number } {
+        let calls = 0;
+        const adapter: Adapter = async () => {
+            const n = (calls += 1);
+            return n === 1
+                ? { status: 404, headers: {}, body: { error: 'not found' } }
+                : { status: 200, headers: {}, body: { id: 1 } };
+        };
+        return { adapter, calls: () => calls };
+    }
+
+    const accepting = (adapter: Adapter): Partial<StitchConfig> => ({
+        url: URL,
+        adapter,
+        trace: false,
+        verdict: { accept: [404] },
+        cache: { ttl: '60s', tenancy: 'app' },
+    });
+
+    test('the absence behind an accepted 404 does not mask the record created after it', async () => {
+        const { adapter, calls } = lateArrival();
+        const s = stitch(accepting(adapter));
+        // Accepted, so it resolves with the error body as the value — that part is unchanged.
+        expect(await s()).toEqual({ error: 'not found' });
+        // The record exists now. Before the fix the stored ABSENCE answered here instead, for the
+        // whole 60s TTL, and the origin was never asked a second time.
+        expect(await s()).toEqual({ id: 1 });
+        expect(calls()).toBe(2);
+    });
+
+    test('the cache is still consulted — the 404 is read past, not bypassed', async () => {
+        // The skip is on the WRITE only: the run still misses the cache and still runs the chain,
+        // so a later 200 on the same key caches normally. (The skip itself is deliberately silent
+        // — a `bypass:` event did not fit the core entry's bundle budget; see the note at the
+        // store gate in engine.ts.)
+        const { adapter, calls } = lateArrival();
+        const s = stitch(accepting(adapter));
+        expect(await cacheTrace(s.stream())).toContain('miss'); // 404 — consulted, not stored
+        expect(await s()).toEqual({ id: 1 }); // so this reaches the origin and finds the record
+        expect(await cacheTrace(s.stream())).toContain('hit'); // …and THAT is what caches
+        expect(calls()).toBe(2); // the 404 and the record; the hit cost nothing
+    });
+
+    test('an accepted 404 cannot be replayed to a reader whose own verdict rejects it', async () => {
+        // A hit replays as `resultEvt(found.data, found.status, 0)` WITHOUT re-running
+        // `interpret`/`verdict`, so a stored entry's accept list would govern whoever reads it
+        // next. Two stitches over one store landing on one derived key (same method + url, and
+        // both fall back to the same default cache id): the WRITER accepts 404, the READER does
+        // not. Before the fix the reader was handed the writer's 404 body as a success.
+        const store = memoryStore();
+        let calls = 0;
+        const adapter: Adapter = async () => {
+            calls += 1;
+            return { status: 404, headers: {}, body: { error: 'not found' } };
+        };
+        const base: Partial<StitchConfig> = {
+            url: URL,
+            adapter,
+            trace: false,
+            store,
+            cache: { ttl: '60s', tenancy: 'app' } satisfies CacheOptions,
+        };
+        const writer = stitch({ ...base, verdict: { accept: [404] } });
+        const reader = stitch(base); // no accept list — here a 404 is a failure
+
+        expect(await writer()).toEqual({ error: 'not found' });
+        await expect(reader()).rejects.toThrow(/404/);
+        expect(calls).toBe(2); // the reader reached the origin instead of inheriting the entry
+    });
+
+    test('a 200 still caches and replays exactly as it always has', async () => {
+        const { adapter, calls } = counting(); // always 200
+        // Same accept list — it simply never fires, so the healthy path must be byte-identical.
+        const s = stitch(accepting(adapter));
+        expect(await s()).toEqual({ n: 1 });
+        expect(await s()).toEqual({ n: 1 }); // served from cache, not the origin
+        expect(calls()).toBe(1);
+        const trace = await cacheTrace(s.stream());
+        expect(trace).toContain('hit');
+        expect(trace.some((d) => d.startsWith('bypass'))).toBe(false);
+    });
+});