Merge remote-tracking branch 'origin/worktree-agent-handle' into worktree-bash-owner-token
This commit is contained in:
@@ -61,7 +61,7 @@ A `session/prompt` resolves (or rejects) exactly once, keyed off the canonical s
|
|||||||
|
|
||||||
## Disposal & disconnect
|
## 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)
|
## Known limitations (tracked TODOs)
|
||||||
|
|
||||||
|
|||||||
@@ -26,7 +26,7 @@ import type { ContentBlock as AcpContentBlock, StopReason } from '@agentclientpr
|
|||||||
*
|
*
|
||||||
* - `completed` → `end_turn` (the model chose to stop)
|
* - `completed` → `end_turn` (the model chose to stop)
|
||||||
* - `max-tokens` → `max_tokens` (cut off at the output-token ceiling)
|
* - `max-tokens` → `max_tokens` (cut off at the output-token ceiling)
|
||||||
* - `aborted` → `cancelled` (an `agent.abort()`, e.g. from `session/cancel`)
|
* - `aborted` → `cancelled` (a step abort or a queue-aware `agent.cancel()`, e.g. from `session/cancel`)
|
||||||
* - `error` → `end_turn` (defensive fallback only: the bridge REJECTS the
|
* - `error` → `end_turn` (defensive fallback only: the bridge REJECTS the
|
||||||
* `session/prompt` RPC on an error turn BEFORE calling this, so
|
* `session/prompt` RPC on an error turn BEFORE calling this, so
|
||||||
* a client sees a JSON-RPC error, not a stop reason — see
|
* a client sees a JSON-RPC error, not a stop reason — see
|
||||||
|
|||||||
@@ -13,7 +13,9 @@
|
|||||||
* - `session/load` → `ctx.agents.resume(...)` then replay the event log
|
* - `session/load` → `ctx.agents.resume(...)` then replay the event log
|
||||||
* - `session/prompt` → `agent.send()`, settle on the owning turn's end (a turn
|
* - `session/prompt` → `agent.send()`, settle on the owning turn's end (a turn
|
||||||
* that ends in `error` rejects the RPC)
|
* that ends in `error` rejects the RPC)
|
||||||
* - `session/cancel` → `agent.abort()` + settle the in-flight prompt
|
* - `session/cancel` → `agent.cancel()` (the queue-aware cancel: aborts a
|
||||||
|
* running step, clears queued + steering work, and drops a
|
||||||
|
* turn about to start) + settle the in-flight prompt
|
||||||
*
|
*
|
||||||
* Multi-session (RFC 011): N concurrent sessions per connection, each mapped to
|
* Multi-session (RFC 011): N concurrent sessions per connection, each mapped to
|
||||||
* its own `ReactLoopAgent`. Sessions are keyed by id in `sessions` (forward) with an
|
* its own `ReactLoopAgent`. Sessions are keyed by id in `sessions` (forward) with an
|
||||||
@@ -628,19 +630,21 @@ export function apply(ctx: Context, config: AcpConfig): void {
|
|||||||
* Tear ALL live sessions down to quiescence (AGENTS.md "dispose must reach
|
* Tear ALL live sessions down to quiescence (AGENTS.md "dispose must reach
|
||||||
* quiescence"): for each session settle any pending prompt `cancelled`, then
|
* quiescence"): for each session settle any pending prompt `cancelled`, then
|
||||||
* run that session's {@link AgentHandle} `dispose()` — which stops the loop
|
* run that session's {@link AgentHandle} `dispose()` — which stops the loop
|
||||||
* with the queue-aware cancel, AWAITS the loop's exit (the final
|
* (sets `disposed`, aborts the in-flight step), AWAITS the loop's exit (the
|
||||||
* `turn/end` + `session/flush` are captured while `onAppend` is still
|
* final `turn/end` + `session/flush` are captured while `onAppend` is still
|
||||||
* attached), unregisters the agent, and removes its session from the store.
|
* attached), unregisters the agent, and removes its session from the store.
|
||||||
* The per-session disposes run in parallel. Idempotent — clears the `sessions`
|
* 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.
|
* map first and memoizes, so a second call (close racing dispose) is a no-op.
|
||||||
* Shared by Cordis disposal AND client disconnect (`conn.closed`).
|
* Shared by Cordis disposal AND client disconnect (`conn.closed`).
|
||||||
*
|
*
|
||||||
* Per-agent disposal closes the former pre-step best-effort window: the
|
* Per-agent disposal closes the former pre-step best-effort window — but via
|
||||||
* queue-aware `cancel()` (RFC 011) drops a turn about to start, so a queued-
|
* the DISPOSED path, not `cancel()`: the start-disposer resolves `handle.disposed`,
|
||||||
* but-not-yet-running prompt never runs after teardown. A bare client
|
* which wakes the parked loop, and `isDisposed()` breaks the loop before a
|
||||||
* disconnect (resolves `conn.closed` WITHOUT disposing the fiber) thus leaves
|
* queued-but-not-yet-running turn can start (a turn cut off mid-flight ends
|
||||||
* NO registered agent and NO session-store entry — not an idled-but-still-
|
* with reason `disposed`, not `aborted`). A bare client disconnect (resolves
|
||||||
* registered one. When the fiber IS disposed (whole-context or an ACP-only HMR
|
* `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
|
* `acpFiber.dispose()`), this same memoized teardown runs first; the factory's
|
||||||
* register+start+session effects are ALSO bound to the bridge fiber (the
|
* register+start+session effects are ALSO bound to the bridge fiber (the
|
||||||
* factory is reached through this bridge's traceable service proxy, so
|
* factory is reached through this bridge's traceable service proxy, so
|
||||||
@@ -665,10 +669,10 @@ export function apply(ctx: Context, config: AcpConfig): void {
|
|||||||
await Promise.all(recs.map(async (rec) => {
|
await Promise.all(recs.map(async (rec) => {
|
||||||
settlePrompt(rec, 'cancelled')
|
settlePrompt(rec, 'cancelled')
|
||||||
// Per-agent dispose (the AgentHandle disposer): unregister this agent,
|
// Per-agent dispose (the AgentHandle disposer): unregister this agent,
|
||||||
// stop its loop with the queue-aware cancel, await quiescence (the loop
|
// stop its loop (sets disposed + aborts the in-flight step), await
|
||||||
// exit + final flush), and remove its session — so a bare client
|
// quiescence (the loop exit + final flush), and remove its session — so
|
||||||
// disconnect leaves NO registered agent and NO session-store entry, not
|
// a bare client disconnect leaves NO registered agent and NO
|
||||||
// just an idled-but-still-registered one.
|
// session-store entry, not just an idled-but-still-registered one.
|
||||||
await rec.dispose()
|
await rec.dispose()
|
||||||
}))
|
}))
|
||||||
})()
|
})()
|
||||||
|
|||||||
@@ -34,6 +34,17 @@ export class ReactLoopAgent implements Agent {
|
|||||||
* leave it set to wrongly drop a later prompt.
|
* leave it set to wrongly drop a later prompt.
|
||||||
*/
|
*/
|
||||||
private cancelRequested = false
|
private cancelRequested = false
|
||||||
|
/**
|
||||||
|
* The resolved reason for the pending {@link cancel} (`reason ?? 'cancelled'`),
|
||||||
|
* read by the driver loop's marker branches so a turn dropped in a
|
||||||
|
* marker-only window (pre-step / continuation, where no `AbortController`
|
||||||
|
* carries the reason) ends with the SAME `{kind:'aborted', reason}` the
|
||||||
|
* mid-step abort path produces from `abort.signal.reason`. Without this the
|
||||||
|
* caller's `cancel(reason)` would be silently replaced by the literal
|
||||||
|
* 'cancelled' whenever the cancel landed outside a running step — making the
|
||||||
|
* logged reason race-dependent and the public `reason?` param half-effective.
|
||||||
|
*/
|
||||||
|
private cancelReason = 'cancelled'
|
||||||
private disposed: Promise<void>
|
private disposed: Promise<void>
|
||||||
private resolveDisposed!: () => void
|
private resolveDisposed!: () => void
|
||||||
/** Resolves when the driver loop has fully exited (tests/disposal). */
|
/** Resolves when the driver loop has fully exited (tests/disposal). */
|
||||||
@@ -196,6 +207,10 @@ export class ReactLoopAgent implements Agent {
|
|||||||
// precisely to cover it.
|
// precisely to cover it.
|
||||||
if (this._status === 'running' || this.currentAbort !== undefined || this.inbox.hasQueued || this.inbox.hasSteering) {
|
if (this._status === 'running' || this.currentAbort !== undefined || this.inbox.hasQueued || this.inbox.hasSteering) {
|
||||||
this.cancelRequested = true
|
this.cancelRequested = true
|
||||||
|
// Capture the resolved reason for the marker-only windows (pre-step /
|
||||||
|
// continuation). The mid-step path reads it from abort.signal.reason
|
||||||
|
// below; the marker path reads it via the LoopHandle's cancelReason().
|
||||||
|
this.cancelReason = reason ?? 'cancelled'
|
||||||
}
|
}
|
||||||
// Drop all pending queued + steering work (un-started prompts never run; the
|
// Drop all pending queued + steering work (un-started prompts never run; the
|
||||||
// cancelled turn's steering is not re-enqueued). Cleared directly even when
|
// cancelled turn's steering is not re-enqueued). Cleared directly even when
|
||||||
@@ -251,6 +266,7 @@ export class ReactLoopAgent implements Agent {
|
|||||||
disposed: this.disposed,
|
disposed: this.disposed,
|
||||||
isDisposed: () => this._status === 'disposed',
|
isDisposed: () => this._status === 'disposed',
|
||||||
isCancelled: () => this.cancelRequested,
|
isCancelled: () => this.cancelRequested,
|
||||||
|
cancelReason: () => this.cancelReason,
|
||||||
clearCancel: () => { this.cancelRequested = false },
|
clearCancel: () => { this.cancelRequested = false },
|
||||||
// Settle whenIdle() waiters WITHOUT a status transition — the pre-step
|
// Settle whenIdle() waiters WITHOUT a status transition — the pre-step
|
||||||
// cancel-skip path drops the about-to-run turn and re-parks without ever
|
// cancel-skip path drops the about-to-run turn and re-parks without ever
|
||||||
|
|||||||
@@ -116,6 +116,14 @@ export interface LoopHandle {
|
|||||||
* marker governs exactly one cancellation and never leaks to a later prompt.
|
* marker governs exactly one cancellation and never leaks to a later prompt.
|
||||||
*/
|
*/
|
||||||
isCancelled(): boolean
|
isCancelled(): boolean
|
||||||
|
/**
|
||||||
|
* The resolved reason for the pending cancel (`reason ?? 'cancelled'`), read
|
||||||
|
* by the marker branches (pre-step / continuation) so a turn dropped where no
|
||||||
|
* `AbortController` carries the reason still records the caller's
|
||||||
|
* `cancel(reason)` value — matching the mid-step abort path. Only meaningful
|
||||||
|
* when {@link isCancelled} is true.
|
||||||
|
*/
|
||||||
|
cancelReason(): string
|
||||||
/** Clear the cancel marker (called once per iteration after the turn returns). */
|
/** Clear the cancel marker (called once per iteration after the turn returns). */
|
||||||
clearCancel(): void
|
clearCancel(): void
|
||||||
/**
|
/**
|
||||||
@@ -396,7 +404,7 @@ async function runTurn(ctx: Context, agent: ReactLoopAgent, handle: LoopHandle,
|
|||||||
// already-appended step/start.
|
// already-appended step/start.
|
||||||
if (handle.isCancelled()) {
|
if (handle.isCancelled()) {
|
||||||
handle.setAbort(undefined)
|
handle.setAbort(undefined)
|
||||||
reason = { kind: 'aborted', reason: 'cancelled' }
|
reason = { kind: 'aborted', reason: handle.cancelReason() }
|
||||||
closeStep()
|
closeStep()
|
||||||
break
|
break
|
||||||
}
|
}
|
||||||
@@ -466,7 +474,7 @@ async function runTurn(ctx: Context, agent: ReactLoopAgent, handle: LoopHandle,
|
|||||||
// ends the turn here. cancel() also cleared the steering FIFO, so the
|
// ends the turn here. cancel() also cleared the steering FIFO, so the
|
||||||
// override above did not re-arm continuation.
|
// override above did not re-arm continuation.
|
||||||
if (handle.isCancelled()) {
|
if (handle.isCancelled()) {
|
||||||
reason = { kind: 'aborted', reason: 'cancelled' }
|
reason = { kind: 'aborted', reason: handle.cancelReason() }
|
||||||
break
|
break
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -186,9 +186,11 @@ describe('Agent.cancel()', () => {
|
|||||||
await waitForIdle(ctx, agent)
|
await waitForIdle(ctx, agent)
|
||||||
dispose()
|
dispose()
|
||||||
|
|
||||||
// No step streamed (the model never ran), and the turn ended aborted.
|
// No step streamed (the model never ran), and the turn ended aborted with
|
||||||
|
// the CALLER's reason — the marker carries `cancel(reason)` through even
|
||||||
|
// though no AbortController observed it in this window.
|
||||||
expect(streamed).toBe(false)
|
expect(streamed).toBe(false)
|
||||||
expect(reasons).toEqual([{ kind: 'aborted', reason: 'cancelled' }])
|
expect(reasons).toEqual([{ kind: 'aborted', reason: 'from turn-start' }])
|
||||||
})
|
})
|
||||||
|
|
||||||
it('cancel during the continuation window ends the turn aborted and runs no further step', async () => {
|
it('cancel during the continuation window ends the turn aborted and runs no further step', async () => {
|
||||||
@@ -219,9 +221,10 @@ describe('Agent.cancel()', () => {
|
|||||||
await waitForIdle(ctx, agent)
|
await waitForIdle(ctx, agent)
|
||||||
|
|
||||||
// Only ONE step ran (the second was cancelled in the continuation window),
|
// Only ONE step ran (the second was cancelled in the continuation window),
|
||||||
// and the turn ended aborted.
|
// and the turn ended aborted with the CALLER's reason (carried by the
|
||||||
|
// marker, since the finished step's AbortController was already cleared).
|
||||||
expect(steps).toBe(1)
|
expect(steps).toBe(1)
|
||||||
expect(reasons).toEqual([{ kind: 'aborted', reason: 'cancelled' }])
|
expect(reasons).toEqual([{ kind: 'aborted', reason: 'from continuation' }])
|
||||||
})
|
})
|
||||||
|
|
||||||
it('cancel from a synchronous agent/status(running) listener drops the turn (window 2)', async () => {
|
it('cancel from a synchronous agent/status(running) listener drops the turn (window 2)', async () => {
|
||||||
|
|||||||
Reference in New Issue
Block a user