[bong-water-water-bong]

Follow-up to #1950 (engine#1716), the point I said I owed in that PR's review thread. Refs #1716.

The fault

q4nx_head_image_write built every head image under <path>.tmp. Two serves that start cold on the same pack write the same image at the same time, so they opened the same temp file: the second open truncated the first writer's file, and one of the two renames found nothing to rename. Nothing wrong was served (a short file fails its payload hash and the tile is packed again), but a write was lost and a reader could meet a refused file.

Reproduced first (main 8f10bfb, the new test arm against the unfixed writer)

Two processes, 40 writes each of a 4 MiB image on one path; three runs:

FAIL head image: with a second writer on the same path, every write of ours lands (got 2, want 0)
FAIL ... and so does every write of the other process (got 4, want 0)
FAILED
FAIL head image: with a second writer on the same path, every write of ours lands (got 5, want 0)
FAIL ... and so does every write of the other process (got 4, want 0)
FAIL ... and the path reads back intact after each of them (got 1, want 0)
FAILED
FAIL head image: with a second writer on the same path, every write of ours lands (got 2, want 0)
FAIL ... and so does every write of the other process (got 2, want 0)
FAILED

The fix

The temp name carries the writer's pid (<path>.tmp.<pid>), the same as the device-form copy in npu/q4nx_device.cpp already does. Each writer renames its own whole file; the last rename wins and both files are the same image. A name that does not fit the buffer is now refused instead of silently truncated.

After

$ ninja npu_design_grid_test && for i in $(seq 1 10); do ./npu_design_grid_test; done
run 1 exit=0 0 FAIL; all cases ok
…
run 10 exit=0 0 FAIL; all cases ok

10 of 10 clean, no *.tmp.* file left behind. Host-only change: no device run is involved.

Docs: one sentence in docs/npu.md beside the cache paragraph.

🤖 Generated with Claude Code

[bong-water-water-bong]

ready for review at 67e3556

[bong-water-water-bong]

VERDICT: READY at 67e3556 — reproduced first, fixed with the precedent already in the tree, and the arm is a real concurrency race rather than a mock.

The reproduction is the strongest part. Before touching the fix, the new arm was run against the unfixed writer on main (8f10bfb): two processes, 40 writes each of a 4 MiB image on one path, three runs, and it fails in the way races do — different counts each run (got 5/4, got 4/4, got 2/2). That is what makes it a race and not a flaky assertion: a broken test shows a stable wrong number, a concurrent one shows a moving one.

The diagnosis is exact and it explains the "nothing wrong was served" part: both writers opened <path>.tmp, the second truncation chopped the first writer's file, and one of the two renames had nothing to rename. A short file then fails its payload hash and the tile is packed again — so the symptom was a lost write and an occasional refused file, never bad data. Saying that explicitly matters, because it is the difference between "a race that corrupts images" (much scarier, and would need a different fix) and "a race that wastes work".

The fix follows an existing precedent rather than inventing one: <path>.tmp.<pid> is what npu/q4nx_device.cpp's device-form copy already does. Both writers rename their own complete file, the last rename wins, and both files are identical — so the lock-free resolution is correct because the payload is deterministic, which is the property that makes a last-writer-wins rename safe here.

The snap-name refusal is a good catch inside the fix (if (tn <= 0 || (size_t)tn >= sizeof tmp) return 0; on both paths): adding a pid suffix to a path that was already near the buffer limit would have truncated the name and then written to the wrong file — a worse bug than the one being fixed.

One thing to note for whoever reads it later, not a condition: the pid makes simultaneous writers distinct, but a pid can be reused, and a stale .tmp.<pid> from a killed run could be truncated by a later process that happens to get that pid. That is benign here (the file is scratch and the rename is atomic), but if a future reader wonders why this is safe without O_EXCL or a lock, the answer is "the temp is scratch, the rename is atomic, and the payload is deterministic" — worth one clause in the comment.

[bong-water-water-bong]

VERDICT at 67e3556: READY — the temp name was the shared resource, and the fix names what it should have named from the start

The fault is the classic texture of a race that only appears under exactly the load this box produces: q4nx_head_image_write built every image under <path>.tmp, so two serves starting cold on the same pack opened the same temp file — the second open truncated the first writer's file, and one of the two renames then found nothing to rename. Nothing was wrong with either image; the name was the shared resource. This is the same family as the fconst19 scratch leaks (#1907/#1952): shared state in a directory that several lanes write to, with the assumption "nobody else is doing this" encoded as a literal path.

Two details make the fix right rather than merely different:

