broker: pass serial numbers with activations

Extend the activation communication between broker and launcher to use
serial numbers. This allows us to discard stale activation failures and
closes a race condition where we would incorrectly fail new activations
immediately after a previous service exited.

Reported-by: Luca Bruno <[email protected]>
Signed-off-by: David Rheinsberg <[email protected]>

docs/dbus-broker.rst

@@ -189,14 +189,22 @@ the broker. See the section below for a list of interfaces on the controller.
 |         **method** Release() -> ()
 |
 |         # Reset the activation state of this name. Any pending activation
-|         # requests are canceled.
-|         **method** Reset() -> ()
+|         # requests are canceled. The call requires a serial number to be
+|         # passed along. This must be the serial number received by the last
+|         # activation even on this name. Calls for other serial numbers are
+|         # silently ignored and considered stale.
+|         **method** Reset(**t** *serial*) -> ()
 |
 |         # This signal is sent whenever a client requests activation of this
 |         # name. Note that multiple activation requests are coalesced by the
 |         # broker. The controller can cancel outstanding requests via the
 |         # **Reset()** method.
-|         **signal** Activate()
+|         # The broker sends a serial number with the event. This number
+|         # represents the activation request and must be used when reacting
+|         # to the request with methods like *Reset()*. The serial number is
+|         # unique for each event, and is never reused. A serial number of 0
+|         # is never sent and considered invalid.
+|         **signal** Activate(**t** *serial*)
 |
 |     }
 | }

src/broker/controller-dbus.c

@@ -54,6 +54,13 @@ struct ControllerMethod {
                 _body                                   \
         )
 
