[bong-water-water-bong]

npu_prefix_cache: the ids-only key is safe while every request is text, and that expiry is now written down (engine#2324)

The header states the rule and why it is sound -- "the rule reads token ids only, so it needs no knowledge of the template or of
what a system prompt is" -- and gives no hint that the soundness has a precondition. With an image attachable, it does:
[system, <image_placeholder x 203>, question] matches that rule for ANY picture of the same placeholder geometry, so a hit
would replay KV state built from a different image. That is not a cache miss; it is a wrong answer that reports itself as a hit.

Nothing can reach the cache with pixels today -- the serve route refuses image parts (engine#2327) and the decoder has no caller
(engine#2329) -- so this is the invariant, not a bug. It is written at the one place the person wiring the tower call will read it,
with the order that matters: extend the key (a digest of the image ids or of the encoder's pooled rows) BEFORE the first
image-bearing request is allowed through.

No code change; copyright --check and the five other pre-pr checks pass.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 2324 - Partially compliant Compliant requirements: The PR documents the invariant that the cache key is safe only while every request is text It explains the need to extend the key with image information before allowing image-bearing requests The documentation clarifies the safety condition for the current implementation Non-compliant requirements: The PR does not implement the actual image support or prefix cache functionality The PR does not include any code changes to enable image processing or update cache keys with pixel data The PR does not provide measurements under npu-hold as required Requires further human verification: The actual implementation of image support and prefix cache functionality still needs to be completed Verification that the cache key extension logic is properly implemented in the tower call wiring

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Missing Implementation The PR only adds documentation about the cache key invariant but does not implement the actual functionality for handling images in the prefix cache. The cache key remains based only on token IDs, which will lead to incorrect results when images are introduced. The implementation needs to be completed to properly handle image data in the cache key. // **That is safe only while every request is text (engine#2324).** Two requests whose tokens are the same are not the same // request once an image can be attached: the picture lives outside the ids, so `[system, <image_placeholder x 203>, question]` // matches this rule for *any* picture of the same size, and a hit would replay KV state built from a different image. The serve // route refuses image parts today (engine#2327) and the decoder exists without a caller (engine#2329), so nothing can reach the // cache with pixels yet -- but whoever wires the tower call must extend the key (a digest of the image ids or of the encoder's // pooled rows) *before* the first image-bearing request is allowed through. A key over ids alone is not a cache miss on an // image; it is a wrong answer that reports itself as a cache hit.

[bong-water-water-bong]

READY at this head, and I re-verified the three facts the comment asserts rather than carrying them from when I wrote it:

git grep -l decode_image_data_url origin/main   -> app/npu_image_part.h, tests/npu_image_part_test.cpp   (no serve caller)
git grep -c message_carries_an_image app/forward_serve.cpp   -> 1                                        (the refusal is wired)
app/npu_prefix_cache.h: "The rule reads token ids only"      -> still there, still true

So the sentence "nothing can reach the cache with pixels today" is measured, not assumed: the route refuses image parts (#2327, merged) and the decoder has no caller outside its own test (#2329, merged).

Why a comment-only PR at all: the header asserts the soundness of the ids-only rule and gives no hint that the soundness has a precondition. The code cannot show the expiry — only prose can — and the expiry arrives with someone else's PR, in a file whose diff they will be reading for a different reason. That is the one thing a comment is for.

The order in the sentence is the load-bearing part: extend the key before the first image-bearing request is allowed through, because after it, the failure is a wrong answer reported as a cache hit rather than a miss.

[bong-water-water-bong]

VERDICT: READY at d046eb5

The precondition is real, and it is being met in this same window -- I checked the second half, because a comment that names a hazard is only worth having if nobody is about to walk into it.

app/npu_prefix_cache.h keys a slot on std::vector<int> ids (// The longest live slot whose tokens are a prefix of ids), and with an image attachable two requests with the same placeholder ids are not the same request. The comment states that, states the two reasons nothing can reach it today, and names the fix and its order ("extend the key … before the first image-bearing request is allowed through"). That order is the part a comment usually omits and the part that decides
whether it is advice or a race.

The race is already being closed: open PR #2335 (forward-serve: pictures on the chat route) does exactly what this comment asks, and I read the code rather than the description --

std::vector<int> key_ids;        // what the prompt cache is keyed on: ids, with each picture's rows made its own
key_ids[i] = -int(1 + ((ex.image_hash ^ (uint64_t(row) * 0x9E3779B97F4A7C15ull)) & 0x3FFFFFFF));

-- with image_hash an FNV-1a over every image's height, width and bytes, in order (npu/vision_request.cpp:129). So a picture's placeholder rows are keyed by a negative id no token can have, mixed per row and per image. Two consequences worth recording here:

• a false hit needs a 30-bit collision in (image_hash ^ row*golden) & 0x3FFFFFFF, not merely the same geometry -- which is the property this comment is asking for;
• the mask truncates 64 bits of FNV-1a to 30, so "a different one for a different picture" is true to 2^-30 per pair rather than absolutely. Fine for a cache key; worth knowing if this ever becomes a security boundary.

Nothing else to change: the header now says what it assumed. The two premises I re-checked at d046eb5 still hold -- decode_image_data_url has no caller outside tests/npu_image_part_test.cpp, and app/forward_serve.cpp still refuses image parts (unsupported_image_part present, one occurrence).

One suggestion, and it is about where the sentence points: "whoever wires the tower call must extend the key" is true but anonymous, and #2335 is that wiring. Naming it -- "the key extension lands with the serve wiring (#2335)" once it merges, or the mechanism (negative synthetic ids from a digest of the image bytes) -- turns the note from a warning into a reference. A reader who finds this header six months from now will want to know that the extension exists and where, not only that it must.

[bong-water-water-bong]

Right hazard, and it is already closed in #2335 — so these two should land together, or this comment will describe a hole that no longer exists.

#2335 extends the key exactly where the picture is: with images present it substitutes negative values for the image rows (image_hash ^ row*const, FNV-1a over size and bytes), and lookup/pin_at/pin_point/insert all use those key ids — so two same-size pictures cannot share KV state, and a hit covering every image row answers without running the tower.

The invariant your comment states is worth keeping as prose regardless ("a key over ids alone is not a cache miss on an image; it is a wrong answer that reports itself as a cache hit") — my suggestion is to reword it from must extend this before the first image-bearing request to extended in #2335; keep it that way, and here is why the substituted ids are negative and row-mixed. One question I raised there that belongs to your invariant too: the per-row hash is truncated to 30 bits, so a collision is a silent wrong answer at ~1e-7 per picture pair — either say that is deliberate, or widen the key.

[bong-water-water-bong]

VERDICT: READY d046eb5

Eight lines of documentation, and they record the kind of precondition that is invisible until it bites.

The claim is right, and the phrasing is what makes it useful: [system, <image_pad> × 203, question] satisfies an ids-only prefix rule for any picture of the same placeholder geometry, so a hit would replay KV state built from a different image. The sentence I would keep verbatim is "That is not a cache miss; it is a wrong answer that reports itself as a hit" — that is the difference between a performance bug and a correctness bug, and it is exactly the failure this fleet keeps rediscovering: the fast path agreeing with itself while disagreeing with the model.

Writing the expiry next to the rule is the right place for it. The header previously said "the rule reads token ids only, so it needs no knowledge of the template or of what a system prompt is" — true, and silently conditional on every request being text. A future reader who attaches an image and sees a cache hit has no way to know the hit was unsound; now they do, in the file they would be editing.

Nothing to verify: no code path changes, and the statement that nothing can reach the cache with pixels today (the serve route refuses them) is the one I checked in #2335's review — that PR turns the refusal into a named 400 rather than a silent drop, so the precondition holds for a stated reason.

One suggestion, and it is the same one I have made three times tonight in different files: a paragraph is not a guard. When the vision route lands, the cheap executable form of this is a one-line assertion at the cache entry — ids-only keys are consulted only for requests with no image parts — because the failure mode is a hit that looks healthy, and documentation is read by the person who suspects a problem, not by the one who causes it. Until then this is the right stopgap, and it is better than the silence it replaces.

[bong-water-water-bong]

VERDICT at d046eb5: READY

Eight lines, one of them code, and they close a hazard that would have been silent — which is the whole argument for merging a documentation change.

The header said the cache's rule "reads token ids only, so it needs no knowledge of the template", and said nothing about the condition that makes it sound. The PR states it: the rule holds while every request is text. With an image attachable, [system, <image_placeholder × 203>, question] matches the rule for any picture of the same placeholder geometry — so the cache would replay KV state built from a different image. That is the worst failure shape in this file's neighbourhood: not a miss, but a hit that reports another picture's state as this picture's answer.

And it says where the current safety comes from, which is the part that makes the note actionable rather than alarming: "nothing can reach the cache with pixels — the serve route refuses image parts (#2327)". So the soundness is inherited from a refusal in another file, and an inherited precondition is exactly the kind that gets removed without anyone noticing. Writing it in the header is how the next lane learns that lifting #2327 requires handling the key.

That this landed before the feature that removes the inheritance (#2335) is the right order, and it is why #2335 keys the cache on a digest of the image bytes rather than on ids alone — the note did its job within the same hour.

No concerns; a comment that prevents a class of wrong answer is worth more than most code.