Skip to content

Extract run_interruptible() from bridge modules to eliminate the 3× duplicated subprocess poll-wait-shutdown loop #313

Description

@arndvs

Problem

The bridge's shutdown-aware subprocess execution pattern — start a Popen in a new session, poll it in a tight loop checking both a shutdown flag and a wall-clock deadline, capture stdout/stderr to temp files, and call terminate_process_group on any failure path — is implemented independently in three modules:

Module Function Lines of loop logic
bridge/workspace.py _run_git() ~50
bridge/github.py mint_token() ~40
bridge/worker.py _wait_for_subprocess() + _run_subprocess() ~30

Issue #255 extracted the termination tail (terminate_process_group) but left the poll-wait loop — the harder, more bug-prone part — duplicated. Each copy uses the same control flow with different variable names and error wrappers. A change to poll interval, deadline semantics, or shutdown behavior must be applied three times.

Proposed solution

Extract a single run_interruptible() helper into bridge/subprocess_helpers.py (where terminate_process_group already lives) with this signature:

def run_interruptible(
    cmd: list[str],
    *,
    env: dict[str, str],
    timeout: float,
    shutdown_check: Callable[[], bool] | None = None,
    capture_output: bool = True,
    cwd: str | None = None,
    start_new_session: bool = True,
) -> subprocess.CompletedProcess:

When shutdown_check is None, delegate to subprocess.run (preserving the fast path all three callers already have). When provided, implement the poll loop once with terminate_process_group on all failure paths.

Each caller replaces its inline loop with a one-line call and keeps only its domain-specific error wrapping (WorkspaceError, GitHubError, RuntimeError).

Success criteria

  • run_interruptible lives in bridge/subprocess_helpers.py
  • workspace._run_git, github.mint_token, and worker._run_subprocess delegate to it
  • Unit tests cover: happy path, timeout, shutdown interruption, non-zero exit
  • Existing test/python/test_bridge_subprocess_helpers.py extended (not a new file)
  • No behavioral change to worker, webhook, or workspace lifecycle

Out of scope

ADR alignment

No ADR governs subprocess execution patterns. This proposal follows the project's stated preference: "Prefer removing or simplifying code over adding abstractions" (CONTEXT.md) — it removes ~120 lines of duplicated control flow and replaces them with one implementation.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    source:architecture-reviewPRDs proposed by the automated architecture-review workflow

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions