diff --git a/CHANGELOG.md b/CHANGELOG.md index cbc0b623..d945a921 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/packages/core/src/engine.ts b/packages/core/src/engine.ts index 481fd349..fbf859a6 100644 --- a/packages/core/src/engine.ts +++ b/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 => { - 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 { diff --git a/packages/core/test/cache.spec.ts b/packages/core/test/cache.spec.ts index 1a5cf2ea..cd996f90 100644 --- a/packages/core/test/cache.spec.ts +++ b/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 => ({ + 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 = { + 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); + }); +});