Skip to content

Fix keybindings menu hang when user configs iterate hl accessors - #10212

Closed
evariste1963 wants to merge 2 commits into
omacom:quattrofrom
evariste1963:fix/keybindings-menu-noop-scan-hang
Closed

evariste1963 wants to merge 2 commits into
omacom:quattrofrom
evariste1963:fix/keybindings-menu-noop-scan-hang

Conversation

@evariste1963

Copy link
Copy Markdown

Problem

omarchy-menu-keybindings parses the user's ~/.config/hypr/hyprland.lua under a stubbed hl so it can resolve Lua bindings that Hyprland reports as __lua. Unknown hl.* accessors fall back to a noop object whose __index returns itself for every key — including numeric indices.

Any real user config that calls a list accessor and walks the result spins forever:

for _, p in ipairs(hl.get_loaded_plugins()) do ... end

This is what the border-fx plugin's generated border-fx.lua does. Result: lua pegs a CPU core, omarchy menu keybindings never opens, and --print never returns.

Fix

In the embedded noop metatable, return nil for numeric keys while still mirroring non-numeric access for method chaining. Stubbed accessors then read as empty sequences, so ipairs() and # terminate immediately instead of recursing on a metatable that never yields nil.

Test

Added a regression test to test/shell.d/keybindings-menu-test.sh that installs a config calling ipairs(hl.get_loaded_plugins()) and asserts the scan completes (wrapped in timeout so a regression fails instead of hanging the suite).

Verified:

  • new test passes; full test/shell.d/keybindings-menu-test.sh passes
  • ./test/cli passes
  • before the fix the reproducer times out (exit 124); after, it exits 0

@Chessing234 Chessing234 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

returning nil for numeric keys on the noop metatable is exactly what stops ipairs from spinning. good catch, and the timeout test pins it.

It shared XDG_CACHE_HOME and the stubbed hyprctl with the block before it, and the records cache is keyed on that stub alone, so the run answered from the cache and never scanned a config at all. The config it installed was also loaded through Omarchy's defaults, which abort the scan at default/hypr/qconsole.lua:64 — comparing the noop against a number — well before any user file. Either of those alone made it pass with the noop reverted, on a worker.

Scan a standalone config from a cache of its own, and assert that the bind declared after the loop still renders with the key only the scan can supply, so a scan that stops early fails the same way a hang does.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@omarchybot

Copy link
Copy Markdown
Collaborator

Reviewed against quattro, with a second opinion from Codex at xhigh reasoning, and everything below that says "verified" was run on a disposable Omarchy worker VM rather than read.

The fix is right, and the mechanism is exactly the one you named. ipairs has honoured __index since Lua 5.3 (it switched from lua_rawgeti to lua_geti), so a noop that answers a non-nil value at every integer index never gives the iterator its terminating nil. Measured on the worker under Lua 5.5.1: the old stub runs past 200,000 iterations of ipairs, the new one stops at 0. End to end, with a config doing ipairs(hl.get_loaded_plugins()), omarchy-menu-keybindings --print exits 124 under timeout 20 before the change and 0 after it.

Two smaller corrections to the write-up. # was never affected — there is no __len, so #noop reads the raw empty table and has always been 0 — and neither was pairs, which iterates the raw table and returns nothing. The comment at bin/omarchy-menu-keybindings:190-191 says the same thing about #, and says "a fresh value" where the old code returned the same noop singleton each time. Codex flagged both; they are wrong rather than harmful, so nothing was changed.

The regression test did not test the regression, and I have pushed a commit that makes it fail without the fix. Verified by mutation on the worker: with your bin/omarchy-menu-keybindings change reverted and the test as written, the file still reported 16 passed, 0 failed. Two independent reasons, either one sufficient:

  1. The records cache. keybindings_cache_key (bin/omarchy-menu-keybindings:539) hashes only v13, the active keymap and hyprctl binds — not $HOME, not the config. The new block reused XDG_CACHE_HOME="$tmpdir/cache" and the stub hyprctl left in place by the block above it, whose run at line 193 had already populated that cache, so output_binding_records answered from the file at bin/omarchy-menu-keybindings:571-572 and build_lua_bind_cache never ran.
  2. Omarchy's own defaults abort the scan first. default/hypr/qconsole.lua:64 does monitor.scale <= 0, and under the stub monitor.scale is a table, so the scan dies with attempt to compare table with number — inside require("default.hypr.omarchy") at config/hypr/hyprland.lua:14, which is before every user file the config loads. DEBUG=1 shows it. Your pcall(require, "hypr.spin") was appended at the end and so was never reached. Giving the block its own cache directory alone does not fix this: on the worker, a fresh cache with the fix reverted still exits 0 rather than hanging.

