Skip to content

Fix debounce timer memory leak and suppress timer cleanup#127

Merged
siddseethepalli merged 1 commit into
mainfrom
fix/file-watcher-timer-cleanup
Feb 8, 2026
Merged

Fix debounce timer memory leak and suppress timer cleanup#127
siddseethepalli merged 1 commit into
mainfrom
fix/file-watcher-timer-cleanup

Conversation

@siddseethepalli
Copy link
Copy Markdown
Contributor

@siddseethepalli siddseethepalli commented Feb 8, 2026

Summary

  • Replace debounceTimers array with a Map<string, Timer> instance field — the Map naturally deduplicates by filename, preventing unbounded growth from repeated file-change events
  • Track the suppressConfigReload reset timer in the Map so it gets cancelled on server stop
  • Reset suppressConfigReload immediately if saveRawConfig throws, preventing hot-reload from staying permanently disabled after a failed model update
  • Remove unused join import

Context

Addresses all 3 review comments from Devin and Codex on #124:

  1. Memory leak (Devin): this.debounceTimers.push(timer) accumulated timer refs without cleanup — replaced array with Map instance field that naturally deduplicates
  2. Untracked timer (Devin): The 300ms suppressConfigReload reset timer wasn't tracked for cancellation on server stop — now stored in the debounce Map
  3. Permanent suppression on failure (Codex): If saveRawConfig threw, suppressConfigReload stayed true forever — now wrapped in try/catch that resets the flag on failure

Test plan

  • npx tsc --noEmit passes
  • bun test passes (212 tests)
  • Manual: verify config hot-reload still works after model set
  • Manual: verify daemon stop doesn't leave stale timers

🤖 Generated with Claude Code


Open with Devin

Address review feedback on PR #124:
- Replace debounceTimers array with Map instance field to prevent
  unbounded growth — the Map naturally deduplicates by filename
- Track the suppressConfigReload reset timer in the Map so it gets
  cancelled on server stop
- Reset suppressConfigReload immediately if saveRawConfig throws,
  preventing hot-reload from staying permanently disabled
- Remove unused `join` import

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@siddseethepalli siddseethepalli merged commit 13643be into main Feb 8, 2026
@siddseethepalli siddseethepalli deleted the fix/file-watcher-timer-cleanup branch February 8, 2026 22:50
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 1 potential issue.

View 5 additional findings in Devin Review.

Open in Devin Review

Comment on lines +377 to +378
const resetTimer = setTimeout(() => { this.suppressConfigReload = false; }, 300);
this.debounceTimers.set('__suppress_reset__', resetTimer);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Missing clearTimeout before overwriting __suppress_reset__ timer in debounceTimers map

When handleModelSet is called multiple times in quick succession, the previous __suppress_reset__ timer is overwritten in the map without being cancelled first. The orphaned timer still fires and prematurely resets suppressConfigReload to false, allowing the file watcher to trigger a redundant config reload that evicts sessions.

Root Cause

At assistant/src/daemon/server.ts:377-378, a new timer is created and stored in the map:

const resetTimer = setTimeout(() => { this.suppressConfigReload = false; }, 300);
this.debounceTimers.set('__suppress_reset__', resetTimer);

But unlike the file watcher debounce code at assistant/src/daemon/server.ts:121-122 which properly clears existing timers:

const existing = this.debounceTimers.get(filename);
if (existing) clearTimeout(existing);

…the handleModelSet method does not clear the previous __suppress_reset__ timer before setting a new one.

Scenario:

  1. First handleModelSet call: sets suppressConfigReload = true, creates timer A (300ms), stores as __suppress_reset__
  2. Second handleModelSet call (within 300ms): sets suppressConfigReload = true, creates timer B (300ms), overwrites timer A in the map
  3. Timer A fires (~300ms after first call): sets suppressConfigReload = false prematurely
  4. File watcher event from the second write arrives: suppressConfigReload is already false, so it triggers a redundant reload that evicts sessions

Impact: Rapid model changes can cause unexpected session eviction due to the suppression window ending too early.

Suggested change
const resetTimer = setTimeout(() => { this.suppressConfigReload = false; }, 300);
this.debounceTimers.set('__suppress_reset__', resetTimer);
const existingSuppressTimer = this.debounceTimers.get('__suppress_reset__');
if (existingSuppressTimer) clearTimeout(existingSuppressTimer);
const resetTimer = setTimeout(() => { this.suppressConfigReload = false; }, 300);
this.debounceTimers.set('__suppress_reset__', resetTimer);
Open in Devin Review

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant