mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-09-07 08:26:51 +00:00
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
This commit is contained in:
parent
13534cc8b7
commit
a0a75fe5da
2 changed files with 103 additions and 22 deletions
|
|
@ -74,12 +74,15 @@ export class ProviderSettingsManager {
|
|||
}
|
||||
|
||||
private readonly context: ExtensionContext
|
||||
private initializationPromise: Promise<void> | 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<void> {
|
||||
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<void> {
|
||||
const timeoutPromise = new Promise<never>((_, 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<ProviderSettingsEntry[]> {
|
||||
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<string> {
|
||||
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<ProviderSettingsWithId & { name: string }> {
|
||||
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<ProviderSettingsWithId & { name: string }> {
|
||||
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<string, ProviderSettingsWithId>,
|
||||
currentActiveProfileName?: string,
|
||||
): Promise<SyncCloudProfilesResult> {
|
||||
await this.ensureInitialized()
|
||||
try {
|
||||
return await this.lock(async () => {
|
||||
const providerProfiles = await this.load()
|
||||
|
|
|
|||
|
|
@ -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",
|
||||
)
|
||||
})
|
||||
})
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue