fix(selection): apply identity-check pattern to demotion timer

Extracts _cancel_demotion_timer() mirroring _cancel_debounce_timer() and
applies the same identity-check guard to the demotion timer callback so
late-firing libuv callbacks cannot mutate state after the timer was
cancelled (e.g. by disable() or by being replaced).

Also adds defense-in-depth by gating handle_selection_demotion() on
tracking_enabled, switches the demotion timer to the uv alias, and adds
a regression test that exercises a stale demotion callback firing after
disable().

Change-Id: Ifce08a736d550605ce99a697a187deecf62768a1
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Thomas Kosiewski <tk@coder.com>

Author: ThomasK33

lua/claudecode/selection.lua

@@ -34,7 +34,8 @@ function M.enable(server, visual_demotion_delay_ms)
 end
 
 ---Disables selection tracking.
----Clears autocommands, resets internal state, and stops any active debounce timers.
+---Clears autocommands, resets internal state, and stops any active debounce or
+---demotion timers.
 function M.disable()
   if not M.state.tracking_enabled then
     return
@@ -48,14 +49,7 @@ function M.disable()
   M.server = nil
 
   M._cancel_debounce_timer()
-
-  if M.state.demotion_timer then
-    local demotion_timer = M.state.demotion_timer
-    M.state.demotion_timer = nil
-
-    demotion_timer:stop()
-    demotion_timer:close()
-  end
+  M._cancel_demotion_timer()
 end
 
 ---Cancels and closes the current debounce timer, if any.
@@ -69,8 +63,20 @@ function M._cancel_debounce_timer()
   -- Clear state before stopping/closing so any already-scheduled callback is a no-op.
   M.state.debounce_timer = nil
 
-  assert(timer.stop, "Expected debounce timer to have :stop()")
-  assert(timer.close, "Expected debounce timer to have :close()")
+  timer:stop()
+  timer:close()
+end
+
+---Cancels and closes the current demotion timer, if any.
+---@local
+function M._cancel_demotion_timer()
+  local timer = M.state.demotion_timer
+  if not timer then
+    return
+  end
+
+  -- Clear state before stopping/closing so any already-scheduled callback is a no-op.
+  M.state.demotion_timer = nil
 
   timer:stop()
   timer:close()
@@ -153,7 +159,7 @@ function M.debounce_update()
         return
       end
 
-      -- Clear state before stopping/closing so cancellation is idempotent.
+      -- Clear state so _cancel_debounce_timer() is a no-op if called after firing.
       M.state.debounce_timer = nil
 
       timer:stop()
@@ -178,11 +184,7 @@ function M.update_selection()
   -- If the buffer name starts with "term://" and contains "claude", do not update selection
   if buf_name and buf_name:match("^term://") and buf_name:lower():find("claude", 1, true) then
     -- Optionally, cancel demotion timer like for the terminal
-    if M.state.demotion_timer then
-      M.state.demotion_timer:stop()
-      M.state.demotion_timer:close()
-      M.state.demotion_timer = nil
-    end
+    M._cancel_demotion_timer()
     return
   end
 
@@ -191,11 +193,7 @@ function M.update_selection()
     local claude_term_bufnr = terminal.get_active_terminal_bufnr()
     if claude_term_bufnr and current_buf == claude_term_bufnr then
       -- Cancel any pending demotion if we switch to the Claude terminal
-      if M.state.demotion_timer then
-        M.state.demotion_timer:stop()
-        M.state.demotion_timer:close()
-        M.state.demotion_timer = nil
-      end
+      M._cancel_demotion_timer()
       return
     end
   end
@@ -206,11 +204,7 @@ function M.update_selection()
 
   if current_mode == "v" or current_mode == "V" or current_mode == "\022" then
     -- If a new visual selection is made, cancel any pending demotion
-    if M.state.demotion_timer then
-      M.state.demotion_timer:stop()
-      M.state.demotion_timer:close()
-      M.state.demotion_timer = nil
-    end
+    M._cancel_demotion_timer()
 
     current_selection = M.get_visual_selection()
 
@@ -246,21 +240,25 @@ function M.update_selection()
       -- The 'current_selection' for comparison should also be this visual one.
       current_selection = M.state.latest_selection
 
-      if M.state.demotion_timer then -- Should not happen due to elseif, but as safeguard
-        M.state.demotion_timer:stop()
-        M.state.demotion_timer:close()
-      end
+      local timer = uv.new_timer()
+      assert(timer, "Expected uv.new_timer() to return a timer handle")
 
