Skip to content

Commit efc8678

Browse files
0xMink0xMink
committed
fix(McpHub): pass cancellationDisposable to cleanup() to avoid TDZ on sync cancellation
When CancellationToken.onCancellationRequested() registers a listener on an already-cancelled token, VS Code fires the callback synchronously inside the registration call — before the `const cancellationDisposable = onCancellationRequested(...)` assignment completes. The old `let`- predeclared shape was no-op-safe via optional chaining, but the prefer-const consolidation in this PR put `cancellationDisposable` into the temporal dead zone for the duration of the sync callback, turning the already-cancelled path into a ReferenceError. Restructured cleanup() to receive the disposable as a parameter instead of capturing it from outer scope: - cleanup signature now takes `disposable?: vscode.Disposable` - non-callback call sites pass cancellationDisposable explicitly - the cancellation callback calls cleanup() with no arg; VS Code's CancellationToken cleans up its own listener after firing, so disposing from inside the listener was already redundant Keeps cancellationDisposable as a const, so prefer-const remains satisfied. Includes a regression test that fires the cancellation listener synchronously during registration and asserts the flow resolves cleanly.
1 parent 21dd749 commit efc8678

2 files changed

Lines changed: 53 additions & 8 deletions

File tree

‎src/services/mcp/McpHub.ts‎

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1130,12 +1130,12 @@ export class McpHub {
11301130
return new Promise<void>((resolve) => {
11311131
let disposed = false
11321132

1133-
const cleanup = () => {
1133+
const cleanup = (disposable?: vscode.Disposable) => {
11341134
if (disposed) return
11351135
disposed = true
11361136
clearTimeout(timeoutHandle)
11371137
unsubscribe()
1138-
cancellationDisposable?.dispose()
1138+
disposable?.dispose()
11391139
this._oauthWatchers.delete(watcherKey)
11401140
}
11411141

@@ -1146,7 +1146,7 @@ export class McpHub {
11461146
// from another window fired after _completeOAuthFlow already succeeded).
11471147
const conn = this.findConnection(name, source)
11481148
if (!conn || conn.server.status === "connected") {
1149-
cleanup()
1149+
cleanup(cancellationDisposable)
11501150
return
11511151
}
11521152
try {
@@ -1155,7 +1155,7 @@ export class McpHub {
11551155
// fired while getOAuthData was in flight.
11561156
if (disposed || this.isDisposed) return
11571157
if (data && Date.now() < data.expires_at - TOKEN_EXPIRY_BUFFER_MS) {
1158-
cleanup()
1158+
cleanup(cancellationDisposable)
11591159
await authProvider.close()
11601160
await this.deleteConnection(name, source)
11611161
const validatedConfig = this.validateServerConfig(config, name)
@@ -1175,7 +1175,7 @@ export class McpHub {
11751175
// --- Timeout ---
11761176
const timeoutHandle = setTimeout(() => {
11771177
if (disposed) return
1178-
cleanup()
1178+
cleanup(cancellationDisposable)
11791179
void authProvider.close()
11801180
const conn = this.findConnection(name, source)
11811181
if (conn && conn.server.status === "connecting") {
@@ -1189,6 +1189,13 @@ export class McpHub {
11891189
// --- Cancellation (progress bar's Cancel button) ---
11901190
const cancellationDisposable = cancellationToken.onCancellationRequested(() => {
11911191
if (disposed) return
1192+
// Don't pass cancellationDisposable here: if the token was already
1193+
// cancelled at registration time, VS Code fires this callback
1194+
// synchronously inside onCancellationRequested(...) — before the
1195+
// `const cancellationDisposable = ...` assignment completes, so the
1196+
// binding is still in the TDZ. VS Code also disposes its own
1197+
// listener once the token transitions to cancelled, so explicit
1198+
// disposal from inside the callback would be redundant anyway.
11921199
cleanup()
11931200
void authProvider.close()
11941201
const conn = this.findConnection(name, source)
@@ -1206,7 +1213,7 @@ export class McpHub {
12061213
// In test mode, skip user interaction and proceed directly to complete the OAuth flow.
12071214
if (process.env.MCP_OAUTH_TEST_MODE === "true") {
12081215
void (async () => {
1209-
cleanup()
1216+
cleanup(cancellationDisposable)
12101217
try {
12111218
await this._completeOAuthFlow(
12121219
authProvider,
@@ -1244,7 +1251,7 @@ export class McpHub {
12441251
const tokens = await this.secretStorage!.getOAuthData(serverUrl)
12451252
if (disposed) return
12461253
if (tokens && Date.now() < tokens.expires_at - TOKEN_EXPIRY_BUFFER_MS) {
1247-
cleanup()
1254+
cleanup(cancellationDisposable)
12481255
await authProvider.close()
12491256
await this.deleteConnection(name, source)
12501257
const validatedFastPathConfig = this.validateServerConfig(config, name)
@@ -1257,7 +1264,7 @@ export class McpHub {
12571264
// Clean up before exchanging tokens — this sets disposed=true so the
12581265
// cross-window token watcher doesn't race to reconnect when SecretStorage
12591266
// fires onDidChange during token exchange inside _completeOAuthFlow.
1260-
cleanup()
1267+
cleanup(cancellationDisposable)
12611268
try {
12621269
await this._completeOAuthFlow(authProvider, transport, connection, name, source, cancellationToken)
12631270
} catch {

‎src/services/mcp/__tests__/McpHub.spec.ts‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2582,6 +2582,44 @@ describe("McpHub", () => {
25822582
expect(mockAuthProvider.close).toHaveBeenCalled()
25832583
})
25842584

2585+
it("should handle a token that is already cancelled at listener registration time", async () => {
2586+
// Regression: VS Code's CancellationToken.onCancellationRequested() fires the
2587+
// callback synchronously inside the registration call when the token is already
2588+
// cancelled. If cleanup() captures `const cancellationDisposable = onCancellationRequested(...)`
2589+
// from outer scope, the callback runs while that binding is still in the temporal
2590+
// dead zone and accessing it throws ReferenceError. cleanup() must therefore
2591+
// receive the disposable as a parameter and the cancellation callback must invoke
2592+
// cleanup() without it.
2593+
vsc.window.withProgress.mockImplementationOnce((_options: any, task: any) => {
2594+
const progress = { report: vi.fn() }
2595+
const cancellationToken = {
2596+
isCancellationRequested: true,
2597+
// Fire synchronously during registration — simulates an already-cancelled
2598+
// token in real VS Code.
2599+
onCancellationRequested: vi.fn((cb: () => void) => {
2600+
cb()
2601+
return { dispose: vi.fn() }
2602+
}),
2603+
}
2604+
return task(progress, cancellationToken)
2605+
})
2606+
2607+
await expect(
2608+
(mcpHub as any)._initiateOAuthFlow(
2609+
serverName,
2610+
source,
2611+
config,
2612+
mockAuthProvider,
2613+
mockTransport,
2614+
mockConnection,
2615+
),
2616+
).resolves.toBeUndefined()
2617+
2618+
expect(mockConnection.server.status).toBe("disconnected")
2619+
expect((mcpHub as any).appendErrorMessage).toHaveBeenCalled()
2620+
expect(mockAuthProvider.close).toHaveBeenCalled()
2621+
})
2622+
25852623
it("should disconnect and flag error when OAuth flow times out", async () => {
25862624
vi.useFakeTimers()
25872625
vsc.window.showInformationMessage.mockImplementation(() => new Promise(() => {}))

0 commit comments

Comments
 (0)