Reject bare * and unknown remotes on evict, serialise with fetch lock

- return 400 for a bare * and 404 for an unknown remote
- take the per-path fetch lock for single-path evicts
- pass the evictor to Routes so local routes no longer carry it
- test If-None-Match revalidation, ForgetPath/ForgetPrefix glob escaping

internal/api/v2/local_evict_cleanup_test.go

@@ -49,7 +49,7 @@ func TestLocalEvictCleansRPMMetadata(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	h := NewObjectsHandler(db, nil)
+	h := NewObjectsHandler(db)
 	router := chi.NewRouter()
 	router.Route("/locals/{name}/objects", func(r chi.Router) {
 		r.Delete("/*", h.LocalRoutes().ServeHTTP)

internal/api/v2/local_objects_test.go

@@ -40,7 +40,7 @@ func TestLocalObjectsListing(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	h := NewObjectsHandler(db, nil)
+	h := NewObjectsHandler(db)
 	router := chi.NewRouter()
 	router.Route("/locals/{name}/objects", func(r chi.Router) {
 		r.Get("/", h.LocalRoutes().ServeHTTP)

internal/api/v2/objects.go

@@ -2,6 +2,7 @@ package v2
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net/http"
 	"strconv"
@@ -9,6 +10,7 @@ import (
 	"github.com/go-chi/chi/v5"
 
 	"git.unkin.net/unkin/artifactapi/internal/database"
+	"git.unkin.net/unkin/artifactapi/internal/proxy"
 )
 
 // Evictor drops a remote path from every cache layer.
@@ -17,18 +19,20 @@ type Evictor interface {
 }
 
 type ObjectsHandler struct {
-	db      *database.DB
-	evictor Evictor
+	db *database.DB
 }
 
-func NewObjectsHandler(db *database.DB, evictor Evictor) *ObjectsHandler {
-	return &ObjectsHandler{db: db, evictor: evictor}
+func NewObjectsHandler(db *database.DB) *ObjectsHandler {
+	return &ObjectsHandler{db: db}
 }
 
-func (h *ObjectsHandler) Routes() chi.Router {
+// Routes lists and evicts objects for remote repos; evictor serves the DELETE.
+func (h *ObjectsHandler) Routes(evictor Evictor) chi.Router {
 	r := chi.NewRouter()
 	r.Get("/", h.list)
-	r.Delete("/*", h.evict)
+	r.Delete("/*", func(w http.ResponseWriter, r *http.Request) {
+		evict(w, r, evictor)
+	})
 	return r
 }
 
@@ -89,11 +93,16 @@ func (h *ObjectsHandler) evictLocal(w http.ResponseWriter, r *http.Request) {
 	w.WriteHeader(http.StatusNoContent)
 }
 
-func (h *ObjectsHandler) evict(w http.ResponseWriter, r *http.Request) {
+func evict(w http.ResponseWriter, r *http.Request, evictor Evictor) {
 	remoteName := chi.URLParam(r, "name")
 	path := chi.URLParam(r, "*")
 
-	if err := h.evictor.Evict(r.Context(), remoteName, path); err != nil {
+	if err := evictor.Evict(r.Context(), remoteName, path); err != nil {
+		var proxyErr *proxy.ProxyError
+		if errors.As(err, &proxyErr) {
+			http.Error(w, proxyErr.Message, proxyErr.Status)
+			return
+		}
 		http.Error(w, fmt.Sprintf("evict failed: %v", err), http.StatusInternalServerError)
 		return
 	}

internal/api/v2/objects_evict_test.go

@@ -2,31 +2,47 @@ package v2
 
 import (
 	"context"
+	"net/http"
 	"net/http/httptest"
 	"testing"
 
 	"github.com/go-chi/chi/v5"
+
+	"git.unkin.net/unkin/artifactapi/internal/proxy"
 )
 
-type fakeEvictor struct{ remote, path string }
+type fakeEvictor struct {
+	remote, path string
+	err          error
+}
 
 func (f *fakeEvictor) Evict(_ context.Context, remote, path string) error {
 	f.remote, f.path = remote, path
-	return nil
+	return f.err
+}
+
+func deleteObject(ev Evictor, path string) int {
+	router := chi.NewRouter()
+	router.Route("/remotes/{name}/objects", func(r chi.Router) {
+		r.Delete("/*", NewObjectsHandler(nil).Routes(ev).ServeHTTP)
+	})
+	w := httptest.NewRecorder()
+	router.ServeHTTP(w, httptest.NewRequest("DELETE", "/remotes/epel/objects/"+path, nil))
+	return w.Code
 }
 
 func TestRemoteEvictDelegatesToEvictor(t *testing.T) {
 	for _, path := range []string{"8/Everything/x86_64/repodata/repomd.xml", "8/Everything/x86_64/repodata/*"} {
 		ev := &fakeEvictor{}
-		h := NewObjectsHandler(nil, ev)
-		router := chi.NewRouter()
-		router.Route("/remotes/{name}/objects", func(r chi.Router) {
-			r.Delete("/*", h.Routes().ServeHTTP)
-		})
-		w := httptest.NewRecorder()
-		router.ServeHTTP(w, httptest.NewRequest("DELETE", "/remotes/epel/objects/"+path, nil))
-		if w.Code != 204 || ev.remote != "epel" || ev.path != path {
-			t.Errorf("DELETE %s: code=%d evicted=%q/%q", path, w.Code, ev.remote, ev.path)
+		if code := deleteObject(ev, path); code != 204 || ev.remote != "epel" || ev.path != path {
+			t.Errorf("DELETE %s: code=%d evicted=%q/%q", path, code, ev.remote, ev.path)
 		}
 	}
 }
+
+func TestRemoteEvictMapsProxyErrorStatus(t *testing.T) {
+	ev := &fakeEvictor{err: &proxy.ProxyError{Status: http.StatusNotFound, Message: "remote not found"}}
+	if code := deleteObject(ev, "x"); code != http.StatusNotFound {
+		t.Errorf("code = %d, want 404", code)
+	}
+}

internal/cache/cache_test.go

@@ -131,3 +131,72 @@ func TestFlushRemote(t *testing.T) {
 		t.Error("expected keys flushed")
 	}
 }
+
+func setPathKeys(t *testing.T, remote string, paths ...string) {
+	t.Helper()
+	ctx := context.Background()
+	for _, p := range paths {
+		if err := testRedis.SetTTL(ctx, remote, p, time.Minute); err != nil {
+			t.Fatal(err)
+		}
+		if err := testRedis.SetETag(ctx, remote, p, `"e"`, time.Minute); err != nil {
+			t.Fatal(err)
+		}
+	}
+}
+
+func pathKeysExist(t *testing.T, remote, path string) (ttl, etag bool) {
+	t.Helper()
+	ctx := context.Background()
+	ttl, _ = testRedis.CheckTTL(ctx, remote, path)
+	e, _ := testRedis.GetETag(ctx, remote, path)
+	return ttl, e != ""
+}
+
+func TestForgetPath(t *testing.T) {
+	requireRedis(t)
+	const meta = `repo/a*b?[c]\d.xml`
+	setPathKeys(t, "fp", meta, "repo/aXb.xml")
+	setPathKeys(t, "fp-other", meta)
+
+	if err := testRedis.ForgetPath(context.Background(), "fp", meta); err != nil {
+		t.Fatal(err)
+	}
+	if ttl, etag := pathKeysExist(t, "fp", meta); ttl || etag {
+		t.Errorf("forgotten path keys remain: ttl=%v etag=%v", ttl, etag)
+	}
+	for _, k := range [][2]string{{"fp", "repo/aXb.xml"}, {"fp-other", meta}} {
+		if ttl, etag := pathKeysExist(t, k[0], k[1]); !ttl || !etag {
+			t.Errorf("%s:%s lost keys: ttl=%v etag=%v", k[0], k[1], ttl, etag)
+		}
+	}
+}
+
+func TestForgetPrefix(t *testing.T) {
+	requireRedis(t)
+	const prefix = `r*[1]?\/`
+	under := []string{prefix + "repomd.xml", prefix + "sub/x.rpm"}
+	// Each would match the prefix if its glob metacharacters were left unescaped.
+	globMatches := []string{`rX11/repomd.xml`, `r[1]?\/x`, `r*1Z/x`}
+	setPathKeys(t, "fx", append(under, globMatches...)...)
+	setPathKeys(t, "fx-other", under...)
+
+	if err := testRedis.ForgetPrefix(context.Background(), "fx", prefix); err != nil {
+		t.Fatal(err)
+	}
+	for _, p := range under {
+		if ttl, etag := pathKeysExist(t, "fx", p); ttl || etag {
+			t.Errorf("%s keys remain: ttl=%v etag=%v", p, ttl, etag)
+		}
+	}
+	for _, p := range globMatches {
+		if ttl, etag := pathKeysExist(t, "fx", p); !ttl || !etag {
+			t.Errorf("%s outside prefix lost keys: ttl=%v etag=%v", p, ttl, etag)
+		}
+	}
+	for _, p := range under {
+		if ttl, etag := pathKeysExist(t, "fx-other", p); !ttl || !etag {
+			t.Errorf("other remote %s lost keys: ttl=%v etag=%v", p, ttl, etag)
+		}
+	}
+}

internal/proxy/engine.go

@@ -16,6 +16,8 @@ import (
 	"sync/atomic"
 	"time"
 
+	"github.com/jackc/pgx/v5"
+
 	"git.unkin.net/unkin/artifactapi/internal/cache"
 	"git.unkin.net/unkin/artifactapi/internal/database"
 	"git.unkin.net/unkin/artifactapi/internal/provider"
@@ -210,10 +212,21 @@ func (e *Engine) Fetch(ctx context.Context, remote models.Remote, path string, p
 
 // Evict drops path from every cache layer (artifact row, index object, Redis
 // freshness and ETag keys) so the next request refetches from upstream. A
-// trailing "*" evicts every path under that prefix.
+// trailing "*" evicts every path under that prefix; a bare "*" is rejected.
 func (e *Engine) Evict(ctx context.Context, remoteName, path string) error {
 	prefix, wildcard := strings.CutSuffix(path, "*")
+	if wildcard && prefix == "" {
+		return &ProxyError{Status: http.StatusBadRequest, Message: "refusing to evict an entire remote"}
+	}
+	if _, err := e.db.GetRemote(ctx, remoteName); errors.Is(err, pgx.ErrNoRows) {
+		return &ProxyError{Status: http.StatusNotFound, Message: fmt.Sprintf("remote %q not found", remoteName)}
+	} else if err != nil {
+		return fmt.Errorf("get remote: %w", err)
+	}
 	if !wildcard {
+		if e.waitForLock(ctx, remoteName, path) {
+			defer e.cache.ReleaseLock(ctx, remoteName, path)
+		}
 		if err := e.db.DeleteArtifact(ctx, remoteName, path); err != nil {
 			return fmt.Errorf("delete artifact: %w", err)
 		}
@@ -231,6 +244,23 @@ func (e *Engine) Evict(ctx context.Context, remoteName, path string) error {
 	return e.cache.ForgetPrefix(ctx, remoteName, prefix)
 }
 
+// waitForLock takes the per-path fetch lock so an in-flight Fetch cannot
+// re-set TTL/ETag keys after an evict. It gives up once the lock would have
+// expired anyway, or when ctx ends.
+func (e *Engine) waitForLock(ctx context.Context, remoteName, path string) bool {
+	deadline := time.Now().Add(fetchLockTTL)
+	for {
+		if ok, err := e.cache.AcquireLock(ctx, remoteName, path, fetchLockTTL); ok || err != nil || time.Now().After(deadline) {
+			return ok
+		}
+		select {
+		case <-ctx.Done():
+			return false
+		case <-time.After(50 * time.Millisecond):
+		}
+	}
+}
+
 // HeadResult carries artifact metadata for a HEAD request. There is no body.
 type HeadResult struct {
 	ContentType string

internal/proxy/evict_test.go

@@ -2,28 +2,40 @@ package proxy
 
 import (
 	"context"
+	"errors"
 	"net/http"
 	"net/http/httptest"
 	"sync/atomic"
 	"testing"
+	"time"
 
 	_ "git.unkin.net/unkin/artifactapi/internal/provider/rpm"
 	"git.unkin.net/unkin/artifactapi/pkg/models"
 )
 
 // changingUpstream serves every path with the current revision, as a mirror
-// does after a sync replaces its repodata.
-func changingUpstream(t *testing.T) (*httptest.Server, *atomic.Value) {
+// does after a sync replaces its repodata. It answers 304 to a matching
+// If-None-Match and counts conditional requests.
+func changingUpstream(t *testing.T) (*httptest.Server, *atomic.Value, *atomic.Int32) {
 	t.Helper()
 	var rev atomic.Value
+	var conditional atomic.Int32
 	rev.Store("rev1")
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		v := rev.Load().(string)
-		w.Header().Set("ETag", `"`+v+`"`)
+		etag := `"` + v + `"`
+		w.Header().Set("ETag", etag)
+		if inm := r.Header.Get("If-None-Match"); inm != "" {
+			conditional.Add(1)
+			if inm == etag {
+				w.WriteHeader(http.StatusNotModified)
+				return
+			}
+		}
 		_, _ = w.Write([]byte(v + ":" + r.URL.Path))
 	}))
 	t.Cleanup(srv.Close)
-	return srv, &rev
+	return srv, &rev, &conditional
 }
 
 func fetchBody(t *testing.T, r models.Remote, path string) string {
@@ -41,7 +53,7 @@ func rpmRemote(t *testing.T, name, baseURL string) models.Remote {
 
 func TestEvictMutableIndexRefetches(t *testing.T) {
 	requireStack(t)
-	srv, rev := changingUpstream(t)
+	srv, rev, conditional := changingUpstream(t)
 	r := rpmRemote(t, "evict-idx", srv.URL)
 	const path = "8/Everything/x86_64/repodata/repomd.xml"
 
@@ -55,14 +67,64 @@ func TestEvictMutableIndexRefetches(t *testing.T) {
 	if err := testEngine.Evict(context.Background(), r.Name, path); err != nil {
 		t.Fatalf("evict: %v", err)
 	}
+	conditional.Store(0)
 	if got := fetchBody(t, r, path); got != "rev2:/"+path {
 		t.Fatalf("after evict = %q, want rev2", got)
 	}
+	if n := conditional.Load(); n != 0 {
+		t.Errorf("post-evict fetch revalidated with the evicted ETag (%d conditional requests)", n)
+	}
+}
+
+func TestEvictRejectsBareWildcard(t *testing.T) {
+	requireStack(t)
+	srv, _, _ := changingUpstream(t)
+	r := rpmRemote(t, "evict-bare", srv.URL)
+	const path = "8/Everything/x86_64/repodata/repomd.xml"
+	fetchBody(t, r, path)
+
+	var pe *ProxyError
+	if err := testEngine.Evict(context.Background(), r.Name, "*"); !errors.As(err, &pe) || pe.Status != http.StatusBadRequest {
+		t.Fatalf("evict * = %v, want 400", err)
+	}
+	if fresh, _ := testEngine.cache.CheckTTL(context.Background(), r.Name, path); !fresh {
+		t.Error("bare * evicted cached keys")
+	}
+}
+
+func TestEvictUnknownRemote(t *testing.T) {
+	requireStack(t)
+	var pe *ProxyError
+	if err := testEngine.Evict(context.Background(), "evict-no-such-remote", "a/b"); !errors.As(err, &pe) || pe.Status != http.StatusNotFound {
+		t.Fatalf("evict unknown remote = %v, want 404", err)
+	}
+}
+
+func TestEvictWaitsForFetchLock(t *testing.T) {
+	requireStack(t)
+	srv, _, _ := changingUpstream(t)
+	r := rpmRemote(t, "evict-lock", srv.URL)
+	ctx := context.Background()
+	const path = "8/Everything/x86_64/repodata/repomd.xml"
+	if ok, err := testEngine.cache.AcquireLock(ctx, r.Name, path, time.Minute); !ok || err != nil {
+		t.Fatalf("acquire: %v %v", ok, err)
+	}
+	done := make(chan error, 1)
+	go func() { done <- testEngine.Evict(ctx, r.Name, path) }()
+	select {
+	case err := <-done:
+		t.Fatalf("evict returned while fetch lock held: %v", err)
+	case <-time.After(200 * time.Millisecond):
+	}
+	_ = testEngine.cache.ReleaseLock(ctx, r.Name, path)
+	if err := <-done; err != nil {
+		t.Fatalf("evict: %v", err)
+	}
 }
 
 func TestEvictWildcardClearsPrefixOnly(t *testing.T) {
 	requireStack(t)
-	srv, rev := changingUpstream(t)
+	srv, rev, _ := changingUpstream(t)
 	r := rpmRemote(t, "evict-wild", srv.URL)
 	const (
 		repomd = "8/Everything/x86_64/repodata/repomd.xml"

internal/server/server.go

@@ -195,13 +195,13 @@ func (s *Server) routes() chi.Router {
 		r.Mount("/probe", probeHandler.Routes())
 
 		r.Route("/remotes/{name}/objects", func(r chi.Router) {
-			objHandler := v2.NewObjectsHandler(s.db, s.engine)
-			r.Get("/", objHandler.Routes().ServeHTTP)
-			r.Delete("/*", objHandler.Routes().ServeHTTP)
+			objHandler := v2.NewObjectsHandler(s.db)
+			r.Get("/", objHandler.Routes(s.engine).ServeHTTP)
+			r.Delete("/*", objHandler.Routes(s.engine).ServeHTTP)
 		})
 
 		r.Route("/locals/{name}/objects", func(r chi.Router) {
-			objHandler := v2.NewObjectsHandler(s.db, s.engine)
+			objHandler := v2.NewObjectsHandler(s.db)
 			r.Get("/", objHandler.LocalRoutes().ServeHTTP)
 			r.Delete("/*", objHandler.LocalRoutes().ServeHTTP)
 		})