launchpad-reviewers team mailing list archive

[Colin Watson <mp+455550@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:signing-cleanup into launchpad:master.

Commit message:
signing: Clean up installed directory on failures

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/455550

If `SigningUpload.installFiles` raises an exception, then it shouldn't leave the half-installed files around, since that will make it impossible to retry processing without manual intervention.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:signing-cleanup into launchpad:master.

diff --git a/lib/lp/archivepublisher/signing.py b/lib/lp/archivepublisher/signing.py
index 36f5681..9938a95 100644
--- a/lib/lp/archivepublisher/signing.py
+++ b/lib/lp/archivepublisher/signing.py
@@ -841,14 +841,20 @@ class SigningUpload(CustomUpload):
         # Avoid circular import.
         from lp.archivepublisher.publishing import DirectoryHash
 
-        super().installFiles(archive, suite)
-
         versiondir = os.path.join(self.targetdir, self.version)
-        with DirectoryHash(versiondir, self.temproot) as hasher:
-            hasher.add_dir(versiondir)
-        for checksum_path in hasher.checksum_paths:
-            if self.shouldSign(checksum_path):
-                self.sign(archive, suite, checksum_path)
+
+        try:
+            super().installFiles(archive, suite)
+
+            with DirectoryHash(versiondir, self.temproot) as hasher:
+                hasher.add_dir(versiondir)
+            for checksum_path in hasher.checksum_paths:
+                if self.shouldSign(checksum_path):
+                    self.sign(archive, suite, checksum_path)
+        except Exception:
+            if os.path.exists(versiondir):
+                shutil.rmtree(versiondir, ignore_errors=True)
+            raise
 
     def shouldInstall(self, filename):
         return filename.startswith("%s/" % self.version)
diff --git a/lib/lp/archivepublisher/tests/test_signing.py b/lib/lp/archivepublisher/tests/test_signing.py
index 11259d7..4318cb8 100644
--- a/lib/lp/archivepublisher/tests/test_signing.py
+++ b/lib/lp/archivepublisher/tests/test_signing.py
@@ -687,6 +687,11 @@ class TestLocalSigningUpload(RunPartsMixin, TestSigningHelpers):
         self.tarfile.add_file("1.0/dir/file.efi", b"foo")
         os.umask(0o002)  # cleanup already handled by setUp
         self.assertRaises(CustomUploadBadUmask, self.process)
+        self.assertFalse(
+            os.path.exists(
+                os.path.join(self.getSignedPath("test", "amd64"), "1.0")
+            )
+        )
 
     def test_correct_uefi_signing_command_executed(self):
         # Check that calling signUefi() will generate the expected command