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

perf: close the v0.7 regressions on JSON models, XRPC writes, Jetstream and CAR parsing #191

Merged
Grandiras merged 4 commits from issue-190-perf into main 2026-09-28 13:21:49 +00:00
Conversation 17 Commits 4 Files changed 24 +1748 -295
Grandiras commented 2026-09-27 23:14:40 +00:00
Owner
Copy link

Closes #190.

The five regressions came from the cost of this cycle's correctness features, not from the features. Profiling showed where that cost was, and each fix below keeps the behaviour it pays for. Every validation rule, open union and extension-data round trip is unchanged.

Hot spots, with the evidence

I profiled with perf and JIT perf maps, running a scenario loop over the harness's own benchmark bodies, and sampled allocations by type from the runtime's GCAllocationTick events. Times below are the main share of one operation.

Hot spot Evidence on main (a2ac148)
Identifier regexes. Handle, Nsid, Did, RecordKey and Tid validated with [GeneratedRegex]; the Handle and NSID patterns backtrack. Timeline: HandlePattern 4.0 %, NsidPattern 3.5 %, DidPattern 0.9 %, and Match.AddMatch/Capture/RunSingleMatch 2.3 %, about 21 µs of 194 µs. IdentifierJsonConverter.Read was 23 % inclusive. Jetstream: NsidPattern 7 %. createRecord: NSID and DID patterns about 12 %.
Decode, then validate. Every identifier and datetime was transcoded to a string before it was checked. A Cid also kept a separate 36-byte array and eagerly built its string from bytes. An AtUri built all its parts. TranscodeHelper and Utf8Utility.TranscodeToUtf16 in every JSON profile. Per-type allocation showed AtUri, AtIdentifier, Nsid, RecordKey, Cid and a byte[] for each value.
Union members read twice. AtProtoUnionConverter called JsonSerializer.Deserialize(ref reader, typeInfo), which first skips the whole value to scope a new reader, so every union member was scanned twice. A nested union is scanned once more. The converter also built a $type string for each member. Timeline: GetReaderScopedToNextValue 5.4 % inclusive, about 10.7 µs.
CAR verification. The recomputed CID escapes as a byte[]. In v0.6.0's code shape the .NET 10 JIT stack-allocated it. The one-byte varints of every CID header went through a ReadUvarint that is no longer inlined. 1000-block CAR: allocation is equal to v0.6.0 without verification and +64 B per block with it. ReadUvarint was 19 % self time.
Jetstream member lookups. Each TryGetProperty(string) transcoded the member name, and the time went through DateTimeOffset.TryParse. TryGetNamedPropertyValue 12 %, GetUtf8FromText 2.8 %, DateTimeParse about 6 %.
AtDatetime.Parse in SerializePostRecord's body. v0.6.0 assigned a string there. About 40 ns: a loop over the preceding months per call, and a digit loop per field.

What changed

  • Identifier syntax (Identity/IdentifierSyntax.cs, new). These are hand-written checks for the DID, handle, NSID, record-key and TID grammars. They are generic over char and byte, so the same code validates a string and a JSON string's UTF-8 bytes. The character sets are checked by vectorized SearchValues scans, which leaves only the structure to walk. Each check names the regex it replaces.
  • UTF-8 read paths.
    • IdentifierJsonConverter and AtDatetimeJsonConverter read an unescaped, single-segment JSON string straight from ValueSpan, through a new IIdentifier<T>.TryCreate(ReadOnlySpan<byte>, out T). A default implementation keeps SpaceUri on the old path.
    • Anything escaped, invalid or @-prefixed falls back to the old string path. The old path also produces the error, so exceptions don't change.
  • AtUri.
    • It validates every part while parsing, as before, but keeps only the offsets.
    • Authority, Repo, Collection and RecordKey are built on first use, from the validated text, into one lazily allocated holder.
    • AtUri.Create pre-fills the holder.
  • Cid.
    • The 36-byte binary form is held inline in the object.
    • Equality and hash codes use the bytes. The string form is canonical, so this compares the same values, and the hash is still randomly seeded.
    • The string form is encoded on first use when the CID came from bytes or from JSON.
    • Digest is materialized once, on first use.
    • The base32 decoder now works in groups of eight characters.
  • AtDatetime.
    • Digit and calendar work is table-based.
    • The struct shrinks from three words to two: the ticks and the state share a word. The offset is read back from the text, which holds it.
    • TryParseUtc reads UTF-8 text exactly as DateTimeOffset.TryParse(…, AdjustToUniversal | AssumeUniversal) does, for the subset where the two provably agree: at most 7 fractional digits, an offset of at most ±14:00, and an instant in range on both sides of the offset. For anything else it returns false.
  • Open unions.
    • The converter scans a copy of the reader for $type, as before, and matches a declared variant on the raw UTF-8 bytes.
    • It then reads the variant with the variant's own JsonConverter<T> on the caller's reader, when that converter is one of System.Text.Json's own. A custom converter still goes through JsonSerializer.Deserialize. Readers are cached per variant and checked against the options.
    • Registered variants and unknown variants take the old string path.
    • If the direct read throws, the outermost union being read directly reads its member once more through JsonSerializer.Deserialize, with every union nested in it doing the same, and throws that error. The error and its variant-relative JSON path are exactly what they were.
  • CAR.
    • The recomputed CID is written into a stackalloc buffer. The new CidComputation.WriteBinaryCid is shared with ComputeBinaryCid.
    • CidComputation.Verify compares bytes instead of two encoded strings.
    • ReadUvarint inlines the one-byte case and keeps the general reader out of line.
  • Jetstream.
    • Member lookups use UTF-8 literals.
    • Identifiers are read from the member's raw UTF-8 through JsonElementExtensions.GetIdentifierOrNull<T>, and fall back to T.TryParse semantics when escaped.
    • operation is matched with ValueEquals.
    • The frame time uses AtDatetime.TryParseUtc, falling back to DateTimeOffset.TryParse.

No public API changed. No PublicAPI file changed, and there is no migration entry.

Review fixes

An adversarial review, with a differential fuzzer against main, found three problems in the union converter's direct read. Each fix is its own commit, with regression tests that fail without it. They never shipped, so the CHANGELOG keeps its one Changed bullet.

  • Nested unions around an invalid member (critical). Every union replayed its member inside its catch block, and so did every union above it. The member was read 2^depth times, with the failed frames still on the stack. Seven nested recordWithMedia embeds (405 bytes) around an invalid external overflowed the stack and killed the process. main throws a JsonException in milliseconds, and anyone can publish such a post.
    • Now a thread-static read mode lets only the outermost union being read directly handle a failure. The unions nested in it pass the exception on.
    • That union's replay runs after its catch has exited, and during it every union reads through JsonSerializer.Deserialize, as on main.
    • A failed member is read directly once and replayed once, however deep the nesting. Replaying from the outermost union also keeps main's order of errors: a reader error anywhere in the member, such as the depth limit, still wins over a conversion error found earlier.
  • Custom variant converters (low). A converter that the application registered for a variant was also called directly. That skipped System.Text.Json's check that a custom converter reads exactly its value, and let it read past the value: an over-reading converter for mentions swallowed the tag facet after it. Now only System.Text.Json's own converters are called directly, and any other converter goes through JsonSerializer.Deserialize, as on main.
  • Where a failed read leaves the reader (low). The outermost union now resets the reader to the member's start and returns what JsonSerializer.Deserialize gives, whether a value or an error, and a nested failure that a converter catches still triggers that reread. Before, a converter that caught an invalid union member and skipped it read on from inside the member, and a reread that succeeded was discarded.

Results

Measured on the root server: AMD EPYC-Genoa, 8 vCPU, in a container (Ubuntu 24.04), with BenchmarkDotNet 0.15.8 (default job, in-process toolchain, server GC) on .NET 10.0.12. The columns are v0.6.0 (7f672c0), main before this PR (a2ac148) and this PR before the review fixes (4ecdde4). Time is the mean, and alloc is per operation.

