Skip to content

fix: add timeout protection to ProviderSettingsManager initialization #8463

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

fix: add timeout protection to ProviderSettingsManager initialization #8463

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 58 additions & 4 deletions src/core/config/ProviderSettingsManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor Author

@roomote roomote bot Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Duplicate error logging and swallowed rejection. initializeWithTimeout already logs the failure; catching here logs again and swallows the rejection. Consider no-op catch to avoid duplicate logs while preventing unhandled rejection warnings.

Suggested change
console.error("ProviderSettingsManager initialization failed:", error)
this.ensureInitialized().catch(() => {
// already logged inside initializeWithTimeout
})

})
}

public generateId() {
Expand All @@ -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
}
}
Comment on lines +121 to +138
Copy link
Contributor Author

@roomote roomote bot Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Timeout promise can cause unhandled rejection and isInitialized is set true on error, allowing calls to bypass init. Suggest clearing the timer and not marking initialized on failure.

Suggested change
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
}
}
let timer: ReturnType<typeof setTimeout> | undefined
const timeoutPromise = new Promise<never>((_, reject) => {
timer = 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)
throw error
} finally {
if (timer) clearTimeout(timer)
this.initializationPromise = null
}


/**
* 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()
Expand Down Expand Up @@ -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()
Expand All @@ -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()
Expand All @@ -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()
Expand Down Expand Up @@ -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 {
Expand All @@ -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()
Expand All @@ -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()
Expand All @@ -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()
Expand All @@ -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()
Expand All @@ -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())
Expand All @@ -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) {
Expand Down Expand Up @@ -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()
Expand Down
63 changes: 45 additions & 18 deletions src/core/config/__tests__/ProviderSettingsManager.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -36,14 +36,16 @@ describe("ProviderSettingsManager", () => {
mockGlobalState.update.mockResolvedValue(undefined)

providerSettingsManager = new ProviderSettingsManager(mockContext)
// Wait for initialization to complete in tests
await providerSettingsManager.ensureInitialized()
})

describe("initialize", () => {
it("should not write to storage when secrets.get returns null", async () => {
// 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()
Expand Down Expand Up @@ -72,7 +74,7 @@ describe("ProviderSettingsManager", () => {
}),
)

await providerSettingsManager.initialize()
await providerSettingsManager.ensureInitialized()

expect(mockSecrets.store).not.toHaveBeenCalled()
})
Expand All @@ -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()
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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(
Expand All @@ -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
Expand All @@ -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",
)
})
Expand Down Expand Up @@ -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",
)
})
})
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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",
)
})
})
Expand Down
Loading