docs(acp): correct teardown wording — dispose uses the disposed path, not cancel() (review)
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.)
This commit is contained in:
@@ -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)
|
||||
|
||||
|
||||
@@ -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()
|
||||
}))
|
||||
})()
|
||||
|
||||
Reference in New Issue
Block a user