From e51a56b0845e697d3dad9ab07b2af1931a445ba6 Mon Sep 17 00:00:00 2001
From: Michael Vogt <michael.vogt@gmail.com>
Date: Fri, 7 Feb 2025 13:19:50 +0100
Subject: [PATCH] progress: stop building when osbuild status cannot be parsed

This commit changes the progress parser (again) to deal with
errors from the osbuild json progress scanner. On errors we
will now exit right away and potentially kill osbuild but
provide an error message that hints how to workaround the
issue.

The original code assumed we get transient errors like json
decode issues. However it turns out that this is not always
the case, some errors from a bufio.Scanner are unrecoverable
(like ErrTooBig) and trying again just leads to an endless
loop. We can also not "break" and wait for the build to finish
because that would appear like the progress is broken
forever and we would still have to report an error (just
much later).
---
 bib/pkg/progress/progress.go      | 30 +++++++++++++++++++++++-------
 bib/pkg/progress/progress_test.go | 31 ++++++++++++++++++++++++++-----
 2 files changed, 49 insertions(+), 12 deletions(-)

diff --git a/bib/pkg/progress/progress.go b/bib/pkg/progress/progress.go
index 508fed4..2326153 100644
--- a/bib/pkg/progress/progress.go
+++ b/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
 }
diff --git a/bib/pkg/progress/progress_test.go b/bib/pkg/progress/progress_test.go
index 4343c98..1daa21c 100644
--- a/bib/pkg/progress/progress_test.go
+++ b/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))
 }