From caa0757c2f9d7e072898ce3b5136149cb395f712 Mon Sep 17 00:00:00 2001
From: Callahan <callahan.kovacs@canonical.com>
Date: Wed, 2 Oct 2024 09:36:19 -0500
Subject: [PATCH] fix(remotebuild): do not auto clean interrupted builds
 (#5081)

Fixes a bug where the remote builder would ignore the user and always
clean the launchpad project.

Also drops some magic values in favor of constants.

Fixes #4929

Signed-off-by: Callahan Kovacs <callahan.kovacs@canonical.com>
---
 snapcraft/commands/remote.py       |  9 +++++---
 tests/unit/commands/test_remote.py | 33 ++++++++++++++++++++++--------
 2 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/snapcraft/commands/remote.py b/snapcraft/commands/remote.py
index 2ac7b0e2b3..a38ea34a1f 100644
--- a/snapcraft/commands/remote.py
+++ b/snapcraft/commands/remote.py
@@ -308,7 +308,7 @@ def _run(  # noqa: PLR0915 [too-many-statements]
                 emit.progress("Remote repository already exists.", permanent=True)
                 emit.progress("Cleaning up")
                 builder.cleanup()
-                return 75
+                return os.EX_TEMPFAIL
 
         try:
             returncode = self._monitor_and_complete(build_id, builds)
@@ -316,10 +316,13 @@ def _run(  # noqa: PLR0915 [too-many-statements]
             if confirm_with_user("Cancel builds?", default=True):
                 emit.progress("Cancelling builds.")
                 builder.cancel_builds()
-            returncode = 0
+                emit.progress("Cleaning up.")
+                builder.cleanup()
+            return os.EX_OK
         except Exception:  # noqa: BLE001 [blind-except]
             returncode = 1  # General error on any other exception
-        if returncode != 75:  # TimeoutError
+
+        if returncode != os.EX_TEMPFAIL:
             emit.progress("Cleaning up")
             builder.cleanup()
         return returncode
diff --git a/tests/unit/commands/test_remote.py b/tests/unit/commands/test_remote.py
index 6ebdc50633..c50abc9b21 100644
--- a/tests/unit/commands/test_remote.py
+++ b/tests/unit/commands/test_remote.py
@@ -22,7 +22,7 @@
 import sys
 import time
 from pathlib import Path
-from unittest.mock import ANY, Mock
+from unittest.mock import ANY, Mock, call
 
 import pytest
 from craft_application import launchpad
@@ -1019,9 +1019,13 @@ def test_monitor_build_error(mocker, emitter, snapcraft_yaml, base, fake_service
 
 
 @pytest.mark.parametrize("base", const.CURRENT_BASES)
-@pytest.mark.usefixtures("mock_confirm")
-def test_monitor_build_interrupt(mocker, emitter, snapcraft_yaml, base, fake_services):
+@pytest.mark.parametrize("cleanup", [True, False])
+def test_monitor_build_interrupt(
+    cleanup, mock_confirm, mocker, emitter, snapcraft_yaml, base, fake_services
+):
     """Test the monitor_build cleanup when a keyboard interrupt occurs."""
+    # first prompt is for public upload, second is to cancel builds
+    mock_confirm.side_effect = [True, cleanup]
     mocker.patch.object(sys, "argv", ["snapcraft", "remote-build"])
     snapcraft_yaml_dict = {"base": base, "build-base": "devel", "grade": "devel"}
     snapcraft_yaml(**snapcraft_yaml_dict)
@@ -1043,6 +1047,10 @@ def test_monitor_build_interrupt(mocker, emitter, snapcraft_yaml, base, fake_ser
         "craft_application.services.remotebuild.RemoteBuildService.cleanup"
     )
 
+    mock_cancel_builds = mocker.patch(
+        "craft_application.services.remotebuild.RemoteBuildService.cancel_builds"
+    )
+
     app = application.create_app()
     app.services.remote_build._name = get_build_id(
         app.services.app.name, app.project.name, app.project_dir
@@ -1050,15 +1058,24 @@ def test_monitor_build_interrupt(mocker, emitter, snapcraft_yaml, base, fake_ser
     app.services.remote_build._is_setup = True
     app.services.remote_build.request.download_files_with_progress = Mock()
 
-    assert app.run() == 0
+    assert app.run() == os.EX_OK
 
     mock_start_builds.assert_called_once()
     mock_monitor_builds.assert_called_once()
     mock_fetch_logs.assert_not_called()
-    mock_cleanup.assert_called_once()
 
-    emitter.assert_progress("Cancelling builds.")
-    emitter.assert_progress("Cleaning up")
+    cancel_emitted = call("progress", "Cancelling builds.") in emitter.interactions
+    clean_emitted = call("progress", "Cleaning up.") in emitter.interactions
+    if cleanup:
+        mock_cancel_builds.assert_called_once()
+        assert cancel_emitted
+        mock_cleanup.assert_called_once()
+        assert clean_emitted
+    else:
+        mock_cancel_builds.assert_not_called()
+        assert not cancel_emitted
+        mock_cleanup.assert_not_called()
+        assert not clean_emitted
 
 
 @pytest.mark.parametrize("base", const.CURRENT_BASES)
@@ -1091,7 +1108,7 @@ def test_monitor_build_timeout(mocker, emitter, snapcraft_yaml, base, fake_servi
         app.services.app.name, app.project.name, app.project_dir
     )
 
-    assert app.run() == 75
+    assert app.run() == os.EX_TEMPFAIL
 
     mock_start_builds.assert_called_once()
     mock_monitor_builds.assert_called_once()