Merge pull request #22548 from mlugg/fix-broken-pipe

Wait for reported spawn success or failure before trying to write to the stdio pipe in the build runner.

Hopefully fixes `error.BrokenPipe` failures

lib/std/Build.zig

@@ -979,7 +979,7 @@ pub const TestOptions = struct {
     /// Deprecated; use `.filters = &.{filter}` instead of `.filter = filter`.
     filter: ?[]const u8 = null,
     filters: []const []const u8 = &.{},
-    test_runner: ?LazyPath = null,
+    test_runner: ?Step.Compile.TestRunner = null,
     use_llvm: ?bool = null,
     use_lld: ?bool = null,
     zig_lib_dir: ?LazyPath = null,
@@ -1136,9 +1136,8 @@ pub fn addRunArtifact(b: *Build, exe: *Step.Compile) *Step.Run {
             run_step.addArtifactArg(exe);
         }
 
-        if (exe.test_server_mode) {
-            run_step.enableTestRunnerMode();
-        }
+        const test_server_mode = if (exe.test_runner) |r| r.mode == .server else true;
+        if (test_server_mode) run_step.enableTestRunnerMode();
     } else {
         run_step.addArtifactArg(exe);
     }

lib/std/Build/Step/Compile.zig

@@ -56,8 +56,7 @@ global_base: ?u64 = null,
 zig_lib_dir: ?LazyPath,
 exec_cmd_args: ?[]const ?[]const u8,
 filters: []const []const u8,
-test_runner: ?LazyPath,
-test_server_mode: bool,
+test_runner: ?TestRunner,
 wasi_exec_model: ?std.builtin.WasiExecModel = null,
 
 installed_headers: ArrayList(HeaderInstallation),
@@ -268,7 +267,7 @@ pub const Options = struct {
     version: ?std.SemanticVersion = null,
     max_rss: usize = 0,
     filters: []const []const u8 = &.{},
-    test_runner: ?LazyPath = null,
+    test_runner: ?TestRunner = null,
     use_llvm: ?bool = null,
     use_lld: ?bool = null,
     zig_lib_dir: ?LazyPath = null,
@@ -347,6 +346,14 @@ pub const HeaderInstallation = union(enum) {
     }
 };
 
