Teach a job to decide its job tree

Job trees come in two flavors: “placeholders” for jobs which are only fish
functions, and non-placeholders which need to track a pgid. This adds
logic to allow a job to decide if its parent's job tree is appropriate,
and allocating a new tree if not.

src/builtin_eval.cpp

@@ -59,7 +59,7 @@ int builtin_eval(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
     }
 
     int status = STATUS_CMD_OK;
-    auto res = parser.eval(new_cmd, ios, streams.parent_pgid);
+    auto res = parser.eval(new_cmd, ios, streams.job_tree);
     if (res.was_empty) {
         // Issue #5692, in particular, to catch `eval ""`, `eval "begin; end;"`, etc.
         // where we have an argument but nothing is executed.

src/exec.cpp

@@ -59,7 +59,7 @@ pgroup_provenance_t get_pgroup_provenance(const shared_ptr<job_t> &j,
     bool has_external = j->has_external_proc();
     assert(first_proc_is_internal ? has_internal : has_external);
 
-    if (lineage.parent_pgid.has_value() && j->is_foreground()) {
+    if (lineage.job_tree && lineage.job_tree->get_pgid().has_value() && j->is_foreground()) {
         // Our lineage indicates a pgid. This means the job is "nested" as a function or block
         // inside another job, which has a real pgroup. We're going to use that, unless it's
         // backgrounded, in which case it should not inherit a pgroup.
@@ -209,6 +209,7 @@ static void maybe_assign_pgid_from_child(const std::shared_ptr<job_t> &j, pid_t
     // If our assignment mode is the first process, then assign it.
     if (j->pgid == INVALID_PID &&
         j->pgroup_provenance == pgroup_provenance_t::first_external_proc) {
+        j->job_tree->set_pgid(child_pid);
         j->pgid = child_pid;
     }
 }
@@ -659,7 +660,7 @@ static proc_performer_t get_performer_for_process(process_t *p, job_t *job,
            "Unexpected process type");
     // Make a lineage for our children.
     job_lineage_t lineage;
-    lineage.parent_pgid = (job->pgid == INVALID_PID ? none() : maybe_t<pid_t>(job->pgid));
+    lineage.job_tree = job->job_tree;
     lineage.block_io = io_chain;
     lineage.root_constructed = job->root_constructed;
     lineage.root_has_job_control = job->wants_job_control();
@@ -862,7 +863,7 @@ static bool exec_process_in_job(parser_t &parser, process_t *p, const std::share
 
         case process_type_t::builtin: {
             io_streams_t builtin_io_streams{stdout_read_limit};
-            if (j->pgid != INVALID_PID) builtin_io_streams.parent_pgid = j->pgid;
+            builtin_io_streams.job_tree = j->job_tree;
             if (!exec_internal_builtin_proc(parser, p, pipe_read.get(), process_net_io_chain,
                                             builtin_io_streams)) {
                 return false;
@@ -956,13 +957,19 @@ bool exec_job(parser_t &parser, const shared_ptr<job_t> &j, const job_lineage_t
     // Perhaps we know our pgroup already.
     assert(j->pgid == INVALID_PID && "Should not yet have a pid.");
     switch (j->pgroup_provenance) {
-        case pgroup_provenance_t::lineage:
-            assert(*lineage.parent_pgid != INVALID_PID && "pgid should be none, not INVALID_PID");
-            j->pgid = *lineage.parent_pgid;
+        case pgroup_provenance_t::lineage: {
+            auto pgid = lineage.job_tree->get_pgid();
+            assert(pgid && *pgid != INVALID_PID && "Should have valid pgid");
+            j->pgid = *pgid;
             break;
+        }
 
         case pgroup_provenance_t::fish_itself:
             j->pgid = pgrp;
+            // TODO: should not need this 'if' here. Rationalize this.
+            if (! j->job_tree->is_placeholder()) {
+                j->job_tree->set_pgid(pgrp);
+            }
             break;
 
         // The remaining cases are all deferred until later.
@@ -1114,15 +1121,15 @@ static void populate_subshell_output(wcstring_list_t *lst, const io_buffer_t &bu
 
 /// Execute \p cmd in a subshell in \p parser. If \p lst is not null, populate it with the output.
 /// Return $status in \p out_status.
-/// If \p parent_pgid is set, any spawned commands should join that pgroup.
+/// If \p job_tree is set, any spawned commands should join that job tree.
 /// If \p apply_exit_status is false, then reset $status back to its original value.
 /// \p is_subcmd controls whether we apply a read limit.
 /// \p break_expand is used to propagate whether the result should be "expansion breaking" in the
 /// sense that subshells used during string expansion should halt that expansion. \return the value
 /// of $status.
-static int exec_subshell_internal(const wcstring &cmd, parser_t &parser, maybe_t<pid_t> parent_pgid,
-                                  wcstring_list_t *lst, bool *break_expand, bool apply_exit_status,
-                                  bool is_subcmd) {
+static int exec_subshell_internal(const wcstring &cmd, parser_t &parser,
+                                  const job_tree_ref_t &job_tree, wcstring_list_t *lst,
+                                  bool *break_expand, bool apply_exit_status, bool is_subcmd) {
     ASSERT_IS_MAIN_THREAD();
     auto &ld = parser.libdata();
 
@@ -1145,8 +1152,7 @@ static int exec_subshell_internal(const wcstring &cmd, parser_t &parser, maybe_t
         *break_expand = true;
         return STATUS_CMD_ERROR;
     }
-    eval_res_t eval_res =
-        parser.eval(cmd, io_chain_t{bufferfill}, parent_pgid, block_type_t::subst);
+    eval_res_t eval_res = parser.eval(cmd, io_chain_t{bufferfill}, job_tree, block_type_t::subst);
     std::shared_ptr<io_buffer_t> buffer = io_bufferfill_t::finish(std::move(bufferfill));
     if (buffer->buffer().discarded()) {
         *break_expand = true;
@@ -1165,24 +1171,24 @@ static int exec_subshell_internal(const wcstring &cmd, parser_t &parser, maybe_t
     return eval_res.status.status_value();
 }
 
-int exec_subshell_for_expand(const wcstring &cmd, parser_t &parser, maybe_t<pid_t> parent_pgid,
+int exec_subshell_for_expand(const wcstring &cmd, parser_t &parser, const job_tree_ref_t &job_tree,
                              wcstring_list_t &outputs) {
     ASSERT_IS_MAIN_THREAD();
     bool break_expand = false;
-    int ret = exec_subshell_internal(cmd, parser, parent_pgid, &outputs, &break_expand, true, true);
+    int ret = exec_subshell_internal(cmd, parser, job_tree, &outputs, &break_expand, true, true);
     // Only return an error code if we should break expansion.
     return break_expand ? ret : STATUS_CMD_OK;
 }
 
 int exec_subshell(const wcstring &cmd, parser_t &parser, bool apply_exit_status) {
     bool break_expand = false;
-    return exec_subshell_internal(cmd, parser, none(), nullptr, &break_expand, apply_exit_status,
+    return exec_subshell_internal(cmd, parser, nullptr, nullptr, &break_expand, apply_exit_status,
                                   false);
 }
 
 int exec_subshell(const wcstring &cmd, parser_t &parser, wcstring_list_t &outputs,
                   bool apply_exit_status) {
     bool break_expand = false;
-    return exec_subshell_internal(cmd, parser, none(), &outputs, &break_expand, apply_exit_status,
+    return exec_subshell_internal(cmd, parser, nullptr, &outputs, &break_expand, apply_exit_status,
                                   false);
 }

src/exec.h

@@ -31,7 +31,7 @@ int exec_subshell(const wcstring &cmd, parser_t &parser, wcstring_list_t &output
 /// "success" (even though the command may have failed), a non-zero return means that we should
 /// halt expansion. If the \p pgid is supplied, then any spawned external commands should join that
 /// pgroup.
-int exec_subshell_for_expand(const wcstring &cmd, parser_t &parser, maybe_t<pid_t> parent_pgid,
+int exec_subshell_for_expand(const wcstring &cmd, parser_t &parser, const job_tree_ref_t &job_tree,
                              wcstring_list_t &outputs);
 
 /// Loops over close until the syscall was run without being interrupted.

src/expand.cpp

@@ -622,7 +622,7 @@ static expand_result_t expand_cmdsubst(wcstring input, const operation_context_t
 
     wcstring_list_t sub_res;
     const wcstring subcmd(paren_begin + 1, paren_end - paren_begin - 1);
-    int subshell_status = exec_subshell_for_expand(subcmd, *ctx.parser, ctx.parent_pgid, sub_res);
+    int subshell_status = exec_subshell_for_expand(subcmd, *ctx.parser, ctx.job_tree, sub_res);
     if (subshell_status != 0) {
         // TODO: Ad-hoc switch, how can we enumerate the possible errors more safely?
         const wchar_t *err;

src/io.h

@@ -21,6 +21,8 @@
 
 using std::shared_ptr;
 
+class job_tree_t;
+
 /// A simple set of FDs.
 struct fd_set_t {
     std::vector<bool> fds;
@@ -462,10 +464,10 @@ struct io_streams_t {
     // Actual IO redirections. This is only used by the source builtin. Unowned.
     const io_chain_t *io_chain{nullptr};
 
-    // The pgid of the job, if any. This enables builtins which run more code like eval() to share
-    // pgid.
+    // The job tree of the job, if any. This enables builtins which run more code like eval() to
+    // share pgid.
     // TODO: this is awkwardly placed, consider just embedding a lineage here.
-    maybe_t<pid_t> parent_pgid{};
+    std::shared_ptr<job_tree_t> job_tree{};
 
     // io_streams_t cannot be copied.
     io_streams_t(const io_streams_t &) = delete;

src/operation_context.h

@@ -7,6 +7,7 @@
 
 class environment_t;
 class parser_t;
+class job_tree_t;
 
 /// A common helper which always returns false.
 bool no_cancel();
@@ -29,10 +30,10 @@ class operation_context_t {
     // context itself.
     const environment_t &vars;
 
-    /// The pgid of the parental job.
+    /// The job tree of the parental job.
     /// This is used only when expanding command substitutions. If this is set, any jobs created by
-    /// the command substitions should use this pgid.
-    maybe_t<pid_t> parent_pgid{};
+    /// the command substitions should use this tree.
+    std::shared_ptr<job_tree_t> job_tree{};
 
     // A function which may be used to poll for cancellation.
     cancel_checker_t cancel_checker;

src/parse_execution.cpp

@@ -1283,8 +1283,9 @@ end_execution_reason_t parse_execution_context_t::run_1_job(tnode_t<g::job> job_
 
     // Clean up the job on failure or cancellation.
     if (pop_result == end_execution_reason_t::ok) {
-        // Set the pgroup assignment mode, now that the job is populated.
+        // Set the pgroup assignment mode and job tree, now that the job is populated.
         job->pgroup_provenance = get_pgroup_provenance(job, lineage);
+        job->job_tree = job_tree_t::decide_tree_for_job(job.get(), lineage.job_tree);
 
         // Success. Give the job to the parser - it will clean it up.
         parser->job_add(job);

src/parser.cpp

@@ -640,12 +640,12 @@ profile_item_t *parser_t::create_profile_item() {
     return result;
 }
 
-eval_res_t parser_t::eval(const wcstring &cmd, const io_chain_t &io, maybe_t<pid_t> parent_pgid,
+eval_res_t parser_t::eval(const wcstring &cmd, const io_chain_t &io, const job_tree_ref_t &job_tree,
                           enum block_type_t block_type) {
     // Parse the source into a tree, if we can.
     parse_error_list_t error_list;
     if (parsed_source_ref_t ps = parse_source(cmd, parse_flag_none, &error_list)) {
-        return this->eval(ps, io, parent_pgid, block_type);
+        return this->eval(ps, io, job_tree, block_type);
     } else {
         // Get a backtrace. This includes the message.
         wcstring backtrace_and_desc;
@@ -662,12 +662,12 @@ eval_res_t parser_t::eval(const wcstring &cmd, const io_chain_t &io, maybe_t<pid
 }
 
 eval_res_t parser_t::eval(const parsed_source_ref_t &ps, const io_chain_t &io,
-                          maybe_t<pid_t> parent_pgid, enum block_type_t block_type) {
+                          const job_tree_ref_t &job_tree, enum block_type_t block_type) {
     assert(block_type == block_type_t::top || block_type == block_type_t::subst);
     if (!ps->tree.empty()) {
         job_lineage_t lineage;
         lineage.block_io = io;
-        lineage.parent_pgid = parent_pgid;
+        lineage.job_tree = job_tree;
         // Execute the first node.
         tnode_t<grammar::job_list> start{&ps->tree, &ps->tree.front()};
         return this->eval_node(ps, start, std::move(lineage), block_type);
@@ -705,8 +705,8 @@ eval_res_t parser_t::eval_node(const parsed_source_ref_t &ps, tnode_t<T> node,
     operation_context_t op_ctx = this->context();
     block_t *scope_block = this->push_block(block_t::scope_block(block_type));
 
-    // Propogate any parent pgid.
-    op_ctx.parent_pgid = lineage.parent_pgid;
+    // Propogate our job tree.
+    op_ctx.job_tree = lineage.job_tree;
 
     // Create and set a new execution context.
     using exc_ctx_ref_t = std::unique_ptr<parse_execution_context_t>;

src/parser.h

@@ -282,20 +282,21 @@ class parser_t : public std::enable_shared_from_this<parser_t> {
     ///
     /// \param cmd the string to evaluate
     /// \param io io redirections to perform on all started jobs
-    /// \param parent_pgid if set, the pgid to give to spawned jobs
+    /// \param job_tree if set, the job tree to give to spawned jobs.
     /// \param block_type The type of block to push on the block stack, which must be either 'top'
     /// or 'subst'.
     /// \param break_expand If not null, return by reference whether the error ought to be an expand
     /// error. This includes nested expand errors, and command-not-found.
     ///
     /// \return the result of evaluation.
-    eval_res_t eval(const wcstring &cmd, const io_chain_t &io, maybe_t<pid_t> parent_pgid = {},
+    eval_res_t eval(const wcstring &cmd, const io_chain_t &io, const job_tree_ref_t &job_tree = {},
                     block_type_t block_type = block_type_t::top);
 
     /// Evaluate the parsed source ps.
     /// Because the source has been parsed, a syntax error is impossible.
     eval_res_t eval(const parsed_source_ref_t &ps, const io_chain_t &io,
-                    maybe_t<pid_t> parent_pgid = {}, block_type_t block_type = block_type_t::top);
+                    const job_tree_ref_t &job_tree = {},
+                    block_type_t block_type = block_type_t::top);
 
     /// Evaluates a node.
     /// The node type must be grammar::statement or grammar::job_list.

src/proc.cpp

@@ -235,7 +235,8 @@ void print_exit_warning_for_jobs(const job_list_t &jobs) {
     fputws(_(L"Use 'disown PID' to remove jobs from the list without terminating them.\n"), stdout);
 }
 
-job_tree_t::job_tree_t(bool placeholder) : is_placeholder_(placeholder) {}
+job_tree_t::job_tree_t(bool job_control, bool placeholder)
+    : job_control_(job_control), is_placeholder_(placeholder) {}
 
 void job_tree_t::set_pgid(pid_t pgid) {
     // TODO: thread safety?
@@ -245,6 +246,39 @@ void job_tree_t::set_pgid(pid_t pgid) {
     pgid_ = pgid;
 }
 
+maybe_t<pid_t> job_tree_t::get_pgid() const { return pgid_; }
+
+job_tree_ref_t job_tree_t::decide_tree_for_job(const job_t *job, const job_tree_ref_t &proposed) {
+    // Note there's three cases to consider:
+    //  nullptr         -> this is a root job, there is no inherited job tree
+    //  placeholder     -> we are running as part of a simple function execution, create a new job
+    //                      tree for any spawned jobs
+    //  non-placeholder -> we are running as part of a real pipeline
+    // Decide if this job can use the placeholder tree.
+    // This is true if it's a simple foreground execution of an internal proc.
+
+    bool can_use_placeholder =
+        job->is_foreground() && job->processes.size() == 1 && job->processes.front()->is_internal();
+
+    bool needs_new_tree = false;
+    if (!proposed) {
+        // We don't have a tree yet.
+        needs_new_tree = true;
+    } else if (!job->is_foreground()) {
+        // Background jobs always get a new tree.
+        needs_new_tree = true;
+    } else if (proposed->is_placeholder() && !can_use_placeholder) {
+        // We cannot use the placeholder tree for this job.
+        needs_new_tree = true;
+    }
+
+    if (!needs_new_tree) {
+        return proposed;
+    } else {
+        return job_tree_ref_t(new job_tree_t(job->wants_job_control(), can_use_placeholder));
+    }
+}
+
 void job_mark_process_as_failed(const std::shared_ptr<job_t> &job, const process_t *failed_proc) {
     // The given process failed to even lift off (e.g. posix_spawn failed) and so doesn't have a
     // valid pid. Mark it and everything after it as dead.

src/proc.h

@@ -179,14 +179,23 @@ class job_tree_t {
     /// Get the pgid, or none() if it has not been set.
     maybe_t<pid_t> get_pgid() const;
 
+    /// \return whether we want job control
+    bool wants_job_control() const { return job_control_; }
+
     /// \return whether this is a placeholder.
     bool is_placeholder() const { return is_placeholder_; }
 
+    /// Given a job and a proposed job tree (possibly null), return the job tree to actually use.
+    /// The proposed tree is the tree from the parent job, or null if this is a root.
+    static job_tree_ref_t decide_tree_for_job(const job_t *job,
+                                              const job_tree_ref_t &proposed_tree);
+
    private:
     maybe_t<pid_t> pgid_{};
+    const bool job_control_;
     const bool is_placeholder_;
 
-    explicit job_tree_t(bool placeholder);
+    explicit job_tree_t(bool job_control, bool placeholder);
 };
 
 /// A structure representing a single fish process. Contains variables for tracking process state
@@ -321,10 +330,10 @@ void release_job_id(job_id_t jid);
 /// job_t. It is also important that job_t not contain this: because it stores block IO, it will
 /// extend the life of the IO which may prevent pipes from closing in a timely manner. See #6397.
 struct job_lineage_t {
-    /// The pgid of the parental job.
+    /// The job tree.
     /// If our job is "nested" as part of a function or block execution, and that function or block
     /// is part of a pipeline, then this may be set.
-    maybe_t<pid_t> parent_pgid{};
+    job_tree_ref_t job_tree{};
 
     /// Whether job control is on for the root.
     /// This is set if our job is nested as part of a function or block execution.
@@ -446,6 +455,10 @@ class job_t {
     /// All the processes in this job.
     process_list_t processes;
 
+    // The tree containing this job.
+    // This is never null and not changed after construction.
+    job_tree_ref_t job_tree{};
+
     /// Process group ID for the process group that this job is running in.
     /// Set to a nonexistent, non-return-value of getpgid() integer by the constructor
     pid_t pgid{INVALID_PID};