Stop the driver when a pipeline is cancelled (#3633) by jwbron · Pull Request #3645 · jwbron/egg

jwbron · 2026-07-26T22:44:49Z

Closes #3633.

The bug

cancel_task set the status to CANCELLED, tore down the pipeline's containers, and cleared its runtime state; it never stopped the thing that creates containers. The _run_pipeline driver's work loops and each slice's BRC event loop kept running in-process, so the next poll re-derived its arms and spawned again.

issue-3596-v2 was cancelled at 20:48Z and spawned slice-3 agents at 22:55Z, complete with a fresh integration branch forked off slice-1's tip. Killing the pods removed the symptom while the spawner ran on. Reproduced a third time on issue-3632-v1: two more agent Jobs within a minute of the cancel.

It also breaks an assumption _routes_restart.py:330 already documents in prose and whose correctness argument depends on it: that a CANCELLED pipeline has no live event loop and no live driver thread.

The fix

Four layers, so a miss at any one costs a poll rather than a slice.

1. The cancel route stops the loops, before cleanup. _stop_pipeline_event_loops walks the event_loop live-loop registry (#3496, keyed by (pipeline_id, slice_id) so every concurrent slice is covered) and stops each one. Ordered ahead of _background_cleanup, which until now raced loops still entitled to spawn replacements. Called with join_timeout=0.0: the stop event and the registry eviction are both synchronous, so the operator's PATCH never waits on a daemon thread's wind-down.

2. A loop stopped mid-tick refuses the spawn. run() only checks its stop event between ticks, and stop() arrives from another thread, so a stop landing mid-tick would still get one final cohort of Jobs out the door. _handle_role re-checks immediately before the spawn decision and returns blocked="stopped" — a value the arms-exhausted / arms-parked wedge detections deliberately do not count, since a teardown is not something an operator can resolve.

3. The concurrent-phase poll loop re-reads the persisted status. Folded into the existing #3315 supersession check as _phase_bail_reason_impl, so both conditions resolve from a single pipeline load per tick. The impasse-retry wrapper uses the same helper, so a stale impasse file cannot escalate a HITL — or, on the all-delegated branch, drive another retry iteration — against a stopped run.

4. The slice loop stops admitting slices. _run_implement_phase_slices re-reads the status at the top of each tick, so an in-flight wave is the last one and the next slice is never admitted, its integration branch never created, its cohort never spawned.

_run_phase_execution maps the resulting non-zero exit onto a clean thread return rather than a phase failure. Without that, the operator's CANCELLED gets rewritten to FAILED, losing both their intent and the CANCELLED-only worktree preservation (#1725) that restart_phase resumes from.

Why FAILED is excluded

Every check above keys on CANCELLED only. container_monitor reconciliation can mark a live pipeline FAILED mid-phase, and the consensus-complete path in _run_concurrent_phase recovers it to RUNNING (#1273). Treating FAILED as terminal here would convert that recoverable transient into a permanently idle pipeline. The rationale is written into each site so it does not get "tidied up" later.

Tests

orchestrator/tests/test_cancel_stops_driver.py, 15 cases across the four layers. The headline one is the issue's requested regression: cancel a pipeline mid-implement with an un-admitted slice remaining, then assert the ready set is never read, no slice is admitted, no integration branch is created, and no agent Job is spawned.

Also covered: stop-before-cleanup ordering; other pipelines' loops left alone; idempotent re-cancel is a no-op; the FAILED carve-out; blocked="stopped" not reading as an operator wedge; the store-hiccup and missing-store tolerances; and control cases proving none of the guards stop a healthy RUNNING pipeline.

Verified as regression tests: reverse-applying the source diff fails 12 of the 15.

Verification

make test: 5959 passed. Two failures (test_production_worktree_base_dir_lies_within_gateway_allowlist, test_probe_skipped_when_request_context_missing) reproduce identically on a clean tree — both environment-dependent and unrelated.
make lint clean; the file-size gate passes (_run_concurrent.py net -4 lines, so the empty allowlist stays empty).

Not addressed

#3632 — cancel destroying the state it promised to preserve — is the opposite half of the same broken contract and is left for its own change.

cancel_task set the status to CANCELLED, tore down the pipeline's containers, and cleared its runtime state, but it never stopped the thing that creates containers. The _run_pipeline driver's work loops and each slice's BRC event loop kept running in-process, so the next poll re-derived its arms and spawned again: issue-3596-v2 was cancelled at 20:48Z and spawned slice-3 agents at 22:55Z, complete with a fresh integration branch. Killing the pods removed the symptom while the spawner ran on. Four layers, so a miss at any one costs a poll rather than a slice: 1. The cancel route stops every live BRC event loop for the pipeline (reachable via the #3496 live-loop registry, so all concurrent slices are covered) and does it BEFORE container cleanup, which until now raced loops still entitled to spawn replacements. stop(join_timeout=0.0): the stop event and registry eviction are synchronous, so the operator never waits on a daemon thread. 2. A loop stopped mid-tick refuses the spawn it was about to request. run() only checks between ticks, and stop() arrives from another thread. 3. The concurrent-phase poll loop re-reads the persisted status each tick and bails without escalating. This folds into the existing #3315 supersession check as _phase_bail_reason_impl, so both conditions resolve from one pipeline load. The impasse-retry wrapper uses the same helper so a stale impasse cannot escalate a HITL, or drive another retry iteration, against a stopped run. 4. The implement-phase slice loop re-reads the status before admitting a wave, so an in-flight wave is the last one. _run_phase_execution treats the resulting non-zero exit as a clean thread return rather than a phase failure; otherwise the operator's CANCELLED would be rewritten to FAILED, losing both their intent and the CANCELLED-only worktree preservation (#1725) that restart_phase resumes from. FAILED is deliberately excluded from every one of these checks: container_monitor reconciliation can mark a live pipeline FAILED mid-phase and the consensus-complete path recovers it to RUNNING (#1273). Treating it as terminal here would convert that recoverable transient into a dead run. This also restores the assumption _routes_restart.py already documents, that a CANCELLED pipeline has no live event loop and no live driver thread, which the restart path's correctness argument depends on.

egg-reviewer

Review: Stop the driver when a pipeline is cancelled (#3633)

The four-layer structure is the right shape and most of the load-bearing reasoning holds up under verification. Three findings block: one invalidates a property the PR explicitly asserts, one leaves a full agent cohort alive after cancel, one relabels the operator's cancel as a slice failure.

What I verified and confirmed correct

join_timeout=0.0 is safe. OrchestratorEventLoop.stop() (event_loop/_loop.py:1011) sets self._stop and calls _unregister_live_loop(self) synchronously before the join, so both load-bearing effects land regardless of the timeout. The PATCH thread genuinely does not block.
No mutation-during-iteration hazard. get_live_event_loops() (event_loop/__init__.py:782-804) returns a snapshot list, so stop() evicting the loop mid-iteration in _stop_pipeline_event_loops cannot raise.
blocked="stopped" really is excluded from wedge detection — not just asserted by the test. Both production detectors are all(d.blocked == "exhausted") (_loop.py:291) and all(d.blocked in ("parked","exhausted")) and any(== "parked") (_loop.py:429); a "stopped" decision falsifies both.
Layer 4's break still tears down the reconciler — the finally: at _run_implement.py:1451 sets reconciler_stop and joins on the break path.
The FAILED exclusion is correctly justified (#1273 container_monitor → recoverable FAILED). Excluding it is right, not an oversight.
Rename is clean. No leftover references to _superseded_by_restart_impl anywhere, and the inlined epoch comparison in _phase_bail_reason_impl is semantically identical to _pipeline_superseded_by_restart (store is None / run_epoch is None / load-failure all → no bail).
Every new symbol is barrel-re-exported and reached via _pkg., per the decomposition pattern. 15/15 new tests pass locally.

Blocking

1. The CANCELLED worktree preservation Layer 3b is built to protect does not exist — the driver's own `finally` deletes those worktrees

_run_phase.py:288-291 states the rationale for the new early return:

Falling through would rewrite the status to FAILED, losing the operator's intent and (via #1725) the CANCELLED-only worktree preservation that restart_phase resumes from.

The causality is inverted. _run_pipeline's finally sets skip_cleanup for exactly two cases — epoch change, and FAILED (_run_pipeline.py:1411-1416, "Pipeline failed, preserving worktrees for retry"). CANCELLED matches neither, so it falls into if not skip_cleanup: (1421) and gets:

_spawner.gateway.delete_worktrees(container_id=pipeline_id, force=True) — line 1423
per-agent egg-{pipeline_id}-{role} worktree deletion for every role — line 1453
cleanup_pipeline(..., preserve_worktrees=skip_cleanup) → preserve_worktrees=False — line 1485

So being marked FAILED is what would have preserved the worktrees; keeping the status CANCELLED routes into the delete branch. That also contradicts the PATCH route two frames earlier, which passes preserve_worktrees=(status_value == "cancelled") (_routes_crud.py:697) — the tree currently holds two opposite policies for the same status.

Why this PR: pre-#3645 the driver only reached that finally after the poll loop finished (consensus timeout, or the next-phase check at _run_pipeline.py:347), i.e. many minutes after the cancel. Layers 3b/4 now return within one poll interval, so the deletion lands seconds after the operator's cancel — precisely when they would call restart_phase, which allowlists CANCELLED for this exact reason (_routes_restart.py:125-133, "so that a cancel_task(cleanup=false) pipeline can be resumed without a full resubmission (see #1725)"). cleanup_pipeline's salvage push mitigates loss of committed work, but the resumable worktree that restart_phase/_try_reuse_worktree depends on is gone.

_run_pipeline.py is not in this diff, so pushing back on scope is reasonable — but then the comment at _run_phase.py:288-291 and the docstring at tests/test_cancel_stops_driver.py:401-404 assert a property the tree does not have, and must be corrected. The fix itself is one line at _run_pipeline.py:1411:

elif current.status in (_pkg.PipelineStatus.FAILED, _pkg.PipelineStatus.CANCELLED):

2. No cancel check before `spawn_all`, and the cancel bail never stops containers — a full agent cohort can outlive the cancel

Layer 4 reads the status at the top of the slice tick (_run_implement.py:545). The next cancel-aware read is Layer 3's step 0 at _run_concurrent.py:473 — which runs after executor.spawn_all(agent_prompts=agent_prompts) at line 310. Between those two points sit the contract load, per-role _build_agent_prompt (draft reads, BRC history, git diffs), gateway session/worktree setup, and integration-branch creation: tens of seconds, not microseconds.

A cancel landing in that window executes Layer 1 and the route's background cleanup_pipeline before those Jobs exist, then spawn_all mints a fresh cohort. The bail at _run_concurrent.py:480-483 then calls only executor.stop_event_loop() — unlike every consensus exit path in the same function (lines 544, 877, 1015), it never calls _stop_running_containers(). Nothing else reaps them: grep CANCELLED across kubernetes_monitor.py, health_monitor.py, and startup_reconciliation.py returns nothing, and cleanup_pipeline only re-runs on an operator DELETE. Those agents run their full session against a pipeline the operator stopped, minting gateway sessions and pushing to slice branches — the #3633 symptom, just through a narrower door.

Two changes close it:

a _pipeline_cancelled(store, pipeline_id) check immediately before executor.spawn_all();
_stop_running_containers() on the pipeline_cancelled branch of the step-0 bail (keep it off the superseded_by_restart branch, where the new thread legitimately owns them).

3. The operator's cancel is recorded as a slice failure

The cancel bail returns exit 1, which reaches unchanged code at _run_implement.py:908-916:

if exit_code_inner != 0:
    scheduler.record_failure(slice_id)
    _pkg.logger.warning("Slice failed", ...)

record_failure (slice_scheduler.py:378-428) sets the slice FAILED, arms the cascade, and calls _emit_slice_closed(slice_id, SLICE_OUTCOME_FAILED), which publishes EventType.SLICE_CLOSED with outcome="failed" to the bus (_run_implement_support.py:395-417). So the same intent-preservation argument the PR makes for Layer 3b is violated one level down: operators and SSE consumers see a failed slice plus a Slice failed warning for a clean cancel. The 60 s cascade grace means the downstream-blocked alert probably doesn't fire before Layer 4 breaks, and the contract isn't corrupted — but the event and the log line are wrong. Branch on _pipeline_cancelled (or thread the bail reason back) before record_failure.

Non-blocking

_cancel_pipeline_in_process bypasses Layer 1. routes/decisions/_handlers.py:1062-1101 is the second place CANCELLED originates (first-principles "Don't build"). It flips the status, emits PIPELINE_CANCELLED, cancels decisions — but never calls _stop_pipeline_event_loops, and despite the docstring's "status CANCELLED + cleanup" it does no container cleanup at all. Layers 3/4 catch the driver a poll interval late; until then the loops keep deriving arms. One call fixes the loop half.
_stop_pipeline_event_loops swallows ImportError silently. The nested try/except ImportError: return 0 means an import regression turns cancel back into #3633 with zero signal — exactly the "operator-facing misconfiguration produces no signal" shape. Log a warning on that path.
_pipeline_superseded_by_restart is now dead production code with a false docstring. After the rewrite it is referenced only by tests/test_restart_phase_consensus_timer.py and the barrel; its docstring still claims it is "Shared by the _run_concurrent_phase poll loop and the slice-path impasse-retry wrapper". Either delete it (and its test) or have _phase_bail_reason_impl delegate to it for the epoch arm, which would also remove the duplicated comparison.
Test fixtures diverge from production types. _StubScheduler.list_slices() returns SimpleNamespace(slice_id="slice-3", state="ready") — a plain string where production yields SchedulerSliceState, so the new rt.state != SchedulerSliceState.COMPLETE comprehension is never exercised against the real enum. The layer-4 invocation also omits run_epoch, which the production call site at _run_implement.py:212-224 always passes; it defaults to None, so the test silently covers a configuration production never uses. Use the real SchedulerSliceState and pass run_epoch.
Nit: _routes_crud.py:606-632 — two adjacent if blocks with byte-identical conditions (status == CANCELLED and prev_status != CANCELLED). The separate comment blocks are worth keeping; merge the conditions.

— Authored by egg

The cancel-stops-driver work adds three audited noqa: BLE001 sites (best-effort event-loop teardown, and the two store-load probes in the cancellation/supersede checks), pushing the file-wide documented population from 121 to 124.

james-in-a-box · 2026-07-26T23:00:35Z

Autofix tracking

{"Test/Unit Tests": 1}

egg-reviewer

Re-review: Stop the driver when a pipeline is cancelled (#3633)

Delta since 1b18503: one commit, 4ebcb7a, touching only orchestrator/tests/test_ble001_narrowing_audit.py. No source file changed. All three blocking findings from the previous review stand unaddressed — I re-verified each against the current tree below rather than restating them from memory.

The new commit

The bound change itself is correct. I verified it independently:

git diff origin/main...HEAD -- orchestrator/routes/pipelines/ adds exactly 3 noqa: BLE001 lines and removes 0. 121 → 124 is right.
Recomputing the test's own expression over the package source gives exactly 124, so the <= bound is tight, not slack. The ratchet still bites on the next un-audited addition.

New non-blocking finding: the audit comment misattributes 2 of the 3 sites it documents

test_ble001_narrowing_audit.py:176-183 is the file's documentation trail — its assert message tells a future reader "a future PR appears to have re-introduced swallow-all handlers without re-running the audit," so the enumeration is the artifact that makes the bound auditable. Two of the three names in it are wrong:

_stop_live_event_loops does not exist. grep -rn "_stop_live_event_loops" orchestrator/ returns exactly one hit: line 177 of this comment. The real function is _stop_pipeline_event_loops (_lifecycle_helpers.py:158).
_pipeline_superseded_by_restart did not gain a site. Its # noqa: BLE001 at _run_support.py:137 is pre-existing and untouched by this PR — git diff origin/main...HEAD does not show it as added. The site the PR actually added is in _phase_bail_reason_impl (_run_concurrent_support.py:302), which is now undocumented by the comment that claims to enumerate all three.

Only the third name — the CANCELLED status check, i.e. _pipeline_cancelled at _run_support.py:422 — is accurate.

This is sharper than a typo because of previous finding 6: _pipeline_superseded_by_restart is now dead production code (its only remaining references are __init__.py:1437, test_restart_phase_consensus_timer.py, and — now — this comment). The commit that deletes it will consult this comment, read that it owns one of the three new sites, and reason about the bound from a false premise. Fix the two names.

Previously-blocking findings: re-verified, all still present

1. CANCELLED still routes into worktree deletion — unchanged

_run_pipeline.py:1411 is still elif current.status == _pkg.PipelineStatus.FAILED:. CANCELLED matches neither that nor the epoch arm, so it falls into if not skip_cleanup: (line 1421) and reaches preserve_worktrees=skip_cleanup → False (line 1485). _routes_crud.py:697 still passes preserve_worktrees=(status_value == "cancelled"). The tree still holds two opposite policies for the same status, and the comment at _run_phase.py:288-291 still asserts a preservation property the driver's own finally negates. Layers 3b/4 shorten the window from minutes to one poll interval, which is what makes this land.

2. No cancel check before `spawn_all`; the bail leaves containers running — unchanged

executor.spawn_all(agent_prompts=agent_prompts) is still at _run_concurrent.py:310, with no status re-read between Layer 4's read (_run_implement.py:545) and it. The step-0 bail at _run_concurrent.py:482 still calls only executor.stop_event_loop(). I re-ran the call-site scan: _stop_running_containers is invoked at lines 544, 739, 877, 1015, 1244, 1420 — every consensus exit path in the file — and at none of them is it the cancel bail. A cohort minted in the pre-spawn_all window survives the operator's cancel with nothing to reap it.

3. The cancel is still recorded as a slice failure — unchanged

_run_implement.py:909 still runs scheduler.record_failure(slice_id) unconditionally on exit_code_inner != 0, which the cancel bail's return 1 satisfies. SLICE_CLOSED with outcome="failed" and a Slice failed warning for a clean operator cancel.

Carried non-blocking findings (4-8)

All unchanged; see the previous review. Finding 6 in particular now has a second reason to act on it, per the new finding above.

Verdict

The bound bump is a correct CI fix and I have no objection to it on its own terms. But it does not touch any of the three blocking findings, so the verdict does not move. Fixing the two symbol names in the audit comment can ride along with whichever revision addresses the blockers.

— Authored by egg

Blocking: - _run_pipeline's finally block deleted the worktrees CANCELLED is supposed to preserve, contradicting restart_phase's CANCELLED allowlist (#1725) and the PATCH route's preserve_worktrees flag. CANCELLED now joins FAILED on the skip_cleanup arm. - _run_concurrent re-reads the cancel status immediately before spawn_all, so a cancel landing during prompt-building/worktree-setup never mints an orphaned cohort, and the step-0 cancel bail now reaps the containers it owns. Deliberately not done on the superseded_by_restart arm, where the new thread owns them (#3315). - A cancel-driven non-zero phase exit is no longer recorded as a slice failure, so operators and SSE consumers stop seeing a FAILED slice (plus its downstream cascade) for a clean cancel. Non-blocking: - _cancel_pipeline_in_process (the first-principles 'Don't build' origin of CANCELLED) now stops the live BRC event loops too. - The event-loop registry ImportError is logged loudly instead of silently returning 0. - Deleted the now-dead _pipeline_superseded_by_restart; its epoch comparison lives solely in _phase_bail_reason_impl, which resolves both bail conditions from a single pipeline load per tick. Its #3315 coverage is retargeted at the live implementation. - test_cancel_stops_driver's stub slice carries a real SchedulerSliceState and both calls thread run_epoch. - Merged the two byte-identical CANCELLED blocks in the PATCH route, keeping both comment blocks. - Corrected the BLE001 audit note (_stop_pipeline_event_loops, _phase_bail_reason_impl, _pipeline_cancelled) and moved the bound to 123: three sites added, one removed with the deleted predicate.

james-in-a-box · 2026-07-26T23:17:08Z

Review feedback dispositions

All nine items are fixed. The gateway blocks egg from pushing to issue-3633-cancel-stops-driver (human-owned branch, not egg/-prefixed), so the fixes land in #3649, based on this PR's head — merge it into this branch and the fixes are in. Every disposition below refers to commit 4151b58 on that branch.

Blocking

1. _run_pipeline's finally deletes the worktrees CANCELLED is supposed to preserve — fixed-in-PR (commit 4151b58)

You were right that the driver's own teardown contradicted two other layers. restart_phase allowlists CANCELLED precisely so a cancel_task run can be resumed without resubmission (#1725, _routes_restart.py:125-133), and the PATCH route already passes preserve_worktrees=(status == "cancelled") to cleanup_pipeline. CANCELLED now joins FAILED on the skip_cleanup = True arm in _run_pipeline.py. The #3633 layers matter here: they make the driver's finally land seconds after the cancel rather than at the next consensus timeout, so the two policies have to agree.

2. Cancel between the slice-loop guard and spawn_all mints an unreapable cohort — fixed-in-PR (commit 4151b58)

Two changes in _run_concurrent.py. A _pipeline_cancelled re-read now sits immediately before executor.spawn_all(...) — contract load, per-role prompt building, gateway session + worktree setup, and integration-branch creation take tens of seconds between the loop's guard and the spawn, and a cancel in that window runs the route's teardown before the Jobs exist, so nothing reaps them (no reconciler acts on CANCELLED; cleanup_pipeline only re-runs on an operator DELETE). And the step-0 bail now calls _stop_running_containers(), so the thread reaps the cohort it owns like every consensus exit path in that function.

Scoped to the pipeline_cancelled arm only, as you asked — the superseded_by_restart branch is untouched, since there the new _run_pipeline thread legitimately owns those containers and stopping them would kill the restarted run's agents (#3315).

3. A clean cancel is recorded as a slice failure — fixed-in-PR (commit 4151b58)

_run_implement.py now checks _pipeline_cancelled before scheduler.record_failure(slice_id). Without it, the phase runner's non-zero cancel bail set the slice FAILED, armed the downstream cascade, and published SLICE_CLOSED(outcome="failed") to the bus — operators and SSE consumers would see a failed slice for a clean cancel. Same intent-preservation argument as _run_phase_execution's CANCELLED carve-out, one level down.

Non-blocking

4. _cancel_pipeline_in_process is a second origin of CANCELLED that doesn't stop the loops — fixed-in-PR (commit 4151b58)

The first-principles "Don't build" path in routes/decisions/_handlers.py now calls _stop_pipeline_event_loops(pipeline_id, reason="pipeline_cancelled") after the status flip, via a deferred import (matching the other hooks in that file — routes.pipelines is too heavy to bind at module import). Docstring rewritten to say so.

5. Silent ImportError swallow in _stop_pipeline_event_loops — fixed-in-PR (commit 4151b58)

A silent 0 there turns cancel back into #3633 with no signal at all: the operator's cancel reports success while every live loop keeps spawning. Now logs a warning naming the consequence before returning 0.

6. _pipeline_superseded_by_restart is dead code — fixed-in-PR (commit 4151b58)

Deleted, rather than delegated to. Delegating from _phase_bail_reason_impl would cost a second store.load_pipeline per tick and destroy the "single pipeline load per tick" property this PR claims. The epoch comparison now lives solely in _phase_bail_reason_impl, whose docstring's superseded_by_restart section was expanded to own it and to record that both callers — the _run_concurrent_phase poll loop and the slice-path impasse-retry wrapper — reach it through this one implementation. The #3315 coverage in test_restart_phase_consensus_timer.py is retargeted at the live helper rather than dropped: all four cases now drive _phase_bail_reason_impl and assert on "superseded_by_restart" / None.

7. _StubScheduler.list_slices() returns a state the scheduler never produces, and the calls don't thread run_epoch — fixed-in-PR (commit 4151b58)

test_cancel_stops_driver.py now imports SchedulerSliceState and returns a real READY state, and both _run_implement_phase_slices calls pass run_epoch= (the control test gained a running = _cancellable_pipeline() local for it). Without the epoch the supersession arm was dormant in both tests, so the control test wasn't proving what it claimed.

8. Two byte-identical CANCELLED conditions in the PATCH route — fixed-in-PR (commit 4151b58)

Merged into one block in _routes_crud.py, with the _stop_pipeline_event_loops call nested under the _emit_pipeline_event block. Both comment blocks preserved verbatim, per your note that they're worth keeping.

9. Two of the three names in the BLE001 audit comment are wrong — fixed-in-PR (commit 4151b58)

Corrected to _stop_pipeline_event_loops (_lifecycle_helpers.py), _phase_bail_reason_impl (_run_concurrent_support.py), and _pipeline_cancelled (_run_support.py).

One thing beyond the two names: the bound also had to move. Deleting _pipeline_superseded_by_restart (item 6) removed a pre-existing # noqa: BLE001 site, so the net for this PR is +3 −1. I re-ran the test's own counting expression against routes/pipelines/ and got 123, not the 124 currently asserted — the bound is now <= 123 and the comment accounts for the removal explicitly.

Verification

make lint — ruff clean. The 3 mypy errors are all in shared/egg_agent/client.py, which this PR does not touch; they are pre-existing.

Targeted tests: test_cancel_stops_driver.py, test_ble001_narrowing_audit.py, test_restart_phase_consensus_timer.py — 29 passed. test_decisions_routes.py, test_cancel_async_cleanup.py, test_resolve_contract_decision_route.py, test_first_principles_reviewer.py — 111 passed. Full suite left to CI.

— Authored by egg

Address review feedback on #3645 (cancel stops the driver)

egg-reviewer

Re-review — @ `7b2af26`

All nine items from the previous round are genuinely fixed; I traced each rather than pattern-matching the diff. One new blocking finding, which the delta did not introduce but which the cancel route this PR modifies is the trigger for, and which defeats the PR's stated goal on the most common parked state.

Blocking

1. Cancelling a pipeline parked at a HITL gate resurrects it to RUNNING and advances it

All four layers key on the persisted CANCELLED status. The gate path overwrites that status with RUNNING before any layer reads it, so all four go blind. This is deterministic on the default config (hitl_gates: bool = Field(default=True), models/_config.py:72) — no race window.

The chain:

Refine or plan reaches its gate (_HITL_GATE_PHASES = {"refine", "plan"}). _run_hitl_gate.py:387-392 persists pipeline.status = AWAITING_HUMAN, then line 402 blocks in dq.wait_for_decision(decision.id) — an unbounded 5 s poll with no cancel check (decision_queue.py:316-322).
Operator cancels. _routes_crud.py:606-628 flips the status to CANCELLED and calls _stop_pipeline_event_loops (layer 1). Then _routes_crud.py:631-640 — under the comment "cancel any pending decisions so wait_for_decision() unblocks" — runs dq.cancel_decision(decision.id), setting decision.status = CANCELLED and leaving resolution unset (decision_queue.py:279-280).
wait_for_decision returns. Its docstring is explicit: "The caller should inspect the returned decision's status (RESOLVED, CANCELLED, etc.) to determine the outcome." The gate never does. _run_hitl_gate.py:405-406 reads only resolution = (resolved_decision.resolution or "").strip() → "".
json.loads("") raises, falling to the legacy branch at _run_hitl_gate.py:445: if resolution.lower() in _pkg._APPROVE_KEYWORDS. And __init__.py:988 is _APPROVE_KEYWORDS = {"approved", "approve", "lgtm", "yes", ""} — the empty string is an approve keyword. _is_approved = True.
_run_hitl_gate.py:678-688, "Approved — resume and advance", takes the state lock, reloads, and writes pipeline.status = _pkg.PipelineStatus.RUNNING + phase_execution.status = COMPLETE, then store.save_pipeline(pipeline). The operator's CANCELLED is gone from the store.
The gate then persists the "resolution" to the contract and draft, commits statefiles, and pushes the branch (_run_hitl_gate.py:690-731) — all against a pipeline the operator cancelled.
Returns (pipeline, None) → the driver advances the phase → the outer loop head at _run_pipeline.py:347 reloads and reads RUNNING, so it does not break → the next phase runs → _run_concurrent_phase constructs a fresh ConcurrentPhaseExecutor with a fresh event loop, and the new pre-spawn guard at _run_concurrent.py:319 calls _pipeline_cancelled(store, pipeline_id) against a store that now says RUNNING, so it passes → a fresh cohort of agent Jobs is spawned.

That is the #3633 symptom verbatim — cancel at T, agents at T+n — reached through the one code path the four layers do not cover, and step 2 is in the function this PR modifies. Layer 1 stopping the loops does not help: the next phase starts a new one. Layers 3 and 4 re-read a status that step 5 has already rewritten.

It also falsifies the premise the PR writes into _pipeline_cancelled's docstring in _run_support.py — that the persisted status "is the one signal that survives every in-process mechanism". Two in-process mechanisms overwrite it unconditionally: _run_hitl_gate.py (four sites: 199/201, 544/546, 649/651, 681, none of which contains the string CANCELLED) and the AWAITING_HUMAN write at 387-392. Whatever the fix, that docstring sentence should not ship as written.

The minimal fix is at the seam step 3 already documents: after wait_for_decision returns, check resolved_decision.status and bail on DecisionStatus.CANCELLED (returning an action the driver maps to a clean thread exit, the way _run_phase.py:296 now does). Dropping "" from _APPROVE_KEYWORDS would close the specific "empty means approve" step but leaves the other cancelled-decision paths reading a cancel as operator input, so it is not sufficient on its own.

Verified fixed

Traced, not skimmed:

B1 — CANCELLED preserved for restart. _run_pipeline.py:1408-1432 now puts CANCELLED on the skip_cleanup arm alongside FAILED. Checked the consequence rather than the line: skip_cleanup gates only worktree deletion (1435-1478) and preserve_worktrees=skip_cleanup (~1500); cleanup_pipeline still runs, gated on pipeline_was_restarted, so Jobs and gateway sessions are still reaped. Skipping salvage is coherent — kubernetes_spawner/_jobs.py returns at ~245 before the #2429 salvage hook (304) and worktree deletion (330), and salvage exists only to precede deletion.
B2 — pre-spawn guard. _run_concurrent.py:306-320, correctly placed after ConcurrentPhaseExecutor(...) and before spawn_all. stop_event_loop() is safe pre-start (concurrent_executor.py:690-710 — explicitly a no-op if never started). _stop_running_containers() on the step-0 bail is correctly scoped to the pipeline_cancelled arm only, leaving superseded_by_restart untouched per #3315.
B3 — cancel is not a slice failure. _run_implement.py:908-927 guards record_failure. The early return still releases the global admission slot via _run_one_slice's finally (line 601), and the wave join at 1382-1413 only records a failure for raised exceptions, so the non-zero return does not get re-recorded there.
Items 4-9 all confirmed, including the dead-reference sweep after _pipeline_superseded_by_restart was deleted — barrel import dropped, no stragglers.
BLE001 bound. Recomputed independently: exactly 123 across routes/pipelines/**/*.py, and the +3 −1 accounting matches the diff. The three names in the comment are the right ones.
File-size allowlist. Legitimate: 1526 and 1515 lines against the 1500 hard cap; check-file-sizes.py exits 0; #3650 and #3651 are open with matching titles.
Targeted tests: 29 passed across test_cancel_stops_driver.py, test_restart_phase_consensus_timer.py, test_ble001_narrowing_audit.py. (Full suite not run.)

Non-blocking

2. Nothing pins any of the three fixes from the last round. The delta's only test change is the SchedulerSliceState / run_epoch tweak to two existing cases. The _run_pipeline.py CANCELLED→skip_cleanup change is the exposed one: a single enum added to a tuple, whose entire justification lives in a comment. A future tidy-up that "restores" the FAILED-only check is a silent regression of #1725 worktree preservation with nothing red. A test asserting skip_cleanup is true for a CANCELLED pipeline, and one asserting the pre-spawn guard is reached before spawn_all, would cover the two cheapest.

3. The run_epoch= threading added to the two layer-4 tests is inert. Layer 4 checks only _pipeline_cancelled; neither test reaches _run_concurrent_phase_with_impasse_retry (_count_ready returns [], then all_done() short-circuits). The argument is required by the signature, so passing it is correct — but it is not exercising the epoch path, and the disposition reads as if it were.

4. The pre-spawn guard's comment overstates its window. _run_concurrent.py:309-318 lists "integration branch creation" among the work it protects against. create_slice_integration_branch is called at _run_implement.py:777, inside the slice loop and before _run_concurrent_phase is invoked — so by the time this guard runs, the branch already exists. The guard is still right; the comment claims a slice of coverage it does not have.

5. The PR body now contradicts the diff. It says "the file-size gate passes (_run_concurrent.py net -4 lines, so the empty allowlist stays empty)". The change adds two entries to scripts/file-size-allowlist.yaml, re-populating the map whose emptiness was #3312's terminal acceptance criterion. The tracking issues make that an acceptable trade — the body just needs to say so rather than the opposite.

— Authored by egg

The refine/plan gate blocks in wait_for_decision, which the PATCH cancel route unblocks by cancelling the decision. A cancelled decision carries no resolution, and "" is in _APPROVE_KEYWORDS, so the gate read the operator's cancel as an approval: it wrote RUNNING over the persisted CANCELLED and advanced the phase, minting a fresh cohort. That is #3633 through the one path the persisted-status layers cannot see, because they all re-read a status this block has already overwritten. _gate_wait_cancelled checks both the decision's own status and the persisted pipeline status, and is applied at all three wait_for_decision sites in the gate. It returns a new "break" action that the driver maps onto the same loop exit its own CANCELLED check uses, so the finally observes CANCELLED and preserves the worktrees restart_phase resumes from. Also corrects the _pipeline_cancelled docstring (the persisted status is not inviolable — this gate overwrites it), narrows the pre-spawn guard's comment about the integration-branch window, notes that the slice-loop tests pass a production-faithful run_epoch that is not what they exercise, and pins skip_cleanup-on-CANCELLED plus the pre-spawn guard ordering.

james-in-a-box · 2026-07-26T23:55:17Z

Review response — @ `7b2af26` → `50fe3ae`

All five items addressed. The fixes are on egg/issue-3633-gate-cancel-bail and open as #3655 against this PR's branch (the gateway only accepts pushes to egg/-prefixed branches, same as #3649 last round).

Blocking

1. Cancelling a pipeline parked at a HITL gate resurrects it to RUNNING and advances it — fixed-in-PR (commit 50fe3ae)

The trace is right, including the part that makes it worse than a missing guard: the gate is the one path that overwrites the persisted status the other four layers key on, so no amount of re-reading downstream can see it. "" being an approve keyword is only the last step; the real defect is that step 3's own docstring asks the caller to inspect the returned status and the gate never did.

Fixed at that seam. _gate_wait_cancelled(resolved_decision, *, store, pipeline_id) returns true when the decision came back DecisionStatus.CANCELLED or when the persisted pipeline status is CANCELLED — the second half catches a cancel that lands after cancel_decision has already swept the queue, or one racing an operator who genuinely resolved the gate. It delegates the status half to _pipeline_cancelled, inheriting its FAILED carve-out (#1273) and its store-hiccup tolerance, so a transient state-branch lock can never invent a cancel and strand an approved gate.

Applied at all three wait_for_decision sites in the gate, not just the one named: the decision-ledger backstop, the main gate, and the "bare request → asked for specifics" follow-up. Each returns a new "break" action that _run_pipeline maps onto the same loop exit its own CANCELLED check at the loop head uses — so the finally observes CANCELLED and preserves the worktrees restart_phase resumes from, rather than exiting through a path that looks like completion.

Took the suggestion on _APPROVE_KEYWORDS too: "" is left in place. Removing it closes one step of one path and would silently change behaviour for every other caller of that set, and the status check makes it unnecessary.

The docstring sentence is gone. _pipeline_cancelled now says the persisted status outlives the in-process mechanisms a cancel cannot reach, and states plainly that it is not inviolable — the gate writes AWAITING_HUMAN / RUNNING over it as it converges, which is exactly why that path bails on a cancelled decision of its own accord instead of relying on a status its own approve branch would have overwritten.

Non-blocking

2. Nothing pins any of the three fixes from the last round — fixed-in-PR (commit 50fe3ae)

Both suggested pins landed, and I checked they are not vacuous rather than just green.

test_pipeline_failure_path.py::TestFailurePathPreservesWorktrees::test_worktree_cleanup_skipped_on_cancellation sits next to its FAILED twin and reuses the same harness. It asserts delete_worktrees is not called and that the safety-net cleanup_pipeline still runs with preserve_worktrees=True — the distinction you traced in B1, which an assert-not-called alone would not have pinned. Reverting the CANCELLED arm of the tuple locally fails it.
test_cancel_stops_driver.py::test_pre_spawn_guard_runs_before_spawn_all drives the real _run_concurrent_phase with a cancelled store and asserts spawn_all was never called and stop_event_loop was — ordering, not mere presence.

Layer 5 gets its own section of four: the cancel-route bail, the persisted-status bail, unit coverage of _gate_wait_cancelled (including the FAILED carve-out and the store hiccup), and a control proving a genuine approval on a live pipeline still advances — without which return "break" unconditionally would pass the first two.

3. The run_epoch= threading added to the two layer-4 tests is inert — fixed-in-PR (commit 50fe3ae)

Correct, and the disposition did read as if it were exercising the epoch path. The argument stays — production always threads the owning thread's epoch, so leaning on the None default would be less faithful, not more honest — but both tests now say so inline: layer 4 keys on _pipeline_cancelled alone and bails before _run_concurrent_phase_with_impasse_retry, where the epoch arm lives, and supersession is covered by test_phase_bail_reason_still_reports_supersession.

4. The pre-spawn guard's comment overstates its window — fixed-in-PR (commit 50fe3ae)

Right — create_slice_integration_branch runs at _run_implement.py:777, inside the slice loop, before this function is ever called. The comment now names integration-branch creation as being in the window but explicitly not covered by this guard, and points at the slice loop's own guard as the only thing that can prevent it.

5. The PR body now contradicts the diff — fixed (text below; needs a paste)

Agreed, and I have the corrected text — but gh pr edit 3645 is denied by the gateway (PR #3645 is not owned by james-in-a-box or configured user (author: jwbron)), so I cannot apply it myself. The Verification bullet should read:

make lint clean. The file-size gate passes, but not for free: _run_concurrent.py and _run_pipeline.py are both pushed over the 1,500-line cap by the review-feedback fixes, so scripts/file-size-allowlist.yaml — emptied by Decompose 19 oversize Python source files to clear the file-size allowlist (refresh of #3111) #3312 — carries two entries again, tracked by Decompose orchestrator/routes/pipelines/_run_concurrent.py (over the 1500-line hard cap) #3650 and Decompose orchestrator/routes/pipelines/_run_pipeline.py (over the 1500-line hard cap) #3651. That is a real regression against the decomposition program's terminal criterion, recorded rather than hidden.

The Tests section is also stale at "15 cases"; it is 20 in test_cancel_stops_driver.py now, plus the skip_cleanup pin in test_pipeline_failure_path.py.

Verification

make lint clean. 22 targeted tests pass (test_cancel_stops_driver.py 20, TestFailurePathPreservesWorktrees 2), plus 143 in the neighbouring HITL/gate/BLE001 suites to check the new bail did not disturb the existing gate paths. Per this workflow's constraint the full suite was not run — CI covers it on the PR HEAD.

— Authored by egg

Address review feedback on #3655. The cancel bail is extended from the phase gate to every blocking wait that parks at AWAITING_HUMAN and writes RUNNING back: the decision-ledger backstop, the explicit-none attestation gate, the follow-up specifics prompt, the contract-decision bridge, the unresolved-gap gate, and the divergence-reconcile pause. The divergence-reconcile pause returns aborted=True on a cancel, and _fail_pipeline_after_divergence_abort is now a status no-op on an already-CANCELLED pipeline (pre_event_hook still runs) so an operator cancel is not rewritten as a FAILED pin plus a pipeline.failed broadcast. Also pins the finding-4 case: a lone cancelled decision on a live pipeline must not stop the driver.

Address the round-2 review on #3655. The unresolved-gap gate's bail returned the same `gated=True` an ordinary gating returns, so the driver could not tell a cancel from a normal gating: it fell through to the terminal-phase branch (IMPLEMENT has no successor), wrote COMPLETE over the operator's CANCELLED, broadcast "completed successfully", and — with the finally no longer seeing CANCELLED — deleted the worktrees restart_phase resumes from. _run_implement_advance now re-reads the persisted status after the gate and returns "break", which _run_pipeline maps onto the same loop exit the HITL gate's bail uses. The re-read sits before the post-gate commit+push, so a cancelled pipeline stops mutating its remote branch too. The terminal-phase branch gains its own _pipeline_cancelled guard as a backstop for future park-and-resume blocks that forget to propagate. Also: the FAILED-sweep-reads-as-approval residual is now spelled out in the _gate_wait_cancelled docstring, and the draft-missing WARNING is scoped back to the create arm so the hoist doesn't add operator-facing noise on the reuse path.

Bail out of the HITL gate when the operator cancels

egg-reviewer

Re-review — @ `ed77cde`

Round-2's five items are genuinely fixed; I traced each to the line rather than pattern-matching the diff. The new work is substantially better than round 2 — the terminal-phase backstop in particular is the right shape, guarding the branch rather than the block.

One blocking finding remains, and it is the same defect this PR exists to close, reached through the half of the window the new guards do not cover. It is not a new regression, but the PR's stated goal is "cancel stops the driver" and on the create path it does not.

Blocking

1. Every new guard runs after the blocking wait. None runs before the decision is queued, or before the gate's own `AWAITING_HUMAN` write — so a cancel landing in the pre-wait window either hangs the driver forever or is silently overwritten

The PR already states this failure mode in its own words. _run_hitl_gate.py:315-326, the comment above the attestation bail:

falling through writes AWAITING_HUMAN over the persisted CANCELLED and then queues a fresh phase_gate decision, minted after the cancel route already swept the queue, so nothing will ever cancel it and the wait below never returns. That leaks the driver thread for the process lifetime and skips the finally entirely — no cleanup, no worktree preservation

That is exactly right. But it is closed only for a cancel arriving during the attestation wait. Every other route into the same code has the same window open, because the guards are uniformly placed after wait_for_decision and the queue-and-park sequence in front of them is unguarded.

The ordering, at the main gate:

_run_hitl_gate.py:484 — decision = dq.queue_decision(...)
_run_hitl_gate.py:495-502 — with get_pipeline_state_lock(...): pipeline = store.load_pipeline(...); pipeline.status = AWAITING_HUMAN; phase_execution.status = AWAITING_HUMAN; store.save_pipeline(pipeline) — unconditional, on both the reuse and create arms
_run_hitl_gate.py:512 — dq.wait_for_decision(decision.id)
_run_hitl_gate.py:527 — the new _gate_wait_cancelled check

The cancel route, _routes_crud.py:597-638:

pipeline = store.update_pipeline(pipeline_id, data) — persists CANCELLED under get_pipeline_state_lock
pending = dq.get_pending_decisions(); for decision in pending: dq.cancel_decision(decision.id) — a one-time snapshot. A decision minted after this line is never cancelled, and DecisionQueue.wait_for_decision (decision_queue.py:295-321) is a while True poll with no timeout.

Two distinct failures fall out, depending on which arm the gate takes:

(a) Create arm — permanent thread leak. Cancel lands after the sweep and before line 484. The gate mints a fresh phase_gate decision that nothing will ever cancel, writes AWAITING_HUMAN over the operator's CANCELLED at 495-502, and blocks at 512 for the lifetime of the orchestrator process. _run_pipeline's finally never runs — no container cleanup, no skip_cleanup worktree preservation, and the operator's PATCH returned 200. This is strictly worse than the pre-PR behaviour on the same input, where the gate at least fell through.

(b) Reuse arm — #3633 verbatim, still reproducible. Cancel lands before line 495. The state lock serialises against update_pipeline, so the gate's reload sees CANCELLED and then overwrites it with AWAITING_HUMAN. The decision was pending at sweep time, so wait_for_decision returns immediately with no resolution; _gate_wait_cancelled re-reads the store, sees the AWAITING_HUMAN the gate itself just wrote, returns False; resolution == "" and "" in _APPROVE_KEYWORDS; the "Approved — resume and advance" branch writes RUNNING, the driver advances, and the next phase mints a fresh cohort.

I verified (b) empirically against the PR's own test harness — driving _run_gate with the store already persisting CANCELLED:

action= break
saved= [<PipelineStatus.AWAITING_HUMAN: 'awaiting_human'>]

The gate persisted AWAITING_HUMAN on top of CANCELLED. The test is green only because assert PipelineStatus.RUNNING not in saved does not look at that write, and because dq.wait_for_decision is a mock that returns instantly rather than the real unbounded poll. See non-blocking 2.

The window is not narrow. _run_phase_execution bails on cancel and returns "break" (_run_pipeline.py:596), so the phase itself is covered — but everything between that return and queue_decision is not: post-phase worktree sync, contract sync, statefile commit + push through the gateway, decision-ledger collection, _read_phase_draft / _read_human_phase_draft. That is seconds to minutes of network git IO on every gated phase transition.

Same shape at five more sites, all in this diff's blast radius:

Site	Queue	Park write	Wait	Guard
Ledger backstop	`_run_hitl_gate.py:192`	`209-214`	`225`	`226`
Phase gate	`484`	`495-502`	`512`	`527`
Follow-up specifics	`615`	—	`626`	`628`
Attestation gate	`_ledger.py:291`	`302-306`	`322`	(post-wait, in caller)
Gap gate	`_ledger.py:931`	`941`	`954`	`967`
Divergence pause	`_alerts.py:240-249` (inside the lock)	`240-247`	`305`	`309`
Contract bridge	`_ledger.py:676-707` (pass 1)	—	`731`	`732`

The bridge is worth calling out separately: pass 1 queues the whole batch, pass 2 waits. A cancel landing before line 678 leaves nothing to sweep, so wait_for_decision at 731 blocks forever and the new cancelled predicate at 732 is never reached. See non-blocking 3 — the docstring describes a different interleaving than the one the predicate actually closes.

Fix. Re-read the status inside the lock that writes AWAITING_HUMAN, and skip both the write and the wait when it is CANCELLED. Because StateStore.update_pipeline holds the same per-pipeline lock (state_store/_crud.py:419-459), that single check closes all three interleavings atomically:

cancel before queue → the in-lock check fires, nothing is queued, nothing hangs;
cancel between queue and lock → the decision was pending so it is swept, and the in-lock check fires before the clobber;
cancel after the lock → the existing post-wait check fires as it does today.

At the main gate that is roughly:

with _pkg.get_pipeline_state_lock(pipeline_id):
    pipeline = store.load_pipeline(pipeline_id)
    if pipeline.status == _pkg.PipelineStatus.CANCELLED:
        return pipeline, "break"
    pipeline.status = _pkg.PipelineStatus.AWAITING_HUMAN
    ...

_alerts.py:240 and _ledger.py:302/941 already reload inside their lock, so it is one condition each. The bridge needs the predicate consulted once before pass 1 as well as after each wait. The gate's create arm additionally wants the check before queue_decision so a cancelled run does not leave an orphan decision in the queue.

Worth stating plainly: the guards that are in this PR are correct and necessary. This is about the other half of the window, not a rework of what landed.

Verified fixed

Traced, not skimmed:

R2-1 — gate reads a cancel as an approval. _gate_wait_cancelled (_run_hitl_gate.py:13-67) at five sites (226, 330, 527, 628, 751), plus cancelled= threaded into the bridge at 733. Dropping the decision's-own-status half relative to what the response comment described is the right call and the docstring earns it: the standalone routes/decisions/_lifecycle.py cancel would over-fire, and a FAILED PATCH sweep would bypass the #1273 carve-out. The residual — a FAILED sweep still reading as an approval — is stated rather than hidden, and test_gate_still_advances_when_only_the_decision_was_cancelled pins the distinction.
"break" propagation. _run_pipeline.py:951-960 maps it to a loop exit alongside the existing "continue"; _run_implement_advance's new (pipeline, action) return is consumed at 907-921. Sole production caller updated; test_advance_phase_thread.py:473 only inspect.getsources the name, so it is unaffected. The return pipeline, "break" at _run_phase_blocks.py:55 sits inside the try but is a return, not an exception, so the except Exception as gap_gate_err below cannot swallow it — and it is placed before the commit + push, so a cancelled run stops mutating the remote branch.
Terminal-phase backstop. _run_pipeline.py:1031-1051. Guarding the branch rather than the block is the right generalisation, and test_terminal_phase_does_not_complete_a_cancelled_pipeline models the bail at _run_implement_advance rather than asserting on the guard's own code.
Divergence-reconcile path. _sync_worktree_reconciling_divergence returns (outcome, True) with outcome bound (_alerts.py:309-325); both callers (_run_pipeline.py:752, _run_pipeline_setup.py:697) route to _fail_pipeline_after_divergence_abort, which is now a status no-op on CANCELLED but still runs pre_event_hook (_alerts.py:131-147) — the overseer teardown is wanted on either exit — and both callers then stop the driver.
UnboundLocalError on the reuse path. phase_label / draft_content hoisted above the branch (_run_hitl_gate.py:350-390); the missing-draft WARNING correctly stays scoped to the create arm with a debug on reuse, and the placeholder is bound on both. test_bare_request_changes_on_a_reused_gate_reaches_the_followup asserts waits == [1, 2], which is the right observable.
_pipeline_cancelled is safe in the new call sites. It returns False on any store exception (_run_support.py:158-164), so none of the new checks can raise into the broad handlers around them.
R2-3/4/5 comment corrections. _run_concurrent.py:309-320 now names integration-branch creation as in-window but not covered, pointing at _run_implement.py:777. Accurate.
BLE001 bound. Recounted: exactly 123 # noqa: BLE001 under routes/pipelines/, matching the <= 123 assertion. The one new site (_run_phase_blocks.py) was already inside the pre-existing handler, so the count is unchanged.
File-size allowlist. _run_pipeline.py 1557 and _run_concurrent.py 1529 against the 1500 hard cap, both allowlisted with #3651 / #3650. Every other touched file is under: _run_hitl_gate.py 888, _ledger.py 1470, _alerts.py 1321.
Targeted tests: 11 gate-related cases in test_cancel_stops_driver.py pass. Full suite left to CI per this workflow's constraint.

Non-blocking

2. The layer-5 test harness cannot express the defect above. _run_gate (test_cancel_stops_driver.py:~700) wires store.load_pipeline.side_effect = lambda _pid: _gate_pipeline(status=cell.status) and store.save_pipeline.side_effect = lambda p, *a, **k: saved.append(p.status) — saves are recorded but never fed back into cell, so the fake store cannot model a lost update, and dq.wait_for_decision is a MagicMock that returns instantly, so it cannot model an unbounded poll. Both of the finding's failure modes are therefore invisible to every test in the section. Two cheap changes would close it: have save_pipeline write back into the _StatusCell, and add assert saved == [] (or an explicit "never persists a non-terminal status over CANCELLED") to test_gate_bails_when_the_cancel_route_cancels_its_decision, whose setup already models "the cancel landed before the gate ran".

3. _queue_and_await_contract_decisions's docstring describes a hazard the predicate does not close, and not the one it does. _ledger.py:562-571 says a cancel "that lands while pass 1 is still queueing this batch leaves the later entries PENDING with nobody to cancel them — without this, the next wait_for_decision would block for the process lifetime." In that interleaving the earlier entries were swept, so wait #1 returns immediately and the predicate fires at 732 — the predicate handles it fine. The case it cannot handle is a cancel landing entirely before line 678, where nothing was queued to sweep and wait #1 never returns. The new test's docstring repeats the same inversion. Worth rewording once the pre-queue check lands.

4. _run_support.py's new docstring makes three claims the code does not support. _run_support.py:101-121: "each of them re-checks from inside, before the write" — true of the RUNNING write, false of the AWAITING_HUMAN write that precedes every wait. "the four _run_hitl_gate.py sites" — there are five (226, 330, 527, 628, 751), and the same docstring says "all five" eleven lines earlier. And _gate_wait_cancelled's own docstring (_run_hitl_gate.py:57-62) — "by the time a swept wait returns the status this reads is already authoritative" — holds only for a swept wait; a decision minted after the sweep is never swept, and the status it reads is the gate's own. These are the load-bearing comments a future reader will trust, so they should not overstate the guarantee.

5. phase_execution.status is left at AWAITING_HUMAN on the divergence-reconcile cancel path. The pause writes both pipeline.status and phase_execution.status to AWAITING_HUMAN (_alerts.py:240-247); the cancel bail at 309 returns before any restore, and _fail_pipeline_after_divergence_abort — which previously normalised the phase box by pinning it FAILED — is now a no-op for CANCELLED. _mark_pipeline_records_terminated (_lifecycle_helpers.py:303-321) only touches container and agent records, not phase status. So a pipeline cancelled during the pause persists as CANCELLED with its phase box still rendering an awaiting-human gate. Cosmetic, and arguably an accurate record of where it stopped — but it is a behaviour change introduced here, so it should be deliberate.

Verification

Targeted only, per this workflow's constraint — the full suite is CI's. 11 gate cases in test_cancel_stops_driver.py pass. _gate_wait_cancelled reproduction run directly against the PR's own _run_gate harness, output quoted above. BLE001 count and file-size caps recomputed independently.

— Authored by egg

Review round 4: every cancel guard added so far runs after wait_for_decision, leaving the queue-and-park sequence in front of it unguarded. Two failures fell out — a decision minted after the cancel route's one-time sweep blocks an untimed poll forever (driver thread leak, no finally, no worktree preservation), and an unconditional AWAITING_HUMAN park clobbers the operator's CANCELLED so the post-wait check reads back the gate's own write. Adds a pre-queue _pipeline_cancelled check at every site that mints a decision, and _park_at_gate_unless_cancelled — a shared helper that re-reads status inside the same per-pipeline lock that performs the park write, which is the lock StateStore.update_pipeline takes. Applied at all seven sites: ledger backstop, phase gate, follow-up specifics, attestation gate, gap gate, divergence pause, contract bridge (checked before pass 1). Also: harness fix so save_pipeline feeds back into the status cell (the lost update was invisible to every prior test), the deliberate phase-box-stays-parked choice documented, the bridge docstring inversion corrected, and _queue_and_await_contract_decisions moved to _contract_bridge.py so _ledger.py stays under the 1500-line cap.

james-in-a-box · 2026-07-27T01:24:42Z

Round-4 review addressed. All five items fixed in-PR; nothing deferred, nothing disputed. Landed on egg/issue-3633-prewait-guards as commit 2965a4b and opened as stacked PR #3657 into issue-3633-cancel-stops-driver (the gateway only accepts egg/-prefixed branches, so I can't push to this PR's branch directly — same route as #3649 and #3655).

1. Blocking — every cancel guard sits after wait_for_decision; the queue-and-park sequence in front of it is unguarded. fixed-in-PR (commit 2965a4b)

You were right on both halves, and the second one is the more embarrassing of the two: the reuse arm reproduces #3633 verbatim through the guard I added for it. The unconditional AWAITING_HUMAN park lands after the operator's CANCELLED write, the post-wait _pipeline_cancelled check then reads back the gate's own write, sees AWAITING_HUMAN, and falls through to the resolution — where "" in _APPROVE_KEYWORDS reads an unset resolution as approval and advances the phase. On the create arm it's the driver-thread leak you described: the cancel route's sweep of dq.get_pending_decisions() at _routes_crud.py:597-638 runs exactly once, so a decision minted after it is never cancelled, and DecisionQueue.wait_for_decision (decision_queue.py:295-321) is a while True 5s poll with no timeout — the thread is gone for the process lifetime, _run_pipeline's finally never runs, so no cleanup and no #1725 worktree preservation.

Two guards, applied at all seven sites you enumerated:

A pre-queue_decision _pipeline_cancelled check, so a cancelled pipeline never mints a decision in the first place.
_park_at_gate_unless_cancelled (_run_support.py), which re-reads the persisted status inside the same get_pipeline_state_lock(pipeline_id) block that performs the park write. That is the lock StateStore.update_pipeline takes (state_store/_crud.py:419-459), which is what makes the check atomic against the cancel route rather than just narrowing the window.

Sites: ledger backstop, phase gate (create arm), follow-up specifics, attestation gate, gap gate, divergence-reconcile pause, and the contract bridge (checked before pass 1). Each bail propagates a stop the driver already acts on — "break", return gated, or aborted=True.

Note on the divergence pause specifically: the park write and the _persist_hitl_decision call have to stay in one lock acquisition, otherwise a reader can observe AWAITING_HUMAN with no pending decision. So that site keeps its inline lock block with the cancel check inside it rather than calling the shared helper, and the _park_cancelled arm is ordered before the decision is None arm — a cancel isn't a persist failure and shouldn't be logged as one.

2. Non-blocking — the _run_gate harness never writes save_pipeline back, so the lost update is invisible. fixed-in-PR (commit 2965a4b)

Correct, and this is why the bug survived a green suite. store.save_pipeline.side_effect now writes the status back into the shared cell, and the harness grew on_draft and dq hooks so a test can land a cancel at a chosen point mid-gate. With the writeback in place, test_gate_bails_when_the_cancel_route_cancels_its_decision gained assert saved == [] and the new test_gate_park_does_not_overwrite_a_cancel_that_lands_before_it fails against the pre-fix code — I reverted each guard in turn to confirm every new test is non-vacuous.

3. Non-blocking — test_bridge_abandons_remaining_waits_when_the_pipeline_is_cancelled docstring inverts what it asserts. fixed-in-PR (commit 2965a4b)

Rewritten. Added test_bridge_never_queues_for_an_already_cancelled_pipeline alongside it, asserting resolved_count == 0 and dq.queued == [].

4. Non-blocking — _park_at_gate_unless_cancelled's in-lock ordering is load-bearing but untested. fixed-in-PR (commit 2965a4b)

test_park_at_gate_unless_cancelled_checks_inside_the_lock asserts the event ordering directly — ["enter", "load", "save", "exit"] for the live case, ["enter", "load", "exit"] for the cancelled one. If the re-read ever drifts outside the lock the ordering breaks and the test fails, which is the property that actually matters here.

5. Non-blocking — the phase box is left at AWAITING_HUMAN on a cancel bail; deliberate or oversight? fixed-in-PR (commit 2965a4b)

Deliberate, now documented rather than implied. Added a paragraph to _gate_wait_cancelled's docstring and a comment at the divergence-reconcile bail: when the guard returns True the park write has already landed, every caller breaks without restoring the phase execution's status, and all gates — gap, attestation, and divergence-reconcile included — behave identically, so a cancelled pipeline renders consistently.

One structural note. The new guards pushed _ledger.py to 1560 lines, past the 1500-line hard cap. Rather than add a third routes/pipelines/ entry to scripts/file-size-allowlist.yaml — a further regression against #3312's terminal "empty allowlist" criterion — I extracted _queue_and_await_contract_decisions into _contract_bridge.py (299 lines) following the canonical sub-package + barrel pattern. _ledger.py is now 1273. No new allowlist entries; BLE001 count is unchanged at 123.

Verification. make lint clean. Targeted tests for the three touched files: 77 passed. Per this workflow's constraints I did not run the full suite — CI has it on the PR HEAD. The 4 failures in test_contract_preserved_across_post_phase_sync.py are pre-existing sandbox git init --bare gateway-policy failures, confirmed by stashing to a clean tree and reproducing them there.

— Authored by egg

Guard the pre-wait window at every HITL gate on cancel

egg-reviewer

Re-review round 5 — delta `ed77cde..HEAD` (10 files, +862/−321)

All five round-4 findings are properly fixed, not papered over. I traced each to the specific lines rather than taking the producer's summary at face value, and re-derived the concurrency argument from the cancel route rather than from the PR's own prose. One new advisory finding and three doc nits below.

Round-4 items — verified

Blocking 1 — the pre-wait window was unguarded at 7 decision-minting sites. FIXED, and the fix is structurally correct, not a sprinkle of checks.

The delta introduces _park_at_gate_unless_cancelled (_run_support.py:161-199), which takes get_pipeline_state_lock, re-reads status inside it, and returns (pipeline, cancelled) without writing on the cancelled arm. That is the right primitive, and the reason it works is verifiable independently of the docstring: _routes_crud.py:597 does store.update_pipeline(...) — which acquires the same per-pipeline lock (state_store/_crud.py:419-459) — and only then, after release, sweeps dq.get_pending_decisions(). So in-lock the interleavings collapse to two: the cancel wins the lock and the park sees CANCELLED, or the park wins and the cancel's subsequent sweep reaches the decision.

I enumerated every queue_decision / wait_for_decision under routes/pipelines/ — 7 sites — and each now carries a pre-queue check, and each that parks uses the new helper:

Site	pre-queue	in-lock park	post-wait
`_run_hitl_gate.py:227` ledger backstop	`:210`	`:248`	`:271`
`_run_hitl_gate.py:544` phase gate	`:536`	`:561`	`:596`
`_run_hitl_gate.py:698` follow-up specifics	`:690`	n/a — gate's park in force	`:711`
`_ledger.py:307` attestation gate	`:299`	`:322`	`:349`
`_ledger.py:720` gap gate	`:711`	`:734`	`:757`
`_contract_bridge.py:171/188` bridge	`:159`	n/a	`:223/:251`
`_alerts.py` divergence pause	in-lock	in-lock	`:352`

The divergence pause (_alerts.py:240-292) is the strongest of the seven and worth calling out: check, park, and _persist_hitl_decision all happen in a single lock acquisition, so it has no check-then-queue gap at all. _park_cancelled and decision are both bound before the with (no UnboundLocalError on the cancelled arm), and the _park_cancelled branch is correctly ordered ahead of the decision is None persist-failure branch — a cancel is not a persist failure and must not route to the fail-closed path.

Propagation is complete, which is the half that's easy to get wrong: the gate sites return (pipeline, "break") → _run_pipeline.py:952-960 exits the loop; the attestation gate returns (False, ledger_note, pipeline) and its caller at _run_hitl_gate.py:350-381 immediately hits _gate_wait_cancelled → "break"; the gap gate returns gated and _run_implement_advance re-reads status → "break" at _run_pipeline.py:917-923; the divergence pause returns aborted=True. No site merely skips its write and falls through.

Non-blocking 2 — the harness could not model a lost update. FIXED. _run_gate's fake now applies saves back onto the _StatusCell (test_cancel_stops_driver.py:750-754). That is exactly what was missing: a fake that only records saves can never reproduce "the park write becomes the persisted status, so the post-wait re-read sees our own AWAITING_HUMAN." With it, test_gate_park_does_not_overwrite_a_cancel_that_lands_before_it (:990) is non-vacuous — it feeds RESOLVED/"approve", i.e. a resolution that would advance if the bail were missed, and asserts saved == []. test_park_at_gate_unless_cancelled_checks_inside_the_lock (:1020) asserts the event sequence ["enter","load","save","exit"] vs ["enter","load","exit"], which pins the actual property (read inside the lock that performs the write) rather than the outcome.

Non-blocking 3 — bridge docstring inverted the failure mode. FIXED (test_contract_decision_bridge.py), plus a new test_bridge_never_queues_for_an_already_cancelled_pipeline asserting resolved_count == 0 and dq.queued == [].

Non-blocking 4 — _pipeline_cancelled docstring overstated its guarantees. FIXED. _run_support.py:102-139 now describes all three checks and corrects "four" → "five" gate sites. Count verified: five _gate_wait_cancelled / bail sites in _run_hitl_gate.py.

Non-blocking 5 — phase box left at AWAITING_HUMAN was undocumented. FIXED, and documented as deliberate in both _gate_wait_cancelled's docstring and _alerts.py:343-353, with the consistency argument spelled out (every gate breaks without restoring its phase box, so a cancelled pipeline uniformly renders the gate it was sitting at).

Move fidelity. _queue_and_await_contract_decisions → _contract_bridge.py is a clean extraction: diffing the moved body against ed77cde:_ledger.py shows only the docstring rewrite and the new pre-check. Barrel updated correctly (__init__.py:1171-1173 adds it, :1252 removes it from ._ledger, :1438 adds _park_at_gate_unless_cancelled). File-size caps satisfied — _ledger.py 1,271 / _contract_bridge.py 298 / _run_hitl_gate.py 971 / _alerts.py 1,359, all under 1,500; scripts/file-size-allowlist.yaml gains no entry. BLE001 noqa count under routes/pipelines/ is exactly 123, matching the bound asserted in test_ble001_narrowing_audit.py:190.

Targeted run: test_cancel_stops_driver.py, test_contract_decision_bridge.py, test_hard_reset_recovery.py, test_ble001_narrowing_audit.py → 81 passed. CI is green on this head (lint, unit, security, docker); integration tests still running.

New — non-blocking

1. The pre-queue checks are the only ones not taken under the state lock, so they narrow the check-then-queue race rather than closing it. _run_hitl_gate.py:690, :536, :210; _ledger.py:299, :711; _contract_bridge.py:159.

_run_support.py:118-121 states the rule this PR is built on:

The park check has to be in-lock rather than merely "just before", because StateStore.update_pipeline — which is how the cancel route persists CANCELLED — takes the same per-pipeline lock. Reading outside it races the cancel and loses the update.

The pre-queue check is exactly the "merely just before" shape that sentence rules out. _pipeline_cancelled (_run_support.py:148-158) does a bare store.load_pipeline with no lock, and queue_decision is a separate critical section. The surviving interleaving: gate reads RUNNING → cancel route update_pipeline commits CANCELLED and releases → cancel sweeps get_pending_decisions(), finds nothing → gate queue_decision mints an orphan the sweep already passed.

Consequences differ by site, and this is why I'm not treating them uniformly:

At the five sites with a subsequent park, _park_at_gate_unless_cancelled catches the cancel and returns "break". The residue is an orphan PENDING decision on a cancelled pipeline — operator-visible noise, no hang.
At _run_hitl_gate.py:690-709 there is no park. The comment says so explicitly: "This site never parks at AWAITING_HUMAN — the gate's park is still in force — so the pre-queue check is the only one it needs." That reasoning holds for the lost-update hazard but not the unsweepable-decision hazard, which is a different failure. If the cancel lands between :690 and :698, dq.wait_for_decision(followup.id) at :709 is a while True poll with no timeout on a decision nothing will ever cancel — the driver thread blocks for the process lifetime, _run_pipeline's finally never runs, no container cleanup, no #1725 CANCELLED-only worktree preservation. That is the precise failure mode this PR exists to eliminate, reachable through the one site that has no second line of defence.

The fix is cheap because DecisionQueue._lock is get_pipeline_state_lock(pipeline_id) (decision_queue.py:76) and it is reentrant, so queue_decision already runs inside it — wrapping check + queue adds no new lock and no new contention:

with _pkg.get_pipeline_state_lock(pipeline_id):
    if _pkg._pipeline_cancelled(store, pipeline_id):
        ...
        return pipeline, "break"
    followup = dq.queue_decision(...)   # re-enters the same RLock

Then either the queue wins the lock (decision is PENDING when the sweep runs → swept → the wait returns → :711 fires) or the cancel wins (the in-lock read sees CANCELLED → skip). Same argument the producer already accepted for the park, applied to the other write.

Why advisory rather than blocking: the trigger is a thread-preemption window between two adjacent statements, which I cannot reproduce in a review, and the current state is strictly better than pre-PR (where the window was the entire gate lifetime). Per the ladder this is CONFIRMED in mechanism, unconfirmed in trigger → downgraded. But it is a two-line change that converts a probabilistic guarantee into a total one, and I'd take it before merge — at minimum at the follow-up site, where the failure is a permanent hang rather than noise.

2. _contract_bridge.py:41-45 asserts a safety property it does not have.

A cancel landing between the pre-pass-1 check and a later queue_decision in pass 1 still mints unsweepable entries — but the first wait is on an entry the sweep did reach (or, if the cancel beat the whole batch, the pre-check fired), so the post-wait check returns before the unsweepable ones are ever waited on.

The parenthetical does not cover the interleaving it claims to. "The cancel beat the whole batch" fires the pre-check only if the cancel commits before :159. If it commits between :159 and the first queue_decision at :171, then queued_decisions[0] is itself unsweepable and dq.wait_for_decision(queued[0].id) at :222 never returns — neither disjunct applies. This is the same race as finding 1, but here the docstring actively tells the next maintainer it's handled, which is worse than silence. Either fix it with the in-lock wrap (which would make the claim true for the whole batch) or state the residual honestly.

3. Doc drift in the orchestrator/CLAUDE.md decomposition table. The delta edits this exact row to add _contract_bridge.py (299) but leaves the neighbouring counts stale: _ledger.py (1,364) is now 1,271, _alerts.py (1,277) is now 1,359 — and this PR is what changed both. _contract_bridge.py is 298, not 299. Small, but the table's whole purpose is telling a future reader how much headroom each module has under the 1,500-line cap, and it currently overstates _ledger.py's consumption and understates _alerts.py's.

4. _gate_pipeline's docstring now contradicts the harness this delta added. test_cancel_stops_driver.py:651-656 says the pending-decision fixture "routes the gate down its existing_pending_gate branch, so the test reaches wait_for_decision without touching draft reads." The draft read was hoisted above the reuse/create branch at _run_hitl_gate.py:405-412 (round 2, to fix the UnboundLocalError on the bare-request-changes path), so the reuse arm does read the draft now — which is precisely why on_draft works as a cancel hook in test_gate_park_does_not_overwrite_a_cancel_that_lands_before_it. One stale clause.

5. Coverage gap (minor). The new pre-queue checks are directly tested at two of six sites — the phase gate (test_gate_never_mints_a_decision_for_a_cancelled_pipeline) and the bridge (test_bridge_never_queues_for_an_already_cancelled_pipeline). The ledger backstop, attestation gate, gap gate, and follow-up specifics are covered only for their post-wait bails: test_gate_bails_when_cancelled_at_the_followup_specifics cancels on wait 2, exercising :711, never :690. _run_gate already has the on_draft/dq hooks needed; a queue_decision.call_args_list == [] assertion per site would be cheap. Not blocking — the shared helper is unit-tested and the sites are structurally identical — but the pre-queue checks are the layer with the least direct coverage and the one finding 1 says is weakest.

Verdict

Approving. The round-4 blocking finding is genuinely closed: the park is atomic against the cancel route by construction, every one of the seven blocking-wait sites is guarded at both writes, every guarded site propagates a stop the driver acts on, and the tests assert the ordering property rather than just the outcome. Findings 1 and 2 are a real residual race with a two-line fix; I'd land them, but they narrow an already-narrowed window rather than reopening the regression, so they don't gate the merge.

— Authored by egg

Layer 4's guard read ``_pipeline_cancelled``, so it caught the cancel but not the other way this loop outlives its ownership of the phase. On ``restart_phase`` the epoch bumps and a new ``_run_pipeline`` thread takes over, but the old slice loop kept ticking: admit a slice, create its integration branch, call the phase runner, have that bail on supersession (#3315), record a spurious slice failure, repeat — branch churn and false FAILED slices racing the restarted run. Both conditions are the same question ("does this loop still own the phase?") and ``_phase_bail_reason_impl`` already answers both from one pipeline load, so the guard now calls that instead. ``run_epoch`` is already threaded in from ``_run_phase.py``; the epoch arm stays dormant on the direct-call paths that pass None. The cancel arm is unchanged in behaviour — only the log line and the aggregate-log marker now name which condition fired.

egg-reviewer

Re-review round 6 — delta `ccd7c3c..HEAD` (1 commit, 2 files, +25/−14)

The delta is a single commit, 3ec18e2 "Stop the slice loop when a restart supersedes it too (#3633)", touching _run_implement.py and test_cancel_stops_driver.py. It generalises layer 4's top-of-tick guard from _pipeline_cancelled to _phase_bail_reason_impl, so the slice loop now stops on supersession as well as on cancel.

The generalisation is right and the reasoning behind it is right. But it stops the second occurrence of the harm its own commit message names and leaves the first one — which, in the restart case, is the one that actually happens. Reproduced below.

Round-5 scope note. None of the five round-5 findings are touched by this delta. Verified: _contract_bridge.py:41-45 carries the same paragraph verbatim, _run_hitl_gate.py:690 still does its pre-queue check outside the state lock, orchestrator/CLAUDE.md:281 still reads _ledger.py (1,364) / _alerts.py (1,277) / _contract_bridge.py (299) against actual 1,271 / 1,359 / 298. They were approved as non-blocking and stay non-blocking; I am not re-litigating them, only recording that they are still open so they are not lost.

Blocking

1. The post-slice carve-out is still cancel-only, so a restart during an in-flight wave records a spurious slice FAILED — the exact harm this commit says it fixes

orchestrator/routes/pipelines/_run_implement.py:929

The commit's thesis, in its own new comment at :535-556, is that cancel and supersession are one question answered by one re-read:

a restart bumped run_epoch (#3315), so a new _run_pipeline thread owns the pipeline. The stale loop would otherwise race it: admit a slice, create its integration branch, call the phase runner, have that bail on supersession, record a spurious slice failure, repeat.

The guard added here is at the top of the tick, so it only prevents the repeat. The slice that was already in flight when the epoch bumped still lands at :919, and the branch that decides whether a non-zero exit is a real failure was not generalised:

if exit_code_inner != 0:
    if _pkg._pipeline_cancelled(store, pipeline_id):   # ← cancel only
        ...
        return exit_code_inner, logs_inner
    scheduler.record_failure(slice_id)

On a restart the pipeline is RUNNING, not CANCELLED (_routes_restart.py:1044-1046 writes pipeline.status = RUNNING and pipeline.run_epoch = now() in the same lock block), so _pipeline_cancelled is False and the fall-through fires.

Full chain, each link verified in the tree:

restart_phase bumps run_epoch mid-wave — this is the documented in-flight case, not a corner: _routes_restart.py:1030 says "bump run_epoch so any lingering old _run_pipeline thread detects the restart and exits (see #1638)".
The slice's _run_concurrent_phase poll loop bails and returns (1, "Phase monitor thread exited: superseded_by_restart.") (_run_concurrent.py:483-517).
_run_concurrent_phase_with_impasse_retry passes that non-zero through unchanged (_run_concurrent_retry.py:143-154 — and note it deliberately declines to escalate on supersession, for the stated reason that this thread "no longer owns the phase").
_run_implement.py:929 — cancel-only check, False.
scheduler.record_failure(slice_id) sets SchedulerSliceState.FAILED, arms _pending_cascades[slice_id], and calls _emit_slice_closed(slice_id, SLICE_OUTCOME_FAILED) synchronously, outside the lock (slice_scheduler.py:408-428).
That emitter is wired in production at _run_implement.py:89-95 and publishes EventType.SLICE_CLOSED with data={"slice_id": …, "outcome": "failed"} to the event bus (_run_implement_support.py:395-417), plus a "Slice failed" WARNING at :938.

So the operator restarts a phase and the SSE/overseer surface reports a failed slice for the run they just restarted. That is verbatim the harm the carve-out's own comment at :920-928 was written to prevent — "recording that as a failure would set the slice FAILED, arm the downstream cascade, and publish SLICE_CLOSED(outcome="failed") to the bus — so operators and SSE consumers would see a failed slice" — applied to the other half of the same predicate.

Two details in the PR's favour, stated so the severity is not overstated: the armed cascade does not fire, because poll_cascades() runs on the same tick inside the 60 s grace window and the next tick breaks on the new guard, and the scheduler is in-memory so a restart discards _pending_cascades. The SLICE_CLOSED(failed) bus event, the scheduler's FAILED state, and the WARNING are immediate and unconditional.

Reproduced. Against the PR HEAD, driving _run_implement_phase_slices with the tick-0 guard seeing the owning epoch and the post-slice re-read seeing a bumped one — the mid-wave restart:

MODE cancel  EXIT 1   FAILURES RECORDED: []          ← carve-out fires
MODE restart EXIT 1   FAILURES RECORDED: ['slice-3'] ← carve-out misses
WARNING  orchestrator.pipelines:_run_implement.py:938 Slice failed

Same harness, one parameter changed, so the asymmetry is the variable under test. CONFIRMED per the ladder.

Fix, and it is the same shape as the one this commit just applied one level up:

_slice_bail = _pkg._phase_bail_reason_impl(
    store=store, pipeline_id=pipeline_id, run_epoch=run_epoch
)
if _slice_bail is not None:
    ...
    return exit_code_inner, logs_inner
scheduler.record_failure(slice_id)

run_epoch is already in scope here — it is threaded to _run_concurrent_phase_with_impasse_retry thirteen lines above at :916. I applied exactly this locally and both parametrisations pass; source reverted, working tree clean.

Worth noting that the new test's own docstring (test_cancel_stops_driver.py:576-580) enumerates the harm as "admitting a slice, creating its integration branch, calling the phase runner, having that bail on supersession, and recording a spurious slice failure, once per tick" — and then asserts only on the admission half. The test agrees with this finding about what the defect is; it just does not cover the part of it that survives.

Non-blocking

2. _phase_bail_reason_impl's docstring enumerates its callers and this delta makes the list wrong. _run_concurrent_support.py:24-29 says the epoch comparison lives here because "this helper's callers — the _run_concurrent_phase poll loop and the slice-path impasse-retry wrapper — both now reach through here". The layer-4 slice loop is now a third caller, and the "both" phrasing reads as an exhaustive list. This is the docstring a future reader consults to know where the predicate is in force, and it now understates that reach by one — which matters, because the finding above is precisely about a site that should be a fourth caller and is not.

3. The new test asserts a strict subset of what its sibling asserts, and the two cases are structurally identical. test_slice_loop_stops_when_a_restart_supersedes_it (:625-629) checks exit_code, the log marker, iter_ready_calls, spawned, and create_slice_integration_branch.call_count. Its cancel-arm sibling (:566-572) checks all five plus spawner.spawn_agent_job.call_count == 0 and reconciler_stop.is_set(). The supersession test even constructs the reconciler threading.Event() inline at :607 and then never looks at it. "No agent was spawned" is the assertion closest to the user-visible harm, and there is no reason for it to be present on one arm and absent on the other. Two lines.

4. The control test's inline comment is now stale. :659-661 says the production-faithful run_epoch is "not the epoch arm under test — all_done() short-circuits before the retry wrapper". That was true when the only epoch check on this path was inside the retry wrapper. As of this commit the layer-4 guard evaluates the epoch arm at the top of every tick, before all_done() is consulted again — the control is now the reason a matching epoch does not spuriously stop a healthy run, which is a stronger property than the comment claims for it.

Verified correct — traced, not assumed

The new exit-1 does not become a phase FAILURE. _run_phase.py:272-298 loads the pipeline once on non-zero exit and returns action "return" for the epoch mismatch (:277-284, #1638) as well as for CANCELLED (:292-298, #3633). So the supersession bail is a clean thread return, and the #1725 CANCELLED-only worktree preservation is untouched. I checked this specifically because a new non-zero exit path is the obvious way to accidentally rewrite CANCELLED to FAILED.
run_epoch threading. _run_pipeline.py:56 binds run_epoch = pipeline.run_epoch or pipeline.created_at (non-None in production), _run_phase.py:212-224 is the sole production caller of _run_implement_phase_slices and passes it through. The epoch arm stays dormant on direct-call paths that pass None, which _phase_bail_reason_impl's run_epoch is not None guard makes explicit.
Nothing was dropped from the replaced guard. Diffing the old block against the new: same unfinished comprehension, same overall_exit = 1, same break, same aggregate_logs append. Only the message text and the added reason changed.
The log-marker rename is safe. "pipeline cancelled" → "pipeline_cancelled" in the aggregate_logs line has exactly one producer (_run_implement.py:566) and one consumer (the test's substring assert). Grepped for the old string across orchestrator/, shared/, and gateway/ — no other reader.
No teardown stomp on the supersession arm. The finally at :1479 sets reconciler_stop, but that reconciler is this loop's own thread started at :89-95, not one shared with the new owner, so stopping it is correct. This is the mirror of _run_concurrent.py:483-517's deliberate decision not to stop containers on superseded_by_restart, and the asymmetry holds up: there the containers belong to the new owner, here the thread belongs to the stale loop.
The new test uses the real enum. _StubScheduler.list_slices returns SchedulerSliceState.READY, not a string, so the guard's rt.state != SchedulerSliceState.COMPLETE comprehension is exercised against what production yields. The stale epoch is persisted - 1h, which is the correct direction.

Verification

Targeted only, per this workflow's constraint — the full suite is CI's.

Reproduction of finding 1 run against the PR HEAD, output quoted above; the proposed fix applied locally makes both parametrisations pass; source reverted and git status clean.
test_slice_loop_stops_when_a_restart_supersedes_it, test_slice_loop_admits_nothing_after_a_cancel, test_slice_loop_keeps_running_while_the_pipeline_is_running, test_phase_bail_reason_still_reports_supersession → 4 passed.
Round-5 carry-forward status re-checked directly against the files rather than inferred from the absence of a diff.

Verdict

Requesting changes on finding 1. The delta closes the tick-N+1 half of the supersession race and leaves the tick-N half open at a line thirteen rows below the one it changed, using the same predicate it just replaced one level up. The failure it produces — a FAILED slice and a SLICE_CLOSED(outcome="failed") on the operator's bus for a phase they deliberately restarted — is the one this commit's message and its test's docstring both name as the thing being fixed. It is a three-line change with run_epoch already in scope, and it is reproducible today rather than dependent on a thread-preemption window, which is why this is blocking rather than advisory.

Findings 2–4 are cheap and I would take them in the same push, but they do not gate.

— Authored by egg

james-in-a-box · 2026-07-27T04:52:54Z

egg review completed. View run logs

15 previous review(s) hidden.

james-in-a-box · 2026-07-27T04:53:20Z

Automated feedback loop has reached the maximum of 5 rounds. Human review is needed to make further progress on this PR.

egg-reviewer Bot suggested changes Jul 26, 2026

View reviewed changes