smashimo/oh-my-pi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(robomp): backfilled partial-clone blobs before worktree add

Browse Source 
`SandboxManager.ensure_workspace` calls `fetch_base_ref` (then
`worktree add origin/<ref>`) and `fetch_pr_head` (then
`worktree add --detach FETCH_HEAD`). Both used to issue a plain
`git fetch origin <ref>`. 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 <sha> 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
This commit is contained in:
roboomp
2026-06-04 05:39:46 +00:00
parent f6fca1f5cd
commit a692ffeb31
2 changed files with 227 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files
python/robomp/src/git_ops.py
+32 -4
View File
@@ -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 <ref>` (best-effort: caller decides to swallow)."""
args = ["fetch", "origin", ref]
"""Fetch ``<ref>`` 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. ``<repo_dir>`` is typically a ``--filter=blob:none`` partial
clone: a plain ``git fetch origin <ref>`` 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 <sha> 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/<n>/head` into FETCH_HEAD for detached PR review worktrees."""
"""Fetch ``refs/pull/<n>/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])
python/robomp/tests/test_sandbox.py
+195 -1
View File
@@ -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/<ref>``. 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
# ---------------------------------------------------------------------------