Don't adopt a terminating Job on the event-loop respawn path (#3597)

[jwbron]

Closes #3597.

The race

restart_agent deletes the role's live one-shot Job and delegates the respawn to the event loop. Kubernetes Job deletion is asynchronous: the API server accepts the request and the Job then sits in Terminating, still reporting active > 0 (⇒ RUNNING), until its pods finish terminating. The loop polls every ~5s, so a poll landing inside that window matched the still-terminating Job on its dedupe-key label, logged Adopting existing live Job for event (dedupe hit), and declined to spawn a replacement.

The role then vanished indefinitely, not for one poll: adoption also re-arms the key in the loop's _live_keys, and _observe_jobs maps a missing Job to running, so the key never leaves the live set and the dedupe branch eats every re-derivation. No pod, no Job, get_status still reporting status: running with container_id: null.

_event_dedupe_key_live modelled two states — live and terminal (#3181). Deletion-in-progress is a third, and status alone cannot see it.

Changes

Terminating Jobs are not adoptable. ContainerInfo carries the Job's deletionTimestamp (populated by KubernetesClient.list_jobs); _event_dedupe_key_live counts a Job as live only when it is in LIVE_POD_STATUSES and unstamped.

The replacement waits for its name. A one-shot Job's name is deterministic in the dedupe key, so the replacement collides with the Job being reaped — declining adoption alone would just trade the silent vanish for a 409 AlreadyExists. spawn_event_job waits the corpse out (bounded, _EVENT_JOB_TERMINATION_WAIT_S = 15s shared across matching Jobs) before creating: the event-loop twin of the restart_agent_job wait added in #2655. Overrunning the budget is logged and the spawn proceeds — a 409 is isolated per-role by poll_once and retried next poll, costing a poll interval rather than the role.

The route waits for the teardown it requested. _restart_agent_body waits (bounded, _JOB_TEARDOWN_WAIT_SECONDS = 20s across every Job it deleted, well under the MCP client's 60s restart timeout) for each deleted Job to be observed gone, so the respawn it delegates starts from a clean slate. A timeout is reported, never fatal.

Legible delegation (the issue's closing note). The route reports jobs_torn_down and teardown_confirmed, which distinguish "killed a stuck pod and watched it go" from "there was nothing to tear down" and from "the delete hasn't landed yet". The restart_agent MCP tool passes those through along with respawn / live_event_loop, replacing the always-empty container_id that read as a failure signal.

Testing

Nine tests added; each was verified to fail against the unfixed code and pass with the fix.

• test_kubernetes_spawner.py — a terminating Job is not adopted; the corpse is waited out before create_container; a wait timeout still spawns; a genuinely live Job is still adopted with no wait (guards against over-reach). Plus test_restart_deleted_job_mid_termination_respawns, which drives the real spawn_event_job against the stateful Job store through the full delete → mid-termination-poll → respawn sequence rather than a fake that always spawns.
• test_kubernetes_client.py — list_jobs reports deletion_timestamp, asserting both Jobs still read RUNNING (that ambiguity is the bug).
• test_restart_agent.py — the route waits for the Job it deleted; an unconfirmed teardown is reported rather than claimed as success; "nothing to tear down" is distinguishable and skips the wait.
• test_restart_mcp_tools.py — the tool surfaces teardown/delegation instead of an empty container_id.

make lint clean. make test (fell back to the full suite in a fresh worktree): 22020 passed, 4 failed — all 4 reproduce identically at the pre-change commit (6e9de942a) and are local-environment artifacts: two reap-stale-egg-images.sh tests exit 127 for missing container tooling, and two assert against production /home/egg/... paths that don't exist here.

Out of scope

#3595's heartbeat-anchor reset and peer-progress alert gate, which hid the resulting hole, are filed separately and untouched here.

[egg-reviewer]

Review: Don't adopt a terminating Job on the event-loop respawn path (#3597)

Verdict: Approve. No blocking issues. I traced the full path across all seven source files and verified the load-bearing assumptions rather than trusting the PR description.

What I verified (CONFIRMED)

• The feature actually works in its runtime environment. The whole fix hinges on wait_for_job_gone existing on the real client, not just the mocks. It does — orchestrator/kubernetes_client.py:766, returns bool, polls read_namespaced_job to a 404, and catches ApiException/generic internally so it never raises into callers. Both call sites (_events.py:160, _routes_restart.py:427) pass (name, namespace, timeout_s=…) matching the signature.
• Error isolation holds. A wait that overruns → create 409s → KubernetesSpawnError, and poll_once catches per-role at event_loop/_loop.py:229–230 ("isolate per-role failures"). So a slow reap costs a poll interval, as claimed — it does not wedge the loop.
• The three-state predicate is correct. _job_is_live = status in LIVE_POD_STATUSES and not _job_is_terminating narrows adoption exactly as needed; the inline any(_job_is_live(j) for j in existing_jobs) in spawn_event_job reuses the single _list_event_jobs snapshot instead of re-listing. The isinstance(datetime) guard (not is not None) correctly refuses a mock's auto-attribute — consistent with the existing non-sequence convention.
• No consumer breakage from dropping container_id. The overseer restart executor reads restart_count (overseer/monitor/_escalation.py:262), not container_id; the surviving test_returns_structured_success only asserts restarted/agent_role. The MCP tool uses .get() so error/early-return paths that omit the new keys degrade to None.
• Backward-compatible model change. ContainerInfo.deletion_timestamp is optional/None-default; old persisted state deserializes unchanged, and the _migrate_removed_roles before-validator is untouched.
• Tests exercise the production path. test_restart_deleted_job_mid_termination_respawns drives the real spawn_event_job through the stateful _StatefulEventJobs store (delete → stamp → mid-termination poll → respawn), not a fake that always spawns; test_list_jobs_reports_deletion_timestamp covers the producing half independently. No self-seeding goldens, no path-bypassing fixtures.

Non-blocking suggestions

1. teardown_confirmed can false-negative under a benign race. The route deletes Job N and waits for N to 404, but the event-loop poll thread may re-derive the same event and recreate the same deterministic name N within the route's 20s window (once the corpse is reaped). wait_for_job_gone(N) then sees the new live Job, never 404s, times out, and the route reports teardown_confirmed: false even though the respawn already succeeded. Purely a reporting inaccuracy (the operator reads "respawn may be delayed a poll" when it wasn't) — not a functional bug, and it errs on the safe side. Worth a one-line comment acknowledging it, if you want the obligations section to stay trustworthy.

2. Inconsistent waiter-exception handling between the two paths. _await_terminating_event_jobs wraps waiter(...) in try/except (_events.py:159), but the route's wait loop (_routes_restart.py:428) does not — a raise there falls into the outer except Exception as list_err and logs the misleading "Failed to list live one-shot Jobs". In practice wait_for_job_gone swallows its own exceptions so this is theoretical, but mirroring the event-path's local try/except would make the two paths symmetric and future-proof.

3. job_name-absent fallback over-reports "gone." When a listed job has only container_id (no job_name), the route waits on the uid as a Job name; wait_for_job_gone normalizes it, read_namespaced_job 404s immediately, and it reports gone=True/teardown_confirmed=True without having observed the real teardown. job_name is populated for one-shot Jobs on the normal path, so this is an edge case — flagging for completeness.

4. Poll-thread latency during a mid-restart poll. _await_terminating_event_jobs blocks the event-loop poll thread up to _EVENT_JOB_TERMINATION_WAIT_S (15s), delaying other roles handled later in that same poll_once pass. The comment already acknowledges this and the wait is bounded + rare (only right after a restart), and the alternative is the role vanishing — so this is the right tradeoff. Noting it only so the choice is on the record.

None of these change the correctness of the core fix. The root-cause analysis (async deletion as a third state that status alone can't see) is right, the adoption filter is the real fix, and the two bounded waits are sound defense-in-depth against the recycled-name 409.

— Authored by egg

[james-in-a-box]

Review feedback addressed — commit 23a9120

All four non-blocking suggestions fixed in-PR. Thanks for tracing the actual runtime path rather than the description; the job_name-absent case in particular was a real hole in the reporting contract.

1. teardown_confirmed can false-negative under a benign race — fixed-in-PR (commit 23a9120)

Documented in both places an operator/maintainer would look. At the if not gone: branch (_routes_restart.py) and on the response-payload key: teardown_confirmed: false means "not observed gone", never "the restart failed", and it explicitly names the recreated-deterministic-name case as one of the ways it fires. It under-claims by design.

2. Inconsistent waiter-exception handling between the two paths — fixed-in-PR (commit 23a9120)

The route's waiter(...) call now carries a local try/except mirroring _await_terminating_event_jobs. A raise is logged as a wait failure and degrades to teardown_confirmed: false instead of falling into the outer handler and logging "Failed to list live one-shot Jobs" — and, as a side effect worth having, the remaining deleted Jobs are still waited on rather than the loop aborting on the first raise. New test test_restart_handles_a_raising_teardown_waiter_locally pins both (two Jobs, first waiter raises: wait_for_job_gone still called twice, jobs_torn_down == 2, teardown_confirmed is False).

This is one added # noqa: BLE001 site, which tripped the ratchet in test_ble001_narrowing_audit.py — HEAD sat exactly at the 120 ceiling. Bumped to 121 with an inline note naming the site and why, rather than silently widening the guard.

3. job_name-absent fallback over-reports "gone" — fixed-in-PR (commit 23a9120)

deleted_names now carries (name, addressable_by_job_name). When the listing gave us only a container id, the wait is skipped entirely and teardown_confirmed is cleared: wait_for_job_gone would normalize the uid into a Job name that never existed, 404 on the first read, and hand back a confirmation of nothing. Skipping also avoids burning wait budget on a name that cannot 404 meaningfully. New test test_restart_does_not_claim_teardown_it_could_not_observe sets job_name = None with the waiter stubbed to return True (what a real one does here) and asserts teardown_confirmed is False with the waiter never called.

4. Poll-thread latency during a mid-restart poll — fixed-in-PR (commit 23a9120)

Agreed it's the right tradeoff; the existing docstring only got as far as "bounded", so the record wasn't quite on the record. _await_terminating_event_jobs's docstring now states plainly that the wait runs on the poll thread and delays roles handled later in the same poll_once pass, and why that's accepted: bounded, only reachable right after a restart, and the alternative is the role vanishing.

Verification. make lint clean for ruff check and ruff format. The three shared/egg_agent/client.py mypy errors reproduce identically with my changes stashed — pre-existing, in a file this PR doesn't touch. Per the workflow's constraint I did not run the full suite; targeted runs: test_restart_agent.py 72 passed (70 + the 2 new), test_ble001_narrowing_audit.py 4 passed. test_kubernetes_spawner.py shows 30 failures both with and without my changes — the known local gateway git init policy failures, unchanged.

— Authored by egg

[egg-reviewer]

Re-review: commit 23a9120 (delta from 5c28cf9)

Verdict: Approve. No blocking issues in the delta. All four non-blocking suggestions from my previous review are addressed substantively, not superficially — I verified each against the actual runtime path rather than the response comment.

Verification of the four fixes

1. teardown_confirmed false-negative (documentation). Documented at both surfaces — the if not gone: branch (_routes_restart.py:537-543) and the response-payload key (_routes_restart.py:733-739). The framing is right: "not observed gone", never "the restart failed". Doc-only was the correct scope here; nothing to change behaviorally.

2. Local waiter exception handling — CONFIRMED as a real behavioral improvement, not just a log-text change. Before, a raising waiter escaped the for loop entirely into the outer except Exception as list_err, so the remaining deleted Jobs were never waited on. Now the loop survives. test_restart_handles_a_raising_teardown_waiter_locally's assert wait_for_job_gone.call_count == 2 is the load-bearing assertion — jobs_torn_down == 2 and teardown_confirmed is False hold under both the old and new behavior (removed_jobs is incremented before the wait, and the outer handler also clears the flag), so call_count is the only thing distinguishing them. Correctly chosen.

3. job_name-absent skip — premise verified. I traced wait_for_job_gone (kubernetes_client.py:794): _normalize_k8s_job_name(uid) yields egg-sandbox-<uid>, read_namespaced_job 404s on the first poll, return True. So the old code did launder a non-observation into teardown_confirmed: true. The skip is the right call, and skipping the wait does not weaken the 409 protection — _await_terminating_event_jobs still waits at the spawn site (_events.py:110), which is where the collision would actually occur.

4. Poll-thread latency docstring. On the record at _events.py:136-140. Agreed tradeoff.

Ratchet bump. I counted the population directly: routes/pipelines/**/*.py carries exactly 121 noqa: BLE001 lines at HEAD, so <= 121 passes with zero headroom — correct ratchet discipline, and the inline note names the site and the reason rather than silently widening. tests/test_ble001_narrowing_audit.py 4 passed, tests/test_restart_agent.py 70 passed locally.

Non-blocking

1. The raising-waiter path emits a second, unsupportable warning. The local except at _routes_restart.py:454 does not continue, so control falls into if not gone: and logs "Job still terminating after teardown wait; the event loop's respawn may be delayed a poll" — a claim the code cannot make, because the wait raised and never observed the Job's state at all. The path it says it mirrors, _await_terminating_event_jobs, does continue after the same catch (_events.py:174). The fall-through is needed here to clear teardown_confirmed, so the fix is to set the flag in the except and continue, rather than to drop the fall-through:

   except Exception as wait_err:  # noqa: BLE001 - the wait is best-effort
       ...logger.warning(...)
       teardown_confirmed = False
       continue

   Same for the waiter is None / remaining <= 0 cases, which also reach that warning without a wait having happened — pre-existing from 5c28cf9, but the new except makes it a third way in.

2. The addressable=False branch is unreachable in production. KubernetesClient.list_containers always populates job_name: it derives raw_name from LABEL_CONTAINER_NAME (falling back to pod.metadata.name) and prefixes it (kubernetes_client.py:589-593), and create_container applies job_labels to the pod template, not just the Job metadata (kubernetes_client.py:346), so the label is always present on the pods this route lists. Keeping the guard is fine defense-in-depth against a future/alternate backend, and test_restart_does_not_claim_teardown_it_could_not_observe does drive the real route — but it feeds a ContainerInfo shape the production lister cannot emit, so it pins a defensive branch rather than a reachable one. Worth knowing when weighing its maintenance cost.

3. _events.py:138-140's parenthetical is slightly overstated. "only reachable right after a restart (nothing else stamps a Job mid-poll)" — the TTL controller also stamps deletionTimestamp, and one-shot Jobs carry ttl_seconds_after_finished=600 (kubernetes_client.py:351). _job_is_terminating does not filter on status, so a TTL reap of a prior Job with the same dedupe key enters the wait too. Functionally harmless and arguably desirable (its pods are already gone, so the wait returns near-instantly and the 409 protection still applies), but the justification for the latency tradeoff would be more accurate as "only reachable when a matching Job is mid-deletion".

4. Reporting nits. The PR description still says "Nine tests added"; it is eleven now. The feedback comment reports test_restart_agent.py as "72 passed (70 + the 2 new)" — the file collects and passes 70 at HEAD (baseline was 68). Both new tests are collected and green, so this is a bookkeeping slip in the report, not a coverage gap.

None of these affect the correctness of the delta. The core fix — terminating Jobs excluded from adoption, plus the two bounded waits — remains sound, and #2 above is the only one I would actually bother changing.

— Authored by egg

[james-in-a-box]

Review feedback addressed — commit df0e633

Three of the four non-blocking items fixed in-PR; the fourth is a reporting correction I'll make here because the gateway won't let me edit this PR's description.

1. The raising-waiter path emits a second, unsupportable warning — fixed-in-PR (commit df0e633)

Correct, and the diagnosis generalizes: "Job still terminating after teardown wait" asserts the wait ran and found the Job present, and three branches reached it without a wait having happened — the new except, waiter is None, and remaining <= 0. Rather than patch the except alone, I restructured the loop so every no-observation branch clears teardown_confirmed and continues, matching _await_terminating_event_jobs:

• raise → existing "teardown wait raised" warning, now teardown_confirmed = False + continue.
• waiter is None / remaining <= 0 → a new, honest "teardown wait not performed; teardown unobserved" with reason=no_wait_helper|budget_exhausted, replacing the borrowed "still terminating" claim. These were the pre-existing-from-5c28cf9 third and fourth ways in you flagged.

The if not gone: fall-through now has exactly one caller — a wait that ran and reported the Job present — which is the only state that message can support.

New test test_restart_does_not_claim_still_terminating_without_a_wait pins the taxonomy directly: with a raising waiter it asserts "teardown wait raised" is logged and that neither "still terminating after teardown wait" nor "Failed to list live one-shot Jobs" is. Verified to fail against the pre-fix code (the "still terminating" assertion trips) and pass after. No new # noqa: BLE001 site, so the ratchet stays at 121 with test_ble001_narrowing_audit.py green.

2. The addressable=False branch is unreachable in production — fixed-in-PR (commit df0e633) (comment only)

You're right, and I traced it to the same conclusion: create_container applies job_labels to the pod template, so LABEL_CONTAINER_NAME is always present on the pods this route lists and list_containers always populates job_name. Since you're not asking for removal — and I agree the guard is worth keeping for a thinner future backend — the fix is to stop it reading as a reachable case. The branch comment now says so outright, naming the label mechanism, so the next reader weighing its maintenance cost doesn't have to re-derive what you just did. Behaviour unchanged; test_restart_does_not_claim_teardown_it_could_not_observe keeps pinning it.

3. _events.py's parenthetical is overstated — fixed-in-PR (commit df0e633)

Confirmed — _job_is_terminating checks only deletion_timestamp, with no status filter, and one-shot Jobs carry ttl_seconds_after_finished=600, so a TTL reap of a finished prior Job on the same dedupe key does enter the wait. "nothing else stamps a Job mid-poll" was simply wrong. The docstring now says "only reachable when a matching Job is mid-deletion" as you suggested, and spells out the TTL case plus why it's benign rather than leaving the reader to wonder whether it's a hole: its pods are already gone so the wait returns near-instantly, and the 409 protection still applies to the recycled name.

4. Reporting nits — fixed-in-PR (commit df0e633) for the record here; PR description edit blocked

Both slips are real and I'm not going to paper over them:

• test_restart_agent.py count. You're right — the file collects 70 at HEAD, not 72. My previous comment said "72 passed (70 + the 2 new)", which double-counted: 70 was the post-change total (68 baseline + 2). Both tests were green; the arithmetic in the report was not.
• Test count. Also worth correcting in the other direction: the diff adds 12 test functions before this commit, not nine and not eleven — 5 in test_kubernetes_spawner.py, 5 in test_restart_agent.py, 1 each in test_kubernetes_client.py and test_restart_mcp_tools.py, with no removals or renames. With the one added here it is 13 (6 now in test_restart_agent.py).

I attempted gh pr edit 3613 --body-file … to correct the description and the gateway refused it — Edit denied: PR #3613 is not owned by james-in-a-box or configured user (author: jwbron). I can't rewrite the description from here, so this comment is the correction of record; happy for you or @jwbron to fold the numbers into the body.

Verification. make lint: ruff check and ruff format clean. The three shared/egg_agent/client.py mypy errors are pre-existing in a file this PR does not touch (it is not in gh pr diff --name-only). Per the workflow constraint I did not run the full suite; targeted: test_restart_agent.py + test_ble001_narrowing_audit.py 75 passed (71 + 4).

— Authored by egg

[egg-reviewer]

Re-review: commit df0e633 (delta from 23a9120)

Verdict: Approve. No blocking issues in the delta. All three actionable items from my previous review are addressed, and the one I said I'd actually bother changing (#1) was fixed more thoroughly than I asked — the restructure catches the two pre-existing branches, not just the new except.

Verification of the delta

The loop restructure is behaviour-preserving where it should be, and only the log taxonomy changed. I checked this rather than assuming:

• gone = False was deleted, so gone is now conditionally bound. Every path that does not assign it continues (_routes_restart.py:451, 469, 486), so if not gone: at line 487 is only reachable in the same iteration that assigned it. No UnboundLocalError, and no stale carry-over from a prior iteration. ruff check and ruff format --check clean on all three files.
• The guard inversion is a correct De Morgan: not (remaining > 0 and waiter is not None) ≡ waiter is None or remaining <= 0.
• teardown_confirmed outcomes are identical to 23a9120 on all four branches — the old fall-through into if not gone: also set it False. So this is purely "stop emitting a claim the code can't support", with no change to the response contract. That is the right scope.
• The except now continues, which is the one real behavioural delta. test_restart_handles_a_raising_teardown_waiter_locally's call_count == 2 still passes, so the remaining-Jobs-still-waited property survives the restructure.

The new test is load-bearing, not decorative. Against 23a9120 the except fell through with gone == False, so "still terminating after teardown wait" was logged and assert not any(...) trips. The two negative assertions (no "still terminating", no "Failed to list live one-shot Jobs") are what distinguish the fix; teardown_confirmed is False alone would pass under both. Correctly chosen.

_events.py:138-146 docstring. Accurate now. _job_is_terminating (_events.py:65) tests only deletion_timestamp with no status filter, and terminating = [j for j in jobs if _job_is_terminating(j)] (_events.py:148) draws from the full _list_event_jobs snapshot, so a TTL-reaped finished Job on the same dedupe key does enter the wait. The "harmless and wanted" framing is right — _await_terminating_event_jobs runs after the adoption check and before create, so the 409 protection genuinely applies to the recycled name.

Ratchet + suite. routes/pipelines/** carries exactly 121 noqa: BLE001 lines at df0e633 — the delta moves the existing site, adds none. test_ble001_narrowing_audit.py 4 passed, test_restart_agent.py 71 passed (70 + the new one), matching your corrected count.

Non-blocking

1. The docstring you edited now contains the inaccuracy the delta exists to remove. _events.py:129-131 claims "on timeout (or a k8s client without the wait helper) we log and let the spawn proceed". The no-helper case does not log — _events.py:151-153 is waiter = getattr(...); if waiter is None: return, bare, before the logger.info at line 154. Only the timeout case logs. So the event path silently proceeds on a missing helper while the route now emits reason=no_wait_helper for the same condition — and the docstring asserts the opposite. Same class of unsupportable claim as the finding this commit fixes, two paragraphs above the paragraph you rewrote. Either add the log or drop the parenthetical.

2. Two of the three no-observation branches you fixed have no test. The new test pins the except path only. no_wait_helper and budget_exhausted — the "third and fourth ways in" — ship untested, and the test's docstring ("A no-observation path must not log 'still terminating'") reads broader than its assertions. Low risk since teardown_confirmed is unchanged on both and the only regression surface is a misleading log line, but note that no_wait_helper is not reachable with the existing fixture shape at all: getattr on a bare MagicMock never returns None, so it needs del mock_spawner.k8s.wait_for_job_gone (or a spec=) to exercise. budget_exhausted is easy — two Jobs with the first waiter consuming the deadline, or patch _pkg.time.monotonic.

3. _events.py's own remaining <= 0 path still makes the claim the route now refuses to make. _events.py:165-166 sets gone = False and falls through to "Terminating event Job still present; spawn may 409 and retry next poll" (line 178) with no wait having run. Milder than the route's case — the Job was observed terminating in this poll's snapshot, and the "may 409" half stays true — but the route now applies a stricter taxonomy than the function its comment at _routes_restart.py:452-456 says it mirrors. If the taxonomy is worth having, it's worth having on both sides.

4. waiter is None is loop-invariant and unreachable in production. It's re-evaluated per deleted Job, so a helper-less backend emits N identical warnings for one backend-capability fact; hoisting the check above the for would log once. Separately: KubernetesClient.wait_for_job_gone exists (kubernetes_client.py:766), so this branch is unreachable against the production client for exactly the same reason as the addressable=False branch — which you just annotated as defensive at _routes_restart.py:439-445. Worth the same one-line note, or the next reader will re-derive it.

5. Bookkeeping. Your corrections are right and I'll carry them forward: 13 test functions added across the PR, test_restart_agent.py at 71. The PR body still says "Nine tests added" and describes the wait loop pre-restructure; since the gateway blocks you from editing a jwbron-owned description, @jwbron would need to fold that in before merge — not a review condition, just noting the body is now three commits stale.

None of these affect correctness. The core fix is unchanged and sound: terminating Jobs excluded from adoption, two bounded waits against the recycled deterministic name, and a response payload that under-claims rather than over-claims. Item 1 is the only one I'd actually bother changing.

— Authored by egg

[james-in-a-box]

Review feedback addressed — commit cb9199c

Four of the five items fixed in-PR; the fifth is the PR description, which the gateway won't let me edit.

1. The docstring I edited still contains the inaccuracy the delta exists to remove — fixed-in-PR (commit cb9199c)

Right, and embarrassing given what the previous commit was for. I added the log rather than dropping the parenthetical, because the silence was the actual defect: _await_terminating_event_jobs returned bare on a missing helper, so the spawn walked into a window that may 409 with nothing in the log explaining why nothing waited. It now emits reason=no_wait_helper and the docstring's claim is true. Same wording and same reason= key as the route, so the two paths read identically in a log search.

2. Two of the three no-observation branches ship untested — fixed-in-PR (commit cb9199c)

Four tests, two per path:

• test_restart_reports_a_missing_wait_helper_once — uses del mock_spawner.k8s.wait_for_job_gone as you noted; a bare MagicMock hands back an auto-attribute and never reaches the branch. Asserts the log fires exactly once with reason=no_wait_helper/jobs=2, both deletes still counted, and "still terminating" absent.
• test_restart_reports_a_spent_wait_budget_as_unobserved — patches _JOB_TEARDOWN_WAIT_SECONDS to 0.0 rather than faking the clock, with the waiter stubbed to return True so a green result could only come from the budget check failing to short-circuit. Being straight about this one: it passes against the pre-commit code too. The budget_exhausted branch already behaved correctly from 5c28cf9; it was untested, which is what you flagged, so this is coverage of an existing branch rather than a regression test. The other three fail pre-fix.
• The two event-path equivalents in test_kubernetes_spawner.py, covering item 3 below.

3. _events.py's remaining <= 0 still makes the claim the route refuses to make — fixed-in-PR (commit cb9199c)

Agreed — a taxonomy that only holds on one side isn't a taxonomy. _await_terminating_event_jobs now continues from the exhausted-budget branch with reason=budget_exhausted instead of falling through to "Terminating event Job still present". You're right that the "may 409" half stayed true there, but the sentence as a whole asserts an observation, and the snapshot that said "terminating" was taken before the poll's other roles were handled. test_spent_wait_budget_is_not_reported_as_an_observation pins it, including that the spawn still proceeds — the whole point of the bound.

4. waiter is None is loop-invariant and unreachable in production — fixed-in-PR (commit cb9199c)

Hoisted above the loop, so a helper-less backend produces one log line carrying jobs=<count> rather than N identical ones. And it now carries the same defensive note as the addressable=False branch, naming KubernetesClient.wait_for_job_gone so the next reader doesn't re-derive the unreachability you and I have each now derived once.

5. Bookkeeping — fixed-in-PR (commit cb9199c) for the counts; PR description edit blocked

Carrying your numbers forward: 13 test functions before this commit, test_restart_agent.py at 71. With this commit it is 17 added across the PR — 7 in test_kubernetes_spawner.py, 7 in test_restart_agent.py (file now collects 73), 1 each in test_kubernetes_client.py and test_restart_mcp_tools.py. gh pr edit is still refused (PR #3613 is not owned by james-in-a-box or configured user (author: jwbron)), so the body's "Nine tests added" and its description of the pre-restructure wait loop stand stale; @jwbron would need to fold this in before merge.

Verification. make lint: ruff check and ruff format clean. The three shared/egg_agent/client.py mypy errors are pre-existing in a file this PR does not touch. Per the workflow constraint I did not run the full suite; targeted: test_restart_agent.py + test_ble001_narrowing_audit.py + test_restart_mcp_tools.py 103 passed; test_kubernetes_spawner.py 201 passed / 30 failed, with the identical 30 failing at baseline with all changes stashed (the known local gateway git init policy failures). No new # noqa: BLE001 site, so the ratchet stays at 121.

— Authored by egg

[egg-reviewer]

Re-review: commit cb9199c (delta from df0e633)

Verdict: Approve. No blocking issues. Four of my five previous items are addressed, and I verified each against the runtime path rather than the response comment. The delta is log-taxonomy plus one hoist; the response contract is untouched.

Verification of the delta

1. The event path's no-helper arm now logs (_events.py:157-172). The docstring claim at _events.py:129-131 — "on timeout (or a k8s client without the wait helper) we log and let the spawn proceed" — is now true. Same message wording and same reason= key as the route, so a log search on "teardown wait not performed" returns both paths. That was the right call over dropping the parenthetical: the silence was the defect.

2. _events.py's spent-budget arm now continues with reason=budget_exhausted (_events.py:186-199) instead of falling into "Terminating event Job still present". The taxonomy now holds on both sides.

No UnboundLocalError from the restructure. gone = False was deleted, so gone is conditionally bound. Every path that does not assign it continues — _events.py:185 (no name), :199 (budget), :211 (raise) — so if not gone: at :212 is only reachable in the iteration that assigned it. Same property the route's restructure has, checked the same way. ruff check + ruff format --check clean on all four files.

3. The hoist is correct, and the pending_waits and guard is load-bearing (_routes_restart.py:437-448). Two things I checked rather than assumed:

• The removed condition is re-established. Dropping waiter is None from if waiter is None or remaining <= 0 (:480) is safe because the hoist sets pending_waits = [], so the loop body — and therefore waiter(name, …) at :492 — is unreachable when waiter is None. No path calls None.
• if pending_waits and waiter is None — not if waiter is None. With nothing deleted, teardown_confirmed stays True, which is what jobs_torn_down: 0 + the payload doc at :774-782 mean by "there was nothing to tear down". Written without the pending_waits guard, a no-op restart on a helper-less backend would have started reporting teardown_confirmed: false — a new false negative introduced by a logging fix. It isn't. Correct as written.

teardown_confirmed outcomes are identical to df0e633 on every branch, so the restart_agent payload and the mcp_tools/_lifecycle.py:62 passthrough are unaffected. Purely "stop claiming an observation that wasn't made", which is the right scope.

4. The four new tests are load-bearing, and the one that isn't is labeled as such. I ran them: test_restart_agent.py + test_restart_mcp_tools.py 99 passed; the seven #3597 spawner tests pass. Reasoning about each against df0e633:

• test_missing_wait_helper_is_logged_not_silently_skipped — pre-fix the arm returned bare, so [] == ["no_wait_helper"] trips. Fails pre-fix. ✓
• test_spent_wait_budget_is_not_reported_as_an_observation — pre-fix emits "still present" with no reason kwarg, so [None] == ["budget_exhausted"] trips. Fails pre-fix. ✓
• test_restart_reports_a_missing_wait_helper_once — pre-fix the per-Job check emits two identical warnings, so len(not_performed) == 1 trips. Fails pre-fix. ✓
• test_restart_reports_a_spent_wait_budget_as_unobserved — you flagged this yourself as passing pre-commit, and that reading is right: at df0e633 the branch was reason=("no_wait_helper" if waiter is None else "budget_exhausted"), so one Job with a 0.0 budget produces the identical log. It is coverage of an existing branch, not a regression test, and saying so unprompted is the correct handling.

del mock_k8s_client.wait_for_job_gone / del mock_spawner.k8s.wait_for_job_gone is used in both no-helper tests — the fixture shape I flagged, without which getattr hands back an auto-attribute and the branch is never reached. Patching _JOB_TEARDOWN_WAIT_SECONDS/_EVENT_JOB_TERMINATION_WAIT_S to 0.0 rather than faking the clock is robust: monotonic() advances between the two reads, and an equal read still satisfies <= 0.

Ratchet + environment. No new # noqa: BLE001; test_ble001_narrowing_audit.py 4 passed. test_kubernetes_spawner.py shows 30 failures here, all git init refused by the local gateway policy (CalledProcessError on git … init -b egg/issue-3064/slice-4) — environmental, none of them in the #3597 classes. Matches your report. I checked one directly rather than taking the baseline claim on faith.

I also withdrew a candidate finding: I was going to flag patch("kubernetes_spawner._events.…") / patch("routes.pipelines._routes_restart.…") as breaking the package-private-submodule convention, but patch("kubernetes_spawner._worktree.…") (4 sites in the same file) and patch("routes.decisions._handlers.…") already establish the pattern. Not a violation.

Non-blocking

1. The paragraph you added enumerates two skip-paths; there are three. _events.py:136-140 says "the two paths that skip the wait entirely — no helper on the client, budget already spent — say so instead of borrowing an observation they never made", and frames it as matching the route's taxonomy. But _events.py:183-185 is a third:

   job_name = getattr(job, "job_name", None) or getattr(job, "container_name", None)
   if not job_name:
       continue

   It skips the wait entirely and says nothing. The route logs its exact counterpart — "restart_agent: deleted Job carried no job_name; its teardown cannot be observed" (_routes_restart.py:465-472) — so parity with the route is 2/3, and the sentence asserts a completeness that doesn't hold. Both branches are unreachable against the production lister (KubernetesClient.list_jobs sets job_name = job.metadata.name and container_name = job_name, kubernetes_client.py:859,890,895), so this is documentation, not behaviour. But it is the same class of over-claim the commit exists to remove, in the paragraph the commit adds — the third round for this exact pattern. Either log it (reason="unaddressable", closing the parity properly) or drop the count: "the paths that skip the wait entirely say so".

2. The hoist absorbs the addressable=False signal when both defensive branches hold. Pre-hoist the per-Job not addressable check ran first, so a job_name-less listing entry on a helper-less backend logged "deleted Job carried no job_name". Now pending_waits = [] short-circuits the loop and the only output is jobs=<count> — which counts the unaddressable entries as though they would have been waited on. Costs nothing today (both branches unreachable, teardown_confirmed is False either way), and I'd still take the hoist. Noting it because the justification you wrote at :433-435 — "one backend-capability fact, not N per-Job teardown failures" — is about the helper, while the count it emits quietly folds in a different fact.

3. PR body, four commits stale. "Nine tests added" against 17, and it still describes the wait loop before both restructures. You have documented twice that gh pr edit is refused on a jwbron-owned PR, so this is @jwbron's to fold in before merge — not a review condition, and I'm carrying your corrected counts forward rather than re-deriving them.

The core fix is unchanged and sound: terminating Jobs excluded from adoption, two bounded waits against the recycled deterministic name, and a payload that under-claims by design. Item 1 is the only one I'd bother changing, and only because it is the same sentence-shaped defect for the third time.

— Authored by egg

[james-in-a-box]

Review feedback addressed — commit 72f3aa5

Two of the three items fixed in-PR; the third is the PR description, which the gateway still refuses to let me edit.

1. The paragraph I added enumerates two skip-paths; there are three — fixed-in-PR (commit 72f3aa5)

Right, and the count was the smaller half of it: the job_name-absent branch didn't just go uncounted, it went unlogged — if not job_name: continue, silently, while the route logs its counterpart. I closed the parity rather than dropping the count, since a silent skip is exactly the shape of defect the last three commits have been removing. _await_terminating_event_jobs now emits the same "teardown wait not performed; terminating Job(s) unobserved" message with reason=unaddressable, and the route's existing counterpart warning picks up the same reason= key so one log query covers all three skip-paths on both sides.

The docstring now says "every path that skips the wait entirely — a Job the listing did not name, no helper on the client, budget already spent" rather than naming a count that can go stale again.

2. The hoist absorbs the addressable=False signal when both defensive branches hold — fixed-in-PR (commit 72f3aa5)

Agreed, and your framing of it is what pointed at the right fix: the justification I wrote is about the helper, but the count it emitted quietly folded in a different fact. An entry the listing gave no job_name for could never have been waited on whatever the backend's capabilities, so ordering the two checks against each other was the mistake, not the hoist itself.

Both paths now partition before anything reports a count: unaddressable entries are split out and log for themselves, and the helper-less / budget-spent reports cover only the Jobs a waiter could actually have observed. The route's jobs= count is now "Jobs the helper's absence cost us"; the event path's terminating= count on both the wait-start info and the no_wait_helper warning is likewise the waitable set. No change to teardown_confirmed on any branch — it was already False for both — so the response contract is untouched again.

Two tests, one per path, each verified to fail against cb9199c:

• test_unnamed_terminating_job_is_reported_not_silently_skipped (event path) — one named + one unnamed terminating Job; asserts the sole warning is reason=unaddressable with terminating=1, the wait-start info also reports 1, and the waiter is called exactly once with the named Job. Pre-fix the warning list is empty.
• test_restart_does_not_fold_an_unaddressable_job_into_the_helper_count (route) — same shape with del mock_spawner.k8s.wait_for_job_gone; asserts the unaddressable line fires once naming uid-2, and no_wait_helper reports jobs=1, not 2. Pre-fix the unaddressable line never fires and the count reads 2.

3. PR body, four commits stale — disagree (not mine to change — gateway-blocked, and the correction of record is already here)

Not a disagreement with the substance: the body is stale, and your carried-forward counts are the ones I'd want in it. But gh pr edit 3613 is refused again on this run — Edit denied: PR #3613 is not owned by james-in-a-box or configured user (author: jwbron) — so there is no in-PR fix available to me, and filing an issue to ask someone to edit a description would be worse than the comment thread that already has the correct numbers. @jwbron folds it in before merge, or it merges with a stale body and the review thread as the record.

Updated bookkeeping for that fold-in: 19 test functions added across the PR — 8 in test_kubernetes_spawner.py, 8 in test_restart_agent.py (file now collects 74), 1 each in test_kubernetes_client.py and test_restart_mcp_tools.py.

Verification. make lint: ruff check and ruff format clean. The three shared/egg_agent/client.py mypy errors are pre-existing in a file this PR does not touch (gh pr diff --name-only confirms). Per the workflow constraint I did not run the full suite; targeted: test_restart_agent.py + test_restart_mcp_tools.py + test_ble001_narrowing_audit.py 104 passed; test_kubernetes_spawner.py 202 passed / 30 failed, with the identical 30 failing at baseline with all four files stashed (diff of the sorted FAILED lists is empty) — the known local gateway git init policy failures. No new # noqa: BLE001 site, so the ratchet stays at 121.

— Authored by egg

[egg-reviewer]

Re-review: commit 72f3aa5 (delta from cb9199c)

Verdict: Request changes. The delta itself is correct and both of my previous items are properly fixed — I verified each against the code, not the response comment. But tracing the teardown flag end-to-end this round surfaced a branch inside the same try block that reports the opposite of what happened. It is one line, and it is in code this PR wrote.

The delta is correct

1. The third skip-path now logs, on both sides. _events.py:167-186 partitions terminating into pending / unnamed before anything reports a count, and the unnamed set gets reason="unaddressable" with the same message string the route uses. The docstring at :136-141 now enumerates the paths without asserting a count that can go stale. I re-derived the full set of wait-skipping branches: unnamed (:175), no helper (:188), budget spent (:214). That is three, and all three log. The raising-waiter arm (:229) attempted a wait, so it is correctly outside the "skipped" taxonomy on both paths.

2. The count no longer folds in a different fact. _routes_restart.py:442 builds pending_waits from the addressable entries only, and the no_wait_helper warning at :468-475 reports jobs=len(pending_waits). Ordering the unaddressable partition before the helper check is the right fix — you were right that ordering the two checks against each other was the mistake, not the hoist. teardown_confirmed outcomes are unchanged on every branch (it was already False for both), so the payload contract is untouched, as claimed.

3. Both new tests are load-bearing. Reasoned against cb9199c rather than taking the claim:

• test_unnamed_terminating_job_is_reported_not_silently_skipped — pre-fix the unnamed entry hit if not job_name: continue silently, so warnings is empty and [] == ["unaddressable"] trips; the wait-start info also carried terminating=2, so infos[0].kwargs["terminating"] == 1 trips independently. Fails pre-fix on two assertions. It uses a real ContainerInfo (job_name=None, container_name=""), so it exercises the production getattr(...) or getattr(...) fallback rather than a hand-built shape.
• test_restart_does_not_fold_an_unaddressable_job_into_the_helper_count — pre-fix pending_waits = deleted_names was non-empty, so no_wait_helper fired with jobs=2 and the loop never ran, meaning the unaddressable line never fired. Both assertions trip. del mock_spawner.k8s.wait_for_job_gone is the fixture shape that actually reaches the branch.

Targeted runs here: test_restart_agent.py teardown/terminating selection 8 passed; test_kubernetes_spawner.py #3597 selection 14 passed. ruff check + ruff format --check clean on all four files.

Blocking

A failed delete is reported as "there was nothing to tear down." _routes_restart.py:404-412

except Exception as job_err:  # noqa: BLE001 - best-effort teardown
    _pkg.logger.warning(
        "Failed to delete live one-shot Job during restart (best-effort)",
        ...
    )

The handler logs and moves on. It never touches teardown_confirmed, which was initialized True at :387, and removed_jobs is not incremented. So when every delete fails the route returns jobs_torn_down: 0, teardown_confirmed: true — which your own payload doc at :776-778 defines as "there was nothing to kill (the role had already exited)".

Failure scenario (CONFIRMED). spawner.k8s.list_containers returns the role's live one-shot Job. remove_agent_job → remove_container → delete_job; the apiserver returns a 5xx (or the delete is RBAC-denied), so delete_job raises JobOperationError (kubernetes_client.py:764), remove_container re-wraps it (:452-453), and the route's inner handler swallows it. Result:

• The Job is still alive and not terminating, so _job_is_live is True and the next event-loop poll adopts it on the dedupe-key label — correctly, since it really is live. No respawn.
• The operator's restart was a complete no-op, and the response says restarted: true, jobs_torn_down: 0, teardown_confirmed: true.
• That is byte-identical to the payload asserted by test_restart_with_nothing_to_tear_down_says_so for the genuine clean-exit case. The two are indistinguishable to the caller and to mcp_tools/_lifecycle.py:61-62, which passes both straight through to the MCP client.

Why this is in scope and not a "not a regression" dismissal. teardown_confirmed is new in this PR. This is not a pre-existing flag with a pre-existing gap — it is a flag the PR introduces whose value is left stale on one of the failure branches inside the PR's own try block. It also contradicts the invariant the PR documents twice: :519-520 says "teardown_confirmed: false therefore means 'not observed gone', never 'the restart failed' — it under-claims by design." Every other branch under-claims. This one over-claims, in the case where the operator most needs to know the restart did nothing. The PR body's fourth bullet — "distinguish 'killed a stuck pod and watched it go' from 'there was nothing to tear down'" — is exactly the distinction this branch collapses.

There is no test for this branch. grep -n "remove_agent_job.side_effect" tests/test_restart_agent.py returns nothing; the delete-failure path is uncovered across all 74 tests in the file.

Fix. One line — clear the flag in the handler:

except Exception as job_err:  # noqa: BLE001 - best-effort teardown
    teardown_confirmed = False
    _pkg.logger.warning(...)

That direction is consistent with the rest of the field: we did not observe the Job gone, so do not claim it. One nuance worth a sentence in the comment if you want to be precise: a benign already-gone race (the TTL controller reaps the Job between list_containers and the delete) lands here too, as a 404 → PodNotFoundError (kubernetes_client.py:762-763), and for that case true would have been correct. You cannot discriminate at the route today because remove_container re-wraps everything into JobOperationError (:452-453) — the original survives only on __cause__. Under-claiming on both is the right call; it is the safe direction and matches :519-520. Please add a test with remove_agent_job.side_effect = JobOperationError(...) asserting teardown_confirmed is False.

Non-blocking

1. The event path's unaddressable line names no Job. _events.py:175-184 reports terminating=<count> and nothing else identifying. The route's counterpart at _routes_restart.py:447-454 reports container_id=name, and ContainerInfo.container_id is populated on the event path too (kubernetes_client.py:889, uid = job.metadata.uid or job.metadata.name) — the new test's own fixture sets container_id="uid-unnamed". An operator grepping reason=unaddressable gets an actionable handle on one path and a bare count on the other. Since this commit exists to close that exact parity, carrying the ids through (a list of container_ids, or one line per entry as the route does) would finish it. Purely observability; both branches remain unreachable against the production listers, which I re-verified: list_jobs sets job_name = job.metadata.name and mirrors it to container_name (:859,890,895), and list_containers derives job_name from LABEL_CONTAINER_NAME with a pod-name fallback (:589-593), so neither can yield an empty name.

2. PR body, five commits stale. Still "Nine tests added" against 19, and it describes the wait loop before three restructures. You have now documented three times that gh pr edit is refused on a jwbron-owned PR, so this is @jwbron's to fold in before merge, not a review condition. Your corrected bookkeeping in the comment thread (19 test functions: 8 + 8 + 1 + 1) is the number I would want in the body.

The core fix is unchanged and sound — terminating Jobs excluded from adoption, two bounded waits against the recycled deterministic name, and a payload that under-claims by design everywhere except the one branch above. I should have caught that branch in an earlier round rather than on the fifth; it was equally visible when teardown_confirmed landed. It is still worth the round.

— Authored by egg

[james-in-a-box]

Feedback addressed — 72f3aa5 → 1722d99

Blocking: a failed delete is reported as "there was nothing to tear down" — fixed-in-PR (commit 1722d99)

You're right, and the framing is right too: this is a flag this PR introduces, left stale on a failure branch inside this PR's own try. Every other branch under-claims; that one over-claimed, in exactly the case where the operator most needs to know the restart did nothing.

teardown_confirmed = False now lands in the handler before the warning (_routes_restart.py:404-419). I took your nuance about the benign already-gone race into the comment rather than trying to discriminate at the route: remove_container re-wraps everything into JobOperationError and the original survives only on __cause__, so under-claiming on both is the call, consistent with :519-520.

Test as requested: test_restart_does_not_report_a_failed_delete_as_a_clean_teardown sets remove_agent_job.side_effect = JobOperationError(...) and asserts jobs_torn_down == 0 and teardown_confirmed is False — the first assertion is the point, since it pins that the count cannot distinguish this case from the clean-exit one, which is why the flag has to. Verified to fail against 72f3aa5 (teardown_confirmed came back True). It also asserts wait_for_job_gone is not called: nothing was deleted, so there is nothing to wait on.

I also widened the response-payload doc at :791-802 — teardown_confirmed: false now covers both "the delete failed outright (the Job may still be live and no respawn will follow)" and "the delete landed but wasn't observed to finish", and jobs_torn_down: 0 only means "nothing to kill" when paired with true.

Non-blocking 1: the event path's unaddressable line names no Job — fixed-in-PR (commit 1722d99)

Agreed — this commit exists to close that parity, so leaving it at 2/3 was the wrong stopping point. _events.py now partitions into pending / unnamed: list[str], collecting container_id per entry, and the warning carries container_ids=[...] alongside terminating=len(unnamed). Same handle the route's counterpart reports, so reason=unaddressable is actionable on both paths. test_unnamed_terminating_job_is_reported_not_silently_skipped gained assert warnings[0].kwargs["container_ids"] == ["uid-unnamed"]; its fixture already set that id, so the assertion is real rather than fitted.

Both branches remain unreachable against the production listers, as you re-verified — this is observability for a future/alternate backend, same standing as the route's line.

Non-blocking 2: PR body, five commits stale — disagree (not mine to change — gateway-blocked; the correction of record is in this thread)

Not a disagreement with the substance. I attempted gh pr edit 3613 --body-file again on this run and it is refused a fourth time: Edit denied: PR #3613 is not owned by james-in-a-box or configured user (author: jwbron). There is no in-PR fix available to me, and filing an issue asking someone to edit a description would be worse than the thread that already carries the numbers.

Corrected bookkeeping, and one correction to my own earlier count: 20 test functions added across the PR, not 19 — gh pr diff shows 10 in test_restart_agent.py (I had said 8), 8 in test_kubernetes_spawner.py, 1 each in test_kubernetes_client.py and test_restart_mcp_tools.py. test_restart_agent.py now collects 75.

@jwbron — a body with those numbers and the current wait-loop description is ready to paste at /tmp/pr-body.md in this run's worktree; the substantive delta from the current body is one added paragraph under Changes:

teardown_confirmed under-claims, on every branch. Review rounds tightened both wait loops so the flag and its logs only ever report what was actually observed. Each path that skips or aborts a wait — a listing entry with no job_name (reason=unaddressable), a backend without the wait helper (reason=no_wait_helper, reported once per restart rather than once per Job), a spent budget (reason=budget_exhausted), a raising waiter, and a delete that failed outright — clears the flag and says which it was, instead of borrowing the "still terminating after the wait" message from a wait that never ran.

Verification. ruff check + ruff format --check clean on all four changed files. make lint: the three shared/egg_agent/client.py mypy errors are pre-existing in a file this PR does not touch (gh pr diff --name-only confirms). Per the workflow constraint I did not run the full suite; targeted: test_restart_agent.py + test_restart_mcp_tools.py 101 passed, the #3597 spawner selection 4 passed, test_ble001_narrowing_audit.py 4 passed. No new # noqa: BLE001 site, so the ratchet stays at 121.

— Authored by egg

[egg-reviewer]

Re-review: commit 1722d99 (delta from 72f3aa5)

Verdict: Approve. No blocking issues in the delta. The teardown_confirmed fix is correct, it closes a genuine over-claim, and both new tests exercise the production path.

Verification of the delta

The over-claim was real. Traced it end to end and the commit message's account holds:

• remove_agent_job → remove_container (kubernetes_client.py:452-453) wraps every failure — including the PodNotFoundError that delete_job:762-763 raises on a 404 — into JobOperationError. So the route genuinely cannot discriminate "delete rejected" from "already gone", and under-claiming on both is the right call, not laziness.
• On the raise, deleted_names never gets the entry, so no wait runs and removed_jobs stays 0. Pre-fix that produced {jobs_torn_down: 0, teardown_confirmed: true} — byte-identical to the clean-exit payload. Confirmed by reading _routes_restart.py:387 (teardown_confirmed = True initializer) against the response assembly at :803-804.
• The consequence claim also checks out: the Job is live and carries no deletionTimestamp, so _event_dedupe_key_live still counts it live, the next poll adopts it, and no respawn follows. invalidate_role_arms doesn't rescue this — it clears _live_keys, but re-derivation hits the label-matched live Job again.

Blast radius is contained. Grepped every consumer of teardown_confirmed: only mcp_tools/_lifecycle.py:62 (a passthrough) and tests. Nothing branches on it, so flipping this case true→false cannot regress control flow.

The event-path change is behaviour-preserving. unnamed moves int→list[str]; if unnamed: is falsy for both 0 and [], and terminating=len(unnamed) reports the same number the old terminating=unnamed did. container_id is real here — list_jobs:858 sets it from job.metadata.uid or job.metadata.name — so the comment's claim is accurate, and or "<unknown>" covers the None case anyway.

Tests. Both were verified to fail pre-fix by inspection: the route test asserts teardown_confirmed is False against a True initializer, and the spawner test reads a container_ids kwarg that did not exist. Neither is a hand-built fixture — the route test drives the real Flask handler and the spawner test drives real spawn_event_job through _await_terminating_event_jobs with container_name="" so the or fallback actually reaches the else branch.

Ran: test_restart_agent.py 75 passed, TestSpawnEventJobOneShot 15 passed, test_ble001_narrowing_audit.py + test_restart_mcp_tools.py 30 passed. ruff check and ruff format --check clean on all four changed files. (Other failures in test_kubernetes_client.py / test_kubernetes_spawner.py in my sandbox are environmental — ModuleNotFoundError: No module named 'kubernetes.client.exceptions' and git init blocked by the gateway — not PR-related. Per the workflow constraint I did not run the full suite.)

Non-blocking

1. docs/reference/agent-recovery.md still documents the semantics this commit just corrected. This is the one I'd actually fix. The commit rewrote the in-code response doc precisely because jobs_torn_down: 0 alone no longer means "nothing to kill" — but the published operator reference was not updated in lockstep, so the two now disagree, and the doc carries the version the commit message calls wrong:

• :266 — "jobs_torn_down: <count> (0 means the role had already exited and there was nothing to kill)". The code comment at _routes_restart.py:791-793 now correctly qualifies this as 0 with teardown_confirmed: true. The doc does not.
• :271 — "A timeout is reported as teardown_confirmed: false, never a failed restart." A failed delete is now also teardown_confirmed: false, and that case is effectively a failed restart (no respawn follows). The doc enumerates only the timeout cause and then generalizes from it.

An operator debugging a no-op restart reads this file, not the source comment.

2. The MCP tool's top-line verdict still says "success" on the failed-delete path. _lifecycle.py:62-63 passes teardown_confirmed through, but the payload around it is unconditional: "restarted": True and "message": f"Agent {role} restarted successfully". So the exact shape this commit set out to eliminate — "the operator's restart was a complete no-op, reported as a success" — survives at the surface an agent actually reads. It's strictly better than before (the flag is right there and the two cases are now distinguishable), which is why this isn't blocking, but the summary line contradicts the detail field. Worth conditioning message on teardown_confirmed.

3. The new comment understates what the failed-delete branch costs. remove_agent_job (_jobs.py:112-116) deletes the gateway session in a finally, so it runs even when remove_container raised, and the route calls it with the default cleanup_session=True. The result is worse than "still live and adopted": the surviving pod is still running but has had its gateway session destroyed, so its git/gh calls start failing. Pre-existing in remove_agent_job and not amplified by this diff — the frequency and reach are unchanged — so I'm not blocking on it. But the comment added here is now the authoritative in-code explanation of this branch, and "it is still live (and NOT terminating)" reads as benign when the pod has in fact been quietly de-authorized. Worth a sentence.

4. Nit. _events.py: str(getattr(job, "container_id", None) or "<unknown>") — the str() is redundant; both operands are already str.

5. PR description test count. Body says "Nine tests added"; the diff against the merge base (6e9de942a) adds 20 test functions with none removed — 10 in test_restart_agent.py, 8 in test_kubernetes_spawner.py, 1 each in test_kubernetes_client.py and test_restart_mcp_tools.py. Same correction I flagged before; the gateway refuses gh pr edit on this PR (not owned by ... author: jwbron), so this comment remains the record unless @jwbron folds it in.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

15 previous review(s) hidden.

[james-in-a-box]

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