From c53bd41787a4337d7a655aa36ba23deacf7ec55d Mon Sep 17 00:00:00 2001 From: Matt Kane Date: Mon, 25 May 2026 08:02:13 +0100 Subject: [PATCH 1/2] fix(pds): return 400 RecordNotFound and treat deleteRecord as no-op Match the reference @atproto PDS: getRecord raises InvalidRequestError (HTTP 400) with RecordNotFound, and deleteRecord on a missing record is a 200 no-op rather than an error. Adds a pdscheck for the getRecord status code. --- .changeset/fix-record-not-found-status.md | 8 +++ apps/check/src/checks/repo-read.ts | 75 +++++++++++++++++++++++ packages/pds/src/xrpc/repo.ts | 14 +---- packages/pds/test/xrpc.test.ts | 19 +++--- 4 files changed, 92 insertions(+), 24 deletions(-) create mode 100644 .changeset/fix-record-not-found-status.md diff --git a/.changeset/fix-record-not-found-status.md b/.changeset/fix-record-not-found-status.md new file mode 100644 index 00000000..7a95978d --- /dev/null +++ b/.changeset/fix-record-not-found-status.md @@ -0,0 +1,8 @@ +--- +"@getcirrus/pds": patch +--- + +Align `com.atproto.repo.getRecord` and `com.atproto.repo.deleteRecord` error handling with the reference @atproto PDS: + +- `getRecord` now returns HTTP 400 (not 404) with `RecordNotFound` when the record is missing. The reference PDS raises `InvalidRequestError`, which maps to 400. +- `deleteRecord` on a missing record is now a 200 no-op (returning an empty body with no commit) instead of `RecordNotFound`, matching the reference PDS. diff --git a/apps/check/src/checks/repo-read.ts b/apps/check/src/checks/repo-read.ts index 79793a86..d793b09a 100644 --- a/apps/check/src/checks/repo-read.ts +++ b/apps/check/src/checks/repo-read.ts @@ -564,6 +564,80 @@ const getRecordValidates: Check = { }, }; +const getRecordMissing: Check = { + id: "repo-read.get-record-missing", + category: "repo-read", + label: "getRecord returns 400 RecordNotFound for missing record", + description: + "Matches the reference @atproto PDS, which raises InvalidRequestError (HTTP 400) with error 'RecordNotFound' rather than returning a 404. Clients that probe a record before writing it (for example detaching a quote, which checks app.bsky.feed.postgate first) rely on this.", + requires: ["pds", "did"], + run: async (ctx): Promise => { + const pds = ctx.pds!; + const did = ctx.did!; + const collection = sampleRecord?.collection ?? "app.bsky.feed.post"; + const rkey = `pdscheck-missing-${Date.now().toString(36)}`; + const url = xrpcUrl(pds, "com.atproto.repo.getRecord", { + repo: did, + collection, + rkey, + }); + try { + const res = await fetch(url); + let body: unknown; + try { + body = await res.json(); + } catch { + body = await res.text().catch(() => undefined); + } + if (res.status !== 400) { + return { + status: "fail", + message: `Expected 400, got ${res.status}`, + evidence: { + request: { method: "GET", url }, + response: { status: res.status, body }, + expected: 400, + actual: res.status, + }, + }; + } + const error = + body && typeof body === "object" && "error" in body + ? (body as { error: unknown }).error + : undefined; + if (error !== "RecordNotFound") { + return { + status: "fail", + message: `Expected error 'RecordNotFound', got ${JSON.stringify(error)}`, + evidence: { + request: { method: "GET", url }, + response: { status: res.status, body }, + expected: "RecordNotFound", + actual: error, + }, + }; + } + return { + status: "pass", + message: "400 RecordNotFound", + evidence: { + request: { method: "GET", url }, + response: { status: res.status, body }, + }, + }; + } catch (error) { + return { + status: "fail", + message: error instanceof Error ? error.message : String(error), + evidence: { + request: { method: "GET", url }, + error: String(error), + }, + }; + } + }, +}; + const getRepoCarValidates: Check = { id: "repo-read.get-repo-car.validates", category: "repo-read", @@ -635,6 +709,7 @@ export const repoReadChecks: Check[] = [ listRecordsValidates, getRecord, getRecordValidates, + getRecordMissing, listRecordsCursor, getRepoCar, getRepoCarValidates, diff --git a/packages/pds/src/xrpc/repo.ts b/packages/pds/src/xrpc/repo.ts index abd239ae..c4169cd0 100644 --- a/packages/pds/src/xrpc/repo.ts +++ b/packages/pds/src/xrpc/repo.ts @@ -194,7 +194,7 @@ export async function getRecord( error: "RecordNotFound", message: `Record not found: ${collection}/${rkey}`, }, - 404, + 400, ); } @@ -372,17 +372,7 @@ export async function deleteRecord( try { const result = await accountDO.rpcDeleteRecord(collection, rkey); - if (!result) { - return c.json( - { - error: "RecordNotFound", - message: `Record not found: ${collection}/${rkey}`, - }, - 404, - ); - } - - return c.json(result); + return c.json(result ?? {}); } catch (err) { const deactivatedError = checkAccountDeactivatedError(c, err); if (deactivatedError) return deactivatedError; diff --git a/packages/pds/test/xrpc.test.ts b/packages/pds/test/xrpc.test.ts index 1b9ef04c..9d037f0f 100644 --- a/packages/pds/test/xrpc.test.ts +++ b/packages/pds/test/xrpc.test.ts @@ -631,10 +631,10 @@ describe("XRPC Endpoints", () => { ), env, ); - expect(getResponse.status).toBe(404); + expect(getResponse.status).toBe(400); }); - it("should handle deleting non-existent record", async () => { + it("should handle deleting non-existent record as a no-op", async () => { const response = await worker.fetch( new Request("http://pds.test/xrpc/com.atproto.repo.deleteRecord", { method: "POST", @@ -650,22 +650,17 @@ describe("XRPC Endpoints", () => { }), env, ); - expect(response.status).toBe(404); - - const data = await response.json(); - expect(data).toMatchObject({ - error: "RecordNotFound", - }); + expect(response.status).toBe(200); }); - it("should return 404 for non-existent record", async () => { + it("should return 400 RecordNotFound for non-existent record", async () => { const response = await worker.fetch( new Request( `http://pds.test/xrpc/com.atproto.repo.getRecord?repo=${env.DID}&collection=app.bsky.feed.post&rkey=does-not-exist`, ), env, ); - expect(response.status).toBe(404); + expect(response.status).toBe(400); const data = await response.json(); expect(data).toMatchObject({ @@ -1032,7 +1027,7 @@ describe("XRPC Endpoints", () => { ), env, ); - expect(getResponse.status).toBe(404); + expect(getResponse.status).toBe(400); }); it("should handle mixed operations", async () => { @@ -1306,7 +1301,7 @@ describe("XRPC Endpoints", () => { ), env, ); - expect(getResponse.status).toBe(404); + expect(getResponse.status).toBe(400); }); it("rejects two creates for the same rkey as 409 RecordAlreadyExists", async () => { From 20e8de5c3d1ed28f7e16c83d87793ae38ab09b9b Mon Sep 17 00:00:00 2001 From: Matt Kane Date: Mon, 25 May 2026 08:06:59 +0100 Subject: [PATCH 2/2] test(pds): update e2e tests for new RecordNotFound semantics --- packages/pds/e2e/crud.e2e.ts | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/packages/pds/e2e/crud.e2e.ts b/packages/pds/e2e/crud.e2e.ts index 6713afa5..0a873283 100644 --- a/packages/pds/e2e/crud.e2e.ts +++ b/packages/pds/e2e/crud.e2e.ts @@ -87,14 +87,14 @@ describe("CRUD Operations", () => { expect(getResult.data.cid).toBe(createResult.data.cid); }); - it("returns 404 for non-existent record", async () => { + it("returns RecordNotFound for non-existent record", async () => { await expect( agent.com.atproto.repo.getRecord({ repo: TEST_DID, collection: "app.bsky.feed.post", rkey: "non-existent-rkey-12345", }), - ).rejects.toThrow(); + ).rejects.toMatchObject({ error: "RecordNotFound", status: 400 }); }); }); @@ -188,16 +188,14 @@ describe("CRUD Operations", () => { ).rejects.toThrow(); }); - it("throws when deleting non-existent record", async () => { - // AT Protocol spec allows this to throw or no-op - // Our implementation throws RecordNotFound - await expect( - agent.com.atproto.repo.deleteRecord({ - repo: TEST_DID, - collection: "app.bsky.feed.post", - rkey: "non-existent-rkey-67890", - }), - ).rejects.toThrow(); + it("treats deleting a non-existent record as a no-op", async () => { + const res = await agent.com.atproto.repo.deleteRecord({ + repo: TEST_DID, + collection: "app.bsky.feed.post", + rkey: "non-existent-rkey-67890", + }); + expect(res.success).toBe(true); + expect(res.data.commit).toBeUndefined(); }); });