Grandiras/ATProto.NET

Fork 0

feat(auth)!: unified session model and a reliable session lifecycle #158

Merged

Grandiras merged 1 commit from issue-124-session into main

2026-09-25 20:28:28 +00:00

Grandiras commented

2026-09-24 23:44:01 +00:00

Owner

Closes #124.

One session model, one persistence abstraction, and a session lifecycle that refreshes, persists and signs out correctly, including under concurrency.

Model

abstract record AtProtoSession (Did, Handle, ServiceEndpoint, ExpiresAt) with PasswordSession (access/refresh JWT, email, status) and OAuthSession (DPoP-bound tokens, DPoPKey as PKCS#8 bytes, Issuer, TokenEndpoint, RevocationEndpoint, Scope). Immutable; ExpiresAt replaces ExpiresIn + obtained-at. Serializes as the base type with a $kind discriminator; ToString() never prints tokens.
AtProtoClient.Session is the single property (OAuthSession property gone), plus event SessionChanged (Created / Refreshed / Expired / Removed, with previous session and, for Expired, the error).
IAtProtoSessionStore (GetAsync(Did), SetAsync(session), RemoveAsync(Did)) replaces ISessionStore (never read by the SDK) and IAtProtoTokenStore. Implementations: InMemoryAtProtoSessionStore (moved to core), FileAtProtoSessionStore, EfCoreAtProtoSessionStore<T>. The client writes every session it installs or refreshes and removes it on sign-out/expiry. The three session↔token-data mappings (debloat 9.14) are gone.
EF Core: no schema change. Same AtProtoTokens table and AtProtoTokenEntity; the encrypted JSON blob now holds the session. The file and EF stores read rows/files the 0.6 token stores wrote (pinned by tests that serialize the exact 0.6 AtProtoTokenData shape with the 0.6 options), so upgraded deployments stay signed in.
Session API on AtProtoClient: LoginAsync (now with allowTakendown, the Lexicon flag #123 added to the internal request, also on ServerClient.CreateSessionAsync; sent only when true), CreateAccountAndLoginAsync (new), ApplySessionAsync(session, oauthClient?) (replaces ApplyOAuthSessionAsync), ResumeSessionAsync(session, oauthClient?) (validates with getSession), TryRestoreSessionAsync(did, oauthClient?) (new; reads the store), RefreshSessionAsync, LogoutAsync.
OAuthClient: CompleteAuthorizationAsync returns OAuthSession; RefreshAsync(session) replaces RefreshTokensAsync; new RevokeAsync(session).

Refresh (the in-pipeline "auth handler")

Implemented inside XrpcClient's pipeline rather than as a DelegatingHandler (the SDK never owns the HttpClient's handler chain): an internal IXrpcSessionHandler that AtProtoClient's internal SessionManager implements.

Proactive: before a session-authenticated request, refresh when the access token expires within 60 s. Password-session expiry is decoded from the access JWT's exp (no more fixed 115 min); OAuth from expires_in. A transient proactive failure still sends the request while the token is valid.
Reactive: on ExpiredToken (400/401) or a 401 with WWW-Authenticate: … error="invalid_token" (RFC 6750/9449), refresh and resend once.
Single-flight + generation check: the published credentials instance is the session's generation. Concurrent callers that saw the same credentials share one refresh task; a caller whose credentials were refreshed while it waited retries with the new ones, and only if they belong to the same account on the same service. A started exchange ends only on its own 30 s limit, never on the caller's token or disposal, and its result is always stored.
Refresh failure: a refused refresh token (XrpcAuthenticationException from refreshSession, OAuth invalid_grant) ends the session: removed locally and from the store, SessionChanged(Expired), error rethrown. Transient failures keep the session.
Background timer is optional: AtProtoClientOptions.BackgroundRefresh / WithBackgroundRefresh (default off). AutoRefreshSession now means on-demand refresh.
Server per-request clients (AtProtoClientFactory) now refresh on demand and write rotated tokens back to the store (previously they never refreshed). The per-DID cross-client coordinator remains #136.

Bugs fixed

Logout revokes nothing: deleteSession now gets the refresh JWT; an OAuth session is revoked at the AS revocation_endpoint (RFC 7009, DPoP proof, use_dpop_nonce retry; endpoint recorded at authorization, discovered from AS metadata otherwise). Teardown is local-first (memory + store), then remote; failures are rethrown afterwards. A token the PDS already considers invalid is not a failure. Blazor /atproto/logout revokes too.
Failed OAuth refresh stored as a token: refresh/revoke go through one PostWithDPoPAsync that checks the status, parses the OAuth error (bounded read), retries once on use_dpop_nonce, remembers AS nonces per origin, and disposes responses. A 2xx without an access token (or non-DPoP token type) is rejected.
ResumeSessionAsync on a fresh client: installs first, then getSession runs through the pipeline, so an expired token is refreshed and retried.
Password login after OAuth: installing any session replaces the whole state (tokens, DPoP key, refresher) under the transition lock; all installs/refreshes/logouts are serialized.
Stateless sub-clients: ServerClient no longer calls SetTokens/ClearTokens; RefreshSessionAsync(refreshJwt) / DeleteSessionAsync(refreshJwt) take the token; createSession/createAccount are sent without the installed session's credentials. PdsAdminClient sets its admin session explicitly.
Lifetime: DisposeAsync is primary (awaits an in-flight exchange and its store write); sync Dispose returns at once, lets the exchange land in the background and releases the DPoP key afterwards (was up to 35 s blocking). Ownership: the client disposes only what it creates; sessions are values, and the client builds/disposes its own DPoPProofGenerator from DPoPKey. Thread-safety and ownership contract written up in docs/session-management.md.

Breaking changes (migration)

Session, OAuthSessionResult, AtProtoTokenData → AtProtoSession / PasswordSession / OAuthSession. Pattern-match client.Session; unverified handle is handle.invalid; DidDoc dropped (use ServiceEndpoint).
ApplyOAuthSessionAsync(result, oauth, store) → ApplySessionAsync(session, oauth) on a client built WithSessionStore(store); ResumeSessionAsync takes AtProtoSession (+ optional OAuthClient) and returns the session.
RefreshTokensAsync → OAuthClient.RefreshAsync(session) (returns a new session); no using on the CompleteAuthorizationAsync result.
ISessionStore / IAtProtoTokenStore → IAtProtoSessionStore; store classes renamed (InMemoryAtProtoSessionStore in core, FileAtProtoSessionStore + FileSessionStoreOptions, EfCoreAtProtoSessionStore<T> + AddAtProtoEfCoreSessionStore<T>()); AtProtoClient ctor, WithSessionStore, AddAtProto<T>, AddAtProtoServer<T>, AtProtoClientFactory take the new interface. No EF migration needed.
ServerClient.RefreshSessionAsync(refreshJwt) / DeleteSessionAsync(refreshJwt); CreateSessionAsync/CreateAccountAsync no longer install tokens → use LoginAsync / CreateAccountAndLoginAsync / LogoutAsync.
AutoRefreshSession = false now disables all self-refresh (was: no timer); the timer is BackgroundRefresh.
LogoutAsync throws on service-side failure (after local teardown).
Blazor ClaimsFactory is Func<OAuthSession, IEnumerable<Claim>>.

Verification

Rebased onto 3fb7938 (#119 parts 2–4 and #123's wire-fidelity drift test merged). Full solution build with --no-incremental: 0 warnings, 0 errors.
Unit tests: 3272 passed, 0 failed, 5 skipped (the 5 are the env-gated Aspire manifest tests), including the #123 drift and convention tests in tests/ATProtoNet.Tests/Lexicon/Upstream/ (37 pass). The drift test's call scanner now recognizes ProcedureWithTokenAsync (it still named the removed ProcedureWithRefreshTokenAsync), so createSession, refreshSession, deleteSession and createAccount are checked against upstream as POSTs; a misspelt NSID there fails it.
New tests (HTTP stub = PDS + AS in one handler, recording every request):
- PasswordSessionLifecycleTests (35 cases): login (JWT exp → ExpiresAt, no auth header on createSession), entryway → didDoc PDS and the three cases that must not be followed (foreign DID doc, HTTP dev PDS, HTTPS→HTTP downgrade), migration on refresh, CreateAccountAndLoginAsync, stateless ServerClient (×3), resume on a fresh client with an expired token, resume updating account details, TryRestoreSessionAsync, ExpiredToken → refresh → retry, 16 concurrent rejected callers → exactly one refreshSession, 8 concurrent RefreshSessionAsync → one refresh, proactive refresh (and not when far from expiry, and transient-failure fallback), refresh token rejected → Expired + store cleared, transient failure keeps session, auto-refresh off, background timer via a manual TimeProvider, logout with refresh JWT / failure surfaced after teardown / already-invalid token, throwing event handler, Dispose with an in-flight refresh returns in < 2 s, DisposeAsync cancels and awaits, disposal leaves supplied HttpClient/store usable.
- OAuthSessionLifecycleTests (20): DPoP-bound requests, refresh form + DPoP proof (htu, no ath), invalid_grant → session expired, nothing stored, 5xx error body never taken for tokens, 2xx without access token rejected, use_dpop_nonce answered once and nonce remembered, invalid_token challenge → refresh → retry, 12 concurrent invalid_token callers → one token request, proactive OAuth refresh, missing OAuthClient, re-apply keeps previous OAuthClient, password login after OAuth refreshes via refreshSession, malformed DPoP key leaves client untouched, RFC 7009 revocation (refresh token + hint + client_id + DPoP), revocation endpoint discovery, access-token revocation without refresh token, revocation failure surfaced after teardown, no deleteSession for OAuth, disposal leaves OAuthClient usable, OAuthClient.RefreshAsync immutability.
- Session JSON round trips, legacy 0.6 token-data reading (verified and unverified handle), garbage rejection; in-memory/file/EF store tests including reading 0.6-written files/rows and the unchanged EF table; factory refresh-and-write-back.
Mutation check: disabling the join/generation check makes the three concurrency tests fail (1 refresh expected, 16/12/8 observed).
Integration tests (gated, not run here): added Logout_RevokesTheRefreshToken and RefreshSession_RotatesTheTokensAndKnowsTheirExpiry; the suite compiles.

Review follow-up

An adversarial review reproduced one blocking and three should-fix defects with 7 probes; each is now a regression test (mostly in tests/ATProtoNet.Tests/Auth/SessionRaceTests.cs), and each fix was mutation-checked (reverting it makes its test fail).

Blocking: a rejected call could be resent with another account's token to the old PDS. TryRecoverAsync now returns the credentials to resend with, and only credentials of the same account on the same service (XrpcCredentials carries Account and Service). XrpcClient publishes the service URL and credentials as one snapshot (SetSession), a call reads it once after the proactive refresh, and every attempt (DPoP nonce retry, 429 wait) uses that snapshot; only a same-account recovery replaces its credentials. Session state is published before the transport snapshot, so a call is always compared against the right generation. Tests: ACallRejectedAfterAnAccountSwitch_IsNotResentWithTheOtherAccountsToken (P2), ACallWaitingOutA429_IsResentToItsOwnServiceWithItsOwnToken_AfterASwitch, AProactiveRefreshThatMovesThePds_SendsTheCallToTheNewPds.
Should-fix: dispose / the refresh timeout cancelled a refresh partway, including its store write. Disposal now cancels only the wait for the session lock. A started exchange ends only on its own 30 s limit (reported as TimeoutException only then), and the refreshed session is persisted with a non-cancellable write. DisposeAsync waits for it; Dispose lets it land in the background and releases the DPoP key afterwards. Tests: ARefreshWhoseCallerGivesUpAndWhoseClientIsDisposed_StillStoresTheRotatedToken (P1), DisposingDuringTheStoreWrite_LetsTheWriteFinish (P6), and the two rewritten disposal tests in PasswordSessionLifecycleTests.
Should-fix: invalid_grant removed the store entry unconditionally. Expiry re-reads the store and removes the entry only if it still holds the refused refresh token, so a losing per-request client no longer deletes the winner's fresh session (full coordination stays with #136). Test: ALosingConcurrentRefresh_LeavesTheWinnersStoredSessionAlone (P7).
Should-fix: the background timer was not re-armed after a transient failure. It is rescheduled with backoff (30 s, doubling to 10 min) while the session is unchanged. Test: ABackgroundRefreshThatFailsTransiently_IsRetriedWithBackoff (P3).
Nit: ResumeSessionAsync doc vs behaviour (P4). A session the service rejects during resume (XrpcAuthenticationException, after a refresh where one applied) is now removed from the client and the store and reported Expired; other failures leave it installed. Doc updated. Tests: ResumeSessionAsync_ATokenTheServiceRejects_RemovesTheSession, ResumeSessionAsync_ServiceUnreachable_KeepsTheSession.
Nit: cancelled store writes left state changed with no event (P5). Every store write that follows a session change (install, account update, expiry, sign-out removal) is non-cancellable; the caller's token only cancels the wait for the lock. Test: ApplySessionAsync_CallerCancelsDuringTheStoreWrite_TheWriteAndTheEventStillHappen.
Nit: a released DPoP key surfaced as ObjectDisposedException. A call whose session was replaced or signed out while in flight now fails with XrpcAuthenticationException (AuthenticationRequired) and sends nothing. Test: ACallOfASessionReplacedMeanwhile_FailsInsteadOfSigningWithTheReleasedKey.
Nit: a non-seekable upload rejected as expired now triggers the refresh (so the next call works) but is not resent. Test: ANonSeekableUploadRejectedAsExpired_RefreshesButIsNotResent.
Nit: OAuth sign-out without an OAuthClient now throws InvalidOperationException after the local teardown instead of only logging. Test: LogoutAsync_AnOAuthSessionWithoutItsOAuthClient_TearsDownThenThrows.
Nit: token-printing ToString(). XrpcCredentials and the session state record override it (account, service, DPoP flag only); the unused XrpcClient.Credentials is removed. Test: Credentials_ToString_NeverShowsTheTokens.
Nit: the private _session alias in AtProtoClient.cs is inlined as _sessions.Session.
Optional: invalid_token with a DPoP-Nonce no longer spends the nonce retry first; the session recovery resends with the new nonce (2 requests instead of 3). Test: AnInvalidTokenChallengeCarryingANewNonce_IsRecoveredWithoutASeparateNonceRetry.
Also from the probe folder (JsonProbes.cs): malformed stored sessions (unknown $kind, invalid DID, relative endpoint, non-base64 DPoP key, legacy or current) now always throw JsonException, which the file and EF stores treat as a corrupt entry; added to StoredJson_Garbage_ThrowsJsonException.

Deliberately deferred / notes

2.7 identity-resolver PDS lookup before LoginAsync is left to #120. This PR only adopts the didDoc PDS that createSession/refreshSession return, in one seam: SessionManager.ResolveServiceEndpoint(didDoc, did, current). It moves only between HTTPS services, because a dev PDS publishes an address (http://localhost:3000, https://pds.test) that is usually not how the client reaches it (CI reaches it at http://pds:3000).
Cross-client coordination (per-DID lock re-reading the store) is #136; in-client single-flight is here. Documented as a limitation.
ServiceAuthGenerator ownsKey stays with #119 part 4 (file untouched).
#132 owns the rest of the OAuth hardening (unifying PAR/token exchange onto the same form helper, a process-wide nonce cache, refresh-response sub/scope validation).
Behaviour choice to review: a store write that fails during refresh is logged and the refreshed session stays installed (the 0.6 code kept the dead token in memory instead).

🤖 Generated with Claude Code

Closes #124. One session model, one persistence abstraction, and a session lifecycle that refreshes, persists and signs out correctly, including under concurrency. ## Model - **`abstract record AtProtoSession`** (`Did`, `Handle`, `ServiceEndpoint`, `ExpiresAt`) with **`PasswordSession`** (access/refresh JWT, email, status) and **`OAuthSession`** (DPoP-bound tokens, `DPoPKey` as PKCS#8 bytes, `Issuer`, `TokenEndpoint`, `RevocationEndpoint`, `Scope`). Immutable; `ExpiresAt` replaces `ExpiresIn` + obtained-at. Serializes as the base type with a `$kind` discriminator; `ToString()` never prints tokens. - **`AtProtoClient.Session`** is the single property (`OAuthSession` property gone), plus **`event SessionChanged`** (`Created` / `Refreshed` / `Expired` / `Removed`, with previous session and, for `Expired`, the error). - **`IAtProtoSessionStore`** (`GetAsync(Did)`, `SetAsync(session)`, `RemoveAsync(Did)`) replaces `ISessionStore` (never read by the SDK) and `IAtProtoTokenStore`. Implementations: `InMemoryAtProtoSessionStore` (moved to core), `FileAtProtoSessionStore`, `EfCoreAtProtoSessionStore<T>`. The client writes every session it installs or refreshes and removes it on sign-out/expiry. The three session↔token-data mappings (debloat 9.14) are gone. - **EF Core: no schema change.** Same `AtProtoTokens` table and `AtProtoTokenEntity`; the encrypted JSON blob now holds the session. The file and EF stores read rows/files the 0.6 token stores wrote (pinned by tests that serialize the exact 0.6 `AtProtoTokenData` shape with the 0.6 options), so upgraded deployments stay signed in. - Session API on `AtProtoClient`: `LoginAsync` (now with `allowTakendown`, the Lexicon flag #123 added to the internal request, also on `ServerClient.CreateSessionAsync`; sent only when true), `CreateAccountAndLoginAsync` (new), `ApplySessionAsync(session, oauthClient?)` (replaces `ApplyOAuthSessionAsync`), `ResumeSessionAsync(session, oauthClient?)` (validates with `getSession`), `TryRestoreSessionAsync(did, oauthClient?)` (new; reads the store), `RefreshSessionAsync`, `LogoutAsync`. - `OAuthClient`: `CompleteAuthorizationAsync` returns `OAuthSession`; `RefreshAsync(session)` replaces `RefreshTokensAsync`; new `RevokeAsync(session)`. ## Refresh (the in-pipeline "auth handler") Implemented inside `XrpcClient`'s pipeline rather than as a `DelegatingHandler` (the SDK never owns the `HttpClient`'s handler chain): an internal `IXrpcSessionHandler` that `AtProtoClient`'s internal `SessionManager` implements. - **Proactive:** before a session-authenticated request, refresh when the access token expires within 60 s. Password-session expiry is decoded from the access JWT's `exp` (no more fixed 115 min); OAuth from `expires_in`. A transient proactive failure still sends the request while the token is valid. - **Reactive:** on `ExpiredToken` (400/401) or a 401 with `WWW-Authenticate: … error="invalid_token"` (RFC 6750/9449), refresh and resend **once**. - **Single-flight + generation check:** the published credentials instance is the session's generation. Concurrent callers that saw the same credentials share one refresh task; a caller whose credentials were refreshed while it waited retries with the new ones, and only if they belong to the same account on the same service. A started exchange ends only on its own 30 s limit, never on the caller's token or disposal, and its result is always stored. - **Refresh failure:** a refused refresh token (`XrpcAuthenticationException` from `refreshSession`, OAuth `invalid_grant`) ends the session: removed locally and from the store, `SessionChanged(Expired)`, error rethrown. Transient failures keep the session. - **Background timer is optional:** `AtProtoClientOptions.BackgroundRefresh` / `WithBackgroundRefresh` (default off). `AutoRefreshSession` now means on-demand refresh. - Server per-request clients (`AtProtoClientFactory`) now refresh on demand and write rotated tokens back to the store (previously they never refreshed). The per-DID cross-client coordinator remains #136. ## Bugs fixed - **Logout revokes nothing:** `deleteSession` now gets the **refresh** JWT; an OAuth session is revoked at the AS `revocation_endpoint` (RFC 7009, DPoP proof, `use_dpop_nonce` retry; endpoint recorded at authorization, discovered from AS metadata otherwise). Teardown is local-first (memory + store), then remote; failures are rethrown afterwards. A token the PDS already considers invalid is not a failure. Blazor `/atproto/logout` revokes too. - **Failed OAuth refresh stored as a token:** refresh/revoke go through one `PostWithDPoPAsync` that checks the status, parses the OAuth error (bounded read), retries once on `use_dpop_nonce`, remembers AS nonces per origin, and disposes responses. A 2xx without an access token (or non-DPoP token type) is rejected. - **`ResumeSessionAsync` on a fresh client:** installs first, then `getSession` runs through the pipeline, so an expired token is refreshed and retried. - **Password login after OAuth:** installing any session replaces the whole state (tokens, DPoP key, refresher) under the transition lock; all installs/refreshes/logouts are serialized. - **Stateless sub-clients:** `ServerClient` no longer calls `SetTokens`/`ClearTokens`; `RefreshSessionAsync(refreshJwt)` / `DeleteSessionAsync(refreshJwt)` take the token; `createSession`/`createAccount` are sent without the installed session's credentials. `PdsAdminClient` sets its admin session explicitly. - **Lifetime:** `DisposeAsync` is primary (awaits an in-flight exchange and its store write); sync `Dispose` returns at once, lets the exchange land in the background and releases the DPoP key afterwards (was up to 35 s blocking). Ownership: the client disposes only what it creates; sessions are values, and the client builds/disposes its own `DPoPProofGenerator` from `DPoPKey`. Thread-safety and ownership contract written up in `docs/session-management.md`. ## Breaking changes (migration) - `Session`, `OAuthSessionResult`, `AtProtoTokenData` → `AtProtoSession` / `PasswordSession` / `OAuthSession`. Pattern-match `client.Session`; unverified handle is `handle.invalid`; `DidDoc` dropped (use `ServiceEndpoint`). - `ApplyOAuthSessionAsync(result, oauth, store)` → `ApplySessionAsync(session, oauth)` on a client built `WithSessionStore(store)`; `ResumeSessionAsync` takes `AtProtoSession` (+ optional `OAuthClient`) and returns the session. - `RefreshTokensAsync` → `OAuthClient.RefreshAsync(session)` (returns a new session); no `using` on the `CompleteAuthorizationAsync` result. - `ISessionStore` / `IAtProtoTokenStore` → `IAtProtoSessionStore`; store classes renamed (`InMemoryAtProtoSessionStore` in core, `FileAtProtoSessionStore` + `FileSessionStoreOptions`, `EfCoreAtProtoSessionStore<T>` + `AddAtProtoEfCoreSessionStore<T>()`); `AtProtoClient` ctor, `WithSessionStore`, `AddAtProto<T>`, `AddAtProtoServer<T>`, `AtProtoClientFactory` take the new interface. **No EF migration needed.** - `ServerClient.RefreshSessionAsync(refreshJwt)` / `DeleteSessionAsync(refreshJwt)`; `CreateSessionAsync`/`CreateAccountAsync` no longer install tokens → use `LoginAsync` / `CreateAccountAndLoginAsync` / `LogoutAsync`. - `AutoRefreshSession = false` now disables all self-refresh (was: no timer); the timer is `BackgroundRefresh`. - `LogoutAsync` throws on service-side failure (after local teardown). - Blazor `ClaimsFactory` is `Func<OAuthSession, IEnumerable<Claim>>`. ## Verification - Rebased onto `3fb7938` (#119 parts 2–4 and #123's wire-fidelity drift test merged). Full solution build with `--no-incremental`: **0 warnings, 0 errors**. - Unit tests: **3272 passed, 0 failed, 5 skipped** (the 5 are the env-gated Aspire manifest tests), including the #123 drift and convention tests in `tests/ATProtoNet.Tests/Lexicon/Upstream/` (37 pass). The drift test's call scanner now recognizes `ProcedureWithTokenAsync` (it still named the removed `ProcedureWithRefreshTokenAsync`), so `createSession`, `refreshSession`, `deleteSession` and `createAccount` are checked against upstream as POSTs; a misspelt NSID there fails it. - New tests (HTTP stub = PDS + AS in one handler, recording every request): - `PasswordSessionLifecycleTests` (35 cases): login (JWT `exp` → `ExpiresAt`, no auth header on createSession), entryway → didDoc PDS and the three cases that must not be followed (foreign DID doc, HTTP dev PDS, HTTPS→HTTP downgrade), migration on refresh, `CreateAccountAndLoginAsync`, stateless `ServerClient` (×3), resume on a fresh client with an expired token, resume updating account details, `TryRestoreSessionAsync`, `ExpiredToken` → refresh → retry, **16 concurrent rejected callers → exactly one `refreshSession`**, 8 concurrent `RefreshSessionAsync` → one refresh, proactive refresh (and not when far from expiry, and transient-failure fallback), refresh token rejected → `Expired` + store cleared, transient failure keeps session, auto-refresh off, background timer via a manual `TimeProvider`, logout with refresh JWT / failure surfaced after teardown / already-invalid token, throwing event handler, `Dispose` with an in-flight refresh returns in < 2 s, `DisposeAsync` cancels and awaits, disposal leaves supplied `HttpClient`/store usable. - `OAuthSessionLifecycleTests` (20): DPoP-bound requests, refresh form + DPoP proof (`htu`, no `ath`), **`invalid_grant` → session expired, nothing stored**, 5xx error body never taken for tokens, 2xx without access token rejected, `use_dpop_nonce` answered once and nonce remembered, `invalid_token` challenge → refresh → retry, **12 concurrent `invalid_token` callers → one token request**, proactive OAuth refresh, missing `OAuthClient`, re-apply keeps previous `OAuthClient`, password login after OAuth refreshes via `refreshSession`, malformed DPoP key leaves client untouched, RFC 7009 revocation (refresh token + hint + client_id + DPoP), revocation endpoint discovery, access-token revocation without refresh token, revocation failure surfaced after teardown, no `deleteSession` for OAuth, disposal leaves `OAuthClient` usable, `OAuthClient.RefreshAsync` immutability. - Session JSON round trips, legacy 0.6 token-data reading (verified and unverified handle), garbage rejection; in-memory/file/EF store tests including reading 0.6-written files/rows and the unchanged EF table; factory refresh-and-write-back. - Mutation check: disabling the join/generation check makes the three concurrency tests fail (1 refresh expected, 16/12/8 observed). - Integration tests (gated, not run here): added `Logout_RevokesTheRefreshToken` and `RefreshSession_RotatesTheTokensAndKnowsTheirExpiry`; the suite compiles. ## Review follow-up An adversarial review reproduced one blocking and three should-fix defects with 7 probes; each is now a regression test (mostly in `tests/ATProtoNet.Tests/Auth/SessionRaceTests.cs`), and each fix was mutation-checked (reverting it makes its test fail). - **Blocking: a rejected call could be resent with another account's token to the old PDS.** `TryRecoverAsync` now returns the credentials to resend with, and only credentials of the *same account on the same service* (`XrpcCredentials` carries `Account` and `Service`). `XrpcClient` publishes the service URL and credentials as one snapshot (`SetSession`), a call reads it once after the proactive refresh, and every attempt (DPoP nonce retry, 429 wait) uses that snapshot; only a same-account recovery replaces its credentials. Session state is published before the transport snapshot, so a call is always compared against the right generation. Tests: `ACallRejectedAfterAnAccountSwitch_IsNotResentWithTheOtherAccountsToken` (P2), `ACallWaitingOutA429_IsResentToItsOwnServiceWithItsOwnToken_AfterASwitch`, `AProactiveRefreshThatMovesThePds_SendsTheCallToTheNewPds`. - **Should-fix: dispose / the refresh timeout cancelled a refresh partway, including its store write.** Disposal now cancels only the wait for the session lock. A started exchange ends only on its own 30 s limit (reported as `TimeoutException` only then), and the refreshed session is persisted with a non-cancellable write. `DisposeAsync` waits for it; `Dispose` lets it land in the background and releases the DPoP key afterwards. Tests: `ARefreshWhoseCallerGivesUpAndWhoseClientIsDisposed_StillStoresTheRotatedToken` (P1), `DisposingDuringTheStoreWrite_LetsTheWriteFinish` (P6), and the two rewritten disposal tests in `PasswordSessionLifecycleTests`. - **Should-fix: `invalid_grant` removed the store entry unconditionally.** Expiry re-reads the store and removes the entry only if it still holds the refused refresh token, so a losing per-request client no longer deletes the winner's fresh session (full coordination stays with #136). Test: `ALosingConcurrentRefresh_LeavesTheWinnersStoredSessionAlone` (P7). - **Should-fix: the background timer was not re-armed after a transient failure.** It is rescheduled with backoff (30 s, doubling to 10 min) while the session is unchanged. Test: `ABackgroundRefreshThatFailsTransiently_IsRetriedWithBackoff` (P3). - **Nit: `ResumeSessionAsync` doc vs behaviour (P4).** A session the service rejects during resume (`XrpcAuthenticationException`, after a refresh where one applied) is now removed from the client and the store and reported `Expired`; other failures leave it installed. Doc updated. Tests: `ResumeSessionAsync_ATokenTheServiceRejects_RemovesTheSession`, `ResumeSessionAsync_ServiceUnreachable_KeepsTheSession`. - **Nit: cancelled store writes left state changed with no event (P5).** Every store write that follows a session change (install, account update, expiry, sign-out removal) is non-cancellable; the caller's token only cancels the wait for the lock. Test: `ApplySessionAsync_CallerCancelsDuringTheStoreWrite_TheWriteAndTheEventStillHappen`. - **Nit: a released DPoP key surfaced as `ObjectDisposedException`.** A call whose session was replaced or signed out while in flight now fails with `XrpcAuthenticationException` (`AuthenticationRequired`) and sends nothing. Test: `ACallOfASessionReplacedMeanwhile_FailsInsteadOfSigningWithTheReleasedKey`. - **Nit: a non-seekable upload rejected as expired** now triggers the refresh (so the next call works) but is not resent. Test: `ANonSeekableUploadRejectedAsExpired_RefreshesButIsNotResent`. - **Nit: OAuth sign-out without an `OAuthClient`** now throws `InvalidOperationException` after the local teardown instead of only logging. Test: `LogoutAsync_AnOAuthSessionWithoutItsOAuthClient_TearsDownThenThrows`. - **Nit: token-printing `ToString()`.** `XrpcCredentials` and the session state record override it (account, service, DPoP flag only); the unused `XrpcClient.Credentials` is removed. Test: `Credentials_ToString_NeverShowsTheTokens`. - **Nit: the private `_session` alias** in `AtProtoClient.cs` is inlined as `_sessions.Session`. - **Optional: `invalid_token` with a `DPoP-Nonce`** no longer spends the nonce retry first; the session recovery resends with the new nonce (2 requests instead of 3). Test: `AnInvalidTokenChallengeCarryingANewNonce_IsRecoveredWithoutASeparateNonceRetry`. - **Also from the probe folder (`JsonProbes.cs`):** malformed stored sessions (unknown `$kind`, invalid DID, relative endpoint, non-base64 DPoP key, legacy or current) now always throw `JsonException`, which the file and EF stores treat as a corrupt entry; added to `StoredJson_Garbage_ThrowsJsonException`. ## Deliberately deferred / notes - **2.7 identity-resolver PDS lookup before `LoginAsync`** is left to #120. This PR only adopts the `didDoc` PDS that `createSession`/`refreshSession` return, in one seam: `SessionManager.ResolveServiceEndpoint(didDoc, did, current)`. It moves only between HTTPS services, because a dev PDS publishes an address (`http://localhost:3000`, `https://pds.test`) that is usually not how the client reaches it (CI reaches it at `http://pds:3000`). - **Cross-client coordination** (per-DID lock re-reading the store) is #136; in-client single-flight is here. Documented as a limitation. - **`ServiceAuthGenerator` `ownsKey`** stays with #119 part 4 (file untouched). - #132 owns the rest of the OAuth hardening (unifying PAR/token exchange onto the same form helper, a process-wide nonce cache, refresh-response `sub`/scope validation). - Behaviour choice to review: a store write that fails during refresh is logged and the refreshed session stays installed (the 0.6 code kept the dead token in memory instead). 🤖 Generated with [Claude Code](https://claude.com/claude-code)

Grandiras added 1 commit

2026-09-24 23:44:01 +00:00

feat(auth)!: unified session model and a reliable session lifecycle (closes #124 )

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

Details

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

Details

c8c5a544e7

Sessions had two shapes (Session, OAuthSessionResult) and two stores
(ISessionStore, never read; IAtProtoTokenStore), and the lifecycle around
them was broken in several ways: logout sent deleteSession the access JWT
and never revoked OAuth sessions, a failed OAuth refresh was installed as
an empty token, ResumeSessionAsync could not refresh on a fresh client,
password login after OAuth kept refreshing through OAuth, ServerClient
changed the client's tokens as a side effect, and only a timer refreshed,
so nothing recovered from ExpiredToken and concurrent callers could spend
a single-use refresh token twice.

- AtProtoSession (PasswordSession, OAuthSession): immutable records with
  ExpiresAt; AtProtoClient.Session plus a SessionChanged event.
- IAtProtoSessionStore replaces both stores; in-memory (core), file and EF
  Core implementations, which still read what 0.6 wrote. The EF schema is
  unchanged, so no migration.
- Refresh lives in the XRPC pipeline: proactive before exp (decoded from
  the access JWT), reactive once on ExpiredToken / DPoP invalid_token,
  single-flight with a credentials-generation check. The timer is opt-in.
- Logout tears down locally, then deleteSession with the refresh JWT or
  RFC 7009 revocation with DPoP, and surfaces failures afterwards.
- OAuthClient.RefreshAsync checks the response and returns a new session;
  RevokeAsync is new. ServerClient is stateless; CreateAccountAndLoginAsync
  and TryRestoreSessionAsync are added.
- DisposeAsync is primary and sync Dispose no longer blocks; the client
  disposes only what it created. Thread-safety and ownership are
  documented in docs/session-management.md.

Password login adopts the didDoc PDS createSession returns (HTTPS only);
resolving the PDS before login is left to #120.

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

opencode-bot commented

2026-09-24 23:44:03 +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-158/run-2026-09-24T23-44-02-856Z.md) — updating every ~5 minutes while the review runs.

opencode-bot commented

2026-09-24 23:44:05 +00:00

Collaborator

⏳ Held on the OpenCode Go window — the 5-hour window is at 2% (the bot holds at 95%).
This continues automatically after the window resets (2026-09-25T03:54:33.092Z) — nothing to re-trigger.

⏳ **Held on the OpenCode Go window** — the 5-hour window is at 2% (the bot holds at 95%). This continues automatically after the window resets (2026-09-25T03:54:33.092Z) — nothing to re-trigger.

Grandiras commented

2026-09-24 23:44:06 +00:00

Author

Owner

@claude /review — this is the core auth path. Please read these in full from the branch: SessionManager (install/refresh single-flight and generation check), the refresh path in XrpcClient's pipeline (proactive and reactive, one retry), OAuthClient's refresh status check and revocation, and AtProtoClient's dispose path.

@claude /review — this is the core auth path. Please read these in full from the branch: `SessionManager` (install/refresh single-flight and generation check), the refresh path in `XrpcClient`'s pipeline (proactive and reactive, one retry), `OAuthClient`'s refresh status check and revocation, and `AtProtoClient`'s dispose path.

claude-bot commented

2026-09-24 23:44:10 +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/597); 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-158/run-597.md) — updating every ~5 minutes while the task runs.

