feat: recover subagents from the run coordinator - #5282
Conversation
github-actions
Bot
added
the
readiness: checking
GPT 5.6 Review — 🔴 changes requested (blocking)GPT 5.6 found at least one blocking issue that must be resolved before merging This comment is updated in place on each push. BLOCKING -- src/kiro_crew/run_coordinator/recovery.py:203 -- Restart cleanup uses an incompatible process-identity contract BLOCKING -- src/kiro_crew/subagent_manager/monitoring.py:67 -- Native rows enter the mutable legacy kill path BLOCKING -- src/kiro_crew/dashboard/handlers/messaging.py:783 -- Windows recovery discards retained transcripts (origin: validation) [GPT-REVIEWED] d885683 |
0f18778 to
065a15d
Compare
github-actions
Bot
added
readiness: action required
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All regions of the patch and the relevant repo mechanisms are now checked. Producing the review. First-Principles-Verdict: CONCERNS The recovery machinery is RFC-derived and earns its place, but three website fixes and an ACP-spawn test ride along undeclared in a 20-bullet description that never mentions them. What this change shipsIntent: after a gateway restart, recover subagent runs durably — import legacy folders, take over expired leases, kill only identity-verified orphans, deliver exactly one terminal outcome. ADDITION (implements the in-repo
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] d885683 |
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound fenced-recovery design with fail-safe biases throughout, but the shadow-phase dual-ownership machinery is intricate scaffolding that must not become permanent. Watch
Suggestions
(Note: the [DESIGN-REVIEWED] d885683 |
065a15d to
2ee02da
Compare
github-actions
Bot
added
readiness: checking
d02032a to
f82ba18
Compare
|
|
|
|
|
|
|
|
|
|
Evidence: regression coverage exercises successful cleanup and failed-cleanup delivery deferral; the shared coordinator contract passes for both memory and SQLite backends. The focused coordinator/authority suite passed 303 tests with one platform skip, and formatting, subprocess encoding, isort, flake8, Linux-parity mypy, and docs lint passed. |
|
/ai-review override gpt 5f224d6: Removing the POSIX deferral would weaken the documented fail-closed process-identity boundary because the mandatory durable audit creates a probe-to-signal window in which a recycled PID could be killed; default POSIX recovery intentionally retries until identity-safe termination is available. |
|
span=3ebfd78990ae — rebutted. The suggested early-return removal would turn a previously verified PID into an unpinned destructive signal after the mandatory off-loop durable audit, allowing PID reuse to retarget termination. The repository specification and regression |
Human judgment recorded@kyleseaman marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
|
|
|
|
Periodic recovery now retries the legacy importer after transient startup failure, with a deterministic regression passing on 58827ed. |
Legacy import now pins the trusted root, each run directory, and every optional file with descriptor-relative no-follow/nonblocking opens before metadata or reads; retained transcript reads use the same trusted-root discipline. Regression tests cover directory swaps, linked parents, hardlinks, off-loop descriptor cleanup, and linked display-root spellings on 003a681. |
Recovered interrupted members retain their exact neutral per-member outcome, but now count in the wave aggregate's non-success bucket so |
Evidence: recovery now opens the run directory once with no-follow semantics, unlinks and verifies through the same descriptor, and has a deterministic swapped-directory regression covering the race. |
bolichen97
left a comment
bolichen97
left a comment
There was a problem hiding this comment.
Review: recover subagents from the run coordinator
Reviewed origin/feat/run-coordinator-outbox...origin/feat/run-coordinator-recovery (41 files, +6763/−793) at 8289db460. Eleven angles; findings verified by executing the real RunRecovery, LegacyRunImporter, MemoryRunCoordinator/SQLiteRunCoordinator and the manager's reap paths, including real child processes and cross-tree differentials.
The headline, found independently by five of the eleven angles: this PR replaces the legacy orphan reconciler with a durable one, and the durable one cannot kill anything on macOS or Linux — while the legacy one it replaced now has zero production callers. Net effect on the two platforms most people run: a crashed gateway's surviving child runs forever, its run never converges, orphan folders accumulate without bound, and the parent is never told.
Blocking
1. run_coordinator/recovery.py:184 — if self._terminate_process is None and not platform_compat.IS_WINDOWS: continue makes POSIX skip the entire run: no kill, no complete(), no clear_recovered_process(). The only production construction is subagent.py:1704 RunRecovery(self._coordinator, self._outbox_delivery) — no keyword args — so _terminate_process is always None in production. Executed on this darwin host with a real sleep 600 child, matching process_start_id, process_owned=True and an expired lease: the run stayed observed_state=running across 3–5 sweeps with terminated=0 / interrupted=0 / delivered=0, lease_epoch climbing 2→3→4→5→6 and updated_at 111→231→351 while the child stayed alive. Each re-claim rewrites the whole runs table (see finding 10), every 60 s, forever. Worse: if the run had already committed its real completion but teardown failed so process_stopped was never set, claim_outbox returns [] and six reconcile passes yield delivered=0 — the finished subagent's actual result is permanently undeliverable. The log line claims the child "cannot be identity-pinned through termination", but platform_compat.kill_process_tree_pinned does support POSIX (:3036 delegates straight to kill_process_tree, i.e. exactly what the deleted _kill_orphan_pid did) — so the branch refuses on the one platform where the pinned kill is available and permits it only on Windows. The single test of the production default is pytest.skipped off Windows; every other termination test injects terminate_process=.
2. subagent_manager/monitoring.py:118 — redirecting the startup task from _reconcile_orphans() to _reconcile_startup() leaves _reconcile_orphans_impl with no production caller, silently deleting the legacy folder-based orphan kill, the gateway_restart tombstone write, and the owner Slack-DM digest. grep -rn '_reconcile_orphans' src/ finds only the impl and the facade delegate — no caller. That dead path owned _reap_orphan_process_impl, which SIGKILLed a surviving child using the folder's own pid+pid_recorded_at; the replacement cannot do it because the importer never imports those fields (docs/system-specs/modules/subagent.md:1035: "Legacy pid, pid_start_id, and process_owned fields are never imported and never authorize a signal") and recovery.py:184 declines to kill on POSIX anyway. _notify_orphan and _send_orphan_slack_dm are called only from inside the dead function, so the DM digest — the one fallback that does not depend on the agent-writable parent_session and could still have reached the operator — is dead code. And because no orphan folder ever receives a tombstone now, prune_stale_tombstones (still called at :579) never matches them: after N crashes, <data_home>/subagents/ accumulates N folders plus their session files forever, and list_orphans() re-reports them on every sweep. The tests still pass because they call manager._reconcile_orphans() directly, so the dead call site is invisible.
3. run_coordinator/legacy.py:99 — _open_base pins the RAW data_home() with a per-component O_NOFOLLOW walk and swallows the refusal with no log, so any symlinked ancestor silently disables 100% of legacy recovery, indistinguishable from "nothing to import". Executed with the same orphan folder reached two ways: real path → RecoveryReport(imported=1, interrupted=1, delivered=1); via a symlinked home → RecoveryReport(imported=0, existing=0, corrupt=0), get_run('crashed1') is None, and with logging.basicConfig(level=DEBUG) zero log records — corrupt is not even incremented, so the report looks like an empty tree. This is not exotic: pinned_fs.pin_parent's own docstring says refusing every symlinked ancestor "would also break paths under /tmp on macOS" (which is why the looser open_dir_pinned exists), macOS /tmp and /var are symlinks and /var/folders is where tempfile.gettempdir() lives, and the repo ships test/test_subagent_result_path_spelling.py whose entire premise is a symlinked data home. Executed at 150 folders: /tmp → 0 imported; the identical tree under /private/tmp → all 150. Note canonical_run_coordinator_dir() calls .resolve() while _base() uses the unresolved data_home(), so the ledger is at the canonical path and the legacy tree is looked for at the non-canonical one. On Windows it is worse: supports_pinned_tree_walk() is always False (no os.O_DIRECTORY), so legacy import is a permanent unlogged no-op there — and with _reconcile_orphans now dead, no pre-coordinator run is ever recovered on Windows and nothing says so. A child agent that renames ~/.kiro/crew/subagents aside and puts a symlink in its place therefore turns off recovery for every sibling run.
4. run_coordinator/legacy.py:221 — the importer discards state.json's parent_session, so a recovered run's completion event carries destination="" and is routed to nobody while still being acknowledged as delivered. Executed: a legacy folder with parent_session="dashboard:main", a result and no tombstone → imported=1, interrupted=1, delivered=1, but run.parent_session == '' and the outbox event's destination == '' / payload.parent_session_key == ''. Downstream dashboard_slot_key('') is falsy, so the gateway's on_done skips both the slot branch and the channel branch and returns having delivered nothing — then _deliver_outbox_event_impl sets _reported_to_parent = True and returns True, so drain_once acks and mark_delivered(fence) removes the durable event. The base _notify_orphan_impl read state.get("parent_session", "") and injected the interruption there. The parent LLM waits forever for a subagent whose interruption was discarded, and the only durable record of it is consumed. Dropping the value as agent-writable is defensible; manufacturing a delivery for it anyway is not — refuse or route, don't ack into the void.
5. subagent_manager/terminal.py:136 — _record_process_identity_impl passes pid_start_id or "" with a hardcoded process_owned=True, which record_process REJECTS, so an unreadable process identity aborts the whole subagent run. Executed: record_process(run, fence, version, 4242, "", True) → rejected/invalid_transition (memory.py:836 refuses process_owned and not process_start_id), and _coordinator_record_process_impl turns that into RuntimeError("coordinator process record refused: invalid_transition") at run.py:924, outside any try/except — so _run_inner fails and the subagent never receives its prompt. platform_compat.process_start_time returns None whenever the identity is unreadable: macOS/BSD when ps exceeds the 2 s _START_TIME_PS_TIMEOUT under load, when trusted_system_bin('ps') misses, or when output doesn't strict-decode; Linux under hidepid=2; Windows when OpenProcess is denied. Its own docstring states "an unreadable value must fail SAFE: None means 'identity unconfirmed', which every caller treats as 'do not kill'" — this new caller escalates that benign signal into total loss of dedicated subagent spawning on such a host. The base code logged at debug and continued. Degrading to process_owned=False (as the session-sharing path at run.py:800 already does) keeps the run alive without granting kill authority.
6. subagent_manager/terminal.py:996 — _force_reap_impl (user Stop and reaper force-kill) calls _run_terminal_report without process_stopped, so _clear_terminal_process_impl never runs and the TERMINAL row keeps its protected child identity — which makes claim_outbox refuse the event for delivery, batch drain AND acknowledgement. Executed: RUNNING → record_process(pid=4242, 'start-token', owned=True) → complete(STOPPED) → row is TERMINAL with process_owned still True; then drain_once(event_id=…) → [], drain_once(limit=16) → [], acknowledge(event_id) → None, 0 deliveries; after clear_recovered_process the same drain immediately returns DELIVERED. So _report_terminal_impl's loop spins on a 1 s sleep and the Stop notification only reaches the parent after the 90 s lease expiry plus a 60 s reaper pass lets recovery clear the identity — and never at all if the child survived teardown, per finding 1. Because the guard also blocks acknowledge(), a queued announce can be re-delivered later as a duplicate.
7. run_coordinator/memory.py:517 — a run admitted through the keyed command authority whose execution command is still PENDING when the gateway dies is permanently invisible to claim_recovery, because the 60 s grace applies only to source_version == "legacy-shadow-v1" and the authority submits with source_version="". Executed: two runs submitted, one keyed (what admit_reserved passes at subagent_command_authority.py:465 — it never sets source_version) and one shadow, gateway crashing between submit() and claim_command(). Over 20 consecutive reconcile() sweeps at the production 60 s cadence: the shadow run converges on sweep 1; the keyed run is still accepted / outcome=None after 20 minutes and was never delivered. Direct claim_recovery probes at +10 s, +130 s, +1 day and +30 days return ['shadow'] only. No production code claims stale PENDING execution commands (claim_commands has zero callers in src/), so the row leaks in coordinator.db forever and the parent never gets a terminal event. This falsifies the invariant added by this PR at docs/system-specs/modules/subagent.md:1052: "A durable keyed submission that has not yet been claimed remains eligible for normal execution after restart."
8. run_coordinator/recovery.py:240 — reconcile always calls completion_for with the default RunOutcome.INTERRUPTED, so a run the user explicitly Stopped is reported after a restart as "interrupted by gateway restart" with user_stopped: false and run.error discarded; the RunOutcome.STOPPED branch is unreachable from production. Executed: admit + start, user presses Stop (which durably records only a CommandOperation.CANCEL control command), gateway crashes before complete() writes STOPPED, lease expires, reconcile() runs → run.desired_state AFTER user Stop: DesiredState.RUN, delivered outcome: interrupted, delivered user_stopped: False, delivered error: interrupted by gateway restart, cancel control status still: PENDING. Nothing in src/ ever writes DesiredState.CANCEL (all three construction sites hardcode RUN and no path mutates it), and reconcile never inspects the PENDING CANCEL command, so the STOPPED branch could not be selected even if the check existed. Downstream, gateway.py:6555 tests info.outcome == OUTCOME_INTERRUPTED before info.user_stopped, so the user sees "⚠ interrupted by gateway restart" instead of "⏹ stopped by user", and _recovered_outcome makes :6632 skip failure accounting — silently violating the neutral-stop contract every other Stop path honours.
Should fix
9. run_coordinator/legacy.py:166 — _read_object double-closes the descriptor: fd = -1 sits AFTER the with os.fdopen(fd) block, so any exception from stream.read lets the with close it and then finally: os.close(fd) closes it again. Two executed repros. (a) With read() raising OSError(EIO) — a failing disk, ESTALE on a network home, MemoryError on the 1 MiB read, KeyboardInterrupt at shutdown — the original error is destroyed and replaced by OSError: [Errno 9] Bad file descriptor, and import_all just counts the folder "corrupt". (b) Modelling the real race, the proxy's __exit__ closed fd n and an unrelated open() was then handed the same number (fd 4, verified); the stray close succeeded silently and the unrelated open file was clobbered — writing to it raised [Errno 9]. _request runs via asyncio.to_thread in a real threadpool worker concurrently with the event loop opening sockets, pipes and SQLite connections, so the reused number can be any of those. _has_result uses the correct unconditional-close shape; only _read_object mixes fdopen ownership with a manual close. Fix: set fd = -1 before the with, or use contextlib.closing.
10. run_coordinator/legacy.py:283 + monitoring.py:479 — import_all awaits one import_legacy per folder, each a full-database DELETE+re-INSERT, so the import is O(N²) — and the reaper re-runs it at full cost every 60 s forever. Measured against the real SQLiteRunCoordinator: 200 folders → 0.81 s; 500 → 2.99 s; 1000 → 9.61 s with 500,500 run-row INSERTs; 2000 → 28.32 s with 2,001,000 INSERTs (exactly N(N+1)/2). Because _invoke persists unconditionally, a re-run that imports nothing costs the same: at 500 folders pass 2/3 = 4.92 s/4.84 s with existing=500, imported=0; at 1000 folders 19.25 s/16.43 s; and an independent measurement at 4000 folders (1.3 s of attacker work: mkdir + a 70-byte write) took 93.6 s, i.e. 23.4 ms/folder. import_all is wired into both startup (:118) and the 60 s reaper (:479), and _MIGRATIONS/_save_memory never delete a runs row, so a user with 1000 pre-coordinator folders burns 16–19 s of every 60 s window permanently. Amplification is also confirmed: every tombstone-less folder is imported as RUNNING, then turned into an INTERRUPTED completion with an outbox event, so N attacker directories become N ledger rows + N outbox events + N delivery attempts. Extrapolated, ~2.5 k folders puts one sweep over _REAPER_INTERVAL; since every call serialises in the bounded coordinator_executor(), live runs' 30 s renew() calls queue behind the sweep, their 90 s leases expire, and claim_recovery then interrupts still-running subagents.
11. run_coordinator/legacy.py:217 — conversation_key is the one imported field that is neither redacted nor length-capped, and agent is redacted but uncapped. Executed: "conversation_key": "subagent:AKIAIOSFODNN7EXAMPLE" is imported and stored verbatim into the conversation_key TEXT NOT NULL column, while every sibling field is redacted (task +1000 cap, agent redacted, error +2000 cap, run_id via a redaction-equality check). It is also a routing key elsewhere (run.py:320, continuation.py:63) — precisely the agent-writable class the comment at :262-268 says must not be trusted. Separately, "agent": "A"*900000 imported cleanly with len(agent) == 900000, and completion_for() produced a payload_json of 900,254 bytes, which complete() stores in outbox.payload_json and _deliver_outbox_event_impl puts through _redact(info.agent) into the dashboard frame. Because _save_memory rewrites all three tables on every call, that ~900 KB blob is re-written on every coordinator operation. Also unbounded: _folders applies no cap on folder count (finding 10), and _number only rejects non-finite values, so "started": -1e308 survives as created_at and completion_for emitted payload["elapsed"] == 1e+308 — which _info_from_outbox_impl parses into info.elapsed and publishes, while claim_recovery's sorted(key=(created_at, run_id)) pins that run to the front of every batch.
12. run_coordinator/recovery.py:142 — on macOS/BSD an identity mismatch returns (None, True) = retry forever with no attempt cap, deadline or escalation, so a genuinely reused PID (or a mere TZ/locale change) wedges the run immortally while Linux/Windows converge from the identical state. Executed: for one live pid, platform_compat.process_start_time returned 'Wed Sep 2 03:25:26 2026' under TZ=UTC and 'Wed Sep 2 12:25:26 2026' under TZ=Asia/Tokyo — so a gateway restarted under a different TZ/LC_TIME (launchd vs shell env) mismatches its own recorded identity for the same live child. Feeding that drifted value to production-default recovery left the run running across 5 sweeps (lease_epoch 2→6), never terminal, never delivered. The continue keeps the 90 s lease, so the row is simply re-claimed and re-warned every pass; nothing in the file can break the loop. Related: the default process_identity is process_start_time, which on Linux is /proc/<pid>/stat field 22 — ticks since boot — stored in a durable row with no boot-id fencing, and the repo's own platform_compat._own_identity_token refuses to hand out that bare value for exactly this reason ("a post-reboot process can repeat an earlier boot's (PID, ticks) pair") and adds _linux_boot_id(). Migration 5 adds no such column and nothing clears process_* at startup, so a post-reboot process can alias an old row and be classified as the recorded child; with IS_LINUX patched and a recorder installed, recovery reported terminated=1 against an unrelated live pid.
13. run_coordinator/memory.py:596 — _claim_commands' acquire_run_lease branch checks neither run.observed_state nor run.lease_expires_at, so it grants a fresh execution fence on an already-TERMINAL, already-delivered run and steals a live recovery owner's unexpired lease. Executed: a legacy-shadow run recovered past its grace; recovery commits INTERRUPTED, delivers the event, and holds the lease (owner=rec, epoch=1, lease_left=90 s), deliberately leaving the spawn command PENDING (recovery.py never touches it — docs :1205: "Recovery never claims or replays its execution command"). A subsequent claim_command("spawn:run-B", OwnerLease("gw2", now+90)) returns GRANTED with RunFence(lease_epoch=2) on a run the claimer itself sees as terminal / INTERRUPTED, and run.owner_id flips to gw2 while rec's lease still had 90 s. Reachable from _resolve_stopped_shadow_claim (subagent.py:2878), which retries exactly this claim_command in a loop: it sets info._coordinator_fence, returns True, and never reaches the _shadow_claim_taken_over check — so cancel_impl proceeds to _force_reap, complete(STOPPED) hits OUTCOME_CONFLICT, _report_terminal returns without delivering, and the user's Stop is silently swallowed. claim_recovery does guard on lease_expires_at > now; _claim_commands does not. Same family: import_legacy writes result_path onto an existing non-terminal run with no fence check and without incrementing version (executed: applied/transitioned, owner_id/lease_epoch unchanged, version still 4, so the fenced owner's complete(expected_version=4) still succeeds — no VERSION_CONFLICT), and since a live run's result_path is always '', the not existing.result_path guard is satisfied for every in-flight run on every 60 s sweep.
14. subagent_manager/terminal.py:210 — the lease heartbeat returns permanently on the first renew() that comes back False and never retries, so with a 90 s lease and a 30 s cadence just two consecutive failed renewals leave a genuinely live run holding no lease for the rest of its 30-minute deadline. Executed: renew at t+30 → True; after 95 s (two missed renewals) → False, and every subsequent renew → False forever, because memory.py:1018 refuses lease_expires_at <= now with no re-acquire path. terminal.py:210-212 logs and returns, popping the lease task. The run keeps executing with an expired lease, so claim_recovery takes it the moment it leaves _tasks (finding 13's shape), and nothing notices: complete(..., allow_expired=True) still returns applied/completed, so the lost lease is silent. One >90 s loop stall or a laptop suspend is enough, since time.time() advances across suspend while the reaper's asyncio.sleep deadline is monotonic. Related asymmetry: reconcile passes allow_expired=True to complete() but clear_recovered_process() validates without it (memory.py:875), so a sweep that outlives its own 90 s lease commits interruptions whose outbox events it can then no longer make claimable — executed with a 200 s probe batch: interrupted=1 delivered=0, run TERMINAL but still process_owned=True, claim_outbox → [].
15. test/conftest.py:487 — a new autouse fixture monkeypatches kiro_crew.subagent.SQLiteRunCoordinator → MemoryRunCoordinator for the entire suite, and the only test asserting the production default was deleted. _isolate_default_run_coordinator has no opt-out, so every SubagentManager(...) in the suite builds an in-memory coordinator: nothing verifies that a real manager gets a durable store, nothing exercises claim_recovery/import_legacy through _offload, and no restart test actually persists anything. The deleted base test was the two-line assert isinstance(manager._coordinator, SQLiteRunCoordinator); grep -rn SQLiteRunCoordinator test/ now shows only the conftest patch and the direct unit tests. Concretely: change the default to MemoryRunCoordinator() (or let the SQLite constructor raise and fall back) and the whole suite stays green while every run and every pending outbox event is lost on restart — the exact durability this PR exists to add. The fixture's own docstring names "coordinator submission timeouts" as the motivation, i.e. it papers over the next finding.
Cut by the cap (all verified)
subagent.py:2578 — _SHADOW_SUBMIT_TIMEOUT_SECS = 1.0 and its wait_for were deleted; _await_retained_shadow_submit now does an unbounded await asyncio.shield(submit_task), so a wedged coordinator blocks _run indefinitely while holding its slot. Executed: _run still blocked after 3 s with _run_inner.await_count == 0 (the base bounded it at 1.0 s and proceeded); the deleted base test test_stalled_shadow_submission_does_not_block_legacy_execution asserted exactly that, and grep -rn '_SHADOW_SUBMIT_TIMEOUT' src/ test/ now returns nothing · subagent.py:2857 — _drain_retained_shadow_submits busy-spins forever once a retained submit task ends cancelled, because _settled/_settlement_done only pop the task when not done.cancelled() and gather(return_exceptions=True) returns CancelledError on every later pass; executed, the function never returns and starves the loop, and it is reachable because it gathers the raw submit task unshielded · run.py:331 — a new RuntimeError("coordinator execution fence is missing") converts the still-documented best-effort shadow mirror ("any coordinator failure is diagnostic rather than an execution failure") into a hard execution precondition, so a transient database is locked now fails a legacy-path subagent that previously ran fine · messaging.py:780 — _read_spawn_result's pinned read hard-requires supports_pinned_walk(), so on Windows spawn_status silently returns no transcript at all (executed with os.O_DIRECTORY deleted: POSIX returns the text, Windows-simulated returns None → "_No result._"), and a result above MAX_FILE_BYTES is dropped rather than truncated — while the covering tests were marked @requires_pinned_walk (skipped) instead of covering the fallback · subagent_persistence.py:90 — agent_dir_for_display was changed from delegating to _agent_dir to calling only _validate_agent_id, dropping the resolved-path containment check, so it now returns a path that escapes the subagents root where it previously raised; executed with a symlinked evil1, _agent_dir raises "Path traversal blocked" while the display helper returns the path and its result.txt reads out-of-root content — and that string is handed to the parent agent to read at four sites. The base docstring said the validation "is delegated, so the two cannot drift apart" · messaging.py:996 — the new fourth outcome makes spawn_retry's old.outcome != "failed" gate reject exactly the runs a restart interrupted (409 "only failed agents can be retried"), i.e. the safest retry case, and the replacement info also loses cwd/_raw_task/max_turns/model · terminal.py:424 — the delivered tombstone is gated on outcome == "completed", so a legacy-imported run (which by construction has no tombstone) is never tombstoned after its interruption is delivered, leaving the folder unprunable and re-imported on every start · terminal.py:614 — if any(attempt.status is DELIVERED) was weakened to if attempts:, so the reporter now stops after a single PENDING attempt (destination raised, or acceptance refused) instead of retrying every _TERMINAL_RETRY_SECONDS; combined with the reaper's drain-to-empty loop being replaced by one non-looping drain_once(limit=256), a transient injection error now delays the parent by up to 60 s (and a >256 backlog by multiples of 60 s) where it previously retried in 1 s · memory.py:373 — import_legacy's existing-run branch looks the outbox up under (run_id, request.event_type) while the importer always sends event_type="", so existing_event is unconditionally None and every receipt falsely reports no pending delivery · _verified_live_process returns tuple[tuple[int,str]|None, bool] where ((pid, id), True) is representable nonsense, and the kill site reads it positionally as kill_process_tree_pinned(process[0], process[1], SIGKILL) — on the one call in the diff where confusing the two arguments kills the wrong process · a kill failing with PermissionError continues without clearing the identity, permanently excluding that run's completion from claim_outbox while re-emitting a critical=True "kill_authorized" SEL record every sweep for a kill that never happened · record_process never bounds process_id, so 2**31 is accepted and then makes pid_exists raise OverflowError (not an OSError, so uncaught) out of reconcile, aborting recovery for every other run and the final drain_once — executed, a healthy second run's committed completion was not delivered · reuse: _redact is duplicated in two new files where security.redact is already imported piecewise (now ~9 copies of a two-line security primitive, and legacy.py:140 uses _redact(run_id) != run_id as an authorization predicate); completion_for is a second subagent_completion payload builder that drops four keys _info_from_outbox_impl reads; _EVENT_TYPE is a fourth literal for one event name; the st_nlink > 1 or not S_ISREG or st_reparse_tag triple appears twice in one file while pinned_fs.refuse_hardlink_alias exists; and _DIR_OPEN_FLAGS captures dir_flags() at import time — the documented anti-pattern that module warns about, since the Windows-simulation tests delete the flag at runtime.
Conventions: terminal.py:643 edits a previously clean comment purely to append (Opus/Design/First-Principles review on #5326) — both a review-round marker and a PR number, forbidden verbatim by AGENTS.md and code-style.md. docs/system-specs/modules/subagent.md:214 says "schema v4 uses…" while _SCHEMA_VERSION = 5, and the test is named test_v1_database_migrates_to_v4_once while asserting == ("5",). recovery.py:97 re-inlines [:1000]/[:2000] and the "interrupted by gateway restart" string that legacy.py:36-37 already names as constants, and no run_coordinator/ row was added to code-style.md's owning-module index for any of the six new limits. Everything else is clean: black (pinned), flake8, isort, mypy --platform linux, docs-lint, brand, harness-parity, subprocess-encoding, check_agent_sdk_boundary, check_sync_io_in_async, check_loop_bound_locks, check_testpaths_coverage, check_lockdown_before_publish, check_changelog_history — and CHANGELOG is correctly untouched. Note check_black_formatting.py is red, on test/test_run_coordinator_sqlite.py from #5279, so this PR inherits a red gate it did not cause.
Execution-verified AI-assisted review (Claude Code). Eleven parallel angles ran against a local checkout at 8289db460 and each verified its own findings by execution — including real child processes under /tmp, cross-tree differentials, and measured latency curves; this summary is my consolidation of their reports, so treat the prioritisation as mine and the evidence as theirs. Nothing was modified in the tree and nothing was posted elsewhere. Verified clean, so please don't re-spend: the runs INSERT round-trips all 22 fields with no swapped pair; all 21 Protocol methods are forwarded by Shadow/SQLite/Memory with identical signatures; LegacyRunImport carries no process_id/process_start_id/process_owned, so no agent-writable field reaches a kill; the audit is correctly ordered before the kill on every path and an audit failure does gate it; and result_path is built from the _valid_id-validated run id.
|
|
|
Bolin review disposition for the current stack All fifteen numbered findings from the September 2 review are fixed in the submitted recovery layer.
The remaining cut-by-cap correctness observations were included in the same recovery audit and covered by the current process-identity, import, lease, and shutdown regressions. Current submitted head: d885683. |
Open PR relationship auditThis is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion. Relationship findings
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
2 participants
Problem / Motivation
Startup recovery understands legacy run files, but durable coordinator rows and leases need a fenced takeover path with safe orphan-process handling. Cancellation can also overlap coordinator admission, leaving terminal ownership ambiguous unless the submission and command claim settle definitively.
Why it matters
Restart recovery must retain terminal output and pending delivery without killing an unrelated process after PID reuse, allowing two owners to complete one run, losing accepted work, or reporting both a local result and a later recovered interruption.
What changed (motivation → approach → change)
Tests
test/test_run_coordinator_recovery.pycovers legacy import, linked and swapped directory rejection, nonblocking file opens, repeated importer recovery, fenced takeover, process identity, pending delivery recovery, the cleanup-lease sweep that must keep terminal delivery blocked, per-run drain isolation when cleanup fails, and swapped-directory tombstone clearance.Manual verification
N/A — restart, import, process identity, cancellation, shutdown, and recovery delivery are covered by deterministic automated tests.
Related Issues
No linked issue: this stack implements the locally reviewed durable run coordinator RFC.
Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)