mirror of
https://github.com/dtzp555-max/ocp.git
synced 2026-07-27 07:55:07 +00:00
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
This commit is contained in:
+14
-11
@@ -393,15 +393,8 @@ const NO_CONTEXT = process.env.CLAUDE_NO_CONTEXT === "true";
|
||||
// Deliberately boot-time-only: runtime-mutable settings (e.g. maxPromptChars via the settings
|
||||
// API) are excluded because a const epoch cannot track them; truncation also only drops
|
||||
// context rather than changing the instruction set.
|
||||
// The ALIAS MAP is folded in for the same reason. Cache keys hash the model string exactly as
|
||||
// the client sent it (`const model = parsed.model || aliases.sonnet`), so a request for "opus"
|
||||
// is cached under the literal "opus" — not under the canonical id it resolved to. models.json is
|
||||
// read once at boot, so repointing an alias (e.g. opus: 4-8 -> 5) only takes effect on restart,
|
||||
// and the SQLite response_cache outlives that restart. Without the alias map in the epoch, an
|
||||
// operator running CLAUDE_CACHE_TTL > 0 keeps being served the OLD model's answers for the alias
|
||||
// until TTL expiry — silently defeating the repoint. Same bug class as #176.
|
||||
const CONFIG_EPOCH = cryptoCreateHash("sha256")
|
||||
.update(JSON.stringify([SYSTEM_PROMPT, SYSTEM_PROMPT_WRAPPER, ALLOWED_TOOLS, NO_CONTEXT, modelsConfig.aliases]))
|
||||
.update(JSON.stringify([SYSTEM_PROMPT, SYSTEM_PROMPT_WRAPPER, ALLOWED_TOOLS, NO_CONTEXT]))
|
||||
.digest("hex").slice(0, 16);
|
||||
// Kill-switch for the FIX-③ default-path spawn-home isolation (see resolveSpawnHome /
|
||||
// spawnHomeMode below). When "1", the -p/stream-json spawn always runs in the operator's
|
||||
@@ -2811,6 +2804,16 @@ 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.
|
||||
const cacheModel = MODEL_MAP[model] || model;
|
||||
const stream = parsed.stream;
|
||||
|
||||
// Validate model against known models
|
||||
@@ -2907,7 +2910,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) {
|
||||
@@ -2926,7 +2929,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);
|
||||
@@ -2975,7 +2978,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);
|
||||
|
||||
@@ -1104,6 +1104,86 @@ test("integration: toggling OCP_LOCAL_TOOLS invalidates the standard response ca
|
||||
} finally { _ltRm(dir, { recursive: true, force: true }); }
|
||||
});
|
||||
|
||||
// ── Cache keys hash the RESOLVED model, not the alias string (#194) ──────────
|
||||
// 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 keep serving the
|
||||
// OLD model's answers under that alias until TTL expiry. Rather than mutate models.json
|
||||
// mid-suite, these assert the equivalent observable: an alias and its canonical target must
|
||||
// land on the SAME cache slot, which is true only if the key is resolved before hashing.
|
||||
// Mutation: change `cacheModel` back to `model` at the three cacheHash call sites in
|
||||
// server.mjs and both tests go red (2 spawns instead of 1).
|
||||
|
||||
// Fake that emits schema-valid JSON, so the structured path caches a VALIDATED result
|
||||
// (the stock LT_FAKE returns "OK", which fails validation → refusal → never cached).
|
||||
const LT_FAKE_JSON = `#!/bin/sh
|
||||
if [ -n "$SP_COUNTER" ]; then c=$(cat "$SP_COUNTER" 2>/dev/null || echo 0); echo $((c+1)) > "$SP_COUNTER"; fi
|
||||
printf '%s\\n' '{"type":"assistant","message":{"content":[{"type":"text","text":"{\\"ok\\":true}"}]}}'
|
||||
printf '%s\\n' '{"type":"result"}'
|
||||
exit 0
|
||||
`;
|
||||
function ltFakeJson(dir) { const p = join(dir, "claude-json"); _ltWrite(p, LT_FAKE_JSON); _ltChmod(p, 0o755); return p; }
|
||||
const LT_SCHEMA = { type: "object", properties: { ok: { type: "boolean" } }, required: ["ok"], additionalProperties: false };
|
||||
|
||||
console.log("\nCache key resolves the model alias (#194):");
|
||||
|
||||
test("integration: an alias and its canonical target share ONE cache slot (normal path)", async () => {
|
||||
if (!LT_POSIX) return;
|
||||
const dir = ltMkdir(); const fake = ltFake(dir); const counter = join(dir, "spawns.txt");
|
||||
const { child, buf } = ltBoot({ CLAUDE_BIN: fake, CLAUDE_PROXY_PORT: "39360", CLAUDE_CACHE_TTL: "60000", SP_COUNTER: counter }, dir);
|
||||
try {
|
||||
assert.ok(await ltWait(() => buf.out.includes("listening on")), `did not start: ${buf.err.slice(0, 200)}`);
|
||||
_ltWrite(counter, "0");
|
||||
const msgs = [{ role: "user", content: "alias-resolution-probe" }];
|
||||
await ltPost(39360, { model: "sonnet", messages: msgs }); // miss → spawn
|
||||
await ltWait(() => (Number(_ltRead(counter, "utf8")) || 0) >= 1, 3000);
|
||||
await ltPost(39360, { model: "claude-sonnet-5", messages: msgs }); // same resolved model → HIT
|
||||
await new Promise(r => setTimeout(r, 600));
|
||||
assert.equal(Number(_ltRead(counter, "utf8")) || 0, 1,
|
||||
"the canonical id must hit the slot the alias populated — a 2nd spawn means the key still hashes the raw alias");
|
||||
} finally { child.kill("SIGKILL"); _ltRm(dir, { recursive: true, force: true }); }
|
||||
});
|
||||
|
||||
test("integration: an alias and its canonical target share ONE cache slot (STRUCTURED path)", async () => {
|
||||
if (!LT_POSIX) return;
|
||||
const dir = ltMkdir(); const fake = ltFakeJson(dir); const counter = join(dir, "spawns.txt");
|
||||
const { child, buf } = ltBoot({ CLAUDE_BIN: fake, CLAUDE_PROXY_PORT: "39361", CLAUDE_CACHE_TTL: "60000", SP_COUNTER: counter }, dir);
|
||||
try {
|
||||
assert.ok(await ltWait(() => buf.out.includes("listening on")), `did not start: ${buf.err.slice(0, 200)}`);
|
||||
_ltWrite(counter, "0");
|
||||
const rf = { type: "json_schema", json_schema: { name: "probe", schema: LT_SCHEMA } };
|
||||
const msgs = [{ role: "user", content: "structured-alias-probe" }];
|
||||
await ltPost(39361, { model: "sonnet", messages: msgs, response_format: rf });
|
||||
await ltWait(() => (Number(_ltRead(counter, "utf8")) || 0) >= 1, 4000);
|
||||
await ltPost(39361, { model: "claude-sonnet-5", messages: msgs, response_format: rf });
|
||||
await new Promise(r => setTimeout(r, 600));
|
||||
assert.equal(Number(_ltRead(counter, "utf8")) || 0, 1,
|
||||
"structured cache key must resolve the alias too — this is the path the epoch-only fix missed");
|
||||
} finally { child.kill("SIGKILL"); _ltRm(dir, { recursive: true, force: true }); }
|
||||
});
|
||||
|
||||
test("integration: a config change invalidates the STRUCTURED cache too (closes the #177 gap)", async () => {
|
||||
if (!LT_POSIX) return;
|
||||
const dir = ltMkdir(); const fake = ltFakeJson(dir); const counter = join(dir, "spawns.txt");
|
||||
const rf = { type: "json_schema", json_schema: { name: "probe", schema: LT_SCHEMA } };
|
||||
const req = { model: "sonnet", messages: [{ role: "user", content: "structured-epoch-probe" }], response_format: rf };
|
||||
const bootOnce = async (env, port) => {
|
||||
const { child, buf } = ltBoot({ CLAUDE_BIN: fake, CLAUDE_PROXY_PORT: String(port), CLAUDE_CACHE_TTL: "60000", SP_COUNTER: counter, ...env }, dir);
|
||||
try {
|
||||
assert.ok(await ltWait(() => buf.out.includes("listening on")), `did not start: ${buf.err.slice(0, 200)}`);
|
||||
_ltWrite(counter, "0");
|
||||
await ltPost(port, req);
|
||||
await ltWait(() => (Number(_ltRead(counter, "utf8")) || 0) >= 1, 4000);
|
||||
return Number(_ltRead(counter, "utf8")) || 0;
|
||||
} finally { child.kill("SIGKILL"); }
|
||||
};
|
||||
try {
|
||||
const off = await bootOnce({}, 39362); // caches under epoch(negative wrapper)
|
||||
const on = await bootOnce({ OCP_LOCAL_TOOLS: "1" }, 39363); // same DB, epoch differs → must re-spawn
|
||||
assert.equal(off, 1, "first structured request (cache empty) must spawn claude");
|
||||
assert.equal(on, 1, "structured cache must honor CONFIG_EPOCH — before #194 it omitted the epoch entirely and served the stale answer");
|
||||
} finally { _ltRm(dir, { recursive: true, force: true }); }
|
||||
});
|
||||
|
||||
// ── Upgrade Tests ──
|
||||
import { runUpgrade, postFlightOk } from "./scripts/upgrade.mjs";
|
||||
|
||||
|
||||
Reference in New Issue
Block a user