Re-drive resume-classified slices at bootstrap instead of parking them (#3685) by jwbron · Pull Request #3688 · jwbron/egg

jwbron · 2026-07-28T04:49:25Z

Root cause

The issue reads as "the restart restores the driver and the tracker but forgets to start the event loop". The gap is one level up: Layer-C case 2 parks the slice somewhere nothing can reach it.

_run_implement_phase_slices' bootstrap classified issue-3665-v3/slice-1 as resume (IN_PROGRESS, commits on the integration branch, consensus not reached) and answered that with scheduler.mark_spawned. Three facts make that a terminal state, not a resume:

Post-Flip event-loop ownership to the orchestrator and retire the in-pod wait arm (gated on a live BRC proving run of #3229) #3164 the orchestrator-owned BRC event loop is the only thing that dispatches work for a slice, it is process-local, and ConcurrentPhaseExecutor.spawn_all is the only thing that starts one.
_run_one_slice is the only caller of scheduler.record_complete / record_failure for a running slice.
iter_ready never re-yields a RUNNING slice, and RUNNING is not terminal for all_done().

So mark_spawned at bootstrap left the slice with no dispatcher and no completer, holding a max_parallel_slices slot (default 1, so the whole DAG stalls behind it), in a loop whose exit condition can never be met. The driver spins on time.sleep(poll_interval) forever while get_status says running, the driver thread exists, the per-slice tracker is correctly reconstructed, and the bootstrap logs resumed=['slice-1']. That is the "every liveness signal reads healthy" property from the issue, and it is why no detector fired: there was no alert to suppress.

The #2914 live-pod guard did not fix this; it narrowed the wedge to "a pod happened to be alive at bootstrap". Post-#3164 a live pod is a one-shot event Job that exits within minutes, which is exactly what the incident shows (live_pod_count=1 at 03:27, zero pods 24 minutes later). It also explains the restart_agent symptom: with no loop for the slice, the respawn it delegates can never occur.

Fix

Case 2 takes no scheduler action. The slice re-yields READY, the run loop admits it, and spawn_all starts its event loop. Re-driving is not a heavyweight respawn: post-#3164 spawn_all spawns no agents up front, it starts the loop, and the loop's one-shot Jobs pick up the commits already on the slice's integration branch (which resumes in place via integration_base_sha, #2947). Re-driving is what "resume" means under orchestrator ownership.

_reap_orphaned_slice_jobs replaces _slice_agents_alive. Before the re-drive, any Job still live for the slice is force-removed. Its loop is gone, so nothing will observe its termination or derive its next event; left in place it holds the role's worktree while the fresh cohort's Job attaches to the same checkout (#3337), and the spawner's live-key adoption cannot collapse that duplicate because the orphan's dedupe key was derived against a tracker the new process does not have. Per-agent worktrees are deliberately not deleted; the re-driven slice wants them warm.

The audit line reports redriven= in place of the resumed= / reclassified_fresh= pair. The distinction those drew (mark-spawned vs re-yielded) no longer exists, and the new field is falsifiable: a slice id under redriven that never appears in a subsequent spawn is a real anomaly, whereas resumed was indistinguishable from a wedge.

On the issue's three suggestions

(1) Start the loop in the reconciliation step is necessary but not sufficient. A mark_spawned slice with a live loop would reach consensus and then sit there: no _run_one_slice worker is polling it, opening its slice PR, or calling record_complete. Re-driving delivers the loop and the completer.
(2) Classify it as blocked would surface it, but blocked means HITL-pending and pauses the pipeline for an operator. This state is unambiguously recoverable without a human, so it recovers. The honesty fix lands on the log field instead.
(3) A deterministic detector ("RUNNING + non-COMPLETE slice + no live loop + no live pods") is still worth having as defence in depth for other paths, and is left to the Supervision, second pass: the layer was silent on seven livelocks and loud at healthy agents #3665 detection plane as the issue suggests.

Known cost

The per-slice tracker reconstructed at startup (#2409) is superseded by the fresh one spawn_all registers, so a re-driven slice replays a BRC round against commits already on its branch. Preserving it would mean adopting a tracker built on the unfiltered phase graph into an executor that deliberately runs a role-filtered one, which is the same "wait forever on an agent that will not spawn" failure class this PR is fixing. Documented in docs/architecture/orchestrator.md; worth its own issue if the replayed round proves expensive in practice.

Testing

TestBootstrapResumeRedrive (new, test_slice_run_loop_integration.py) drives the real bootstrap + run loop over a resume-classified slice from both sides of the old guard: with and without a surviving pod, _run_concurrent_phase must be reached, and the live-pod case pins the reap and its slice-scoped label selector. Verified these fail against the pre-fix branch, and the failure mode is the bug itself: the run loop hangs in time.sleep(poll_interval) until pytest-timeout kills it at 60 s, because all_done() can never turn true.
TestReapOrphanedSliceJobs (new, test_slice_phase_restart_hardening.py, replaces TestSliceAgentsAlive) covers live/terminal/empty pod sets, both failure paths (label query, removal) being swallowed, and the label selector.
make lint clean. 315 tests green across the slice run loop, restart hardening, cancel-driver, slice-closed, scheduler, restart_phase and restart_agent suites. The full narrowed make test run is in flight; I will report it on this PR.

Layer-C bootstrap reconciliation answered case 2 (IN_PROGRESS + commits + consensus not reached) with scheduler.mark_spawned, on the reasoning that the surviving agent pods carried the slice forward. Post-#3164 nothing does. The orchestrator-owned BRC event loop is the only dispatcher for a slice, it is process-local, ConcurrentPhaseExecutor.spawn_all is the only thing that starts one, and _run_one_slice is the only caller of scheduler.record_complete for a running slice. So mark_spawned at bootstrap parked the slice in scheduler-RUNNING with no dispatcher and no completer: iter_ready never re-yields a RUNNING slice, it holds a max_parallel_slices slot, and all_done() can never turn true. The pipeline reports status: running with zero pods indefinitely while every liveness signal reads healthy (driver thread relaunched, per-slice tracker reconstructed, slice logged as resumed). The #2914 live-pod guard only narrowed the wedge to "a pod was still alive at bootstrap", which post-#3164 means a one-shot event Job that exits within minutes. It also left restart_agent a no-op for the slice, since the respawn it delegates to the event loop can never happen. Case 2 now takes no scheduler action, so the slice re-yields READY and the run loop re-drives it. That is what "resume" already means under orchestrator ownership: spawn_all spawns no agents up front, it starts the event loop, and the loop's one-shot Jobs pick up the commits already on the slice's integration branch (which resumes in place via integration_base_sha, #2947). _reap_orphaned_slice_jobs replaces _slice_agents_alive: before the re-drive it force-removes any Job still live for the slice. That Job's loop is gone, so nothing will observe its termination or derive its next event, and left in place it holds the role's worktree while the fresh cohort's Job attaches to the same checkout (#3337) -- the spawner's live-key adoption cannot collapse the duplicate, because the orphan's dedupe key was derived against a tracker the new process does not have. Per-agent worktrees are deliberately not deleted; the re-driven slice wants them warm. The bootstrap audit line reports redriven= in place of the resumed= / reclassified_fresh= pair. A slice id under redriven that never appears in a subsequent spawn is a real anomaly, whereas resumed was indistinguishable from a wedge. Known cost, called out in the docs: the per-slice tracker reconstructed at startup (#2409) is superseded by the fresh one spawn_all registers, so the slice replays a BRC round against commits already on its branch. Fixes #3685

egg-reviewer

No agent-mode design concerns.

This PR is confined to orchestrator-side slice lifecycle: Layer-C bootstrap case 2 stops calling scheduler.mark_spawned and instead reaps orphaned Jobs and lets the slice re-yield READY, plus the corresponding docs and test updates. Checked against each anti-pattern in docs/guides/agent-mode-design.md:

Pre-fetching / prompt assembly — untouched. The only prompt-adjacent line in the diff is a comment in _slice_state.py noting that a probe-failure misclassification now costs "only the prompt content of the re-driven cohort". The classification value is consumed solely by the scheduler branches at orchestrator/routes/pipelines/_run_implement.py:407-437; it never reaches prompt construction, so there is no context being baked in or withheld either way.
Structured output / post-processing — none added. _reap_orphaned_slice_jobs returns a list[str] of container ids consumed in-process for a log line, not agent output being re-parsed.
Direct LLM API calls / Agent SDK bypass (EGG200, item 7) — no httpx, requests, or Anthropic SDK usage introduced; agent spawning still goes through the existing spawn_all / event-loop path.
Hardcoded model identifiers (EGG201) — no model strings in the diff.
Prompt-level security — the change strengthens infrastructure enforcement rather than weakening it: remove_agent_container(force=True) is a real teardown, and the two-live-pods race (#3337) is closed at the k8s layer, not by telling an agent not to touch a worktree.

One observation, non-blocking and explicitly not a finding: the known cost you documented — the reconstructed per-slice tracker being superseded so the slice replays a BRC round — means re-driven agents re-derive their state from the integration branch rather than from carried-over orchestrator state. That is the agent-mode-aligned direction (the agent reads its own ground truth from git instead of trusting injected state), so the trade-off reads correctly from this lens even though it costs a round.

— Authored by egg

egg-reviewer

Review: #3688 — Layer-C case-2 re-drive + orphaned-Job reap

The root-cause analysis is correct and I verified it independently. Post-#3164 the run loop at _run_implement.py:1389 is the only surviving production scheduler.mark_spawned caller, iter_ready() yields only READY, and _run_one_slice is the only completer for a running slice — so the old bootstrap mark_spawned genuinely parked case-2 slices unreachably. Deleting it is right. _run_one_slice_inner's integration_base_sha threading confirms the re-drive resumes in place rather than losing branch work, and create_peer_consensus_tracker (peer_consensus/__init__.py:280-299) unconditionally overwrites _trackers[key], which confirms the "one replayed BRC round" cost the PR documents.

The new reap is where the problems are. Two blocking, then advisories.

BLOCKING 1 — `"fresh"` takes the same restart with the same orphan and does not reap

_run_implement.py:424-437 reaps on classification == "resume". The "fresh" branch is a bare comment with no reap:

# "fresh" → no Layer-C action, scheduler re-yields READY.

Both branches then do the identical thing: leave the slice READY so the run loop admits it and spawns a fresh cohort. The only thing that separates them is _classify_non_complete_slice's probe of refs/heads/{issue_branch}/{slice_id} — whether commits reached the integration branch on origin. That signal is orthogonal to whether an agent pod is still live.

Two concrete paths into "fresh" with a live orphan:

Orchestrator pod recycles while slice-1's coder Job is Running and has produced commits in its worktree (or on its per-agent branch) but has not yet pushed to the integration branch. get_remote_branch_sha → None → has_commits = False → "fresh" (_slice_state.py:699-704). No reap.
The gateway probe raises transiently. _slice_state.py:672-697 swallows it and sets has_commits = False → "fresh". No reap.

In both cases the run loop admits slice-1, spawn_all starts a fresh event loop, and its first coder Job calls _try_reuse_worktree on worktree id {pipeline_id}-slice-1-coder (kubernetes_spawner/__init__.py:325-341 — the id is keyed by (pipeline, slice, role), not by Job), then _clean_reused_worktree runs git reset --hard && git clean -fd in a checkout the still-running orphan coder is actively writing. That is precisely the #3337 two-live-pods race this PR's own helper docstring says it exists to prevent.

Nothing else covers it. _reap_superseded_siblings (event_loop/_loop.py:784) explicitly documents that it cannot:

Restart-boundary limitation: this matches superseded siblings via the in-memory _key_meta role label, which reconcile() does not seed for keys adopted after an orchestrator restart. So a stale same-role key adopted across a restart […] is unlabeled and won't be reaped here. That re-opens the #3337 two-live-pods window […]

_reap_orphaned_slice_jobs is the fix for exactly that documented gap. Wiring it to only one of the two branches that lead to the same respawn leaves the gap open on the other.

The PR also makes this worse than pre-existing, and its own docs say so. _slice_state.py:674-678 now reads:

# Both the "fresh" and "resume" outcomes re-yield READY today (#3685),
# so the cost of guessing wrong here is only the prompt content of the
# re-driven cohort, not whether the slice runs at all.

After this PR that is no longer true. Guessing "fresh" when the state is really "resume" now also skips the reap. The comment is asserting an equivalence the same commit removes.

Fix: hoist the reap above the classification switch, or call it on the "fresh" branch too. Every branch that leaves the slice admissible must reap first; the classification should decide the prompt, not whether orphan teardown happens.

BLOCKING 2 — the reap is fire-and-forget against an asynchronous delete, so the window is still open when the slice is admitted

_pod_liveness.py:140-142 claims:

Reaping is foreground (force=True) so the pod is gone rather than merely terminating before the run loop admits the slice, matching restart_phase step 4's teardown contract.

force=True does not give that. KubernetesClient.remove_container (kubernetes_client.py:433-452) disclaims it in its own docstring:

This orders pod teardown ahead of the owner, but the GC deletes the pods asynchronously — they are not guaranteed gone by the time the call returns.

So _reap_orphaned_slice_jobs returns while the orphan pod is still Running inside its termination grace period with the worktree mounted, and the very next statements make the slice admissible. The fresh cohort's Job then attaches to the same checkout and git reset --hard && git clean -fds it.

The codebase already solved this, twice, for the same reason:

_routes_restart.py:428-440 (#3597): "the delete above is ASYNCHRONOUS — it returns as soon as the API server accepts it, and the Job then lingers in Terminating […] we also wait for the teardown we requested to be OBSERVED before returning, so the respawn we are delegating starts from a clean slate." Bounded by _JOB_TEARDOWN_WAIT_SECONDS = 20.0 via k8s.wait_for_job_gone.
kubernetes_spawner/_events.py:113-215: spawn_event_job's pre-spawn wait on terminating Jobs, same wait_for_job_gone, bounded by _EVENT_JOB_TERMINATION_WAIT_S.

Neither covers this reap. The spawn-side wait selects on LABEL_EVENT_DEDUPE={dedupe_key} (_events.py:34) — dedupe-key-scoped only. The orphan's key differs from the fresh cohort's first event, which is the PR's own premise for reaping at all. So by the PR's own argument the spawn-side wait provably will not see the orphan.

Note the tension: the two premises can't both be convenient. Either the keys match — in which case spawner adoption and the pre-spawn wait already handled it and the reap is destructive (see advisory 3) — or they don't, in which case the reap is needed and its async gap is entirely uncovered.

Fix: wait for observed teardown before returning, bounded, exactly as _routes_restart.py:440-500 does. list_containers already populates job_name (kubernetes_client.py:588-594), so you have the addressable handle:

reaped_jobs.append(pod.job_name)
...
deadline = _pkg.time.monotonic() + _REAP_TEARDOWN_WAIT_SECONDS
for name in reaped_jobs:
    remaining = deadline - _pkg.time.monotonic()
    if remaining <= 0:
        # log unconfirmed, don't claim an observation you didn't make
        break
    spawner.k8s.wait_for_job_gone(name, spawner._namespace, timeout_s=remaining)

A timeout should be logged as unconfirmed rather than swallowed — an operator needs to see that the slice was admitted into an unclosed window.

Advisory (PLAUSIBLE) — the stated justification for reaping contradicts `compute_dedupe_key`'s documented contract

_pod_liveness.py:136-138 and the architecture doc both assert:

the spawner's live-key adoption cannot collapse the duplicate because the orphan's dedupe key was derived against a tracker this process does not have.

compute_dedupe_key (event_loop/__init__.py:280-308) documents the opposite:

Deterministic across orchestrator restarts: identical inputs always yield the identical key, which is what makes live-Job reconciliation able to recognise an in-flight event after a restart.

And the new process does have a tracker for the slice — startup_reconciliation reconstructs it (#2409) and _classify_non_complete_slice:717 consults it via _lookup_peer_consensus_tracker_or_none on the very code path making this claim. What is actually true is narrower: spawn_all registers a fresh tracker that supersedes the reconstructed one, so the identity the new loop derives comes from a zeroed tracker — and for a first WORKING propose event_identity collapses to "v|", which can match the orphan's key. Where it matches, _event_dedupe_key_live adoption would have collapsed the duplicate correctly and the reap instead kills a Job that was making progress, forcing a replayed round.

I'm not asking you to drop the reap — uncertainty about the orphan's session and tracker state justifies it. I'm asking you to state the real reason. As written, the next maintainer reading _pod_liveness.py or docs/architecture/orchestrator.md:137 will believe cross-restart key matching is impossible, which is not what compute_dedupe_key promises.

Advisory — the `restart_phase` step-4 analogy fails at the points this depends on

restart_phase step 4 (_routes_restart.py:1097-1115) is stop_agent_container(cleanup_session=True) → remove_agent_container(force=True, cleanup_session=False) → step 4b deletes the per-agent worktrees (with salvage). The reap here is remove_agent_container(force=True, cleanup_session=True) alone, and deliberately leaves the worktrees. That difference is not incidental — worktree deletion is exactly why step 4's missing teardown-wait is harmless there and load-bearing here. Please drop or qualify the "matching restart_phase step 4's teardown contract" sentence.

Advisory — `cleanup_session=True` receives a Pod UID and silently no-ops

pod.container_id is pod.metadata.uid (kubernetes_client.py:597). Passing it to remove_agent_container is fine for deletion — _resolve_job_name (kubernetes_client.py:1109) explicitly handles the Pod-UID path — but remove_agent_job forwards the same value to gateway.delete_session_by_container(job_name) (kubernetes_spawner/_jobs.py:116). Event-mode sessions are keyed by the stable base container_id, not the per-event Job name (see cleanup_pipeline's note in the same file), so this is a no-op that a GatewayError swallow makes invisible. Either pass cleanup_session=False and say why, or use pod.job_name for consistency with every other call site.

Advisory — duplicates `list_slice_jobs`

_pod_liveness.py:157-162 hand-rolls the query that KubernetesSpawner.list_slice_jobs(pipeline_id, slice_id) (kubernetes_spawner/_jobs.py:143-157) already performs with the identical label pair. Reusing it would also drop the spawner.backend reach-through.

Advisory — terminating Jobs are indistinguishable from live ones here

The filter is pod.status not in _LIVE_POD_STATUSES only. _job_is_live (_events.py:49-107) additionally excludes terminating Jobs because of #3597, and ContainerInfo.deletion_timestamp exists for exactly that (models/_execution.py:55) — but KubernetesClient.list_containers never populates it, and a Terminating pod still reports phase Running. Consequence is bounded (a redundant delete, plus an inflated reaped= audit list), but worth a comment so the asymmetry with _job_is_live isn't read as an oversight.

Advisory — test gaps

TestReapOrphanedSliceJobs and TestBootstrapResumeRedrive are well-constructed; the shared _run(spawner) helper and the slice-scoped-label assertion are the right things to pin. Missing:

A case-1 ("fresh") slice with a live orphan Job — i.e. get_remote_branch_sha returning None while list_containers returns a RUNNING pod. That test would fail today and is the regression guard for BLOCKING 1.
A redriven= audit-field assertion. The PR argues operators should read redriven as an anomaly signal, but nothing pins its contents.
Teardown-wait coverage once BLOCKING 2 is addressed.

Verified and not an issue

Nothing dangles from the _slice_agents_alive → _reap_orphaned_slice_jobs rename; the barrel re-export at routes/pipelines/__init__.py:1287 is the only other reference.
The removed resumed= / reclassified_fresh= log fields have no consumers outside .egg-state/brc-history transcripts. Dropping them is safe.
Passing a Pod UID to remove_agent_container is supported (_resolve_job_name handles Job name, Job UID, and Pod UID). My initial read of this as a wrong-identifier bug was wrong.
The "worktrees stay warm" claim holds: _clean_reused_worktree preserves dirty trees (#3639) and salvages discarded tips to egg/recovered/* (#3509) before its reset.

— Authored by egg

…ardown Addresses the two blocking findings and four advisories on PR #3688. The reap was wired into the "resume" arm of the Layer-C classification switch. Cases 1 ("fresh") and 2 ("resume") both leave the slice READY and therefore admissible, and the only thing separating them is whether commits reached the integration branch on origin — orthogonal to whether an agent Job is still live. An orchestrator recycle before the slice's first push, or a transient gateway probe failure (which _classify_non_complete_slice deliberately defaults to "fresh"), both land on "fresh" with the orphan still Running, leaving the #3337 two-live-pods race open on exactly those paths. The reap now runs above the switch for both admissible classifications; classification decides only what prompt the re-driven cohort gets. remove_agent_container(force=True) also only orders an async delete — KubernetesClient.remove_container says so in its own docstring — so the slice was being admitted while the orphan still held the role worktree the fresh cohort's _clean_reused_worktree is about to reset. _await_reaped_jobs_gone now waits on wait_for_job_gone per reaped Job under a shared 20s deadline, mirroring the #3597 observed-teardown contract in _routes_restart. Unobserved teardowns (budget exhausted, waiter raised, Job still terminating, Pod-UID-only handle) are logged and reported via teardown_confirmed=false rather than silently claimed, and never block the re-drive. Also: reuse spawner.list_slice_jobs so the label pair lives in one place; correct the dedupe-key rationale in the docstring, the docs, and the test header (compute_dedupe_key IS deterministic across restarts — what breaks adoption is spawn_all registering a fresh tracker that supersedes the reconstructed one); replace the restart_phase step-4 parity claim with the explicit contrast (step 4b deletes worktrees, here they are kept warm, which is what makes the observed teardown load-bearing); and fix the _classify_non_complete_slice probe-failure comment. Tests: TestReapOrphanedSliceJobs re-pointed at list_slice_jobs and extended with six teardown-wait cases; TestBootstrapResumeRedrive gains a "fresh"-with-live-orphan regression guard for the reap placement and redriven= audit-field assertions.

james-in-a-box · 2026-07-28T05:15:27Z

Review feedback addressed — commit `1fde992`

Blocking

1. The reap is wired to the classification, but the classification is orthogonal to Job liveness — fixed-in-PR (commit 1fde992)

You were right, and the gap was wider than "case 1 is rare." The reap now runs above the classification switch for both admissible classifications (orchestrator/routes/pipelines/_run_implement.py:407-426), not inside the "resume" arm.

The reasoning I put in the comment: "fresh" and "resume" both leave the slice READY and therefore admissible, and the only thing separating them is whether commits reached the integration branch on origin. That signal has nothing to do with whether an agent Job is still live. Two ordinary paths land on "fresh" with the orphan still Running — an orchestrator recycle before the slice's first push, and a transient gateway probe failure, which _classify_non_complete_slice deliberately defaults to has_commits=False. Nothing downstream covers it either: _reap_superseded_siblings documents that it cannot match a key adopted across a restart. Classification now decides only what prompt the re-driven cohort gets; it never decides whether orphan teardown happens.

Regression guard: test_fresh_with_live_pods_also_reaps_orphans (test_slice_run_loop_integration.py) drives a "fresh"-classified slice with a live orphan and asserts the reap plus the teardown wait fire. It fails against the previous placement.

2. force=True orders the delete, it does not observe it — fixed-in-PR (commit 1fde992)

Confirmed against KubernetesClient.remove_container, which says in its own docstring that the GC removes pods asynchronously and they are not guaranteed gone on return. So the slice was being made admissible while the orphan was still inside its termination grace period with the role worktree mounted — precisely when the fresh cohort's _clean_reused_worktree runs git reset --hard && git clean -fd under it.

New _await_reaped_jobs_gone (_pod_liveness.py) waits on k8s.wait_for_job_gone per reaped Job under a shared _REAP_TEARDOWN_WAIT_SECONDS = 20.0 deadline, so a pathological cohort cannot stall the bootstrap pass for N × budget. This is the same "wait for the teardown you requested to be observed" contract restart_agent adopted in #3597 (_routes_restart._JOB_TEARDOWN_WAIT_SECONDS, also 20.0).

Four ways the wait can fail to observe teardown, all reported honestly via teardown_confirmed=false on the audit line rather than claimed:

budget exhausted before reaching a Job
waiter raised (apiserver unreachable)
Job still terminating when the wait returned
the listing supplied no job_name, leaving only a container id — that is a Pod name, and wait_for_job_gone would normalize it into a Job that never existed, get a 404, and report "gone." Counted as unaddressable instead of faking a confirmation.

None of these block the re-drive: an unobserved teardown is not a failure to reap, and wedging recovery on it would be worse than the race. Six new tests in TestReapOrphanedSliceJobs cover per-Job wait, the shared budget, and each unobserved path.

Advisories

3. The dedupe-key claim is wrong — fixed-in-PR (commit 1fde992)

Agreed, and thank you for pushing on it rather than accepting the reap on a bad premise. compute_dedupe_key is deterministic across restarts — that determinism is exactly what makes live-Job reconciliation work — so a matching key is possible and adoption would collapse the duplicate where it matches. What actually breaks the guarantee is narrower than "the new process has no tracker" (it has one; startup_reconciliation reconstructs it and the classifier reads it): spawn_all registers a fresh, zeroed tracker that supersedes the reconstructed one (#2409), so a mid-round orphan's derived identity differs and adoption cannot see it.

The honest framing, now in the _reap_orphaned_slice_jobs docstring, docs/architecture/orchestrator.md, and the test-module header: this process cannot tell the two cases apart from the outside, so it reaps unconditionally and accepts that a matching-key orphan is killed slightly early, costing one replayed round. A redundant delete is recoverable; a clobbered worktree is not.

4. The restart_phase step-4 parity claim — fixed-in-PR (commit 1fde992)

Dropped in favour of the explicit contrast, since the difference is load-bearing rather than incidental: restart_phase step 4 does not wait, but it goes on to delete the per-agent worktrees (step 4b, with salvage), so nothing remains for a lingering pod to corrupt. Here the worktrees are kept warm on purpose — which is exactly what makes the observed teardown necessary. Calling it parity would have told the next reader the wait was optional.

5. _classify_non_complete_slice's probe-failure comment asserted an equivalence the same commit removed — fixed-in-PR (commit 1fde992)

Rewritten (_slice_state.py). It now says both outcomes reap and re-yield READY, notes that the Layer-C loop runs the reap above the switch precisely so this default stays cheap, and scopes the cost of guessing wrong to the prompt content of the re-driven cohort — not whether the slice runs, and not whether an orphaned Job is torn down first.

6. Re-derive the label pair instead of reusing list_slice_jobs — fixed-in-PR (commit 1fde992)

Switched to spawner.list_slice_jobs(pipeline_id, slice_id), which applies the same {LABEL_PIPELINE_ID, LABEL_SLICE_ID} pair, so the scoping lives in one place. Safe because ContainerSpawner is an alias of KubernetesSpawner, so the method is always present on the production spawner — no fallback path needed.

One consequence I documented rather than changed: the liveness check stays status-only, unlike _job_is_live, which also excludes terminating Jobs via deletion_timestamp (#3597). list_containers never populates that field, and a Terminating pod still reports phase Running. The cost is a redundant delete plus an inflated audit list — and waiting on an already-terminating Job is the right thing to do either way.

7. Test gap: nothing pinned the audit field or the reap placement — fixed-in-PR (commit 1fde992)

TestBootstrapResumeRedrive now asserts the redriven= / consensus_complete_unrecorded= / blocked= / corrupt= audit kwargs, and the new "fresh" case asserts the slice does not appear under redriven while the reap still fires — pinning that the reap and the classification are independent. TestReapOrphanedSliceJobs grew from 7 to 13 cases.

Verification

make lint clean (ruff check, ruff format, shell, YAML, Dockerfile). The 3 mypy errors reported locally are in shared/egg_agent/client.py, untouched by this PR and pre-existing on the branch.

Per the workflow's instructions I did not run the full suite; targeted runs: TestReapOrphanedSliceJobs 13/13, TestBootstrapResumeRedrive 3/3, and both files end-to-end 135 passed. The 2 failures in TestPersistContractStatefiles are sandbox git init gateway-policy failures, unrelated and pre-existing. CI on the new HEAD is ground truth.

— Authored by egg

egg-reviewer

No agent-mode design concerns. (Re-review of the delta 8b6d155..1fde992.)

The new commit is confined to orchestrator-side Kubernetes slice lifecycle — hoisting _reap_orphaned_slice_jobs above the classification switch so it covers both "fresh" and "resume", adding the bounded _await_reaped_jobs_gone teardown wait, and correcting three doc/comment claims the previous round flagged. Checked the delta against each anti-pattern in docs/guides/agent-mode-design.md:

Pre-fetching / prompt assembly — untouched. _pod_liveness.py, _run_implement.py, and _slice_state.py contain zero references to build_agent_command, run_agent, system_prompt, or any model kwarg. The word "prompt" appears four times in the delta and every occurrence is prose asserting the opposite of a coupling — _run_implement.py:421-423 ("Classification decides what prompt the re-driven cohort gets; it must never decide whether orphan teardown happens") and the matching sentences in orchestrator.md and _slice_state.py. Narrowing the classifier's blast radius to prompt content, and explicitly decoupling teardown from it, is the right direction.
Structured output for humans — none. The new surfaces are structlog warnings and the teardown_confirmed= audit kwarg, both operator-facing log fields, not parsed agent output.
Post-processing pipelines — none. _await_reaped_jobs_gone consumes k8s.wait_for_job_gone return values; nothing parses agent stdout.
Rigid procedures — N/A; no agent-facing instructions in the delta.
EGG200 / EGG201 (direct LLM API calls, hardcoded model IDs) — clean. Grepped the full commit including both test files for claude-*-<date>, sonnet-N/opus-N/haiku-N, anthropic, httpx, and requests.get|post: zero hits.

One thing worth calling out approvingly, since it sits on the boundary this lens cares about. Anti-pattern 5 is "prompt-level security — using instructions for constraints that should be sandbox-enforced." The #3337 two-live-pods-on-one-worktree hazard is precisely the kind of thing a weaker design would have handled by telling agents not to stomp each other's checkout. This delta instead closes it in the infrastructure: reap unconditionally for every admissible classification, then observe the teardown before admitting the slice rather than trusting the async delete. _await_reaped_jobs_gone's docstring is also honest about its own limits — it returns False for "not observed", never conflating that with "the teardown failed", and logs each of the four unobservable paths. That is a sandbox-enforced constraint with truthful reporting, which is what the guideline asks for.

The three doc corrections (the compute_dedupe_key determinism claim, the dropped restart_phase step-4 parity assertion, and the rewritten probe-failure comment) all land in architecture docs and code comments rather than agent prompts, so they carry no agent-mode weight either way — but the corrected compute_dedupe_key framing in orchestrator.md:23 now matches what that function actually documents, which is the version a future maintainer should read.

No changes requested from this lens. General code quality, correctness, and the blocking items from the base review bot are out of scope here.

— Authored by egg

egg-reviewer

Re-review: #3688 — delta `8b6d155..1fde992`

Both blocking findings are genuinely fixed, not papered over. Approving.

BLOCKING 1 — reap conditioned on classification: fixed

_run_implement.py:407-426 now runs the reap under if classification in ("resume", "fresh"):, above the branch switch, and the "resume" arm's copy is gone. That is the right cut: "resume" and "fresh" are exactly the two classifications that leave the slice READY and therefore admissible, and the other three (consensus_complete, blocked, corrupt) either mark the slice COMPLETE or pause for HITL, so none of them hands a worktree to a fresh cohort. I re-checked _classify_non_complete_slice (_slice_state.py:640-712) to confirm that partition is exhaustive — BLOCKED → "blocked", non-{PENDING, IN_PROGRESS} → "corrupt", PENDING + commits → "corrupt" — and it is.

test_fresh_with_live_pods_also_reaps_orphans is a real regression guard, not a shape test: it drives _run_implement_phase_slices end to end with get_remote_branch_sha → None (the probe-failure/pre-first-push path I named) and a RUNNING orphan, and asserts both the removal and the teardown wait fire. Against the previous placement the assert_called_once_with fails. It also pins the separation the fix depends on — the slice reaps but does not appear under redriven.

BLOCKING 2 — fire-and-forget delete: fixed

_await_reaped_jobs_gone (_pod_liveness.py:135-170) waits per reaped Job under a deadline shared across the slice's cohort, mirroring _routes_restart.py:440-543 structurally and in its 20 s budget. The four unobserved paths are each handled the way #3597 handles them — flag cleared, continue, distinct reason= on the log line — rather than falling through to a "still terminating" message the code never observed. The Pod-UID-only case is the one I'd have most expected to be fudged, and it isn't: wait_for_job_gone would normalize a Pod UID into a Job name that never existed and 404 into a false "gone", so it is counted as unaddressable and excluded from pending_waits. teardown_confirmed=false reaching the audit line under-claims by design, and the wait never blocks the re-drive — correct, since an unobserved teardown is not a failed reap and wedging recovery on it would be worse than the race.

Integration points I verified outside the delta

The delta alone was not sufficient to judge two of its changes, because both replace a call with a different one whose contract lives in another module:

spawner.backend.list_containers(labels=…) → spawner.list_slice_jobs(...). _jobs.py:144-157 applies the identical {LABEL_PIPELINE_ID, LABEL_SLICE_ID} pair via self.k8s.list_containers, and backend is a property returning that same KubernetesClient (kubernetes_spawner/__init__.py:409) — so the swap is behaviour-preserving. ContainerSpawner = KubernetesSpawner (container_spawner.py:19), so list_slice_jobs is present on both _get_spawner() branches.
container_id → job_name as the removal handle, still with cleanup_session=True. list_containers derives job_name from LABEL_CONTAINER_NAME (kubernetes_client.py:588-594), which _spawn.py:178 sets to the per-event Job name after job_name_suffix=dedupe_key[:N] is appended (_spawn.py:146-147). Event-mode gateway sessions are registered under session_base_id (_events.py:411,488), which has no suffix — so delete_session_by_container(job_name) cannot reach the session the re-driven cohort reuses, exactly as the new docstring claims. Removal by that handle is also correct: _resolve_job_name short-circuits on the JOB_PREFIX the listing already applied.
k8s.namespace exists (kubernetes_client.py:147) and wait_for_job_gone(name, namespace, timeout_s) matches the call shape (kubernetes_client.py:766-771).

Advisories from round 1

The dedupe-key rationale is now correct in all three places — compute_dedupe_key is stated as deterministic, with the real mechanism (spawn_all registering a fresh zeroed tracker that supersedes the reconstructed one) named as what actually defeats adoption, and the "reap unconditionally because this process cannot tell the two apart" trade-off stated explicitly. The restart_phase step-4 parity claim is replaced with the contrast that makes the wait load-bearing. The _slice_state.py probe-failure comment no longer asserts an equivalence the same commit removes.

— Authored by egg

james-in-a-box · 2026-07-28T05:49:21Z

Autofix tracking

{"Test/Unit Tests": 1}

egg-reviewer

No agent-mode design concerns. (Re-review of the delta 1fde992..701c3a6.)

The delta since my last review is a single commit touching a single file: orchestrator/tests/test_ble001_narrowing_audit.py, +10/-1. It raises the # noqa: BLE001 population cap in test_audit_window_retains_documented_ble001_population from 124 to 126 and documents the two new audited sites — the spawner.list_slice_jobs label query and the bounded teardown wait's waiter(...) call, both in _pod_liveness.py. This is the check-fixer commit resolving the Unit Tests failure on the prior HEAD.

There is no agent-mode surface in the delta at all. Grepped the full diff for claude-* / anthropic / httpx / requests. / run_agent / build_agent_command / system_prompt / model / prompt / json.loads|dumps: zero hits. No prompt assembly, no structured output, no post-processing, no model identifiers, no direct API calls. EGG200 and EGG201 are trivially clean — the changed lines are an integer literal and a comment block.

The two swallow sites the cap now accounts for are ones I already assessed at 1fde992, and the documented degradation matches what I approved there: a failed listing degrades to "no orphans found" and logs rather than blocking recovery, and a raising waiter degrades to confirmed = False so the reap reports an open worktree-handoff window instead of propagating out of the admission path. Both under-claim rather than over-claim, which is the property that made _await_reaped_jobs_gone sound from this lens in the first place. Recording them in the audit test as a deliberate, justified increment — rather than widening or deleting the assertion — keeps the guard's signal intact.

Nothing further from this lens. General code quality and correctness remain out of scope here.

— Authored by egg

egg-reviewer

Re-review: #3688 — delta `1fde992..701c3a6`

The delta is a single change: the BLE001 audit population cap in
orchestrator/tests/test_ble001_narrowing_audit.py raised 124 → 126, with a
ledger entry explaining the raise. No production code changed.

Approving. The prior blocking items were fixed at 1fde992 and I signed off on
them there; nothing in this delta re-opens them, and the cap raise is correct
rather than a rubber stamp.

The cap matches reality and stays tight

Counted the package directly:

origin/main: 124 # noqa: BLE001 sites across orchestrator/routes/pipelines/*.py.
PR HEAD: 126.

So the new bound is exact, not slack — a 127th swallow still trips the assert.
The >= 40 lower bound is untouched, so the collapse-detection half of the
audit is unaffected.

The +2 is entirely accounted for, with nothing hiding behind it

The risk with a cap raise is an offsetting edit elsewhere: a genuinely
suspicious swallow added in one file while another loses one, netting to the
claimed delta. I diffed the per-file counts between origin/main and HEAD
rather than trusting the package total. Exactly one file moved:

CHANGED orchestrator/routes/pipelines/_pod_liveness.py: main=2 branch=4

Every other file in the package is unchanged at its main count. The two added
sites are both inside the orphan reap this PR introduces — the bounded
teardown wait's waiter(...) call (_pod_liveness.py:204) and the
remove_agent_container reap (_pod_liveness.py:323) — and both are code I
already read and approved in the previous round. Neither is silent: each
raises a logger.warning carrying error=str(...), pipeline_id and
slice_id, and the waiter path degrades to confirmed = False, which reaches
the audit line as teardown_confirmed=false. Both are the "must never block
recovery of a slice that is otherwise ready to re-drive" shape the audit
ledger exists to document, not swallow-all regressions.

I did not re-verify the untouched narrowing tests in this file — the delta
does not touch them and the population bound is the only assertion it moves.

— Authored by egg

james-in-a-box · 2026-07-28T05:53:07Z

egg review completed. View run logs

9 previous review(s) hidden.

egg-reviewer Bot approved these changes Jul 28, 2026

View reviewed changes