Merge pull request #27451 from yuwata/core-path-trigger-notify

core/path: do not install new job in .trigger_notify()

src/core/path.c

@@ -10,6 +10,7 @@
 #include "dbus-path.h"
 #include "dbus-unit.h"
 #include "escape.h"
+#include "event-util.h"
 #include "fd-util.h"
 #include "glob-util.h"
 #include "inotify-util.h"
@@ -26,10 +27,10 @@
 #include "unit.h"
 
 static const UnitActiveState state_translation_table[_PATH_STATE_MAX] = {
-        [PATH_DEAD] = UNIT_INACTIVE,
+        [PATH_DEAD]    = UNIT_INACTIVE,
         [PATH_WAITING] = UNIT_ACTIVE,
         [PATH_RUNNING] = UNIT_ACTIVE,
-        [PATH_FAILED] = UNIT_FAILED,
+        [PATH_FAILED]  = UNIT_FAILED,
 };
 
 static int path_dispatch_io(sd_event_source *source, int fd, uint32_t revents, void *userdata);
@@ -300,6 +301,7 @@ static void path_done(Unit *u) {
 
         assert(p);
 
+        p->trigger_notify_event_source = sd_event_source_disable_unref(p->trigger_notify_event_source);
         path_free_specs(p);
 }
 
@@ -575,6 +577,9 @@ static void path_enter_waiting(Path *p, bool initial, bool from_trigger_notify)
         Unit *trigger;
         int r;
 
+        if (p->trigger_notify_event_source)
+                (void) event_source_disable(p->trigger_notify_event_source);
+
         /* If the triggered unit is already running, so are we */
         trigger = UNIT_TRIGGER(UNIT(p));
         if (trigger && !UNIT_IS_INACTIVE_OR_FAILED(unit_active_state(trigger))) {
@@ -799,8 +804,28 @@ fail:
         return 0;
 }
 
