Bump kanata engine: fast stuck-key recovery after VHID reconnect (MAL-57 Layer 2)

[malpern]

Summary

Submodule bump pulling malpern/kanata#26 into the bundled engine — MAL-57 Layer 2 (Layer 1 was #882). When the kanata↔VHID-daemon socket dies mid-keystroke, a lost key Release no longer autorepeats for ~3 seconds:

1. Stable per-PID client socket path — the daemon keys the DriverKit virtual-keyboard instance by this path, so a reconnect reuses the existing keyboard instead of spawning a second one while the stuck instance repeats until GC
2. virtual_hid_keyboard_reset on keyboard-ready rising edge — clears stuck keys as soon as the connection recovers; held keys survive because kanata's reports carry absolute full state
3. Daemon heartbeat detection 3s → 1s — matches the daemon's own per-client ping rate

Worst-case burst ~3s → ~1.5s, typical <1s. Also cuts daemon stdout log volume ~95% (ready-status logged on transitions only — it was 183MB of once-per-second spam).

Implemented by vendoring karabiner-driverkit 0.3.1 inside the fork ([patch.crates-io]); full causal chain in docs/bugs/MAL-57-duplicate-keypresses.md.

Test plan

• kanata cargo test green on the fork branch; cargo build + release engine build (Scripts/build-kanata.sh, cmd+tcp_server, aarch64) signs and passes binary smoke test
• Post-deploy manual verification: sudo kill -STOP the VHID daemon ~4s while holding a key, kill -CONT, confirm the repeat burst is bounded (~1s) and held modifiers survive; confirm virtual_hid_keyboard_reset sent after ready transition in the kanata log

[claude]

Code Review: Bump kanata engine — fast stuck-key recovery (MAL-57 Layer 2)

Summary: Submodule pointer bump (External/kanata: 49b856a3b → 6a05a4a3f), pulling three engine-side changes from malpern/kanata#26 to fix the ~3s autorepeat burst after VHID reconnect.

---

✅ What's solid

• Causal chain is complete. The PR description and docs/bugs/MAL-57-duplicate-keypresses.md trace the failure end-to-end: missed heartbeat → dropped Release → stuck virtual-keyboard state → ~3s OS autorepeat. The fix targets the right layer.
• Stable per-PID socket path is well-reasoned. Keying the DriverKit virtual-keyboard instance by socket path prevents the double-keyboard ghost that holds a stuck key while a new clean instance is already reporting state. This is the crux of the fix.
• 95% log volume reduction is a real quality-of-life win. 183MB/day of ready-status noise was obscuring useful signal; transition-only logging is strictly better.
• Vendoring karabiner-driverkit via [patch.crates-io] is the right mechanism for this kind of downstream Rust patch — clean and auditable.
• Test plan is honest about physical-keyboard requirement. The MAL-57 doc's hard-won lesson (osascript bypasses Kanata entirely) is correctly applied here.

---

⚠️ Issues to address before merge

1. CLAUDE.md fast-forward check is not documented (blocking per repo invariants)

From CLAUDE.md:

Before pushing to any remote branch in the fork, verify the push is a fast-forward: git log --oneline <source>..<target>. If commits would be lost, cherry-pick instead.

The PR body doesn't confirm this was verified for the keypath/bundled bump. Please add a one-liner to the PR description confirming:

git log --oneline 49b856a3bdeb66791b65eaad718f1e7b9fb95c59..6a05a4a3fea117eb1d240d9c77492fb9ed02f7d1

was checked and was a clean fast-forward. This protects against accidentally force-pushing to the fork's main lineage.

2. Heartbeat tightening may worsen the disconnect rate under load

The PR tightens detection from 3s → 1s to match the daemon's ping rate. This is the right direction for minimizing the damage window — but it means heartbeat misses will now trigger a reconnect 3× faster under CPU starvation.

The MAL-57 doc's Layer 1 fix (ProcessType=Interactive on the VHID daemon plist) should reduce starvation significantly, but if Layer 1 is not yet deployed on a given install, Layer 2 could increase reconnect frequency while reducing each event's damage window. Net effect depends on the real-world miss rate.

Request: Add a note in the PR (or bug doc) on whether the 1s ping rate was chosen because it provably matches the daemon's heartbeat tolerance, or because it's an empirical improvement. If the daemon itself is the process answering pings, a 1s window under starvation may miss more often.

