fix(robot-server, api): Standardize command slice behavior (#18203)

Closes EXEC-1466

Currently, when requesting a command slice or a command error slice via the HTTP API, the exact behavior varies depending on the run status, the “current” status of the run, and which parameters are (or are not) supplied. The behavior varies in the following way:

* When invoking get_slice with identical cursor and pageLength parameters, there is inconsistency in the number of returned commands when the run is in a recovery state.
* When a run is historical, an unsupplied cursor but a supplied pageLength returns the most recent pageLength commands, however, when a run is current, only the most recently running/executed command is returned.
* The above always applies to run command errors.

This commit fixes those issues by standardizing behavior in a manner that is most useful given typical client access patterns: if a cursor is not supplied but a pageLength is supplied, we return the pageLength most recent commands and command errors (so all behavior reflects the current behavior if the command slice is derived from a historical run). After doing a complete client-side audit, the client actually seemingly expects the API to behave this way, and has fortunately worked by chance.

(cherry picked from commit 2c374ff)

Author: mjhuff

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

@@ -611,49 +611,35 @@ def get_slice(
         """Get a subset of commands around a given cursor.
 
         If the cursor is omitted, a cursor will be selected automatically
-        based on the currently running or most recently executed command.
+        based on the currently running or most recently executed command,
+        and the slice of commands returned is the previous `length` commands
+        inclusive of the currently running or most recently executed command.
         """
         command_ids = self._state.command_history.get_filtered_command_ids(
             include_fixit_commands=include_fixit_commands
         )
-        running_command = self._state.command_history.get_running_command()
-        queued_command_ids = self._state.command_history.get_queue_ids()
         total_length = len(command_ids)
 
-        # TODO(mm, 2024-05-17): This looks like it's attempting to do the same thing
-        # as self.get_current(), but in a different way. Can we unify them?
         if cursor is None:
-            if running_command is not None:
-                cursor = running_command.index
-            elif len(queued_command_ids) > 0:
-                # Get the most recently executed command,
-                # which we can find just before the first queued command.
-                cursor = (
-                    self._state.command_history.get(queued_command_ids.head()).index - 1
-                )
-            elif (
-                self._state.run_result
-                and self._state.run_result == RunResult.FAILED
-                and self._state.failed_command
-            ):
-                # Currently, if the run fails, we mark all the commands we didn't
-                # reach as failed. This makes command status alone insufficient to
-                # find the most recent command that actually executed, so we need to
-                # store that separately.
-                cursor = self._state.failed_command.index
+            current_pointer = self.get_current()
+
+            if current_pointer is not None:
+                cursor = current_pointer.index
             else:
-                cursor = total_length - length
+                cursor = total_length - 1
+
+            cursor = max(cursor - length + 1, 0)
 
         # start is inclusive, stop is exclusive
-        actual_cursor = max(0, min(cursor, total_length - 1))
-        stop = min(total_length, actual_cursor + length)
+        start = max(0, min(cursor, total_length - 1))
+        stop = min(total_length, start + length)
         commands = self._state.command_history.get_slice(
-            start=actual_cursor, stop=stop, command_ids=command_ids
+            start=start, stop=stop, command_ids=command_ids
         )
 
         return CommandSlice(
             commands=commands,
-            cursor=actual_cursor,
+            cursor=start,
             total_length=total_length,
         )
 

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

@@ -1000,8 +1000,8 @@ def test_get_slice_default_cursor_failed_command() -> None:
     result = subject.get_slice(cursor=None, length=3, include_fixit_commands=True)
 
     assert result == CommandSlice(
-        commands=[command_3, command_4],
-        cursor=2,
+        commands=[command_2, command_3, command_4],
+        cursor=1,
         total_length=4,
     )
 
@@ -1022,14 +1022,14 @@ def test_get_slice_default_cursor_running() -> None:
     result = subject.get_slice(cursor=None, length=2, include_fixit_commands=True)
 
     assert result == CommandSlice(
-        commands=[command_3, command_4],
-        cursor=2,
+        commands=[command_2, command_3],
+        cursor=1,
         total_length=5,
     )
 
 
 def test_get_slice_without_fixit() -> None:
-    """It should select a cursor based on the running command, if present."""
+    """It should filter out fixit commands when requested."""
     command_1 = create_succeeded_command(command_id="command-id-1")
     command_2 = create_succeeded_command(command_id="command-id-2")
     command_3 = create_running_command(command_id="command-id-3")
@@ -1071,3 +1071,44 @@ def test_get_slice_without_fixit() -> None:
         cursor=0,
         total_length=5,
     )
+
+
+def test_get_slice_large_length() -> None:
+    """It should handle cases where length is larger than available commands."""
+    command_1 = create_succeeded_command(command_id="command-id-1")
+    command_2 = create_succeeded_command(command_id="command-id-2")
+    command_3 = create_running_command(command_id="command-id-3")
+
+    subject = get_command_view(
+        commands=[command_1, command_2, command_3],
+        running_command_id="command-id-3",
+    )
+
+    result = subject.get_slice(cursor=None, length=10, include_fixit_commands=True)
+
+    assert result == CommandSlice(
+        commands=[command_1, command_2, command_3],
+        cursor=0,
+        total_length=3,
+    )
+
+
+def test_get_slice_explicit_cursor_with_length() -> None:
+    """It should use the cursor as the start position when explicitly provided."""
+    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")
+    command_4 = create_succeeded_command(command_id="command-id-4")
+    command_5 = create_succeeded_command(command_id="command-id-5")
+
+    subject = get_command_view(
+        commands=[command_1, command_2, command_3, command_4, command_5],
+    )
+
+    result = subject.get_slice(cursor=1, length=3, include_fixit_commands=True)
+
+    assert result == CommandSlice(
+        commands=[command_2, command_3, command_4],
+        cursor=1,
+        total_length=5,
+    )

robot-server/robot_server/commands/router.py

@@ -135,7 +135,9 @@ 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 currently running or most recently executed command."
+                " based on the currently running or most recently executed command, "
+                " and the slice of commands returned is the previous `pageLength` commands"
+                " inclusive of the currently running or most recently executed command."
             ),
         ),
     ] = None,

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

