From 9089a546fbafde0a420448deefcdf5a76126bc4d Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Wed, 2 Sep 2026 22:12:51 -0700 Subject: [PATCH] shell: exit non-zero when a piped command fails (#11117) * shell: non-interactive mode exits non-zero when a command fails A failed command in a piped weed shell run printed 'error: ...' but the process still exited 0, so a CronJob wrapping e.g. echo 's3.lifecycle.run-shard -shards 0-15' | weed shell -master=... reported green while the run aborted partway (shards N+1..15 unwalked). An unknown command likewise exited 0. RunShell now returns the last command failure from the non-interactive stdin path (unknown commands included), and the shell command exits 2 on it. Interactive sessions are unchanged: errors are shown to the operator and the session continues, exiting 0 as before. * shell: route the piped-failure exit through main's shutdown path Review follow-up: os.Exit(2) inside the shell command skipped main's shutdown work. The command now records the status (SetCommandExitStatus) and returns normally; main applies it via setExitStatus before exit(). exit() itself now flushes sentry before os.Exit -- main's deferred sentry.Flush never ran on this path (os.Exit skips defers), so the existing 'flush buffered events before the program terminates' intent only worked for the autocomplete early-return. Exit status 2 on a failed piped run is preserved (verified: piped success exits 0, piped failing command exits 2). * shell: test the registered-command failure path Review follow-up: the error-propagation test only covered unknown commands. A fake registered command now drives processEachCmd's real dispatch path: a failing Do surfaces its exact error (errors.Is) and a succeeding one returns nil. The non-interactive exit status itself is main-level plumbing, verified end to end against the reproduction (piped failure exits 2). * shell: trim the comments added with the exit status Keep the non-obvious why -- why a piped run has to fail its wrapper, why the status is recorded instead of os.Exit'ed -- and drop the narration. Claude-Session: https://claude.ai/code/session_018DWwctzD4T2DmnczPRM47t * shell: fail a piped run with the status weed already uses for that weed.go spends 1 on a command that failed and 2 on a usage or syntax error, and runShell returns true precisely so the usage dump is skipped. Exiting 2 there told a wrapper the command line was wrong. Claude-Session: https://claude.ai/code/session_018DWwctzD4T2DmnczPRM47t --------- Co-authored-by: Carlos Leyva --- weed/command/command.go | 14 ++++++++ weed/command/shell.go | 6 +++- weed/shell/shell_liner.go | 31 ++++++++++------ weed/shell/shell_liner_test.go | 66 ++++++++++++++++++++++++++++++++++ weed/weed.go | 4 +++ 5 files changed, 110 insertions(+), 11 deletions(-) create mode 100644 weed/shell/shell_liner_test.go diff --git a/weed/command/command.go b/weed/command/command.go index daa3823a1..bc40e4c29 100644 --- a/weed/command/command.go +++ b/weed/command/command.go @@ -97,3 +97,17 @@ func (c *Command) Usage() { func (c *Command) Runnable() bool { return c.Run != nil } + +// Recorded, not os.Exit'ed, so the failure still travels through main's +// shutdown path. The highest requested status wins. +var commandExitStatus int + +func SetCommandExitStatus(n int) { + if n > commandExitStatus { + commandExitStatus = n + } +} + +func CommandExitStatus() int { + return commandExitStatus +} diff --git a/weed/command/shell.go b/weed/command/shell.go index 2645f4932..0162d1501 100644 --- a/weed/command/shell.go +++ b/weed/command/shell.go @@ -68,7 +68,11 @@ func runShell(command *Command, args []string) bool { fmt.Fprintf(os.Stderr, "master: %s filer: %s\n", *shellOptions.Masters, shellOptions.FilerAddress) } - shell.RunShell(shellOptions) + if err := shell.RunShell(shellOptions); err != nil { + // The command already printed the error; a piped run has to fail the + // script wrapping it too. + SetCommandExitStatus(1) + } return true diff --git a/weed/shell/shell_liner.go b/weed/shell/shell_liner.go index 9ea8ca873..c3e0d86d7 100644 --- a/weed/shell/shell_liner.go +++ b/weed/shell/shell_liner.go @@ -24,7 +24,9 @@ import ( var historyPath = path.Join(os.TempDir(), "weed-shell") -func RunShell(options ShellOptions) { +// Piped stdin returns the last command failure; an interactive session shows +// the error to the operator and keeps going. +func RunShell(options ShellOptions) error { slices.SortFunc(Commands, func(a, b command) int { return strings.Compare(a.Name(), b.Name()) }) @@ -94,7 +96,7 @@ func RunShell(options ShellOptions) { if err != io.EOF { fmt.Fprintf(os.Stderr, "%v\n", err) } - return + return nil } if strings.TrimSpace(cmd) != "" { @@ -102,32 +104,39 @@ func RunShell(options ShellOptions) { } for _, c := range util.StringSplit(cmd, ";") { - if processEachCmd(c, commandEnv) { - return + if exit, _ := processEachCmd(c, commandEnv); exit { + return nil } } } } else { + var lastErr error scanner := bufio.NewScanner(os.Stdin) for scanner.Scan() { cmd := scanner.Text() for _, c := range util.StringSplit(cmd, ";") { - if processEachCmd(c, commandEnv) { - return + exit, err := processEachCmd(c, commandEnv) + if err != nil { + lastErr = err + } + if exit { + return lastErr } } } if err := scanner.Err(); err != nil { fmt.Fprintf(os.Stderr, "error reading stdin: %v\n", err) + lastErr = err } + return lastErr } } -func processEachCmd(cmd string, commandEnv *CommandEnv) bool { +func processEachCmd(cmd string, commandEnv *CommandEnv) (exit bool, cmdErr error) { cmds := splitCommandLine(cmd) if len(cmds) == 0 { - return false + return false, nil } else { args := cmds[1:] @@ -136,7 +145,7 @@ func processEachCmd(cmd string, commandEnv *CommandEnv) bool { if cmd == "help" || cmd == "?" { printHelp(cmds) } else if cmd == "exit" || cmd == "quit" { - return true + return true, nil } else { foundCommand := false for _, c := range Commands { @@ -149,17 +158,19 @@ func processEachCmd(cmd string, commandEnv *CommandEnv) bool { commandEnv.SetNoLock(false) if err := c.Do(args, commandEnv, os.Stdout); err != nil { fmt.Fprintf(os.Stderr, "error: %v\n", err) + cmdErr = err } foundCommand = true } } if !foundCommand { fmt.Fprintf(os.Stderr, "unknown command: %v\n", cmd) + cmdErr = fmt.Errorf("unknown command: %v", cmd) } } } - return false + return false, cmdErr } func splitCommandLine(line string) []string { diff --git a/weed/shell/shell_liner_test.go b/weed/shell/shell_liner_test.go new file mode 100644 index 000000000..6d9bda9e7 --- /dev/null +++ b/weed/shell/shell_liner_test.go @@ -0,0 +1,66 @@ +package shell + +import ( + "errors" + "io" + "strings" + "testing" +) + +type fakeCommand struct { + name string + err error +} + +func (c *fakeCommand) Name() string { return c.name } +func (c *fakeCommand) Help() string { return "test helper" } +func (c *fakeCommand) HasTag(CommandTag) bool { return false } +func (c *fakeCommand) Do([]string, *CommandEnv, io.Writer) error { return c.err } + +func TestProcessEachCmdReturnsErrors(t *testing.T) { + + exit, err := processEachCmd("definitely.not.a.command", nil) + if exit { + t.Errorf("unknown command must not request exit") + } + if err == nil { + t.Errorf("unknown command must return an error") + } + if err != nil && !strings.Contains(err.Error(), "unknown command") { + t.Errorf("unexpected error for unknown command: %v", err) + } + + exit, err = processEachCmd("exit", nil) + if !exit { + t.Errorf("exit must request exit") + } + if err != nil { + t.Errorf("exit must not return an error, got: %v", err) + } + + exit, err = processEachCmd(" ", nil) + if exit || err != nil { + t.Errorf("blank input must be a no-op, got exit=%v err=%v", exit, err) + } +} + +func TestProcessEachCmdPreservesRegisteredCommandError(t *testing.T) { + doErr := errors.New("daily_run: shard=3: recovery walk failed") + failing := &fakeCommand{name: "test.failing.command", err: doErr} + succeeding := &fakeCommand{name: "test.succeeding.command"} + Commands = append(Commands, failing, succeeding) + t.Cleanup(func() { Commands = Commands[:len(Commands)-2] }) + + exit, err := processEachCmd("test.failing.command -some -args", nil) + if exit { + t.Errorf("a failing command must not request exit") + } + if !errors.Is(err, doErr) { + t.Errorf("the command's own error must be preserved, got: %v", err) + } + + exit, err = processEachCmd("test.succeeding.command", nil) + if exit || err != nil { + t.Errorf("a succeeding command must return no error, got exit=%v err=%v", exit, err) + } +} diff --git a/weed/weed.go b/weed/weed.go index f83777bf5..17f6a44e1 100644 --- a/weed/weed.go +++ b/weed/weed.go @@ -102,6 +102,8 @@ func main() { // Command execution failed - general error setExitStatus(1) } + // A command can also record a failure without returning false. + setExitStatus(command.CommandExitStatus()) exit() return } @@ -200,5 +202,7 @@ func exit() { for _, f := range atexitFuncs { f() } + // os.Exit below skips main's deferred flush. + sentry.Flush(2 * time.Second) os.Exit(exitStatus) }