feat(robot-server): Return a currentlyRecoveringFrom command pointer (

#15198)

api-client/src/runs/commands/types.ts

@@ -15,16 +15,18 @@ export interface CommandDetail {
 }
 
 export interface CommandsLinks {
-  current: {
-    // link to the currently executing command
-    href: string
-    meta: {
-      runId: string
-      commandId: string
-      key: string
-      createdAt: string
-      index: number
-    }
+  current?: CommandsLink | null
+  currentlyRecoveringFrom?: CommandsLink | null
+}
+
+interface CommandsLink {
+  href: string
+  meta: {
+    runId: string
+    commandId: string
+    key: string
+    createdAt: string
+    index: number
   }
 }
 

api/src/opentrons/protocol_engine/__init__.py

@@ -22,7 +22,7 @@
     CommandType,
     CommandIntent,
 )
-from .state import State, StateView, StateSummary, CommandSlice, CurrentCommand, Config
+from .state import State, StateView, StateSummary, CommandSlice, CommandPointer, Config
 from .plugins import AbstractPlugin
 
 from .types import (
@@ -85,7 +85,7 @@
     "State",
     "StateView",
     "CommandSlice",
-    "CurrentCommand",
+    "CommandPointer",
     # public value interfaces and models
     "LabwareOffset",
     "LabwareOffsetCreate",

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

@@ -7,7 +7,7 @@
     CommandState,
     CommandView,
     CommandSlice,
-    CurrentCommand,
+    CommandPointer,
 )
 from .command_history import CommandEntry
 from .labware import LabwareState, LabwareView
@@ -39,7 +39,7 @@
     "CommandState",
     "CommandView",
     "CommandSlice",
-    "CurrentCommand",
+    "CommandPointer",
     "CommandEntry",
     # labware state and values
     "LabwareState",

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

@@ -107,8 +107,8 @@ class CommandSlice:
 
 
 @dataclass(frozen=True)
-class CurrentCommand:
-    """The "current" command's ID and index in the overall commands list."""
+class CommandPointer:
+    """Brief info about a command and where to find it."""
 
     command_id: str
     command_key: str
@@ -593,28 +593,28 @@ def get_queue_ids(self) -> OrderedSet[str]:
         """Get the IDs of all queued protocol commands, in FIFO order."""
         return self._state.command_history.get_queue_ids()
 
-    def get_current(self) -> Optional[CurrentCommand]:
+    def get_current(self) -> Optional[CommandPointer]:
         """Return the "current" command, if any.
 
         The "current" command is the command that is currently executing,
         or the most recent command to have completed.
         """
         running_command = self._state.command_history.get_running_command()
         if running_command:
-            return CurrentCommand(
+            return CommandPointer(
                 command_id=running_command.command.id,
                 command_key=running_command.command.key,
                 created_at=running_command.command.createdAt,
                 index=running_command.index,
             )
 
-        final_command = self.get_final_command()
-        if final_command:
-            return CurrentCommand(
-                command_id=final_command.command.id,
-                command_key=final_command.command.key,
-                created_at=final_command.command.createdAt,
-                index=final_command.index,
+        most_recently_finalized_command = self.get_most_recently_finalized_command()
+        if most_recently_finalized_command:
+            return CommandPointer(
+                command_id=most_recently_finalized_command.command.id,
+                command_key=most_recently_finalized_command.command.key,
+                created_at=most_recently_finalized_command.command.createdAt,
+                index=most_recently_finalized_command.index,
             )
 
         return None
@@ -678,7 +678,7 @@ def get_is_running(self) -> bool:
         """Get whether the protocol is running & queued commands should be executed."""
         return self._state.queue_status == QueueStatus.RUNNING
 
-    def get_final_command(self) -> Optional[CommandEntry]:
+    def get_most_recently_finalized_command(self) -> Optional[CommandEntry]:
         """Get the most recent command that has reached its final `status`. See get_command_is_final."""
         run_requested_to_stop = self._state.run_result is not None
 
@@ -691,22 +691,22 @@ def get_final_command(self) -> Optional[CommandEntry]:
             else:
                 return self._state.command_history.get_prev(tail_command.command.id)
         else:
-            final_command = self._state.command_history.get_terminal_command()
+            most_recently_finalized = self._state.command_history.get_terminal_command()
             # This iteration is effectively O(1) as we'll only ever have to iterate one or two times at most.
-            while final_command is not None:
+            while most_recently_finalized is not None:
                 next_command = self._state.command_history.get_next(
-                    final_command.command.id
+                    most_recently_finalized.command.id
                 )
                 if (
                     next_command is not None
                     and next_command.command.status != CommandStatus.QUEUED
                     and next_command.command.status != CommandStatus.RUNNING
                 ):
-                    final_command = next_command
+                    most_recently_finalized = next_command
                 else:
                     break
 
-        return final_command
+            return most_recently_finalized
 
     def get_command_is_final(self, command_id: str) -> bool:
         """Get whether a given command has reached its final `status`.
@@ -751,9 +751,24 @@ def get_all_commands_final(self) -> bool:
 
         return no_command_running and no_command_to_execute
 
+    def get_recovery_target(self) -> Optional[CommandPointer]:
+        """Return the command currently undergoing error recovery, if any."""
+        recovery_target_command_id = self._state.recovery_target_command_id
+        if recovery_target_command_id is None:
+            return None
+        else:
+            entry = self._state.command_history.get(recovery_target_command_id)
+            return CommandPointer(
+                command_id=entry.command.id,
+                command_key=entry.command.key,
+                created_at=entry.command.createdAt,
+                index=entry.index,
+            )
+
     def get_recovery_in_progress_for_command(self, command_id: str) -> bool:
         """Return whether the given command failed and its error recovery is in progress."""
-        return self._state.recovery_target_command_id == command_id
+        pointer = self.get_recovery_target()
+        return pointer is not None and pointer.command_id == command_id
 
     def raise_fatal_command_error(self) -> None:
         """Raise the run's fatal command error, if there was one, as an exception.

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

@@ -356,8 +356,8 @@ def test_error_recovery_type_tracking() -> None:
     assert view.get_error_recovery_type("c2") == ErrorRecoveryType.FAIL_RUN
 
 
-def test_get_recovery_in_progress_for_command() -> None:
-    """It should return whether error recovery is in progress for the given command."""
+def test_recovery_target_tracking() -> None:
+    """It should keep track of the command currently undergoing error recovery."""
     subject = CommandStore(config=_make_config(), is_door_open=False)
     subject_view = CommandView(subject.state)
 
@@ -382,12 +382,16 @@ def test_get_recovery_in_progress_for_command() -> None:
     subject.handle_action(fail_1)
 
     # c1 failed recoverably and we're currently recovering from it.
+    recovery_target = subject_view.get_recovery_target()
+    assert recovery_target is not None
+    assert recovery_target.command_id == "c1"
     assert subject_view.get_recovery_in_progress_for_command("c1")
 
     resume_from_1_recovery = actions.ResumeFromRecoveryAction()
     subject.handle_action(resume_from_1_recovery)
 
     # c1 failed recoverably, but we've already completed its recovery.
+    assert subject_view.get_recovery_target() is None
     assert not subject_view.get_recovery_in_progress_for_command("c1")
 
     queue_2 = actions.QueueCommandAction(
@@ -411,6 +415,9 @@ def test_get_recovery_in_progress_for_command() -> None:
     subject.handle_action(fail_2)
 
     # c2 failed recoverably and we're currently recovering from it.
+    recovery_target = subject_view.get_recovery_target()
+    assert recovery_target is not None
+    assert recovery_target.command_id == "c2"
     assert subject_view.get_recovery_in_progress_for_command("c2")
     # ...and that means we're *not* currently recovering from c1,
     # even though it failed recoverably before.
@@ -439,7 +446,8 @@ def test_get_recovery_in_progress_for_command() -> None:
     subject.handle_action(fail_3)
 
     # c3 failed, but not recoverably.
-    assert not subject_view.get_recovery_in_progress_for_command("c2")
+    assert subject_view.get_recovery_target() is None
+    assert not subject_view.get_recovery_in_progress_for_command("c3")
 
 
 def test_final_state_after_estop() -> None:

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

@@ -25,7 +25,7 @@
     CommandState,
     CommandView,
     CommandSlice,
-    CurrentCommand,
+    CommandPointer,
     RunResult,
     QueueStatus,
 )
@@ -846,7 +846,7 @@ def test_get_current() -> None:
         queued_command_ids=[],
         commands=[command],
     )
-    assert subject.get_current() == CurrentCommand(
+    assert subject.get_current() == CommandPointer(
         index=0,
         command_id="command-id",
         command_key="command-key",
@@ -866,7 +866,7 @@ def test_get_current() -> None:
     subject = get_command_view(commands=[command_1, command_2])
     subject.state.command_history._set_terminal_command_id(command_1.id)
 
-    assert subject.get_current() == CurrentCommand(
+    assert subject.get_current() == CommandPointer(
         index=1,
         command_id="command-id-2",
         command_key="key-2",
@@ -886,7 +886,7 @@ def test_get_current() -> None:
     subject = get_command_view(commands=[command_1, command_2])
     subject.state.command_history._set_terminal_command_id(command_1.id)
 
-    assert subject.get_current() == CurrentCommand(
+    assert subject.get_current() == CommandPointer(
         index=1,
         command_id="command-id-2",
         command_key="key-2",

robot-server/robot_server/maintenance_runs/maintenance_run_data_manager.py

@@ -7,7 +7,7 @@
     LabwareOffsetCreate,
     StateSummary,
     CommandSlice,
-    CurrentCommand,
+    CommandPointer,
     Command,
 )
 
@@ -183,7 +183,7 @@ def get_commands_slice(
         )
         return the_slice
 
-    def get_current_command(self, run_id: str) -> Optional[CurrentCommand]:
+    def get_current_command(self, run_id: str) -> Optional[CommandPointer]:
         """Get the "current" command, if any.
 
         See `ProtocolEngine.state_view.commands.get_current()` for the definition
@@ -194,7 +194,25 @@ def get_current_command(self, run_id: str) -> Optional[CurrentCommand]:
         """
         if self._engine_store.current_run_id == run_id:
             return self._engine_store.engine.state_view.commands.get_current()
-        return None
+        else:
+            # todo(mm, 2024-05-20):
+            # For historical runs to behave consistently with the current run,
+            # this should be the most recently completed command, not `None`.
+            return None
+
+    def get_recovery_target_command(self, run_id: str) -> Optional[CommandPointer]:
+        """Get the current error recovery target.
+
+        See `ProtocolEngine.state_view.commands.get_recovery_target()`.
+
+        Args:
+            run_id: ID of the run.
+        """
+        if self._engine_store.current_run_id == run_id:
+            return self._engine_store.engine.state_view.commands.get_recovery_target()
+        else:
+            # Historical runs can't have any ongoing error recovery.
+            return None
 
     def get_command(self, run_id: str, command_id: str) -> Command:
         """Get a run's command by ID.

robot-server/robot_server/maintenance_runs/router/commands_router.py

@@ -7,6 +7,7 @@
 from fastapi import APIRouter, Depends, Query, status
 
 from opentrons.protocol_engine import (
+    CommandPointer,
     ProtocolEngine,
     commands as pe_commands,
 )
@@ -220,6 +221,7 @@ async def get_run_commands(
         raise RunNotFound(detail=str(e)).as_error(status.HTTP_404_NOT_FOUND) from e
 
     current_command = run_data_manager.get_current_command(run_id=runId)
+    recovery_target_command = run_data_manager.get_recovery_target_command(run_id=runId)
 
     data = [
         MaintenanceRunCommandSummary.construct(
@@ -242,19 +244,10 @@ async def get_run_commands(
         totalLength=command_slice.total_length,
     )
 
-    links = CommandCollectionLinks()
-
-    if current_command is not None:
-        links.current = CommandLink(
-            href=f"/runs/{runId}/commands/{current_command.command_id}",
-            meta=CommandLinkMeta(
-                runId=runId,
-                commandId=current_command.command_id,
-                index=current_command.index,
-                key=current_command.command_key,
-                createdAt=current_command.created_at,
-            ),
-        )
+    links = CommandCollectionLinks.construct(
+        current=_make_command_link(runId, current_command),
+        currentlyRecoveringFrom=_make_command_link(runId, recovery_target_command),
+    )
 
     return await PydanticResponse.create(
         content=MultiBody.construct(data=data, meta=meta, links=links),
@@ -302,3 +295,22 @@ async def get_run_command(
         content=SimpleBody.construct(data=command),
         status_code=status.HTTP_200_OK,
     )
+
+
+def _make_command_link(
+    run_id: str, command_pointer: Optional[CommandPointer]
+) -> Optional[CommandLink]:
+    return (
+        CommandLink.construct(
+            href=f"/maintenance_runs/{run_id}/commands/{command_pointer.command_id}",
+            meta=CommandLinkMeta(
+                runId=run_id,
+                commandId=command_pointer.command_id,
+                index=command_pointer.index,
+                key=command_pointer.command_key,
+                createdAt=command_pointer.created_at,
+            ),
+        )
+        if command_pointer is not None
+        else None
+    )