fix(workflows): make agent startup abortable - #147
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The cancellation behavior changes appear correct and well-covered by targeted regression tests, with only a minor error-message clarity nit remaining.
Pull request overview
This PR hardens workflow child-agent lifecycle handling by making runAgent() startup (session creation + extension binding) responsive to AbortSignal cancellation, ensuring cancellation produces an explicit aborted outcome and any late-created sessions are still properly cleaned up.
Changes:
- Installs the abort listener before child-session creation and races startup steps against cancellation.
- Ensures late session creation after cancellation is still owned and disposed via the bounded cleanup path.
- Adds deterministic regression tests covering startup cancellation races and late failures.
File summaries
| File | Description |
|---|---|
| extensions/workflows/runner.ts | Makes startup abortable by wiring abort handling earlier and racing session creation/binding against cancellation, with safe cleanup of late sessions. |
| extensions/workflows/runner.test.ts | Adds regression coverage for pre-abort, abort during creation/binding, late factory/binding failures, and prompt suppression after startup cancellation. |
Review details
Suppressed comments (1)
extensions/workflows/runner.ts:636
- The startup try/catch covers more than session creation (it also races extension binding and installs tool guards), but this error message still says "Failed to create agent session". If binding/tool-guard setup fails, the message is misleading and makes diagnosis harder.
return {
ok: false,
output: "",
error: `Failed to create agent session: ${errorText(error)}${cleanupError ? `; cleanup failed: ${cleanupError}` : ""}`,
aborted: false,
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
tt-a1i
left a comment
There was a problem hiding this comment.
Thanks for the focused lifecycle hardening. I reviewed the startup cancellation races across pre-aborted startup, pending session creation, late factory settlement, extension binding, prompting, and bounded idempotent cleanup. The branch also integrates cleanly with current main; bun run check, the 21 runner tests, and the full suite (874 node tests plus 30 file-search tests) pass. No blocking issues from my side.
Summary
Closes #116.
Make workflow child-agent startup responsive to cancellation during both session creation and extension binding.
When cancellation wins the startup race,
runAgent()now returns an explicit aborted outcome without waiting indefinitely. Any session created after cancellation is still owned and cleaned up, and late startup rejections are observed.Changes
aborted: true.bindExtensions()does not currently accept anAbortSignal, so its internal initialization may still finish in the background. This change stops the workflow from waiting on it indefinitely and safely handles its eventual result.Tests
Added deterministic regression coverage for:
Validation:
extensions/workflows/runner.test.ts: 21/21 passedbun run lint: passedbun run typecheck: passedLocal full-suite validation is affected by existing Windows-specific failures involving process handling, path separators, file modes, and symlink permissions. The runner test suite and all new regression cases pass.