fix(webapp): keep env var value deletes to a fixed statement shape

The shared value delete pads its per-row OR arms to a power of two so a
call site does not mint one prepared statement per distinct row count,
folds the emptied-variable sweep into one conditional delete, and takes
the environment id per row so a caller cannot mix environments and
clear the wrong secret store entry.

Adds behavioural tests for the single-value delete path (last value
drops the variable, other environments keep theirs, a value without a
secret reference deletes cleanly) and for skipped duplicate and
already-removed keys in the bulk delete.

Author: matt-aitken

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

@@ -80,19 +80,21 @@ export type EnvironmentVariableValueRow = {
   /** When set, the row is only removed if it still has this version. */
   version?: number;
   variableId: string;
+  environmentId: string;
   key: string;
   secretReferenceKey?: string;
 };
 
 /**
- * The single code path that removes value rows of one environment together with their secret
- * store entries and secret references, using a fixed number of statements for any number of
- * rows. A variable left with no values afterwards is removed as well.
+ * The single code path that removes value rows together with their secret store entries and
+ * secret references, in a fixed number of statements for any number of rows. A variable left
+ * with no values afterwards is removed as well; under READ COMMITTED a value created for that
+ * variable at the same moment can still be swept away with it, a pre-existing window that this
+ * narrows but does not close.
  */
 export async function deleteEnvironmentVariableValueRows(
   tx: PrismaClientOrTransaction,
   projectId: string,
-  environmentId: string,
   rows: EnvironmentVariableValueRow[]
 ): Promise<{ deleted: EnvironmentVariableValueRow[]; skipped: EnvironmentVariableValueRow[] }> {
   if (rows.length === 0) {
@@ -101,7 +103,7 @@ export async function deleteEnvironmentVariableValueRows(
 
   const removed = await tx.environmentVariableValue.deleteMany({
     where: {
-      OR: rows.map((row) =>
+      OR: boundedIn(rows).map((row) =>
         row.version === undefined ? { id: row.id } : { id: row.id, version: row.version }
       ),
     },
@@ -132,22 +134,18 @@ export async function deleteEnvironmentVariableValueRows(
 
   await tx.secretStore.deleteMany({
     where: {
-      key: { in: boundedIn(deleted.map((row) => secretKey(projectId, environmentId, row.key))) },
+      key: {
+        in: boundedIn(deleted.map((row) => secretKey(projectId, row.environmentId, row.key))),
+      },
     },
   });
 
-  const emptied = await tx.environmentVariable.findMany({
+  await tx.environmentVariable.deleteMany({
     where: {
       id: { in: boundedIn(deleted.map((row) => row.variableId)) },
       values: { none: {} },
     },
-    select: { id: true },
   });
-  if (emptied.length > 0) {
-    await tx.environmentVariable.deleteMany({
-      where: { id: { in: boundedIn(emptied.map((variable) => variable.id)) } },
-    });
-  }
 
   return { deleted, skipped };
 }
@@ -1010,10 +1008,11 @@ export class EnvironmentVariablesRepository implements Repository {
 
     try {
       await $transaction(this.prismaClient, "delete env var value", async (tx) => {
-        await deleteEnvironmentVariableValueRows(tx, projectId, options.environmentId, [
+        await deleteEnvironmentVariableValueRows(tx, projectId, [
           {
             id: value.id,
             variableId: environmentVariable.id,
+            environmentId: options.environmentId,
             key: environmentVariable.key,
             secretReferenceKey: value.valueReference?.key,
           },
@@ -1101,17 +1100,13 @@ export class EnvironmentVariablesRepository implements Repository {
             id: own.id,
             version: own.version,
             variableId: variable.id,
+            environmentId: options.environmentId,
             key: variable.key,
             secretReferenceKey: own.valueReference?.key,
           });
         }
 
-        const { deleted } = await deleteEnvironmentVariableValueRows(
-          tx,
-          projectId,
-          options.environmentId,
-          rows
-        );
+        const { deleted } = await deleteEnvironmentVariableValueRows(tx, projectId, rows);
         return deleted.map((row) => row.key);
       }
     );

apps/webapp/test/environmentVariablesRepository.test.ts

@@ -765,7 +765,7 @@ postgresTest(
   }
 );
 
-describe("EnvironmentVariablesRepository.deleteValues", () => {
+describe("EnvironmentVariablesRepository value deletes", () => {
   const vercel = { type: "integration" as const, integration: "vercel" };
 
   async function createBranchWithParent(
@@ -954,12 +954,13 @@ describe("EnvironmentVariablesRepository.deleteValues", () => {
         id: value.id,
         version: value.variable.key === "STALE" ? value.version + 1 : value.version,
         variableId: value.variableId,
+        environmentId: branch.id,
         key: value.variable.key,
         secretReferenceKey: value.valueReference?.key,
       }));
 
       const result = await prisma.$transaction((tx) =>
-        deleteEnvironmentVariableValueRows(tx, project.id, branch.id, rows)
+        deleteEnvironmentVariableValueRows(tx, project.id, rows)
       );
 
       expect(result.deleted.map((r) => r.key)).toEqual(["FRESH"]);
@@ -994,4 +995,87 @@ describe("EnvironmentVariablesRepository.deleteValues", () => {
     ).toEqual({ deleted: [], skipped: ["SHARED"] });
     expect(await theirs.ownKeys(theirs.branch.id)).toEqual(["SHARED"]);
   });
+  postgresTest("skips duplicate keys and keys whose row is already gone", async ({ prisma }) => {
+    const { project, branch, repository, write, ownKeys } = await createBranchWithParent(prisma);
+    await write(branch.id, { GONE: "g", KEPT: "k" }, vercel);
+    await prisma.environmentVariableValue.deleteMany({
+      where: { environmentId: branch.id, variable: { key: "GONE" } },
+    });
+
+    expect(
+      await repository.deleteValues(project.id, {
+        environmentId: branch.id,
+        keys: ["GONE", "GONE", "KEPT", "KEPT", "NEVER"],
+      })
+    ).toEqual({ deleted: ["KEPT"], skipped: ["GONE", "NEVER"] });
+    expect(await ownKeys(branch.id)).toEqual([]);
+  });
+
+  postgresTest(
+    "deleteValue removes the variable with its last value and its secret rows",
+    async ({ prisma }) => {
+      const { project, branch, repository, write, variableKeys, secretRows } =
+        await createBranchWithParent(prisma);
+      await write(branch.id, { ONLY: "o", OTHER: "x" }, vercel);
+      const variable = await prisma.environmentVariable.findFirstOrThrow({
+        where: { projectId: project.id, key: "ONLY" },
+      });
+
+      expect(
+        await repository.deleteValue(project.id, { id: variable.id, environmentId: branch.id })
+      ).toEqual({ success: true });
+      expect(await variableKeys()).toEqual(["OTHER"]);
+      expect(await secretRows(branch.id)).toEqual({ store: ["OTHER"], references: ["OTHER"] });
+      expect(
+        await repository.deleteValue(project.id, { id: variable.id, environmentId: branch.id })
+      ).toEqual({ success: false, error: "Environment variable not found" });
+    }
+  );
+
+  postgresTest(
+    "deleteValue keeps the value the variable has in another environment",
+    async ({ prisma }) => {
+      const { project, parent, branch, repository, write, ownKeys, variableKeys, secretRows } =
+        await createBranchWithParent(prisma);
+      await write(branch.id, { BOTH: "branch" }, vercel);
+      await write(parent.id, { BOTH: "root" }, vercel);
+      const variable = await prisma.environmentVariable.findFirstOrThrow({
+        where: { projectId: project.id, key: "BOTH" },
+      });
+
+      expect(
+        await repository.deleteValue(project.id, { id: variable.id, environmentId: branch.id })
+      ).toEqual({ success: true });
+      expect(await variableKeys()).toEqual(["BOTH"]);
+      expect(await ownKeys(branch.id)).toEqual([]);
+      expect(await ownKeys(parent.id)).toEqual(["BOTH"]);
+      expect(await secretRows(branch.id)).toEqual({ store: [], references: [] });
+      expect(await secretRows(parent.id)).toEqual({ store: ["BOTH"], references: ["BOTH"] });
+      expect(await repository.getEnvironmentVariables(project.id, branch.id, parent.id)).toEqual([
+        { key: "BOTH", value: "root" },
+      ]);
+      expect(
+        await repository.deleteValue(project.id, { id: variable.id, environmentId: branch.id })
+      ).toEqual({ success: false, error: "Environment variable value not found" });
+    }
+  );
+
+  postgresTest("deleteValue handles a value without a secret reference", async ({ prisma }) => {
+    const { project, branch, repository, write, variableKeys, secretRows } =
+      await createBranchWithParent(prisma);
+    await write(branch.id, { UNLINKED: "u" }, vercel);
+    const variable = await prisma.environmentVariable.findFirstOrThrow({
+      where: { projectId: project.id, key: "UNLINKED" },
+    });
+    await prisma.environmentVariableValue.updateMany({
+      where: { variableId: variable.id, environmentId: branch.id },
+      data: { valueReferenceId: null },
+    });
+
+    expect(
+      await repository.deleteValue(project.id, { id: variable.id, environmentId: branch.id })
+    ).toEqual({ success: true });
+    expect(await variableKeys()).toEqual([]);
+    expect((await secretRows(branch.id)).store).toEqual([]);
+  });
 });