-      M.state.demotion_timer = vim.loop.new_timer()
-      M.state.demotion_timer:start(
+      M.state.demotion_timer = timer
+      timer:start(
         M.state.visual_demotion_delay_ms,
         0, -- 0 repeat = one-shot
         vim.schedule_wrap(function()
-          if M.state.demotion_timer then -- Check if it wasn't cancelled right before firing
-            M.state.demotion_timer:stop()
-            M.state.demotion_timer:close()
-            M.state.demotion_timer = nil
+          -- Ignore stale timers (e.g., cancelled and replaced before callback runs)
+          if M.state.demotion_timer ~= timer then
+            return
           end
+
+          -- Clear state so _cancel_demotion_timer() is a no-op if called after firing.
+          M.state.demotion_timer = nil
+
+          timer:stop()
+          timer:close()
+
           M.handle_selection_demotion(current_buf) -- Pass buffer at time of scheduling
         end)
       )
@@ -296,6 +294,10 @@ function M.handle_selection_demotion(original_bufnr_when_scheduled)
   -- Timer object is already stopped and cleared by its own callback wrapper or cancellation points.
   -- M.state.demotion_timer should be nil here if it fired normally or was cancelled.
 
+  if not M.state.tracking_enabled then
+    return
+  end
+
   local current_buf = vim.api.nvim_get_current_buf()
   local claude_term_bufnr = terminal.get_active_terminal_bufnr()
 

tests/selection_test.lua

@@ -233,12 +233,6 @@ if not _G.vim then
 
         return timer
       end,
-      timer_stop = function(timer)
-        if timer and timer.stop then
-          timer:stop()
-        end
-        return true
-      end,
     },
 
     test = { ---@type vim_test_utils
@@ -453,6 +447,9 @@ describe("Selection module", function()
       -- A callback from a cancelled timer should be ignored.
       timer1:fire()
       assert.are.equal(0, update_calls)
+      -- Stale callback must not double-stop or double-close the already-cancelled timer.
+      assert.are.equal(1, timer1._stop_calls)
+      assert.are.equal(1, timer1._close_calls)
 
       timer2:fire()
       assert.are.equal(1, update_calls)
@@ -476,6 +473,66 @@ describe("Selection module", function()
     end)
   end)
 
+  describe("demotion_timer", function()
+    local function install_terminal_stub()
+      local terminal_module = package.loaded["claudecode.terminal"]
+      local original_get = terminal_module and terminal_module.get_active_terminal_bufnr or nil
+      if not terminal_module then
+        terminal_module = {}
+        package.loaded["claudecode.terminal"] = terminal_module
+      end
+      terminal_module.get_active_terminal_bufnr = function()
+        return nil
+      end
+      return original_get, terminal_module
+    end
+
+    it("disable() should cancel an active demotion timer and ignore stale callbacks", function()
+      local original_get, terminal_module = install_terminal_stub()
+
+      selection.enable(mock_server)
+
+      -- Seed a non-empty visual selection so the demotion path triggers on normal-mode entry.
+      selection.state.last_active_visual_selection = {
+        bufnr = 1,
+        selection_data = {
+          text = "x",
+          filePath = "/path/to/test.lua",
+          fileUrl = "file:///path/to/test.lua",
+          selection = {
+            start = { line = 0, character = 0 },
+            ["end"] = { line = 0, character = 1 },
+            isEmpty = false,
+          },
+        },
+        timestamp = 0,
+      }
+      selection.state.latest_selection = selection.state.last_active_visual_selection.selection_data
+
+      _G.vim.test.set_mode("n")
+      selection.update_selection()
+
+      local timer = selection.state.demotion_timer
+      assert(timer ~= nil)
+
+      selection.disable()
+
+      assert(selection.state.demotion_timer == nil)
+      assert(selection.state.latest_selection == nil)
+      assert.are.equal(1, timer._stop_calls)
+      assert.are.equal(1, timer._close_calls)
+
+      -- A late-firing callback from the cancelled timer must not mutate state after teardown.
+      timer:fire()
+      assert(selection.state.latest_selection == nil)
+      assert(selection.state.demotion_timer == nil)
+      assert.are.equal(1, timer._stop_calls)
+      assert.are.equal(1, timer._close_calls)
+
+      terminal_module.get_active_terminal_bufnr = original_get
+    end)
+  end)
+
   it("should get cursor position in normal mode", function()
     local old_win_get_cursor = _G.vim.api.nvim_win_get_cursor
     _G.vim.api.nvim_win_get_cursor = function()