[flaky-ci] Stabilize AndroidMessageHandler cancellation rewind test - #12712
simonrozsival wants to merge 9 commits into
Conversation
Track both server handlers, synchronize cancellation after upload begins, and fully validate the retried request body before responding. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
simonrozsival
added
the
flaky-ci
Replace MemoryStream position inference with an explicit destination-write gate and add focused coverage for cancellation and retry behavior. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Flush the controlled stream's initial destination write before signaling progress and cover buffered destinations that hide unflushed bytes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Use a server-observed body prefix before cancellation, abort the first response instead of waiting for EOF, and read exactly Content-Length on retry. 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.
⚠️ Needs Changes
Findings: 0 errors, 1 warning, 0 suggestions.
The revised synchronization is substantially stronger: cancellation is gated on an actual flushed upload plus a server-validated prefix, retry consumption is exact and byte-validated, and cleanup bounds all outstanding work. The remaining issue is configuration coverage: the cancellation regression is still hosted in an SSL-tagged fixture and is therefore excluded from NativeAOT.
CI build 1587386 is still in progress; the completed CLA and Android Tools/Java.Interop checks are green, with platform builds currently running.
Generated by Android PR Reviewer for #12712 · gpt56 · 132.9 AIC · ⌖ 13.4 AIC · ⊞ 25.7K
Comment /review to run again
Move the cancellation rewind regression and its focused helpers out of the SSL-tagged handler fixture so PublishAot lanes execute them. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/review |
|
✅ Android PR Reviewer completed successfully! Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection results could not be parsed. Review the workflow run logs for details. 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.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection results could not be parsed.
Review the workflow run logs for details.
⚠️ Needs Changes
Findings: 0 errors, 0 warnings, 1 suggestion.
The move into AndroidMessageHandlerCancellationTests fixes the previous NativeAOT coverage problem: PublishAot excludes the old fixture’s SSL category, but not the destination fixture’s categories. The revised synchronization also waits for both client write progress and a server-validated prefix before cancellation, verifies rewind before retry, and bounds cleanup.
Azure build 1587832 is not green: Linux and macOS builds failed, while the Windows build is still running. Detailed Azure timeline/log access is unavailable here because Azure authentication is not configured, so I could not determine whether those failures are related to this PR; CI must be green before LGTM.
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 #12712 · gpt56 · 156.1 AIC · ⌖ 21.9 AIC · ⊞ 25.7K
Comment /review to run again
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The changes are test-only and materially improve determinism; the remaining feedback is minor clarity/maintainability in the new test code.
Review tier: Lite
Findings: 2
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
 |
tests/Mono.Android-Tests/Mono.Android-Tests/Xamarin.Android.Net/AndroidMessageHandlerCancellationTests.Rewind.cs — The timeout-path failure message mentions "after releasing the request body", but this branch never… |
|
tests/Mono.Android-Tests/Mono.Android-Tests/Xamarin.Android.Net/AndroidMessageHandlerCancellationTests.Rewind.cs — cancellationObserved.TrySetResult(true) is called in both the OperationCanceledException catch and… |
What changed in this PR
Moves and rewrites the cancellation/rewind regression test to make it deterministic and runnable under NativeAOT lanes, eliminating a server/client race that was causing flaky failures during retry uploads.
Changes:
- Replaces the previous rewind-after-cancellation test with a gated, seekable stream and a tighter client/server synchronization protocol (including server-side abort on cancellation).
- Adds focused regressions covering buffered destination flushing and
HttpListenerResponse.Abort()behavior for blocked request-body reads. - Wires the new rewind test file into the on-device test project and makes the cancellation fixture
partialto host the new file.
| File | Description |
|---|---|
| tests/Mono.Android-Tests/Mono.Android-Tests/Xamarin.Android.Net/AndroidMessageHandlerTests.cs | Removes the older rewind-after-cancellation test from the SSL-tagged fixture. |
| tests/Mono.Android-Tests/Mono.Android-Tests/Xamarin.Android.Net/AndroidMessageHandlerCancellationTests.Rewind.cs | Adds the new deterministic rewind/cancellation regression plus helper regressions and controlled stream implementations. |
| tests/Mono.Android-Tests/Mono.Android-Tests/Xamarin.Android.Net/AndroidMessageHandlerCancellationTests.cs | Makes the fixture partial so the rewind tests can live in a separate file. |
| tests/Mono.Android-Tests/Mono.Android-Tests/Mono.Android.NET-Tests.csproj | Adds an explicit <Compile Include=...Rewind.cs /> so the moved tests are included in the on-device test assembly. |
Suppressed comments (1)
tests/Mono.Android-Tests/Mono.Android-Tests/Xamarin.Android.Net/AndroidMessageHandlerCancellationTests.Rewind.cs:344
- ReleaseFirstCopy() unconditionally calls firstWriteCompleted.TrySetCanceled(); when the first write already completed successfully this no-ops, but it obscures the intent (only cancel the signal if it hasn't fired). Making it conditional reduces confusion when reasoning about task states in failures.
public void ReleaseFirstCopy ()
{
releaseFirstCopy.TrySetResult (true);
firstWriteCompleted.TrySetCanceled ();
}
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Summary
AndroidMessageHandlerCancellationTestsfixture so NativeAOT includes themCopyToAsynconly after its first destination write is flushedOperationCanceledExceptionwhile that content copy is explicitly blockedHttpListenerResponseafter client cancellation instead of draining an incomplete fixed-length body to EOFContent-Length, byte-validate the reused body, then respond without an additional EOF-dependent readHttpListenerrequest-body readFailure analysis
In Azure DevOps build 1584741, the stream-position assertion had already passed. The reported
Broken pipeoccurred on the secondSendAsyncbecause the retry server closed its response without consuming the 1 MB upload. This change removes that server/client race rather than changingAndroidMessageHandlerproduct behavior.Private review identified two synchronization pitfalls in earlier revisions:
ByteArrayContent.ReadAsStreamAsync()returnsMemoryStream, whose optimizedCopyToAsynccan advancePositiontoLengthbefore its destination write completes. The test now uses an explicit copy gate instead of inferring active upload state fromPosition.HttpURLConnection/Okio can buffer the initial 4096-byte destination write. The controlled stream now awaits cancellation-awareFlushAsyncbefore signaling progress, ensuring headers/body can reach the server while further copying remains blocked.Azure DevOps build 1587086 then failed identically in Debug, Release, NoAab, and CoreCLRTrimmable because the first server handler drained the canceled fixed-length request to EOF. Managed
HttpListeneronly reports body EOF after the declaredContent-Lengthis consumed, and client cancellation does not guarantee an immediate FIN/RST, so itsInputStream.ReadAsynccan remain pending indefinitely. The server now validates one bounded body prefix, waits for the client to observe cancellation, and callsHttpListenerResponse.Abort()to terminate the context without draining.The tests were also moved out of the
SSL-taggedAndroidMessageHandlerTestsfixture.TestInstrumentationexcludesSSLwhenPublishAotis enabled, while the destination fixture'sAndroidMessageHandlerCancellationandInetAccesscategories are not excluded, so the regression now runs in NativeAOT.References #12704.
Validation
git diff --checkAndroidMessageHandlerCancellationTests.Rewind.csTestInstrumentationexcludesSSL, notAndroidMessageHandlerCancellationorInetAccess, forPublishAot./dotnet-local.sh build -c Debug tests/Mono.Android-Tests/Mono.Android-Tests/Mono.Android.NET-Tests.csproj; blocked byNETSDK1147because this worktree's local SDK has noandroidworkload installedANDROID_HOME,adb, and an Android emulator/device are not configured on this machine