perf: leaner XRPC request path (HTTP/2, single JSON serialization, DPoP) #201

Merged

Grandiras merged 1 commit from issue-194-request-path-perf into main 2026-10-05 12:09:04 +00:00

Conversation 12 Commits 1 Files changed 10 +77 -111

Grandiras commented 2026-10-05 02:29:46 +00:00

Owner

Copy link

Closes #194.

The request path was already lean, so these are small, measured wins plus dead-code removal. There is no public API change (PublicAPI.*.txt untouched).

• HTTP/2 offered on XRPC calls (XrpcClient.CreateMessage): Version = 2.0 with RequestVersionOrLower. Concurrent calls to one PDS or AppView multiplex over a single connection. Plain-http and handlers without h2 fall back to 1.1. Against plc.directory the same change cut a concurrent burst from ~550 ms to ~335 ms p50 (#197).
• JSON bodies serialized once, into a ByteArrayContent reused across retries. Requests carry a Content-Length instead of going out chunked.
• DPoP proofs take the parsed request Uri directly, skipping a render and re-parse per request (about −240 B per DPoP request). The htu normalization is unchanged; the public string overloads share the same code.
• The debug log call is guarded by IsEnabled.
• Dead code removed: AuthorizationServerDiscovery.ResolveFromIdentifierAsync, the two Fetch*MetadataAsync wrappers, XrpcClient.HasAdminCredentials and ClearAdminCredentials. The tests that drove the discovery wrappers go through a small test-side extension.

Scenario (fake handler, in-process)	Baseline alloc	Candidate alloc
getProfile, anonymous	3.82 KB	3.78 KB
getProfile, DPoP	6.43 KB	6.20 KB
createRecord, bearer	4.21 KB	4.27 KB (bytes held for retry, now with Content-Length)
createRecord, DPoP	6.78 KB	6.64 KB
getRecord, typed	4.69 KB	4.65 KB

Net src: −34 lines. Build has 0 warnings; 4962 unit tests pass.

🤖 Generated with Claude Code

Closes #194. The request path was already lean, so these are small, measured wins plus dead-code removal. There is no public API change (`PublicAPI.*.txt` untouched). - **HTTP/2 offered on XRPC calls** (`XrpcClient.CreateMessage`): `Version = 2.0` with `RequestVersionOrLower`. Concurrent calls to one PDS or AppView multiplex over a single connection. Plain-http and handlers without h2 fall back to 1.1. Against plc.directory the same change cut a concurrent burst from ~550 ms to ~335 ms p50 (#197). - **JSON bodies serialized once**, into a `ByteArrayContent` reused across retries. Requests carry a `Content-Length` instead of going out chunked. - **DPoP proofs** take the parsed request `Uri` directly, skipping a render and re-parse per request (about −240 B per DPoP request). The `htu` normalization is unchanged; the public string overloads share the same code. - **The debug log call** is guarded by `IsEnabled`. - **Dead code removed:** `AuthorizationServerDiscovery.ResolveFromIdentifierAsync`, the two `Fetch*MetadataAsync` wrappers, `XrpcClient.HasAdminCredentials` and `ClearAdminCredentials`. The tests that drove the discovery wrappers go through a small test-side extension. | Scenario (fake handler, in-process) | Baseline alloc | Candidate alloc | |---|---|---| | getProfile, anonymous | 3.82 KB | 3.78 KB | | getProfile, DPoP | 6.43 KB | 6.20 KB | | createRecord, bearer | 4.21 KB | 4.27 KB (bytes held for retry, now with Content-Length) | | createRecord, DPoP | 6.78 KB | 6.64 KB | | getRecord, typed | 4.69 KB | 4.65 KB | Net `src`: −34 lines. Build has 0 warnings; 4962 unit tests pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

Grandiras added 1 commit 2026-10-05 02:29:46 +00:00

perf: leaner XRPC request path (HTTP/2, Content-Length bodies, cheaper DPoP proofs) and dead-code removal (closes #194) ... f68d55d382

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

opencode-bot commented 2026-10-05 02:29:47 +00:00

Collaborator

Copy link

🤖 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. <!-- opencode-run:review --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-201/run-2026-10-05T02-29-47-137Z.md) — updating every ~5 minutes while the review runs.

Grandiras referenced this pull request 2026-10-05 02:38:45 +00:00

refactor: debloat the auth and server layers #204

opencode-reviewer approved these changes 2026-10-05 02:48:19 +00:00

Dismissed

opencode-reviewer left a comment

Copy link

✅ Verdict: approve — no blocking issues found.

Small, well-scoped performance/dead-code pass with no correctness issues found.

• HTTP/2: Version = 2.0 + RequestVersionOrLower is the right pattern — ALPN negotiation on TLS, automatic 1.1 fallback for plain http, handlers without h2 or proxy paths. Nothing in the client or readers depends on the response version.
• Single JSON serialization: the ByteArrayContent factory is safe across the retry loop (each attempt gets a fresh content over the same serialized bytes; serialization is lazy so a null body still sends none). Content-Length comes for free, and the "bytes held for retry" trade-off is acknowledged in the PR table.
• DPoP: the Uri overloads compute htu via the same GetComponents normalization as before; userinfo/query/fragment handling is unchanged, and both public overloads keep their argument contracts (null → ArgumentNullException; non-absolute/no-host → ArgumentException), matching the existing tests.
• Dead code: only internal members removed; OAuthClient performs the identity → PDS → authorization-server steps inline with identical semantics, and the tests cover themselves through DiscoveryTestExtensions.

Minor observation (not blocking): the tested adjustedScopeString-style cleanups and the XrpcParams.AddAll rewrite are pure refactors with equivalent semantics; the log call behind IsEnabled avoids the arguments allocation only when a logger that filters Debug is installed — with NullLogger it's a skip either way.

---

⏱ 18m 31s · 13 steps · $0.0303 · tokens: 117.7k in / 1.5k out (+96.4k cache-read) · Go: 5h 0% · wk 0% · mo 91%
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. Small, well-scoped performance/dead-code pass with no correctness issues found. - **HTTP/2**: `Version = 2.0` + `RequestVersionOrLower` is the right pattern — ALPN negotiation on TLS, automatic 1.1 fallback for plain http, handlers without h2 or proxy paths. Nothing in the client or readers depends on the response version. - **Single JSON serialization**: the `ByteArrayContent` factory is safe across the retry loop (each attempt gets a fresh content over the same serialized bytes; serialization is lazy so a null body still sends none). Content-Length comes for free, and the "bytes held for retry" trade-off is acknowledged in the PR table. - **DPoP**: the `Uri` overloads compute `htu` via the same `GetComponents` normalization as before; userinfo/query/fragment handling is unchanged, and both public overloads keep their argument contracts (null → ArgumentNullException; non-absolute/no-host → ArgumentException), matching the existing tests. - **Dead code**: only internal members removed; `OAuthClient` performs the identity → PDS → authorization-server steps inline with identical semantics, and the tests cover themselves through `DiscoveryTestExtensions`. Minor observation (not blocking): the tested `adjustedScopeString`-style cleanups and the `XrpcParams.AddAll` rewrite are pure refactors with equivalent semantics; the log call behind `IsEnabled` avoids the arguments allocation only when a logger that filters Debug is installed — with `NullLogger` it's a skip either way. --- ⏱ 18m 31s · 13 steps · $0.0303 · tokens: 117.7k in / 1.5k out (+96.4k cache-read) · Go: 5h 0% · wk 0% · mo 91% 📜 [Full session transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-201/run-2026-10-05T02-29-47-137Z.md) <!-- opencode-reviewed-head:f68d55d3821c97e941c83087ae7032e263e9f311 --> <!-- opencode-verdict:approve -->

Grandiras force-pushed issue-194-request-path-perf from f68d55d382 to 0e0dd80e14 2026-10-05 02:58:16 +00:00 Compare

opencode-bot commented 2026-10-05 03:29:03 +00:00

Collaborator

Copy link

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

🤖 Reviewing this PR — the review will link the full session transcript. <!-- opencode-run:review -->

Grandiras force-pushed issue-194-request-path-perf from 0e0dd80e14 to 1abe9dfd6d 2026-10-05 04:07:21 +00:00 Compare

opencode-bot commented 2026-10-05 04:08:24 +00:00

Collaborator

Copy link

🤖 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. <!-- opencode-run:review --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-201/run-2026-10-05T04-08-23-450Z.md) — updating every ~5 minutes while the review runs.

