Skip to content

Fix rerun poisoning in ConnectionTests, and move the test template off a retired preview API version #1869

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Ewerton Scaboro da Silva (ewertons) merged 6 commits into from
Sep 8, 2026
Merged

Fix rerun poisoning in ConnectionTests, and move the test template off a retired preview API version #1869

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cfd15fb
Move the IoT hub in the test template off a retired preview API version
Copilot Sep 2, 2026
81c8157
Use the documented storageEndpoints casing on the IoT hub
Copilot Sep 2, 2026
6767c6f
Merge remote-tracking branch 'origin/main' into fix-iothub-arm-apiver…
Copilot Sep 4, 2026
e1b5127
Drop the unused security solution parameter from the test template
Copilot Sep 5, 2026
dc3f070
Scope the test instance lifecycle to one attempt, and log around the …
Copilot Sep 5, 2026
79785f0
Publish the identity under the lifecycle lock, and stop deleting ECC …
Copilot Sep 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
214 changes: 175 additions & 39 deletions ...java/tests/integration/com/microsoft/azure/sdk/iot/iothub/connection/ConnectionTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,14 +113,26 @@ public class ConnectionTestInstance
// same client twice and, worse, requeue the same identity into the shared pool twice.
private TestIdentity identityToDispose;

// Set once setupEccDevice starts creating an identity, and never cleared. Identity classification has to
// outlive eccDeviceIdToDelete: teardown claims and clears that id, so if teardown runs between the device
// being registered and the identity being published, the late disposeIfTeardownAlreadyRan would see an
// identity with no ecc id and hand a self signed identity to the shared x509 pool.
private volatile boolean identityIsEcc;

// Set once teardown has run. Guarded by lifecycleLock along with the two fields above.
private boolean disposed;
// Set once setupEccDevice starts creating an identity, and cleared only when a new attempt begins.
// Identity classification has to outlive eccDeviceIdToDelete: teardown claims and clears that id, so if
// teardown runs between the device being registered and the identity being published, the late
// disposeIfTeardownAlreadyRan would see an identity with no ecc id and hand a self signed identity to the
// shared x509 pool.
private boolean identityIsEcc;

// Which setup attempt this instance is currently serving, and how far teardown has run.
//
// RerunFailedTestRule reuses this instance for every attempt at a test, so lifecycle state cannot be a plain
// "teardown has happened" flag. It was, and the result was that once the first attempt's @After had run,
// every later attempt disposed its own freshly acquired client the moment setup published it, and open()
// then failed with "Client was closed while attempting to open the connection". One flaky timeout became a
// guaranteed failure of every remaining attempt.
//
// Each setup takes a generation number, and teardown records the generation it covered. A setup only cleans
// up after itself if teardown has already run for its own generation, so an earlier attempt's teardown can
// no longer reach into a later attempt's.
private long setupGeneration;
private long disposedThrough;