@@ -197,7 +197,9 @@ async def get_run_commands(
             description=(
                 "The starting index of the desired first command in the list."
                 " If unspecified, a cursor will be selected automatically"
-                " based on the currently running or most recently executed command."
+                " based on the currently running or most recently executed command, "
+                " and the slice of commands returned is the previous `length` commands"
+                " inclusive of the currently running or most recently executed command."
             ),
         ),
     ] = None,

robot-server/robot_server/runs/router/base_router.py

@@ -473,7 +473,8 @@ async def get_run_commands_error(
             description=(
                 "The starting index of the desired first command error in the list."
                 " If unspecified, a cursor will be selected automatically"
-                " based on the last error added."
+                " based on the last error added, and the slice of errors returned "
+                " is the previous `pageLength` errors."
             ),
         ),
     ] = None,
@@ -490,12 +491,9 @@ async def get_run_commands_error(
         all_errors_count = run_data_manager.get_command_errors_count(run_id=runId)
 
         if cursor is None:
-            if all_errors_count > 0:
-                # Get the most recent error,
-                # which we can find just at the end of the list.
-                cursor = all_errors_count - 1
-            else:
-                cursor = 0
+            cursor = max(all_errors_count - 1, 0)
+            cursor = max(cursor - pageLength + 1, 0)
+            cursor = min(cursor, all_errors_count)
 
         command_error_slice = run_data_manager.get_command_error_slice(
             run_id=runId,

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

@@ -276,7 +276,9 @@ async def get_run_commands(
             description=(
                 "The starting index of the desired first command in the list."
                 " If unspecified, a cursor will be selected automatically"
-                " based on the currently running or most recently executed command."
+                " based on the currently running or most recently executed command, "
+                " and the slice of commands returned is the previous `pageLength` commands"
+                " inclusive of the currently running or most recently executed command."
             ),
         ),
     ] = None,

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

@@ -91,9 +91,36 @@ stages:
               key: !anystr
               createdAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
         meta:
-          cursor: 3
+          cursor: 0
           totalLength: 4
         data:
+          - id: !anystr
+            key: !anystr
+            commandType: home
+            createdAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
+            startedAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
+            completedAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
+            status: succeeded
+            params: !anydict
+            notes: !anylist
+          - id: !anystr
+            key: !anystr
+            commandType: loadLabware
+            createdAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
+            startedAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
+            completedAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
+            status: succeeded
+            params: !anydict
+            notes: !anylist
+          - id: !anystr
+            key: !anystr
+            commandType: loadPipette
+            createdAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
+            startedAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
+            completedAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
+            status: succeeded
+            params: !anydict
+            notes: !anylist
           - id: !anystr
             key: !anystr
             commandType: aspirate
@@ -115,12 +142,6 @@ stages:
               pipetteId: pipetteId
               labwareId: tipRackId
               wellName: A1
-              wellLocation:
-                origin: bottom
-                offset:
-                  x: 0
-                  y: 0
-                  z: 1
-                volumeOffset: 0
-              flowRate: 3.78
-              volume: 100
+              wellLocation: !anydict
+              flowRate: !anyfloat
+              volume: !anyfloat

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

@@ -92,17 +92,51 @@ stages:
               key: !anystr
               createdAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
         meta:
-          cursor: 3
+          cursor: 0
           totalLength: 4
         data:
           - id: !anystr
             key: !anystr
+            commandType: home
+            createdAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
+            startedAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
+            completedAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
+            status: succeeded
+            params: !anydict
+            notes: !anylist
+          - id: !re_fullmatch "commands\\.LOAD_LABWARE-\\d+"
+            key: !re_fullmatch "commands\\.LOAD_LABWARE-\\d+"
+            commandType: loadLabware
+            createdAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
+            startedAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
+            completedAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
+            status: succeeded
+            params:
+              location:
+                slotName: "1"
+              loadName: opentrons_96_tiprack_300ul
+              namespace: opentrons
+              version: 1
+            notes: !anylist
+          - id: !re_fullmatch "commands\\.LOAD_PIPETTE-\\d+"
+            key: !re_fullmatch "commands\\.LOAD_PIPETTE-\\d+"
+            commandType: loadPipette
+            createdAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
+            startedAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
+            completedAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
+            status: succeeded
+            params:
+              pipetteName: p300_single
+              mount: right
+            notes: !anylist
+          - id: !re_fullmatch "command\\.ASPIRATE-\\d+"
+            key: !re_fullmatch "command\\.ASPIRATE-\\d+"
             commandType: aspirate
             createdAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
             startedAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
             completedAt: !re_fullmatch "\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d+(Z|([+-]\\d{2}:\\d{2}))"
             status: failed
-            notes: []
+            notes: !anylist
             error:
               id: !anystr
               errorType: LegacyContextCommandError
@@ -119,9 +153,9 @@ stages:
               wellLocation:
                 origin: top
                 offset:
-                  x: 0
-                  y: 0
-                  z: 0
-                volumeOffset: 0
-              flowRate: 150
-              volume: 100
+                  x: !anyfloat
+                  y: !anyfloat
+                  z: !anyfloat
+                volumeOffset: !anyfloat
+              flowRate: !anyfloat
+              volume: !anyfloat