From 546a12259185b66c0db333c69c164a2bf528f248 Mon Sep 17 00:00:00 2001 From: Bogdan Chadkin Date: Tue, 12 May 2026 00:44:56 +0700 Subject: [PATCH 1/2] fix: avoid race condition while resolving PDS I noticed about 1/4 of identities in my app did not have a handle after backfill. Looks like getPDS over dids run twice over the same set of dids creates race condition. One is from this loop, and the other getPDS is invoked from getClient function. I tested with my set of data and edited variant always gives 100% of handles in identities table. --- packages/contrail/src/core/backfill.ts | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) 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"); + }); +}); -- 2.51.2