Benchmark v0.6.0 main this PR PR / v0.6 PR / main alloc v0.6 alloc main alloc PR alloc PR / v0.6
CarBenchmarks.ParseCommitCar 21.81 µs 25.04 µs 21.92 µs 1.01× 0.88× 13.0 KB 15.2 KB 13.4 KB 1.03×
CarBenchmarks.ParseSynthetic1000BlockCar 610.17 µs 711.20 µs 596.14 µs 0.98× 0.84× 282.2 KB 345.2 KB 282.6 KB 1.00×
CryptoBenchmarks.ParseDidKey 596.91 µs 586.25 µs 589.93 µs 0.99× 1.01× 2.1 KB 1.2 KB 1.2 KB 0.55×
CryptoBenchmarks.VerifyK256Signature 341.18 µs 336.23 µs 341.48 µs 1.00× 1.02× 1.6 KB 1.4 KB 1.4 KB 0.86×
CryptoBenchmarks.VerifyP256Signature 65.38 µs 65.80 µs 65.32 µs 1.00× 0.99× 1.7 KB 1.4 KB 1.4 KB 0.84×
DagCborBenchmarks.DecodeCommitFrameBody 51.73 µs 15.03 µs 15.07 µs 0.29× 1.00× 58.7 KB 47.9 KB 47.9 KB 0.82×
DagCborBenchmarks.DecodeLargerRecord 20.93 µs 15.92 µs 15.60 µs 0.75× 0.98× 29.9 KB 12.6 KB 12.6 KB 0.42×
FirehoseBenchmarks.ParseFirehoseCommitFrame 17.00 µs 15.81 µs 15.60 µs 0.92× 0.99× 46.2 KB 46.3 KB 45.5 KB 0.98×
IdentifierBenchmarks.GenerateTid 62.0 ns 61.5 ns 61.9 ns 1.00× 1.01× 72 B 72 B 72 B 1.00×
IdentifierBenchmarks.ParseAtUri 443.0 ns 464.9 ns 74.6 ns 0.17× 0.16× 1.1 KB 360 B 40 B 0.04×
IdentifierBenchmarks.ParseDid 37.0 ns 38.0 ns 17.6 ns 0.48× 0.46× 24 B 24 B 24 B 1.00×
IdentifierBenchmarks.ParseHandle 257.5 ns 249.4 ns 29.4 ns 0.11× 0.12× 24 B 24 B 24 B 1.00×
IdentifierBenchmarks.ParseNsid 311.4 ns 240.9 ns 33.9 ns 0.11× 0.14× 208 B 24 B 24 B 0.12×
IdentifierBenchmarks.ParseTid 32.5 ns 29.0 ns 14.3 ns 0.44× 0.49× 24 B 24 B 24 B 1.00×
JetstreamBenchmarks.ParseJetstreamCommitEvent 2.14 µs 2.58 µs 1.85 µs 0.86× 0.71× 2.0 KB 1.6 KB 1.3 KB 0.63×
JsonBenchmarks.DeserializeGetTimelineResponse 193.38 µs 296.30 µs 217.80 µs 1.13× 0.74× 151.6 KB 213.1 KB 178.7 KB 1.18×
JsonBenchmarks.SerializePostRecord 668.6 ns 749.6 ns 712.2 ns 1.07× 0.95× 944 B 960 B 952 B 1.01×
LtHashBenchmarks.LtHashAddElement 20.11 µs 1.20 µs 1.21 µs 0.06× 1.01× 800 B 0 B 0 B 0.00×
MstBenchmarks.BuildFrom1000KeysAndComputeRoot 1.624 ms 1.551 ms 1.379 ms 0.85× 0.89× 1.30 MB 651.3 KB 651.3 KB 0.49×
MstBenchmarks.SingleInsertIntoExistingTree 1.605 ms 1.217 ms 1.112 ms 0.69× 0.91× 1.15 MB 594.5 KB 594.5 KB 0.50×
XrpcBenchmarks.XrpcCreateRecord 3.26 µs 4.43 µs 3.40 µs 1.04× 0.77× 4.6 KB 4.7 KB 4.3 KB 0.94×
XrpcBenchmarks.XrpcGetProfile 3.51 µs 4.05 µs 3.37 µs 0.96× 0.83× 4.6 KB 3.8 KB 3.8 KB 0.82×

The five paths from the issue, against v0.6.0:

  • Timeline (DeserializeGetTimelineResponse): 1.13× the time and 1.18× the allocation, down from 1.53× and 1.41× on main.
  • XrpcCreateRecord: 1.04× the time, 0.94× the allocation.
  • Jetstream (ParseJetstreamCommitEvent): 0.86× the time, 0.63× the allocation.
  • SerializePostRecord: 1.07× the time, 1.01× the allocation.
  • CAR: the 1000-block CAR at 0.98× the time and 1.00× the allocation, the commit CAR at 1.01× and 1.03×.

None of the benchmarks this PR doesn't target is more than 2 % slower than main.

The review fixes change only the union converter's error path and the variants with a custom converter. A local A/B of the timeline on an AMD Ryzen 5 7600X, alternating 4ecdde4 with the fixed branch, confirms that the happy path is unchanged. With the first two fixes, five runs each gave a median of the medians of 128.1 µs before and 127.8 µs after. With all three, three runs each gave a mean of the means of 127.2 µs before and 127.7 µs after, which is within the run-to-run noise of about ±3 µs. Every run allocated 182,952 B.

Not fully closed, and why

  • The timeline is still 13 % slower than v0.6.0 (218 vs 193 µs) and allocates 18 % more (179 vs 152 KB). The rest is the cost of this cycle's open unions, typed identifiers and fuller models, which v0.6.0 did not have. The owner accepted this.
  • SerializePostRecord is 7 % slower than v0.6.0 (712 vs 669 ns). Its v0.7 benchmark body also parses an AtDatetime, where v0.6.0 assigned a string.

Verification

  • dotnet build -c Release of the whole solution, which includes the compiled doc snippets: 0 warnings, 0 errors.
  • dotnet test tests/ATProtoNet.Tests -c Release, run twice: 4964 passed, 0 failed, 5 skipped. The 5 are the Aspire manifest tests, which the next check runs.
  • Aspire manifest: samples/ManagedPdsSample.AppHost --publisher manifest, then AspireManifestTests with ATPROTO_REQUIRE_INTEGRATION=1: 6 passed.
  • New equivalence tests (Identity/IdentifierSyntaxTests):
    • Each hand-written check is held to the regex it replaced. The inputs are:
      • every interop-fixture line;
      • 25 seeded mutations of each line;
      • 2,000 generated near-misses per grammar;
      • valid values exactly at each length limit, and one character over.
    • Both UTF-16 and UTF-8 are checked. AtUri is compared with the old composition of patterns, part by part.
    • I seeded 10 mutations into the checks (off-by-one limits, a dropped hyphen, TLD or .. rule), and the tests caught every one.
    • The JSON read of every fixture line, both plain and fully \u-escaped, is compared with TryParse. Datetime fixtures are compared with the wire reading, including validity, instant and offset.
    • AtDatetime.TryParseUtc is compared with DateTimeOffset.TryParse on the fixtures and 5,000 generated times. Removing its 7-digit limit or its 14-hour limit fails the test.
  • Other new tests.
    • OpenUnionTests: an invalid member of a variant still fails with the variant-relative path ($.did, $.uri). Removing the replay fails this test. An escaped $type value still resolves.
    • JetstreamV2EventParserTests: 20 time forms, including rounding past 7 digits, offsets beyond ±14:00, -00:00, out-of-range instants and escapes, match DateTimeOffset.TryParse. Escaped fields read as their text, and invalid required and optional fields drop what they did before.
    • CidTests and AtDatetimeTests: bytes-built and string-built CIDs are equal with equal hash codes. Valid and lenient datetimes keep their offset.
  • Review-fix tests (OpenUnionTests). The expected errors are pinned from main, and each test also passes with main's union converter swapped in.
    • Unions nested 60 deep around an invalid member throw main's JsonException (type, path and message) within 10 s. The cases are embeds read as EmbedBase, the same embeds as a post's embed, and thread parents read as ThreadNode. Each runs once on a thread-pool thread and once on a thread with a 256 MB stack. Without the fix, the test process dies of a stack overflow.
    • A converter that catches an invalid facet-feature member and skips it reads on after the member, as on main. There are 8 cases: the union read outermost or inside recordWithMedia, a bad or numeric did, and reading member by member or token by token. All 8 fail on the previous head, and the nested token-by-token case fails without the failure flag.
    • A custom converter for MentionFeature that reads past its value gives main's [null, TagFeature], and one that stops short gives main's error and path ($). Without the fix, the first returns [null] and the second names the union converter at $.facets[0].features[0].
  • The reviewer's differential fuzzer, rebuilt against the fixed branch:
    • The nested-union repro covers embeds and thread parents at 12 depths from 1 to 100, each on the main thread, a thread-pool thread and a 1 GB-stack thread. All 72 cases give output identical to main, including the depth-limit error from 64 levels on.
    • The custom-converter repro matches main for both converters.
    • I added an ordering check: a conversion error in a nested union, followed later in the outer member by a depth-limit or malformed-JSON error. All 3 cases give main's reader error.
    • The reviewer's fix mode has 784 nested-union parity cases (depth 0 to 62, good and bad leaves, six MaxDepth values, depth-versus-conversion races) and 42 DeserializeAsync documents with a thread hop on every read. All give main's result, and so do the six lenient-converter cases. One case still differs in a side effect: a converter that runs its own, unrelated deserialization during a direct read and catches the error. The document's result is main's, but that converter runs twice. The first run sees the error at Path: $, and the reread sees main's $.external.
    • Across 200,000 mutated union documents, read as a string, as a trickled async stream and as a multi-segment sequence, there are 0 divergences.

🤖 Generated with Claude Code

