fix(robot-server): matching RTPs check during analysis of non-RTP pro…

…tocols (#16171)

Closes RQA-3118

# Overview

The check for matching RTPs had a bug where if a protocol had no RTPs,
it would deem that protocol to have 'no previously matching analyses'
and hence would always re-analyze the protocol, even when the protocol
has not changed.

This PR fixes that check and adds tests so that we'll know if it happens
again.

Fixing this bug revealed another bug that the legacy context plugin task
being created while setting up the protocol runner, was never being
stopped/ cancelled, hence preventing the server from shutting down. To
fix that, I am stopping the orchestrator when the analyzer's destructor
is called.

## Test Plan and Hands on Testing

Follow the instructions in the RQA ticket above and see that the
erroneous behavior is not seen.

## Review requests

- check that the tests cover all cases of analyzing RTP & non-RTP
protocols with various combinations of new and previous parameters (if
any)

## Risk assessment

Medium. Bug fix, but need to make sure that all combinations of possible
protocol analysis params are covered. Also affects server shutdown.

---------

Co-authored-by: Josh McVey <[email protected]>

robot-server/robot_server/protocols/analysis_store.py

@@ -392,14 +392,18 @@ async def matching_rtp_values_in_analysis(
                 last_analysis_summary.id
             )
         )
-        if len(primitive_rtps_in_last_analysis) == 0:
-            # Protocols migrated from v4 will not have any entries in RTP table,
-            # this is fine and we should just trigger a new analysis and have
-            # the new values be stored in the RTP table.
-            return False
         csv_rtps_in_last_analysis = self._completed_store.get_csv_rtps_by_analysis_id(
             last_analysis_summary.id
         )
+        if (
+            len(new_parameters) != 0
+            and len(primitive_rtps_in_last_analysis) + len(csv_rtps_in_last_analysis)
+            == 0
+        ):
+            # Protocols migrated from v4 will not have any entries in RTP table,
+            # this is fine, and we should just trigger a new analysis and have
+            # the new values be stored in the RTP table.
+            return False
         total_params_in_last_analysis = list(
             primitive_rtps_in_last_analysis.keys()
         ) + list(csv_rtps_in_last_analysis.keys())

robot-server/robot_server/protocols/protocol_analyzer.py

@@ -1,5 +1,6 @@
 """Protocol analysis module."""
 import logging
+import asyncio
 from typing import Optional, List
 
 from opentrons_shared_data.robot.types import RobotType
@@ -137,6 +138,24 @@ async def update_to_failed_analysis(
             liquids=[],
         )
 
