From 7b752dabf92534c776e6406768b7d356b4e93146 Mon Sep 17 00:00:00 2001 From: Himanshu Kumar Date: Sun, 19 Jul 2026 17:06:50 +0530 Subject: [PATCH] fix(ollama): cloud-model guard, safe update, safe cache clean (issue #156 M1-M3) M1: OllamaRuntime.remove() now short-circuits cloud/subscription models (tag contains 'cloud') with a clear DownloadError instead of blocking on a daemon delete. Added ModelRef.is_cloud. M2: ModelManager.update() now requires confirm=True (destructive remove+re-pull) and rejects cloud models. SDK update() passes confirm through. M3: CachePort.clean()/FilesystemCache.clean()/CacheService.clean() are safe by default (only corrupt/partial entries removed) and accept force=True to wipe all. No longer deletes every valid entry. Co-Authored-By: Claude --- src/modeldock/__init__.py | 8 ++--- src/modeldock/adapters/cache/filesystem.py | 13 +++---- src/modeldock/adapters/runtimes/ollama.py | 10 ++++++ src/modeldock/core/cache.py | 10 ++++-- src/modeldock/core/manager.py | 26 ++++++++++++-- src/modeldock/domain/model.py | 9 +++++ src/modeldock/ports/cache.py | 8 +++-- tests/conftest.py | 7 ++-- tests/unit/test_cache.py | 42 +++++++++++++++++----- tests/unit/test_core.py | 21 +++++++++-- tests/unit/test_ollama_runtime.py | 9 +++++ tests/unit/test_port_contract.py | 4 +++ 12 files changed, 135 insertions(+), 32 deletions(-) diff --git a/src/modeldock/__init__.py b/src/modeldock/__init__.py index 467744b..958e4bd 100644 --- a/src/modeldock/__init__.py +++ b/src/modeldock/__init__.py @@ -102,11 +102,11 @@ def install_category(category: str, backend: Optional[str] = None) -> List[Model return _manager().install_category(category) -def update(name: str, backend: Optional[str] = None) -> ModelRef: - """Pull a newer tag.""" +def update(name: str, backend: Optional[str] = None, confirm: bool = False) -> ModelRef: + """Pull a newer tag (destructive: removes then re-downloads).""" if backend is not None: - return Manager(backend=backend).update(name) - return _manager().update(name) + return Manager(backend=backend).update(name, confirm=confirm) + return _manager().update(name, confirm=confirm) def remove(name: str, backend: Optional[str] = None) -> None: diff --git a/src/modeldock/adapters/cache/filesystem.py b/src/modeldock/adapters/cache/filesystem.py index e03defa..25a1703 100644 --- a/src/modeldock/adapters/cache/filesystem.py +++ b/src/modeldock/adapters/cache/filesystem.py @@ -72,16 +72,17 @@ def get_record(self, ref: ModelRef) -> Optional[Dict[str, Any]]: entry = data.get("entries", {}).get(self._key(ref)) return cast(Optional[Dict[str, Any]], entry) - def clean(self) -> List[str]: + def clean(self, force: bool = False) -> List[str]: removed: List[str] = [] data = self._read_manifest() entries = data.get("entries", {}) for key, entry in list(entries.items()): - # Remove entries whose artifact file is missing. - name = entry.get("name", "") - artifact = self._cache_dir / f"{name}.bin" - if not artifact.exists(): - removed.append(str(artifact)) + # Safe default: only drop entries that are corrupt/partial (missing + # the fields we recorded). ModelDock does not manage the model blobs + # for Ollama, so a missing artifact file is NOT grounds for removal. + # force=True wipes every entry. + if force or not isinstance(entry, dict) or not entry.get("sha256"): + removed.append(key) del entries[key] if removed: self._write_manifest(data) diff --git a/src/modeldock/adapters/runtimes/ollama.py b/src/modeldock/adapters/runtimes/ollama.py index 8ded235..3e1e6d0 100644 --- a/src/modeldock/adapters/runtimes/ollama.py +++ b/src/modeldock/adapters/runtimes/ollama.py @@ -185,6 +185,16 @@ def _get_client(self, ref: ModelRef) -> Any: return self._ensure_client() def remove(self, ref: ModelRef) -> None: + # Cloud/subscription models are not managed by the local runtime; a + # delete call would block on the remote service. Fail fast instead. + if ref.is_cloud: + raise DownloadError( + ref.name, + reason=( + f"{ref.qualified_name()} is a cloud/subscription model and " + "cannot be removed locally." + ), + ) client = self._ensure_client() try: client.delete(ref.qualified_name()) diff --git a/src/modeldock/core/cache.py b/src/modeldock/core/cache.py index ca7a828..827f48c 100644 --- a/src/modeldock/core/cache.py +++ b/src/modeldock/core/cache.py @@ -29,9 +29,13 @@ def status(self) -> List[Dict[str, Any]]: """Return a snapshot of cached entries.""" return self._cache.status() - def clean(self) -> List[str]: - """Remove orphaned/partial artifacts; return removed paths.""" - return self._cache.clean() + def clean(self, force: bool = False) -> List[str]: + """Remove orphaned/partial artifacts; return removed paths. + + Safe by default (only corrupt/partial entries); pass ``force=True`` to + wipe every cached entry. + """ + return self._cache.clean(force=force) def path(self) -> str: """Return the cache directory path.""" diff --git a/src/modeldock/core/manager.py b/src/modeldock/core/manager.py index 83d43d9..619103d 100644 --- a/src/modeldock/core/manager.py +++ b/src/modeldock/core/manager.py @@ -15,6 +15,7 @@ from modeldock.adapters.runtimes.registry import RuntimeRegistry from modeldock.common.config import Settings from modeldock.common.errors import ( + DownloadError, ModelNotFoundError, RuntimeUnavailableError, ) @@ -164,11 +165,32 @@ def install_category(self, category: str) -> List[ModelRef]: refs.append(ref) return refs - def update(self, name: str) -> ModelRef: - """Pull a newer tag for an installed model.""" + def update(self, name: str, confirm: bool = False) -> ModelRef: + """Pull a newer tag for an installed model. + + Destructive: removes the current copy and re-downloads. Requires + ``confirm=True`` to proceed, so a large model is never re-pulled by + accident. Cloud/subscription models cannot be updated locally. + """ ref = ModelRef.parse(name, backend=self._backend) + if ref.is_cloud: + raise DownloadError( + ref.name, + reason=( + f"{ref.qualified_name()} is a cloud/subscription model and " + "cannot be updated locally." + ), + ) if not self._runtime.is_installed(ref): raise ModelNotFoundError(name) + if not confirm: + raise DownloadError( + ref.name, + reason=( + f"update() removes and re-downloads {ref.qualified_name()}. " + "Pass confirm=True to proceed." + ), + ) self._runtime.remove(ref) self._download.pull(ref) return ref diff --git a/src/modeldock/domain/model.py b/src/modeldock/domain/model.py index c3b95dd..f03dd54 100644 --- a/src/modeldock/domain/model.py +++ b/src/modeldock/domain/model.py @@ -236,6 +236,15 @@ def qualified_name(self) -> str: """Return ``name:tag`` form used by runtimes.""" return f"{self.name}:{self.tag}" + @property + def is_cloud(self) -> bool: + """True for cloud/subscription models (tag contains ``cloud``). + + These cannot be installed, run, or removed through a local runtime and + must be short-circuited with a clear error instead of a daemon call. + """ + return "cloud" in self.tag + def __hash__(self) -> int: return hash((self.name, self.tag, self.backend)) diff --git a/src/modeldock/ports/cache.py b/src/modeldock/ports/cache.py index c12313e..3e1740b 100644 --- a/src/modeldock/ports/cache.py +++ b/src/modeldock/ports/cache.py @@ -27,8 +27,12 @@ def get_record(self, ref: ModelRef) -> Optional[Dict[str, Any]]: """Return the cached manifest entry for ``ref``, if any.""" ... - def clean(self) -> List[str]: - """Remove orphaned/partial artifacts; return removed paths.""" + def clean(self, force: bool = False) -> List[str]: + """Remove orphaned/partial artifacts; return removed paths. + + Safe by default: only corrupt/partial manifest entries are removed. + Pass ``force=True`` to wipe every cached entry. + """ ... def status(self) -> List[Dict[str, Any]]: diff --git a/tests/conftest.py b/tests/conftest.py index 43fce64..ef74b6b 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -82,10 +82,11 @@ def record(self, ref: ModelRef, tag: str, sha256: str, size_bytes: int) -> None: def get_record(self, ref: ModelRef) -> Optional[dict]: return self.entries.get(self._key(ref)) - def clean(self) -> List[str]: - removed = [f"{k}" for k in self.entries] + def clean(self, force: bool = False) -> List[str]: + removed = [k for k, v in self.entries.items() if force or not v.get("sha256")] + for k in removed: + del self.entries[k] self.cleaned.extend(removed) - self.entries.clear() return removed def status(self) -> List[dict]: diff --git a/tests/unit/test_cache.py b/tests/unit/test_cache.py index c0aed9e..2faae7a 100644 --- a/tests/unit/test_cache.py +++ b/tests/unit/test_cache.py @@ -25,8 +25,11 @@ def test_cache_service_status_and_clean(fake_cache: object) -> None: ref = ModelRef.parse("llama3") svc.record(ref, "latest", "abc", 10) assert len(svc.status()) == 1 - removed = svc.clean() - assert removed + # Safe default keeps a valid entry. + assert svc.clean() == [] + assert len(svc.status()) == 1 + # force=True wipes it. + assert svc.clean(force=True) assert svc.status() == [] @@ -49,23 +52,44 @@ def test_filesystem_cache_persists_across_instances(tmp_path: Path) -> None: assert again.is_fresh(ref) -def test_filesystem_cache_clean_removes_missing_artifacts(tmp_path: Path) -> None: +def test_filesystem_cache_clean_keeps_valid_entries_by_default(tmp_path: Path) -> None: + cache = FilesystemCache(tmp_path) + ref = ModelRef.parse("llama3") + cache.record(ref, "latest", "x", 1) + # Safe default: a valid entry is kept even though no .bin artifact exists. + assert cache.clean() == [] + assert cache.is_fresh(ref) + + +def test_filesystem_cache_clean_removes_corrupt_entries(tmp_path: Path) -> None: cache = FilesystemCache(tmp_path) ref = ModelRef.parse("llama3") cache.record(ref, "latest", "x", 1) - # No artifact file was written, so clean() should report it removed. + # Corrupt an entry (drop sha256) to simulate a partial/corrupt record. + data = cache._read_manifest() + data["entries"]["llama3:latest"].pop("sha256") + cache._write_manifest(data) removed = cache.clean() - assert removed + assert removed == ["llama3:latest"] assert cache.status() == [] -def test_filesystem_cache_keeps_present_artifacts(tmp_path: Path) -> None: +def test_filesystem_cache_clean_force_wipes_all(tmp_path: Path) -> None: cache = FilesystemCache(tmp_path) ref = ModelRef.parse("llama3") cache.record(ref, "latest", "x", 1) - (tmp_path / "llama3.bin").write_text("blob") - assert cache.clean() == [] - assert cache.is_fresh(ref) + removed = cache.clean(force=True) + assert removed == ["llama3:latest"] + assert cache.status() == [] + + +def test_cache_service_clean_passes_force(tmp_path: Path) -> None: + cache = FilesystemCache(tmp_path) + ref = ModelRef.parse("llama3") + cache.record(ref, "latest", "x", 1) + svc = CacheService(cache) + assert svc.clean() == [] + assert svc.clean(force=True) == ["llama3:latest"] def test_filesystem_cache_corrupt_manifest_raises(tmp_path: Path) -> None: diff --git a/tests/unit/test_core.py b/tests/unit/test_core.py index 3578847..939df87 100644 --- a/tests/unit/test_core.py +++ b/tests/unit/test_core.py @@ -4,7 +4,7 @@ import pytest -from modeldock.common.errors import DownloadError, ModelNotInstalledError +from modeldock.common.errors import DownloadError, ModelNotFoundError, ModelNotInstalledError from modeldock.core.download import DownloadService from modeldock.core.lifecycle import LifecycleOrchestrator from modeldock.domain.model import ModelRef @@ -103,13 +103,28 @@ def test_manager_install_category(manager_with_fakes: object) -> None: assert all(mgr.verify(r.name) for r in refs) -def test_manager_update(manager_with_fakes: object) -> None: +def test_manager_update_requires_confirm(manager_with_fakes: object) -> None: mgr = manager_with_fakes mgr.install("llama3") - ref = mgr.update("llama3") + with pytest.raises(DownloadError): + mgr.update("llama3") + # With confirm=True it proceeds (remove + re-pull). + ref = mgr.update("llama3", confirm=True) assert ref.name == "llama3" +def test_manager_update_unknown_raises(manager_with_fakes: object) -> None: + mgr = manager_with_fakes + with pytest.raises(ModelNotFoundError): + mgr.update("ghost-model", confirm=True) + + +def test_manager_update_cloud_model_raises(manager_with_fakes: object) -> None: + mgr = manager_with_fakes + with pytest.raises(DownloadError): + mgr.update("glm-5.2:cloud", confirm=True) + + def test_manager_remove(manager_with_fakes: object) -> None: mgr = manager_with_fakes mgr.install("llama3") diff --git a/tests/unit/test_ollama_runtime.py b/tests/unit/test_ollama_runtime.py index c9e0c1e..29e2be7 100644 --- a/tests/unit/test_ollama_runtime.py +++ b/tests/unit/test_ollama_runtime.py @@ -179,6 +179,15 @@ def delete(self, name: str) -> dict[str, Any]: runtime.remove(ModelRef.parse("llama3:latest")) +def test_remove_cloud_model_fails_fast_without_daemon_call() -> None: + # Cloud/subscription models must not trigger a (hanging) daemon delete. + client = _PullClient([{"models": []}]) + runtime = _runtime_with(client) + with pytest.raises(DownloadError): + runtime.remove(ModelRef.parse("glm-5.2:cloud")) + assert client.deleted == [] + + def test_host_override_applied_at_client_build(monkeypatch: Any) -> None: built: dict[str, Any] = {} diff --git a/tests/unit/test_port_contract.py b/tests/unit/test_port_contract.py index bf6cda0..93d6009 100644 --- a/tests/unit/test_port_contract.py +++ b/tests/unit/test_port_contract.py @@ -104,5 +104,9 @@ def test_cache_status_and_clean(cache_factory: Callable[[], List[CachePort]]) -> ref = ModelRef.parse("llama3") impl.record(ref, "latest", "sha", 10) assert len(impl.status()) == 1 + # Safe default keeps valid entries. impl.clean() + assert len(impl.status()) == 1 + # force=True wipes everything. + impl.clean(force=True) assert impl.status() == []