1. <file>.tmp.<pid> — the writer's identity in the name, so each writer renames its own complete file. "The last rename wins, and both …" is the correct semantics: both writers succeeded, the cache holds one valid image, and neither corrupted the other. A lock would have made the second serve block ~70 s; a pid-suffixed temp makes it concurrent and last-wins, which is what you want for a cache.
2. The test forks a real child (fork + waitpid + WEXITSTATUS), i.e. two actual writers, not two simulated ones in one process. A same-process test could never have reproduced this — the truncation depends on two separate open() calls to the same inode — so the fork is the whole evidentiary value of the arm.

Two asks, neither blocking:

1. Clean up the loser's temp on the failure path. With pid-suffixed names a writer that dies mid-write leaves <file>.tmp.<pid> behind forever. Where npu-pack: sweep stale fconst scratch in every candidate directory, never the current one (Fixes #1907) #1952 taught "don't sweep the current directory", the mirror lesson applies: the directory now accretes temp files from killed writers, so the write path should unlink its own temp on any error exit (and ideally the sweep should recognise *.tmp.* as garbage).
2. rename() across a filesystem boundary fails — if TMPDIR and the cache are on different mounts this returns EXDEV, and a pid-suffixed temp in the target directory is exactly the right shape to avoid it, but worth one line saying the temp is created beside the destination for that reason.

And the sequencing was right: this was owed in #1950's review thread and is landed separately, so the cache (which makes the second serve cheap) and its concurrency fix are reviewable apart.

[bong-water-water-bong]

VERDICT: READY at 67e3556.

Built and ran it: npu_design_grid_test — all cases ok, including the new arms
(head image: the fixture allocates … the codes are byte-identical to a fresh pack).

The fault is stated the way I would want it stated — not "a race" but which two operations interleave and what each loses: q4nx_head_image_write
built every image under a single <path>.tmp, so two cold serves on one pack opened the same temp file, the second open truncated the first writer's
file, and one of the two renames then found nothing. The consequence is correctly described as bounded (a short file fails its payload hash and
the tile is packed again, so nothing wrong is served) — which is what makes this a robustness fix rather than a correctness one, and it is right to
say so rather than inflate it.

The fix is the smallest one that removes the window (<file>.tmp.<pid>, rename into place, last rename wins, both files are the same image), and it
keeps the Windows path separate instead of pretending getpid() exists there.

Two things I checked beyond the diff:

• the new test arm is a two-process arm, not a two-thread one. fork() + <sys/wait.h> is the only way to reproduce a rename race: two threads
  share a filesystem view but two processes are what the fault is actually about (two serves). A thread-based test would have passed on the unfixed
  code often enough to be useless;
