From 3e752a1e426cf3718badc178cb2d84af884882cf Mon Sep 17 00:00:00 2001 From: Arnaldo Cesco Date: Thu, 21 Nov 2024 15:32:44 +0100 Subject: [PATCH] DUP: do not generate redundant disconnection triggers In some corner cases, e.g. deletion of a disconnected device, Astarte would generate a disconnection trigger even if the device is already disconnected. Avoid doing so by checking the device connection state before generating a trigger. Signed-off-by: Arnaldo Cesco --- CHANGELOG.md | 6 ++ .../data_updater/impl.ex | 12 ++- .../data_updater_test.exs | 96 +++++++++++++++++++ 3 files changed, 112 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 267171141..55ace3773 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html). +## [1.2.1] - Unreleased +### Fixed +- [astarte_data_updater_plant] Do not generate redundant disconnection + triggers in corner cases when a device is already disconnected. + Fix [#1014](https://github.com/astarte-platform/astarte/issues/1014). + ## [1.2.0] - 2024-07-02 ### Fixed - Forward port changes from release-1.1 (connection failure when delivering diff --git a/apps/astarte_data_updater_plant/lib/astarte_data_updater_plant/data_updater/impl.ex b/apps/astarte_data_updater_plant/lib/astarte_data_updater_plant/data_updater/impl.ex index 8afe26158..1f3b1400b 100644 --- a/apps/astarte_data_updater_plant/lib/astarte_data_updater_plant/data_updater/impl.ex +++ b/apps/astarte_data_updater_plant/lib/astarte_data_updater_plant/data_updater/impl.ex @@ -2263,6 +2263,16 @@ defmodule Astarte.DataUpdaterPlant.DataUpdater.Impl do state.interface_exchanged_bytes ) + maybe_execute_device_disconnected_trigger(state, timestamp_ms) + + %{state | connected: false} + end + + defp maybe_execute_device_disconnected_trigger(%State{connected: false}, _) do + :ok + end + + defp maybe_execute_device_disconnected_trigger(state, timestamp_ms) do trigger_target_with_policy_list = Map.get(state.device_triggers, :on_device_disconnection, []) |> Enum.map(fn target -> @@ -2283,8 +2293,6 @@ defmodule Astarte.DataUpdaterPlant.DataUpdater.Impl do %{}, %{realm: state.realm} ) - - %{state | connected: false} end defp ask_clean_session( diff --git a/apps/astarte_data_updater_plant/test/astarte_data_updater_plant/data_updater_test.exs b/apps/astarte_data_updater_plant/test/astarte_data_updater_plant/data_updater_test.exs index dbb685063..6ff8c7c1b 100644 --- a/apps/astarte_data_updater_plant/test/astarte_data_updater_plant/data_updater_test.exs +++ b/apps/astarte_data_updater_plant/test/astarte_data_updater_plant/data_updater_test.exs @@ -23,6 +23,7 @@ defmodule Astarte.DataUpdaterPlant.DataUpdaterTest do alias Astarte.DataUpdaterPlant.Config alias Astarte.Core.Device alias Astarte.Core.Triggers.SimpleEvents.DeviceConnectedEvent + alias Astarte.Core.Triggers.SimpleEvents.DeviceDisconnectedEvent alias Astarte.Core.Triggers.SimpleEvents.IncomingDataEvent alias Astarte.Core.Triggers.SimpleEvents.PathRemovedEvent alias Astarte.Core.Triggers.SimpleEvents.SimpleEvent @@ -1813,6 +1814,101 @@ defmodule Astarte.DataUpdaterPlant.DataUpdaterTest do assert [] = Registry.lookup(Registry.DataUpdater, {realm, device_id}) end + test "a disconnected device does not generate a disconnection trigger" do + AMQPTestHelper.clean_queue() + + realm = "autotestrealm" + + encoded_device_id = + :crypto.strong_rand_bytes(16) + |> Base.url_encode64(padding: false) + + {:ok, device_id} = Device.decode_device_id(encoded_device_id) + + DatabaseTestHelper.insert_device(device_id) + + timestamp_us_x_10 = make_timestamp("2017-12-09T14:00:32+00:00") + timestamp_ms = div(timestamp_us_x_10, 10_000) + + volatile_trigger_parent_id = :crypto.strong_rand_bytes(16) + volatile_trigger_id = :crypto.strong_rand_bytes(16) + + assert DataUpdater.handle_install_volatile_trigger( + realm, + encoded_device_id, + device_id, + 1, + volatile_trigger_parent_id, + volatile_trigger_id, + generate_disconnection_trigger_data(), + generate_trigger_target() + ) == :ok + + DataUpdater.handle_disconnection( + realm, + encoded_device_id, + gen_tracking_id(), + timestamp_us_x_10 + ) + + # Receive the first disconnection trigger + {event, headers, _metadata} = AMQPTestHelper.wait_and_get_message() + assert headers["x_astarte_event_type"] == "device_disconnected_event" + assert headers["x_astarte_realm"] == realm + assert headers["x_astarte_device_id"] == encoded_device_id + + assert :uuid.string_to_uuid(headers["x_astarte_parent_trigger_id"]) == + volatile_trigger_parent_id + + assert :uuid.string_to_uuid(headers["x_astarte_simple_trigger_id"]) == volatile_trigger_id + + assert SimpleEvent.decode(event) == %SimpleEvent{ + device_id: encoded_device_id, + event: { + :device_disconnected_event, + %DeviceDisconnectedEvent{} + }, + parent_trigger_id: volatile_trigger_parent_id, + timestamp: timestamp_ms, + realm: realm, + simple_trigger_id: volatile_trigger_id + } + + DataUpdater.handle_disconnection( + realm, + encoded_device_id, + gen_tracking_id(), + timestamp_us_x_10 + ) + + # The second disconnection trigger is not sent + assert AMQPTestHelper.awaiting_messages_count() == 0 + end + + defp generate_disconnection_trigger_data() do + %SimpleTriggerContainer{ + simple_trigger: { + :device_trigger, + %DeviceTrigger{ + device_event_type: :DEVICE_DISCONNECTED + } + } + } + |> SimpleTriggerContainer.encode() + end + + defp generate_trigger_target() do + %TriggerTargetContainer{ + trigger_target: { + :amqp_trigger_target, + %AMQPTriggerTarget{ + routing_key: AMQPTestHelper.events_routing_key() + } + } + } + |> TriggerTargetContainer.encode() + end + defp retrieve_endpoint_id(client, interface_name, interface_major, path) do query = DatabaseQuery.new()