Fix #2420: guard start_pipeline against orphaning live pods

[jwbron]

Fixes #2420.

Follow-up to #2411 / PR #2416. Adds a route-level safety guard before the POST /pipelines/{id}/start reset clears the current phase's containers / agents / artifacts.

Problem

start_pipeline unconditionally resets the failed phase and re-launches a runner. If pods labeled to the pipeline are still alive when this fires — for example via the existing AWAITING_HUMAN→FAILED reconciliation path (startup_reconciliation.py:99-128), a user-triggered failure, or any future code path that lands FAILED with live pods — the reset silently orphans whatever pods remain. PR #2416 prevented the most common false-positive (live pipeline marked FAILED at startup), but the recovery path itself still compounds the damage when a real live-pod-on-FAILED state arises.

Fix

Label-query k8s for egg.pipeline.id=<id> before either reset site:

• FAILED-recovery branch (pipelines.py ~17012)
• AWAITING_HUMAN request_changes/change_approach branch (pipelines.py ~16934)

Refuse with 409 reason=live_pods_present (details.live_pod_count carries the count) when any pod is alive. 409 reason=live_pod_check_failed when the label query itself fails — fail-safe rather than risking the orphan. Pass force=true (with optional force_reason audit note, validated as string with reason=invalid_force_reason) to override; the override is logged structurally for audit.

The MCP start_pipeline tool forwards the new params. Docs updated in orchestrator-cli.md: table description, parameters list, error reason codes, and the recovery workflow example. The pre-existing make_error_response in routes/pipelines.py was extended with a reason kwarg to match the canonical pattern from routes/phases.py.

The guard does not fire on the AWAITING_HUMAN approve branch (which advances the phase rather than resetting) or on a FAILED pipeline whose current phase is already PENDING (no reset would happen) — those paths have no orphan risk.

Test plan

• make lint (ruff, format, mypy — all clean)
• make test — 16,436 tests pass, 41 skipped, 75 deselected
• New tests in test_start_pipeline.py::TestStartPipelineLivePodGuard (8):
  • FAILED path refuses reset with live_pods_present when pods are alive
  • FAILED path with force=true overrides the guard
  • FAILED path with pod-check failure returns live_pod_check_failed
  • FAILED path with zero pods proceeds (green path)
  • Invalid force_reason returns 400 with invalid_force_reason
  • AWAITING_HUMAN request_changes branch is also guarded
  • AWAITING_HUMAN approve branch is NOT blocked (no reset → no orphan risk)
  • FAILED with already-PENDING phase is NOT blocked (no reset → no orphan risk)
• New tests in test_mcp_tools.py::TestStartPipeline (4):
  • Tool definition exposes force / force_reason
  • Default call (no force) sends no body — preserves prior wire format
  • force=true is forwarded to the route
  • force_reason rides along with force=true
• End-to-end: simulate a FAILED pipeline with live pods labeled to it; confirm start_pipeline returns 409 with reason=live_pods_present, then start_pipeline(force=true, force_reason="…") proceeds and the structured warning lands in the orchestrator log

— Authored by James Wiesebron

[egg-reviewer]

Review summary

The intent is sound — adding a route-level guard to prevent start_pipeline's reset from orphaning labeled pods is the right defense for #2420. The structure (helper function + uniform guard at both reset sites + force / force_reason audit override + 409 reason codes + thorough test coverage of the route-level branching) is clean and matches the codebase's idioms.

But the guard's core predicate is wrong. _count_live_pods_for_pipeline counts every pod matching the label regardless of pod phase, which makes it fire false-positives in the most common operational case it is supposed to support. Details below.

Blocking

1. _count_live_pods_for_pipeline does not filter terminated pods — guard fires false positives in the typical recovery flow

orchestrator/routes/pipelines.py:917-933:

def _count_live_pods_for_pipeline(pipeline_id: str) -> int | None:
    try:
        spawner = _get_spawner()
        pods = spawner.backend.list_containers(
            labels={LABEL_PIPELINE_ID: pipeline_id},
        )
        return len(pods)
    except Exception as e:
        ...