claude-reviewer reviewed

2026-09-24 23:45:36 +00:00

claude-reviewer left a comment

💬 Verdict: comment — notes only, nothing blocking.

This is a very large, well-documented refactor (unified session model, single-flight refresh, revised persistence). The portion of the diff actually shown is internally consistent with the PR description: the breaking-change/CHANGELOG entries line up with the code changes visible (session-store renames, ServerClient becoming stateless, PdsAdminClient now setting tokens explicitly, Blazor claims/logout using OAuthSession, EF Core/file stores reading legacy 0.6 payloads via AtProtoSessionJson, DI registrations making the session store optional). I did not find a clear correctness bug in what is visible.

Caveat: the diff is truncated before the core of the new lifecycle machinery (SessionManager, the IXrpcSessionHandler pipeline hookup inside XrpcClient, the single-flight/generation-check refresh logic, and the new test files) is shown. Those are exactly the pieces the PR's own risk analysis calls out (concurrency correctness, refresh-token single-use safety, disposal timing), so this review can't independently confirm those guarantees hold — it can only confirm that the parts shown (call sites, DI wiring, docs) are consistent with the described design. Given the PR's own verification section (mutation testing disabling the generation check, 16/12/8-concurrent-caller tests), this is probably fine, but a maintainer should make sure SessionManager.cs and the XrpcClient pipeline changes get eyes too if they haven't already been reviewed in a separate pass.

A couple of very minor things worth a look, not blockers:

AtProtoClient.cs adds a private property _session (underscore-prefixed, like a field) that just forwards to _sessions.Session, duplicating the public Session property one screen above it. Harmless, but the naming makes it easy to mistake for a backing field during future edits — might be worth inlining _sessions.Session at the (unshown) call sites instead of keeping this alias.
PdsAdminClient.cs now calls _adminXrpc.SetTokens(session.AccessJwt) with only the access token (no refresh token), consistent with ServerClient no longer auto-installing sessions; since the admin session is short-lived and admin auth is otherwise HTTP Basic, this looks intentional rather than an oversight, but worth a sanity check that nothing downstream expects the admin XRPC client to be able to refresh.

Overall: no findings confirmed severe enough to request changes based on what's visible; recommend the reviewer also look at SessionManager.cs / XrpcClient pipeline changes directly if that hasn't happened, since that's where the hardest concurrency guarantees live.

⏱ 1m 15s · 1 turns · tokens: 76.1k in / 7.1k out (+23.9k cache-read)
📜 Full session transcript

💬 **Verdict: comment** — notes only, nothing blocking. This is a very large, well-documented refactor (unified session model, single-flight refresh, revised persistence). The portion of the diff actually shown is internally consistent with the PR description: the breaking-change/CHANGELOG entries line up with the code changes visible (session-store renames, `ServerClient` becoming stateless, `PdsAdminClient` now setting tokens explicitly, Blazor claims/logout using `OAuthSession`, EF Core/file stores reading legacy 0.6 payloads via `AtProtoSessionJson`, DI registrations making the session store optional). I did not find a clear correctness bug in what is visible. Caveat: the diff is truncated before the core of the new lifecycle machinery (`SessionManager`, the `IXrpcSessionHandler` pipeline hookup inside `XrpcClient`, the single-flight/generation-check refresh logic, and the new test files) is shown. Those are exactly the pieces the PR's own risk analysis calls out (concurrency correctness, refresh-token single-use safety, disposal timing), so this review can't independently confirm those guarantees hold — it can only confirm that the parts shown (call sites, DI wiring, docs) are consistent with the described design. Given the PR's own verification section (mutation testing disabling the generation check, 16/12/8-concurrent-caller tests), this is probably fine, but a maintainer should make sure `SessionManager.cs` and the `XrpcClient` pipeline changes get eyes too if they haven't already been reviewed in a separate pass. A couple of very minor things worth a look, not blockers: - `AtProtoClient.cs` adds a private property `_session` (underscore-prefixed, like a field) that just forwards to `_sessions.Session`, duplicating the public `Session` property one screen above it. Harmless, but the naming makes it easy to mistake for a backing field during future edits — might be worth inlining `_sessions.Session` at the (unshown) call sites instead of keeping this alias. - `PdsAdminClient.cs` now calls `_adminXrpc.SetTokens(session.AccessJwt)` with only the access token (no refresh token), consistent with `ServerClient` no longer auto-installing sessions; since the admin session is short-lived and admin auth is otherwise HTTP Basic, this looks intentional rather than an oversight, but worth a sanity check that nothing downstream expects the admin XRPC client to be able to refresh. Overall: no findings confirmed severe enough to request changes based on what's visible; recommend the reviewer also look at `SessionManager.cs` / `XrpcClient` pipeline changes directly if that hasn't happened, since that's where the hardest concurrency guarantees live. --- ⏱ 1m 15s · 1 turns · tokens: 76.1k in / 7.1k out (+23.9k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-158/run-597.md)

