From 6e62f9e51cd0472709c30c5fed21a734e2831ca2 Mon Sep 17 00:00:00 2001 From: Bruno Postle Date: Sat, 25 Apr 2026 09:05:13 +0100 Subject: [PATCH] git.py: log warning and return is_stale flag when mutable-ref fetch fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a branch/HEAD fetch fails (network error, auth failure, server down), _open_remote now logs a warning rather than silently passing. A boolean is_stale propagates through _get_repo → fetch_ifc so callers know the data comes from a stale local cache. The service adds X-Ifcurl-Cache: stale to /preview and /render_diff responses when stale data was served, so Forgejo or a browser viewer can surface a warning to the user. Co-Authored-By: Claude Sonnet 4.6 --- ifcurl/git.py | 47 ++++++++++++++++++++++++++++++++----------- ifcurl/service.py | 26 ++++++++++++++++-------- tests/test_bcf.py | 2 +- tests/test_git.py | 19 ++++++++++------- tests/test_service.py | 12 +++++------ 5 files changed, 72 insertions(+), 34 deletions(-) diff --git a/ifcurl/git.py b/ifcurl/git.py index 9289c5d..11ea281 100644 --- a/ifcurl/git.py +++ b/ifcurl/git.py @@ -35,10 +35,13 @@ from __future__ import annotations import hashlib # allows git import even if git executable isn't found during import +import logging import os import shutil from pathlib import Path +_logger = logging.getLogger(__name__) + from platformdirs import user_cache_dir os.environ.setdefault("GIT_PYTHON_REFRESH", "quiet") @@ -53,12 +56,16 @@ except ImportError: from ifcurl.url import IfcUrl -def fetch_ifc(ifc_url: IfcUrl, token: str | None = None) -> tuple[str, bytes]: - """Return ``(commit_hexsha, ifc_bytes)`` for the file addressed by *ifc_url*. +def fetch_ifc(ifc_url: IfcUrl, token: str | None = None) -> tuple[str, bytes, bool]: + """Return ``(commit_hexsha, ifc_bytes, is_stale)`` for the file addressed by *ifc_url*. The commit hexsha is the resolved, immutable identifier for the ref — useful as a cache key even when the URL uses a mutable ref like a branch. + *is_stale* is ``True`` when the URL uses a mutable ref (branch/HEAD) and + the remote fetch failed — the returned bytes come from the local cache and + may not reflect the latest commit. A warning is logged in this case. + :param ifc_url: A parsed :class:`IfcUrl`. :param token: Optional bearer token for HTTPS authentication. Injected as ``https://@host/path``. Ignored for SSH and local repos. @@ -73,8 +80,9 @@ def fetch_ifc(ifc_url: IfcUrl, token: str | None = None) -> tuple[str, bytes]: if ifc_url.path is None: raise ValueError("URL has no 'path' parameter — cannot fetch IFC file") - repo = _get_repo(ifc_url, token=token) - return _read_commit_blob(repo, ifc_url.git_ref(), ifc_url.path) + repo, is_stale = _get_repo(ifc_url, token=token) + hexsha, data = _read_commit_blob(repo, ifc_url.git_ref(), ifc_url.path) + return hexsha, data, is_stale def diff_text(base_url: IfcUrl, head_url: IfcUrl, token: str | None = None) -> str: @@ -125,7 +133,7 @@ def fetch_ifc_bytes(ifc_url: IfcUrl, token: str | None = None) -> bytes: :raises ValueError: If ``ifc_url.path`` is unset, the repo cannot be reached, or the file is not found at the specified ref. """ - _, data = fetch_ifc(ifc_url, token=token) + _, data, _ = fetch_ifc(ifc_url, token=token) return data @@ -134,10 +142,10 @@ def fetch_ifc_bytes(ifc_url: IfcUrl, token: str | None = None) -> bytes: # --------------------------------------------------------------------------- -def _get_repo(ifc_url: IfcUrl, token: str | None = None) -> git.Repo: - """Open or fetch the repository for *ifc_url*.""" +def _get_repo(ifc_url: IfcUrl, token: str | None = None) -> tuple[git.Repo, bool]: + """Open or fetch the repository for *ifc_url*. Returns ``(repo, is_stale)``.""" if ifc_url.transport == "local": - return _open_local(ifc_url.repo_path) + return _open_local(ifc_url.repo_path), False return _open_remote(ifc_url.git_remote_url(), ifc_url.is_mutable_ref(), token=token) @@ -272,13 +280,17 @@ def _clone_bare(auth_url: str, git_dir: Path, remote_url: str) -> git.Repo: def _open_remote( remote_url: str, is_mutable: bool, token: str | None = None -) -> git.Repo: - """Return a GitPython Repo for *remote_url*, cloning it if necessary. +) -> tuple[git.Repo, bool]: + """Return ``(repo, is_stale)`` for *remote_url*, cloning if necessary. Bare clones are stored under the OS cache dir keyed on the clean URL. For mutable refs (branches, HEAD) the remote is fetched on every call. Immutable refs (commit hashes, tags) skip the fetch. + *is_stale* is ``True`` when a mutable-ref fetch was attempted but all + network attempts failed — the caller receives cached data that may be + outdated. A warning is logged in this case. + If *token* is provided it is injected into the URL for clone and fetch operations but is never written to the on-disk git config. @@ -288,6 +300,7 @@ def _open_remote( cache_dir = _cache_dir_for(remote_url) git_dir = cache_dir / "repo.git" auth = _auth_url(remote_url, token) + is_stale = False if not git_dir.exists(): repo = _clone_bare(auth, git_dir, remote_url) @@ -296,6 +309,7 @@ def _open_remote( else: repo = git.Repo(str(git_dir)) if is_mutable: + fetch_ok = False try: if auth != remote_url: # Fetch directly from the authenticated URL so the token @@ -304,20 +318,29 @@ def _open_remote( fallback = _http_fallback(auth) try: repo.git.fetch(fetch_url, "+refs/*:refs/*") + fetch_ok = True except git.exc.GitCommandError: if fallback: try: repo.git.fetch(fallback, "+refs/*:refs/*") + fetch_ok = True except git.exc.GitCommandError: pass else: repo.git.fetch("origin") + fetch_ok = True except git.exc.GitCommandError: - pass # offline — use cached data + pass # offline — fall through to stale check + + if not fetch_ok: + is_stale = True + _logger.warning( + "fetch failed for %r — serving stale cached data", remote_url + ) _evict_if_needed() _touch_cache(cache_dir) - return repo + return repo, is_stale def _read_commit_blob( diff --git a/ifcurl/service.py b/ifcurl/service.py index 9304421..996e472 100644 --- a/ifcurl/service.py +++ b/ifcurl/service.py @@ -415,7 +415,7 @@ def preview(request: PreviewRequest) -> Response: # --- Fetch IFC bytes + commit hexsha --- try: - hexsha, ifc_bytes = fetch_ifc(ifc_url, token=token) + hexsha, ifc_bytes, is_stale = fetch_ifc(ifc_url, token=token) except ImportError as exc: raise HTTPException(status_code=503, detail=str(exc)) from exc except ValueError as exc: @@ -470,19 +470,23 @@ def preview(request: PreviewRequest) -> Response: _t3_put(hexsha, ifc_url.path, ifc_url.selector, new_guids) # --- Tier 4 / 4m: store PNG and return with appropriate cache headers --- + extra_headers = {"X-Ifcurl-Cache": "stale"} if is_stale else {} if ifc_url.is_mutable_ref(): _t4m_put(request.url, png_bytes) return Response( content=png_bytes, media_type="image/png", - headers={"Cache-Control": f"public, max-age={_T4M_TTL}"}, + headers={"Cache-Control": f"public, max-age={_T4M_TTL}", **extra_headers}, ) else: _t4_put(request.url, png_bytes) return Response( content=png_bytes, media_type="image/png", - headers={"Cache-Control": "public, max-age=31536000, immutable"}, + headers={ + "Cache-Control": "public, max-age=31536000, immutable", + **extra_headers, + }, ) @@ -511,7 +515,7 @@ def bcf_export(request: BcfRequest) -> Response: if token is None and ifc_url.host: token = get_token_for_host(ifc_url.host) try: - hexsha, ifc_bytes = fetch_ifc(ifc_url, token=token) + hexsha, ifc_bytes, _ = fetch_ifc(ifc_url, token=token) except (ImportError, ValueError) as exc: raise HTTPException(status_code=404, detail=str(exc)) from exc @@ -614,8 +618,8 @@ def render_diff(request: DiffRequest) -> Response: # --- Fetch IFC bytes for both commits --- try: - base_hexsha, base_bytes = fetch_ifc(base_url, token=token) - head_hexsha, head_bytes = fetch_ifc(head_url, token=token) + base_hexsha, base_bytes, base_stale = fetch_ifc(base_url, token=token) + head_hexsha, head_bytes, head_stale = fetch_ifc(head_url, token=token) except ImportError as exc: raise HTTPException(status_code=503, detail=str(exc)) from exc except ValueError as exc: @@ -667,16 +671,22 @@ def render_diff(request: DiffRequest) -> Response: raise HTTPException(status_code=422, detail=str(exc)) from exc # --- Tier 4: cache and return --- + diff_stale_headers = ( + {"X-Ifcurl-Cache": "stale"} if (base_stale or head_stale) else {} + ) if both_immutable: _t4_put(cache_key, png_bytes) return Response( content=png_bytes, media_type="image/png", - headers={"Cache-Control": "public, max-age=31536000, immutable"}, + headers={ + "Cache-Control": "public, max-age=31536000, immutable", + **diff_stale_headers, + }, ) return Response( content=png_bytes, media_type="image/png", - headers={"Cache-Control": "no-store"}, + headers={"Cache-Control": "no-store", **diff_stale_headers}, ) diff --git a/tests/test_bcf.py b/tests/test_bcf.py index c50b170..07bda0e 100644 --- a/tests/test_bcf.py +++ b/tests/test_bcf.py @@ -206,7 +206,7 @@ class TestBcfEndpoint: ifc_bytes = model_with_geometry.to_string().encode() def mock_fetch(ifc_url, token=None): - return FAKE_HEXSHA, ifc_bytes + return FAKE_HEXSHA, ifc_bytes, False selector_url = MUTABLE_URL.replace( "?path=model.ifc", "?path=model.ifc&selector=IfcWall" diff --git a/tests/test_git.py b/tests/test_git.py index d4dbc2b..f0d4c57 100644 --- a/tests/test_git.py +++ b/tests/test_git.py @@ -18,31 +18,36 @@ def _local_url(repo_path: str, ref: str = "HEAD", path: str = "model.ifc") -> If class TestFetchIfc: def test_returns_hexsha_and_bytes(self, local_ifc_repo): url = _local_url(local_ifc_repo["path"]) - hexsha, data = fetch_ifc(url) + hexsha, data, _ = fetch_ifc(url) assert len(hexsha) == 40 assert all(c in "0123456789abcdef" for c in hexsha) assert isinstance(data, bytes) assert len(data) > 0 + def test_local_repo_never_stale(self, local_ifc_repo): + url = _local_url(local_ifc_repo["path"]) + _, _, is_stale = fetch_ifc(url) + assert is_stale is False + def test_hexsha_matches_commit(self, local_ifc_repo): url = _local_url(local_ifc_repo["path"]) - hexsha, _ = fetch_ifc(url) + hexsha, _, _ = fetch_ifc(url) assert hexsha == local_ifc_repo["hexsha"] def test_bytes_match_committed_file(self, local_ifc_repo): url = _local_url(local_ifc_repo["path"]) - _, data = fetch_ifc(url) + _, data, _ = fetch_ifc(url) assert data == local_ifc_repo["bytes"] def test_explicit_commit_hash_ref(self, local_ifc_repo): url = _local_url(local_ifc_repo["path"], ref=local_ifc_repo["hexsha"]) - hexsha, data = fetch_ifc(url) + hexsha, data, _ = fetch_ifc(url) assert hexsha == local_ifc_repo["hexsha"] assert data == local_ifc_repo["bytes"] def test_tag_ref(self, local_ifc_repo): url = _local_url(local_ifc_repo["path"], ref="tags/v1.0") - hexsha, data = fetch_ifc(url) + hexsha, data, _ = fetch_ifc(url) assert hexsha == local_ifc_repo["hexsha"] def test_branch_ref(self, local_ifc_repo): @@ -50,7 +55,7 @@ class TestFetchIfc: branch = gitpkg.Repo(local_ifc_repo["path"]).active_branch.name url = _local_url(local_ifc_repo["path"], ref=f"heads/{branch}") - hexsha, data = fetch_ifc(url) + hexsha, data, _ = fetch_ifc(url) assert hexsha == local_ifc_repo["hexsha"] assert data == local_ifc_repo["bytes"] @@ -85,7 +90,7 @@ class TestFetchIfcBytes: def test_backwards_compat_with_fetch_ifc(self, local_ifc_repo): url = _local_url(local_ifc_repo["path"]) - _, expected = fetch_ifc(url) + _, expected, _ = fetch_ifc(url) assert fetch_ifc_bytes(url) == expected diff --git a/tests/test_service.py b/tests/test_service.py index e086a18..122e66d 100644 --- a/tests/test_service.py +++ b/tests/test_service.py @@ -64,7 +64,7 @@ def tmp_t4_cache(tmp_path): def _mock_fetch(ifc_bytes: bytes): - return lambda ifc_url, token=None: (FAKE_HEXSHA, ifc_bytes) + return lambda ifc_url, token=None: (FAKE_HEXSHA, ifc_bytes, False) # --------------------------------------------------------------------------- @@ -149,7 +149,7 @@ class TestTier2Cache: def counting_fetch(ifc_url, token=None): nonlocal call_count call_count += 1 - return FAKE_HEXSHA, ifc_bytes + return FAKE_HEXSHA, ifc_bytes, False # Disable tier-4m so both requests reach fetch_ifc, letting us # verify that tier-2 serves ifc_bytes from cache on the second call. @@ -305,7 +305,7 @@ class TestAuthentication: def capturing_fetch(ifc_url, token=None): received_token.append(token) - return FAKE_HEXSHA, ifc_bytes + return FAKE_HEXSHA, ifc_bytes, False with ( patch("ifcurl.service.fetch_ifc", capturing_fetch), @@ -323,7 +323,7 @@ class TestAuthentication: def capturing_fetch(ifc_url, token=None): received_token.append(token) - return FAKE_HEXSHA, ifc_bytes + return FAKE_HEXSHA, ifc_bytes, False monkeypatch.setattr( "ifcurl.service.get_token_for_host", lambda host: "config_token" @@ -344,7 +344,7 @@ class TestAuthentication: def capturing_fetch(ifc_url, token=None): received_token.append(token) - return FAKE_HEXSHA, ifc_bytes + return FAKE_HEXSHA, ifc_bytes, False monkeypatch.setattr( "ifcurl.service.get_token_for_host", lambda host: "config_token" @@ -389,7 +389,7 @@ class TestGetEndpoint: def mock_fetch(ifc_url, token=None): received["token"] = token - return FAKE_HEXSHA, ifc_bytes + return FAKE_HEXSHA, ifc_bytes, False with ( patch("ifcurl.service.fetch_ifc", mock_fetch),