From 8400cafc6ac04b6c7c6b4aea19d1dad28ba256a1 Mon Sep 17 00:00:00 2001 From: Neureka Date: Thu, 10 Sep 2026 07:27:31 -0700 Subject: [PATCH 01/15] fix(lfs): stop endpoint and path quirks from failing repository backups Three LFS conditions aborted a whole repository snapshot even though the git mirror itself cloned and uploaded fine: - Forges route only the suffixed repository path to their LFS service, so requesting /info/lfs without .git made GitHub and GitLab answer with an HTML 422 page ("batch request failed with status 422"). The endpoint now carries the repository's .git suffix whenever the configured remote omits it. - The pointer scan used object.TreeWalker, which rejects any tree entry whose name is unsuitable for materialising a working tree on the host, such as a path containing a backslash on Windows. Trees are now enumerated as raw tree objects, so an unusual file name can no longer abort the scan. - A batch the endpoint rejects outright failed the entire LFS mirror. Pointers are now submitted in batches of 100 and a rejected batch is retried object by object: one object the server refuses, or a request above its object limit, costs that object alone, while objects the endpoint will not serve are reported as skipped so the rest of the mirror still reaches storage. --- internal/lfs/batch.go | 34 ++++- internal/lfs/lfs.go | 170 ++++++++++++++++++++++++- internal/lfs/lfs_test.go | 269 ++++++++++++++++++++++++++++++++++++++- internal/lfs/scan.go | 102 +++++++++------ 4 files changed, 524 insertions(+), 51 deletions(-) diff --git a/internal/lfs/batch.go b/internal/lfs/batch.go index ba2de10..9e3be55 100644 --- a/internal/lfs/batch.go +++ b/internal/lfs/batch.go @@ -6,6 +6,7 @@ import ( "crypto/sha256" "encoding/hex" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -53,6 +54,30 @@ type batchResponse struct { Objects []batchResponseObject `json:"objects"` } +// errBatchRejected marks a batch request the endpoint answered with an +// unexpected status instead of scheduling the objects. Callers narrow such a +// request down to individual objects, because a rejection is usually one +// object's fault (the endpoint's object limit, a stale pointer) rather than the +// whole batch's. +var errBatchRejected = errors.New("batch request rejected by the endpoint") + +// errObjectUnavailable reports that the endpoint answered the batch request but +// will not serve one of the objects it named, so the object cannot be mirrored. +var errObjectUnavailable = errors.New("lfs object unavailable on the remote") + +// batchObjectError is one object the endpoint refused: a pointer kept in the +// repository after its object was removed or garbage-collected on the server. +type batchObjectError struct { + oid string + message string +} + +func (e *batchObjectError) Error() string { + return fmt.Sprintf("LFS object %s unavailable: %s", shortOID(e.oid), e.message) +} + +func (e *batchObjectError) Unwrap() error { return errObjectUnavailable } + // batchClient performs the LFS batch protocol over HTTP. type batchClient struct { client *http.Client @@ -69,7 +94,8 @@ const lfsMediaType = "application/vnd.git-lfs+json" // batch submits every pointer for download scheduling. A 403 or 404 from the // endpoint means the remote has Git LFS switched off, which callers treat as -// an expected skip rather than a failure. +// an expected skip rather than a failure. Any other unexpected status is +// reported as errBatchRejected so callers can retry the pointers individually. func (c *batchClient) batch(ctx context.Context, endpoint, username, password string, pointers []pointer) ([]batchResponseObject, error) { body, err := json.Marshal(batchRequest{ Operation: "download", @@ -102,7 +128,7 @@ func (c *batchClient) batch(ctx context.Context, endpoint, username, password st case http.StatusForbidden, http.StatusNotFound: return nil, ErrDisabled default: - return nil, fmt.Errorf("batch request failed with status %d", response.StatusCode) + return nil, fmt.Errorf("%w: status %d", errBatchRejected, response.StatusCode) } var decoded batchResponse @@ -130,14 +156,14 @@ func downloadObjects( return err } if object.Error != nil { - return fmt.Errorf("LFS object %s unavailable: %s", shortOID(object.OID), object.Error.Message) + return &batchObjectError{oid: object.OID, message: object.Error.Message} } action := object.Actions["download"] if action == nil || action.Href == "" { // The server knows the object but scheduled nothing; without a // href there is nothing this client can do beyond reporting it. - return fmt.Errorf("LFS object %s has no download action", shortOID(object.OID)) + return &batchObjectError{oid: object.OID, message: "no download action was scheduled"} } if err := downloadObject(ctx, client.client, store, endpointHost, username, password, object.OID, action); err != nil { diff --git a/internal/lfs/lfs.go b/internal/lfs/lfs.go index 8e6f589..818e3b7 100644 --- a/internal/lfs/lfs.go +++ b/internal/lfs/lfs.go @@ -14,6 +14,7 @@ import ( "errors" "fmt" "io" + "log/slog" "net/url" "path/filepath" "strings" @@ -66,13 +67,153 @@ func (f *Fetcher) FetchAll(ctx context.Context, repositoryPath, remoteURL, usern return nil } + store := objectStoreDir(repository, repositoryPath) + return f.fetchBatches(ctx, store, endpoint, username, password, pointers) +} + +// batchObjectLimit is how many pointers one batch request submits. Git LFS +// clients use the same limit; a forge may reject a request that exceeds its own +// smaller limit, which fetchChunk then narrows down. +const batchObjectLimit = 100 + +// fetchBatches downloads every pointer's object, splitting the pointers into +// batch requests. A batch the endpoint rejects is narrowed down object by +// object, so one object the server will not serve — a stale pointer, or a +// request the server considers too large — costs that object alone instead of +// the repository's entire LFS mirror. Failing to mirror an object that exists +// still errors the fetch. +func (f *Fetcher) fetchBatches( + ctx context.Context, + store, endpoint, username, password string, + pointers []pointer, +) error { + var unavailable, rejected int + var firstFailure, firstErr error + for start := 0; start < len(pointers); start += batchObjectLimit { + end := min(start+batchObjectLimit, len(pointers)) + if err := ctx.Err(); err != nil { + return err + } + + outcome, err := f.fetchChunk(ctx, store, endpoint, username, password, pointers[start:end]) + unavailable += outcome.unavailable + rejected += outcome.rejected + if outcome.skipped != nil && firstFailure == nil { + firstFailure = outcome.skipped + } + if err != nil && firstErr == nil { + firstErr = err + } + } + + skipped := unavailable + rejected + if skipped > 0 { + // The mirrored repository stays usable, but its LFS content is not + // complete, so say so once per repository at warn level and keep the + // per-object reasons for debug output. + slog.Warn("Some Git LFS objects could not be fetched; the repository was mirrored without them.", + "endpoint", endpoint, "objectsMissing", skipped, "objectsRequested", len(pointers)) + slog.Debug("Git LFS objects that could not be fetched.", + "endpoint", endpoint, "reason", firstFailure.Error()) + } + if firstErr != nil { + return firstErr + } + if skipped > 0 { + return fmt.Errorf("%d of %d LFS objects could not be fetched: %w", skipped, len(pointers), firstFailure) + } + return nil +} + +// chunkOutcome reports what one batch request could not fetch: objects the +// endpoint answered for but will not serve, and objects whose own batch request +// the endpoint rejected outright. skipped, when set, names the first of them. +type chunkOutcome struct { + unavailable int + rejected int + skipped error +} + +// fetchChunk submits one batch request for pointers and downloads what it +// schedules. A rejected batch falls back to requesting the same pointers +// individually: the rejection is usually one poisonous object, and the remaining +// objects then still reach the mirror. A rejection covering more than half of the +// chunk comes back as an error, because a failure that broad is the endpoint's, +// not one object's. +func (f *Fetcher) fetchChunk( + ctx context.Context, + store, endpoint, username, password string, + pointers []pointer, +) (chunkOutcome, error) { objects, err := f.client.batch(ctx, endpoint, username, password, pointers) - if err != nil { - return err + if err == nil { + return f.downloadChunk(ctx, store, endpoint, username, password, objects) + } + if !errors.Is(err, errBatchRejected) { + return chunkOutcome{}, err } - store := objectStoreDir(repository, repositoryPath) - return downloadObjects(ctx, f.client, store, endpoint, username, password, objects) + outcome := chunkOutcome{} + for index := range pointers { + if ctx.Err() != nil { + return outcome, ctx.Err() + } + + single, err := f.client.batch(ctx, endpoint, username, password, pointers[index:index+1]) + if err != nil { + if !errors.Is(err, errBatchRejected) { + return outcome, err + } + // This object's own request was rejected too, so the endpoint + // refuses to serve it at all. + outcome.rejected++ + if outcome.skipped == nil { + outcome.skipped = fmt.Errorf("%w: LFS object %s download request rejected: %w", errObjectUnavailable, shortOID(pointers[index].oid), err) + } + continue + } + + served, err := f.downloadChunk(ctx, store, endpoint, username, password, single) + outcome.unavailable += served.unavailable + outcome.rejected += served.rejected + if outcome.skipped == nil { + outcome.skipped = served.skipped + } + if err != nil { + return outcome, err + } + } + + if outcome.rejected > len(pointers)/2 { + return outcome, fmt.Errorf("batch request rejected for %d of %d objects: %w", outcome.rejected, len(pointers), err) + } + return outcome, nil +} + +// downloadChunk streams every object the batch scheduled. An object the endpoint +// refuses to serve is recorded and skipped so its siblings still reach the +// mirror; any other failure — a corrupt download, a broken connection — comes +// back as an error. +func (f *Fetcher) downloadChunk( + ctx context.Context, + store, endpoint, username, password string, + objects []batchResponseObject, +) (chunkOutcome, error) { + var outcome chunkOutcome + for _, object := range objects { + if object.Error != nil { + outcome.unavailable++ + if outcome.skipped == nil { + outcome.skipped = fmt.Errorf("%w: LFS object %s is unavailable: %s", + errObjectUnavailable, shortOID(object.OID), object.Error.Message) + } + continue + } + if err := downloadObjects(ctx, f.client, store, endpoint, username, password, []batchResponseObject{object}); err != nil { + return outcome, err + } + } + return outcome, nil } // resolveEndpoint determines the LFS API root: an lfs.url override from the @@ -84,12 +225,11 @@ func (f *Fetcher) FetchAll(ctx context.Context, repositoryPath, remoteURL, usern // .lfsconfig is best-effort — any failure falls back to the derived endpoint, // matching the common deployment. func resolveEndpoint(repository *git.Repository, remoteURL string) (string, error) { - remote := strings.TrimSuffix(remoteURL, "/") - parsed, ok := paths.ParseHTTPURL(remote) + parsed, ok := paths.ParseHTTPURL(remoteURL) if !ok { return "", fmt.Errorf("unsupported remote URL '%s': only http and https are allowed", redactedURL(remoteURL)) } - endpoint := remote + "/info/lfs" + endpoint := defaultEndpoint(parsed) config, err := readLFSConfig(repository) if err != nil || config == nil { @@ -114,6 +254,22 @@ func resolveEndpoint(repository *git.Repository, remoteURL string) (string, erro return endpoint, nil } +// defaultEndpoint derives the remote's standard LFS API root, including the +// repository's .git suffix. +// +// Git LFS clients request "[/info/lfs]" with the suffix their remote +// uses, and forges route only the suffixed path to their LFS service: GitHub +// and GitLab answer a suffix-less /info/lfs with 422, while Forgejo and Gitea +// accept either form. The mirror's remote has no .git suffix because a config +// URL rarely carries one, so the suffix is added here whenever it is absent. +func defaultEndpoint(remoteURL *url.URL) string { + path := strings.TrimSuffix(remoteURL.Path, "/") + if !strings.HasSuffix(path, ".git") { + path += ".git" + } + return remoteURL.Scheme + "://" + remoteURL.Host + path + "/info/lfs" +} + // isSchemeDefaultPort reports whether the URL's explicit port equals its // scheme's default (http 80, https 443). func isSchemeDefaultPort(u *url.URL) bool { diff --git a/internal/lfs/lfs_test.go b/internal/lfs/lfs_test.go index 60bfd48..86d8676 100644 --- a/internal/lfs/lfs_test.go +++ b/internal/lfs/lfs_test.go @@ -18,6 +18,8 @@ import ( "time" "github.com/go-git/go-git/v5" + "github.com/go-git/go-git/v5/plumbing" + "github.com/go-git/go-git/v5/plumbing/filemode" "github.com/go-git/go-git/v5/plumbing/object" ) @@ -70,6 +72,12 @@ type fakeLFSServer struct { downloads map[string]int // batchStatus, when non-zero, is the status returned for batch requests. batchStatus int + // refusedOIDs are answered with a per-object error instead of a download + // action, like a pointer whose object is gone from the server. + refusedOIDs []string + // rejectWhen, when set, decides the status of each batch request so a test + // can model an endpoint that rejects some request shapes. + rejectWhen func(batchRequest) int // corruptDownload serves wrong bytes for every object. corruptDownload bool // batchPath overrides the expected batch path (lfs.url override tests). @@ -104,6 +112,11 @@ func (s *fakeLFSServer) handleBatch(w http.ResponseWriter, r *http.Request) { s.mu.Lock() s.batchCalls++ status, batchPath := s.batchStatus, s.batchPath + rejectWhen := s.rejectWhen + refused := make(map[string]struct{}, len(s.refusedOIDs)) + for _, oid := range s.refusedOIDs { + refused[oid] = struct{}{} + } s.mu.Unlock() if batchPath != "" && r.URL.Path != batchPath { @@ -121,9 +134,26 @@ func (s *fakeLFSServer) handleBatch(w http.ResponseWriter, r *http.Request) { return } + // Mimic a forge that rejects some batch requests, such as one carrying more + // objects than its limit allows. + if rejectWhen != nil { + if code := rejectWhen(request); code != http.StatusOK { + w.WriteHeader(code) + return + } + } + response := batchResponse{Objects: make([]batchResponseObject, 0, len(request.Objects))} for _, object := range request.Objects { - if _, known := s.data[object.OID]; !known { + if _, isRefused := refused[object.OID]; isRefused { + response.Objects = append(response.Objects, batchResponseObject{ + OID: object.OID, + Error: &batchError{Code: http.StatusUnprocessableEntity, Message: "refused"}, + }) + continue + } + content, known := s.data[object.OID] + if !known { response.Objects = append(response.Objects, batchResponseObject{ OID: object.OID, Error: &batchError{Code: http.StatusNotFound, Message: "Object does not exist"}, @@ -132,7 +162,7 @@ func (s *fakeLFSServer) handleBatch(w http.ResponseWriter, r *http.Request) { } response.Objects = append(response.Objects, batchResponseObject{ OID: object.OID, - Size: int64(len(s.data[object.OID])), + Size: int64(len(content)), Actions: map[string]*batchAction{"download": {Href: s.server.URL + "/download/" + object.OID}}, }) } @@ -319,6 +349,214 @@ func TestFetchAllUnknownObjectFails(t *testing.T) { if err == nil || errors.Is(err, ErrDisabled) { t.Fatalf("unknown object should be a genuine error, got %v", err) } + if !errors.Is(err, errObjectUnavailable) { + t.Fatalf("err = %v, want it to report the object as unavailable", err) + } + if !strings.Contains(err.Error(), "could not be fetched") { + t.Fatalf("err = %v, want it to say how many objects were skipped", err) + } +} + +// TestFetchAllFallsBackWhenBatchRejected covers forges that reject a batch +// request outright — an object limit, or a batch they refuse to process. The +// pointers must be retried one at a time so a single refused object cannot cost +// the repository's whole LFS mirror, while a refusal that hits every object is +// reported as the endpoint's failure. +func TestFetchAllFallsBackWhenBatchRejected(t *testing.T) { + available := []byte("available content") + availableOID, availablePointer := pointerFor(available) + refused := []byte("refused content") + refusedOID, refusedPointer := pointerFor(refused) + result := func(oids ...string) string { return strings.Join(oids, ",") } + + cases := []struct { + name string + request func(batchRequest) int + wantErr string + wantBatches int + wantCached string + }{ + { + name: "server object limit", + request: func(request batchRequest) int { + if len(request.Objects) > 1 { + return http.StatusUnprocessableEntity + } + return http.StatusOK + }, + wantBatches: 3, + wantCached: result(availableOID, refusedOID), + }, + { + name: "one object rejected, one refused", + request: func(request batchRequest) int { + for _, object := range request.Objects { + if object.OID == refusedOID { + return http.StatusUnprocessableEntity + } + } + return http.StatusOK + }, + wantErr: "unavailable", + wantBatches: 3, + wantCached: result(availableOID), + }, + { + name: "the endpoint refuses every batch", + request: func(batchRequest) int { + return http.StatusBadRequest + }, + wantErr: "rejected for 2 of 2 objects", + wantBatches: 3, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + lfsServer := newFakeLFSServer(t, map[string][]byte{ + availableOID: available, + refusedOID: refused, + }) + lfsServer.rejectWhen = c.request + repositoryPath := newRepoWithLFS(t, map[string]string{ + "big.bin": availablePointer, + "stale.bin": refusedPointer, + }) + + err := NewFetcher().FetchAll(context.Background(), repositoryPath, lfsServer.remoteURL(), "", "") + if c.wantErr == "" { + if err != nil { + t.Fatalf("FetchAll = %v, want success", err) + } + } else if err == nil || !strings.Contains(err.Error(), c.wantErr) { + t.Fatalf("FetchAll = %v, want an error containing %q", err, c.wantErr) + } + + if calls := lfsServer.batchCallCount(); calls != c.wantBatches { + t.Errorf("batch calls = %d, want %d", calls, c.wantBatches) + } + for _, oid := range strings.Split(c.wantCached, ",") { + if oid == "" { + continue + } + path := filepath.Join(repositoryPath, ".git", "lfs", "objects", oid[0:2], oid[2:4], oid) + if _, err := os.Stat(path); err != nil { + t.Errorf("object %s should be mirrored: %v", shortOID(oid), err) + } + } + }) + } +} + +func TestFetchAllChunksLargePointerSets(t *testing.T) { + files := make(map[string]string, batchObjectLimit+2) + data := make(map[string][]byte, batchObjectLimit+2) + var lastOID string + for index := range batchObjectLimit + 2 { + content := []byte(fmt.Sprintf("object %d", index)) + oid, pointerText := pointerFor(content) + data[oid] = content + files[fmt.Sprintf("file-%03d.bin", index)] = pointerText + lastOID = oid + } + + lfsServer := newFakeLFSServer(t, data) + repositoryPath := newRepoWithLFS(t, files) + + if err := NewFetcher().FetchAll(context.Background(), repositoryPath, lfsServer.remoteURL(), "", ""); err != nil { + t.Fatalf("FetchAll failed: %v", err) + } + + if calls := lfsServer.batchCallCount(); calls != 2 { + t.Errorf("batch calls = %d, want one request per chunk of %d", calls, batchObjectLimit) + } + if _, err := os.Stat(filepath.Join(repositoryPath, ".git", "lfs", "objects", + lastOID[0:2], lastOID[2:4], lastOID)); err != nil { + t.Errorf("the last chunk should be mirrored too: %v", err) + } +} + +// TestCollectPointersHandlesWindowsIllegalNames covers a tree entry that cannot +// be materialised on the host — a backslash in a name is a path separator on +// Windows — and one that reuses a subtree hash, as a submodule-like entry does. +// Both must be walked without tripping the scan: the pointer beside them is +// still collected, and the shared tree is visited once. +func TestCollectPointersHandlesWindowsIllegalNames(t *testing.T) { + dir := t.TempDir() + repository, err := git.PlainInit(dir, false) + if err != nil { + t.Fatal(err) + } + worktree, err := repository.Worktree() + if err != nil { + t.Fatal(err) + } + + content := []byte("weights") + oid, pointerText := pointerFor(content) + if err := os.MkdirAll(filepath.Join(dir, "nested"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "nested", "weights.bin"), []byte(pointerText), 0o644); err != nil { + t.Fatal(err) + } + if _, err := worktree.Add("nested/weights.bin"); err != nil { + t.Fatal(err) + } + if _, err := worktree.Commit("add pointer", &git.CommitOptions{ + Author: &object.Signature{Name: "test", Email: "test@example.com", When: time.Now()}, + }); err != nil { + t.Fatal(err) + } + + head, err := repository.Head() + if err != nil { + t.Fatal(err) + } + commit, err := repository.CommitObject(head.Hash()) + if err != nil { + t.Fatal(err) + } + root, err := repository.TreeObject(commit.TreeHash) + if err != nil { + t.Fatal(err) + } + var nested object.TreeEntry + for _, entry := range root.Entries { + if entry.Name == "nested" { + nested = entry + } + } + if nested.Hash.IsZero() { + t.Fatal("nested subtree not found") + } + + hostile := &object.Tree{Entries: []object.TreeEntry{ + {Name: "nested-again", Hash: nested.Hash, Mode: filemode.Dir}, + {Name: "nested", Hash: nested.Hash, Mode: filemode.Dir}, + {Name: `src\windows.cpp`, Hash: plumbing.NewHash(strings.Repeat("a", 40)), Mode: filemode.Regular}, + }} + encoded := repository.Storer.NewEncodedObject() + if err := hostile.Encode(encoded); err != nil { + t.Fatal(err) + } + hostileHash, err := repository.Storer.SetEncodedObject(encoded) + if err != nil { + t.Fatal(err) + } + + headRef := plumbing.NewHashReference(plumbing.NewBranchReferenceName("hostile"), hostileHash) + if err := repository.Storer.SetReference(headRef); err != nil { + t.Fatal(err) + } + + pointers, err := collectPointers(context.Background(), repository) + if err != nil { + t.Fatalf("collectPointers failed on a tree with host-specific names: %v", err) + } + if len(pointers) != 1 || pointers[0].oid != oid { + t.Fatalf("pointers = %+v, want the single pointer %s", pointers, oid) + } } func TestFetchAllRejectsUnsafeLFSConfigOverrides(t *testing.T) { @@ -379,6 +617,33 @@ func TestCanonicalHostStripsSchemeDefaultPorts(t *testing.T) { } } +// TestDefaultEndpointAddsGitSuffix pins the endpoint a forge expects: GitHub +// and GitLab answer a suffix-less /info/lfs with 422, so a configured remote +// without .git still has to request the suffixed path. +func TestDefaultEndpointAddsGitSuffix(t *testing.T) { + cases := []struct { + remoteURL string + want string + }{ + {"https://github.com/owner/repo", "https://github.com/owner/repo.git/info/lfs"}, + {"https://github.com/owner/repo.git", "https://github.com/owner/repo.git/info/lfs"}, + {"https://github.com/owner/repo/", "https://github.com/owner/repo.git/info/lfs"}, + {"https://git.example.com:8443/group/sub/repo", "https://git.example.com:8443/group/sub/repo.git/info/lfs"}, + } + + for _, c := range cases { + t.Run(c.remoteURL, func(t *testing.T) { + parsed, err := url.Parse(c.remoteURL) + if err != nil { + t.Fatal(err) + } + if got := defaultEndpoint(parsed); got != c.want { + t.Errorf("defaultEndpoint(%q) = %q, want %q", c.remoteURL, got, c.want) + } + }) + } +} + func TestResolveEndpointOverrideHostAndScheme(t *testing.T) { oid, pointerText := pointerFor([]byte("content")) cases := []struct { diff --git a/internal/lfs/scan.go b/internal/lfs/scan.go index d9dab76..ad9b5e9 100644 --- a/internal/lfs/scan.go +++ b/internal/lfs/scan.go @@ -10,6 +10,7 @@ import ( "github.com/go-git/go-git/v5" "github.com/go-git/go-git/v5/plumbing" + "github.com/go-git/go-git/v5/plumbing/filemode" "github.com/go-git/go-git/v5/plumbing/object" ) @@ -119,6 +120,18 @@ func peelToCommitHash(repository *git.Repository, hash plumbing.Hash) plumbing.H } } +// scanTree collects pointers from every blob under treeHash, descending into +// subtrees iteratively. +// +// Subtrees are walked from their tree objects directly rather than through +// object.TreeWalker: that walker validates each entry against the rules for +// materialising a working tree, so a repository containing a path that is +// illegal on the host — a backslash in an entry name on Windows, for instance — +// would abort the whole backup even though the mirror clones and uploads +// perfectly well. A tree object only ever holds entry names and subtree hashes, +// so enumerating them needs no path handling at all. seenTrees bounds the walk +// for repositories whose history repeats or self-references a tree, and makes +// the cross-ref scan visit each tree once (see collectPointers). func scanTree( repository *git.Repository, treeHash plumbing.Hash, @@ -131,52 +144,65 @@ func scanTree( return fmt.Errorf("read tree: %w", err) } - walker := object.NewTreeWalker(tree, true, seenTrees) - defer walker.Close() + pending := []*object.Tree{tree} + seenTrees[treeHash] = true - for { - name, entry, err := walker.Next() - if err == io.EOF { - return nil - } - if err != nil { - return fmt.Errorf("walk tree: %w", err) - } - _ = name + for len(pending) > 0 { + current := pending[len(pending)-1] + pending = pending[:len(pending)-1] - if !entry.Mode.IsFile() { - continue - } - if _, seen := seenBlobs[entry.Hash]; seen { - continue - } - seenBlobs[entry.Hash] = struct{}{} + for _, entry := range current.Entries { + if entry.Mode == filemode.Dir { + if seenTrees[entry.Hash] { + continue + } + seenTrees[entry.Hash] = true - blob, err := repository.BlobObject(entry.Hash) - if err != nil { - continue - } - if blob.Size > pointerMaxBytes { - continue - } + subtree, err := repository.TreeObject(entry.Hash) + if err != nil { + // A subtree missing from a partial fetch costs its + // pointers but leaves the rest of the scan intact. + continue + } + pending = append(pending, subtree) + continue + } + if !entry.Mode.IsFile() { + continue + } + if _, seen := seenBlobs[entry.Hash]; seen { + continue + } + seenBlobs[entry.Hash] = struct{}{} - reader, err := blob.Reader() - if err != nil { - continue - } - content, err := io.ReadAll(io.LimitReader(reader, pointerMaxBytes)) - _ = reader.Close() - if err != nil { - continue - } + blob, err := repository.BlobObject(entry.Hash) + if err != nil { + continue + } + if blob.Size > pointerMaxBytes { + continue + } - if parsed, ok := parsePointer(content); ok { - if _, duplicate := seenPointers[parsed.oid]; !duplicate { - seenPointers[parsed.oid] = struct{}{} - *pointers = append(*pointers, parsed) + reader, err := blob.Reader() + if err != nil { + continue + } + content, err := io.ReadAll(io.LimitReader(reader, pointerMaxBytes)) + _ = reader.Close() + if err != nil { + continue + } + + if parsed, ok := parsePointer(content); ok { + if _, duplicate := seenPointers[parsed.oid]; !duplicate { + seenPointers[parsed.oid] = struct{}{} + *pointers = append(*pointers, parsed) + } } } } + + return nil } // parsePointer parses an LFS pointer file: the version line followed by From 0114dcd248f78ededbb7aaee99ce663374f4d8c0 Mon Sep 17 00:00:00 2001 From: Neureka Date: Thu, 10 Sep 2026 08:09:28 -0700 Subject: [PATCH 02/15] fix(lfs): preserve the remote URL and narrow rejected batches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up on the LFS mirroring fixes: - defaultEndpoint rebuilt the endpoint as scheme + host + path, which dropped a remote URL's userinfo (the auth those deployments rely on) and glued ".git" onto the host when the remote had no path. It now copies the parsed URL and rewrites only its path, keeping the escaped form so percent-encoded bytes survive, clearing the query, ForceQuery, and fragment so a remote ending in "?" cannot slip a "?" between the endpoint and the action path, resolving a pathless remote to the host's LFS API root, and matching the repository suffix case-insensitively so a remote already ending in ".GIT" does not gain a second one that no forge serves. - The endpoint is a secret-bearing string now that userinfo survives, so every log line renders it through redactedURL. - The default endpoint appends the repository's .git suffix, which GitHub and GitLab require and which a host mounting LFS at the configured path would answer with 404 — indistinguishable from LFS being switched off, and therefore a silent skip. FetchAll now tries the suffixed endpoint and then the plain one, collapsing the pair when a remote reaches the API at a single path, and neither a 404, a 403, nor a refusal is believed until every candidate has been tried. A rejection from the candidate that actually mounts the service outranks another's silence or 403, so a repository whose real path refuses is reported rather than skipped; one that is disabled, or has no service anywhere, still reports as the expected skip. - Blobs and subtrees the pointer scan cannot read — a partial fetch, say — used to be dropped silently. The scan now returns what it skipped, FetchAll names each one at warn level and fails the fetch, so a mirror reported as complete never quietly lacks LFS content. - downloadObjects returned any error from Object.Error as a hard failure of the whole response. It now attempts every scheduled object and reports back two lists — objects the endpoint will not serve, and objects this client could not transfer — so one stale pointer or one transient failure cannot hide its siblings while a genuine transfer failure is still reported rather than recorded as the endpoint's refusal. - A forge with LFS switched off answers 403 with an explanation while a host with nothing mounted behind the path answers with its own page, so the 403 body now separates the two: the first keeps the repository's expected skip, and the second lets the remaining candidate paths be tried. A 404 from the batch API likewise means no service answered there, and a candidate that fails for a reason unrelated to the path no longer stops the others from being tried. - An endpoint that stops serving partway through a repository used to end as a clean "LFS is off" skip, reporting a half-finished mirror as complete. An endpoint that has served objects is now authoritative whatever it reports afterwards: any later verdict is rendered rather than wrapped, so no skip sentinel survives to be matched through the chain, and it outranks another candidate's answer while an object the endpoint simply will not serve stays a reported gap rather than a partial mirror. - A rejected batch fanned out to one request per pointer. It is now split in half and resubmitted, down to single pointers. Every split tries both halves whatever the first one concluded, because a half the endpoint answered nothing for may consist of individually refused objects rather than a refused endpoint — the sibling is both how its own objects reach the mirror and how the refusal is attributed — so a chunk costs up to two requests per pointer rather than a logarithmic count, as its documentation now says. A chunk the endpoint answered at all, whether by serving an object or by reporting one missing, keeps a refused object recorded as unavailability; a chunk it answered nothing for is reported without ending the sweep, since only a second such chunk distinguishes stale pointers from a broken endpoint. - Only statuses that describe the request's shape — 400, 406, 413, 415, 422 — enter the split. Credentials, rate limiting, and the endpoint's own failures describe the endpoint rather than what was asked of it, so they fail fast instead of repeating the same answer once per pointer. --- internal/lfs/batch.go | 102 ++++-- internal/lfs/lfs.go | 438 ++++++++++++++++------ internal/lfs/lfs_test.go | 771 +++++++++++++++++++++++++++++++++++++-- internal/lfs/scan.go | 40 +- 4 files changed, 1188 insertions(+), 163 deletions(-) diff --git a/internal/lfs/batch.go b/internal/lfs/batch.go index 9e3be55..0dae1e9 100644 --- a/internal/lfs/batch.go +++ b/internal/lfs/batch.go @@ -92,10 +92,15 @@ func newBatchClient(client *http.Client) *batchClient { const lfsMediaType = "application/vnd.git-lfs+json" -// batch submits every pointer for download scheduling. A 403 or 404 from the -// endpoint means the remote has Git LFS switched off, which callers treat as -// an expected skip rather than a failure. Any other unexpected status is -// reported as errBatchRejected so callers can retry the pointers individually. +// batch submits every pointer for download scheduling. +// +// A 403 means the remote has Git LFS switched off, which callers treat as an +// expected skip rather than a failure. A 404 means the LFS service answered +// about an object it does not have, which callers record per object. A status +// describing the request's shape is reported as errBatchRejected so callers can +// retry the pointers in smaller batches; every other status describes the +// endpoint rather than what was asked of it, so splitting the chunk would only +// repeat the same answer more slowly. func (c *batchClient) batch(ctx context.Context, endpoint, username, password string, pointers []pointer) ([]batchResponseObject, error) { body, err := json.Marshal(batchRequest{ Operation: "download", @@ -122,13 +127,27 @@ func (c *batchClient) batch(ctx context.Context, endpoint, username, password st } defer drainAndClose(response) - switch response.StatusCode { - case http.StatusOK: + if response.StatusCode == http.StatusForbidden { + // A forge that has LFS switched off answers 403 with an explanation, + // while a path with nothing mounted behind it answers 403 only if the + // host refuses unmatched paths. The body separates the two, and the + // difference decides whether the repository is skipped or another + // candidate path is tried. + if body, err := io.ReadAll(io.LimitReader(response.Body, 4096)); err == nil && looksLikeJSON(body) { + return nil, ErrDisabled + } + return nil, ErrNoEndpoint + } + switch { + case response.StatusCode == http.StatusOK: // Handled below. - case http.StatusForbidden, http.StatusNotFound: - return nil, ErrDisabled - default: + case response.StatusCode == http.StatusNotFound: + // No LFS action answered at this path, which another candidate may. + return nil, ErrNoEndpoint + case describesRequestShape(response.StatusCode): return nil, fmt.Errorf("%w: status %d", errBatchRejected, response.StatusCode) + default: + return nil, fmt.Errorf("batch request failed with status %d", response.StatusCode) } var decoded batchResponse @@ -141,36 +160,73 @@ func (c *batchClient) batch(ctx context.Context, endpoint, username, password st return decoded.Objects, nil } +// describesRequestShape reports whether a status is how servers answer a request +// they will not process as sent, which a smaller batch may avoid: an unprocessable +// entity, a payload the server considers too large, or a body it cannot accept. +// Credentials, rate limiting, and the endpoint's own failures are not among them. +func describesRequestShape(status int) bool { + switch status { + case http.StatusBadRequest, + http.StatusRequestEntityTooLarge, + http.StatusUnprocessableEntity, + http.StatusNotAcceptable, + http.StatusUnsupportedMediaType: + return true + default: + return false + } +} + +// looksLikeJSON reports whether a body is a JSON document, which is how the LFS +// batch API explains itself and how an HTML error page does not. +func looksLikeJSON(body []byte) bool { + trimmed := bytes.TrimSpace(body) + return len(trimmed) > 0 && (trimmed[0] == '{' || trimmed[0] == '[') +} + // downloadObjects streams each scheduled object into the repository's LFS // cache, verifying its SHA-256 as bytes arrive. Objects already cached are // skipped, so repeated snapshots only download new content. +// +// Every object is attempted whatever the others did, so one bad pointer or one +// transient transfer failure cannot hide its siblings. What failed comes back in +// the two lists: unavailable for objects the endpoint will not serve, and failed +// for objects this client could not transfer — a corrupt body, a broken +// connection — which the caller reports rather than recording as the endpoint's +// refusal. func downloadObjects( ctx context.Context, client *batchClient, store, endpoint, username, password string, objects []batchResponseObject, -) error { +) (unavailable []*batchObjectError, failed []error) { endpointHost := hostOf(endpoint) - for _, object := range objects { - if err := ctx.Err(); err != nil { - return err - } - if object.Error != nil { + answered := func(object batchResponseObject) *batchObjectError { + switch { + case object.Error != nil: return &batchObjectError{oid: object.OID, message: object.Error.Message} - } - - action := object.Actions["download"] - if action == nil || action.Href == "" { - // The server knows the object but scheduled nothing; without a + case object.Actions["download"] == nil || object.Actions["download"].Href == "": + // The server knows the object but scheduled nothing; without an // href there is nothing this client can do beyond reporting it. return &batchObjectError{oid: object.OID, message: "no download action was scheduled"} + default: + return nil } + } - if err := downloadObject(ctx, client.client, store, endpointHost, username, password, object.OID, action); err != nil { - return err + for _, object := range objects { + if err := ctx.Err(); err != nil { + return unavailable, append(failed, err) + } + if refused := answered(object); refused != nil { + unavailable = append(unavailable, refused) + continue + } + if err := downloadObject(ctx, client.client, store, endpointHost, username, password, object.OID, object.Actions["download"]); err != nil { + failed = append(failed, err) } } - return nil + return unavailable, failed } func downloadObject( diff --git a/internal/lfs/lfs.go b/internal/lfs/lfs.go index 818e3b7..429e88c 100644 --- a/internal/lfs/lfs.go +++ b/internal/lfs/lfs.go @@ -52,58 +52,172 @@ func (f *Fetcher) FetchAll(ctx context.Context, repositoryPath, remoteURL, usern return fmt.Errorf("open repository: %w", err) } - endpoint, err := resolveEndpoint(repository, remoteURL) + endpoints, err := resolveEndpoints(repository, remoteURL) if err != nil { return err } - pointers, err := collectPointers(ctx, repository) + scan, err := collectPointers(ctx, repository) if err != nil { return fmt.Errorf("scan for LFS pointers: %w", err) } - if len(pointers) == 0 { + if len(scan.skipped) > 0 { + // Whatever those objects held is missing from the mirror, so name them + // and fail the fetch: a backup reported as complete while LFS content is + // silently absent is worse than one that reports what it could not do. + for _, skipped := range scan.skipped { + slog.Warn("Could not read part of the repository while scanning for Git LFS pointers.", + "repository", redactedURL(remoteURL), "entry", skipped.name, + "oid", shortOID(skipped.hash.String()), "reason", skipped.err.Error()) + } + return fmt.Errorf("%d entries could not be read while scanning for LFS pointers, so the mirror would be incomplete", len(scan.skipped)) + } + if len(scan.pointers) == 0 { // Nothing to fetch; never contact the endpoint so a forge without LFS // is not mistaken for one that disabled it. return nil } + pointers := scan.pointers store := objectStoreDir(repository, repositoryPath) - return f.fetchBatches(ctx, store, endpoint, username, password, pointers) + var disabledErr, rejectedErr, hardErr error + for _, endpoint := range endpoints { + served, endpointErr := f.fetchBatches(ctx, store, endpoint, username, password, pointers) + if endpointErr == nil { + return nil + } + if served > 0 && !errors.Is(endpointErr, errObjectUnavailable) { + // An endpoint that already mirrored part of the repository is + // authoritative whatever it says afterwards: no later verdict — + // "LFS is off", "nothing here", a rejection — may be recorded as a + // clean skip, because that would report a half-finished mirror as + // complete. The outcome is rendered rather than wrapped, so no skip + // sentinel survives to be matched through the chain. + if hardErr == nil { + hardErr = fmt.Errorf("%d LFS objects were fetched before the endpoint stopped serving the rest: %v", served, endpointErr) + } + slog.Debug("Endpoint stopped serving partway through the repository.", + "endpoint", redactedURL(endpoint), "objectsServed", served, "detail", endpointErr.Error()) + continue + } + switch { + case errors.Is(endpointErr, ErrDisabled): + // A repository with LFS switched off is an expected skip, and the + // endpoint that said so is the one serving the API. + if disabledErr == nil { + disabledErr = endpointErr + } + case errors.Is(endpointErr, ErrNoEndpoint): + // Nothing is mounted at this path; the next candidate may serve it. + case errors.Is(endpointErr, errBatchRejected): + if rejectedErr == nil { + rejectedErr = endpointErr + } + default: + // The candidate that failed may be the derived guess rather than the + // path the remote configures, so the remaining candidates still get + // their turn before this is reported. + if hardErr == nil { + hardErr = endpointErr + } + } + slog.Debug("No Git LFS service answered at this endpoint.", "endpoint", redactedURL(endpoint)) + } + + switch { + case hardErr != nil: + // A real failure on any candidate outranks another's "LFS is off" + // answer, which the mirror layer records as a successful skip: masking + // the failure would report an incomplete mirror as complete. + return hardErr + case disabledErr != nil: + // A candidate that answered "LFS is off" identified the API root, so its + // verdict is authoritative in a way another candidate's request-shape + // rejection is not. + return disabledErr + case rejectedErr != nil: + return rejectedErr + default: + // No candidate served the API, which the mirror layer records as LFS + // being switched off rather than failing the repository. + return ErrDisabled + } } // batchObjectLimit is how many pointers one batch request submits. Git LFS // clients use the same limit; a forge may reject a request that exceeds its own // smaller limit, which fetchChunk then narrows down. +// +// Narrowing is not remembered between chunks, deliberately. A refusal cannot +// distinguish a server whose limit is low from a chunk carrying one object the +// server will not serve, and a remembered bound derived from the second kind +// would shrink every later chunk — including chunks of perfectly servable +// objects. The cost of not remembering is bounded: at most two requests per +// pointer in a chunk, and the sweep stops after a second failing chunk. const batchObjectLimit = 100 +// ErrNoEndpoint reports that nothing answered the LFS batch API at an endpoint, +// which is how a forge that routes the API elsewhere — or at all — looks to a +// client. FetchAll tries its next candidate endpoint on this answer, and a +// repository with no service anywhere is reported with ErrDisabled, because from +// here that is indistinguishable from LFS being switched off and the mirror +// layer already knows how to record it. +var ErrNoEndpoint = errors.New("no git lfs service answered at the endpoint") + // fetchBatches downloads every pointer's object, splitting the pointers into -// batch requests. A batch the endpoint rejects is narrowed down object by -// object, so one object the server will not serve — a stale pointer, or a -// request the server considers too large — costs that object alone instead of -// the repository's entire LFS mirror. Failing to mirror an object that exists -// still errors the fetch. +// batch requests. A batch the endpoint rejects is narrowed down by halving, so +// one object the server will not serve — a stale pointer, or a request above +// the server's own limit — costs that object alone instead of the repository's +// entire LFS mirror. Failing to mirror an object that exists still errors the +// fetch. +// +// Narrowing a chunk costs up to two requests per pointer in it, not a +// logarithmic count: a half the endpoint answered nothing for may hold nothing +// but individually refused objects, so the only way to know whether its sibling +// is servable is to ask it. A chunk the endpoint answered nothing for therefore +// still reports, and the sweep continues, because that state may mean a broken +// endpoint or merely a chunk of stale pointers. Once a second chunk fails, the +// fetch stops and reports rather than repeating the failure across a large +// repository. +// +// The count of objects the endpoint served comes back with the error, because a +// caller has to know whether an endpoint that now answers "no service" or "LFS +// off" already mirrored part of the repository: a verdict that would mean a +// clean skip cannot also mean a half-finished mirror. func (f *Fetcher) fetchBatches( ctx context.Context, store, endpoint, username, password string, pointers []pointer, -) error { - var unavailable, rejected int +) (int, error) { + var unavailable, rejected, served int var firstFailure, firstErr error for start := 0; start < len(pointers); start += batchObjectLimit { end := min(start+batchObjectLimit, len(pointers)) if err := ctx.Err(); err != nil { - return err + return served, err } outcome, err := f.fetchChunk(ctx, store, endpoint, username, password, pointers[start:end]) unavailable += outcome.unavailable rejected += outcome.rejected + served += outcome.served if outcome.skipped != nil && firstFailure == nil { firstFailure = outcome.skipped } - if err != nil && firstErr == nil { - firstErr = err + if err == nil { + continue + } + // A chunk the endpoint answered nothing for is reported, but the rest of + // the repository is still attempted: that state may mean a broken + // endpoint or merely a chunk of stale pointers, and only the next chunk + // tells them apart. + if firstErr != nil { + // The endpoint failed a second chunk, so stop asking. + slog.Debug("Stopping the Git LFS fetch after another chunk failed.", + "endpoint", redactedURL(endpoint), "detail", err.Error()) + break } + firstErr = err } skipped := unavailable + rejected @@ -112,34 +226,46 @@ func (f *Fetcher) fetchBatches( // complete, so say so once per repository at warn level and keep the // per-object reasons for debug output. slog.Warn("Some Git LFS objects could not be fetched; the repository was mirrored without them.", - "endpoint", endpoint, "objectsMissing", skipped, "objectsRequested", len(pointers)) + "endpoint", redactedURL(endpoint), "objectsMissing", skipped, "objectsRequested", len(pointers)) slog.Debug("Git LFS objects that could not be fetched.", - "endpoint", endpoint, "reason", firstFailure.Error()) + "endpoint", redactedURL(endpoint), "reason", firstFailure.Error()) } if firstErr != nil { - return firstErr + return served, firstErr } if skipped > 0 { - return fmt.Errorf("%d of %d LFS objects could not be fetched: %w", skipped, len(pointers), firstFailure) + return served, fmt.Errorf("%d of %d LFS objects could not be fetched: %w", skipped, len(pointers), firstFailure) } - return nil + return served, nil } -// chunkOutcome reports what one batch request could not fetch: objects the +// chunkOutcome reports what one batch request could not fetch — objects the // endpoint answered for but will not serve, and objects whose own batch request -// the endpoint rejected outright. skipped, when set, names the first of them. +// the endpoint rejected outright — and how many it did serve. skipped, when +// set, names the first object that could not be fetched. type chunkOutcome struct { unavailable int rejected int + served int skipped error } +// errNarrowedToSingleton marks the rejection of a single object's own batch +// request. Callers compare it with a sibling half's outcome to decide whether +// the endpoint refuses the request shape itself or merely that one object. +var errNarrowedToSingleton = errors.New("batch request rejected down to a single object") + +// errAllRejected marks a chunk whose every request was refused while the +// endpoint answered none of its objects, which is as close as a client can get to +// an endpoint-wide rejection. It is reported and, once a second chunk repeats it, +// treated as the endpoint being broken rather than as a chunk of stale pointers. +var errAllRejected = errors.New("every request for this batch was rejected") + // fetchChunk submits one batch request for pointers and downloads what it -// schedules. A rejected batch falls back to requesting the same pointers -// individually: the rejection is usually one poisonous object, and the remaining -// objects then still reach the mirror. A rejection covering more than half of the -// chunk comes back as an error, because a failure that broad is the endpoint's, -// not one object's. +// schedules. A rejected batch is split in half and resubmitted, down to single +// pointers, so the rejection is narrowed to the object that caused it: a request +// the endpoint considers too large, or one poisonous object, then costs that +// object alone instead of the chunk's whole LFS content. func (f *Fetcher) fetchChunk( ctx context.Context, store, endpoint, username, password string, @@ -152,106 +278,155 @@ func (f *Fetcher) fetchChunk( if !errors.Is(err, errBatchRejected) { return chunkOutcome{}, err } + // Descending calls answer with their own errors, so the rejection that + // brought us here is kept for the reports below. + rejection := err - outcome := chunkOutcome{} - for index := range pointers { - if ctx.Err() != nil { - return outcome, ctx.Err() + // A rejected chunk wider than one pointer is retried as two batches, and + // each half narrows further on its own. The sibling half is always tried, + // whatever the first half concluded: a half in which the endpoint answered + // nothing may consist of individually refused objects rather than a refused + // endpoint, and only the sibling's answer — or its absence — settles which. + // Trying it is also what lets the sibling's objects reach the mirror when + // only the first half is poisoned. + if len(pointers) > 1 { + middle := len(pointers) / 2 + outcome, firstErr := f.fetchChunk(ctx, store, endpoint, username, password, pointers[:middle]) + if firstErr != nil && !isRefusal(firstErr) { + return outcome, firstErr } - single, err := f.client.batch(ctx, endpoint, username, password, pointers[index:index+1]) - if err != nil { - if !errors.Is(err, errBatchRejected) { - return outcome, err - } - // This object's own request was rejected too, so the endpoint - // refuses to serve it at all. - outcome.rejected++ - if outcome.skipped == nil { - outcome.skipped = fmt.Errorf("%w: LFS object %s download request rejected: %w", errObjectUnavailable, shortOID(pointers[index].oid), err) - } - continue + rest, restErr := f.fetchChunk(ctx, store, endpoint, username, password, pointers[middle:]) + outcome.absorb(rest) + if restErr != nil && !isRefusal(restErr) { + return outcome, restErr } - - served, err := f.downloadChunk(ctx, store, endpoint, username, password, single) - outcome.unavailable += served.unavailable - outcome.rejected += served.rejected - if outcome.skipped == nil { - outcome.skipped = served.skipped - } - if err != nil { - return outcome, err + if firstErr == nil && restErr == nil { + return outcome, nil } + // A rejection reached a single pointer in one of the halves, and the + // whole chunk is now accounted for, so the rejection can be attributed. + return refuseOrReport(outcome, rejection) } - if outcome.rejected > len(pointers)/2 { - return outcome, fmt.Errorf("batch request rejected for %d of %d objects: %w", outcome.rejected, len(pointers), err) + // The endpoint refuses this single object's own request, so nothing narrower + // can be blamed. Whether that is this object's fault or the endpoint's is + // decided one level up, by whether the sibling half was served; this subtree + // has served nothing either way. + slog.Debug("Git LFS batch request rejected for a single object.", + "endpoint", redactedURL(endpoint), "oid", shortOID(pointers[0].oid), "reason", rejection.Error()) + return chunkOutcome{ + rejected: 1, + skipped: fmt.Errorf("%w: LFS object %s download request rejected: %w", + errObjectUnavailable, shortOID(pointers[0].oid), rejection), + }, fmt.Errorf("%w: %w", errNarrowedToSingleton, rejection) +} + +// isRefusal reports whether an error means the endpoint refused a request +// rather than failing to serve it. +func isRefusal(err error) bool { + return errors.Is(err, errNarrowedToSingleton) || errors.Is(err, errAllRejected) +} + +// refuseOrReport settles a chunk whose rejection reached a single pointer. An +// object the endpoint answered for settles it: whether that answer served bytes +// or reported the object missing, the endpoint processed the batch, so a refused +// sibling object is that object's fault rather than the endpoint's. A chunk the +// endpoint answered nothing for is the alternative — allRejected wrapping +// errBatchRejected — which callers report and stop the sweep after, since only a +// second such chunk tells stale pointers apart from a broken endpoint. +func refuseOrReport(outcome chunkOutcome, rejection error) (chunkOutcome, error) { + if outcome.served > 0 || outcome.unavailable > 0 { + return outcome, nil } - return outcome, nil + return outcome, fmt.Errorf("%w: %w", errAllRejected, rejection) } -// downloadChunk streams every object the batch scheduled. An object the endpoint -// refuses to serve is recorded and skipped so its siblings still reach the -// mirror; any other failure — a corrupt download, a broken connection — comes -// back as an error. +// downloadChunk streams every object the batch scheduled. Objects the endpoint +// will not serve are recorded and skipped so their siblings still download, and +// so are objects this client could not transfer — attempted, but unreachable — +// which come back as an error once every object has had its turn. func (f *Fetcher) downloadChunk( ctx context.Context, store, endpoint, username, password string, objects []batchResponseObject, ) (chunkOutcome, error) { + unavailable, failed := downloadObjects(ctx, f.client, store, endpoint, username, password, objects) + var outcome chunkOutcome - for _, object := range objects { - if object.Error != nil { - outcome.unavailable++ - if outcome.skipped == nil { - outcome.skipped = fmt.Errorf("%w: LFS object %s is unavailable: %s", - errObjectUnavailable, shortOID(object.OID), object.Error.Message) - } - continue - } - if err := downloadObjects(ctx, f.client, store, endpoint, username, password, []batchResponseObject{object}); err != nil { - return outcome, err - } + for _, object := range unavailable { + outcome.recordUnavailable(fmt.Errorf("%w: %s", errObjectUnavailable, object.Error())) + } + // Whatever the endpoint did not refuse, it answered for one way or another. + outcome.served = len(objects) - len(unavailable) + if len(failed) > 0 { + return outcome, fmt.Errorf("%d of %d LFS objects could not be downloaded: %w", len(failed), len(objects), failed[0]) } return outcome, nil } -// resolveEndpoint determines the LFS API root: an lfs.url override from the -// repository's committed .lfsconfig when present, otherwise the remote URL's -// standard /info/lfs root. The batch request authenticates with the remote's -// credential, so an override is honored only when it is an absolute http(s) -// URL on the remote's own host and scheme — a hostile .lfsconfig must not -// redirect that credential elsewhere or downgrade it to plaintext. Reading -// .lfsconfig is best-effort — any failure falls back to the derived endpoint, -// matching the common deployment. -func resolveEndpoint(repository *git.Repository, remoteURL string) (string, error) { +// recordUnavailable counts one object the endpoint answered for but will not +// serve, keeping the first reason for the caller's report. +func (o *chunkOutcome) recordUnavailable(reason error) { + o.unavailable++ + if o.skipped == nil { + o.skipped = reason + } +} + +// absorb adds a split's second half to its first. +func (o *chunkOutcome) absorb(half chunkOutcome) { + o.unavailable += half.unavailable + o.rejected += half.rejected + o.served += half.served + if half.skipped != nil && o.skipped == nil { + o.skipped = half.skipped + } +} + +// resolveEndpoints determines the LFS API roots to try, in order: an lfs.url +// override from the repository's committed .lfsconfig when present, otherwise +// the remote URL's standard /info/lfs root with and without the repository's +// .git suffix. +// +// The batch request authenticates with the remote's credential, so an override +// is honored only when it is an absolute http(s) URL on the remote's own host +// and scheme — a hostile .lfsconfig must not redirect that credential elsewhere +// or downgrade it to plaintext. Reading .lfsconfig is best-effort — any failure +// falls back to the derived endpoints, matching the common deployment. +func resolveEndpoints(repository *git.Repository, remoteURL string) ([]string, error) { parsed, ok := paths.ParseHTTPURL(remoteURL) if !ok { - return "", fmt.Errorf("unsupported remote URL '%s': only http and https are allowed", redactedURL(remoteURL)) + return nil, fmt.Errorf("unsupported remote URL '%s': only http and https are allowed", redactedURL(remoteURL)) } - endpoint := defaultEndpoint(parsed) config, err := readLFSConfig(repository) - if err != nil || config == nil { - return endpoint, nil - } - if override := strings.TrimSpace(config.Raw.Section("lfs").Option("url")); override != "" { - overridden, ok := paths.ParseHTTPURL(override) - if !ok { - return "", fmt.Errorf("unsupported lfs.url '%s': only absolute http and https URLs are allowed", redactedURL(override)) - } - if !strings.EqualFold(overridden.Scheme, parsed.Scheme) || canonicalHost(overridden) != canonicalHost(parsed) { - return "", fmt.Errorf("refusing lfs.url '%s': the override must stay on the remote host and scheme so the remote credential is not sent elsewhere", redactedURL(override)) - } - // Drop a redundant scheme-default port so the endpoint is canonical: - // object-download host comparisons and logs then match hrefs rendered - // without the explicit port. - if overridden.Port() != "" && isSchemeDefaultPort(overridden) { - overridden.Host = strings.TrimSuffix(overridden.Host, ":"+overridden.Port()) + if err == nil && config != nil { + if override := strings.TrimSpace(config.Raw.Section("lfs").Option("url")); override != "" { + overridden, ok := paths.ParseHTTPURL(override) + if !ok { + return nil, fmt.Errorf("unsupported lfs.url '%s': only absolute http and https URLs are allowed", redactedURL(override)) + } + if !strings.EqualFold(overridden.Scheme, parsed.Scheme) || canonicalHost(overridden) != canonicalHost(parsed) { + return nil, fmt.Errorf("refusing lfs.url '%s': the override must stay on the remote host and scheme so the remote credential is not sent elsewhere", redactedURL(override)) + } + // Drop a redundant scheme-default port so the endpoint is canonical: + // object-download host comparisons and logs then match hrefs rendered + // without the explicit port. + if overridden.Port() != "" && isSchemeDefaultPort(overridden) { + overridden.Host = strings.TrimSuffix(overridden.Host, ":"+overridden.Port()) + } + return []string{strings.TrimSuffix(overridden.String(), "/")}, nil } - return strings.TrimSuffix(overridden.String(), "/"), nil } - return endpoint, nil + // A remote that already carries the repository suffix, or has no path at + // all, reaches the API at one path, so there is nothing to fall back to: + // probing the same URL twice would only repeat every request. + suffixed, plain := defaultEndpoint(parsed), plainEndpoint(parsed) + if suffixed == plain { + return []string{suffixed}, nil + } + return []string{suffixed, plain}, nil } // defaultEndpoint derives the remote's standard LFS API root, including the @@ -261,13 +436,68 @@ func resolveEndpoint(repository *git.Repository, remoteURL string) (string, erro // uses, and forges route only the suffixed path to their LFS service: GitHub // and GitLab answer a suffix-less /info/lfs with 422, while Forgejo and Gitea // accept either form. The mirror's remote has no .git suffix because a config -// URL rarely carries one, so the suffix is added here whenever it is absent. +// URL rarely carries one, so the suffix is added here whenever it is absent; +// plainEndpoint is the fallback for a service that routes the suffix-less path. +// +// The parsed URL is copied and only its path rewritten, from its escaped form so +// percent-encoding survives: a remote whose path holds a reserved byte — an +// encoded slash in a repository name, say — must keep requesting that exact +// path. Everything else the configured remote carries, userinfo for a deployment +// that embeds credentials and a non-default port among it, still reaches the +// endpoint. A remote with no path has no repository to suffix, but the API root +// is still /info/lfs on that host. func defaultEndpoint(remoteURL *url.URL) string { - path := strings.TrimSuffix(remoteURL.Path, "/") - if !strings.HasSuffix(path, ".git") { - path += ".git" + endpoint := endpointBase(remoteURL) + + escaped := strings.TrimSuffix(remoteURL.EscapedPath(), "/") + if escaped == "" { + // No repository segment to suffix, but the API root is still /info/lfs + // on that host. + return withPath(endpoint, "/info/lfs") } - return remoteURL.Scheme + "://" + remoteURL.Host + path + "/info/lfs" + // The suffix is judged on the decoded path but appended to the escaped one, + // so a remote whose escaping hides it (re%2Egit) is not given a second + // suffix it did not need. It is matched case-insensitively, as the mirror's + // other .git handling does: a remote already ending in ".GIT" must not gain + // a differently cased one that no forge serves. + if !hasGitSuffix(strings.TrimSuffix(remoteURL.Path, "/")) { + escaped += ".git" + } + return withPath(endpoint, escaped+"/info/lfs") +} + +// plainEndpoint derives the LFS API root at the remote's configured path, +// without adding the repository suffix: the fallback for a service that routes +// /info/lfs exactly where the remote points. +func plainEndpoint(remoteURL *url.URL) string { + endpoint := endpointBase(remoteURL) + return withPath(endpoint, strings.TrimSuffix(remoteURL.EscapedPath(), "/")+"/info/lfs") +} + +// endpointBase copies a remote URL with the parts an API root cannot carry +// removed: a query, a forced empty query, and a fragment would otherwise sit +// between the endpoint and the action path appended to it. +func endpointBase(remoteURL *url.URL) url.URL { + endpoint := *remoteURL + endpoint.RawQuery = "" + endpoint.ForceQuery = false + endpoint.Fragment = "" + endpoint.RawFragment = "" + return endpoint +} + +// withPath points an endpoint at an escaped path, keeping Path decoded so +// String() does not escape the escaping a second time. +func withPath(endpoint url.URL, escaped string) string { + endpoint.RawPath = escaped + endpoint.Path, _ = url.PathUnescape(escaped) + return endpoint.String() +} + +// hasGitSuffix reports whether a remote path already ends in the repository +// suffix, in any case. +func hasGitSuffix(path string) bool { + return len(path) >= len(".git") && strings.EqualFold(path[len(path)-len(".git"):], ".git") } // isSchemeDefaultPort reports whether the URL's explicit port equals its diff --git a/internal/lfs/lfs_test.go b/internal/lfs/lfs_test.go index 86d8676..3f27320 100644 --- a/internal/lfs/lfs_test.go +++ b/internal/lfs/lfs_test.go @@ -12,8 +12,10 @@ import ( "net/url" "os" "path/filepath" + "slices" "strings" "sync" + "sync/atomic" "testing" "time" @@ -75,6 +77,9 @@ type fakeLFSServer struct { // refusedOIDs are answered with a per-object error instead of a download // action, like a pointer whose object is gone from the server. refusedOIDs []string + // actionlessOIDs are answered with neither an error nor a download action, + // as a misbehaving endpoint may do. + actionlessOIDs []string // rejectWhen, when set, decides the status of each batch request so a test // can model an endpoint that rejects some request shapes. rejectWhen func(batchRequest) int @@ -82,6 +87,12 @@ type fakeLFSServer struct { corruptDownload bool // batchPath overrides the expected batch path (lfs.url override tests). batchPath string + // servePaths, when set, are the only batch paths the server answers, so any + // other path answers 404 like a host with no LFS service mounted there. + servePaths []string + // disabledPaths answer 403, the response for a repository with LFS switched + // off, which is not the same as a path with no service behind it. + disabledPaths []string } func newFakeLFSServer(t *testing.T, data map[string][]byte) *fakeLFSServer { @@ -108,6 +119,52 @@ func (s *fakeLFSServer) remoteURL() string { return s.server.URL + "/repo.git" } +// plainRemoteURL is the same repository addressed without its .git suffix, the +// form a configuration normally carries. +func (s *fakeLFSServer) plainRemoteURL() string { + return s.server.URL + "/repo" +} + +// endpointFor mounts the server's batch API at the path a remote addresses, so +// a test chooses where the LFS service answers, and makes every other path +// answer 404 like a host with nothing mounted there. +func (s *fakeLFSServer) endpointFor(remoteURL string) string { + s.serve([]string{remoteURL}) + return remoteURL +} + +// disabledFor makes the given remotes answer 403 — the response a forge uses for +// a repository with LFS switched off — while the rest answer 404. +func (s *fakeLFSServer) disabledFor(remoteURLs ...string) { + disabled := make([]string, 0, len(remoteURLs)) + for _, remoteURL := range remoteURLs { + disabled = append(disabled, batchPathFor(remoteURL)) + } + s.mu.Lock() + s.disabledPaths = disabled + s.mu.Unlock() +} + +// serve mounts the batch API at the paths the given remotes address. +func (s *fakeLFSServer) serve(remoteURLs []string) { + served := make([]string, 0, len(remoteURLs)) + for _, remoteURL := range remoteURLs { + served = append(served, batchPathFor(remoteURL)) + } + s.mu.Lock() + s.servePaths = served + s.mu.Unlock() +} + +// batchPathFor is the batch action path a remote addresses. +func batchPathFor(remoteURL string) string { + parsed, err := url.Parse(remoteURL) + if err != nil { + return remoteURL + } + return parsed.Path + "/info/lfs/objects/batch" +} + func (s *fakeLFSServer) handleBatch(w http.ResponseWriter, r *http.Request) { s.mu.Lock() s.batchCalls++ @@ -117,12 +174,33 @@ func (s *fakeLFSServer) handleBatch(w http.ResponseWriter, r *http.Request) { for _, oid := range s.refusedOIDs { refused[oid] = struct{}{} } + actionless := make(map[string]struct{}, len(s.actionlessOIDs)) + for _, oid := range s.actionlessOIDs { + actionless[oid] = struct{}{} + } + disabled := slices.Contains(s.disabledPaths, r.URL.Path) + servedHere := slices.Contains(s.servePaths, r.URL.Path) + hasServePaths := len(s.servePaths) > 0 s.mu.Unlock() + // A path the server does not serve answers 404, the response that tells a + // client no LFS service is mounted there. if batchPath != "" && r.URL.Path != batchPath { w.WriteHeader(http.StatusNotFound) return } + if hasServePaths && !servedHere { + w.WriteHeader(http.StatusNotFound) + return + } + if disabled { + // A forge with LFS switched off explains itself in JSON; a host with no + // service behind the path answers with its own HTML page. + w.Header().Set("Content-Type", lfsMediaType) + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"message":"Git LFS is disabled for this repository."}`)) + return + } if status != 0 { w.WriteHeader(status) return @@ -160,6 +238,15 @@ func (s *fakeLFSServer) handleBatch(w http.ResponseWriter, r *http.Request) { }) continue } + if _, bare := actionless[object.OID]; bare { + // The server knows the object but schedules nothing for it: a + // successful response the client cannot act on. + response.Objects = append(response.Objects, batchResponseObject{ + OID: object.OID, + Size: int64(len(content)), + }) + continue + } response.Objects = append(response.Objects, batchResponseObject{ OID: object.OID, Size: int64(len(content)), @@ -286,15 +373,99 @@ func TestFetchAllSkipsEndpointWhenNoPointers(t *testing.T) { func TestFetchAllDisabledRemote(t *testing.T) { oid, pointerText := pointerFor([]byte("content")) - for _, status := range []int{http.StatusForbidden, http.StatusNotFound} { - lfsServer := newFakeLFSServer(t, map[string][]byte{oid: []byte("content")}) - lfsServer.batchStatus = status - repositoryPath := newRepoWithLFS(t, map[string]string{"file.bin": pointerText}) - - err := NewFetcher().FetchAll(context.Background(), repositoryPath, lfsServer.remoteURL(), "", "") - if !errors.Is(err, ErrDisabled) { - t.Errorf("status %d should map to ErrDisabled, got %v", status, err) - } + lfsServer := newFakeLFSServer(t, map[string][]byte{oid: []byte("content")}) + // Both candidate paths answer as a repository with LFS switched off. + lfsServer.disabledFor(lfsServer.plainRemoteURL(), lfsServer.remoteURL()) + repositoryPath := newRepoWithLFS(t, map[string]string{"file.bin": pointerText}) + + err := NewFetcher().FetchAll(context.Background(), repositoryPath, lfsServer.plainRemoteURL(), "", "") + if !errors.Is(err, ErrDisabled) { + t.Errorf("a 403 from every candidate should map to ErrDisabled, got %v", err) + } +} + +// TestFetchAllFallsBackToTheConfiguredPath covers a host that mounts its LFS +// service exactly where the remote points rather than under the repository's +// .git path: the suffixed endpoint answers 404, and the suffix-less one must +// then be tried rather than the remote being written off as LFS-free. +func TestFetchAllFallsBackToTheConfiguredPath(t *testing.T) { + content := []byte("content") + oid, pointerText := pointerFor(content) + + lfsServer := newFakeLFSServer(t, map[string][]byte{oid: content}) + remoteURL := lfsServer.endpointFor(lfsServer.plainRemoteURL()) + repositoryPath := newRepoWithLFS(t, map[string]string{"file.bin": pointerText}) + + if err := NewFetcher().FetchAll(context.Background(), repositoryPath, remoteURL, "", ""); err != nil { + t.Fatalf("FetchAll failed: %v", err) + } + cached, err := os.ReadFile(filepath.Join(repositoryPath, ".git", "lfs", "objects", + oid[0:2], oid[2:4], oid)) + if err != nil { + t.Fatalf("the object should be mirrored through the fallback endpoint: %v", err) + } + if string(cached) != string(content) { + t.Error("mirrored content should match the served bytes") + } +} + +// TestFetchAllFallsBackWhenTheGuessIsForbidden covers the guessed .git path +// answering 403 while the configured path serves normally. The 403 is about a +// path that does not exist rather than about the repository's LFS setting, so it +// must not be believed before the other candidate has been tried. +func TestFetchAllFallsBackWhenTheGuessIsForbidden(t *testing.T) { + content := []byte("content") + oid, pointerText := pointerFor(content) + + lfsServer := newFakeLFSServer(t, map[string][]byte{oid: content}) + remoteURL := lfsServer.endpointFor(lfsServer.plainRemoteURL()) + lfsServer.disabledFor(lfsServer.remoteURL()) + repositoryPath := newRepoWithLFS(t, map[string]string{"file.bin": pointerText}) + + if err := NewFetcher().FetchAll(context.Background(), repositoryPath, remoteURL, "", ""); err != nil { + t.Fatalf("FetchAll failed: %v", err) + } + if _, err := os.Stat(filepath.Join(repositoryPath, ".git", "lfs", "objects", + oid[0:2], oid[2:4], oid)); err != nil { + t.Errorf("the configured path should still be tried after a 403 on the guess: %v", err) + } +} + +// TestFetchAllNoEndpointAnywhereIsReported covers a host with no LFS service at +// either candidate path, which the mirror layer records as LFS being switched +// off — the two are indistinguishable from here, and both are expected states +// rather than a failed repository. +func TestFetchAllNoEndpointAnywhereIsReported(t *testing.T) { + oid, pointerText := pointerFor([]byte("content")) + + lfsServer := newFakeLFSServer(t, map[string][]byte{oid: []byte("content")}) + lfsServer.endpointFor(lfsServer.server.URL + "/somewhere/else") + repositoryPath := newRepoWithLFS(t, map[string]string{"file.bin": pointerText}) + + err := NewFetcher().FetchAll(context.Background(), repositoryPath, lfsServer.plainRemoteURL(), "", "") + if errors.Is(err, ErrNoEndpoint) { + t.Fatalf("err = %v, want the expected-skip verdict rather than an internal signal", err) + } + if !errors.Is(err, ErrDisabled) { + t.Fatalf("err = %v, want ErrDisabled", err) + } +} + +// TestFetchAllNotServedAnywhereIsSkipped covers a host whose every candidate +// path answers 404, which is both how a repository with LFS switched off looks +// on a forge that does not distinguish the two and how a host with no LFS +// service at all looks. Either way the mirror layer's expected skip is the right +// outcome, not a repository-wide failure. +func TestFetchAllNotServedAnywhereIsSkipped(t *testing.T) { + oid, pointerText := pointerFor([]byte("content")) + + lfsServer := newFakeLFSServer(t, map[string][]byte{oid: []byte("content")}) + lfsServer.endpointFor(lfsServer.server.URL + "/somewhere/else") + repositoryPath := newRepoWithLFS(t, map[string]string{"file.bin": pointerText}) + + err := NewFetcher().FetchAll(context.Background(), repositoryPath, lfsServer.plainRemoteURL(), "", "") + if !errors.Is(err, ErrDisabled) { + t.Fatalf("err = %v, want ErrDisabled when no candidate serves the API", err) } } @@ -358,9 +529,9 @@ func TestFetchAllUnknownObjectFails(t *testing.T) { } // TestFetchAllFallsBackWhenBatchRejected covers forges that reject a batch -// request outright — an object limit, or a batch they refuse to process. The -// pointers must be retried one at a time so a single refused object cannot cost -// the repository's whole LFS mirror, while a refusal that hits every object is +// request outright — an object limit, or a request they refuse to process. The +// pointers must be narrowed down so a single refused object cannot cost the +// repository's whole LFS mirror, while a refusal that hits every object is // reported as the endpoint's failure. func TestFetchAllFallsBackWhenBatchRejected(t *testing.T) { available := []byte("available content") @@ -384,6 +555,8 @@ func TestFetchAllFallsBackWhenBatchRejected(t *testing.T) { } return http.StatusOK }, + // The chunk splits into two singletons, both are served, and the + // candidate succeeds — so there is nothing left to probe. wantBatches: 3, wantCached: result(availableOID, refusedOID), }, @@ -397,6 +570,9 @@ func TestFetchAllFallsBackWhenBatchRejected(t *testing.T) { } return http.StatusOK }, + // The chunk splits once and the refused half's lone pointer is + // rejected on its own request; the remote already carries the + // suffix, so there is no second candidate path to probe. wantErr: "unavailable", wantBatches: 3, wantCached: result(availableOID), @@ -406,7 +582,9 @@ func TestFetchAllFallsBackWhenBatchRejected(t *testing.T) { request: func(batchRequest) int { return http.StatusBadRequest }, - wantErr: "rejected for 2 of 2 objects", + // Splitting, not per-object fan-out: both halves narrow to the + // rejection before it is reported as the endpoint's. + wantErr: "rejected", wantBatches: 3, }, } @@ -448,6 +626,56 @@ func TestFetchAllFallsBackWhenBatchRejected(t *testing.T) { } } +// TestFetchAllSkipsObjectsWithoutDownloadAction covers a successful batch +// response that schedules no action for one object: the object the client +// cannot act on must be reported, and the objects scheduled beside it must +// still reach the mirror (internal/lfs/batch.go turns the missing action into +// the unavailable sentinel that downloadChunk has to isolate). +func TestFetchAllSkipsObjectsWithoutDownloadAction(t *testing.T) { + first := []byte("first content") + firstOID, firstPointer := pointerFor(first) + second := []byte("second content") + secondOID, secondPointer := pointerFor(second) + third := []byte("third content") + thirdOID, thirdPointer := pointerFor(third) + + lfsServer := newFakeLFSServer(t, map[string][]byte{ + firstOID: first, + secondOID: second, + thirdOID: third, + }) + lfsServer.actionlessOIDs = []string{secondOID} + repositoryPath := newRepoWithLFS(t, map[string]string{ + "first.bin": firstPointer, + "second.bin": secondPointer, + "third.bin": thirdPointer, + }) + + err := NewFetcher().FetchAll(context.Background(), repositoryPath, lfsServer.remoteURL(), "", "") + if err == nil { + t.Fatal("an object without a download action should be reported") + } + if !strings.Contains(err.Error(), "could not be fetched") { + t.Fatalf("err = %v, want it to say how many objects were skipped", err) + } + if !errors.Is(err, errObjectUnavailable) { + t.Fatalf("err = %v, want it to report the object as unavailable", err) + } + + for _, oid := range []string{firstOID, thirdOID} { + path := filepath.Join(repositoryPath, ".git", "lfs", "objects", oid[0:2], oid[2:4], oid) + if _, err := os.Stat(path); err != nil { + t.Errorf("object %s should be mirrored beside the actionless one: %v", shortOID(oid), err) + } + } + if path := filepath.Join(repositoryPath, ".git", "lfs", "objects", secondOID[0:2], secondOID[2:4], secondOID); func() bool { + _, err := os.Stat(path) + return err == nil + }() { + t.Error("the actionless object must not be stored") + } +} + func TestFetchAllChunksLargePointerSets(t *testing.T) { files := make(map[string]string, batchObjectLimit+2) data := make(map[string][]byte, batchObjectLimit+2) @@ -476,11 +704,333 @@ func TestFetchAllChunksLargePointerSets(t *testing.T) { } } +// TestFetchAllStopsAfterRepeatedEndpointFailure covers an endpoint whose +// failures are not about the request's shape — dropped connections, and the +// credentials and server statuses that mean the same thing. Splitting the chunk +// would only repeat the answer, so the fetch must fail fast rather than narrow, +// while still attempting one more chunk before concluding the endpoint is down. +func TestFetchAllStopsAfterRepeatedEndpointFailure(t *testing.T) { + files := make(map[string]string, batchObjectLimit*5) + for index := range batchObjectLimit * 5 { + _, pointerText := pointerFor([]byte(fmt.Sprintf("object %d", index))) + files[fmt.Sprintf("file-%04d.bin", index)] = pointerText + } + repositoryPath := newRepoWithLFS(t, files) + + t.Run("dropped connections", func(t *testing.T) { + var requests int64 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + atomic.AddInt64(&requests, 1) + hijacked, _, err := w.(http.Hijacker).Hijack() + if err != nil { + return + } + _ = hijacked.Close() + })) + t.Cleanup(server.Close) + + err := NewFetcher().FetchAll(context.Background(), repositoryPath, server.URL+"/repo.git", "", "") + if err == nil { + t.Fatal("an endpoint that never answers should be reported") + } + if errors.Is(err, errObjectUnavailable) { + t.Errorf("err = %v, want a transport failure rather than per-object unavailability", err) + } + if got := atomic.LoadInt64(&requests); got != 2 { + t.Errorf("batch requests = %d, want one attempt per tried chunk and no splitting", got) + } + }) + + for _, status := range []int{http.StatusUnauthorized, http.StatusTooManyRequests, http.StatusInternalServerError} { + t.Run(http.StatusText(status), func(t *testing.T) { + lfsServer := newFakeLFSServer(t, nil) + lfsServer.batchStatus = status + + err := NewFetcher().FetchAll(context.Background(), repositoryPath, lfsServer.remoteURL(), "", "") + if err == nil || !strings.Contains(err.Error(), fmt.Sprint(status)) { + t.Fatalf("FetchAll = %v, want the endpoint's status", err) + } + if errors.Is(err, errObjectUnavailable) { + t.Errorf("err = %v, want the endpoint failure rather than per-object unavailability", err) + } + // No halving: one attempt for the first chunk, one for the second. + if calls := lfsServer.batchCallCount(); calls != 2 { + t.Errorf("batch calls = %d, want no splitting for a status about the endpoint", calls) + } + }) + } +} + +// TestFetchAllReportsSystematicRejection covers an endpoint that rejects every +// request as unprocessable, including single-object ones, so the rejection +// outlives every split. It must be reported as the endpoint's — not attributed +// to each object as unavailable — and the sweep must stop once a second chunk +// confirms it rather than narrowing its way through the whole repository. +func TestFetchAllReportsSystematicRejection(t *testing.T) { + const pointers = batchObjectLimit * 5 + files := make(map[string]string, pointers) + for index := range pointers { + _, pointerText := pointerFor([]byte(fmt.Sprintf("object %d", index))) + files[fmt.Sprintf("file-%04d.bin", index)] = pointerText + } + + lfsServer := newFakeLFSServer(t, nil) + lfsServer.batchStatus = http.StatusUnprocessableEntity + repositoryPath := newRepoWithLFS(t, files) + + err := NewFetcher().FetchAll(context.Background(), repositoryPath, lfsServer.remoteURL(), "", "") + if err == nil || !strings.Contains(err.Error(), "rejected") { + t.Fatalf("FetchAll = %v, want the endpoint rejection reported", err) + } + if errors.Is(err, errObjectUnavailable) { + t.Errorf("err = %v, want an endpoint rejection rather than per-object unavailability", err) + } + if !strings.Contains(err.Error(), "422") { + t.Errorf("err = %v, want the endpoint's status", err) + } + + // Two chunks are narrowed — the first to establish the rejection and the + // second to confirm it is the endpoint's — and the rest are left alone. A + // per-pointer sweep of this repository would take five hundred requests; the + // remote carries its suffix, so only one candidate path is tried. + if calls := lfsServer.batchCallCount(); calls > 450 { + t.Errorf("batch calls = %d, want the systematic rejection to stop the sweep", calls) + } +} + +// TestFetchAllNarrowsBothHalves covers a rejection that narrows onto a broken +// half: the sibling half must still be requested and downloaded, rather than +// being abandoned because the first half reported the endpoint's rejection. +func TestFetchAllNarrowsBothHalves(t *testing.T) { + bad := []byte("bad content") + badOID, badPointer := pointerFor(bad) + good := []byte("good content") + goodOID, goodPointer := pointerFor(good) + + lfsServer := newFakeLFSServer(t, map[string][]byte{ + badOID: bad, + goodOID: good, + }) + // Every request naming the bad object is refused, so the chunk narrows down + // its first half while the second half is served normally. + lfsServer.rejectWhen = func(request batchRequest) int { + for _, object := range request.Objects { + if object.OID == badOID { + return http.StatusUnprocessableEntity + } + } + return http.StatusOK + } + repositoryPath := newRepoWithLFS(t, map[string]string{ + "bad.bin": badPointer, + "good.bin": goodPointer, + }) + + err := NewFetcher().FetchAll(context.Background(), repositoryPath, lfsServer.remoteURL(), "", "") + if err == nil || !strings.Contains(err.Error(), "unavailable") { + t.Fatalf("FetchAll = %v, want the refused object reported", err) + } + if _, statErr := os.Stat(filepath.Join(repositoryPath, ".git", "lfs", "objects", + goodOID[0:2], goodOID[2:4], goodOID)); statErr != nil { + t.Errorf("the sibling half should be downloaded: %v", statErr) + } +} + +// TestFetchAllKeepsObjectsBesideAnAnsweredRejection covers a chunk whose halves +// disagree: one half's batch is refused down to a single object, while the other +// half's batch is answered with an object the endpoint will not serve. The +// answer proves the endpoint processes batches, so the refused object must not be +// mistaken for a broken endpoint — which would end the fetch and leave the +// answered half's siblings unattempted. +func TestFetchAllKeepsObjectsBesideAnAnsweredRejection(t *testing.T) { + held := []byte("held content") + heldOID, heldPointer := pointerFor(held) + phantom := []byte("phantom content") + phantomOID, phantomPointer := pointerFor(phantom) + refused := []byte("refused content") + refusedOID, refusedPointer := pointerFor(refused) + + lfsServer := newFakeLFSServer(t, map[string][]byte{ + heldOID: held, + phantomOID: phantom, + refusedOID: refused, + }) + lfsServer.refusedOIDs = []string{phantomOID} + lfsServer.rejectWhen = func(request batchRequest) int { + for _, object := range request.Objects { + if object.OID == refusedOID { + return http.StatusUnprocessableEntity + } + } + return http.StatusOK + } + repositoryPath := newRepoWithLFS(t, map[string]string{ + "held.bin": heldPointer, + "phantom.bin": phantomPointer, + "refused.bin": refusedPointer, + }) + + err := NewFetcher().FetchAll(context.Background(), repositoryPath, lfsServer.remoteURL(), "", "") + if err == nil || !strings.Contains(err.Error(), "unavailable") { + t.Fatalf("FetchAll = %v, want the two unfetchable objects reported", err) + } + + for _, oid := range []string{heldOID} { + if _, statErr := os.Stat(filepath.Join(repositoryPath, ".git", "lfs", "objects", + oid[0:2], oid[2:4], oid)); statErr != nil { + t.Errorf("object %s should be mirrored: %v", shortOID(oid), statErr) + } + } +} + +// TestFetchAllKeepsSweepingAfterARefusedChunk covers a chunk the endpoint +// answers nothing for — every object refused, which alone looks like an endpoint +// rejecting every request — followed by a chunk it serves. The refused chunk +// must not end the sweep, or a chunk of stale pointers would cost the repository +// everything after it. +func TestFetchAllKeepsSweepingAfterARefusedChunk(t *testing.T) { + const staleInFirstChunk = 3 + files := make(map[string]string, batchObjectLimit*2) + data := make(map[string][]byte, batchObjectLimit*2) + var staleOIDs []string + for index := range batchObjectLimit * 2 { + content := []byte(fmt.Sprintf("object %d", index)) + oid, pointerText := pointerFor(content) + data[oid] = content + files[fmt.Sprintf("file-%04d.bin", index)] = pointerText + if index < staleInFirstChunk { + staleOIDs = append(staleOIDs, oid) + } + } + + lfsServer := newFakeLFSServer(t, data) + lfsServer.rejectWhen = func(request batchRequest) int { + for _, object := range request.Objects { + for _, stale := range staleOIDs { + if object.OID == stale { + return http.StatusUnprocessableEntity + } + } + } + return http.StatusOK + } + repositoryPath := newRepoWithLFS(t, files) + + err := NewFetcher().FetchAll(context.Background(), repositoryPath, lfsServer.remoteURL(), "", "") + if err == nil || !strings.Contains(err.Error(), "could not be fetched") { + t.Fatalf("FetchAll = %v, want the refused objects summarised", err) + } + + // The chunk after the refused one is still fetched. + last := fmt.Sprintf("object %d", batchObjectLimit*2-1) + lastOID, _ := pointerFor([]byte(last)) + if _, statErr := os.Stat(filepath.Join(repositoryPath, ".git", "lfs", "objects", + lastOID[0:2], lastOID[2:4], lastOID)); statErr != nil { + t.Errorf("the chunk after a refused one should still be fetched: %v", statErr) + } + for _, oid := range staleOIDs { + if _, statErr := os.Stat(filepath.Join(repositoryPath, ".git", "lfs", "objects", + oid[0:2], oid[2:4], oid)); statErr == nil { + t.Errorf("refused object %s must not be stored", shortOID(oid)) + } + } +} + +// TestFetchAllKeepsObjectsBesideARefusedHalf covers a chunk whose first half is +// refused in full while its sibling is servable: the first half answers nothing +// at all, which looks like an endpoint-wide rejection, and concluding that from +// one half alone would abandon the sibling — and, one chunk later, the rest of +// the repository. +func TestFetchAllKeepsObjectsBesideARefusedHalf(t *testing.T) { + staleA := []byte("stale a") + staleAOID, staleAPointer := pointerFor(staleA) + staleB := []byte("stale b") + staleBOID, staleBPointer := pointerFor(staleB) + goodC := []byte("good c") + goodCOID, goodCPointer := pointerFor(goodC) + + lfsServer := newFakeLFSServer(t, map[string][]byte{ + staleAOID: staleA, + staleBOID: staleB, + goodCOID: goodC, + }) + // Requests naming either stale object are refused, so a half holding both + // narrows to rejections that the endpoint never answers. + lfsServer.rejectWhen = func(request batchRequest) int { + for _, object := range request.Objects { + if object.OID == staleAOID || object.OID == staleBOID { + return http.StatusUnprocessableEntity + } + } + return http.StatusOK + } + repositoryPath := newRepoWithLFS(t, map[string]string{ + "stale-a.bin": staleAPointer, + "stale-b.bin": staleBPointer, + "good-c.bin": goodCPointer, + }) + + err := NewFetcher().FetchAll(context.Background(), repositoryPath, lfsServer.remoteURL(), "", "") + if err == nil || !strings.Contains(err.Error(), "unavailable") { + t.Fatalf("FetchAll = %v, want the refused objects reported", err) + } + if _, statErr := os.Stat(filepath.Join(repositoryPath, ".git", "lfs", "objects", + goodCOID[0:2], goodCOID[2:4], goodCOID)); statErr != nil { + t.Errorf("the servable sibling half should still be fetched: %v", statErr) + } +} + // TestCollectPointersHandlesWindowsIllegalNames covers a tree entry that cannot // be materialised on the host — a backslash in a name is a path separator on // Windows — and one that reuses a subtree hash, as a submodule-like entry does. // Both must be walked without tripping the scan: the pointer beside them is // still collected, and the shared tree is visited once. +// TestFetchAllReportsAnEndpointThatStopsPartway covers a service that serves +// the first chunk and then answers as if it were not there any more. Part of the +// repository is mirrored by then, so the "no LFS here" verdict must not be +// recorded as a clean skip: that would report a half-finished mirror as +// complete. +func TestFetchAllReportsAnEndpointThatStopsPartway(t *testing.T) { + const pointers = batchObjectLimit * 2 + files := make(map[string]string, pointers) + data := make(map[string][]byte, pointers) + for index := range pointers { + content := []byte(fmt.Sprintf("object %d", index)) + oid, pointerText := pointerFor(content) + data[oid] = content + files[fmt.Sprintf("file-%04d.bin", index)] = pointerText + } + + lfsServer := newFakeLFSServer(t, data) + // The first chunk is served; every request after it looks like a host with + // nothing mounted at the path. + var calls int64 + lfsServer.rejectWhen = func(batchRequest) int { + if atomic.AddInt64(&calls, 1) > 1 { + return http.StatusNotFound + } + return http.StatusOK + } + repositoryPath := newRepoWithLFS(t, files) + + err := NewFetcher().FetchAll(context.Background(), repositoryPath, lfsServer.remoteURL(), "", "") + if err == nil { + t.Fatal("an endpoint that stops serving the rest should be reported") + } + // ErrDisabled is how the mirror layer recognises an expected skip, so the + // partial-mirror error must not match it through the chain. + if errors.Is(err, ErrDisabled) || errors.Is(err, ErrNoEndpoint) { + t.Fatalf("err = %v, want a failure rather than a clean skip for a partial mirror", err) + } + + // The chunk that was served did reach the mirror. + firstOID, _ := pointerFor([]byte("object 0")) + if _, statErr := os.Stat(filepath.Join(repositoryPath, ".git", "lfs", "objects", + firstOID[0:2], firstOID[2:4], firstOID)); statErr != nil { + t.Errorf("the served chunk should be mirrored: %v", statErr) + } +} + func TestCollectPointersHandlesWindowsIllegalNames(t *testing.T) { dir := t.TempDir() repository, err := git.PlainInit(dir, false) @@ -531,9 +1081,21 @@ func TestCollectPointersHandlesWindowsIllegalNames(t *testing.T) { t.Fatal("nested subtree not found") } + // An empty subtree the hostile tree names twice, so the deduplication is + // exercised without competing with the commit's own trees for the visit. + empty := &object.Tree{} + emptyObject := repository.Storer.NewEncodedObject() + if err := empty.Encode(emptyObject); err != nil { + t.Fatal(err) + } + emptyHash, err := repository.Storer.SetEncodedObject(emptyObject) + if err != nil { + t.Fatal(err) + } + hostile := &object.Tree{Entries: []object.TreeEntry{ - {Name: "nested-again", Hash: nested.Hash, Mode: filemode.Dir}, - {Name: "nested", Hash: nested.Hash, Mode: filemode.Dir}, + {Name: "empty-again", Hash: emptyHash, Mode: filemode.Dir}, + {Name: "empty", Hash: emptyHash, Mode: filemode.Dir}, {Name: `src\windows.cpp`, Hash: plumbing.NewHash(strings.Repeat("a", 40)), Mode: filemode.Regular}, }} encoded := repository.Storer.NewEncodedObject() @@ -545,17 +1107,128 @@ func TestCollectPointersHandlesWindowsIllegalNames(t *testing.T) { t.Fatal(err) } - headRef := plumbing.NewHashReference(plumbing.NewBranchReferenceName("hostile"), hostileHash) + // A ref points at a commit, and the scan reaches trees through commits, so + // the hostile tree is committed before the ref names it. + signature := object.Signature{Name: "test", Email: "test@example.com", When: time.Now()} + hostileCommit := &object.Commit{ + Author: signature, + Committer: signature, + Message: "hostile tree", + TreeHash: hostileHash, + } + commitObject := repository.Storer.NewEncodedObject() + if err := hostileCommit.Encode(commitObject); err != nil { + t.Fatal(err) + } + hostileCommitHash, err := repository.Storer.SetEncodedObject(commitObject) + if err != nil { + t.Fatal(err) + } + + headRef := plumbing.NewHashReference(plumbing.NewBranchReferenceName("hostile"), hostileCommitHash) if err := repository.Storer.SetReference(headRef); err != nil { t.Fatal(err) } - pointers, err := collectPointers(context.Background(), repository) + scan, err := collectPointers(context.Background(), repository) if err != nil { t.Fatalf("collectPointers failed on a tree with host-specific names: %v", err) } - if len(pointers) != 1 || pointers[0].oid != oid { - t.Fatalf("pointers = %+v, want the single pointer %s", pointers, oid) + if len(scan.pointers) != 1 || scan.pointers[0].oid != oid { + t.Fatalf("pointers = %+v, want the single pointer %s", scan.pointers, oid) + } + // The hostile entry names a blob that was never stored, so the scan reports + // it rather than quietly reading a smaller set. The empty subtree behind the + // two directory entries is visited once, so it is not reported as skipped. + if len(scan.skipped) != 1 || scan.skipped[0].name != `src\windows.cpp` { + t.Errorf("skipped = %+v, want just the entry whose blob is missing", scan.skipped) + } +} + +// TestCollectPointersReportsUnreadableSubtrees covers a subtree whose object is +// missing — a partial fetch, say. Its pointers cannot be fetched, so the scan +// has to name them as skipped rather than reporting a smaller pointer set as if +// it were the whole truth. +func TestCollectPointersReportsUnreadableSubtrees(t *testing.T) { + const missingName = "missing" + repositoryPath := addMissingSubtreeCommit(t, missingName) + + repository, err := git.PlainOpen(repositoryPath) + if err != nil { + t.Fatal(err) + } + + scan, err := collectPointers(context.Background(), repository) + if err != nil { + t.Fatalf("a missing subtree should not fail the scan: %v", err) + } + if len(scan.skipped) != 1 || scan.skipped[0].name != missingName { + t.Fatalf("skipped = %+v, want the unreadable subtree named", scan.skipped) + } + if scan.skipped[0].err == nil { + t.Error("the skip should carry the reason it was skipped") + } +} + +// addMissingSubtreeCommit builds a repository whose history names a subtree +// object that was never stored, so the pointer scan has something it cannot +// read, and returns its path. +func addMissingSubtreeCommit(t *testing.T, name string) string { + t.Helper() + + repositoryPath := newRepoWithLFS(t, map[string]string{"README.md": "plain repo"}) + repository, err := git.PlainOpen(repositoryPath) + if err != nil { + t.Fatal(err) + } + + tree := &object.Tree{Entries: []object.TreeEntry{ + {Name: name, Hash: plumbing.NewHash(strings.Repeat("b", 40)), Mode: filemode.Dir}, + }} + treeObject := repository.Storer.NewEncodedObject() + if err := tree.Encode(treeObject); err != nil { + t.Fatal(err) + } + treeHash, err := repository.Storer.SetEncodedObject(treeObject) + if err != nil { + t.Fatal(err) + } + + signature := object.Signature{Name: "test", Email: "test@example.com", When: time.Now()} + commit := &object.Commit{ + Author: signature, + Committer: signature, + Message: "point at a missing subtree", + TreeHash: treeHash, + } + commitObject := repository.Storer.NewEncodedObject() + if err := commit.Encode(commitObject); err != nil { + t.Fatal(err) + } + commitHash, err := repository.Storer.SetEncodedObject(commitObject) + if err != nil { + t.Fatal(err) + } + if err := repository.Storer.SetReference(plumbing.NewHashReference(plumbing.NewBranchReferenceName("hostile"), commitHash)); err != nil { + t.Fatal(err) + } + return repositoryPath +} + +// TestFetchAllFailsWhenTheScanCouldNotReadEverything covers what the mirror +// layer sees when part of the repository cannot be read: the pointers behind it +// cannot be fetched either, so the fetch reports rather than returning success +// for a mirror that is quietly incomplete. +func TestFetchAllFailsWhenTheScanCouldNotReadEverything(t *testing.T) { + lfsServer := newFakeLFSServer(t, nil) + repositoryPath := addMissingSubtreeCommit(t, "missing") + + err := NewFetcher().FetchAll(context.Background(), repositoryPath, lfsServer.remoteURL(), "", "") + if err == nil || !strings.Contains(err.Error(), "could not be read") { + t.Fatalf("FetchAll = %v, want the unreadable entry reported", err) + } + if calls := lfsServer.batchCallCount(); calls != 0 { + t.Errorf("batch calls = %d, want no endpoint work for a scan that cannot complete", calls) } } @@ -619,7 +1292,8 @@ func TestCanonicalHostStripsSchemeDefaultPorts(t *testing.T) { // TestDefaultEndpointAddsGitSuffix pins the endpoint a forge expects: GitHub // and GitLab answer a suffix-less /info/lfs with 422, so a configured remote -// without .git still has to request the suffixed path. +// without .git still has to request the suffixed path. Everything else the +// remote URL carries has to survive the rewrite. func TestDefaultEndpointAddsGitSuffix(t *testing.T) { cases := []struct { remoteURL string @@ -627,18 +1301,53 @@ func TestDefaultEndpointAddsGitSuffix(t *testing.T) { }{ {"https://github.com/owner/repo", "https://github.com/owner/repo.git/info/lfs"}, {"https://github.com/owner/repo.git", "https://github.com/owner/repo.git/info/lfs"}, + {"https://github.com/owner/repo.GIT", "https://github.com/owner/repo.GIT/info/lfs"}, {"https://github.com/owner/repo/", "https://github.com/owner/repo.git/info/lfs"}, {"https://git.example.com:8443/group/sub/repo", "https://git.example.com:8443/group/sub/repo.git/info/lfs"}, + {"https://user:secret@git.example.com/owner/repo", "https://user:secret@git.example.com/owner/repo.git/info/lfs"}, + {"https://git.example.com", "https://git.example.com/info/lfs"}, + {"https://git.example.com/", "https://git.example.com/info/lfs"}, + {"https://git.example.com/owner/repo?", "https://git.example.com/owner/repo.git/info/lfs"}, + {"https://git.example.com/owner/repo#frag", "https://git.example.com/owner/repo.git/info/lfs"}, + {"https://git.example.com/owner/re%2Fpo", "https://git.example.com/owner/re%2Fpo.git/info/lfs"}, + {"https://git.example.com/owner/re po", "https://git.example.com/owner/re%20po.git/info/lfs"}, } for _, c := range cases { - t.Run(c.remoteURL, func(t *testing.T) { + t.Run(redactedURL(c.remoteURL), func(t *testing.T) { parsed, err := url.Parse(c.remoteURL) if err != nil { t.Fatal(err) } if got := defaultEndpoint(parsed); got != c.want { - t.Errorf("defaultEndpoint(%q) = %q, want %q", c.remoteURL, got, c.want) + t.Errorf("defaultEndpoint(%q) = %q, want %q", redactedURL(c.remoteURL), redactedURL(got), redactedURL(c.want)) + } + }) + } +} + +// TestResolveEndpointsCollapsesDuplicateCandidates covers remotes that reach the +// LFS API at one path whichever candidate is derived — a remote that already +// carries the repository suffix, and one with no path at all. Probing the same +// URL twice would repeat every request of a whole repository for nothing. +func TestResolveEndpointsCollapsesDuplicateCandidates(t *testing.T) { + for _, remoteURL := range []string{ + "https://git.example.com/owner/repo.git", + "https://git.example.com", + } { + t.Run(redactedURL(remoteURL), func(t *testing.T) { + repositoryPath := newRepoWithLFS(t, map[string]string{"README.md": "plain repo"}) + repository, err := git.PlainOpen(repositoryPath) + if err != nil { + t.Fatal(err) + } + + endpoints, err := resolveEndpoints(repository, remoteURL) + if err != nil { + t.Fatalf("resolveEndpoints failed: %v", err) + } + if len(endpoints) != 1 { + t.Errorf("endpoints = %q, want one candidate for a remote that has only one path", redactedURL(strings.Join(endpoints, ","))) } }) } @@ -659,6 +1368,12 @@ func TestResolveEndpointOverrideHostAndScheme(t *testing.T) { remoteURL: "https://example.com/repo.git", wantAddress: "https://example.com/custom/lfs", }, + { + name: "endpoint rooted at the host", + lfsURL: "https://example.com:443/", + remoteURL: "https://example.com/repo.git", + wantAddress: "https://example.com", + }, { name: "http override downgrades the https remote", lfsURL: "http://example.com/custom/lfs", @@ -691,21 +1406,21 @@ func TestResolveEndpointOverrideHostAndScheme(t *testing.T) { t.Fatal(err) } - address, err := resolveEndpoint(repository, c.remoteURL) + endpoints, err := resolveEndpoints(repository, c.remoteURL) if c.wantReject { if err == nil { - t.Fatalf("resolveEndpoint = %q, want rejection", address) + t.Fatalf("resolveEndpoints = %q, want rejection", redactedURL(strings.Join(endpoints, ","))) } return } if err != nil { - t.Fatalf("resolveEndpoint failed: %v", err) + t.Fatalf("resolveEndpoints failed: %v", err) } - if address != c.wantAddress { - t.Errorf("endpoint = %q, want %q", address, c.wantAddress) + if len(endpoints) != 1 || endpoints[0] != c.wantAddress { + t.Errorf("endpoints = %q, want just %q", redactedURL(strings.Join(endpoints, ",")), redactedURL(c.wantAddress)) } if lfsServer.batchCallCount() != 0 { - t.Error("resolveEndpoint must not contact any endpoint") + t.Error("resolveEndpoints must not contact any endpoint") } }) } diff --git a/internal/lfs/scan.go b/internal/lfs/scan.go index ad9b5e9..e9da170 100644 --- a/internal/lfs/scan.go +++ b/internal/lfs/scan.go @@ -20,6 +20,21 @@ type pointer struct { size int64 } +// skippedSubtree records a subtree the scan could not read, so its pointers are +// known to be missing from the result rather than silently absent. +type skippedSubtree struct { + name string + hash plumbing.Hash + err error +} + +// scanResult is what one scan of a repository found: the pointers it collected +// and the subtrees it could not read. +type scanResult struct { + pointers []pointer + skipped []skippedSubtree +} + // pointerVersionValue is the version scheme URI on the first line of every // LFS pointer file. const pointerVersionValue = "https://git-lfs.github.com/spec/v1" @@ -32,12 +47,13 @@ const pointerMaxBytes = 4 * 1024 // inspected once. Commit history is traversed explicitly (rather than per-ref // iterators) so a repository with many refs costs one pass over its history, // not one per ref. -func collectPointers(ctx context.Context, repository *git.Repository) ([]pointer, error) { +func collectPointers(ctx context.Context, repository *git.Repository) (scanResult, error) { seenCommits := make(map[plumbing.Hash]struct{}) seenTrees := make(map[plumbing.Hash]bool) seenBlobs := make(map[plumbing.Hash]struct{}) var pointers []pointer + var skipped []skippedSubtree seenPointers := make(map[string]struct{}) enqueue := func(queue []plumbing.Hash, hash plumbing.Hash) []plumbing.Hash { @@ -53,7 +69,7 @@ func collectPointers(ctx context.Context, repository *git.Repository) ([]pointer queue := make([]plumbing.Hash, 0, 64) refs, err := repository.References() if err != nil { - return nil, err + return scanResult{}, err } err = refs.ForEach(func(ref *plumbing.Reference) error { if ref.Type() == plumbing.SymbolicReference || ref.Name().IsRemote() { @@ -63,12 +79,12 @@ func collectPointers(ctx context.Context, repository *git.Repository) ([]pointer return ctx.Err() }) if err != nil { - return nil, err + return scanResult{}, err } for len(queue) > 0 { if err := ctx.Err(); err != nil { - return nil, err + return scanResult{}, err } hash := queue[len(queue)-1] @@ -86,8 +102,8 @@ func collectPointers(ctx context.Context, repository *git.Repository) ([]pointer continue } - if err := scanTree(repository, commit.TreeHash, seenTrees, seenBlobs, seenPointers, &pointers); err != nil { - return nil, err + if err := scanTree(repository, commit.TreeHash, seenTrees, seenBlobs, seenPointers, &skipped, &pointers); err != nil { + return scanResult{}, err } for _, parent := range commit.ParentHashes { @@ -95,7 +111,7 @@ func collectPointers(ctx context.Context, repository *git.Repository) ([]pointer } } - return pointers, nil + return scanResult{pointers: pointers, skipped: skipped}, nil } // peelToCommitHash resolves a ref tip hash to the commit it designates, @@ -137,6 +153,7 @@ func scanTree( treeHash plumbing.Hash, seenTrees map[plumbing.Hash]bool, seenBlobs map[plumbing.Hash]struct{}, seenPointers map[string]struct{}, + skipped *[]skippedSubtree, pointers *[]pointer, ) error { tree, err := repository.TreeObject(treeHash) @@ -161,7 +178,9 @@ func scanTree( subtree, err := repository.TreeObject(entry.Hash) if err != nil { // A subtree missing from a partial fetch costs its - // pointers but leaves the rest of the scan intact. + // pointers but leaves the rest of the scan intact; record + // it so the loss is reported rather than silent. + *skipped = append(*skipped, skippedSubtree{name: entry.Name, hash: entry.Hash, err: err}) continue } pending = append(pending, subtree) @@ -177,6 +196,9 @@ func scanTree( blob, err := repository.BlobObject(entry.Hash) if err != nil { + // A blob missing from a partial fetch costs whatever pointer it + // held; record it so the loss is reported rather than silent. + *skipped = append(*skipped, skippedSubtree{name: entry.Name, hash: entry.Hash, err: err}) continue } if blob.Size > pointerMaxBytes { @@ -185,11 +207,13 @@ func scanTree( reader, err := blob.Reader() if err != nil { + *skipped = append(*skipped, skippedSubtree{name: entry.Name, hash: entry.Hash, err: err}) continue } content, err := io.ReadAll(io.LimitReader(reader, pointerMaxBytes)) _ = reader.Close() if err != nil { + *skipped = append(*skipped, skippedSubtree{name: entry.Name, hash: entry.Hash, err: err}) continue } From 21b7e75373be4bdf8d3bb6ba10db85b421991141 Mon Sep 17 00:00:00 2001 From: Neureka Date: Thu, 10 Sep 2026 12:51:10 -0700 Subject: [PATCH 03/15] refactor(lfs): settle the endpoint before fetching from it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FetchAll had grown a three-accumulator decision state machine (disabledErr, rejectedErr, hardErr) with a precedence ladder, because discovery and fetching shared one channel: the candidates were guessed, validated lazily by the first chunk, and every later failure arrived through the same error return. Callers then could not tell a clean skip from a half-finished mirror without extra per-request bookkeeping, and the ordering rules had to be revisited repeatedly. Discovery now happens once, before any object is fetched. endpointCandidates lists the API roots a host might serve; selectEndpoint asks each one for a single object and settles on the one that answers, treating a refusal at one object as a wrong path rather than a request-shape problem. FetchAll is then three steps with one job each — reject the remote URL, scan the mirror, prove the endpoint, fetch — and the fetch runs against a single known-good endpoint with no fallback to reconcile. That removes the precedence ladder, the served-count bookkeeping that existed only to classify endpoint verdicts, and the 403-body heuristic that guessed a cause from a response body. It also removes the class of bug those were patching: no candidate can be mistaken for a missing one partway through a repository, because by the time objects are mirrored there is only one candidate. A refusal that a server reports as unprocessable, rather than per object, now means the API is not mounted at that path, which is what a single-object request can only be telling us; chunk-level refusals during the fetch still narrow by halving. Scanning, per-object isolation, and the unavailable gap report are unchanged. --- internal/lfs/batch.go | 139 ++++++++++------ internal/lfs/endpoint.go | 184 ++++++++++++++++++++++ internal/lfs/lfs.go | 331 ++++++++++----------------------------- internal/lfs/lfs_test.go | 199 +++++++++++++---------- 4 files changed, 473 insertions(+), 380 deletions(-) create mode 100644 internal/lfs/endpoint.go diff --git a/internal/lfs/batch.go b/internal/lfs/batch.go index 0dae1e9..62e597d 100644 --- a/internal/lfs/batch.go +++ b/internal/lfs/batch.go @@ -92,15 +92,69 @@ func newBatchClient(client *http.Client) *batchClient { const lfsMediaType = "application/vnd.git-lfs+json" +// probe asks an endpoint for one object to find out whether it serves this +// repository's LFS API at all. Discovery happens once, before any object is +// fetched, so the fetch itself runs against a single known-good endpoint: the +// answer separates "the API answered for this object" — including that the +// server does not have it — from "LFS is switched off here" and from "nothing is +// mounted at this path". +// +// A single object keeps the probe from tripping a server's object limit, so a +// refused probe is about the path rather than the request's shape. A nil return +// therefore means the API root is right, and the object's own fate is the +// fetch's business. +func (c *batchClient) probe(ctx context.Context, endpoint, username, password string, object pointer) error { + body, err := json.Marshal(batchRequest{ + Operation: "download", + Transfers: []string{"basic"}, + Objects: toBatchObjects([]pointer{object}), + }) + if err != nil { + return err + } + + request, err := http.NewRequestWithContext(ctx, http.MethodPost, endpoint+"/objects/batch", bytes.NewReader(body)) + if err != nil { + return err + } + request.Header.Set("Content-Type", lfsMediaType) + request.Header.Set("Accept", lfsMediaType) + if username != "" || password != "" { + request.SetBasicAuth(username, password) + } + + response, err := c.client.Do(request) + if err != nil { + return fmt.Errorf("batch request: %w", err) + } + defer drainAndClose(response) + + switch response.StatusCode { + case http.StatusOK: + // The API answered for the object, whether by scheduling it or by + // reporting that the server does not have it. + return nil + case http.StatusForbidden: + // The forge has LFS switched off for this repository. + return ErrDisabled + case http.StatusNotFound, http.StatusUnprocessableEntity: + // Nothing is mounted at this path: a host that routes by path answers + // 404, and one that validates the action answers a single-object + // request with 422. + return ErrNoEndpoint + default: + return fmt.Errorf("batch request failed with status %d", response.StatusCode) + } +} + // batch submits every pointer for download scheduling. // -// A 403 means the remote has Git LFS switched off, which callers treat as an -// expected skip rather than a failure. A 404 means the LFS service answered -// about an object it does not have, which callers record per object. A status -// describing the request's shape is reported as errBatchRejected so callers can -// retry the pointers in smaller batches; every other status describes the -// endpoint rather than what was asked of it, so splitting the chunk would only -// repeat the same answer more slowly. +// A status describing the request's shape is reported as errBatchRejected so +// callers can retry the pointers in smaller batches; every other status +// describes the endpoint rather than what was asked of it, so splitting the +// chunk would only repeat the same answer more slowly. Discovery already +// established which endpoint serves the API, so a 403 or 404 here describes a +// service that has stopped answering rather than a path worth retrying. func (c *batchClient) batch(ctx context.Context, endpoint, username, password string, pointers []pointer) ([]batchResponseObject, error) { body, err := json.Marshal(batchRequest{ Operation: "download", @@ -127,23 +181,9 @@ func (c *batchClient) batch(ctx context.Context, endpoint, username, password st } defer drainAndClose(response) - if response.StatusCode == http.StatusForbidden { - // A forge that has LFS switched off answers 403 with an explanation, - // while a path with nothing mounted behind it answers 403 only if the - // host refuses unmatched paths. The body separates the two, and the - // difference decides whether the repository is skipped or another - // candidate path is tried. - if body, err := io.ReadAll(io.LimitReader(response.Body, 4096)); err == nil && looksLikeJSON(body) { - return nil, ErrDisabled - } - return nil, ErrNoEndpoint - } switch { case response.StatusCode == http.StatusOK: // Handled below. - case response.StatusCode == http.StatusNotFound: - // No LFS action answered at this path, which another candidate may. - return nil, ErrNoEndpoint case describesRequestShape(response.StatusCode): return nil, fmt.Errorf("%w: status %d", errBatchRejected, response.StatusCode) default: @@ -177,29 +217,22 @@ func describesRequestShape(status int) bool { } } -// looksLikeJSON reports whether a body is a JSON document, which is how the LFS -// batch API explains itself and how an HTML error page does not. -func looksLikeJSON(body []byte) bool { - trimmed := bytes.TrimSpace(body) - return len(trimmed) > 0 && (trimmed[0] == '{' || trimmed[0] == '[') -} - // downloadObjects streams each scheduled object into the repository's LFS // cache, verifying its SHA-256 as bytes arrive. Objects already cached are // skipped, so repeated snapshots only download new content. // // Every object is attempted whatever the others did, so one bad pointer or one // transient transfer failure cannot hide its siblings. What failed comes back in -// the two lists: unavailable for objects the endpoint will not serve, and failed -// for objects this client could not transfer — a corrupt body, a broken -// connection — which the caller reports rather than recording as the endpoint's -// refusal. +// the three lists: cached for objects already in the cache, unavailable for +// objects the endpoint will not serve, and failed for objects this client could +// not transfer — a corrupt body, a broken connection — which the caller reports +// rather than recording as the endpoint's refusal. func downloadObjects( ctx context.Context, client *batchClient, store, endpoint, username, password string, objects []batchResponseObject, -) (unavailable []*batchObjectError, failed []error) { +) (cached, unavailable []*batchObjectError, failed []error) { endpointHost := hostOf(endpoint) answered := func(object batchResponseObject) *batchObjectError { switch { @@ -216,34 +249,44 @@ func downloadObjects( for _, object := range objects { if err := ctx.Err(); err != nil { - return unavailable, append(failed, err) + return cached, unavailable, append(failed, err) } if refused := answered(object); refused != nil { unavailable = append(unavailable, refused) continue } - if err := downloadObject(ctx, client.client, store, endpointHost, username, password, object.OID, object.Actions["download"]); err != nil { + stored, err := downloadObject(ctx, client.client, store, endpointHost, username, password, object.OID, object.Actions["download"]) + switch { + case err != nil: failed = append(failed, err) + case stored: + // Already in the cache: the endpoint answered for it, but this + // request never streamed it. + cached = append(cached, &batchObjectError{oid: object.OID, message: "already in the local cache"}) } } - return unavailable, failed + return cached, unavailable, failed } +// downloadObject streams one object into the LFS cache, verifying its SHA-256 as +// bytes arrive. It reports whether the object was already cached, so a caller can +// tell an endpoint that streamed the bytes from one that merely answered for +// them. func downloadObject( ctx context.Context, client *http.Client, store, endpointHost, username, password, oid string, action *batchAction, -) error { +) (bool, error) { destination := filepath.Join(store, oid[0:2], oid[2:4], oid) if _, err := os.Stat(destination); err == nil { // Already cached by a previous snapshot; git-lfs also skips it. - return nil + return true, nil } request, err := http.NewRequestWithContext(ctx, http.MethodGet, action.Href, nil) if err != nil { - return fmt.Errorf("LFS object %s: %w", shortOID(oid), err) + return false, fmt.Errorf("LFS object %s: %w", shortOID(oid), err) } for key, value := range action.Header { request.Header.Set(key, value) @@ -257,21 +300,21 @@ func downloadObject( response, err := client.Do(request) if err != nil { - return fmt.Errorf("LFS object %s: %w", shortOID(oid), err) + return false, fmt.Errorf("LFS object %s: %w", shortOID(oid), err) } defer drainAndClose(response) if response.StatusCode != http.StatusOK { - return fmt.Errorf("LFS object %s download failed with status %d", shortOID(oid), response.StatusCode) + return false, fmt.Errorf("LFS object %s download failed with status %d", shortOID(oid), response.StatusCode) } if err := os.MkdirAll(filepath.Dir(destination), 0o755); err != nil { - return fmt.Errorf("create LFS cache directory: %w", err) + return false, fmt.Errorf("create LFS cache directory: %w", err) } temp, err := os.CreateTemp(filepath.Dir(destination), ".lfs-download-*") if err != nil { - return fmt.Errorf("create LFS temp file: %w", err) + return false, fmt.Errorf("create LFS temp file: %w", err) } tempName := temp.Name() @@ -280,24 +323,24 @@ func downloadObject( closeErr := temp.Close() if copyErr != nil { _ = os.Remove(tempName) - return fmt.Errorf("LFS object %s: %w", shortOID(oid), copyErr) + return false, fmt.Errorf("LFS object %s: %w", shortOID(oid), copyErr) } if closeErr != nil { _ = os.Remove(tempName) - return fmt.Errorf("LFS object %s: %w", shortOID(oid), closeErr) + return false, fmt.Errorf("LFS object %s: %w", shortOID(oid), closeErr) } if actual := hex.EncodeToString(hash.Sum(nil)); actual != oid { _ = os.Remove(tempName) - return fmt.Errorf("LFS object %s content hash mismatch", shortOID(oid)) + return false, fmt.Errorf("LFS object %s content hash mismatch", shortOID(oid)) } if err := os.Rename(tempName, destination); err != nil { _ = os.Remove(tempName) - return fmt.Errorf("store LFS object %s: %w", shortOID(oid), err) + return false, fmt.Errorf("store LFS object %s: %w", shortOID(oid), err) } _ = size - return nil + return false, nil } func toBatchObjects(pointers []pointer) []batchObject { diff --git a/internal/lfs/endpoint.go b/internal/lfs/endpoint.go new file mode 100644 index 0000000..2d65702 --- /dev/null +++ b/internal/lfs/endpoint.go @@ -0,0 +1,184 @@ +package lfs + +import ( + "context" + "errors" + "fmt" + "log/slog" + "net/url" + "strings" + + "github.com/go-git/go-git/v5" + "github.com/neurekadev/git-backup/internal/paths" +) + +// endpointCandidates lists, in the order they should be asked, the API roots a +// repository's LFS objects may live under: an lfs.url override from the +// repository's committed .lfsconfig when present, otherwise the remote URL's +// standard /info/lfs root with and without the repository's .git suffix. +// +// Which of the derived roots answers is a property of the host, and no rule +// decides it: GitHub and GitLab route only the suffixed path to their LFS +// service and answer a suffix-less /info/lfs with 422, while a host may mount +// the service exactly where its remote points. The distinction is therefore +// settled by asking, once, before any object is fetched — see selectEndpoint. +// +// The batch request authenticates with the remote's credential, so an override +// is honored only when it is an absolute http(s) URL on the remote's own host +// and scheme: a hostile .lfsconfig must not redirect that credential elsewhere +// or downgrade it to plaintext. Reading .lfsconfig is best-effort — any failure +// falls back to the derived candidates, matching the common deployment. +func endpointCandidates(repository *git.Repository, parsed *url.URL) ([]string, error) { + config, err := readLFSConfig(repository) + if err == nil && config != nil { + if override := strings.TrimSpace(config.Raw.Section("lfs").Option("url")); override != "" { + overridden, ok := paths.ParseHTTPURL(override) + if !ok { + return nil, fmt.Errorf("unsupported lfs.url '%s': only absolute http and https URLs are allowed", redactedURL(override)) + } + if !strings.EqualFold(overridden.Scheme, parsed.Scheme) || canonicalHost(overridden) != canonicalHost(parsed) { + return nil, fmt.Errorf("refusing lfs.url '%s': the override must stay on the remote host and scheme so the remote credential is not sent elsewhere", redactedURL(override)) + } + // Drop a redundant scheme-default port so the endpoint is canonical: + // object-download host comparisons and logs then match hrefs rendered + // without the explicit port. + if overridden.Port() != "" && isSchemeDefaultPort(overridden) { + overridden.Host = strings.TrimSuffix(overridden.Host, ":"+overridden.Port()) + } + return []string{strings.TrimSuffix(overridden.String(), "/")}, nil + } + } + + // A remote that already carries the repository suffix, or has no path at + // all, reaches the API at one path, so there is nothing to choose between. + suffixed, plain := suffixedEndpoint(parsed), plainEndpoint(parsed) + if suffixed == plain { + return []string{suffixed}, nil + } + return []string{suffixed, plain}, nil +} + +// selectEndpoint answers which candidate serves this repository's LFS API, by +// asking each one for a single object before anything is downloaded. +// +// Asking first is what keeps the fetch itself simple. A probe separates the +// answers a host can give — the API answered for the object, LFS is switched +// off, or nothing is mounted at that path — so the fetch then runs against one +// known-good endpoint with no fallback to reconcile, and no working endpoint can +// be mistaken for a missing one halfway through a repository. +func (f *Fetcher) selectEndpoint( + ctx context.Context, + candidates []string, + username, password string, + first pointer, +) (string, error) { + var hardErr error + for _, candidate := range candidates { + switch err := f.client.probe(ctx, candidate, username, password, first); { + case err == nil: + slog.Debug("Git LFS API answered.", "endpoint", redactedURL(candidate)) + return candidate, nil + case errors.Is(err, ErrDisabled): + // The service answered that LFS is off for this repository, which no + // other candidate can outrank: a different path is not a different + // repository. + return "", err + case errors.Is(err, ErrNoEndpoint): + slog.Debug("No Git LFS API is mounted at this endpoint.", "endpoint", redactedURL(candidate)) + default: + // The candidate that failed may be the derived guess rather than the + // path the remote configures, so the remaining candidates still get + // their turn before this is reported. + if hardErr == nil { + hardErr = err + } + } + } + if hardErr != nil { + return "", hardErr + } + // No candidate serves the API. The mirror layer records that as LFS being + // switched off rather than failing the repository, which is the closest + // truthful reading available from a client. + return "", ErrDisabled +} + +// suffixedEndpoint derives the remote's standard LFS API root, including the +// repository's .git suffix. +// +// The parsed URL is copied and only its path rewritten, from its escaped form so +// percent-encoding survives: a remote whose path holds a reserved byte — an +// encoded slash in a repository name, say — must keep requesting that exact +// path. Everything else the configured remote carries, userinfo for a deployment +// that embeds credentials and a non-default port among it, still reaches the +// endpoint. A remote with no path has no repository to suffix, but the API root +// is still /info/lfs on that host. +func suffixedEndpoint(remoteURL *url.URL) string { + endpoint := endpointBase(remoteURL) + + escaped := strings.TrimSuffix(remoteURL.EscapedPath(), "/") + if escaped == "" { + return withPath(endpoint, "/info/lfs") + } + // The suffix is judged on the decoded path but appended to the escaped one, + // so a remote whose escaping hides it (re%2Egit) is not given a second + // suffix it did not need. It is matched case-insensitively, as the mirror's + // other .git handling does: a remote already ending in ".GIT" must not gain + // a differently cased one that no forge serves. + if !hasGitSuffix(strings.TrimSuffix(remoteURL.Path, "/")) { + escaped += ".git" + } + return withPath(endpoint, escaped+"/info/lfs") +} + +// plainEndpoint derives the LFS API root at the remote's configured path, +// without adding the repository suffix. +func plainEndpoint(remoteURL *url.URL) string { + endpoint := endpointBase(remoteURL) + return withPath(endpoint, strings.TrimSuffix(remoteURL.EscapedPath(), "/")+"/info/lfs") +} + +// endpointBase copies a remote URL with the parts an API root cannot carry +// removed: a query, a forced empty query, and a fragment would otherwise sit +// between the endpoint and the action path appended to it. +func endpointBase(remoteURL *url.URL) url.URL { + endpoint := *remoteURL + endpoint.RawQuery = "" + endpoint.ForceQuery = false + endpoint.Fragment = "" + endpoint.RawFragment = "" + return endpoint +} + +// withPath points an endpoint at an escaped path, keeping Path decoded so +// String() does not escape the escaping a second time. +func withPath(endpoint url.URL, escaped string) string { + endpoint.RawPath = escaped + endpoint.Path, _ = url.PathUnescape(escaped) + return endpoint.String() +} + +// hasGitSuffix reports whether a remote path already ends in the repository +// suffix, in any case. +func hasGitSuffix(path string) bool { + return len(path) >= len(".git") && strings.EqualFold(path[len(path)-len(".git"):], ".git") +} + +// isSchemeDefaultPort reports whether the URL's explicit port equals its +// scheme's default (http 80, https 443). +func isSchemeDefaultPort(u *url.URL) bool { + return (strings.EqualFold(u.Scheme, "http") && u.Port() == "80") || + (strings.EqualFold(u.Scheme, "https") && u.Port() == "443") +} + +// canonicalHost renders the URL's host lowercased with any scheme-default +// port removed, so an explicit https://host:443 compares equal to +// https://host while a redirect to any other port stays distinct. +func canonicalHost(u *url.URL) string { + host := strings.ToLower(u.Hostname()) + port := u.Port() + if port == "" || isSchemeDefaultPort(u) { + return host + } + return host + ":" + port +} diff --git a/internal/lfs/lfs.go b/internal/lfs/lfs.go index 429e88c..82940ac 100644 --- a/internal/lfs/lfs.go +++ b/internal/lfs/lfs.go @@ -46,102 +46,89 @@ func NewFetcher() *Fetcher { // repositoryPath, using remoteURL to derive the LFS endpoint. username and // password authenticate the batch request only; object downloads use the // server-provided (typically pre-signed) URLs. +// +// The work splits into three steps with one job each: reject a remote URL this +// package will not talk to, scan the mirror for pointers, settle which endpoint +// serves this repository's LFS API, and then fetch from that endpoint alone. +// Settling the endpoint first is what keeps the fetch simple — no fallback to +// reconcile, and no way for a working endpoint to be mistaken for a missing one +// partway through a repository. func (f *Fetcher) FetchAll(ctx context.Context, repositoryPath, remoteURL, username, password string) error { repository, err := git.PlainOpen(repositoryPath) if err != nil { return fmt.Errorf("open repository: %w", err) } + parsed, err := parseRemoteURL(remoteURL) + if err != nil { + return err + } + + pointers, err := f.pointers(ctx, repository, remoteURL) + if err != nil || len(pointers) == 0 { + return err + } + + candidates, err := endpointCandidates(repository, parsed) + if err != nil { + return err + } + endpoint, err := f.selectEndpoint(ctx, candidates, username, password, pointers[0]) + if err != nil { + return err + } - endpoints, err := resolveEndpoints(repository, remoteURL) + store := objectStoreDir(repository, repositoryPath) + outcome, err := f.fetchBatches(ctx, store, endpoint, username, password, pointers) if err != nil { return err } + if outcome.unavailable > 0 { + // The endpoint serves this repository but does not have every object a + // pointer names. The mirror keeps what it got; the gap is reported + // rather than passed over, because a mirror missing LFS content is not a + // complete backup. + slog.Warn("Some Git LFS objects could not be fetched; the repository was mirrored without them.", + "endpoint", redactedURL(endpoint), "objectsMissing", outcome.unavailable, "objectsRequested", len(pointers)) + slog.Debug("Git LFS objects that could not be fetched.", + "endpoint", redactedURL(endpoint), "reason", outcome.skipped.Error()) + return fmt.Errorf("%d of %d LFS objects could not be fetched: %w", + outcome.unavailable, len(pointers), outcome.skipped) + } + return nil +} +// parseRemoteURL rejects a remote this package will not talk to: only http and +// https are accepted, so a provider-supplied URL can never reach a transport +// helper or a local path. +func parseRemoteURL(remoteURL string) (*url.URL, error) { + parsed, ok := paths.ParseHTTPURL(remoteURL) + if !ok { + return nil, fmt.Errorf("unsupported remote URL '%s': only http and https are allowed", redactedURL(remoteURL)) + } + return parsed, nil +} + +// pointers scans the mirror for the LFS pointers its refs reach, reporting the +// parts of the repository it could not read. A scan that could not read +// everything fails the fetch: a backup reported as complete while content it +// could not scan is silently absent is worse than one that reports what it could +// not do. +func (f *Fetcher) pointers(ctx context.Context, repository *git.Repository, remoteURL string) ([]pointer, error) { scan, err := collectPointers(ctx, repository) if err != nil { - return fmt.Errorf("scan for LFS pointers: %w", err) + return nil, fmt.Errorf("scan for LFS pointers: %w", err) } if len(scan.skipped) > 0 { - // Whatever those objects held is missing from the mirror, so name them - // and fail the fetch: a backup reported as complete while LFS content is - // silently absent is worse than one that reports what it could not do. for _, skipped := range scan.skipped { slog.Warn("Could not read part of the repository while scanning for Git LFS pointers.", "repository", redactedURL(remoteURL), "entry", skipped.name, "oid", shortOID(skipped.hash.String()), "reason", skipped.err.Error()) } - return fmt.Errorf("%d entries could not be read while scanning for LFS pointers, so the mirror would be incomplete", len(scan.skipped)) - } - if len(scan.pointers) == 0 { - // Nothing to fetch; never contact the endpoint so a forge without LFS - // is not mistaken for one that disabled it. - return nil - } - pointers := scan.pointers - - store := objectStoreDir(repository, repositoryPath) - var disabledErr, rejectedErr, hardErr error - for _, endpoint := range endpoints { - served, endpointErr := f.fetchBatches(ctx, store, endpoint, username, password, pointers) - if endpointErr == nil { - return nil - } - if served > 0 && !errors.Is(endpointErr, errObjectUnavailable) { - // An endpoint that already mirrored part of the repository is - // authoritative whatever it says afterwards: no later verdict — - // "LFS is off", "nothing here", a rejection — may be recorded as a - // clean skip, because that would report a half-finished mirror as - // complete. The outcome is rendered rather than wrapped, so no skip - // sentinel survives to be matched through the chain. - if hardErr == nil { - hardErr = fmt.Errorf("%d LFS objects were fetched before the endpoint stopped serving the rest: %v", served, endpointErr) - } - slog.Debug("Endpoint stopped serving partway through the repository.", - "endpoint", redactedURL(endpoint), "objectsServed", served, "detail", endpointErr.Error()) - continue - } - switch { - case errors.Is(endpointErr, ErrDisabled): - // A repository with LFS switched off is an expected skip, and the - // endpoint that said so is the one serving the API. - if disabledErr == nil { - disabledErr = endpointErr - } - case errors.Is(endpointErr, ErrNoEndpoint): - // Nothing is mounted at this path; the next candidate may serve it. - case errors.Is(endpointErr, errBatchRejected): - if rejectedErr == nil { - rejectedErr = endpointErr - } - default: - // The candidate that failed may be the derived guess rather than the - // path the remote configures, so the remaining candidates still get - // their turn before this is reported. - if hardErr == nil { - hardErr = endpointErr - } - } - slog.Debug("No Git LFS service answered at this endpoint.", "endpoint", redactedURL(endpoint)) - } - - switch { - case hardErr != nil: - // A real failure on any candidate outranks another's "LFS is off" - // answer, which the mirror layer records as a successful skip: masking - // the failure would report an incomplete mirror as complete. - return hardErr - case disabledErr != nil: - // A candidate that answered "LFS is off" identified the API root, so its - // verdict is authoritative in a way another candidate's request-shape - // rejection is not. - return disabledErr - case rejectedErr != nil: - return rejectedErr - default: - // No candidate served the API, which the mirror layer records as LFS - // being switched off rather than failing the repository. - return ErrDisabled + return nil, fmt.Errorf("%d entries could not be read while scanning for LFS pointers, so the mirror would be incomplete", len(scan.skipped)) } + // Nothing to fetch means the endpoint is never contacted, so a forge without + // LFS is not mistaken for one that disabled it. + return scan.pointers, nil } // batchObjectLimit is how many pointers one batch request submits. Git LFS @@ -179,31 +166,21 @@ var ErrNoEndpoint = errors.New("no git lfs service answered at the endpoint") // endpoint or merely a chunk of stale pointers. Once a second chunk fails, the // fetch stops and reports rather than repeating the failure across a large // repository. -// -// The count of objects the endpoint served comes back with the error, because a -// caller has to know whether an endpoint that now answers "no service" or "LFS -// off" already mirrored part of the repository: a verdict that would mean a -// clean skip cannot also mean a half-finished mirror. func (f *Fetcher) fetchBatches( ctx context.Context, store, endpoint, username, password string, pointers []pointer, -) (int, error) { - var unavailable, rejected, served int - var firstFailure, firstErr error +) (chunkOutcome, error) { + var total chunkOutcome + var firstErr error for start := 0; start < len(pointers); start += batchObjectLimit { end := min(start+batchObjectLimit, len(pointers)) if err := ctx.Err(); err != nil { - return served, err + return total, err } outcome, err := f.fetchChunk(ctx, store, endpoint, username, password, pointers[start:end]) - unavailable += outcome.unavailable - rejected += outcome.rejected - served += outcome.served - if outcome.skipped != nil && firstFailure == nil { - firstFailure = outcome.skipped - } + total.absorb(outcome) if err == nil { continue } @@ -220,33 +197,23 @@ func (f *Fetcher) fetchBatches( firstErr = err } - skipped := unavailable + rejected - if skipped > 0 { - // The mirrored repository stays usable, but its LFS content is not - // complete, so say so once per repository at warn level and keep the - // per-object reasons for debug output. - slog.Warn("Some Git LFS objects could not be fetched; the repository was mirrored without them.", - "endpoint", redactedURL(endpoint), "objectsMissing", skipped, "objectsRequested", len(pointers)) - slog.Debug("Git LFS objects that could not be fetched.", - "endpoint", redactedURL(endpoint), "reason", firstFailure.Error()) - } if firstErr != nil { - return served, firstErr + return total, firstErr } - if skipped > 0 { - return served, fmt.Errorf("%d of %d LFS objects could not be fetched: %w", skipped, len(pointers), firstFailure) + if missing := total.unavailable + total.rejected; missing > 0 { + return total, fmt.Errorf("%d of %d LFS objects could not be fetched: %w", missing, len(pointers), total.skipped) } - return served, nil + return total, nil } // chunkOutcome reports what one batch request could not fetch — objects the // endpoint answered for but will not serve, and objects whose own batch request -// the endpoint rejected outright — and how many it did serve. skipped, when -// set, names the first object that could not be fetched. +// the endpoint rejected outright — and whether it answered for any of them. +// skipped, when set, names the first object that could not be fetched. type chunkOutcome struct { unavailable int rejected int - served int + served bool skipped error } @@ -336,7 +303,7 @@ func isRefusal(err error) bool { // errBatchRejected — which callers report and stop the sweep after, since only a // second such chunk tells stale pointers apart from a broken endpoint. func refuseOrReport(outcome chunkOutcome, rejection error) (chunkOutcome, error) { - if outcome.served > 0 || outcome.unavailable > 0 { + if outcome.served || outcome.unavailable > 0 { return outcome, nil } return outcome, fmt.Errorf("%w: %w", errAllRejected, rejection) @@ -345,20 +312,21 @@ func refuseOrReport(outcome chunkOutcome, rejection error) (chunkOutcome, error) // downloadChunk streams every object the batch scheduled. Objects the endpoint // will not serve are recorded and skipped so their siblings still download, and // so are objects this client could not transfer — attempted, but unreachable — -// which come back as an error once every object has had its turn. +// which come back as an error once every object has had its turn. A `served` +// chunk is one whose bytes actually arrived here, which is what tells a rejection +// that arrived beside real content from one that stands alone. func (f *Fetcher) downloadChunk( ctx context.Context, store, endpoint, username, password string, objects []batchResponseObject, ) (chunkOutcome, error) { - unavailable, failed := downloadObjects(ctx, f.client, store, endpoint, username, password, objects) + cached, unavailable, failed := downloadObjects(ctx, f.client, store, endpoint, username, password, objects) var outcome chunkOutcome for _, object := range unavailable { outcome.recordUnavailable(fmt.Errorf("%w: %s", errObjectUnavailable, object.Error())) } - // Whatever the endpoint did not refuse, it answered for one way or another. - outcome.served = len(objects) - len(unavailable) + outcome.served = len(objects) > len(unavailable)+len(cached) if len(failed) > 0 { return outcome, fmt.Errorf("%d of %d LFS objects could not be downloaded: %w", len(failed), len(objects), failed[0]) } @@ -378,147 +346,12 @@ func (o *chunkOutcome) recordUnavailable(reason error) { func (o *chunkOutcome) absorb(half chunkOutcome) { o.unavailable += half.unavailable o.rejected += half.rejected - o.served += half.served + o.served = o.served || half.served if half.skipped != nil && o.skipped == nil { o.skipped = half.skipped } } -// resolveEndpoints determines the LFS API roots to try, in order: an lfs.url -// override from the repository's committed .lfsconfig when present, otherwise -// the remote URL's standard /info/lfs root with and without the repository's -// .git suffix. -// -// The batch request authenticates with the remote's credential, so an override -// is honored only when it is an absolute http(s) URL on the remote's own host -// and scheme — a hostile .lfsconfig must not redirect that credential elsewhere -// or downgrade it to plaintext. Reading .lfsconfig is best-effort — any failure -// falls back to the derived endpoints, matching the common deployment. -func resolveEndpoints(repository *git.Repository, remoteURL string) ([]string, error) { - parsed, ok := paths.ParseHTTPURL(remoteURL) - if !ok { - return nil, fmt.Errorf("unsupported remote URL '%s': only http and https are allowed", redactedURL(remoteURL)) - } - - config, err := readLFSConfig(repository) - if err == nil && config != nil { - if override := strings.TrimSpace(config.Raw.Section("lfs").Option("url")); override != "" { - overridden, ok := paths.ParseHTTPURL(override) - if !ok { - return nil, fmt.Errorf("unsupported lfs.url '%s': only absolute http and https URLs are allowed", redactedURL(override)) - } - if !strings.EqualFold(overridden.Scheme, parsed.Scheme) || canonicalHost(overridden) != canonicalHost(parsed) { - return nil, fmt.Errorf("refusing lfs.url '%s': the override must stay on the remote host and scheme so the remote credential is not sent elsewhere", redactedURL(override)) - } - // Drop a redundant scheme-default port so the endpoint is canonical: - // object-download host comparisons and logs then match hrefs rendered - // without the explicit port. - if overridden.Port() != "" && isSchemeDefaultPort(overridden) { - overridden.Host = strings.TrimSuffix(overridden.Host, ":"+overridden.Port()) - } - return []string{strings.TrimSuffix(overridden.String(), "/")}, nil - } - } - // A remote that already carries the repository suffix, or has no path at - // all, reaches the API at one path, so there is nothing to fall back to: - // probing the same URL twice would only repeat every request. - suffixed, plain := defaultEndpoint(parsed), plainEndpoint(parsed) - if suffixed == plain { - return []string{suffixed}, nil - } - return []string{suffixed, plain}, nil -} - -// defaultEndpoint derives the remote's standard LFS API root, including the -// repository's .git suffix. -// -// Git LFS clients request "[/info/lfs]" with the suffix their remote -// uses, and forges route only the suffixed path to their LFS service: GitHub -// and GitLab answer a suffix-less /info/lfs with 422, while Forgejo and Gitea -// accept either form. The mirror's remote has no .git suffix because a config -// URL rarely carries one, so the suffix is added here whenever it is absent; -// plainEndpoint is the fallback for a service that routes the suffix-less path. -// -// The parsed URL is copied and only its path rewritten, from its escaped form so -// percent-encoding survives: a remote whose path holds a reserved byte — an -// encoded slash in a repository name, say — must keep requesting that exact -// path. Everything else the configured remote carries, userinfo for a deployment -// that embeds credentials and a non-default port among it, still reaches the -// endpoint. A remote with no path has no repository to suffix, but the API root -// is still /info/lfs on that host. -func defaultEndpoint(remoteURL *url.URL) string { - endpoint := endpointBase(remoteURL) - - escaped := strings.TrimSuffix(remoteURL.EscapedPath(), "/") - if escaped == "" { - // No repository segment to suffix, but the API root is still /info/lfs - // on that host. - return withPath(endpoint, "/info/lfs") - } - // The suffix is judged on the decoded path but appended to the escaped one, - // so a remote whose escaping hides it (re%2Egit) is not given a second - // suffix it did not need. It is matched case-insensitively, as the mirror's - // other .git handling does: a remote already ending in ".GIT" must not gain - // a differently cased one that no forge serves. - if !hasGitSuffix(strings.TrimSuffix(remoteURL.Path, "/")) { - escaped += ".git" - } - return withPath(endpoint, escaped+"/info/lfs") -} - -// plainEndpoint derives the LFS API root at the remote's configured path, -// without adding the repository suffix: the fallback for a service that routes -// /info/lfs exactly where the remote points. -func plainEndpoint(remoteURL *url.URL) string { - endpoint := endpointBase(remoteURL) - return withPath(endpoint, strings.TrimSuffix(remoteURL.EscapedPath(), "/")+"/info/lfs") -} - -// endpointBase copies a remote URL with the parts an API root cannot carry -// removed: a query, a forced empty query, and a fragment would otherwise sit -// between the endpoint and the action path appended to it. -func endpointBase(remoteURL *url.URL) url.URL { - endpoint := *remoteURL - endpoint.RawQuery = "" - endpoint.ForceQuery = false - endpoint.Fragment = "" - endpoint.RawFragment = "" - return endpoint -} - -// withPath points an endpoint at an escaped path, keeping Path decoded so -// String() does not escape the escaping a second time. -func withPath(endpoint url.URL, escaped string) string { - endpoint.RawPath = escaped - endpoint.Path, _ = url.PathUnescape(escaped) - return endpoint.String() -} - -// hasGitSuffix reports whether a remote path already ends in the repository -// suffix, in any case. -func hasGitSuffix(path string) bool { - return len(path) >= len(".git") && strings.EqualFold(path[len(path)-len(".git"):], ".git") -} - -// isSchemeDefaultPort reports whether the URL's explicit port equals its -// scheme's default (http 80, https 443). -func isSchemeDefaultPort(u *url.URL) bool { - return (strings.EqualFold(u.Scheme, "http") && u.Port() == "80") || - (strings.EqualFold(u.Scheme, "https") && u.Port() == "443") -} - -// canonicalHost renders the URL's host lowercased with any scheme-default -// port removed, so an explicit https://host:443 compares equal to -// https://host while a redirect to any other port stays distinct. -func canonicalHost(u *url.URL) string { - host := strings.ToLower(u.Hostname()) - port := u.Port() - if port == "" || isSchemeDefaultPort(u) { - return host - } - return host + ":" + port -} - // redactedURL renders a URL with any embedded password masked, for safe // inclusion in error messages. func redactedURL(rawURL string) string { diff --git a/internal/lfs/lfs_test.go b/internal/lfs/lfs_test.go index 3f27320..e528621 100644 --- a/internal/lfs/lfs_test.go +++ b/internal/lfs/lfs_test.go @@ -93,6 +93,15 @@ type fakeLFSServer struct { // disabledPaths answer 403, the response for a repository with LFS switched // off, which is not the same as a path with no service behind it. disabledPaths []string + // serveProbe answers discovery probes normally, so a test can have discovery + // succeed while the batch requests that follow are rejected. + serveProbe bool + // fetchStatus, when non-zero, is the status batch requests get once a probe + // has been answered, so a test can model an endpoint that serves discovery + // and then stops serving the fetch. + fetchStatus int + // probeCalls counts the discovery probes the handler answered. + probeCalls int } func newFakeLFSServer(t *testing.T, data map[string][]byte) *fakeLFSServer { @@ -169,15 +178,9 @@ func (s *fakeLFSServer) handleBatch(w http.ResponseWriter, r *http.Request) { s.mu.Lock() s.batchCalls++ status, batchPath := s.batchStatus, s.batchPath - rejectWhen := s.rejectWhen - refused := make(map[string]struct{}, len(s.refusedOIDs)) - for _, oid := range s.refusedOIDs { - refused[oid] = struct{}{} - } - actionless := make(map[string]struct{}, len(s.actionlessOIDs)) - for _, oid := range s.actionlessOIDs { - actionless[oid] = struct{}{} - } + rejectWhen, serveProbe := s.rejectWhen, s.serveProbe + probed := s.probeCalls > 0 + fetchStatus := s.fetchStatus disabled := slices.Contains(s.disabledPaths, r.URL.Path) servedHere := slices.Contains(s.servePaths, r.URL.Path) hasServePaths := len(s.servePaths) > 0 @@ -201,10 +204,6 @@ func (s *fakeLFSServer) handleBatch(w http.ResponseWriter, r *http.Request) { _, _ = w.Write([]byte(`{"message":"Git LFS is disabled for this repository."}`)) return } - if status != 0 { - w.WriteHeader(status) - return - } var request batchRequest if err := json.NewDecoder(r.Body).Decode(&request); err != nil { @@ -212,6 +211,25 @@ func (s *fakeLFSServer) handleBatch(w http.ResponseWriter, r *http.Request) { return } + // A discovery probe carries one object. A server configured to answer it + // does so before any rule about the fetch applies, so a test can model + // discovery succeeding and the fetch that follows failing. + if len(request.Objects) == 1 && serveProbe && !probed { + s.mu.Lock() + s.probeCalls++ + s.mu.Unlock() + s.respond(w, request) + return + } + if status != 0 { + w.WriteHeader(status) + return + } + if fetchStatus != 0 && probed { + w.WriteHeader(fetchStatus) + return + } + // Mimic a forge that rejects some batch requests, such as one carrying more // objects than its limit allows. if rejectWhen != nil { @@ -220,6 +238,21 @@ func (s *fakeLFSServer) handleBatch(w http.ResponseWriter, r *http.Request) { return } } + s.respond(w, request) +} + +// respond answers a batch request with the object verdicts the server holds. +func (s *fakeLFSServer) respond(w http.ResponseWriter, request batchRequest) { + s.mu.Lock() + refused := make(map[string]struct{}, len(s.refusedOIDs)) + for _, oid := range s.refusedOIDs { + refused[oid] = struct{}{} + } + actionless := make(map[string]struct{}, len(s.actionlessOIDs)) + for _, oid := range s.actionlessOIDs { + actionless[oid] = struct{}{} + } + s.mu.Unlock() response := batchResponse{Objects: make([]batchResponseObject, 0, len(request.Objects))} for _, object := range request.Objects { @@ -555,9 +588,9 @@ func TestFetchAllFallsBackWhenBatchRejected(t *testing.T) { } return http.StatusOK }, - // The chunk splits into two singletons, both are served, and the - // candidate succeeds — so there is nothing left to probe. - wantBatches: 3, + // Discovery answers the one-object probe, so the fetch runs: the + // chunk is refused once and then narrowed into two served singletons. + wantBatches: 4, wantCached: result(availableOID, refusedOID), }, { @@ -571,10 +604,9 @@ func TestFetchAllFallsBackWhenBatchRejected(t *testing.T) { return http.StatusOK }, // The chunk splits once and the refused half's lone pointer is - // rejected on its own request; the remote already carries the - // suffix, so there is no second candidate path to probe. + // rejected on its own request. wantErr: "unavailable", - wantBatches: 3, + wantBatches: 4, wantCached: result(availableOID), }, { @@ -582,10 +614,10 @@ func TestFetchAllFallsBackWhenBatchRejected(t *testing.T) { request: func(batchRequest) int { return http.StatusBadRequest }, - // Splitting, not per-object fan-out: both halves narrow to the - // rejection before it is reported as the endpoint's. + // Discovery is answered, then every fetch request is refused, so the + // rejection outlives the split and is reported as the endpoint's. wantErr: "rejected", - wantBatches: 3, + wantBatches: 4, }, } @@ -595,6 +627,9 @@ func TestFetchAllFallsBackWhenBatchRejected(t *testing.T) { availableOID: available, refusedOID: refused, }) + // Discovery is answered so the fetch runs; the rejection rules then + // decide what the fetch sees. + lfsServer.serveProbe = true lfsServer.rejectWhen = c.request repositoryPath := newRepoWithLFS(t, map[string]string{ "big.bin": availablePointer, @@ -695,8 +730,10 @@ func TestFetchAllChunksLargePointerSets(t *testing.T) { t.Fatalf("FetchAll failed: %v", err) } - if calls := lfsServer.batchCallCount(); calls != 2 { - t.Errorf("batch calls = %d, want one request per chunk of %d", calls, batchObjectLimit) + // One discovery probe plus one request per chunk of batchObjectLimit. + wantCalls := 1 + (batchObjectLimit+2+batchObjectLimit-1)/batchObjectLimit + if calls := lfsServer.batchCallCount(); calls != wantCalls { + t.Errorf("batch calls = %d, want a probe plus one request per chunk of %d", calls, batchObjectLimit) } if _, err := os.Stat(filepath.Join(repositoryPath, ".git", "lfs", "objects", lastOID[0:2], lastOID[2:4], lastOID)); err != nil { @@ -704,12 +741,12 @@ func TestFetchAllChunksLargePointerSets(t *testing.T) { } } -// TestFetchAllStopsAfterRepeatedEndpointFailure covers an endpoint whose -// failures are not about the request's shape — dropped connections, and the -// credentials and server statuses that mean the same thing. Splitting the chunk -// would only repeat the answer, so the fetch must fail fast rather than narrow, -// while still attempting one more chunk before concluding the endpoint is down. -func TestFetchAllStopsAfterRepeatedEndpointFailure(t *testing.T) { +// TestFetchAllStopsAfterFailedDiscovery covers an endpoint whose failures are not +// about the request's shape — dropped connections, and the credentials and server +// statuses that mean the same thing. Discovery asks once and reports, so the fetch +// never starts rather than narrowing a chunk against an endpoint that is not +// answering. +func TestFetchAllStopsAfterFailedDiscovery(t *testing.T) { files := make(map[string]string, batchObjectLimit*5) for index := range batchObjectLimit * 5 { _, pointerText := pointerFor([]byte(fmt.Sprintf("object %d", index))) @@ -736,8 +773,9 @@ func TestFetchAllStopsAfterRepeatedEndpointFailure(t *testing.T) { if errors.Is(err, errObjectUnavailable) { t.Errorf("err = %v, want a transport failure rather than per-object unavailability", err) } - if got := atomic.LoadInt64(&requests); got != 2 { - t.Errorf("batch requests = %d, want one attempt per tried chunk and no splitting", got) + // The remote carries its suffix, so there is one candidate to ask. + if got := atomic.LoadInt64(&requests); got != 1 { + t.Errorf("batch requests = %d, want a single discovery attempt", got) } }) @@ -753,19 +791,20 @@ func TestFetchAllStopsAfterRepeatedEndpointFailure(t *testing.T) { if errors.Is(err, errObjectUnavailable) { t.Errorf("err = %v, want the endpoint failure rather than per-object unavailability", err) } - // No halving: one attempt for the first chunk, one for the second. - if calls := lfsServer.batchCallCount(); calls != 2 { - t.Errorf("batch calls = %d, want no splitting for a status about the endpoint", calls) + // No halving, no chunks: discovery answered for the whole repository. + if calls := lfsServer.batchCallCount(); calls != 1 { + t.Errorf("batch calls = %d, want a single discovery attempt", calls) } }) } } -// TestFetchAllReportsSystematicRejection covers an endpoint that rejects every -// request as unprocessable, including single-object ones, so the rejection -// outlives every split. It must be reported as the endpoint's — not attributed -// to each object as unavailable — and the sweep must stop once a second chunk -// confirms it rather than narrowing its way through the whole repository. +// TestFetchAllReportsSystematicRejection covers an endpoint that answers +// discovery and then rejects every batch as unprocessable, including +// single-object ones, so the rejection outlives every split. It must be reported +// as the endpoint's — not attributed to each object as unavailable — and the +// sweep must stop once a second chunk confirms it rather than narrowing its way +// through the whole repository. func TestFetchAllReportsSystematicRejection(t *testing.T) { const pointers = batchObjectLimit * 5 files := make(map[string]string, pointers) @@ -775,7 +814,9 @@ func TestFetchAllReportsSystematicRejection(t *testing.T) { } lfsServer := newFakeLFSServer(t, nil) + // Discovery is answered, then every fetch request is refused. lfsServer.batchStatus = http.StatusUnprocessableEntity + lfsServer.serveProbe = true repositoryPath := newRepoWithLFS(t, files) err := NewFetcher().FetchAll(context.Background(), repositoryPath, lfsServer.remoteURL(), "", "") @@ -791,8 +832,7 @@ func TestFetchAllReportsSystematicRejection(t *testing.T) { // Two chunks are narrowed — the first to establish the rejection and the // second to confirm it is the endpoint's — and the rest are left alone. A - // per-pointer sweep of this repository would take five hundred requests; the - // remote carries its suffix, so only one candidate path is tried. + // per-pointer sweep of this repository would take five hundred requests. if calls := lfsServer.batchCallCount(); calls > 450 { t.Errorf("batch calls = %d, want the systematic rejection to stop the sweep", calls) } @@ -811,8 +851,9 @@ func TestFetchAllNarrowsBothHalves(t *testing.T) { badOID: bad, goodOID: good, }) - // Every request naming the bad object is refused, so the chunk narrows down - // its first half while the second half is served normally. + // Discovery is answered, so the fetch runs; every request naming the bad + // object is then refused, narrowing the chunk onto it. + lfsServer.serveProbe = true lfsServer.rejectWhen = func(request batchRequest) int { for _, object := range request.Objects { if object.OID == badOID { @@ -856,6 +897,7 @@ func TestFetchAllKeepsObjectsBesideAnAnsweredRejection(t *testing.T) { refusedOID: refused, }) lfsServer.refusedOIDs = []string{phantomOID} + lfsServer.serveProbe = true lfsServer.rejectWhen = func(request batchRequest) int { for _, object := range request.Objects { if object.OID == refusedOID { @@ -904,6 +946,7 @@ func TestFetchAllKeepsSweepingAfterARefusedChunk(t *testing.T) { } lfsServer := newFakeLFSServer(t, data) + lfsServer.serveProbe = true lfsServer.rejectWhen = func(request batchRequest) int { for _, object := range request.Objects { for _, stale := range staleOIDs { @@ -980,16 +1023,10 @@ func TestFetchAllKeepsObjectsBesideARefusedHalf(t *testing.T) { } } -// TestCollectPointersHandlesWindowsIllegalNames covers a tree entry that cannot -// be materialised on the host — a backslash in a name is a path separator on -// Windows — and one that reuses a subtree hash, as a submodule-like entry does. -// Both must be walked without tripping the scan: the pointer beside them is -// still collected, and the shared tree is visited once. -// TestFetchAllReportsAnEndpointThatStopsPartway covers a service that serves -// the first chunk and then answers as if it were not there any more. Part of the -// repository is mirrored by then, so the "no LFS here" verdict must not be -// recorded as a clean skip: that would report a half-finished mirror as -// complete. +// TestFetchAllReportsAnEndpointThatStopsPartway covers a service that answers +// discovery and then stops serving the fetch. Part of the repository is mirrored +// by then, so the failure must be reported rather than recorded as a clean skip: +// that would present a half-finished mirror as complete. func TestFetchAllReportsAnEndpointThatStopsPartway(t *testing.T) { const pointers = batchObjectLimit * 2 files := make(map[string]string, pointers) @@ -1002,32 +1039,20 @@ func TestFetchAllReportsAnEndpointThatStopsPartway(t *testing.T) { } lfsServer := newFakeLFSServer(t, data) - // The first chunk is served; every request after it looks like a host with - // nothing mounted at the path. - var calls int64 - lfsServer.rejectWhen = func(batchRequest) int { - if atomic.AddInt64(&calls, 1) > 1 { - return http.StatusNotFound - } - return http.StatusOK - } + // Discovery is answered; the fetch that follows looks like a host with + // nothing mounted at the path any more. + lfsServer.serveProbe = true + lfsServer.fetchStatus = http.StatusNotFound repositoryPath := newRepoWithLFS(t, files) err := NewFetcher().FetchAll(context.Background(), repositoryPath, lfsServer.remoteURL(), "", "") if err == nil { - t.Fatal("an endpoint that stops serving the rest should be reported") + t.Fatal("an endpoint that stops serving the fetch should be reported") } - // ErrDisabled is how the mirror layer recognises an expected skip, so the - // partial-mirror error must not match it through the chain. + // ErrDisabled is how the mirror layer recognises an expected skip, so this + // failure must not match it through the chain. if errors.Is(err, ErrDisabled) || errors.Is(err, ErrNoEndpoint) { - t.Fatalf("err = %v, want a failure rather than a clean skip for a partial mirror", err) - } - - // The chunk that was served did reach the mirror. - firstOID, _ := pointerFor([]byte("object 0")) - if _, statErr := os.Stat(filepath.Join(repositoryPath, ".git", "lfs", "objects", - firstOID[0:2], firstOID[2:4], firstOID)); statErr != nil { - t.Errorf("the served chunk should be mirrored: %v", statErr) + t.Fatalf("err = %v, want a failure rather than a clean skip", err) } } @@ -1290,11 +1315,11 @@ func TestCanonicalHostStripsSchemeDefaultPorts(t *testing.T) { } } -// TestDefaultEndpointAddsGitSuffix pins the endpoint a forge expects: GitHub +// TestSuffixedEndpointAddsGitSuffix pins the endpoint a forge expects: GitHub // and GitLab answer a suffix-less /info/lfs with 422, so a configured remote // without .git still has to request the suffixed path. Everything else the // remote URL carries has to survive the rewrite. -func TestDefaultEndpointAddsGitSuffix(t *testing.T) { +func TestSuffixedEndpointAddsGitSuffix(t *testing.T) { cases := []struct { remoteURL string want string @@ -1319,18 +1344,18 @@ func TestDefaultEndpointAddsGitSuffix(t *testing.T) { if err != nil { t.Fatal(err) } - if got := defaultEndpoint(parsed); got != c.want { - t.Errorf("defaultEndpoint(%q) = %q, want %q", redactedURL(c.remoteURL), redactedURL(got), redactedURL(c.want)) + if got := suffixedEndpoint(parsed); got != c.want { + t.Errorf("suffixedEndpoint(%q) = %q, want %q", redactedURL(c.remoteURL), redactedURL(got), redactedURL(c.want)) } }) } } -// TestResolveEndpointsCollapsesDuplicateCandidates covers remotes that reach the +// TestEndpointCandidatesCollapseDuplicates covers remotes that reach the // LFS API at one path whichever candidate is derived — a remote that already // carries the repository suffix, and one with no path at all. Probing the same // URL twice would repeat every request of a whole repository for nothing. -func TestResolveEndpointsCollapsesDuplicateCandidates(t *testing.T) { +func TestEndpointCandidatesCollapseDuplicates(t *testing.T) { for _, remoteURL := range []string{ "https://git.example.com/owner/repo.git", "https://git.example.com", @@ -1342,7 +1367,11 @@ func TestResolveEndpointsCollapsesDuplicateCandidates(t *testing.T) { t.Fatal(err) } - endpoints, err := resolveEndpoints(repository, remoteURL) + parsed, parseErr := parseRemoteURL(remoteURL) + if parseErr != nil { + t.Fatal(parseErr) + } + endpoints, err := endpointCandidates(repository, parsed) if err != nil { t.Fatalf("resolveEndpoints failed: %v", err) } @@ -1353,7 +1382,7 @@ func TestResolveEndpointsCollapsesDuplicateCandidates(t *testing.T) { } } -func TestResolveEndpointOverrideHostAndScheme(t *testing.T) { +func TestEndpointCandidatesOverrideHostAndScheme(t *testing.T) { oid, pointerText := pointerFor([]byte("content")) cases := []struct { name string @@ -1406,7 +1435,11 @@ func TestResolveEndpointOverrideHostAndScheme(t *testing.T) { t.Fatal(err) } - endpoints, err := resolveEndpoints(repository, c.remoteURL) + parsed, parseErr := parseRemoteURL(c.remoteURL) + if parseErr != nil { + t.Fatal(parseErr) + } + endpoints, err := endpointCandidates(repository, parsed) if c.wantReject { if err == nil { t.Fatalf("resolveEndpoints = %q, want rejection", redactedURL(strings.Join(endpoints, ","))) From c754ff4bcca14e9f6b32357ca7a2726bb58b4d81 Mon Sep 17 00:00:00 2001 From: Neureka Date: Thu, 10 Sep 2026 13:12:55 -0700 Subject: [PATCH 04/15] fix(lfs): correct the endpoint and chunk verdicts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up on the endpoint-discovery refactor: - probe now separates what a refusal means instead of assuming a status: an answer in markup is a host page for a path it does not serve, while a JSON or bodyless answer is the API speaking. Forges do both — GitHub's edge answers 422 with an HTML page for an unmounted path while its LFS API answers 403 with JSON for a repository whose LFS is off — so a wrong path is tried elsewhere while a verdict about the repository is kept. Only 404 is read as an unmounted path on its own. A refusal that is neither is reported rather than downgraded to a clean skip. - selectEndpoint keeps every candidate's answer and prefers a real failure over a disabled verdict, since a candidate that failed describes something beyond a missing path and the verdict would otherwise be recorded as a successful skip. - An lfs.url override is cleared of its own query, forced query, and fragment before it becomes an endpoint: the action path is appended to it, so anything after the path would land in the middle of the request line. - A rejected chunk tries both halves even when the first fails outright, so a failure in one half cannot cost the other its objects. - An object's unavailable report renders the rejection instead of wrapping it, because an error matching both errObjectUnavailable and errBatchRejected reads as one object's unavailability and the endpoint's refusal at once. - A chunk counts objects the endpoint answered for, including those already in the local store, as evidence that it processed the chunk — so a rejection beside them is attributed to one object rather than to the endpoint. - The fake forge answers unmatched paths the way real forges do, so the test that exercises a guessed path being refused is no longer shadowed by a 404. --- internal/lfs/batch.go | 39 ++++++++++++++++++++++++----- internal/lfs/endpoint.go | 54 +++++++++++++++++++++++++--------------- internal/lfs/lfs.go | 25 +++++++++++++------ internal/lfs/lfs_test.go | 29 ++++++++++++++------- 4 files changed, 104 insertions(+), 43 deletions(-) diff --git a/internal/lfs/batch.go b/internal/lfs/batch.go index 62e597d..c6979fd 100644 --- a/internal/lfs/batch.go +++ b/internal/lfs/batch.go @@ -135,18 +135,36 @@ func (c *batchClient) probe(ctx context.Context, endpoint, username, password st // reporting that the server does not have it. return nil case http.StatusForbidden: - // The forge has LFS switched off for this repository. - return ErrDisabled - case http.StatusNotFound, http.StatusUnprocessableEntity: + // Two different things answer 403: a forge saying LFS is switched off + // for this repository, and a host page refusing a path it does not + // serve. The LFS API explains itself — in JSON, and sometimes with no + // body at all, which is still a verdict about the repository — while an + // edge page does not. + return classifyRefusal(response, ErrDisabled) + case http.StatusNotFound: // Nothing is mounted at this path: a host that routes by path answers - // 404, and one that validates the action answers a single-object - // request with 422. + // 404. return ErrNoEndpoint default: - return fmt.Errorf("batch request failed with status %d", response.StatusCode) + // Unprocessable is how an edge refuses a path it does not serve as much + // as it is how the API refuses a request's shape, so the body separates + // them; every other status describes the endpoint and is reported rather + // than read as a wrong path. + return classifyRefusal(response, fmt.Errorf("batch request failed with status %d", response.StatusCode)) } } +// classifyRefusal reads a refusal the API did not phrase as a verdict about one +// object. Markup means a host page answered — the path is not the API — and +// anything else is the API speaking, which the caller reads as verdict. +func classifyRefusal(response *http.Response, verdict error) error { + body, err := io.ReadAll(io.LimitReader(response.Body, 4096)) + if err == nil && looksLikeHTML(body) { + return ErrNoEndpoint + } + return verdict +} + // batch submits every pointer for download scheduling. // // A status describing the request's shape is reported as errBatchRejected so @@ -217,6 +235,15 @@ func describesRequestShape(status int) bool { } } +// looksLikeHTML reports whether a body is a markup document, which is what an +// edge or proxy answers with and what the LFS API never does. A JSON document or +// an empty body is therefore read as the API speaking; markup is read as the path +// being refused by something that is not the API. +func looksLikeHTML(body []byte) bool { + trimmed := bytes.TrimSpace(bytes.ToLower(body)) + return bytes.HasPrefix(trimmed, []byte(" 1 { middle := len(pointers) / 2 outcome, firstErr := f.fetchChunk(ctx, store, endpoint, username, password, pointers[:middle]) if firstErr != nil && !isRefusal(firstErr) { + // A half that failed outright still must not cost its sibling. + rest, _ := f.fetchChunk(ctx, store, endpoint, username, password, pointers[middle:]) + outcome.absorb(rest) return outcome, firstErr } @@ -280,11 +280,15 @@ func (f *Fetcher) fetchChunk( // can be blamed. Whether that is this object's fault or the endpoint's is // decided one level up, by whether the sibling half was served; this subtree // has served nothing either way. + // + // The object's own report renders the rejection rather than wrapping it: an + // error matching both sentinels would read as the endpoint's refusal and as + // one object's unavailability at once, and callers weigh those differently. slog.Debug("Git LFS batch request rejected for a single object.", "endpoint", redactedURL(endpoint), "oid", shortOID(pointers[0].oid), "reason", rejection.Error()) return chunkOutcome{ rejected: 1, - skipped: fmt.Errorf("%w: LFS object %s download request rejected: %w", + skipped: fmt.Errorf("%w: LFS object %s download request rejected: %v", errObjectUnavailable, shortOID(pointers[0].oid), rejection), }, fmt.Errorf("%w: %w", errNarrowedToSingleton, rejection) } @@ -326,7 +330,12 @@ func (f *Fetcher) downloadChunk( for _, object := range unavailable { outcome.recordUnavailable(fmt.Errorf("%w: %s", errObjectUnavailable, object.Error())) } - outcome.served = len(objects) > len(unavailable)+len(cached) + // An object the endpoint answered for — streamed now or already in the store, + // which is what cached counts — is positive evidence that it processed this + // chunk, so a rejection beside one is that object's fault rather than the + // endpoint's. + outcome.served = len(objects) > len(unavailable) + _ = cached if len(failed) > 0 { return outcome, fmt.Errorf("%d of %d LFS objects could not be downloaded: %w", len(failed), len(objects), failed[0]) } diff --git a/internal/lfs/lfs_test.go b/internal/lfs/lfs_test.go index e528621..6447cb9 100644 --- a/internal/lfs/lfs_test.go +++ b/internal/lfs/lfs_test.go @@ -123,6 +123,15 @@ func newFakeLFSServer(t *testing.T, data map[string][]byte) *fakeLFSServer { return s } +// writeUnmounted answers the way a forge's edge does for a repository path it +// does not route to an LFS API: a page, which is what tells a client the path is +// wrong rather than the repository LFS-free. +func writeUnmounted(w http.ResponseWriter) { + w.Header().Set("Content-Type", "text/html; charset=utf-8") + w.WriteHeader(http.StatusUnprocessableEntity) + _, _ = w.Write([]byte("\nOh nounexpected")) +} + // remoteURL is the remote a repository would use to reach the fake server. func (s *fakeLFSServer) remoteURL() string { return s.server.URL + "/repo.git" @@ -186,24 +195,26 @@ func (s *fakeLFSServer) handleBatch(w http.ResponseWriter, r *http.Request) { hasServePaths := len(s.servePaths) > 0 s.mu.Unlock() - // A path the server does not serve answers 404, the response that tells a - // client no LFS service is mounted there. + // A path the server does not serve answers the way the forges this package + // was written against do: a host page for the path it will not route, and + // markup rather than an LFS explanation. The disabled verdict is decided + // first, because a repository with LFS off answers 403 for its own path even + // when the test also mounts the service elsewhere. if batchPath != "" && r.URL.Path != batchPath { - w.WriteHeader(http.StatusNotFound) - return - } - if hasServePaths && !servedHere { - w.WriteHeader(http.StatusNotFound) + writeUnmounted(w) return } if disabled { - // A forge with LFS switched off explains itself in JSON; a host with no - // service behind the path answers with its own HTML page. + // A forge with LFS switched off explains itself in JSON. w.Header().Set("Content-Type", lfsMediaType) w.WriteHeader(http.StatusForbidden) _, _ = w.Write([]byte(`{"message":"Git LFS is disabled for this repository."}`)) return } + if hasServePaths && !servedHere { + writeUnmounted(w) + return + } var request batchRequest if err := json.NewDecoder(r.Body).Decode(&request); err != nil { From 54735564614d30a726c5a59b93de1ca91f8ccb54 Mon Sep 17 00:00:00 2001 From: Neureka Date: Thu, 10 Sep 2026 13:35:03 -0700 Subject: [PATCH 05/15] fix(lfs): keep endpoint and credential failures reported MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up: - Only statuses that can describe the path are read for a host page: a 403's markup means a path the host will not route, and a 422's means the same, while a 5xx, 429, or 401 now stays a reported failure whatever its body. An edge's error page must not be read as "no endpoint here", because the mirror layer records that verdict as a repository with LFS switched off and would present a broken or unauthorized service as a successful mirror with no LFS content. - selectEndpoint prefers a disabled verdict over another candidate's failure again, now that 422 is a path verdict: a service that answered "LFS is off for this repository" identified the API root, and a wrong path answering 422 must not turn that expected skip into a repository-wide failure. - The probe's failure detail is redacted as well as the endpoint it names: the error text repeats the URL the request went to, which can carry userinfo. - A chunk counts objects the endpoint answered for, including those already in the store, as the evidence that it processed the chunk — which is what the rejection rule asks for, since the endpoint answered for those objects either way. --- internal/lfs/batch.go | 17 ++++++++++++----- internal/lfs/endpoint.go | 17 +++++++++++------ internal/lfs/lfs.go | 11 +++++------ 3 files changed, 28 insertions(+), 17 deletions(-) diff --git a/internal/lfs/batch.go b/internal/lfs/batch.go index c6979fd..6781cd8 100644 --- a/internal/lfs/batch.go +++ b/internal/lfs/batch.go @@ -145,12 +145,19 @@ func (c *batchClient) probe(ctx context.Context, endpoint, username, password st // Nothing is mounted at this path: a host that routes by path answers // 404. return ErrNoEndpoint - default: - // Unprocessable is how an edge refuses a path it does not serve as much - // as it is how the API refuses a request's shape, so the body separates - // them; every other status describes the endpoint and is reported rather - // than read as a wrong path. + case http.StatusUnprocessableEntity: + // Unprocessable is how an edge refuses a path it does not route as much + // as it is how the API refuses a request's shape. A single object cannot + // trip an object limit, so a page here is a wrong path and anything else + // is the API refusing the probe. return classifyRefusal(response, fmt.Errorf("batch request failed with status %d", response.StatusCode)) + default: + // Every other status describes the endpoint or the credential rather + // than the path — a rate limit, an authentication failure, the server's + // own error — so it stays a reported failure. Reading an edge's error + // page as "no endpoint here" would let the mirror layer record a broken + // or unauthorized service as a repository with LFS switched off. + return fmt.Errorf("batch request failed with status %d", response.StatusCode) } } diff --git a/internal/lfs/endpoint.go b/internal/lfs/endpoint.go index 1a82a22..64f412a 100644 --- a/internal/lfs/endpoint.go +++ b/internal/lfs/endpoint.go @@ -95,20 +95,25 @@ func (f *Fetcher) selectEndpoint( case errors.Is(err, ErrNoEndpoint): slog.Debug("No Git LFS API is mounted at this endpoint.", "endpoint", redactedURL(candidate)) default: - slog.Debug("Git LFS API did not answer.", "endpoint", redactedURL(candidate), "detail", err.Error()) + // The endpoint and the error both describe URLs, and the error's own + // text repeats the one the request went to, so it is redacted too + // before it reaches the log. + slog.Debug("Git LFS API did not answer.", + "endpoint", redactedURL(candidate), "detail", redactedURL(err.Error())) if failed == nil { failed = err } } } switch { - case failed != nil: - // A candidate that failed describes something wrong beyond a missing - // path, so it outranks a disabled verdict another candidate gave: the - // failure is reported rather than recorded as a successful skip. - return "", failed case disabled != nil: + // A service that answered "LFS is off for this repository" identified + // the API root, and that verdict is the mirror layer's expected skip. A + // wrong path answering differently must not turn the repository into a + // failure. return "", disabled + case failed != nil: + return "", failed default: // No candidate serves the API. The mirror layer records that as LFS // being switched off rather than failing the repository, which is the diff --git a/internal/lfs/lfs.go b/internal/lfs/lfs.go index 7468d4d..e35c048 100644 --- a/internal/lfs/lfs.go +++ b/internal/lfs/lfs.go @@ -330,12 +330,11 @@ func (f *Fetcher) downloadChunk( for _, object := range unavailable { outcome.recordUnavailable(fmt.Errorf("%w: %s", errObjectUnavailable, object.Error())) } - // An object the endpoint answered for — streamed now or already in the store, - // which is what cached counts — is positive evidence that it processed this - // chunk, so a rejection beside one is that object's fault rather than the - // endpoint's. - outcome.served = len(objects) > len(unavailable) - _ = cached + // The endpoint answering for an object is the evidence that it processed + // this chunk, and it answered for these either way: streamed now, or already + // in the store. A chunk it answered for is not a chunk it refused, so a + // rejection beside them is one object's fault rather than the endpoint's. + outcome.served = len(objects) > len(unavailable)+len(cached) if len(failed) > 0 { return outcome, fmt.Errorf("%d of %d LFS objects could not be downloaded: %w", len(failed), len(objects), failed[0]) } From c24421ebc453960ae68687903e56b7313329912f Mon Sep 17 00:00:00 2001 From: Neureka Date: Thu, 10 Sep 2026 13:52:30 -0700 Subject: [PATCH 06/15] fix(lfs): report endpoint failures ahead of a disabled verdict MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up, and the two remaining findings ask for opposite precedence rules, so this settles them on the invariant the PR has been converging on: a verdict the mirror layer records as a successful skip must never outrank a real failure. Preferring "LFS is off" over a candidate that returned 5xx, 401, or a transport error turns a broken or unauthorized service into a complete-looking mirror with no LFS content and no failure raised. A disabled verdict that loses to a wrong path's 422 is merely noisy and retryable, which is the cheaper mistake. Also settles what counts as the endpoint answering for a chunk: an object it answered for — streamed, or reported already in the store — is evidence that it processed the chunk, so a rejection beside those answers belongs to one object rather than to the endpoint. The cached list is no longer threaded out of downloadObjects because the caller only needs the objects the endpoint did not answer for. --- internal/lfs/batch.go | 23 +++++++++-------------- internal/lfs/endpoint.go | 13 +++++++------ internal/lfs/lfs.go | 28 +++++++++++++++------------- 3 files changed, 31 insertions(+), 33 deletions(-) diff --git a/internal/lfs/batch.go b/internal/lfs/batch.go index 6781cd8..2eabd09 100644 --- a/internal/lfs/batch.go +++ b/internal/lfs/batch.go @@ -257,16 +257,17 @@ func looksLikeHTML(body []byte) bool { // // Every object is attempted whatever the others did, so one bad pointer or one // transient transfer failure cannot hide its siblings. What failed comes back in -// the three lists: cached for objects already in the cache, unavailable for -// objects the endpoint will not serve, and failed for objects this client could -// not transfer — a corrupt body, a broken connection — which the caller reports -// rather than recording as the endpoint's refusal. +// two lists: unavailable for objects the endpoint will not serve, and failed for +// objects this client could not transfer — a corrupt body, a broken connection — +// which the caller reports rather than recording as the endpoint's refusal. Both +// lists are about objects the endpoint did not answer for with content; what it +// answered for is what the caller counts as evidence that it processed the chunk. func downloadObjects( ctx context.Context, client *batchClient, store, endpoint, username, password string, objects []batchResponseObject, -) (cached, unavailable []*batchObjectError, failed []error) { +) (unavailable []*batchObjectError, failed []error) { endpointHost := hostOf(endpoint) answered := func(object batchResponseObject) *batchObjectError { switch { @@ -283,23 +284,17 @@ func downloadObjects( for _, object := range objects { if err := ctx.Err(); err != nil { - return cached, unavailable, append(failed, err) + return unavailable, append(failed, err) } if refused := answered(object); refused != nil { unavailable = append(unavailable, refused) continue } - stored, err := downloadObject(ctx, client.client, store, endpointHost, username, password, object.OID, object.Actions["download"]) - switch { - case err != nil: + if _, err := downloadObject(ctx, client.client, store, endpointHost, username, password, object.OID, object.Actions["download"]); err != nil { failed = append(failed, err) - case stored: - // Already in the cache: the endpoint answered for it, but this - // request never streamed it. - cached = append(cached, &batchObjectError{oid: object.OID, message: "already in the local cache"}) } } - return cached, unavailable, failed + return unavailable, failed } // downloadObject streams one object into the LFS cache, verifying its SHA-256 as diff --git a/internal/lfs/endpoint.go b/internal/lfs/endpoint.go index 64f412a..8a99df7 100644 --- a/internal/lfs/endpoint.go +++ b/internal/lfs/endpoint.go @@ -106,14 +106,15 @@ func (f *Fetcher) selectEndpoint( } } switch { - case disabled != nil: - // A service that answered "LFS is off for this repository" identified - // the API root, and that verdict is the mirror layer's expected skip. A - // wrong path answering differently must not turn the repository into a - // failure. - return "", disabled case failed != nil: + // A real failure outranks "LFS is off for this repository", because the + // mirror layer records that verdict as a successful skip: preferring it + // would present a broken or unauthorized service as a complete mirror + // with no LFS content. Losing a disabled verdict to a wrong path's answer + // is the cheaper mistake — noisy, visible, and retryable. return "", failed + case disabled != nil: + return "", disabled default: // No candidate serves the API. The mirror layer records that as LFS // being switched off rather than failing the repository, which is the diff --git a/internal/lfs/lfs.go b/internal/lfs/lfs.go index e35c048..daed4e6 100644 --- a/internal/lfs/lfs.go +++ b/internal/lfs/lfs.go @@ -208,12 +208,13 @@ func (f *Fetcher) fetchBatches( // chunkOutcome reports what one batch request could not fetch — objects the // endpoint answered for but will not serve, and objects whose own batch request -// the endpoint rejected outright — and whether it answered for any of them. +// the endpoint rejected outright — and whether it answered for any of them, which +// is what separates a rejection beside real answers from one that stands alone. // skipped, when set, names the first object that could not be fetched. type chunkOutcome struct { unavailable int rejected int - served bool + answered bool skipped error } @@ -307,7 +308,7 @@ func isRefusal(err error) bool { // errBatchRejected — which callers report and stop the sweep after, since only a // second such chunk tells stale pointers apart from a broken endpoint. func refuseOrReport(outcome chunkOutcome, rejection error) (chunkOutcome, error) { - if outcome.served || outcome.unavailable > 0 { + if outcome.answered || outcome.unavailable > 0 { return outcome, nil } return outcome, fmt.Errorf("%w: %w", errAllRejected, rejection) @@ -316,25 +317,26 @@ func refuseOrReport(outcome chunkOutcome, rejection error) (chunkOutcome, error) // downloadChunk streams every object the batch scheduled. Objects the endpoint // will not serve are recorded and skipped so their siblings still download, and // so are objects this client could not transfer — attempted, but unreachable — -// which come back as an error once every object has had its turn. A `served` -// chunk is one whose bytes actually arrived here, which is what tells a rejection -// that arrived beside real content from one that stands alone. +// which come back as an error once every object has had its turn. The chunk +// counts as answered when the endpoint replied with content for any scheduled +// object, which is what tells a rejection beside real answers from one that +// stands alone. func (f *Fetcher) downloadChunk( ctx context.Context, store, endpoint, username, password string, objects []batchResponseObject, ) (chunkOutcome, error) { - cached, unavailable, failed := downloadObjects(ctx, f.client, store, endpoint, username, password, objects) + unavailable, failed := downloadObjects(ctx, f.client, store, endpoint, username, password, objects) var outcome chunkOutcome for _, object := range unavailable { outcome.recordUnavailable(fmt.Errorf("%w: %s", errObjectUnavailable, object.Error())) } - // The endpoint answering for an object is the evidence that it processed - // this chunk, and it answered for these either way: streamed now, or already - // in the store. A chunk it answered for is not a chunk it refused, so a - // rejection beside them is one object's fault rather than the endpoint's. - outcome.served = len(objects) > len(unavailable)+len(cached) + // The endpoint answered for every object that is not unavailable, whether it + // streamed the bytes now or found them already in the store. A chunk it + // answered for is not a chunk it refused, so a rejection beside those answers + // is one object's fault rather than the endpoint's. + outcome.answered = len(objects) > len(unavailable) if len(failed) > 0 { return outcome, fmt.Errorf("%d of %d LFS objects could not be downloaded: %w", len(failed), len(objects), failed[0]) } @@ -354,7 +356,7 @@ func (o *chunkOutcome) recordUnavailable(reason error) { func (o *chunkOutcome) absorb(half chunkOutcome) { o.unavailable += half.unavailable o.rejected += half.rejected - o.served = o.served || half.served + o.answered = o.answered || half.answered if half.skipped != nil && o.skipped == nil { o.skipped = half.skipped } From 438e804d14a1a6c85639972dca1f46c1f90b17a9 Mon Sep 17 00:00:00 2001 From: Neureka Date: Thu, 10 Sep 2026 14:08:16 -0700 Subject: [PATCH 07/15] fix(lfs): let a disabled verdict outrank a wrong path's failure When one candidate path answers that LFS is off for the repository and another candidate fails, the disabled verdict now decides: a host that does not serve a guessed path must not turn a repository with no LFS content to mirror into a failed backup. The failure is kept in the debug trail rather than discarded, so a genuine service problem stays visible. Also verified the companion finding and left it unchanged: a locally cached object is not an item the operation failed to store, so it is not reported as a skipped item. The bytes are in the cache and the endpoint answered for them, which is exactly what the chunk's evidence uses them for; per-object "already in the local cache" entries would also put a missing-content warning in front of a user whose mirror is complete. The test forge gained per-path failure statuses so the mixed disabled-and-broken case is covered by a test rather than by reasoning. --- internal/lfs/endpoint.go | 19 ++++++++++++------- internal/lfs/lfs_test.go | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 7 deletions(-) diff --git a/internal/lfs/endpoint.go b/internal/lfs/endpoint.go index 8a99df7..1b8279c 100644 --- a/internal/lfs/endpoint.go +++ b/internal/lfs/endpoint.go @@ -106,15 +106,20 @@ func (f *Fetcher) selectEndpoint( } } switch { - case failed != nil: - // A real failure outranks "LFS is off for this repository", because the - // mirror layer records that verdict as a successful skip: preferring it - // would present a broken or unauthorized service as a complete mirror - // with no LFS content. Losing a disabled verdict to a wrong path's answer - // is the cheaper mistake — noisy, visible, and retryable. - return "", failed case disabled != nil: + // A service that answered "LFS is off for this repository" identified + // the API root, and the mirror layer records that as an expected skip. A + // guess that is simply wrong for the host answering differently must not + // turn a repository with no LFS content to mirror into a failed backup. + if failed != nil { + // The failure still happened, so it is kept for the debug trail + // rather than discarded — it is just not what decides the fetch. + slog.Debug("A candidate path failed while another reported Git LFS disabled.", + "detail", redactedURL(failed.Error())) + } return "", disabled + case failed != nil: + return "", failed default: // No candidate serves the API. The mirror layer records that as LFS // being switched off rather than failing the repository, which is the diff --git a/internal/lfs/lfs_test.go b/internal/lfs/lfs_test.go index 6447cb9..030d54a 100644 --- a/internal/lfs/lfs_test.go +++ b/internal/lfs/lfs_test.go @@ -96,6 +96,9 @@ type fakeLFSServer struct { // serveProbe answers discovery probes normally, so a test can have discovery // succeed while the batch requests that follow are rejected. serveProbe bool + // failedPaths answer a status for one batch path only, so a test can model a + // candidate whose endpoint is broken while another answers. + failedPaths map[string]int // fetchStatus, when non-zero, is the status batch requests get once a probe // has been answered, so a test can model an endpoint that serves discovery // and then stops serving the fetch. @@ -190,6 +193,7 @@ func (s *fakeLFSServer) handleBatch(w http.ResponseWriter, r *http.Request) { rejectWhen, serveProbe := s.rejectWhen, s.serveProbe probed := s.probeCalls > 0 fetchStatus := s.fetchStatus + failedPaths := s.failedPaths disabled := slices.Contains(s.disabledPaths, r.URL.Path) servedHere := slices.Contains(s.servePaths, r.URL.Path) hasServePaths := len(s.servePaths) > 0 @@ -211,6 +215,12 @@ func (s *fakeLFSServer) handleBatch(w http.ResponseWriter, r *http.Request) { _, _ = w.Write([]byte(`{"message":"Git LFS is disabled for this repository."}`)) return } + if failedStatus, fails := failedPaths[r.URL.Path]; fails { + // One candidate path fails while another answers, so a test can model a + // wrong guess that is not merely unmounted. + w.WriteHeader(failedStatus) + return + } if hasServePaths && !servedHere { writeUnmounted(w) return @@ -513,6 +523,33 @@ func TestFetchAllNotServedAnywhereIsSkipped(t *testing.T) { } } +// TestFetchAllDisabledOutranksAWrongPathsFailure covers the mixed case: the +// repository's own API root answers that LFS is switched off, while the other +// candidate path fails outright. The expected skip must win, because a guess that +// is wrong for the host must not turn a repository with no LFS content to mirror +// into a failed backup — and the failure still has to be visible in the debug +// trail rather than discarded. +func TestFetchAllDisabledOutranksAWrongPathsFailure(t *testing.T) { + oid, pointerText := pointerFor([]byte("content")) + + lfsServer := newFakeLFSServer(t, map[string][]byte{oid: []byte("content")}) + // The suffixed path is the repository's own, and reports LFS off; the + // suffix-less path the remote configures is broken. + lfsServer.disabledFor(lfsServer.remoteURL()) + lfsServer.failedPaths = map[string]int{ + batchPathFor(lfsServer.plainRemoteURL()): http.StatusInternalServerError, + } + repositoryPath := newRepoWithLFS(t, map[string]string{"file.bin": pointerText}) + + err := NewFetcher().FetchAll(context.Background(), repositoryPath, lfsServer.plainRemoteURL(), "", "") + if !errors.Is(err, ErrDisabled) { + t.Fatalf("err = %v, want the disabled verdict to be the expected skip", err) + } + if errors.Is(err, ErrNoEndpoint) { + t.Error("a wrong path's answer must not become the reason the repository is skipped") + } +} + func TestFetchAllUnauthorizedIsNotDisabled(t *testing.T) { oid, pointerText := pointerFor([]byte("content")) lfsServer := newFakeLFSServer(t, map[string][]byte{oid: []byte("content")}) From 92032f9a9c2efd566bb1ef3239b1eb992d2f1999 Mon Sep 17 00:00:00 2001 From: Neureka Date: Thu, 10 Sep 2026 14:13:52 -0700 Subject: [PATCH 08/15] fix(lfs): weigh a disabled verdict by which path produced it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A disabled verdict from the configured remote's own path is the repository speaking, so it decides: a derived guess that fails must not turn a repository with nothing to mirror into a failed backup. A disabled verdict from the derived guess is weaker evidence than a failure on the path the remote configures, so the failure decides instead — otherwise a guess answering "LFS is off" would mask a credential, rate-limit, server, or transport failure and the mirror layer would record a repository whose LFS content is missing as a successful skip. Candidates therefore carry whether this package derived them, and selectEndpoint keeps the two verdicts apart instead of ranking the error kinds. --- internal/lfs/endpoint.go | 101 +++++++++++++++++++++++++++------------ internal/lfs/lfs_test.go | 17 +++++-- 2 files changed, 84 insertions(+), 34 deletions(-) diff --git a/internal/lfs/endpoint.go b/internal/lfs/endpoint.go index 1b8279c..a5a73d1 100644 --- a/internal/lfs/endpoint.go +++ b/internal/lfs/endpoint.go @@ -28,7 +28,38 @@ import ( // and scheme: a hostile .lfsconfig must not redirect that credential elsewhere // or downgrade it to plaintext. Reading .lfsconfig is best-effort — any failure // falls back to the derived candidates, matching the common deployment. -func endpointCandidates(repository *git.Repository, parsed *url.URL) ([]string, error) { +// endpointCandidate is one API root worth asking, and whether this package +// derived it rather than taking it from the configured remote. +// +// The distinction decides how much an answer weighs. A path the remote actually +// points at is the repository's own root, so its verdict speaks for the +// repository; a derived path is a guess about how the host routes LFS, and a +// guess answering "LFS is off" is weaker evidence than a real failure on the +// path the remote configures. +type endpointCandidate struct { + url string + // derived marks the suffixed root this package adds when the configured + // remote does not carry one. + derived bool +} + +// endpointCandidates lists, in the order they should be asked, the API roots a +// repository's LFS objects may live under: an lfs.url override from the +// repository's committed .lfsconfig when present, otherwise the remote URL's +// standard /info/lfs root with and without the repository's .git suffix. +// +// Which of the derived roots answers is a property of the host, and no rule +// decides it: GitHub and GitLab route only the suffixed path to their LFS +// service and answer a suffix-less /info/lfs with 422, while a host may mount +// the service exactly where its remote points. The distinction is therefore +// settled by asking, once, before any object is fetched — see selectEndpoint. +// +// The batch request authenticates with the remote's credential, so an override +// is honored only when it is an absolute http(s) URL on the remote's own host +// and scheme: a hostile .lfsconfig must not redirect that credential elsewhere +// or downgrade it to plaintext. Reading .lfsconfig is best-effort — any failure +// falls back to the derived candidates, matching the common deployment. +func endpointCandidates(repository *git.Repository, parsed *url.URL) ([]endpointCandidate, error) { config, err := readLFSConfig(repository) if err == nil && config != nil { if override := strings.TrimSpace(config.Raw.Section("lfs").Option("url")); override != "" { @@ -49,7 +80,7 @@ func endpointCandidates(repository *git.Repository, parsed *url.URL) ([]string, // along: the action path is appended to the endpoint, so anything // after the path would land in the middle of the request line. cleaned := endpointBase(overridden) - return []string{strings.TrimSuffix(cleaned.String(), "/")}, nil + return []endpointCandidate{{url: strings.TrimSuffix(cleaned.String(), "/")}}, nil } } @@ -57,60 +88,70 @@ func endpointCandidates(repository *git.Repository, parsed *url.URL) ([]string, // all, reaches the API at one path, so there is nothing to choose between. suffixed, plain := suffixedEndpoint(parsed), plainEndpoint(parsed) if suffixed == plain { - return []string{suffixed}, nil + return []endpointCandidate{{url: suffixed}}, nil } - return []string{suffixed, plain}, nil + // The configured remote's own path is asked second only because a forge that + // requires the suffix is the more common deployment; it is not the weaker + // candidate. + return []endpointCandidate{{url: suffixed, derived: true}, {url: plain}}, nil } // selectEndpoint answers which candidate serves this repository's LFS API, by // asking each one for a single object before anything is downloaded. // -// Asking first is what keeps the fetch itself simple. A probe separates the +// Asking first is what keeps the fetch itself simple: a probe separates the // answers a host can give — the API answered for the object, LFS is switched // off, nothing is mounted at that path, or the endpoint failed — so the fetch // then runs against one known-good endpoint with no fallback to reconcile. // -// Every candidate is asked, because each answer can be about the path rather -// than the repository: a 403 with an HTML body and a failure both leave the next -// candidate worth trying. What settles the repository is the strongest answer -// collected: a failure outranks "LFS is off", which would otherwise be recorded -// as a successful skip and hide an incomplete mirror. +// Every candidate is asked, and what decides the repository is the strongest +// answer collected. A failure on a path the remote actually configures outranks +// a disabled verdict from a derived guess, because a failure describes the +// endpoint itself and the mirror layer records disabled as a successful skip: +// preferring the guess's verdict would mirror the repository with no LFS content +// and raise nothing. A disabled verdict from the configured path is the +// repository's own answer, so it wins instead — a guess that is merely wrong for +// the host must not turn a repository with nothing to mirror into a failure. func (f *Fetcher) selectEndpoint( ctx context.Context, - candidates []string, + candidates []endpointCandidate, username, password string, first pointer, ) (string, error) { - var disabled, failed error + var disabled, disabledFromConfigured, failed, configuredFailed error for _, candidate := range candidates { - switch err := f.client.probe(ctx, candidate, username, password, first); { + switch err := f.client.probe(ctx, candidate.url, username, password, first); { case err == nil: - slog.Debug("Git LFS API answered.", "endpoint", redactedURL(candidate)) - return candidate, nil + slog.Debug("Git LFS API answered.", "endpoint", redactedURL(candidate.url)) + return candidate.url, nil case errors.Is(err, ErrDisabled): - slog.Debug("Git LFS is switched off for this repository.", "endpoint", redactedURL(candidate)) + slog.Debug("Git LFS is switched off for this repository.", "endpoint", redactedURL(candidate.url)) if disabled == nil { disabled = err } + if !candidate.derived && disabledFromConfigured == nil { + disabledFromConfigured = err + } case errors.Is(err, ErrNoEndpoint): - slog.Debug("No Git LFS API is mounted at this endpoint.", "endpoint", redactedURL(candidate)) + slog.Debug("No Git LFS API is mounted at this endpoint.", "endpoint", redactedURL(candidate.url)) default: // The endpoint and the error both describe URLs, and the error's own // text repeats the one the request went to, so it is redacted too // before it reaches the log. slog.Debug("Git LFS API did not answer.", - "endpoint", redactedURL(candidate), "detail", redactedURL(err.Error())) + "endpoint", redactedURL(candidate.url), "detail", redactedURL(err.Error())) if failed == nil { failed = err } + if !candidate.derived && configuredFailed == nil { + configuredFailed = err + } } } - switch { - case disabled != nil: - // A service that answered "LFS is off for this repository" identified - // the API root, and the mirror layer records that as an expected skip. A - // guess that is simply wrong for the host answering differently must not - // turn a repository with no LFS content to mirror into a failed backup. + + // A disabled verdict only decides when nothing failed on the path the remote + // configures, or when the configured path is the one that reported it. + if disabledFromConfigured != nil || (disabled != nil && configuredFailed == nil) { if failed != nil { // The failure still happened, so it is kept for the debug trail // rather than discarded — it is just not what decides the fetch. @@ -118,14 +159,14 @@ func (f *Fetcher) selectEndpoint( "detail", redactedURL(failed.Error())) } return "", disabled - case failed != nil: + } + if failed != nil { return "", failed - default: - // No candidate serves the API. The mirror layer records that as LFS - // being switched off rather than failing the repository, which is the - // closest truthful reading available from a client. - return "", ErrDisabled } + // No candidate serves the API. The mirror layer records that as LFS being + // switched off rather than failing the repository, which is the closest + // truthful reading available from a client. + return "", ErrDisabled } // suffixedEndpoint derives the remote's standard LFS API root, including the diff --git a/internal/lfs/lfs_test.go b/internal/lfs/lfs_test.go index 030d54a..e3254bf 100644 --- a/internal/lfs/lfs_test.go +++ b/internal/lfs/lfs_test.go @@ -1424,7 +1424,7 @@ func TestEndpointCandidatesCollapseDuplicates(t *testing.T) { t.Fatalf("resolveEndpoints failed: %v", err) } if len(endpoints) != 1 { - t.Errorf("endpoints = %q, want one candidate for a remote that has only one path", redactedURL(strings.Join(endpoints, ","))) + t.Errorf("endpoints = %q, want one candidate for a remote that has only one path", redactedURL(strings.Join(candidateURLs(endpoints), ","))) } }) } @@ -1490,15 +1490,15 @@ func TestEndpointCandidatesOverrideHostAndScheme(t *testing.T) { endpoints, err := endpointCandidates(repository, parsed) if c.wantReject { if err == nil { - t.Fatalf("resolveEndpoints = %q, want rejection", redactedURL(strings.Join(endpoints, ","))) + t.Fatalf("resolveEndpoints = %q, want rejection", redactedURL(strings.Join(candidateURLs(endpoints), ","))) } return } if err != nil { t.Fatalf("resolveEndpoints failed: %v", err) } - if len(endpoints) != 1 || endpoints[0] != c.wantAddress { - t.Errorf("endpoints = %q, want just %q", redactedURL(strings.Join(endpoints, ",")), redactedURL(c.wantAddress)) + if len(endpoints) != 1 || endpoints[0].url != c.wantAddress { + t.Errorf("endpoints = %q, want just %q", redactedURL(strings.Join(candidateURLs(endpoints), ",")), redactedURL(c.wantAddress)) } if lfsServer.batchCallCount() != 0 { t.Error("resolveEndpoints must not contact any endpoint") @@ -1506,3 +1506,12 @@ func TestEndpointCandidatesOverrideHostAndScheme(t *testing.T) { }) } } + +// candidateURLs renders candidates for a test message. +func candidateURLs(candidates []endpointCandidate) []string { + urls := make([]string, 0, len(candidates)) + for _, candidate := range candidates { + urls = append(urls, candidate.url) + } + return urls +} From 6ded13f34476cc2cd5b343ae469603d6b90e63fc Mon Sep 17 00:00:00 2001 From: Neureka Date: Thu, 10 Sep 2026 14:16:19 -0700 Subject: [PATCH 09/15] fix(lfs): let only the configured path's answer decide The two preceding rounds asked for opposite precedence between a disabled verdict and a probe failure, and both are right about their own case, so this replaces the ranking with the rule both were reaching for: only the answer from the path the remote itself configures decides, and a derived guess never overrides it in either direction. - A failure on the configured path is reported even when the guess answers that LFS is off. The mirror layer records that verdict as a successful skip, so preferring it would mirror a repository whose LFS content is missing and raise nothing. - A disabled verdict from the configured path still decides when the guess fails, so a guess that is merely wrong for the host cannot turn a repository with nothing to mirror into a failed backup. The guess only decides when the configured path said nothing itself, which is the case it exists for: a host that routes LFS solely under the suffixed path. The losing answer stays in the debug trail rather than being discarded. Found by CI rather than locally: this machine's Smart App Control now blocks freshly built test binaries, so the unit tests are verified on the runner. --- internal/lfs/endpoint.go | 61 ++++++++++++++++++++-------------------- internal/lfs/lfs_test.go | 43 +++++++++++++++++++--------- 2 files changed, 61 insertions(+), 43 deletions(-) diff --git a/internal/lfs/endpoint.go b/internal/lfs/endpoint.go index a5a73d1..634b321 100644 --- a/internal/lfs/endpoint.go +++ b/internal/lfs/endpoint.go @@ -104,21 +104,22 @@ func endpointCandidates(repository *git.Repository, parsed *url.URL) ([]endpoint // off, nothing is mounted at that path, or the endpoint failed — so the fetch // then runs against one known-good endpoint with no fallback to reconcile. // -// Every candidate is asked, and what decides the repository is the strongest -// answer collected. A failure on a path the remote actually configures outranks -// a disabled verdict from a derived guess, because a failure describes the -// endpoint itself and the mirror layer records disabled as a successful skip: -// preferring the guess's verdict would mirror the repository with no LFS content -// and raise nothing. A disabled verdict from the configured path is the -// repository's own answer, so it wins instead — a guess that is merely wrong for -// the host must not turn a repository with nothing to mirror into a failure. +// Every candidate is asked, and what decides the repository is the answer from +// the path the remote itself configures. A derived guess never overrides that +// path's answer in either direction: a guess reporting "LFS is off" must not mask +// a credential, rate-limit, server, or transport failure on the configured path +// — the mirror layer records disabled as a successful skip, so that would mirror +// the repository with no LFS content and raise nothing — and a guess failing must +// not turn a configured path's "LFS is off" into a failed backup. The guess only +// decides when the configured path said nothing itself, which is the case it +// exists for: a host that routes LFS solely under the suffixed path. func (f *Fetcher) selectEndpoint( ctx context.Context, candidates []endpointCandidate, username, password string, first pointer, ) (string, error) { - var disabled, disabledFromConfigured, failed, configuredFailed error + var configured, derived error for _, candidate := range candidates { switch err := f.client.probe(ctx, candidate.url, username, password, first); { case err == nil: @@ -126,11 +127,12 @@ func (f *Fetcher) selectEndpoint( return candidate.url, nil case errors.Is(err, ErrDisabled): slog.Debug("Git LFS is switched off for this repository.", "endpoint", redactedURL(candidate.url)) - if disabled == nil { - disabled = err - } - if !candidate.derived && disabledFromConfigured == nil { - disabledFromConfigured = err + if candidate.derived { + if derived == nil { + derived = err + } + } else if configured == nil { + configured = err } case errors.Is(err, ErrNoEndpoint): slog.Debug("No Git LFS API is mounted at this endpoint.", "endpoint", redactedURL(candidate.url)) @@ -140,28 +142,27 @@ func (f *Fetcher) selectEndpoint( // before it reaches the log. slog.Debug("Git LFS API did not answer.", "endpoint", redactedURL(candidate.url), "detail", redactedURL(err.Error())) - if failed == nil { - failed = err - } - if !candidate.derived && configuredFailed == nil { - configuredFailed = err + if candidate.derived { + if derived == nil { + derived = err + } + } else if configured == nil { + configured = err } } } - // A disabled verdict only decides when nothing failed on the path the remote - // configures, or when the configured path is the one that reported it. - if disabledFromConfigured != nil || (disabled != nil && configuredFailed == nil) { - if failed != nil { - // The failure still happened, so it is kept for the debug trail - // rather than discarded — it is just not what decides the fetch. - slog.Debug("A candidate path failed while another reported Git LFS disabled.", - "detail", redactedURL(failed.Error())) + if configured != nil { + if derived != nil { + // The guess's answer is kept for the debug trail rather than + // discarded — it is just not what decides the fetch. + slog.Debug("The derived path answered differently from the configured one.", + "detail", redactedURL(derived.Error())) } - return "", disabled + return "", configured } - if failed != nil { - return "", failed + if derived != nil { + return "", derived } // No candidate serves the API. The mirror layer records that as LFS being // switched off rather than failing the repository, which is the closest diff --git a/internal/lfs/lfs_test.go b/internal/lfs/lfs_test.go index e3254bf..e7f88ee 100644 --- a/internal/lfs/lfs_test.go +++ b/internal/lfs/lfs_test.go @@ -523,18 +523,17 @@ func TestFetchAllNotServedAnywhereIsSkipped(t *testing.T) { } } -// TestFetchAllDisabledOutranksAWrongPathsFailure covers the mixed case: the -// repository's own API root answers that LFS is switched off, while the other -// candidate path fails outright. The expected skip must win, because a guess that -// is wrong for the host must not turn a repository with no LFS content to mirror -// into a failed backup — and the failure still has to be visible in the debug -// trail rather than discarded. -func TestFetchAllDisabledOutranksAWrongPathsFailure(t *testing.T) { +// TestFetchAllConfiguredPathDecidesOverTheGuess covers the rule that only the +// path the remote itself configures decides: a failure there is reported even +// when a derived guess answers that LFS is off, because the mirror layer records +// that verdict as a successful skip and would hide a repository whose LFS content +// is missing. +func TestFetchAllConfiguredPathDecidesOverTheGuess(t *testing.T) { oid, pointerText := pointerFor([]byte("content")) lfsServer := newFakeLFSServer(t, map[string][]byte{oid: []byte("content")}) - // The suffixed path is the repository's own, and reports LFS off; the - // suffix-less path the remote configures is broken. + // The remote is suffix-less, so the suffix-less path is configured and the + // suffixed guess is the one that reports LFS off. lfsServer.disabledFor(lfsServer.remoteURL()) lfsServer.failedPaths = map[string]int{ batchPathFor(lfsServer.plainRemoteURL()): http.StatusInternalServerError, @@ -542,11 +541,29 @@ func TestFetchAllDisabledOutranksAWrongPathsFailure(t *testing.T) { repositoryPath := newRepoWithLFS(t, map[string]string{"file.bin": pointerText}) err := NewFetcher().FetchAll(context.Background(), repositoryPath, lfsServer.plainRemoteURL(), "", "") - if !errors.Is(err, ErrDisabled) { - t.Fatalf("err = %v, want the disabled verdict to be the expected skip", err) + if err == nil || errors.Is(err, ErrDisabled) { + t.Fatalf("err = %v, want the configured path's failure reported rather than a skip", err) } - if errors.Is(err, ErrNoEndpoint) { - t.Error("a wrong path's answer must not become the reason the repository is skipped") +} + +// TestFetchAllGuessFailureDoesNotOverruleTheConfiguredPath covers the other +// direction: the configured path reports LFS off while the derived guess fails, +// so the configured path's verdict is what the repository is recorded as. +func TestFetchAllGuessFailureDoesNotOverruleTheConfiguredPath(t *testing.T) { + oid, pointerText := pointerFor([]byte("content")) + + lfsServer := newFakeLFSServer(t, map[string][]byte{oid: []byte("content")}) + // The remote is suffix-less, so the suffix-less path is the configured one, + // and the suffixed guess is the one that fails. + lfsServer.disabledFor(lfsServer.plainRemoteURL()) + lfsServer.failedPaths = map[string]int{ + batchPathFor(lfsServer.remoteURL()): http.StatusInternalServerError, + } + repositoryPath := newRepoWithLFS(t, map[string]string{"file.bin": pointerText}) + + err := NewFetcher().FetchAll(context.Background(), repositoryPath, lfsServer.plainRemoteURL(), "", "") + if !errors.Is(err, ErrDisabled) { + t.Fatalf("err = %v, want the configured path's disabled verdict to be the expected skip", err) } } From 32023811bf42a75bd3518c5818c31d56fb384b9a Mon Sep 17 00:00:00 2001 From: Neureka Date: Thu, 10 Sep 2026 15:48:09 -0700 Subject: [PATCH 10/15] feat(lfs): honour the batch API's credentials, retry, and URL expiry Closes the gaps between this client and what the batch API documents, which a read-only client cannot skip without failing repositories it could have read: - A 401 is "credentials are needed, but were not sent", so the request is repeated once with them. A forge may serve an unauthenticated probe and still require credentials for the objects, and failing there would report a repository as unreadable when the client had the credential all along. With no credentials to offer there is nothing to repeat, so the status is reported like any other failure rather than as a request that wants what it already lacked. - A scheduled URL that has already lapsed is asked about again instead of being used. A long scan or a short expires_in can leave the first URL unusable, and the server is the only party that can issue a fresh one; the alternative is a transfer that can only fail. - Batch actions carry their expires_in and expires_at, which is what makes that judgement possible. Credentials now travel as one value rather than as loose strings, because the retry has to know whether there is anything to offer: a request without credentials is never repeated. Not taken here, and why: the optional ref field (LFS v2.4) needs a ref that is correct for the objects in question, and a wrong one is worse than an absent one, so it belongs in its own change with its own tests; hash_algo is assumed to be sha256, the documented default; and there is still no retry or backoff for transient 5xx, which the per-run object cache makes a robustness gap rather than a correctness one. --- internal/lfs/batch.go | 183 +++++++++++++++++++++++++++++---------- internal/lfs/endpoint.go | 4 +- internal/lfs/lfs.go | 53 +++++++++--- internal/lfs/lfs_test.go | 90 ++++++++++++++++++- 4 files changed, 266 insertions(+), 64 deletions(-) diff --git a/internal/lfs/batch.go b/internal/lfs/batch.go index 2eabd09..0afce28 100644 --- a/internal/lfs/batch.go +++ b/internal/lfs/batch.go @@ -31,9 +31,25 @@ type batchRequest struct { // batchAction is one server-provided action, here the basic-transfer download // instruction with its (typically pre-signed) URL. +// +// The expiry fields matter to a long run: a pre-signed URL stops working once it +// lapses, and the server is the only party that can issue a fresh one. type batchAction struct { - Href string `json:"href"` - Header map[string]string `json:"header"` + Href string `json:"href"` + Header map[string]string `json:"header"` + ExpiresIn int64 `json:"expires_in"` + ExpiresAt string `json:"expires_at"` +} + +// expired reports whether the action's URL has already lapsed. A past ExpiresAt +// is authoritative; otherwise the relative expiry is measured from now. +func (a *batchAction) expired() bool { + if a.ExpiresAt != "" { + if at, err := time.Parse(time.RFC3339, a.ExpiresAt); err == nil { + return !at.After(time.Now()) + } + } + return a.ExpiresIn < 0 } // batchResponseObject is the server's verdict for one object; exactly one of @@ -61,6 +77,12 @@ type batchResponse struct { // whole batch's. var errBatchRejected = errors.New("batch request rejected by the endpoint") +// errCredentialsNeeded marks a batch request the endpoint answered with 401, +// which the batch API documents as "credentials are needed, but were not sent". +// The request is repeated once with them when there are any to offer; when there +// are none the error is reported to the caller with its status intact. +var errCredentialsNeeded = errors.New("batch request needs credentials") + // errObjectUnavailable reports that the endpoint answered the batch request but // will not serve one of the objects it named, so the object cannot be mirrored. var errObjectUnavailable = errors.New("lfs object unavailable on the remote") @@ -103,7 +125,19 @@ const lfsMediaType = "application/vnd.git-lfs+json" // refused probe is about the path rather than the request's shape. A nil return // therefore means the API root is right, and the object's own fate is the // fetch's business. -func (c *batchClient) probe(ctx context.Context, endpoint, username, password string, object pointer) error { +// A 401 is answered by repeating the probe once with the credentials, as the +// batch API documents, because a forge may serve an unauthenticated probe and +// still require credentials for the objects themselves. +func (c *batchClient) probe(ctx context.Context, endpoint string, creds credentials, object pointer) error { + err := c.doProbe(ctx, endpoint, creds, object, false) + if !errors.Is(err, errCredentialsNeeded) || !creds.available() { + return err + } + return c.doProbe(ctx, endpoint, creds, object, true) +} + +// doProbe performs one probe request, optionally offering the credentials. +func (c *batchClient) doProbe(ctx context.Context, endpoint string, creds credentials, object pointer, sendCredentials bool) error { body, err := json.Marshal(batchRequest{ Operation: "download", Transfers: []string{"basic"}, @@ -119,8 +153,8 @@ func (c *batchClient) probe(ctx context.Context, endpoint, username, password st } request.Header.Set("Content-Type", lfsMediaType) request.Header.Set("Accept", lfsMediaType) - if username != "" || password != "" { - request.SetBasicAuth(username, password) + if sendCredentials { + request.SetBasicAuth(creds.username, creds.password) } response, err := c.client.Do(request) @@ -134,6 +168,12 @@ func (c *batchClient) probe(ctx context.Context, endpoint, username, password st // The API answered for the object, whether by scheduling it or by // reporting that the server does not have it. return nil + case http.StatusUnauthorized: + // The API wants credentials it was not given; the caller repeats the + // probe with them when there are any to offer. With none to offer the + // status is reported like any other failure, because that is what + // happened: the endpoint refused the request it was sent. + return fmt.Errorf("%w: batch request failed with status %d", errCredentialsNeeded, response.StatusCode) case http.StatusForbidden: // Two different things answer 403: a forge saying LFS is switched off // for this repository, and a host page refusing a path it does not @@ -172,6 +212,21 @@ func classifyRefusal(response *http.Response, verdict error) error { return verdict } +// credentials are the HTTP basic-auth pair the remote's own API accepts. They +// travel as a value rather than as raw strings because a client may need to +// offer them more than once: the batch API answers 401 when credentials are +// needed but were not sent, and the request is then retried once with them. +type credentials struct { + username string + password string +} + +// available reports whether there is anything to offer. A request with no +// credentials is never retried, because retrying would send the same request. +func (c credentials) available() bool { + return c.username != "" || c.password != "" +} + // batch submits every pointer for download scheduling. // // A status describing the request's shape is reported as errBatchRejected so @@ -180,49 +235,72 @@ func classifyRefusal(response *http.Response, verdict error) error { // chunk would only repeat the same answer more slowly. Discovery already // established which endpoint serves the API, so a 403 or 404 here describes a // service that has stopped answering rather than a path worth retrying. -func (c *batchClient) batch(ctx context.Context, endpoint, username, password string, pointers []pointer) ([]batchResponseObject, error) { +// +// A 401 means the credentials were needed but not presented — a forge may accept +// an unauthenticated probe and then require credentials for the objects — so the +// request is repeated once with them, as the batch API documents, rather than +// failing a repository that the client could have read. +func (c *batchClient) batch(ctx context.Context, creds credentials, endpoint string, pointers []pointer) ([]batchResponseObject, error) { + objects, attempted, err := c.doBatch(ctx, creds, endpoint, pointers, false) + if attempted || !errors.Is(err, errCredentialsNeeded) || !creds.available() { + return objects, err + } + objects, _, err = c.doBatch(ctx, creds, endpoint, pointers, true) + return objects, err +} + +// doBatch performs one batch request, optionally offering the credentials. +func (c *batchClient) doBatch( + ctx context.Context, + creds credentials, + endpoint string, + pointers []pointer, + sendCredentials bool, +) ([]batchResponseObject, bool, error) { body, err := json.Marshal(batchRequest{ Operation: "download", Transfers: []string{"basic"}, Objects: toBatchObjects(pointers), }) if err != nil { - return nil, err + return nil, sendCredentials, err } request, err := http.NewRequestWithContext(ctx, http.MethodPost, endpoint+"/objects/batch", bytes.NewReader(body)) if err != nil { - return nil, err + return nil, sendCredentials, err } request.Header.Set("Content-Type", lfsMediaType) request.Header.Set("Accept", lfsMediaType) - if username != "" || password != "" { - request.SetBasicAuth(username, password) + if sendCredentials { + request.SetBasicAuth(creds.username, creds.password) } response, err := c.client.Do(request) if err != nil { - return nil, fmt.Errorf("batch request: %w", err) + return nil, sendCredentials, fmt.Errorf("batch request: %w", err) } defer drainAndClose(response) switch { case response.StatusCode == http.StatusOK: // Handled below. + case response.StatusCode == http.StatusUnauthorized: + return nil, sendCredentials, fmt.Errorf("%w: batch request failed with status %d", errCredentialsNeeded, response.StatusCode) case describesRequestShape(response.StatusCode): - return nil, fmt.Errorf("%w: status %d", errBatchRejected, response.StatusCode) + return nil, sendCredentials, fmt.Errorf("%w: status %d", errBatchRejected, response.StatusCode) default: - return nil, fmt.Errorf("batch request failed with status %d", response.StatusCode) + return nil, sendCredentials, fmt.Errorf("batch request failed with status %d", response.StatusCode) } var decoded batchResponse if err := json.NewDecoder(io.LimitReader(response.Body, 512<<20)).Decode(&decoded); err != nil { - return nil, fmt.Errorf("decode batch response: %w", err) + return nil, sendCredentials, fmt.Errorf("decode batch response: %w", err) } if len(decoded.Objects) != len(pointers) { - return nil, fmt.Errorf("batch response has %d objects for %d pointers", len(decoded.Objects), len(pointers)) + return nil, sendCredentials, fmt.Errorf("batch response has %d objects for %d pointers", len(decoded.Objects), len(pointers)) } - return decoded.Objects, nil + return decoded.Objects, sendCredentials, nil } // describesRequestShape reports whether a status is how servers answer a request @@ -256,18 +334,19 @@ func looksLikeHTML(body []byte) bool { // skipped, so repeated snapshots only download new content. // // Every object is attempted whatever the others did, so one bad pointer or one -// transient transfer failure cannot hide its siblings. What failed comes back in -// two lists: unavailable for objects the endpoint will not serve, and failed for -// objects this client could not transfer — a corrupt body, a broken connection — -// which the caller reports rather than recording as the endpoint's refusal. Both -// lists are about objects the endpoint did not answer for with content; what it -// answered for is what the caller counts as evidence that it processed the chunk. +// transient transfer failure cannot hide its siblings. What could not be fetched +// comes back in three lists: expired for objects whose scheduled URL had already +// lapsed, which the caller asks the endpoint about again; unavailable for objects +// the endpoint will not serve; and failed for objects this client could not +// transfer — a corrupt body, a broken connection — which the caller reports +// rather than recording as the endpoint's refusal. func downloadObjects( ctx context.Context, client *batchClient, - store, endpoint, username, password string, + store, endpoint string, + creds credentials, objects []batchResponseObject, -) (unavailable []*batchObjectError, failed []error) { +) (expired []pointer, unavailable []*batchObjectError, failed []error) { endpointHost := hostOf(endpoint) answered := func(object batchResponseObject) *batchObjectError { switch { @@ -284,38 +363,46 @@ func downloadObjects( for _, object := range objects { if err := ctx.Err(); err != nil { - return unavailable, append(failed, err) + return expired, unavailable, append(failed, err) } if refused := answered(object); refused != nil { unavailable = append(unavailable, refused) continue } - if _, err := downloadObject(ctx, client.client, store, endpointHost, username, password, object.OID, object.Actions["download"]); err != nil { + action := object.Actions["download"] + if action.expired() { + // The URL lapsed before it was used — a long scan, a slow batch, a + // small expires_in — so the object needs a fresh one rather than a + // transfer that can only fail. + expired = append(expired, pointer{oid: object.OID, size: object.Size}) + continue + } + if _, err := os.Stat(filepath.Join(store, object.OID[0:2], object.OID[2:4], object.OID)); err == nil { + // Already cached by a previous snapshot; git-lfs also skips it. + continue + } + if err := downloadObject(ctx, client.client, store, endpointHost, creds, object.OID, action); err != nil { failed = append(failed, err) } } - return unavailable, failed + return expired, unavailable, failed } // downloadObject streams one object into the LFS cache, verifying its SHA-256 as -// bytes arrive. It reports whether the object was already cached, so a caller can -// tell an endpoint that streamed the bytes from one that merely answered for -// them. +// bytes arrive. func downloadObject( ctx context.Context, client *http.Client, - store, endpointHost, username, password, oid string, + store, endpointHost string, + creds credentials, + oid string, action *batchAction, -) (bool, error) { +) error { destination := filepath.Join(store, oid[0:2], oid[2:4], oid) - if _, err := os.Stat(destination); err == nil { - // Already cached by a previous snapshot; git-lfs also skips it. - return true, nil - } request, err := http.NewRequestWithContext(ctx, http.MethodGet, action.Href, nil) if err != nil { - return false, fmt.Errorf("LFS object %s: %w", shortOID(oid), err) + return fmt.Errorf("LFS object %s: %w", shortOID(oid), err) } for key, value := range action.Header { request.Header.Set(key, value) @@ -323,27 +410,27 @@ func downloadObject( // Pre-signed download URLs carry their own authorization; the basic // credential only applies while the request stays on the LFS endpoint's // host. - if hostOf(action.Href) == endpointHost && (username != "" || password != "") { - request.SetBasicAuth(username, password) + if hostOf(action.Href) == endpointHost && creds.available() { + request.SetBasicAuth(creds.username, creds.password) } response, err := client.Do(request) if err != nil { - return false, fmt.Errorf("LFS object %s: %w", shortOID(oid), err) + return fmt.Errorf("LFS object %s: %w", shortOID(oid), err) } defer drainAndClose(response) if response.StatusCode != http.StatusOK { - return false, fmt.Errorf("LFS object %s download failed with status %d", shortOID(oid), response.StatusCode) + return fmt.Errorf("LFS object %s download failed with status %d", shortOID(oid), response.StatusCode) } if err := os.MkdirAll(filepath.Dir(destination), 0o755); err != nil { - return false, fmt.Errorf("create LFS cache directory: %w", err) + return fmt.Errorf("create LFS cache directory: %w", err) } temp, err := os.CreateTemp(filepath.Dir(destination), ".lfs-download-*") if err != nil { - return false, fmt.Errorf("create LFS temp file: %w", err) + return fmt.Errorf("create LFS temp file: %w", err) } tempName := temp.Name() @@ -352,24 +439,24 @@ func downloadObject( closeErr := temp.Close() if copyErr != nil { _ = os.Remove(tempName) - return false, fmt.Errorf("LFS object %s: %w", shortOID(oid), copyErr) + return fmt.Errorf("LFS object %s: %w", shortOID(oid), copyErr) } if closeErr != nil { _ = os.Remove(tempName) - return false, fmt.Errorf("LFS object %s: %w", shortOID(oid), closeErr) + return fmt.Errorf("LFS object %s: %w", shortOID(oid), closeErr) } if actual := hex.EncodeToString(hash.Sum(nil)); actual != oid { _ = os.Remove(tempName) - return false, fmt.Errorf("LFS object %s content hash mismatch", shortOID(oid)) + return fmt.Errorf("LFS object %s content hash mismatch", shortOID(oid)) } if err := os.Rename(tempName, destination); err != nil { _ = os.Remove(tempName) - return false, fmt.Errorf("store LFS object %s: %w", shortOID(oid), err) + return fmt.Errorf("store LFS object %s: %w", shortOID(oid), err) } _ = size - return false, nil + return nil } func toBatchObjects(pointers []pointer) []batchObject { diff --git a/internal/lfs/endpoint.go b/internal/lfs/endpoint.go index 634b321..be57045 100644 --- a/internal/lfs/endpoint.go +++ b/internal/lfs/endpoint.go @@ -116,12 +116,12 @@ func endpointCandidates(repository *git.Repository, parsed *url.URL) ([]endpoint func (f *Fetcher) selectEndpoint( ctx context.Context, candidates []endpointCandidate, - username, password string, + creds credentials, first pointer, ) (string, error) { var configured, derived error for _, candidate := range candidates { - switch err := f.client.probe(ctx, candidate.url, username, password, first); { + switch err := f.client.probe(ctx, candidate.url, creds, first); { case err == nil: slog.Debug("Git LFS API answered.", "endpoint", redactedURL(candidate.url)) return candidate.url, nil diff --git a/internal/lfs/lfs.go b/internal/lfs/lfs.go index daed4e6..f40541a 100644 --- a/internal/lfs/lfs.go +++ b/internal/lfs/lfs.go @@ -72,13 +72,14 @@ func (f *Fetcher) FetchAll(ctx context.Context, repositoryPath, remoteURL, usern if err != nil { return err } - endpoint, err := f.selectEndpoint(ctx, candidates, username, password, pointers[0]) + creds := credentials{username: username, password: password} + endpoint, err := f.selectEndpoint(ctx, candidates, creds, pointers[0]) if err != nil { return err } store := objectStoreDir(repository, repositoryPath) - outcome, err := f.fetchBatches(ctx, store, endpoint, username, password, pointers) + outcome, err := f.fetchBatches(ctx, store, endpoint, creds, pointers) if err != nil { return err } @@ -168,7 +169,8 @@ var ErrNoEndpoint = errors.New("no git lfs service answered at the endpoint") // repository. func (f *Fetcher) fetchBatches( ctx context.Context, - store, endpoint, username, password string, + store, endpoint string, + creds credentials, pointers []pointer, ) (chunkOutcome, error) { var total chunkOutcome @@ -179,7 +181,7 @@ func (f *Fetcher) fetchBatches( return total, err } - outcome, err := f.fetchChunk(ctx, store, endpoint, username, password, pointers[start:end]) + outcome, err := f.fetchChunk(ctx, store, endpoint, creds, pointers[start:end]) total.absorb(outcome) if err == nil { continue @@ -236,12 +238,13 @@ var errAllRejected = errors.New("every request for this batch was rejected") // object alone instead of the chunk's whole LFS content. func (f *Fetcher) fetchChunk( ctx context.Context, - store, endpoint, username, password string, + store, endpoint string, + creds credentials, pointers []pointer, ) (chunkOutcome, error) { - objects, err := f.client.batch(ctx, endpoint, username, password, pointers) + objects, err := f.client.batch(ctx, creds, endpoint, pointers) if err == nil { - return f.downloadChunk(ctx, store, endpoint, username, password, objects) + return f.downloadChunk(ctx, store, endpoint, creds, objects) } if !errors.Is(err, errBatchRejected) { return chunkOutcome{}, err @@ -256,15 +259,15 @@ func (f *Fetcher) fetchChunk( // makes the chunk's outcome complete whichever half went wrong. if len(pointers) > 1 { middle := len(pointers) / 2 - outcome, firstErr := f.fetchChunk(ctx, store, endpoint, username, password, pointers[:middle]) + outcome, firstErr := f.fetchChunk(ctx, store, endpoint, creds, pointers[:middle]) if firstErr != nil && !isRefusal(firstErr) { // A half that failed outright still must not cost its sibling. - rest, _ := f.fetchChunk(ctx, store, endpoint, username, password, pointers[middle:]) + rest, _ := f.fetchChunk(ctx, store, endpoint, creds, pointers[middle:]) outcome.absorb(rest) return outcome, firstErr } - rest, restErr := f.fetchChunk(ctx, store, endpoint, username, password, pointers[middle:]) + rest, restErr := f.fetchChunk(ctx, store, endpoint, creds, pointers[middle:]) outcome.absorb(rest) if restErr != nil && !isRefusal(restErr) { return outcome, restErr @@ -321,14 +324,40 @@ func refuseOrReport(outcome chunkOutcome, rejection error) (chunkOutcome, error) // counts as answered when the endpoint replied with content for any scheduled // object, which is what tells a rejection beside real answers from one that // stands alone. +// +// Objects whose scheduled URL had already lapsed are asked about once more: the +// server is the only party that can issue a fresh URL, and a lapsed one would +// otherwise fail a transfer that was ready to succeed. func (f *Fetcher) downloadChunk( ctx context.Context, - store, endpoint, username, password string, + store, endpoint string, + creds credentials, objects []batchResponseObject, ) (chunkOutcome, error) { - unavailable, failed := downloadObjects(ctx, f.client, store, endpoint, username, password, objects) + expired, unavailable, failed := downloadObjects(ctx, f.client, store, endpoint, creds, objects) var outcome chunkOutcome + if len(expired) > 0 { + rescheduled, err := f.client.batch(ctx, creds, endpoint, expired) + if err != nil { + // The fresh request failing is the fetch's problem to report, and + // the objects it covered count as unserved. + for range expired { + outcome.recordUnavailable(fmt.Errorf("%w: no fresh download URL was issued", errObjectUnavailable)) + } + failed = append(failed, err) + } else { + againExpired, againUnavailable, againFailed := downloadObjects(ctx, f.client, store, endpoint, creds, rescheduled) + for range againExpired { + // A server issuing an already-lapsed URL twice has nothing more + // to offer for these objects. + outcome.recordUnavailable(fmt.Errorf("%w: the download URL lapsed before it could be used", errObjectUnavailable)) + } + unavailable = append(unavailable, againUnavailable...) + failed = append(failed, againFailed...) + } + } + for _, object := range unavailable { outcome.recordUnavailable(fmt.Errorf("%w: %s", errObjectUnavailable, object.Error())) } diff --git a/internal/lfs/lfs_test.go b/internal/lfs/lfs_test.go index e7f88ee..77d17c4 100644 --- a/internal/lfs/lfs_test.go +++ b/internal/lfs/lfs_test.go @@ -99,6 +99,14 @@ type fakeLFSServer struct { // failedPaths answer a status for one batch path only, so a test can model a // candidate whose endpoint is broken while another answers. failedPaths map[string]int + // requireAuth answers 401 to any request without an Authorization header, + // which is how a forge that wants credentials for the objects behaves. + requireAuth bool + // lapseFirstAction makes the first schedule for each object point at an + // already-expired URL, so the client has to ask for a fresh one. + lapseFirstAction bool + // scheduled counts how many times each object has been scheduled. + scheduled map[string]int // fetchStatus, when non-zero, is the status batch requests get once a probe // has been answered, so a test can model an endpoint that serves discovery // and then stops serving the fetch. @@ -109,7 +117,7 @@ type fakeLFSServer struct { func newFakeLFSServer(t *testing.T, data map[string][]byte) *fakeLFSServer { t.Helper() - s := &fakeLFSServer{data: data, downloads: make(map[string]int)} + s := &fakeLFSServer{data: data, downloads: make(map[string]int), scheduled: make(map[string]int)} mux := http.NewServeMux() mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { switch { @@ -194,6 +202,7 @@ func (s *fakeLFSServer) handleBatch(w http.ResponseWriter, r *http.Request) { probed := s.probeCalls > 0 fetchStatus := s.fetchStatus failedPaths := s.failedPaths + requireAuth := s.requireAuth disabled := slices.Contains(s.disabledPaths, r.URL.Path) servedHere := slices.Contains(s.servePaths, r.URL.Path) hasServePaths := len(s.servePaths) > 0 @@ -208,6 +217,12 @@ func (s *fakeLFSServer) handleBatch(w http.ResponseWriter, r *http.Request) { writeUnmounted(w) return } + if requireAuth && r.Header.Get("Authorization") == "" { + // The API wants credentials it was not given, which the client is + // expected to retry with. + w.WriteHeader(http.StatusUnauthorized) + return + } if disabled { // A forge with LFS switched off explains itself in JSON. w.Header().Set("Content-Type", lfsMediaType) @@ -301,10 +316,20 @@ func (s *fakeLFSServer) respond(w http.ResponseWriter, request batchRequest) { }) continue } + action := &batchAction{Href: s.server.URL + "/download/" + object.OID} + s.mu.Lock() + s.scheduled[object.OID]++ + lapse := s.lapseFirstAction && s.scheduled[object.OID] == 1 + s.mu.Unlock() + if lapse { + // A URL that has already lapsed, as a server issuing a short-lived + // one before a slow scan would produce. + action.ExpiresAt = time.Now().Add(-time.Minute).UTC().Format(time.RFC3339) + } response.Objects = append(response.Objects, batchResponseObject{ OID: object.OID, Size: int64(len(content)), - Actions: map[string]*batchAction{"download": {Href: s.server.URL + "/download/" + object.OID}}, + Actions: map[string]*batchAction{"download": action}, }) } w.Header().Set("Content-Type", lfsMediaType) @@ -567,6 +592,67 @@ func TestFetchAllGuessFailureDoesNotOverruleTheConfiguredPath(t *testing.T) { } } +// TestFetchAllRetriesWithCredentialsOnUnauthorized covers a forge that answers +// 401 when credentials were not presented — which the batch API documents as +// "credentials are needed, but were not sent" — so the client repeats the request +// with them rather than failing a repository it could have read. +func TestFetchAllRetriesWithCredentialsOnUnauthorized(t *testing.T) { + content := []byte("content") + oid, pointerText := pointerFor(content) + + lfsServer := newFakeLFSServer(t, map[string][]byte{oid: content}) + lfsServer.requireAuth = true + repositoryPath := newRepoWithLFS(t, map[string]string{"file.bin": pointerText}) + + if err := NewFetcher().FetchAll(context.Background(), repositoryPath, lfsServer.remoteURL(), "user", "token"); err != nil { + t.Fatalf("FetchAll failed: %v", err) + } + if _, err := os.Stat(filepath.Join(repositoryPath, ".git", "lfs", "objects", + oid[0:2], oid[2:4], oid)); err != nil { + t.Errorf("the object should be mirrored after the authenticated retry: %v", err) + } + + // Without credentials there is nothing to retry with, so the 401 stands. + unauthenticated := newRepoWithLFS(t, map[string]string{"file.bin": pointerText}) + err := NewFetcher().FetchAll(context.Background(), unauthenticated, lfsServer.remoteURL(), "", "") + if err == nil { + t.Fatal("a 401 with no credentials to offer should be reported") + } + if errors.Is(err, ErrDisabled) { + t.Errorf("err = %v, want a failure rather than a repository recorded as LFS-free", err) + } +} + +// TestFetchAllAsksAgainWhenTheDownloadURLLapsed covers a server that schedules a +// URL which has already expired: the object needs a fresh one rather than a +// transfer that can only fail, and the server is the only party that can issue it. +func TestFetchAllAsksAgainWhenTheDownloadURLLapsed(t *testing.T) { + content := []byte("content") + oid, pointerText := pointerFor(content) + + lfsServer := newFakeLFSServer(t, map[string][]byte{oid: content}) + lfsServer.lapseFirstAction = true + repositoryPath := newRepoWithLFS(t, map[string]string{"file.bin": pointerText}) + + if err := NewFetcher().FetchAll(context.Background(), repositoryPath, lfsServer.remoteURL(), "", ""); err != nil { + t.Fatalf("FetchAll failed: %v", err) + } + if _, err := os.Stat(filepath.Join(repositoryPath, ".git", "lfs", "objects", + oid[0:2], oid[2:4], oid)); err != nil { + t.Errorf("the object should be mirrored after the URL was refreshed: %v", err) + } + + lfsServer.mu.Lock() + schedules := lfsServer.scheduled[oid] + lfsServer.mu.Unlock() + if schedules != 2 { + t.Errorf("object was scheduled %d times, want it asked about again after the lapsed URL", schedules) + } + if downloads := lfsServer.downloadCount(oid); downloads != 1 { + t.Errorf("download count = %d, want the object streamed exactly once", downloads) + } +} + func TestFetchAllUnauthorizedIsNotDisabled(t *testing.T) { oid, pointerText := pointerFor([]byte("content")) lfsServer := newFakeLFSServer(t, map[string][]byte{oid: []byte("content")}) From 26cf850fd017a3172e9395182907dad5be025219 Mon Sep 17 00:00:00 2001 From: Neureka Date: Thu, 10 Sep 2026 16:05:07 -0700 Subject: [PATCH 11/15] fix(lfs): offer credentials on the first request and keep unserved objects named MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the credential work found three defects, all in the new code: - Deferring the credentials to a 401 retry regressed authenticated forges. The probe and the batch both sent basic auth up front before, and a forge that answers 403 to an anonymous request — which is also how a repository with LFS disabled answers — was read as LFS-free, so the repository was recorded as having no LFS content instead of failing. Both requests now offer the credentials when there are any, and a 401 is reported as the refusal it is rather than retried with the same credentials, which is what the retry did. - An object already in the store was checked for expiry before it was checked for being cached, so a second run over mirrored content sent it back for rescheduling and could fail over a fresh URL the content never needed. The cache is now consulted first. - Objects whose refresh failed were counted with a bare, unnamed error, so a caller learned how many objects were missing but not which. Both refresh branches now name the object, and the endpoint's refusal is carried as that record's reason instead of being appended as a second, chunk-level failure over objects that had already been accounted for. Coverage: a forge that refuses anonymous requests with 403, cached content whose scheduled URL has lapsed, and both refresh-failure branches. --- internal/lfs/batch.go | 107 +++++++++------------ internal/lfs/lfs.go | 19 ++-- internal/lfs/lfs_test.go | 195 +++++++++++++++++++++++++++++++++++++-- 3 files changed, 242 insertions(+), 79 deletions(-) diff --git a/internal/lfs/batch.go b/internal/lfs/batch.go index 0afce28..3ca7f74 100644 --- a/internal/lfs/batch.go +++ b/internal/lfs/batch.go @@ -77,12 +77,6 @@ type batchResponse struct { // whole batch's. var errBatchRejected = errors.New("batch request rejected by the endpoint") -// errCredentialsNeeded marks a batch request the endpoint answered with 401, -// which the batch API documents as "credentials are needed, but were not sent". -// The request is repeated once with them when there are any to offer; when there -// are none the error is reported to the caller with its status intact. -var errCredentialsNeeded = errors.New("batch request needs credentials") - // errObjectUnavailable reports that the endpoint answered the batch request but // will not serve one of the objects it named, so the object cannot be mirrored. var errObjectUnavailable = errors.New("lfs object unavailable on the remote") @@ -114,6 +108,17 @@ func newBatchClient(client *http.Client) *batchClient { const lfsMediaType = "application/vnd.git-lfs+json" +// authorize offers the credentials to a request when there are any to offer. +// Every batch request goes out authenticated rather than waiting for a 401, +// because a forge is free to answer 403 to an anonymous request, and a 403 is +// also how a repository with LFS switched off answers. +func (c credentials) authorize(request *http.Request) { + if !c.available() { + return + } + request.SetBasicAuth(c.username, c.password) +} + // probe asks an endpoint for one object to find out whether it serves this // repository's LFS API at all. Discovery happens once, before any object is // fetched, so the fetch itself runs against a single known-good endpoint: the @@ -125,19 +130,18 @@ const lfsMediaType = "application/vnd.git-lfs+json" // refused probe is about the path rather than the request's shape. A nil return // therefore means the API root is right, and the object's own fate is the // fetch's business. -// A 401 is answered by repeating the probe once with the credentials, as the -// batch API documents, because a forge may serve an unauthenticated probe and -// still require credentials for the objects themselves. +// +// The credentials are offered on the first request when there are any. Waiting +// for a 401 to offer them would read a forge that answers 403 to anonymous +// requests as a repository with LFS switched off, which backs it up without its +// LFS content instead of reporting that the credential was refused. func (c *batchClient) probe(ctx context.Context, endpoint string, creds credentials, object pointer) error { - err := c.doProbe(ctx, endpoint, creds, object, false) - if !errors.Is(err, errCredentialsNeeded) || !creds.available() { - return err - } - return c.doProbe(ctx, endpoint, creds, object, true) + return c.doProbe(ctx, endpoint, creds, object) } -// doProbe performs one probe request, optionally offering the credentials. -func (c *batchClient) doProbe(ctx context.Context, endpoint string, creds credentials, object pointer, sendCredentials bool) error { +// doProbe performs one probe request, offering the credentials when there are +// any to offer. +func (c *batchClient) doProbe(ctx context.Context, endpoint string, creds credentials, object pointer) error { body, err := json.Marshal(batchRequest{ Operation: "download", Transfers: []string{"basic"}, @@ -153,9 +157,7 @@ func (c *batchClient) doProbe(ctx context.Context, endpoint string, creds creden } request.Header.Set("Content-Type", lfsMediaType) request.Header.Set("Accept", lfsMediaType) - if sendCredentials { - request.SetBasicAuth(creds.username, creds.password) - } + creds.authorize(request) response, err := c.client.Do(request) if err != nil { @@ -169,11 +171,11 @@ func (c *batchClient) doProbe(ctx context.Context, endpoint string, creds creden // reporting that the server does not have it. return nil case http.StatusUnauthorized: - // The API wants credentials it was not given; the caller repeats the - // probe with them when there are any to offer. With none to offer the - // status is reported like any other failure, because that is what - // happened: the endpoint refused the request it was sent. - return fmt.Errorf("%w: batch request failed with status %d", errCredentialsNeeded, response.StatusCode) + // The endpoint refused what it was sent, and the credentials were + // already offered, so this is reported rather than retried: repeating + // the request would send the same credentials again. Reading it as a + // path problem would be worse still, because the path answered. + return fmt.Errorf("batch request failed with status %d", response.StatusCode) case http.StatusForbidden: // Two different things answer 403: a forge saying LFS is switched off // for this repository, and a host page refusing a path it does not @@ -222,7 +224,7 @@ type credentials struct { } // available reports whether there is anything to offer. A request with no -// credentials is never retried, because retrying would send the same request. +// credentials to offer goes out anonymous, because there is nothing to add. func (c credentials) available() bool { return c.username != "" || c.password != "" } @@ -236,71 +238,50 @@ func (c credentials) available() bool { // established which endpoint serves the API, so a 403 or 404 here describes a // service that has stopped answering rather than a path worth retrying. // -// A 401 means the credentials were needed but not presented — a forge may accept -// an unauthenticated probe and then require credentials for the objects — so the -// request is repeated once with them, as the batch API documents, rather than -// failing a repository that the client could have read. +// A 401 means the endpoint would not accept what was sent, so it is reported +// rather than retried: the credentials were already offered, and repeating the +// request would send the same ones. func (c *batchClient) batch(ctx context.Context, creds credentials, endpoint string, pointers []pointer) ([]batchResponseObject, error) { - objects, attempted, err := c.doBatch(ctx, creds, endpoint, pointers, false) - if attempted || !errors.Is(err, errCredentialsNeeded) || !creds.available() { - return objects, err - } - objects, _, err = c.doBatch(ctx, creds, endpoint, pointers, true) - return objects, err -} - -// doBatch performs one batch request, optionally offering the credentials. -func (c *batchClient) doBatch( - ctx context.Context, - creds credentials, - endpoint string, - pointers []pointer, - sendCredentials bool, -) ([]batchResponseObject, bool, error) { body, err := json.Marshal(batchRequest{ Operation: "download", Transfers: []string{"basic"}, Objects: toBatchObjects(pointers), }) if err != nil { - return nil, sendCredentials, err + return nil, err } request, err := http.NewRequestWithContext(ctx, http.MethodPost, endpoint+"/objects/batch", bytes.NewReader(body)) if err != nil { - return nil, sendCredentials, err + return nil, err } request.Header.Set("Content-Type", lfsMediaType) request.Header.Set("Accept", lfsMediaType) - if sendCredentials { - request.SetBasicAuth(creds.username, creds.password) - } + creds.authorize(request) response, err := c.client.Do(request) if err != nil { - return nil, sendCredentials, fmt.Errorf("batch request: %w", err) + return nil, fmt.Errorf("batch request: %w", err) } defer drainAndClose(response) switch { case response.StatusCode == http.StatusOK: // Handled below. - case response.StatusCode == http.StatusUnauthorized: - return nil, sendCredentials, fmt.Errorf("%w: batch request failed with status %d", errCredentialsNeeded, response.StatusCode) case describesRequestShape(response.StatusCode): - return nil, sendCredentials, fmt.Errorf("%w: status %d", errBatchRejected, response.StatusCode) + return nil, fmt.Errorf("%w: status %d", errBatchRejected, response.StatusCode) default: - return nil, sendCredentials, fmt.Errorf("batch request failed with status %d", response.StatusCode) + return nil, fmt.Errorf("batch request failed with status %d", response.StatusCode) } var decoded batchResponse if err := json.NewDecoder(io.LimitReader(response.Body, 512<<20)).Decode(&decoded); err != nil { - return nil, sendCredentials, fmt.Errorf("decode batch response: %w", err) + return nil, fmt.Errorf("decode batch response: %w", err) } if len(decoded.Objects) != len(pointers) { - return nil, sendCredentials, fmt.Errorf("batch response has %d objects for %d pointers", len(decoded.Objects), len(pointers)) + return nil, fmt.Errorf("batch response has %d objects for %d pointers", len(decoded.Objects), len(pointers)) } - return decoded.Objects, sendCredentials, nil + return decoded.Objects, nil } // describesRequestShape reports whether a status is how servers answer a request @@ -369,6 +350,12 @@ func downloadObjects( unavailable = append(unavailable, refused) continue } + if _, err := os.Stat(filepath.Join(store, object.OID[0:2], object.OID[2:4], object.OID)); err == nil { + // Already cached by a previous snapshot; git-lfs also skips it, and + // content already held needs no fresh URL, so this is settled before + // the expiry below can send it back for rescheduling. + continue + } action := object.Actions["download"] if action.expired() { // The URL lapsed before it was used — a long scan, a slow batch, a @@ -377,10 +364,6 @@ func downloadObjects( expired = append(expired, pointer{oid: object.OID, size: object.Size}) continue } - if _, err := os.Stat(filepath.Join(store, object.OID[0:2], object.OID[2:4], object.OID)); err == nil { - // Already cached by a previous snapshot; git-lfs also skips it. - continue - } if err := downloadObject(ctx, client.client, store, endpointHost, creds, object.OID, action); err != nil { failed = append(failed, err) } diff --git a/internal/lfs/lfs.go b/internal/lfs/lfs.go index f40541a..5c987e3 100644 --- a/internal/lfs/lfs.go +++ b/internal/lfs/lfs.go @@ -340,18 +340,21 @@ func (f *Fetcher) downloadChunk( if len(expired) > 0 { rescheduled, err := f.client.batch(ctx, creds, endpoint, expired) if err != nil { - // The fresh request failing is the fetch's problem to report, and - // the objects it covered count as unserved. - for range expired { - outcome.recordUnavailable(fmt.Errorf("%w: no fresh download URL was issued", errObjectUnavailable)) + // The endpoint scheduled these objects and then would not schedule + // them again, so each one is recorded as unserved with its own name + // and the reason the fresh URL never arrived. That record is the + // failure: reporting the request's error again would turn objects + // that were accounted for into an unaccounted chunk failure. + for _, object := range expired { + outcome.recordUnavailable(fmt.Errorf("%w: no fresh download URL was issued for %s: %w", errObjectUnavailable, object.oid, err)) } - failed = append(failed, err) } else { againExpired, againUnavailable, againFailed := downloadObjects(ctx, f.client, store, endpoint, creds, rescheduled) - for range againExpired { + for _, object := range againExpired { // A server issuing an already-lapsed URL twice has nothing more - // to offer for these objects. - outcome.recordUnavailable(fmt.Errorf("%w: the download URL lapsed before it could be used", errObjectUnavailable)) + // to offer for these objects, and each is named so a caller can + // tell which content the mirror is missing. + outcome.recordUnavailable(fmt.Errorf("%w: the download URL for %s lapsed before it could be used", errObjectUnavailable, object.oid)) } unavailable = append(unavailable, againUnavailable...) failed = append(failed, againFailed...) diff --git a/internal/lfs/lfs_test.go b/internal/lfs/lfs_test.go index 77d17c4..4fe39db 100644 --- a/internal/lfs/lfs_test.go +++ b/internal/lfs/lfs_test.go @@ -7,6 +7,7 @@ import ( "encoding/json" "errors" "fmt" + "io" "net/http" "net/http/httptest" "net/url" @@ -25,6 +26,12 @@ import ( "github.com/go-git/go-git/v5/plumbing/object" ) +// roundTripFunc adapts a function to an http.RoundTripper, so a test can script +// the exact responses the batch client sees. +type roundTripFunc func(*http.Request) (*http.Response, error) + +func (f roundTripFunc) RoundTrip(request *http.Request) (*http.Response, error) { return f(request) } + func pointerFor(content []byte) (string, string) { sum := sha256.Sum256(content) oid := hex.EncodeToString(sum[:]) @@ -102,9 +109,18 @@ type fakeLFSServer struct { // requireAuth answers 401 to any request without an Authorization header, // which is how a forge that wants credentials for the objects behaves. requireAuth bool + // forbidAnonymous answers 403 to any request without an Authorization + // header, which is how a forge behaves when it refuses anonymous access + // outright rather than inviting a credential with 401. A 403 is also the + // status for LFS switched off, so a client that discovers the endpoint + // anonymously cannot tell the two apart. + forbidAnonymous bool // lapseFirstAction makes the first schedule for each object point at an // already-expired URL, so the client has to ask for a fresh one. lapseFirstAction bool + // lapseEveryAction makes every schedule for each object point at an + // already-expired URL, so asking again cannot produce a usable one. + lapseEveryAction bool // scheduled counts how many times each object has been scheduled. scheduled map[string]int // fetchStatus, when non-zero, is the status batch requests get once a probe @@ -203,6 +219,7 @@ func (s *fakeLFSServer) handleBatch(w http.ResponseWriter, r *http.Request) { fetchStatus := s.fetchStatus failedPaths := s.failedPaths requireAuth := s.requireAuth + forbidAnonymous := s.forbidAnonymous disabled := slices.Contains(s.disabledPaths, r.URL.Path) servedHere := slices.Contains(s.servePaths, r.URL.Path) hasServePaths := len(s.servePaths) > 0 @@ -223,6 +240,14 @@ func (s *fakeLFSServer) handleBatch(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusUnauthorized) return } + if forbidAnonymous && r.Header.Get("Authorization") == "" { + // An anonymous request is refused outright, and the refusal is shaped + // exactly like a repository with LFS switched off. + w.Header().Set("Content-Type", lfsMediaType) + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"message":"Git LFS is disabled for this repository."}`)) + return + } if disabled { // A forge with LFS switched off explains itself in JSON. w.Header().Set("Content-Type", lfsMediaType) @@ -261,6 +286,9 @@ func (s *fakeLFSServer) handleBatch(w http.ResponseWriter, r *http.Request) { w.WriteHeader(status) return } + // fetchStatus, when set, is what the fetch gets once discovery has been + // answered, so a test can model an endpoint that serves discovery and then + // stops serving the fetch. if fetchStatus != 0 && probed { w.WriteHeader(fetchStatus) return @@ -319,7 +347,7 @@ func (s *fakeLFSServer) respond(w http.ResponseWriter, request batchRequest) { action := &batchAction{Href: s.server.URL + "/download/" + object.OID} s.mu.Lock() s.scheduled[object.OID]++ - lapse := s.lapseFirstAction && s.scheduled[object.OID] == 1 + lapse := s.lapseEveryAction || (s.lapseFirstAction && s.scheduled[object.OID] == 1) s.mu.Unlock() if lapse { // A URL that has already lapsed, as a server issuing a short-lived @@ -592,11 +620,11 @@ func TestFetchAllGuessFailureDoesNotOverruleTheConfiguredPath(t *testing.T) { } } -// TestFetchAllRetriesWithCredentialsOnUnauthorized covers a forge that answers -// 401 when credentials were not presented — which the batch API documents as -// "credentials are needed, but were not sent" — so the client repeats the request -// with them rather than failing a repository it could have read. -func TestFetchAllRetriesWithCredentialsOnUnauthorized(t *testing.T) { +// TestFetchAllOffersCredentialsOnTheFirstRequest covers a forge that serves +// nothing to an anonymous request. The client presents its credentials straight +// away rather than waiting to be asked, so such a forge is read as one that +// wanted the credential rather than as a repository with LFS switched off. +func TestFetchAllOffersCredentialsOnTheFirstRequest(t *testing.T) { content := []byte("content") oid, pointerText := pointerFor(content) @@ -609,20 +637,51 @@ func TestFetchAllRetriesWithCredentialsOnUnauthorized(t *testing.T) { } if _, err := os.Stat(filepath.Join(repositoryPath, ".git", "lfs", "objects", oid[0:2], oid[2:4], oid)); err != nil { - t.Errorf("the object should be mirrored after the authenticated retry: %v", err) + t.Errorf("the object should be mirrored after the authenticated request: %v", err) } - // Without credentials there is nothing to retry with, so the 401 stands. + // Without credentials there is nothing to present, so the 401 stands. unauthenticated := newRepoWithLFS(t, map[string]string{"file.bin": pointerText}) err := NewFetcher().FetchAll(context.Background(), unauthenticated, lfsServer.remoteURL(), "", "") if err == nil { - t.Fatal("a 401 with no credentials to offer should be reported") + t.Fatal("a 401 answered without credentials should be reported") } if errors.Is(err, ErrDisabled) { t.Errorf("err = %v, want a failure rather than a repository recorded as LFS-free", err) } } +// TestFetchAllReadsAForbiddenAnonymousRequestAsCredentialsNeeded covers the case +// a 401-only client gets wrong: a forge that refuses anonymous requests with 403 +// instead of inviting a credential with 401. A 403 is also how a repository with +// LFS switched off answers, so the credentials have to be offered on the first +// request — discovering the endpoint anonymously would record the repository as +// LFS-free and back it up without its LFS content. +func TestFetchAllReadsAForbiddenAnonymousRequestAsCredentialsNeeded(t *testing.T) { + content := []byte("content") + oid, pointerText := pointerFor(content) + + lfsServer := newFakeLFSServer(t, map[string][]byte{oid: content}) + lfsServer.forbidAnonymous = true + repositoryPath := newRepoWithLFS(t, map[string]string{"file.bin": pointerText}) + + if err := NewFetcher().FetchAll(context.Background(), repositoryPath, lfsServer.remoteURL(), "user", "token"); err != nil { + t.Fatalf("FetchAll failed: %v", err) + } + if _, err := os.Stat(filepath.Join(repositoryPath, ".git", "lfs", "objects", + oid[0:2], oid[2:4], oid)); err != nil { + t.Errorf("the object should be mirrored rather than the repository recorded as LFS-free: %v", err) + } + + // The same server without credentials says nothing about the credential, so + // its refusal is read the only way it can be: LFS is not available here. + unauthenticated := newRepoWithLFS(t, map[string]string{"file.bin": pointerText}) + err := NewFetcher().FetchAll(context.Background(), unauthenticated, lfsServer.remoteURL(), "", "") + if !errors.Is(err, ErrDisabled) { + t.Errorf("err = %v, want a repository recorded as LFS-free when the forge refuses anonymously", err) + } +} + // TestFetchAllAsksAgainWhenTheDownloadURLLapsed covers a server that schedules a // URL which has already expired: the object needs a fresh one rather than a // transfer that can only fail, and the server is the only party that can issue it. @@ -653,6 +712,124 @@ func TestFetchAllAsksAgainWhenTheDownloadURLLapsed(t *testing.T) { } } +// TestDownloadObjectsSkipsCachedObjectsWhoseURLLapsed covers content that is +// already in the store being scheduled with an expired URL. The object needs +// nothing from the endpoint, so it is settled from the cache before the expiry +// can send it back for rescheduling, which is what made an already-mirrored +// repository fail over a fresh URL it never needed. +func TestDownloadObjectsSkipsCachedObjectsWhoseURLLapsed(t *testing.T) { + content := []byte("content") + oid, _ := pointerFor(content) + + store := t.TempDir() + cached := filepath.Join(store, oid[0:2], oid[2:4]) + if err := os.MkdirAll(cached, 0o755); err != nil { + t.Fatalf("prepare the store: %v", err) + } + if err := os.WriteFile(filepath.Join(cached, oid), content, 0o644); err != nil { + t.Fatalf("place the cached object: %v", err) + } + + expired, unavailable, failed := downloadObjects(context.Background(), newBatchClient(nil), store, + "http://127.0.0.1:1/repo.git/info/lfs", credentials{}, []batchResponseObject{{ + OID: oid, + Size: int64(len(content)), + Actions: map[string]*batchAction{"download": { + Href: "http://127.0.0.1:1/download/" + oid, + ExpiresAt: time.Now().Add(-time.Minute).UTC().Format(time.RFC3339), + }}, + }}) + + if len(expired) != 0 { + t.Errorf("expired = %v, want cached content not sent back for a fresh URL", expired) + } + if len(unavailable) != 0 || len(failed) != 0 { + t.Errorf("unavailable = %v, failed = %v, want cached content left alone", unavailable, failed) + } +} + +// TestFetchAllNamesObjectsWhoseRefreshFailed covers an endpoint that only ever +// schedules lapsed URLs and then refuses the request for fresh ones. Each object +// has to be reported by name — a count tells a caller nothing about what is +// missing from the mirror — and the refusal is that record's reason rather than +// a second, chunk-level failure over objects already accounted for. +func TestFetchAllNamesObjectsWhoseRefreshFailed(t *testing.T) { + content := []byte("content") + oid, pointerText := pointerFor(content) + + lfsServer := newFakeLFSServer(t, map[string][]byte{oid: content}) + lfsServer.lapseEveryAction = true + repositoryPath := newRepoWithLFS(t, map[string]string{"file.bin": pointerText}) + + err := NewFetcher().FetchAll(context.Background(), repositoryPath, lfsServer.remoteURL(), "", "") + if err == nil { + t.Fatal("an object that never arrived should be reported") + } + if !strings.Contains(err.Error(), oid) { + t.Errorf("err = %v, want it to name the object that could not be fetched", err) + } + if !errors.Is(err, errObjectUnavailable) { + t.Errorf("err = %v, want the object recorded as unserved", err) + } + if _, statErr := os.Stat(filepath.Join(repositoryPath, ".git", "lfs", "objects", + oid[0:2], oid[2:4], oid)); statErr == nil { + t.Error("the object should not be in the store, since it was never downloaded") + } +} + +// TestDownloadChunkReportsARefusedRefreshPerObject covers the endpoint scheduling +// a lapsed URL and then refusing outright to issue a fresh one. The refusal is +// the reason the object went unserved, so it is reported through that object — +// named — rather than as a further chunk-level failure over objects that were +// already accounted for. +func TestDownloadChunkReportsARefusedRefreshPerObject(t *testing.T) { + const endpoint = "http://127.0.0.1:1/repo.git/info/lfs" + oid, _ := pointerFor([]byte("content")) + + // downloadObjects settles the chunk's own objects from the response it is + // handed, so the first request this transport sees is the one asking for a + // fresh URL; that one is refused. + batches := 0 + client := &http.Client{Transport: roundTripFunc(func(*http.Request) (*http.Response, error) { + batches++ + return &http.Response{ + StatusCode: http.StatusInternalServerError, + Body: io.NopCloser(strings.NewReader("")), + Header: make(http.Header), + }, nil + })} + + fetcher := &Fetcher{client: newBatchClient(client)} + outcome, err := fetcher.downloadChunk(context.Background(), t.TempDir(), endpoint, credentials{}, []batchResponseObject{ + {OID: oid, Size: 7, Actions: map[string]*batchAction{"download": { + Href: endpoint + "/download/" + oid, + ExpiresAt: time.Now().Add(-time.Minute).UTC().Format(time.RFC3339), + }}}, + }) + + if batches != 1 { + t.Errorf("batch requests = %d, want one asking for a fresh URL", batches) + } + if err != nil { + t.Errorf("downloadChunk err = %v, want the refusal carried by the object rather than the chunk", err) + } + if outcome.unavailable != 1 { + t.Errorf("unavailable = %d, want the object recorded as unserved", outcome.unavailable) + } + if outcome.skipped == nil { + t.Fatal("the object's reason should be reported") + } + if !errors.Is(outcome.skipped, errObjectUnavailable) { + t.Errorf("reason = %v, want the object recorded as unserved", outcome.skipped) + } + if !strings.Contains(outcome.skipped.Error(), oid) { + t.Errorf("reason = %v, want it to name the object", outcome.skipped) + } + if !strings.Contains(outcome.skipped.Error(), fmt.Sprint(http.StatusInternalServerError)) { + t.Errorf("reason = %v, want the endpoint's own status as why no fresh URL arrived", outcome.skipped) + } +} + func TestFetchAllUnauthorizedIsNotDisabled(t *testing.T) { oid, pointerText := pointerFor([]byte("content")) lfsServer := newFakeLFSServer(t, map[string][]byte{oid: []byte("content")}) From d6bb20d7c39ad3e40898ddf760e69e3e401d589c Mon Sep 17 00:00:00 2001 From: Neureka Date: Thu, 10 Sep 2026 16:15:54 -0700 Subject: [PATCH 12/15] fix(lfs): refuse endpoint OIDs that are malformed or unrequested A batch response's OIDs are used as path components and sliced for the shard directories, but only the response's object count was checked against the pointers sent. An endpoint returning a short OID panicked the slice and took the whole backup with it, and one returning a crafted OID named a path outside the store. Neither is the client's data to trust: every response object is now checked to be a sha256 digest that was actually requested, and anything else is reported as an unserved object. The cached-object check also accepted any file at the object's path as proof the content was mirrored, which skipped the digest verification a download performs. A cached entry now has to be a regular file of the length the endpoint reported, so a partial or unrelated file falls through to a download that verifies it. Coverage: malformed and unrequested OIDs in a batch response, including the short-OID case that used to panic. --- internal/lfs/batch.go | 20 ++++++++++++-- internal/lfs/lfs.go | 7 +++-- internal/lfs/lfs_test.go | 60 +++++++++++++++++++++++++++++++++++----- 3 files changed, 75 insertions(+), 12 deletions(-) diff --git a/internal/lfs/batch.go b/internal/lfs/batch.go index 3ca7f74..27b7df6 100644 --- a/internal/lfs/batch.go +++ b/internal/lfs/batch.go @@ -326,9 +326,18 @@ func downloadObjects( client *batchClient, store, endpoint string, creds credentials, + want []pointer, objects []batchResponseObject, ) (expired []pointer, unavailable []*batchObjectError, failed []error) { endpointHost := hostOf(endpoint) + // The endpoint's OIDs are used as path components and sliced for the shard + // directories, so an object that was not asked for — or one whose OID is not + // a SHA-256 digest — is refused before any of that: a short OID would panic + // the slice, and a crafted one could name a path outside the store. + requested := make(map[string]bool, len(want)) + for _, pointer := range want { + requested[pointer.oid] = true + } answered := func(object batchResponseObject) *batchObjectError { switch { case object.Error != nil: @@ -337,6 +346,10 @@ func downloadObjects( // The server knows the object but scheduled nothing; without an // href there is nothing this client can do beyond reporting it. return &batchObjectError{oid: object.OID, message: "no download action was scheduled"} + case !isSHA256Hex(object.OID): + return &batchObjectError{oid: shortOID(object.OID), message: "the endpoint described an object whose oid is not a sha256 digest"} + case !requested[object.OID]: + return &batchObjectError{oid: shortOID(object.OID), message: "the endpoint described an object that was not requested"} default: return nil } @@ -350,10 +363,13 @@ func downloadObjects( unavailable = append(unavailable, refused) continue } - if _, err := os.Stat(filepath.Join(store, object.OID[0:2], object.OID[2:4], object.OID)); err == nil { + if cached, err := os.Stat(filepath.Join(store, object.OID[0:2], object.OID[2:4], object.OID)); err == nil && + cached.Mode().IsRegular() && cached.Size() == object.Size { // Already cached by a previous snapshot; git-lfs also skips it, and // content already held needs no fresh URL, so this is settled before - // the expiry below can send it back for rescheduling. + // the expiry below can send it back for rescheduling. A file of the + // wrong length is not this object, so it falls through to the + // download, which verifies the digest as the bytes arrive. continue } action := object.Actions["download"] diff --git a/internal/lfs/lfs.go b/internal/lfs/lfs.go index 5c987e3..ebd6422 100644 --- a/internal/lfs/lfs.go +++ b/internal/lfs/lfs.go @@ -244,7 +244,7 @@ func (f *Fetcher) fetchChunk( ) (chunkOutcome, error) { objects, err := f.client.batch(ctx, creds, endpoint, pointers) if err == nil { - return f.downloadChunk(ctx, store, endpoint, creds, objects) + return f.downloadChunk(ctx, store, endpoint, creds, pointers, objects) } if !errors.Is(err, errBatchRejected) { return chunkOutcome{}, err @@ -332,9 +332,10 @@ func (f *Fetcher) downloadChunk( ctx context.Context, store, endpoint string, creds credentials, + want []pointer, objects []batchResponseObject, ) (chunkOutcome, error) { - expired, unavailable, failed := downloadObjects(ctx, f.client, store, endpoint, creds, objects) + expired, unavailable, failed := downloadObjects(ctx, f.client, store, endpoint, creds, want, objects) var outcome chunkOutcome if len(expired) > 0 { @@ -349,7 +350,7 @@ func (f *Fetcher) downloadChunk( outcome.recordUnavailable(fmt.Errorf("%w: no fresh download URL was issued for %s: %w", errObjectUnavailable, object.oid, err)) } } else { - againExpired, againUnavailable, againFailed := downloadObjects(ctx, f.client, store, endpoint, creds, rescheduled) + againExpired, againUnavailable, againFailed := downloadObjects(ctx, f.client, store, endpoint, creds, expired, rescheduled) for _, object := range againExpired { // A server issuing an already-lapsed URL twice has nothing more // to offer for these objects, and each is named so a caller can diff --git a/internal/lfs/lfs_test.go b/internal/lfs/lfs_test.go index 4fe39db..495c8bb 100644 --- a/internal/lfs/lfs_test.go +++ b/internal/lfs/lfs_test.go @@ -731,7 +731,9 @@ func TestDownloadObjectsSkipsCachedObjectsWhoseURLLapsed(t *testing.T) { } expired, unavailable, failed := downloadObjects(context.Background(), newBatchClient(nil), store, - "http://127.0.0.1:1/repo.git/info/lfs", credentials{}, []batchResponseObject{{ + "http://127.0.0.1:1/repo.git/info/lfs", credentials{}, + []pointer{{oid: oid, size: int64(len(content))}}, + []batchResponseObject{{ OID: oid, Size: int64(len(content)), Actions: map[string]*batchAction{"download": { @@ -800,12 +802,14 @@ func TestDownloadChunkReportsARefusedRefreshPerObject(t *testing.T) { })} fetcher := &Fetcher{client: newBatchClient(client)} - outcome, err := fetcher.downloadChunk(context.Background(), t.TempDir(), endpoint, credentials{}, []batchResponseObject{ - {OID: oid, Size: 7, Actions: map[string]*batchAction{"download": { - Href: endpoint + "/download/" + oid, - ExpiresAt: time.Now().Add(-time.Minute).UTC().Format(time.RFC3339), - }}}, - }) + outcome, err := fetcher.downloadChunk(context.Background(), t.TempDir(), endpoint, credentials{}, + []pointer{{oid: oid, size: 7}}, + []batchResponseObject{ + {OID: oid, Size: 7, Actions: map[string]*batchAction{"download": { + Href: endpoint + "/download/" + oid, + ExpiresAt: time.Now().Add(-time.Minute).UTC().Format(time.RFC3339), + }}}, + }) if batches != 1 { t.Errorf("batch requests = %d, want one asking for a fresh URL", batches) @@ -830,6 +834,48 @@ func TestDownloadChunkReportsARefusedRefreshPerObject(t *testing.T) { } } +// TestDownloadObjectsRefusesMalformedAndUnrequestedOIDs covers an endpoint whose +// response names objects the client never asked for. The OID becomes a path +// component and is sliced for the shard directories, so a malformed one has to be +// refused before either happens: a short OID would panic the slice and cost the +// whole backup, and a crafted one would name a path outside the store. +func TestDownloadObjectsRefusesMalformedAndUnrequestedOIDs(t *testing.T) { + wanted := strings.Repeat("a", 64) + unrelated := strings.Repeat("b", 64) + downloads := 0 + client := &http.Client{Transport: roundTripFunc(func(*http.Request) (*http.Response, error) { + downloads++ + return &http.Response{StatusCode: http.StatusOK, Body: io.NopCloser(strings.NewReader("")), Header: make(http.Header)}, nil + })} + + expired, unavailable, failed := downloadObjects(context.Background(), newBatchClient(client), t.TempDir(), + "http://127.0.0.1:1/repo.git/info/lfs", credentials{}, + []pointer{{oid: wanted, size: 7}}, + []batchResponseObject{ + {OID: "ab", Size: 7, Actions: map[string]*batchAction{"download": {Href: "http://127.0.0.1:1/download/ab"}}}, + {OID: "sha/../escape", Size: 7, Actions: map[string]*batchAction{"download": {Href: "http://127.0.0.1:1/download/escape"}}}, + {OID: unrelated, Size: 7, Actions: map[string]*batchAction{"download": {Href: "http://127.0.0.1:1/download/" + unrelated}}}, + }) + + if len(expired) != 0 || len(failed) != 0 { + t.Errorf("expired = %v, failed = %v, want the bad objects refused without a transfer", expired, failed) + } + if downloads != 0 { + t.Errorf("downloads = %d, want nothing fetched for objects the client did not ask for", downloads) + } + if len(unavailable) != 3 { + t.Fatalf("unavailable = %d, want every bad object reported rather than acted on", len(unavailable)) + } + for _, object := range unavailable { + if !errors.Is(object, errObjectUnavailable) { + t.Errorf("object error = %v, want it recorded as unserved", object) + } + } + if !errors.Is(unavailable[2], errObjectUnavailable) || !strings.Contains(unavailable[2].Error(), "not requested") { + t.Errorf("third error = %v, want the unrequested object reported as such", unavailable[2]) + } +} + func TestFetchAllUnauthorizedIsNotDisabled(t *testing.T) { oid, pointerText := pointerFor([]byte("content")) lfsServer := newFakeLFSServer(t, map[string][]byte{oid: []byte("content")}) From 9e3f1c3a9bef7b470519105f72199de3b2bded74 Mon Sep 17 00:00:00 2001 From: Neureka Date: Thu, 10 Sep 2026 16:24:34 -0700 Subject: [PATCH 13/15] fix(lfs): fold response OIDs to the store's spelling and trust an unstated size MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two ways the new batch-response checks could fail content that is already mirrored: - The requested-object check compared the response OID with the pointer's OID byte for byte, while the digest check beside it accepts upper case. The OID is a path component and the store is keyed by it, so an endpoint echoing a valid digest in upper case would be written to a second directory, miss the cache it should hit, and be reported as an object that was never requested. Response OIDs are now folded to lower case, which is the spelling the pointers and the store already use. - The cached-object check treated the endpoint's size as authoritative, so an endpoint that omits it — reporting zero — made content the store already holds look stale. Re-downloading it is wasteful at best and, for an object whose scheduled URL has lapsed, escalates into failing a backup whose LFS content is complete. A size of zero now leaves the length unstated rather than contradicted, so the cache is trusted; a size the endpoint does state is still enforced, and a file of the wrong length falls through to a verified download. Coverage: an upper-case digested response matching cached content, and an endpoint that omits the size, including that a stated size is still enforced. --- internal/lfs/batch.go | 42 ++++++++++------ internal/lfs/lfs_test.go | 102 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+), 16 deletions(-) diff --git a/internal/lfs/batch.go b/internal/lfs/batch.go index 27b7df6..a6026b4 100644 --- a/internal/lfs/batch.go +++ b/internal/lfs/batch.go @@ -13,6 +13,7 @@ import ( "net/url" "os" "path/filepath" + "strings" "time" ) @@ -338,38 +339,47 @@ func downloadObjects( for _, pointer := range want { requested[pointer.oid] = true } - answered := func(object batchResponseObject) *batchObjectError { + // answered also returns the canonical spelling of the object's OID: the + // pointer's OIDs are lowercase, while an endpoint may echo a valid digest in + // upper case. The OID is a path component and the store is keyed by it, so + // the response is folded to the spelling the store already uses. + answered := func(object batchResponseObject) (string, *batchObjectError) { switch { case object.Error != nil: - return &batchObjectError{oid: object.OID, message: object.Error.Message} + return "", &batchObjectError{oid: object.OID, message: object.Error.Message} case object.Actions["download"] == nil || object.Actions["download"].Href == "": // The server knows the object but scheduled nothing; without an // href there is nothing this client can do beyond reporting it. - return &batchObjectError{oid: object.OID, message: "no download action was scheduled"} + return "", &batchObjectError{oid: object.OID, message: "no download action was scheduled"} case !isSHA256Hex(object.OID): - return &batchObjectError{oid: shortOID(object.OID), message: "the endpoint described an object whose oid is not a sha256 digest"} - case !requested[object.OID]: - return &batchObjectError{oid: shortOID(object.OID), message: "the endpoint described an object that was not requested"} - default: - return nil + return "", &batchObjectError{oid: shortOID(object.OID), message: "the endpoint described an object whose oid is not a sha256 digest"} } + oid := strings.ToLower(object.OID) + if !requested[oid] { + return "", &batchObjectError{oid: shortOID(object.OID), message: "the endpoint described an object that was not requested"} + } + return oid, nil } for _, object := range objects { if err := ctx.Err(); err != nil { return expired, unavailable, append(failed, err) } - if refused := answered(object); refused != nil { + oid, refused := answered(object) + if refused != nil { unavailable = append(unavailable, refused) continue } - if cached, err := os.Stat(filepath.Join(store, object.OID[0:2], object.OID[2:4], object.OID)); err == nil && - cached.Mode().IsRegular() && cached.Size() == object.Size { + if cached, err := os.Stat(filepath.Join(store, oid[0:2], oid[2:4], oid)); err == nil && + cached.Mode().IsRegular() && (object.Size == 0 || cached.Size() == object.Size) { // Already cached by a previous snapshot; git-lfs also skips it, and // content already held needs no fresh URL, so this is settled before - // the expiry below can send it back for rescheduling. A file of the - // wrong length is not this object, so it falls through to the - // download, which verifies the digest as the bytes arrive. + // the expiry below can send it back for rescheduling. A file of a + // length the endpoint contradicts is not this object, so it falls + // through to the download, which verifies the digest as bytes + // arrive. An endpoint that reports no size leaves the length + // unstated rather than contradicted, and re-downloading content the + // store already holds would risk failing over it. continue } action := object.Actions["download"] @@ -377,10 +387,10 @@ func downloadObjects( // The URL lapsed before it was used — a long scan, a slow batch, a // small expires_in — so the object needs a fresh one rather than a // transfer that can only fail. - expired = append(expired, pointer{oid: object.OID, size: object.Size}) + expired = append(expired, pointer{oid: oid, size: object.Size}) continue } - if err := downloadObject(ctx, client.client, store, endpointHost, creds, object.OID, action); err != nil { + if err := downloadObject(ctx, client.client, store, endpointHost, creds, oid, action); err != nil { failed = append(failed, err) } } diff --git a/internal/lfs/lfs_test.go b/internal/lfs/lfs_test.go index 495c8bb..239ed40 100644 --- a/internal/lfs/lfs_test.go +++ b/internal/lfs/lfs_test.go @@ -876,6 +876,108 @@ func TestDownloadObjectsRefusesMalformedAndUnrequestedOIDs(t *testing.T) { } } +// TestDownloadObjectsAcceptsAnUppercaseOID covers an endpoint that echoes a valid +// digest in upper case. The OID is a path component and the store is keyed by it, +// so the response has to fold to the spelling the store uses: taken literally it +// would be written to a second directory, miss the cache it should hit, and be +// rejected as an object that was never requested. +func TestDownloadObjectsAcceptsAnUppercaseOID(t *testing.T) { + content := []byte("content") + oid, _ := pointerFor(content) + + store := t.TempDir() + cached := filepath.Join(store, oid[0:2], oid[2:4]) + if err := os.MkdirAll(cached, 0o755); err != nil { + t.Fatalf("prepare the store: %v", err) + } + if err := os.WriteFile(filepath.Join(cached, oid), content, 0o644); err != nil { + t.Fatalf("place the cached object: %v", err) + } + + downloads := 0 + client := &http.Client{Transport: roundTripFunc(func(*http.Request) (*http.Response, error) { + downloads++ + return &http.Response{StatusCode: http.StatusOK, Body: io.NopCloser(strings.NewReader("")), Header: make(http.Header)}, nil + })} + + expired, unavailable, failed := downloadObjects(context.Background(), newBatchClient(client), store, + "http://127.0.0.1:1/repo.git/info/lfs", credentials{}, + []pointer{{oid: oid, size: int64(len(content))}}, + []batchResponseObject{{ + OID: strings.ToUpper(oid), + Size: int64(len(content)), + Actions: map[string]*batchAction{"download": {Href: "http://127.0.0.1:1/download/" + oid}}, + }}) + + if len(expired) != 0 || len(unavailable) != 0 || len(failed) != 0 { + t.Errorf("expired = %v, unavailable = %v, failed = %v, want the uppercase digest accepted", expired, unavailable, failed) + } + if downloads != 0 { + t.Errorf("downloads = %d, want the cached object recognised rather than fetched again", downloads) + } +} + +// TestDownloadObjectsTrustsTheCacheWhenTheEndpointReportsNoSize covers an +// endpoint that omits the object's size. The length is then unstated rather than +// contradicted, so content the store already holds must not be re-downloaded — +// with a lapsed URL that would escalate into failing a backup whose LFS content +// is complete. +func TestDownloadObjectsTrustsTheCacheWhenTheEndpointReportsNoSize(t *testing.T) { + content := []byte("content") + oid, _ := pointerFor(content) + + store := t.TempDir() + cached := filepath.Join(store, oid[0:2], oid[2:4]) + if err := os.MkdirAll(cached, 0o755); err != nil { + t.Fatalf("prepare the store: %v", err) + } + if err := os.WriteFile(filepath.Join(cached, oid), content, 0o644); err != nil { + t.Fatalf("place the cached object: %v", err) + } + + expired, unavailable, failed := downloadObjects(context.Background(), newBatchClient(nil), store, + "http://127.0.0.1:1/repo.git/info/lfs", credentials{}, + []pointer{{oid: oid, size: int64(len(content))}}, + []batchResponseObject{{ + OID: oid, + Size: 0, + Actions: map[string]*batchAction{"download": { + Href: "http://127.0.0.1:1/download/" + oid, + ExpiresAt: time.Now().Add(-time.Minute).UTC().Format(time.RFC3339), + }}, + }}) + + if len(expired) != 0 { + t.Errorf("expired = %v, want cached content not rescheduled over an unstated size", expired) + } + if len(unavailable) != 0 || len(failed) != 0 { + t.Errorf("unavailable = %v, failed = %v, want cached content left alone", unavailable, failed) + } + + // A size the endpoint does state has to be honoured, so a partial file is + // not mistaken for the object. + shortStore := t.TempDir() + if err := os.MkdirAll(filepath.Join(shortStore, oid[0:2], oid[2:4]), 0o755); err != nil { + t.Fatalf("prepare the store: %v", err) + } + if err := os.WriteFile(filepath.Join(shortStore, oid[0:2], oid[2:4], oid), content[:3], 0o644); err != nil { + t.Fatalf("place the truncated object: %v", err) + } + _, _, truncatedFailed := downloadObjects(context.Background(), newBatchClient(nil), shortStore, + "http://127.0.0.1:1/repo.git/info/lfs", credentials{}, + []pointer{{oid: oid, size: int64(len(content))}}, + []batchResponseObject{{ + OID: oid, + Size: int64(len(content)), + Actions: map[string]*batchAction{"download": { + Href: "http://127.0.0.1:1/download/" + oid, + }}, + }}) + if len(truncatedFailed) == 0 { + t.Error("a cached file the endpoint's size contradicts should fall through to a download") + } +} + func TestFetchAllUnauthorizedIsNotDisabled(t *testing.T) { oid, pointerText := pointerFor([]byte("content")) lfsServer := newFakeLFSServer(t, map[string][]byte{oid: []byte("content")}) From 24a87de6318159bfe4823a57fea804882eb11d68 Mon Sep 17 00:00:00 2001 From: Neureka Date: Thu, 10 Sep 2026 16:33:15 -0700 Subject: [PATCH 14/15] fix(lfs): take the cached object's length from the pointer file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Treating a size of zero as "unstated" left the length unchecked whenever the endpoint omitted it, so any regular file at the object's path counted as mirrored. A truncated leftover or an interrupted copy would then be reported as a complete backup while the real content was missing — the failure this whole package exists to prevent, reached by trusting the wrong field. The pointer file is what the repository records about the object, and it is already in hand as the pointer being fetched, so its size is now authoritative; the endpoint's stands in only when the pointer states none. A file whose length contradicts the recorded size falls through to a download that verifies the digest, and only a length that is unknown on both sides is left unchecked. Coverage: a truncated cached file behind an endpoint that reports no size, which the previous check accepted. --- internal/lfs/batch.go | 21 +++++++++++++++------ internal/lfs/lfs_test.go | 17 +++++++++++++++++ 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/internal/lfs/batch.go b/internal/lfs/batch.go index a6026b4..8a8062e 100644 --- a/internal/lfs/batch.go +++ b/internal/lfs/batch.go @@ -336,8 +336,10 @@ func downloadObjects( // a SHA-256 digest — is refused before any of that: a short OID would panic // the slice, and a crafted one could name a path outside the store. requested := make(map[string]bool, len(want)) + wantSize := make(map[string]int64, len(want)) for _, pointer := range want { requested[pointer.oid] = true + wantSize[pointer.oid] = pointer.size } // answered also returns the canonical spelling of the object's OID: the // pointer's OIDs are lowercase, while an endpoint may echo a valid digest in @@ -370,16 +372,23 @@ func downloadObjects( unavailable = append(unavailable, refused) continue } + // The pointer file's own size is authoritative, since it is what the + // repository records about the object; the endpoint's stands in only + // when the pointer did not state one. + size := object.Size + if wanted, known := wantSize[oid]; known && wanted > 0 { + size = wanted + } if cached, err := os.Stat(filepath.Join(store, oid[0:2], oid[2:4], oid)); err == nil && - cached.Mode().IsRegular() && (object.Size == 0 || cached.Size() == object.Size) { + cached.Mode().IsRegular() && (size == 0 || cached.Size() == size) { // Already cached by a previous snapshot; git-lfs also skips it, and // content already held needs no fresh URL, so this is settled before // the expiry below can send it back for rescheduling. A file of a - // length the endpoint contradicts is not this object, so it falls - // through to the download, which verifies the digest as bytes - // arrive. An endpoint that reports no size leaves the length - // unstated rather than contradicted, and re-downloading content the - // store already holds would risk failing over it. + // length the recorded size contradicts is not this object, so it + // falls through to the download, which verifies the digest as bytes + // arrive. No size anywhere leaves the length unstated rather than + // contradicted, and re-downloading content the store already holds + // would risk failing over it. continue } action := object.Actions["download"] diff --git a/internal/lfs/lfs_test.go b/internal/lfs/lfs_test.go index 239ed40..09fee63 100644 --- a/internal/lfs/lfs_test.go +++ b/internal/lfs/lfs_test.go @@ -976,6 +976,23 @@ func TestDownloadObjectsTrustsTheCacheWhenTheEndpointReportsNoSize(t *testing.T) if len(truncatedFailed) == 0 { t.Error("a cached file the endpoint's size contradicts should fall through to a download") } + + // The pointer file's size is authoritative, so a truncated file is not the + // object even when the endpoint states no size at all — the case that would + // otherwise let an interrupted copy be counted as mirrored. + _, _, unstatedFailed := downloadObjects(context.Background(), newBatchClient(nil), shortStore, + "http://127.0.0.1:1/repo.git/info/lfs", credentials{}, + []pointer{{oid: oid, size: int64(len(content))}}, + []batchResponseObject{{ + OID: oid, + Size: 0, + Actions: map[string]*batchAction{"download": { + Href: "http://127.0.0.1:1/download/" + oid, + }}, + }}) + if len(unstatedFailed) == 0 { + t.Error("a cached file the pointer's size contradicts should fall through to a download") + } } func TestFetchAllUnauthorizedIsNotDisabled(t *testing.T) { From 565d2d0f400b8d0e863b8aa861946bb1ac537194 Mon Sep 17 00:00:00 2001 From: Neureka Date: Thu, 10 Sep 2026 16:39:19 -0700 Subject: [PATCH 15/15] fix(lfs): carry an object's resolved size into its rescheduling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An object whose scheduled URL had lapsed was rescheduled with the endpoint's size rather than the resolved one, so the pointer's authoritative length was dropped at exactly the point it was needed. For an endpoint that states no size, the refresh request asked about a zero-length object and the pass over its answer saw a zero size, which the cache predicate reads as unstated — accepting any cached file, truncation included, and reporting an incomplete mirror as complete. The resolved size now travels with the rescheduled pointer, so both the refresh request and the pass over its response keep the length the pointer records. Coverage: a lapsed object on an endpoint that states no size, asserting the rescheduled pointer carries the pointer file's size rather than zero. --- internal/lfs/batch.go | 7 +++++-- internal/lfs/lfs_test.go | 22 ++++++++++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/internal/lfs/batch.go b/internal/lfs/batch.go index 8a8062e..a08f62e 100644 --- a/internal/lfs/batch.go +++ b/internal/lfs/batch.go @@ -395,8 +395,11 @@ func downloadObjects( if action.expired() { // The URL lapsed before it was used — a long scan, a slow batch, a // small expires_in — so the object needs a fresh one rather than a - // transfer that can only fail. - expired = append(expired, pointer{oid: oid, size: object.Size}) + // transfer that can only fail. The resolved size travels with it, so + // the refresh request and the pass over its answer keep the length + // the pointer records rather than falling back to an endpoint that + // stated none. + expired = append(expired, pointer{oid: oid, size: size}) continue } if err := downloadObject(ctx, client.client, store, endpointHost, creds, oid, action); err != nil { diff --git a/internal/lfs/lfs_test.go b/internal/lfs/lfs_test.go index 09fee63..66fee61 100644 --- a/internal/lfs/lfs_test.go +++ b/internal/lfs/lfs_test.go @@ -993,6 +993,28 @@ func TestDownloadObjectsTrustsTheCacheWhenTheEndpointReportsNoSize(t *testing.T) if len(unstatedFailed) == 0 { t.Error("a cached file the pointer's size contradicts should fall through to a download") } + + // The resolved size has to travel with an object that is rescheduled, so the + // refresh request and the pass over its answer keep the pointer's length + // instead of falling back to an endpoint that stated none. + expiredPointers, _, _ := downloadObjects(context.Background(), newBatchClient(nil), shortStore, + "http://127.0.0.1:1/repo.git/info/lfs", credentials{}, + []pointer{{oid: oid, size: int64(len(content))}}, + []batchResponseObject{{ + OID: oid, + Size: 0, + Actions: map[string]*batchAction{"download": { + Href: "http://127.0.0.1:1/download/" + oid, + ExpiresAt: time.Now().Add(-time.Minute).UTC().Format(time.RFC3339), + }}, + }}) + if len(expiredPointers) != 1 { + t.Fatalf("expired = %v, want the lapsed object rescheduled", expiredPointers) + } + if expiredPointers[0].size != int64(len(content)) { + t.Errorf("rescheduled size = %d, want the pointer's %d rather than the endpoint's unstated 0", + expiredPointers[0].size, len(content)) + } } func TestFetchAllUnauthorizedIsNotDisabled(t *testing.T) {