Grandiras/ATProto.NET

Fork 0

feat(sync): Sync 1.1 inductive firehose verification and a Tap client #171

Merged

Grandiras merged 6 commits from issue-134-sync into main

2026-09-26 16:55:56 +00:00

Grandiras commented

2026-09-26 16:28:16 +00:00

Owner

Closes #134.

The CHANGELOG has claimed "Sync v1.1 support" since 0.4.0, but only the message fields existed. This adds the verification itself, resynchronization, and a client for Tap, Bluesky's reference Sync 1.1 consumer.

Sync 1.1

RepoSyncVerifier (ATProtoNet.Streaming) runs the checklist of the rewritten sync spec on #commit and #sync events:

size limits: at most 200 operations, 2,000,000 bytes of blocks, 1,000,000 bytes per record block;
every block against its CID; the CAR's first root is the event's commit; commit version 3; the event's repo/did and rev match the signed commit;
the revision is not in the future (5 minutes of skew, as the reference relay allows) and is newer than the last one seen (Stale otherwise);
the signature against the account's #atproto key through AtProtoCrypto.VerifySignature (the low-S path, unchanged), refetching the DID document once on failure, as FirehoseVerifier and ServiceAuthVerifier do;
operations well formed (create: cid, no prev; update: both; delete: prev, no cid; no path twice; valid collection/rkey), every created or updated record present as DAG-CBOR;
the operations inverted against the partial MST in the blocks land exactly on prevData;
prevData equals the tree of the last verified commit; a commit without prevData (pre-Sync 1.1 host) chains on since instead; a #sync that moves the tree resets the repository.

Outcomes are Valid, Stale, Invalid (rejected, state untouched) and Desynchronized (state marked, OnDesynchronized invoked once with the reason). A valid event's state is recorded only by ApplyAsync, after processing, so a crash replays rather than skips it. Like the reference relay and Tap, since is not required to match when prevData does (an empty commit moves the revision, not the tree).

MST inversion — Repo/PartialMerkleSearchTree.cs is a port of indigo's atproto/repo/mst node tree (the code the reference relay and Tap invert with), holding CID-only subtrees, including its stub-root trimming. Loading checks key validity, ordering within the parent's range, and layers.

State — IRepoSyncStateStore (per DID: Rev, Data, Status = Synchronized / Desynchronized / Resynchronizing), with InMemoryRepoSyncStateStore and, in ATProtoNet.Server, EfCoreRepoSyncStateStore<TContext> + RepoSyncStateDbContext + ConfigureRepoSyncStateModel + AddAtProtoEfCoreRepoSyncStateStore<TContext>(), in the style of the JTI replay store (Sync/EntityFrameworkCore/, namespace ATProtoNet.Server.EntityFrameworkCore).

Typed consumer (opt-in) — TypedFirehoseConsumerOptions.SyncVerifier (exclusive with Verifier, which keeps working unchanged). Only events that verify and chain are delivered; the repository's state is recorded when the caller asks for the next message (a new async DeliveredAsync hook in the internal EventStreamLoop). With a sync verifier the collection filter can no longer skip commits unparsed: every commit is verified to keep its chain, and the filter only decides delivery. New StreamDropReason.Stale and Desynchronized.

