seat, cli, harness: git kill reach, close bound and host.lock (#1597)

Row 103, the row 100 follow-up. Built by Darkwing, packet agents/darkwing/work/queue-103/r1/, candidate manifest 6e5ac8c0782bc595d7c0d711d457738ffb42f1ba5f38253912943dd55e55888a. Sage approved on #1597, comment 28252; Dewey approved, comment 28260. Host git runs through runGit: a detached group, SIGTERM at the limit, SIGKILL after the grace, and one more SIGKILL to the group after a signalled git closes. git-socket close() waits at most 2 s. startHost claims bus-host/host.lock by hard link before the broker fork. packages/cli/README.md and tests/host.test.mjs carry row 85's hunks too; row 85 lands first. Sage's gate rerun on 3a1e11b83 with rows 85, 102 and 103 applied, Docker up: node suites bus 128, business 67, cli 165, control-board 124, conversation 182, discord 178, harness 73, ledger 78, mosaic 69, queue 155, runs 43, seat 113, tasks 92, webui 108; scripts auth 15, conductor 17, config 24, discord 66, extension-package 18, foundation 44, queue 28, release 14, task 98. All pass, 0 fail. Co-Authored-By: Claude Opus 5.5 <[email protected]>
2026-10-13 16:34:47 -05:00
parent 2a9b77b56b
commit 465bbdfbbc
12 changed files with 665 additions and 106 deletions
@@ -28,6 +28,9 @@ const PROJECT = /^[a-z0-9][a-z0-9._-]{0,63}$/;
 const WORK_REF = /^work\/vikunja-[1-9][0-9]*-[1-9][0-9]*$/;
 const OID = /^[0-9a-f]{40}([0-9a-f]{24})?$/;
 const TIMEOUT_MS = 300_000;
+// How long a git has after SIGTERM before SIGKILL, as on the host.
+const GRACE_MS = 5000;
+const LIMITS = { timeoutMs: TIMEOUT_MS, graceMs: GRACE_MS };
 const SAFE = [
  "-c", "core.hooksPath=/dev/null",
  "-c", "core.fsmonitor=false",
@@ -42,20 +45,44 @@ const failure = (code, message = code) => Object.assign(new Error(message), { co
 export const validBranch = (branch) =>
  typeof branch === "string" && /^[A-Za-z0-9][A-Za-z0-9._\/-]{0,199}$/.test(branch) && !/\.\.|\/\/|\/\.|@\{|\.lock(\/|$)|[./]$/.test(branch);

-function git(args, env = process.env) {
+// git with SAFE first, in its own process group (detached). Past
+// limits.timeoutMs the group gets SIGTERM, on which git removes its
+// lockfiles, then SIGKILL limits.graceMs later; once a signalled git has
+// closed, the group gets SIGKILL again. The same sequence as the host's
+// runGit in packages/seat/src/checkout.mjs, kept here so the runner doesn't
+// load the host's module. Exported for the tests, which shorten `limits`.
+export function git(args, env = process.env, limits = LIMITS) {
  return new Promise((resolve) => {
    const child = spawn("git", [...SAFE, ...args], {
      env: { PATH: env.PATH ?? "/usr/bin:/bin", HOME: "/nonexistent", GIT_CONFIG_NOSYSTEM: "1", GIT_CONFIG_GLOBAL: "/dev/null", GIT_TERMINAL_PROMPT: "0", LC_ALL: "C" },
-      timeout: TIMEOUT_MS,
-      killSignal: "SIGKILL",
+      detached: true,
      stdio: ["ignore", "pipe", "pipe"],
    });
    let out = "";
    let err = "";
+    let signalled = false;
+    let kill = null;
+    // A git that never started has no pid; an empty group is ESRCH.
+    const group = (signal) => {
+      try {
+        process.kill(-child.pid, signal);
+      } catch {}
+    };
+    const term = setTimeout(() => {
+      signalled = true;
+      group("SIGTERM");
+      kill = setTimeout(() => group("SIGKILL"), limits.graceMs);
+    }, limits.timeoutMs);
+    const settle = (r) => {
+      clearTimeout(term);
+      clearTimeout(kill);
+      if (signalled) group("SIGKILL");
+      resolve(r);
+    };
    child.stdout.setEncoding("utf8").on("data", (d) => (out += d));
    child.stderr.setEncoding("utf8").on("data", (d) => (err += d));
-    child.on("error", (e) => resolve({ ok: false, out, err, why: e.code ?? e.message }));
-    child.on("close", (code, signal) => resolve({ ok: code === 0, out, err, why: signal ? `killed by ${signal}` : err.trim().split("\n").pop() || `exit ${code}` }));
+    child.on("error", (e) => settle({ ok: false, out, err, why: e.code ?? e.message }));
+    child.on("close", (code, signal) => settle({ ok: code === 0, out, err, why: signal ? `killed by ${signal}` : err.trim().split("\n").pop() || `exit ${code}` }));
  });
 }

@@ -169,8 +196,11 @@ export async function pushWorking({ path, cap, workspace, task_ref, project, bra
 // Resolves { project, ref, commit, clone }; ref is the clone's remote ref.
 export async function fetchReview({ path, cap, workspace, task_ref, request, tmp = "/tmp", env = process.env }) {
  // Commits the host may leave out: what the clones' remotes already hold.
-  // The working refs come first, newest first, since a review is most often
-  // of a commit built on one; origin's fill what is left of HAVE_MAX.
+  // The working refs come first, since a review is most often of a commit
+  // built on one; origin's fill what is left of HAVE_MAX. The order is per
+  // clone, not across clones: each clone's refs newest first, one clone
+  // after another in the workspace's directory order. So past HAVE_MAX, a
+  // later clone's newer refs can lose to an earlier clone's older ones.
  const have = new Set();
  const projects = projectsIn(workspace);
  for (const remote of ["refs/remotes/working", "refs/remotes/origin"])
@@ -5,10 +5,10 @@
 import { test } from "node:test";
 import assert from "node:assert/strict";
 import { spawnSync } from "node:child_process";
-import { mkdirSync } from "node:fs";
+import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs";
 import { createServer } from "node:net";
 import { join } from "node:path";
-import { fetchReview, HAVE_MAX } from "../src/git-tools.mjs";
+import { fetchReview, git as runnerGit, HAVE_MAX } from "../src/git-tools.mjs";
 import { scratch } from "./helpers.mjs";

 const ENV = { PATH: process.env.PATH, HOME: "/nonexistent", GIT_CONFIG_NOSYSTEM: "1", GIT_CONFIG_GLOBAL: "/dev/null", LC_ALL: "C" };
@@ -78,3 +78,88 @@ test("fetch_review's have lists the working refs before origin's, and no more th
  const origin = new Set(Object.entries(a).filter(([n]) => n.startsWith("refs/remotes/origin/")).map(([, oid]) => oid));
  for (const oid of req.have.slice(13)) assert.ok(origin.has(oid), oid);
 });
+
+// Past HAVE_MAX the order is per clone: each clone's working refs newest
+// first, one clone after another, so an earlier clone's older refs come
+// ahead of a later clone's newer ones. The clones' order is the workspace's
+// directory order, so the test takes it from the request.
+test("fetch_review's have orders the working refs newest first within each clone, one clone after another", async (t) => {
+  const dir = scratch(t);
+  const workspace = join(dir, "ws");
+  // 20 working refs in each clone; beta's are all newer than alpha's.
+  // Created in a shuffled order, so the name order isn't the date order.
+  const order = [7, 2, 15, 0, 11, 19, 4, 9, 13, 1, 17, 6, 3, 18, 10, 5, 14, 8, 16, 12];
+  const refs = (task, base) => Object.fromEntries(order.map((i) => [`refs/remotes/working/work/vikunja-${task}-${i + 1}`, base + i * 60]));
+  const alpha = clone(join(workspace, "alpha"), refs(1, 1_700_000_000));
+  const beta = clone(join(workspace, "beta"), refs(2, 1_750_000_000));
+  const host = await fakeHost(t, dir);
+  await assert.rejects(fetchReview({ path: host.path, cap: "c".repeat(64), workspace, task_ref: "vikunja:1/2", request: "r1", tmp: dir, env: ENV }), (e) => e.code === "request-not-found");
+  const [req] = host.lines;
+  // Newest first: the ref for i = 19 first, i = 0 last.
+  const newest = (oids, task) => order.slice().sort((x, y) => y - x).map((i) => oids[`refs/remotes/working/work/vikunja-${task}-${i + 1}`]);
+  const [first, second] = new Set(Object.values(alpha)).has(req.have[0]) ? [newest(alpha, 1), newest(beta, 2)] : [newest(beta, 2), newest(alpha, 1)];
+  assert.deepEqual(req.have, [...first, ...second].slice(0, HAVE_MAX));
+});
+
+// A git on PATH that runs `body` in place of the real one.
+function fakeGit(dir, body) {
+  const bin = join(dir, "bin");
+  mkdirSync(bin);
+  writeFileSync(join(bin, "git"), `#!/bin/bash\n${body}\n`, { mode: 0o755 });
+  return { ...ENV, PATH: `${bin}:${process.env.PATH}` };
+}
+
+// True once `pid` has gone, or is a zombie waiting to be reaped.
+async function gone(pid, ms) {
+  const end = Date.now() + ms;
+  for (;;) {
+    let stat = null;
+    try {
+      stat = readFileSync(`/proc/${pid}/stat`, "utf8");
+    } catch {}
+    if (stat === null || stat.slice(stat.lastIndexOf(")") + 2)[0] === "Z") return true;
+    if (Date.now() > end) return false;
+    await new Promise((r) => setTimeout(r, 50));
+  }
+}
+
+async function pidIn(file) {
+  for (let i = 0; i < 100 && !existsSync(file); i++) await new Promise((r) => setTimeout(r, 20));
+  return Number(readFileSync(file, "utf8").trim());
+}
+
+const kill = (pid) => {
+  try {
+    process.kill(pid, "SIGKILL");
+  } catch {}
+};
+
+// git ignores SIGTERM and waits on a sleep it started. SIGTERM goes to the
+// group, so the sleep dies at the timeout; git itself lasts until SIGKILL.
+test("a runner git past its time: SIGTERM reaches its process group, and SIGKILL follows after the grace period", { timeout: 20000 }, async (t) => {
+  const dir = scratch(t);
+  const file = join(dir, "child.pid");
+  const env = fakeGit(dir, `sleep 30 </dev/null >/dev/null 2>&1 &\necho $! > '${file}'\ntrap '' TERM\nwait $!\nexec sleep 30`);
+  const started = Date.now();
+  const run = runnerGit(["fetch"], env, { timeoutMs: 300, graceMs: 1500 });
+  const pid = await pidIn(file);
+  t.after(() => kill(pid));
+  assert.equal(await gone(pid, 800), true, "the sleep git started died at SIGTERM");
+  assert.ok(Date.now() - started < 1500, "before the grace period was over");
+  const r = await run;
+  assert.equal(r.why, "killed by SIGKILL");
+  assert.ok(Date.now() - started >= 1800, "SIGKILL waited out the grace period");
+});
+
+// git dies at SIGTERM; a process it started ignores SIGTERM and holds none
+// of git's output. Once git has closed, its group gets SIGKILL.
+test("once a signalled runner git has closed, its group gets SIGKILL, so a process that ignored SIGTERM dies too", { timeout: 20000 }, async (t) => {
+  const dir = scratch(t);
+  const file = join(dir, "child.pid");
+  const env = fakeGit(dir, `(trap '' TERM; exec sleep 30) </dev/null >/dev/null 2>&1 &\necho $! > '${file}'\nexec sleep 30`);
+  const r = await runnerGit(["fetch"], env, { timeoutMs: 300, graceMs: 20000 });
+  assert.equal(r.why, "killed by SIGTERM");
+  const pid = await pidIn(file);
+  t.after(() => kill(pid));
+  assert.equal(await gone(pid, 1500), true, "the sleep that ignored SIGTERM is gone well inside the grace period");
+});