Fix mutable path refresh: default mutable_ttl, revalidate after expiry (#151)

A remote created through the API without `mutable_ttl` was stored as 0, so its mutable paths never refreshed. With `check_mutable`, the ETag expired together with the freshness key, so an expired mutable path was always refetched in full and never revalidated.

- default an omitted `mutable_ttl` to 3600 on create; keep the stored value on update; explicit 0 still means never expire
- keep mutable ETags for `max(10 × mutable_ttl, 7d)`, re-armed on each 304, only when `check_mutable` is set and `mutable_ttl` is non-zero
- drop the stored ETag when a refetch returns none; never store one for immutable paths
- flush a remote's Redis keys on delete
- update docs/common/caching.md

Reviewed-on: #151
Co-authored-by: unkin-agent <unkin-agent@unkin.net>
Co-committed-by: unkin-agent <unkin-agent@unkin.net>

internal/api/v2/remotes.go

@@ -67,8 +67,12 @@ func (h *RemotesHandler) get(w http.ResponseWriter, r *http.Request) {
 	writeJSON(w, http.StatusOK, remote)
 }
 
+// defaultMutableTTL matches the remotes.mutable_ttl column default; a create
+// that omits mutable_ttl gets it, an explicit 0 still means never expire.
+const defaultMutableTTL = 3600
+
 func (h *RemotesHandler) create(w http.ResponseWriter, r *http.Request) {
-	var remote models.Remote
+	remote := models.Remote{MutableTTL: defaultMutableTTL}
 	if err := json.NewDecoder(r.Body).Decode(&remote); err != nil {
 		http.Error(w, "invalid json", http.StatusBadRequest)
 		return
@@ -114,7 +118,15 @@ func (h *RemotesHandler) create(w http.ResponseWriter, r *http.Request) {
 
 func (h *RemotesHandler) update(w http.ResponseWriter, r *http.Request) {
 	name := chi.URLParam(r, "name")
+	// Capture the current backend before the update so we can tell whether the
+	// remote's base_url (its upstream) changed. A read failure just means we
+	// skip the freshness flush; it must not block the update.
+	existing, err := h.db.GetRemote(r.Context(), name)
+	oldKnown := err == nil
 	var remote models.Remote
+	if oldKnown {
+		remote.MutableTTL = existing.MutableTTL
+	}
 	if err := json.NewDecoder(r.Body).Decode(&remote); err != nil {
 		http.Error(w, "invalid json", http.StatusBadRequest)
 		return
@@ -132,13 +144,6 @@ func (h *RemotesHandler) update(w http.ResponseWriter, r *http.Request) {
 		http.Error(w, err.Error(), http.StatusBadRequest)
 		return
 	}
-	// Capture the current backend before the update so we can tell whether the
-	// remote's base_url (its upstream) changed. A read failure just means we
-	// skip the freshness flush; it must not block the update.
-	oldBaseURL, oldKnown := "", false
-	if existing, err := h.db.GetRemote(r.Context(), name); err == nil {
-		oldBaseURL, oldKnown = existing.BaseURL, true
-	}
 	if err := h.db.UpdateRemote(r.Context(), &remote); err != nil {
 		http.Error(w, err.Error(), http.StatusInternalServerError)
 		return
@@ -147,13 +152,13 @@ func (h *RemotesHandler) update(w http.ResponseWriter, r *http.Request) {
 	// Release / APKINDEX): purge it so the next request re-fetches from the new
 	// upstream instead of serving stale data until TTL expiry. A flush failure
 	// is logged but does not fail the request — the DB update already landed.
-	if oldKnown && oldBaseURL != remote.BaseURL && h.cache != nil {
+	if oldKnown && existing.BaseURL != remote.BaseURL && h.cache != nil {
 		if err := h.cache.FlushRemote(r.Context(), name); err != nil {
 			slog.Warn("flush cached metadata after base_url change failed",
 				"remote", name, "error", err)
 		} else {
 			slog.Info("flushed cached metadata after base_url change",
-				"remote", name, "old_base_url", oldBaseURL, "new_base_url", remote.BaseURL)
+				"remote", name, "old_base_url", existing.BaseURL, "new_base_url", remote.BaseURL)
 		}
 	}
 	writeJSON(w, http.StatusOK, remote)
@@ -165,6 +170,11 @@ func (h *RemotesHandler) del(w http.ResponseWriter, r *http.Request) {
 		http.Error(w, err.Error(), http.StatusInternalServerError)
 		return
 	}
+	if h.cache != nil {
+		if err := h.cache.FlushRemote(r.Context(), name); err != nil {
+			slog.Warn("flush cached metadata after delete failed", "remote", name, "error", err)
+		}
+	}
 	w.WriteHeader(http.StatusNoContent)
 }
 

internal/api/v2/remotes_flush_test.go

@@ -94,3 +94,28 @@ func TestUpdateFlushFailureStillSucceeds(t *testing.T) {
 		t.Fatalf("flush calls = %v, want exactly one attempted flush", ff.calls)
 	}
 }
+
+// Deleting a remote flushes its cached state so a re-created remote of the
+// same name starts cold; a flush error does not fail the delete.
+func TestDeleteFlushesCache(t *testing.T) {
+	if testDSN == "" {
+		t.Skip("Docker unavailable")
+	}
+	db, err := database.New(testDSN)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer db.Close()
+
+	for _, ff := range []*fakeFlusher{{}, {err: errors.New("redis down")}} {
+		const name = "rpm-flush-delete"
+		seedRemote(t, db, name, "https://example.com/repo")
+		h := NewRemotesHandler(db, ff, nil).Routes()
+		if c := do(t, h, "DELETE", "/"+name, ""); c != 204 {
+			t.Fatalf("delete = %d, want 204", c)
+		}
+		if len(ff.calls) != 1 || ff.calls[0] != name {
+			t.Fatalf("flush calls = %v, want exactly one flush of %q", ff.calls, name)
+		}
+	}
+}

internal/api/v2/remotes_ttl_test.go

@@ -0,0 +1,57 @@
+package v2
+
+import (
+	"context"
+	"testing"
+
+	"git.unkin.net/unkin/artifactapi/internal/database"
+)
+
+// An omitted mutable_ttl defaults to 3600 on create and keeps the stored value
+// on update; an explicit 0 is stored as 0 in both.
+func TestMutableTTLOmittedVsZero(t *testing.T) {
+	if testDSN == "" {
+		t.Skip("Docker unavailable")
+	}
+	db, err := database.New(testDSN)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer db.Close()
+	h := NewRemotesHandler(db, nil, nil).Routes()
+	ttl := func(name string) int {
+		t.Helper()
+		r, err := db.GetRemote(context.Background(), name)
+		if err != nil {
+			t.Fatalf("get %s: %v", name, err)
+		}
+		return r.MutableTTL
+	}
+	const body = `"package_type":"rpm","base_url":"https://example.com/repo"`
+
+	steps := []struct {
+		method, path, json string
+		want               int
+	}{
+		{"POST", "/", `{"name":"ttl-omit",` + body + `}`, 3600},
+		{"PUT", "/ttl-omit", `{` + body + `,"mutable_ttl":120}`, 120},
+		{"PUT", "/ttl-omit", `{` + body + `}`, 120},
+		{"PUT", "/ttl-omit", `{` + body + `,"mutable_ttl":0}`, 0},
+		{"PUT", "/ttl-omit", `{` + body + `}`, 0},
+	}
+	for i, s := range steps {
+		if c := do(t, h, s.method, s.path, s.json); c/100 != 2 {
+			t.Fatalf("step %d %s %s = %d", i, s.method, s.path, c)
+		}
+		if got := ttl("ttl-omit"); got != s.want {
+			t.Errorf("step %d: mutable_ttl = %d, want %d", i, got, s.want)
+		}
+	}
+
+	if c := do(t, h, "POST", "/", `{"name":"ttl-zero",`+body+`,"mutable_ttl":0}`); c != 201 {
+		t.Fatalf("create explicit 0 = %d", c)
+	}
+	if got := ttl("ttl-zero"); got != 0 {
+		t.Errorf("explicit 0 stored as %d", got)
+	}
+}