diff --git a/npu/pack_import.cpp b/npu/pack_import.cpp index dfef45831..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; } @@ -699,6 +705,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 +744,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 +4348,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..9774d9241 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,50 @@ 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"); + + // 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); @@ -673,7 +718,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) {