fix(webapp): make the branch env var prune tolerant of concurrent edits

The prune now selects and deletes inside one transaction and only
removes a branch value row at the version it read, so a value edited
in the meantime is left alone rather than deleted. Missing secret or
reference rows for one key no longer abort the whole prune.

Author: matt-aitken

apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts

@@ -980,83 +980,83 @@ export class EnvironmentVariablesRepository implements Repository {
       return { prunedKeys: [], failures: [] };
     }
 
-    const variables = await this.prismaClient.environmentVariable.findMany({
-      where: {
-        projectId,
-        key: { in: boundedIn(options.keys) },
-        project: { deletedAt: null },
-      },
-      select: {
-        id: true,
-        key: true,
-        values: {
-          where: { environmentId: { in: [options.environmentId, options.parentEnvironmentId] } },
-          select: {
-            id: true,
-            environmentId: true,
-            lastUpdatedBy: true,
-            valueReference: { select: { key: true } },
-          },
-        },
-      },
-    });
-
-    const shadowing: { id: string; key: string; secretReferenceKey: string | undefined }[] = [];
-    for (const variable of variables) {
-      const branchValue = variable.values.find((v) => v.environmentId === options.environmentId);
-      const parentValue = variable.values.find(
-        (v) => v.environmentId === options.parentEnvironmentId
-      );
-      if (
-        !branchValue ||
-        !parentValue ||
-        !isSameUpdater(branchValue.lastUpdatedBy, options.source)
-      ) {
-        continue;
-      }
-      shadowing.push({
-        id: branchValue.id,
-        key: variable.key,
-        secretReferenceKey: branchValue.valueReference?.key,
-      });
-    }
-
-    if (shadowing.length === 0) {
-      return { prunedKeys: [], failures: [] };
-    }
-
-    const keys = shadowing.map((v) => v.key);
     try {
-      await $transaction(this.prismaClient, "prune branch env var values", async (tx) => {
-        const secretStore = getSecretStore("DATABASE", {
-          prismaClient: tx,
-        });
+      const prunedKeys = await $transaction(
+        this.prismaClient,
+        "prune branch env var values",
+        async (tx) => {
+          const variables = await tx.environmentVariable.findMany({
+            where: {
+              projectId,
+              key: { in: boundedIn(options.keys) },
+              project: { deletedAt: null },
+            },
+            select: {
+              id: true,
+              key: true,
+              values: {
+                where: {
+                  environmentId: { in: [options.environmentId, options.parentEnvironmentId] },
+                },
+                select: {
+                  id: true,
+                  version: true,
+                  environmentId: true,
+                  lastUpdatedBy: true,
+                  valueReference: { select: { key: true } },
+                },
+              },
+            },
+          });
 
-        for (const value of shadowing) {
-          await secretStore.deleteSecret(secretKey(projectId, options.environmentId, value.key));
+          const pruned: string[] = [];
+          for (const variable of variables) {
+            const branchValue = variable.values.find(
+              (v) => v.environmentId === options.environmentId
+            );
+            const parentValue = variable.values.find(
+              (v) => v.environmentId === options.parentEnvironmentId
+            );
+            if (
+              !branchValue ||
+              !parentValue ||
+              !isSameUpdater(branchValue.lastUpdatedBy, options.source)
+            ) {
+              continue;
+            }
 
-          if (value.secretReferenceKey) {
-            await tx.secretReference.delete({
-              where: {
-                key: value.secretReferenceKey,
-              },
+            const deleted = await tx.environmentVariableValue.deleteMany({
+              where: { id: branchValue.id, version: branchValue.version },
             });
+            if (deleted.count === 0) {
+              continue;
+            }
+
+            if (branchValue.valueReference) {
+              await tx.secretReference.deleteMany({
+                where: { key: branchValue.valueReference.key },
+              });
+            }
+            await tx.secretStore.deleteMany({
+              where: { key: secretKey(projectId, options.environmentId, variable.key) },
+            });
+
+            pruned.push(variable.key);
           }
 
-          await tx.environmentVariableValue.delete({
-            where: {
-              id: value.id,
-            },
-          });
+          return pruned;
         }
-      });
+      );
 
-      return { prunedKeys: keys, failures: [] };
+      return { prunedKeys: prunedKeys ?? [], failures: [] };
     } catch (error) {
       return {
         prunedKeys: [],
         failures: [
-          { keys, error: error instanceof Error ? error.message : "Something went wrong" },
+          {
+            keys: options.keys,
+            error: error instanceof Error ? error.message : "Something went wrong",
+          },
         ],
       };
     }

apps/webapp/test/environmentVariablesRepository.test.ts

@@ -994,6 +994,62 @@ describe("EnvironmentVariablesRepository.pruneBranchValuesShadowingParent", () =
     }
   );
 
+  postgresTest("prunes several same-source keys in one call", async ({ prisma }) => {
+    const { project, parent, branch, repository, write, ownKeys } =
+      await createBranchWithParent(prisma);
+    await write(branch.id, { A: "a-branch", B: "b-branch", C: "c-branch" }, vercel);
+    await write(parent.id, { A: "a-root", B: "b-root", C: "c-root" }, vercel);
+
+    const result = await repository.pruneBranchValuesShadowingParent(project.id, {
+      environmentId: branch.id,
+      parentEnvironmentId: parent.id,
+      keys: ["A", "B"],
+      source: vercel,
+    });
+
+    expect(result).toEqual({ prunedKeys: ["A", "B"], failures: [] });
+    expect(await ownKeys(branch.id)).toEqual(["C"]);
+    expect(await ownKeys(parent.id)).toEqual(["A", "B", "C"]);
+    const branchPrefix = `environmentvariable:${project.id}:${branch.id}:`;
+    expect(
+      await prisma.secretStore.findMany({ where: { key: { startsWith: branchPrefix } } })
+    ).toHaveLength(1);
+    expect(
+      await prisma.secretReference.findMany({ where: { key: { startsWith: branchPrefix } } })
+    ).toHaveLength(1);
+    expect(
+      Object.fromEntries(
+        (await repository.getEnvironmentVariables(project.id, branch.id, parent.id)).map(
+          ({ key, value }) => [key, value]
+        )
+      )
+    ).toEqual({ A: "a-root", B: "b-root", C: "c-branch" });
+  });
+
+  postgresTest("a missing secret row for one key does not stop the others", async ({ prisma }) => {
+    const { project, parent, branch, repository, write, ownKeys } =
+      await createBranchWithParent(prisma);
+    await write(branch.id, { A: "a", B: "b" }, vercel);
+    await write(parent.id, { A: "root", B: "root" }, vercel);
+    await prisma.secretStore.deleteMany({
+      where: { key: `environmentvariable:${project.id}:${branch.id}:A` },
+    });
+    await prisma.secretReference.deleteMany({
+      where: { key: `environmentvariable:${project.id}:${branch.id}:B` },
+    });
+
+    const result = await repository.pruneBranchValuesShadowingParent(project.id, {
+      environmentId: branch.id,
+      parentEnvironmentId: parent.id,
+      keys: ["A", "B"],
+      source: vercel,
+    });
+
+    expect(result).toEqual({ prunedKeys: ["A", "B"], failures: [] });
+    expect(await ownKeys(branch.id)).toEqual([]);
+    expect(await ownKeys(parent.id)).toEqual(["A", "B"]);
+  });
+
   postgresTest("does not reach into another project's variables", async ({ prisma }) => {
     const mine = await createBranchWithParent(prisma);
     const theirs = await createBranchWithParent(prisma);