[ci-fix] Use DoNotWrapExceptions across ReflectionMemberAccessor Invoke paths

[github-actions]

Workflow artifact: ci-fix
Artifact kind: help
Linked KBE: #128766

> [!NOTE]
> This is an AI/Copilot-generated fix updated after PR feedback and validated with targeted local build/test runs.

Root cause (best analysis)

AsyncEnumerableTests.SerializeAsyncEnumerable_*_PartialItemFailure_PriorItemsAreFullyWritten fails on tvos-arm64 AllSubsets_Mono because the test expects InvalidOperationException but receives TargetInvocationException.

The failing log line:

[FAIL] ...AsyncEnumerableTests_AsyncPipeSerializer.SerializeAsyncEnumerable_TopLevelValues_PartialItemFailure_PriorItemsAreFullyWritten
Expected: typeof(System.InvalidOperationException)
Actual:   typeof(System.Reflection.TargetInvocationException)

On Mono/tvOS (AOT environments), System.Text.Json uses ReflectionMemberAccessor, where reflection Invoke can wrap target exceptions. CoreCLR commonly uses ReflectionEmitMemberAccessor, which propagates underlying exceptions directly.

Attempted fix

Updated ReflectionMemberAccessor to apply BindingFlags.DoNotWrapExceptions on NET targets across relevant reflection Invoke call sites in the file (including property accessors and other Invoke usages raised in review), while preserving non-NET compatibility paths.

Additional polish from feedback:

• Kept exception behavior aligned with ReflectionEmitMemberAccessor for add-method delegate behavior.
• Improved null-handling/diagnostics for union accessor delegate creation.

What is unverified / where I need help

• Full matrix/platform validation (especially tvOS Mono) still depends on CI legs; local validation was targeted.

Validation

• ./build.sh clr+libs -rc release ✅
• ./build.sh clr+libs+host -rc release -lc release ✅
• dotnet build src/libraries/System.Text.Json/src/System.Text.Json.csproj -c Release ✅
• dotnet build /t:test src/libraries/System.Text.Json/tests/System.Text.Json.Tests/System.Text.Json.Tests.csproj -p:Configuration=Release /p:XunitMethodName=System.Text.Json.Serialization.Tests.AsyncEnumerableTests_AsyncPipeSerializer.SerializeAsyncEnumerable_TopLevelValues_PartialItemFailure_PriorItemsAreFullyWritten ✅

Evidence

• Failing build: https://dev.azure.com/dnceng-public/public/_build/results?buildId=1438268
• First build it occurred: build 1433744, finished 2026-05-25T09:02:27Z (computed within scanned window; may not be true origin)
• Suspected regressing change: none identified with sufficient confidence

Help wanted

• Area owners (area-System.Text.Json): @StephenMolloy, @dotnet/area-system-text-json

---

Filed by ci-failure-fix. Comment here or on the workflow file to suggest changes; ci-failure-scan-feedback reads in-scope feedback daily and opens (or updates) a PR with prompt edits.

> [!WARNING]
>

> Generated by CI Outer-Loop Failure Fixer · ● 35M · ◷

[Copilot]

Pull request overview

Updates System.Text.Json’s reflection-based MemberAccessor implementation to avoid TargetInvocationException wrapping when invoking constructors/methods via reflection, aligning exception propagation with the ReflectionEmit-based accessor paths.

Changes:

• Use BindingFlags.DoNotWrapExceptions for reflection Invoke calls when targeting #if NET.
• On non-NET TFMs, unwrap TargetInvocationException.InnerException via ExceptionDispatchInfo to preserve the original exception and stack.
• Apply this across parameterless/parameterized constructors, add-method delegates, and property getter/setter delegates.

Show a summary per file

File	Description
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionMemberAccessor.cs	Adjusts reflection invocation patterns to avoid exception wrapping and preserve original exception behavior.

Copilot's findings

