diff options
| author | Patrick Steinhardt <psteinhardt@gitlab.com> | 2022-04-26 14:15:57 +0300 |
|---|---|---|
| committer | Patrick Steinhardt <psteinhardt@gitlab.com> | 2022-04-29 13:56:46 +0300 |
| commit | 77b8ced5bd60eebd0614e5ab408d048ab1116d43 (patch) | |
| tree | 62daf35e0dd58a736a717197485279efc7dd0291 | |
| parent | fa62277145bce6832f98181d4c3d660ccfa570d1 (diff) | |
catfile: Fix losing all context information on decorrelation
When creating a new cached catfile process we need to decorrelate it
from the parent context. For one this means that we need to remove the
correlation ID and any tracing spans from the context. And second it
means we need to suppress cancellation of the parent so that our cached
process doesn't get killed when the RPC context is cancelled.
The way we do this right now is to use `context.Background()`: this just
creates a completely new background. Funny enough, we still try to strip
information from this new context, which doesn't make any sense given
that it's empty already. But this hints at an actual bug: the intent is
not to get a completely new context, but by using `context.Background()`
we also lose information such as feature flags here.
Fix this bug by not creating a completely new context. Instead, we just
suppress context cancellation via `helper.SuppressCancellation()` and
then continue to decorrelate it like we always used to do. Only that now
it actually does something.
Changelog: fixed
| -rw-r--r-- | internal/git/catfile/cache.go | 10 |
1 files changed, 9 insertions, 1 deletions
diff --git a/internal/git/catfile/cache.go b/internal/git/catfile/cache.go index 78c183407..293ec3e65 100644 --- a/internal/git/catfile/cache.go +++ b/internal/git/catfile/cache.go @@ -14,6 +14,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/v14/internal/helper" "gitlab.com/gitlab-org/gitaly/v14/internal/metadata" + "gitlab.com/gitlab-org/labkit/correlation" ) const ( @@ -231,7 +232,14 @@ func (c *ProcessCache) getOrCreateProcess( // We have not found any cached process, so we need to create a new one. In this // case, we need to detach the process from the current context such that it does // not get killed when the parent context is cancelled. - ctx = context.Background() + // + // Note that we explicitly retain feature flags here, which means that cached + // processes may retain flags for some time which have been changed meanwhile. While + // not ideal, it feels better compared to just ignoring feature flags altogether. + // The latter would mean that we cannot use flags in the catfile code, but more + // importantly we also wouldn't be able to use feature-flagged Git version upgrades + // for catfile processes. + ctx = helper.SuppressCancellation(ctx) // We have to decorrelate the process from the current context given that it // may potentially be reused across different RPC calls. ctx = correlation.ContextWithCorrelation(ctx, "") |