From cea1da59f658dbfe5a8d868d3149be9e4355a77a Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 17 Oct 2026 12:44:39 +1100 Subject: [PATCH] Write npm dist-tags only after the manifest lands --- e2e-docker/npm_local_test.go | 2 +- internal/api/v1/npm.go | 57 ++++++++------- internal/api/v1/npm_handler_test.go | 107 ++++++++++++++++++++++++---- internal/api/v1/npm_test.go | 4 +- 4 files changed, 131 insertions(+), 39 deletions(-) diff --git a/e2e-docker/npm_local_test.go b/e2e-docker/npm_local_test.go index 9455611..30c873d 100644 --- a/e2e-docker/npm_local_test.go +++ b/e2e-docker/npm_local_test.go @@ -36,7 +36,7 @@ mk() { d="/w/src/$1-$2"; mkdir -p "$d"; cd "$d" echo "module.exports = '$1@$2';" > index.js; } mk e2e-npm-plain 1.0.0 && npm publish mk @e2e/npm-scoped 1.0.0 && npm publish -mk e2e-npm-plain 1.0.0 +mk e2e-npm-plain 1.0.0 && echo "// changed" >> index.js if npm publish 2>/dev/null; then echo "republish accepted"; exit 1; fi mkdir -p /w/app && cd /w/app && npm init -y >/dev/null npm install --no-audit --cache /tmp/fresh-cache e2e-npm-plain @e2e/npm-scoped diff --git a/internal/api/v1/npm.go b/internal/api/v1/npm.go index 837b79b..41cc5e1 100644 --- a/internal/api/v1/npm.go +++ b/internal/api/v1/npm.go @@ -175,27 +175,24 @@ func (h *ProxyHandler) handleLocalPut(w http.ResponseWriter, r *http.Request) { base := pkg[strings.LastIndex(pkg, "/")+1:] stem := pkg + "/-/" + base + "-" + rel.version - conflict := func() { - npmError(w, http.StatusConflict, fmt.Sprintf("%s@%s already exists; overwrites are not allowed", pkg, rel.version)) - } - // The manifest gates visibility, so it is written last; a tarball left by a - // failed publish is accepted again only with identical bytes. - hash, err := h.npmStore(r.Context(), remote.Name, stem+".tgz", rel.tarball, "application/octet-stream", false) - if errors.Is(err, database.ErrAlreadyExists) { - existing, getErr := h.db.GetLocalFile(r.Context(), remote.Name, stem+".tgz") - if getErr != nil || existing == nil { - npmError(w, http.StatusInternalServerError, fmt.Sprintf("read existing tarball: %v", getErr)) + // The manifest gates visibility and tags may only name visible versions, so + // the order is tarball, manifest, tags. Identical bytes make retries idempotent. + for _, f := range []struct { + path, ct string + data []byte + }{ + {stem + ".tgz", "application/octet-stream", rel.tarball}, + {stem + ".json", "application/json", rel.manifest}, + } { + ok, err := h.npmStoreImmutable(r.Context(), remote.Name, f.path, f.data, f.ct) + if err != nil { + npmError(w, http.StatusInternalServerError, err.Error()) return } - if existing.ContentHash != hash { - conflict() + if !ok { + npmError(w, http.StatusConflict, fmt.Sprintf("%s@%s already exists; overwrites are not allowed", pkg, rel.version)) return } - err = nil - } - if err != nil { - npmError(w, http.StatusInternalServerError, err.Error()) - return } for _, tag := range rel.tags { if _, err := h.npmStore(r.Context(), remote.Name, pkg+"/-/dist-tags/"+tag, []byte(rel.version), "text/plain", true); err != nil { @@ -203,14 +200,6 @@ func (h *ProxyHandler) handleLocalPut(w http.ResponseWriter, r *http.Request) { return } } - if _, err := h.npmStore(r.Context(), remote.Name, stem+".json", rel.manifest, "application/json", false); err != nil { - if errors.Is(err, database.ErrAlreadyExists) { - conflict() - return - } - npmError(w, http.StatusInternalServerError, err.Error()) - return - } slog.Info("local npm package published", "repo", remote.Name, "package", pkg, "version", rel.version) w.Header().Set("Content-Type", "application/json") @@ -218,6 +207,19 @@ func (h *ProxyHandler) handleLocalPut(w http.ResponseWriter, r *http.Request) { _ = json.NewEncoder(w).Encode(map[string]any{"ok": true, "id": pkg}) } +// npmStoreImmutable reports false when path already holds different bytes. +func (h *ProxyHandler) npmStoreImmutable(ctx context.Context, repo, path string, data []byte, contentType string) (bool, error) { + hash, err := h.npmStore(ctx, repo, path, data, contentType, false) + if !errors.Is(err, database.ErrAlreadyExists) { + return err == nil, err + } + existing, err := h.db.GetLocalFile(ctx, repo, path) + if err != nil || existing == nil { + return false, fmt.Errorf("read existing %s: %v", path, err) + } + return existing.ContentHash == hash, nil +} + func (h *ProxyHandler) npmStore(ctx context.Context, repo, path string, data []byte, contentType string, mutable bool) (string, error) { result, err := h.cas.Store(ctx, bytes.NewReader(data), contentType) if err != nil { @@ -307,5 +309,10 @@ func buildNPMPackument(pkg string, files []database.LocalFile, read func(hash st if len(versions) == 0 { return nil, nil } + for tag, v := range tags { + if _, ok := versions[v]; !ok { + delete(tags, tag) + } + } return map[string]any{"_id": pkg, "name": pkg, "dist-tags": tags, "versions": versions}, nil } diff --git a/internal/api/v1/npm_handler_test.go b/internal/api/v1/npm_handler_test.go index 95e91b8..70821d6 100644 --- a/internal/api/v1/npm_handler_test.go +++ b/internal/api/v1/npm_handler_test.go @@ -108,6 +108,33 @@ func packumentVersions(t *testing.T, url string) map[string]any { return doc.Versions } +func packumentTags(t *testing.T, url string) map[string]string { + t.Helper() + _, body := npmDo(t, http.MethodGet, url, "") + var doc struct { + Tags map[string]string `json:"dist-tags"` + } + if err := json.Unmarshal([]byte(body), &doc); err != nil { + t.Fatal(err) + } + return doc.Tags +} + +func concurrentPuts(t *testing.T, url string, bodies ...string) []int { + t.Helper() + var wg sync.WaitGroup + codes := make([]int, len(bodies)) + for i, b := range bodies { + wg.Add(1) + go func() { + defer wg.Done() + codes[i], _ = npmDo(t, http.MethodPut, url, b) + }() + } + wg.Wait() + return codes +} + func TestNPMPublishHandler(t *testing.T) { h, srv := npmTestServer(t) repo := npmRepo(t, h, models.PackageNPM) @@ -136,8 +163,71 @@ func TestNPMPublishHandler(t *testing.T) { if code, _ := npmDo(t, http.MethodPut, base+"repub", publishBody(t, "repub", []byte("different"), publishOpts{})); code != http.StatusConflict { t.Errorf("different bytes = %d, want 409", code) } - if code, _ := npmDo(t, http.MethodPut, base+"repub", publishBody(t, "repub", []byte("v1"), publishOpts{})); code != http.StatusConflict { - t.Errorf("identical complete republish = %d, want 409", code) + if code, _ := npmDo(t, http.MethodPut, base+"repub", publishBody(t, "repub", []byte("v1"), publishOpts{})); code != http.StatusCreated { + t.Errorf("identical republish = %d, want 201", code) + } + }) + + t.Run("rejected republish leaves tags", func(t *testing.T) { + for _, v := range []string{"1.0.0", "1.1.0"} { + if code, b := npmDo(t, http.MethodPut, base+"rej", publishBody(t, "rej", []byte("tgz "+v), publishOpts{version: v})); code != http.StatusCreated { + t.Fatalf("publish %s = %d: %s", v, code, b) + } + } + if code, _ := npmDo(t, http.MethodPut, base+"rej", publishBody(t, "rej", []byte("tgz 1.0.0"), publishOpts{shasum: "x"})); code != http.StatusConflict { + t.Fatalf("re-PUT 1.0.0 = %d, want 409", code) + } + if tags := packumentTags(t, base+"rej"); tags["latest"] != "1.1.0" { + t.Errorf("latest = %q, want 1.1.0", tags["latest"]) + } + }) + + t.Run("mid-publish window", func(t *testing.T) { + if code, b := npmDo(t, http.MethodPut, base+"mid", publishBody(t, "mid", []byte("v1"), publishOpts{})); code != http.StatusCreated { + t.Fatalf("publish = %d: %s", code, b) + } + ctx := context.Background() + if _, err := h.npmStore(ctx, repo, "mid/-/mid-1.0.1.tgz", []byte("v2"), "application/octet-stream", false); err != nil { + t.Fatal(err) + } + if tags := packumentTags(t, base+"mid"); tags["latest"] != "1.0.0" { + t.Errorf("tarball-only 1.0.1: latest = %q", tags["latest"]) + } + if _, err := h.npmStore(ctx, repo, "mid/-/dist-tags/latest", []byte("1.0.1"), "text/plain", true); err != nil { + t.Fatal(err) + } + if tags := packumentTags(t, base+"mid"); tags["latest"] == "1.0.1" { + t.Error("latest points at invisible 1.0.1") + } + }) + + t.Run("concurrent identical", func(t *testing.T) { + body := publishBody(t, "same", []byte("same tgz"), publishOpts{}) + codes := concurrentPuts(t, base+"same", body, body) + if codes[0] != http.StatusCreated || codes[1] != http.StatusCreated { + t.Fatalf("codes = %v", codes) + } + }) + + t.Run("concurrent different manifests", func(t *testing.T) { + tgz := []byte("race tgz") + codes := concurrentPuts(t, base+"race", + publishBody(t, "race", tgz, publishOpts{shasum: "a", tags: map[string]string{"a": "1.0.0"}}), + publishBody(t, "race", tgz, publishOpts{shasum: "b", tags: map[string]string{"b": "1.0.0"}})) + won := map[int]string{} + for i, c := range codes { + won[c] = []string{"a", "b"}[i] + } + if len(won) != 2 || won[http.StatusCreated] == "" || won[http.StatusConflict] == "" { + t.Fatalf("codes = %v, want one 201 and one 409", codes) + } + v := packumentVersions(t, base+"race")["1.0.0"].(map[string]any) + if got := v["dist"].(map[string]any)["shasum"]; got != won[http.StatusCreated] { + t.Errorf("manifest shasum = %v, winner %s", got, won[http.StatusCreated]) + } + tags := packumentTags(t, base+"race") + if len(tags) != 1 || tags[won[http.StatusCreated]] != "1.0.0" { + t.Errorf("tags = %v, winner %s", tags, won[http.StatusCreated]) } }) @@ -161,16 +251,9 @@ func TestNPMPublishHandler(t *testing.T) { }) t.Run("concurrent versions", func(t *testing.T) { - var wg sync.WaitGroup - codes := make([]int, 2) - for i, v := range []string{"1.0.0", "2.0.0"} { - wg.Add(1) - go func() { - defer wg.Done() - codes[i], _ = npmDo(t, http.MethodPut, base+"conc", publishBody(t, "conc", []byte("tgz "+v), publishOpts{version: v})) - }() - } - wg.Wait() + codes := concurrentPuts(t, base+"conc", + publishBody(t, "conc", []byte("tgz 1.0.0"), publishOpts{version: "1.0.0"}), + publishBody(t, "conc", []byte("tgz 2.0.0"), publishOpts{version: "2.0.0"})) if codes[0] != http.StatusCreated || codes[1] != http.StatusCreated { t.Fatalf("codes = %v", codes) } diff --git a/internal/api/v1/npm_test.go b/internal/api/v1/npm_test.go index bd025e5..82cf676 100644 --- a/internal/api/v1/npm_test.go +++ b/internal/api/v1/npm_test.go @@ -122,6 +122,7 @@ func TestBuildNPMPackument(t *testing.T) { "h1": `{"name":"my_pkg","version":"1.0.0","dist":{"shasum":"aa","tarball":"http://elsewhere/x.tgz"}}`, "h2": `{"name":"my_pkg","version":"1.1.0"}`, "h3": "1.1.0", + "h4": "2.0.0", "hx": `{"name":"myXpkg","version":"6.6.6"}`, } read := func(h string) ([]byte, error) { @@ -133,6 +134,7 @@ func TestBuildNPMPackument(t *testing.T) { } files := []database.LocalFile{ {FilePath: "my_pkg/-/dist-tags/latest", ContentHash: "h3"}, + {FilePath: "my_pkg/-/dist-tags/next", ContentHash: "h4"}, {FilePath: "my_pkg/-/my_pkg-1.0.0.json", ContentHash: "h1"}, {FilePath: "my_pkg/-/my_pkg-1.0.0.tgz", ContentHash: "nope"}, {FilePath: "my_pkg/-/my_pkg-1.1.0.json", ContentHash: "h2"}, @@ -143,7 +145,7 @@ func TestBuildNPMPackument(t *testing.T) { if err != nil { t.Fatal(err) } - if tags := doc["dist-tags"].(map[string]string); tags["latest"] != "1.1.0" { + if tags := doc["dist-tags"].(map[string]string); tags["latest"] != "1.1.0" || len(tags) != 1 { t.Errorf("dist-tags = %v", tags) } versions := doc["versions"].(map[string]any)