opencode-bot commented

2026-09-25 00:16:10 +00:00

Collaborator

🤖 Resolving the merge conflicts — the reply will link the full session transcript.

📜 Live transcript — updating every ~5 minutes while the fix runs.

🤖 Resolving the merge conflicts — the reply will link the full session transcript.  📜 [Live transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-158/run-2026-09-25T00-16-10-044Z.md) — updating every ~5 minutes while the fix runs.

opencode-bot commented

2026-09-25 00:16:12 +00:00

Collaborator

Grandiras force-pushed issue-124-session from c8c5a544e7

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

Details

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

Details

to 03f98f0bed

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

Details

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

Details

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

Details

2026-09-25 00:19:34 +00:00

Compare

Grandiras commented

2026-09-25 00:19:34 +00:00

Author

Owner

Follow-up in 03f98f0, rebased onto main after #159.

Because the automated review only saw a truncated diff, I ran an independent adversarial review of the session core (SessionManager, the refresh path in XrpcClient, the OAuthClient refresh and revocation, and dispose). It reproduced 1 blocking and 3 should-fix defects with probe tests. All are fixed, and each now has a regression test. Reverting any of the fixes fails its test.

Blocking: a request rejected under one account could be retried with another account's token against the old PDS. Service URL and credentials are now swapped together, and each call pins them for all its attempts.
Dispose or the refresh timeout cancelled an in-flight refresh or its store write, which could leave a spent refresh token stored. An exchange that has started now finishes, and the refreshed session is always persisted.
After invalid_grant, a client removed a session that another client had just refreshed. The entry is now removed only if it still holds the refused token.
The background refresh timer was not re-armed after a transient failure. It now retries with backoff.

