From 8d3487cdfdb7297b2ab6bd9d7a96f6b9727e1002 Mon Sep 17 00:00:00 2001
From: Michael Nguyen <13559011+nguyenm7@users.noreply.github.com>
Date: Sun, 13 Sep 2026 00:45:58 -0700
Subject: [PATCH] fix(apps): preserve artwork on cleanup failure

---
 scripts/app-brand-maintenance.mjs      | 15 ++++++++++++---
 scripts/app-brand-maintenance.test.mjs | 25 +++++++++++++++++++++++++
 scripts/import-app-brand-assets.mjs    |  7 ++++++-
 3 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/scripts/app-brand-maintenance.mjs b/scripts/app-brand-maintenance.mjs
index ff0e893ba8..a06ae1c569 100644
--- a/scripts/app-brand-maintenance.mjs
+++ b/scripts/app-brand-maintenance.mjs
@@ -37,9 +37,6 @@ export function applyFileBatchAtomically(changes, options = {}) {
       }
     }
 
-    for (const entry of prepared) {
-      if (entry.existed && fsApi.existsSync(entry.backup)) fsApi.unlinkSync(entry.backup);
-    }
   } catch (error) {
     const rollbackErrors = [];
     for (const entry of [...prepared].reverse()) {
@@ -54,4 +51,16 @@ export function applyFileBatchAtomically(changes, options = {}) {
     const suffix = rollbackErrors.length ? ` Rollback errors: ${rollbackErrors.join("; ")}` : "";
     throw new Error(`Connector artwork import failed; the previous batch was restored.${suffix}`, { cause: error });
   }
+
+  // Cleanup happens only after the transaction is committed. A cleanup error
+  // must never enter rollback after another backup has already been removed.
+  const cleanupErrors = [];
+  for (const entry of prepared) {
+    try {
+      if (entry.existed && fsApi.existsSync(entry.backup)) fsApi.unlinkSync(entry.backup);
+    } catch (error) {
+      cleanupErrors.push(`${entry.backup}: ${error.message}`);
+    }
+  }
+  return cleanupErrors;
 }
diff --git a/scripts/app-brand-maintenance.test.mjs b/scripts/app-brand-maintenance.test.mjs
index 17e3ffb019..5116f46c31 100644
--- a/scripts/app-brand-maintenance.test.mjs
+++ b/scripts/app-brand-maintenance.test.mjs
@@ -56,3 +56,28 @@ test("failed artwork batches restore every previous destination", (t) => {
   assert.equal(fs.readFileSync(second, "utf8"), "second-old");
   assert.deepEqual(fs.readdirSync(directory).sort(), ["first.svg", "second.svg"]);
 });
+
+test("backup cleanup failures preserve committed artwork", (t) => {
+  const directory = fs.mkdtempSync(path.join(os.tmpdir(), "paperclip-brand-cleanup-"));
+  t.after(() => fs.rmSync(directory, { recursive: true, force: true }));
+  const target = path.join(directory, "icon.svg");
+  fs.writeFileSync(target, "old");
+  const fsApi = {
+    ...fs,
+    unlinkSync(file) {
+      if (file.endsWith(".bak")) throw new Error("simulated cleanup failure");
+      return fs.unlinkSync(file);
+    },
+  };
+
+  const cleanupErrors = applyFileBatchAtomically(
+    [{ target, bytes: Buffer.from("new") }],
+    { fsApi, transactionId: "cleanup-test" },
+  );
+  assert.equal(fs.readFileSync(target, "utf8"), "new");
+  assert.equal(cleanupErrors.length, 1);
+  assert.equal(
+    fs.readFileSync(`${target}.paperclip-cleanup-test.bak`, "utf8"),
+    "old",
+  );
+});
diff --git a/scripts/import-app-brand-assets.mjs b/scripts/import-app-brand-assets.mjs
index 87da8ab9dd..4313d4ef8e 100644
--- a/scripts/import-app-brand-assets.mjs
+++ b/scripts/import-app-brand-assets.mjs
@@ -27,5 +27,10 @@ for (const [asset, bytes] of pending) {
   changed++;
   changes.push({ target, bytes });
 }
-if (args.includes("--apply")) applyFileBatchAtomically(changes);
+if (args.includes("--apply")) {
+  const cleanupErrors = applyFileBatchAtomically(changes);
+  if (cleanupErrors.length) {
+    console.warn(`Artwork applied, but ${cleanupErrors.length} backup file(s) could not be removed:\n${cleanupErrors.join("\n")}`);
+  }
+}
 console.log(`${args.includes("--apply") ? "Applied" : "Dry run"}: ${count} identities validated; ${changed} asset files ${args.includes("--apply") ? "updated" : "would change"}.`);