Skip to content

fix(sdk): await session.idle after Copilot send() to prevent premature stage completion - #617

Merged
lavaman131 merged 1 commit into
mainfrom
fix/copilot-send-await-idle
Apr 13, 2026
Merged

fix(sdk): await session.idle after Copilot send() to prevent premature stage completion#617
lavaman131 merged 1 commit into
mainfrom
fix/copilot-send-await-idle

Conversation

@lavaman131

@lavaman131 lavaman131 commented Apr 13, 2026

Copy link
Copy Markdown
Collaborator

Summary

After the sendAndWaitsend refactor (#614), Copilot workflow stages were completing prematurely because send() is fire-and-forget — it returns immediately after queuing the message, before the agent finishes processing. This fix wraps send() at the executor level to block until session.idle (or session.error), aligning Copilot's blocking semantics with Claude's query() and OpenCode's session.prompt().

Key Changes

  • src/sdk/runtime/executor.ts: Intercepts copilotSession.send() after session initialization (step 12a) to wrap the native fire-and-forget call with a promise that resolves on session.idle or rejects on session.error
  • Listeners are registered before nativeSend() is called to prevent a race condition where the agent could finish before the listener attaches
  • Cleans up both event subscriptions (unsubIdle / unsubError) in a shared cleanup() callback to prevent leaks
  • Fully compatible with the SDK's sendAndWait() because _dispatchEvent broadcasts to all registered handlers independently

Root Cause

The sendAndWait→send refactor (#614) moved the waiting responsibility out of the SDK method, but the executor did not compensate by adding its own wait. This left Copilot stages without the blocking guarantee that Claude and OpenCode already had.

Testing

  • Copilot stages now complete only after the agent has finished processing
  • getMessages() returns complete conversation data instead of partial results
  • Session errors surface correctly as rejected promises rather than silent hangs

…e stage completion

After the sendAndWait→send refactor (#614), Copilot stages were marked
complete while the agent was still processing because send() is
fire-and-forget. Wrap send() at the executor level to block until
session.idle (or session.error), matching Claude query() and OpenCode
session.prompt() blocking semantics.
@lavaman131
lavaman131 merged commit 5cc972a into main Apr 13, 2026
4 checks passed
@lavaman131
lavaman131 deleted the fix/copilot-send-await-idle branch April 13, 2026 20:58
@claude claude Bot changed the title fix(sdk): await session.idle after Copilot send() to prevent prematur… fix(sdk): await session.idle after Copilot send() to prevent premature stage completion Apr 13, 2026
@claude

claude Bot commented Apr 13, 2026

Copy link
Copy Markdown

Review — fix(sdk): await session.idle after Copilot send()

Solid, well-motivated fix. The bug diagnosis (stages completing before Copilot finishes because send() is fire-and-forget post-#614) is clear, and wrapping at the executor level is the right layer — it matches the existing provider-specific handling (e.g. cleanupProvider in src/sdk/runtime/executor.ts:610). Listener registration before nativeSend() correctly avoids the idle-before-listen race, and subscribing to both session.idle and session.error closes the "hang forever on failure" hole.

A few things worth addressing as follow-ups:

1. Listener leak when nativeSend() throws — src/sdk/runtime/executor.ts:876

const messageId = await nativeSend(options);
await idle;
return messageId;

Listeners are attached inside the new Promise executor, but cleanup() only runs from inside the idle/error handlers. If nativeSend() rejects (network error, transport issue), control exits via exception before await idle is ever awaited — the two listeners stay attached to the session for its lifetime. Over many failing sends that's a real leak. Suggest hoisting cleanup out of the executor and wrapping the awaits:

let cleanup = () => {};
const idle = new Promise<void>((resolve, reject) => {
  const unsubIdle = copilotSession.on("session.idle", () => { cleanup(); resolve(); });
  const unsubError = copilotSession.on("session.error", (event) => {
    cleanup();
    const data = event.data as { message?: string } | undefined;
    reject(new Error(data?.message ?? "Copilot session error"));
  });
  cleanup = () => { unsubIdle(); unsubError(); };
});
try {
  const messageId = await nativeSend(options);
  await idle;
  return messageId;
} catch (err) {
  cleanup();
  throw err;
}

2. No test coverage

The wrapper is testable without a live Copilot CLI: a fake session with .on() / .send() + a manual event emitter covers (a) idle resolves, (b) error rejects with message, (c) listeners unsubscribe after resolution, (d) leak path if nativeSend throws. src/sdk/runtime/executor.test.ts is the natural home. Given this blocks stage completion for every Copilot workflow, a regression test would be very cheap insurance.

3. Documentation drift

Two places still describe send as fire-and-forget and that's now stale for the stage callback surface:

  • .agents/skills/workflow-creator/SKILL.md:223 — "s.session.send({ prompt }) — fire-and-forget"
  • .agents/skills/workflow-creator/references/failure-modes.md:42, 487 — F10 entry recommends send over sendAndWait "to avoid timeout", but with this wrapper send effectively becomes sendAndWait with no timeout. Users reading these docs will be surprised when their send call blocks.

The comment at executor.ts:843 also still reads "Copilot's send() is fire-and-forget" — true for the native method, but confusing alongside the wrapper it introduces; a one-word tweak ("native send() is fire-and-forget") would help future readers.

4. Infinite-hang risk

This preserves the "no timeout" property from F10 on purpose — good — but the workflow is now fully wedged if session.idle and session.error both fail to fire (e.g. transport hangs, SDK regression). The sibling agents have natural escape hatches (query() completes its async iterator, session.prompt() has HTTP semantics). Worth at least noting in a comment that a disconnect/cancellation path exists elsewhere, or considering an AbortSignal hookup in a future pass — not a blocker for this PR.

5. Concurrent sends on one session (minor)

If a workflow ever issues two send() calls without awaiting between them, both wrappers attach listeners simultaneously and the first idle event resolves both promises. Not a bug today (documented usage is always await), but worth being aware of if parallel multi-turn gets added.

Nits

  • copilotSession.send = async (options) => { ... }options is inferred; explicitly annotating it as the SDK's MessageOptions type (imported via the provider map) would let TS catch shape drift if the SDK updates.
  • Comment on executor.ts:851 pins behavior to the SDK's _dispatchEvent internal — that'll go stale if the SDK ever restructures. Consider rephrasing as "the SDK's public event API fans out to all subscribers" without the private name.

Overall

Correct fix, right layer, good defensive wiring around the idle/error pair. Biggest real concern is the listener leak in the error path (#1); the rest is polish/follow-up.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant