fix(acp): drain updates before end turn (#40422)
This commit is contained in:
@@ -40,7 +40,10 @@ export class Subscription {
|
|||||||
private readonly abort = new AbortController()
|
private readonly abort = new AbortController()
|
||||||
private readonly shellSnapshots = new Map<string, string>()
|
private readonly shellSnapshots = new Map<string, string>()
|
||||||
private readonly toolStarts = new Set<string>()
|
private readonly toolStarts = new Set<string>()
|
||||||
|
private readonly connectionWaiters = new Set<() => void>()
|
||||||
|
private readonly idleWaiters = new Map<string, Set<ReturnType<typeof signal>>>()
|
||||||
private readonly permission: ACPPermission.Handler
|
private readonly permission: ACPPermission.Handler
|
||||||
|
private connected = false
|
||||||
private started = false
|
private started = false
|
||||||
|
|
||||||
constructor(
|
constructor(
|
||||||
@@ -63,10 +66,35 @@ export class Subscription {
|
|||||||
|
|
||||||
stop() {
|
stop() {
|
||||||
this.abort.abort()
|
this.abort.abort()
|
||||||
|
this.disconnected()
|
||||||
|
for (const resolve of this.connectionWaiters) resolve()
|
||||||
|
this.connectionWaiters.clear()
|
||||||
|
}
|
||||||
|
|
||||||
|
async runUntilIdle<A>(sessionId: string, request: () => Promise<A>) {
|
||||||
|
await this.waitUntilConnected()
|
||||||
|
const waiter = signal()
|
||||||
|
const waiters = this.idleWaiters.get(sessionId) ?? new Set()
|
||||||
|
waiters.add(waiter)
|
||||||
|
this.idleWaiters.set(sessionId, waiters)
|
||||||
|
|
||||||
|
try {
|
||||||
|
// Idle is queued after the turn's events, and this subscription awaits each update in order.
|
||||||
|
void waiter.promise.catch(() => {})
|
||||||
|
const response = await request()
|
||||||
|
await waiter.promise
|
||||||
|
return response
|
||||||
|
} finally {
|
||||||
|
waiters.delete(waiter)
|
||||||
|
if (waiters.size === 0) this.idleWaiters.delete(sessionId)
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
async handle(event: Event) {
|
async handle(event: Event) {
|
||||||
switch (event.type) {
|
switch (event.type) {
|
||||||
|
case "session.status":
|
||||||
|
if (event.properties.status.type === "idle") this.idle(event.properties.sessionID)
|
||||||
|
return
|
||||||
case "permission.asked":
|
case "permission.asked":
|
||||||
this.permission.handle(event)
|
this.permission.handle(event)
|
||||||
return
|
return
|
||||||
@@ -115,17 +143,49 @@ export class Subscription {
|
|||||||
|
|
||||||
private async run() {
|
private async run() {
|
||||||
while (!this.abort.signal.aborted) {
|
while (!this.abort.signal.aborted) {
|
||||||
|
await this.consume().catch(() => {})
|
||||||
|
this.disconnected()
|
||||||
|
if (!this.abort.signal.aborted) await new Promise((resolve) => setTimeout(resolve, 1000))
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
private async consume() {
|
||||||
const events = (await this.input.sdk.global.event({
|
const events = (await this.input.sdk.global.event({
|
||||||
signal: this.abort.signal,
|
signal: this.abort.signal,
|
||||||
})) as GlobalEventStream
|
})) as GlobalEventStream
|
||||||
|
this.connected = true
|
||||||
|
for (const resolve of this.connectionWaiters) resolve()
|
||||||
|
this.connectionWaiters.clear()
|
||||||
|
|
||||||
for await (const event of events.stream) {
|
for await (const event of events.stream) {
|
||||||
if (this.abort.signal.aborted) return
|
if (this.abort.signal.aborted) return
|
||||||
if (!event.payload) continue
|
if (!event.payload) continue
|
||||||
await this.handle(event.payload).catch(() => {})
|
await this.handle(event.payload).catch(() => {})
|
||||||
}
|
}
|
||||||
if (!this.abort.signal.aborted) await new Promise((resolve) => setTimeout(resolve, 1000))
|
|
||||||
}
|
}
|
||||||
|
|
||||||
|
private async waitUntilConnected() {
|
||||||
|
while (!this.connected) {
|
||||||
|
if (this.abort.signal.aborted) throw new Error("ACP event subscription stopped")
|
||||||
|
await new Promise<void>((resolve) => this.connectionWaiters.add(resolve))
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
private disconnected() {
|
||||||
|
if (!this.connected) return
|
||||||
|
this.connected = false
|
||||||
|
const error = new Error("ACP event stream disconnected")
|
||||||
|
for (const waiters of this.idleWaiters.values()) {
|
||||||
|
for (const waiter of waiters) waiter.reject(error)
|
||||||
|
}
|
||||||
|
this.idleWaiters.clear()
|
||||||
|
}
|
||||||
|
|
||||||
|
private idle(sessionId: string) {
|
||||||
|
const waiters = this.idleWaiters.get(sessionId)
|
||||||
|
if (!waiters) return
|
||||||
|
this.idleWaiters.delete(sessionId)
|
||||||
|
for (const waiter of waiters) waiter.resolve()
|
||||||
}
|
}
|
||||||
|
|
||||||
private async handlePartUpdated(event: EventMessagePartUpdated) {
|
private async handlePartUpdated(event: EventMessagePartUpdated) {
|
||||||
@@ -339,4 +399,23 @@ export class Subscription {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
function signal() {
|
||||||
|
const state: {
|
||||||
|
resolve: () => void
|
||||||
|
reject: (reason?: unknown) => void
|
||||||
|
} = {
|
||||||
|
resolve: () => {},
|
||||||
|
reject: () => {},
|
||||||
|
}
|
||||||
|
const promise = new Promise<void>((resolve, reject) => {
|
||||||
|
state.resolve = resolve
|
||||||
|
state.reject = reject
|
||||||
|
})
|
||||||
|
return {
|
||||||
|
promise,
|
||||||
|
resolve: () => state.resolve(),
|
||||||
|
reject: (reason?: unknown) => state.reject(reason),
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
export * as ACPEvent from "./event"
|
export * as ACPEvent from "./event"
|
||||||
|
|||||||
@@ -88,6 +88,8 @@ export function make(input: {
|
|||||||
? ACPEvent.start({ sdk: input.sdk, connection: input.connection, session })
|
? ACPEvent.start({ sdk: input.sdk, connection: input.connection, session })
|
||||||
: undefined
|
: undefined
|
||||||
if (events) input.eventSubscription?.(events)
|
if (events) input.eventSubscription?.(events)
|
||||||
|
const runUntilIdle = <A>(sessionId: string, fn: () => Promise<A>) =>
|
||||||
|
events ? events.runUntilIdle(sessionId, fn) : fn()
|
||||||
|
|
||||||
const initialize = Effect.fn("ACP.initialize")(function* (params: InitializeRequest) {
|
const initialize = Effect.fn("ACP.initialize")(function* (params: InitializeRequest) {
|
||||||
const started = performance.now()
|
const started = performance.now()
|
||||||
@@ -504,6 +506,7 @@ export function make(input: {
|
|||||||
if (!command) {
|
if (!command) {
|
||||||
const response = yield* request(
|
const response = yield* request(
|
||||||
() =>
|
() =>
|
||||||
|
runUntilIdle(current.id, () =>
|
||||||
input.sdk.session.prompt(
|
input.sdk.session.prompt(
|
||||||
{
|
{
|
||||||
sessionID: current.id,
|
sessionID: current.id,
|
||||||
@@ -518,6 +521,7 @@ export function make(input: {
|
|||||||
},
|
},
|
||||||
{ throwOnError: true },
|
{ throwOnError: true },
|
||||||
),
|
),
|
||||||
|
),
|
||||||
"session",
|
"session",
|
||||||
)
|
)
|
||||||
yield* sendUsageUpdate(input.usage, input.sdk, input.connection, current.id, current.cwd)
|
yield* sendUsageUpdate(input.usage, input.sdk, input.connection, current.id, current.cwd)
|
||||||
@@ -528,6 +532,7 @@ export function make(input: {
|
|||||||
if (known) {
|
if (known) {
|
||||||
const response = yield* request(
|
const response = yield* request(
|
||||||
() =>
|
() =>
|
||||||
|
runUntilIdle(current.id, () =>
|
||||||
input.sdk.session.command(
|
input.sdk.session.command(
|
||||||
{
|
{
|
||||||
sessionID: current.id,
|
sessionID: current.id,
|
||||||
@@ -540,6 +545,7 @@ export function make(input: {
|
|||||||
},
|
},
|
||||||
{ throwOnError: true },
|
{ throwOnError: true },
|
||||||
),
|
),
|
||||||
|
),
|
||||||
"session",
|
"session",
|
||||||
)
|
)
|
||||||
yield* sendUsageUpdate(input.usage, input.sdk, input.connection, current.id, current.cwd)
|
yield* sendUsageUpdate(input.usage, input.sdk, input.connection, current.id, current.cwd)
|
||||||
@@ -549,6 +555,7 @@ export function make(input: {
|
|||||||
if (command.name === "compact") {
|
if (command.name === "compact") {
|
||||||
yield* request(
|
yield* request(
|
||||||
() =>
|
() =>
|
||||||
|
runUntilIdle(current.id, () =>
|
||||||
input.sdk.session.summarize(
|
input.sdk.session.summarize(
|
||||||
{
|
{
|
||||||
sessionID: current.id,
|
sessionID: current.id,
|
||||||
@@ -558,6 +565,7 @@ export function make(input: {
|
|||||||
},
|
},
|
||||||
{ throwOnError: true },
|
{ throwOnError: true },
|
||||||
),
|
),
|
||||||
|
),
|
||||||
"session",
|
"session",
|
||||||
)
|
)
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -10,7 +10,7 @@ import type {
|
|||||||
SessionConfigSelectOption,
|
SessionConfigSelectOption,
|
||||||
SetSessionConfigOptionResponse,
|
SetSessionConfigOptionResponse,
|
||||||
} from "@agentclientprotocol/sdk"
|
} from "@agentclientprotocol/sdk"
|
||||||
import type { AssistantMessage, OpencodeClient } from "@opencode-ai/sdk/v2"
|
import type { AssistantMessage, Event, OpencodeClient } from "@opencode-ai/sdk/v2"
|
||||||
import { ProviderV2 } from "@opencode-ai/core/provider"
|
import { ProviderV2 } from "@opencode-ai/core/provider"
|
||||||
import { ModelV2 } from "@opencode-ai/core/model"
|
import { ModelV2 } from "@opencode-ai/core/model"
|
||||||
import { Effect } from "effect"
|
import { Effect } from "effect"
|
||||||
@@ -24,6 +24,54 @@ const modelID = ModelV2.ID.make("test-model")
|
|||||||
const configuredModelID = ModelV2.ID.make("configured-model")
|
const configuredModelID = ModelV2.ID.make("configured-model")
|
||||||
const secondModelID = ModelV2.ID.make("second-model")
|
const secondModelID = ModelV2.ID.make("second-model")
|
||||||
|
|
||||||
|
function createEventStream() {
|
||||||
|
const queue: Event[] = []
|
||||||
|
const waiters: Array<(event: Event | undefined) => void> = []
|
||||||
|
const push = (event: Event) => {
|
||||||
|
const waiter = waiters.shift()
|
||||||
|
if (waiter) return waiter(event)
|
||||||
|
queue.push(event)
|
||||||
|
}
|
||||||
|
const stream = async function* (signal?: AbortSignal) {
|
||||||
|
while (!signal?.aborted) {
|
||||||
|
const event = queue.shift()
|
||||||
|
if (event) {
|
||||||
|
yield { payload: event }
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
const next = await new Promise<Event | undefined>((resolve) => {
|
||||||
|
waiters.push(resolve)
|
||||||
|
signal?.addEventListener("abort", () => resolve(undefined), { once: true })
|
||||||
|
})
|
||||||
|
if (!next) return
|
||||||
|
yield { payload: next }
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return { push, stream }
|
||||||
|
}
|
||||||
|
|
||||||
|
function idleEvent(sessionID: string): Event {
|
||||||
|
return {
|
||||||
|
id: `evt_idle_${sessionID}`,
|
||||||
|
type: "session.status",
|
||||||
|
properties: {
|
||||||
|
sessionID,
|
||||||
|
status: { type: "idle" },
|
||||||
|
},
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
function deferred<A>() {
|
||||||
|
const state: { resolve?: (value: A) => void } = {}
|
||||||
|
const promise = new Promise<A>((resolve) => {
|
||||||
|
state.resolve = resolve
|
||||||
|
})
|
||||||
|
return {
|
||||||
|
promise,
|
||||||
|
resolve: (value: A) => state.resolve?.(value),
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
const provider: Provider.Info = {
|
const provider: Provider.Info = {
|
||||||
id: providerID,
|
id: providerID,
|
||||||
name: "Test",
|
name: "Test",
|
||||||
@@ -147,6 +195,7 @@ describe("ACP service sessions", () => {
|
|||||||
options?: {
|
options?: {
|
||||||
abort?: (input: { sessionID: string }) => Promise<{ data: boolean }>
|
abort?: (input: { sessionID: string }) => Promise<{ data: boolean }>
|
||||||
prompt?: (input: unknown) => Promise<{ data: { info: ReturnType<typeof assistantInfo> } }>
|
prompt?: (input: unknown) => Promise<{ data: { info: ReturnType<typeof assistantInfo> } }>
|
||||||
|
sessionUpdate?: (update: SessionNotification) => Promise<void>
|
||||||
},
|
},
|
||||||
) => {
|
) => {
|
||||||
const updates: SessionNotification[] = []
|
const updates: SessionNotification[] = []
|
||||||
@@ -157,6 +206,7 @@ describe("ACP service sessions", () => {
|
|||||||
const commands: unknown[] = []
|
const commands: unknown[] = []
|
||||||
const summarizes: unknown[] = []
|
const summarizes: unknown[] = []
|
||||||
const usageUpdates: string[] = []
|
const usageUpdates: string[] = []
|
||||||
|
const events = createEventStream()
|
||||||
const sessions = Array.from({ length: 102 }, (_, index) => ({
|
const sessions = Array.from({ length: 102 }, (_, index) => ({
|
||||||
id: `ses_${index + 1}`,
|
id: `ses_${index + 1}`,
|
||||||
directory: index % 2 === 0 ? "/workspace" : "/other",
|
directory: index % 2 === 0 ? "/workspace" : "/other",
|
||||||
@@ -164,6 +214,9 @@ describe("ACP service sessions", () => {
|
|||||||
time: { created: index + 1, updated: index + 1 },
|
time: { created: index + 1, updated: index + 1 },
|
||||||
}))
|
}))
|
||||||
const sdk = {
|
const sdk = {
|
||||||
|
global: {
|
||||||
|
event: (input?: { signal?: AbortSignal }) => Promise.resolve({ stream: events.stream(input?.signal) }),
|
||||||
|
},
|
||||||
config: {
|
config: {
|
||||||
providers: () => Promise.resolve({ data: { providers: [provider], default: { test: modelID } } }),
|
providers: () => Promise.resolve({ data: { providers: [provider], default: { test: modelID } } }),
|
||||||
get: () => Promise.resolve({ data: {} }),
|
get: () => Promise.resolve({ data: {} }),
|
||||||
@@ -196,11 +249,9 @@ describe("ACP service sessions", () => {
|
|||||||
data: input.directory ? sessions.filter((session) => session.directory === input.directory) : sessions,
|
data: input.directory ? sessions.filter((session) => session.directory === input.directory) : sessions,
|
||||||
}),
|
}),
|
||||||
messages: () => Promise.resolve({ data: messages }),
|
messages: () => Promise.resolve({ data: messages }),
|
||||||
prompt:
|
prompt: async (input: { sessionID: string }) => {
|
||||||
options?.prompt ??
|
const response = await (options?.prompt?.(input) ??
|
||||||
((input: unknown) => {
|
Promise.resolve({
|
||||||
prompts.push(input)
|
|
||||||
return Promise.resolve({
|
|
||||||
data: {
|
data: {
|
||||||
info: assistantInfo({
|
info: assistantInfo({
|
||||||
input: 100,
|
input: 100,
|
||||||
@@ -209,10 +260,14 @@ describe("ACP service sessions", () => {
|
|||||||
cache: { read: 11, write: 13 },
|
cache: { read: 11, write: 13 },
|
||||||
}),
|
}),
|
||||||
},
|
},
|
||||||
})
|
}))
|
||||||
}),
|
prompts.push(input)
|
||||||
command: (input: unknown) => {
|
events.push(idleEvent(input.sessionID))
|
||||||
|
return response
|
||||||
|
},
|
||||||
|
command: (input: { sessionID: string }) => {
|
||||||
commands.push(input)
|
commands.push(input)
|
||||||
|
events.push(idleEvent(input.sessionID))
|
||||||
return Promise.resolve({
|
return Promise.resolve({
|
||||||
data: {
|
data: {
|
||||||
info: assistantInfo({
|
info: assistantInfo({
|
||||||
@@ -224,8 +279,9 @@ describe("ACP service sessions", () => {
|
|||||||
},
|
},
|
||||||
})
|
})
|
||||||
},
|
},
|
||||||
summarize: (input: unknown) => {
|
summarize: (input: { sessionID: string }) => {
|
||||||
summarizes.push(input)
|
summarizes.push(input)
|
||||||
|
events.push(idleEvent(input.sessionID))
|
||||||
return Promise.resolve({ data: true })
|
return Promise.resolve({ data: true })
|
||||||
},
|
},
|
||||||
abort:
|
abort:
|
||||||
@@ -249,7 +305,7 @@ describe("ACP service sessions", () => {
|
|||||||
const connection = {
|
const connection = {
|
||||||
sessionUpdate: (update: SessionNotification) => {
|
sessionUpdate: (update: SessionNotification) => {
|
||||||
updates.push(update)
|
updates.push(update)
|
||||||
return Promise.resolve()
|
return options?.sessionUpdate?.(update) ?? Promise.resolve()
|
||||||
},
|
},
|
||||||
} as Pick<AgentSideConnection, "sessionUpdate">
|
} as Pick<AgentSideConnection, "sessionUpdate">
|
||||||
const usage = UsageService.Service.of({
|
const usage = UsageService.Service.of({
|
||||||
@@ -273,6 +329,7 @@ describe("ACP service sessions", () => {
|
|||||||
commands,
|
commands,
|
||||||
summarizes,
|
summarizes,
|
||||||
usageUpdates,
|
usageUpdates,
|
||||||
|
events,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -1018,6 +1075,75 @@ describe("ACP service sessions", () => {
|
|||||||
expect(usageUpdates).toEqual([session.sessionId])
|
expect(usageUpdates).toEqual([session.sessionId])
|
||||||
})
|
})
|
||||||
|
|
||||||
|
it("waits for queued session updates before returning end_turn", async () => {
|
||||||
|
const called = deferred<void>()
|
||||||
|
const response = deferred<{ data: { info: ReturnType<typeof assistantInfo> } }>()
|
||||||
|
const update = deferred<void>()
|
||||||
|
const release = deferred<void>()
|
||||||
|
const order: string[] = []
|
||||||
|
const fixture = makeService([], {
|
||||||
|
prompt: () => {
|
||||||
|
called.resolve(undefined)
|
||||||
|
return response.promise
|
||||||
|
},
|
||||||
|
sessionUpdate: (notification) => {
|
||||||
|
if (notification.update.sessionUpdate !== "agent_thought_chunk") return Promise.resolve()
|
||||||
|
update.resolve(undefined)
|
||||||
|
return release.promise.then(() => {
|
||||||
|
order.push("update")
|
||||||
|
})
|
||||||
|
},
|
||||||
|
})
|
||||||
|
const session = await Effect.runPromise(fixture.service.newSession({ cwd: "/workspace", mcpServers: [] }))
|
||||||
|
const result = Effect.runPromise(
|
||||||
|
fixture.service.prompt({ sessionId: session.sessionId, prompt: [{ type: "text", text: "hello" }] }),
|
||||||
|
).then((value) => {
|
||||||
|
order.push("response")
|
||||||
|
return value
|
||||||
|
})
|
||||||
|
|
||||||
|
await called.promise
|
||||||
|
fixture.events.push({
|
||||||
|
id: "evt_part",
|
||||||
|
type: "message.part.updated",
|
||||||
|
properties: {
|
||||||
|
sessionID: session.sessionId,
|
||||||
|
time: Date.now(),
|
||||||
|
part: {
|
||||||
|
id: "part_reasoning",
|
||||||
|
sessionID: session.sessionId,
|
||||||
|
messageID: "msg_assistant",
|
||||||
|
type: "reasoning",
|
||||||
|
text: "",
|
||||||
|
time: { start: Date.now() },
|
||||||
|
},
|
||||||
|
},
|
||||||
|
})
|
||||||
|
fixture.events.push({
|
||||||
|
id: "evt_delta",
|
||||||
|
type: "message.part.delta",
|
||||||
|
properties: {
|
||||||
|
sessionID: session.sessionId,
|
||||||
|
messageID: "msg_assistant",
|
||||||
|
partID: "part_reasoning",
|
||||||
|
field: "text",
|
||||||
|
delta: "thinking",
|
||||||
|
},
|
||||||
|
})
|
||||||
|
response.resolve({
|
||||||
|
data: {
|
||||||
|
info: assistantInfo({ input: 1, output: 1, reasoning: 1, cache: { read: 0, write: 0 } }),
|
||||||
|
},
|
||||||
|
})
|
||||||
|
|
||||||
|
await update.promise
|
||||||
|
expect(order).toEqual([])
|
||||||
|
|
||||||
|
release.resolve(undefined)
|
||||||
|
expect((await result).stopReason).toBe("end_turn")
|
||||||
|
expect(order).toEqual(["update", "response"])
|
||||||
|
})
|
||||||
|
|
||||||
it("maps assistant prompt errors to request errors instead of end turn", async () => {
|
it("maps assistant prompt errors to request errors instead of end turn", async () => {
|
||||||
const { service } = makeService([], {
|
const { service } = makeService([], {
|
||||||
prompt: () =>
|
prompt: () =>
|
||||||
|
|||||||
Reference in New Issue
Block a user