Fix/shim sigterm exit window

exec/shim/internal_test.go

@@ -112,7 +112,7 @@ func callMainExitCode(t *testing.T, defs []job.Registrable, req *exec.Request) i
 	t.Setenv(EnvRequestFD, strconv.Itoa(reqDup))
 	t.Setenv(EnvResultFD, strconv.Itoa(resDup))
 
-	return mainExitCode(defs)
+	return mainExitCode(defs, false)
 }
 
 // TestMainExitCode pins the exit-code contract mainExitCode must satisfy:

exec/shim/localfs.go

@@ -142,6 +142,7 @@ func (fs *LocalFS) Create(_ context.Context, bucket, key string) (artifact.Write
 		// already confined to fs.root; nothing here reads attacker input.
 		// Safe to remove: the path came from resolve's containment check, not
 		// from a raw key.
+		// #nosec G703 -- os.CreateTemp generated this name inside a directory resolve already confined to fs.root.
 		_ = os.Remove(tmp.Name())
 
 		return nil, fmt.Errorf("shim: create %s/%s: %w", bucket, key, cherr)
@@ -237,12 +238,14 @@ func (w *localWriter) Commit(_ context.Context) (artifact.ObjectInfo, error) {
 	// resolve already confined to fs.root; it is not attacker input.
 	if err := w.file.Sync(); err != nil {
 		_ = w.file.Close()
+		// #nosec G703 -- tmpName is this writer's own CreateTemp file, confined to fs.root by resolve.
 		_ = os.Remove(tmpName)
 
 		return artifact.ObjectInfo{}, fmt.Errorf("shim: commit %s/%s: %w", w.bucket, w.key, err)
 	}
 
 	if err := w.file.Close(); err != nil {
+		// #nosec G703 -- tmpName is this writer's own CreateTemp file, confined to fs.root by resolve.
 		_ = os.Remove(tmpName)
 
 		return artifact.ObjectInfo{}, fmt.Errorf("shim: commit %s/%s: %w", w.bucket, w.key, err)
@@ -252,7 +255,9 @@ func (w *localWriter) Commit(_ context.Context) (artifact.ObjectInfo, error) {
 	// (w.final at Create time; tmpName is a sibling CreateTemp made inside
 	// that same, already-contained directory).
 	// Both paths are confined to fs.root by resolve.
+	// #nosec G703 -- both operands passed resolve's containment check; see the comment above.
 	if err := os.Rename(tmpName, w.final); err != nil {
+		// #nosec G703 -- tmpName is this writer's own CreateTemp file, confined to fs.root by resolve.
 		_ = os.Remove(tmpName)
 
 		return artifact.ObjectInfo{}, fmt.Errorf("shim: commit %s/%s: %w", w.bucket, w.key, err)
@@ -280,6 +285,7 @@ func (w *localWriter) Abort() error {
 	// name is this writer's own temp file, created inside the directory
 	// resolve already confined to fs.root at Create time.
 	// name is our own temp file under the resolved, contained directory.
+	// #nosec G703 -- name is this writer's own CreateTemp file, confined to fs.root by resolve.
 	if err := os.Remove(name); err != nil && !os.IsNotExist(err) {
 		return fmt.Errorf("shim: abort %s/%s: %w", w.bucket, w.key, err)
 	}

exec/shim/main.go

@@ -88,7 +88,11 @@ const (
 // clean run without having to inspect the frame first: the exit code
 // corroborates what the Result already says.
 func Main(defs ...job.Registrable) {
-	os.Exit(mainExitCode(defs))
+	// true: this is the real child, so mainExitCode's signal teardown
+	// must leave SIGTERM ignored rather than restore the default. See the
+	// exiting parameter's own comment for why that matters here and not
+	// for the in-process callers that pass false.
+	os.Exit(mainExitCode(defs, true))
 }
 
 // mainExitCode does the real work of Main and returns the process exit
@@ -104,10 +108,12 @@ func Main(defs ...job.Registrable) {
 // err == nil alone cannot tell mainExitCode apart from a clean success.
 // Only StatusHandlerError keeps exit 0; every other non-OK status,
 // including one Run reported without an error, is a nonzero exit.
-func mainExitCode(defs []job.Registrable) int {
+func mainExitCode(defs []job.Registrable, exiting bool) int {
 	// fdFromEnv guarantees a non-negative descriptor, so the uintptr
 	// conversion cannot wrap.
+	// #nosec G115 -- fdFromEnv rejects negatives and both defaults are positive, so neither conversion can wrap.
 	in := os.NewFile(uintptr(fdFromEnv(EnvRequestFD, defaultRequestFD)), "dispatch-exec-request")
+	// #nosec G115 -- fdFromEnv rejects negatives and both defaults are positive, so neither conversion can wrap.
 	out := os.NewFile(uintptr(fdFromEnv(EnvResultFD, defaultResultFD)), "dispatch-exec-result")
 
 	// Applied before anything else touches the request: RLIMIT_CORE in
@@ -140,6 +146,29 @@ func mainExitCode(defs []job.Registrable) int {
 
 	sigCh := make(chan os.Signal, 1)
 	signal.Notify(sigCh, syscall.SIGTERM)
+
+	// Registered before the Stop below so that it runs after it: defers
+	// unwind last-in-first-out, and the order is what this is for.
+	// signal.Stop puts SIGTERM back on its default disposition, which is
+	// fatal, while this process is still alive and still a target. The
+	// parent's kill ladder (subprocess.terminate) signals the whole
+	// process group the moment a deadline fires, and a handler that
+	// cooperated has returned ctx.Err() at that same instant off the
+	// child's own copy of the deadline, so the two events collide by
+	// design. Without this, a SIGTERM landing in the gap between Stop and
+	// the process actually exiting kills a child that had already written
+	// its Result frame and settled on exit 0, and the parent reports
+	// Signal 15 for a handler that shut down cleanly. Measured at roughly
+	// 7% of runs under load before this was added.
+	//
+	// Only the real child does this. An in-process caller (the tests,
+	// which pass exiting false) is not about to exit, and leaving SIGTERM
+	// ignored for the rest of the test binary's life would outlive the
+	// call that set it.
+	if exiting {
+		defer signal.Ignore(syscall.SIGTERM)
+	}
+
 	defer signal.Stop(sigCh)
 
 	// done unblocks the goroutine below once mainExitCode is about to
@@ -367,6 +396,7 @@ func collectOutputs(dir string) ([]exec.OutputFile, error) {
 	// attempt, never a value read out of the untrusted payload the
 	// handler parses.
 	// dir is the request's own OutputDir, not attacker-controlled.
+	// #nosec G703 -- dir is req.OutputDir, chosen by the parent and delivered over the request fd, never handler input.
 	err := filepath.WalkDir(dir, func(path string, d fs.DirEntry, err error) error {
 		if err != nil {
 			return err

exec/subprocess/kill_unix_test.go

@@ -103,7 +103,34 @@ func TestKillLadderKillsTheWholeProcessGroup(t *testing.T) {
 	// that surviving would show up as "still alive," not as "happened to
 	// exit on its own around the same time," see envLongSleep's doc
 	// comment (main_test.go).
-	if kerr := syscall.Kill(pid, 0); kerr != syscall.ESRCH {
+	//
+	// Polled rather than probed once, for the same reason
+	// TestKillLadderReapsAHelperAfterACooperativeLeaderExits polls its own
+	// helper: the grandchild is forked by the fixture, not by this test,
+	// so once the fixture is killed the grandchild is reparented and
+	// reaped by whatever subreaper the OS hands it to, on no schedule
+	// this test is synchronised with. terminate sends the group its
+	// SIGKILL and returns without waiting, so Run can return while the
+	// grandchild is a zombie that has died but not yet been reaped, and
+	// syscall.Kill reports success rather than ESRCH for exactly that
+	// state. A single check right after Run raced the reap on Linux CI
+	// under parallel-package load. Two seconds is nowhere near longSleep,
+	// so a grandchild that genuinely survived the group kill still fails
+	// this the same way it always did.
+	deadline := time.Now().Add(2 * time.Second)
+
+	var kerr error
+
+	for {
+		kerr = syscall.Kill(pid, 0)
+		if errors.Is(kerr, syscall.ESRCH) || time.Now().After(deadline) {
+			break
+		}
+
+		time.Sleep(10 * time.Millisecond)
+	}
+
+	if !errors.Is(kerr, syscall.ESRCH) {
 		t.Errorf("syscall.Kill(%d, 0) = %v, want ESRCH — grandchild pid %d is still alive", pid, kerr, pid)
 	}
 }