mirror of
https://github.com/agentclientprotocol/claude-agent-acp.git
synced 2026-10-27 12:40:32 +00:00
fix: prevent race conditions in SettingsManager setCwd and debounce (#485)
## Summary - **Concurrent `setCwd()` calls** could produce duplicate file watchers because both calls cleared `initialized` before either completed `initialize()`. Added an `initPromise` guard so concurrent calls await the same initialization. - **Debounce timer callback** could fire after `dispose()`, reloading stale settings and calling `onChange` on a disposed manager. Added a `disposed` flag checked in the async callback before proceeding. Follows up on #454 which fixed the missing `dispose()` calls — this PR hardens the `SettingsManager` lifecycle against race conditions. ## Test plan - [x] `npm run lint` passes - [x] All settings tests pass - [x] Verified `disposed` flag prevents stale callback execution - [x] Verified `initPromise` deduplicates concurrent `initialize()` calls
This commit is contained in:
@@ -92,7 +92,9 @@ export class SettingsManager {
|
||||
private onChange?: () => void;
|
||||
private logger: { log: (...args: any[]) => void; error: (...args: any[]) => void };
|
||||
private initialized = false;
|
||||
private disposed = false;
|
||||
private debounceTimer: ReturnType<typeof setTimeout> | null = null;
|
||||
private initPromise: Promise<void> | null = null;
|
||||
|
||||
constructor(cwd: string, options?: SettingsManagerOptions) {
|
||||
this.cwd = cwd;
|
||||
@@ -107,10 +109,19 @@ export class SettingsManager {
|
||||
if (this.initialized) {
|
||||
return;
|
||||
}
|
||||
if (this.initPromise) {
|
||||
return this.initPromise;
|
||||
}
|
||||
|
||||
await this.loadAllSettings();
|
||||
this.setupWatchers();
|
||||
this.initialized = true;
|
||||
this.disposed = false;
|
||||
this.initPromise = this.loadAllSettings().then(() => {
|
||||
if (!this.disposed) {
|
||||
this.setupWatchers();
|
||||
this.initialized = true;
|
||||
}
|
||||
this.initPromise = null;
|
||||
});
|
||||
return this.initPromise;
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -235,9 +246,14 @@ export class SettingsManager {
|
||||
|
||||
this.debounceTimer = setTimeout(async () => {
|
||||
this.debounceTimer = null;
|
||||
if (this.disposed) {
|
||||
return;
|
||||
}
|
||||
try {
|
||||
await this.loadAllSettings();
|
||||
this.onChange?.();
|
||||
if (!this.disposed) {
|
||||
this.onChange?.();
|
||||
}
|
||||
} catch (error) {
|
||||
this.logger.error("Failed to reload settings:", error);
|
||||
}
|
||||
@@ -268,7 +284,6 @@ export class SettingsManager {
|
||||
|
||||
this.dispose();
|
||||
this.cwd = cwd;
|
||||
this.initialized = false;
|
||||
await this.initialize();
|
||||
}
|
||||
|
||||
@@ -276,6 +291,10 @@ export class SettingsManager {
|
||||
* Disposes of file watchers and cleans up resources
|
||||
*/
|
||||
dispose(): void {
|
||||
this.disposed = true;
|
||||
this.initialized = false;
|
||||
this.initPromise = null;
|
||||
|
||||
if (this.debounceTimer) {
|
||||
clearTimeout(this.debounceTimer);
|
||||
this.debounceTimer = null;
|
||||
@@ -285,6 +304,5 @@ export class SettingsManager {
|
||||
watcher.close();
|
||||
}
|
||||
this.watchers = [];
|
||||
this.initialized = false;
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user