Closes #190. The five regressions came from the cost of this cycle's correctness features, not from the features. Profiling showed where that cost was, and each fix below keeps the behaviour it pays for. Every validation rule, open union and extension-data round trip is unchanged. ## Hot spots, with the evidence I profiled with `perf` and JIT perf maps, running a scenario loop over the harness's own benchmark bodies, and sampled allocations by type from the runtime's `GCAllocationTick` events. Times below are the `main` share of one operation. | Hot spot | Evidence on `main` (a2ac148) | |---|---| | **Identifier regexes.** `Handle`, `Nsid`, `Did`, `RecordKey` and `Tid` validated with `[GeneratedRegex]`; the Handle and NSID patterns backtrack. | Timeline: `HandlePattern` 4.0 %, `NsidPattern` 3.5 %, `DidPattern` 0.9 %, and `Match.AddMatch`/`Capture`/`RunSingleMatch` 2.3 %, about 21 µs of 194 µs. `IdentifierJsonConverter.Read` was 23 % inclusive. Jetstream: `NsidPattern` 7 %. createRecord: NSID and DID patterns about 12 %. | | **Decode, then validate.** Every identifier and datetime was transcoded to a `string` before it was checked. A `Cid` also kept a separate 36-byte array and eagerly built its string from bytes. An `AtUri` built all its parts. | `TranscodeHelper` and `Utf8Utility.TranscodeToUtf16` in every JSON profile. Per-type allocation showed `AtUri`, `AtIdentifier`, `Nsid`, `RecordKey`, `Cid` and a `byte[]` for each value. | | **Union members read twice.** `AtProtoUnionConverter` called `JsonSerializer.Deserialize(ref reader, typeInfo)`, which first skips the whole value to scope a new reader, so every union member was scanned twice. A nested union is scanned once more. The converter also built a `$type` string for each member. | Timeline: `GetReaderScopedToNextValue` 5.4 % inclusive, about 10.7 µs. | | **CAR verification.** The recomputed CID escapes as a `byte[]`. In v0.6.0's code shape the .NET 10 JIT stack-allocated it. The one-byte varints of every CID header went through a `ReadUvarint` that is no longer inlined. | 1000-block CAR: allocation is equal to v0.6.0 without verification and +64 B per block with it. `ReadUvarint` was 19 % self time. | | **Jetstream member lookups.** Each `TryGetProperty(string)` transcoded the member name, and the time went through `DateTimeOffset.TryParse`. | `TryGetNamedPropertyValue` 12 %, `GetUtf8FromText` 2.8 %, `DateTimeParse` about 6 %. | | **`AtDatetime.Parse`** in `SerializePostRecord`'s body. v0.6.0 assigned a string there. | About 40 ns: a loop over the preceding months per call, and a digit loop per field. | ## What changed - **Identifier syntax (`Identity/IdentifierSyntax.cs`, new).** These are hand-written checks for the DID, handle, NSID, record-key and TID grammars. They are generic over `char` and `byte`, so the same code validates a string and a JSON string's UTF-8 bytes. The character sets are checked by vectorized `SearchValues` scans, which leaves only the structure to walk. Each check names the regex it replaces. - **UTF-8 read paths.** - `IdentifierJsonConverter` and `AtDatetimeJsonConverter` read an unescaped, single-segment JSON string straight from `ValueSpan`, through a new `IIdentifier<T>.TryCreate(ReadOnlySpan<byte>, out T)`. A default implementation keeps `SpaceUri` on the old path. - Anything escaped, invalid or `@`-prefixed falls back to the old string path. The old path also produces the error, so exceptions don't change. - **`AtUri`.** - It validates every part while parsing, as before, but keeps only the offsets. - `Authority`, `Repo`, `Collection` and `RecordKey` are built on first use, from the validated text, into one lazily allocated holder. - `AtUri.Create` pre-fills the holder. - **`Cid`.** - The 36-byte binary form is held inline in the object. - Equality and hash codes use the bytes. The string form is canonical, so this compares the same values, and the hash is still randomly seeded. - The string form is encoded on first use when the CID came from bytes or from JSON. - `Digest` is materialized once, on first use. - The base32 decoder now works in groups of eight characters. - **`AtDatetime`.** - Digit and calendar work is table-based. - The struct shrinks from three words to two: the ticks and the state share a word. The offset is read back from the text, which holds it. - `TryParseUtc` reads UTF-8 text exactly as `DateTimeOffset.TryParse(…, AdjustToUniversal | AssumeUniversal)` does, for the subset where the two provably agree: at most 7 fractional digits, an offset of at most ±14:00, and an instant in range on both sides of the offset. For anything else it returns false. - **Open unions.** - The converter scans a copy of the reader for `$type`, as before, and matches a declared variant on the raw UTF-8 bytes. - It then reads the variant with the variant's own `JsonConverter<T>` on the caller's reader, when that converter is one of System.Text.Json's own. A custom converter still goes through `JsonSerializer.Deserialize`. Readers are cached per variant and checked against the options. - Registered variants and unknown variants take the old string path. - If the direct read throws, the outermost union being read directly reads its member once more through `JsonSerializer.Deserialize`, with every union nested in it doing the same, and throws that error. The error and its variant-relative JSON path are exactly what they were. - **CAR.** - The recomputed CID is written into a `stackalloc` buffer. The new `CidComputation.WriteBinaryCid` is shared with `ComputeBinaryCid`. - `CidComputation.Verify` compares bytes instead of two encoded strings. - `ReadUvarint` inlines the one-byte case and keeps the general reader out of line. - **Jetstream.** - Member lookups use UTF-8 literals. - Identifiers are read from the member's raw UTF-8 through `JsonElementExtensions.GetIdentifierOrNull<T>`, and fall back to `T.TryParse` semantics when escaped. - `operation` is matched with `ValueEquals`. - The frame time uses `AtDatetime.TryParseUtc`, falling back to `DateTimeOffset.TryParse`. No public API changed. No `PublicAPI` file changed, and there is no migration entry. ## Review fixes An adversarial review, with a differential fuzzer against `main`, found three problems in the union converter's direct read. Each fix is its own commit, with regression tests that fail without it. They never shipped, so the CHANGELOG keeps its one `Changed` bullet. - **Nested unions around an invalid member (critical).** Every union replayed its member inside its `catch` block, and so did every union above it. The member was read 2^depth times, with the failed frames still on the stack. Seven nested `recordWithMedia` embeds (405 bytes) around an invalid `external` overflowed the stack and killed the process. `main` throws a `JsonException` in milliseconds, and anyone can publish such a post. - Now a thread-static read mode lets only the outermost union being read directly handle a failure. The unions nested in it pass the exception on. - That union's replay runs after its `catch` has exited, and during it every union reads through `JsonSerializer.Deserialize`, as on `main`. - A failed member is read directly once and replayed once, however deep the nesting. Replaying from the outermost union also keeps `main`'s order of errors: a reader error anywhere in the member, such as the depth limit, still wins over a conversion error found earlier. - **Custom variant converters (low).** A converter that the application registered for a variant was also called directly. That skipped System.Text.Json's check that a custom converter reads exactly its value, and let it read past the value: an over-reading converter for mentions swallowed the tag facet after it. Now only System.Text.Json's own converters are called directly, and any other converter goes through `JsonSerializer.Deserialize`, as on `main`. - **Where a failed read leaves the reader (low).** The outermost union now resets the reader to the member's start and returns what `JsonSerializer.Deserialize` gives, whether a value or an error, and a nested failure that a converter catches still triggers that reread. Before, a converter that caught an invalid union member and skipped it read on from inside the member, and a reread that succeeded was discarded. ## Results Measured on the root server: AMD EPYC-Genoa, 8 vCPU, in a container (Ubuntu 24.04), with BenchmarkDotNet 0.15.8 (default job, in-process toolchain, server GC) on .NET 10.0.12. The columns are v0.6.0 (`7f672c0`), `main` before this PR (`a2ac148`) and this PR before the review fixes (`4ecdde4`). Time is the mean, and alloc is per operation. | Benchmark | v0.6.0 | main | this PR | PR / v0.6 | PR / main | alloc v0.6 | alloc main | alloc PR | alloc PR / v0.6 | |---|---:|---:|---:|---:|---:|---:|---:|---:|---:| | CarBenchmarks.ParseCommitCar | 21.81 µs | 25.04 µs | 21.92 µs | 1.01× | 0.88× | 13.0 KB | 15.2 KB | 13.4 KB | 1.03× | | CarBenchmarks.ParseSynthetic1000BlockCar | 610.17 µs | 711.20 µs | 596.14 µs | 0.98× | 0.84× | 282.2 KB | 345.2 KB | 282.6 KB | 1.00× | | CryptoBenchmarks.ParseDidKey | 596.91 µs | 586.25 µs | 589.93 µs | 0.99× | 1.01× | 2.1 KB | 1.2 KB | 1.2 KB | 0.55× | | CryptoBenchmarks.VerifyK256Signature | 341.18 µs | 336.23 µs | 341.48 µs | 1.00× | 1.02× | 1.6 KB | 1.4 KB | 1.4 KB | 0.86× | | CryptoBenchmarks.VerifyP256Signature | 65.38 µs | 65.80 µs | 65.32 µs | 1.00× | 0.99× | 1.7 KB | 1.4 KB | 1.4 KB | 0.84× | | DagCborBenchmarks.DecodeCommitFrameBody | 51.73 µs | 15.03 µs | 15.07 µs | 0.29× | 1.00× | 58.7 KB | 47.9 KB | 47.9 KB | 0.82× | | DagCborBenchmarks.DecodeLargerRecord | 20.93 µs | 15.92 µs | 15.60 µs | 0.75× | 0.98× | 29.9 KB | 12.6 KB | 12.6 KB | 0.42× | | FirehoseBenchmarks.ParseFirehoseCommitFrame | 17.00 µs | 15.81 µs | 15.60 µs | 0.92× | 0.99× | 46.2 KB | 46.3 KB | 45.5 KB | 0.98× | | IdentifierBenchmarks.GenerateTid | 62.0 ns | 61.5 ns | 61.9 ns | 1.00× | 1.01× | 72 B | 72 B | 72 B | 1.00× | | IdentifierBenchmarks.ParseAtUri | 443.0 ns | 464.9 ns | 74.6 ns | 0.17× | 0.16× | 1.1 KB | 360 B | 40 B | 0.04× | | IdentifierBenchmarks.ParseDid | 37.0 ns | 38.0 ns | 17.6 ns | 0.48× | 0.46× | 24 B | 24 B | 24 B | 1.00× | | IdentifierBenchmarks.ParseHandle | 257.5 ns | 249.4 ns | 29.4 ns | 0.11× | 0.12× | 24 B | 24 B | 24 B | 1.00× | | IdentifierBenchmarks.ParseNsid | 311.4 ns | 240.9 ns | 33.9 ns | 0.11× | 0.14× | 208 B | 24 B | 24 B | 0.12× | | IdentifierBenchmarks.ParseTid | 32.5 ns | 29.0 ns | 14.3 ns | 0.44× | 0.49× | 24 B | 24 B | 24 B | 1.00× | | JetstreamBenchmarks.ParseJetstreamCommitEvent | 2.14 µs | 2.58 µs | 1.85 µs | 0.86× | 0.71× | 2.0 KB | 1.6 KB | 1.3 KB | 0.63× | | JsonBenchmarks.DeserializeGetTimelineResponse | 193.38 µs | 296.30 µs | 217.80 µs | 1.13× | 0.74× | 151.6 KB | 213.1 KB | 178.7 KB | 1.18× | | JsonBenchmarks.SerializePostRecord | 668.6 ns | 749.6 ns | 712.2 ns | 1.07× | 0.95× | 944 B | 960 B | 952 B | 1.01× | | LtHashBenchmarks.LtHashAddElement | 20.11 µs | 1.20 µs | 1.21 µs | 0.06× | 1.01× | 800 B | 0 B | 0 B | 0.00× | | MstBenchmarks.BuildFrom1000KeysAndComputeRoot | 1.624 ms | 1.551 ms | 1.379 ms | 0.85× | 0.89× | 1.30 MB | 651.3 KB | 651.3 KB | 0.49× | | MstBenchmarks.SingleInsertIntoExistingTree | 1.605 ms | 1.217 ms | 1.112 ms | 0.69× | 0.91× | 1.15 MB | 594.5 KB | 594.5 KB | 0.50× | | XrpcBenchmarks.XrpcCreateRecord | 3.26 µs | 4.43 µs | 3.40 µs | 1.04× | 0.77× | 4.6 KB | 4.7 KB | 4.3 KB | 0.94× | | XrpcBenchmarks.XrpcGetProfile | 3.51 µs | 4.05 µs | 3.37 µs | 0.96× | 0.83× | 4.6 KB | 3.8 KB | 3.8 KB | 0.82× | The five paths from the issue, against v0.6.0: - **Timeline** (`DeserializeGetTimelineResponse`): 1.13× the time and 1.18× the allocation, down from 1.53× and 1.41× on `main`. - **`XrpcCreateRecord`**: 1.04× the time, 0.94× the allocation. - **Jetstream** (`ParseJetstreamCommitEvent`): 0.86× the time, 0.63× the allocation. - **`SerializePostRecord`**: 1.07× the time, 1.01× the allocation. - **CAR**: the 1000-block CAR at 0.98× the time and 1.00× the allocation, the commit CAR at 1.01× and 1.03×. None of the benchmarks this PR doesn't target is more than 2 % slower than `main`. The review fixes change only the union converter's error path and the variants with a custom converter. A local A/B of the timeline on an AMD Ryzen 5 7600X, alternating `4ecdde4` with the fixed branch, confirms that the happy path is unchanged. With the first two fixes, five runs each gave a median of the medians of 128.1 µs before and 127.8 µs after. With all three, three runs each gave a mean of the means of 127.2 µs before and 127.7 µs after, which is within the run-to-run noise of about ±3 µs. Every run allocated 182,952 B. ## Not fully closed, and why - **The timeline** is still 13 % slower than v0.6.0 (218 vs 193 µs) and allocates 18 % more (179 vs 152 KB). The rest is the cost of this cycle's open unions, typed identifiers and fuller models, which v0.6.0 did not have. The owner accepted this. - **`SerializePostRecord`** is 7 % slower than v0.6.0 (712 vs 669 ns). Its v0.7 benchmark body also parses an `AtDatetime`, where v0.6.0 assigned a string. ## Verification - `dotnet build -c Release` of the whole solution, which includes the compiled doc snippets: 0 warnings, 0 errors. - `dotnet test tests/ATProtoNet.Tests -c Release`, run twice: 4964 passed, 0 failed, 5 skipped. The 5 are the Aspire manifest tests, which the next check runs. - Aspire manifest: `samples/ManagedPdsSample.AppHost --publisher manifest`, then `AspireManifestTests` with `ATPROTO_REQUIRE_INTEGRATION=1`: 6 passed. - **New equivalence tests** (`Identity/IdentifierSyntaxTests`): - Each hand-written check is held to the regex it replaced. The inputs are: - every interop-fixture line; - 25 seeded mutations of each line; - 2,000 generated near-misses per grammar; - valid values exactly at each length limit, and one character over. - Both UTF-16 and UTF-8 are checked. `AtUri` is compared with the old composition of patterns, part by part. - I seeded 10 mutations into the checks (off-by-one limits, a dropped hyphen, TLD or `..` rule), and the tests caught every one. - The JSON read of every fixture line, both plain and fully `\u`-escaped, is compared with `TryParse`. Datetime fixtures are compared with the wire reading, including validity, instant and offset. - `AtDatetime.TryParseUtc` is compared with `DateTimeOffset.TryParse` on the fixtures and 5,000 generated times. Removing its 7-digit limit or its 14-hour limit fails the test. - **Other new tests.** - `OpenUnionTests`: an invalid member of a variant still fails with the variant-relative path (`$.did`, `$.uri`). Removing the replay fails this test. An escaped `$type` value still resolves. - `JetstreamV2EventParserTests`: 20 time forms, including rounding past 7 digits, offsets beyond ±14:00, `-00:00`, out-of-range instants and escapes, match `DateTimeOffset.TryParse`. Escaped fields read as their text, and invalid required and optional fields drop what they did before. - `CidTests` and `AtDatetimeTests`: bytes-built and string-built CIDs are equal with equal hash codes. Valid and lenient datetimes keep their offset. - **Review-fix tests** (`OpenUnionTests`). The expected errors are pinned from `main`, and each test also passes with `main`'s union converter swapped in. - Unions nested 60 deep around an invalid member throw `main`'s `JsonException` (type, path and message) within 10 s. The cases are embeds read as `EmbedBase`, the same embeds as a post's `embed`, and thread parents read as `ThreadNode`. Each runs once on a thread-pool thread and once on a thread with a 256 MB stack. Without the fix, the test process dies of a stack overflow. - A converter that catches an invalid facet-feature member and skips it reads on after the member, as on `main`. There are 8 cases: the union read outermost or inside `recordWithMedia`, a bad or numeric `did`, and reading member by member or token by token. All 8 fail on the previous head, and the nested token-by-token case fails without the failure flag. - A custom converter for `MentionFeature` that reads past its value gives `main`'s `[null, TagFeature]`, and one that stops short gives `main`'s error and path (`$`). Without the fix, the first returns `[null]` and the second names the union converter at `$.facets[0].features[0]`. - **The reviewer's differential fuzzer**, rebuilt against the fixed branch: - The nested-union repro covers embeds and thread parents at 12 depths from 1 to 100, each on the main thread, a thread-pool thread and a 1 GB-stack thread. All 72 cases give output identical to `main`, including the depth-limit error from 64 levels on. - The custom-converter repro matches `main` for both converters. - I added an ordering check: a conversion error in a nested union, followed later in the outer member by a depth-limit or malformed-JSON error. All 3 cases give `main`'s reader error. - The reviewer's `fix` mode has 784 nested-union parity cases (depth 0 to 62, good and bad leaves, six `MaxDepth` values, depth-versus-conversion races) and 42 `DeserializeAsync` documents with a thread hop on every read. All give `main`'s result, and so do the six lenient-converter cases. One case still differs in a side effect: a converter that runs its own, unrelated deserialization during a direct read and catches the error. The document's result is `main`'s, but that converter runs twice. The first run sees the error at `Path: $`, and the reread sees `main`'s `$.external`. - Across 200,000 mutated union documents, read as a string, as a trickled async stream and as a multi-segment sequence, there are 0 divergences. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
perf: close the v0.7 regressions on JSON models, XRPC writes, Jetstream and CAR parsing (closes #190)
All checks were successful
CI / pds-integration (pull_request) Successful in 29s
Details
CI / build-and-test (pull_request) Successful in 1m4s
Details
4ecdde48a2 
Profiling the regressed benchmarks against v0.6.0 found the cost in the
v0.7 correctness features, not in the features themselves being wrong:

- Handle, NSID, DID, record key and TID validation ran generated regular
  expressions (the Handle and NSID ones backtrack), about a tenth of a
  timeline read. They are now hand-written checks with vectorized
  character-set scans, held to the old patterns by a differential test
  on the interop fixtures, seeded mutations and length-limit cases.
- The JSON converters decoded every identifier and datetime to a string
  before validating it. They now validate the UTF-8 bytes and build the
  string once; a CID keeps only its binary form (inline) and encodes its
  string on use, and an AT URI builds its parts on first access.
- The open-union converter deserialized each member with
  JsonSerializer.Deserialize, which first skips the whole value to scope
  a reader: every union member was scanned twice, nested ones more. It
  now calls the variant's converter on the caller's reader, matches the
  declared $type on its UTF-8 bytes, and replays through Deserialize only
  to fail with the same exception as before.
- CAR verification allocated each recomputed CID (no longer stack
  allocated after the #182 reshaping), and the one-byte varints of every
  CID header went through a non-inlined reader.
- Jetstream frames transcoded every member name and parsed the time with
  DateTimeOffset.TryParse; names are UTF-8 literals now, and the time takes
  the atproto parser where that provably agrees with the framework's.

Validation rules, open unions and extension data are unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
opencode-bot commented 2026-09-27 23:14:42 +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-191/run-2026-09-27T23-14-42-024Z.md) — updating every ~5 minutes while the review runs.
opencode-bot commented 2026-09-27 23:14:45 +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-28T04:14:42.469Z) — 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-28T04:14:42.469Z) — nothing to re-trigger. <!-- opencode-run:paused --> <!-- opencode-session:ses_f1ad9c1a4ffeam8BP09KIEN8Ms -->
Grandiras commented 2026-09-27 23:14:49 +00:00
Author
Owner
Copy link

