Fix concurrent Playground cache lease handoffs

packages/runtime-playground/src/playground-wordpress-archive-cache.ts

@@ -147,7 +147,16 @@ export async function withPlaygroundArchiveCacheLock<T>(cacheDirectory: string,
     if (lease) {
       break
     }
-    if (!await cacheLockIsActive(lockPath, Date.now(), policy, true)) {
+    let active: boolean
+    try {
+      active = await cacheLockIsActive(lockPath, Date.now(), policy, true)
+    } catch (error) {
+      // A waiter can observe the completed owner removing its expired lease sidecar.
+      // Re-read the path rather than treating this expected handoff as a replacement attack.
+      if (isLeaseSidecarHandoff(error)) continue
+      throw error
+    }
+    if (!active) {
       continue
     }
     if (Date.now() - startedAt > 120_000) {
@@ -370,9 +379,11 @@ async function tryAcquireCacheLock(lockPath: string, leaseMs: number): Promise<L
     directory = await openSafeLeaseDirectory(lockPath, false)
     return await createDirectoryLease(directory, lockPath, token, leaseMs)
   } catch (error) {
-    await directory?.handle.close().catch(() => undefined)
-    await rmdir(lockPath).catch(() => undefined)
-    if (isConcurrentDisappearance(error)) return undefined
+    if (directory) {
+      await removeLeaseDirectoryIfCurrent(directory).catch(() => undefined)
+      await directory.handle.close().catch(() => undefined)
+    }
+    if (isConcurrentDisappearance(error) || isLeaseSidecarHandoff(error)) return undefined
     throw error
   }
 }
@@ -459,6 +470,7 @@ async function cacheLockIsActive(lockPath: string, now: number, policy: Pick<Pla
     throw error
   }
   let active = false
+  let inspectionComplete = false
   try {
     let names: string[]
     try {
@@ -490,13 +502,10 @@ async function cacheLockIsActive(lockPath: string, now: number, policy: Pick<Pla
       }
     }
     if (names.length === 0 && now - lockStat.mtimeMs <= policy.staleLockMs) active = true
+    inspectionComplete = true
   } finally {
+    if (removeStale && !active && inspectionComplete) await removeLeaseDirectoryIfCurrent(directory)
     await directory.handle.close()
-    if (removeStale && !active) {
-      await rmdir(lockPath).catch((error) => {
-        if (!isConcurrentDisappearance(error) && !errorHasCode(error, "ENOTEMPTY")) throw error
-      })
-    }
   }
   return active
 }
@@ -709,12 +718,32 @@ function leaseDirectoryChildPath(directory: LeaseDirectory, name?: string): stri
 
 async function assertLeaseDirectoryGeneration(directory: LeaseDirectory): Promise<void> {
   if (directory.access !== "generation-checked-path") return
+  await assertLeaseDirectoryPathGeneration(directory)
+}
+
+async function assertLeaseDirectoryPathGeneration(directory: LeaseDirectory): Promise<void> {
   const [pathStat, handleStat] = await Promise.all([lstat(directory.path), directory.handle.stat()])
   if (!pathStat.isDirectory() || pathStat.isSymbolicLink() || pathStat.dev !== handleStat.dev || pathStat.ino !== handleStat.ino) {
     throw new LeaseSidecarUnsafeError(`Playground cache lease sidecar changed while accessing: ${directory.path}`)
   }
 }
 
