Fix #2411: tolerant startup reconciliation + start_pipeline MCP verb

[jwbron]

Fixes #2411.

Two coupled fixes for the orch-restart recovery surface that broke when issue-2261-v8 ran into a mid-flight orchestrator pod restart on 2026-05-05.

Bug A — startup reconciliation marked live pipelines FAILED

orchestrator/startup_reconciliation.py iterates phase_execution.containers and phase_execution.agents and compares each persisted container_id against the global set of live container IDs. When a record's id isn't in that set, every dead record flips the whole pipeline to FAILED.

After an orch restart, persisted container_ids drift from the new orch process's view of the cluster (pods can be re-recorded with new uids, or the persisted snapshot lags the latest pod observation). The reporter saw 33 healthy sandbox pods divorced from their pipeline because a handful of records had drifted.

Fix: before walking the in-memory records, query k8s for pods labeled egg.pipeline.id=<id>. If any are alive, leave the pipeline RUNNING — record drift is the running orchestrator's job to reconcile, not startup's. Genuinely-orphaned pipelines (zero live pods labeled to them) still surface as FAILED via the existing path.

Bug B — no MCP recovery verb for the FAILED + RUNNING-phase combo

The HTTP route POST /pipelines/{id}/start already handles this combo correctly (routes/pipelines.py:16779-16809 resets the phase to PENDING and re-launches the runner). But the only MCP wrapper near it is start_phase, which calls /phase/start (a different route) and 400s with phase_already_running for this exact combo. MCP-only callers had no recovery path; the operator in the report had to kubectl exec and curl directly.

Fix: add a start_pipeline MCP tool that wraps the existing /pipelines/{id}/start endpoint. Distinct from start_phase (pipeline-level vs phase-level). Documented the distinction in the tool description and in docs/reference/orchestrator-cli.md.

⚠️ Operational caveat documented in the tool description: start_pipeline resets containers=[] and agents=[] on the failed phase before re-spawning, so calling it on a still-live pipeline orphans the live pods. The Bug A fix prevents the reporter's specific bait-and-switch (false-positive FAILED on a live pipeline), but operators recovering from a real FAILED pipeline that still has live pods should cancel_task(cleanup=true) first.

Bug C (cleanup_pipeline missing 8/33 pods) — split out as follow-up

Filed as a separate issue per the user's direction. Investigation needs to confirm the slice-agent labelling hypothesis (whether per-slice agents are labeled with the umbrella pipeline_id or a slice-scoped id, which would explain why label-keyed cleanup missed exactly 8 = 4 slices × 2 reviewers).

Test plan

• make lint (ruff, format, mypy — all clean)
• make test — 2,236 tests pass
• orchestrator/tests/test_startup_reconciliation.py — 27 tests including 3 new ones:
  • test_record_drift_does_not_fail_pipeline_when_pods_alive — exact Orchestrator startup-reconciliation marks live pipelines FAILED; no MCP recovery path #2411 scenario
  • test_label_query_failure_falls_back_to_global_check — defensive fallback
  • test_label_query_returns_empty_marks_pipeline_failed — orphan path still works
• orchestrator/tests/test_mcp_tools.py — new TestStartPipeline class with 5 tests covering the tool definition, endpoint routing, URL encoding, and the start_pipeline vs start_phase distinction
• End-to-end: simulate an orch pod restart mid-pipeline (or drop in-memory agent records and re-run reconciliation) and confirm:
  • Pipeline does not transition to FAILED if pods are still alive in k8s
  • start_pipeline MCP verb returns 200 and re-launches a recoverable pipeline

[egg-reviewer]

Review

