Namespace isic-partial files by pid

This prevents the tool from trying to delete partial files that are
created by other (possibly concurrent) invocations of isic-cli which has
been seen in the wild. It doesn't imply support for concurrent
executions of isic-cli with the same output directory, but it at least
prevents it from trying to tamper with files outside of its custody.

Author: danlamanna

isic_cli/cli/image.py

@@ -27,7 +27,7 @@
 
 
 def cleanup_partially_downloaded_files(directory: Path) -> None:
-    for p in directory.glob("**/.isic-partial.*"):
+    for p in directory.glob(f"**/.isic-partial.{os.getpid()}.*"):
         # missing_ok=True because it's possible that another thread moved the temporary file to
         # its final destination after listing it but before unlinking.
         p.unlink(missing_ok=True)

isic_cli/io/http.py

@@ -1,6 +1,7 @@
 from __future__ import annotations
 
 import logging
+import os
 import shutil
 from tempfile import NamedTemporaryFile
 from typing import TYPE_CHECKING
@@ -140,7 +141,9 @@ def download_image(image: dict, to: Path, progress, task) -> None:
         r.raise_for_status()
 
         temp_file_name = None
-        with NamedTemporaryFile(dir=to, prefix=".isic-partial.", delete=False) as outfile:
+        with NamedTemporaryFile(
+            dir=to, prefix=f".isic-partial.{os.getpid()}.", delete=False
+        ) as outfile:
             temp_file_name = outfile.name
             for chunk in r.iter_content(1024 * 1024 * 5):
                 outfile.write(chunk)

tests/test_cli_image.py

@@ -1,10 +1,12 @@
 from __future__ import annotations
 
+import os
 from pathlib import Path
-from unittest.mock import MagicMock
 
 import pytest
 
+from isic_cli.cli.image import cleanup_partially_downloaded_files
+
 
 @pytest.fixture()
 def outdir():
@@ -58,22 +60,16 @@ def test_image_download_metadata_newlines(cli_run, outdir):
 
 
 @pytest.mark.usefixtures("_isolated_filesystem", "_mock_images")
-def test_image_download_cleanup_on_interrupt(mocker, outdir):
-    from click.testing import CliRunner  # noqa: I001
-    from isic_cli.cli import cli
-
-    partial_file = Path(outdir) / ".isic-partial.ISIC_0000000.jpg"
+def test_image_download_cleanup(cli_run, outdir):
+    partial_file = Path(outdir) / f".isic-partial.{os.getpid()}.ISIC_0000000.jpg"
     partial_file.parent.mkdir(parents=True)
+    partial_file.touch()
 
-    runner = CliRunner()
-
-    cleanup = MagicMock(side_effect=KeyboardInterrupt)
-    mocker.patch("isic_cli.cli.image.get_num_images", cleanup)
-
-    result = runner.invoke(cli, ["image", "download", outdir], standalone_mode=False)
-
-    assert cleanup.called
-    assert result.exit_code == 1
-    assert isinstance(result.exception.__cause__, KeyboardInterrupt)
+    result = cli_run(["image", "download", outdir])
+    assert result.exit_code == 0
 
+    # this is run via atexit, but we want to test it here since we can't
+    # easily test running the command in a subprocess.
+    assert partial_file.exists()
+    cleanup_partially_downloaded_files(Path(outdir))
     assert not partial_file.exists()