fix(pds): return 400 RecordNotFound and treat deleteRecord as no-op (#189)

* 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.

* test(pds): update e2e tests for new RecordNotFound semantics

Author: ascorbic

.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.

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<CheckOutcome> => {
+		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,

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();
 		});
 	});
 

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;

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 () => {