Put job_id into job_tree

Job IDs are really a property of a job tree, not individual jobs. Reflect
that fact by migrating job IDs into job_tree.

src/exec.cpp

@@ -662,7 +662,6 @@ static proc_performer_t get_performer_for_process(process_t *p, job_t *job,
     job_lineage_t lineage;
     lineage.job_tree = job->job_tree;
     lineage.block_io = io_chain;
-    lineage.root_constructed = job->root_constructed;
 
     if (p->type == process_type_t::block_node) {
         const parsed_source_ref_t &source = p->block_node_source;
@@ -713,14 +712,6 @@ static bool exec_block_or_func_process(parser_t &parser, const std::shared_ptr<j
         io_chain.push_back(block_output_bufferfill);
     }
 
-    // If this function is the only process in the current job
-    // and that job is in the foreground then mark this job as internal
-    // so it doesn't increment the job id for any jobs created within this
-    // function.
-    if (p->is_first_in_job && p->is_last_in_job && j->flags().foreground) {
-        j->mark_internal();
-    }
-
     // Get the process performer, and just execute it directly.
     // Do it in this scoped way so that the performer function can be eagerly deallocating releasing
     // its captured io chain.

src/parse_execution.cpp

@@ -1260,7 +1260,7 @@ end_execution_reason_t parse_execution_context_t::run_1_job(tnode_t<g::job> job_
     props.from_event_handler = ld.is_event;
     props.job_control = wants_job_control;
 
-    shared_ptr<job_t> job = std::make_shared<job_t>(acquire_job_id(), props, this->lineage);
+    shared_ptr<job_t> job = std::make_shared<job_t>(props);
     job->tmodes = tmodes;
 
     job->mut_flags().foreground = !job_node_is_background(job_node);
@@ -1285,7 +1285,8 @@ end_execution_reason_t parse_execution_context_t::run_1_job(tnode_t<g::job> job_
     if (pop_result == end_execution_reason_t::ok) {
         // 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);
+        job_tree_t::populate_tree_for_job(job.get(), lineage.job_tree);
+        assert(job->job_tree && "Should have a job tree");
 
         // Success. Give the job to the parser - it will clean it up.
         parser->job_add(job);

src/proc.cpp

@@ -149,6 +149,12 @@ bool job_t::should_report_process_exits() const {
         return false;
     }
 
+    // Only report root job exits.
+    // For example in `ls | begin ; cat ; end` we don't need to report the cat sub-job.
+    if (!flags().is_tree_root) {
+        return false;
+    }
+
     // Return whether we have an external process.
     for (const auto &p : this->processes) {
         if (p->type == process_type_t::external) {
@@ -158,7 +164,7 @@ bool job_t::should_report_process_exits() const {
     return false;
 }
 
-bool job_t::job_chain_is_fully_constructed() const { return *root_constructed; }
+bool job_t::job_chain_is_fully_constructed() const { return job_tree->is_root_constructed(); }
 
 bool job_t::signal(int signal) {
     // Presumably we are distinguishing between the two cases below because we do
@@ -236,7 +242,15 @@ void print_exit_warning_for_jobs(const job_list_t &jobs) {
 }
 
 job_tree_t::job_tree_t(bool job_control, bool placeholder)
-    : job_control_(job_control), is_placeholder_(placeholder) {}
+    : job_control_(job_control),
+      is_placeholder_(placeholder),
+      job_id_(placeholder ? -1 : acquire_job_id()) {}
+
+job_tree_t::~job_tree_t() {
+    if (job_id_ > 0) {
+        release_job_id(job_id_);
+    }
+}
 
 void job_tree_t::set_pgid(pid_t pgid) {
     // TODO: thread safety?
@@ -248,7 +262,7 @@ void job_tree_t::set_pgid(pid_t 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) {
+void job_tree_t::populate_tree_for_job(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
@@ -256,7 +270,6 @@ job_tree_ref_t job_tree_t::decide_tree_for_job(const job_t *job, const job_tree_
     //  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();
 
@@ -272,10 +285,13 @@ job_tree_ref_t job_tree_t::decide_tree_for_job(const job_t *job, const job_tree_
         needs_new_tree = true;
     }
 
+    job->mut_flags().is_tree_root = needs_new_tree;
+
     if (!needs_new_tree) {
-        return proposed;
+        job->job_tree = proposed;
     } else {
-        return job_tree_ref_t(new job_tree_t(job->wants_job_control(), can_use_placeholder));
+        job->job_tree =
+            job_tree_ref_t(new job_tree_t(job->wants_job_control(), can_use_placeholder));
     }
 }
 
@@ -354,19 +370,17 @@ static uint64_t next_internal_job_id() {
     return ++s_next;
 }
 
-job_t::job_t(job_id_t job_id, const properties_t &props, const job_lineage_t &lineage)
+job_t::job_t(const properties_t &props)
-    : properties(props),
+    : properties(props), internal_job_id(next_internal_job_id()) {}
-      job_id_(job_id),
-      internal_job_id(next_internal_job_id()),
-      root_constructed(lineage.root_constructed ? lineage.root_constructed : this->constructed) {}
 
-job_t::~job_t() {
+job_t::~job_t() = default;
-    if (job_id_ != -1) release_job_id(job_id_);
-}
 
 void job_t::mark_constructed() {
     assert(!is_constructed() && "Job was already constructed");
-    *constructed = true;
+    mut_flags().constructed = true;
+    if (flags().is_tree_root) {
+        job_tree->mark_root_constructed();
+    }
 }
 
 bool job_t::has_internal_proc() const {
@@ -944,7 +958,7 @@ void job_t::continue_job(parser_t &parser, bool reclaim_foreground_pgrp, bool se
     mut_flags().notified = false;
 
     FLOGF(proc_job_run, L"%ls job %d, gid %d (%ls), %ls, %ls",
-          send_sigcont ? L"Continue" : L"Start", job_id_, pgid, command_wcstr(),
+          send_sigcont ? L"Continue" : L"Start", job_id(), pgid, command_wcstr(),
           is_completed() ? L"COMPLETED" : L"UNCOMPLETED",
           parser.libdata().is_interactive ? L"INTERACTIVE" : L"NON-INTERACTIVE");
 

src/proc.h

@@ -154,6 +154,12 @@ class internal_proc_t {
 ///   function
 enum { INVALID_PID = -2 };
 
+/// A job ID, corresponding to what is printed in 'jobs'.
+/// 1 is the first valid job ID.
+using job_id_t = int;
+job_id_t acquire_job_id(void);
+void release_job_id(job_id_t jid);
+
 /// job_tree_t is conceptually similar to the idea of a process group. It represents data which
 /// is shared among all of the "subjobs" that may be spawned by a single job.
 /// For example, two fish functions in a pipeline may themselves spawn multiple jobs, but all will
@@ -185,15 +191,27 @@ class job_tree_t {
     /// \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.
+    /// \return the job ID, or -1 if none.
+    job_id_t get_id() const { return job_id_; }
+
+    /// Mark the root as constructed.
+    /// This is used to avoid reaping a process group leader while there are still procs that may
+    /// want to enter its group.
+    void mark_root_constructed() { root_constructed_ = true; };
+    bool is_root_constructed() const { return root_constructed_; }
+
+    /// Given a job and a proposed job tree (possibly null), populate the job's tree.
     /// 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,
+    static void populate_tree_for_job(job_t *job, const job_tree_ref_t &proposed_tree);
-                                              const job_tree_ref_t &proposed_tree);
+
+    ~job_tree_t();
 
    private:
     maybe_t<pid_t> pgid_{};
     const bool job_control_;
     const bool is_placeholder_;
+    const job_id_t job_id_;
+    relaxed_atomic_bool_t root_constructed_{};
 
     explicit job_tree_t(bool job_control, bool placeholder);
 };
@@ -338,10 +356,6 @@ struct job_lineage_t {
     /// The IO chain associated with any block containing this job.
     /// For example, in `begin; foo ; end < file.txt` this would have the 'file.txt' IO.
     io_chain_t block_io{};
-
-    /// A shared pointer indicating that the entire tree of jobs is safe to disown.
-    /// This is set to true by the "root" job after it is constructed.
-    std::shared_ptr<relaxed_atomic_bool_t> root_constructed{};
 };
 
 /// A job has a mode which describes how its pgroup is assigned (before the value is known).
@@ -390,15 +404,12 @@ class job_t {
     /// messages about job status on the terminal.
     wcstring command_str;
 
-    /// The job_id for this job.
-    job_id_t job_id_;
-
     // No copying.
     job_t(const job_t &rhs) = delete;
     void operator=(const job_t &) = delete;
 
    public:
-    job_t(job_id_t job_id, const properties_t &props, const job_lineage_t &lineage);
+    explicit job_t(const properties_t &props);
     ~job_t();
 
     /// Returns the command as a wchar_t *. */
@@ -464,35 +475,22 @@ class job_t {
 
     /// The id of this job.
     /// This is user-visible, is recycled, and may be -1.
-    job_id_t job_id() const { return job_id_; }
+    job_id_t job_id() const { return job_tree->get_id(); }
 
     /// A non-user-visible, never-recycled job ID.
     const internal_job_id_t internal_job_id;
 
-    /// Mark this job as internal. Internal jobs' job_ids are removed from the
-    /// list of jobs so that, among other things, they don't take a job_id
-    /// entry.
-    void mark_internal() {
-        release_job_id(job_id_);
-        job_id_ = -1;
-    }
-
     /// The saved terminal modes of this job. This needs to be saved so that we can restore the
     /// terminal to the same state after temporarily taking control over the terminal when a job
     /// stops.
     struct termios tmodes {};
 
-    /// Whether the specified job is completely constructed, i.e. completely parsed, and every
-    /// process in the job has been forked, etc.
-    /// This is a shared_ptr because it may be passed to child jobs through the lineage.
-    const std::shared_ptr<relaxed_atomic_bool_t> constructed =
-        std::make_shared<relaxed_atomic_bool_t>(false);
-
-    /// Whether the root job is constructed; this may share a reference with 'constructed'.
-    const std::shared_ptr<relaxed_atomic_bool_t> root_constructed;
-
     /// Flags associated with the job.
     struct flags_t {
+        /// Whether the specified job is completely constructed: every process in the job has been
+        /// forked, etc.
+        bool constructed{false};
+
         /// Whether the user has been told about stopped job.
         bool notified{false};
 
@@ -507,6 +505,10 @@ class job_t {
 
         /// Whether to print timing for this job.
         bool has_time_prefix{false};
+
+        // Indicates that we are the "tree root." Any other jobs using this tree are nested.
+        bool is_tree_root{false};
+
     } job_flags{};
 
     /// Access the job flags.
@@ -532,7 +534,7 @@ class job_t {
 
     // Helper functions to check presence of flags on instances of jobs
     /// The job has been fully constructed, i.e. all its member processes have been launched
-    bool is_constructed() const { return *constructed; }
+    bool is_constructed() const { return flags().constructed; }
     /// The job was launched in the foreground and has control of the terminal
     bool is_foreground() const { return flags().foreground; }
     /// The job is complete, i.e. all its member processes have been reaped