diff options
author | Sami Hiltunen <shiltunen@gitlab.com> | 2022-08-02 08:41:24 +0300 |
---|---|---|
committer | Sami Hiltunen <shiltunen@gitlab.com> | 2022-08-02 08:41:24 +0300 |
commit | 3aad262e1f54936f9eef6f7f2cbf571cbc1a7307 (patch) | |
tree | 41fa80036d814efca58777873830382b78b76610 | |
parent | 445024454b60b661cc7bc7782c9e9367517f42e2 (diff) |
Return context error from requestFinalizer
-rw-r--r-- | internal/praefect/coordinator.go | 15 | ||||
-rw-r--r-- | internal/praefect/coordinator_test.go | 21 |
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{ |