+static const CDVarType controller_type_in_t[] = {
+        C_DVAR_T_INIT(
+                C_DVAR_T_TUPLE1(
+                        C_DVAR_T_t
+                )
+        )
+};
 static const CDVarType controller_type_in_v[] = {
         C_DVAR_T_INIT(
                 C_DVAR_T_TUPLE1(
@@ -304,7 +311,7 @@ static int controller_method_name_release(Controller *controller, const char *pa
         if (!name)
                 return CONTROLLER_E_NAME_NOT_FOUND;
 
-        r = controller_name_reset(name);
+        r = controller_name_reset(name, name->activation.pending);
         if (r)
                 return error_trace(r);
 
@@ -352,9 +359,10 @@ static int controller_method_listener_set_policy(Controller *controller, const c
 
 static int controller_method_name_reset(Controller *controller, const char *path, CDVar *in_v, FDList *fds, CDVar *out_v) {
         ControllerName *name;
+        uint64_t serial;
         int r;
 
-        c_dvar_read(in_v, "()");
+        c_dvar_read(in_v, "(t)", &serial);
 
         r = controller_end_read(in_v);
         if (r)
@@ -364,7 +372,7 @@ static int controller_method_name_reset(Controller *controller, const char *path
         if (!name)
                 return CONTROLLER_E_NAME_NOT_FOUND;
 
-        r = controller_name_reset(name);
+        r = controller_name_reset(name, serial);
         if (r)
                 return error_trace(r);
 
@@ -457,7 +465,7 @@ static int controller_dispatch_controller(Controller *controller, uint32_t seria
 
 static int controller_dispatch_name(Controller *controller, uint32_t serial, const char *method, const char *path, const char *signature, Message *message) {
         static const ControllerMethod methods[] = {
-                { "Reset",      controller_method_name_reset,   c_dvar_type_unit,       controller_type_out_unit },
+                { "Reset",      controller_method_name_reset,   controller_type_in_t,   controller_type_out_unit },
                 { "Release",    controller_method_name_release, c_dvar_type_unit,       controller_type_out_unit },
         };
 
@@ -636,11 +644,13 @@ int controller_dbus_dispatch(Controller *controller, Message *message) {
 /**
  * controller_dbus_send_activation() - XXX
  */
-int controller_dbus_send_activation(Controller *controller, const char *path) {
+int controller_dbus_send_activation(Controller *controller, const char *path, uint64_t serial) {
         static const CDVarType type[] = {
                 C_DVAR_T_INIT(
                         CONTROLLER_T_MESSAGE(
-                                C_DVAR_T_TUPLE0
+                                C_DVAR_T_TUPLE1(
+                                        C_DVAR_T_t
+                                )
                         )
                 )
         };
@@ -651,11 +661,13 @@ int controller_dbus_send_activation(Controller *controller, const char *path) {
         int r;
 
         c_dvar_begin_write(&var, (__BYTE_ORDER == __BIG_ENDIAN), type, 1);
-        c_dvar_write(&var, "((yyyyuu[(y<o>)(y<s>)(y<s>)])())",
+        c_dvar_write(&var, "((yyyyuu[(y<o>)(y<s>)(y<s>)(y<g>)])(t))",
                      c_dvar_is_big_endian(&var) ? 'B' : 'l', DBUS_MESSAGE_TYPE_SIGNAL, DBUS_HEADER_FLAG_NO_REPLY_EXPECTED, 1, 0, (uint32_t)-1,
                      DBUS_MESSAGE_FIELD_PATH, c_dvar_type_o, path,
                      DBUS_MESSAGE_FIELD_INTERFACE, c_dvar_type_s, "org.bus1.DBus.Name",
-                     DBUS_MESSAGE_FIELD_MEMBER, c_dvar_type_s, "Activate");
+                     DBUS_MESSAGE_FIELD_MEMBER, c_dvar_type_s, "Activate",
+                     DBUS_MESSAGE_FIELD_SIGNATURE, c_dvar_type_g, "t",
+                     serial);
 
         r = c_dvar_end_write(&var, &data, &n_data);
         if (r)

src/broker/controller.c

@@ -66,10 +66,10 @@ static int controller_name_new(ControllerName **namep, Controller *controller, c
 /**
  * controller_name_reset() - XXX
  */
-int controller_name_reset(ControllerName *name) {
+int controller_name_reset(ControllerName *name, uint64_t serial) {
         int r;
 
-        r = driver_name_activation_failed(&name->controller->broker->bus, &name->activation);
+        r = driver_name_activation_failed(&name->controller->broker->bus, &name->activation, serial);
         if (r)
                 return error_fold(r);
 
@@ -79,8 +79,8 @@ int controller_name_reset(ControllerName *name) {
 /**
  * controller_name_activate() - XXX
  */
-int controller_name_activate(ControllerName *name) {
-        return controller_dbus_send_activation(name->controller, name->path);
+int controller_name_activate(ControllerName *name, uint64_t serial) {
+        return controller_dbus_send_activation(name->controller, name->path, serial);
 }
 
 static int controller_reload_compare(CRBTree *t, void *k, CRBNode *rb) {
@@ -311,7 +311,7 @@ int controller_add_name(Controller *controller,
         if (r)
                 return error_trace(r);
 
-        r = activation_init(&name->activation, name_entry, user_entry);
+        r = activation_init(&name->activation, &controller->broker->bus, name_entry, user_entry);
         if (r)
                 return (r == ACTIVATION_E_ALREADY_ACTIVATABLE) ? CONTROLLER_E_NAME_IS_ACTIVATABLE : error_fold(r);
 

src/broker/controller.h

@@ -101,8 +101,8 @@ struct Controller {
 /* names */
 
 ControllerName *controller_name_free(ControllerName *name);
-int controller_name_reset(ControllerName *name);
-int controller_name_activate(ControllerName *name);
+int controller_name_reset(ControllerName *name, uint64_t serial);
+int controller_name_activate(ControllerName *name, uint64_t serial);
 
 C_DEFINE_CLEANUP(ControllerName *, controller_name_free);
 
@@ -144,7 +144,7 @@ ControllerListener *controller_find_listener(Controller *controller, const char
 ControllerReload *controller_find_reload(Controller *controller, uint32_t serial);
 
 int controller_dbus_dispatch(Controller *controller, Message *message);
-int controller_dbus_send_activation(Controller *controller, const char *path);
+int controller_dbus_send_activation(Controller *controller, const char *path, uint64_t serial);
 int controller_dbus_send_reload(Controller *controller, User *user, uint32_t serial);
 int controller_dbus_send_environment(Controller *controller, const char * const *env, size_t n_env);
 

src/bus/activation.c

@@ -7,6 +7,7 @@
 #include <stdlib.h>
 #include "broker/controller.h"
 #include "bus/activation.h"
+#include "bus/bus.h"
 #include "bus/name.h"
 #include "bus/policy.h"
 #include "dbus/message.h"
@@ -48,13 +49,14 @@ C_DEFINE_CLEANUP(ActivationMessage *, activation_message_free);
 /**
  * activation_init() - XXX
  */
-int activation_init(Activation *a, Name *name, User *user) {
+int activation_init(Activation *a, Bus *bus, Name *name, User *user) {
         _c_cleanup_(activation_deinitp) Activation *activation = a;
 
         if (name->activation)
                 return ACTIVATION_E_ALREADY_ACTIVATABLE;
 
         *activation = (Activation)ACTIVATION_NULL(*activation);
+        activation->bus = bus;
         activation->name = name_ref(name);
         activation->user = user_ref(user);
 
@@ -113,14 +115,15 @@ void activation_get_stats_for(Activation *activation,
 static int activation_request(Activation *activation) {
         int r;
 
-        if (activation->requested)
+        if (activation->pending)
                 return 0;
 
-        r = controller_name_activate(CONTROLLER_NAME(activation));
+        r = controller_name_activate(CONTROLLER_NAME(activation),
+                                     ++activation->bus->activation_ids);
         if (r)
                 return error_fold(r);
 
-        activation->requested = true;
+        activation->pending = activation->bus->activation_ids;
         return 0;
 }
 

src/bus/activation.h

@@ -13,6 +13,7 @@
 typedef struct Activation Activation;
 typedef struct ActivationMessage ActivationMessage;
 typedef struct ActivationRequest ActivationRequest;
+typedef struct Bus Bus;
 typedef struct Message Message;
 typedef struct Name Name;
 typedef struct NameOwner NameOwner;
@@ -43,11 +44,12 @@ struct ActivationMessage {
 };
 
 struct Activation {
+        Bus *bus;
         Name *name;
         User *user;
         CList activation_messages;
         CList activation_requests;
-        bool requested : 1;
+        uint64_t pending;
 };
 
 #define ACTIVATION_NULL(_x) {                                                   \
@@ -65,7 +67,7 @@ ActivationMessage *activation_message_free(ActivationMessage *message);
 
 /* activation */
 
-int activation_init(Activation *activation, Name *name, User *user);
+int activation_init(Activation *activation, Bus *bus, Name *name, User *user);
 void activation_deinit(Activation *activation);
 
 void activation_get_stats_for(Activation *activation,

src/bus/bus.h

@@ -50,6 +50,7 @@ struct Bus {
 
         uint64_t n_monitors;
         uint64_t listener_ids;
+        uint64_t activation_ids;
 
         Metrics metrics;
 };

src/bus/driver.c

@@ -721,22 +721,19 @@ static int driver_name_owner_changed(Bus *bus, MatchRegistry *matches, const cha
         return 0;
 }
 
-int driver_name_activation_failed(Bus *bus, Activation *activation) {
+int driver_name_activation_failed(Bus *bus, Activation *activation, uint64_t serial) {
         ActivationRequest *request, *request_safe;
         ActivationMessage *message, *message_safe;
         int r;
 
-        /* in case the name is activated again in the future, we should request it again */
-        activation->requested = false;
-
         /*
-         * NOTE: We can get activation-failure notifications by the launcher
-         *       even when the name was already claimed. The launcher tracks
-         *       services for their entire lifetime.
-         *       We don't really care for late notifications so far, since both
-         *       the queues are empty in this case. But we need to be aware of
-         *       this if we extend this in the future.
+         * If there is no pending activation, or if it is an event for an old
+         * activation, we ignore it silently.
          */
+        if (!activation->pending || serial != activation->pending)
+                return 0;
+
+        activation->pending = 0;
 
         c_list_for_each_entry_safe(request, request_safe, &activation->activation_requests, link) {
                 Peer *sender;
@@ -776,7 +773,7 @@ static int driver_name_activated(Activation *activation, Peer *receiver) {
                 return 0;
 
         /* in case the name is dropped again in the future, we should request it again */
-        activation->requested = false;
+        activation->pending = 0;
 
         c_list_for_each_entry_safe(request, request_safe, &activation->activation_requests, link) {
                 Peer *sender;

src/bus/driver.h

@@ -63,7 +63,7 @@ enum {
         _DRIVER_E_MAX,
 };
 
-int driver_name_activation_failed(Bus *bus, Activation *activation);
+int driver_name_activation_failed(Bus *bus, Activation *activation, uint64_t serial);
 int driver_reload_config_completed(Bus *bus, uint64_t sender_id, uint32_t reply_serial);
 int driver_reload_config_invalid(Bus *bus, uint64_t sender_id, uint32_t reply_serial);
 

src/launch/launcher.c

@@ -386,8 +386,13 @@ static int launcher_fork(Launcher *launcher, int fd_controller) {
 
 static int launcher_on_name_activate(Launcher *launcher, sd_bus_message *m, const char *id) {
         Service *service;
+        uint64_t serial;
         int r;
 
+        r = sd_bus_message_read(m, "t", &serial);
+        if (r < 0)
+                return error_origin(r);
+
         service = c_rbtree_find_entry(&launcher->services,
                                       service_compare,
                                       id,
@@ -403,7 +408,7 @@ static int launcher_on_name_activate(Launcher *launcher, sd_bus_message *m, cons
                 return 0;
         }
 
-        r = service_activate(service);
+        r = service_activate(service, serial);
         if (r)
                 return error_fold(r);
 

src/launch/service.c

@@ -202,18 +202,6 @@ static int service_reset_activation(Service *service) {
          *      dbus error to each pending transaction.
          *      So far, we did not define such errors, so this is left for the
          *      future.
-         *
-         * XXX: Additionally, we would probably want a serial-number for each
-         *      activation request, which we then include in this reset. This
-         *      would properly order consequetive activation requests and allow
-         *      the broker to discard resets that come out-of-order.
-         *      However, this is quite unlikely to solve any real issue, since
-         *      there is no defined behavior of failed activation requests,
-         *      anyway. While they are recoverable in most situations, there is
-         *      no defined barrier at which point a followup should succeed.
-         *      Hence, this is left for the future, in case this situation gets
-         *      a well-defined behavior (though, needs coordination with the
-         *      reference implementation and systemd).
          */
         r = sd_bus_call_method(service->launcher->bus_controller,
                                NULL,
@@ -222,7 +210,8 @@ static int service_reset_activation(Service *service) {
                                "Reset",
                                NULL,
                                NULL,
-                               "");
+                               "t",
+                               service->last_serial);
         if (r < 0)
                 return error_origin(r);
 
@@ -794,6 +783,7 @@ static int service_start_transient_unit(Service *service) {
 /**
  * service_activate() - trigger a service activation
  * @service:            service to activate
+ * @serial:             activation serial number
  *
  * This activates the specified service. Any previous activation is discarded
  * silently. The new activation replaces a possible old one.
@@ -825,10 +815,11 @@ static int service_start_transient_unit(Service *service) {
  *
  * Returns: 0 on success, negative error code on failure.
  */
-int service_activate(Service *service) {
+int service_activate(Service *service, uint64_t serial) {
         int r;
 
         service_discard_activation(service);
+        service->last_serial = serial;
 
         if (!strcmp(service->name, "org.freedesktop.systemd1")) {
                 /*

src/launch/service.h

@@ -32,6 +32,7 @@ struct Service {
         uint64_t instance;
         uint64_t n_missing_unit;
         uint64_t n_masked_unit;
+        uint64_t last_serial;
         char id[];
 };
 
@@ -56,5 +57,5 @@ int service_compare(CRBTree *t, void *k, CRBNode *n);
 int service_compare_by_name(CRBTree *t, void *k, CRBNode *n);
 
 int service_add(Service *service);
-int service_activate(Service *service);
+int service_activate(Service *service, uint64_t serial);
 int service_remove(Service *service);