internal/trace: emit sync event before deferred spilled error

CL 648315 and CL 648195 fixed #71615 in the case where we fail to read
the next generation by emitting an extra sync event before returning an
error. But, it's possible we failed to even read the next spilled batch
when we read the first generation, and have been carrying the error from
trying to read a spilled batch since the last generation. In this case,
we don't emit a final sync event, meaning that there are still some
cases where #71615 happens.

This change emits the final sync event in this corner case. I believe
this is the final corner case. I could previously reproduce the issue
by running the test under stress2, but I can no longer reproduce any
failures after this change.

Fixes #71615, for real this time.

Change-Id: I10688a3c0e4b8327a95f31add365338c77c091ab
Reviewed-on: https://go-review.googlesource.com/c/go/+/649259
Reviewed-by: Cherry Mui <cherryyz@google.com>
Auto-Submit: Michael Knyszek <mknyszek@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

1 files changed, 19 insertions, 12 deletions

diff --git a/src/internal/trace/reader.go b/src/internal/trace/reader.go
index f5f871763f..7212a424d7 100644
--- a/src/internal/trace/reader.go
+++ b/src/internal/trace/reader.go
@@ -22,17 +22,18 @@ import (
 // event as the first event, and a Sync event as the last event.
 // (There may also be any number of Sync events in the middle, too.)
 type Reader struct {
-	version    version.Version
-	r          *bufio.Reader
-	lastTs     Time
-	gen        *generation
-	spill      *spilledBatch
-	spillErr   error // error from reading spill
-	frontier   []*batchCursor
-	cpuSamples []cpuSample
-	order      ordering
-	syncs      int
-	done       bool
+	version      version.Version
+	r            *bufio.Reader
+	lastTs       Time
+	gen          *generation
+	spill        *spilledBatch
+	spillErr     error // error from reading spill
+	spillErrSync bool  // whether we emitted a Sync before reporting spillErr
+	frontier     []*batchCursor
+	cpuSamples   []cpuSample
+	order        ordering
+	syncs        int
+	done         bool
 
 	v1Events *traceV1Converter
 }
@@ -139,7 +140,12 @@ func (r *Reader) ReadEvent() (e Event, err error) {
 	// Check if we need to refresh the generation.
 	if len(r.frontier) == 0 && len(r.cpuSamples) == 0 {
 		if r.spillErr != nil {
-			return Event{}, r.spillErr
+			if r.spillErrSync {
+				return Event{}, r.spillErr
+			}
+			r.spillErrSync = true
+			r.syncs++
+			return syncEvent(nil, r.lastTs, r.syncs), nil
 		}
 		if r.gen != nil && r.spill == nil {
 			// If we have a generation from the last read,
@@ -154,6 +160,7 @@ func (r *Reader) ReadEvent() (e Event, err error) {
 		// Read the next generation.
 		r.gen, r.spill, r.spillErr = readGeneration(r.r, r.spill)
 		if r.gen == nil {
+			r.spillErrSync = true
 			r.syncs++
 			return syncEvent(nil, r.lastTs, r.syncs), nil
 		}