Skip to content

Fix file watcher robustness: dir watching, cleanup, suppression #124

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

Merged
siddseethepalli merged 1 commit into from
Feb 8, 2026
Merged

Fix file watcher robustness: dir watching, cleanup, suppression #124

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
98 changes: 58 additions & 40 deletions assistant/src/daemon/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ export class DaemonServer {
private socketToSession = new Map<net.Socket, string>();
private socketPath: string;
private watchers: FSWatcher[] = [];
private debounceTimers: ReturnType<typeof setTimeout>[] = [];
private suppressConfigReload = false;

constructor() {
this.socketPath = getSocketPath();
Expand Down Expand Up @@ -83,53 +85,63 @@ export class DaemonServer {

private startFileWatchers(): void {
const dataDir = getDataDir();
const configPath = join(dataDir, 'config.json');
const trustPath = join(dataDir, 'trust.json');

const watchFile = (filePath: string, label: string, onChange: () => void) => {
if (!existsSync(filePath)) return;
try {
let debounceTimer: ReturnType<typeof setTimeout> | null = null;
const watcher = watch(filePath, () => {
// Debounce: editors often write files in multiple steps
if (debounceTimer) clearTimeout(debounceTimer);
debounceTimer = setTimeout(() => {
log.info({ file: label }, 'File changed, reloading');
onChange();
}, 200);
});
this.watchers.push(watcher);
log.info({ file: label }, 'Watching for changes');
} catch (err) {
log.warn({ err, file: label }, 'Failed to watch file');
}
// Watch the data directory instead of individual files so we detect:
// - Files that don't exist yet at startup (new config/trust creation)
// - Atomic rename writes (trust-store uses renameSync)
const handlers: Record<string, () => void> = {
'config.json': () => {
if (this.suppressConfigReload) return;
invalidateConfigCache();
try {
const config = getConfig();
initializeProviders(config);
} catch (err) {
log.error({ err }, 'Failed to reload config');
return;
}
// Evict idle sessions; mark busy ones as stale
for (const [id, session] of this.sessions) {
if (!session.isProcessing()) {
this.sessions.delete(id);
} else {
session.markStale();
}
}
},
'trust.json': () => {
clearTrustCache();
},
};

watchFile(configPath, 'config.json', () => {
invalidateConfigCache();
try {
const config = getConfig();
initializeProviders(config);
} catch (err) {
log.error({ err }, 'Failed to reload config');
return;
}
// Evict idle sessions; mark busy ones as stale
for (const [id, session] of this.sessions) {
if (!session.isProcessing()) {
this.sessions.delete(id);
} else {
session.markStale();
}
}
});
const debounceTimers = new Map<string, ReturnType<typeof setTimeout>>();

watchFile(trustPath, 'trust.json', () => {
clearTrustCache();
});
try {
const watcher = watch(dataDir, (_eventType, filename) => {
if (!filename || !handlers[filename]) return;
// Debounce: editors often write files in multiple steps
const existing = debounceTimers.get(filename);
if (existing) clearTimeout(existing);
const timer = setTimeout(() => {
debounceTimers.delete(filename);
log.info({ file: filename }, 'File changed, reloading');
handlers[filename]();
}, 200);
debounceTimers.set(filename, timer);
this.debounceTimers.push(timer);
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot Feb 8, 2026

Choose a reason for hiding this comment

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

🟡 debounceTimers array grows without bound (memory leak)

Every file-change event pushes a new timer to this.debounceTimers (line 131), but completed or replaced timers are never removed from the array. The local debounceTimers Map correctly deduplicates per filename, but the instance-level array only accumulates.

Root Cause and Impact

When a watched file changes, the handler at assistant/src/daemon/server.ts:125-131 creates a new setTimeout and pushes it to this.debounceTimers. If the same file changes again within the debounce window, the old timer is cleared via the local Map (debounceTimers.delete / clearTimeout), but its reference remains in this.debounceTimers. Over the lifetime of a long-running daemon, every single fs event adds an entry:

const timer = setTimeout(() => {
  debounceTimers.delete(filename);
  log.info({ file: filename }, 'File changed, reloading');
  handlers[filename]();
}, 200);
debounceTimers.set(filename, timer);  // local Map — properly replaces
this.debounceTimers.push(timer);       // instance array — only grows

Additionally, stopFileWatchers() calls clearTimeout on every entry including already-fired timers (harmless but wasteful), and the array is only cleared on stop(). For a daemon that runs for days/weeks, this is an unbounded memory leak proportional to the number of file-change events received.

Impact: Memory leak in long-running daemon processes. Each fs event leaks a timer ID reference that is never reclaimed until server stop.

Prompt for agents 
Replace the this.debounceTimers array with a different approach. The simplest fix is to store the local debounceTimers Map as an instance field (e.g. this.debounceTimers = new Map<string, ReturnType<typeof setTimeout>>()) instead of using both a local Map and an instance array. Then in stopFileWatchers(), iterate the Map values to clearTimeout, and clear the Map. This eliminates the unbounded growth since the Map naturally deduplicates by filename. Specifically:

1. Change the class field from `private debounceTimers: ReturnType<typeof setTimeout>[] = []` to `private debounceTimers = new Map<string, ReturnType<typeof setTimeout>>()`
2. In startFileWatchers(), remove the local `debounceTimers` Map and use `this.debounceTimers` directly
3. In stopFileWatchers(), change to: `for (const timer of this.debounceTimers.values()) { clearTimeout(timer); } this.debounceTimers.clear();`
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

});
this.watchers.push(watcher);
log.info({ dir: dataDir }, 'Watching data directory for config/trust changes');
} catch (err) {
log.warn({ err }, 'Failed to watch data directory');
}
}

private stopFileWatchers(): void {
for (const timer of this.debounceTimers) {
clearTimeout(timer);
}
this.debounceTimers = [];
for (const watcher of this.watchers) {
watcher.close();
}
Expand Down Expand Up @@ -351,7 +363,13 @@ export class DaemonServer {
// Use raw config to avoid persisting env-var API keys to disk
const raw = loadRawConfig();
raw.model = model;

// Suppress the file watcher callback — handleModelSet already does
// the full reload sequence; a redundant watcher-triggered reload
// would incorrectly evict sessions created after this method returns.
this.suppressConfigReload = true;
saveRawConfig(raw);
Comment on lines +370 to 371
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot Feb 8, 2026

Choose a reason for hiding this comment

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

P2 Badge Reset reload suppression on config write failure

handleModelSet sets suppressConfigReload = true before calling saveRawConfig, but the reset is only scheduled afterward; if saveRawConfig throws (for example due to permissions or disk errors), the flag stays true indefinitely and later config.json watcher events are ignored by the early return in the watcher handler. That leaves hot-reload effectively disabled until daemon restart after a single failed model update.

Useful? React with 👍 / 👎.

setTimeout(() => { this.suppressConfigReload = false; }, 300);
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot Feb 8, 2026

Choose a reason for hiding this comment

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

🟡 suppressConfigReload reset timer not tracked for cleanup on stop

The setTimeout at line 372 that resets suppressConfigReload to false is not added to this.debounceTimers, so it is not cancelled when stopFileWatchers() is called during server shutdown.

Detailed Explanation

In handleModelSet at assistant/src/daemon/server.ts:370-372:

this.suppressConfigReload = true;
saveRawConfig(raw);
setTimeout(() => { this.suppressConfigReload = false; }, 300);

This PR specifically aims to fix use-after-teardown issues from timers firing after stopFileWatchers(). The debounce timers in startFileWatchers are tracked and cancelled on stop, but this 300ms timer in handleModelSet is not tracked at all. If stop() is called within 300ms of a model_set command, this timer fires after teardown.

The practical impact is minimal (it just sets a boolean to false on a stopped server), but it contradicts the PR's own stated goal and pattern. More importantly, if the server were ever restarable or if future code adds teardown logic that depends on timer quiescence, this would be a real problem.

Impact: Minor inconsistency — timer fires after server stop, but only sets a boolean with no side effects on a stopped server.

Suggested change
setTimeout(() => { this.suppressConfigReload = false; }, 300);
const resetTimer = setTimeout(() => { this.suppressConfigReload = false; }, 300);
this.debounceTimers.push(resetTimer);
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.


// Re-initialize provider with the new model so LLM calls use it
const config = getConfig();
Expand Down
Loading