From a0a75fe5da4289df4f9b92b657cbc695bc8ca2cc Mon Sep 17 00:00:00 2001 From: Roo Code Date: Thu, 2 Oct 2025 11:53:06 +0000 Subject: [PATCH] fix: add timeout protection to ProviderSettingsManager initialization - Add ensureInitialized() method with configurable timeout (default 10s) - Implement initialization tracking to prevent multiple init attempts - Handle initialization errors gracefully to prevent infinite retries - Update constructor to call ensureInitialized() with error handling - Update tests to handle async initialization properly This prevents the extension from freezing indefinitely during startup when the ProviderSettingsManager initialization gets stuck. Fixes #8462 --- src/core/config/ProviderSettingsManager.ts | 62 ++++++++++++++++-- .../__tests__/ProviderSettingsManager.spec.ts | 63 +++++++++++++------ 2 files changed, 103 insertions(+), 22 deletions(-) diff --git a/src/core/config/ProviderSettingsManager.ts b/src/core/config/ProviderSettingsManager.ts index 357a04b33a..70a6330913 100644 --- a/src/core/config/ProviderSettingsManager.ts +++ b/src/core/config/ProviderSettingsManager.ts @@ -74,12 +74,15 @@ export class ProviderSettingsManager { } private readonly context: ExtensionContext + private initializationPromise: Promise | null = null + private isInitialized = false constructor(context: ExtensionContext) { this.context = context - - // TODO: We really shouldn't have async methods in the constructor. - this.initialize().catch(console.error) + // Initialize asynchronously with timeout protection + this.ensureInitialized().catch((error) => { + console.error("ProviderSettingsManager initialization failed:", error) + }) } public generateId() { @@ -94,10 +97,50 @@ export class ProviderSettingsManager { return next } + /** + * Ensure the manager is initialized before use. + * This method is idempotent and will only initialize once. + */ + public async ensureInitialized(timeoutMs: number = 10000): Promise { + if (this.isInitialized) { + return + } + + if (!this.initializationPromise) { + this.initializationPromise = this.initializeWithTimeout(timeoutMs) + } + + await this.initializationPromise + } + + /** + * Initialize config if it doesn't exist and run migrations. + * This is now a private method that should be called via ensureInitialized. + */ + private async initializeWithTimeout(timeoutMs: number): Promise { + const timeoutPromise = new Promise((_, reject) => { + setTimeout(() => { + reject(new Error(`ProviderSettingsManager initialization timed out after ${timeoutMs}ms`)) + }, timeoutMs) + }) + + const initPromise = this.initialize() + + try { + await Promise.race([initPromise, timeoutPromise]) + this.isInitialized = true + } catch (error) { + console.error("ProviderSettingsManager initialization failed:", error) + // Set to initialized even on error to prevent infinite retries + this.isInitialized = true + throw error + } + } + /** * Initialize config if it doesn't exist and run migrations. */ - public async initialize() { + private async initialize() { try { return await this.lock(async () => { const providerProfiles = await this.load() @@ -349,6 +392,7 @@ export class ProviderSettingsManager { * List all available configs with metadata. */ public async listConfig(): Promise { + await this.ensureInitialized() try { return await this.lock(async () => { const providerProfiles = await this.load() @@ -371,6 +415,7 @@ export class ProviderSettingsManager { * otherwise generates a new one (for creation scenarios). */ public async saveConfig(name: string, config: ProviderSettingsWithId): Promise { + await this.ensureInitialized() try { return await this.lock(async () => { const providerProfiles = await this.load() @@ -392,6 +437,7 @@ export class ProviderSettingsManager { public async getProfile( params: { name: string } | { id: string }, ): Promise { + await this.ensureInitialized() try { return await this.lock(async () => { const providerProfiles = await this.load() @@ -434,6 +480,7 @@ export class ProviderSettingsManager { public async activateProfile( params: { name: string } | { id: string }, ): Promise { + await this.ensureInitialized() const { name, ...providerSettings } = await this.getProfile(params) try { @@ -452,6 +499,7 @@ export class ProviderSettingsManager { * Delete a config by name. */ public async deleteConfig(name: string) { + await this.ensureInitialized() try { return await this.lock(async () => { const providerProfiles = await this.load() @@ -476,6 +524,7 @@ export class ProviderSettingsManager { * Check if a config exists by name. */ public async hasConfig(name: string) { + await this.ensureInitialized() try { return await this.lock(async () => { const providerProfiles = await this.load() @@ -490,6 +539,7 @@ export class ProviderSettingsManager { * Set the API config for a specific mode. */ public async setModeConfig(mode: Mode, configId: string) { + await this.ensureInitialized() try { return await this.lock(async () => { const providerProfiles = await this.load() @@ -510,6 +560,7 @@ export class ProviderSettingsManager { * Get the API config ID for a specific mode. */ public async getModeConfigId(mode: Mode) { + await this.ensureInitialized() try { return await this.lock(async () => { const { modeApiConfigs } = await this.load() @@ -521,6 +572,7 @@ export class ProviderSettingsManager { } public async export() { + await this.ensureInitialized() try { return await this.lock(async () => { const profiles = providerProfilesSchema.parse(await this.load()) @@ -537,6 +589,7 @@ export class ProviderSettingsManager { } public async import(providerProfiles: ProviderProfiles) { + await this.ensureInitialized() try { return await this.lock(() => this.store(providerProfiles)) } catch (error) { @@ -631,6 +684,7 @@ export class ProviderSettingsManager { cloudProfiles: Record, currentActiveProfileName?: string, ): Promise { + await this.ensureInitialized() try { return await this.lock(async () => { const providerProfiles = await this.load() diff --git a/src/core/config/__tests__/ProviderSettingsManager.spec.ts b/src/core/config/__tests__/ProviderSettingsManager.spec.ts index b710dc6cca..39c5c4fb8f 100644 --- a/src/core/config/__tests__/ProviderSettingsManager.spec.ts +++ b/src/core/config/__tests__/ProviderSettingsManager.spec.ts @@ -26,7 +26,7 @@ const mockContext = { describe("ProviderSettingsManager", () => { let providerSettingsManager: ProviderSettingsManager - beforeEach(() => { + beforeEach(async () => { vi.clearAllMocks() // Reset all mock implementations to default successful behavior mockSecrets.get.mockResolvedValue(null) @@ -36,6 +36,8 @@ describe("ProviderSettingsManager", () => { mockGlobalState.update.mockResolvedValue(undefined) providerSettingsManager = new ProviderSettingsManager(mockContext) + // Wait for initialization to complete in tests + await providerSettingsManager.ensureInitialized() }) describe("initialize", () => { @@ -43,7 +45,7 @@ describe("ProviderSettingsManager", () => { // Mock readConfig to return null mockSecrets.get.mockResolvedValueOnce(null) - await providerSettingsManager.initialize() + await providerSettingsManager.ensureInitialized() // Should not write to storage because readConfig returns defaultConfig expect(mockSecrets.store).not.toHaveBeenCalled() @@ -72,7 +74,7 @@ describe("ProviderSettingsManager", () => { }), ) - await providerSettingsManager.initialize() + await providerSettingsManager.ensureInitialized() expect(mockSecrets.store).not.toHaveBeenCalled() }) @@ -97,7 +99,9 @@ describe("ProviderSettingsManager", () => { }), ) - await providerSettingsManager.initialize() + // Create a new instance to trigger initialization with the mocked data + const newManager = new ProviderSettingsManager(mockContext) + await newManager.ensureInitialized() // Should have written the config with new IDs expect(mockSecrets.store).toHaveBeenCalled() @@ -136,7 +140,9 @@ describe("ProviderSettingsManager", () => { }), ) - await providerSettingsManager.initialize() + // Create a new instance to trigger initialization with the mocked data + const newManager = new ProviderSettingsManager(mockContext) + await newManager.ensureInitialized() // Get the last call to store, which should contain the migrated config const calls = mockSecrets.store.mock.calls @@ -176,7 +182,9 @@ describe("ProviderSettingsManager", () => { }), ) - await providerSettingsManager.initialize() + // Create a new instance to trigger initialization with the mocked data + const newManager = new ProviderSettingsManager(mockContext) + await newManager.ensureInitialized() // Get the last call to store, which should contain the migrated config const calls = mockSecrets.store.mock.calls @@ -218,7 +226,9 @@ describe("ProviderSettingsManager", () => { }), ) - await providerSettingsManager.initialize() + // Create a new instance to trigger initialization with the mocked data + const newManager = new ProviderSettingsManager(mockContext) + await newManager.ensureInitialized() // Get the last call to store, which should contain the migrated config const calls = mockSecrets.store.mock.calls @@ -267,7 +277,9 @@ describe("ProviderSettingsManager", () => { }), ) - await providerSettingsManager.initialize() + // Create a new instance to trigger initialization with the mocked data + const newManager = new ProviderSettingsManager(mockContext) + await newManager.ensureInitialized() // Get the last call to store, which should contain the migrated config const calls = mockSecrets.store.mock.calls @@ -305,15 +317,17 @@ describe("ProviderSettingsManager", () => { }), ) - await providerSettingsManager.initialize() + // Create a new instance to trigger initialization with the mocked data + const firstManager = new ProviderSettingsManager(mockContext) + await firstManager.ensureInitialized() // Verify migration happened let calls = mockSecrets.store.mock.calls let storedConfig = JSON.parse(calls[calls.length - 1][1]) expect(storedConfig.apiConfigs.default.apiModelId).toEqual("roo/code-supernova-1-million") - // Create a new instance to simulate another load - const newManager = new ProviderSettingsManager(mockContext) + // Clear mocks for second test + mockSecrets.store.mockClear() // Somehow the model got reverted (e.g., manual edit, sync issue) mockSecrets.get.mockResolvedValue( @@ -336,7 +350,9 @@ describe("ProviderSettingsManager", () => { }), ) - await newManager.initialize() + // Create another new instance to simulate another load + const secondManager = new ProviderSettingsManager(mockContext) + await secondManager.ensureInitialized() // Verify migration happened again calls = mockSecrets.store.mock.calls @@ -347,7 +363,10 @@ describe("ProviderSettingsManager", () => { it("should throw error if secrets storage fails", async () => { mockSecrets.get.mockRejectedValue(new Error("Storage failed")) - await expect(providerSettingsManager.initialize()).rejects.toThrow( + // Create a new instance that will fail during initialization + const failingManager = new ProviderSettingsManager(mockContext) + + await expect(failingManager.ensureInitialized()).rejects.toThrow( "Failed to initialize config: Error: Failed to read provider profiles from secrets: Error: Storage failed", ) }) @@ -408,8 +427,11 @@ describe("ProviderSettingsManager", () => { it("should throw error if reading from secrets fails", async () => { mockSecrets.get.mockRejectedValue(new Error("Read failed")) - await expect(providerSettingsManager.listConfig()).rejects.toThrow( - "Failed to list configs: Error: Failed to read provider profiles from secrets: Error: Read failed", + // Create a new instance that will fail during initialization + const failingManager = new ProviderSettingsManager(mockContext) + + await expect(failingManager.listConfig()).rejects.toThrow( + "Failed to initialize config: Error: Failed to read provider profiles from secrets: Error: Read failed", ) }) }) @@ -730,7 +752,9 @@ describe("ProviderSettingsManager", () => { mockSecrets.get.mockResolvedValue(JSON.stringify(invalidConfig)) - await providerSettingsManager.initialize() + // Create a new instance to trigger initialization with the mocked data + const newManager = new ProviderSettingsManager(mockContext) + await newManager.ensureInitialized() const storeCalls = mockSecrets.store.mock.calls expect(storeCalls.length).toBeGreaterThan(0) // Ensure store was called at least once. @@ -788,8 +812,11 @@ describe("ProviderSettingsManager", () => { it("should throw error if secrets storage fails", async () => { mockSecrets.get.mockRejectedValue(new Error("Storage failed")) - await expect(providerSettingsManager.hasConfig("test")).rejects.toThrow( - "Failed to check config existence: Error: Failed to read provider profiles from secrets: Error: Storage failed", + // Create a new instance that will fail during initialization + const failingManager = new ProviderSettingsManager(mockContext) + + await expect(failingManager.hasConfig("test")).rejects.toThrow( + "Failed to initialize config: Error: Failed to read provider profiles from secrets: Error: Storage failed", ) }) })