Plus the nits: the resume doc/behaviour, non-cancellable post-change store writes, tokens no longer shown by ToString, the _session alias inlined, and invalid_token combined with a DPoP nonce needing 2 requests instead of 3. LoginAsync(allowTakendown:) is new. See Review follow-up in the description.

@claude /review

Follow-up in `03f98f0`, rebased onto main after #159. Because the automated review only saw a truncated diff, I ran an **independent adversarial review** of the session core (`SessionManager`, the refresh path in `XrpcClient`, the `OAuthClient` refresh and revocation, and dispose). It reproduced 1 blocking and 3 should-fix defects with probe tests. All are fixed, and each now has a regression test. Reverting any of the fixes fails its test. 1. **Blocking:** a request rejected under one account could be retried with another account's token against the old PDS. Service URL and credentials are now swapped together, and each call pins them for all its attempts. 2. Dispose or the refresh timeout cancelled an in-flight refresh or its store write, which could leave a spent refresh token stored. An exchange that has started now finishes, and the refreshed session is always persisted. 3. After `invalid_grant`, a client removed a session that another client had just refreshed. The entry is now removed only if it still holds the refused token. 4. The background refresh timer was not re-armed after a transient failure. It now retries with backoff. Plus the nits: the resume doc/behaviour, non-cancellable post-change store writes, tokens no longer shown by `ToString`, the `_session` alias inlined, and `invalid_token` combined with a DPoP nonce needing 2 requests instead of 3. `LoginAsync(allowTakendown:)` is new. See *Review follow-up* in the description. @claude /review