3. "Held keys survive because kanata reports carry absolute full state" — needs explicit reference

This invariant is load-bearing. If virtual_hid_keyboard_reset fires but the subsequent "re-grab" event carrying full key state is delayed or lost, held modifiers would drop silently.

Request: Add a pointer in the PR or bug doc to the specific code in malpern/kanata#26 (the sink_ready rising edge handler) that guarantees the reset is immediately followed by a full-state report. A link to the relevant lines in the Rust source would give reviewers confidence this isn't a race.

4. Worst-case bound changed, not eliminated

The PR description says "worst-case burst ~3s → ~1.5s" — the 1.5s residual is the reconnect latency floor. This is accurate and honest. The test plan's kill -STOP / kill -CONT verification validates the improvement but should also confirm the 1.5s bound under the compile load preset from Scripts/manual-keystroke-test.sh --reconnect-after, not just a clean kill -STOP. Under load, reconnect may take longer than 1s.

---

📋 Minor / non-blocking

• Layer 3 gap acknowledged but not scheduled. The bug doc describes Layer 3 (never silently drop a Release — replay after reconnect) as open. Is there a tracking item for it, or is MAL-57 being left open? Even a note like "Layer 3 deferred; Layer 1+2 reduce frequency and damage window to acceptable levels" would close the loop.
• No Swift-side tests for the integration. Since this is a pure submodule bump there's nothing to unit-test in Swift, but consider whether ServiceHealthCheckerTests or a smoke test should assert the binary version/build hash matches an expected value after a kanata engine bump. Prevents stale builds from silently shipping the old engine.

---

Verdict

The fix is architecturally correct and the PR description is exemplary. Block on #1 (fast-forward proof, per CLAUDE.md invariants) and #3 (explicit reference to the held-key safety invariant in the Rust source). Items #2 and #4 are documentation/confidence requests that won't block merge but should be addressed.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d96f7e7181

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Clear local report state before returning not-ready

When the VHID daemon is marked not ready before kanata processes the physical key-up, the vendored driverkit/c_src/driverkit.cpp returns 2 from send_key before updating its local keyboard.keys report cache. The macOS recovery path then clears Rust-side tracking while input is released, but this C++ cache still contains the pressed key; after this bump reuses the same client and resets the virtual keyboard on reconnect, the next emitted report can re-assert that stale key and make the stuck key come back. Please update the not-ready/recovery path to apply releases to the local report state or explicitly clear the C++ report caches before resuming output.

Useful? React with 👍 / 👎.

[malpern]

Addressing the review:

#1 — Fast-forward proof (blocking): keypath/bundled advanced 49b856a3b → 6a05a4a3f via GitHub's merge of malpern/kanata#26 — not a direct push. Verified locally:

