Skip to content

Commit 4497257

Browse files
jingle2008claude
andcommitted
fix(cli): make stderr redirect portable + survive log rotation
Code review feedback on the inline redirectStderr from 13967d0: - Critical: syscall.Dup/syscall.Dup2 don't exist on linux/arm64 (kernel only exposes dup3) and windows. Splits the redirect into redirect_stderr_unix.go (uses golang.org/x/sys/unix.Dup/Dup2, which abstracts to dup3 on linux/arm64) and redirect_stderr_other.go (no-op stub). Verified clean builds for darwin, linux/arm64, windows/amd64. - Important: write captured stderr to "<cfg.LogFile>.stderr" instead of the rotated log itself, so lumberjack can't rename/gzip the file out from under our cached fd 2 mid-session. - Minor: move redirectStderr() call before tui.NewModel so a bad log path fails fast before the production loader spawns goroutines. - Minor: re-attach the orphaned docstring on handleSpinnerTickMsg (separated by a blank line, godoc didn't pick it up). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 6b6c773 commit 4497257

5 files changed

Lines changed: 78 additions & 44 deletions

File tree

go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ require (
2020
go.uber.org/zap v1.28.0
2121
golang.org/x/exp v0.0.0-20260312153236-7ab1446f8b90
2222
golang.org/x/sync v0.20.0
23+
golang.org/x/sys v0.45.0
2324
golang.org/x/tools v0.45.0
2425
gopkg.in/natefinch/lumberjack.v2 v2.2.1
2526
k8s.io/api v0.36.1
@@ -284,7 +285,6 @@ require (
284285
golang.org/x/mod v0.36.0 // indirect
285286
golang.org/x/net v0.55.0 // indirect
286287
golang.org/x/oauth2 v0.36.0 // indirect
287-
golang.org/x/sys v0.45.0 // indirect
288288
golang.org/x/telemetry v0.0.0-20260508192327-42602be52be6 // indirect
289289
golang.org/x/term v0.43.0 // indirect
290290
golang.org/x/text v0.37.0 // indirect
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
//go:build !unix
2+
3+
package cli
4+
5+
// redirectStderr is a no-op on non-unix platforms. The fd-level
6+
// redirection used on unix targets relies on dup2 syscall semantics
7+
// that aren't portable to windows/plan9/etc. Anyone running the TUI
8+
// on a non-unix host will see plugin stderr leak to their terminal —
9+
// acceptable tradeoff for a TUI primarily targeting unix.
10+
func redirectStderr(_ string) (func(), error) {
11+
return func() {}, nil
12+
}
Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
//go:build unix
2+
3+
package cli
4+
5+
import (
6+
"fmt"
7+
"os"
8+
9+
"golang.org/x/sys/unix"
10+
)
11+
12+
// redirectStderr points the process's fd 2 at the given file for the
13+
// duration of the returned closure's lifetime. It's used only by the
14+
// TUI path: bubbletea's alt-screen has exclusive ownership of the
15+
// terminal, so any bytes written to fd 2 by client-go's exec auth
16+
// plugins (oci-cli prints "Abort:" on a non-tty prompt failure) or
17+
// runtime panic stacks would otherwise interleave with bubbletea's
18+
// frame writes and corrupt the rendered display.
19+
//
20+
// The fd is swapped at the kernel level via unix.Dup2 — not via
21+
// reassigning os.Stderr — so child processes spawned by exec.Cmd
22+
// inherit the redirected fd. unix.Dup3-based on linux/arm64,
23+
// dup2-based on darwin and the other unix targets.
24+
func redirectStderr(path string) (func(), error) {
25+
f, err := os.OpenFile(path, os.O_APPEND|os.O_WRONLY|os.O_CREATE, 0o644)
26+
if err != nil {
27+
return nil, fmt.Errorf("open stderr sink %q: %w", path, err)
28+
}
29+
orig, err := unix.Dup(int(os.Stderr.Fd()))
30+
if err != nil {
31+
_ = f.Close()
32+
return nil, fmt.Errorf("dup stderr: %w", err)
33+
}
34+
if err := unix.Dup2(int(f.Fd()), int(os.Stderr.Fd())); err != nil {
35+
_ = f.Close()
36+
_ = unix.Close(orig)
37+
return nil, fmt.Errorf("redirect stderr: %w", err)
38+
}
39+
return func() {
40+
// Restore so any post-exit writes (e.g., logger Sync errors,
41+
// deferred cleanups) reach the user's actual terminal. Order
42+
// matters: restore fd 2 before closing f so any stderr write
43+
// between the dup2 and the close lands on the real terminal,
44+
// not a closed fd.
45+
_ = unix.Dup2(orig, int(os.Stderr.Fd()))
46+
_ = unix.Close(orig)
47+
_ = f.Close()
48+
}, nil
49+
}

internal/cli/root.go

Lines changed: 12 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -161,6 +161,18 @@ func runToolkit(ctx context.Context, logger logging.Logger, cfg config.Config, v
161161
"category", category,
162162
)
163163

164+
// Redirect process stderr to a sibling capture file for the
165+
// duration of the TUI session — done before model construction so
166+
// a misconfigured log-file path fails fast before the loader
167+
// spawns goroutines. We use cfg.LogFile + ".stderr" rather than
168+
// the rotated log itself so lumberjack can't rename/gzip the file
169+
// out from under our cached fd 2 mid-session.
170+
restoreStderr, err := redirectStderr(cfg.LogFile + ".stderr")
171+
if err != nil {
172+
return err
173+
}
174+
defer restoreStderr()
175+
164176
model, err := tui.NewModel(
165177
tui.WithRepoPath(repoPath),
166178
tui.WithKubeConfig(kubeConfig),
@@ -176,19 +188,6 @@ func runToolkit(ctx context.Context, logger logging.Logger, cfg config.Config, v
176188
logger.Errorw("failed to create toolkit model", "error", err)
177189
return fmt.Errorf("create toolkit model: %w", err)
178190
}
179-
// Redirect process stderr to the log file for the duration of the
180-
// TUI session. The bubbletea alt-screen has exclusive ownership of
181-
// the terminal; any bytes written to fd 2 by client-go's exec
182-
// auth plugins (oci-cli prints "Abort:" on a non-tty prompt
183-
// failure, for instance) or by runtime panic stacks would otherwise
184-
// interleave with bubbletea's frame writes. Restored on exit so
185-
// post-cleanup writes still reach the user's terminal.
186-
restoreStderr, err := redirectStderr(cfg.LogFile)
187-
if err != nil {
188-
return err
189-
}
190-
defer restoreStderr()
191-
192191
p := tea.NewProgram(model, tea.WithAltScreen(), tea.WithContext(ctx))
193192
_, err = p.Run()
194193
if err != nil && !errors.Is(err, context.Canceled) {
@@ -198,27 +197,3 @@ func runToolkit(ctx context.Context, logger logging.Logger, cfg config.Config, v
198197
return nil
199198
}
200199

201-
// redirectStderr points the process's fd 2 at the given file for the
202-
// duration of the returned function's lifetime. The restoration
203-
// function dup2's the original fd back and closes both sides.
204-
func redirectStderr(path string) (func(), error) {
205-
f, err := os.OpenFile(path, os.O_APPEND|os.O_WRONLY|os.O_CREATE, 0o644)
206-
if err != nil {
207-
return nil, fmt.Errorf("open stderr sink %q: %w", path, err)
208-
}
209-
orig, err := syscall.Dup(int(os.Stderr.Fd()))
210-
if err != nil {
211-
_ = f.Close()
212-
return nil, fmt.Errorf("dup stderr: %w", err)
213-
}
214-
if err := syscall.Dup2(int(f.Fd()), int(os.Stderr.Fd())); err != nil {
215-
_ = f.Close()
216-
_ = syscall.Close(orig)
217-
return nil, fmt.Errorf("redirect stderr: %w", err)
218-
}
219-
return func() {
220-
_ = syscall.Dup2(orig, int(os.Stderr.Fd()))
221-
_ = syscall.Close(orig)
222-
_ = f.Close()
223-
}, nil
224-
}

internal/ui/tui/update_loading.go

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,10 @@ func (m *Model) updateLoadingView(msg tea.Msg) (tea.Model, tea.Cmd) {
2020
return m, nil
2121
}
2222

23-
// Spinner/stopwatch ticks are self-perpetuating: Update on a TickMsg
24-
// returns the next Tick cmd. We let that chain die when no load is in
25-
// flight so we don't burn ~10Hz of empty event-loop wakeups idle.
26-
// beginTask kicks off a fresh chain via tea.Sequence the next time
27-
// pendingTasks goes 0 → 1.
28-
23+
// handleSpinnerTickMsg advances the spinner one frame and lets the
24+
// tick chain die when no load is in flight, so we don't burn empty
25+
// event-loop wakeups idle. beginTask kicks off a fresh chain via
26+
// tea.Sequence the next time pendingTasks goes 0 → 1.
2927
func (m *Model) handleSpinnerTickMsg(msg spinner.TickMsg) tea.Cmd {
3028
loadingSpinner, cmd := m.loadingSpinner.Update(msg)
3129
m.loadingSpinner = &loadingSpinner

0 commit comments

Comments
 (0)