claude-bot commented

2026-09-25 00:19:36 +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/599); 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-158/run-599.md) — updating every ~5 minutes while the task runs.

opencode-bot commented

2026-09-25 00:20:35 +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-158/run-2026-09-25T00-20-34-597Z.md) — updating every ~5 minutes while the review runs.

opencode-bot commented

2026-09-25 00:20:37 +00:00

Collaborator

opencode-bot commented

2026-09-25 03:55:03 +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-158/run-2026-09-25T03-55-03-102Z.md) — updating every ~5 minutes while the review runs.

opencode-bot commented

2026-09-25 03:55:06 +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-25T08:55:03.502Z) — 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-25T08:55:03.502Z) — nothing to re-trigger.

opencode-bot commented

2026-09-25 08:55:34 +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-158/run-2026-09-25T08-55-33-515Z.md) — updating every ~5 minutes while the review runs.

opencode-bot commented

2026-09-25 08:55:37 +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-25T13:55:34.014Z) — 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-25T13:55:34.014Z) — nothing to re-trigger.

opencode-bot commented

2026-09-25 13:56:04 +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-158/run-2026-09-25T13-56-04-021Z.md) — updating every ~5 minutes while the review runs.

opencode-bot commented

2026-09-25 13:56:07 +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-25T18:56:04.484Z) — 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-25T18:56:04.484Z) — nothing to re-trigger.