Resync (TypedFirehoseConsumerOptions.Resync) — a desynchronized repository is fetched in the background with getRepo, from the relay first (the spec's thundering-herd guidance; redirects to the PDS are followed, up to 3), then from the account's PDS; a copy older than the event that broke the chain is skipped. The export is verified as a RepoSnapshot (all CIDs, commit, signature with one refresh, complete and canonical tree) and delivered as a RepoResyncEvent; the repository's events that arrived during the fetch were verified and held, and follow it in order. All state changes happen on the consumer's loop (a new NextPendingAsync hook), so there is no locking around the store and a repository's events stay in order. Bounded: MaxConcurrency (4), MaxPendingRepos (1,000), MaxRepoBytes (256 MiB), MaxHeldEventsPerRepo (1,000), MaxHeldBytes (64 MiB), FetchTimeout, RetryDelay after a failure; overflow triggers a refetch. Repositories the store lists as unsynchronized (left from a crash, or marked for backfill) are picked up every ScanInterval. IRepoFetcher (HostRepoFetcher, PdsRepoFetcher) is the pluggable seam; the default fetchers use the SDK's SSRF-hardened handler (AllowPrivateNetworks for development). RepoSnapshot.Verify(car, did, key) is also public for standalone use.

S2 (untrusted MST PrefixLen) was already fixed by #111: MstNodeData bounds p by the previous key's length before any allocation, with regression tests in MerkleSearchTreeTests (Deserialize_FirstEntryPrefixOutOfRange_ThrowsFormatException, …PrefixLongerThanThePreviousKey…). The partial tree decodes through the same MstNodeData.

Tap

TapClient (ATProtoNet.Tap): OpenChannel() → TapChannel.ReadAllAsync() is an IAsyncEnumerable<TapEvent> over /channel with explicit AckAsync ({"type":"ack","id":N}, exactly as @atproto/tap); acks made while disconnected are queued and flushed on reconnect, as the TS channel does. Reconnects per StreamReconnectPolicy over the internal StreamSocket (which gained a send path); a refused upgrade (e.g. webhook mode, 400) throws. Admin: AddReposAsync / RemoveReposAsync ({"dids":[…]}), ResolveDidAsync (404 → null), GetRepoInfoAsync, all with Basic base64("admin:"+password) (FormatAdminAuthHeader("secret") = Basic YWRtaW46c2VjcmV0, the README vector).
Events: TapRecordEvent (implements IRecordEvent, so Tap, firehose and Jetstream indexing code is shared) and TapIdentityEvent, parsed from the shapes indigo's cmd/tap marshals (pinned in tests); unknown types are skipped unacknowledged and reported to OnError, as the TS client does.
Webhook receiver (ATProtoNet.Server.Tap): MapTapWebhook(pattern, handler) / MapTapWebhook<THandler>() with ITapEventHandler. Basic admin auth checked in constant time (SHA-256 digests + FixedTimeEquals, split at the first colon like Go's SetBasicAuth), 401 with WWW-Authenticate; body bounded (MaxBodyBytes, 4 MiB; 413 declared or streamed); 400 for a non-event; 500 when the handler throws so Tap retries; mapping without a password throws unless AllowUnauthenticated.

Docs

docs/firehose.md: new "Verifying the firehose (Sync 1.1)" section (checklist table, consumer usage, resync, state stores, cost, direct use), option table, drop reasons, message types.
New docs/tap.md, linked from docs/index.md, docs/firehose.md and README.
CHANGELOG: four Added bullets; Fixed bullets for the claim (and the 0.4.0 entry reworded to "Sync v1.1 message fields") and for the stricter CBOR below.

Breaking changes

None to public signatures. Behaviour notes:

CID links, and MST node field names, encoded as indefinite-length CBOR strings are now refused (FormatException) in CAR headers, commits and MST nodes; DAG-CBOR forbids them. (Side effect of reading them without an intermediate copy.)
StreamDropReason has two new members; an exhaustive switch over it gains two cases.

Verification

Unit tests: 4750 passed, 0 failed, 5 skipped (rebased on a081657). Full dotnet build: 0 warnings, 0 errors.
MST inversion: the six interop commit-proof vectors (already vendored in MstReferenceVectorTests) — the SDK's own covering proof is loaded as a partial tree and inverted back to rootBeforeCommit; plus 240 random commits (1–8 creates/updates/deletes) over trees of 0 to 1,500 keys.
Captured real frames: tests/ATProtoNet.Tests/Streaming/TestData/firehose-sync-frames.json, six frames from wss://bsky.network (2026-09-26, 28 KB): two consecutive commits of one repository, an update, a delete, a 3-op commit and a #sync, with the signing keys. All verify from an empty store, the pair chains, and tampering with ops or prevData fails inversion.
Edge cases: every checklist rule broken on its own, each asserted with its reason (15 cases); limits at and over 200 ops / 2 MB; future revision at 4 vs 6 minutes; key rotation with one refresh; forged signature; legacy commits; #sync same/different tree.
Consumer: state recorded only on delivery; stale/invalid/desync drop reasons; filter still chains skipped commits; resync delivering snapshot then held events in order; failed fetch; store-marked backfill; stale relay copy falling back to the PDS; held-event overflow causing a refetch. EF store over SQLite; Tap client with a scripted socket (acks, queued acks flushed on reconnect, refused upgrade, bad messages); webhook over TestServer (auth variants, 413 both ways, 400, 500, DI handler).
Cost (no benchmarks project; measured with a scratch harness on the captured frames, Release, one thread of a Ryzen 5 7600X):

Commit	`FirehoseVerifier` (sig + CIDs)	`RepoSyncVerifier` (full checklist)
1 op, 3.3 KB blocks	234 µs, 9.5 KB alloc	259 µs, 30 KB alloc
3 ops, 10 KB blocks	224 µs, 20 KB alloc	304 µs, 74 KB alloc

The ECDSA check dominates. Allocation started at 50–150 KB/commit; it came down by comparing MST node field names as bytes, reading CID links with one copy, pre-sizing the node encoder, validating keys on bytes, and encoding inverted nodes straight into one reused CborWriter (only the CID is kept). The first three also speed up MerkleSearchTree.Deserialize and RecordProof.

Review fixes

Six findings from a review of 6aa8eaf, fixed on top of it; each has a regression test that fails without the fix.

A commit without prevData could hide operations (9e9b9b5). Chaining it on since let a relay strip prevData and drop an operation unnoticed. Once a repository has a chain, such a commit now desynchronizes it. indigo's relay rejects it ("missing prevData"); desynchronizing rejects it too, and also gets the repository fetched again, so the commit's changes are not lost if no later commit comes to reveal the gap. It is still accepted as a repository's first commit.
Held events could be lost across a restart (7dbf41a). CursorTracker gained a persist limit, and the resync keeps it just below the oldest held event not yet delivered or dropped, so a restart replays those events against the recorded snapshot (at-least-once kept). A snapshot whose held events overflowed is recorded as desynchronized.
Fetched snapshots escaped MaxConcurrency (18423d2). A fetch now holds its slot until the consumer's loop takes the completion, so exports in memory stay near (MaxConcurrency + 1) × MaxRepoBytes. The docs say so.
FetchTimeout did not cover the body (bc2a3b9). The download runs under a linked token cancelled after the client's Timeout, and the resync gives each fetcher attempt FetchTimeout, custom fetchers included. Running out is a failed source; the caller's own cancellation stays cancellation.
The webhook answered 400 to unreadable events (72b501e), which Tap retries forever while holding back the repository. They are now acknowledged with a 200 and reported to TapWebhookOptions.OnUnreadableEvent (a warning log by default). 401 and 413 are unchanged.
A declared Content-Length sized the download buffer (bc2a3b9), so a claim past 2 GiB threw and any host could force a MaxRepoBytes allocation. The buffer starts at no more than 1 MiB and grows up to the limit.

After the fixes: 4759 passed, 0 failed, 5 skipped; full build 0 warnings.

Deferred / not done

#account hosting status is not tracked by the verifier, and there is no host-authority check (direct-PDS subscriptions); a relay applies both. The issue's checklist does not include them.
No per-DID tracking scope (Tap's signal collection / dynamic list): every repository seen is chained; users can pre-mark repositories for backfill in the store.
The EF store does two round trips per event; batching or a write-back cache for full-network use is left for later (documented).
getRepo exports are buffered in memory (bounded by MaxRepoBytes); a streaming CAR reader is research item S3.
TapChannel still leaves an unparseable message unacknowledged (reported to OnError), exactly as @atproto/tap's channel does; review fix 5 changed only the webhook. The same stall argument applies, but acknowledging needs an id a malformed message may not have, so it is left to follow the reference client.

🤖 Generated with Claude Code

Closes #134. The CHANGELOG has claimed "Sync v1.1 support" since 0.4.0, but only the message fields existed. This adds the verification itself, resynchronization, and a client for Tap, Bluesky's reference Sync 1.1 consumer. ## Sync 1.1 **`RepoSyncVerifier`** (`ATProtoNet.Streaming`) runs the checklist of the rewritten [sync spec](https://atproto.com/specs/sync) on `#commit` and `#sync` events: - size limits: at most 200 operations, 2,000,000 bytes of blocks, 1,000,000 bytes per record block; - every block against its CID; the CAR's first root is the event's `commit`; commit version 3; the event's `repo`/`did` and `rev` match the signed commit; - the revision is not in the future (5 minutes of skew, as the reference relay allows) and is newer than the last one seen (`Stale` otherwise); - the signature against the account's `#atproto` key through `AtProtoCrypto.VerifySignature` (the low-S path, unchanged), refetching the DID document once on failure, as `FirehoseVerifier` and `ServiceAuthVerifier` do; - operations well formed (create: `cid`, no `prev`; update: both; delete: `prev`, no `cid`; no path twice; valid `collection/rkey`), every created or updated record present as DAG-CBOR; - **the operations inverted against the partial MST in the blocks land exactly on `prevData`**; - `prevData` equals the tree of the last verified commit; a commit without `prevData` (pre-Sync 1.1 host) chains on `since` instead; a `#sync` that moves the tree resets the repository. Outcomes are `Valid`, `Stale`, `Invalid` (rejected, state untouched) and `Desynchronized` (state marked, `OnDesynchronized` invoked once with the reason). A valid event's state is recorded only by `ApplyAsync`, after processing, so a crash replays rather than skips it. Like the reference relay and Tap, `since` is not required to match when `prevData` does (an empty commit moves the revision, not the tree). **MST inversion** — `Repo/PartialMerkleSearchTree.cs` is a port of indigo's `atproto/repo/mst` node tree (the code the reference relay and Tap invert with), holding CID-only subtrees, including its stub-root trimming. Loading checks key validity, ordering within the parent's range, and layers. **State** — `IRepoSyncStateStore` (per DID: `Rev`, `Data`, `Status` = `Synchronized` / `Desynchronized` / `Resynchronizing`), with `InMemoryRepoSyncStateStore` and, in `ATProtoNet.Server`, `EfCoreRepoSyncStateStore<TContext>` + `RepoSyncStateDbContext` + `ConfigureRepoSyncStateModel` + `AddAtProtoEfCoreRepoSyncStateStore<TContext>()`, in the style of the JTI replay store (`Sync/EntityFrameworkCore/`, namespace `ATProtoNet.Server.EntityFrameworkCore`). **Typed consumer (opt-in)** — `TypedFirehoseConsumerOptions.SyncVerifier` (exclusive with `Verifier`, which keeps working unchanged). Only events that verify and chain are delivered; the repository's state is recorded when the caller asks for the next message (a new async `DeliveredAsync` hook in the internal `EventStreamLoop`). With a sync verifier the collection filter can no longer skip commits unparsed: every commit is verified to keep its chain, and the filter only decides delivery. New `StreamDropReason.Stale` and `Desynchronized`. **Resync (`TypedFirehoseConsumerOptions.Resync`)** — a desynchronized repository is fetched in the background with `getRepo`, from the relay first (the spec's thundering-herd guidance; redirects to the PDS are followed, up to 3), then from the account's PDS; a copy older than the event that broke the chain is skipped. The export is verified as a `RepoSnapshot` (all CIDs, commit, signature with one refresh, complete and canonical tree) and delivered as a `RepoResyncEvent`; the repository's events that arrived during the fetch were verified and held, and follow it in order. All state changes happen on the consumer's loop (a new `NextPendingAsync` hook), so there is no locking around the store and a repository's events stay in order. Bounded: `MaxConcurrency` (4), `MaxPendingRepos` (1,000), `MaxRepoBytes` (256 MiB), `MaxHeldEventsPerRepo` (1,000), `MaxHeldBytes` (64 MiB), `FetchTimeout`, `RetryDelay` after a failure; overflow triggers a refetch. Repositories the store lists as unsynchronized (left from a crash, or marked for backfill) are picked up every `ScanInterval`. `IRepoFetcher` (`HostRepoFetcher`, `PdsRepoFetcher`) is the pluggable seam; the default fetchers use the SDK's SSRF-hardened handler (`AllowPrivateNetworks` for development). `RepoSnapshot.Verify(car, did, key)` is also public for standalone use. **S2 (untrusted MST `PrefixLen`)** was already fixed by #111: `MstNodeData` bounds `p` by the previous key's length before any allocation, with regression tests in `MerkleSearchTreeTests` (`Deserialize_FirstEntryPrefixOutOfRange_ThrowsFormatException`, `…PrefixLongerThanThePreviousKey…`). The partial tree decodes through the same `MstNodeData`. ## Tap - **`TapClient`** (`ATProtoNet.Tap`): `OpenChannel()` → `TapChannel.ReadAllAsync()` is an `IAsyncEnumerable<TapEvent>` over `/channel` with explicit `AckAsync` (`{"type":"ack","id":N}`, exactly as `@atproto/tap`); acks made while disconnected are queued and flushed on reconnect, as the TS channel does. Reconnects per `StreamReconnectPolicy` over the internal `StreamSocket` (which gained a send path); a refused upgrade (e.g. webhook mode, 400) throws. Admin: `AddReposAsync` / `RemoveReposAsync` (`{"dids":[…]}`), `ResolveDidAsync` (404 → null), `GetRepoInfoAsync`, all with `Basic base64("admin:"+password)` (`FormatAdminAuthHeader("secret")` = `Basic YWRtaW46c2VjcmV0`, the README vector). - **Events**: `TapRecordEvent` (implements `IRecordEvent`, so Tap, firehose and Jetstream indexing code is shared) and `TapIdentityEvent`, parsed from the shapes indigo's `cmd/tap` marshals (pinned in tests); unknown types are skipped unacknowledged and reported to `OnError`, as the TS client does. - **Webhook receiver** (`ATProtoNet.Server.Tap`): `MapTapWebhook(pattern, handler)` / `MapTapWebhook<THandler>()` with `ITapEventHandler`. Basic admin auth checked in constant time (SHA-256 digests + `FixedTimeEquals`, split at the first colon like Go's `SetBasicAuth`), 401 with `WWW-Authenticate`; body bounded (`MaxBodyBytes`, 4 MiB; 413 declared or streamed); 400 for a non-event; 500 when the handler throws so Tap retries; mapping without a password throws unless `AllowUnauthenticated`. ## Docs - `docs/firehose.md`: new "Verifying the firehose (Sync 1.1)" section (checklist table, consumer usage, resync, state stores, cost, direct use), option table, drop reasons, message types. - New `docs/tap.md`, linked from `docs/index.md`, `docs/firehose.md` and README. - CHANGELOG: four `Added` bullets; `Fixed` bullets for the claim (and the 0.4.0 entry reworded to "Sync v1.1 message fields") and for the stricter CBOR below. ## Breaking changes None to public signatures. Behaviour notes: - CID links, and MST node field names, encoded as **indefinite-length** CBOR strings are now refused (`FormatException`) in CAR headers, commits and MST nodes; DAG-CBOR forbids them. (Side effect of reading them without an intermediate copy.) - `StreamDropReason` has two new members; an exhaustive `switch` over it gains two cases. ## Verification - Unit tests: **4750 passed, 0 failed, 5 skipped** (rebased on `a081657`). Full `dotnet build`: **0 warnings, 0 errors**. - **MST inversion**: the six interop commit-proof vectors (already vendored in `MstReferenceVectorTests`) — the SDK's own covering proof is loaded as a partial tree and inverted back to `rootBeforeCommit`; plus 240 random commits (1–8 creates/updates/deletes) over trees of 0 to 1,500 keys. - **Captured real frames**: `tests/ATProtoNet.Tests/Streaming/TestData/firehose-sync-frames.json`, six frames from `wss://bsky.network` (2026-09-26, 28 KB): two consecutive commits of one repository, an update, a delete, a 3-op commit and a `#sync`, with the signing keys. All verify from an empty store, the pair chains, and tampering with ops or `prevData` fails inversion. - **Edge cases**: every checklist rule broken on its own, each asserted with its reason (15 cases); limits at and over 200 ops / 2 MB; future revision at 4 vs 6 minutes; key rotation with one refresh; forged signature; legacy commits; `#sync` same/different tree. - **Consumer**: state recorded only on delivery; stale/invalid/desync drop reasons; filter still chains skipped commits; resync delivering snapshot then held events in order; failed fetch; store-marked backfill; stale relay copy falling back to the PDS; held-event overflow causing a refetch. EF store over SQLite; Tap client with a scripted socket (acks, queued acks flushed on reconnect, refused upgrade, bad messages); webhook over `TestServer` (auth variants, 413 both ways, 400, 500, DI handler). - **Cost** (no benchmarks project; measured with a scratch harness on the captured frames, Release, one thread of a Ryzen 5 7600X): | Commit | `FirehoseVerifier` (sig + CIDs) | `RepoSyncVerifier` (full checklist) | |---|---|---| | 1 op, 3.3 KB blocks | 234 µs, 9.5 KB alloc | 259 µs, 30 KB alloc | | 3 ops, 10 KB blocks | 224 µs, 20 KB alloc | 304 µs, 74 KB alloc | The ECDSA check dominates. Allocation started at 50–150 KB/commit; it came down by comparing MST node field names as bytes, reading CID links with one copy, pre-sizing the node encoder, validating keys on bytes, and encoding inverted nodes straight into one reused `CborWriter` (only the CID is kept). The first three also speed up `MerkleSearchTree.Deserialize` and `RecordProof`. ## Review fixes Six findings from a review of `6aa8eaf`, fixed on top of it; each has a regression test that fails without the fix. 1. **A commit without `prevData` could hide operations** (`9e9b9b5`). Chaining it on `since` let a relay strip `prevData` and drop an operation unnoticed. Once a repository has a chain, such a commit now **desynchronizes** it. indigo's relay rejects it ("missing prevData"); desynchronizing rejects it too, and also gets the repository fetched again, so the commit's changes are not lost if no later commit comes to reveal the gap. It is still accepted as a repository's first commit. 2. **Held events could be lost across a restart** (`7dbf41a`). `CursorTracker` gained a persist limit, and the resync keeps it just below the oldest held event not yet delivered or dropped, so a restart replays those events against the recorded snapshot (at-least-once kept). A snapshot whose held events overflowed is recorded as desynchronized. 3. **Fetched snapshots escaped `MaxConcurrency`** (`18423d2`). A fetch now holds its slot until the consumer's loop takes the completion, so exports in memory stay near `(MaxConcurrency + 1) × MaxRepoBytes`. The docs say so. 4. **`FetchTimeout` did not cover the body** (`bc2a3b9`). The download runs under a linked token cancelled after the client's `Timeout`, and the resync gives each fetcher attempt `FetchTimeout`, custom fetchers included. Running out is a failed source; the caller's own cancellation stays cancellation. 5. **The webhook answered 400 to unreadable events** (`72b501e`), which Tap retries forever while holding back the repository. They are now acknowledged with a 200 and reported to `TapWebhookOptions.OnUnreadableEvent` (a warning log by default). 401 and 413 are unchanged. 6. **A declared `Content-Length` sized the download buffer** (`bc2a3b9`), so a claim past 2 GiB threw and any host could force a `MaxRepoBytes` allocation. The buffer starts at no more than 1 MiB and grows up to the limit. After the fixes: **4759 passed, 0 failed, 5 skipped**; full build 0 warnings. ## Deferred / not done - `#account` hosting status is not tracked by the verifier, and there is no host-authority check (direct-PDS subscriptions); a relay applies both. The issue's checklist does not include them. - No per-DID tracking scope (Tap's signal collection / dynamic list): every repository seen is chained; users can pre-mark repositories for backfill in the store. - The EF store does two round trips per event; batching or a write-back cache for full-network use is left for later (documented). - `getRepo` exports are buffered in memory (bounded by `MaxRepoBytes`); a streaming CAR reader is research item S3. - `TapChannel` still leaves an unparseable message unacknowledged (reported to `OnError`), exactly as `@atproto/tap`'s channel does; review fix 5 changed only the webhook. The same stall argument applies, but acknowledging needs an `id` a malformed message may not have, so it is left to follow the reference client. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

Grandiras added 1 commit

2026-09-26 16:28:16 +00:00

feat(sync): Sync 1.1 inductive firehose verification and a Tap client (closes #134 )

CI / pds-integration (pull_request) Successful in 27s

Details

CI / build-and-test (pull_request) Successful in 55s

Details

6aa8eafadc

The CHANGELOG has claimed "Sync v1.1 support" since 0.4.0, but only the
message fields existed: nothing checked that a commit's operations are the
ones its blocks prove, or that a consumer received every commit.

RepoSyncVerifier runs the sync spec's validation checklist on #commit and
partial MST in its blocks (a port of indigo's atproto/repo/mst, which the
reference relay and Tap use) and comparing the result with prevData, then
chaining each repository on the revision and tree kept in an
IRepoSyncStateStore (in memory, or EF Core in ATProtoNet.Server). The typed
firehose consumer runs it as an opt-in mode that records state on delivery,
and can resynchronize broken repositories: getRepo from the relay first,
then the PDS, bounded, delivered as a RepoResyncEvent followed by the events
held meanwhile.

TapClient reads Tap's acknowledged /channel stream and calls its admin
endpoints, and MapTapWebhook receives its webhook mode, with the wire
protocol and Basic admin auth of @atproto/tap.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

opencode-bot commented

2026-09-26 16:28:17 +00:00

Collaborator

🤖 Reviewing this PR — the review will link the full session transcript.

📜 Live transcript — updating every ~5 minutes while the review runs.

🤖 Reviewing this PR — the review will link the full session transcript.  📜 [Live transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-171/run-2026-09-26T16-28-17-183Z.md) — updating every ~5 minutes while the review runs.

opencode-bot commented

2026-09-26 16:28:19 +00:00

Collaborator

⏳ Held on the OpenCode Go window — the 5-hour window is at 0% (the bot holds at 95%).
This continues automatically after the window resets (2026-09-26T21:28:17.643Z) — nothing to re-trigger.

⏳ **Held on the OpenCode Go window** — the 5-hour window is at 0% (the bot holds at 95%). This continues automatically after the window resets (2026-09-26T21:28:17.643Z) — nothing to re-trigger.

Grandiras commented

2026-09-26 16:34:10 +00:00

Author

Owner

@claude /review — large and verification-critical. The diff will be truncated, so please read these in full from the branch: src/ATProtoNet/Repo/PartialMerkleSearchTree.cs (the port of indigo's partial-MST operation inversion; compare with indigo atproto/repo), src/ATProtoNet/Streaming/RepoSyncVerifier.cs (the Sync 1.1 checklist: signature with refetch-once, event fields vs the signed commit, the prevData comparison, rev ordering and the future check, since/prevData chain, commit v3, size limits, #sync reset), RepoResync.cs + RepoFetchers.cs (relay-first getRepo, bounds, the ordering of held events), RepoSyncState.cs, the TypedFirehoseConsumer.cs change (opt-in SyncVerifier; state recorded only after the caller takes the next message), src/ATProtoNet/Tap/TapClient.cs + TapModels.cs (ack protocol and admin API vs @atproto/tap), src/ATProtoNet.Server/Tap/TapWebhookExtensions.cs (auth compare, body bound) and the EF store. Focus on verification bypasses (a commit that passes with ops not matching the signed tree, rev regressions, a #sync that resets state without a valid signature), memory or DoS bounds on untrusted CAR/MST input, and webhook auth.

@claude /review — large and verification-critical. The diff will be truncated, so please read these in full from the branch: `src/ATProtoNet/Repo/PartialMerkleSearchTree.cs` (the port of indigo's partial-MST operation inversion; compare with indigo `atproto/repo`), `src/ATProtoNet/Streaming/RepoSyncVerifier.cs` (the Sync 1.1 checklist: signature with refetch-once, event fields vs the signed commit, the prevData comparison, rev ordering and the future check, since/prevData chain, commit v3, size limits, #sync reset), `RepoResync.cs` + `RepoFetchers.cs` (relay-first getRepo, bounds, the ordering of held events), `RepoSyncState.cs`, the `TypedFirehoseConsumer.cs` change (opt-in SyncVerifier; state recorded only after the caller takes the next message), `src/ATProtoNet/Tap/TapClient.cs` + `TapModels.cs` (ack protocol and admin API vs `@atproto/tap`), `src/ATProtoNet.Server/Tap/TapWebhookExtensions.cs` (auth compare, body bound) and the EF store. Focus on verification bypasses (a commit that passes with ops not matching the signed tree, rev regressions, a #sync that resets state without a valid signature), memory or DoS bounds on untrusted CAR/MST input, and webhook auth.

claude-bot commented

2026-09-26 16:34:12 +00:00

Collaborator

🤖 Reviewing this PR — follow along in the action run; the reply will link the full session transcript.

📜 Live transcript — updating every ~5 minutes while the task runs.

🤖 Reviewing this PR — follow along in [the action run](https://git.grandiras.net/Grandiras/claude-bot/actions/runs/618); the reply will link the full session transcript.  📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-171/run-618.md) — updating every ~5 minutes while the task runs.

claude-reviewer approved these changes

2026-09-26 16:38:19 +00:00

Dismissed

claude-reviewer left a comment

✅ Verdict: approve — no blocking issues found.

This is a large, well-scoped PR that finally implements the Sync 1.1 verification the CHANGELOG had been claiming since 0.4.0, plus a Tap client/webhook receiver. I focused deep review on the highest-risk, newest algorithmic and security-sensitive code rather than skimming everything:

RepoSyncVerifier (src/ATProtoNet/Streaming/RepoSyncVerifier.cs): traced VerifyCommitAsync/VerifySyncAsync end to end — staleness check, signature verify-then-refresh-once, operation shape validation (create/update/delete pre/post-conditions), and the chain check against prevData/since. The RepoOpAction shape-check switch correctly guards against null-ref on op.Prev! later (inductive commits are required to carry Prev for update/delete before the code that dereferences it runs). The two-stage validation — self-consistency (inverted tree equals the commit's own declared prevData) followed by chain continuity (declared prevData equals last stored state) — matches the PR's description and the spec.
PartialMerkleSearchTree: read through load-time structural validation (key ordering, layer consistency, bounds) and the insert/remove/split/merge logic. This is intricate, MST-are-content-addressed-so-order-shouldn't-matter code; I didn't attempt an independent formal proof of the insert/remove algebra, but didn't spot an obvious defect either, and the PR states it was checked against six interop reference vectors plus 240 randomized fuzz commits, which is credible coverage for this kind of code.
CommitBlock / Cid.AsSpan: confirmed Cid's binary layout (_bytes[1] is the multicodec byte) matches the assumption in RepoSyncVerifier.CheckRecord's DAG-CBOR codec check.
TapClient/TapChannel: the send/ack concurrency design (_sendLock serializing AckAsync/AttachAsync, _socket swapped only under that lock or via CompareExchange in the single reading loop) looks race-free; queued acks flush correctly on reconnect.
TapWebhookExtensions: the admin-auth check hashes both sides with SHA-256 before FixedTimeEquals (avoiding a length-based timing signal), the body is bounded both via IHttpMaxRequestBodySizeFeature and a manual streaming read, and errors map to the documented status codes.
TypedFirehoseConsumer integration: state is only persisted in DeliveredAsync (called after yield return resumes), so a resync snapshot's state is durably applied before NextPendingAsync re-verifies held events against it — the sequencing that makes an at-least-once resync correct.

Given the size of the diff (~7,100 lines) and the truncation of the diff itself partway through RepoSyncVerifier.cs, I pulled the full files from the PR branch directly via git rather than relying solely on the (truncated) diff text. I did not exhaustively re-derive every MST split/merge case or review every line of RepoResync.cs's bookkeeping (backoff maps, overflow handling), but skimmed them for obvious bounds/lifecycle issues and found none. No blocking issues found.

⏱ 3m 57s · 20 turns · tokens: 126.5k in / 16.9k out (+2283.3k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. This is a large, well-scoped PR that finally implements the Sync 1.1 verification the CHANGELOG had been claiming since 0.4.0, plus a Tap client/webhook receiver. I focused deep review on the highest-risk, newest algorithmic and security-sensitive code rather than skimming everything: - **`RepoSyncVerifier`** (`src/ATProtoNet/Streaming/RepoSyncVerifier.cs`): traced `VerifyCommitAsync`/`VerifySyncAsync` end to end — staleness check, signature verify-then-refresh-once, operation shape validation (create/update/delete pre/post-conditions), and the chain check against `prevData`/`since`. The `RepoOpAction` shape-check switch correctly guards against null-ref on `op.Prev!` later (inductive commits are required to carry `Prev` for update/delete before the code that dereferences it runs). The two-stage validation — self-consistency (inverted tree equals the commit's own declared `prevData`) followed by chain continuity (declared `prevData` equals last stored state) — matches the PR's description and the spec. - **`PartialMerkleSearchTree`**: read through load-time structural validation (key ordering, layer consistency, bounds) and the insert/remove/split/merge logic. This is intricate, MST-are-content-addressed-so-order-shouldn't-matter code; I didn't attempt an independent formal proof of the insert/remove algebra, but didn't spot an obvious defect either, and the PR states it was checked against six interop reference vectors plus 240 randomized fuzz commits, which is credible coverage for this kind of code. - **`CommitBlock` / `Cid.AsSpan`**: confirmed `Cid`'s binary layout (`_bytes[1]` is the multicodec byte) matches the assumption in `RepoSyncVerifier.CheckRecord`'s DAG-CBOR codec check. - **`TapClient`/`TapChannel`**: the send/ack concurrency design (`_sendLock` serializing `AckAsync`/`AttachAsync`, `_socket` swapped only under that lock or via `CompareExchange` in the single reading loop) looks race-free; queued acks flush correctly on reconnect. - **`TapWebhookExtensions`**: the admin-auth check hashes both sides with SHA-256 before `FixedTimeEquals` (avoiding a length-based timing signal), the body is bounded both via `IHttpMaxRequestBodySizeFeature` and a manual streaming read, and errors map to the documented status codes. - **`TypedFirehoseConsumer` integration**: state is only persisted in `DeliveredAsync` (called after `yield return` resumes), so a resync snapshot's state is durably applied before `NextPendingAsync` re-verifies held events against it — the sequencing that makes an at-least-once resync correct. Given the size of the diff (~7,100 lines) and the truncation of the diff itself partway through `RepoSyncVerifier.cs`, I pulled the full files from the PR branch directly via git rather than relying solely on the (truncated) diff text. I did not exhaustively re-derive every MST split/merge case or review every line of `RepoResync.cs`'s bookkeeping (backoff maps, overflow handling), but skimmed them for obvious bounds/lifecycle issues and found none. No blocking issues found. --- ⏱ 3m 57s · 20 turns · tokens: 126.5k in / 16.9k out (+2283.3k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-171/run-618.md)

Grandiras added 5 commits

2026-09-26 16:52:20 +00:00

fix(sync): a commit without prevData desynchronizes a chained repository after review (#134 ) 9e9b9b552c

Chaining a prevData-less commit on since alone let a relay strip prevData
and drop an operation unnoticed: without prevData nothing is inverted, so
nothing proves the operation list complete. Once a repository has a chain,
such a commit now marks it desynchronized, so it is fetched again rather
than trusted; indigo's relay rejects it as missing prevData. It is still
accepted, uninverted, as a repository's first commit.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

fix(sync): keep the saved cursor below undelivered held events after review (#134 ) 7dbf41af09

An event held during a resync advanced the cursor past itself when it was
held, and the snapshot was recorded as synchronized before the held events
followed it, so a process stopping in between lost them silently. The
cursor tracker now takes a persist limit, and the resync keeps it just
below the oldest held event that has not been delivered or dropped, so a
restart replays those events against the recorded snapshot. A snapshot
whose held events overflowed is recorded as desynchronized, since it lacks
events the refetch still has to cover.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

fix(sync): count fetched snapshots against the resync concurrency limit after review (#134 ) 18423d2816

A fetch released its slot as soon as it queued its snapshot, so finished
snapshots, each a whole repository in memory, could pile up while the
consumer was busy: the real bound was MaxPendingRepos x MaxRepoBytes, not
what the docs said. The slot is now held until the consumer's loop takes
the completion, so exports in memory stay near (MaxConcurrency + 1) x
MaxRepoBytes, and the docs say so.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

fix(sync): bound whole repository downloads and their buffers after review (#134 ) bc2a3b9306

A download read with ResponseHeadersRead, so HttpClient.Timeout covered
only the headers and a PDS trickling the body held a fetch slot for good.
The download now runs under a linked token cancelled after the client's
timeout, and the resync gives every fetcher attempt FetchTimeout the same
way; running out is a failed source, while the caller's own cancellation
stays cancellation. A declared Content-Length also no longer sizes the
buffer: a claim past 2 GiB threw, and any host could make it reserve
MaxRepoBytes with the header alone. The buffer now starts at no more than
1 MiB and grows as bytes arrive, up to the limit.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

fix(tap): acknowledge unreadable webhook events after review (#134 )

CI / pds-integration (pull_request) Successful in 29s

Details

CI / build-and-test (pull_request) Successful in 57s

Details

Sync Closures to GitHub / sync-closure (pull_request) Successful in 7s

Details

72b501e833

The receiver answered 400 to a body it could not parse, but Tap's webhook
client retries every non-2xx indefinitely and delivers each repository's
events in order, so one event of a type this SDK does not model stalled
its repository for good. Such a delivery is now acknowledged with a 200
and reported to TapWebhookOptions.OnUnreadableEvent (a warning log by
default); bad credentials still get 401 and an oversized body 413.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Grandiras commented

2026-09-26 16:52:28 +00:00

Author

Owner

@claude /review — re-review please. Five new commits (9e9b9b5..72b501e) on top of the approved 6aa8eaf fix six findings from an independent adversarial review (see "Review fixes" in the description):

With prior state, a commit without prevData desynchronizes the repo.
The saved cursor stays below held events that haven't been delivered, so a crash after a resync snapshot replays them.
A fetched snapshot holds its concurrency slot until the consumer takes it.
The whole repo download is bounded by the timeout, and the initial buffer is at most 1 MiB.
The Tap webhook acknowledges unreadable events with 200 and reports them to OnUnreadableEvent.

Please review these commits in full (git diff 6aa8eaf..72b501e), especially the CursorTracker persist-limit interaction with RepoResync and the TypedFirehoseConsumer delivery path.

@claude /review — re-review please. Five new commits (9e9b9b5..72b501e) on top of the approved 6aa8eaf fix six findings from an independent adversarial review (see "Review fixes" in the description): 1. With prior state, a commit without `prevData` desynchronizes the repo. 2. The saved cursor stays below held events that haven't been delivered, so a crash after a resync snapshot replays them. 3. A fetched snapshot holds its concurrency slot until the consumer takes it. 4. The whole repo download is bounded by the timeout, and the initial buffer is at most 1 MiB. 5. The Tap webhook acknowledges unreadable events with 200 and reports them to `OnUnreadableEvent`. Please review these commits in full (`git diff 6aa8eaf..72b501e`), especially the `CursorTracker` persist-limit interaction with `RepoResync` and the `TypedFirehoseConsumer` delivery path.

claude-bot commented

2026-09-26 16:52:33 +00:00

Collaborator

🤖 Reviewing this PR — follow along in the action run; the reply will link the full session transcript.

📜 Live transcript — updating every ~5 minutes while the task runs.

🤖 Reviewing this PR — follow along in [the action run](https://git.grandiras.net/Grandiras/claude-bot/actions/runs/620); the reply will link the full session transcript.  📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-171/run-620.md) — updating every ~5 minutes while the task runs.

claude-reviewer approved these changes

2026-09-26 16:54:51 +00:00

claude-reviewer left a comment

✅ Verdict: approve — no blocking issues found.

This is a re-review of the five fix commits that landed on top of the original 6aa8eaf (which my prior pass had already approved as no-blocking-issues): 9e9b9b5, 7dbf41a, 18423d2, bc2a3b9, 72b501e. Since the diff supplied in the PR body was truncated partway through, I read each fix commit directly from the local origin/issue-134-sync ref to verify the actual changes rather than the truncated text.

All six previously-reported findings are fixed correctly:

prevData-less commit after a chain now desynchronizes (9e9b9b5) — the old accept-on-matching-since path is removed; a commit without prevData is only trusted as a repository's very first event. The test suite correctly flips the old "chains on since" assertion to "desynchronizes", and adds a first-commit-accepted case plus a stripped-prevData-with-dropped-op regression that reproduces the exact attack described.
Cursor persist limit for held events (7dbf41a) — CursorTracker.SetPersistLimit/Persistable() correctly caps the saved cursor; RepoResyncCoordinator tracks per-sequence hold counts (_undelivered, a SortedDictionary so the minimum is cheap to find) and publishes min - 1 on every hold/release. I traced all three release call sites — normal delivery (DeliveredAsync), a held event failing re-verification, and abandonment after a failed fetch — plus the overflow-clear path, which previously would have left dropped events counted forever and blocked cursor persistence permanently; it's now correctly released too. The new ConsumeAsync_StopBetweenSnapshotAndHeldEvents_ReplaysTheHeldEvents test exercises the exact crash-recovery scenario.
Fetched-but-undelivered snapshots now count against MaxConcurrency (18423d2) — _concurrency.Release() moved from the background worker (right after the fetch) into CompleteAsync, which only runs on the consumer's own loop when it dequeues the completion. This matches the corrected memory-bound doc comment (MaxConcurrency + 1) × MaxRepoBytes.
FetchTimeout now covers the whole download body, including for custom IRepoFetchers (bc2a3b9) — a linked, CancelAfter-based token wraps every fetcher attempt in FetchSnapshotAsync, and RepoDownload.GetRepoAsync separately budgets the streaming read against the client's own Timeout (since ResponseHeadersRead doesn't cover the body). A declared Content-Length no longer pre-sizes the buffer past 1 MiB, closing the forced-allocation angle.
Tap webhook now acknowledges unreadable events with 200 instead of 400 (72b501e), reported to the new OnUnreadableEvent hook (defaulting to a warning log); 401/413 paths are unchanged.

I did not have the ability to fetch/build a worktree in this session (git worktree/fetch operations require approval I couldn't obtain), so I relied on reading the full commits from the already-present origin/issue-134-sync remote-tracking ref rather than running the test suite myself; the PR's own reported numbers (4759 passed, 0 failed, 0 warnings) are consistent with what I'd expect given the fixes and their accompanying regression tests. No new issues found in this pass.

⏱ 2m 9s · 26 turns · tokens: 105.7k in / 8.6k out (+2790.3k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. This is a re-review of the five fix commits that landed on top of the original 6aa8eaf (which my prior pass had already approved as no-blocking-issues): 9e9b9b5, 7dbf41a, 18423d2, bc2a3b9, 72b501e. Since the diff supplied in the PR body was truncated partway through, I read each fix commit directly from the local origin/issue-134-sync ref to verify the actual changes rather than the truncated text. All six previously-reported findings are fixed correctly: 1. **prevData-less commit after a chain now desynchronizes** (9e9b9b5) — the old accept-on-matching-since path is removed; a commit without prevData is only trusted as a repository's very first event. The test suite correctly flips the old "chains on since" assertion to "desynchronizes", and adds a first-commit-accepted case plus a stripped-prevData-with-dropped-op regression that reproduces the exact attack described. 2. **Cursor persist limit for held events** (7dbf41a) — `CursorTracker.SetPersistLimit`/`Persistable()` correctly caps the saved cursor; `RepoResyncCoordinator` tracks per-sequence hold counts (`_undelivered`, a `SortedDictionary` so the minimum is cheap to find) and publishes `min - 1` on every hold/release. I traced all three release call sites — normal delivery (`DeliveredAsync`), a held event failing re-verification, and abandonment after a failed fetch — plus the overflow-clear path, which previously would have left dropped events counted forever and blocked cursor persistence permanently; it's now correctly released too. The new `ConsumeAsync_StopBetweenSnapshotAndHeldEvents_ReplaysTheHeldEvents` test exercises the exact crash-recovery scenario. 3. **Fetched-but-undelivered snapshots now count against `MaxConcurrency`** (18423d2) — `_concurrency.Release()` moved from the background worker (right after the fetch) into `CompleteAsync`, which only runs on the consumer's own loop when it dequeues the completion. This matches the corrected memory-bound doc comment `(MaxConcurrency + 1) × MaxRepoBytes`. 4. **`FetchTimeout` now covers the whole download body**, including for custom `IRepoFetcher`s (bc2a3b9) — a linked, `CancelAfter`-based token wraps every fetcher attempt in `FetchSnapshotAsync`, and `RepoDownload.GetRepoAsync` separately budgets the streaming read against the client's own `Timeout` (since `ResponseHeadersRead` doesn't cover the body). A declared `Content-Length` no longer pre-sizes the buffer past 1 MiB, closing the forced-allocation angle. 5. **Tap webhook now acknowledges unreadable events with 200** instead of 400 (72b501e), reported to the new `OnUnreadableEvent` hook (defaulting to a warning log); 401/413 paths are unchanged. I did not have the ability to fetch/build a worktree in this session (git worktree/fetch operations require approval I couldn't obtain), so I relied on reading the full commits from the already-present `origin/issue-134-sync` remote-tracking ref rather than running the test suite myself; the PR's own reported numbers (4759 passed, 0 failed, 0 warnings) are consistent with what I'd expect given the fixes and their accompanying regression tests. No new issues found in this pass. --- ⏱ 2m 9s · 26 turns · tokens: 105.7k in / 8.6k out (+2790.3k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-171/run-620.md)

Grandiras merged commit a5f2489c45 into main

2026-09-26 16:55:56 +00:00

Grandiras deleted branch issue-134-sync

2026-09-26 16:55:56 +00:00

Grandiras referenced this pull request from a commit

2026-09-26 16:55:58 +00:00

feat(sync): Sync 1.1 inductive firehose verification and a Tap client (#171)

Grandiras referenced this pull request

2026-09-26 20:49:24 +00:00

fix(spaces)!: protocol hardening, performance and a smaller server surface #172

Grandiras referenced this pull request

2026-09-26 20:49:25 +00:00