fix(term): reset visual modes on the fd when the batched writer fails
EnterRaw's two error paths only restored termios, so a mode sequence that had already reached the terminal left it on the alt screen with autowrap off — state that outlives the process. restore() had the same hole: a failed write or flush was reported but never retried through another channel. Fall back to writing the exit sequence straight to the descriptor, past the writer that just failed. Best effort by construction, since that descriptor is usually broken too; both modes are idempotent, so replaying them when nothing had been emitted is harmless. Deliberately does not keep `entered` set for a retry: makeRestoreLocked's sync.Once and the single stopResizeLocked call are what order the resize watcher's start against its stop, and the sink is an *os.File — poll.FD.Write loops to completion, so a write error means a broken descriptor, not a busy one. Retrying would fail identically and cost the idempotency contract. Tests poison t.bw while leaving t.out intact, which isolates the fallback: the buffered path fails and the real descriptor records what was attempted.
- commit
- 28ffa89a0c604b2c177053ec38c89d70c52fa87a
- author
- brice · · 2026-07-25 14:14 UTC
- parent
- fd7fd71
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
2 files · +87 -0
M internal/adapters/term/terminal.go +17 -0
| @@ -116,10 +116,12 @@ func (t *Terminal) EnterRaw() (func() error, error) { | |||
| 116 | 116 | t.orig = old | |
| 117 | 117 | ||
| 118 | 118 | if _, err := t.bw.WriteString(altScreenEnter + autowrapDisable + cursorHide + mouseEnable + bracketedPasteEnable + colorSchemeEnable); err != nil { | |
| 119 | + | t.resetVisualModesDirect() | |
| 119 | 120 | _ = t.restoreRawLocked() | |
| 120 | 121 | return nil, fmt.Errorf("term: enter alt screen: %w", err) | |
| 121 | 122 | } | |
| 122 | 123 | if err := t.bw.Flush(); err != nil { | |
| 124 | + | t.resetVisualModesDirect() | |
| 123 | 125 | _ = t.restoreRawLocked() | |
| 124 | 126 | return nil, fmt.Errorf("term: enter alt screen: %w", err) | |
| 125 | 127 | } | |
| @@ -129,6 +131,18 @@ func (t *Terminal) EnterRaw() (func() error, error) { | |||
| 129 | 131 | return t.restoreFn, nil | |
| 130 | 132 | } | |
| 131 | 133 | ||
| 134 | + | // resetVisualModesDirect returns the terminal to the normal screen with | |
| 135 | + | // autowrap on, writing straight to the descriptor instead of through the | |
| 136 | + | // batched writer. It is the fallback for a writer that has already failed: | |
| 137 | + | // part of a mode sequence may have reached the terminal, and leaving it on | |
| 138 | + | // the alt screen or with autowrap off would outlive the process. Both modes | |
| 139 | + | // are idempotent, so replaying them when nothing was emitted is harmless. | |
| 140 | + | // Best effort by construction — the descriptor is usually broken too. | |
| 141 | + | // Must be called with t.mu held. | |
| 142 | + | func (t *Terminal) resetVisualModesDirect() { | |
| 143 | + | _, _ = t.out.WriteString(autowrapEnable + altScreenExit) | |
| 144 | + | } | |
| 145 | + | ||
| 132 | 146 | // makeRestoreLocked returns an idempotent restore closure for the | |
| 133 | 147 | // current EnterRaw "session". Must be called with t.mu held. | |
| 134 | 148 | func (t *Terminal) makeRestoreLocked() func() error { | |
| @@ -151,6 +165,9 @@ func (t *Terminal) restore() error { | |||
| 151 | 165 | ||
| 152 | 166 | _, werr := t.bw.WriteString(cursorShow + cursorStyleDefault + mouseDisable + bracketedPasteDisable + colorSchemeDisable + autowrapEnable + altScreenExit) | |
| 153 | 167 | ferr := t.bw.Flush() | |
| 168 | + | if werr != nil || ferr != nil { | |
| 169 | + | t.resetVisualModesDirect() | |
| 170 | + | } | |
| 154 | 171 | ||
| 155 | 172 | rerr := t.restoreRawLocked() | |
| 156 | 173 | ||
| @@ -116,10 +116,12 @@ func (t *Terminal) EnterRaw() (func() error, error) { | |||||
| 116 | t.orig = old | 116 | t.orig = old | ||
| 117 | 117 | ||||
| 118 | if _, err := t.bw.WriteString(altScreenEnter + autowrapDisable + cursorHide + mouseEnable + bracketedPasteEnable + colorSchemeEnable); err != nil { | 118 | if _, err := t.bw.WriteString(altScreenEnter + autowrapDisable + cursorHide + mouseEnable + bracketedPasteEnable + colorSchemeEnable); err != nil { | ||
| 119 | + | t.resetVisualModesDirect() | |||
| 119 | _ = t.restoreRawLocked() | 120 | _ = t.restoreRawLocked() | ||
| 120 | return nil, fmt.Errorf("term: enter alt screen: %w", err) | 121 | return nil, fmt.Errorf("term: enter alt screen: %w", err) | ||
| 121 | } | 122 | } | ||
| 122 | if err := t.bw.Flush(); err != nil { | 123 | if err := t.bw.Flush(); err != nil { | ||
| 124 | + | t.resetVisualModesDirect() | |||
| 123 | _ = t.restoreRawLocked() | 125 | _ = t.restoreRawLocked() | ||
| 124 | return nil, fmt.Errorf("term: enter alt screen: %w", err) | 126 | return nil, fmt.Errorf("term: enter alt screen: %w", err) | ||
| 125 | } | 127 | } | ||
| @@ -129,6 +131,18 @@ func (t *Terminal) EnterRaw() (func() error, error) { | |||||
| 129 | return t.restoreFn, nil | 131 | return t.restoreFn, nil | ||
| 130 | } | 132 | } | ||
| 131 | 133 | ||||
| 134 | + | // resetVisualModesDirect returns the terminal to the normal screen with | |||
| 135 | + | // autowrap on, writing straight to the descriptor instead of through the | |||
| 136 | + | // batched writer. It is the fallback for a writer that has already failed: | |||
| 137 | + | // part of a mode sequence may have reached the terminal, and leaving it on | |||
| 138 | + | // the alt screen or with autowrap off would outlive the process. Both modes | |||
| 139 | + | // are idempotent, so replaying them when nothing was emitted is harmless. | |||
| 140 | + | // Best effort by construction — the descriptor is usually broken too. | |||
| 141 | + | // Must be called with t.mu held. | |||
| 142 | + | func (t *Terminal) resetVisualModesDirect() { | |||
| 143 | + | _, _ = t.out.WriteString(autowrapEnable + altScreenExit) | |||
| 144 | + | } | |||
| 145 | + | ||||
| 132 | // makeRestoreLocked returns an idempotent restore closure for the | 146 | // makeRestoreLocked returns an idempotent restore closure for the | ||
| 133 | // current EnterRaw "session". Must be called with t.mu held. | 147 | // current EnterRaw "session". Must be called with t.mu held. | ||
| 134 | func (t *Terminal) makeRestoreLocked() func() error { | 148 | func (t *Terminal) makeRestoreLocked() func() error { | ||
| @@ -151,6 +165,9 @@ func (t *Terminal) restore() error { | |||||
| 151 | 165 | ||||
| 152 | _, werr := t.bw.WriteString(cursorShow + cursorStyleDefault + mouseDisable + bracketedPasteDisable + colorSchemeDisable + autowrapEnable + altScreenExit) | 166 | _, werr := t.bw.WriteString(cursorShow + cursorStyleDefault + mouseDisable + bracketedPasteDisable + colorSchemeDisable + autowrapEnable + altScreenExit) | ||
| 153 | ferr := t.bw.Flush() | 167 | ferr := t.bw.Flush() | ||
| 168 | + | if werr != nil || ferr != nil { | |||
| 169 | + | t.resetVisualModesDirect() | |||
| 170 | + | } | |||
| 154 | 171 | ||||
| 155 | rerr := t.restoreRawLocked() | 172 | rerr := t.restoreRawLocked() | ||
| 156 | 173 | ||||
M internal/adapters/term/terminal_test.go +70 -0
| @@ -2,6 +2,7 @@ package term | |||
| 2 | 2 | ||
| 3 | 3 | import ( | |
| 4 | 4 | "bytes" | |
| 5 | + | "errors" | |
| 5 | 6 | "io" | |
| 6 | 7 | "os" | |
| 7 | 8 | "strings" | |
| @@ -144,6 +145,75 @@ func TestTerminal_EnterRaw_DisablesAutowrapInsideAltScreen(t *testing.T) { | |||
| 144 | 145 | } | |
| 145 | 146 | } | |
| 146 | 147 | ||
| 148 | + | // failingWriter fails every write, standing in for a tty that stops | |
| 149 | + | // accepting output midway through a mode change. | |
| 150 | + | type failingWriter struct{} | |
| 151 | + | ||
| 152 | + | func (failingWriter) Write(p []byte) (int, error) { | |
| 153 | + | return 0, errors.New("sink failed") | |
| 154 | + | } | |
| 155 | + | ||
| 156 | + | // When the batched writer fails, part of the mode sequence may already have | |
| 157 | + | // reached the terminal, so the visual modes must be reset straight to the fd | |
| 158 | + | // rather than through the writer that just failed. Poisoning tm.bw while | |
| 159 | + | // leaving tm.out intact isolates that fallback: the buffered path fails, the | |
| 160 | + | // real descriptor still records what the fallback attempted. | |
| 161 | + | func TestTerminal_WriteFailure_AttemptsVisualResetOnFd(t *testing.T) { | |
| 162 | + | for _, tc := range []struct { | |
| 163 | + | name string | |
| 164 | + | poisonAfter bool // poison the writer after EnterRaw, i.e. fail during restore | |
| 165 | + | want string | |
| 166 | + | }{ | |
| 167 | + | { | |
| 168 | + | name: "enter fails before the session starts", | |
| 169 | + | want: autowrapEnable + altScreenExit, | |
| 170 | + | }, | |
| 171 | + | { | |
| 172 | + | name: "restore fails after a successful enter", | |
| 173 | + | poisonAfter: true, | |
| 174 | + | want: altScreenEnter + autowrapDisable + cursorHide + mouseEnable + | |
| 175 | + | bracketedPasteEnable + colorSchemeEnable + autowrapEnable + altScreenExit, | |
| 176 | + | }, | |
| 177 | + | } { | |
| 178 | + | t.Run(tc.name, func(t *testing.T) { | |
| 179 | + | inR, inW, err := os.Pipe() | |
| 180 | + | if err != nil { | |
| 181 | + | t.Fatalf("os.Pipe(in): %v", err) | |
| 182 | + | } | |
| 183 | + | defer func() { _ = inW.Close() }() | |
| 184 | + | defer func() { _ = inR.Close() }() | |
| 185 | + | ||
| 186 | + | outR, outW, captured, done := pipeCapture(t) | |
| 187 | + | defer func() { _ = outR.Close() }() | |
| 188 | + | ||
| 189 | + | tm := NewWithFiles(inR, outW) | |
| 190 | + | ||
| 191 | + | if !tc.poisonAfter { | |
| 192 | + | tm.bw = newBatchWriter(failingWriter{}, bufSize) | |
| 193 | + | if _, err := tm.EnterRaw(); err == nil { | |
| 194 | + | t.Fatalf("EnterRaw returned nil error on a failing sink") | |
| 195 | + | } | |
| 196 | + | } else { | |
| 197 | + | restore, err := tm.EnterRaw() | |
| 198 | + | if err != nil { | |
| 199 | + | t.Fatalf("EnterRaw: %v", err) | |
| 200 | + | } | |
| 201 | + | tm.bw = newBatchWriter(failingWriter{}, bufSize) | |
| 202 | + | if err := restore(); err == nil { | |
| 203 | + | t.Fatalf("restore returned nil error on a failing sink") | |
| 204 | + | } | |
| 205 | + | } | |
| 206 | + | ||
| 207 | + | _ = outW.Close() | |
| 208 | + | <-done | |
| 209 | + | ||
| 210 | + | if got := captured.String(); got != tc.want { | |
| 211 | + | t.Fatalf("bytes written to the fd = %q, want %q", got, tc.want) | |
| 212 | + | } | |
| 213 | + | }) | |
| 214 | + | } | |
| 215 | + | } | |
| 216 | + | ||
| 147 | 217 | func TestTerminal_EnterRaw_IsIdempotentAcrossCalls(t *testing.T) { | |
| 148 | 218 | inR, inW, err := os.Pipe() | |
| 149 | 219 | if err != nil { | |
| @@ -2,6 +2,7 @@ package term | |||||
| 2 | 2 | ||||
| 3 | import ( | 3 | import ( | ||
| 4 | "bytes" | 4 | "bytes" | ||
| 5 | + | "errors" | |||
| 5 | "io" | 6 | "io" | ||
| 6 | "os" | 7 | "os" | ||
| 7 | "strings" | 8 | "strings" | ||
| @@ -144,6 +145,75 @@ func TestTerminal_EnterRaw_DisablesAutowrapInsideAltScreen(t *testing.T) { | |||||
| 144 | } | 145 | } | ||
| 145 | } | 146 | } | ||
| 146 | 147 | ||||
| 148 | + | // failingWriter fails every write, standing in for a tty that stops | |||
| 149 | + | // accepting output midway through a mode change. | |||
| 150 | + | type failingWriter struct{} | |||
| 151 | + | ||||
| 152 | + | func (failingWriter) Write(p []byte) (int, error) { | |||
| 153 | + | return 0, errors.New("sink failed") | |||
| 154 | + | } | |||
| 155 | + | ||||
| 156 | + | // When the batched writer fails, part of the mode sequence may already have | |||
| 157 | + | // reached the terminal, so the visual modes must be reset straight to the fd | |||
| 158 | + | // rather than through the writer that just failed. Poisoning tm.bw while | |||
| 159 | + | // leaving tm.out intact isolates that fallback: the buffered path fails, the | |||
| 160 | + | // real descriptor still records what the fallback attempted. | |||
| 161 | + | func TestTerminal_WriteFailure_AttemptsVisualResetOnFd(t *testing.T) { | |||
| 162 | + | for _, tc := range []struct { | |||
| 163 | + | name string | |||
| 164 | + | poisonAfter bool // poison the writer after EnterRaw, i.e. fail during restore | |||
| 165 | + | want string | |||
| 166 | + | }{ | |||
| 167 | + | { | |||
| 168 | + | name: "enter fails before the session starts", | |||
| 169 | + | want: autowrapEnable + altScreenExit, | |||
| 170 | + | }, | |||
| 171 | + | { | |||
| 172 | + | name: "restore fails after a successful enter", | |||
| 173 | + | poisonAfter: true, | |||
| 174 | + | want: altScreenEnter + autowrapDisable + cursorHide + mouseEnable + | |||
| 175 | + | bracketedPasteEnable + colorSchemeEnable + autowrapEnable + altScreenExit, | |||
| 176 | + | }, | |||
| 177 | + | } { | |||
| 178 | + | t.Run(tc.name, func(t *testing.T) { | |||
| 179 | + | inR, inW, err := os.Pipe() | |||
| 180 | + | if err != nil { | |||
| 181 | + | t.Fatalf("os.Pipe(in): %v", err) | |||
| 182 | + | } | |||
| 183 | + | defer func() { _ = inW.Close() }() | |||
| 184 | + | defer func() { _ = inR.Close() }() | |||
| 185 | + | ||||
| 186 | + | outR, outW, captured, done := pipeCapture(t) | |||
| 187 | + | defer func() { _ = outR.Close() }() | |||
| 188 | + | ||||
| 189 | + | tm := NewWithFiles(inR, outW) | |||
| 190 | + | ||||
| 191 | + | if !tc.poisonAfter { | |||
| 192 | + | tm.bw = newBatchWriter(failingWriter{}, bufSize) | |||
| 193 | + | if _, err := tm.EnterRaw(); err == nil { | |||
| 194 | + | t.Fatalf("EnterRaw returned nil error on a failing sink") | |||
| 195 | + | } | |||
| 196 | + | } else { | |||
| 197 | + | restore, err := tm.EnterRaw() | |||
| 198 | + | if err != nil { | |||
| 199 | + | t.Fatalf("EnterRaw: %v", err) | |||
| 200 | + | } | |||
| 201 | + | tm.bw = newBatchWriter(failingWriter{}, bufSize) | |||
| 202 | + | if err := restore(); err == nil { | |||
| 203 | + | t.Fatalf("restore returned nil error on a failing sink") | |||
| 204 | + | } | |||
| 205 | + | } | |||
| 206 | + | ||||
| 207 | + | _ = outW.Close() | |||
| 208 | + | <-done | |||
| 209 | + | ||||
| 210 | + | if got := captured.String(); got != tc.want { | |||
| 211 | + | t.Fatalf("bytes written to the fd = %q, want %q", got, tc.want) | |||
| 212 | + | } | |||
| 213 | + | }) | |||
| 214 | + | } | |||
| 215 | + | } | |||
| 216 | + | ||||
| 147 | func TestTerminal_EnterRaw_IsIdempotentAcrossCalls(t *testing.T) { | 217 | func TestTerminal_EnterRaw_IsIdempotentAcrossCalls(t *testing.T) { | ||
| 148 | inR, inW, err := os.Pipe() | 218 | inR, inW, err := os.Pipe() | ||
| 149 | if err != nil { | 219 | if err != nil { | ||