From ffe3247bff05d7db5b59e41ffc9cc6eb4febed1d Mon Sep 17 00:00:00 2001 From: Ryan Melton Date: Sat, 3 Oct 2026 21:09:47 -0600 Subject: [PATCH 1/2] fix(microservices): ack malformed and released commands, align flag defaults Malformed JSON in cmd_params, interface_cmd or protocol_cmd (and, in Python, a missing target_name or cmd_name) is now acked with an error instead of killing the command handler thread. A released critical command whose target was disabled while held is acked with an error. Python validators now reject only on an explicit False and report the exception message. Ruby now treats missing range_check, hazardous_check, validate and log_message as true. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../microservices/interface_microservice.rb | 35 +++++-- .../microservices/interface_microservice.py | 35 ++++--- .../test_interface_microservice.py | 98 +++++++++++++++++-- .../interface_microservice_spec.rb | 85 ++++++++++++++++ 4 files changed, 226 insertions(+), 27 deletions(-) diff --git a/openc3/lib/openc3/microservices/interface_microservice.rb b/openc3/lib/openc3/microservices/interface_microservice.rb index 8d5e337d8c..977460242e 100644 --- a/openc3/lib/openc3/microservices/interface_microservice.rb +++ b/openc3/lib/openc3/microservices/interface_microservice.rb @@ -156,8 +156,8 @@ def run next 'SUCCESS' end if msg_hash.key?('interface_cmd') - params = JSON.parse(msg_hash['interface_cmd'], allow_nan: true, create_additions: true) begin + params = JSON.parse(msg_hash['interface_cmd'], allow_nan: true, create_additions: true) @logger.info "#{@interface.name}: interface_cmd: #{params['cmd_name']} #{params['cmd_params'].join(' ')}" @interface.interface_cmd(params['cmd_name'], *params['cmd_params']) InterfaceStatusModel.set(@interface.as_json(), queued: true, scope: @scope) @@ -168,8 +168,8 @@ def run next 'SUCCESS' end if msg_hash.key?('protocol_cmd') - params = JSON.parse(msg_hash['protocol_cmd'], allow_nan: true, create_additions: true) begin + params = JSON.parse(msg_hash['protocol_cmd'], allow_nan: true, create_additions: true) @logger.info "#{@interface.name}: protocol_cmd: #{params['cmd_name']} #{params['cmd_params'].join(' ')} read_write: #{params['read_write']} index: #{params['index']}" @interface.protocol_cmd(params['cmd_name'], *params['cmd_params'], read_write: params['read_write'], index: params['index']) InterfaceStatusModel.set(@interface.as_json(), queued: true, scope: @scope) @@ -232,21 +232,28 @@ def run target_name = msg_hash['target_name'] if target_name and not @interface.cmd_target_enabled[target_name] + # A released critical command was already acked as pending so it must get a result + next "Target #{target_name} is disabled" if release_critical next nil # Return and don't ack given target_name if disabled end cmd_name = msg_hash['cmd_name'] - manual = ConfigParser.handle_true_false(msg_hash['manual']) + manual = flag(msg_hash, 'manual', false) cmd_params = nil range_check = true raw = false cmd_buffer = nil hazardous_check = nil if msg_hash['cmd_params'] - cmd_params = JSON.parse(msg_hash['cmd_params'], allow_nan: true, create_additions: true) - range_check = ConfigParser.handle_true_false(msg_hash['range_check']) - raw = ConfigParser.handle_true_false(msg_hash['raw']) - hazardous_check = ConfigParser.handle_true_false(msg_hash['hazardous_check']) + begin + cmd_params = JSON.parse(msg_hash['cmd_params'], allow_nan: true, create_additions: true) + rescue => e + @logger.error "#{@interface.name}: Invalid cmd_params: #{e.message}" + next "Invalid cmd_params: #{e.message}" + end + range_check = flag(msg_hash, 'range_check', true) + raw = flag(msg_hash, 'raw', false) + hazardous_check = flag(msg_hash, 'hazardous_check', true) elsif msg_hash['cmd_buffer'] cmd_buffer = msg_hash['cmd_buffer'] end @@ -275,6 +282,7 @@ def run command.received_count = orig_command.received_count command.received_time = Time.now else + next "Target #{command.target_name} is disabled" if release_critical next nil # Don't ack disabled targets end rescue RangeError => e @@ -322,7 +330,7 @@ def run end end - validate = ConfigParser.handle_true_false(msg_hash['validate']) + validate = flag(msg_hash, 'validate', true) begin if @interface.connected? result = true @@ -344,7 +352,7 @@ def run @count += 1 @metric.set(name: 'interface_cmd_total', value: @count, type: 'counter') if @metric - log_message = ConfigParser.handle_true_false(msg_hash['log_message']) + log_message = flag(msg_hash, 'log_message', true) if log_message @logger.info(msg_hash['cmd_string'], user: msg_hash['username'], scope: @scope) end @@ -390,6 +398,15 @@ def run end end end + + private + + # Command message flags are 'true'/'false' strings; a missing flag uses its safe default + def flag(msg_hash, key, default) + value = msg_hash[key] + return default if value.nil? + ConfigParser.handle_true_false(value) + end end class RouterTlmHandlerThread diff --git a/openc3/python/openc3/microservices/interface_microservice.py b/openc3/python/openc3/microservices/interface_microservice.py index eb0fd384a3..07232dedd1 100644 --- a/openc3/python/openc3/microservices/interface_microservice.py +++ b/openc3/python/openc3/microservices/interface_microservice.py @@ -196,8 +196,8 @@ def process_cmd(self, topic, msg_id, msg_hash, _redis): self.interface.stop_raw_logging() return "SUCCESS" if msg_hash.get(b"interface_cmd"): - params = json.loads(msg_hash[b"interface_cmd"]) try: + params = json.loads(msg_hash[b"interface_cmd"]) str_params = " ".join([str(i) for i in params["cmd_params"]]) self.logger.info(f"{self.interface.name}: interface_cmd: {params['cmd_name']} {str_params}") self.interface.interface_cmd(params["cmd_name"], *params["cmd_params"]) @@ -208,8 +208,8 @@ def process_cmd(self, topic, msg_id, msg_hash, _redis): return str(e) return "SUCCESS" if msg_hash.get(b"protocol_cmd"): - params = json.loads(msg_hash[b"protocol_cmd"]) try: + params = json.loads(msg_hash[b"protocol_cmd"]) str_params = " ".join([str(i) for i in params["cmd_params"]]) self.logger.info( f"{self.interface.name}: protocol_cmd: {params['cmd_name']} {str_params} read_write: {params['read_write']} index: {params['index']}" @@ -281,10 +281,15 @@ def process_cmd(self, topic, msg_id, msg_hash, _redis): ) return str(e) - target_name = msg_hash[b"target_name"].decode() + target_name = msg_hash.get(b"target_name") + target_name = target_name.decode() if target_name is not None else None if target_name and not self.interface.cmd_target_enabled.get(target_name, False): + # A released critical command was already acked as pending so it must get a result + if release_critical: + return f"Target {target_name} is disabled" return None # Return and don't ack given target_name if disabled - cmd_name = msg_hash[b"cmd_name"].decode() + cmd_name = msg_hash.get(b"cmd_name") + cmd_name = cmd_name.decode() if cmd_name is not None else None manual = ConfigParser.handle_true_false(msg_hash.get(b"manual", b"FALSE").decode()) cmd_params = None range_check = True @@ -292,7 +297,11 @@ def process_cmd(self, topic, msg_id, msg_hash, _redis): cmd_buffer = None hazardous_check = None if msg_hash.get(b"cmd_params") is not None: - cmd_params = json.loads(msg_hash.get(b"cmd_params"), cls=JsonDecoder) + try: + cmd_params = json.loads(msg_hash.get(b"cmd_params"), cls=JsonDecoder) + except ValueError as e: + self.logger.error(f"{self.interface.name}: Invalid cmd_params: {e}") + return f"Invalid cmd_params: {e}" range_check = ConfigParser.handle_true_false(msg_hash.get(b"range_check", b"TRUE").decode()) raw = ConfigParser.handle_true_false(msg_hash.get(b"raw", b"FALSE").decode()) hazardous_check = ConfigParser.handle_true_false(msg_hash.get(b"hazardous_check", b"TRUE").decode()) @@ -315,6 +324,8 @@ def process_cmd(self, topic, msg_id, msg_hash, _redis): raise RuntimeError(f"Invalid command received:\n{msg_hash}") if not self.interface.cmd_target_enabled.get(command.target_name, False): + if release_critical: + return f"Target {command.target_name} is disabled" return None # Don't ack disabled targets orig_command = System.commands.packet(command.target_name, command.packet_name) @@ -384,10 +395,11 @@ def process_cmd(self, topic, msg_id, msg_hash, _redis): if command.validator and validate: try: result, reason = command.validator.pre_check(command) - except Exception: + except Exception as e: result = False - reason = traceback.format_exc() - if not result: + reason = str(e) + # Explicitly check for False to allow None to represent unknown + if result is False: message = f"pre_check returned false for {command.extra['cmd_string']} due to {reason}" raise WriteRejectError(message) @@ -410,9 +422,9 @@ def process_cmd(self, topic, msg_id, msg_hash, _redis): if command.validator and validate: try: result, reason = command.validator.post_check(command) - except Exception: + except Exception as e: result = False - reason = traceback.format_exc() + reason = str(e) command.extra["cmd_success"] = result if reason: command.extra["cmd_reason"] = reason @@ -421,7 +433,8 @@ def process_cmd(self, topic, msg_id, msg_hash, _redis): CommandTopic.write_packet(command, scope=self.scope) InterfaceStatusModel.set(self.interface.as_json(), queued=True, scope=self.scope) - if not result: + # Explicitly check for False to allow None to represent unknown + if result is False: message = f"post_check returned false for {command.extra['cmd_string']} due to {reason}" raise WriteRejectError(message) diff --git a/openc3/python/test/microservices/test_interface_microservice.py b/openc3/python/test/microservices/test_interface_microservice.py index 85255cb2bc..bdaecd2dad 100644 --- a/openc3/python/test/microservices/test_interface_microservice.py +++ b/openc3/python/test/microservices/test_interface_microservice.py @@ -628,13 +628,14 @@ def test_process_cmd_with_all_fields_and_missing_optional_fields(self): result = handler.process_cmd(topic, msg_id, minimal_msg_hash, None) self.assertEqual(result, "SUCCESS") - # Missing target_name raises KeyError (required field uses direct [] access) - with self.assertRaises(KeyError): - handler.process_cmd(topic, msg_id, {b"cmd_name": b"ABORT"}, None) - - # Missing cmd_name raises KeyError (required field uses direct [] access) - with self.assertRaises(KeyError): - handler.process_cmd(topic, msg_id, {b"target_name": b"INST"}, None) + # Missing target_name or cmd_name is acked with an error rather than raising + cmd_params = json.dumps({}).encode() + result = handler.process_cmd(topic, msg_id, {b"cmd_name": b"ABORT", b"cmd_params": cmd_params}, None) + self.assertIsInstance(result, str) + self.assertNotEqual(result, "SUCCESS") + result = handler.process_cmd(topic, msg_id, {b"target_name": b"INST", b"cmd_params": cmd_params}, None) + self.assertIsInstance(result, str) + self.assertNotEqual(result, "SUCCESS") # Disabled target returns None (no ack) handler.interface.cmd_target_enabled["INST"] = False @@ -769,6 +770,89 @@ def test_process_cmd_command_error_and_hazardous_branches(self): ) self.assertEqual(result, "Interface not connected: INST_INT") + def test_process_cmd_acks_malformed_json(self): + """Malformed JSON in a directive or cmd_params is acked with an error + instead of killing the command handler thread.""" + im = InterfaceMicroservice("DEFAULT__INTERFACE__INST_INT") + self.addCleanup(im.shutdown) + handler = im.handler_thread + msg_id = f"{int(time.time() * 1000)}-0" + + for directive in [b"interface_cmd", b"protocol_cmd"]: + result = handler.process_cmd("{DEFAULT__CMD}INTERFACE__INST_INT", msg_id, {directive: b"{not json"}, None) + self.assertIsInstance(result, str) + self.assertNotEqual(result, "SUCCESS") + + result = handler.process_cmd( + "{DEFAULT__CMD}TARGET__INST", + msg_id, + {b"target_name": b"INST", b"cmd_name": b"ABORT", b"cmd_params": b"{not json"}, + None, + ) + self.assertIn("Invalid cmd_params", result) + + def test_process_cmd_acks_released_critical_cmd_for_disabled_target(self): + """A released critical command was already acked as pending, so a target + disabled in the meantime must produce an error ack, not silence.""" + im = InterfaceMicroservice("DEFAULT__INTERFACE__INST_INT") + self.addCleanup(im.shutdown) + handler = im.handler_thread + msg_id = f"{int(time.time() * 1000)}-0" + + critical_model = Mock() + critical_model.approver = "approver" + critical_model.cmd_hash = { + b"target_name": b"INST", + b"cmd_name": b"ABORT", + b"cmd_params": json.dumps({}).encode(), + b"cmd_string": b"cmd('INST ABORT')", + } + handler.interface.cmd_target_enabled["INST"] = False + with patch("openc3.microservices.interface_microservice.CriticalCmdModel", create=True) as model_class: + model_class.get_model.return_value = critical_model + result = handler.process_cmd( + "{DEFAULT__CMD}INTERFACE__INST_INT", msg_id, {b"release_critical": b"abc-123"}, None + ) + self.assertEqual(result, "Target INST is disabled") + + def test_process_cmd_validator_results(self): + """Only an explicit False from pre_check/post_check rejects a command (None + means unknown), and a raising validator reports its message, not a traceback.""" + im = InterfaceMicroservice("DEFAULT__INTERFACE__INST_INT") + thread = threading.Thread(target=im.run) + thread.start() + self.addCleanup(thread.join, 5) + self.addCleanup(im.shutdown) + time.sleep(0.1) + + handler = im.handler_thread + topic = "{DEFAULT__CMD}TARGET__INST" + msg_id = f"{int(time.time() * 1000)}-0" + msg_hash = { + b"target_name": b"INST", + b"cmd_name": b"ABORT", + b"cmd_params": json.dumps({}).encode(), + b"cmd_string": b"cmd('INST ABORT')", + } + + validator = Mock() + packet = System.commands.packet("INST", "ABORT") + packet.validator = validator + self.addCleanup(setattr, packet, "validator", None) + + validator.pre_check.return_value = (None, None) + validator.post_check.return_value = (None, None) + self.assertEqual(handler.process_cmd(topic, msg_id, msg_hash, None), "SUCCESS") + + validator.pre_check.return_value = (False, "not ready") + result = handler.process_cmd(topic, msg_id, msg_hash, None) + self.assertEqual(result, "pre_check returned false for cmd('INST ABORT') due to not ready") + + validator.pre_check.return_value = (True, None) + validator.post_check.side_effect = RuntimeError("post boom") + result = handler.process_cmd(topic, msg_id, msg_hash, None) + self.assertEqual(result, "post_check returned false for cmd('INST ABORT') due to post boom") + def test_process_cmd_identifies_a_cmd_buffer(self): """A command sent as a raw cmd_buffer is identified and written to the connected interface.""" diff --git a/openc3/spec/microservices/interface_microservice_spec.rb b/openc3/spec/microservices/interface_microservice_spec.rb index 48b5ed2760..8fa9620170 100644 --- a/openc3/spec/microservices/interface_microservice_spec.rb +++ b/openc3/spec/microservices/interface_microservice_spec.rb @@ -228,6 +228,91 @@ def handle_interface_details(interface) end end + describe "command message handling" do + # Drive the command handler block directly with one crafted message and + # return the block's result, which becomes the ack (nil means no ack) + def handle(msg_hash, topic: "{DEFAULT__CMD}TARGET__INST") + handler = InterfaceCmdHandlerThread.new(@interface, double("tlm").as_null_object, scope: "DEFAULT") + result = :not_run + mutex = Mutex.new + allow(InterfaceTopic).to receive(:receive_commands) do |*_args, **_kwargs, &block| + # The microservice's own handler thread may also land here, so only run the message once + mutex.synchronize do + result = block.call(topic, "#{(Time.now.to_f * 1000).to_i}-0", msg_hash, nil) if result == :not_run + end + end + handler.run + result + end + + before(:each) do + @im = InterfaceMicroservice.new("DEFAULT__INTERFACE__INST_INT") + @interface = @im.instance_variable_get(:@interface) + allow(@interface).to receive(:connected?).and_return(true) + allow(@interface).to receive(:write) + end + + after(:each) do + @im.shutdown + sleep 0.1 # Allow threads to exit + end + + it "acks malformed interface_cmd and protocol_cmd JSON with an error" do + %w(interface_cmd protocol_cmd).each do |directive| + result = handle({ directive => '{not json' }, topic: "{DEFAULT__CMD}INTERFACE__INST_INT") + expect(result).to be_a(String) + expect(result).to_not eql 'SUCCESS' + end + end + + it "acks malformed cmd_params JSON with an error" do + result = handle({ 'target_name' => 'INST', 'cmd_name' => 'ABORT', 'cmd_params' => '{not json' }) + expect(result).to match(/Invalid cmd_params/) + end + + it "does not ack a command for a disabled target" do + @interface.cmd_target_enabled['INST'] = false + expect(handle({ 'target_name' => 'INST', 'cmd_name' => 'ABORT', 'cmd_params' => '{}' })).to be_nil + end + + it "acks a released critical command whose target is now disabled" do + critical_model = double("CriticalCmdModel", approver: 'approver', + cmd_hash: { 'target_name' => 'INST', 'cmd_name' => 'ABORT', 'cmd_params' => '{}', 'cmd_string' => 'cmd("INST ABORT")' }) + stub_const("OpenC3::CriticalCmdModel", double(get_model: critical_model)) + @interface.cmd_target_enabled['INST'] = false + result = handle({ 'release_critical' => 'abc-123' }, topic: "{DEFAULT__CMD}INTERFACE__INST_INT") + expect(result).to eql "Target INST is disabled" + end + + it "range checks a command that omits range_check" do + result = handle({ 'target_name' => 'INST', 'cmd_name' => 'COLLECT', 'cmd_params' => JSON.generate({ 'TYPE' => 'NORMAL', 'DURATION' => 100 }) }) + expect(result).to match(/DURATION/) + end + + it "hazardous checks a command that omits hazardous_check" do + result = handle({ 'target_name' => 'INST', 'cmd_name' => 'CLEAR', 'cmd_params' => '{}', 'cmd_string' => 'cmd("INST CLEAR")' }) + expect(result).to start_with("HazardousError") + end + + it "validates and logs a command that omits validate and log_message" do + validator = double("validator") + allow_any_instance_of(Packet).to receive(:validator).and_return(validator) + expect(validator).to receive(:pre_check).and_return([true, nil]) + expect(validator).to receive(:post_check).and_return([true, nil]) + expect(Logger).to receive(:info).with('cmd("INST ABORT")', anything).at_least(:once) + allow(Logger).to receive(:info) + result = handle({ 'target_name' => 'INST', 'cmd_name' => 'ABORT', 'cmd_params' => '{}', 'cmd_string' => 'cmd("INST ABORT")' }) + expect(result).to eql 'SUCCESS' + end + + it "does not log a command with log_message false" do + expect(Logger).to_not receive(:info).with('cmd("INST ABORT")', anything) + allow(Logger).to receive(:info) + result = handle({ 'target_name' => 'INST', 'cmd_name' => 'ABORT', 'cmd_params' => '{}', 'cmd_string' => 'cmd("INST ABORT")', 'log_message' => 'false' }) + expect(result).to eql 'SUCCESS' + end + end + describe "handle_packet" do it "does not write the status model after cancel_thread is set" do # A packet already buffered can be returned from read() after stop() From cb1e30730ddbe0ba2b25e21e29b24601b2129ab9 Mon Sep 17 00:00:00 2001 From: Jason Thomas Date: Thu, 15 Oct 2026 17:36:49 -0600 Subject: [PATCH 2/2] refactor(config): add default to handle_true_false for missing values Replace the private flag helper in InterfaceCmdHandlerThread with a default: keyword on ConfigParser.handle_true_false, returned when the value is nil. The default is nil so existing callers are unchanged. Co-Authored-By: Claude Opus 5.5 --- openc3/lib/openc3/config/config_parser.rb | 4 +++- .../microservices/interface_microservice.rb | 21 ++++++------------- openc3/spec/config/config_parser_spec.rb | 7 +++++++ 3 files changed, 16 insertions(+), 16 deletions(-) diff --git a/openc3/lib/openc3/config/config_parser.rb b/openc3/lib/openc3/config/config_parser.rb index cc4c711fd1..3c96357b9a 100644 --- a/openc3/lib/openc3/config/config_parser.rb +++ b/openc3/lib/openc3/config/config_parser.rb @@ -319,8 +319,10 @@ def self.handle_nil(value) # primitive. All other values are simply returned. # # @param value [Object] + # @param default [Object] Returned if value is nil # @return [true|false|Object] - def self.handle_true_false(value) + def self.handle_true_false(value, default: nil) + return default if value.nil? if String === value case value.upcase when 'TRUE' diff --git a/openc3/lib/openc3/microservices/interface_microservice.rb b/openc3/lib/openc3/microservices/interface_microservice.rb index 977460242e..2af068959a 100644 --- a/openc3/lib/openc3/microservices/interface_microservice.rb +++ b/openc3/lib/openc3/microservices/interface_microservice.rb @@ -238,7 +238,7 @@ def run end cmd_name = msg_hash['cmd_name'] - manual = flag(msg_hash, 'manual', false) + manual = ConfigParser.handle_true_false(msg_hash['manual'], default: false) cmd_params = nil range_check = true raw = false @@ -251,9 +251,9 @@ def run @logger.error "#{@interface.name}: Invalid cmd_params: #{e.message}" next "Invalid cmd_params: #{e.message}" end - range_check = flag(msg_hash, 'range_check', true) - raw = flag(msg_hash, 'raw', false) - hazardous_check = flag(msg_hash, 'hazardous_check', true) + range_check = ConfigParser.handle_true_false(msg_hash['range_check'], default: true) + raw = ConfigParser.handle_true_false(msg_hash['raw'], default: false) + hazardous_check = ConfigParser.handle_true_false(msg_hash['hazardous_check'], default: true) elsif msg_hash['cmd_buffer'] cmd_buffer = msg_hash['cmd_buffer'] end @@ -330,7 +330,7 @@ def run end end - validate = flag(msg_hash, 'validate', true) + validate = ConfigParser.handle_true_false(msg_hash['validate'], default: true) begin if @interface.connected? result = true @@ -352,7 +352,7 @@ def run @count += 1 @metric.set(name: 'interface_cmd_total', value: @count, type: 'counter') if @metric - log_message = flag(msg_hash, 'log_message', true) + log_message = ConfigParser.handle_true_false(msg_hash['log_message'], default: true) if log_message @logger.info(msg_hash['cmd_string'], user: msg_hash['username'], scope: @scope) end @@ -398,15 +398,6 @@ def run end end end - - private - - # Command message flags are 'true'/'false' strings; a missing flag uses its safe default - def flag(msg_hash, key, default) - value = msg_hash[key] - return default if value.nil? - ConfigParser.handle_true_false(value) - end end class RouterTlmHandlerThread diff --git a/openc3/spec/config/config_parser_spec.rb b/openc3/spec/config/config_parser_spec.rb index 781cab08e9..628f607633 100644 --- a/openc3/spec/config/config_parser_spec.rb +++ b/openc3/spec/config/config_parser_spec.rb @@ -537,6 +537,13 @@ module OpenC3 expect(ConfigParser.handle_true_false("HI")).to eql "HI" expect(ConfigParser.handle_true_false(5.0)).to eql 5.0 end + + it "returns the default for nil" do + expect(ConfigParser.handle_true_false(nil)).to be_nil + expect(ConfigParser.handle_true_false(nil, default: true)).to be true + expect(ConfigParser.handle_true_false(nil, default: false)).to be false + expect(ConfigParser.handle_true_false('FALSE', default: true)).to be false + end end describe "self.handle_true_false_strict" do