Skip to content

Commit a0a75fe

Browse files
roomoteroomote
committed
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
1 parent 13534cc commit a0a75fe

File tree

2 files changed

+103
-22
lines changed

2 files changed

+103
-22
lines changed

‎src/core/config/ProviderSettingsManager.ts‎

Lines changed: 58 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -74,12 +74,15 @@ export class ProviderSettingsManager {
7474
}
7575

7676
private readonly context: ExtensionContext
77+
private initializationPromise: Promise<void> | null = null
78+
private isInitialized = false
7779

7880
constructor(context: ExtensionContext) {
7981
this.context = context
80-
81-
// TODO: We really shouldn't have async methods in the constructor.
82-
this.initialize().catch(console.error)
82+
// Initialize asynchronously with timeout protection
83+
this.ensureInitialized().catch((error) => {
84+
console.error("ProviderSettingsManager initialization failed:", error)
85+
})
8386
}
8487

8588
public generateId() {
@@ -94,10 +97,50 @@ export class ProviderSettingsManager {
9497
return next
9598
}
9699

100+
/**
101+
* Ensure the manager is initialized before use.
102+
* This method is idempotent and will only initialize once.
103+
*/
104+
public async ensureInitialized(timeoutMs: number = 10000): Promise<void> {
105+
if (this.isInitialized) {
106+
return
107+
}
108+
109+
if (!this.initializationPromise) {
110+
this.initializationPromise = this.initializeWithTimeout(timeoutMs)
111+
}
112+
113+
await this.initializationPromise
114+
}
115+
116+
/**
117+
* Initialize config if it doesn't exist and run migrations.
118+
* This is now a private method that should be called via ensureInitialized.
119+
*/
120+
private async initializeWithTimeout(timeoutMs: number): Promise<void> {
121+
const timeoutPromise = new Promise<never>((_, reject) => {
122+
setTimeout(() => {
123+
reject(new Error(`ProviderSettingsManager initialization timed out after ${timeoutMs}ms`))
124+
}, timeoutMs)
125+
})
126+
127+
const initPromise = this.initialize()
128+
129+
try {
130+
await Promise.race([initPromise, timeoutPromise])
131+
this.isInitialized = true
132+
} catch (error) {
133+
console.error("ProviderSettingsManager initialization failed:", error)
134+
// Set to initialized even on error to prevent infinite retries
135+
this.isInitialized = true
136+
throw error
137+
}
138+
}
139+
97140
/**
98141
* Initialize config if it doesn't exist and run migrations.
99142
*/
100-
public async initialize() {
143+
private async initialize() {
101144
try {
102145
return await this.lock(async () => {
103146
const providerProfiles = await this.load()
@@ -349,6 +392,7 @@ export class ProviderSettingsManager {
349392
* List all available configs with metadata.
350393
*/
351394
public async listConfig(): Promise<ProviderSettingsEntry[]> {
395+
await this.ensureInitialized()
352396
try {
353397
return await this.lock(async () => {
354398
const providerProfiles = await this.load()
@@ -371,6 +415,7 @@ export class ProviderSettingsManager {
371415
* otherwise generates a new one (for creation scenarios).
372416
*/
373417
public async saveConfig(name: string, config: ProviderSettingsWithId): Promise<string> {
418+
await this.ensureInitialized()
374419
try {
375420
return await this.lock(async () => {
376421
const providerProfiles = await this.load()
@@ -392,6 +437,7 @@ export class ProviderSettingsManager {
392437
public async getProfile(
393438
params: { name: string } | { id: string },
394439
): Promise<ProviderSettingsWithId & { name: string }> {
440+
await this.ensureInitialized()
395441
try {
396442
return await this.lock(async () => {
397443
const providerProfiles = await this.load()
@@ -434,6 +480,7 @@ export class ProviderSettingsManager {
434480
public async activateProfile(
435481
params: { name: string } | { id: string },
436482
): Promise<ProviderSettingsWithId & { name: string }> {
483+
await this.ensureInitialized()
437484
const { name, ...providerSettings } = await this.getProfile(params)
438485

439486
try {
@@ -452,6 +499,7 @@ export class ProviderSettingsManager {
452499
* Delete a config by name.
453500
*/
454501
public async deleteConfig(name: string) {
502+
await this.ensureInitialized()
455503
try {
456504
return await this.lock(async () => {
457505
const providerProfiles = await this.load()
@@ -476,6 +524,7 @@ export class ProviderSettingsManager {
476524
* Check if a config exists by name.
477525
*/
478526
public async hasConfig(name: string) {
527+
await this.ensureInitialized()
479528
try {
480529
return await this.lock(async () => {
481530
const providerProfiles = await this.load()
@@ -490,6 +539,7 @@ export class ProviderSettingsManager {
490539
* Set the API config for a specific mode.
491540
*/
492541
public async setModeConfig(mode: Mode, configId: string) {
542+
await this.ensureInitialized()
493543
try {
494544
return await this.lock(async () => {
495545
const providerProfiles = await this.load()
@@ -510,6 +560,7 @@ export class ProviderSettingsManager {
510560
* Get the API config ID for a specific mode.
511561
*/
512562
public async getModeConfigId(mode: Mode) {
563+
await this.ensureInitialized()
513564
try {
514565
return await this.lock(async () => {
515566
const { modeApiConfigs } = await this.load()
@@ -521,6 +572,7 @@ export class ProviderSettingsManager {
521572
}
522573

523574
public async export() {
575+
await this.ensureInitialized()
524576
try {
525577
return await this.lock(async () => {
526578
const profiles = providerProfilesSchema.parse(await this.load())
@@ -537,6 +589,7 @@ export class ProviderSettingsManager {
537589
}
538590

539591
public async import(providerProfiles: ProviderProfiles) {
592+
await this.ensureInitialized()
540593
try {
541594
return await this.lock(() => this.store(providerProfiles))
542595
} catch (error) {
@@ -631,6 +684,7 @@ export class ProviderSettingsManager {
631684
cloudProfiles: Record<string, ProviderSettingsWithId>,
632685
currentActiveProfileName?: string,
633686
): Promise<SyncCloudProfilesResult> {
687+
await this.ensureInitialized()
634688
try {
635689
return await this.lock(async () => {
636690
const providerProfiles = await this.load()

‎src/core/config/__tests__/ProviderSettingsManager.spec.ts‎

Lines changed: 45 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ const mockContext = {
2626
describe("ProviderSettingsManager", () => {
2727
let providerSettingsManager: ProviderSettingsManager
2828

29-
beforeEach(() => {
29+
beforeEach(async () => {
3030
vi.clearAllMocks()
3131
// Reset all mock implementations to default successful behavior
3232
mockSecrets.get.mockResolvedValue(null)
@@ -36,14 +36,16 @@ describe("ProviderSettingsManager", () => {
3636
mockGlobalState.update.mockResolvedValue(undefined)
3737

3838
providerSettingsManager = new ProviderSettingsManager(mockContext)
39+
// Wait for initialization to complete in tests
40+
await providerSettingsManager.ensureInitialized()
3941
})
4042

4143
describe("initialize", () => {
4244
it("should not write to storage when secrets.get returns null", async () => {
4345
// Mock readConfig to return null
4446
mockSecrets.get.mockResolvedValueOnce(null)
4547

46-
await providerSettingsManager.initialize()
48+
await providerSettingsManager.ensureInitialized()
4749

4850
// Should not write to storage because readConfig returns defaultConfig
4951
expect(mockSecrets.store).not.toHaveBeenCalled()
@@ -72,7 +74,7 @@ describe("ProviderSettingsManager", () => {
7274
}),
7375
)
7476

75-
await providerSettingsManager.initialize()
77+
await providerSettingsManager.ensureInitialized()
7678

7779
expect(mockSecrets.store).not.toHaveBeenCalled()
7880
})
@@ -97,7 +99,9 @@ describe("ProviderSettingsManager", () => {
9799
}),
98100
)
99101

100-
await providerSettingsManager.initialize()
102+
// Create a new instance to trigger initialization with the mocked data
103+
const newManager = new ProviderSettingsManager(mockContext)
104+
await newManager.ensureInitialized()
101105

102106
// Should have written the config with new IDs
103107
expect(mockSecrets.store).toHaveBeenCalled()
@@ -136,7 +140,9 @@ describe("ProviderSettingsManager", () => {
136140
}),
137141
)
138142

139-
await providerSettingsManager.initialize()
143+
// Create a new instance to trigger initialization with the mocked data
144+
const newManager = new ProviderSettingsManager(mockContext)
145+
await newManager.ensureInitialized()
140146

141147
// Get the last call to store, which should contain the migrated config
142148
const calls = mockSecrets.store.mock.calls
@@ -176,7 +182,9 @@ describe("ProviderSettingsManager", () => {
176182
}),
177183
)
178184

179-
await providerSettingsManager.initialize()
185+
// Create a new instance to trigger initialization with the mocked data
186+
const newManager = new ProviderSettingsManager(mockContext)
187+
await newManager.ensureInitialized()
180188

181189
// Get the last call to store, which should contain the migrated config
182190
const calls = mockSecrets.store.mock.calls
@@ -218,7 +226,9 @@ describe("ProviderSettingsManager", () => {
218226
}),
219227
)
220228

221-
await providerSettingsManager.initialize()
229+
// Create a new instance to trigger initialization with the mocked data
230+
const newManager = new ProviderSettingsManager(mockContext)
231+
await newManager.ensureInitialized()
222232

223233
// Get the last call to store, which should contain the migrated config
224234
const calls = mockSecrets.store.mock.calls
@@ -267,7 +277,9 @@ describe("ProviderSettingsManager", () => {
267277
}),
268278
)
269279

270-
await providerSettingsManager.initialize()
280+
// Create a new instance to trigger initialization with the mocked data
281+
const newManager = new ProviderSettingsManager(mockContext)
282+
await newManager.ensureInitialized()
271283

272284
// Get the last call to store, which should contain the migrated config
273285
const calls = mockSecrets.store.mock.calls
@@ -305,15 +317,17 @@ describe("ProviderSettingsManager", () => {
305317
}),
306318
)
307319

308-
await providerSettingsManager.initialize()
320+
// Create a new instance to trigger initialization with the mocked data
321+
const firstManager = new ProviderSettingsManager(mockContext)
322+
await firstManager.ensureInitialized()
309323

310324
// Verify migration happened
311325
let calls = mockSecrets.store.mock.calls
312326
let storedConfig = JSON.parse(calls[calls.length - 1][1])
313327
expect(storedConfig.apiConfigs.default.apiModelId).toEqual("roo/code-supernova-1-million")
314328

315-
// Create a new instance to simulate another load
316-
const newManager = new ProviderSettingsManager(mockContext)
329+
// Clear mocks for second test
330+
mockSecrets.store.mockClear()
317331

318332
// Somehow the model got reverted (e.g., manual edit, sync issue)
319333
mockSecrets.get.mockResolvedValue(
@@ -336,7 +350,9 @@ describe("ProviderSettingsManager", () => {
336350
}),
337351
)
338352

339-
await newManager.initialize()
353+
// Create another new instance to simulate another load
354+
const secondManager = new ProviderSettingsManager(mockContext)
355+
await secondManager.ensureInitialized()
340356

341357
// Verify migration happened again
342358
calls = mockSecrets.store.mock.calls
@@ -347,7 +363,10 @@ describe("ProviderSettingsManager", () => {
347363
it("should throw error if secrets storage fails", async () => {
348364
mockSecrets.get.mockRejectedValue(new Error("Storage failed"))
349365

350-
await expect(providerSettingsManager.initialize()).rejects.toThrow(
366+
// Create a new instance that will fail during initialization
367+
const failingManager = new ProviderSettingsManager(mockContext)
368+
369+
await expect(failingManager.ensureInitialized()).rejects.toThrow(
351370
"Failed to initialize config: Error: Failed to read provider profiles from secrets: Error: Storage failed",
352371
)
353372
})
@@ -408,8 +427,11 @@ describe("ProviderSettingsManager", () => {
408427
it("should throw error if reading from secrets fails", async () => {
409428
mockSecrets.get.mockRejectedValue(new Error("Read failed"))
410429

411-
await expect(providerSettingsManager.listConfig()).rejects.toThrow(
412-
"Failed to list configs: Error: Failed to read provider profiles from secrets: Error: Read failed",
430+
// Create a new instance that will fail during initialization
431+
const failingManager = new ProviderSettingsManager(mockContext)
432+
433+
await expect(failingManager.listConfig()).rejects.toThrow(
434+
"Failed to initialize config: Error: Failed to read provider profiles from secrets: Error: Read failed",
413435
)
414436
})
415437
})
@@ -730,7 +752,9 @@ describe("ProviderSettingsManager", () => {
730752

731753
mockSecrets.get.mockResolvedValue(JSON.stringify(invalidConfig))
732754

733-
await providerSettingsManager.initialize()
755+
// Create a new instance to trigger initialization with the mocked data
756+
const newManager = new ProviderSettingsManager(mockContext)
757+
await newManager.ensureInitialized()
734758

735759
const storeCalls = mockSecrets.store.mock.calls
736760
expect(storeCalls.length).toBeGreaterThan(0) // Ensure store was called at least once.
@@ -788,8 +812,11 @@ describe("ProviderSettingsManager", () => {
788812
it("should throw error if secrets storage fails", async () => {
789813
mockSecrets.get.mockRejectedValue(new Error("Storage failed"))
790814

791-
await expect(providerSettingsManager.hasConfig("test")).rejects.toThrow(
792-
"Failed to check config existence: Error: Failed to read provider profiles from secrets: Error: Storage failed",
815+
// Create a new instance that will fail during initialization
816+
const failingManager = new ProviderSettingsManager(mockContext)
817+
818+
await expect(failingManager.hasConfig("test")).rejects.toThrow(
819+
"Failed to initialize config: Error: Failed to read provider profiles from secrets: Error: Storage failed",
793820
)
794821
})
795822
})

0 commit comments

Comments
 (0)