opencode-reviewer approved these changes 2026-10-05 04:09:00 +00:00

opencode-reviewer left a comment

Copy link

✅ Verdict: approve — no blocking issues found.

Re-review of the current head: unchanged from the state I approved earlier — no blockers, then or now.

• HTTP/2: Version = 2.0 with RequestVersionOrLower is correct — ALPN on TLS, automatic 1.1 fallback for plain http, handlers without h2, and proxies. Nothing downstream depends on the response version.
• Single JSON serialization: the ByteArrayContent factory is safe across retries (a fresh content over the same serialized bytes each attempt; ByteArrayContent wraps without copying) and gives Content-Length for free; the "bytes held for retry" trade-off is acknowledged in the PR table. Serialization stays deferred to first use, so failure timing matches the old JsonContent behaviour.
• DPoP: the Uri overloads go through the same GetComponents(SchemeAndServer | Path) normalization as before (query/fragment/userinfo handling unchanged), and both public string overloads keep their argument contracts — null → ArgumentNullException, non-absolute/no-host → ArgumentException naming url — as the existing DPoPProofGeneratorTests expect. The relaively relative/parsed-URI split is internal-only, with InternalsVisibleTo where the tests need it.
• Dead code: only internal members removed (ResolveFromIdentifierAsync, the two Fetch*MetadataAsync wrappers, HasAdminCredentials/ClearAdminCredentials); no stale references remain in src, and the tests cover the same paths through DiscoveryTestExtensions.

