refactor(api,robot-server): Various small refactors (#14665)

api/src/opentrons/protocol_engine/protocol_engine.py

@@ -241,7 +241,7 @@ def estop(self, maintenance_run: bool) -> None:
         if self._state_store.commands.get_is_stopped():
             return
         current_id = (
-            self._state_store.commands.state.running_command_id
+            self._state_store.commands.get_running_command_id()
             or self._state_store.commands.state.queued_command_ids.head(None)
         )
 

api/src/opentrons/protocol_engine/state/commands.py

@@ -271,45 +271,30 @@ def handle_action(self, action: Action) -> None:  # noqa: C901
                 error=action.error,
             )
             prev_entry = self._state.commands_by_id[action.command_id]
-            self._state.commands_by_id[action.command_id] = CommandEntry(
-                index=prev_entry.index,
-                # TODO(mc, 2022-06-06): add new "cancelled" status or similar
-                # and don't set `completedAt` in commands other than the
-                # specific one that failed
-                command=prev_entry.command.copy(
-                    update={
-                        "error": error_occurrence,
-                        "completedAt": action.failed_at,
-                        "status": CommandStatus.FAILED,
-                    }
-                ),
+            # TODO(mc, 2022-06-06): add new "cancelled" status or similar
+            self._update_to_failed(
+                command_id=action.command_id,
+                failed_at=action.failed_at,
+                error_occurrence=error_occurrence,
             )
 
             self._state.failed_command = self._state.commands_by_id[action.command_id]
+
             if prev_entry.command.intent == CommandIntent.SETUP:
-                other_command_ids_to_fail = [
-                    *[i for i in self._state.queued_setup_command_ids],
-                ]
+                other_command_ids_to_fail = self._state.queued_setup_command_ids
+                for id in other_command_ids_to_fail:
+                    self._update_to_failed(
+                        command_id=id, failed_at=action.failed_at, error_occurrence=None
+                    )
                 self._state.queued_setup_command_ids.clear()
             else:
-                other_command_ids_to_fail = [
-                    *[i for i in self._state.queued_command_ids],
-                ]
+                other_command_ids_to_fail = self._state.queued_command_ids
+                for id in other_command_ids_to_fail:
+                    self._update_to_failed(
+                        command_id=id, failed_at=action.failed_at, error_occurrence=None
+                    )
                 self._state.queued_command_ids.clear()
 
-            for command_id in other_command_ids_to_fail:
-                prev_entry = self._state.commands_by_id[command_id]
-
-                self._state.commands_by_id[command_id] = CommandEntry(
-                    index=prev_entry.index,
-                    command=prev_entry.command.copy(
-                        update={
-                            "completedAt": action.failed_at,
-                            "status": CommandStatus.FAILED,
-                        }
-                    ),
-                )
-
             if self._state.running_command_id == action.command_id:
                 self._state.running_command_id = None
 
@@ -378,6 +363,24 @@ def handle_action(self, action: Action) -> None:  # noqa: C901
                 elif action.door_state == DoorState.CLOSED:
                     self._state.is_door_blocking = False
 
+    def _update_to_failed(
+        self,
+        command_id: str,
+        failed_at: datetime,
+        error_occurrence: Optional[ErrorOccurrence],
+    ) -> None:
+        prev_entry = self._state.commands_by_id[command_id]
+        updated_command = prev_entry.command.copy(
+            update={
+                "completedAt": failed_at,
+                "status": CommandStatus.FAILED,
+                **({"error": error_occurrence} if error_occurrence else {}),
+            }
+        )
+        self._state.commands_by_id[command_id] = CommandEntry(
+            index=prev_entry.index, command=updated_command
+        )
+
     @staticmethod
     def _map_run_exception_to_error_occurrence(
         error_id: str, created_at: datetime, exception: Exception
@@ -516,6 +519,10 @@ def get_error(self) -> Optional[ErrorOccurrence]:
         else:
             return run_error or finish_error
 
+    def get_running_command_id(self) -> Optional[str]:
+        """Return the ID of the command that's currently running, if there is one."""
+        return self._state.running_command_id
+
     def get_current(self) -> Optional[CurrentCommand]:
         """Return the "current" command, if any.
 
@@ -632,6 +639,9 @@ def get_all_commands_final(self) -> bool:
         )
 
         if no_command_running and no_command_to_execute:
+            # TODO(mm, 2024-03-14): This is a slow O(n) scan. When a long run ends and
+            # we reach this loop, it can disrupt the robot server.
+            # https://opentrons.atlassian.net/browse/EXEC-55
             for command_id in self._state.all_command_ids:
                 command = self._state.commands_by_id[command_id].command
                 if command.error and command.intent != CommandIntent.SETUP:

api/tests/opentrons/protocol_engine/state/test_command_view.py

@@ -683,6 +683,15 @@ def test_get_okay_to_clear(subject: CommandView, expected_is_okay: bool) -> None
     assert subject.get_is_okay_to_clear() is expected_is_okay
 
 
+def test_get_running_command_id() -> None:
+    """It should return the running command ID."""
+    subject_with_running = get_command_view(running_command_id="command-id")
+    assert subject_with_running.get_running_command_id() == "command-id"
+
+    subject_without_running = get_command_view(running_command_id=None)
+    assert subject_without_running.get_running_command_id() is None
+
+
 def test_get_current() -> None:
     """It should return the "current" command."""
     subject = get_command_view(
@@ -851,7 +860,7 @@ def test_get_slice_default_cursor_running() -> None:
 
 
 def test_get_slice_default_cursor_queued() -> None:
-    """It should select a cursor based on the next queued command, if present."""
+    """It should select a cursor automatically."""
     command_1 = create_succeeded_command(command_id="command-id-1")
     command_2 = create_succeeded_command(command_id="command-id-2")
     command_3 = create_succeeded_command(command_id="command-id-3")
@@ -861,7 +870,7 @@ def test_get_slice_default_cursor_queued() -> None:
     subject = get_command_view(
         commands=[command_1, command_2, command_3, command_4, command_5],
         running_command_id=None,
-        queued_command_ids=["command-id-4", "command-id-4", "command-id-5"],
+        queued_command_ids=[command_4.id, command_5.id],
     )
 
     result = subject.get_slice(cursor=None, length=2)

api/tests/opentrons/protocol_engine/test_protocol_engine.py

@@ -749,7 +749,7 @@ async def test_estop_during_command(
     decoy.when(model_utils.get_timestamp()).then_return(timestamp)
     decoy.when(model_utils.generate_id()).then_return(error_id)
     decoy.when(state_store.commands.get_is_stopped()).then_return(False)
-    decoy.when(state_store.commands.state.running_command_id).then_return(command_id)
+    decoy.when(state_store.commands.get_running_command_id()).then_return(command_id)
     decoy.when(state_store.commands.state.queued_command_ids).then_return(
         fake_command_set
     )
@@ -793,7 +793,7 @@ async def test_estop_without_command(
     decoy.when(model_utils.get_timestamp()).then_return(timestamp)
     decoy.when(model_utils.generate_id()).then_return(error_id)
     decoy.when(state_store.commands.get_is_stopped()).then_return(False)
-    decoy.when(state_store.commands.state.running_command_id).then_return(None)
+    decoy.when(state_store.commands.get_running_command_id()).then_return(None)
 
     expected_stop = StopAction(from_estop=True)
     expected_hardware_stop = HardwareStoppedAction(

robot-server/robot_server/commands/router.py

@@ -140,7 +140,7 @@ async def get_commands_list(
         description=(
             "The starting index of the desired first command in the list."
             " If unspecified, a cursor will be selected automatically"
-            " based on the next queued or more recently executed command."
+            " based on the currently running or most recently executed command."
         ),
     ),
     pageLength: int = Query(

robot-server/tests/integration/http_api/runs/test_json_v6_protocol_run.tavern.yaml

@@ -329,7 +329,14 @@ stages:
       status_code: 200
       json:
         links:
-          current: !anydict
+          current:
+            href: !anystr
+            meta:
+              runId: !anystr
+              commandId: !anystr
+              index: 14
+              key: !anystr
+              createdAt: !anystr
         meta:
           cursor: 0
           totalLength: 15
@@ -564,7 +571,14 @@ stages:
       status_code: 200
       json:
         links:
-          current: !anydict
+          current:
+            href: !anystr
+            meta:
+              runId: !anystr
+              commandId: !anystr
+              index: 14
+              key: !anystr
+              createdAt: !anystr
         meta:
           cursor: 5
           totalLength: 15

robot-server/tests/integration/http_api/runs/test_json_v6_run_failure.tavern.yaml

@@ -81,7 +81,14 @@ stages:
       status_code: 200
       json:
         links:
-          current: !anydict
+          current:
+            href: !anystr
+            meta:
+              runId: !anystr
+              commandId: !anystr
+              index: 4
+              key: !anystr
+              createdAt: !anystr
         meta:
           cursor: 3
           totalLength: 5

robot-server/tests/integration/http_api/runs/test_json_v7_protocol_run.tavern.yaml

@@ -329,7 +329,14 @@ stages:
       status_code: 200
       json:
         links:
-          current: !anydict
+          current:
+            href: !anystr
+            meta:
+              runId: !anystr
+              commandId: !anystr
+              index: 14
+              key: !anystr
+              createdAt: !anystr
         meta:
           cursor: 0
           totalLength: 15

robot-server/tests/integration/http_api/runs/test_papi_v2_run_failure.tavern.yaml

@@ -82,7 +82,14 @@ stages:
       status_code: 200
       json:
         links:
-          current: !anydict
+          current:
+            href: !anystr
+            meta:
+              runId: !anystr
+              commandId: !anystr
+              index: 3
+              key: !anystr
+              createdAt: !anystr
         meta:
           cursor: 3
           totalLength: 4

robot-server/tests/integration/http_api/runs/test_protocol_run.tavern.yaml

@@ -151,7 +151,14 @@ stages:
       status_code: 200
       json:
         links:
-          current: !anydict
+          current:
+            href: !anystr
+            meta:
+              runId: !anystr
+              commandId: !anystr
+              index: 1
+              key: !anystr
+              createdAt: !anystr
         meta:
           cursor: 0
           totalLength: 2

robot-server/tests/integration/http_api/runs/test_run_queued_protocol_commands.tavern.yaml

@@ -139,7 +139,14 @@ stages:
       status_code: 200
       json:
         links:
-          current: !anydict
+          current:
+            href: !anystr
+            meta:
+              runId: !anystr
+              commandId: !anystr
+              index: 3
+              key: !anystr
+              createdAt: !anystr
         meta:
           cursor: 0
           totalLength: 4