fix(webapp): lock the variable row in every env var writer

Creating values for an existing variable and editing a single value did
not lock the variable row before touching value rows, so a concurrent
delete could still form a lock cycle with them. Both now lock the
variable row up front, the same way edit and the delete paths do, and
the lock helper's docblock is the single statement of that order.

Also makes the concurrent parent-delete test deterministic by starting
the bulk delete only after the competing delete has run.

Author: matt-aitken

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

@@ -86,9 +86,9 @@ export type EnvironmentVariableValueRow = {
 };
 
 /**
- * Locks the given variable rows for the rest of the transaction. Every delete path takes this
- * lock before touching any value row, the same variable-then-value order `create` uses through
- * its variable upsert, so a concurrent import and a delete never wait on each other in a cycle.
+ * Locks the given variable rows for the rest of the transaction. Every writer of value rows
+ * calls this before touching any value row, so all of them take locks in the same
+ * variable-then-value order and no two can wait on each other in a cycle.
  */
 async function lockEnvironmentVariableRows(tx: PrismaClientOrTransaction, variableIds: string[]) {
   if (variableIds.length === 0) {
@@ -107,8 +107,9 @@ async function lockEnvironmentVariableRows(tx: PrismaClientOrTransaction, variab
  * with no values afterwards is removed as well. The affected variable rows are locked FOR UPDATE
  * before anything else: inserting a value takes a FOR KEY SHARE lock on its variable row, which
  * conflicts with FOR UPDATE, so an in-flight insert makes the lock wait and the emptiness check
- * then sees the new value, while an insert that starts later waits for the commit and recreates
- * the variable through its upsert.
+ * then sees the new value. An insert that starts later waits for the commit and then either
+ * recreates the variable or fails its foreign key check and fails the import for that key, in
+ * both cases without losing data.
  */
 export async function deleteEnvironmentVariableValueRows(
   tx: PrismaClientOrTransaction,
@@ -314,6 +315,8 @@ export class EnvironmentVariablesRepository implements Repository {
             update: {},
           });
 
+          await lockEnvironmentVariableRows(tx, [environmentVariable.id]);
+
           const secretStore = getSecretStore("DATABASE", {
             prismaClient: tx,
           });
@@ -631,6 +634,8 @@ export class EnvironmentVariablesRepository implements Repository {
 
     try {
       await $transaction(this.prismaClient, "edit env var value", async (tx) => {
+        await lockEnvironmentVariableRows(tx, [options.id]);
+
         const secretStore = getSecretStore("DATABASE", {
           prismaClient: tx,
         });

apps/webapp/test/environmentVariablesRepository.test.ts

@@ -1131,17 +1131,28 @@ describe("EnvironmentVariablesRepository value deletes", () => {
       const deleterHoldsRow = new Promise<void>((resolve) => {
         releaseDeleter = resolve;
       });
+      let markRowDeleted: () => void = () => {};
+      const rowDeleted = new Promise<void>((resolve) => {
+        markRowDeleted = resolve;
+      });
       const deleter = prisma.$transaction(async (tx) => {
         await tx.environmentVariableValue.delete({ where: { id: parentValue.id } });
+        markRowDeleted();
         await deleterHoldsRow;
       });
+      await rowDeleted;
+
       const bulk = repository.deleteValues(project.id, {
         environmentId: branch.id,
         keys: ["SHARED"],
         onlyShadowingParent: true,
       });
-      await new Promise((resolve) => setTimeout(resolve, 300));
-      releaseDeleter();
+      bulk.catch(() => undefined);
+      try {
+        await new Promise((resolve) => setTimeout(resolve, 300));
+      } finally {
+        releaseDeleter();
+      }
       await deleter;
 
       expect(await bulk).toEqual({ deleted: [], skipped: ["SHARED"] });