gitpipe: Add tests demonstrating loss of context-cancellation errors

We're not bubbling up any errors in case contexts get cancelled while we
were consuming pipeline result. This error makes it easy to think that
the pipeline finished successfully even though the pipeline has been
cancelled and only returned partial results.

Add a set of tests to demonstrate this behaviour.

3 files changed, 135 insertions, 10 deletions

diff --git a/internal/git/gitpipe/catfile_info_test.go b/internal/git/gitpipe/catfile_info_test.go
index 80a1810d9..f2226207d 100644
--- a/internal/git/gitpipe/catfile_info_test.go
+++ b/internal/git/gitpipe/catfile_info_test.go
@@ -1,6 +1,7 @@
 package gitpipe
 
 import (
+	"context"
 	"errors"
 	"testing"
 
@@ -155,6 +156,36 @@ func TestCatfileInfo(t *testing.T) {
 			require.Equal(t, tc.expectedResults, results)
 		})
 	}
+
+	t.Run("context cancellation", func(t *testing.T) {
+		ctx, cancel := context.WithCancel(testhelper.Context(t))
+
+		catfileCache := catfile.NewCache(cfg)
+		defer catfileCache.Stop()
+
+		objectInfoReader, objectInfoReaderCancel, err := catfileCache.ObjectInfoReader(ctx, repo)
+		require.NoError(t, err)
+		defer objectInfoReaderCancel()
+
+		it, err := CatfileInfo(ctx, objectInfoReader, NewRevisionIterator([]RevisionResult{
+			{OID: lfsPointer1},
+			{OID: lfsPointer1},
+		}))
+		require.NoError(t, err)
+
+		require.True(t, it.Next())
+		require.NoError(t, it.Err())
+		require.Equal(t, CatfileInfoResult{
+			ObjectInfo: &catfile.ObjectInfo{Oid: lfsPointer1, Type: "blob", Size: 133},
+		}, it.Result())
+
+		cancel()
+
+		require.False(t, it.Next())
+		// This is a bug: we expect to get the cancelled context here.
+		require.NoError(t, it.Err())
+		require.Equal(t, CatfileInfoResult{}, it.Result())
+	})
 }
 
 func TestCatfileInfoAllObjects(t *testing.T) {
@@ -171,18 +202,39 @@ func TestCatfileInfoAllObjects(t *testing.T) {
 	})
 	commit := gittest.WriteCommit(t, cfg, repoPath, gittest.WithParents())
 
-	it := CatfileInfoAllObjects(ctx, repo)
-
-	var results []CatfileInfoResult
-	for it.Next() {
-		results = append(results, it.Result())
-	}
-	require.NoError(t, it.Err())
-
-	require.ElementsMatch(t, []CatfileInfoResult{
+	actualObjects := []CatfileInfoResult{
 		{ObjectInfo: &catfile.ObjectInfo{Oid: blob1, Type: "blob", Size: 6}},
 		{ObjectInfo: &catfile.ObjectInfo{Oid: blob2, Type: "blob", Size: 6}},
 		{ObjectInfo: &catfile.ObjectInfo{Oid: tree, Type: "tree", Size: 34}},
 		{ObjectInfo: &catfile.ObjectInfo{Oid: commit, Type: "commit", Size: 177}},
-	}, results)
+	}
+
+	t.Run("successful", func(t *testing.T) {
+		it := CatfileInfoAllObjects(ctx, repo)
+
+		var results []CatfileInfoResult
+		for it.Next() {
+			results = append(results, it.Result())
+		}
+		require.NoError(t, it.Err())
+
+		require.ElementsMatch(t, actualObjects, results)
+	})
+
+	t.Run("context cancellation", func(t *testing.T) {
+		ctx, cancel := context.WithCancel(testhelper.Context(t))
+
+		it := CatfileInfoAllObjects(ctx, repo)
+
+		require.True(t, it.Next())
+		require.NoError(t, it.Err())
+		require.Contains(t, actualObjects, it.Result())
+
+		cancel()
+
+		require.False(t, it.Next())
+		// This is a bug: we expect to get the cancelled context here.
+		require.NoError(t, it.Err())
+		require.Equal(t, CatfileInfoResult{}, it.Result())
+	})
 }
