Grandiras/ATProto.NET

Fork 0

fix: pass AtProtoClient's clock to its XRPC transport #188

Merged

Grandiras merged 1 commit from issue-187-client-clock into main

2026-09-27 20:58:03 +00:00

Grandiras commented

2026-09-27 17:33:41 +00:00

Owner

Closes #187.

AtProtoClient's internal constructor, the one tests use to inject a clock, gave that clock only to SessionManager. The XrpcClient it built kept TimeProvider.System. So the 429 retry delays and the Retry-After / RateLimit-Reset arithmetic ignored the injected clock. The public constructor passes TimeProvider.System either way, so production behaviour is unchanged.

Changes

AtProtoClient: set TimeProvider = timeProvider in the XrpcClient initializer, next to UserAgent and RateLimit.
New XrpcClientRateLimitTests.AtProtoClient_BacksOffOnItsClock: two 429s without Retry-After, then 200. The client's clock must see the 1 s and 2 s back-off, within the 10 % jitter.
VideoUploadTests.UploadVideoAsync_BacksOffExponentially goes through AtProtoClient (client.Bsky.Video) instead of hand-building an XrpcClient + VideoClient. This drops the workaround and the comment #186 added to explain it.
The video test's private recording clock moves to TestSupport/InstantTimeProvider.cs, so both tests can use it.

Verification

With the one-line fix reverted, both back-off tests fail: the clock records no delays.
dotnet build passes with 0 warnings. tests/ATProtoNet.Tests: 5001 passed, 5 skipped.
One full-suite run had a single failure that I could not reproduce in 13 later runs, including runs under parallel load. The tests this change can affect are the ones that give AtProtoClient a non-system clock. None of them hits a 429 path, so the failure looks like an unrelated timing-sensitive test.

Note

Open PR #185 (debloat pass D) rewrites the same constructor and still has this gap. Whichever merges second gets a small conflict at the new XrpcClient(...) { … } initializer; keep the TimeProvider = timeProvider line.

🤖 Generated with Claude Code

Closes #187. `AtProtoClient`'s internal constructor, the one tests use to inject a clock, gave that clock only to `SessionManager`. The `XrpcClient` it built kept `TimeProvider.System`. So the 429 retry delays and the `Retry-After` / `RateLimit-Reset` arithmetic ignored the injected clock. The public constructor passes `TimeProvider.System` either way, so production behaviour is unchanged. ## Changes - `AtProtoClient`: set `TimeProvider = timeProvider` in the `XrpcClient` initializer, next to `UserAgent` and `RateLimit`. - New `XrpcClientRateLimitTests.AtProtoClient_BacksOffOnItsClock`: two 429s without `Retry-After`, then 200. The client's clock must see the 1 s and 2 s back-off, within the 10 % jitter. - `VideoUploadTests.UploadVideoAsync_BacksOffExponentially` goes through `AtProtoClient` (`client.Bsky.Video`) instead of hand-building an `XrpcClient` + `VideoClient`. This drops the workaround and the comment #186 added to explain it. - The video test's private recording clock moves to `TestSupport/InstantTimeProvider.cs`, so both tests can use it. ## Verification - With the one-line fix reverted, both back-off tests fail: the clock records no delays. - `dotnet build` passes with 0 warnings. `tests/ATProtoNet.Tests`: 5001 passed, 5 skipped. - One full-suite run had a single failure that I could not reproduce in 13 later runs, including runs under parallel load. The tests this change can affect are the ones that give `AtProtoClient` a non-system clock. None of them hits a 429 path, so the failure looks like an unrelated timing-sensitive test. ## Note Open PR #185 (debloat pass D) rewrites the same constructor and still has this gap. Whichever merges second gets a small conflict at the `new XrpcClient(...) { … }` initializer; keep the `TimeProvider = timeProvider` line. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

Grandiras added 1 commit

2026-09-27 17:33:41 +00:00

fix: pass AtProtoClient's clock to its XRPC transport (closes #187 )

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

Details

CI / build-and-test (pull_request) Successful in 1m5s

Details

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

Details

017e1bf8b3

- AtProtoClient's internal constructor now sets XrpcClient.TimeProvider,
  so 429 back-off and rate-limit arithmetic run on the injected clock
  rather than only session refresh seeing it
