fix: keep a permanent SIGUSR1 listener so resizes cannot kill dmux - #102
Open
hozantaher wants to merge 1 commit into
Open
fix: keep a permanent SIGUSR1 listener so resizes cannot kill dmux#102hozantaher wants to merge 1 commit into
hozantaher wants to merge 1 commit into
Conversation
The client-resized hook sends SIGUSR1, but the only listener lives in a React effect that unregisters on every dialog toggle, leaving the default terminate action to kill the control pane.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #101.
Problem
setupResizeHook()installs a tmux hook that sendsSIGUSR1to dmux onclient-resized. The only listener for that signal is registered inside auseEffectinuseLayoutManagement.tswhose deps includehasActiveDialog, so it unregisters and re-registers on every dialog toggle. In that gap the process has noSIGUSR1listener and the default action — terminate — applies, so a resize at the wrong moment kills the control pane.The failure is quiet and confusing in practice: the control pane drops to a bare shell, and because the dying process never disabled mouse reporting, tmux keeps forwarding SGR mouse sequences into that shell, which reports them as
command not found: 73M64.Why it's intermittent
Worth recording, because it explains the shape of the bug. Node reserves
SIGUSR1for the inspector, so a process that has never registered a JS listener survives the signal — it just starts the debugger. The default disposition is only restored once a JS listener has been added and then removed. So dmux is safe untiluseLayoutManagementmounts and cleans up for the first time, and vulnerable from then on:The window is widest when the effect's cleanup runs without a matching re-registration — e.g. the early
returnwhencontrolPaneIdis falsy, or unmount — sinceSIGUSR1then stays fatal rather than being fatal for only a moment.Change
One permanent listener in
setupGlobalSignalHandlers(), alongside the existingSIGINT/SIGTERM/SIGUSR2registrations —SIGUSR1was the only signal dmux sends itself that wasn't handled at process level.useLayoutManagement.tsis deliberately untouched. Node invokes every registered listener, and the effect'sprocess.off('SIGUSR1', handleResize)removes only its own function reference, so the permanent listener survives cleanup and resize behaviour is unchanged. The listener exists purely to keep the default terminate action from applying.On testing
I tried to add an e2e regression test in the style of
__tests__/dmux.e2e.*— start dmux in an isolated tmux server, send itSIGUSR1, assert it's still alive — and deliberately dropped it, because it passed against the unpatched build too. Given the mechanism above, dmux survives the signal at startup regardless of this fix, and reaching the genuinely vulnerable window from outside the process means racing a React effect commit measured in microseconds. A test that green-lights the bug it's named after is worse than no test, and a timing race would just be flaky in CI.If you'd like coverage here, the seam that would make it testable is extracting the signal registration out of the
Dmuxclass (it's currentlyprivate setupGlobalSignalHandlers()on a non-exported class) so a unit test can assertprocess.listenerCount('SIGUSR1')stays above zero across an effect teardown. Happy to do that in a follow-up if you want it — it felt like more surface than a bug fix should carry.Verified manually:
pnpm install,tsc, and dmux running against the patched build with the control pane stable.Possibly worth separate issues
kill -USR1/-USR2from hitting recycled PIDs.cleanTerminalExitcovers the graceful paths, but a signal kill leaves the pane in mouse-reporting mode, which is what makes the failure so hard to read.