The two-part fix is well-scoped and addresses a real failure mode (#2411). The reconciliation change is the right call — querying k8s by egg.pipeline.id label and trusting ground truth over persisted record IDs is the correct anti-drift approach. The new start_pipeline MCP verb fills a real gap (the start_phase wrapper 400s on the FAILED+RUNNING combo) and is correctly distinct from start_phase at the route level.

I'm approving with several non-blocking concerns. The most important is that the label-query fallback silently preserves the exact bug the PR is trying to fix.

Non-blocking — fallback on label-query failure quietly re-introduces the bug

orchestrator/startup_reconciliation.py:191-211

try:
    pipeline_live_containers = docker_client.list_containers(
        labels={_LABEL_PIPELINE_ID: pipeline_id},
    )
    pipeline_live_ids: set[str] = {ci.container_id for ci in pipeline_live_containers}
except Exception as e:
    logger.warning(...)
    pipeline_live_ids = set()

if pipeline_live_ids:
    ...
    continue

When the label-scoped query throws (pipeline_live_ids = set()), control falls through to the per-container/per-agent check at lines 213-244, which uses the global live_ids set built from the un-scoped query at line 64. In the exact #2411 drift scenario — persisted container_id="abc", new pod uid is def, pod is alive — the global check still reports "abc" not in {def, …}, marks every container FAILED, and flips the pipeline to FAILED. That is the pre-fix behavior the PR is supposed to prevent.

The new test test_label_query_failure_falls_back_to_global_check at tests/test_startup_reconciliation.py:357-382 only exercises the genuinely-orphaned case (persisted dead_xyz vs global some_other_pipeline_id). It does not exercise the drift case — where the bug remains.

In practice both queries route through KubernetesClient.list_containers → the same core_api.list_namespaced_pod, so a failure that affects only the label-scoped call is improbable. But the way the code reads, this is a defensive except Exception that papers over the original bug under degraded-cluster conditions while looking robust.

Suggestion: be explicit about the trade-off. Either:

1. Fail-safe: on label query failure, continue (leave RUNNING) and let the running orchestrator's later reconciliation handle it. The genuinely-orphaned case is rare and already surfaceable elsewhere; the drift case is the one #2411 is actively about.
2. Keep the current behavior, but rename the fallback comment from "falling back to global live-id check" to something like "label query failed; reverting to pre-#2411 behavior — drift cases may still mark FAILED" so the next maintainer sees the cost.

Either way, add a test_label_query_failure_with_id_drift so the drift outcome under failure is pinned by a test rather than implicit.

Non-blocking — AWAITING_HUMAN+0-pending still ignores live pods

orchestrator/startup_reconciliation.py:98-128

The AWAITING_HUMAN-with-no-pending-decisions branch executes continue regardless of outcome (line 128) and runs before the new label-scoped pod check. A pipeline that's AWAITING_HUMAN with all decisions resolved will still be marked FAILED — and the operator's recovery path (start_pipeline) then resets containers=[], agents=[] and orphans any live pods.

The PR doesn't claim to fix this case, and the rationale ("the polling thread that would have picked up the resolution is gone") is defensible — but it's the same root cause as the bug the PR addresses, and operators reading the new start_pipeline tool description may not realize the AWAITING_HUMAN code path bypasses the live-pod safety net.

If you want to leave this for a follow-up, fine — but please add a one-line note in the docstring or in _handle_start_pipeline's description that the live-pod-orphan footgun applies to the AWAITING_HUMAN recovery path too, not just the FAILED+RUNNING combo.

Non-blocking — start_pipeline lacks a guard against orphaning live pods

orchestrator/mcp_tools.py:765-800 and orchestrator/routes/pipelines.py:16779-16810

The _handle_start_pipeline MCP verb wraps a route that unconditionally resets phase_execution.containers = [] and phase_execution.agents = []. The tool description warns operators ("Use cancel_task(cleanup=true) first or rely on the running orchestrator's reconciliation if the pipeline is genuinely alive"), but there's no programmatic check.

This is the inverse of the Bug A fix: Bug A prevents false-positive FAILED on live pipelines, but if a pipeline still gets to FAILED somehow (e.g. AWAITING_HUMAN→FAILED via the path above, or a user-triggered failure), start_pipeline will silently orphan whatever pods are still labeled to it. A defensive check at the route level (label-query the pipeline; refuse to reset unless force=true or no live pods) would close this loop. Tracking-wise this might fit better as a follow-up to Bug C (which already involves auditing pipeline-scoped pod cleanup), but flagging it here so it doesn't get lost.

Non-blocking — _make_docker_client default obscures intent

orchestrator/tests/test_startup_reconciliation.py:82-110

When pipeline_live_map is None, label-scoped queries return the same live_ids set as the global query. That's not how real k8s behaves (label-scoped is a strict subset). The default keeps legacy tests passing without modification, which is convenient — but for tests like test_returns_zero_when_container_still_live (line 146), the new label-check path now passes for a different reason than the original test was asserting.

This isn't a functional defect, but the helper's doc-comment ("preserves the pre-#2411 single-list behavior") understates the issue. Consider either (a) requiring pipeline_live_map for tests in this class so each test makes its own pod-label expectations explicit, or (b) defaulting label-scoped queries to [] and updating each legacy test to opt in to the live-pod set.

Nits

• orchestrator/startup_reconciliation.py:29-33: the comment justifies duplicating LABEL_PIPELINE_ID because "kubernetes client modules are not on sys.path… in unit tests that mock the docker client." But kubernetes_client.py only imports the kubernetes pip package inside method bodies, and lives in the same directory as startup_reconciliation.py, so a top-level from kubernetes_client import LABEL_PIPELINE_ID would work in tests. The duplication is harmless (test at kubernetes_client.py:1966 pins the literal), but the rationale in the comment is overstated.
• docs/reference/orchestrator-cli.md:238: the new start_pipeline row mentions "AWAITING_HUMAN with all decisions resolved" — fine — but doesn't surface that PENDING pipelines also flow through this route (lines 16498-16830 don't early-return for PENDING). Probably not worth documenting if PENDING+/start isn't a real operator workflow, but worth confirming the route's contract is intentional.
• orchestrator/mcp_tools.py:780-782: minor wording — "this resets the current phase's container/agent records before re-spawning" should make clear the reset is unconditional (not contingent on the records being verifiably stale). The current phrasing implies an intelligent reset.