@claude /review, please. This is the perf follow-up (#190). It changes no behaviour, and the full benchmark table goes into the description once the root-server run finishes.

Please read these in full:

  • Identity/IdentifierSyntax.cs. It replaces the Handle/NSID/DID/AtUri/RecordKey/TID regexes with hand-written UTF-16/UTF-8 checks. They must accept and reject exactly what the regexes did. See the equivalence tests and the interop SyntaxInteropTests; I'm most interested in edge cases around length limits, label rules, trailing characters and non-ASCII.
  • The identifier and AtDatetime JSON converters, which now read straight from UTF-8.
  • Cid's lazy string and AtUri's lazy parts. Check thread safety and equality/hash consistency.
  • AtProtoUnionConverter. It dispatches to the variant converter directly, matches $type on UTF-8, and replays through Deserialize on error. Exceptions and JSON paths must be unchanged, and unknown variants must still round-trip.
  • The CAR uvarint fast path and the stack buffer.
  • The Jetstream UTF-8 names and time fast path.
@claude /review, please. This is the perf follow-up (#190). It changes no behaviour, and the full benchmark table goes into the description once the root-server run finishes. Please read these in full: - `Identity/IdentifierSyntax.cs`. It replaces the Handle/NSID/DID/AtUri/RecordKey/TID regexes with hand-written UTF-16/UTF-8 checks. They must accept and reject exactly what the regexes did. See the equivalence tests and the interop `SyntaxInteropTests`; I'm most interested in edge cases around length limits, label rules, trailing characters and non-ASCII. - The identifier and `AtDatetime` JSON converters, which now read straight from UTF-8. - `Cid`'s lazy string and `AtUri`'s lazy parts. Check thread safety and equality/hash consistency. - `AtProtoUnionConverter`. It dispatches to the variant converter directly, matches `$type` on UTF-8, and replays through `Deserialize` on error. Exceptions and JSON paths must be unchanged, and unknown variants must still round-trip. - The CAR uvarint fast path and the stack buffer. - The Jetstream UTF-8 names and time fast path.
claude-bot commented 2026-09-27 23:14:52 +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/646); 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-191/run-646.md) — updating every ~5 minutes while the task runs.
claude-reviewer approved these changes 2026-09-27 23:22:11 +00:00
Dismissed
claude-reviewer left a comment
Copy link