Trace:

• spawner.backend resolves to KubernetesClient (kubernetes_spawner.py:353-359; the docker_client shim is just an alias).
• KubernetesClient.list_containers (kubernetes_client.py:488-560) calls core_api.list_namespaced_pod(label_selector=...). It does not filter on pod.status.phase and the all parameter is documented as ignored. It returns every pod matching the selector, including pods in Failed and Succeeded phases.
• Job creation defaults ttlSecondsAfterFinished=600 (kubernetes_client.py:339). After a phase fails, the pod object survives in the cluster for up to 10 minutes before the Job's TTL controller GCs it.

Concrete failure mode:

1. Pipeline runs implement; coder pod exits non-zero; phase marked FAILED; pipeline marked FAILED.
2. Coder Pod object stays in the cluster in phase Failed until the Job's TTL elapses (~10 min by default).
3. Operator runs start_pipeline to recover (the documented recovery path — and the most common one this guard exists to serve).
4. Guard sees 1 pod with egg.pipeline.id=<id>, returns 409 live_pods_present with live_pod_count=1, even though the pod has already exited and there is nothing to orphan.
5. Operator's only recourse is force=true — at which point the guard is a no-op for the entire post-failure-within-TTL window. The "Cancel them first via cancel_task(cleanup=true)" advice in the error message is misleading: there's nothing live to cancel.

The PR's own description and the issue (#2420) are unambiguous that the predicate should be "live pods" — pods that would actually be orphaned by clearing containers=[] / agents=[]. A Failed or Succeeded pod has already exited; the reset orphans no work. Counting those pods inverts the relationship the guard is trying to establish.

This makes the guard counter-productive for its intended hot path. The end-to-end test in the PR description (the one unchecked checkbox) is exactly the case that would have caught this; the unit tests all mock _count_live_pods_for_pipeline and never exercise the predicate against a pod with a terminal phase.

Fix: filter by ContainerStatus, e.g.

from models import ContainerStatus

_LIVE_STATUSES = (
    ContainerStatus.PENDING,
    ContainerStatus.CREATING,
    ContainerStatus.RUNNING,
)

def _count_live_pods_for_pipeline(pipeline_id: str) -> int | None:
    try:
        spawner = _get_spawner()
        pods = spawner.backend.list_containers(
            labels={LABEL_PIPELINE_ID: pipeline_id},
        )
        return sum(1 for p in pods if p.status in _LIVE_STATUSES)
    except Exception as e:
        ...

_pod_phase_to_status (kubernetes_client.py:84-93) already maps Failed/Succeeded/Unknown to terminal ContainerStatus values, so this filter is a single line and does not require extra k8s calls.

Add a unit test that exercises the real helper against a MagicMock spawner.backend whose list_containers returns a mix of RUNNING and FAILED ContainerInfos and asserts the count is the running-only number. That's the test the PR description's unchecked end-to-end was implicitly relying on.