Minor non-blocking notes:

• In DPoPProofGenerator.GenerateProof, the越过 htu is null branch now raises ArgumentException("The URL is not an absolute URL naming a host.", "url") with a literal param name rather than a nameof — same value, so no behavioural difference.
• The IsEnabled(LogLevel.Debug) guard avoids the format-argument allocation only when a filtering logger is installed; with NullLogger it was already a skip. Pure hygiene either way.

Net effect is a small, measured improvement with no public API or behavioural change, per the PR's scenario table. Good to merge.

---

⏱ 0m 37s · 11 steps · $0.0122 · tokens: 36.9k in / 1.3k out (+132.6k cache-read) · Go: 5h 1% · wk 0% · mo 91%
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. Re-review of the current head: unchanged from the state I approved earlier — no blockers, then or now. - **HTTP/2**: `Version = 2.0` with `RequestVersionOrLower` is correct — ALPN on TLS, automatic 1.1 fallback for plain http, handlers without h2, and proxies. Nothing downstream depends on the response version. - **Single JSON serialization**: the `ByteArrayContent` factory is safe across retries (a fresh content over the same serialized bytes each attempt; `ByteArrayContent` wraps without copying) and gives `Content-Length` for free; the "bytes held for retry" trade-off is acknowledged in the PR table. Serialization stays deferred to first use, so failure timing matches the old `JsonContent` behaviour. - **DPoP**: the `Uri` overloads go through the same `GetComponents(SchemeAndServer | Path)` normalization as before (query/fragment/userinfo handling unchanged), and both public string overloads keep their argument contracts — null → `ArgumentNullException`, non-absolute/no-host → `ArgumentException` naming `url` — as the existing `DPoPProofGeneratorTests` expect. The relaively relative/parsed-URI split is internal-only, with `InternalsVisibleTo` where the tests need it. - **Dead code**: only internal members removed (`ResolveFromIdentifierAsync`, the two `Fetch*MetadataAsync` wrappers, `HasAdminCredentials`/`ClearAdminCredentials`); no stale references remain in `src`, and the tests cover the same paths through `DiscoveryTestExtensions`. Minor non-blocking notes: - In `DPoPProofGenerator.GenerateProof`, the越过 `htu is null` branch now raises `ArgumentException("The URL is not an absolute URL naming a host.", "url")` with a literal param name rather than a `nameof` — same value, so no behavioural difference. - The `IsEnabled(LogLevel.Debug)` guard avoids the format-argument allocation only when a filtering logger is installed; with `NullLogger` it was already a skip. Pure hygiene either way. Net effect is a small, measured improvement with no public API or behavioural change, per the PR's scenario table. Good to merge. --- ⏱ 0m 37s · 11 steps · $0.0122 · tokens: 36.9k in / 1.3k out (+132.6k cache-read) · Go: 5h 1% · wk 0% · mo 91% 📜 [Full session transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-201/run-2026-10-05T04-08-23-450Z.md) <!-- opencode-reviewed-head:1abe9dfd6dc43254766ceac2adde3a71071b78ae --> <!-- opencode-verdict:approve -->

