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
| if (!superseded) | ||
| { | ||
| this.identityToDispose = newIdentity; | ||
|
|
||
| if (isEcc) | ||
| { | ||
| this.identityIsEcc = true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (superseded) | ||
| { | ||
| // A setup the timeout abandoned finished after a later attempt had already started. Publishing now | ||
| // would give this instance an identity the running attempt is not using, and lose the one it is. | ||
| disposeSupersededIdentity(newIdentity, isEcc); | ||
| return; | ||
| } | ||
|
|
||
| // Published for the test body. Never cleared, so a thread the JUnit timeout abandoned can keep reading it. | ||
| this.identity = newIdentity; | ||
| } |
| if (isEcc) | ||
| { | ||
| removeEccDevice(supersededIdentity.getDeviceId()); | ||
| } | ||
| else | ||
| { | ||
| Tools.disposeTestIdentity(supersededIdentity, iotHubConnectionString); | ||
| } |
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.