diff --git a/packages/contrail/src/core/backfill.ts b/packages/contrail/src/core/backfill.ts index 559131a..9b46d3a 100644 --- a/packages/contrail/src/core/backfill.ts +++ b/packages/contrail/src/core/backfill.ts @@ -350,16 +350,14 @@ export async function backfillPending( const dids = [...byDid.keys()]; - // Resolve PDS endpoints in background (populates in-memory cache) - const resolvePromise = (async () => { - for (let i = 0; i < dids.length; i += 200) { - await Promise.allSettled( - dids.slice(i, i + 200).map((did) => - getPDS(did as Did, db).catch(() => {}) - ) - ); - } - })(); + // Resolve PDS endpoints (populates in-memory cache) + for (let i = 0; i < dids.length; i += 200) { + await Promise.allSettled( + dids.slice(i, i + 200).map((did) => + getPDS(did as Did, db).catch(() => {}) + ) + ); + } let roundBackfilled = 0; let usersComplete = 0; @@ -471,7 +469,6 @@ export async function backfillPending( } } - await resolvePromise; totalBackfilled += roundBackfilled; // If nothing was backfilled this round, we're stuck -- 2.51.2 From 4c8fedb5cd45e6bdcf5a0455b5020fd1290e4cb6 Mon Sep 17 00:00:00 2001 From: Florian <45694132+flo-bit@users.noreply.github.com> Date: Sun, 7 Jun 2026 20:26:14 +0200 Subject: [PATCH 2/2] identity fix, heal missing handles --- .changeset/identity-handle-resolution.md | 25 ++++ .../contrail-appview/src/core/backfill.ts | 23 ++-- packages/contrail-base/src/client.ts | 19 ++- packages/contrail-base/src/identity.ts | 6 +- .../contrail/tests/identity-handle.test.ts | 114 ++++++++++++++++++ 5 files changed, 170 insertions(+), 17 deletions(-) create mode 100644 .changeset/identity-handle-resolution.md create mode 100644 packages/contrail/tests/identity-handle.test.ts diff --git a/.changeset/identity-handle-resolution.md b/.changeset/identity-handle-resolution.md new file mode 100644 index 0000000..2226180 --- /dev/null +++ b/.changeset/identity-handle-resolution.md @@ -0,0 +1,25 @@ +--- +"@atmo-dev/contrail-base": patch +--- + +fix(identity): stop stranding/clobbering handles during resolution (#42) + +Backfill left a meaningful fraction of identities with a PDS but no handle. +Two root causes: + +- `resolvePDSCached` short-circuited on any row with a non-null PDS and returned + without ever resolving the handle. A partial resolution (slingshot can return + a PDS without a handle under load) was therefore persisted and never healed. + It now treats a row as a complete cache hit only when both PDS *and* handle are + present; a PDS-only row falls through to re-resolve and fill the handle, while + still serving the known PDS (including if the re-resolution fails). +- `saveIdentity` overwrote `handle`/`pds` unconditionally, so + `refreshStaleIdentities` (which passes a null handle through when slingshot + omits one) could clobber a previously-resolved handle with null. The upsert now + COALESCEs both columns: a fresh non-null value still applies (handle changes + work), but a null never nulls a good value. + +Backfill also resolves PDS endpoints up front instead of in a detached +background promise, so identity resolution no longer competes with record +backfill for slingshot — reducing the partial responses that triggered the +above in the first place. diff --git a/packages/contrail-appview/src/core/backfill.ts b/packages/contrail-appview/src/core/backfill.ts index f7fdd8b..e75eb62 100644 --- a/packages/contrail-appview/src/core/backfill.ts +++ b/packages/contrail-appview/src/core/backfill.ts @@ -351,16 +351,18 @@ export async function backfillPending( const dids = [...byDid.keys()]; - // Resolve PDS endpoints in background (populates in-memory cache) - const resolvePromise = (async () => { - for (let i = 0; i < dids.length; i += 200) { - await Promise.allSettled( - dids.slice(i, i + 200).map((did) => - getPDS(did as Did, db, config).catch(() => {}) - ) - ); - } - })(); + // Resolve PDS endpoints up front (populates the in-memory cache) rather + // than concurrently with the backfill passes below. Overlapping the two put + // identity resolution and record backfill in contention for slingshot at + // once, and the partial responses that produced (a PDS without a handle) + // got persisted and stranded. Resolving first keeps that load separate. + for (let i = 0; i < dids.length; i += 200) { + await Promise.allSettled( + dids.slice(i, i + 200).map((did) => + getPDS(did as Did, db, config).catch(() => {}) + ) + ); + } let roundBackfilled = 0; let usersComplete = 0; @@ -472,7 +474,6 @@ export async function backfillPending( } } - await resolvePromise; totalBackfilled += roundBackfilled; // If nothing was backfilled this round, we're stuck diff --git a/packages/contrail-base/src/client.ts b/packages/contrail-base/src/client.ts index cd154ef..10c89fa 100644 --- a/packages/contrail-base/src/client.ts +++ b/packages/contrail-base/src/client.ts @@ -196,23 +196,32 @@ async function resolvePDSCached( db?: Database, config?: ContrailConfig, ): Promise { + let knownPds: string | undefined; if (db) { const cached = await db - .prepare("SELECT pds FROM identities WHERE did = ? AND pds IS NOT NULL") + .prepare("SELECT pds, handle FROM identities WHERE did = ? AND pds IS NOT NULL") .bind(did) - .first<{ pds: string }>(); + .first<{ pds: string; handle: string | null }>(); if (cached?.pds) { pdsCacheSet(did, cached.pds); - return cached.pds; + // A row with both a PDS and a handle is a complete cache hit. A row with + // a PDS but no handle is a *partial* resolution — slingshot can return a + // PDS without a handle under load — so fall through to re-resolve and + // fill the handle instead of stranding the row forever (this DB + // short-circuit previously meant the handle was never backfilled). We + // keep serving the known PDS meanwhile, including if the re-resolve fails. + if (cached.handle) return cached.pds; + knownPds = cached.pds; } } const resolved = await resolvePDS(did, config); - if (!resolved?.pds) return undefined; + if (!resolved?.pds) return knownPds; pdsCacheSet(did, resolved.pds); - // Persist to DB for future runs + // Persist to DB for future runs. COALESCE keeps an existing handle when this + // resolution didn't return one, and never nulls a good handle. if (db) { await db .prepare( diff --git a/packages/contrail-base/src/identity.ts b/packages/contrail-base/src/identity.ts index 26e84fd..a18120d 100644 --- a/packages/contrail-base/src/identity.ts +++ b/packages/contrail-base/src/identity.ts @@ -15,7 +15,11 @@ export interface Identity { async function saveIdentity(db: Database, identity: Identity): Promise { await db .prepare( - "INSERT INTO identities (did, handle, pds, resolved_at) VALUES (?, ?, ?, ?) ON CONFLICT(did) DO UPDATE SET handle = excluded.handle, pds = excluded.pds, resolved_at = excluded.resolved_at" + // COALESCE so a null handle/pds from a partial resolution never clobbers + // a previously-resolved value (e.g. refreshStaleIdentities passes through + // a null handle when slingshot omits it). A fresh non-null value still + // overwrites — handle changes apply normally. + "INSERT INTO identities (did, handle, pds, resolved_at) VALUES (?, ?, ?, ?) ON CONFLICT(did) DO UPDATE SET handle = COALESCE(excluded.handle, identities.handle), pds = COALESCE(excluded.pds, identities.pds), resolved_at = excluded.resolved_at" ) .bind(identity.did, identity.handle, identity.pds, identity.resolved_at) .run(); diff --git a/packages/contrail/tests/identity-handle.test.ts b/packages/contrail/tests/identity-handle.test.ts new file mode 100644 index 0000000..8d42f0c --- /dev/null +++ b/packages/contrail/tests/identity-handle.test.ts @@ -0,0 +1,114 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { getPDS, __resetPdsCachesForTests } from "../src/core/client"; +import { refreshStaleIdentities } from "../src/core/identity"; +import { createTestDbWithSchema } from "./helpers"; +import type { Database } from "../src/core/types"; +import type { Did } from "@atcute/lexicons"; + +// Root-cause coverage for PR #42: identities that end up with a PDS but no +// handle must not be stranded, and a partial re-resolution must never clobber a +// handle that was already known. + +const DID = "did:plc:aaaaaaaaaaaaaaaaaaaaaaaa" as Did; +const PDS = "https://pds.example.host.bsky.network"; + +let db: Database; +let fetchSpy: ReturnType; + +async function seedIdentity( + did: string, + handle: string | null, + pds: string | null, + resolvedAt: number +): Promise { + await db + .prepare( + "INSERT INTO identities (did, handle, pds, resolved_at) VALUES (?, ?, ?, ?)" + ) + .bind(did, handle, pds, resolvedAt) + .run(); +} + +async function readIdentity( + did: string +): Promise<{ handle: string | null; pds: string | null; resolved_at: number } | null> { + return db + .prepare("SELECT handle, pds, resolved_at FROM identities WHERE did = ?") + .bind(did) + .first(); +} + +function slingshotReturns(body: { did?: string; handle?: string; pds?: string }) { + fetchSpy.mockResolvedValue(new Response(JSON.stringify(body), { status: 200 })); +} + +beforeEach(async () => { + db = await createTestDbWithSchema(); + __resetPdsCachesForTests(); + fetchSpy = vi.spyOn(global, "fetch"); +}); + +afterEach(() => { + fetchSpy.mockRestore(); + __resetPdsCachesForTests(); +}); + +describe("getPDS heals a PDS-but-no-handle row", () => { + it("re-resolves and backfills the missing handle", async () => { + // A partial resolution persisted earlier: PDS known, handle null. + await seedIdentity(DID, null, PDS, Date.now()); + // Slingshot now returns the handle. + slingshotReturns({ did: DID, handle: "alice.test", pds: PDS }); + + const pds = await getPDS(DID, db); + + expect(pds).toBe(PDS); + expect(fetchSpy).toHaveBeenCalled(); // it did re-resolve, not strand + expect((await readIdentity(DID))?.handle).toBe("alice.test"); + }); + + it("still serves the known PDS if the heal resolution fails", async () => { + await seedIdentity(DID, null, PDS, Date.now()); + fetchSpy.mockRejectedValue(new Error("slingshot down")); + + const pds = await getPDS(DID, db); + + expect(pds).toBe(PDS); // known PDS still returned + expect((await readIdentity(DID))?.handle).toBeNull(); + }); + + it("does NOT re-resolve a complete row (PDS + handle)", async () => { + await seedIdentity(DID, "bob.test", PDS, Date.now()); + + const pds = await getPDS(DID, db); + + expect(pds).toBe(PDS); + expect(fetchSpy).not.toHaveBeenCalled(); // short-circuit preserved + expect((await readIdentity(DID))?.handle).toBe("bob.test"); + }); +}); + +describe("refreshStaleIdentities does not clobber a known handle", () => { + it("keeps the existing handle when slingshot omits one", async () => { + // Stale row with a good handle. + await seedIdentity(DID, "carol.test", PDS, 0); + // Partial response: PDS but no handle. + slingshotReturns({ did: DID, pds: PDS }); + + await refreshStaleIdentities(db, [DID]); + + const row = await readIdentity(DID); + expect(row?.handle).toBe("carol.test"); // preserved, not nulled + expect(row?.pds).toBe(PDS); + expect(row?.resolved_at).toBeGreaterThan(0); // refresh did run + }); + + it("still applies a changed handle (non-null overwrites)", async () => { + await seedIdentity(DID, "carol.test", PDS, 0); + slingshotReturns({ did: DID, handle: "carol.new", pds: PDS }); + + await refreshStaleIdentities(db, [DID]); + + expect((await readIdentity(DID))?.handle).toBe("carol.new"); + }); +});