✅ Verdict: approve — no blocking issues found.

This is a large, low-level performance change (identifier/datetime validation, AtUri/Cid laziness, union single-pass reads, CAR/base32 decoding, Jetstream UTF-8 lookups), and I traced through the trickiest parts by hand rather than skimming:

  • AtDatetime's bit-packed _ticksAndState (ticks in the low 62 bits, state in the top 2): the packing/unpacking is safe because ticks is provably always non-negative and below 2^62 for any year the grammar accepts, so the OR never collides with the state bits. OffsetMinutes() reconstructing the offset from the last 6 characters of _text for a Valid value is correct given the fixed Z/±HH:MM suffix shape the parser already validated.
  • TryParseInstant's calendar math (CommonDaysBeforeMonth + a leap-day adjustment instead of the old per-month loop) is the standard technique and matches the old DaysBeforeMonth loop's output.
  • Base32Lower.TryDecode's 8-char/5-byte grouped decode: verified the bit-shifts (group >> 32/24/16/8/0) and the deferred seen > 31 invalid-character check — an invalid character corrupts a group's bits, but since it always still trips seen > 31 before any of bytes is trusted by the caller, no bad output can leak through on failure.
  • AtUri's lazy Parts and offset-based Authority/Collection/RecordKey: the authorityEnd/collectionEnd bookkeeping in TryParseSyntax and in the Create factory lines up exactly with how Value is built, including the _collectionEnd == Value.Length sentinel for "no record key".
  • AtProtoUnionConverter's single-pass variant read with the on-error replay: ExceptionDispatchInfo.Throw(ex) is only a fallback for the case where the replay unexpectedly doesn't throw; the expected path is that JsonSerializer.Deserialize(ref start, typeInfo) throws its own exception (with the correct variant-relative path) and that one propagates instead, which matches the PR description and the new OpenUnionTests.
  • Cid's inline binary storage plus lazy Value/Digest: equality/hashing moved to bytes is consistent with the string form being canonical, and GetHashCode still uses a per-process-seeded HashCode, preserving the DoS-resistance the old string.GetHashCode had.

I didn't find a functional regression. The one thing I couldn't independently verify (dotnet build/test is blocked in this environment) is whether bytes.CopyTo(_binary) against the readonly Binary _binary inline-array field in Cid's constructor actually compiles — C# does permit ref/write access to a readonly instance field from within its own constructor, which is what this relies on, and the PR states a clean Release build, so I'm treating this as very likely fine rather than a blocking concern.

Only a cosmetic nit below; nothing that should block merge.

