Default omitted mutable_ttl and keep the ETag past TTL expiry

- default mutable_ttl to 3600 on create and keep the stored value on update
- store mutable ETags without a TTL so expiry revalidates with If-None-Match
- drop the stored ETag when a refetch returns none

docs/common/caching.md

@@ -12,8 +12,8 @@ Every proxied request on a remote is classified as **denied**, **immutable** or
 | `immutable_patterns` | Regexes forcing a path immutable. |
 | `mutable_patterns` | Regexes forcing a path mutable. |
 | `immutable_ttl` | Seconds an immutable path stays fresh. `0` = forever. |
-| `mutable_ttl` | Seconds a mutable path stays fresh. `0` also means never expires, and the API stores `0` when the field is omitted. |
-| `check_mutable` | Stores the upstream ETag and tries a conditional `HEAD` (`If-None-Match`) when a mutable path is fetched. The ETag expires with the `mutable_ttl` freshness key, so an expired mutable path is refetched in full, not revalidated. |
+| `mutable_ttl` | Seconds a mutable path stays fresh. `0` also means never expires. Omitted on create, it defaults to `3600`; omitted on update, the stored value is kept. |
+| `check_mutable` | When a mutable path's `mutable_ttl` expires, sends a conditional `HEAD` (`If-None-Match`) with the stored upstream ETag. A `304` re-arms the TTL and serves the stored copy; anything else refetches in full. The ETag has no expiry of its own and is replaced or dropped on each full fetch. |
 | `stale_on_error` | On an upstream network error, serve the stored copy instead of failing. |
 | `quarantine_enabled` / `quarantine_days` | Stored on the remote but not enforced by the proxy yet. |
 | `mirrorlist` / `mirror_strategy` | rpm/deb/alpine only. Upstream pool is `[base_url] + mirrorlist`; `round_robin` (default) rotates the starting mirror, `least_conn` starts at the one with the fewest in-flight requests. Network errors and 5xx fail over to the next mirror; other statuses return as-is. |
@@ -46,7 +46,7 @@ So `blocklist` beats the allowlist, and `immutable_patterns` beats
 |---|---|---|
 | Stored as | content-addressed blob (sha256), streamed | per-path index object, buffered and rewritten by the provider |
 | Fresh for | `immutable_ttl` (`0` = forever) | `mutable_ttl` |
-| After expiry | refetched from upstream | refetched from upstream (the ETag expires with the TTL, so `check_mutable` does not revalidate it) |
+| After expiry | refetched from upstream | revalidated with `If-None-Match` when `check_mutable` is set and an ETag is stored, else refetched |
 | Upstream down | `stale_on_error` serves the stored copy and re-arms the TTL; an open circuit breaker always serves stale if a copy exists, else 503 | same |
 
 Concurrent misses on one path share a fetch lock. Waiters poll the store for

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)

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

internal/cache/cache_test.go

@@ -86,6 +86,12 @@ func TestETagAndToken(t *testing.T) {
 	if v, _ := testRedis.GetETag(ctx, "r", "p"); v != `"abc"` {
 		t.Errorf("etag = %q", v)
 	}
+	if err := testRedis.SetETag(ctx, "r", "p", "", 0); err != nil {
+		t.Fatal(err)
+	}
+	if v, _ := testRedis.GetETag(ctx, "r", "p"); v != "" {
+		t.Errorf("empty etag should drop the stored one, got %q", v)
+	}
 
 	if v, _ := testRedis.GetToken(ctx, "missing"); v != "" {
 		t.Error("missing token should be empty")

internal/cache/redis.go

@@ -57,8 +57,12 @@ func (r *Redis) ReleaseLock(ctx context.Context, remote, path string) error {
 	return r.client.Del(ctx, key).Err()
 }
 
+// SetETag stores etag for path, or drops the stored one when etag is empty.
 func (r *Redis) SetETag(ctx context.Context, remote, path, etag string, ttl time.Duration) error {
 	key := fmt.Sprintf("etag:%s:%s", remote, path)
+	if etag == "" {
+		return r.client.Del(ctx, key).Err()
+	}
 	return r.client.Set(ctx, key, etag, ttl).Err()
 }
 

internal/proxy/engine.go

@@ -160,7 +160,6 @@ func (e *Engine) Fetch(ctx context.Context, remote models.Remote, path string, p
 			notModified, err := e.checkUpstream(ctx, remote, path, etag, prov)
 			if err == nil && notModified {
 				_ = e.cache.SetTTL(ctx, remote.Name, path, ttl)
-				_ = e.cache.SetETag(ctx, remote.Name, path, etag, ttl)
 				result, err := e.serveFromStore(ctx, remote, path)
 				if err == nil {
 					result.Source = "cache"
@@ -491,9 +490,8 @@ func (e *Engine) fetchFromUpstreamOnce(ctx context.Context, remote models.Remote
 		}
 
 		_ = e.cache.SetTTL(ctx, remote.Name, path, ttl)
-		if etag := resp.Header.Get("ETag"); etag != "" {
-			_ = e.cache.SetETag(ctx, remote.Name, path, etag, ttl)
-		}
+		// The ETag outlives the freshness key so an expired path is revalidated.
+		_ = e.cache.SetETag(ctx, remote.Name, path, resp.Header.Get("ETag"), 0)
 
 		return &FetchResult{
 			Reader:      io.NopCloser(bytesReader(body)),

internal/proxy/engine_test.go

@@ -262,28 +262,37 @@ func TestCircuitOpenServesStale(t *testing.T) {
 
 func TestMutableRevalidation(t *testing.T) {
 	requireStack(t)
-	ctx := context.Background()
-	r := seed(t, models.Remote{Name: "eng-npm", PackageType: models.PackageNPM, RepoType: models.RepoTypeRemote, BaseURL: upstream.URL, CheckMutable: true, MutableTTL: 3600, StaleOnError: true})
-	p := prov(t, models.PackageNPM)
+	srv, rev, conditional := changingUpstream(t)
+	r := seed(t, models.Remote{Name: "eng-reval", PackageType: models.PackageRPM, RepoType: models.RepoTypeRemote, BaseURL: srv.URL, CheckMutable: true, MutableTTL: 1})
+	const path = "8/Everything/x86_64/repodata/repomd.xml"
+	fetch := func() (string, string) {
+		t.Helper()
+		res, err := testEngine.Fetch(context.Background(), r, path, prov(t, models.PackageRPM))
+		if err != nil {
+			t.Fatalf("fetch: %v", err)
+		}
+		return readAll(t, res), res.Source
+	}
 
-	res, err := testEngine.Fetch(ctx, r, "pkg", p)
-	if err != nil {
-		t.Fatalf("initial mutable fetch: %v", err)
+	if body, _ := fetch(); body != "rev1:/"+path {
+		t.Fatalf("initial fetch = %q", body)
+	}
+	time.Sleep(1100 * time.Millisecond)
+	if body, src := fetch(); body != "rev1:/"+path || src != "cache" {
+		t.Errorf("after expiry = %q from %s, want cached rev1", body, src)
+	}
+	if n := conditional.Load(); n != 1 {
+		t.Errorf("conditional requests = %d, want 1 revalidation", n)
+	}
+	if fresh, _ := testCache.CheckTTL(context.Background(), r.Name, path); !fresh {
+		t.Error("304 should re-arm the freshness key")
 	}
-	res.Reader.Close()
 
-	// Expire only the freshness marker; the ETag persists, forcing a
-	// conditional revalidation that the upstream answers with 304.
-	testCache.SetTTL(ctx, "eng-npm", "pkg", time.Millisecond)
-	time.Sleep(10 * time.Millisecond)
-	res, err = testEngine.Fetch(ctx, r, "pkg", p)
-	if err != nil {
-		t.Fatalf("revalidation fetch: %v", err)
+	rev.Store("rev2")
+	time.Sleep(1100 * time.Millisecond)
+	if body, src := fetch(); body != "rev2:/"+path || src != "remote" {
+		t.Errorf("after upstream change = %q from %s, want remote rev2", body, src)
 	}
-	if res.Source != "cache" {
-		t.Errorf("revalidated response should come from cache, got %s", res.Source)
-	}
-	res.Reader.Close()
 }
 
 func TestBearerTokenFlow(t *testing.T) {