Merge pull request #1 from stekker/fix-endless-reply

Fix endless reply loop

.rubocop.yml

@@ -216,6 +216,10 @@ Naming/AccessorMethodName:
   Exclude:
   - app/models/**/*.rb
 
+Naming/FileName:
+  Exclude:
+    - lib/s2-ruby.rb
+
 Naming/MethodParameterName:
   MinNameLength: 2
 

lib/s2/connection.rb

@@ -2,20 +2,14 @@ module S2
   class Connection
     include S2::MessageHandler
 
-    on S2::Messages::ReceptionStatus do |message|
-      unless message_sent?(message.subject_message_id)
-        # next reply message, status: S2::Messages::ReceptionStatusValues::InvalidContent
-        @logger.error("Received ReceptionStatus for unknown message ID #{message.subject_message_id}")
-      end
+    attr_reader :sent_messages
 
-      case message.status
-      when S2::Messages::ReceptionStatusValues::Ok
-        # no-op
+    on S2::Messages::ReceptionStatus do |message|
+      if message_sent?(message.subject_message_id)
+        delete_sent_message(message)
+        close if message.status == S2::Messages::ReceptionStatusValues::PermanentError
       else
-        @logger.error(
-          "Received ReceptionStatus with status #{message.status} " \
-          "for message ID #{message.subject_message_id}",
-        )
+        @logger.error("Received ReceptionStatus for unknown message ID #{message.subject_message_id}")
       end
     end
 
@@ -31,7 +25,6 @@ def receive_message(message_json)
 
       message = deserialize_message(message_json)
       handle_message(message)
-      delete_sent_message(message) if message.is_a?(S2::Messages::ReceptionStatus)
     rescue JSON::ParserError
       @logger.error("Received invalid JSON: #{message_json}")
     rescue KeyError => e
@@ -61,20 +54,14 @@ def close(message: nil)
 
     def reply(message, status:)
       if message.is_a?(S2::Messages::ReceptionStatus)
-        send_message(
-          S2::Messages::ReceptionStatus,
-          status:,
-          subject_message_id: message.subject_message_id,
-        )
+        @logger.error("Cannot reply to ReceptionStatus message")
       else
         send_message(
           S2::Messages::ReceptionStatus,
           status:,
           subject_message_id: message.message_id,
         )
       end
-
-      @ws.close if status == S2::Messages::ReceptionStatusValues::PermanentError
     end
 
     def message_sent?(message_id)

spec/lib/s2/connection_spec.rb

@@ -31,36 +31,59 @@
     end
 
     context "when the message is a ReceptionStatus" do
-      it "logs an error if the status is not OK" do
+      it "logs an error when the original message has not been sent" do
+        logger = Logger.new(nil)
+        connection = described_class.new(ws, logger:)
+
+        allow(logger).to receive(:error)
+
+        reception_status = build(:s2_reception_status, :invalid_content, subject_message_id: "123")
+        connection.receive_message(reception_status.to_json)
+
+        expect(logger).to have_received(:error).with("Received ReceptionStatus for unknown message ID 123")
+      end
+
+      it "does not remove the original message from the sent messages when it was not sent" do
         allow(SecureRandom).to receive(:uuid).and_return("123")
 
         logger = Logger.new(nil)
         connection = described_class.new(ws, logger:)
 
-        allow(logger).to receive(:error)
+        reception_status = build(:s2_reception_status, :invalid_content, subject_message_id: "123")
+
+        expect do
+          connection.receive_message(reception_status.to_json)
+        end.not_to change { connection.sent_messages.size }
+      end
+
+      it "removes the original message from the sent messages" do
+        allow(SecureRandom).to receive(:uuid).and_return("123")
+
+        logger = Logger.new(nil)
+        connection = described_class.new(ws, logger:)
 
         connection.send_message(S2::Messages::Handshake, { role: S2::Messages::EnergyManagementRole::Cem })
         reception_status = build(:s2_reception_status, :invalid_content, subject_message_id: "123")
-        connection.receive_message(reception_status.to_json)
 
-        expect(logger).to have_received(:error)
-          .with(/Received ReceptionStatus with status INVALID_CONTENT for message ID 123/)
+        expect do
+          connection.receive_message(reception_status.to_json)
+        end.to change { connection.sent_messages.size }.from(1).to(0)
       end
 
-      it "sends an error response when the original message is not known",
-         skip: "sending this error causes an endless loop" do
-           error_response = build(:s2_reception_status, :invalid_content, subject_message_id: "123")
+      it "closes the connection when the status is PermanentError" do
+        allow(SecureRandom).to receive(:uuid).and_return("123")
 
-           logger = Logger.new(nil)
-           connection = described_class.new(ws, logger:)
+        logger = Logger.new(nil)
+        connection = described_class.new(ws, logger:)
+        allow(connection).to receive(:close)
 
-           allow(logger).to receive(:error)
+        connection.send_message(S2::Messages::Handshake, { role: S2::Messages::EnergyManagementRole::Cem })
+        reception_status = build(:s2_reception_status, :permanent_error, subject_message_id: "123")
 
-           reception_status = build(:s2_reception_status, :invalid_content, subject_message_id: "123")
-           connection.receive_message(reception_status.to_json)
+        connection.receive_message(reception_status.to_json)
 
-           expect(ws).to have_sent_message(error_response.to_json)
-         end
+        expect(connection).to have_received(:close)
+      end
     end
   end