From 9670c85a18f1d989bd1a8128458d660c588cffa6 Mon Sep 17 00:00:00 2001 From: bong-water-water-bong Date: Mon, 9 Nov 2026 23:31:59 -0400 Subject: [PATCH 1/2] npu-pack: the scratch sweep reaches the pre-version names and the packs nobody imports again Follow-up to #1770/#1772. #1772 evicts sibling copies of the geometry being written, which is right, but it matches on `ident_prefix` = `1bit-fconst19v-----`, and the generation before the source identity was in the name is spelled `1bit-fconst19----.fconst` -- no `v-`, no size, no mtime. So those files were invisible to it and stayed resident. Measured on this box while writing this: /tmp/1bit-fconst19-*.fconst 3 files, 10.67 GiB (1bit-fconst19-20133236-4096-32-32.fconst and two siblings) /tmp/1bit-fconst19v1-*.fconst 96 files, 11.57 GiB /tmp/1bit-fconst19* 99 files <- tmpfs, i.e. RAM, 22 GiB of it Two additions, both to the same sweep: 1. **the pre-version name for the same pack and geometry**, matched exactly (`legacy_name`), because it carries no identity to compare and there is at most one per geometry. Another geometry's legacy name is kept -- it is not this run's to remove, which is the rule #1772 already applies to current-generation copies; 2. **an age rule**: any `1bit-fconst19*` in the directory older than `max_age_seconds` (6 h; 0 disables) goes. A write-triggered sweep can only reach the geometry being written, so a pack nobody imports again is otherwise resident for the life of the box -- that is the tail #1770 measured (699 files, 8.7 GiB, 691 of them 0-byte) and it needs the clock, not the write, to reach it. 6 h is far above the longest import measured (minutes), so nothing in flight is touched; the concurrent-writer hazard #1772's comment does not cover is bounded by the same number. Verified standalone (the function is `#include`d from the source, not retyped -- the NPU/vendor stack this box does not build is not needed for it): 9 arms, all pass -- keep, same-pack-other-mtime, legacy-for-this-geometry (removed), legacy-for-another-geometry (kept), abandoned-older-than-age (removed), fresh-other-geometry (kept), `max_age = 0` disables the age rule so the existing three-argument callers are unchanged, and the two degenerate arguments still remove nothing. The engine's own test gains the same four arms (16 total in `test_evict_stale_canonical_copies`), with the mtime set 7 h back through `fs::last_write_time`. --- npu/pack_import.cpp | 48 +++++++++++++++++++++++++++++----- npu/pack_import.h | 18 +++++++++---- tests/npu_pack_import_test.cpp | 26 +++++++++++++++++- 3 files changed, 79 insertions(+), 13 deletions(-) diff --git a/npu/pack_import.cpp b/npu/pack_import.cpp index dfef45831..d6df1838d 100644 --- a/npu/pack_import.cpp +++ b/npu/pack_import.cpp @@ -699,6 +699,12 @@ std::string materialise_token_fconst(const std::string& pack_dir, const Dims& di // canonical form (a new kFconst19Rules entry, a different tile order) leaves the source file untouched, so source // identity alone would keep serving a copy of the old form. Bump this when the canonical form changes. constexpr int kCanonicalFormVersion = 1; + // Every generation of the scratch cache starts with this (engine#1770). Named because the sweep below has to match the + // generations that have no version suffix. + constexpr const char* kScratchStem = "1bit-fconst19"; + // A scratch copy older than this is a pack no run is importing: no import holds one for hours, so an mtime this far back + // means the pack was abandoned, not that somebody is using it. Kept well above the longest import measured (~minutes). + constexpr int64_t kScratchMaxAgeSeconds = 6 * 3600; // Everything but the source identity: the group this run evicts its stale siblings from (engine#1770). const std::string ident_prefix = "1bit-fconst19v" + std::to_string(kCanonicalFormVersion) + "-" + tag + "-" + std::to_string(dims.hidden) + "-" + std::to_string(dims.heads) + "-" + @@ -732,7 +738,12 @@ std::string materialise_token_fconst(const std::string& pack_dir, const Dims& di work = cand.string(); // This is the copy that will be written, so the copies of the same pack and geometry at other sizes/mtimes are // the ones nothing will reuse (engine#1770). - evict_stale_canonical_copies(d.string(), name, ident_prefix); + // The pre-version generation of the same pack and geometry: `1bit-fconst19----.fconst`, + // with no source identity in the name, so the prefix above cannot match it (engine#1770: three such files, 10.67 + // GiB of tmpfs, had been resident all day). + const std::string legacy_name = std::string(kScratchStem) + "-" + tag + "-" + std::to_string(dims.hidden) + "-" + + std::to_string(dims.heads) + "-" + std::to_string(dims.layers) + ".fconst"; + evict_stale_canonical_copies(d.string(), name, ident_prefix, legacy_name, kScratchMaxAgeSeconds); // Say where it went whenever that is not the ordinary place: the copy is a few hundred MB and it is scratch. std::error_code ec2; const fs::path tmpdir = [&] { try { return fs::temp_directory_path(); } catch (...) { return fs::path(); } }(); @@ -4331,15 +4342,38 @@ bool canonical_copy_reusable(const std::string& work, int64_t expected_len) { // a copy a reader is about to use: reuse requires the entire name to match, and the one name that can match is the one // being kept. A reader holding a removed file open is unaffected (the inode outlives the unlink); at worst a concurrent // lane rewrites a copy it would have reused, which costs a file write and no correctness. -void evict_stale_canonical_copies(const std::string& dir, const std::string& keep_name, const std::string& prefix) { +void evict_stale_canonical_copies(const std::string& dir, const std::string& keep_name, const std::string& prefix, + const std::string& legacy_name, int64_t max_age_seconds) { if (dir.empty() || keep_name.empty() || prefix.empty() || keep_name.size() <= prefix.size()) return; - std::error_code ec; + const std::string stem = "1bit-fconst19"; + const auto now = fs::file_time_type::clock::now(); + std::error_code ec; for (fs::directory_iterator it(fs::path(dir), ec), end; !ec && it != end; it.increment(ec)) { const std::string base = it->path().filename().string(); - if (base == keep_name || base.size() <= prefix.size()) continue; - if (base.compare(0, prefix.size(), prefix) != 0) continue; - std::error_code rm; - fs::remove(it->path(), rm); + if (base == keep_name) continue; + // same pack and geometry, different source identity: nothing reuses it + if (base.size() > prefix.size() && base.compare(0, prefix.size(), prefix) == 0) { + std::error_code rm; + fs::remove(it->path(), rm); + continue; + } + // the generation before the source identity was part of the name: at most one per pack+geometry, matched exactly + if (!legacy_name.empty() && base == legacy_name) { + std::error_code rm; + fs::remove(it->path(), rm); + continue; + } + // and any scratch copy at all that nobody has touched for max_age_seconds is an abandoned pack: a write-triggered + // sweep is enough for the geometry being written, but a pack that is never imported again would otherwise stay in + // tmpfs (i.e. in RAM) for the life of the box (engine#1770) + if (max_age_seconds > 0 && base.size() > stem.size() && base.compare(0, stem.size(), stem) == 0) { + std::error_code tec; + const auto mtime = fs::last_write_time(it->path(), tec); + if (!tec && now - mtime > std::chrono::seconds(max_age_seconds)) { + std::error_code rm; + fs::remove(it->path(), rm); + } + } } } diff --git a/npu/pack_import.h b/npu/pack_import.h index b4ad5821d..8857dcee6 100644 --- a/npu/pack_import.h +++ b/npu/pack_import.h @@ -177,12 +177,20 @@ Plan plan(const std::string& pack_dir); // and a directory on the scratch path must all come back false. bool canonical_copy_reusable(const std::string& work, int64_t expected_len); -// Removes the canonical Token .fconst copies in `dir` that share `prefix` -- one pack path and geometry -- but are not -// `keep_name` (engine#1770). Reuse writes a new copy when the source changes rather than overwriting, so without this the -// scratch directory grows one multi-hundred-MB file per change; on this box 699 files / 8.7 GiB of tmpfs. Exposed for the -// same reason as canonical_copy_reusable(): the rule it applies is what can be wrong, and a test can prove it on a +// Removes the canonical Token .fconst copies in `dir` that nothing will reuse (engine#1770). Reuse writes a new copy when the +// source changes rather than overwriting, so without this the scratch directory grows one multi-hundred-MB file per change; on +// this box 699 files / 8.7 GiB of tmpfs. Three rules: +// * share `prefix` (one pack path and geometry) but are not `keep_name` -- normal generations of a superseded source; +// * equal `legacy_name` -- the generation before the source identity was part of the name +// (`1bit-fconst19----.fconst`), which `prefix` cannot match because it was written without the version +// suffix, so those files were invisible to this sweep and stayed resident (three of them, 10.67 GiB, on 2026-11-10); +// * start with `1bit-fconst19` and are older than `max_age_seconds` (0 disables) -- a pack nobody imports again. A write-triggered +// sweep only reaches the geometry being written, so the abandoned tail needs an age rule. +// Exposed for the same reason as canonical_copy_reusable(): the rule it applies is what can be wrong, and a test can prove it on a // directory of empty files with no pack at all. -void evict_stale_canonical_copies(const std::string& dir, const std::string& keep_name, const std::string& prefix); +void evict_stale_canonical_copies(const std::string& dir, const std::string& keep_name, const std::string& prefix, + const std::string& legacy_name = std::string(), + int64_t max_age_seconds = 0); // The text artifacts a runnable container needs beside model.q4nx, and which of them the pack // actually carries. A pack fetched weight by weight can be weights-only (engine#1024; AMD's repos diff --git a/tests/npu_pack_import_test.cpp b/tests/npu_pack_import_test.cpp index 698b336dc..f24d62c59 100644 --- a/tests/npu_pack_import_test.cpp +++ b/tests/npu_pack_import_test.cpp @@ -47,6 +47,7 @@ #include #include #include +#include #include #include #include @@ -664,6 +665,28 @@ void test_evict_stale_canonical_copies() { check(exists(other_form), "evict: another canonical form version is kept"); check(exists(unrelated), "evict: unrelated files in the directory are kept"); + // engine#1770 (second generation): the pre-version naming has no source identity, so a prefix match on the current form + // never sees it -- three such files sat in /tmp for a day at 3.6 GB each, i.e. 10.67 GiB of RAM. + const std::string legacy = "1bit-fconst19-12345-896-14-24.fconst"; + const std::string legacy_other_geometry = "1bit-fconst19-12345-1536-12-28.fconst"; + touch(legacy); + touch(legacy_other_geometry); + pi::evict_stale_canonical_copies(dir.string(), keep, prefix, legacy); + check(!exists(legacy), "evict: the pre-version name for THIS geometry goes (no identity to reuse)"); + check(exists(legacy_other_geometry), "evict: ... but another geometry's pre-version name is kept"); + + // and the abandoned tail: a pack nobody imports again is only reachable by age, because the sweep runs when a pack is + // written. 0 must disable it, so the callers that pass nothing keep the old behaviour. + const std::string old_other = "1bit-fconst19v1-77777-1536-12-28-995712640-73345656.fconst"; + const std::string fresh_other = "1bit-fconst19v1-88888-1536-12-28-995712640-73345656.fconst"; + touch(old_other); + touch(fresh_other); + const auto seven_hours_ago = fs::file_time_type::clock::now() - std::chrono::hours(7); + fs::last_write_time(dir / old_other, seven_hours_ago, ec); + pi::evict_stale_canonical_copies(dir.string(), keep, prefix, legacy, 6 * 3600); + check(!exists(old_other), "evict: an abandoned geometry older than the age goes (the tail a write cannot reach)"); + check(exists(fresh_other), "evict: a fresh copy of another geometry stays -- another run may be writing it"); + // The degenerate arguments must not turn into "delete the directory". for (const auto& f : {keep, stale}) touch(f); pi::evict_stale_canonical_copies(dir.string(), "", prefix); @@ -673,7 +696,8 @@ void test_evict_stale_canonical_copies() { pi::evict_stale_canonical_copies((dir / "absent").string(), keep, prefix); check(exists(keep), "evict: an absent directory is not an error"); fs::remove_all(dir, ec); - std::printf(" canonical copy eviction: 12 arms (name composition + keep/stale/geometry/pack/form/unrelated + 3 degenerate)\n"); + std::printf(" canonical copy eviction: 16 arms (name composition + keep/stale/geometry/pack/form/unrelated + legacy name +" + " geometry/age rules + 3 degenerate)\n"); } void test_modelbin_genre_refusal(const std::string& root) { From 4103df756a85fbaa4ae09eca7f4431a3ec4201c1 Mon Sep 17 00:00:00 2001 From: bong-water-water-bong Date: Mon, 9 Nov 2026 23:56:19 -0400 Subject: [PATCH 2/2] npu-pack: reuse touches the canonical copy, so the eviction age means "last use" Review of #1797: the age rule sweeps any `1bit-fconst19*` whose `last_write_time` is older than `max_age_seconds` ("nobody has touched", 6 h), but the reuse path only READS the copy -- `canonical_copy_reusable(work, expected)` then `return work` -- so a copy in continuous use ages past the window and gets swept, and the next run rewrites a multi-hundred-MB file. That is the same cost the whole eviction family exists to remove, arriving by the rule meant to bound it. The reuse path now bumps the mtime before returning, so the timestamp means what the rule assumes. Errors are deliberately ignored: failing to bump an mtime must not fail an import that already has a good copy. Arm: a copy of a DIFFERENT geometry aged 10 h is swept by a 6 h window; one with a fresh mtime survives; the copy being written survives. Different geometry matters -- the name rule would decide for a same-geometry file, and my first version of this arm made exactly that mistake and failed on it, which is why the arm says so where the names are chosen. Verified by building and running `npu_pack_import_test` at this head: `all npu-pack checks passed`, 16 eviction arms. The arm pins the RULE (fresh mtime survives an age sweep); the touch that produces a fresh mtime is the one-line change above, and it is not exercised end to end here because that needs a real pack through `canonical_token_fconst`. Refs #1797 --- npu/pack_import.cpp | 6 ++++++ tests/npu_pack_import_test.cpp | 22 ++++++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/npu/pack_import.cpp b/npu/pack_import.cpp index d6df1838d..5c2970314 100644 --- a/npu/pack_import.cpp +++ b/npu/pack_import.cpp @@ -562,6 +562,12 @@ std::string canonical_token_fconst(const std::string& path, int hidden, int head // and a TRUNCATED copy from an interrupted run is caught by the length test rather than trusted. const int64_t expected = int64_t(fc.size) - int64_t(layers) * hdr_len; if (canonical_copy_reusable(work, expected)) { + // engine#1797 review: the eviction age is `last_write_time`, and a reuse used to READ the file without writing + // it -- so a copy in continuous use aged past `max_age_seconds` and was swept, and the next run rewrote a + // multi-hundred-MB file. Touching on reuse makes the timestamp mean what the rule assumes: last USE. Errors + // are ignored: failing to bump an mtime must not fail an import that already has a good copy. + std::error_code tec; + fs::last_write_time(fs::path(work), fs::file_time_type::clock::now(), tec); unmap_file(fc.map, fc.size); return work; } diff --git a/tests/npu_pack_import_test.cpp b/tests/npu_pack_import_test.cpp index f24d62c59..9774d9241 100644 --- a/tests/npu_pack_import_test.cpp +++ b/tests/npu_pack_import_test.cpp @@ -687,6 +687,28 @@ void test_evict_stale_canonical_copies() { check(!exists(old_other), "evict: an abandoned geometry older than the age goes (the tail a write cannot reach)"); check(exists(fresh_other), "evict: a fresh copy of another geometry stays -- another run may be writing it"); + // engine#1797 review: the age rule reads `last_write_time`, and reuse used to READ the copy without writing it -- so + // a file in continuous use aged past `max_age_seconds` and was swept, and the next run rewrote it. The repair is + // that reuse now touches the copy, which makes the timestamp mean "last use". This arm pins the rule that repair + // relies on: a copy with a FRESH mtime survives an age sweep, and one with an old mtime does not. + { + // DIFFERENT geometry (hidden 1536), so the name rule cannot reach them and only the age rule decides -- which is + // the rule this arm is about. Same geometry would test the name rule instead, and my first version of this arm + // made exactly that mistake and failed on it. + const std::string stem = "1bit-fconst19v1-"; + const std::string aged = stem + "12345-1536-12-28-995712640-73345656.fconst"; + const std::string fresh_name = stem + "12345-1536-12-28-500000000-11111111.fconst"; + touch(aged); + touch(fresh_name); + std::error_code tec; + fs::last_write_time(dir / aged, fs::file_time_type::clock::now() - std::chrono::hours(10), tec); + check(!tec, "evict age: the fixture can age a copy"); + // 6 h window: the 10 h copy goes, the one just written stays + pi::evict_stale_canonical_copies(dir.string(), keep, prefix, "", 6 * 3600); + check(!exists(aged), "evict age: a copy nobody has touched for 10 h is swept"); + check(exists(fresh_name), "evict age: a copy with a fresh mtime survives -- what reuse's touch produces"); + check(exists(keep), "evict age: and the copy being written is still kept"); + } // The degenerate arguments must not turn into "delete the directory". for (const auto& f : {keep, stale}) touch(f); pi::evict_stale_canonical_copies(dir.string(), "", prefix);