feat: implement true async signal handling

[georglauterbach]

This makes handling signals like SIGUSR2 truly async, i.e. flyline does not wait anymore before the signal handler is called but instead calls it directly.

Disclaimer: The changes in this MR are assisted by Claude Opus 5. It does not show up in the commit because I did not let it commit - sorry!

Closes #852

[HalFrgrd]

Thanks for the PR. This changes a few things that I want to make sure we get right. It seems like the current behaviour when set a sigusr2 trap is that flyline does nothing till the app exits and then bash runs the trap function? And your desired behaviour is that flyline someone picks on the signal so that the signal can take effect immediately? If thats the case, I propose that flyline picks up the signal the next time it stops polling, then it exits, lets bash take over, bash runs the trap, then flyline takes over when bash needs another char, and flyline app starts up again.

I think this would be a much cleaner design. Flyline could exit with something like SignalHandlingRequired(current_buffer) so that we can pick up on it. I dont think fyline should try and handle the signal itself by calling the bash func. Is there a need for shell::backend().run_pending_traps() ?

We could put current_buffer in settings.initial_buffer so that flyline can pick up where it left off. Maybe flyline should leave teh cursor at the top of the inline viewport and clear the screen before it exits with SignalHandlingRequired. So that it would effectively pick up where it left off.

[HalFrgrd]

I think this would effectively disable the 5s refresh rate on idle?

If we really want flyline to always be responsive to signals, then we can add a setting to control idle fps: flyline --set-idle-frame-rate default to 0.2.

[HalFrgrd]

with my proposed solution, I think all this goes away.

[HalFrgrd]

How would this interact with DEBUG traps set by bash? When someone launches a bash function via https://github.com/HalFrgrd/flyline#launching-third-party-apps, will the trap trigger?

[georglauterbach]

In short: Yes, your design proposals work. I'd factor out the two existing bug fixes into two PRs (or one) (see details below) and make the changes in this PRs leaner. SignalHandlingRequired(buffer) works, run_pending_traps() is not necessary, and we can use EventReader::waker() instead of a dedicated setting.

I have collected and attached my findings while going over your comments with Claude. To not make this comment too big, I have put them behind a collapsible.

If you're fine with factoring out the two fixes first, I'll open separate PRs and then come back to this one. Just let me know :)

Agent Answers

Short version

HalFrgrd's core design proposal is the right call, and I'd take it — but for a different reason than he gives, and it doesn't make the whole PR go away. Two of the three things in this branch are independent bug fixes that survive his redesign. There's also a real bug in run_pending_traps() as written, and his objection to the 100 ms poll cap has a fixable root cause that neither of you named.

Comment 1 — design: exit with SignalHandlingRequired(current_buffer)

#issuecomment-5302913358

His reading of current behavior is correct. Confirmed in bash's eval.c: parse_command() calls run_pending_traps() before parsing the next command, so pending traps fire on the next prompt cycle, after flyline returns a line. That's exactly the "nothing happens until I run a command" symptom from #852.

His design works, and the mechanism is sound. Return an empty line, bash's read_command → parse_command runs the pending traps before re-prompting, then calls flyline_get_char again. Even an empty line triggers it, because run_pending_traps() runs at the top of parse_command(), not from command execution.

The stronger argument for his design than "cleaner": running traps in place is genuinely unsafe. check_signals_and_traps() → check_signals() → QUIT, and QUIT calls throw_to_top_level() when interrupt_state is set (and termsig_handler() when terminating_signal is set). Both siglongjmp out of C, straight through the Rust frames of App::run. That skips every destructor:

• the terminal stays in raw mode with the inline viewport half-drawn;
• BASH_LOCK — a parking_lot::ReentrantMutex that funcs::check_signals_and_traps holds at that moment — is never released, so every worker thread that later touches bash globals blocks forever.

The PR checks interrupt_pending() before calling the hook, which narrows the window but doesn't close it (a signal can land between the check and the call). Exiting first, restoring the terminal, and letting bash run traps from its own reader loop removes this entire class of hazard, plus it gives trap bodies a cooked terminal instead of a raw-mode TUI to print into.

Answer to "Is there a need for shell::backend().run_pending_traps()?" No, not under his design — parse_command() does it. You still need first_pending_trap() (available since bash 4.2, so fine for the 4.4+ matrix) as a detector, but never the invocation. That deletes most of the src/shell/bash/funcs.rs change from this PR.

