[bong-water-water-bong]

Fixes #1985.

Qwen3-1.7B "imported with 0 gaps yesterday and refuses with 280 today" was not a change in the reader. It is a race between two imports of the same pack, and it reproduces on a clean build of main with one binary.

What was wrong

A 1.9 pack's Token .fconst is rewritten into a canonical copy before it is read. That copy lives at a path every import of the same pack on the box shares (~/.cache/1bit/fconst19/<name>), it was written in place (std::ofstream(work, trunc)), and it is reused when its length is right. So:

1. import A starts writing the copy;
2. import B starts, sees a file shorter than expected, decides to rewrite, and truncates it;
3. A finishes its write and reads its own copy while B is refilling it.

A then reads a layout out of a partly written file and reports it, with exit 0.

The fix

write_canonical_copy() writes under a temporary name in the same directory and renames it into place. A reader finds no copy or a complete one; two writers each install the same bytes.

The temporary name is 1bit-fconst19tmp-<pid>-<n>-<name>. It starts with the scratch stem, so the existing 6-hour rule removes one a killed run left behind. It does not start with any pack's 1bit-fconst19v<n>-... prefix, so another import's sweep cannot delete a copy that is still being written.

Measured on strixhalo

Qwen3-1.7B_rai_1.9.0_npu (2,660,258,221 bytes), an empty private scratch directory per trial (ONEBIT_FCONST_SCRATCH), three 1bit npu-pack info <pack> started a fixed interval apart. "Normal" is exit 0, incomplete 0, qk_norm true, tiles_measured 64.

binary	stagger 0.10 s	stagger 0.15 s	stagger 0.20 s
main 233fcad4, clean build	24/24 normal	20/24 normal, 4 wrong	24/24 normal
this change	48/48 normal	48/48 normal	48/48 normal

A wrong run on main (always the first process of its trio), exit 0:

incomplete 280    attention_payload {qk_norm false, tiles_measured 0, o_proj_k 0}
model.layers.0.self_attn.q_proj.weight: layer blocks not located: the 1.9 header gives stride 27931456,
but the epsilon anchors repeat at 0 in-block offset(s), and a layer block has 1 (2-header) or 3 (4-header) ...

280 gaps, qk_norm true to false and tiles_measured 64 to 0 are the issue's three numbers. The note text is not the issue's ("attention payload holds 64 tiles; q/k/v/o at 64 rows each need 96"). Which wrong layout is read depends on how much of the file had been refilled, so I expect that text from a different interleaving, but I did not catch it and do not claim it.

The 144 runs with the fix were made at the commit before the final rebase onto 468d029f; the rebase touched none of this PR's lines. The unit test below was run at the pushed head.

Tests

tests/npu_pack_import_test.cpp, test_write_canonical_copy(), 11 arms, no pack needed:

• the bytes written are the bytes at the path, and no temporary file stays;
• a second write replaces the copy; a write into a missing directory throws and leaves nothing;
• the temporary name is beside the copy, new on each call, outside the pack-prefix sweep (a fresh one survives evict_stale_canonical_copies()), inside the age sweep (a 2-hour-old one is removed);
• the race: six threads each do what an import does (reuse the copy if its length is right, otherwise write it, then read the path) while one of them removes the copy every fourth round. Any copy that opens must be whole.

At the head 9e45031:

$ ./build/npu_pack_import_test tests/golden
  atomic canonical copy: 11 arms (bytes, replace, failed write, temporary name x4, 38 racing writes / 143 reads, no leftovers)
all npu-pack checks passed -- pack leg SKIPPED, no pack directory given          exit 0
$ ctest -R npu_pack_import        1/1 pass
$ tools/copyright.py --check      exit 0

It passed five runs in a row before the rebase. Red against the old behaviour (the function body changed back to an in-place write, nothing else):

