Fix pause-abort worktree-slot leak + retry storm; add board auto-recovery - #1687
Merged
Conversation
A global pause/resume cycle parked tasks that had re-queued to todo as
status:"failed" ("operator action required") and leaked their in-memory
worktree slot. The scheduler kept re-dispatching the todo task, the
genuine-pause-abort branch re-fired on the still-set pausedAborted marker,
and it re-parked instantly with no backoff — a retry storm (75x/hr) that
pinned maxWorktrees=3/3 and concurrency-starved the whole queue.
- R1+R2 (executor.ts handleGraphFailure): treat a pause-abort that left a
task in `todo` as benign (FN-6782) — don't park failed, clear the
pausedAborted marker so the next dispatch is clean, and release the
leaked activeWorktrees slot. Operator-action failure preserved for
genuinely stranded non-todo columns (FN-6478).
- A1 (self-healing.ts recoverPausedAbortFailures): new maintenance sweep
that auto-recovers any pause-abort park still on the board and requeues
it (status:null = schedulable) so the board self-heals.
- run-audit.ts: new mutation types for the recovery telemetry.
Corrected the spec's null-vs-queued assumption: the scheduler dispatch set
is column==="todo" && !paused (scheduler.ts:1288); status:"queued" is the
*blocked* marker, status:null is runnable — so recovered tasks are left null.
Deferred (documented): A2 leaked-slot reaper needs a new executor
listWorktreeHolders introspection API to reap in-memory worktree slots
safely; R1 closes the observed leak at its source.
Tests: self-healing-paused-abort-recovery.test.ts (3),
executor-paused-abort-todo-benign.test.ts (2). Engine typecheck clean;
106 existing pause/graph-failure/limbo tests still pass.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughFixes a pause-abort failure mode in ChangesPause-abort benign handling and self-healing recovery
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Ready to review this PR? Stage has broken it down into 5 individual chapters for you: 
Chapters generated by Stage for commit 7f0ad62 on Jun 20, 2026 3:54am UTC. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/engine/src/__tests__/executor-paused-abort-todo-benign.test.ts (1)
72-85: ⚡ Quick winAdd an assertion for benign-path token-usage persistence.
The test covers status/log/worktree/marker behavior, but not the token-usage persistence contract described for this fix.
Suggested assertion
await invokeGraphFailure(executor, task); @@ expect((executor as any).activeWorktrees.has(task.id)).toBe(false); + expect(store.recordTokenUsage).toHaveBeenCalled();Based on PR objectives, the benign early-return path should also persist token usage; asserting it here hardens regression coverage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/engine/src/__tests__/executor-paused-abort-todo-benign.test.ts` around lines 72 - 85, Add an assertion to verify token usage persistence in the benign early-return path test. After the existing expect statements for parkedFailed, logText, pausedAborted, and activeWorktrees, add a new assertion that checks the store.updateTask mock calls to confirm token usage was persisted when invokeGraphFailure completes via the benign path. The assertion should follow the same pattern as parkedFailed and verify that the token usage data was properly passed to store.updateTask during this benign scenario.packages/engine/src/self-healing.ts (1)
7878-7892: ⚡ Quick winApply FNXC_LOG formatting to the new recovery docblock.
This new comment block should use the required
FNXC:Area-of-product yyyy-MM-dd-hh:mmformat so it remains grep-friendly and policy-compliant.As per coding guidelines, "Add FNXC_LOG comments describing the date (yyyy-MM-dd-hh:mm format)... Write FNXC:Area-of-product in front of all comments so they can be grepped."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/engine/src/self-healing.ts` around lines 7878 - 7892, The docblock comment describing the pause-abort auto-recovery logic in the failed-task recovery section is missing the required FNXC_LOG format. Add the FNXC:Area-of-product timestamp prefix in the format FNXC:Area-of-product yyyy-MM-dd-hh:mm at the beginning of the docblock comment (after the /** opening) to make it grep-friendly and policy-compliant with the coding guidelines.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/engine/src/__tests__/executor-paused-abort-todo-benign.test.ts`:
- Around line 74-84: The inline comments in the test section do not follow the
required repository policy for comment formatting. Update all four inline
comments in the test block (the ones preceding the parkedFailed variable
declaration, the logText expect call, the pausedAborted.has call, and the
activeWorktrees.has call) by prefixing each comment with the appropriate
FNXC:Area-of-product tag according to the coding guidelines specified for
packages/**/*.{ts,tsx,js} files.
In `@packages/engine/src/run-audit.ts`:
- Around line 462-465: The comments for the new audit mutation entries
"task:auto-recover-paused-abort-park" and "task:reap-leaked-concurrency-slot" do
not follow the required FNXC-formatted comment style for files in the packages
directory. Update both comments to include the FNXC:Area-of-product prefix
followed by the appropriate FNXC_LOG date and requirement context, following the
established coding guidelines for packages/**/*.ts files. Replace the current
informal comment format with comments that start with FNXC: and include proper
dated requirement tracking.
In `@packages/engine/src/self-healing.ts`:
- Around line 7944-7954: The recordRunAuditEvent call in the self-healing
recovery block can throw an error that gets caught by the outer per-task catch
block, incorrectly marking the recovery as failed and preventing the recovered
counter from being incremented. Wrap the recordRunAuditEvent call in its own
try/catch block to make audit emission best-effort and isolated from the core
recovery logic. Keep the recovered++ increment outside and after this isolated
audit try/catch so that successful state mutations are always counted even if
audit fails.
- Around line 7922-7934: The guard condition at line 7924 validates the recovery
predicate using a potentially stale executingIds snapshot, but the mutations
(updateTask and moveTask calls on lines 7930-7936) are applied without
re-validating after the await operations. If executingIds changes between the
guard check and the mutation calls, an ineligible task could still be moved.
Re-validate the full recovery predicate on the fresh task object immediately
before calling updateTask, ensuring the task is still not in executingIds and
meets all other eligibility criteria at the moment of mutation.
---
Nitpick comments:
In `@packages/engine/src/__tests__/executor-paused-abort-todo-benign.test.ts`:
- Around line 72-85: Add an assertion to verify token usage persistence in the
benign early-return path test. After the existing expect statements for
parkedFailed, logText, pausedAborted, and activeWorktrees, add a new assertion
that checks the store.updateTask mock calls to confirm token usage was persisted
when invokeGraphFailure completes via the benign path. The assertion should
follow the same pattern as parkedFailed and verify that the token usage data was
properly passed to store.updateTask during this benign scenario.
In `@packages/engine/src/self-healing.ts`:
- Around line 7878-7892: The docblock comment describing the pause-abort
auto-recovery logic in the failed-task recovery section is missing the required
FNXC_LOG format. Add the FNXC:Area-of-product timestamp prefix in the format
FNXC:Area-of-product yyyy-MM-dd-hh:mm at the beginning of the docblock comment
(after the /** opening) to make it grep-friendly and policy-compliant with the
coding guidelines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e15634d-6fb7-4bb7-a52a-60ed2a9c9fc0
📒 Files selected for processing (6)
.changeset/fix-pause-abort-leak-storm.mdpackages/engine/src/__tests__/executor-paused-abort-todo-benign.test.tspackages/engine/src/__tests__/self-healing-paused-abort-recovery.test.tspackages/engine/src/executor.tspackages/engine/src/run-audit.tspackages/engine/src/self-healing.ts
Contributor
Greptile SummaryThis PR fixes the global-pause → resume stall that starved the live board:
Confidence Score: 5/5Safe to merge — the benign-todo early-return in handleGraphFailure is tightly scoped, the double-check re-read guards in recoverPausedAbortFailures are thorough, and the leaked-slot reaper's conservative column/grace/refusal gating makes it safe to run continuously. The three-part fix correctly closes the leak at its source, the auto-recovery sweep has the globalPause guard and fresh-re-read race protection that were flagged in earlier review rounds, and both new methods are covered by focused tests including a mid-sweep TOCTOU case. Minor observability gaps in reapLeakedConcurrencySlots do not affect correctness. packages/engine/src/self-healing.ts — the reapLeakedConcurrencySlots method has two small observability gaps: the task:reap-leaked-concurrency-slot audit type is never emitted, and a refused clearPhantomExecutorBinding call goes unlogged. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Global Pause triggered] --> B[pausedAborted marker set on session]
B --> C[handleGraphFailure called]
C --> D{live.column?}
D -->|todo| E[FN-6782 benign path]
E --> F[clearPausedAborted]
F --> G[activeWorktrees.delete]
G --> H[Log benign + persistTokenUsage]
H --> I[Return — no failed park]
D -->|non-todo| J[Build park message with shared constants]
J --> K{done/archived?}
K -->|No| L[updateTask status:failed]
K -->|Yes| M[Log only — terminal column]
subgraph batch2[Batch-2 self-healing sweeps]
N[recoverPausedAbortFailures] --> O{globalPause?}
O -->|Yes| P[return 0]
O -->|No| Q[listTasks — find park markers]
Q --> R[re-read fresh + re-validate]
R --> S[updateTask status:null error:null]
S --> T{column todo?}
T -->|No| U[moveTask to todo]
T -->|Yes| V[clearPhantomExecutorBinding]
U --> V
V --> W[recordRunAuditEvent]
X[reapLeakedConcurrencySlots] --> Y{globalPause?}
Y -->|Yes| Z[return 0]
Y -->|No| AA[listWorktreeHolders]
AA --> AB[check column + grace + fresh executingIds]
AB --> AC[clearPhantomExecutorBinding]
AC --> AD{returned true?}
AD -->|No| AE[skip]
AD -->|Yes| AF[reaped++ logEntry]
end
L --> batch2
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Global Pause triggered] --> B[pausedAborted marker set on session]
B --> C[handleGraphFailure called]
C --> D{live.column?}
D -->|todo| E[FN-6782 benign path]
E --> F[clearPausedAborted]
F --> G[activeWorktrees.delete]
G --> H[Log benign + persistTokenUsage]
H --> I[Return — no failed park]
D -->|non-todo| J[Build park message with shared constants]
J --> K{done/archived?}
K -->|No| L[updateTask status:failed]
K -->|Yes| M[Log only — terminal column]
subgraph batch2[Batch-2 self-healing sweeps]
N[recoverPausedAbortFailures] --> O{globalPause?}
O -->|Yes| P[return 0]
O -->|No| Q[listTasks — find park markers]
Q --> R[re-read fresh + re-validate]
R --> S[updateTask status:null error:null]
S --> T{column todo?}
T -->|No| U[moveTask to todo]
T -->|Yes| V[clearPhantomExecutorBinding]
U --> V
V --> W[recordRunAuditEvent]
X[reapLeakedConcurrencySlots] --> Y{globalPause?}
Y -->|Yes| Z[return 0]
Y -->|No| AA[listWorktreeHolders]
AA --> AB[check column + grace + fresh executingIds]
AB --> AC[clearPhantomExecutorBinding]
AC --> AD{returned true?}
AD -->|No| AE[skip]
AD -->|Yes| AF[reaped++ logEntry]
end
L --> batch2
Reviews (4): Last reviewed commit: "Update executor-recovery test for benign..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
reapLeakedConcurrencySlots() reclaims in-memory worktree slots whose holder is no longer in-progress (the FN-6756 "in todo yet still a maxWorktrees holder" leak) without an engine restart — defense-in-depth behind the source fix. - executor: new listWorktreeHolders() read-only introspection over activeWorktrees; wired through in-process-runtime to SelfHealingManager. - reaper releases ONLY when every guard agrees: not executing, task missing or in todo/triage, past a 60s grace, and clearPhantomExecutor Binding itself refuses (returns false) if a live session surface is registered — so it can never pull a worktree from a running agent. - registered in maintenance batch 2 (respects globalPause/enginePaused skip + FN-4962 ordering). - widened the clearPhantomExecutorBinding option type to surface its boolean refusal signal. Engine typecheck clean; 19 tests pass (new reaper 7 cases + regression). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Substantive (A1 recoverPausedAbortFailures): - Self-guard on globalPause/enginePaused at method entry (greptile P1) — the public method must not requeue tasks an operator intentionally froze. - Re-validate the FULL predicate with a FRESH executing set on the re-read before the backward move (coderabbit Major + greptile): add fresh.userPaused and column re-check so a task that became ineligible across awaits is skipped. - Isolate audit emission in its own try/catch (coderabbit) so an audit throw after a successful mutation can't log a false "recovery failed". - Decouple the recovery predicate from the literal error text via shared PAUSE_ABORT_PARK_ERROR_MARKER/OPERATOR_MARKER constants (greptile) — the executor builds the parked message from the same constants. - Use the wired clearPhantomExecutorBinding (live-session-guarded) instead of the declared-but-never-wired releaseExecutorWorktreeOwnership, which no-op'd. Nits: - FNXC-prefix new comments in executor.ts, run-audit.ts, and the benign test per repo comment policy. - Fix a test-only type error on the clearPhantomExecutorBinding mock. Added a test asserting the globalPause self-guard. Engine typecheck clean; pause-abort/reaper/benign + regression suites pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
Review feedback addressed (commit 7eaf513)All 8 review threads resolved. Summary of what changed in Substantive (correctness):
Nits: FNXC-prefixed the new comments in Engine typecheck clean; pause-abort / reaper / benign + regression suites pass (re-run locally). |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/engine/src/self-healing.ts (1)
7980-7993:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor
clearPhantomExecutorBindingrefusal before making the task runnable.
clearPhantomExecutorBindingcan returnfalsewhen a live session surface still exists, but the task is already cleared/requeued before that signal is ignored. That can make atodotask schedulable while the old binding/session is still live. Also, ifmoveTaskfails after Line 7980, the failed marker is gone and the next sweep cannot retry this recovery. Gate on explicit refusal first, then move, then clear the failure state.Proposed fix
- await this.store.updateTask(task.id, { status: null, error: null }); + const clearResult = this.options.clearPhantomExecutorBinding?.(fresh.id); + if (clearResult === false) { + log.log(`Skipped pause-abort park recovery for ${fresh.id}: live executor binding refused phantom clear`); + continue; + } + if (fresh.column !== "todo") { - await this.store.moveTask(task.id, "todo", { + await this.store.moveTask(fresh.id, "todo", { preserveProgress: true, moveSource: "engine", recoveryRehome: true, }); } + await this.store.updateTask(fresh.id, { status: null, error: null }); // Release any in-memory worktree ownership the leaked park may still // pin, so the requeued task does not re-block the concurrency gate. // FNXC:WorkflowLifecycle 2026-06-20-00:00: use clearPhantomExecutorBinding // (wired + live-session-refusal guarded), NOT releaseExecutorWorktreeOwnership // which is a declared-but-never-wired option — it would silently no-op. - this.options.clearPhantomExecutorBinding?.(task.id);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/engine/src/self-healing.ts` around lines 7980 - 7993, In the self-healing.ts file, the order of operations in the task recovery block needs to be restructured to honor clearPhantomExecutorBinding refusals. Currently, the code updates the task status and moves it to todo before checking if clearPhantomExecutorBinding succeeds, which can leave the task in a schedulable state even when the old binding/session is still live. Refactor the code to call clearPhantomExecutorBinding first and check if it returns false indicating refusal, then only proceed with updateTask and moveTask if the binding was successfully cleared (or the operation is not applicable). This ensures the task state remains unchanged if a live session still holds the binding, and protects against lost error markers if moveTask fails during recovery.
🧹 Nitpick comments (2)
packages/engine/src/self-healing.ts (2)
245-257: ⚡ Quick winUse the required
FNXC_LOGcomment marker on the new requirement comments.Several new FNXC requirement comments include area/date context but omit the grepable
FNXC_LOGtoken required by the repo convention.As per coding guidelines, “Add FNXC_LOG comments describing the date (yyyy-MM-dd-hh:mm format) and requirements/changes that made you implement functionality. Write FNXC:Area-of-product in front of all comments so they can be grepped.”
Also applies to: 371-376, 415-424, 2166-2169, 7926-7930, 7961-7966, 7990-8002, 8071-8090
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/engine/src/self-healing.ts` around lines 245 - 257, Add the FNXC_LOG comment marker to the requirement comments for the new functionality. Update the comments describing listWorktreeHolders and clearPhantomExecutorBinding to include the FNXC:Area-of-product prefix along with the date in yyyy-MM-dd-hh:mm format and the requirement/change details, making them grepable according to repo conventions. Apply the same fix to all other locations mentioned (371-376, 415-424, 2166-2169, 7926-7930, 7961-7966, 7990-8002, 8071-8090) where new requirement comments lack the FNXC_LOG token.Source: Coding guidelines
8121-8126: ⚡ Quick winEmit the leaked-slot audit event when a slot is reaped.
task:reap-leaked-concurrency-slotwas added to the audit mutation contract, but this sweep only writes a task log. Add a best-effort run-audit event here so reclaimed concurrency slots are queryable after the fact; avoid relying onlogEntryfor missing-task holders.Proposed fix
- for (const { taskId } of holders) { + for (const { taskId, worktreePath } of holders) { @@ reaped++; - await this.store.logEntry( - taskId, - "Auto-recovered: released leaked worktree/concurrency slot (holder no longer in-progress)", - ); + if (task) { + await this.store.logEntry( + taskId, + "Auto-recovered: released leaked worktree/concurrency slot (holder no longer in-progress)", + ); + } + try { + await createRunAuditor(this.store, { + runId: generateSyntheticRunId("self-healing-leaked-slot-reaper", taskId), + agentId: "self-healing", + taskId, + taskLineageId: task?.lineageId, + phase: "reap-leaked-concurrency-slots", + }).database({ + type: "task:reap-leaked-concurrency-slot", + target: taskId, + metadata: { + taskId, + column: task?.column ?? "missing", + worktreePath, + }, + }); + } catch (auditErr: unknown) { + log.warn(`Leaked-slot reaper audit emission failed for ${taskId}: ${auditErr instanceof Error ? auditErr.message : String(auditErr)}`); + } log.warn(`Reaped leaked worktree slot held by ${taskId} (column=${task?.column ?? "missing"})`);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/engine/src/self-healing.ts` around lines 8121 - 8126, In the leaked worktree slot reaping block where reaped is incremented and logEntry is called, add a best-effort audit event emission for the task:reap-leaked-concurrency-slot event alongside the existing logEntry call. The audit event should include the taskId and relevant context (such as the column information) to make reclaimed concurrency slots queryable for audit purposes. Since some holders may not have corresponding tasks, use error handling (try-catch or optional chaining) to ensure the audit event emission does not block the reaping operation if the task or audit system is unavailable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/engine/src/self-healing.ts`:
- Around line 8100-8117: The executingIds set is captured once at the beginning
of the loop before iterating through holders, but a task can start executing
while the loop is processing and awaiting getTask. Before calling
clearPhantomExecutorBinding with the taskId, re-check the current executing set
by calling getExecutingTaskIds again and verify that the taskId is not in the
updated set. This ensures the reaper does not clear a phantom executor binding
for a holder that became active during the sweep, preventing race conditions
where a task starts executing between the initial check and the final release
operation.
---
Outside diff comments:
In `@packages/engine/src/self-healing.ts`:
- Around line 7980-7993: In the self-healing.ts file, the order of operations in
the task recovery block needs to be restructured to honor
clearPhantomExecutorBinding refusals. Currently, the code updates the task
status and moves it to todo before checking if clearPhantomExecutorBinding
succeeds, which can leave the task in a schedulable state even when the old
binding/session is still live. Refactor the code to call
clearPhantomExecutorBinding first and check if it returns false indicating
refusal, then only proceed with updateTask and moveTask if the binding was
successfully cleared (or the operation is not applicable). This ensures the task
state remains unchanged if a live session still holds the binding, and protects
against lost error markers if moveTask fails during recovery.
---
Nitpick comments:
In `@packages/engine/src/self-healing.ts`:
- Around line 245-257: Add the FNXC_LOG comment marker to the requirement
comments for the new functionality. Update the comments describing
listWorktreeHolders and clearPhantomExecutorBinding to include the
FNXC:Area-of-product prefix along with the date in yyyy-MM-dd-hh:mm format and
the requirement/change details, making them grepable according to repo
conventions. Apply the same fix to all other locations mentioned (371-376,
415-424, 2166-2169, 7926-7930, 7961-7966, 7990-8002, 8071-8090) where new
requirement comments lack the FNXC_LOG token.
- Around line 8121-8126: In the leaked worktree slot reaping block where reaped
is incremented and logEntry is called, add a best-effort audit event emission
for the task:reap-leaked-concurrency-slot event alongside the existing logEntry
call. The audit event should include the taskId and relevant context (such as
the column information) to make reclaimed concurrency slots queryable for audit
purposes. Since some holders may not have corresponding tasks, use error
handling (try-catch or optional chaining) to ensure the audit event emission
does not block the reaping operation if the task or audit system is unavailable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ca1a394a-ab2e-43e5-9a24-c5127d5ac677
📒 Files selected for processing (8)
.changeset/fix-pause-abort-leak-storm.mdpackages/engine/src/__tests__/executor-paused-abort-todo-benign.test.tspackages/engine/src/__tests__/self-healing-leaked-slot-reaper.test.tspackages/engine/src/__tests__/self-healing-paused-abort-recovery.test.tspackages/engine/src/executor.tspackages/engine/src/run-audit.tspackages/engine/src/runtimes/in-process-runtime.tspackages/engine/src/self-healing.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/fix-pause-abort-leak-storm.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/engine/src/run-audit.ts
- packages/engine/src/tests/executor-paused-abort-todo-benign.test.ts
coderabbit Major: reapLeakedConcurrencySlots captured executingIds once before the loop, but each holder awaits getTask — a task could start executing mid-sweep and have its worktree slot pulled. Refresh the executing set immediately before clearPhantomExecutorBinding and skip if the holder is now executing (same race the A1 recovery fix closed). clearPhantomExecutorBinding's live-session refusal remains the last line of defense; this avoids racing it. Added a mid-sweep race test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The gate test asserted the OLD behavior — a paused graph exit in the `todo` column parked `status:"failed"` with "operator action required". FN-6782 made the todo case benign (no failed park; benign log + cleared marker), so split the parameterized test: `todo` now asserts the benign path (never parked failed), `done` keeps the operator-action surfacing (log only, no park). Full engine-core gate suite passes (644/644). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/engine/src/__tests__/executor-recovery.test.ts`:
- Around line 1897-1901: The inline comment starting with "FN-6782" does not
follow the repository's mandated FNXC comment format. Update the comment header
to use the required FNXC:Area-of-product format with proper dating for
grep-ability, replacing the current "FN-6782" prefix with the appropriate
FNXC:Area-of-product identifier. Apply this same format fix to all affected
inline comments in the test file (including the one at line 1931).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e01d3f4-7625-47be-85b5-8ff9ee11fab6
📒 Files selected for processing (1)
packages/engine/src/__tests__/executor-recovery.test.ts
Comment on lines
+1897
to
+1901
| // FN-6782: a paused graph exit that already landed back in `todo` is BENIGN — | ||
| // it must NOT be parked `failed` (that re-fail loop was the retry storm). It | ||
| // logs a benign line, clears the pause-abort marker, and leaves the task in | ||
| // todo for normal scheduling. (Previously this was surfaced as an | ||
| // operator-action failure; see the `done` case below for the still-surfaced path.) |
Contributor
There was a problem hiding this comment.
Use required FNXC-prefixed comment format for new inline comments.
The new comments don’t follow the repo’s mandated FNXC:Area-of-product + dated FNXC log style. Please update these comment headers to match policy.
Suggested edit
- // FN-6782: a paused graph exit that already landed back in `todo` is BENIGN —
+ // FNXC:WorkflowLifecycle 2026-06-20-00:00:
+ // FN-6782: a paused graph exit that already landed back in `todo` is BENIGN —
// it must NOT be parked `failed` (that re-fail loop was the retry storm). It
// logs a benign line, clears the pause-abort marker, and leaves the task in
// todo for normal scheduling. (Previously this was surfaced as an
// operator-action failure; see the `done` case below for the still-surfaced path.)
@@
- // done/archived are terminal — surfaced via log only, never parked failed.
+ // FNXC:WorkflowLifecycle 2026-06-20-00:00:
+ // done/archived are terminal — surfaced via log only, never parked failed.As per coding guidelines, "packages/**/*.{ts,tsx,js,jsx}: Add FNXC_LOG comments ... Write FNXC:Area-of-product in front of all comments for grep-ability."
Also applies to: 1931-1931
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/engine/src/__tests__/executor-recovery.test.ts` around lines 1897 -
1901, The inline comment starting with "FN-6782" does not follow the
repository's mandated FNXC comment format. Update the comment header to use the
required FNXC:Area-of-product format with proper dating for grep-ability,
replacing the current "FN-6782" prefix with the appropriate FNXC:Area-of-product
identifier. Apply this same format fix to all affected inline comments in the
test file (including the one at line 1931).
Source: Coding guidelines
This was referenced Jun 21, 2026
This was referenced Jul 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
A manual global pause → resume could stall the entire board. We hit it live: ~93
task:failedvs 1task:mergedin an hour, zero real throughput, the queue concurrency-starved behind stuck holders.Root cause (full chain)
executor.ts:2814-2880) marks sessionspausedAbortedand disposes them but releases no worktree/semaphore/lease slot.handleGraphFailureparks the taskstatus:"failed"("operator action required") and the early-return path never deletesthis.activeWorktrees— so a task that's been re-queued totodokeeps pinning its worktree slot (the "FN-6756 in todo yet stillmaxWorktrees=3/3holder" symptom). That concurrency-blocks the whole queue.execute()doesn't clearpausedAborted, sogenuinePauseAbortre-fires and re-parks instantly, no backoff → the 75×/hour retry storm.The fix
handleGraphFailure, a pause-abort that left the task intodois now treated as benign — it is not parkedfailed, itclearPausedAborted(task.id)so the next dispatch is clean, and it releases the leaked worktree slot (activeWorktrees.delete). The operator-actionfailedpark is preserved for genuinely stranded non-todocolumns (FN-6478 intact). Placed in the terminalhandleGraphFailurebranch — deliberately not theexecute()finally, which the graph seam re-enters mid-run.recoverPausedAbortFailures) — new self-healing batch-2 sweep that clears any pause-abort park still on the board (status:"failed"+ "operator action required") and requeues it for normal scheduling, so the board self-heals without operator intervention. Respects the existing globalPause/enginePaused skip guard.Correction worth noting
The initial theory was that recovered tasks parked with
status:nullwere unschedulable. Verified from code (scheduler.ts:1288) that the dispatch set iscolumn==="todo" && !paused—status:nullis runnable;status:"queued"is the blocked marker. So no requeue-path change was needed; the queue stall was 100% the leaked worktree slots, not status. (R3 dropped after this finding.)Deferred (follow-up)
listWorktreeHolders()introspection API (today self-healing can only seegetExecutingTaskIds(), already cleared by the time a slot leaks). The leak is now closed at its source (this PR) and parks auto-recover, so the incident is covered; the reaper is defense-in-depth for any future leak path. Tracked as a follow-up.Tests / verification (independently re-run)
pnpm --filter @fusion/engine typecheck→ clean.executor-paused-abort-todo-benign.test.ts,self-healing-paused-abort-recovery.test.ts.executor-pause,self-healing-in-progress-limbo) → 95 passed, 0 failed (re-run by the reviewer, not just the author).Operational note for the live board
The fix only takes effect on the running engine after a restart. Until this merges and the engine restarts, keep the three poisoned tasks (FN-6749/6750/6706) paused — unpausing them on an unfixed engine re-triggers the storm.
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features
Tests