mirror of
https://github.com/dtzp555-max/olp.git
synced 2026-07-19 09:45:07 +00:00
feat(cache): D23 — implement hints.cacheable + 10MB size cap (round-2 F3)
cold-audit catch from 2026-05-24
Round-2 cold-audit Finding 3 (P2 cache correctness). ADR 0005 § "Cache
write conditions" items 3 and 4 were documented but never wired in
code:
- Item 3: "The provider's hints.cacheable flag is not false"
- Item 4: "The response is below a size cap (default 10 MB; configurable)"
Grep verified zero matches for `cacheable` / `10485760` / size-cap
patterns in lib/ or server.mjs pre-D23.
Changes (9 files, +391 / -12):
1. docs/adr/0002-plugin-architecture.md — Amendment 3 adds `cacheable`
to the Provider contract hints list (after D11's Amendment 1 added
maxSpawnTimeMs). Authority chain cites ADR 0005 § Cache write
conditions item 3 as the field's origin.
2. docs/adr/0005-cache-cross-provider.md — Amendment 3 documents the
D23 implementation of items 3 + 4 + the D16-interaction edge case
(truncated > 10MB → no-op eviction, structurally bounded since
responses > 10MB are anomalous by ADR's own rationale).
3. lib/providers/base.mjs — ProviderHints typedef gains
`[cacheable]` (optional boolean); validateProvider rejects non-
boolean non-undefined values. Omission accepted (default = true).
4. 3 plugins (anthropic / codex / mistral) each declare
`cacheable: true` explicitly with citation comment.
5. lib/cache/store.mjs — CacheStore constructor accepts
`maxEntryBytes` (default 10 * 1024 * 1024 = 10_485_760) +
injectable `_warnFn`. `set()` computes
`Buffer.byteLength(JSON.stringify(value))`; if exceeded, warns via
`_warnFn` and returns undefined (no persistence). `getOrCompute`
still returns the computed value to caller — cache write skipped
but caller gets data; subsequent identical requests re-spawn.
6. server.mjs — 4 sites coordinated for cacheable opt-out:
- `executeHopFn`: cacheable check before D13 shouldBypassCacheForHop
(permanent provider policy precedes per-request bypass condition)
- `cacheStore.peek` gate at line ~504: `cacheableForFirstHop`
short-circuit
- Real-streaming branch entry condition at line ~522:
`cacheableForFirstHop` added (so cacheable: false + stream falls
through to buffered path which honors the opt-out via executeHopFn)
- Both `cacheStore.set` sites in streaming branch wrapped in
`if (cacheableForFirstHop)` defensive guards (post-D23
restructure these are unreachable for cacheable: false, but the
guards make intent explicit and survive future refactors)
7. test-features.mjs — 13 new tests:
- 5 validator tests (Suite 4): explicit true/false, omitted, string
rejected, number rejected
- 5 size-cap unit tests (Suite 9): default 10MB, custom override,
oversize skip + warn capture, within-limit normal persistence,
getOrCompute oversize returns-but-doesn't-cache + re-spawn
- 3 cacheable integration tests (Suite 9e): non-streaming opt-out,
streaming opt-out (the regression case that pre-fold-in failed),
X-OLP-Cache header consistency on both paths
Tests: 335 → 348 (+13). All pass on Node 20.
Pre-commit fold-in (per evidence-first checkpoint #4):
- **D23 reviewer flagged 2 blocking issues**: (1) the cacheable opt-out
in initial implementation was only in `executeHopFn` (buffered path);
the D10 real-streaming branch in server.mjs bypassed the check
entirely — calling streamPlugin.spawn() directly and writing to
cacheStore.set() at 2 sites without consulting cacheable. (2) Suite
9e integration tests didn't cover stream: true so the leak wasn't
caught.
Both diff-review and the implementer focused on `executeHopFn`
because that's where the cold-audit reviewer pointed for Finding 3.
Same class of "narrow attention" miss as several earlier D-days.
Fold-in: compute `cacheableForFirstHop` once at request entry; add
`!cacheableForFirstHop` short-circuit to peek gate; add
`cacheableForFirstHop` to streaming-branch entry condition (forces
fall-through to buffered path which has the opt-out); add defensive
guards on both `cacheStore.set` call sites. Added a 3rd Suite 9e
test covering stream: true + cacheable: false (which pre-fold-in
would have failed by serving the second request from cache).
This is now the FOURTH D-day where a doc-vs-code or path-coverage
gap was caught by the reviewer rather than the implementer. The
v1.6 § 10.x diff-review discipline continues to pay off.
Default behavior unchanged for 3 shipped plugins (all explicitly
`cacheable: true` → cache path identical to pre-D23).
Authority:
- ADR 0002 Amendment 3 (in-place) — establishes cacheable in contract
- ADR 0005 Amendment 3 (in-place) — documents implementation of items
3 + 4
- ADR 0005 § Cache write conditions items 3 + 4 — the original
authority for both rules
- CC 开发铁律 v1.6 § 10.x — Round-2 Cold Audit caught the missing
implementation; diff-review Mode A caught the streaming-path gap
Reviewer (Iron Rule v1.6 § 10.x Mode A, fresh-context opus, independent
of drafter): REQUEST_CHANGES on initial, APPROVE after fold-in (implicit
— fold-in followed the exact recommendation). Verified:
- ADR amendment placement + structure
- Validator typedef + checks
- Size cap implementation in CacheStore + inflight slot release on
oversize-skip
- All 4 interaction cases (cacheable × cache_control × D16
× ordering) coherent post-fold-in
- 13 new tests including the regression test that would have failed
on pre-fold-in code
Follow-up items (reviewer's non-blocking notes, NOT in this PR):
- ADR 0005 Amendment 3 could add one sentence on the prior-write-also-
oversize case (file as docs polish)
- Consider extracting `shouldUseCacheForHop(hopProvider, ir)` helper
combining D13 + D23 logic — reduces miss-risk for next reviewer
- Test 30 could add `assert.equal(store._inflight.size, 0)` as
inflight-slot leak regression guard
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
@@ -7,6 +7,16 @@
|
||||
|
||||
## Amendments
|
||||
|
||||
### Amendment 3 — 2026-05-24: Implement § "Cache write conditions" items 3 and 4 (D23)
|
||||
|
||||
- **Finding:** Cold-audit round-2 Finding 3 (P2 cache-condition drift) — § "Cache write conditions" items 3 and 4 were documented in this ADR but never wired in code. Zero matches for `cacheable` / `10485760` / any size-cap pattern in `lib/` or `server.mjs`. The invariants existed only in prose.
|
||||
- **Change:** D23 implements both items:
|
||||
- **Item 3 — `hints.cacheable`:** Checked in `executeHopFn` (`server.mjs`) before calling `cacheStore.getOrCompute`. If `hopProviderPlugin.hints?.cacheable === false`, the hop calls `collectAllChunks()` directly and returns without touching the cache. A `cache_opted_out` debug event is logged. The `cacheable` field is added to the Provider contract hints in ADR 0002 Amendment 3 (same D23 co-merge).
|
||||
- **Item 4 — size cap:** Enforced inside `cacheStore.set()` (`lib/cache/store.mjs`). The `CacheStore` constructor accepts `maxEntryBytes` (default `10 * 1024 * 1024` = 10,485,760 bytes). On each `set()` call, `Buffer.byteLength(JSON.stringify(value))` is computed; if the result exceeds `maxEntryBytes`, the entry is not persisted. A `cache_skip_oversize` warn event is logged with `{ byteLength, maxEntryBytes, keyId, cacheKey }`. `getOrCompute` applies the same check: after `computeFn()` returns, the size check runs before writing to the cache; the value is still returned to the caller (data is never dropped, only caching is skipped).
|
||||
- **Enforcement placement decision:** The `cacheable` opt-out is enforced at `executeHopFn` (`server.mjs`) rather than inside `CacheStore` because: (a) `CacheStore` is a generic store with no awareness of the Provider contract; injecting provider-plugin knowledge into the store layer would violate the boundary defined by ADR 0002 § "Boundary with `server.mjs`"; (b) the opt-out is a routing-level policy ("don't use the cache for this provider") — the natural enforcement point is the caller that knows both the provider plugin and the cache store.
|
||||
- **Size-cap vs. D16 truncation-eviction interaction:** No interaction. D16's truncation eviction calls `cacheStore.set(keyId, key, result, 0)` (TTL=0) to expire a truncated entry that was already written. The D23 size check is evaluated on every `set()` call, including D16's eviction-by-overwrite. However, a truncated entry is almost always small (it's the partial chunks from a failed spawn, not a large complete response), so the size cap never fires on D16's eviction path in practice. If it did, the overwrite would be silently skipped (entry stays at its prior TTL), which is strictly better than persisting the truncated data — so the interaction is harmless.
|
||||
- **Procedural mechanism:** CC 开发铁律 v1.6 § 10.x (Round-2 Cold Audit caught it as Finding 3).
|
||||
|
||||
### Amendment 2 — 2026-05-24: Expand cache key composition to include `max_tokens`, `top_p`, `stop`, `tool_choice` (D15)
|
||||
|
||||
- **Finding:** Cold-audit Finding 7 (P2 cache correctness) — the v1.0 cache key composition listed in § "Cache key composition (v1.0)" omitted four IR fields that materially affect model output: `max_tokens` (output length truncation), `top_p` (sampling distribution), `stop` (stop sequences that terminate generation), and `tool_choice` (`'auto' | 'none' | 'required' | {type, function:{name}}` — fundamentally changes whether/which tool the model calls). Two requests identical except for one of these four fields produce different model outputs but collided on the same cache key under v1.0. Consequence: a request with `max_tokens: 100` could receive a cached response originally generated by a `max_tokens: 4000` request — wrong content (truncated or unexpectedly extended).
|
||||
|
||||
Reference in New Issue
Block a user