taodeng/ocp
1
0
Fork 0
mirror of https://github.com/dtzp555-max/ocp.git synced 2026-07-27 07:55:07 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(server): hash the resolved model in cache keys; give the structured path its epoch (#194)

Browse Source 
* fix(server): fold the alias map into CONFIG_EPOCH so alias repoints invalidate the cache

Cache keys hash the model string exactly as the client sent it — `const model =
parsed.model || modelsConfig.aliases.sonnet` (server.mjs:2806), which then goes
straight into `cacheHash(model, ...)`. A request for "opus" is therefore cached
under the literal string "opus", not under the canonical id it resolved to.

models.json is read once at boot, so repointing an alias only takes effect on
restart — and the response cache is SQLite-backed (keys.mjs getCachedResponse
queries the `response_cache` table), so it OUTLIVES that restart. CONFIG_EPOCH
folded four config values into every cache key for exactly this hazard (#176 /
#177) but omitted the alias map. Consequence: an operator running
CLAUDE_CACHE_TTL > 0 who repoints an alias keeps being served the OLD model's
answers under that alias until TTL expiry — silently defeating the repoint.

Fix: add `modelsConfig.aliases` to the epoch digest, so any alias change is an
instant whole-cache invalidation. This is the mechanism #177 already
established for the other config inputs, applied to the one it missed; it
covers the normal, structured and singleflight paths at once because they all
key off the same epoch, and it needs no change at the three call sites.

Reproduced end-to-end on the real server, both directions (NODE_ENV=test +
OCP_DIR_OVERRIDE, isolated SQLite store, CLAUDE_CACHE_TTL=600000):

  1. boot with aliases.opus = claude-opus-4-8, request model:"opus"
     -> claude_spawned model=claude-opus-4-8, response cached
  2. stop, repoint aliases.opus -> claude-sonnet-5, restart
  3. same request again:
       on origin/main : cache_hit    (stale Opus 4.8 answer, no spawn)
       on this branch : cache MISS -> claude_spawned model=claude-sonnet-5

Default CLAUDE_CACHE_TTL is 0 (caching off), so shipped default behavior is
unaffected; this bites only operators who deliberately enabled the cache.

cli.js citation: NOT APPLICABLE — no cli.js-derived wire behavior is touched.
Scope justified under ALIGNMENT.md Rule 2 as OCP-owned cache bookkeeping:
cli.js has no response cache and no equivalent operation, and the spawn
arguments and protocol are unchanged.

Found by an independent codex review (gpt-5.6-sol, reasoning=high) of the
Opus 5 alias repoint in #192; verified first-hand before applying rather than
taken on the reviewer's word.

Tests: 449 passed, 0 failed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017gbqUZ8HfBZpjjbzQ85oH8

* fix(server): hash the RESOLVED model in cache keys; give the structured path its epoch

Supersedes the first attempt on this branch, which folded modelsConfig.aliases
into CONFIG_EPOCH. An independent reviewer falsified that approach with a live
repro: the structured-output cache key never reads CONFIG_EPOCH at all, so
adding inputs to the epoch could not possibly fix it. My PR body claimed it
covered "normal, structured and singleflight" — that claim was false.

Two defects, both now fixed:

1. Cache keys hashed the model string as the client sent it. `model` is
   whatever arrived — a canonical id, an alias ("opus"), or a legacyAlias
   ("claude-opus-4"); MODEL_MAP carries all three. models.json is read once at
   boot, so repointing an alias only takes effect on restart, while the SQLite
   response_cache outlives it — serving the OLD model's answers under that
   alias until TTL expiry. Now all three call sites hash
   `cacheModel = MODEL_MAP[model] || model`.

   Chosen over folding the alias map into CONFIG_EPOCH because it is strictly
   better on three counts: it fixes the normal, structured AND dedup keys at
   once (they all already pass `model`); it covers legacyAliases for free; and
   it invalidates PRECISELY — only entries for the repointed alias change key,
   where the epoch approach flushed the entire cache on any alias edit,
   including unrelated sonnet/haiku rows. Only the cache KEY is resolved;
   `model` is still echoed to the client verbatim, so the wire is unchanged.

2. The structured and dedup keys omitted `configEpoch` entirely, so #177 never
   actually covered the structured path: changing SYSTEM_PROMPT (the original
   #176 scenario) still served structured answers composed under the OLD
   config. Live-verified by the reviewer, independently reproduced here. Both
   now pass `configEpoch: CONFIG_EPOCH`. This has been latent for
   structured-output clients since #153 landed.

Tests: +3, all mutation-proven — the previous attempt shipped with ZERO
coverage (reverting it left the suite at 449/0), which the reviewer proved.
Built on the existing `ltBoot` child-process harness (test-features.mjs:987)
that I had wrongly claimed did not exist; a fake CLAUDE_BIN means zero quota
cost. Rather than mutate models.json mid-suite, they assert the equivalent
observable: an alias and its canonical target must land on the SAME cache slot,
which holds only if the key is resolved before hashing.

  - revert cacheModel -> model at the 3 sites  => 2 failures (both alias tests)
  - drop configEpoch from the structured hash  => 1 failure (#177-gap test)
  - restored                                   => 452 passed, 0 failed

Blast radius, previously undisclosed: on upgrade, alias-addressed cache rows
change key once and orphan, reaped by the TTL cleanup within one window — the
same shape as the v3.13.0 v1->v2 hash upgrade already documented in README.
Literal-id rows are unaffected. Default CLAUDE_CACHE_TTL is 0, so a default
deployment sees nothing.

cli.js citation: NOT APPLICABLE — no cli.js-derived wire behavior is touched.
Scope justified under ALIGNMENT.md Rule 2 as OCP-owned cache bookkeeping;
cli.js has no response cache. The endpoint is Class B.1 under ADR 0006, which
authorizes the OpenAI-compat surface this caching sits behind.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017gbqUZ8HfBZpjjbzQ85oH8

* harden: hasOwn lookup for cacheModel + pin legacyAlias coverage with a test

Two non-blocking nits from the approving review, both worth taking:

1. `MODEL_MAP` is a plain object, so a bare `MODEL_MAP[model]` returns an
   INHERITED function for "constructor"/"toString"/"valueOf". Unreachable
   today — the VALID_MODELS gate 400s first, and it is built from
   Object.keys() so it holds only own keys — but the binding sits before that
   gate, so a bare lookup would hand cacheHash a function the day anyone
   widens the gate or moves the binding. Now Object.hasOwn-guarded.

2. legacyAlias coverage was constructive (MODEL_MAP = models + aliases +
   legacyAliases) but untested; all three tests used a plain alias. Added an
   explicit claude-haiku-4-5 vs claude-haiku-4-5-20251001 same-slot assertion.

Declined the third nit (structuredHash and dedupKey compute the same value
twice): it is cosmetic, and collapsing them changes evaluation order in a path
that is currently correct — not worth the risk in a fix PR.

Tests: 453 passed, 0 failed. Mutation now bites 3 (was 2): reverting
`cacheHash(cacheModel,` -> `cacheHash(model,` fails the normal, structured AND
legacyAlias slot tests.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017gbqUZ8HfBZpjjbzQ85oH8

---------

Co-authored-by: dtzp555 <dtzp555@gmail.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
dtzp555-max
2026-07-27 07:37:23 +10:00
committed by GitHub
co-authored by taodeng Claude Opus 5
parent a32bc9af17
commit 9f6707ee9d
2 changed files with 117 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files
server.mjs
+17 -3
View File
@@ -2804,6 +2804,20 @@ async function handleChatCompletions(req, res) {
const messages = parsed.messages || parsed.input || [{ role: "user", content: parsed.prompt || "" }];
const model = parsed.model || modelsConfig.aliases.sonnet;
// Cache keys must hash the RESOLVED model, never the string the client happened to use.
// `model` is whatever was sent — a canonical id, an alias ("opus"), or a legacyAlias
// ("claude-opus-4"). MODEL_MAP carries all three, and models.json is read once at boot, so
// repointing an alias only takes effect on restart — while the SQLite response_cache outlives
// it. Hashing the raw string would therefore keep serving the OLD model's answers under that
// alias until TTL expiry, silently defeating the repoint (the #176 hazard, for aliases).
// Resolving first also means "opus" and "claude-opus-5" correctly share one slot: identical
// spawn, identical answer. Only the cache KEY is resolved — `model` is still echoed back to
// the client verbatim, so the wire response is unchanged.
// hasOwn, not a bare lookup: MODEL_MAP is a plain object, so `MODEL_MAP["constructor"]`
// would return an inherited FUNCTION. Unreachable today (the VALID_MODELS gate below 400s
// first, and it is built from Object.keys so it holds only own keys), but a bare lookup
// would hand cacheHash a function the day anyone widens that gate or moves this binding.
const cacheModel = Object.hasOwn(MODEL_MAP, model) ? MODEL_MAP[model] : model;
const stream = parsed.stream;
// Validate model against known models
@@ -2900,7 +2914,7 @@ async function handleChatCompletions(req, res) {
const promptCharsS = messages.reduce((a, m) => a + contentToText(m.content).length, 0);
let structuredHash = null;
if (CACHE_TTL > 0 && !conversationId && !hasCacheControl(messages)) {
structuredHash = cacheHash(model, messages, { keyId: req._authKeyId, temperature: parsed.temperature, max_tokens: parsed.max_tokens, top_p: parsed.top_p, structured });
structuredHash = cacheHash(cacheModel, messages, { keyId: req._authKeyId, temperature: parsed.temperature, max_tokens: parsed.max_tokens, top_p: parsed.top_p, structured, configEpoch: CONFIG_EPOCH });
try {
const cached = getCachedResponse(structuredHash, CACHE_TTL);
if (cached) {
@@ -2919,7 +2933,7 @@ async function handleChatCompletions(req, res) {
// one-off structured request (not stateful sessions / client-side prompt caching), independent of
// whether OCP response caching is enabled; when caching IS on, the same key gates cache read/write.
const dedupKey = (!conversationId && !hasCacheControl(messages))
? cacheHash(model, messages, { keyId: req._authKeyId, temperature: parsed.temperature, max_tokens: parsed.max_tokens, top_p: parsed.top_p, structured })
? cacheHash(cacheModel, messages, { keyId: req._authKeyId, temperature: parsed.temperature, max_tokens: parsed.max_tokens, top_p: parsed.top_p, structured, configEpoch: CONFIG_EPOCH })
: null;
const runStructured = async () => {
const c = await runStructuredCompletion(upstreamCall, model, messages, conversationId, req._authKeyName, res, structured);
@@ -2968,7 +2982,7 @@ async function handleChatCompletions(req, res) {
} else {
// D1: include keyId in hash to isolate per-key cache pools (v2 format).
// configEpoch (#176): any boot-config change that shapes answers invalidates the cache.
const hash = cacheHash(model, messages, { keyId: req._authKeyId, temperature: parsed.temperature, max_tokens: parsed.max_tokens, top_p: parsed.top_p, configEpoch: CONFIG_EPOCH });
const hash = cacheHash(cacheModel, messages, { keyId: req._authKeyId, temperature: parsed.temperature, max_tokens: parsed.max_tokens, top_p: parsed.top_p, configEpoch: CONFIG_EPOCH });
req._cacheHash = hash; // store for later write-back
try {
const cached = getCachedResponse(hash, CACHE_TTL);