Write npm dist-tags only after the manifest lands

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

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
 }

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)
 		}

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)