From de0c4605bd59918667008b155bb6c443e5db46e6 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sat, 20 Jun 2026 11:55:56 +0800 Subject: [PATCH] =?UTF-8?q?docs(acp):=20correct=20teardown=20wording=20?= =?UTF-8?q?=E2=80=94=20dispose=20uses=20the=20disposed=20path,=20not=20can?= =?UTF-8?q?cel()=20(review)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A reviewer noted the quiesce() comment + ACP README said `AgentHandle.dispose()` stops the loop "with the queue-aware cancel", but the handle delegates to the start-disposer's `stop(); await agent.done`, where `stop()` sets `disposed` and aborts the current controller — it does NOT call `agent.cancel()`. The pre-step teardown window is still closed (the disposed promise wakes the parked loop and `isDisposed()` breaks before a turn starts), but the mechanism is the DISPOSED path and a mid-flight turn ends with reason `disposed`, not `aborted`. Corrected the comment and the README to describe the actual path. (This commit follows the merge of PR C's `cancel(reason)` fix up into this branch.) --- packages/acp/README.md | 2 +- packages/acp/src/index.ts | 26 ++++++++++++++------------ 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/packages/acp/README.md b/packages/acp/README.md index 5cdfa3a202..78c35eff86 100644 --- a/packages/acp/README.md +++ b/packages/acp/README.md @@ -61,7 +61,7 @@ A `session/prompt` resolves (or rejects) exactly once, keyed off the canonical s ## Disposal & disconnect -Teardown reaches quiescence: for EVERY live session settle any pending prompt as `cancelled`, then run that session's [`AgentHandle`](../agent/README.md) `dispose()` — which stops the loop with the queue-aware `cancel()`, `await`s the loop's exit (the final `turn/end` + `session/flush` are captured while the session is still attached), unregisters the agent, and removes its session from the store. The per-session disposes run in parallel. The same teardown runs on a **client disconnect** (`conn.closed` resolves when the editor quits / the transport EOFs), so a vanished client never leaves an orphaned running — or idled-but-still-registered — agent whose `session/update` writes are silently swallowed. The two paths are idempotent and memoized (the first clears the `sessions` map; a second caller awaits the same teardown promise). +Teardown reaches quiescence: for EVERY live session settle any pending prompt as `cancelled`, then run that session's [`AgentHandle`](../agent/README.md) `dispose()` — which stops the loop (sets `disposed` + aborts the in-flight step), `await`s the loop's exit (the final `turn/end` + `session/flush` are captured while the session is still attached), unregisters the agent, and removes its session from the store. A turn cut off mid-flight by teardown ends with reason `disposed` (not `aborted` — `dispose()` uses the disposed path, not `session/cancel`'s queue-aware `cancel()`). The per-session disposes run in parallel. The same teardown runs on a **client disconnect** (`conn.closed` resolves when the editor quits / the transport EOFs), so a vanished client never leaves an orphaned running — or idled-but-still-registered — agent whose `session/update` writes are silently swallowed. The two paths are idempotent and memoized (the first clears the `sessions` map; a second caller awaits the same teardown promise). ## Known limitations (tracked TODOs) diff --git a/packages/acp/src/index.ts b/packages/acp/src/index.ts index d6eda1cc04..d107c2c0cb 100644 --- a/packages/acp/src/index.ts +++ b/packages/acp/src/index.ts @@ -630,19 +630,21 @@ export function apply(ctx: Context, config: AcpConfig): void { * Tear ALL live sessions down to quiescence (AGENTS.md "dispose must reach * quiescence"): for each session settle any pending prompt `cancelled`, then * run that session's {@link AgentHandle} `dispose()` — which stops the loop - * with the queue-aware cancel, AWAITS the loop's exit (the final - * `turn/end` + `session/flush` are captured while `onAppend` is still + * (sets `disposed`, aborts the in-flight step), AWAITS the loop's exit (the + * final `turn/end` + `session/flush` are captured while `onAppend` is still * attached), unregisters the agent, and removes its session from the store. * The per-session disposes run in parallel. Idempotent — clears the `sessions` * map first and memoizes, so a second call (close racing dispose) is a no-op. * Shared by Cordis disposal AND client disconnect (`conn.closed`). * - * Per-agent disposal closes the former pre-step best-effort window: the - * queue-aware `cancel()` (RFC 011) drops a turn about to start, so a queued- - * but-not-yet-running prompt never runs after teardown. A bare client - * disconnect (resolves `conn.closed` WITHOUT disposing the fiber) thus leaves - * NO registered agent and NO session-store entry — not an idled-but-still- - * registered one. When the fiber IS disposed (whole-context or an ACP-only HMR + * Per-agent disposal closes the former pre-step best-effort window — but via + * the DISPOSED path, not `cancel()`: the start-disposer resolves `handle.disposed`, + * which wakes the parked loop, and `isDisposed()` breaks the loop before a + * queued-but-not-yet-running turn can start (a turn cut off mid-flight ends + * with reason `disposed`, not `aborted`). A bare client disconnect (resolves + * `conn.closed` WITHOUT disposing the fiber) thus leaves NO registered agent + * and NO session-store entry — not an idled-but-still-registered one. When the + * fiber IS disposed (whole-context or an ACP-only HMR * `acpFiber.dispose()`), this same memoized teardown runs first; the factory's * register+start+session effects are ALSO bound to the bridge fiber (the * factory is reached through this bridge's traceable service proxy, so @@ -667,10 +669,10 @@ export function apply(ctx: Context, config: AcpConfig): void { await Promise.all(recs.map(async (rec) => { settlePrompt(rec, 'cancelled') // Per-agent dispose (the AgentHandle disposer): unregister this agent, - // stop its loop with the queue-aware cancel, await quiescence (the loop - // exit + final flush), and remove its session — so a bare client - // disconnect leaves NO registered agent and NO session-store entry, not - // just an idled-but-still-registered one. + // stop its loop (sets disposed + aborts the in-flight step), await + // quiescence (the loop exit + final flush), and remove its session — so + // a bare client disconnect leaves NO registered agent and NO + // session-store entry, not just an idled-but-still-registered one. await rec.dispose() })) })()