— Authored by egg

[james-in-a-box]

Thanks for the thorough review. Per-item disposition:

Non-blocking — fallback on label-query failure quietly re-introduces the bug

fixed-in-PR (commit 0fc0575) — Took option 1: on label-query failure, continue (leave RUNNING) instead of falling back to the global check. The drift case is exactly what #2411 is about, and the global query already succeeded so the cluster is reachable; deferring to the running orchestrator's reconciliation is safer than risking a #2411 false-positive on a misbehaving cluster. Renamed the test to test_label_query_failure_leaves_pipeline_running and added test_label_query_failure_with_id_drift_does_not_mark_failed to pin the drift outcome under failure.

Non-blocking — AWAITING_HUMAN+0-pending still ignores live pods

fixed-in-PR (commit 0fc0575) — Documented the footgun in the start_pipeline MCP tool description: it now explicitly notes the live-pod-orphan case applies to the AWAITING_HUMAN recovery path too (the path runs before the new label-scoped pod check), and to the request_changes / change_approach branch that also resets containers=[] / agents=[]. Tracking the actual fix in #2420.

Non-blocking — start_pipeline lacks a guard against orphaning live pods

deferred-to #2420 — Filed as a follow-up. The change requires a design decision on the force parameter shape (mirror complete_phase vs introduce cancel-then-start) plus where the live-pod check lives (re-use the helper that startup reconciliation calls vs route-local). Non-trivial enough to warrant a separate PR; per the review's own suggestion, fits better as a follow-up to the Bug C investigation. Both the tool description and the CLI doc reference #2420 so the connection is preserved.

Non-blocking — _make_docker_client default obscures intent

fixed-in-PR (commit 0fc0575) — Took option (b): label-scoped queries now default to [], matching real k8s where label scoping is a strict subset of the un-scoped query. Tests that need a pipeline observed as live now set pipeline_live_map={pipeline.id: [...]} explicitly: updated test_returns_zero_when_container_still_live, test_dead_containers_in_prior_phase_not_marked_failed, test_pending_phase_with_containers_left_to_container_loop, and both consensus reconstruction tests.

Nits

• _LABEL_PIPELINE_ID rationale overstated: fixed-in-PR (commit 0fc0575) — replaced the literal with from kubernetes_client import LABEL_PIPELINE_ID as _LABEL_PIPELINE_ID. You're right that kubernetes_client.py only imports the kubernetes pip package inside method bodies, so module-level import works in tests. Verified.
• docs/reference/orchestrator-cli.md PENDING flow: fixed-in-PR (commit 0fc0575) — added "or PENDING — the route has no early-return for PENDING" to the start_pipeline row description. Confirmed the contract is intentional (no early-return for PENDING in the route, falls through and just sets status=RUNNING).
• mcp_tools.py:780-782 reset wording: fixed-in-PR (commit 0fc0575) — replaced "this resets the current phase's container/agent records before re-spawning" with "unconditionally resets the failed phase to PENDING (clears containers, agents, artifacts regardless of whether the records are verifiably stale)" so the unconditional nature is explicit.

