Grandiras/ATProto.NET
1
1
Fork 0
Code Issues Pull requests Projects Releases 9 Packages 8 Wiki Activity Actions

refactor!: debloat shared primitives: one verify helper, one cache, one bounded reader #185

Merged
Grandiras merged 15 commits from debloat-d-primitives into main 2026-09-27 17:55:02 +00:00
Conversation 11 Commits 15 Files changed 137 +3280 -4361
Grandiras commented 2026-09-27 17:13:21 +00:00
Owner
Copy link

Part of #180 (pass D: shared primitives).

One way to do each shared thing: verify a signature against a DID-published key, read a bounded body, cache, read optional JSON members, send an XRPC call. Behaviour is unchanged except where noted below; every regression, security and refresh test passes unchanged or retargeted.

What changed

V1 + B[7]: one verify-with-refresh path

  • DidResolverExtensions.VerifyWithRefreshAsync(did, selectKey, verify) (internal, Identity/) holds the refetch-once rule and its rationale, once:
    • refetch only after a missing key or a failed verification, and a passing one never refetches;
    • retry only if the refetch changed the key;
    • the refetch is IDidResolver.RefreshAsync, which CachingDidResolver rate-limits per DID;
    • the key selector never falls back, and throws to refuse a document with no refetch;
    • resolution failures propagate for each caller to map.
  • Callers keep their own rules:
    Caller Key rule Resolution failure
    RepoSyncVerifier GetSigningKey() (#atproto) "identity could not be resolved" result
    LabelVerifier #atproto_label via TryGetVerificationKey == Found IssuerUnresolved
    ServiceAuthVerifier the token's allowed kid BadJwtIss
    SpaceDidResolution.VerifyTokenAsync (credentials, delegation tokens) the kid fragment, absent or malformed refused at once; only a signature failure refetches NotAuthorized for any resolver exception, as before
    SpaceSyncer #atproto, refused at once when unusable propagates, as before
  • AtProtoCrypto.TryVerifySignature / TryVerifyJwtSignature (internal) replace the copied catch lists in RepoSyncVerifier, LabelSigning, ServiceAuthVerifier, JsonWebKeyVerifier and SpaceCommit.
  • New DidResolverExtensionsTests pin the rule on the helper. The callers' refresh tests are unchanged: rotated key verifies, forged key refused after exactly one refresh, valid token never refreshes, rate limit.

K1

DidDocument.GetVerificationKey / GetSigningKey return null for key material that does not decode, instead of throwing FormatException. The four catch (FormatException) wrappers go.

H1 + B[12]: one bounded reader

  • BoundedContent moves to Http/ and gains a Stream overload.
  • The copies in SyncClient, RepoFetchers, the Tap webhook and SpaceSyncer.CopyBoundedAsync go.
  • Each caller keeps its limit and its errors, and RepoFetchers keeps its IOException mapping.
  • A declared length pre-allocates at most 1 MiB. SpaceSyncer used 32 MiB, RepoFetchers 1 MiB; I took the tighter bound.

L1(core) + B[16] + L8: one LRU cache, one resolution-cache core

  • Internal Caching/LruCache<TKey,TValue> (thread-safe) replaces the hand-rolled bounded maps of:
    • CachingDidResolver, DidKeyCache and SpaceCredentialVerifier, whose per-authority quota stays on top (RemoveLeastRecent);
    • JsonWebKeyVerifier, SpaceClientAttestationVerifier, AtProtoClientFactory, the OAuth metadata cache and DPoPNonceCache.
  • Policy decisions:
    • These five previously cleared themselves when full, dropped arbitrary entries, or sorted out a quarter at a time. They now all evict the least recently used entry.
    • The metadata cache no longer purges expired entries at overflow. An expired entry is refetched when it is next read.
  • Internal Caching/ResolutionCache<TKey,TValue,TError> holds CachingDidResolver's logic:
    • stale-while-revalidate;
    • negative TTL;
    • single-flight fetches, detached by an invalidation;
    • RetryAfter after a failed refresh;
    • the rate-limited forced refresh;
    • the distributed tier with its pending-write undo.
  • CachingLexiconResolver runs on the same core, with differences only in:
    • Failures share its one capacity, as before (failureCapacity: null).
    • Shared fetches start through Task.Run, as the Lexicon cache's did, and now the DID cache's too (review fix 1).
    • A failed background refresh marks the entry used. Each resolver logs exactly what it logged before (review fix 5).
  • LexiconCacheOptions is kept rather than reusing DidCacheOptions. The defaults differ (1,000 entries and 5 minutes, against 10,000 and 1 hour), and reusing the type would change the public API for no gain.

J1, X1, N1, A1

  • J1. JsonElementExtensions (GetStringOrNull, new GetInt64OrNull / GetInt32OrNull / GetBooleanOrNull, and TryGetNumericDate) moves to Serialization/. The copies in these go:

    • the session JSON reader;
    • the Jetstream parser;
    • the chat unknown variants;
    • LexiconSchemaRecord.MainType;
    • the Tap models.

    The helpers also answer null for a non-object element, where the copies threw InvalidOperationException.

  • X1. QueryAsync, ProcedureAsync, UploadAsync and the output-less calls share one SendAsync<T> for the per-call deadline. BuildUri uses XrpcParams.ToQueryString.

  • N1. Identity service URLs go through AtProtoHttp.ValidateServiceUrl(…, allowLoopback: false), and IdentityNetworkPolicy.ValidateServiceUrl is removed. The repo fetchers' default client is IdentityNetworkPolicy.CreateClient(false, 5 min).

  • A1. AtProtoJsonDefaults.FormatTimestamp / NowTimestamp are removed.

C-Q3

The anonymous-object QueryAsync<TOut>(Nsid, object, …) overload and XrpcParams.From's reflection path are removed. The From_* formatting tests that still apply (DateTime, local time, identifier types) are retargeted to Add.

Pass A's leftover

SpaceRecordCbor catches through DagCborDecoder.IsMalformed.

Docs (Pass F's rule on this pass's files)

A separate, comment-only commit: plain-prose public summaries sit on one line, and docs on internal and private members become // comments with the XML stripped. A script checked the diff: 0 non-comment lines changed. The refresh-once rationale now lives only on the helper.

Breaking changes (CHANGELOG bullet + docs/migrating-to-0.7.md entry each)

Removed or changed Migration
AtProtoJsonDefaults.FormatTimestamp, NowTimestamp (shipped in 0.6) AtDatetime.FromDateTime(dt).ToString(), AtDatetime.Now().ToString()
DidDocument.GetSigningKey / GetVerificationKey no longer throw FormatException (shipped in 0.6) TryGetVerificationKey to tell malformed from absent
QueryAsync<TOut>(Nsid, object, …) (0.7-only; the 0.6 anonymous-object form was already a breaking bullet) build an XrpcParams. The #122 bullet and the migration entry are amended.

PublicAPI.Shipped.txt: 3 lines removed. Unshipped.txt is unchanged.

Other bullets:

  • Of the two Fixed bullets that named FormatTimestamp, one is reworded without the name, and the other, a fix to the removed method alone, is dropped.
  • Three Changed bullets were added: the one reader, the one refetch path, and LRU caches.

Behaviour changes worth knowing

  • SpaceCommitVerifier.Verify returns false instead of throwing for a key on a curve the platform lacks, which is the rule RepoSyncVerifier already had.
  • SpaceSyncer reports a missing or broken #atproto key as "publishes no usable AT Protocol signing key", one message instead of two.
  • IdentityNetworkPolicy's HTTPS check now runs after the query/fragment check, so a URL with both problems reports the query.

Verification

  • dotnet build -c Release: 0 warnings, 0 errors (full solution, doc snippets included).
  • dotnet test tests/ATProtoNet.Tests/ -c Release: 5003 passed, 0 failed, 5 skipped, run twice. The count on origin/main was 5000/0/5.
    • Removed: the XrpcParams.From and anonymous-object QueryAsync tests, and the timestamp tests that AtDatetimeTests already covers.
    • Added: tests for the refetch helper, the LRU cache, the stream reader and its sizing (moved from SpaceSyncerTests), K1, and Add for DateTime and identifiers.
  • Aspire manifest check: the manifest generated from samples/ManagedPdsSample.AppHost, then ATPROTO_REQUIRE_INTEGRATION=1 … --filter AspireManifest: 6 passed.
  • One test failed once in a full run on the shared machine. I did not capture its name, and it did not reproduce in more than ten later full runs.

Line counts (loc.py origin/main HEAD, origin/main = 199b7d1)

code xmldoc comments
src/ total 24977 → 24662 (−315, including +407 for the new Caching/) 12095 → 9703 (−2392; the refactor commits alone +52) 1775 → 2991 (+1216, internal docs as //)
src/ATProtoNet/Identity 2460 → 2230 1337 → 931 100 → 247
src/ATProtoNet/Http 1192 → 1136 738 → 323 40 → 323
src/ATProtoNet.Server/Spaces 2693 → 2661 1508 → 929 157 → 528
tests/ATProtoNet.Tests 48007 → 48063 (+56) 1476 → 1480 1842 → 1838

Review fixes

Seven commits on top of a03171b, rebased onto origin/main (d8a7404, #186). Each code fix has a regression test, and I checked that each test fails when run against the code before the fix.

  1. Shared resolutions started on the first caller's context (1518ff0).
    • ResolutionCache.StartFetch ran the fetch on the first caller's thread and SynchronizationContext. A resolver that awaits without ConfigureAwait(false) then stalled for every caller that joined it if that context stopped running.
    • It uses Task.Run now. CachingLexiconResolver did that on main; CachingDidResolver did not, so it gets a Fixed bullet.
    • Test: ResolutionCacheTests.GetAsync_FirstCallerOnAContextThatNeverRuns_DoesNotStallTheSharedFetch. It timed out without the fix.
  2. The Tap webhook sized its buffer from the sender's Content-Length (ccdf133).
    • Inbound bodies now pass declaredLength: null; outbound responses keep the hint.
    • Test: ReadBodyAsync_DeclaredLength_ReservesNothingBeforeTheBytesArrive. It measures the backing array: 1,048,577 bytes before the fix, 16 KiB after.
    • The CHANGELOG bullet is amended.
  3. UploadAsync<T> threw synchronously on bad arguments (fbc8da9). It awaits again, so an invalid argument faults the returned task, as on main.
    • Test: UploadAsync_InvalidArguments_FaultTheTaskRatherThanThrowing.
  4. The attestation key cache updated in two steps (ea3f5d9).
    • It read with TryPeek, then wrote with Set, so a fetch landing in between could be overwritten by the stale marker.
    • LruCache.AddOrUpdate does the update in one step under the cache's lock, as ConcurrentDictionary.AddOrUpdate did on main.
    • Test: a deterministic interleaving. A write started inside the update must land after it. With a two-step update, the write landed inside it.
  5. Log messages and properties (7c1005c).
    • The core no longer logs. Each resolver passes callbacks that log exactly what it logged on main:
      • {Did} and the "distributed DID document cache" warnings;
      • the Lexicon cache's Information line with {Nsid}.
    • CachingDidResolver no longer logs failed refreshes.
    • Tests: ResolverLoggingTests check the templates, the property names, and that CachingDidResolver logs nothing on a failed refresh.
  6. Coverage (de584b5).
    • SpaceTokenKeyIdTests: an absent or malformed #atproto_space key is refused with RefreshCount == 0.
    • RepoSyncVerifier: "publishes no usable atproto signing key" after exactly one refetch, for an absent key and for an undecodable one.
    • SpaceSyncer: refuses both of those without a refetch.
  7. Cache hits take no lock (16dd23f).
    • LruCache reads a ConcurrentDictionary without the lock. A hit moves its entry to the front only when the lock is free, so eviction is exact LRU single-threaded and approximate under contention.
    • Writes take the lock. A replaced value is a new node, so a lock-free reader never sees a torn struct.
    • Test: a lookup completes while a write holds the lock. It waited on the locked version.
    • Microbenchmark: 16 threads × 2M TryGetValue on a warm 1,024-entry cache, 12 logical CPUs, best of 5. The figures are aggregate wall-clock ns per get:
      ConcurrentDictionary (main) locked LruCache lock-free hits
      one hot key 1.3 34–40 1.5–1.6
      random of 1,024 keys 1.6–2.0 52 8.3–9.9
    • Single-threaded, a hit costs 15.6 ns against 8.1 for ConcurrentDictionary (one key), and 29 ns against 14 (random keys). DidKeyCache.Shared gains lock-free hits too; it was lock-based on main.

After the fixes:

  • Release build: 0 warnings.
  • Unit tests in Release: 5017 passed, 0 failed, 5 skipped, run twice.
  • Aspire manifest check: 6/6.

Items 3–5 and 7 restore main's behaviour, so they have no CHANGELOG entry.

Deferred

C-Q6 (PdsAdminClient through IXrpcSessionHandler) is not done, because it would change behaviour.

  • XrpcClient asks the session handler to recover only on ExpiredToken or a WWW-Authenticate: … invalid_token challenge.
  • PdsAdminClient signs in again on any 401. Tranquil answers a revoked or invalidated session with 401 AuthenticationFailed, which the handler path would no longer recover from.
  • The handler path would also start retrying through the raw Admin / Server clients.

Pass E's files are untouched. No Lexicon client test needed a call-site fix.

🤖 Generated with Claude Code

Part of #180 (pass D: shared primitives). One way to do each shared thing: verify a signature against a DID-published key, read a bounded body, cache, read optional JSON members, send an XRPC call. Behaviour is unchanged except where noted below; every regression, security and refresh test passes unchanged or retargeted. ## What changed ### V1 + B[7]: one verify-with-refresh path - `DidResolverExtensions.VerifyWithRefreshAsync(did, selectKey, verify)` (internal, `Identity/`) holds the refetch-once rule and its rationale, once: - refetch only after a missing key or a failed verification, and a passing one never refetches; - retry only if the refetch changed the key; - the refetch is `IDidResolver.RefreshAsync`, which `CachingDidResolver` rate-limits per DID; - the key selector never falls back, and throws to refuse a document with no refetch; - resolution failures propagate for each caller to map. - Callers keep their own rules: | Caller | Key rule | Resolution failure | |---|---|---| | `RepoSyncVerifier` | `GetSigningKey()` (`#atproto`) | "identity could not be resolved" result | | `LabelVerifier` | `#atproto_label` via `TryGetVerificationKey == Found` | `IssuerUnresolved` | | `ServiceAuthVerifier` | the token's allowed `kid` | `BadJwtIss` | | `SpaceDidResolution.VerifyTokenAsync` (credentials, delegation tokens) | the `kid` fragment, absent or malformed refused at once; only a signature failure refetches | `NotAuthorized` for any resolver exception, as before | | `SpaceSyncer` | `#atproto`, refused at once when unusable | propagates, as before | - `AtProtoCrypto.TryVerifySignature` / `TryVerifyJwtSignature` (internal) replace the copied catch lists in `RepoSyncVerifier`, `LabelSigning`, `ServiceAuthVerifier`, `JsonWebKeyVerifier` and `SpaceCommit`. - New `DidResolverExtensionsTests` pin the rule on the helper. The callers' refresh tests are unchanged: rotated key verifies, forged key refused after exactly one refresh, valid token never refreshes, rate limit. ### K1 `DidDocument.GetVerificationKey` / `GetSigningKey` return `null` for key material that does not decode, instead of throwing `FormatException`. The four `catch (FormatException)` wrappers go. ### H1 + B[12]: one bounded reader - `BoundedContent` moves to `Http/` and gains a `Stream` overload. - The copies in `SyncClient`, `RepoFetchers`, the Tap webhook and `SpaceSyncer.CopyBoundedAsync` go. - Each caller keeps its limit and its errors, and `RepoFetchers` keeps its `IOException` mapping. - A declared length pre-allocates at most 1 MiB. `SpaceSyncer` used 32 MiB, `RepoFetchers` 1 MiB; I took the tighter bound. ### L1(core) + B[16] + L8: one LRU cache, one resolution-cache core - Internal `Caching/LruCache<TKey,TValue>` (thread-safe) replaces the hand-rolled bounded maps of: - `CachingDidResolver`, `DidKeyCache` and `SpaceCredentialVerifier`, whose per-authority quota stays on top (`RemoveLeastRecent`); - `JsonWebKeyVerifier`, `SpaceClientAttestationVerifier`, `AtProtoClientFactory`, the OAuth metadata cache and `DPoPNonceCache`. - Policy decisions: - These five previously cleared themselves when full, dropped arbitrary entries, or sorted out a quarter at a time. They now all evict the least recently used entry. - The metadata cache no longer purges expired entries at overflow. An expired entry is refetched when it is next read. - Internal `Caching/ResolutionCache<TKey,TValue,TError>` holds `CachingDidResolver`'s logic: - stale-while-revalidate; - negative TTL; - single-flight fetches, detached by an invalidation; - `RetryAfter` after a failed refresh; - the rate-limited forced refresh; - the distributed tier with its pending-write undo. - `CachingLexiconResolver` runs on the same core, with differences only in: - Failures share its one capacity, as before (`failureCapacity: null`). - Shared fetches start through `Task.Run`, as the Lexicon cache's did, and now the DID cache's too (review fix 1). - A failed background refresh marks the entry used. Each resolver logs exactly what it logged before (review fix 5). - `LexiconCacheOptions` is kept rather than reusing `DidCacheOptions`. The defaults differ (1,000 entries and 5 minutes, against 10,000 and 1 hour), and reusing the type would change the public API for no gain. ### J1, X1, N1, A1 - **J1.** `JsonElementExtensions` (`GetStringOrNull`, new `GetInt64OrNull` / `GetInt32OrNull` / `GetBooleanOrNull`, and `TryGetNumericDate`) moves to `Serialization/`. The copies in these go: - the session JSON reader; - the Jetstream parser; - the chat unknown variants; - `LexiconSchemaRecord.MainType`; - the Tap models. The helpers also answer `null` for a non-object element, where the copies threw `InvalidOperationException`. - **X1.** `QueryAsync`, `ProcedureAsync`, `UploadAsync` and the output-less calls share one `SendAsync<T>` for the per-call deadline. `BuildUri` uses `XrpcParams.ToQueryString`. - **N1.** Identity service URLs go through `AtProtoHttp.ValidateServiceUrl(…, allowLoopback: false)`, and `IdentityNetworkPolicy.ValidateServiceUrl` is removed. The repo fetchers' default client is `IdentityNetworkPolicy.CreateClient(false, 5 min)`. - **A1.** `AtProtoJsonDefaults.FormatTimestamp` / `NowTimestamp` are removed. ### C-Q3 The anonymous-object `QueryAsync<TOut>(Nsid, object, …)` overload and `XrpcParams.From`'s reflection path are removed. The `From_*` formatting tests that still apply (`DateTime`, local time, identifier types) are retargeted to `Add`. ### Pass A's leftover `SpaceRecordCbor` catches through `DagCborDecoder.IsMalformed`. ### Docs (Pass F's rule on this pass's files) A separate, comment-only commit: plain-prose public summaries sit on one line, and docs on internal and private members become `//` comments with the XML stripped. A script checked the diff: 0 non-comment lines changed. The refresh-once rationale now lives only on the helper. ## Breaking changes (CHANGELOG bullet + `docs/migrating-to-0.7.md` entry each) | Removed or changed | Migration | |---|---| | `AtProtoJsonDefaults.FormatTimestamp`, `NowTimestamp` (shipped in 0.6) | `AtDatetime.FromDateTime(dt).ToString()`, `AtDatetime.Now().ToString()` | | `DidDocument.GetSigningKey` / `GetVerificationKey` no longer throw `FormatException` (shipped in 0.6) | `TryGetVerificationKey` to tell malformed from absent | | `QueryAsync<TOut>(Nsid, object, …)` (0.7-only; the 0.6 anonymous-object form was already a breaking bullet) | build an `XrpcParams`. The #122 bullet and the migration entry are amended. | `PublicAPI.Shipped.txt`: 3 lines removed. `Unshipped.txt` is unchanged. Other bullets: - Of the two *Fixed* bullets that named `FormatTimestamp`, one is reworded without the name, and the other, a fix to the removed method alone, is dropped. - Three *Changed* bullets were added: the one reader, the one refetch path, and LRU caches. ## Behaviour changes worth knowing - `SpaceCommitVerifier.Verify` returns `false` instead of throwing for a key on a curve the platform lacks, which is the rule `RepoSyncVerifier` already had. - `SpaceSyncer` reports a missing or broken `#atproto` key as "publishes no usable AT Protocol signing key", one message instead of two. - `IdentityNetworkPolicy`'s HTTPS check now runs after the query/fragment check, so a URL with both problems reports the query. ## Verification - `dotnet build -c Release`: 0 warnings, 0 errors (full solution, doc snippets included). - `dotnet test tests/ATProtoNet.Tests/ -c Release`: **5003 passed, 0 failed, 5 skipped**, run twice. The count on `origin/main` was 5000/0/5. - Removed: the `XrpcParams.From` and anonymous-object `QueryAsync` tests, and the timestamp tests that `AtDatetimeTests` already covers. - Added: tests for the refetch helper, the LRU cache, the stream reader and its sizing (moved from `SpaceSyncerTests`), K1, and `Add` for `DateTime` and identifiers. - Aspire manifest check: the manifest generated from `samples/ManagedPdsSample.AppHost`, then `ATPROTO_REQUIRE_INTEGRATION=1 … --filter AspireManifest`: 6 passed. - One test failed once in a full run on the shared machine. I did not capture its name, and it did not reproduce in more than ten later full runs. ## Line counts (`loc.py origin/main HEAD`, origin/main = 199b7d1) | | code | xmldoc | comments | |---|---|---|---| | `src/` total | 24977 → 24662 (**−315**, including +407 for the new `Caching/`) | 12095 → 9703 (**−2392**; the refactor commits alone +52) | 1775 → 2991 (+1216, internal docs as `//`) | | `src/ATProtoNet/Identity` | 2460 → 2230 | 1337 → 931 | 100 → 247 | | `src/ATProtoNet/Http` | 1192 → 1136 | 738 → 323 | 40 → 323 | | `src/ATProtoNet.Server/Spaces` | 2693 → 2661 | 1508 → 929 | 157 → 528 | | `tests/ATProtoNet.Tests` | 48007 → 48063 (+56) | 1476 → 1480 | 1842 → 1838 | ## Review fixes Seven commits on top of `a03171b`, rebased onto `origin/main` (`d8a7404`, #186). Each code fix has a regression test, and I checked that each test fails when run against the code before the fix. 1. **Shared resolutions started on the first caller's context** (`1518ff0`). - `ResolutionCache.StartFetch` ran the fetch on the first caller's thread and `SynchronizationContext`. A resolver that awaits without `ConfigureAwait(false)` then stalled for every caller that joined it if that context stopped running. - It uses `Task.Run` now. `CachingLexiconResolver` did that on main; `CachingDidResolver` did not, so it gets a *Fixed* bullet. - Test: `ResolutionCacheTests.GetAsync_FirstCallerOnAContextThatNeverRuns_DoesNotStallTheSharedFetch`. It timed out without the fix. 2. **The Tap webhook sized its buffer from the sender's `Content-Length`** (`ccdf133`). - Inbound bodies now pass `declaredLength: null`; outbound responses keep the hint. - Test: `ReadBodyAsync_DeclaredLength_ReservesNothingBeforeTheBytesArrive`. It measures the backing array: 1,048,577 bytes before the fix, 16 KiB after. - The CHANGELOG bullet is amended. 3. **`UploadAsync<T>` threw synchronously on bad arguments** (`fbc8da9`). It awaits again, so an invalid argument faults the returned task, as on main. - Test: `UploadAsync_InvalidArguments_FaultTheTaskRatherThanThrowing`. 4. **The attestation key cache updated in two steps** (`ea3f5d9`). - It read with `TryPeek`, then wrote with `Set`, so a fetch landing in between could be overwritten by the stale marker. - `LruCache.AddOrUpdate` does the update in one step under the cache's lock, as `ConcurrentDictionary.AddOrUpdate` did on main. - Test: a deterministic interleaving. A write started inside the update must land after it. With a two-step update, the write landed inside it. 5. **Log messages and properties** (`7c1005c`). - The core no longer logs. Each resolver passes callbacks that log exactly what it logged on main: - `{Did}` and the "distributed DID document cache" warnings; - the Lexicon cache's Information line with `{Nsid}`. - `CachingDidResolver` no longer logs failed refreshes. - Tests: `ResolverLoggingTests` check the templates, the property names, and that `CachingDidResolver` logs nothing on a failed refresh. 6. **Coverage** (`de584b5`). - `SpaceTokenKeyIdTests`: an absent or malformed `#atproto_space` key is refused with `RefreshCount == 0`. - `RepoSyncVerifier`: "publishes no usable atproto signing key" after exactly one refetch, for an absent key and for an undecodable one. - `SpaceSyncer`: refuses both of those without a refetch. 7. **Cache hits take no lock** (`16dd23f`). - `LruCache` reads a `ConcurrentDictionary` without the lock. A hit moves its entry to the front only when the lock is free, so eviction is exact LRU single-threaded and approximate under contention. - Writes take the lock. A replaced value is a new node, so a lock-free reader never sees a torn struct. - Test: a lookup completes while a write holds the lock. It waited on the locked version. - Microbenchmark: 16 threads × 2M `TryGetValue` on a warm 1,024-entry cache, 12 logical CPUs, best of 5. The figures are aggregate wall-clock ns per get: | | `ConcurrentDictionary` (main) | locked `LruCache` | lock-free hits | |---|---|---|---| | one hot key | 1.3 | 34–40 | 1.5–1.6 | | random of 1,024 keys | 1.6–2.0 | 52 | 8.3–9.9 | - Single-threaded, a hit costs 15.6 ns against 8.1 for `ConcurrentDictionary` (one key), and 29 ns against 14 (random keys). `DidKeyCache.Shared` gains lock-free hits too; it was lock-based on main. After the fixes: - Release build: 0 warnings. - Unit tests in Release: **5017 passed, 0 failed, 5 skipped**, run twice. - Aspire manifest check: 6/6. Items 3–5 and 7 restore main's behaviour, so they have no CHANGELOG entry. ## Deferred **C-Q6 (`PdsAdminClient` through `IXrpcSessionHandler`) is not done, because it would change behaviour.** - `XrpcClient` asks the session handler to recover only on `ExpiredToken` or a `WWW-Authenticate: … invalid_token` challenge. - `PdsAdminClient` signs in again on any 401. Tranquil answers a revoked or invalidated session with 401 `AuthenticationFailed`, which the handler path would no longer recover from. - The handler path would also start retrying through the raw `Admin` / `Server` clients. **Pass E's files are untouched.** No Lexicon client test needed a call-site fix. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Pass D, first part:
- H1: BoundedContent moves to Http and gains a Stream overload; the
  SyncClient, RepoFetchers (keeping its IOException mapping), Tap webhook
  and SpaceSyncer copies go.
- J1: GetStringOrNull and friends live in Serialization; the copies in the
  session JSON reader, Jetstream parser, chat unknown variants, Lexicon
  models and Tap models go.
- K1: DidDocument.GetSigningKey/GetVerificationKey return null for key
  material that does not decode.
- A1: AtProtoJsonDefaults.FormatTimestamp/NowTimestamp are removed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
V1 + B[7]: IDidResolver.VerifyWithRefreshAsync holds the refetch-once rule
(refetch only after a missing key or a failed signature, retry only if the
key changed, through the rate-limited RefreshAsync) and its rationale.
RepoSyncVerifier, LabelVerifier, ServiceAuthVerifier, SpaceSyncer and the
space credential and delegation checks (SpaceDidResolution.VerifyTokenAsync)
use it; each keeps its own key rule and maps resolution failures to its own
error. AtProtoCrypto.TryVerifySignature / TryVerifyJwtSignature replace the
copied catch lists.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- X1: QueryAsync, ProcedureAsync, UploadAsync and the output-less calls
  share one SendAsync<T> for the per-call deadline, and BuildUri uses
  XrpcParams.ToQueryString.
- C-Q3: the anonymous-object QueryAsync overload and XrpcParams.From's
  reflection path are removed; query parameters are an XrpcParams.
- N1: identity service URLs are validated by AtProtoHttp.ValidateServiceUrl
  (allowLoopback: false), and the repository fetchers' default client is
  IdentityNetworkPolicy.CreateClient with their five-minute timeout.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
L1(core) + B[16] + L8: the internal LruCache replaces the hand-rolled
bounded maps of CachingDidResolver, DidKeyCache, SpaceCredentialVerifier
(its per-authority quota kept on top), JsonWebKeyVerifier,
SpaceClientAttestationVerifier, AtProtoClientFactory, the OAuth metadata
cache and DPoPNonceCache. The four that cleared themselves or dropped
arbitrary entries when full now evict the least recently used.

ResolutionCache holds CachingDidResolver's stale-while-revalidate,
negative-TTL, single-flight and distributed-tier logic;
CachingLexiconResolver runs on it with failures sharing its one capacity,
as before. Its options stay LexiconCacheOptions: the defaults differ.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SpaceRecordCbor.TryValidateRecord catches through DagCborDecoder.IsMalformed,
the filter the repository readers share, instead of its own copy of the list.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Pass F's lean doc rule for the files Pass D owns: Identity, Http, Crypto,
Serialization, Labeling, Spaces (client and server), the verifiers,
PdsAdminClient, AtProtoClient and the cache classes. Plain-prose public
summaries sit on one line; docs on internal and private members become //
comments with the XML stripped, or go when they said nothing. Comments only.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
docs(changelog): tighten the pass D bullets (#180)
All checks were successful
CI / pds-integration (pull_request) Successful in 30s
Details
CI / build-and-test (pull_request) Successful in 59s
Details
a03171b7f6 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
opencode-bot commented 2026-09-27 17:13:23 +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-185/run-2026-09-27T17-13-22-899Z.md) — updating every ~5 minutes while the review runs.
opencode-bot commented 2026-09-27 17:13:26 +00:00
Collaborator
Copy link

⏳ 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:13:23.362Z) — 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:13:23.362Z) — nothing to re-trigger. <!-- opencode-run:paused --> <!-- opencode-session:ses_f1c248cbeffezbNGoilqeZhCNH -->
Grandiras commented 2026-09-27 17:13:31 +00:00
Author
Owner
Copy link

@claude /review, please. This is debloat pass D of #180 (shared primitives), and it is security-relevant: the verify-with-refresh helper sits on every signature-verification path. The diff will be truncated, so please read these files in full from the branch:

  • the new internal DidResolverExtensions.VerifyWithRefreshAsync, and its use in RepoSyncVerifier, LabelVerifier, ServiceAuthVerifier, SpaceDidResolution and SpaceSyncer. Check that each caller keeps its own key-id rule and resolution-failure mapping, refetches only on a missing key or failed signature, retries only on a changed key, and stays under the per-DID rate limit;
  • AtProtoCrypto.TryVerifySignature / TryVerifyJwtSignature;
  • the new Caching/LruCache and ResolutionCache, which now back CachingDidResolver and CachingLexiconResolver. Check stale-while-revalidate, the negative TTL, single-flight and detach-on-invalidate;
  • the five caches whose eviction changed to LRU, and SpaceCredentialVerifier's per-authority quota;
  • the shared bounded reader (pre-allocation now capped at 1 MiB);
  • DidDocument.GetSigningKey/GetVerificationKey returning null for a malformed key;
  • the XrpcClient send path.
    The last commit, e095384, only changes comments.
@claude /review, please. This is debloat pass D of #180 (shared primitives), and it is security-relevant: the verify-with-refresh helper sits on every signature-verification path. The diff will be truncated, so please read these files in full from the branch: - the new internal `DidResolverExtensions.VerifyWithRefreshAsync`, and its use in `RepoSyncVerifier`, `LabelVerifier`, `ServiceAuthVerifier`, `SpaceDidResolution` and `SpaceSyncer`. Check that each caller keeps its own key-id rule and resolution-failure mapping, refetches only on a missing key or failed signature, retries only on a changed key, and stays under the per-DID rate limit; - `AtProtoCrypto.TryVerifySignature` / `TryVerifyJwtSignature`; - the new `Caching/LruCache` and `ResolutionCache`, which now back `CachingDidResolver` and `CachingLexiconResolver`. Check stale-while-revalidate, the negative TTL, single-flight and detach-on-invalidate; - the five caches whose eviction changed to LRU, and `SpaceCredentialVerifier`'s per-authority quota; - the shared bounded reader (pre-allocation now capped at 1 MiB); - `DidDocument.GetSigningKey`/`GetVerificationKey` returning null for a malformed key; - the `XrpcClient` send path. The last commit, e095384, only changes comments.
claude-bot commented 2026-09-27 17:13: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/641); 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-185/run-641.md) — updating every ~5 minutes while the task runs.
claude-reviewer reviewed 2026-09-27 17:15:01 +00:00
claude-reviewer left a comment
Copy link

