Restore terminal state better. by amosbird · Pull Request #10603 · fish-shell/fish-shell

amosbird · 2024-07-04T14:23:59Z

Description

Restore terminal state better when running commands.

Only update TTY_MODES_FOR_EXTERNAL_CMDS when the command finishes successfully.
Pop a few entries as a best effort to disable Kitty's progressive enhancement.

TODOs:

Changes to fish usage are reflected in user documentation/manpages.
Tests have been added for regressions fixed
User-visible changes noted in CHANGELOG.rst

1. Only update TTY_MODES_FOR_EXTERNAL_CMDS when the command finishes successfully. 2. Pop a few entries as a best effort to disable Kitty's progressive enhancement.

src/reader.rs

krobelus · 2024-07-05T18:42:04Z

src/input_common.rs

            "\x1b[?2004l",
            "\x1b[>4;0m",
-            "\x1b[<1u", // Konsole breaks unless we pass an explicit number of entries to pop.
+            "\x1b[<9u", // Pop a few number of entries as a best effort to disable kitty progressive enhancement.


we should never need to pop more than one set of flags because we never push more than one set at a time.
It seems like the motivation for changing this is to work around buggy child processes, which I'm not sure we should do. The problems are that we'd mask bugs in child processes, and we'd break nesting (by popping flags set by a parent process).

I'm not sure what's your motivating example but to give a concrete one, if I run fish and then run Kakoune from fish, then a crashing Kakoune will leak progressive enhancement flags. It seems appropriate to fix the leak in Kakoune (fish does that when it panics, which covers most cases).

work around buggy child processes,

The goal is to address the issue of crashing child processes. This can be easily reproduced by running a child fish shell and then using kill -9 to terminate it. After doing this, any newly created child processes will always have the kitty keyboard protocol enabled, regardless of whether they support it or not.

any particular reason why your child process crashes? Usually it should be possible to clean up in a crash handler.

I think the expectation that a crashing child process not leak terminal state makes sense.
However we should not implement it by violating the protocol. Instead we could extend the protocol to maybe allow both getting the stack size and truncating the stack later

any particular reason why your child process crashes?

"Crash" is a general term in this context. A more specific example is the loss of connection to a remote shell.

right, when doing ssh -> fish, then a disconnect will leave the terminal in a bad state.
We could extend the protocol as described above and use that in SSH, to restore flags upon exit (assuming that flags are never leaked intentionally). Then it will work also if the local shell is not fish.
Until then, we could stop pushing protocols if $SSH_TTY is set ~~but $TMUX (or equivalent) is not?~~ nvm it also affects tmux. Is there any other relevant scenario?

Until then, we could stop pushing protocols if $SSH_TTY is set

This approach will only help with ssh -> fish since we are building fish, but it won't address issues for any other ssh -> kitty-keyboard-protocol-enabled-app.

We could extend the protocol as described above and use that in SSH

Yes, something like "\x1b[<0u" to clear the stack would be useful. However, the current workaround of disabling the extension, similar to disabling bracketed paste, is likely sufficient for now.

Yeah actually I think the benefits of the stack are somewhat theoretical, so if we don't find a better solution, we should abandon the stack and set/clear flags directly.

I think the best case is if we can change SSH to clean up. I've openend kovidgoyal/kitty#7603 to summarize my thoughts for a different audience.

As for gauging the importance of this issue. I think if ctrl-p stops working in bash that's pretty bad. The connection drop scenario is an everyday one, so we should fix it. Any crashes are not so important I think; those cases should be rare that it's reasonable to run reset or restart the terminal

I respectfully disagree; if we assume crashes don't happen then the bulk of the reasoning for managing terminal state ourselves goes away altogether.

if we assume crashes don't happen

we don't have numbers but the scenario of "SSH disconnect, then run a program that doesn't support CSI u" sounds like it could happen once in a while to the unsuspecting user.
At the same time, I'm still trying to find a tangible benefit for using the flag stack at all; it seems better to set/clear the flags directly, to fix the above scenario.

krobelus · 2024-07-07T11:39:07Z

src/reader.rs

    }

-    term_steal();
+    term_steal(eval_res.status.is_success());


actually your suggestion of copying modes only on success makes a lot of sense.
Do you maybe have a concrete example of bad modes copied, was it something from ssh too?

was it something from ssh too?

Yes.

ok but was it something that is fixed by term_fix_external_modes()? Since that's been re-added in latest master.
It's not very clearly defined what we should do here so I'd prefer a concrete motivation (like ECHO not being set or similar)

The term_fix_external_modes() function is incomplete. I attempted to manually restore additional options like echoe and echok. However, some options, such as ignpar and ignbrk, don't even exist in the rust libc package. Additionally, I'm unsure if more options need to be restored. Using stty sane results in a different set of options compared to the initialization of fish shell, which is why I opted for a different approach instead.

According to the discussion in #2315, we adopt TTY modes for external commands mainly for "stty". If our child process crashes (or SSH disconnect), we might get weird modes. Let's ignore the modes in the failure case. Co-authored-by: Johannes Altmanninger <aclopte@gmail.com> Part of #10603

Restore terminal state better.

43ec968

1. Only update TTY_MODES_FOR_EXTERNAL_CMDS when the command finishes successfully. 2. Pop a few entries as a best effort to disable Kitty's progressive enhancement.

krobelus self-requested a review July 5, 2024 18:25

krobelus reviewed Jul 5, 2024

View reviewed changes

krobelus mentioned this pull request Jul 7, 2024

Progressive Enhancement flag stack may leak on SSH connection drop kovidgoyal/kitty#7603

Closed

krobelus reviewed Jul 7, 2024

View reviewed changes

krobelus added this to the fish next-3.x milestone Jul 7, 2024

Use CSI = 0 u to clear kitty keyboard extension

89deb81

krobelus closed this in c3c8327 Aug 3, 2024

github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2025

Uh oh!

Conversation

amosbird commented Jul 4, 2024

Description

TODOs:

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krobelus Jul 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mqudsi Jul 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amosbird Jul 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

krobelus Jul 6, 2024 •

edited

Loading

mqudsi Jul 7, 2024 •

edited

Loading

amosbird Jul 7, 2024 •

edited

Loading