Allow on-job-exit handlers to be added for any pid in the job

Prior to this change, a function with an on-job-exit event handler must be
added with the pgid of the job. But sometimes the pgid of the job is fish
itself (if job control is disabled) and the previous commit made last_pid
an actual pid from the job, instead of its pgroup.

Switch on-job-exit to accept any pid from the job (except fish itself).
This allows it to be used directly with $last_pid, except that it now
works if job control is off. This is implemented by "resolving" the pid to
the internal job id at the point the event handler is added.

Also switch to passing the last pid of the job, rather than its pgroup.
This aligns better with $last_pid.

CHANGELOG.rst

@@ -30,6 +30,7 @@ Scripting improvements
 - ``$last_pid`` now reports the pid of the last process in the pipeline, allowing it to be used in scripts (:issue:`5036`, :issue:`5832`, :issue:`7721`).
 - ``process-exit`` event handlers now receive the same value as ``$status`` in all cases, instead of receiving -1 when the exit was due to a signal.
 - ``process-exit`` event handlers for pid 0 also received ``JOB_EXIT`` events; this has been fixed.
+- ``job-exit`` event handlers may now be created with any of the pids from the job. The handler is passed the last pid in the job as its second argument, instead of the process group.
 
 Interactive improvements
 -------------------------

doc_src/cmds/function.rst

@@ -30,7 +30,7 @@ The following options are available:
 
 - ``-v`` or ``--on-variable VARIABLE_NAME`` tells fish to run this function when the variable VARIABLE_NAME changes value. Note that fish makes no guarantees on any particular timing or even that the function will be run for every single ``set``. Rather it will be run when the variable has been set at least once, possibly skipping some values or being run when the variable has been set to the same value (except for universal variables set in other shells - only changes in the value will be picked up for those).
 
-- ``-j PGID`` or ``--on-job-exit PGID`` tells fish to run this function when the job with group ID PGID exits. Instead of PGID, the string 'caller' can be specified. This is only legal when in a command substitution, and will result in the handler being triggered by the exit of the job which created this command substitution.
+- ``-j PID`` or ``--on-job-exit PID`` tells fish to run this function when the job containing a child process with the given PID exits. Instead of PID, the string 'caller' can be specified. This is only legal when in a command substitution, and will result in the handler being triggered by the exit of the job which created this command substitution.
 
 - ``-p PID`` or ``--on-process-exit PID`` tells fish to run this function when the fish child process
   with process ID PID exits. Instead of a PID, for backward compatibility,

src/builtin_function.cpp

@@ -53,6 +53,18 @@ static const struct woption long_options[] = {
     {L"inherit-variable", required_argument, nullptr, 'V'},
     {nullptr, 0, nullptr, 0}};
 
+/// \return the internal_job_id for a pid, or 0 if none.
+/// This looks through both active and finished jobs.
+static internal_job_id_t job_id_for_pid(pid_t pid, parser_t &parser) {
+    if (const auto *job = parser.job_get_from_pid(pid)) {
+        return job->internal_job_id;
+    }
+    if (wait_handle_ref_t wh = parser.get_wait_handles().get_by_pid(pid)) {
+        return wh->internal_job_id;
+    }
+    return 0;
+}
+
 static int parse_cmd_opts(function_cmd_opts_t &opts, int *optind,  //!OCLINT(high ncss method)
                           int argc, const wchar_t **argv, parser_t &parser, io_streams_t &streams) {
     const wchar_t *cmd = L"function";
@@ -127,7 +139,7 @@ static int parse_cmd_opts(function_cmd_opts_t &opts, int *optind,  //!OCLINT(hig
                         e.param1.pid = pid;
                     } else {
                         e.type = event_type_t::job_exit;
-                        e.param1.pgid = pid;
+                        e.param1.jobspec = {pid, job_id_for_pid(pid, parser)};
                     }
                 }
                 opts.events.push_back(e);