💬 Verdict: comment — notes only, nothing blocking.

Large, well-organized refactor. The parts of the diff I can check are internally consistent: SpaceDidResolution.VerifyTokenAsync reproduces the old refetch-once/retry-only-if-key-changed semantics of VerifyWithKeyRefreshAsync through the new shared DidResolverExtensions.VerifyWithRefreshAsync helper, SpaceCredentialVerifier's LRU-backed cache and per-authority quota logic look equivalent to the old linked-list version, and the K1 change (GetVerificationKey returning null instead of throwing FormatException) is correctly threaded through call sites that used to catch FormatException (e.g. SpaceCredentialVerifier.IsKeyCurrentAsync, ServiceAuthVerifier). CHANGELOG and migration-guide entries are present as required by CLAUDE.md.

Note: the PR diff was truncated before I could see the new core files themselves (Caching/LruCache.cs, Caching/ResolutionCache.cs, Identity/DidResolverExtensions.cs, Http/BoundedContent.cs, Crypto/AtProtoCrypto.cs), so I could only verify caller-side consistency with the PR description's stated rules, not the helpers' internals.

One minor concurrency regression worth a look (see inline finding) in SpaceClientAttestationVerifier.FetchKeysAsync.

⏱ 1m 17s · 1 turns · tokens: 76.4k in / 8.6k out (+23.9k cache-read)
📜 Full session transcript

