many: deal with incorrect status from osbuild better

bib/pkg/progress/progress.go

@@ -8,6 +8,7 @@ import (
 	"os"
 	"os/exec"
 	"strings"
+	"syscall"
 	"time"
 
 	"github.com/cheggaaa/pb/v3"
@@ -340,7 +341,7 @@ func runOSBuildNoProgress(pb ProgressBar, manifest []byte, store, outputDirector
 
 var osbuildCmd = "osbuild"
 
-func runOSBuildWithProgress(pb ProgressBar, manifest []byte, store, outputDirectory string, exports, extraEnv []string) error {
+func runOSBuildWithProgress(pb ProgressBar, manifest []byte, store, outputDirectory string, exports, extraEnv []string) (err error) {
 	rp, wp, err := os.Pipe()
 	if err != nil {
 		return fmt.Errorf("cannot create pipe for osbuild: %w", err)
@@ -372,14 +373,32 @@ func runOSBuildWithProgress(pb ProgressBar, manifest []byte, store, outputDirect
 		return fmt.Errorf("error starting osbuild: %v", err)
 	}
 	wp.Close()
+	defer func() {
+		// Try to stop osbuild if we exit early, we are gentle
+		// here to give osbuild the chance to release its
+		// resources (like mounts in the buildroot). This is
+		// best effort only (but also a pretty uncommon error
+		// condition). If ProcessState is set the process has
+		// already exited and we have nothing to do.
+		if err != nil && cmd.Process != nil && cmd.ProcessState == nil {
+			sigErr := cmd.Process.Signal(syscall.SIGINT)
+			err = errors.Join(err, sigErr)
+		}
+	}()
 
 	var tracesMsgs []string
-	var statusErrs []error
 	for {
 		st, err := osbuildStatus.Status()
 		if err != nil {
-			statusErrs = append(statusErrs, err)
-			continue
+			// This should never happen but if it does we try
+			// to be helpful. We need to exit here (and kill
+			// osbuild in the defer) or we would appear to be
+			// handing as cmd.Wait() would wait to finish but
+			// no progress or other message is reported. We
+			// can also not (in the general case) recover as
+			// the underlying osbuildStatus.scanner maybe in
+			// an unrecoverable state (like ErrTooBig).
+			return fmt.Errorf(`error parsing osbuild status, please report a bug and try with "--progress=verbose": %w`, err)
 		}
 		if st == nil {
 			break
@@ -408,9 +427,6 @@ func runOSBuildWithProgress(pb ProgressBar, manifest []byte, store, outputDirect
 	if err := cmd.Wait(); err != nil {
 		return fmt.Errorf("error running osbuild: %w\nBuildLog:\n%s\nOutput:\n%s", err, strings.Join(tracesMsgs, "\n"), stdio.String())
 	}
-	if len(statusErrs) > 0 {
-		return fmt.Errorf("errors parsing osbuild status:\n%w", errors.Join(statusErrs...))
-	}
 
 	return nil
 }

bib/pkg/progress/progress_test.go

@@ -8,6 +8,7 @@ import (
 	"path/filepath"
 	"reflect"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 
@@ -167,15 +168,35 @@ osbuild-stderr-output
 }
 
 func TestRunOSBuildWithProgressIncorrectJSON(t *testing.T) {
-	restore := progress.MockOsbuildCmd(makeFakeOsbuild(t, `echo osbuild-stdout-output
->&2 echo osbuild-stderr-output
+	signalDeliveredMarkerPath := filepath.Join(t.TempDir(), "sigint-delivered")
+
+	restore := progress.MockOsbuildCmd(makeFakeOsbuild(t, fmt.Sprintf(`
+trap 'touch "%s";exit 2' INT
+
 >&3 echo invalid-json
-`))
+
+# we cannot sleep infinity here or the shell script trap is never run
+while true; do
+    sleep 0.1
+done
+`, signalDeliveredMarkerPath)))
 	defer restore()
 
 	pbar, err := progress.New("debug")
 	assert.NoError(t, err)
 	err = progress.RunOSBuild(pbar, []byte(`{"fake":"manifest"}`), "", "", nil, nil)
-	assert.EqualError(t, err, `errors parsing osbuild status:
-cannot scan line "invalid-json": invalid character 'i' looking for beginning of value`)
+	assert.EqualError(t, err, `error parsing osbuild status, please report a bug and try with "--progress=verbose": cannot scan line "invalid-json": invalid character 'i' looking for beginning of value`)
+
+	// ensure the SIGINT got delivered
+	var pathExists = func(p string) bool {
+		_, err := os.Stat(p)
+		return err == nil
+	}
+	for i := 0; i < 20; i++ {
+		time.Sleep(100 * time.Millisecond)
+		if pathExists(signalDeliveredMarkerPath) {
+			break
+		}
+	}
+	assert.True(t, pathExists(signalDeliveredMarkerPath))
 }

test/test_build.py

@@ -405,6 +405,8 @@ def build_images(shared_tmpdir, build_container, request, force_aws_upload, gpg_
             "-v", "/var/tmp/osbuild-test-store:/store",  # share the cache between builds
             "-v", "/var/lib/containers/storage:/var/lib/containers/storage",  # mount the host's containers storage
         ]
+        if tc.podman_terminal:
+            cmd.append("-t")
 
         if tc.sign:
             sign_container_image(gpg_conf, registry_conf, tc.container_ref)

test/testcases.py

@@ -27,6 +27,8 @@ class TestCase:
     disk_config: str = ""
     # use librepo for the downloading
     use_librepo: bool = False
+    # podman_terminal enables the podman -t option to get progress
+    podman_terminal: bool = False
 
     def bib_rootfs_args(self):
         if self.rootfs:
@@ -61,6 +63,7 @@ class TestCaseC9S(TestCase):
         "BIB_TEST_BOOTC_CONTAINER_TAG",
         "quay.io/centos-bootc/centos-bootc:stream9")
     use_librepo: bool = True
+    use_terminal: bool = True
 
 
 @dataclasses.dataclass

test/testutil.py

@@ -202,6 +202,8 @@ def maybe_create_disk_customizations(cfg, tc):
     "--privileged",
     "-v", "/var/lib/containers/storage:/var/lib/containers/storage",
     "--security-opt", "label=type:unconfined_t",
+    # ensure we run in reasonable memory limits
+    "--memory=8g", "--memory-swap=8g",
 ]