Skip to content

Commit 7506223

Browse files
authored
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
1 parent ae6c388 commit 7506223

1 file changed

Lines changed: 24 additions & 6 deletions

File tree

src/settings.ts

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,9 @@ export class SettingsManager {
9292
private onChange?: () => void;
9393
private logger: { log: (...args: any[]) => void; error: (...args: any[]) => void };
9494
private initialized = false;
95+
private disposed = false;
9596
private debounceTimer: ReturnType<typeof setTimeout> | null = null;
97+
private initPromise: Promise<void> | null = null;
9698

9799
constructor(cwd: string, options?: SettingsManagerOptions) {
98100
this.cwd = cwd;
@@ -107,10 +109,19 @@ export class SettingsManager {
107109
if (this.initialized) {
108110
return;
109111
}
112+
if (this.initPromise) {
113+
return this.initPromise;
114+
}
110115

111-
await this.loadAllSettings();
112-
this.setupWatchers();
113-
this.initialized = true;
116+
this.disposed = false;
117+
this.initPromise = this.loadAllSettings().then(() => {
118+
if (!this.disposed) {
119+
this.setupWatchers();
120+
this.initialized = true;
121+
}
122+
this.initPromise = null;
123+
});
124+
return this.initPromise;
114125
}
115126

116127
/**
@@ -235,9 +246,14 @@ export class SettingsManager {
235246

236247
this.debounceTimer = setTimeout(async () => {
237248
this.debounceTimer = null;
249+
if (this.disposed) {
250+
return;
251+
}
238252
try {
239253
await this.loadAllSettings();
240-
this.onChange?.();
254+
if (!this.disposed) {
255+
this.onChange?.();
256+
}
241257
} catch (error) {
242258
this.logger.error("Failed to reload settings:", error);
243259
}
@@ -268,14 +284,17 @@ export class SettingsManager {
268284

269285
this.dispose();
270286
this.cwd = cwd;
271-
this.initialized = false;
272287
await this.initialize();
273288
}
274289

275290
/**
276291
* Disposes of file watchers and cleans up resources
277292
*/
278293
dispose(): void {
294+
this.disposed = true;
295+
this.initialized = false;
296+
this.initPromise = null;
297+
279298
if (this.debounceTimer) {
280299
clearTimeout(this.debounceTimer);
281300
this.debounceTimer = null;
@@ -285,6 +304,5 @@ export class SettingsManager {
285304
watcher.close();
286305
}
287306
this.watchers = [];
288-
this.initialized = false;
289307
}
290308
}

0 commit comments

Comments
 (0)