From 894dd243dacd8b20459b6656d729d17b589f03f6 Mon Sep 17 00:00:00 2001 From: Evan Jarrett Date: Mon, 24 Aug 2026 21:21:31 -0500 Subject: [PATCH] hold/admin: cover the top-users panel 7d9de7c fixed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 7d9de7c moved handle resolution below the sort-and-truncate, so a hold with ~500 crew stopped making ~500 serial identity lookups to render ten rows. It shipped without a test, and the regression is a two-line move. The property pinned here is the LOOKUP COUNT, not the wall clock. Timing would pass or fail on how fast the machine is; the count fails precisely when resolution moves back above the truncation. Mutation-verified: restoring the old shape produces 60 lookups for 10 rendered rows against a 50-user hold, and the failure message names the cause. The second test covers the 3s resolve deadline the same commit added — a stalled lookup must degrade to a bare DID rather than consume the reverse proxy budget, which is what left the client hanging up mid-render before. resolveHandle becomes a package var, since counting lookups is the only way to observe either property from outside. Two things worth knowing for the next test in this package. AdminUI.pds is a concrete *pds.HoldPDS, so this needed a real one: NewHoldPDS with a file-backed path (":memory:" is per-connection in libsql and disables the records index QuotasByDID reads), then Bootstrap, or the first record write fails with "cannot serialize undefined cid". And BatchCreateLayerRecords writes only to the CAR store — the records index is fed from the repo event stream, which is not running in a test, so BackfillRecordsIndex has to be called explicitly or the quota query returns nothing and the count assertion passes vacuously at zero. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VwxF2N3HuZ8xSkx6nkirgB --- pkg/hold/admin/handlers_crew.go | 6 +- pkg/hold/admin/top_users_test.go | 136 +++++++++++++++++++++++++++++++ 2 files changed, 141 insertions(+), 1 deletion(-) create mode 100644 pkg/hold/admin/top_users_test.go diff --git a/pkg/hold/admin/handlers_crew.go b/pkg/hold/admin/handlers_crew.go index 012ec0c..d64bb50 100644 --- a/pkg/hold/admin/handlers_crew.go +++ b/pkg/hold/admin/handlers_crew.go @@ -32,7 +32,11 @@ type CrewMemberView struct { // resolveHandle attempts to resolve a DID to a handle // Returns empty string if resolution fails -func resolveHandle(ctx context.Context, did string) string { +// resolveHandle is a package var so tests can count how many identity lookups a +// panel performs. The count is the property worth pinning on the dashboard: +// resolving before the limit is applied scales with hold size, not with the +// number of rows rendered. +var resolveHandle = func(ctx context.Context, did string) string { _, handle, _, err := atproto.ResolveIdentity(ctx, did) if err != nil { slog.Debug("Failed to resolve handle for DID", "did", did, "error", err) diff --git a/pkg/hold/admin/top_users_test.go b/pkg/hold/admin/top_users_test.go new file mode 100644 index 0000000..2868417 --- /dev/null +++ b/pkg/hold/admin/top_users_test.go @@ -0,0 +1,136 @@ +package admin + +import ( + "context" + "fmt" + "net/http/httptest" + "path/filepath" + "sync/atomic" + "testing" + "time" + + "atcr.io/pkg/atproto" + "atcr.io/pkg/hold/pds" +) + +// The dashboard's top-users panel used to resolve a handle for every user with +// a quota record before sorting and truncating to ten. On a hold with ~500 crew +// that is ~500 serial identity lookups to render ten rows, each allowed 10s by +// the shared directory's HTTP client — one stall consumed the whole reverse +// proxy budget and the client hung up mid-render. +// +// The property worth pinning is the LOOKUP COUNT, not the wall clock. Timing +// would pass or fail on how fast the machine is; the count fails precisely when +// someone moves resolution back above the truncation. + +// seededPDS returns a hold PDS carrying one layer record for each of n distinct +// user DIDs, which is what GetAllUserQuotas derives its rows from. +// +// A file-backed path, not ":memory:" — libsql scopes an in-memory database to a +// single connection, and the records index that QuotasByDID reads is disabled +// without it. +func seededPDS(t *testing.T, n int) *pds.HoldPDS { + t.Helper() + dir := t.TempDir() + p, err := pds.NewHoldPDS( + context.Background(), + "did:web:top-users-test", + "http://127.0.0.1:0", "http://127.0.0.1:0", + filepath.Join(dir, "hold.db"), filepath.Join(dir, "signing.key"), + false, + ) + if err != nil { + t.Fatalf("NewHoldPDS: %v", err) + } + + // Bootstrap writes the repo root; without it the first record write fails + // with "cannot serialize undefined cid". Bootstrap prints a banner to + // stdout, which is noise here but harmless. + if err := p.Bootstrap(context.Background(), nil, pds.BootstrapConfig{ + OwnerDID: "did:plc:topuserstestowner", + Public: true, + }); err != nil { + t.Fatalf("Bootstrap: %v", err) + } + + records := make([]*atproto.LayerRecord, 0, n) + for i := range n { + did := fmt.Sprintf("did:plc:topuserstest%08d", i) + records = append(records, atproto.NewLayerRecord( + fmt.Sprintf("sha256:%064x", i), + int64((i+1)*1024), + "application/vnd.oci.image.layer.v1.tar+gzip", + did, + "at://"+did+"/io.atcr.manifest/m"+fmt.Sprint(i), + )) + } + if _, err := p.BatchCreateLayerRecords(context.Background(), records); err != nil { + t.Fatalf("seed layer records: %v", err) + } + + // BatchCreateLayerRecords writes to the CAR store only. The records index + // that QuotasByDID reads is fed from the repo event stream, which is not + // running here, so index it explicitly. + if err := p.BackfillRecordsIndex(context.Background()); err != nil { + t.Fatalf("backfill records index: %v", err) + } + return p +} + +// TestTopUsers_ResolvesOnlyTheRowsItRenders is the regression guard: with 50 +// users on the hold and a limit of 10, exactly 10 identity lookups may happen. +func TestTopUsers_ResolvesOnlyTheRowsItRenders(t *testing.T) { + const users, limit = 50, 10 + + var lookups atomic.Int32 + prev := resolveHandle + resolveHandle = func(ctx context.Context, did string) string { + lookups.Add(1) + return "handle-for-" + did + } + t.Cleanup(func() { resolveHandle = prev }) + + ui := &AdminUI{pds: seededPDS(t, users)} + + req := httptest.NewRequest("GET", fmt.Sprintf("/admin/api/top-users?limit=%d", limit), nil) + rr := httptest.NewRecorder() + ui.handleTopUsersAPI(rr, req) + + if rr.Code != 200 { + t.Fatalf("status = %d, want 200 (body %q)", rr.Code, rr.Body.String()) + } + if got := lookups.Load(); got != limit { + t.Errorf("performed %d identity lookups to render %d rows (hold has %d users); "+ + "resolution is scaling with hold size again, not with the limit", got, limit, users) + } +} + +// TestTopUsers_SlowLookupDegradesToBareDID: resolution runs under its own +// deadline, so one stalled lookup renders a bare DID instead of taking the +// request down with it. Without the bound this blocks until the directory's own +// 10s timeout, which is the entire reverse proxy budget. +func TestTopUsers_SlowLookupDegradesToBareDID(t *testing.T) { + prev := resolveHandle + resolveHandle = func(ctx context.Context, did string) string { + <-ctx.Done() // never answers; only the deadline releases it + return "" + } + t.Cleanup(func() { resolveHandle = prev }) + + ui := &AdminUI{pds: seededPDS(t, 3)} + + req := httptest.NewRequest("GET", "/admin/api/top-users?limit=3", nil) + rr := httptest.NewRecorder() + + start := time.Now() + ui.handleTopUsersAPI(rr, req) + elapsed := time.Since(start) + + if rr.Code != 200 { + t.Fatalf("status = %d, want 200", rr.Code) + } + // Generous bound: asserting the panel returns at all rather than timing it. + if elapsed > topUsersResolveTimeout+3*time.Second { + t.Errorf("panel took %v with stalled lookups; the resolve deadline did not apply", elapsed) + } +}