[flaky-ci] Stabilize Gradle Facebook device coverage - #12719
simonrozsival wants to merge 4 commits into
Conversation
Retry recognized transient dependency-resolution build failures while keeping deployment and runtime validation single-shot. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
simonrozsival
added
the
flaky-ci
Evaluate XA4236 permanent HTTP failures per artifact block so rejected JAR probes do not mask transient AAR failures. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Reject
Found 1 error, 0 warnings, and 0 suggestions. The bounded retry and unique startup marker are well scoped, and all 44 CI checks pass. However, a failed Maven transfer can leave the final cache path populated, causing the next attempt to reuse a truncated artifact rather than retry the download; the new retry path therefore does not reliably recover one of its target failure modes.
Generated by Android PR Reviewer for #12719 · gpt56 · 285.1 AIC · ⌖ 9.08 AIC · ⊞ 25.7K
Comment /review to run again
Write downloads to temporary files before publishing them so interrupted GradleFBProj retries cannot reuse truncated artifacts. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
⚠️ Needs Changes
Findings: 0 errors · 1 warning · 0 suggestions
The atomic Maven-cache publication and interrupted-transfer coverage address the previously reported cache poisoning well, and the device test now keeps build attempts bounded while preserving separate logs and a single launch. The remaining classifier fallback retries deterministic missing AGP plugin versions whenever the dotnet mirror appears in the diagnostic; it should distinguish that case from a transient mirror failure.
CI: Azure build #1587837 is currently red: Linux and macOS build checks failed, Windows is still running, while the Java.Interop and Android Tools test checks shown so far passed. The failed-job details were unavailable in this run, so I could not establish whether those build failures are related to this change.
Generated by Android PR Reviewer for #12719 · gpt56 · 140.2 AIC · ⌖ 19.1 AIC · ⊞ 25.7K
Comment /review to run again
| return true; | ||
| } | ||
|
|
||
| if (hasAgpPluginResolutionFailure && usesDotNetPublicMaven) { |
There was a problem hiding this comment.
🤖 dotnet-public-maven is treated as transient here, even when Gradle deterministically reports that the requested plugin/version does not exist and no reset, DNS failure, or timeout occurred. The negative plugin test only searches Maven Central, so the same not-found diagnostic with the mirror URL would return true and spend two retries on a permanent failure. Please gate this fallback on a transport signature, or explicitly reject Gradle not-found diagnostics, and add a mirror-specific negative case.
Rule: Retry only recognized transient failures
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
CachedMavenRepository’s post-publish temporary-file deletion can throw and should be made best-effort so cleanup failures don’t surface as artifact resolution failures.
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 |
|---|---|
 |
external/Java.Interop/src/Java.Interop.Tools.Maven/Repositories/CachedMavenRepository.cs — File.Delete (temporary_file) is best-effort cleanup; if it… |
What changed in this PR
This PR targets CI flakiness in the Gradle-based Facebook device integration coverage by adding bounded retry logic for recognized transient dependency-resolution failures, enforcing a unique post-Facebook-SDK logcat success marker, and hardening Maven cache writes against partial downloads.
Changes:
- Add a bounded (max 3) build retry loop to
InstallAndRunTests.GradleFBProj, retrying only when output matches recognized transient Gradle/Maven/SDK resolution failures, while keeping install/start single-shot. - Introduce
TransientBuildFailure.TryGetDependencyResolutionReason()plus focused unit tests to classify transient vs permanent dependency-resolution failures (including XA4236 artifact-block handling). - Update
CachedMavenRepositoryto publish downloads via a temporary file + atomic move, with new tests covering interrupted downloads and a concurrent “publisher” scenario.
| File | Description |
|---|---|
| tests/MSBuildDeviceIntegration/Tests/InstallAndRunTests.cs | Adds retry-on-transient build loop and logcat success marker gating for GradleFBProj device execution. |
| src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Common/TransientBuildFailure.cs | New helper to classify dependency-resolution failures as transient (retryable) vs permanent. |
| src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/AndroidGradleProjectTests.cs | Adds unit tests validating transient/permanent classification behavior. |
| external/Java.Interop/src/Java.Interop.Tools.Maven/Repositories/CachedMavenRepository.cs | Switches Maven cache population to temp-file + move to avoid partial cache entries; handles concurrent completion. |
| external/Java.Interop/tests/Java.Interop.Tools.Maven-Tests/CachedMavenRepositoryTests.cs | Adds regression tests ensuring failed downloads don’t populate cache and validates concurrent publisher behavior. |
Suppressed comments (1)
external/Java.Interop/src/Java.Interop.Tools.Maven/Repositories/CachedMavenRepository.cs:96
⚠️ warning (resource management): Same as the sync path: failing to delete the temporary download file shouldn't fail the overall operation. Consider swallowing IO/Unauthorized exceptions when deleting the temp file after publishing.
File.Delete (temporary_file);
| DeleteTemporaryFileAfterFailure (temporary_file, ex); | ||
| throw; | ||
| } | ||
| File.Delete (temporary_file); |
simonrozsival
added
the
ready-to-review
| } catch (IOException) when (File.Exists (file)) { | ||
| // Another process completed the same artifact download first. | ||
| } |
There was a problem hiding this comment.
Why do we treat this as a success? What if the other process copied the wrong file contents?
| static void PublishTemporaryFile (string temporaryFile, string file) | ||
| { | ||
| try { | ||
| File.Move (temporaryFile, file); |
There was a problem hiding this comment.
Is it possible this could move across volumes? Z:\temp to C:\Users\jon\...
I think that this can fail in that case on Windows.
Copilot thinks maybe the temp file should be in the target directory and use FileMode.CreateNew so it would throw if a file with the same name exists already.
| } catch (Exception cleanupException) when (cleanupException is IOException || cleanupException is UnauthorizedAccessException) { | ||
| failure.Data ["MavenCacheTemporaryFileCleanupException"] = cleanupException; | ||
| } |
There was a problem hiding this comment.
did copilot do this "to avoid empty catch blocks"?
is there a way it could be logged instead?
Summary
InstallAndRunTests.GradleFBProjADB_TARGETto the generated project so build deployment, startup, and logcat use the same deviceRoot cause
All five flaky examples inventoried in #12704 failed before APK installation or application startup while the fixture performed live dependency or SDK resolution. The observed signatures were AGP mirror resolution failures, Azure Artifacts DNS/connection resets,
XA4236downloadingfacebook-bolts, and an Android SDK platform installation timeout. None contained a GradleFBProj package/process startup signature in logcat.The retry classifier rejects explicit HTTP 401/403/404 responses on the required payload and deterministic compiler or Gradle configuration failures.
XA4236Maven diagnostics are evaluated per artifact block so an expected rejected JAR probe does not mask a transient failure downloading the required AAR. A successful build still receives exactly one install/start attempt and must execute through the Facebook binding calls.Testing
Xamarin.Android.Build.Tests: 15 focused transient/permanent classification cases passed, including mixed JAR 401/404 plus AAR DNS/reset and permanent required-AAR regressionsMSBuildDeviceIntegration: CoreCLR Debug, CoreCLR Release, and NativeAOT ReleaseGradleFBProjcases passed on an emulator; unsupported NativeAOT Debug remained skipped