fix: key A2AJsonConverter safe-options cache per JsonSerializerOptions instance - #436
fix: key A2AJsonConverter safe-options cache per JsonSerializerOptions instance#436chopmob-cloud wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the caching mechanism in A2AJsonConverter<T>.GetSafeOptions to use a ConditionalWeakTable instead of a single static field, resolving an issue where using multiple distinct JsonSerializerOptions instances caused an InvalidOperationException. A unit test was also added to verify this behavior. The review feedback suggests calling clone.MakeReadOnly() on the cloned options before caching them to ensure they are frozen and thread-safe.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…s instance (a2aproject#298) A2AJsonConverter<T>.GetSafeOptions cached its converter-stripped options in two static fields, on the assumption that A2A uses a single JsonSerializerOptions instance for the process lifetime. When a second, distinct instance reached it (for example a host layer such as Microsoft.Agents.AI.Hosting that builds its own options from A2AJsonUtilities.DefaultOptions with a rebuilt resolver chain), the guard threw InvalidOperationException. In Release Debug.Fail is compiled out, so the throw ran with its default message "Operation is not valid due to the current state of the object", surfaced by Write as "A2AException: Failed to serialize FileContent: ...". The static fields are per closed generic, so the first instance poisoned the cache for every later one. Replace the single-global-instance cache with a ConditionalWeakTable keyed on the caller JsonSerializerOptions instance, so each instance gets its own converter-stripped clone. The table is reference-keyed (JsonSerializerOptions has no value equality) and its entries are collected together with the caller options, so the cache neither collides across instances nor leaks. The guard and Debug.Fail are removed. Adds tests/A2A.V0_3.UnitTests/GitHubIssues/Issue298.cs, which reproduces the throw on the pre-fix converter and round-trips cleanly with the fix.
6304a22 to
17c4551
Compare
|
Good call, adopted. Freezing the cached clone with |
1 participant
Summary
Fixes #298.
A2AJsonConverter<T>.GetSafeOptionscached its converter-stripped options in twostaticfields, on the documented assumption that A2A uses a singleJsonSerializerOptionsinstance for the process lifetime. When a second, distinct instance reached it, a guard threw. In ReleaseDebug.Failis compiled out, sothrow new InvalidOperationException()ran with its default message "Operation is not valid due to the current state of the object", surfaced byWriteasA2AException: Failed to serialize FileContent: .... Because the fields are per closed generic, the first instance poisoned the cache for every later one.A host layer such as
Microsoft.Agents.AI.Hostingbuilds its own options fromA2AJsonUtilities.DefaultOptionswith a rebuilt resolver chain. That is a second, distinct instance, and it is what tripped the guard in the reported traces (confirmed in the issue thread reproducing on the current preview line via theA2A.V0_3Compatpath).Fix
Replace the single-global-instance cache with a
ConditionalWeakTable<JsonSerializerOptions, JsonSerializerOptions>keyed on the caller options instance, so each instance gets its own converter-stripped clone. The table is reference-keyed (JsonSerializerOptionshas no value equality) and its entries are collected together with the caller options, so the cache neither collides across instances nor leaks. The guard andDebug.Failare removed. No public API changes.Validation
tests/A2A.V0_3.UnitTests/GitHubIssues/Issue298.csreproduces the throw on the pre-fix converter (fails with the exactA2AException) and round-trips cleanly with the fix.A2A.V0_3.UnitTests263/263 on net8.0 and net10.0, andA2A.UnitTests420/420 on net8.0 and net10.0.