diff --git a/internal/gitaly/service/smarthttp/cache.go b/internal/gitaly/service/smarthttp/cache.go index 515029c0fce27515d6dbe9a778c739a60a6eb661..b0fda42f5fedf362619aa844fb6d6db0bc40fbee 100644 --- a/internal/gitaly/service/smarthttp/cache.go +++ b/internal/gitaly/service/smarthttp/cache.go @@ -106,6 +106,7 @@ func (c infoRefCache) tryCache(ctx context.Context, in *gitalypb.InfoRefsRequest c.logger.WithError(err). ErrorContext(ctx, "unable to discard remaining InfoRefsUploadPack cache stream") } + _ = pr.CloseWithError(err) // always returns nil } }() diff --git a/internal/gitaly/service/smarthttp/inforefs_test.go b/internal/gitaly/service/smarthttp/inforefs_test.go index 8d864e6452af87ca45f95b524ad79e171e93391d..20a42ab074fbf787fd156a3ca5da1b6ad33fa2b0 100644 --- a/internal/gitaly/service/smarthttp/inforefs_test.go +++ b/internal/gitaly/service/smarthttp/inforefs_test.go @@ -10,6 +10,7 @@ import ( "path/filepath" "strings" "testing" + "time" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/v16/internal/cache" @@ -306,6 +307,60 @@ func testInfoRefsUploadPackGitProtocol(t *testing.T, ctx context.Context) { require.Contains(t, envData, fmt.Sprintf("GIT_PROTOCOL=%s\n", git.ProtocolV2)) } +func TestInfoRefsUploadPack_cacheNotStuckUponContextCancellation(t *testing.T) { + t.Parallel() + + testhelper.NewFeatureSets( + featureflag.BundleURI, + ).Run(t, testInfoRefsUploadPackCacheNotStuckUponContextCancellation) +} + +// We mainly test for goroutine leakage here, so if the server is hanging on finishing the request, +// we will be leaking a goroutine and the test framework will panic. +func testInfoRefsUploadPackCacheNotStuckUponContextCancellation(t *testing.T, ctx context.Context) { + t.Parallel() + + cfg := testcfg.Build(t) + + locator := config.NewLocator(cfg) + logger := testhelper.NewLogger(t) + cache := cache.New(cfg, locator, logger) + + streamer := mockStreamer{ + Streamer: cache, + } + mockInfoRefCache := newInfoRefCache(logger, &streamer) + + // This is to simulate a long-running `git upload-pack` command so that we can have time to cancel the request abruptly. + gitCmdFactory := gittest.NewInterceptingCommandFactory(t, ctx, cfg, func(execEnv git.ExecutionEnvironment) string { + return fmt.Sprintf(`#!/usr/bin/env bash + if [[ "$@" =~ "upload-pack" ]]; then + while true; do echo "foo"; done + exit 1 + fi + exec %q "$@"`, execEnv.BinaryPath) + }) + server := startSmartHTTPServerWithOptions(t, cfg, + []ServerOpt{withInfoRefCache(mockInfoRefCache)}, + []testserver.GitalyServerOpt{testserver.WithGitCommandFactory(gitCmdFactory)}, + ) + cfg.SocketPath = server.Address() + + repo, repoPath := gittest.CreateRepository(t, ctx, cfg) + + commitID := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("main"), gittest.WithParents()) + _ = gittest.WriteTag(t, cfg, repoPath, "v1.0.0", commitID.Revision(), gittest.WriteTagConfig{ + Message: "annotated tag", + }) + + requestCtx, cancel := context.WithTimeout(ctx, 1*time.Second) + defer cancel() + + rpcRequest := &gitalypb.InfoRefsRequest{Repository: repo} + _, err := makeInfoRefsUploadPackRequest(t, requestCtx, cfg.SocketPath, cfg.Auth.Token, rpcRequest) + require.Error(t, err) +} + func makeInfoRefsUploadPackRequest(t *testing.T, ctx context.Context, serverSocketPath, token string, rpcRequest *gitalypb.InfoRefsRequest) ([]byte, error) { t.Helper()