Four costs to raise before committing to exit-and-restart

1. parse_command() also calls execute_prompt_command(), so PROMPT_COMMAND re-runs on every signal. For the question: updating flyline from signal handlers #852 use case (theme switch, a few times a day) that's nothing, but any periodic trap makes it a treadmill.
2. read_command() re-arms alarm(TMOUT) on each cycle, so a recurring trap silently defeats TMOUT.
3. App::run emits shell-integration codes at startup (write_startup_codes) and exit (write_on_exit_codes), so each restart injects a spurious prompt mark into terminals doing OSC 133 semantic zones.
4. settings.initial_buffer restores text only. Cursor offset, undo stack, kill ring, an open completion menu, and history-search state are all lost. His note about cursor placement covers the viewport, not the buffer cursor. At minimum this needs an initial_cursor alongside initial_buffer.

None of these are blockers for the theme-switch case; they should just be acknowledged, and 1–3 argue for restarting only when a trap is actually pending, never speculatively.

Comment 2 — inline: "this would effectively disable the 5s refresh rate on idle"

#discussion_r3789655339 on src/app/mod.rs

Half right, and the fix is better than the setting he proposes. Redraws still respect the idle rate — line 1164 gates on min_refresh_rate, not on the poll timeout, so IDLE_FRAME_RATE still controls drawing. What the cap actually destroys is the sleep: the process wakes 10 times a second forever, in every interactive shell, which is what the 0.2 fps idle path existed to avoid. So his instinct to reject it is correct.

But --set-idle-frame-rate doesn't fix it either; it just picks a point on a bad tradeoff curve (0.2 fps means up to 5 s of latency, 1 fps means wasted wakeups plus 1 s of latency). Signal-driven wakeup dominates both: zero latency and zero extra wakeups.

Why the cap was needed at all (unstated in the thread): the Err(ErrorKind::Interrupted) arm added to the match is dead code.

• termina's Unix event source swallows EINTR internally (Err(err) if err.kind() == Interrupted => continue in event/source/unix.rs), and Shared::poll maps its own wake-pipe interrupt to Ok(false).
• The SIGALRM handler is installed with libc::signal, which on glibc carries BSD semantics and sets SA_RESTART, so the syscall restarts anyway.

A signal simply cannot surface as Interrupted here, hence the 100 ms band-aid.

The clean fix is already in the dependency. EventReader::waker() returns a UnixWaker backed by a self-pipe that poll selects on. Chain a handler onto the trapped signals that calls bash's trap_handler and then wakes that pipe, and the idle poll can go back to blocking for the full frame interval while still reacting in microseconds. Two caveats:

• UnixWaker::wake() takes a parking_lot::Mutex before writing, so it is not strictly async-signal-safe — a dedicated self-pipe with a raw write(2) in the handler is the safe version;
• the handler must chain rather than replace bash's, or pending_traps[] never gets set.

This wake mechanism is needed under either design; his proposal doesn't avoid it, since exiting promptly still requires noticing the signal promptly.

Comment 3 — inline: "with my proposed solution, I think all this goes away"

#discussion_r3789655980 on src/lib.rs

Partly true, and this is where I'd push back. The re-entrancy hazard is not created by this PR — it exists on master today. flyline_get_char holds the FLYLINE_INSTANCE_PTR guard across boxed.get(), and run_bash_command (src/app/mod.rs:1294) runs arbitrary shell code via evalstring from inside that call. So any user who binds runBashCommand(some_func) where some_func calls the flyline builtin — flyline set-style in a theme helper is exactly the plausible case — hits flyline_call_command → Mutex::lock() on a thread that already holds it, and the shell hangs. The completion path at src/app/mod.rs:1788 does the same thing.

His design removes the trap-driven route into that deadlock, not the deadlock. I'd split the lib.rs change into its own PR with a repro, and keep the TLS approach (it correctly covers same-thread re-entry; worth noting in the comment that a worker thread calling the builtin would still block, which is fine today since only the main thread evaluates shell strings).

Same argument for the SIGALRM work. alrm_catcher is a signal handler, not a pending trap, so it longjmps the moment SIGALRM is delivered no matter what flyline's exit strategy is. Without interception, a TMOUT expiry during editing longjmps out of the TUI and logs you out with the terminal in raw mode. That's an independent bug fix his redesign does not cover, and it deserves its own PR too. One nit: use sigaction with save/restore of the full struct sigaction, matching the existing SigchldGuard in lib.rs, rather than libc::signal, which loses bash's sa_mask/flags on restore.