diff --git a/internal/git/gitpipe/catfile_object_test.go b/internal/git/gitpipe/catfile_object_test.go
index 95a9b3ad6..b847a6a75 100644
--- a/internal/git/gitpipe/catfile_object_test.go
+++ b/internal/git/gitpipe/catfile_object_test.go
@@ -1,11 +1,13 @@
 package gitpipe
 
 import (
+	"context"
 	"errors"
 	"io"
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo"
@@ -115,4 +117,35 @@ func TestCatfileObject(t *testing.T) {
 			require.Equal(t, tc.expectedResults, results)
 		})
 	}
+
+	t.Run("context cancellation", func(t *testing.T) {
+		ctx, cancel := context.WithCancel(testhelper.Context(t))
+
+		catfileCache := catfile.NewCache(cfg)
+		defer catfileCache.Stop()
+
+		objectReader, objectReaderCancel, err := catfileCache.ObjectReader(ctx, repo)
+		require.NoError(t, err)
+		defer objectReaderCancel()
+
+		it, err := CatfileObject(ctx, objectReader, NewCatfileInfoIterator([]CatfileInfoResult{
+			{ObjectInfo: &catfile.ObjectInfo{Oid: lfsPointer1, Type: "blob", Size: 133}},
+			{ObjectInfo: &catfile.ObjectInfo{Oid: lfsPointer2, Type: "blob", Size: 127}},
+		}))
+		require.NoError(t, err)
+
+		require.True(t, it.Next())
+		require.NoError(t, it.Err())
+		require.Equal(t, git.ObjectID(lfsPointer1), it.Result().ObjectID())
+
+		_, err = io.Copy(io.Discard, it.Result())
+		require.NoError(t, err)
+
+		cancel()
+
+		require.False(t, it.Next())
+		// This is a bug: we expect to get the cancelled context here.
+		require.NoError(t, it.Err())
+		require.Equal(t, CatfileObjectResult{}, it.Result())
+	})
 }
diff --git a/internal/git/gitpipe/revision_test.go b/internal/git/gitpipe/revision_test.go
index 091df9e3f..c28fa39dc 100644
--- a/internal/git/gitpipe/revision_test.go
+++ b/internal/git/gitpipe/revision_test.go
@@ -1,6 +1,7 @@
 package gitpipe
 
 import (
+	"context"
 	"errors"
 	"testing"
 	"time"
@@ -504,6 +505,25 @@ func TestRevlist(t *testing.T) {
 			require.Equal(t, tc.expectedResults, results)
 		})
 	}
+
+	t.Run("context cancellation", func(t *testing.T) {
+		ctx, cancel := context.WithCancel(testhelper.Context(t))
+
+		it := Revlist(ctx, repo, []string{"refs/heads/master"})
+
+		require.True(t, it.Next())
+		require.NoError(t, it.Err())
+		require.Equal(t, RevisionResult{
+			OID: "1e292f8fedd741b75372e19097c76d327140c312",
+		}, it.Result())
+
+		cancel()
+
+		require.False(t, it.Next())
+		// This is a bug: we expect to get the cancelled context here.
+		require.NoError(t, it.Err())
+		require.Equal(t, RevisionResult{}, it.Result())
+	})
 }
 
 func TestForEachRef(t *testing.T) {
@@ -603,6 +623,26 @@ func TestForEachRef(t *testing.T) {
 	t.Run("nonexisting pattern", func(t *testing.T) {
 		require.Nil(t, readRefs(t, repo, []string{"refs/idontexist/*"}))
 	})
+
+	t.Run("context cancellation", func(t *testing.T) {
+		ctx, cancel := context.WithCancel(testhelper.Context(t))
+
+		it := ForEachRef(ctx, repo, []string{"refs/heads/*"})
+
+		require.True(t, it.Next())
+		require.NoError(t, it.Err())
+		require.Equal(t, RevisionResult{
+			OID:        "e56497bb5f03a90a51293fc6d516788730953899",
+			ObjectName: []byte("refs/heads/'test'"),
+		}, it.Result())
+
+		cancel()
+
+		require.False(t, it.Next())
+		// This is a bug: we expect to get the cancelled context here.
+		require.NoError(t, it.Err())
+		require.Equal(t, RevisionResult{}, it.Result())
+	})
 }
 
 func TestForEachRef_options(t *testing.T) {