mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(workspace): re-apply agent ownership after refresh fetch
I1: _fetch_origin_best_effort runs as root and writes new pack files + ref updates that land root-owned, undoing _ensure_agent_owned that ran before. Subsequent spawns hit Permission denied. Mirror the fetch_branch_for_inspection pattern: re-run _ensure_agent_owned AFTER the fetch. I2: separate workspace_refresh_fetch_timeout_seconds (default 60s) from workspace_clone_timeout (300s). Refresh transfers small deltas; 300s of blocking on every spawn against a hung remote is operationally bad. 60s is enough for any sane refresh.
This commit is contained in:
@@ -238,6 +238,17 @@ class Settings(BaseSettings):
|
|||||||
ge=30,
|
ge=30,
|
||||||
description="Timeout in seconds for git clone operations",
|
description="Timeout in seconds for git clone operations",
|
||||||
)
|
)
|
||||||
|
workspace_refresh_fetch_timeout_seconds: int = Field(
|
||||||
|
default=60,
|
||||||
|
ge=5,
|
||||||
|
description=(
|
||||||
|
"Timeout in seconds for the best-effort `git fetch origin` "
|
||||||
|
"that runs on every healthy-clone re-entry into "
|
||||||
|
"ensure_workspace. Refresh fetches transfer small deltas only "
|
||||||
|
"— blocking 300s (the full-clone timeout) on every spawn "
|
||||||
|
"against a hung remote is operationally bad."
|
||||||
|
),
|
||||||
|
)
|
||||||
|
|
||||||
# ==========================================================================
|
# ==========================================================================
|
||||||
# Agent Guardrails (per-session budgets, loop detection, SLAs)
|
# Agent Guardrails (per-session budgets, loop detection, SLAs)
|
||||||
|
|||||||
@@ -321,6 +321,11 @@ class WorkspaceService:
|
|||||||
stripped remote URL. Public repos and refresh-only fetches succeed
|
stripped remote URL. Public repos and refresh-only fetches succeed
|
||||||
without auth; auth-protected refreshes will surface their stderr
|
without auth; auth-protected refreshes will surface their stderr
|
||||||
in the warning log without aborting workspace setup.
|
in the warning log without aborting workspace setup.
|
||||||
|
|
||||||
|
Timeout uses `workspace_refresh_fetch_timeout_seconds` (default
|
||||||
|
60s), NOT `workspace_clone_timeout` (300s) — a refresh transfers
|
||||||
|
small deltas, so 300s of blocking on every spawn against a hung
|
||||||
|
remote is operationally bad.
|
||||||
"""
|
"""
|
||||||
|
|
||||||
def _do_fetch() -> subprocess.CompletedProcess[str]:
|
def _do_fetch() -> subprocess.CompletedProcess[str]:
|
||||||
@@ -329,7 +334,7 @@ class WorkspaceService:
|
|||||||
cwd=str(workspace),
|
cwd=str(workspace),
|
||||||
capture_output=True,
|
capture_output=True,
|
||||||
text=True,
|
text=True,
|
||||||
timeout=settings.workspace_clone_timeout,
|
timeout=settings.workspace_refresh_fetch_timeout_seconds,
|
||||||
check=False,
|
check=False,
|
||||||
)
|
)
|
||||||
|
|
||||||
@@ -432,6 +437,13 @@ class WorkspaceService:
|
|||||||
# network blips and offline mode must not break workspace
|
# network blips and offline mode must not break workspace
|
||||||
# setup; checkout is unchanged.
|
# setup; checkout is unchanged.
|
||||||
await self._fetch_origin_best_effort(workspace, project_slug)
|
await self._fetch_origin_best_effort(workspace, project_slug)
|
||||||
|
# Re-chown so the agent user can still write into .git
|
||||||
|
# after our root-side fetch updated refs/objects. Mirrors
|
||||||
|
# the pattern in `fetch_branch_for_inspection` — without
|
||||||
|
# this, new pack files under .git/objects/pack/ and ref
|
||||||
|
# updates under .git/refs/remotes/origin/ land root-owned
|
||||||
|
# and undo the chown we just ran above.
|
||||||
|
await asyncio.to_thread(_ensure_agent_owned, workspace)
|
||||||
logger.debug(
|
logger.debug(
|
||||||
"Workspace already exists",
|
"Workspace already exists",
|
||||||
workspace=str(workspace),
|
workspace=str(workspace),
|
||||||
|
|||||||
@@ -25,6 +25,11 @@ if TYPE_CHECKING:
|
|||||||
# `origin`). Named to satisfy ruff PLR2004 — magic-value comparison.
|
# `origin`). Named to satisfy ruff PLR2004 — magic-value comparison.
|
||||||
_MIN_GIT_FETCH_ARGC = 3
|
_MIN_GIT_FETCH_ARGC = 3
|
||||||
|
|
||||||
|
# Healthy short-circuit chowns BEFORE fetch (repair pre-existing root
|
||||||
|
# ownership) AND AFTER fetch (repair root-owned pack/refs the fetch
|
||||||
|
# just wrote). Named to satisfy ruff PLR2004.
|
||||||
|
_MIN_CHOWN_CALLS_AROUND_FETCH = 2
|
||||||
|
|
||||||
|
|
||||||
def _service() -> WorkspaceService:
|
def _service() -> WorkspaceService:
|
||||||
"""Build a WorkspaceService over a MagicMock session."""
|
"""Build a WorkspaceService over a MagicMock session."""
|
||||||
@@ -101,10 +106,72 @@ async def test_ensure_workspace_fetches_origin_on_healthy_short_circuit(
|
|||||||
f"Expected `git fetch origin` on healthy short-circuit, "
|
f"Expected `git fetch origin` on healthy short-circuit, "
|
||||||
f"got subprocess calls: {captured}"
|
f"got subprocess calls: {captured}"
|
||||||
)
|
)
|
||||||
# Specifically: `git fetch origin` (no extra positional refspec — fetch
|
# Specifically: `git fetch origin` with NO `-c` flag and no extra
|
||||||
# all branches' refs so PM/Doc sees every dev branch).
|
# positional refspec. The `-c` check protects the docstring's
|
||||||
assert any(a[-2:] == ["fetch", "origin"] for a in fetch_calls), (
|
# no-token-injection invariant — a future refactor that added
|
||||||
f"Expected exact `git fetch origin`, got: {fetch_calls}"
|
# `git -c http.extraheader=...` would still satisfy a loose
|
||||||
|
# `a[-2:] == ["fetch", "origin"]` assertion, silently regressing
|
||||||
|
# the no-PAT-injection guarantee.
|
||||||
|
assert any(
|
||||||
|
a[0] == "git" and "-c" not in a and a[-2:] == ["fetch", "origin"]
|
||||||
|
for a in fetch_calls
|
||||||
|
), f"Expected exact `git fetch origin` (no `-c`), got: {fetch_calls}"
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_ensure_workspace_rechowns_after_refresh_fetch(
|
||||||
|
healthy_workspace: Path,
|
||||||
|
) -> None:
|
||||||
|
"""Healthy-clone re-entry MUST chown again AFTER `git fetch origin`.
|
||||||
|
|
||||||
|
The orchestrator runs as root, so `git fetch` writes new pack files
|
||||||
|
under `.git/objects/pack/` and updates refs under
|
||||||
|
`.git/refs/remotes/origin/` — those land root-owned, undoing the
|
||||||
|
pre-fetch chown. Without a post-fetch chown, the next agent-side
|
||||||
|
write (.git/index.lock, packed-refs, etc.) hits Permission denied.
|
||||||
|
|
||||||
|
This mirrors the pattern in `fetch_branch_for_inspection`.
|
||||||
|
"""
|
||||||
|
svc = _service()
|
||||||
|
agent = _fake_agent()
|
||||||
|
_bind(svc, "_lookup_agent_or_raise", AsyncMock(return_value=agent))
|
||||||
|
_bind(svc, "get_workspace_path", MagicMock(return_value=healthy_workspace))
|
||||||
|
|
||||||
|
call_log: list[str] = []
|
||||||
|
|
||||||
|
def _fake_run(
|
||||||
|
args: list[str], **_kwargs: object
|
||||||
|
) -> subprocess.CompletedProcess[str]:
|
||||||
|
if "fetch" in args:
|
||||||
|
call_log.append("fetch")
|
||||||
|
return subprocess.CompletedProcess(
|
||||||
|
args=args, returncode=0, stdout="", stderr=""
|
||||||
|
)
|
||||||
|
|
||||||
|
def _fake_chown(_workspace: object) -> None:
|
||||||
|
call_log.append("chown")
|
||||||
|
|
||||||
|
with (
|
||||||
|
patch("roboco.services.workspace.subprocess.run", side_effect=_fake_run),
|
||||||
|
patch(
|
||||||
|
"roboco.services.workspace._ensure_agent_owned",
|
||||||
|
side_effect=_fake_chown,
|
||||||
|
),
|
||||||
|
):
|
||||||
|
await svc.ensure_workspace(
|
||||||
|
project_slug="roboco",
|
||||||
|
agent_id=agent.id,
|
||||||
|
)
|
||||||
|
|
||||||
|
# Expect at least: chown (pre-fetch) -> fetch -> chown (post-fetch).
|
||||||
|
# The post-fetch chown is the load-bearing one — it repairs ownership
|
||||||
|
# of objects/refs the root-side fetch just wrote.
|
||||||
|
assert call_log.count("chown") >= _MIN_CHOWN_CALLS_AROUND_FETCH, (
|
||||||
|
f"Expected at least two chown calls (pre + post fetch), got: {call_log}"
|
||||||
|
)
|
||||||
|
fetch_idx = call_log.index("fetch")
|
||||||
|
assert "chown" in call_log[fetch_idx + 1 :], (
|
||||||
|
f"Expected a chown AFTER the fetch, got: {call_log}"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user