Fix restart_phase wedge on sliced implement phases

[jwbron]

Summary

Fixes #2914

restart_phase on a sliced implement phase was wedging the pipeline. After stopping containers and clearing per-slice consensus trackers, the bootstrap reconciler would classify the slice as "resume" and call scheduler.mark_spawned(slice_id) — telling the run loop the cohort was already live. No agents would ever spawn, and the pipeline would hang indefinitely.

Root Cause

The bootstrap reconciliation's Layer-C classifier checked contract state and git commits to distinguish "resume" from "fresh", but never verified the slice's agents were actually running. After restart_phase stops all containers, the contract still shows IN_PROGRESS with commits, so the reconciler classified as "resume" and marked the slice as spawned — even though no pods existed.

Fix

• _slice_agents_alive(pipeline_id, slice_id): Queries k8s for live pods labeled with both egg.pipeline.id and egg.slice.id. Returns True only if at least one pod is in a live state (Pending/Creating/Running). Returns False on zero pods or API errors — the conservative default forces re-spawn rather than risking a wedge.

• Modified resume branch (~line 16149 in routes/pipelines.py): Before calling scheduler.mark_spawned(s.id), verify agents are actually live. If not alive, treat as "fresh" instead (don't mark_spawned, let the scheduler re-yield READY and spawn a new cohort).

Testing

Added 6 unit tests in tests/orchestrator/test_slice_4_restart_hardening.py:

• Returns True when running/pending pods exist
• Returns False when no pods or only terminal (Exited/Failed) pods exist
• Returns False on k8s API errors (defensive: force re-spawn on uncertainty)
• Verifies label selector includes both egg.pipeline.id and egg.slice.id

59 passed in tests/orchestrator/test_slice_4_restart_hardening.py

Impact

Restores restart_phase as a viable recovery for failed/wedged slices in sliced implement phases. Operators can now use restart_phase instead of having to cancel and resubmit the entire pipeline.

[egg-reviewer]

The fix correctly targets the wedge described in #2914: the bootstrap reconciler's "resume" classification looks at contract state (IN_PROGRESS + commits on origin) but never verifies the producer cohort is actually alive. After restart_phase tears containers down, contract still shows IN_PROGRESS with commits, classifier returns "resume", mark_spawned flips the slice to RUNNING in the scheduler, and iter_ready (slice_scheduler.py:287) never re-yields it — wedge.

Adding a k8s pod-presence guard before mark_spawned is the right shape. LIVE_POD_STATUSES covers Pending/Creating/Running per models.py:104-108, the label query mirrors list_slice_jobs (kubernetes_spawner.py:1118-1120), and the conservative False-on-error default biases toward re-spawn rather than a wedge. Spawn idempotency (kubernetes_spawner.py:548-562 pre-cleans existing Jobs by deterministic name) means double-spawn from a transient list_containers failure is safe.

I traced the flow and found no blocking issues. The reclassified-fresh path leaves the contract status as IN_PROGRESS (intentional: TASK-4-2 PENDING→IN_PROGRESS flip is idempotent at pipelines.py:~16424), the scheduler stays at READY, the run loop re-yields and respawns at pipelines.py:16849-16850, and _resolve_slice_base_branch + create_slice_integration_branch are idempotent on re-entry.

Non-blocking suggestions

1. Missing integration test for the new branch. TestSliceAgentsAlive covers the helper in isolation, but nothing exercises the actual integration point at pipelines.py:16150-16166. A regression that e.g. inverts the condition or stops calling the helper would not be caught. test_slice_run_loop_integration.py already has a test_bootstrap_* family — a sibling test that stands up a slice with status=IN_PROGRESS, commits on origin, and _get_spawner().backend.list_containers returning [] would close the gap and would have caught the original bug.

2. ContainerStatus.CREATING not in the test matrix. _LIVE_POD_STATUSES includes CREATING (models.py:106), and Job→Pod transitions go through it. Tests cover RUNNING and PENDING but not CREATING. Inexpensive to add — same shape as test_true_when_pending_pod_exists.

3. Pod-Terminating race during restart_phase. A pod whose Job has been deleted but whose container is still in its termination grace period reports phase=Running (kubernetes_client.py:85-94 maps Running→RUNNING, no Terminating handling). restart_phase step 4 does remove_agent_container(force=True) (Foreground propagation, which should block until pods are gone) but step 4 also silently logs-and-continues on remove_container failure (pipelines.py:3236-3242). On a transient k8s API error during teardown, the bootstrap could still observe a "live" pod that is actually dead and re-wedge. Not a regression introduced by this PR, but worth a one-line comment on _slice_agents_alive calling out the assumption: callers must have already torn down with foreground propagation, otherwise the helper can false-positive against terminating pods.

4. Minor — _slice_agents_alive re-fetches the spawner singleton via _get_spawner() rather than accepting it as a parameter. The bootstrap caller already has spawner in scope (used three lines above at pipelines.py:16129). It's a singleton so the cost is negligible, but threading it through would make the helper testable without a patch("routes.pipelines._get_spawner", …) and would parallel how _classify_non_complete_slice receives gateway as a parameter.

5. Tiny — comment placement at pipelines.py:16195-16196. The trailing comment about bootstrap_reclassified_fresh is wedged inside the bootstrap_blocked-rationale block, which makes it look like a continuation of that note. Consider promoting it to its own paragraph above the logger.info call so future readers don't conflate the two reasons.

— Authored by egg

[james-in-a-box]

Thanks for the thorough review and approval. All five non-blocking suggestions are addressed in commit 458d88b.

Per-item disposition

1. Missing integration test for the new branch — fixed-in-PR (commit 458d88b)
   New TestBootstrapResumeAliveGuard::test_resume_with_no_live_pods_respawns_fresh in orchestrator/tests/test_slice_run_loop_integration.py exercises the call site end-to-end through _run_implement_phase_slices: slice in IN_PROGRESS with commits on origin (classifier → "resume") + list_containers returning [] → _run_concurrent_phase IS invoked for the slice, and the liveness probe fires with the slice label. A regression that inverts the new condition or drops the helper call would fail this test.

2. ContainerStatus.CREATING not in test matrix — fixed-in-PR (commit 458d88b)
   Added test_true_when_creating_pod_exists mirroring the RUNNING/PENDING shape. Covers the Job→Pod CREATING transition.

3. Pod-Terminating race during restart_phase — fixed-in-PR (commit 458d88b)
_slice_agents_alive's docstring now spells out the caller contract: callers must have already torn down stale cohorts with foreground propagation, because kubernetes_client.py maps phase=Running → RUNNING without a Terminating-specific status, so without foreground teardown the helper can false-positive against terminating pods.

4. Minor — _slice_agents_alive re-fetches via _get_spawner() — fixed-in-PR (commit 458d88b)
   Signature is now _slice_agents_alive(spawner, pipeline_id, slice_id). Parallels _classify_non_complete_slice accepting gateway. The Layer-C call site already has spawner in scope (used at pipelines.py:16129), and unit tests now inject a stub directly instead of patching routes.pipelines._get_spawner.

5. Tiny — comment placement at pipelines.py:16195-16196 — fixed-in-PR (commit 458d88b)
   Promoted the bootstrap_reclassified_fresh note to its own paragraph above the logger.info call so it reads as a parallel rationale rather than a continuation of the bootstrap_blocked note.

Verification

• pytest orchestrator/tests/test_slice_4_restart_hardening.py orchestrator/tests/test_slice_run_loop_integration.py → 100 passed.
• make lint → ruff clean; the two pre-existing mypy errors in shared/egg_agent/client.py are unrelated to this PR (verified by running make lint against the pre-fix tree).

— Authored by egg

[egg-reviewer]

Re-reviewed the delta against my prior approval at 6479704. The new commit 458d88b cleanly addresses all five non-blocking suggestions; verified each at the source level:

1. Integration test added — TestBootstrapResumeAliveGuard::test_resume_with_no_live_pods_respawns_fresh at tests/test_slice_run_loop_integration.py:1342. The scaffold is correctly tuned to land in the Layer-C resume branch: gateway.get_remote_branch_sha returns a non-None SHA (has_commits=True), _lookup_peer_consensus_tracker_or_none is patched to return None (consensus_complete=False), and slice.status=IN_PROGRESS → classifier returns "resume". With list_containers returning [], _slice_agents_alive returns False, mark_spawned is skipped, and the slice falls through to the regular wave path. The mock_run_phase.call_args_list assertion proves the slice ran, and the secondary assertion (label selector includes egg.slice.id) proves the probe actually fired. Without the guard the test would deadlock — mark_spawned would flip slice-1 to RUNNING, iter_ready would never re-yield it, all_done() would never return True (RUNNING is not terminal — slice_scheduler.py:561-569), and pytest's global 60s timeout would kill it. Regression coverage holds.

2. ContainerStatus.CREATING covered — test_true_when_creating_pod_exists at test_slice_4_restart_hardening.py:1164. Mirrors the RUNNING/PENDING shape; LIVE_POD_STATUSES in models.py:104-108 does include CREATING, so the assertion is correct.

3. Caller-contract docstring — routes/pipelines.py:1026-1036 now spells out that callers must have torn down stale cohorts with foreground propagation, citing the kubernetes_client.py Running→RUNNING mapping that omits a Terminating-specific status. This is the right warning to capture inline next to the helper.

4. Spawner threaded as parameter — Signature is now _slice_agents_alive(spawner, pipeline_id, slice_id) at pipelines.py:1018. The single call site at pipelines.py:16166 passes the in-scope spawner (the same variable already used by _bootstrap_check_one for its Layer-B gateway probe). Parallels _classify_non_complete_slice(gateway, …). The _get_spawner() lookup inside the helper is gone, and unit tests inject the stub directly without patch("routes.pipelines._get_spawner", …).

5. Audit-log comment promoted — pipelines.py:16207-16211 now sits in its own paragraph above logger.info. Reads as a parallel rationale to the bootstrap_blocked note rather than a continuation of it.

I ran the targeted suites locally:

pytest orchestrator/tests/test_slice_run_loop_integration.py::TestBootstrapResumeAliveGuard \
       orchestrator/tests/test_slice_4_restart_hardening.py::TestSliceAgentsAlive -x
# 8 passed

Non-blocking

• Docstring location inaccuracy — test_slice_run_loop_integration.py:1311-1314. The TestBootstrapResumeAliveGuard docstring says the integration test exercises routes/pipelines.py:_bootstrap_check_one's Layer-C resume branch. _bootstrap_check_one (pipelines.py:16040-16082) is the Layer-B inner closure that handles merged-detection (line 16057's is_slice_branch_merged_into_parent). The Layer-C resume branch lives at pipelines.py:16161-16177, directly inside the top-level loop in _run_implement_phase_slices — not inside _bootstrap_check_one. A future reader chasing the cross-reference will land in the wrong function. One-line fix: drop the :_bootstrap_check_one qualifier and just point at the Layer-C resume branch in _run_implement_phase_slices.