⏱ 7m 7s · 12 turns · tokens: 114.0k in / 39.3k out (+1365.8k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. This is a large, low-level performance change (identifier/datetime validation, `AtUri`/`Cid` laziness, union single-pass reads, CAR/base32 decoding, Jetstream UTF-8 lookups), and I traced through the trickiest parts by hand rather than skimming: - **`AtDatetime`'s bit-packed `_ticksAndState`** (ticks in the low 62 bits, state in the top 2): the packing/unpacking is safe because ticks is provably always non-negative and below 2^62 for any year the grammar accepts, so the OR never collides with the state bits. `OffsetMinutes()` reconstructing the offset from the last 6 characters of `_text` for a `Valid` value is correct given the fixed `Z`/`±HH:MM` suffix shape the parser already validated. - **`TryParseInstant`'s calendar math** (`CommonDaysBeforeMonth` + a leap-day adjustment instead of the old per-month loop) is the standard technique and matches the old `DaysBeforeMonth` loop's output. - **`Base32Lower.TryDecode`'s 8-char/5-byte grouped decode**: verified the bit-shifts (`group >> 32/24/16/8/0`) and the deferred `seen > 31` invalid-character check — an invalid character corrupts a group's bits, but since it always still trips `seen > 31` before any of `bytes` is trusted by the caller, no bad output can leak through on failure. - **`AtUri`'s lazy `Parts` and offset-based `Authority`/`Collection`/`RecordKey`**: the `authorityEnd`/`collectionEnd` bookkeeping in `TryParseSyntax` and in the `Create` factory lines up exactly with how `Value` is built, including the `_collectionEnd == Value.Length` sentinel for "no record key". - **`AtProtoUnionConverter`'s single-pass variant read with the on-error replay**: `ExceptionDispatchInfo.Throw(ex)` is only a fallback for the case where the replay unexpectedly *doesn't* throw; the expected path is that `JsonSerializer.Deserialize(ref start, typeInfo)` throws its own exception (with the correct variant-relative path) and that one propagates instead, which matches the PR description and the new `OpenUnionTests`. - **`Cid`'s inline binary storage plus lazy `Value`/`Digest`**: equality/hashing moved to bytes is consistent with the string form being canonical, and `GetHashCode` still uses a per-process-seeded `HashCode`, preserving the DoS-resistance the old `string.GetHashCode` had. I didn't find a functional regression. The one thing I couldn't independently verify (dotnet build/test is blocked in this environment) is whether `bytes.CopyTo(_binary)` against the `readonly Binary _binary` inline-array field in `Cid`'s constructor actually compiles — C# does permit ref/write access to a readonly instance field from within its own constructor, which is what this relies on, and the PR states a clean Release build, so I'm treating this as very likely fine rather than a blocking concern. Only a cosmetic nit below; nothing that should block merge. --- ⏱ 7m 7s · 12 turns · tokens: 114.0k in / 39.3k out (+1365.8k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-191/run-646.md) <!-- claude-verdict:approve --> <!-- claude-reviewed-head:4ecdde48a2adabc9c5b37746cb91b2a5fc58ec27 -->
CHANGELOG.md
@ -412,6 +412,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- **Bodies from untrusted senders go through one bounded reader** — the Tap webhook, record proofs, repository downloads and space recovery use the identity and OAuth fetches' reader, with their own limits and errors; a response's declared length pre-allocates at most 1 MiB, and a webhook sender's none (#180)
- **Signature checks against a DID document share one refetch-once path** — the Sync 1.1, label, service auth and space token verifiers and `SpaceSyncer` keep their key rules and errors. `SpaceCommitVerifier.Verify` returns `false`, not an exception, for a curve this platform lacks (#180)
- **Every bounded cache evicts the least recently used entry** — the DPoP key, nonce, client-metadata, JWK and OAuth metadata caches no longer clear themselves or drop arbitrary entries when full; `CachingLexiconResolver` runs on `CachingDidResolver`'s cache core (#180)
- **Faster JSON model (de)serialization, XRPC calls, Jetstream parsing and CAR reading** — identifiers and datetimes are validated without regular expressions, straight from the UTF-8 bytes; `AtUri` builds its parts and `Cid` its string on first use; a union member is read in one pass. What is accepted and produced is unchanged (#190)
claude-reviewer commented 2026-09-27 23:22:11 +00:00
Collaborator
Copy link

Minor style nit: this bullet runs to roughly 330 characters, a bit past CLAUDE.md's guideline of "at most ~300 characters" per entry. Not blocking.

Minor style nit: this bullet runs to roughly 330 characters, a bit past CLAUDE.md's guideline of "at most ~300 characters" per entry. Not blocking.
opencode-bot commented 2026-09-28 04:15:13 +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-191/run-2026-09-28T04-15-12-472Z.md) — updating every ~5 minutes while the review runs.
opencode-reviewer reviewed 2026-09-28 04:45:15 +00:00
opencode-reviewer left a comment
Copy link

💬 Verdict: comment — the reviewer did not submit a structured review; the raw output follows.

AtUri.Equals is explicitly overridden (line 174) to compare only Value ordinally — so the new _parts holder and offset fields don't affect equality. And Handle's @-stripping is intact in the string path. Both concerns resolved. Now the union converter — the diff's FindDiscriminator looked suspicious (checking TokenType == String while seemingly on the property-name token), so let me read the real code.

⏱ 30m 3s · 4 steps · $0.0236 · tokens: 102.9k in / 378 out (+0 cache-read) · Go: 5h 0% · wk 0% · mo 82%
📜 Full session transcript

💬 **Verdict: comment** — the reviewer did not submit a structured review; the raw output follows. `AtUri.Equals` is explicitly overridden (line 174) to compare only `Value` ordinally — so the new `_parts` holder and offset fields don't affect equality. And `Handle`'s `@`-stripping is intact in the string path. Both concerns resolved. Now the union converter — the diff's `FindDiscriminator` looked suspicious (checking `TokenType == String` while seemingly on the property-name token), so let me read the real code. --- ⏱ 30m 3s · 4 steps · $0.0236 · tokens: 102.9k in / 378 out (+0 cache-read) · Go: 5h 0% · wk 0% · mo 82% 📜 [Full session transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-191/run-2026-09-28T04-15-12-472Z.md)
Grandiras commented 2026-09-28 12:59:11 +00:00
Author
Owner
Copy link

@claude /review, please re-review. An independent adversarial review found two problems in the union converter, and two new commits fix them.

  1. 95a898d (critical). Nested unions with an invalid leaf overflowed the stack. At every level the replay ran inside the catch, so the member was re-read 2^depth times without the stack unwinding. A 405-byte post was enough. Now a [ThreadStatic] UnionReadMode lets only the outermost union catch. It captures the exception, leaves the catch, and replays once in Reread mode, so every nested union takes main's plain Deserialize path. A failed member is therefore read directly once and replayed once, at any depth, and main's error order is kept. Regression tests nest 60 deep through embeds, a post's embed and thread parents, on a pool thread, and match main's JsonException.
  2. ee6a1f7 (low). A user-supplied custom converter no longer gets the direct read; only System.Text.Json's own converters do. Tests cover an over-reading and an under-reading converter.

The reviewer's fuzzer now matches main in all 72 stack-overflow cases (depths 1–100, pool, main and 1 GB stacks), in the custom-converter cases, and in 200k differential union documents. The timeline benchmark is unchanged (128.1 vs 127.8 µs), and 4,956 tests pass. Please read AtProtoUnionConverter.cs in full.

@claude /review, please re-review. An independent adversarial review found two problems in the union converter, and two new commits fix them. 1. **95a898d (critical).** Nested unions with an invalid leaf overflowed the stack. At every level the replay ran inside the `catch`, so the member was re-read 2^depth times without the stack unwinding. A 405-byte post was enough. Now a `[ThreadStatic]` `UnionReadMode` lets only the outermost union catch. It captures the exception, leaves the `catch`, and replays once in `Reread` mode, so every nested union takes main's plain `Deserialize` path. A failed member is therefore read directly once and replayed once, at any depth, and main's error order is kept. Regression tests nest 60 deep through embeds, a post's `embed` and thread parents, on a pool thread, and match main's `JsonException`. 2. **ee6a1f7 (low).** A user-supplied custom converter no longer gets the direct read; only System.Text.Json's own converters do. Tests cover an over-reading and an under-reading converter. The reviewer's fuzzer now matches main in all 72 stack-overflow cases (depths 1–100, pool, main and 1 GB stacks), in the custom-converter cases, and in 200k differential union documents. The timeline benchmark is unchanged (128.1 vs 127.8 µs), and 4,956 tests pass. Please read `AtProtoUnionConverter.cs` in full.
A union read directly replayed its member through JsonSerializer.Deserialize
inside its catch block whenever the read failed, and every union above it did
the same. Unions nested around an invalid member therefore read it 2^depth
times while the failed frames stayed on the stack: seven nested embeds of 405
bytes overflowed the stack and killed the process, where main throws a
JsonException in milliseconds. Anyone can publish such a post.

A thread-static read mode now lets only the outermost union being read
directly handle a failure. The unions nested in it pass the exception on, and
during its reread every union uses JsonSerializer.Deserialize, as on main. The
reread runs after the catch has exited, so the failed read has unwound. A
failed member is read directly once and again once, and fails with main's
error, JSON path and position.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
fix(serialization): read a variant with a custom converter through Deserialize after review (#190)
All checks were successful
CI / pds-integration (pull_request) Successful in 32s
Details
CI / build-and-test (pull_request) Successful in 1m6s
Details
ee6a1f755b 
The union converter called every variant's converter directly on the
caller's reader. For a converter the application registered, that skipped
System.Text.Json's check that a custom converter reads exactly its value, and
no longer confined it to the variant's bytes: a converter that read too far
consumed the next union member, so a post's tag facet vanished silently, and
one that stopped short failed with another error and path than on main.

Only System.Text.Json's own converters, which read exactly one value, are now
called directly. Any other converter goes through JsonSerializer.Deserialize,
as on main.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude-bot commented 2026-09-28 12:59:14 +00:00
Collaborator
Copy link

🤖 Reviewing this PR — follow along in the action run; the reply will link the full session transcript.

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

🤖 Reviewing this PR — follow along in [the action run](https://git.grandiras.net/Grandiras/claude-bot/actions/runs/647); 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-191/run-647.md) — updating every ~5 minutes while the task runs.
opencode-bot commented 2026-09-28 13:00:14 +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-191/run-2026-09-28T13-00-13-403Z.md) — updating every ~5 minutes while the review runs.
claude-reviewer approved these changes 2026-09-28 13:01:01 +00:00
Dismissed
claude-reviewer left a comment
Copy link

✅ Verdict: approve — no blocking issues found.

Re-review, focused mainly on the two "review fixes" this round added on top of the design I already approved in my previous pass (the union converter's nested-failure replay and the custom-converter direct-read gate), since those are exactly the kind of subtle concurrency/control-flow bugs that are easy to get wrong.

Nested-union replay (AtProtoUnionConverter.cs). Traced VariantReader<TVariant>.Read against UnionReadState.Mode:

  • Nested unions (mode == Direct) call _converter.Read with no try/catch, so a failure unwinds straight through every nested frame to the single outermost frame — this is what avoids the old exponential (2^depth) replay and the stack overflow it caused. Confirmed the mode is reset to None in finally on both the direct attempt and the reread, so a thrown exception can never leave the thread-static flag stuck.
  • The outermost frame's replay parses from a saved copy of the reader (start) via JsonSerializer.Deserialize, during which nested unions see mode == Reread and also go through JsonSerializer.Deserialize — matching main's behavior exactly, including error precedence (a reader-level error such as the depth limit still wins over an earlier conversion error, since the reread reparses from scratch).
  • Direct reads only ever start at StartObject (the reader passed to declared.Read is untouched by the $type lookahead, which operates on a copy), so skipping JsonSerializer's null-handling wrapper around Read is safe here.

Custom variant converters. The gate (typeInfo.Converter.GetType().Assembly == typeof(JsonSerializer).Assembly) correctly restricts the direct-read fast path to System.Text.Json's own converters, which are trusted to read exactly one value; anything else (including the adversarial over-/under-reading converters added in the new tests) goes through JsonSerializer.Deserialize, which enforces that check. This matches the PR's description and the new regression tests.

I did not find a correctness problem in either fix, and the described differential fuzzing (200,000 mutated union documents, 72 nested-depth cases up to 100 levels, 0 divergences from main) is strong corroborating evidence beyond what I can check by inspection alone.

One very minor observation carried over from my last pass: the new CHANGELOG bullet (CHANGELOG.md, the "Faster JSON model (de)serialization..." line) is about 334 characters, still somewhat past CLAUDE.md's "~300 characters" guideline. Not blocking, same as last time.

No new blocking issues found. Approving.

⏱ 1m 34s · 5 turns · tokens: 91.7k in / 9.1k out (+442.6k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. Re-review, focused mainly on the two "review fixes" this round added on top of the design I already approved in my previous pass (the union converter's nested-failure replay and the custom-converter direct-read gate), since those are exactly the kind of subtle concurrency/control-flow bugs that are easy to get wrong. **Nested-union replay (`AtProtoUnionConverter.cs`).** Traced `VariantReader<TVariant>.Read` against `UnionReadState.Mode`: - Nested unions (`mode == Direct`) call `_converter.Read` with no try/catch, so a failure unwinds straight through every nested frame to the single outermost frame — this is what avoids the old exponential (2^depth) replay and the stack overflow it caused. Confirmed the mode is reset to `None` in `finally` on both the direct attempt and the reread, so a thrown exception can never leave the thread-static flag stuck. - The outermost frame's replay parses from a saved copy of the reader (`start`) via `JsonSerializer.Deserialize`, during which nested unions see `mode == Reread` and also go through `JsonSerializer.Deserialize` — matching `main`'s behavior exactly, including error precedence (a reader-level error such as the depth limit still wins over an earlier conversion error, since the reread reparses from scratch). - Direct reads only ever start at `StartObject` (the `reader` passed to `declared.Read` is untouched by the `$type` lookahead, which operates on a copy), so skipping `JsonSerializer`'s null-handling wrapper around `Read` is safe here. **Custom variant converters.** The gate (`typeInfo.Converter.GetType().Assembly == typeof(JsonSerializer).Assembly`) correctly restricts the direct-read fast path to System.Text.Json's own converters, which are trusted to read exactly one value; anything else (including the adversarial over-/under-reading converters added in the new tests) goes through `JsonSerializer.Deserialize`, which enforces that check. This matches the PR's description and the new regression tests. I did not find a correctness problem in either fix, and the described differential fuzzing (200,000 mutated union documents, 72 nested-depth cases up to 100 levels, 0 divergences from `main`) is strong corroborating evidence beyond what I can check by inspection alone. One very minor observation carried over from my last pass: the new CHANGELOG bullet (`CHANGELOG.md`, the "Faster JSON model (de)serialization..." line) is about 334 characters, still somewhat past CLAUDE.md's "~300 characters" guideline. Not blocking, same as last time. No new blocking issues found. Approving. --- ⏱ 1m 34s · 5 turns · tokens: 91.7k in / 9.1k out (+442.6k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-191/run-647.md) <!-- claude-verdict:approve --> <!-- claude-reviewed-head:ee6a1f755ba5f3cd314d187bf6b2fdf14af2e7cf -->
opencode-reviewer approved these changes 2026-09-28 13:12:34 +00:00
Dismissed
opencode-reviewer left a comment
Copy link

✅ Verdict: approve — no blocking issues found.

Re-review of the perf PR (new commits since my last pass). My earlier pass was cut off mid-check on three open threads; I've now finished verifying each against the real code, and all are resolved:

  • FindDiscriminator (AtProtoUnionConverter.cs) — my truncated pass had flagged the TokenType == String check as suspicious on the property-name token. Re-reading the current code: line 254 computes isType on the property name via ValueTextEquals("$type"u8), then line 255's reader.Read() advances the copy onto the value before the String check — so discriminator.ValueSpan/GetString() read the $type value's unquoted unescaped content correctly. False alarm.
  • AtUri.Equals compares only Value ordinally (line 174), so the new lazily-built _parts/offset fields don't affect equality, hashing or ordering; a race building a part twice only produces equal values.
  • Handle's @-stripping is intact on the string path; an @-prefixed value fails the UTF-8 fast path (@ is outside DomainChars) and falls back to the string path, preserving the old behavior (@alice.bsky.social remains parseable as an AtIdentifier, per the fixture test).

Beyond that, I traced the parts that changed since the last look:

  • Union read-mode machine: UnionReadState.Mode is written only inside VariantReader<TVariant>.Read; exactly the outermost union (mode None → sets Direct) wraps its member read in try/catch, while variants read under Direct (line 310) and everything under Reread (line 307) pass exceptions through without catching, so a failed member is read directly once and replayed exactly once regardless of depth. The replay's "deserialize, then failure.Throw() only if the replay didn't throw" structure preserves main's error-choice order (a later reader error beats an earlier conversion error). The replay runs after the catch has exited (post-finally), so the 2^depth stack-growth failure is genuinely gone; the new deep-nesting tests at depth 60 on small stacks cover the regression. Bounded replay: one direct read + one full deserialize, no exponential nesting.
  • IIdentifier<T>.TryCreate(ReadOnlySpan<byte>) default returns false, so SpaceUri (which has no UTF-8 override) stays entirely on the decoded-string path — no behavior change for server space identifiers.
  • UTF-8 fast paths all correctly gate on !ValueIsEscaped && !HasValueSequence and fall back to the string path, which also produces the errors — exception surface unchanged.
  • Jetstream's GetIdentifierOrNull falls back to T.TryCreate(text, text, …) for escaped values; combined with Handle's behavior this keeps @, escapes and normalization identical to the old GetStringOrNull + TryParse flow.
  • Base32Lower/Cid byte-form equality: canonical string form means byte comparison matches string comparison; HashCode.AddBytes keeps per-process seeding for hash-flooding resistance.

The two adversarial-review fixes (nested-union replay and custom-variant-converter confinement) each carry regression tests that pin expected behavior against main's converter, plus the differential fuzzer results — that's as strong as static review gets without running the harness here.

Minor, non-blocking notes (consolidated, per claude-reviewer's pass):

  • The CHANGELOG bullet runs past the ~300-character guideline; cosmetic only.
  • VariantReader's STJ-assembly check (typeInfo.Converter.GetType().Assembly == typeof(JsonSerializer).Assembly) is a heuristic; any future STJ converter split into another assembly silently takes the safe Deserialize path — a perf only footnote, not correctness.

Previous threads: this pass's three open items all resolved as above; claude-reviewer's APPROVED stands with no blockers either. Good to merge.

⏱ 12m 20s · 7 steps · $0.0512 · tokens: 206.1k in / 1.7k out (+156.8k cache-read) · Go: 5h 0% · wk 0% · mo 82%
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. Re-review of the perf PR (new commits since my last pass). My earlier pass was cut off mid-check on three open threads; I've now finished verifying each against the real code, and all are resolved: - **`FindDiscriminator`** (AtProtoUnionConverter.cs) — my truncated pass had flagged the `TokenType == String` check as suspicious on the property-name token. Re-reading the current code: line 254 computes `isType` on the property name via `ValueTextEquals("$type"u8)`, then line 255's `reader.Read()` advances the copy onto the *value* before the `String` check — so `discriminator.ValueSpan`/`GetString()` read the `$type` value's unquoted unescaped content correctly. False alarm. - **`AtUri.Equals`** compares only `Value` ordinally (line 174), so the new lazily-built `_parts`/offset fields don't affect equality, hashing or ordering; a race building a part twice only produces equal values. - **`Handle`'s `@`-stripping** is intact on the string path; an `@`-prefixed value fails the UTF-8 fast path (`@` is outside `DomainChars`) and falls back to the string path, preserving the old behavior (`@alice.bsky.social` remains parseable as an `AtIdentifier`, per the fixture test). Beyond that, I traced the parts that changed since the last look: - **Union read-mode machine**: `UnionReadState.Mode` is written only inside `VariantReader<TVariant>.Read`; exactly the outermost union (mode `None` → sets `Direct`) wraps its member read in try/catch, while variants read under `Direct` (line 310) and everything under `Reread` (line 307) pass exceptions through without catching, so a failed member is read directly once and replayed exactly once regardless of depth. The replay's "deserialize, then `failure.Throw()` only if the replay didn't throw" structure preserves `main`'s error-choice order (a later reader error beats an earlier conversion error). The replay runs after the `catch` has exited (post-finally), so the 2^depth stack-growth failure is genuinely gone; the new deep-nesting tests at depth 60 on small stacks cover the regression. Bounded replay: one direct read + one full deserialize, no exponential nesting. - **`IIdentifier<T>.TryCreate(ReadOnlySpan<byte>)` default** returns false, so `SpaceUri` (which has no UTF-8 override) stays entirely on the decoded-string path — no behavior change for server space identifiers. - **UTF-8 fast paths** all correctly gate on `!ValueIsEscaped && !HasValueSequence` and fall back to the string path, which also produces the errors — exception surface unchanged. - **Jetstream's `GetIdentifierOrNull`** falls back to `T.TryCreate(text, text, …)` for escaped values; combined with `Handle`'s behavior this keeps `@`, escapes and normalization identical to the old `GetStringOrNull` + `TryParse` flow. - **`Base32Lower`/`Cid` byte-form equality**: canonical string form means byte comparison matches string comparison; `HashCode.AddBytes` keeps per-process seeding for hash-flooding resistance. The two adversarial-review fixes (nested-union replay and custom-variant-converter confinement) each carry regression tests that pin expected behavior against `main`'s converter, plus the differential fuzzer results — that's as strong as static review gets without running the harness here. Minor, non-blocking notes (consolidated, per claude-reviewer's pass): - The CHANGELOG bullet runs past the ~300-character guideline; cosmetic only. - `VariantReader`'s STJ-assembly check (`typeInfo.Converter.GetType().Assembly == typeof(JsonSerializer).Assembly`) is a heuristic; any future STJ converter split into another assembly silently takes the safe `Deserialize` path — a perf only footnote, not correctness. Previous threads: this pass's three open items all resolved as above; claude-reviewer's APPROVED stands with no blockers either. Good to merge. --- ⏱ 12m 20s · 7 steps · $0.0512 · tokens: 206.1k in / 1.7k out (+156.8k cache-read) · Go: 5h 0% · wk 0% · mo 82% 📜 [Full session transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-191/run-2026-09-28T13-00-13-403Z.md) <!-- opencode-reviewed-head:ee6a1f755ba5f3cd314d187bf6b2fdf14af2e7cf --> <!-- opencode-verdict:approve -->
fix(serialization): reread a failed union member as Deserialize would, from its start, after review (#190)
All checks were successful
CI / pds-integration (pull_request) Successful in 31s
Details
CI / build-and-test (pull_request) Successful in 1m3s
Details
Sync Closures to GitHub / sync-closure (pull_request) Successful in 8s
Details
ba8049650e 
After a failed direct read, the outermost union replayed its member on a copy
of the reader and rethrew the first error. The caller's reader stayed where
the direct read had stopped, inside the member, whereas JsonSerializer.
Deserialize puts it back at the member's start on a JsonException. A replay
that succeeded was thrown away. A converter that called a union's converter
and skipped an invalid member therefore failed ("read too much or not
enough"), or silently read on from inside the member. A nested union's raw
error could also reach such a converter where main gives it a JsonException.

The outermost union now resets the reader to the member's start and returns
what JsonSerializer.Deserialize returns, or lets its error through. A union
nested in a direct read records its failure in an exception filter, which
leaves the exception alone. The outermost union rereads when a failure was
recorded, even if a converter in between caught it and the direct read then
returned. A member is still read directly once and reread at most once.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Grandiras commented 2026-09-28 13:20:21 +00:00
Author
Owner
Copy link

@claude /review, please re-review. The adversarial re-review verified both earlier fixes and raised one more LOW difference from main, which was already present in 4ecdde4. ba80496 fixes it.

  • Reader position: a failed union read left the reader inside the member. Now the outermost union resets the reader to the member's start and returns JsonSerializer.Deserialize in Reread mode, which gives main's value on success and main's error on failure (failure.Throw() is gone).
  • Swallowed failures: a nested failure that an intermediate converter caught is still recorded, through a non-catching exception filter, so the outermost union re-reads anyway.
  • Bound: one direct read and at most one re-read, at any depth.
  • Tests: 8 lenient-converter cases, which fail at ee6a1f7 and pass with main's converter.
  • Fuzzer: union over 220k documents, uniondos in 72 cases and fix in 832 cases, 0 divergences.
  • Timeline A/B: unchanged.
  • Unit tests: 4,964 pass.
    Please read AtProtoUnionConverter.cs in full.
@claude /review, please re-review. The adversarial re-review verified both earlier fixes and raised one more LOW difference from main, which was already present in 4ecdde4. **ba80496** fixes it. - **Reader position:** a failed union read left the reader inside the member. Now the outermost union resets the reader to the member's start and returns `JsonSerializer.Deserialize` in Reread mode, which gives main's value on success and main's error on failure (`failure.Throw()` is gone). - **Swallowed failures:** a nested failure that an intermediate converter caught is still recorded, through a non-catching exception filter, so the outermost union re-reads anyway. - **Bound:** one direct read and at most one re-read, at any depth. - **Tests:** 8 lenient-converter cases, which fail at ee6a1f7 and pass with main's converter. - **Fuzzer:** `union` over 220k documents, `uniondos` in 72 cases and `fix` in 832 cases, 0 divergences. - **Timeline A/B:** unchanged. - **Unit tests:** 4,964 pass. Please read `AtProtoUnionConverter.cs` in full.
claude-bot commented 2026-09-28 13:20:25 +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/648); 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-191/run-648.md) — updating every ~5 minutes while the task runs.
opencode-bot commented 2026-09-28 13:21:04 +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-191/run-2026-09-28T13-21-03-404Z.md) — updating every ~5 minutes while the review runs.
claude-reviewer approved these changes 2026-09-28 13:21:12 +00:00
claude-reviewer left a comment
Copy link

✅ Verdict: approve — no blocking issues found.

This is my third pass on #191. The diff supplied here matches exactly what I reviewed and approved in my previous pass (2026-09-28T13:01:01Z): the identifier-syntax rewrite, AtUri/Cid laziness, the CAR/base32 changes, the Jetstream UTF-8 lookups, and the three union-converter review fixes (nested-failure replay via the thread-static UnionReadState, the custom-converter direct-read gate, and the reader-reset-on-failure behavior). I re-traced the trickier pieces I hadn't previously called out in detail:

  • JsonElementExtensions.GetIdentifierOrNull<T> and JetstreamEventParser.TryParseTime both use JsonMarshal.GetRawUtf8Value(...)[1..^1] to strip the JSON string's surrounding quotes before checking for \ and taking the fast UTF-8 path. This is safe: GetRawUtf8Value on a JsonValueKind.String element always returns the quoted span, so the slice is correct, and any escape sequence necessarily contains a \ byte, so the IndexOf((byte)'\') < 0 guard correctly routes anything escaped to the decoded-string fallback.
  • AtDatetime.TryParseUtc's fraction-digit count (utf8[19] == '.' ? utf8.Length - 20 - (utf8[^1] == 'Z' ? 1 : 6) : 0) is only reached after TryParseInstant has already validated the full grammar, so the fixed offsets (index 19 for the dot, suffix length 1 for Z or 6 for ±HH:MM) are safe to rely on.
  • The bit-packed AtDatetime._ticksAndState, the AtUri lazy Parts/offset bookkeeping, the Cid inline-array storage and hash/equality-on-bytes, and the grouped base32 decode were already traced in my first pass and are unchanged here.

I found no new correctness issues. Nothing here changes the assessment from my last two passes — approving again. The only previously-noted non-blocking nit (the CHANGELOG bullet running slightly past CLAUDE.md's ~300-character guideline) still stands but was never blocking.

⏱ 0m 30s · 5 turns · tokens: 83.3k in / 2.6k out (+446.2k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. This is my third pass on #191. The diff supplied here matches exactly what I reviewed and approved in my previous pass (2026-09-28T13:01:01Z): the identifier-syntax rewrite, `AtUri`/`Cid` laziness, the CAR/base32 changes, the Jetstream UTF-8 lookups, and the three union-converter review fixes (nested-failure replay via the thread-static `UnionReadState`, the custom-converter direct-read gate, and the reader-reset-on-failure behavior). I re-traced the trickier pieces I hadn't previously called out in detail: - `JsonElementExtensions.GetIdentifierOrNull<T>` and `JetstreamEventParser.TryParseTime` both use `JsonMarshal.GetRawUtf8Value(...)[1..^1]` to strip the JSON string's surrounding quotes before checking for `\` and taking the fast UTF-8 path. This is safe: `GetRawUtf8Value` on a `JsonValueKind.String` element always returns the quoted span, so the slice is correct, and any escape sequence necessarily contains a `\` byte, so the `IndexOf((byte)'\') < 0` guard correctly routes anything escaped to the decoded-string fallback. - `AtDatetime.TryParseUtc`'s fraction-digit count (`utf8[19] == '.' ? utf8.Length - 20 - (utf8[^1] == 'Z' ? 1 : 6) : 0`) is only reached after `TryParseInstant` has already validated the full grammar, so the fixed offsets (index 19 for the dot, suffix length 1 for `Z` or 6 for `±HH:MM`) are safe to rely on. - The bit-packed `AtDatetime._ticksAndState`, the `AtUri` lazy `Parts`/offset bookkeeping, the `Cid` inline-array storage and hash/equality-on-bytes, and the grouped base32 decode were already traced in my first pass and are unchanged here. I found no new correctness issues. Nothing here changes the assessment from my last two passes — approving again. The only previously-noted non-blocking nit (the CHANGELOG bullet running slightly past CLAUDE.md's ~300-character guideline) still stands but was never blocking. --- ⏱ 0m 30s · 5 turns · tokens: 83.3k in / 2.6k out (+446.2k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-191/run-648.md) <!-- claude-verdict:approve --> <!-- claude-reviewed-head:ba8049650e70f0b61da18d75f1b160b371ab2dd4 -->
Grandiras deleted branch issue-190-perf 2026-09-28 13:21:49 +00:00
opencode-reviewer approved these changes 2026-09-28 13:33:52 +00:00
opencode-reviewer left a comment
Copy link

✅ Verdict: approve — no blocking issues found.

Re-review of the new commits since my last pass. All three review fixes are in place and each is the fix I would have demanded:

  • Nested-union replay (critical fix): verified in AtProtoUnionConverter.cs — nested unions (mode Direct/DirectFailed) read via _converter.Read with an exception filter (UnionReadState.RecordFailure) that sets DirectFailed and returns false, so the catch body is never entered at nested level and failures unwind to the single outermost frame. The outermost frame replays only after its finally has reset the mode (fully unwound stack) and inside the replay sets Reread, which forces every nested union through JsonSerializer.Deserialize — one direct read plus one replay per failed member at any depth, matching main's error choice (a later reader error beats an earlier conversion error). Thread-static mode is reset in finally on both the direct and reread paths, so no stale state survives a throw.
  • Custom variant converters: the gate (typeInfo.Converter.GetType().Assembly == typeof(JsonSerializer).Assembly) correctly confines the direct-read fast path to System.Text.Json's own converters; non-STJ converters (including the adversarial over-/under-readers in the new tests) go through JsonSerializer.Deserialize, which enforces the read-exactly-your-value contract, as on main.
  • Reader position after a failed read: the outermost frame resets the reader to the member's start before the reread, and a nested failure a converter caught still triggers it (the outer check is UnionReadState.Mode == Direct vs DirectFailed), so result, error and final reader position all match main.

Carried over, re-verified on the current code: AtUri.Equals compares only Value ordinally so the new lazy Parts/offset fields don't affect equality, hashCode or ordering; Handle's @-stripping is intact (the UTF-8 fast path rejects @ and falls back to the string path that strips it); AtDatetime's bit-packing and offset-from-text reconstruction are safe for every shape TryParseInstant can accept.

The documented residual divergence (a custom converter that performs an unrelated deserialization during a direct read and catches the error runs twice, with main's document result) is an accepted, single-vertex wedge of a much wider parity sweep — 784 nested-union cases, 42 async documents and 200k mutated documents with zero divergences — far sharper than manual verification can reach here.

The only remaining nit is cosmetic: the CHANGELOG bullet runs ~330 characters, past the ~300-character guideline, unchanged since the first pass. Not blocking.

⏱ 12m 49s · 5 steps · $0.0327 · tokens: 118.6k in / 1.3k out (+172.5k cache-read) · Go: 5h 0% · wk 0% · mo 82%
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. Re-review of the new commits since my last pass. All three review fixes are in place and each is the fix I would have demanded: - **Nested-union replay (critical fix)**: verified in `AtProtoUnionConverter.cs` — nested unions (mode `Direct`/`DirectFailed`) read via `_converter.Read` with an exception filter (`UnionReadState.RecordFailure`) that sets `DirectFailed` and returns `false`, so the `catch` body is never entered at nested level and failures unwind to the single outermost frame. The outermost frame replays only after its `finally` has reset the mode (fully unwound stack) and inside the replay sets `Reread`, which forces every nested union through `JsonSerializer.Deserialize` — one direct read plus one replay per failed member at any depth, matching `main`'s error choice (a later reader error beats an earlier conversion error). Thread-static mode is reset in `finally` on both the direct and reread paths, so no stale state survives a throw. - **Custom variant converters**: the gate (`typeInfo.Converter.GetType().Assembly == typeof(JsonSerializer).Assembly`) correctly confines the direct-read fast path to System.Text.Json's own converters; non-STJ converters (including the adversarial over-/under-readers in the new tests) go through `JsonSerializer.Deserialize`, which enforces the read-exactly-your-value contract, as on `main`. - **Reader position after a failed read**: the outermost frame resets the reader to the member's start before the reread, and a nested failure a converter caught still triggers it (the outer check is `UnionReadState.Mode == Direct` vs `DirectFailed`), so result, error and final reader position all match `main`. Carried over, re-verified on the current code: `AtUri.Equals` compares only `Value` ordinally so the new lazy `Parts`/offset fields don't affect equality, hashCode or ordering; `Handle`'s `@`-stripping is intact (the UTF-8 fast path rejects `@` and falls back to the string path that strips it); `AtDatetime`'s bit-packing and offset-from-text reconstruction are safe for every shape `TryParseInstant` can accept. The documented residual divergence (a custom converter that performs an unrelated deserialization during a direct read and catches the error runs twice, with `main`'s document result) is an accepted, single-vertex wedge of a much wider parity sweep — 784 nested-union cases, 42 async documents and 200k mutated documents with zero divergences — far sharper than manual verification can reach here. The only remaining nit is cosmetic: the CHANGELOG bullet runs ~330 characters, past the ~300-character guideline, unchanged since the first pass. Not blocking. --- ⏱ 12m 49s · 5 steps · $0.0327 · tokens: 118.6k in / 1.3k out (+172.5k cache-read) · Go: 5h 0% · wk 0% · mo 82% 📜 [Full session transcript](https://git.grandiras.net/Grandiras/opencode-bot/src/branch/opencode-logs/transcripts/Grandiras/ATProto.NET/pr-191/run-2026-09-28T13-21-03-404Z.md) <!-- opencode-reviewed-head:ba8049650e70f0b61da18d75f1b160b371ab2dd4 --> <!-- opencode-verdict:approve -->
Sign in to join this conversation.
Reviewers
No reviewers
claude-reviewer
opencode-reviewer
Labels
Clear labels   
breaking-change
Introduces a breaking API change

  
bug
Something is not working correctly

  
documentation
Documentation improvements

  
duplicate
This issue or PR already exists

  
enhancement
New feature or improvement

  
good first issue
Good for newcomers to the project

  
help wanted
Extra attention is needed

  
performance
Performance-related issue or improvement

  
question
Further information is requested

  
wontfix
This will not be worked on

No labels
breaking-change
bug
documentation
duplicate
enhancement
good first issue
help wanted
performance
question
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
5 participants
Notifications
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!191
No description provided.