+pub const TestRunner = struct {
+    path: LazyPath,
+    /// Test runners can either be "simple", running tests when spawned and terminating when the
+    /// tests are complete, or they can use `std.zig.Server` over stdio to interact more closely
+    /// with the build system.
+    mode: enum { simple, server },
+};
+
 pub fn create(owner: *std.Build, options: Options) *Compile {
     const name = owner.dupe(options.name);
     if (mem.indexOf(u8, name, "/") != null or mem.indexOf(u8, name, "\\") != null) {
@@ -411,8 +418,7 @@ pub fn create(owner: *std.Build, options: Options) *Compile {
         .zig_lib_dir = null,
         .exec_cmd_args = null,
         .filters = options.filters,
-        .test_runner = null,
-        .test_server_mode = options.test_runner == null,
+        .test_runner = null, // set below
         .rdynamic = false,
         .installed_path = null,
         .force_undefined_symbols = StringHashMap(void).init(owner.allocator),
@@ -438,9 +444,12 @@ pub fn create(owner: *std.Build, options: Options) *Compile {
         lp.addStepDependencies(&compile.step);
     }
 
-    if (options.test_runner) |lp| {
-        compile.test_runner = lp.dupe(compile.step.owner);
-        lp.addStepDependencies(&compile.step);
+    if (options.test_runner) |runner| {
+        compile.test_runner = .{
+            .path = runner.path.dupe(compile.step.owner),
+            .mode = runner.mode,
+        };
+        runner.path.addStepDependencies(&compile.step);
     }
 
     // Only the PE/COFF format has a Resource Table which is where the manifest
@@ -1399,7 +1408,7 @@ fn getZigArgs(compile: *Compile, fuzz: bool) ![][]const u8 {
 
     if (compile.test_runner) |test_runner| {
         try zig_args.append("--test-runner");
-        try zig_args.append(test_runner.getPath2(b, step));
+        try zig_args.append(test_runner.path.getPath2(b, step));
     }
 
     for (b.debug_log_scopes) |log_scope| {

lib/std/Build/Step/Run.zig

@@ -1354,6 +1354,9 @@ fn spawnChildAndCollect(
             _ = child.kill() catch {};
         }
 
+        // We need to report `error.InvalidExe` *now* if applicable.
+        try child.waitForSpawn();
+
         var timer = try std.time.Timer.start();
 
         const result = if (run.stdio == .zig_test)

lib/std/process/Child.zig

@@ -73,7 +73,7 @@ cwd: ?[]const u8,
 /// Once that is done, `cwd` will be deprecated in favor of this field.
 cwd_dir: ?fs.Dir = null,
 
-err_pipe: ?if (native_os == .windows) void else [2]posix.fd_t,
+err_pipe: if (native_os == .windows) void else ?posix.fd_t,
 
 expand_arg0: Arg0Expand,
 
@@ -211,7 +211,7 @@ pub fn init(argv: []const []const u8, allocator: mem.Allocator) ChildProcess {
         .argv = argv,
         .id = undefined,
         .thread_handle = undefined,
-        .err_pipe = null,
+        .err_pipe = if (native_os == .windows) {} else null,
         .term = null,
         .env_map = null,
         .cwd = null,
@@ -293,17 +293,49 @@ pub fn killPosix(self: *ChildProcess) !Term {
         error.ProcessNotFound => return error.AlreadyTerminated,
         else => return err,
     };
-    self.waitUnwrapped();
+    self.waitUnwrappedPosix();
     return self.term.?;
 }
 
 pub const WaitError = SpawnError || std.os.windows.GetProcessMemoryInfoError;
 
+/// On some targets, `spawn` may not report all spawn errors, such as `error.InvalidExe`.
+/// This function will block until any spawn errors can be reported, and return them.
+pub fn waitForSpawn(self: *ChildProcess) SpawnError!void {
+    if (native_os == .windows) return; // `spawn` reports everything
+    if (self.term) |term| {
+        _ = term catch |spawn_err| return spawn_err;
+        return;
+    }
+
+    const err_pipe = self.err_pipe orelse return;
+    self.err_pipe = null;
+
+    // Wait for the child to report any errors in or before `execvpe`.
+    if (readIntFd(err_pipe)) |child_err_int| {
+        posix.close(err_pipe);
+        const child_err: SpawnError = @errorCast(@errorFromInt(child_err_int));
+        self.term = child_err;
+        return child_err;
+    } else |_| {
+        // Write end closed by CLOEXEC at the time of the `execvpe` call, indicating success!
+        posix.close(err_pipe);
+    }
+}
+
 /// Blocks until child process terminates and then cleans up all resources.
 pub fn wait(self: *ChildProcess) WaitError!Term {
-    const term = if (native_os == .windows) try self.waitWindows() else self.waitPosix();
+    try self.waitForSpawn(); // report spawn errors
+    if (self.term) |term| {
+        self.cleanupStreams();
+        return term;
+    }
+    switch (native_os) {
+        .windows => try self.waitUnwrappedWindows(),
+        else => self.waitUnwrappedPosix(),
+    }
     self.id = undefined;
-    return term;
+    return self.term.?;
 }
 
 pub const RunResult = struct {
@@ -405,26 +437,6 @@ pub fn run(args: struct {
     };
 }
 
-fn waitWindows(self: *ChildProcess) WaitError!Term {
-    if (self.term) |term| {
-        self.cleanupStreams();
-        return term;
-    }
-
-    try self.waitUnwrappedWindows();
-    return self.term.?;
-}
-
-fn waitPosix(self: *ChildProcess) SpawnError!Term {
-    if (self.term) |term| {
-        self.cleanupStreams();
-        return term;
-    }
-
-    self.waitUnwrapped();
-    return self.term.?;
-}
-
 fn waitUnwrappedWindows(self: *ChildProcess) WaitError!void {
     const result = windows.WaitForSingleObjectEx(self.id, windows.INFINITE, false);
 
@@ -447,7 +459,7 @@ fn waitUnwrappedWindows(self: *ChildProcess) WaitError!void {
     return result;
 }
 
-fn waitUnwrapped(self: *ChildProcess) void {
+fn waitUnwrappedPosix(self: *ChildProcess) void {
     const res: posix.WaitPidResult = res: {
         if (self.request_resource_usage_statistics) {
             switch (native_os) {
@@ -469,7 +481,7 @@ fn waitUnwrapped(self: *ChildProcess) void {
 }
 
 fn handleWaitResult(self: *ChildProcess, status: u32) void {
-    self.term = self.cleanupAfterWait(status);
+    self.term = statusToTerm(status);
 }
 
 fn cleanupStreams(self: *ChildProcess) void {
@@ -487,46 +499,6 @@ fn cleanupStreams(self: *ChildProcess) void {
     }
 }
 
-fn cleanupAfterWait(self: *ChildProcess, status: u32) !Term {
-    if (self.err_pipe) |err_pipe| {
-        defer destroyPipe(err_pipe);
-
-        if (native_os == .linux) {
-            var fd = [1]posix.pollfd{posix.pollfd{
-                .fd = err_pipe[0],
-                .events = posix.POLL.IN,
-                .revents = undefined,
-            }};
-
-            // Check if the eventfd buffer stores a non-zero value by polling
-            // it, that's the error code returned by the child process.
-            _ = posix.poll(&fd, 0) catch unreachable;
-
-            // According to eventfd(2) the descriptor is readable if the counter
-            // has a value greater than 0
-            if ((fd[0].revents & posix.POLL.IN) != 0) {
-                const err_int = try readIntFd(err_pipe[0]);
-                return @as(SpawnError, @errorCast(@errorFromInt(err_int)));
-            }
-        } else {
-            // Write maxInt(ErrInt) to the write end of the err_pipe. This is after
-            // waitpid, so this write is guaranteed to be after the child
-            // pid potentially wrote an error. This way we can do a blocking
-            // read on the error pipe and either get maxInt(ErrInt) (no error) or
-            // an error code.
-            try writeIntFd(err_pipe[1], maxInt(ErrInt));
-            const err_int = try readIntFd(err_pipe[0]);
-            // Here we potentially return the fork child's error from the parent
-            // pid.
-            if (err_int != maxInt(ErrInt)) {
-                return @as(SpawnError, @errorCast(@errorFromInt(err_int)));
-            }
-        }
-    }
-
-    return statusToTerm(status);
-}
-
 fn statusToTerm(status: u32) Term {
     return if (posix.W.IFEXITED(status))
         Term{ .Exited = posix.W.EXITSTATUS(status) }
@@ -636,18 +608,9 @@ fn spawnPosix(self: *ChildProcess) SpawnError!void {
         }
     };
 
-    // This pipe is used to communicate errors between the time of fork
-    // and execve from the child process to the parent process.
-    const err_pipe = blk: {
-        if (native_os == .linux) {
-            const fd = try posix.eventfd(0, linux.EFD.CLOEXEC);
-            // There's no distinction between the readable and the writeable
-            // end with eventfd
-            break :blk [2]posix.fd_t{ fd, fd };
-        } else {
-            break :blk try posix.pipe2(.{ .CLOEXEC = true });
-        }
-    };
+    // This pipe communicates to the parent errors in the child between `fork` and `execvpe`.
+    // It is closed by the child (via CLOEXEC) without writing if `execvpe` succeeds.
+    const err_pipe: [2]posix.fd_t = try posix.pipe2(.{ .CLOEXEC = true });
     errdefer destroyPipe(err_pipe);
 
     const pid_result = try posix.fork();
@@ -687,6 +650,11 @@ fn spawnPosix(self: *ChildProcess) SpawnError!void {
     }
 
     // we are the parent
+    errdefer comptime unreachable; // The child is forked; we must not error from now on
+
+    posix.close(err_pipe[1]); // make sure only the child holds the write end open
+    self.err_pipe = err_pipe[0];
+
     const pid: i32 = @intCast(pid_result);
     if (self.stdin_behavior == .Pipe) {
         self.stdin = .{ .handle = stdin_pipe[1] };
@@ -705,7 +673,6 @@ fn spawnPosix(self: *ChildProcess) SpawnError!void {
     }
 
     self.id = pid;
-    self.err_pipe = err_pipe;
     self.term = null;
 
     if (self.stdin_behavior == .Pipe) {

test/standalone/test_runner_module_imports/build.zig

@@ -13,7 +13,10 @@ pub fn build(b: *std.Build) void {
 
     const t = b.addTest(.{
         .root_module = test_mod,
-        .test_runner = b.path("test_runner/main.zig"),
+        .test_runner = .{
+            .path = b.path("test_runner/main.zig"),
+            .mode = .simple,
+        },
     });
 
     const test_step = b.step("test", "Run unit tests");

test/standalone/test_runner_path/build.zig

@@ -8,7 +8,10 @@ pub fn build(b: *std.Build) void {
         .target = b.graph.host,
         .root_source_file = b.path("test.zig"),
     }) });
-    test_exe.test_runner = b.path("test_runner.zig");
+    test_exe.test_runner = .{
+        .path = b.path("test_runner.zig"),
+        .mode = .simple,
+    };
 
     const test_run = b.addRunArtifact(test_exe);
     test_step.dependOn(&test_run.step);