private final Object lifecycleLock = new Object();
public AuthenticationType authenticationType;
Expand Down Expand Up @@ -166,26 +178,37 @@ private void applyProxySettings(ClientOptions.ClientOptionsBuilder optionsBuilde

public void setup() throws Exception
{
long generation = beginSetup();

ClientOptions.ClientOptionsBuilder optionsBuilder = ClientOptions.builder();
applyProxySettings(optionsBuilder);

log.info("Acquiring test identity");

if (clientType == ClientType.DEVICE_CLIENT)
{
trackForCleanup(Tools.getTestDevice(iotHubConnectionString, this.protocol, this.authenticationType, false, optionsBuilder));
trackForCleanup(generation, Tools.getTestDevice(iotHubConnectionString, this.protocol, this.authenticationType, false, optionsBuilder), false);
}
else if (clientType == ClientType.MODULE_CLIENT)
{
trackForCleanup(Tools.getTestModule(iotHubConnectionString, this.protocol, this.authenticationType , false, optionsBuilder));
trackForCleanup(generation, Tools.getTestModule(iotHubConnectionString, this.protocol, this.authenticationType , false, optionsBuilder), false);
}

disposeIfTeardownAlreadyRan();
log.info("Test identity acquired");

disposeIfTeardownAlreadyRan(generation);
}

public void setupEccDevice() throws Exception
{
long generation = beginSetup();

// Marked before anything is created, so that every identity this method goes on to publish is classified
// as ECC even if teardown runs partway through.
this.identityIsEcc = true;
synchronized (lifecycleLock)
{
this.identityIsEcc = true;
}

ClientOptions.ClientOptionsBuilder optionsBuilder = ClientOptions.builder();
applyProxySettings(optionsBuilder);
Expand All @@ -200,12 +223,12 @@ public void setupEccDevice() throws Exception
eccDevice.setThumbprint(certificateGenerator.getX509Thumbprint(), certificateGenerator.getX509Thumbprint());

Tools.addDeviceWithRetry(new RegistryClient(iotHubConnectionString), eccDevice);
trackEccDeviceForCleanup(eccDevice.getDeviceId());
trackEccDeviceForCleanup(generation, eccDevice.getDeviceId());

String deviceConnectionString = Tools.getDeviceConnectionString(iotHubConnectionString, eccDevice);
trackForCleanup(new TestDeviceIdentity(
trackForCleanup(generation, new TestDeviceIdentity(
new DeviceClient(deviceConnectionString, testInstance.protocol, optionsBuilder.build()),
eccDevice));
eccDevice), true);
}
else if (clientType == ClientType.MODULE_CLIENT)
{
Expand All @@ -215,52 +238,151 @@ else if (clientType == ClientType.MODULE_CLIENT)
eccModule.setThumbprint(certificateGenerator.getX509Thumbprint(), certificateGenerator.getX509Thumbprint());

Tools.addDeviceWithRetry(new RegistryClient(iotHubConnectionString), eccDevice);
trackEccDeviceForCleanup(eccDevice.getDeviceId());
trackEccDeviceForCleanup(generation, eccDevice.getDeviceId());

Tools.addModuleWithRetry(new RegistryClient(iotHubConnectionString), eccModule);

String moduleConnectionString = Tools.getDeviceConnectionString(iotHubConnectionString, eccDevice) + ";ModuleId=" + eccModule.getId();
trackForCleanup(new TestModuleIdentity(
trackForCleanup(generation, new TestModuleIdentity(
new ModuleClient(moduleConnectionString, testInstance.protocol, optionsBuilder.build()),
eccDevice,
eccModule));
eccModule), true);
}

disposeIfTeardownAlreadyRan();
disposeIfTeardownAlreadyRan(generation);
}

/**
* Begin a new setup attempt and take its generation number.
*
* <p>Reclaims anything a previous attempt left behind first. Normally there is nothing: that attempt's
* {@code @After} already claimed and cleared what it owned. Anything still present is residue from a setup
* abandoned by the timeout, and disposing it here is the last chance to reclaim it.</p>
*
* @return The generation number this setup attempt owns
*/
private long beginSetup()
{
dispose();

synchronized (lifecycleLock)
{
return ++this.setupGeneration;
}
}

/**
* Hand an identity to teardown, and publish it for the test body to use.
*
* @param generation The generation of the setup attempt that produced this identity
* @param newIdentity The identity this test just acquired or created
* @param isEcc Whether this identity was created by setupEccDevice
*/
private void trackForCleanup(TestIdentity newIdentity)
private void trackForCleanup(long generation, TestIdentity newIdentity, boolean isEcc)
{
// Published for the test body. Never cleared, so a thread the JUnit timeout abandoned can keep reading it.
this.identity = newIdentity;

boolean superseded;
synchronized (lifecycleLock)
{
this.identityToDispose = newIdentity;
superseded = generation != this.setupGeneration;

if (!superseded)
{
this.identityToDispose = newIdentity;

if (isEcc)
{
this.identityIsEcc = true;
}

// Published for the test body under the same lock, so that what teardown owns and what the test
// body can see change together. Never cleared, so a thread the JUnit timeout abandoned can keep
// reading it.
this.identity = newIdentity;
}
}

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);
}
}

