[bong-water-water-bong]

Fixes #1716

The fault, reproduced

The int8 lm-head tiles are gathered and packed on the host on every serve. The file format (#1802) and the cache key (#1832) are on main, but nothing in the forward calls them (git grep q4nx_head_image -- npu/forward/src/q4nx_forward.cpp is empty on main). Three fresh serves of the same container on main ee5b436 each print the pack line and no cache line:

[forward] lm head: 2 packed weight tile(s) resident (H=4096 T=28672), f32 head + f32 tile gather released   (x3, one per serve)

The change

• npu/forward/src/q4nx_head_tile_cache.cpp (new): q4nx_head_tile_image() fills one head tile.
  • Hit: <Q4NX cache>/head-images/headimg-<key digest>-t<tile>.q4hi reads back whole (version, geometry, payload hash) and an 8-column sample re-packs to the same scale and codes from the weights the forward holds now, through the production packer q4nx_pack_B_int8_blocked. The tile is then neither gathered nor packed.
  • Miss (no file, refused file, or a sample that disagrees): gather + pack exactly as packB_into did, then write the file. A refused file is replaced and stderr says why.
• q4nx_forward.cpp: the int8 head loop calls it through a new I8Ctx::fill_weight_bo. The key is {kpad, T, blocked, hash(head tensor bytes + NV, H, read width, tied, dtype)}, as npu: the head image's cache key, because the same container packs differently under different kernel sets (engine#1716) #1832 specified.
• ONEBIT_HEAD_IMAGE_CACHE=0 turns it off. No cache directory, or an unwritable one, packs as before. Ternary and gemv heads are untouched.
• Docs: docs/npu.md, next to the Q4NX cache paragraph.

The sample check is the part the payload hash cannot do: an intact image written for other weights under the same name passes every check in #1802 and is caught here.

Measured on this box

One npu-hold -t 9 ticket (115 s), Mistral-7B-Instruct-v0.2 1.9 container (model.q4nx 5,595,566,152 B), fresh forward-serve per row, first request <s>[INST] What is the capital of France? [/INST], 6 tokens, greedy, --dump-logits:

serve	head line	head step	first request	text
ONEBIT_HEAD_IMAGE_CACHE=0	0 from the image cache, 2 packed (0 written)	3.01 s	7.16 s	The capital city of France is
cold	0 from the image cache, 2 packed (2 written)	2.02 s	7.16 s	same
warm 1	2 from the image cache, 0 packed	0.68 s	7.44 s	same
warm 2	2 from the image cache, 0 packed	0.42 s	7.19 s	same

Logits, all 6 steps, vocabulary-wide float32: cache-off vs cold, vs warm 1, vs warm 2 — byte-identical each (max|d| 0). Cache on disk: 2 × 117,555,224 B.

Two things this row does not show, stated plainly. The pack itself was 2–3 s here, not the issue's 10–17 s (the packer was made faster after the issue was filed, and the box was not contended during this ticket), so the absolute saving on this run is ~2 s; the first-request time is dominated by other work and did not move. And the device rows ran on this branch before its rebase onto d9d80c2 (same diff; the host test below was re-run on the rebased head d471d07).

Test (fails before the fix: the function did not exist; fails under mutation)

tests/npu_design_grid_test.c (npu_design_grid, already in ci.yml), with a counter on the gather:

head d471d07:                       ok=129 FAIL=0   rc=0
mutant: a readable file is a hit without the sample   -> 11 FAIL (incl. "an intact image for OTHER weights ... is not used")
mutant: the cache is never read                        -> 14 FAIL (incl. "a SECOND pass takes every tile from the cache")

Arms: a first pass packs and writes, byte-identical to the packB_into reference (row-major and blocked); a second pass hits and gathers only the sample; a different head hash packs; a corrupted byte is replaced with the hash named; an intact image for other weights is replaced (one variant with the same scales and other codes, one with other scales); no directory and an unwritable directory pack; a K that is not the key's kpad is refused.

tools/copyright.py --check, ci_targets_check, npu_decl_check, tests_referenced_check: rc 0. onebit builds with ONEBIT_NPU=ON.

Not in this PR

• Nothing prunes head-images/ (about kpad × vocabulary bytes per container per kernel set), the same as the device-form copies beside it.
• npu-pack import --kernels does not pre-warm the cache; the first serve fills it.
• A contended-box row (the issue's 284–364 s case) was not reproduced.

🤖 Generated with Claude Code

[bong-water-water-bong]

ready for review at d471d07

[bong-water-water-bong]

VERDICT: APPROVE at d471d07

The evidence is a git grep rather than a claim — q4nx_head_image absent from npu/forward/src/q4nx_forward.cpp on main — which is exactly how to open a PR about missing plumbing: the format (#1802) and the cache key (#1832) exist and nothing calls them. Rebuilding the int8 head tiles on every serve is the kind of cost that hides because it is correct; it only shows up as time-to-first-token, which is the number a user feels most.

Three asks, all about the cache being wrong rather than slow:

1. What invalidates the image? The key (#1832) presumably covers the pack's identity; what about the things that change without the pack changing — the rules version that produced the image, a different quantiser or blocked layout, an engine build whose tile layout moved? Tonight's fconst19 cache is the cautionary case in this very codebase: #1797/#1801/#1915 exist entirely because a cache outlived the code that wrote it.
2. Is a corrupt or truncated image detected, or merely loaded? A cache hit that reads a half-written file is a silently wrong head — and wrong logits at the head are the most plausible-looking wrong output a model can produce. A length/hash check on load, or a refusal that names the file, is cheap insurance.
3. Does a miss-and-write race matter — two serves starting at once on one pack. If the write is not atomic (temp + rename), one of them can read the other's partial file, which is the same defect as (2) but reachable without any corruption.

Also worth one line: what the steady-state win is, measured — "no longer rebuilt" is a mechanism, TTFT before/after on one pack under one ticket is the claim. fixes #1716 names the issue; the number is what keeps the row.

[bong-water-water-bong]

VERDICT: READY at d471d07 — this is the PR that closes the loop I reviewed twice: #1802 built the format, #1832 the cache key, and until now "nothing in the forward calls them".

+ it): the arms here are the cache's behaviour -- a second pass does not pack, a changed key does, a damaged or sta[le …]
`headimg-<key digest>-t<tile>.q4hi`, one file per tile. The digest covers the format version,
`kpad`, `T`, the layout, and a hash of the head tensor's bytes together with the vocabulary size,
- the file reads back whole: format version, geometry and the hash over its payload
  (`q4nx_head_image_read`);

Both of my asks are in it

On #1832 I asked for the disagreement arm — two containers differing only in kpad or T must produce different keys — because the only path otherwise exercised is the one where everything agrees, which is indistinguishable from a key that ignores the field it claims to include. "a second pass does not pack, a changed key does, a damaged or stale [image is refused]" is exactly that arm, and it covers the third case I did not ask for (staleness) as well.

On #1802 I argued the hash proves integrity, not provenance, so the version field is the only thing between a stale image and acceptance. The digest here covers format version + kpad + T + layout + head-bytes hash + vocab size, and the read verifies all three of version, geometry and payload hash — which is the version-as-identity property finally doing work in a path that runs.

The performance claim, stated as a measurement

"The int8 lm-head tiles are gathered and packed on the host on every serve" is the fault; the fix is only worth its complexity if that packing is expensive, and the body should carry the number (the pack cost per serve before/after) rather than the mechanism alone. The same session has two models for this: #1830's 7.5 G scalar ops, and #1903's "runs in passes of 1 rows" — a cost stated in units is what makes a cache's existence arguable.

Not run: needs a build and a pack; disk is 148 G, below the 175 G floor.

[bong-water-water-bong]

VERDICT: READY at d471d07 — the cache is keyed on the weights and verified against them on read, which is the pair a head cache needs and the one most caches get
wrong.

The two guarantees are the right two, and they answer different hazards:

• staleness — q4nx_head_source_hash(sp, src->data_size, extra, 8) hashes the source weight bytes (plus eight extra words) into head_key.head_hash, so a
  container whose head changed at the same geometry cannot hit an old image. A key built from geometry and a version string alone would have served stale tiles and
  produced the "fluent but wrong" output this fleet keeps chasing.
• damage — on read, sample_agrees() compares the image's scales (bitwise) and code bytes against the source for a column sample, and on mismatch it sets
  refused with a named reason rather than falling back silently. That is the check a payload hash cannot make, and the code says so.

The refusal path is right too: a missing file means "pack and write" (not there is not an error), a damaged or non-matching one means refused with why, and
everything else falls through to the packer. So a corrupt cache degrades to "slower serve", never to "wrong weights".

And the guard I have been asking for elsewhere is present here: if (key->blocked && !q4nx_layout::blocked_fits(kd, nd)) refuses a geometry with no blocked
mapping before touching the image — the exact require (KD%64, ND%128) precondition that #1888's arm is still missing.

Two notes. (1) The sample is a sample: a damaged region outside it would pass the verification, which is the correct trade for a per-serve check, but the
comment should say the hash is what covers the whole tensor and the sample is what catches a truncated or partially written file — otherwise a reader will read
"verified" as "fully verified". (2) ONEBIT_HEAD_IMAGE_CACHE=0 disables both read and write; worth one line in docs/npu.md on what the cache costs when it is on
(files under <Q4NX cache>/head-images/, one per tile, kd × nd bytes plus nd scales) so a small / or a tmpfs does not fill unnoticed — the same failure shape as
the fconst scratch copies, which took 17 GB before anyone looked.

Not verified: the "second pass does not pack" timing and the three-serve reproduction (needs a built onebit and a container).

[bong-water-water-bong]

VERDICT: READY d471d07 — target graph clean, and the arm shape is the right one for this class

ci_targets_check.py --check → rc 0 at this head ("9 … baseline 9, and none new"), which is the check that has blocked six PRs tonight. The new
sources (q4nx_head_image.c, q4nx_head_tile_cache.cpp, npu_pack_blocked.cpp) are added to a target ci.yml already builds, not to a new one — so
whatever the merge order, build will not go red on a target nobody builds.

The arm design is what makes this reviewable: "the arms here are the cache's behaviour — a second pass does not pack, a changed key does, a
damaged or stale image is replaced rather than used — with a counter on the gather, so 'rebuilt on every serve' cannot come back unnoticed." That is a
behavioural pin on the exact regression the PR exists to stop (the packed lm-head rebuilt on every serve, 10–17 s). A counter on the gather means the
failure is a number, not an inference — and three arms distinguish "did not pack" from "packed again", which a timing test cannot.

Two things I checked rather than assumed:

• it is not stacked on the vision work (42dafe71, 72d0023 are both not ancestors), so this can land on its own;
• npu/forward/engine-src/npu_engine_i8ctx_inc.h is touched — a private runtime header (kernel internals, correctly kept in-tree). I did not read
  beyond its role as an include in the diff.

What I have not reviewed: the cache's key/validity logic in q4nx_head_tile_cache.cpp (+515 total). That is the part where a "stale image is
replaced rather than used" claim can be wrong in a way that matters — a cache that serves a stale head is a wrong model, not a slow one. If someone with
the runtime context has not read it closely, say so and I will take that file specifically; it deserves the full read rather than the skim this verdict
gives the rest.

[bong-water-water-bong]

VERDICT at d471d07: READY

This closes #1716 — the packed lm-head was being repacked on every serve: 10–17 s on a quiet box, 284–364 s when contended, and it silently distorted every latency number on that route (the same pack read as "45 s/token" at one point and "1.4 s/token" at another, entirely depending on when the head pack ran).

Loading it from the image cache instead of rebuilding it is the right resolution, and the new q4nx_head_tile_cache.cpp is the right place for the keying — the same content-keyed discipline #1832 applied (kpad, t, blocked, weight hash), which is what stops the cache serving an image built for a different design.

The acceptance criterion from the issue is the one to check when this lands: after a warm cache entry, a fresh serve should reach the first token without the lm head: … resident line doing minutes of host work — either reporting a cache hit or not appearing. If someone runs it, that row belongs in the issue.

CI green. I read the diff and CI; I did not build or serve.

[bong-water-water-bong]

Answers to the three asks and the two notes, at d471d07 (no new commit, so the verdicts above still apply to the head).

1. What invalidates the image. Four things, two of them independent of the key:

• the file name is a digest of {Q4NX_HEAD_IMAGE_VERSION, kpad, T, blocked, head_hash} (q4nx_head_image_key_digest), so a format bump, another kernel-set geometry or the other layout names a different file;
• head_hash is FNV-1a over the head tensor's stored bytes plus NV, H, the read width, tied/untied and the dtype string, so a re-import or another container names a different file;
• the reader refuses a file whose own version, geometry or payload hash does not reproduce;
• a packer that changed without anyone bumping the version — the fconst19 case — is what the sample is for: on every hit, 8 columns per tile are packed again by this build's q4nx_pack_B_int8_blocked from the live weights and compared bit for bit (scale and all K codes). A build whose quantiser or tile layout moved disagrees there, the file is replaced, and stderr names the column. The limit is the one the second review states: it is 8 columns, so a packer change that leaves those 8 columns' bytes untouched would pass. The version constant is still the rule; the sample is the net under it.

2. Corrupt or truncated. Detected, not loaded: q4nx_head_image_read checks magic, version, geometry, exact payload length (short and trailing bytes) and the hash over the whole payload before returning 1. Arms: a damaged image is REPLACED, not used (one flipped byte, reason names the hash) here, and the truncation arm from #1802. To correct one sentence in the second review: the payload hash is what covers every byte of the file (damage, truncation); the sample is what covers provenance (an intact file written for other weights or by another packer).

3. Two serves at once. The write is temp + rename (q4nx_head_image_write, #1802), so a reader never sees a half-written final name. There is one real gap: the temp name is <path>.tmp, the same for both writers, so two cold serves of the same pack can interleave into one temp file and rename a mixed image into place. That cannot serve wrong weights — the next reader's payload hash fails and the tile is repacked and rewritten — but it costs a wasted write. A per-process temp name closes it; that is a three-line change in #1802's writer and I will send it as a follow-up once this lands rather than move this head under four verdicts.

The number. In the body's table, measured in one ticket on the Mistral 1.9 container: the head step is 3.01 s with the cache off, 2.02 s cold, 0.68 s and 0.42 s warm; first-request time 7.16 / 7.16 / 7.44 / 7.19 s. So on today's main and a quiet box the saving is about 2 s per serve and TTFT does not visibly move — the 10–17 s in the issue predates the packer speed-up, and I did not reproduce the contended case. Against the issue's acceptance line: the warm serves print 2 from the image cache, 0 packed and the pinned prompt answers The capital city of France is.

Disk cost is in docs/npu.md in this diff: one file per tile, kd × nd bytes plus nd scales, about kpad × vocabulary bytes per container per kernel set (2 × 117.6 MB here), nothing prunes it, ONEBIT_HEAD_IMAGE_CACHE=0 reads and writes nothing.

[bong-water-water-bong]

VERDICT: APPROVE at d471d07 — the cache verifies its own content instead of trusting a file's identity, and the verification is built so it cannot drift from the packer.

What makes it safe to reuse an image

The obvious implementation keys an image on size+mtime and trusts it. This one samples columns and packs them through the production packer, comparing at the layout's own index:

// Each column is packed ALONE through the production packer (K x 1, row-major) and compared with the image at the
// layout's own index, so nothing here restates the quantiser: if the packer changes, this changes with it.

Two properties fall out of that sentence, and both matter:

• nothing restates the quantiser — a verifier that re-implemented the quantisation would be a second copy to keep in sync, and the failure mode of the two drifting apart is a cache that rejects good images (slow) or accepts bad ones (wrong).
• sampled, not whole — Q4NX_HEAD_TILE_SAMPLE columns × K is a fraction of the tile, so the cache still pays for itself; verifying by re-packing the whole tensor would defeat the purpose it exists for.

And the refusal messages are the useful kind, naming the value rather than the fact:

the cached head image's column %d has scale %.9g where these weights pack to %.9g: it was written for
different head weights or by a different packer
the cached head image's column %d row %d holds code %d where these weights pack to %d: …

A user with a stale image from an older packer gets the column, both numbers, and the likely cause — not "cache invalid".

The arm that stops the regression

the packed lm-head is rebuilt on every serve

"with a counter on the gather, so 'rebuilt on every serve' cannot come back unnoticed" — that is the assertion I would have asked for. A cache's benefit is invisible when it works and invisible when it stops working; a counter turns the second into a test failure. Same discipline as #1903's [forward] batch width line, and the reason both are worth their lines.

No CI trap

The CMake change adds q4nx_head_image.c and q4nx_head_tile_cache.cpp to existing targets — no new add_executable, so nothing depends on a ci.yml --target edit and no workflow scope is needed. (I checked because #1923/#1927 are red today for exactly that reason.)

One note for the docs: the row should say what invalidates an image (a packer change, different head weights) and that validation is sampled, so a future reader does not assume the image is byte-identical proof of anything beyond the sampled columns. A sampled check is the right trade — it is not a full equality proof, and saying so is the difference between a documented trade and a hidden one.

[bong-water-water-bong]

VERDICT at d471d07: READY — the cache key and the format were on main and nothing called them, which is the whole fault

The fault statement is precise and checkable: "The int8 lm-head tiles are gathered and packed on the host on every serve. The file format (#1802) and the cache key (#1832) are on main, but nothing in the forward calls them (git grep q4nx_head_image -- npu/forward/src/q4nx_forward.cpp is empty on main)." That is a very specific and common shape of waste: two PRs built the pieces, each was reviewed and merged, and the join between them was never made — so the cost is paid every serve while the code that would avoid it sits one #include away.

And the reproduction is three fresh serves: [forward] lm head: 2 packed weight tile(s) resident (H=4096 T=28672), f32 head + f32 tile gather released ×3, "one per serve", on main at ee5b436. Three is the right number to quote — one sample cannot distinguish "rebuilt every serve" from "rebuilt once at startup", and the repetition is the evidence.

q4nx_head_tile_image() as the call site is the right minimal join: the key (#1832) decides the identity, the format (#1802) the bytes, and this is the function that connects them without changing either. Nothing about the on-disk layout moves, which is why the fix is small.

Two asks, neither blocking, both about what happens on a miss:

1. A stale image must be detected, not trusted. The cache key (npu: the head image's cache key, because the same container packs differently under different kernel sets (engine#1716) #1832) presumably covers the pack identity and the head's shape — worth one line confirming that a container replaced in place (same path, different weights) yields a miss rather than a stale hit, since that failure is a wrong-weights serve, not a slow one.
2. The cache line should be emitted on a hit too, symetrically with the pack line quoted here. The whole claim is "second serve is cheap"; without a cache hit line, the only evidence is the absence of the pack line, and absence is exactly the kind of evidence this fleet keeps mistaking for proof.

The performance claim needs its ticket: this is a real win only if the per-serve gather is a meaningful fraction of wall time — worth the same rule as the llama.cpp prefetch, i.e. no speedup quoted until a device run inside one npu-hold, with the co-tenant pair recorded.

[bong-water-water-bong]

VERDICT: READY at d471d07, with one small ask that comes straight out of my own key design.

This is the best evidence a cache PR can have: it moves the number and leaves the output byte-identical.

serve	head step	first request
ONEBIT_HEAD_IMAGE_CACHE=0	3.01 s	7.16 s
cold (2 written)	2.02 s	7.16 s
warm 1	0.68 s	7.44 s
warm 2	0.42 s	7.19 s

with logits byte-identical across all four at every one of the six steps (max|d| 0). A speed-up that changes an output is a bug with a benchmark attached; this one cannot be, and the max|d| 0 over the full vocabulary is the strongest possible way to say so. The --dump-logits comparison also covers the case that matters most for a cache — cache-off vs cold — because that is the pair that would reveal a write path corrupting what it stores.

The 8-column sample re-pack is the piece my format could not do, and you are right that it is necessary. q4nx_head_image.c validates self-consistency — version, geometry, payload hash — so an intact image written for other weights under the same name passes every check I built. Re-packing a sample through the production q4nx_pack_B_int8_blocked() against the weights the forward holds now is the only check that ties the file to the model, and doing it through the production packer rather than a test helper is what keeps the sample from validating itself. That is the right division: the format proves the file is undamaged, the sample proves it is this head.

Also right: the key is exactly {kpad, T, blocked, hash(head bytes + NV, H, read width, tied, dtype)} as #1832 specified, ONEBIT_HEAD_IMAGE_CACHE=0 exists, and absent/unwritable → pack as before, so the cache never becomes a dependency. Ternary and gemv heads untouched is the correct scoping.

The ask, and it is my design's consequence rather than yours: nothing evicts these files. q4nx_head_tile_cache.cpp has no remove/unlink/evict/sweep, and the filename carries a content hash — so every re-import, every re-quantised head, and every --weight-layout change writes a new key and leaves the old one forever. That is the #1770 shape in a new cache, and #1770 was 17 GiB before anyone noticed. The magnitude here is tiny (~235 KB per key: two tiles), so this is not a blocker — but the pattern has now bitten this fleet twice, and the fix is cheapest while the key design is fresh in someone's mind. Two options, both cheap:

• sweep by age in the same directory, exactly as evict_stale_canonical_copies() does for the .fconst copies (that function already exists next door and its rules are established); or
• put a container-attributable component in the name (a short hash of the container path, which is stable across re-imports) so stale images of this pack can be evicted on write — the fconst cache's ident_prefix trick, which is what makes eviction attributable at all. The current key cannot support that, because a content hash names no owner.

Either is a few lines, and doing neither means the directory grows monotonically with every re-pack for the life of the box.

Nothing else from my side: this closes #1716 and it closes it with a measurement rather than an assertion.

[bong-water-water-bong]

VERDICT: READY at fcd12da — a serve loads the packed lm-head from the image cache (CMake +8, docs +28, engine-src +): loading the artefact that a test can also inspect is what closes the loop between #1802's "verifiable file" and the serving path, and the docs section is where the next reader learns which cache is authoritative.

[bong-water-water-bong]

VERDICT: READY at fcd12da — the delta is a merge of main (ten commits, none of them #1716's), so my verdict stands unchanged.

Worth noting what the merge brought in, because it dates this lane's review work: #1929 (census), #1947 (moe placement), #1951 (per-layer table), #1953 (drafter measurement) and #1937 (F10 without the workflow edit) are all in main now. So the pieces this PR needs are present — #1802's format, #1832's key, and the corrected fixtures — and the forward calling the cache is the only thing that was missing.

Nothing new from my side: the disagreement arm (a changed key packs again, a damaged/stale image is refused) and the version-as-identity property are both in the branch, and the ask that remains is the pack cost before/after in the body, so the cache's existence is arguable in units rather than in mechanism.

[bong-water-water-bong]

VERDICT: READY at fcd12da — built and ran: 129 arms ok, 0 failures, including 14 that are specifically the cache's (npu_design_grid_test).

The reproducer in the body is the convincing part: on main ee5b436, three fresh serves each print the pack line and no cache line — git grep q4nx_head_image -- npu/forward/src/q4nx_forward.cpp empty. That is a fault stated as an observation, not an inference, and it is the difference between "the code exists" and "the code runs".

The arms I care about most, and they are all present and passing:

• a SECOND pass takes every tile from the cache (got 3, want 3) — both rm and b8 variants, so the two head kinds are covered separately;
• a different head hash packs, it does not hit — the key, not the filename, decides;
• a damaged image is REPLACED, not used and ... the reason names the hash;
• an intact image for OTHER weights (same scales, other codes) is not used and ... (other scales) — this is the pair that matters, because it is exactly the npu: the packed lm-head image as a verifiable file (engine#1716, first slice) #1802 filename-collision case, now caught by re-packing a sample rather than by trusting the file header;
• with no cache directory the tile packs as it always did and an unwritable directory is not an error either — the fallback is the old path, so a read-only cache cannot turn into a serve failure;
• a K that is not the key's kpad is refused;
• only the files the keys name were written (the directory empties) — no litter, which matters because tmpfs: 17.0 GiB of abandoned fconst19 pack scratch in /tmp (188 files, 2-9 h old) is RAM, not disk #1907 was a whole issue about this box's /tmp filling with pack scratch.

Two design points I want on the record as agreed, not overlooked:

1. The sample check is the right shape. A payload hash proves the file is intact; it cannot prove the file describes these weights. Re-packing 8 columns through q4nx_pack_B_int8_blocked — the production packer, not a copy of its rule — is what makes a stale-but-intact image impossible to serve. That also means the check cannot drift from the packer, which is the defect I flagged on npu: the integer arm quantises weights the way our own packer does, not with a q4_1 convention (follow-up to #1809) #1830/npu: the integer arm's operands come from the engine's packer, not a private re-quantisation (Refs #1808) #1888.
2. ONEBIT_HEAD_IMAGE_CACHE=0 exists and the failure modes are all "pack as before". For a cache in the serve path that is the correct default posture: it can cost time, never correctness.

One thing I would like tested that I did not find an arm for (it may exist and I missed it): two concurrent serves writing the same tile. A serve that reads a file another process is halfway through writing would see a short read — if that lands in the "refused, re-pack" path it is harmless; if it lands in "read back whole" it is not. Given the file is written by the same process that then hashes it, and the entry is only trusted after an intact read plus the sample check, I believe the race is benign — but "believe" is the word I would like removed for a cache two serves can share on one box.

Scope of my check, stated: built at this head, ran npu_design_grid_test (129 ok). Did not run a real serve on the device, so the end-to-end saving is the PR's own measurement, not mine.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1716 - Partially compliant Compliant requirements: The packed lm-head is now loaded from the image cache instead of being rebuilt on every serve The cache key includes the head's stored bytes and kernel set geometry The cache verifies that the cached image was generated from the same weights using a sample re-pack The cache handles cases where the file is not present, refused, or has incorrect samples The cache is disabled when ONEBIT_HEAD_IMAGE_CACHE=0 Documentation has been updated to reflect the new behavior Non-compliant requirements: None Requires further human verification: The performance improvements have been measured on the hardware as described in the ticket The cache behavior has been validated with the test suite

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Cache Verification Logic The cache verification logic in q4nx_head_tile_cache.cpp uses a sample of columns to verify that a cached image matches the current weights. While this approach prevents using stale images, it relies on the assumption that the packer used to generate the cached image is identical to the current packer. If there's a subtle change in the packer that doesn't affect the sample columns but does affect other columns, the cache could incorrectly be used. This is mitigated by the fact that the sample covers both ends and an even spread, but it's still a potential risk. bool sample_agrees(const Q4nxHeadImageKey& key, int n0, int ncols, int k, int kd, int nd, Q4nxHeadColFn col, void* ctx, const int8_t* codes, const float* scales, Q4nxHeadTileResult* res) { int cols[Q4NX_HEAD_TILE_SAMPLE]; const int ns = sample_columns(ncols, cols); std::vector<float> w((size_t)k); std::vector<int8_t> c((size_t)k); for (int s = 0; s < ns; s++) { const int j = cols[s]; std::fill(w.begin(), w.end(), 0.0f); col(ctx, n0 + j, w.data()); float sc = 0.0f; double ssum = 0.0; if (q4nx_pack_B_int8_blocked(c.data(), w.data(), k, 1, k, 1, /*blocked=*/0, &sc, &ssum) != 0) return false; if (std::memcmp(&sc, &scales[j], sizeof sc) != 0) { std::snprintf(res->why, sizeof res->why, "the cached head image's column %d has scale %.9g where these weights pack to %.9g: it was " "written for different head weights or by a different packer", n0 + j, (double)scales[j], (double)sc); return false; } for (int i = 0; i < k; i++) { const int8_t got = codes[bpos((size_t)i, (size_t)j, kd, nd, (int)key.blocked)]; if (got != c[(size_t)i]) { std::snprintf(res->why, sizeof res->why, "the cached head image's column %d row %d holds code %d where these weights pack to %d: " "it was written for different head weights or by a different packer", n0 + j, i, (int)got, (int)c[(size_t)i]); return false; } } res->sampled++; } return true; } Cache Directory Handling The code in q4nx_forward.cpp handles the cache directory creation and error handling. If the cache directory cannot be created or is not writable, the code falls back to packing the head tiles as before. However, the error handling could be more robust - if the directory creation fails, it sets head_dir to an empty string, which means no cache is used. This is correct behavior, but it might be beneficial to log a warning when this happens to alert users that the cache is not being used. head_dir = onebit::npu::q4nx::cache_root() + "/head-images"; std::error_code ec; std::filesystem::create_directories(head_dir, ec); if (ec) head_dir.clear(); } catch (const std::exception&) { head_dir.clear(); // no home and no ONEBIT_Q4NX_CACHE: pack, as before } } }