mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-10-07 16:19:09 +00:00
fix: fail closed for unbound-legacy sandboxes and catch repo mismatch
Gap 1: a thread with a persisted sandbox_id but no in-memory cache and no recorded bound_repo (a pre-binding legacy thread, post-deploy) previously reconnected-and-served the sandbox to the current repo, then rebound it. Now fail closed: drop the stale id and recreate a fresh sandbox bound to this repo, logging a reconnect-with-missing-binding event. A sandbox is never served to a repo unless its binding is known and matches; new threads bind on first run unchanged. Gap 3: catch SandboxRepoMismatchError at the agent and reviewer run entrypoints, log it for alarming, and surface a clean sanitized error instead of letting an opaque deep-stack exception crash-loop the worker.
This commit is contained in:
parent
969b8e3882
commit
8552e871f4
4 changed files with 165 additions and 19 deletions
|
|
@ -59,6 +59,7 @@ from .server import (
|
||||||
DEFAULT_LLM_MAX_TOKENS,
|
DEFAULT_LLM_MAX_TOKENS,
|
||||||
DEFAULT_RECURSION_LIMIT,
|
DEFAULT_RECURSION_LIMIT,
|
||||||
MODEL_CALL_RECURSION_LIMIT,
|
MODEL_CALL_RECURSION_LIMIT,
|
||||||
|
SandboxRepoMismatchError,
|
||||||
_general_purpose_subagent,
|
_general_purpose_subagent,
|
||||||
ensure_sandbox_for_thread,
|
ensure_sandbox_for_thread,
|
||||||
graph_loaded_for_execution,
|
graph_loaded_for_execution,
|
||||||
|
|
@ -851,12 +852,18 @@ async def get_reviewer_agent(config: RunnableConfig) -> Pregel:
|
||||||
if repo_config.get("owner") and repo_config.get("name")
|
if repo_config.get("owner") and repo_config.get("name")
|
||||||
else None
|
else None
|
||||||
)
|
)
|
||||||
sandbox_backend = await ensure_sandbox_for_thread(
|
try:
|
||||||
thread_id,
|
sandbox_backend = await ensure_sandbox_for_thread(
|
||||||
github_proxy_token=github_proxy_token,
|
thread_id,
|
||||||
github_proxy_repositories=[repo_name_for_scope] if repo_name_for_scope else None,
|
github_proxy_token=github_proxy_token,
|
||||||
repo=repo_for_snapshot,
|
github_proxy_repositories=[repo_name_for_scope] if repo_name_for_scope else None,
|
||||||
)
|
repo=repo_for_snapshot,
|
||||||
|
)
|
||||||
|
except SandboxRepoMismatchError as exc:
|
||||||
|
# Repo-binding refusal at the run boundary: log for alarming and surface the
|
||||||
|
# sanitized terminal error rather than crash-looping the reviewer worker.
|
||||||
|
logger.error("Refusing reviewer run for thread %s: %s", thread_id, exc)
|
||||||
|
raise RuntimeError(str(exc)) from exc
|
||||||
|
|
||||||
work_dir = await aresolve_sandbox_work_dir(sandbox_backend)
|
work_dir = await aresolve_sandbox_work_dir(sandbox_backend)
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -451,14 +451,19 @@ async def ensure_sandbox_for_thread(
|
||||||
sandbox_backend = SANDBOX_BACKENDS.get(thread_id)
|
sandbox_backend = SANDBOX_BACKENDS.get(thread_id)
|
||||||
sandbox_id = await get_sandbox_id_from_metadata(thread_id)
|
sandbox_id = await get_sandbox_id_from_metadata(thread_id)
|
||||||
|
|
||||||
# Repo-binding guard (TID-COLLIDE-01): refuse to reuse a thread's sandbox for
|
if sandbox_id == SANDBOX_CREATING and not sandbox_backend:
|
||||||
# a repo it is not bound to, so a colliding thread_id from a different repo
|
logger.info("Sandbox creation in progress for thread %s, waiting...", thread_id)
|
||||||
# cannot bind to (or clobber) another thread's sandbox.
|
sandbox_id = await _resolve_creating_sentinel(thread_id)
|
||||||
|
|
||||||
|
# Repo-binding guard (TID-COLLIDE-01): a sandbox is never served to a repo
|
||||||
|
# unless its binding is known and matches.
|
||||||
current_repo = repo_cache_key(repo)
|
current_repo = repo_cache_key(repo)
|
||||||
bound_repo = await get_bound_repo_from_metadata(thread_id)
|
bound_repo = await get_bound_repo_from_metadata(thread_id)
|
||||||
proxy_bound = getattr(sandbox_backend, "bound_repo", None)
|
proxy_bound = getattr(sandbox_backend, "bound_repo", None)
|
||||||
effective_bound = bound_repo or (proxy_bound if isinstance(proxy_bound, str) else None)
|
effective_bound = bound_repo or (proxy_bound if isinstance(proxy_bound, str) else None)
|
||||||
if current_repo and effective_bound and effective_bound != current_repo:
|
if current_repo and effective_bound and effective_bound != current_repo:
|
||||||
|
# Known binding that does not match the current repo: refuse outright so a
|
||||||
|
# colliding thread_id from a different repo cannot reuse/clobber it.
|
||||||
logger.error(
|
logger.error(
|
||||||
"Repo mismatch for thread %s: bound=%s current=%s; refusing sandbox reuse",
|
"Repo mismatch for thread %s: bound=%s current=%s; refusing sandbox reuse",
|
||||||
thread_id,
|
thread_id,
|
||||||
|
|
@ -466,10 +471,26 @@ async def ensure_sandbox_for_thread(
|
||||||
current_repo,
|
current_repo,
|
||||||
)
|
)
|
||||||
raise SandboxRepoMismatchError(thread_id, effective_bound, current_repo)
|
raise SandboxRepoMismatchError(thread_id, effective_bound, current_repo)
|
||||||
|
if (
|
||||||
if sandbox_id == SANDBOX_CREATING and not sandbox_backend:
|
current_repo
|
||||||
logger.info("Sandbox creation in progress for thread %s, waiting...", thread_id)
|
and not effective_bound
|
||||||
sandbox_id = await _resolve_creating_sentinel(thread_id)
|
and sandbox_backend is None
|
||||||
|
and isinstance(sandbox_id, str)
|
||||||
|
and sandbox_id not in (None, SANDBOX_CREATING)
|
||||||
|
):
|
||||||
|
# Fail CLOSED for unbound-legacy threads (migration window): a thread with a
|
||||||
|
# persisted sandbox_id but no in-memory cache and no recorded bound_repo
|
||||||
|
# cannot be confirmed to belong to the current repo, so never
|
||||||
|
# reconnect-and-serve it. Drop the stale id and recreate a fresh sandbox
|
||||||
|
# bound to this repo below.
|
||||||
|
logger.error(
|
||||||
|
"reconnect-with-missing-binding for thread %s: persisted sandbox %s has no "
|
||||||
|
"bound_repo; refusing reuse and recreating for repo %s",
|
||||||
|
thread_id,
|
||||||
|
sandbox_id,
|
||||||
|
current_repo,
|
||||||
|
)
|
||||||
|
sandbox_id = None
|
||||||
|
|
||||||
if sandbox_backend:
|
if sandbox_backend:
|
||||||
logger.info("Using cached sandbox backend for thread %s", thread_id)
|
logger.info("Using cached sandbox backend for thread %s", thread_id)
|
||||||
|
|
@ -668,11 +689,21 @@ async def get_agent(config: RunnableConfig) -> Pregel:
|
||||||
)
|
)
|
||||||
team_defaults_task = asyncio.create_task(get_team_default_model_pair("agent"))
|
team_defaults_task = asyncio.create_task(get_team_default_model_pair("agent"))
|
||||||
profile_task = asyncio.create_task(load_profile(profile_login)) if profile_login else None
|
profile_task = asyncio.create_task(load_profile(profile_login)) if profile_login else None
|
||||||
triggering_user_identity, sandbox_backend, team_defaults = await asyncio.gather(
|
try:
|
||||||
triggering_user_identity_task,
|
triggering_user_identity, sandbox_backend, team_defaults = await asyncio.gather(
|
||||||
sandbox_task,
|
triggering_user_identity_task,
|
||||||
team_defaults_task,
|
sandbox_task,
|
||||||
)
|
team_defaults_task,
|
||||||
|
)
|
||||||
|
except SandboxRepoMismatchError as exc:
|
||||||
|
# Repo-binding refusal at the run boundary: log for alarming and surface the
|
||||||
|
# already-sanitized terminal error (no sandbox/token internals) to the caller,
|
||||||
|
# rather than letting an opaque deep-stack exception crash-loop the worker.
|
||||||
|
logger.error("Refusing agent run for thread %s: %s", thread_id, exc)
|
||||||
|
for pending in (triggering_user_identity_task, team_defaults_task, profile_task):
|
||||||
|
if pending is not None and not pending.done():
|
||||||
|
pending.cancel()
|
||||||
|
raise RuntimeError(str(exc)) from exc
|
||||||
profile = await profile_task if profile_task is not None else None
|
profile = await profile_task if profile_task is not None else None
|
||||||
del github_token
|
del github_token
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -309,7 +309,7 @@ class TestRefreshProxyOnSandboxReuse:
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_refreshes_proxy_when_reconnecting_to_existing_langsmith_sandbox(self) -> None:
|
async def test_refreshes_proxy_when_reconnecting_to_existing_langsmith_sandbox(self) -> None:
|
||||||
"""Reconnected sandboxes should also get a fresh proxy token."""
|
"""A bound thread reconnecting to its sandbox should get a fresh proxy token."""
|
||||||
config = self._execution_config()
|
config = self._execution_config()
|
||||||
mock_sandbox = MagicMock(id="sandbox-existing")
|
mock_sandbox = MagicMock(id="sandbox-existing")
|
||||||
|
|
||||||
|
|
@ -325,6 +325,13 @@ class TestRefreshProxyOnSandboxReuse:
|
||||||
new_callable=AsyncMock,
|
new_callable=AsyncMock,
|
||||||
return_value="sandbox-existing",
|
return_value="sandbox-existing",
|
||||||
),
|
),
|
||||||
|
# Thread is bound to the current repo, so reconnect proceeds (an unbound
|
||||||
|
# legacy thread would instead fail closed and recreate).
|
||||||
|
patch(
|
||||||
|
"agent.server.get_bound_repo_from_metadata",
|
||||||
|
new_callable=AsyncMock,
|
||||||
|
return_value="langchain-ai/open-swe",
|
||||||
|
),
|
||||||
patch("agent.server.create_sandbox", return_value=mock_sandbox) as mock_create,
|
patch("agent.server.create_sandbox", return_value=mock_sandbox) as mock_create,
|
||||||
patch(
|
patch(
|
||||||
"agent.server.get_github_app_installation_token_with_expiry",
|
"agent.server.get_github_app_installation_token_with_expiry",
|
||||||
|
|
|
||||||
|
|
@ -53,11 +53,24 @@ def test_unbound_token_served_when_repo_unknown() -> None:
|
||||||
assert github_token.get_github_token({"configurable": {"thread_id": "tid"}}) == "ghp_legacy"
|
assert github_token.get_github_token({"configurable": {"thread_id": "tid"}}) == "ghp_legacy"
|
||||||
|
|
||||||
|
|
||||||
|
def test_cached_token_reused_for_same_repo_case_insensitive() -> None:
|
||||||
|
"""``Org/Repo`` and ``org/repo`` are the same repo: no spurious refusal."""
|
||||||
|
future = (datetime.now(UTC) + timedelta(hours=1)).isoformat()
|
||||||
|
github_token.cache_github_token_for_thread(
|
||||||
|
"tid", "ghp_repoA", expires_at=future, repo={"owner": "Acme", "name": "Alpha"}
|
||||||
|
)
|
||||||
|
cfg = {"configurable": {"thread_id": "tid", "repo": {"owner": "acme", "name": "alpha"}}}
|
||||||
|
assert github_token.get_github_token(cfg) == "ghp_repoA"
|
||||||
|
|
||||||
|
|
||||||
def test_repo_cache_key_normalizes() -> None:
|
def test_repo_cache_key_normalizes() -> None:
|
||||||
assert github_token.repo_cache_key({"owner": "o", "name": "r"}) == "o/r"
|
assert github_token.repo_cache_key({"owner": "o", "name": "r"}) == "o/r"
|
||||||
assert github_token.repo_cache_key("o/r") == "o/r"
|
assert github_token.repo_cache_key("o/r") == "o/r"
|
||||||
assert github_token.repo_cache_key({"owner": "o"}) is None
|
assert github_token.repo_cache_key({"owner": "o"}) is None
|
||||||
assert github_token.repo_cache_key(None) is None
|
assert github_token.repo_cache_key(None) is None
|
||||||
|
# Casefolded so different casing of the same repo collapses to one key.
|
||||||
|
assert github_token.repo_cache_key({"owner": "Org", "name": "Repo"}) == "org/repo"
|
||||||
|
assert github_token.repo_cache_key("Org/Repo") == "org/repo"
|
||||||
|
|
||||||
|
|
||||||
# --- sandbox repo binding ----------------------------------------------------
|
# --- sandbox repo binding ----------------------------------------------------
|
||||||
|
|
@ -122,3 +135,91 @@ async def test_ensure_sandbox_allows_matching_repo(monkeypatch: pytest.MonkeyPat
|
||||||
result = await server.ensure_sandbox_for_thread("tid", repo={"owner": "acme", "name": "alpha"})
|
result = await server.ensure_sandbox_for_thread("tid", repo={"owner": "acme", "name": "alpha"})
|
||||||
assert result is backend
|
assert result is backend
|
||||||
assert calls["git"] == 1
|
assert calls["git"] == 1
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_ensure_sandbox_reuses_same_repo_case_insensitive(
|
||||||
|
monkeypatch: pytest.MonkeyPatch,
|
||||||
|
) -> None:
|
||||||
|
"""A bound sandbox is reused when the current repo differs only by casing."""
|
||||||
|
|
||||||
|
async def fake_sandbox_id(_tid: str) -> str:
|
||||||
|
return "sb-A"
|
||||||
|
|
||||||
|
async def fake_bound_repo(_tid: str) -> str:
|
||||||
|
return "acme/alpha"
|
||||||
|
|
||||||
|
backend = _FakeBackend("sb-A")
|
||||||
|
backend.bound_repo = "acme/alpha"
|
||||||
|
|
||||||
|
async def fake_check(b: Any, *_a: Any, **_k: Any) -> Any:
|
||||||
|
return b
|
||||||
|
|
||||||
|
async def fake_refresh(b: Any, *_a: Any, **_k: Any) -> Any:
|
||||||
|
return b
|
||||||
|
|
||||||
|
async def fake_git(_b: Any) -> None:
|
||||||
|
return None
|
||||||
|
|
||||||
|
monkeypatch.setattr(server, "get_sandbox_id_from_metadata", fake_sandbox_id)
|
||||||
|
monkeypatch.setattr(server, "get_bound_repo_from_metadata", fake_bound_repo)
|
||||||
|
monkeypatch.setattr(server, "check_or_recreate_sandbox", fake_check)
|
||||||
|
monkeypatch.setattr(server, "_refresh_github_proxy_or_recreate", fake_refresh)
|
||||||
|
monkeypatch.setattr(server, "set_sandbox_backend", lambda _tid, b, **_k: b)
|
||||||
|
monkeypatch.setattr(server, "_configure_git_identity", fake_git)
|
||||||
|
server.SANDBOX_BACKENDS["tid"] = backend # type: ignore[assignment]
|
||||||
|
|
||||||
|
# Different casing of the same repo must not raise and must reuse the sandbox.
|
||||||
|
result = await server.ensure_sandbox_for_thread("tid", repo={"owner": "ACME", "name": "Alpha"})
|
||||||
|
assert result is backend
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_legacy_unbound_sandbox_recreated_not_reused(
|
||||||
|
monkeypatch: pytest.MonkeyPatch,
|
||||||
|
) -> None:
|
||||||
|
"""A legacy thread (sandbox_id present, bound_repo absent) must fail closed.
|
||||||
|
|
||||||
|
With no recorded binding, the existing sandbox cannot be confirmed to belong
|
||||||
|
to the current repo, so it is never reconnected-and-served: a fresh sandbox is
|
||||||
|
created and bound to the requesting repo instead.
|
||||||
|
"""
|
||||||
|
|
||||||
|
async def fake_sandbox_id(_tid: str) -> str:
|
||||||
|
return "sb-legacy"
|
||||||
|
|
||||||
|
async def fake_bound_repo(_tid: str) -> None:
|
||||||
|
return None
|
||||||
|
|
||||||
|
def fake_reconnect(*_a: Any, **_k: Any) -> Any: # pragma: no cover - must not run
|
||||||
|
raise AssertionError("must not reconnect to a legacy unbound sandbox")
|
||||||
|
|
||||||
|
fresh = _FakeBackend("sb-fresh")
|
||||||
|
|
||||||
|
async def fake_create_with_proxy(*_a: Any, **_k: Any) -> Any:
|
||||||
|
return fresh
|
||||||
|
|
||||||
|
async def fake_git(_b: Any) -> None:
|
||||||
|
return None
|
||||||
|
|
||||||
|
class _Threads:
|
||||||
|
async def update(self, **_k: Any) -> None:
|
||||||
|
return None
|
||||||
|
|
||||||
|
async def get(self, *_a: Any, **_k: Any) -> dict[str, Any]:
|
||||||
|
return {}
|
||||||
|
|
||||||
|
class _Client:
|
||||||
|
threads = _Threads()
|
||||||
|
|
||||||
|
monkeypatch.setattr(server, "get_sandbox_id_from_metadata", fake_sandbox_id)
|
||||||
|
monkeypatch.setattr(server, "get_bound_repo_from_metadata", fake_bound_repo)
|
||||||
|
monkeypatch.setattr(server, "create_sandbox", fake_reconnect)
|
||||||
|
monkeypatch.setattr(server, "_create_sandbox_with_proxy", fake_create_with_proxy)
|
||||||
|
monkeypatch.setattr(server, "_configure_git_identity", fake_git)
|
||||||
|
monkeypatch.setattr(server, "client", _Client())
|
||||||
|
|
||||||
|
result = await server.ensure_sandbox_for_thread("tid", repo={"owner": "evil", "name": "beta"})
|
||||||
|
|
||||||
|
assert result.id == "sb-fresh"
|
||||||
|
assert server.SANDBOX_BACKENDS["tid"].bound_repo == "evil/beta"
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue