[flaky-ci] Isolate TryFindClass JNI leak checks - #12709
simonrozsival wants to merge 6 commits into
Conversation
Run the process-wide GREF assertions alone in a fresh filtered Android test process and amplify sustained per-call leaks without allowing positive count growth. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
simonrozsival
added
the
flaky-ci
Use only GC and finalizer synchronization before the after-count, and add a retained-global-reference control proving the assertion detects positive GREF growth. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/review |
|
✅ Android PR Reviewer completed successfully! Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "azcliprod.blob.core.windows.net"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Needs Changes
Findings: 0 errors · 0 warnings · 1 suggestion
The dedicated Package Tests APKs 2 lane passed, including the focused three-test JNI-reference category, and the category filtering and cleanup strategy are coherent. I left one inline naming suggestion so the helper accurately communicates its net-growth semantics.
The overall Azure build is still red: Package Tests macOS > APKs 1 and Windows > Build & Smoke Test failed. Azure log access was unavailable in this review environment, so I could not classify those failures and cannot consider the PR merge-ready until CI is green or the failures are confirmed unrelated.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
azcliprod.blob.core.windows.net
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "azcliprod.blob.core.windows.net"See Network Configuration for more information.
Generated by Android PR Reviewer for #12709 · gpt56 · 159.2 AIC · ⌖ 8.88 AIC · ⊞ 25.7K
Comment /review to run again
Rename the helper and retained-reference control so their names reflect warmup and net-growth measurement semantics. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
The dedicated lane, warmup/baseline structure, and retained-GREF control are well targeted. I left one inline suggestion to make the category guard enforce the isolation invariant even when multiple include categories are supplied.
CI is currently red in Azure DevOps build 1587815: the Linux, macOS, and Windows build jobs all failed, while the Java.Interop test jobs passed. The Azure CLI could not initialize in this runner, so I could not reliably classify those build failures further; CI needs to be green before merge.
Generated by Android PR Reviewer for #12709 · gpt56 · 152.1 AIC · ⌖ 21.1 AIC · ⊞ 25.7K
Comment /review to run again
…utf8-leak-isolation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
IncludeCategories parsing doesn’t trim split entries, which can cause category filtering (and the new isolated lane’s intent) to silently misbehave if whitespace appears in the list.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
 |
