From 19c6ce878d6c2cd4422a844d05292710f61c138d Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Tue, 5 May 2026 14:18:46 -0700 Subject: [PATCH 1/2] Fix #2398: share one synthetic session across slice integration branch creation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `create_slice_integration_branch` previously made three sequential gateway calls — `fetch_branch`, `get_remote_branch_sha`, and the push — each registering and deleting its own synthetic session. For a 15-slice pipeline that's 45 session lifecycles where one would do. Add an optional keyword-only `bearer_token` to `fetch_branch` and `get_remote_branch_sha`: when supplied, they skip the per-call register/delete and authenticate with the caller's token. Then move the `register_session` in `create_slice_integration_branch` to run before the fetch/ls-remote/push sequence and pass that one token to all three. The session metadata is the strictest of the three (push needs `branch=integration_branch` + `agent_role` for the slice-integration-branch exemption); the fetch and ls-remote endpoints accept any synthetic session, so the extra metadata is harmless there. 15-slice pipelines now register 15 sessions instead of 45. Two new tests in `test_create_slice_integration_branch.py` pin the contract: the same bearer token reaches `fetch_branch`, `get_remote_branch_sha`, and the push, and the session is still cleaned up when the parent is missing on origin (which now runs after register_session, so the cleanup path matters). Closes #2398. --- orchestrator/gateway_client.py | 137 +++++++++++------- .../test_create_slice_integration_branch.py | 70 +++++++++ 2 files changed, 153 insertions(+), 54 deletions(-) diff --git a/orchestrator/gateway_client.py b/orchestrator/gateway_client.py index c9b1b948f8..39a5548f3d 100644 --- a/orchestrator/gateway_client.py +++ b/orchestrator/gateway_client.py @@ -1611,39 +1611,15 @@ def create_slice_integration_branch( # No-op: integration branch already exists at parent's tip. return True - # Refresh the local remote-tracking ref + odb so the parent's - # commit object is available for the push below. ``git push - # :refs/heads/...`` requires the source object to be - # locally reachable; the fetch makes that true even when the - # worktree was just created and has never seen this ref. - # Best-effort: if it fails the object may already be local - # from a prior step, so we still attempt the ls-remote / push. - self.fetch_branch( - pipeline_id, - repo_path, - args=[f"+refs/heads/{parent_branch}:refs/remotes/origin/{parent_branch}"], - mode=mode, - ) - - # Resolve the parent to a SHA on origin. Failing fast here - # produces a clear "parent not found" error instead of git's - # confusing ``src refspec X does not match any``. - parent_sha = self.get_remote_branch_sha( - pipeline_id, - repo_path, - f"refs/heads/{parent_branch}", - mode=mode, - ) - if not parent_sha: - logger.warning( - "Parent branch not found on origin; cannot create slice integration branch", - pipeline_id=pipeline_id, - integration_branch=integration_branch, - parent_branch=parent_branch, - ) - return False - + # Register one synthetic session up front and share it across + # the fetch, ls-remote, and push (#2398). The session is + # tagged for the push (branch=integration_branch + agent_role + # — required for the gateway's slice integration-branch + # exemption and branch-ownership check); the fetch and + # ls-remote endpoints accept any synthetic session, so the + # extra metadata is harmless for those calls. temp_container_id = f"{pipeline_id}-slice-branch-{integration_branch.replace('/', '-')}" + parent_sha: str | None = None session_token: str | None = None try: session = self.register_session( @@ -1657,6 +1633,41 @@ def create_slice_integration_branch( ) session_token = session.session_token + # Refresh the local remote-tracking ref + odb so the + # parent's commit object is available for the push below. + # ``git push :refs/heads/...`` requires the source + # object to be locally reachable; the fetch makes that + # true even when the worktree was just created and has + # never seen this ref. Best-effort: if it fails the + # object may already be local from a prior step, so we + # still attempt the ls-remote / push. + self.fetch_branch( + pipeline_id, + repo_path, + args=[f"+refs/heads/{parent_branch}:refs/remotes/origin/{parent_branch}"], + mode=mode, + bearer_token=session_token, + ) + + # Resolve the parent to a SHA on origin. Failing fast + # here produces a clear "parent not found" error instead + # of git's confusing ``src refspec X does not match any``. + parent_sha = self.get_remote_branch_sha( + pipeline_id, + repo_path, + f"refs/heads/{parent_branch}", + mode=mode, + bearer_token=session_token, + ) + if not parent_sha: + logger.warning( + "Parent branch not found on origin; cannot create slice integration branch", + pipeline_id=pipeline_id, + integration_branch=integration_branch, + parent_branch=parent_branch, + ) + return False + refspec = f"{parent_sha}:refs/heads/{integration_branch}" self._make_request( "/api/v1/git/push", @@ -1935,6 +1946,8 @@ def fetch_branch( repo_path: str, args: list[str] | None = None, mode: Literal["public", "private"] = "public", + *, + bearer_token: str | None = None, ) -> bool: """Fetch with custom args using a temporary session. @@ -1944,21 +1957,28 @@ def fetch_branch( pipeline_id: Pipeline ID (used as container_id for the temp session) repo_path: Path to the repo directory args: Additional args for git fetch (e.g., ["+remote:local"]) + bearer_token: Pre-registered synthetic session token to reuse + (#2398). When provided, skip the internal + ``register_session``/``delete_session`` and authenticate + the fetch with the supplied token — lets a caller share + one session across several gateway calls. Returns: True if fetch succeeded, False otherwise """ - temp_container_id = f"{pipeline_id}-state-fetch" - session_token: str | None = None + owns_session = bearer_token is None + session_token: str | None = bearer_token try: - session = self.register_session( - container_id=temp_container_id, - container_ip=self.self_ip, - mode=mode, - pipeline_id=pipeline_id, - synthetic=True, - ) - session_token = session.session_token + if owns_session: + temp_container_id = f"{pipeline_id}-state-fetch" + session = self.register_session( + container_id=temp_container_id, + container_ip=self.self_ip, + mode=mode, + pipeline_id=pipeline_id, + synthetic=True, + ) + session_token = session.session_token # Do NOT include container_id — repo_path is already the # resolved path; the synthetic container_id has no real @@ -1989,7 +2009,7 @@ def fetch_branch( ) return False finally: - if session_token: + if owns_session and session_token: try: self.delete_session(session_token) except Exception: @@ -2063,6 +2083,8 @@ def get_remote_branch_sha( repo_path: str, ref: str, mode: Literal["public", "private"] = "public", + *, + bearer_token: str | None = None, ) -> str | None: """Resolve a remote ref to its commit SHA via ``git ls-remote``. @@ -2073,18 +2095,25 @@ def get_remote_branch_sha( different SHA than ``origin/``, the branch carries prior-pipeline commits and starting on top of it would inherit them — so refuse with a hint to ``cancel_task(cleanup=true)``. + + ``bearer_token`` lets a caller pass in a pre-registered synthetic + session to share across several gateway calls (#2398). When + provided, the per-call ``register_session``/``delete_session`` + round-trip is skipped. """ - temp_container_id = f"{pipeline_id}-state-ls-remote-sha" - session_token: str | None = None + owns_session = bearer_token is None + session_token: str | None = bearer_token try: - session = self.register_session( - container_id=temp_container_id, - container_ip=self.self_ip, - mode=mode, - pipeline_id=pipeline_id, - synthetic=True, - ) - session_token = session.session_token + if owns_session: + temp_container_id = f"{pipeline_id}-state-ls-remote-sha" + session = self.register_session( + container_id=temp_container_id, + container_ip=self.self_ip, + mode=mode, + pipeline_id=pipeline_id, + synthetic=True, + ) + session_token = session.session_token result = self._make_request( "/api/v1/git/fetch", @@ -2113,7 +2142,7 @@ def get_remote_branch_sha( ) return None finally: - if session_token: + if owns_session and session_token: try: self.delete_session(session_token) except Exception: diff --git a/orchestrator/tests/test_create_slice_integration_branch.py b/orchestrator/tests/test_create_slice_integration_branch.py index bafa40d88c..6a0db30465 100644 --- a/orchestrator/tests/test_create_slice_integration_branch.py +++ b/orchestrator/tests/test_create_slice_integration_branch.py @@ -347,3 +347,73 @@ def test_synthetic_session_carries_integration_branch_and_role(self, gateway_cli assert kwargs["agent_role"] == "coder" assert kwargs["mode"] == "private" assert kwargs["pipeline_id"] == "pipe-1" + + def test_one_session_shared_across_fetch_lsremote_and_push(self, gateway_client): + """#2398: the fetch, ls-remote, and push must reuse a single + synthetic session — exactly one ``register_session`` and one + ``delete_session`` per call, with the same bearer token + forwarded to ``fetch_branch``, ``get_remote_branch_sha``, and + the push request.""" + register_spy = MagicMock(return_value=_session_info("shared-tok")) + delete_spy = MagicMock(return_value=True) + fetch_spy = MagicMock(return_value=True) + ls_spy = MagicMock(return_value="cafebabe" * 5) + + push_tokens: list[str | None] = [] + + def fake_make_request(endpoint, method=None, data=None, **kwargs): + if endpoint == "/api/v1/git/push": + push_tokens.append(kwargs.get("bearer_token")) + return {"success": True, "data": {}} + + with ( + patch.object(gateway_client, "register_session", side_effect=register_spy), + patch.object(gateway_client, "delete_session", side_effect=delete_spy), + patch.object(gateway_client, "fetch_branch", side_effect=fetch_spy), + patch.object(gateway_client, "get_remote_branch_sha", side_effect=ls_spy), + patch.object(gateway_client, "_make_request", side_effect=fake_make_request), + ): + ok = gateway_client.create_slice_integration_branch( + "pipe-1", + "/repo", + integration_branch="egg/issue-2393/slice-1", + parent_branch="egg/issue-2393", + ) + + assert ok is True + assert register_spy.call_count == 1, "session must be registered exactly once" + assert delete_spy.call_args_list == [((("shared-tok"),), {})], ( + "session must be deleted exactly once with the shared token" + ) + assert fetch_spy.call_args.kwargs.get("bearer_token") == "shared-tok" + assert ls_spy.call_args.kwargs.get("bearer_token") == "shared-tok" + assert push_tokens == ["shared-tok"] + + def test_session_cleaned_up_when_parent_missing(self, gateway_client): + """With the shared-session refactor (#2398) ``register_session`` + runs before the ``ls-remote`` SHA lookup, so a missing parent + still has to clean up the session it just registered.""" + register_spy = MagicMock(return_value=_session_info("orphan-tok")) + delete_spy = MagicMock(return_value=True) + + with ( + patch.object(gateway_client, "register_session", side_effect=register_spy), + patch.object(gateway_client, "delete_session", side_effect=delete_spy), + patch.object(gateway_client, "fetch_branch", return_value=True), + patch.object(gateway_client, "get_remote_branch_sha", return_value=None), + patch.object( + gateway_client, + "_make_request", + return_value={"success": True, "data": {}}, + ), + ): + ok = gateway_client.create_slice_integration_branch( + "pipe-1", + "/repo", + integration_branch="egg/issue-2393/slice-1", + parent_branch="egg/issue-2393", + ) + + assert ok is False + assert register_spy.call_count == 1 + assert delete_spy.call_args_list == [((("orphan-tok"),), {})] From ebe86c11a85e86c211226ced48b6149789181327 Mon Sep 17 00:00:00 2001 From: "egg-reviewer[bot]" <261018737+egg-reviewer[bot]@users.noreply.github.com> Date: Tue, 5 May 2026 21:33:28 +0000 Subject: [PATCH 2/2] Address review: clarify docstrings + use mock.call() in assertions - fetch_branch / get_remote_branch_sha docstrings now note that pipeline_id is only used for log fields and that mode is ignored when bearer_token is supplied (the supplied session's mode was fixed at register time). - Switch the two delete_spy.call_args_list assertions in test_create_slice_integration_branch.py to mock.call() so the expected call list reads as [call("shared-tok")] instead of the unusual [((("shared-tok"),), {})] tuple form. All three are non-blocking notes from the egg-reviewer LGTM on #2405. --- orchestrator/gateway_client.py | 11 +++++++++-- .../tests/test_create_slice_integration_branch.py | 6 +++--- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/orchestrator/gateway_client.py b/orchestrator/gateway_client.py index 39a5548f3d..d539524a08 100644 --- a/orchestrator/gateway_client.py +++ b/orchestrator/gateway_client.py @@ -1954,9 +1954,15 @@ def fetch_branch( Best-effort operation used to fetch specific refs from remote. Args: - pipeline_id: Pipeline ID (used as container_id for the temp session) + pipeline_id: Pipeline ID; used as ``container_id`` for the + temp session and for log fields. When ``bearer_token`` + is supplied no session is registered, so it's only used + for log fields in that case. repo_path: Path to the repo directory args: Additional args for git fetch (e.g., ["+remote:local"]) + mode: Network mode for the temp session. Ignored when + ``bearer_token`` is supplied — the supplied session's + mode was fixed at its register time. bearer_token: Pre-registered synthetic session token to reuse (#2398). When provided, skip the internal ``register_session``/``delete_session`` and authenticate @@ -2099,7 +2105,8 @@ def get_remote_branch_sha( ``bearer_token`` lets a caller pass in a pre-registered synthetic session to share across several gateway calls (#2398). When provided, the per-call ``register_session``/``delete_session`` - round-trip is skipped. + round-trip is skipped, and the ``mode`` argument is ignored — + the supplied session's mode was fixed at its register time. """ owns_session = bearer_token is None session_token: str | None = bearer_token diff --git a/orchestrator/tests/test_create_slice_integration_branch.py b/orchestrator/tests/test_create_slice_integration_branch.py index 6a0db30465..55d0ddfd32 100644 --- a/orchestrator/tests/test_create_slice_integration_branch.py +++ b/orchestrator/tests/test_create_slice_integration_branch.py @@ -18,7 +18,7 @@ """ from datetime import datetime, timedelta -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock, call, patch import pytest from gateway_client import GatewayClient, SessionInfo @@ -382,7 +382,7 @@ def fake_make_request(endpoint, method=None, data=None, **kwargs): assert ok is True assert register_spy.call_count == 1, "session must be registered exactly once" - assert delete_spy.call_args_list == [((("shared-tok"),), {})], ( + assert delete_spy.call_args_list == [call("shared-tok")], ( "session must be deleted exactly once with the shared token" ) assert fetch_spy.call_args.kwargs.get("bearer_token") == "shared-tok" @@ -416,4 +416,4 @@ def test_session_cleaned_up_when_parent_missing(self, gateway_client): assert ok is False assert register_spy.call_count == 1 - assert delete_spy.call_args_list == [((("orphan-tok"),), {})] + assert delete_spy.call_args_list == [call("orphan-tok")]