FEAT-165: Fix job status detection across process lifecycle by shafty023 · Pull Request #43 · closedloop-ai/closedloop-electron

shafty023 · 2026-03-25T22:05:59Z

Kill process groups (-pid) instead of single PID in plan-loop cancel so pipeline children (grep, tee, claude) do not survive as orphans
Update JobStore in handleLoopKill with CANCEL_PENDING before deleting from runningLoops, so killed jobs show CANCELLED instead of FAILED
Add three cancellation gates in handleProcessCompletion success path to prevent git/PR work and completed events after cancel
Track attemptLlmCommit child PID in runningLoops, JobStore, and on-disk process.pid so cancel and status routes see the live process
Wrap post-processing in try/finally to prevent stale runningLoops entries that block future launches
Add restart-aware fallback in handleLoopKill: when runningLoops is empty (post-restart), fall back to JobStore PID for cancellation
Plumb JobStore into symphony-kill routes so legacy kill endpoint immediately upserts STOPPED status instead of waiting for enrichment
Suppress terminal status from state.json when process is still alive in both enrichJobSnapshot and resolveEffectiveState
Export getActiveLoopPid for plan-loop cancel PID fallback chain

Testing:

339 tests pass (19 new), typecheck and lint clean
New unit tests for shouldApplyStateStatus and enrichJobSnapshot integration with state.json status/phase suppression
New integration tests for kill JobStore update, restart-fallback cancel via loop/kill and plan-loop cancel, and status endpoint terminal status normalization
New cancellation gate tests for cancel before/during LLM commit and non-zero exit with CANCEL_PENDING

Risks:

Cancellation gates add early returns in handleProcessCompletion; artifacts may not be uploaded if cancelled mid-processing (intended)
Process group kill (-pid) requires detached spawn (already the case)

- Kill process groups (-pid) instead of single PID in plan-loop cancel so pipeline children (grep, tee, claude) do not survive as orphans - Update JobStore in handleLoopKill with CANCEL_PENDING before deleting from runningLoops, so killed jobs show CANCELLED instead of FAILED - Add three cancellation gates in handleProcessCompletion success path to prevent git/PR work and completed events after cancel - Track attemptLlmCommit child PID in runningLoops, JobStore, and on-disk process.pid so cancel and status routes see the live process - Wrap post-processing in try/finally to prevent stale runningLoops entries that block future launches - Add restart-aware fallback in handleLoopKill: when runningLoops is empty (post-restart), fall back to JobStore PID for cancellation - Plumb JobStore into symphony-kill routes so legacy kill endpoint immediately upserts STOPPED status instead of waiting for enrichment - Suppress terminal status from state.json when process is still alive in both enrichJobSnapshot and resolveEffectiveState - Export getActiveLoopPid for plan-loop cancel PID fallback chain Testing: - 339 tests pass (19 new), typecheck and lint clean - New unit tests for shouldApplyStateStatus and enrichJobSnapshot integration with state.json status/phase suppression - New integration tests for kill JobStore update, restart-fallback cancel via loop/kill and plan-loop cancel, and status endpoint terminal status normalization - New cancellation gate tests for cancel before/during LLM commit and non-zero exit with CANCEL_PENDING Risks: - Cancellation gates add early returns in handleProcessCompletion; artifacts may not be uploaded if cancelled mid-processing (intended) - Process group kill (-pid) requires detached spawn (already the case)

closedloop-ai-stage · 2026-03-25T22:18:58Z

Code Review Summary

Status: Needs Attention

Reviewers: Bug Hunter A, Bug Hunter B, Unified Auditor, Premise Reviewer, gateway-core-architect

Findings

Severity	Count
Blocking	0
High	2
Medium	3

HIGH Issues (should fix)

[P1] [symphony-loop.ts:2051] Restart-fallback kill sets CANCEL_PENDING when process already dead, leaving job permanently stuck
[P1] [symphony-loop.ts:2082] CANCEL_PENDING upsert set after SIGTERM 3s wait, racing with handleProcessCompletion cancellation gates

MEDIUM Issues (consider)

[P2] [symphony-kill.ts:51] Identical jobStore upsert-to-STOPPED block repeated 4x in registerSymphonyKillRoutes
[P2] [symphony-kill.ts:104] Four identical jobStore upsert blocks duplicated across all kill code paths
[P2] [symphony-loop-execute.test.ts:542] waitForJobTerminal duplicates identical function in symphony-loop-cloud-failures.test.ts

Validation Stats

Agent failures: 0 partitions skipped
Duplicates merged: 3

Recommendation: Address the two HIGH issues before merging. Both are race conditions in the cancel/kill lifecycle.

Replace waitForPidChange (polling JobStore for PID update) with a marker-file signal from the fake claude binary. The fake script touches a marker file on entry, and the test polls for that file to detect when attemptLlmCommit has been entered. This is deterministic regardless of PID update timing. Testing: - 339 tests pass locally - Removed unused waitForPidChange helper

- Move CANCEL_PENDING upsert before SIGTERM in handleLoopKill so handleProcessCompletion sees cancellation intent when exit fires - Restart-fallback kill: track process liveness and use CANCELLED (with completedAt) when process was already dead, CANCEL_PENDING when it was alive and signals were sent - Extract markJobStopped helper in symphony-kill.ts to eliminate 4 identical jobStore upsert blocks Testing: - 339 tests pass, typecheck and lint clean

shafty023 requested review from thadeusb and wongk March 25, 2026 22:06

mikeangstadt approved these changes Mar 25, 2026

View reviewed changes