tests/Mono.Android-Tests/Mono.Android-Tests/Xamarin.Android.RuntimeTests/TestInstrumentation.cs — GetIncludedCategories() splits IncludeCategories without TrimEntries, so a pipeline value… |
What changed in this PR
This PR reduces flaky CI behavior by isolating process-wide JNI global-reference leak checks into a dedicated, category-filtered on-device run, while keeping those checks excluded from ordinary shared-process test runs.
Changes:
- Re-enabled and categorized both
TryFindClassglobal-reference leak checks, replacing exact before/after equality with a 100-iteration “no positive GREF growth” assertion plus a retained-GREF control. - Updated on-device test instrumentation to exclude
JniReferenceLeaktests unless explicitly included viaIncludeCategories. - Added a focused CoreCLR Debug pipeline lane that runs only the categorized leak checks in a fresh instrumentation process.
| File | Description |
|---|---|
| tests/Mono.Android-Tests/Mono.Android-Tests/Xamarin.Android.RuntimeTests/TestInstrumentation.cs | Excludes JniReferenceLeak by default unless included via IncludeCategories, and refactors category parsing helpers. |
| external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniTypeUtf8Test.cs | Reintroduces TryFindClass leak tests under a category with a more robust multi-iteration leak assertion and a control test. |
| build-tools/automation/yaml-templates/stage-package-tests.yaml | Adds a dedicated Debug/CoreCLR lane to run only JniReferenceLeak tests in isolation. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Run the process-wide GREF assertion in a dedicated category-filtered instrumentation process, matching the isolation strategy in #12709. Require no positive net growth after amplification and retain a control that proves the helper detects live global references. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
## Summary - mark the process-wide `JavaSideActivation` GREF check as an explicit `JniReferenceLeak` test - run that category in a dedicated JcwGen instrumentation process, separate from the ordinary 38-test suite - warm and measure 100 Java-side activations while requiring no positive net GREF growth - run destructive peer cleanup only before the measured baseline, then use GC/finalizer synchronization before the final count - add a retained-global-reference control proving the assertion detects a real leak ## Why `BindingTests.JavaSideActivation` compared the process-global JNI GREF count around one activation in a shared device-test process. It failed in builds 1577025, 1578128, 1581740, and 1583153 with deltas of +1, +6, +3, and +4; every failure ended at 101 GREFs from a different lower baseline. The affected jobs passed when rerun, which points to one-time or ambient shared-process reference creation rather than a stable per-call leak. This now follows the isolation strategy from #12709. Both PRs use the `JniReferenceLeak` category, `[Explicit]` default exclusion, a fresh instrumentation process, 100 warmup and measured operations, and a retained-GREF control. This PR needs a separate JcwGen invocation because its Java-side activation fixture and generated binding types exist only in the JcwGen test application. Isolation removes the need for the previous `+10` tolerance. The focused lane allows no positive net GREF growth, while 100 measured activations preserve a strong signal for the known `TypeManager.Activate` regression that leaked three GREFs per activation. ## Validation - isolated Release `JniReferenceLeak` category on API 35 emulator: 2 passed, 0 failed, 0 skipped - repeated isolated category: 10/10 runs passed - ordinary Release JcwGen suite: 38 passed, 0 failed, 2 explicit leak checks skipped - package-test YAML parsed successfully - `git diff --check` Tracks #12704 Addresses #12031
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/review |
|
✅ Android PR Reviewer completed successfully! Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "azcliprod.blob.core.windows.net"See Network Configuration for more information.
|
There was a problem hiding this comment.
✅ LGTM
Findings: 0 errors · 0 warnings · 1 suggestion
The focused CoreCLR Debug lane now runs the three categorized JNI-reference tests in isolation, ordinary Android runs exclude them, and the warmup/baseline/control structure matches the intended sustained-GREF-growth check. The latest dotnet-android build and all surfaced CI lanes are green.
I left one non-blocking inline suggestion to keep the isolation invariant robust if Android MTP later begins forwarding instrumentation include filters.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
azcliprod.blob.core.windows.net
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "azcliprod.blob.core.windows.net"See Network Configuration for more information.
Generated by Android PR Reviewer for #12709 · copilot · gpt56 · 84.9 AIC · ⌖ 11.2 AIC · ⊞ 21K
Comment /review to run again
| } | ||
|
|
||
| // Process-wide reference counts are only stable in the dedicated filtered run. | ||
| if (!IsOnlyIncludedCategory (JniReferenceLeakCategory)) { |
There was a problem hiding this comment.
🤖 💡 Testing — This isolation check considers only IncludeCategories from runtimeconfig, but BuildNUnitFilter() also merges categories from the instrumentation include argument. If Android MTP starts forwarding its execution filter (the limitation discussed above), the effective run could include JniReferenceLeak plus unrelated categories while this guard still removes the exclusion. Could the isolation decision be based on the final merged include set, or on a dedicated isolation switch, so future filter plumbing cannot silently reintroduce the process-wide-count flake?
Rule: Deterministic test isolation
simonrozsival
added
the
ready-to-review
Summary
TryFindClassglobal-reference leak check and categorize both overloadsWhy
GlobalReferenceCountis process-wide on Android. The one-call before/after assertion failed in unrelated PR builds 1574982 and 1576388, where unrelated runtime or test-infrastructure activity could change the count.The new category-isolated lane starts a fresh process and executes only the two
TryFindClasschecks plus their retained-reference control. A 100-call measured batch keeps the assertion sensitive to sustained per-call leaks, while allowing unrelated cleanup to reduce the count and allowing no positive growth.CollectPeers()runs only after warmup and before the baseline; only GC/finalizer synchronization occurs before the after-count, so registered peers retained by the operation cannot be destructively removed from the measurement.Validation
JniReferenceLeakcategory: 3 passedgit diff --checkThe Android lane could not be run locally because this worktree has no local Android SDK build or connected emulator; the new CI flavor provides the on-device validation.
Tracking: #12704
Refs #12031.