So: three PRs, not one — TMOUT/SIGALRM safety, the re-entrancy deadlock, and then the actual async trap delivery, which by then is small.

Comment 4 — DEBUG traps and third-party app launching

#issuecomment-5302928139

Two separate answers, and both are "no change from today."

Will the new code fire DEBUG traps? No. run_pending_traps() iterates for (sig = 1; sig < NSIG; sig++) over pending_traps[], and DEBUG/ERR/RETURN/EXIT are pseudo-signals stored at indices >= NSIG (DEBUG_TRAP == NSIG). They are driven by run_debug_trap() from execute_command_internal, never by the pending-trap path. Neither first_pending_trap() nor check_signals_and_traps() can trigger one.

Will a DEBUG trap fire for runBashCommand(__atuin_widget_run)? Yes — and it already does on master, unchanged by this PR. evaluate_shell_string calls evalstring/parse_and_execute, which goes through execute_command_internal, which runs the DEBUG trap per simple command. That matches stock bash's bind -x, which uses the same parse_and_execute path, so it's readline parity rather than a flyline quirk. Worth an explicit test though, since bash-preexec (which Atuin installs) hangs its preexec hooks off the DEBUG trap — so a runBashCommand binding plausibly fires preexec for the widget itself.

The one genuinely new interaction is under the in-place design: a SIGUSR2 trap body running mid-edit would fire DEBUG traps while the TUI owns the terminal in raw mode, so bash-preexec could print into the viewport and Atuin could record a phantom command start. Under HalFrgrd's exit-first design that runs on a clean cooked terminal instead — another point in its favor.

Bug to fix regardless of which design wins

pub fn run_pending_traps() -> bool {
    if let Some(hook) = rl_signal_event_hook() {
        log::info!("Calling rl_signal_event_hook for pending Bash signal/trap");
        let _guard = super::symbols::BASH_LOCK.lock();
        hook();
        return true;
    }

This returns true unconditionally whenever the hook is non-null, so the caller sets needs_full_redraw = true on every loop iteration — continuous full redraws, BASH_LOCK acquisition, and an info-level log line ten times a second. It's masked today only because flyline swaps bash_input.getter and bash never calls initialize_readline(), leaving rl_signal_event_hook NULL, so the code silently falls through to the first_pending_trap() branch. Anything that initializes readline (the bind builtin, programmable completion) flips it on. Since bash_event_hook is just check_signals_and_traps() plus terminating-signal handling — no more than the other branch, but with the longjmp risk — drop the hook path entirely and gate on first_pending_trap() > 0.

Suggested Strategy

1. Yes to SignalHandlingRequired(buffer), and no to run_pending_traps() since parse_command covers it.
2. longjmp makes exit-first the safer design rather than merely cleaner.
3. Note the lib.rs re-entrancy fix and the TMOUT fix are pre-existing bugs that outlive the redesign — land them as two small separate PRs, leaving this one as just the trap-delivery change.
4. On the idle-fps knob, counter-propose the waker: fewer lines than a new setting, and strictly better on both latency and battery.

Evidence trail

Claim	Source
Traps run on the next prompt cycle	eval.c:344, run_pending_traps() at top of parse_command()
PROMPT_COMMAND re-runs per cycle	eval.c:360, execute_prompt_command() in parse_command()
TMOUT alarm re-armed per cycle	eval.c:405-406, read_command()
DEBUG traps excluded from pending path	trap.c:375, for (sig = 1; sig < NSIG; sig++); DEBUG_TRAP == NSIG
check_signals_and_traps can longjmp	trap.c:669-684, check_signals() → QUIT
first_pending_trap exists since bash 4.2	bash CHANGES; verified present in local bash 5.2 via nm -D
termina swallows EINTR, exposes a waker	termina-0.3.3/src/event/source/unix.rs:99, :39-43

[HalFrgrd]

That sounds good to me. And if you could ask claude to be concise and not put in too many unnecessary comments, that would be appreciated :)

[georglauterbach]

Converted to draft until #933 is merged. Once this PR is then merged, I will add a third one with the waker logic.

[georglauterbach]

Closing as functionality was added already, see #852