core: split $MONITOR_METADATA and return it only if a single unit triggers OnFailure/OnSuccess

Remove the list logic, and simply skip passing metadata if more than one
unit triggered an OnFailure/OnSuccess handler.
Instead of a single env var to loop over, provide each separate item
as its own variable.

Fixes https://github.com/systemd/systemd/issues/22370

8 files changed, 84 insertions, 154 deletions

diff --git a/src/core/job.c b/src/core/job.c
index f28821071b..94ab381626 100644
--- a/src/core/job.c
+++ b/src/core/job.c
@@ -99,9 +99,6 @@ Job* job_free(Job *j) {
         assert(!j->subject_list);
         assert(!j->object_list);
 
-        while (!LIST_IS_EMPTY(j->triggered_by))
-                LIST_POP(triggered_by, j->triggered_by);
-
         job_unlink(j);
 
         sd_bus_track_unref(j->bus_track);
@@ -110,13 +107,6 @@ Job* job_free(Job *j) {
         return mfree(j);
 }
 
-void job_add_triggering_unit(Job *j, Unit *u) {
-        assert(j);
-        assert(u);
-
-        LIST_APPEND(triggered_by, j->triggered_by, u);
-}
-
 static void job_set_state(Job *j, JobState state) {
         assert(j);
         assert(state >= 0);
@@ -197,8 +187,6 @@ static void job_merge_into_installed(Job *j, Job *other) {
 
         j->irreversible = j->irreversible || other->irreversible;
         j->ignore_order = j->ignore_order || other->ignore_order;
-        if (other->triggered_by)
-                LIST_JOIN(triggered_by, j->triggered_by, other->triggered_by);
 }
 
 Job* job_install(Job *j) {
diff --git a/src/core/job.h b/src/core/job.h
index 762b0bb19b..a66e5985b8 100644
--- a/src/core/job.h
+++ b/src/core/job.h
@@ -124,8 +124,6 @@ struct Job {
         LIST_HEAD(JobDependency, subject_list);
         LIST_HEAD(JobDependency, object_list);
 
-        LIST_HEAD(Unit, triggered_by);
-
         /* Used for graph algs as a "I have been here" marker */
         Job* marker;
         unsigned generation;
@@ -246,5 +244,3 @@ JobResult job_result_from_string(const char *s) _pure_;
 const char* job_type_to_access_method(JobType t);
 
 int job_compare(Job *a, Job *b, UnitDependencyAtom assume_dep);
-
-void job_add_triggering_unit(Job *j, Unit *u);
diff --git a/src/core/service.c b/src/core/service.c
index 92af448ff4..551f3df355 100644
--- a/src/core/service.c
+++ b/src/core/service.c
@@ -1447,81 +1447,37 @@ static bool service_exec_needs_notify_socket(Service *s, ExecFlags flags) {
         return s->notify_access != NOTIFY_NONE;
 }
 
-static int service_create_monitor_md_env(Job *j, char **ret) {
-        _cleanup_free_ char *var = NULL;
-        const char *list_delim = ";";
-        bool first = true;
-        Unit *tu;
+static Service *service_get_triggering_service(Service *s) {
+        Unit *candidate = NULL, *other;
 
-        assert(j);
-        assert(ret);
+        assert(s);
 
-        /* Create an environment variable 'MONITOR_METADATA', if creation is successful
-         * a pointer to it is returned via ret.
-         *
-         * This variable contains a space separated set of fields which relate to
-         * the service(s) which triggered job 'j'. Job 'j' is the JOB_START job for
-         * an OnFailure= or OnSuccess= dependency. Format of the MONITOR_METADATA
-         * variable is as follows:
-         *
-         * MONITOR_METADATA="SERVICE_RESULT=<result-string0>,EXIT_CODE=<exit-code0>,EXIT_STATUS=<exit-status0>,
-         *                   INVOCATION_ID=<id>,UNIT=<triggering-unit0.service>;
-         *                   SERVICE_RESULT=<result-stringN>,EXIT_CODE=<exit-codeN>,EXIT_STATUS=<exit-statusN>,
-         *                   INVOCATION_ID=<id>,UNIT=<triggering-unitN.service>"
-         *
-         * Multiple results may be passed as in the above example if jobs are merged, i.e.
-         * some services a and b contain an OnFailure= or OnSuccess= dependency on the same
-         * service.
-         *
-         * For example:
+        /* Return the service which triggered service 's', this means dependency
+         * types which include the UNIT_ATOM_ON_{FAILURE,SUCCESS}_OF atoms.
          *
-         * MONITOR_METADATA="SERVICE_RESULT=exit-code,EXIT_CODE=exited,EXIT_STATUS=1,INVOCATION_ID=02dd868af2f344b18edaf74b618b2f90,UNIT=failure.service;
-         *                   SERVICE_RESULT=exit-code,EXIT_CODE=exited,EXIT_STATUS=1,INVOCATION_ID=80cb228bd7344f77a090eda603a3cfe2,UNIT=failure2.service"
-         */
-
-        LIST_FOREACH(triggered_by, tu, j->triggered_by) {
-                Service *env_source = SERVICE(tu);
-                int r;
-
-                if (!env_source)
-                        continue;
-
-                /* Add the environment variable name first. */
-                if (first && !strextend(&var, "MONITOR_METADATA="))
-                        return -ENOMEM;
-
-                if (!strextend(&var, !first ? list_delim : "", "SERVICE_RESULT=", service_result_to_string(env_source->result)))
-                        return -ENOMEM;
-
-                first = false;
-
-                if (env_source->main_exec_status.pid > 0 &&
-                    dual_timestamp_is_set(&env_source->main_exec_status.exit_timestamp)) {
-                        if (!strextend(&var, ",EXIT_CODE=", sigchld_code_to_string(env_source->main_exec_status.code)))
-                                return -ENOMEM;
-
-                        if (env_source->main_exec_status.code == CLD_EXITED) {
-                                r = strextendf(&var, ",EXIT_STATUS=%i",
-                                               env_source->main_exec_status.status);
-                                if (r < 0)
-                                        return r;
-                        } else if (!strextend(&var, ",EXIT_STATUS=", signal_to_string(env_source->main_exec_status.status)))
-                                return -ENOMEM;
+         * N.B. if there are multiple services which could trigger 's' via OnFailure=
+         * or OnSuccess= then we return NULL. This is since we don't know from which
+         * one to propagate the exit status. */
+
+        UNIT_FOREACH_DEPENDENCY(other, UNIT(s), UNIT_ATOM_ON_FAILURE_OF) {
+                if (candidate) {
+                        log_unit_debug(UNIT(s), "multiple trigger source candidates for exit status propagation, skipping.");
+                        return NULL;
                 }
 
-                if (!sd_id128_is_null(UNIT(env_source)->invocation_id)) {
-                        r = strextendf(&var, ",INVOCATION_ID=" SD_ID128_FORMAT_STR,
-                                       SD_ID128_FORMAT_VAL(UNIT(env_source)->invocation_id));
-                        if (r < 0)
-                                return r;
+                candidate = other;
+        }
+
+        UNIT_FOREACH_DEPENDENCY(other, UNIT(s), UNIT_ATOM_ON_SUCCESS_OF) {
+                if (candidate) {
+                        log_unit_debug(UNIT(s), "multiple trigger source candidates for exit status propagation, skipping.");
+                        return NULL;
                 }
 
-                if (!strextend(&var, ",UNIT=", UNIT(env_source)->id))
-                        return -ENOMEM;
+                candidate = other;
         }
 
-        *ret = TAKE_PTR(var);
-        return 0;
+        return SERVICE(candidate);
 }
 
 static int service_spawn(
@@ -1588,7 +1544,7 @@ static int service_spawn(
         if (r < 0)
                 return r;
 
-        our_env = new0(char*, 10);
+        our_env = new0(char*, 12);
         if (!our_env)
                 return -ENOMEM;
 
@@ -1645,30 +1601,44 @@ static int service_spawn(
                 }
         }
 
+        Service *env_source = NULL;
+        const char *monitor_prefix;
         if (flags & EXEC_SETENV_RESULT) {
-                if (asprintf(our_env + n_env++, "SERVICE_RESULT=%s", service_result_to_string(s->result)) < 0)
+                env_source = s;
+                monitor_prefix = "";
+        } else if (flags & EXEC_SETENV_MONITOR_RESULT) {
+                env_source = service_get_triggering_service(s);
+                monitor_prefix = "MONITOR_";
+        }
+
+        if (env_source) {
+                if (asprintf(our_env + n_env++, "%sSERVICE_RESULT=%s", monitor_prefix, service_result_to_string(env_source->result)) < 0)
                         return -ENOMEM;
 
-                if (s->main_exec_status.pid > 0 &&
-                    dual_timestamp_is_set(&s->main_exec_status.exit_timestamp)) {
-                        if (asprintf(our_env + n_env++, "EXIT_CODE=%s", sigchld_code_to_string(s->main_exec_status.code)) < 0)
+                if (env_source->main_exec_status.pid > 0 &&
+                    dual_timestamp_is_set(&env_source->main_exec_status.exit_timestamp)) {
+                        if (asprintf(our_env + n_env++, "%sEXIT_CODE=%s", monitor_prefix, sigchld_code_to_string(env_source->main_exec_status.code)) < 0)
                                 return -ENOMEM;
 
-                        if (s->main_exec_status.code == CLD_EXITED)
-                                r = asprintf(our_env + n_env++, "EXIT_STATUS=%i", s->main_exec_status.status);
+                        if (env_source->main_exec_status.code == CLD_EXITED)
+                                r = asprintf(our_env + n_env++, "%sEXIT_STATUS=%i", monitor_prefix, env_source->main_exec_status.status);
                         else
-                                r = asprintf(our_env + n_env++, "EXIT_STATUS=%s", signal_to_string(s->main_exec_status.status));
+                                r = asprintf(our_env + n_env++, "%sEXIT_STATUS=%s", monitor_prefix, signal_to_string(env_source->main_exec_status.status));
 
                         if (r < 0)
                                 return -ENOMEM;
                 }
 
-        } else if (flags & EXEC_SETENV_MONITOR_RESULT) {
-                Job *j = UNIT(s)->job;
-                if (j) {
-                        r = service_create_monitor_md_env(j, our_env + n_env++);
-                        if (r < 0)
-                                return r;
+                if (env_source != s) {
+                        if (!sd_id128_is_null(UNIT(env_source)->invocation_id)) {
+                                r = asprintf(our_env + n_env++, "%sINVOCATION_ID=" SD_ID128_FORMAT_STR,
+                                             monitor_prefix, SD_ID128_FORMAT_VAL(UNIT(env_source)->invocation_id));
+                                if (r < 0)
+                                        return -ENOMEM;
+                        }
+
+                        if (asprintf(our_env + n_env++, "%sUNIT=%s", monitor_prefix, UNIT(env_source)->id) < 0)
+                                return -ENOMEM;
                 }
         }
 
diff --git a/src/core/unit-dependency-atom.c b/src/core/unit-dependency-atom.c
index 333eea6c3d..81333523e7 100644
--- a/src/core/unit-dependency-atom.c
+++ b/src/core/unit-dependency-atom.c
@@ -82,13 +82,11 @@ static const UnitDependencyAtom atom_map[_UNIT_DEPENDENCY_MAX] = {
         [UNIT_PROPAGATES_STOP_TO]     = UNIT_ATOM_RETROACTIVE_STOP_ON_STOP |
                                         UNIT_ATOM_PROPAGATE_STOP,
 
-        [UNIT_ON_FAILURE]             = UNIT_ATOM_ON_FAILURE |
-                                        UNIT_ATOM_BACK_REFERENCE_IMPLIED,
-
-        [UNIT_ON_SUCCESS]             = UNIT_ATOM_ON_SUCCESS |
-                                        UNIT_ATOM_BACK_REFERENCE_IMPLIED,
-
         /* These are simple dependency types: they consist of a single atom only */
+        [UNIT_ON_FAILURE]             = UNIT_ATOM_ON_FAILURE,
+        [UNIT_ON_SUCCESS]             = UNIT_ATOM_ON_SUCCESS,
+        [UNIT_ON_FAILURE_OF]          = UNIT_ATOM_ON_FAILURE_OF,
+        [UNIT_ON_SUCCESS_OF]          = UNIT_ATOM_ON_SUCCESS_OF,
         [UNIT_BEFORE]                 = UNIT_ATOM_BEFORE,
         [UNIT_AFTER]                  = UNIT_ATOM_AFTER,
         [UNIT_TRIGGERS]               = UNIT_ATOM_TRIGGERS,
@@ -104,8 +102,6 @@ static const UnitDependencyAtom atom_map[_UNIT_DEPENDENCY_MAX] = {
          * things discoverable/debuggable as they are the inverse dependencies to some of the above. As they
          * have no effect of their own, they all map to no atoms at all, i.e. the value 0. */
         [UNIT_RELOAD_PROPAGATED_FROM] = 0,
-        [UNIT_ON_SUCCESS_OF]          = 0,
-        [UNIT_ON_FAILURE_OF]          = 0,
         [UNIT_STOP_PROPAGATED_FROM]   = 0,
 };
 
@@ -200,17 +196,19 @@ UnitDependency unit_dependency_from_unique_atom(UnitDependencyAtom atom) {
         case UNIT_ATOM_PROPAGATE_STOP_FAILURE:
                 return UNIT_CONFLICTED_BY;
 
-        case UNIT_ATOM_ON_FAILURE |
-                UNIT_ATOM_BACK_REFERENCE_IMPLIED:
+        /* And now, the simple ones */
+
         case UNIT_ATOM_ON_FAILURE:
                 return UNIT_ON_FAILURE;
 
-        case UNIT_ATOM_ON_SUCCESS |
-                UNIT_ATOM_BACK_REFERENCE_IMPLIED:
         case UNIT_ATOM_ON_SUCCESS:
                 return UNIT_ON_SUCCESS;
 
-        /* And now, the simple ones */
+        case UNIT_ATOM_ON_SUCCESS_OF:
+                return UNIT_ON_SUCCESS_OF;
+
+        case UNIT_ATOM_ON_FAILURE_OF:
+                return UNIT_ON_FAILURE_OF;
 
         case UNIT_ATOM_BEFORE:
                 return UNIT_BEFORE;
diff --git a/src/core/unit-dependency-atom.h b/src/core/unit-dependency-atom.h
index 1cf9795786..02532e57d6 100644
--- a/src/core/unit-dependency-atom.h
+++ b/src/core/unit-dependency-atom.h
@@ -66,27 +66,22 @@ typedef enum UnitDependencyAtom {
         /* Recheck default target deps on other units (which are target units) */
         UNIT_ATOM_DEFAULT_TARGET_DEPENDENCIES         = UINT64_C(1) << 21,
 
-        /* Dependencies which include this atom automatically get a reverse
-         * REFERENCES/REFERENCED_BY dependency. */
-        UNIT_ATOM_BACK_REFERENCE_IMPLIED              = UINT64_C(1) << 22,
-
-        /* Trigger a dependency on successful service exit. */
-        UNIT_ATOM_ON_SUCCESS                          = UINT64_C(1) << 23,
-        /* Trigger a dependency on unsuccessful service exit. */
-        UNIT_ATOM_ON_FAILURE                          = UINT64_C(1) << 24,
-
         /* The remaining atoms map 1:1 to the equally named high-level deps */
-        UNIT_ATOM_BEFORE                              = UINT64_C(1) << 25,
-        UNIT_ATOM_AFTER                               = UINT64_C(1) << 26,
-        UNIT_ATOM_TRIGGERS                            = UINT64_C(1) << 27,
-        UNIT_ATOM_TRIGGERED_BY                        = UINT64_C(1) << 28,
-        UNIT_ATOM_PROPAGATES_RELOAD_TO                = UINT64_C(1) << 29,
-        UNIT_ATOM_JOINS_NAMESPACE_OF                  = UINT64_C(1) << 30,
-        UNIT_ATOM_REFERENCES                          = UINT64_C(1) << 31,
-        UNIT_ATOM_REFERENCED_BY                       = UINT64_C(1) << 32,
-        UNIT_ATOM_IN_SLICE                            = UINT64_C(1) << 33,
-        UNIT_ATOM_SLICE_OF                            = UINT64_C(1) << 34,
-        _UNIT_DEPENDENCY_ATOM_MAX                     = (UINT64_C(1) << 35) - 1,
+        UNIT_ATOM_ON_FAILURE                          = UINT64_C(1) << 22,
+        UNIT_ATOM_ON_SUCCESS                          = UINT64_C(1) << 23,
+        UNIT_ATOM_ON_FAILURE_OF                       = UINT64_C(1) << 24,
+        UNIT_ATOM_ON_SUCCESS_OF                       = UINT64_C(1) << 25,
+        UNIT_ATOM_BEFORE                              = UINT64_C(1) << 26,
+        UNIT_ATOM_AFTER                               = UINT64_C(1) << 27,
+        UNIT_ATOM_TRIGGERS                            = UINT64_C(1) << 28,
+        UNIT_ATOM_TRIGGERED_BY                        = UINT64_C(1) << 29,
+        UNIT_ATOM_PROPAGATES_RELOAD_TO                = UINT64_C(1) << 30,
+        UNIT_ATOM_JOINS_NAMESPACE_OF                  = UINT64_C(1) << 31,
+        UNIT_ATOM_REFERENCES                          = UINT64_C(1) << 32,
+        UNIT_ATOM_REFERENCED_BY                       = UINT64_C(1) << 33,
+        UNIT_ATOM_IN_SLICE                            = UINT64_C(1) << 34,
+        UNIT_ATOM_SLICE_OF                            = UINT64_C(1) << 35,
+        _UNIT_DEPENDENCY_ATOM_MAX                     = (UINT64_C(1) << 36) - 1,
         _UNIT_DEPENDENCY_ATOM_INVALID                 = -EINVAL,
 } UnitDependencyAtom;
 
diff --git a/src/core/unit.c b/src/core/unit.c
index 2cddc924f3..af6cf097fc 100644
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -2222,24 +2222,17 @@ void unit_start_on_failure(
 
         UNIT_FOREACH_DEPENDENCY(other, u, atom) {
                 _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL;
-                Job *job = NULL;
 
                 if (!logged) {
                         log_unit_info(u, "Triggering %s dependencies.", dependency_name);
                         logged = true;
                 }
 
-                r = manager_add_job(u->manager, JOB_START, other, job_mode, NULL, &error, &job);
+                r = manager_add_job(u->manager, JOB_START, other, job_mode, NULL, &error, NULL);
                 if (r < 0)
                         log_unit_warning_errno(
                                         u, r, "Failed to enqueue %s job, ignoring: %s",
                                         dependency_name, bus_error_message(&error, r));
-                else if (job)
-                        /* u will be kept pinned since both UNIT_ON_FAILURE and UNIT_ON_SUCCESS includes
-                         * UNIT_ATOM_BACK_REFERENCE_IMPLIED. We save the triggering unit here since we
-                         * want to be able to reference it when we come to run the OnFailure= or OnSuccess=
-                         * dependency. */
-                        job_add_triggering_unit(job, u);
         }
 
         if (logged)
@@ -3121,20 +3114,6 @@ int unit_add_dependency(
                         noop = false;
         }
 
-        if (FLAGS_SET(a, UNIT_ATOM_BACK_REFERENCE_IMPLIED)) {
-                r = unit_add_dependency_hashmap(&other->dependencies, UNIT_REFERENCES, u, 0, mask);
-                if (r < 0)
-                        return r;
-                if (r)
-                        noop = false;
-
-                r = unit_add_dependency_hashmap(&u->dependencies, UNIT_REFERENCED_BY, other, 0, mask);
-                if (r < 0)
-                        return r;
-                if (r)
-                        noop = false;
-        }
-
         if (add_reference) {
                 r = unit_add_dependency_hashmap(&u->dependencies, UNIT_REFERENCES, other, mask, 0);
                 if (r < 0)
diff --git a/src/core/unit.h b/src/core/unit.h
index 786c15d623..94f2180951 100644
--- a/src/core/unit.h
+++ b/src/core/unit.h
@@ -242,9 +242,6 @@ typedef struct Unit {
         /* Queue of units that have a BindTo= dependency on some other unit, and should possibly be shut down */
         LIST_FIELDS(Unit, stop_when_bound_queue);
 
-        /* Queue of units which have triggered an OnFailure= or OnSuccess= dependency job. */
-        LIST_FIELDS(Unit, triggered_by);
-
         /* PIDs we keep an eye on. Note that a unit might have many
          * more, but these are the ones we care enough about to
          * process SIGCHLD for */
diff --git a/src/test/test-engine.c b/src/test/test-engine.c
index 673c665612..473e896acc 100644
--- a/src/test/test-engine.c
+++ b/src/test/test-engine.c
@@ -223,6 +223,7 @@ int main(int argc, char *argv[]) {
         assert_se(unit_add_dependency_by_name(stub, UNIT_AFTER, SPECIAL_ROOT_SLICE, true, UNIT_DEPENDENCY_FILE) >= 0);
         assert_se(unit_add_dependency_by_name(stub, UNIT_REQUIRES, "non-existing.mount", true, UNIT_DEPENDENCY_FILE) >= 0);
         assert_se(unit_add_dependency_by_name(stub, UNIT_ON_FAILURE, "non-existing-on-failure.target", true, UNIT_DEPENDENCY_FILE) >= 0);
+        assert_se(unit_add_dependency_by_name(stub, UNIT_ON_SUCCESS, "non-existing-on-success.target", true, UNIT_DEPENDENCY_FILE) >= 0);
 
         log_info("/* Merging a+stub, dumps before */");
         unit_dump(a, stderr, NULL);
@@ -237,7 +238,13 @@ int main(int argc, char *argv[]) {
         assert_se(unit_has_dependency(a, UNIT_ATOM_PULL_IN_START, manager_get_unit(m, "non-existing.mount")));
         assert_se(unit_has_dependency(a, UNIT_ATOM_RETROACTIVE_START_REPLACE, manager_get_unit(m, "non-existing.mount")));
         assert_se(unit_has_dependency(a, UNIT_ATOM_ON_FAILURE, manager_get_unit(m, "non-existing-on-failure.target")));
+        assert_se(unit_has_dependency(manager_get_unit(m, "non-existing-on-failure.target"), UNIT_ATOM_ON_FAILURE_OF, a));
+        assert_se(unit_has_dependency(a, UNIT_ATOM_ON_SUCCESS, manager_get_unit(m, "non-existing-on-success.target")));
+        assert_se(unit_has_dependency(manager_get_unit(m, "non-existing-on-success.target"), UNIT_ATOM_ON_SUCCESS_OF, a));
         assert_se(!unit_has_dependency(a, UNIT_ATOM_ON_FAILURE, manager_get_unit(m, "basic.target")));
+        assert_se(!unit_has_dependency(a, UNIT_ATOM_ON_SUCCESS, manager_get_unit(m, "basic.target")));
+        assert_se(!unit_has_dependency(a, UNIT_ATOM_ON_FAILURE_OF, manager_get_unit(m, "basic.target")));
+        assert_se(!unit_has_dependency(a, UNIT_ATOM_ON_SUCCESS_OF, manager_get_unit(m, "basic.target")));
         assert_se(!unit_has_dependency(a, UNIT_ATOM_PROPAGATES_RELOAD_TO, manager_get_unit(m, "non-existing-on-failure.target")));
 
         assert_se(unit_has_name(a, "a.service"));