diff --git a/src/features/chat/lib/sessionTargetTransition.integration.test.ts b/src/features/chat/lib/sessionTargetTransition.integration.test.ts index ab5799a71..894732a0b 100644 --- a/src/features/chat/lib/sessionTargetTransition.integration.test.ts +++ b/src/features/chat/lib/sessionTargetTransition.integration.test.ts @@ -15,6 +15,10 @@ vi.mock("@/shared/api/acpApi", () => ({ vi.mock("@/shared/api/acpConnection", () => ({ getClient: () => mockGetClient(), getBackendClient: () => mockGetClient(), + captureBackendConnectionGeneration: () => ({ + isCurrent: () => true, + invalidate: async () => {}, + }), })); const managedRuntimeConfig: RuntimeConfig = { @@ -118,12 +122,16 @@ describe("transitionSessionTarget with managed Goose models", () => { expect(mockSetProvider).toHaveBeenCalledWith( "legacy-session", "databricks_v2", - { requestId: undefined }, + { requestId: undefined, assertActive: expect.any(Function) }, ); expect(mockSetModel).toHaveBeenCalledWith( "legacy-session", "goose-gpt-5-5", - { providerId: "databricks_v2", requestId: undefined }, + { + providerId: "databricks_v2", + requestId: undefined, + assertActive: expect.any(Function), + }, ); expect(mockSetModel).not.toHaveBeenCalledWith("legacy-session", "goose"); }); @@ -297,13 +305,17 @@ describe("transitionSessionTarget with managed Goose models", () => { expect(mockSetProvider).toHaveBeenCalledWith( "managed-opus-session", "databricks_v2", - { requestId: undefined }, + { requestId: undefined, assertActive: expect.any(Function) }, ); expect(mockSetModel).toHaveBeenCalledOnce(); expect(mockSetModel).toHaveBeenCalledWith( "managed-opus-session", "goose-claude-opus-4-8", - { providerId: "databricks_v2", requestId: undefined }, + { + providerId: "databricks_v2", + requestId: undefined, + assertActive: expect.any(Function), + }, ); }); }); diff --git a/src/features/security/acp/securityPermissionHandler.test.ts b/src/features/security/acp/securityPermissionHandler.test.ts index c6ac9dc4d..0cdb62773 100644 --- a/src/features/security/acp/securityPermissionHandler.test.ts +++ b/src/features/security/acp/securityPermissionHandler.test.ts @@ -117,4 +117,83 @@ describe("security permission explanation fallback", () => { useSecurityConfirmationStore.getState().cancel("external-agent-session"); }); + + it("does not enqueue a request from an already retired generation", async () => { + const lifetime = new AbortController(); + lifetime.abort(); + await expect( + handleSecurityPermissionRequest(securityRequest(), lifetime.signal), + ).resolves.toEqual({ outcome: { outcome: "cancelled" } }); + expect(useSecurityConfirmationStore.getState().pendingBySessionId).toEqual( + {}, + ); + expect(readinessMocks.readDefaultProviderReadiness).not.toHaveBeenCalled(); + }); + + it("retires only the detached generation's requests, including queued ones", async () => { + inferenceMocks.alertLacksExplanation + .mockReturnValueOnce(false) + .mockReturnValueOnce(false) + .mockReturnValueOnce(false); + const lifetime = new AbortController(); + const first = handleSecurityPermissionRequest( + securityRequest(), + lifetime.signal, + ); + const second = handleSecurityPermissionRequest( + securityRequest(), + lifetime.signal, + ); + const replacementRequest = securityRequest(); + const replacement = handleSecurityPermissionRequest( + replacementRequest, + new AbortController().signal, + ); + + lifetime.abort(); + await expect(first).resolves.toEqual({ outcome: { outcome: "cancelled" } }); + await expect(second).resolves.toEqual({ + outcome: { outcome: "cancelled" }, + }); + const store = useSecurityConfirmationStore.getState(); + expect( + store.pendingBySessionId["external-agent-session"]?.map( + (pending) => pending.request, + ), + ).toEqual([replacementRequest]); + store.resolveWith( + "external-agent-session", + replacementRequest, + "allow-once", + ); + await expect(replacement).resolves.toEqual({ + outcome: { outcome: "selected", optionId: "allow-once" }, + }); + }); + + it("does not start delayed inference for a retired confirmation", async () => { + let resolveReadiness!: (value: unknown) => void; + readinessMocks.readDefaultProviderReadiness.mockReturnValueOnce( + new Promise((resolve) => { + resolveReadiness = resolve; + }), + ); + const lifetime = new AbortController(); + const pending = handleSecurityPermissionRequest( + securityRequest(), + lifetime.signal, + ); + lifetime.abort(); + resolveReadiness({ + status: "ready", + providerId: "anthropic", + modelId: "claude-sonnet", + }); + await pending; + await Promise.resolve(); + expect(inferenceMocks.inferSecurityExplanation).not.toHaveBeenCalled(); + expect(useSecurityConfirmationStore.getState().pendingBySessionId).toEqual( + {}, + ); + }); }); diff --git a/src/features/security/acp/securityPermissionHandler.ts b/src/features/security/acp/securityPermissionHandler.ts index b6b56ac89..d8bbf19a3 100644 --- a/src/features/security/acp/securityPermissionHandler.ts +++ b/src/features/security/acp/securityPermissionHandler.ts @@ -80,7 +80,11 @@ function isSecurityRequest(alertText: string): boolean { */ export function handleSecurityPermissionRequest( request: RequestPermissionRequest, + signal?: AbortSignal, ): Promise { + if (signal?.aborted) { + return Promise.resolve({ outcome: { outcome: "cancelled" } }); + } const alertText = collectContentText(request.toolCall.content); if (!isSecurityRequest(alertText)) { @@ -95,17 +99,25 @@ export function handleSecurityPermissionRequest( // decision while inference runs in the background; other sessions remain // interactive. const promise = new Promise((resolve) => { + const cancel = () => + useSecurityConfirmationStore.getState().cancelRequest(request); + signal?.addEventListener("abort", cancel, { once: true }); useSecurityConfirmationStore.getState().enqueue({ request, title: request.toolCall.title ?? "Tool call", command, alertText, - resolve, + resolve: (response) => { + signal?.removeEventListener("abort", cancel); + resolve(response); + }, }); + // Store subscribers may synchronously detach the requesting transport. + if (signal?.aborted) cancel(); }); // If the alert only has confidence (no explanation), infer one - if (command && alertLacksExplanation(alertText)) { + if (!signal?.aborted && command && alertLacksExplanation(alertText)) { const store = useSecurityConfirmationStore.getState(); store.setInferredExplanation(request.sessionId, request, { status: "loading", @@ -114,6 +126,7 @@ export function handleSecurityPermissionRequest( const confidence = extractConfidence(alertText); readDefaultProviderReadiness() .then(async (readiness) => { + if (signal?.aborted) return { status: "failed" as const }; if (readiness.status === "needs_setup") { return { status: "needs_setup" as const }; } diff --git a/src/features/security/stores/securityConfirmationStore.ts b/src/features/security/stores/securityConfirmationStore.ts index d7cce64b0..cb3ad63e6 100644 --- a/src/features/security/stores/securityConfirmationStore.ts +++ b/src/features/security/stores/securityConfirmationStore.ts @@ -39,6 +39,7 @@ interface SecurityConfirmationState { optionId: string, ) => void; cancel: (sessionId: string) => void; + cancelRequest: (request: RequestPermissionRequest) => void; cancelAll: (sessionId: string) => void; blockAll: (sessionId: string) => void; mountSurface: (sessionId: string) => void; @@ -121,6 +122,20 @@ export const useSecurityConfirmationStore = create( pending.resolve({ outcome: { outcome: "cancelled" } }); }, + cancelRequest: (request) => { + const sessionId = request.sessionId; + const queue = get().pendingBySessionId[sessionId]; + const pending = queue?.find((entry) => entry.request === request); + if (!pending) return; + const remaining = queue?.filter((entry) => entry !== pending) ?? []; + set((state) => ({ + pendingBySessionId: remaining.length + ? { ...state.pendingBySessionId, [sessionId]: remaining } + : removeSessionPending(state.pendingBySessionId, sessionId), + })); + pending.resolve({ outcome: { outcome: "cancelled" } }); + }, + cancelAll: (sessionId) => { const pending = get().pendingBySessionId[sessionId] ?? []; if (pending.length === 0) { diff --git a/src/shared/api/__tests__/acp.test.ts b/src/shared/api/__tests__/acp.test.ts index 9e36ddf81..9b645d0d1 100644 --- a/src/shared/api/__tests__/acp.test.ts +++ b/src/shared/api/__tests__/acp.test.ts @@ -20,10 +20,14 @@ const mockAppendSessionSystemPrompt = vi.fn(); const mockForkSession = vi.fn(); const mockRenameSession = vi.fn(); const mockArchiveSession = vi.fn(); -const noRequestProviderContext = { requestId: undefined }; +const noRequestProviderContext = { + requestId: undefined, + assertActive: expect.any(Function), +}; const noRequestModelContext = (providerId: string) => ({ providerId, requestId: undefined, + assertActive: expect.any(Function), }); const managedRuntimeConfig: RuntimeConfig = { @@ -955,6 +959,7 @@ describe("acpCreateSession", () => { expect(mockLoadSession).toHaveBeenCalledWith( "acp-session-1", "/tmp/project", + expect.any(Function), ); expect(mockSetProvider).toHaveBeenCalledWith( "acp-session-1", @@ -1223,6 +1228,7 @@ describe("acpPrepareSession", () => { expect(mockLoadSession).toHaveBeenCalledWith( "acp-session-1", "/tmp/project", + expect.any(Function), ); expect(mockNewSession).not.toHaveBeenCalled(); expect(mockSetProvider).toHaveBeenCalledWith( diff --git a/src/shared/api/__tests__/acpBackends.test.ts b/src/shared/api/__tests__/acpBackends.test.ts index d479ebb5d..c08de52a5 100644 --- a/src/shared/api/__tests__/acpBackends.test.ts +++ b/src/shared/api/__tests__/acpBackends.test.ts @@ -1,5 +1,10 @@ -import { beforeEach, describe, expect, it, vi } from "vitest"; -import type { Client, SessionNotification } from "@agentclientprotocol/sdk"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import type { + Client, + RequestPermissionRequest, + RequestPermissionResponse, + SessionNotification, +} from "@agentclientprotocol/sdk"; import { LOCAL_BACKEND_ID, backendIdForSession, @@ -21,6 +26,11 @@ const mocks = vi.hoisted(() => ({ connectRemoteHost: vi.fn(), disconnectRemoteHost: vi.fn(), createWebSocketStream: vi.fn(), + initialize: vi.fn(), + sessionInfo: vi.fn(), + loadSession: vi.fn(), + setSessionConfigOption: vi.fn(), + updateWorkingDir: vi.fn(), clientCallbackFactories: [] as Array<() => Client>, })); @@ -46,7 +56,13 @@ vi.mock("@agentclientprotocol/sdk", () => ({ vi.mock("@aaif/goose-sdk", () => ({ DEFAULT_GOOSE_MCP_HOST_CAPABILITIES: {}, GooseClient: class { - initialize = vi.fn(async () => {}); + initialize = vi.fn((...args: unknown[]) => mocks.initialize(...args)); + goose = { + GooseUnstableSessionInfo: mocks.sessionInfo, + GooseUnstableSessionWorkingDirUpdate: mocks.updateWorkingDir, + }; + loadSession = mocks.loadSession; + setSessionConfigOption = mocks.setSessionConfigOption; closed: Promise; resolveClosed!: () => void; constructor(callbacks: () => Client, _stream: unknown) { @@ -82,6 +98,13 @@ beforeEach(() => { }), ); mocks.clientCallbackFactories.length = 0; + mocks.initialize.mockReset().mockResolvedValue(undefined); + mocks.sessionInfo.mockReset(); + mocks.loadSession.mockReset().mockResolvedValue({ configOptions: [] }); + mocks.setSessionConfigOption + .mockReset() + .mockResolvedValue({ configOptions: [] }); + mocks.updateWorkingDir.mockReset().mockResolvedValue(undefined); mocks.invoke.mockResolvedValue("ws://local"); mocks.connectRemoteHost.mockResolvedValue({ wsUrl: "ws://remote", @@ -96,6 +119,10 @@ beforeEach(() => { })); }); +afterEach(() => { + vi.useRealTimers(); +}); + describe("acpBackendId", () => { it("builds ssh backend ids from trimmed hosts", () => { expect(sshBackendId(" dev-box ")).toBe("ssh:dev-box"); @@ -324,6 +351,274 @@ describe("backend connection registry", () => { expect(mocks.connectRemoteHost).toHaveBeenCalledTimes(2); }); + it.each([ + "local", + "ssh:dev-box", + ] as const)("detaches %s synchronously and preserves its replacement during late cleanup", async (backendId) => { + vi.spyOn(console, "warn").mockImplementation(() => {}); + const conn = await importConnection(); + const connection = conn.getBackendConnection(backendId); + let resolveAbort!: () => void; + let resolveDisconnect!: () => void; + const oldStream = { + writable: { + abort: vi.fn().mockReturnValue( + new Promise((resolve) => { + resolveAbort = resolve; + }), + ), + }, + }; + mocks.createWebSocketStream.mockReturnValueOnce(oldStream); + const first = (await connection.getClient()) as unknown as FakeClient; + const onClosed = vi.fn(); + connection.onClosed(onClosed); + if (backendId !== "local") { + mocks.disconnectRemoteHost.mockReturnValueOnce( + new Promise((resolve) => { + resolveDisconnect = resolve; + }), + ); + mocks.connectRemoteHost.mockResolvedValueOnce({ + wsUrl: "ws://replacement", + generation: 2, + }); + } + + let cleanupSettled = false; + const cleanup = conn.invalidateBackendConnection(backendId).then(() => { + cleanupSettled = true; + }); + // These assertions deliberately precede any await: timeout callers + // depend on detachment happening before invalidation returns. + expect(connection.getClientSync()).toBeNull(); + expect(connection.isReady()).toBe(false); + expect(oldStream.writable.abort).toHaveBeenCalledOnce(); + if (backendId !== "local") { + expect(mocks.disconnectRemoteHost).toHaveBeenCalledExactlyOnceWith( + "dev-box", + 1, + ); + } + + const second = await conn.getBackendClient(backendId); + const newStream = mocks.createWebSocketStream.mock.results.at(-1) + ?.value as { writable: { abort: ReturnType } }; + expect(second).not.toBe(first); + expect(cleanupSettled).toBe(false); + expect(connection.getClientSync()).toBe(second); + + first.resolveClosed(); + await flushClosedMonitor(); + expect(onClosed).not.toHaveBeenCalled(); + expect(connection.getClientSync()).toBe(second); + + resolveAbort(); + resolveDisconnect?.(); + await cleanup; + expect(await connection.getClient()).toBe(second); + expect(connection.isReady()).toBe(true); + expect(newStream.writable.abort).not.toHaveBeenCalled(); + expect(oldStream.writable.abort).toHaveBeenCalledOnce(); + if (backendId !== "local") { + expect(mocks.connectRemoteHost).toHaveBeenCalledTimes(2); + expect(mocks.createWebSocketStream).toHaveBeenLastCalledWith( + "ws://replacement", + ); + expect(mocks.disconnectRemoteHost).toHaveBeenCalledExactlyOnceWith( + "dev-box", + 1, + ); + } else { + expect(mocks.invoke).toHaveBeenCalledTimes(2); + expect(mocks.disconnectRemoteHost).not.toHaveBeenCalled(); + } + }); + + it.each([ + "local", + "ssh:dev-box", + ] as const)("does not let a second old-session timeout invalidate the %s replacement", async (backendId) => { + vi.useFakeTimers(); + const conn = await importConnection(); + const { loadSession } = await import("../acpSessionRegistry"); + const firstId = compositeSessionId(backendId, "session-1"); + const secondId = compositeSessionId(backendId, "session-2"); + mocks.sessionInfo.mockReturnValue(new Promise(() => {})); + + const first = loadSession(firstId); + const firstRejection = expect(first).rejects.toThrow( + "ACP operation timed out", + ); + await vi.advanceTimersByTimeAsync(10_000); + const second = loadSession(secondId); + const secondRejection = expect(second).rejects.toThrow( + "ACP operation timed out", + ); + await vi.advanceTimersByTimeAsync(0); + expect(mocks.sessionInfo).toHaveBeenCalledTimes(2); + expect(mocks.createWebSocketStream).toHaveBeenCalledOnce(); + const oldStream = mocks.createWebSocketStream.mock.results[0].value; + + await vi.advanceTimersByTimeAsync(50_000); + await firstRejection; + expect(oldStream.writable.abort).toHaveBeenCalledOnce(); + await loadSession(firstId, "/new/project"); + const connection = conn.getBackendConnection(backendId); + const replacement = connection.getClientSync(); + const newStream = mocks.createWebSocketStream.mock.results[1].value; + expect(replacement).not.toBeNull(); + + await vi.advanceTimersByTimeAsync(10_000); + await secondRejection; + expect(newStream.writable.abort).not.toHaveBeenCalled(); + expect(connection.getClientSync()).toBe(replacement); + expect(await connection.getClient()).toBe(replacement); + expect(mocks.createWebSocketStream).toHaveBeenCalledTimes(2); + }); + + it.each([ + "close", + "initialize failure", + ])("retires the captured generation after %s without invalidating its replacement", async (failure) => { + vi.spyOn(console, "warn").mockImplementation(() => {}); + const conn = await importConnection(); + const connection = conn.getBackendConnection("local"); + const generation = connection.captureGeneration(); + if (failure === "close") { + const first = (await connection.getClient()) as unknown as FakeClient; + first.resolveClosed(); + await flushClosedMonitor(); + } else { + mocks.initialize.mockRejectedValueOnce(new Error("failed initialize")); + await expect(connection.getClient()).rejects.toThrow("failed initialize"); + } + expect(generation.isCurrent()).toBe(false); + const replacement = await connection.getClient(); + const stream = mocks.createWebSocketStream.mock.results.at(-1)?.value; + + await generation.invalidate(); + + expect(connection.getClientSync()).toBe(replacement); + expect(stream.writable.abort).not.toHaveBeenCalled(); + }); + + it.each([ + "cold provider", + "cold load", + "model", + "configuration", + "working directory", + ])("does not let a late %s mutation overwrite or publish newer session state", async (operation) => { + vi.useFakeTimers(); + const registry = await import("../acpSessionRegistry"); + const { setSessionConfigSnapshotHandlers } = await import( + "../acpSessionConfigSnapshots" + ); + const configResponse = ( + providerId: string, + modelId: string, + effort: string, + ) => ({ + configOptions: [ + { + id: "provider", + kind: { type: "select", currentValue: providerId, options: [] }, + }, + { + id: "model", + category: "model", + kind: { type: "select", currentValue: modelId, options: [] }, + }, + { + id: "thinking_effort", + category: "thought_level", + kind: { type: "select", currentValue: effort, options: [] }, + }, + ], + }); + const published = vi.fn(); + setSessionConfigSnapshotHandlers({ applyConfigSnapshots: published }); + mocks.setSessionConfigOption.mockResolvedValue( + configResponse("openai", "new-model", "high"), + ); + let resolveOld!: (value: unknown) => void; + const oldResponse = new Promise((resolve) => { + resolveOld = resolve; + }); + let old: Promise; + if (operation.startsWith("cold")) { + if (operation === "cold load") { + mocks.loadSession.mockReturnValueOnce(oldResponse); + } else { + mocks.setSessionConfigOption.mockReturnValueOnce(oldResponse); + } + old = registry.configureSession( + "session-1", + "anthropic", + "/stale/project", + "stale-model", + ); + } else { + registry.registerPreparedSession( + "session-1", + "openai", + "/initial/project", + "initial-model", + ); + if (operation === "working directory") { + mocks.updateWorkingDir.mockReturnValueOnce(oldResponse); + old = registry.prepareSession( + "session-1", + "anthropic", + "/stale/project", + ); + } else { + mocks.setSessionConfigOption.mockReturnValueOnce(oldResponse); + old = + operation === "model" + ? registry.applySessionModel("session-1", "stale-model") + : registry.applySessionConfigOption( + "session-1", + "thinking_effort", + "low", + ); + } + } + const rejection = expect(old).rejects.toThrow("ACP operation timed out"); + await vi.advanceTimersByTimeAsync(60_000); + await rejection; + await registry.configureSession( + "session-1", + "openai", + "/new/project", + "new-model", + ); + const newPublications = [...published.mock.calls]; + const newRequests = [...mocks.setSessionConfigOption.mock.calls]; + expect(newPublications.length).toBeGreaterThan(0); + expect(registry.requireSessionInvocationSelection("session-1")).toEqual({ + providerId: "openai", + modelId: "new-model", + }); + + resolveOld(configResponse("anthropic", "stale-model", "low")); + await vi.advanceTimersByTimeAsync(0); + + expect(registry.requireSessionInvocationSelection("session-1")).toEqual({ + providerId: "openai", + modelId: "new-model", + }); + expect(published.mock.calls).toEqual(newPublications); + expect(mocks.setSessionConfigOption.mock.calls).toEqual(newRequests); + await registry.loadSession("session-1"); + expect(mocks.loadSession).toHaveBeenLastCalledWith({ + sessionId: "session-1", + cwd: "/new/project", + mcpServers: [], + }); + }); + it("delegates invalidateClientConnection to the local backend only", async () => { const conn = await importConnection(); const localClient = await conn.getClient(); @@ -456,6 +751,16 @@ describe("session backend routing", () => { }); describe("inbound session id translation", () => { + const permissionRequest = { + sessionId: "session-1", + toolCall: { toolCallId: "tool-1", title: "Test tool" }, + options: [{ optionId: "approve", kind: "allow_once", name: "Allow" }], + } satisfies RequestPermissionRequest; + const approval: RequestPermissionResponse = { + outcome: { outcome: "selected", optionId: "approve" }, + }; + const cancellation = { outcome: { outcome: "cancelled" } }; + function sessionUpdatePayload(sessionId: string): SessionNotification { return { sessionId, @@ -474,6 +779,151 @@ describe("inbound session id translation", () => { return factory(); } + it.each([ + "local", + "ssh:dev-box", + ] as const)("does not auto-approve or surface permissions from a detached %s transport", async (backendId) => { + const conn = await importConnection(); + const connection = conn.getBackendConnection(backendId); + await connection.getClient(); + const oldCallbacks = await latestCallbacks(); + await connection.invalidate(); + await connection.getClient(); + const replacementCallbacks = await latestCallbacks(); + + await expect( + oldCallbacks.requestPermission(permissionRequest), + ).resolves.toEqual(cancellation); + await expect( + replacementCallbacks.requestPermission(permissionRequest), + ).resolves.toEqual(approval); + const handler = vi.fn().mockResolvedValue(approval); + conn.setPermissionHandler(handler); + await expect( + oldCallbacks.requestPermission(permissionRequest), + ).resolves.toEqual(cancellation); + expect(handler).not.toHaveBeenCalled(); + await expect( + replacementCallbacks.requestPermission(permissionRequest), + ).resolves.toEqual(approval); + expect(handler).toHaveBeenCalledOnce(); + }); + + it.each([ + { backendId: "local", close: false }, + { backendId: "ssh:dev-box", close: false }, + { backendId: "local", close: true }, + { backendId: "ssh:dev-box", close: true }, + ] as const)("cancels pending $backendId permissions on detachment (close: $close)", async ({ + backendId, + close, + }) => { + const conn = await importConnection(); + const connection = conn.getBackendConnection(backendId); + const first = (await connection.getClient()) as unknown as FakeClient; + const oldCallbacks = await latestCallbacks(); + let resolveDecision!: (response: RequestPermissionResponse) => void; + let lifetime!: AbortSignal; + conn.setPermissionHandler((_request, signal) => { + lifetime = signal!; + return new Promise((resolve) => { + resolveDecision = resolve; + }); + }); + const pending = oldCallbacks.requestPermission(permissionRequest); + if (close) { + first.resolveClosed(); + await flushClosedMonitor(); + } else { + const stream = mocks.createWebSocketStream.mock.results.at(-1)?.value; + stream.writable.abort.mockReturnValue(new Promise(() => {})); + void connection.invalidate(); + } + expect(lifetime.aborted).toBe(true); + // Cancellation must not wait for transport cleanup or a user's decision. + await expect(pending).resolves.toEqual(cancellation); + await connection.getClient(); + resolveDecision(approval); + await expect(pending).resolves.toEqual(cancellation); + conn.setPermissionHandler(async () => approval); + const replacementCallbacks = await latestCallbacks(); + await expect( + replacementCallbacks.requestPermission(permissionRequest), + ).resolves.toEqual(approval); + }); + + it("does not return approval when detachment races a resolved decision", async () => { + const conn = await importConnection(); + const connection = conn.getBackendConnection("local"); + await connection.getClient(); + const callbacks = await latestCallbacks(); + conn.setPermissionHandler(async () => { + void connection.invalidate(); + return approval; + }); + await expect( + callbacks.requestPermission(permissionRequest), + ).resolves.toEqual(cancellation); + }); + + it("retires permissions when initialization fails even if cleanup never settles", async () => { + const conn = await importConnection(); + const connection = conn.getBackendConnection("local"); + let rejectInitialization!: (error: Error) => void; + mocks.initialize.mockReturnValueOnce( + new Promise((_, reject) => { + rejectInitialization = reject; + }), + ); + const initialization = connection.getClient(); + void initialization.catch(() => {}); + await vi.waitFor(() => expect(mocks.initialize).toHaveBeenCalledOnce()); + const callbacks = await latestCallbacks(); + conn.setPermissionHandler(() => new Promise(() => {})); + const pending = callbacks.requestPermission(permissionRequest); + const stream = mocks.createWebSocketStream.mock.results.at(-1)?.value; + stream.writable.abort.mockReturnValue(new Promise(() => {})); + rejectInitialization(new Error("handshake failed")); + await expect(pending).resolves.toEqual(cancellation); + const replacement = await connection.getClient(); + expect(connection.getClientSync()).toBe(replacement); + await expect( + callbacks.requestPermission(permissionRequest), + ).resolves.toEqual(cancellation); + }); + + it.each([ + LOCAL_BACKEND_ID, + sshBackendId("dev-box"), + ])("ignores notifications from a detached %s generation even while cleanup hangs", async (backendId) => { + const conn = await importConnection(); + const received = vi.fn(); + const intercepted = vi.fn(() => false); + conn.setNotificationHandler({ handleSessionNotification: received }); + const stop = conn.interceptSessionNotifications(intercepted); + const connection = conn.getBackendConnection(backendId); + await connection.getClient(); + const oldCallbacks = await latestCallbacks(); + const oldStream = mocks.createWebSocketStream.mock.results.at(-1)?.value; + oldStream.writable.abort.mockReturnValue(new Promise(() => {})); + + void connection.captureGeneration().invalidate(); + await connection.getClient(); + const replacementCallbacks = await latestCallbacks(); + const payload = sessionUpdatePayload("session-1"); + await oldCallbacks.sessionUpdate?.(payload); + expect(received).not.toHaveBeenCalled(); + expect(intercepted).not.toHaveBeenCalled(); + + await replacementCallbacks.sessionUpdate?.(payload); + expect(received).toHaveBeenCalledOnce(); + expect(intercepted).toHaveBeenCalledOnce(); + expect(received.mock.calls[0]?.[0].sessionId).toBe( + compositeSessionId(backendId, "session-1"), + ); + stop(); + }); + it("rewrites remote notification session ids to composite ids", async () => { const conn = await importConnection(); const received: SessionNotification[] = []; diff --git a/src/shared/api/__tests__/acpLoadSession.cwd.test.ts b/src/shared/api/__tests__/acpLoadSession.cwd.test.ts new file mode 100644 index 000000000..29ff12d10 --- /dev/null +++ b/src/shared/api/__tests__/acpLoadSession.cwd.test.ts @@ -0,0 +1,537 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +const mocks = vi.hoisted(() => ({ + getBackendClient: vi.fn(), + sessionInfo: vi.fn(), + loadSession: vi.fn(), + invalidateBackendConnection: vi.fn(), + generations: new Map(), +})); + +vi.mock("../acpConnection", () => ({ + getClient: mocks.getBackendClient, + getBackendClient: mocks.getBackendClient, + invalidateBackendConnection: mocks.invalidateBackendConnection, + captureBackendConnectionGeneration: (backendId: string) => { + const generation = mocks.generations.get(backendId) ?? 0; + return { + isCurrent: () => (mocks.generations.get(backendId) ?? 0) === generation, + invalidate: () => mocks.invalidateBackendConnection(backendId), + }; + }, + interceptSessionNotifications: vi.fn(), +})); + +// Keep acpLoadSession, the mutation registry, and acpApi real. A mock at the +// acpLoadSession boundary would hide the literal "~" sent to the backend. +describe("acpLoadSession working directory at the transport boundary", () => { + beforeEach(() => { + vi.resetModules(); + vi.resetAllMocks(); + mocks.generations.clear(); + mocks.invalidateBackendConnection.mockImplementation( + async (backendId: string) => { + mocks.generations.set( + backendId, + (mocks.generations.get(backendId) ?? 0) + 1, + ); + }, + ); + mocks.getBackendClient.mockResolvedValue({ + goose: { GooseUnstableSessionInfo: mocks.sessionInfo }, + loadSession: mocks.loadSession, + }); + mocks.sessionInfo.mockResolvedValue({ + session: { sessionId: "session-1", cwd: "/saved/project" }, + }); + mocks.loadSession.mockImplementation(async ({ cwd }: { cwd: string }) => { + if (!/^(?:\/|[A-Za-z]:[\\/]|\\\\)/.test(cwd)) { + throw new Error("cwd must be an absolute path"); + } + return { configOptions: [] }; + }); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it("recovers the saved directory when the renderer has no workspace path", async () => { + const { acpLoadSession } = await import("../acp"); + + await acpLoadSession("session-1"); + + expect(mocks.sessionInfo).toHaveBeenCalledWith({ + sessionId: "session-1", + }); + expect(mocks.loadSession).toHaveBeenCalledWith({ + sessionId: "session-1", + cwd: "/saved/project", + mcpServers: [], + }); + }); + + it("reuses the prepared directory for a post-compaction reload", async () => { + const registry = await import("../acpSessionRegistry"); + const { acpLoadSession } = await import("../acp"); + registry.registerPreparedSession( + "session-1", + "openai", + "/prepared/project", + "test-model", + ); + + await acpLoadSession("session-1"); + + expect(mocks.sessionInfo).not.toHaveBeenCalled(); + expect(mocks.loadSession).toHaveBeenCalledWith({ + sessionId: "session-1", + cwd: "/prepared/project", + mcpServers: [], + }); + }); + + it.each([ + "/chosen/project", + "/chosen/project with trailing space ", + "C:\\Users\\dev\\project", + "\\\\server\\share\\project", + ])("preserves the explicit directory %s", async (workingDir) => { + const registry = await import("../acpSessionRegistry"); + const { acpLoadSession } = await import("../acp"); + registry.registerPreparedSession("session-1", "openai", "/old/project"); + + await acpLoadSession("session-1", workingDir); + + expect(mocks.sessionInfo).not.toHaveBeenCalled(); + expect(mocks.loadSession).toHaveBeenCalledWith({ + sessionId: "session-1", + cwd: workingDir, + mcpServers: [], + }); + }); + + it("recovers a remote directory from its owning backend, not the local home", async () => { + const { acpLoadSession } = await import("../acp"); + mocks.sessionInfo.mockResolvedValue({ + session: { sessionId: "session-1", cwd: "/remote/project" }, + }); + + await acpLoadSession("ssh:devbox#session-1"); + + expect(mocks.getBackendClient).toHaveBeenCalledTimes(2); + expect(mocks.getBackendClient).toHaveBeenNthCalledWith(1, "ssh:devbox"); + expect(mocks.getBackendClient).toHaveBeenNthCalledWith(2, "ssh:devbox"); + expect(mocks.sessionInfo).toHaveBeenCalledWith({ sessionId: "session-1" }); + expect(mocks.loadSession).toHaveBeenCalledWith({ + sessionId: "session-1", + cwd: "/remote/project", + mcpServers: [], + }); + }); + + it.each([ + "", + " \t\n", + ])("recovers a remote directory when the renderer path is blank (%j)", async (workingDir) => { + const { acpLoadSession } = await import("../acp"); + mocks.sessionInfo.mockResolvedValue({ + session: { sessionId: "session-1", cwd: "/remote/project" }, + }); + + await acpLoadSession("ssh:devbox#session-1", workingDir); + + expect(mocks.getBackendClient).toHaveBeenCalledTimes(2); + expect(mocks.getBackendClient).toHaveBeenNthCalledWith(1, "ssh:devbox"); + expect(mocks.getBackendClient).toHaveBeenNthCalledWith(2, "ssh:devbox"); + expect(mocks.sessionInfo).toHaveBeenCalledWith({ sessionId: "session-1" }); + expect(mocks.loadSession).toHaveBeenCalledWith({ + sessionId: "session-1", + cwd: "/remote/project", + mcpServers: [], + }); + }); + + it("reuses a prepared directory when the renderer path is blank", async () => { + const registry = await import("../acpSessionRegistry"); + const { acpLoadSession } = await import("../acp"); + registry.registerPreparedSession( + "session-1", + "openai", + "/prepared/project", + ); + + await acpLoadSession("session-1", ""); + + expect(mocks.sessionInfo).not.toHaveBeenCalled(); + expect(mocks.loadSession).toHaveBeenCalledWith({ + sessionId: "session-1", + cwd: "/prepared/project", + mcpServers: [], + }); + }); + + it.each([ + "", + " \t\n", + ])("recovers the saved directory when the prepared path is blank (%j)", async (workingDir) => { + const registry = await import("../acpSessionRegistry"); + const { acpLoadSession } = await import("../acp"); + registry.registerPreparedSession("session-1", "openai", workingDir); + + await acpLoadSession("session-1"); + + expect(mocks.sessionInfo).toHaveBeenCalledWith({ sessionId: "session-1" }); + expect(mocks.loadSession).toHaveBeenCalledWith({ + sessionId: "session-1", + cwd: "/saved/project", + mcpServers: [], + }); + }); + + it.each([ + null, + "", + " \t\n", + ])("does not invent a directory when backend metadata returns %s", async (cwd) => { + const { acpLoadSession } = await import("../acp"); + mocks.sessionInfo.mockResolvedValue({ + session: { sessionId: "session-1", cwd }, + }); + + await expect(acpLoadSession("session-1")).rejects.toThrow( + "Session working directory is unavailable", + ); + + expect(mocks.loadSession).not.toHaveBeenCalled(); + }); + + it("does not send a fallback directory when metadata lookup fails", async () => { + const { acpLoadSession } = await import("../acp"); + mocks.sessionInfo.mockRejectedValue(new Error("session info unavailable")); + + await expect(acpLoadSession("session-1")).rejects.toThrow( + "session info unavailable", + ); + + expect(mocks.loadSession).not.toHaveBeenCalled(); + }); + + it.each([ + { sessionId: "session-1", backendId: "local", stalledCleanup: false }, + { + sessionId: "ssh:devbox#session-1", + backendId: "ssh:devbox", + stalledCleanup: false, + }, + { sessionId: "session-1", backendId: "local", stalledCleanup: true }, + { + sessionId: "ssh:devbox#session-1", + backendId: "ssh:devbox", + stalledCleanup: true, + }, + ])("times out stuck metadata for $sessionId and admits an explicit queued load (stalled cleanup: $stalledCleanup)", async ({ + sessionId, + backendId, + stalledCleanup, + }) => { + vi.useFakeTimers(); + const { acpLoadSession } = await import("../acp"); + mocks.sessionInfo.mockReturnValueOnce(new Promise(() => {})); + if (stalledCleanup) { + mocks.invalidateBackendConnection.mockReturnValueOnce( + new Promise(() => {}), + ); + } + + let recoveryRejected = false; + const recovery = acpLoadSession(sessionId); + const recoveryRejection = expect(recovery) + .rejects.toThrow("ACP operation timed out") + .then(() => { + recoveryRejected = true; + }); + await vi.advanceTimersByTimeAsync(0); + expect(mocks.sessionInfo).toHaveBeenCalledOnce(); + const newerLoad = acpLoadSession(sessionId, "/new/project"); + + await vi.advanceTimersByTimeAsync(59_999); + expect(mocks.invalidateBackendConnection).not.toHaveBeenCalled(); + expect(mocks.loadSession).not.toHaveBeenCalled(); + + await vi.advanceTimersByTimeAsync(1); + expect(mocks.invalidateBackendConnection).toHaveBeenCalledOnce(); + expect(mocks.invalidateBackendConnection).toHaveBeenCalledWith(backendId); + // Check settlement before awaiting, so stalled cleanup fails at the + // liveness bound instead of hanging the test itself. + expect(recoveryRejected).toBe(true); + await recoveryRejection; + await newerLoad; + + expect(mocks.getBackendClient).toHaveBeenLastCalledWith(backendId); + expect(mocks.loadSession).toHaveBeenCalledOnce(); + expect(mocks.loadSession).toHaveBeenCalledWith({ + sessionId: "session-1", + cwd: "/new/project", + mcpServers: [], + }); + expect( + mocks.invalidateBackendConnection.mock.invocationCallOrder[0], + ).toBeLessThan(mocks.loadSession.mock.invocationCallOrder[0]); + + await acpLoadSession(sessionId); + expect(mocks.sessionInfo).toHaveBeenCalledOnce(); + expect(mocks.loadSession).toHaveBeenLastCalledWith({ + sessionId: "session-1", + cwd: "/new/project", + mcpServers: [], + }); + }); + + it.each([ + "resolve", + "reject", + ])("ignores metadata that %ss after a timeout and a newer explicit load", async (settlement) => { + vi.useFakeTimers(); + const { acpLoadSession } = await import("../acp"); + let resolveInfo!: (value: unknown) => void; + let rejectInfo!: (reason: unknown) => void; + mocks.sessionInfo.mockReturnValueOnce( + new Promise((resolve, reject) => { + resolveInfo = resolve; + rejectInfo = reject; + }), + ); + const recovery = acpLoadSession("ssh:devbox#session-1"); + const recoveryRejection = expect(recovery).rejects.toThrow( + "ACP operation timed out", + ); + + await vi.advanceTimersByTimeAsync(60_000); + expect(mocks.invalidateBackendConnection).toHaveBeenCalledOnce(); + await recoveryRejection; + await acpLoadSession("ssh:devbox#session-1", "/new/project"); + + if (settlement === "resolve") { + resolveInfo({ + session: { sessionId: "session-1", cwd: "/stale/project" }, + }); + } else { + rejectInfo(new Error("late metadata failure")); + } + await vi.advanceTimersByTimeAsync(0); + expect(mocks.loadSession).toHaveBeenCalledOnce(); + expect(mocks.invalidateBackendConnection).toHaveBeenCalledOnce(); + + await acpLoadSession("ssh:devbox#session-1"); + expect(mocks.sessionInfo).toHaveBeenCalledOnce(); + expect(mocks.loadSession).toHaveBeenLastCalledWith({ + sessionId: "session-1", + cwd: "/new/project", + mcpServers: [], + }); + }); + + it("does not apply the metadata timeout to a long history replay", async () => { + vi.useFakeTimers(); + const { acpLoadSession } = await import("../acp"); + let resolveLoad!: (value: unknown) => void; + mocks.loadSession.mockReturnValueOnce( + new Promise((resolve) => { + resolveLoad = resolve; + }), + ); + const load = acpLoadSession("session-1"); + + await vi.advanceTimersByTimeAsync(120_000); + expect(mocks.sessionInfo).toHaveBeenCalledOnce(); + expect(mocks.loadSession).toHaveBeenCalledOnce(); + expect(mocks.invalidateBackendConnection).not.toHaveBeenCalled(); + + resolveLoad({ configOptions: [] }); + await load; + await acpLoadSession("session-1"); + expect(mocks.sessionInfo).toHaveBeenCalledOnce(); + expect(mocks.loadSession).toHaveBeenLastCalledWith({ + sessionId: "session-1", + cwd: "/saved/project", + mcpServers: [], + }); + }); + + it.each([ + { sessionId: "session-1", backendId: "local" }, + { sessionId: "ssh:devbox#session-1", backendId: "ssh:devbox" }, + ])("does not commit or publish a detached $backendId replay", async ({ + sessionId, + backendId, + }) => { + const registry = await import("../acpSessionRegistry"); + const { acpLoadSession } = await import("../acp"); + const { setSessionConfigSnapshotHandlers } = await import( + "../acpSessionConfigSnapshots" + ); + const publish = vi.fn(); + setSessionConfigSnapshotHandlers({ applyConfigSnapshots: publish }); + registry.registerPreparedSession( + sessionId, + "openai", + "/old/project", + "old-model", + ); + let resolveLoad!: (value: unknown) => void; + mocks.loadSession.mockReturnValueOnce( + new Promise((resolve) => { + resolveLoad = resolve; + }), + ); + const replay = acpLoadSession(sessionId); + const rejection = expect(replay).rejects.toThrow( + "ACP history replay was abandoned", + ); + await vi.waitFor(() => expect(mocks.loadSession).toHaveBeenCalledOnce()); + await mocks.invalidateBackendConnection(backendId); + registry.registerPreparedSession( + sessionId, + "anthropic", + "/replacement/project", + "replacement-model", + ); + resolveLoad({ + configOptions: [ + { + id: "provider", + kind: { type: "select", currentValue: "openai", options: [] }, + }, + { + id: "model", + category: "model", + kind: { type: "select", currentValue: "stale-model", options: [] }, + }, + ], + }); + await rejection; + expect(publish).not.toHaveBeenCalled(); + expect(registry.requireSessionInvocationSelection(sessionId)).toEqual({ + providerId: "anthropic", + modelId: "replacement-model", + }); + await acpLoadSession(sessionId); + expect(mocks.sessionInfo).not.toHaveBeenCalled(); + expect(mocks.loadSession).toHaveBeenLastCalledWith({ + sessionId: "session-1", + cwd: "/replacement/project", + mcpServers: [], + }); + }); + + it("rechecks the replay generation after registry settlement and before snapshot publication", async () => { + const registry = await import("../acpSessionRegistry"); + const { acpLoadSession } = await import("../acp"); + const { setSessionConfigSnapshotHandlers } = await import( + "../acpSessionConfigSnapshots" + ); + const publish = vi.fn(); + setSessionConfigSnapshotHandlers({ applyConfigSnapshots: publish }); + const realLoad = registry.loadSession; + const spy = vi + .spyOn(registry, "loadSession") + .mockImplementationOnce(async (...args) => { + const result = await realLoad(...args); + await mocks.invalidateBackendConnection("local"); + registry.registerPreparedSession( + "session-1", + "anthropic", + "/replacement/project", + "replacement-model", + ); + return result; + }); + try { + await expect(acpLoadSession("session-1", "/old/project")).rejects.toThrow( + "ACP history replay was abandoned", + ); + expect(publish).not.toHaveBeenCalled(); + expect(registry.requireSessionInvocationSelection("session-1")).toEqual({ + providerId: "anthropic", + modelId: "replacement-model", + }); + } finally { + spy.mockRestore(); + } + }); + + it("does not dispatch replay after its client lookup detaches", async () => { + const { acpLoadSession } = await import("../acp"); + let resolveClient!: (value: unknown) => void; + mocks.getBackendClient.mockReturnValueOnce( + new Promise((resolve) => { + resolveClient = resolve; + }), + ); + const replay = acpLoadSession("session-1", "/old/project"); + const rejection = expect(replay).rejects.toThrow( + "ACP history replay was abandoned", + ); + await vi.waitFor(() => + expect(mocks.getBackendClient).toHaveBeenCalledOnce(), + ); + await mocks.invalidateBackendConnection("local"); + resolveClient({ loadSession: mocks.loadSession }); + await rejection; + expect(mocks.loadSession).not.toHaveBeenCalled(); + }); + + it("does not dispatch metadata after a timed-out client lookup settles", async () => { + vi.useFakeTimers(); + const { acpLoadSession } = await import("../acp"); + let resolveClient!: (value: unknown) => void; + mocks.getBackendClient.mockReturnValueOnce( + new Promise((resolve) => { + resolveClient = resolve; + }), + ); + const recovery = acpLoadSession("session-1"); + const rejection = expect(recovery).rejects.toThrow( + "ACP operation timed out", + ); + await vi.advanceTimersByTimeAsync(60_000); + await rejection; + await acpLoadSession("session-1", "/new/project"); + + resolveClient({ goose: { GooseUnstableSessionInfo: mocks.sessionInfo } }); + await vi.advanceTimersByTimeAsync(0); + + expect(mocks.sessionInfo).not.toHaveBeenCalled(); + expect(mocks.loadSession).toHaveBeenCalledOnce(); + expect(mocks.loadSession).toHaveBeenCalledWith({ + sessionId: "session-1", + cwd: "/new/project", + mcpServers: [], + }); + }); + + it("serializes directory recovery with later loads and retains the latest cwd", async () => { + const { acpLoadSession } = await import("../acp"); + let resolveInfo!: (value: unknown) => void; + mocks.sessionInfo.mockReturnValueOnce( + new Promise((resolve) => { + resolveInfo = resolve; + }), + ); + + const coldLoad = acpLoadSession("session-1"); + await vi.waitFor(() => expect(mocks.sessionInfo).toHaveBeenCalledTimes(1)); + const newerLoad = acpLoadSession("session-1", "/new/project"); + expect(mocks.loadSession).not.toHaveBeenCalled(); + + resolveInfo({ session: { sessionId: "session-1", cwd: "/saved/project" } }); + await Promise.all([coldLoad, newerLoad]); + await acpLoadSession("session-1"); + + expect(mocks.sessionInfo).toHaveBeenCalledTimes(1); + expect( + mocks.loadSession.mock.calls.map(([request]) => request.cwd), + ).toEqual(["/saved/project", "/new/project", "/new/project"]); + }); +}); diff --git a/src/shared/api/__tests__/acpSessionRegistry.test.ts b/src/shared/api/__tests__/acpSessionRegistry.test.ts index 4798691f9..40c2aa8f1 100644 --- a/src/shared/api/__tests__/acpSessionRegistry.test.ts +++ b/src/shared/api/__tests__/acpSessionRegistry.test.ts @@ -10,14 +10,22 @@ const mockSetSessionConfigOption = vi.fn(); const mockUpdateWorkingDir = vi.fn(); const mockLoadSession = vi.fn(); const mockInvalidateClientConnection = vi.fn(); -const noRequestProviderContext = { requestId: undefined }; +const noRequestProviderContext = { + requestId: undefined, + assertActive: expect.any(Function), +}; const noRequestModelContext = (providerId: string) => ({ providerId, requestId: undefined, + assertActive: expect.any(Function), }); vi.mock("../acpConnection", () => ({ getBackendClient: vi.fn(), + captureBackendConnectionGeneration: (backendId: string) => ({ + isCurrent: () => true, + invalidate: () => mockInvalidateClientConnection(backendId), + }), invalidateBackendConnection: (...args: unknown[]) => mockInvalidateClientConnection(...args), })); @@ -335,12 +343,20 @@ describe("applySessionModel", () => { } }); - it("times out a stuck mutation, invalidates ACP, and admits queued work", async () => { + it.each([ + false, + true, + ])("times out a stuck mutation and admits queued work (stalled cleanup: %s)", async (stalledCleanup) => { vi.useFakeTimers(); try { const registry = await importPreparedRegistry("openai", "gpt-5.5"); const stuck = deferred(); mockSetSessionConfigOption.mockReturnValueOnce(stuck.promise); + if (stalledCleanup) { + mockInvalidateClientConnection.mockReturnValueOnce( + new Promise(() => {}), + ); + } mockLoadSession.mockResolvedValueOnce( executionConfigResponse("openai", "gpt-5.5"), ); @@ -350,11 +366,18 @@ describe("applySessionModel", () => { "thinking_effort", "high", ); + let reasoningRejected = false; + const rejection = expect(reasoning) + .rejects.toThrow("ACP operation timed out") + .then(() => { + reasoningRejected = true; + }); const load = registry.loadSession("session-1", "/project"); await vi.advanceTimersByTimeAsync(60_000); - await expect(reasoning).rejects.toThrow("ACP operation timed out"); + expect(reasoningRejected).toBe(true); + await rejection; await expect(load).resolves.toMatchObject({ isCurrent: true }); expect(mockInvalidateClientConnection).toHaveBeenCalledOnce(); expect(mockInvalidateClientConnection).toHaveBeenCalledWith("local"); diff --git a/src/shared/api/acp.ts b/src/shared/api/acp.ts index 29925b06d..cba912b92 100644 --- a/src/shared/api/acp.ts +++ b/src/shared/api/acp.ts @@ -579,21 +579,21 @@ export async function acpLoadSession( sessionId: string, workingDir?: string, ): Promise { - const effectiveWorkingDir = workingDir ?? "~"; const sid = sessionId.slice(0, 8); const t0 = performance.now(); logReasoningEffortInfo("acpLoadSession start", { sessionId: shortLogId(sessionId), }); perfLog(`[perf:load] ${sid} acpLoadSession → client.loadSession`); - const { response, isCurrent, executionSelection } = - await sessionRegistry.loadSession(sessionId, effectiveWorkingDir); + const { response, isCurrent, executionSelection, assertActive } = + await sessionRegistry.loadSession(sessionId, workingDir); if (!isCurrent) { perfLog( `[perf:load] ${sid} dropped superseded load snapshot in ${(performance.now() - t0).toFixed(1)}ms`, ); return undefined; } + assertActive(); const snapshots = readSessionConfigOptionsSnapshots(response); logReasoningEffortInfo("acpLoadSession response", { sessionId: shortLogId(sessionId), diff --git a/src/shared/api/acpApi.ts b/src/shared/api/acpApi.ts index 9d289eb78..f50dcb310 100644 --- a/src/shared/api/acpApi.ts +++ b/src/shared/api/acpApi.ts @@ -149,11 +149,14 @@ function mapSessionInfo(info: SessionInfo): AcpSessionInfo { export async function getSessionInfo( sessionId: string, + assertActive?: () => void, ): Promise { const client = await getClientForSession(sessionId); + assertActive?.(); const result = await client.goose.GooseUnstableSessionInfo({ sessionId: getWireSessionId(sessionId), }); + assertActive?.(); const info = mapSessionInfo(result.session as unknown as SessionInfo); // The backend echoes its bare wire id; callers hold the composite id. return { ...info, sessionId }; @@ -281,17 +284,26 @@ export async function forkSession( export async function setModel( sessionId: string, modelId: string, - context: { providerId?: string; requestId?: string } = {}, + context: { + providerId?: string; + requestId?: string; + assertActive?: () => void; + } = {}, ): Promise { + const { assertActive, ...snapshotContext } = context; const sid = sessionId.slice(0, 8); const tClient = performance.now(); const client = await getClientForSession(sessionId); + // A queued mutation may time out while client setup or the wire call awaits. + // Never send more work or publish a response after its owner abandons it. + assertActive?.(); const tCall = performance.now(); const response = await client.setSessionConfigOption({ sessionId: getWireSessionId(sessionId), configId: "model", value: modelId, }); + assertActive?.(); const snapshots = readSessionConfigOptionsSnapshots(response); logReasoningEffortInfo("setModel response", { sessionId: shortLogId(sessionId), @@ -304,7 +316,7 @@ export async function setModel( }); applySessionConfigOptionsSnapshot(sessionId, response, { origin: "response", - ...context, + ...snapshotContext, modelId: snapshots.model?.modelId ?? modelId, }); perfLog( @@ -317,17 +329,24 @@ export async function setSessionConfigOption( sessionId: string, configId: string, value: string, - context: Omit = {}, + context: Omit & { + assertActive?: () => void; + } = {}, ): Promise { + const { assertActive, ...snapshotContext } = context; const sid = sessionId.slice(0, 8); const tClient = performance.now(); const client = await getClientForSession(sessionId); + // A queued mutation may time out while client setup or the wire call awaits. + // Never send more work or publish a response after its owner abandons it. + assertActive?.(); const tCall = performance.now(); const response = await client.setSessionConfigOption({ sessionId: getWireSessionId(sessionId), configId, value, }); + assertActive?.(); const snapshots = readSessionConfigOptionsSnapshots(response); logReasoningEffortInfo("setSessionConfigOption response", { sessionId: shortLogId(sessionId), @@ -341,7 +360,7 @@ export async function setSessionConfigOption( }); applySessionConfigOptionsSnapshot(sessionId, response, { origin: "response", - ...context, + ...snapshotContext, }); perfLog( `[perf:api] ${sid} setSessionConfigOption(${configId}=${value}) getClient=${(tCall - tClient).toFixed(1)}ms wire=${(performance.now() - tCall).toFixed(1)}ms`, @@ -352,11 +371,15 @@ export async function setSessionConfigOption( export async function setProvider( sessionId: string, providerId: string, - context: { requestId?: string } = {}, + context: { requestId?: string; assertActive?: () => void } = {}, ): Promise { + const { assertActive, ...snapshotContext } = context; const sid = sessionId.slice(0, 8); const tClient = performance.now(); const client = await getClientForSession(sessionId); + // A queued mutation may time out while client setup or the wire call awaits. + // Never send more work or publish a response after its owner abandons it. + assertActive?.(); const wireProvider = toWireProviderId(providerId); const tCall = performance.now(); const response = await client.setSessionConfigOption({ @@ -364,6 +387,7 @@ export async function setProvider( configId: "provider", value: wireProvider, }); + assertActive?.(); const snapshots = readSessionConfigOptionsSnapshots(response); logReasoningEffortInfo("setProvider response", { sessionId: shortLogId(sessionId), @@ -377,7 +401,7 @@ export async function setProvider( }); applySessionConfigOptionsSnapshot(sessionId, response, { origin: "response", - ...context, + ...snapshotContext, providerId, modelId: snapshots.model?.modelId, }); @@ -522,16 +546,19 @@ export async function newSession( export async function loadSession( sessionId: string, workingDir: string, + assertActive?: () => void, ): Promise { const sid = sessionId.slice(0, 8); const tClient = performance.now(); const client = await getClientForSession(sessionId); + assertActive?.(); const tCall = performance.now(); const response = await client.loadSession({ sessionId: getWireSessionId(sessionId), cwd: workingDir, mcpServers: [], }); + assertActive?.(); const snapshots = readSessionConfigOptionsSnapshots(response); logReasoningEffortInfo("loadSession response", { sessionId: shortLogId(sessionId), diff --git a/src/shared/api/acpConnection.ts b/src/shared/api/acpConnection.ts index cbf2fc76f..9f172cdc9 100644 --- a/src/shared/api/acpConnection.ts +++ b/src/shared/api/acpConnection.ts @@ -58,9 +58,11 @@ export function interceptSessionNotifications( * Handles ACP permission requests. When set, `requestPermission` delegates to * it; otherwise the connection falls back to auto-approving (preserving the * default behavior for environments where no handler is registered). + * The signal retires pending UI when the requesting transport is detached. */ export type PermissionRequestHandler = ( request: RequestPermissionRequest, + signal?: AbortSignal, ) => Promise; let permissionHandler: PermissionRequestHandler | null = null; @@ -75,7 +77,11 @@ export function setPermissionHandler(handler: PermissionRequestHandler): void { // connection rewrites inbound payloads to the composite renderer-side id // before they reach the shared handlers; the local backend stays a // byte-identical passthrough. -function createClientCallbacks(backendId: AcpBackendId): () => Client { +function createClientCallbacks( + backendId: AcpBackendId, + isCurrent: () => boolean, + signal: AbortSignal, +): () => Client { const toRendererSessionId = ( payload: T, ): T => @@ -90,9 +96,30 @@ function createClientCallbacks(backendId: AcpBackendId): () => Client { requestPermission: async ( args: RequestPermissionRequest, ): Promise => { + const cancelled: RequestPermissionResponse = { + outcome: { outcome: "cancelled" }, + }; + if (!isCurrent()) return cancelled; const request = toRendererSessionId(args); if (permissionHandler) { - return permissionHandler(request); + // Cancel immediately on detachment even if a handler never settles. + // The signal also retires this generation's pending confirmation UI. + let onAbort!: () => void; + const cancellation = new Promise( + (resolve) => { + onAbort = () => resolve(cancelled); + signal.addEventListener("abort", onAbort, { once: true }); + }, + ); + try { + const response = await Promise.race([ + permissionHandler(request, signal), + cancellation, + ]); + return isCurrent() ? response : cancelled; + } finally { + signal.removeEventListener("abort", onAbort); + } } const optionId = request.options?.[0]?.optionId ?? "approve"; return { @@ -106,6 +133,9 @@ function createClientCallbacks(backendId: AcpBackendId): () => Client { sessionUpdate: async ( wireNotification: SessionNotification, ): Promise => { + // Detaching a timed-out transport does not wait for its cleanup. Ignore + // late notifications before they can publish stale chat/config state. + if (!isCurrent()) return; const notification = toRendererSessionId(wireNotification); for (const interceptor of sessionNotificationInterceptors) { if (interceptor(notification)) { @@ -119,15 +149,25 @@ function createClientCallbacks(backendId: AcpBackendId): () => Client { }); } +export interface AcpConnectionGeneration { + isCurrent(): boolean; + /** Detach only this generation; late cleanup must not touch a replacement. */ + invalidate(): Promise; +} + export interface AcpConnection { getClient(): Promise; getClientSync(): GooseClient | null; isReady(): boolean; + captureGeneration(): AcpConnectionGeneration; /** * Abort the current transport after an ACP request exceeds its liveness * bound. A timed-out request leaves the connection state unknowable; * reconnecting is safer than allowing later mutations to race work still * running remotely. + * Detaches cached and initializing clients synchronously, before returning. + * The promise waits for cleanup of only the detached transport generation; + * callers enforcing a liveness bound must not wait for that cleanup. */ invalidate(): Promise; /** Notifies when the active transport closes. Returns an unsubscribe. */ @@ -156,9 +196,21 @@ export function createAcpConnection( let activeStream: ReturnType | null = null; let activeEndpointCleanup: (() => Promise) | null = null; let invalidationGeneration = 0; + let permissionLifetime = new AbortController(); const closedListeners = new Set<() => void>(); const cleanedStreams = new WeakSet(); + function advanceGeneration(): void { + invalidationGeneration += 1; + resolvedClient = null; + clientPromise = null; + activeStream = null; + activeEndpointCleanup = null; + const previousLifetime = permissionLifetime; + permissionLifetime = new AbortController(); + previousLifetime.abort(); + } + function assertInitializationCurrent(expectedGeneration: number): void { assertAvailable?.(); if (invalidationGeneration !== expectedGeneration) { @@ -205,10 +257,7 @@ export function createAcpConnection( if (activeStream !== stream) { return; } - resolvedClient = null; - clientPromise = null; - activeStream = null; - activeEndpointCleanup = null; + advanceGeneration(); for (const listener of closedListeners) { listener(); } @@ -228,14 +277,16 @@ export function createAcpConnection( }); } - async function invalidate(): Promise { - invalidationGeneration += 1; + async function invalidate(expectedGeneration?: number): Promise { + if ( + expectedGeneration !== undefined && + expectedGeneration !== invalidationGeneration + ) { + return; + } const stream = activeStream; const cleanupEndpoint = activeEndpointCleanup; - activeStream = null; - activeEndpointCleanup = null; - resolvedClient = null; - clientPromise = null; + advanceGeneration(); await cleanupOwnedTransport(stream, cleanupEndpoint); } @@ -265,7 +316,14 @@ export function createAcpConnection( activeStream = stream; activeEndpointCleanup = cleanupEndpoint; - const client = new GooseClient(createClientCallbacks(backendId), stream); + const client = new GooseClient( + createClientCallbacks( + backendId, + () => invalidationGeneration === expectedGeneration, + permissionLifetime.signal, + ), + stream, + ); perfLog( `[perf:conn] ws stream + client created in ${(performance.now() - tStream).toFixed(1)}ms`, ); @@ -289,6 +347,9 @@ export function createAcpConnection( } satisfies GooseInitializeRequest); assertInitializationCurrent(expectedGeneration); } catch (error) { + if (invalidationGeneration === expectedGeneration) { + advanceGeneration(); + } await cleanupOwnedTransport(stream, cleanupEndpoint); throw error; } @@ -312,12 +373,13 @@ export function createAcpConnection( const expectedGeneration = invalidationGeneration; const initialization = initializeConnection(expectedGeneration) .then((client) => { + assertInitializationCurrent(expectedGeneration); resolvedClient = client; return client; }) .catch((error) => { if (clientPromise === initialization) { - clientPromise = null; + advanceGeneration(); } throw error; }); @@ -335,6 +397,13 @@ export function createAcpConnection( getClient, getClientSync: () => resolvedClient, isReady: () => resolvedClient !== null, + captureGeneration: () => { + const generation = invalidationGeneration; + return { + isCurrent: () => generation === invalidationGeneration, + invalidate: () => invalidate(generation), + }; + }, invalidate, onClosed: (cb: () => void) => { closedListeners.add(cb); @@ -427,6 +496,17 @@ export function getBackendClient( return getBackendConnection(backendId).getClient(); } +/** Capture before starting a request, including while its client initializes. */ +export function captureBackendConnectionGeneration( + backendId: AcpBackendId, +): AcpConnectionGeneration { + return getBackendConnection(backendId).captureGeneration(); +} + +/** + * Synchronously detach this backend's client; the returned promise waits for + * generation-scoped transport cleanup, which may not settle on a stuck peer. + */ export async function invalidateBackendConnection( backendId: AcpBackendId, ): Promise { diff --git a/src/shared/api/acpSessionRegistry.ts b/src/shared/api/acpSessionRegistry.ts index 63740f82c..697475ead 100644 --- a/src/shared/api/acpSessionRegistry.ts +++ b/src/shared/api/acpSessionRegistry.ts @@ -1,5 +1,5 @@ import * as acpApi from "./acpApi"; -import { invalidateBackendConnection } from "./acpConnection"; +import { captureBackendConnectionGeneration } from "./acpConnection"; import { getSessionBackend } from "./acpSessionBackends"; import { readSessionExecutionConfigSnapshot, @@ -64,13 +64,25 @@ function replaceExecutionSelection( async function runBoundedSessionMutation( sessionId: string, - mutation: Promise, + mutation: (assertActive: () => void) => Promise, ): Promise { + const generation = captureBackendConnectionGeneration( + getSessionBackend(sessionId), + ); let timeoutId: ReturnType | undefined; let didTimeOut = false; + // Promise.race does not cancel the underlying work. Fence both subsequent + // wire calls and local commits once this entry or its transport is abandoned. + const assertActive = () => { + if (didTimeOut || !generation.isCurrent()) { + throw new Error( + "ACP session mutation was abandoned. Reconnect and retry.", + ); + } + }; try { return await Promise.race([ - mutation, + mutation(assertActive), new Promise((_, reject) => { timeoutId = setTimeout(() => { didTimeOut = true; @@ -83,16 +95,18 @@ async function runBoundedSessionMutation( }), ]); } catch (error) { - if (didTimeOut) { + if (didTimeOut || !generation.isCurrent()) { prepared.delete(sessionId); - await invalidateBackendConnection(getSessionBackend(sessionId)).catch( - (invalidationError) => { - console.error( - "Failed to invalidate timed-out ACP connection:", - invalidationError, - ); - }, - ); + } + if (didTimeOut) { + // Invalidation detaches the client synchronously. Its generation-scoped + // transport cleanup is best-effort and must not hold the session queue. + void generation.invalidate().catch((invalidationError) => { + console.error( + "Failed to invalidate timed-out ACP connection:", + invalidationError, + ); + }); } throw error; } finally { @@ -104,7 +118,7 @@ async function runBoundedSessionMutation( function serializeSessionMutation( sessionId: string, - mutation: (isLatest: () => boolean) => Promise, + mutation: (isLatest: () => boolean, assertActive: () => void) => Promise, bounded = true, ): Promise { let queue = mutationQueues.get(sessionId); @@ -115,9 +129,10 @@ function serializeSessionMutation( const sequence = nextMutationSequence++; queue.latestSequence = sequence; - const execute = () => mutation(() => queue?.latestSequence === sequence); + const execute = (assertActive = () => {}) => + mutation(() => queue?.latestSequence === sequence, assertActive); const result = queue.tail.then(() => - bounded ? runBoundedSessionMutation(sessionId, execute()) : execute(), + bounded ? runBoundedSessionMutation(sessionId, execute) : execute(), ); const tail = result.then( () => undefined, @@ -138,8 +153,8 @@ export async function prepareSession( workingDir: string, options: SessionConfigMutationOptions = {}, ): Promise { - return serializeSessionMutation(sessionId, () => - prepareSessionNow(sessionId, providerId, workingDir, options), + return serializeSessionMutation(sessionId, (_, assertActive) => + prepareSessionNow(sessionId, providerId, workingDir, options, assertActive), ); } @@ -148,7 +163,9 @@ async function prepareSessionNow( providerId: string, workingDir: string, options: SessionConfigMutationOptions, + assertActive: () => void, ): Promise { + assertActive(); const sid = sessionId.slice(0, 8); const existing = prepared.get(sessionId); if (existing) { @@ -165,7 +182,8 @@ async function prepareSessionNow( cachedModelId: existing.executionSelection?.modelId ?? null, }); if (existing.workingDir !== workingDir) { - await acpApi.updateWorkingDir(sessionId, workingDir); + await acpApi.updateWorkingDir(sessionId, workingDir, assertActive); + assertActive(); existing.workingDir = workingDir; changed = true; } @@ -174,8 +192,11 @@ async function prepareSessionNow( try { snapshots = await acpApi.setProvider(sessionId, providerId, { requestId: options.requestId, + assertActive, }); + assertActive(); } catch (error) { + assertActive(); // Goose can apply the provider and then fail while building the // response snapshot. The complete backend pair is unknown until the // UI selection is prepared again. @@ -203,7 +224,8 @@ async function prepareSessionNow( sessionId: shortLogId(sessionId), providerId, }); - await acpApi.loadSession(sessionId, workingDir); + await acpApi.loadSession(sessionId, workingDir, assertActive); + assertActive(); perfLog( `[perf:prepare] ${sid} registry loadSession ok in ${(performance.now() - tLoad).toFixed(1)}ms`, ); @@ -211,7 +233,9 @@ async function prepareSessionNow( const tProv = performance.now(); const snapshots = await acpApi.setProvider(sessionId, providerId, { requestId: options.requestId, + assertActive, }); + assertActive(); perfLog( `[perf:prepare] ${sid} registry setProvider(${providerId}) in ${(performance.now() - tProv).toFixed(1)}ms`, ); @@ -244,8 +268,8 @@ export async function applySessionModel( if (!concreteModelId) { throw new Error(`Invalid model id: ${modelId}`); } - return serializeSessionMutation(sessionId, () => - applySessionModelNow(sessionId, concreteModelId, options), + return serializeSessionMutation(sessionId, (_, assertActive) => + applySessionModelNow(sessionId, concreteModelId, options, assertActive), ); } @@ -253,7 +277,9 @@ async function applySessionModelNow( sessionId: string, modelId: string, options: SessionConfigMutationOptions, + assertActive: () => void, ): Promise { + assertActive(); const sid = sessionId.slice(0, 8); const entry = prepared.get(sessionId); const executionSelection = entry?.executionSelection; @@ -282,8 +308,11 @@ async function applySessionModelNow( snapshots = await acpApi.setModel(sessionId, modelId, { providerId: executionSelection.providerId, requestId: options.requestId, + assertActive, }); + assertActive(); } catch (error) { + assertActive(); // Drop the cached value so the next attempt retries over the wire. replaceExecutionSelection(entry, executionSelection.providerId); throw error; @@ -322,17 +351,23 @@ export async function configureSession( if (modelId && !concreteModelId) { throw new Error(`Invalid model id: ${modelId}`); } - return serializeSessionMutation(sessionId, async () => { + return serializeSessionMutation(sessionId, async (_, assertActive) => { let snapshots = await prepareSessionNow( sessionId, providerId, workingDir, concreteModelId ? {} : options, + assertActive, ); + assertActive(); if (concreteModelId) { snapshots = - (await applySessionModelNow(sessionId, concreteModelId, options)) ?? - snapshots; + (await applySessionModelNow( + sessionId, + concreteModelId, + options, + assertActive, + )) ?? snapshots; } return snapshots; }); @@ -344,8 +379,11 @@ export function applySessionConfigOption( value: string, context: Omit = {}, ): Promise { - return serializeSessionMutation(sessionId, () => - acpApi.setSessionConfigOption(sessionId, configId, value, context), + return serializeSessionMutation(sessionId, (_, assertActive) => + acpApi.setSessionConfigOption(sessionId, configId, value, { + ...context, + assertActive, + }), ); } @@ -383,27 +421,75 @@ export function runPreparedSessionPrompt( ); } +function nonBlankWorkingDir( + workingDir: string | null | undefined, +): string | undefined { + // Detect missing paths without changing spaces in a real directory name. + return workingDir?.trim() ? workingDir : undefined; +} + export async function loadSession( sessionId: string, - workingDir: string, + workingDir?: string, ): Promise<{ response: Awaited>; isCurrent: boolean; + /** Recheck the replay's transport immediately before publishing snapshots. */ + assertActive: () => void; executionSelection?: AcpSessionExecutionSelection; }> { return serializeSessionMutation( sessionId, async (isLatest) => { - const response = await acpApi.loadSession(sessionId, workingDir); + // Replay can legitimately take longer than a config mutation, but it + // still belongs to one transport. Capture after entering the queue so + // a prior timed-out mutation can reconnect before this load starts. + const generation = captureBackendConnectionGeneration( + getSessionBackend(sessionId), + ); + const assertActive = () => { + if (!generation.isCurrent()) { + throw new Error( + "ACP history replay was abandoned. Reconnect and retry.", + ); + } + }; + assertActive(); + // A replay refresh may have no renderer workspace path. Reuse the + // prepared cwd or ask the owning backend; ACP + // requires an absolute path and does not expand a literal "~". + let effectiveWorkingDir = + nonBlankWorkingDir(workingDir) ?? + nonBlankWorkingDir(prepared.get(sessionId)?.workingDir); + if (!effectiveWorkingDir) { + // Bound only metadata recovery, not the potentially long replay. + // Await outside the race so a late response cannot load stale cwd. + const info = await runBoundedSessionMutation( + sessionId, + (assertActive) => acpApi.getSessionInfo(sessionId, assertActive), + ); + assertActive(); + effectiveWorkingDir = nonBlankWorkingDir(info.workingDir); + } + if (!effectiveWorkingDir) { + throw new Error("Session working directory is unavailable."); + } + const response = await acpApi.loadSession( + sessionId, + effectiveWorkingDir, + assertActive, + ); + assertActive(); const isCurrentResult = isLatest(); const executionSnapshot = readSessionExecutionConfigSnapshot(response); prepared.set(sessionId, { - workingDir, + workingDir: effectiveWorkingDir, executionSelection: executionSnapshot ?? undefined, }); return { response, isCurrent: isCurrentResult, + assertActive, executionSelection: executionSnapshot ?? undefined, }; },