fix(selection): close debounce timers safely

[ThomasK33]

Fix debounce timer lifecycle: use uv timer API with proper stop/close

Replaces the vim.defer_fn-based debounce implementation with an explicit uv.new_timer() approach that correctly calls both :stop() and :close() on the timer handle. Previously, cancelled timers were only stopped but never closed, leaking libuv handles. Stale timer callbacks could also fire after being superseded by a newer debounce call.

Key changes:

• Introduces M._cancel_debounce_timer() to consistently stop and close the active debounce timer, clearing state before doing so to make cancellation idempotent
• The debounce callback now checks whether its timer is still the active one before proceeding, ignoring stale callbacks from replaced timers
• disable() also properly stops and closes the demotion timer
• Uses vim.uv or vim.loop for forward compatibility
• Expands the test mock to provide a full uv-compatible timer object with :start(), :stop(), :close(), and :fire() methods, along with new tests covering timer replacement, stale callback suppression, and disable() cleanup

Change-Id: I4caa6d010f8f824aff1c38a7a73d08d47b400cce
Signed-off-by: Thomas Kosiewski tk@coder.com

[ThomasK33]

• fix(selection): close debounce timers safely #245 👈 (View in Graphite)
• chore: add reviewfixer configs #244
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[ThomasK33]

/coder-agents-review

[coder-agents-review]

The lifecycle fix is well-motivated: vim.defer_fn manages its timer's close internally, and stopping the timer externally prevents the internal close, leaking the libuv handle. Switching to explicit uv.new_timer() with manual stop/close is the right call. The identity-based stale guard (M.state.debounce_timer ~= timer), nil-first cancellation ordering, and _cancel_debounce_timer() extraction are clean. Tests cover the three key scenarios with explicit call counts.

1 P2, 3 P3, 3 Nit, 3 Note.

The P2 is convergent across 6 reviewers: the demotion timer callback (selection.lua:259) has the same stale-callback vulnerability the PR fixes for the debounce timer. It uses a truthiness check instead of an identity check, and runs handle_selection_demotion() unconditionally outside the guard. The PR's new disable() cleanup creates a concrete interleaving where a stale vim.schedule_wrap callback fires handle_selection_demotion() after teardown, mutating latest_selection that disable() explicitly cleared. Same class of bug, same fix pattern already in the PR. Extracting a _cancel_demotion_timer() helper mirroring _cancel_debounce_timer() and adding an identity check in the callback would close the gap.

As Hisoka put it: "The PR introduced the identity-check pattern to solve exactly this class of bug. The demotion timer is the sibling."

---

lua/claudecode/selection.lua:259

P2 [DEREM-6] The demotion timer callback uses a truthiness check (if M.state.demotion_timer then) instead of the identity check this PR introduces for the debounce timer. handle_selection_demotion(current_buf) at line 264 runs unconditionally outside the guard block, so stale callbacks still fire after cancellation. (Hisoka P2, Pariston P2, Meruem P2, Takumi P2, Mafuuu P2)

"What happens at three thousand rapid visual-exit cycles (macros, key repeat, automated tests)? Shall I show you?" — Hisoka

Sequence: (1) Visual exit creates demotion timer T1. libuv fires it; vim.schedule_wrap enqueues the callback. (2) disable() runs: nils demotion_timer, stops, closes the handle. (3) Scheduled callback runs: M.state.demotion_timer is nil, skips cleanup, but calls handle_selection_demotion() which writes M.state.latest_selection after teardown. Second consequence: if a demotion timer is replaced within the 50ms window, the stale callback reads M.state.demotion_timer (now pointing to the replacement) and closes it.

Fix: capture the timer in a local at creation, add if M.state.demotion_timer ~= timer then return end at the top of the callback. Extract a _cancel_demotion_timer() mirroring _cancel_debounce_timer() to consolidate the 5+ inline cancellation sites.

🤖

lua/claudecode/selection.lua:295

Note [DEREM-10] handle_selection_demotion has no tracking_enabled guard. update_selection() checks it at line 171, but handle_selection_demotion is called directly by the demotion timer callback. A stale callback after disable() reaches this function unchecked. Adding if not M.state.tracking_enabled then return end at the top would be independent defense-in-depth beyond the identity-check fix. (Takumi)

🤖

🤖 This review was automatically generated with Coder Agents.

[ThomasK33]

/coder-agents-review

[coder-agents-review]

All R1 findings addressed. The P2 (demotion timer identity guard) is correctly fixed: _cancel_demotion_timer() extracts the nil-first pattern, the callback checks M.state.demotion_timer ~= timer, and the inline cancellation sites are consolidated. The demotion timer test covers disable-then-stale-fire with call-count assertions. Thorough response to the review.

0 P2, 2 P3 new.

Hisoka verified every R1 fix and found no new issues: "The timer lifecycle is airtight: every new_timer() has a matching :close() on all paths."

The two new P3s are about completeness: the demotion timer's normal-fire path (identity guard passes, callback runs handle_selection_demotion) is never exercised in tests, and disable() clears 5 of 6 mutable state fields but leaves last_active_visual_selection, causing a phantom demotion timer on re-enable that self-heals after one cycle.

🤖 This review was automatically generated with Coder Agents.

[ThomasK33]

/coder-agents-review

[coder-agents-review]

R2 findings addressed. The normal-fire demotion test (selection_test.lua:535) exercises the full callback path with cursor-position demotion, broadcast, and handle cleanup. disable() now clears last_active_visual_selection, closing the phantom-timer gap.

Hisoka traced every lifecycle path (debounce create/cancel/fire/stale, demotion create/cancel/fire/stale, disable racing with pending callbacks, rapid debounce-to-demotion cascades) and found them clean. Mafuuu verified the DEREM-18 root cause is addressed.

1 P3 new, 0 P2.

The remaining P3 is a missing regression assertion: the disable() test doesn't assert last_active_visual_selection == nil, so reverting the one-line fix at selection.lua:49 would pass silently. The production code is correct; this is a test completeness issue.

As Hisoka put it: "All roads lead to the same guard."

🤖 This review was automatically generated with Coder Agents.

[ThomasK33]

/coder-agents-review

[coder-agents-review]

DEREM-19 addressed in fcc462a. Netero R4 scan clean: all sections pass, no new findings. All 19 findings across 4 rounds resolved (18 fixed, 1 acknowledged).

The PR is ready to merge.

🤖 This review was automatically generated with Coder Agents.

[ThomasK33]

Merge activity

• Apr 27, 6:17 PM UTC: A user started a stack merge that includes this pull request via Graphite.
• Apr 27, 6:18 PM UTC: Graphite rebased this pull request as part of a merge.
• Apr 27, 6:19 PM UTC: @ThomasK33 merged this pull request with Graphite.