opencode-bot commented

2026-09-25 18:56:35 +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-158/run-2026-09-25T18-56-34-500Z.md) — updating every ~5 minutes while the review runs.

opencode-bot commented

2026-09-25 18:56:38 +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-25T23:56:34.971Z) — 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-25T23:56:34.971Z) — nothing to re-trigger.

Grandiras commented

2026-09-25 20:26:27 +00:00

Author

Owner

@claude /review — re-requesting; the previous request coincided with a usage-limit window. See the follow-up comment above for what changed since the last review.

claude-bot commented

2026-09-25 20:26:30 +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/600); 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-158/run-600.md) — updating every ~5 minutes while the task runs.

claude-reviewer approved these changes

2026-09-25 20:28:07 +00:00

claude-reviewer left a comment

✅ Verdict: approve — no blocking issues found.

Re-review after the adversarial-review follow-up commits. My previous pass flagged that the diff shown was truncated before SessionManager.cs and the XrpcClient pipeline hookup, which are exactly where the hardest concurrency guarantees live, so this time I fetched the full issue-124-session branch and read both files in full (not just the diff) rather than relying on the truncated PR diff text.

SessionManager.cs: the single-flight refresh (RefreshOperation keyed by the observed XrpcCredentials reference, joined under lock (_gate)), the generation check (ReferenceEquals(state.Credentials, rejected) / SameSession account+service comparison), the non-cancellable persist-after-transition pattern, the ExpireLockedAsync/ForgetAsync "only remove from store if it still holds the refused token" logic, and the Dispose/DisposeAsync split (background continuation disposing the DPoP key only after an in-flight exchange finishes) all match what the PR description claims, and I didn't find a race or ordering bug in the state machine. The _state = next (compare-state) vs _xrpc.SetSession(...) (send-state) two-step publish in Publish() has a theoretically nonzero instruction-reordering window, but it's two adjacent synchronous statements with no await between them, not a real exploitable race.

