diff --git a/studio/backend/hub/services/models/cache_inventory.py b/studio/backend/hub/services/models/cache_inventory.py index a27e4860e6..1f38af9381 100644 --- a/studio/backend/hub/services/models/cache_inventory.py +++ b/studio/backend/hub/services/models/cache_inventory.py @@ -49,6 +49,10 @@ _REPO_SIZE_NEG_TTL = 60.0 _MODEL_METADATA_TIMEOUT_SECONDS = 5.0 _repo_size_cache_lock = threading.Lock() +# Identity for a cached file with no HF blob (Windows without Developer Mode: hf +# moves the blob into snapshots/ and leaves blobs/ empty). +_LOCAL_SIZE_IDENTITY_PREFIX = "size:" + def get_repo_snapshot_metadata_cached( repo_id: str, hf_token: Optional[str] = None @@ -135,23 +139,52 @@ def _cached_repo_file_name(file_obj) -> str: return str(getattr(file_obj, "file_name", "")).replace("\\", "/") +def _is_real_cache_blob(blob: Optional[Path], repo_dir: Optional[Path]) -> bool: + """True only for a real cache blob at ``/blobs/``. + + A no-symlink ``snapshots/`` file (name is the filename, not an etag) or a + repo's own ``blobs/`` subdir is not the cache blob store. + """ + if blob is None or repo_dir is None: + return False + try: + return blob.parent.resolve(strict = False) == (repo_dir / "blobs").resolve(strict = False) + except OSError: + return False + + +def _cached_blob_hash(blob_path, repo_path = None) -> Optional[str]: + """The cache blob hash (etag) for a cached file, or None when there is no blob. + + Only a real blob under the repo's ``blobs/`` dir has name == hash; a moved + no-symlink ``snapshots/`` file is "no blob", so the caller uses a size identity. + """ + path = Path(blob_path) + repo_dir = Path(repo_path) if repo_path is not None else None + return path.name if _is_real_cache_blob(path, repo_dir) else None + + +def local_size_identity(size: int) -> str: + """Identity for a cached file whose blob hash is unknowable: its size. + + Re-hashing multi-GB GGUFs on the inventory hot path is not viable, and a + ``size:`` token never collides with a hex hash. + """ + return f"{_LOCAL_SIZE_IDENTITY_PREFIX}{int(size)}" + + def _repo_gguf_blob_map(repo_info, *, include_companions: bool = False) -> dict[str, set[str]]: """Map each cached GGUF file's repo-relative name to the SET of its local - blob hashes across all cached revisions. + identities across all revisions. - HF names each local cache blob FILE by the file's etag (lfs.sha256 else - blob_id), so a local file's blob hash == ``Path(blob_path).name``. An updated - repo keeps BOTH the old and new revision snapshots until HF garbage-collects - them, so the same file resolves to several blobs; collecting them ALL (not - just the first one seen, since ``repo_info.revisions`` is a frozenset and - yields them in arbitrary order) lets the remote-vs-local diff treat the file - as current when the remote (``main``) blob is present in any cached revision. - Mirrors the ``cached_blob_ids`` membership test in routes/models.py. - - By default this keeps the historical MAIN-GGUF-only behavior. GGUF update - checks opt into companions so a shared mmproj/MTP blob can be compared too. + An identity is the file's blob hash, or a size identity when the cache holds no + blob (Windows without Developer Mode). BOTH old and new revision blobs are kept + (a set), so the diff treats the file as current when the remote ``main`` blob is + in any cached revision. Main GGUF only by default; update checks opt into + companions to compare a shared mmproj/MTP blob too. """ blob_map: dict[str, set[str]] = {} + repo_path = getattr(repo_info, "repo_path", None) for revision in repo_info.revisions: for f in revision.files: if include_companions: @@ -163,7 +196,13 @@ def _repo_gguf_blob_map(repo_info, *, include_companions: bool = False) -> dict[ if not blob_path: continue name = _cached_repo_file_name(f) - blob_map.setdefault(name, set()).add(Path(blob_path).name) + identity = _cached_blob_hash(blob_path, repo_path) + if identity is None: + size = int(getattr(f, "size_on_disk", 0) or 0) + if size <= 0: + continue + identity = local_size_identity(size) + blob_map.setdefault(name, set()).add(identity) return blob_map diff --git a/studio/backend/hub/services/models/deletion.py b/studio/backend/hub/services/models/deletion.py index e7c54fc75b..636a223d4e 100644 --- a/studio/backend/hub/services/models/deletion.py +++ b/studio/backend/hub/services/models/deletion.py @@ -408,7 +408,13 @@ def reclaim_replaced_gguf_variant( and extract_quant_label(name).lower() == variant_key, ) for snap, blob, name in matches: - blob_hash = _blob_hash_from_path(blob) if blob is not None else None + # Prune only a file we can identify as a real, stale cache blob. A + # no-symlink snapshot file has no identifiable blob hash, so keep it. + blob_hash = ( + _blob_hash_from_path(blob) + if cache_inventory._is_real_cache_blob(blob, repo_dir) + else None + ) if blob_hash is None or blob_hash in keep_main_hashes: continue stale_matches.append((snap, blob, name)) diff --git a/studio/backend/hub/services/models/gguf_variants.py b/studio/backend/hub/services/models/gguf_variants.py index 0147bba19a..33f0297ff5 100644 --- a/studio/backend/hub/services/models/gguf_variants.py +++ b/studio/backend/hub/services/models/gguf_variants.py @@ -337,6 +337,22 @@ def _local_main_gguf_blobs_by_quant(repo_id: str) -> dict[str, dict[str, set[str return result +def _size_identity_matches(local_set: set[str], remote_size: int) -> bool: + """Whether a cached file with NO blob hash is current, judged by size. + + A size token only lands in ``local_set`` for a file the cache has no blob for, + so it never loosens the hash comparison for a normal file. Tradeoff: an + equal-size requant is missed, versus the status quo where every no-blob GGUF + shows a phantom update that no re-download clears. + """ + size = int(remote_size or 0) + if size <= 0: + return False + from hub.services.models import cache_inventory + + return cache_inventory.local_size_identity(size) in local_set + + def _variant_update_available_from_requirement( local_blobs: dict[str, set[str]], requirement: Optional[_GgufVariantRequirement], variant: str ) -> bool: @@ -355,8 +371,13 @@ def _variant_update_available_from_requirement( if not remote_blob: continue local_set = local_by_posix.get(path) - if not local_set or remote_blob not in local_set: + if not local_set: return True + if remote_blob in local_set: + continue + if _size_identity_matches(local_set, expected.size): + continue + return True return False diff --git a/studio/backend/tests/test_model_update_robustness.py b/studio/backend/tests/test_model_update_robustness.py index 300eb587b3..edf55812e2 100644 --- a/studio/backend/tests/test_model_update_robustness.py +++ b/studio/backend/tests/test_model_update_robustness.py @@ -382,14 +382,148 @@ def test_repo_gguf_blob_map_collects_all_revision_blobs(): """Every cached revision's blob for a gguf file is kept as a set, not collapsed to one arbitrary blob.""" repo_info = SimpleNamespace( + repo_path = "/", # real blobs live at /blobs/ revisions = [ _rev(("lfm2-350m-q4_k_m.gguf", "OLDsha")), _rev(("lfm2-350m-q4_k_m.gguf", "NEWsha")), - ] + ], ) assert CI._repo_gguf_blob_map(repo_info) == {"lfm2-350m-q4_k_m.gguf": {"OLDsha", "NEWsha"}} +# ── no-symlink (Windows without Developer Mode) GGUF update detection ── +# +# Regression for the phantom "Update available" that NEVER clears (#7060). Without +# the symlink privilege, hf_hub_download MOVES the blob into snapshots/ instead of +# symlinking it, so blobs/ is empty and scan_cache_dir reports blob_path = the +# snapshot file. Its name is the FILENAME, not an etag, so a remote-vs-local sha256 +# comparison can never match and every cached GGUF reports an update forever -- +# which re-downloading cannot fix, since the same file is rewritten with no blob. + + +def _rev_no_symlink(*files): + """A revision whose GGUFs were MOVED into snapshots/ (no blobs/ entry).""" + return SimpleNamespace( + files = [ + SimpleNamespace( + file_name = name, + blob_path = f"/hf/models--org--repo/snapshots/{'a' * 40}/{name}", + size_on_disk = size, + ) + for name, size in files + ] + ) + + +def _requirement(*expected): + from hub.utils.download_manifest import ExpectedFile + from hub.utils.gguf_plan import GgufVariantPlan + + expected_files = tuple( + ExpectedFile(path = path, size = size, sha256 = sha) for path, size, sha in expected + ) + return GgufVariantPlan( + main_filenames = frozenset(e.path for e in expected_files), + target_filenames = tuple(e.path for e in expected_files), + main_hashes = frozenset(e.sha256 for e in expected_files if e.sha256), + required_hashes = frozenset(e.sha256 for e in expected_files if e.sha256), + companion_hashes = frozenset(), + mmproj_filenames = frozenset(), + mmproj_hashes = frozenset(), + expected_files = expected_files, + main_size_bytes = sum(e.size for e in expected_files), + download_size_bytes = sum(e.size for e in expected_files), + ) + + +def test_repo_gguf_blob_map_uses_size_identity_when_cache_has_no_blob(): + """A snapshot-resident GGUF (no blobs/ entry) must NOT be recorded under its + filename as if that were a hash -- it gets a size identity instead.""" + repo_info = SimpleNamespace( + repo_path = "/hf/models--org--repo", + revisions = [_rev_no_symlink(("model-Q4_K_M.gguf", 4096))], + ) + + assert CI._repo_gguf_blob_map(repo_info) == { + "model-Q4_K_M.gguf": {CI.local_size_identity(4096)} + } + + +def test_repo_gguf_blob_map_skips_snapshot_file_with_unknown_size(): + """No blob and no readable size means no identity at all, rather than a + filename masquerading as a hash.""" + repo_info = SimpleNamespace( + repo_path = "/hf/models--org--repo", + revisions = [_rev_no_symlink(("model-Q4_K_M.gguf", 0))], + ) + + assert CI._repo_gguf_blob_map(repo_info) == {} + + +def test_repo_gguf_blob_map_ignores_repo_blobs_subdir_on_no_symlink(): + """A repo that ships a GGUF under its own blobs/ subdir lands at + snapshots//blobs/model.gguf on a no-symlink cache. Its parent is named + 'blobs' but it is NOT the cache blob store, so it gets a size identity rather + than having its filename recorded as a hash (which would show a phantom update).""" + repo_path = "/hf/models--org--repo" + repo_info = SimpleNamespace( + repo_path = repo_path, + revisions = [ + SimpleNamespace( + files = [ + SimpleNamespace( + file_name = "model-Q4_K_M.gguf", + blob_path = f"{repo_path}/snapshots/{'a' * 40}/blobs/model-Q4_K_M.gguf", + size_on_disk = 4096, + ) + ] + ) + ], + ) + + assert CI._repo_gguf_blob_map(repo_info) == { + "model-Q4_K_M.gguf": {CI.local_size_identity(4096)} + } + + +def test_no_symlink_cache_matching_remote_size_reports_no_update(): + """The #7060 repro: a GGUF stored directly in snapshots/ whose size matches the + remote is CURRENT, and must not show a phantom 'update available'.""" + local_blobs = {"model-Q4_K_M.gguf": {CI.local_size_identity(4096)}} + requirement = _requirement(("model-Q4_K_M.gguf", 4096, "REMOTEsha256")) + + assert ( + GV._variant_update_available_from_requirement(local_blobs, requirement, "Q4_K_M") is False + ) + + +def test_no_symlink_cache_with_different_remote_size_still_reports_update(): + """A genuine upstream change is still detected in the no-symlink layout.""" + local_blobs = {"model-Q4_K_M.gguf": {CI.local_size_identity(4096)}} + requirement = _requirement(("model-Q4_K_M.gguf", 8192, "REMOTEsha256")) + + assert GV._variant_update_available_from_requirement(local_blobs, requirement, "Q4_K_M") is True + + +def test_symlinked_cache_with_stale_blob_still_reports_update(): + """The blob-hash path is untouched: a real blob that does not match the remote + sha256 is still stale, and a size-identity fallback must not rescue it.""" + local_blobs = {"model-Q4_K_M.gguf": {"OLDsha"}} + requirement = _requirement(("model-Q4_K_M.gguf", 4096, "NEWsha")) + + assert GV._variant_update_available_from_requirement(local_blobs, requirement, "Q4_K_M") is True + + +def test_symlinked_cache_with_current_blob_reports_no_update(): + """The blob-hash path is untouched: a matching blob is current.""" + local_blobs = {"model-Q4_K_M.gguf": {"OLDsha", "NEWsha"}} + requirement = _requirement(("model-Q4_K_M.gguf", 4096, "NEWsha")) + + assert ( + GV._variant_update_available_from_requirement(local_blobs, requirement, "Q4_K_M") is False + ) + + def test_reclaim_replaced_gguf_variant_prunes_old_revision_only(monkeypatch, tmp_path): """After a verified update, stale same-variant files/blobs are removed while the freshly downloaded hash and sibling variants remain cached.""" @@ -463,3 +597,42 @@ def test_reclaim_replaced_gguf_variant_prunes_old_revision_only(monkeypatch, tmp assert sibling_snap.exists() is True assert sibling_blob.exists() is True assert invalidated == [True] + + +def test_reclaim_replaced_gguf_variant_keeps_no_symlink_current_file(monkeypatch, tmp_path): + """No-symlink cache (Windows without Developer Mode): the moved GGUF lives + directly in snapshots/ and blobs/ is empty, so scan_cache_dir reports + blob_path == the snapshot file and its name is the FILENAME, not an etag. + Reclaim must NOT mistake that filename for a stale hash and delete the + freshly-downloaded current file.""" + repo_id = "org/repo-GGUF" + repo_path = tmp_path / "models--org--repo-GGUF" + snap = repo_path / "snapshots" / ("a" * 40) / "model-Q4_K_M.gguf" + snap.parent.mkdir(parents = True, exist_ok = True) + snap.write_bytes(b"current-download") + (repo_path / "blobs").mkdir(parents = True, exist_ok = True) # empty: moved, not linked + + repo_info = SimpleNamespace( + repo_id = repo_id, + repo_type = "model", + repo_path = repo_path, + revisions = [ + SimpleNamespace( + files = [ + SimpleNamespace( + file_name = "model-Q4_K_M.gguf", + file_path = str(snap), + blob_path = str(snap), # no-symlink: blob_path == the snapshot file + ) + ] + ) + ], + ) + monkeypatch.setattr(CI, "all_hf_cache_scans", lambda: [SimpleNamespace(repos = [repo_info])]) + monkeypatch.setattr(CI, "invalidate_hf_cache_scans", lambda: None) + + result = D.reclaim_replaced_gguf_variant(repo_id, "Q4_K_M", frozenset({"REMOTEsha256"})) + + assert snap.exists() is True # the current file must survive + assert result["removed_snapshots"] == 0 + assert result["deleted_blobs"] == 0