Subagent armTimeout fires against defunct instance if initScheduler rejects

[acoliver]

Description

In SubAgentScope.runInteractive(), prepareRun() calls armTimeout(abortController) which starts a setTimeout. Control then returns to runInteractive() which calls await this.initScheduler(options) outside the try/finally block whose finally calls clearTimeoutHandle().

If initScheduler() rejects, the timeout handle is never cleared. After max_time_minutes the timer fires and calls abortController.abort() against a defunct subagent instance, causing a resource leak and potentially confusing error logs.

Reproduction

1. Configure a subagent with max_time_minutes set
2. Cause initScheduler() to reject (e.g., scheduler factory throws)
3. Observe the timeout fires after the configured minutes despite the subagent already having failed

Expected Behavior

The timeout should be cleared if initScheduler() (or any setup after armTimeout) fails.

Suggested Fix

Either:

• Move armTimeout() to after initScheduler() succeeds, or
• Wrap initScheduler() inside the same try/finally boundary that calls clearTimeoutHandle()

Location

packages/core/src/core/subagent.ts \u2014 runInteractive() method (lines ~337\u2013342)

Context

Identified during code review of PR #1876. Pre-existing issue, not introduced by that PR.

[coderabbitai]

🔗 Related PRs

#649 - fix: add SubAgentScope.dispose() to prevent memory leaks (#648) [merged]
#1436 - fix(core): unify scheduler path for subagent and non-interactive tool execution (Fixes #1426) [merged]
#1452 - Fix task subagent sessionId context propagation (Fixes #1447) [merged]
#1795 - fix(core): rationalize duplicate getOrCreateScheduler wrappers in subagent scheduler setup (Fixes #1787) [merged]
#1840 - fix(subagent): prevent crashes and hangs during subagent launch (Fixes #1838) [merged]

---

📝 Issue Planner

_{Check the box below or use the @coderabbitai plan command to generate an implementation plan and prompts that you can use with your favorite coding assistant.}

✅ Successfully generated implementation plan.

💡 Use @coderabbitai <feedback> to replan

---

🧪 Issue enrichment is currently in open beta.

You can configure auto-planning by selecting labels in the issue_enrichment configuration.

To disable automatic issue enrichment, add the following to your .coderabbit.yaml:

issue_enrichment:
  auto_enrich:
    enabled: false

💬 Have feedback or questions? Drop into our discord!

[coderabbitai]

Coding Plan

Summary

• Wrap initScheduler() inside the existing try/finally block that calls cleanupInteractive()
• Declare scheduler-related variables before the try block so they're accessible in finally
• Handle undefined schedulerDispose in cleanup to cover the failure case
• This follows the same pattern used by runNonInteractive() and maintains existing timeout semantics

Design Choices

Design Choice 1: Should the timeout be armed before or after scheduler initialization?

Options Considered:

1. Move armTimeout() to after initScheduler() succeeds — changes semantics so timeout measures only execution time, not setup time
2. Wrap initScheduler() inside the existing try/finally boundary — maintains current timing semantics while ensuring cleanup on any failure

Chosen Option: 2

Rationale: Wrapping initScheduler() in the try/finally maintains existing behavior and follows the pattern already used by runNonInteractive(), which correctly handles cleanup for all setup code.

💡 User Tips

Regenerate the plan with different choices with @coderabbitai <feedback>.

Implementation Steps

Fix Timeout Handle Leak in runInteractive()

Restructure the control flow in runInteractive() so that initScheduler() is called inside the try/finally block, ensuring clearTimeoutHandle() is always invoked if any setup after armTimeout() fails.

Restructure runInteractive() Control Flow

Move the initScheduler() call inside the try block and ensure the finally block handles cleanup correctly when scheduler initialization fails.

• In packages/core/src/core/subagent.ts, modify runInteractive() (around lines 337-409) to declare scheduler and schedulerDispose variables before the try block, initialized to undefined
• Move the await this.initScheduler(options) call to be the first statement inside the try block, assigning results to the outer-scoped variables
• Update the finally block's call to cleanupInteractive() to handle the case where schedulerDispose may be undefined (if initScheduler() threw before completing)
• Ensure cleanupInteractive() gracefully handles an undefined schedulerDispose parameter by adding a truthy check before invoking it, if not already present

🤖 Prompt for AI agents

Fix a resource leak in `runInteractive()` in
`packages/core/src/core/subagent.ts` by moving `initScheduler()` inside the
existing try/finally block so that `clearTimeoutHandle()` is always called on
failure.

- In `runInteractive()` (around lines 337-409):
- Declare `scheduler` and `schedulerDispose` variables before the try block,
initialized to `undefined`
- Move the `await this.initScheduler(options)` call to be the **first statement
inside the try block**, assigning results to those outer-scoped variables
- Update the finally block's call to `cleanupInteractive()` to handle the case
where `schedulerDispose` may be `undefined` (i.e., if `initScheduler()` threw
before completing)
- Ensure `cleanupInteractive()` gracefully handles an `undefined`
`schedulerDispose` parameter by adding a truthy check before invoking it, if not
already present
- Use `runNonInteractive()` as a reference pattern, which already correctly
places all setup code inside the try/finally boundary

Research

The SubAgentScope class in packages/core/src/core/subagent.ts manages subagent execution with timeout handling. The runInteractive() method currently calls prepareRun() (which arms the timeout), then initScheduler() outside the try/finally block, and only clears the timeout in cleanupInteractive() within the finally block. The sister method runNonInteractive() correctly places all setup inside the try/finally boundary, providing a reference pattern for the fix.

---

🚀 Next Steps

🤖 All AI agent prompts combined

Fix a resource leak in `runInteractive()` in
`packages/core/src/core/subagent.ts` by moving `initScheduler()` inside the
existing try/finally block so that `clearTimeoutHandle()` is always called on
failure.

- In `runInteractive()` (around lines 337-409):
- Declare `scheduler` and `schedulerDispose` variables before the try block,
initialized to `undefined`
- Move the `await this.initScheduler(options)` call to be the **first statement
inside the try block**, assigning results to those outer-scoped variables
- Update the finally block's call to `cleanupInteractive()` to handle the case
where `schedulerDispose` may be `undefined` (i.e., if `initScheduler()` threw
before completing)
- Ensure `cleanupInteractive()` gracefully handles an `undefined`
`schedulerDispose` parameter by adding a truthy check before invoking it, if not
already present
- Use `runNonInteractive()` as a reference pattern, which already correctly
places all setup code inside the try/finally boundary

💡 Iterate on the plan with: @coderabbitai <feedback>

Example Feedback
- `@coderabbitai` You can skip phase 3. Add a simple unit test case for phase 2.
- `@coderabbitai` For design choice 1 go ahead with option 3 and replan.

---

💬 Have feedback or questions? Drop into our discord!