Skip to content

fix(pds): include cid on applyWrites create/update results#181

Merged
ascorbic merged 1 commit into
mainfrom
fix/applywrites-transient-record
May 24, 2026
Merged

fix(pds): include cid on applyWrites create/update results#181
ascorbic merged 1 commit into
mainfrom
fix/applywrites-transient-record

Conversation

@ascorbic
Copy link
Copy Markdown
Owner

Summary

Two related fixes surfaced by the latest pdscheck run against pds.mk.gg, both stemming from PR #176 (accept create+delete on the same rkey).

Bug 1 — createResult.cid missing

The lexicon (com.atproto.repo.applyWrites#createResult) marks cid as required, but Cirrus was reading it from updatedRepo.data.get(dataKey) after applying the commit. For a record that was created then deleted within the same batch, the MST has no entry, so recordCid was undefined and the field was omitted from the response.

Fix: precompute the create/update record's CID via @atproto/repo's cidForRecord(op.record) before formatting the commit. The function encodes the record to DAG-CBOR and hashes deterministically — same algorithm the MST uses internally. This matches reference PDS behaviour (packages/pds/src/repo/prepare.ts:217: cid: await cidForCbor(encode(record))).

Bug 2 — pdscheck flags delete ops with no prev

Sync 1.1 requires prev on update/delete ops in the firehose. For a [create(K), delete(K)] batch, the delete has no prev because the record didn't exist pre-commit. Reference PDS reads prev from a pre-batch query (actor-store/repo/transactor.ts:125), so it also omits prev in this case.

Fix: pdscheck's firehose.commit-ops-have-prev check now exempts an update/delete op whose path was also created earlier in the same #commit. No change to Cirrus's firehose payload — its existing behaviour was already aligned with the reference.

Test plan

  • pnpm --filter @getcirrus/pds test — 313 unit + 84 CLI passing, including a strengthened assertion in the existing create+delete-same-rkey test that cid and uri are present on createResult.
  • pnpm --filter @getcirrus/pds check — clean.
  • apps/check typechecks.
  • Re-run pdscheck against the deployed PDS once this lands: expect repo-write.apply-writes.validates and firehose.commit-ops-have-prev to pass.

The lexicon marks `cid` as required on createResult and updateResult,
but the PDS was reading it from the post-commit MST. A record that was
created then deleted within the same batch leaves no MST entry, so the
field was missing. Precompute via cidForRecord(op.record), matching
reference PDS behaviour.

Also relaxes pdscheck's commit-ops-have-prev check: a delete or update
whose path was also created earlier in the same #commit has no prev
(the record didn't exist before the commit), and reference PDS doesn't
emit one. The check now exempts those ops instead of flagging them.
@cloudflare-workers-and-pages
Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
pdscheck 191ef18 May 24 2026, 08:32 PM

@cloudflare-workers-and-pages
Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
cirrusdocs 191ef18 Commit Preview URL

Branch Preview URL
May 24 2026, 08:32 PM

@cloudflare-workers-and-pages
Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
atproto-pds 191ef18 May 24 2026, 08:32 PM

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented May 24, 2026

Open in StackBlitz

npm i https://pkg.pr.new/create-pds@181
npm i https://pkg.pr.new/@getcirrus/oauth-provider@181
npm i https://pkg.pr.new/@getcirrus/pds@181

commit: 191ef18

@ascorbic ascorbic merged commit 6589e1d into main May 24, 2026
7 checks passed
@ascorbic ascorbic deleted the fix/applywrites-transient-record branch May 24, 2026 20:34
@mixie-bot mixie-bot Bot mentioned this pull request May 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant