fix(#5978,#5999): unique temp names for link-pass atomic writes; cancel heavy batch children by process group

cmd/grafel/daemon.go

@@ -1202,8 +1202,8 @@ var subprocessLinksRunner = sched.RunSubprocessLinks
 // CANCELLATION. ctx is the scheduler's per-group cancel context (derived from
 // shutdownCtx), so daemon shutdown or a CancelGroup on a group delete still
 // stops the pass; across the fork the in-process ctx checks become a SIGKILL of
-// the child (exec.CommandContext's default Cancel is cmd.Process.Kill — not a
-// SIGTERM). More prompt than the boundary checks it replaces, at the cost of
+// the child's whole process group (#5999 — not a SIGTERM, and not the
+// single-pid kill os/exec would do by default). More prompt than the boundary checks it replaces, at the cost of
 // leaking the pass's staging temp dir, which stageGraphsDir sweeps.
 func daemonSchedulerLinks(ctx context.Context, group string) error {
 	if sched.SubprocessIndexEnabled() {

cmd/grafel/links_internal.go

@@ -62,8 +62,8 @@ func runLinksInternal(args []string) int {
 	defer memSampler.Stop()
 
 	// context.Background(), deliberately: cancellation of this child is a
-	// SIGKILL from the parent (exec.CommandContext's default Cancel is
-	// cmd.Process.Kill), not an in-process ctx. A signal handler is therefore
+	// SIGKILL from the parent, delivered to this process's whole process group
+	// (#5999), not an in-process ctx. A signal handler is therefore
 	// not an option — SIGKILL cannot be caught — and none is wanted: the kill
 	// is more prompt than the ctx checks it replaces, which only took effect at
 	// a pass boundary. The pass's writes are temp+rename so a kill cannot tear

internal/cli/links.go

@@ -329,7 +329,7 @@ const stagingDirMaxAge = 24 * time.Hour
 // sweepStaleStagingDirs removes abandoned staging dirs from previous passes.
 //
 // #5954: the scheduler-driven pass now runs in a child that the daemon cancels
-// with SIGKILL (exec.CommandContext's default Cancel is cmd.Process.Kill), so
+// with a SIGKILL of the child's whole process group (#5999), so
 // the `defer cleanup()` below cannot run on cancellation. That is a genuine
 // regression against the in-process path, where a CancelGroup unwound the pass
 // cooperatively and the cleanup ran; a group deleted or a daemon stopped

internal/daemon/sched/cancel_processgroup_5999_test.go

@@ -0,0 +1,139 @@
+//go:build !windows
+
+package sched
+
+// cancel_processgroup_5999_test.go — cancellation must kill the child's whole
+// process GROUP, not just its pid (#5999).
+//
+// The heavy batch children are spawned with Setpgid (applyGroupAlgoNice), so a
+// process group already exists around each of them. exec.CommandContext's
+// DEFAULT Cancel is cmd.Process.Kill() — a single-pid SIGKILL — which leaves
+// any grandchild the child forked alive. That is not a test artifact: the index
+// child fans out `grafel extract` subprocesses and the link child shells out
+// too, so a cancelled pass could leave live grandchildren behind holding the
+// inherited stdout/stderr pipes. The runner drains those pipes to EOF before
+// cmd.Wait(), so a surviving grandchild also wedges the runner itself: the call
+// never returns, and the stage token it is holding is never released.
+//
+// The stand-in child below makes that deterministic rather than flaky: it forks
+// a long `sleep` that inherits the pipes and records its pid, then blocks.
+
+import (
+	"context"
+	"os"
+	"path/filepath"
+	"strconv"
+	"strings"
+	"syscall"
+	"testing"
+	"time"
+)
+
+// fakeGroupAlgoBinary points the group-algo runner at a shell-script stand-in,
+// the group-algo analogue of fakeChildScript.
+func fakeGroupAlgoBinary(t *testing.T, body string) {
+	t.Helper()
+	path := filepath.Join(t.TempDir(), "fake-group-algo-child.sh")
+	if err := os.WriteFile(path, []byte("#!/bin/sh\n"+body+"\n"), 0o755); err != nil {
+		t.Fatalf("write fake group-algo child: %v", err)
+	}
+	prev := groupAlgoChildBinary
+	groupAlgoChildBinary = func() (string, error) { return path, nil }
+	t.Cleanup(func() { groupAlgoChildBinary = prev })
+}
+
+// pidIsAlive reports whether pid still exists (signal 0 probe).
+func pidIsAlive(pid int) bool {
+	return syscall.Kill(pid, 0) == nil
+}
+
+func waitPidGone(pid int, d time.Duration) bool {
+	deadline := time.Now().Add(d)
+	for time.Now().Before(deadline) {
+		if !pidIsAlive(pid) {
+			return true
+		}
+		time.Sleep(20 * time.Millisecond)
+	}
+	return !pidIsAlive(pid)
+}
+
+// readPidFile waits for the stand-in child's grandchild to record its pid, and
+// registers a cleanup that kills it. The cleanup matters on the FAILURE path:
+// when the group kill does not bind, every failing run would otherwise leave a
+// live `sleep` behind, and a mutation sweep leaves a pile of them on the box.
+func readPidFile(t *testing.T, path string) int {
+	t.Helper()
+	deadline := time.Now().Add(5 * time.Second)
+	for time.Now().Before(deadline) {
+		if b, err := os.ReadFile(path); err == nil {
+			if pid, err := strconv.Atoi(strings.TrimSpace(string(b))); err == nil && pid > 0 {
+				t.Cleanup(func() { _ = syscall.Kill(pid, syscall.SIGKILL) })
+				return pid
+			}
+		}
+		time.Sleep(20 * time.Millisecond)
+	}
+	t.Fatalf("grandchild never recorded its pid in %s", path)
+	return 0
+}
+
+// TestRunSubprocessLinks_CancelKillsGrandchildren is the deterministic form of
+// the flaky #5999 failure: with a single-pid kill the backgrounded `sleep`
+// survives, keeps the inherited stdout pipe open, and the runner blocks in its
+// drain loop past the assertion window.
+func TestRunSubprocessLinks_CancelKillsGrandchildren(t *testing.T) {
+	pidFile := filepath.Join(t.TempDir(), "grandchild.pid")
+	fakeChildScript(t, "sleep 60 &\necho $! > "+pidFile+"\nsleep 60")
+
+	ctx, cancel := context.WithCancel(context.Background())
+	done := make(chan error, 1)
+	go func() { done <- RunSubprocessLinks(ctx, "g", nil) }()
+
+	pid := readPidFile(t, pidFile)
+	cancel()
+
+	select {
+	case err := <-done:
+		if err == nil {
+			t.Fatal("cancelled run returned nil error, want a cancellation error")
+		}
+	case <-time.After(5 * time.Second):
+		t.Fatalf("runner still blocked 5s after cancel — grandchild pid %d alive=%v holding the inherited pipes",
+			pid, pidIsAlive(pid))
+	}
+
+	if !waitPidGone(pid, 2*time.Second) {
+		t.Fatalf("grandchild pid %d survived cancellation — the kill did not reach the process group", pid)
+	}
+}
+
+// TestRunSubprocessGroupAlgo_CancelKillsGrandchildren pins the same contract on
+// the other Setpgid child. group-algo does not fan out today, but it is spawned
+// through the same hook, and a supervisor whose cancellation reaches only the
+// direct child is the defect — not the particular argv it happens to run.
+func TestRunSubprocessGroupAlgo_CancelKillsGrandchildren(t *testing.T) {
+	pidFile := filepath.Join(t.TempDir(), "grandchild.pid")
+	fakeGroupAlgoBinary(t, "sleep 60 &\necho $! > "+pidFile+"\nsleep 60")
+
+	ctx, cancel := context.WithCancel(context.Background())
+	done := make(chan error, 1)
+	go func() { done <- RunSubprocessGroupAlgo(ctx, "g", nil) }()
+
+	pid := readPidFile(t, pidFile)
+	cancel()
+
+	select {
+	case err := <-done:
+		if err == nil {
+			t.Fatal("cancelled group-algo returned nil error, want a cancellation error")
+		}
+	case <-time.After(5 * time.Second):
+		t.Fatalf("group-algo runner still blocked 5s after cancel — grandchild pid %d alive=%v",
+			pid, pidIsAlive(pid))
+	}
+
+	if !waitPidGone(pid, 2*time.Second) {
+		t.Fatalf("group-algo grandchild pid %d survived cancellation — the kill did not reach the process group", pid)
+	}
+}

internal/daemon/sched/nice_unix.go

@@ -3,6 +3,7 @@
 package sched
 
 import (
+	"os"
 	"os/exec"
 	"strconv"
 	"syscall"
@@ -22,11 +23,56 @@ const groupAlgoNice = 10
 // startup (see niceSelf), because Go's exec package does not expose a portable
 // "spawn at nice N" knob; here we only ensure the child is independently
 // schedulable. Kept as a hook so the spawn site stays platform-agnostic.
+//
+// It also wires cmd.Cancel so cancellation reaches that whole process group —
+// see applyProcessGroupCancel. The two belong together: the moment a child gets
+// its own group, the default single-pid kill stops covering everything the
+// child spawned, so no caller of this hook can opt into one without the other.
 func applyGroupAlgoNice(cmd *exec.Cmd) {
 	if cmd.SysProcAttr == nil {
 		cmd.SysProcAttr = &syscall.SysProcAttr{}
 	}
 	cmd.SysProcAttr.Setpgid = true
+	applyProcessGroupCancel(cmd)
+}
+
+// applyProcessGroupCancel makes ctx cancellation SIGKILL the child's entire
+// process GROUP instead of its pid alone (#5999).
+//
+// exec.CommandContext's default Cancel is cmd.Process.Kill(): one pid, one
+// SIGKILL. Because Setpgid above already gave the child its own group (pgid ==
+// the child's pid), every process the child forks lands in that same group, and
+// the default kill leaves all of them running. Two consequences, both real:
+// the index child fans out `grafel extract` subprocesses, so a cancelled pass
+// leaked live grandchildren; and those grandchildren inherit the child's
+// stdout/stderr pipes, so the runner's drain-to-EOF loop never finishes and the
+// supervising call never returns — with the daemon's exclusive heavy-stage
+// token still held.
+//
+// Must be called only on a cmd that carries Setpgid. kill(-pid) addresses the
+// group whose PGID equals that pid — NOT the caller's own group — so without
+// Setpgid the child leads no group, the kill returns ESRCH, and the fallback
+// below quietly papers over it: the group kill would never actually bind. (The
+// residual hazard is narrower but real: after pid reuse, that pid could have
+// become the leader of an unrelated group, and the signal would land there.)
+// That is why the only caller is applyGroupAlgoNice, which sets Setpgid on the
+// same cmd — the pairing must not be split.
+//
+// Setting Cancel on a cmd built without a context is harmless: os/exec only
+// consults it when a context is present.
+func applyProcessGroupCancel(cmd *exec.Cmd) {
+	cmd.Cancel = func() error {
+		if cmd.Process == nil {
+			return os.ErrProcessDone
+		}
+		// Negative pid == "the process group led by that pid".
+		if err := syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL); err != nil {
+			// The group may already be gone, or Setpgid may not have taken;
+			// fall back to the single-pid kill we are replacing.
+			return cmd.Process.Kill()
+		}
+		return nil
+	}
 }
 
 // NiceSelf lowers the CURRENT process's OS scheduling priority by

internal/daemon/sched/nice_windows.go

@@ -12,6 +12,13 @@ import "os/exec"
 const groupAlgoNice = 0
 
 // applyGroupAlgoNice is a no-op on Windows.
+//
+// It is also where the #5999 process-group kill lives on Unix. Windows has no
+// setpgid and no signals, so there is nothing equivalent to wire here: cmd.Cancel
+// is left at the os/exec default (cmd.Process.Kill(), which terminates the child
+// alone). Killing a whole tree on Windows needs a Job object around the spawn,
+// which this hook deliberately does not attempt — so on Windows a cancelled
+// child's grandchildren still outlive it.
 func applyGroupAlgoNice(cmd *exec.Cmd) {}
 
 // NiceSelf is a no-op on Windows.

internal/daemon/sched/subprocess_runner.go

@@ -403,9 +403,11 @@ type ipcEvent struct {
 // repo basename) so the daemon log file includes child extractor output
 // without growing the daemon's own heap.
 //
-// Cancellation: ctx.Done() sends SIGTERM to the child. The child is
-// expected to exit on SIGTERM; if it does not, the parent waits and the
-// context timeout (if any) will eventually unblock the caller.
+// Cancellation: ctx.Done() SIGKILLs the child's whole process group — the
+// child plus every `grafel extract` subprocess its coordinator forked (#5999).
+// It is NOT a SIGTERM: an earlier version of this comment said so and was
+// simply wrong, and nothing on this path may depend on the child getting a
+// chance to run deferred cleanup.
 func RunSubprocessIndex(ctx context.Context, repoPath, ref string, skipPasses []string, opts *SubprocessIndexOptions, logger *slog.Logger) error {
 	binary, err := os.Executable()
 	if err != nil {
@@ -487,6 +489,15 @@ func RunSubprocessIndex(ctx context.Context, repoPath, ref string, skipPasses []
 	if logger != nil {
 		logger.Info("subprocess-indexer: "+reason, "gomaxprocs", gmp, "repo", repoPath)
 	}
+	// Own process group + group-wide kill on cancellation (#5999). This child is
+	// the one that actually fans out: the extract coordinator forks a `grafel
+	// extract` subprocess per batch. Under the os/exec default Cancel — a
+	// single-pid SIGKILL — every one of those extract processes survived the
+	// cancellation, kept the inherited stderr pipe open, and so also kept this
+	// runner blocked in its drain loop. Both call sites are inside the daemon
+	// (no controlling terminal), so moving the child out of the daemon's process
+	// group costs no signal delivery that anything relies on.
+	applyGroupAlgoNice(cmd)
 	// On Windows, prevent a console window from flashing when the daemon
 	// (running as a Task Scheduler task) spawns this subprocess.
 	executil.NoWindow(cmd)
@@ -573,9 +584,9 @@ func RunSubprocessIndex(ctx context.Context, repoPath, ref string, skipPasses []
 // MCP apply path picks up the fresh overlay by mtime on the next group load.
 //
 // Cancellation: ctx.Done() (daemon shutdown, or a newer link pass superseding
-// this one) SIGKILLs the child. exec.CommandContext's default Cancel is
-// cmd.Process.Kill(), not a SIGTERM — an earlier version of this comment said
-// SIGTERM and was simply wrong. The child gets no chance to run deferred
+// this one) SIGKILLs the child's process group — see applyProcessGroupCancel,
+// wired by applyGroupAlgoNice below. It is not a SIGTERM: an earlier version of
+// this comment said SIGTERM and was simply wrong. The child gets no chance to run deferred
 // cleanup, so nothing on the group-algo path may depend on a graceful shutdown;
 // the overlay write is a temp+rename swap, so a kill mid-write leaves an orphan
 // temp file and never a torn overlay.
@@ -607,8 +618,14 @@ func groupAlgoChildEnv(base []string, gomaxprocs int, foreground bool) []string
 	return withMadvDontNeed(env)
 }
 
+// groupAlgoChildBinary resolves the executable to fork for the group-algo
+// child. A var for the same reason as linksChildBinary: a test can substitute a
+// stand-in and exercise the fork / cancel contract without running the real
+// pass.
+var groupAlgoChildBinary = os.Executable
+
 func RunSubprocessGroupAlgo(ctx context.Context, group string, logger *slog.Logger) error {
-	binary, err := os.Executable()
+	binary, err := groupAlgoChildBinary()
 	if err != nil {
 		return fmt.Errorf("subprocess-group-algo: resolve binary: %w", err)
 	}
@@ -771,16 +788,19 @@ var linksChildBinary = os.Executable
 // SetRepoSourcePaths is written before use on every entry, so the child's fresh
 // globals are not a hazard — they are one less thing to invalidate.
 //
-// CONCURRENCY around the known #5978 hazard (candidates.go / string_pass.go
-// build temp files as path+".tmp", deterministic per destination, so two
-// concurrent writers collide): this change does NOT make it worse. The pass is
+// CONCURRENCY: the pass's temp files are now uniquely named per writer
+// (#5978 — links.writeFileAtomic), so two concurrent writers to one
+// destination no longer collide. Independently of that, the pass is
 // serialised by the daemon's EXCLUSIVE heavy-stage token, which is held across
 // this call for the child's whole lifetime, so at most one background link pass
 // exists at a time — the same guarantee the in-process path had.
 //
 // CANCELLATION IS SIGKILL, NOT SIGTERM. ctx.Done() (daemon shutdown, or
-// CancelGroup on a group delete) makes exec.CommandContext run its default
-// Cancel, which is cmd.Process.Kill() — see os/exec. That is more prompt than
+// CancelGroup on a group delete) SIGKILLs the child's whole process GROUP:
+// applyGroupAlgoNice below sets Setpgid and overrides cmd.Cancel accordingly
+// (#5999), replacing os/exec's default single-pid cmd.Process.Kill() — which
+// left anything the child forked alive, holding this runner's pipes open past
+// the child's own death. That is more prompt than
 // the in-process ctx checks it replaces (those only took effect at a pass
 // boundary, which on this pass can be minutes away), and it is why the child
 // installs no signal handler: under SIGKILL no handler could run.