From d0085cae3cd824f5598dfda780fae428fcfcafbd Mon Sep 17 00:00:00 2001
From: Mahmoud Al-Qudsi <mqudsi@neosmart.net>
Date: Sun, 18 Nov 2018 17:14:08 -0600
Subject: [PATCH] Fix zombie job on failed redirection in `exec_job`

Closes #5346.
---
 src/exec.cpp            | 21 ++++++++++-----------
 src/exec.h              |  2 +-
 src/parse_execution.cpp |  4 +++-
 3 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/src/exec.cpp b/src/exec.cpp
index 247b3a4fc..1c93c0e07 100644
--- a/src/exec.cpp
+++ b/src/exec.cpp
@@ -990,16 +990,16 @@ static bool exec_process_in_job(parser_t &parser, process_t *p, std::shared_ptr<
     return true;
 }
 
-void exec_job(parser_t &parser, shared_ptr<job_t> j) {
+bool exec_job(parser_t &parser, shared_ptr<job_t> j) {
     assert(j && "null job_t passed to exec_job!");
 
-    // Set to true if something goes wrong while exec:ing the job, in which case the cleanup code
-    // will kick in.
+    // Set to true if something goes wrong while executing the job, in which case the cleanup
+    // code will kick in.
     bool exec_error = false;
 
     // If fish was invoked with -n or --no-execute, then no_exec will be set and we do nothing.
     if (no_exec) {
-        return;
+        return true;
     }
 
     const std::shared_ptr<job_t> parent_job = j->get_parent();
@@ -1059,7 +1059,7 @@ void exec_job(parser_t &parser, shared_ptr<job_t> j) {
     // 3. The pipe that the next process should read from (courtesy of us)
     //
     autoclose_fd_t pipe_next_read;
-    for (std::unique_ptr<process_t> &unique_p : j->processes) {
+    for (const auto &unique_p : j->processes) {
         autoclose_fd_t current_read = std::move(pipe_next_read);
         if (!exec_process_in_job(parser, unique_p.get(), j, std::move(current_read),
                                  &pipe_next_read, all_ios, stdout_read_limit)) {
@@ -1077,13 +1077,12 @@ void exec_job(parser_t &parser, shared_ptr<job_t> j) {
         env_set_one(L"last_pid", ENV_GLOBAL, to_string(j->pgid));
     }
 
-    if (!exec_error) {
-        j->continue_job(false);
-    } else {
-        // Mark the errored job as not in the foreground. I can't fully justify whether this is the
-        // right place, but it prevents sanity_lose from complaining.
-        j->set_flag(job_flag_t::FOREGROUND, false);
+    if (exec_error) {
+        return false;
     }
+
+    j->continue_job(false);
+    return true;
 }
 
 static int exec_subshell_internal(const wcstring &cmd, wcstring_list_t *lst, bool apply_exit_status,
diff --git a/src/exec.h b/src/exec.h
index e244e8212..5e04fe3e5 100644
--- a/src/exec.h
+++ b/src/exec.h
@@ -14,7 +14,7 @@
 /// Execute the processes specified by \p j in the parser \p.
 class job_t;
 class parser_t;
-void exec_job(parser_t &parser, std::shared_ptr<job_t> j);
+bool exec_job(parser_t &parser, std::shared_ptr<job_t> j);
 
 /// Evaluate the expression cmd in a subshell, add the outputs into the list l. On return, the
 /// status flag as returned bu \c proc_gfet_last_status will not be changed.
diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp
index 376c87eac..48d0134a0 100644
--- a/src/parse_execution.cpp
+++ b/src/parse_execution.cpp
@@ -1232,7 +1232,9 @@ parse_execution_result_t parse_execution_context_t::run_1_job(tnode_t<g::job> jo
         }
 
         // Actually execute the job.
-        exec_job(*this->parser, job);
+        if (!exec_job(*this->parser, job)) {
+            parser->job_remove(job.get());
+        }
 
         // Only external commands require a new fishd barrier.
         if (job_contained_external_command) {