/**
* Hand a freshly registered ECC device to teardown, so it is removed from the registry even if the rest of
* setupEccDevice never completes.
*
* @param generation The generation of the setup attempt that registered this device
* @param deviceId The device id that was just added to the registry
*/
private void trackEccDeviceForCleanup(String deviceId)
private void trackEccDeviceForCleanup(long generation, String deviceId)
{
boolean superseded;
synchronized (lifecycleLock)
{
this.eccDeviceIdToDelete = deviceId;
superseded = generation != this.setupGeneration;

if (!superseded)
{
this.eccDeviceIdToDelete = deviceId;
}
}

if (superseded)
{
removeEccDevice(deviceId);
}
}

/**
* Dispose anything registered after teardown already ran.
* Close and discard an identity produced by a setup attempt that has already been superseded.
*
* @param supersededIdentity The identity to reclaim
* @param isEcc Whether it was created by setupEccDevice, and so must never be recycled
*/
private void disposeSupersededIdentity(TestIdentity supersededIdentity, boolean isEcc)
{
log.debug("Reclaiming identity {} from a superseded setup attempt", supersededIdentity.getDeviceId());

if (supersededIdentity.getClient() != null)
{
supersededIdentity.getClient().close();
}

if (isEcc)
{
// Only the client is reclaimed here. setupEccDevice always registers the device and calls
// trackEccDeviceForCleanup before it builds the identity, so the device is already owned by whichever
// path saw it first: trackEccDeviceForCleanup removed it directly if it was superseded at
// registration, and otherwise the dispose that superseded this attempt removed it. Deleting it again
// would just log a not found error over a cleanup that had already worked.
return;
}

Tools.disposeTestIdentity(supersededIdentity, iotHubConnectionString);
}

/**
* Remove an ECC device from the registry. These are never recycled: they are self signed with a certificate
* no other test knows about, so returning one to the shared x509 pool would fail a later test.
*
* @param deviceId The device to remove
*/
private void removeEccDevice(String deviceId)
{
try
{
Tools.getRegistyManager(iotHubConnectionString).removeDevice(deviceId);
}
catch (IOException | IotHubException e)
{
log.error("Failed to clean up ECC test device {}", deviceId, e);
}
}

