Conversation
WalkthroughIntroduces an overall execution timeout in planner.ts with AbortSignal propagation and deterministic cleanup. Updates test executors to the new execute(tool, input, abort) signature. Adds a timeout-focused unit test and adapts existing tests for fallback behavior and planner injection via a stub. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Planner
participant Executor
participant Timer as OverallTimeout(setTimeout)
Client->>Planner: plan(ctx with timeout_ms)
activate Planner
Planner->>Timer: start(timeout_ms, abort 'overall-timeout')
note over Planner,Timer: Planner stores handle and unrefs if supported
Planner->>Executor: execute(tool, input, overallAbort)
alt Executor completes before timeout
Executor-->>Planner: ExecutionResult(status: success|failure)
Planner->>Timer: clear()
Planner-->>Client: PlanResult with exec result
else Timeout fires
Timer-->>Planner: abort signal ('overall-timeout')
Executor-->>Planner: ExecutionResult(status: timeout)
Planner->>Timer: clear()
Planner-->>Client: PlanResult with status 'timeout'
end
opt Fallback iteration
Planner->>Executor: execute(next candidate,...)
alt overallAbort already signaled
Planner->>Timer: clear()
Planner-->>Client: Failure (ALL_CANDIDATES_FAILED or timeout)
end
end
deactivate Planner
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/planner/planner.ts(2 hunks)tests/e2e/plan.e2e.test.ts(2 hunks)tests/e2e/tools.e2e.test.ts(2 hunks)tests/unit/planner.fallback.test.ts(1 hunks)tests/unit/planner.timeout.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
tests/e2e/tools.e2e.test.ts (2)
src/planner/contracts.ts (3)
IPlanner(63-65)PlanContext(10-23)PlanResult(40-46)src/app.ts (1)
buildApp(14-30)
tests/unit/planner.fallback.test.ts (1)
src/planner/contracts.ts (1)
ExecutionResult(32-37)
src/planner/planner.ts (1)
src/metrics/metrics.ts (2)
plannerSelectionTotal(21-25)plannerFallbacksTotal(28-32)
tests/e2e/plan.e2e.test.ts (1)
src/planner/contracts.ts (2)
JsonRecord(7-7)ExecutionResult(32-37)
tests/unit/planner.timeout.test.ts (5)
src/planner/contracts.ts (3)
IToolExecutor(54-60)JsonRecord(7-7)ExecutionResult(32-37)src/registry/service.ts (1)
IRegistryService(4-4)src/planner/planner.ts (1)
Planner(26-132)src/planner/scoring.simple.ts (1)
SimpleScorer(40-54)src/tracing/traceStore.ts (1)
TraceStore(16-34)
🔇 Additional comments (4)
tests/unit/planner.timeout.test.ts (1)
15-34: Hanging executor behaves perfectly under overall abortAsync wait on the planner’s AbortSignal and returning a timeout result keeps the test deterministic and mirrors the real executor contract nicely.
tests/e2e/tools.e2e.test.ts (1)
7-31: Solid planner stub wiringInjecting a minimal
IPlannerstub keeps the tools routes focused while satisfying the new DI requirement—clean approach.tests/e2e/plan.e2e.test.ts (1)
24-34: Updated stub matches executor contractThe three-argument
executesignature keeps the e2e flow aligned with the planner’s expectations—looks good.tests/unit/planner.fallback.test.ts (1)
22-48: Fallback path test stays in step with executor APIAdjusting the stub to fail once (non-timeout) and then succeed keeps the fallback scenario realistic and aligned with the new signature. Nicely done.
| if (overallController.signal.aborted) { | ||
| if (overallTimeout) clearTimeout(overallTimeout); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // No candidate succeeded | ||
| if (overallTimeout) clearTimeout(overallTimeout); | ||
| result.execution = { status: 'failure', error: 'ALL_CANDIDATES_FAILED' }; |
There was a problem hiding this comment.
Surface overall timeout instead of “ALL_CANDIDATES_FAILED”
When the overall abort fires during fallback (e.g., deadline expires right after a failure), we break out of the loop, but the final result remains { status: 'failure', error: 'ALL_CANDIDATES_FAILED' }. That masks the timeout as a generic failure. We should return { status: 'timeout', error: 'overall-timeout' } (and record a matching trace event) whenever overallController.signal.aborted is true so callers receive the correct cause.
🤖 Prompt for AI Agents
In src/planner/planner.ts around lines 120 to 128, the code breaks out of the
candidate loop when overallController.signal.aborted but still sets
result.execution = { status: 'failure', error: 'ALL_CANDIDATES_FAILED' }; update
that branch so when overallController.signal.aborted is true you clear the
overallTimeout (if set), set result.execution = { status: 'timeout', error:
'overall-timeout' }, and record a matching trace event for the overall timeout
before breaking/returning, ensuring callers see the actual timeout cause instead
of ALL_CANDIDATES_FAILED.
1 participant
Summary by CodeRabbit
Bug Fixes
Tests