💬 **Verdict: comment** — notes only, nothing blocking. Large, well-organized refactor. The parts of the diff I can check are internally consistent: `SpaceDidResolution.VerifyTokenAsync` reproduces the old refetch-once/retry-only-if-key-changed semantics of `VerifyWithKeyRefreshAsync` through the new shared `DidResolverExtensions.VerifyWithRefreshAsync` helper, `SpaceCredentialVerifier`'s LRU-backed cache and per-authority quota logic look equivalent to the old linked-list version, and the K1 change (`GetVerificationKey` returning `null` instead of throwing `FormatException`) is correctly threaded through call sites that used to catch `FormatException` (e.g. `SpaceCredentialVerifier.IsKeyCurrentAsync`, `ServiceAuthVerifier`). CHANGELOG and migration-guide entries are present as required by CLAUDE.md. Note: the PR diff was truncated before I could see the new core files themselves (`Caching/LruCache.cs`, `Caching/ResolutionCache.cs`, `Identity/DidResolverExtensions.cs`, `Http/BoundedContent.cs`, `Crypto/AtProtoCrypto.cs`), so I could only verify caller-side consistency with the PR description's stated rules, not the helpers' internals. One minor concurrency regression worth a look (see inline finding) in `SpaceClientAttestationVerifier.FetchKeysAsync`. --- ⏱ 1m 17s · 1 turns · tokens: 76.4k in / 8.6k out (+23.9k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-185/run-641.md) <!-- claude-verdict:comment --> <!-- claude-reviewed-head:a03171b7f648589d77607cc19d5f374bd95cc3f1 -->
src/ATProtoNet.Server/Spaces/SpaceClientAttestationVerifier.cs
claude-reviewer commented 2026-09-27 17:15:01 +00:00
Collaborator
Copy link

