core/service: actually allow to "hurry up" auto restarts

unit_start() advertises that start requests don't get suppressed,
so that it could be used to manually speed up auto restarts.
However, service_start() so far rejected this, stating that
clients should issue restart request in order to trigger
BindsTo=/OnFailure=.

That seems to be a red herring though, because for a long time
the service states between auto-restarts were buggy (#27594).
With the introduction of RestartMode=direct, the behavior
is sane again and customizable, hence I see no reason to refuse
this anymore. Whether those deps are triggered solely depends
on RestartMode= now.

Plus, filter out some intermediate states that should never
be seen in service_start().

Fixes #33890

diff --git a/src/core/service.c b/src/core/service.c
index 2c6d83babe5692e4f0d367c86421582cc7ade587..49ed3b088a749357af2fd05c062867f2ef5114c6 100644 (file)
--- a/src/core/service.c
+++ b/src/core/service.c
@@ -2567,13 +2567,16 @@ fail:
         service_enter_dead(s, SERVICE_FAILURE_RESOURCES, /* allow_restart= */ true);
 }
 
-static void service_enter_restart(Service *s) {
+static void service_enter_restart(Service *s, bool shortcut) {
         _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL;
         int r;
 
+        /* shortcut: a manual start request is received, restart immediately */
+
         assert(s);
+        assert(s->state == SERVICE_AUTO_RESTART);
 
-        if (unit_has_job_type(UNIT(s), JOB_STOP)) {
+        if (!shortcut && unit_has_job_type(UNIT(s), JOB_STOP)) {
                 /* Don't restart things if we are going down anyway */
                 log_unit_info(UNIT(s), "Stop job pending for unit, skipping automatic restart.");
                 return;
@@ -2598,7 +2601,8 @@ static void service_enter_restart(Service *s) {
                         "MESSAGE_ID=" SD_MESSAGE_UNIT_RESTART_SCHEDULED_STR,
                         LOG_UNIT_INVOCATION_ID(UNIT(s)),
                         LOG_UNIT_MESSAGE(UNIT(s),
-                                         "Scheduled restart job, restart counter is at %u.", s->n_restarts),
+                                         "Scheduled restart job%s, restart counter is at %u.",
+                                         shortcut ? " immediately on client request" : "", s->n_restarts),
                         "N_RESTARTS=%u", s->n_restarts);
 
         service_set_state(s, SERVICE_AUTO_RESTART_QUEUED);
@@ -2758,11 +2762,9 @@ static void service_run_next_main(Service *s) {
 }
 
 static int service_start(Unit *u) {
-        Service *s = SERVICE(u);
+        Service *s = ASSERT_PTR(SERVICE(u));
         int r;
 
-        assert(s);
-
         /* We cannot fulfill this request right now, try again later
          * please! */
         if (IN_SET(s->state,
@@ -2774,13 +2776,17 @@ static int service_start(Unit *u) {
         if (IN_SET(s->state, SERVICE_CONDITION, SERVICE_START_PRE, SERVICE_START, SERVICE_START_POST))
                 return 0;
 
-        /* A service that will be restarted must be stopped first to trigger BindsTo and/or OnFailure
-         * dependencies. If a user does not want to wait for the holdoff time to elapse, the service should
-         * be manually restarted, not started. We simply return EAGAIN here, so that any start jobs stay
-         * queued, and assume that the auto restart timer will eventually trigger the restart. */
-        if (IN_SET(s->state, SERVICE_AUTO_RESTART, SERVICE_DEAD_BEFORE_AUTO_RESTART, SERVICE_FAILED_BEFORE_AUTO_RESTART))
+        if (s->state == SERVICE_AUTO_RESTART) {
+                /* As mentioned in unit_start(), we allow manual starts to act as "hurry up" signals
+                 * for auto restart. We need to re-enqueue the job though, as the job type has changed
+                 * (JOB_RESTART_DEPENDENCIES). */
+
+                service_enter_restart(s, /* shortcut = */ true);
                 return -EAGAIN;
+        }
 
+        /* SERVICE_*_BEFORE_AUTO_RESTART are not to be expected here, as those are intermediate states
+         * that should never be seen outside of service_enter_dead(). */
         assert(IN_SET(s->state, SERVICE_DEAD, SERVICE_FAILED, SERVICE_DEAD_RESOURCES_PINNED, SERVICE_AUTO_RESTART_QUEUED));
 
         r = unit_acquire_invocation_id(u);
@@ -4290,7 +4296,7 @@ static int service_dispatch_timer(sd_event_source *source, usec_t usec, void *us
                         log_unit_debug(UNIT(s),
                                        "Service has no hold-off time (RestartSec=0), scheduling restart.");
 
-                service_enter_restart(s);
+                service_enter_restart(s, /* shortcut = */ false);
                 break;
 
         case SERVICE_CLEANING:

diff --git a/test/units/TEST-03-JOBS.sh b/test/units/TEST-03-JOBS.sh
index 115b941f926826781a5f70ca8500014bd7194b34..067f2ce67defb28685f93e2a0da27bec26067da5 100755 (executable)
--- a/test/units/TEST-03-JOBS.sh
+++ b/test/units/TEST-03-JOBS.sh
@@ -1,5 +1,7 @@
 #!/usr/bin/env bash
 # SPDX-License-Identifier: LGPL-2.1-or-later
+# shellcheck disable=SC2016
+
 set -eux
 set -o pipefail
 
@@ -166,4 +168,30 @@ assert_rc 3 systemctl --quiet is-active succeeds-on-restart.target
 systemctl start fails-on-restart.target || :
 assert_rc 3 systemctl --quiet is-active fails-on-restart.target
 
+# Test shortcutting auto restart
+
+export UNIT_NAME="TEST-03-JOBS-shortcut-restart.service"
+TMP_FILE="/tmp/test-03-shortcut-restart-test$RANDOM"
+
+cat >"/run/systemd/system/$UNIT_NAME" <<EOF
+[Service]
+Type=oneshot
+ExecStart=rm -v "$TMP_FILE"
+Restart=on-failure
+RestartSec=1d
+RemainAfterExit=yes
+EOF
+
+(! systemctl start "$UNIT_NAME")
+timeout 10 bash -c 'while [[ "$(systemctl show "$UNIT_NAME" -P SubState)" != "auto-restart" ]]; do sleep .5; done'
+touch "$TMP_FILE"
+assert_eq "$(systemctl show "$UNIT_NAME" -P SubState)" "auto-restart"
+
+timeout 30 systemctl start "$UNIT_NAME"
+systemctl --quiet is-active "$UNIT_NAME"
+assert_eq "$(systemctl show "$UNIT_NAME" -P NRestarts)" "1"
+[[ ! -f "$TMP_FILE" ]]
+
+rm /run/systemd/system/"$UNIT_NAME"
+
 touch /testok