@@ -281,15 +293,19 @@ maybe_t<int> builtin_function(parser_t &parser, io_streams_t &streams,
     // If there is an --on-process-exit or --on-job-exit event handler for some pid, and that
     // process has already exited, run it immediately (#7210).
     for (const event_description_t &ed : opts.events) {
-        if ((ed.type == event_type_t::process_exit || ed.type == event_type_t::job_exit) &&
-            ed.param1.pid != EVENT_ANY_PID) {
-            wait_handle_ref_t wh = parser.get_wait_handles().get_by_pid(ed.param1.pid);
+        if (ed.type == event_type_t::process_exit) {
+            pid_t pid = ed.param1.pid;
+            if (pid == EVENT_ANY_PID) continue;
+            wait_handle_ref_t wh = parser.get_wait_handles().get_by_pid(pid);
             if (wh && wh->completed) {
-                if (ed.type == event_type_t::process_exit) {
-                    event_fire(parser, event_t::process_exit(ed.param1.pid, wh->status));
-                } else {
-                    event_fire(parser, event_t::job_exit(ed.param1.pgid));
-                }
+                event_fire(parser, event_t::process_exit(pid, wh->status));
+            }
+        } else if (ed.type == event_type_t::job_exit) {
+            pid_t pid = ed.param1.jobspec.pid;
+            if (pid == EVENT_ANY_PID) continue;
+            wait_handle_ref_t wh = parser.get_wait_handles().get_by_pid(pid);
+            if (wh && wh->completed) {
+                event_fire(parser, event_t::job_exit(pid, wh->internal_job_id));
             }
         }
     }

src/common.h

@@ -156,6 +156,13 @@ enum {
 };
 typedef unsigned int escape_flags_t;
 
+/// A user-visible job ID.
+using job_id_t = int;
+
+/// The non user-visible, never-recycled job ID.
+/// Every job has a unique positive value for this.
+using internal_job_id_t = uint64_t;
+
 /// Issue a debug message with printf-style string formating and automatic line breaking. The string
 /// will begin with the string \c program_name, followed by a colon and a whitespace.
 ///

src/event.cpp

@@ -120,9 +120,10 @@ static bool handler_matches(const event_handler_t &classv, const event_t &instan
             return classv.desc.param1.pid == instance.desc.param1.pid;
         }
         case event_type_t::job_exit: {
-            if (classv.desc.param1.pgid == EVENT_ANY_PID) return true;
+            const auto &jobspec = classv.desc.param1.jobspec;
+            if (jobspec.pid == EVENT_ANY_PID) return true;
             only_once = true;
-            return classv.desc.param1.pgid == instance.desc.param1.pgid;
+            return jobspec.internal_job_id == instance.desc.param1.jobspec.internal_job_id;
         }
         case event_type_t::caller_exit: {
             only_once = true;
@@ -167,12 +168,12 @@ wcstring event_get_desc(const parser_t &parser, const event_t &evt) {
         }
 
         case event_type_t::job_exit: {
-            if (job_t *j = parser.job_get_from_pid(ed.param1.pid)) {
+            const auto &jobspec = ed.param1.jobspec;
+            if (const job_t *j = parser.job_get_from_pid(jobspec.pid)) {
                 return format_string(_(L"exit handler for job %d, '%ls'"), j->job_id(),
                                      j->command_wcstr());
             } else {
-                return format_string(_(L"exit handler for job with process group %d"),
-                                     ed.param1.pgid);
+                return format_string(_(L"exit handler for job with pid %d"), jobspec.pid);
             }
         }
 
@@ -431,7 +432,7 @@ void event_print(io_streams_t &streams, const wcstring &type_filter) {
                       case event_type_t::process_exit:
                           return d1.param1.pid < d2.param1.pid;
                       case event_type_t::job_exit:
-                          return d1.param1.pgid < d2.param1.pgid;
+                          return d1.param1.jobspec.pid < d2.param1.jobspec.pid;
                       case event_type_t::caller_exit:
                           return d1.param1.caller_id < d2.param1.caller_id;
                       case event_type_t::variable:
@@ -526,12 +527,12 @@ event_t event_t::process_exit(pid_t pid, int status) {
 }
 
 // static
-event_t event_t::job_exit(pid_t pgid) {
+event_t event_t::job_exit(pid_t pid, internal_job_id_t jid) {
     event_t evt{event_type_t::job_exit};
-    evt.desc.param1.pgid = pgid;
+    evt.desc.param1.jobspec = {pid, jid};
     evt.arguments.reserve(3);
     evt.arguments.push_back(L"JOB_EXIT");
-    evt.arguments.push_back(to_string(pgid));
+    evt.arguments.push_back(to_string(pid));
     evt.arguments.push_back(L"0");  // historical
     return evt;
 }

src/event.h

@@ -43,6 +43,16 @@ extern const wchar_t *const event_filter_names[];
 
 /// Properties of an event.
 struct event_description_t {
+    /// Helper type for on-job-exit events.
+    struct job_spec_t {
+        // pid requested by the event, or ANY_PID for all.
+        pid_t pid;
+
+        // internal_job_id of the job to match.
+        // If this is 0, we match either all jobs (pid == ANY_PID) or no jobs (otherwise).
+        uint64_t internal_job_id;
+    };
+
     /// The event type.
     event_type_t type;
 
@@ -50,12 +60,12 @@ struct event_description_t {
     ///
     /// signal: Signal number for signal-type events.Use EVENT_ANY_SIGNAL to match any signal
     /// pid: Process id for process-type events. Use EVENT_ANY_PID to match any pid.
-    /// pgid: Process id for job-type events. This value is positive (or EVENT_ANY_PID).
+    /// jobspec: Info for on-job-exit events.
     /// caller_id: Internal job id for caller_exit type events
     union {
         int signal;
         pid_t pid;
-        pid_t pgid;
+        job_spec_t jobspec;
         uint64_t caller_id;
     } param1{};
 
@@ -103,7 +113,7 @@ struct event_t {
 
     /// Create a JOB_EXIT event. The pgid should be positive.
     /// The reported status is always 0 for historical reasons.
-    static event_t job_exit(pid_t pgid);
+    static event_t job_exit(pid_t pgid, internal_job_id_t jid);
 
     /// Create a caller_exit event.
     static event_t caller_exit(uint64_t internal_job_id, int job_id);

src/function.cpp

@@ -388,7 +388,7 @@ wcstring functions_def(const wcstring &name) {
                 break;
             }
             case event_type_t::job_exit: {
-                append_format(out, L" --on-job-exit %d", d.param1.pgid);
+                append_format(out, L" --on-job-exit %d", d.param1.jobspec.pid);
                 break;
             }
             case event_type_t::caller_exit: {

src/proc.cpp

@@ -610,6 +610,7 @@ static void save_wait_handle_for_completed_job(const shared_ptr<job_t> &job,
     for (auto &proc : job->processes) {
         if (wait_handle_ref_t wh = proc->get_wait_handle(false /* create */)) {
             wh->status = proc->status.status_value();
+            wh->internal_job_id = job->internal_job_id;
             wh->completed = true;
         }
     }
@@ -673,8 +674,9 @@ static bool process_clean_after_marking(parser_t &parser, bool allow_interactive
             // If this job already came from an event handler,
             // don't create an event or it's easy to get an infinite loop.
             if (!j->from_event_handler() && j->should_report_process_exits()) {
-                pid_t pgid = *j->get_pgid();
-                exit_events.push_back(event_t::job_exit(pgid));
+                if (auto last_pid = j->get_last_pid()) {
+                    exit_events.push_back(event_t::job_exit(*last_pid, j->internal_job_id));
+                }
             }
             // Caller exit events we still create, which anecdotally fixes `source (thing | psub)`
             // inside event handlers. This seems benign since this event is barely used (basically

src/proc.h

@@ -298,13 +298,6 @@ class process_t {
 using process_ptr_t = std::unique_ptr<process_t>;
 using process_list_t = std::vector<process_ptr_t>;
 
-/// A user-visible job ID.
-using job_id_t = int;
-
-/// The non user-visible, never-recycled job ID.
-/// Every job has a unique positive value for this.
-using internal_job_id_t = uint64_t;
-
 /// A struct representing a job. A job is a pipeline of one or more processes.
 class job_t {
    public:

src/wait_handle.h

@@ -23,6 +23,10 @@ struct wait_handle_t {
     /// The pid of this process.
     pid_t pid{};
 
+    /// The internal job id of the job which contained this process.
+    /// This is initially 0; it is set when the job is completed.
+    internal_job_id_t internal_job_id{};
+
     /// The "base name" of this process.
     /// For example if the process is "/bin/sleep" then this will be 'sleep'.
     wcstring base_name{};

tests/checks/jobs.fish

@@ -25,6 +25,11 @@ sleep 0.1
 # (there should be none).
 ps -o stat | string match 'Z*'
 
+# Verify disown can be used with last_pid, even if it is separate from the pgroup.
+# This should silently succeed.
+command true | sleep 0.5 &
+disown $last_pid
+
 jobs -q
 echo $status
 #CHECK: 1