refactor: debloat the Spaces server, EF Core stores and streaming consumers #208
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
5 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Grandiras/ATProto.NET!208
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "issue-207-spaces-streaming-debloat"
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 #207.
A debloat of
src/ATProtoNet.Server/Spaces,src/ATProtoNet.Server.EntityFrameworkCore,src/ATProtoNet/StreamingandTap, with no behaviour or public API change (PublicAPI.*.txtuntouched).EventStreamFrame.cs, which is in #205, is untouched.DbContextFactoryExtensions.UseAsyncreplaces the create-context /await using/ query boilerplate at 15 sites. The optional cursor filter inListReposAsync/ListMembersAsyncstays a conditionalWhere, so the first page's SQL carries no@cursor IS NULL OR …that could defeat the index.SpaceServerOptions.ServiceDidRequired.RequireHttpshelper serves the attestation verifier's two https checks.RepoSyncVerifier,TryReadCommit/IsStale(bool+out error) becomeReadCommit/StaleReasonreturningstring?. The Sync 1.1 checks and their order are unchanged.CursorTracker,StreamSocket).Security-adjacent spots are brace-only:
DPoPProof,SpaceCredentialVerifier,SpaceDelegationTokenVerifier,SpaceRequestAuthenticator,SpaceAuthorityEndpointsandSimpleSpaceAccessPolicy.Net
src: about −190 lines. Build has 0 warnings; 4964 tests pass, including the EF stores on SQLite in memory.🤖 Generated with Claude Code
@claude /review
🤖 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.
✅ Verdict: approve — no blocking issues found.
I read the diff but did not build it or run the tests. It looks like a behaviour-preserving cleanup.
UseAsync: the helper has the same lifetime and cancellation semantics as the code it replaces. The one new call site that passesCancellationToken.Nonematches the old token-lessCreateDbContextAsync()in the JTI purge.Where, so the first page's SQL is unchanged.RepoSyncVerifier: the checks and their order are the same afterTryReadCommit/IsStalebecameReadCommit/StaleReason. Theout carandout blockvalues are assigned or nulled as before.||guards in the Jetstream parsers still satisfy definite assignment of the pattern variables.RequireHttps: it keeps the same scheme comparison and error messages.elseor nested-statement hazard.CursorTrackerandStreamSocketare internal, so the public API is unaffected.One minor point:
SpaceServerOptions.ServiceDidRequiredhardcodesSpaceServerOptions.ServiceDidin the string where the old code usednameof, so it will no longer follow a rename of the property. Consider a$"…{nameof(…)}"interpolatedconst, which C# allows for constant strings.🧠 claude-sonnet-5-5 · ⏱ 0m 12s · 2 turns · tokens: 40.4k in / 1.5k out (+12.2k cache-read)
📜 Full session transcript
🤖 Resolving the merge conflicts — the reply will link the full session transcript.
📜 Live transcript — updating every ~5 minutes while the fix runs.
Conflicted file:
CHANGELOG.mdonly.Cause: both commits appended a bullet right after the same anchor — the
Leaner Lexicon clientsline under[Unreleased] → ### Changed.Resolution: kept both bullets, one per line:
Leaner Spaces server, EF Core stores and streaming consumers … (#207)— the PR's own entryorigin/main'sIdentity fetches use HTTP/2 … (#197)The two commits touch disjoint file sets (verified with
git show --name-only+comm), so no code-side logic could clash and nothing else needed resolution. All 7 other files main had touched (src/ATProtoNet/Identity/*,docs/did-resolution.md,PublicAPI.Unshipped.txt,HandleResolverTests.cs) merged in cleanly frommain.Verified:
<<<<<<</=======/>>>>>>>markers left anywhere (remaining grep hits were====separator strings inside sample programs, not markers)dotnet buildover the whole solution: succeeded, 0 warnings, 0 errorsgit commit --no-edit); branch is 2 commits ahead, ready for the daemon to push→ Merge resolved and pushed; a re-review follows.
⏱ 0m 47s · 9 steps · $0.0112 · tokens: 36.6k in / 1.4k out (+134.7k cache-read) · Go: 5h 0% · wk 0% · mo 91%
📜 Full session transcript
dc7f07c6cbc630fc7a70🤖 Reviewing this PR — the review will link the full session transcript.
📜 Live transcript — updating every ~5 minutes while the review 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.
✅ Verdict: approve — no blocking issues found.
A faithful debloat with no correctness or behaviour regressions found. Specific checks:
DbContextFactoryExtensions.UseAsyncpreserves the old semantics: context factored with the caller's token, disposed viaawait usingon exception and success alike, and the work lambda gets the same token the old inline code used. All 15 call sites replicate the original queries verbatim, including the conditional cursorWhereinListMembersAsync/ListReposAsync(so the first-page SQL still avoids@cursor IS NULL OR …, as claimed).RepoSyncVerifier:TryReadCommit/IsStale→ReadCommit/StaleReasonkeep the exact check order (no blocks, size, CAR parse, commit DID, commit rev, future-TID) and messages; all call sites handle thestring?result identically.foreach+yield(JetstreamSegmentReader, JetstreamReplayConsumer) are semantically identical.ServiceDidRequiredconst matches the previously interpolated message string exactly, and beinginternalit leaves the public API (andPublicAPI.*.txt) untouched.CursorTracker/StreamSocketkeep theMath.Max(1, persistInterval)clamp and all captured-field logic intact; both types are internal.RequireHttpshelper short-circuits exactly like the two checks it replaces.Minor note, non-blocking:
SweepAsyncinEfCoreJtiReplayStoreswitched from an implicit token to explicitCancellationToken.Nonefor the factory and delete — that matches the old behaviour (which passed no token), just spelled out now.⏱ 1m 18s · 5 steps · $0.0068 · tokens: 27.2k in / 892 out (+68.0k cache-read) · Go: 5h 0% · wk 0% · mo 91%
📜 Full session transcript
✅ Verdict: approve — no blocking issues found.
I read the diff again but did not build it or run the tests. Nothing blocking turned up, and the change still looks like a behaviour-preserving cleanup.
My earlier note is still open and non-blocking.
SpaceServerOptions.ServiceDidRequiredstill hardcodesSpaceServerOptions.ServiceDid, so it won't follow a rename of the property. Aconstbuilt as$"…{nameof(…)}"would fix that.Everything else I checked last time is unchanged:
UseAsynckeeps the same context lifetime and cancellation behaviour as the code it replaces.RepoSyncVerifierchecks run in the same order.||guards still satisfy definite assignment of the pattern variables.🧠 claude-sonnet-5-5 · ⏱ 0m 6s · 2 turns · tokens: 41.4k in / 560 out (+12.2k cache-read)
📜 Full session transcript
c630fc7a705660eea196🤖 Reviewing this PR — the review will link the full session transcript.
📜 Live transcript — updating every ~5 minutes while the review 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.
✅ Verdict: approve — no blocking issues found.
I re-read the diff but did not build it or run the tests. It still looks like a behaviour-preserving cleanup, and nothing blocks merging.
My earlier note is still open and non-blocking.
SpaceServerOptions.ServiceDidRequiredhardcodesSpaceServerOptions.ServiceDidwhere the old messages usednameof, so it won't follow a rename of the property. Aconstbuilt as$"…{nameof(SpaceServerOptions)}.{nameof(ServiceDid)}…"would fix that, since C# allows interpolated constant strings.The rest is unchanged from my last pass:
UseAsynckeeps the same context lifetime and cancellation behaviour as the code it replaces.RepoSyncVerifierchecks run in the same order.||guards still satisfy definite assignment of the pattern variables.🧠 claude-sonnet-5-5 · ⏱ 0m 4s · 2 turns · tokens: 41.7k in / 566 out (+12.2k cache-read)
📜 Full session transcript
Approval carry-over: this head (
5660eea) is a CHANGELOG-only rebase ofc630fc7, which opencode-reviewer approved. The PR's patch against its merge base, excludingCHANGELOG.md, has the samegit patch-id(6e320e80), CI is green on the new head, and claude-reviewer approved it.✅ Verdict: approve — no blocking issues found.
Re-review of the current head: no new issues found. Everything I checked last time still holds:
DbContextFactoryExtensions.UseAsynckeeps the exact lifetime and cancellation semantics of the inline boilerplate (factory token,await usingdisposal on success and exception, same token handed to the work lambda), and the multi-statement upsert/fallback sites (EfCoreSimpleSpaceStore.PutMemberAsync,EfCoreSpaceAuthorityStore.DeleteSpaceAsync/RegisterNotifyAsync) still dispose the write context before falling through to the second context, matching the oldawait using (…) { }extents.@cursor IS NULL ORclause;SpacePaging.Pagestill receives one past-the-end row.RepoSyncVerifier: theTryReadCommit/IsStale→ReadCommit/StaleReasonconversion preserves the check order (no blocks, size limit, CAR parse, commit DID, commit rev, future-TID) and every message;carandblockare still force-initialized asnull!before the early returns, so all call sites compile and behave the same.did/time/collection/rkey(each operand is evaluated when the condition is false), preserving short-circuit order.foreach+yieldsites in the segment reader and replay consumer.CursorTracker/StreamSocketkeep theMath.Max(1, persistInterval)clamp and all captured-field logic; the public API surface (PublicAPI.*.txt) is untouched.SweepAsync's explicitCancellationToken.Nonematches the previous token-lessCreateDbContextAsync()/ExecuteDeleteAsync()behaviour.The one minor non-blocking note from my earlier passes is still open:
SpaceServerOptions.ServiceDidRequiredhardcodesSpaceServerOptions.ServiceDidin the literal and won't follow a rename of the property. Since it's an internalconst, this is purely rename-robustness polish — a$"…{nameof(SpaceServerOptions.ServiceDid)}."interpolated constant string (valid forconstin C# 10+) would make it rename-safe. Not a blocker; fine to land as is.⏱ 5m 14s · 23 steps · $0.0319 · tokens: 76.6k in / 2.4k out (+580.1k cache-read) · Go: 5h 0% · wk 1% · mo 91%
📜 Full session transcript