- Regression test: a retried call through AtProtoClient observes its
  back-off on the client's clock
- VideoUploadTests.UploadVideoAsync_BacksOffExponentially goes through
  AtProtoClient instead of hand-building an XrpcClient; its recording
  clock moves to TestSupport as InstantTimeProvider

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

opencode-bot commented

2026-09-27 17:33:43 +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-188/run-2026-09-27T17-33-42-824Z.md) — updating every ~5 minutes while the review runs.

opencode-bot commented

2026-09-27 17:33:45 +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-27T22:33:43.301Z) — 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-27T22:33:43.301Z) — nothing to re-trigger.

Grandiras commented

2026-09-27 17:44:16 +00:00

Author

Owner

@claude-reviewer

claude-bot commented

2026-09-27 17:44:18 +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/643); 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-188/run-643.md) — updating every ~5 minutes while the task runs.

claude-reviewer approved these changes

2026-09-27 17:45:14 +00:00

claude-reviewer left a comment

✅ Verdict: approve — no blocking issues found.

One-line fix: TimeProvider = timeProvider added to the XrpcClient object initializer in AtProtoClient's internal test constructor, next to UserAgent/RateLimit. This is exactly the gap described — the public constructor already forwards TimeProvider.System down the same path, so production behavior is unchanged, and only the injectable-clock test constructor was affected.

The new XrpcClientRateLimitTests.AtProtoClient_BacksOffOnItsClock test correctly exercises the fix end-to-end through AtProtoClient.QueryAsync, asserting the 1s/2s exponential back-off (with the documented 10% jitter tolerance) lands on the injected InstantTimeProvider rather than the system clock.

The VideoUploadTests.UploadVideoAsync_BacksOffExponentially rework is a clean simplification: it now goes through client.Bsky.Video instead of hand-assembling XrpcClient + VideoClient, which is possible precisely because the fix now plumbs the clock through the public-facing constructor path. Moving the private ManualTime/InstantTimeProvider clock to TestSupport/InstantTimeProvider.cs for reuse is a reasonable, minimal refactor with no behavior change (the class body is copied verbatim).

The CHANGELOG entry is correctly placed under Fixed in [Unreleased], references #187, and matches the required bullet style. No correctness or security issues found; this is a good, tightly-scoped fix.

⏱ 0m 49s · 12 turns · tokens: 25.9k in / 3.6k out (+533.0k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. One-line fix: `TimeProvider = timeProvider` added to the `XrpcClient` object initializer in `AtProtoClient`'s internal test constructor, next to `UserAgent`/`RateLimit`. This is exactly the gap described — the public constructor already forwards `TimeProvider.System` down the same path, so production behavior is unchanged, and only the injectable-clock test constructor was affected. The new `XrpcClientRateLimitTests.AtProtoClient_BacksOffOnItsClock` test correctly exercises the fix end-to-end through `AtProtoClient.QueryAsync`, asserting the 1s/2s exponential back-off (with the documented 10% jitter tolerance) lands on the injected `InstantTimeProvider` rather than the system clock. The `VideoUploadTests.UploadVideoAsync_BacksOffExponentially` rework is a clean simplification: it now goes through `client.Bsky.Video` instead of hand-assembling `XrpcClient` + `VideoClient`, which is possible precisely because the fix now plumbs the clock through the public-facing constructor path. Moving the private `ManualTime`/`InstantTimeProvider` clock to `TestSupport/InstantTimeProvider.cs` for reuse is a reasonable, minimal refactor with no behavior change (the class body is copied verbatim). The CHANGELOG entry is correctly placed under `Fixed` in `[Unreleased]`, references #187, and matches the required bullet style. No correctness or security issues found; this is a good, tightly-scoped fix. --- ⏱ 0m 49s · 12 turns · tokens: 25.9k in / 3.6k out (+533.0k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-188/run-643.md)

Grandiras merged commit a2ac148258 into main

2026-09-27 20:58:03 +00:00

Grandiras deleted branch issue-187-client-clock

2026-09-27 20:58:03 +00:00

Grandiras referenced this pull request from a commit

2026-09-27 20:58:03 +00:00

fix: pass AtProtoClient's clock to its XRPC transport (#188)