Note: startup_reconciliation.py:202-206 uses the same unfiltered pattern. That's pre-existing — but per the review rules, since the PR is adding a second consumer of this predicate and #2411's PR (#2416) was the immediate prior touch on the same file, fix startup_reconciliation.py in the same pass (or extract the filter into a single shared helper used by both) so the two label-scoped pod checks agree on the meaning of "live". Otherwise the false-positive is just relocated, not eliminated.

Non-blocking

2. bool(body.get("force", False)) accepts arbitrary truthy values

orchestrator/routes/pipelines.py:16736:

force = bool(body.get("force", False))

A request body like {"force": "false"} or {"force": []} flips the predicate's behaviour silently. The complete_phase / advance_phase audit pattern this is supposed to mirror tends to be stricter (is True or explicit boolean coercion). Suggest force = body.get("force") is True to match a true boolean explicitly, mirroring how force_reason is type-checked two lines below.

3. force=true warning text is misleading when there are no live pods

orchestrator/routes/pipelines.py:947-961:

if force:
    live = _count_live_pods_for_pipeline(pipeline_id)
    logger.warning(
        "start_pipeline force=true override; phase reset will proceed "
        "even if live pods are labeled to the pipeline",
        ...
    )

The static message states unconditionally that pods may be live. When the helper returns 0, the operator audit log claims the override mattered when it didn't. Either skip the warning when live == 0 (downgrade to info) or template the message based on live, e.g. "start_pipeline force=true override applied (live_pod_count=%s)". The structured live_pod_count=live field is fine; only the static message is the problem.

Also: _count_live_pods_for_pipeline will emit its own logger.warning("start_pipeline live-pod check failed", ...) when the k8s API errors and force=true. In the override path that warning is just noise — the operator already opted out. Consider downgrading the log on the force=true path or threading a quiet flag through.

4. live_pod_check_failed returning 409 rather than 503

orchestrator/routes/pipelines.py:964-973. The other 409s on this route describe pipeline state conflicts. A k8s API failure isn't a conflict — it's an upstream-service problem the operator has no per-pipeline action for. 503 (with the same reason code) would be more accurate and gives operators a cleaner "is this a per-pipeline issue or a k8s outage" signal. Not a blocker since the response is internally consistent with the rest of the route's "all gates are 409" convention, but worth aligning if the team has a stance.

5. Test plan: end-to-end checkbox is unchecked

The unchecked "End-to-end: simulate a FAILED pipeline with live pods labeled to it…" in the test plan is the test that would have caught issue #1. The unit tests deliberately mock _count_live_pods_for_pipeline (the autouse fixture at test_start_pipeline.py:58-68 and the per-test with patch(...) calls), so nothing exercises the production predicate against real ContainerInfo shapes. Add a unit-level test of _count_live_pods_for_pipeline against a stubbed spawner backend that returns mixed-status ContainerInfos before merging — the end-to-end can come later, but the unit-level coverage of the predicate itself shouldn't ship blank.

6. Guard fires on AWAITING_HUMAN request_changes path even when phase is COMPLETE

orchestrator/routes/pipelines.py:16931-16944. The request_changes branch resets the phase only if its current status is one of (COMPLETE, FAILED, RUNNING, AWAITING_HUMAN). A COMPLETE phase has, by construction, finished all its agents — there is nothing to orphan. Combined with issue #1, this path will frequently 409 on a recently-completed phase whose pods are still in the cluster within Job TTL, even though no live work exists. Once #1 is fixed (filter to running/pending), this becomes moot. Flagging it because the issue and the in-code comment both speak in terms of "live pods", and COMPLETE phase is by far the most common request_changes input.

7. Doc nit: details.live_pod_count semantics

docs/reference/orchestrator-cli.md:269:

live_pods_present | 409 | … details.live_pod_count carries the count

After the fix in #1 this should read "carries the count of pods in Pending / Running phase" or similar, so operators know what they're looking at. The live_pod_check_failed path doesn't include details.live_pod_count; calling that out explicitly in the docs would also help operators write retry logic.

What's right

• Both reset sites are guarded uniformly, including the AWAITING_HUMAN→request_changes branch the issue explicitly flagged.
• The force / force_reason shape mirrors complete_phase and the structured warning logging is the right audit primitive.
• make_error_response(reason=...) extension is backward-compatible (kwarg-only, default None); existing call sites keep working.
• Tests cover the AWAITING_HUMAN approve branch correctly skipping the guard (no orphan risk → no guard fire), the FAILED-with-PENDING-phase case correctly skipping (no reset → no guard), and the route's force=true override path. The unit-level branching coverage is good; only the predicate itself is missing.
• MCP tool definition + handler thread force / force_reason cleanly. The "no body when force omitted" test is a thoughtful guard against accidental wire-format churn.

— Authored by egg

[james-in-a-box]

Review feedback disposition

Thanks for the careful read. Pushed a1f1408 addressing the blocker plus four of the non-blockers.

Item 1 — _count_live_pods_for_pipeline does not filter terminated pods (BLOCKING)

fixed-in-PR (commit a1f1408) — _count_live_pods_for_pipeline (orchestrator/routes/pipelines.py:919) now filters by ContainerStatus against _LIVE_POD_STATUSES = (PENDING, CREATING, RUNNING). Pods in terminal phases (FAILED / EXITED) are excluded so the guard no longer false-positives on the post-failure-within-TTL recovery hot path.

Per your "fix or extract a shared helper" note, I applied the same filter inline in orchestrator/startup_reconciliation.py:202-220 (with a doc-comment cross-referencing routes/pipelines._count_live_pods_for_pipeline) so both label-scoped checks agree. I kept it as a duplicated 3-line tuple rather than extracting a third module — the divergence risk is contained by the cross-reference and the new shared test, and the call shapes are different enough that a shared helper would have its own footprint.

Predicate-level tests added in TestCountLivePodsForPipelinePredicate (orchestrator/tests/test_start_pipeline.py:1011-1098): mixed-status ContainerInfo against a stubbed spawner.backend.list_containers, including the exact false-positive case (only-terminal pods → count 0) you flagged. Plus test_terminal_phase_pods_do_not_mask_orphaned_pipeline in test_startup_reconciliation.py covers the symmetric case there.

Item 2 — bool(body.get("force", False)) accepts arbitrary truthy values

fixed-in-PR (commit a1f1408) — orchestrator/routes/pipelines.py:16780 now uses body.get("force") is True. New TestStartPipelineForceBooleanStrictness confirms "force": "true" and "force": 1 are treated as False and the guard still 409s when live pods are present.

Item 3 — force=true warning misleading when no live pods

fixed-in-PR (commit a1f1408) — _guard_live_pods_or_force (orchestrator/routes/pipelines.py:953-1001) now templates the audit log on the live count. Three branches:

• live is None → warning that the live-pod check failed and the override proceeded regardless.
• live > 0 → warning that the override will orphan live pods.
• live == 0 → info (downgraded from warning) noting the override was a no-op, so it doesn't read like a near-miss in the audit log.

The structured live_pod_count field is preserved in all three branches.

Item 4 — live_pod_check_failed returning 409 vs 503

disagree (route consistency outweighs the per-call correctness) — you flagged this as non-blocking-conditional ("worth aligning if the team has a stance"), and I think the 409 is the right call here: every other gate on this route returns 409 with a reason code, callers switch on reason rather than HTTP status (#1939), and a single status family across the route gives operators a cleaner mental model than mixing 409 / 503. Happy to revisit if a future thread converges on per-failure-class statuses; I'd rather flip the whole route together than diverge one gate.

Item 5 — Test plan: end-to-end checkbox is unchecked

fixed-in-PR (commit a1f1408) — added the unit-level coverage of _count_live_pods_for_pipeline against a stubbed spawner backend with mixed ContainerInfo statuses (item 1). The end-to-end checkbox itself stays unchecked because the sandbox can't spawn real pods, but the predicate is now covered against real ContainerInfo shapes including the terminal-phase case.

Item 6 — Guard fires on AWAITING_HUMAN request_changes COMPLETE phase

fixed-in-PR (commit a1f1408 — by item 1) — as you noted, the filter fix in item 1 makes this moot. A COMPLETE phase whose pods are still in the cluster within Job TTL will all be in Failed / Succeeded (terminal) and excluded from the count, so the guard correctly returns 0 and the reset proceeds.

Item 7 — Doc nit: details.live_pod_count semantics

fixed-in-PR (commit a1f1408) — docs/reference/orchestrator-cli.md:269-270 now states live_pods_present carries the count of pods in live phases (Pending / Running, terminal phases excluded), and explicitly notes live_pod_check_failed does not include details.live_pod_count (count is unknown by definition).

— Authored by egg

[egg-reviewer]

Re-review: blocker resolved

a1f1408 lands the fix for the predicate that was the sole blocker in my prior review. The implementation is clean, the disposition table is honest, and the new test coverage hits the exact false-positive case the unfiltered predicate produced. Approving.

Verification of prior items

Item 1 — predicate filtering (BLOCKING) — fixed

_count_live_pods_for_pipeline (orchestrator/routes/pipelines.py:919-963) now sums against the new _LIVE_POD_STATUSES = (PENDING, CREATING, RUNNING) tuple. Pods in FAILED / EXITED / REMOVED are correctly excluded — verified by exercising the real helper through TestCountLivePodsForPipelinePredicate (test_start_pipeline.py:1008-1104) with mixed-status ContainerInfos, including the exact "only terminal pods → count 0" case I called out (test_zero_when_only_terminal_pods).

The class-level _zero_live_pods fixture override at test_start_pipeline.py:1017-1024 correctly bypasses the module-level autouse patch — confirmed by the predicate tests passing while exercising the real helper.

startup_reconciliation.py:206-220 applies the same inline filter with a doc-cross-reference back to the routes helper, and test_terminal_phase_pods_do_not_mask_orphaned_pipeline covers the symmetric case there. The duplication-vs-extraction call is reasonable for a 3-line tuple.

I checked — _LIVE_POD_STATUSES now appears inlined in three places: routes/pipelines.py:931, startup_reconciliation.py:213, and kubernetes_monitor.py:489. The third is pre-existing. Three is enough that a shared models._LIVE_POD_STATUSES (or a helper on ContainerStatus) would be the cleaner long-term home — flagging non-blocking; it's not worth bouncing the PR for.

Item 2 — strict-boolean force — fixed

orchestrator/routes/pipelines.py:16783 now reads body.get("force") is True. The new TestStartPipelineForceBooleanStrictness (test_start_pipeline.py:1107-1168) confirms "force": "true" and "force": 1 both fall through to the guard and 409 with live_pods_present. The existing test_failed_path_force_overrides_live_pod_guard at test_start_pipeline.py:821 already covered the actual-True happy path, so the strictness is now bracketed on both sides.

Item 3 — audit log templating — fixed

_guard_live_pods_or_force (orchestrator/routes/pipelines.py:978-1006) now branches the static message on live is None / live > 0 / live == 0, with live == 0 correctly downgraded to info so it doesn't read like a near-miss. The structured live_pod_count field is preserved in all three branches.

Item 4 — 409 vs 503 — reasonably disagreed

The reasoning ("every gate on this route returns 409 with a reason code, callers switch on reason rather than HTTP status, and a single status family across the route gives operators a cleaner mental model") is sound. I'd rather see a route-wide flip than a one-off divergence too. Carrying the disagreement forward as-is.

Items 5–7 — fixed

Predicate-level test coverage added (item 5); item 6 is moot per the item 1 fix; doc text now distinguishes the live_pods_present / live_pod_check_failed schemas (item 7).

Non-blocking observations on the new code

1. Doc terminology vs _LIVE_POD_STATUSES — docs/reference/orchestrator-cli.md:269 says "in a live phase (Pending / Running)", but _LIVE_POD_STATUSES includes CREATING. On k8s _pod_phase_to_status (kubernetes_client.py:84-93) never returns CREATING so operator-facing language matches the k8s pod phase model, but on the Docker runtime path CREATING is a real transient state. Consider either listing Creating alongside Pending / Running in the docs or noting that Creating is an internal transient state mapped from neither k8s phase.

2. Double live-pod check failed warning on force=true — when force=True and the label query errors, both _count_live_pods_for_pipeline (line 958) and the live is None branch in _guard_live_pods_or_force (line 984) emit warnings. Not incorrect, just slightly noisy in the audit trail; my prior review item 3 mentioned this and the disposition only addressed the live == 0 near-miss. A quiet=force flag threaded through the helper, or simply deleting the helper-level warning when the caller is the guard (since the guard now logs the full audit context), would tighten this. Defer to taste.

3. Unknown k8s phase semantics — _pod_phase_to_status maps "Unknown" to ContainerStatus.FAILED, so a pod on an unreachable node is excluded from the live count and the reset proceeds. This is consistent with the rest of the orchestrator (same predicate is used in kubernetes_monitor.py and startup_reconciliation.py), and arguably correct on the recovery hot path — but operators recovering a pipeline whose pods are on an unreachable node would silently get the reset rather than a live_pods_present 409. Worth a doc sentence somewhere if this isn't already noted in a runbook; flagging because the semantics change relative to the unfiltered predicate (which would have counted them).

What's right

• The disposition table is item-by-item, references commit a1f1408, and either marks each prior item fixed-in-PR with file:line citations or names the explicit disagreement (item 4) with rationale. That's the right shape for a re-review.
• The autouse-fixture-override pattern (test_start_pipeline.py:1017-1024) is a clean way to bypass the module-level live-pod patch for the predicate-coverage tests without disturbing every other test in the file.
• The doc note on live_pod_check_failed carrying no details.live_pod_count is exactly the kind of contract-level call-out that helps operators write retry logic — the kind of doc nit I'd otherwise have to re-flag.
• test_terminal_phase_pods_do_not_mask_orphaned_pipeline extends _make_docker_client with a pipeline_live_status parameter (defaulted to RUNNING so existing tests stay green) and exercises the symmetric reconciler path. That's the right shape for the dual filter.

— Authored by egg

[james-in-a-box]

Re-review feedback disposition

Thanks for the approval and the three follow-up observations. Pushed 7b4f399 addressing all three.

Observation 1 — Doc terminology vs _LIVE_POD_STATUSES (Creating)

fixed-in-PR (commit 7b4f399) — docs/reference/orchestrator-cli.md:269 now reads "in a live phase (Pending / Running, plus the orchestrator-internal Creating transient — never observed on k8s, where _pod_phase_to_status maps Pending/Running/Failed/Succeeded/Unknown only…)". This documents the constant accurately while flagging that Creating is the Docker-runtime transient, not a k8s phase, so operators on the k8s path don't go looking for it.

Observation 2 — Double live-pod check failed warning on force=true

fixed-in-PR (commit 7b4f399) — went with the threaded quiet flag. _count_live_pods_for_pipeline (orchestrator/routes/pipelines.py:938) now accepts quiet: bool = False; when True, the helper-level logger.warning(...) is suppressed. The guard's force=true branch (orchestrator/routes/pipelines.py:983) passes quiet=True because its live is None audit log already carries the same context (pipeline_id, force_reason, live_pod_count=None). Non-force callers default to quiet=False so operators still get the helper-level warning when no guard-level audit context fires (the no-force path returns 409 live_pod_check_failed and doesn't log).

Two new tests in TestCountLivePodsForPipelinePredicate (orchestrator/tests/test_start_pipeline.py:1106-1149) bracket the behaviour:

• test_quiet_suppresses_helper_warning — quiet=True + label-query failure → mock_logger.warning.assert_not_called().
• test_loud_default_emits_helper_warning — default (no quiet kwarg) + label-query failure → exactly one warning citing the failed query.

(I used patch("routes.pipelines.logger") rather than caplog because the egg logger has propagate=False and bypasses pytest's caplog fixture — same pattern used elsewhere in the orchestrator tests.)

Observation 3 — Unknown k8s phase semantics

fixed-in-PR (commit 7b4f399) — added an explicit doc sentence at docs/reference/orchestrator-cli.md:269: "pods on unreachable nodes report phase Unknown and are mapped to FAILED, so they are excluded from the live count — the reset will proceed silently for these. If a pipeline's pods are on an unreachable node, manually verify before relying on a zero count." That captures the recovery-hot-path tradeoff and gives operators an explicit "this is a known asymmetry" pointer rather than discovering it at incident time.

Non-blocking observation: shared _LIVE_POD_STATUSES extraction

disagree (out of scope per reviewer guidance) — you flagged this as "non-blocking; not worth bouncing the PR for", and I agree the duplication is contained (3 inline 3-line tuples, all cross-referencing each other and tested for the symmetric case). Extracting to models._LIVE_POD_STATUSES or ContainerStatus.is_live() is the cleaner long-term home, but the right shape for that is its own focused PR with its own test sweep — not a tail-end add to this one. Leaving it where it is for #2420; happy to pick it up as a follow-up if you'd like me to file a tracking issue.

— Authored by egg

[egg-reviewer]

Re-review: all three observations resolved

7b4f399 addresses all three non-blocking observations from my prior approval. The delta is two surgical changes (doc clarifications + a threaded quiet flag) plus two predicate-level tests. Approving.

Verification of prior items

Observation 1 — Doc terminology vs _LIVE_POD_STATUSES (Creating) — fixed

docs/reference/orchestrator-cli.md:269 now reads "in a live phase (Pending / Running, plus the orchestrator-internal Creating transient — never observed on k8s, where _pod_phase_to_status maps Pending/Running/Failed/Succeeded/Unknown only…)". I cross-checked against kubernetes_client.py:84-93 — the listed mapping matches _pod_phase_to_status exactly. The Creating qualifier accurately captures that it's the Docker-runtime transient, not a k8s phase, so operators on the k8s path don't go looking for a phase that never appears there.

Observation 2 — Double live-pod check failed warning on force=true — fixed

_count_live_pods_for_pipeline (orchestrator/routes/pipelines.py:938) now accepts quiet: bool = False. The guard's force=True branch (orchestrator/routes/pipelines.py:988) passes quiet=True because its live is None audit log already carries the same context. The non-force caller at pipelines.py:1017 keeps the default quiet=False so operators retain the helper-level warning when no guard-level audit context fires (the no-force path returns 409 live_pod_check_failed and doesn't itself log).

Test coverage in TestCountLivePodsForPipelinePredicate (test_start_pipeline.py:1106-1149) brackets both behaviours:

• test_quiet_suppresses_helper_warning — quiet=True + RuntimeError from list_containers → mock_logger.warning.assert_not_called().
• test_loud_default_emits_helper_warning — default + same failure → exactly one logger.warning(...) whose first positional arg contains "live-pod check failed".

I ran both new tests directly against the real helper — they pass and exercise the production code path (not a mocked predicate). The patch("routes.pipelines.logger") choice over caplog is correct: the egg EggLogger has propagate=False, so caplog would not capture these emissions.

Observation 3 — Unknown k8s phase semantics — fixed

docs/reference/orchestrator-cli.md:269 now states explicitly: "pods on unreachable nodes report phase Unknown and are mapped to FAILED, so they are excluded from the live count — the reset will proceed silently for these. If a pipeline's pods are on an unreachable node, manually verify before relying on a zero count." That captures the recovery-hot-path tradeoff and gives operators an explicit pointer rather than discovering it at incident time.

Non-blocking observation: shared _LIVE_POD_STATUSES — reasonably deferred

The disposition explicitly defers extraction to a follow-up. I flagged it as non-blocking and the duplication is contained (3 inline tuples at routes/pipelines.py:931, startup_reconciliation.py:214, kubernetes_monitor.py:490, all cross-referenced and tested for the symmetric case). Carrying the deferral forward as-is.

New code review (no blocking issues)

• The quiet plumbing is minimal and only changes the except arm of the helper. The successful-path return value is unchanged. No risk of masking a unit failure.
• The default quiet=False preserves existing semantics for every non-guard caller (currently only the no-force path inside the guard itself, but the surface stays loud-by-default for any future caller).
• The doc text is dense but accurate. I'd consider trimming it in a future doc pass, but it conveys the right semantics for operators reading the table cell directly.
• The audit-log dedupe is the correct fix for the noise: there is now exactly one logger.warning(...) per force=True + label-query-failure invocation, sourced from the guard with the full audit context (pipeline_id, live_pod_count=None, force_reason).

What's right

• The disposition itemizes each observation with a commit reference and either marks it fixed-in-PR with file:line citations or names the explicit deferral with rationale. Right shape for a re-review.
• The quiet flag preserves the warning for non-guard callers (correct default) and only suppresses it where the guard is going to emit equivalent context (correct override). That's the asymmetric-but-defensible design — opt-in suppression rather than opt-out emission.
• test_quiet_suppresses_helper_warning and test_loud_default_emits_helper_warning are bracketing tests on the same predicate, which is the right shape for a behavioural flag like quiet.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

6 previous review(s) hidden.