XrpcClient.cs: SendAsync reads _target into a local target once, after BeforeSendAsync has had a chance to proactively refresh, and every retry (DPoP nonce, 429, session recovery) reuses that same snapshot except for a session recovery, which is only allowed to replace Credentials with what TryRecoverAsync returns (already constrained to same-account/same-service by SessionManager.SameSession). The disposed-DPoP-key race is handled by catching ObjectDisposedException from ApplyAuthorization and turning it into XrpcAuthenticationException(AuthenticationRequired) rather than letting a raw ObjectDisposedException escape, matching the "nit" fix claimed in the PR description. The DPoP-nonce-vs-invalid-token disambiguation (HasInvalidTokenChallenge gating the plain nonce-retry branch) also matches the "no separate nonce retry when the challenge already carries a new nonce" optimization described.

I did not attempt to re-verify every one of the ~55 new/changed test names by running the suite, but the two core files that implement the concurrency contract are internally consistent, well-documented, and I found no correctness bug in them. Combined with the extensive breaking-change/CHANGELOG bookkeeping (which lines up with the code), this looks ready to merge.

⏱ 1m 24s · 15 turns · tokens: 127.2k in / 7.5k out (+1627.5k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. Re-review after the adversarial-review follow-up commits. My previous pass flagged that the diff shown was truncated before `SessionManager.cs` and the `XrpcClient` pipeline hookup, which are exactly where the hardest concurrency guarantees live, so this time I fetched the full `issue-124-session` branch and read both files in full (not just the diff) rather than relying on the truncated PR diff text. `SessionManager.cs`: the single-flight refresh (`RefreshOperation` keyed by the observed `XrpcCredentials` reference, joined under `lock (_gate)`), the generation check (`ReferenceEquals(state.Credentials, rejected)` / `SameSession` account+service comparison), the non-cancellable persist-after-transition pattern, the `ExpireLockedAsync`/`ForgetAsync` "only remove from store if it still holds the refused token" logic, and the `Dispose`/`DisposeAsync` split (background continuation disposing the DPoP key only after an in-flight exchange finishes) all match what the PR description claims, and I didn't find a race or ordering bug in the state machine. The `_state = next` (compare-state) vs `_xrpc.SetSession(...)` (send-state) two-step publish in `Publish()` has a theoretically nonzero instruction-reordering window, but it's two adjacent synchronous statements with no await between them, not a real exploitable race. `XrpcClient.cs`: `SendAsync` reads `_target` into a local `target` once, after `BeforeSendAsync` has had a chance to proactively refresh, and every retry (DPoP nonce, 429, session recovery) reuses that same snapshot except for a session recovery, which is only allowed to replace `Credentials` with what `TryRecoverAsync` returns (already constrained to same-account/same-service by `SessionManager.SameSession`). The disposed-DPoP-key race is handled by catching `ObjectDisposedException` from `ApplyAuthorization` and turning it into `XrpcAuthenticationException(AuthenticationRequired)` rather than letting a raw `ObjectDisposedException` escape, matching the "nit" fix claimed in the PR description. The DPoP-nonce-vs-invalid-token disambiguation (`HasInvalidTokenChallenge` gating the plain nonce-retry branch) also matches the "no separate nonce retry when the challenge already carries a new nonce" optimization described. I did not attempt to re-verify every one of the ~55 new/changed test names by running the suite, but the two core files that implement the concurrency contract are internally consistent, well-documented, and I found no correctness bug in them. Combined with the extensive breaking-change/CHANGELOG bookkeeping (which lines up with the code), this looks ready to merge. --- ⏱ 1m 24s · 15 turns · tokens: 127.2k in / 7.5k out (+1627.5k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-158/run-600.md)

Grandiras merged commit ca43404e53 into main

2026-09-25 20:28:28 +00:00

Grandiras deleted branch issue-124-session

2026-09-25 20:28:29 +00:00

Grandiras referenced this pull request from a commit

2026-09-25 20:28:29 +00:00

feat(auth)!: unified session model and a reliable session lifecycle (#158)

Grandiras referenced this pull request

2026-09-25 20:54:40 +00:00

feat(atproto): remaining com.atproto and site.standard gaps #161

Grandiras referenced this pull request

2026-09-25 20:58:53 +00:00