🐛 Fix: dynamic-sidecar self-triggered upload loop when data mounting is not enabled (#9053)

Co-authored-by: Andrei Neagu <neagu@itis.swiss>

Author: GitHK

services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/modules/file_notification_subscriber.py

@@ -31,28 +31,33 @@ def _resolve_local_path_from_storage_id(
     mounted_volumes: MountedVolumes,
     storage_path: StorageFileID,
 ) -> Path | None:
-    """Maps a StorageFileID to a local disk path within mounted volumes."""
+    """Maps a StorageFileID to a local disk path within mounted volumes.
+
+    Only state volumes are resolved. Inputs and outputs are intentionally ignored:
+    - outputs are produced by this node; reacting to a notification about them would
+      rewrite the file under the outputs watcher and trigger an upload loop
+      (see also the rclone path which only acts on explicitly registered mounts).
+    - inputs are handled via the rclone mount path when applicable.
+    """
     path_parts = storage_path.split("/")
     if len(path_parts) < _MIN_STORAGE_PATH_PARTS:
         return None
 
     volume_name = path_parts[2]
     relative_parts = path_parts[_MIN_STORAGE_PATH_PARTS:]
 
-    local_base: Path | None = None
-    if volume_name == mounted_volumes.inputs_path.name:
-        local_base = mounted_volumes.disk_inputs_path
-    elif volume_name == mounted_volumes.outputs_path.name:
-        local_base = mounted_volumes.disk_outputs_path
-    else:
-        for state_path, disk_state_path in zip(
-            mounted_volumes.state_paths,
-            mounted_volumes.disk_state_paths_iter(),
-            strict=True,
-        ):
-            if volume_name == state_path.name:
-                local_base = disk_state_path
-                break
+    local_base: Path | None = next(
+        (
+            disk
+            for state_path, disk in zip(
+                mounted_volumes.state_paths,
+                mounted_volumes.disk_state_paths_iter(),
+                strict=True,
+            )
+            if volume_name == state_path.name
+        ),
+        None,
+    )
 
     if local_base is None:
         return None

services/dynamic-sidecar/tests/unit/test_modules_file_notification_subscriber.py

@@ -1,5 +1,6 @@
 # pylint:disable=redefined-outer-name
 
+from pathlib import Path
 from unittest.mock import AsyncMock
 
 import pytest
@@ -10,11 +11,14 @@
     FileNotificationEventType,
     FileNotificationMessage,
 )
+from models_library.services_types import ServiceRunID
 from models_library.users import UserID
 from pytest_mock import MockerFixture
 from simcore_service_dynamic_sidecar.modules.file_notification_subscriber import (
     _handle_file_notification,
+    _resolve_local_path_from_storage_id,
 )
+from simcore_service_dynamic_sidecar.modules.mounted_fs import MountedVolumes
 
 
 @pytest.fixture()
@@ -94,3 +98,105 @@ async def test_handle_file_notification_with_optional_ids(
     mock_notify_path_change.assert_awaited_once_with(
         app=None, event_type=FileNotificationEventType.FILE_UPLOADED, path=file_id, recursive=False
     )
+
+
+_INPUTS_PATH = Path("/inputs")
+_OUTPUTS_PATH = Path("/outputs")
+_STATE_PATH_A = Path("/workspace/state-a")
+_STATE_PATH_B = Path("/workspace/state-b")
+
+
+@pytest.fixture()
+def mounted_volumes(tmp_path: Path, faker: Faker, node_id: NodeID) -> MountedVolumes:
+    return MountedVolumes(
+        service_run_id=ServiceRunID(faker.uuid4()),
+        node_id=node_id,
+        inputs_path=_INPUTS_PATH,
+        outputs_path=_OUTPUTS_PATH,
+        user_preferences_path=None,
+        state_paths=[_STATE_PATH_A, _STATE_PATH_B],
+        state_exclude=set(),
+        compose_namespace="test-namespace",
+        dy_volumes=tmp_path,
+    )
+
+
+def _make_storage_id(project_id: ProjectID, node_id: NodeID, volume_name: str, *parts: str) -> str:
+    return "/".join((f"{project_id}", f"{node_id}", volume_name, *parts))
+
+
+@pytest.mark.parametrize(
+    "state_path_index, sub_parts",
+    [
+        pytest.param(0, ("sub", "file.bin"), id="first-state-volume-with-subdir"),
+        pytest.param(1, ("file.bin",), id="second-state-volume-flat"),
+    ],
+)
+def test_resolve_local_path_for_state_volume_resolves_correctly(
+    mounted_volumes: MountedVolumes,
+    project_id: ProjectID,
+    node_id: NodeID,
+    state_path_index: int,
+    sub_parts: tuple[str, ...],
+):
+    state_paths = [_STATE_PATH_A, _STATE_PATH_B]
+    state_disk_paths = list(mounted_volumes.disk_state_paths_iter())
+    storage_id = _make_storage_id(project_id, node_id, state_paths[state_path_index].name, *sub_parts)
+
+    resolved = _resolve_local_path_from_storage_id(mounted_volumes, storage_id)
+
+    assert resolved is not None
+    expected = state_disk_paths[state_path_index].joinpath(*sub_parts).resolve()
+    assert resolved == expected
+
+
+@pytest.mark.parametrize(
+    "volume_path",
+    [
+        pytest.param(_INPUTS_PATH, id="inputs"),
+        pytest.param(_OUTPUTS_PATH, id="outputs"),
+    ],
+)
+def test_resolve_local_path_for_inputs_and_outputs_returns_none(
+    mounted_volumes: MountedVolumes,
+    project_id: ProjectID,
+    node_id: NodeID,
+    volume_path: Path,
+):
+    """Regression: inputs/outputs volumes MUST NOT be resolved by the bind-mount fallback.
+
+    Resolving outputs would cause the dynamic-sidecar to download (via the file-notification
+    fallback) the file it just uploaded, into the very directory the outputs watcher is
+    observing — re-triggering the upload and producing an infinite loop.
+    """
+    storage_id = _make_storage_id(project_id, node_id, volume_path.name, "some-file.txt")
+
+    assert _resolve_local_path_from_storage_id(mounted_volumes, storage_id) is None
+
+
+@pytest.mark.parametrize(
+    "storage_id_template",
+    [
+        pytest.param(
+            "{project_id}/{node_id}/not-a-volume/file.bin",
+            id="unknown-volume",
+        ),
+        pytest.param(
+            "only/two",
+            id="too-few-parts",
+        ),
+        pytest.param(
+            f"{{project_id}}/{{node_id}}/{_STATE_PATH_A.name}/../../etc/passwd",
+            id="path-traversal",
+        ),
+    ],
+)
+def test_resolve_local_path_returns_none_on_invalid_storage_ids(
+    mounted_volumes: MountedVolumes,
+    project_id: ProjectID,
+    node_id: NodeID,
+    storage_id_template: str,
+):
+    storage_id = storage_id_template.format(project_id=project_id, node_id=node_id)
+
+    assert _resolve_local_path_from_storage_id(mounted_volumes, storage_id) is None