temporalio / cli

Command-line interface for running Temporal Server and interacting with Workflows, Activities, Namespaces, and other parts of Temporal
https://docs.temporal.io/cli
MIT License
243 stars 32 forks source link

[Bug] CLI panics if started without a `stdout` file descriptor #544

Closed mjameswh closed 2 months ago

mjameswh commented 2 months ago

Describe the bug

Since 0.12.0, launching the CLI without a stdout file descriptor causes the Printer to panic as soon as the process attempts to emit some output.

This notably happens when starting a dev server using TS SDK's TestWorkflowEnvironment.createLocal(), due to Node's setting FD_CLOEXEC on all of its standard file descriptors. This has been recently fixed on the TS SDK (to be included in upcoming v1.9.4), but previous releases will be unusable with CLI 0.12+ until we fix this on CLI side.

Minimal Reproduction

This can be easily tested using Bash or a similar shell, with any command:

$ temporal-v0.12.0 workflow start --workflow-id abcd --task-queue test --type qwerty >&-

panic: write /dev/stdout: device not configured

goroutine 1 [running]:
github.com/temporalio/cli/temporalcli/internal/printer.(*Printer).write(...)
        /home/runner/work/cli/cli/temporalcli/internal/printer/printer.go:199
github.com/temporalio/cli/temporalcli/internal/printer.(*Printer).writeStr(...)
        /home/runner/work/cli/cli/temporalcli/internal/printer/printer.go:204
github.com/temporalio/cli/temporalcli/internal/printer.(*Printer).Print(0x14000890320, {0x1400036e0c0?, 0x2, 0x14000b51838?})
        /home/runner/work/cli/cli/temporalcli/internal/printer/printer.go:42 +0xcc
github.com/temporalio/cli/temporalcli/internal/printer.(*Printer).Println(...)
        /home/runner/work/cli/cli/temporalcli/internal/printer/printer.go:49
github.com/temporalio/cli/temporalcli.(*TemporalWorkflowCommand).startWorkflow(0x140009ca000, 0x140008ae2a0, {0x12a516a08, 0x14000956000}, 0x140009cb6e0, 0x0?, 0x0?, 0x1)
        /home/runner/work/cli/cli/temporalcli/commands.workflow_exec.go:226 +0x268
github.com/temporalio/cli/temporalcli.(*TemporalWorkflowStartCommand).run(0x140009cb400, 0x0?, {0x0?, 0x0?, 0x0?})
        /home/runner/work/cli/cli/temporalcli/commands.workflow_exec.go:28 +0xc0
github.com/temporalio/cli/temporalcli.NewTemporalWorkflowStartCommand.func1(0x140008e9d00?, {0x1400063bce0?, 0x4?, 0x1021b4473?})
        /home/runner/work/cli/cli/temporalcli/commands.gen.go:2118 +0x3c
github.com/spf13/cobra.(*Command).execute(0x140009cb408, {0x1400063bc80, 0x6, 0x6})
        /home/runner/go/pkg/mod/github.com/spf13/cobra@v1.8.0/command.go:987 +0x828
github.com/spf13/cobra.(*Command).ExecuteC(0x1400004fc00)
        /home/runner/go/pkg/mod/github.com/spf13/cobra@v1.8.0/command.go:1115 +0x344
github.com/spf13/cobra.(*Command).Execute(...)
        /home/runner/go/pkg/mod/github.com/spf13/cobra@v1.8.0/command.go:1039
github.com/spf13/cobra.(*Command).ExecuteContext(...)
        /home/runner/go/pkg/mod/github.com/spf13/cobra@v1.8.0/command.go:1032
github.com/temporalio/cli/temporalcli.Execute({0x10336df30?, 0x14000890230?}, {{0x0, 0x0, 0x0}, {0x0, 0x0}, {0x0, 0x0}, 0x0, ...})
        /home/runner/work/cli/cli/temporalcli/commands.go:318 +0xec
main.main()
        /home/runner/work/cli/cli/cmd/temporal/main.go:15 +0x64

Note that the >&- shell redirection operator at the end of the command instructs the shell to close the STDOUT file descriptor before executing the child process.

There is no error if the same command is run without the >&- shell redirection operator, or when using previous versions of the CLI with the redirection operator.

cretz commented 2 months ago

Hrmm, not much Go binary documentation out there on this issue. Can you confirm that this panics for every Go binary that uses os.Stdout.Write([]byte("foo")) in this situation? I am not aware of any Go binaries that account for missing descriptors as opposed to just ones forwarded to /dev/null (can check kubectl, cockroachdb, etc, etc). I think the issue is the absence of the descriptor not how the CLI reacts to it.

mjameswh commented 2 months ago

Stdout.Write returns an error. Printer does panic on that error.

package main

import (
    "os"
)

func main() {
    _, err := os.Stdout.Write([]byte("foo"))
    if err != nil {
        panic(err)
    }
}

when I run that, I get:

$ go run test.go >&-

panic: write /dev/stdout: bad file descriptor

goroutine 1 [running]:
main.main()
        /Users/jwatkins/Development/Temporal/CLI/cli/test.go:10 +0x64
exit status 2
cretz commented 2 months ago

:+1: I wonder if we can find it out before attempting our first write