Skip to content

fix: serve the moved tag when the pinned commit is gone and a hash is recorded #85

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
traefiker merged 1 commit into from
Oct 27, 2026
Merged

fix: serve the moved tag when the pinned commit is gone and a hash is recorded #85

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 12 additions & 10 deletions pkg/handlers/module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,25 +236,27 @@ func (h Handlers) downloadGitHub(ctx context.Context, moduleName, version string

logger := log.With().Str("module_name", moduleName).Str("module_version", version).Logger()

request, err := h.getArchiveLinkRequest(ctxDownload, moduleName, version)
if err != nil {
span.RecordError(err)
logger.Error().Err(err).Msg("Failed to get archive link")
_, errHash := h.store.GetHashByName(ctxDownload, moduleName, version)
if errHash != nil && !errors.As(errHash, &db.NotFoundError{}) {
span.RecordError(errHash)
logger.Error().Err(errHash).Msg("Failed to get plugin hash")
JSONErrorf(rw, http.StatusInternalServerError, "Failed to get plugin %s@%s", moduleName, version)

return
}

_, err = h.store.GetHashByName(ctxDownload, moduleName, version)
if err != nil && !errors.As(err, &db.NotFoundError{}) {
recorded := errHash == nil

request, err := h.getArchiveLinkRequest(ctxDownload, moduleName, version, recorded)
if err != nil {
span.RecordError(err)
logger.Error().Err(err).Msg("Failed to get plugin hash")
logger.Error().Err(err).Msg("Failed to get archive link")
JSONErrorf(rw, http.StatusInternalServerError, "Failed to get plugin %s@%s", moduleName, version)

return
}

if err == nil {
if recorded {
_, err = h.gh.Do(ctxDownload, request, rw)
if err != nil {
span.RecordError(err)
Expand Down Expand Up @@ -479,14 +481,14 @@ func (h Handlers) downloadGitHubFromAssets(ctx context.Context, moduleName, vers
}
}

func (h Handlers) getArchiveLinkRequest(ctx context.Context, moduleName, version string) (*http.Request, error) {
func (h Handlers) getArchiveLinkRequest(ctx context.Context, moduleName, version string, recorded bool) (*http.Request, error) {
ctx, span := h.tracer.Start(ctx, "handler_getArchiveLinkRequest")
defer span.End()

owner, repoName := path.Split(strings.TrimPrefix(moduleName, "github.com/"))
owner = strings.TrimSuffix(owner, "/")

ref, err := h.archiveRef(ctx, owner, repoName, moduleName, version)
ref, err := h.archiveRef(ctx, owner, repoName, moduleName, version, recorded)
if err != nil {
span.RecordError(err)

Expand Down
54 changes: 45 additions & 9 deletions pkg/handlers/pinned.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ var errNotPinned = errors.New("no pinned commit")
// The pinned commit of a moved annotated tag cannot give those bytes back, nor match a hash recorded after the move:
// those hashes are realigned with `piceus realign --only`, reviewed one by one.
// Without an answer from the Go module proxy, it falls back to the tag, as before pinning.
func (h Handlers) archiveRef(ctx context.Context, owner, repoName, moduleName, version string) (string, error) {
// When GitHub no longer has the pinned commit, the moved tag is served only if a hash is recorded:
// validate then rejects any other bytes, and no hash is ever recorded from code the proxy did not pin.
func (h Handlers) archiveRef(ctx context.Context, owner, repoName, moduleName, version string, recorded bool) (string, error) {
if h.pinProxyURL == "" {
return version, nil
}
Expand Down Expand Up @@ -52,19 +54,53 @@ func (h Handlers) archiveRef(ctx context.Context, owner, repoName, moduleName, v
case unchanged:
return version, nil
case moved:
log.Info().Str("module_name", moduleName).Str("module_version", version).
Str("tag_commit", sha).Str("pinned_commit", pinned).
Msg("Tag moved after publication, serving the pinned commit")
return h.refAfterTagChange(ctx, owner, repoName, moduleName, version, pinned, sha, recorded)
case deleted:
return h.refAfterTagChange(ctx, owner, repoName, moduleName, version, pinned, "", recorded)
default:
return "", fmt.Errorf("resolve tag %s: %w", version, err)
}
}

// refAfterTagChange returns the ref to serve once the tag moved to tagCommit, or was deleted when tagCommit is empty.
func (h Handlers) refAfterTagChange(ctx context.Context, owner, repoName, moduleName, version, pinned, tagCommit string, recorded bool) (string, error) {
logger := log.With().Str("module_name", moduleName).Str("module_version", version).Str("pinned_commit", pinned).Logger()

gone, err := h.commitGone(ctx, owner, repoName, pinned)
if err != nil {
return "", err
}

switch {
case !gone && tagCommit != "":
logger.Info().Str("tag_commit", tagCommit).Msg("Tag moved after publication, serving the pinned commit")

return pinned, nil
case deleted:
log.Info().Str("module_name", moduleName).Str("module_version", version).
Str("pinned_commit", pinned).Int("status", resp.StatusCode).
Msg("Tag not found, serving the pinned commit")
case !gone:
logger.Info().Msg("Tag not found, serving the pinned commit")

return pinned, nil
case tagCommit != "" && recorded:
logger.Warn().Str("tag_commit", tagCommit).Msg("Pinned commit gone from GitHub, serving the moved tag, checked against the recorded hash")

return version, nil
default:
return "", fmt.Errorf("resolve tag %s: %w", version, err)
return "", fmt.Errorf("pinned commit %s of %s@%s is gone from GitHub", pinned, moduleName, version)
}
}

// commitGone reports whether GitHub no longer has the commit sha.
func (h Handlers) commitGone(ctx context.Context, owner, repoName, sha string) (bool, error) {
// A conditional request answered 304 is not counted in the GitHub rate limit.
_, resp, err := h.gh.Repositories.GetCommitSHA1(ctx, owner, repoName, sha, sha)

switch {
case err == nil, resp != nil && resp.StatusCode == http.StatusNotModified:
return false, nil
case resp != nil && (resp.StatusCode == http.StatusNotFound || resp.StatusCode == http.StatusUnprocessableEntity):
return true, nil
default:
return false, fmt.Errorf("check commit %s: %w", sha, err)
}
}

Expand Down
57 changes: 41 additions & 16 deletions pkg/handlers/pinned_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func TestArchiveRef(t *testing.T) {
rw.WriteHeader(http.StatusGone)
})

for _, repo := range []string{"stable", "moved", "deleted", "notfound", "ghdown"} {
for _, repo := range []string{"stable", "moved", "deleted", "notfound", "ghdown", "movedgone", "deletedgone", "checkdown"} {
proxy.HandleFunc("/github.com/o/"+repo+"/@v/v1.0.0.info", func(rw http.ResponseWriter, _ *http.Request) {
_, _ = fmt.Fprint(rw, `{"Version":"v1.0.0","Origin":{"VCS":"git","Hash":"pinned"}}`)
})
Expand All @@ -49,18 +49,30 @@ func TestArchiveRef(t *testing.T) {
}
}

status := func(code int) http.HandlerFunc {
return func(rw http.ResponseWriter, _ *http.Request) {
rw.WriteHeader(code)
}
}

gh := http.NewServeMux()
gh.HandleFunc("/repos/o/stable/commits/v1.0.0", commit("pinned"))
gh.HandleFunc("/repos/o/moved/commits/v1.0.0", commit("other"))
gh.HandleFunc("/repos/o/deleted/commits/v1.0.0", func(rw http.ResponseWriter, _ *http.Request) {
rw.WriteHeader(http.StatusUnprocessableEntity)
})
gh.HandleFunc("/repos/o/notfound/commits/v1.0.0", func(rw http.ResponseWriter, _ *http.Request) {
rw.WriteHeader(http.StatusNotFound)
})
gh.HandleFunc("/repos/o/ghdown/commits/v1.0.0", func(rw http.ResponseWriter, _ *http.Request) {
rw.WriteHeader(http.StatusServiceUnavailable)
})
gh.HandleFunc("/repos/o/deleted/commits/v1.0.0", status(http.StatusUnprocessableEntity))
gh.HandleFunc("/repos/o/notfound/commits/v1.0.0", status(http.StatusNotFound))
gh.HandleFunc("/repos/o/ghdown/commits/v1.0.0", status(http.StatusServiceUnavailable))
gh.HandleFunc("/repos/o/movedgone/commits/v1.0.0", commit("other"))
gh.HandleFunc("/repos/o/deletedgone/commits/v1.0.0", status(http.StatusUnprocessableEntity))
gh.HandleFunc("/repos/o/checkdown/commits/v1.0.0", commit("other"))

// The pinned commit, still on GitHub or gone.
for _, repo := range []string{"moved", "deleted", "notfound"} {
gh.HandleFunc("/repos/o/"+repo+"/commits/pinned", commit("pinned"))
}

gh.HandleFunc("/repos/o/movedgone/commits/pinned", status(http.StatusUnprocessableEntity))
gh.HandleFunc("/repos/o/deletedgone/commits/pinned", status(http.StatusUnprocessableEntity))
gh.HandleFunc("/repos/o/checkdown/commits/pinned", status(http.StatusServiceUnavailable))

ghSrv := httptest.NewServer(gh)
t.Cleanup(ghSrv.Close)
Expand All @@ -71,9 +83,11 @@ func TestArchiveRef(t *testing.T) {
h := New(nil, nil, ghClient, proxySrv.URL)

testCases := []struct {
repo string
ref string
wantErr bool
desc string
repo string
recorded bool
ref string
wantErr bool
}{
{repo: "unknown", ref: "v1.0.0"},
{repo: "noorigin", ref: "v1.0.0"},
Expand All @@ -85,11 +99,22 @@ func TestArchiveRef(t *testing.T) {
{repo: "proxydown", ref: "v1.0.0"},
{repo: "proxygone", ref: "v1.0.0"},
{repo: "Bad!Path", ref: "v1.0.0"},
// validate checks the moved tag against the recorded hash.
{desc: "movedgone recorded", repo: "movedgone", recorded: true, ref: "v1.0.0"},
// Serving the moved tag would record a hash from code the proxy did not pin.
{desc: "movedgone not recorded", repo: "movedgone", wantErr: true},
{desc: "deletedgone recorded", repo: "deletedgone", recorded: true, wantErr: true},
{repo: "checkdown", recorded: true, wantErr: true},
}

for _, test := range testCases {
t.Run(test.repo, func(t *testing.T) {
ref, err := h.archiveRef(t.Context(), "o", test.repo, "github.com/o/"+test.repo, "v1.0.0")
desc := test.desc
if desc == "" {
desc = test.repo
}

t.Run(desc, func(t *testing.T) {
ref, err := h.archiveRef(t.Context(), "o", test.repo, "github.com/o/"+test.repo, "v1.0.0", test.recorded)

if test.wantErr {
require.Error(t, err)
Expand Down Expand Up @@ -125,7 +150,7 @@ func TestPinnedCommit_cached(t *testing.T) {
func TestArchiveRef_noPinProxy(t *testing.T) {
h := New(nil, nil, github.NewClient(nil), "")

ref, err := h.archiveRef(t.Context(), "o", "moved", "github.com/o/moved", "v1.0.0")
ref, err := h.archiveRef(t.Context(), "o", "moved", "github.com/o/moved", "v1.0.0", false)
require.NoError(t, err)

assert.Equal(t, "v1.0.0", ref)
Expand Down
Loading