diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index 813ba992fb..5e40569af0 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -1708,6 +1708,24 @@ def workflow_list(): console.print() +def _cleanup_download_tmp_path(tmp_path: Path | None) -> None: + """Best-effort unlink of a partially-downloaded workflow temp file. + + A cleanup ``OSError`` here must never replace/mask whatever error or + interrupt is already propagating -- warn about it and keep going. + """ + if tmp_path is None: + return + try: + tmp_path.unlink(missing_ok=True) + except OSError as cleanup_exc: + console.print( + "[yellow]Warning:[/yellow] Could not remove temporary " + f"workflow download file: {_escape_markup(str(cleanup_exc))} " + f"(path: {_escape_markup(str(tmp_path))})" + ) + + @workflow_app.command("add") def workflow_add( source: str = typer.Argument(..., help="Workflow ID, URL, or local path"), @@ -2037,23 +2055,23 @@ def _validate_and_install_local( _enforce_workflow_yaml_size(downloaded_content) tmp.write(downloaded_content) except typer.Exit: + _cleanup_download_tmp_path(tmp_path) raise except Exception as exc: - if tmp_path is not None: - # A cleanup failure here must never replace/mask the - # original download error below with a raw, unhandled - # OSError -- warn about it and keep going, exactly like the - # later post-install finally cleanup does. - try: - tmp_path.unlink(missing_ok=True) - except OSError as cleanup_exc: - console.print( - "[yellow]Warning:[/yellow] Could not remove temporary " - f"workflow download file: {_escape_markup(str(cleanup_exc))} " - f"(path: {_escape_markup(str(tmp_path))})" - ) + # A cleanup failure here must never replace/mask the + # original download error below with a raw, unhandled + # OSError -- warn about it and keep going, exactly like the + # later post-install finally cleanup does. + _cleanup_download_tmp_path(tmp_path) console.print(f"[red]Error:[/red] Failed to download workflow: {_escape_markup(str(exc))}") raise typer.Exit(1) + except BaseException: + # Covers KeyboardInterrupt and other non-Exception exits: the + # temp file is already created on disk (delete=False) by this + # point, so an interrupt during the size-limited read must still + # unlink it rather than leaking it to the system temp directory. + _cleanup_download_tmp_path(tmp_path) + raise try: if downloaded_archive_format is None: _validate_and_install_local( diff --git a/tests/test_workflows.py b/tests/test_workflows.py index afd70adecf..2242daad97 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -12343,6 +12343,46 @@ def test_add_from_url_oversized_streamed_body_leaves_no_temp_file( leaked = list(scratch_tmp.glob("*.yml")) assert leaked == [], f"leaked temp files: {leaked}" + def test_add_from_url_interrupt_during_read_leaves_no_temp_file( + self, project_dir, monkeypatch, tmp_path + ): + """A KeyboardInterrupt while streaming the response body must still + unlink the already-created (delete=False) temp file. Unlike a + download ``ValueError``, ``KeyboardInterrupt`` is a ``BaseException`` + and is not caught by ``except Exception`` -- only a ``BaseException`` + handler around the temp-file lifetime can clean it up.""" + import tempfile as tempfile_mod + from unittest.mock import patch + from typer.testing import CliRunner + from specify_cli import app + from specify_cli.workflows import _commands as wf_commands + + monkeypatch.chdir(project_dir) + scratch_tmp = tmp_path / "scratch-tmp" + scratch_tmp.mkdir() + monkeypatch.setattr(tempfile_mod, "tempdir", str(scratch_tmp)) + + def _boom(*args, **kwargs): + raise KeyboardInterrupt() + + monkeypatch.setattr(wf_commands, "_read_response_within_limit", _boom) + body = b"id: align-wf\n" + runner = CliRunner() + with patch( + "specify_cli.authentication.http.open_url", + side_effect=lambda url, timeout=None, extra_headers=None, redirect_validator=None: self._FakeResponse( + body, url + ), + ): + result = runner.invoke( + app, + ["workflow", "add", "align-wf", "--from", "https://example.com/workflow.yml"], + input="y\n", + ) + assert result.exit_code != 0 + leaked = list(scratch_tmp.glob("*.yml")) + assert leaked == [], f"leaked temp files: {leaked}" + def test_add_from_url_oversized_content_length_leaves_no_temp_file( self, project_dir, monkeypatch, tmp_path ):