From 1df6aefadf9ecd44deed44fff8c055d6df16893f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mat=C3=ADas=20Insaurralde?= Date: Wed, 12 Aug 2026 20:12:46 -0300 Subject: [PATCH] fix(crafter): release go-git packfile descriptors after repository reads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit go-git v6.0.0-alpha.5 gives every PlainOpen* call its own 256-entry descriptor pool holding roughly 3 descriptors per packfile (.pack/.idx/.rev). The pool frees them only on LRU eviction, which never fires below 256 entries, so the descriptors stayed open for the lifetime of the process. alpha.4 released them right after each read. The crafter opens the repository up to twice per attestation init and never closed the storer, so a heavily packed checkout retained around 48 descriptors per open. Release them explicitly through storer.IdleReleaser once the reads are done; they reopen lazily, so the repository stays usable afterwards. Signed-off-by: Matías Insaurralde --- pkg/attestation/crafter/cioverride.go | 2 + pkg/attestation/crafter/crafter.go | 21 +++ .../crafter/git_descriptors_test.go | 47 +++++ .../crafter/git_descriptors_unix_test.go | 177 ++++++++++++++++++ 4 files changed, 247 insertions(+) create mode 100644 pkg/attestation/crafter/git_descriptors_test.go create mode 100644 pkg/attestation/crafter/git_descriptors_unix_test.go diff --git a/pkg/attestation/crafter/cioverride.go b/pkg/attestation/crafter/cioverride.go index dd15ab5ac..06f6c163a 100644 --- a/pkg/attestation/crafter/cioverride.go +++ b/pkg/attestation/crafter/cioverride.go @@ -95,6 +95,8 @@ func overrideHeadWithPRCommit(headCommit *HeadCommit, path, actualSHA string, lo return } + defer releaseGitDescriptors(repo, logger) + hash := plumbing.NewHash(actualSHA) commit, err := repo.CommitObject(hash) if err != nil { diff --git a/pkg/attestation/crafter/crafter.go b/pkg/attestation/crafter/crafter.go index 7e719e9ab..2bf383ed5 100644 --- a/pkg/attestation/crafter/crafter.go +++ b/pkg/attestation/crafter/crafter.go @@ -40,6 +40,7 @@ import ( "github.com/chainloop-dev/chainloop/pkg/policies" "github.com/go-git/go-git/v6" "github.com/go-git/go-git/v6/plumbing" + "github.com/go-git/go-git/v6/plumbing/storer" "github.com/google/go-containerregistry/pkg/authn" intoto "github.com/in-toto/attestation/go/v1" "github.com/rs/zerolog" @@ -338,6 +339,8 @@ func gracefulGitRepoHead(path string, logger *zerolog.Logger) (*HeadCommit, erro return nil, fmt.Errorf("opening repository: %w", err) } + defer releaseGitDescriptors(repo, logger) + head, err := repo.Head() if err != nil { if errors.Is(err, plumbing.ErrReferenceNotFound) { @@ -385,6 +388,24 @@ func gracefulGitRepoHead(path string, logger *zerolog.Logger) (*HeadCommit, erro return c, nil } +// releaseGitDescriptors closes the packfile descriptors go-git keeps open after +// a read. Since go-git v6.0.0-alpha.5 every PlainOpen* call builds its own +// 256-entry descriptor pool holding roughly 3 descriptors per packfile +// (.pack/.idx/.rev). Those are released only on LRU eviction, which never fires +// below 256 entries, so without this call they stay open for the lifetime of the +// process. Descriptors reopen lazily, so the repository remains usable +// afterwards. +func releaseGitDescriptors(repo *git.Repository, logger *zerolog.Logger) { + releaser, ok := repo.Storer.(storer.IdleReleaser) + if !ok { + return + } + + if err := releaser.CloseIdleDescriptors(); err != nil && logger != nil { + logger.Debug().Err(err).Msg("releasing git pack descriptors") + } +} + // Clear any basic auth credentials from the remote URL func sanitizeRemoteURL(remoteURL string) (string, error) { uri, err := url.Parse(remoteURL) diff --git a/pkg/attestation/crafter/git_descriptors_test.go b/pkg/attestation/crafter/git_descriptors_test.go new file mode 100644 index 000000000..79c5d7989 --- /dev/null +++ b/pkg/attestation/crafter/git_descriptors_test.go @@ -0,0 +1,47 @@ +// +// Copyright 2026 The Chainloop Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package crafter + +import ( + "testing" + + "github.com/go-git/go-git/v6" + "github.com/go-git/go-git/v6/plumbing/storer" + "github.com/stretchr/testify/require" +) + +// TestGitStorerImplementsIdleReleaser pins the interface that +// releaseGitDescriptors depends on. That helper deliberately fails open: if a +// future go-git release stops satisfying storer.IdleReleaser it would silently +// stop releasing descriptors, reintroducing the retention regression with +// nothing failing. This assertion turns that into a build-time-visible test +// failure instead. +// +// The behavioural counterpart lives in +// TestGracefulGitRepoHeadReleasesDescriptors, which is unix-only; this test +// carries the guard on every platform. +func TestGitStorerImplementsIdleReleaser(t *testing.T) { + repoDir := t.TempDir() + _, err := git.PlainInit(repoDir, false) + require.NoError(t, err) + + repo, err := git.PlainOpenWithOptions(repoDir, &git.PlainOpenOptions{DetectDotGit: true}) + require.NoError(t, err) + + _, ok := repo.Storer.(storer.IdleReleaser) + require.True(t, ok, + "go-git storer must implement storer.IdleReleaser; releaseGitDescriptors is a no-op without it") +} diff --git a/pkg/attestation/crafter/git_descriptors_unix_test.go b/pkg/attestation/crafter/git_descriptors_unix_test.go new file mode 100644 index 000000000..b764a862f --- /dev/null +++ b/pkg/attestation/crafter/git_descriptors_unix_test.go @@ -0,0 +1,177 @@ +// +// Copyright 2026 The Chainloop Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// The descriptor probe below has no Windows equivalent, and the CLI is built for +// Windows too, so this guard is scoped to unix. Linux CI and macOS development +// hosts — where descriptor limits actually bind — are both covered. +//go:build unix + +package crafter + +import ( + "os" + "os/exec" + "path/filepath" + "slices" + "strings" + "syscall" + "testing" + "time" + + "github.com/go-git/go-git/v6" + "github.com/go-git/go-git/v6/plumbing/object" + "github.com/stretchr/testify/require" +) + +// TestGitDescriptorsReleasedAtAllOpenSites guards the file-descriptor retention +// regression that go-git v6.0.0-alpha.5 introduced: every PlainOpenWithOptions +// call builds its own 256-entry descriptor pool holding roughly 3 descriptors +// per packfile (.pack/.idx/.rev), released only on LRU eviction — which never +// fires below 256 entries. Without an explicit release those descriptors stay +// open for the lifetime of the process. +// +// Every function that opens a repository must appear here: the release is a +// per-call-site defer, so a site omitted from this table can silently drop its +// release without any test failing. +// +// Each case repeats its open so a genuine leak accumulates into an unmistakable +// signal (~3 descriptors per iteration) while correct behaviour stays flat. A +// single open leaks too few descriptors to separate from incidental churn. +func TestGitDescriptorsReleasedAtAllOpenSites(t *testing.T) { + const ( + iterations = 40 + // The assertion is "flat", not "exact". Runtime and harness activity can + // shift the count by a few descriptors, whereas a real regression grows + // by ~3 per iteration (~120 total). + maxRetained = 20 + ) + + repoDir, headSHA := initPackedRepo(t) + + testCases := []struct { + name string + // open performs one complete open-read-release cycle against the fixture. + // It must assert that the read actually reached packed object data, + // otherwise a short-circuiting code path would make the measurement + // vacuous. + open func(t *testing.T) + }{ + { + name: "gracefulGitRepoHead", + open: func(t *testing.T) { + got, err := gracefulGitRepoHead(repoDir, nil) + require.NoError(t, err) + require.NotNil(t, got) + // Proves the commit object was read, not just the ref. + require.Equal(t, headSHA, got.Hash) + require.Equal(t, "John Doe", got.AuthorName) + }, + }, + { + name: "overrideHeadWithPRCommit", + open: func(t *testing.T) { + headCommit := &HeadCommit{} + overrideHeadWithPRCommit(headCommit, repoDir, headSHA, nil) + // Hash is assigned on the shallow-clone path too, so it cannot + // distinguish a real commit read from a lookup miss. AuthorName is + // only populated once the commit object resolves, which is what + // puts pack descriptors in play. + require.Equal(t, headSHA, headCommit.Hash) + require.Equal(t, "John Doe", headCommit.AuthorName) + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Warm up once so lazily-initialised state is not counted as retention. + tc.open(t) + + before := countOpenDescriptors() + for range iterations { + tc.open(t) + } + retained := countOpenDescriptors() - before + + t.Logf("retained %d descriptors across %d opens", retained, iterations) + require.Less(t, retained, maxRetained, + "%s retained %d descriptors across %d opens: go-git pools packfile "+ + "descriptors per repository open and frees them only via "+ + "CloseIdleDescriptors (see releaseGitDescriptors)", + tc.name, retained, iterations) + }) + } +} + +// initPackedRepo builds a single-commit repository whose objects live in a +// packfile, returning its path and HEAD SHA. Packing is essential rather than +// incidental: reads of loose objects never touch the pooled pack descriptors, so +// an unpacked fixture would make every retention assertion pass even with the +// regression fully present. +func initPackedRepo(t *testing.T) (repoDir, headSHA string) { + t.Helper() + + // go-git cannot repack, so the packing step needs the git CLI. + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not found in PATH; needed to pack objects") + } + + repoDir = t.TempDir() + repo, err := git.PlainInit(repoDir, false) + require.NoError(t, err) + require.NoError(t, disableGPGSign(repo)) + + wt, err := repo.Worktree() + require.NoError(t, err) + require.NoError(t, os.WriteFile(filepath.Join(repoDir, "f.txt"), []byte("hello"), 0o600)) + _, err = wt.Add("f.txt") + require.NoError(t, err) + hash, err := wt.Commit("initial commit", &git.CommitOptions{ + Author: &object.Signature{Name: "John Doe", Email: "john@doe.org", When: time.Now()}, + }) + require.NoError(t, err) + + // A GIT_DIR or GIT_WORK_TREE inherited from the ambient environment would + // redirect gc at a different repository, leaving this fixture unpacked while + // the command still reports success. + cmd := exec.Command("git", "-C", repoDir, "gc", "--quiet") + cmd.Env = slices.DeleteFunc(os.Environ(), func(kv string) bool { + return strings.HasPrefix(kv, "GIT_DIR=") || strings.HasPrefix(kv, "GIT_WORK_TREE=") + }) + out, err := cmd.CombinedOutput() + require.NoError(t, err, "git gc: %s", out) + + // Assert the outcome rather than trusting the command: this is what turns a + // silently vacuous guard into a loud failure. + packs, err := filepath.Glob(filepath.Join(repoDir, ".git", "objects", "pack", "*.pack")) + require.NoError(t, err) + require.NotEmpty(t, packs, + "fixture must be packed or the descriptor guards below are vacuous") + + return repoDir, hash.String() +} + +// countOpenDescriptors counts live descriptors by probing F_GETFD. /proc is +// absent on darwin and /dev/fd enumeration is unreliable there, so probe +// directly rather than reading a pseudo-filesystem. +func countOpenDescriptors() int { + var n int + for fd := 0; fd < 4096; fd++ { + if _, _, errno := syscall.Syscall(syscall.SYS_FCNTL, uintptr(fd), syscall.F_GETFD, 0); errno == 0 { + n++ + } + } + return n +}