diff --git a/projects/butter-backup/src/butter_backup/cli.py b/projects/butter-backup/src/butter_backup/cli.py index 9511959..64da253 100644 --- a/projects/butter-backup/src/butter_backup/cli.py +++ b/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() diff --git a/projects/butter-backup/tests/cli/test_cli_commands.py b/projects/butter-backup/tests/cli/test_cli_commands.py index 43724b2..ddd3078 100644 --- a/projects/butter-backup/tests/cli/test_cli_commands.py +++ b/projects/butter-backup/tests/cli/test_cli_commands.py @@ -1,6 +1,8 @@ import datetime as dt import re import time +import types +from contextlib import contextmanager from pathlib import Path from tempfile import NamedTemporaryFile from uuid import UUID @@ -477,7 +479,7 @@ def test_unmount_error_does_not_cause_content_deletion( # 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 @@ def test_unmount_error_does_not_cause_content_deletion( 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 diff --git a/projects/shell-interface/src/shell_interface/run_cmd.py b/projects/shell-interface/src/shell_interface/run_cmd.py index e2cd956..fd0964c 100644 --- a/projects/shell-interface/src/shell_interface/run_cmd.py +++ b/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 diff --git a/projects/storage-device-managers/src/storage_device_managers/__init__.py b/projects/storage-device-managers/src/storage_device_managers/__init__.py index 3ddf300..0555515 100644 --- a/projects/storage-device-managers/src/storage_device_managers/__init__.py +++ b/projects/storage-device-managers/src/storage_device_managers/__init__.py @@ -49,7 +49,7 @@ class InvalidDecryptedDevice(ValueError): pass -class UnmountError(RuntimeError): +class UnmountError(sh.ShellInterfaceError): pass @@ -146,7 +146,17 @@ def decrypted_device(device: Path, pass_cmd: str) -> Iterator[Path]: decrypted = open_encrypted_device(device, pass_cmd) try: yield decrypted - finally: + except BaseException as exc: + # Closing the encrypted device is expected to fail too after an UnmountError; + # suppress it so the original exception propagates. + try: + close_decrypted_device(decrypted) + except Exception as close_exc: + raise BaseExceptionGroup( + "closing the encrypted device failed while handling another error", + [exc, close_exc], + ) from None + else: close_decrypted_device(decrypted) @@ -402,9 +412,9 @@ def unmount_device(device: Path) -> None: 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 +520,7 @@ def encrypt_device(device: Path, password_cmd: str) -> UUID: 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 diff --git a/projects/storage-device-managers/tests/test_device_manager.py b/projects/storage-device-managers/tests/test_device_manager.py index 12cb51a..680993f 100644 --- a/projects/storage-device-managers/tests/test_device_manager.py +++ b/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( + encrypted_device, tmp_path +) -> None: + encrypted_device, pass_cmd = encrypted_device + with ( + pytest.raises(BaseExceptionGroup) as exc_info, + sdm.decrypted_device(encrypted_device, pass_cmd) as decrypted, + sdm.mounted_device(decrypted), + ): + close_cmd = ["sudo", "cryptsetup", "close", decrypted.name] + raise MyCustomTestException + close_error = sh.ShellInterfaceError(close_cmd, None) + + assert len(exc_info.value.exceptions) == 2 # noqa: PLR2004 + assert isinstance(exc_info.value.exceptions[0], MyCustomTestException) + assert exc_info.value.exceptions[1] is close_error + + +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 diff --git a/projects/storage-device-managers/tests/test_mount_unmount.py b/projects/storage-device-managers/tests/test_mount_unmount.py index 3c39955..eb4c689 100644 --- a/projects/storage-device-managers/tests/test_mount_unmount.py +++ b/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."