fix(modes): keep the keyboard captured across a mode-to-mode switch (#1519)

Author: y3owk1n

internal/adapter/eventtap/linux/evdev_cgo.go

@@ -24,12 +24,54 @@ const (
 	waylandEvdevEventBufferSize           = 128
 	waylandEvdevModifierReleasePollPeriod = 5 * time.Millisecond
 	waylandEvdevPreGrabHoldPollPeriod     = 50 * time.Millisecond
-	waylandEvdevPreGrabTimeout            = 5 * time.Second
 	waylandEvdevHotplugBufSize            = 4096
 	waylandEvdevHotplugSettleDelay        = 100 * time.Millisecond
 	waylandEvdevHotplugPollInterval       = 500 * time.Millisecond
 )
 
+// The two bounds on how long a mode activation waits for the keyboard to go
+// quiet before grabbing it. Waiting at all is #1087's first line of defense:
+// grabbing while a key is held routes that key's release to our fd alone, so
+// libinput never sees it, considers the key down forever and eats its next
+// press. The other two lines are the ones that make *these* bounds affordable —
+// initialKeys suppresses the kernel's replay of a key already held at grab
+// time, and shutdownEvdevSession injects a synthetic release for it on the way
+// out — so a wait that expires falls back onto a handled path rather than off a
+// cliff. That is what lets both of these be bounds instead of conditions.
+//
+// They differ because what is being waited for differs, and because the cost of
+// giving up differs with it.
+const (
+	// waylandEvdevModifierReleaseTimeout bounds the wait for held *modifiers*.
+	// It is the long one: a modifier whose release we swallow is stuck across
+	// every application the user touches next, not just this mode, so it is
+	// worth waiting out an activation chord that is being held. What it may not
+	// be is unbounded, which is what it was — the loop had no deadline at all,
+	// so a modifier the kernel reports as held (a chord the user rests on, a
+	// key wedged by something else) left the mode active, the overlay drawn and
+	// the keyboard never grabbed, with everything typed going to the focused
+	// application and nothing said about it. `platform/linux/AGENTS.md` has the
+	// rule this broke: never block on the eventtap goroutine.
+	waylandEvdevModifierReleaseTimeout = 5 * time.Second
+
+	// waylandEvdevPreGrabHoldTimeout bounds the wait for ordinary keys, and is
+	// short because by the time it runs the modifier wait above has already
+	// finished — so what is still down is a plain key, whose worst case is one
+	// key eaten rather than a modifier stuck under everything.
+	//
+	// Five seconds here was the reported bug (a user mashing keys as the mode
+	// came up, seeing "the grid clearly not moving… when I stop for a second,
+	// it seems to fix the issue"): someone typing through an activation holds a
+	// key at almost every poll, so the wait runs to its deadline and the mode
+	// takes no input until it does. Waiting is futile in exactly that case —
+	// the next poll finds another key down, and the grab that eventually
+	// happens is a grab with a key held either way. Half a second is long
+	// enough for the release of a deliberate keypress, which is what waiting is
+	// for, and short enough that typing into an activation costs a beat rather
+	// than the five seconds it cost before.
+	waylandEvdevPreGrabHoldTimeout = 500 * time.Millisecond
+)
+
 // waylandEvdevKeyboardActive reports whether an evdev keyboard grab is currently
 // held, i.e. keys are captured directly from the input devices rather than via
 // the overlay layer-surface's keyboard grab. When true, the overlay must NOT

internal/adapter/eventtap/linux/evdev_pregrab_bounds_test.go

@@ -0,0 +1,83 @@
+//go:build linux && cgo
+
+package linux
+
+import (
+	"testing"
+	"time"
+)
+
+// TestPreGrabBoundsStayBoundedAndOrdered pins the two pre-grab waits as bounds
+// rather than conditions, and pins which of them is the long one.
+//
+// waitForEvdevKeysReleased cannot be driven from a test — it reads EVIOCGKEY off
+// real /dev/input devices — so what is asserted here is the policy those
+// constants carry, which is the half that regressed twice and both times
+// silently:
+//
+//   - The modifier wait had no deadline at all. A modifier the kernel reported
+//     as held left the mode active, the overlay drawn and the keyboard never
+//     grabbed, with every key going to the focused application and nothing
+//     logged. Nothing failed; the daemon just stopped taking input.
+//   - The hold wait had the same five seconds as the modifier one, which is
+//     what a user typing through an activation actually waits out: they hold a
+//     key at almost every poll, so the wait runs to its deadline and the mode
+//     accepts nothing until it does.
+//
+// The ordering is the design and not an accident of the numbers: the modifier
+// wait is allowed to be the patient one because swallowing a modifier's release
+// leaves it stuck across every application the user touches next, while the
+// hold wait runs only once modifiers are clear, so the worst it can cost is one
+// suppressed press on a plain key — which initialKeys already handles.
+//
+// Both bounds must also outlast the poll that watches them, or the loop reaches
+// its deadline before it has looked at the keyboard even once and the wait
+// becomes a sleep.
+func TestPreGrabBoundsStayBoundedAndOrdered(t *testing.T) {
+	t.Parallel()
+
+	if waylandEvdevModifierReleaseTimeout <= 0 {
+		t.Errorf(
+			"waylandEvdevModifierReleaseTimeout = %v; the modifier wait must carry a "+
+				"deadline — without one a held modifier means the keyboard is never "+
+				"grabbed and every key reaches the focused application instead "+
+				"(internal/adapter/platform/linux/AGENTS.md: never block on the "+
+				"eventtap goroutine)",
+			waylandEvdevModifierReleaseTimeout,
+		)
+	}
+
+	if waylandEvdevPreGrabHoldTimeout <= 0 {
+		t.Errorf(
+			"waylandEvdevPreGrabHoldTimeout = %v; the hold wait must carry a deadline",
+			waylandEvdevPreGrabHoldTimeout,
+		)
+	}
+
+	if waylandEvdevPreGrabHoldTimeout >= waylandEvdevModifierReleaseTimeout {
+		t.Errorf(
+			"hold wait %v is not shorter than the modifier wait %v; a plain key held "+
+				"at grab time costs one suppressed press, a modifier costs every "+
+				"application the user touches next, so the patient one is the modifier "+
+				"wait — a hold wait grown to match it is the mashing-keys bug coming back",
+			waylandEvdevPreGrabHoldTimeout, waylandEvdevModifierReleaseTimeout,
+		)
+	}
+
+	for _, bound := range []struct {
+		name    string
+		timeout time.Duration
+		poll    time.Duration
+	}{
+		{"modifier", waylandEvdevModifierReleaseTimeout, waylandEvdevModifierReleasePollPeriod},
+		{"hold", waylandEvdevPreGrabHoldTimeout, waylandEvdevPreGrabHoldPollPeriod},
+	} {
+		if bound.poll >= bound.timeout {
+			t.Errorf(
+				"the %s wait polls every %v against a %v deadline; a wait that expires "+
+					"before it has read the keyboard twice is a sleep, not a wait",
+				bound.name, bound.poll, bound.timeout,
+			)
+		}
+	}
+}

internal/adapter/eventtap/linux/evdev_session_cgo.go

@@ -540,18 +540,49 @@ func (et *EventTap) injectSyntheticRelease(code uint16) {
 }
 
 // waitForEvdevKeysReleased blocks until every physically held key is released,
-// or the pre-grab timeout passes. Grabbing while a key is held makes the kernel
-// route that key's release to our fd only — libinput never sees it, considers
-// the key pressed forever, and silently eats its next press. Returns true when
-// the tap was stopped while waiting.
+// or one of the two pre-grab bounds passes. Grabbing while a key is held makes
+// the kernel route that key's release to our fd only — libinput never sees it,
+// considers the key pressed forever, and silently eats its next press. Returns
+// true when the tap was stopped while waiting.
+//
+// It waits in two stages, and the constants say why they are bounded
+// differently. Modifiers first, because a swallowed modifier release outlives
+// this mode; then everything still down, briefly, because by then it cannot be
+// a modifier. Both stages end in the same place when their bound passes: the
+// grab happens anyway, onto the handled path #1087 built for it.
+//
+// The stages are not redundant even though the second one's question subsumes
+// the first — EVIOCGKEY reports modifiers like any other key, so "nothing is
+// pressed" already implies "no modifier is held". What separates them is the
+// bound each carries and how closely each watches: the modifier stage polls at
+// five milliseconds because releasing an activation chord is on the path of
+// every mode entry and the latency is paid there, while the hold stage can tick
+// slower and wake on evdev traffic instead.
 func (et *EventTap) waitForEvdevKeysReleased(
 	capture *waylandEvdevCapture,
 	overlayCapture overlaymanager.KeyboardCaptureController,
 ) bool {
+	modifierDeadline := time.After(waylandEvdevModifierReleaseTimeout)
+
 	for capture.modifierKeysHeld() {
 		select {
 		case <-et.stopCh:
 			return true
+		case <-modifierDeadline:
+			// Grab with the modifier still down rather than never grabbing.
+			// Warn, not debug: this is the keyboard being taken while the
+			// kernel says a modifier is held, so the synthetic release on the
+			// way out is the only thing standing between the user and a
+			// modifier stuck under every application they use next.
+			if et.logger != nil {
+				et.logger.Warn(
+					"Grabbing the keyboard with a modifier still held; its release will "+
+						"not reach the compositor until this mode exits",
+					zap.Duration("waited", waylandEvdevModifierReleaseTimeout),
+				)
+			}
+
+			return false
 		case <-time.After(waylandEvdevModifierReleasePollPeriod):
 		}
 	}
@@ -567,7 +598,7 @@ func (et *EventTap) waitForEvdevKeysReleased(
 		overlayCapture.SetKeyboardCaptureEnabled(true)
 	}
 
-	deadline := time.After(waylandEvdevPreGrabTimeout)
+	deadline := time.After(waylandEvdevPreGrabHoldTimeout)
 	ticker := time.NewTicker(waylandEvdevPreGrabHoldPollPeriod)
 
 	defer func() {
@@ -590,6 +621,19 @@ func (et *EventTap) waitForEvdevKeysReleased(
 		case <-et.stopCh:
 			return true
 		case <-deadline:
+			// Debug rather than warn, unlike the modifier bound above: this is
+			// the ordinary way a user typing into an activation gets their mode
+			// promptly, and the key held through the grab costs them one
+			// suppressed press. A count, never the keys — the keystream is not
+			// something this may log (root AGENTS.md, Conventions).
+			if et.logger != nil {
+				et.logger.Debug(
+					"Grabbing the keyboard with keys still held; their first press is suppressed",
+					zap.Int("held", len(pressed)),
+					zap.Duration("waited", waylandEvdevPreGrabHoldTimeout),
+				)
+			}
+
 			return false
 		case <-ticker.C:
 		case _, ok := <-capture.events:

internal/app/modes/AGENTS.md

@@ -11,6 +11,7 @@
 - **State a focus change moves is settled on a read; the event tap's copy of it is pushed.** ADR 0005 has the app watcher publish the focused app into a lock-free cell and the keymap settle from it on the next read, because a keystroke *is* that read. The event tap's passthrough lists are the exception, and the reason is that they decide whether there will be a next read at all: an unblacklisted chord goes to the focused application instead of to Neru, so waiting for a keystroke means waiting for the keystroke that already went elsewhere. So they are pushed — `RefreshPassthroughForFocusedAppChange` (`passthrough.go`), a locked entry point started **on a goroutine** by `handleAppActivation` (`app/lifecycle.go`), never called inline from the watcher callback, which on macOS is the main queue. What the goroutine buys is only that the handler is reached off the main queue: it still queues behind whatever holds `h.mu`, so the guarantee is "as soon as the handler is free", not "before the next key". Anything else a focus change has to move goes the same way — settled on a read, or pushed through a goroutine — and never straight from the callback. The order those pushes take is `h.mu` → the event tap adapter's `mu` (`adapter/eventtap/adapter.go`), which every handler caller of `syncModifierPassthrough` has always used; the rule that comes with it is that **nothing holding the adapter's `mu` may wait on anything that takes `h.mu`**. `Adapter.Destroy` was the one violation of it and since #1511 there is none: it marks itself destroyed and disabled under `mu`, releases it, and tears the tap down outside the hold. It has to, because the two backends that wait unboundedly spend that teardown waiting for the key dispatcher — `dispatchWg.Wait()` on Linux, `stopDispatcher` on macOS — and the dispatcher drains keys into `HandleKeyPress`, so holding `mu` across it deadlocked a shutdown racing any pusher, with neither side able to give way. Windows is the third and is not one of them: its hook join is bounded at 250ms and reaps in the background past that, for this hazard, stated where it is done (`platform/windows/keyboard_hook.go`). The destroyed flag is the other half of moving the wait off the lock: from that point every adapter method that drives the tap handle is a no-op, because nothing may reach a tap being freed once the lock has stopped serializing them — the two that take no lock, `PostModifierEvent` and `SetKeyboardLayout`, are outside it because neither reaches the capture handle on any backend — they post through the display server or set a process-wide input source — and each says so where it is written. `TestAdapter_Destroy_DoesNotHoldTheLockWhileTheTapDrainsItsDispatcher` (`internal/adapter/eventtap/adapter_test.go`) is the pin, and each of the two waiting backends keeps a `TestEventTap_Destroy_WaitsForAnInFlightKeyDispatch` for the wait that makes it matter. Do not add a second violation.
 - **No method releases a lock it did not take** — every lock is released, via `defer`, by the method that took it; never unlock mid-method to make a blocking call safe. Instead compute a plan under the lock and have the lock-taking method execute it after release (`planIndicatorTick`/`drawIndicators` in `indicator_polling.go`), or hand the blocking call to a goroutine that re-enters through the outer locked surface guarded by the mode-session token (`requestScreenCapturePermissionAndResume` in `hints.go`).
 - **Everything the handler holds is reconfigured through `Handler.UpdateConfig`**, never from the app's reconfigure path. The mode components (`h.grid`, `h.scroll`) carry domain state derived from configuration, and the grid manager — the one that matters — has no lock of its own: the handler assigns it on activation and reads it on every keystroke, both under `h.mu`. So `updateComponentConfigs` lives here (`handler.go`) and runs inside the same hold; a reload calling `GridComponent.UpdateConfig` from `app/config.go` was a plain data race on live mode state (#1277), the same shape #459 had for the mode indicator. `App.reconfigureRuntimeFromConfig` reaches the handler only through `a.modes.UpdateConfig`, and a new piece of handler-held state derived from config is reconfigured there or not at all. Its `-race` regression is the one test on this list that does not live here: driving a reload against an activation needs the whole app, so it is `TestSimulation_ConfigReloadRacingGridActivation` in `internal/app` and `go test -race ./internal/app/modes/` alone will not run it.
+- **A mode-to-mode transition hands the keyboard over; it does not give it back.** `exitMode` releases the capture because that is what returning to idle means, and an activation entering from another mode calls `exitModeForTransition` instead, which is the same teardown with the release left out (`cleanup.go`). Releasing it there opened a window with nobody holding the keyboard, spanning the whole of the next mode's activation — the screen query, the domain state, the overlay draw — and on Linux that is long enough to type into: the keys reach the focused application as text, and the re-grab that follows costs a keymap rebuild plus a wait for every physically-held key to be released, so a user still mashing keys keeps extending the window (scroll had carried a carve-out of its own for the macOS shape of it since long before). Keeping it up is also where buffering comes from, and only from the moment the activation owns `h.mu`: it holds the lock end to end and the tap's dispatcher delivers through `HandleKeyPress`, so a key the tap reads *after* that waits on the lock and lands in the mode coming up. A key that reaches the handler *before* it is still the old mode's and may be dropped there — scroll's generic key handler does nothing — and nothing orders the two, because the hotkey dispatch and the dispatcher are separate goroutines contending for `h.mu`. That window is open and is not what this closes. **The exit returns its release, and the only form that compiles past the guardrail is `defer h.exitModeForTransition()()`** — exit now, release at return. It is one expression because pairing two statements by hand is a thing a new call site can half-do, and the cost of half-doing it is idle holding the keyboard, where every key the user presses goes nowhere at all. On the abandon path those buffered keys are *discarded* rather than delivered to the focused app (`Disable` drains the dispatch queue), which is the deliberate half of a two-bad-options trade and is stated on `releaseKeyboardIfNoModeEntered`. `TestModeTransitionExitsDeferTheirRelease` and `TestModeTransitionReleaseIsReachedThroughTheExit` (`internal/architecture`) fail on any other shape; `TestExitModeForTransition_KeepsTheKeyboard` and `TestReleaseKeyboardIfNoModeEntered_ReleasesOnlyWhenNothingWasEntered` pin the pair here; `TestSimulation_ModeSwitchNeverDropsTheKeyboard`, `TestSimulation_KeyPressedDuringAModeSwitchLandsInTheNewMode` and `TestSimulation_AbandonedModeSwitchGivesTheKeyboardBack` (`internal/app`) pin capture, delivery and give-back as journeys.
 - **Mode-lifecycle dispatch takes `h.mu` once**, and the whole of it — reading the active mode, selecting the implementation, calling the method — happens inside that one hold. A mode cannot change between being chosen and being used, so **an implementation must not re-check the active mode**; a re-check reads reasonable in isolation and is exactly what was deleted, so add one only with a caller that genuinely selects unlocked. `refreshActiveModeForMonitorMove` (`monitor.go`) is the pattern, `RefreshActiveModeForThemeChange` and `RefreshActiveModeForScreenChange` (`handler_refresh.go`) follow it, and ADR 0004 (`docs/adr/0004-mode-lifecycle-dispatch-under-one-lock-hold.md`) is why. There is no exception left: the screen-change dispatch was the last caller that snapshotted the mode unlocked, and the three re-checks that guarded that window went with it. The screen-change dispatch answers its caller with whether the overlay still needs a resize of its own; the app layer performs that resize but no longer decides whose overlay it is, and idle is answered inside the same hold, because resizing is what brings the overlay up and a display change with nothing open has to leave it hidden.
 
 The `Mode` interface is `Activate(modecmd.Activation)`, `HandleKey(string)`, `Exit()`, `ModeType()`, `RefreshForMonitorMove(context.Context, image.Rectangle)`. Each mode is its own type: embed `baseMode` (`base.go`), which supplies the handler reference and answers `ModeType`, then write the other four as methods on that type — there is no shared fallback, so what a mode does on each of them is answered by reading the mode. Add a `var _ Mode = (*XMode)(nil)` assertion and register in `newModes` (`handler.go`). `Handler.ActivateMode` is the only activation entry point. Monitor-move refresh is core rather than an optional extension because all five modes participate — even scroll, which draws nothing but still has to switch the overlay back to the mode its indicators name — and because a mode that quietly failed to implement it would leave an overlay stranded on the display the user left.