Merge branch 'smh-fix-wrong-status-code' into 'master'

Return context error from requestFinalizer

Closes #4391

See merge request gitlab-org/gitaly!4755

2 files changed, 27 insertions, 9 deletions

diff --git a/internal/praefect/coordinator.go b/internal/praefect/coordinator.go
index 6f6d439b6..0e00c03c0 100644
--- a/internal/praefect/coordinator.go
+++ b/internal/praefect/coordinator.go
@@ -989,7 +989,7 @@ func routerNodesToStorages(nodes []RouterNode) []string {
 }
 
 func (c *Coordinator) newRequestFinalizer(
-	ctx context.Context,
+	originalCtx context.Context,
 	repositoryID int64,
 	virtualStorage string,
 	targetRepo *gitalypb.Repository,
@@ -1006,7 +1006,7 @@ func (c *Coordinator) newRequestFinalizer(
 		// canceled. We need to perform the database updates regardless whether the request was canceled or not as
 		// the primary replica could have been dirtied and secondaries become outdated. Otherwise we'd have no idea of
 		// the possible changes performed on the disk.
-		ctx, cancel := context.WithTimeout(helper.SuppressCancellation(ctx), 30*time.Second)
+		ctx, cancel := context.WithTimeout(helper.SuppressCancellation(originalCtx), 30*time.Second)
 		defer cancel()
 
 		log := ctxlogrus.Extract(ctx).WithFields(logrus.Fields{
@@ -1090,7 +1090,16 @@ func (c *Coordinator) newRequestFinalizer(
 				return nil
 			})
 		}
-		return g.Wait()
+
+		if err := g.Wait(); err != nil {
+			return err
+		}
+
+		// The cancellation signal is suppressed earlier in the function, so we'd return no error if the
+		// orignal context exceeded its deadline while running the request finalizer. If there were
+		// no other errors, return the possible error from the context so we don't return OK code for
+		// failed requests.
+		return originalCtx.Err()
 	}
 }
 
diff --git a/internal/praefect/coordinator_test.go b/internal/praefect/coordinator_test.go
index caf2ad28f..19486f40c 100644
--- a/internal/praefect/coordinator_test.go
+++ b/internal/praefect/coordinator_test.go
@@ -2627,29 +2627,38 @@ func TestNewRequestFinalizer_contextIsDisjointedFromTheRPC(t *testing.T) {
 	err := errors.New("error")
 
 	for _, tc := range []struct {
-		change datastore.ChangeType
-		errMsg string
+		desc         string
+		change       datastore.ChangeType
+		errMsg       string
+		enqueueError error
 	}{
 		{
+			desc:   "increment generation receives suppressed cancellation",
 			change: datastore.UpdateRepo,
 			errMsg: "increment generation: error",
 		},
 		{
+			desc:   "rename repository receives suppressed cancellation",
 			change: datastore.RenameRepo,
 			errMsg: "rename repository: error",
 		},
 		{
-			change: "replication jobs only",
-			errMsg: "enqueue replication event: error",
+			desc:         "enqueue receives suppressed cancellation",
+			errMsg:       "enqueue replication event: error",
+			enqueueError: err,
+		},
+		{
+			desc:   "original context error is returned",
+			errMsg: context.DeadlineExceeded.Error(),
 		},
 	} {
-		t.Run(string(tc.change), func(t *testing.T) {
+		t.Run(tc.desc, func(t *testing.T) {
 			require.EqualError(t,
 				NewCoordinator(
 					&datastore.MockReplicationEventQueue{
 						EnqueueFunc: func(ctx context.Context, _ datastore.ReplicationEvent) (datastore.ReplicationEvent, error) {
 							requireSuppressedCancellation(t, ctx)
-							return datastore.ReplicationEvent{}, err
+							return datastore.ReplicationEvent{}, tc.enqueueError
 						},
 					},
 					datastore.MockRepositoryStore{