mirror of
https://github.com/dtzp555-max/olp.git
synced 2026-07-21 21:15:10 +00:00
fix(cache): D13 — per-hop cache_control bypass evaluation (ADR 0005 § D2)
cold-audit catch from 2026-05-23
Cold-audit Finding 2 (P2 cache correctness). Pre-D13 server.mjs:325
computed `bypassCache` once per request globally and passed it unchanged
into every chain hop. Per ADR 0005 § D2, the bypass must only fire when
the active hop's provider is Anthropic. Pre-D13 Codex/Mistral hops in
fallback chains wrongly bypassed OLP's response cache whenever the
original body had a cache_control marker, defeating per-model cache.
Changes (server.mjs +54/-12, test-features.mjs +218):
1. Replaced the global `bypassCache` const with:
- `hasCacheControlMarkers` (request-level boolean, computed once)
- `shouldBypassCacheForHop(hopProviderName)` (helper, returns
hasCacheControlMarkers && hopProviderName === 'anthropic')
2. Refactored 4 call sites to use the helper:
- executeHopFn: bypassCacheForThisHop = shouldBypassCacheForHop(hopProvider)
— log now includes provider field for observability
- preCheckHit: gated on shouldBypassCacheForHop(chain[0].provider)
— first-hop scope (correct for the pre-loop peek)
- Real-streaming gate (D10): same bypassCacheForFirstHop — single-hop
streaming so first === serving
- cacheStatus header: shouldBypassCacheForHop(providerUsed) — reports
SERVING hop's bypass status. Semantic improvement: a fallback chain
anthropic→openai where openai serves now correctly reports
`X-OLP-Cache: miss` rather than the pre-D13 global `bypass`
No engine.mjs change required — executeWithFallback already passes
hopProvider as a string into executeHopFn.
No marker-strip step needed — verified that openai-to-ir.mjs's
translateMessage drops cache_control from the IR object (copies only
role/content/name/tool_call_id/tool_calls/function_call). Non-Anthropic
plugins never see the markers. Cache key over the IR is therefore
identical for "same prompt with markers" and "same prompt without
markers" on non-Anthropic hops — caching is safe and beneficial.
Tests: 288 → 291 (+3 in new Suite 9d):
- Test 31: openai + cache_control → X-OLP-Cache: miss (the fix's core
assertion)
- Test 32: anthropic + cache_control → X-OLP-Cache: bypass (regression
guard, preserves Anthropic behavior)
- Test 33: 2-hop chain anthropic→openai, anthropic hard-fails, openai
serves → X-OLP-Cache: miss + X-OLP-Provider-Used: openai
(per-hop correctness in fallback)
Authority:
- ADR 0005 § D2: "If the IR request contains Anthropic cache_control
markers AND the active provider in the current chain hop is Anthropic,
the OLP response cache is bypassed... If the active provider is not
Anthropic, the cache_control markers are stripped from the IR before
provider translation"
https://github.com/dtzp555-max/olp/blob/main/docs/adr/0005-cache-cross-provider.md
- ADR 0004 § Observability headers (X-OLP-Cache semantics for the
serving hop)
https://github.com/dtzp555-max/olp/blob/main/docs/adr/0004-fallback-engine.md
Reviewer (Iron Rule v1.6 § 10.x Mode A, fresh-context opus, independent
of drafter): APPROVE.
Key verification: reviewer read openai-to-ir.mjs::translateMessage
end-to-end to verify cache_control markers never propagate into IR
(the load-bearing claim — if markers DID propagate, D13 would have
been incomplete and needed a strip step). Confirmed: markers are
dropped at IR translation, no additional strip needed in D13.
Reviewer also walked 4 scenarios (anthropic-only no markers,
anthropic-only with markers, mixed chain anthropic-fail openai-serves,
mixed chain openai-first) against the post-D13 code; all match the
expected per-hop semantics.
Follow-up tracked separately: ADR 0005 § D2 mentions "logged once per
request at debug level" for non-Anthropic noop case; post-D13 the
log fires only for actual bypass (Anthropic + markers). Reviewer's
non-blocking suggestion to either add the log or amend the ADR will
be tracked as a GitHub issue.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
+41
-11
@@ -320,12 +320,26 @@ async function handleChatCompletions(req, res) {
|
||||
// real OLP API key ID here for D1 per-key isolation.
|
||||
const keyId = '__anonymous__';
|
||||
|
||||
// D2 bypass: if the request contains Anthropic cache_control markers,
|
||||
// skip OLP's response cache (prompt cache lives at Anthropic's side per ADR 0005 § D2).
|
||||
const bypassCache = hasCacheControl(ir) || extractCacheControlMarkers(body?.messages ?? []).length > 0;
|
||||
// D2 bypass (per-hop, per ADR 0005 § D2):
|
||||
// cache_control markers bypass OLP's response cache ONLY when the active hop
|
||||
// provider is Anthropic. For non-Anthropic hops the markers are noop'd
|
||||
// (openai-to-ir already strips them from the IR before provider translation,
|
||||
// so they never reach a non-Anthropic plugin — no separate strip needed here).
|
||||
//
|
||||
// Pre-compute whether the raw request carries any cache_control markers at all.
|
||||
// The per-hop decision is: markers present AND hop is Anthropic → bypass.
|
||||
const hasCacheControlMarkers =
|
||||
hasCacheControl(ir) || extractCacheControlMarkers(body?.messages ?? []).length > 0;
|
||||
|
||||
if (bypassCache) {
|
||||
logEvent('debug', 'cache_bypass', { model: ir.model, reason: 'cache_control_markers' });
|
||||
/**
|
||||
* Returns true if OLP's response cache should be bypassed for the given hop.
|
||||
* Per ADR 0005 § D2: bypass only when provider is Anthropic AND markers present.
|
||||
*
|
||||
* @param {string} hopProviderName — e.g. 'anthropic', 'openai', 'mistral'
|
||||
* @returns {boolean}
|
||||
*/
|
||||
function shouldBypassCacheForHop(hopProviderName) {
|
||||
return hasCacheControlMarkers && hopProviderName === 'anthropic';
|
||||
}
|
||||
|
||||
// ── executeHopFn: per-hop spawn + cache wrapper ─────────────────────────
|
||||
@@ -369,7 +383,15 @@ async function handleChatCompletions(req, res) {
|
||||
return chunks;
|
||||
}
|
||||
|
||||
if (bypassCache) {
|
||||
// D13: per-hop bypass evaluation (ADR 0005 § D2).
|
||||
// Bypass only when this hop's provider is Anthropic AND markers are present.
|
||||
const bypassCacheForThisHop = shouldBypassCacheForHop(hopProvider);
|
||||
if (bypassCacheForThisHop) {
|
||||
logEvent('debug', 'cache_bypass', {
|
||||
model: hopModel,
|
||||
provider: hopProvider,
|
||||
reason: 'cache_control_markers',
|
||||
});
|
||||
return collectAllChunks();
|
||||
}
|
||||
|
||||
@@ -381,15 +403,19 @@ async function handleChatCompletions(req, res) {
|
||||
|
||||
// ── Execute with fallback (ADR 0004) ────────────────────────────────────
|
||||
// Pre-check for cache status reporting uses first hop's key (primary provider).
|
||||
// D13: preCheckHit is gated on whether the first hop would bypass — if it would
|
||||
// bypass (anthropic + markers), the cache is not consulted (preCheckHit=false).
|
||||
// If the first hop is non-Anthropic (or no markers), the cache peek proceeds normally.
|
||||
const bypassCacheForFirstHop = shouldBypassCacheForHop(chain[0].provider);
|
||||
const firstHopCacheKey = computeCacheKey(chain[0].provider, chain[0].model, ir);
|
||||
const preCheckHit = bypassCache ? false : await cacheStore.peek(keyId, firstHopCacheKey);
|
||||
const preCheckHit = bypassCacheForFirstHop ? false : await cacheStore.peek(keyId, firstHopCacheKey);
|
||||
|
||||
// ── P1.2: Real SSE streaming path (single-hop cache-miss) ──────────────
|
||||
// ADR 0003 entry adapter pattern: for await irChunk → res.write(irChunkToOpenAISSE).
|
||||
// Condition: streaming + single-hop + no bypass + no pre-check cache hit.
|
||||
// - stream===true → caller wants SSE
|
||||
// - chain.length===1 → no fallback needed; first-chunk rule allows streaming
|
||||
// - !bypassCache + !preCheckHit → genuine cache miss (not hit/bypass)
|
||||
// - !bypassCacheForFirstHop + !preCheckHit → genuine cache miss (not hit/bypass)
|
||||
//
|
||||
// If any chunk has been written (firstChunkEmitted), fallback is impossible
|
||||
// per ADR 0004 § Fallback safety first-chunk rule. On error after first chunk:
|
||||
@@ -398,7 +424,7 @@ async function handleChatCompletions(req, res) {
|
||||
//
|
||||
// On success: write chunks to res AND cache so subsequent identical requests
|
||||
// hit the burst-replay path.
|
||||
if (ir.stream && chain.length === 1 && !bypassCache && !preCheckHit) {
|
||||
if (ir.stream && chain.length === 1 && !bypassCacheForFirstHop && !preCheckHit) {
|
||||
const streamProvider = chain[0].provider;
|
||||
const streamModel = chain[0].model;
|
||||
const streamCacheKey = computeCacheKey(streamProvider, streamModel, ir);
|
||||
@@ -561,12 +587,16 @@ async function handleChatCompletions(req, res) {
|
||||
// ── Success: emit response ─────────────────────────────────────────────
|
||||
// Per ADR 0004 § Observability headers: X-OLP-Fallback-Hops reflects the
|
||||
// chain index of the serving hop; 0 = primary served, 1 = first fallback, etc.
|
||||
const cacheStatus = bypassCache ? 'bypass' : (preCheckHit && fallbackHops === 0 ? 'hit' : 'miss');
|
||||
// D13: bypass status is per the SERVING hop's provider (providerUsed), not a
|
||||
// global flag. If the serving provider was Anthropic and markers were present,
|
||||
// the cache was bypassed; otherwise it was a hit (preCheckHit) or miss.
|
||||
const bypassCacheForServingHop = shouldBypassCacheForHop(providerUsed);
|
||||
const cacheStatus = bypassCacheForServingHop ? 'bypass' : (preCheckHit && fallbackHops === 0 ? 'hit' : 'miss');
|
||||
const headers = olpHeaders({ providerUsed, modelUsed, startMs, cacheStatus, fallbackHops });
|
||||
|
||||
if (ir.stream) {
|
||||
// Streaming response path: burst replay from buffered chunks.
|
||||
// Reaches here only when: bypassCache=true OR preCheckHit=true OR chain.length>1.
|
||||
// Reaches here only when: bypassCacheForFirstHop=true OR preCheckHit=true OR chain.length>1.
|
||||
// (Single-hop cache-miss streaming is handled by the real-streaming path above.)
|
||||
res.writeHead(200, {
|
||||
'Content-Type': 'text/event-stream',
|
||||
|
||||
Reference in New Issue
Block a user