/**
* Dispose anything registered after teardown already ran for this setup's generation.
*
* <p>Every test in this class is bounded by a timeout, the two minute one that {@link IntegrationTest}
* applies. JUnit runs the test body on a
Expand All @@ -269,15 +391,21 @@ private void trackEccDeviceForCleanup(String deviceId)
* acquiring its identity. Without this, the identity that setup goes on to produce would have no owner and
* would leak, which is precisely the leak this class is trying to stop.</p>
*
* <p>The generation matters. A rerun reuses this instance, so an unqualified "teardown has run" would still
* be set when the next attempt started, and that attempt would dispose its own client the moment it
* published it.</p>
*
* <p>This does not wait for setup, in either direction. Blocking teardown on a setup that is itself hung -
* which is how these tests have actually timed out - would stall the rest of the run.</p>
*
* @param generation The generation of the setup attempt that is finishing
*/
private void disposeIfTeardownAlreadyRan()
private void disposeIfTeardownAlreadyRan(long generation)
{
boolean teardownAlreadyRan;
synchronized (lifecycleLock)
{
teardownAlreadyRan = this.disposed;
teardownAlreadyRan = this.disposedThrough >= generation;
}

if (teardownAlreadyRan)
Expand All @@ -296,16 +424,21 @@ public void dispose()
{
TestIdentity identityToClean;
String eccDeviceIdToClean;
boolean wasEcc;

synchronized (lifecycleLock)
{
this.disposed = true;
// Teardown covers every setup that has begun so far, and nothing later. A setup that starts after
// this takes a higher generation and is unaffected.
this.disposedThrough = this.setupGeneration;

identityToClean = this.identityToDispose;
eccDeviceIdToClean = this.eccDeviceIdToDelete;
wasEcc = this.identityIsEcc;

this.identityToDispose = null;
this.eccDeviceIdToDelete = null;
this.identityIsEcc = false;
}

if (identityToClean != null && identityToClean.getClient() != null)
Expand All @@ -319,16 +452,9 @@ public void dispose()
// the next test that takes an x509 identity from the shared pool, so delete it instead. This runs even
// when the identity was never finished being built, because the device is in the registry from the
// moment it is registered, whether or not the rest of the setup succeeded.
try
{
Tools.getRegistyManager(iotHubConnectionString).removeDevice(eccDeviceIdToClean);
}
catch (IOException | IotHubException e)
{
log.error("Failed to clean up ECC test device {}", eccDeviceIdToClean, e);
}
removeEccDevice(eccDeviceIdToClean);
}
else if (identityToClean != null && this.identityIsEcc)
else if (identityToClean != null && wasEcc)
{
// An earlier dispose already claimed and deleted the device id, and this identity was published after
// that. The device is gone from the registry, so there is nothing left to delete, but it must still
Expand Down Expand Up @@ -450,7 +576,12 @@ public void CanOpenConnection() throws Exception
InternalClient client = testInstance.identity.getClient();

logConnectionStatusChanges(client);

// Bracketing the open so a timeout can be attributed. Silence after "Acquiring test identity" and before this
// line means setup was stuck getting an identity; silence after this line means the connect itself stalled.
log.info("Opening client");
client.open(true);
log.info("Client opened");

// deviceClient.open() is a no-op on HTTP, so a message needs to be sent to actually test opening the connection
if (testInstance.protocol == HTTPS)
Expand Down Expand Up @@ -482,7 +613,12 @@ public void CanOpenConnectionWithECCCertificates() throws Exception
InternalClient client = testInstance.identity.getClient();

logConnectionStatusChanges(client);

// Bracketing the open so a timeout can be attributed. Silence after "Acquiring test identity" and before this
// line means setup was stuck getting an identity; silence after this line means the connect itself stalled.
log.info("Opening client");
client.open(true);
log.info("Client opened");

// deviceClient.open() is a no-op on HTTP, so a message needs to be sent to actually test opening the connection
if (testInstance.protocol == HTTPS)
Expand Down
7 changes: 2 additions & 5 deletions vsts/E2ETestsSetup/test-resources.bicep
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,6 @@ param BlobServiceName string = 'default'
@description('The name of the Container inside the BlobService.')
param ContainerName string = 'fileupload'

@description('Flag to indicate if IoT hub should have security solution enabled.')
param EnableIotHubSecuritySolution bool = false

var hubKeysId = resourceId('Microsoft.Devices/IotHubs/Iothubkeys', HubName, 'iothubowner')
var dpsKeysId = resourceId('Microsoft.Devices/ProvisioningServices/keys', DpsName, 'provisioningserviceowner')

Expand Down Expand Up @@ -70,7 +67,7 @@ resource container 'Microsoft.Storage/storageAccounts/blobServices/containers@20
}
}

resource iotHub 'Microsoft.Devices/IotHubs@2021-03-03-preview' = {
resource iotHub 'Microsoft.Devices/IotHubs@2023-06-30' = {
Comment thread
ewertons marked this conversation as resolved.
name: HubName
location: resourceGroup().location
identity: {
Expand Down Expand Up @@ -99,7 +96,7 @@ resource iotHub 'Microsoft.Devices/IotHubs@2021-03-03-preview' = {
maxDeliveryCount: 100
}
}
StorageEndpoints: {
storageEndpoints: {
'$default': {
sasTtlAsIso8601: 'PT1H'
connectionString: 'DefaultEndpointsProtocol=https;AccountName=${storageAccount.name};AccountKey=${listkeys(storageAccount.id, '2019-06-01').keys[0].value}'
Expand Down
Loading