Claude: changes for issue #40 #65
No reviewers
Labels
No labels
breaking-change
bug
documentation
duplicate
enhancement
good first issue
help wanted
performance
question
wontfix
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Grandiras/ATProto.NET!65
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "claude/issue-40"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #40.
🤖 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; 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; the reply will link the full session transcript.
📜 Live transcript — updating every ~5 minutes while the task runs.
Substantial, well-documented PDS federation feature (real DIDs, DAG-CBOR/MST repos, signed commits, sync surface, firehose). Most of the crypto/CBOR/CID plumbing looks careful and the docs/CHANGELOG are thorough. However, there's one change that looks like it silently breaks the backward-compatibility guarantee the PR itself promises, plus a secondary correctness gap in
listRepos.Main issue — federation is wired unconditionally, breaking pre-existing
IRepoStoreimplementations for ordinary CRUD, not just the new sync surface.AddFederationServices(called from everyAddAtProtoPdsoverload, including the plain non-generic one) unconditionally doesservices.TryAddSingleton<PdsRepoManager>().PdsServicenow has two public constructors — a 4-arg one and a 6-arg one takingPdsRepoManager?/PdsIdentityService?. SincePdsRepoManagerandPdsIdentityServiceare now always resolvable from the container, ASP.NET Core's default DI constructor selection (most-parameters-that-resolve) will always pick the 6-arg constructor, so_repoManageris effectively nevernullfor any host built withAddAtProtoPds(). That means everycreateRecord/putRecord/deleteRecordcall now unconditionally goes throughPdsRepoManager.CommitAsync→BuildSnapshotAsync→IRepoStore.ListAllRecordsAsync, which throwsNotSupportedExceptionby default. This directly contradicts the stated design goal inIRepoStore.csand the CHANGELOG ("a store written before federation support still compiles and still serves repo CRUD; only the federation surface reports the gap") — in practice, any consumer with a pre-existing customIRepoStorethat hasn't implemented the two new members will now fail on every write, not just on the new sync endpoints. Worth confirming whether this is intentional (federation always-on) and, if not, gatingPdsRepoManager/PdsIdentityServiceregistration or the commit call behind something that preserves the documented opt-out.Secondary —
com.atproto.sync.listReposhardcodesactive = true.RepoCommitStatecarries no active/status flag, so the endpoint always reports every listed repo as active, even for deactivated accounts (whichGetRepoStatuscorrectly reports via the account/session lookup). A relay relying onlistReposwon't see deactivated accounts reflected accurately.Minor/non-blocking observations:
PdsRepoManager.PublishCommitinlines the entire MST block set into every#commitfirehose frame (not just the touched path), so any repo of non-trivial size will quickly exceedMaxFirehoseFrameBytesand fall back totooBigon nearly every write — acknowledged as a scale tradeoff in the docs, but worth flagging since it undermines incremental firehose consumption for real repos. The per-DIDSemaphoreSlimdictionary inPdsRepoManageralso grows unboundedly over the process lifetime (never removed), though this is a minor memory concern only.src/ATProtoNet.Pds/PdsHostingExtensions.cs:103—AddFederationServicesis called unconditionally from everyAddAtProtoPdsoverload and always registersPdsRepoManager/PdsIdentityServiceviaTryAddSingleton. BecausePdsService(seePdsService.cs) has both a 4-arg and a 6-arg constructor, and bothPdsRepoManagerandPdsIdentityServiceare now always resolvable, the framework's default DI constructor selection will always bind the 6-arg ("federating") constructor — the 4-arg constructor becomes effectively dead code. As a result, every repo write now unconditionally callsPdsRepoManager.CommitAsync, which rebuilds the MST viaIRepoStore.ListAllRecordsAsync. Any pre-existingIRepoStoreimplementation that hasn't overridden that default-interface member (which throwsNotSupportedException) will now fail on ordinarycreateRecord/putRecord/deleteRecordcalls, contradicting the explicit compatibility promise inIRepoStore.csand the CHANGELOG that only the new federation surface would report the gap.src/ATProtoNet.Pds/PdsService.cs:43— This constructor documentsrepoManager/identityas optionallynull("the PDS keeps serving repo CRUD but nothing on the network can follow it"), but given howAddFederationServicesregisters these services unconditionally (see PdsHostingExtensions.cs), DI will always select this constructor over the 4-arg one, so in practicerepoManageris never null for any host wired throughAddAtProtoPds. If federation is meant to be effectively mandatory now, the 4-arg constructor and its null-repoManager code paths (e.g.CommitAsync's_repoManager is nullcheck) are misleading dead code; if it's meant to be optional, the DI wiring needs to makePdsRepoManagerconditional or otherwise avoid always satisfying this constructor's parameters.src/ATProtoNet.Pds/PdsHostingExtensions.cs:449—com.atproto.sync.listReposhardcodesactive = truefor every repo returned, regardless of the account's actual hosting status.GetRepoStatuscorrectly derivesactivefrom the session/account lookup, butRepoCommitState(and thusListReposAsync) has no such field, so a deactivated account still reports as active inlistRepos. Relays enumerating repos via this endpoint would get an inconsistent picture relative togetRepoStatus.⏱ 3m 37s · 1 turns · tokens: 69.0k in / 21.9k out (+29.1k cache-read)
📜 Full session transcript
@claude-bot Please fix the conflicts and pick up the comments.
🤖 On it — follow along in the action run; the reply will link the full session transcript.
📜 Live transcript — updating every ~5 minutes while the task runs.
Pushed to
claude/issue-40— conflicts resolved and all three review points addressed.Conflicts
mainhad moved ahead by the issue #37 (session signing key) and #45 (lexgen) work. Three files conflicted —CHANGELOG.md,docs/pds.md,PdsHostingExtensions.cs— all additive on both sides, so both sides were kept. Committed as a separate merge commit (7afb7f7) so the review fixes are reviewable on their own.Review fixes (
4044191)Federation broke pre-existing
IRepoStoreimplementations — confirmed, and fixed. Your read was right: every write went throughCommitAsync→BuildSnapshotAsync→ListAllRecordsAsync, so a legacy store failed on everycreateRecord/putRecord/deleteRecord, not just on the sync surface.I kept federation always-on (that's the intended design) and instead made it honour the promise:
CommitAsyncnow returnsnullrather than throwing when the store can't enumerate a repository. The write succeeds, the gap is logged once as a warning naming the member to implement, andPdsRepoManager.IsRepositoryEnumerationUnsupportedreports it. The sync endpoints, which genuinely can't work without the record set, still surface theNotSupportedException— so the gap is still reported where it matters.On the dead-constructor point:
PdsServiceandPdsRepoManagerare now registered via explicit factories, so which constructor runs is stated in code rather than falling out of the container's greediest-constructor heuristic. The 4-arg constructor stays for hosts that construct the service themselves, and its doc comment now says so instead of implying DI might pick it.listReposhardcodedactive = true— fixed. NewListRepoListingsAsync/RepoListingreads each account's status from the account store, the same sourcegetRepoStatususes, so the two endpoints agree. A head whose account is gone reportsdeleted;statusis omitted for active repos.Firehose inlining the whole MST — fixed rather than left as a tradeoff. Added
MerkleSearchTree.SerializeProof(keys), which serializes only the root plus the nodes on the root→key search paths. Both#commitandcom.atproto.sync.getRecorduse it, so frame size now grows with the operation count and the log of repo size. A 300-record repo's commit frame no longer tripstooBig.Unbounded lock dictionary — bounded. The per-DID gate is dropped on repo deletion (removed, not disposed — a racing commit must not hit a disposed semaphore).
Verification
dotnet buildclean (0 errors, no new warnings on touched files). Unit tests in Release, matching CI: 1245 passed, 0 failed — 1229 was the post-merge baseline, so 16 new tests. They cover CRUD surviving a non-enumerating store,CommitAsyncreturning null and publishing nothing, the sync surface still throwing,listReposagreeing withgetRepoStatusfor a deactivated account, DI resolving a federatingPdsService, and seven MST proof tests including one that walks a proof block-set root→key the way a relay would.CHANGELOG.mdanddocs/pds.mdupdated — the docs previously described the old block-inlining behaviour and the compatibility promise the code wasn't keeping.⏱ 13m 51s · 113 turns · tokens: 142.2k in / 51.1k out (+9959.7k cache-read)
📜 Full session transcript