From e8ed3ce83f850918800204307434cacefe9f2c65 Mon Sep 17 00:00:00 2001 From: Jordan Dziat Date: Mon, 5 Oct 2026 09:31:51 -0700 Subject: [PATCH 1/6] perf(vcs): read the reviewed commit from the checkout The GitHub provider answered every file read and directory listing with an API request, and a read with no revision named also fetched the pull request and its head commit first. Reads now come from the commit in the checkout when it holds that commit, listings from one ls-tree, and the head commit is asked for once per pull request. Anything the checkout cannot answer goes to the API as before. Refs #167 --- internal/vcs/github.go | 25 +++-- internal/vcs/github_checkout.go | 131 +++++++++++++++++++++++++++ internal/vcs/github_checkout_test.go | 113 +++++++++++++++++++++++ 3 files changed, 263 insertions(+), 6 deletions(-) create mode 100644 internal/vcs/github_checkout.go create mode 100644 internal/vcs/github_checkout_test.go diff --git a/internal/vcs/github.go b/internal/vcs/github.go index ad754aa..b587c66 100644 --- a/internal/vcs/github.go +++ b/internal/vcs/github.go @@ -37,6 +37,9 @@ type GitHub struct { // more than 300 files; with a checkout the diff is taken from git // instead, which has no such limit. Checkout string + + // index caches each commit's tree read from Checkout. + index checkoutIndex } // GitHubOptions configure the provider. @@ -299,11 +302,16 @@ func (g *GitHub) FileContent(ctx context.Context, ref Ref, path string) ([]byte, sha := ref.Head if sha == "" { - pr, err := g.PullRequest(ctx, ref) - if err != nil { + var err error + if sha, err = g.headSHA(ctx, ref); err != nil { return nil, err } - sha = pr.HeadSHA + } + + if tree := g.checkoutTree(ctx, sha); tree != nil && tree.regular(path) { + if data, err := (&Local{Dir: g.Checkout}).FileContent(ctx, Ref{Head: sha}, path); err == nil { + return data, nil + } } // DownloadContents falls back to listing the parent even after a file 404, @@ -361,11 +369,16 @@ func (g *GitHub) ListDir(ctx context.Context, ref Ref, dir string) ([]string, er sha := ref.Head if sha == "" { - pr, err := g.PullRequest(ctx, ref) - if err != nil { + var err error + if sha, err = g.headSHA(ctx, ref); err != nil { return nil, err } - sha = pr.HeadSHA + } + + if tree := g.checkoutTree(ctx, sha); tree != nil { + if names, ok := tree.dirs[strings.Trim(dir, "/")]; ok { + return append([]string(nil), names...), nil + } } file, entries, resp, err := g.client.Repositories.GetContents(ctx, ref.Owner, ref.Repo, strings.Trim(dir, "/"), diff --git a/internal/vcs/github_checkout.go b/internal/vcs/github_checkout.go new file mode 100644 index 0000000..cce2e86 --- /dev/null +++ b/internal/vcs/github_checkout.go @@ -0,0 +1,131 @@ +package vcs + +import ( + "bytes" + "context" + "sort" + "strings" + "sync" +) + +// checkoutTree is one commit's file list, read from the checkout in a single +// git call. Directory listings and file modes are answered from it, so a +// repository of ninety directories costs one process instead of ninety +// requests. +type checkoutTree struct { + modes map[string]string + dirs map[string][]string +} + +// checkoutIndex holds the trees read so far, keyed by commit. +type checkoutIndex struct { + mu sync.Mutex + trees map[string]*checkoutTree + heads map[string]string +} + +// tree returns the commit's tree from the checkout, or nil when the checkout +// does not hold the commit. A nil answer sends the caller to the API. +// +// The commit is read from git objects and never the working tree: the review +// is of the commit named, and the checkout may have something else out. +func (g *GitHub) checkoutTree(ctx context.Context, sha string) *checkoutTree { + if g.Checkout == "" || sha == "" || strings.HasPrefix(sha, "-") { + return nil + } + g.index.mu.Lock() + defer g.index.mu.Unlock() + if t, seen := g.index.trees[sha]; seen { + return t + } + if g.index.trees == nil { + g.index.trees = map[string]*checkoutTree{} + } + t := readCheckoutTree(ctx, g.Checkout, sha) + // A failure caused by a cancelled run is not a fact about the checkout. + if t != nil || ctx.Err() == nil { + g.index.trees[sha] = t + } + return t +} + +func readCheckoutTree(ctx context.Context, dir, sha string) *checkoutTree { + local := &Local{Dir: dir} + if _, err := local.gitRaw(ctx, "cat-file", "-e", sha+"^{commit}"); err != nil { + return nil + } + out, err := local.gitRaw(ctx, "ls-tree", "-r", "-t", "-z", sha) + if err != nil { + return nil + } + t := &checkoutTree{modes: map[string]string{}, dirs: map[string][]string{}} + for _, rec := range bytes.Split(out, []byte{0}) { + meta, name, ok := strings.Cut(string(rec), "\t") + if !ok || name == "" { + continue + } + fields := strings.Fields(meta) + if len(fields) < 2 { + continue + } + parent := "" + if i := strings.LastIndex(name, "/"); i >= 0 { + parent = name[:i] + } + base := name[strings.LastIndex(name, "/")+1:] + switch fields[1] { + case "tree": + t.dirs[parent] = append(t.dirs[parent], base+"/") + if _, has := t.dirs[name]; !has { + t.dirs[name] = nil + } + case "blob": + t.modes[name] = fields[0] + // Symlinks are left out of listings, as the API leaves them out. + if fields[0] == "100644" || fields[0] == "100755" { + t.dirs[parent] = append(t.dirs[parent], base) + } + } + } + for _, names := range t.dirs { + sort.Strings(names) + } + return t +} + +// regular reports whether path is a regular file in the tree. A symlink or a +// submodule is not source, and the API path refuses them with its own error. +func (t *checkoutTree) regular(path string) bool { + mode := t.modes[path] + return mode == "100644" || mode == "100755" +} + +// headSHA is the pull request's head commit, asked for once per pull request. +// +// A read that names no revision used to fetch the pull request and its head +// commit again for every file, so a review reading four hundred files made +// well over a thousand requests. It also meant a push during the run could +// split the reads across two commits. +func (g *GitHub) headSHA(ctx context.Context, ref Ref) (string, error) { + key := ref.String() + g.index.mu.Lock() + sha, ok := g.index.heads[key] + g.index.mu.Unlock() + if ok { + return sha, nil + } + pr, err := g.PullRequest(ctx, ref) + if err != nil { + return "", err + } + if pr.HeadSHA == "" { + return "", nil + } + g.index.mu.Lock() + if g.index.heads == nil { + g.index.heads = map[string]string{} + } + g.index.heads[key] = pr.HeadSHA + g.index.mu.Unlock() + return pr.HeadSHA, nil +} diff --git a/internal/vcs/github_checkout_test.go b/internal/vcs/github_checkout_test.go new file mode 100644 index 0000000..3b78b5d --- /dev/null +++ b/internal/vcs/github_checkout_test.go @@ -0,0 +1,113 @@ +package vcs + +import ( + "context" + "encoding/json" + "net/http" + "os" + "path/filepath" + "slices" + "strings" + "sync/atomic" + "testing" +) + +// countingGitHub serves one head commit and counts what it was asked for. +func countingGitHub(t *testing.T, sha string, pulls, contents *atomic.Int64) *GitHub { + t.Helper() + return newFakeGitHub(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.Contains(r.URL.Path, "/pulls/7"): + pulls.Add(1) + _ = json.NewEncoder(w).Encode(map[string]any{"number": 7, "head": map[string]any{"sha": sha}}) + case strings.Contains(r.URL.Path, "/contents/"): + contents.Add(1) + http.NotFound(w, r) + default: + http.NotFound(w, r) + } + }) +} + +func headOf(t *testing.T, dir string) string { + t.Helper() + return strings.TrimSpace(gitIn(t, dir, "rev-parse", "HEAD")) +} + +// The checkout answers reads of the commit under review, so a repository's +// size no longer sets the number of requests. The pull request is asked for +// once however many files follow. +func TestGitHubReadsTheReviewedCommitFromTheCheckout(t *testing.T) { + dir := newRepo(t) + write(t, dir, "pkg/b.go", "package pkg\n") + gitIn(t, dir, "add", "-A") + gitIn(t, dir, "commit", "-qm", "second") + var pulls, contents atomic.Int64 + gh := countingGitHub(t, headOf(t, dir), &pulls, &contents) + gh.Checkout = dir + + for range 5 { + got, err := gh.FileContent(context.Background(), testRef(), "pkg/b.go") + if err != nil || string(got) != "package pkg\n" { + t.Fatalf("FileContent = %q, %v", got, err) + } + } + names, err := gh.ListDir(context.Background(), testRef(), "") + if err != nil || !slices.Equal(names, []string{"a.go", "pkg/"}) { + t.Fatalf("ListDir = %v, %v", names, err) + } + if pulls.Load() != 1 || contents.Load() != 0 { + t.Fatalf("pull request asked %d times and contents %d times, want 1 and 0", pulls.Load(), contents.Load()) + } +} + +// The commit is read from git objects. A checkout with something else out, or +// an edited file, must not change what is reviewed. +func TestGitHubCheckoutReadsIgnoreTheWorkingTree(t *testing.T) { + dir := newRepo(t) + sha := headOf(t, dir) + write(t, dir, "a.go", "package edited\n") + write(t, dir, "untracked.go", "package untracked\n") + var pulls, contents atomic.Int64 + gh := countingGitHub(t, sha, &pulls, &contents) + gh.Checkout = dir + + got, err := gh.FileContent(context.Background(), testRef(), "a.go") + if err != nil || strings.Contains(string(got), "edited") { + t.Fatalf("read %q, %v: the working tree leaked into the review", got, err) + } + names, _ := gh.ListDir(context.Background(), testRef(), "") + if slices.Contains(names, "untracked.go") { + t.Fatalf("ListDir = %v, want only what the commit holds", names) + } +} + +// A checkout that lacks the commit, or a file the checkout cannot serve as +// source, goes to the API as before. +func TestGitHubFallsBackToTheAPIWhenTheCheckoutCannotAnswer(t *testing.T) { + dir := newRepo(t) + if err := os.Symlink("a.go", filepath.Join(dir, "link.go")); err != nil { + t.Skip("symlinks unavailable") + } + gitIn(t, dir, "add", "-A") + gitIn(t, dir, "commit", "-qm", "link") + + var pulls, contents atomic.Int64 + gh := countingGitHub(t, headOf(t, dir), &pulls, &contents) + gh.Checkout = dir + if _, err := gh.FileContent(context.Background(), testRef(), "link.go"); err == nil { + t.Fatal("a symlink was served as source") + } + if contents.Load() != 1 { + t.Fatalf("contents asked %d times for a symlink, want 1: the API must refuse it", contents.Load()) + } + + var pulls2, contents2 atomic.Int64 + absent := countingGitHub(t, strings.Repeat("0", 40), &pulls2, &contents2) + absent.Checkout = dir + _, _ = absent.FileContent(context.Background(), testRef(), "a.go") + _, _ = absent.ListDir(context.Background(), testRef(), "") + if contents2.Load() != 2 { + t.Fatalf("contents asked %d times with a commit the checkout lacks, want 2", contents2.Load()) + } +} From a0e33eb38efc9e7097707cbe7239242a04a390fc Mon Sep 17 00:00:00 2001 From: Jordan Dziat Date: Mon, 5 Oct 2026 09:36:41 -0700 Subject: [PATCH 2/6] fix(vcs): list symlinks and read trees outside the lock The contents API reports a symlink as a file, so a listing from the checkout now includes it; reading one is still refused. The index lock no longer spans the git calls, so a slow tree read does not block reads of other commits. --- internal/vcs/github_checkout.go | 32 ++++++++++++++++++---------- internal/vcs/github_checkout_test.go | 21 ++++++++++++++++++ 2 files changed, 42 insertions(+), 11 deletions(-) diff --git a/internal/vcs/github_checkout.go b/internal/vcs/github_checkout.go index cce2e86..3af9764 100644 --- a/internal/vcs/github_checkout.go +++ b/internal/vcs/github_checkout.go @@ -18,6 +18,10 @@ type checkoutTree struct { } // checkoutIndex holds the trees read so far, keyed by commit. +// +// The lock guards the maps only. A tree is built outside it and then +// published, so one slow git call does not stall reads of other commits; two +// callers racing on an uncached commit read it twice and keep one. type checkoutIndex struct { mu sync.Mutex trees map[string]*checkoutTree @@ -34,18 +38,25 @@ func (g *GitHub) checkoutTree(ctx context.Context, sha string) *checkoutTree { return nil } g.index.mu.Lock() - defer g.index.mu.Unlock() - if t, seen := g.index.trees[sha]; seen { + t, seen := g.index.trees[sha] + g.index.mu.Unlock() + if seen { return t } + t = readCheckoutTree(ctx, g.Checkout, sha) + // A failure caused by a cancelled run is not a fact about the checkout. + if t == nil && ctx.Err() != nil { + return nil + } + g.index.mu.Lock() + defer g.index.mu.Unlock() + if kept, raced := g.index.trees[sha]; raced { + return kept + } if g.index.trees == nil { g.index.trees = map[string]*checkoutTree{} } - t := readCheckoutTree(ctx, g.Checkout, sha) - // A failure caused by a cancelled run is not a fact about the checkout. - if t != nil || ctx.Err() == nil { - g.index.trees[sha] = t - } + g.index.trees[sha] = t return t } @@ -81,10 +92,9 @@ func readCheckoutTree(ctx context.Context, dir, sha string) *checkoutTree { } case "blob": t.modes[name] = fields[0] - // Symlinks are left out of listings, as the API leaves them out. - if fields[0] == "100644" || fields[0] == "100755" { - t.dirs[parent] = append(t.dirs[parent], base) - } + // The contents API types a symlink as a file, so it is listed here + // too. Only reading one is refused, in regular below. + t.dirs[parent] = append(t.dirs[parent], base) } } for _, names := range t.dirs { diff --git a/internal/vcs/github_checkout_test.go b/internal/vcs/github_checkout_test.go index 3b78b5d..bab8ed0 100644 --- a/internal/vcs/github_checkout_test.go +++ b/internal/vcs/github_checkout_test.go @@ -111,3 +111,24 @@ func TestGitHubFallsBackToTheAPIWhenTheCheckoutCannotAnswer(t *testing.T) { t.Fatalf("contents asked %d times with a commit the checkout lacks, want 2", contents2.Load()) } } + +// The contents API types a symlink as a file, measured against git/git's +// RelNotes. A directory must list the same names whichever side answers. +func TestGitHubListsASymlinkWhetherTheCheckoutOrTheAPIAnswers(t *testing.T) { + dir := newRepo(t) + if err := os.Symlink("a.go", filepath.Join(dir, "link.go")); err != nil { + t.Skip("symlinks unavailable") + } + gitIn(t, dir, "add", "-A") + gitIn(t, dir, "commit", "-qm", "link") + var pulls, contents atomic.Int64 + gh := countingGitHub(t, headOf(t, dir), &pulls, &contents) + gh.Checkout = dir + + names, err := gh.ListDir(context.Background(), testRef(), "") + if err != nil || !slices.Equal(names, []string{"a.go", "link.go"}) { + t.Fatalf("ListDir = %v, %v, want the symlink listed beside its target", names, err) + } +} + +// A checkout that lacks the commit, or a file the checkout cannot serve as From 38b75ce743f2d88c420e1cdacf72f4593baa515d Mon Sep 17 00:00:00 2001 From: Jordan Dziat Date: Mon, 5 Oct 2026 09:41:19 -0700 Subject: [PATCH 3/6] fix(vcs): list submodules and say when the checkout is unusable The contents API types a submodule as a file, so the checkout listing includes it and sends its read to the API. A checkout that cannot supply the commit now logs once per commit instead of quietly paying for every read. --- internal/vcs/github_checkout.go | 15 +++++++++++++++ internal/vcs/github_checkout_test.go | 20 +++++++++++++++++++- 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/internal/vcs/github_checkout.go b/internal/vcs/github_checkout.go index 3af9764..2d2ce45 100644 --- a/internal/vcs/github_checkout.go +++ b/internal/vcs/github_checkout.go @@ -3,6 +3,7 @@ package vcs import ( "bytes" "context" + "log/slog" "sort" "strings" "sync" @@ -48,6 +49,12 @@ func (g *GitHub) checkoutTree(ctx context.Context, sha string) *checkoutTree { if t == nil && ctx.Err() != nil { return nil } + if t == nil { + // Once per commit, because the miss is cached below. Without it a wrong + // Checkout path looks like a slow review, not a misconfiguration. + slog.Warn("checkout does not hold the reviewed commit; reading it through the API", + "checkout", g.Checkout, "commit", short(sha)) + } g.index.mu.Lock() defer g.index.mu.Unlock() if kept, raced := g.index.trees[sha]; raced { @@ -95,6 +102,11 @@ func readCheckoutTree(ctx context.Context, dir, sha string) *checkoutTree { // The contents API types a symlink as a file, so it is listed here // too. Only reading one is refused, in regular below. t.dirs[parent] = append(t.dirs[parent], base) + case "commit": + // A submodule is typed as a file by the contents API as well, and has + // no blob here to read. + t.modes[name] = fields[0] + t.dirs[parent] = append(t.dirs[parent], base) } } for _, names := range t.dirs { @@ -111,6 +123,9 @@ func (t *checkoutTree) regular(path string) bool { } // headSHA is the pull request's head commit, asked for once per pull request. +// It serves file and directory reads only. The approval check, Propose and its +// final re-read call PullRequest themselves, so a push during the run is still +// caught there. // // A read that names no revision used to fetch the pull request and its head // commit again for every file, so a review reading four hundred files made diff --git a/internal/vcs/github_checkout_test.go b/internal/vcs/github_checkout_test.go index bab8ed0..54fd3bc 100644 --- a/internal/vcs/github_checkout_test.go +++ b/internal/vcs/github_checkout_test.go @@ -131,4 +131,22 @@ func TestGitHubListsASymlinkWhetherTheCheckoutOrTheAPIAnswers(t *testing.T) { } } -// A checkout that lacks the commit, or a file the checkout cannot serve as +// The contents API types a submodule as a file, measured against apache/arrow's +// cpp/submodules/parquet-testing. The checkout must list it and must not claim +// to read it, since there is no blob behind it. +func TestGitHubListsASubmoduleButDoesNotReadIt(t *testing.T) { + dir := newRepo(t) + gitIn(t, dir, "update-index", "--add", "--cacheinfo", "160000,"+headOf(t, dir)+",vendor") + gitIn(t, dir, "commit", "-qm", "submodule") + var pulls, contents atomic.Int64 + gh := countingGitHub(t, headOf(t, dir), &pulls, &contents) + gh.Checkout = dir + + names, err := gh.ListDir(context.Background(), testRef(), "") + if err != nil || !slices.Equal(names, []string{"a.go", "vendor"}) { + t.Fatalf("ListDir = %v, %v, want the submodule listed as a file", names, err) + } + if _, err := gh.FileContent(context.Background(), testRef(), "vendor"); err == nil || contents.Load() != 1 { + t.Fatalf("FileContent = %v after %d API reads, want the API to answer and refuse", err, contents.Load()) + } +} From 6d7d3338856077390af953b753f3cf9a8b75dbb1 Mon Sep 17 00:00:00 2001 From: Jordan Dziat Date: Mon, 5 Oct 2026 09:47:33 -0700 Subject: [PATCH 4/6] fix(vcs): retry the checkout after a transient git failure Only a commit the checkout does not hold is remembered as a miss. A failed ls-tree on a commit that is there is retried on the next read and logged once, and the isolation test now fails if the checkout did not answer. --- internal/vcs/github_checkout.go | 59 ++++++++++++++++++++-------- internal/vcs/github_checkout_test.go | 35 ++++++++++++++++- 2 files changed, 76 insertions(+), 18 deletions(-) diff --git a/internal/vcs/github_checkout.go b/internal/vcs/github_checkout.go index 2d2ce45..8450f7b 100644 --- a/internal/vcs/github_checkout.go +++ b/internal/vcs/github_checkout.go @@ -3,6 +3,7 @@ package vcs import ( "bytes" "context" + "fmt" "log/slog" "sort" "strings" @@ -27,10 +28,18 @@ type checkoutIndex struct { mu sync.Mutex trees map[string]*checkoutTree heads map[string]string + // warned holds the commits already reported, so a checkout that keeps + // failing logs once and not once per file. + warned map[string]bool } -// tree returns the commit's tree from the checkout, or nil when the checkout -// does not hold the commit. A nil answer sends the caller to the API. +// checkoutTree returns the commit's tree from the checkout, or nil when the +// checkout cannot supply it. A nil answer sends the caller to the API. +// +// A commit the checkout does not hold is remembered, because asking again +// cannot change the answer. A git call that failed after the commit was found +// is not remembered: it may be transient, and caching it would send every +// later read of the commit through the API. // // The commit is read from git objects and never the working tree: the review // is of the commit named, and the checkout may have something else out. @@ -44,16 +53,15 @@ func (g *GitHub) checkoutTree(ctx context.Context, sha string) *checkoutTree { if seen { return t } - t = readCheckoutTree(ctx, g.Checkout, sha) - // A failure caused by a cancelled run is not a fact about the checkout. - if t == nil && ctx.Err() != nil { - return nil - } - if t == nil { - // Once per commit, because the miss is cached below. Without it a wrong - // Checkout path looks like a slow review, not a misconfiguration. - slog.Warn("checkout does not hold the reviewed commit; reading it through the API", - "checkout", g.Checkout, "commit", short(sha)) + t, held, err := readCheckoutTree(ctx, g.Checkout, sha) + if err != nil { + if ctx.Err() != nil { + return nil + } + g.warnOnce(sha, "checkout could not be read; reading through the API", err) + if held { + return nil + } } g.index.mu.Lock() defer g.index.mu.Unlock() @@ -67,16 +75,33 @@ func (g *GitHub) checkoutTree(ctx context.Context, sha string) *checkoutTree { return t } -func readCheckoutTree(ctx context.Context, dir, sha string) *checkoutTree { +// warnOnce logs a checkout problem the first time it is seen for a commit. +func (g *GitHub) warnOnce(sha, msg string, err error) { + g.index.mu.Lock() + seen := g.index.warned[sha] + if g.index.warned == nil { + g.index.warned = map[string]bool{} + } + g.index.warned[sha] = true + g.index.mu.Unlock() + if !seen { + slog.Warn(msg, "checkout", g.Checkout, "commit", short(sha), "err", err) + } +} + +// readCheckoutTree reads one commit's tree. held reports whether the checkout +// has the commit: false with an error is a miss to remember, true with an +// error is a failure that should be retried. +func readCheckoutTree(ctx context.Context, dir, sha string) (t *checkoutTree, held bool, err error) { local := &Local{Dir: dir} if _, err := local.gitRaw(ctx, "cat-file", "-e", sha+"^{commit}"); err != nil { - return nil + return nil, false, fmt.Errorf("commit not in checkout: %w", err) } out, err := local.gitRaw(ctx, "ls-tree", "-r", "-t", "-z", sha) if err != nil { - return nil + return nil, true, fmt.Errorf("list tree: %w", err) } - t := &checkoutTree{modes: map[string]string{}, dirs: map[string][]string{}} + t = &checkoutTree{modes: map[string]string{}, dirs: map[string][]string{}} for _, rec := range bytes.Split(out, []byte{0}) { meta, name, ok := strings.Cut(string(rec), "\t") if !ok || name == "" { @@ -112,7 +137,7 @@ func readCheckoutTree(ctx context.Context, dir, sha string) *checkoutTree { for _, names := range t.dirs { sort.Strings(names) } - return t + return t, true, nil } // regular reports whether path is a regular file in the tree. A symlink or a diff --git a/internal/vcs/github_checkout_test.go b/internal/vcs/github_checkout_test.go index 54fd3bc..709decd 100644 --- a/internal/vcs/github_checkout_test.go +++ b/internal/vcs/github_checkout_test.go @@ -76,12 +76,45 @@ func TestGitHubCheckoutReadsIgnoreTheWorkingTree(t *testing.T) { if err != nil || strings.Contains(string(got), "edited") { t.Fatalf("read %q, %v: the working tree leaked into the review", got, err) } - names, _ := gh.ListDir(context.Background(), testRef(), "") + names, err := gh.ListDir(context.Background(), testRef(), "") + if err != nil || contents.Load() != 0 { + t.Fatalf("ListDir = %v, %v after %d API reads: the checkout did not answer, so the isolation below proves nothing", names, err, contents.Load()) + } if slices.Contains(names, "untracked.go") { t.Fatalf("ListDir = %v, want only what the commit holds", names) } } +// A git failure after the commit was found may be transient, so it must not be +// remembered. The tree object is hidden for one read and put back; the second +// read has to come from the checkout, not stay on the API for the rest of the +// run. +func TestGitHubRetriesTheCheckoutAfterATransientFailure(t *testing.T) { + dir := newRepo(t) + sha := headOf(t, dir) + tree := strings.TrimSpace(gitIn(t, dir, "rev-parse", sha+"^{tree}")) + object := filepath.Join(dir, ".git", "objects", tree[:2], tree[2:]) + hidden := object + ".hidden" + if err := os.Rename(object, hidden); err != nil { + t.Skipf("tree object is not loose: %v", err) + } + var pulls, contents atomic.Int64 + gh := countingGitHub(t, sha, &pulls, &contents) + gh.Checkout = dir + + _, _ = gh.ListDir(context.Background(), testRef(), "") + if contents.Load() != 1 { + t.Fatalf("API asked %d times while the checkout was broken, want 1", contents.Load()) + } + if err := os.Rename(hidden, object); err != nil { + t.Fatal(err) + } + names, err := gh.ListDir(context.Background(), testRef(), "") + if err != nil || !slices.Equal(names, []string{"a.go"}) || contents.Load() != 1 { + t.Fatalf("ListDir = %v, %v after %d API reads, want the recovered checkout to answer", names, err, contents.Load()) + } +} + // A checkout that lacks the commit, or a file the checkout cannot serve as // source, goes to the API as before. func TestGitHubFallsBackToTheAPIWhenTheCheckoutCannotAnswer(t *testing.T) { From 2d26daa859e04586367466baeadaf19b2cf1894a Mon Sep 17 00:00:00 2001 From: Jordan Dziat Date: Mon, 5 Oct 2026 09:54:47 -0700 Subject: [PATCH 5/6] docs(findings): record the design read from the checkout --- docs/findings.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/docs/findings.md b/docs/findings.md index ba61fab..ba0a43a 100644 --- a/docs/findings.md +++ b/docs/findings.md @@ -2640,6 +2640,17 @@ widely, 4s to 2m7s over the same runs with a median near 1m25s, so it is still the larger part of a review. What is left to measure is the fixed pre-batch step as a whole, not this read alone. +The read still cost one forge request per file and directory, about 490, plus +two more per file because a read that named no revision fetched the pull +request and its head commit first. The Action already holds the history, so +the provider now answers from the checkout when it has the reviewed commit: +file contents from the commit's objects, listings from one `git ls-tree`, the +head commit once per pull request. Two CI runs of the same tree, 93 +directories and 398 files, read in 290ms and 285ms, against 16.6s in the +concurrent run before it and 1m6s before that. `design assembled` fell from +17.1s to 0.9s. Two runs are not a distribution; the range from the earlier +thirteen is why 16s was reported as a median and not a best case. + `TestDesignSourceCaptureOverlapsRequestsAndStaysWithinTheLimit` pins the overlap as peak requests in flight rather than wall clock, so a loaded runner cannot fail it without a regression. Three siblings pin what the overlap must From 37c5922a910bb03b54f705e47ebb0065f70d163b Mon Sep 17 00:00:00 2001 From: Jordan Dziat Date: Tue, 6 Oct 2026 15:40:21 -0700 Subject: [PATCH 6/6] docs(findings): count the files at the reviewed commit --- docs/findings.md | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/docs/findings.md b/docs/findings.md index ba0a43a..031e5f5 100644 --- a/docs/findings.md +++ b/docs/findings.md @@ -2632,10 +2632,10 @@ each read was a separate forge request, made one at a time. The same tree holds the answers, so the requests did not depend on each other. Listing the tree and reading its files now overlap, at most eight requests in -flight. Across the thirteen CI runs after the change the same tree, all 93 -directories and 396 files, reads in a median of 16.0s, from 7.3s to 40.5s. The -widest values land on the most loaded runners, so the range is the number to -keep; the baseline is the one pre-change run at 1m6s. Model batch time swings as +flight. Across the thirteen CI runs after the change the same 93 directories +read in a median of 16.0s, from 7.3s to 40.5s. The widest values land on the +most loaded runners, so the range is the number to keep; the baseline is the +one pre-change run at 1m6s. Model batch time swings as widely, 4s to 2m7s over the same runs with a median near 1m25s, so it is still the larger part of a review. What is left to measure is the fixed pre-batch step as a whole, not this read alone. @@ -2645,11 +2645,14 @@ two more per file because a read that named no revision fetched the pull request and its head commit first. The Action already holds the history, so the provider now answers from the checkout when it has the reviewed commit: file contents from the commit's objects, listings from one `git ls-tree`, the -head commit once per pull request. Two CI runs of the same tree, 93 -directories and 398 files, read in 290ms and 285ms, against 16.6s in the -concurrent run before it and 1m6s before that. `design assembled` fell from -17.1s to 0.9s. Two runs are not a distribution; the range from the earlier -thirteen is why 16s was reported as a median and not a best case. +head commit once per pull request. The read covers whatever the reviewed +commit holds, so the file count rises as this branch adds tests: the +concurrent runs held 396 or 397 files, the checkout runs 398 or 399. Those +five reads covered all 93 directories in 290ms, 196ms, 277ms, 285ms and +165ms, against 16.6s in the concurrent run before it and 1m6s before that. +`design assembled` fell from 17.1s to a median near 0.9s. Five runs are not a +distribution; the range from the earlier thirteen is why 16s was reported as a +median and not a best case. `TestDesignSourceCaptureOverlapsRequestsAndStaysWithinTheLimit` pins the overlap as peak requests in flight rather than wall clock, so a loaded runner