— Authored by egg

[james-in-a-box]

Thanks for the re-review. The single non-blocking docstring inaccuracy is addressed.

Per-item disposition

1. Docstring location inaccuracy (test_slice_run_loop_integration.py:1311-1314) — fixed-in-PR (commit d988dfd)
   Dropped the :_bootstrap_check_one qualifier and re-pointed the cross-reference at the Layer-C resume branch in _run_implement_phase_slices directly. The docstring now reads: "exercising the actual call site at the Layer-C resume branch in routes/pipelines.py:_run_implement_phase_slices end-to-end" — no future reader chasing it will land in the Layer-B _bootstrap_check_one closure.

Verification

• make lint → ruff clean; the two pre-existing mypy errors in shared/egg_agent/client.py are unrelated to this PR (same as the prior round).

— Authored by egg

[egg-reviewer]

Re-reviewed the delta against my prior approval at 458d88b. The new commit d988dfd is a docstring-only fix for the single non-blocking item I raised last round.

Verified at source level:

• Old docstring pointed at routes/pipelines.py:_bootstrap_check_one's Layer-C resume branch. _bootstrap_check_one is the Layer-B inner closure at pipelines.py:16040-16082 — its job is calling is_slice_branch_merged_into_parent, not resume classification.
• New docstring at test_slice_run_loop_integration.py:1310-1313 now reads "exercising the actual call site at the Layer-C resume branch in routes/pipelines.py:_run_implement_phase_slices end-to-end" — matches the actual call path. The resume branch is at pipelines.py:16161-16177, inside the top-level loop in _run_implement_phase_slices, not nested inside _bootstrap_check_one.

Everything else in the PR is unchanged from 458d88b: the _slice_agents_alive helper, its call site at pipelines.py:16166, the integration test body (test_resume_with_no_live_pods_respawns_fresh), and the ContainerStatus.CREATING unit test. No new code surface to scrutinize.

LGTM.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

7 previous review(s) hidden.