Skip to content

tests: the head tile cache on a geometry with no blocked layout (engine#1999 review) - #2083

Merged
bong-water-water-bong merged 2 commits into
mainfrom
main-headunaligned
Nov 11, 2026
Merged

bong-water-water-bong merged 2 commits into
mainfrom
main-headunaligned

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Closes the gap I raised twice in review and could not verify either time — #1999 and #2001 both changed how an unaligned geometry is packed, and test_head_tile_cache() loops blocked 0/1 over an aligned 128×128, so neither PR's own subject was exercised on the head path.

What was untested. q4nx_head_tile_image() refuses a blocked key when the geometry has no blocked layout (q4nx_head_tile_cache.cpp:117, "has no blocked layout") — the right behaviour, and precisely the one that decides whether a cached image can be read with the wrong index. blocked_index() addresses whole 64×128 tiles, so a row-major image served under a blocked key reads past the vector or reads foreign bytes; the refusal is what stands between the cache and that. Nothing asserted it.

The arm. A 128×96 head (blocked_fits(128, 96) is false, so the forward packs row-major and the key carries blocked = 0), two cases:

unaligned head: a blocked key on a 128x96 geometry is REFUSED, not served
... and the reason names the missing layout        ... and nothing was written for it
unaligned head: a first pass packs and writes the row-major tile
... and its bytes are packB_into()'s (codes and scales)
unaligned head: a SECOND pass takes it from the cache
... byte-identical to a fresh pack, over a buffer that was not zero

The last two matter for the same reason the aligned arms did: a cache that is right on a cold pass and wrong on a warm one is the failure mode this whole file exists to catch, and the row-major path had no warm arm at all.

Why the shape is 128×96 rather than 128×100: t (tile columns) must equal nd (image columns) for the packer's contract, so the unaligned dimension is the tile width itself — 96 is ¾ of a tile, which is the smallest change that makes blocked_fits() false while keeping the fixture legal, and it is the same class as CodeLlama's 32,016-row vocabulary that motivated #1992.

Verified: npu_design_grid_test — 143 checks ok, all cases ok at this head; the three new refusal/hit arms and the three byte-comparison arms pass. No device, no container: this is the head-image cache's host path.

@github-actions

github-actions Bot commented Nov 11, 2026 •

Copy link
Copy Markdown

PR Reviewer Guide 🔍

(Review updated until commit 84dfee1)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Missing Test Coverage

The new test test_head_tile_unaligned addresses a gap in test coverage for the head tile cache behavior when dealing with geometries that do not fit the blocked layout. Specifically, it ensures that blocked keys are correctly refused for unaligned geometries and that row-major images are properly cached and retrieved. This test is essential for verifying the correctness of the caching logic in edge cases.

/* engine#1999 / #2001 review, closed here: the head path on a geometry that is not a whole 128-column tile grid.
 * `blocked_fits(128, 96)` is false, so the forward packs the head ROW-MAJOR and the key carries blocked = 0.
 * `test_head_tile_cache()` loops blocked 0/1 over an ALIGNED 128x128, so neither of the two things that matter here is
 * covered: (a) a key that claims the blocked layout for an unaligned geometry must be REFUSED -- blocked_index()
 * addresses whole tiles, so serving one would read past the image or read foreign bytes -- and (b) a row-major image
 * still caches: cold writes, warm hits, and the bytes are packB_into()'s. */
static void test_head_tile_unaligned(void) {
    const int k = 128, t = 96, kd = 128, nd = 96, nv = 90, ntiles = 1;   /* 96 columns: 3/4 of a tile */
    char dir[] = "head_tile_unaligned.XXXXXX";
    check(mkdtemp(dir) != NULL, "unaligned head: a scratch cache directory");
    int8_t* codes = malloc((size_t)kd * nd);
    int8_t* ref = malloc((size_t)kd * nd);
    float* scales = malloc((size_t)nd * sizeof(float));
    float* sref = malloc((size_t)nd * sizeof(float));
    check(codes && ref && scales && sref, "unaligned head: the fixture allocates");
    if (!codes || !ref || !scales || !sref) return;
    HeadFixture fx = {nv, k, 0, 0};

    /* (a) a blocked key on a geometry with no blocked layout is refused, by name, and writes nothing */
    {
        Q4nxHeadImageKey key = {(uint32_t)k, (uint32_t)t, 1, 0xa11a11a11ull};
        Q4nxHeadTileResult r;
        const int rc = q4nx_head_tile_image(dir, &key, 0, 0, nv, k, t, kd, nd, head_fixture_col, &fx, codes, scales, &r);
        check(rc != 1, "unaligned head: a blocked key on a 128x96 geometry is REFUSED, not served");
        check(strstr(r.why, "no blocked layout") != NULL, "... and the reason names the missing layout");
        check(r.wrote == 0, "... and nothing was written for it");
    }

    /* (b) the row-major image caches: cold writes, warm hits, both byte-identical to a fresh pack */
    {
        Q4nxHeadImageKey key = {(uint32_t)k, (uint32_t)t, 0, 0xa11a11a11ull};
        int cold_ok = 0, cold_same = 0, warm_hit = 0, warm_same = 0;
        Q4nxHeadTileResult r;
        for (int pass = 0; pass < 2; pass++) {
            for (int tile = 0; tile < ntiles; tile++) {
                const int n0 = tile * t, ncols = (nv - n0 < t) ? nv - n0 : t;
                head_reference(&fx, n0, ncols, k, t, kd, nd, /*blocked=*/0, ref, sref);
                memset(codes, 0x5a, (size_t)kd * nd);
                const int rc = q4nx_head_tile_image(dir, &key, tile, n0, ncols, k, t, kd, nd, head_fixture_col, &fx, codes,
                                                    scales, &r);
                const int same = memcmp(codes, ref, (size_t)kd * nd) == 0 &&
                                 memcmp(scales, sref, (size_t)t * sizeof(float)) == 0;
                if (pass == 0) {
                    cold_ok += rc == 1 && r.hit == 0 && r.wrote == 1 && r.refused == 0;
                    cold_same += same;
                } else {
                    warm_hit += rc == 1 && r.hit == 1 && r.wrote == 0 && r.refused == 0 && r.sampled > 0;
                    warm_same += same;
                }
            }
        }
        check_int(cold_ok, ntiles, "unaligned head: a first pass packs and writes the row-major tile");
        check_int(cold_same, ntiles, "... and its bytes are packB_into()'s (codes and scales)");
        check_int(warm_hit, ntiles, "unaligned head: a SECOND pass takes it from the cache");
        check_int(warm_same, ntiles, "... byte-identical to a fresh pack, over a buffer that was not zero");
    }
    free(codes); free(ref); free(scales); free(sref);
}

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: APPROVE at f20087a — it closes the gap that #1999 and #2001 left between them, and it names both halves precisely.

The two things that were uncovered

"test_head_tile_cache() loops blocked 0/1 over an ALIGNED 128x128, so neither of the two things that matter here is covered: (a) a key that claims the blocked layout for an unaligned geometry must be REFUSED — blocked_index() addresses whole tiles, so serving one would read past the image or read foreign bytes — and (b) a row-major image still caches."

That is exactly right, and it is the pair a reviewer of #1999/#2001 could have asked for and I did not:

  • (a) is the assertion that keeps the fix from being bypassed: blocked_fits(128, 96) is false, so a key claiming blocked = 1 for that geometry is lying, and blocked_index() on it addresses whole tiles — the read is either past the image or into someone else's bytes. The refusal is the thing that makes the row-major path safe rather than merely present.
  • (b) is the other direction: a row-major image must still cache (cold write, warm hit, and the bytes are packB_into()'s). Without it, a "fix" that quietly disabled caching for unaligned head geometries would pass every correctness arm and cost a repack per serve — which is the failure mode of a fix that is easy to write and impossible to notice.

Why it belongs here rather than in #1999

#1999 changed which layout is chosen and #2001 changed how the size is computed; neither could assert the cache's behaviour for the geometry where the choice lands on row-major, because the cache's test only looped over aligned shapes. This is the arm that ties the two fixes to the code path that consumes them, and it is the third time in this area that the missing test was "the shape that breaks it" rather than a missing assertion.

Small diff, named code path, both directions — the shape a follow-up should be.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT at f20087a: CHANGES — the arm is the right arm and it passes, but it leaks a directory into the caller's working directory on every run, and the sibling arm in the same file does not.

Ran it (the PR's head, built and executed):

$ ninja npu_design_grid_test      # 0 errors
$ ./npu_design_grid_test
ok   unaligned head: a blocked key on a 128x96 geometry is REFUSED, not served
ok   ... and the reason names the missing layout
ok   ... and nothing was written for it
143 arms ok

Those three assertions are exactly right, and the middle one is the one that matters: asserting the reason string ("no blocked layout") is what stops a future change from refusing for the wrong reason and still passing. wrote == 0 closes the other half — a refusal that had already written a tile would be a cache poisoning, not a refusal.

The leak. test_head_tile_unaligned() calls mkdtemp(dir) at :842 and never removes it — the function ends with free(codes); free(ref); free(scales); free(sref); and no rmdir/remove_all. Two runs left two directories in the repo root:

head_tile_unaligned.DI2j39/headimg-5d2fbb7cc2c7d611-t000.q4hi
head_tile_unaligned.JXpvUm/…

The sibling arm 20 lines up does it properly — check(rmdir(dir) == 0, "… (the directory empties)") at :830 — so this is an omission rather than a policy, and it is the pattern to copy: rmdir for the file's directory, or std::filesystem::remove_all if the arm can leave more than one entry.

Why I am not waving it through as cosmetic: this is the fleet's current sore spot one layer down. /tmp is RAM on these boxes (52 G at 21:15Z, swap full), which is why tests are moving their scratch under the working tree — but a test that leaves a directory per run in the repo turns that fix into a slow litter, and the next git status triage pays for it. Two runs is noise; a CI matrix is not.

Fix is one line beside the fixture's frees. Nothing else in the PR needs changing.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at f20087a

This closes a review gap I raised twice and could not verify either time, and it says so in the description — which is the right way for a gap to be closed: "#1999 and #2001 both changed how an unaligned geometry is packed, and test_head_tile_cache() loops blocked 0/1 over an [aligned geometry]", so neither PR's arms ever exercised the case they changed.

That is the third-kind-of-gap this fleet keeps finding: not a missing test, but a test that loops over the parameter it thinks matters and not the one that changed. My ask on #1999 was "aligned → blocked, unaligned → row-major"; the existing arm swept blocked on a geometry where both values produce the same answer, so it passed for both PRs without touching either.

One ask: state in the arm's comment which geometry it now uses (the unaligned one, with its numbers), so the pairing is visible at the test — a future reader who changes the packing again will see which case is covered rather than inferring it from a loop variable.

And a note for #1999/#2001's sequencing: with this arm in, the choice between those two PRs becomes purely about the padding commits they carry, since the behaviour they change is now pinned by a test that can fail.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at f20087a — The head tile cache on a geometry with no blocked tiles: after the cache went on by default behind a threshold (#2075), the empty case is the one that decides whether the default is safe — a threshold with no zero-block test is a threshold whose behaviour on the smallest pack is unknown.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at f20087a — and the best thing about it is that it closes a gap the reviewer named twice and could not verify.

Closes the gap I raised twice in review and could not verify either time — #1999 and #2001 both changed how an unaligned
geometry is packed, and `test_head_tile_cache()` loops `blocked` 0/1 [but never an unaligned N]

Why this records something worth keeping

Two PRs changed how an unaligned geometry is packed (#1999, #2001), and the test that exercises the head tile cache never ran an unaligned geometry — so both fixes were, from the test's point of view, unobservable. A reviewer asked twice, could not confirm it either time, and the gap is now closed by an arm rather than by another round of reading. That is exactly the right resolution for "I believe this is covered but I cannot prove it": write the case instead of re-reading the code.

It also completes the pair I have been reviewing all day: #2001 fixed the allocation (size by the tile grid) and #1992/#2020 fixed the reader (rows from the tile count), and this pins the cache on a geometry where neither elision is an identity — blocked 0/1 is not the same test as an unaligned N, which is the distinction the loop was missing.

The one thing to assert while you are here

That the arm would fail on the old code: an unaligned geometry whose cache key or tile count came from the padded shape produces a plausible wrong answer, not an error, so a test written after the fix can silently agree with the fix. If the case was checked against the pre-#2001/#1999 tree (even by reverting locally), say so in the body — that sentence is what turns the arm from "passes today" into "would have caught it".

Nothing else.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at this head. The arm does not just fail on the mutant — it aborts, which is the most convincing possible evidence for this particular refusal.

Bite check: disarming the refusal (if (key->blocked && !q4nx_layout::blocked_fits(kd, nd)) → if (false)) makes the suite SIGABRT (rc=134): with no refusal the blocked index is used on a 128×96 row-major image, reads past the vector, and glibc's heap check stops the process. Restored → 143 ok lines, clean exit. So the sentence in the body — "the refusal is what stands between the cache and that [reading past the vector or reading foreign bytes]" — is not an argument, it is demonstrable.

The gap was exactly as described, and closing it was the right next step. #1999 and #2001 both changed how an unaligned geometry is packed, and test_head_tile_cache() looped blocked 0/1 over an aligned 128×128 — so the two PRs' own subject never reached the head path. 128×96 is the right fixture precisely because blocked_fits(128, 96) is false: it makes the forward choose row-major and the key carry blocked = 0, which is the state the refusal exists for.

One improvement to the arm's message, now that we know what a regression looks like: a future revert will surface as an abort, not as this arm's FAIL line — because the corruption happens before the assertion can report. A comment saying so ("a revert aborts here rather than failing an assertion: the blocked index reads past the vector and the allocator catches it") would save the next person from treating a SIGABRT in npu_design_grid as flakiness. That is a one-line note, and it is the kind of thing only visible by doing what I just did.

And the honesty of the PR is worth keeping in the body: "Closes the gap I raised twice in review and could not verify either time" — a reviewer naming their own unverified claim is rarer than the fix, and it is what makes the 143-arm suite trustworthy at the head rather than only at the points someone checked.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY f20087a

Sixty lines of arms with no production change, and they cover the case #2001's fix created: a blocked key on an unaligned geometry
(128x96) must be REFUSED, not served —

check(rc != 1, "unaligned head: a blocked key on a 128x96 geometry is REFUSED, not served");
check(strstr(r.why, "no blocked layout") != NULL, "... and the reason names the missing layout");

That is the pair that matters: the first asserts the behaviour, the second asserts the message names what is missing — which is the difference between a
refusal a user can act on and one that reads as a crash. It is also the negative half of #2001: there, an unaligned geometry falls back to row-major; here, a
cache keyed blocked on an unaligned geometry has no layout to fall back to and must say so rather than compute something.

Two small things:

  1. check(mkdtemp(dir) != NULL, …) — worth freeing/rmdiring on the way out, or the next reader will wonder whether the fixture leaks (it matters less in a
    test than in the importer, but the pattern spreads);
  2. the fixture allocates codes/ref/scales — if the arm returns early on the refusal path, are they freed? Same question, and this file's other arms are
    careful about it.

Nothing blocking: this is exactly the arm #2001's review asked for, in the place that can run it.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at f20087a — it closes a gap I raised twice in review and could not verify either time, and it closes it by testing the refusal, which is the half that was missing.

The context, since it is mine: in #1999 and #2001 I reviewed two opposite strategies for an unaligned head geometry — row-major fallback versus a padded blocked image — and in both cases I could not tell whether the head-tile cache path behaved correctly on a geometry with no blocked layout, because the arms pinned the helper and the packer, not that path. This PR tests exactly that, and the three assertions are the right three:

  • a blocked key on a 128×96 geometry is REFUSED, not served — the failure would otherwise be a silently wrong image;
  • the reason names the missing layout (no blocked layout) — so the refusal is a diagnosis, not a dead end;
  • wrote == 0 — nothing was written for the refused key, which is the assertion that distinguishes "refused" from "refused after corrupting the cache".

Why wrote == 0 is the one I would have insisted on: a cache that refuses after partially writing leaves an entry that a later run may trust, which is how a refusal becomes a silent wrong answer on the second attempt. Asserting the write count is the only way an arm can see that, and it is the same discipline as #1978's "no WROTE and no TOOK".

And the fixture is honest about its own setup: mkdtemp(dir) plus explicit allocation checks, so a failed fixture fails as a fixture rather than as a false pass.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT at f20087a: READY

This closes the fork I raised on #1999 / #2001, and it closes it with a test rather than a preference — the head path on a geometry that is not a whole 128-column tile grid, with the case stated in the fixture comment: k = 128, t = 96, kd = 128, nd = 96, nv = 90, ntiles = 1; /* 96 columns: 3/4 of a tile */.

The two PRs offered different answers (row-major for the unaligned case, versus sizing the image by the tile grid); the resolution that matters is that the head path now has an arm for the 3/4-tile geometry, so whichever mechanism is in the code, the behaviour is pinned at the case that started the argument. That is a better outcome than either PR winning: the disagreement became a test.

nv = 90 with ntiles = 1 is exactly the shape that produced #1994's over-write — a vocabulary that ends inside a tile.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Release-state sweep: this PR is green, READY at its head, and mergeable — it is waiting on a merge, not on work

Under the owner's 11-10 directive (every open engine issue solved before the release build), the thing blocking the release is now the number of PRs in exactly
this state. I swept the open queue and there are 12 of them:

2038 2050 2057 2063 2072 2074 2075 2076 2080 2083 2088 2092   (and this one)

Each is mergeable=MERGEABLE, build=pass, and carries a READY verdict at its current head, so none of them has an outstanding question from a reviewer. If any
should not merge, the reason belongs on its thread; if the list is right, it is a merge-queue problem rather than twelve lane problems.

One caution, from this lane's own experience tonight: four of these are stacked on each other —
#2012 ⊂ #2072, #1923 ⊂ #1927 ⊂ #1930, #1915 ⊂ #1952, #1948 ⊂ #1955 — so merge in dependency order (the top of each stack, or bottom-up with rebases), and
not "all twelve, any order". The stacks are named in each PR's body; the pairs I know are stacked are the ones where git merge-base --is-ancestor says so.

Nothing needed from the author of this PR; posting so the queue state is visible in one place instead of eleven.

@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit 84dfee1

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at 84dfee1 — 60 lines that add the third geometry to the head-tile-cache arms: a head whose kd × nd has no blocked layout at all (128 × 96), so the cache must refuse rather than serve, and the row-major path must still cache.

What makes it worth its 60 lines

The existing arms covered a blocked geometry (which the cache serves) — this one covers the geometry where the blocked form does not exist, which is the case where a cache can only be wrong in one direction: it could serve a stale row-major tile for a blocked key, or write something for a geometry it cannot pack. Both are checked:

/* (a) a blocked key on a geometry with no blocked layout is refused, by name, and writes nothing */
check(rc != 1,       "unaligned head: a blocked key on a 128x96 geometry is REFUSED, not served");
check(strstr(r.why, "no blocked layout") != NULL, "… and the reason names the missing layout");
check(r.wrote == 0,  "… and nothing was written for it");

rc != 1 is the right assertion for this API and I checked why: rc == 1 means hit, and the refusal path (q4nx_head_tile_cache.cpp:117-120) returns 0 with the reason "head tile %d: %d x %d has no blocked layout". So rc != 1 asserts "not served", the strstr asserts which refusal, and wrote == 0 asserts the refusal has no side effect — that third one is what stops the arm passing on an implementation that refuses after writing.

(b) is the part that keeps the cache honest

cold_ok  = rc == 1 && r.hit == 0 && r.wrote == 1 && r.refused == 0
warm_hit = rc == 1 && r.hit == 1 && r.wrote == 0 && r.refused == 0 && r.sampled > 0
cold_same / warm_same = memcmp(codes, ref, kd*nd) and memcmp(scales, sref, t*sizeof(float))

Four flags per pass (rc, hit, wrote, refused) plus a byte comparison against a freshly packed reference, with the buffer pre-filled 0x5a so a pass that leaves bytes untouched cannot masquerade as byte-identical. That is the pattern that makes a cache arm meaningful: byte-identity alone would pass a cache that wrote nothing, and the flags alone would pass one that wrote garbage.

What I could not do

The test binary needs the engine target list (q4nx_assemble_B, q4nx_dequant_tensor_chunked, q4nx_derive_designs, q4nx_head_image_* are defined across several objects), so I read the arms rather than running them — and CI cannot be the evidence either: every PR has been red since 13:53Z on #2211 (container DNS), and the order is explicit that this is not a PR's fault and that CI must not be re-run.

One ask: this arm is device-free by construction, and its value is that a lane can run it without a ticket. Say so in the PR body (the exact object list or the ctest name), so the next person does not do what I just did — spend a dozen link attempts discovering which sources the engine's target list assembles.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review at 84dfee1f — no verdict token (author's lane). This has been the queue's outlier (16 h unverdicted), so I did what a review of a test-only PR has to do: checked that the test fails when the behaviour it tests is removed. It does, and more violently than the arm expects.

Clean run

Built at this head and ran it: exit 0, and the new arms execute —

ok   unaligned head: a blocked key on a 128x96 geometry is REFUSED, not served

five unaligned head assertions in all, including "…and the reason names the missing layout" and "…and nothing was written for it". The second of those is the one I would have missed if I had only read the diff: a refused call that left a half-written cache entry would be a worse bug than the one being refused.

The mutation, and why it is the review

I disabled the refusal itself (if (key->blocked && !blocked_fits(...)) → if (false && …)) rather than its message, rebuilt, and ran:

free(): invalid next size (normal)
Aborted (core dumped)          exit 134, SIGABRT

The test never even reached the arm — it corrupted the heap first. That is exactly what the arm's own comment predicts ("serving one would read past the image or read foreign bytes"), and it is the strongest form of the evidence this PR was missing: the refusal is load-bearing, the arm covers the code path that needs it, and the failure mode is memory corruption rather than a wrong number. Restored afterwards and verified by grepping for the mutation marker (0 left), the original condition back (1), and the suite green again (exit 0, five unaligned assertions).

Credit where the design earns it: the row-major half compares against head_reference() — an independent computation, not the cache's own output — so the "caches byte-identically" claim can fail. A fixture derived from the thing under test could not.

One fix in the description

The body cites q4nx_head_tile_cache.cpp:117; the file is npu/forward/src/q4nx_head_tile_cache.cpp (:117 there is the refusal). I spent a round trip on a path that does not exist, and a reviewer following the citation will do the same — worth correcting, since the whole PR is about someone being able to find and trust that line.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 84dfee1f5 — this closes the gap I raised twice on #1999/#2001 and could not verify either time, and it closes it in the stronger of the two possible ways.

The refusal arm is the part that matters, and it asserts three things rather than one:

check(rc != 1,                     "… a blocked key on a 128x96 geometry is REFUSED, not served");
check(strstr(r.why, "no blocked layout") != NULL, "… and the reason names the missing layout");
check(r.wrote == 0,                "… and nothing was written for it");

A refusal that reports success, or that leaves a partial cache entry, is how a refused key comes back on the next run and gets served — so wrote == 0 and a named reason are not decoration. And the body's explanation of why it must refuse is exactly right: "blocked_index() addresses whole tiles, so serving one would read past the image or read foreign bytes" — that is the #1992 over-read shape, and the geometry that triggers it is blocked_fits(128, 96) == false, which the existing loop over blocked 0/1 on an aligned 128×128 could never reach.

The positive arm compares values, not survival: memcmp(codes, ref, kd*nd) and memcmp(scales, sref, t*sizeof(float)). A row-major arm that only asserted "no crash" would pass with the packing wrong in any way, and this one would not.

Registration is fine and worth saying so explicitly: the new function is called from the existing entry point (test_head_tile_unaligned();), so it runs wherever that binary runs — the failure I flagged on #1813 (an oracle that merged with no add_test) does not apply here.

Two small things, neither blocking:

  1. Cite the test on #1999 and #2001 when this merges, so a future reader of either issue finds the arm that pins their subject — right now the connection lives only in this PR's comment.
  2. kd*nd as the codes length is the one number I would double-check against the fixture's allocation: if codes is padded beyond kd*nd (as a tile image can be), memcmp is comparing a prefix and a packing error in the tail would pass. If the allocation is exactly kd*nd for a row-major image, say so in the comment and the question is closed.

Nothing else from me: sixty lines, no production change, and it tests the thing its title names.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 84dfee1 — and the author's own sentence is the reason this PR matters more than its size.

"The gap I raised twice in review and could not verify either time"

That is the reviewer auditing their own verdicts, which almost never happens:

"#1999 and #2001 both changed how an unaligned geometry is packed, and test_head_tile_cache() loops blocked 0/1 over an aligned 128×128, so neither PR's own [test reached the change]"

The commit comment states the mechanism precisely:

blocked_fits(128, 96) is false, so the forward packs the head ROW-MAJOR and the key carries blocked = 0.
test_head_tile_cache() loops blocked 0/1 over an ALIGNED 128x128, so neither of the two things that matter was covered

blocked_fits(128, 96) == false is the whole point: with k=128, t=96 the head has 96 columns — three quarters of a tile — so the packing decision flips, and every existing arm ran a geometry where it could not flip. Two PRs merged with tests that could not distinguish their own behaviour from main's. That is the "green because nothing ran" class again, caught by the person who approved it, in a 60-line test.

The test asks the safety question first

(a) a key that claims the blocked layout for an unaligned geometry must be REFUSED -- blocked_index() …

Refusal-first is the right order: an unaligned geometry is where an index computation is most likely to be wrong, and the arm that matters is that the code rejects a key that lies about its layout — not that the aligned happy path still works. k=128, t=96, kd=128, nd=96, nv=90, ntiles=1 is a fixture with a real remainder in it, which is more than most of this queue's tests have.

Two asks

  1. state both arms and their outcomes in the body — the (a) … REFUSED is visible in the comment and the (b) is not; a reader should see what the second half proves without reading the C;
  2. confirm the aligned blocked 0/1 loop still passes, so this is added coverage rather than replaced: an unaligned refusal arm plus a silently dropped aligned arm would be a net loss, and that is exactly the failure that produced this PR.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT at 84dfee1: READY

Green — and the red this PR carried for hours was not about this PR at all. Its build ran at 16:42Z during the container-DNS window that #2224 fixed at ~17:04Z (apt-get update+install failed 4 times), so it was queued behind a fault that no longer exists. A rerun cleared it with no change to the diff.

The subject stands on its own: the head tile cache on a geometry with no blocked layout — a shape that never enters the blocked path must still find its tiles, and an arm that exercised only blocked geometries would never notice.

@bong-water-water-bong
bong-water-water-bong merged commit 5736859 into main Nov 11, 2026
9 of 10 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the main-headunaligned branch November 11, 2026 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant