Fix rerun poisoning in ConnectionTests, and move the test template off a retired preview API version - #1869
Conversation
Every pipeline has been failing in deployCloudTestResources since
2026-09-01, on main as well as on pull requests, before any test runs:
ERROR: {"status":"Failed","error":{"code":"DeploymentFailed",
"details":[{"code":"BadRequest", ...
e2eTestsSetup.ps1:89 throw "Error running resource group deployment."
Nothing under vsts/E2ETestsSetup had changed since May, so the trigger is
service side. The template pinned Microsoft.Devices/IotHubs to
2021-03-03-preview; preview API versions are retired on a much shorter
horizon than GA ones, and a retired version fails exactly this way. Every
other resource in the template is already on a GA version.
Moved to 2023-06-30, the newest GA version. The properties the template
sets - eventHubEndpoints, cloudToDevice, messagingEndpoints,
StorageEndpoints, enableFileUploadNotifications, sku - are all GA stable
and unchanged.
test-resources.json is regenerated from the bicep, so it also picks up
EnableIotHubSecuritySolution, a parameter the bicep has declared since
January that the committed JSON was missing. It is never referenced, and
it defaults to false, so it changes nothing at deployment. Resource list
and outputs are otherwise identical.
There was a problem hiding this comment.
🟡 Changes recommended
The IoT Hub resource still uses StorageEndpoints casing, which does not match the documented storageEndpoints schema for Microsoft.Devices/IotHubs@2023-06-30 and may cause deployment/configuration failures under the new GA API.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates the Azure test-resource deployment templates used by deployCloudTestResources to stop using a retired preview IoT Hub API version, unblocking CI resource-group deployments for the Java SDK’s E2E test infrastructure.
Changes:
- Update
Microsoft.Devices/IotHubsfrom2021-03-03-previewto GA2023-06-30in the Bicep source template. - Regenerate the ARM JSON (
test-resources.json) from Bicep, updating generator metadata and syncing parameters (includingEnableIotHubSecuritySolutionwith a default offalse).
File summaries
| File | Description |
|---|---|
| vsts/E2ETestsSetup/test-resources.bicep | Updates IoT Hub resource API version to GA 2023-06-30. |
| vsts/E2ETestsSetup/test-resources.json | Regenerated ARM template reflecting the new IoT Hub apiVersion and synced parameter metadata. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The template wrote StorageEndpoints. ARM matches property names case insensitively, which is why file upload has been configured correctly all along, but the documented schema is storageEndpoints and the bicep type checker is case sensitive, so the wrong casing produced a BCP089 warning saying the property is not allowed. Emitted template is byte identical apart from that one key, so this changes no deployed configuration. It clears both BCP089 warnings, and it means the template no longer relies on case insensitive matching. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Correction to post on PR 1869 once GitHub access is restoredMy earlier claim was wrongI said the retired-preview-API diagnosis was "confirmed". It is not. Correcting. Deploy outcomes for
163159 and 163283 are the problem for my theory: The inner The clean control is the next What this PR is actually worthJudge it as hygiene, not as an outage fix:
Verified inert: emitted template diffed before/after is identical apart from the I have no Azure access here, so the deployment itself is exercised only by the gate. The Linux failure on this PR163194 failed one test, This PR changes 2 ARM template files and no Java, gradle, pom, PowerShell, YAML or shell. It cannot affect a Java test. That test is the dominant failure on |
Why Linux JDK 21 failed hereI called this "a known flake" without reading it. That was lazy. It has a specific cause, and it is not random. Line 277 on The variant is It is the proxied variants, every time
Why this branch is exposedThis PR branches from
Both are exactly what #1863 fixes: on that branch So this is not this PR's doing - it changes 2 ARM template files, no Java, gradle, pom, PowerShell, YAML or shell - and it is not unexplained. It is a real defect in the test harness that is fixed in #1863 and not yet merged. Ordering note: #1863 is currently blocked behind this PR, since its Windows and Android jobs were failing in resource deployment before reaching any test. Whether that deployment failure was actually fixed by this change is still unproven - see my previous comment; |
The Linux failure here is not stale, but it is the only thing redChecks on this PR point at exactly three builds, all from 09-02 13:15 on
Within 163194, The premise for this PR is now disprovenI said earlier this was unproven. It is now settled, against it. PR #1863 was re-run on the same commit that had failed in resource deployment, with no change of any kind, and So the deployment outage was transient and cleared on its own. This change did not fix it. What this PR is still worthHygiene, and it should be judged only as that:
Verified inert: emitted template is identical apart from the Reasonable to close this if the drift and the preview pin are not considered worth carrying on their own. |
There was a problem hiding this comment.
🟢 Approval recommended
The changes are low-risk template hygiene (GA apiVersion + casing + regeneration) and do not introduce functional logic changes that would block merging.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
EnableIotHubSecuritySolution has been declared in the bicep since January and is referenced nowhere: not by another parameter, not by a resource, not by an output, and not by e2eTestsSetup.ps1, which passes only StorageAccountName and HubUnitsCount. No security solution resource has ever existed in this template, so it was never wired to anything. It stayed invisible because the committed JSON had drifted from the bicep and did not carry it. Regenerating the JSON in this change would have published a dead parameter to callers, so it is removed at the source instead. The emitted template now has the same seven parameters as before, so this removes the only parameter that regenerating would otherwise have added. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The Linux JDK 21 failure is not from this PR, and it is a regression in
|
…open RerunFailedTestRule reuses the same test instance for every attempt at a test, so "teardown has run" could not be a plain flag on that instance. It was. Once the first attempt's @after had run, every later attempt called setup(), published its fresh client, and disposeIfTeardownAlreadyRan immediately closed it. open() then failed with "Client was closed while attempting to open the connection", turning one flaky timeout into a guaranteed failure of all three attempts. Build 163484 shows it: run 3 acquired its device at 03:39:27.203 and logged "Closing device client" at 03:39:27.768, half a second later and before any connect. Each setup now takes a generation number and teardown records the generation it covered, so a setup only cleans up after itself when teardown has run for its own generation. beginSetup also reclaims any residue from a previous attempt first, which is normally nothing. The same generation check makes a superseded setup safe: a setup abandoned by the timeout that finishes after a later attempt has started now reclaims what it produced instead of publishing it over the identity the running attempt is using. Also logs around identity acquisition and around open. The stalls seen so far go silent after "Acquiring test device", which does not distinguish a setup stuck getting an identity from a connect that never completes. These three lines separate the two. Verified the lifecycle across the interleavings: three reruns, teardown mid setup, the ECC variant of that, a superseded setup overlapping a later attempt, normal ECC ordering, and repeated dispose. The rerun cases fail against the previous model and pass against this one. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Ewerton Scaboro da Silva (ewertons)
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
The updated ConnectionTests lifecycle code still has concurrency/cleanup edge cases (atomic publication under lock and avoiding double ECC deletion) that should be addressed to prevent flaky behavior and misleading CI errors.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
…devices twice Two follow ups on the generation scoped lifecycle. trackForCleanup set identityToDispose under lifecycleLock but assigned the public identity field after releasing it. dispose() is explicitly allowed to run concurrently with setup, so it could interleave between the two writes and claim the identity before the test body could see it. Both writes now happen under the same lock, so what teardown owns and what the test body sees change together. disposeSupersededIdentity deleted the ECC device as well as closing the client. That device is already owned elsewhere: setupEccDevice always registers it and calls trackEccDeviceForCleanup before building the identity, so either that call removed it directly when it was superseded at registration, or the dispose that superseded the attempt removed it. Deleting again logged a not found error over a cleanup that had already succeeded. The superseded path now only reclaims the client. Verified both against the interleavings, including an ECC attempt superseded after registration and one superseded at registration. The double delete reproduces against the previous code and does not against this one. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The new logging localises the stall: it is
|
| marker | count |
|---|---|
Acquiring test identity |
6 |
Test identity acquired |
7 |
Opening client |
8 |
Client opened |
4 |
Every acquisition completes, consistently in about 0.53s. Half the opens never complete. So the stall is inside client.open(true), not in Tools.getTestDevice and not in the shared queue lock. That was the question the previous logs could not answer.
It is not random
Linux JDK 21 is the failing job in every build where this has appeared - 163073, 163194, 163484, 163502 - while Linux JDK 8, 11 and 17 passed in all four, and Windows JDK 21 passed. Four for four on one JDK, on one platform, is not flakiness.
That reframes it. This is not general proxy starvation, which would not respect a JDK boundary. Something about the MQTT-over-WebSocket connect through the local proxy behaves differently on JDK 21 on Linux. The most likely candidates are the TLS defaults that changed after JDK 8 and the WebSocket upgrade path, but I have not proven either and will not guess further without evidence.
Worth noting CanOpenConnection carries no @FlakeyTest, so this runs in gate builds, and Linux JDK 21 is a required check.
Two review comments addressed
Both were correct and are fixed in 79785f079:
trackForCleanuppublishedidentityoutsidelifecycleLockwhile settingidentityToDisposeinside it. Both writes now happen under the lock.disposeSupersededIdentitydeleted the ECC device thattrackEccDeviceForCleanupor the supersedingdispose()had already removed, logging a not-found error over a cleanup that had succeeded. The superseded path now reclaims only the client.
Correction: this is not JDK 21 onlyMy previous comment said "four for four on one JDK, on one platform, is not flakiness". That was wrong, and the sample was selection biased - the four builds I cited were the ones I had been investigating, which were the JDK 21 failures. Across the last 25 completed
Every JDK fails sometimes. 14 of those 25 builds had at least one JDK job fail. What survives is a skew, not exclusivity. JDK 11, 17 and 21 run the identical 499 test set, and on that like for like comparison JDK 21 fails 9 times against 3 and 1. JDK 8 is not comparable, since it runs the full 2274 test suite including unit tests. So JDK 21 is disproportionately affected and worth investigating first, but any theory that requires it to be JDK 21 exclusive is already contradicted. I am not treating "it must be a JDK 21 behaviour change" as established. What does still hold, from the logging in
So the stall is inside |
Local reproduction attempt: the proxy plus TLS path is not itTried to reproduce without a hub, by driving the SDK's own proxied TLS path the way this class does: an embedded proxyee 1.7.6 proxy in-process, a local TLS server, clients built through the SDK's
Repeated pinned to 2 CPUs, matching the hosted agent, with 12 concurrent threads and 360 handshakes: still 0 stalls, max 664ms, on both 8 and 21. Negative result, and a useful one. The proxy plus TLS path alone does not reproduce this under any load I can generate, and JDK 21 was marginally the fastest. Proxy starvation as a standalone explanation is not supported by this. What the harness did not cover, and what the investigation moves to next:
This does not block this PR. Its own changes are the template hygiene and the rerun poisoning fix, and the rerun fix is confirmed working in 163502: the failure is now a clean 120s timeout instead of three attempts poisoned into immediate failure. |
Two things: a fix for a test harness regression currently on
main, and hygiene on the e2e test resource template. The template part does not fix an outage - see below.Test harness fix
RerunFailedTestRulereuses one test instance for every attempt, so the "teardown has run" flag added in #1863 stayed set across reruns. Every attempt after the first published its fresh client and had it closed immediately, andopen()failed withClient was closed while attempting to open the connection. One flaky timeout became a guaranteed failure of all three attempts, which is worse than before #1863.Build 163484: run 3 acquired its device at 03:39:27.203 and logged "Closing device client" at 03:39:27.768, half a second later and before any connect.
Each setup now takes a generation number and teardown records the generation it covered, so an earlier attempt's teardown cannot reach a later attempt. The same check makes a superseded setup reclaim what it produced rather than publish it over the identity the running attempt is using.
Also adds logging around identity acquisition and around
open. The stalls go silent after "Acquiring test device", which does not separate a setup stuck getting an identity from a connect that never completes.Carried here rather than in its own PR because this PR cannot go green without it: the regression is on
mainand lands in this PR's merge commit.Verified across the interleavings - three reruns, teardown mid setup, the ECC variant of that, a superseded setup overlapping a later attempt, normal ECC ordering, and repeated dispose. The rerun cases fail against the previous model and pass against this one.
ARM template hygiene
What changed
Microsoft.Devices/IotHubsmoves from2021-03-03-previewto GA2023-06-30. It was the only preview pin in the template; every other resource was already on a GA version. Preview API versions are retired on a much shorter horizon than GA.StorageEndpoints->storageEndpoints, the documented casing. ARM matches property names case-insensitively, so file upload has always been configured correctly; the wrong casing was a schema warning, not a defect.test-resources.jsonis regenerated from the bicep. That surfacedEnableIotHubSecuritySolution, a parameter the bicep had declared since January that the committed JSON was missing, so the two had drifted. It is referenced by nothing - no resource, no output, and not by the deploy script - and no security solution resource has ever existed here, so it is removed at the source rather than published. The emitted template therefore has the same seven parameters asmain.Correcting the original premise
This PR was opened believing the retired preview API version caused the
deployCloudTestResourcesfailures on 09-01. That was wrong.maindeployed successfully at 163159 and 163283 without this change, and a re-run of another PR on the exact commit that had failed deployed fine with no change at all. The outage was transient and cleared on its own. A retired API version cannot intermittently succeed.Judge this on the three items above, not as a fix for that.
Verification
bicep buildsucceeds; regenerating the JSON from the bicep reproduces the committed file byte for byte.tierread-only), identical to whatmain's template produces with the same tool. The 2x BCP089 that the old casing produced are gone.apiVersionstring, that one key's casing, and generator metadata differ. Resource list,skuand outputs unchanged.No Azure subscription access here, so the deployment itself is exercised by the gate rather than locally.
Note on the earlier red check
The Linux failure on this branch was
CanOpenConnection[MQTT_WS_SAS_DEVICE_CLIENT_true_true], which had nothing to do with these two files. That test bounded itself with the same 60s the client gives one MQTT CONNECT round trip, so a stalled attempt and the test expired together. Fixed in #1863 and now onmain, which this branch has been updated with.