fix: address Copilot review round 3 (#2339)

- The spawn itself — spawnSpec and the synchronous argument validation in
  child_process.spawn — now sits inside the try whose finally releases the
  lease, so a command that fails before it starts cannot strand the lock
  until stale takeover. Tested with a non-array args value.
- A genuine release failure no longer falls through to the "released
  after" line; the lock may still be blocking the queue, and the message
  that says so is the last word.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>

scripts/gate-lease.mjs

@@ -460,44 +460,49 @@ export async function runUnderLease({
       log(
         `gate-lease: could not release the lease (${err?.message ?? err}). A waiter takes it over once it is ${formatDuration(STALE_MS)} stale — unless whatever blocked this removal persists, in which case remove ${lockPathOf(dir)} by hand.`,
       );
+      // Not "released after": the lock may still be blocking the queue.
+      return;
     }
     log(
       `gate-lease: released after ${formatDuration(Date.now() - startedRunning)}${waited >= pollMs ? ` (waited ${formatDuration(waited)} first)` : ""}.`,
     );
   };
 
-  const spec = spawnSpec(command, args);
-  const child = spawn(spec.command, spec.args, {
-    stdio,
-    env,
-    shell: spec.shell,
-    // Its own group, so one signal reaches every descendant. See the header.
-    detached: spec.detached,
-  });
-
   const signals = ["SIGINT", "SIGTERM", "SIGHUP"];
   let stoppedBy = null;
   let escalation = null;
-  const onSignal = (signal) => {
+  let handlers = [];
-    if (stoppedBy !== null) return;
-    stoppedBy = signal;
-    log(`gate-lease: received ${signal}; stopping the gate.`);
-    signalTree(child, signal);
-    escalation = setTimeout(() => {
-      log(
-        `gate-lease: the gate did not exit within ${graceMs}ms of ${signal}; sending SIGKILL.`,
-      );
-      signalTree(child, "SIGKILL");
-    }, graceMs);
-  };
-  const handlers = signals.map((signal) => {
-    const handler = () => onSignal(signal);
-    process.on(signal, handler);
-    return [signal, handler];
-  });
-
   let outcome;
+  // Everything from here to the child's exit is inside the `try`, so a
+  // synchronous failure to even start it — an argument `winShellArgs`
+  // refuses, a spawn option Node rejects — releases the lease the same way
+  // an asynchronous spawn error does.
   try {
+    const spec = spawnSpec(command, args);
+    const child = spawn(spec.command, spec.args, {
+      stdio,
+      env,
+      shell: spec.shell,
+      // Its own group, so one signal reaches every descendant. See the header.
+      detached: spec.detached,
+    });
+    const onSignal = (signal) => {
+      if (stoppedBy !== null) return;
+      stoppedBy = signal;
+      log(`gate-lease: received ${signal}; stopping the gate.`);
+      signalTree(child, signal);
+      escalation = setTimeout(() => {
+        log(
+          `gate-lease: the gate did not exit within ${graceMs}ms of ${signal}; sending SIGKILL.`,
+        );
+        signalTree(child, "SIGKILL");
+      }, graceMs);
+    };
+    handlers = signals.map((signal) => {
+      const handler = () => onSignal(signal);
+      process.on(signal, handler);
+      return [signal, handler];
+    });
     outcome = await new Promise((resolve, reject) => {
       child.once("error", reject);
       child.once("exit", (code, signal) => resolve({ code, signal }));

scripts/gate-lease.test.mjs

@@ -274,6 +274,27 @@ test("a command that cannot be spawned still releases the lease", async () => {
   assert.ok(!existsSync(leaseTarget(dir)));
 });
 
+test("a command that fails synchronously before it starts still releases the lease", async () => {
+  const dir = freshDir();
+  const { log } = collectLog();
+  // `spawn` validates its arguments synchronously: a non-array `args` throws
+  // before any child exists, the same class as `winShellArgs` refusing a `%`
+  // on Windows. That throw must not strand the lock until stale takeover.
+  await assert.rejects(
+    runUnderLease({
+      command: process.execPath,
+      args: "-e",
+      dir,
+      log,
+      stdio: "ignore",
+      pollMs: 25,
+    }),
+    { code: "ERR_INVALID_ARG_TYPE" },
+  );
+  assert.ok(!existsSync(lockPathOf(dir)));
+  assert.ok(!existsSync(leaseTarget(dir)));
+});
+
 test("a lock replaced mid-run (stale takeover) is the winner's, and is left alone", async () => {
   const dir = freshDir();
   const { lines, log } = collectLog();