From 471df1c1f73435ce30f364fcfb2ef5b2aabcd2ea Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Tue, 30 Jun 2026 16:02:09 -0700 Subject: [PATCH 1/5] core/bazel: fix race There's a race in `streamOutput` and `streamAndParseTargets` where the stderr buffer was being read / written concurrently. When the context gets cancelled and bails, it's trying to read the buffer while it's still being written to by the other goroutine. The streaming goroutine should block until the other goroutine exits, so that it doesn't attempt to read the byte buffer concurrently. Fixes #134. --- core/bazel/stream.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/core/bazel/stream.go b/core/bazel/stream.go index 2e78fd7b..959717c6 100644 --- a/core/bazel/stream.go +++ b/core/bazel/stream.go @@ -32,6 +32,7 @@ func streamOutput(ctx context.Context, src io.Reader, dst io.Writer) error { select { case <-ctx.Done(): + <-done return ctx.Err() case err := <-done: return err @@ -52,6 +53,7 @@ func streamAndParseTargets(ctx context.Context, src io.Reader, dst io.Writer) (* select { case <-ctx.Done(): + <-done return nil, ctx.Err() case res := <-done: return res.queryResult, res.err From 03c53ec5e698d17b372b6de82944c53ad35f00cf Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Tue, 7 Jul 2026 23:01:09 -0700 Subject: [PATCH 2/5] second thought - address code review comments differently. --- core/bazel/bazel.go | 9 +++ core/bazel/query.go | 42 ++++++++-- core/bazel/query_test.go | 169 +++++++++++++++++++++++++++++++++++++++ core/bazel/stream.go | 48 ++++------- core/execcmd/execcmd.go | 13 +-- 5 files changed, 236 insertions(+), 45 deletions(-) diff --git a/core/bazel/bazel.go b/core/bazel/bazel.go index 4b138860..52cf2691 100644 --- a/core/bazel/bazel.go +++ b/core/bazel/bazel.go @@ -32,6 +32,13 @@ import ( const ( // default query timeout if not provided in config _queryTimeout = 15 * time.Minute + // _pipeUnblockDelay is how long after the command context ends a query + // waits before force-closing the stdout/stderr pipes to unblock stream + // reads that never saw EOF (a descendant of the killed process can inherit + // the pipe write ends and hold them open, see go.dev/issue/23019). It + // exceeds execcmd.GracePeriod so the SIGTERM→SIGKILL sequence gets a + // chance to end the streams with a natural EOF first. + _pipeUnblockDelay = execcmd.GracePeriod + 5*time.Second ) type QueryRequest struct { @@ -56,6 +63,7 @@ type BazelClient struct { logger *zap.SugaredLogger execCommandContext func(ctx context.Context, name string, arg ...string) commander queryTimeout time.Duration + pipeUnblockDelay time.Duration streamLogs bool } @@ -98,6 +106,7 @@ func NewBazelClient(ctx context.Context, p Params) (*BazelClient, error) { logger: p.Logger, execCommandContext: execCmd, queryTimeout: timeout, + pipeUnblockDelay: _pipeUnblockDelay, streamLogs: p.StreamLogs, }, nil } diff --git a/core/bazel/query.go b/core/bazel/query.go index 37f10a73..2d5e97a3 100644 --- a/core/bazel/query.go +++ b/core/bazel/query.go @@ -22,6 +22,7 @@ import ( "io" "os" "strings" + "time" buildpb "github.com/bazelbuild/buildtools/build_proto" "go.uber.org/zap" @@ -74,8 +75,6 @@ func (b *BazelClient) executeQueryInternal(ctx context.Context, query string, st if b.streamLogs { stderrSink = os.Stderr } - // orchestrate `allOfFailFast` - // create a `g` group and a new `gCtx` derived from our 15 minute timeout `ctx`. g, gCtx := errgroup.WithContext(cmdCtx) // Start the process @@ -84,16 +83,47 @@ func (b *BazelClient) executeQueryInternal(ctx context.Context, query string, st } // stream and parse targets g.Go(func() error { - var err error - queryResults, err = streamAndParseTargets(gCtx, stdout, &stdoutBuf) - return err + res, err := getQueryResult(gCtx, stdout, &stdoutBuf) + if err != nil { + return err + } + queryResults = res + return nil }) // stream stderr g.Go(func() error { return streamOutput(gCtx, stderr, stderrSink) }) - waitErr := cmd.Wait() + + // The stream reads normally end with EOF when the process dies and the + // kernel closes the pipe write ends. But a descendant of a killed process + // that inherited the pipes can hold the write ends open indefinitely + // (go.dev/issue/23019). cmd.Wait would force-close the parent ends, but we + // only call Wait after the reads finish, so this watchdog is what + // guarantees the reads can't block forever. + readsDone := make(chan struct{}) + go func() { + select { + case <-readsDone: + return + case <-cmdCtx.Done(): + } + timer := time.NewTimer(b.pipeUnblockDelay) + defer timer.Stop() + select { + case <-readsDone: + case <-timer.C: + stdout.Close() + stderr.Close() + } + }() + + // Drain the streams before reaping the process: cmd.Wait closes the parent + // pipe ends as soon as the process exits, which would truncate whatever + // the readers hadn't consumed yet and race on the output buffers. streamErr := g.Wait() + close(readsDone) + waitErr := cmd.Wait() if waitErr != nil { return queryResults, b.wrapQueryFailure("bazel query failed", waitErr, &stderrBuf) } diff --git a/core/bazel/query_test.go b/core/bazel/query_test.go index c506214f..a35737ad 100644 --- a/core/bazel/query_test.go +++ b/core/bazel/query_test.go @@ -18,6 +18,7 @@ import ( "bytes" "context" "errors" + "fmt" "io" "strings" "testing" @@ -175,6 +176,174 @@ func TestExecuteQueryInternal_ContextTimeout(t *testing.T) { assert.Contains(t, err.Error(), "deadline exceeded") } +// TestExecuteQueryInternal_PipesHeldOpenAfterCancel simulates a descendant of +// the killed bazel process inheriting the stdout/stderr pipes and holding +// their write ends open past the kill (go.dev/issue/23019). Since cmd.Wait — +// which would force-close the parent ends — only runs after the stream reads +// finish, the pipe watchdog must unblock the reads or the query would hang. +func TestExecuteQueryInternal_PipesHeldOpenAfterCancel(t *testing.T) { + defer goleak.VerifyNone(t) + ctrl := gomock.NewController(t) + mockCmd := commandermock.NewMockcommander(ctrl) + + // The write ends are intentionally never closed by the "process". + prStdout, pwStdout := io.Pipe() + prStderr, pwStderr := io.Pipe() + defer pwStdout.Close() + defer pwStderr.Close() + + var cmdCtx context.Context + gomock.InOrder( + mockCmd.EXPECT().StdoutPipe().Return(prStdout, nil), + mockCmd.EXPECT().StderrPipe().Return(prStderr, nil), + mockCmd.EXPECT().Start().Return(nil), + mockCmd.EXPECT().Wait().DoAndReturn(func() error { + // The process itself dies with the context; only its orphaned + // descendant lives on, holding the pipes. + <-cmdCtx.Done() + return context.DeadlineExceeded + }), + ) + + client, err := NewBazelClient(context.Background(), Params{ + BazelCommand: "bazel", + WorkspacePath: "/tmp/test", + EnvVarsMap: map[string]string{}, + Logger: zap.NewNop().Sugar(), + QueryTimeout: 10 * time.Millisecond, + ExecCommandContext: func(ctx context.Context, name string, arg ...string) commander { + cmdCtx = ctx + return mockCmd + }, + }) + require.NoError(t, err) + client.pipeUnblockDelay = 20 * time.Millisecond + + type queryOutcome struct { + result *buildpb.QueryResult + err error + } + done := make(chan queryOutcome, 1) + go func() { + result, err := client.executeQueryInternal(context.Background(), "//...", nil) + done <- queryOutcome{result: result, err: err} + }() + + select { + case out := <-done: + require.Error(t, out.err) + assert.ErrorIs(t, out.err, context.DeadlineExceeded) + assert.Nil(t, out.result) + case <-time.After(10 * time.Second): + t.Fatal("executeQueryInternal never returned: pipe reads were not unblocked") + } +} + +// TestExecuteQueryInternal_DrainsStreamsBeforeWait verifies that cmd.Wait runs +// only after the stream reads finish. The mock Waits emulate exec.Cmd.Wait, +// which force-closes the parent pipe ends when it returns — had Wait run +// before the reads completed, the still-streaming output would be truncated. +func TestExecuteQueryInternal_DrainsStreamsBeforeWait(t *testing.T) { + t.Run("stdout is fully parsed", func(t *testing.T) { + defer goleak.VerifyNone(t) + ctrl := gomock.NewController(t) + mockCmd := commandermock.NewMockcommander(ctrl) + + prStdout, pwStdout := io.Pipe() + prStderr, pwStderr := io.Pipe() + + // io.Pipe is synchronous, so every write below blocks until the query's + // stream reader consumes it: the "process" is still producing output + // while the query runs. + const targetCount = 100 + go func() { + defer pwStdout.Close() + defer pwStderr.Close() + ruleClass := "go_library" + for i := 0; i < targetCount; i++ { + name := fmt.Sprintf("//pkg:target%d", i) + target := &buildpb.Target{ + Type: buildpb.Target_RULE.Enum(), + Rule: &buildpb.Rule{Name: &name, RuleClass: &ruleClass}, + } + if _, err := protodelim.MarshalTo(pwStdout, target); err != nil { + return + } + } + }() + + gomock.InOrder( + mockCmd.EXPECT().StdoutPipe().Return(prStdout, nil), + mockCmd.EXPECT().StderrPipe().Return(prStderr, nil), + mockCmd.EXPECT().Start().Return(nil), + mockCmd.EXPECT().Wait().DoAndReturn(func() error { + prStdout.Close() + prStderr.Close() + return nil + }), + ) + + client, err := NewBazelClient(context.Background(), Params{ + BazelCommand: "bazel", + WorkspacePath: "/tmp/test", + EnvVarsMap: map[string]string{}, + Logger: zap.NewNop().Sugar(), + ExecCommandContext: func(ctx context.Context, name string, arg ...string) commander { + return mockCmd + }, + }) + require.NoError(t, err) + + result, err := client.executeQueryInternal(context.Background(), "//...", nil) + require.NoError(t, err) + require.NotNil(t, result) + assert.Len(t, result.Target, targetCount) + }) + + t.Run("stderr is fully captured in the failure error", func(t *testing.T) { + defer goleak.VerifyNone(t) + ctrl := gomock.NewController(t) + mockCmd := commandermock.NewMockcommander(ctrl) + + prStderr, pwStderr := io.Pipe() + + const stderrTail = "FINAL STDERR LINE" + go func() { + defer pwStderr.Close() + _, _ = io.WriteString(pwStderr, strings.Repeat("bazel progress line\n", 200)+stderrTail) + }() + + gomock.InOrder( + mockCmd.EXPECT().StdoutPipe().Return(io.NopCloser(strings.NewReader("")), nil), + mockCmd.EXPECT().StderrPipe().Return(prStderr, nil), + mockCmd.EXPECT().Start().Return(nil), + mockCmd.EXPECT().Wait().DoAndReturn(func() error { + prStderr.Close() + return errors.New("exit status 7") + }), + ) + + client, err := NewBazelClient(context.Background(), Params{ + BazelCommand: "bazel", + WorkspacePath: "/tmp/test", + EnvVarsMap: map[string]string{}, + Logger: zap.NewNop().Sugar(), + ExecCommandContext: func(ctx context.Context, name string, arg ...string) commander { + return mockCmd + }, + }) + require.NoError(t, err) + + result, err := client.executeQueryInternal(context.Background(), "//...", nil) + require.Error(t, err) + require.NotNil(t, result) + // This asserts on the payload the error carries (the captured stderr), + // not on the error's own wording: the tail marker only appears if the + // whole stream was drained before the process was reaped. + assert.Contains(t, err.Error(), stderrTail) + }) +} + func TestExecuteQueryInternal_Failures(t *testing.T) { tests := []struct { name string diff --git a/core/bazel/stream.go b/core/bazel/stream.go index 959717c6..fb12abc1 100644 --- a/core/bazel/stream.go +++ b/core/bazel/stream.go @@ -23,41 +23,17 @@ import ( "google.golang.org/protobuf/encoding/protodelim" ) +// streamOutput copies src into dst until the stream ends. The copy blocks +// until it is unblocked externally — by the process exiting and closing the +// pipe write end, or by the caller force-closing the read end — so when ctx +// has ended, any copy error is just fallout from that shutdown and the +// context error is reported instead. func streamOutput(ctx context.Context, src io.Reader, dst io.Writer) error { - done := make(chan error, 1) - go func() { - _, err := io.Copy(dst, src) - done <- err - }() - - select { - case <-ctx.Done(): - <-done - return ctx.Err() - case err := <-done: - return err - } -} - -func streamAndParseTargets(ctx context.Context, src io.Reader, dst io.Writer) (*buildpb.QueryResult, error) { - type result struct { - queryResult *buildpb.QueryResult - err error - } - done := make(chan result, 1) - - go func() { - queryResult, err := getQueryResult(ctx, src, dst) - done <- result{queryResult: queryResult, err: err} - }() - - select { - case <-ctx.Done(): - <-done - return nil, ctx.Err() - case res := <-done: - return res.queryResult, res.err + _, err := io.Copy(dst, src) + if ctxErr := ctx.Err(); ctxErr != nil { + return ctxErr } + return err } // cancelCheckInterval is how often we poll ctx.Err() inside per-target hot loops. @@ -94,6 +70,10 @@ func getQueryResult(ctx context.Context, src io.Reader, dst io.Writer) (*buildpb } result.Target = append(result.Target, &target) } - + // Like streamOutput, prefer the context error over whatever the shutdown + // did to the stream mid-parse. + if err := ctx.Err(); err != nil { + return result, err + } return result, parseErr } diff --git a/core/execcmd/execcmd.go b/core/execcmd/execcmd.go index 1819b3f3..aa48e87d 100644 --- a/core/execcmd/execcmd.go +++ b/core/execcmd/execcmd.go @@ -26,12 +26,15 @@ import ( "time" ) -// gracePeriod is how long the child has to exit after SIGTERM before the Go -// runtime escalates to SIGKILL via Cmd.WaitDelay. -const gracePeriod = 10 * time.Second +// GracePeriod is how long the child has to exit after SIGTERM before the Go +// runtime escalates to SIGKILL via Cmd.WaitDelay. Exported so callers that +// layer their own shutdown timeouts on top of a command (e.g. force-closing +// its I/O pipes) can schedule them to fire only after the kill sequence has +// run its course. +const GracePeriod = 10 * time.Second // CommandContext is a drop-in replacement for exec.CommandContext that sends -// SIGTERM when ctx is canceled and escalates to SIGKILL after gracePeriod if +// SIGTERM when ctx is canceled and escalates to SIGKILL after GracePeriod if // the process is still running. This gives child processes (e.g. git, bazel) // a chance to release lock files and disconnect cleanly before being killed. func CommandContext(ctx context.Context, name string, args ...string) *exec.Cmd { @@ -43,6 +46,6 @@ func CommandContext(ctx context.Context, name string, args ...string) *exec.Cmd } return err } - cmd.WaitDelay = gracePeriod + cmd.WaitDelay = GracePeriod return cmd } From d8c3b1c73221390c292afba0265e9122944b98bc Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Wed, 8 Jul 2026 15:34:33 -0700 Subject: [PATCH 3/5] use files instead --- core/bazel/BUILD.bazel | 4 +- core/bazel/bazel.go | 13 +- core/bazel/bazel_test.go | 7 +- core/bazel/command.go | 20 +- core/bazel/command_test.go | 98 +++++ core/bazel/commandermock/commandermock.go | 56 +-- core/bazel/query.go | 119 +++--- core/bazel/query_test.go | 459 ++++++---------------- core/bazel/stream.go | 18 +- core/execcmd/execcmd.go | 13 +- 10 files changed, 306 insertions(+), 501 deletions(-) create mode 100644 core/bazel/command_test.go diff --git a/core/bazel/BUILD.bazel b/core/bazel/BUILD.bazel index 3fa695b7..064b4dbb 100644 --- a/core/bazel/BUILD.bazel +++ b/core/bazel/BUILD.bazel @@ -15,7 +15,6 @@ go_library( "@com_github_bazelbuild_buildtools//build_proto", "@org_golang_google_protobuf//encoding/protodelim", "@org_golang_google_protobuf//proto", - "@org_golang_x_sync//errgroup", "@org_uber_go_zap//:zap", ], ) @@ -24,16 +23,17 @@ go_test( name = "bazel_test", srcs = [ "bazel_test.go", + "command_test.go", "query_test.go", ], embed = [":bazel"], deps = [ "//core/bazel/commandermock", + "//core/execcmd", "@com_github_bazelbuild_buildtools//build_proto", "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", "@org_golang_google_protobuf//encoding/protodelim", - "@org_uber_go_goleak//:goleak", "@org_uber_go_mock//gomock", "@org_uber_go_zap//:zap", ], diff --git a/core/bazel/bazel.go b/core/bazel/bazel.go index cd2c3668..ec26eca9 100644 --- a/core/bazel/bazel.go +++ b/core/bazel/bazel.go @@ -33,13 +33,6 @@ import ( const ( // default query timeout if not provided in config _queryTimeout = 15 * time.Minute - // _pipeUnblockDelay is how long after the command context ends a query - // waits before force-closing the stdout/stderr pipes to unblock stream - // reads that never saw EOF (a descendant of the killed process can inherit - // the pipe write ends and hold them open, see go.dev/issue/23019). It - // exceeds execcmd.GracePeriod so the SIGTERM→SIGKILL sequence gets a - // chance to end the streams with a natural EOF first. - _pipeUnblockDelay = execcmd.GracePeriod + 5*time.Second ) type QueryRequest struct { @@ -64,7 +57,7 @@ type BazelClient struct { logger *zap.SugaredLogger execCommandContext func(ctx context.Context, name string, arg ...string) commander queryTimeout time.Duration - pipeUnblockDelay time.Duration + tempDir string streamLogs bool } @@ -88,7 +81,7 @@ func NewBazelClient(ctx context.Context, p Params) (*BazelClient, error) { cmd.Env = append(cmd.Env, key+"="+value) } cmd.Stdin = nil - return cmd + return &execCommander{Cmd: cmd} } } timeout := p.QueryTimeout @@ -107,7 +100,7 @@ func NewBazelClient(ctx context.Context, p Params) (*BazelClient, error) { logger: p.Logger, execCommandContext: execCmd, queryTimeout: timeout, - pipeUnblockDelay: _pipeUnblockDelay, + tempDir: os.TempDir(), streamLogs: p.StreamLogs, }, nil } diff --git a/core/bazel/bazel_test.go b/core/bazel/bazel_test.go index c76342aa..3999987b 100644 --- a/core/bazel/bazel_test.go +++ b/core/bazel/bazel_test.go @@ -16,7 +16,7 @@ package bazel import ( "context" - "os/exec" + "os" "testing" "github.com/stretchr/testify/assert" @@ -61,6 +61,7 @@ func TestNewBazelClient(t *testing.T) { assert.Equal(t, tt.params.WorkspacePath, client.workspacePath) assert.Equal(t, tt.params.EnvVarsMap, client.envVarsMap) assert.Equal(t, tt.params.Logger, client.logger) + assert.Equal(t, os.TempDir(), client.tempDir) assert.NotNil(t, client.execCommandContext) }) } @@ -79,8 +80,10 @@ func TestNewBazelClient_WithNilExecCommand(t *testing.T) { cmd := client.execCommandContext(context.Background(), "test", "arg1") require.NotNil(t, cmd) - execCmd, ok := cmd.(*exec.Cmd) + execCmd, ok := cmd.(*execCommander) require.True(t, ok) assert.Equal(t, "/workspace", execCmd.Dir) assert.Contains(t, execCmd.Env, "KEY=value") + assert.NotNil(t, execCmd.Cancel) + assert.Positive(t, execCmd.WaitDelay) } diff --git a/core/bazel/command.go b/core/bazel/command.go index 42dc5861..f756374c 100644 --- a/core/bazel/command.go +++ b/core/bazel/command.go @@ -16,11 +16,23 @@ package bazel import ( "io" + "os/exec" ) type commander interface { - StdoutPipe() (io.ReadCloser, error) - StderrPipe() (io.ReadCloser, error) - Start() error - Wait() error + Run(stdout, stderr io.Writer) error +} + +type execCommander struct { + *exec.Cmd +} + +// Run attaches the supplied writers before starting the command. Query +// execution intentionally supplies *os.File values: os/exec passes those file +// descriptors directly to the child instead of creating copy goroutines that +// Wait must drain. +func (c *execCommander) Run(stdout, stderr io.Writer) error { + c.Stdout = stdout + c.Stderr = stderr + return c.Cmd.Run() } diff --git a/core/bazel/command_test.go b/core/bazel/command_test.go new file mode 100644 index 00000000..160195f4 --- /dev/null +++ b/core/bazel/command_test.go @@ -0,0 +1,98 @@ +// Copyright (c) 2025 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package bazel + +import ( + "context" + "io" + "os" + "os/exec" + "os/signal" + "strconv" + "strings" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/require" + "github.com/uber/tango/core/execcmd" +) + +func TestExecCommander_OutputFilesDoNotWaitForDescendant(t *testing.T) { + stdout, err := os.CreateTemp(t.TempDir(), "stdout-*") + require.NoError(t, err) + defer stdout.Close() + stderr, err := os.CreateTemp(t.TempDir(), "stderr-*") + require.NoError(t, err) + defer stderr.Close() + + cmd := &execCommander{Cmd: exec.Command("/bin/sh", "-c", "sleep 5 & echo $! >&2")} + started := time.Now() + require.NoError(t, cmd.Run(stdout, stderr)) + elapsed := time.Since(started) + + contents, err := os.ReadFile(stderr.Name()) + require.NoError(t, err) + pid, err := strconv.Atoi(strings.TrimSpace(string(contents))) + require.NoError(t, err) + process, err := os.FindProcess(pid) + require.NoError(t, err) + _ = process.Kill() + require.Less(t, elapsed, 2*time.Second) +} + +func TestExecCommander_ContextCancellation(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + readyReader, readyWriter, err := os.Pipe() + require.NoError(t, err) + defer readyReader.Close() + defer readyWriter.Close() + + cmd := execcmd.CommandContext(ctx, os.Args[0], "-test.run=TestBazelCommandHelper") + cmd.Env = append(os.Environ(), "TANGO_BAZEL_COMMAND_HELPER=1") + cmd.ExtraFiles = []*os.File{readyWriter} + runner := &execCommander{Cmd: cmd} + + done := make(chan error, 1) + go func() { + done <- runner.Run(io.Discard, io.Discard) + }() + ready := make([]byte, 1) + _, err = io.ReadFull(readyReader, ready) + require.NoError(t, err) + + cancel() + select { + case err := <-done: + require.ErrorIs(t, err, context.Canceled) + case <-time.After(2 * time.Second): + t.Fatal("command did not exit after context cancellation") + } +} + +func TestBazelCommandHelper(t *testing.T) { + if os.Getenv("TANGO_BAZEL_COMMAND_HELPER") != "1" { + return + } + signals := make(chan os.Signal, 1) + signal.Notify(signals, syscall.SIGTERM) + + ready := os.NewFile(3, "ready") + _, _ = ready.Write([]byte{1}) + _ = ready.Close() + + <-signals +} diff --git a/core/bazel/commandermock/commandermock.go b/core/bazel/commandermock/commandermock.go index f11f4c66..54f2d463 100644 --- a/core/bazel/commandermock/commandermock.go +++ b/core/bazel/commandermock/commandermock.go @@ -40,60 +40,16 @@ func (m *Mockcommander) EXPECT() *MockcommanderMockRecorder { return m.recorder } -// Start mocks base method. -func (m *Mockcommander) Start() error { +// Run mocks base method. +func (m *Mockcommander) Run(stdout, stderr io.Writer) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Start") + ret := m.ctrl.Call(m, "Run", stdout, stderr) ret0, _ := ret[0].(error) return ret0 } -// Start indicates an expected call of Start. -func (mr *MockcommanderMockRecorder) Start() *gomock.Call { +// Run indicates an expected call of Run. +func (mr *MockcommanderMockRecorder) Run(stdout, stderr any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Start", reflect.TypeOf((*Mockcommander)(nil).Start)) -} - -// StderrPipe mocks base method. -func (m *Mockcommander) StderrPipe() (io.ReadCloser, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "StderrPipe") - ret0, _ := ret[0].(io.ReadCloser) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// StderrPipe indicates an expected call of StderrPipe. -func (mr *MockcommanderMockRecorder) StderrPipe() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "StderrPipe", reflect.TypeOf((*Mockcommander)(nil).StderrPipe)) -} - -// StdoutPipe mocks base method. -func (m *Mockcommander) StdoutPipe() (io.ReadCloser, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "StdoutPipe") - ret0, _ := ret[0].(io.ReadCloser) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// StdoutPipe indicates an expected call of StdoutPipe. -func (mr *MockcommanderMockRecorder) StdoutPipe() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "StdoutPipe", reflect.TypeOf((*Mockcommander)(nil).StdoutPipe)) -} - -// Wait mocks base method. -func (m *Mockcommander) Wait() error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Wait") - ret0, _ := ret[0].(error) - return ret0 -} - -// Wait indicates an expected call of Wait. -func (mr *MockcommanderMockRecorder) Wait() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Wait", reflect.TypeOf((*Mockcommander)(nil).Wait)) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Run", reflect.TypeOf((*Mockcommander)(nil).Run), stdout, stderr) } diff --git a/core/bazel/query.go b/core/bazel/query.go index 2d5e97a3..a5398a39 100644 --- a/core/bazel/query.go +++ b/core/bazel/query.go @@ -15,18 +15,16 @@ package bazel import ( - "bytes" "compress/gzip" "context" + "errors" "fmt" "io" "os" "strings" - "time" buildpb "github.com/bazelbuild/buildtools/build_proto" "go.uber.org/zap" - "golang.org/x/sync/errgroup" "google.golang.org/protobuf/proto" ) @@ -51,84 +49,65 @@ func (b *BazelClient) ExecuteQuery(ctx context.Context, req *QueryRequest) (*Que } func (b *BazelClient) executeQueryInternal(ctx context.Context, query string, startupOptions []string, additionalArgs ...string) (*buildpb.QueryResult, error) { - var ( - stdoutBuf, stderrBuf bytes.Buffer - queryResults *buildpb.QueryResult - ) cmdCtx, cancel := context.WithTimeout(ctx, b.queryTimeout) defer cancel() - // setup bazel query command - cmd := b.setupCommand(cmdCtx, query, startupOptions, additionalArgs...) - // Get pipes for stdout and stderr BEFORE starting the process - stdout, err := cmd.StdoutPipe() - if err != nil { + if err := cmdCtx.Err(); err != nil { return nil, err } - stderr, err := cmd.StderrPipe() + cmd := b.setupCommand(cmdCtx, query, startupOptions, additionalArgs...) + + // Keep command output backed by files rather than StdoutPipe, StderrPipe, + // or custom pipe writers. A Bazel descendant can inherit a pipe's write end + // and prevent readers from seeing EOF after Bazel exits. Calling Wait early + // to activate its pipe cleanup is not an alternative: Wait may close the + // read ends before they are fully drained, truncating output. Direct files + // avoid both lifecycle problems and are rewound for parsing below. + stdout, err := os.CreateTemp(b.tempDir, "tango-bazel-stdout-*") if err != nil { - return nil, err + return nil, fmt.Errorf("create stdout file: %w", err) } + defer os.Remove(stdout.Name()) + defer stdout.Close() + // In streamLogs mode, stderr goes straight to os.Stderr so operators see // bazel progress live; otherwise it's captured for inclusion in failure // errors (see wrapQueryFailure). - stderrSink := io.Writer(&stderrBuf) - if b.streamLogs { - stderrSink = os.Stderr - } - g, gCtx := errgroup.WithContext(cmdCtx) - - // Start the process - if err = cmd.Start(); err != nil { - return nil, err - } - // stream and parse targets - g.Go(func() error { - res, err := getQueryResult(gCtx, stdout, &stdoutBuf) + stderrSink := io.Writer(os.Stderr) + stderrPath := "" + var stderr *os.File + if !b.streamLogs { + stderr, err = os.CreateTemp(b.tempDir, "tango-bazel-stderr-*") if err != nil { - return err + return nil, fmt.Errorf("create stderr file: %w", err) } - queryResults = res - return nil - }) - // stream stderr - g.Go(func() error { - return streamOutput(gCtx, stderr, stderrSink) - }) + defer os.Remove(stderr.Name()) + defer stderr.Close() + stderrSink = stderr + stderrPath = stderr.Name() + } - // The stream reads normally end with EOF when the process dies and the - // kernel closes the pipe write ends. But a descendant of a killed process - // that inherited the pipes can hold the write ends open indefinitely - // (go.dev/issue/23019). cmd.Wait would force-close the parent ends, but we - // only call Wait after the reads finish, so this watchdog is what - // guarantees the reads can't block forever. - readsDone := make(chan struct{}) - go func() { - select { - case <-readsDone: - return - case <-cmdCtx.Done(): - } - timer := time.NewTimer(b.pipeUnblockDelay) - defer timer.Stop() - select { - case <-readsDone: - case <-timer.C: - stdout.Close() - stderr.Close() - } - }() + // CommandContext and WaitDelay still govern process shutdown; using files + // changes only output transport and does not weaken cancellation. + waitErr := cmd.Run(stdout, stderrSink) + if ctxErr := cmdCtx.Err(); waitErr != nil && ctxErr != nil { + // A process terminated by SIGTERM or SIGKILL reports an ExitError, which + // does not wrap the context error. Preserve both causes so callers can + // reliably identify cancellation, and avoid parsing output after it. + return nil, b.wrapQueryFailure("bazel query canceled", errors.Join(ctxErr, waitErr), "") + } - // Drain the streams before reaping the process: cmd.Wait closes the parent - // pipe ends as soon as the process exits, which would truncate whatever - // the readers hadn't consumed yet and race on the output buffers. - streamErr := g.Wait() - close(readsDone) - waitErr := cmd.Wait() + if _, err := stdout.Seek(0, io.SeekStart); err != nil { + return nil, fmt.Errorf("rewind bazel query output: %w", err) + } + queryResults, streamErr := getQueryResult(cmdCtx, stdout) if waitErr != nil { - return queryResults, b.wrapQueryFailure("bazel query failed", waitErr, &stderrBuf) + return queryResults, b.wrapQueryFailure("bazel query failed", waitErr, stderrPath) } if streamErr != nil { - return nil, b.wrapQueryFailure("stream processing failed", streamErr, &stderrBuf) + if cmdCtx.Err() != nil { + stderrPath = "" + } + return nil, b.wrapQueryFailure("stream processing failed", streamErr, stderrPath) } b.logger.Debugw("Parsed targets from bazel query", zap.Int("target_count", len(queryResults.Target))) return queryResults, nil @@ -138,10 +117,14 @@ func (b *BazelClient) executeQueryInternal(ctx context.Context, query string, st // was captured (streamLogs off), its contents are appended so the failure is // self-contained. When streamLogs is on the operator has already seen stderr // live, so it's omitted. -func (b *BazelClient) wrapQueryFailure(msg string, cause error, stderrBuf *bytes.Buffer) error { +func (b *BazelClient) wrapQueryFailure(msg string, cause error, stderrPath string) error { tail := "" - if !b.streamLogs { - tail = "\nstderr:\n" + stderrBuf.String() + if stderrPath != "" { + stderr, err := os.ReadFile(stderrPath) + if err != nil { + return fmt.Errorf("%s: %w (read stderr: %v)", msg, cause, err) + } + tail = "\nstderr:\n" + string(stderr) } return fmt.Errorf("%s: %w%s", msg, cause, tail) } diff --git a/core/bazel/query_test.go b/core/bazel/query_test.go index a35737ad..d3f129ab 100644 --- a/core/bazel/query_test.go +++ b/core/bazel/query_test.go @@ -20,6 +20,7 @@ import ( "errors" "fmt" "io" + "os" "strings" "testing" "time" @@ -28,100 +29,46 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/uber/tango/core/bazel/commandermock" - "go.uber.org/goleak" "go.uber.org/mock/gomock" "go.uber.org/zap" "google.golang.org/protobuf/encoding/protodelim" ) func TestExecuteQuery_Success(t *testing.T) { - defer goleak.VerifyNone(t) ctrl := gomock.NewController(t) mockCmd := commandermock.NewMockcommander(ctrl) - var ( - ruleName, ruleClass = "//pkg:target", "go_library" - ) - target := &buildpb.Target{ - Type: buildpb.Target_RULE.Enum(), - Rule: &buildpb.Rule{ - Name: &ruleName, - RuleClass: &ruleClass, - }, - } - var protoData bytes.Buffer - _, err := protodelim.MarshalTo(&protoData, target) - require.NoError(t, err) - gomock.InOrder( - mockCmd.EXPECT().StdoutPipe().Return(io.NopCloser(&protoData), nil), - mockCmd.EXPECT().StderrPipe().Return(io.NopCloser(strings.NewReader("")), nil), - mockCmd.EXPECT().Start().Return(nil), - mockCmd.EXPECT().Wait().Return(nil), - ) - client, err := NewBazelClient(context.Background(), Params{ - BazelCommand: "bazel", - WorkspacePath: "/tmp/test", - EnvVarsMap: map[string]string{}, - Logger: zap.NewNop().Sugar(), - ExecCommandContext: func(ctx context.Context, name string, arg ...string) commander { - return mockCmd - }, + protoData := marshalTarget(t, "//pkg:target") + mockCmd.EXPECT().Run(gomock.Any(), gomock.Any()).DoAndReturn(func(stdout, _ io.Writer) error { + _, err := stdout.Write(protoData) + return err }) + client := newTestClient(t, func(context.Context, string, ...string) commander { return mockCmd }) resp, err := client.ExecuteQuery(context.Background(), &QueryRequest{Query: "//..."}) + require.NoError(t, err) - require.NotNil(t, resp) - require.NotNil(t, resp.Result) - require.Equal(t, 1, len(resp.Result.Target)) - assert.Equal(t, &ruleName, resp.Result.Target[0].Rule.Name) - assert.Equal(t, &ruleClass, resp.Result.Target[0].Rule.RuleClass) + require.Len(t, resp.Result.Target, 1) + assert.Equal(t, "//pkg:target", resp.Result.Target[0].Rule.GetName()) } func TestExecuteQuery_WithStartupOptions(t *testing.T) { - defer goleak.VerifyNone(t) ctrl := gomock.NewController(t) mockCmd := commandermock.NewMockcommander(ctrl) - var ( - ruleName, ruleClass = "//pkg:target", "go_library" - ) - target := &buildpb.Target{ - Type: buildpb.Target_RULE.Enum(), - Rule: &buildpb.Rule{ - Name: &ruleName, - RuleClass: &ruleClass, - }, - } - var protoData bytes.Buffer - _, err := protodelim.MarshalTo(&protoData, target) - require.NoError(t, err) + mockCmd.EXPECT().Run(gomock.Any(), gomock.Any()).Return(nil) var capturedArgs []string - gomock.InOrder( - mockCmd.EXPECT().StdoutPipe().Return(io.NopCloser(&protoData), nil), - mockCmd.EXPECT().StderrPipe().Return(io.NopCloser(strings.NewReader("")), nil), - mockCmd.EXPECT().Start().Return(nil), - mockCmd.EXPECT().Wait().Return(nil), - ) - client, err := NewBazelClient(context.Background(), Params{ - BazelCommand: "bazel", - WorkspacePath: "/tmp/test", - EnvVarsMap: map[string]string{}, - Logger: zap.NewNop().Sugar(), - ExecCommandContext: func(ctx context.Context, name string, arg ...string) commander { - capturedArgs = arg - return mockCmd - }, + client := newTestClient(t, func(_ context.Context, _ string, args ...string) commander { + capturedArgs = args + return mockCmd }) - require.NoError(t, err) - resp, err := client.ExecuteQuery(context.Background(), &QueryRequest{ + _, err := client.ExecuteQuery(context.Background(), &QueryRequest{ Query: "//...", StartupOptions: []string{"--bazelrc=/custom/.bazelrc", "--output_base=/tmp/bazel"}, AdditionalArgs: []string{"--keep_going"}, }) - require.NoError(t, err) - require.NotNil(t, resp) - // Verify command structure: bazel query --output=streamed_proto - require.Equal(t, []string{ + require.NoError(t, err) + assert.Equal(t, []string{ "--bazelrc=/custom/.bazelrc", "--output_base=/tmp/bazel", "query", @@ -132,313 +79,143 @@ func TestExecuteQuery_WithStartupOptions(t *testing.T) { } func TestExecuteQueryInternal_ContextTimeout(t *testing.T) { - defer goleak.VerifyNone(t) ctrl := gomock.NewController(t) mockCmd := commandermock.NewMockcommander(ctrl) + var cmdCtx context.Context + mockCmd.EXPECT().Run(gomock.Any(), gomock.Any()).DoAndReturn(func(io.Writer, io.Writer) error { + <-cmdCtx.Done() + return errors.New("signal: terminated") + }) - prStdout, pwStdout := io.Pipe() - prStderr, pwStderr := io.Pipe() - - // Set up the mock expectations in the exact order they will be called. - gomock.InOrder( - mockCmd.EXPECT().StdoutPipe().Return(prStdout, nil), - mockCmd.EXPECT().StderrPipe().Return(prStderr, nil), - mockCmd.EXPECT().Start().Return(nil), - mockCmd.EXPECT().Wait().DoAndReturn(func() error { - // Simulate process ending after timeout - return context.DeadlineExceeded - }), - ) - - client, err := NewBazelClient(context.Background(), Params{ - BazelCommand: "bazel", - WorkspacePath: "/tmp/test", - Logger: zap.NewNop().Sugar(), - EnvVarsMap: map[string]string{}, - QueryTimeout: 10 * time.Millisecond, // Short timeout for test - - ExecCommandContext: func(ctx context.Context, name string, arg ...string) commander { - // Simulate process behavior: when context is cancelled, close pipes - go func() { - <-ctx.Done() - // Close pipes to unblock readers - pwStdout.Close() - pwStderr.Close() - }() - return mockCmd - }, + client := newTestClient(t, func(ctx context.Context, _ string, _ ...string) commander { + cmdCtx = ctx + return mockCmd }) - require.NoError(t, err) + client.queryTimeout = 10 * time.Millisecond result, err := client.executeQueryInternal(context.Background(), "//...", nil) - require.Nil(t, result) + require.Error(t, err) - // Should get timeout or deadline exceeded error - assert.Contains(t, err.Error(), "deadline exceeded") + assert.ErrorIs(t, err, context.DeadlineExceeded) + assert.Nil(t, result) } -// TestExecuteQueryInternal_PipesHeldOpenAfterCancel simulates a descendant of -// the killed bazel process inheriting the stdout/stderr pipes and holding -// their write ends open past the kill (go.dev/issue/23019). Since cmd.Wait — -// which would force-close the parent ends — only runs after the stream reads -// finish, the pipe watchdog must unblock the reads or the query would hang. -func TestExecuteQueryInternal_PipesHeldOpenAfterCancel(t *testing.T) { - defer goleak.VerifyNone(t) +func TestExecuteQueryInternal_PreCanceledContext(t *testing.T) { ctrl := gomock.NewController(t) mockCmd := commandermock.NewMockcommander(ctrl) + client := newTestClient(t, func(context.Context, string, ...string) commander { return mockCmd }) + ctx, cancel := context.WithCancel(context.Background()) + cancel() - // The write ends are intentionally never closed by the "process". - prStdout, pwStdout := io.Pipe() - prStderr, pwStderr := io.Pipe() - defer pwStdout.Close() - defer pwStderr.Close() + result, err := client.executeQueryInternal(ctx, "//...", nil) - var cmdCtx context.Context - gomock.InOrder( - mockCmd.EXPECT().StdoutPipe().Return(prStdout, nil), - mockCmd.EXPECT().StderrPipe().Return(prStderr, nil), - mockCmd.EXPECT().Start().Return(nil), - mockCmd.EXPECT().Wait().DoAndReturn(func() error { - // The process itself dies with the context; only its orphaned - // descendant lives on, holding the pipes. - <-cmdCtx.Done() - return context.DeadlineExceeded - }), - ) + require.ErrorIs(t, err, context.Canceled) + assert.Nil(t, result) +} - client, err := NewBazelClient(context.Background(), Params{ - BazelCommand: "bazel", - WorkspacePath: "/tmp/test", - EnvVarsMap: map[string]string{}, - Logger: zap.NewNop().Sugar(), - QueryTimeout: 10 * time.Millisecond, - ExecCommandContext: func(ctx context.Context, name string, arg ...string) commander { - cmdCtx = ctx - return mockCmd - }, +func TestExecuteQueryInternal_CancelDuringParsing(t *testing.T) { + ctrl := gomock.NewController(t) + mockCmd := commandermock.NewMockcommander(ctrl) + ctx, cancel := context.WithCancel(context.Background()) + mockCmd.EXPECT().Run(gomock.Any(), gomock.Any()).DoAndReturn(func(stdout, _ io.Writer) error { + _, err := stdout.Write(marshalTarget(t, "//pkg:target")) + cancel() + return err }) - require.NoError(t, err) - client.pipeUnblockDelay = 20 * time.Millisecond - type queryOutcome struct { - result *buildpb.QueryResult - err error - } - done := make(chan queryOutcome, 1) - go func() { - result, err := client.executeQueryInternal(context.Background(), "//...", nil) - done <- queryOutcome{result: result, err: err} - }() + client := newTestClient(t, func(context.Context, string, ...string) commander { return mockCmd }) + result, err := client.executeQueryInternal(ctx, "//...", nil) - select { - case out := <-done: - require.Error(t, out.err) - assert.ErrorIs(t, out.err, context.DeadlineExceeded) - assert.Nil(t, out.result) - case <-time.After(10 * time.Second): - t.Fatal("executeQueryInternal never returned: pipe reads were not unblocked") - } + require.ErrorIs(t, err, context.Canceled) + assert.Nil(t, result) } -// TestExecuteQueryInternal_DrainsStreamsBeforeWait verifies that cmd.Wait runs -// only after the stream reads finish. The mock Waits emulate exec.Cmd.Wait, -// which force-closes the parent pipe ends when it returns — had Wait run -// before the reads completed, the still-streaming output would be truncated. -func TestExecuteQueryInternal_DrainsStreamsBeforeWait(t *testing.T) { - t.Run("stdout is fully parsed", func(t *testing.T) { - defer goleak.VerifyNone(t) - ctrl := gomock.NewController(t) - mockCmd := commandermock.NewMockcommander(ctrl) - - prStdout, pwStdout := io.Pipe() - prStderr, pwStderr := io.Pipe() - - // io.Pipe is synchronous, so every write below blocks until the query's - // stream reader consumes it: the "process" is still producing output - // while the query runs. - const targetCount = 100 - go func() { - defer pwStdout.Close() - defer pwStderr.Close() - ruleClass := "go_library" - for i := 0; i < targetCount; i++ { - name := fmt.Sprintf("//pkg:target%d", i) - target := &buildpb.Target{ - Type: buildpb.Target_RULE.Enum(), - Rule: &buildpb.Rule{Name: &name, RuleClass: &ruleClass}, - } - if _, err := protodelim.MarshalTo(pwStdout, target); err != nil { - return - } +func TestExecuteQueryInternal_CapturesCompleteOutput(t *testing.T) { + ctrl := gomock.NewController(t) + mockCmd := commandermock.NewMockcommander(ctrl) + const targetCount = 100 + const stderrTail = "FINAL STDERR LINE" + mockCmd.EXPECT().Run(gomock.Any(), gomock.Any()).DoAndReturn(func(stdout, stderr io.Writer) error { + for i := 0; i < targetCount; i++ { + if _, err := stdout.Write(marshalTarget(t, fmt.Sprintf("//pkg:target%d", i))); err != nil { + return err } - }() - - gomock.InOrder( - mockCmd.EXPECT().StdoutPipe().Return(prStdout, nil), - mockCmd.EXPECT().StderrPipe().Return(prStderr, nil), - mockCmd.EXPECT().Start().Return(nil), - mockCmd.EXPECT().Wait().DoAndReturn(func() error { - prStdout.Close() - prStderr.Close() - return nil - }), - ) - - client, err := NewBazelClient(context.Background(), Params{ - BazelCommand: "bazel", - WorkspacePath: "/tmp/test", - EnvVarsMap: map[string]string{}, - Logger: zap.NewNop().Sugar(), - ExecCommandContext: func(ctx context.Context, name string, arg ...string) commander { - return mockCmd - }, - }) - require.NoError(t, err) - - result, err := client.executeQueryInternal(context.Background(), "//...", nil) - require.NoError(t, err) - require.NotNil(t, result) - assert.Len(t, result.Target, targetCount) + } + _, err := io.WriteString(stderr, strings.Repeat("bazel progress line\n", 200)+stderrTail) + if err != nil { + return err + } + return errors.New("exit status 7") }) - t.Run("stderr is fully captured in the failure error", func(t *testing.T) { - defer goleak.VerifyNone(t) - ctrl := gomock.NewController(t) - mockCmd := commandermock.NewMockcommander(ctrl) - - prStderr, pwStderr := io.Pipe() + client := newTestClient(t, func(context.Context, string, ...string) commander { return mockCmd }) + result, err := client.executeQueryInternal(context.Background(), "//...", nil) - const stderrTail = "FINAL STDERR LINE" - go func() { - defer pwStderr.Close() - _, _ = io.WriteString(pwStderr, strings.Repeat("bazel progress line\n", 200)+stderrTail) - }() + require.Error(t, err) + require.Len(t, result.Target, targetCount) + assert.Contains(t, err.Error(), stderrTail) +} - gomock.InOrder( - mockCmd.EXPECT().StdoutPipe().Return(io.NopCloser(strings.NewReader("")), nil), - mockCmd.EXPECT().StderrPipe().Return(prStderr, nil), - mockCmd.EXPECT().Start().Return(nil), - mockCmd.EXPECT().Wait().DoAndReturn(func() error { - prStderr.Close() - return errors.New("exit status 7") - }), - ) +func TestExecuteQueryInternal_ParseFailure(t *testing.T) { + ctrl := gomock.NewController(t) + mockCmd := commandermock.NewMockcommander(ctrl) + mockCmd.EXPECT().Run(gomock.Any(), gomock.Any()).DoAndReturn(func(stdout, _ io.Writer) error { + _, err := io.WriteString(stdout, "not a streamed proto") + return err + }) - client, err := NewBazelClient(context.Background(), Params{ - BazelCommand: "bazel", - WorkspacePath: "/tmp/test", - EnvVarsMap: map[string]string{}, - Logger: zap.NewNop().Sugar(), - ExecCommandContext: func(ctx context.Context, name string, arg ...string) commander { - return mockCmd - }, - }) - require.NoError(t, err) + client := newTestClient(t, func(context.Context, string, ...string) commander { return mockCmd }) + result, err := client.executeQueryInternal(context.Background(), "//...", nil) - result, err := client.executeQueryInternal(context.Background(), "//...", nil) - require.Error(t, err) - require.NotNil(t, result) - // This asserts on the payload the error carries (the captured stderr), - // not on the error's own wording: the tail marker only appears if the - // whole stream was drained before the process was reaped. - assert.Contains(t, err.Error(), stderrTail) - }) + require.Error(t, err) + assert.Nil(t, result) } -func TestExecuteQueryInternal_Failures(t *testing.T) { - tests := []struct { - name string - setupMock func(*commandermock.Mockcommander) - expectedError string - expectNilResult bool - }{ - { - name: "stdout pipe failure", - setupMock: func(m *commandermock.Mockcommander) { - m.EXPECT().StdoutPipe().Return(nil, errors.New("stdout pipe failed")) - }, - expectedError: "stdout pipe failed", - expectNilResult: true, - }, - { - name: "stderr pipe failure", - setupMock: func(m *commandermock.Mockcommander) { - m.EXPECT().StdoutPipe().Return(io.NopCloser(strings.NewReader("")), nil) - m.EXPECT().StderrPipe().Return(nil, errors.New("stderr pipe failed")) - }, - expectedError: "stderr pipe failed", - expectNilResult: true, - }, - { - name: "command start failure", - setupMock: func(m *commandermock.Mockcommander) { - m.EXPECT().StdoutPipe().Return(io.NopCloser(strings.NewReader("")), nil) - m.EXPECT().StderrPipe().Return(io.NopCloser(strings.NewReader("")), nil) - m.EXPECT().Start().Return(errors.New("failed to start process")) - }, - expectedError: "failed to start process", - expectNilResult: true, - }, - { - name: "command wait failure", - setupMock: func(m *commandermock.Mockcommander) { - m.EXPECT().StdoutPipe().Return(io.NopCloser(strings.NewReader("")), nil) - m.EXPECT().StderrPipe().Return(io.NopCloser(strings.NewReader("")), nil) - m.EXPECT().Start().Return(nil) - m.EXPECT().Wait().Return(errors.New("command wait failed")) - }, - expectedError: "command wait failed", - expectNilResult: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - defer goleak.VerifyNone(t) - ctrl := gomock.NewController(t) - mockCmd := commandermock.NewMockcommander(ctrl) - tt.setupMock(mockCmd) +func TestExecuteQueryInternal_StreamLogs(t *testing.T) { + ctrl := gomock.NewController(t) + mockCmd := commandermock.NewMockcommander(ctrl) + mockCmd.EXPECT().Run(gomock.Any(), os.Stderr).Return(errors.New("exit status 1")) - client, err := NewBazelClient(context.Background(), Params{ - BazelCommand: "bazel", - WorkspacePath: "/tmp/test", - EnvVarsMap: map[string]string{}, - Logger: zap.NewNop().Sugar(), - ExecCommandContext: func(ctx context.Context, name string, arg ...string) commander { - return mockCmd - }, - }) - require.NoError(t, err) - result, err := client.executeQueryInternal(context.Background(), "//...", nil) - require.Error(t, err) - if tt.expectNilResult { - require.Nil(t, result) - } else { - require.NotNil(t, result) - } - assert.Contains(t, err.Error(), tt.expectedError) - }) - } + client := newTestClient(t, func(context.Context, string, ...string) commander { return mockCmd }) + client.streamLogs = true + _, err := client.executeQueryInternal(context.Background(), "//...", nil) + require.Error(t, err) } func TestExecuteQuery_ErrorCase(t *testing.T) { - defer goleak.VerifyNone(t) ctrl := gomock.NewController(t) mockCmd := commandermock.NewMockcommander(ctrl) + mockCmd.EXPECT().Run(gomock.Any(), gomock.Any()).Return(errors.New("command failed")) - mockCmd.EXPECT().StdoutPipe().Return(nil, errors.New("stdout pipe failed")) + client := newTestClient(t, func(context.Context, string, ...string) commander { return mockCmd }) + resp, err := client.ExecuteQuery(context.Background(), &QueryRequest{Query: "//..."}) + require.Error(t, err) + assert.Nil(t, resp) + assert.Contains(t, err.Error(), "command failed") +} + +func marshalTarget(t *testing.T, name string) []byte { + t.Helper() + ruleClass := "go_library" + target := &buildpb.Target{ + Type: buildpb.Target_RULE.Enum(), + Rule: &buildpb.Rule{Name: &name, RuleClass: &ruleClass}, + } + var out bytes.Buffer + _, err := protodelim.MarshalTo(&out, target) + require.NoError(t, err) + return out.Bytes() +} + +func newTestClient(t *testing.T, execCmd func(context.Context, string, ...string) commander) *BazelClient { + t.Helper() client, err := NewBazelClient(context.Background(), Params{ - BazelCommand: "bazel", - WorkspacePath: "/tmp/test", - EnvVarsMap: map[string]string{}, - Logger: zap.NewNop().Sugar(), - ExecCommandContext: func(ctx context.Context, name string, arg ...string) commander { - return mockCmd - }, + BazelCommand: "bazel", + WorkspacePath: "/tmp/test", + EnvVarsMap: map[string]string{}, + Logger: zap.NewNop().Sugar(), + ExecCommandContext: execCmd, }) - - resp, err := client.ExecuteQuery(context.Background(), &QueryRequest{Query: "//..."}) - require.Error(t, err) - require.Nil(t, resp) - assert.Contains(t, err.Error(), "stdout pipe failed") + require.NoError(t, err) + return client } diff --git a/core/bazel/stream.go b/core/bazel/stream.go index fb12abc1..751c1eb8 100644 --- a/core/bazel/stream.go +++ b/core/bazel/stream.go @@ -23,31 +23,17 @@ import ( "google.golang.org/protobuf/encoding/protodelim" ) -// streamOutput copies src into dst until the stream ends. The copy blocks -// until it is unblocked externally — by the process exiting and closing the -// pipe write end, or by the caller force-closing the read end — so when ctx -// has ended, any copy error is just fallout from that shutdown and the -// context error is reported instead. -func streamOutput(ctx context.Context, src io.Reader, dst io.Writer) error { - _, err := io.Copy(dst, src) - if ctxErr := ctx.Err(); ctxErr != nil { - return ctxErr - } - return err -} - // cancelCheckInterval is how often we poll ctx.Err() inside per-target hot loops. // Picked to keep overhead negligible while still surfacing cancellation in <100ms // for typical target rates. const cancelCheckInterval = 1024 // getQueryResult reads a QueryResult containing targets from the stream and returns it. -func getQueryResult(ctx context.Context, src io.Reader, dst io.Writer) (*buildpb.QueryResult, error) { +func getQueryResult(ctx context.Context, src io.Reader) (*buildpb.QueryResult, error) { result := &buildpb.QueryResult{ Target: make([]*buildpb.Target, 0), } - tr := io.TeeReader(src, dst) - br := bufio.NewReader(tr) + br := bufio.NewReader(src) unmarshalOpts := protodelim.UnmarshalOptions{ MaxSize: 64 * 1024 * 1024, // 64MB limit } diff --git a/core/execcmd/execcmd.go b/core/execcmd/execcmd.go index aa48e87d..1819b3f3 100644 --- a/core/execcmd/execcmd.go +++ b/core/execcmd/execcmd.go @@ -26,15 +26,12 @@ import ( "time" ) -// GracePeriod is how long the child has to exit after SIGTERM before the Go -// runtime escalates to SIGKILL via Cmd.WaitDelay. Exported so callers that -// layer their own shutdown timeouts on top of a command (e.g. force-closing -// its I/O pipes) can schedule them to fire only after the kill sequence has -// run its course. -const GracePeriod = 10 * time.Second +// gracePeriod is how long the child has to exit after SIGTERM before the Go +// runtime escalates to SIGKILL via Cmd.WaitDelay. +const gracePeriod = 10 * time.Second // CommandContext is a drop-in replacement for exec.CommandContext that sends -// SIGTERM when ctx is canceled and escalates to SIGKILL after GracePeriod if +// SIGTERM when ctx is canceled and escalates to SIGKILL after gracePeriod if // the process is still running. This gives child processes (e.g. git, bazel) // a chance to release lock files and disconnect cleanly before being killed. func CommandContext(ctx context.Context, name string, args ...string) *exec.Cmd { @@ -46,6 +43,6 @@ func CommandContext(ctx context.Context, name string, args ...string) *exec.Cmd } return err } - cmd.WaitDelay = GracePeriod + cmd.WaitDelay = gracePeriod return cmd } From 37a3b3e128845582f064f69c9ed947f4d7c2fac0 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Wed, 8 Jul 2026 15:41:05 -0700 Subject: [PATCH 4/5] fix hardcoded mock --- core/bazel/BUILD.bazel | 13 +++++- core/bazel/bazel.go | 6 +-- core/bazel/bazel_test.go | 2 +- core/bazel/command.go | 3 +- core/bazel/commandermock/BUILD.bazel | 9 ---- core/bazel/commandermock/commandermock.go | 55 ----------------------- core/bazel/query.go | 2 +- core/bazel/query_test.go | 41 +++++++++-------- 8 files changed, 38 insertions(+), 93 deletions(-) delete mode 100644 core/bazel/commandermock/BUILD.bazel delete mode 100644 core/bazel/commandermock/commandermock.go diff --git a/core/bazel/BUILD.bazel b/core/bazel/BUILD.bazel index 064b4dbb..2cc80972 100644 --- a/core/bazel/BUILD.bazel +++ b/core/bazel/BUILD.bazel @@ -1,3 +1,4 @@ +load("@rules_go//extras:gomock.bzl", "gomock") load("@rules_go//go:def.bzl", "go_library", "go_test") go_library( @@ -24,17 +25,25 @@ go_test( srcs = [ "bazel_test.go", "command_test.go", + "mock_commander_test.go", "query_test.go", ], embed = [":bazel"], deps = [ - "//core/bazel/commandermock", "//core/execcmd", "@com_github_bazelbuild_buildtools//build_proto", + "@com_github_golang_mock//gomock", "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", "@org_golang_google_protobuf//encoding/protodelim", - "@org_uber_go_mock//gomock", "@org_uber_go_zap//:zap", ], ) + +gomock( + name = "mock_commander", + out = "mock_commander_test.go", + interfaces = ["Commander"], + library = ":bazel", + package = "bazel", +) diff --git a/core/bazel/bazel.go b/core/bazel/bazel.go index ec26eca9..86c9e7ec 100644 --- a/core/bazel/bazel.go +++ b/core/bazel/bazel.go @@ -55,7 +55,7 @@ type BazelClient struct { envVarsMap map[string]string bazelCommand string logger *zap.SugaredLogger - execCommandContext func(ctx context.Context, name string, arg ...string) commander + execCommandContext func(ctx context.Context, name string, arg ...string) Commander queryTimeout time.Duration tempDir string streamLogs bool @@ -66,7 +66,7 @@ type Params struct { WorkspacePath string EnvVarsMap map[string]string Logger *zap.SugaredLogger - ExecCommandContext func(ctx context.Context, name string, arg ...string) commander + ExecCommandContext func(ctx context.Context, name string, arg ...string) Commander QueryTimeout time.Duration StreamLogs bool } @@ -74,7 +74,7 @@ type Params struct { func NewBazelClient(ctx context.Context, p Params) (*BazelClient, error) { execCmd := p.ExecCommandContext if execCmd == nil { - execCmd = func(ctx context.Context, name string, arg ...string) commander { + execCmd = func(ctx context.Context, name string, arg ...string) Commander { cmd := execcmd.CommandContext(ctx, name, arg...) cmd.Dir = p.WorkspacePath for key, value := range p.EnvVarsMap { diff --git a/core/bazel/bazel_test.go b/core/bazel/bazel_test.go index 3999987b..12d3146a 100644 --- a/core/bazel/bazel_test.go +++ b/core/bazel/bazel_test.go @@ -36,7 +36,7 @@ func TestNewBazelClient(t *testing.T) { WorkspacePath: "/tmp/test", EnvVarsMap: map[string]string{"FOO": "bar"}, Logger: zap.NewNop().Sugar(), - ExecCommandContext: func(ctx context.Context, name string, arg ...string) commander { + ExecCommandContext: func(ctx context.Context, name string, arg ...string) Commander { return nil }, }, diff --git a/core/bazel/command.go b/core/bazel/command.go index f756374c..dddea87c 100644 --- a/core/bazel/command.go +++ b/core/bazel/command.go @@ -19,7 +19,8 @@ import ( "os/exec" ) -type commander interface { +// Commander runs a command with the supplied output destinations. +type Commander interface { Run(stdout, stderr io.Writer) error } diff --git a/core/bazel/commandermock/BUILD.bazel b/core/bazel/commandermock/BUILD.bazel deleted file mode 100644 index b90b729e..00000000 --- a/core/bazel/commandermock/BUILD.bazel +++ /dev/null @@ -1,9 +0,0 @@ -load("@rules_go//go:def.bzl", "go_library") - -go_library( - name = "commandermock", - srcs = ["commandermock.go"], - importpath = "github.com/uber/tango/core/bazel/commandermock", - visibility = ["//visibility:public"], - deps = ["@org_uber_go_mock//gomock"], -) diff --git a/core/bazel/commandermock/commandermock.go b/core/bazel/commandermock/commandermock.go deleted file mode 100644 index 54f2d463..00000000 --- a/core/bazel/commandermock/commandermock.go +++ /dev/null @@ -1,55 +0,0 @@ -// Code generated by MockGen. DO NOT EDIT. -// Source: core/bazel/command.go -// -// Generated by this command: -// -// mockgen -package=commandermock -destination=core/bazel/commandermock/commandermock.go -source=core/bazel/command.go commander -// - -// Package commandermock is a generated GoMock package. -package commandermock - -import ( - io "io" - reflect "reflect" - - gomock "go.uber.org/mock/gomock" -) - -// Mockcommander is a mock of commander interface. -type Mockcommander struct { - ctrl *gomock.Controller - recorder *MockcommanderMockRecorder - isgomock struct{} -} - -// MockcommanderMockRecorder is the mock recorder for Mockcommander. -type MockcommanderMockRecorder struct { - mock *Mockcommander -} - -// NewMockcommander creates a new mock instance. -func NewMockcommander(ctrl *gomock.Controller) *Mockcommander { - mock := &Mockcommander{ctrl: ctrl} - mock.recorder = &MockcommanderMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *Mockcommander) EXPECT() *MockcommanderMockRecorder { - return m.recorder -} - -// Run mocks base method. -func (m *Mockcommander) Run(stdout, stderr io.Writer) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Run", stdout, stderr) - ret0, _ := ret[0].(error) - return ret0 -} - -// Run indicates an expected call of Run. -func (mr *MockcommanderMockRecorder) Run(stdout, stderr any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Run", reflect.TypeOf((*Mockcommander)(nil).Run), stdout, stderr) -} diff --git a/core/bazel/query.go b/core/bazel/query.go index a5398a39..4e8594ce 100644 --- a/core/bazel/query.go +++ b/core/bazel/query.go @@ -28,7 +28,7 @@ import ( "google.golang.org/protobuf/proto" ) -func (b *BazelClient) setupCommand(ctx context.Context, query string, startupOptions []string, additionalArgs ...string) commander { +func (b *BazelClient) setupCommand(ctx context.Context, query string, startupOptions []string, additionalArgs ...string) Commander { // Build command: bazel query --output=streamed_proto args := make([]string, 0, len(startupOptions)+1+len(additionalArgs)+2) args = append(args, startupOptions...) diff --git a/core/bazel/query_test.go b/core/bazel/query_test.go index d3f129ab..14a34a02 100644 --- a/core/bazel/query_test.go +++ b/core/bazel/query_test.go @@ -26,24 +26,23 @@ import ( "time" buildpb "github.com/bazelbuild/buildtools/build_proto" + "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/uber/tango/core/bazel/commandermock" - "go.uber.org/mock/gomock" "go.uber.org/zap" "google.golang.org/protobuf/encoding/protodelim" ) func TestExecuteQuery_Success(t *testing.T) { ctrl := gomock.NewController(t) - mockCmd := commandermock.NewMockcommander(ctrl) + mockCmd := NewMockCommander(ctrl) protoData := marshalTarget(t, "//pkg:target") mockCmd.EXPECT().Run(gomock.Any(), gomock.Any()).DoAndReturn(func(stdout, _ io.Writer) error { _, err := stdout.Write(protoData) return err }) - client := newTestClient(t, func(context.Context, string, ...string) commander { return mockCmd }) + client := newTestClient(t, func(context.Context, string, ...string) Commander { return mockCmd }) resp, err := client.ExecuteQuery(context.Background(), &QueryRequest{Query: "//..."}) require.NoError(t, err) @@ -53,11 +52,11 @@ func TestExecuteQuery_Success(t *testing.T) { func TestExecuteQuery_WithStartupOptions(t *testing.T) { ctrl := gomock.NewController(t) - mockCmd := commandermock.NewMockcommander(ctrl) + mockCmd := NewMockCommander(ctrl) mockCmd.EXPECT().Run(gomock.Any(), gomock.Any()).Return(nil) var capturedArgs []string - client := newTestClient(t, func(_ context.Context, _ string, args ...string) commander { + client := newTestClient(t, func(_ context.Context, _ string, args ...string) Commander { capturedArgs = args return mockCmd }) @@ -80,14 +79,14 @@ func TestExecuteQuery_WithStartupOptions(t *testing.T) { func TestExecuteQueryInternal_ContextTimeout(t *testing.T) { ctrl := gomock.NewController(t) - mockCmd := commandermock.NewMockcommander(ctrl) + mockCmd := NewMockCommander(ctrl) var cmdCtx context.Context mockCmd.EXPECT().Run(gomock.Any(), gomock.Any()).DoAndReturn(func(io.Writer, io.Writer) error { <-cmdCtx.Done() return errors.New("signal: terminated") }) - client := newTestClient(t, func(ctx context.Context, _ string, _ ...string) commander { + client := newTestClient(t, func(ctx context.Context, _ string, _ ...string) Commander { cmdCtx = ctx return mockCmd }) @@ -101,8 +100,8 @@ func TestExecuteQueryInternal_ContextTimeout(t *testing.T) { func TestExecuteQueryInternal_PreCanceledContext(t *testing.T) { ctrl := gomock.NewController(t) - mockCmd := commandermock.NewMockcommander(ctrl) - client := newTestClient(t, func(context.Context, string, ...string) commander { return mockCmd }) + mockCmd := NewMockCommander(ctrl) + client := newTestClient(t, func(context.Context, string, ...string) Commander { return mockCmd }) ctx, cancel := context.WithCancel(context.Background()) cancel() @@ -114,7 +113,7 @@ func TestExecuteQueryInternal_PreCanceledContext(t *testing.T) { func TestExecuteQueryInternal_CancelDuringParsing(t *testing.T) { ctrl := gomock.NewController(t) - mockCmd := commandermock.NewMockcommander(ctrl) + mockCmd := NewMockCommander(ctrl) ctx, cancel := context.WithCancel(context.Background()) mockCmd.EXPECT().Run(gomock.Any(), gomock.Any()).DoAndReturn(func(stdout, _ io.Writer) error { _, err := stdout.Write(marshalTarget(t, "//pkg:target")) @@ -122,7 +121,7 @@ func TestExecuteQueryInternal_CancelDuringParsing(t *testing.T) { return err }) - client := newTestClient(t, func(context.Context, string, ...string) commander { return mockCmd }) + client := newTestClient(t, func(context.Context, string, ...string) Commander { return mockCmd }) result, err := client.executeQueryInternal(ctx, "//...", nil) require.ErrorIs(t, err, context.Canceled) @@ -131,7 +130,7 @@ func TestExecuteQueryInternal_CancelDuringParsing(t *testing.T) { func TestExecuteQueryInternal_CapturesCompleteOutput(t *testing.T) { ctrl := gomock.NewController(t) - mockCmd := commandermock.NewMockcommander(ctrl) + mockCmd := NewMockCommander(ctrl) const targetCount = 100 const stderrTail = "FINAL STDERR LINE" mockCmd.EXPECT().Run(gomock.Any(), gomock.Any()).DoAndReturn(func(stdout, stderr io.Writer) error { @@ -147,7 +146,7 @@ func TestExecuteQueryInternal_CapturesCompleteOutput(t *testing.T) { return errors.New("exit status 7") }) - client := newTestClient(t, func(context.Context, string, ...string) commander { return mockCmd }) + client := newTestClient(t, func(context.Context, string, ...string) Commander { return mockCmd }) result, err := client.executeQueryInternal(context.Background(), "//...", nil) require.Error(t, err) @@ -157,13 +156,13 @@ func TestExecuteQueryInternal_CapturesCompleteOutput(t *testing.T) { func TestExecuteQueryInternal_ParseFailure(t *testing.T) { ctrl := gomock.NewController(t) - mockCmd := commandermock.NewMockcommander(ctrl) + mockCmd := NewMockCommander(ctrl) mockCmd.EXPECT().Run(gomock.Any(), gomock.Any()).DoAndReturn(func(stdout, _ io.Writer) error { _, err := io.WriteString(stdout, "not a streamed proto") return err }) - client := newTestClient(t, func(context.Context, string, ...string) commander { return mockCmd }) + client := newTestClient(t, func(context.Context, string, ...string) Commander { return mockCmd }) result, err := client.executeQueryInternal(context.Background(), "//...", nil) require.Error(t, err) @@ -172,10 +171,10 @@ func TestExecuteQueryInternal_ParseFailure(t *testing.T) { func TestExecuteQueryInternal_StreamLogs(t *testing.T) { ctrl := gomock.NewController(t) - mockCmd := commandermock.NewMockcommander(ctrl) + mockCmd := NewMockCommander(ctrl) mockCmd.EXPECT().Run(gomock.Any(), os.Stderr).Return(errors.New("exit status 1")) - client := newTestClient(t, func(context.Context, string, ...string) commander { return mockCmd }) + client := newTestClient(t, func(context.Context, string, ...string) Commander { return mockCmd }) client.streamLogs = true _, err := client.executeQueryInternal(context.Background(), "//...", nil) require.Error(t, err) @@ -183,10 +182,10 @@ func TestExecuteQueryInternal_StreamLogs(t *testing.T) { func TestExecuteQuery_ErrorCase(t *testing.T) { ctrl := gomock.NewController(t) - mockCmd := commandermock.NewMockcommander(ctrl) + mockCmd := NewMockCommander(ctrl) mockCmd.EXPECT().Run(gomock.Any(), gomock.Any()).Return(errors.New("command failed")) - client := newTestClient(t, func(context.Context, string, ...string) commander { return mockCmd }) + client := newTestClient(t, func(context.Context, string, ...string) Commander { return mockCmd }) resp, err := client.ExecuteQuery(context.Background(), &QueryRequest{Query: "//..."}) require.Error(t, err) @@ -207,7 +206,7 @@ func marshalTarget(t *testing.T, name string) []byte { return out.Bytes() } -func newTestClient(t *testing.T, execCmd func(context.Context, string, ...string) commander) *BazelClient { +func newTestClient(t *testing.T, execCmd func(context.Context, string, ...string) Commander) *BazelClient { t.Helper() client, err := NewBazelClient(context.Background(), Params{ BazelCommand: "bazel", From 705ebe6756dddc19059637c3737a3bf0eddf6fce Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Wed, 8 Jul 2026 19:02:03 -0700 Subject: [PATCH 5/5] still use goleak for goroutine leak checks --- core/bazel/BUILD.bazel | 1 + core/bazel/command_test.go | 3 +++ core/bazel/query_test.go | 3 +++ 3 files changed, 7 insertions(+) diff --git a/core/bazel/BUILD.bazel b/core/bazel/BUILD.bazel index 2cc80972..0b8a3df3 100644 --- a/core/bazel/BUILD.bazel +++ b/core/bazel/BUILD.bazel @@ -36,6 +36,7 @@ go_test( "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", "@org_golang_google_protobuf//encoding/protodelim", + "@org_uber_go_goleak//:goleak", "@org_uber_go_zap//:zap", ], ) diff --git a/core/bazel/command_test.go b/core/bazel/command_test.go index 160195f4..d5bcdd4e 100644 --- a/core/bazel/command_test.go +++ b/core/bazel/command_test.go @@ -28,9 +28,11 @@ import ( "github.com/stretchr/testify/require" "github.com/uber/tango/core/execcmd" + "go.uber.org/goleak" ) func TestExecCommander_OutputFilesDoNotWaitForDescendant(t *testing.T) { + defer goleak.VerifyNone(t) stdout, err := os.CreateTemp(t.TempDir(), "stdout-*") require.NoError(t, err) defer stdout.Close() @@ -54,6 +56,7 @@ func TestExecCommander_OutputFilesDoNotWaitForDescendant(t *testing.T) { } func TestExecCommander_ContextCancellation(t *testing.T) { + defer goleak.VerifyNone(t) ctx, cancel := context.WithCancel(context.Background()) defer cancel() readyReader, readyWriter, err := os.Pipe() diff --git a/core/bazel/query_test.go b/core/bazel/query_test.go index 14a34a02..de96a06e 100644 --- a/core/bazel/query_test.go +++ b/core/bazel/query_test.go @@ -29,6 +29,7 @@ import ( "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/goleak" "go.uber.org/zap" "google.golang.org/protobuf/encoding/protodelim" ) @@ -78,6 +79,7 @@ func TestExecuteQuery_WithStartupOptions(t *testing.T) { } func TestExecuteQueryInternal_ContextTimeout(t *testing.T) { + defer goleak.VerifyNone(t) ctrl := gomock.NewController(t) mockCmd := NewMockCommander(ctrl) var cmdCtx context.Context @@ -112,6 +114,7 @@ func TestExecuteQueryInternal_PreCanceledContext(t *testing.T) { } func TestExecuteQueryInternal_CancelDuringParsing(t *testing.T) { + defer goleak.VerifyNone(t) ctrl := gomock.NewController(t) mockCmd := NewMockCommander(ctrl) ctx, cancel := context.WithCancel(context.Background())