From a692ffeb31793518245bbb3ccaca7075751c30a4 Mon Sep 17 00:00:00 2001 From: roboomp Date: Thu, 4 Jun 2026 05:39:46 +0000 Subject: [PATCH] fix(robomp): backfilled partial-clone blobs before worktree add MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SandboxManager.ensure_workspace` calls `fetch_base_ref` (then `worktree add origin/`) and `fetch_pr_head` (then `worktree add --detach FETCH_HEAD`). Both used to issue a plain `git fetch origin `. On a `--filter=blob:none` pool the fetch inherits `remote.origin.partialclonefilter` from the pool config and brings the commit + tree but no blobs. The next `worktree add` runs in a non-token subprocess, hits a missing blob, and tries a lazy promisor fetch — which under `ProxyGitTransport` deployments has no PAT in the orchestrator container and dies with fatal: could not read Username for 'https://github.com' fatal: could not fetch from promisor remote `git_ops.fetch_ref` and `fetch_pr_head` now pass `--refetch --no-filter` so the fetch (which already runs through the token-bearing transport) eagerly materializes every blob reachable from the requested ref. `--refetch` is required: without it git short-circuits on "we already have this commit" and the lazy-fetch path stays primed. `fetch_prune` (the periodic pool refresh) is untouched, so the partial-clone disk savings are preserved on the steady-state path. `remote.origin.partialclonefilter` is left intact in the pool config. Fixes #1818 --- python/robomp/src/git_ops.py | 36 ++++- python/robomp/tests/test_sandbox.py | 196 +++++++++++++++++++++++++++- 2 files changed, 227 insertions(+), 5 deletions(-) diff --git a/python/robomp/src/git_ops.py b/python/robomp/src/git_ops.py index 312319f2d..4a460bce3 100644 --- a/python/robomp/src/git_ops.py +++ b/python/robomp/src/git_ops.py @@ -429,8 +429,29 @@ def fetch_prune(repo_dir: Path, *, token: str | None, safe_directory: Path | Non def fetch_ref(repo_dir: Path, ref: str, *, token: str | None, safe_directory: Path | None = None) -> None: - """`git fetch origin ` (best-effort: caller decides to swallow).""" - args = ["fetch", "origin", ref] + """Fetch ```` from origin AND materialize every reachable blob locally. + + Callers invoke this immediately before a ``git worktree add`` / checkout + that needs the working-tree contents, so the blobs MUST be present after + this returns. ```` is typically a ``--filter=blob:none`` partial + clone: a plain ``git fetch origin `` would inherit + ``remote.origin.partialclonefilter`` from the pool, bringing the commit + and tree but no blobs, leaving the subsequent ``worktree add`` to trigger + a lazy promisor fetch — which in proxy-transport deployments has no PAT + in the orchestrator process and dies with:: + + fatal: could not read Username for 'https://github.com' + fatal: could not fetch from promisor remote + + (oh-my-pi#1818). ``--refetch`` forces a fresh negotiation that ignores + "we already have this commit", and ``--no-filter`` overrides the inherited + filter for this one invocation without touching the on-disk config — so + ``fetch_prune`` keeps its cheap blob-skipping semantics on the next pool + refresh. Best-effort: a non-zero exit is logged and swallowed because the + caller still attempts the checkout (and a stale-ref worktree add will + surface a more actionable error than this fetch ever could). + """ + args = ["fetch", "--refetch", "--no-filter", "origin", ref] proc = _run_git(args, cwd=repo_dir, token=token, safe_directory=safe_directory) if proc.returncode != 0: log.debug( @@ -446,10 +467,17 @@ def fetch_pr_head( token: str | None, safe_directory: Path | None = None, ) -> None: - """Fetch `refs/pull//head` into FETCH_HEAD for detached PR review worktrees.""" + """Fetch ``refs/pull//head`` into FETCH_HEAD with all reachable blobs. + + Immediately followed by ``git worktree add --detach FETCH_HEAD`` for PR + review checkouts. See :func:`fetch_ref` for why ``--refetch --no-filter`` + is required: without the blob backfill, the worktree-add triggers a + promisor lazy fetch that fails under proxy-transport deployments + (oh-my-pi#1818). + """ if pr_number <= 0: raise ValueError(f"invalid PR number: {pr_number!r}") - args = ["fetch", "origin", f"pull/{pr_number}/head"] + args = ["fetch", "--refetch", "--no-filter", "origin", f"pull/{pr_number}/head"] _check(_run_git(args, cwd=repo_dir, token=token, safe_directory=safe_directory), ["git", *args]) diff --git a/python/robomp/tests/test_sandbox.py b/python/robomp/tests/test_sandbox.py index 896a30098..c54a9cf2e 100644 --- a/python/robomp/tests/test_sandbox.py +++ b/python/robomp/tests/test_sandbox.py @@ -9,7 +9,18 @@ from pathlib import Path import pytest -from robomp.git_ops import GitCommandError +from robomp.git_ops import ( + GitCommandError, +) +from robomp.git_ops import ( + fetch_pr_head as git_fetch_pr_head, +) +from robomp.git_ops import ( + fetch_prune as git_fetch_prune, +) +from robomp.git_ops import ( + fetch_ref as git_fetch_ref, +) from robomp.sandbox import ( SandboxManager, Workspace, @@ -1313,6 +1324,189 @@ def test_run_git_kills_hung_child(tmp_path: Path, monkeypatch: pytest.MonkeyPatc assert "timed out" in exc.value.stderr.lower() +# --------------------------------------------------------------------------- +# Partial-clone blob backfill (oh-my-pi#1818) +# --------------------------------------------------------------------------- + + +def _partial_clone_upstream(tmp_path: Path) -> Path: + """Bare upstream that advertises ``uploadpack.allowFilter`` so partial + clones over ``file://`` actually skip blobs (local-protocol clones + otherwise ignore ``--filter``).""" + repo = tmp_path / "partial-upstream.git" + repo.mkdir() + _git(["init", "--initial-branch=main", "--bare", str(repo)], cwd=tmp_path) + _git(["-C", str(repo), "config", "uploadpack.allowFilter", "true"], cwd=tmp_path) + _git(["-C", str(repo), "config", "uploadpack.allowAnySHA1InWant", "true"], cwd=tmp_path) + seed = tmp_path / "partial-seed" + seed.mkdir() + _git(["init", "--initial-branch=main", str(seed)], cwd=tmp_path) + (seed / "README.md").write_text("hello\n", encoding="utf-8") + _git(["-C", str(seed), "add", "."], cwd=tmp_path) + subprocess.run( + ["git", "-C", str(seed), "commit", "-m", "init"], + check=True, + capture_output=True, + text=True, + env=os.environ + | { + "GIT_AUTHOR_NAME": "t", + "GIT_AUTHOR_EMAIL": "t@t", + "GIT_COMMITTER_NAME": "t", + "GIT_COMMITTER_EMAIL": "t@t", + }, + ) + _git(["-C", str(seed), "remote", "add", "origin", str(repo)], cwd=tmp_path) + _git(["-C", str(seed), "push", "origin", "main"], cwd=tmp_path) + return repo + + +def _commit_new_blob_upstream(upstream: Path, tmp_path: Path, *, path: str, content: str, ref: str = "main") -> str: + """Add a fresh blob upstream and return the new commit SHA.""" + contrib = tmp_path / f"contrib-{path.replace('/', '_')}" + _git(["clone", f"file://{upstream}", str(contrib)], cwd=tmp_path) + (contrib / path).write_text(content, encoding="utf-8") + _git(["-C", str(contrib), "add", path], cwd=tmp_path) + subprocess.run( + ["git", "-C", str(contrib), "commit", "-m", f"add {path}"], + check=True, + capture_output=True, + text=True, + env=os.environ + | { + "GIT_AUTHOR_NAME": "t", + "GIT_AUTHOR_EMAIL": "t@t", + "GIT_COMMITTER_NAME": "t", + "GIT_COMMITTER_EMAIL": "t@t", + }, + ) + sha = subprocess.run( + ["git", "-C", str(contrib), "rev-parse", "HEAD"], + check=True, + capture_output=True, + text=True, + ).stdout.strip() + _git(["-C", str(contrib), "push", "origin", f"HEAD:{ref}"], cwd=tmp_path) + return sha + + +def _missing_object_oids(repo: Path, rev: str) -> list[str]: + """OIDs of promisor-deferred objects reachable from ``rev``.""" + proc = subprocess.run( + ["git", "-C", str(repo), "rev-list", "--objects", "--missing=print", rev], + check=True, + capture_output=True, + text=True, + ) + return [line[1:].split()[0] for line in proc.stdout.splitlines() if line.startswith("?")] + + +def test_fetch_ref_backfills_missing_blobs_into_partial_clone(tmp_path: Path) -> None: + """Regression for oh-my-pi#1818: ``fetch_ref`` is called immediately before + ``git worktree add origin/``. On a ``--filter=blob:none`` pool whose + periodic ``fetch --prune`` inherited that filter, the ref's blobs are + absent and the worktree-add triggers a promisor lazy fetch that — under + proxy transport — has no PAT and dies. ``fetch_ref`` MUST materialize + every reachable blob so the checkout never hits the lazy path.""" + upstream = _partial_clone_upstream(tmp_path) + pool = tmp_path / "pool" + _git( + [ + "clone", + "--filter=blob:none", + "--no-tags", + "--branch", + "main", + f"file://{upstream}", + str(pool), + ], + cwd=tmp_path, + ) + + # New upstream commit → fresh blob not yet pulled into the pool. + _commit_new_blob_upstream(upstream, tmp_path, path="payload.txt", content="v2 contents here\n") + + # Pool refresh mirrors `SandboxManager.ensure_clone` → inherits filter. + git_fetch_prune(pool, token=None) + missing_before = _missing_object_oids(pool, "origin/main") + assert missing_before, ( + "test precondition broken: partial-clone fetch should leave at least one blob promisor-deferred" + ) + + # Pool config must show the partial-clone state we're recovering from. + cfg_before = (pool / ".git" / "config").read_text(encoding="utf-8") + assert "partialclonefilter = blob:none" in cfg_before + assert "promisor = true" in cfg_before + + # The fix: fetch_ref backfills every reachable blob in a single call. + git_fetch_ref(pool, "main", token=None) + + missing_after = _missing_object_oids(pool, "origin/main") + assert missing_after == [], f"fetch_ref left missing objects: {missing_after}" + + # And the partial-clone config is intact — `fetch_prune` stays cheap on + # the next pool refresh; only the explicit pre-checkout fetch eagerly + # fills blobs. + cfg_after = (pool / ".git" / "config").read_text(encoding="utf-8") + assert "partialclonefilter = blob:none" in cfg_after + assert "promisor = true" in cfg_after + + # End-to-end: worktree add must succeed even if origin is unreachable — + # the blobs are local now, no lazy fetch can fire. + _git(["-C", str(pool), "remote", "set-url", "origin", "https://example.invalid/missing.git"], cwd=tmp_path) + ws_dir = tmp_path / "ws" + subprocess.run( + ["git", "-C", str(pool), "worktree", "add", "-b", "verify-1818", str(ws_dir), "origin/main"], + check=True, + capture_output=True, + text=True, + env=os.environ | {"GIT_TERMINAL_PROMPT": "0"}, + ) + assert (ws_dir / "payload.txt").read_text(encoding="utf-8") == "v2 contents here\n" + + +def test_fetch_pr_head_backfills_missing_blobs_into_partial_clone(tmp_path: Path) -> None: + """Same regression as ``test_fetch_ref_backfills…`` but on the PR-review + path: ``fetch_pr_head`` precedes ``git worktree add --detach FETCH_HEAD`` + and so MUST eagerly fetch blobs reachable from the PR head.""" + upstream = _partial_clone_upstream(tmp_path) + pool = tmp_path / "pool" + _git( + [ + "clone", + "--filter=blob:none", + "--no-tags", + "--branch", + "main", + f"file://{upstream}", + str(pool), + ], + cwd=tmp_path, + ) + + # Publish a PR head with a fresh blob. + pr_sha = _commit_new_blob_upstream( + upstream, tmp_path, path="pr.txt", content="pr blob payload\n", ref="refs/pull/7/head" + ) + + git_fetch_pr_head(pool, 7, token=None) + missing_after = _missing_object_oids(pool, pr_sha) + assert missing_after == [], f"fetch_pr_head left missing objects: {missing_after}" + + # End-to-end: a detached worktree add against the freshly-fetched PR head + # succeeds without lazy-fetching against (now-broken) origin. + _git(["-C", str(pool), "remote", "set-url", "origin", "https://example.invalid/missing.git"], cwd=tmp_path) + ws_dir = tmp_path / "pr-ws" + subprocess.run( + ["git", "-C", str(pool), "worktree", "add", "--detach", str(ws_dir), "FETCH_HEAD"], + check=True, + capture_output=True, + text=True, + env=os.environ | {"GIT_TERMINAL_PROMPT": "0"}, + ) + assert (ws_dir / "pr.txt").read_text(encoding="utf-8") == "pr blob payload\n" + + # --------------------------------------------------------------------------- # NativesCache integration into ensure_workspace # ---------------------------------------------------------------------------