acme / dashboard · Issues
bug high priority websocket

Memory leak in WebSocket reconnection logic #1247

marcusw opened this issue 2 days ago · 5 comments
marcusw commented 2 days ago

Describe the bug

The real-time dashboard WebSocket client has a memory leak when the connection is interrupted and reconnects multiple times. Memory usage increases by ~20MB per reconnection and is never garbage collected.

To Reproduce

  1. Open the dashboard
  2. Open Chrome DevTools → Performance → Memory
  3. Simulate network interruption (Chrome DevTools → Network → Offline)
  4. Re-enable network
  5. Repeat steps 3-4 ten times
  6. Observe memory heap growing from ~50MB to 250MB+

Expected behavior

Memory should stabilize after reconnection. Old WebSocket instances and event listeners should be properly cleaned up.

Screenshots

(Chrome DevTools memory profile showing heap growth attached)

Environment:

  • Browser: Chrome 127.0.6533.72
  • OS: macOS 14.5
  • Dashboard version: v3.2.1
sarahchen commented 2 days ago · Maintainer

Thanks for the detailed report! I can reproduce this. Looking at the heap snapshot, it seems we're not removing event listeners on the old WebSocket instance before creating a new one.

The issue is in src/websocket/client.ts:45-58. We need to call ws.removeAllListeners() before reassigning.

marcusw commented 1 day ago

Good catch! I took a quick look and here's the problematic code:

private reconnect() {
  console.log('Reconnecting WebSocket...');
  this.ws = new WebSocket(this.url);
  this.setupListeners(); // <-- Adds new listeners without removing old ones
}

private setupListeners() {
  this.ws.addEventListener('open', this.onOpen);
  this.ws.addEventListener('message', this.onMessage);
  this.ws.addEventListener('error', this.onError);
  this.ws.addEventListener('close', this.onClose);
}

Should I open a PR with the fix? I'm thinking:

private reconnect() {
  if (this.ws) {
    this.ws.removeEventListener('open', this.onOpen);
    this.ws.removeEventListener('message', this.onMessage);
    this.ws.removeEventListener('error', this.onError);
    this.ws.removeEventListener('close', this.onClose);
    this.ws.close();
  }
  this.ws = new WebSocket(this.url);
  this.setupListeners();
}
sarahchen commented 1 day ago · Maintainer

Yes, that would fix it! Though we should also clear the reconnection timeout to avoid multiple concurrent reconnect attempts. Can you add:

if (this.reconnectTimeout) {
  clearTimeout(this.reconnectTimeout);
  this.reconnectTimeout = null;
}

Also, please add a test case that simulates multiple reconnections and asserts that event listeners don't accumulate.

marcusw commented 1 day ago

Will do! Opening PR shortly. Should I target main or develop?

sarahchen commented 22 hours ago · Maintainer

Target develop — we'll cherry-pick to main for the patch release. Thanks for jumping on this!