-static void path_trigger_notify(Unit *u, Unit *other) {
+static void path_trigger_notify_impl(Unit *u, Unit *other, bool on_defer);
+
+static int path_trigger_notify_on_defer(sd_event_source *s, void *userdata) {
+        Path *p = ASSERT_PTR(userdata);
+        Unit *trigger;
+
+        assert(s);
+
+        trigger = UNIT_TRIGGER(UNIT(p));
+        if (!trigger) {
+                log_unit_error(UNIT(p), "Unit to trigger vanished.");
+                path_enter_dead(p, PATH_FAILURE_RESOURCES);
+                return 0;
+        }
+
+        path_trigger_notify_impl(UNIT(p), trigger, /* on_defer = */ true);
+        return 0;
+}
+
+static void path_trigger_notify_impl(Unit *u, Unit *other, bool on_defer) {
         Path *p = PATH(u);
+        int r;
 
         assert(u);
         assert(other);
@@ -826,13 +851,46 @@ static void path_trigger_notify(Unit *u, Unit *other) {
 
         if (p->state == PATH_RUNNING &&
             UNIT_IS_INACTIVE_OR_FAILED(unit_active_state(other))) {
-                log_unit_debug(UNIT(p), "Got notified about unit deactivation.");
-                path_enter_waiting(p, false, true);
+                if (!on_defer)
+                        log_unit_debug(u, "Got notified about unit deactivation.");
         } else if (p->state == PATH_WAITING &&
                    !UNIT_IS_INACTIVE_OR_FAILED(unit_active_state(other))) {
-                log_unit_debug(UNIT(p), "Got notified about unit activation.");
-                path_enter_waiting(p, false, true);
+                if (!on_defer)
+                        log_unit_debug(u, "Got notified about unit activation.");
+        } else
+                return;
+
+        if (on_defer) {
+                path_enter_waiting(p, /* initial = */ false, /* from_trigger_notify = */ true);
+                return;
         }
+
+        /* Do not call path_enter_waiting() directly from path_trigger_notify(), as this may be called by
+         * job_install() -> job_finish_and_invalidate() -> unit_trigger_notify(), and path_enter_waiting()
+         * may install another job and will trigger assertion in job_install().
+         * https://github.com/systemd/systemd/issues/24577#issuecomment-1522628906
+         * Hence, first setup defer event source here, and call path_enter_waiting() slightly later. */
+        if (p->trigger_notify_event_source) {
+                r = sd_event_source_set_enabled(p->trigger_notify_event_source, SD_EVENT_ONESHOT);
+                if (r < 0) {
+                        log_unit_warning_errno(u, r, "Failed to enable event source for triggering notify: %m");
+                        path_enter_dead(p, PATH_FAILURE_RESOURCES);
+                        return;
+                }
+        } else {
+                r = sd_event_add_defer(u->manager->event, &p->trigger_notify_event_source, path_trigger_notify_on_defer, p);
+                if (r < 0) {
+                        log_unit_warning_errno(u, r, "Failed to allocate event source for triggering notify: %m");
+                        path_enter_dead(p, PATH_FAILURE_RESOURCES);
+                        return;
+                }
+
+                (void) sd_event_source_set_description(p->trigger_notify_event_source, "path-trigger-notify");
+        }
+}
+
+static void path_trigger_notify(Unit *u, Unit *other) {
+        path_trigger_notify_impl(u, other, /* on_defer = */ false);
 }
 
 static void path_reset_failed(Unit *u) {

src/core/path.h

@@ -65,6 +65,8 @@ struct Path {
         PathResult result;
 
         RateLimit trigger_limit;
+
+        sd_event_source *trigger_notify_event_source;
 };
 
 struct ActivationDetailsPath {

test/testsuite-63.units/test63-issue-24577-dep.service

@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: LGPL-2.1-or-later
+[Service]
+Type=oneshot
+ExecStart=bash -c 'sleep infinity'

test/testsuite-63.units/test63-issue-24577.path

@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: LGPL-2.1-or-later
+[Path]
+PathExists=/tmp/hoge

test/testsuite-63.units/test63-issue-24577.service

@@ -0,0 +1,8 @@
+# SPDX-License-Identifier: LGPL-2.1-or-later
+[Unit]
+Requires=test63-issue-24577-dep.service
+After=test63-issue-24577-dep.service
+
+[Service]
+Type=oneshot
+ExecStart=bash -c 'sleep infinity'

test/units/testsuite-33.sh

@@ -5,7 +5,7 @@
 set -eux
 set -o pipefail
 
-cat >/etc/systemd/system/test-service.service <<EOF
+cat >/run/systemd/system/test-service.service <<EOF
 [Service]
 ConfigurationDirectory=test-service
 RuntimeDirectory=test-service
@@ -75,7 +75,7 @@ test ! -e /var/lib/test-service
 test ! -e /var/cache/test-service
 test ! -e /var/log/test-service
 
-cat >/etc/systemd/system/test-service.service <<EOF
+cat >/run/systemd/system/test-service.service <<EOF
 [Service]
 DynamicUser=yes
 ConfigurationDirectory=test-service
@@ -170,7 +170,7 @@ test ! -L /var/lib/test-service
 test ! -L /var/cache/test-service
 test ! -L /var/log/test-service
 
-cat >/etc/systemd/system/tmp-hoge.mount <<EOF
+cat >/run/systemd/system/tmp-hoge.mount <<EOF
 [Mount]
 What=tmpfs
 Type=tmpfs
@@ -245,7 +245,7 @@ test ! -d /var/lib/hoge
 test ! -d /var/cache/hoge
 test ! -d /var/log/hoge
 
-cat >/etc/systemd/system/test-service.socket <<EOF
+cat >/run/systemd/system/test-service.socket <<EOF
 [Socket]
 ListenSequentialPacket=/run/test-service.socket
 RemoveOnStop=yes

test/units/testsuite-63.sh

@@ -3,6 +3,9 @@
 set -ex
 set -o pipefail
 
+# shellcheck source=test/units/assert.sh
+. "$(dirname "$0")"/assert.sh
+
 systemctl log-level debug
 
 # Test that a path unit continuously triggering a service that fails condition checks eventually fails with
@@ -41,6 +44,42 @@ systemctl stop test63-glob.path test63-glob.service
 
 test "$(busctl --json=short get-property org.freedesktop.systemd1 /org/freedesktop/systemd1/unit/test63_2dglob_2eservice org.freedesktop.systemd1.Unit ActivationDetails)" = '{"type":"a(ss)","data":[]}'
 
+# tests for issue https://github.com/systemd/systemd/issues/24577#issuecomment-1522628906
+rm -f /tmp/hoge
+systemctl start test63-issue-24577.path
+systemctl status -n 0 test63-issue-24577.path
+systemctl status -n 0 test63-issue-24577.service || :
+systemctl list-jobs
+output=$(systemctl list-jobs --no-legend)
+assert_not_in "test63-issue-24577.service" "$output"
+assert_not_in "test63-issue-24577-dep.service" "$output"
+
+touch /tmp/hoge
+systemctl status -n 0 test63-issue-24577.path
+systemctl status -n 0 test63-issue-24577.service || :
+systemctl list-jobs
+output=$(systemctl list-jobs --no-legend)
+assert_in "test63-issue-24577.service" "$output"
+assert_in "test63-issue-24577-dep.service" "$output"
+
+# even if the service is stopped, it will be soon retriggered.
+systemctl stop test63-issue-24577.service
+systemctl status -n 0 test63-issue-24577.path
+systemctl status -n 0 test63-issue-24577.service || :
+systemctl list-jobs
+output=$(systemctl list-jobs --no-legend)
+assert_in "test63-issue-24577.service" "$output"
+assert_in "test63-issue-24577-dep.service" "$output"
+
+rm -f /tmp/hoge
+systemctl stop test63-issue-24577.service
+systemctl status -n 0 test63-issue-24577.path
+systemctl status -n 0 test63-issue-24577.service || :
+systemctl list-jobs
+output=$(systemctl list-jobs --no-legend)
+assert_not_in "test63-issue-24577.service" "$output"
+assert_in "test63-issue-24577-dep.service" "$output"
+
 systemctl log-level info
 
 echo OK >/testok