fix(windows): dispatch keys off the low-level hook thread, as the other taps do (#1617)

Author: y3owk1n

internal/adapter/eventtap/adapter.go

@@ -202,9 +202,10 @@ func (a *Adapter) PostModifierEvent(modifier string, isDown bool) {
 // through SetModifierPassthrough, which takes mu. Holding mu across the wait
 // inverts the documented handler → adapter order, so a shutdown racing a focus
 // change deadlocked with neither side able to give way. The Linux tap's own
-// Destroy releases its lock before waiting for the same reason. Windows waits
-// too, but bounded: its hook join gives up after 250ms and reaps in the
-// background, for this same hazard one layer down.
+// Destroy releases its lock before waiting for the same reason, and the
+// Windows tap waits the same way since it gained a dispatcher
+// (dispatchWg.Wait() in eventtap/windows/tap.go); only the hook join beneath
+// it stays bounded at 250ms.
 //
 // What mu still covers is the state: the adapter marks itself destroyed and
 // disabled under it, in one hold, before letting go — so a caller racing the

internal/adapter/eventtap/adapter_test.go

@@ -17,9 +17,9 @@ const blockedCallTimeout = 2 * time.Second
 // fakeTap stands in for a platform tap. Every method records that it was
 // reached, so a test can say what the adapter did and did not forward.
 //
-// onDestroy is what makes it useful here: both real backends spend their
-// Destroy waiting for the key dispatcher to drain — dispatchWg.Wait() on Linux,
-// stopDispatcher on macOS — and that wait runs whatever the dispatcher is in
+// onDestroy is what makes it useful here: all three real backends spend their
+// Destroy waiting for the key dispatcher to drain — dispatchWg.Wait() on Linux
+// and Windows, stopDispatcher on macOS — and that wait runs whatever the dispatcher is in
 // the middle of delivering. onDestroy is where a test puts that.
 type fakeTap struct {
 	mu     sync.Mutex

internal/adapter/eventtap/windows/tap.go

@@ -6,6 +6,7 @@ import (
 	"context"
 	"strings"
 	"sync"
+	"sync/atomic"
 
 	"go.uber.org/zap"
 
@@ -15,6 +16,19 @@ import (
 	"github.com/y3owk1n/neru/internal/domain/keyvocab"
 )
 
+// dispatchChBufferSize bounds how many events the hook thread can queue ahead
+// of the dispatcher. The Linux tap uses the same figure.
+const dispatchChBufferSize = 256
+
+// dispatchEvent is one item on the dispatcher's queue: a key for the handler,
+// or a passthrough notification, which rides the same queue so it lands after
+// the keys that preceded it.
+type dispatchEvent struct {
+	key         string
+	passthrough tap.PassthroughCallback
+	epoch       uint64
+}
+
 // EventTap is a keyboard event interceptor on Windows.
 type EventTap struct {
 	logger *zap.Logger
@@ -44,6 +58,26 @@ type EventTap struct {
 	heldModifiers   map[string]struct{}
 
 	hook *winplatform.KeyboardHook
+
+	// dispatchCh carries events from the hook thread to dispatchLoop, the one
+	// goroutine that calls the handler. Windows silently removes a
+	// WH_KEYBOARD_LL hook whose procedure overruns LowLevelHooksTimeout, and
+	// the handler holds the mode lock across a click, a mode exit or a hint
+	// refresh, so the hook procedure only classifies the event and enqueues.
+	// The channel and loop live for the tap's lifetime, like the Linux tap's;
+	// stopDispatch ends them from Destroy, and the channel is never closed,
+	// so a key the hook delivers during teardown is dropped rather than sent
+	// on a closed channel.
+	dispatchCh   chan dispatchEvent
+	stopDispatch chan struct{}
+	stopOnce     sync.Once
+	dispatchWg   sync.WaitGroup
+
+	// dispatchEpoch is stamped on every event as it is queued and bumped on
+	// every Disable. dispatchLoop drops an event stamped with an earlier
+	// epoch, so a key the hook read while the mode was exiting is not
+	// delivered to whatever mode comes next.
+	dispatchEpoch atomic.Uint64
 }
 
 // NewEventTap creates a new event tap.
@@ -52,10 +86,16 @@ func NewEventTap(callback tap.Callback, logger *zap.Logger) *EventTap {
 		logger = zap.NewNop()
 	}
 
-	return &EventTap{
-		logger:   logger.Named("eventtap"),
-		callback: callback,
+	eventTap := &EventTap{
+		logger:       logger.Named("eventtap"),
+		callback:     callback,
+		dispatchCh:   make(chan dispatchEvent, dispatchChBufferSize),
+		stopDispatch: make(chan struct{}),
 	}
+
+	eventTap.dispatchWg.Go(eventTap.dispatchLoop)
+
+	return eventTap
 }
 
 // Enable enables the event tap.
@@ -104,11 +144,33 @@ func (et *EventTap) Disable() {
 	if hook != nil {
 		hook.Stop()
 	}
+
+	// Anything still queued was read by the hook before it stopped and belongs
+	// to the mode that just exited. The bump covers an event dispatchLoop has
+	// already taken off the channel; the drain empties the rest. Disable runs
+	// on the dispatcher itself when a key exits the mode, which is why neither
+	// step may wait on it.
+	et.dispatchEpoch.Add(1)
+
+	for {
+		select {
+		case <-et.dispatchCh:
+		default:
+			return
+		}
+	}
 }
 
-// Destroy destroys the event tap.
+// Destroy disables the tap and stops its dispatcher, waiting for a key that
+// is being delivered to finish. Safe to call more than once.
 func (et *EventTap) Destroy() {
 	et.Disable()
+
+	et.stopOnce.Do(func() {
+		close(et.stopDispatch)
+	})
+
+	et.dispatchWg.Wait()
 }
 
 // SetHotkeys sets the hotkeys. These are the global [hotkeys] chords the
@@ -414,27 +476,66 @@ func (et *EventTap) shouldPassthroughChord(chord string) bool {
 	return true
 }
 
-// firePassthroughCallback invokes the registered passthrough callback (if any)
-// on its own goroutine and without holding et.mu. The hook procedure runs on
-// the hook thread, where the mode handler's lock must not be taken: mode exit
-// stops the hook while holding it (see KeyboardHook.Stop).
+// firePassthroughCallback queues the registered passthrough callback (if any)
+// for the dispatcher, so it runs off the hook thread and after the keys that
+// preceded it.
 func (et *EventTap) firePassthroughCallback() {
 	et.mu.RLock()
 	callback := et.passthroughCallback
 	et.mu.RUnlock()
 
 	if callback != nil {
-		go callback()
+		et.enqueue(dispatchEvent{passthrough: callback})
 	}
 }
 
+// dispatchKey queues a key for the dispatcher. It never blocks: the hook
+// procedure calls it on the hook thread, which has to return before Windows
+// gives up on it.
 func (et *EventTap) dispatchKey(key string) {
+	if key != "" {
+		et.enqueue(dispatchEvent{key: key})
+	}
+}
+
+func (et *EventTap) enqueue(event dispatchEvent) {
+	event.epoch = et.dispatchEpoch.Load()
+
+	select {
+	case <-et.stopDispatch:
+	case et.dispatchCh <- event:
+	default:
+		et.logger.Warn("Dispatch queue full, dropping event")
+	}
+}
+
+// dispatchLoop delivers queued events to the handler, in order, on this one
+// goroutine.
+func (et *EventTap) dispatchLoop() {
+	for {
+		select {
+		case <-et.stopDispatch:
+			return
+		case event := <-et.dispatchCh:
+			et.deliver(event)
+		}
+	}
+}
+
+func (et *EventTap) deliver(event dispatchEvent) {
+	if et.dispatchEpoch.Load() != event.epoch {
+		return
+	}
+
 	et.mu.RLock()
 	callback := et.callback
 	et.mu.RUnlock()
 
-	if callback != nil && key != "" {
-		callback(key)
+	switch {
+	case event.passthrough != nil:
+		event.passthrough()
+	case callback != nil:
+		callback(event.key)
 	}
 }
 

internal/adapter/eventtap/windows/tap_hotkey_test.go

@@ -75,13 +75,13 @@ func TestEventTap_SetHotkeys_ReplacesTheList(t *testing.T) {
 func TestEventTap_HandleKey_StickyModifierDoesNotFireGlobalHotkey(t *testing.T) {
 	t.Parallel()
 
-	var dispatched []string
-
-	tap := NewEventTap(func(key string) { dispatched = append(dispatched, key) }, nil)
+	tap := newTestTap(t)
 	tap.SetHotkeys([]string{"Ctrl+Shift+J"})
 	tap.notePostedModifier("shift", true)
 
-	if !tap.handleKey("Ctrl+Shift+J", false) {
+	dispatched, consumed := routedKey(t, tap, "Ctrl+Shift+J")
+
+	if !consumed {
 		t.Fatal("Ctrl+J with sticky Shift was handed to RegisterHotKey instead of consumed")
 	}
 

internal/adapter/eventtap/windows/tap_passthrough_test.go

@@ -16,22 +16,42 @@ import (
 const chordCtrlC = "ctrl+c"
 
 // routedKey drives handleKey for one key-down and reports where it went.
+// Delivery is asynchronous, so a sentinel is queued behind the key and the
+// keys that arrive ahead of it are the ones the chord dispatched: the
+// dispatcher delivers in order, which is what makes the sentinel a fence.
 func routedKey(t *testing.T, eventTap *EventTap, key string) ([]string, bool) {
 	t.Helper()
 
-	var dispatched []string
+	const sentinel = "routedKey-sentinel"
+
+	arrived := make(chan string, 8)
 
-	eventTap.SetHandler(func(key string) { dispatched = append(dispatched, key) })
+	eventTap.SetHandler(func(key string) { arrived <- key })
 
 	consumed := eventTap.handleKey(key, false)
 
-	return dispatched, consumed
+	eventTap.dispatchKey(sentinel)
+
+	var dispatched []string
+
+	for {
+		select {
+		case delivered := <-arrived:
+			if delivered == sentinel {
+				return dispatched, consumed
+			}
+
+			dispatched = append(dispatched, delivered)
+		case <-time.After(time.Second):
+			t.Fatal("the dispatcher never delivered the sentinel queued behind the key")
+		}
+	}
 }
 
 func TestEventTap_HandleKey_ConsumesAnUnboundChordWithPassthroughOff(t *testing.T) {
 	t.Parallel()
 
-	eventTap := NewEventTap(nil, nil)
+	eventTap := newTestTap(t)
 	eventTap.SetModifierPassthrough(false, nil)
 
 	dispatched, consumed := routedKey(t, eventTap, chordCtrlC)
@@ -48,7 +68,7 @@ func TestEventTap_HandleKey_ConsumesAnUnboundChordWithPassthroughOff(t *testing.
 func TestEventTap_HandleKey_PassesAnUnboundChordThrough(t *testing.T) {
 	t.Parallel()
 
-	eventTap := NewEventTap(nil, nil)
+	eventTap := newTestTap(t)
 	eventTap.SetModifierPassthrough(true, nil)
 
 	fired := make(chan struct{}, 1)
@@ -97,7 +117,7 @@ func TestEventTap_HandleKey_KeepsAChordTheModeBinds(t *testing.T) {
 		t.Run(testCase.name, func(t *testing.T) {
 			t.Parallel()
 
-			eventTap := NewEventTap(nil, nil)
+			eventTap := newTestTap(t)
 			testCase.setup(eventTap)
 
 			dispatched, consumed := routedKey(t, eventTap, "ctrl+shift+c")
@@ -118,7 +138,7 @@ func TestEventTap_HandleKey_KeepsAChordTheModeBinds(t *testing.T) {
 func TestEventTap_HandleKey_ConsumesShiftOnlyChordsWithPassthroughOn(t *testing.T) {
 	t.Parallel()
 
-	eventTap := NewEventTap(nil, nil)
+	eventTap := newTestTap(t)
 	eventTap.SetModifierPassthrough(true, nil)
 
 	for _, key := range []string{"Shift+A", "a", "Return"} {
@@ -133,7 +153,7 @@ func TestEventTap_HandleKey_ConsumesShiftOnlyChordsWithPassthroughOn(t *testing.
 func TestEventTap_HandleKey_HandsARegisteredHotkeyBack(t *testing.T) {
 	t.Parallel()
 
-	eventTap := NewEventTap(nil, nil)
+	eventTap := newTestTap(t)
 	eventTap.SetHotkeys([]string{"Ctrl+G"})
 	eventTap.SetModifierPassthrough(false, nil)