diff --git a/src/vs/platform/request/common/request.ts b/src/vs/platform/request/common/request.ts index 8d4dfc7a5383c..8379a0cae038e 100644 --- a/src/vs/platform/request/common/request.ts +++ b/src/vs/platform/request/common/request.ts @@ -5,7 +5,7 @@ import { streamToBuffer } from '../../../base/common/buffer.js'; import { CancellationToken } from '../../../base/common/cancellation.js'; -import { getErrorMessage } from '../../../base/common/errors.js'; +import { getErrorMessage, isCancellationError } from '../../../base/common/errors.js'; import { Emitter, Event } from '../../../base/common/event.js'; import { Disposable } from '../../../base/common/lifecycle.js'; import { IHeaders, IRequestContext, IRequestOptions } from '../../../base/parts/request/common/request.js'; @@ -110,7 +110,11 @@ export abstract class AbstractRequestService extends Disposable implements IRequ }); return result; } catch (error) { - this.logService.error(`${prefix} - error`, options.type, getErrorMessage(error)); + if (isCancellationError(error)) { + this.logService.trace(`${prefix} - cancelled`, options.type); + } else { + this.logService.error(`${prefix} - error`, options.type, getErrorMessage(error)); + } throw error; } } diff --git a/src/vs/platform/request/test/common/requestService.test.ts b/src/vs/platform/request/test/common/requestService.test.ts index 3760902fb1f9c..1cb89aaa00392 100644 --- a/src/vs/platform/request/test/common/requestService.test.ts +++ b/src/vs/platform/request/test/common/requestService.test.ts @@ -6,15 +6,29 @@ import assert from 'assert'; import { bufferToStream, VSBuffer } from '../../../../base/common/buffer.js'; import { CancellationToken } from '../../../../base/common/cancellation.js'; +import { CancellationError, isCancellationError } from '../../../../base/common/errors.js'; import { IRequestContext, IRequestOptions } from '../../../../base/parts/request/common/request.js'; -import { NullLogService } from '../../../log/common/log.js'; +import { ILogService, NullLogService } from '../../../log/common/log.js'; import { AbstractRequestService, AuthInfo, Credentials, IRequestCompleteEvent, NO_FETCH_TELEMETRY } from '../../common/request.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../base/test/common/utils.js'; +class TestLogService extends NullLogService { + readonly traces: string[] = []; + readonly errors: (string | Error)[] = []; + + override trace(message: string, ...args: unknown[]): void { + this.traces.push([message, ...args].join(' ')); + } + + override error(error: string | Error, ..._args: unknown[]): void { + this.errors.push(error); + } +} + class TestRequestService extends AbstractRequestService { - constructor(private readonly handler: (options: IRequestOptions) => Promise) { - super(new NullLogService()); + constructor(private readonly handler: (options: IRequestOptions) => Promise, logService: ILogService = new NullLogService()) { + super(logService); } async request(options: IRequestOptions, token: CancellationToken): Promise { @@ -87,6 +101,24 @@ suite('AbstractRequestService', () => { assert.strictEqual(events.length, 0); }); + test('logs cancellation at trace level', async () => { + const logService = new TestLogService(); + const service = store.add(new TestRequestService(() => Promise.reject(new CancellationError()), logService)); + + await assert.rejects( + () => service.request({ url: 'http://test', callSite: 'test.cancelled' }, CancellationToken.None), + error => isCancellationError(error), + ); + + assert.deepStrictEqual({ + cancelledTraces: logService.traces.filter(message => message.includes(' - cancelled')).length, + errors: logService.errors, + }, { + cancelledTraces: 1, + errors: [], + }); + }); + test('onDidCompleteRequest fires for each request', async () => { const service = store.add(new TestRequestService(() => Promise.resolve(makeResponse(200)))); diff --git a/src/vs/sessions/contrib/providers/remoteAgentHost/browser/cloudSandboxAgentHostContribution.ts b/src/vs/sessions/contrib/providers/remoteAgentHost/browser/cloudSandboxAgentHostContribution.ts index b0820787decfd..bb0cfc14542ff 100644 --- a/src/vs/sessions/contrib/providers/remoteAgentHost/browser/cloudSandboxAgentHostContribution.ts +++ b/src/vs/sessions/contrib/providers/remoteAgentHost/browser/cloudSandboxAgentHostContribution.ts @@ -10,7 +10,7 @@ import { CancellationToken, CancellationTokenSource } from '../../../../../base/common/cancellation.js'; import { Codicon } from '../../../../../base/common/codicons.js'; -import { CancellationError } from '../../../../../base/common/errors.js'; +import { CancellationError, isCancellationError } from '../../../../../base/common/errors.js'; import { Event } from '../../../../../base/common/event.js'; import { Disposable, DisposableMap, DisposableStore, MutableDisposable, toDisposable } from '../../../../../base/common/lifecycle.js'; import { URI } from '../../../../../base/common/uri.js'; @@ -282,6 +282,9 @@ export class CloudSandboxAgentHostContribution extends Disposable implements IWo try { result = await this._apiService.listSessions(token); } catch (error) { + if (token.isCancellationRequested || isCancellationError(error) || !this._isEnabled()) { + return; + } result = { kind: 'failed', reason: error instanceof Error ? error.message : String(error) }; } if (result.kind === 'failed') { diff --git a/src/vs/sessions/contrib/providers/remoteAgentHost/browser/cloudSandboxApiService.ts b/src/vs/sessions/contrib/providers/remoteAgentHost/browser/cloudSandboxApiService.ts index 4b770d4268050..9b6bf633b6241 100644 --- a/src/vs/sessions/contrib/providers/remoteAgentHost/browser/cloudSandboxApiService.ts +++ b/src/vs/sessions/contrib/providers/remoteAgentHost/browser/cloudSandboxApiService.ts @@ -6,7 +6,7 @@ import { Limiter, timeout } from '../../../../../base/common/async.js'; import { CancellationToken } from '../../../../../base/common/cancellation.js'; import { toErrorMessage } from '../../../../../base/common/errorMessage.js'; -import { isCancellationError } from '../../../../../base/common/errors.js'; +import { CancellationError, isCancellationError } from '../../../../../base/common/errors.js'; import { Disposable } from '../../../../../base/common/lifecycle.js'; import { CLOUD_SANDBOX_AGENT_SLUG, @@ -210,6 +210,12 @@ export class CloudSandboxApiService extends Disposable implements ICloudSandboxA batch = response.tasks; hasNextPage = hasNextLink(context.res.headers?.['link']); } catch (error) { + if (isCancellationError(error)) { + throw error; + } + if (token.isCancellationRequested) { + throw new CancellationError(); + } if (page === 1) { return { kind: 'failed', reason: `listTasks failed: ${toErrorMessage(error)}` }; } @@ -524,13 +530,18 @@ export class CloudSandboxApiService extends Disposable implements ICloudSandboxA return context; } catch (error) { // A cancelled request was never answered, so it is not a failure worth counting. - if (!isCancellationError(error) && !token.isCancellationRequested) { + const cancellationError = isCancellationError(error) + ? error + : token.isCancellationRequested ? new CancellationError() : undefined; + if (cancellationError) { + this._logService.trace(`${LOG_PREFIX} ${action} -> cancelled after ${Date.now() - started}ms (budget ${timeoutMs}ms)`); + } else { this._telemetry.reportRequest(action, 'networkError'); + // Elapsed at the budget means our own timeout fired; shorter means something else did. + this._logService.trace(`${LOG_PREFIX} ${action} -> failed after ${Date.now() - started}ms (budget ${timeoutMs}ms)`); + this._logService.error(`${LOG_PREFIX} ${requestMethod} ${url} failed: ${toErrorMessage(error)}`); } - // Elapsed at the budget means our own timeout fired; shorter means something else did. - this._logService.trace(`${LOG_PREFIX} ${action} -> failed after ${Date.now() - started}ms (budget ${timeoutMs}ms)`); - this._logService.error(`${LOG_PREFIX} ${requestMethod} ${url} failed: ${toErrorMessage(error)}`); - throw error; + throw cancellationError ?? error; } } diff --git a/src/vs/sessions/contrib/providers/remoteAgentHost/test/browser/cloudSandboxAgentHostContribution.test.ts b/src/vs/sessions/contrib/providers/remoteAgentHost/test/browser/cloudSandboxAgentHostContribution.test.ts index 6d2ecbe33675a..ee6e0b7d682ee 100644 --- a/src/vs/sessions/contrib/providers/remoteAgentHost/test/browser/cloudSandboxAgentHostContribution.test.ts +++ b/src/vs/sessions/contrib/providers/remoteAgentHost/test/browser/cloudSandboxAgentHostContribution.test.ts @@ -147,6 +147,14 @@ class StubSessionsProvidersService extends Disposable { getProviders(): ISessionsProvider[] { return []; } } +class TestLogService extends NullLogService { + readonly warnings: (string | Error)[] = []; + + override warn(message: string | Error, ..._args: unknown[]): void { + this.warnings.push(message); + } +} + /** The single host filter entry every sandbox environment folds into. */ const GITHUB_SANDBOX_GROUP: IAgentHostGroup = { id: 'githubsandbox', @@ -186,9 +194,11 @@ interface ITestHarness { async function createContribution(store: Pick, sessions: readonly ICloudSandboxDiscoveredSession[], options?: { /** Task Mission Control returns from `createSession`, or a rejection. */ readonly createSession?: () => Promise; + readonly listSessions?: (token: CancellationToken) => Promise; readonly getEnvironment?: (id: string, token: CancellationToken) => Promise; /** Whether the sandbox feature settings start on. Defaults to `true`. */ readonly enabled?: boolean; + readonly logService?: ILogService; }): Promise { const discoveryHandlers: (() => Promise)[] = []; const hostGroups: IAgentHostGroup[] = []; @@ -222,7 +232,10 @@ async function createContribution(store: Pick, sessions: } as ITestHarness; instantiationService.stub(ICloudSandboxApiService, new class extends mock() { - override async listSessions(_token: CancellationToken): Promise { + override async listSessions(token: CancellationToken): Promise { + if (options?.listSessions) { + return options.listSessions(token); + } return { kind: 'complete', sessions: harness.discovered }; } override async getEnvironment(id: string, token: CancellationToken): Promise { @@ -298,7 +311,7 @@ async function createContribution(store: Pick, sessions: }); } }()); - instantiationService.stub(ILogService, new NullLogService()); + instantiationService.stub(ILogService, options?.logService ?? new NullLogService()); const contribution = store.add(instantiationService.createInstance(TestCloudSandboxContribution)); // The constructor kicks off discovery eagerly; re-running the registered handler awaits it, @@ -405,6 +418,22 @@ suite('CloudSandboxAgentHostContribution', () => { assert.deepStrictEqual([...hostGroups], []); }); + test('does not warn when discovery is cancelled', async () => { + const logService = new TestLogService(); + const { contribution } = await createContribution(store, [], { + listSessions: async () => { throw new CancellationError(); }, + logService, + }); + + assert.deepStrictEqual({ + providers: [...contribution.stubProviders.keys()], + warnings: logService.warnings, + }, { + providers: [], + warnings: [], + }); + }); + test('serves a dormant environment from history instead of waking it to open a session', async () => { // Resuming costs minutes and Mission Control cannot say in advance whether a dormant // environment will come back, so opening a session must not gamble that on the user's diff --git a/src/vs/sessions/contrib/providers/remoteAgentHost/test/browser/cloudSandboxApiService.test.ts b/src/vs/sessions/contrib/providers/remoteAgentHost/test/browser/cloudSandboxApiService.test.ts index a096d18e4a393..8768ac86c50f1 100644 --- a/src/vs/sessions/contrib/providers/remoteAgentHost/test/browser/cloudSandboxApiService.test.ts +++ b/src/vs/sessions/contrib/providers/remoteAgentHost/test/browser/cloudSandboxApiService.test.ts @@ -6,7 +6,8 @@ import assert from 'assert'; import { timeout } from '../../../../../../base/common/async.js'; import { bufferToStream, VSBuffer } from '../../../../../../base/common/buffer.js'; -import { CancellationToken } from '../../../../../../base/common/cancellation.js'; +import { CancellationToken, CancellationTokenSource } from '../../../../../../base/common/cancellation.js'; +import { CancellationError, isCancellationError } from '../../../../../../base/common/errors.js'; import { Event } from '../../../../../../base/common/event.js'; import { mock } from '../../../../../../base/test/common/mock.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../../base/test/common/utils.js'; @@ -47,6 +48,19 @@ interface ITestSetup { readonly concurrency: { max: number; current: number }; } +class TestLogService extends NullLogService { + readonly traces: string[] = []; + readonly errors: (string | Error)[] = []; + + override trace(message: string, ...args: unknown[]): void { + this.traces.push([message, ...args].join(' ')); + } + + override error(error: string | Error, ..._args: unknown[]): void { + this.errors.push(error); + } +} + function createService(store: Pick<{ add(t: T): T }, 'add'>, options: { readonly tasks: readonly unknown[]; /** Repository id -> response, or 'error' to fail the lookup. */ @@ -61,6 +75,8 @@ function createService(store: Pick<{ add(t: T): T readonly retryAfterSeconds?: number; /** Suspend every task-detail response by this many ms, so overlapping fetches are observable. */ readonly taskFetchDelayMs?: number; + readonly requestError?: Error; + readonly logService?: ILogService; }): ITestSetup { const requestedUrls: string[] = []; const concurrency = { max: 0, current: 0 }; @@ -75,6 +91,9 @@ function createService(store: Pick<{ add(t: T): T instantiationService.stub(IRequestService, new class extends mock() { override async request(opts: { url?: string }): Promise { + if (options.requestError) { + throw options.requestError; + } const url = opts.url ?? ''; requestedUrls.push(url); const repoMatch = url.match(/\/repositories\/(\d+)$/); @@ -126,7 +145,7 @@ function createService(store: Pick<{ add(t: T): T override readonly onDidChangeSessions = Event.None; }()); instantiationService.stub(IProductService, { defaultChatAgent: undefined } as unknown as IProductService); - instantiationService.stub(ILogService, new NullLogService()); + instantiationService.stub(ILogService, options.logService ?? new NullLogService()); instantiationService.stub(ICloudSandboxTelemetryService, new class extends mock() { override reportRequest(): void { } }()); @@ -159,6 +178,46 @@ suite('CloudSandboxApiService repository resolution', () => { }); }); + test('propagates task-list cancellation without error logging', async () => { + const logService = new TestLogService(); + const { service } = createService(store, { + tasks: [], + repositories: new Map(), + requestError: new CancellationError(), + logService, + }); + + await assert.rejects(() => service.listSessions(CancellationToken.None), error => isCancellationError(error)); + assert.deepStrictEqual({ + cancelledTraces: logService.traces.filter(message => message.includes(' -> cancelled')).length, + errors: logService.errors, + }, { + cancelledTraces: 1, + errors: [], + }); + }); + + test('normalizes a transport failure after cancellation without error logging', async () => { + const logService = new TestLogService(); + const cancellation = store.add(new CancellationTokenSource()); + cancellation.cancel(); + const { service } = createService(store, { + tasks: [], + repositories: new Map(), + requestError: new Error('transport stopped'), + logService, + }); + + await assert.rejects(() => service.listSessions(cancellation.token), error => isCancellationError(error)); + assert.deepStrictEqual({ + cancelledTraces: logService.traces.filter(message => message.includes(' -> cancelled')).length, + errors: logService.errors, + }, { + cancelledTraces: 1, + errors: [], + }); + }); + test('resolves each repository once across a whole discovery pass', async () => { // Tasks resolve concurrently, so the in-flight promise must be shared, not just the result. const { service, requestedUrls } = createService(store, { diff --git a/src/vs/sessions/contrib/providers/remoteAgentHost/test/browser/remoteAgentHostCustomizationHarness.test.ts b/src/vs/sessions/contrib/providers/remoteAgentHost/test/browser/remoteAgentHostCustomizationHarness.test.ts index 2c3711e852f4c..dd74806fde9c3 100644 --- a/src/vs/sessions/contrib/providers/remoteAgentHost/test/browser/remoteAgentHostCustomizationHarness.test.ts +++ b/src/vs/sessions/contrib/providers/remoteAgentHost/test/browser/remoteAgentHostCustomizationHarness.test.ts @@ -19,7 +19,7 @@ import { sessionReducer } from '../../../../../../platform/agentHost/common/stat import { type IAgentSubscription } from '../../../../../../platform/agentHost/common/state/agentSubscription.js'; import { IFileDialogService } from '../../../../../../platform/dialogs/common/dialogs.js'; import { VSBuffer } from '../../../../../../base/common/buffer.js'; -import { IFileService, type IFileContent, type IFileStat, type IFileStatResult } from '../../../../../../platform/files/common/files.js'; +import { FileOperationError, FileOperationResult, IFileService, type IFileContent, type IFileStat, type IFileStatWithMetadata } from '../../../../../../platform/files/common/files.js'; import { PromptsType } from '../../../../../../workbench/contrib/chat/common/promptSyntax/promptTypes.js'; import { NullLogService } from '../../../../../../platform/log/common/log.js'; import { INotificationService } from '../../../../../../platform/notification/common/notification.js'; @@ -44,6 +44,26 @@ class MockPromptsService extends BaseMockPromptsService { } } +/** Throws the file-not-found result used for absent optional plugin directories. */ +function throwFileNotFound(): never { + throw new FileOperationError('File not found', FileOperationResult.FILE_NOT_FOUND); +} + +/** Adds deterministic metadata to a test file stat. */ +function withMetadata(stat: IFileStat): IFileStatWithMetadata { + return { + ...stat, + mtime: stat.mtime ?? 0, + ctime: stat.ctime ?? 0, + etag: stat.etag ?? '', + size: stat.size ?? 0, + readonly: stat.readonly ?? false, + locked: stat.locked ?? false, + executable: stat.executable ?? false, + children: stat.children?.map(withMetadata), + }; +} + class MockAgentConnection extends mock() { private readonly _onDidAction = new Emitter(); @@ -238,7 +258,6 @@ suite('RemoteAgentHostCustomizationHarness', () => { const fileService = new class extends mock() { override async canHandleResource() { return false; } - override async resolveAll() { return []; } }; const provider = disposables.add(new AgentCustomizationItemProvider( @@ -298,7 +317,6 @@ suite('RemoteAgentHostCustomizationHarness', () => { const fileService = new class extends mock() { override async canHandleResource() { return false; } - override async resolveAll() { return []; } }; const provider = disposables.add(new AgentCustomizationItemProvider( @@ -341,7 +359,6 @@ suite('RemoteAgentHostCustomizationHarness', () => { const fileService = new class extends mock() { override async canHandleResource() { return false; } - override async resolveAll() { return []; } }; const provider = disposables.add(new AgentCustomizationItemProvider( @@ -391,37 +408,32 @@ suite('RemoteAgentHostCustomizationHarness', () => { const skillFileUri = URI.parse(`${bundleUri}/skills/my-skill`); const fileService = new class extends mock() { override async canHandleResource() { return true; } - override async resolveAll(resources: { resource: URI }[]): Promise { - return resources.map(r => { - if (r.resource.path.endsWith('/skills')) { - return { - success: true, - stat: { - resource: r.resource, - name: 'skills', - isFile: false, - isDirectory: true, - isSymbolicLink: false, - readonly: false, - mtime: 0, - ctime: 0, - size: 0, - children: [{ - name: 'my-skill', - resource: skillFileUri, - isFile: false, - isDirectory: true, - isSymbolicLink: false, - readonly: false, - mtime: 0, - ctime: 0, - size: 0, - children: [], - }], - }, - } satisfies IFileStatResult; - } - return { success: false, stat: undefined } as unknown as IFileStatResult; + override async resolve(resource: URI): Promise { + if (!resource.path.endsWith('/skills')) { + throwFileNotFound(); + } + return withMetadata({ + resource, + name: 'skills', + isFile: false, + isDirectory: true, + isSymbolicLink: false, + readonly: false, + mtime: 0, + ctime: 0, + size: 0, + children: [{ + name: 'my-skill', + resource: skillFileUri, + isFile: false, + isDirectory: true, + isSymbolicLink: false, + readonly: false, + mtime: 0, + ctime: 0, + size: 0, + children: [], + }], }); } override async readFile(resource: URI): Promise { @@ -476,7 +488,6 @@ suite('RemoteAgentHostCustomizationHarness', () => { const fileService = new class extends mock() { override async canHandleResource() { return false; } - override async resolveAll() { return []; } }; const provider = disposables.add(new AgentCustomizationItemProvider( @@ -523,7 +534,6 @@ suite('RemoteAgentHostCustomizationHarness', () => { const fileService = new class extends mock() { override async canHandleResource() { return false; } - override async resolveAll() { return []; } }; const provider = disposables.add(new AgentCustomizationItemProvider( @@ -562,7 +572,6 @@ suite('RemoteAgentHostCustomizationHarness', () => { const fileService = new class extends mock() { override async canHandleResource() { return false; } - override async resolveAll() { return []; } }; const provider = disposables.add(new AgentCustomizationItemProvider( @@ -645,7 +654,6 @@ suite('RemoteAgentHostCustomizationHarness', () => { const fileService = new class extends mock() { override async canHandleResource() { return false; } - override async resolveAll() { return []; } }; const provider = disposables.add(new AgentCustomizationItemProvider( @@ -743,16 +751,11 @@ suite('RemoteAgentHostCustomizationHarness', () => { const fileService = new class extends mock() { override async canHandleResource() { return true; } - override async resolveAll(toResolve: { resource: URI }[]): Promise { - return toResolve.map(({ resource }) => { - if (resource.path.endsWith('/skills')) { - return { - success: true, - stat: { name: 'skills', resource, isFile: false, isDirectory: true, isSymbolicLink: false, children: skillsDirChildren }, - }; - } - return { success: false }; - }); + override async resolve(resource: URI): Promise { + if (!resource.path.endsWith('/skills')) { + throwFileNotFound(); + } + return withMetadata({ name: 'skills', resource, isFile: false, isDirectory: true, isSymbolicLink: false, children: skillsDirChildren }); } override async readFile(resource: URI): Promise { if (resource.path.endsWith('/valid-skill/SKILL.md')) { @@ -808,10 +811,11 @@ suite('RemoteAgentHostCustomizationHarness', () => { const fileService = new class extends mock() { override async canHandleResource() { return true; } - override async resolveAll(toResolve: { resource: URI }[]): Promise { - return toResolve.map(({ resource }) => resource.path.endsWith('/rules') - ? { success: true, stat: { name: 'rules', resource, isFile: false, isDirectory: true, isSymbolicLink: false, children: rulesDirChildren } } - : { success: false }); + override async resolve(resource: URI): Promise { + if (!resource.path.endsWith('/rules')) { + throwFileNotFound(); + } + return withMetadata({ name: 'rules', resource, isFile: false, isDirectory: true, isSymbolicLink: false, children: rulesDirChildren }); } override async readFile(resource: URI): Promise { const content = '---\nname: My Rule\ndescription: A synced rule\n---\n'; @@ -860,10 +864,11 @@ suite('RemoteAgentHostCustomizationHarness', () => { const ruleResource = URI.parse(`${bundleUri}/rules/user-rule.instructions.md`); const fileService = new class extends mock() { override async canHandleResource() { return true; } - override async resolveAll(toResolve: { resource: URI }[]): Promise { - return toResolve.map(({ resource }) => resource.path.endsWith('/rules') - ? { success: true, stat: { name: 'rules', resource, isFile: false, isDirectory: true, isSymbolicLink: false, children: [{ name: 'user-rule.instructions.md', resource: ruleResource, isFile: true, isDirectory: false, isSymbolicLink: false, children: undefined }] } } - : { success: false }); + override async resolve(resource: URI): Promise { + if (!resource.path.endsWith('/rules')) { + throwFileNotFound(); + } + return withMetadata({ name: 'rules', resource, isFile: false, isDirectory: true, isSymbolicLink: false, children: [{ name: 'user-rule.instructions.md', resource: ruleResource, isFile: true, isDirectory: false, isSymbolicLink: false, children: undefined }] }); } override async readFile(resource: URI): Promise { const content = 'User rule'; @@ -921,10 +926,11 @@ suite('RemoteAgentHostCustomizationHarness', () => { const fileService = new class extends mock() { override async canHandleResource() { return true; } - override async resolveAll(toResolve: { resource: URI }[]): Promise { - return toResolve.map(({ resource }) => resource.path.endsWith('/rules') - ? { success: true, stat: { name: 'rules', resource, isFile: false, isDirectory: true, isSymbolicLink: false, children: rulesDirChildren } } - : { success: false }); + override async resolve(resource: URI): Promise { + if (!resource.path.endsWith('/rules')) { + throwFileNotFound(); + } + return withMetadata({ name: 'rules', resource, isFile: false, isDirectory: true, isSymbolicLink: false, children: rulesDirChildren }); } override async readFile(resource: URI): Promise { const content = '---\nname: My Rule\n---\n'; @@ -972,16 +978,11 @@ suite('RemoteAgentHostCustomizationHarness', () => { const fileService = new class extends mock() { override async canHandleResource() { return true; } - override async resolveAll(toResolve: { resource: URI }[]): Promise { - return toResolve.map(({ resource }) => { - if (resource.path.endsWith('/skills')) { - return { - success: true, - stat: { name: 'skills', resource, isFile: false, isDirectory: true, isSymbolicLink: false, children: skillsDirChildren }, - }; - } - return { success: false }; - }); + override async resolve(resource: URI): Promise { + if (!resource.path.endsWith('/skills')) { + throwFileNotFound(); + } + return withMetadata({ name: 'skills', resource, isFile: false, isDirectory: true, isSymbolicLink: false, children: skillsDirChildren }); } override async readFile(resource: URI): Promise { if (resource.path.endsWith('/lint/SKILL.md')) { diff --git a/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentCustomizationContentExpander.ts b/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentCustomizationContentExpander.ts index 83d98f17bc441..bdc6807c40c18 100644 --- a/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentCustomizationContentExpander.ts +++ b/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentCustomizationContentExpander.ts @@ -4,11 +4,12 @@ *--------------------------------------------------------------------------------------------*/ import { CancellationToken } from '../../../../../../base/common/cancellation.js'; +import { isCancellationError } from '../../../../../../base/common/errors.js'; import { extname } from '../../../../../../base/common/path.js'; import { joinPath } from '../../../../../../base/common/resources.js'; import { URI } from '../../../../../../base/common/uri.js'; import { parseFrontMatter } from '../../../../../../base/common/yaml.js'; -import { IFileService } from '../../../../../../platform/files/common/files.js'; +import { FileOperationResult, IFileService, toFileOperationResult } from '../../../../../../platform/files/common/files.js'; import { ILogService } from '../../../../../../platform/log/common/log.js'; import { AICustomizationSource } from '../../../common/aiCustomizationWorkspaceService.js'; import { ICustomizationItem } from '../../../common/customizationHarnessService.js'; @@ -42,7 +43,17 @@ export class AgentCustomizationContentExpander { const dirNames = ['agents', 'skills', 'commands', 'rules'] as const; const promptTypes = [PromptsType.agent, PromptsType.skill, PromptsType.prompt, PromptsType.instructions] as const; - const stats = await this.fileService.resolveAll(dirNames.map(name => ({ resource: URI.joinPath(fsRoot, name) }))); + const stats = await Promise.all(dirNames.map(async name => { + const resource = URI.joinPath(fsRoot, name); + try { + return await this.fileService.resolve(resource); + } catch (err) { + if (!isExpectedFileAccessError(err, token)) { + this.logService.trace(`[AgentCustomizationContentExpander] Failed to resolve customization directory ${resource.toString()}: ${err}`); + } + return undefined; + } + })); if (token.isCancellationRequested) { return []; @@ -51,18 +62,20 @@ export class AgentCustomizationContentExpander { for (let i = 0; i < dirNames.length; i++) { const stat = stats[i]; const promptType = promptTypes[i]; - if (!stat.success || !stat.stat?.isDirectory || !stat.stat.children) { + if (!stat?.isDirectory || !stat.children) { continue; } if (promptType === PromptsType.skill) { - children.push(...await this.collectFromSkillDir(stat.stat.children, pluginUri, source, groupKey, isBundleItem, pluginLabel, token)); + children.push(...await this.collectFromSkillDir(stat.children, pluginUri, source, groupKey, isBundleItem, pluginLabel, token)); } else { - children.push(...await this.collectFromRegularDir(stat.stat.children, pluginUri, source, promptType, groupKey, isBundleItem, pluginLabel, token)); + children.push(...await this.collectFromRegularDir(stat.children, pluginUri, source, promptType, groupKey, isBundleItem, pluginLabel, token)); } } children.sort((a, b) => `${a.type}:${a.name}`.localeCompare(`${b.type}:${b.name}`)); } catch (err) { - this.logService.trace(`[AgentCustomizationContentExpander] Failed to expand plugin ${pluginUri.toString()}: ${err}`); + if (!isExpectedFileAccessError(err, token)) { + this.logService.trace(`[AgentCustomizationContentExpander] Failed to expand plugin ${pluginUri.toString()}: ${err}`); + } return []; } return children; @@ -194,12 +207,20 @@ export class AgentCustomizationContentExpander { } return { name: undefined, description: undefined, userInvocable: undefined }; } catch (err) { - this.logService.trace(`[AgentCustomizationContentExpander] Failed to read prompt metadata ${promptFileUri.toString()}: ${err}`); + if (!isExpectedFileAccessError(err, token)) { + this.logService.trace(`[AgentCustomizationContentExpander] Failed to read prompt metadata ${promptFileUri.toString()}: ${err}`); + } return undefined; } } } +function isExpectedFileAccessError(error: Error, token: CancellationToken): boolean { + return token.isCancellationRequested + || isCancellationError(error) + || toFileOperationResult(error) === FileOperationResult.FILE_NOT_FOUND; +} + /** * Strips conventional prompt file extensions so we can show `foo` * for `foo.prompt.md`, `foo.instructions.md`, etc. diff --git a/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/syncedCustomizationBundler.ts b/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/syncedCustomizationBundler.ts index c4fde0679fb09..6aea301f22697 100644 --- a/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/syncedCustomizationBundler.ts +++ b/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/syncedCustomizationBundler.ts @@ -3,9 +3,10 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -import { Limiter } from '../../../../../../base/common/async.js'; +import { Limiter, SequencerByKey } from '../../../../../../base/common/async.js'; import { VSBuffer } from '../../../../../../base/common/buffer.js'; -import { Disposable } from '../../../../../../base/common/lifecycle.js'; +import { CancellationError, isCancellationError } from '../../../../../../base/common/errors.js'; +import { Disposable, IDisposable } from '../../../../../../base/common/lifecycle.js'; import { equals } from '../../../../../../base/common/objects.js'; import { ResourceMap } from '../../../../../../base/common/map.js'; import { basename, dirname, extUri } from '../../../../../../base/common/resources.js'; @@ -29,6 +30,7 @@ export { SYNCED_CUSTOMIZATION_SCHEME }; const DISPLAY_NAME = 'VS Code Synced Data'; const FILE_OPERATION_CONCURRENCY = 10; const SKILL_DIRECTORY_IGNORE = new IgnoreFile('.git\nnode_modules\n', '/', undefined, true); +const bundleSequencer = new SequencerByKey(); const MANIFEST_CONTENT = JSON.stringify({ name: DISPLAY_NAME, @@ -55,12 +57,47 @@ function pluginDirForType(type: PromptsType): string | undefined { type QueueFileOperation = (operation: () => Promise) => Promise; +/** Cancels queued operations on disposal while allowing already-started operations to settle. */ +class DrainingFileOperationLimiter implements IDisposable { + + private readonly _limiter = new Limiter(FILE_OPERATION_CONCURRENCY); + private _isDisposed = false; + + queue(operation: () => Promise): Promise { + if (this._isDisposed) { + return Promise.reject(new CancellationError()); + } + return this._limiter.queue(async () => { + if (this._isDisposed) { + throw new CancellationError(); + } + return operation(); + }) as Promise; + } + + dispose(): void { + if (this._isDisposed) { + return; + } + this._isDisposed = true; + void this._disposeWhenIdle(); + } + + private async _disposeWhenIdle(): Promise { + await this._limiter.whenIdle(); + this._limiter.dispose(); + } +} + async function collectDirectoryFiles(fileService: IFileService, logService: ILogService, root: URI, directory: URI, queueFileOperation: QueueFileOperation): Promise { const stat = await queueFileOperation(() => fileService.resolve(directory)); const children = (await Promise.all((stat.children ?? []).map(async child => { try { return await queueFileOperation(() => fileService.stat(child.resource)); } catch (error) { + if (isCancellationError(error)) { + throw error; + } logService.trace('[SyncedCustomizationBundler] Failed to stat skill resource', child.resource.toString(), error); return undefined; } @@ -154,10 +191,11 @@ interface IBundleResult { */ export class SyncedCustomizationBundler extends Disposable { - private readonly _fileOperationLimiter = this._register(new Limiter(FILE_OPERATION_CONCURRENCY)); + private readonly _fileOperationLimiter = this._register(new DrainingFileOperationLimiter()); private readonly _authority: string; private _lastNonce: string | undefined; private _lastRef: IBundleResult | undefined; + private _isDisposed = false; /** Maps a synced (destination) URI string back to its original source location. Rebuilt on every {@link bundle}. */ private _originByDest = new ResourceMap(); @@ -182,7 +220,14 @@ export class SyncedCustomizationBundler extends Disposable { } private _queueFileOperation(operation: () => Promise): Promise { - return this._fileOperationLimiter.queue(operation) as Promise; + this._throwIfDisposed(); + return this._fileOperationLimiter.queue(operation); + } + + private _throwIfDisposed(): void { + if (this._isDisposed) { + throw new CancellationError(); + } } /** @@ -195,6 +240,19 @@ export class SyncedCustomizationBundler extends Disposable { * @returns The bundle result, or `undefined` if there is nothing to sync. */ async bundle(files: readonly ISyncableFile[], mcpServers: readonly ISyncableMcpServer[] = []): Promise { + this._throwIfDisposed(); + try { + const result = await bundleSequencer.queue(this._authority, () => this._bundle(files, mcpServers)); + this._throwIfDisposed(); + return result; + } catch (error) { + this._throwIfDisposed(); + throw error; + } + } + + private async _bundle(files: readonly ISyncableFile[], mcpServers: readonly ISyncableMcpServer[]): Promise { + this._throwIfDisposed(); const syncable = files.filter(f => pluginDirForType(f.type) !== undefined); if (syncable.length === 0 && mcpServers.length === 0) { return undefined; @@ -246,6 +304,7 @@ export class SyncedCustomizationBundler extends Disposable { addEntry(file, source, URI.joinPath(this._rootUri, dir, fileName), `${dir}/${fileName}`); } })); + this._throwIfDisposed(); // Write MCP servers into `.mcp.json`. The agent host's Open Plugin // adapter reads this file relative to the plugin root. Servers are @@ -278,6 +337,7 @@ export class SyncedCustomizationBundler extends Disposable { // Stable nonce: sort so file ordering doesn't matter. hashParts.sort(); const nonce = String(hash(hashParts.join('\n'))); + this._throwIfDisposed(); // Nothing changed since the last successful bundle — reuse it and skip // reading file contents and rewriting the in-memory plugin tree. @@ -298,7 +358,7 @@ export class SyncedCustomizationBundler extends Disposable { destUri: entry.destUri, content: (await this._queueFileOperation(() => this._fileService.readFile(entry.sourceUri))).value, }))); - this._originByDest = originByDest; + this._throwIfDisposed(); // Delete the previous tree for this authority, preserving other authorities try { @@ -308,21 +368,26 @@ export class SyncedCustomizationBundler extends Disposable { } // Write the manifest + this._throwIfDisposed(); const manifestUri = URI.joinPath(this._rootUri, '.plugin', 'plugin.json'); await this._fileService.writeFile(manifestUri, VSBuffer.fromString(MANIFEST_CONTENT)); // Write each source file into the correct plugin directory. for (const entry of fileContents) { + this._throwIfDisposed(); await this._fileService.writeFile(entry.destUri, entry.content); } // Write MCP servers into `.mcp.json`. The agent host's Open Plugin // adapter reads this file relative to the plugin root. if (mcpContent !== undefined) { + this._throwIfDisposed(); const mcpUri = URI.joinPath(this._rootUri, '.mcp.json'); await this._fileService.writeFile(mcpUri, VSBuffer.fromString(mcpContent)); } + this._throwIfDisposed(); + this._originByDest = originByDest; this._lastNonce = nonce; const rootUriString = this._rootUri.toString() as ProtocolURI; @@ -365,4 +430,12 @@ export class SyncedCustomizationBundler extends Disposable { getOrigin(syncedUri: URI): ISyncedCustomizationOrigin | undefined { return this._originByDest.get(syncedUri); } + + override dispose(): void { + if (this._isDisposed) { + return; + } + this._isDisposed = true; + super.dispose(); + } } diff --git a/src/vs/workbench/contrib/chat/test/browser/agentHost/agentCustomizationContentExpander.test.ts b/src/vs/workbench/contrib/chat/test/browser/agentHost/agentCustomizationContentExpander.test.ts index 9f923305739dd..add56943a6a08 100644 --- a/src/vs/workbench/contrib/chat/test/browser/agentHost/agentCustomizationContentExpander.test.ts +++ b/src/vs/workbench/contrib/chat/test/browser/agentHost/agentCustomizationContentExpander.test.ts @@ -21,6 +21,14 @@ import { CancellationToken } from '../../../../../../base/common/cancellation.js const REMOTE_HOST_GROUP = 'remote-host'; const REMOTE_CLIENT_GROUP = 'remote-client'; +class TestLogService extends NullLogService { + readonly traces: string[] = []; + + override trace(message: string, ...args: unknown[]): void { + this.traces.push([message, ...args].join(' ')); + } +} + // --------------------------------------------------------------------------- // Helpers // --------------------------------------------------------------------------- @@ -79,12 +87,17 @@ suite('AgentCustomizationContentExpander', () => { }, ]); - const expander = new AgentCustomizationContentExpander(fileService, new NullLogService()); + const logService = new TestLogService(); + const expander = new AgentCustomizationContentExpander(fileService, logService); const items = await expand(expander, pluginRoot, REMOTE_HOST_GROUP, false, AICustomizationSources.plugin, CancellationToken.None); - assert.deepStrictEqual(items.map(i => ({ type: i.type, name: i.name, description: i.description })), [ - { type: PromptsType.skill, name: 'Lint', description: 'Runs linting' }, - ]); + assert.deepStrictEqual({ + items: items.map(i => ({ type: i.type, name: i.name, description: i.description })), + traces: logService.traces, + }, { + items: [{ type: PromptsType.skill, name: 'Lint', description: 'Runs linting' }], + traces: [], + }); }); test('uses folder name as fallback when SKILL.md has no name frontmatter', async () => { diff --git a/src/vs/workbench/contrib/chat/test/browser/agentSessions/syncedCustomizationBundler.test.ts b/src/vs/workbench/contrib/chat/test/browser/agentSessions/syncedCustomizationBundler.test.ts index 90c0527c69085..70ceccea0b40a 100644 --- a/src/vs/workbench/contrib/chat/test/browser/agentSessions/syncedCustomizationBundler.test.ts +++ b/src/vs/workbench/contrib/chat/test/browser/agentSessions/syncedCustomizationBundler.test.ts @@ -6,6 +6,7 @@ import assert from 'assert'; import sinon from 'sinon'; import { timeout } from '../../../../../../base/common/async.js'; +import { isCancellationError } from '../../../../../../base/common/errors.js'; import { DisposableStore } from '../../../../../../base/common/lifecycle.js'; import { ResourceSet } from '../../../../../../base/common/map.js'; import { Schemas } from '../../../../../../base/common/network.js'; @@ -27,8 +28,9 @@ class TestInMemoryFileSystemProvider extends InMemoryFileSystemProvider { private readonly symbolicLinks = new ResourceSet(); private readonly statFailures = new ResourceSet(); private statDelay = 0; - private activeStats = 0; + activeStats = 0; maxActiveStats = 0; + statCalls = 0; markSymbolicLink(resource: URI): void { this.symbolicLinks.add(resource); @@ -43,6 +45,7 @@ class TestInMemoryFileSystemProvider extends InMemoryFileSystemProvider { } override async stat(resource: URI): Promise { + this.statCalls++; this.activeStats++; this.maxActiveStats = Math.max(this.maxActiveStats, this.activeStats); try { @@ -319,6 +322,71 @@ suite('SyncedCustomizationBundler', () => { assert.strictEqual(memFs.maxActiveStats, 10); }); + test('cancels while queued skill operations drain after disposal', async () => { + const bundler = createBundler(); + const skill = await seedFile('/skills/disposed/SKILL.md', 'skill content'); + for (let index = 0; index < 20; index++) { + await seedFile(`/skills/disposed/references/${index}.md`, `reference ${index}`); + } + memFs.delayStats(20); + + const bundle = bundler.bundle([{ uri: skill, type: PromptsType.skill }]); + while (memFs.maxActiveStats < 10) { + await timeout(0); + } + const statCallsAtDisposal = memFs.statCalls; + bundler.dispose(); + + await assert.rejects(bundle, error => isCancellationError(error)); + await assert.rejects(bundler.bundle([{ uri: skill, type: PromptsType.skill }]), error => isCancellationError(error)); + while (memFs.activeStats > 0) { + await timeout(0); + } + assert.strictEqual(memFs.statCalls, statCallsAtDisposal); + }); + + test('normalizes an in-flight provider failure after disposal to cancellation', async () => { + const bundler = createBundler(); + const resource = await seedFile('/test/unavailable.md', 'content'); + memFs.delayStats(20); + memFs.failStat(resource); + + const bundle = bundler.bundle([{ uri: resource, type: PromptsType.instructions }]); + while (memFs.activeStats === 0) { + await timeout(0); + } + bundler.dispose(); + + await assert.rejects(bundle, error => isCancellationError(error)); + }); + + test('serializes replacement bundles for the same authority', async () => { + const disposedBundler = createBundler('shared-agent'); + const replacementBundler = createBundler('shared-agent'); + const skill = await seedFile('/skills/replaced/SKILL.md', 'old skill content'); + for (let index = 0; index < 20; index++) { + await seedFile(`/skills/replaced/references/${index}.md`, `reference ${index}`); + } + const replacement = await seedFile('/replacement.md', 'replacement content'); + memFs.delayStats(50); + + const disposedBundle = disposedBundler.bundle([{ uri: skill, type: PromptsType.skill }]); + while (memFs.maxActiveStats < 10) { + await timeout(0); + } + disposedBundler.dispose(); + const replacementBundle = replacementBundler.bundle([{ uri: replacement, type: PromptsType.instructions }]); + await timeout(0); + + assert.strictEqual(memFs.activeStats, 10); + await assert.rejects(disposedBundle, error => isCancellationError(error)); + await replacementBundle; + assert.strictEqual( + (await fileService.readFile(URI.from({ scheme: SYNCED_CUSTOMIZATION_SCHEME, path: '/shared-agent/rules/replacement.md' }))).value.toString(), + 'replacement content' + ); + }); + test('skips unreadable nested skill resources', async () => { const bundler = createBundler(); const skill = await seedFile('/skills/unreadable/SKILL.md', 'skill content');