refactor(http)!: one error model and a robust XRPC transport #152
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
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Grandiras/ATProto.NET!152
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "issue-117-xrpc-transport"
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 #117.
One exception model for the whole SDK, and an XRPC transport that is safe to share between clients and callers, streams large responses, and cannot hang on a rate limit.
Error model
AtProtoException(namespaceATProtoNet) is the abstract base of every exception the SDK raises itself. Argument errors, misuse, cancellation and transport faults (HttpRequestExceptionwhen no response arrived) stay as .NET's own.XrpcException : AtProtoException(namespaceATProtoNet.Http) replacesAtProtoHttpException. It carriesNsid,Error(never null: without an envelope it gets the XRPC name for the status, e.g.UpstreamFailurefor 502),ErrorMessage,StatusCode(HttpStatusCode, no longer shadowingHttpRequestException's),ResponseBody,HeadersandIs(name).XrpcRateLimitException(429 that could not be waited out):RetryAfter,RateLimit.XrpcAuthenticationException: any 401, plusExpiredToken/InvalidToken(a PDS answers those with 400).XrpcResponseFormatException : AtProtoException: a 2xx body that does not deserialize (XRPC calls,RecordCollection<T>,RepoClient.GetRecordAsync<T>), withNsidand theJsonExceptioninside.XrpcErrors: constants for the generic XRPC names and the commoncom.atproto.*ones.ATProtoNet.Server.Xrpc.XrpcExceptionis deleted; hosted endpoints throw the core type.SpaceVerificationExceptionderives from it. The routing's behaviour is unchanged: the{error, message}body (message falls back to the error name as before), the status, andHeaders(WWW-Authenticate) are written as before. I did not keep a same-named subclass inATProtoNet.Server.Xrpc: every file that uses both namespaces would hit CS0104 ambiguity, which is worse than the one-lineusingmigration.AtProtoException:OAuthException(ErrorCoderenamedError),SpaceCredentialException,SpaceTokenException,SpaceRepoVerificationException,JetstreamConnectException(gains anErrorinit property),JetstreamArchiveException.PlcException/DidWebExceptionare left to the identity issue.XrpcResponseReader, internal) replaces the copies inXrpcClient,AtProtoHttpException,SpaceCredentialProvider(its duplicateXrpcErrorBodyis gone),JetstreamArchiveClientandJetstreamDictionaryClient. The Jetstream archive client also uses itsRetry-Afterparser (9.11). It reuses #151's primitives: error bodies are read throughBoundedContent.ReadBoundedAsyncwith a 64 KiB cap (past it the body is not read and the status names the error), and the envelope fields throughJsonElementExtensions.GetStringOrNull.XrpcErrors/ex.Is(...); server throws useXrpcErrors.InvalidRequest.RecordCollection.ExistsAsyncnow treats onlyRecordNotFoundas absence (it also accepted anyInvalidRequest).docs/error-handling.mdis rewritten around the hierarchy, and documents the namespaces, what is not wrapped, rate limits, timeouts and server-side use.Transport (
XrpcClient)/xrpc/, as the spec requires);HttpClient.BaseAddressis never read or written.AtProtoClient.PdsUrl/SetPdsUrl(string)becomeUri ServiceUrl/SetServiceUrl(Uri).InstanceUrlalways wins over a supplied client'sBaseAddress(same forPdsAdminClientandPdsAdminOptions.Url).SetServiceUrlkeeps the HTTPS-unless-loopback rule, since it is fed from DID documents and OAuth sessions; the constructor keeps trusting configured URLs (e.g.http://pds:3000in a container network) but now rejects non-absolute and non-http(s) ones up front.AtProtoClientOptions.UserAgent(defaultATProtoNet/<version>;nullleaves theHttpClient's default). No SDK code appends to a sharedDefaultRequestHeadersany more (AtProtoClient,PdsAdminClient,SpaceReader; the Blazor OAuth service only sets it on the client it owns; the Aspire named client's deadATProtoNet.Aspire/1.0default is removed).XrpcCallOptions(Proxy,AcceptLabelers,Headers,Timeout) onAtProtoClient.QueryAsync/ProcedureAsync. The client-wideSetProxy/SetLabelersstay as defaults (I did not obsolete them; they are fine for single-user clients). Session calls (createSession,refreshSession,getSession,deleteSession,createAccount) are never routed through the client-wide proxy, since sending them unauthenticated-with-proxy would now reroute sign-in to the AppView.Authorization/DPoPcannot be set throughHeaders. ATimeoutthrowsTimeoutException; the caller's own token still throwsOperationCanceledException. Session state (tokens + DPoP key) is swapped as one immutable snapshot.ResponseHeadersRead+JsonSerializer.DeserializeAsyncfrom the stream (F5).AtProtoHttp.SharedHandler, aSocketsHttpHandlerwithAutomaticDecompression = All,PooledConnectionLifetime = 5 min,ConnectTimeout = 10 s, used withdisposeHandler: falseby every SDK-owned client (AtProtoClient,PdsAdminClient,SpaceCredentialProvider/readers,JetstreamArchiveClient,JetstreamDictionaryClient,PlcClient,DidWebResolver, the Blazor OAuth service). Falls back toHttpClientHandlerwhere sockets are unsupported (Blazor WASM). The named DI clients (AddAtProtoServer,AddAtProto,AddAtProtoScoped,AddAtProtoClient,AddAtProtoSpaces,AddAtProtoPdsAdmin) getConfigurePrimaryHttpMessageHandler(AtProtoHttp.CreateHandler), nothing else (#136 redesigns DI). This needsInternalsVisibleToforATProtoNet.Server(already added by #151) andATProtoNet.Blazor(added here) in the core csproj.AtProtoClientOptions.RateLimit=XrpcRateLimitOptions { MaxRetries = 3, MaxDelay = 30 s }. A requested wait beyondMaxDelaythrowsXrpcRateLimitExceptionimmediately withRetryAfter;Retry-Afteris parsed as seconds or HTTP date, falling back toRateLimit-Reset; up to 10 % jitter; our own exponential back-off is clamped toMaxDelay.MaxRetries = 0lets an outer resilience handler own retries.AtProtoJsonDefaults.FormatTimestamp/NowTimestampnow format in the invariant culture; underth-THeverycreatedAtthe SDK stamped carried a Buddhist-calendar year.XrpcParamsformats timestamps through it.XrpcQueryBuilderis merged intoXrpcParams(XrpcParams.From(object?)), with a per-type property cache.DateTime/DateTimeOffset→yyyy-MM-ddTHH:mm:ss.fffZ(invariant culture and calendar;Unspecifiedtaken as UTC), enums → their JSON name viaAtProtoJsonDefaults.Options(honours[JsonStringEnumMemberName]), identifiers →ToString(). New overloads:Add(string, long?),Add(string, DateTimeOffset?),Add<T>(string, T?) where T : struct, ISpanFormattable.XrpcStreamResponse : IAsyncDisposable, IDisposable(Content,ContentType,ContentLength) fromSyncClient.GetBlobAsync/GetRepoAsyncandSpaceClient.GetBlobAsync/GetRepoAsync; it owns theHttpResponseMessage. A failed download now disposes its response too.HttpContent(Content-Length computed from that position). A non-seekable stream that needs a retry throwsInvalidOperationException("…cannot be rewound…")with the XRPC error as inner exception, instead of resending an empty body.QueryAsync<T>,ProcedureAsync<T>/ProcedureAsync(body asobject?, serialized by runtime type),UploadAsync<T>,DownloadAsync, and oneReadJsonAsync;ProcedureWithRefreshTokenAsyncgoes through the same pipeline. The three internal proxy overloads becameXrpcCallOptions. Removed:BaseUrl,IsAuthenticated,UpdateDPoPNonce, the body-lessQueryAsync,_useDPoP, the no-opIDisposable.ILoggeris optional.XrpcClientis internal. Nothing public exposed it (every sub-client constructor was already internal, andAtProtoClientnever returned it); its only doc mention wasmanaged-pds.md. #122 is the place for a public transport seam, designed as such rather than inherited from this class. The ~200 Lexicon call sites were updated mechanically (ProcedureAsync<TReq, TResp>→ProcedureAsync<TResp>, void generic calls → non-generic, positional cancellation tokens named);EmptyResponseBodyTestscaught the one void call that would have silently rebound to the deserializing overload.Breaking changes and migration
AtProtoHttpException→XrpcException(no longer anHttpRequestException);ErrorType→Errorcatch (XrpcException ex) when (ex.Is(XrpcErrors.X)); catchXrpcExceptionwhereHttpRequestExceptionwas caught for XRPC errorsATProtoNet.Server.Xrpc.XrpcExceptionremoved; status isHttpStatusCode(also onSpaceVerificationException)using ATProtoNet.Http;,HttpStatusCode.NotFoundforStatusCodes.Status404NotFoundAtProtoException;OAuthException.ErrorCode→Errorex.ErrorXrpcResponseFormatExceptionJsonException/InvalidOperationExceptionPdsUrl/SetPdsUrl(string)→ServiceUrl/SetServiceUrl(Uri)SetServiceUrl(new Uri(url))HttpClient.BaseAddressignoredInstanceUrl/PdsAdminOptions.UrlQueryAsync/ProcedureAsyncgainXrpcCallOptions? optionsbefore the tokencancellationToken:by nameXrpcStreamResponseawait using var r = …; r.ContentXrpcClientinternal;MaxRateLimitRetries→AtProtoClientOptions.RateLimit.MaxRetriesAtProtoClient.QueryAsync/ProcedureAsyncBehaviour changes without an API break (CHANGELOG Changed): compressed responses, 30 s rate-limit cap, proxy/labeler headers without a session, Aspire/Blazor UA handling,
SpaceReaders sharing the provider'sHttpClient(SpaceReader.Disposenow releases nothing).Verification
dotnet build(full solution,--no-incremental): 0 warnings, 0 errors.Unit tests: 2,824 passed, 0 failed, 5 skipped (2,829 total), on top of main at
06becd0(#149, #150, #151). The oldAtProtoHttpExceptionandXrpcQueryBuildertests were rewritten intoXrpcExceptionTestsandXrpcParamsTests.New tests:
XrpcTransportTests(BaseAddress ignored/untouched,SetServiceUrlandApplyOAuthSessionAsyncafter a request on a shared client, 100 clients on oneHttpClientleaveDefaultRequestHeaders.UserAgentempty and send one UA, per-client/nullUA, proxy+labelers without a session, per-call options vs defaults, 50 concurrent calls with different proxies, session calls unproxied, reserved headers, timeout vs caller cancellation, no-buffer streaming via a content that records whether HttpClient buffered it, format/empty-body errors, download metadata and disposal on success and failure, upload position/rewind/non-seekable, DPoP nonce retry withnonce/htuchecked in the proof),XrpcExceptionTests(properties, hierarchy for all rebased types, envelope/HTML/malformed bodies, 401/token-error mapping, 429 withRetry-Afterseconds and HTTP date,RateLimit-Resetfallback),XrpcClientRateLimitTests(cap exceeded throws at once withRetryAfter= 1 day or ~3 h fromRateLimit-Reset, HTTP date in past/future, retries exhausted, options wiring),XrpcParamsTests(merged builder tests; ISO timestamps underth-TH/fi-FI/ar-SA,Unspecified/LocalDateTime, camelCase and[JsonStringEnumMemberName]enums, identifiers, cached reflection),AtProtoHttpTests(handler settings, owned clients share the handler, disposing an owner leaves it usable,AddAtProtoServer's named client),RecordCollectionErrorTests(ExistsAsynconly onRecordNotFound, format errors fromGetAsyncandGetRecordAsync<T>),TimestampFormatTests(FormatTimestampunderth-TH,ja-JP,fi-FI,ar-SA), and error-body tests for the 64 KiB cap. The DPoP nonce test decodes the proof with #151'sJwt.TryDecode.Benchmark (the audit's
XrpcBench,QueryAsync<FeedResponse>, 50 items / 185,834 B, in-memory handler, BenchmarkDotNet MediumRun, same machine and run):XrpcClientBearer, this branchResponseContentRead+ReadFromJsonAsync(what v0.6 did)XrpcClientBearer, from the audit's runAbout −20 % to −23 % time, −32 % allocation, no Gen2 collections. DPoP adds ~20 µs and ~11 KB per call in the ShortRun (the MediumRun DPoP row was noisy); proof generation is unchanged here.
Rebase onto #149, #150, #151
Rebased onto main after #149, #150 and #151. Semantic conflicts fixed: #149's new
UpdateProfileAsynccaughtAtProtoHttpExceptionforInvalidSwap/RecordNotFound; it now matchesXrpcErrorsand reads the stored profile throughRepoClient.GetRecordAsync<ProfileRecord>, so a malformed one surfaces asXrpcResponseFormatException. #151's newOAuthException.ErrorCodetest assertion was moved toError; itsInternalsVisibleTo ATProtoNet.Serverline is kept and only the Blazor line is added.SpaceTokenExceptionkeeps #151's changes and derives fromAtProtoException.Review follow-up
Addressing the claude-bot review (COMMENT):
AtProtoHttp.NormalizeBaseUrl(both overloads) now rejects them withArgumentException, like the constructors'ValidateServiceUrl, and builds the result from scheme + authority + path (GetLeftPart(UriPartial.Path)) instead of the raw string. It also requireshttp/https: on Unix a rooted path such as/xrpcotherwise parses as an absolutefile:URI.ValidateServiceUrlchecks the same with the caller's parameter name, soInstanceUrl,SetServiceUrl,PdsAdminOptions.Url,PlcClient's directory URL andSpaceCredentialProvider.CreateReaderAsync's host URL all refuse such URLs up front. Endpoints read from DID documents go through a newTryNormalizeBaseUrland fail in each caller's own terms:SpaceCredentialProvider.ResolveHostAsyncthrowsSpaceCredentialException, and the managing-app client throws theInvalidOperationExceptionthe access policy already treats as a refusal. Tests cover both overloads (trailing slashes, ports, paths,?x=1, bare?,#frag, bare#, relative, non-http),ValidateServiceUrl's parameter name,AtProtoClient's constructor andSetServiceUrl, the reader host check, a DID document with a bad endpoint, and the managing-app refusal. CHANGELOG Changed has a bullet for it.Directand the proxy default. The request path did already suppress the client-wide default forIsDirectcalls. The precedence is now explicit in one place:ResolveProxy/ResolveLabelersare a per-call value if set, otherwise nothing for a direct call, otherwise the client default.Directis justIsDirect = true, andIsDirectnow suppresses the labeler default too, instead of relying on an emptyAcceptLabelerslist. TheProxy,AcceptLabelersandIsDirectXML docs state the precedence and list the session calls.SessionCalls_AreNotRoutedThroughTheDefaultProxynow callsSetProxyandSetLabelers, thenLoginAsync,RefreshSessionAsync,ResumeSessionAsync,Server.CreateAccountAsyncandLogoutAsync. It asserts that none of the five session requests carriesatproto-proxyoratproto-accept-labelers, and that an ordinary call in the same test carries both. A second test pins that an explicit per-callProxystill wins on a direct call.Deferred / flagged
AtProtoClientOptions.InstanceUrlstays astring(config binding, builder, Aspire settings); aUrithere belongs with the options redesign in #136.FindAsync/FindRecordAsync) andWithDefaults(XrpcCallOptions)views are not in the issue checklist and are left out.ConfigureAwait(false)(5.9), and DPoP nonce sharing across clients (F3) are later PRs.🤖 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: comment — notes only, nothing blocking.
This is a large, well-documented refactor and the visible portions are consistent with the PR description: exception hierarchy, CHANGELOG entries, docs, and the mechanical server-side call-site updates (
XrpcErrors.InvalidRequestinstead of string literals,HttpStatusCodeinstead ofStatusCodes.Status*,ServiceUrl/SetServiceUrlinstead ofPdsUrl/SetPdsUrl) all line up correctly with each other.Important caveat: the diff for
src/ATProtoNet/Http/XrpcClient.csis truncated mid-method (cuts off insideSendAsync, beforeApplyAuthHeader, the DPoP-nonce retry loop, the rate-limit retry loop,BuildUri, and response/error handling are shown). That is the highest-risk part of this PR — concurrent header application on a sharedHttpClient, DPoP nonce replay, and the new bounded rate-limit wait logic — and I could not verify its correctness from what's visible. I'd want a follow-up pass (or the full file) before treating this as fully reviewed.One small, low-severity issue spotted in the visible code below (
AtProtoHttp.NormalizeBaseUrl(Uri)), plus a design detail worth a sanity check (XrpcClient.Direct) called out inline. Everything else visible (constructors validating URLs,PdsAdminClientno longer trusting a suppliedHttpClient.BaseAddress, the DI extension methods wiringConfigurePrimaryHttpMessageHandler(AtProtoHttp.CreateHandler), the Blazor OAuth service change, theUpdateProfileAsyncrebase) looks correct and matches the stated behavior.⏱ 1m 54s · 1 turns · tokens: 78.8k in / 11.9k out (+23.9k cache-read)
📜 Full session transcript
@ -0,0 +68,4 @@/// <inheritdoc cref="NormalizeBaseUrl(string)"/>internal static Uri NormalizeBaseUrl(Uri url) =>url.AbsolutePath.EndsWith('/') ? url : new Uri(url.AbsoluteUri.TrimEnd('/') + "/", UriKind.Absolute);NormalizeBaseUrl(Uri url)builds the normalized URI fromurl.AbsoluteUri(which includes any query string/fragment) rather than just the path. If a service URL ever carried a query string and didn't already end in/on its path (e.g.https://pds.example/base?x=1), this produceshttps://pds.example/base?x=1/— a slash appended after the query string, corrupting the URL. Same issue applies to thestringoverload just above (url.TrimEnd('/') + "/"operates on the raw string including any query). Low real-world impact since service base URLs essentially never carry query strings, but worth guarding against (e.g. build fromUriBuilder.Pathinstead ofAbsoluteUri) since this is now used for admin/space endpoints as well as the main service URL.@ -725,0 +618,4 @@}// ──────────────────────────────────────────────────────────// TypesDirectsetsAcceptLabelers = []andIsDirect = truebut leavesProxyat its default (null), which perXrpcCallOptions.Proxy's own doc means "use the client's default, if any." For this to actually keep session calls (createSession/refreshSession/etc.) from being routed through the client-wideSetProxydefault — as the PR description promises — the (truncated, not visible in this diff) request-building code must special-caseoptions.IsDirectto suppress the proxy header regardless of the client default. Worth double-checking that path exists and is exercised by a test, since a regression here would silently start proxying sign-in traffic through an AppView once a caller has calledSetProxy.Addressed both review points in
a5c1e48(see Review follow-up in the description):NormalizeBaseUrl(both overloads) and the shared URL validator now reject a query or fragment. DID-document endpoints go throughTryNormalizeBaseUrl. Tests cover each case.@claude /review. The previous pass only saw a truncated diff of
src/ATProtoNet/Http/XrpcClient.cs. Please read that file in full from the PR branch: header application, the DPoP nonce retry, the bounded rate-limit wait,BuildUri, and response/error handling.5e49fbcf1ea5c1e487c2🤖 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.
Both blocking issues from my previous pass are fixed, verified directly against the branch source (the diff for
XrpcClient.cswas truncated again, so I read the full files fromorigin/issue-117-xrpc-transportrather than relying on the diff alone).AtProtoHttp.NormalizeBaseUrlquery/fragment corruption — now fixed correctly. Both overloads reject a URL with a query or fragment viaHasQueryOrFragmentbefore building the result, and the result is built fromurl.GetLeftPart(UriPartial.Path)rather than the raw string/AbsoluteUri, so the append-after-query corruption I flagged can no longer happen.ValidateServiceUrlandTryNormalizeBaseUrlapply the same checks consistently, and callers (PdsAdminClient,AtProtoClient,SpaceCredentialProvider,SimpleSpaceManagingAppClient,SpaceWriteNotifier) were updated to go through them.Direct/proxy-precedence for session calls — now explicit and correct.XrpcClient.ResolveProxy/ResolveLabelersimplement the documented precedence (per-call value wins; else nothing forIsDirect; else the client-wide default), andXrpcClient.Direct(IsDirect = true) is passed fromServerClienton all the session calls that need to bypass the client-wide proxy/labeler defaults:createSession,getSession,deleteSession,createAccount, andrefreshSession(viaProcedureWithRefreshTokenAsync, which also usesDirect). This matches the PR description's claim and the described test coverage.I did not re-review the entire ~2,800-line diff line by line this pass since the prior review already covered the bulk of it and found it sound; this pass was targeted at confirming the two previously-raised issues were actually resolved (not just claimed) by reading the current file contents directly. Both are. No new issues found.
⏱ 0m 46s · 16 turns · tokens: 101.6k in / 4.0k out (+1725.5k cache-read)
📜 Full session transcript
Supplementary review of the
XrpcClient.cssections the bot's truncated diff hid (SendAsync,GetRateLimitDelay,EnsureReplayableAsync,ReadJsonAsync,BuildUri,CreateMessage,ApplyAuthorization,ObserveResponseHeaders). Findings:jtiand re-created content. The nonce retry fires at most once per call. 429 retries are bounded byMaxRetries, and a requested wait aboveMaxDelayfalls through to the typed exception rather than sleeping.ResponseHeadersRead, and format errors becomeXrpcResponseFormatException.Nothing blocking found.