Grandiras commented 2026-10-05 11:49:29 +00:00

Author

Owner

Copy link

@claude /review

@claude /review

claude-bot commented 2026-10-05 11:49:34 +00:00

Collaborator

Copy link

🤖 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/729); the reply will link the full session transcript. <!-- claude-run:pr_review --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-201/run-729.md) — updating every ~5 minutes while the task runs.

claude-reviewer approved these changes 2026-10-05 11:50:00 +00:00

Dismissed

claude-reviewer left a comment

Copy link

✅ Verdict: approve — no blocking issues found.

I read the diff only and did not build or run the tests. It looks correct and matches the description.

• HTTP/2: Version = 2.0 with RequestVersionOrLower is the right policy. Plain-http and handlers without h2 fall back to 1.1.
• JSON body: JsonBody serializes lazily once and builds a fresh ByteArrayContent per attempt, so retries are safe and the request gets a Content-Length. The content type is still the shared JsonMediaType, as before.
• DPoP: NormalizeHtu(Uri) produces the same htu as the string path, and the IsAbsoluteUri check covers relative URIs. The public string overloads still throw ArgumentNullException for null and ArgumentException for non-absolute URLs.
• Dead code: removing ResolveFromIdentifierAsync, the two Fetch*MetadataAsync wrappers, HasAdminCredentials and ClearAdminCredentials is consistent. The test-side extension keeps the discovery tests compiling.

Minor notes, non-blocking:

• The ArgumentException for a bad htu no longer includes the offending URL.
• The ObjectDisposedException check in GenerateProof now runs after NormalizeHtu, so a disposed generator given a bad URL throws ArgumentException rather than ObjectDisposedException.
• The ClearAdminCredentials_RemovesAuthorizationHeader test was deleted along with the method, which is fine since nothing calls it.

---

🧠 claude-sonnet-5-5 · ⏱ 0m 11s · 2 turns · tokens: 32.4k in / 1.4k out (+0 cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. I read the diff only and did not build or run the tests. It looks correct and matches the description. - **HTTP/2:** `Version = 2.0` with `RequestVersionOrLower` is the right policy. Plain-http and handlers without h2 fall back to 1.1. - **JSON body:** `JsonBody` serializes lazily once and builds a fresh `ByteArrayContent` per attempt, so retries are safe and the request gets a `Content-Length`. The content type is still the shared `JsonMediaType`, as before. - **DPoP:** `NormalizeHtu(Uri)` produces the same `htu` as the string path, and the `IsAbsoluteUri` check covers relative URIs. The public string overloads still throw `ArgumentNullException` for null and `ArgumentException` for non-absolute URLs. - **Dead code:** removing `ResolveFromIdentifierAsync`, the two `Fetch*MetadataAsync` wrappers, `HasAdminCredentials` and `ClearAdminCredentials` is consistent. The test-side extension keeps the discovery tests compiling. Minor notes, non-blocking: - The `ArgumentException` for a bad `htu` no longer includes the offending URL. - The `ObjectDisposedException` check in `GenerateProof` now runs after `NormalizeHtu`, so a disposed generator given a bad URL throws `ArgumentException` rather than `ObjectDisposedException`. - The `ClearAdminCredentials_RemovesAuthorizationHeader` test was deleted along with the method, which is fine since nothing calls it. --- 🧠 claude-sonnet-5-5 · ⏱ 0m 11s · 2 turns · tokens: 32.4k in / 1.4k out (+0 cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-201/run-729.md) <!-- claude-verdict:approve --> <!-- claude-reviewed-head:1abe9dfd6dc43254766ceac2adde3a71071b78ae -->

Grandiras force-pushed issue-194-request-path-perf from 1abe9dfd6d to 3de051cb98 2026-10-05 12:07:09 +00:00 Compare

opencode-bot commented 2026-10-05 12:08:12 +00:00

Collaborator

Copy link

🤖 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. <!-- opencode-run:review --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-201/run-2026-10-05T12-08-11-595Z.md) — updating every ~5 minutes while the review runs.

claude-bot commented 2026-10-05 12:08:14 +00:00

Collaborator

