feat(robot-server): Add PATCH /errorRecovery/settings endpoint with…

… `enableErrorRecovery` setting (#16355)

robot-server/robot_server/error_recovery/__init__.py

@@ -0,0 +1,5 @@
+"""The implementation of the `/errorRecovery` routes.
+
+Note that much of the code for the overall error recovery feature lives elsewhere,
+e.g. in `robot_server.runs`.
+"""

robot-server/robot_server/error_recovery/settings/__init__.py

@@ -0,0 +1 @@
+"""The implementation of the `/errorRecovery/settings` routes."""

robot-server/robot_server/error_recovery/settings/models.py

@@ -0,0 +1,46 @@
+"""HTTP request/response models for error recovery settings."""
+
+
+import textwrap
+from typing import Annotated
+import pydantic
+
+
+class ResponseData(pydantic.BaseModel):
+    """Response body data from the `/errorRecovery/settings` endpoints."""
+
+    enabled: Annotated[
+        bool,
+        pydantic.Field(
+            description=textwrap.dedent(
+                """\
+                Whether error recovery mode is globally enabled.
+                See `PATCH /errorRecovery/settings`.
+                """
+            )
+        ),
+    ]
+
+
+class RequestData(pydantic.BaseModel):
+    """Request body data for `PATCH /errorRecovery/settings`."""
+
+    enabled: Annotated[
+        bool | None,
+        pydantic.Field(
+            description=textwrap.dedent(
+                """\
+                If provided, globally enables or disables error recovery mode.
+
+                If this is `true`, a run (see the `/runs` endpoints) will *potentially*
+                enter recovery mode when an error happens, depending on the details of
+                the error and depending on `/runs/{runId}/errorRecoveryPolicy`.
+
+                If this is `false`, a run will just fail if it encounters an error.
+
+                The default is `true`. This currently only has an effect on Flex robots.
+                On OT-2s, error recovery is not supported.
+                """
+            )
+        ),
+    ] = None

robot-server/robot_server/error_recovery/settings/router.py

@@ -0,0 +1,70 @@
+"""FastAPI endpoint functions to implement `/errorRecovery/settings`."""
+
+
+from typing import Annotated
+
+import fastapi
+
+from robot_server.service.json_api import PydanticResponse, RequestModel, SimpleBody
+from .models import RequestData, ResponseData
+from .store import ErrorRecoverySettingStore, get_error_recovery_setting_store
+
+
+router = fastapi.APIRouter()
+_PATH = "/errorRecovery/settings"
+
+
+@PydanticResponse.wrap_route(
+    router.get,
+    path=_PATH,
+    summary="Get current error recovery settings",
+)
+async def get_error_recovery_settings(  # noqa: D103
+    store: Annotated[
+        ErrorRecoverySettingStore, fastapi.Depends(get_error_recovery_setting_store)
+    ]
+) -> PydanticResponse[SimpleBody[ResponseData]]:
+    return await _get_current_response(store)
+
+
+@PydanticResponse.wrap_route(
+    router.patch,
+    path=_PATH,
+    summary="Set error recovery settings",
+)
+async def patch_error_recovery_settings(  # noqa: D103
+    request_body: RequestModel[RequestData],
+    store: Annotated[
+        ErrorRecoverySettingStore, fastapi.Depends(get_error_recovery_setting_store)
+    ],
+) -> PydanticResponse[SimpleBody[ResponseData]]:
+    if request_body.data.enabled is not None:
+        store.set_is_enabled(request_body.data.enabled)
+    return await _get_current_response(store)
+
+
+@PydanticResponse.wrap_route(
+    router.delete,
+    path=_PATH,
+    summary="Reset error recovery settings to defaults",
+)
+async def delete_error_recovery_settings(  # noqa: D103
+    store: Annotated[
+        ErrorRecoverySettingStore, fastapi.Depends(get_error_recovery_setting_store)
+    ],
+) -> PydanticResponse[SimpleBody[ResponseData]]:
+    store.set_is_enabled(None)
+    return await _get_current_response(store)
+
+
+async def _get_current_response(
+    store: ErrorRecoverySettingStore,
+) -> PydanticResponse[SimpleBody[ResponseData]]:
+    is_enabled = store.get_is_enabled()
+    if is_enabled is None:
+        # todo(mm, 2024-09-30): This defaulting will probably need to move down a layer
+        # when we connect this setting to `POST /runs`.
+        is_enabled = True
+    return await PydanticResponse.create(
+        SimpleBody.construct(data=ResponseData.construct(enabled=is_enabled))
+    )

robot-server/robot_server/runs/error_recovery_setting_store.py → robot-server/robot_server/error_recovery/settings/store.py

@@ -1,8 +1,12 @@
 # noqa: D100
 
 
+from typing import Annotated
+
+import fastapi
 import sqlalchemy
 
+from robot_server.persistence.fastapi_dependencies import get_sql_engine
 from robot_server.persistence.tables import boolean_setting_table, BooleanSettingKey
 
 
@@ -12,7 +16,7 @@ class ErrorRecoverySettingStore:
     def __init__(self, sql_engine: sqlalchemy.engine.Engine) -> None:
         self._sql_engine = sql_engine
 
-    def get_is_disabled(self) -> bool | None:
+    def get_is_enabled(self) -> bool | None:
         """Get the value of the "error recovery enabled" setting.
 
         `None` is the default, i.e. it's never been explicitly set one way or the other.
@@ -21,11 +25,11 @@ def get_is_disabled(self) -> bool | None:
             return transaction.execute(
                 sqlalchemy.select(boolean_setting_table.c.value).where(
                     boolean_setting_table.c.key
-                    == BooleanSettingKey.DISABLE_ERROR_RECOVERY
+                    == BooleanSettingKey.ENABLE_ERROR_RECOVERY
                 )
             ).scalar_one_or_none()
 
-    def set_is_disabled(self, is_disabled: bool | None) -> None:
+    def set_is_enabled(self, is_enabled: bool | None) -> None:
         """Set the value of the "error recovery enabled" setting.
 
         `None` means revert to the default.
@@ -34,13 +38,23 @@ def set_is_disabled(self, is_disabled: bool | None) -> None:
             transaction.execute(
                 sqlalchemy.delete(boolean_setting_table).where(
                     boolean_setting_table.c.key
-                    == BooleanSettingKey.DISABLE_ERROR_RECOVERY
+                    == BooleanSettingKey.ENABLE_ERROR_RECOVERY
                 )
             )
-            if is_disabled is not None:
+            if is_enabled is not None:
                 transaction.execute(
                     sqlalchemy.insert(boolean_setting_table).values(
-                        key=BooleanSettingKey.DISABLE_ERROR_RECOVERY,
-                        value=is_disabled,
+                        key=BooleanSettingKey.ENABLE_ERROR_RECOVERY,
+                        value=is_enabled,
                     )
                 )
+
+
+async def get_error_recovery_setting_store(
+    sql_engine: Annotated[sqlalchemy.engine.Engine, fastapi.Depends(get_sql_engine)]
+) -> ErrorRecoverySettingStore:
+    """A FastAPI dependency to return the server's ErrorRecoverySettingStore."""
+    # Since the store itself has no state, and no asyncio.Locks or anything,
+    # instances are fungible and disposable, and we can use a fresh one for each
+    # request instead of having to maintain a global singleton.
+    return ErrorRecoverySettingStore(sql_engine)

robot-server/robot_server/persistence/tables/schema_7.py

@@ -269,7 +269,7 @@ class ProtocolKindSQLEnum(enum.Enum):
 class BooleanSettingKey(enum.Enum):
     """Keys for boolean settings."""
 
-    DISABLE_ERROR_RECOVERY = "disable_error_recovery"
+    ENABLE_ERROR_RECOVERY = "enable_error_recovery"
 
 
 boolean_setting_table = sqlalchemy.Table(

robot-server/robot_server/router.py

@@ -8,6 +8,7 @@
 from .client_data.router import router as client_data_router
 from .commands.router import commands_router
 from .deck_configuration.router import router as deck_configuration_router
+from .error_recovery.settings.router import router as error_recovery_settings_router
 from .health.router import health_router
 from .instruments.router import instruments_router
 from .maintenance_runs.router import maintenance_runs_router
@@ -89,6 +90,12 @@
     dependencies=[Depends(check_version_header)],
 )
 
+router.include_router(
+    router=error_recovery_settings_router,
+    tags=["Error Recovery Settings"],
+    dependencies=[Depends(check_version_header)],
+)
+
 router.include_router(
     router=modules_router,
     tags=["Attached Modules"],

robot-server/robot_server/service/legacy/routers/settings.py

@@ -65,7 +65,7 @@
 
 # TODO: (ba, 2024-04-11): We should have a proper IPC mechanism to talk between
 # the servers instead of one off endpoint calls like these.
-async def set_oem_mode_request(enable):
+async def _set_oem_mode_request(enable: bool) -> int:
     """PUT request to set the OEM Mode for the system server."""
     async with aiohttp.ClientSession() as session:
         async with session.put(
@@ -94,7 +94,14 @@ async def post_settings(
     try:
         # send request to system server if this is the enableOEMMode setting
         if update.id == "enableOEMMode" and robot_type == RobotTypeEnum.FLEX:
-            resp = await set_oem_mode_request(update.value)
+            resp = await _set_oem_mode_request(
+                # Unlike opentrons.advanced_settings, system-server cannot store
+                # `None`/`null` to restore to default. Storing `False` instead is close
+                # enough.
+                update.value
+                if update.value is not None
+                else False
+            )
             if resp != 200:
                 # TODO: raise correct error here
                 raise Exception(f"Something went wrong setting OEM Mode. err: {resp}")

robot-server/tests/error_recovery/__init__.py

@@ -0,0 +1 @@
+# noqa: D104

robot-server/tests/error_recovery/settings/__init__.py

@@ -0,0 +1 @@
+# noqa: D104

robot-server/tests/error_recovery/settings/test_store.py

@@ -0,0 +1,29 @@
+"""Tests for the error recovery settings store."""
+
+
+from robot_server.error_recovery.settings.store import ErrorRecoverySettingStore
+
+import pytest
+import sqlalchemy
+
+
+@pytest.fixture
+def subject(
+    sql_engine: sqlalchemy.engine.Engine,
+) -> ErrorRecoverySettingStore:
+    """Return a test subject."""
+    return ErrorRecoverySettingStore(sql_engine=sql_engine)
+
+
+def test_error_recovery_setting_store(subject: ErrorRecoverySettingStore) -> None:
+    """Test `ErrorRecoverySettingStore`."""
+    assert subject.get_is_enabled() is None
+
+    subject.set_is_enabled(is_enabled=False)
+    assert subject.get_is_enabled() is False
+
+    subject.set_is_enabled(is_enabled=True)
+    assert subject.get_is_enabled() is True
+
+    subject.set_is_enabled(is_enabled=None)
+    assert subject.get_is_enabled() is None

robot-server/tests/integration/conftest.py

@@ -131,6 +131,7 @@ async def _clean_server_state_async() -> None:
             await _delete_all_sessions(robot_client)
 
             await _reset_deck_configuration(robot_client)
+            await _reset_error_recovery_settings(robot_client)
 
             await _delete_client_data(robot_client)
 
@@ -174,3 +175,7 @@ async def _delete_client_data(robot_client: RobotClient) -> None:
 
 async def _reset_deck_configuration(robot_client: RobotClient) -> None:
     await robot_client.post_setting_reset_options({"deckConfiguration": True})
+
+
+async def _reset_error_recovery_settings(robot_client: RobotClient) -> None:
+    await robot_client.delete_error_recovery_settings()

robot-server/tests/integration/http_api/error_recovery/test_settings.tavern.yaml

@@ -0,0 +1,88 @@
+test_name: Test the /errorRecovery/settings endpoints
+
+marks:
+  - usefixtures:
+      - ot3_server_base_url
+
+stages:
+  - name: Get default settings
+    request:
+      method: GET
+      url: '{ot3_server_base_url}/errorRecovery/settings'
+    response:
+      status_code: 200
+      json: &initial_get_settings_response
+        data:
+          enabled: true
+
+  - name: Change settings
+    request:
+      method: PATCH
+      url: '{ot3_server_base_url}/errorRecovery/settings'
+      json:
+        data:
+          enabled: false
+    response:
+      status_code: 200
+      json: &patch_settings_response
+        data:
+          enabled: false
+
+  - name: Get the settings again and make sure they're still changed
+    request:
+      method: GET
+      url: '{ot3_server_base_url}/errorRecovery/settings'
+    response:
+      status_code: 200
+      json: *patch_settings_response
+
+  - name: Restore defaults
+    request:
+      method: DELETE
+      url: '{ot3_server_base_url}/errorRecovery/settings'
+    response:
+      status_code: 200
+      json: *initial_get_settings_response
+
+  - name: Get the settings again and make sure they're still the defaults
+    request:
+      method: GET
+      url: '{ot3_server_base_url}/errorRecovery/settings'
+    response:
+      status_code: 200
+      json: *initial_get_settings_response
+
+---
+test_name: Test no-op PATCH requests
+
+marks:
+  - usefixtures:
+      - ot3_server_base_url
+  - parametrize:
+      key: enabled
+      vals:
+        - true
+        - false
+
+stages:
+  - name: Set initial settings
+    request:
+      method: PATCH
+      url: '{ot3_server_base_url}/errorRecovery/settings'
+      json:
+        data:
+          enabled: '{enabled}'
+    response:
+      save:
+        json:
+          initial_response: data
+
+  - name: Send a no-op PATCH and make sure it doesn't change anything
+    request:
+      method: PATCH
+      url: '{ot3_server_base_url}/errorRecovery/settings'
+      json:
+        data: {}
+    response:
+      json:
+        data: !force_original_structure '{initial_response}'

robot-server/tests/integration/robot_client.py

@@ -377,6 +377,13 @@ async def delete_all_client_data(self) -> Response:
         response.raise_for_status()
         return response
 
+    async def delete_error_recovery_settings(self) -> Response:
+        response = await self.httpx_client.delete(
+            url=f"{self.base_url}/errorRecovery/settings"
+        )
+        response.raise_for_status()
+        return response
+
 
 async def poll_until_run_completes(
     robot_client: RobotClient, run_id: str, poll_interval: float = _RUN_POLL_INTERVAL