Tracing a memory leak in Solid Queue workers

Memory Leak Analysis - Stimulus Controllers

Summary

Analysis of memory leak issues in Stimulus controllers, particularly related to event listener cleanup and timer management when navigating with Turbo.

🔴 Critical Issues Found

1. password_validator_controller.js - Event Listener Memory Leak

Status: ❌ CONFIRMED MEMORY LEAK

Location: Lines 49-60 (disconnect), 62-70 (setupPasswordToggle), 72-81 (setupFormSubmission)

Problem:

• Event listeners are added with .bind(this) in connect() and setupPasswordToggle()
• In disconnect(), new .bind(this) references are created, which don’t match the original listeners
• Result: Event listeners are never removed, causing memory leaks on Turbo navigation

Current Code:

// connect() calls setupPasswordToggle() and setupFormSubmission()
setupPasswordToggle() {
  this.toggleButtonTarget.addEventListener('click', this.togglePasswordVisibility.bind(this))
}

setupFormSubmission() {
  form.addEventListener('submit', this.handleFormSubmit.bind(this))
}

disconnect() {
  // ❌ These won't work - new function references!
  this.toggleButtonTarget.removeEventListener('click', this.togglePasswordVisibility.bind(this))
  form.removeEventListener('submit', this.handleFormSubmit.bind(this))
}

Impact:

• Memory leaks accumulate on every Turbo navigation
• Event listeners fire multiple times (one per navigation)
• Performance degradation over time

2. confetti_controller.js - setInterval Not Cleared

Status: ⚠️ POTENTIAL MEMORY LEAK

Location: Line 50 (setInterval)

Problem:

• setInterval is created in fire() method
• Interval is only cleared inside its own callback (when timeLeft <= 0)
• If controller disconnects before interval completes, interval continues running
• No reference stored to clear it in disconnect()

Current Code:

connect() {
  setTimeout(() => {
    this.fire()  // Creates setInterval
  }, 300)
}

fire() {
  const interval = setInterval(() => {
    // ... confetti logic
    if (timeLeft <= 0) {
      return clearInterval(interval)  // Only cleared here
    }
  }, 250)
  // ❌ No reference stored, can't clear in disconnect()
}

Impact:

• Interval continues running after controller disconnects
• Memory leak if user navigates away quickly
• Unnecessary CPU usage

3. rate_limit_controller.js - setTimeout Not Cleared

Status: ⚠️ POTENTIAL MEMORY LEAK

Location: Line 125 (setTimeout in scheduleReEnable)

Problem:

• setTimeout is created but never stored
• If controller disconnects before timeout fires, it still executes
• Could cause errors if controller is disconnected

Current Code:

scheduleReEnable() {
  setTimeout(() => {
    this.enableForm()  // ❌ May execute after disconnect
    // ...
  }, this.retryAfterValue * 1000)
  // No reference stored to clear it
}

Impact:

• Timeout may execute after controller disconnects
• Potential errors accessing this.enableForm() on disconnected controller
• Memory leak (timeout reference not cleared)

✅ Correctly Implemented Controllers

autosave_controller.js - ✅ Properly Handled

• Stores bound handlers: this.boundScheduleAutosave, this.lexxyChangeHandler, etc.
• Properly removes listeners in disconnect()
• Clears timeouts correctly

modal_controller.js - ✅ Properly Handled

• Stores bound handler: this.boundAnchorHandler
• Properly removes listener in disconnect()

📋 Controllers Without Issues

These controllers don’t add event listeners or timers:

• link_unfurl_controller.js - Uses data-action attributes
• unsplash_picker_controller.js - Uses data-action attributes
• flash_injector_controller.js - Only DOM manipulation in connect()
• tabs_controller.js - Only Bootstrap initialization
• syntax_highlight_controller.js - Only DOM manipulation

🔧 Recommended Fixes

Fix 1: password_validator_controller.js

Store bound handlers in connect() and use them in disconnect():

connect() {
  // Store bound handlers
  this.boundTogglePasswordVisibility = this.togglePasswordVisibility.bind(this)
  this.boundHandleFormSubmit = this.handleFormSubmit.bind(this)
  
  // ... rest of connect logic
}

setupPasswordToggle() {
  if (this.hasToggleButtonTarget) {
    this.toggleButtonTarget.addEventListener('click', this.boundTogglePasswordVisibility)
  }
}

setupFormSubmission() {
  const form = this.element.querySelector('form')
  if (form) {
    form.addEventListener('submit', this.boundHandleFormSubmit)
  }
}

disconnect() {
  if (this.hasToggleButtonTarget && this.boundTogglePasswordVisibility) {
    this.toggleButtonTarget.removeEventListener('click', this.boundTogglePasswordVisibility)
  }
  
  const form = this.element.querySelector('form')
  if (form && this.boundHandleFormSubmit) {
    form.removeEventListener('submit', this.boundHandleFormSubmit)
  }
  
  this.isSubmitting = false
}

Fix 2: confetti_controller.js

Store interval reference and clear in disconnect:

connect() {
  this.confettiTimeout = setTimeout(() => {
    this.fire()
  }, 300)
}

disconnect() {
  if (this.confettiTimeout) {
    clearTimeout(this.confettiTimeout)
    this.confettiTimeout = null
  }
  if (this.confettiInterval) {
    clearInterval(this.confettiInterval)
    this.confettiInterval = null
  }
}

fire() {
  // ... existing code ...
  this.confettiInterval = setInterval(() => {
    // ... existing logic ...
  }, 250)
}

Fix 3: rate_limit_controller.js

Store timeout reference, clear in disconnect, and add disconnection flag to prevent closure memory leaks:

connect() {
  // Reset disconnection flag when controller connects
  this.isDisconnected = false
  
  // ... rest of connect logic
}

disconnect() {
  if (this.reEnableTimeout) {
    clearTimeout(this.reEnableTimeout)
    this.reEnableTimeout = null
  }
  // Mark as disconnected for any pending callbacks to prevent memory leaks
  // Even if clearTimeout is called, the closure may still hold a reference
  this.isDisconnected = true
}

scheduleReEnable() {
  if (this.retryAfterValue > 0) {
    this.reEnableTimeout = setTimeout(() => {
      // Double-check disconnection state to prevent memory leaks
      // Even if clearTimeout is called, the closure may still hold a reference
      if (this.isDisconnected || !this.element || !this.hasErrorContainerTarget) {
        return
      }
      
      this.enableForm()
      // ... rest of logic ...
      
      this.reEnableTimeout = null
    }, this.retryAfterValue * 1000)
  }
}

Why the isDisconnected flag?

• Even after clearTimeout() is called, the closure created by the arrow function still holds a reference to this (the controller instance)
• In race conditions where disconnect() is called but the timeout callback hasn’t been garbage collected yet, the closure prevents the controller from being fully released
• The isDisconnected flag ensures the callback exits early and doesn’t access controller properties, allowing garbage collection

📊 Impact Assessment

✅ Verification Checklist

After fixes:

• All event listeners use stored bound handlers
• All removeEventListener calls use the same reference as addEventListener
• All setTimeout/setInterval references are stored
• All timers are cleared in disconnect()
• Disconnection flags added for timeout callbacks to prevent closure memory leaks
• Test with Turbo navigation to verify cleanup