From 4983f9a008266bbd40a77db3e1dda822359aa879 Mon Sep 17 00:00:00 2001 From: Volodymyr Kolesnykov Date: Fri, 9 Oct 2026 11:26:19 +0300 Subject: [PATCH 01/13] fix(wp): restore interactive WebSocket shell --- cmd/vip-next/commands/wp.go | 165 +++- cmd/vip-next/commands/wp_test.go | 69 ++ internal/parity/surface_differential_test.go | 5 +- internal/parity/wp_shell_differential_test.go | 751 ++++++++++++++++++ internal/wpshell/input.go | 88 ++ internal/wpshell/input_test.go | 64 ++ internal/wpshell/repl.go | 32 +- internal/wpstream/run.go | 2 + src/bin/vip-wp.js | 48 +- .../wp-websocket-redirect/resolve-app.json | 1 - .../wp-websocket-shell/resolve-app.json | 1 + testdata/parity/wp-websocket-redirect.yaml | 14 - testdata/parity/wp-websocket-shell.yaml | 13 + 13 files changed, 1185 insertions(+), 68 deletions(-) create mode 100644 internal/parity/wp_shell_differential_test.go create mode 100644 internal/wpshell/input.go create mode 100644 internal/wpshell/input_test.go delete mode 100644 testdata/parity/recordings/wp-websocket-redirect/resolve-app.json create mode 100644 testdata/parity/recordings/wp-websocket-shell/resolve-app.json delete mode 100644 testdata/parity/wp-websocket-redirect.yaml create mode 100644 testdata/parity/wp-websocket-shell.yaml diff --git a/cmd/vip-next/commands/wp.go b/cmd/vip-next/commands/wp.go index 8684e87d4..117b0afe4 100644 --- a/cmd/vip-next/commands/wp.go +++ b/cmd/vip-next/commands/wp.go @@ -1,6 +1,7 @@ package commands import ( + "bufio" "context" "errors" "fmt" @@ -8,11 +9,13 @@ import ( "os" "os/signal" "strings" + "sync" "syscall" "github.com/Khan/genqlient/graphql" "github.com/fatih/color" "github.com/spf13/cobra" + "github.com/vektah/gqlparser/v2/gqlerror" "golang.org/x/term" "github.com/Automattic/vip/internal/appctx" @@ -33,6 +36,18 @@ var wpYes bool // SetWPYes records the extracted --yes flag. Called by main.go. func SetWPYes(v bool) { wpYes = v } +func printWPError(out io.Writer, err error) error { + var list gqlerror.List + if errors.As(err, &list) { + for _, item := range list { + fmt.Fprintln(out, color.RedString("Error: "+item.Message)) + } + } else { + fmt.Fprintln(out, color.RedString("Error: "+err.Error())) + } + return exit.Handled(err) +} + // nodejsTypeIDs — NODEJS_SITE_TYPE_IDS (src/lib/constants/vipgo.ts:12). var nodejsTypeIDs = map[int64]bool{3: true, 5: true, 7: true, 8: true} @@ -126,9 +141,111 @@ func runWP(cmd *cobra.Command, args []string) error { } } + if isSubShell && info.WpcliStrategy == "websocket" { + return runWPShell(cmd, ae, info) + } return dispatchWP(cmd, ae, info, args, isSubShell) } +func runWPShell(cmd *cobra.Command, ae *appctx.AppEnv, info *wpEnvInfo) error { + out := cmd.OutOrStdout() + trackEvent("wpcli_command_execute", map[string]any{"method": "subshell"}) + fmt.Fprintf(out, "Welcome to the WP-CLI shell for the %s environment of %s (%s)!\n", + formatEnvironment(ae.Env.Type), ae.App.Name, info.PrimaryDomainName) + identifier := ae.Env.Type + if ae.Env.Name != "" && ae.Env.Name != ae.Env.Type && ae.Env.AppId != ae.Env.ID { + identifier += "." + ae.Env.Name + } + prompt := fmt.Sprintf("%s.%s:~$ ", ae.App.Name, identifier) + originalCtx := cmd.Context() + ctx, cancel := signal.NotifyContext(originalCtx, syscall.SIGINT, syscall.SIGTERM) + defer cancel() + cmd.SetContext(ctx) + defer cmd.SetContext(originalCtx) + var interruptMu sync.Mutex + commandActive := false + interrupts := 0 + keyboardExit := false + commandInterrupted := false + input := wpshell.NewInput(ctx, cmd.InOrStdin(), func() bool { + interruptMu.Lock() + defer interruptMu.Unlock() + interrupts++ + if !commandActive || interrupts > 1 { + keyboardExit = true + cancel() + return true + } + // Forward the first Ctrl-C to remote stdin; closing only the local + // connection does not establish that the remote command was stopped. + commandInterrupted = true + return false + }) + reader := input.Reader(ctx) + defer reader.Close() + loop := &wpshell.REPL{ + Prompt: prompt, + Run: func(command string) error { + commandCtx, commandCancel := context.WithCancel(ctx) + interruptMu.Lock() + commandActive = true + commandInterrupted = false + interruptMu.Unlock() + cmd.SetContext(commandCtx) + commandInput := input.Reader(commandCtx) + err := runWPWebsocketCommand(cmd, ae, command, true, commandInput) + commandCancel() + _ = commandInput.Close() + cmd.SetContext(ctx) + interruptMu.Lock() + commandActive = false + interrupted := commandInterrupted + interruptMu.Unlock() + if interrupted && ctx.Err() == nil { + fmt.Fprintln(out, "Command cancelled by user") + return nil + } + if ctx.Err() != nil { + return err + } + if err != nil { + var handled interface{ AlreadyPrinted() bool } + if !errors.As(err, &handled) || !handled.AlreadyPrinted() { + fmt.Fprintln(out, color.RedString("Error: "+err.Error())) + } + } + return err + }, + } + if stdin, ok := cmd.InOrStdin().(*os.File); ok && term.IsTerminal(int(stdin.Fd())) { + state, err := term.MakeRaw(int(stdin.Fd())) + if err != nil { + return err + } + defer func() { _ = term.Restore(int(stdin.Fd()), state) }() + terminal := term.NewTerminal(struct { + io.Reader + io.Writer + }{reader, out}, prompt) + if width, height, err := term.GetSize(int(stdin.Fd())); err == nil { + _ = terminal.SetSize(width, height) + } + originalOut := out + defer cmd.SetOut(originalOut) + out = terminal + cmd.SetOut(out) + loop.ReadLine = terminal.ReadLine + } + err := loop.Serve(bufio.NewReader(reader), out) + interruptMu.Lock() + defer interruptMu.Unlock() + if keyboardExit { + fmt.Fprintln(out, "Command cancelled by user") + return nil + } + return err +} + // dispatchWP routes to the appropriate WP-CLI execution strategy based on // info.WpcliStrategy. Ports vip-wp.js + wp-ssh.ts dispatch logic. func dispatchWP(cmd *cobra.Command, ae *appctx.AppEnv, info *wpEnvInfo, args []string, isSubShell bool) error { @@ -169,23 +286,20 @@ func dispatchWP(cmd *cobra.Command, ae *appctx.AppEnv, info *wpEnvInfo, args []s // Surface GraphQL error in the same format other commands use for // allowed-error contexts: print "Error: " in red and return an // error so the caller sees a non-zero exit (sync.go:80 pattern). - fmt.Fprintln(out, color.RedString("Error: "+err.Error())) - return err + return printWPError(out, err) } payload := resp.GetTriggerWPCLICommandOnAppEnvironment() if payload == nil { err := errors.New("WP-CLI SSH Authentication failed") - fmt.Fprintln(out, color.RedString("Error: "+err.Error())) - return err + return printWPError(out, err) } sshAuth := payload.GetSshAuthentication() if sshAuth == nil { // wp-ssh.ts:114 err := errors.New("WP-CLI SSH Authentication failed") - fmt.Fprintln(out, color.RedString("Error: "+err.Error())) - return err + return printWPError(out, err) } // Extract GUID and InputToken from the payload. @@ -292,25 +406,21 @@ func dispatchWP(cmd *cobra.Command, ae *appctx.AppEnv, info *wpEnvInfo, args []s // dispatchWPWebsocket handles the "websocket" wpcliStrategy by connecting to // the environment over socket.io (internal/wpstream, WP2). -// -// NOTE: Node's socket.io path supports an interactive REPL for subshell mode, -// but WP2 ships single-command socket.io first. The isSubShell parameter is -// accepted for signature symmetry; the websocket branch runs the (possibly -// empty) joined command string regardless — same as the SSH branch's documented -// quirk. The interactive REPL over socket.io (internal/wpshell.REPL) is a -// follow-up task. func dispatchWPWebsocket(cmd *cobra.Command, ae *appctx.AppEnv, _ *wpEnvInfo, args []string, isSubShell bool) error { + return runWPWebsocketCommand(cmd, ae, strings.Join(wpshell.RequoteArgs(args), " "), isSubShell, cmd.InOrStdin()) +} + +func runWPWebsocketCommand(cmd *cobra.Command, ae *appctx.AppEnv, cmdStr string, isSubShell bool, stdin io.Reader) error { cfg := GetConfig() out := cmd.OutOrStdout() - // Build the WP-CLI command string (single-command mode; REPL is a follow-up). - cmdStr := strings.Join(wpshell.RequoteArgs(args), " ") - method := "shell" if isSubShell { method = "subshell" } - trackEvent("wpcli_command_execute", map[string]any{"method": method}) + if !isSubShell { + trackEvent("wpcli_command_execute", map[string]any{"method": method}) + } // Call TriggerWPCLICommand under WithAllowGQLErrors so GraphQL errors come // back to us rather than triggering an os.Exit in the middleware. @@ -322,15 +432,16 @@ func dispatchWPWebsocket(cmd *cobra.Command, ae *appctx.AppEnv, _ *wpEnvInfo, ar triggerCtx := gql.WithAllowGQLErrors(cmd.Context()) resp, err := gql.TriggerWPCLICommand(triggerCtx, cfg.GQLClient, triggerInput) if err != nil { - fmt.Fprintln(out, color.RedString("Error: "+err.Error())) - return err + if cmd.Context().Err() != nil { + return cmd.Context().Err() + } + return printWPError(out, err) } payload := resp.GetTriggerWPCLICommandOnAppEnvironment() if payload == nil { err := errors.New("WP-CLI command trigger failed: empty payload") - fmt.Fprintln(out, color.RedString("Error: "+err.Error())) - return err + return printWPError(out, err) } // Extract GUID and InputToken from the payload. @@ -363,14 +474,20 @@ func dispatchWPWebsocket(cmd *cobra.Command, ae *appctx.AppEnv, _ *wpEnvInfo, ar Columns: cols, Rows: rows, IsTTY: tty, - Stdin: os.Stdin, - Stdout: os.Stdout, - Stderr: os.Stderr, + Stdin: stdin, + Stdout: out, + Stderr: cmd.ErrOrStderr(), }) if runErr != nil { return runErr } trackEvent("wpcli_command_end", map[string]any{"method": method}) + if isSubShell { + if res.ExitCode != 0 { + return fmt.Errorf("WP-CLI command failed with exit code %d", res.ExitCode) + } + return nil + } exit.WithCode(res.ExitCode, nil) return nil // unreachable — exit.WithCode calls os.Exit } diff --git a/cmd/vip-next/commands/wp_test.go b/cmd/vip-next/commands/wp_test.go index 4b77aaf9b..dfe9c9bc2 100644 --- a/cmd/vip-next/commands/wp_test.go +++ b/cmd/vip-next/commands/wp_test.go @@ -3,6 +3,7 @@ package commands import ( "bytes" "context" + "encoding/json/v2" "io" "net/http" "net/http/httptest" @@ -17,6 +18,74 @@ import ( "github.com/Automattic/vip/internal/appctx" ) +func TestWPShellWaitsForCommand(t *testing.T) { + stub := &wpStub{body: wpEnvInfoBodyWithStrategy(2, "websocket", "production")} + setupWPTest(t, stub) + cmd := WPCmd() + cmd.SetContext(wpCtx(42, 7, 2, "production")) + cmd.SetIn(strings.NewReader("\nls\nexit\n")) + var out bytes.Buffer + cmd.SetOut(&out) + if err := runWP(cmd, nil); err != nil { + t.Fatal(err) + } + if stub.triggerHits.Load() != 0 { + t.Fatal("opening, invalid input, and exiting the shell must not trigger a command") + } + if !strings.Contains(out.String(), "Welcome to the WP-CLI shell for the PRODUCTION environment of parityapp (example.com)!") { + t.Fatalf("missing shell greeting: %q", out.String()) + } + if strings.Count(out.String(), "parityapp.production:~$ ") != 3 { + t.Fatalf("missing prompts: %q", out.String()) + } +} + +func TestWPShellContinuesAfterTriggerError(t *testing.T) { + var commands []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var request struct { + OperationName string `json:"operationName"` + Variables struct { + Input struct { + Command string `json:"command"` + ID int64 `json:"id"` + EnvironmentID int64 `json:"environmentId"` + } `json:"input"` + } `json:"variables"` + } + if err := json.UnmarshalRead(r.Body, &request); err != nil { + t.Error(err) + } + w.Header().Set("Content-Type", "application/json") + if request.OperationName == "WPEnvInfo" { + _, _ = io.WriteString(w, wpEnvInfoBodyWithStrategy(2, "websocket", "production")) + return + } + if request.Variables.Input.ID != 42 || request.Variables.Input.EnvironmentID != 7 { + t.Errorf("wrong target: %+v", request.Variables.Input) + } + commands = append(commands, request.Variables.Input.Command) + _, _ = io.WriteString(w, `{"errors":[{"message":"command failed","locations":[{"line":3,"column":1}]}]}`) + })) + defer srv.Close() + SetConfig(Config{GQLClient: graphql.NewClient(srv.URL, srv.Client()), APIHost: srv.URL}) + t.Cleanup(func() { SetConfig(Config{}) }) + cmd := WPCmd() + cmd.SetContext(wpCtx(42, 7, 2, "production")) + cmd.SetIn(strings.NewReader(" wp option get home\r\nwp option set key \"line1\nline2\"\nexit\n")) + var out bytes.Buffer + cmd.SetOut(&out) + if err := runWP(cmd, nil); err != nil { + t.Fatal(err) + } + if len(commands) != 2 || commands[0] != "option get home" || commands[1] != "option set key \"line1\nline2\"" { + t.Fatalf("commands = %q", commands) + } + if strings.Count(out.String(), "Error: command failed\n") != 2 || strings.Contains(out.String(), "input:3") { + t.Fatalf("errors must appear once without GraphQL locations: %q", out.String()) + } +} + // wpEnvInfoBody builds a minimal WPEnvInfo JSON response. func wpEnvInfoBody(typeID int64) string { return `{"data":{"app":{"id":42,"name":"parityapp","typeId":` + diff --git a/internal/parity/surface_differential_test.go b/internal/parity/surface_differential_test.go index 57f497983..72042d259 100644 --- a/internal/parity/surface_differential_test.go +++ b/internal/parity/surface_differential_test.go @@ -321,9 +321,8 @@ var surfaceDifferentialExclusions = map[string]string{ "stands up an in-process echo server that speaks vip-next's exec preamble; Node would " + "need the same server to speak ITS protocol, which is a separate fake to build. Out of " + "reach for this pass.", - "wp-websocket-redirect": "asserts vip-next's deliberate 'requires the Node CLI' redirect " + - "(register §3). Node has no such redirect — it just runs the command — so the two " + - "implementations are not answering the same question and a diff is meaningless.", + "wp-websocket-shell": "TestWPWebsocketShellDifferential drives both binaries through the " + + "same stdin prompts, socket events, and per-command API request assertions.", "wp-nodejs-rejected": "needs the same wp transport fake as wp-ssh-happy before Node can " + "reach the environment-type gate.", "wp-production-confirm-decline": "combines the wp transport problem with the " + diff --git a/internal/parity/wp_shell_differential_test.go b/internal/parity/wp_shell_differential_test.go new file mode 100644 index 000000000..2c9253176 --- /dev/null +++ b/internal/parity/wp_shell_differential_test.go @@ -0,0 +1,751 @@ +//go:build parity + +package parity + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "io" + "net/http" + "os" + "os/exec" + "reflect" + "strconv" + "strings" + "sync" + "syscall" + "testing" + "time" + + "github.com/coder/websocket" + "github.com/creack/pty" + "golang.org/x/term" +) + +type shellOutput struct { + mu sync.Mutex + text strings.Builder + changed chan struct{} +} + +func newShellOutput() *shellOutput { + return &shellOutput{changed: make(chan struct{}, 1)} +} + +func (o *shellOutput) Write(p []byte) (int, error) { + o.mu.Lock() + n, err := o.text.Write(p) + o.mu.Unlock() + select { + case o.changed <- struct{}{}: + default: + } + return n, err +} + +func (o *shellOutput) String() string { + o.mu.Lock() + defer o.mu.Unlock() + return o.text.String() +} + +func waitForShellOutput(t *testing.T, output *shellOutput, done <-chan error, after int, needle string) int { + t.Helper() + deadline := time.NewTimer(12 * time.Second) + defer deadline.Stop() + for { + got := output.String() + if after <= len(got) { + if i := strings.Index(got[after:], needle); i >= 0 { + return after + i + len(needle) + } + } + select { + case <-output.changed: + case err := <-done: + t.Fatalf("shell exited before %q: %v\nstdout: %q", needle, err, output.String()) + case <-deadline.C: + t.Fatalf("timeout waiting for %q\nstdout: %q", needle, output.String()) + } + } +} + +type shellMock struct { + mu sync.Mutex + commands []string + appBody []byte + poll chan string + stdoutStreamID string + streamExitCode int + stdinStreamID string + needsInput bool + inputReady chan struct{} + inputReadyOnce sync.Once + remoteBytes chan byte +} + +func (m *shellMock) ServeHTTP(w http.ResponseWriter, r *http.Request) { + if strings.HasPrefix(r.URL.Path, "/socket.io/") { + m.serveSocket(w, r) + return + } + body, _ := io.ReadAll(r.Body) + s := string(body) + w.Header().Set("Content-Type", "application/json") + switch { + case strings.Contains(s, `"operationName":"ResolveAppByName"`), + strings.Contains(s, `"operationName":"ResolveAppByID"`), + strings.Contains(s, `"operationName":"App"`): + _, _ = w.Write(m.appBody) + case strings.Contains(s, `"operationName":"WPEnvInfo"`): + _, _ = w.Write(wpEnvInfoJSON(2, "websocket", "develop")) + case strings.Contains(s, `"operationName":"TriggerWPCLICommand"`), + strings.Contains(s, `"operationName":"TriggerWPCLICommandMutation"`): + var request struct { + Variables struct { + Input struct { + Command string `json:"command"` + } `json:"input"` + } `json:"variables"` + } + if err := json.Unmarshal(body, &request); err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + m.mu.Lock() + m.commands = append(m.commands, request.Variables.Input.Command) + m.stdoutStreamID = "" + m.streamExitCode = 0 + m.stdinStreamID = "" + m.needsInput = false + m.mu.Unlock() + if request.Variables.Input.Command == "option get home" { + _, _ = w.Write([]byte(`{"errors":[{"message":"Test WP-CLI error"}],"data":null}`)) + } else { + guid := "ws-guid-001" + if request.Variables.Input.Command == "option get missing" || + request.Variables.Input.Command == `"option" "get" "missing"` { + guid = "ws-fail-guid" + } else if request.Variables.Input.Command == "eval read" { + guid = "ws-stdin-guid" + } + _, _ = fmt.Fprintf(w, `{"data":{"triggerWPCLICommandOnAppEnvironment":{"inputToken":"tok-ws","command":{"guid":%q},"sshAuthentication":null}}}`, guid) + } + default: + http.Error(w, "unexpected GraphQL operation", http.StatusBadRequest) + } +} + +func (m *shellMock) commandsSeen() []string { + m.mu.Lock() + defer m.mu.Unlock() + return append([]string(nil), m.commands...) +} + +func (m *shellMock) serveSocket(w http.ResponseWriter, r *http.Request) { + if r.URL.Query().Get("transport") == "polling" { + w.Header().Set("Content-Type", "text/plain; charset=UTF-8") + if r.URL.Query().Get("sid") == "" { + _, _ = io.WriteString(w, `0{"sid":"shell","upgrades":["websocket"],"pingInterval":25000,"pingTimeout":20000,"maxPayload":1000000}`) + return + } + if r.Method == http.MethodPost { + body, _ := io.ReadAll(r.Body) + for _, packet := range strings.Split(string(body), "\x1e") { + m.recordStreamID(packet) + } + if strings.Contains(string(body), "40/wp-cli,") { + select { + case m.poll <- `40/wp-cli,{"sid":"shell-nsp"}`: + default: + } + } + _, _ = io.WriteString(w, "ok") + return + } + select { + case packet := <-m.poll: + _, _ = io.WriteString(w, packet) + case <-time.After(3 * time.Second): + _, _ = io.WriteString(w, "6") + case <-r.Context().Done(): + } + return + } + + ws, err := websocket.Accept(w, r, nil) + if err != nil { + return + } + defer ws.Close(websocket.StatusNormalClosure, "") + ctx := r.Context() + _, probe, err := ws.Read(ctx) + if err != nil || string(probe) != "2probe" { + return + } + if err := ws.Write(ctx, websocket.MessageText, []byte("3probe")); err != nil { + return + } + _, upgrade, err := ws.Read(ctx) + if err != nil || string(upgrade) != "5" { + return + } + m.mu.Lock() + streamID := m.stdoutStreamID + exitCode := m.streamExitCode + stdinID := m.stdinStreamID + needsInput := m.needsInput + m.mu.Unlock() + if streamID != "" { + if err := m.startShellStream(ctx, ws, streamID, stdinID, needsInput, exitCode); err != nil { + return + } + } + var pendingAck int + for { + messageType, raw, err := ws.Read(ctx) + if err != nil { + return + } + if messageType == websocket.MessageBinary && pendingAck > 0 { + for _, b := range raw { + if m.remoteBytes != nil { + m.remoteBytes <- b + } + } + ack := fmt.Sprintf(`43/wp-cli,%d[]`, pendingAck) + pendingAck = 0 + if err := ws.Write(ctx, websocket.MessageText, []byte(ack)); err != nil { + return + } + if strings.ContainsRune(string(raw), 3) { + m.mu.Lock() + streamID := m.stdoutStreamID + m.mu.Unlock() + _ = finishShellStream(ctx, ws, streamID, 0) + } + continue + } + packet := string(raw) + switch { + case packet == "40/wp-cli,": + _ = ws.Write(ctx, websocket.MessageText, []byte(`40/wp-cli,{"sid":"shell-nsp"}`)) + case strings.HasPrefix(packet, `42/wp-cli,["$stream","cmd",`): + streamID := m.recordStreamID(packet) + m.mu.Lock() + exitCode := m.streamExitCode + stdinID := m.stdinStreamID + needsInput := m.needsInput + m.mu.Unlock() + if streamID == "" || m.startShellStream(ctx, ws, streamID, stdinID, needsInput, exitCode) != nil { + return + } + case strings.HasPrefix(packet, "451-/wp-cli,"): + rest := strings.TrimPrefix(packet, "451-/wp-cli,") + bracket := strings.IndexByte(rest, '[') + if bracket < 0 { + return + } + pendingAck, err = strconv.Atoi(rest[:bracket]) + if err != nil { + return + } + } + } +} + +func (m *shellMock) recordStreamID(packet string) string { + if !strings.HasPrefix(packet, `42/wp-cli,["$stream","cmd",`) { + return "" + } + var args []json.RawMessage + if err := json.Unmarshal([]byte(strings.TrimPrefix(packet, "42/wp-cli,")), &args); err != nil || len(args) < 5 { + return "" + } + var stream struct { + ID string `json:"$stream"` + } + if err := json.Unmarshal(args[4], &stream); err != nil || stream.ID == "" { + return "" + } + var stdinStream struct { + ID string `json:"$stream"` + } + if err := json.Unmarshal(args[3], &stdinStream); err != nil { + return "" + } + var data struct { + GUID string `json:"guid"` + } + if err := json.Unmarshal(args[2], &data); err != nil { + return "" + } + m.mu.Lock() + m.stdoutStreamID = stream.ID + m.stdinStreamID = stdinStream.ID + m.streamExitCode = 0 + m.needsInput = data.GUID == "ws-stdin-guid" + if data.GUID == "ws-fail-guid" { + m.streamExitCode = 3 + } + m.mu.Unlock() + return stream.ID +} + +func (m *shellMock) startShellStream(ctx context.Context, ws *websocket.Conn, stdoutID, stdinID string, needsInput bool, exitCode int) error { + if !needsInput { + return finishShellStream(ctx, ws, stdoutID, exitCode) + } + if stdinID == "" { + return errors.New("stdin stream ID missing") + } + read := fmt.Sprintf(`42/wp-cli,["$stream-read",%q,65536]`, stdinID) + if err := ws.Write(ctx, websocket.MessageText, []byte(read)); err != nil { + return err + } + m.inputReadyOnce.Do(func() { + if m.inputReady != nil { + close(m.inputReady) + } + }) + return nil +} + +func finishShellStream(ctx context.Context, ws *websocket.Conn, id string, exitCode int) error { + // Give the command handler time to bind its stream-end listener after emit. + time.Sleep(100 * time.Millisecond) + end := fmt.Sprintf(`42/wp-cli,["$stream-end",%q]`, id) + if err := ws.Write(ctx, websocket.MessageText, []byte(end)); err != nil { + return err + } + if exitCode != 0 { + // The real service can send its exit status after stdout EOF. + time.Sleep(100 * time.Millisecond) + exit := fmt.Sprintf(`42/wp-cli,["exit",{"exitCode":%d}]`, exitCode) + return ws.Write(ctx, websocket.MessageText, []byte(exit)) + } + return nil +} + +func runShellSide(t *testing.T, rig *differentialRig, scenario *Scenario, bin string) *RunResult { + t.Helper() + appBody, err := os.ReadFile("../../testdata/parity/recordings/wp-websocket-shell/resolve-app.json") + if err != nil { + t.Fatal(err) + } + mock := &shellMock{appBody: appBody, poll: make(chan string, 2)} + rig.serve(t, mock) + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, bin, scenario.Argv...) + cmd.Env = FixtureEnv(scenario.Env) + stdin, err := cmd.StdinPipe() + if err != nil { + t.Fatal(err) + } + stdout, stderr := newShellOutput(), newShellOutput() + cmd.Stdout, cmd.Stderr = stdout, stderr + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + done := make(chan error, 1) + go func() { done <- cmd.Wait() }() + t.Cleanup(func() { + _ = stdin.Close() + cancel() + select { + case <-done: + default: + } + }) + + prompt := "parityapp.develop:~$ " + pos := waitForShellOutput(t, stdout, done, 0, prompt) + if got := mock.commandsSeen(); len(got) != 0 { + t.Errorf("trigger before shell input: %q", got) + } + if _, err := io.WriteString(stdin, "invalid\n"); err != nil { + t.Fatal(err) + } + pos = waitForShellOutput(t, stdout, done, pos, "invalid command, please pass a valid WP-CLI command.") + pos = waitForShellOutput(t, stdout, done, pos, prompt) + if got := mock.commandsSeen(); len(got) != 0 { + t.Errorf("trigger after invalid command: %q", got) + } + if _, err := io.WriteString(stdin, "wp option get home\n"); err != nil { + t.Fatal(err) + } + pos = waitForShellOutput(t, stdout, done, pos, "Test WP-CLI error") + pos = waitForShellOutput(t, stdout, done, pos, prompt) + if got := mock.commandsSeen(); len(got) != 1 || got[0] != "option get home" { + t.Errorf("trigger commands = %q, want [option get home]", got) + } + if _, err := io.WriteString(stdin, "wp site list\n"); err != nil { + t.Fatal(err) + } + pos = waitForShellOutput(t, stdout, done, pos, prompt) + if got := mock.commandsSeen(); len(got) != 2 || got[1] != "site list" { + t.Errorf("trigger commands after first successful socket command = %q", got) + } + if _, err := io.WriteString(stdin, "wp option get missing\n"); err != nil { + t.Fatal(err) + } + pos = waitForShellOutput(t, stdout, done, pos, "Error: WP-CLI command failed with exit code 3") + pos = waitForShellOutput(t, stdout, done, pos, prompt) + if got := mock.commandsSeen(); len(got) != 3 || got[2] != "option get missing" { + t.Errorf("trigger commands after failed socket command = %q", got) + } + if _, err := io.WriteString(stdin, "wp post list\n"); err != nil { + t.Fatal(err) + } + pos = waitForShellOutput(t, stdout, done, pos, prompt) + if got := mock.commandsSeen(); len(got) != 4 || got[3] != "post list" { + t.Errorf("trigger commands after second successful socket command = %q", got) + } + if _, err := io.WriteString(stdin, "exit\n"); err != nil { + t.Fatal(err) + } + _ = stdin.Close() + select { + case err := <-done: + res := &RunResult{Stdout: stdout.String(), Stderr: stderr.String()} + if err != nil { + var exitErr *exec.ExitError + if !errors.As(err, &exitErr) { + t.Fatalf("shell wait: %v", err) + } + res.ExitCode = exitErr.ExitCode() + } + return res + case <-ctx.Done(): + t.Fatalf("shell did not exit after exit command: %v\nstdout: %q\nstderr: %q", ctx.Err(), stdout.String(), stderr.String()) + } + return nil +} + +func TestWPWebsocketShellDifferential(t *testing.T) { + rig, skip := differentialAvailable(t) + if skip != "" { + t.Skip(LoudSkip("TestWPWebsocketShellDifferential", skip)) + } + scenario, err := LoadScenario("../../testdata/parity/wp-websocket-shell.yaml") + if err != nil { + t.Fatal(err) + } + scenario.Env = rig.scenarioEnv(scenario) + node := runShellSide(t, rig, scenario, rig.nodeBin) + goResult := runShellSide(t, rig, scenario, rig.goBin) + for side, result := range map[string]*RunResult{"node": node, "go": goResult} { + if result.ExitCode != scenario.Expect.ExitCode { + t.Errorf("%s shell exit=%d, want %d\nstdout: %q\nstderr: %q", side, result.ExitCode, scenario.Expect.ExitCode, result.Stdout, result.Stderr) + } + if !strings.Contains(result.Stdout, "Welcome to the WP-CLI shell for the develop environment of parityapp (d.example)!") { + t.Errorf("%s shell greeting missing: %q", side, result.Stdout) + } + if got := strings.Count(result.Stdout, "parityapp.develop:~$ "); got != 6 { + t.Errorf("%s shell showed %d prompts, want 6: %q", side, got, result.Stdout) + } + for _, message := range []string{ + "Error: invalid command, please pass a valid WP-CLI command.", + "Error: Test WP-CLI error", + "Error: WP-CLI command failed with exit code 3", + } { + if got := strings.Count(result.Stdout, message); got != 1 { + t.Errorf("%s shell printed %q %d times, want once: %q", side, message, got, result.Stdout) + } + } + } + if node.ExitCode != goResult.ExitCode { + t.Errorf("shell exit code differs: node=%d go=%d", node.ExitCode, goResult.ExitCode) + } + nodeErr, err := normalizeStderr(node.Stderr, nil) + if err != nil { + t.Fatal(err) + } + goErr, err := normalizeStderr(goResult.Stderr, nil) + if err != nil { + t.Fatal(err) + } + if nodeErr != goErr { + t.Errorf("shell stderr differs: node=%q go=%q", nodeErr, goErr) + } +} + +func runShellPTYSide(t *testing.T, rig *differentialRig, scenario *Scenario, bin, input string) *RunResult { + t.Helper() + appBody, err := os.ReadFile("../../testdata/parity/recordings/wp-websocket-shell/resolve-app.json") + if err != nil { + t.Fatal(err) + } + mock := &shellMock{appBody: appBody, poll: make(chan string, 2)} + rig.serve(t, mock) + + ctx, cancel := context.WithTimeout(context.Background(), 12*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, bin, scenario.Argv...) + cmd.Env = FixtureEnv(scenario.Env) + terminal, err := pty.StartWithSize(cmd, &pty.Winsize{Rows: 24, Cols: 100}) + if err != nil { + t.Fatal(err) + } + defer terminal.Close() + output := newShellOutput() + go func() { _, _ = io.Copy(output, terminal) }() + done := make(chan error, 1) + go func() { done <- cmd.Wait() }() + _ = waitForShellOutput(t, output, done, 0, "parityapp.develop:~$ ") + if got := mock.commandsSeen(); len(got) != 0 { + t.Errorf("PTY trigger before shell input: %q", got) + } + if _, err := io.WriteString(terminal, input); err != nil { + t.Fatal(err) + } + select { + case err := <-done: + result := &RunResult{Stdout: output.String()} + if err != nil { + var exitErr *exec.ExitError + if !errors.As(err, &exitErr) { + t.Fatalf("PTY shell wait: %v", err) + } + result.ExitCode = exitErr.ExitCode() + } + if got := mock.commandsSeen(); len(got) != 0 { + t.Errorf("PTY trigger on exit: %q", got) + } + return result + case <-ctx.Done(): + t.Fatalf("PTY shell did not exit: %v\noutput: %q", ctx.Err(), output.String()) + } + return nil +} + +func TestWPWebsocketShellPTYDifferential(t *testing.T) { + rig, skip := differentialAvailable(t) + if skip != "" { + t.Skip(LoudSkip("TestWPWebsocketShellPTYDifferential", skip)) + } + scenario, err := LoadScenario("../../testdata/parity/wp-websocket-shell.yaml") + if err != nil { + t.Fatal(err) + } + scenario.Env = rig.scenarioEnv(scenario) + node := runShellPTYSide(t, rig, scenario, rig.nodeBin, "exit\r") + goResult := runShellPTYSide(t, rig, scenario, rig.goBin, "exit\r") + if node.ExitCode != 0 || goResult.ExitCode != 0 { + t.Errorf("PTY shell exit: node=%d go=%d\nnode: %q\ngo: %q", node.ExitCode, goResult.ExitCode, node.Stdout, goResult.Stdout) + } + for side, result := range map[string]*RunResult{"node": node, "go": goResult} { + if !strings.Contains(result.Stdout, "Welcome to the WP-CLI shell for the develop environment of parityapp (d.example)!") { + t.Errorf("%s PTY shell greeting missing: %q", side, result.Stdout) + } + } +} + +func TestWPWebsocketShellPTYCtrlC(t *testing.T) { + rig, skip := differentialAvailable(t) + if skip != "" { + t.Skip(LoudSkip("TestWPWebsocketShellPTYCtrlC", skip)) + } + scenario, err := LoadScenario("../../testdata/parity/wp-websocket-shell.yaml") + if err != nil { + t.Fatal(err) + } + scenario.Env = rig.scenarioEnv(scenario) + for side, bin := range map[string]string{"node": rig.nodeBin, "go": rig.goBin} { + result := runShellPTYSide(t, rig, scenario, bin, "\x03") + if result.ExitCode != 0 { + t.Errorf("%s PTY Ctrl-C exit=%d, want 0; output: %q", side, result.ExitCode, result.Stdout) + } + } +} + +func TestWPWebsocketShellRestoresTerminalOnSIGTERM(t *testing.T) { + rig, skip := differentialAvailable(t) + if skip != "" { + t.Skip(LoudSkip("TestWPWebsocketShellRestoresTerminalOnSIGTERM", skip)) + } + scenario, err := LoadScenario("../../testdata/parity/wp-websocket-shell.yaml") + if err != nil { + t.Fatal(err) + } + scenario.Env = rig.scenarioEnv(scenario) + appBody, err := os.ReadFile("../../testdata/parity/recordings/wp-websocket-shell/resolve-app.json") + if err != nil { + t.Fatal(err) + } + mock := &shellMock{appBody: appBody, poll: make(chan string, 2)} + rig.serve(t, mock) + + master, slave, err := pty.Open() + if err != nil { + t.Fatal(err) + } + defer master.Close() + defer slave.Close() + if err := pty.Setsize(master, &pty.Winsize{Rows: 24, Cols: 100}); err != nil { + t.Fatal(err) + } + before, err := term.GetState(int(slave.Fd())) + if err != nil { + t.Fatal(err) + } + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, rig.goBin, scenario.Argv...) + cmd.Env = FixtureEnv(scenario.Env) + cmd.Stdin, cmd.Stdout, cmd.Stderr = slave, slave, slave + cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true, Setctty: true} + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + output := newShellOutput() + go func() { _, _ = io.Copy(output, master) }() + done := make(chan error, 1) + go func() { done <- cmd.Wait() }() + _ = waitForShellOutput(t, output, done, 0, "parityapp.develop:~$ ") + during, err := term.GetState(int(slave.Fd())) + if err != nil { + t.Fatal(err) + } + if reflect.DeepEqual(before, during) { + t.Error("Go shell did not put the terminal in raw mode") + } + if err := cmd.Process.Signal(syscall.SIGTERM); err != nil { + t.Fatal(err) + } + select { + case <-done: + case <-ctx.Done(): + t.Fatalf("Go shell did not stop after SIGTERM: %v\noutput: %q", ctx.Err(), output.String()) + } + after, err := term.GetState(int(slave.Fd())) + if err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(before, after) { + t.Errorf("Go shell left the terminal in raw mode after SIGTERM") + } + if got := mock.commandsSeen(); len(got) != 0 { + t.Errorf("SIGTERM caused a command trigger: %q", got) + } +} + +func TestWPWebsocketShellForwardsActiveCtrlC(t *testing.T) { + rig, skip := differentialAvailable(t) + if skip != "" { + t.Skip(LoudSkip("TestWPWebsocketShellForwardsActiveCtrlC", skip)) + } + scenario, err := LoadScenario("../../testdata/parity/wp-websocket-shell.yaml") + if err != nil { + t.Fatal(err) + } + scenario.Env = rig.scenarioEnv(scenario) + appBody, err := os.ReadFile("../../testdata/parity/recordings/wp-websocket-shell/resolve-app.json") + if err != nil { + t.Fatal(err) + } + mock := &shellMock{ + appBody: appBody, poll: make(chan string, 2), + inputReady: make(chan struct{}), remoteBytes: make(chan byte, 16), + } + rig.serve(t, mock) + + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, rig.goBin, scenario.Argv...) + cmd.Env = FixtureEnv(scenario.Env) + stdin, err := cmd.StdinPipe() + if err != nil { + t.Fatal(err) + } + defer stdin.Close() + stdout, stderr := newShellOutput(), newShellOutput() + cmd.Stdout, cmd.Stderr = stdout, stderr + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + done := make(chan error, 1) + go func() { done <- cmd.Wait() }() + prompt := "parityapp.develop:~$ " + _ = waitForShellOutput(t, stdout, done, 0, prompt) + if _, err := io.WriteString(stdin, "wp eval read\n"); err != nil { + t.Fatal(err) + } + select { + case <-mock.inputReady: + case <-ctx.Done(): + t.Fatalf("remote stdin was not opened: %v\nstdout: %q\nstderr: %q", ctx.Err(), stdout.String(), stderr.String()) + } + if _, err := stdin.Write([]byte{3}); err != nil { + t.Fatal(err) + } + select { + case b := <-mock.remoteBytes: + if b != 3 { + t.Errorf("remote stdin received %#x, want Ctrl-C (0x03)", b) + } + case <-ctx.Done(): + t.Fatalf("remote stdin did not receive Ctrl-C: %v\nstdout: %q", ctx.Err(), stdout.String()) + } + pos := waitForShellOutput(t, stdout, done, 0, "Command cancelled by user") + _ = waitForShellOutput(t, stdout, done, pos, prompt) + if got := mock.commandsSeen(); len(got) != 1 || got[0] != "eval read" { + t.Errorf("trigger commands = %q, want [eval read]", got) + } + if _, err := io.WriteString(stdin, "exit\n"); err != nil { + t.Fatal(err) + } + select { + case err := <-done: + if err != nil { + t.Errorf("shell exit after Ctrl-C: %v\nstdout: %q\nstderr: %q", err, stdout.String(), stderr.String()) + } + case <-ctx.Done(): + t.Fatalf("shell did not exit after Ctrl-C then exit: %v\nstdout: %q", ctx.Err(), stdout.String()) + } +} + +func TestWPWebsocketOneShotDelayedNonzeroExit(t *testing.T) { + rig, skip := differentialAvailable(t) + if skip != "" { + t.Skip(LoudSkip("TestWPWebsocketOneShotDelayedNonzeroExit", skip)) + } + scenario, err := LoadScenario("../../testdata/parity/wp-websocket-shell.yaml") + if err != nil { + t.Fatal(err) + } + scenario.Env = rig.scenarioEnv(scenario) + appBody, err := os.ReadFile("../../testdata/parity/recordings/wp-websocket-shell/resolve-app.json") + if err != nil { + t.Fatal(err) + } + argv := []string{"@parityapp.develop", "--", "wp", "option", "get", "missing"} + for side, bin := range map[string]string{"node": rig.nodeBin, "go": rig.goBin} { + t.Run(side, func(t *testing.T) { + mock := &shellMock{appBody: appBody, poll: make(chan string, 2)} + rig.serve(t, mock) + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, bin, argv...) + cmd.Env = FixtureEnv(scenario.Env) + var stdout, stderr bytes.Buffer + cmd.Stdout, cmd.Stderr = &stdout, &stderr + err := cmd.Run() + if ctx.Err() != nil { + t.Fatalf("one-shot command timed out: %v\nstdout: %q\nstderr: %q", ctx.Err(), stdout.String(), stderr.String()) + } + var exitErr *exec.ExitError + if !errors.As(err, &exitErr) || exitErr.ExitCode() != 3 { + t.Errorf("exit=%v, want code 3\nstdout: %q\nstderr: %q", err, stdout.String(), stderr.String()) + } + if got := mock.commandsSeen(); len(got) != 1 || got[0] != `"option" "get" "missing"` { + t.Errorf("trigger commands=%q, want a single quoted option get missing", got) + } + }) + } +} diff --git a/internal/wpshell/input.go b/internal/wpshell/input.go new file mode 100644 index 000000000..6f841b46e --- /dev/null +++ b/internal/wpshell/input.go @@ -0,0 +1,88 @@ +package wpshell + +import ( + "context" + "io" + "sync" +) + +// Input shares stdin between the prompt and remote commands. A command reader +// can be cancelled without leaving a goroutine consuming the next prompt's input. +type Input struct { + bytes chan byte + done chan struct{} + err error +} + +func NewInput(ctx context.Context, in io.Reader, interrupt ...func() bool) *Input { + i := &Input{bytes: make(chan byte, 1), done: make(chan struct{})} + go func() { + defer close(i.done) + var b [1]byte + for { + n, err := in.Read(b[:]) + if n > 0 { + if b[0] == 3 && len(interrupt) > 0 && interrupt[0]() { + continue + } + select { + case i.bytes <- b[0]: + case <-ctx.Done(): + i.err = ctx.Err() + return + } + } + if err != nil { + i.err = err + return + } + } + }() + return i +} + +func (i *Input) Reader(ctx context.Context) io.ReadCloser { + ctx, cancel := context.WithCancel(ctx) + return &inputReader{input: i, ctx: ctx, cancel: cancel} +} + +type inputReader struct { + input *Input + ctx context.Context + cancel context.CancelFunc + mu sync.Mutex +} + +func (r *inputReader) Read(p []byte) (int, error) { + r.mu.Lock() + defer r.mu.Unlock() + if len(p) == 0 { + return 0, nil + } + if err := r.ctx.Err(); err != nil { + return 0, err + } + select { + case b := <-r.input.bytes: + p[0] = b + return 1, nil + case <-r.ctx.Done(): + return 0, r.ctx.Err() + case <-r.input.done: + select { + case b := <-r.input.bytes: + p[0] = b + return 1, nil + default: + } + return 0, r.input.err + } +} + +func (r *inputReader) Close() error { + r.cancel() + // Wait for a pending Read to release ownership before the prompt resumes. + r.mu.Lock() + r.mu.Unlock() + return nil +} diff --git a/internal/wpshell/input_test.go b/internal/wpshell/input_test.go new file mode 100644 index 000000000..e9b4ec977 --- /dev/null +++ b/internal/wpshell/input_test.go @@ -0,0 +1,64 @@ +package wpshell + +import ( + "context" + "io" + "strings" + "testing" + "time" +) + +func TestInputForwardsRemoteInterruptAndDrainsEOF(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + interrupts := 0 + input := NewInput(ctx, strings.NewReader("\x03x"), func() bool { + interrupts++ + return false + }) + reader := input.Reader(ctx) + defer reader.Close() + got, err := io.ReadAll(reader) + if err != nil { + t.Fatal(err) + } + if string(got) != "\x03x" || interrupts != 1 { + t.Fatalf("remote stdin = %q, interrupts = %d", got, interrupts) + } +} + +func TestInputReturnsOwnershipToPrompt(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + in, writer := io.Pipe() + defer in.Close() + defer writer.Close() + input := NewInput(ctx, in) + command := input.Reader(ctx) + finished := make(chan error, 1) + go func() { + _, err := io.Copy(io.Discard, command) + finished <- err + }() + if err := command.Close(); err != nil { + t.Fatal(err) + } + select { + case err := <-finished: + if err != context.Canceled { + t.Fatalf("command read error = %v", err) + } + case <-time.After(time.Second): + t.Fatal("command reader did not stop") + } + go func() { _, _ = io.WriteString(writer, "wp option get home\n") }() + prompt := input.Reader(ctx) + defer prompt.Close() + buf := make([]byte, len("wp option get home\n")) + if _, err := io.ReadFull(prompt, buf); err != nil { + t.Fatal(err) + } + if string(buf) != "wp option get home\n" { + t.Fatalf("next shell command lost bytes: %q", buf) + } +} diff --git a/internal/wpshell/repl.go b/internal/wpshell/repl.go index 6caa5cccf..87c695b05 100644 --- a/internal/wpshell/repl.go +++ b/internal/wpshell/repl.go @@ -13,6 +13,8 @@ import ( type REPL struct { Prompt string Run func(command string) error + // ReadLine supplies terminal editing when set. It also renders the prompt. + ReadLine func() (string, error) } // Serve reads lines until EOF / exit. Port of the readline 'line' handler @@ -22,11 +24,20 @@ func (r *REPL) Serve(in *bufio.Reader, out io.Writer) error { state := NewCmdState() seenWP := false - fmt.Fprint(out, r.Prompt) + r.prompt(out) for { - line, err := in.ReadString('\n') - line = strings.TrimRight(line, "\n") + var line string + var err error + if r.ReadLine != nil { + line, err = r.ReadLine() + } else { + line, err = in.ReadString('\n') + line = strings.TrimSuffix(strings.TrimSuffix(line, "\n"), "\r") + } atEOF := err == io.EOF + if err != nil && !atEOF { + return err + } if !atEOF || line != "" { if r.handleLine(out, state, &seenWP, line) == exitREPL { @@ -39,6 +50,12 @@ func (r *REPL) Serve(in *bufio.Reader, out io.Writer) error { } } +func (r *REPL) prompt(out io.Writer) { + if r.ReadLine == nil { + fmt.Fprint(out, r.Prompt) + } +} + type lineResult int const ( @@ -49,7 +66,7 @@ const ( func (r *REPL) handleLine(out io.Writer, state *CmdState, seenWP *bool, line string) lineResult { // Blank line re-prompts (vip-wp.js:451). if line == "" { - fmt.Fprint(out, r.Prompt) + r.prompt(out) return continueREPL } // exit / exit; quits when not mid-command (vip-wp.js:457). @@ -63,7 +80,7 @@ func (r *REPL) handleLine(out io.Writer, state *CmdState, seenWP *bool, line str if !*seenWP { ResetState(state) fmt.Fprintln(out, "Error: invalid command, please pass a valid WP-CLI command.") - fmt.Fprint(out, r.Prompt) + r.prompt(out) return continueREPL } @@ -72,10 +89,11 @@ func (r *REPL) handleLine(out io.Writer, state *CmdState, seenWP *bool, line str return continueREPL // keep accumulating (multiline quote) } - cmd := strings.TrimPrefix(state.Command, "wp ") + cmd := strings.TrimLeft(state.Command, " \t") + cmd = strings.TrimLeft(strings.TrimPrefix(cmd, "wp"), " \t") *seenWP = false ResetState(state) _ = r.Run(cmd) - fmt.Fprint(out, r.Prompt) + r.prompt(out) return continueREPL } diff --git a/internal/wpstream/run.go b/internal/wpstream/run.go index 13646fa84..b9e309090 100644 --- a/internal/wpstream/run.go +++ b/internal/wpstream/run.go @@ -289,6 +289,8 @@ func runOnce(ctx context.Context, opts Options, cli *Client, ss *StreamSocket, o closeRunDone() // also aborts stdoutStream via watcher (redundant but safe) return Result{}, false case <-ctx.Done(): + closeRunDone() + <-stdoutDone return Result{}, true // caller checks ctx.Err() } } diff --git a/src/bin/vip-wp.js b/src/bin/vip-wp.js index 4f5509464..dc98586c4 100755 --- a/src/bin/vip-wp.js +++ b/src/bin/vip-wp.js @@ -76,6 +76,29 @@ const unpipeStreamsFromProcess = ( { stdin, stdout: outStream } ) => { outStream.unpipe( process.stdout ); }; +const finishCommand = async ( { subShellRl, commonTrackingParams, isSubShell, exitCode = 0 } ) => { + if ( currentJob.finished ) { + return; + } + currentJob.finished = true; + clearTimeout( currentJob.exitTimer ); + subShellRl.clearLine(); + commandRunning = false; + await trackEvent( 'wpcli_command_end', commonTrackingParams ); + currentJob.socket.close(); + unpipeStreamsFromProcess( { stdin: currentJob.stdinStream, stdout: currentJob.stdoutStream } ); + currentOffset = 0; + if ( ! isSubShell ) { + subShellRl.close(); + process.exit( exitCode ); + } + if ( exitCode ) { + console.log( chalk.red( `Error: WP-CLI command failed with exit code ${ exitCode }` ) ); + } + safeResume( subShellRl ); + safePrompt( subShellRl ); +}; + const bindStreamEvents = ( { subShellRl, commonTrackingParams, isSubShell, stdoutStream } ) => { const criticalErrors = [ 'ECONNRESET', @@ -99,24 +122,12 @@ const bindStreamEvents = ( { subShellRl, commonTrackingParams, isSubShell, stdou } ); stdoutStream.on( 'end', async () => { - subShellRl.clearLine(); - commandRunning = false; - - await trackEvent( 'wpcli_command_end', commonTrackingParams ); - - // Tell socket.io to stop trying to connect - currentJob.socket.close(); - unpipeStreamsFromProcess( { stdin: currentJob.stdinStream, stdout: currentJob.stdoutStream } ); - - // Reset offset - currentOffset = 0; - - if ( ! isSubShell ) { - subShellRl.close(); - process.exit(); + // Allow the server's exit event to deliver its status after stdout EOF. + if ( ! currentJob.finished ) { + currentJob.exitTimer = setTimeout( () => { + finishCommand( { subShellRl, commonTrackingParams, isSubShell } ); + }, 2000 ); } - safeResume( subShellRl ); - safePrompt( subShellRl ); } ); }; @@ -268,8 +279,7 @@ const bindReconnectEvents = ( { currentJob.stdinStream.destroy(); currentJob.stdoutStream.destroy(); - currentJob.socket.close(); - process.exit( exitCode ); + await finishCommand( { subShellRl, commonTrackingParams, isSubShell, exitCode } ); } ); currentJob.socket.io.on( 'reconnect_attempt', attempt => { diff --git a/testdata/parity/recordings/wp-websocket-redirect/resolve-app.json b/testdata/parity/recordings/wp-websocket-redirect/resolve-app.json deleted file mode 100644 index 5df8d6907..000000000 --- a/testdata/parity/recordings/wp-websocket-redirect/resolve-app.json +++ /dev/null @@ -1 +0,0 @@ -{"data":{"apps":{"edges":[{"id":42,"name":"parityapp","typeId":2,"environments":[{"id":7,"appId":42,"name":"develop","type":"develop","defaultDomain":"d.example"}],"organization":{"id":1,"name":"Parity Org"}}]}}} \ No newline at end of file diff --git a/testdata/parity/recordings/wp-websocket-shell/resolve-app.json b/testdata/parity/recordings/wp-websocket-shell/resolve-app.json new file mode 100644 index 000000000..079fc7711 --- /dev/null +++ b/testdata/parity/recordings/wp-websocket-shell/resolve-app.json @@ -0,0 +1 @@ +{"data":{"apps":{"edges":[{"id":42,"name":"parityapp","typeId":2,"environments":[{"id":7,"appId":42,"name":"develop","type":"develop","defaultDomain":"d.example","wpcliStrategy":"websocket","primaryDomain":{"name":"d.example"}}],"organization":{"id":1,"name":"Parity Org"}}]}}} diff --git a/testdata/parity/wp-websocket-redirect.yaml b/testdata/parity/wp-websocket-redirect.yaml deleted file mode 100644 index 85eb1996b..000000000 --- a/testdata/parity/wp-websocket-redirect.yaml +++ /dev/null @@ -1,14 +0,0 @@ -name: wp-websocket-redirect -description: | - WPEnvInfo returns wpcliStrategy:"websocket" for a develop environment. - The command must redirect with "requires the Node CLI" and exit 1. - TriggerWPCLICommand must NOT fire. This is the intended WP1 behavior, - not drift. -argv: ["@parityapp.develop", "wp", "site", "list"] -env: - DO_NOT_TRACK: "1" - NODE_ENV: test - NO_COLOR: "1" -recording: wp-websocket-redirect -expect: - exit_code: 1 diff --git a/testdata/parity/wp-websocket-shell.yaml b/testdata/parity/wp-websocket-shell.yaml new file mode 100644 index 000000000..9261fcf1d --- /dev/null +++ b/testdata/parity/wp-websocket-shell.yaml @@ -0,0 +1,13 @@ +name: wp-websocket-shell +description: | + A no-argument WebSocket wp command opens a shell without triggering WP-CLI. + Invalid input, a GraphQL error, two completed socket commands, and a delayed + nonzero socket exit each return to the prompt. Exit closes the shell. +argv: ["@parityapp.develop", "wp"] +env: + DO_NOT_TRACK: "1" + NODE_ENV: test + NO_COLOR: "1" +recording: wp-websocket-shell +expect: + exit_code: 0 From e4cbc6b104707ddc31a5a36ed1ee07928d24af7c Mon Sep 17 00:00:00 2001 From: Volodymyr Kolesnykov Date: Fri, 9 Oct 2026 11:37:31 +0300 Subject: [PATCH 02/13] fix: address SonarCloud issues --- src/bin/vip-wp.js | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/bin/vip-wp.js b/src/bin/vip-wp.js index dc98586c4..29d9c884f 100755 --- a/src/bin/vip-wp.js +++ b/src/bin/vip-wp.js @@ -76,15 +76,18 @@ const unpipeStreamsFromProcess = ( { stdin, stdout: outStream } ) => { outStream.unpipe( process.stdout ); }; -const finishCommand = async ( { subShellRl, commonTrackingParams, isSubShell, exitCode = 0 } ) => { +const finishCommand = ( { subShellRl, commonTrackingParams, isSubShell, exitCode = 0 } ) => { if ( currentJob.finished ) { return; } + currentJob.finished = true; clearTimeout( currentJob.exitTimer ); subShellRl.clearLine(); commandRunning = false; - await trackEvent( 'wpcli_command_end', commonTrackingParams ); + + trackEvent( 'wpcli_command_end', commonTrackingParams ).catch( () => {} ); + currentJob.socket.close(); unpipeStreamsFromProcess( { stdin: currentJob.stdinStream, stdout: currentJob.stdoutStream } ); currentOffset = 0; @@ -92,9 +95,11 @@ const finishCommand = async ( { subShellRl, commonTrackingParams, isSubShell, ex subShellRl.close(); process.exit( exitCode ); } + if ( exitCode ) { console.log( chalk.red( `Error: WP-CLI command failed with exit code ${ exitCode }` ) ); } + safeResume( subShellRl ); safePrompt( subShellRl ); }; @@ -121,7 +126,7 @@ const bindStreamEvents = ( { subShellRl, commonTrackingParams, isSubShell, stdou } } ); - stdoutStream.on( 'end', async () => { + stdoutStream.on( 'end', () => { // Allow the server's exit event to deliver its status after stdout EOF. if ( ! currentJob.finished ) { currentJob.exitTimer = setTimeout( () => { @@ -270,7 +275,7 @@ const bindReconnectEvents = ( { currentJob.socket.io.opts.transports = [ 'websocket', 'polling' ]; } ); - currentJob.socket.on( 'exit', async ( { exitCode, message } ) => { + currentJob.socket.on( 'exit', ( { exitCode, message } ) => { debug( 'socket: exit. Code: %d. Message: %s', exitCode, message ); if ( message ) { @@ -279,7 +284,7 @@ const bindReconnectEvents = ( { currentJob.stdinStream.destroy(); currentJob.stdoutStream.destroy(); - await finishCommand( { subShellRl, commonTrackingParams, isSubShell, exitCode } ); + finishCommand( { subShellRl, commonTrackingParams, isSubShell, exitCode } ); } ); currentJob.socket.io.on( 'reconnect_attempt', attempt => { From d05ea7ef33bd7ea0d275f8663c32f8bcb3ff2e1e Mon Sep 17 00:00:00 2001 From: Volodymyr Kolesnykov Date: Fri, 9 Oct 2026 12:19:04 +0300 Subject: [PATCH 03/13] fix: address Copilot review comments --- internal/wpshell/input.go | 44 ++++++++++++---- internal/wpshell/input_test.go | 34 +++++++++++++ internal/wpstream/run.go | 9 ++-- internal/wpstream/run_test.go | 93 ++++++++++++++++++++++++++++++++++ 4 files changed, 166 insertions(+), 14 deletions(-) diff --git a/internal/wpshell/input.go b/internal/wpshell/input.go index 6f841b46e..509ed0903 100644 --- a/internal/wpshell/input.go +++ b/internal/wpshell/input.go @@ -9,9 +9,11 @@ import ( // Input shares stdin between the prompt and remote commands. A command reader // can be cancelled without leaving a goroutine consuming the next prompt's input. type Input struct { - bytes chan byte - done chan struct{} - err error + bytes chan byte + done chan struct{} + err error + readMu sync.Mutex + pending *byte } func NewInput(ctx context.Context, in io.Reader, interrupt ...func() bool) *Input { @@ -47,40 +49,60 @@ func (i *Input) Reader(ctx context.Context) io.ReadCloser { } type inputReader struct { - input *Input - ctx context.Context - cancel context.CancelFunc - mu sync.Mutex + input *Input + ctx context.Context + cancel context.CancelFunc + mu sync.Mutex + stateMu sync.Mutex } func (r *inputReader) Read(p []byte) (int, error) { r.mu.Lock() defer r.mu.Unlock() + r.input.readMu.Lock() + defer r.input.readMu.Unlock() if len(p) == 0 { return 0, nil } if err := r.ctx.Err(); err != nil { return 0, err } + if r.input.pending != nil { + b := *r.input.pending + r.input.pending = nil + return r.deliver(p, b) + } select { case b := <-r.input.bytes: - p[0] = b - return 1, nil + return r.deliver(p, b) case <-r.ctx.Done(): return 0, r.ctx.Err() case <-r.input.done: select { case b := <-r.input.bytes: - p[0] = b - return 1, nil + return r.deliver(p, b) default: } return 0, r.input.err } } +func (r *inputReader) deliver(p []byte, b byte) (int, error) { + r.stateMu.Lock() + defer r.stateMu.Unlock() + if err := r.ctx.Err(); err != nil { + // Keep the selected byte ahead of any later input for the next owner. + r.input.pending = &b + return 0, err + } + p[0] = b + return 1, nil +} + func (r *inputReader) Close() error { + r.stateMu.Lock() r.cancel() + r.stateMu.Unlock() // Wait for a pending Read to release ownership before the prompt resumes. r.mu.Lock() r.mu.Unlock() diff --git a/internal/wpshell/input_test.go b/internal/wpshell/input_test.go index e9b4ec977..ecb4f5c1c 100644 --- a/internal/wpshell/input_test.go +++ b/internal/wpshell/input_test.go @@ -62,3 +62,37 @@ func TestInputReturnsOwnershipToPrompt(t *testing.T) { t.Fatalf("next shell command lost bytes: %q", buf) } } + +func TestInputPreservesSelectedByteDuringCancellation(t *testing.T) { + commandCtx, cancelCommand := context.WithCancel(context.Background()) + defer cancelCommand() + input := &Input{bytes: make(chan byte), done: make(chan struct{})} + command := input.Reader(commandCtx).(*inputReader) + // Hold delivery until the read has selected a byte and cancellation wins. + command.stateMu.Lock() + finished := make(chan error, 1) + go func() { + var b [1]byte + _, err := command.Read(b[:]) + finished <- err + }() + input.bytes <- 'w' + cancelCommand() + command.stateMu.Unlock() + if err := <-finished; err != context.Canceled { + t.Fatalf("command read error = %v", err) + } + if err := command.Close(); err != nil { + t.Fatal(err) + } + go func() { input.bytes <- 'p' }() + prompt := input.Reader(context.Background()) + defer prompt.Close() + var got [2]byte + if _, err := io.ReadFull(prompt, got[:]); err != nil { + t.Fatal(err) + } + if string(got[:]) != "wp" { + t.Fatalf("prompt input lost or reordered: %q", got) + } +} diff --git a/internal/wpstream/run.go b/internal/wpstream/run.go index b9e309090..090cc9da8 100644 --- a/internal/wpstream/run.go +++ b/internal/wpstream/run.go @@ -267,7 +267,12 @@ func runOnce(ctx context.Context, opts Options, cli *Client, ss *StreamSocket, o // Drain stdout: signal runDone so the watcher goroutine aborts the // stream, unblocking the stdout goroutine. closeRunDone() - <-stdoutDone + select { + case <-stdoutDone: + case <-ctx.Done(): + // An arbitrary Stdout writer may block even after the stream is + // aborted. Cancellation must not wait for that writer. + } return Result{ExitCode: exitCode}, true case <-stdoutDone: // stdout EOF before any exit event — the server always sends an 'exit' @@ -289,8 +294,6 @@ func runOnce(ctx context.Context, opts Options, cli *Client, ss *StreamSocket, o closeRunDone() // also aborts stdoutStream via watcher (redundant but safe) return Result{}, false case <-ctx.Done(): - closeRunDone() - <-stdoutDone return Result{}, true // caller checks ctx.Err() } } diff --git a/internal/wpstream/run_test.go b/internal/wpstream/run_test.go index 655406c05..ea67943b3 100644 --- a/internal/wpstream/run_test.go +++ b/internal/wpstream/run_test.go @@ -3,12 +3,27 @@ package wpstream import ( "bytes" "context" + "errors" "strings" + "sync" "sync/atomic" "testing" "time" ) +type blockingWriter struct { + entered chan struct{} + release chan struct{} + finished chan struct{} +} + +func (w *blockingWriter) Write(p []byte) (int, error) { + close(w.entered) + <-w.release + close(w.finished) + return len(p), nil +} + // loopbackPair holds both sides of an in-memory loopback for run tests. type loopbackPair struct { // client (A) side — passed to run() @@ -182,3 +197,81 @@ func TestRunStdoutStreamed(t *testing.T) { t.Errorf("stdout not streamed: %q", out) } } + +func TestRunCancellationDoesNotWaitForBlockedStdout(t *testing.T) { + for _, tc := range []struct { + name string + sendExit bool + }{ + {name: "context cancellation"}, + {name: "exit then context cancellation", sendExit: true}, + } { + t.Run(tc.name, func(t *testing.T) { + pair := newLoopbackPair(t) + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + w := &blockingWriter{ + entered: make(chan struct{}), + release: make(chan struct{}), + finished: make(chan struct{}), + } + var releaseOnce sync.Once + releaseWriter := func() { releaseOnce.Do(func() { close(w.release) }) } + t.Cleanup(releaseWriter) + + streamID := make(chan string, 1) + pair.ssB.On("cmd", func(args []any, _ *int) { + stdoutStream := args[2].(*IOStream) + streamID <- stdoutStream.id + _, _ = stdoutStream.Write([]byte("output")) + }) + + result := make(chan error, 1) + go func() { + _, err := run(ctx, Options{Stdin: strings.NewReader(""), Stdout: w}, pair.cliA, pair.ssA, nil, 0) + result <- err + }() + + select { + case <-w.entered: + case <-time.After(2 * time.Second): + t.Fatal("stdout writer was not called") + } + if tc.sendExit { + clientStdout := pair.ssA.get(<-streamID) + if clientStdout == nil { + t.Fatal("client stdout stream not found") + } + if err := pair.cliB.Emit(ctx, "exit", []any{map[string]any{"exitCode": float64(0)}}, nil); err != nil { + t.Fatalf("emit exit: %v", err) + } + select { + case <-clientStdout.closed: // runOnce has entered its exit drain. + case <-time.After(2 * time.Second): + t.Fatal("run did not enter exit drain") + } + select { + case <-result: + t.Fatal("run returned before blocked stdout was released or context cancelled") + default: + } + } + cancel() + select { + case err := <-result: + if !errors.Is(err, context.Canceled) { + t.Fatalf("run error = %v, want context.Canceled", err) + } + case <-time.After(time.Second): + t.Fatal("run waited for blocked stdout writer after cancellation") + } + + releaseWriter() + select { + case <-w.finished: + case <-time.After(time.Second): + t.Fatal("stdout writer did not finish after release") + } + }) + } +} From 9bfb5b4302a080945ce8efbc6d242a55c17839b0 Mon Sep 17 00:00:00 2001 From: Volodymyr Kolesnykov Date: Fri, 9 Oct 2026 12:33:29 +0300 Subject: [PATCH 04/13] fix: exclude Unix WP shell parity tests on Windows --- internal/parity/wp_shell_differential_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/parity/wp_shell_differential_test.go b/internal/parity/wp_shell_differential_test.go index 2c9253176..e9e24ea8e 100644 --- a/internal/parity/wp_shell_differential_test.go +++ b/internal/parity/wp_shell_differential_test.go @@ -1,4 +1,4 @@ -//go:build parity +//go:build parity && !windows package parity From 85034e291d0c0038fb6ee0488271e71be01d7f83 Mon Sep 17 00:00:00 2001 From: Volodymyr Kolesnykov Date: Fri, 9 Oct 2026 12:44:45 +0300 Subject: [PATCH 05/13] fix: reset WP shell interrupts for each command --- cmd/vip-next/commands/wp.go | 1 + internal/parity/wp_shell_differential_test.go | 87 ++++++++++--------- src/bin/vip-wp.js | 1 + 3 files changed, 49 insertions(+), 40 deletions(-) diff --git a/cmd/vip-next/commands/wp.go b/cmd/vip-next/commands/wp.go index 117b0afe4..7de7cee68 100644 --- a/cmd/vip-next/commands/wp.go +++ b/cmd/vip-next/commands/wp.go @@ -189,6 +189,7 @@ func runWPShell(cmd *cobra.Command, ae *appctx.AppEnv, info *wpEnvInfo) error { commandCtx, commandCancel := context.WithCancel(ctx) interruptMu.Lock() commandActive = true + interrupts = 0 commandInterrupted = false interruptMu.Unlock() cmd.SetContext(commandCtx) diff --git a/internal/parity/wp_shell_differential_test.go b/internal/parity/wp_shell_differential_test.go index e9e24ea8e..b8fa5f77f 100644 --- a/internal/parity/wp_shell_differential_test.go +++ b/internal/parity/wp_shell_differential_test.go @@ -82,8 +82,7 @@ type shellMock struct { streamExitCode int stdinStreamID string needsInput bool - inputReady chan struct{} - inputReadyOnce sync.Once + inputOpened chan struct{} remoteBytes chan byte } @@ -306,11 +305,9 @@ func (m *shellMock) startShellStream(ctx context.Context, ws *websocket.Conn, st if err := ws.Write(ctx, websocket.MessageText, []byte(read)); err != nil { return err } - m.inputReadyOnce.Do(func() { - if m.inputReady != nil { - close(m.inputReady) - } - }) + if m.inputOpened != nil { + m.inputOpened <- struct{}{} + } return nil } @@ -635,67 +632,77 @@ func TestWPWebsocketShellRestoresTerminalOnSIGTERM(t *testing.T) { } } -func TestWPWebsocketShellForwardsActiveCtrlC(t *testing.T) { +func TestWPWebsocketShellInterruptsAcrossCommands(t *testing.T) { rig, skip := differentialAvailable(t) if skip != "" { - t.Skip(LoudSkip("TestWPWebsocketShellForwardsActiveCtrlC", skip)) + t.Skip(LoudSkip("TestWPWebsocketShellInterruptsAcrossCommands", skip)) } scenario, err := LoadScenario("../../testdata/parity/wp-websocket-shell.yaml") if err != nil { t.Fatal(err) } scenario.Env = rig.scenarioEnv(scenario) + for side, bin := range map[string]string{"node": rig.nodeBin, "go": rig.goBin} { + t.Run(side, func(t *testing.T) { + testShellInterruptsAcrossCommands(t, rig, scenario, bin) + }) + } +} + +func testShellInterruptsAcrossCommands(t *testing.T, rig *differentialRig, scenario *Scenario, bin string) { + t.Helper() appBody, err := os.ReadFile("../../testdata/parity/recordings/wp-websocket-shell/resolve-app.json") if err != nil { t.Fatal(err) } mock := &shellMock{ appBody: appBody, poll: make(chan string, 2), - inputReady: make(chan struct{}), remoteBytes: make(chan byte, 16), + inputOpened: make(chan struct{}, 2), remoteBytes: make(chan byte, 16), } rig.serve(t, mock) ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) defer cancel() - cmd := exec.CommandContext(ctx, rig.goBin, scenario.Argv...) + cmd := exec.CommandContext(ctx, bin, scenario.Argv...) cmd.Env = FixtureEnv(scenario.Env) - stdin, err := cmd.StdinPipe() + stdin, err := pty.StartWithSize(cmd, &pty.Winsize{Rows: 24, Cols: 100}) if err != nil { t.Fatal(err) } defer stdin.Close() - stdout, stderr := newShellOutput(), newShellOutput() - cmd.Stdout, cmd.Stderr = stdout, stderr - if err := cmd.Start(); err != nil { - t.Fatal(err) - } + stdout := newShellOutput() + go func() { _, _ = io.Copy(stdout, stdin) }() done := make(chan error, 1) go func() { done <- cmd.Wait() }() prompt := "parityapp.develop:~$ " - _ = waitForShellOutput(t, stdout, done, 0, prompt) - if _, err := io.WriteString(stdin, "wp eval read\n"); err != nil { - t.Fatal(err) - } - select { - case <-mock.inputReady: - case <-ctx.Done(): - t.Fatalf("remote stdin was not opened: %v\nstdout: %q\nstderr: %q", ctx.Err(), stdout.String(), stderr.String()) - } - if _, err := stdin.Write([]byte{3}); err != nil { - t.Fatal(err) - } - select { - case b := <-mock.remoteBytes: - if b != 3 { - t.Errorf("remote stdin received %#x, want Ctrl-C (0x03)", b) + pos := waitForShellOutput(t, stdout, done, 0, prompt) + for command := 0; command < 2; command++ { + if _, err := io.WriteString(stdin, "wp eval read\n"); err != nil { + t.Fatal(err) } - case <-ctx.Done(): - t.Fatalf("remote stdin did not receive Ctrl-C: %v\nstdout: %q", ctx.Err(), stdout.String()) + select { + case <-mock.inputOpened: + case <-ctx.Done(): + t.Fatalf("remote stdin was not opened: %v\nstdout: %q", ctx.Err(), stdout.String()) + } + if _, err := stdin.Write([]byte{3}); err != nil { + t.Fatal(err) + } + if bin == rig.goBin { + select { + case b := <-mock.remoteBytes: + if b != 3 { + t.Errorf("remote stdin received %#x, want Ctrl-C (0x03)", b) + } + case <-ctx.Done(): + t.Fatalf("remote stdin did not receive Ctrl-C: %v\nstdout: %q", ctx.Err(), stdout.String()) + } + } + pos = waitForShellOutput(t, stdout, done, pos, "Command cancelled by user") + pos = waitForShellOutput(t, stdout, done, pos, prompt) } - pos := waitForShellOutput(t, stdout, done, 0, "Command cancelled by user") - _ = waitForShellOutput(t, stdout, done, pos, prompt) - if got := mock.commandsSeen(); len(got) != 1 || got[0] != "eval read" { - t.Errorf("trigger commands = %q, want [eval read]", got) + if got := mock.commandsSeen(); !reflect.DeepEqual(got, []string{"eval read", "eval read"}) { + t.Errorf("trigger commands = %q, want [eval read eval read]", got) } if _, err := io.WriteString(stdin, "exit\n"); err != nil { t.Fatal(err) @@ -703,7 +710,7 @@ func TestWPWebsocketShellForwardsActiveCtrlC(t *testing.T) { select { case err := <-done: if err != nil { - t.Errorf("shell exit after Ctrl-C: %v\nstdout: %q\nstderr: %q", err, stdout.String(), stderr.String()) + t.Errorf("shell exit after Ctrl-C: %v\nstdout: %q", err, stdout.String()) } case <-ctx.Done(): t.Fatalf("shell did not exit after Ctrl-C then exit: %v\nstdout: %q", ctx.Err(), stdout.String()) diff --git a/src/bin/vip-wp.js b/src/bin/vip-wp.js index 29d9c884f..021a9f70c 100755 --- a/src/bin/vip-wp.js +++ b/src/bin/vip-wp.js @@ -505,6 +505,7 @@ commandWrapper( { } safePause( subShellRl ); + countSIGINT = 0; let result; const wpCliCmd = commandState.command.replace( /^wp\s+/, '' ); From 6a8dd144b8017d8f34900b48fc1ed0816a037f94 Mon Sep 17 00:00:00 2001 From: Volodymyr Kolesnykov Date: Fri, 9 Oct 2026 12:55:55 +0300 Subject: [PATCH 06/13] fix: preserve WP shell SIGTERM exit status --- cmd/vip-next/commands/wp.go | 24 +++++++++++- internal/parity/wp_shell_differential_test.go | 38 +++++++++++++++++-- 2 files changed, 56 insertions(+), 6 deletions(-) diff --git a/cmd/vip-next/commands/wp.go b/cmd/vip-next/commands/wp.go index 7de7cee68..5cf2b1d6d 100644 --- a/cmd/vip-next/commands/wp.go +++ b/cmd/vip-next/commands/wp.go @@ -158,8 +158,28 @@ func runWPShell(cmd *cobra.Command, ae *appctx.AppEnv, info *wpEnvInfo) error { } prompt := fmt.Sprintf("%s.%s:~$ ", ae.App.Name, identifier) originalCtx := cmd.Context() - ctx, cancel := signal.NotifyContext(originalCtx, syscall.SIGINT, syscall.SIGTERM) - defer cancel() + ctx, cancel := context.WithCancel(originalCtx) + signals := make(chan os.Signal, 1) + signal.Notify(signals, syscall.SIGINT, syscall.SIGTERM) + signalDone := make(chan struct{}) + var receivedSignal os.Signal + go func() { + defer close(signalDone) + select { + case receivedSignal = <-signals: + cancel() + case <-ctx.Done(): + } + }() + // Registered before terminal cleanup so restoration finishes before exit. + defer func() { + cancel() + signal.Stop(signals) + <-signalDone + if receivedSignal == syscall.SIGTERM { + exit.WithCode(128+int(syscall.SIGTERM), nil) + } + }() cmd.SetContext(ctx) defer cmd.SetContext(originalCtx) var interruptMu sync.Mutex diff --git a/internal/parity/wp_shell_differential_test.go b/internal/parity/wp_shell_differential_test.go index b8fa5f77f..531ec761e 100644 --- a/internal/parity/wp_shell_differential_test.go +++ b/internal/parity/wp_shell_differential_test.go @@ -571,11 +571,20 @@ func TestWPWebsocketShellRestoresTerminalOnSIGTERM(t *testing.T) { t.Fatal(err) } scenario.Env = rig.scenarioEnv(scenario) + for _, active := range []bool{false, true} { + t.Run(fmt.Sprintf("command_active=%t", active), func(t *testing.T) { + testShellSIGTERM(t, rig, scenario, active) + }) + } +} + +func testShellSIGTERM(t *testing.T, rig *differentialRig, scenario *Scenario, active bool) { + t.Helper() appBody, err := os.ReadFile("../../testdata/parity/recordings/wp-websocket-shell/resolve-app.json") if err != nil { t.Fatal(err) } - mock := &shellMock{appBody: appBody, poll: make(chan string, 2)} + mock := &shellMock{appBody: appBody, poll: make(chan string, 2), inputOpened: make(chan struct{}, 1)} rig.serve(t, mock) master, slave, err := pty.Open() @@ -605,6 +614,16 @@ func TestWPWebsocketShellRestoresTerminalOnSIGTERM(t *testing.T) { done := make(chan error, 1) go func() { done <- cmd.Wait() }() _ = waitForShellOutput(t, output, done, 0, "parityapp.develop:~$ ") + if active { + if _, err := io.WriteString(master, "wp eval read\n"); err != nil { + t.Fatal(err) + } + select { + case <-mock.inputOpened: + case <-ctx.Done(): + t.Fatalf("remote stdin was not opened: %v\noutput: %q", ctx.Err(), output.String()) + } + } during, err := term.GetState(int(slave.Fd())) if err != nil { t.Fatal(err) @@ -616,7 +635,11 @@ func TestWPWebsocketShellRestoresTerminalOnSIGTERM(t *testing.T) { t.Fatal(err) } select { - case <-done: + case err := <-done: + var exitErr *exec.ExitError + if !errors.As(err, &exitErr) || exitErr.ExitCode() != 128+int(syscall.SIGTERM) { + t.Errorf("Go shell SIGTERM exit = %v, want status 143", err) + } case <-ctx.Done(): t.Fatalf("Go shell did not stop after SIGTERM: %v\noutput: %q", ctx.Err(), output.String()) } @@ -627,8 +650,15 @@ func TestWPWebsocketShellRestoresTerminalOnSIGTERM(t *testing.T) { if !reflect.DeepEqual(before, after) { t.Errorf("Go shell left the terminal in raw mode after SIGTERM") } - if got := mock.commandsSeen(); len(got) != 0 { - t.Errorf("SIGTERM caused a command trigger: %q", got) + if strings.Contains(output.String(), "context canceled") { + t.Errorf("Go shell printed a cancellation error after SIGTERM: %q", output.String()) + } + var wantCommands []string + if active { + wantCommands = []string{"eval read"} + } + if got := mock.commandsSeen(); !reflect.DeepEqual(got, wantCommands) { + t.Errorf("trigger commands = %q, want %q", got, wantCommands) } } From 9ca897e62f438661540defc86347e524a4a9c0d3 Mon Sep 17 00:00:00 2001 From: Volodymyr Kolesnykov Date: Fri, 9 Oct 2026 13:16:12 +0300 Subject: [PATCH 07/13] fix: harden WP shell prompts and reconnect lifecycle --- __tests__/bin/vip-wp.test.js | 179 ++++++++++++++++++ cmd/vip-next/commands/wp.go | 13 +- internal/parity/wp_shell_differential_test.go | 174 +++++++++++++++-- internal/wpshell/repl.go | 6 +- internal/wpshell/repl_test.go | 26 +++ internal/wpstream/run.go | 4 + src/bin/vip-wp.js | 107 ++++++++--- 7 files changed, 468 insertions(+), 41 deletions(-) create mode 100644 __tests__/bin/vip-wp.test.js diff --git a/__tests__/bin/vip-wp.test.js b/__tests__/bin/vip-wp.test.js new file mode 100644 index 000000000..c5aadd3e2 --- /dev/null +++ b/__tests__/bin/vip-wp.test.js @@ -0,0 +1,179 @@ +import { beforeEach, afterEach, expect, jest, test } from '@jest/globals'; +import { EventEmitter } from 'node:events'; + +let mockHandler; +let mockReadline; +let mockSocket; +let mockStreamSocket; +let mockStreams; +const mockTrackEvent = jest.fn(); + +jest.mock( '../../src/lib/cli/command', () => ( { + __esModule: true, + default: () => { + const runner = { + option: () => runner, + examples: () => runner, + argv: ( argv, handler ) => { + mockHandler = handler; + }, + }; + return runner; + }, + getEnvIdentifier: () => 'develop', +} ) ); +jest.mock( '../../src/lib/api', () => ( { + __esModule: true, + API_HOST: 'https://test.invalid', + disableGlobalGraphQLErrorHandling: jest.fn(), + default: () => ( { + mutate: async () => ( { + data: { + triggerWPCLICommandOnAppEnvironment: { command: { guid: 'test' }, inputToken: 'test' }, + }, + } ), + } ), +} ) ); +jest.mock( '../../src/commands/wp-ssh', () => ( { WPCliCommandOverSSH: jest.fn() } ) ); +jest.mock( '../../src/lib/token', () => ( { + __esModule: true, + default: { get: async () => ( { raw: 'test' } ) }, +} ) ); +jest.mock( '../../src/lib/tracker', () => ( { + trackEvent: ( ...args ) => mockTrackEvent( ...args ), +} ) ); +jest.mock( '../../src/lib/http/proxy-agent', () => ( { createProxyAgent: jest.fn() } ) ); +jest.mock( '../../src/lib/cli/prompt', () => ( { confirm: jest.fn() } ) ); +jest.mock( 'readline', () => ( { + __esModule: true, + default: { createInterface: () => mockReadline }, +} ) ); +jest.mock( 'socket.io-client', () => ( { __esModule: true, default: () => mockSocket } ) ); +jest.mock( 'socket.io-stream', () => { + const factory = () => mockStreamSocket; + factory.createStream = () => { + const stream = new ( require( 'node:events' ).EventEmitter )(); + stream.pipe = jest.fn( () => stream ); + stream.unpipe = jest.fn(); + stream.destroy = jest.fn(); + mockStreams.push( stream ); + return stream; + }; + return { __esModule: true, default: factory }; +} ); + +const options = { + app: { id: 1, name: 'test', typeId: 2, organization: { id: 1 } }, + env: { + id: 2, + type: 'develop', + wpcliStrategy: 'websocket', + primaryDomain: { name: 'test.invalid' }, + }, +}; + +beforeEach( () => { + jest.resetModules(); + jest.useFakeTimers(); + mockStreams = []; + mockTrackEvent.mockReset().mockResolvedValue( undefined ); + mockReadline = new EventEmitter(); + for ( const method of [ 'pause', 'resume', 'prompt', 'clearLine', 'close', 'write' ] ) { + mockReadline[ method ] = jest.fn(); + } + mockSocket = new EventEmitter(); + mockStreamSocket = new EventEmitter(); + jest.spyOn( mockStreamSocket, 'emit' ); + mockSocket.close = jest.fn(); + mockSocket.io = new EventEmitter(); + mockSocket.io.opts = {}; + mockSocket.io.engine = { close: jest.fn() }; + jest.spyOn( process.stdin, 'pipe' ).mockReturnValue( process.stdin ); + jest.spyOn( process.stdin, 'unpipe' ).mockReturnValue( process.stdin ); + jest.spyOn( process.stdout, 'write' ).mockReturnValue( true ); + jest.spyOn( process, 'exit' ).mockImplementation( () => {} ); + jest.spyOn( console, 'log' ).mockImplementation( () => {} ); + require( '../../src/bin/vip-wp' ); +} ); + +afterEach( () => { + jest.clearAllTimers(); + jest.useRealTimers(); + jest.restoreAllMocks(); +} ); + +async function startCommand( isSubShell = true ) { + await mockHandler( isSubShell ? [] : [ 'option', 'get', 'home' ], options ); + await mockReadline.listeners( 'line' )[ 0 ]( 'wp option get home' ); +} + +test( 'stale stream completion and retry timers cannot finish a reconnected job', async () => { + await startCommand(); + const oldStdout = mockStreams[ 0 ]; + oldStdout.emit( 'data', Buffer.from( 'old' ) ); + oldStdout.emit( 'end' ); + mockSocket.emit( 'retry' ); + mockSocket.io.emit( 'reconnect_attempt', 1 ); + mockSocket.io.emit( 'reconnect' ); + oldStdout.emit( 'end' ); + oldStdout.emit( 'error', new Error( 'stale stream' ) ); + oldStdout.emit( 'data', Buffer.from( 'stale' ) ); + mockSocket.io.emit( 'reconnect' ); + expect( + mockStreamSocket.emit.mock.calls.filter( ( [ event ] ) => event === 'cmd' ).at( -1 )[ 1 ].offset + ).toBe( 3 ); + await jest.advanceTimersByTimeAsync( 6000 ); + expect( mockSocket.close ).not.toHaveBeenCalled(); + expect( mockSocket.io.engine.close ).not.toHaveBeenCalled(); + expect( + mockTrackEvent.mock.calls.filter( ( [ event ] ) => event === 'wpcli_command_end' ) + ).toHaveLength( 0 ); + mockSocket.emit( 'exit', { exitCode: 3, message: 'done' } ); + expect( mockSocket.close ).toHaveBeenCalledTimes( 1 ); +} ); + +test( 'repeated reconnects retain one socket listener and finish exactly once', async () => { + const externalErrorHandler = jest.fn(); + mockSocket.on( 'error', externalErrorHandler ); + await startCommand(); + mockSocket.io.emit( 'reconnect' ); + mockSocket.io.emit( 'reconnect' ); + for ( const event of [ 'exit', 'cancel', 'unauthorized', 'retry' ] ) { + expect( mockSocket.listenerCount( event ) ).toBe( 1 ); + } + expect( mockSocket.listeners( 'error' ) ).toContain( externalErrorHandler ); + expect( mockSocket.listenerCount( 'error' ) ).toBe( 2 ); + mockSocket.emit( 'exit', { exitCode: 3, message: 'done' } ); + mockSocket.emit( 'cancel', 'stale cancellation' ); + expect( process.exit ).not.toHaveBeenCalled(); + mockSocket.emit( 'exit', { exitCode: 3, message: 'done' } ); + expect( console.log.mock.calls.filter( ( [ message ] ) => message === 'done' ) ).toHaveLength( + 1 + ); + expect( mockSocket.close ).toHaveBeenCalledTimes( 1 ); + expect( + mockTrackEvent.mock.calls.filter( ( [ event ] ) => event === 'wpcli_command_end' ) + ).toHaveLength( 1 ); +} ); + +test.each( [ false, true ] )( + 'one-shot exit waits for end telemetry (reject=%s)', + async shouldReject => { + let settle; + mockTrackEvent.mockImplementation( event => + event === 'wpcli_command_end' + ? new Promise( ( resolve, reject ) => { + settle = shouldReject ? reject : resolve; + } ) + : Promise.resolve() + ); + await startCommand( false ); + mockSocket.emit( 'exit', { exitCode: 3 } ); + expect( process.exit ).not.toHaveBeenCalled(); + expect( mockSocket.close ).toHaveBeenCalledTimes( 1 ); + settle(); + await Promise.resolve(); + await Promise.resolve(); + expect( process.exit ).toHaveBeenCalledWith( 3 ); + } +); diff --git a/cmd/vip-next/commands/wp.go b/cmd/vip-next/commands/wp.go index 5cf2b1d6d..5e72d84a5 100644 --- a/cmd/vip-next/commands/wp.go +++ b/cmd/vip-next/commands/wp.go @@ -176,8 +176,8 @@ func runWPShell(cmd *cobra.Command, ae *appctx.AppEnv, info *wpEnvInfo) error { cancel() signal.Stop(signals) <-signalDone - if receivedSignal == syscall.SIGTERM { - exit.WithCode(128+int(syscall.SIGTERM), nil) + if receivedSignal != nil { + exit.WithCode(128+int(receivedSignal.(syscall.Signal)), nil) } }() cmd.SetContext(ctx) @@ -255,7 +255,14 @@ func runWPShell(cmd *cobra.Command, ae *appctx.AppEnv, info *wpEnvInfo) error { defer cmd.SetOut(originalOut) out = terminal cmd.SetOut(out) - loop.ReadLine = terminal.ReadLine + loop.ReadLine = func(continuation bool) (string, error) { + if continuation { + terminal.SetPrompt("") + } else { + terminal.SetPrompt(prompt) + } + return terminal.ReadLine() + } } err := loop.Serve(bufio.NewReader(reader), out) interruptMu.Lock() diff --git a/internal/parity/wp_shell_differential_test.go b/internal/parity/wp_shell_differential_test.go index 531ec761e..0740ab649 100644 --- a/internal/parity/wp_shell_differential_test.go +++ b/internal/parity/wp_shell_differential_test.go @@ -84,6 +84,8 @@ type shellMock struct { needsInput bool inputOpened chan struct{} remoteBytes chan byte + reconnect bool + reconnectRuns int } func (m *shellMock) ServeHTTP(w http.ResponseWriter, r *http.Request) { @@ -130,6 +132,8 @@ func (m *shellMock) ServeHTTP(w http.ResponseWriter, r *http.Request) { guid = "ws-fail-guid" } else if request.Variables.Input.Command == "eval read" { guid = "ws-stdin-guid" + } else if request.Variables.Input.Command == "eval reconnect" { + guid = "ws-reconnect-guid" } _, _ = fmt.Fprintf(w, `{"data":{"triggerWPCLICommandOnAppEnvironment":{"inputToken":"tok-ws","command":{"guid":%q},"sshAuthentication":null}}}`, guid) } @@ -287,6 +291,7 @@ func (m *shellMock) recordStreamID(packet string) string { m.stdinStreamID = stdinStream.ID m.streamExitCode = 0 m.needsInput = data.GUID == "ws-stdin-guid" + m.reconnect = data.GUID == "ws-reconnect-guid" if data.GUID == "ws-fail-guid" { m.streamExitCode = 3 } @@ -295,6 +300,30 @@ func (m *shellMock) recordStreamID(packet string) string { } func (m *shellMock) startShellStream(ctx context.Context, ws *websocket.Conn, stdoutID, stdinID string, needsInput bool, exitCode int) error { + m.mu.Lock() + reconnect := m.reconnect + if reconnect { + m.reconnectRuns++ + } + run := m.reconnectRuns + m.mu.Unlock() + if reconnect { + if run == 1 { + if err := finishShellStream(ctx, ws, stdoutID, 0); err != nil { + return err + } + // Let stdout EOF be observed before the transport drops. + time.Sleep(100 * time.Millisecond) + ws.CloseNow() + return errors.New("simulated disconnect after EOF") + } + select { + case <-time.After(3 * time.Second): + return finishShellStream(ctx, ws, stdoutID, 3) + case <-ctx.Done(): + return ctx.Err() + } + } if !needsInput { return finishShellStream(ctx, ws, stdoutID, exitCode) } @@ -543,6 +572,64 @@ func TestWPWebsocketShellPTYDifferential(t *testing.T) { } } +func TestWPWebsocketShellMultilinePTY(t *testing.T) { + rig, skip := differentialAvailable(t) + if skip != "" { + t.Skip(LoudSkip("TestWPWebsocketShellMultilinePTY", skip)) + } + scenario, err := LoadScenario("../../testdata/parity/wp-websocket-shell.yaml") + if err != nil { + t.Fatal(err) + } + scenario.Env = rig.scenarioEnv(scenario) + for side, bin := range map[string]string{"node": rig.nodeBin, "go": rig.goBin} { + t.Run(side, func(t *testing.T) { + appBody, err := os.ReadFile("../../testdata/parity/recordings/wp-websocket-shell/resolve-app.json") + if err != nil { + t.Fatal(err) + } + mock := &shellMock{appBody: appBody, poll: make(chan string, 2)} + rig.serve(t, mock) + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, bin, scenario.Argv...) + cmd.Env = FixtureEnv(scenario.Env) + terminal, err := pty.StartWithSize(cmd, &pty.Winsize{Rows: 24, Cols: 100}) + if err != nil { + t.Fatal(err) + } + defer terminal.Close() + output := newShellOutput() + go func() { _, _ = io.Copy(output, terminal) }() + done := make(chan error, 1) + go func() { done <- cmd.Wait() }() + prompt := "parityapp.develop:~$ " + pos := waitForShellOutput(t, output, done, 0, prompt) + if _, err := io.WriteString(terminal, "wp option set key \"line1\nline2\"\n"); err != nil { + t.Fatal(err) + } + _ = waitForShellOutput(t, output, done, pos, prompt) + if got := strings.Count(output.String(), prompt); got != 2 { + t.Errorf("multiline command showed %d prompts, want 2: %q", got, output.String()) + } + if got := mock.commandsSeen(); !reflect.DeepEqual(got, []string{"option set key \"line1\nline2\""}) { + t.Errorf("multiline commands = %q", got) + } + if _, err := io.WriteString(terminal, "exit\n"); err != nil { + t.Fatal(err) + } + select { + case err := <-done: + if err != nil { + t.Fatalf("shell exit: %v\noutput: %q", err, output.String()) + } + case <-ctx.Done(): + t.Fatalf("shell did not exit: %v\noutput: %q", ctx.Err(), output.String()) + } + }) + } +} + func TestWPWebsocketShellPTYCtrlC(t *testing.T) { rig, skip := differentialAvailable(t) if skip != "" { @@ -561,24 +648,26 @@ func TestWPWebsocketShellPTYCtrlC(t *testing.T) { } } -func TestWPWebsocketShellRestoresTerminalOnSIGTERM(t *testing.T) { +func TestWPWebsocketShellRestoresTerminalOnSignal(t *testing.T) { rig, skip := differentialAvailable(t) if skip != "" { - t.Skip(LoudSkip("TestWPWebsocketShellRestoresTerminalOnSIGTERM", skip)) + t.Skip(LoudSkip("TestWPWebsocketShellRestoresTerminalOnSignal", skip)) } scenario, err := LoadScenario("../../testdata/parity/wp-websocket-shell.yaml") if err != nil { t.Fatal(err) } scenario.Env = rig.scenarioEnv(scenario) - for _, active := range []bool{false, true} { - t.Run(fmt.Sprintf("command_active=%t", active), func(t *testing.T) { - testShellSIGTERM(t, rig, scenario, active) - }) + for _, sig := range []syscall.Signal{syscall.SIGTERM, syscall.SIGINT} { + for _, active := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/command_active=%t", sig, active), func(t *testing.T) { + testShellSignal(t, rig, scenario, active, sig) + }) + } } } -func testShellSIGTERM(t *testing.T, rig *differentialRig, scenario *Scenario, active bool) { +func testShellSignal(t *testing.T, rig *differentialRig, scenario *Scenario, active bool, sig syscall.Signal) { t.Helper() appBody, err := os.ReadFile("../../testdata/parity/recordings/wp-websocket-shell/resolve-app.json") if err != nil { @@ -631,27 +720,27 @@ func testShellSIGTERM(t *testing.T, rig *differentialRig, scenario *Scenario, ac if reflect.DeepEqual(before, during) { t.Error("Go shell did not put the terminal in raw mode") } - if err := cmd.Process.Signal(syscall.SIGTERM); err != nil { + if err := cmd.Process.Signal(sig); err != nil { t.Fatal(err) } select { case err := <-done: var exitErr *exec.ExitError - if !errors.As(err, &exitErr) || exitErr.ExitCode() != 128+int(syscall.SIGTERM) { - t.Errorf("Go shell SIGTERM exit = %v, want status 143", err) + if !errors.As(err, &exitErr) || exitErr.ExitCode() != 128+int(sig) { + t.Errorf("Go shell %s exit = %v, want status %d", sig, err, 128+int(sig)) } case <-ctx.Done(): - t.Fatalf("Go shell did not stop after SIGTERM: %v\noutput: %q", ctx.Err(), output.String()) + t.Fatalf("Go shell did not stop after %s: %v\noutput: %q", sig, ctx.Err(), output.String()) } after, err := term.GetState(int(slave.Fd())) if err != nil { t.Fatal(err) } if !reflect.DeepEqual(before, after) { - t.Errorf("Go shell left the terminal in raw mode after SIGTERM") + t.Errorf("Go shell left the terminal in raw mode after %s", sig) } if strings.Contains(output.String(), "context canceled") { - t.Errorf("Go shell printed a cancellation error after SIGTERM: %q", output.String()) + t.Errorf("Go shell printed a cancellation error after %s: %q", sig, output.String()) } var wantCommands []string if active { @@ -747,6 +836,65 @@ func testShellInterruptsAcrossCommands(t *testing.T, rig *differentialRig, scena } } +func TestWPWebsocketShellReconnectExit(t *testing.T) { + rig, skip := differentialAvailable(t) + if skip != "" { + t.Skip(LoudSkip("TestWPWebsocketShellReconnectExit", skip)) + } + scenario, err := LoadScenario("../../testdata/parity/wp-websocket-shell.yaml") + if err != nil { + t.Fatal(err) + } + scenario.Env = rig.scenarioEnv(scenario) + for side, bin := range map[string]string{"node": rig.nodeBin, "go": rig.goBin} { + t.Run(side, func(t *testing.T) { + appBody, err := os.ReadFile("../../testdata/parity/recordings/wp-websocket-shell/resolve-app.json") + if err != nil { + t.Fatal(err) + } + mock := &shellMock{appBody: appBody, poll: make(chan string, 2)} + rig.serve(t, mock) + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, bin, scenario.Argv...) + cmd.Env = FixtureEnv(scenario.Env) + stdin, err := cmd.StdinPipe() + if err != nil { + t.Fatal(err) + } + defer stdin.Close() + output := newShellOutput() + cmd.Stdout, cmd.Stderr = output, output + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + done := make(chan error, 1) + go func() { done <- cmd.Wait() }() + prompt := "parityapp.develop:~$ " + _ = waitForShellOutput(t, output, done, 0, prompt) + if _, err := io.WriteString(stdin, "wp eval reconnect\n"); err != nil { + t.Fatal(err) + } + pos := waitForShellOutput(t, output, done, 0, "Error: WP-CLI command failed with exit code 3") + _ = waitForShellOutput(t, output, done, pos, prompt) + if got := mock.commandsSeen(); !reflect.DeepEqual(got, []string{"eval reconnect"}) { + t.Errorf("reconnect triggered new commands: %q", got) + } + if _, err := io.WriteString(stdin, "exit\n"); err != nil { + t.Fatal(err) + } + select { + case err := <-done: + if err != nil { + t.Fatalf("shell exit: %v\noutput: %q", err, output.String()) + } + case <-ctx.Done(): + t.Fatalf("shell did not exit: %v\noutput: %q", ctx.Err(), output.String()) + } + }) + } +} + func TestWPWebsocketOneShotDelayedNonzeroExit(t *testing.T) { rig, skip := differentialAvailable(t) if skip != "" { diff --git a/internal/wpshell/repl.go b/internal/wpshell/repl.go index 87c695b05..faa76b13f 100644 --- a/internal/wpshell/repl.go +++ b/internal/wpshell/repl.go @@ -13,8 +13,8 @@ import ( type REPL struct { Prompt string Run func(command string) error - // ReadLine supplies terminal editing when set. It also renders the prompt. - ReadLine func() (string, error) + // ReadLine supplies terminal editing, suppressing the prompt for continuations. + ReadLine func(continuation bool) (string, error) } // Serve reads lines until EOF / exit. Port of the readline 'line' handler @@ -29,7 +29,7 @@ func (r *REPL) Serve(in *bufio.Reader, out io.Writer) error { var line string var err error if r.ReadLine != nil { - line, err = r.ReadLine() + line, err = r.ReadLine(seenWP) } else { line, err = in.ReadString('\n') line = strings.TrimSuffix(strings.TrimSuffix(line, "\n"), "\r") diff --git a/internal/wpshell/repl_test.go b/internal/wpshell/repl_test.go index fa7883336..e875acc4f 100644 --- a/internal/wpshell/repl_test.go +++ b/internal/wpshell/repl_test.go @@ -2,6 +2,7 @@ package wpshell import ( "bufio" + "reflect" "strings" "testing" ) @@ -22,6 +23,31 @@ func TestREPLRunsValidCommand(t *testing.T) { } } +func TestREPLTerminalContinuation(t *testing.T) { + lines := []string{"wp option set k \"line1", "line2\"", "invalid", "exit"} + var continuations []bool + var ran []string + loop := &REPL{ + Run: func(cmd string) error { ran = append(ran, cmd); return nil }, + ReadLine: func(continuation bool) (string, error) { + continuations = append(continuations, continuation) + line := lines[0] + lines = lines[1:] + return line, nil + }, + } + var out strings.Builder + if err := loop.Serve(nil, &out); err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(continuations, []bool{false, true, false, false}) { + t.Errorf("continuation modes = %v", continuations) + } + if !reflect.DeepEqual(ran, []string{"option set k \"line1\nline2\""}) { + t.Errorf("commands = %q", ran) + } +} + func TestREPLInvalidCommand(t *testing.T) { in := strings.NewReader("ls -la\nexit\n") var out strings.Builder diff --git a/internal/wpstream/run.go b/internal/wpstream/run.go index 090cc9da8..85dbbf216 100644 --- a/internal/wpstream/run.go +++ b/internal/wpstream/run.go @@ -280,6 +280,10 @@ func runOnce(ctx context.Context, opts Options, cli *Client, ss *StreamSocket, o // so a non-zero exit code is not silently lost. select { case exitCode = <-exitCh: + case <-disconnected: + ss.abortAll(io.ErrUnexpectedEOF) + closeRunDone() + return Result{}, false case <-time.After(2 * time.Second): case <-ctx.Done(): } diff --git a/src/bin/vip-wp.js b/src/bin/vip-wp.js index 021a9f70c..be2759084 100755 --- a/src/bin/vip-wp.js +++ b/src/bin/vip-wp.js @@ -76,24 +76,32 @@ const unpipeStreamsFromProcess = ( { stdin, stdout: outStream } ) => { outStream.unpipe( process.stdout ); }; -const finishCommand = ( { subShellRl, commonTrackingParams, isSubShell, exitCode = 0 } ) => { - if ( currentJob.finished ) { +const finishCommand = ( { + subShellRl, + commonTrackingParams, + isSubShell, + exitCode = 0, + job = currentJob, +} ) => { + if ( job !== currentJob || job.finished ) { return; } currentJob.finished = true; clearTimeout( currentJob.exitTimer ); + clearTimeout( currentJob.retryTimer ); subShellRl.clearLine(); commandRunning = false; - trackEvent( 'wpcli_command_end', commonTrackingParams ).catch( () => {} ); + const tracking = trackEvent( 'wpcli_command_end', commonTrackingParams ).catch( () => {} ); currentJob.socket.close(); unpipeStreamsFromProcess( { stdin: currentJob.stdinStream, stdout: currentJob.stdoutStream } ); currentOffset = 0; if ( ! isSubShell ) { subShellRl.close(); - process.exit( exitCode ); + tracking.then( () => process.exit( exitCode ) ); + return; } if ( exitCode ) { @@ -105,6 +113,9 @@ const finishCommand = ( { subShellRl, commonTrackingParams, isSubShell, exitCode }; const bindStreamEvents = ( { subShellRl, commonTrackingParams, isSubShell, stdoutStream } ) => { + const job = currentJob; + const isCurrentStream = () => + job === currentJob && ! job.finished && stdoutStream === job.stdoutStream; const criticalErrors = [ 'ECONNRESET', 'ETIMEDOUT', @@ -116,6 +127,9 @@ const bindStreamEvents = ( { subShellRl, commonTrackingParams, isSubShell, stdou ]; stdoutStream.on( 'error', err => { + if ( ! isCurrentStream() ) { + return; + } commandRunning = false; if ( criticalErrors.includes( err.code ) ) { @@ -128,9 +142,12 @@ const bindStreamEvents = ( { subShellRl, commonTrackingParams, isSubShell, stdou stdoutStream.on( 'end', () => { // Allow the server's exit event to deliver its status after stdout EOF. - if ( ! currentJob.finished ) { - currentJob.exitTimer = setTimeout( () => { - finishCommand( { subShellRl, commonTrackingParams, isSubShell } ); + if ( isCurrentStream() ) { + clearTimeout( job.exitTimer ); + job.exitTimer = setTimeout( () => { + if ( isCurrentStream() ) { + finishCommand( { subShellRl, commonTrackingParams, isSubShell, job } ); + } }, 2000 ); } } ); @@ -168,12 +185,14 @@ const getTokenForCommand = async ( appId, envId, command ) => { */ const onSocketError = err => err; -const launchCommandAndGetStreams = async ( { socket, guid, inputToken, offset = 0 } ) => { +const launchCommandAndGetStreams = ( { socket, guid, inputToken, offset = 0 } ) => { const stdoutStream = IOStream.createStream(); const stdinStream = IOStream.createStream(); stdoutStream.on( 'data', data => { - currentOffset = data.length + currentOffset; + if ( currentJob?.stdoutStream === stdoutStream && ! currentJob.finished ) { + currentOffset += data.length; + } } ); // TODO handle all arguments @@ -190,18 +209,27 @@ const launchCommandAndGetStreams = async ( { socket, guid, inputToken, offset = IOStream( socket ).emit( 'cmd', data, stdinStream, stdoutStream ); - socket.on( 'unauthorized', err => { + const onUnauthorized = err => { + if ( currentJob?.socket !== socket || currentJob.finished ) { + return; + } console.log( 'There was an error with the authentication:', err.message ); - } ); + }; - socket.on( 'cancel', message => { + const onCancel = message => { + if ( currentJob?.socket !== socket || currentJob.finished ) { + return; + } socket.close(); exit.withError( `Cancel received from server: ${ message }` ); - } ); + }; IOStream( socket ).off( 'error', onSocketError ).on( 'error', onSocketError ); - socket.on( 'error', err => { + const onError = err => { + if ( currentJob?.socket !== socket || currentJob.finished ) { + return; + } if ( err === 'Rate limit exceeded' ) { console.log( chalk.red( '\nError:' ), @@ -211,9 +239,18 @@ const launchCommandAndGetStreams = async ( { socket, guid, inputToken, offset = } console.log( err ); - } ); + }; + if ( currentJob?.socket === socket ) { + for ( const [ event, handler ] of Object.entries( currentJob.socketHandlers ) ) { + socket.off( event, handler ); + } + } + const socketHandlers = { unauthorized: onUnauthorized, cancel: onCancel, error: onError }; + for ( const [ event, handler ] of Object.entries( socketHandlers ) ) { + socket.on( event, handler ); + } - return { stdinStream, stdoutStream, socket }; + return { stdinStream, stdoutStream, socket, socketHandlers }; }; const bindReconnectEvents = ( { @@ -223,20 +260,29 @@ const bindReconnectEvents = ( { commonTrackingParams, isSubShell, } ) => { + const job = currentJob; + const isCurrentJob = () => job === currentJob && ! job.finished; currentJob.socket.io.removeAllListeners( 'reconnect' ); currentJob.socket.io.removeAllListeners( 'reconnect_attempt' ); currentJob.socket.removeAllListeners( 'retry' ); currentJob.socket.removeAllListeners( 'connect_error' ); + currentJob.socket.removeAllListeners( 'exit' ); - currentJob.socket.io.on( 'reconnect', async () => { + currentJob.socket.io.on( 'reconnect', () => { + if ( ! isCurrentJob() ) { + return; + } debug( 'socket.io: reconnect' ); + clearTimeout( job.exitTimer ); + clearTimeout( job.retryTimer ); // Close old streams unpipeStreamsFromProcess( { stdin: currentJob.stdinStream, stdout: currentJob.stdoutStream } ); trackEvent( 'wpcli_command_reconnect', commonTrackingParams ).catch( () => {} ); - currentJob = await launchCommandAndGetStreams( { + job.finished = true; + currentJob = launchCommandAndGetStreams( { socket: currentJob.socket, guid: cliCommand.guid, inputToken, @@ -259,15 +305,24 @@ const bindReconnectEvents = ( { safeResume( subShellRl ); } ); - currentJob.socket.on( 'retry', async () => { + currentJob.socket.on( 'retry', () => { + if ( ! isCurrentJob() ) { + return; + } debug( 'socket: retry' ); - setTimeout( () => { - currentJob.socket.io.engine.close(); + clearTimeout( job.retryTimer ); + job.retryTimer = setTimeout( () => { + if ( isCurrentJob() ) { + job.socket.io.engine.close(); + } }, 5000 ); } ); currentJob.socket.on( 'connect_error', () => { + if ( ! isCurrentJob() ) { + return; + } debug( 'socket: connect_error; forcing the preference for websocket' ); // Force the preference for WebSocket in case we see an error during connection @@ -276,6 +331,9 @@ const bindReconnectEvents = ( { } ); currentJob.socket.on( 'exit', ( { exitCode, message } ) => { + if ( ! isCurrentJob() ) { + return; + } debug( 'socket: exit. Code: %d. Message: %s', exitCode, message ); if ( message ) { @@ -288,11 +346,16 @@ const bindReconnectEvents = ( { } ); currentJob.socket.io.on( 'reconnect_attempt', attempt => { + if ( ! isCurrentJob() ) { + return; + } debug( 'There was an error connecting to the server. Retrying...' ); if ( attempt > 1 ) { return; } + clearTimeout( job.exitTimer ); + clearTimeout( job.retryTimer ); // create a new input stream so that we can still catch things like SIGINT while reconnecting if ( currentJob.stdinStream ) { @@ -557,7 +620,7 @@ commandWrapper( { agent: createProxyAgent( API_HOST ), } ); - currentJob = await launchCommandAndGetStreams( { + currentJob = launchCommandAndGetStreams( { socket, guid: cliCommand.guid, inputToken, From 655becf3189ecc048af566caa8257c3951c1b70e Mon Sep 17 00:00:00 2001 From: Volodymyr Kolesnykov Date: Fri, 9 Oct 2026 13:21:54 +0300 Subject: [PATCH 08/13] fix: address SonarCloud issues --- src/bin/vip-wp.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bin/vip-wp.js b/src/bin/vip-wp.js index be2759084..f8561afea 100755 --- a/src/bin/vip-wp.js +++ b/src/bin/vip-wp.js @@ -100,7 +100,7 @@ const finishCommand = ( { currentOffset = 0; if ( ! isSubShell ) { subShellRl.close(); - tracking.then( () => process.exit( exitCode ) ); + void tracking.catch( () => {} ).finally( () => process.exit( exitCode ) ); return; } From 25693ae52bee1c15909a493b7c72936fa82b9487 Mon Sep 17 00:00:00 2001 From: Volodymyr Kolesnykov Date: Fri, 9 Oct 2026 13:30:11 +0300 Subject: [PATCH 09/13] test: await WP command exit deterministically --- __tests__/bin/vip-wp.test.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/__tests__/bin/vip-wp.test.js b/__tests__/bin/vip-wp.test.js index c5aadd3e2..d66bd296b 100644 --- a/__tests__/bin/vip-wp.test.js +++ b/__tests__/bin/vip-wp.test.js @@ -160,6 +160,9 @@ test.each( [ false, true ] )( 'one-shot exit waits for end telemetry (reject=%s)', async shouldReject => { let settle; + const exited = new Promise( resolve => { + process.exit.mockImplementation( code => resolve( code ) ); + } ); mockTrackEvent.mockImplementation( event => event === 'wpcli_command_end' ? new Promise( ( resolve, reject ) => { @@ -172,8 +175,7 @@ test.each( [ false, true ] )( expect( process.exit ).not.toHaveBeenCalled(); expect( mockSocket.close ).toHaveBeenCalledTimes( 1 ); settle(); - await Promise.resolve(); - await Promise.resolve(); + await expect( exited ).resolves.toBe( 3 ); expect( process.exit ).toHaveBeenCalledWith( 3 ); } ); From 1fde3195bae812cf64126c7e357838d9de923434 Mon Sep 17 00:00:00 2001 From: Volodymyr Kolesnykov Date: Fri, 9 Oct 2026 13:45:30 +0300 Subject: [PATCH 10/13] fix: normalize WP input using stdin terminal state --- cmd/vip-next/commands/wp.go | 11 +- internal/parity/wp_shell_differential_test.go | 138 +++++++++++++++++- 2 files changed, 141 insertions(+), 8 deletions(-) diff --git a/cmd/vip-next/commands/wp.go b/cmd/vip-next/commands/wp.go index 5e72d84a5..7903f4ae0 100644 --- a/cmd/vip-next/commands/wp.go +++ b/cmd/vip-next/commands/wp.go @@ -484,10 +484,15 @@ func runWPWebsocketCommand(cmd *cobra.Command, ae *appctx.AppEnv, cmdStr string, inputToken = *t } + // Shell command readers wrap stdin without exposing its file descriptor. + stdinTTY := false + if input, ok := cmd.InOrStdin().(*os.File); ok { + stdinTTY = term.IsTerminal(int(input.Fd())) + } // Determine terminal dimensions (same defaults as SSH branch: 15 rows / 100 cols). - tty := term.IsTerminal(int(os.Stdout.Fd())) + stdoutTTY := term.IsTerminal(int(os.Stdout.Fd())) rows, cols := 15, 100 - if tty { + if stdoutTTY { if w, h, err := term.GetSize(int(os.Stdout.Fd())); err == nil { cols = w rows = h @@ -501,7 +506,7 @@ func runWPWebsocketCommand(cmd *cobra.Command, ae *appctx.AppEnv, cmdStr string, InputToken: inputToken, Columns: cols, Rows: rows, - IsTTY: tty, + IsTTY: stdinTTY, Stdin: stdin, Stdout: out, Stderr: cmd.ErrOrStderr(), diff --git a/internal/parity/wp_shell_differential_test.go b/internal/parity/wp_shell_differential_test.go index 0740ab649..faa55446a 100644 --- a/internal/parity/wp_shell_differential_test.go +++ b/internal/parity/wp_shell_differential_test.go @@ -84,8 +84,11 @@ type shellMock struct { needsInput bool inputOpened chan struct{} remoteBytes chan byte + finishOnInput bool reconnect bool reconnectRuns int + streamRows int + streamColumns int } func (m *shellMock) ServeHTTP(w http.ResponseWriter, r *http.Request) { @@ -207,28 +210,36 @@ func (m *shellMock) serveSocket(w http.ResponseWriter, r *http.Request) { return } } - var pendingAck int + pendingAck := -1 for { messageType, raw, err := ws.Read(ctx) if err != nil { return } - if messageType == websocket.MessageBinary && pendingAck > 0 { + if messageType == websocket.MessageBinary && pendingAck >= 0 { for _, b := range raw { if m.remoteBytes != nil { m.remoteBytes <- b } } ack := fmt.Sprintf(`43/wp-cli,%d[]`, pendingAck) - pendingAck = 0 + pendingAck = -1 if err := ws.Write(ctx, websocket.MessageText, []byte(ack)); err != nil { return } - if strings.ContainsRune(string(raw), 3) { + if strings.ContainsRune(string(raw), 3) || (m.finishOnInput && strings.ContainsAny(string(raw), "\r\n")) { m.mu.Lock() streamID := m.stdoutStreamID m.mu.Unlock() _ = finishShellStream(ctx, ws, streamID, 0) + } else { + m.mu.Lock() + stdinID := m.stdinStreamID + m.mu.Unlock() + read := fmt.Sprintf(`42/wp-cli,["$stream-read",%q,65536]`, stdinID) + if err := ws.Write(ctx, websocket.MessageText, []byte(read)); err != nil { + return + } } continue } @@ -281,7 +292,9 @@ func (m *shellMock) recordStreamID(packet string) string { return "" } var data struct { - GUID string `json:"guid"` + GUID string `json:"guid"` + Rows int `json:"rows"` + Columns int `json:"columns"` } if err := json.Unmarshal(args[2], &data); err != nil { return "" @@ -292,6 +305,7 @@ func (m *shellMock) recordStreamID(packet string) string { m.streamExitCode = 0 m.needsInput = data.GUID == "ws-stdin-guid" m.reconnect = data.GUID == "ws-reconnect-guid" + m.streamRows, m.streamColumns = data.Rows, data.Columns if data.GUID == "ws-fail-guid" { m.streamExitCode = 3 } @@ -836,6 +850,120 @@ func testShellInterruptsAcrossCommands(t *testing.T, rig *differentialRig, scena } } +func TestWPWebsocketShellStdinNormalization(t *testing.T) { + rig, skip := differentialAvailable(t) + if skip != "" { + t.Skip(LoudSkip("TestWPWebsocketShellStdinNormalization", skip)) + } + scenario, err := LoadScenario("../../testdata/parity/wp-websocket-shell.yaml") + if err != nil { + t.Fatal(err) + } + scenario.Env = rig.scenarioEnv(scenario) + for side, bin := range map[string]string{"node": rig.nodeBin, "go": rig.goBin} { + for _, stdinTTY := range []bool{true, false} { + t.Run(fmt.Sprintf("%s/stdin_tty=%t", side, stdinTTY), func(t *testing.T) { + testShellStdinNormalization(t, rig, scenario, bin, stdinTTY) + }) + } + } +} + +func testShellStdinNormalization(t *testing.T, rig *differentialRig, scenario *Scenario, bin string, stdinTTY bool) { + t.Helper() + appBody, err := os.ReadFile("../../testdata/parity/recordings/wp-websocket-shell/resolve-app.json") + if err != nil { + t.Fatal(err) + } + mock := &shellMock{ + appBody: appBody, poll: make(chan string, 2), inputOpened: make(chan struct{}, 1), + remoteBytes: make(chan byte, 16), finishOnInput: true, + } + rig.serve(t, mock) + master, slave, err := pty.Open() + if err != nil { + t.Fatal(err) + } + defer master.Close() + defer slave.Close() + if err := pty.Setsize(master, &pty.Winsize{Rows: 24, Cols: 100}); err != nil { + t.Fatal(err) + } + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, bin, scenario.Argv...) + cmd.Env = FixtureEnv(scenario.Env) + output := newShellOutput() + cmd.Stderr = output + var input io.WriteCloser + if stdinTTY { + cmd.Stdin, cmd.Stdout = slave, output + input = master + } else { + input, err = cmd.StdinPipe() + if err != nil { + t.Fatal(err) + } + defer input.Close() + cmd.Stdout = slave + go func() { _, _ = io.Copy(output, master) }() + } + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + done := make(chan error, 1) + go func() { done <- cmd.Wait() }() + prompt := "parityapp.develop:~$ " + pos := waitForShellOutput(t, output, done, 0, prompt) + if _, err := io.WriteString(input, "wp eval read\n"); err != nil { + t.Fatal(err) + } + select { + case <-mock.inputOpened: + case <-ctx.Done(): + t.Fatalf("remote stdin was not opened: %v\noutput: %q", ctx.Err(), output.String()) + } + if _, err := io.WriteString(input, "input\r"); err != nil { + t.Fatal(err) + } + want := "input\r" + if stdinTTY { + want = "input\n" + } + for i := range len(want) { + select { + case got := <-mock.remoteBytes: + if got != want[i] { + t.Errorf("remote stdin byte %d = %#x, want %#x", i, got, want[i]) + } + case <-ctx.Done(): + t.Fatalf("missing remote stdin byte %d: %v\noutput: %q", i, ctx.Err(), output.String()) + } + } + wantRows := 24 + if stdinTTY { + wantRows = 15 // stdout is redirected, regardless of stdin's terminal size. + } + mock.mu.Lock() + rows, columns := mock.streamRows, mock.streamColumns + mock.mu.Unlock() + if rows != wantRows || columns != 100 { + t.Errorf("remote dimensions = %dx%d, want %dx100", rows, columns, wantRows) + } + _ = waitForShellOutput(t, output, done, pos, prompt) + if _, err := io.WriteString(input, "exit\n"); err != nil { + t.Fatal(err) + } + select { + case err := <-done: + if err != nil { + t.Fatalf("shell exit: %v\noutput: %q", err, output.String()) + } + case <-ctx.Done(): + t.Fatalf("shell did not exit: %v\noutput: %q", ctx.Err(), output.String()) + } +} + func TestWPWebsocketShellReconnectExit(t *testing.T) { rig, skip := differentialAvailable(t) if skip != "" { From 9cae212a09a75a830639b38302a97fd34d92d01c Mon Sep 17 00:00:00 2001 From: Volodymyr Kolesnykov Date: Fri, 9 Oct 2026 14:03:18 +0300 Subject: [PATCH 11/13] test: isolate release promotion HTTP transports --- internal/releasepromotion/github_test.go | 31 ++++++++++--------- .../optional_installers_test.go | 2 +- 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/internal/releasepromotion/github_test.go b/internal/releasepromotion/github_test.go index cca883449..46370f7d6 100644 --- a/internal/releasepromotion/github_test.go +++ b/internal/releasepromotion/github_test.go @@ -59,7 +59,7 @@ func TestEnsureDraftCreatesReleaseAndTagAtCommit(t *testing.T) { })) defer server.Close() - release, err := testGitHubClient(server.URL).EnsureDraft(context.Background(), version, commit) + release, err := testGitHubClient(t, server.URL).EnsureDraft(context.Background(), version, commit) if err != nil { t.Fatal(err) } @@ -85,7 +85,7 @@ func TestEnsureDraftResumesSameCommitDraft(t *testing.T) { } })) defer server.Close() - release, err := testGitHubClient(server.URL).EnsureDraft(context.Background(), "5.0.0-rc.1", commit) + release, err := testGitHubClient(t, server.URL).EnsureDraft(context.Background(), "5.0.0-rc.1", commit) if err != nil { t.Fatal(err) } @@ -128,7 +128,7 @@ func TestEnsureDraftFindsDraftWhenTagEndpointOmitsDraft(t *testing.T) { })) defer server.Close() - release, err := testGitHubClient(server.URL).EnsureDraft(context.Background(), version, commit) + release, err := testGitHubClient(t, server.URL).EnsureDraft(context.Background(), version, commit) if err != nil { t.Fatal(err) } @@ -154,7 +154,7 @@ func TestEnsureDraftRejectsTagAtDifferentCommit(t *testing.T) { }) })) defer server.Close() - _, err := testGitHubClient(server.URL).EnsureDraft(context.Background(), "5.0.0-rc.1", strings.Repeat("b", 40)) + _, err := testGitHubClient(t, server.URL).EnsureDraft(context.Background(), "5.0.0-rc.1", strings.Repeat("b", 40)) if err == nil || !strings.Contains(err.Error(), "different commit") || mutations != 0 { t.Fatalf("error = %v, mutations = %d", err, mutations) } @@ -179,7 +179,7 @@ func TestEnsureDraftRejectsAnnotatedTag(t *testing.T) { })) defer server.Close() - _, err := testGitHubClient(server.URL).EnsureDraft(context.Background(), version, commit) + _, err := testGitHubClient(t, server.URL).EnsureDraft(context.Background(), version, commit) if err == nil || !strings.Contains(err.Error(), "lightweight tag") { t.Fatalf("error = %v, want lightweight tag error", err) } @@ -200,7 +200,7 @@ func TestEnsureDraftRejectsMissingTagForExistingDraft(t *testing.T) { })) defer server.Close() - _, err := testGitHubClient(server.URL).EnsureDraft(context.Background(), version, strings.Repeat("b", 40)) + _, err := testGitHubClient(t, server.URL).EnsureDraft(context.Background(), version, strings.Repeat("b", 40)) if err == nil || !strings.Contains(err.Error(), "has no tag") { t.Fatalf("error = %v, want missing tag error", err) } @@ -212,7 +212,7 @@ func TestEnsureDraftRejectsPublishedRelease(t *testing.T) { writeJSON(t, w, Release{ID: 44, TagName: "5.0.0-rc.1", Draft: false, Prerelease: true}) })) defer server.Close() - _, err := testGitHubClient(server.URL).EnsureDraft(context.Background(), "5.0.0-rc.1", strings.Repeat("b", 40)) + _, err := testGitHubClient(t, server.URL).EnsureDraft(context.Background(), "5.0.0-rc.1", strings.Repeat("b", 40)) if err == nil || !strings.Contains(err.Error(), "published") { t.Fatalf("error = %v", err) } @@ -243,7 +243,7 @@ func TestEnsureDraftResumesExistingMatchingTagWithoutRelease(t *testing.T) { } })) defer server.Close() - release, err := testGitHubClient(server.URL).EnsureDraft(context.Background(), version, commit) + release, err := testGitHubClient(t, server.URL).EnsureDraft(context.Background(), version, commit) if err != nil || release.ID != 13 || strings.Join(mutations, ",") != "create" { t.Fatalf("release = %#v, error = %v, mutations = %v", release, err, mutations) } @@ -258,7 +258,7 @@ func TestReplaceAssetsRejectsUnexpectedExistingAsset(t *testing.T) { })) defer server.Close() release := Release{ID: 1, Draft: true, Assets: []ReleaseAsset{{ID: 9, Name: "notes.txt"}}, UploadURL: server.URL + "/uploads{?name,label}"} - err := testGitHubClient(server.URL).ReplaceAssets(context.Background(), release, t.TempDir()) + err := testGitHubClient(t, server.URL).ReplaceAssets(context.Background(), release, t.TempDir()) if err == nil || !strings.Contains(err.Error(), "unexpected existing asset") || requests != 0 { t.Fatalf("error = %v, requests = %d", err, requests) } @@ -299,7 +299,7 @@ func TestReplaceAssetsDeletesKnownAssetsAndUploadsVerifiedFiles(t *testing.T) { Assets: []ReleaseAsset{{ID: 9, Name: "vip-next-darwin-amd64.tar.gz"}}, UploadURL: server.URL + "/uploads{?name,label}", } - if err := testGitHubClient(server.URL).ReplaceAssets(context.Background(), release, root); err != nil { + if err := testGitHubClient(t, server.URL).ReplaceAssets(context.Background(), release, root); err != nil { t.Fatal(err) } if len(deleted) != 1 || len(uploaded) != len(ExpectedArtifactPaths()) { @@ -318,7 +318,7 @@ func TestPublishPrereleaseIsLastMutation(t *testing.T) { writeJSON(t, w, Release{ID: 88, Draft: false, Prerelease: true}) })) defer server.Close() - if err := testGitHubClient(server.URL).PublishPrerelease(context.Background(), Release{ID: 88, TagName: "5.0.0-rc.1", Draft: true}); err != nil { + if err := testGitHubClient(t, server.URL).PublishPrerelease(context.Background(), Release{ID: 88, TagName: "5.0.0-rc.1", Draft: true}); err != nil { t.Fatal(err) } if body["draft"] != false || body["prerelease"] != true { @@ -337,7 +337,7 @@ func TestPublishPrereleaseIgnoresMalformedSuccessfulResponse(t *testing.T) { })) defer server.Close() - err := testGitHubClient(server.URL).PublishPrerelease(context.Background(), Release{ID: 88, TagName: "5.0.0-rc.1", Draft: true}) + err := testGitHubClient(t, server.URL).PublishPrerelease(context.Background(), Release{ID: 88, TagName: "5.0.0-rc.1", Draft: true}) if err != nil { t.Fatal(err) } @@ -372,8 +372,11 @@ func TestPublishPrereleaseReconcilesAmbiguousTransportError(t *testing.T) { } } -func testGitHubClient(baseURL string) *GitHubClient { - return &GitHubClient{BaseURL: baseURL, UploadsURL: baseURL, Token: "github-token", HTTPClient: http.DefaultClient} +func testGitHubClient(t testing.TB, baseURL string) *GitHubClient { + t.Helper() + client := &http.Client{Transport: http.DefaultTransport.(*http.Transport).Clone()} + t.Cleanup(client.CloseIdleConnections) + return &GitHubClient{BaseURL: baseURL, UploadsURL: baseURL, Token: "github-token", HTTPClient: client} } func assertGitHubRequest(t *testing.T, r *http.Request) { diff --git a/internal/releasepromotion/optional_installers_test.go b/internal/releasepromotion/optional_installers_test.go index c07d024a8..2483620cc 100644 --- a/internal/releasepromotion/optional_installers_test.go +++ b/internal/releasepromotion/optional_installers_test.go @@ -74,7 +74,7 @@ func TestDownloadVerifyAndReplaceWithOptionalInstallers(t *testing.T) { // A retry also removes an old optional installer from the draft when the // current exact build did not produce it. release := Release{ID: 1, Draft: true, UploadURL: server.URL + "/uploads{?name,label}", Assets: []ReleaseAsset{{ID: 9, Name: "vip-next-darwin-amd64.pkg"}}} - if err := testGitHubClient(server.URL).ReplaceAssets(context.Background(), release, downloads); err != nil { + if err := testGitHubClient(t, server.URL).ReplaceAssets(context.Background(), release, downloads); err != nil { t.Fatal(err) } sort.Strings(expected) From 58b81bcc048298f3d597489eb815193f95f09e26 Mon Sep 17 00:00:00 2001 From: Volodymyr Kolesnykov Date: Fri, 9 Oct 2026 14:06:25 +0300 Subject: [PATCH 12/13] fix: preserve WP input and exit status across reconnects --- internal/parity/wp_shell_differential_test.go | 185 +++++++++++++++++- internal/wpstream/engineio.go | 10 +- internal/wpstream/engineio_test.go | 22 +++ internal/wpstream/run.go | 56 +++--- internal/wpstream/run_test.go | 36 ++++ internal/wpstream/stdin.go | 93 +++++++++ internal/wpstream/stdin_test.go | 140 +++++++++++++ 7 files changed, 508 insertions(+), 34 deletions(-) create mode 100644 internal/wpstream/stdin.go create mode 100644 internal/wpstream/stdin_test.go diff --git a/internal/parity/wp_shell_differential_test.go b/internal/parity/wp_shell_differential_test.go index faa55446a..8e450470a 100644 --- a/internal/parity/wp_shell_differential_test.go +++ b/internal/parity/wp_shell_differential_test.go @@ -86,7 +86,11 @@ type shellMock struct { remoteBytes chan byte finishOnInput bool reconnect bool + reconnectInput bool + exitThenClose bool reconnectRuns int + streamRuns int + lastStreamID string streamRows int streamColumns int } @@ -137,6 +141,10 @@ func (m *shellMock) ServeHTTP(w http.ResponseWriter, r *http.Request) { guid = "ws-stdin-guid" } else if request.Variables.Input.Command == "eval reconnect" { guid = "ws-reconnect-guid" + } else if request.Variables.Input.Command == "eval reconnect-read" { + guid = "ws-reconnect-stdin-guid" + } else if request.Variables.Input.Command == `"option" "get" "close"` { + guid = "ws-exit-close-guid" } _, _ = fmt.Fprintf(w, `{"data":{"triggerWPCLICommandOnAppEnvironment":{"inputToken":"tok-ws","command":{"guid":%q},"sshAuthentication":null}}}`, guid) } @@ -201,11 +209,13 @@ func (m *shellMock) serveSocket(w http.ResponseWriter, r *http.Request) { } m.mu.Lock() streamID := m.stdoutStreamID + lastStreamID := m.lastStreamID + reconnectInput := m.reconnectInput exitCode := m.streamExitCode stdinID := m.stdinStreamID needsInput := m.needsInput m.mu.Unlock() - if streamID != "" { + if streamID != "" && (!reconnectInput || streamID != lastStreamID) { if err := m.startShellStream(ctx, ws, streamID, stdinID, needsInput, exitCode); err != nil { return } @@ -305,6 +315,11 @@ func (m *shellMock) recordStreamID(packet string) string { m.streamExitCode = 0 m.needsInput = data.GUID == "ws-stdin-guid" m.reconnect = data.GUID == "ws-reconnect-guid" + m.reconnectInput = data.GUID == "ws-reconnect-stdin-guid" + m.exitThenClose = data.GUID == "ws-exit-close-guid" + if m.reconnectInput { + m.needsInput = true + } m.streamRows, m.streamColumns = data.Rows, data.Columns if data.GUID == "ws-fail-guid" { m.streamExitCode = 3 @@ -316,11 +331,29 @@ func (m *shellMock) recordStreamID(packet string) string { func (m *shellMock) startShellStream(ctx context.Context, ws *websocket.Conn, stdoutID, stdinID string, needsInput bool, exitCode int) error { m.mu.Lock() reconnect := m.reconnect + reconnectInput := m.reconnectInput + exitThenClose := m.exitThenClose + m.streamRuns++ + m.lastStreamID = stdoutID if reconnect { m.reconnectRuns++ } - run := m.reconnectRuns + run, streamRun := m.reconnectRuns, m.streamRuns m.mu.Unlock() + if exitThenClose { + // The handler binds its stdout listener just after emitting the command. + time.Sleep(100 * time.Millisecond) + end := fmt.Sprintf(`42/wp-cli,["$stream-end",%q]`, stdoutID) + exit := `42/wp-cli,["exit",{"exitCode":3}]` + if err := ws.Write(ctx, websocket.MessageText, []byte(end)); err != nil { + return err + } + if err := ws.Write(ctx, websocket.MessageText, []byte(exit)); err != nil { + return err + } + _ = ws.Close(websocket.StatusGoingAway, "") + return errors.New("simulated disconnect immediately after exit") + } if reconnect { if run == 1 { if err := finishShellStream(ctx, ws, stdoutID, 0); err != nil { @@ -351,6 +384,12 @@ func (m *shellMock) startShellStream(ctx context.Context, ws *websocket.Conn, st if m.inputOpened != nil { m.inputOpened <- struct{}{} } + if reconnectInput && streamRun == 1 { + // Allow the first read request to reach the client before dropping it. + time.Sleep(100 * time.Millisecond) + ws.CloseNow() + return errors.New("simulated disconnect after stdin read") + } return nil } @@ -1023,6 +1062,148 @@ func TestWPWebsocketShellReconnectExit(t *testing.T) { } } +func TestWPWebsocketShellStdinAcrossReconnect(t *testing.T) { + rig, skip := differentialAvailable(t) + if skip != "" { + t.Skip(LoudSkip("TestWPWebsocketShellStdinAcrossReconnect", skip)) + } + scenario, err := LoadScenario("../../testdata/parity/wp-websocket-shell.yaml") + if err != nil { + t.Fatal(err) + } + scenario.Env = rig.scenarioEnv(scenario) + appBody, err := os.ReadFile("../../testdata/parity/recordings/wp-websocket-shell/resolve-app.json") + if err != nil { + t.Fatal(err) + } + for side, bin := range map[string]string{"node": rig.nodeBin, "go": rig.goBin} { + t.Run(side, func(t *testing.T) { + mock := &shellMock{ + appBody: appBody, poll: make(chan string, 2), + inputOpened: make(chan struct{}, 2), remoteBytes: make(chan byte, 32), finishOnInput: true, + } + rig.serve(t, mock) + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, bin, scenario.Argv...) + cmd.Env = FixtureEnv(scenario.Env) + stdin, err := cmd.StdinPipe() + if err != nil { + t.Fatal(err) + } + defer stdin.Close() + output := newShellOutput() + cmd.Stdout, cmd.Stderr = output, output + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + done := make(chan error, 1) + go func() { done <- cmd.Wait() }() + prompt := "parityapp.develop:~$ " + pos := waitForShellOutput(t, output, done, 0, prompt) + if _, err := io.WriteString(stdin, "wp eval reconnect-read\n"); err != nil { + t.Fatal(err) + } + for attempt := 1; attempt <= 2; attempt++ { + select { + case <-mock.inputOpened: + case <-ctx.Done(): + t.Fatalf("stdin read did not start on attempt %d: %v\noutput: %q", attempt, ctx.Err(), output.String()) + } + } + const input = "reconnected input\n" + if _, err := io.WriteString(stdin, input); err != nil { + t.Fatal(err) + } + for i := range len(input) { + select { + case got := <-mock.remoteBytes: + if got != input[i] { + t.Errorf("remote stdin byte %d = %#x, want %#x", i, got, input[i]) + } + case <-ctx.Done(): + t.Fatalf("missing remote stdin byte %d: %v\noutput: %q", i, ctx.Err(), output.String()) + } + } + pos = waitForShellOutput(t, output, done, pos, prompt) + if _, err := io.WriteString(stdin, "exit\n"); err != nil { + t.Fatal(err) + } + select { + case err := <-done: + if err != nil { + t.Fatalf("shell exit: %v\noutput: %q", err, output.String()) + } + case <-ctx.Done(): + t.Fatalf("shell did not exit: %v\noutput: %q", ctx.Err(), output.String()) + } + mock.mu.Lock() + streamRuns := mock.streamRuns + mock.mu.Unlock() + if streamRuns != 2 { + t.Errorf("stream attempts = %d, want 2", streamRuns) + } + if got := mock.commandsSeen(); !reflect.DeepEqual(got, []string{"eval reconnect-read"}) { + t.Errorf("command API calls = %q, want one eval reconnect-read", got) + } + select { + case b := <-mock.remoteBytes: + t.Errorf("unexpected extra remote stdin byte %#x", b) + default: + } + }) + } +} + +func TestWPWebsocketOneShotExitBeforeTransportClose(t *testing.T) { + rig, skip := differentialAvailable(t) + if skip != "" { + t.Skip(LoudSkip("TestWPWebsocketOneShotExitBeforeTransportClose", skip)) + } + scenario, err := LoadScenario("../../testdata/parity/wp-websocket-shell.yaml") + if err != nil { + t.Fatal(err) + } + scenario.Env = rig.scenarioEnv(scenario) + appBody, err := os.ReadFile("../../testdata/parity/recordings/wp-websocket-shell/resolve-app.json") + if err != nil { + t.Fatal(err) + } + argv := []string{"@parityapp.develop", "--", "wp", "option", "get", "close"} + for side, bin := range map[string]string{"node": rig.nodeBin, "go": rig.goBin} { + t.Run(side, func(t *testing.T) { + mock := &shellMock{appBody: appBody, poll: make(chan string, 2)} + rig.serve(t, mock) + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, bin, argv...) + cmd.Env = FixtureEnv(scenario.Env) + var stdout, stderr bytes.Buffer + cmd.Stdout, cmd.Stderr = &stdout, &stderr + err := cmd.Run() + if ctx.Err() != nil { + mock.mu.Lock() + streamRuns := mock.streamRuns + mock.mu.Unlock() + t.Fatalf("one-shot command timed out: %v; stream attempts: %d; commands: %q\nstdout: %q\nstderr: %q", ctx.Err(), streamRuns, mock.commandsSeen(), stdout.String(), stderr.String()) + } + var exitErr *exec.ExitError + if !errors.As(err, &exitErr) || exitErr.ExitCode() != 3 { + t.Errorf("exit=%v, want code 3\nstdout: %q\nstderr: %q", err, stdout.String(), stderr.String()) + } + mock.mu.Lock() + streamRuns := mock.streamRuns + mock.mu.Unlock() + if streamRuns != 1 { + t.Errorf("stream attempts = %d, want 1", streamRuns) + } + if got := mock.commandsSeen(); !reflect.DeepEqual(got, []string{`"option" "get" "close"`}) { + t.Errorf("command API calls = %q, want one quoted option get close", got) + } + }) + } +} + func TestWPWebsocketOneShotDelayedNonzeroExit(t *testing.T) { rig, skip := differentialAvailable(t) if skip != "" { diff --git a/internal/wpstream/engineio.go b/internal/wpstream/engineio.go index ec7981121..a13028b69 100644 --- a/internal/wpstream/engineio.go +++ b/internal/wpstream/engineio.go @@ -140,6 +140,7 @@ func Dial(ctx context.Context, opts DialOptions) (*Engine, error) { // I1: ctx is derived from a cancelable context created in Dial; Close() cancels // it, which unblocks e.ws.Read and terminates the goroutine cleanly. func (e *Engine) readLoop(ctx context.Context) { + defer close(e.recvCh) defer close(e.errCh) for { typ, data, err := e.ws.Read(ctx) @@ -247,14 +248,13 @@ func (e *Engine) Read(ctx context.Context) (Packet, error) { select { case pkt, ok := <-e.recvCh: if !ok { + // All received packets have been drained before reporting the error. + if err, ok := <-e.errCh; ok { + return Packet{}, err + } return Packet{}, errClosed } return pkt, nil - case err, ok := <-e.errCh: - if !ok { - return Packet{}, errClosed - } - return Packet{}, err case <-ctx.Done(): return Packet{}, ctx.Err() case <-e.closed: diff --git a/internal/wpstream/engineio_test.go b/internal/wpstream/engineio_test.go index e383c8186..757eccf07 100644 --- a/internal/wpstream/engineio_test.go +++ b/internal/wpstream/engineio_test.go @@ -2,6 +2,7 @@ package wpstream import ( "context" + "io" "net/http" "net/http/httptest" "testing" @@ -10,6 +11,27 @@ import ( "github.com/coder/websocket" ) +func TestEngineReadDrainsPacketsBeforeTransportError(t *testing.T) { + eng := &Engine{recvCh: make(chan Packet, 2), errCh: make(chan error, 1), closed: make(chan struct{})} + eng.recvCh <- Packet{Type: eioMessage, Data: []byte("stdout EOF")} + eng.recvCh <- Packet{Type: eioMessage, Data: []byte("exit 3")} + eng.errCh <- io.EOF + close(eng.errCh) + close(eng.recvCh) + for _, want := range []string{"stdout EOF", "exit 3"} { + pkt, err := eng.Read(context.Background()) + if err != nil || string(pkt.Data) != want { + t.Fatalf("Read = %q, %v; want %q before disconnect", pkt.Data, err, want) + } + } + if _, err := eng.Read(context.Background()); err != io.EOF { + t.Fatalf("terminal read = %v, want EOF", err) + } + if _, err := eng.Read(context.Background()); err != errClosed { + t.Fatalf("subsequent read = %v, want closed", err) + } +} + // fakeEIOServer serves the EIO4 polling handshake then accepts a websocket // upgrade, completes the 2probe/5 dance, and sends one message packet. func fakeEIOServer(t *testing.T) *httptest.Server { diff --git a/internal/wpstream/run.go b/internal/wpstream/run.go index 85dbbf216..30872f598 100644 --- a/internal/wpstream/run.go +++ b/internal/wpstream/run.go @@ -52,11 +52,12 @@ type Result struct { // arrives) it re-dials with exponential backoff and resumes from offset. // // C1/C2 fix: the loop is entirely self-contained; every Engine is explicitly -// closed before the next attempt or before returning. No goroutine is launched -// that outlives its engine. +// closed before the next attempt or before returning. The stdin pump spans +// attempts so a disconnected stream cannot consume the next attempt's input. func Run(ctx context.Context, opts Options) (Result, error) { ctx, cancel := context.WithCancel(ctx) defer cancel() + input := newStdinPump(ctx, opts.Stdin, opts.IsTTY) var offset atomic.Int64 backoff := time.Second @@ -111,7 +112,7 @@ func Run(ctx context.Context, opts Options) (Result, error) { debuglog.Printf(ctx, "@automattic/vip:wp", "socket: connected") first = false - res, clean := runOnce(ctx, opts, cli, ss, &offset, offset.Load()) + res, clean := runOnce(ctx, opts, cli, ss, &offset, offset.Load(), input) eng.Close() // C2: ALWAYS close engine before next attempt or return if ctx.Err() != nil { @@ -149,7 +150,7 @@ func waitBackoff(ctx context.Context, backoff *time.Duration, max time.Duration) // Returns (result, true) on clean exit, or (Result{}, false) on disconnect // so the caller can reconnect. ctx cancel is treated as clean=true and the // caller checks ctx.Err() afterwards. -func runOnce(ctx context.Context, opts Options, cli *Client, ss *StreamSocket, offset *atomic.Int64, resumeAt int64) (Result, bool) { +func runOnce(ctx context.Context, opts Options, cli *Client, ss *StreamSocket, offset *atomic.Int64, resumeAt int64, input *stdinPump) (Result, bool) { // exitCh is buffered: handlers run in goroutines and MUST NOT block on send. exitCh := make(chan int, 4) signalExit := func(code int) { @@ -218,19 +219,8 @@ func runOnce(ctx context.Context, opts Options, cli *Client, ss *StreamSocket, o } _ = ss.Emit(ctx, "cmd", []any{data, stdinStream, stdoutStream}, nil) - // Pipe stdin → stdinStream. - // When stdinStream is aborted (C3) Write returns an error, io.Copy stops, - // and the goroutine exits — no leak. - go func() { - src := opts.Stdin - if opts.IsTTY && src != nil { - src = crToLF{src} - } - if src != nil { - _, _ = io.Copy(stdinStream, src) - } - _ = stdinStream.Close() - }() + input.attach(stdinStream) + defer input.detach(stdinStream) // Background watcher: inject errRunDone into stdoutStream when runOnce is // about to return, unblocking the stdout goroutine. @@ -261,9 +251,7 @@ func runOnce(ctx context.Context, opts Options, cli *Client, ss *StreamSocket, o defer closeRunDone() - var exitCode int - select { - case exitCode = <-exitCh: + finish := func(exitCode int) (Result, bool) { // Drain stdout: signal runDone so the watcher goroutine aborts the // stream, unblocking the stdout goroutine. closeRunDone() @@ -274,6 +262,22 @@ func runOnce(ctx context.Context, opts Options, cli *Client, ss *StreamSocket, o // aborted. Cancellation must not wait for that writer. } return Result{ExitCode: exitCode}, true + } + disconnect := func() (Result, bool) { + // A delivered exit wins even when select also observes a disconnect. + select { + case exitCode := <-exitCh: + return finish(exitCode) + default: + } + ss.abortAll(io.ErrUnexpectedEOF) + closeRunDone() + return Result{}, false + } + var exitCode int + select { + case exitCode = <-exitCh: + return finish(exitCode) case <-stdoutDone: // stdout EOF before any exit event — the server always sends an 'exit' // event after streaming completes (vip-wp.js:261). Wait briefly for it @@ -281,9 +285,7 @@ func runOnce(ctx context.Context, opts Options, cli *Client, ss *StreamSocket, o select { case exitCode = <-exitCh: case <-disconnected: - ss.abortAll(io.ErrUnexpectedEOF) - closeRunDone() - return Result{}, false + return disconnect() case <-time.After(2 * time.Second): case <-ctx.Done(): } @@ -294,9 +296,7 @@ func runOnce(ctx context.Context, opts Options, cli *Client, ss *StreamSocket, o // We do this here (not in NewStreamSocket) to ensure the disconnected // channel is selected BEFORE stdoutDone can fire — preventing the race // where stdoutDone fires first and runOnce returns clean=true incorrectly. - ss.abortAll(io.ErrUnexpectedEOF) - closeRunDone() // also aborts stdoutStream via watcher (redundant but safe) - return Result{}, false + return disconnect() case <-ctx.Done(): return Result{}, true // caller checks ctx.Err() } @@ -305,7 +305,9 @@ func runOnce(ctx context.Context, opts Options, cli *Client, ss *StreamSocket, o // run is the internal single-attempt function used by the unit tests (run_test.go). // The public API uses runOnce via Run. Kept for backward compatibility with tests. func run(ctx context.Context, opts Options, cli *Client, ss *StreamSocket, offset *atomic.Int64, resumeAt int64) (Result, error) { - res, clean := runOnce(ctx, opts, cli, ss, offset, resumeAt) + ctx, cancel := context.WithCancel(ctx) + defer cancel() + res, clean := runOnce(ctx, opts, cli, ss, offset, resumeAt, newStdinPump(ctx, opts.Stdin, opts.IsTTY)) if !clean { // disconnect treated as context cancellation for unit-test callers return Result{}, ctx.Err() diff --git a/internal/wpstream/run_test.go b/internal/wpstream/run_test.go index ea67943b3..7e8ed45e4 100644 --- a/internal/wpstream/run_test.go +++ b/internal/wpstream/run_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "errors" + "io" "strings" "sync" "sync/atomic" @@ -11,6 +12,41 @@ import ( "time" ) +type exitDisconnectTransport struct { + transport + queue func() +} + +func (t *exitDisconnectTransport) WriteMessage(_ context.Context, p []byte) error { + if strings.Contains(string(p), `"cmd"`) { + t.queue() + } + return nil +} + +func TestRunQueuedExitWinsOverDisconnect(t *testing.T) { + for _, eof := range []bool{false, true} { + for range 100 { + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + tr := &exitDisconnectTransport{} + cli := NewClient(tr, "/wp-cli") + ss := NewStreamSocket(ctx, cli) + tr.queue = func() { + if eof { + ss.abortAll(io.EOF) + } + cli.dispatch("exit", []any{map[string]any{"exitCode": float64(3)}}) + cli.dispatch("disconnect", nil) + } + res, clean := runOnce(ctx, Options{Stdout: io.Discard}, cli, ss, nil, 0, newStdinPump(ctx, nil, false)) + cancel() + if !clean || res.ExitCode != 3 { + t.Fatalf("EOF=%v: queued exit lost: result=%+v clean=%v", eof, res, clean) + } + } + } +} + type blockingWriter struct { entered chan struct{} release chan struct{} diff --git a/internal/wpstream/stdin.go b/internal/wpstream/stdin.go new file mode 100644 index 000000000..0d4763aa1 --- /dev/null +++ b/internal/wpstream/stdin.go @@ -0,0 +1,93 @@ +package wpstream + +import ( + "context" + "io" + "sync" +) + +type stdinDestination interface { + io.WriteCloser + abort(error) +} + +// stdinPump keeps a single source reader across attempts. Pending writes move +// to the next stream instead of letting an abandoned copier consume input. +type stdinPump struct { + ctx context.Context + src io.Reader + start sync.Once + mu sync.Mutex + stream stdinDestination + changed chan struct{} + ended bool +} + +func newStdinPump(ctx context.Context, src io.Reader, tty bool) *stdinPump { + if tty && src != nil { + src = crToLF{src} + } + return &stdinPump{ctx: ctx, src: src, changed: make(chan struct{}), ended: src == nil} +} + +func (p *stdinPump) attach(stream stdinDestination) { + p.mu.Lock() + p.stream = stream + ended := p.ended + close(p.changed) + p.changed = make(chan struct{}) + p.mu.Unlock() + if ended { + _ = stream.Close() + return + } + p.start.Do(func() { + go func() { + _, _ = io.Copy(p, p.src) + p.mu.Lock() + p.ended = true + stream := p.stream + p.mu.Unlock() + if stream != nil { + _ = stream.Close() + } + }() + }) +} + +func (p *stdinPump) detach(stream stdinDestination) { + p.mu.Lock() + if p.stream == stream { + p.stream = nil + close(p.changed) + p.changed = make(chan struct{}) + } + p.mu.Unlock() + stream.abort(errRunDone) +} + +func (p *stdinPump) Write(data []byte) (int, error) { + written := 0 + for written < len(data) { + if err := p.ctx.Err(); err != nil { + return written, err + } + p.mu.Lock() + stream, changed := p.stream, p.changed + p.mu.Unlock() + if stream == nil { + select { + case <-changed: + continue + case <-p.ctx.Done(): + return written, p.ctx.Err() + } + } + n, err := stream.Write(data[written:]) + written += n + if err != nil { + p.detach(stream) + } + } + return written, nil +} diff --git a/internal/wpstream/stdin_test.go b/internal/wpstream/stdin_test.go new file mode 100644 index 000000000..eaeb246d4 --- /dev/null +++ b/internal/wpstream/stdin_test.go @@ -0,0 +1,140 @@ +package wpstream + +import ( + "context" + "io" + "strings" + "sync" + "sync/atomic" + "testing" + "time" +) + +type pumpDestination struct { + data chan string + closed chan struct{} + aborted chan struct{} + entered chan struct{} + closeOnce sync.Once + abortOnce sync.Once +} + +func newPumpDestination() *pumpDestination { + return &pumpDestination{data: make(chan string, 1), closed: make(chan struct{}), aborted: make(chan struct{})} +} + +func (d *pumpDestination) Write(p []byte) (int, error) { + if d.entered != nil { + close(d.entered) + <-d.aborted + return 0, errRunDone + } + d.data <- string(p) + return len(p), nil +} + +func (d *pumpDestination) Close() error { + d.closeOnce.Do(func() { close(d.closed) }) + return nil +} + +func (d *pumpDestination) abort(error) { + d.abortOnce.Do(func() { close(d.aborted) }) +} + +type countedReader struct { + io.Reader + reads atomic.Int32 + entered chan struct{} + once sync.Once +} + +func (r *countedReader) Read(p []byte) (int, error) { + r.reads.Add(1) + r.once.Do(func() { close(r.entered) }) + return r.Reader.Read(p) +} + +func awaitPump(t *testing.T, ch <-chan struct{}) { + t.Helper() + select { + case <-ch: + case <-time.After(2 * time.Second): + t.Fatal("stdin pump timed out") + } +} + +func TestStdinPumpReconnectWhileReadBlocked(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + r, w := io.Pipe() + defer r.Close() + defer w.Close() + src := &countedReader{Reader: r, entered: make(chan struct{})} + pump := newStdinPump(ctx, src, true) + first, second := newPumpDestination(), newPumpDestination() + pump.attach(first) + awaitPump(t, src.entered) + pump.detach(first) + pump.attach(second) + if got := src.reads.Load(); got != 1 { + t.Fatalf("concurrent input reads: got %d, want 1", got) + } + if _, err := io.WriteString(w, "input\r"); err != nil { + t.Fatal(err) + } + select { + case got := <-second.data: + if got != "input\n" { + t.Fatalf("input after reconnect = %q", got) + } + case <-time.After(2 * time.Second): + t.Fatal("reconnected stream did not receive input") + } + select { + case got := <-first.data: + t.Fatalf("abandoned stream consumed input: %q", got) + default: + } +} + +func TestStdinPumpRetriesPendingWriteAndPreservesEOF(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + pump := newStdinPump(ctx, strings.NewReader("pending"), false) + first, second := newPumpDestination(), newPumpDestination() + first.entered = make(chan struct{}) + pump.attach(first) + awaitPump(t, first.entered) + pump.detach(first) + pump.attach(second) + select { + case got := <-second.data: + if got != "pending" { + t.Fatalf("pending input = %q", got) + } + case <-time.After(2 * time.Second): + t.Fatal("pending input lost on reconnect") + } + awaitPump(t, second.closed) + pump.detach(second) + third := newPumpDestination() + pump.attach(third) + awaitPump(t, third.closed) +} + +func TestStdinPumpCancellationBetweenAttempts(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + pump := newStdinPump(ctx, nil, false) + done := make(chan error, 1) + go func() { _, err := pump.Write([]byte("pending")); done <- err }() + cancel() + select { + case err := <-done: + if err != context.Canceled { + t.Fatalf("write error = %v", err) + } + case <-time.After(2 * time.Second): + t.Fatal("cancelled pump did not release writer") + } +} From e8e6c0cc90dc5090ee43bbd7fbce5c1e86aa8234 Mon Sep 17 00:00:00 2001 From: Volodymyr Kolesnykov Date: Fri, 9 Oct 2026 14:24:21 +0300 Subject: [PATCH 13/13] fix: align WP shell whitespace and multiline parsing --- __tests__/bin/vip-wp.test.js | 34 ++++++++++++++++--- internal/parity/wp_shell_differential_test.go | 8 ++--- internal/wpshell/repl.go | 11 +++--- internal/wpshell/repl_test.go | 33 ++++++++++++++++++ src/bin/vip-wp.js | 4 +-- testdata/parity/wp-websocket-shell.yaml | 3 +- 6 files changed, 77 insertions(+), 16 deletions(-) diff --git a/__tests__/bin/vip-wp.test.js b/__tests__/bin/vip-wp.test.js index d66bd296b..8d174fae2 100644 --- a/__tests__/bin/vip-wp.test.js +++ b/__tests__/bin/vip-wp.test.js @@ -7,6 +7,7 @@ let mockSocket; let mockStreamSocket; let mockStreams; const mockTrackEvent = jest.fn(); +const mockMutate = jest.fn(); jest.mock( '../../src/lib/cli/command', () => ( { __esModule: true, @@ -27,11 +28,7 @@ jest.mock( '../../src/lib/api', () => ( { API_HOST: 'https://test.invalid', disableGlobalGraphQLErrorHandling: jest.fn(), default: () => ( { - mutate: async () => ( { - data: { - triggerWPCLICommandOnAppEnvironment: { command: { guid: 'test' }, inputToken: 'test' }, - }, - } ), + mutate: ( ...args ) => mockMutate( ...args ), } ), } ) ); jest.mock( '../../src/commands/wp-ssh', () => ( { WPCliCommandOverSSH: jest.fn() } ) ); @@ -77,6 +74,11 @@ beforeEach( () => { jest.useFakeTimers(); mockStreams = []; mockTrackEvent.mockReset().mockResolvedValue( undefined ); + mockMutate.mockReset().mockResolvedValue( { + data: { + triggerWPCLICommandOnAppEnvironment: { command: { guid: 'test' }, inputToken: 'test' }, + }, + } ); mockReadline = new EventEmitter(); for ( const method of [ 'pause', 'resume', 'prompt', 'clearLine', 'close', 'write' ] ) { mockReadline[ method ] = jest.fn(); @@ -107,6 +109,28 @@ async function startCommand( isSubShell = true ) { await mockReadline.listeners( 'line' )[ 0 ]( 'wp option get home' ); } +test.each( [ ' ', '\t ', '\u00a0', '\ufeff', '\u2003' ] )( + 'shell strips leading whitespace before wp (%j)', + async prefix => { + await mockHandler( [], options ); + await mockReadline.listeners( 'line' )[ 0 ]( `${ prefix }wp \t\u00a0option get home` ); + expect( mockMutate.mock.calls[ 0 ][ 0 ].variables.input.command ).toBe( 'option get home' ); + mockSocket.emit( 'exit', { exitCode: 0 } ); + } +); + +test( 'shell preserves blank lines and indentation inside multiline quotes', async () => { + await mockHandler( [], options ); + const onLine = mockReadline.listeners( 'line' )[ 0 ]; + await onLine( ' wp option set key "first' ); + await onLine( '' ); + await onLine( ' second"' ); + expect( mockMutate.mock.calls[ 0 ][ 0 ].variables.input.command ).toBe( + 'option set key "first\n\n second"' + ); + mockSocket.emit( 'exit', { exitCode: 0 } ); +} ); + test( 'stale stream completion and retry timers cannot finish a reconnected job', async () => { await startCommand(); const oldStdout = mockStreams[ 0 ]; diff --git a/internal/parity/wp_shell_differential_test.go b/internal/parity/wp_shell_differential_test.go index 8e450470a..f683d48eb 100644 --- a/internal/parity/wp_shell_differential_test.go +++ b/internal/parity/wp_shell_differential_test.go @@ -463,7 +463,7 @@ func runShellSide(t *testing.T, rig *differentialRig, scenario *Scenario, bin st if got := mock.commandsSeen(); len(got) != 1 || got[0] != "option get home" { t.Errorf("trigger commands = %q, want [option get home]", got) } - if _, err := io.WriteString(stdin, "wp site list\n"); err != nil { + if _, err := io.WriteString(stdin, " wp \t site list\n"); err != nil { t.Fatal(err) } pos = waitForShellOutput(t, stdout, done, pos, prompt) @@ -478,7 +478,7 @@ func runShellSide(t *testing.T, rig *differentialRig, scenario *Scenario, bin st if got := mock.commandsSeen(); len(got) != 3 || got[2] != "option get missing" { t.Errorf("trigger commands after failed socket command = %q", got) } - if _, err := io.WriteString(stdin, "wp post list\n"); err != nil { + if _, err := io.WriteString(stdin, "\u00a0wp \u00a0post list\n"); err != nil { t.Fatal(err) } pos = waitForShellOutput(t, stdout, done, pos, prompt) @@ -658,14 +658,14 @@ func TestWPWebsocketShellMultilinePTY(t *testing.T) { go func() { done <- cmd.Wait() }() prompt := "parityapp.develop:~$ " pos := waitForShellOutput(t, output, done, 0, prompt) - if _, err := io.WriteString(terminal, "wp option set key \"line1\nline2\"\n"); err != nil { + if _, err := io.WriteString(terminal, " wp option set key \"line1\n\n line2\"\n"); err != nil { t.Fatal(err) } _ = waitForShellOutput(t, output, done, pos, prompt) if got := strings.Count(output.String(), prompt); got != 2 { t.Errorf("multiline command showed %d prompts, want 2: %q", got, output.String()) } - if got := mock.commandsSeen(); !reflect.DeepEqual(got, []string{"option set key \"line1\nline2\""}) { + if got := mock.commandsSeen(); !reflect.DeepEqual(got, []string{"option set key \"line1\n\n line2\""}) { t.Errorf("multiline commands = %q", got) } if _, err := io.WriteString(terminal, "exit\n"); err != nil { diff --git a/internal/wpshell/repl.go b/internal/wpshell/repl.go index faa76b13f..72575fd74 100644 --- a/internal/wpshell/repl.go +++ b/internal/wpshell/repl.go @@ -7,6 +7,9 @@ import ( "strings" ) +// Match JavaScript trimStart() and /\s/, including BOM but excluding NEL. +const commandWhitespace = "\t\n\v\f\r \u00a0\u1680\u2000\u2001\u2002\u2003\u2004\u2005\u2006\u2007\u2008\u2009\u200a\u2028\u2029\u202f\u205f\u3000\ufeff" + // REPL drives the interactive WP-CLI subshell. Run is invoked with each // finalized command (leading "wp " stripped, matching vip-wp.js:493). // Serve returns when input reaches EOF or the user types `exit`. @@ -65,7 +68,7 @@ const ( func (r *REPL) handleLine(out io.Writer, state *CmdState, seenWP *bool, line string) lineResult { // Blank line re-prompts (vip-wp.js:451). - if line == "" { + if line == "" && !*seenWP { r.prompt(out) return continueREPL } @@ -73,7 +76,7 @@ func (r *REPL) handleLine(out io.Writer, state *CmdState, seenWP *bool, line str if !*seenWP && strings.HasPrefix(line, "exit") { return exitREPL } - if !*seenWP && strings.HasPrefix(strings.TrimLeft(line, " \t"), "wp ") { + if !*seenWP && strings.HasPrefix(strings.TrimLeft(line, commandWhitespace), "wp ") { *seenWP = true ResetState(state) } @@ -89,8 +92,8 @@ func (r *REPL) handleLine(out io.Writer, state *CmdState, seenWP *bool, line str return continueREPL // keep accumulating (multiline quote) } - cmd := strings.TrimLeft(state.Command, " \t") - cmd = strings.TrimLeft(strings.TrimPrefix(cmd, "wp"), " \t") + cmd := strings.TrimLeft(state.Command, commandWhitespace) + cmd = strings.TrimLeft(strings.TrimPrefix(cmd, "wp"), commandWhitespace) *seenWP = false ResetState(state) _ = r.Run(cmd) diff --git a/internal/wpshell/repl_test.go b/internal/wpshell/repl_test.go index e875acc4f..878596955 100644 --- a/internal/wpshell/repl_test.go +++ b/internal/wpshell/repl_test.go @@ -23,6 +23,39 @@ func TestREPLRunsValidCommand(t *testing.T) { } } +func TestREPLCommandWhitespace(t *testing.T) { + for _, prefix := range []string{" ", "\t ", "\u00a0", "\ufeff", "\u2003"} { + t.Run(prefix, func(t *testing.T) { + var ran []string + loop := &REPL{Run: func(cmd string) error { ran = append(ran, cmd); return nil }} + in := strings.NewReader(prefix + "wp \t\u00a0option get home\nexit\n") + var out strings.Builder + if err := loop.Serve(bufio.NewReader(in), &out); err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(ran, []string{"option get home"}) { + t.Fatalf("commands = %q", ran) + } + }) + } +} + +func TestREPLMultilineBlankLine(t *testing.T) { + var ran []string + loop := &REPL{Prompt: "P$ ", Run: func(cmd string) error { ran = append(ran, cmd); return nil }} + in := strings.NewReader(" wp option set key \"first\n\n second\"\nexit\n") + var out strings.Builder + if err := loop.Serve(bufio.NewReader(in), &out); err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(ran, []string{"option set key \"first\n\n second\""}) { + t.Fatalf("commands = %q", ran) + } + if strings.Count(out.String(), "P$ ") != 2 { + t.Fatalf("unexpected continuation prompt: %q", out.String()) + } +} + func TestREPLTerminalContinuation(t *testing.T) { lines := []string{"wp option set k \"line1", "line2\"", "invalid", "exit"} var continuations []bool diff --git a/src/bin/vip-wp.js b/src/bin/vip-wp.js index f8561afea..f31a0926c 100755 --- a/src/bin/vip-wp.js +++ b/src/bin/vip-wp.js @@ -528,7 +528,7 @@ commandWrapper( { } // Handle plain return / newline - if ( ! line ) { + if ( ! line && ! seenWP ) { safePrompt( subShellRl ); return; } @@ -571,7 +571,7 @@ commandWrapper( { countSIGINT = 0; let result; - const wpCliCmd = commandState.command.replace( /^wp\s+/, '' ); + const wpCliCmd = commandState.command.trimStart().replace( /^wp\s+/, '' ); seenWP = false; resetState( commandState ); diff --git a/testdata/parity/wp-websocket-shell.yaml b/testdata/parity/wp-websocket-shell.yaml index 9261fcf1d..c780c19c5 100644 --- a/testdata/parity/wp-websocket-shell.yaml +++ b/testdata/parity/wp-websocket-shell.yaml @@ -2,7 +2,8 @@ name: wp-websocket-shell description: | A no-argument WebSocket wp command opens a shell without triggering WP-CLI. Invalid input, a GraphQL error, two completed socket commands, and a delayed - nonzero socket exit each return to the prompt. Exit closes the shell. + nonzero socket exit each return to the prompt. Leading ASCII and Unicode + whitespace is stripped before dispatch. Exit closes the shell. argv: ["@parityapp.develop", "wp"] env: DO_NOT_TRACK: "1"