fix: own module verification process groups through cleanup

Setpgid plus TERM/KILL with a bounded wait, preserve the checkout when
cleanup is not proven, and skip later lanes so a leftover writer cannot
be reported as a passing verification.

Signed-off-by: rldyourmnd <danil@nddev.it.com>
Co-authored-by: Cursor <cursoragent@cursor.com>

Author: rldyourmnd

core/app/module_process_darwin.go

@@ -0,0 +1,16 @@
+//go:build darwin
+
+package app
+
+import (
+	"errors"
+	"syscall"
+)
+
+func moduleProcessGroupRunning(group int) (bool, error) {
+	err := syscall.Kill(-group, 0)
+	if errors.Is(err, syscall.ESRCH) {
+		return false, nil
+	}
+	return true, err
+}

core/app/module_process_linux.go

@@ -0,0 +1,55 @@
+//go:build linux
+
+package app
+
+import (
+	"errors"
+	"fmt"
+	"os"
+	"path/filepath"
+	"strconv"
+	"strings"
+	"syscall"
+)
+
+func moduleProcessGroupRunning(group int) (bool, error) {
+	if err := syscall.Kill(-group, 0); errors.Is(err, syscall.ESRCH) {
+		return false, nil
+	} else if err != nil {
+		return true, err
+	}
+	// Killed orphans may await init's reaping. Zombies cannot execute or write
+	// the checkout; do not mistake their remaining group membership for work.
+	entries, err := os.ReadDir("/proc")
+	if err != nil {
+		return true, err
+	}
+	for _, entry := range entries {
+		if _, err := strconv.Atoi(entry.Name()); err != nil {
+			continue
+		}
+		raw, err := os.ReadFile(filepath.Join("/proc", entry.Name(), "stat"))
+		if os.IsNotExist(err) {
+			continue
+		}
+		if err != nil {
+			return true, fmt.Errorf("inspect process group: %w", err)
+		}
+		end := strings.LastIndexByte(string(raw), ')')
+		if end < 0 {
+			return true, errors.New("process status is incomplete")
+		}
+		fields := strings.Fields(string(raw)[end+1:])
+		if len(fields) < 3 {
+			return true, errors.New("process status is incomplete")
+		}
+		pgid, err := strconv.Atoi(fields[2])
+		if err != nil {
+			return true, err
+		}
+		if pgid == group && fields[0] != "Z" && fields[0] != "X" {
+			return true, nil
+		}
+	}
+	return false, nil
+}

core/app/module_process_other.go

@@ -0,0 +1,12 @@
+//go:build !darwin && !linux
+
+package app
+
+import (
+	"errors"
+	"os/exec"
+)
+
+func configureModuleProcess(_ *exec.Cmd) (func() (bool, error), error) {
+	return nil, errors.New("module command process ownership is supported only on Linux and macOS")
+}

core/app/module_process_unix.go

@@ -0,0 +1,77 @@
+//go:build darwin || linux
+
+package app
+
+import (
+	"errors"
+	"fmt"
+	"os"
+	"os/exec"
+	"sync"
+	"syscall"
+	"time"
+)
+
+const moduleTerminationGrace = 250 * time.Millisecond
+const moduleTerminationWait = 2 * time.Second
+
+// Own a new group, never the caller's group. Cancel and normal-exit cleanup
+// share one synchronous operation, so no delayed signal goroutine outlives the
+// command or its verification workspace.
+func configureModuleProcess(command *exec.Cmd) (func() (bool, error), error) {
+	command.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
+	var once sync.Once
+	var present bool
+	var stopErr error
+	stop := func() (bool, error) {
+		once.Do(func() {
+			if command.Process == nil {
+				return
+			}
+			pid := command.Process.Pid
+			if pid <= 1 || pid == syscall.Getpgrp() {
+				stopErr = errors.New("refusing to signal an unowned process group")
+				return
+			}
+			err := syscall.Kill(-pid, syscall.SIGTERM)
+			if errors.Is(err, syscall.ESRCH) {
+				return
+			}
+			present = true
+			if err != nil {
+				stopErr = fmt.Errorf("terminate module process group: %w", err)
+				return
+			}
+			time.Sleep(moduleTerminationGrace)
+			if err := syscall.Kill(-pid, syscall.SIGKILL); err != nil && !errors.Is(err, syscall.ESRCH) {
+				stopErr = fmt.Errorf("kill module process group: %w", err)
+				return
+			}
+			deadline := time.Now().Add(moduleTerminationWait)
+			for {
+				running, err := moduleProcessGroupRunning(pid)
+				if err != nil {
+					stopErr = err
+					return
+				}
+				if !running {
+					return
+				}
+				if time.Now().After(deadline) {
+					stopErr = errors.New("module process group did not stop within cleanup deadline")
+					return
+				}
+				time.Sleep(10 * time.Millisecond)
+			}
+		})
+		return present, stopErr
+	}
+	command.Cancel = func() error {
+		present, err := stop()
+		if !present && err == nil {
+			return os.ErrProcessDone
+		}
+		return err
+	}
+	return stop, nil
+}