Copy link

🤖 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/734); the reply will link the full session transcript. <!-- claude-run:pr_review --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-201/run-734.md) — updating every ~5 minutes while the task runs.

claude-reviewer approved these changes 2026-10-05 12:08:34 +00:00

claude-reviewer left a comment

Copy link

✅ Verdict: approve — no blocking issues found.

I re-read the diff only and did not build or run the tests. My earlier approval had no blocking issues, so there was nothing to resolve. Nothing new blocks merging.

• HTTP/2: Version = 2.0 with RequestVersionOrLower falls back to 1.1 for plain-http and handlers without h2.
• JSON body: The bytes are serialized once and each attempt gets a fresh ByteArrayContent. Retries are therefore safe, and the request carries a Content-Length.
• DPoP: The Uri overload and the string overloads share the same htu normalization.
• Dead code: The removals are consistent with the test-side extension.
• CHANGELOG: The new entry is present and follows the repo's style.

Minor, non-blocking notes (the first two were in my earlier review):

• The ArgumentException for a bad htu no longer names the offending URL.
• GenerateProof now checks ObjectDisposedException after normalizing the URL. A disposed generator given a bad URL therefore throws ArgumentException instead.

---

🧠 claude-sonnet-5-5 · ⏱ 0m 5s · 2 turns · tokens: 21.1k in / 596 out (+12.2k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. I re-read the diff only and did not build or run the tests. My earlier approval had no blocking issues, so there was nothing to resolve. Nothing new blocks merging. - **HTTP/2:** `Version = 2.0` with `RequestVersionOrLower` falls back to 1.1 for plain-http and handlers without h2. - **JSON body:** The bytes are serialized once and each attempt gets a fresh `ByteArrayContent`. Retries are therefore safe, and the request carries a `Content-Length`. - **DPoP:** The `Uri` overload and the string overloads share the same `htu` normalization. - **Dead code:** The removals are consistent with the test-side extension. - **CHANGELOG:** The new entry is present and follows the repo's style. Minor, non-blocking notes (the first two were in my earlier review): - The `ArgumentException` for a bad `htu` no longer names the offending URL. - `GenerateProof` now checks `ObjectDisposedException` after normalizing the URL. A disposed generator given a bad URL therefore throws `ArgumentException` instead. --- 🧠 claude-sonnet-5-5 · ⏱ 0m 5s · 2 turns · tokens: 21.1k in / 596 out (+12.2k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-201/run-734.md) <!-- claude-verdict:approve --> <!-- claude-reviewed-head:3de051cb98d2eb76a609cd4af16067903e47fed9 -->

Grandiras commented 2026-10-05 12:09:03 +00:00

Author

Owner

Copy link

Approval carry-over: this head (3de051c) is a CHANGELOG-only rebase of the approved 1abe9dfd. The PR's patch against its merge base, excluding CHANGELOG.md, has the same git patch-id (d3b6dbe1). CI is green on the new head, and claude-reviewer approved it.

Approval carry-over: this head (3de051c) is a CHANGELOG-only rebase of the approved 1abe9dfd. The PR's patch against its merge base, excluding `CHANGELOG.md`, has the same `git patch-id` (d3b6dbe1). CI is green on the new head, and claude-reviewer approved it.

Grandiras merged commit 100012dfd2 into main 2026-10-05 12:09:04 +00:00

Grandiras deleted branch issue-194-request-path-perf 2026-10-05 12:09:04 +00:00

Grandiras referenced this pull request from a commit 2026-10-05 12:09:05 +00:00

perf: leaner XRPC request path (HTTP/2, single JSON serialization, DPoP) (#201)

Sign in to join this conversation.

Reviewers

No reviewers

opencode-reviewer

claude-reviewer

Labels

Clear labels   

breaking-change

Introduces a breaking API change

  

bug

Something is not working correctly

  

documentation

Documentation improvements

  

duplicate

This issue or PR already exists

  

enhancement

New feature or improvement

  

good first issue

Good for newcomers to the project

  

help wanted

Extra attention is needed

  

performance

Performance-related issue or improvement

  

question

Further information is requested

  

wontfix

This will not be worked on

No labels

breaking-change

bug

documentation

duplicate

enhancement

good first issue

help wanted

performance

question

wontfix

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

5 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Grandiras/ATProto.NET!201

No description provided.