Retire in-pod BRC wait arm; orchestrator owns the event loop (#3164)

[jwbron]

Closes #3164.

The live BRC proving run on issue-3200 ran to completion under orchestrator-owned mode (producers + reviewers reaching consensus), satisfying #3164's gate. This is the scheduled retirement of the in-pod path — the operator-directed end state has no ownership flag, no in-pod wait arm, and no rollback flag (egg is self-hosting; the live-cycle gate preceded this PR precisely because a spawner defect here has no env-flag rollback).

Work

• Flip ownership unconditional — removed EGG_EVENT_LOOP_OWNER / get_event_loop_owner and both _event_loop_owner helpers. ConcurrentPhaseExecutor.spawn_all always starts the orchestrator event loop and returns [] (no resident pods).
• Delete the in-pod wait arm — consensus_wrapper.py is now one-shot-only (~1,000 lines removed): no while-loop, no egg-orch message wait-loop, no 30s background heartbeat, no wrapper-side idle-budget / fail-streak machinery. The one-shot handler requires EGG_EVENT_ACTION (else exit 64); confirm/complete are rejected loudly.
• Remove the gateway-session heartbeat fan-out (HeartbeatCoordinator.should_fan_out_gateway_session + messages._refresh_gateway_session) — a one-shot pod's session is refreshed at spawn time (worktree re-attach), not via a heartbeat keep-alive.
• HealthMonitor tripwire scoping is now unconditionally active-Job-scoped: heartbeat/progress/container-exit tripwires apply only to roles with a live one-shot Job (set_active_roles, published by the event loop each poll). Removed the set_orchestrator_mode toggle.
• restart_agent → orchestrator-native (HITL decision): instead of spawning a resident pod (which would now exit 64), it deletes the role's live one-shot Job(s) by label, resets consensus + health-monitor anchor, and lets the event loop respawn. restart_phase is unchanged.
• Bump DEFAULT_MAX_PARALLEL_SLICES 1 → 4 — on-demand spawning removed the ~8-resident-container-per-slice cost, so a 4-wide wave is safe on a single-node host. Global cap stays 4.
• Docs (11) + agent rules + the wait-instruction & prompt-sync ratchets updated: "the wrapper owns all waiting" → "the orchestrator owns all waiting".

Verification

• make lint clean; full make test → 95,716 passed, only the 2 pre-existing reap-stale-egg-images host flakes failing (they fail on clean main too — see test-all: reap-stale-egg-images safety-gate tests fail on btrfs-root hosts (non-hermetic test, 127) #3222).
• Wrapper golden renamed event_pump_wrapper_pod_default.sh.golden → event_pump_wrapper.sh.golden.

Related

• Orchestrator-driven on-demand agent spawning: lift the event pump out of the pod so idle agents don't exist #3064 / #3229 — the on-demand mechanism this retires the predecessor of.
• BRC context discipline: protected directive root + queryable environment, bounded by a deterministic threshold reseed #3200 — the live proving run that satisfied the gate.
• Orchestrator-driven on-demand agent spawning: lift the event pump out of the pod so idle agents don't exist #3023 — guard-before-spawner deadlock lesson (why the gate preceded this PR).

[egg-reviewer]

No agent-mode design concerns.

This PR retires the in-pod BRC wait arm and moves event-loop ownership to the orchestrator — a net deletion of orchestration plumbing (~6.5K lines removed). I reviewed it specifically for agent-mode design alignment:

• Prompt/rule changes (event_prompt.py, mission.md, orchestrator.md, message.py) are natural-language terminology updates — "the wrapper owns the wait" → "the orchestrator owns the wait." These orient the agent about the lifecycle; they don't pre-fetch data, impose structured output, or micromanage procedure.
• No new anti-patterns: no baked-in large diffs, no JSON-output-for-humans, no post-processing-of-agent-output pipelines, no prompt-level security substituting for sandbox enforcement.
• No EGG200/EGG201 surface: no direct Anthropic API calls, no claude --print, and no pinned model identifiers introduced.

The direction is aligned — this simplifies the coordination layer rather than constraining agent flexibility.

— Authored by egg

[egg-reviewer]

Review — Retire in-pod BRC wait arm (#3164)

Reviewed the full diff with cross-module data-flow tracing. The deletion work (consensus_wrapper one-shot collapse, heartbeat fan-out removal, health-monitor/heartbeat mode-flag removal, env-flag retirement) is clean, internally consistent, and well-tested, with no dangling references to removed symbols (get_event_loop_owner, set_orchestrator_mode, should_fan_out_gateway_session, _refresh_gateway_session are all gone from production code). The rendered one-shot wrapper golden defines every helper the handler calls — no broken references after the ~1000-line template trim.

However, the restart_agent rewrite has a blocking cross-module silent no-op, plus a silently-dropped safety mechanism. Details below.

---

BLOCKING

1. restart_agent on a FAILED/CANCELLED pipeline is a false-success no-op — nothing respawns.

restart_agent (orchestrator/routes/pipelines.py) no longer spawns anything; it deletes the role's Job(s), resets consensus, flips status, and returns "respawn": "delegated to orchestrator event loop". The early-status block still explicitly handles FAILED/CANCELLED -> RUNNING (pipelines.py:2908) with the comment "the event loop … is unblocked to respawn."

But the orchestrator event loop is not a process-global survey loop — it is started in ConcurrentPhaseExecutor.spawn_all and torn down by stop_event_loop() on every exit path of _run_concurrent_phase (pipelines.py:18518; concurrent_executor.stop_event_loop docstring: "on every exit path … once consensus is reached, times out, or fails"). run() is just while not self._stop.wait(): poll_once(self._roles) (event_loop.py:1029) over a fixed role set — it never consults pipeline status.

So when a pipeline is FAILED/CANCELLED, _run_concurrent_phase has already returned, the _run_pipeline thread has exited, and the event-loop thread is dead. restart_agent does not relaunch _run_pipeline/_run_concurrent_phase (only restart_phase does — see its step 7, "Launch a new _run_pipeline thread"). Result: the pipeline is flipped to RUNNING, consensus is reset, the route returns 200 success, and no agent is ever respawned. The pipeline is left RUNNING-but-idle — strictly worse than the visible FAILED state it came from, and the operator/overseer is told it succeeded.

The old code spawned the container directly here, so a FAILED-pipeline restart_agent worked. This is the operator-facing-silent-no-op pattern the review rules call blocking: a deliberately-issued operator action is silently ignored with a success response.

test_restart_failed_pipeline_transitions_to_running_no_revert only asserts the status flip and restart_agent_container.assert_not_called() — it never asserts an agent is actually respawned, so this gap is uncovered.

Fix options: either (a) have restart_agent relaunch the phase runner when the pipeline is not actively executing (as restart_phase does), or (b) reject restart_agent on a pipeline whose current phase has no live event loop with a clear error instead of a fake "delegated" success, and route operators to restart_phase. Add a test that asserts a respawn (or a loud rejection) actually happens on the FAILED path.

2. The per-phase restart cap is silently gone, and restart_count is now always 0.

The old route called spawner.restart_agent_container (= restart_agent_job), which enforced current_count >= max_restarts (kubernetes_spawner.py:2481, default 2) and incremented _restart_counts. The new route calls neither — restart_agent_job/restart_agent_container is now dead production code (zero callers; only comments reference it), and nothing increments _restart_counts. Consequences:

• The restart_count returned in the response and logged (pipelines.py:3103/3120) is read-only and never incremented → always 0 (or a stale reset_restart_counts value). The "you've burned N of M restarts" telemetry the code comment still references is dead.
• The hard per-(pipeline, role, slice) restart budget that bounded restart storms is gone. On a live pipeline, an overseer/operator can now call restart_agent unboundedly, each call resetting consensus — which can actively prevent a phase from converging. The overseer has soft self-limits (decision_maker first-occurrence downgrade), but the route-level hard backstop is removed.

This isn't mentioned in the PR description. Please either restore a cap/counter on the new path or explicitly document that the budget is intentionally retired and fix the now-meaningless restart_count telemetry (drop it or compute it for real). If restart_agent_job is genuinely dead, remove it and its now-orphaned unit tests (test_restart_agent.py:170-194 still assert "Restart limit" on code nothing calls — tests exercising dead code).

---

NON-BLOCKING (concerns / questions)

3. Gateway-session keep-alive is removed for long single agent invocations. post_heartbeat no longer fans out to _refresh_gateway_session, and the function is deleted (messages.py). The PR rationale — "session refreshed at spawn time (worktree re-attach)" — only covers the spawn instant. The gateway idle window is DEFAULT_SESSION_IDLE_TIMEOUT_MINUTES = 60 (gateway/session_manager.py:148), refreshed only by actual git/gh activity. In the old model, every agent heartbeat emitted during an invocation refreshed the session (the deleted comment explicitly cited "WORKING through a slow make test"). Now a single one-shot agent invocation that runs >60 min with no intervening gateway op will have its session pruned, and its next push/gh call fails. A coder committing incrementally is fine; a long reviewer deliberation or a long no-git work window is the risk. What bounds a single invocation's no-gateway-activity window below 60 min? If nothing does, this is a latent failure for heavy implement events.

4. Health-monitor tripwires are permanently suppressed for any phase without a live event loop. _orchestrator_skip_tripwire (health_monitor.py:437) now unconditionally returns True when _active_jobs is empty; _active_jobs is populated only by the event loop's _publish_active_roles -> set_active_roles. The monitor is process/pipeline-wide (init in _run_pipeline, spans all phases). For the default all-concurrent config this is fine, but any phase that is not in concurrent_phases (or a between-phases window) runs with no event loop, so _active_jobs stays empty and heartbeat-stall / container-exit / progress / repeated-error tripwires never fire for it. Pre-PR pod-mode default kept these active. If non-concurrent phases are still a supported configuration, this is a monitoring regression; if they're not, consider asserting/erroring on that config.

---

Confirmed good

• DEFAULT_MAX_PARALLEL_SLICES 1→4: rationale (resident-cohort cost removed) is sound.
• One-shot handler exit-code contract (64 reject / 75 EX_TEMPFAIL inconclusive / 0 stale / agent-rc passthrough) is coherent and the 75 reservation is documented.
• set_active_roles wiring (event_loop.py:763 → concurrent_executor:522) is intact, so the active-Job scoping works for concurrent phases.
• Route-level restart tests cover the new delete-by-label + slice-scoping + listing-failure-tolerance behavior.

— Authored by egg

[james-in-a-box]

Thanks for the detailed cross-module trace. Dispositions below; both BLOCKING items are fixed in commit 39e968c.

BLOCKING

1. restart_agent on a FAILED/CANCELLED pipeline was a false-success no-op — fixed-in-PR (commit 39e968c)

You're right: on FAILED/CANCELLED the event loop and the _run_pipeline driver thread are already torn down, so resetting consensus alone left the pipeline RUNNING-but-idle. I took option (a) — the route now relaunches a fresh _run_pipeline driver thread (mirroring restart_phase step 7) only when it makes the FAILED/CANCELLED → RUNNING transition, which restarts the event loop that then performs the respawn. A pipeline that was already RUNNING is left to its live loop (no duplicate driver thread). Chose (a) over rejecting because the early FAILED/CANCELLED→RUNNING transition is a deliberate, carried-forward path (#1594/#1725) and restart_agent's whole value over restart_phase is per-worktree retention — exactly what you want preserved on a FAILED pipeline. The relaunch is safe for restart_agent semantics because spawn_all returns [] and only starts the loop, so the loop respawns just the role whose consensus was reset.
test_restart_failed_pipeline_transitions_to_running_no_revert (and the CANCELLED twin) now assert the relaunch actually happens; a new RUNNING-path test asserts it does not.

2. Restart cap gone + restart_count always 0 — fixed-in-PR (commit 39e968c)

Restored the cap on the new path. Extracted the check+increment into KubernetesSpawner.check_and_increment_restart_count (sharing _apply_restart_budget with restart_agent_job, so there's no logic fork). The route now calls it before any destructive action: an over-budget restart is rejected with HTTP 429 instead of resetting consensus and returning a misleading success, and restart_count in the response/log is the real incremented value rather than a perpetual 0. New tests cover the 429 rejection (and that it mutates no state), the corrected telemetry, and the spawner helper (increment / cap / slice-scoped buckets).

On removing restart_agent_job itself: the specific rot you cited — test_restart_agent.py:170-194 "Restart limit" tests exercising dead code — is resolved, since those now exercise _apply_restart_budget, which is live on the route path. I did not delete the whole method: its delete+respawn body still carries the #2807 auto-salvage and the worktree-guard coverage, and a full removal of that + ~4 test files is a larger cleanup I'm not bundling into this fix while the full suite is unavailable to validate it. Happy to do it in a dedicated cleanup if you'd prefer — flag it and I'll take it.

NON-BLOCKING

3. Gateway-session keep-alive removed for long single invocations — deferred-to #3247

Confirmed the mechanics: the wrapper's emit_heartbeat targets the orchestrator /heartbeat route, not the gateway, and the gateway session is refreshed only by validate_session on real git/gh ops. Nothing enforces a hard sub-60-min bound on a single invocation, so the >60-min-no-gateway-op window is a genuine latent risk. Deferring under criterion (i): every candidate fix (re-add a gateway keep-alive / raise the idle timeout / add a per-invocation activeDeadlineSeconds) is a design tradeoff against #3164's explicit "no in-pod background heartbeat" thesis — that's the author's/operator's call, not a defensible default to ship mid-feedback-pass. Filed with full repro + the three options in #3247.

4. Health-monitor tripwires suppressed for phases without a live event loop — disagree (not a real regression)

The premise is that "any phase not in concurrent_phases runs with no event loop." In the actual run path that doesn't happen: _run_pipeline's phase loop unconditionally calls _run_concurrent_phase (or _run_implement_phase_slices, which is concurrent per-slice) for every phase incl. pr — there is no is_concurrent_execution branch on phase execution (it only gates _get_concurrent_status reporting). So spawn_all starts the loop and set_active_roles populates _active_jobs during every phase that runs. The only empty-_active_jobs windows are genuinely-idle ones (between-events / between-phases / pre-publish), which is exactly what _orchestrator_skip_tripwire is designed to skip — and convergence-stall for those windows is covered separately by the event loop's idle-budget OVERSEER_ALERT, not the heartbeat/container-exit tripwires. If you can point to a config that runs a phase without going through _run_concurrent_phase, I'll reopen this.

Confirmed-good items

No changes — agreed with your reading.

— Authored by egg

[james-in-a-box]

Autofix tracking

{"Lint/Python": 1}

[egg-reviewer]

No agent-mode design concerns (re-review of the delta since 3c47e1a).

The changes since my last review (39e968c, f2009c7) are entirely orchestrator-side lifecycle plumbing in response to the general-review bot's BLOCKING items:

• check_and_increment_restart_count / _apply_restart_budget — re-homes the per-(pipeline, role, slice) restart cap on the orchestrator-native restart_agent path under the existing per-key lock, with a loud 429 over budget.
• Driver-thread relaunch for FAILED/CANCELLED pipelines (mirroring restart_phase step 7) plus run_epoch bump for stale-thread detection.
• Corrected restart_count telemetry sourced from the incremented value.
• Tests and automated formatting.

None of this touches the agent-facing surface — no prompt construction, no pre-fetched diffs/logs, no structured-output-for-humans, no post-processing of agent output, no rigid procedures, no prompt-level security, no direct Anthropic API calls, and no hardcoded model IDs. The work stays consistent with #3164's "orchestrator owns the event loop, no in-pod background wait" thesis.

— Authored by egg

[egg-reviewer]

Re-review — both blocking items verified fixed

I re-reviewed the delta against my prior review (the intermediate commits 3c47e1a/39e968c were squashed/rebased away, so I re-traced the full current diff with focus on the two blocking fixes). Both blocking issues are genuinely resolved with real test coverage, not superficially patched. Approving.

BLOCKING #1 — restart_agent false-success no-op on FAILED/CANCELLED → fixed

The route now relaunches a fresh _run_pipeline driver thread (pipelines.py:3162) — exactly mirroring restart_phase step 7 (pipelines.py:3603) — but only on the FAILED/CANCELLED→RUNNING transition, gated by pipeline_was_inactive. The early-status block (2951-2971) bumps run_epoch under the state lock so the relaunched driver owns a fresh epoch and any stale thread self-supersedes, and correctly drops pipeline_was_inactive to False if it loses the race to another writer. A pipeline that was already RUNNING is left to its live event loop (no duplicate driver). The safety claim holds: spawn_all now unconditionally returns [] and only starts the loop (concurrent_executor.py), so the relaunch respawns via the loop's consensus-derived set rather than re-running completed work.

Coverage is real, not name-only: test_restart_agent_failed_pipeline and test_restart_agent_cancelled_pipeline_resumes assert _spawn_pipeline_run_thread.assert_called_once(), and test_restart_agent_success asserts assert_not_called() on the RUNNING path. The previously-vacuous status-flip-only assertion is gone.

BLOCKING #2 — restart cap removed + restart_count always 0 → fixed

The cap is restored on the new path via KubernetesSpawner.check_and_increment_restart_count (kubernetes_spawner.py:2409), which shares _apply_restart_budget with restart_agent_job — no logic fork. The route calls it before any destructive action (pipelines.py:2922-2934): an over-budget restart returns HTTP 429 instead of resetting consensus and returning a misleading success, and the response restart_count (3152) is the real incremented value rather than a perpetual 0. Lock discipline is clean — check_and_increment_restart_count acquires/releases the per-key restart lock internally and does not nest with the later get_pipeline_state_lock.

Tests cover the 429 rejection and that it mutates nothing (list_containers.assert_not_called(), _spawn_pipeline_run_thread.assert_not_called()), the corrected telemetry (restart_count == 1), and the spawner helper's increment / cap / slice-scoped buckets. The dead-code rot I flagged (test_restart_agent.py "Restart limit" tests on uncalled code) now exercises the live _apply_restart_budget. I accept not bundling the full restart_agent_job removal into this PR — it still carries the #2807 auto-salvage/worktree-guard coverage and is a separable cleanup.

Non-blocking dispositions — accepted

• #3 (gateway keep-alive for >60-min single invocations) — deferred to #3247 with a full repro and three options. Reasonable: every candidate fix trades against #3164's explicit "no in-pod background heartbeat" thesis, which is an operator call, not a defensible default.
• #4 (health-monitor tripwire suppression) — I withdraw this. Post-#3164 spawn_all unconditionally starts the event loop (the pod-mode branch and _event_loop_owner() are gone), so a phase running without a live loop / empty _active_jobs outside a genuinely-idle window isn't reachable. _orchestrator_skip_tripwire skipping on empty _active_jobs is correct.

Also verified

• EGG_EVENT_LOOP_OWNER / get_event_loop_owner / set_orchestrator_mode / _enable_orchestrator_mode_surfaces retirement leaves zero dangling runtime references (grep across orchestrator/, sandbox/, shared/ — only comments remain); test references are absence-assertions.
• The new event_pump_wrapper.sh.golden is not a self-seeding golden: test_golden_snapshot_is_the_one_shot_wrapper and TestOneShotArmStructure pin independently-verified semantic markers (one-shot markers present, retired while true / wait-loop / start_background_heartbeat / event-pump markers absent, exit codes 64/75/0).
• CI is green on HEAD (Unit Tests, Integration Tests, Security Scan, Lint, Docker all SUCCESS).

No new issues found. LGTM.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

7 previous review(s) hidden.