diff --git a/orchestrator/CLAUDE.md b/orchestrator/CLAUDE.md index 1a3145f69..f374edcd0 100644 --- a/orchestrator/CLAUDE.md +++ b/orchestrator/CLAUDE.md @@ -278,7 +278,7 @@ Pure refactor, no behaviour change: the 27 method bodies are AST-identical to th | Prompt building: `_prompt_phase.py` (1,407), `_prompt_agent.py` (1,346), `_prompt_review.py` (765), `_prompt_reviewer.py` (593) | Phase-prompt, agent-prompt, review-prompt and reviewer-prep prompt assembly | `build_phase_prompt`, `build_agent_prompt`, `build_review_prompt`, `build_reviewer_prompt` | | Readers / synthesis: `_criteria.py` (961), `_drafts.py` (758), `_reviews.py` (186), `_context_pr.py` (1,220), `_brc_history.py` (961), `_populate.py` (1,460) | Review-criteria builders, draft-path + source-branch-artifact readers, review-verdict readers, context-PR composition, BRC-history readers, plan-draft synthesis + contract population | `build_review_criteria`, `read_draft_path`, `read_review_verdicts`, `compose_context_pr`, `populate_contract` | | State / lifecycle: `_slice_state.py` (1,094), `_statefiles.py` (613), `_worktree_sync.py` (1,393), `_slice_completion.py` (135), `_lifecycle_helpers.py` (339), `_status_view.py` (391), `_status_wait.py` (147) | Slice-DAG state helpers, statefile read/write, worktree-sync, slice-completion, lifecycle helpers, status view + long-poll wait | `slice_state`, `sync_worktree`, `complete_slice`, `status_view`, `wait_for_status` | -| Decisions / overseer: `_ledger.py` (1,364), `_decision_candidates.py` (159), `_decisions.py` (259), `_resolve.py` (196), `_hitl_rerun.py` (337), `_overseer.py` (740), `_alerts.py` (1,277), `_pod_liveness.py` (228), `_first_principles.py` (247) | Decision-ledger + gap-gate + apply-handoff, considered-candidate rendering + refine→plan deferral handoff + ledger-summary persistence (#3526), HITL + divergence-reconcile decisions, decision resolution, HITL rerun, overseer detection-plane, divergence/alert/timeout emission, live-pod guarding, first-principles review seed | `register_decision`, `resolve_decision`, `rerun_hitl`, `detect_divergence`, `guard_live_pods` | +| Decisions / overseer: `_ledger.py` (1,364), `_contract_bridge.py` (299), `_decision_candidates.py` (159), `_decisions.py` (259), `_resolve.py` (196), `_hitl_rerun.py` (337), `_overseer.py` (740), `_alerts.py` (1,277), `_pod_liveness.py` (228), `_first_principles.py` (247) | Decision-ledger + gap-gate + apply-handoff, the contract-decision bridge (#1889 — split out of `_ledger.py` to stay under the file-size cap), considered-candidate rendering + refine→plan deferral handoff + ledger-summary persistence (#3526), HITL + divergence-reconcile decisions, decision resolution, HITL rerun, overseer detection-plane, divergence/alert/timeout emission, live-pod guarding, first-principles review seed | `register_decision`, `_queue_and_await_contract_decisions`, `resolve_decision`, `rerun_hitl`, `detect_divergence`, `guard_live_pods` | | PR / drivers / salvage: `_drivers.py` (263), `_stacked_pr.py` (251), `_salvage.py` (71) | Pipeline-driver lifecycle helpers, stacked-PR assembly, agent-output salvage | `pipeline_drivers`, `build_stacked_pr`, `salvage_agent_output` | Pure refactor, no behaviour change: every route handler body, helper, and constant is AST-identical to the pre-split file (modulo the sanctioned `_pkg.`-prefixing and the decorator relocation onto thin wrappers). Patch seams preserved: the 16 `@pipelines_bp.route` decorators stay in the barrel so the URL rule → handler map registers identically; the private submodules reach the test-patched module globals via `import routes.pipelines as _pkg`, and the barrel re-exports every externally-referenced symbol across the dominant back-compat import surface (~137 referencing files repo-wide; the audited ~57 distinct `patch("routes.pipelines.")` targets), so both `from routes.pipelines import X` and `patch("routes.pipelines.X")` resolve unchanged — the existing dense seam coverage (`test_consensus_polling`, `test_brc_nack_iteration`, `test_concurrent_*`, `test_advance_phase_*`) stays green. `_run_pipeline` becomes a thin loop delegating to per-phase handlers with no transition-ordering change. **Packaging-neutral:** `orchestrator/routes/` is already shipped by the recursive `COPY orchestrator/routes/ ./routes/` (Dockerfile:45), so the new submodules are auto-included — no Dockerfile change. `pipelines.py`'s allowlist entry — the **LAST** in the program — is dropped, so `scripts/file-size-allowlist.yaml`'s `files:` map is now **EMPTY**: the terminal acceptance criterion of the file-size decomposition program, closing #3312. diff --git a/orchestrator/event_loop/__init__.py b/orchestrator/event_loop/__init__.py index be96dd629..820009806 100644 --- a/orchestrator/event_loop/__init__.py +++ b/orchestrator/event_loop/__init__.py @@ -372,10 +372,13 @@ class EventDecision: ``timing`` is a structured mapping for the slice-4 latency budget on a fresh spawn, ``None`` otherwise. ``blocked`` (#3496) names why a spawn-action decision did not spawn when the block is operator-relevant: - ``"exhausted"`` (terminal retry-budget exhaustion) or ``"parked"`` - (#3548 — a no-op-park that only the retry heartbeat will release), so - the all-arms wedge detections can distinguish them from the benign - not-spawned shapes (dedupe, backoff). + ``"exhausted"`` (terminal retry-budget exhaustion), ``"parked"`` + (#3548 — a no-op-park that only the retry heartbeat will release), or + ``"stopped"`` (#3633 — the loop was stopped mid-tick, typically by a + cancel), so the all-arms wedge detections can distinguish them from the + benign not-spawned shapes (dedupe, backoff). Only ``"exhausted"`` and + ``"parked"`` count toward a wedge; ``"stopped"`` is a teardown, not a + condition an operator can resolve. """ role: str diff --git a/orchestrator/event_loop/_loop.py b/orchestrator/event_loop/_loop.py index 14d225887..f870e399c 100644 --- a/orchestrator/event_loop/_loop.py +++ b/orchestrator/event_loop/_loop.py @@ -616,6 +616,22 @@ def _handle_role(self, role: str) -> EventDecision: # wait / unknown — nothing to spawn. return EventDecision(role=role, action=action) + # #3633: never spawn once the loop has been stopped. ``run()`` checks the + # stop event between ticks, but ``stop()`` is also called from outside the + # loop's own thread — the cancel route stops every live loop for a + # pipeline synchronously — so a stop that lands mid-tick would otherwise + # still get one final cohort of one-shot Jobs out the door. Re-checking + # immediately before the spawn decision closes that window. + if self._stop.is_set(): + logger.info( + "event-loop: spawn blocked, loop is stopping", + pipeline_id=self.pipeline_id, + slice_id=self.slice_id, + role=role, + action=action, + ) + return EventDecision(role=role, action=action, spawned=False, blocked="stopped") + identity = _pkg.event_identity(action, payload) key = _pkg.compute_dedupe_key( self.pipeline_id, self.slice_id, self.phase, role, action, identity diff --git a/orchestrator/routes/decisions/_handlers.py b/orchestrator/routes/decisions/_handlers.py index e0ef497a7..e144eae21 100644 --- a/orchestrator/routes/decisions/_handlers.py +++ b/orchestrator/routes/decisions/_handlers.py @@ -1060,11 +1060,19 @@ def _read_redirect_seed_from_contract(pipeline_id: str, decision_id: Any) -> str def _cancel_pipeline_in_process(pipeline_id: str, *, reason: str) -> None: - """Cancel a pipeline from a resolution hook (status CANCELLED + cleanup). + """Cancel a pipeline from a resolution hook (status CANCELLED + loop stop). Mirrors the inline cancel pattern used elsewhere: flip status under the - pipeline state lock, emit ``PIPELINE_CANCELLED``, and cancel pending - decisions so any ``wait_for_decision`` unblocks. + pipeline state lock, stop the pipeline's live BRC event loops, emit + ``PIPELINE_CANCELLED``, and cancel pending decisions so any + ``wait_for_decision`` unblocks. + + This is the second place CANCELLED originates (the PATCH route is the + first), so it needs the same #3633 loop teardown: without it the live + loops keep deriving arms and requesting one-shot Jobs, and only the + driver loops' own CANCELLED re-reads — a poll interval later — would + catch it. Note this hook does NOT tear down containers; the driver's + cancel bail and the operator's own cleanup own that half. """ from models import PipelineStatus from state_store import get_pipeline_state_lock @@ -1077,6 +1085,19 @@ def _cancel_pipeline_in_process(pipeline_id: str, *, reason: str) -> None: pipeline.error = reason store.update_pipeline(pipeline_id, pipeline.model_dump(mode="json")) + try: + # Deferred import: routes.pipelines is too heavy to bind at module + # import time (same reason as the other hooks in this file). + from routes.pipelines import _stop_pipeline_event_loops + + _stop_pipeline_event_loops(pipeline_id, reason="pipeline_cancelled") + except Exception: + logger.warning( + "Failed to stop BRC event loops after first-principles cancel", + pipeline_id=pipeline_id, + exc_info=True, + ) + try: _pkg.emit_event( EventType.PIPELINE_CANCELLED, diff --git a/orchestrator/routes/pipelines/__init__.py b/orchestrator/routes/pipelines/__init__.py index 829ccd96c..aec437505 100644 --- a/orchestrator/routes/pipelines/__init__.py +++ b/orchestrator/routes/pipelines/__init__.py @@ -1168,6 +1168,9 @@ def stream_pipeline(pipeline_id: str) -> Response: _refresh_context_pr_body, _repos_with_slices, ) +from ._contract_bridge import ( # noqa: E402,F401 + _queue_and_await_contract_decisions, +) from ._criteria import ( # noqa: E402,F401 _get_agent_design_criteria, _get_code_review_criteria, @@ -1246,7 +1249,6 @@ def stream_pipeline(pipeline_id: str) -> Response: _ledger_attestation_rerun_directive, _next_phases_for_epic, _persist_phase_gate_resolution, - _queue_and_await_contract_decisions, _sync_pipeline_decisions_to_contract, _unwrap_choice_resolution, _write_apply_phase_handoff, @@ -1258,6 +1260,7 @@ def stream_pipeline(pipeline_id: str) -> Response: _compute_gateway_mode, _mark_pipeline_records_terminated, _normalize_submission_repos, + _stop_pipeline_event_loops, _sync_contract_phase_to_pipeline, ) from ._overseer import ( # noqa: E402,F401 @@ -1382,14 +1385,15 @@ def stream_pipeline(pipeline_id: str) -> Response: ) from ._run_concurrent_support import ( # noqa: E402,F401 _latest_proposal_ts_impl, + _phase_bail_reason_impl, _record_container_exit_impl, _record_spawned_agents_impl, _retry_transient_spawn_failures_impl, _stop_running_containers_impl, - _superseded_by_restart_impl, _update_agents_complete_impl, ) from ._run_hitl_gate import ( # noqa: E402,F401 + _gate_wait_cancelled, _run_hitl_gate_converge, ) from ._run_implement import ( # noqa: E402,F401 @@ -1431,8 +1435,9 @@ def stream_pipeline(pipeline_id: str) -> Response: ) from ._run_support import ( # noqa: E402,F401 _clear_stale_impasses_for_producers, + _park_at_gate_unless_cancelled, _parse_resolution, - _pipeline_superseded_by_restart, + _pipeline_cancelled, _spawn_and_wait, ) from ._salvage import ( # noqa: E402,F401 diff --git a/orchestrator/routes/pipelines/_alerts.py b/orchestrator/routes/pipelines/_alerts.py index 2bb70e20c..79df05bd5 100644 --- a/orchestrator/routes/pipelines/_alerts.py +++ b/orchestrator/routes/pipelines/_alerts.py @@ -111,6 +111,10 @@ def _fail_pipeline_after_divergence_abort( ``pre_event_hook`` runs after the FAILED-write but before the public ``pipeline.failed`` broadcast (the post-phase site uses it to tear down the per-phase overseer container). + + On an already-CANCELLED pipeline the status write and the broadcast are + both skipped — ``pre_event_hook`` still runs — so an operator cancel that + unblocked the reconcile pause is not rewritten as a failure (#3633). """ phase_label = phase.value if phase is not None else "current phase" reason = ( @@ -124,6 +128,25 @@ def _fail_pipeline_after_divergence_abort( f"preserved under {backup_ref or '(backup ref write failed)'} " f"({len(local_only_commit_shas)} commit(s))." ) + # ``_sync_worktree_reconciling_divergence`` also returns ``aborted=True`` + # when what unblocked its ``wait_for_decision`` was the operator + # cancelling the pipeline (#3633). That is a stop, not a failure: the + # FAILED write below would overwrite the persisted CANCELLED every driver + # work loop keys on, and the ``pipeline.failed`` broadcast would report a + # failure the operator never caused. Run ``pre_event_hook`` anyway — the + # per-phase overseer teardown it carries is wanted on either exit — then + # return, leaving CANCELLED intact for the caller to stop on. + if _pkg._pipeline_cancelled(store, pipeline_id): + _pkg.logger.info( + "Divergence reconcile ended on an operator cancel — leaving the " + "persisted CANCELLED intact instead of pinning FAILED (#3633)", + pipeline_id=pipeline_id, + phase=phase_label, + ) + if pre_event_hook is not None: + pre_event_hook() + return + with _pkg.get_pipeline_state_lock(pipeline_id): pipeline = store.load_pipeline(pipeline_id) if phase is not None: @@ -173,11 +196,13 @@ def _sync_worktree_reconciling_divergence( nothing discarded. Returns ``(outcome, aborted)``. ``aborted`` is True when the operator - chose "Abort pipeline" or the reconcile-pause budget was exhausted; the - caller should fail the pipeline via - :func:`_fail_pipeline_after_divergence_abort`. When ``aborted`` is - False the worktree is reconciled (or never diverged) and the caller - proceeds normally. + chose "Abort pipeline", the reconcile-pause budget was exhausted, or the + operator cancelled the pipeline while this was blocked on the pause + (#3633); the caller should stop driving the phase via + :func:`_fail_pipeline_after_divergence_abort`, which pins FAILED for the + first two and is a status no-op for the third so the persisted CANCELLED + survives. When ``aborted`` is False the worktree is reconciled (or never + diverged) and the caller proceeds normally. Only call this from inside the ``_run_pipeline`` loop thread, which is allowed to block; route handlers that cannot block use @@ -212,30 +237,56 @@ def _sync_worktree_reconciling_divergence( # Persist the reconcile HITL and flip to AWAITING_HUMAN under the # (reentrant) state lock so a reader never sees AWAITING_HUMAN # without the pending decision. + # + # The status is re-read from inside that lock and a cancel short- + # circuits both writes. Two things go wrong otherwise: the park + # clobbers the operator's CANCELLED (so the post-wait check below + # reads back our own AWAITING_HUMAN and resumes the run), and the HITL + # is minted after the cancel route's one-time pending sweep, leaving + # ``wait_for_decision`` — an unbounded poll — blocking for the process + # lifetime (#3633 review round 3). The state lock is what makes this + # airtight: ``StateStore.update_pipeline``, the cancel route's + # persistence path, takes the same one. + _park_cancelled = False + decision = None with _pkg.get_pipeline_state_lock(pipeline_id): pipeline = store.load_pipeline(pipeline_id) - pipeline.status = PipelineStatus.AWAITING_HUMAN - if phase is not None: - phase_execution = pipeline.get_phase_execution(phase) - if phase_execution is not None: - phase_execution.status = PipelineStatus.AWAITING_HUMAN - store.save_pipeline(pipeline) - decision = _pkg._persist_hitl_decision( - pipeline_id, - pipeline, - store, - question=_pkg._divergence_reconcile_hitl_question( - pipeline_id=pipeline_id, + if pipeline.status == PipelineStatus.CANCELLED: + _park_cancelled = True + else: + pipeline.status = PipelineStatus.AWAITING_HUMAN + if phase is not None: + phase_execution = pipeline.get_phase_execution(phase) + if phase_execution is not None: + phase_execution.status = PipelineStatus.AWAITING_HUMAN + store.save_pipeline(pipeline) + decision = _pkg._persist_hitl_decision( + pipeline_id, + pipeline, + store, + question=_pkg._divergence_reconcile_hitl_question( + pipeline_id=pipeline_id, + phase=phase, + backup_ref=outcome.backup_ref, + local_only_commit_shas=outcome.local_only_commit_shas, + rebase_category=outcome.rebase_category, + rebase_detail=outcome.rebase_detail, + ), + options=list(_pkg._DIVERGENCE_RECONCILE_HITL_OPTIONS), phase=phase, - backup_ref=outcome.backup_ref, - local_only_commit_shas=outcome.local_only_commit_shas, - rebase_category=outcome.rebase_category, - rebase_detail=outcome.rebase_detail, - ), - options=list(_pkg._DIVERGENCE_RECONCILE_HITL_OPTIONS), - phase=phase, - context=_pkg._DIVERGENCE_RECONCILE_HITL_CONTEXT, + context=_pkg._DIVERGENCE_RECONCILE_HITL_CONTEXT, + ) + if _park_cancelled: + # Checked before the ``decision is None`` arm below: nothing was + # persisted on this path, and a cancel is not a persist failure. + _pkg.logger.info( + "Divergence reconcile pause: pipeline cancelled before the " + "pause was persisted — leaving the persisted CANCELLED " + "intact (#3633)", + pipeline_id=pipeline_id, + phase=phase_label, ) + return outcome, True if decision is None: # Could not persist the HITL — fail closed rather than spin on # a pause the operator can never see. @@ -280,6 +331,37 @@ def _sync_worktree_reconciling_divergence( dq.wait_for_decision(decision.id) + # The wait also returns when the operator cancels the pipeline — + # the cancel route sweeps every pending decision, this one + # included, with no resolution. Bail before the RUNNING write + # below: restoring RUNNING would overwrite the persisted + # CANCELLED the driver keys on and re-admit the run the operator + # just stopped (#3633). ``aborted=True`` is how the two callers + # spell "stop driving this phase"; the FAILED pin they route to + # is suppressed on a cancelled pipeline in + # ``_fail_pipeline_after_divergence_abort``. + # + # The phase box is deliberately left at AWAITING_HUMAN. Once the + # park above has landed, restoring it would mean a second write to + # a pipeline the operator has already stopped, and the only status + # that would be honest to write is the one already on the pipeline + # record. Leaving it records *where* the run stopped — parked at + # the reconcile gate — which is what an operator reading the phase + # timeline of a cancelled pipeline wants. This is uniform with + # every other gate: ``_gate_wait_cancelled`` and the gap / + # attestation gates all break without restoring their phase box + # either, so a cancelled pipeline consistently renders the gate it + # was sitting at (#3633 review round 4). + if _pkg._pipeline_cancelled(store, pipeline_id): + _pkg.logger.info( + "Divergence reconcile pause: pipeline cancelled while " + "awaiting the operator — leaving the persisted CANCELLED " + "intact (#3633)", + pipeline_id=pipeline_id, + phase=phase_label, + ) + return outcome, True + resolved = dq.get_decision(decision.id) resolution = (resolved.resolution or "") if resolved is not None else "" if _pkg._divergence_reconcile_is_abort(resolution): diff --git a/orchestrator/routes/pipelines/_contract_bridge.py b/orchestrator/routes/pipelines/_contract_bridge.py new file mode 100644 index 000000000..8471dc184 --- /dev/null +++ b/orchestrator/routes/pipelines/_contract_bridge.py @@ -0,0 +1,298 @@ +"""Contract-decision bridge for routes/pipelines (#1889). + +Split out of ``_ledger.py`` to keep it under the 1,500-line file-size cap +(#3312). Barrel-resident and test-patched globals are reached via ``_pkg`` so +``patch("routes.pipelines.")`` keeps intercepting. +""" + +from __future__ import annotations + +import routes.pipelines as _pkg # noqa: E402,F401 + + +def _queue_and_await_contract_decisions( + dq: _pkg.Any, + worktree_repo_path: _pkg.Path, + pipeline_id: str, + pipeline_identifier: int | str, + phase: _pkg.PipelinePhase, + cancelled: _pkg.Callable[[], bool] | None = None, +) -> int: + """Promote unresolved contract decisions/feedback into the orchestrator queue. + + ``cancelled`` is an optional predicate the caller supplies to answer "has + the operator cancelled this pipeline?". It is consulted at two points, and + the two close different interleavings: + + - **Before pass 1**, because the cancel route's pending-decision sweep is + a one-time snapshot. A cancel that lands entirely *before* this batch is + queued has nothing to sweep, so every decision below is minted with + nobody to cancel it and the very first ``wait_for_decision`` — an + unbounded poll with no timeout — blocks for the process lifetime, + leaking the driver thread and skipping ``_run_pipeline``'s ``finally`` + (#3633 review round 3). The post-wait check cannot help here: it is + never reached. + - **After each blocking wait**, which covers the cancel that lands once + some of the batch is already pending. Those entries *are* swept, so + their waits return immediately with no resolution; the check stops the + remaining waits rather than letting the loop walk a cancelled run's + questions. + + 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. + + Callers must still re-check the cancel themselves once this returns: the + early return is deliberately indistinguishable from a zero-resolution + round. + + Returns the number of contract decisions/feedback this call surfaced and + the operator *resolved* this round — the converge-before-advance signal + (#3392). Decisions that were surfaced but came back non-RESOLVED (e.g. the + operator cancelled them) are **not** counted: the contract ``cq-N`` stays + open, and counting it would re-run the phase, re-surface the still-open + question (carry-forward only adopts *resolved* questions), and loop with no + termination now that the force-advance backstop is gone. A non-zero count + means the operator just answered something, so the caller re-runs the + phase to fold the resolutions into the documents; a zero count means the + round resolved nothing new and the caller may advance. + + + Agents register architectural questions via ``egg-contract add-decision`` + and ``add-feedback``. Those writes only touch ``.egg-state/contracts/ + {identifier}.json`` — the orchestrator's decision queue never sees them, + so approving the phase_gate silently drops the questions and the next + phase's agents have to guess (issue #1889). + + This helper bridges contract-scoped questions for the current phase into + the orchestrator queue after phase_gate approval, so HTTP/MCP callers + (e.g. the ``/sdlc`` skill's Phase 4 handler) surface them as individual + ``choice`` / ``feedback`` decisions. Resolutions are written back to + the contract so implement-phase agents see the human's answers. + + All pending decisions (plus the feedback entry, if any) are queued up + front before any ``wait_for_decision`` call, so ``get_status`` surfaces + them as a single batch. Callers can then prompt for up to 4 at a time + and submit answers in parallel, collapsing what was previously N prompts + and N polling cycles into ~⌈N/4⌉ prompts and one cycle (issue #1956). + + Once the batch is queued, a single ``decision.created`` event is + published to the EventBus so event-driven watchers (the ``wait-status`` + monitor long-polling ``/status/wait``) wake immediately. + ``DecisionQueue.queue_decision`` itself emits no event, so without this + the bridged decisions are created silently and the operator only + discovers them via a manual ``get_status`` (issue #2770). + """ + try: + from egg_contracts.loader import load_contract, save_contract + except ImportError: + _pkg.logger.warning( + "egg_contracts not available, skipping contract decision bridge", + pipeline_id=pipeline_id, + ) + return 0 + + try: + contract = load_contract(pipeline_identifier, worktree_repo_path) + except Exception as e: + _pkg.logger.debug( + "Contract not loadable, skipping contract decision bridge", + pipeline_id=pipeline_id, + error=str(e), + ) + return 0 + + phase_value = phase.value + pending_decisions = [ + d + for d in contract.decisions + if not d.resolved + and getattr(d.type, "value", d.type) == "hitl" + and (d.phase is None or getattr(d.phase, "value", d.phase) == phase_value) + ] + fb = contract.feedback + pending_feedback = None + if fb is not None and not fb.submitted: + fb_phase_val = getattr(fb.phase, "value", fb.phase) if fb.phase is not None else None + if fb_phase_val is None or fb_phase_val == phase_value: + pending_feedback = fb + + if not pending_decisions and pending_feedback is None: + return 0 + + _pkg.logger.info( + "Bridging contract decisions/feedback into orchestrator queue", + pipeline_id=pipeline_id, + phase=phase_value, + decision_count=len(pending_decisions), + has_feedback=pending_feedback is not None, + ) + + def _save_contract_update(mutator: _pkg.Callable[[_pkg.Any], bool]) -> None: + try: + latest = load_contract(pipeline_identifier, worktree_repo_path) + except Exception as e: + _pkg.logger.warning( + "Could not reload contract to persist bridged resolution", + pipeline_id=pipeline_id, + error=str(e), + ) + return + if not mutator(latest): + return + try: + save_contract(latest, worktree_repo_path) + except Exception as e: + _pkg.logger.warning( + "Failed to save contract after bridged resolution", + pipeline_id=pipeline_id, + error=str(e), + ) + + # A cancel that landed before pass 1 left nothing in the queue to sweep, + # so every decision minted below would be unsweepable and the first wait + # in pass 2 would never return. Check once here, before anything is + # queued — the post-wait checks in pass 2 cannot reach that interleaving + # (#3633 review round 3). + if cancelled is not None and cancelled(): + _pkg.logger.info( + "Contract decision bridge skipped: pipeline cancelled before queueing (#3633)", + pipeline_id=pipeline_id, + phase=phase_value, + ) + return 0 + + # Pass 1: queue every pending decision + feedback up front. + queued_decisions: list[tuple[str, _pkg.Any]] = [] + for contract_decision in pending_decisions: + options_labels = [opt.label for opt in contract_decision.options] + queued = dq.queue_decision( + question=contract_decision.question, + context=( + f"Open contract question {contract_decision.id}, " + f"registered by an agent during the {phase_value} phase." + ), + options=options_labels, + decision_type="choice", + phase=phase, + ) + queued_decisions.append((contract_decision.id, queued)) + + queued_feedback: _pkg.HITLDecision | None = None + if pending_feedback is not None: + questions_payload = [ + {"id": q.id, "question": q.question, "answer": ""} for q in pending_feedback.questions + ] + queued_feedback = dq.queue_decision( + question=f"Open feedback request {pending_feedback.id}", + context=( + f"Open contract feedback {pending_feedback.id}, " + f"registered by an agent during the {phase_value} phase." + ), + options=[], + decision_type="feedback", + questions=questions_payload, + phase=phase, + ) + + # Surface the freshly-queued batch to event-driven watchers before + # blocking on resolution. ``DecisionQueue.queue_decision`` emits no + # EventBus event, so without this the bridged decisions are created + # silently — the operator's ``wait-status`` monitor never wakes and + # only finds them via a manual ``get_status`` (#2770). The phase_gate + # decision emits ``decision.created`` the same way. + if _pkg._emit_event is not None: + _pkg._emit_event( + _pkg.EventType.DECISION_CREATED, + pipeline_id, + data={"phase": phase_value}, + ) + + # Pass 2: wait for each to resolve and persist back to the contract. + # Count only decisions whose queue resolution was RESOLVED — a + # CANCELLED / non-resolved outcome leaves the contract ``cq-N`` open and + # must NOT count toward the convergence signal, or the caller would re-run + # the phase, re-surface the still-open question (carry-forward only adopts + # *resolved* questions), and loop without the operator ever being able to + # break out (#3392 review). + resolved_count = 0 + for contract_id, queued in queued_decisions: + resolved = dq.wait_for_decision(queued.id) + if cancelled is not None and cancelled(): + _pkg.logger.info( + "Contract decision bridge abandoned: pipeline cancelled (#3633)", + pipeline_id=pipeline_id, + phase=phase_value, + resolved_count=resolved_count, + ) + return resolved_count + if resolved.status != _pkg.DecisionStatus.RESOLVED: + continue + resolved_count += 1 + resolution_str = (resolved.resolution or "").strip() + + def _apply(latest: _pkg.Any, _cd_id: str = contract_id, _res: str = resolution_str) -> bool: + for d in latest.decisions: + if d.id == _cd_id: + d.resolved = True + d.resolution = _res + d.resolved_by = "human" + d.resolved_at = _pkg.datetime.now(_pkg.UTC) + return True + return False + + _save_contract_update(_apply) + + feedback_resolved = False + if queued_feedback is not None and pending_feedback is not None: + resolved = dq.wait_for_decision(queued_feedback.id) + if cancelled is not None and cancelled(): + _pkg.logger.info( + "Contract feedback bridge abandoned: pipeline cancelled (#3633)", + pipeline_id=pipeline_id, + phase=phase_value, + resolved_count=resolved_count, + ) + return resolved_count + if resolved.status == _pkg.DecisionStatus.RESOLVED: + feedback_resolved = True + answers: dict[str, str] = {} + try: + payload = _pkg.json.loads(resolved.resolution or "") + if isinstance(payload, dict): + raw_answers = payload.get("answers") + if isinstance(raw_answers, dict): + answers = {str(k): str(v) for k, v in raw_answers.items()} + except _pkg.json.JSONDecodeError, TypeError: + pass + + fb_id = pending_feedback.id + + def _apply_fb( + latest: _pkg.Any, _fb_id: str = fb_id, _answers: dict[str, str] = answers + ) -> bool: + if latest.feedback is None or latest.feedback.id != _fb_id: + return False + for q in latest.feedback.questions: + if q.id in _answers: + q.answer = _answers[q.id] + # Always mark submitted after resolution — even if + # individual answers didn't parse, the human responded + # and shouldn't be asked again. + latest.feedback.submitted = True + latest.feedback.submitted_by = "human" + latest.feedback.submitted_at = _pkg.datetime.now(_pkg.UTC) + return True + + _save_contract_update(_apply_fb) + + # Convergence signal (#3392): the number of decisions + feedback this + # round the operator actually *resolved* (not merely surfaced). Non-zero ⇒ + # the operator answered something ⇒ caller re-runs the phase to fold the + # resolutions in. A surfaced-but-cancelled decision is deliberately + # excluded: counting it would re-run the phase, re-surface the still-open + # question, and loop indefinitely now that the force-advance backstop is + # gone. + return resolved_count + (1 if feedback_resolved else 0) diff --git a/orchestrator/routes/pipelines/_ledger.py b/orchestrator/routes/pipelines/_ledger.py index 98304eeff..b36c5051d 100644 --- a/orchestrator/routes/pipelines/_ledger.py +++ b/orchestrator/routes/pipelines/_ledger.py @@ -242,8 +242,10 @@ def _handle_explicit_none_attestation_gate( - ``rerun_requested`` — True when the operator rejected the attestation and the phase has already been re-run here; the caller must ``continue`` its poll loop. False when the attestation was confirmed (or fail-open on - a cancelled/non-RESOLVED terminal state); the caller proceeds to the - phase gate. + a cancelled/non-RESOLVED terminal state, or on an operator cancel + detected *before* the decision was queued or parked); the caller + proceeds to the phase gate — where its own ``_gate_wait_cancelled`` + check turns the cancelled case into a driver break. - ``ledger_note`` — the note to thread into the phase_gate question, annotated with the confirmation outcome. - ``pipeline`` — the (possibly reloaded) pipeline the caller must rebind, @@ -288,6 +290,20 @@ def _handle_explicit_none_attestation_gate( attest_decision = pending_attest newly_created = False else: + # Never mint a decision for a cancelled run: the cancel route's + # pending-decision sweep is a one-time snapshot, so a decision created + # after it has nobody to cancel it and ``wait_for_decision`` below — + # an unbounded poll — would block for the process lifetime (#3633 + # review round 3). Fail open to the caller, which re-checks the cancel + # immediately on return and breaks the driver loop. + if _pkg._pipeline_cancelled(store, pipeline_id): + _pkg.logger.info( + "Pipeline cancelled before the explicit-none attestation was " + "queued — skipping the gate (#3633)", + pipeline_id=pipeline_id, + phase=current_phase.value, + ) + return False, ledger_note, pipeline attest_decision = dq.queue_decision( question=attest_question, context=ledger_note, @@ -299,12 +315,21 @@ def _handle_explicit_none_attestation_gate( phase=current_phase, ) newly_created = True - with _pkg.get_pipeline_state_lock(pipeline_id): - pipeline = store.load_pipeline(pipeline_id) - pipeline.status = _pkg.PipelineStatus.AWAITING_HUMAN - phase_execution = pipeline.get_phase_execution(current_phase) - phase_execution.status = _pkg.PipelineStatus.AWAITING_HUMAN - store.save_pipeline(pipeline) + # Park under the state lock, re-reading the status from inside it — the + # cancel route persists CANCELLED under the same lock, so an unconditional + # write here would lose it and the caller's post-return check would read + # back this gate's own AWAITING_HUMAN (#3633 review round 3). + pipeline, _park_cancelled = _pkg._park_at_gate_unless_cancelled( + store, pipeline_id, current_phase + ) + if _park_cancelled: + _pkg.logger.info( + "Pipeline cancelled before parking at the explicit-none " + "attestation — skipping the gate (#3633)", + pipeline_id=pipeline_id, + phase=current_phase.value, + ) + return False, ledger_note, pipeline # Only announce a freshly-created decision. Reusing a pending decision # across polls must not re-emit ``decision.created`` — a duplicate event # for a decision the operator is already looking at (#3462 review). @@ -549,236 +574,6 @@ def _collect_decision_ledger_status( ) -def _queue_and_await_contract_decisions( - dq: _pkg.Any, - worktree_repo_path: _pkg.Path, - pipeline_id: str, - pipeline_identifier: int | str, - phase: _pkg.PipelinePhase, -) -> int: - """Promote unresolved contract decisions/feedback into the orchestrator queue. - - Returns the number of contract decisions/feedback this call surfaced and - the operator *resolved* this round — the converge-before-advance signal - (#3392). Decisions that were surfaced but came back non-RESOLVED (e.g. the - operator cancelled them) are **not** counted: the contract ``cq-N`` stays - open, and counting it would re-run the phase, re-surface the still-open - question (carry-forward only adopts *resolved* questions), and loop with no - termination now that the force-advance backstop is gone. A non-zero count - means the operator just answered something, so the caller re-runs the - phase to fold the resolutions into the documents; a zero count means the - round resolved nothing new and the caller may advance. - - - Agents register architectural questions via ``egg-contract add-decision`` - and ``add-feedback``. Those writes only touch ``.egg-state/contracts/ - {identifier}.json`` — the orchestrator's decision queue never sees them, - so approving the phase_gate silently drops the questions and the next - phase's agents have to guess (issue #1889). - - This helper bridges contract-scoped questions for the current phase into - the orchestrator queue after phase_gate approval, so HTTP/MCP callers - (e.g. the ``/sdlc`` skill's Phase 4 handler) surface them as individual - ``choice`` / ``feedback`` decisions. Resolutions are written back to - the contract so implement-phase agents see the human's answers. - - All pending decisions (plus the feedback entry, if any) are queued up - front before any ``wait_for_decision`` call, so ``get_status`` surfaces - them as a single batch. Callers can then prompt for up to 4 at a time - and submit answers in parallel, collapsing what was previously N prompts - and N polling cycles into ~⌈N/4⌉ prompts and one cycle (issue #1956). - - Once the batch is queued, a single ``decision.created`` event is - published to the EventBus so event-driven watchers (the ``wait-status`` - monitor long-polling ``/status/wait``) wake immediately. - ``DecisionQueue.queue_decision`` itself emits no event, so without this - the bridged decisions are created silently and the operator only - discovers them via a manual ``get_status`` (issue #2770). - """ - try: - from egg_contracts.loader import load_contract, save_contract - except ImportError: - _pkg.logger.warning( - "egg_contracts not available, skipping contract decision bridge", - pipeline_id=pipeline_id, - ) - return 0 - - try: - contract = load_contract(pipeline_identifier, worktree_repo_path) - except Exception as e: - _pkg.logger.debug( - "Contract not loadable, skipping contract decision bridge", - pipeline_id=pipeline_id, - error=str(e), - ) - return 0 - - phase_value = phase.value - pending_decisions = [ - d - for d in contract.decisions - if not d.resolved - and getattr(d.type, "value", d.type) == "hitl" - and (d.phase is None or getattr(d.phase, "value", d.phase) == phase_value) - ] - fb = contract.feedback - pending_feedback = None - if fb is not None and not fb.submitted: - fb_phase_val = getattr(fb.phase, "value", fb.phase) if fb.phase is not None else None - if fb_phase_val is None or fb_phase_val == phase_value: - pending_feedback = fb - - if not pending_decisions and pending_feedback is None: - return 0 - - _pkg.logger.info( - "Bridging contract decisions/feedback into orchestrator queue", - pipeline_id=pipeline_id, - phase=phase_value, - decision_count=len(pending_decisions), - has_feedback=pending_feedback is not None, - ) - - def _save_contract_update(mutator: _pkg.Callable[[_pkg.Any], bool]) -> None: - try: - latest = load_contract(pipeline_identifier, worktree_repo_path) - except Exception as e: - _pkg.logger.warning( - "Could not reload contract to persist bridged resolution", - pipeline_id=pipeline_id, - error=str(e), - ) - return - if not mutator(latest): - return - try: - save_contract(latest, worktree_repo_path) - except Exception as e: - _pkg.logger.warning( - "Failed to save contract after bridged resolution", - pipeline_id=pipeline_id, - error=str(e), - ) - - # Pass 1: queue every pending decision + feedback up front. - queued_decisions: list[tuple[str, _pkg.Any]] = [] - for contract_decision in pending_decisions: - options_labels = [opt.label for opt in contract_decision.options] - queued = dq.queue_decision( - question=contract_decision.question, - context=( - f"Open contract question {contract_decision.id}, " - f"registered by an agent during the {phase_value} phase." - ), - options=options_labels, - decision_type="choice", - phase=phase, - ) - queued_decisions.append((contract_decision.id, queued)) - - queued_feedback: _pkg.HITLDecision | None = None - if pending_feedback is not None: - questions_payload = [ - {"id": q.id, "question": q.question, "answer": ""} for q in pending_feedback.questions - ] - queued_feedback = dq.queue_decision( - question=f"Open feedback request {pending_feedback.id}", - context=( - f"Open contract feedback {pending_feedback.id}, " - f"registered by an agent during the {phase_value} phase." - ), - options=[], - decision_type="feedback", - questions=questions_payload, - phase=phase, - ) - - # Surface the freshly-queued batch to event-driven watchers before - # blocking on resolution. ``DecisionQueue.queue_decision`` emits no - # EventBus event, so without this the bridged decisions are created - # silently — the operator's ``wait-status`` monitor never wakes and - # only finds them via a manual ``get_status`` (#2770). The phase_gate - # decision emits ``decision.created`` the same way. - if _pkg._emit_event is not None: - _pkg._emit_event( - _pkg.EventType.DECISION_CREATED, - pipeline_id, - data={"phase": phase_value}, - ) - - # Pass 2: wait for each to resolve and persist back to the contract. - # Count only decisions whose queue resolution was RESOLVED — a - # CANCELLED / non-resolved outcome leaves the contract ``cq-N`` open and - # must NOT count toward the convergence signal, or the caller would re-run - # the phase, re-surface the still-open question (carry-forward only adopts - # *resolved* questions), and loop without the operator ever being able to - # break out (#3392 review). - resolved_count = 0 - for contract_id, queued in queued_decisions: - resolved = dq.wait_for_decision(queued.id) - if resolved.status != _pkg.DecisionStatus.RESOLVED: - continue - resolved_count += 1 - resolution_str = (resolved.resolution or "").strip() - - def _apply(latest: _pkg.Any, _cd_id: str = contract_id, _res: str = resolution_str) -> bool: - for d in latest.decisions: - if d.id == _cd_id: - d.resolved = True - d.resolution = _res - d.resolved_by = "human" - d.resolved_at = _pkg.datetime.now(_pkg.UTC) - return True - return False - - _save_contract_update(_apply) - - feedback_resolved = False - if queued_feedback is not None and pending_feedback is not None: - resolved = dq.wait_for_decision(queued_feedback.id) - if resolved.status == _pkg.DecisionStatus.RESOLVED: - feedback_resolved = True - answers: dict[str, str] = {} - try: - payload = _pkg.json.loads(resolved.resolution or "") - if isinstance(payload, dict): - raw_answers = payload.get("answers") - if isinstance(raw_answers, dict): - answers = {str(k): str(v) for k, v in raw_answers.items()} - except _pkg.json.JSONDecodeError, TypeError: - pass - - fb_id = pending_feedback.id - - def _apply_fb( - latest: _pkg.Any, _fb_id: str = fb_id, _answers: dict[str, str] = answers - ) -> bool: - if latest.feedback is None or latest.feedback.id != _fb_id: - return False - for q in latest.feedback.questions: - if q.id in _answers: - q.answer = _answers[q.id] - # Always mark submitted after resolution — even if - # individual answers didn't parse, the human responded - # and shouldn't be asked again. - latest.feedback.submitted = True - latest.feedback.submitted_by = "human" - latest.feedback.submitted_at = _pkg.datetime.now(_pkg.UTC) - return True - - _save_contract_update(_apply_fb) - - # Convergence signal (#3392): the number of decisions + feedback this - # round the operator actually *resolved* (not merely surfaced). Non-zero ⇒ - # the operator answered something ⇒ caller re-runs the phase to fold the - # resolutions in. A surfaced-but-cancelled decision is deliberately - # excluded: counting it would re-run the phase, re-surface the still-open - # question, and loop indefinitely now that the force-advance backstop is - # gone. - return resolved_count + (1 if feedback_resolved else 0) - - def _await_unresolved_gap_gate( store: _pkg.Any, pipeline_id: str, @@ -869,6 +664,12 @@ def _set_status(status: _pkg.PipelineStatus) -> _pkg.Pipeline: # Mirror the phase_gate block: drive both pipeline and the phase # box so the DAG visualization renders the gate on the right # phase, and the operator's wait-status monitor wakes. + # + # Only the RUNNING restore goes through here, and only after the + # post-wait cancel check has cleared. The AWAITING_HUMAN park uses + # ``_park_at_gate_unless_cancelled`` instead, which re-reads the + # status from inside this same lock so it cannot overwrite an + # operator cancel (#3633 review round 3). with _pkg.get_pipeline_state_lock(pipeline_id): pipeline = store.load_pipeline(pipeline_id) pipeline.status = status @@ -900,6 +701,22 @@ def _set_status(status: _pkg.PipelineStatus) -> _pkg.Pipeline: "and fails CI (test_models_gaps.py) red on the PR.\n\n" f"{gap_lines}" ) + # Never mint a decision for a cancelled run. The cancel route sweeps + # the pending queue once; a decision created after that sweep has + # nobody to cancel it, and ``wait_for_decision`` below is an unbounded + # poll — the driver thread would block for the process lifetime and + # ``_run_pipeline``'s ``finally`` would never run (#3633 review round + # 3). ``gated`` is already True here, which is the same value an + # ordinary gating returns, so ``_run_implement_advance``'s own cancel + # re-check is what stops the driver. + if _pkg._pipeline_cancelled(store, pipeline_id): + _pkg.logger.info( + "Unresolved-gap gate: pipeline cancelled before the gate was " + "queued — leaving the persisted CANCELLED intact (#3633)", + pipeline_id=pipeline_id, + phase=phase.value, + ) + return gated decision = dq.queue_decision( question=question, context=context, @@ -909,8 +726,20 @@ def _set_status(status: _pkg.PipelineStatus) -> _pkg.Pipeline: ) # Mark AWAITING_HUMAN + surface to event watchers, mirroring the - # phase_gate block so the operator's wait-status monitor wakes. - pipeline = _set_status(_pkg.PipelineStatus.AWAITING_HUMAN) + # phase_gate block so the operator's wait-status monitor wakes — but + # only if the operator has not cancelled since the check above. The + # re-read happens inside the same state lock as the write (the cancel + # route persists CANCELLED under it), so the park cannot clobber the + # cancel that the post-wait check below is trying to read. + pipeline, _park_cancelled = _pkg._park_at_gate_unless_cancelled(store, pipeline_id, phase) + if _park_cancelled: + _pkg.logger.info( + "Unresolved-gap gate: pipeline cancelled before parking at " + "the gate — leaving the persisted CANCELLED intact (#3633)", + pipeline_id=pipeline_id, + phase=phase.value, + ) + return gated _pkg.report_pipeline_status( pipeline, event_type="phase.gap_gate", @@ -924,6 +753,27 @@ def _set_status(status: _pkg.PipelineStatus) -> _pkg.Pipeline: ) resolved = dq.wait_for_decision(decision.id) + # ...unless what unblocked the wait was the operator cancelling the + # pipeline, which sweeps this decision to CANCELLED. Restoring RUNNING + # here would overwrite the persisted CANCELLED that every driver work + # loop keys on, re-admitting the run the operator just stopped + # (#3633 review). Bail before the status write, not after it. + # + # Skipping the write is necessary but not sufficient: ``gated`` is the + # same value an ordinary gating returns, so the *caller* has to re-read + # the cancel and stop the driver. ``_run_implement_advance`` does, and + # must keep doing so — IMPLEMENT is terminal, so a driver that merely + # falls through here writes COMPLETE over the CANCELLED this bail just + # preserved (#3633 review round 2). + if _pkg._pipeline_cancelled(store, pipeline_id): + _pkg.logger.info( + "Unresolved-gap gate: pipeline cancelled while awaiting the " + "operator — leaving the persisted CANCELLED intact (#3633)", + pipeline_id=pipeline_id, + phase=phase.value, + ) + return gated + # Restore RUNNING now the gate cleared (re-set to AWAITING_HUMAN # above on the next loop if gaps remain). _set_status(_pkg.PipelineStatus.RUNNING) diff --git a/orchestrator/routes/pipelines/_lifecycle_helpers.py b/orchestrator/routes/pipelines/_lifecycle_helpers.py index bab80d3c4..54d012871 100644 --- a/orchestrator/routes/pipelines/_lifecycle_helpers.py +++ b/orchestrator/routes/pipelines/_lifecycle_helpers.py @@ -155,6 +155,77 @@ def _assert_repo_set_uniform(repos: list[str]) -> str | None: return None +def _stop_pipeline_event_loops(pipeline_id: str, *, reason: str) -> int: + """Stop every live orchestrator-owned BRC event loop for ``pipeline_id`` (#3633). + + ``cancel_task`` used to set the status to CANCELLED, tear down the + pipeline's containers, and clear its runtime state — but nothing stopped + the thing that *creates* containers. Each slice's ``OrchestratorEventLoop`` + kept polling on its daemon thread, so the next tick re-derived every + role's arm and requested fresh one-shot Jobs. Killing the pods removed + the symptom while the spawner ran on: ``issue-3596-v2`` was cancelled at + 20:48Z and spawned slice-3 agents at 22:55Z, against a pipeline the + operator believed was stopped. + + The loops are reachable through the ``event_loop`` live-loop registry + (#3496), which is keyed by ``(pipeline_id, slice_id)`` and populated by + ``start()`` — so this covers every concurrent slice of the run. + + Call this BEFORE container cleanup: cleanup that races a live loop is + removing pods the loop is still entitled to replace. + + ``stop()`` is called with ``join_timeout=0.0`` because this runs in the + PATCH request thread. Both effects that matter — setting the loop's stop + event and evicting it from the registry — are synchronous, so the daemon + thread winds down on its own without making the operator wait on it. + + Returns the number of loops signalled (0 when none were live, which is + the normal case for a pipeline cancelled between phases). + """ + try: + try: + from event_loop import get_live_event_loops + except ImportError: + from ..event_loop import ( # type: ignore[no-redef] + get_live_event_loops, + ) + except ImportError as import_err: + # A silent 0 here would turn cancel back into #3633 with no signal at + # all: the operator's cancel would report success while every live + # loop kept spawning. Log loudly so an import regression is visible. + _pkg.logger.warning( + "Cannot reach the live event-loop registry; cancelled pipeline's " + "BRC event loops were NOT stopped (they will keep spawning until " + "the driver loops' own CANCELLED re-read catches up)", + pipeline_id=pipeline_id, + reason=reason, + error=str(import_err), + ) + return 0 + + stopped = 0 + for loop in get_live_event_loops(pipeline_id): + try: + loop.stop(join_timeout=0.0) + stopped += 1 + except Exception as e: # noqa: BLE001 — best-effort teardown + _pkg.logger.warning( + "Failed to stop BRC event loop", + pipeline_id=pipeline_id, + slice_id=getattr(loop, "slice_id", None), + reason=reason, + error=str(e), + ) + if stopped: + _pkg.logger.info( + "Stopped live BRC event loops", + pipeline_id=pipeline_id, + reason=reason, + loops_stopped=stopped, + ) + return stopped + + def _clear_pipeline_runtime_state(pipeline_id: str, *, reason: str) -> None: """Evict per-pipeline runtime state that is keyed by pipeline_id alone. diff --git a/orchestrator/routes/pipelines/_routes_crud.py b/orchestrator/routes/pipelines/_routes_crud.py index c113698d1..7ca68509d 100644 --- a/orchestrator/routes/pipelines/_routes_crud.py +++ b/orchestrator/routes/pipelines/_routes_crud.py @@ -610,6 +610,24 @@ def _update_pipeline_body(pipeline_id: str) -> tuple[_pkg.Response, int]: ): _pkg._emit_pipeline_event(pipeline, "pipeline.cancelled") + # Then stop the machinery that spawns agents, BEFORE tearing down + # the agents it spawned (#3633). Cancel used to do only the + # teardown, so the live BRC event loops kept deriving arms and + # requesting one-shot Jobs against a pipeline the operator believed + # was stopped — and the container cleanup below raced loops that + # were still entitled to spawn replacements. Ordering it ahead of + # the cleanup closes that race. + # + # Shares the CANCELLED-*transition* gate above: an idempotent + # re-cancel has nothing live left to stop, and FAILED is excluded + # deliberately — ``container_monitor`` reconciliation can mark a + # live pipeline FAILED mid-phase, and the poll loop recovers it to + # RUNNING once consensus completes (#1273). Tearing the loop down + # there would convert that recoverable transient into a permanently + # idle pipeline. The driver loops' own CANCELLED re-reads are the + # backstop for a loop that started after this ran. + _pkg._stop_pipeline_event_loops(pipeline_id, reason="pipeline_cancelled") + # If pipeline is being cancelled or failed, clean up containers # and cancel any pending decisions so wait_for_decision() unblocks. if pipeline.status in (_pkg.PipelineStatus.CANCELLED, _pkg.PipelineStatus.FAILED): diff --git a/orchestrator/routes/pipelines/_run_concurrent.py b/orchestrator/routes/pipelines/_run_concurrent.py index 3a684c824..c5dcaa8e6 100644 --- a/orchestrator/routes/pipelines/_run_concurrent.py +++ b/orchestrator/routes/pipelines/_run_concurrent.py @@ -306,6 +306,29 @@ def _live_pipeline_phase() -> str: event_status_view=event_status_view, ) + # Last chance to notice a cancel before minting a cohort (#3633 review). + # The slice loop's guard runs at the top of its tick; everything between + # there and here — contract load, per-role prompt building (draft reads, + # BRC history, git diffs), gateway session + worktree setup — takes tens + # of seconds. (Integration-branch creation is in that window too, but it + # happens back in ``_run_implement`` before this function is called, so + # this guard cannot prevent it — only the slice loop's own guard can.) + # A cancel landing in that window + # runs the route's teardown BEFORE these Jobs exist, so nothing would reap + # them: no reconciler acts on CANCELLED, and ``cleanup_pipeline`` only + # re-runs on an operator DELETE. Re-read the status here so the cohort is + # never minted rather than minted-and-orphaned. FAILED is excluded for the + # same #1273 reason as every other layer. + if _pkg._pipeline_cancelled(store, pipeline_id): + _pkg.logger.info( + "Pipeline cancelled before agent spawn — no cohort minted", + pipeline_id=pipeline_id, + phase=phase, + slice_id=slice_id, + ) + executor.stop_event_loop() + return 1, "Phase monitor thread exited: pipeline_cancelled." + # Spawn all agents with their prompts. executions = executor.spawn_all(agent_prompts=agent_prompts) @@ -397,21 +420,19 @@ def _live_pipeline_phase() -> str: start_time = _pkg.time.monotonic() objection_decision_created = False - # ``run_epoch`` is the authoritative epoch the owning ``_run_pipeline`` - # thread captured at start (#1638). The poll loop uses it to detect a - # ``restart_phase`` (or any restart that bumps ``run_epoch``) that - # superseded this thread (#3315). ``start_time`` is a fresh monotonic - # clock per call, but a parked-then-restarted phase leaves the *old* - # ``_run_concurrent_phase`` thread alive in its poll loop with a - # ``start_time`` from the original phase start; once its ``elapsed`` - # crosses ``consensus_timeout`` it would fire a spurious consensus-timeout - # OVERSEER_ALERT + HITL decision against the freshly-restarted phase. The - # new ``_run_pipeline`` thread owns the pipeline now, so this stale thread - # must bail before escalating. When ``run_epoch`` is not supplied (legacy - # / direct-call callers) the guard is dormant — behaviour is unchanged. - - _superseded_by_restart = _pkg.functools.partial( - _pkg._superseded_by_restart_impl, + # Per-tick "should this thread still be here?" check. Two conditions, + # one pipeline load — see ``_phase_bail_reason_impl`` for the full + # rationale of each and for why FAILED is excluded: + # + # * a restart bumped ``run_epoch``, so a new ``_run_pipeline`` thread + # owns the pipeline and this stale one must stop before it fires a + # spurious consensus-timeout escalation against the fresh phase + # (#3315; dormant when ``run_epoch`` is not supplied); + # * the operator cancelled the run, so this thread must stop driving + # the slice DAG rather than keep spawning into it (#3633). + + _phase_bail_reason = _pkg.functools.partial( + _pkg._phase_bail_reason_impl, store=store, pipeline_id=pipeline_id, run_epoch=run_epoch, @@ -465,26 +486,35 @@ def _live_pipeline_phase() -> str: driver_heartbeat.record_tick(pipeline_id) elapsed = _pkg.time.monotonic() - start_time - # 0. Bail if a restart superseded this thread (#3315). A parked phase - # that is restarted after the consensus-timeout budget elapsed - # leaves this old thread polling with a stale ``start_time``; the - # new ``_run_pipeline`` thread already owns the pipeline. Exit - # cleanly — stop this executor's event loop so it stops requesting - # one-shot spawns — WITHOUT firing the timeout escalation. Return a - # NON-zero exit so the caller never mistakes this for success and - # advances the phase; the post-return epoch check (#1638) at the - # call site re-confirms the restart and exits the old thread without - # marking the phase FAILED. - if _superseded_by_restart(): + # 0. Bail if this thread no longer owns the phase (restart, #3315) or + # the run was cancelled (#3633). Exit cleanly — stop this executor's + # event loop so it stops requesting one-shot spawns — WITHOUT firing + # the timeout escalation. Return a NON-zero exit so the caller never + # mistakes this for success and advances the phase; the post-return + # checks at the call site re-confirm the reason and exit the thread + # without marking the phase FAILED. + _bail_reason = _phase_bail_reason() + if _bail_reason is not None: _pkg.logger.info( - "Phase superseded by restart (run_epoch changed) — exiting stale " - "_run_concurrent_phase thread without escalation", + "Exiting _run_concurrent_phase poll loop without escalation", pipeline_id=pipeline_id, phase=phase, slice_id=slice_id, + reason=_bail_reason, ) executor.stop_event_loop() - return 1, "Phase superseded by restart; stale monitor thread exited." + # On a cancel, this thread is the last owner of the cohort it + # spawned — reap it like every consensus exit path in this + # function does (#3633 review). A cohort minted after the route's + # ``cleanup_pipeline`` ran has nothing else to stop it: no + # reconciler acts on CANCELLED, and cleanup only re-runs on an + # operator DELETE. Deliberately NOT done on the + # ``superseded_by_restart`` branch — there the new + # ``_run_pipeline`` thread legitimately owns those containers and + # stopping them would kill the restarted run's agents (#3315). + if _bail_reason == "pipeline_cancelled": + _stop_running_containers() + return 1, f"Phase monitor thread exited: {_bail_reason}." # 1. Check consensus try: diff --git a/orchestrator/routes/pipelines/_run_concurrent_retry.py b/orchestrator/routes/pipelines/_run_concurrent_retry.py index 3262d3c3b..1a6450860 100644 --- a/orchestrator/routes/pipelines/_run_concurrent_retry.py +++ b/orchestrator/routes/pipelines/_run_concurrent_retry.py @@ -132,18 +132,24 @@ def _run_concurrent_phase_with_impasse_retry( # Defense-in-depth (#3315 facet a, slice path): if a restart bumped # ``run_epoch`` while this thread was running, a stale producer-written # impasse file could otherwise drive ``route_impasses`` into a HITL - # against the freshly-restarted phase. The poll loop in - # ``_run_concurrent_phase`` already bails on supersession before any - # escalation; mirror that here so the "no escalation when superseded" - # property holds on the slice path too — return the (superseded) result - # without routing. - if _pkg._pipeline_superseded_by_restart(store, pipeline_id, run_epoch): + # against the freshly-restarted phase. The same applies to a cancel + # (#3633): the phase call above returns immediately once the pipeline + # is CANCELLED, and without this guard a stale impasse would escalate + # a HITL — and, on the all-delegated branch, drive another retry + # iteration — against a run the operator already stopped. The poll loop + # in ``_run_concurrent_phase`` bails on both conditions before any + # escalation; mirror that here so the "no escalation once this thread + # no longer owns the phase" property holds on the slice path too. + _bail_reason = _pkg._phase_bail_reason_impl( + store=store, pipeline_id=pipeline_id, run_epoch=run_epoch + ) + if _bail_reason is not None: _pkg.logger.info( - "Restart superseded this thread before impasse routing; " - "skipping route_impasses to avoid escalating against a " - "freshly-restarted phase", + "Thread no longer owns this phase before impasse routing; " + "skipping route_impasses to avoid escalating against it", pipeline_id=pipeline_id, slice_id=slice_id, + reason=_bail_reason, ) return last_exit, last_logs diff --git a/orchestrator/routes/pipelines/_run_concurrent_support.py b/orchestrator/routes/pipelines/_run_concurrent_support.py index 1e3956310..5af9ba848 100644 --- a/orchestrator/routes/pipelines/_run_concurrent_support.py +++ b/orchestrator/routes/pipelines/_run_concurrent_support.py @@ -10,17 +10,56 @@ import routes.pipelines as _pkg # noqa: E402,F401 -def _superseded_by_restart_impl(*, store, pipeline_id, run_epoch) -> bool: - """True if a newer run_epoch means another thread owns this pipeline. - - Reloads pipeline state and compares its ``run_epoch`` against the - epoch this thread runs under. Mirrors the post-return epoch check - (#1638) but runs *inside* the poll loop so a superseded thread stops - polling before it can fire stale escalations. Best-effort: a load - failure returns ``False`` so a transient store hiccup never tears - down a legitimately-running phase. +def _phase_bail_reason_impl(*, store, pipeline_id, run_epoch) -> str | None: + """Why this poll loop must exit without escalating, or ``None`` to keep polling. + + Two independent conditions, resolved from a single pipeline load: + + ``superseded_by_restart`` + A newer ``run_epoch`` means another ``_run_pipeline`` thread owns + this pipeline (#3315 facet a). Mirrors the post-return epoch check + (#1638) but runs *inside* the poll loop so a superseded thread stops + polling before it can fire stale escalations. Dormant when + ``run_epoch`` is ``None`` (direct-call paths that thread no epoch). + This is the sole home of the epoch comparison: it replaced the + standalone ``_pipeline_superseded_by_restart`` predicate, which this + helper's callers — the ``_run_concurrent_phase`` poll loop and the + slice-path impasse-retry wrapper — both now reach through here, so + the "no escalation when superseded" property holds on both routes + from one implementation and one load. + + ``pipeline_cancelled`` + The operator cancelled the run (#3633). The cancel route stops this + phase's event loop directly, but nothing stops *this* thread, and + before this check it kept polling — admitting the next slice, + creating its integration branch, and spawning agents against a + pipeline the operator believes is stopped. Re-reading the persisted + status bounds a missed stop signal to one poll interval. + + FAILED is deliberately NOT a bail condition: ``container_monitor`` + reconciliation can mark a live pipeline FAILED while this loop polls, + and the consensus-complete branch recovers it to RUNNING (#1273). + Bailing here would turn that recoverable transient into a real failure. + + Best-effort: a missing store or a load failure returns ``None`` so a + transient store hiccup never tears down a legitimately-running phase. """ - return _pkg._pipeline_superseded_by_restart(store, pipeline_id, run_epoch) + if store is None: + return None + try: + _pip = store.load_pipeline(pipeline_id) + except Exception as _err: # noqa: BLE001 — never wedge the caller + _pkg.logger.debug( + "Phase bail check failed; continuing", + pipeline_id=pipeline_id, + error=str(_err), + ) + return None + if _pip.status == _pkg.PipelineStatus.CANCELLED: + return "pipeline_cancelled" + if run_epoch is not None and (_pip.run_epoch or _pip.created_at) != run_epoch: + return "superseded_by_restart" + return None def _record_container_exit_impl( diff --git a/orchestrator/routes/pipelines/_run_hitl_gate.py b/orchestrator/routes/pipelines/_run_hitl_gate.py index 0cbfa785f..4b07cc95e 100644 --- a/orchestrator/routes/pipelines/_run_hitl_gate.py +++ b/orchestrator/routes/pipelines/_run_hitl_gate.py @@ -10,6 +10,84 @@ import routes.pipelines as _pkg # noqa: E402,F401 +def _gate_wait_cancelled(store, pipeline_id: str) -> bool: + """True when a gate's ``wait_for_decision`` was unblocked by an operator cancel (#3633). + + ``wait_for_decision`` returns on two very different events: a human + resolved the decision, or a cancel route called ``cancel_decision`` on it — + which the pipeline cancel does deliberately, under the comment "cancel any + pending decisions so ``wait_for_decision()`` unblocks" + (``_routes_crud.py``). Downstream the two were indistinguishable: a + cancelled decision carries no ``resolution``, and the empty string is a + member of ``_APPROVE_KEYWORDS``, so the gate read the operator's cancel as + an approval, took the "Approved — resume and advance" branch, wrote + ``RUNNING`` over the persisted ``CANCELLED``, and let the driver advance + into the next phase and mint a fresh cohort. + + That is the #3633 symptom reached through the one family of paths the + persisted-status layers cannot see: every gate re-reads a status the gate + block itself has already overwritten with ``AWAITING_HUMAN`` / + ``RUNNING``, so the loop-head check alone never fires. Each blocking wait + in the gate therefore has to ask this question on its own. + + **Why the returned decision's own status is deliberately not consulted.** + It reads like the sharper signal, and an earlier revision keyed on it, but + it over-fires in two ways that a persisted-status check does not: + + - ``routes/decisions/_lifecycle.py`` exposes a standalone endpoint that + cancels *one* decision without touching the pipeline. Treating that as a + pipeline cancel makes the driver exit and strands a live pipeline at + ``AWAITING_HUMAN`` with no waiter. + - the PATCH route sweeps pending decisions on ``FAILED`` too, so a + ``container_monitor`` false-positive ``FAILED`` — the case #1273 + deliberately carves out of ``_pipeline_cancelled`` — would bail here + anyway, bypassing that carve-out. + + Not bailing on ``FAILED`` is not the same as ignoring it, and the cost is + worth stating plainly: a ``FAILED`` PATCH sweeps the gate's decision, the + wait returns a decision with no ``resolution``, and the empty string is in + ``_APPROVE_KEYWORDS`` — so the gate reads it as an approval and advances + the phase. That is the lesser of the two evils (a false-positive + ``FAILED`` recovers to RUNNING under #1273, and stopping the driver on one + would strand a live pipeline), and no writer PATCHing ``status=FAILED`` + through that route is known today. If one ever appears, the fix belongs in + the resolution parsing — an unset ``resolution`` on a swept decision is not + an approval — not in a ``FAILED`` bail here. + + Both *real* cancel paths (the PATCH route in ``_routes_crud.py`` and + ``_cancel_pipeline_in_process`` in ``routes/decisions/_handlers.py``) + persist ``CANCELLED`` **before** sweeping the queue, so by the time a + *swept* wait returns the status this reads is already authoritative. Keep + that ordering if either route is ever restructured — it is what makes the + persisted status sufficient here. + + That qualifier is load-bearing: the sweep is a one-time snapshot of the + pending queue, so it only reaches decisions that already existed when the + cancel ran. A wait that returns for any *other* reason reads whatever the + store holds — including, before the in-lock park checks landed, the gate's + own ``AWAITING_HUMAN``. This function is therefore the *last* of three + checks each gate needs, not the only one: the other two are the pre-queue + check (so a post-sweep decision is never minted, since nothing would ever + unblock its wait) and the in-lock park check + (:func:`_park_at_gate_unless_cancelled`, so the park write cannot clobber + the cancel this one is trying to read). See ``_pipeline_cancelled``. + + **The phase box is deliberately left at ``AWAITING_HUMAN``.** When this + returns True the park write has already landed, and every caller breaks + without restoring the phase execution's status. That is a choice, not an + oversight: a second write to a pipeline the operator has already stopped + buys nothing, and leaving the box records *where* the run stopped — parked + at this gate — which is what an operator reading a cancelled pipeline's + phase timeline wants. All gates behave the same way (the gap, attestation, + and divergence-reconcile gates included), so a cancelled pipeline renders + consistently. + + Delegates to ``_pipeline_cancelled``, inheriting its FAILED carve-out + (#1273) and its best-effort store-hiccup tolerance. + """ + return _pkg._pipeline_cancelled(store, pipeline_id) + + def _run_hitl_gate_converge( pipeline, *, @@ -132,6 +210,20 @@ def _run_hitl_gate_converge( if _ledger_missing: dq = _pkg.get_decision_queue(pipeline_id, repo_path) + # Never mint a decision for a cancelled run. The cancel route + # sweeps the pending queue *once*; a decision created after that + # sweep has nobody to cancel it, and ``wait_for_decision`` is an + # unbounded poll — the driver thread would block for the process + # lifetime, skipping the ``finally`` that cleans up containers and + # preserves worktrees (#3633 review round 3). + if _pkg._pipeline_cancelled(store, pipeline_id): + _pkg.logger.info( + "Pipeline cancelled before the decision-ledger backstop " + "was queued — exiting the gate without advancing (#3633)", + pipeline_id=pipeline_id, + phase=current_phase.value, + ) + return pipeline, "break" _backstop = dq.queue_decision( question=( f"The {current_phase.value} phase reached its gate " @@ -149,12 +241,21 @@ def _run_hitl_gate_converge( decision_type="choice", phase=current_phase, ) - with _pkg.get_pipeline_state_lock(pipeline_id): - pipeline = store.load_pipeline(pipeline_id) - pipeline.status = _pkg.PipelineStatus.AWAITING_HUMAN - phase_execution = pipeline.get_phase_execution(current_phase) - phase_execution.status = _pkg.PipelineStatus.AWAITING_HUMAN - store.save_pipeline(pipeline) + # Park under the state lock, re-reading the status from inside it: + # the cancel route persists CANCELLED under the same lock, so an + # unconditional write here would silently lose it and the + # post-wait check below would read back our own AWAITING_HUMAN. + pipeline, _park_cancelled = _pkg._park_at_gate_unless_cancelled( + store, pipeline_id, current_phase + ) + if _park_cancelled: + _pkg.logger.info( + "Pipeline cancelled before parking at the decision-ledger " + "backstop — exiting the gate without advancing (#3633)", + pipeline_id=pipeline_id, + phase=current_phase.value, + ) + return pipeline, "break" _pkg.report_pipeline_status( pipeline, event_type="decision.created", @@ -166,6 +267,14 @@ def _run_hitl_gate_converge( _pkg._emit_pipeline_event(pipeline, "decision.created") _backstop_resolved = dq.wait_for_decision(_backstop.id) + if _pkg._gate_wait_cancelled(store, pipeline_id): + _pkg.logger.info( + "Pipeline cancelled while awaiting the decision-ledger " + "backstop — exiting the gate without advancing (#3633)", + pipeline_id=pipeline_id, + phase=current_phase.value, + ) + return pipeline, "break" _backstop_resolution = str( getattr(_backstop_resolved, "resolution", None) or "" ).strip() @@ -250,6 +359,26 @@ def _run_hitl_gate_converge( ) if _rerun_requested: return pipeline, "continue" # Re-enter outer loop → re-run phase + # The attestation gate blocks in its own ``wait_for_decision`` + # and, on any non-RESOLVED terminal status, deliberately "fails + # open to the phase gate" — a safe posture when a cancelled + # attestation only meant one more operator prompt. It is not safe + # once the cancel is the operator stopping the run: 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 + # (#3633 review). + if _pkg._gate_wait_cancelled(store, pipeline_id): + _pkg.logger.info( + "Pipeline cancelled while awaiting the explicit-none " + "attestation — exiting the gate without advancing (#3633)", + pipeline_id=pipeline_id, + phase=current_phase.value, + ) + return pipeline, "break" # Check for an existing pending phase_gate decision for this # phase. A prior agent-exit event may @@ -262,6 +391,55 @@ def _run_hitl_gate_converge( for d in pipeline.decisions ) + # Read the draft up front, before the reuse-vs-create branch below. + # Both ``draft_content`` and ``phase_label`` are read unconditionally + # further down — the follow-up decision in the "bare request changes" + # path uses them for its question and context — but they used to be + # bound only inside the ``else:`` arm. Resuming onto an existing + # pending gate (an orchestrator restart or a driver respawn while + # parked at a refine/plan gate) and then answering with a bare + # "request changes" therefore raised ``UnboundLocalError`` before the + # follow-up could be queued (#3633 review). Binding both here costs + # one draft read on the reuse path and makes the follow-up reachable + # from either branch. + phase_label = "analysis" if current_phase.value == "refine" else current_phase.value + draft_content = _pkg._read_phase_draft( + worktree_repo_path, + current_phase.value, + issue_number=pipeline.issue_number, + pipeline_id=pipeline_id, + branch=pipeline.branch, + ) + # Warn if draft is missing — the agent may not have written + # it to the expected path. See #1016. The warning stays scoped to + # the create arm, which is the arm that renders the draft into a new + # gate comment: the reuse arm never read the draft before the hoist + # above, so warning there would be new operator-facing noise about a + # draft nothing is about to show (#3633 review round 2). The + # placeholder is still bound on both arms — the follow-up prompt uses + # it as decision context regardless of which arm queued the gate. + if draft_content is None: + if existing_pending_gate: + _pkg.logger.debug( + "HITL gate: draft not found on work branch (reusing an " + "existing pending gate; not rendering a draft)", + pipeline_id=pipeline_id, + phase=current_phase.value, + worktree_path=str(worktree_repo_path), + ) + else: + _pkg.logger.warning( + "HITL gate: draft not found on work branch", + pipeline_id=pipeline_id, + phase=current_phase.value, + worktree_path=str(worktree_repo_path), + ) + draft_content = ( + f"**Warning**: No {phase_label} draft was found on the " + f"work branch. The agent may not have written the output " + f"to the expected path." + ) + if existing_pending_gate: _pkg.logger.info( "HITL gate: reusing existing pending phase_gate decision", @@ -278,30 +456,6 @@ def _run_hitl_gate_converge( and d.status == _pkg.DecisionStatus.PENDING ) else: - draft_content = _pkg._read_phase_draft( - worktree_repo_path, - current_phase.value, - issue_number=pipeline.issue_number, - pipeline_id=pipeline_id, - branch=pipeline.branch, - ) - phase_label = "analysis" if current_phase.value == "refine" else current_phase.value - - # Warn if draft is missing — the agent may not have written - # it to the expected path. See #1016. - if draft_content is None: - _pkg.logger.warning( - "HITL gate: draft not found on work branch", - pipeline_id=pipeline_id, - phase=current_phase.value, - worktree_path=str(worktree_repo_path), - ) - draft_content = ( - f"**Warning**: No {phase_label} draft was found on the " - f"work branch. The agent may not have written the output " - f"to the expected path." - ) - question = ( f"The {current_phase.value} phase has completed. " f"Please review the {phase_label} and approve to continue, " @@ -371,6 +525,22 @@ def _run_hitl_gate_converge( _content_changed = gate_context != _prev_gate.context dq = _pkg.get_decision_queue(pipeline_id, repo_path) + # Everything above this line — the phase-draft reads, the + # human-companion read, the previous-gate scan — is seconds of IO + # the cancel route can land in. Minting the gate for a cancelled + # run leaves an orphan decision the one-time pending sweep already + # passed, so nothing will ever cancel it and the unbounded + # ``wait_for_decision`` below never returns: the driver thread + # leaks for the process lifetime and ``_run_pipeline``'s + # ``finally`` never runs (#3633 review round 3). + if _pkg._pipeline_cancelled(store, pipeline_id): + _pkg.logger.info( + "Pipeline cancelled before the phase gate was queued — " + "exiting the gate without advancing (#3633)", + pipeline_id=pipeline_id, + phase=current_phase.value, + ) + return pipeline, "break" decision = dq.queue_decision( question=question, context=gate_context, @@ -381,15 +551,24 @@ def _run_hitl_gate_converge( ) # Reload pipeline to pick up the decision persisted by queue_decision(), - # otherwise the stale local object overwrites it with an empty decisions list. - with _pkg.get_pipeline_state_lock(pipeline_id): - pipeline = store.load_pipeline(pipeline_id) - pipeline.status = _pkg.PipelineStatus.AWAITING_HUMAN - # Also mark the phase as awaiting human so the DAG visualization - # shows the HITL gate on the correct phase box. - phase_execution = pipeline.get_phase_execution(current_phase) - phase_execution.status = _pkg.PipelineStatus.AWAITING_HUMAN - store.save_pipeline(pipeline) + # otherwise the stale local object overwrites it with an empty decisions + # list — and park at AWAITING_HUMAN, but only if the operator has not + # cancelled in the meantime. The check lives inside the same state lock + # as the write (see ``_park_at_gate_unless_cancelled``): this write is + # on the reuse arm as well as the create arm, and an unconditional one + # is what let a cancel arriving before the gate be read back as the + # gate's own AWAITING_HUMAN and then advanced (#3633 review round 3). + pipeline, _park_cancelled = _pkg._park_at_gate_unless_cancelled( + store, pipeline_id, current_phase + ) + if _park_cancelled: + _pkg.logger.info( + "Pipeline cancelled before parking at the phase gate — " + "exiting the gate without advancing (#3633)", + pipeline_id=pipeline_id, + phase=current_phase.value, + ) + return pipeline, "break" # Report HITL gate to collaborator _pkg.report_pipeline_status( @@ -403,6 +582,26 @@ def _run_hitl_gate_converge( # Check resolution — did the human approve or request changes? resolved_decision = dq.get_decision(decision.id) + + # ...or did neither happen, because the operator cancelled the + # pipeline and the route cancelled this decision to unblock the wait + # above? Bail before any of the resolution parsing below: an unset + # resolution reads as an approval (``"" in _APPROVE_KEYWORDS``), and + # the approve branch rewrites the operator's CANCELLED to RUNNING and + # advances the phase — the #3633 spawn, through the one path the + # persisted-status layers cannot see. "break" leaves the driver loop + # the same way its own CANCELLED check at the loop head does, so the + # ``finally`` observes CANCELLED and preserves the worktrees + # ``restart_phase`` resumes from (#1725). + if _pkg._gate_wait_cancelled(store, pipeline_id): + _pkg.logger.info( + "Pipeline cancelled while awaiting the phase gate — " + "exiting the gate without advancing (#3633)", + pipeline_id=pipeline_id, + phase=current_phase.value, + ) + return pipeline, "break" + resolution = (resolved_decision.resolution or "").strip() # JSON-first resolution parsing: try structured payload before @@ -482,6 +681,20 @@ def _run_hitl_gate_converge( ) except _pkg.json.JSONDecodeError, TypeError, AttributeError: display_resolution = resolution + # Same one-time-sweep hazard as the gate above: a cancel landing + # between the gate's resolution and this follow-up would mint a + # decision nothing can cancel, and the wait below would never + # return (#3633 review round 3). 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. + if _pkg._pipeline_cancelled(store, pipeline_id): + _pkg.logger.info( + "Pipeline cancelled before the gate follow-up was queued " + "— exiting the gate without advancing (#3633)", + pipeline_id=pipeline_id, + phase=current_phase.value, + ) + return pipeline, "break" followup = dq.queue_decision( question=( f'You selected "{display_resolution}" but didn\'t provide specific feedback. ' @@ -495,6 +708,14 @@ def _run_hitl_gate_converge( ) dq.wait_for_decision(followup.id) resolved_followup = dq.get_decision(followup.id) + if _pkg._gate_wait_cancelled(store, pipeline_id): + _pkg.logger.info( + "Pipeline cancelled while awaiting gate follow-up " + "specifics — exiting the gate without advancing (#3633)", + pipeline_id=pipeline_id, + phase=current_phase.value, + ) + return pipeline, "break" followup_resolution = (resolved_followup.resolution or "").strip() # Parse follow-up resolution (also JSON-first) @@ -592,6 +813,7 @@ def _run_hitl_gate_converge( pipeline_id, _pkg._pipeline_identifier(pipeline.issue_number, pipeline_id), current_phase, + cancelled=lambda: _pkg._gate_wait_cancelled(store, pipeline_id), ) except Exception as bridge_err: _pkg.logger.warning( @@ -601,6 +823,23 @@ def _run_hitl_gate_converge( error=str(bridge_err), ) + # The bridge is the *longest* human-latency window in the gate — one + # blocking wait per contract question, answered sequentially — and a + # cancel landing in it unblocks every one of them with no + # ``resolution``. The bridge only counts RESOLVED answers, so + # ``_decisions_resolved_this_round`` stays 0, the converge branch + # below is skipped, and control falls straight through to "Approved — + # resume and advance", which writes RUNNING over the operator's + # CANCELLED and advances the phase: #3633 verbatim (#3633 review). + if _pkg._gate_wait_cancelled(store, pipeline_id): + _pkg.logger.info( + "Pipeline cancelled while bridging contract decisions — " + "exiting the gate without advancing (#3633)", + pipeline_id=pipeline_id, + phase=current_phase.value, + ) + return pipeline, "break" + # Converge-before-advance (#3392): if the operator just # resolved one or more decisions, re-run the phase so the # documents reflect those resolutions and any decision the diff --git a/orchestrator/routes/pipelines/_run_implement.py b/orchestrator/routes/pipelines/_run_implement.py index 3dfd62fed..55aeb7ae4 100644 --- a/orchestrator/routes/pipelines/_run_implement.py +++ b/orchestrator/routes/pipelines/_run_implement.py @@ -38,9 +38,12 @@ def _run_implement_phase_slices( slice reached CONFIRMED; non-zero means at least one slice failed. """ try: - from orchestrator.slice_scheduler import SliceScheduler + from orchestrator.slice_scheduler import SchedulerSliceState, SliceScheduler except ImportError: - from slice_scheduler import SliceScheduler + from slice_scheduler import ( # type: ignore[no-redef] + SchedulerSliceState, + SliceScheduler, + ) try: from egg_contracts.loader import load_contract, save_contract @@ -528,6 +531,42 @@ def _bootstrap_check_one(slice_obj: _pkg.Any) -> tuple[str, bool]: try: while not scheduler.all_done(): driver_heartbeat.record_tick(pipeline_id) # #3540 liveness tick + + # 0. Stop walking the DAG once this loop no longer owns the phase. + # Nothing used to stop it: the next tick admitted the next ready + # slice, created its integration branch, and spawned a fresh + # agent cohort. Two ways that goes wrong, both caught by the + # same re-read — see ``_phase_bail_reason_impl``: + # + # * the operator cancelled the run (#3633) — observed opening + # slice-3 two hours after the cancel; + # * 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. + # + # Bail before admitting another wave, so an in-flight one is the + # last. ``_run_phase_execution`` maps the non-zero exit onto a + # clean thread return rather than a phase FAILURE. + _loop_bail = _pkg._phase_bail_reason_impl( + store=store, pipeline_id=pipeline_id, run_epoch=run_epoch + ) + if _loop_bail is not None: + _pkg.logger.info( + "Stopping the slice loop without admitting further slices", + pipeline_id=pipeline_id, + reason=_loop_bail, + unfinished=[ + rt.slice_id + for rt in scheduler.list_slices() + if rt.state != SchedulerSliceState.COMPLETE + ], + ) + aggregate_logs.append(f"--- slice loop stopped: {_loop_bail} ---") + overall_exit = 1 + break + # 1. Snapshot ready slices for this tick. ready_batch = list(scheduler.iter_ready()) if not ready_batch: @@ -878,6 +917,23 @@ def _run_one_slice_inner( ) if exit_code_inner != 0: + # An operator cancel is not a slice failure (#3633 + # review). The phase runner returns non-zero on its + # cancel bail, and 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 for a clean cancel. Same intent-preservation + # argument as ``_run_phase_execution``'s CANCELLED + # carve-out, one level down. + if _pkg._pipeline_cancelled(store, pipeline_id): + _pkg.logger.info( + "Slice stopped by pipeline cancellation (not recorded as a failure)", + pipeline_id=pipeline_id, + slice_id=slice_id, + exit_code=exit_code_inner, + ) + return exit_code_inner, logs_inner scheduler.record_failure(slice_id) _pkg.logger.warning( "Slice failed", diff --git a/orchestrator/routes/pipelines/_run_phase.py b/orchestrator/routes/pipelines/_run_phase.py index d8b92cf84..e78057036 100644 --- a/orchestrator/routes/pipelines/_run_phase.py +++ b/orchestrator/routes/pipelines/_run_phase.py @@ -283,6 +283,20 @@ def _run_phase_execution( ) return pipeline, phase_execution, phase_failed, "return" + # Same for a cancel (#3633): the phase runner returns non-zero + # once it sees the CANCELLED status, and the operator's cancel + # is the reason this phase stopped — not a failure of it. + # 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. + if _check_pip.status == _pkg.PipelineStatus.CANCELLED: + _pkg.logger.info( + "Pipeline was cancelled during phase execution, exiting thread", + pipeline_id=pipeline_id, + phase=current_phase.value, + ) + return pipeline, phase_execution, phase_failed, "return" + error_msg = f"Container exited with code {exit_code}" if container_logs: log_lines = container_logs.strip().splitlines() diff --git a/orchestrator/routes/pipelines/_run_phase_blocks.py b/orchestrator/routes/pipelines/_run_phase_blocks.py index 1e0b3e4e7..0cf77ced1 100644 --- a/orchestrator/routes/pipelines/_run_phase_blocks.py +++ b/orchestrator/routes/pipelines/_run_phase_blocks.py @@ -21,7 +21,9 @@ def _run_implement_advance( store, worktree_repo_path, ): - """IMPLEMENT-phase advance loop block (extracted verbatim; pure fall-through).""" + """IMPLEMENT-phase advance loop block. Returns ``(pipeline, action)``; + ``action == "break"`` -> the operator cancelled inside the gap gate and the + caller must leave the driver loop instead of advancing.""" if current_phase == _pkg.PipelinePhase.IMPLEMENT: try: gap_gated = _pkg._await_unresolved_gap_gate( @@ -34,6 +36,25 @@ def _run_implement_advance( pipeline.config.hitl_gates, ) pipeline = store.load_pipeline(pipeline_id) + # The gate bails out of its wait without restoring RUNNING when the + # operator cancelled mid-wait (#3633) — but it reports that through + # the same ``gated`` boolean an ordinary gating returns, so nothing + # downstream can tell the two apart. Re-read the persisted status + # here and hand the driver an explicit stop: without it the cancel + # is silently defaulted away, the commit+push below mutates the + # remote branch of a pipeline the operator just stopped, and + # IMPLEMENT being terminal (``PHASE_TRANSITIONS[IMPLEMENT] == []``) + # takes _run_pipeline straight into its "pipeline complete" branch, + # overwriting CANCELLED with COMPLETE and deleting the worktrees + # restart_phase resumes from (#3633 review round 2). + if _pkg._pipeline_cancelled(store, pipeline_id): + _pkg.logger.info( + "Unresolved-gap gate: pipeline cancelled while awaiting the " + "operator — stopping the driver without advancing (#3633)", + pipeline_id=pipeline_id, + phase=current_phase.value, + ) + return pipeline, "break" # The gate ran after the statefile commit+push above, so # when it changed the contract (operator resolved a gap, # or the override audit landed) the resolution is still @@ -88,7 +109,7 @@ def _run_implement_advance( phase=current_phase.value, error=str(gap_gate_err), ) - return pipeline + return pipeline, None def _run_plan_advance( diff --git a/orchestrator/routes/pipelines/_run_pipeline.py b/orchestrator/routes/pipelines/_run_pipeline.py index 4e59865d4..73decf3f3 100644 --- a/orchestrator/routes/pipelines/_run_pipeline.py +++ b/orchestrator/routes/pipelines/_run_pipeline.py @@ -744,7 +744,10 @@ def _hook() -> None: # discarded — the local commits remain pinned under the backup # ref for offline recovery. ``pre_event_hook`` tears down the # per-phase overseer under its own lock before the public - # ``pipeline.failed`` event, matching the prior ordering. + # ``pipeline.failed`` event, matching the prior ordering. The + # helper also fields the operator-cancelled-during-the-pause case + # (#3633), where it tears down but leaves CANCELLED persisted + # instead of pinning FAILED; the ``break`` below is right for both. if post_phase_sync_aborted and post_phase_sync_outcome is not None: _pkg._fail_pipeline_after_divergence_abort( pipeline_id, @@ -901,7 +904,7 @@ def _hook() -> None: # not block — both options need a human, so blocking would # stall the pipeline indefinitely; the reactive CI check stays # the backstop there. - pipeline = _pkg._run_implement_advance( + pipeline, _gap_gate_action = _pkg._run_implement_advance( pipeline, current_phase=current_phase, gateway_mode=gateway_mode, @@ -911,6 +914,13 @@ def _hook() -> None: store=store, worktree_repo_path=worktree_repo_path, ) + if _gap_gate_action == "break": + # The operator cancelled while the gap gate was blocked in + # ``wait_for_decision`` (#3633). Leave the loop the same way the + # CANCELLED check at the loop head does. Falling through instead + # would reach the terminal-phase branch below — IMPLEMENT has no + # successor — and write COMPLETE over the operator's CANCELLED. + break # --- HITL gate: pause for human approval --- # Refine/plan are gated by the converge-before-advance loop @@ -941,6 +951,16 @@ def _hook() -> None: ) if _hitl_gate_action == "continue": continue + if _hitl_gate_action == "break": + # The operator cancelled while the gate was blocked in one of + # its ``wait_for_decision`` calls (#3633). Leave the loop the + # same way the CANCELLED check at the loop head does rather + # than advancing the phase: the gate parks at AWAITING_HUMAN + # and writes RUNNING back on its way out, so by the time + # control returns here the loop-head check would be reading a + # status the gate itself had already overwritten. It has to + # bail from inside. + break # ---------------------------------------------------------- # #2777 (cq-4, TASK-1-2) — inline ``_run_pipeline`` @@ -1008,7 +1028,29 @@ def _hook() -> None: ) if not next_phases: - # Terminal phase — pipeline complete + # Terminal phase — pipeline complete. + # + # ...unless the operator cancelled somewhere between the loop + # head and here. This branch is the last unguarded status write + # in the loop, and it is the one every park-and-resume block in + # a terminal phase falls into: the block bails without writing + # RUNNING back, the driver reads that as "nothing left to do", + # and CANCELLED becomes COMPLETE plus a "completed + # successfully" broadcast — after which the ``finally`` sees a + # non-CANCELLED status, leaves ``skip_cleanup`` False, and + # deletes the worktrees ``restart_phase`` resumes from (#3633). + # Re-read the persisted status here so the guard holds for + # future blocks too, not just today's gap gate. + if _pkg._pipeline_cancelled(store, pipeline_id): + _pkg.logger.info( + "Terminal phase reached on a cancelled pipeline — " + "leaving the persisted CANCELLED intact and skipping " + "the completion broadcast (#3633)", + pipeline_id=pipeline_id, + phase=current_phase.value, + ) + break + with _pkg.get_pipeline_state_lock(pipeline_id): pipeline = store.load_pipeline(pipeline_id) pipeline.status = _pkg.PipelineStatus.COMPLETE @@ -1408,11 +1450,26 @@ def _hook() -> None: old_epoch=run_epoch.isoformat(), new_epoch=_cleanup_epoch.isoformat(), ) - elif current.status == _pkg.PipelineStatus.FAILED: + elif current.status in ( + _pkg.PipelineStatus.FAILED, + _pkg.PipelineStatus.CANCELLED, + ): + # CANCELLED joins FAILED here (#3633 review): ``restart_phase`` + # allowlists CANCELLED precisely so a ``cancel_task`` run can be + # resumed without a full resubmission (#1725, + # ``_routes_restart.py``), and the PATCH route already passes + # ``preserve_worktrees=(status == "cancelled")`` to + # ``cleanup_pipeline`` (``_routes_crud.py``). Before this, the + # driver's own ``finally`` contradicted both by deleting the + # worktrees the operator was told they could resume from. The + # #3633 layers make that land seconds after the cancel rather + # than at the next consensus timeout, so the two policies have + # to agree. skip_cleanup = True _pkg.logger.info( - "Pipeline failed, preserving worktrees for retry", + "Pipeline failed or was cancelled, preserving worktrees for retry", pipeline_id=pipeline_id, + status=current.status.value, ) except Exception: # Pipeline was deleted and not recreated — safe to clean up diff --git a/orchestrator/routes/pipelines/_run_pipeline_setup.py b/orchestrator/routes/pipelines/_run_pipeline_setup.py index 2381af8e7..a2b3fdc93 100644 --- a/orchestrator/routes/pipelines/_run_pipeline_setup.py +++ b/orchestrator/routes/pipelines/_run_pipeline_setup.py @@ -690,7 +690,10 @@ def _resolve_worktree_repo( # Operator aborted the manual reconcile (or the pause # budget was exhausted). Fail the pipeline; the local # commits remain pinned under the backup ref for offline - # recovery — nothing was discarded. + # recovery — nothing was discarded. Also covers an operator + # cancel landing during the pause (#3633): the helper is a + # status no-op there, leaving CANCELLED persisted, and the + # ``return pipeline, True`` below stops the driver either way. _pkg._fail_pipeline_after_divergence_abort( pipeline_id, store, diff --git a/orchestrator/routes/pipelines/_run_support.py b/orchestrator/routes/pipelines/_run_support.py index e09f3a1f0..fd9ad63e9 100644 --- a/orchestrator/routes/pipelines/_run_support.py +++ b/orchestrator/routes/pipelines/_run_support.py @@ -88,31 +88,115 @@ def _clear_stale_impasses_for_producers( ) -def _pipeline_superseded_by_restart( - store, pipeline_id: str, run_epoch: _pkg.datetime | None -) -> bool: - """True if a newer ``run_epoch`` means another thread now owns this pipeline. - - Reloads pipeline state and compares its ``run_epoch`` against the epoch the - caller runs under (#3315 facet a). Best-effort: a missing epoch or a load - failure returns ``False`` so a transient store hiccup never tears down a - legitimately-running phase. Shared by the ``_run_concurrent_phase`` poll - loop and the slice-path impasse-retry wrapper so the "no escalation when - superseded" property holds on both routes. +def _pipeline_cancelled(store, pipeline_id: str) -> bool: + """True if ``pipeline_id`` is persisted as CANCELLED (#3633). + + The operator's cancel is the authoritative "stop driving this run" + signal, and the persisted status outlives the in-process mechanisms a + cancel cannot reach (a stop event a thread never checks, an event loop + the cancel route did not know about). Driver work loops re-read it so a + cancelled pipeline stops admitting slices and spawning agents rather + than walking the rest of its DAG against an operator who believes it is + stopped. + + It is not inviolable, and callers should not treat it as such. Every + operator-blocking gate parks the pipeline at ``AWAITING_HUMAN`` and writes + ``RUNNING`` back once its wait returns, so a loop that only re-reads the + status *after* such a gate sees the gate's own write, not the operator's + cancel. That is why each of them re-checks from inside, at **both** writes: + + - before the park write, from inside the same state lock that performs it + (:func:`_park_at_gate_unless_cancelled`, and the bespoke in-lock checks + in ``_alerts.py`` and the gap gate's ``_set_status``). Re-checking only + after the wait is not enough: the park write itself clobbers the + operator's ``CANCELLED``, and the post-wait check then reads back the + gate's own ``AWAITING_HUMAN`` and concludes nothing happened; + - before the resume write, after the wait returns + (``_gate_wait_cancelled``, at all five of its blocking waits; + ``_await_unresolved_gap_gate``; the divergence-reconcile pause). + + 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. + + A third check belongs *before* the decision is queued, at every site that + mints one: the cancel route sweeps pending decisions once, so a decision + minted after that sweep is never cancelled and ``wait_for_decision`` — an + unbounded poll with no timeout — blocks for the process lifetime, leaking + the driver thread and skipping ``_run_pipeline``'s ``finally`` entirely + (#3633 review round 3). Any new park-and-resume block needs all three. + + Re-checking is only half of it. A block that merely *skips* its RUNNING + write leaves the persisted CANCELLED intact and then hands its caller the + same value an ordinary gating returns — so the driver keeps walking the + DAG and the next unguarded status write clobbers the cancel anyway. Each + block therefore has to propagate a stop the driver acts on: the five + ``_run_hitl_gate.py`` sites and the gap gate's caller + (``_run_implement_advance``) return ``"break"``, and the + divergence-reconcile pause returns ``aborted=True``. The terminal-phase + "pipeline complete" branch in ``_run_pipeline`` re-checks as a backstop for + the case a future block forgets. + + FAILED is deliberately not included: ``container_monitor`` + reconciliation can mark a live pipeline FAILED mid-poll, and the + consensus-complete path recovers it to RUNNING (#1273). + + Best-effort: a missing store or a load failure returns ``False`` so a + transient store hiccup never tears down a legitimately-running phase. """ - if store is None or run_epoch is None: + if store is None: return False try: - _epoch_pip = store.load_pipeline(pipeline_id) - except Exception as _epoch_err: # noqa: BLE001 — never wedge the caller + return store.load_pipeline(pipeline_id).status == _pkg.PipelineStatus.CANCELLED + except Exception as exc: # noqa: BLE001 — never wedge the caller _pkg.logger.debug( - "Epoch supersession check failed; continuing", + "Cancellation check failed; continuing", pipeline_id=pipeline_id, - error=str(_epoch_err), + error=str(exc), ) return False - current_epoch = _epoch_pip.run_epoch or _epoch_pip.created_at - return current_epoch != run_epoch + + +def _park_at_gate_unless_cancelled(store, pipeline_id: str, phase) -> tuple[_pkg.Any, bool]: + """Persist ``AWAITING_HUMAN`` for pipeline+phase, unless the run is cancelled (#3633). + + Every operator-blocking gate parks the pipeline (and its phase box, so the + DAG visualization renders the gate on the right phase) at + ``AWAITING_HUMAN`` before blocking in ``wait_for_decision``. That write is + unconditional in every pre-#3633 gate, and it is the write that made the + persisted-status layers blind: it lands *on top of* the operator's + ``CANCELLED``, so the post-wait re-check reads back the gate's own + ``AWAITING_HUMAN``, concludes the run is live, and advances the phase. + + The check is performed **inside** the same per-pipeline state lock that + performs the write, not just before taking it. ``StateStore.update_pipeline`` + — the cancel route's persistence path — holds that lock, so an out-of-lock + read can observe RUNNING and then have the write land after the cancel's, + silently losing it. In-lock, the two interleavings are the only ones + possible: the cancel wins the lock (we see ``CANCELLED`` and skip), or we + do (the cancel's own sweep then unblocks the wait and the post-wait check + fires). + + Returns ``(pipeline, cancelled)``. On ``cancelled=True`` nothing was + written and the caller must **not** wait — it has to propagate a stop the + driver acts on (``"break"`` from the gate blocks, ``aborted=True`` from + the divergence pause); skipping the write alone leaves the driver walking + the DAG until the next unguarded write clobbers the cancel anyway. + + The returned ``pipeline`` is the freshly-loaded object in both cases, so + callers can rebind and keep reporting against current state. + """ + with _pkg.get_pipeline_state_lock(pipeline_id): + pipeline = store.load_pipeline(pipeline_id) + if pipeline.status == _pkg.PipelineStatus.CANCELLED: + return pipeline, True + pipeline.status = _pkg.PipelineStatus.AWAITING_HUMAN + phase_execution = pipeline.get_phase_execution(phase) + if phase_execution is not None: + phase_execution.status = _pkg.PipelineStatus.AWAITING_HUMAN + store.save_pipeline(pipeline) + return pipeline, False def _spawn_and_wait( diff --git a/orchestrator/tests/test_ble001_narrowing_audit.py b/orchestrator/tests/test_ble001_narrowing_audit.py index 3768dbcf0..e38c8bb40 100644 --- a/orchestrator/tests/test_ble001_narrowing_audit.py +++ b/orchestrator/tests/test_ble001_narrowing_audit.py @@ -173,7 +173,21 @@ def test_audit_window_retains_documented_ble001_population() -> None: # it degrades to ``teardown_confirmed: false`` instead of falling through to # the outer handler and logging a misleading "failed to list" — mirroring # ``_await_terminating_event_jobs`` on the event-loop path. - assert len(noqa_lines) <= 121, ( + # Moved 121 -> 123 by #3633 (cancel stops the driver): three audited sites + # added, one pre-existing site removed. + # + ``_stop_pipeline_event_loops`` (``_lifecycle_helpers.py``) swallows a + # raising ``loop.stop()`` so one wedged loop cannot abort teardown of + # the rest; + # + ``_phase_bail_reason_impl`` (``_run_concurrent_support.py``) and + # ``_pipeline_cancelled`` (``_run_support.py``) each swallow a raising + # ``store.load_pipeline`` so a transient store hiccup degrades to "keep + # polling / not cancelled" instead of tearing down a legitimately- + # running phase; + # - ``_pipeline_superseded_by_restart`` was deleted — its epoch + # comparison folded into ``_phase_bail_reason_impl``, which resolves + # both bail conditions from a single load — taking its own + # ``store.load_pipeline`` swallow with it. + assert len(noqa_lines) <= 123, ( f"Found {len(noqa_lines)} ``# noqa: BLE001`` swallows in " f"routes/pipelines.py, well past the documented population — a future " f"PR appears to have re-introduced swallow-all handlers without " diff --git a/orchestrator/tests/test_cancel_stops_driver.py b/orchestrator/tests/test_cancel_stops_driver.py new file mode 100644 index 000000000..0db42c7ce --- /dev/null +++ b/orchestrator/tests/test_cancel_stops_driver.py @@ -0,0 +1,1363 @@ +"""A cancelled pipeline must stop spawning agents (issue #3633). + +``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 thread 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. + +These tests pin the five layers of the fix: + +1. the cancel route stops every live BRC event loop, and does it *before* + container cleanup (cleanup that races a live loop is removing pods the + loop is entitled to replace); +2. a loop stopped mid-tick refuses the spawn it was about to request; +3. the concurrent-phase poll loop re-reads the persisted status and bails + (without escalating, and without rewriting CANCELLED to FAILED); +4. the implement-phase slice loop refuses to admit another slice; +5. the refine/plan HITL gate — the one path that *overwrites* the persisted + status the four layers above key on — bails on its own signal (a cancelled + decision) instead of reading the operator's cancel as an approval. +""" + +from __future__ import annotations + +import threading +from datetime import UTC, datetime, timedelta +from pathlib import Path +from types import SimpleNamespace +from unittest.mock import MagicMock, patch + +import pytest +import routes.pipelines as pipelines_pkg +from event_loop import ( + _LIVE_LOOPS, + OrchestratorEventLoop, + _register_live_loop, + _unregister_live_loop, +) +from flask import Flask +from models import ( + DecisionStatus, + HITLDecision, + Pipeline, + PipelinePhase, + PipelineStatus, +) +from routes.pipelines import pipelines_bp +from slice_scheduler import SchedulerSliceState + +PIPELINE_ID = "issue-3633" + + +# --------------------------------------------------------------------------- +# Layer 1 — the cancel route stops live event loops, before cleanup +# --------------------------------------------------------------------------- + + +class _FakeLoop: + """Minimal stand-in for a live ``OrchestratorEventLoop`` registry entry.""" + + def __init__(self, pipeline_id: str, slice_id: str | None, log: list[str]) -> None: + self.pipeline_id = pipeline_id + self.slice_id = slice_id + self._log = log + self.stop_calls: list[float | None] = [] + + def stop(self, *, join_timeout: float | None = 5.0) -> None: + self.stop_calls.append(join_timeout) + self._log.append(f"stop:{self.slice_id}") + + +@pytest.fixture +def client(): + app = Flask(__name__) + app.register_blueprint(pipelines_bp) + app.config["TESTING"] = True + return app.test_client() + + +@pytest.fixture +def live_loops(): + """Register fake loops for two concurrent slices; always clean up.""" + log: list[str] = [] + loops = [ + _FakeLoop(PIPELINE_ID, "slice-1", log), + _FakeLoop(PIPELINE_ID, "slice-2", log), + # A different pipeline's loop must be left alone. + _FakeLoop("issue-other", "slice-1", log), + ] + for loop in loops: + _register_live_loop(loop) + try: + yield loops, log + finally: + for loop in loops: + _unregister_live_loop(loop) + + +def _cancellable_pipeline(status=PipelineStatus.RUNNING) -> Pipeline: + return Pipeline( + id=PIPELINE_ID, + issue_number=3633, + repo="owner/repo", + branch=f"egg/{PIPELINE_ID}/work", + status=status, + current_phase=PipelinePhase.IMPLEMENT, + ) + + +def _patch_cancel_route(pipeline, cleanup_log, *, prev_status=PipelineStatus.RUNNING): + """Patch the collaborators the PATCH-cancel path reaches out to. + + ``_resolve_pipeline`` yields the *pre-update* pipeline (whose status the + route reads as ``prev_status``) and ``update_pipeline`` yields the + post-update one — the transition the route gates on. + """ + before = _cancellable_pipeline(status=prev_status) + store = MagicMock() + store.update_pipeline.return_value = pipeline + store.load_pipeline.return_value = pipeline + + spawner = MagicMock() + + def _cleanup(pipeline_id, **kwargs): + cleanup_log.append(f"cleanup:{pipeline_id}") + return 0 + + spawner.cleanup_pipeline.side_effect = _cleanup + + dq = MagicMock() + dq.get_pending_decisions.return_value = [] + + return ( + patch("routes.pipelines.get_repo_path", return_value="/repo"), + patch("routes.pipelines._resolve_pipeline", return_value=(store, before)), + patch("routes.pipelines.get_container_spawner", return_value=spawner), + patch("routes.pipelines.get_decision_queue", return_value=dq), + ) + + +def test_cancel_stops_live_event_loops_before_container_cleanup(client, live_loops): + """The regression: cancel must stop the spawner, not just its output. + + Both of this pipeline's slice loops are signalled, another pipeline's + loop is untouched, and the stops are ordered ahead of the container + cleanup so the teardown does not race a loop still entitled to spawn. + """ + loops, log = live_loops + pipeline = _cancellable_pipeline() + pipeline.status = PipelineStatus.CANCELLED + patches = _patch_cancel_route(pipeline, log) + + with patches[0], patches[1], patches[2], patches[3]: + response = client.patch( + f"/api/v1/pipelines/{PIPELINE_ID}", + json={"status": "cancelled"}, + ) + assert response.status_code == 200 + # Cleanup runs on a daemon thread; give it a moment to land. + for _ in range(50): + if any(entry.startswith("cleanup:") for entry in log): + break + threading.Event().wait(0.02) + + assert loops[0].stop_calls, "slice-1's event loop was never stopped" + assert loops[1].stop_calls, "slice-2's event loop was never stopped" + assert not loops[2].stop_calls, "another pipeline's event loop must not be stopped" + + # join_timeout=0.0: the PATCH runs in the request thread and must not + # block the operator on a daemon thread's wind-down. + assert loops[0].stop_calls == [0.0] + + stop_indices = [i for i, entry in enumerate(log) if entry.startswith("stop:")] + cleanup_indices = [i for i, entry in enumerate(log) if entry.startswith("cleanup:")] + assert cleanup_indices, "container cleanup never ran" + assert max(stop_indices) < min(cleanup_indices), ( + "event loops must be stopped BEFORE container cleanup; cleanup that " + "races a live loop just removes pods the loop will respawn" + ) + + +def test_cancel_evicts_loops_from_the_live_registry(client, live_loops): + """A stopped loop is gone from the registry, so a re-cancel is a no-op.""" + loops, log = live_loops + pipeline = _cancellable_pipeline() + pipeline.status = PipelineStatus.CANCELLED + patches = _patch_cancel_route(pipeline, log) + + # Real loops unregister inside stop(); the fakes do not, so drive the + # real registry contract directly here. + real = OrchestratorEventLoop( + MagicMock(), MagicMock(), pipeline_id=PIPELINE_ID, slice_id="slice-real", phase="implement" + ) + _register_live_loop(real) + try: + with patches[0], patches[1], patches[2], patches[3]: + client.patch(f"/api/v1/pipelines/{PIPELINE_ID}", json={"status": "cancelled"}) + assert (PIPELINE_ID, "slice-real") not in _LIVE_LOOPS + finally: + _unregister_live_loop(real) + + +def test_recancel_of_an_already_cancelled_pipeline_does_not_re_stop(client, live_loops): + """Gated on the transition: an idempotent re-cancel has nothing live left.""" + loops, log = live_loops + pipeline = _cancellable_pipeline(status=PipelineStatus.CANCELLED) + patches = _patch_cancel_route(pipeline, log, prev_status=PipelineStatus.CANCELLED) + + with patches[0], patches[1], patches[2], patches[3]: + # ``prev_status`` is read off the resolved pipeline, which is already + # CANCELLED — so this PATCH is not a transition. + response = client.patch( + f"/api/v1/pipelines/{PIPELINE_ID}", + json={"status": "cancelled"}, + ) + assert response.status_code == 200 + + assert not loops[0].stop_calls + assert not loops[1].stop_calls + + +def test_failed_transition_leaves_the_event_loop_alone(client, live_loops): + """FAILED is excluded: ``container_monitor`` marks live pipelines FAILED + mid-phase and the poll loop recovers them to RUNNING (#1273). Tearing the + loop down there would convert a recoverable transient into a dead run.""" + loops, log = live_loops + pipeline = _cancellable_pipeline() + pipeline.status = PipelineStatus.FAILED + patches = _patch_cancel_route(pipeline, log) + + with patches[0], patches[1], patches[2], patches[3]: + response = client.patch( + f"/api/v1/pipelines/{PIPELINE_ID}", + json={"status": "failed"}, + ) + assert response.status_code == 200 + + assert not loops[0].stop_calls + assert not loops[1].stop_calls + + +# --------------------------------------------------------------------------- +# Layer 2 — a loop stopped mid-tick refuses the spawn it was about to make +# --------------------------------------------------------------------------- + + +def _loop_with_stub_spawner(spawn_log): + spawner = SimpleNamespace( + spawn_event=lambda **kwargs: spawn_log.append(kwargs) or SimpleNamespace(container_id="c1") + ) + tracker = MagicMock() + return OrchestratorEventLoop( + tracker, + spawner, + pipeline_id=PIPELINE_ID, + slice_id="slice-3", + phase="implement", + roles=["coder"], + ) + + +def test_stopped_loop_refuses_to_spawn_mid_tick(): + """``stop()`` lands from another thread (the cancel route), so a tick + already in flight must re-check before requesting a Job.""" + spawn_log: list[dict] = [] + loop = _loop_with_stub_spawner(spawn_log) + + with patch( + "event_loop._derive_next_action", + return_value=("propose", {"version": 1}, "reason"), + ): + # Sanity: with the loop running, this derivation *does* spawn. + decision = loop._handle_role("coder") + assert decision.spawned is True + assert len(spawn_log) == 1 + + loop._stop.set() + blocked = loop._handle_role("coder") + + assert blocked.spawned is False + assert blocked.blocked == "stopped" + assert len(spawn_log) == 1, "a stopped loop spawned another one-shot Job" + + +def test_stopped_block_does_not_read_as_an_operator_wedge(): + """``blocked="stopped"`` is a teardown, not something an operator can + resolve — it must not trip the arms-exhausted / arms-parked alerts.""" + spawn_log: list[dict] = [] + loop = _loop_with_stub_spawner(spawn_log) + exhausted_alerts: list[tuple] = [] + parked_alerts: list[tuple] = [] + loop._arms_exhausted_notifier = lambda **kw: exhausted_alerts.append(kw) + loop._arms_parked_notifier = lambda **kw: parked_alerts.append(kw) + loop._stop.set() + + with patch( + "event_loop._derive_next_action", + return_value=("propose", {"version": 1}, "reason"), + ): + decisions = loop.poll_once(["coder"]) + + assert [d.blocked for d in decisions] == ["stopped"] + assert not exhausted_alerts + assert not parked_alerts + + +# --------------------------------------------------------------------------- +# Layer 3 — the phase poll loop re-reads the persisted status +# --------------------------------------------------------------------------- + + +def _store_returning(pipeline): + store = MagicMock() + store.load_pipeline.return_value = pipeline + return store + + +def test_phase_bail_reason_reports_cancellation(): + pipeline = _cancellable_pipeline(status=PipelineStatus.CANCELLED) + epoch = pipeline.run_epoch or pipeline.created_at + assert ( + pipelines_pkg._phase_bail_reason_impl( + store=_store_returning(pipeline), pipeline_id=PIPELINE_ID, run_epoch=epoch + ) + == "pipeline_cancelled" + ) + + +def test_phase_bail_reason_still_reports_supersession(): + """The pre-existing #3315 condition keeps working through the same load.""" + pipeline = _cancellable_pipeline() + pipeline.run_epoch = datetime.now(UTC) + stale = pipeline.run_epoch - timedelta(hours=1) + assert ( + pipelines_pkg._phase_bail_reason_impl( + store=_store_returning(pipeline), pipeline_id=PIPELINE_ID, run_epoch=stale + ) + == "superseded_by_restart" + ) + + +def test_phase_bail_reason_is_none_for_a_healthy_run(): + pipeline = _cancellable_pipeline() + epoch = pipeline.run_epoch or pipeline.created_at + assert ( + pipelines_pkg._phase_bail_reason_impl( + store=_store_returning(pipeline), pipeline_id=PIPELINE_ID, run_epoch=epoch + ) + is None + ) + + +def test_phase_bail_reason_ignores_failed(): + """#1273: reconciliation can mark a live pipeline FAILED mid-poll and the + consensus-complete branch recovers it. Bailing would break that.""" + pipeline = _cancellable_pipeline(status=PipelineStatus.FAILED) + epoch = pipeline.run_epoch or pipeline.created_at + assert ( + pipelines_pkg._phase_bail_reason_impl( + store=_store_returning(pipeline), pipeline_id=PIPELINE_ID, run_epoch=epoch + ) + is None + ) + + +def test_phase_bail_reason_tolerates_a_store_hiccup(): + """A transient load failure must never tear down a running phase.""" + store = MagicMock() + store.load_pipeline.side_effect = OSError("state branch locked") + assert ( + pipelines_pkg._phase_bail_reason_impl( + store=store, pipeline_id=PIPELINE_ID, run_epoch=datetime.now(UTC) + ) + is None + ) + assert ( + pipelines_pkg._phase_bail_reason_impl( + store=None, pipeline_id=PIPELINE_ID, run_epoch=datetime.now(UTC) + ) + is None + ) + + +def test_pipeline_cancelled_helper(): + for status, expected in ( + (PipelineStatus.CANCELLED, True), + (PipelineStatus.RUNNING, False), + (PipelineStatus.FAILED, False), + ): + pipeline = _cancellable_pipeline(status=status) + assert ( + pipelines_pkg._pipeline_cancelled(_store_returning(pipeline), PIPELINE_ID) is expected + ) + + broken = MagicMock() + broken.load_pipeline.side_effect = RuntimeError("boom") + assert pipelines_pkg._pipeline_cancelled(broken, PIPELINE_ID) is False + assert pipelines_pkg._pipeline_cancelled(None, PIPELINE_ID) is False + + +# --------------------------------------------------------------------------- +# Layer 3b — a cancelled phase must not be rewritten to FAILED +# --------------------------------------------------------------------------- + + +def test_cancelled_phase_is_not_marked_failed(): + """``_run_concurrent_phase`` returns non-zero on the cancel bail. Falling + through to the failure path would overwrite the operator's CANCELLED with + FAILED — losing their intent and the CANCELLED-only worktree preservation + (#1725) that ``restart_phase`` resumes from.""" + pipeline = _cancellable_pipeline() + epoch = pipeline.run_epoch or pipeline.created_at + + # A real store re-reads from disk on every call, so each load yields a + # fresh object carrying the persisted CANCELLED the cancel route wrote. + # (Returning one shared object would let the phase-start "reset status to + # RUNNING" write at the top of the cycle mask the cancel.) + def _load(_pipeline_id): + cancelled = _cancellable_pipeline(status=PipelineStatus.CANCELLED) + cancelled.run_epoch = epoch + return cancelled + + store = MagicMock() + store.load_pipeline.side_effect = _load + + with patch.object( + pipelines_pkg, + "_run_concurrent_phase", + return_value=(1, "Phase monitor thread exited: pipeline_cancelled."), + ): + _pipeline, _phase_exec, phase_failed, action = pipelines_pkg._run_phase_execution( + pipeline, + pipeline.get_phase_execution(PipelinePhase.PLAN), + False, + certs_volume=None, + current_phase=PipelinePhase.PLAN, + gateway_mode="public", + pipeline_id=PIPELINE_ID, + pipeline_mode="issue", + repo_volumes={}, + repos=["owner/repo"], + run_epoch=epoch, + sandbox_env={}, + spawner=MagicMock(), + store=store, + worktree_repo_path=pipelines_pkg.Path("/tmp/does-not-matter"), + ) + + assert action == "return", "the driver thread must exit cleanly on a cancel" + assert phase_failed is False + saved_statuses = [call.args[0].status for call in store.save_pipeline.call_args_list] + assert PipelineStatus.FAILED not in saved_statuses, ( + "a cancelled phase must not be persisted as FAILED" + ) + + +# --------------------------------------------------------------------------- +# Layer 4 — the slice loop stops admitting slices +# --------------------------------------------------------------------------- + + +def _count_ready(scheduler) -> list: + """Record that the loop read the ready set, and hand back an empty wave + so the control test never enters the spawn machinery.""" + scheduler.iter_ready_calls += 1 + return [] + + +def _pending_slice(): + """The un-admitted slice the cancelled pipeline must never reach.""" + from egg_contracts.models import Slice + + return Slice( + id="slice-3", + name="slice-3", + goal="the slice a cancelled pipeline must not admit", + dependencies=["slice-1"], + ) + + +class _StubScheduler: + """Scheduler with one un-admitted slice left — the #3633 shape.""" + + def __init__(self, *_a, **_kw) -> None: + self.iter_ready_calls = 0 + self.spawned: list[str] = [] + + def all_done(self) -> bool: + return False + + def iter_ready(self): + self.iter_ready_calls += 1 + return iter([("slice-3", "slice-1")]) + + def mark_spawned(self, slice_id: str) -> None: + self.spawned.append(slice_id) + + def record_complete(self, slice_id: str) -> None: # pragma: no cover - unused + pass + + def list_slices(self): + # Real ``SchedulerSliceState``, not the bare string: the guard's + # ``rt.state != SchedulerSliceState.COMPLETE`` comprehension must be + # exercised against the enum production actually yields. + return [SimpleNamespace(slice_id="slice-3", state=SchedulerSliceState.READY)] + + def poll_cascades(self): # pragma: no cover - unused + return [] + + +def test_slice_loop_admits_nothing_after_a_cancel(): + """The reported failure, end to end: a pipeline cancelled mid-implement + with an un-admitted slice remaining must not admit it, must not create + its integration branch, and must not spawn its agent cohort.""" + scheduler = _StubScheduler() + contract = SimpleNamespace(slices=[_pending_slice()]) + pipeline = SimpleNamespace( + # ``repo=None`` skips the origin-side bootstrap probe so the test + # never reaches the gateway. + repo=None, + branch=f"egg/{PIPELINE_ID}/work", + issue_number=3633, + current_phase=PipelinePhase.IMPLEMENT, + config=SimpleNamespace(max_parallel_slices=2), + ) + cancelled = _cancellable_pipeline(status=PipelineStatus.CANCELLED) + store = MagicMock() + store.load_pipeline.return_value = cancelled + + spawner = MagicMock() + reconciler_stop = threading.Event() + + with ( + patch("orchestrator.slice_scheduler.SliceScheduler", lambda *a, **kw: scheduler), + patch("slice_scheduler.SliceScheduler", lambda *a, **kw: scheduler), + patch("egg_contracts.loader.load_contract", return_value=contract), + patch("egg_contracts.loader.save_contract"), + patch.object(pipelines_pkg, "_open_context_pr_safety_net_impl", return_value=None), + patch.object(pipelines_pkg, "_classify_non_complete_slice", return_value="fresh"), + patch.object( + pipelines_pkg, + "_start_stacked_pr_reconciler", + return_value=(MagicMock(), reconciler_stop), + ), + ): + exit_code, logs = pipelines_pkg._run_implement_phase_slices( + PIPELINE_ID, + pipeline, + spawner=spawner, + repo_volumes={}, + gateway_mode="public", + repos=[], + sandbox_env={}, + store=store, + certs_volume=None, + worktree_repo_path=pipelines_pkg.Path("/tmp/does-not-matter"), + # Production (``_run_phase.py``) always threads the owning + # thread's epoch through, so pass it rather than leaning on the + # ``None`` default. Matching epoch here isolates the cancel arm; + # the supersession arm of the same guard is exercised by + # ``test_slice_loop_stops_when_a_restart_supersedes_it``. + run_epoch=cancelled.run_epoch or cancelled.created_at, + ) + + assert exit_code == 1 + assert "pipeline_cancelled" in logs + assert scheduler.iter_ready_calls == 0, "a cancelled pipeline read the ready set" + assert scheduler.spawned == [], "a cancelled pipeline admitted a slice" + assert spawner.gateway.create_slice_integration_branch.call_count == 0 + assert spawner.spawn_agent_job.call_count == 0 + assert reconciler_stop.is_set(), "the stacked-PR reconciler must be torn down" + + +def test_slice_loop_stops_when_a_restart_supersedes_it(): + """The other arm of layer 4's guard (#3315). A restart bumps ``run_epoch`` + and starts a new ``_run_pipeline`` thread; the old slice loop must stop + rather than race it — 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.""" + scheduler = _StubScheduler() + contract = SimpleNamespace(slices=[_pending_slice()]) + pipeline = SimpleNamespace( + repo=None, + branch=f"egg/{PIPELINE_ID}/work", + issue_number=3633, + current_phase=PipelinePhase.IMPLEMENT, + config=SimpleNamespace(max_parallel_slices=2), + ) + restarted = _cancellable_pipeline() + restarted.run_epoch = datetime.now(UTC) + store = MagicMock() + store.load_pipeline.return_value = restarted + + spawner = MagicMock() + + with ( + patch("orchestrator.slice_scheduler.SliceScheduler", lambda *a, **kw: scheduler), + patch("slice_scheduler.SliceScheduler", lambda *a, **kw: scheduler), + patch("egg_contracts.loader.load_contract", return_value=contract), + patch("egg_contracts.loader.save_contract"), + patch.object(pipelines_pkg, "_open_context_pr_safety_net_impl", return_value=None), + patch.object(pipelines_pkg, "_classify_non_complete_slice", return_value="fresh"), + patch.object( + pipelines_pkg, + "_start_stacked_pr_reconciler", + return_value=(MagicMock(), threading.Event()), + ), + ): + exit_code, logs = pipelines_pkg._run_implement_phase_slices( + PIPELINE_ID, + pipeline, + spawner=spawner, + repo_volumes={}, + gateway_mode="public", + repos=[], + sandbox_env={}, + store=store, + certs_volume=None, + worktree_repo_path=pipelines_pkg.Path("/tmp/does-not-matter"), + # The stale thread's epoch — one restart behind the persisted one. + run_epoch=restarted.run_epoch - timedelta(hours=1), + ) + + assert exit_code == 1 + assert "superseded_by_restart" in logs + assert scheduler.iter_ready_calls == 0, "a superseded slice loop read the ready set" + assert scheduler.spawned == [], "a superseded slice loop admitted a slice" + assert spawner.gateway.create_slice_integration_branch.call_count == 0 + + +def test_slice_loop_keeps_running_while_the_pipeline_is_running(): + """Control: the guard must not stop a healthy run.""" + scheduler = _StubScheduler() + contract = SimpleNamespace(slices=[_pending_slice()]) + pipeline = SimpleNamespace( + repo=None, + branch=f"egg/{PIPELINE_ID}/work", + issue_number=3633, + current_phase=PipelinePhase.IMPLEMENT, + config=SimpleNamespace(max_parallel_slices=2), + ) + running = _cancellable_pipeline() + store = MagicMock() + store.load_pipeline.return_value = running + + # Let the loop reach the ready-set read once, then claim completion so + # it exits without running a slice through the spawn machinery. + calls = {"n": 0} + + def _all_done() -> bool: + calls["n"] += 1 + return calls["n"] > 2 + + scheduler.all_done = _all_done # type: ignore[method-assign] + scheduler.iter_ready = lambda: iter(_count_ready(scheduler)) # type: ignore[method-assign] + + with ( + patch("orchestrator.slice_scheduler.SliceScheduler", lambda *a, **kw: scheduler), + patch("slice_scheduler.SliceScheduler", lambda *a, **kw: scheduler), + patch("egg_contracts.loader.load_contract", return_value=contract), + patch("egg_contracts.loader.save_contract"), + patch.object(pipelines_pkg, "_open_context_pr_safety_net_impl", return_value=None), + patch.object(pipelines_pkg, "_classify_non_complete_slice", return_value="fresh"), + patch.object( + pipelines_pkg, + "_start_stacked_pr_reconciler", + return_value=(MagicMock(), threading.Event()), + ), + ): + pipelines_pkg._run_implement_phase_slices( + PIPELINE_ID, + pipeline, + spawner=MagicMock(), + repo_volumes={}, + gateway_mode="public", + repos=[], + sandbox_env={}, + store=store, + certs_volume=None, + worktree_repo_path=pipelines_pkg.Path("/tmp/does-not-matter"), + # As above: production-faithful, but not the epoch arm under + # test — ``all_done()`` short-circuits before the retry wrapper. + run_epoch=running.run_epoch or running.created_at, + ) + + assert scheduler.iter_ready_calls >= 1, "the guard stopped a RUNNING pipeline" + + +# --------------------------------------------------------------------------- +# Layer 5 — the HITL gate bails instead of reading a cancel as an approval +# --------------------------------------------------------------------------- +# +# The four layers above all key on the *persisted* status. The refine/plan +# gate is the one path that overwrites that status before they read it: it +# parks at AWAITING_HUMAN, blocks in ``wait_for_decision``, and the operator's +# cancel unblocks it by cancelling the decision — leaving no ``resolution``. +# An unset resolution reads as an approval (``"" in _APPROVE_KEYWORDS``), so +# the gate took its "Approved — resume and advance" branch, wrote RUNNING over +# the operator's CANCELLED, and let the driver advance into the next phase and +# mint a fresh cohort: #3633 verbatim, through the one door the +# persisted-status layers cannot watch. + + +def _gate_pipeline(status=PipelineStatus.AWAITING_HUMAN) -> Pipeline: + """A pipeline parked at the plan gate with a pending phase_gate decision. + + The pending decision routes the gate down its ``existing_pending_gate`` + branch, so the test reaches ``wait_for_decision`` without touching draft + reads or the decision queue's create path. + """ + pipeline = Pipeline( + id=PIPELINE_ID, + issue_number=3633, + repo="owner/repo", + branch=f"egg/{PIPELINE_ID}/work", + status=status, + current_phase=PipelinePhase.PLAN, + ) + pipeline.decisions = [ + HITLDecision( + id="gate-1", + question="Approve the plan?", + decision_type="phase_gate", + phase=PipelinePhase.PLAN, + status=DecisionStatus.PENDING, + ) + ] + return pipeline + + +def _gate_decision(status, resolution=None) -> HITLDecision: + return HITLDecision( + id="gate-1", + question="Approve the plan?", + decision_type="phase_gate", + phase=PipelinePhase.PLAN, + status=status, + resolution=resolution, + ) + + +class _StatusCell: + """The pipeline status the fake store persists, mutable mid-run. + + A cancel is not a value the gate is handed — it is a write that lands on + the store while the gate is parked in ``wait_for_decision``. Tests for the + later bail sites flip this from inside a hook to model exactly that. + """ + + def __init__(self, status): + self.status = status + + def cancel(self): + self.status = PipelineStatus.CANCELLED + + +def _run_gate( + resolved_decision, + *, + persisted_status=None, + cell=None, + pipeline=None, + ledger_status=("", False, None, {}), + attestation=None, + bridge=None, + on_wait=None, + on_draft=None, + dq=None, +): + """Drive ``_run_hitl_gate_converge`` to its phase-gate wait and back. + + ``load_pipeline`` hands out a *fresh* object per call, as the real store + does: the gate mutates what it loads, and a shared MagicMock return value + would let its own ``AWAITING_HUMAN`` write mask the persisted status the + bail re-reads. + + ``save_pipeline`` writes the status back into the cell, which is what makes + the lost-update mode testable at all: in the real store a park write + *becomes* the persisted status, so a gate that parks AWAITING_HUMAN over a + cancel and then re-reads would read back its own write and sail on. A fake + that only records saves without applying them can never reproduce that. + + The hooks steer the gate onto whichever branch's bail is under test: + ``ledger_status`` is the ``_collect_decision_ledger_status`` 4-tuple (set + ``missing=True`` for the backstop, ``explicit_none`` for the attestation + gate), ``attestation`` / ``bridge`` stand in for the two helpers that block + on their own waits, and ``on_wait`` fires on every ``wait_for_decision``. + Each receives the ``_StatusCell`` so it can cancel from inside the wait. + + ``on_draft`` fires from ``_read_phase_draft``, which is the *pre*-wait + window: it sits after the phase's own cancel bail and before both the + create arm's ``queue_decision`` and the park write, so it models a cancel + landing during the seconds-to-minutes of git IO the gate does before it + ever blocks — the window the post-wait checks cannot see. + + Pass ``dq`` to supply your own decision-queue mock when a test needs to + assert on what the gate did — or did not — queue. + """ + cell = cell if cell is not None else _StatusCell(persisted_status) + saved: list[PipelineStatus] = [] + store = MagicMock() + store.load_pipeline.side_effect = lambda _pid: _gate_pipeline(status=cell.status) + + def _save(p, *_args, **_kwargs): + saved.append(p.status) + cell.status = p.status + + store.save_pipeline.side_effect = _save + + waits: list[str] = [] + + def _wait(decision_id): + waits.append(decision_id) + if on_wait is not None: + on_wait(len(waits), cell) + return resolved_decision + + dq = dq if dq is not None else MagicMock() + dq.wait_for_decision.side_effect = _wait + dq.get_decision.return_value = resolved_decision + + def _attestation(**kwargs): + if attestation is not None: + attestation(cell) + return False, "", kwargs["pipeline"] + + def _bridge(*args, **kwargs): + if bridge is not None: + return bridge(cell) + return 0 + + def _draft(*_args, **_kwargs): + if on_draft is not None: + on_draft(cell) + return "draft body" + + with ( + patch.object( + pipelines_pkg, + "_collect_decision_ledger_status", + return_value=ledger_status, + ), + patch.object(pipelines_pkg, "_persist_decision_ledger_summary", return_value=None), + patch.object( + pipelines_pkg, + "_handle_explicit_none_attestation_gate", + side_effect=_attestation, + ), + patch.object(pipelines_pkg, "get_decision_queue", return_value=dq), + patch.object(pipelines_pkg, "get_pipeline_state_lock"), + patch.object(pipelines_pkg, "report_pipeline_status"), + patch.object(pipelines_pkg, "_emit_pipeline_event"), + patch.object(pipelines_pkg, "_read_phase_draft", side_effect=_draft), + patch.object(pipelines_pkg, "_read_human_phase_draft", return_value=None), + patch.object(pipelines_pkg, "_queue_and_await_contract_decisions", side_effect=_bridge), + patch.object(pipelines_pkg, "_persist_phase_gate_resolution"), + patch.object(pipelines_pkg, "_commit_statefiles_to_worktree"), + ): + _pipeline, action = pipelines_pkg._run_hitl_gate_converge( + pipeline if pipeline is not None else _gate_pipeline(), + current_phase=PipelinePhase.PLAN, + gateway_mode="public", + pipeline_id=PIPELINE_ID, + repo_path=Path("/repo"), + spawner=MagicMock(), + store=store, + worktree_repo_path=Path("/tmp/egg-worktree"), + ) + return action, saved + + +def test_gate_bails_when_the_cancel_route_cancels_its_decision(): + """The headline regression: cancelling a pipeline parked at a HITL gate + must not resurrect it to RUNNING and advance the phase.""" + # What ``cancel_decision`` leaves behind: CANCELLED, no resolution. + action, saved = _run_gate( + _gate_decision(DecisionStatus.CANCELLED), + persisted_status=PipelineStatus.CANCELLED, + ) + + assert action == "break", "the gate must exit the driver loop on a cancel" + assert PipelineStatus.RUNNING not in saved, ( + "the gate rewrote the operator's CANCELLED back to RUNNING" + ) + # Stronger than "no RUNNING": the gate must not persist *any* non-terminal + # status over CANCELLED. The park write is a status write too, and because + # the fake now applies saves back onto the cell, an AWAITING_HUMAN park here + # would be read back by the post-wait check as "not cancelled" and the gate + # would advance — the lost-update mode the in-lock park check closes. + assert saved == [], f"the gate persisted {saved} over the operator's CANCELLED" + + +def test_gate_bails_on_a_cancelled_pipeline_even_if_the_decision_resolved(): + """A cancel that lands after the decision-queue sweep — or one racing an + operator who resolved the gate — must still bail: the persisted status is + checked alongside the decision's own.""" + action, saved = _run_gate( + _gate_decision(DecisionStatus.RESOLVED, "approve"), + persisted_status=PipelineStatus.CANCELLED, + ) + + assert action == "break" + assert PipelineStatus.RUNNING not in saved + + +def test_gate_still_advances_a_genuine_approval(): + """Control: a real approval on a live pipeline still advances. Without + this, ``return "break"`` unconditionally would pass the two above.""" + action, saved = _run_gate( + _gate_decision(DecisionStatus.RESOLVED, "approve"), + persisted_status=PipelineStatus.AWAITING_HUMAN, + ) + + assert action is None, "an approved gate must fall through and advance" + assert PipelineStatus.RUNNING in saved + + +def test_gate_still_advances_when_only_the_decision_was_cancelled(): + """A *lone* decision cancel on a live pipeline is not a pipeline cancel. + + ``routes/decisions/_lifecycle.py`` exposes a standalone endpoint that + cancels one decision without touching the pipeline. Keying the bail on the + returned decision's status would read that as a stop, exit the driver, and + strand a live pipeline at AWAITING_HUMAN with no waiter — which is why the + bail consults the persisted pipeline status only. + """ + action, saved = _run_gate( + _gate_decision(DecisionStatus.CANCELLED), + persisted_status=PipelineStatus.AWAITING_HUMAN, + ) + + assert action is None, "a lone decision cancel must not break the driver loop" + assert PipelineStatus.RUNNING in saved + + +def test_gate_bails_when_cancelled_at_the_ledger_backstop(): + """The decision-ledger backstop (#3390) blocks on its own wait before the + phase gate is ever queued. A cancel landing there must not fall through to + the gate — the ``proceed`` branch treats a non-RESOLVED backstop as an + operator override and walks straight into the phase gate below.""" + action, saved = _run_gate( + _gate_decision(DecisionStatus.CANCELLED), + persisted_status=PipelineStatus.AWAITING_HUMAN, + ledger_status=("no ledger", True, None, {}), + on_wait=lambda n, cell: cell.cancel(), + ) + + assert action == "break" + assert PipelineStatus.RUNNING not in saved + + +def test_gate_bails_when_cancelled_at_the_attestation_gate(): + """The explicit-none attestation gate (#3462) "fails open to the phase + gate" on any non-RESOLVED status — safe for a cancelled attestation, not + safe for a cancelled pipeline. Falling through would queue a *fresh* + phase_gate decision minted after the cancel route already swept the queue, + so nothing would ever cancel it and the wait below would never return.""" + action, saved = _run_gate( + _gate_decision(DecisionStatus.RESOLVED, "approve"), + persisted_status=PipelineStatus.AWAITING_HUMAN, + ledger_status=("attested none", False, ("coder", "abc1234", []), {}), + attestation=lambda cell: cell.cancel(), + ) + + assert action == "break" + assert PipelineStatus.RUNNING not in saved + + +def test_gate_bails_when_cancelled_at_the_followup_specifics(): + """A bare "request changes" queues a follow-up asking for specifics and + blocks on it. That wait is swept by a cancel exactly like the gate's own, + and an unset follow-up resolution reads as an approval.""" + action, saved = _run_gate( + _gate_decision(DecisionStatus.RESOLVED, "request changes"), + persisted_status=PipelineStatus.AWAITING_HUMAN, + # Wait 1 is the phase gate (still live); wait 2 is the follow-up. + on_wait=lambda n, cell: cell.cancel() if n == 2 else None, + ) + + assert action == "break" + assert PipelineStatus.RUNNING not in saved + + +def test_gate_bails_when_cancelled_bridging_contract_decisions(): + """The contract-decision bridge (#1889) is the longest human-latency + window in the gate — one blocking wait per contract question. A cancel + there leaves every answer unresolved, so the converge branch is skipped and + control falls through to "Approved — resume and advance".""" + action, saved = _run_gate( + _gate_decision(DecisionStatus.RESOLVED, "approve"), + persisted_status=PipelineStatus.AWAITING_HUMAN, + bridge=lambda cell: (cell.cancel(), 0)[1], + ) + + assert action == "break" + assert PipelineStatus.RUNNING not in saved, ( + "the gate advanced the phase after the operator cancelled mid-bridge" + ) + + +def _gate_pipeline_without_a_pending_gate() -> Pipeline: + """A pipeline at the plan gate with no pending decision — the *create* arm. + + The reuse arm waits on a decision the cancel route's sweep already reached; + the create arm mints a new one, which is the arm that can leave an orphan. + """ + pipeline = _gate_pipeline() + pipeline.decisions = [] + return pipeline + + +def test_gate_never_mints_a_decision_for_a_cancelled_pipeline(): + """The create arm must not queue a gate for a run the operator stopped. + + The cancel route's sweep of pending decisions is a one-time snapshot, so a + decision minted after it runs is never cancelled — and + ``DecisionQueue.wait_for_decision`` is a ``while True`` poll with no + timeout. Queueing here parks the driver thread for the lifetime of the + process: ``_run_pipeline``'s ``finally`` never runs, so there is no + container cleanup and no ``skip_cleanup`` worktree preservation, and the + operator's PATCH already returned 200. That is strictly worse than the + pre-#3633 behaviour on the same input, where the gate at least fell + through — which is why the check goes *before* ``queue_decision``. + """ + dq = MagicMock() + action, saved = _run_gate( + _gate_decision(DecisionStatus.CANCELLED), + persisted_status=PipelineStatus.AWAITING_HUMAN, + pipeline=_gate_pipeline_without_a_pending_gate(), + # The cancel lands while the gate is reading the draft — after the + # phase's own bail, before the queue. + on_draft=lambda cell: cell.cancel(), + dq=dq, + ) + + assert action == "break" + assert dq.queue_decision.call_args_list == [], ( + "the gate minted a decision nothing will ever cancel" + ) + dq.wait_for_decision.assert_not_called() + assert saved == [] + + +def test_gate_park_does_not_overwrite_a_cancel_that_lands_before_it(): + """The reuse arm's park write must not clobber the operator's CANCELLED. + + This is #3633 verbatim, through the half of the window the post-wait checks + cannot see. The park write is unconditional on both arms, so a cancel + landing before it is overwritten with ``AWAITING_HUMAN``; the decision was + pending at sweep time so the wait returns at once with no resolution; the + post-wait check re-reads the store and sees the gate's *own* write, not the + cancel; and ``"" in _APPROVE_KEYWORDS`` sends the run down "Approved — + resume and advance". Hence the in-lock check in + ``_park_at_gate_unless_cancelled``: ``StateStore.update_pipeline`` takes + the same per-pipeline lock, so checking inside it is what makes the read + and the write atomic against the cancel route. + """ + dq = MagicMock() + action, saved = _run_gate( + # A resolution that *would* advance the phase if the bail were missed. + _gate_decision(DecisionStatus.RESOLVED, "approve"), + persisted_status=PipelineStatus.AWAITING_HUMAN, + on_draft=lambda cell: cell.cancel(), + dq=dq, + ) + + assert action == "break" + assert saved == [], f"the park wrote {saved} over the operator's CANCELLED" + assert dq.wait_for_decision.call_args_list == [], ( + "the gate blocked on a decision belonging to a cancelled run" + ) + + +def test_park_at_gate_unless_cancelled_checks_inside_the_lock(): + """Unit coverage for the shared park helper. + + The ordering assertion is the point: a check that merely runs *just before* + the write still races ``update_pipeline``, which holds the same + per-pipeline lock. Only a check taken after the lock is acquired and before + the save is atomic against the cancel route. + """ + events: list[str] = [] + + lock = MagicMock() + lock.__enter__.side_effect = lambda: events.append("enter") + lock.__exit__.side_effect = lambda *_a: events.append("exit") + + def _store_for(pipeline): + store = MagicMock() + store.load_pipeline.side_effect = lambda _pid: (events.append("load"), pipeline)[1] + store.save_pipeline.side_effect = lambda *_a, **_k: events.append("save") + return store + + live = _store_for(_cancellable_pipeline()) + with patch.object(pipelines_pkg, "get_pipeline_state_lock", return_value=lock): + pipeline, cancelled = pipelines_pkg._park_at_gate_unless_cancelled( + live, PIPELINE_ID, PipelinePhase.PLAN + ) + + assert cancelled is False + assert pipeline.status == PipelineStatus.AWAITING_HUMAN + assert events == ["enter", "load", "save", "exit"], ( + "the status read must sit inside the lock that performs the write" + ) + + events.clear() + dead = _store_for(_cancellable_pipeline(status=PipelineStatus.CANCELLED)) + with patch.object(pipelines_pkg, "get_pipeline_state_lock", return_value=lock): + pipeline, cancelled = pipelines_pkg._park_at_gate_unless_cancelled( + dead, PIPELINE_ID, PipelinePhase.PLAN + ) + + assert cancelled is True + assert pipeline.status == PipelineStatus.CANCELLED + assert events == ["enter", "load", "exit"], "a cancelled run must not be written to" + + +def test_bare_request_changes_on_a_reused_gate_reaches_the_followup(): + """Regression for the reuse path: ``draft_content`` / ``phase_label`` used + to be bound only on the create-a-new-gate arm, so resuming onto an existing + pending gate and answering with a bare "request changes" raised + ``UnboundLocalError`` before the follow-up could be queued.""" + waits: list[int] = [] + action, saved = _run_gate( + _gate_decision(DecisionStatus.RESOLVED, "request changes"), + persisted_status=PipelineStatus.AWAITING_HUMAN, + on_wait=lambda n, cell: waits.append(n), + ) + + assert waits == [1, 2], "the follow-up asking for specifics was never queued" + # The follow-up came back bare too, which the gate reads as an approval. + assert action is None + assert PipelineStatus.RUNNING in saved + + +def test_gate_wait_cancelled_helper(): + """Unit coverage for the seam itself, including the store-hiccup + tolerance it inherits from ``_pipeline_cancelled``.""" + live = _store_returning(_cancellable_pipeline()) + dead = _store_returning(_cancellable_pipeline(status=PipelineStatus.CANCELLED)) + + # A cancelled pipeline bails. Both real cancel paths persist CANCELLED + # before sweeping the decision queue, so a swept wait always sees it. + assert pipelines_pkg._gate_wait_cancelled(dead, PIPELINE_ID) is True + # A live pipeline proceeds. + assert pipelines_pkg._gate_wait_cancelled(live, PIPELINE_ID) is False + # FAILED is not a cancel (#1273): container_monitor can mark a live + # pipeline FAILED mid-gate and the consensus-complete path recovers it. + assert ( + pipelines_pkg._gate_wait_cancelled( + _store_returning(_cancellable_pipeline(status=PipelineStatus.FAILED)), + PIPELINE_ID, + ) + is False + ) + # A store hiccup must never invent a cancel and strand an approved gate. + broken = MagicMock() + broken.load_pipeline.side_effect = RuntimeError("state branch locked") + assert pipelines_pkg._gate_wait_cancelled(broken, PIPELINE_ID) is False + + +# --------------------------------------------------------------------------- +# The pre-spawn guard sits between executor construction and spawn_all +# --------------------------------------------------------------------------- + + +def test_pre_spawn_guard_runs_before_spawn_all(): + """A cancel landing in the prompt-build/session-setup window must stop the + cohort being minted at all. + + That window is tens of seconds wide, and a cancel inside it runs the + route's teardown BEFORE these Jobs exist — nothing would reap them, since + no reconciler acts on CANCELLED and ``cleanup_pipeline`` only re-runs on + an operator DELETE. So the guard has to be the last thing before + ``spawn_all``, not merely present somewhere in the function. + """ + cancelled = _cancellable_pipeline(status=PipelineStatus.CANCELLED) + cancelled.base_branch = "main" + cancelled.current_phase = PipelinePhase.PLAN + store = MagicMock() + store.load_pipeline.return_value = cancelled + + executor = MagicMock() + spawner = MagicMock() + + with ( + patch("concurrent_executor.ConcurrentPhaseExecutor", return_value=executor), + patch.object(pipelines_pkg, "_build_agent_prompt", return_value="prompt"), + ): + exit_code, logs = pipelines_pkg._run_concurrent_phase( + PIPELINE_ID, + cancelled, + "plan", + spawner, + {}, + "public", + ["owner/repo"], + {}, + store, + None, + Path("/tmp/egg-worktree"), + ) + + assert exit_code == 1 + assert "pipeline_cancelled" in logs + executor.spawn_all.assert_not_called() + # The executor owns a live event loop the moment it is constructed, so + # bailing without stopping it would leak the very thing layer 2 stops. + executor.stop_event_loop.assert_called_once() + + +# --------------------------------------------------------------------------- +# Layer 6 — the unresolved-gap gate's bail has to reach the driver +# --------------------------------------------------------------------------- +# +# The gap gate is the one park-and-resume block whose caller, not the block +# itself, decides the pipeline's fate. Skipping the RUNNING write leaves the +# operator's CANCELLED intact for exactly as long as it takes control to +# return to ``_run_pipeline``: IMPLEMENT is terminal +# (``PHASE_TRANSITIONS[IMPLEMENT] == []``), so an unstopped driver walks +# straight into its "pipeline complete" branch, writes COMPLETE over the +# cancel, broadcasts "Pipeline completed successfully", and — now that the +# ``finally`` no longer sees CANCELLED — deletes the worktrees ``restart_phase`` +# resumes from. The gate returning ``gated=True`` cannot be told apart from an +# ordinary gating, so the stop has to be propagated explicitly (#3633 review +# round 2). These tests drive the containing functions, not the gate alone. + + +def _gap_contract(*, resolved: bool): + """A contract carrying one tester→coder gap, resolved or not.""" + from egg_contracts.models import Contract + + return Contract( + pipeline_id=PIPELINE_ID, + slices=[ + { + "id": "slice-1", + "name": "n", + "tasks": [ + { + "id": "task-1-2", + "description": "d", + "gaps": [ + { + "id": "gap-1", + "from_role": "tester", + "to_role": "coder", + "description": "no error-path test", + "resolved": resolved, + } + ], + } + ], + } + ], + ) + + +def _implement_pipeline(status=PipelineStatus.RUNNING) -> Pipeline: + pipeline = Pipeline( + id=PIPELINE_ID, + issue_number=3633, + repo="owner/repo", + branch=f"egg/{PIPELINE_ID}/work", + base_branch="main", + status=status, + current_phase=PipelinePhase.IMPLEMENT, + ) + return pipeline + + +def _run_implement_advance(*, cancel_during_wait: bool, resolution: str | None): + """Drive ``_run_implement_advance`` over the *real* gap gate. + + ``cancel_during_wait`` models the operator's cancel as a store write that + lands while the gate is parked in ``wait_for_decision`` — the shape the + PATCH route produces (persist CANCELLED, then sweep the queue). + """ + cell = _StatusCell(PipelineStatus.RUNNING) + saved: list[PipelineStatus] = [] + + decision = HITLDecision( + id="gap-gate-1", + question="Resolve the gap?", + decision_type="phase_gate", + phase=PipelinePhase.IMPLEMENT, + status=(DecisionStatus.CANCELLED if resolution is None else DecisionStatus.RESOLVED), + resolution=resolution, + ) + + def _wait(_decision_id): + if cancel_during_wait: + cell.cancel() + return decision + + dq = MagicMock() + dq.queue_decision.return_value = decision + dq.wait_for_decision.side_effect = _wait + + def _load(_pipeline_id): + # A fresh object per load, as the real store does — a shared one would + # let the gate's own AWAITING_HUMAN write mask the persisted cancel. + return _implement_pipeline(status=cell.status) + + store = MagicMock() + store.load_pipeline.side_effect = _load + store.save_pipeline.side_effect = lambda p, *a, **k: saved.append(p.status) + + spawner = MagicMock() + + with ( + patch.object(pipelines_pkg, "get_decision_queue", return_value=dq), + patch.object(pipelines_pkg, "get_pipeline_state_lock"), + patch.object(pipelines_pkg, "report_pipeline_status"), + patch.object(pipelines_pkg, "_emit_event", None), + patch.object(pipelines_pkg, "_commit_statefiles_to_worktree", return_value=True), + patch( + "egg_contracts.loader.load_contract", + side_effect=[_gap_contract(resolved=False), _gap_contract(resolved=True)], + ), + ): + pipeline, action = pipelines_pkg._run_implement_advance( + _implement_pipeline(), + current_phase=PipelinePhase.IMPLEMENT, + gateway_mode="public", + pipeline_id=PIPELINE_ID, + repo_path=Path("/repo"), + spawner=spawner, + store=store, + worktree_repo_path=Path("/tmp/egg-worktree"), + ) + return action, saved, spawner + + +def test_implement_advance_stops_the_driver_on_a_cancel_at_the_gap_gate(): + """The gate's bail is only half the fix — its caller has to stop the driver. + + Returning ``gated`` alone is indistinguishable from an ordinary gating, so + ``_run_pipeline`` fell through to the terminal-phase branch and overwrote + the operator's CANCELLED with COMPLETE. + """ + action, saved, spawner = _run_implement_advance(cancel_during_wait=True, resolution=None) + + assert action == "break", ( + "a cancel inside the gap gate must stop the driver, not just skip the " + "gate's own RUNNING write" + ) + assert PipelineStatus.RUNNING not in saved, ( + "the gap gate rewrote the operator's CANCELLED back to RUNNING" + ) + # A cancelled pipeline must not keep mutating the remote work branch. + spawner.gateway.push_worktree_branch.assert_not_called() + + +def test_implement_advance_still_advances_after_a_genuine_gap_resolution(): + """The bail must not swallow the ordinary path: an operator who resolves + the gap and approves gets the post-gate commit+push and a fall-through.""" + action, saved, spawner = _run_implement_advance(cancel_during_wait=False, resolution="approve") + + assert action is None, "a resolved gap gate must let the driver advance" + assert PipelineStatus.RUNNING in saved + spawner.gateway.push_worktree_branch.assert_called_once() diff --git a/orchestrator/tests/test_contract_decision_bridge.py b/orchestrator/tests/test_contract_decision_bridge.py index b4bbca258..f95d8b485 100644 --- a/orchestrator/tests/test_contract_decision_bridge.py +++ b/orchestrator/tests/test_contract_decision_bridge.py @@ -273,6 +273,171 @@ def test_bridge_excludes_cancelled_decision_from_convergence_count( assert cancelled_d["resolved"] is False +def test_bridge_abandons_remaining_waits_when_the_pipeline_is_cancelled( + tmp_path: Path, +) -> None: + """A cancel mid-bridge must stop the remaining waits (#3633). + + This pins the interleaving the ``cancelled`` predicate actually closes: the + cancel lands *after* pass 1 queued the batch, so the route's one-time sweep + reached every entry. Each swept wait returns immediately with no resolution + — and an unset resolution reads as an approval — so without the predicate + the bridge would walk the whole remaining batch answering the operator's + cancel as consent. It stops after the wait that observed the cancel. + + The predicate is deliberately *not* what saves the other interleaving — a + cancel landing entirely before pass 1, where nothing was queued to sweep + and the first wait would never return. Nothing after that first wait runs, + so no post-wait check can help; that case is closed by the pre-pass-1 check + covered in ``test_bridge_never_queues_for_an_already_cancelled_pipeline`` + (#3633 review round 4). + """ + from routes.pipelines import _queue_and_await_contract_decisions + + identifier = "issue-3633" + _make_contract_file( + tmp_path, + identifier, + decisions=[ + { + "id": f"decision-{n}", + "question": f"Question {n}?", + "type": "hitl", + "phase": "refine", + "options": [], + "resolved": False, + "resolution": None, + "resolved_by": None, + "resolved_at": None, + "debounce_until": None, + } + for n in (1, 2, 3) + ], + ) + + class _CountingQueue(_FakeQueue): + def __init__(self, resolutions): + super().__init__(resolutions) + self.waits = 0 + + def wait_for_decision(self, decision_id: str) -> HITLDecision: + self.waits += 1 + return super().wait_for_decision(decision_id) + + dq = _CountingQueue(resolutions=["a", "b", "c"]) + + resolved_count = _queue_and_await_contract_decisions( + dq, + tmp_path, + "pipeline-id", + identifier, + PipelinePhase.REFINE, + # The operator cancels while the second question is being answered. + cancelled=lambda: dq.waits >= 2, + ) + + assert dq.waits == 2, "the bridge kept waiting after the pipeline was cancelled" + # Only the pre-cancel answer counts — the cancel check runs before the + # count, so the round the cancel landed in contributes nothing. + assert resolved_count == 1 + + data = json.loads((tmp_path / f".egg-state/contracts/{identifier}.json").read_text()) + resolved_ids = [d["id"] for d in data["decisions"] if d["resolved"]] + assert resolved_ids == ["decision-1"] + + +def test_bridge_never_queues_for_an_already_cancelled_pipeline( + tmp_path: Path, +) -> None: + """A cancel that lands before pass 1 must stop the bridge before it queues. + + This is the interleaving no post-wait check can reach (#3633 review round + 4). The cancel route's sweep of pending decisions is a one-time snapshot, + so a batch minted after it runs is never cancelled — and + ``DecisionQueue.wait_for_decision`` is an untimed poll. Queue here and the + first wait blocks for the process lifetime, taking the driver thread and + ``_run_pipeline``'s ``finally`` (container cleanup, worktree preservation) + with it. The pre-pass-1 check is what keeps the batch unminted. + """ + from routes.pipelines import _queue_and_await_contract_decisions + + identifier = "issue-3633-precancel" + _make_contract_file( + tmp_path, + identifier, + decisions=[ + { + "id": f"decision-{n}", + "question": f"Question {n}?", + "type": "hitl", + "phase": "refine", + "options": [], + "resolved": False, + "resolution": None, + "resolved_by": None, + "resolved_at": None, + "debounce_until": None, + } + for n in (1, 2) + ], + ) + + dq = _FakeQueue(resolutions=["a", "b"]) + + resolved_count = _queue_and_await_contract_decisions( + dq, + tmp_path, + "pipeline-id", + identifier, + PipelinePhase.REFINE, + # Already cancelled when the bridge is entered. + cancelled=lambda: True, + ) + + assert resolved_count == 0 + assert dq.queued == [], "the bridge minted decisions nothing will ever cancel" + + +def test_bridge_without_a_cancelled_predicate_answers_every_decision( + tmp_path: Path, +) -> None: + """Control: ``cancelled`` is optional and defaults to never firing, so the + existing callers that omit it keep answering the whole batch.""" + from routes.pipelines import _queue_and_await_contract_decisions + + identifier = "issue-3633-control" + _make_contract_file( + tmp_path, + identifier, + decisions=[ + { + "id": f"decision-{n}", + "question": f"Question {n}?", + "type": "hitl", + "phase": "refine", + "options": [], + "resolved": False, + "resolution": None, + "resolved_by": None, + "resolved_at": None, + "debounce_until": None, + } + for n in (1, 2, 3) + ], + ) + dq = _FakeQueue(resolutions=["a", "b", "c"]) + + resolved_count = _queue_and_await_contract_decisions( + dq, + tmp_path, + "pipeline-id", + identifier, + PipelinePhase.REFINE, + ) + + assert resolved_count == 3 + + def test_bridge_promotes_unsubmitted_feedback(tmp_path: Path) -> None: from routes.pipelines import _queue_and_await_contract_decisions diff --git a/orchestrator/tests/test_hard_reset_recovery.py b/orchestrator/tests/test_hard_reset_recovery.py index 47c4eea66..37638111f 100644 --- a/orchestrator/tests/test_hard_reset_recovery.py +++ b/orchestrator/tests/test_hard_reset_recovery.py @@ -332,6 +332,37 @@ def _hook() -> None: mock_report.assert_called_once() assert mock_report.call_args.kwargs["event_type"] == "pipeline.failed" + def test_cancelled_pipeline_keeps_cancelled_and_skips_the_broadcast(self): + """An operator cancel that unblocked the reconcile pause routes here + too (#3633). Pinning FAILED would overwrite the persisted CANCELLED + every driver work loop keys on, and report a failure the operator never + caused. The teardown hook still runs — it is wanted on either exit.""" + pipeline = MagicMock() + pipeline.status = PipelineStatus.CANCELLED + store = MagicMock() + store.load_pipeline.return_value = pipeline + order: list[str] = [] + + with ( + patch("routes.pipelines.get_pipeline_state_lock"), + patch("routes.pipelines.report_pipeline_status") as mock_report, + patch("routes.pipelines._emit_pipeline_event") as mock_emit, + ): + mock_emit.side_effect = lambda *a, **k: order.append("event") + _fail_pipeline_after_divergence_abort( + "pipe-1", + store, + phase=PipelinePhase.PLAN, + backup_ref="refs/egg-backup/sync-recovery/pipe-1/9", + local_only_commit_shas=("abc1234 foo",), + pre_event_hook=lambda: order.append("hook"), + ) + + assert pipeline.status == PipelineStatus.CANCELLED + store.save_pipeline.assert_not_called() + assert order == ["hook"], "pipeline.failed was broadcast for a cancel" + mock_report.assert_not_called() + class TestSyncWorktreeReconcilingDivergence: """The in-loop pause→reconcile→resume / abort loop (#2979).""" @@ -428,6 +459,113 @@ def test_abort_returns_aborted(self): assert result.diverged_unreconciled is True mock_sync.assert_called_once() + def test_cancel_during_the_pause_stops_without_restoring_running(self): + """The pause is a park-and-resume block: it writes AWAITING_HUMAN, + blocks, and writes RUNNING back once the wait returns. A cancel sweeps + that decision with no resolution, so without an inside check the RUNNING + write would land on top of the operator's CANCELLED and re-admit the + run (#3633). ``aborted=True`` is how the two callers spell "stop".""" + # ``load_pipeline`` hands out a fresh object per call, as the real + # store does — the pause mutates what it loads, so a shared return + # value would let its own AWAITING_HUMAN write mask the cancel. + persisted = {"status": PipelineStatus.RUNNING} + saved: list[PipelineStatus] = [] + store = MagicMock() + + def _load(_pipeline_id): + loaded = MagicMock() + loaded.status = persisted["status"] + return loaded + + store.load_pipeline.side_effect = _load + store.save_pipeline.side_effect = lambda p, *a, **k: saved.append(p.status) + dq = MagicMock() + + def _wait(_decision_id): + # The operator cancels while the pause is blocked here. + persisted["status"] = PipelineStatus.CANCELLED + + dq.wait_for_decision.side_effect = _wait + # What the cancel route leaves behind: swept, no resolution. + dq.get_decision.return_value = MagicMock(resolution=None) + lock_p, persist_p, report_p, emit_p = self._patch_ctx() + with ( + patch( + "routes.pipelines._sync_worktree_with_remote", + return_value=_diverged_outcome(), + ) as mock_sync, + patch("routes.pipelines.get_decision_queue", return_value=dq), + lock_p, + persist_p, + report_p, + emit_p, + ): + _result, aborted = _sync_worktree_reconciling_divergence( + MagicMock(), + "pipe-1", + store, + Path("/repo"), + worktree_repo_path=Path("/wt"), + phase=PipelinePhase.PLAN, + ) + + assert aborted is True + assert PipelineStatus.RUNNING not in saved, ( + "the reconcile pause rewrote the operator's CANCELLED to RUNNING" + ) + # Bailed before the resolution read, so no second sync attempt. + mock_sync.assert_called_once() + + def test_cancel_before_the_pause_is_not_overwritten_by_the_park(self): + """The park write is the other half of the window (#3633 review round 4). + + The pause reloads inside the state lock and writes AWAITING_HUMAN to + the pipeline and its phase box. A cancel landing before that — during + the sync itself, which is git IO over the gateway — used to be + overwritten, and the post-wait check would then read back the pause's + own AWAITING_HUMAN rather than the operator's CANCELLED. Because + ``StateStore.update_pipeline`` takes the same per-pipeline lock, doing + the check inside it makes the read and the write atomic against the + cancel route: nothing is persisted, and no decision is minted for a run + that is already stopped. + """ + saved: list[PipelineStatus] = [] + store = MagicMock() + + def _load(_pipeline_id): + loaded = MagicMock() + loaded.status = PipelineStatus.CANCELLED + return loaded + + store.load_pipeline.side_effect = _load + store.save_pipeline.side_effect = lambda p, *a, **k: saved.append(p.status) + dq = MagicMock() + lock_p, persist_p, report_p, emit_p = self._patch_ctx() + with ( + patch( + "routes.pipelines._sync_worktree_with_remote", + return_value=_diverged_outcome(), + ), + patch("routes.pipelines.get_decision_queue", return_value=dq), + lock_p, + persist_p as mock_persist, + report_p, + emit_p, + ): + _result, aborted = _sync_worktree_reconciling_divergence( + MagicMock(), + "pipe-1", + store, + Path("/repo"), + worktree_repo_path=Path("/wt"), + phase=PipelinePhase.PLAN, + ) + + assert aborted is True + assert saved == [], f"the pause persisted {saved} over the operator's CANCELLED" + mock_persist.assert_not_called() + dq.wait_for_decision.assert_not_called() + def test_reconcile_budget_exhausted_aborts(self): """If every resume re-diverges, the bounded budget eventually aborts rather than pausing forever.""" diff --git a/orchestrator/tests/test_pipeline_failure_path.py b/orchestrator/tests/test_pipeline_failure_path.py index f5abfe85c..d3c8fdad6 100644 --- a/orchestrator/tests/test_pipeline_failure_path.py +++ b/orchestrator/tests/test_pipeline_failure_path.py @@ -484,6 +484,162 @@ def test_worktree_cleanup_skipped_on_failure( # delete_worktrees should NOT be called since pipeline is FAILED mock_gateway.delete_worktrees.assert_not_called() + @patch(_COMMON_PATCHES[7]) + @patch(_COMMON_PATCHES[6]) + @patch(_COMMON_PATCHES[5]) + @patch(_COMMON_PATCHES[4]) + @patch(_COMMON_PATCHES[3]) + @patch(_COMMON_PATCHES[2]) + @patch(_COMMON_PATCHES[1]) + @patch(_COMMON_PATCHES[0]) + def test_worktree_cleanup_skipped_on_cancellation( + self, + mock_emit, + mock_get_spawner, + mock_get_store, + mock_spawn_wait, + mock_state_lock, + mock_build_prompt, + mock_read_draft, + mock_report, + ): + """CANCELLED joins FAILED on the ``skip_cleanup`` arm (#3633 review). + + ``restart_phase`` allowlists CANCELLED precisely so a ``cancel_task`` + run can be resumed without a full resubmission (#1725), and the PATCH + cancel route already passes ``preserve_worktrees=(status == + "cancelled")``. Before this, the driver's own ``finally`` contradicted + both by deleting the worktrees the operator was told they could resume + from — and the #3633 layers make that teardown land seconds after the + cancel rather than at the next consensus timeout, so the two policies + have to agree. The whole justification lives in a comment, which makes + a "tidy-up" back to FAILED-only a silent regression; pin it. + """ + from routes.pipelines import _run_pipeline + + pipeline = _make_running_pipeline() + pipeline.status = PipelineStatus.CANCELLED + mock_store, mock_gateway = _setup_mocks( + mock_report, + mock_read_draft, + mock_build_prompt, + mock_state_lock, + mock_spawn_wait, + mock_get_store, + mock_get_spawner, + mock_emit, + pipeline, + ) + mock_spawner = mock_get_spawner.return_value + mock_spawner.cleanup_pipeline.return_value = 0 + + with ( + patch.dict(os.environ, {"EGG_HOST_REPO_MAP": '{"repo": "/host/repo"}'}, clear=False), + patch("pathlib.Path.exists", return_value=True), + # The ``finally`` reaches the spawner through ``_get_spawner`` + # (runtime-dependent), not the ``get_container_spawner`` seam the + # rest of the harness patches. + patch("routes.pipelines._get_spawner", return_value=mock_spawner), + ): + _run_pipeline("issue-42", Path("/repo")) + + assert pipeline.status == PipelineStatus.CANCELLED + mock_gateway.delete_worktrees.assert_not_called() + + # The safety-net container sweep must still run — only the worktrees + # are preserved, and it has to be told so explicitly. + assert mock_spawner.cleanup_pipeline.call_args is not None, ( + "safety-net container cleanup must still run for a cancelled run" + ) + assert mock_spawner.cleanup_pipeline.call_args.kwargs["preserve_worktrees"] is True + + @patch(_COMMON_PATCHES[7]) + @patch(_COMMON_PATCHES[6]) + @patch(_COMMON_PATCHES[5]) + @patch(_COMMON_PATCHES[4]) + @patch(_COMMON_PATCHES[3]) + @patch(_COMMON_PATCHES[2]) + @patch(_COMMON_PATCHES[1]) + @patch(_COMMON_PATCHES[0]) + def test_terminal_phase_does_not_complete_a_cancelled_pipeline( + self, + mock_emit, + mock_get_spawner, + mock_get_store, + mock_spawn_wait, + mock_state_lock, + mock_build_prompt, + mock_read_draft, + mock_report, + ): + """The terminal-phase branch must not write COMPLETE over a cancel. + + It is the last unguarded status write in the driver loop, and every + park-and-resume block in a terminal phase lands on it: the block bails + without restoring RUNNING (leaving CANCELLED persisted), the driver + reads "no next phase" as success, and the operator's cancel becomes + COMPLETE plus a "Pipeline completed successfully" broadcast — after + which the ``finally`` no longer sees CANCELLED, so ``skip_cleanup`` + stays False and the worktrees ``restart_phase`` resumes from are + deleted (#3633 review round 2). + + The bail is modelled at ``_run_implement_advance`` — the gap gate's + caller, and today's only such block — but the guard is deliberately on + the branch rather than the block, so a future one cannot reintroduce + this by forgetting to propagate. + """ + from routes.pipelines import _run_pipeline + + pipeline = _make_running_pipeline() + pipeline.current_phase = PipelinePhase.IMPLEMENT + execution = pipeline.get_phase_execution(PipelinePhase.IMPLEMENT) + execution.status = PipelineStatus.RUNNING + execution.started_at = datetime.now(UTC) + + mock_store, mock_gateway = _setup_mocks( + mock_report, + mock_read_draft, + mock_build_prompt, + mock_state_lock, + mock_spawn_wait, + mock_get_store, + mock_get_spawner, + mock_emit, + pipeline, + ) + mock_spawner = mock_get_spawner.return_value + mock_spawner.cleanup_pipeline.return_value = 0 + + def _bail_without_propagating(pl, **_kwargs): + """A park-and-resume block that skips its RUNNING write and stops + there — what the gap gate did before its caller learned to stop + the driver.""" + pipeline.status = PipelineStatus.CANCELLED + return pl, None + + with ( + patch.dict(os.environ, {"EGG_HOST_REPO_MAP": '{"repo": "/host/repo"}'}, clear=False), + patch("pathlib.Path.exists", return_value=True), + patch("routes.pipelines._run_concurrent_phase", return_value=(0, "")), + patch( + "routes.pipelines._run_implement_advance", + side_effect=_bail_without_propagating, + ), + patch("routes.pipelines._get_spawner", return_value=mock_spawner), + ): + _run_pipeline("issue-42", Path("/repo")) + + assert pipeline.status == PipelineStatus.CANCELLED, ( + "the terminal-phase branch overwrote the operator's CANCELLED" + ) + completed = [ + c + for c in mock_emit.call_args_list + if len(c.args) >= 2 and c.args[1] == "pipeline.completed" + ] + assert completed == [], "a cancelled pipeline must not broadcast completion" + mock_gateway.delete_worktrees.assert_not_called() + class TestWorktreeCreationFailure: """Verify pipeline fails when worktree creation returns empty worktrees.""" diff --git a/orchestrator/tests/test_restart_phase_consensus_timer.py b/orchestrator/tests/test_restart_phase_consensus_timer.py index 1ba8d0ba7..aecff315b 100644 --- a/orchestrator/tests/test_restart_phase_consensus_timer.py +++ b/orchestrator/tests/test_restart_phase_consensus_timer.py @@ -32,7 +32,7 @@ from routes.pipelines import ( _CONSENSUS_TIMEOUT_HITL_CONTEXT, _cancel_consensus_timeout_decisions, - _pipeline_superseded_by_restart, + _phase_bail_reason_impl, _run_concurrent_phase, _run_concurrent_phase_with_impasse_retry, ) @@ -261,42 +261,62 @@ def test_convergence_withdraws_stale_decision_end_to_end( class TestPipelineSupersededHelper: - """The shared epoch-supersession predicate (facet a).""" + """The epoch-supersession arm of the shared bail predicate (facet a). + + The standalone ``_pipeline_superseded_by_restart`` predicate these cases + used to exercise was folded into ``_phase_bail_reason_impl`` (#3633), so + they now pin the same #3315 semantics on the live implementation — the + one both the poll loop and the impasse-retry wrapper actually call. + """ + + @staticmethod + def _running(run_epoch, created_at=None): + reloaded = MagicMock() + reloaded.status = PipelineStatus.RUNNING + reloaded.run_epoch = run_epoch + reloaded.created_at = created_at if created_at is not None else run_epoch + return reloaded def test_none_run_epoch_is_never_superseded(self): - # Direct-call paths that don't thread an epoch must opt out entirely — - # the helper must not even touch the store. + # Direct-call paths that don't thread an epoch opt out of the epoch + # arm entirely — no epoch, no supersession, whatever is on disk. store = MagicMock() - assert _pipeline_superseded_by_restart(store, "issue-3315", None) is False - store.load_pipeline.assert_not_called() + store.load_pipeline.return_value = self._running(datetime(2030, 1, 1, tzinfo=UTC)) + assert ( + _phase_bail_reason_impl(store=store, pipeline_id="issue-3315", run_epoch=None) is None + ) def test_newer_on_disk_epoch_means_superseded(self): - reloaded = MagicMock() - reloaded.run_epoch = datetime(2030, 1, 1, tzinfo=UTC) - reloaded.created_at = datetime(2030, 1, 1, tzinfo=UTC) store = MagicMock() - store.load_pipeline.return_value = reloaded + store.load_pipeline.return_value = self._running(datetime(2030, 1, 1, tzinfo=UTC)) assert ( - _pipeline_superseded_by_restart(store, "issue-3315", datetime(2020, 1, 1, tzinfo=UTC)) - is True + _phase_bail_reason_impl( + store=store, + pipeline_id="issue-3315", + run_epoch=datetime(2020, 1, 1, tzinfo=UTC), + ) + == "superseded_by_restart" ) def test_matching_epoch_is_not_superseded(self): epoch = datetime(2025, 6, 1, tzinfo=UTC) - reloaded = MagicMock() - reloaded.run_epoch = epoch - reloaded.created_at = epoch store = MagicMock() - store.load_pipeline.return_value = reloaded - assert _pipeline_superseded_by_restart(store, "issue-3315", epoch) is False + store.load_pipeline.return_value = self._running(epoch) + assert ( + _phase_bail_reason_impl(store=store, pipeline_id="issue-3315", run_epoch=epoch) is None + ) - def test_load_failure_returns_false(self): + def test_load_failure_returns_no_bail(self): # A transient store hiccup must never tear down a running phase. store = MagicMock() store.load_pipeline.side_effect = RuntimeError("git read failed") assert ( - _pipeline_superseded_by_restart(store, "issue-3315", datetime(2020, 1, 1, tzinfo=UTC)) - is False + _phase_bail_reason_impl( + store=store, + pipeline_id="issue-3315", + run_epoch=datetime(2020, 1, 1, tzinfo=UTC), + ) + is None ) diff --git a/orchestrator/tests/test_unresolved_gap_gate.py b/orchestrator/tests/test_unresolved_gap_gate.py index e0ae6852a..c87a1e779 100644 --- a/orchestrator/tests/test_unresolved_gap_gate.py +++ b/orchestrator/tests/test_unresolved_gap_gate.py @@ -267,3 +267,60 @@ def test_unresolved_decision_does_not_spin() -> None: gated, _, _ = _run_gate(dq, load_side_effect=[_contract(resolved=False)]) assert gated is True assert len(dq.queued) == 1 + + +def test_pipeline_cancelled_during_the_gate_keeps_cancelled() -> None: + """This gate is a park-and-resume block: AWAITING_HUMAN, block, then + RUNNING back once the wait returns. When what unblocked the wait was the + operator cancelling the pipeline, that RUNNING write lands on top of the + persisted CANCELLED every driver work loop keys on, re-admitting the run + the operator just stopped (#3633). Bail before the status write.""" + from routes.pipelines import _await_unresolved_gap_gate + + persisted = {"status": PipelineStatus.RUNNING} + saved: list[PipelineStatus] = [] + + class _CancellingQueue(_FakeQueue): + def wait_for_decision(self, decision_id: str): + # The operator cancels while the gate is blocked here; the cancel + # route sweeps this decision, leaving no resolution. + persisted["status"] = PipelineStatus.CANCELLED + return super().wait_for_decision(decision_id) + + dq = _CancellingQueue(resolutions=[None]) + + # A fresh object per load, as the real store does — a shared one would let + # the gate's own AWAITING_HUMAN write mask the cancel the bail re-reads. + def _load(_pipeline_id): + pipeline = Pipeline( + id="issue-42", issue_number=42, repo="owner/repo", branch="egg/issue-42" + ) + pipeline.current_phase = PipelinePhase.IMPLEMENT + pipeline.status = persisted["status"] + return pipeline + + store = MagicMock() + store.load_pipeline.side_effect = _load + store.save_pipeline.side_effect = lambda p, *a, **k: saved.append(p.status) + + with ( + patch("routes.pipelines.get_decision_queue", return_value=dq), + patch("routes.pipelines.get_pipeline_state_lock", return_value=nullcontext()), + patch("routes.pipelines.report_pipeline_status"), + patch("routes.pipelines._emit_event", None), + patch("egg_contracts.loader.load_contract", side_effect=[_contract(resolved=False)]), + ): + gated = _await_unresolved_gap_gate( + store, + "issue-42", + Path("/repo"), + Path("/worktree"), + 42, + PipelinePhase.IMPLEMENT, + True, + ) + + assert gated is True + assert PipelineStatus.RUNNING not in saved, ( + "the gap gate rewrote the operator's CANCELLED back to RUNNING" + ) diff --git a/scripts/file-size-allowlist.yaml b/scripts/file-size-allowlist.yaml index ee54979e8..4643b89ce 100644 --- a/scripts/file-size-allowlist.yaml +++ b/scripts/file-size-allowlist.yaml @@ -38,3 +38,11 @@ files: # both sides. orchestrator/slice_green_gate.py: issue: "3627" + # Pushed over the cap by #3633 review-feedback fixes (PR #3649) — the + # cancel-before-spawn guard and cohort reaping in the phase monitor loop. + orchestrator/routes/pipelines/_run_concurrent.py: + issue: "3650" + # Pushed over the cap by #3633 review-feedback fixes (PR #3649) — the + # CANCELLED branch of the driver's worktree-preservation cleanup hook. + orchestrator/routes/pipelines/_run_pipeline.py: + issue: "3651"