Properly report errors when builtin output fails

This correctly sets $status when a builtin succeeds but its output fails;
for example if the output is redirected to a file and that write fails.

Fixes #7857

CHANGELOG.rst

@@ -18,6 +18,7 @@ Scripting improvements
 - Exit codes are better aligned with bash. A failed exec now reports ``$status`` of 127 if the file is not found, and 126 if it is not executable.
 - ``echo`` no longer writes its output one byte at a time, improving performance and allowing use with linux' special API files (``/proc``, ``/sys`` and such) (:issue:`7836`).
 - fish should now better handle ``cd`` on filesystems with broken ``stat(3)`` responses (:issue:`7577`).
+- Builtins now properly report a ``$status`` of 1 upon unsuccessful writes (:issue:`7857`).
 
 Interactive improvements
 -------------------------

src/builtin.cpp

@@ -469,20 +469,29 @@ proc_status_t builtin_run(parser_t &parser, const wcstring_list_t &argv, io_stre
     }
 
     if (const builtin_data_t *data = builtin_lookup(cmdname)) {
-        // Construct the permutable argv array which the builtin expects.
+        // Construct the permutable argv array which the builtin expects, and execute the builtin.
         null_terminated_array_t<wchar_t> argv_arr(argv);
-        maybe_t<int> ret = data->func(parser, streams, argv_arr.get());
+        maybe_t<int> builtin_ret = data->func(parser, streams, argv_arr.get());
-        if (!ret) {
+
-            return proc_status_t::empty();
+        // Flush our out and error streams, and check for their errors.
-        }
+        int out_ret = streams.out.flush_and_check_error();
+        int err_ret = streams.err.flush_and_check_error();
+
+        // Resolve our status code.
+        // If the builtin itself produced an error, use that error.
+        // Otherwise use any errors from writing to out and writing to err, in that order.
+        int code = builtin_ret ? *builtin_ret : 0;
+        if (code == 0) code = out_ret;
+        if (code == 0) code = err_ret;
 
         // The exit code is cast to an 8-bit unsigned integer, so saturate to 255. Otherwise,
         // multiples of 256 are reported as 0.
-        int code = ret.value();
+        if (code > 255) code = 255;
-        if (code > 255) {
-            code = 255;
-        }
 
+        // Handle the case of an empty status.
+        if (code == 0 && !builtin_ret.has_value()) {
+            return proc_status_t::empty();
+        }
         return proc_status_t::from_exit_code(code);
     }
 

src/exec.cpp

@@ -502,11 +502,6 @@ static void handle_builtin_output(parser_t &parser, const std::shared_ptr<job_t>
                                   const output_stream_t &err) {
     assert(p->type == process_type_t::builtin && "Process is not a builtin");
 
-    // Mark if we discarded output.
-    if (out.discarded() || err.discarded()) {
-        p->status = proc_status_t::from_exit_code(STATUS_READ_TOO_MUCH);
-    }
-
     // Figure out any data remaining to write. We may have none, in which case we can short-circuit.
     std::string outbuff = wcs2string(out.contents());
     std::string errbuff = wcs2string(err.contents());

src/io.cpp

@@ -299,6 +299,8 @@ void output_stream_t::append_with_separation(const wchar_t *s, size_t len, separ
 
 const wcstring &output_stream_t::contents() const { return g_empty_string; }
 
+int output_stream_t::flush_and_check_error() { return STATUS_CMD_OK; }
+
 void fd_output_stream_t::append(const wchar_t *s, size_t amt) {
     if (errored_) return;
     int res = wwrite_to_fd(s, amt, this->fd_);
@@ -309,6 +311,11 @@ void fd_output_stream_t::append(const wchar_t *s, size_t amt) {
     }
 }
 
+int fd_output_stream_t::flush_and_check_error() {
+    // Return a generic 1 on any write failure.
+    return errored_ ? STATUS_CMD_ERROR : STATUS_CMD_OK;
+}
+
 void null_output_stream_t::append(const wchar_t *, size_t) {}
 
 void string_output_stream_t::append(const wchar_t *s, size_t amt) { contents_.append(s, amt); }
@@ -324,4 +331,9 @@ void buffered_output_stream_t::append_with_separation(const wchar_t *s, size_t l
     buffer_->append(wcs2string(s, len), type);
 }
 
-bool buffered_output_stream_t::discarded() const { return buffer_->discarded(); }
+int buffered_output_stream_t::flush_and_check_error() {
+    if (buffer_->discarded()) {
+        return STATUS_READ_TOO_MUCH;
+    }
+    return 0;
+}

src/io.h

@@ -363,14 +363,15 @@ class output_stream_t {
     /// Required override point. The output stream receives a string \p s with \p amt chars.
     virtual void append(const wchar_t *s, size_t amt) = 0;
 
-    /// \return true if output was discarded. This only applies to buffered output streams.
-    virtual bool discarded() const { return false; }
-
     /// \return any internally buffered contents.
     /// This is only implemented for a string_output_stream; others flush data to their underlying
     /// receiver (fd, or separated buffer) immediately and so will return an empty string here.
     virtual const wcstring &contents() const;
 
+    /// Flush any unwritten data to the underlying device, and return an error code.
+    /// A 0 code indicates success. The base implementation returns 0.
+    virtual int flush_and_check_error();
+
     /// An optional override point. This is for explicit separation.
     virtual void append_with_separation(const wchar_t *s, size_t len, separation_type_t type);
 
@@ -420,6 +421,8 @@ class fd_output_stream_t final : public output_stream_t {
     /// Construct from a file descriptor, which must be nonegative.
     explicit fd_output_stream_t(int fd) : fd_(fd) { assert(fd_ >= 0 && "Invalid fd"); }
 
+    int flush_and_check_error() override;
+
     void append(const wchar_t *s, size_t amt) override;
 
    private:
@@ -453,7 +456,7 @@ class buffered_output_stream_t final : public output_stream_t {
 
     void append(const wchar_t *s, size_t amt) override;
     void append_with_separation(const wchar_t *s, size_t len, separation_type_t type) override;
-    bool discarded() const override;
+    int flush_and_check_error() override;
 
    private:
     /// The buffer we are filling.

tests/checks/status.fish

@@ -80,3 +80,28 @@ if false
 end
 echo $status
 #CHECK: 0
+
+# Verify errors from writes - see #7857.
+if test -e /dev/full
+    # Failed writes to stdout produce 1.
+    echo foo > /dev/full
+    if test $status -ne 1
+        echo "Wrong status when writing to /dev/full"
+    end
+
+   # Here the builtin should fail with status 2,
+   # and also the write should fail with status 1.
+   # The builtin has precedence.
+   builtin string --not-a-valid-option 2> /dev/full
+   if test $status -ne 2
+       echo "Wrong status for failing builtin"
+   end
+   echo "Failed write tests finished"
+else
+    echo "Failed write tests skipped"
+    echo "write: skipped" 1>&2
+    echo "write: skipped" 1>&2
+end
+# CHECK: Failed write tests {{finished|skipped}}
+# CHECKERR: write: {{.*}}
+# CHECKERR: write: {{.*}}