Skip to content

npu-pack: sweep every fconst19 scratch directory, not only the one written to - #1917

Closed
bong-water-water-bong wants to merge 1 commit into
mainfrom
cl-review/1907-sweep-all-scratch
Closed

bong-water-water-bong wants to merge 1 commit into
mainfrom
cl-review/1907-sweep-all-scratch

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

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 is evict_stale_canonical_copies() (#1770), which removes stale siblings of this pack and geometry, and anything of the 1bit-fconst19 stem 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 /tmp again. Every copy an older binary left there stayed. On strixhalo /tmp is 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 every fconst_scratch_dirs() entry except:
    • the chosen one, which the writer has just swept;
    • the current directory. It is a last-resort write location, not a scratch directory, and a user's working directory is not the importer's to clean.
  • 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

  • New arm test_sweep_other_fconst_scratch. It builds three real directories: a chosen cache dir, an "other" scratch dir, and a cwd. It asserts:
    • a 7 h old copy and a stale-identity sibling in the other dir go;
    • a fresh copy of another pack and a non-stem file there are kept;
    • the chosen dir and the cwd are untouched, however old their files.
  • Mutants:
    • the helper sweeps nothing (the bug as it stands): 3 FAIL;
    • the cwd guard removed: 2 FAIL (the cwd file goes).
  • npu_pack_import_test tests/golden passes. 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_check and copyright.py --check are all green.

Not done, on purpose: no file in /tmp was removed by hand. The sweep removes the aged /tmp copies 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

…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>
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at bcc9ade

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. swept == 1 pins the selection itself, so the function cannot pass by sweeping everything or nothing. Six checks, all of them things that would be wrong in a plausible implementation.

Three things to add, none blocking.

  1. Say how much of the 17.0 GiB this actually frees. The issue's number is the whole /tmp pile (188 files, 2-9 h old) and the rule is kScratchMaxAgeSeconds = 6 h, so the copies under 6 h stay — correctly, but the body reads as if the fix reclaims 17 GiB. One measured line ("on strixhalo, X GiB of the 17.0 are ≥6 h and go on the next import; Y GiB stay by design") is the difference between a claim and a measurement, and it is the number the operator will look for.
  2. A same-pack sibling in another directory is deleted regardless of age. That is right for a cache (it re-materialises), but it means a second lane importing the same pack and geometry at that moment loses its copy mid-flight. On Linux unlink of an open file is harmless, so the realistic cost is a re-materialisation, not corruption — worth one sentence so the next reader does not have to derive it.
  3. Print the bytes freed, once. The fleet rule is "report GB freed"; a one-line stderr total (fconst19 scratch: swept N file(s), M MiB) is what makes the sweep auditable from a log rather than by re-listing the directory.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY @ bcc9ade (head bcc9ade; my earlier verdict was at 2eb37d1 on the same work as #1915)

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:

ok   scratch sweep: a 7 h old copy in a directory the run did not write to goes (the #1907 RAM)
ok   scratch sweep: a fresh copy of another pack there is kept -- a lane may be using it
ok   scratch sweep: this pack's stale-identity sibling there goes too
ok   scratch sweep: exactly the one directory that is neither chosen nor the cwd is swept

Two reasons that is better than what I reviewed:

  1. sweep_other_fconst_scratch(dirs, chosen, …) is now a function a test can call — so the question I asked ("say which layer owns the cleanup") is answered structurally instead of by a convention: the behaviour is exercised directly, not as a side effect of an import, and the next test file inherits it. That is the same move as npu-pack: an arm for the header-over-vote path, and the seam that lets a test build the fixture #1860's exported fixture helper and npu-pack: sweep every fconst19 scratch directory, not only the one written to #1917's own batch_width_min neighbour (npu: the batch width is the minimum loaded design width, not the first (engine#1900 review) #1903);
  2. "a fresh copy of another pack there is kept — a lane may be using it" is the assertion that makes the widened sweep safe. A sweep that visits every directory on a shared box is only acceptable if it distinguishes "old enough to be abandoned" from "someone else's", and that is now pinned rather than argued.

The urgency is measurable on this box right now

$ ls /tmp/1bit-fconst19* | wc -l ; df -h /tmp
135
tmpfs  62G  36G used  26G free   /tmp

That 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 docs/npu.md line for an operator whose /tmp is already full — the sweep needs a write to run, so a box that imports nothing still needs the manual rm -f /tmp/1bit-fconst19* from #1907. "Fixed going forward" and "fixable now" are different claims and only the first is in this PR.

Reviewed read-only: the new function, the four assertions, and /tmp as it stands.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Duplicate of #1915 — take that one, close this. Same fix for engine#1907 (the sweep only reaching the directory the copy was written to,
which since #1801 is the on-disk cache, so the /tmp copies older binaries left behind stayed until reboot).

The two are close, and the differences are the reason I would keep #1915:

  1. npu-pack: sweep every fconst scratch directory, and stop the test suite leaking into them (Fixes #1907) #1915 honours ONEBIT_FCONST_SCRATCH: when a lane (or the test suite) names its own scratch, only that directory is swept, so a run
    cannot delete another lane's files out of a shared /tmp or cache. npu-pack: sweep every fconst19 scratch directory, not only the one written to #1917 sweeps fconst_scratch_dirs() unconditionally — which is correct
    for the box's own clean-up, and a shared-filesystem hazard for anyone who set the override to isolate themselves. That distinction is the one
    I would insist on before this widens what a run deletes;
  2. npu-pack: sweep every fconst scratch directory, and stop the test suite leaking into them (Fixes #1907) #1915 also cites the measurement and updates docs/npu.md, so the 17.0 GiB of tmpfs (and what a run will now reclaim) is written where
    an operator looks, not only in the code;
  3. both skip non-existent candidates rather than creating them, and both keep the exact name wherever they sweep — that part is identical.

Your arm count is higher (7 checks against #1915's test additions), so if #1915 is declined for some reason this is a correct PR to take
instead — I would just add the override clause to sweep_other_fconst_scratch first, because "sweep every directory" without it is a wider
blast radius than the problem needs.

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
cache that moved to disk strands whatever the earlier location holds.

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT at bcc9ade: READY — the eviction ran only where the write went, so the directory that stopped being written to was never swept

This is a resource leak that a correct-looking mechanism caused, and the diagnosis is exact: since #1801 the canonical copy goes to the on-disk cache first, while evict_stale_canonical_copies() runs only on the directory the copy was written to. So the moment the cache moved from /tmp to disk, nothing ever looked at /tmp again — and every copy an older binary left there stayed. On strixhalo /tmp is tmpfs, so the issue's 188 files / 17.0 GiB are RAM, 2–9 h old. A garbage collector whose scope is "the place I just wrote" cannot collect a place I no longer write to; the leak is not in the eviction logic but in its address.

The new sweep is scoped by the right things and keeps the right things: it takes the candidate directories, the chosen one, the current pack's identity and a prefix, and removes stale siblings of this pack/geometry plus 1bit-fconst19 files untouched for 6 h — the same policy, applied to every directory instead of one.

And the test is the discriminating kind, not a demonstration — five cases with three that must be KEPT:

check(swept == 1, "exactly the one directory that is neither chosen nor the cwd is swept")
check(!fs::exists(o_old),  "a 7 h old copy in a directory the run did not write to goes")   -- the leak
check(fs::exists(o_new),   "a fresh copy of another pack there is kept -- a lane may be using it")
check(!fs::exists(o_sib),  "this pack's stale-identity sibling there goes too")
check(fs::exists(o_unrel), "a file that is not fconst19 scratch is kept, however old")

The two keeps are the ones that matter: a cross-lane scratch directory is shared state on this box, and a sweeper that deleted another lane's fresh copy would be far worse than the leak it fixes — that is exactly how a "successful" cleanup takes out a running measurement. And o_unrel (an unrelated old file kept) is what stops the sweep from being rm -rf with extra steps.

One question worth answering in the body, not blocking: the 6 h floor is a policy constant, and the lane that hits it will be the one whose copy is 5 h 59 m old on a stalled evening — worth a line saying why 6 h and not, e.g., "6 h or the pack's own mtime", so the next person changing it knows what it was protecting.

Free-RAM note for this box specifically: 17 GiB of tmpfs is 17 GiB of the RAM every other lane's measurement is budgeting against — the same resource the MAX arms and the co-tenant pair report in co-tenancy.txt. A leak of this size in /tmp is invisible to df on the 150 GB floor yet shows up in free -g, which is why it survived several sessions.

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

VERDICT: 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

  1. weakly_canonical on both sides of the comparison. #1915 compares dir == written_dir as strings (:4928); this PR canonicalises chosen_dir and each candidate, which is the correct comparison on a box where /tmp can be a symlink and where $TMPDIR and the literal /tmp are 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.
  2. Skip cwd. plans fall 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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

VERDICT: 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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Heads-up: this PR and #1915 (or #1917, whichever you are reading) both Fixes #1907 and carry substantially the same change — sweeping every scratch directory, not only the one written to, plus the test_fconst_scratch_order() environment restore. Measured on both heads:

PR sweep change test-env lines (setenv/HOME/TMPDIR/restore)
#1915 yes 22
#1917 yes 17

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): /tmp/1bit-fconst19* held 145 files, 140 of them 0-byte, and — the part that costs RAM — 5 non-zero copies totalling 15,491 MB on tmpfs, ages 2.2–5.4 h. Note for whoever lands this: because the directory loop breaks at the first writable directory, once ~/.cache/1bit/fconst19 exists no /tmp copy can ever be reused, so the 6 h age qualifier is protecting something unreachable — and none of the five measured copies was over 6 h old, i.e. the rule as written would have swept none of them. Sweeping non-chosen directories on sight (age only on the chosen one) is what would actually reclaim the 15 GB.

I reviewed #1915 in depth (verdict at 2eb37d1); I have not reviewed #1917's own diff line by line. Flagging the overlap rather than picking a winner — that is the owner's call.

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

VERDICT at 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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Heads up before this and #1915 both land: they claim the same issue.

head diff files
#1915 2eb37d1 +109/−2 docs/npu.md, npu/pack_import.cpp, npu/pack_import.h, tests/npu_pack_import_test.cpp
#1917 (this) bcc9ade +72/−1 —

Both say Fixes #1907, both describe the same cause (materialise_token_fconst() prefers the on-disk cache since #1801, so evict_stale_canonical_copies() stopped visiting /tmp). I reviewed #1915 in detail and it holds up: the eviction call is per-directory (pack_import.cpp:755 → d.string()), the fix sweeps every candidate directory, and ONEBIT_FCONST_SCRATCH restricts the sweep to that directory so a lane with its own scratch cannot delete another lane's files — plus it stops the test suite producing the files in the first place.

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 add_test block became contentious, and it is cheaper to settle now than after both merge.)

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at bcc9ade — and it fixes a consequence of my own #1801, so it deserves saying plainly rather than as a neutral review.

// engine#1907: the sweep above ran only on the directory the copy was WRITTEN to. Since #1801 that is the on-disk ca[che]
// [measured on] strixhalo 2026-11-10, 2-9 h old. Every other scratch directory gets the same rules (stale siblings of this
// pack an[d] anything of the stem untouched for `max_age_seconds`). The current directory is never swept: it is the last-re[cently used]
int sweep_other_fconst_scratch(const std::vector<std::string>& dirs, const std::string& chosen_dir, …)

The regression my change introduced

Before #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 /tmp, were never collected again. The measurement says it exactly: 2–9 h old copies on strixhalo. That is the shape of a fix that moves where a thing lives without moving the code that cleans up after it.

What this PR gets right beyond the fix

  • it sweeps the others, not all of them — chosen_dir is excluded because it is the last-recently-used one, so the sweep cannot delete the copy the run is about to reuse. A blanket sweep would have "fixed" the leak by causing a re-materialisation storm;
  • the same rules apply everywhere (stale siblings of this pack under the identity prefix, plus anything of the stem untouched past max_age_seconds), so a directory's age is a property of the files, not of which directory it is;
  • the predicate takes the list (fconst_scratch_dirs()), which is the function tests: pin the .fconst scratch order and that a missing cache directory is created (#1801's CHANGES ask) #1818 introduced precisely so the order could be tested — the sweep and the writer now agree on the candidate set by construction.

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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

VERDICT: 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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

VERDICT: READY at 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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: CHANGES at bcc9ade — the helper is well tested, but the call is not, and a mutant proves it.

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;
all npu-pack checks passed -- pack leg SKIPPED

and that is exactly what the arm covers: test_sweep_other_fconst_scratch() calls pi::sweep_other_fconst_scratch(...) directly (:668) and asserts the five outcomes on its own fixture tree — the 7 h old copy goes, the fresh copy of another pack stays, this pack's stale-identity sibling goes, a non-fconst19 file stays, the chosen directory is left alone. All correct, all worth having. But the defect this PR fixes is that materialise_token_fconst() never called it for other directories, and no arm can fail if that call is deleted or moved inside the wrong branch.

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 d — the chosen directory — instead of the list, or it could be skipped on the cached-hit path (the early return when the copy already exists), which is the common case in a sweep of 30 packs and the only one that ever touches /tmp in practice.

What closes it, and it needs no fixture beyond what the test already builds: drive materialise_token_fconst() itself once with ONEBIT_FCONST_SCRATCH pointing at a temporary directory, with a 7 h old copy planted in a second scratch directory (TMPDIR), and assert the planted file is gone afterwards. The importer already has a seam for a temp-scratch path (materialise_token_fconst is called from plan()/import_pack()), and the pack fixture for a 1.9 .fconst exists in this file, so this is one arm on top of what is there.

Everything else I have no complaint about, and two of the details are the ones I would have missed:

  • skipping the cwd — "it is a last-resort WRITE location, not a scratch directory, and a user's working directory is not this importer's to clean" — is exactly right, and it is the difference between a tidy-up and deleting someone's files;
  • keeping a fresh copy of another pack — a lane may be mid-import — is the concurrency rule that makes this safe to run unconditionally instead of behind a flag.

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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

VERDICT: READY at 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:

  1. The age threshold is the only thing protecting another lane's in-flight scratch. max_age_seconds at 2-9 h was measured, not guessed, which is good — but the name is kScratchMaxAgeSeconds, a compile-time constant, and the pool of /tmp writers on this box is unbounded (other lanes' probes, the kernel emitter's mktemp -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.
  2. Legacy names. The legacy_name parameter 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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

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 bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

VERDICT: READY at 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_canonical is called with an std::error_code that is then ignored for both chosen and cwd. If it fails, chosen compares as empty and the "never sweep the directory we wrote to" guarantee silently
    degrades to "sweep it but keep this one file". Checking !ec before the comparison (or comparing lexically_normal() paths) closes that.
  • fs::remove is called without checking the entry is a regular file, so a directory named 1bit-fconst19… that happens to be empty would be removed as well. Harmless in practice, one is_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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT at bcc9ade: CHANGES — with the same headline note I put on #1915: #1915 is a second PR for #1907, and each of these two has the safety arm the other is missing. Converge on one.

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 swept

The candidate list's last entry is the current directory, so without this a npu-pack import run inside a directory holding 1bit-fconst19* files deletes them — a new risk created by the fix itself, since before it the importer only ever swept the directory it wrote to.

What is missing: it does not honour ONEBIT_FCONST_SCRATCH. #1915 does, and its argument is a good one: the override is a lane saying "my scratch is here", and the test suite sets one per run. Without that arm, an override run still sweeps /tmp and ~/.cache, i.e. a test can clean files another lane is using. That is the arm to fold in:

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 1bit-fconst19* in another candidate is reclaimed.

One measured datum in support of the goal: #1797 measured the pre-version names at 3.6 GB each in /tmp, and one was still there when I checked today — so the reclaim is worth doing properly rather than quickly.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at bcc9ade — built and ran; the arm passes (scratch sweep: other scratch dirs swept by age and sibling, chosen dir and cwd left alone).

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 /tmp — tmpfs, i.e. RAM — became unreachable by the sweep that only looked at the directory it wrote to. 188 files / 17.0 GiB on this box, 2–9 h old, matches what the owner was chasing at 22:4x for a different reason.

Three safety properties, and I checked each rather than trusting the comment:

  • the chosen directory is excluded via weakly_canonical comparison, not string equality — so /tmp vs /tmp/ vs a symlinked path cannot make it sweep where it just wrote;
  • the current directory is excluded unconditionally, with the right reason stated ("a user's working directory is not this importer's to clean");
  • the age rule only fires on names carrying the 1bit-fconst19 stem (kScratchStem), so it cannot touch unrelated files in a shared /tmp — I checked the prefix test (base.compare(0, stem.size(), stem)) rather than assuming the glob.

One thing worth a decision rather than a note: kScratchMaxAgeSeconds = 6 h, and the age sweep is cross-pack. A copy of pack A that another lane imported 6 h 1 min ago is deleted by this run importing pack B — correct per the comment's intent (an abandoned pack otherwise sits in RAM for the life of the box), and harmless because a deleted copy only costs a rebuild and a reader holding it open survives the unlink. But it means a lane's scratch is now a shared cache with an eviction policy it did not opt into. Worth one line in the docs/comment naming that trade explicitly, because the failure a lane would report is "the importer is rewriting a 291 MB copy I already had", which reads like a bug and is the policy working.

Two smaller notes:

  1. The 6 h clock and the sweeper disagree in a way that will age badly: the name carries the source's mtime (src_stamp), so a pack that has not changed keeps one copy forever while a lane that re-downloads often churns — good — but the sweep uses the copy's own mtime, so a copy that is merely read repeatedly never ages. If that is intended (only rebuilt copies refresh), say so; it explains why a hot pack's file survives while a cold one is deleted after 6 h.
  2. The arm's name ("other scratch dirs swept by age and sibling") is right but it does not assert the counter-case that protects lanes: a file with the stem that is newer than the age must survive, and one without the stem must survive regardless of age. If the fixture already covers those, ignore this — but they are the two checks that would catch a future "just delete the stem" simplification.

Scope of my check, stated: built and ran npu_pack_import_test at this head; read sweep_other_fconst_scratch, evict_stale_canonical_copies, the stem and the age constant. No device, no pack. I also saw the litter it targets for real: /tmp/1bit-fconst19* files present on this box right now.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at bcc9ade

The 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
written to — and the copies older binaries had left in /tmp were never looked at again: 188 files, 17.0 GiB measured on strixhalo, 2–9 h old. That
is 17 GiB of tmpfs, i.e. RAM, which is the resource this box fights over all day.

The fix is scoped by three refusals, each of which I would have asked for:

  1. The chosen directory is never swept — "it is the last-resort WRITE location, not a scratch directory". Sweeping where you just wrote is how a
    canonical copy deletes itself.
  2. The user's current directory is never swept, and it says why: "a user's working directory is not this importer's to clean". A tool that removes files
    from $PWD because they match a stem is a tool nobody should run twice.
  3. What is eligible is narrow and named: stale siblings of this pack and geometry (prefix, legacy_name), plus anything of the stem untouched for
    max_age_seconds. Not "anything that looks like a scratch file".

Both paths are canonicalised with weakly_canonical(..., ec) before comparison, so a symlinked /tmp (or a chosen_dir reached by a different path) cannot
defeat the two exclusions — that comparison is the security-relevant line in a delete path, and it is done with an error_code rather than throwing.

+72/-1, and returning the count of swept directories gives the caller something to log; a sweep that cannot say what it did is indistinguishable from one
that did nothing. Nothing to change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tmpfs: 17.0 GiB of abandoned fconst19 pack scratch in /tmp (188 files, 2-9 h old) is RAM, not disk

1 participant