• git log --oneline 49b856a..6a05a4a → exactly the merge commit + the one feature commit
• git rev-parse 6a05a4a^1 → 49b856a3b... (old tip is the merge's first parent)
• git merge-base --is-ancestor 49b856a 6a05a4a → true

No commits lost; the bundled lineage is strictly append-only.

#3 — Held-key invariant reference (blocking): the guarantee is stronger than "reset is followed by a full-state report" — every report kanata posts is full-state by construction. The send_key template at the top of driverkit/c_src/driverkit.cpp maintains a persistent keyboard.keys set (insert on press, erase on release) and posts the entire report object on every event. So there is no race to lose: after virtual_hid_keyboard_reset, the next key event of any kind re-asserts the complete held-key set atomically in one report.

#2 — Heartbeat rationale: 1000ms was chosen to mirror the daemon's own per-client set_server_check_interval(1000ms) in virtual_hid_device_service_clients_manager.hpp — the daemon already requires clients to be responsive at that cadence, so the client expecting the same of the daemon adds no new tolerance class. The reviewer is right that detection fires 3× sooner under starvation — that is the intent, and it is safe because of the other two changes in #26: with the stable per-PID socket path a reconnect reuses the same daemon-side keyboard instance (no ghost-keyboard spawn), and the reset makes each reconnect self-healing. Reconnects are now cheap; staying connected to a dead daemon for 3s is what was expensive. Layer 1 (#882, ProcessType=Interactive) independently reduces the miss rate.

#4 — Load-test bound: agreed; the post-deploy verification will include a compile-load run alongside the clean kill -STOP test, and results will be recorded in docs/bugs/MAL-57-duplicate-keypresses.md.

Layer 3 tracking: filed as malpern/kanata#27.

[claude]

Code Review — PR #885: Bump kanata engine (MAL-57 Layer 2)

Overview

This is a one-line submodule bump (External/kanata from 49b856a3b → 6a05a4a3f) that pulls in kanata fork PR malpern/kanata#26. The three targeted changes in the vendored karabiner-driverkit 0.3.1:

1. Stable per-PID client socket path — reconnect reuses the existing DriverKit keyboard instance
2. virtual_hid_keyboard_reset on keyboard-ready rising edge — clears stuck keys on recovery
3. Server-check interval 3 000 ms → 1 000 ms — detect dead daemon connection ~2s sooner

---

What's Good

• Tight causal reasoning. The PR description maps each change to the confirmed failure mode in MAL-57-duplicate-keypresses.md. The three-point fix addresses reconnect identity, stuck-key clearance, and heartbeat latency independently — none of them is load-bearing for the others, so partial failure degrades gracefully rather than silently.
• Safe absolute-state model. The rationale for virtual_hid_keyboard_reset is sound: kanata's HID reports carry full key state, so a reset-then-reassert cycle after reconnect can't permanently drop a held modifier. The causal doc confirms this.
• Logging improvement is a genuine fix. Cutting the daemon stdout from 183 MB of once-per-second spam to transition-only logging removes a concrete I/O pressure source that may itself contribute to CPU starvation.
• Vendoring approach is correct. Using [patch.crates-io] in Cargo.toml to vendor karabiner-driverkit 0.3.1 without forking it is the right scope — the changes are localized to the C++ headers and don't ripple into the Rust surface.
• Submodule fork safety. The PR description and CLAUDE.md both require that pushes to keypath/bundled are fast-forwards. Worth confirming (see below).

---

Issues / Questions

1. Fast-forward verification (required by CLAUDE.md)

Before pushing to any remote branch in the fork, verify the push is a fast-forward: git log --oneline <source>..<target>. If commits would be lost, cherry-pick instead.

The PR description doesn't explicitly confirm the keypath/bundled push was a fast-forward. Given that malpern/kanata PR #26 is described as landing on keypath/bundled, this should be called out — a rebase on top of a diverged main could lose commits. Was this verified before the push?

2. Socket path stability assumption

The description says "stable per-PID client socket path". This is a meaningful guarantee only if the kanata process PID doesn't change across the reconnect window. Under launchd with KeepAlive=true, a crash-restart would change the PID and defeat the stable-path optimization. Is there a scenario where kanata itself restarts (not just the VHID daemon) and the stuck key survives? If so, a PID-keyed path might leave an orphaned socket on the next run — though the PR notes local_datagram::client_impl removes the file before bind, so this should be harmless in practice.

3. Residual gap: Layer 3 not addressed

docs/bugs/MAL-57-duplicate-keypresses.md explicitly names a Layer 3 item (never silently drop a Release; replay dropped releases after reconnect via output_pressed_since). This PR doesn't address it and that's fine — but it's worth noting that the worst-case window after this fix is still non-zero. The PR description correctly calls this out (~1.5s worst case), but the milestone/follow-up for Layer 3 isn't tracked in the PR.

4. Test plan scope

The manual test — SIGSTOP the VHID daemon for 4s while holding a key, SIGCONT, confirm burst is bounded — is the right test. One gap: the plan doesn't verify that held modifiers survive a benign reconnect that doesn't involve a stuck key. The PR body mentions "held modifiers survive" but doesn't specify the test steps. A modifier held across a clean reconnect (no stuck key) should produce no spurious key-up, which is the main regression risk from the virtual_hid_keyboard_reset call.

---

Nit

The PR title says "fast stuck-key recovery" — the description makes clear worst-case is ~1.5s, not instant. "Bounded" or "shortened" would be more precise, though this is cosmetic.

---

Verdict

Approve with the fast-forward confirmation. The change is well-scoped, the causal chain is documented, and the fix is mechanically sound. The main ask is explicit confirmation that the keypath/bundled push was a fast-forward (per CLAUDE.md kanata fork safety rules), and adding "held modifier survives benign reconnect" to the manual test plan.