diff --git a/.gds/bundle.lock.yaml b/.gds/bundle.lock.yaml index 8825401..e2d6d0e 100644 --- a/.gds/bundle.lock.yaml +++ b/.gds/bundle.lock.yaml @@ -5,14 +5,14 @@ bundle: version: "0.8.0-dev" release_sequence: 0 channel: "development" - source_tree_digest: "sha256:d76906627f908b796164bf3070579800f5087462335823f6524ff918862e991a" - digest: "sha256:71793f34bbb38088800964c34a2c2a0f0ba02a0b725620db9d370c81ff4a9496" + source_tree_digest: "sha256:f650570277313a15cfc92ea1a12cd6f5c9b8af7ea410cffd5e8931ca8d01c70c" + digest: "sha256:7c44e6973edfd5c73f73b9e6d414b263ceee36faaa17072d82bb2bc40b6b6beb" projection: - input_digest: "sha256:5b4f5697a87f13d38bedc7ae5b37a36de65b08c2cd4986358f526f90cb005e04" - output_digest: "sha256:186592ce7518765ffa6c604a16740c827ad8f6ce1f0eb6aa4f6822dafe028321" + input_digest: "sha256:9f629a827361728b8ccb873792881b41b4a396ff9a0f73f25d97e47c26e2a3fc" + output_digest: "sha256:eded8fc365dbce57f899de267242c2ee796552f5cb509046af4dbf9eb7dbf701" files: - path: ".gds/compiled-policy.json" digest: "sha256:807282f820294914e1c7e6ad1bf27c54a799d56305c58630254ab50ab286f379" - path: ".github/workflows/gds-ci.yml" - digest: "sha256:28e93787aeee67af6e43b237224ec5444ea01dcf7e87079cf910d7acc2de225e" + digest: "sha256:4b482bb6bc7b3eef9ba5e2a2bf09c955ef6d6213fea8485c8586dbc79328511f" diff --git a/.github/workflows/gds-ci.yml b/.github/workflows/gds-ci.yml index b9bf022..cadbce6 100644 --- a/.github/workflows/gds-ci.yml +++ b/.github/workflows/gds-ci.yml @@ -1,8 +1,8 @@ # GENERATED FILE - DO NOT EDIT DIRECTLY # generator: gds # bundle: 0.8.0-dev -# source-tree-digest: sha256:d76906627f908b796164bf3070579800f5087462335823f6524ff918862e991a -# input-digest: sha256:5b4f5697a87f13d38bedc7ae5b37a36de65b08c2cd4986358f526f90cb005e04 +# source-tree-digest: sha256:f650570277313a15cfc92ea1a12cd6f5c9b8af7ea410cffd5e8931ca8d01c70c +# input-digest: sha256:9f629a827361728b8ccb873792881b41b4a396ff9a0f73f25d97e47c26e2a3fc # output-digest: sha256:8c045e745cc69b731bc695a4a9d58a48c10f1ab7dd85b7354db7bfd0e072711c # edit-source: # - .gds/repository.yaml diff --git a/core/app/module_process_darwin.go b/core/app/module_process_darwin.go new file mode 100644 index 0000000..6facba8 --- /dev/null +++ b/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 +} diff --git a/core/app/module_process_linux.go b/core/app/module_process_linux.go new file mode 100644 index 0000000..fd5cc0f --- /dev/null +++ b/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 +} diff --git a/core/app/module_process_other.go b/core/app/module_process_other.go new file mode 100644 index 0000000..2455b40 --- /dev/null +++ b/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") +} diff --git a/core/app/module_process_unix.go b/core/app/module_process_unix.go new file mode 100644 index 0000000..78c6c82 --- /dev/null +++ b/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 +} diff --git a/core/app/module_process_unix_test.go b/core/app/module_process_unix_test.go new file mode 100644 index 0000000..2016945 --- /dev/null +++ b/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 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) + } +} diff --git a/core/app/module_verify.go b/core/app/module_verify.go index 3f7be24..76d26e3 100644 --- a/core/app/module_verify.go +++ 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,9 +227,7 @@ 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{}} @@ -216,6 +235,14 @@ func (services *Services) runModuleLanes( 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()) } diff --git a/core/app/module_verify_lanes_test.go b/core/app/module_verify_lanes_test.go new file mode 100644 index 0000000..8c7d540 --- /dev/null +++ b/core/app/module_verify_lanes_test.go @@ -0,0 +1,140 @@ +package app + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" + "time" + + moduleworkflow "github.com/NDDev-OpenNetwork/github-device-sync/core/module" + gitprovider "github.com/NDDev-OpenNetwork/github-device-sync/core/providers/git" +) + +func TestCleanupPendingStopsLaterLanesInSource(t *testing.T) { + t.Parallel() + source := moduleVerifySource(t) + for _, need := range []string{ + "if result.CleanupPending {", + "preserve = true", + "no later lane was started", + "return report, findings", + } { + if !strings.Contains(source, need) { + t.Fatalf("missing %q", need) + } + } +} + +func TestFailedRemoveWorktreeDoesNotDeleteWorkspace(t *testing.T) { + root, oid := moduleVerifyRepository(t) + runner, err := gitprovider.NewMutationRunner() + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + _ = os.Chmod(filepath.Join(root, ".git"), 0o700) + }) + services := &Services{GitMutations: runner} + plan := moduleworkflow.VerificationPlan{ + GitmodulesName: "example", + Path: "modules/example", + GitlinkOID: oid, + Lanes: []moduleworkflow.LaneSelection{ + {Lane: "lint", Commands: []string{`chmod 000 "$(git rev-parse --git-common-dir)"`}}, + }, + } + report, findings := services.runModuleLanes(context.Background(), root, plan, 30*time.Second) + if len(report.Lanes) != 1 || len(report.Lanes[0].Commands) != 1 || report.Lanes[0].Commands[0].Status != "passed" { + t.Fatalf("command=%#v", report) + } + foundCleanup := false + for _, finding := range findings { + if finding.Code == "GDS_MODULE_VERIFICATION_CLEANUP_NOT_PROVEN" { + foundCleanup = true + } + } + if !foundCleanup { + t.Fatalf("cleanup finding missing: %#v", findings) + } +} + +func TestFailedRemoveWorktreeSourceContract(t *testing.T) { + t.Parallel() + source := moduleVerifySource(t) + if !strings.Contains(source, "GDS_MODULE_VERIFICATION_CLEANUP_NOT_PROVEN") || + !strings.Contains(source, "if cleanupErr == nil") || + !strings.Contains(source, "os.RemoveAll(workspace)") { + t.Fatal("failed RemoveWorktree no longer blocks workspace deletion") + } +} + +func TestUnsupportedOSProcessOwnershipFailsClosed(t *testing.T) { + t.Parallel() + raw, err := os.ReadFile(filepath.Join(moduleVerifyDir(t), "module_process_other.go")) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(raw), "supported only on Linux and macOS") { + t.Fatal("unsupported OS no longer fails closed") + } +} + +func moduleVerifyDir(t *testing.T) string { + t.Helper() + _, current, _, ok := runtime.Caller(0) + if !ok { + t.Fatal("runtime.Caller failed") + } + return filepath.Dir(current) +} + +func moduleVerifySource(t *testing.T) string { + t.Helper() + raw, err := os.ReadFile(filepath.Join(moduleVerifyDir(t), "module_verify.go")) + if err != nil { + t.Fatal(err) + } + return string(raw) +} + +func moduleVerifyRepository(t *testing.T) (string, string) { + t.Helper() + root := t.TempDir() + commands := [][]string{ + {"init", "-q"}, + {"config", "user.name", "GDS Test"}, + {"config", "user.email", "gds@example.invalid"}, + } + for _, arguments := range commands { + command := exec.Command("git", arguments...) + command.Dir = root + if output, err := command.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v: %s", arguments, err, output) + } + } + if err := os.WriteFile(filepath.Join(root, "fixture.txt"), []byte("fixture\n"), 0o644); err != nil { + t.Fatal(err) + } + for _, arguments := range [][]string{{"add", "fixture.txt"}, {"commit", "-qm", "fixture"}} { + command := exec.Command("git", arguments...) + command.Dir = root + if output, err := command.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v: %s", arguments, err, output) + } + } + command := exec.Command("git", "rev-parse", "HEAD") + command.Dir = root + output, err := command.Output() + if err != nil { + t.Fatal(err) + } + oid := strings.TrimSpace(string(output)) + if len(oid) != 40 { + t.Fatalf("fixture OID %q is not a 40-character object name", oid) + } + return root, oid +}