• Files reviewed: 1/1 changed files
• Comments generated: 1

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "c3d9f506f9c16a96845011d9b213ec9e5d243f19",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "24547a76ba95bee359f4c0b58dd98976973aa797",
  "last_reviewed_commit": "c3d9f506f9c16a96845011d9b213ec9e5d243f19",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "24547a76ba95bee359f4c0b58dd98976973aa797",
  "last_recorded_worker_run_id": "29679151104",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "c3d9f506f9c16a96845011d9b213ec9e5d243f19",
      "review_id": 4730524249
    }
  ]
}

[github-actions]

Holistic Review

Motivation: Justified. On Mono/tvOS AOT, System.Text.Json uses ReflectionMemberAccessor, whose MethodBase.Invoke wraps target exceptions in TargetInvocationException, whereas CoreCLR's ReflectionEmitMemberAccessor emits direct IL calls that propagate the original exception. This divergence caused AsyncEnumerableTests.SerializeAsyncEnumerable_*_PartialItemFailure_* to observe TargetInvocationException instead of the expected InvalidOperationException on tvos-arm64 Mono (KBE #128766). The problem is real and the platform-dependent behavior gap is a legitimate correctness issue.

Approach: Sound and consistent with the codebase. On NET, every reflection Invoke call site now passes BindingFlags.DoNotWrapExceptions, which matches the exception semantics of the emit-based accessor (verified: Mono's RuntimeType.Mono.cs honors this flag). The #else (netstandard/netfx) fallbacks replace the previous throw e.InnerException ?? e with ExceptionDispatchInfo.Capture(e.InnerException).Throw(), which is a strict improvement because it preserves the original stack trace rather than resetting it. The when (e.InnerException is not null) guard is correct: if there is no inner exception, the original TargetInvocationException now propagates unchanged instead of being rethrown as itself, which is fine. Void-returning delegates (add-method, setter) correctly omit the throw; // unreachable.

Summary: ⚠️ Needs Human Review. The change is well-scoped, internally consistent, and the exception-propagation logic is correct on both the NET and fallback paths. My only reservation is empirical: the fix's effectiveness on the actual failing platform (tvOS Mono AOT) depends on CI confirmation, since the author states full-matrix validation still relies on CI legs and local validation was targeted. A human should confirm the relevant Mono/tvOS CI leg now passes before merge. No blocking code defects found.

---

Detailed Findings

✅ Correctness — Exception propagation aligned with emit accessor

Applying BindingFlags.DoNotWrapExceptions on NET makes ReflectionMemberAccessor propagate the underlying exception directly, matching ReflectionEmitMemberAccessor's direct-IL behavior. All eight Invoke sites (parameterless/parameterized/single-parameter constructors, add-method delegate, both property getters, property setter) are updated uniformly, so the behavior is now consistent across every member-access path rather than only the constructor sites originally implicated.

✅ Correctness — Fallback path is a strict improvement

The pre-existing constructor fallbacks used throw e.InnerException ?? e, which rethrows the inner exception with a reset stack trace. Switching to ExceptionDispatchInfo.Capture(e.InnerException).Throw() preserves the original stack trace. The newly added when (e.InnerException is not null) filter means a TargetInvocationException with no inner exception now propagates as-is rather than being caught and rethrown — acceptable, since that case is not the scenario being fixed.

✅ Test coverage — Existing regression tests exercise the path

No new test is added, which is appropriate: the failing AsyncEnumerableTests.SerializeAsyncEnumerable_*_PartialItemFailure_PriorItemsAreFullyWritten tests already assert the expected exception type and directly cover the corrected behavior. The value is in those tests now passing under the reflection-based accessor.

⚠️ Verification — Platform validation pending

The author notes local validation was targeted and full tvOS Mono AOT validation depends on CI. Since the entire motivation is a platform-specific behavior gap, confirmation from the relevant Mono/tvOS CI leg (not just CoreCLR, which already used the emit accessor and passed) is the key remaining check before merge.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 77.3 AIC · ⌖ 10.9 AIC · ⊞ 10K