fix(gateway): gate worktree cleanup on pipeline liveness, never delete branches in orphan sweeps (#3070)

[jwbron]

Summary

Gateway-side half of #3070 (redeploy wiped in-flight pipeline state). On the 2026-06-10 redeploy, startup cleanup ran with active_containers=0 and force-removed every worktree — including pipeline-c978dac3, parked at an operator-approved refine HITL gate. The contract died with the worktree, and branch_deleted=True left the approved analysis reachable only as a dangling commit.

Root cause: the sweep equates "no live container" with "orphaned worktree". A pipeline parked at a HITL gate (or between phases) has no containers and no sessions — that is its normal state — so the #3005/#3006 session-store persistence fix structurally cannot protect it. This closes the liveness-gate gap deliberately deferred in #3005. Today, every gateway restart with a parked pipeline repeats the incident.

Changes

• New gateway/orchestrator_pipelines.py: asks the orchestrator which pipelines are non-terminal (GET /api/v1/pipelines?active_only=true, via the existing EGG_ORCHESTRATOR_URL). Hard invariant: failure returns None, never an empty set, so callers can distinguish "verified nothing active" from "could not verify".
• cleanup_orphaned_worktrees / list_orphan_worktree_dirs: preserve worktrees anchored to an active pipeline ({pid} and {pid}-*, delimiter-bound so issue-302 can't anchor issue-3023-*).
• startup_cleanup: the background cleanup thread polls the orchestrator (up to EGG_CLEANUP_ORCHESTRATOR_WAIT_SECONDS, default 600s — on a redeploy both pods restart together and the orchestrator's cold boot can take minutes) and skips the sweep entirely when liveness cannot be verified, logging at ERROR. Safe maintenance (git worktree prune, pack-file cleanup) still runs. Fail-safe cost: stale worktrees accumulate until the next verified startup or an operator prune — visible, recoverable, and strictly better than deleting live state.
• /api/v1/worktrees/prune: returns 503 instead of sweeping blind when the orchestrator can't answer; response/audit now include active_pipelines_count.
• Orphan sweeps never delete branches: the sweep can't know whether the work was pushed; worktree-dir removal alone is recoverable while branch deletion strands commits as dangling. Branch deletion stays with the explicit per-container teardown paths.

Composition with #3084

Cleanup now consults pipeline records; #3084 (orchestrator half) makes those records survive the restart. Without #3084 a freshly-restarted orchestrator wouldn't know about in-flight prompt-driven pipelines and this gating would have a stale answer — land both.

Testing

• pytest gateway/tests/ — 3286 passed; the 3 test_phase_api path-traversal failures pre-exist on clean main (environment-specific socket-bind denial, unrelated).
• New: 10 tests for the client module; pipeline-anchor preserve/delimiter/no-branch-deletion/dry-run-mirror tests for the manager; fail-safe skip test for startup_cleanup; 503 + forwarding tests for the prune route.

Related

#3070 (incident), #3005/#3006 (deferred liveness gate), #3084 (orchestrator half)

[egg-reviewer]

Reviewed the full diff and traced data flow end-to-end. The fix correctly closes the #3070 gap: the orchestrator-derived active-pipeline set is the only signal that can distinguish a HITL-parked pipeline (no containers, no sessions) from a crashed leftover, and the None vs empty-set discipline on the client is the right shape for a fail-safe. The defense-in-depth delete_branch=False on the orphan path is independently valuable — even if the pipeline-anchor check is bypassed by stale orchestrator state, branch refs survive and commits stay reachable.

The 503 short-circuit, the bounded poll on startup, the delimiter-bound prefix matcher (pid vs f"{pid}-"), and the dry-run path mirroring all line up. Tests cover the regression scenarios I'd want — preserved live worktrees, near-miss prefix collision, branch-deletion suppression, dry-run mirror, 503 forwarding, and the startup fail-safe.

A few non-blocking observations below; none of them gate this merge.

Non-blocking

1. cleanup_stale_pipeline_worktrees is the same shape of bug, deferred. gateway/worktree_manager.py:2106 and :2186 still call remove_worktree(..., delete_branch=True), and the function doesn't consult active_pipeline_ids. Its docstring marks it TODO: Wire this into the orchestrator's maintenance loop. Currently only called from tests. Today that means it's harmless — but the moment someone wires it up to scheduled maintenance, an idle HITL-parked pipeline (slow operator review, ticket sitting for a day) becomes the next #3070. Worth applying the same delete_branch=False + pipeline-anchor skip now while the contract is fresh in mind, rather than leaving a tripwire that re-introduces the bug on its first production use.

2. Stdlib logger fallback in orchestrator_pipelines.py:26-35 would crash if ever activated. Every logger.warning(...) / logger.info(...) in this module passes kwargs (url=, error=, count=, attempts=, max_wait_seconds=). egg_logging.EggLogger.warning accepts them; stdlib logging.Logger.warning does not — it would raise TypeError: _log() got an unexpected keyword argument 'url' and unwind through the except block instead of returning None. The bigger background-thread except Exception catches it, so practically you'd get a silently-failed startup cleanup. In reality egg_logging is a hard dep elsewhere in the gateway (gateway.py:71, worktree_manager.py:35, confluence_client.py:102, …), so the ImportError branch is dead code. Two options to make it consistent: drop the try/except and from egg_logging import get_logger directly (matches gateway.py, simpler), or have the fallback wrap stdlib so kwargs route through extra=.

3. Dev/local startup pays the 600s wait. With the default EGG_CLEANUP_ORCHESTRATOR_WAIT_SECONDS=600, a gateway started against a non-existent orchestrator (developer laptop, isolated test container) spins the background cleanup thread for the full 10 minutes before logging ERROR and giving up — urlopen against http://egg-orchestrator:9849 will throw URLError ~immediately, so the loop just sleeps and retries until the deadline. Acknowledged in the design as the right fail-safe cost in prod, but worth a one-liner in the gateway README pointing devs at EGG_CLEANUP_ORCHESTRATOR_WAIT_SECONDS=0 (which the env-var-deadline test already exercises). Even better, gate the wait on a runtime signal — e.g., skip the wait entirely if EGG_CONTAINER is unset.

4. Looser anchor than list_worktrees_for_pipeline — intentional, but worth a comment cross-reference. _is_pipeline_anchored uses startswith(f"{pid}-"); list_worktrees_for_pipeline (worktree_manager.py:1571-1584) uses the stricter {pid}-[a-z_]+ regex because of the #1865 issue-1758 vs issue-1758-worktree-fix-tester collision. The looser check here is correct — slice-scoped suffixes like issue-3023-slice-1-coder contain digits and won't match [a-z_]+, so the regex would falsely orphan them. The cost is over-preservation when one active pipeline ID is a prefix of another inactive one, which is on the right side of the fail-safe principle. A one-line comment in _is_pipeline_anchored pointing at list_worktrees_for_pipeline + #1865 would save a future reader the same archaeology.

5. test_returns_none_on_malformed_body doesn't quite cover what it claims. The body stub in that test is missing the read() and decode() chain that fetch_active_pipeline_ids actually performs — it relies on BytesIO.read() already returning bytes, which works, but a json.loads(b"not json".decode("utf-8")) is what's being exercised, not a malformed-body-from-the-network path. The test passes for the right reason (JSONDecodeError → None), but if you ever swap urlopen for requests you'd need to revisit. Minor.

— Authored by egg

[james-in-a-box]

Thanks for the thorough review. All five non-blocking observations addressed in commit 9e8be45 — defaulted to in-PR fixes since none of them needed a HITL decision and none asked for follow-up.

1. cleanup_stale_pipeline_worktrees — same shape, fix it now — fixed-in-PR (commit 9e8be45). Added active_pipeline_ids parameter to cleanup_stale_pipeline_worktrees mirroring cleanup_orphaned_worktrees, switched the removal call to delete_branch=False with the same rationale (an age sweep cannot tell whether the branch was pushed), and updated the docstring TODO to note that whoever wires this into maintenance must pass the orchestrator-derived set. Updated the existing test_removes_old_worktrees to assert delete_branch=False, added two new tests covering pipeline-level and per-agent active-pipeline preservation.

2. Stdlib logger fallback would crash — fixed-in-PR (commit 9e8be45). Agreed — egg_logging is a hard dep across every other gateway module (gateway.py:71, worktree_manager.py:35, …) so the ImportError branch is dead code, and if it ever activated the kwargs would raise. Dropped the try/except and from egg_logging import get_logger directly, matching gateway.py's pattern.

3. Dev/local startup pays the 600s wait — fixed-in-PR (commit 9e8be45). Took the simpler of the two options (README note over EGG_CONTAINER runtime gate — the latter would need a new convention; EGG_CONTAINER isn't an established gateway env var). Added a "Local Development" subsection to gateway/README.md pointing devs at EGG_CLEANUP_ORCHESTRATOR_WAIT_SECONDS=0 for the single-attempt path, with the rationale that local dev has nothing to preserve so fail-safe-skipping immediately is the right behavior.

4. Looser anchor than list_worktrees_for_pipeline — fixed-in-PR (commit 9e8be45). Added a comment block in _is_pipeline_anchored explaining why the startswith(f"{pid}-") anchor is intentionally looser than list_worktrees_for_pipeline's {pid}-[a-z_]+ regex (slice-scoped suffixes like issue-3023-slice-1-coder contain digits), with cross-references to that function and #1865.

5. test_returns_none_on_malformed_body coverage gap — fixed-in-PR (commit 9e8be45). Factored a _raw_response(body_bytes) helper that builds the urlopen stub from raw bytes, with _response(payload) now delegating to it. The malformed-body test now uses the same helper as the success path so the full with urlopen(...) as resp: resp.read().decode("utf-8") chain is exercised — only the bytes the server returns differ between the two tests.

Verified locally: pytest gateway/tests/test_orchestrator_pipelines.py (10 passed), pytest orchestrator/tests/test_worktree_hitl.py::TestCleanupStalePipelineWorktrees (7 passed, including the 2 new ones), and the broader gateway/tests/test_worktree_manager.py cleanup/orphan/startup subset (28 passed). The 2 pre-existing shared/egg_agent/client.py mypy errors flagged by make lint are unrelated to this PR (present on the parent commit).

— Authored by egg

[egg-reviewer]

Re-reviewed the delta 689d515..9e8be45 against the five non-blocking observations from the prior pass. All addressed cleanly; no new issues.

Verification of prior observations

1. cleanup_stale_pipeline_worktrees shape-bug — fixed. gateway/worktree_manager.py:2116 adds active_pipeline_ids, the anchor check is inserted before the mtime walk (:2163), and the removal call at :2218 flips to delete_branch=False. The docstring TODO now tells the future wirer-upper to pass the orchestrator-derived set. Two new tests (test_preserves_active_pipeline_worktrees, test_preserves_per_agent_active_pipeline_worktrees) cover both the {pid} and {pid}-{role} shapes with stale mtimes, and the existing test_removes_old_worktrees is updated to assert delete_branch=False. The remaining delete_branch=True in the file (cleanup_clean_worktree, :2113) is the explicit per-container teardown path that the PR body deliberately retains as the only branch-deletion entry point — consistent with the design.

2. Stdlib logger fallback — fixed. gateway/orchestrator_pipelines.py now does from egg_logging import get_logger directly and the import logging is gone. Matches gateway/anthropic_credentials.py:25, gateway/confluence_client.py:102, etc. The dead ImportError branch (which would have TypeError'd on every kwarg call site) is removed.

3. Local-dev 600s wait — fixed. New "Local Development" subsection in gateway/README.md points devs at EGG_CLEANUP_ORCHESTRATOR_WAIT_SECONDS=0 with the right rationale ("nothing to preserve" → fail-safe-skip immediately is the correct local behavior).

4. Anchor-pattern cross-reference — fixed. _is_pipeline_anchored (worktree_manager.py:1627-1632) now carries the comment explaining why the looser startswith(f"{pid}-") is intentional vs list_worktrees_for_pipeline's {pid}-[a-z_]+ regex, with the slice-scoped-digit example and the #1865 + #3070 cross-references.

5. Malformed-body test coverage — fixed. _raw_response(body_bytes) factored out, _response(payload) delegates to it, and test_returns_none_on_malformed_body now goes through the same stub shape as test_returns_ids_on_success. The full with urlopen(...) as resp: resp.read().decode("utf-8") chain is exercised; only the bytes differ.

Delta-pass scrutiny

Read every changed file against the rest of the codebase:

• The new test docstrings correctly describe what each test guards (parked-HITL preservation regardless of mtime).
• active_pipeline_ids=None continues to mean "skip the anchor check" — the function remains test-only, so no production caller is silently affected by the new parameter.
• The README note's described behavior (600s default, single-attempt-on-zero) matches wait_for_active_pipeline_ids's control flow exactly.
• Dropping the stdlib fallback hardens the module's contract (egg_logging is now required at import time, not silently degraded at first call site), which is the right tradeoff given every other gateway module assumes it.

LGTM.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

3 previous review(s) hidden.