Skip to content

Commit c3d94a5

Browse files
committed
Fix pipe read race in exec_service server
SetReadDeadline(time.Now()) can fire before the read loop consumes buffered pipe data. Go's poller checks the deadline before attempting syscall.Read, so ErrDeadlineExceeded is returned and buffered output is silently dropped. Fix: kill the process group after cmd.Wait() so background children close their inherited pipe fds, letting the read loop reach EOF without any deadline. A 100ms deadline fallback handles the rare case of children that escaped the process group (e.g. via setsid). Reproducer (fails 10/10 without the fix): bazel test //exec_service/cmd/exec_client:exec_client_test \ --test_arg=-test.run=TestStderrOutput \ --test_arg=-test.count=500 \ --test_arg=-test.cpu=1 \ --runs_per_test=10 --local_test_jobs=10
1 parent 14c6373 commit c3d94a5

File tree

1 file changed

+8
-4
lines changed

1 file changed

+8
-4
lines changed

exec_service/server/server.go

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -61,13 +61,17 @@ func (s *ExecServer) RunCommand(req *pb.StartCommandRequest, stream pb.ExecServi
6161
syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL)
6262
}
6363

64-
// Wait for the command concurrently. When it exits, expire the pipe
65-
// read deadline so the read loop drains buffered output without
66-
// blocking on background children that inherited the pipe fds.
64+
// Wait for the command concurrently. When it exits, kill the
65+
// process group so that background children close their inherited
66+
// pipe fds, allowing the read loop to reach EOF cleanly. The
67+
// deadline is a fallback for children that escaped the group
68+
// (e.g. via setsid); it is not reached in the normal case
69+
// because SIGKILL closes the pipe before the deadline fires.
6770
waitCh := make(chan error, 1)
6871
go func() {
6972
err := cmd.Wait()
70-
pr.SetReadDeadline(time.Now())
73+
killGroup()
74+
pr.SetReadDeadline(time.Now().Add(100 * time.Millisecond))
7175
waitCh <- err
7276
}()
7377

0 commit comments

Comments
 (0)