core/app/module_process_unix_test.go

@@ -0,0 +1,110 @@
+//go:build darwin || linux
+
+package app
+
+import (
+	"context"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strconv"
+	"strings"
+	"testing"
+	"time"
+)
+
+// Run the actual shell/pipe/descendant path. A bounded Wait on the shell alone
+// cannot prove that its children stopped touching the verification checkout.
+func TestDeclaredTimeoutStopsStubbornDescendants(t *testing.T) {
+	dir := t.TempDir()
+	worker := "trap '' TERM\necho $$ > child.pid\nwhile :; do echo tick >> heartbeat; sleep 0.03; done\n"
+	if err := os.WriteFile(filepath.Join(dir, "worker.sh"), []byte(worker), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	report := runDeclaredCommand(context.Background(), dir, "bash worker.sh & wait", 350*time.Millisecond)
+	pid := readOwnedTestChild(t, dir)
+	defer stopOwnedTestChild(pid)
+	if report.Status != "timeout" {
+		t.Fatalf("report=%#v", report)
+	}
+	assertTestChildStopped(t, pid)
+	before, err := os.ReadFile(filepath.Join(dir, "heartbeat"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	time.Sleep(120 * time.Millisecond)
+	after, err := os.ReadFile(filepath.Join(dir, "heartbeat"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if string(before) != string(after) {
+		t.Fatal("descendant wrote after command completion")
+	}
+}
+
+func TestDeclaredCancellationStopsPipelineChildren(t *testing.T) {
+	dir := t.TempDir()
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+	done := make(chan CommandReport, 1)
+	go func() {
+		done <- runDeclaredCommand(ctx, dir, "sleep 20 & echo $! > child.pid; wait | cat", 30*time.Second)
+	}()
+	pid := readOwnedTestChild(t, dir)
+	defer stopOwnedTestChild(pid)
+	cancel()
+	select {
+	case report := <-done:
+		if report.Status == "passed" {
+			t.Fatalf("cancellation passed: %#v", report)
+		}
+	case <-time.After(6 * time.Second):
+		t.Fatal("cancellation did not settle")
+	}
+	assertTestChildStopped(t, pid)
+}
+
+func TestDeclaredSuccessCannotLeaveBackgroundWriter(t *testing.T) {
+	dir := t.TempDir()
+	report := runDeclaredCommand(context.Background(), dir,
+		"sleep 20 </dev/null >/dev/null 2>&1 & echo $! > child.pid", time.Second)
+	pid := readOwnedTestChild(t, dir)
+	defer stopOwnedTestChild(pid)
+	if report.Status == "passed" {
+		t.Error("unjoined background process was reported as completed work")
+	}
+	assertTestChildStopped(t, pid)
+}
+
+func readOwnedTestChild(t *testing.T, dir string) int {
+	t.Helper()
+	deadline := time.Now().Add(3 * time.Second)
+	for time.Now().Before(deadline) {
+		b, err := os.ReadFile(filepath.Join(dir, "child.pid"))
+		if err == nil {
+			pid, err := strconv.Atoi(strings.TrimSpace(string(b)))
+			if err == nil && pid > 1 {
+				return pid
+			}
+		}
+		time.Sleep(10 * time.Millisecond)
+	}
+	t.Fatal("test child did not start")
+	return 0
+}
+
+func stopOwnedTestChild(pid int) {
+	if p, err := os.FindProcess(pid); err == nil {
+		_ = p.Kill()
+	}
+}
+
+func assertTestChildStopped(t *testing.T, pid int) {
+	t.Helper()
+	b, err := exec.Command("ps", "-o", "stat=", "-p", strconv.Itoa(pid)).Output()
+	// An unreaped zombie has already stopped executing; reaping belongs to its
+	// parent/init. It cannot continue writing into the verification workspace.
+	if err == nil && strings.TrimSpace(string(b)) != "" && !strings.HasPrefix(strings.TrimSpace(string(b)), "Z") {
+		t.Fatalf("child %d still executes after command returned: %s", pid, b)
+	}
+}

core/app/module_verify.go

@@ -51,6 +51,8 @@ type CommandReport struct {
 	// "No module named pytest" is not a broken module, and a reader must be able
 	// to see that without rerunning anything.
 	Diagnostic string `json:"diagnostic,omitempty"`
+	// A failed cleanup must not be followed by workspace deletion or another lane.
+	CleanupPending bool `json:"cleanup_pending,omitempty"`
 }
 
 const defaultModuleCommandTimeout = 10 * time.Minute
@@ -176,13 +178,13 @@ func (services *Services) runModuleLanes(
 	modulePath string,
 	plan moduleworkflow.VerificationPlan,
 	timeout time.Duration,
-) (ModuleVerification, []domain.Finding) {
-	report := ModuleVerification{
+) (report ModuleVerification, findings []domain.Finding) {
+	report = ModuleVerification{
 		GitmodulesName: plan.GitmodulesName, Path: plan.Path,
 		GitlinkOID: plan.GitlinkOID, RepositoryID: plan.RepositoryID,
 		Lanes: []LaneReport{},
 	}
-	findings := []domain.Finding{}
+	findings = []domain.Finding{}
 
 	workspace, err := os.MkdirTemp("", "gds-module-verify-")
 	if err != nil {
@@ -192,8 +194,27 @@ func (services *Services) runModuleLanes(
 			Evidence: map[string]any{"gitmodules_name": plan.GitmodulesName},
 		})
 	}
-	defer os.RemoveAll(workspace)
 	checkout := filepath.Join(workspace, "checkout")
+	registered, preserve := false, false
+	defer func() {
+		if preserve {
+			return
+		}
+		cleanupCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+		defer cancel()
+		var cleanupErr error
+		if registered {
+			cleanupErr = services.GitMutations.RemoveWorktree(cleanupCtx, modulePath, checkout)
+		}
+		if cleanupErr == nil {
+			cleanupErr = os.RemoveAll(workspace)
+		}
+		if cleanupErr != nil {
+			findings = append(findings, domain.Finding{Code: "GDS_MODULE_VERIFICATION_CLEANUP_NOT_PROVEN", Severity: domain.SeverityHigh,
+				Message:  "Verification workspace cleanup failed; retained state requires inspection.",
+				Evidence: map[string]any{"workspace": workspace, "error": cleanupErr.Error()}})
+		}
+	}()
 
 	if err := services.GitMutations.AddDetachedWorktree(
 		ctx, modulePath, checkout, plan.GitlinkOID,
@@ -206,16 +227,22 @@ func (services *Services) runModuleLanes(
 			},
 		})
 	}
-	defer func() {
-		_ = services.GitMutations.RemoveWorktree(ctx, modulePath, checkout)
-	}()
+	registered = true
 
 	for _, lane := range plan.Lanes {
 		laneReport := LaneReport{Lane: lane.Lane, Commands: []CommandReport{}}
 		failed := false
 		for _, declared := range lane.Commands {
 			result := runDeclaredCommand(ctx, checkout, declared, timeout)
 			laneReport.Commands = append(laneReport.Commands, result)
+			if result.CleanupPending {
+				preserve = true
+				report.Lanes = append(report.Lanes, laneReport)
+				findings = append(findings, domain.Finding{Code: "GDS_MODULE_VERIFICATION_CLEANUP_NOT_PROVEN", Severity: domain.SeverityHigh,
+					Message:  "Command descendants may still own the verification workspace; no later lane was started.",
+					Evidence: map[string]any{"workspace": workspace, "command": declared, "diagnostic": result.Diagnostic}})
+				return report, findings
+			}
 			if result.Status == "passed" {
 				continue
 			}
@@ -275,6 +302,10 @@ func runDeclaredCommand(
 	command := exec.CommandContext(bounded, "bash", "-euo", "pipefail", "-c", declared)
 	command.Dir = directory
 	command.Stdin = nil
+	stop, configureErr := configureModuleProcess(command)
+	if configureErr != nil {
+		return CommandReport{Command: declared, Status: "failed", ExitCode: -1, Diagnostic: configureErr.Error()}
+	}
 	// This selector belongs to the controller operation. Module commands prove
 	// their own source checkout, and must not silently select its consumer's
 	// estate. A declared command can still explicitly select an estate itself.
@@ -291,14 +322,23 @@ func runDeclaredCommand(
 	command.Stdout = diagnostic
 	command.Stderr = diagnostic
 	err := command.Run()
+	leftover, cleanupErr := stop()
+	if err == nil && leftover {
+		err = errors.New("declared command exited with unjoined descendants")
+	}
+	err = errors.Join(err, bounded.Err(), cleanupErr)
 	report := CommandReport{
 		Command: declared, Status: "passed",
-		DurationMS: time.Since(started).Milliseconds(),
+		DurationMS:     time.Since(started).Milliseconds(),
+		CleanupPending: cleanupErr != nil,
 	}
 	if err == nil {
 		return report
 	}
 	report.Diagnostic = boundedDiagnostic(diagnostic.String())
+	if cleanupErr != nil {
+		report.Diagnostic = boundedDiagnostic(report.Diagnostic + "\n" + cleanupErr.Error())
+	}
 	if report.Diagnostic == "" {
 		report.Diagnostic = boundedDiagnostic(err.Error())
 	}