From 687747a62161f5a4c7bc5efe44e2960194a7d83e Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Thu, 13 Aug 2026 03:55:51 +0000 Subject: [PATCH] fix(GATE-SNAPSHOT-CONTENT-PIN): pin the Nemotron checkpoint by CONTENT, not by "this revision was downloaded here once" (#569) `parity::Nemotron35LightningSnapshot()` gated the staged `hf download --local-dir` tree on the EXISTENCE of `.cache/huggingface/trees/.json`. That file records that the revision was fetched into this directory at some point. It does not record that the bytes now on disk are that revision, and the two come apart. DEMONSTRATED (RED, against this header at c6b240ed): a directory holding a DIFFERENT model's config.json -- `{"architectures": ["LlamaForCausalLM"]}` -- whose every `.cache/huggingface/download/.metadata` named revision `bbbb...`, resolved. The manifest was an empty `touch`ed file and a decoy manifest beside it changed nothing. W6 of `.agents/specs/nemotron-h-model.md` (#517) is a token-exact gate that resolves through this function, and a token gate is precisely the instrument that cannot see a substituted checkpoint: the wrong model reads as a correctness regression in whatever changed last. THE `trees/` QUESTION, ANSWERED. `hf download --local-dir` does NOT delete a superseded manifest. huggingface_hub 1.7.2 (this box) has no `trees` code at all; the writer is `_tree_cache.py`, added in 1.23.0/1.24.0, which is what is installed on the gate host where the NAS copy was fetched. Its module docstring states the design outright -- "Because a commit hash is immutable, its tree listing never changes and can be cached forever WITHOUT ANY INVALIDATION LOGIC" -- and `write_tree_cache` only `os.replace`s `.json`. `trees` appears five times in the whole library, all reads and path joins; nothing removes one. So manifests ACCUMULATE, and the worst case is the real case: after a re-download at a later revision the pinned revision's manifest is still there, vouching for bytes that are no longer it. `test_hf_snapshot_pinning` stages exactly that. THE FIX. Sweep the per-file download sidecars instead. Line 1 of `.cache/huggingface/download/.metadata` is that file's `commit_hash` (`huggingface_hub/_local_folder.py` module docstring and `read_download_metadata`), and it does not accumulate: `file_download.py` calls `write_download_metadata(..., commit_hash=...)` on all four outcomes, including the `local_metadata.etag == etag` early return that downloads nothing, so it always names the revision the bytes were fetched as. On the NAS copy all 69 sidecars read 29f2d174..., in exact bijection with the 69 staged files. SCOPE: EVERY staged file, not just config.json. The goldens depend on the WEIGHTS. `hf download --revision B` into a tree fetched at A leaves a correct config beside wrong shards, which is the substitution that matters and the one a config-only check cannot see. 69 sidecar reads of ~130 bytes against a 20.1 GiB checkpoint the gate is about to load is not a trade-off. The check is symmetric -- a file with NO sidecar refuses too, or substituting a shard would cost one `rm`. The manifest check is REPLACED, not kept as an extra term: it can no longer refuse anything the sweep does not already refuse, while it CAN refuse a good tree fetched by a huggingface_hub older than 1.23, which writes sidecars and no manifest. Preserved unchanged: `CHECKPOINT_ROOT` as the default path, `VT_NEMOTRON35_SNAPSHOT` as the explicit override that wins over it and is deliberately NOT revision-gated (naming ONE directory is the different-checkpoint run), and a set-but-wrong override refusing rather than falling back. The resolver now returns the reason it refused through an optional `why` out-param; `test_modelopt_mixed_precision_checkpoint` prints it verbatim in its exit-77 banner, so the skip names WHICH file recorded WHICH revision. The no-argument spelling is unchanged, so `test_nemotron_h_scaffold` is untouched. The four sibling accessors were checked and NOT changed. They resolve `models--org--name/snapshots//`, where the revision is the directory name and every entry is a symlink into `blobs/`; a re-download at a new revision creates a NEW snapshot directory rather than replacing bytes under the old one. Verified on the gate host, where that repo holds both @890bdef7 and @ccdaab7e side by side. Genuinely a different situation, and widening this change to them would be unevidenced. IMP-MUTATE, four mutations, no survivors: disabling the wrong-revision refusal (3 cases RED), deleting the missing-sidecar refusal (1 RED -- it needed the MESSAGE asserted, since falling through to the wrong-revision branch also refuses), replacing the sweep with a config.json-only check (3 RED), and deleting the vacuous-tree refusal (1 RED, reachable via a config.json that is a DIRECTORY). Header restored byte-for-byte, md5 807c9171c3529fa300e6f7729d6a57d4. Evidence: test_hf_snapshot_pinning 11 cases / 42 assertions, Status: SUCCESS. Header compiled in a real build, not only swept: test_qwen36_paged_engine, test_qwen27_paged_engine, test_qwen36_weights, test_nemotron_h_scaffold and test_modelopt_mixed_precision_checkpoint all rebuilt clean, and the full Release `-Werror` build exits 0 -- `agent-preflight.sh` does not compile the parity TUs, and this header RED'd main today when an accessor was ordered above the `HfSnapshot` it called. Against the LIVE NAS checkpoint the arm resolves and passes 12181/12181. df -h /: 68G avail, 85% used. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code] --- tests/parity/hf_snapshot.h | 182 +++++++++-- tests/parity/test_hf_snapshot_pinning.cpp | 283 ++++++++++++++++++ ...st_modelopt_mixed_precision_checkpoint.cpp | 30 +- 3 files changed, 468 insertions(+), 27 deletions(-) diff --git a/tests/parity/hf_snapshot.h b/tests/parity/hf_snapshot.h index c5478bc73..f0e2ff311 100644 --- a/tests/parity/hf_snapshot.h +++ b/tests/parity/hf_snapshot.h @@ -20,8 +20,10 @@ // pass. Publishers re-quantize in place; a correctness gate must name the // revision its golden belongs to. +#include #include #include +#include #include #include @@ -72,6 +74,106 @@ inline std::string HfSnapshot(const char* repo_dir, const char* revision, return snap.string(); } +// GATE-SNAPSHOT-CONTENT-PIN (issue #569). The revision the bytes of +// `/` were actually downloaded as, or "" when nothing records +// it. +// +// A `hf download --local-dir` tree keeps one sidecar per file at +// `/.cache/huggingface/download/.metadata`, three lines: +// commit hash, etag, timestamp. That layout is the library's own -- see the +// `huggingface_hub/_local_folder.py` module docstring, and +// `read_download_metadata`, which parses the three back in that order into +// `LocalDownloadFileMetadata.commit_hash`. +// +// Line 1 is the record this header needs, and the reason it is trustworthy is +// that it is rewritten IN PLACE on every download: +// `huggingface_hub/file_download.py` calls +// `write_download_metadata(..., commit_hash=commit_hash, ...)` on all four +// outcomes -- fresh download, copy out of the shared cache, LFS hash matched, +// and the `local_metadata.etag == etag` early return that downloads nothing at +// all. So after re-fetching a repo at a new revision, every file's sidecar names +// the new one, including files whose bytes did not change. +inline std::string HfLocalDownloadCommit(const std::filesystem::path& local_dir, + const std::string& name) { + std::ifstream in(local_dir / ".cache/huggingface/download" / + (name + ".metadata")); + std::string line; + if (!std::getline(in, line)) return ""; + while (!line.empty() && (line.back() == '\r' || line.back() == '\n')) { + line.pop_back(); + } + return line; +} + +// True when EVERY file staged directly in `local_dir` was downloaded at +// `revision`. `why`, when non-null, receives the first thing that was wrong, so +// the caller's skip banner can name it. +// +// WHY EVERY FILE AND NOT JUST `config.json`. The cheap version of this check +// reads one sidecar, and one sidecar answers a question nobody asked: the +// goldens depend on the WEIGHTS. `hf download config.json --revision B` +// into a tree fetched at A leaves a config from B beside 52 shards from A, and +// the reverse -- shards re-fetched at B under a config still recording A -- is +// the substitution that matters and the one a config-only check cannot see. The +// sweep costs 69 sidecar reads of ~130 bytes each against a 20.1 GiB checkpoint +// the gate is about to load, so there is nothing to trade off. +// +// It is symmetric on purpose. A file with NO sidecar refuses just as a file with +// the wrong one does, because otherwise substituting a shard would cost one `rm` +// of its sidecar, and a foreign file dropped into the tree would carry no +// provenance at all. Directories are skipped -- `.cache` is the bookkeeping +// itself -- and a tree with no staged files refuses rather than passing +// vacuously. +inline bool HfLocalDirIsRevision(const std::filesystem::path& local_dir, + const std::string& revision, + std::string* why) { + namespace fs = std::filesystem; + std::error_code ec; + const fs::directory_iterator end; + fs::directory_iterator it(local_dir, ec); + if (ec) { + if (why != nullptr) { + *why = "cannot list " + local_dir.string() + ": " + ec.message(); + } + return false; + } + std::size_t checked = 0; + for (; it != end; it.increment(ec)) { + if (ec) { + if (why != nullptr) { + *why = "cannot list " + local_dir.string() + ": " + ec.message(); + } + return false; + } + std::error_code kind; + if (!it->is_regular_file(kind) || kind) continue; + const std::string name = it->path().filename().string(); + const std::string got = HfLocalDownloadCommit(local_dir, name); + if (got.empty()) { + if (why != nullptr) { + *why = name + " has no .cache/huggingface/download/" + name + + ".metadata, so nothing records which revision its bytes are"; + } + return false; + } + if (got != revision) { + if (why != nullptr) { + *why = name + " was downloaded at revision " + got + ", not " + + revision; + } + return false; + } + ++checked; + } + if (checked == 0) { + if (why != nullptr) { + *why = "no files staged in " + local_dir.string(); + } + return false; + } + return true; +} + // The Nemotron-3.5-Lightning gate model (#517), and the ONE resolver for it. // // Unlike the Qwen pins above, this one is NOT in the HF cache: it is staged on @@ -86,9 +188,36 @@ inline std::string HfSnapshot(const char* repo_dir, const char* revision, // an env override is deliberately not revision-checked. So the pin named the // revision the goldens belong to and could not refuse a different one, which is // the exact failure `kQwen27NvfP4Revision` exists because of. Both spellings now -// resolve HERE, and the `local_dir` layout does record its revision, just not in -// the path: `hf download --local-dir` writes a per-revision file manifest at -// `/.cache/huggingface/trees/.json`. That is what is checked. +// resolve HERE. +// +// GATE-SNAPSHOT-CONTENT-PIN (issue #569). LOW-3 first gated this arm on the +// EXISTENCE of `/.cache/huggingface/trees/.json`, the manifest +// `hf download --local-dir` writes per revision. That records "this revision was +// downloaded into this directory once". It does not record "these bytes are that +// revision", and DEMONSTRATED during review: a directory holding a different +// model's `config.json`, whose own +// `.cache/huggingface/download/config.json.metadata` named a different revision, +// still resolved -- with the manifest an empty `touch`ed file and a decoy +// manifest beside it changing nothing. +// +// The two records come apart, and huggingface_hub is explicit about why. +// `_tree_cache.py` (1.23.0 / 1.24.0, the versions on the gate host that wrote +// this tree; 1.7.2 here has no `trees` code at all) opens with "Because a commit +// hash is immutable, its tree listing never changes and can be cached forever +// WITHOUT ANY INVALIDATION LOGIC". `write_tree_cache` only ever `os.replace`s +// `.json`, and no code path in the library removes one -- the string +// `trees` appears exactly five times in it, all reads and path joins. So +// manifests ACCUMULATE: re-download at a later revision and the pinned +// revision's manifest is still sitting there, vouching for bytes that are no +// longer it. The existence check could not have been salvaged; it is not a +// weaker version of the right check, it is a check of a different fact. +// +// The per-file sidecar is the right fact and it does not accumulate -- see +// `HfLocalDownloadCommit` above for the four call sites that rewrite it. The +// manifest check is therefore REPLACED rather than kept as an extra term: as an +// AND-term it can no longer refuse anything the sidecar sweep does not already +// refuse, while it CAN refuse a good tree fetched by a huggingface_hub older +// than 1.23, which writes sidecars and no manifest at all. // // Resolution order, in the order the code checks it, and why: // @@ -97,11 +226,12 @@ inline std::string HfSnapshot(const char* repo_dir, const char* revision, // that a set-but-wrong override refuses rather than falling back. First, so // that setting it OVERRIDES `CHECKPOINT_ROOT` rather than racing it. // 2. Otherwise `CHECKPOINT_ROOT` -> `/`, -// and the revision manifest MUST be present. This is the DEFAULT path every -// gate takes, so it is the one that has to carry the pin: a re-download of -// the same repo name lands a different revision under the identical path, -// and a gate that cannot tell would substitute it silently. Missing -// manifest => "" => the caller's loud skip, never a substitution. +// and EVERY file staged there must record the pinned revision as the one it +// was downloaded at. This is the DEFAULT path every gate takes, so it is the +// one that has to carry the pin: a re-download of the same repo name lands a +// different revision under the identical path, and a gate that cannot tell +// would substitute it silently. Any mismatch => "" plus a reason in `why` +// => the caller's loud skip, never a substitution. // 3. Otherwise the ordinary HF cache layout, for a host that fetched it that // way. // @@ -111,23 +241,41 @@ inline std::string HfSnapshot(const char* repo_dir, const char* revision, // // Absent both env vars => "" => the caller emits its loud SKIP, which is the // intended behavior off the gate host. -inline std::string Nemotron35LightningSnapshot() { +// +// `why`, when non-null, receives the reason for a "" -- an empty string when the +// answer is non-empty. A token-exact gate that silently loaded the wrong +// checkpoint is the failure this header exists to prevent, and a skip that says +// only "not staged" gets re-run rather than investigated. +inline std::string Nemotron35LightningSnapshot(std::string* why = nullptr) { namespace fs = std::filesystem; std::error_code ec; + if (why != nullptr) why->clear(); const char* over = std::getenv("VT_NEMOTRON35_SNAPSHOT"); const char* root = std::getenv("CHECKPOINT_ROOT"); if ((over == nullptr || *over == '\0') && root != nullptr && *root != '\0') { const fs::path dir = fs::path(root) / kNemotron35LightningLocalDirName; - const fs::path tree = dir / ".cache/huggingface/trees" / - (std::string(kNemotron35LightningNvfP4Revision) + - ".json"); - if (!fs::exists(dir / "config.json", ec)) return ""; - if (!fs::exists(tree, ec)) return ""; + if (!fs::exists(dir / "config.json", ec)) { + if (why != nullptr) *why = "no config.json under " + dir.string(); + return ""; + } + // Bound to a local rather than passed inline: tests/scripts/ + // test_check_snapshot_pins.py asserts each `k*Revision` constant is followed + // by a comma EXACTLY once in this header, so that a pin cannot exist without + // an accessor resolving through it. That single use is the HfSnapshot call + // below, and it stays the single use. + const std::string want = kNemotron35LightningNvfP4Revision; + if (!HfLocalDirIsRevision(dir, want, why)) return ""; return dir.string(); } - return HfSnapshot("models--nvidia--NVIDIA-Nemotron-3.5-Lightning-30B-A3B-NVFP4", - kNemotron35LightningNvfP4Revision, - "VT_NEMOTRON35_SNAPSHOT"); + const std::string resolved = HfSnapshot( + "models--nvidia--NVIDIA-Nemotron-3.5-Lightning-30B-A3B-NVFP4", + kNemotron35LightningNvfP4Revision, "VT_NEMOTRON35_SNAPSHOT"); + if (resolved.empty() && why != nullptr) { + *why = std::string("neither VT_NEMOTRON35_SNAPSHOT nor CHECKPOINT_ROOT " + "resolves a checkpoint at revision ") + + kNemotron35LightningNvfP4Revision; + } + return resolved; } // The 27B NVFP4 gate model, pinned to the goldens' revision. diff --git a/tests/parity/test_hf_snapshot_pinning.cpp b/tests/parity/test_hf_snapshot_pinning.cpp index b15bac4e1..e04ed85a6 100644 --- a/tests/parity/test_hf_snapshot_pinning.cpp +++ b/tests/parity/test_hf_snapshot_pinning.cpp @@ -58,6 +58,31 @@ class ScopedHome { bool had_prev_ = false; }; +// A variable forced UNSET for the duration. The local_dir arm +// below is only reached when `VT_NEMOTRON35_SNAPSHOT` is unset, and a developer +// who exports it in their own shell would otherwise silently take a DIFFERENT +// branch than the one the case is about -- which reads as a pass. +class ScopedUnsetEnv { + public: + explicit ScopedUnsetEnv(const char* name) : name_(name) { + const char* prev = std::getenv(name); + had_prev_ = prev != nullptr; + if (had_prev_) prev_ = prev; + unsetenv(name_); + } + ~ScopedUnsetEnv() { + if (had_prev_) setenv(name_, prev_.c_str(), /*overwrite=*/1); + } + + ScopedUnsetEnv(const ScopedUnsetEnv&) = delete; + ScopedUnsetEnv& operator=(const ScopedUnsetEnv&) = delete; + + private: + const char* name_; + std::string prev_; + bool had_prev_ = false; +}; + // Same for an env override, so an unset variable stays unset afterwards. class ScopedEnv { public: @@ -109,6 +134,59 @@ constexpr const char* kUnsloth27bRepo = "models--unsloth--Qwen3.6-27B-NVFP4"; constexpr const char* kUnsloth27bDecoyRevision = "ccdaab7e68af2409599b8949a8f2685703c9bae5"; +// ------------------------------------------------------------------------- // +// GATE-SNAPSHOT-CONTENT-PIN (issue #569): the `hf download --local-dir` arm. +// ------------------------------------------------------------------------- // + +// One `.cache/huggingface/download/.metadata` sidecar, in the layout +// huggingface_hub actually writes: commit hash, etag, timestamp, one per line +// (huggingface_hub/_local_folder.py module docstring, and +// `LocalDownloadFileMetadata` / `read_download_metadata` which parse it back in +// that order). +void PlantSidecar(const fs::path& dir, const std::string& file, + const std::string& commit) { + const fs::path meta = dir / ".cache/huggingface/download"; + fs::create_directories(meta); + std::ofstream(meta / (file + ".metadata")) + << commit << "\n" + << std::string(64, 'e') << "\n" + << "1786554554.7491977\n"; +} + +// The per-revision tree listing `hf download --local-dir` leaves behind. The +// body is deliberately a bare `{}`: the DEMONSTRATED failure this row closes had +// an empty `touch`ed manifest, because nothing ever read the contents. +void PlantTreeManifest(const fs::path& dir, const std::string& revision) { + const fs::path trees = dir / ".cache/huggingface/trees"; + fs::create_directories(trees); + std::ofstream(trees / (revision + ".json")) << "{}\n"; +} + +// A staged `hf download --local-dir` tree: the payload files a gate loads, the +// tree manifest for `manifest_revision`, and one sidecar per payload file naming +// `sidecar_revision`. Passing two different revisions is the substitution the +// old existence check could not see. +void PlantLocalDir(const fs::path& dir, const std::string& manifest_revision, + const std::string& sidecar_revision) { + fs::create_directories(dir); + for (const char* file : {"config.json", "hf_quant_config.json", + "model-00001-of-00001.safetensors"}) { + std::ofstream(dir / file) << "{}\n"; + PlantSidecar(dir, file, sidecar_revision); + } + PlantTreeManifest(dir, manifest_revision); +} + +// `$CHECKPOINT_ROOT/`, which is the ONE path the default arm +// resolves. +fs::path StagedCheckpoint(const fs::path& root) { + return root / parity::kNemotron35LightningLocalDirName; +} + +// A revision that is not the pin: the shape of "the publisher re-quantized and +// the same `hf download` command landed different bytes under the same path". +const std::string kOtherRevision(40, 'b'); + } // namespace TEST_CASE("hf_snapshot: the pinned revision is selected out of a two-revision cache") { @@ -224,3 +302,208 @@ TEST_CASE("hf_snapshot: the env override is the ONLY way to gate another checkpo std::string::npos); } } + +// --------------------------------------------------------------------------- // +// GATE-SNAPSHOT-CONTENT-PIN (issue #569). +// +// The Nemotron-3.5-Lightning gate model is staged as a `hf download --local-dir` +// tree, so the revision is not in the PATH. The pin used to be the EXISTENCE of +// `.cache/huggingface/trees/.json`, which records "this revision was +// downloaded into this directory once" and never "these bytes are that +// revision". The two come apart, and huggingface_hub says so itself: +// +// * `_tree_cache.py` (1.23.0/1.24.0, the versions that wrote the manifest on +// the gate host) opens with "Because a commit hash is immutable, its tree +// listing never changes and can be cached forever WITHOUT ANY INVALIDATION +// LOGIC", and `write_tree_cache` only ever `os.replace`s `.json`. +// Nothing in the library removes one. Manifests ACCUMULATE, so after a +// re-download at a new revision the OLD manifest is still sitting there +// vouching for the NEW bytes. +// * The per-file sidecar does not accumulate. `file_download.py` calls +// `write_download_metadata(..., commit_hash=...)` on EVERY outcome -- +// fresh download, cache copy, and the etag-already-matches early return -- +// so line 1 of `.cache/huggingface/download/.metadata` always names +// the revision the bytes on disk were fetched as. +// +// So the manifest is the wrong record and the sidecar is the right one. These +// cases pin that, and they are the reason W6 of `.agents/specs/nemotron-h-model.md` +// (#517) may be believed: a token-exact gate is precisely the instrument that +// CANNOT see a substituted checkpoint. +// --------------------------------------------------------------------------- // + +TEST_CASE("hf_snapshot: a local_dir whose sidecars name ANOTHER revision refuses") { + const fs::path root = ScratchRoot("nemotron_substituted"); + ScopedHome guard(root); // also cleans `root` up on scope exit + ScopedUnsetEnv no_override("VT_NEMOTRON35_SNAPSHOT"); + ScopedEnv checkpoint_root("CHECKPOINT_ROOT", root.string()); + + // The DEMONSTRATED failure, staged exactly: the pinned revision's manifest is + // present, and every file in the tree was actually downloaded at a different + // one. An existence check on the manifest resolves this directory. + const fs::path staged = StagedCheckpoint(root); + PlantLocalDir(staged, parity::kNemotron35LightningNvfP4Revision, + kOtherRevision); + + std::string why; + const std::string got = parity::Nemotron35LightningSnapshot(&why); + CHECK(got.empty()); + // The refusal has to SAY what was wrong, or the exit-77 banner the callers + // print degenerates into "not staged" and the operator re-runs it. WHICH file + // is named is readdir order and deliberately not asserted; that it names the + // revision actually on disk is the part an operator needs. + CHECK(why.find(kOtherRevision) != std::string::npos); + CHECK(why.find("not " + + std::string(parity::kNemotron35LightningNvfP4Revision)) != + std::string::npos); +} + +TEST_CASE("hf_snapshot: a RIGHT config beside WRONG weights refuses") { + const fs::path root = ScratchRoot("nemotron_weights_only"); + ScopedHome guard(root); + ScopedUnsetEnv no_override("VT_NEMOTRON35_SNAPSHOT"); + ScopedEnv checkpoint_root("CHECKPOINT_ROOT", root.string()); + + // The case that decides config-only versus per-file, staged as the difference + // between them: `config.json` is genuinely the pinned revision, the SHARD the + // goldens actually depend on is not. `hf download --revision B` + // into a tree fetched at A produces exactly this, and a resolver that read one + // sidecar would hand the gate a mixed checkpoint and call it pinned. + const fs::path staged = StagedCheckpoint(root); + PlantLocalDir(staged, parity::kNemotron35LightningNvfP4Revision, + parity::kNemotron35LightningNvfP4Revision); + PlantSidecar(staged, "model-00001-of-00001.safetensors", kOtherRevision); + + std::string why; + CHECK(parity::Nemotron35LightningSnapshot(&why).empty()); + CHECK(why.find("model-00001-of-00001.safetensors") != std::string::npos); + CHECK(why.find(kOtherRevision) != std::string::npos); +} + +TEST_CASE("hf_snapshot: a local_dir whose sidecars name the PIN resolves") { + const fs::path root = ScratchRoot("nemotron_matching"); + ScopedHome guard(root); + ScopedUnsetEnv no_override("VT_NEMOTRON35_SNAPSHOT"); + ScopedEnv checkpoint_root("CHECKPOINT_ROOT", root.string()); + + const fs::path staged = StagedCheckpoint(root); + PlantLocalDir(staged, parity::kNemotron35LightningNvfP4Revision, + parity::kNemotron35LightningNvfP4Revision); + + std::string why; + CHECK(parity::Nemotron35LightningSnapshot(&why) == staged.string()); + CHECK(why.empty()); + // The no-argument spelling every existing caller uses stays identical. + CHECK(parity::Nemotron35LightningSnapshot() == staged.string()); +} + +TEST_CASE("hf_snapshot: an ACCUMULATED manifest does not vouch for newer bytes") { + const fs::path root = ScratchRoot("nemotron_accumulated"); + ScopedHome guard(root); + ScopedUnsetEnv no_override("VT_NEMOTRON35_SNAPSHOT"); + ScopedEnv checkpoint_root("CHECKPOINT_ROOT", root.string()); + + // What `hf download --local-dir` leaves behind after fetching the pin and then + // re-fetching the same repo at a later revision: BOTH manifests, and sidecars + // rewritten in place to the later one. Nothing here is hypothetical -- it is + // what the writer's own "cached forever without any invalidation logic" means. + const fs::path staged = StagedCheckpoint(root); + PlantLocalDir(staged, kOtherRevision, kOtherRevision); + PlantTreeManifest(staged, parity::kNemotron35LightningNvfP4Revision); + + std::string why; + CHECK(parity::Nemotron35LightningSnapshot(&why).empty()); + CHECK(why.find(kOtherRevision) != std::string::npos); +} + +TEST_CASE("hf_snapshot: a local_dir file with NO sidecar refuses") { + const fs::path root = ScratchRoot("nemotron_unrecorded"); + ScopedHome guard(root); + ScopedUnsetEnv no_override("VT_NEMOTRON35_SNAPSHOT"); + ScopedEnv checkpoint_root("CHECKPOINT_ROOT", root.string()); + + const fs::path staged = StagedCheckpoint(root); + PlantLocalDir(staged, parity::kNemotron35LightningNvfP4Revision, + parity::kNemotron35LightningNvfP4Revision); + + SUBCASE("a shard dropped in by hand carries no provenance at all") { + // The check is symmetric on purpose. If only the files that HAVE a sidecar + // were checked, substituting a checkpoint would cost one `rm` of the + // sidecar -- and the file whose bytes the goldens depend on is exactly the + // one an attacker or an accident replaces. + std::ofstream(staged / "model-00002-of-00002.safetensors") << "{}\n"; + std::string why; + CHECK(parity::Nemotron35LightningSnapshot(&why).empty()); + CHECK(why.find("model-00002-of-00002.safetensors") != std::string::npos); + // The MESSAGE is asserted, not only the refusal. Falling through to the + // wrong-revision branch also refuses -- "" never equals the pin -- so + // without this the no-sidecar branch could be deleted outright and every + // case here would still pass while the banner told the operator the file + // "was downloaded at revision , not 29f2d174...", which is not what + // happened and sends them looking for the wrong thing. + CHECK(why.find("has no .cache/huggingface/download/") != std::string::npos); + } + SUBCASE("deleting a sidecar is not a way to pass") { + fs::remove(staged / ".cache/huggingface/download/config.json.metadata"); + std::string why; + CHECK(parity::Nemotron35LightningSnapshot(&why).empty()); + CHECK(why.find("config.json") != std::string::npos); + CHECK(why.find("has no .cache/huggingface/download/") != std::string::npos); + } +} + +TEST_CASE("hf_snapshot: a tree with no staged FILE refuses rather than passing") { + const fs::path root = ScratchRoot("nemotron_vacuous"); + ScopedHome guard(root); + ScopedUnsetEnv no_override("VT_NEMOTRON35_SNAPSHOT"); + ScopedEnv checkpoint_root("CHECKPOINT_ROOT", root.string()); + + // `config.json` as a DIRECTORY. `fs::exists` says yes, so the arm gets past + // the staging probe, and the sweep then has zero regular files to check. A + // sweep with nothing to check must not report success: "every file matches" + // is vacuously true of no files, and that is the one way a content pin can go + // quiet without anyone deleting a line of it. + const fs::path staged = StagedCheckpoint(root); + fs::create_directories(staged / "config.json"); + + std::string why; + CHECK(parity::Nemotron35LightningSnapshot(&why).empty()); + CHECK(why.find("no files staged") != std::string::npos); +} + +TEST_CASE("hf_snapshot: the local_dir arm keeps the behaviors it already had") { + const fs::path root = ScratchRoot("nemotron_preserved"); + ScopedHome guard(root); + + SUBCASE("no CHECKPOINT_ROOT and no override -> refuse") { + ScopedUnsetEnv no_override("VT_NEMOTRON35_SNAPSHOT"); + ScopedUnsetEnv no_root("CHECKPOINT_ROOT"); + CHECK(parity::Nemotron35LightningSnapshot().empty()); + } + SUBCASE("CHECKPOINT_ROOT set but nothing staged under it -> refuse") { + ScopedUnsetEnv no_override("VT_NEMOTRON35_SNAPSHOT"); + ScopedEnv checkpoint_root("CHECKPOINT_ROOT", root.string()); + CHECK(parity::Nemotron35LightningSnapshot().empty()); + } + SUBCASE("the override names a directory outright and wins over the root") { + // Unchanged semantics: naming ONE directory is the deliberate + // different-checkpoint run, and it is not revision-gated. Naming a ROOT is + // not, which is why only the root arm carries the content pin. + const fs::path elsewhere = root / "deliberate-checkpoint"; + fs::create_directories(elsewhere); + std::ofstream(elsewhere / "config.json") << "{}\n"; + PlantLocalDir(StagedCheckpoint(root), + parity::kNemotron35LightningNvfP4Revision, + parity::kNemotron35LightningNvfP4Revision); + ScopedEnv checkpoint_root("CHECKPOINT_ROOT", root.string()); + ScopedEnv over("VT_NEMOTRON35_SNAPSHOT", elsewhere.string()); + CHECK(parity::Nemotron35LightningSnapshot() == elsewhere.string()); + } + SUBCASE("a set-but-wrong override refuses rather than falling back") { + PlantLocalDir(StagedCheckpoint(root), + parity::kNemotron35LightningNvfP4Revision, + parity::kNemotron35LightningNvfP4Revision); + ScopedEnv checkpoint_root("CHECKPOINT_ROOT", root.string()); + ScopedEnv over("VT_NEMOTRON35_SNAPSHOT", (root / "nonexistent").string()); + CHECK(parity::Nemotron35LightningSnapshot().empty()); + } +} diff --git a/tests/vllm/model_executor/layers/quantization/test_modelopt_mixed_precision_checkpoint.cpp b/tests/vllm/model_executor/layers/quantization/test_modelopt_mixed_precision_checkpoint.cpp index 1f6709ef1..70e92209a 100644 --- a/tests/vllm/model_executor/layers/quantization/test_modelopt_mixed_precision_checkpoint.cpp +++ b/tests/vllm/model_executor/layers/quantization/test_modelopt_mixed_precision_checkpoint.cpp @@ -51,10 +51,18 @@ namespace { // staging directory name by hand, which meant two env vars — // `VT_NEMOTRON35_SNAPSHOT` there, `CHECKPOINT_ROOT` here — reached one // checkpoint and neither refused a revision the goldens were not captured -// against. Both spellings now go through that resolver, which gates the staged -// `local_dir` on its own `.cache/huggingface/trees/.json` manifest, so -// a re-download landing a different revision under the identical path SKIPS -// here rather than being silently substituted. +// against. Both spellings now go through that resolver. +// +// GATE-SNAPSHOT-CONTENT-PIN (#569). That resolver gates the staged `local_dir` +// on the per-file `.cache/huggingface/download/.metadata` sidecars, whose +// first line is the revision the bytes were downloaded at, so a re-download +// landing a different revision under the identical path SKIPS here rather than +// being silently substituted. It used to gate on the presence of +// `.cache/huggingface/trees/.json`, which records only that the +// revision was fetched here once and is never deleted when a later one lands. +// The resolver now hands back the reason it refused, and the banner below prints +// it verbatim -- a skip that does not say WHICH file named WHICH revision gets +// re-run instead of investigated. // // HOW TO MAKE THIS ARM RUN. `CHECKPOINT_ROOT` is a `.env` key, and `.env` is not // exported into a login shell by anything: `.env.example:8` documents the loader @@ -65,12 +73,13 @@ namespace { // has not sourced it SKIPS this arm, which is correct behavior and not a pass — // the banner below names the exact export. std::string CheckpointFile(const char* filename) { - const std::string snapshot = parity::Nemotron35LightningSnapshot(); + std::string why; + const std::string snapshot = parity::Nemotron35LightningSnapshot(&why); if (snapshot.empty()) { SkipGate( std::string("no pinned Nemotron-3.5-Lightning snapshot, so ") + - filename + - " cannot be located.\n" + filename + " cannot be located.\n*** REFUSED BECAUSE: " + why + + "\n" "*** To RUN this arm, load the repo env first — `.env.example:8`\n" "*** documents exactly this:\n" "*** set -a; . ./.env; set +a\n" @@ -81,10 +90,11 @@ std::string CheckpointFile(const char* filename) { "*** $CHECKPOINT_ROOT/" + parity::kNemotron35LightningLocalDirName + "/{config,hf_quant_config}.json — no weights, no GPU.\n" - "*** A staged directory that IS present skips too unless it carries\n" - "*** .cache/huggingface/trees/" + + "*** A staged directory that IS present skips too unless EVERY file\n" + "*** in it records revision " + parity::kNemotron35LightningNvfP4Revision + - ".json,\n" + "\n" + "*** on line 1 of its .cache/huggingface/download/.metadata —\n" "*** the revision the committed goldens were captured against."); } const std::filesystem::path p = std::filesystem::path(snapshot) / filename;