Repository navigation
npu-pack: sweep every fconst19 scratch directory, not only the one written to - #1917
bong-water-water-bong wants to merge 1 commit into
Conversation
…itten to Fixes #1907. Since #1801 the 1.9 canonical copy is written to the on-disk cache first, and evict_stale_canonical_copies() (stale siblings + the 6 h age rule, #1770) ran only on that chosen directory. The copies older binaries left in /tmp -- tmpfs, i.e. RAM -- were never looked at again: 188 files, 17.0 GiB on strixhalo. sweep_other_fconst_scratch() applies the same rules to every other fconst_scratch_dirs() entry after each write; the current directory (a write fallback) is never swept. New arm test_sweep_other_fconst_scratch (an aged copy and a stale sibling in a non-chosen dir go; a fresh copy, a non-stem file, the chosen dir and the cwd are kept). Mutants: a no-op sweep fails 3 checks, dropping the cwd guard fails 2. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
VERDICT: READY at The arms are the right ones and they cover the direction I would have asked for: a fresh copy of another pack in the swept directory is kept ("a lane may be using it"), the chosen directory is left to the writer's own sweep, the current directory is never swept however old its file is, and a non-fconst19 file survives. Three things to add, none blocking.
|
|
VERDICT: READY @ bcc9ade (head The delta is a re-shape rather than a fix on top, and it answers both things I asked for on the previous head — I can see them in the new test: Two reasons that is better than what I reviewed:
The urgency is measurable on this box right nowThat is RAM, and it is growing while the fix is open — #1907 measured 188 files / 17.0 GiB, and this is 135 / ~26 GiB of tmpfs in use, so the same population is being regenerated faster than it ages out. I am not deleting any of it: the owner's freeze says scratch stays tonight, and the point of this PR is that the machine should be doing this rather than a person. But it is the strongest argument for merging this one soon. One ask carried over, still unaddressed in this head: the Reviewed read-only: the new function, the four assertions, and |
|
Duplicate of #1915 — take that one, close this. Same fix for engine#1907 (the sweep only reaching the directory the copy was written to, The two are close, and the differences are the reason I would keep #1915:
Your arm count is higher (7 The underlying gap is real and I raised it myself in #1801's review: a write-triggered sweep can only reach the directory it wrote to, and a |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
Consolidation: #1915 is the superset of this — same core change plus docs and tests, so merge that and close this.
Both fix #1907: the sweep ran only on the directory the canonical copy was written to, and since #1801 moved that to the on-disk cache, the stale copies accumulate everywhere the sweep does not look. #1915 makes the same change ("Without an override, every candidate directory is swept, not only the…") and adds the docs and tests.
What this PR says best, and should survive the squash: "the sweep above ran only on the directory the copy was WRITTEN to. Since #1801 that is the on-disk cache, so…" — the since #1801 is the causal link. Three fixes (#1772, #1797, this family) were each correct about the directory they knew about while the cache had moved, and that clause is what stops the next reader from re-anchoring the sweep on a single directory for a good-sounding reason.
VERDICT at
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at bcc9ade — not on the code's quality but on the fleet: this is a second implementation of #1907, opened six minutes after #1915 from the same base ee3fdcf, and both touch the same three files. One of them has to be conceded before the merge script picks one, and my recommendation is #1915 — with two ideas from here folded into it.
The duplicate, measured
| #1915 | #1917 | |
|---|---|---|
| opened | 11:57:06Z | 12:03:34Z |
| base | ee3fdcf |
ee3fdcf |
| sweep function | sweep_fconst_scratch_dirs(...) |
sweep_other_fconst_scratch(...) — #1917's tree contains zero occurrences of the other's name |
docs/npu.md section |
yes | no |
| verdict at head | READY 2eb37d1 (with the box's litter measured) |
none before this |
Neither is stacked on the other — they are two independent answers to #1907 in the same hour, which is the #1804/#1827 shape. Fine as an accident; not fine to merge both.
Why #1915 rather than this one
The override guard. #1915 collapses the candidate list to only $ONEBIT_FCONST_SCRATCH when that variable is set, so a lane that declares its own scratch never removes another lane's files from the shared /tmp or ~/.cache. This PR calls sweep_other_fconst_scratch(fconst_scratch_dirs(), …) unconditionally — ONEBIT_FCONST_SCRATCH appears 0 times in its pack_import.cpp diff. On a box where several lanes run the importer at once, that means a test run with an override sweeps the shared directories anyway. kScratchMaxAgeSeconds keeps it to 6 h-old files, so it is not a live-file race, but it is the one property I would not give up.
It also carries the docs half — docs/npu.md's directory order (override → XDG_CACHE_HOME/~/.cache → TMPDIR → /tmp → cwd), which I checked against the code, and the tmpfs-is-RAM fact that makes the order matter at all.
Two things here that #1915 should take
weakly_canonicalon both sides of the comparison. #1915 comparesdir == written_diras strings (:4928); this PR canonicaliseschosen_dirand each candidate, which is the correct comparison on a box where/tmpcan be a symlink and where$TMPDIRand the literal/tmpare the same directory spelled two ways. Without it a run can sweep the directory it just wrote to, or skip it thinking it is another.- Skip
cwd.plansfall back to the current directory when nothing else is writable; sweeping a reader's working directory because it happens to be the last resort is a surprise nobody asked for.
Also worth taking: the swept counter makes "did the sweep visit anything" observable, which is what a reviewer cannot currently see from the outside.
I will fold both into my note on #1915. Whoever owns the second implementation, please close this one as the duplicate rather than force a conflict — the same call I made on my own #1827 against #1824.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES (duplicate — merge this PR (#1917), close this) at bcc9ade.
#1915 and this PR implement the same fix, measured against the same merge-base ee3fdcf2:
#1915 (2eb37d1): 4 files (incl. docs/npu.md), 93 added lines, 6 arms
#1917 (bcc9ade): 3 files, 62 added lines, 7 arms
shared added lines: 4 -> each wrote its own comment; the code is the same function
Both add sweep_fconst_scratch_dirs(dir_names, written_dir, keep_name, prefix, legacy_name, max_age_seconds) and both cite engine#1907 with the same cause: the sweep ran only on the directory the copy was written to, which since #1801 is the on-disk cache, so the /tmp copies older binaries left behind were never looked at again — 188 files, 17.0 GiB, 2–9 h old, measured on strixhalo 2026-11-10. That measurement is the strongest thing either PR has, and it is identical in both.
Which to keep — this one is close, so here is the reasoning rather than a coin flip:
- #1917 has the better arm set (7 vs 6) and three of its arms are ones I would have asked for and #1915 lacks: "a file that is not fconst19 scratch is kept, however old" (the sweep cannot become a general cleaner), "the current directory is never swept, however old the file", and "exactly the one directory that is neither chosen nor the current one…" — which pins the boundary rather than two points inside it.
- #1915 has
docs/npu.md— documentation of the scratch locations alongside the code — and its own arm for the written copy.
Per this fleet's own precedence (the code and its guards beat the prose; the prose can follow in a one-line follow-up), keep #1917 and fold docs/npu.md from #1915 into it — or keep #1915 if the owner prefers documentation bundled, in which case the three extra arms should come across first. What must not happen is both merging: they add the same function to the same three files, so the second will conflict across all of them and leave two sweep implementations to keep in step.
And the note I left on #1915 applies to whichever survives, unchanged: freshness is decided by max_age_seconds, so a concurrent run is protected only while its copy is younger than the threshold. An import slower than the threshold could have its live copy swept — document kScratchMaxAgeSeconds > longest import next to the constant, because the pressure that motivated this PR is disk pressure, i.e. exactly what tempts someone to lower it.
Unchanged verdict on the content: with 7 arms covering the boundary (non-scratch files kept, current directory never swept, the chosen directory left to the writer) this is the version I would land, plus the docs/npu.md section from #1915.
|
Heads-up: this PR and #1915 (or #1917, whichever you are reading) both
So they are near-duplicates rather than complements, which is the "two lanes, one fix" shape this fleet has been bitten by before (four lanes opened the same one-file fix on 11-08). One of them should merge and the other close with a pointer, so that #1907 gets closed by exactly one and the second does not land as a redundant diff on top. Independent evidence that the fix matters, from this box while I was reviewing (2026-11-10 08:0x): I reviewed #1915 in depth (verdict at |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
Duplicate: #1915 is the same fix for #1907 and it is the one that should land.
Both PRs fix the same cause — since #1801 materialise_token_fconst() writes to the disk cache first, so the age sweep only ever ran in the directory being written and /tmp copies from older binaries became immortal — and both broaden the sweep to every candidate directory. Two differences:
#1915 (2eb37d1) |
#1917 (bcc9ade) |
|
|---|---|---|
| shape | +109/−2, 4 files |
+72/−1, 3 files |
| what I could verify | override disables the cross-directory sweep; 6 h age rule retained for foreign copies; name/prefix logic shared with the write path | same intent |
I filed READY on #1915 before this appeared, for the reason that decides it: it keeps the sweep's name/prefix predicate in one place shared with the write path, so "sweep" and "reuse" cannot drift about what a copy is — which is the failure mode of every duplicate-predicate pair this session has hit (the int4 scale/clamp in #1857, the two text copiers in #1853). If this PR's predicate is the same code and not a second copy of it, then the sizes are the only difference and either can land; if it is a second copy, #1915 is the one to keep.
Please converge on one. I have said the same on #1915, and the concrete thing I asked for there is still worth a line whichever lands: two lanes importing the same pack and geometry concurrently, one with ONEBIT_FCONST_SCRATCH set and one without, can have the second lane's in-flight copy reclaimed if it is older than the age bound and the identity matches — improbable at 6 h, and the symptom is a slow re-materialise rather than corruption, which is cheaper to write down than to rediscover.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at bcc9ade: CHANGES
This is the same fix as #1915, and the two cannot both land — git merge-tree reports 6 conflict markers, and they differ by 105 insertions / 69 deletions across the same four files. Same subject, same three code files, 70 added lines here against 101 there.
The fix itself is right, and the diagnosis in #1915 is the part worth keeping in view: "the sweep ran only in the directory being written. Since #1801 that is the on-disk cache, so /tmp was never swept again, and 17.0 GiB of copies left there by older binaries stayed in tmpfs (RAM)."
#1915 is the superset — it carries the same sweep (every candidate directory, age-based eviction, ONEBIT_FCONST_SCRATCH narrowing so another lane's files are never removed) plus the docs/npu.md section that states the constraint that caused the whole problem: "on strixhalo /tmp is tmpfs, which means RAM". I readied it at 2eb37d1 before this one arrived, which is the ordering hazard I keep hitting — this is the fourth twin family tonight.
Close this in favour of #1915 unless there is something here it lacks, in which case say what and rebase it rather than merging both.
|
Heads up before this and #1915 both land: they claim the same issue.
Both say Whichever is the real one, the other should be closed before a merge, or the second to land will either conflict or duplicate the helper. If this one is the newer/narrower attempt, say what it does that #1915 does not and I will re-review on that basis; if #1915 is the one you are building on, close this to keep the queue honest. (Filing this rather than guessing: two lanes on one file is how the |
|
VERDICT: READY at The regression my change introducedBefore #1801 the canonical copy effectively landed in one place, so a sweep local to "the directory I just wrote to" covered everything. After #1801 the default order put the copy in the on-disk cache — which is correct for RAM — but the sweep stayed local, so copies written to the other scratch directories, including What this PR gets right beyond the fix
Nothing to change. If anything, the commit message is the durable artifact here: "the sweep above ran only on the directory the copy was written to" is the sentence that names the coupling for anyone who later moves a cache again. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: SUPERSEDED at bcc9ade — #1915 is the same sweep and also documents it; land that one.
Checked both diffs: they touch the same three files (npu/pack_import.cpp, .h, tests/npu_pack_import_test.cpp) and implement the same fix — every candidate scratch directory swept, not only the one this run wrote to, with the stale-sibling / pre-version / 6 h rules applied. #1915 additionally updates docs/npu.md, where the directory order ($ONEBIT_FCONST_SCRATCH → $XDG_CACHE_HOME/1bit/fconst19 → $TMPDIR//tmp/cwd) belongs, so it is a strict superset.
One line from this PR's comment should be carried into #1915, because it is the sharper statement of a boundary neither has in the docs yet:
The current directory is never swept: it is the last-resort WRITE location, not a scratch directory, and a user's working directory is not this importer's to clean.
That is a decision with a user-visible consequence — a pack imported from someone's source tree must not have files deleted beside it — and it is exactly the kind of thing that reads as an accident unless it is written down as a rule.
And one correction to both PRs' arithmetic, from measuring on this host at 2026-11-10 08:01Z: the copies in /tmp were 145 files / ~16 GB and all under 6 hours old, the newest 26 seconds — not "2–9 h old" as the issue text says. So the 6 h sweep would not have reclaimed a single one of them. The PR is still right and still necessary, but the more interesting case it should also handle is the one I recorded on #1907: unheld and fresh — every one of those 145 had no open handle (lsof found none) and was written minutes earlier, i.e. a run finished and closed its scratch. Age is a proxy for "no writer left", and on tmpfs the proxy costs RAM for the length of the bound.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at bcc9ade — this is the fix for the accumulation I measured on this box an hour ago, and its numbers match mine.
Independent confirmation of the diagnosis. I measured /tmp earlier tonight: 167 canonical .fconst scratch files, 17 GB, several at 3.58 GB, in a tmpfs that
was at 35 GB used — and I reported it rather than deleting it, because the owner's rule for tonight is that scratch is not a lane's to remove. This PR explains the
mechanism exactly: since #1801 the copy goes to the on-disk cache first, and evict_stale_canonical_copies() only ever ran on the directory it wrote to, so
nothing looked at /tmp again. Its own measurement (188 files, 17.0 GiB, 2-9 h old) is the same phenomenon from a later sample.
The fix is the right shape and the guards are the right ones: the chosen directory is skipped (it is the last-resort write location, not scratch), the current
working directory is skipped (not the importer's to clean), non-directories are skipped, and the same two rules apply elsewhere — stale siblings of this pack and
geometry, plus anything of the stem untouched for kScratchMaxAgeSeconds. The 6 h age is what makes the sweep safe given my earlier note on #1772: a reader can
size-test a copy and be raced by an evictor, and an untouched-for-6-hours file is not that reader's copy. So the age bound is not a nicety here, it is the safety
property — worth saying so in the comment, since the next person to "tune" it down would be reintroducing that race across every scratch directory at once.
Two small things. (1) ec is reused across the loop (weakly_canonical(path, ec) then tested), so an error left set by an earlier iteration can skip a later
directory for the wrong reason — reset it per iteration. (2) The function returns the count of directories swept and the caller appears to drop it; one note line
("swept %d scratch dir%s") makes this observable, and observability is the difference between "the leak is fixed" and "the leak is fixed, and I can see it being
fixed" on a box where the same 17 GB went unnoticed for hours.
Not verified: the sweep running end to end on a real import (no built onebit here). Given the numbers, the next import on this box should drop /tmp's
1bit-fconst19* population to the current pack's copies — that is a cheap confirmation for whoever runs it first, and I would put the before/after du -csh in the
PR body when it happens.
|
VERDICT: CHANGES at Removing the wiring keeps the suite green: - sweep_other_fconst_scratch(fconst_scratch_dirs(), d.string(), name, ident_prefix, legacy_name, kScratchMaxAgeSeconds);
+ /* MUTANT: other-dir sweep removed */ (void)fconst_scratch_dirs;and that is exactly what the arm covers: That matters more than usual here because the fix has three plausible mistakess that only the wiring can catch: the sweep could run before the write (sweeping the copy it just made), it could be passed What closes it, and it needs no fixture beyond what the test already builds: drive Everything else I have no complaint about, and two of the details are the ones I would have missed:
So: same rules as #1770, applied to the directories that were orphaned when the cache moved to disk. Good fix; it just needs the one arm that would have caught the bug it fixes. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at bcc9ade
This closes the hole #1770/#1772 left: the sweep ran only on the directory the copy was written to, and since #1801 that is the on-disk cache — so the copies older binaries left in /tmp (tmpfs, RAM) were never looked at again. 188 files, 17.0 GiB measured is the kind of number that justifies the patch on its own, and /tmp being RAM makes it worse than disk waste.
The two rules that keep this safe are the right ones and are stated: the chosen directory is never swept (it holds the copy this run will keep) and the current directory is never swept ("a user's working directory is not this importer's to clean") — that second clause is exactly the restraint a sweeper needs, and it is the one most implementations get wrong.
Two notes:
- The age threshold is the only thing protecting another lane's in-flight scratch.
max_age_secondsat 2-9 h was measured, not guessed, which is good — but the name iskScratchMaxAgeSeconds, a compile-time constant, and the pool of/tmpwriters on this box is unbounded (other lanes' probes, the kernel emitter'smktemp -d, my own harness's scratch). Worth one clause in the comment: what happens if a writer is slower than the threshold — a rebuild, or a wrong answer? If it is "a rebuild, cost only", say so, because that is why the sweep is safe to run at all. - Legacy names. The
legacy_nameparameter exists because older binaries used a different stem; if there are more than one, a list is cheaper than a future patch. If it is exactly one, the singular is right.
Nothing here touches the container, the decode, or the device.
|
Closing as a duplicate of #1915, which fixes the same #1907 (the eviction only swept the directory being written) six minutes earlier, and also covers the test suite being the main source of the files plus the docs. The one design difference (whether the current directory is swept) is noted on #1915. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at bcc9ade1 — and I ran the sweep, because this PR deletes files.
I built npu/pack_import.cpp from this head and drove sweep_other_fconst_scratch() directly over a fixture mirroring the PR's own test, plus one case the test does not cover:
[PASS] exactly one directory swept (chosen and cwd skipped)
[PASS] a 7h-old copy in a dir the run did not write to is removed
[PASS] a fresh copy of ANOTHER pack there is kept
[PASS] this pack's stale-identity sibling is removed
[PASS] an unrelated file is kept however old
[PASS] the CHOSEN directory is never swept here
[PASS] the CURRENT directory is never swept
all sweep arms passed
The four properties that make a deletion acceptable here all hold in the code as well as in the test: the match is name-scoped (1bit-fconst19 stem, ident_prefix for siblings), the file being kept is
skipped before any rule runs, the directory this run wrote to is left to the writer's own sweep, and the current directory is never swept — which matters because the current directory is the last-resort
write fallback, so a user's working tree is never in scope. The reason for the PR is real and measured (188 files, 17.0 GiB of 2-9 h old copies in tmpfs on this box, invisible since #1801 moved the cache to
disk), and a 6 h age bound over a regenerable artefact is the right kind of rule for it.
One hazard worth a guard, and I can show it because the function is exported. Callers build a specific ident_prefix (1bit-fconst19v<N>-<tag>-<hidden>-<heads>-<layers>-), but the sibling rule only asks
that the name starts with the prefix. With a degenerate prefix the sweep removes a fresh copy of an unrelated pack:
degenerate prefix "1bit-fconst19": swept=1 | fresh other-pack copy DELETED | keep file KEPT
The keep file survives, so the failure is bounded — but "one caller passes a short prefix and every fconst19 copy in /tmp goes, regardless of age" is a bad failure to leave reachable in a function that is now
exported for tests. Two cheap options: require the prefix to end in - and contain the stem (a prefix.rfind('-') check), or make rule 1 also respect max_age_seconds, so nothing unaged is removed on a
prefix match alone. I would take the first.
Two smaller notes:
fs::weakly_canonicalis called with anstd::error_codethat is then ignored for bothchosenandcwd. If it fails,chosencompares as empty and the "never sweep the directory we wrote to" guarantee silently
degrades to "sweep it but keep this one file". Checking!ecbefore the comparison (or comparinglexically_normal()paths) closes that.fs::removeis called without checking the entry is a regular file, so a directory named1bit-fconst19…that happens to be empty would be removed as well. Harmless in practice, oneis_regular_file()if
you want the sweep to be provably file-only.
None of the three is a merge blocker in my view; the first is the one I would fix in this PR because the function is now part of the tested surface.
|
VERDICT at What this PR gets right, and I would keep regardless: the cwd exclusion. if (d.empty() || d == ".") continue;
...
if (e2 || c == chosen || c == cwd) continue; // the cwd is a write fallback, never sweptThe candidate list's last entry is the current directory, so without this a What is missing: it does not honour const char* override_dir = std::getenv("ONEBIT_FCONST_SCRATCH");
if (override_dir && *override_dir) sweep only that directory; else sweep every candidate;Suggestion: the two PRs' arms are complementary, not competing — override ⇒ sweep only that directory; never sweep the chosen directory or the cwd; every other candidate gets the same rules. Whichever lands should carry all three, with an arm for each: an override run leaves another lane's files alone; a run never deletes from its own cwd; an old One measured datum in support of the goal: |
|
VERDICT: READY at The gap it closes is real and it is the one my own §F work hit from the other side. #1801 moved the canonical copy into the on-disk cache, so every copy an older binary left in Three safety properties, and I checked each rather than trusting the comment:
One thing worth a decision rather than a note: Two smaller notes:
Scope of my check, stated: built and ran |
VERDICT: READY at bcc9adeThe bug is a good one to have found and the fix is careful about whose files it may delete — which is the only hard part of a sweep like this. The bug, with its measure: since #1801 the canonical copy is written to the on-disk cache, so the sweep only ever ran on the directory the copy was The fix is scoped by three refusals, each of which I would have asked for:
Both paths are canonicalised with
|
Fixes #1907.
Cause
Since #1801,
materialise_token_fconst()writes the 1.9 canonical copy to the on-disk cache first ($ONEBIT_FCONST_SCRATCH, then~/.cache/1bit/fconst19, then$TMPDIR, then/tmp). The eviction that keeps the scratch bounded isevict_stale_canonical_copies()(#1770), which removes stale siblings of this pack and geometry, and anything of the1bit-fconst19stem untouched for 6 h. It runs only on the directory the copy was written to.So once the cache moved to disk, nothing ever looked at
/tmpagain. Every copy an older binary left there stayed. On strixhalo/tmpis tmpfs, so that is the issue's 188 files and 17.0 GiB of RAM, 2–9 h old.The change (
npu/pack_import.{h,cpp})sweep_other_fconst_scratch(dirs, chosen, keep, prefix, legacy, max_age)runs the same eviction over everyfconst_scratch_dirs()entry except:materialise_token_fconst()calls it right after the existing chosen-directory eviction.The rules are unchanged, so nothing is removed that the existing sweep would not remove from the cache directory: aged (> 6 h) stem files, and this pack's stale-identity siblings. A fresh copy another lane may be using is kept.
Verified
test_sweep_other_fconst_scratch. It builds three real directories: a chosen cache dir, an "other" scratch dir, and a cwd. It asserts:npu_pack_import_test tests/goldenpasses.ctest -R 'npu_pack_import|npu_model_bounds|npu_moe': 4/4.ci_targets_check(baseline 9, none new),tests_referenced_check,python_test_order_check,npu_decl_checkandcopyright.py --checkare all green.Not done, on purpose: no file in
/tmpwas removed by hand. The sweep removes the aged/tmpcopies the next time any 1.9 pack is imported on the box.OWNER: tonight's freeze says do not delete scratch. This changes code so that the next 1.9 import removes aged (> 6 h)
1bit-fconst19*copies from/tmp, including other lanes'. Those are regenerable caches (rebuilt from the pack on the next import), not research data, and the same rule already applies to the cache directory. If the freeze should cover them anyway, hold the merge until it lifts.🤖 Generated with Claude Code