FAIL atomic copy: no import ever reads a copy another import is still writing (engine#1985)
  ... 77 racing writes / 97 reads ...                                             exit 1

What this does not do

• Old binaries still write in place. A box where an older 1bit and a new one import the same pack at the same moment can still show the failure, until the old binaries are gone. The docs say so.
• A wrong reading still exits 0. npu-pack info reports 280 refused tensors and returns success by design; this PR removes the cause it found, not that convention.
• npu-pack: a short epsilon vote no longer wins over the anchor residues (Qwen3-1.7B imports again, #1985) #1986 and npu-pack: the in-block header cut scales with the blocks in the window (refs #1985) — fixture still owed #2021 change how a short epsilon vote is handled. Neither is needed to explain npu-pack: Qwen3-1.7B imported with 0 gaps yesterday and refuses with 280 today -- the 4-header block is read as 2-header #1985 as measured here. They may still be wanted as hardening; that is a separate decision from closing this issue.
• npu-pack: refuse a one-header vote on a pack that declares per-head q/k gains, naming both witnesses (engine#1985) #1995's refusal stays. It fires on exactly the kind of inconsistent reading this race produced, which is the right behaviour if a partly written file is ever read again.

Docs: docs/npu.md, "Where a 1.9 pack's rewritten .fconst goes".

🤖 Generated with Claude Code

[bong-water-water-bong]

ready for review at 9e45031

[bong-water-water-bong]

VERDICT at 9e45031: READY — a shared cache file written in place, and the "reader changed" story was wrong

This is the best kind of resolution of a regression report: the reader did not change, and the fix is where the assumption was. "Qwen3-1.7B 'imported with 0 gaps yesterday and refuses with 280 today' was not a change in the reader. It is a race between two imports of the same pack, and it reproduces on a clean build of main with one binary." Two of tonight's PRs (#1986, #1995) were treating that symptom as a heuristic regression in the reader — and this says it is a concurrency bug that happens to look like one.

The mechanism is stated as an interleaving, which is what makes it checkable:

1. import A starts writing the copy;
2. import B starts, sees a file shorter than expected, decides to rewrite, and truncates it;
3. A finishes its write and reads its own copy while B is refilling it — so A parses a layout out of a partly written file and reports it.

Three properties made that possible and each is independently fixable: the path is shared (~/.cache/1bit/fconst19/<name>), the write is in place (std::ofstream(work, trunc)), and reuse is decided by length only, which is exactly the test that a truncated file passes partway through a rewrite.

Why this one matters more than its size: it explains a class of "intermittent" findings that lanes will otherwise chase for hours — and it is the same family as #1970 (two cold serves sharing <path>.tmp) and #1907/#1952 (shared scratch with a sweep that did not distinguish its own writes). Shared state under ~/.cache, written in place, with a validity test that cannot see a partial write. Three of tonight's bugs are that sentence.

Four asks, in order:

1. Write-then-rename, always — a temp name carrying the writer's pid (the npu: two cold serves of one pack both finish writing the head image (per-process temp name) (engine#1716 follow-up) #1970 fix) and rename() into place. That makes a reader either see the old complete file or the new complete file, never a partial one, and it is the pattern already established twice tonight.
2. Length is not validity. Even with atomic rename, a length test accepts a file written by a different geometry. If the canonical name does not include the geometry/identity, two packs with the same name and coincidentally equal lengths collide — worth confirming the name carries what the key carries (the npu: the head image's cache key, because the same container packs differently under different kernel sets (engine#1716) #1832/npu: a Q4NX cache hit counts as a use, and the cache bound cannot wrap (Refs #1956) #1988 lesson).
3. Say what a reader does when it loses the race now: with rename, the loser re-reads the winner's file — correct, but it means the "rewrite" decision becomes advisory. Worth one line saying the loser must re-open after the rename rather than continuing to read its own now-unlinked handle.
4. A test with two real processes (fork, as npu: two cold serves of one pack both finish writing the head image (per-process temp name) (engine#1716 follow-up) #1970 does), because a same-process test cannot produce this interleaving — one open() sequence per process is the preconditions.

And the correction is worth recording in the issue: if #1986/#1995 landed a heuristic change on the strength of this symptom, their comments should point here, or the next reader will believe the reader-side story. Two PRs and one issue are currently describing the same 280-gap refusal with two different causes.

[bong-water-water-bong]

VERDICT: READY at 9e45031 — Two imports of one 1.9 pack no longer read each other's halves (+45/+10, docs +9, tests): a scratch name that collides is a race whose result depends on timing, which is the worst kind — the fix is a per-import name, and the test is what keeps it that way.

[bong-water-water-bong]

VERDICT: READY at 9e45031 — I built the writer from this head and drove it, and the four properties the fix depends on all hold.

[PASS] the bytes arrive at the canonical path
[PASS] no temporary file stays after a successful write
[PASS] each call gets its own temporary name
[PASS] the temp name starts with the scratch stem, so the AGE sweep reclaims an abandoned one
[PASS] and it does NOT start with an ident_prefix, so another import's SIBLING sweep cannot take a live temp
[PASS] the temp sits in the same directory, where rename is atomic
[PASS] a second write replaces the copy whole

The fourth and fifth arms are the design, and they are the part a reader will not think of: 1bit-fconst19tmp-<pid>-<n>-<name> starts with the scratch stem, so the 6-hour age sweep reclaims a temp left by a killed run — and it does not start with an ident_prefix, so a concurrent import's sibling sweep cannot delete a temp someone else is writing. A name that satisfied only one of those would trade a leak for a live deletion, which is worse.

And the mechanism is measured rather than argued: "three imports 0.15 s apart: 4 of 24 runs read '0 in-block offsets', refused all 280 attention tensors and exited 0" → 0 of 144 with the rename. The exit-0 part is what makes this worth a PR of its own: a concurrent import did not fail, it succeeded with a different pack.

Three details I checked in the code:

• the temp is created in p.parent_path() — rename is only atomic inside one filesystem, and the comment says so;
• a failed fs::rename removes the temp and throws with the path and the OS message, rather than leaving a stray file whose age sweep would later delete the evidence;
• the serial is a static std::atomic<unsigned> combined with getpid(), so two threads in one process and two processes on the box cannot collide.

One nit: the arms in the PR's test cover the race with threads in one process, which exercises the atomic serial; the getpid() half (two processes) is only exercised by the measured 24-run experiment, not by a test. That is a reasonable place to stop — a two-process arm in a unit test is flaky by construction — but the comment should say the process half is validated by the measurement, so a future reader does not think the arms cover it.

My own correction, since it is the third time tonight: my first run failed "the temp name starts with the scratch stem" — my assertion ran rfind(stem, 0) on the full path, which starts with the directory. The function is right; my instrument was wrong by one path component, and I fixed the assertion rather than the claim.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1985 - Partially compliant Compliant requirements: Fix the race condition by writing canonical copies under temporary names and renaming into place Ensure Qwen3 models import correctly with 0 gaps Prevent the regression in header classification Non-compliant requirements: None Requires further human verification: Verification that the fix works on actual hardware (strixhalo) with the specific Qwen3-1.7B pack Confirmation that the 280 refused tensors issue is resolved

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Race Condition Fix The fix addresses a race condition where concurrent imports of the same 1.9 pack could corrupt the canonical copy. Previously, the canonical copy was written in place, which allowed a second import to truncate a file that the first import was still writing. This led to the first import reading a partially written file, causing incorrect header classification and resulting in 280 refused tensors for Qwen3 models. The fix now writes under a temporary name and renames into place, ensuring atomicity. void write_canonical_copy(const std::string& work, const uint8_t* data, size_t len) { const std::string tmp = canonical_copy_temp_name(work); { std::ofstream os(tmp, std::ios::binary | std::ios::trunc); if (!os) throw std::runtime_error(tmp + ": cannot write the canonical copy of the 1.9 .fconst"); os.write(reinterpret_cast<const char*>(data), std::streamsize(len)); os.flush(); if (!os) { os.close(); std::error_code rm; fs::remove(tmp, rm); throw std::runtime_error(tmp + ": short write of the canonical copy of the 1.9 .fconst"); } } std::error_code ec; fs::rename(tmp, work, ec); if (ec) { std::error_code rm; fs::remove(tmp, rm); throw std::runtime_error(work + ": cannot move the canonical copy of the 1.9 .fconst into place (" + ec.message() + ")"); } } Test Coverage for Race Condition The test test_write_canonical_copy provides comprehensive coverage for the race condition fix. It verifies that the canonical copy is written atomically, that temporary files are properly handled, and that concurrent access does not result in partial reads. The test also checks that the temporary name is unique, does not conflict with pack prefixes, and is properly cleaned up by the age sweep rule. void test_write_canonical_copy() { const fs::path dir = fs::temp_directory_path() / ("1bit-atomic-" + std::to_string(::getpid())); std::error_code ec; fs::remove_all(dir, ec); fs::create_directories(dir); const std::string prefix = "1bit-fconst19v1-7-2048-16-28-"; const std::string name = prefix + "957724736-1234.fconst"; const std::string work = (dir / name).string(); const size_t n = size_t(4) << 20; std::vector<uint8_t> body(n); for (size_t i = 0; i < n; ++i) body[i] = uint8_t((i * 2654435761u) >> 24) | 1; // never 0: a refilled hole reads as 0 auto same = [&](const std::string& f) { std::ifstream is(f, std::ios::binary); if (!is) return false; std::vector<uint8_t> got(n + 1); is.read(reinterpret_cast<char*>(got.data()), std::streamsize(got.size())); return size_t(is.gcount()) == n && std::memcmp(got.data(), body.data(), n) == 0; }; auto entries = [&] { int k = 0; for (const auto& e : fs::directory_iterator(dir)) { (void)e; ++k; } return k; }; pi::write_canonical_copy(work, body.data(), n); check(same(work), "atomic copy: the bytes written are the bytes at the path"); check(entries() == 1, "atomic copy: no temporary file is left beside the copy"); std::vector<uint8_t> other(body); other[n / 2] ^= 0xff; pi::write_canonical_copy(work, other.data(), n); check(!same(work) && entries() == 1, "atomic copy: a second write replaces the copy, again leaving one file"); pi::write_canonical_copy(work, body.data(), n); bool threw = false; try { pi::write_canonical_copy((dir / "no-such-dir" / name).string(), body.data(), n); } catch (const std::runtime_error&) { threw = true; } check(threw && entries() == 1, "atomic copy: a copy that cannot be written throws and leaves nothing behind"); // the temporary name: beside the copy, not the copy, unique per call, invisible to the prefix sweep, visible to the age sweep const std::string t1 = pi::canonical_copy_temp_name(work), t2 = pi::canonical_copy_temp_name(work); const std::string b1 = fs::path(t1).filename().string(); check(fs::path(t1).parent_path() == fs::path(work).parent_path(), "atomic copy: the temporary file is in the copy's directory"); check(t1 != work && t1 != t2, "atomic copy: the temporary name is not the copy's and is new on every call"); check(b1.compare(0, prefix.size(), prefix) != 0 && b1.compare(0, 13, "1bit-fconst19") == 0, "atomic copy: the temporary name starts with the scratch stem and not with the pack's prefix"); { std::ofstream o(t1, std::ios::binary); o << "being written"; } pi::evict_stale_canonical_copies(dir.string(), name, prefix, std::string(), 3600); check(fs::exists(t1) && fs::exists(work), "atomic copy: another import's sweep does not remove a copy that is being written"); fs::last_write_time(t1, fs::file_time_type::clock::now() - std::chrono::hours(2)); pi::evict_stale_canonical_copies(dir.string(), name, prefix, std::string(), 3600); check(!fs::exists(t1) && fs::exists(work), "atomic copy: a temporary file a killed run left is removed by the age rule"); // the race: every thread is an import. It reuses a complete-length copy or writes one, then reads the path, as // canonical_token_fconst()'s caller does. One thread also removes the copy now and then, so the cycle restarts. fs::remove(work, ec); std::atomic<int> bad{0}, read_ok{0}, wrote{0}; auto importer = [&](int id) { for (int r = 0; r < 24; ++r) { if (!pi::canonical_copy_reusable(work, int64_t(n))) { try { pi::write_canonical_copy(work, body.data(), n); ++wrote; } catch (const std::runtime_error&) { ++bad; } } std::error_code e2; // An open that fails is the remover having been there, not a bad copy; a copy that OPENS must be whole. if (std::ifstream(work, std::ios::binary).good()) { std::ifstream is(work, std::ios::binary); std::vector<uint8_t> got(n + 1); if (is) { is.read(reinterpret_cast<char*>(got.data()), std::streamsize(got.size())); if (size_t(is.gcount()) == n && std::memcmp(got.data(), body.data(), n) == 0) ++read_ok; else ++bad; } } if (id == 0 && r % 4 == 3) fs::remove(work, e2); } }; std::vector<std::thread> pool; for (int i = 0; i < 6; ++i) pool.emplace_back(importer, i); for (auto& t : pool) t.join(); check(bad.load() == 0, "atomic copy: no import ever reads a copy another import is still writing (engine#1985)"); check(read_ok.load() > 0 && wrote.load() > 1, "atomic copy: the race arm really wrote more than once and read copies back"); int leftover = 0; for (const auto& e : fs::directory_iterator(dir)) if (e.path().filename().string() != name) ++leftover; check(leftover == 0, "atomic copy: the race leaves no temporary files"); fs::remove_all(dir, ec); std::printf(" atomic canonical copy: 11 arms (bytes, replace, failed write, temporary name x4, %d racing writes / %d reads, no leftovers)\n", wrote.load(), read_ok.load()); }

[bong-water-water-bong]

VERDICT at 78627ff: READY

The new head is a merge of main, and the PR's own diff is identical to 9e45031 (verdicted READY). A three-way merge onto current main is clean. I checked it at this head (worktree at 78627ff):

• npu_pack_import_test tests/golden: all npu-pack checks pass, including "atomic canonical copy: 11 arms (bytes, replace, failed write, temporary name x4, … racing writes / 144 reads, no leftovers)".
• The racing arm can fail. With write_canonical_copy writing in place again (temp name = work), the suite fails "atomic copy: no import ever reads a copy another import is still writing (engine#1985)". Restored, it passes.
• Read: the copy is written under 1bit-fconst19tmp-<pid>-<n>-<name> in the same directory (so the rename is atomic) and renamed onto work. A short write or a failed rename removes the temp file and throws. The tmp- infix keeps it out of any pack's 1bit-fconst19v<n>- prefix, so another import's sweep can't delete it mid-write, while the 6 h age rule still removes one a killed run left.

What I could not reproduce: the end-to-end race on main. Two binaries built from main d4dc5e7 and this head, Qwen3-1.7B_rai_1.9.0_npu, three npu-pack info per trio on a private ONEBIT_FCONST_SCRATCH, under thermal-run: main was 72/72 normal across staggers 0.15 / 0.3 / 0.6 / 1.0 / 1.5 s (24 at 0.15 s), and this head 24/24. The window is timing-sensitive (your table shows 0.10 and 0.20 s clean on main too), and the thermal-run scope probably shifts it. So the failing case is your measurement, not mine. It doesn't need to be: the fix is right by construction and the unit arm races writers against readers and catches the in-place form.

This also accounts for what I saw on #1986: I couldn't make Qwen3-1.7B misread from any clean build, and the canonical copies of two different pack directories in the shared cache were byte-identical. One shared path plus an in-place write is exactly the mechanism. #1986's "short vote" premise was this race, so #1986 can close in favour of this one.