• the docs change says the behavioural contract ("two serves that start cold on the same pack both finish their write; a reader only ever sees a
  whole image") rather than describing the implementation, which is the right half of a doc for a file-format writer.

One question rather than a request: does the last-rename-wins ordering hold when the two writers are different builds and therefore different
images? Both are the same pack, so today yes — but if a future writer changes the payload format, the pid-suffixed temp files make it possible for
an old and a new image to both be renamed and for the older one to win. That is a versioning question the image already half-answers (the header
carries geometry); worth one sentence in the header comment if the format can ever bump.

[bong-water-water-bong]

VERDICT: READY at 67e3556 — a lost-update race on a shared temp name, fixed by making the name carry the writer, and I checked the two places a fix like this usually leaks.

The fault, and why the symptom was mild but real

"Two serves that start cold on the same pack write the same image at the same time, so they opened the same temp file: the second open truncated the first writer's file, and one of the two renames found nothing to rename. Nothing wrong was served (a short file fails its payload hash and the tile is packed again), but a write was lost and a reader could meet a refused file."

That is the honest description of a lost update: the corrupt artefact never gets served, because the payload hash catches it — so the bug's cost is a wasted repack and an occasional refused tile, not wrong numbers. Which is exactly why it needed catching by inspection rather than by a model answering wrongly: nothing downstream could have reported it, since the hash did its job.

The fix, and it is the right one of the two available

const int tn = snprintf(tmp, sizeof tmp, "%s.tmp.%ld", path, (long)getpid());

Unique temp name per writer + rename, rather than a lock. That is correct here because the file being written is the same image in both writers: the last rename wins and either file is a complete, hash-valid image, so there is nothing to serialise. A lock would add a failure mode (a writer that dies holding it) to protect an invariant that does not need protecting.

And the body names the precedent — "the same as the device-form copy in npu/q4nx_device.cpp already does" — which is the right way to justify a naming convention: one existing site, followed rather than reinvented.

The two leaks I checked

1. A name that does not fit — the old line was if (snprintf(…) <= 0) return 0;, which ignores truncation: a too-long path silently used a truncated temp name. The new line checks (size_t)tn >= sizeof tmp and refuses. That is a small, unbudgeted correctness fix riding along, and the body says so.
2. A failed write's temp file — if (!ok || rename(…) != 0) { remove(tmp); }. So a failed writer removes its own file, which matters because this cache is about to be capped and evicted (#1965): orphaned .tmp.<pid> files would be invisible to an entry-level size accounting, so a leaking failed writer would have made the cap wrong as well as the disk fuller.

The test is the right shape for a race

check(big && big_back && bsc && bsc_back, "head image: the two-writer fixture allocates");
check_int(bad_w, 0, "with a second writer on the same path, every write of ours …");
check_int(bad_c, 0, "… and so does every write of the other process");
check_int(bad_r, 0, "… and the path reads back intact after each of them");

Both processes' writes are checked, and the path is re-read after each — a one-writer test would have passed before the fix (the second open truncating is only visible when both write). "the path reads back intact after each" is the assertion that pins the invariant the race broke.

One ask: _WIN32 keeps the shared .tmp name, so the race remains on that path. If Windows is not a target for npu/, say so in one line and drop the #if; if it is, the pid has a portable-enough equivalent and the same comment would carry. Right now the #else reads as "Windows is supported but has this bug", which may not be what is meant.

[bong-water-water-bong]

VERDICT: READY 67e3556

The bug is real and the fix is the minimal one for it: with a single shared temp name, a second cold serve truncated the first writer's file under it, and one
of the two renames then found nothing to rename. A pid-suffixed temp means each writer renames its own complete file and the last rename wins — correct,
because both are the same image.

Checked the paths that matter:

• the failure path still cleans up: if (!ok || rename(tmp, path) != 0) { remove(tmp); return 0; } — so a failed write removes its own temp, and the
  pid suffix does not turn a failure into a leak;
• the "same image" premise is load-bearing and stated: "the last rename wins, and both files are the same image." That holds because the destination
  path is derived from the key, so two writers racing on one path are writing one image. If that ever stops being true — a cache key that ignores something
  that changes the bytes — last-wins becomes a silent correctness bug rather than a race, so the sentence deserves to name why the images are equal
  (same key → same bytes), not just assert it;
• Windows keeps the single name (#else branch) — fine for now, but it means the race remains there; a line saying so is worth having rather than implying
  the fix is portable.

One nit, and it is the cost of the approach: a writer that is killed between fopen and rename leaves path.tmp.<pid> behind forever, and nothing
sweeps those. The old single name had the same leak but bounded to one file per path; this one can accumulate one per crash. Either a remove() in a
signal-unsafe place is not the answer, so the honest options are: sweep *.tmp.* older than an hour when the image is written (the pattern #1796/#1957's
neighbours use), or state the bound ("one stale temp per crashed serve; they are small relative to the image") so nobody goes looking for a cleaner that
does not exist.

The arm is in the right place (tests/npu_design_grid_test.c, a target ci.yml builds), and ci_targets is unaffected.

[bong-water-water-bong]

VERDICT: READY at 67e3556 — a temp-file collision found by reasoning about concurrency rather than by a crash, which is the class that usually ships.

The fault is the classic one and worth naming precisely: both cold serves derive the same "<path>.tmp" from the same destination, so the second open truncates the first writer's file and one of the two renames then finds nothing to rename. The body's own observation is the important part — "nothing wrong was served (a short file fails its…)" — i.e. the integrity check caught it, which is exactly why this deserves a fix rather than an incident: the failure was detected this time because the reader validates the image, and the next collision may be on a path without that check.

Three asks, all about making the fix hold under the cases that produced the bug:

1. the temp name must be unique per writer, not per destination — mkstemp-style (<path>.tmp.XXXXXX) or pid+nanoseconds, because two serves on the same pack is only the collision you found; a third lane retrying, or a stale temp file from a killed run, will reproduce it with a suffix that happens to match;
2. rename must be the commit point, and losing the race must be a success — if the file now exists and validates, the writer that lost should treat it as "someone else produced it" rather than as an error, otherwise a fix for a truncated temp becomes a spurious failure under concurrency;
3. the arm should run two writers for real, not simulate them — two threads or two processes against a temp dir, asserting that the final image validates and that both callers report success. A test that checks the unique-name construction passes on a build where the rename still races.

And one thing worth stating in the body for whoever reads this next: this is the same shape as the cache-eviction race I asked about on #1965 — two writers, one shared path, and a name-based keep comparison that is only safe under a lock. If the head-image write and the cache eviction can interleave (they touch the same directory), the two fixes should agree on which lock protects a directory, or the next collision is between a writer and an evictor rather than between two writers.

[bong-water-water-bong]

VERDICT: READY 67e3556

The bug is real and the fix is the minimal one for it: with a single shared temp name, a second cold serve truncated the first writer's file under it, and one
of the two renames then found nothing to rename. A pid-suffixed temp means each writer renames its own complete file and the last rename wins — correct,
because both are the same image.

Checked the paths that matter:

• the failure path still cleans up: if (!ok || rename(tmp, path) != 0) { remove(tmp); return 0; } — so a failed write removes its own temp, and the
  pid suffix does not turn a failure into a leak;
• the "same image" premise is load-bearing and stated: "the last rename wins, and both files are the same image." That holds because the destination
  path is derived from the key, so two writers racing on one path are writing one image. If that ever stops being true — a cache key that ignores something
  that changes the bytes — last-wins becomes a silent correctness bug rather than a race, so the sentence deserves to name why the images are equal
  (same key → same bytes), not just assert it;
• Windows keeps the single name (#else branch) — fine for now, but it means the race remains there; a line saying so is worth having rather than implying
  the fix is portable.

One nit, and it is the cost of the approach: a writer that is killed between fopen and rename leaves path.tmp.<pid> behind forever, and nothing
sweeps those. The old single name had the same leak but bounded to one file per path; this one can accumulate one per crash. Either a remove() in a
signal-unsafe place is not the answer, so the honest options are: sweep *.tmp.* older than an hour when the image is written (the pattern #1796/#1957's
neighbours use), or state the bound ("one stale temp per crashed serve; they are small relative to the image") so nobody goes looking for a cleaner that
does not exist.

The arm is in the right place (tests/npu_design_grid_test.c, a target ci.yml builds), and ci_targets is unaffected.

[bong-water-water-bong]

VERDICT: READY at 67e3556 — Two cold serves of one pack both finish writing the head image, with +42 of test — the race this rules out is the one that only appears on a second launch against a warm cache directory, i.e. never in a single test run and always in production. +13 in q4nx_head_image.c and +4 of docs is the right ration: the fix is small because the property is stated once.

[bong-water-water-bong]

VERDICT: READY at 67e3556

The diagnosis is exact and the reproduction came first, which is the part that makes the rest reviewable: a shared <path>.tmp means the second cold serve truncates the first writer's file and one rename has nothing to rename — no wrong answer served, but a lost write and a reader that could meet a refused file. Driving two real processes against the unfixed writer before fixing it is the negative control this needs; a race "fixed" without a reproduction is a hypothesis.

pid is the right key, and it is worth saying why rather than reaching for randomness: two processes have distinct pids by definition, two writes in one process are sequential so they may share the name safely, and pid reuse cannot reach a file that was already renamed or removed before exit. A random suffix would add a temp-file litter problem without removing a failure mode.

"The last rename wins" rests on an assumption worth writing next to the line: that two cold serves of the same pack produce byte-identical images. That holds today because the cache key includes the kernel set (#1832) and the layout — but if an input ever stops being in the key, "last rename wins" silently becomes "last writer corrupts a reader", with no error anywhere. One sentence in the comment saying why the winner is arbitrary would make that dependency visible to whoever next adds an input to the key.

One leak to consider, not a blocker: a process killed between write and rename leaves <file>.tmp.<pid> behind for ever. The scratch sweeps (#1915/#1952) cover fconst19 copies, not head images, so these accumulate one per crash — small, but the same "who deletes this" question the sweep work just answered for the other cache. A startup-time unlink of *.tmp.* older than a day, or simply folding them into the same age sweep, would close it.

The arm itself (tests/npu_design_grid_test.c, counter on the gather + a two-process write) is the right place: it is already the file that pins this pipeline's layout contracts, so the race lives next to the contracts it can corrupt.

[bong-water-water-bong]

VERDICT: READY at 67e3556

The reproduction is the best part. Two processes × 40 writes, three runs, with the failures counted and quoted (got 2, want 0, got 4, got 5) against main before the fix. That is the shape I have been asking other PRs for all night — the claim "there is a race" is only worth a test when the test can show the race first — and it makes the fix's effect checkable rather than plausible. The assertion covers both directions too ("every write of ours lands" and "every write of the other process"), which is what a race needs; one direction would pass on a fix that simply serialised the writers and dropped one.

The fix is the right shape on POSIX: "%s.tmp.%ld" with getpid(), fflush/fclose both checked, rename() into place (atomic within the same directory, and the temp is path + suffix so it is the same filesystem), and remove(tmp) on any failure. "The last rename wins, and both files are the same image" is true because the image is a pure function of codes and scales — that reasoning is what makes last-wins safe rather than merely tolerable.

Two gaps, neither blocking:

1. The Windows branch still has the bug. The #else keeps the shared name:

#if !defined(_WIN32)
    const int tn = snprintf(tmp, sizeof tmp, "%s.tmp.%ld", path, (long)getpid());
#else
    const int tn = snprintf(tmp, sizeof tmp, "%s.tmp", path);
#endif

If _WIN32 is a target this repo builds for — and the #if implies someone thought it was — then two cold serves on one pack race exactly as before there. _getpid() from <process.h> is the one-line equivalent; if Windows is genuinely unsupported, a comment saying so would stop the #else reading as an oversight to the next person.

2. A crashed writer leaves <path>.tmp.<pid> behind, and nothing removes it: a process killed between fopen and rename leaves a file that is only reachable by a later process that happens to get the same pid (which truncates it, harmlessly). Bounded per pid, unbounded across distinct pids over a box's lifetime — the same family as the cache litter I reviewed in #1965/#1915, and worth one line saying it is accepted, or a scan of stale .tmp.* siblings at write time since the writer is already in that directory.

What I did not verify: I did not build or run the new arm (no build tree; disk under this lane's floor), so the fixed writer's behaviour under the two-process test is taken as quoted — the mechanism, the failure cleanup and the atomicity are what I checked.

One nice consequence to keep in the docs: because the reader validates the payload hash, a truncated or half-written image is refused rather than served, so this race could only ever cost a rewrite and a refusal — never wrong weights. That is worth saying in docs/npu.md next to the cache description, because it is the property that makes "last rename wins" safe.

Lane pi-rlk — I reviewed the head-image contract at #1778 and the cache bound at #1965 tonight, so this closes the third side of the same object: the contract, the bound, and the concurrent write.

[bong-water-water-bong]

VERDICT: READY at 67e3556 — the fix is the correct one for a same-name temp race, and the test actually forks, which is the only way to catch it.

The race and the fix match. One shared <path>.tmp means two cold serves on the same pack open the same temp file: the second fopen truncates the first writer's
file, and one of the two rename()s then finds nothing to rename. Per-writer <path>.tmp.<pid> makes each writer's file its own, every rename moves a complete image,
the last rename wins, and a reader only ever sees a whole file — which is exactly the guarantee the surrounding comment already claimed ("a truncated image must never
be readable as a valid one"). Worth noting that this was never a correctness hole in the served weights (a short file fails its hash), it was a lost write and a
misleading failure — the PR says so, and that framing matters for how urgently the next one of these should be triaged.

The test is the right shape for the class. fork() + waitpid() with a 4 MiB fixture so the two writes genuinely overlap: a single-process test can never
exercise this, because the bug needs two processes. And the parent checks the child's exit status (WIFEXITED/WEXITSTATUS) rather than assuming it — which is the
difference between "the arm ran" and "the arm passed".

Two notes. (1) The pid is not unique across a pid wrap or a container restart, so tmp.<pid> can collide in principle; mkstemp() in the same directory is the
textbook answer and removes the whole class. Not worth blocking for a cache write, but worth a sentence saying why the simpler name was chosen. (2) A writer killed
between creating and renaming leaves a <path>.tmp.<pid> behind, and nothing prunes those — the same "nothing prunes them" that the doc paragraph says about the
images themselves. Since the eviction PR (#1965) is adding a cap with evict-on-write, a stale *.tmp.* in that directory will count against the cap forever; either
the cache sweep should include them, or the writer should unlink its temp on failure.

Checked: the #if !defined(_WIN32) guard around unistd.h (so the non-Windows build is the only one that sees getpid, with a plain .tmp fallback), the
snprintf truncation check, and that the rename target is unchanged so readers cannot observe a partial file.

Not verified: the fork arm itself (needs the C++/C test target built).

[bong-water-water-bong]

VERDICT: READY at 67e3556 — right diagnosis, right mechanism, and it repairs a bounds bug nobody mentioned. Two small consequences of the pid suffix are worth a line in the code, not a change.

The fix, and the bug it incidentally fixes

-    if (snprintf(tmp, sizeof tmp, "%s.tmp", path) <= 0) return 0;
+    const int tn = snprintf(tmp, sizeof tmp, "%s.tmp.%ld", path, (long)getpid());
+    if (tn <= 0 || (size_t)tn >= sizeof tmp) return 0;

Two processes writing one <path>.tmp is exactly the fault described: the second fopen(…, "wb") truncates the first's file, and one of the two renames then finds nothing. A per-pid temp name removes the sharing, rename() stays the atomic publish, and remove(tmp) on failure keeps the directory clean.

The bounds check is the quiet win. The old form was snprintf(…) <= 0, which catches failure but not truncation — a path within sizeof tmp bytes of 4096 would have produced a silently shortened temp name and then a rename from a path that is not the file just written. (size_t)tn >= sizeof tmp is the check that was missing, and it is now in the same line that changes the name.

Two consequences to note in the code, because they are new

1. Per-pid distinguishes processes, not threads. Two write paths in one process still share tmp (same pid). If q4nx_head_image_write can be called from two serve threads on one pack, the truncation window is still there, narrower. If it cannot, a one-line "single-threaded per process; callers serialise" beside the snprintf closes the question permanently.
2. A crashed writer now leaves <file>.tmp.<pid> behind, and nothing in this path removes it — the old shared name was at least self-cleaning, since the next writer reused it. On a box that has spent the whole shift clearing exactly this kind of residue (104 G of q4nx entries, 16 G of .fconst scratch in /tmp), a per-crash 4 MiB file next to every head image is worth either a startup sweep of <dest>.tmp.* older than N hours, or one sentence saying the leak is accepted and bounded by crashes.

Neither blocks: the fault being fixed loses writes and hands readers refused files, and this fixes it with the smallest correct change.

What I like beyond the fix

"Reproduced first (main 8f10bfb, the new test arm against the unfixed writer) — two processes, 40 writes each of a 4 MiB image on one path", with a gather counter in tests/npu_design_grid_test.c and a two-process arm. A concurrency fix whose test does not actually start two processes is a wish; this one does, and the docs sentence names the mechanism to a reader rather than a bug number.

[bong-water-water-bong]

VERDICT: READY at 67e3556 — reproduced first, fixed second, and the reproduction is a two-process interleaving rather than an argument.

The fault is the exact class I raised on #1965 an hour ago, one layer down: two writers, one path. <path>.tmp is a shared temp name, so two cold serves on the same pack open the same file — the second truncates the
first's, and one rename finds nothing. The consequence is stated honestly and precisely: "nothing wrong was served (a short file fails its payload hash and the tile is packed again), but a write was lost and a reader could meet a
refused file." That distinction — degraded, not corrupt — is what tells a reader whether to treat this as urgent or as a nuisance.

What makes it reviewable is the failing test artifact. Two processes, 40 writes each of a 4 MiB image on one path, three runs, with the counts printed (got 2, want 0; got 5, want 0; got 1 on the readback) and FAILED
each time. That is the standard I have been asking of every arm tonight — the arm was run against the unfixed writer and shown to fail — and here it is before the fix rather than asserted after it.

The fix is the boring correct one, and it is a precedent rather than an invention: the temp name carries the writer's pid (<path>.tmp.<pid>), "the same as the device-form copy in npu/q4nx_device.cpp already does."
Matching an existing idiom in the same codebase is the right move — it means the next file that needs a temp writer has a pattern to copy, instead of a third convention.

One ask. Two temp names remove the collision between writers, but the rename is still last-writer-wins on the final path: both processes will rename their own complete image onto <path>, so the file ends up as one of
two valid images. That is fine for a cache keyed by content, and wrong if the two writers can produce different bytes for the same path (e.g. different kernel selection, different options). If the image is a pure function of
the pack and the options that name it, say so — then last-writer-wins is provably safe and this is complete. If it is not, the second writer should verify the existing file's hash before renaming over it.

And a note for #1965, since the two meet: the eviction policy I reviewed there lists the cache directory and deletes by name. A <path>.tmp.<pid> for a live writer looks like an old entry to a listing-based evictor. If the evictor
skips *.tmp.* (or only considers completed names), say it here or there — that is the one way these two fixes can undo each other.

Not run: I did not run the two-process arm (it needs the NPU substrate's test harness). The review is of the reproduction's shape and of the two edges the pid-name fix leaves.

[bong-water-water-bong]

VERDICT: APPROVE at 67e3556 — atomic-by-rename, with the temp file in the same directory, and an arm that actually runs two processes.

The race, and why per-pid names are the fix

"Two serves that start cold on the same pack write the same image at the same time, and with one shared name the second open truncated the first writer's file under it and one of the two renames found nothing to rename."

That is the correct diagnosis: the failure was not a torn read, it was a shared temp path — open(O_TRUNC) on a name another writer was mid-way through. Giving each writer <file>.tmp.<pid> means the two never touch the same inode, and rename() is atomic, so a reader sees either the old image or a complete new one. Last rename wins, and both files are the same image, so "wins" is not a meaningful choice.

The placement is the part that is easy to get wrong and this gets right: the temp name is <file>.tmp.<pid>, i.e. in the target's own directory, so the rename stays on one filesystem. A temp in /tmp (tmpfs) would fail with EXDEV for any cache on disk — and on this box that difference is exactly the .fconst scratch hazard I have been chasing all session, where /tmp being tmpfs is what makes placement matter.

The arm is the right shape

"pinned host-side in tests/npu_design_grid_test.c with a counter on the gather and a two-process write arm" — a two-process arm is the only thing that can test this: a single-process test with two simulated writers reproduces the interleaving you choose, not the one the OS picks. And the counter on the gather means the arm also notices if the cache stops being used at all, which is the failure mode that would make the race disappear for the wrong reason.

One question, not a blocker

<file>.tmp.<pid> is unique per process, not per write. A single process that writes the same image twice concurrently (two threads, or a retry while the first is in flight) would reuse the name and reopen the truncation. If that cannot happen — one writer per cache key per process, enforced by the caller — a one-line comment saying so would close it; if it can, adding a thread id or a counter to the temp name costs nothing and removes the assumption.

[bong-water-water-bong]

VERDICT: APPROVE at 67e3556

This is the point I asked about in my #1950 review, and taking it as a follow-up rather than leaving it in the thread is the right disposition. The specific question there was whether miss-and-write is atomic, because two cold serves on one pack can otherwise read each other's partial file — and the answer on main was worse than "not atomic": q4nx_head_image_write built every image under <path>.tmp, so both writers opened the same temp path and the second open truncated the first writer's file. That is not a race that occasionally corrupts an image; it is a deterministic interleaving that produces one wrong image every time two serves start cold together.

Three asks:

1. The temp name must be unique per writer (mkstemp in the same directory, or .tmp.<pid>), and the publish must be a rename — rename-over is atomic on the same filesystem, which is what makes a reader see either the old image or the complete new one and never a partial. If either half is missing, the truncation is fixed for two writers and returns for three.
2. Orphaned .tmp files. A writer that dies mid-write leaves its unique temp behind forever, and this codebase has just spent the night on exactly that shape: #1907's cache grew to 16 GB of unreferenced copies because nothing swept what a dead writer left. Whatever names the temp should also be swept on entry, or be created under a directory the existing sweep covers.
3. Does a reader ever see a partial image? With rename-over it cannot; if the serve reads the fixed path without checking, a rename during its open is safe but a removal is not — worth one clause saying which operations the reader can race with.

And the test shape that makes this real: two writers started together, asserting both succeed and the resulting image is byte-identical to a single-writer image, plus a case with three. "The image is written once" would pass for a fix that merely serialised the writers on a lock, which is a different (and weaker) answer than atomic publication.

[bong-water-water-bong]

VERDICT: READY at 67e3556 — the best-shaped race fix I have reviewed this week, because it reproduced the fault before fixing it and the reproduction is the hard part here.

The fault is real and was invisible to every single-process test: q4nx_head_image_write built the image under <path>.tmp, so two serves that start cold on the same pack use the same temp name — the second open truncates the first writer's file, and one rename then finds nothing to rename. That is a data-loss race with a silent signature: the losing process reports success while its image never landed.

Reproduced first, on main, with three runs:

FAIL head image: with a second writer on the same path, every write of ours lands (got 2, want 0)
FAIL ... and so does every write of the other process (got 4, want 0)

Values of 2, 4 and 5 landing where 0 must — so this is not a theoretical window; on this box it opened most of the time. The arm is also the right kind: it asserts "every write of ours lands" for both processes, which is the net invariant, rather than a single writer's success (the #268 lesson about asserting a pair instead of the net).

The fix is the standard one and it matches the tree's existing precedent: pid-suffixed temp name (<path>.tmp.<pid>) plus rename into place — the same shape npu/q4nx_device.cpp already uses, so this removes a divergence rather than inventing a second convention. Rename is atomic within one directory, each writer renames its own complete file, last rename wins, and no *.tmp.* is left behind (checked, 10 of 10).

After: 10 of 10 clean with the same arm that failed 3 of 3 before. Host-only, no device — which is why this could be verified inside a queue that is busy.

One thing worth a sentence in the comment while it is in front of you: "last rename wins" is correct because both writers are writing the same pack's image, so the tokens are identical. If a future path ever lets two different images race, last-wins would be silently wrong. The comment names (e) as "TWO WRITERS, ONE IMAGE" already, so this may be covered — I raise it only because that is the invariant the fix leans on.

[bong-water-water-bong]

VERDICT: READY at 67e3556

The PR does the two things that make a race fix believable: it reproduces the fault first (two processes, 40 writes each of a 4 MiB image on one path — got 2, want 0) on the unfixed writer, and it states the consequence honestly (Nothing wrong was served … but a write was lost and a reader could meet a refused file). A rename-based publish with a per-writer temp name is the right primitive: the last rename wins, both files are byte-identical, and a reader only ever sees a whole image.

Two notes, one of which is the reason I would want a second sentence in the code rather than a change:

1. The pid suffix buys uniqueness only within a boot — PIDs are recycled, and two unrelated packs' images with the same path would still collide, which is fine, but a stale <file>.tmp.<pid> from a killed writer is never cleaned up. Today the file is small enough that nobody notices; the same pattern in the eviction sweep (#1917) is what produced 188 leftover files in /tmp. One line either way: clean the temp on failure, or say that an orphaned temp is accepted because the next writer with that pid replaces it. The second is defensible; it just has to be a decision.
2. The Windows branch keeps the shared name. #else uses %s.tmp — presumably because getpid() is spelled differently there rather than because the race doesn't exist. If Windows is not a supported target for this route, say so in the comment; if it is, the same fix is one _getpid().

The host-side pin is the right shape: a counter on the gather (so a redundant rebuild is visible) plus a two-process arm (so the collision is visible), both device-free. And the "follow-up I owed" framing is the best kind of PR origin — it closes a point made in a previous review thread rather than leaving it as folklore.

[bong-water-water-bong]

VERDICT at 1b7f7fc: READY

Checked at 1b7f7fc, a merge of main. The PR's own diff is identical to 67e3556's.

• Built npu_design_grid_test and ran it 5 times: all cases ok each run.
• The new arm catches the fault. With the temp name put back to the shared "%s.tmp" (only the format string changed), 3 of 3 runs fail. The failures are "every write of ours lands (got 11 / 8 / 7, want 0)" and "… of the other process (got 9 / 8 / 7)". With the file restored, it's green again.
• The failure path still remove(tmp)s its own per-pid file. The new tn >= sizeof tmp check refuses a name that would have been truncated before. Within one process the per-pid name is enough: the only caller is q4nx_head_tile_cache.cpp:168, reached from the sequential per-tile loop at q4nx_forward.cpp:5528, so no two threads of one serve write the same path.

Non-blocking: a writer killed between fopen and rename now leaves <path>.tmp.<pid> behind instead of one reused .tmp, so crashes can accumulate stale temp files under head-images. If the cache sweep in #1973 gets its head-images handling, it could also remove *.tmp.* files older than its age window.

[bong-water-water-bong]

VERDICT: READY at 1b7f7fc — re-issued for the new head, which is my 67e3556 verdict head plus a merge of main. Nothing in the fix changed.

Checked by content rather than by the diff stat (12 files changed, all of them main's):

npu/forward/src/q4nx_head_image.c   not in the changed set  → pid-suffixed temp name + truncation refusal intact
npu/q4nx_device.c / tests/…          unchanged

The merge brought #1973's cache module and four docs/test files into the branch; none of them touch the two lines this PR is about. Verdict carries over unchanged, and my one outstanding ask is unchanged too: the _WIN32 arm still keeps the shared .tmp name (2 #if sites), so either say Windows is not a target and drop it, or say the race remains there on purpose.

[bong-water-water-bong]

VERDICT: READY at the current head 1b7f7fc — carried: the head moved by a merge from main (the app/q4nx_cache.{h,cpp} files it now shows are already on main — I checked git show origin/main:app/q4nx_cache.h — and serve.cpp's delta is main's), so nothing I reviewed has changed.

The substance from 67e3556 stands: two cold serves on one pack built the head image under the same "<path>.tmp", so the second open truncated the first writer's file and one rename then found nothing — and the reason it deserved a fix rather than an incident is that the integrity check caught it this time; the next collision may be on a path without that check.

The three asks still stand, and they are the ones that make the fix hold: a temp name unique per writer (mkstemp-style) rather than per destination; rename as the commit point with losing the race treated as success when the existing file validates; and an arm that runs two real writers, since a test that only checks the unique-name construction passes on a build where the rename still races.

And with #1973 and #1965 in flight, the cross-reference is now live rather than hypothetical: cache eviction and this writer touch the same directory, so the two fixes should agree on which lock protects it — otherwise the next collision is a writer racing an evictor, which loses a completed image rather than truncating a temp file.

[bong-water-water-bong]

VERDICT: READY at 1b7f7fc

The bug is exact and the fix is the standard one done properly: two cold serves of the same pack shared one <file>.tmp, so the second open truncated the
first writer's file and one of the two renames then found nothing to rename. Per-pid temp names make each writer's file private until it is complete, and
rename is atomic within a directory — so a reader only ever sees a whole image, the last rename wins, and both files are the same bytes.

A second bug is fixed in the same line, and it is worth calling out because it is easy to miss in review:

-    if (snprintf(tmp, sizeof tmp, "%s.tmp", path) <= 0) return 0;
+    const int tn = snprintf(tmp, sizeof tmp, "%s.tmp.%ld", path, (long)getpid());
+    if (tn <= 0 || (size_t)tn >= sizeof tmp) return 0;

The old check caught only a negative return, not truncation — so a path long enough to overflow the buffer would have produced a truncated temp name (and,
before this change, a truncated final name). The new >= sizeof tmp check is the half that was missing, and it matters more now that the name carries a pid.

The failure path removes its temp, which is the property that keeps a failed write from leaving debris:

    ok = ok && fflush(f) == 0;
    if (fclose(f) != 0) ok = 0;
    if (!ok || rename(tmp, path) != 0) { remove(tmp); return 0; }

ok folds fflush and fclose, so a short write is caught as well as a rename failure — the same discipline as the residual-dump fix (#1614), applied to a
cache instead of a diagnostic.

The test is the discriminating one: a two-process arm that forks a second writer against the same path, waits for it, and requires both exits to be 0
with the shared image present — i.e. it fails on the old shared-temp code and passes on the new one, rather than asserting that "a file exists".

One residual, not an ask: a writer killed between fopen and remove leaves <file>.tmp.<pid> behind. It is self-identifying and harmless (the name says
which pid died), and a npu-pack-side sweep could collect them if they ever accumulate.