Prevent signals from tearing multi-char bindings

Say the user has a multi-char binding (typically an escape sequence), and a
signal arrives partway through the binding. The signal has an event handler
which enques some readline event, for example, `repaint`. Prior to this
change, the readline event would cause the multi-char binding to fail. This
would cause bits of the escape sequence to be printed to the screen.

Fix this by noticing when a sequence was "interrupted" by a non-char event,
and then rotating a sequence of such interruptions to the front of the
queue.

Fixes #8628

CHANGELOG.rst

@@ -121,6 +121,7 @@ Interactive improvements
 - The Web-based configuration tool gained a number of improvements, including the ability to set pager colors.
 - The default ``fish_title`` prints a shorter title with shortened $PWD and no more redundant "fish" (:issue:`8641`).
 - Holding down an arrow key won't freeze the terminal with long periods of flashing (:issue:`8610`).
+- Multi-char bindings are no longer interrupted if a signal handler enqueues an event. (:issue:`8628`).
 
 New or improved bindings
 ^^^^^^^^^^^^^^^^^^^^^^^^

src/input.cpp

@@ -504,6 +504,14 @@ class event_queue_peeker_t {
         idx_ = 0;
     }
 
+    /// Test if any of our peeked events are readline or check_exit.
+    bool char_sequence_interrupted() const {
+        for (const auto &evt : peeked_) {
+            if (evt.is_readline() || evt.is_check_exit()) return true;
+        }
+        return false;
+    }
+
     /// Reset our index back to 0.
     void restart() { idx_ = 0; }
 
@@ -594,7 +602,9 @@ static bool try_peek_sequence(event_queue_peeker_t *peeker, const wcstring &str)
 }
 
 /// \return the first mapping that matches, walking first over the user's mapping list, then the
-/// preset list. \return null if nothing matches.
+/// preset list.
+/// \return none if nothing matches, or if we may have matched a longer sequence but it was
+/// interrupted by a readline event.
 maybe_t<input_mapping_t> inputter_t::find_mapping(event_queue_peeker_t *peeker) {
     const input_mapping_t *generic = nullptr;
     const auto &vars = parser_->vars();
@@ -627,6 +637,12 @@ maybe_t<input_mapping_t> inputter_t::find_mapping(event_queue_peeker_t *peeker)
         peeker->restart();
     }
 
+    if (peeker->char_sequence_interrupted()) {
+        // We might have matched a longer sequence, but we were interrupted, e.g. by a signal.
+        FLOG(reader, "torn sequence, rearranging events");
+        return none();
+    }
+
     if (escape) {
         // We need to reconsume the escape.
         peeker->next();
@@ -671,6 +687,15 @@ void inputter_t::mapping_execute_matching_or_generic(const command_handler_t &co
     }
     peeker.restart();
 
+    if (peeker.char_sequence_interrupted()) {
+        // This may happen if we received a signal in the middle of an escape sequence or other
+        // multi-char binding. Move these non-char events to the front of the queue, handle them
+        // first, and then later we'll return and try the sequence again. See #8628.
+        peeker.consume();
+        this->promote_interruptions_to_front();
+        return;
+    }
+
     FLOGF(reader, L"no generic found, ignoring char...");
     auto evt = peeker.next();
     peeker.consume();

src/input_common.cpp

@@ -255,6 +255,15 @@ void input_event_queue_t::push_back(const char_event_t& ch) { queue_.push_back(c
 
 void input_event_queue_t::push_front(const char_event_t& ch) { queue_.push_front(ch); }
 
+void input_event_queue_t::promote_interruptions_to_front() {
+    // Find the first sequence of non-char events.
+    // EOF is considered a char: we don't want to pull EOF in front of real chars.
+    auto is_char = [](const char_event_t& ch) { return ch.is_char() || ch.is_eof(); };
+    auto first = std::find_if_not(queue_.begin(), queue_.end(), is_char);
+    auto last = std::find_if(first, queue_.end(), is_char);
+    std::rotate(queue_.begin(), first, last);
+}
+
 void input_event_queue_t::prepare_to_select() {}
 void input_event_queue_t::select_interrupted() {}
 input_event_queue_t::~input_event_queue_t() = default;

src/input_common.h

@@ -207,6 +207,9 @@ class input_event_queue_t {
     /// will be the next character returned by readch.
     void push_front(const char_event_t &ch);
 
+    /// Find the first sequence of non-char events, and promote them to the front.
+    void promote_interruptions_to_front();
+
     /// Add multiple characters or readline events to the front of the queue of unread characters.
     /// The order of the provided events is not changed, i.e. they are not inserted in reverse
     /// order.

tests/pexpects/torn_escapes.py

@@ -0,0 +1,81 @@
+#!/usr/bin/env python3
+import os
+import signal
+from pexpect_helper import SpawnedProc
+
+
+sp = SpawnedProc()
+send, sendline, sleep, expect_prompt, expect_str, expect_re = (
+    sp.send,
+    sp.sendline,
+    sp.sleep,
+    sp.expect_prompt,
+    sp.expect_str,
+    sp.expect_re,
+)
+# Ensure that signals don't tear escape sequences. See #8628.
+expect_prompt()
+
+# Allow for long delays before matching escape.
+sendline(r"set -g fish_escape_delay_ms 2000")
+expect_prompt()
+
+# Set up a handler for SIGUSR1.
+sendline(r"set -g sigusr1_count 0")
+expect_prompt()
+
+sendline(
+    r"""
+    function usr1_handler --on-signal SIGUSR1;
+        set sigusr1_count (math $sigusr1_count + 1);
+        echo Got SIGUSR1 $sigusr1_count;
+        commandline -f repaint;
+    end
+""".strip().replace(
+        "\n", os.linesep
+    )
+)
+expect_prompt()
+
+# Set up a wacky binding with an escape.
+sendline(r"function wacky_handler; echo Wacky Handler; end")
+expect_prompt()
+
+sendline(r"bind abc\edef wacky_handler")
+expect_prompt()
+
+# We can respond to SIGUSR1.
+os.kill(sp.spawn.pid, signal.SIGUSR1)
+expect_str(r"Got SIGUSR1 1")
+sendline(r"")
+expect_prompt()
+
+# Our wacky binding works.
+send("abc\x1bdef")
+expect_str(r"Wacky Handler")
+sendline(r"")
+expect_prompt()
+
+# Now we interleave the sequence with SIGUSR1.
+# What we expect to happen is that the signal is "shuffled to the front" ahead of the key events.
+send("abc")
+sleep(0.05)
+os.kill(sp.spawn.pid, signal.SIGUSR1)
+sleep(0.05)
+send("\x1bdef")
+expect_str(r"Got SIGUSR1 2")
+expect_str(r"Wacky Handler")
+sendline(r"")
+expect_prompt()
+
+# As before but it comes after the ESC.
+# The signal will arrive while we are waiting in getch_timed().
+send("abc\x1b")
+sleep(0.05)
+os.kill(sp.spawn.pid, signal.SIGUSR1)
+sleep(0.05)
+send("def")
+expect_str(r"Got SIGUSR1 3")
+expect_str(r"Wacky Handler")
+sendline(r"")
+expect_prompt()