From db79ee42aa4ce246143fd292e083ad85441de20d Mon Sep 17 00:00:00 2001
From: Aleksandr Nogikh <nogikh@google.com>
Date: Mon, 2 Dec 2024 16:28:50 +0100
Subject: pkg/bisect: ignore irrelevant lost connection crashes

These have been the cause of too many invalid bisection results
recently.

This is the first step towards the more universal approach of #5414.
---
 pkg/bisect/bisect.go      | 22 +++++++++++++++++-----
 pkg/bisect/bisect_test.go | 42 +++++++++++++++++++++++++++++++-----------
 2 files changed, 48 insertions(+), 16 deletions(-)

(limited to 'pkg/bisect')

diff --git a/pkg/bisect/bisect.go b/pkg/bisect/bisect.go
index e8f85c47f..73a1971c8 100644
--- a/pkg/bisect/bisect.go
+++ b/pkg/bisect/bisect.go
@@ -1014,12 +1014,24 @@ func mostFrequentReports(reports []*report.Report) (*report.Report, []crash.Type
 func (env *env) isTransientError(rep *report.Report) bool {
 	// If we're not chasing a SYZFATAL error, ignore them.
 	// Otherwise it indicates some transient problem of the tested kernel revision.
-	hadSyzFailure := false
-	for _, t := range env.reportTypes {
-		hadSyzFailure = hadSyzFailure || t == crash.SyzFailure
+	if rep.Type == crash.SyzFailure {
+		hadSyzFailure := false
+		for _, t := range env.reportTypes {
+			hadSyzFailure = hadSyzFailure || t == crash.SyzFailure
+		}
+		return len(env.reportTypes) > 0 && !hadSyzFailure
+	}
+	// Lost connection is a frequent source of flaky results.
+	// Ignore if it is was not in the canonical crash types set.
+	if rep.Type == crash.LostConnection {
+		hadLostConnection := false
+		for _, t := range env.reportTypes {
+			hadLostConnection = hadLostConnection || t == crash.LostConnection
+		}
+		return len(env.reportTypes) > 0 && !hadLostConnection
 	}
-	return rep.Type == crash.SyzFailure &&
-		len(env.reportTypes) > 0 && !hadSyzFailure
+	// All other errors are okay.
+	return false
 }
 
 func (env *env) saveDebugFile(hash string, idx int, data []byte) {
diff --git a/pkg/bisect/bisect_test.go b/pkg/bisect/bisect_test.go
index 959d6640a..7d251c9e7 100644
--- a/pkg/bisect/bisect_test.go
+++ b/pkg/bisect/bisect_test.go
@@ -116,18 +116,27 @@ func (env *testEnv) Test(numVMs int, reproSyz, reproOpts, reproC []byte) ([]inst
 		}
 		return ret, nil
 	}
-	ret = make([]instance.EnvTestResult, numVMs-1)
+	ret = make([]instance.EnvTestResult, numVMs)
 	if env.test.injectSyzFailure {
-		ret = append(ret, instance.EnvTestResult{
-			Error: &instance.TestError{
+		ret[0] = instance.EnvTestResult{
+			Error: &instance.CrashError{
 				Report: &report.Report{
 					Title: "SYZFATAL: test",
 					Type:  crash.SyzFailure,
 				},
 			},
-		})
-	} else {
-		ret = append(ret, instance.EnvTestResult{})
+		}
+	} else if env.test.injectLostConnection {
+		for i := 0; i < numVMs/3; i++ {
+			ret[i] = instance.EnvTestResult{
+				Error: &instance.CrashError{
+					Report: &report.Report{
+						Title: "lost connection to test machine",
+						Type:  crash.LostConnection,
+					},
+				},
+			}
+		}
 	}
 	return ret, nil
 }
@@ -312,11 +321,12 @@ type BisectionTest struct {
 	expectErr     bool
 	expectErrType any
 	// Expect res.Report != nil.
-	expectRep        bool
-	noopChange       bool
-	isRelease        bool
-	flaky            bool
-	injectSyzFailure bool
+	expectRep            bool
+	noopChange           bool
+	isRelease            bool
+	flaky                bool
+	injectSyzFailure     bool
+	injectLostConnection bool
 	// Expected number of returned commits for inconclusive bisection.
 	commitLen int
 	// For cause bisection: Oldest commit returned by bisection.
@@ -496,6 +506,16 @@ var bisectionTests = []BisectionTest{
 		fixCommit:        "500",
 		isRelease:        true,
 	},
+	// Tests that bisection returns the correct fix commit despite `lost connection to test machine`.
+	{
+		name:                 "fix-finds-fix-despite-lost-connection",
+		fix:                  true,
+		startCommit:          400,
+		injectLostConnection: true,
+		commitLen:            1,
+		fixCommit:            "500",
+		isRelease:            true,
+	},
 	// Tests that bisection returns the correct fix commit in case of SYZFATAL.
 	{
 		name:        "fix-finds-fix-for-syzfatal",
-- 
cgit mrf-deployment