This replaces the previous atomic ConcurrentDictionary.AddOrUpdate with a non-atomic TryPeek + Set pair. Two concurrent callers can both read the same existing value and then both call Set, so one thread's AttemptedAt update can be silently lost (or a fresh ClientKeys(null, now, now) can overwrite a value that just got the AttemptedAt bump from another thread). The class doc for LruCache says the cache itself is thread-safe, but that only makes each individual TryPeek/Set call atomic, not this check-then-act sequence — the same class of race AddOrUpdate existed specifically to avoid. Impact is limited (worst case an extra JWKS refetch within the 30s window rather than a security issue), but it's a real behavior change from the atomic original that isn't called out in the PR description.

This replaces the previous atomic `ConcurrentDictionary.AddOrUpdate` with a non-atomic `TryPeek` + `Set` pair. Two concurrent callers can both read the same `existing` value and then both call `Set`, so one thread's `AttemptedAt` update can be silently lost (or a fresh `ClientKeys(null, now, now)` can overwrite a value that just got the `AttemptedAt` bump from another thread). The class doc for `LruCache` says the cache itself is thread-safe, but that only makes each individual `TryPeek`/`Set` call atomic, not this check-then-act sequence — the same class of race `AddOrUpdate` existed specifically to avoid. Impact is limited (worst case an extra JWKS refetch within the 30s window rather than a security issue), but it's a real behavior change from the atomic original that isn't called out in the PR description.
Grandiras force-pushed debloat-d-primitives from a03171b7f6
All checks were successful
CI / pds-integration (pull_request) Successful in 30s
Details
CI / build-and-test (pull_request) Successful in 59s
Details
to 3c48b0a42b
All checks were successful
CI / pds-integration (pull_request) Successful in 26s
Details
CI / build-and-test (pull_request) Successful in 1m4s
Details
Sync Closures to GitHub / sync-closure (pull_request) Successful in 7s
Details
2026-09-27 17:52:48 +00:00 Compare
Grandiras commented 2026-09-27 17:52:57 +00:00
Author
Owner
Copy link

@claude /review, please re-review. The branch is rebased onto main (#186, test-only, no conflicts). New commits fix an independent adversarial review and your note:

  1. Shared resolutions start on the thread pool, so a caller's SynchronizationContext can no longer deadlock them. There is a test with a context that is never pumped, and a Fixed bullet, because CachingDidResolver had the same problem on main.
  2. The Tap webhook sizes a body by its bytes, not by its declared Content-Length.
  3. UploadAsync<T> faults its task on bad arguments again.
  4. Your note: the client-attestation cache uses a new atomic LruCache.AddOrUpdate. The test drives the race deterministically.
  5. Each resolver's log messages and structured properties are restored.
  6. Caller tests pin which key failures trigger a refetch.
  7. Cache hits no longer take a lock. The hot nonce and JWK caches had become serialised on one global lock: 34–52 ns per get under 16 threads, against 1.3–2 ns on main. With lock-free hits it is 1.5–10 ns.

I then added a concurrent stress test of the lock-free path. It caught Count briefly one over capacity during a write, and 3c48b0a fixes that by evicting before adding.

Please read src/ATProtoNet/Caching/LruCache.cs in full, especially the lock-free TryGetValue: the node.List recheck under the lock, and the claim that published nodes are immutable. Please also read the fix commits.

@claude /review, please re-review. The branch is rebased onto main (#186, test-only, no conflicts). New commits fix an independent adversarial review and your note: 1. Shared resolutions start on the thread pool, so a caller's `SynchronizationContext` can no longer deadlock them. There is a test with a context that is never pumped, and a Fixed bullet, because `CachingDidResolver` had the same problem on main. 2. The Tap webhook sizes a body by its bytes, not by its declared `Content-Length`. 3. `UploadAsync<T>` faults its task on bad arguments again. 4. Your note: the client-attestation cache uses a new atomic `LruCache.AddOrUpdate`. The test drives the race deterministically. 5. Each resolver's log messages and structured properties are restored. 6. Caller tests pin which key failures trigger a refetch. 7. Cache hits no longer take a lock. The hot nonce and JWK caches had become serialised on one global lock: 34–52 ns per get under 16 threads, against 1.3–2 ns on main. With lock-free hits it is 1.5–10 ns. I then added a concurrent stress test of the lock-free path. It caught `Count` briefly one over capacity during a write, and **3c48b0a** fixes that by evicting before adding. Please read `src/ATProtoNet/Caching/LruCache.cs` in full, especially the lock-free `TryGetValue`: the `node.List` recheck under the lock, and the claim that published nodes are immutable. Please also read the fix commits.
claude-bot commented 2026-09-27 17:53:01 +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/644); 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-185/run-644.md) — updating every ~5 minutes while the task runs.
claude-reviewer approved these changes 2026-09-27 17:53:47 +00:00
claude-reviewer left a comment
Copy link

