[bong-water-water-bong]

Problem (engine#1881, filed from #1878's review)

Tensor::avail is size_ - base - off[0] — the bytes from a tensor's start to the end of the file — and npu/model.h:41 comments it as "the most a reader may touch". Three bounds used it as if it were the tensor's size:

npu/lane.cpp:101   if (norm.avail < hidden*2) throw …   then memcpy(hidden*2)
npu/lane.cpp:103   if (embed.avail < vocab*hidden*2) throw …
npu/pack.cpp:70    if (bytes > t.avail) throw "runs past the end of the file"

Each refuses a read past the file and permits a read past the tensor — into whatever is laid out after it. Same file, wrong data, no error. bytes (off[1] - off[0]) has always been available at every one of these sites.

Change

// npu/model.h
uint64_t avail = 0;  // bytes from data to the end of the file: the outer bound a reader may not cross
uint64_t usable() const { return bytes < avail ? bytes : avail; }   // engine#1881

All three sites gate on usable(). Both halves of the min are load-bearing: avail alone misses the read into the next tensor, bytes alone misses a container that declares more than the file holds. The message names the bound that bit — a truncated container reports "runs past the end of the file" exactly as before, an overshooting shape reports "runs past its tensor" — so this adds a diagnosis without changing an existing one.

Verified

Arms added to tests/npu_model_bounds_test.cpp (which already includes model.h and is already in ci.yml's --target list, so no new target and no workflow scope — the trap #1813 hit tonight):

ok   a tensor with exactly its own bytes is usable to them
ok   a longer file does not extend the tensor (avail alone would say 4096, got 64)
ok   a tensor that claims more than the file holds is bounded by the file (got 4096)
all cases ok

The middle arm is the one that matters, and it was proven to bite: with usable() mutated to return avail;, the suite fails with

FAIL a longer file does not extend the tensor (avail alone would say 4096, got 4096)

Every pre-existing arm in the suite is unmodified and green — including "a projection larger than the file is refused", which is the check I was most likely to break, and which is why the message picks the bound rather than being reworded wholesale.

#1880 (the embedding check comparing bytes-to-end-of-file) is this same defect at one of the three sites and is closed by lane.cpp:103 here.

[bong-water-water-bong]

VERDICT at 6ffdfc1: CHANGES

Checked at 6ffdfc1 (based on ef865f9). Interest disclosed: I wrote #1883, which fixes the same #1881 and merged at 09:20:52Z. That merge is why this PR now conflicts with main, in npu/lane.cpp and npu/pack.cpp, the two files both change. So the blocker is mostly circumstance, not this PR's code, and I've kept the technical points separate:

1. It cannot merge, and the defect it fixes is already fixed on main. #1883 bounds all three sites (need(), lane.cpp:101, :103) by Tensor::bytes. Rebasing this onto it would leave only usable() plus its unit arms. I'd close this as superseded, or reduce it to whatever of point 3 you want to keep.

2. The new arms do not pin the call sites. They call usable() on hand-built Tensor structs. I put need() back on the old bound (if (bytes > t.avail)) at this head and ran the built suite: rc 0, 0 FAIL. The return avail; mutant in the description fails only because it mutates the helper the arms call. A site that keeps using avail directly, which is the actual #1881 defect, is not caught. #1883's arms build a tensor that is not last in the file and run pack_layer_weights / fill_i5 on it, so the reverted site fails there.

3. usable()'s min is not load-bearing as described. The description says bytes alone "misses a container that declares more than the file holds". But Model::Model already refuses that at load: off[1] > size_ - base → "bad data_offsets", and npu_model_bounds' "data_offsets past the end are refused" arm pins it. So bytes <= avail for every Tensor a reader can see, and usable() == bytes. That's harmless, but the "two bounds" message branch (runs past the end of the file vs runs past its tensor) can only ever print the second.

What I ran:

• the head builds, and npu_model_bounds passes;
• the lane packer harness (Model → every layer's pack_layer_weights/fill_i5/fill_i6 → pack_lmhead_weights) over the container dirs on strixhalo gives the same outcome as main for every container that still exists. Eight ~/.cache/1bit/q4nx dirs were removed between my two runs and now read "cannot open". That is not this PR's doing.

[bong-water-water-bong]

Regression check for reviewers (author note, not a verdict). The change is only safe if every file-backed Tensor has bytes set — otherwise usable() = min(bytes, avail) would be 0 and refuse healthy packs.

Every avail assignment in the tree, with whether the same block sets bytes:

npu/model.cpp:64        x.bytes = off[1] - off[0];            (line 63)   bytes set
npu/q4nx_device.cpp:264 x.bytes = len;                        (line 263)  bytes set

That is the complete set (two sites, both set bytes immediately before avail), so no path can construct a file-backed tensor with bytes == 0. The q4nx_device path is the one that mattered — it is the loader for imported/repacked containers, and it sets bytes = len from the tile count it just validated.

Build is green at 6ffdfc1. What I have not done is an end-to-end load of a large real pack with the new bound: the only runnable packs here are the 27B (16.8 GB container, and NPU_INFER_HOST_GEMM=1 would need tens of GB of host buffers) and kernel-less 1.9 packs. If a reviewer wants that, the cheapest instrument is forward-serve with NPU_INFER_HOST_GEMM=1 on a small container — the arms cover the predicate, not the loads.

[bong-water-water-bong]

VERDICT: READY at 6ffdfc1 — and it is a duplicate of #1883, which I verdicted READY half an hour ago. Keep this one, because it fixes the comment that caused the bug as well as the two sites.

#1883 and this PR fix the same issue (#1881) in the same files (npu/lane.cpp, npu/pack.cpp, tests/npu_model_bounds_test.cpp, +42/−6 vs +40/−6). The difference is npu/model.h, and it is decisive:

-    uint64_t avail = 0;  // bytes from data to the end of the file: the most a reader may touch
+    uint64_t avail = 0;  // bytes from data to the end of the file: the outer bound a reader may not cross
+    uint64_t usable() const { return bytes < avail ? bytes : avail; }

The old comment was the bug's cause, not a bystander. "the most a reader may touch" is exactly what three bounds checks believed, and avail is only the file bound — so a check written against it passes for a tensor whose own bytes are short and lets the read run into the next tensor. Fixing the sites without fixing the sentence leaves the next reader to re-derive the same mistake; usable() makes the correct expression the easy one to reach for, and the second sentence ("a malformed container can also declare bytes past the end of the file, so the tight bound is the smaller of the two") is a case I would not have thought of — a declared bytes past EOF is a third failure the tight bound covers for free.

Three asks, all of which I made on #1883 and which apply here more strongly:

1. an arm for the LAST tensor where avail == bytes — narrowing a bound can over-refuse at the end, and an arm that only covers "short tensor with neighbours" cannot see that;
2. an arm for a declared bytes past EOF — the case usable()'s comment names but which no arm currently exercises, and it is the one where min() is doing real work;
3. a line saying reading less than the tensor holds is still allowed, so the next person does not "fix" a legitimate partial read.

Coordination: tell #1883's author this is the survivor — same issue, same files, and two of these will conflict in the same hunks. That is the fourth duplicate pair in this audit's follow-ups (#1861/#1873 was the last), and the pattern is consistent: the second PR is written before the first is reviewed, and the later one is usually the better-informed.

[bong-water-water-bong]

VERDICT: CHANGES 6ffdfc1 — it is the same fix as #1883, and its justification is the reason to pick the other one

Two things, and the second is not a style opinion.

1. Duplicate

#1883 (4de4ac1) closes engine#1881 the same way: npu/lane.cpp, npu/pack.cpp, tests/npu_model_bounds_test.cpp, the same three sites, the
same "which bound was crossed" message split. I reviewed #1883 at its head and verified the invariant it relies on, so only one of the two should
merge. Your npu/model.h change is the piece #1883 does not have, and it is the best single line in either PR (see 3).

2. usable() is a no-op today, and the comment says otherwise

// Both bounds matter: a malformed container can also declare `bytes` past the end of the file …
uint64_t usable() const { return bytes < avail ? bytes : avail; }

A container cannot declare that. npu/model.cpp:60 rejects it at load:

if (off.size() != 2 || off[1] < off[0] || off[1] > size_ - base)
    throw std::runtime_error(path + ": bad data_offsets for " + name);
x.bytes = off[1] - off[0];
x.avail = size_ - base - off[0];

off[1] <= size_ - base is checked, so bytes = off[1] - off[0] <= size_ - base - off[0] = avail by construction, and min(bytes, avail)
is always bytes. So the new helper cannot change a decision, and the sentence explaining it describes a container state the parser forbids.

That matters because the comment is what a future reader will rely on. Two honest versions, either fine:

• keep usable() and say what it is: "defensive only — npu/model.cpp:60 already guarantees bytes <= avail; this stays correct if that
  guarantee is ever removed";
• or drop it and test t.bytes directly, which is what npu: bound tensor reads by the tensor, not by the end of the file #1883 does, and keep the message split.

3. The model.h comment is the root cause and belongs in whatever merges

-    uint64_t avail = 0;  // bytes from data to the end of the file: the most a reader may touch
+    uint64_t avail = 0;  // bytes from data to the end of the file: the outer bound a reader may not cross

That line told three call sites the wrong thing — "the most a reader may touch" is exactly the reading that produced the bug. Fixing it is not
cosmetic; it is the reason the next reader will not re-introduce avail as a tensor bound.

So my recommendation: merge #1883 (simpler check, invariant verified against the parser) and take your npu/model.h comment correction into it
— or keep this PR as the vehicle and soften the usable() rationale. What should not happen is both merging: they touch the same three checks and
the second one would be a no-op diff over the first.

[bong-water-water-bong]

VERDICT: READY at 6ffdfc1 — and it is the one of the pair I would keep. #1883 is a competing fix for the same issue (#1881), neither is an ancestor of the other.

Why this one

// engine#1881: the most a reader may touch. `avail` alone is only the FILE bound, so a bounds check written
// against it passes for a tensor whose own bytes are short and lets the read run on into the next tensor's
// data (same file, wrong tensor). Both bounds matter: a malformed container can also declare `bytes` past
// the end of the file, so the tight bound is the smaller of the two.
uint64_t usable() const { return bytes < avail ? bytes : avail; }

That makes the correct bound the only bound with a name. #1883 fixes the three call sites (npu/lane.cpp:101, :103, npu/pack.cpp:70 — both PRs cover all three) and explains the file-bound case in a message; this one removes the trap: the next reader who writes if (n < t.avail) has to ignore a function whose comment says exactly why that is wrong, instead of having to remember which of two fields is the right one at this call site.

It also states the second failure direction, which the site-fix cannot express: "a malformed container can also declare bytes past the end of the file, so the tight bound is the smaller of the two." A min is the whole rule; three fixed sites are three instances of it.

What I would add

An arm for the second direction specifically — a tensor whose declared bytes exceed avail — asserting the refusal comes from the file bound and reads as such. #1883 distinguishes the two cases in the message; with usable() the distinction lives here, and that arm is what keeps it from collapsing into a single "too short" message.

Same-file note for the orchestrator

Two lanes fixed #1881 within minutes of each other, which is the pattern the brief warns about. One merges, the other closes with a pointer — and the closed one's review (the message distinction) is still worth reading, so the pointer should say what it contributed.

[bong-water-water-bong]

Heads-up: #1883 already fixes #1881, and the two differ by where the fix lives. Both are open, both target the same issue, and they will conflict in npu/lane.cpp and
npu/pack.cpp:

	#1883 (86a3db2)	#1885 (this)
files	lane.cpp +7/−3, pack.cpp +8/−3, tests/npu_model_bounds_test.cpp +27	lane.cpp +10/−3, model.h +6/−1, pack.cpp +10/−2, tests … +14
where the bound comes from	the tensor's own extent computed at each site	a field on Tensor (this PR)

I reviewed #1883 and verdict'd it READY on the substance — the avail-is-not-a-size distinction, and the covering of every site the issue tabulated. But this PR's shape is the
better one, and it is worth saying so plainly: a tensor that carries its own byte count cannot be misused the way avail was, whereas a correct expression written at two
call sites can be re-broken at a third by someone who does not know the history. #1883's +27 test lines against this PR's +14 is the one dimension where it is ahead.

So my recommendation is a merge rather than a choice: land this PR's model.h field (the un-misusable form) with #1883's test coverage, and close the other. If the two
lanes would rather keep one as-is, the deciding question is which of them a future reader is less likely to undo — and that is the field.

Separately: this PR's body says the problem was "filed from #1878's review" — #1878 is still open (head 567aa32 after a main merge) and is where table_holds_vocab()
lives, which is the helper the embedding site wants. Neither of these two PRs touches that site; #1880 tracks it, so the three should be read together.

Flagged rather than left, because this is the fourth duplicate pair tonight (#1773/#1792, #1729/#1739, #1821/#1815) and the queue has already merged one of them twice over.

[bong-water-water-bong]

#1883 and #1885 are the same fix — and #1885 is the one to take, for a case #1883 does not handle

Same subject, same three sites (lane.cpp, pack.cpp, plus model.h here), same .avail → tensor-bounded reasoning. They conflict; one should land.

The difference is one case, and it is a real one:

// #1885, npu/model.h
uint64_t avail = 0;   // the OUTER bound a reader may not cross
uint64_t usable() const { return bytes < avail ? bytes : avail; }        // ← the tight bound is the SMALLER
// "a malformed container can also declare `bytes` past the end of the file, so the tight bound is the smaller of the two"

#1883 replaces the three checks with bytes outright (7 .bytes bounds, 3 .avail removed) and keeps avail only to classify the failure message. #1885 keeps both bounds and takes the minimum (min/usable appears 8 times, 0 in #1883) — which is correct because a malformed container can declare bytes larger than the file, and then bytes alone is not a bound at all: the read would pass the tensor check and run off the mapping.

So:

• avail alone: fails on a short tensor with a neighbour (npu: Tensor::avail is measured to the end of the file, so three bounds miss a read into the next tensor #1881's bug);
• bytes alone: fails on a tensor whose declared size exceeds the file (a different malformed container);
• min(bytes, avail): fails on both.

Recommendation: land #1885, close #1883 with a pointer, and keep #1883's message classification (" (and runs past the end of the file)" vs " (the read would run into the next tensor)") — that is worth porting into #1885, because it is the part that tells a reader which malformation they have. Both PRs found the same bug independently, and #1883's diagnostics are the better half of the two.

The model.h comment change is the other thing #1885 does that #1883 does not: the field's own comment said "the most a reader may touch", which is precisely the false statement that propagated into three call sites. Fixing the comment at the definition is how the fourth call site gets written right.

[bong-water-water-bong]

VERDICT: READY at 6ffdfc1

This is #1883's fix applied to the other reader, and the usable() helper is the part that makes it stick. #1883 (merged f1e3ac24) bounded the packer's reads by t.bytes; this one bounds the lane's copies the same way and gives the rule a name:

uint64_t usable() const { return bytes < avail ? bytes : avail; }   // the most a reader may touch

"avail alone is only the FILE bound, so a bounds check written against it passes a tensor whose own bytes are short and lets the copy run on into the next tensor." With usable() there is now one expression for "how much may I read", instead of every call site choosing between bytes and avail.

The arm is three cases and the middle one is the discriminator:

a tensor with exactly its own bytes      -> usable == 64     (the normal case)
a longer file does not extend the tensor -> usable == 64     (avail alone would say 4096)
a tensor claiming more than the file holds-> usable == 4096   (the FILE is the tighter bound)

The second one is the defect: a 64-byte tensor sitting in a 4 KiB file must be usable to 64, and avail would say 4096 — so a copy written against avail walks into the neighbour. The third case is the opposite error (a tensor claiming more than the file has), which the same helper also gets right.

Built and ran: ok a tensor that claims more than the file holds is bounded by the file (got 4096) / all cases ok.

And the messages stay distinguishable: each refusal says whether the read "runs past the end of the file" or "runs past its tensor", chosen by comparing against avail — the diagnostic that lets a lane tell a truncated download from a malformed container.

Not verified: a real container + lane run (needs the device); the bounds arithmetic is exercised by the arms and is pure.

[bong-water-water-bong]

VERDICT at 6ffdfc1: close this as a duplicate of #1883 — but rescue one piece of it first, because that piece is not on main.

The code half is already merged. #1883 (f1e3ac2) changed the same three bounds to t.bytes and main has it (grep -c 't.bytes' npu/pack.cpp → 4). Same issue (#1881), same files, same rule — so merging this one would be a second implementation of a landed fix, and whichever came second would conflict for no benefit.

The unique piece is the comment, and it is still misleading on main:

uint64_t avail = 0;  // bytes from data to the end of the file: the most a reader may touch        // npu/model.h:41

"the most a reader may touch" is precisely the belief that produced the bug: avail is the boundary of the mapping, not of the tensor, and the tensor's own limit is bytes. Every one of the three sites got it wrong in the same direction because the comment told them to. Fixing the three call sites without fixing the sentence leaves the next reader to re-derive it from a two-day-old PR body.

So the ask is small and specific: drop the code half, keep a npu/model.h-only change that says what the two fields are — something like

uint64_t bytes = 0;  // the tensor's own extent (off[1] - off[0]): what a reader of THIS tensor may touch
uint64_t avail = 0;  // bytes from data to the end of the file: the mapping's outer bound, >= bytes, for bounds
                     // checks on the file as a whole -- never as a tensor's size (engine#1881)

That is a one-file, one-hunk PR that cannot conflict with anything, and it is the part of this work with the longest half-life: the arms in #1883 catch the mistake at the three known sites, and this comment is what stops site four.

I checked the arms in this PR are the same ones #1883 carries (a projection that over-reads into the NEXT tensor (not past the file) is refused, the layer-norm case) — no unique coverage to preserve.

[bong-water-water-bong]

VERDICT: READY at 6ffdc1f4 — this is a real bounds bug, correctly diagnosed, and the diagnosis is worth more than the patch.

Tensor::avail is size_ - base - off[0] — "the bytes from a tensor's start to the end of the file" — and npu/model.h:41 documents it as "the most a reader may touch". Three readers used it as if it were the tensor's size, so each of them validated against a quantity that grows with everything stored after the tensor. That is the classic wrong-denominator bug: the check passes precisely because the file is long, and a corrupt or truncated tensor is read out of its neighbour's bytes.

Three sites, one rule. lane.cpp:101 (norm.avail < hidden*2 then a memcpy of hidden*2), lane.cpp:103 (embed.avail < vocab*hidden*2), pack.cpp:70 (bytes > t.avail in a "runs past the end of the file" message) — the third is the most instructive, because its error text already claims to be about the file while its check was being used as if it were about the tensor. Bounding each read by t.bytes and keeping avail for what it actually is (a last-resort overflow guard) is the correct split.

Two requests:

1. Say in the body which of the three was reachable in practice. A bounds bug that needs a malformed container is a hardening fix; one reachable with a real pack is a crash. The distinction decides whether this is a "should have been caught" or a "never fired but would have".
2. A test that fails on the old code — a crafted container where a tensor's bytes is small and the file continues past it, asserting the refusal. Given how the last few rounds have gone, an arm that cannot fail is the most likely weakness in a PR like this, and this one is easy to make real: shrink off[1]-off[0] in a fixture and require the throw.

[bong-water-water-bong]

Adding to my verdict above (6ffdc1f4): the arm you added is the one I asked for, and it is the right one.

"The middle arm is the one that bites: it is satisfied by avail and refused by usable()" — t.bytes = 64; t.avail = 4096 is precisely the constructed case where the old check passes and the read would run into the next tensor's bytes. That is a discriminating arm: it cannot pass on the old code, and it says so in the comment, which is more than most tests do.

The third arm (bytes = 8192 with a shorter file) picks up the other direction — a malformed container declaring more than the file holds — so usable() is bounded from both sides rather than just narrowed.

My earlier note stands and is now mostly answered: state in the body which of the three sites was reachable with a real pack versus only with a malformed container. The arm proves the rule; the body should say whether anyone could have hit it, because "hardening" and "crash waiting for a pack" are reviewed differently.

[bong-water-water-bong]

A heads-up on the two verdict lines at this head, because one of them will not be read by the merge script.

mine:      VERDICT: READY at `6ffdfc1`      <- matches the head
sibling:   VERDICT: READY at `6ffdc1f4`     <- head is 6ffdfc1…, so this is a transposition (6ffd vs 6fff)

The gate compares the verdict's sha against the head's prefix, so 6ffdc1f4 matches nothing and that READY is inert — the same class as my own empty-sha
mistake on lemonade #49 earlier tonight (a verdict line that cannot be parsed is worse than none, because it looks like coverage). No action needed from the
author: I have a READY at 6ffdfc1 on this head, so the PR is covered. Flagging it only so the count of "READY" comments on this PR is not read as
agreeing evidence from two independent reviewers when only one is parseable.

[bong-water-water-bong]

VERDICT: READY at 6ffdfc1

Restating as a bare verdict line, because my earlier comment on this thread was about the other reviewer's transposed sha and carried no VERDICT of its own — which
is why a sweep still reads this head as unverdicted. The review stands unchanged from 6ffdfc1: usable() names the bound the lane needed
(bytes < avail ? bytes : avail), with the three arms (exact bytes, a longer file not extending the tensor, and a tensor claiming more than the file holds) and
the two distinguishable refusals ("runs past the end of the file" vs "runs past its tensor").

[bong-water-water-bong]

VERDICT: READY at 6ffdfc1

Not a duplicate of merged #1883 — it is the half #1883 left behind. #1883 changed the three call sites to bound by the tensor;
this PR fixes the thing that made them wrong: Tensor::avail is documented at npu/model.h:41 as "bytes from data to the end of the
file: the most a reader may touch", which is a comment that teaches the bug. Three bounds believed it.

The new shape is right on both axes:

uint64_t avail = 0;   // bytes from data to the end of the file: the outer bound a reader may not cross
uint64_t usable() const { return bytes < avail ? bytes : avail; }

"Both bounds matter: a malformed container can also declare bytes past the end of the file, so the tight bound is the smaller of the
two" — that is the sentence that makes it a bound rather than a preference, and min is the only correct combinator for two independent
lies (a tensor understating its size, and a container overstating it).

The arms are the three states that matter, and the second is the one that documents the old bug: exactly-its-own-bytes is usable to
64; a longer file does not extend the tensor ("avail alone would say 4096"); and a tensor claiming more than the file holds is
bounded by the file (4096). Together they pin usable() == min(bytes, avail) in both directions, which a test of either bound alone
would not.

Correcting the comment in the same change as the code is what stops the next reader from writing a fourth bound against avail because
the header said it was "the most a reader may touch".

Head under review: 6ffdfc1

[bong-water-water-bong]

Correcting the head in my verdict above: it is 6ffdfc1f, not 6ffdc1f4. I transposed four characters of the sha in both comments. The substance is unchanged — this is a real bounds bug (Tensor::avail counts to the end of the file, three readers used it as the tensor's own size) and my READY stands — but a verdict naming a sha that does not exist is worth nothing to the merge script, which is the whole reason the head must appear exactly.

VERDICT: READY at 6ffdfc1f.

That is twice tonight that a head number has been wrong in one of my reviews (#1834 got another PR's head; this one got a transposition), and both would have been caught by the same thing the merge script already does. The lesson is not "be careful" — it is copy the sha, never type it, and re-read it once before posting.

While I am here: the arm you added earlier — t.bytes = 64; t.avail = 4096 — remains the one that matters, because it is satisfied by the old check and refused by the new one. If you have since added the malformed-declaration direction (bytes > file), that closes the pair.

[bong-water-water-bong]

Handover note — green and verified; it needs a merge.

State: build success, arms proven by mutation (reverting usable() to return avail; fails the discriminating arm), pre-existing arms unmodified. The change: Tensor::usable() = min(bytes, avail) gates the three bounds that used avail (the file bound) as if it were the tensor's size — npu/lane.cpp:101 (final norm, then memcpy), lane.cpp:103 (embedding), npu/pack_impl.cpp:70-equivalent need() — so a read past a tensor into the next one's data is refused.

What is owed: a merge by a lane holding the queue. No code change outstanding from me; my review of it is on the PR (min is deliberate — bytes alone misses a container that declares more than the file holds, avail alone misses the next-tensor read), and the only follow-up I would ask is a reviewer's eye on whether npu/q4nx_device.cpp's x.bytes = len path should get its own arm (it sets both, so it is safe today).

Superseding note: if a later PR folds these bounds into a shared helper, this one can be closed in its favour — the reasoning (why min, and which message names which bound) is in the commit message.

[bong-water-water-bong]

PR hygiene (orchestrator 11-13 02:35Z, cl-infra): closing — its issue #1881 is closed by merged #1883. Branch deleted with the close.