From 8f29a84b2b18f52a0634092829a60d303a48b242 Mon Sep 17 00:00:00 2001 From: Cameron Reeves Date: Mon, 28 Sep 2026 16:39:10 +1000 Subject: [PATCH 1/3] fix(mappings): serialise module mapping writes for a system Two updates of one system could run set_mappings at the same time. Each cleared the redis hash and wrote its keys one by one, so the two writers interleaved and the hash ended up in an arbitrary order. Drivers that pick a module by position in that hash, the mailers in particular, then picked the wrong module; on HIO UAT this routed every templated email to the Calendar module and made the Template Mailer send to itself. The writes for a system now take a per-system lock, and the mapping is resolved before the hash is cleared so it is only empty while being written. PPT-2816 --- spec/mappings/control_system_modules_spec.cr | 48 +++++++++++++++++++ .../mappings/control_system_modules.cr | 29 +++++++++-- 2 files changed, 73 insertions(+), 4 deletions(-) diff --git a/spec/mappings/control_system_modules_spec.cr b/spec/mappings/control_system_modules_spec.cr index 15845d44..8548cd4f 100644 --- a/spec/mappings/control_system_modules_spec.cr +++ b/spec/mappings/control_system_modules_spec.cr @@ -81,6 +81,54 @@ module PlaceOS::Core::Mappings end end + describe ".set_mappings" do + it "waits for an update of the same system that is still writing" do + driver = Model::Generator.driver(:device).save! + + modules = %w(alpha beta gamma delta epsilon).map do |name| + m = Model::Generator.module(driver) + m.custom_name = name + m.save! + end + + cs = Model::Generator.control_system + cs.modules = modules.compact_map &.id + cs.save! + + expected = modules.map { |m| "#{m.custom_name}/1" } + storage = Driver::RedisStorage.new(cs.id.as(String), "system") + + # an earlier update of this system is part way through writing + lock = ControlSystemModules.mapping_lock(cs.id.as(String)) + lock.lock + + finished = false + done = Channel(Nil).new + spawn do + ControlSystemModules.set_mappings(cs, nil) + finished = true + ensure + done.send(nil) + end + + # plenty of time for the second update to run if nothing held it back + sleep 0.3.seconds + finished.should be_false + + # the earlier update's writes land while the second one waits + storage.clear + modules.reverse_each { |m| storage["#{m.custom_name}/1"] = m.id.as(String) } + storage.keys.should eq expected.reverse + + lock.unlock + done.receive + finished.should be_true + + # the second update rewrote the mapping in the system's order + storage.keys.should eq expected + end + end + describe ".update_logic_modules" do it "does not update if system is destroyed" do cs = Model::ControlSystem.new diff --git a/src/placeos-core/mappings/control_system_modules.cr b/src/placeos-core/mappings/control_system_modules.cr index bd0b8e01..17ffedb5 100644 --- a/src/placeos-core/mappings/control_system_modules.cr +++ b/src/placeos-core/mappings/control_system_modules.cr @@ -85,6 +85,17 @@ module PlaceOS::Core updated_modules end + # One lock per system, so two updates of the same system write its mappings + # one after the other rather than interleaved + @@mapping_locks = {} of String => Mutex + @@mapping_locks_lock = Mutex.new + + def self.mapping_lock(system_id : String) : Mutex + @@mapping_locks_lock.synchronize do + @@mapping_locks[system_id] ||= Mutex.new + end + end + # Set the module mappings for a ControlSystem # # Pass module_id and updated_name to overrride a lookup @@ -93,13 +104,21 @@ module PlaceOS::Core mod : Model::Module?, ) : Hash(String, String) system_id = control_system.id.as(String) - storage = Driver::RedisStorage.new(system_id, "system") + mapping_lock(system_id).synchronize do + write_mappings(control_system, mod, system_id) + end + end - # Clear out the ControlSystem's mapping - storage.clear + protected def self.write_mappings( + control_system : Model::ControlSystem, + mod : Model::Module?, + system_id : String, + ) : Hash(String, String) + storage = Driver::RedisStorage.new(system_id, "system") # No mappings to set if ControlSystem has been destroyed if control_system.destroyed? + storage.clear Log.info { { message: "module mappings deleted", system_id: control_system.id, @@ -122,7 +141,9 @@ module PlaceOS::Core end end - # Set the mappings in redis + # Replace the ControlSystem's mapping. The lookups above can take a + # while, so the hash is only empty for the time it takes to write it. + storage.clear mappings.each do |mapping, module_id| storage[mapping] = module_id end From 9e2576de7041291919b48cbc5a09748140bc5119 Mon Sep 17 00:00:00 2001 From: Stephen von Takach Date: Wed, 30 Sep 2026 15:51:38 +1000 Subject: [PATCH 2/3] use a single lock --- src/placeos-core/mappings/control_system_modules.cr | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/src/placeos-core/mappings/control_system_modules.cr b/src/placeos-core/mappings/control_system_modules.cr index 17ffedb5..4b1820ac 100644 --- a/src/placeos-core/mappings/control_system_modules.cr +++ b/src/placeos-core/mappings/control_system_modules.cr @@ -87,14 +87,7 @@ module PlaceOS::Core # One lock per system, so two updates of the same system write its mappings # one after the other rather than interleaved - @@mapping_locks = {} of String => Mutex - @@mapping_locks_lock = Mutex.new - - def self.mapping_lock(system_id : String) : Mutex - @@mapping_locks_lock.synchronize do - @@mapping_locks[system_id] ||= Mutex.new - end - end + class_getter mapping_lock : Mutex = Mutex.new # Set the module mappings for a ControlSystem # @@ -104,9 +97,7 @@ module PlaceOS::Core mod : Model::Module?, ) : Hash(String, String) system_id = control_system.id.as(String) - mapping_lock(system_id).synchronize do - write_mappings(control_system, mod, system_id) - end + mapping_lock.synchronize { write_mappings(control_system, mod, system_id) } end protected def self.write_mappings( From b70323cca314adc2a132ffbf7489422aa7e28c03 Mon Sep 17 00:00:00 2001 From: Cameron Reeves Date: Wed, 30 Sep 2026 15:56:04 +1000 Subject: [PATCH 3/3] fix(mappings): follow the single mapping lock in the spec, and pull minio from pgsty The spec still asked for a lock by system id. The minio and mc images moved to pgsty/minio and pgsty/mc, and the old names no longer pull, which failed every CI run before the specs started. PPT-2816 --- docker-compose.yml | 4 ++-- spec/mappings/control_system_modules_spec.cr | 2 +- src/placeos-core/mappings/control_system_modules.cr | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/docker-compose.yml b/docker-compose.yml index 2e225817..eb029814 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -98,7 +98,7 @@ services: <<: *s3-client-env minio: - image: minio/minio:latest + image: pgsty/minio:latest volumes: - s3:/data ports: @@ -111,7 +111,7 @@ services: command: server /data --console-address ":9090" testbucket: - image: minio/mc:latest + image: pgsty/mc:latest depends_on: - minio environment: diff --git a/spec/mappings/control_system_modules_spec.cr b/spec/mappings/control_system_modules_spec.cr index 8548cd4f..f33fba6d 100644 --- a/spec/mappings/control_system_modules_spec.cr +++ b/spec/mappings/control_system_modules_spec.cr @@ -99,7 +99,7 @@ module PlaceOS::Core::Mappings storage = Driver::RedisStorage.new(cs.id.as(String), "system") # an earlier update of this system is part way through writing - lock = ControlSystemModules.mapping_lock(cs.id.as(String)) + lock = ControlSystemModules.mapping_lock lock.lock finished = false diff --git a/src/placeos-core/mappings/control_system_modules.cr b/src/placeos-core/mappings/control_system_modules.cr index 4b1820ac..23e5e345 100644 --- a/src/placeos-core/mappings/control_system_modules.cr +++ b/src/placeos-core/mappings/control_system_modules.cr @@ -85,8 +85,8 @@ module PlaceOS::Core updated_modules end - # One lock per system, so two updates of the same system write its mappings - # one after the other rather than interleaved + # Held while a system's mappings are written, so two updates write one after + # the other rather than interleaved class_getter mapping_lock : Mutex = Mutex.new # Set the module mappings for a ControlSystem