Return context error from requestFinalizersmh-fix-wrong-status-code

Praefect runs the request finalizer even if the request deadline was
exceeded in order to ensure the required metadata updates are made.
If the context deadline is exceeded in the request finalizer, no error
is returned as the metadata updates don't see it. This causes Praefect
to return, log and observe a status OK for requests that exceeded the
deadline or were canceled while running the request finalizer. This
commit returns the error from the original context if no other errors
are raised from the requestFinalizer to ensure the status gets correctly
set.

desc field is added to the tests as they currently abuse the change field
as the test name.

Changelog: fixed

2 files changed, 27 insertions, 9 deletions

diff --git a/internal/praefect/coordinator.go b/internal/praefect/coordinator.go
index 6f6d439b6..db3fef0b5 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 in the beginning, 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{