✅ Verdict: approve — no blocking issues found.

This is a re-review after new commits (the "review fixes" batch described in the PR body). My previous pass flagged one concrete issue: SpaceClientAttestationVerifier.FetchKeysAsync had replaced the old atomic ConcurrentDictionary.AddOrUpdate with a non-atomic TryPeek + Set pair, which could lose an AttemptedAt update under concurrent callers.

That is now fixed (review fix 4, commit ea3f5d9): the code now calls _clientKeys.AddOrUpdate(clientId, new ClientKeys(null, now, now), existing => existing with { AttemptedAt = now }), and LruCache.AddOrUpdate performs the read-modify-write in one step under the cache's lock, matching ConcurrentDictionary.AddOrUpdate's atomicity. The PR description also notes this exact class of bug was caught independently in SpaceClientAttestationVerifier's sibling, the attestation key cache (ea3f5d9), with a deterministic-interleaving regression test — consistent with what I flagged.

Spot-checking the rest of the new/changed logic visible in the diff:

  • SpaceDidResolution.VerifyTokenAsync correctly threads key selection (throwing SpaceVerificationException for absent/malformed key) and verification (catching SpaceTokenException when IsSignatureFailure) through the new shared DidResolverExtensions.VerifyWithRefreshAsync, and only wraps SpaceTokenException from the outer call, letting SpaceVerificationException from key selection propagate unchanged — matches the old VerifyWithKeyRefreshAsync semantics.
  • AtProtoClientFactory's LruCache-backed DPoP key cache correctly drops the manual LastUsed/EvictLeastRecentlyUsed bookkeeping now that the cache tracks recency itself, and ForgetKey's TryPeek + Remove(key, value) pair is safe since removal is conditioned on value equality.
  • SpaceCredentialVerifier's cache now wraps an optional LruCache (null when capacity ≤ 0), and Lookup/Remember/Forget are equivalent to the prior linked-list implementation, including the per-authority quota eviction via RemoveLeastRecent.