+async function removeLeaseDirectoryIfCurrent(directory: LeaseDirectory): Promise<boolean> {
+  try {
+    await assertLeaseDirectoryPathGeneration(directory)
+  } catch (error) {
+    if (isConcurrentDisappearance(error) || isLeaseSidecarHandoff(error)) return false
+    throw error
+  }
+  try {
+    await rmdir(directory.path)
+    return true
+  } catch (error) {
+    if (isConcurrentDisappearance(error) || errorHasCode(error, "ENOTEMPTY")) return false
+    throw error
+  }
+}
+
 async function readLeaseDirectory(directory: LeaseDirectory): Promise<string[]> {
   await assertLeaseDirectoryGeneration(directory)
   try {
@@ -844,6 +873,10 @@ function isConcurrentDisappearance(error: unknown): boolean {
   return errorHasCode(error, "ENOENT") || errorHasCode(error, "ESTALE")
 }
 
+function isLeaseSidecarHandoff(error: unknown): boolean {
+  return error instanceof LeaseSidecarUnsafeError && error.message.includes("changed while accessing")
+}
+
 async function unlinkIfPresent(path: string): Promise<boolean> {
   try {
     await unlink(path)

tests/fixtures/playground-cache-lock-child.ts

@@ -0,0 +1,34 @@
+import { lstat, writeFile } from "node:fs/promises"
+
+import { withPlaygroundArchiveCacheLock } from "../../packages/runtime-playground/src/playground-wordpress-archive-cache.js"
+
+const [cacheDirectory, version, archivePath, iterations, readyPath, startPath] = process.argv.slice(2)
+if (!cacheDirectory || !version || !archivePath || !iterations || !readyPath || !startPath) {
+  throw new Error("cacheDirectory, version, archivePath, iterations, readyPath, and startPath are required")
+}
+
+await writeFile(readyPath, "ready")
+while (!await exists(startPath)) {
+  await new Promise((resolve) => setTimeout(resolve, 5))
+}
+
+for (let index = 0; index < Number(iterations); index += 1) {
+  await withPlaygroundArchiveCacheLock(cacheDirectory, version, async () => {
+    try {
+      await writeFile(archivePath, `${process.pid}\n`, { flag: "wx" })
+    } catch (error) {
+      if (!error || typeof error !== "object" || !("code" in error) || error.code !== "EEXIST") throw error
+    }
+    await new Promise((resolve) => setTimeout(resolve, 2))
+  })
+}
+
+async function exists(path: string): Promise<boolean> {
+  try {
+    await lstat(path)
+    return true
+  } catch (error) {
+    if (error && typeof error === "object" && "code" in error && error.code === "ENOENT") return false
+    throw error
+  }
+}

tests/playground-custom-archive-cache-process.test.ts

@@ -1,13 +1,14 @@
 import assert from "node:assert/strict"
 import { spawn, type ChildProcess } from "node:child_process"
-import { lstat, mkdtemp, readFile, readdir, rm, utimes, writeFile } from "node:fs/promises"
+import { lstat, mkdir, mkdtemp, readFile, readdir, rm, utimes, writeFile } from "node:fs/promises"
 import { tmpdir } from "node:os"
 import { join } from "node:path"
 
 import { maintainPlaygroundCustomArchiveCache } from "../packages/runtime-playground/src/playground-wordpress-archive-cache.js"
 
 const root = await mkdtemp(join(tmpdir(), "wp-codebox-playground-cache-process-"))
 const fixture = join(process.cwd(), "tests/fixtures/playground-cache-lease-child.ts")
+const lockFixture = join(process.cwd(), "tests/fixtures/playground-cache-lock-child.ts")
 const tsx = join(process.cwd(), "node_modules/.bin/tsx")
 const children: ChildProcess[] = []
 
@@ -37,6 +38,33 @@ try {
   assert.ok(afterExpiry.removedCount >= 1, JSON.stringify(afterExpiry))
   assert.ok(!await exists(archivePath), "expired crashed-process lease must become reclaimable")
 
+  const lockArchivePath = join(root, "7.0.3.zip")
+  const firstLockChild = await startLockChild("first-lock-child", "7.0.3", lockArchivePath, 300)
+  const secondLockChild = await startLockChild("second-lock-child", "7.0.3", lockArchivePath, 300)
+  await Promise.all([writeFile(firstLockChild.startPath, "start"), writeFile(secondLockChild.startPath, "start")])
+  const lockResults = await Promise.all([firstLockChild.result, secondLockChild.result])
+  assert.deepEqual(lockResults.map((result) => result.code), [0, 0], lockResults.map((result) => result.output).join("\n"))
+  assert.match(await readFile(lockArchivePath, "utf8"), /^\d+\n$/, "exactly one lock owner must materialize the archive")
+
+  const staleVersion = "7.0.4"
+  const staleLockPath = join(root, `${staleVersion}.zip.lock`)
+  await mkdir(staleLockPath)
+  await writeFile(join(staleLockPath, "owner.json"), JSON.stringify({
+    schema: "wp-codebox/playground-cache-lease/v1",
+    token: "crashed-owner",
+    hostname: "test-host",
+    bootId: "test-boot",
+    pid: 1,
+    processStart: "0",
+    createdAt: new Date(0).toISOString(),
+    heartbeatAt: new Date(0).toISOString(),
+    expiresAt: new Date(0).toISOString(),
+  }))
+  const staleChild = await startLockChild("stale-lock-child", staleVersion, join(root, `${staleVersion}.zip`), 1)
+  await writeFile(staleChild.startPath, "start")
+  const staleResult = await staleChild.result
+  assert.equal(staleResult.code, 0, staleResult.output)
+
   console.log("playground custom archive separate-process leases passed")
 } finally {
   for (const child of children) {
@@ -66,6 +94,25 @@ async function stopChild(entry: { child: ChildProcess; stopPath: string }): Prom
   await childExit(entry.child)
 }
 
+async function startLockChild(name: string, version: string, archivePath: string, iterations: number): Promise<{ startPath: string; result: Promise<{ code: number | null; output: string }> }> {
+  const readyPath = join(root, `${name}.ready`)
+  const startPath = join(root, `${name}.start`)
+  let child!: ChildProcess
+  const result = new Promise<{ code: number | null; output: string }>((resolve, reject) => {
+    child = spawn(tsx, [lockFixture, root, version, archivePath, String(iterations), readyPath, startPath], {
+      env: { ...process.env, WP_CODEBOX_PLAYGROUND_CUSTOM_ARCHIVE_LEASE_MS: "300" },
+      stdio: ["ignore", "pipe", "pipe"],
+    })
+    let output = ""
+    child.stdout?.on("data", (chunk) => { output += String(chunk) })
+    child.stderr?.on("data", (chunk) => { output += String(chunk) })
+    child.once("error", reject)
+    child.once("close", (code) => resolve({ code, output }))
+  })
+  await waitForPath(readyPath, child)
+  return { startPath, result }
+}
+
 async function waitForPath(path: string, child: ChildProcess): Promise<void> {
   for (let attempt = 0; attempt < 100; attempt += 1) {
     if (await exists(path)) return