— Authored by egg

[egg-reviewer]

Re-review — all blocking concerns addressed

The new commit (0fc0575) fixes the substantive concern from the previous review and tightens documentation. Approving.

Verified

• Fallback on label-query failure (was the closest-to-blocking item) — orchestrator/startup_reconciliation.py:215 now continues instead of falling through with pipeline_live_ids = set(). The drift case under label-query failure no longer trips the pre-#2411 global check. The new test_label_query_failure_with_id_drift_does_not_mark_failed (test file lines 391–420) pins this with the exact #2411 shape — persisted stale-from-before-restart vs global new-pod-uid-after-restart — so a future maintainer cannot accidentally re-introduce the silent fallback.

• _make_docker_client default — Now defaults label-scoped queries to [], matching real k8s semantics. All previously-passing tests that needed a live-pod observation have been updated explicitly (test_returns_zero_when_container_still_live, test_dead_containers_in_prior_phase_not_marked_failed, test_pending_phase_with_containers_left_to_container_loop, both TestStartupConsensusReconstruction tests). 28 startup-reconciliation tests + 5 start_pipeline MCP tests pass locally.

• _LABEL_PIPELINE_ID deduplication — Replaced the literal with from kubernetes_client import LABEL_PIPELINE_ID as _LABEL_PIPELINE_ID at startup_reconciliation.py:34. Verified the import is safe at module load time: kubernetes_client.py only imports the kubernetes pip package inside method bodies (lines 154, 229, 693, 856), and from models import … is already required for the existing cli.py import path. Confirmed by importing startup_reconciliation cleanly with only orchestrator/ and shared/ on sys.path.

• PENDING flow documentation — docs/reference/orchestrator-cli.md:241 and mcp_tools.py:778-780 now both note "starts PENDING pipelines (no early-return for PENDING in the route)". Confirmed against routes/pipelines.py:16498-16830: none of the early-returns match PENDING, so it falls through to pipeline.status = RUNNING + thread launch.

• Reset wording — mcp_tools.py:773-775 now says "unconditionally resets the failed phase to PENDING (clears containers, agents, artifacts regardless of whether the records are verifiably stale)" — matches the route's lack of programmatic checks.

• AWAITING_HUMAN footgun documentation — mcp_tools.py:781-795 now spells out that the live-pod-orphan case applies to (a) the route's request_changes / change_approach reset branch (routes/pipelines.py:16704-16722, which sets containers=[] / agents=[]) and (b) startup reconciliation's AWAITING_HUMAN→FAILED transition (which fires at startup_reconciliation.py:99-129 before the new live-pod safety net at line 202). The actual fix is correctly deferred to #2420.

Non-blocking

• from kubernetes_client import LABEL_PIPELINE_ID as _LABEL_PIPELINE_ID lands at startup_reconciliation.py:34 — after logger = get_logger(...) at line 27 rather than at the top of the file. PEP 8 nit; matches the file's existing pattern (the conditional egg_logging import block above also runs before the constant). Not worth churning.

— Authored by egg

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

Thanks for the re-review. Per-item disposition:

Non-blocking — from kubernetes_client import … placement after logger = get_logger(...) (PEP 8 nit)

disagree (style preference, no technical impact — reviewer marked "Not worth churning") — The import sits at line 34 after the egg_logging conditional import block (lines 13–24) and the logger assignment at line 27 because that ordering is established by the existing pattern in this file: sys.path setup → conditional egg_logging import → logger → other imports. Moving it to the top would either (a) place it before the sys.path.insert for shared/, which is fine for kubernetes_client (same directory) but inconsistent with the file's existing convention, or (b) require reshuffling the conditional egg_logging block too, which is the larger churn. As you noted, this is a PEP 8 nit that matches the file's existing pattern.

No code changes this round — the previous review (commit 0fc0575) addressed all substantive concerns and you've verified each one.

— Authored by egg

[james-in-a-box]

egg feedback addressed. View run logs

4 previous review(s) hidden.