Skip to content

fix(server): pressing Stop now clears orphaned delegated follow-ups - #21607

Open
sybeeria wants to merge 1 commit into
pingdotgg:mainfrom
sybeeria:fix/stop-clears-orphaned-followups
Open

sybeeria wants to merge 1 commit into
pingdotgg:mainfrom
sybeeria:fix/stop-clears-orphaned-followups

Conversation

@sybeeria

Copy link
Copy Markdown

Problem

#20310 added a repair that cancels app-owned follow-ups left running after losing their child-run binding (the #19002 bug; #19019 prevents new ones). The repair only runs on thread.stop, an internal command the server sends to delegated child threads. The Stop a user presses sends run.interrupt with holdQueue: true, so it never reaches the repair for the thread being stopped. On main, Stop interrupts the orphan's turn item and clears the waiting banner, but the task and its node stay running. Lineage can keep showing the agent as Running, and task_status keeps reporting it as running.

Change

cancelIdleOrphans moves out of dispatchThreadStop with its guards and events unchanged, so run.interrupt with holdQueue: true can also run it on the stopped thread. thread.stop keeps its two call sites, and each Stop path runs the repair once.

On the user's Stop, the repair writes to its own copy of the pending events, and they are merged only if it succeeds. Otherwise it logs a warning, so a child that can't be read does not make Stop itself fail. thread.stop keeps its existing error behavior.

Scope and approval

Refs #19002. In the maintainer triage, the fix includes letting "Stop settle stuck app-owned nodes whose child is idle". #20310 implemented this for thread.stop; this PR applies the same repair to the Stop users press. Server only; no contract or client change.

Verification

macOS arm64, base 1451e7cb.

  • New case Stop clears an orphaned follow-up only when its child is finished (idle via run.interrupt) in ThreadStop.test.ts. It reuses the fix(server): parent Stop clears orphaned delegated follow-ups #20310 fixture and dispatches the client's Stop command on the completed parent run. On main it fails with expected 'running' to equal 'cancelled'; with this change it passes. The 12 existing thread.stop cases are unchanged apart from their titles.
  • New Stop still stops the thread when a delegated child cannot be read. Without the isolation it fails with OrchestratorProjectionError; it passes with the change, and on main.
  • vp test run for ThreadStop, BackgroundWorkStop.integration, DelegatedCompletionDelivery, OrchestratorMcpToolkit.integration, SteeringCompletion.integration, and ProjectionRecovery: 6 files, 95 tests pass.
  • vp fmt --check, t3 typecheck, and git diff --check pass. vp lint reports only the two warnings already on main (the moved Schema.is and the unused layerUnavailable).
  • Real data: I took a read-only SQLite backup of a database where a top-level thread had five orphaned follow-ups, all with the same finished child. On that copy I dispatched the client's Stop command through the orchestrator, with the effect worker off and no provider sessions. On main, the five turn items became interrupted while the five tasks and nodes stayed running. With this change, exactly those five tasks, nodes, and turn items became cancelled, and the thread's other 36 tasks did not change.

Not checked: a live provider session or a pass in a real client.

Model and harness: GPT-6.1-Sol in Codex (investigation, implementation, and review), coordinated by Claude Opus 5.5 in Claude Code, running in T3 Code.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Oct 19, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — The existing Stop path now performs orphan repair and enqueues recursive delegated-task stopping, which can affect active child workflows across threads rather than only correcting stale status. Tests cover the repair and unreadable-child cases, but the broader production side effect warrants human review.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 19, 2026

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 6bc9616d-b788-4542-baca-6b036e7b787e
📥 Commits

Reviewing files that changed from the base of the PR and between 1451e7c and 5936612.

📒 Files selected for processing: 2
  • apps/server/src/orchestration-v2/Orchestrator.ts
  • apps/server/src/orchestration-v2/ThreadStop.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

The orchestrator now reuses orphan cancellation across stop paths and attempts the repair during held-queue interrupts. Tests cover orphaned follow-ups for both stop types and verify that an invalid delegated-child run projection does not prevent interrupting the parent.

Changes: Orphaned subagent cancellation

Layer / File(s) Summary
Extract and reuse orphan cancellation
apps/server/src/orchestration-v2/Orchestrator.ts
The orchestrator extracts the orphan check and cancellation logic into a helper. It invokes the helper after successful interrupts and in the fallback stop path.
Repair orphans during held-queue interrupts
apps/server/src/orchestration-v2/Orchestrator.ts, apps/server/src/orchestration-v2/ThreadStop.test.ts
For run.interrupt commands with holdQueue: true, dispatch attempts repair with a copy of the events. It keeps the original events if repair fails. Tests cover both stop types and a corrupted child run projection.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: 1kuna

Merge Risk: ⚪ Minimal · up to 59366

Stop and held-queue interrupt now clean up orphaned follow-ups. A failure while reading a child thread is logged and does not block the interrupt. No merge-blocking risk was identified.

Security architecture risk: 🔵 Low · up to 59366

The change reuses existing Stop safeguards and prevents failed cleanup from blocking Stop. No new authorization bypass was established, although concurrent child restart behavior remains incompletely verified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • observed — A caller controlling the existing command's threadId, runId, and holdQueue can trigger repair of every qualifying orphan in the selected parent thread. Repair writes parent task/node/turn-item metadata, not child-thread state. The existing downstream stop effect separately traverses app-owned children.

Security Findings and Attack Paths

  • inferred — No introduced authorization bypass was established on the inspected path. The additional mutation is reached through the existing operation-authorized command, with inherited parent-lineage and child-idleness guards. This bounded conclusion does not establish safety for unexamined surfaces.

Trust Boundaries and Controls

  • observed — WebSocket dispatch uses authenticated-session environment scopes and requires orchestration:operate. The inspected authorization layer is environment-scoped, not a demonstrated per-project ownership boundary. The direct MCP interrupt route does not set holdQueue and therefore does not gain this repair behavior.

Resilience and Maintainability Implications

  • inferred — Parent metadata cancellation does not suppress downstream child stopping: that consumer scans app-owned children regardless of task status and uses deterministic child-stop command IDs. Parent commands are locked, but the repair does not establish an atomic parent/child snapshot; all child promotion and restart interleavings remain incompletely verified. The idle predicate itself predates this PR.
Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title is concise, specific, and accurately describes the main change: user-initiated Stop now clears orphaned delegated follow-ups.
Description check Passed The description includes complete Problem, Change, Scope and approval, and Verification sections. It explains the issue, implementation, approval reference, focused tests, broader checks, observed res…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description includes complete Problem, Change, Scope and approval, and Verification sections. It explains the issue, implementation, approval reference, focused tests, broader checks, observed results, and limitations.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant