From 7045e84c00ec2be0e4cb81ebbfa76e568f4799ff Mon Sep 17 00:00:00 2001 From: Evan Jarrett Date: Wed, 9 Sep 2026 20:42:58 -0500 Subject: [PATCH] appview: read the sailor profile from the local users row, not the PDS, per request Hold discovery in the registry middleware called getRecord on the repository owner's PDS for every request under /v2/: every HEAD, POST, PATCH, PUT and GET. A 10-layer push was 40 or more PDS round trips, and it was the last per-request network call on the push path that had nothing to do with moving bytes. Only two profile fields are used there: the default hold and the auto-remove-untagged flag. The users row already caches the default hold, written by the Jetstream processor on every profile event and prefilled by the backfill, and the auth gate already reads it from there. This makes the row a faithful copy of what the registry needs and switches the middleware to it. The auto-remove flag gets a nullable users column. NULL means the value has never been learned; the processor writes 0 or 1 on every profile event and never NULL. On a request whose row is missing or still NULL, the middleware does one live fetch, uses it, and writes both fields back, including a 0 for a user with no profile at all, so the fallback runs at most once per user. A failed fetch writes nothing and uses the appview default for that request, so a network error is never cached. That single mechanism covers the minutes after a deploy while the startup backfill fills the column, a brand-new user, and a user the backfill has not reached. The processor also stops returning early on an empty default hold, which left a user who removed their custom hold pushing to it forever. Empty is now written through and means the appview default, matching what the auth gate already reads. Tests count PDS requests with a test server: a populated row makes none, a NULL row makes exactly one and then none, a missing profile is cached as known, and a failed fetch degrades without writing. The migration was applied to a fresh database and to one built from the previous schema. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_018Yf1ZVA7sXYhQNb9tCo1m5 --- CLAUDE.md | 6 +- docs/BYOS.md | 7 +- docs/HOLD_PUSH_OFFLOAD.md | 4 +- .../0035_add_auto_remove_untagged.yaml | 3 + pkg/appview/db/queries.go | 95 ++++++ pkg/appview/db/schema.sql | 6 + pkg/appview/jetstream/processor.go | 33 +- pkg/appview/jetstream/profile_prefs_test.go | 184 ++++++++++++ pkg/appview/middleware/hold_prefs_test.go | 281 ++++++++++++++++++ pkg/appview/middleware/registry.go | 143 +++++++-- pkg/appview/middleware/registry_test.go | 8 +- pkg/appview/server.go | 1 + 12 files changed, 726 insertions(+), 45 deletions(-) create mode 100644 pkg/appview/db/migrations/0035_add_auto_remove_untagged.yaml create mode 100644 pkg/appview/jetstream/profile_prefs_test.go create mode 100644 pkg/appview/middleware/hold_prefs_test.go diff --git a/CLAUDE.md b/CLAUDE.md index 0990221..dd498a4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -92,10 +92,12 @@ ATCR uses **distribution/distribution** as a library, extending it via middlewar **Pull:** AppView fetches manifest from user's PDS. The manifest's `holdDid` field tells where blobs were stored. Blobs fetched from that hold via presigned download URLs. Pull always uses the historical hold from the manifest, even if the user changed their default since pushing. -**Hold discovery priority** (in `findHoldDIDAndProfile()`, `pkg/appview/middleware/registry.go`): +**Hold discovery priority** (in `findHoldDIDAndPrefs()`, `pkg/appview/middleware/registry.go`): 1. Sailor profile's `defaultHold` (user preference) 2. AppView's default hold (`server.managed_holds[0]`, the fallback) +The registry reads both values (`defaultHold` and `autoRemoveUntagged`) from the local `users` row, which Jetstream keeps current and the backfill prefills, so a push costs no PDS round trips for them. Only when that row is missing or its `auto_remove_untagged` is still NULL does it fetch the profile live, once, and write the answer back. + After discovery, `resolveSuccessor()` applies a single-hop redirect: if the chosen hold's captain record declares a `successor` DID (migration redirect), blobs route to the successor instead. Single-hop only — successor chains are not followed. ### Name Resolution @@ -228,7 +230,7 @@ See `config-appview.example.yaml` and `config-hold.example.yaml` for all options **Changing name resolution:** 1. Modify `pkg/atproto/resolver.go` for DID/handle resolution 2. Update `pkg/appview/middleware/registry.go` if changing routing -3. `findHoldDIDAndProfile()` checks: sailor profile `defaultHold` → AppView default hold (`server.managed_holds[0]`), then `resolveSuccessor()` applies a single-hop successor redirect +3. `findHoldDIDAndPrefs()` checks: cached `users.default_hold_did` (from the sailor profile's `defaultHold`) → AppView default hold (`server.managed_holds[0]`), then `resolveSuccessor()` applies a single-hop successor redirect **Working with OAuth client:** - Self-contained: pass `baseURL`, handles client ID/redirect URI/scopes diff --git a/docs/BYOS.md b/docs/BYOS.md index afe4008..9838a23 100644 --- a/docs/BYOS.md +++ b/docs/BYOS.md @@ -18,7 +18,8 @@ ATCR supports "Bring Your Own Storage" (BYOS) for blob storage. Users can: │ - Profile management │ └────────────┬─────────────────────────────┘ │ - │ Hold discovery (findHoldDIDAndProfile): + │ Hold discovery (findHoldDIDAndPrefs), read from + │ the local users row, not the PDS: │ 1. io.atcr.sailor.profile.defaultHold (DID) │ 2. AppView default hold (server.managed_holds[0]) │ @@ -215,7 +216,9 @@ fly secrets set HOLD_REGISTRATION_OWNER_DID=did:plc:your-did-here 2. AppView resolves alice → did:plc:alice123 3. AppView discovers hold DID: - - Check alice's sailor profile for defaultHold + - Read alice's cached defaultHold from the local users row, which Jetstream + keeps current from her sailor profile. Only a row that has never been + filled costs one live profile fetch, and that fetch is written back. - Returns: "did:web:alice-storage.fly.dev" 4. AppView gets service token from alice's PDS: diff --git a/docs/HOLD_PUSH_OFFLOAD.md b/docs/HOLD_PUSH_OFFLOAD.md index 8ca3df5..83d14df 100644 --- a/docs/HOLD_PUSH_OFFLOAD.md +++ b/docs/HOLD_PUSH_OFFLOAD.md @@ -128,7 +128,7 @@ The flow the operator described: ``` 1. docker: POST https:///v2///blobs/uploads/ -2. AppView: resolve hold DID (findHoldDIDAndProfile + resolveSuccessor) +2. AppView: resolve hold DID (findHoldDIDAndPrefs + resolveSuccessor) check hold push policy (anonymous push allowed?) if auth required: mint/fetch service token (aud=hold DID) via user OAuth -> 202 Accepted @@ -228,7 +228,7 @@ pursued. | Change | Where | |---|---| | `POST .../blobs/uploads/` returns a cross-host hold Location instead of driving `ProxyBlobStore.Create` | new handler ahead of the distribution `/v2` handler; see "Distribution interaction" | -| Hold push-policy check during handshake | `findHoldDIDAndProfile` result + captain push policy (Jetstream-fed local table, keep it local-fast) | +| Hold push-policy check during handshake | `findHoldDIDAndPrefs` result + captain push policy (Jetstream-fed local table, keep it local-fast) | | Mint service token for upload, embed in Location | reuse `GetOrFetchServiceToken` (`pkg/auth/servicetoken.go`) | | `HEAD .../blobs/` (existence / layer skip) stays on AppView | answer from local layer metadata / hold query; keeps docker's skip-existing fast | | Manifest `PUT` stays on AppView; verify referenced blobs exist on the hold before storing | `manifest_store.go` + a hold existence check | diff --git a/pkg/appview/db/migrations/0035_add_auto_remove_untagged.yaml b/pkg/appview/db/migrations/0035_add_auto_remove_untagged.yaml new file mode 100644 index 0000000..82473d5 --- /dev/null +++ b/pkg/appview/db/migrations/0035_add_auto_remove_untagged.yaml @@ -0,0 +1,3 @@ +description: Cache the sailor profile autoRemoveUntagged preference on users so the registry hot path does not read the PDS on every request +query: | + ALTER TABLE users ADD COLUMN auto_remove_untagged INTEGER; diff --git a/pkg/appview/db/queries.go b/pkg/appview/db/queries.go index c388e69..a3e340b 100644 --- a/pkg/appview/db/queries.go +++ b/pkg/appview/db/queries.go @@ -649,6 +649,89 @@ func UpdateUserRegistryDomain(db DBTX, did string, registryDomain string) error return err } +// UpdateUserAutoRemoveUntagged caches the sailor profile's autoRemoveUntagged +// preference. Callers here always know the answer, so this writes 0 or 1 and +// never NULL; NULL is reserved for "not learned yet". See UserHoldPrefs. +func UpdateUserAutoRemoveUntagged(db DBTX, did string, autoRemove bool) error { + _, err := db.Exec(` + UPDATE users SET auto_remove_untagged = ? WHERE did = ? + `, autoRemove, did) + return err +} + +// UserHoldPrefs is the slice of a sailor profile the registry hot path reads on +// every /v2/ request: which hold the owner's blobs go to, and whether a tag +// overwrite deletes the manifest that lost its last tag. +type UserHoldPrefs struct { + // Found is false when the user has no row at all. + Found bool + + // DefaultHoldDID is the user's chosen hold, already normalized to a DID by + // whoever cached it. Empty means "use the appview's default hold", the same + // meaning it carries for the auth gate's resolveHoldDID. + DefaultHoldDID string + + // AutoRemoveUntagged is invalid when the column is NULL, meaning no profile + // has been processed for this user yet and the value is simply unknown. A + // valid false is a known false. Callers must not collapse the two: unknown + // is what triggers the one-shot live fetch. + AutoRemoveUntagged sql.NullBool +} + +// GetUserHoldPrefs reads the cached sailor profile preferences for a user. +// A missing row is not an error; it comes back with Found false. +func GetUserHoldPrefs(db DBTX, did string) (UserHoldPrefs, error) { + var ( + prefs UserHoldPrefs + holdDID sql.NullString + autoRemove sql.NullBool + ) + err := db.QueryRow(` + SELECT default_hold_did, auto_remove_untagged FROM users WHERE did = ? + `, did).Scan(&holdDID, &autoRemove) + if err == sql.ErrNoRows { + return prefs, nil + } + if err != nil { + return prefs, err + } + + prefs.Found = true + if holdDID.Valid { + prefs.DefaultHoldDID = holdDID.String + } + prefs.AutoRemoveUntagged = autoRemove + return prefs, nil +} + +// CacheUserHoldPrefs writes both cached profile preferences for a user, +// creating the row if this DID has never been seen before. +// +// It exists for the registry's one-shot fallback: when the Jetstream-fed cache +// has nothing to say about a user, the middleware fetches the profile once and +// records the answer here so no later request has to. autoRemove is written as +// 0 or 1 even when the user has no profile record at all, which is what stops +// the fallback from firing again for that user. +// +// last_seen is stamped only on insert, and only because the column is NOT NULL. +// It is deliberately left alone on update: serving somebody else's pull of this +// user's repository is not this user's activity, and last_seen is read as an +// activity signal. +func CacheUserHoldPrefs(db DBTX, did, handle, pdsEndpoint, holdDID string, autoRemove bool) error { + // A handle belongs to exactly one DID at a time; release it from any stale + // owner before claiming it, exactly as the other user upserts do. + _, _ = db.Exec(`UPDATE users SET handle = did WHERE handle = ? AND did != ?`, handle, did) + + _, err := db.Exec(` + INSERT INTO users (did, handle, pds_endpoint, default_hold_did, auto_remove_untagged, last_seen) + VALUES (?, ?, ?, ?, ?, ?) + ON CONFLICT(did) DO UPDATE SET + default_hold_did = excluded.default_hold_did, + auto_remove_untagged = excluded.auto_remove_untagged + `, did, handle, pdsEndpoint, holdDID, autoRemove, time.Now()) + return err +} + // GetUserDefaultHoldDID returns a user's explicitly-set default hold DID from // the cached profile, or "" if unset. Unlike GetUserHoldDID it does NOT fall // back to a manifest's hold_endpoint (which may be a URL, not a DID): callers @@ -2359,6 +2442,18 @@ func (h *HoldDIDDB) GetDistinctManifestHoldDIDs(did string) ([]string, error) { return GetDistinctManifestHoldDIDs(h.db, did) } +// GetUserHoldPrefs reads the cached sailor profile preferences for a user. +// Implements the registry middleware's UserPrefsCache. +func (h *HoldDIDDB) GetUserHoldPrefs(did string) (UserHoldPrefs, error) { + return GetUserHoldPrefs(h.db, did) +} + +// CacheUserHoldPrefs records both cached profile preferences for a user. +// Implements the registry middleware's UserPrefsCache. +func (h *HoldDIDDB) CacheUserHoldPrefs(did, handle, pdsEndpoint, holdDID string, autoRemove bool) error { + return CacheUserHoldPrefs(h.db, did, handle, pdsEndpoint, holdDID, autoRemove) +} + // IsManifestReferenced checks if a digest is a child of any manifest list for the user. // Implements storage.ManifestReferenceChecker. func (h *HoldDIDDB) IsManifestReferenced(did, digest string) (bool, error) { diff --git a/pkg/appview/db/schema.sql b/pkg/appview/db/schema.sql index c9fc552..571b15f 100644 --- a/pkg/appview/db/schema.sql +++ b/pkg/appview/db/schema.sql @@ -15,6 +15,12 @@ CREATE TABLE IF NOT EXISTS users ( default_hold_did TEXT, oci_client TEXT DEFAULT '', registry_domain TEXT DEFAULT '', + -- Cached io.atcr.sailor.profile autoRemoveUntagged. Deliberately nullable + -- with no default: NULL means "no profile event has been processed for this + -- user yet, so the value is unknown", which is distinct from a known false. + -- The registry hot path treats NULL as a cue to fetch the profile live once + -- and write the answer back here. + auto_remove_untagged INTEGER, last_seen TIMESTAMP NOT NULL, UNIQUE(handle) ); diff --git a/pkg/appview/jetstream/processor.go b/pkg/appview/jetstream/processor.go index 50b0319..93ffcec 100644 --- a/pkg/appview/jetstream/processor.go +++ b/pkg/appview/jetstream/processor.go @@ -573,9 +573,12 @@ func (p *Processor) ProcessSailorProfile(ctx context.Context, did string, record slog.Warn("Failed to cache registry domain preference", "component", "processor", "did", did, "registryDomain", profileRecord.RegistryDomain, "error", err) } - // Skip hold processing if no default hold set - if profileRecord.DefaultHold == "" { - return nil + // Cache the untagged-cleanup preference. A processed profile is always a + // known value, so this writes 0 or 1 and never leaves the column NULL. NULL + // means "never learned", which is what makes the registry middleware fetch + // the profile live once; writing here is what stops it doing so again. + if err := db.UpdateUserAutoRemoveUntagged(p.db, did, profileRecord.AutoRemoveUntagged); err != nil { + slog.Warn("Failed to cache autoRemoveUntagged preference", "component", "processor", "did", did, "autoRemoveUntagged", profileRecord.AutoRemoveUntagged, "error", err) } // Convert hold URL/DID to canonical DID. On failure, cache the raw reference @@ -584,11 +587,22 @@ func (p *Processor) ProcessSailorProfile(ctx context.Context, did string, record // unresolvable must not fall through to managed. A managed hold's DID never // fails resolution (DIDs return as-is), so only unreachable URL-form refs land // here, and a raw non-DID value correctly reads as non-managed (fail closed). - holdDID, err := atproto.ResolveHoldDID(ctx, profileRecord.DefaultHold) - if err != nil { - slog.Warn("Invalid hold reference in profile; caching raw value (fails closed for billing)", - "component", "processor", "did", did, "default_hold", profileRecord.DefaultHold, "error", err) - holdDID = profileRecord.DefaultHold + // + // An empty defaultHold is not resolved, it is written straight through. The + // user cleared their custom hold, and "" is exactly how both the billing gate + // and the auth gate's resolveHoldDID spell "fall back to the operator + // default". Returning early here instead, as this used to, left the old value + // in the row forever, so a user who moved back to the managed hold kept being + // routed to a hold they had abandoned. + holdDID := "" + if profileRecord.DefaultHold != "" { + resolved, err := atproto.ResolveHoldDID(ctx, profileRecord.DefaultHold) + if err != nil { + slog.Warn("Invalid hold reference in profile; caching raw value (fails closed for billing)", + "component", "processor", "did", did, "default_hold", profileRecord.DefaultHold, "error", err) + resolved = profileRecord.DefaultHold + } + holdDID = resolved } // Cache default hold DID on the user record @@ -598,7 +612,8 @@ func (p *Processor) ProcessSailorProfile(ctx context.Context, did string, record // Query and cache the captain record using provided function // This allows backfill-specific logic (retries, test mode handling) without duplicating it here - if queryCaptainFn != nil { + // There is nothing to query when the user has no hold of their own. + if queryCaptainFn != nil && holdDID != "" { return queryCaptainFn(ctx, holdDID) } diff --git a/pkg/appview/jetstream/profile_prefs_test.go b/pkg/appview/jetstream/profile_prefs_test.go new file mode 100644 index 0000000..d2f14a0 --- /dev/null +++ b/pkg/appview/jetstream/profile_prefs_test.go @@ -0,0 +1,184 @@ +package jetstream + +import ( + "context" + "database/sql" + "testing" + "time" + + "atcr.io/pkg/appview/db" + "atcr.io/pkg/atproto" + "github.com/bluesky-social/indigo/atproto/identity" + "github.com/bluesky-social/indigo/atproto/syntax" +) + +// readPrefs returns the cached row state, with auto_remove_untagged as a +// tri-state: NULL means the processor has never seen a profile for this user, +// which is what makes the registry middleware fetch one live. +func readPrefs(t *testing.T, database *sql.DB, did string) (found bool, holdDID sql.NullString, autoRemove sql.NullBool) { + t.Helper() + err := database.QueryRow( + `SELECT default_hold_did, auto_remove_untagged FROM users WHERE did = ?`, did, + ).Scan(&holdDID, &autoRemove) + if err == sql.ErrNoRows { + return false, holdDID, autoRemove + } + if err != nil { + t.Fatalf("read cached prefs: %v", err) + } + return true, holdDID, autoRemove +} + +// TestProcessSailorProfile_ClearingDefaultHold covers a bug that had no +// expiry: the processor returned early when defaultHold was empty, so a user +// who removed their custom hold kept the old DID in users.default_hold_did +// forever and went on being routed to a hold they had abandoned. Empty is a +// value, and it means "use the operator's default" to both the auth gate's +// resolveHoldDID and the billing gate. +func TestProcessSailorProfile_ClearingDefaultHold(t *testing.T) { + database := setupTestDB(t) + defer database.Close() + + const did = "did:plc:clearhold" + if err := db.UpsertUser(database, &db.User{ + DID: did, Handle: "clear.test", PDSEndpoint: "https://pds", LastSeen: time.Now(), + }); err != nil { + t.Fatalf("upsert user: %v", err) + } + + p := NewProcessor(database, false, nil) + ctx := context.Background() + + withHold := []byte(`{"$type":"io.atcr.sailor.profile","defaultHold":"did:web:user.hold.io","createdAt":"2025-01-01T00:00:00Z"}`) + if err := p.ProcessSailorProfile(ctx, did, withHold, nil); err != nil { + t.Fatalf("ProcessSailorProfile (with hold): %v", err) + } + if got := db.GetUserDefaultHoldDID(database, did); got != "did:web:user.hold.io" { + t.Fatalf("default_hold_did = %q, want the profile's hold", got) + } + + cleared := []byte(`{"$type":"io.atcr.sailor.profile","createdAt":"2025-01-01T00:00:00Z"}`) + if err := p.ProcessSailorProfile(ctx, did, cleared, nil); err != nil { + t.Fatalf("ProcessSailorProfile (cleared): %v", err) + } + if got := db.GetUserDefaultHoldDID(database, did); got != "" { + t.Errorf("default_hold_did = %q after the user cleared it, want \"\" (operator default)", got) + } +} + +// TestProcessSailorProfile_ClearedHoldSkipsCaptainQuery: with no hold of their +// own there is no captain record to go and fetch. +func TestProcessSailorProfile_ClearedHoldSkipsCaptainQuery(t *testing.T) { + database := setupTestDB(t) + defer database.Close() + + const did = "did:plc:clearcaptain" + if err := db.UpsertUser(database, &db.User{ + DID: did, Handle: "clearcap.test", PDSEndpoint: "https://pds", LastSeen: time.Now(), + }); err != nil { + t.Fatalf("upsert user: %v", err) + } + + queried := 0 + queryCaptain := func(context.Context, string) error { + queried++ + return nil + } + + p := NewProcessor(database, false, nil) + cleared := []byte(`{"$type":"io.atcr.sailor.profile","createdAt":"2025-01-01T00:00:00Z"}`) + if err := p.ProcessSailorProfile(context.Background(), did, cleared, queryCaptain); err != nil { + t.Fatalf("ProcessSailorProfile: %v", err) + } + if queried != 0 { + t.Errorf("captain query ran %d times for a user with no hold, want 0", queried) + } +} + +// TestProcessSailorProfile_CachesAutoRemoveUntagged: a processed profile is +// always a known value, so the column must come out 0 or 1 and never NULL. +// NULL is reserved for "never learned", and only the registry's one-shot +// fallback is allowed to resolve it. +func TestProcessSailorProfile_CachesAutoRemoveUntagged(t *testing.T) { + database := setupTestDB(t) + defer database.Close() + + const did = "did:plc:autoremove" + if err := db.UpsertUser(database, &db.User{ + DID: did, Handle: "auto.test", PDSEndpoint: "https://pds", LastSeen: time.Now(), + }); err != nil { + t.Fatalf("upsert user: %v", err) + } + + if _, _, autoRemove := readPrefs(t, database, did); autoRemove.Valid { + t.Fatalf("a fresh user row should start with auto_remove_untagged NULL, got %v", autoRemove.Bool) + } + + p := NewProcessor(database, false, nil) + ctx := context.Background() + + on := []byte(`{"$type":"io.atcr.sailor.profile","autoRemoveUntagged":true,"createdAt":"2025-01-01T00:00:00Z"}`) + if err := p.ProcessSailorProfile(ctx, did, on, nil); err != nil { + t.Fatalf("ProcessSailorProfile (on): %v", err) + } + _, _, autoRemove := readPrefs(t, database, did) + if !autoRemove.Valid || !autoRemove.Bool { + t.Errorf("auto_remove_untagged = %+v, want a valid true", autoRemove) + } + + // autoRemoveUntagged is omitempty, so "off" arrives as an absent field. + // That still has to be written as a known 0, not left at the previous true. + off := []byte(`{"$type":"io.atcr.sailor.profile","createdAt":"2025-01-01T00:00:00Z"}`) + if err := p.ProcessSailorProfile(ctx, did, off, nil); err != nil { + t.Fatalf("ProcessSailorProfile (off): %v", err) + } + _, _, autoRemove = readPrefs(t, database, did) + if !autoRemove.Valid { + t.Fatalf("auto_remove_untagged went back to NULL; the registry would refetch forever") + } + if autoRemove.Bool { + t.Errorf("auto_remove_untagged = true after the user turned it off") + } +} + +// TestProcessRecord_SailorProfileCreatesMissingUserRow pins the ensure step. +// ProcessSailorProfile writes with UPDATE, which is a silent no-op against a +// missing row, so a profile event for a DID that has never opened the web UI +// would land nowhere at all without it. ProcessRecord is the only entry point +// on both the live worker and the backfill, and both go through EnsureUser. +func TestProcessRecord_SailorProfileCreatesMissingUserRow(t *testing.T) { + database := setupTestDB(t) + defer database.Close() + + const did = "did:plc:neverseen" + atproto.SetDirectory(&fakeDirectory{byDID: map[string]*identity.Identity{ + did: { + DID: syntax.DID(did), + Handle: syntax.Handle("neverseen.example.com"), + Services: map[string]identity.ServiceEndpoint{ + "atproto_pds": {Type: "AtprotoPersonalDataServer", URL: "https://pds.example.com"}, + }, + }, + }}) + + if found, _, _ := readPrefs(t, database, did); found { + t.Fatal("user row should not exist yet") + } + + p := NewProcessor(database, true, nil) + record := []byte(`{"$type":"io.atcr.sailor.profile","defaultHold":"did:web:user.hold.io","autoRemoveUntagged":true,"createdAt":"2025-01-01T00:00:00Z"}`) + if err := p.ProcessRecord(context.Background(), did, atproto.SailorProfileCollection, "self", record, false, nil); err != nil { + t.Fatalf("ProcessRecord: %v", err) + } + + found, holdDID, autoRemove := readPrefs(t, database, did) + if !found { + t.Fatal("a profile event for an unknown DID left no row behind") + } + if holdDID.String != "did:web:user.hold.io" { + t.Errorf("default_hold_did = %q, want the profile's hold", holdDID.String) + } + if !autoRemove.Valid || !autoRemove.Bool { + t.Errorf("auto_remove_untagged = %+v, want a valid true", autoRemove) + } +} diff --git a/pkg/appview/middleware/hold_prefs_test.go b/pkg/appview/middleware/hold_prefs_test.go new file mode 100644 index 0000000..f0c8025 --- /dev/null +++ b/pkg/appview/middleware/hold_prefs_test.go @@ -0,0 +1,281 @@ +package middleware + +import ( + "context" + "database/sql" + "encoding/json" + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "atcr.io/pkg/appview/db" + "atcr.io/pkg/atproto" +) + +// The registry used to read the owner's sailor profile from their PDS on every +// single /v2/ request, which made a ten-layer push forty-odd round trips to a +// third-party server before a byte of image data moved. These tests count the +// PDS requests rather than only checking the values that come back, because the +// whole point of the change is the request that does not happen. + +const prefsOwnerDID = "did:plc:prefsowner" +const prefsOwnerHandle = "prefs.example.com" + +// profilePDS starts a PDS whose getRecord handler is supplied by the caller, +// and returns the server plus a counter of profile reads it served. +func profilePDS(t *testing.T, handler http.HandlerFunc) (*httptest.Server, *atomic.Int64) { + t.Helper() + var calls atomic.Int64 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/xrpc/com.atproto.repo.getRecord" { + calls.Add(1) + handler(w, r) + return + } + w.WriteHeader(http.StatusNotFound) + })) + t.Cleanup(srv.Close) + return srv, &calls +} + +// servesProfile answers every profile read with the given record. +func servesProfile(profile *atproto.SailorProfileRecord) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{"value": profile}) + } +} + +func prefsTestDB(t *testing.T) *sql.DB { + t.Helper() + database, err := db.InitDB(":memory:", db.LibsqlConfig{}) + require.NoError(t, err, "open test database") + t.Cleanup(func() { database.Close() }) + return database +} + +// seedUser inserts a users row. autoRemove nil leaves auto_remove_untagged +// NULL, which is the "never learned" state. +func seedUser(t *testing.T, database *sql.DB, holdDID string, autoRemove *bool) { + t.Helper() + var val any + if autoRemove != nil { + val = *autoRemove + } + _, err := database.Exec(` + INSERT INTO users (did, handle, pds_endpoint, default_hold_did, auto_remove_untagged, last_seen) + VALUES (?, ?, ?, ?, ?, ?)`, + prefsOwnerDID, prefsOwnerHandle, "https://pds.example.com", holdDID, val, time.Now()) + require.NoError(t, err, "seed user") +} + +// readCachedPrefs returns the raw row state, with auto_remove_untagged as a +// tri-state so a written 0 is distinguishable from a still-NULL column. +func readCachedPrefs(t *testing.T, database *sql.DB) (found bool, holdDID sql.NullString, autoRemove sql.NullBool) { + t.Helper() + err := database.QueryRow( + `SELECT default_hold_did, auto_remove_untagged FROM users WHERE did = ?`, + prefsOwnerDID).Scan(&holdDID, &autoRemove) + if err == sql.ErrNoRows { + return false, holdDID, autoRemove + } + require.NoError(t, err, "read cached prefs") + return true, holdDID, autoRemove +} + +// TestFindHoldDIDAndPrefs_CachedRowMakesNoPDSRequest is the whole point of the +// change: when the Jetstream-fed row has both answers, the hot path must not +// touch the owner's PDS at all. +func TestFindHoldDIDAndPrefs_CachedRowMakesNoPDSRequest(t *testing.T) { + database := prefsTestDB(t) + known := true + seedUser(t, database, "did:web:user.hold.io", &known) + + pds, calls := profilePDS(t, servesProfile(atproto.NewSailorProfileRecord("did:web:should.not.be.read"))) + + resolver := &NamespaceResolver{ + defaultHoldDID: "did:web:default.atcr.io", + userPrefs: db.NewHoldDIDDB(database), + } + + holdDID, prefs := resolver.findHoldDIDAndPrefs(context.Background(), prefsOwnerDID, prefsOwnerHandle, pds.URL) + + assert.Equal(t, "did:web:user.hold.io", holdDID, "hold DID should come from the cached row") + assert.True(t, prefs.AutoRemoveUntagged, "auto-remove should come from the cached row") + assert.Equal(t, int64(0), calls.Load(), "a fully cached row must not read the PDS") +} + +// TestFindHoldDIDAndPrefs_CachedEmptyHoldUsesDefault pins the meaning of an +// empty default_hold_did: not "unknown", but "use the operator's hold", the +// same reading the auth gate's resolveHoldDID gives it. +func TestFindHoldDIDAndPrefs_CachedEmptyHoldUsesDefault(t *testing.T) { + database := prefsTestDB(t) + known := false + seedUser(t, database, "", &known) + + pds, calls := profilePDS(t, servesProfile(atproto.NewSailorProfileRecord("did:web:should.not.be.read"))) + + resolver := &NamespaceResolver{ + defaultHoldDID: "did:web:default.atcr.io", + userPrefs: db.NewHoldDIDDB(database), + } + + holdDID, prefs := resolver.findHoldDIDAndPrefs(context.Background(), prefsOwnerDID, prefsOwnerHandle, pds.URL) + + assert.Equal(t, "did:web:default.atcr.io", holdDID, "an empty cached hold means the appview default") + assert.False(t, prefs.AutoRemoveUntagged) + assert.Equal(t, int64(0), calls.Load(), "a known-false auto-remove is still a known value") +} + +// TestFindHoldDIDAndPrefs_NullAutoRemoveFetchesOnce covers the window after a +// deploy, while the backfill is still filling the new column: one live fetch, +// written back, and never again. +func TestFindHoldDIDAndPrefs_NullAutoRemoveFetchesOnce(t *testing.T) { + database := prefsTestDB(t) + seedUser(t, database, "did:web:stale.hold.io", nil) // auto_remove_untagged NULL + + profile := atproto.NewSailorProfileRecord("did:web:user.hold.io") + profile.AutoRemoveUntagged = true + pds, calls := profilePDS(t, servesProfile(profile)) + + resolver := &NamespaceResolver{ + defaultHoldDID: "did:web:default.atcr.io", + userPrefs: db.NewHoldDIDDB(database), + } + ctx := context.Background() + + holdDID, prefs := resolver.findHoldDIDAndPrefs(ctx, prefsOwnerDID, prefsOwnerHandle, pds.URL) + assert.Equal(t, "did:web:user.hold.io", holdDID, "the live profile should serve this request") + assert.True(t, prefs.AutoRemoveUntagged) + assert.Equal(t, int64(1), calls.Load(), "a NULL auto-remove should cost exactly one fetch") + + found, cachedHold, cachedAuto := readCachedPrefs(t, database) + require.True(t, found) + assert.Equal(t, "did:web:user.hold.io", cachedHold.String, "the fetch should be written back") + require.True(t, cachedAuto.Valid, "auto_remove_untagged must no longer be NULL") + assert.True(t, cachedAuto.Bool) + + holdDID, prefs = resolver.findHoldDIDAndPrefs(ctx, prefsOwnerDID, prefsOwnerHandle, pds.URL) + assert.Equal(t, "did:web:user.hold.io", holdDID) + assert.True(t, prefs.AutoRemoveUntagged) + assert.Equal(t, int64(1), calls.Load(), "the second request must be served from the row") +} + +// TestFindHoldDIDAndPrefs_MissingRowFetchesOnce covers a user the backfill has +// never reached: no row at all, and the fallback still has to bound itself. +func TestFindHoldDIDAndPrefs_MissingRowFetchesOnce(t *testing.T) { + database := prefsTestDB(t) + + profile := atproto.NewSailorProfileRecord("did:web:user.hold.io") + profile.AutoRemoveUntagged = true + pds, calls := profilePDS(t, servesProfile(profile)) + + resolver := &NamespaceResolver{ + defaultHoldDID: "did:web:default.atcr.io", + userPrefs: db.NewHoldDIDDB(database), + } + ctx := context.Background() + + holdDID, prefs := resolver.findHoldDIDAndPrefs(ctx, prefsOwnerDID, prefsOwnerHandle, pds.URL) + assert.Equal(t, "did:web:user.hold.io", holdDID) + assert.True(t, prefs.AutoRemoveUntagged) + assert.Equal(t, int64(1), calls.Load()) + + found, cachedHold, cachedAuto := readCachedPrefs(t, database) + require.True(t, found, "the fallback should create the row it was missing") + assert.Equal(t, "did:web:user.hold.io", cachedHold.String) + require.True(t, cachedAuto.Valid) + assert.True(t, cachedAuto.Bool) + + _, _ = resolver.findHoldDIDAndPrefs(ctx, prefsOwnerDID, prefsOwnerHandle, pds.URL) + assert.Equal(t, int64(1), calls.Load(), "a created row must stop the fallback repeating") +} + +// TestFindHoldDIDAndPrefs_MissingProfileCachedAsKnown covers the user who has +// never written a sailor profile at all. A 404 is an answer, so it has to be +// written down as 0 rather than left NULL, or that user pays for a PDS round +// trip on every request forever. +func TestFindHoldDIDAndPrefs_MissingProfileCachedAsKnown(t *testing.T) { + database := prefsTestDB(t) + + pds, calls := profilePDS(t, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + }) + + resolver := &NamespaceResolver{ + defaultHoldDID: "did:web:default.atcr.io", + userPrefs: db.NewHoldDIDDB(database), + } + ctx := context.Background() + + holdDID, prefs := resolver.findHoldDIDAndPrefs(ctx, prefsOwnerDID, prefsOwnerHandle, pds.URL) + assert.Equal(t, "did:web:default.atcr.io", holdDID, "no profile means the appview default") + assert.False(t, prefs.AutoRemoveUntagged) + assert.Equal(t, int64(1), calls.Load()) + + found, cachedHold, cachedAuto := readCachedPrefs(t, database) + require.True(t, found, "a 404 profile should still leave a row behind") + assert.Equal(t, "", cachedHold.String, "no custom hold: empty means the operator default") + require.True(t, cachedAuto.Valid, "a 404 is a known answer, not an unknown one") + assert.False(t, cachedAuto.Bool) + + _, _ = resolver.findHoldDIDAndPrefs(ctx, prefsOwnerDID, prefsOwnerHandle, pds.URL) + assert.Equal(t, int64(1), calls.Load(), "a user with no profile must not refetch every request") +} + +// TestFindHoldDIDAndPrefs_FailedFetchIsNotCached is the other side of the same +// coin: a network failure is not an answer. Caching it would freeze a transient +// blip into the user's routing for good. +func TestFindHoldDIDAndPrefs_FailedFetchIsNotCached(t *testing.T) { + database := prefsTestDB(t) + + pds, calls := profilePDS(t, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + }) + + resolver := &NamespaceResolver{ + defaultHoldDID: "did:web:default.atcr.io", + userPrefs: db.NewHoldDIDDB(database), + } + ctx := context.Background() + + holdDID, prefs := resolver.findHoldDIDAndPrefs(ctx, prefsOwnerDID, prefsOwnerHandle, pds.URL) + assert.Equal(t, "did:web:default.atcr.io", holdDID, "a failed fetch should degrade to the default hold") + assert.False(t, prefs.AutoRemoveUntagged) + assert.Equal(t, int64(1), calls.Load()) + + found, _, _ := readCachedPrefs(t, database) + assert.False(t, found, "a transient error must not be written down as the user's preference") + + _, _ = resolver.findHoldDIDAndPrefs(ctx, prefsOwnerDID, prefsOwnerHandle, pds.URL) + assert.Equal(t, int64(2), calls.Load(), "an uncached failure should be retried on the next request") +} + +// TestFindHoldDIDAndPrefs_FailedFetchLeavesExistingRowAlone is the same rule +// applied to a user who already has a row: the blip must not clear the hold +// they are actually using, nor stamp a guessed auto-remove over the unknown. +func TestFindHoldDIDAndPrefs_FailedFetchLeavesExistingRowAlone(t *testing.T) { + database := prefsTestDB(t) + seedUser(t, database, "did:web:user.hold.io", nil) + + pds, _ := profilePDS(t, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + }) + + resolver := &NamespaceResolver{ + defaultHoldDID: "did:web:default.atcr.io", + userPrefs: db.NewHoldDIDDB(database), + } + + _, _ = resolver.findHoldDIDAndPrefs(context.Background(), prefsOwnerDID, prefsOwnerHandle, pds.URL) + + found, cachedHold, cachedAuto := readCachedPrefs(t, database) + require.True(t, found) + assert.Equal(t, "did:web:user.hold.io", cachedHold.String, "a failed fetch must not clear the cached hold") + assert.False(t, cachedAuto.Valid, "a failed fetch must leave the unknown unknown") +} diff --git a/pkg/appview/middleware/registry.go b/pkg/appview/middleware/registry.go index 2833b06..ccadb58 100644 --- a/pkg/appview/middleware/registry.go +++ b/pkg/appview/middleware/registry.go @@ -17,6 +17,7 @@ import ( "github.com/distribution/distribution/v3/registry/storage/driver" "github.com/distribution/reference" + "atcr.io/pkg/appview/db" "atcr.io/pkg/appview/readme" "atcr.io/pkg/appview/storage" "atcr.io/pkg/atproto" @@ -176,6 +177,16 @@ type LabelChecker interface { IsTakenDown(did, repository string) (bool, error) } +// UserPrefsCache reads and writes the appview's local copy of the two sailor +// profile fields the registry hot path needs. It is kept current by the +// Jetstream processor and prefilled by the startup backfill; the middleware +// only writes to it on the one-shot fallback for a user it has never seen. +// Implemented by db.HoldDIDDB. +type UserPrefsCache interface { + GetUserHoldPrefs(did string) (db.UserHoldPrefs, error) + CacheUserHoldPrefs(did, handle, pdsEndpoint, holdDID string, autoRemoveUntagged bool) error +} + // Global variables for initialization only // These are set by main.go during startup and copied into NamespaceResolver instances. // After initialization, request handling uses the NamespaceResolver's instance fields. @@ -186,6 +197,7 @@ var ( globalWebhookDispatcher storage.PushWebhookDispatcher globalManifestRefChecker storage.ManifestReferenceChecker globalLabelChecker LabelChecker + globalUserPrefs UserPrefsCache ) // SetGlobalRefresher sets the OAuth refresher instance during initialization @@ -200,6 +212,13 @@ func SetGlobalDatabase(database storage.HoldDIDLookup) { globalDatabase = database } +// SetGlobalUserPrefs sets the cached sailor-profile preference store during +// initialization. Must be called before the registry starts serving requests. +// Leaving it nil is safe but costs a live profile fetch on every request. +func SetGlobalUserPrefs(prefs UserPrefsCache) { + globalUserPrefs = prefs +} + // SetGlobalManifestRefChecker sets the manifest reference checker during initialization func SetGlobalManifestRefChecker(checker storage.ManifestReferenceChecker) { globalManifestRefChecker = checker @@ -248,6 +267,7 @@ type NamespaceResolver struct { manifestRefChecker storage.ManifestReferenceChecker // Manifest reference checker (copied from global on init) validationCache *validationCache // Request-level service token cache readmeFetcher *readme.Fetcher // README fetcher for repo pages + userPrefs UserPrefsCache // Cached sailor profile preferences (copied from global on init) } // initATProtoResolver initializes the name resolution middleware @@ -285,6 +305,7 @@ func initATProtoResolver(ctx context.Context, ns distribution.Namespace, _ drive manifestRefChecker: globalManifestRefChecker, validationCache: newValidationCache(), readmeFetcher: readme.NewFetcher(), + userPrefs: globalUserPrefs, }, nil } @@ -405,8 +426,8 @@ func (nr *NamespaceResolver) Repository(ctx context.Context, name reference.Name } // Query for hold DID - either user's hold or default hold service - // Also returns the sailor profile so we can read preferences (e.g. AutoRemoveUntagged) - holdDID, sailorProfile := nr.findHoldDIDAndProfile(ctx, did, pdsEndpoint) + // Also returns the cached profile preferences (e.g. AutoRemoveUntagged) + holdDID, prefs := nr.findHoldDIDAndPrefs(ctx, did, handle, pdsEndpoint) if holdDID == "" { // A fatal configuration error: the registry cannot function without a // hold service, so a 5xx is honest here. It still has to be a coded @@ -631,7 +652,7 @@ func (nr *NamespaceResolver) Repository(ctx context.Context, name reference.Name PullerPDSEndpoint: pullerPDSEndpoint, // Puller's PDS for service token refresh HasPushScope: hasPushScope, // Whether JWT has push scope (for pull stats filtering) Anonymous: pullerDID == "", // No puller identity: hold decides via captain.Public - AutoRemoveUntagged: sailorProfile != nil && sailorProfile.AutoRemoveUntagged, + AutoRemoveUntagged: prefs.AutoRemoveUntagged, Database: nr.database, Authorizer: nr.authorizer, Refresher: nr.refresher, @@ -658,39 +679,109 @@ func (nr *NamespaceResolver) BlobStatter() distribution.BlobStatter { return nr.Namespace.BlobStatter() } -// findHoldDIDAndProfile determines which hold DID to use for blob storage and -// returns the user's sailor profile (if available) for reading preferences like -// AutoRemoveUntagged without an extra PDS call. -// Priority order: -// 1. User's sailor profile defaultHold (if set) +// holdPrefs carries the only two sailor profile fields the registry hot path +// reads. The full profile record is not needed here, and fetching it was the +// last per-request PDS round trip on the push path that had nothing to do with +// moving bytes. +type holdPrefs struct { + // AutoRemoveUntagged is whether a tag overwrite deletes the manifest that + // lost its last tag. + AutoRemoveUntagged bool +} + +// findHoldDIDAndPrefs determines which hold DID to use for blob storage and +// returns the owner's cached profile preferences. +// +// The answer comes from the local `users` row, which Jetstream keeps current +// (ProcessSailorProfile) and the startup backfill prefills. That row is read +// once per request instead of doing a com.atproto.repo.getRecord against the +// owner's PDS on every HEAD, POST, PATCH, PUT and GET under /v2/, which on a +// ten-layer push was forty-odd round trips. +// +// Priority order is unchanged: +// 1. The user's cached defaultHold (if set) // 2. AppView's default hold DID -// Returns a hold DID (e.g., "did:web:hold01.atcr.io"), or empty string if none configured -func (nr *NamespaceResolver) findHoldDIDAndProfile(ctx context.Context, did, pdsEndpoint string) (string, *atproto.SailorProfileRecord) { - // Create ATProto client (without auth - reading public records) +// +// Returns a hold DID (e.g., "did:web:hold01.atcr.io"), or empty string if none +// configured anywhere. +func (nr *NamespaceResolver) findHoldDIDAndPrefs(ctx context.Context, did, handle, pdsEndpoint string) (string, holdPrefs) { + if nr.userPrefs != nil { + prefs, err := nr.userPrefs.GetUserHoldPrefs(did) + if err != nil { + slog.Warn("Failed to read cached hold preferences, falling back to a live profile fetch", + "component", "registry/middleware", "did", did, "error", err) + } else if prefs.Found && prefs.AutoRemoveUntagged.Valid { + // Both fields are known locally. The cached defaultHold was already + // normalized to a DID by whoever wrote it, so no URL-to-DID + // migration is needed on this path. + return nr.applyTestModeFallback(ctx, prefs.DefaultHoldDID), + holdPrefs{AutoRemoveUntagged: prefs.AutoRemoveUntagged.Bool} + } + } + + // The row is missing, or auto_remove_untagged is still NULL ("never + // learned"). Learn it from the PDS once and write it down. + return nr.learnHoldPrefs(ctx, did, handle, pdsEndpoint) +} + +// learnHoldPrefs fetches the sailor profile live, exactly once per user, and +// caches what it finds so no later request has to. +// +// This one mechanism covers every way the local cache can have nothing to say: +// the minutes after a deploy while the startup backfill fills a new column, a +// brand-new user with no row at all, and a user the backfill has not reached. +func (nr *NamespaceResolver) learnHoldPrefs(ctx context.Context, did, handle, pdsEndpoint string) (string, holdPrefs) { + // Unauthenticated client: the sailor profile is a public record. client := atproto.NewClient(pdsEndpoint, did, "") - // Check for sailor profile profile, err := storage.GetProfile(ctx, client) if err != nil { - // Error reading profile (not a 404) - log and continue - slog.Warn("Failed to read profile", "did", did, "error", err) + // A network failure is not an answer. Serve this request from the + // appview default and write nothing, so a transient error does not get + // cached as the user's preference and silence the fallback forever. + slog.Warn("Failed to read profile, using default hold for this request", + "component", "registry/middleware", "did", did, "error", err) + return nr.defaultHoldDID, holdPrefs{} } - if profile != nil && profile.DefaultHold != "" { - // Profile exists with defaultHold set - // In test mode, verify it's reachable before using it - if nr.testMode { - if nr.isHoldReachable(ctx, profile.DefaultHold) { - return profile.DefaultHold, profile - } - slog.Debug("User's defaultHold unreachable, falling back to default", "component", "registry/middleware/testmode", "default_hold", profile.DefaultHold) - return nr.defaultHoldDID, profile + // A missing profile (404) is an answer: no custom hold, no auto-remove. + // It is cached like any other, so the fallback does not repeat for a user + // who has never written a profile record. + holdDID, autoRemove := "", false + if profile != nil { + holdDID = profile.DefaultHold + autoRemove = profile.AutoRemoveUntagged + } + + slog.Debug("Sailor profile not cached locally, fetched live and caching", + "component", "registry/middleware", "did", did, + "default_hold", holdDID, "auto_remove_untagged", autoRemove, + "profile_exists", profile != nil) + + if nr.userPrefs != nil { + if err := nr.userPrefs.CacheUserHoldPrefs(did, handle, pdsEndpoint, holdDID, autoRemove); err != nil { + slog.Warn("Failed to cache hold preferences; the live fetch will repeat", + "component", "registry/middleware", "did", did, "error", err) } - return profile.DefaultHold, profile } - // No profile defaultHold - use AppView default - return nr.defaultHoldDID, profile + return nr.applyTestModeFallback(ctx, holdDID), holdPrefs{AutoRemoveUntagged: autoRemove} +} + +// applyTestModeFallback turns a user's chosen hold into the hold to actually +// use. An empty choice means the appview default. In test mode a chosen hold +// that is not answering also falls back, so a developer whose local hold is +// down can still push. +func (nr *NamespaceResolver) applyTestModeFallback(ctx context.Context, userHoldDID string) string { + if userHoldDID == "" { + return nr.defaultHoldDID + } + if nr.testMode && !nr.isHoldReachable(ctx, userHoldDID) { + slog.Debug("User's defaultHold unreachable, falling back to default", + "component", "registry/middleware/testmode", "default_hold", userHoldDID) + return nr.defaultHoldDID + } + return userHoldDID } // resolveSuccessor checks if a hold has declared a successor and returns it. diff --git a/pkg/appview/middleware/registry_test.go b/pkg/appview/middleware/registry_test.go index 83304ab..d19afe1 100644 --- a/pkg/appview/middleware/registry_test.go +++ b/pkg/appview/middleware/registry_test.go @@ -163,7 +163,7 @@ func TestFindHoldDID_DefaultFallback(t *testing.T) { } ctx := context.Background() - holdDID, _ := resolver.findHoldDIDAndProfile(ctx, "did:plc:test123", mockPDS.URL) + holdDID, _ := resolver.findHoldDIDAndPrefs(ctx, "did:plc:test123", "test.example.com", mockPDS.URL) assert.Equal(t, "did:web:default.atcr.io", holdDID, "should fall back to default hold DID") } @@ -191,7 +191,7 @@ func TestFindHoldDID_SailorProfile(t *testing.T) { } ctx := context.Background() - holdDID, _ := resolver.findHoldDIDAndProfile(ctx, "did:plc:test123", mockPDS.URL) + holdDID, _ := resolver.findHoldDIDAndPrefs(ctx, "did:plc:test123", "test.example.com", mockPDS.URL) assert.Equal(t, "did:web:user.hold.io", holdDID, "should use sailor profile's defaultHold") } @@ -218,7 +218,7 @@ func TestFindHoldDID_Priority(t *testing.T) { } ctx := context.Background() - holdDID, _ := resolver.findHoldDIDAndProfile(ctx, "did:plc:test123", mockPDS.URL) + holdDID, _ := resolver.findHoldDIDAndPrefs(ctx, "did:plc:test123", "test.example.com", mockPDS.URL) // Profile should take priority over hold records and default assert.Equal(t, "did:web:profile.hold.io", holdDID, "should prioritize sailor profile over hold records") @@ -247,7 +247,7 @@ func TestFindHoldDID_TestModeFallback(t *testing.T) { } ctx := context.Background() - holdDID, _ := resolver.findHoldDIDAndProfile(ctx, "did:plc:test123", mockPDS.URL) + holdDID, _ := resolver.findHoldDIDAndPrefs(ctx, "did:plc:test123", "test.example.com", mockPDS.URL) // In test mode with unreachable hold, should fall back to default assert.Equal(t, "did:web:default.atcr.io", holdDID, "should fall back to default in test mode when hold unreachable") diff --git a/pkg/appview/server.go b/pkg/appview/server.go index 8214cda..c20be13 100644 --- a/pkg/appview/server.go +++ b/pkg/appview/server.go @@ -270,6 +270,7 @@ func NewAppViewServer(cfg *Config, branding *BrandingOverrides) (*AppViewServer, holdDIDDB := db.NewHoldDIDDB(s.Database) middleware.SetGlobalDatabase(holdDIDDB) middleware.SetGlobalManifestRefChecker(holdDIDDB) + middleware.SetGlobalUserPrefs(holdDIDDB) // Set label checker for takedown filtering middleware.SetGlobalLabelChecker(db.NewLabelChecker(s.Database))