No new issues found in the visible diff (the full diff was truncated again past SpaceServerExtensions.cs, as last time, so the newest-added core files aren't directly visible here, but the caller-side usage is consistent with the description). Approving.

⏱ 0m 32s · 2 turns · tokens: 80.8k in / 2.5k out (+127.0k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. This is a re-review after new commits (the "review fixes" batch described in the PR body). My previous pass flagged one concrete issue: `SpaceClientAttestationVerifier.FetchKeysAsync` had replaced the old atomic `ConcurrentDictionary.AddOrUpdate` with a non-atomic `TryPeek` + `Set` pair, which could lose an `AttemptedAt` update under concurrent callers. That is now fixed (review fix 4, commit `ea3f5d9`): the code now calls `_clientKeys.AddOrUpdate(clientId, new ClientKeys(null, now, now), existing => existing with { AttemptedAt = now })`, and `LruCache.AddOrUpdate` performs the read-modify-write in one step under the cache's lock, matching `ConcurrentDictionary.AddOrUpdate`'s atomicity. The PR description also notes this exact class of bug was caught independently in `SpaceClientAttestationVerifier`'s sibling, the attestation key cache (`ea3f5d9`), with a deterministic-interleaving regression test — consistent with what I flagged. Spot-checking the rest of the new/changed logic visible in the diff: - `SpaceDidResolution.VerifyTokenAsync` correctly threads key selection (throwing `SpaceVerificationException` for absent/malformed key) and verification (catching `SpaceTokenException` when `IsSignatureFailure`) through the new shared `DidResolverExtensions.VerifyWithRefreshAsync`, and only wraps `SpaceTokenException` from the outer call, letting `SpaceVerificationException` from key selection propagate unchanged — matches the old `VerifyWithKeyRefreshAsync` semantics. - `AtProtoClientFactory`'s `LruCache`-backed DPoP key cache correctly drops the manual `LastUsed`/`EvictLeastRecentlyUsed` bookkeeping now that the cache tracks recency itself, and `ForgetKey`'s `TryPeek` + `Remove(key, value)` pair is safe since removal is conditioned on value equality. - `SpaceCredentialVerifier`'s cache now wraps an optional `LruCache` (null when capacity ≤ 0), and `Lookup`/`Remember`/`Forget` are equivalent to the prior linked-list implementation, including the per-authority quota eviction via `RemoveLeastRecent`. No new issues found in the visible diff (the full diff was truncated again past `SpaceServerExtensions.cs`, as last time, so the newest-added core files aren't directly visible here, but the caller-side usage is consistent with the description). Approving. --- ⏱ 0m 32s · 2 turns · tokens: 80.8k in / 2.5k out (+127.0k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-185/run-644.md) <!-- claude-verdict:approve --> <!-- claude-reviewed-head:3c48b0a42beb3252f413b502769661474836e3b4 -->
opencode-bot commented 2026-09-27 17:53:49 +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-185/run-2026-09-27T17-53-48-615Z.md) — updating every ~5 minutes while the review runs.
opencode-bot commented 2026-09-27 17:53:51 +00:00
Collaborator
Copy link

⏳ 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:53:49.031Z) — 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:53:49.031Z) — nothing to re-trigger. <!-- opencode-run:paused --> <!-- opencode-session:ses_f1bff8a74ffeHXftRKQX810saX -->
Grandiras commented 2026-09-27 17:55:01 +00:00
Author
Owner
Copy link

