Surface umount stderr in UnmountError; user-friendly error messages in backup/close

projects/butter-backup/src/butter_backup/cli.py

@@ -153,6 +153,13 @@ def _open_device(
         typer.echo(f"Speichermedium {cfg.Name} wurde in {mount_dir} geöffnet.")
 
 
+def _unmount_errmsg(cfg: cp.DeviceConfiguration, e: sdm.UnmountError) -> str:
+    if e.stderr is None:
+        return f"Speichermedium {cfg.Name} konnte nicht ausgehängt werden. Es ist keine Fehlermeldung verfügbar."
+    stderr = e.stderr.decode("utf-8", errors="replace")
+    return f"Aushängen des Speichermediums {cfg.Name} ist fehlgeschlagen. Die Fehlermeldung ist: {stderr}"
+
+
 @app.command("open")
 def cli_open(
     config: t.Annotated[Path | None, CONFIG_OPTION] = None,
@@ -221,7 +228,13 @@ def close(
                 )
                 continue
             sh.refresh_sudo(parsed_config.SudoPassCmd)
-            sdm.unmount_device(map_name)
+            try:
+                sdm.unmount_device(map_name)
+            except sdm.UnmountError as e:
+                typer.echo(_unmount_errmsg(cfg, e), err=True)
+                # The device is still mounted, so closing the decrypted device
+                # would fail. Leave it for the user to handle manually.
+                continue
             sdm.close_decrypted_device(map_name)
 
 
@@ -250,6 +263,7 @@ def backup(
     """
     setup_logging(verbose)
     parsed_config = _read_configuration(config)
+    had_unmount_error = False
     for cfg in parsed_config.DeviceConfigurations:
         if _skip_device(
             cfg,
@@ -265,17 +279,23 @@ def backup(
         sh.refresh_sudo(parsed_config.SudoPassCmd)
         open_dir = parsed_config.OpenDirectory
         dest = open_dir / cfg.Name if open_dir is not None else None
-        with (
-            sdm.decrypted_device(cfg.device(), cfg.DevicePassCmd) as decrypted,
-            sdm.mounted_device(
-                decrypted, dest, compression=cfg.compression()
-            ) as mount_dir,
-        ):
-            backend.do_backup(mount_dir, parsed_config.SudoPassCmd)
-            # A backup could take so long that the sudo session expires. In this
-            # case the user would have to enter the password again to unmount and
-            # close the device. To prevent this, the sudo session is refreshed.
-            sh.refresh_sudo(parsed_config.SudoPassCmd)
+        try:
+            with (
+                sdm.decrypted_device(cfg.device(), cfg.DevicePassCmd) as decrypted,
+                sdm.mounted_device(
+                    decrypted, dest, compression=cfg.compression()
+                ) as mount_dir,
+            ):
+                backend.do_backup(mount_dir, parsed_config.SudoPassCmd)
+                # A backup could take so long that the sudo session expires. In this
+                # case the user would have to enter the password again to unmount and
+                # close the device. To prevent this, the sudo session is refreshed.
+                sh.refresh_sudo(parsed_config.SudoPassCmd)
+        except sdm.UnmountError as e:
+            typer.echo(_unmount_errmsg(cfg, e), err=True)
+            had_unmount_error = True
+    if had_unmount_error:
+        raise typer.Exit(1)
 
 
 @app.command()

projects/butter-backup/tests/cli/test_cli_commands.py

@@ -1,23 +1,25 @@
 import datetime as dt
+import types
 import re
 import time
+from contextlib import contextmanager
 from pathlib import Path
 from tempfile import NamedTemporaryFile
 from uuid import UUID
 
 import pytest
 import shell_interface as sh
 import storage_device_managers as sdm
 import typer
 from loguru import logger
 from typer.testing import CliRunner
 
 from butter_backup import cli
 from butter_backup import config_parser as cp
 from butter_backup.cli import app
 from tests import complement_configuration, get_random_filename
 
 from . import in_docker_container, prepare_tmp_path
 
 
 def wait_until_gone(p: Path, timeout: dt.timedelta = dt.timedelta(seconds=3)) -> None:
@@ -477,7 +479,7 @@
     # This test "successfully" provoked the buggy behaviour before the bug was fixed.
     mocker.patch(
         "storage_device_managers.unmount_device",
-        side_effect=sdm.UnmountError("Mocked unmount error"),
+        side_effect=sdm.UnmountError("Mocked unmount error", b"Mocked stderr"),
     )
 
     config = complement_configuration(encrypted_device, tmp_path)
@@ -503,3 +505,86 @@
     assert result.exit_code == 0
     assert mount_of_device.exists()  # Target directory should be kept after closing.
     assert sdm.is_mounted(mount_of_device) is False
+
+
+def test_close_prints_unmount_error_and_keeps_device_open(
+    runner: CliRunner, mocker
+) -> None:
+    map_name = Path("/dev/mapper/mock-device")
+    cfg = types.SimpleNamespace(
+        Name="mock-device",
+        device=lambda: Path("/tmp/mock-device"),
+        map_name=lambda: map_name,
+    )
+    parsed = types.SimpleNamespace(
+        DeviceConfigurations=[cfg], SudoPassCmd=None, OpenDirectory=None
+    )
+    mocker.patch.object(cli, "_read_configuration", return_value=parsed)
+    mocker.patch.object(Path, "exists", return_value=True)
+    mocker.patch.object(
+        cli.sdm,
+        "get_mounted_devices",
+        return_value={str(map_name): {Path("/mnt/mock"): frozenset()}},
+    )
+    mocker.patch.object(cli.sh, "refresh_sudo")
+    mocker.patch.object(
+        cli.sdm,
+        "unmount_device",
+        side_effect=sdm.UnmountError(["sudo", "umount", map_name], b"Mocked stderr\n"),
+    )
+    close_mock = mocker.patch.object(cli.sdm, "close_decrypted_device")
+
+    result = runner.invoke(app, ["close"])
+
+    assert result.exit_code == 0
+    assert (
+        "Aushängen des Speichermediums mock-device ist fehlgeschlagen." in result.stderr
+    )
+    assert "Mocked stderr" in result.stderr
+    close_mock.assert_not_called()
+
+
+def test_backup_keeps_unmount_error_as_primary_failure(
+    runner: CliRunner, mocker
+) -> None:
+    @contextmanager
+    def _failing_mounted_device(*_args, **_kwargs):
+        try:
+            yield Path("/mnt/mock")
+        finally:
+            raise sdm.UnmountError(
+                ["sudo", "umount", Path("/dev/mapper/mock-device")], b"Mocked stderr\n"
+            )
+
+    cfg = types.SimpleNamespace(
+        Name="mock-device",
+        DevicePassCmd="echo pw",
+        device=lambda: Path("/dev/disk/by-uuid/mock-device"),
+        compression=lambda: None,
+    )
+    parsed = types.SimpleNamespace(
+        DeviceConfigurations=[cfg], SudoPassCmd=None, OpenDirectory=None
+    )
+    mocker.patch.object(cli, "_read_configuration", return_value=parsed)
+    mocker.patch.object(cli, "_skip_device", return_value=False)
+    backend = mocker.Mock()
+    mocker.patch.object(cli.bb.BackupBackend, "from_config", return_value=backend)
+    mocker.patch.object(cli.sh, "refresh_sudo")
+    mocker.patch.object(
+        cli.sdm, "open_encrypted_device", return_value=Path("/dev/mapper/mock-device")
+    )
+    close_cmd = ["sudo", "cryptsetup", "close", "mock-device"]
+    mocker.patch.object(
+        cli.sdm,
+        "close_decrypted_device",
+        side_effect=sh.ShellInterfaceError(close_cmd, None),
+    )
+    mocker.patch.object(cli.sdm, "mounted_device", _failing_mounted_device)
+
+    result = runner.invoke(app, ["backup"])
+
+    assert result.exit_code == 1
+    assert (
+        "Aushängen des Speichermediums mock-device ist fehlgeschlagen." in result.stderr
+    )
+    assert "Mocked stderr" in result.stderr

projects/shell-interface/src/shell_interface/run_cmd.py

@@ -1,6 +1,7 @@
 import os
 import subprocess
 from collections import abc
+from dataclasses import dataclass
 from pathlib import Path
 
 StrPathList = abc.Sequence[str | Path]
@@ -10,8 +11,13 @@ class PassCmdError(RuntimeError):
     pass
 
 
+@dataclass(frozen=False)
 class ShellInterfaceError(RuntimeError):
-    pass
+    # Setting frozen=True would prevent updating `__traceback__` in the except blocks,
+    # causing error handling to crash itself. Therefore frozen=False is used, contrary
+    # to the usual practice of making dataclasses frozen.
+    command: StrPathList
+    stderr: bytes | None
 
 
 def run_cmd(
@@ -57,13 +63,12 @@ def run_cmd(
     try:
         result = subprocess.run(cmd, capture_output=capture_output, check=True, env=env)
     except subprocess.CalledProcessError as e:
-        errmsg = f"Shell-Befehl `{cmd}` ist fehlgeschlagen."
-        raise ShellInterfaceError(errmsg) from e
+        raise ShellInterfaceError(cmd, e.stderr) from e
     return result
 
 
 def pipe_pass_cmd_to_real_cmd(
-    pass_cmd: str, command: StrPathList, *, capture_output: bool = False
+    pass_cmd: str, cmd: StrPathList, *, capture_output: bool = False
 ) -> subprocess.CompletedProcess[bytes]:
     """
     Pipe result of first command to second command
@@ -80,7 +85,7 @@ def pipe_pass_cmd_to_real_cmd(
     -----------
     pass_cmd
         command to run in shell and whose output is piped to the second command
-    command
+    cmd
         command to run in shell and whose input is piped from the first command
     capture_output
         whether to capture the output of the real command; if `True`, the output is
@@ -108,9 +113,8 @@ def pipe_pass_cmd_to_real_cmd(
         raise PassCmdError(errmsg) from e
     try:
         completed_process = subprocess.run(
-            command, input=pwd_proc.stdout, check=True, capture_output=capture_output
+            cmd, input=pwd_proc.stdout, check=True, capture_output=capture_output
         )
     except subprocess.CalledProcessError as e:
-        errmsg = f"Shell-Befehl `{command}` ist fehlgeschlagen."
-        raise ShellInterfaceError(errmsg) from e
+        raise ShellInterfaceError(cmd, e.stderr) from e
     return completed_process

projects/shell-interface/tests/test_run_cmd.py

@@ -22,6 +22,19 @@ def test_run_cmd_fails() -> None:
         sh.run_cmd(cmd=["false"])
 
 
+def test_shellinterfaceerror_can_be_raised_and_reraised_with_traceback() -> None:
+    error = sh.ShellInterfaceError(["false"], b"stderr")
+
+    with pytest.raises(sh.ShellInterfaceError) as exc_info:
+        raise error
+    assert exc_info.value is error
+    assert exc_info.value.__traceback__ is not None
+
+    with pytest.raises(sh.ShellInterfaceError) as exc_info:
+        raise error
+    assert exc_info.value.__traceback__ is not None
+
+
 @given(environment=st.dictionaries(environment_variable_names, echoable_text))
 def test_run_cmd_forwards_env(environment: dict[str, str]) -> None:
     proc = sh.run_cmd(cmd=["env"], capture_output=True, env=environment)

projects/storage-device-managers/src/storage_device_managers/__init__.py

@@ -49,7 +49,7 @@
     pass
 
 
-class UnmountError(RuntimeError):
+class UnmountError(sh.ShellInterfaceError):
     pass
 
 
@@ -146,7 +146,15 @@
     decrypted = open_encrypted_device(device, pass_cmd)
     try:
         yield decrypted
-    finally:
+    except Exception:
+        try:
+            close_decrypted_device(decrypted)
+        except Exception:
+            # If cleanup follows another failure (e.g. an unmount error), keep the
+            # original exception as root cause.
+            pass
+        raise
+    else:
         close_decrypted_device(decrypted)
 
 
@@ -402,9 +410,9 @@
     sync_device(device)
     cmd: sh.StrPathList = ["sudo", "umount", device]
     try:
-        sh.run_cmd(cmd=cmd)
+        sh.run_cmd(cmd=cmd, capture_output=True)
     except sh.ShellInterfaceError as e:
-        raise UnmountError from e
+        raise UnmountError(e.command, e.stderr) from e
 
 
 def open_encrypted_device(device: Path, pass_cmd: str) -> Path:
@@ -510,7 +518,7 @@
         str(new_uuid),
         device,
     ]
-    sh.pipe_pass_cmd_to_real_cmd(pass_cmd=password_cmd, command=format_cmd)
+    sh.pipe_pass_cmd_to_real_cmd(pass_cmd=password_cmd, cmd=format_cmd)
     return new_uuid
 
 

projects/storage-device-managers/tests/test_device_manager.py

@@ -76,6 +76,42 @@ def test_decrypted_device_closes_in_case_of_exception(encrypted_device) -> None:
     assert not dd.exists()
 
 
+def test_decrypted_device_preserves_original_exception_when_close_fails(mocker) -> None:
+    encrypted = Path("/dev/sdz1")
+    decrypted = Path("/dev/mapper/mock-decrypted")
+    close_cmd = ["sudo", "cryptsetup", "close", decrypted.name]
+    mocker.patch(
+        "storage_device_managers.open_encrypted_device", return_value=decrypted
+    )
+    mocker.patch(
+        "storage_device_managers.close_decrypted_device",
+        side_effect=sh.ShellInterfaceError(close_cmd, None),
+    )
+    with (
+        pytest.raises(MyCustomTestException),
+        sdm.decrypted_device(encrypted, "echo pw"),
+    ):
+        raise MyCustomTestException
+
+
+def test_decrypted_device_raises_close_error_without_prior_exception(mocker) -> None:
+    encrypted = Path("/dev/sdz1")
+    decrypted = Path("/dev/mapper/mock-decrypted")
+    close_cmd = ["sudo", "cryptsetup", "close", decrypted.name]
+    mocker.patch(
+        "storage_device_managers.open_encrypted_device", return_value=decrypted
+    )
+    mocker.patch(
+        "storage_device_managers.close_decrypted_device",
+        side_effect=sh.ShellInterfaceError(close_cmd, None),
+    )
+    with (
+        pytest.raises(sh.ShellInterfaceError),
+        sdm.decrypted_device(encrypted, "echo pw"),
+    ):
+        pass
+
+
 def test_decrypted_device_can_use_home_for_passcmd(encrypted_device) -> None:
     # Regression Test
     # Test if `decrypted_device` can use a program that is located in PATH. For

projects/storage-device-managers/tests/test_mount_unmount.py

@@ -143,13 +143,26 @@ def test_unmount_device_raises_unmounterror(tmp_path: Path) -> None:
         sdm.unmount_device(mountpoint)
 
 
+def test_unmount_device_preserves_shell_error_details(tmp_path: Path) -> None:
+    mountpoint = tmp_path
+    with pytest.raises(sdm.UnmountError) as exc_info:
+        sdm.unmount_device(mountpoint)
+
+    exc = exc_info.value
+    assert isinstance(exc, sh.ShellInterfaceError)
+    assert exc.command[:2] == ["sudo", "umount"]
+    assert mountpoint in exc.command
+    assert exc.stderr is not None
+    assert exc.stderr.strip()
+
+
 def test_mounted_device_does_not_delete_content_on_umount_error(
     device_with_fs, compression_kwargs: CompressionKwargsT, mocker
 ) -> None:
     device, _ = device_with_fs
     mocker.patch(
         "storage_device_managers.unmount_device",
-        side_effect=sdm.UnmountError("Mocked unmount error"),
+        side_effect=sdm.UnmountError("Mocked unmount error", b"Mocked stderr"),
     )
     user = sh.get_user()
     sentinel_text = "This file should not be deleted."