Fix up --on-job-exit caller

The `function --on-job-exit caller` feature allows a command substitution
to observe when the parent job exits. This has never worked very well - in
particular it is based on job IDs, so a function that observes this will
run multiple times. Implement it properly.

Do this by having a not-recycled "internal job id".

This is only used by psub, but ensure it works properly none-the-less.

src/builtin_function.cpp

@@ -103,17 +103,15 @@ static int parse_cmd_opts(function_cmd_opts_t &opts, int *optind,  //!OCLINT(hig
                 event_description_t e(event_type_t::any);
 
                 if ((opt == 'j') && (wcscasecmp(w.woptarg, L"caller") == 0)) {
-                    job_id_t job_id = -1;
-                    if (parser.libdata().is_subshell) {
-                        job_id = parser.libdata().caller_job_id;
-                    }
-                    if (job_id == -1) {
+                    internal_job_id_t caller_id =
+                        parser.libdata().is_subshell ? parser.libdata().caller_id : 0;
+                    if (caller_id == 0) {
                         streams.err.append_format(
                             _(L"%ls: Cannot find calling job for event handler"), cmd);
                         return STATUS_INVALID_ARGS;
                     }
                     e.type = event_type_t::caller_exit;
-                    e.param1.job_id = job_id;
+                    e.param1.caller_id = caller_id;
                 } else if ((opt == 'p') && (wcscasecmp(w.woptarg, L"%self") == 0)) {
                     e.type = event_type_t::exit;
                     e.param1.pid = getpid();

src/event.cpp

@@ -114,7 +114,7 @@ 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::caller_exit: {
-            return classv.desc.param1.job_id == instance.desc.param1.job_id;
+            return classv.desc.param1.caller_id == instance.desc.param1.caller_id;
         }
         case event_type_t::generic: {
             return classv.desc.str_param1 == instance.desc.str_param1;
@@ -378,7 +378,7 @@ void event_print(io_streams_t &streams, maybe_t<event_type_t> type_filter) {
                       case event_type_t::exit:
                           return d1.param1.pid < d2.param1.pid;
                       case event_type_t::caller_exit:
-                          return d1.param1.job_id < d2.param1.job_id;
+                          return d1.param1.caller_id < d2.param1.caller_id;
                       case event_type_t::variable:
                       case event_type_t::any:
                       case event_type_t::generic:

src/event.h

@@ -44,10 +44,11 @@ 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. (Negative
-    /// values are used for PGIDs). job_id: Job id for EVENT_JOB_ID type events
+    /// values are used for PGIDs).
+    /// caller_id: Internal job id for caller_exit type events
     union {
         int signal;
-        int job_id;
+        uint64_t caller_id;
         pid_t pid;
     } param1{};
 

src/parse_execution.cpp

@@ -1283,17 +1283,13 @@ end_execution_reason_t parse_execution_context_t::run_1_job(tnode_t<g::job> job_
     // We are about to populate a job. One possible argument to the job is a command substitution
     // which may be interested in the job that's populating it, via '--on-job-exit caller'. Record
     // the job ID here.
-    auto &libdata = parser->libdata();
-    const auto saved_caller_jid = libdata.caller_job_id;
-    libdata.caller_job_id = job->job_id();
+    scoped_push<internal_job_id_t> caller_id(&parser->libdata().caller_id, job->internal_job_id);
 
     // Populate the job. This may fail for reasons like command_not_found. If this fails, an error
     // will have been printed.
     end_execution_reason_t pop_result =
         this->populate_job_from_job_node(job.get(), job_node, associated_block);
-
-    assert(libdata.caller_job_id == job->job_id() && "Caller job ID unexpectedly changed");
-    parser->libdata().caller_job_id = saved_caller_jid;
+    caller_id.restore();
 
     // Store time it took to 'parse' the command.
     if (profile_item != nullptr) {

src/parser.h

@@ -148,9 +148,9 @@ struct library_data_t {
     /// Whether we are currently cleaning processes.
     bool is_cleaning_procs{false};
 
-    /// The job id of the job being populated.
+    /// The internal job id of the job being populated, or 0 if none.
     /// This supports the '--on-job-exit caller' feature.
-    job_id_t caller_job_id{-1};
+    internal_job_id_t caller_id{0};
 
     /// Whether we are running a subshell command.
     bool is_subshell{false};

src/proc.cpp

@@ -275,9 +275,15 @@ bool process_t::is_internal() const {
     return true;
 }
 
+static uint64_t next_internal_job_id() {
+    static std::atomic<uint64_t> s_next{};
+    return ++s_next;
+}
+
 job_t::job_t(job_id_t job_id, const properties_t &props, const job_lineage_t &lineage)
     : properties(props),
       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() {
@@ -447,6 +453,7 @@ event_t proc_create_event(const wchar_t *msg, event_type_t type, pid_t pid, int
     event_t event{type};
     event.desc.param1.pid = pid;
 
+    event.arguments.reserve(3);
     event.arguments.push_back(msg);
     event.arguments.push_back(to_string(pid));
     event.arguments.push_back(to_string(status));
@@ -617,6 +624,7 @@ static bool process_clean_after_marking(parser_t &parser, bool allow_interactive
             }
             exit_events.push_back(
                 proc_create_event(L"JOB_EXIT", event_type_t::caller_exit, j->job_id(), 0));
+            exit_events.back().desc.param1.caller_id = j->internal_job_id;
         }
     }
 

src/proc.h

@@ -269,7 +269,12 @@ class process_t {
 typedef std::unique_ptr<process_t> process_ptr_t;
 typedef std::vector<process_ptr_t> process_list_t;
 
-typedef int job_id_t;
+/// The non user-visible, never-recycled job ID.
+/// Every job has a unique positive value for this.
+using internal_job_id_t = uint64_t;
+
+/// The user-visible, optional, recycled job ID.
+using job_id_t = int;
 job_id_t acquire_job_id(void);
 void release_job_id(job_id_t jid);
 
@@ -411,8 +416,12 @@ class job_t {
     pgroup_provenance_t pgroup_provenance{};
 
     /// The id of this job.
+    /// This is user-visible, is recycled, and may be -1.
     job_id_t job_id() const { return job_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.

tests/checks/caller-observer.fish

@@ -0,0 +1,34 @@
+#RUN: %fish %s
+
+# Verify the '--on-job-exit caller' misfeature.
+function make_call_observer -a type
+    function observer_$type --on-job-exit caller --inherit-variable type
+        echo "Observed exit of $type"
+    end
+end
+
+command echo "echo ran 1" (make_call_observer external)
+#CHECK: echo ran 1
+#CHECK: Observed exit of external
+
+builtin echo "echo ran 2" (make_call_observer builtin)
+#CHECK: echo ran 2
+#CHECK: Observed exit of builtin
+
+function func_echo
+    builtin echo $argv
+end
+
+func_echo "echo ran 3" (make_call_observer function)
+#CHECK: echo ran 3
+#CHECK: Observed exit of function
+
+# They should not fire again.
+# TODO: We don't even clean up the functions!
+
+command echo "echo ran 4"
+builtin echo "echo ran 5"
+func_echo "echo ran 6"
+#CHECK: echo ran 4
+#CHECK: echo ran 5
+#CHECK: echo ran 6