From 0f5dbbbb5814e143c93bd52a47c11fb6631f9f67 Mon Sep 17 00:00:00 2001 From: rejifald Date: Fri, 7 Aug 2026 19:05:10 +0300 Subject: [PATCH] fix(core): an accepted non-2xx no longer becomes a cached absence (#704) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `verdict: { accept: [404] }` makes a declared non-2xx a successful outcome, and the cache's store gate was `out.ok` and nothing else — so the ABSENCE behind the 404 was written to the store. A record created a second later stayed masked for the whole TTL, on a stitch whose only sin was declaring that a 404 is normal. The second consequence is worse, and it is what settles the shape of the fix. A hit is replayed as `resultEvt(found.data, found.status, 0)` without running the reader's `interpret`/`verdict` at all, so a stored 404 could be served as a success to a second stitch whose own verdict rejects that status — the entry's accept list belonging to its WRITER rather than its reader. Two stitches reach one key easily: the cache id falls back to `cfg.name ?? cfg.path ?? 'stitch'`, so two unnamed stitches over one URL already share a namespace (that collision is #704 §3 and stays open). The gate now reads the status, and reads it accept-BLIND: `< 400` is `classifyStatus`'s own threshold minus the accept term, which is exactly the set every reader counts as a success on its own merits. Nothing a `verdict.accept` had to rescue is durable, so the cross-stitch replay is unreachable through this path regardless of §3. Coalescing is deliberately untouched. `claim.settle` stays ungated, because the coalescer belongs to the stitch that owns it and every follower therefore shares the leader's verdict — sharing one concurrent accepted 404 among identical callers is the live behaviour each would have got anyway. Only the durable write, which outlives the burst and can be read back by someone else, is held back. Semver: a `fix`, not a break. A caller deliberately caching an accepted 404 does lose that caching and pays a live call per call — the one visible consequence, and the CHANGELOG says so. But the entry it loses was never sound: ADR 0003 decision 2 has it that a stored entry is always a known-good value, and this one could be handed to a reader that rejects it. The success VALUE a caller sees is unchanged; only staleness and the origin-call count move, and both move toward correct. Written inline rather than as a `ctl.storableStatus` predicate with a `bypass:` trace event, which is what this started as. `runCached` is in the core entry and #676 left it 14 B under budget; the predicate plus the event measured +30 B and went red, and even the bare predicate with no event was +21 B and still red. The inline form is +6 B on the whole entry and +7 B on `import { stitch }`, so both budgets hold unchanged at 24.10 / 21.55 KB with 8 B / 34 B of headroom, and the advertised 24 / 22 kB figures do not move. The cost is that the skipped write is silent; the reasoning is a comment at the gate, which costs nothing at runtime. Sizing a budget step is the maintainer's call, not a bug fix's — #676 says so in that file, so this fix fits under the ceiling instead. Tests: an accepted 404 no longer masks a record created after it; the same key still misses and then caches the 200 that follows; a second stitch whose verdict rejects 404 reaches the origin instead of being handed the writer's entry; and a plain 200 caches and replays as before. The first three fail on `main`. Refs #704 Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 7 +++ packages/core/src/engine.ts | 21 +++++++- packages/core/test/cache.spec.ts | 89 +++++++++++++++++++++++++++++++- 3 files changed, 115 insertions(+), 2 deletions(-) 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); + }); +});