Supplementary check of src/ATProtoNet/Caching/LruCache.cs at 3c48b0a. The claude diff was truncated before this file, so I read it in full myself.

  • Every write (Set, AddOrUpdate, GetOrAdd, Remove*, RemoveLeastRecent) runs under _lock. The map and the recency list are only ever changed together, under that lock.
  • The lock-free TryGetValue reads the node's KeyValuePair from a node that is never changed after it is published: a new value always gets a new node. It only moves the node when TryEnter succeeds, and re-checks node.List is not null first, so an entry that was evicted or replaced in the meantime is never re-inserted.
  • The concurrent stress test ConcurrentUse_KeepsTheBoundAndLeavesACacheThatStillEvictsInOrder runs 16 threads × 20k mixed operations. It checks the bound on every step, then that a single-threaded refill evicts in exact order, which proves the map and list still agree. It found the add-then-evict transient that 3c48b0a fixes. It has passed 5 out of 5 runs since then.
  • The full Release suite passes (5,018 tests), and so does the Aspire manifest check.

Merging on the approval of this head.

Supplementary check of `src/ATProtoNet/Caching/LruCache.cs` at 3c48b0a. The claude diff was truncated before this file, so I read it in full myself. - Every write (`Set`, `AddOrUpdate`, `GetOrAdd`, `Remove*`, `RemoveLeastRecent`) runs under `_lock`. The map and the recency list are only ever changed together, under that lock. - The lock-free `TryGetValue` reads the node's `KeyValuePair` from a node that is never changed after it is published: a new value always gets a new node. It only moves the node when `TryEnter` succeeds, and re-checks `node.List is not null` first, so an entry that was evicted or replaced in the meantime is never re-inserted. - The concurrent stress test `ConcurrentUse_KeepsTheBoundAndLeavesACacheThatStillEvictsInOrder` runs 16 threads × 20k mixed operations. It checks the bound on every step, then that a single-threaded refill evicts in exact order, which proves the map and list still agree. It found the add-then-evict transient that 3c48b0a fixes. It has passed 5 out of 5 runs since then. - The full Release suite passes (5,018 tests), and so does the Aspire manifest check. Merging on the approval of this head.
Grandiras deleted branch debloat-d-primitives 2026-09-27 17:55:03 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
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
4 participants
Notifications
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!185
No description provided.