review: fact-gathering commands get a deadline; a hung probe cannot stall a campaign

Author: marwen-abid

runner/internal/bundle/metadata.go

@@ -2,6 +2,7 @@ package bundle
 
 import (
 	"bufio"
+	"context"
 	"encoding/json"
 	"fmt"
 	"io"
@@ -238,10 +239,21 @@ func memTotalKB() int64 {
 	return 0
 }
 
+// factTimeout bounds every fact-gathering command. A probe that hangs — lsblk on
+// a wedged device, a binary whose `version` never returns — must not cost an
+// unattended campaign its start, so a fact that takes too long is treated the
+// same as one that cannot be gathered. A var so tests can shrink it.
+var factTimeout = 5 * time.Second
+
 // commandOutput runs a fact-gathering command and returns its trimmed stdout,
-// or "" if it cannot be run. Every caller here is best-effort by contract.
+// or "" if it cannot be run or does not finish inside factTimeout. Every caller
+// here is best-effort by contract.
 func commandOutput(name string, args ...string) string {
-	out, err := exec.Command(name, args...).Output()
+	ctx, cancel := context.WithTimeout(context.Background(), factTimeout)
+	defer cancel()
+	cmd := exec.CommandContext(ctx, name, args...)
+	cmd.WaitDelay = time.Second
+	out, err := cmd.Output()
 	if err != nil {
 		return ""
 	}

runner/internal/bundle/metadata_test.go

@@ -255,6 +255,19 @@ func TestCollectHardwareOffEC2(t *testing.T) {
 	}
 }
 
+func TestCommandOutputTimesOut(t *testing.T) {
+	old := factTimeout
+	factTimeout = 100 * time.Millisecond
+	t.Cleanup(func() { factTimeout = old })
+	start := time.Now()
+	if got := commandOutput("sleep", "30"); got != "" {
+		t.Errorf("commandOutput = %q, want \"\" on timeout", got)
+	}
+	if elapsed := time.Since(start); elapsed > 5*time.Second {
+		t.Errorf("commandOutput took %s, want well under the 30s sleep", elapsed)
+	}
+}
+
 func TestWriteMetadataUnwritableDir(t *testing.T) {
 	err := WriteMetadata(filepath.Join(t.TempDir(), "nope"), goldenInput(t))
 	if err == nil {

runner/internal/bundle/provenance.go

@@ -1,6 +1,7 @@
 package bundle
 
 import (
+	"context"
 	"fmt"
 	"os"
 	"os/exec"
@@ -158,9 +159,14 @@ func fsyncProbe(benchRoot string) string {
 
 // binaryVersion is the benchmarked binary's own version output, first 3 lines,
 // stdout and stderr together — the binary reports its build stamp there. A
-// binary that cannot run says so in place of its version.
+// binary that cannot run, or does not answer inside factTimeout, says so in
+// place of its version.
 func binaryVersion(binPath string) []string {
-	out, err := exec.Command(binPath, "version").CombinedOutput()
+	ctx, cancel := context.WithTimeout(context.Background(), factTimeout)
+	defer cancel()
+	cmd := exec.CommandContext(ctx, binPath, "version")
+	cmd.WaitDelay = time.Second
+	out, err := cmd.CombinedOutput()
 	lines := splitLines(head(strings.TrimRight(string(out), "\n"), 3))
 	if err != nil && len(lines) == 0 {
 		return []string{fmt.Sprintf("version: %v", err)}