Skip to content

Commit 16bef9b

Browse files
nullvariantona-agentclaude
authored
fix: improve cancellation handling in getCurrentGitConfig (#119)
* fix: improve cancellation handling in getCurrentGitConfig The getCurrentGitConfig function now properly handles cancellation tokens by racing the git operations against a cancellation promise. This prevents resource leaks when initialization is cancelled (e.g., during quick workspace switches or extension deactivation). Changes: - Added Promise.race to abort early when cancellation is requested - Created cancellation promise that rejects when token is cancelled - Maintained backward compatibility for calls without cancellation token - Added E2E tests to verify cancellation behavior This fix addresses a resource leak where git commands would continue running in the background even after the operation was cancelled. 🖥️ IDE: [Cursor](https://cursor.sh) 🔌 Extension: [Claude Code](https://claude.ai/download) Co-authored-by: Ona <no-reply@ona.com> Model-Raw: ona * fix: add disposable cleanup in cancellation handling Review feedback from Claude Code: - Critical: Add finally block to dispose event listener, preventing memory leaks when configPromise completes before cancellation - Minor: Consolidate dynamic imports in tests using before() hook 🖥️ IDE: [Cursor](https://cursor.sh) 🔌 Extension: [Claude Code](https://claude.ai/download) Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> Model-Raw: claude-opus-4-5-20251101 * test: fix cancellation tests for CI without git config Tests were asserting userName/userEmail are defined, but these values are undefined when git config is not set globally (like in CI). Changed assertions to verify the function returns a valid object with expected properties, rather than asserting defined values. 🖥️ IDE: [Cursor](https://cursor.sh) 🔌 Extension: [Claude Code](https://claude.ai/download) Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> Model-Raw: claude-opus-4-5-20251101 --------- Co-authored-by: Ona <no-reply@ona.com> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
1 parent 5bb42cc commit 16bef9b

2 files changed

Lines changed: 128 additions & 9 deletions

File tree

extensions/git-id-switcher/src/gitConfig.ts

Lines changed: 53 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -73,21 +73,65 @@ export async function getCurrentGitConfig(
7373
return { userName: undefined, userEmail: undefined, signingKey: undefined };
7474
}
7575

76-
const [userName, userEmail, signingKey] = await Promise.all([
76+
// If no token provided, execute normally
77+
if (!token) {
78+
const [userName, userEmail, signingKey] = await Promise.all([
79+
execGitInWorkspace(['config', 'user.name']),
80+
execGitInWorkspace(['config', 'user.email']),
81+
execGitInWorkspace(['config', 'user.signingkey']),
82+
]);
83+
84+
return {
85+
userName: userName || undefined,
86+
userEmail: userEmail || undefined,
87+
signingKey: signingKey || undefined,
88+
};
89+
}
90+
91+
// With cancellation token: race the operations against cancellation
92+
const configPromise = Promise.all([
7793
execGitInWorkspace(['config', 'user.name']),
7894
execGitInWorkspace(['config', 'user.email']),
7995
execGitInWorkspace(['config', 'user.signingkey']),
8096
]);
8197

82-
if (token?.isCancellationRequested) {
83-
return { userName: undefined, userEmail: undefined, signingKey: undefined };
84-
}
98+
// Track disposable for cleanup
99+
let disposable: vscode.Disposable | undefined;
85100

86-
return {
87-
userName: userName || undefined,
88-
userEmail: userEmail || undefined,
89-
signingKey: signingKey || undefined,
90-
};
101+
// Create a promise that rejects when cancellation is requested
102+
const cancellationPromise = new Promise<never>((_, reject) => {
103+
if (token.isCancellationRequested) {
104+
reject(new Error('Operation cancelled'));
105+
return;
106+
}
107+
108+
disposable = token.onCancellationRequested(() => {
109+
reject(new Error('Operation cancelled'));
110+
});
111+
});
112+
113+
try {
114+
const [userName, userEmail, signingKey] = await Promise.race([
115+
configPromise,
116+
cancellationPromise,
117+
]);
118+
119+
return {
120+
userName: userName || undefined,
121+
userEmail: userEmail || undefined,
122+
signingKey: signingKey || undefined,
123+
};
124+
} catch (error) {
125+
// If cancelled, return empty config
126+
if (token.isCancellationRequested) {
127+
return { userName: undefined, userEmail: undefined, signingKey: undefined };
128+
}
129+
// Re-throw other errors
130+
throw error;
131+
} finally {
132+
// Always clean up the event listener to prevent memory leaks
133+
disposable?.dispose();
134+
}
91135
}
92136

93137
/**

extensions/git-id-switcher/src/test/e2e/gitConfig.test.ts

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -410,4 +410,79 @@ describe('Git Config E2E Test Suite', function () {
410410
assert.ok(fs.existsSync(gitDir), '.git directory should exist');
411411
});
412412
});
413+
414+
describe('Cancellation Handling', () => {
415+
// Import once for all tests in this describe block
416+
let getCurrentGitConfig: typeof import('../../gitConfig.js').getCurrentGitConfig;
417+
418+
before(async () => {
419+
const module = await import('../../gitConfig.js');
420+
getCurrentGitConfig = module.getCurrentGitConfig;
421+
});
422+
423+
it('should handle cancellation token that is already cancelled', async () => {
424+
// Create a cancellation token that's already cancelled
425+
const tokenSource = new vscode.CancellationTokenSource();
426+
tokenSource.cancel();
427+
428+
const startTime = Date.now();
429+
const config = await getCurrentGitConfig(tokenSource.token);
430+
const duration = Date.now() - startTime;
431+
432+
// Should return empty config immediately
433+
assert.strictEqual(config.userName, undefined, 'userName should be undefined');
434+
assert.strictEqual(config.userEmail, undefined, 'userEmail should be undefined');
435+
assert.strictEqual(config.signingKey, undefined, 'signingKey should be undefined');
436+
437+
// Should complete very quickly (within 100ms)
438+
assert.ok(duration < 100, `Should return immediately for cancelled token, took ${duration}ms`);
439+
440+
tokenSource.dispose();
441+
});
442+
443+
it('should handle cancellation during operation', async () => {
444+
// Create a cancellation token
445+
const tokenSource = new vscode.CancellationTokenSource();
446+
447+
// Start the operation
448+
const configPromise = getCurrentGitConfig(tokenSource.token);
449+
450+
// Cancel immediately (simulating quick workspace switch)
451+
tokenSource.cancel();
452+
453+
const config = await configPromise;
454+
455+
// Should return empty config when cancelled
456+
assert.strictEqual(config.userName, undefined, 'userName should be undefined after cancellation');
457+
assert.strictEqual(config.userEmail, undefined, 'userEmail should be undefined after cancellation');
458+
assert.strictEqual(config.signingKey, undefined, 'signingKey should be undefined after cancellation');
459+
460+
tokenSource.dispose();
461+
});
462+
463+
it('should complete normally without cancellation', async () => {
464+
// Create a cancellation token but don't cancel it
465+
const tokenSource = new vscode.CancellationTokenSource();
466+
467+
const config = await getCurrentGitConfig(tokenSource.token);
468+
469+
// Should return a valid config object (values may be undefined in CI without git config)
470+
assert.ok(typeof config === 'object', 'Should return a config object');
471+
assert.ok('userName' in config, 'Config should have userName property');
472+
assert.ok('userEmail' in config, 'Config should have userEmail property');
473+
assert.ok('signingKey' in config, 'Config should have signingKey property');
474+
475+
tokenSource.dispose();
476+
});
477+
478+
it('should work without cancellation token', async () => {
479+
// Call without token
480+
const config = await getCurrentGitConfig();
481+
482+
// Should return a valid config object (values may be undefined in CI without git config)
483+
assert.ok(typeof config === 'object', 'Should return a config object');
484+
assert.ok('userName' in config, 'Config should have userName property');
485+
assert.ok('userEmail' in config, 'Config should have userEmail property');
486+
});
487+
});
413488
});

0 commit comments

Comments
 (0)