+    def __del__(self) -> None:
+        """Stop the simulating run orchestrator.
+
+        Once the analyzer is no longer in use- either because analysis completed
+        or was not required, stop the orchestrator so that all its background tasks
+        are stopped timely and do not block server shutdown.
+        """
+        if self._orchestrator is not None:
+            if self._orchestrator.get_is_okay_to_clear():
+                asyncio.run_coroutine_threadsafe(
+                    self._orchestrator.stop(), asyncio.get_running_loop()
+                )
+            else:
+                log.warning(
+                    "Analyzer is no longer in use but orchestrator is busy. "
+                    "Cannot stop the orchestrator currently."
+                )
+
 
 def create_protocol_analyzer(
     analysis_store: AnalysisStore,

robot-server/tests/integration/http_api/protocols/test_analyses.tavern.yaml

@@ -96,6 +96,9 @@ stages:
         data:
           forceReAnalyze: true
     response:
+      save:
+        json:
+          analysis_id2: data[1].id
       strict:
         - json:off
       status_code: 201
@@ -106,3 +109,36 @@ stages:
           - id: !anystr
             status: pending
             runTimeParameters: []
+
+
+  - name: Retry until analysis is completed
+    max_retries: 5
+    delay_after: 1
+    request:
+      url: '{ot2_server_base_url}/protocols/{protocol_id}/analyses/{analysis_id2}'
+    response:
+      strict:
+        - json:off
+      json:
+        data:
+          id: '{analysis_id2}'
+          status: completed
+
+
+  - name: Check that a new analysis is NOT started for the same protocol
+    request:
+      url: '{ot2_server_base_url}/protocols/{protocol_id}/analyses'
+      method: POST
+    response:
+      strict:
+        - json:off
+      status_code: 200
+      json:
+        meta:
+          cursor: 0
+          totalLength: 2
+        data:
+          - id: '{analysis_id}'
+            status: completed
+          - id: '{analysis_id2}'
+            status: completed

robot-server/tests/integration/http_api/protocols/test_analyses_with_run_time_parameters.tavern.yaml

@@ -200,10 +200,17 @@ stages:
           runTimeParameterValues:
             sample_count: 2.0
     response:
+      save:
+        json:
+          analysis_id3: data[2].id
+          run_time_parameters_data3: data[2].runTimeParameters
       strict:
         - json:off
       status_code: 201
       json:
+        meta:
+          cursor: 0
+          totalLength: 3
         data:
           - id: '{analysis_id}'
             status: completed
@@ -252,4 +259,41 @@ stages:
                 description: What pipette to use during the protocol.
               - displayName: Liquid handling CSV file
                 variableName: liq_handling_csv_file
-                description: A CSV file that contains wells to use for pipetting
+                description: A CSV file that contains wells to use for pipetting
+
+  - name: Retry until analysis is completed
+    max_retries: 5
+    delay_after: 1
+    request:
+      url: '{ot2_server_base_url}/protocols/{protocol_id}/analyses/{analysis_id3}'
+    response:
+      strict:
+        - json:off
+      json:
+        data:
+          id: '{analysis_id3}'
+          status: completed
+
+  - name: Check that a new analysis is NOT started for the protocol when RTP values are same
+    request:
+      url: '{ot2_server_base_url}/protocols/{protocol_id}/analyses'
+      method: POST
+      json:
+        data:
+          runTimeParameterValues:
+            sample_count: 2.0
+    response:
+      strict:
+        - json:off
+      status_code: 200
+      json:
+        meta:
+          cursor: 0
+          totalLength: 3
+        data:
+          - id: '{analysis_id}'
+            status: completed
+          - id: '{analysis_id2}'
+            status: completed
+          - id: '{analysis_id3}'
+            status: completed

robot-server/tests/protocols/test_analysis_store.py

@@ -642,3 +642,46 @@ async def test_matching_default_rtp_values_in_analysis_with_pending_analysis(
         await subject.matching_rtp_values_in_analysis(
             AnalysisSummary(id="analysis-id", status=AnalysisStatus.PENDING), []
         )
+
+
+async def test_matching_rtp_values_in_analysis_with_no_rtps(
+    decoy: Decoy,
+    sql_engine: SQLEngine,
+    subject: AnalysisStore,
+    protocol_store: ProtocolStore,
+) -> None:
+    """It should handle the cases of no RTPs, either previously or newly, appropriately."""
+    mock_completed_store = decoy.mock(cls=CompletedAnalysisStore)
+    subject = AnalysisStore(sql_engine=sql_engine, completed_store=mock_completed_store)
+    protocol_store.insert(make_dummy_protocol_resource(protocol_id="protocol-id"))
+
+    decoy.when(
+        mock_completed_store.get_primitive_rtps_by_analysis_id("analysis-2")
+    ).then_return({})
+    decoy.when(
+        mock_completed_store.get_csv_rtps_by_analysis_id("analysis-2")
+    ).then_return({})
+    assert (
+        await subject.matching_rtp_values_in_analysis(
+            last_analysis_summary=AnalysisSummary(
+                id="analysis-2", status=AnalysisStatus.COMPLETED
+            ),
+            new_parameters=[],
+        )
+        is True
+    )
+    decoy.when(
+        mock_completed_store.get_primitive_rtps_by_analysis_id("analysis-2")
+    ).then_return({})
+    decoy.when(
+        mock_completed_store.get_csv_rtps_by_analysis_id("analysis-2")
+    ).then_return({})
+    assert (
+        await subject.matching_rtp_values_in_analysis(
+            last_analysis_summary=AnalysisSummary(
+                id="analysis-2", status=AnalysisStatus.COMPLETED
+            ),
+            new_parameters=[mock_number_param("cool_param", 2.0)],
+        )
+        is False
+    )