The commit scans a standalone hyprland.lua from a cache of its own, keeping your ipairs(hl.get_loaded_plugins()) reproducer, and adds an assertion that the hl.bind declared after the loop still renders. The stub reports that bind without its key, so a rendered SUPER + P is the scan getting past the loop — which means a scan that stops early now fails the same way a hang does. Verified both directions on the worker: passes with your fix, fails in 20s without it.

Two things left open, deliberately not pushed.

Returning nil for numeric keys is right for a stub that should read as an empty sequence, but it converts one failure mode into another for a different shape of config. local m = hl.get_monitors()[1] followed by m.name used to return noop twice and carry on; it now raises attempt to index a nil value, pcall(dofile, ...) at bin/omarchy-menu-keybindings:242 swallows it, and every hl.bind after that point is lost silently — the row still appears from hyprctl binds but with no dispatcher, so selecting it does nothing (bin/omarchy-menu-keybindings:652). That is a real trade, not a reason to hold the change; a hang is worse. Worth the maintainer knowing it is a trade.

dsp_proxy (bin/omarchy-menu-keybindings:171) and the outer hl metatable (bin/omarchy-menu-keybindings:231) still have the identical never-nil numeric __index, so ipairs(hl.dsp) and ipairs(hl) both still spin — confirmed on the worker at 200,000 iterations, and dsp_proxy allocates a table per iteration while it does it. Neither is something a config would plausibly write, which is why this is reported rather than pushed: widening your change is the maintainer's call.

What this collides with, which is the part worth reading. #10214 is the fifth open report of this bug — #7025, #8819, #9052 and #9395 all describe the same ipairs over the stub, and #9395 names the same wmfeht.border-fx plugin. Two open pull requests already fix it: #8876 carries the same numeric-nil change and puts its reproducer at line 1 of the config so it runs before the qconsole.lua abort, and #7564 is a strict superset — the same numeric-nil guard plus __len, __lt/__le, arithmetic, __concat and __tostring on the stub, which is what actually clears the qconsole.lua:64 abort described above, plus timeout 10 around the scan so a config the stub still cannot satisfy cannot leave a runaway lua behind after the menu closes. Which of the three lands is entirely the maintainer's decision and nothing here touches the others.

One mechanical thing: the body has no Fixes #10214, so merging this will not close it. I read both and the pairing holds — same author, same mechanism, same reproducer, and this branch is its fix.

Waiting on the maintainer, both for that choice and for whether default/hypr/qconsole.lua:64 gets fixed separately: while it stands, the scan stops before require("hypr.bindings"), so a user's own Lua binds reach the menu with an empty dispatcher and do nothing when selected. Reproduced on the worker with the stock config and a single o.bind in ~/.config/hypr/bindings.lua.

Codex reviewed the same head independently at xhigh and reached the vacuous-test and early-abort findings too; its independence is not currently guaranteed, so read that as agreement rather than confirmation. Two things were its own: the Lua 5.2-to-5.3 boundary above, and that the guard calls type as a mutable global, so a config with a stray global type assignment would now reach its replacement from inside the stub. That last one is pre-existing in kind — bin/omarchy-menu-keybindings:172 and :215 already call tostring and type the same way — so it is noted rather than changed. Its suggested minimal fix for the test, a separate cache directory on its own, does not work, for the reason measured above.

@devcrvftwerk

Copy link
Copy Markdown

Independent reproduction on Omarchy 4.0.0.alpha (the linked issues cite 4.0.2-1), Hyprland 0.56.2, kernel 7.2.3-arch1-2. Triggered by wmfeht.border-fx 0.2.0, whose generated ~/.config/hypr/border-fx.lua contains the exact reproducer:

for _, p in ipairs(hl.get_loaded_plugins()) do

Two behavioural details that don't appear in the write-up and may matter for how the fix gets prioritised:

The spinner outlives its parent. Killing omarchy-menu-keybindings does not stop the lua child — it is reparented to systemd --user and holds a core at 99.7% indefinitely. Mine ran 29 minutes unnoticed. Its /proc/<pid>/cmdline is just lua with no argv, so nothing about the process points back to the keybindings menu; it took a parent-chain walk on the still-parented siblings to attribute it.

It accumulates once per invocation. Every open leaves another spinner behind. I ended up with four concurrent (~400% CPU, load average 3.2) from three menu opens plus the one orphan above.

So on an affected config the user-visible cost isn't just a menu that won't open — it's a permanently lost core per attempt, with no obvious attribution back to the cause.

@bjarneo

bjarneo commented Sep 21, 2026

Copy link
Copy Markdown
Member

Automated duplication check: this pull request looks similar to #8876, which covers the same keybindings scanner ipairs termination fix. I keep that one open and close this one to consolidate review.

If you feel this is the wrong decision, please open the PR again with a note on the difference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants