From 8d618699ac60e1f1926426406498b7abecd80625 Mon Sep 17 00:00:00 2001 From: Cameron Reeves Date: Mon, 28 Sep 2026 16:39:31 +1000 Subject: [PATCH 1/2] fix(proxy): return implementing modules in the system's module order system.implementing(interface) walked the redis hash that maps module names to ids and returned modules in that hash's order. The hash has no defined order, and core rewrites it on every system update, so two overlapping updates left it in an arbitrary order. Drivers that take the first or second implementer (the mailers do) then picked the wrong module: on HIO UAT the visitor mailer sent through the Calendar module and the Template Mailer forwarded to itself. The system model now carries its module id list and implementing sorts by it, so the first result is the module listed first on the system. Modules not on the list follow in name and index order. all(name) is sorted by index for the same reason. PPT-2816 --- spec/drivers_proxy_spec.cr | 33 ++++++++++++++++++++++++++++++ src/placeos-driver/driver_model.cr | 2 ++ src/placeos-driver/proxy/system.cr | 26 ++++++++++++++++++++--- 3 files changed, 58 insertions(+), 3 deletions(-) diff --git a/spec/drivers_proxy_spec.cr b/spec/drivers_proxy_spec.cr index 10d4f064..bc4df6e2 100644 --- a/spec/drivers_proxy_spec.cr +++ b/spec/drivers_proxy_spec.cr @@ -33,6 +33,39 @@ module PlaceOS::Driver::Proxy responses.get.should eq Array(JSON::Any).from_json("[]") end + it "returns implementing modules in the system's module order" do + cs = PlaceOS::Driver::DriverModel::ControlSystem.from_json(%( + { + "id": "sys-order-1", + "name": "Ordered System", + "capacity": 0, + "bookable": false, + "zones": ["zone-1234"], + "modules": ["mod-first", "mod-second", "mod-third"] + } + )) + system = PlaceOS::Driver::Proxy::System.new cs, "reply_id" + + # the redis hash was written in a different order to the system's list + storage = PlaceOS::Driver::RedisStorage.new(cs.id, "system") + storage.clear + storage["Calendar/1"] = "mod-third" + storage["Mailer/1"] = "mod-first" + storage["Mailer/2"] = "mod-second" + storage["Extra/1"] = "mod-unlisted" + + redis = PlaceOS::Driver::RedisStorage.new_redis_client + meta = PlaceOS::Driver::DriverModel::Metadata.new({ + "send_mail" => {} of String => JSON::Any, + }, ["Mailer"]) + {"mod-first", "mod-second", "mod-third", "mod-unlisted"}.each { |id| redis.set("interface/#{id}", meta.to_json) } + + system.implementing(:Mailer).map(&.module_id).should eq(["mod-first", "mod-second", "mod-third", "mod-unlisted"]) + system.all(:Mailer).map(&.index).should eq([1, 2]) + + storage.clear + end + it "should execute functions on collections of remote drivers" do cs = PlaceOS::Driver::DriverModel::ControlSystem.from_json(%( { diff --git a/src/placeos-driver/driver_model.cr b/src/placeos-driver/driver_model.cr index e46ffccf..0b5ef23b 100644 --- a/src/placeos-driver/driver_model.cr +++ b/src/placeos-driver/driver_model.cr @@ -20,6 +20,8 @@ struct PlaceOS::Driver::DriverModel property timezone : String? property support_url : String? property zones : Array(String) + # module ids in the order they are listed on the system + property modules : Array(String) = [] of String property images : Array(String)? property security_groups : Array(String)? end diff --git a/src/placeos-driver/proxy/system.cr b/src/placeos-driver/proxy/system.cr index 1f8f7690..100cb0f0 100644 --- a/src/placeos-driver/proxy/system.cr +++ b/src/placeos-driver/proxy/system.cr @@ -105,7 +105,7 @@ struct PlaceOS::Driver::Proxy::System end end - PlaceOS::Driver::Proxy::Drivers.new(drivers) + PlaceOS::Driver::Proxy::Drivers.new(drivers.sort_by(&.index)) end def all(module_name, *, implementing) : PlaceOS::Driver::Proxy::Drivers @@ -125,7 +125,7 @@ struct PlaceOS::Driver::Proxy::System drivers << Proxy::Driver.new(@reply_id, mod_name, index.to_i, module_id, self, metadata) end - PlaceOS::Driver::Proxy::Drivers.new(drivers) + PlaceOS::Driver::Proxy::Drivers.new(drivers.sort_by(&.index)) end private def get_metadata(module_id : String?) : DriverModel::Metadata @@ -140,6 +140,9 @@ struct PlaceOS::Driver::Proxy::System end # grabs all modules implementing(Powerable) for example + # + # Modules are returned in the order they are listed on the system, so the + # first result is the module an administrator placed first def implementing(interface) : PlaceOS::Driver::Proxy::Drivers interface = interface.to_s drivers = [] of Proxy::Driver @@ -155,7 +158,24 @@ struct PlaceOS::Driver::Proxy::System drivers << Proxy::Driver.new(@reply_id, mod_name, index.to_i, module_id, self, metadata) end - PlaceOS::Driver::Proxy::Drivers.new(drivers) + PlaceOS::Driver::Proxy::Drivers.new(in_system_order(drivers)) + end + + # The redis hash holding the module mappings has no defined order, so sort by + # the system's module list. Modules missing from the list keep a stable + # name and index order after those found. + private def in_system_order(drivers : Array(Proxy::Driver)) : Array(Proxy::Driver) + return drivers if drivers.size < 2 + order = begin + config.modules + rescue error + logger.warn(exception: error) { "unable to load the module order for system #{@system_id}" } + [] of String + end + drivers.sort_by do |driver| + position = order.index(driver.module_id) || Int32::MAX + {position, driver.module_name, driver.index} + end end # coordination to occur on placeos core From 20183c9b792d3bbeb9f1bcb6cfb8d9161698af9c Mon Sep 17 00:00:00 2001 From: Stephen von Takach Date: Wed, 30 Sep 2026 15:44:30 +1000 Subject: [PATCH 2/2] user sort_by! so we're not creating new arrays --- src/placeos-driver/proxy/system.cr | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/placeos-driver/proxy/system.cr b/src/placeos-driver/proxy/system.cr index 100cb0f0..0c697695 100644 --- a/src/placeos-driver/proxy/system.cr +++ b/src/placeos-driver/proxy/system.cr @@ -105,7 +105,7 @@ struct PlaceOS::Driver::Proxy::System end end - PlaceOS::Driver::Proxy::Drivers.new(drivers.sort_by(&.index)) + PlaceOS::Driver::Proxy::Drivers.new(drivers.sort_by!(&.index)) end def all(module_name, *, implementing) : PlaceOS::Driver::Proxy::Drivers @@ -125,7 +125,7 @@ struct PlaceOS::Driver::Proxy::System drivers << Proxy::Driver.new(@reply_id, mod_name, index.to_i, module_id, self, metadata) end - PlaceOS::Driver::Proxy::Drivers.new(drivers.sort_by(&.index)) + PlaceOS::Driver::Proxy::Drivers.new(drivers.sort_by!(&.index)) end private def get_metadata(module_id : String?) : DriverModel::Metadata @@ -172,7 +172,7 @@ struct PlaceOS::Driver::Proxy::System logger.warn(exception: error) { "unable to load the module order for system #{@system_id}" } [] of String end - drivers.sort_by do |driver| + drivers.sort_by! do |driver| position = order.index(driver.module_id) || Int32::MAX {position, driver.module_name, driver.index} end