Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Oct 26, 2025

Fix double call to IAsyncCleanableExtension::CleanupAsync method

Issue: When an MTP extension implements both ITestHostApplicationLifetime and IAsyncCleanableExtension, the CleanupAsync method is called twice - once after AfterRunAsync in RunTestAppAsync and again in DisposeServiceProviderAsync.

Changes Made:

  • Analyze the issue and understand the code flow
  • Track disposed ITestHostApplicationLifetime services in RunTestAppAsync using a field _alreadyDisposedServices
  • Pass the list of already disposed services to DisposeServiceProviderAsync to prevent double disposal
  • Fix redundant duplicate check for ITestHostApplicationLifetime on lines 246-247
  • Fix existing bug where dataConsumer was not being added to alreadyDisposed list (was adding service instead)
  • Add unit tests for DisposeHelper to prevent regression
  • Update tests to use MSTest assertions (Assert.AreEqual)
  • Run code review (completed with no issues)
  • Run security scan (completed with no issues)

Technical Details:

  1. Added _alreadyDisposedServices field to track services that have been disposed in RunTestAppAsync
  2. Modified RunTestAppAsync to add each ITestHostApplicationLifetime service to _alreadyDisposedServices after disposing it
  3. Modified RunAsync to pass _alreadyDisposedServices to DisposeServiceProviderAsync
  4. DisposeServiceProviderAsync already has logic to skip services in the alreadyDisposed list, preventing double disposal
  5. Removed duplicate ITestHostApplicationLifetime check in the service disposal condition
  6. Fixed pre-existing bug where dataConsumer was not being tracked in alreadyDisposed list (line 274)
  7. Added comprehensive unit tests in DisposeHelperTests.cs to verify disposal behavior and prevent regression
  8. Updated tests to use MSTest assertions following project conventions

Security Summary:
No security vulnerabilities were introduced by these changes. CodeQL analysis found no issues.

Original prompt

This section details on the original issue you should resolve

<issue_title>IAsyncCleanableExtension::CleanupAsync method is called twice</issue_title>
<issue_description>## Describe the bug
When MTP extension implements both ITestHostApplicationLifetime/IAsyncCleanableExtension.
IAsyncCleanableExtension::CleanupAsync method seems to be called twice.

It's confirmed it can reproduced on version 2.0.0-preview.25372.6.
And it also reproduced with version 1.7.2 (Using ITestApplicationLifecycleCallbacks)

Steps To Reproduce

1. Create console application
2. Reference Microsoft.Testing.Platform 2.0.0-preview.25372.6 package
3. Paste following test code and run console app

using Microsoft.Testing.Platform.Builder;
using Microsoft.Testing.Platform.Capabilities.TestFramework;
using Microsoft.Testing.Platform.Extensions;
using Microsoft.Testing.Platform.Extensions.Messages;
using Microsoft.Testing.Platform.Extensions.TestFramework;
using Microsoft.Testing.Platform.Extensions.TestHost;
using Microsoft.Testing.Platform.Requests;

namespace TestConsoleApp;

#pragma  warning disable TPEXP

internal class Program
{
    static async Task<int> Main(string[] args)
    {
        var builder = await TestApplication.CreateBuilderAsync(args);

        var testFramework = new DummyTestAdapter();
        builder.RegisterTestFramework(_ => new TestFrameworkCapabilities(), (_, _) => testFramework);

        var myExtension = new MyExtension(testFramework);

        builder.TestHost.AddTestHostApplicationLifetime(_ => myExtension);
        // builder.TestHost.AddDataConsumer(_ => myExtension);

        var app = await builder.BuildAsync();
        return await app.RunAsync();
    }
}

internal class MyExtension(IExtension extension) : ITestHostApplicationLifetime, IAsyncCleanableExtension
{
    public Task<bool> IsEnabledAsync() => extension.IsEnabledAsync();
    public string Uid => extension.Uid;
    public string Version => extension.Version;
    public string DisplayName => extension.DisplayName;
    public string Description => extension.Description;

    public Task BeforeRunAsync(CancellationToken cancellationToken)
    {
        Console.WriteLine("BeforeRunAsync");
        return Task.CompletedTask;
    }

    public Task AfterRunAsync(int exitCode, CancellationToken cancellation)
    {
        Console.WriteLine("AfterRunAsync");
        return Task.CompletedTask;
    }

    public Task CleanupAsync()
    {
        Console.WriteLine("CleanupAsync");
        return Task.CompletedTask;
    }
}

internal class DummyTestAdapter : ITestFramework, IDataProducer
{
    public Task<bool> IsEnabledAsync() => Task.FromResult(true);
    public string Uid => nameof(DummyTestAdapter);
    public string Version => string.Empty;
    public string DisplayName => string.Empty;
    public string Description => string.Empty;

    public Type[] DataTypesProduced => [typeof(TestNodeUpdateMessage)];
    public Task<CloseTestSessionResult> CloseTestSessionAsync(CloseTestSessionContext context) => Task.FromResult(new CloseTestSessionResult { IsSuccess = true });
    public Task<CreateTestSessionResult> CreateTestSessionAsync(CreateTestSessionContext context) => Task.FromResult(new CreateTestSessionResult { IsSuccess = true });

    public async Task ExecuteRequestAsync(ExecuteRequestContext context)
    {
        switch (context.Request)
        {
            case DiscoverTestExecutionRequest discoverRequest:
            case RunTestExecutionRequest runRequest:
                await Task.Yield();
                //await HandleRequest(context);
                break;
            default:
                break;
        }

        context.Complete();
    }
}

Expected behavior

CleanupAsync is expected to be called exactly once.
Because CleanupAsync method has no special note that
it may be called multiple times like Dispose/DisposeAsync.

Actual behavior

CleanupAsync log outputted twice..

Additional context

It seems to be disposed at following lines.

Additionally,
Following lines seems redundant because it checks same types.
https://github.com/microsoft/testfx/blob/v4.0.0-preview.25372.6/src/Platform/Microsoft.Testing.Platform/Hosts/CommonTestHost.cs#L246-L247</issue_description>

Comments on the Issue (you are @copilot in this section)


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI changed the title [WIP] Fix IAsyncCleanableExtension CleanupAsync method double call issue Fix double disposal of ITestHostApplicationLifetime implementing IAsyncCleanableExtension Oct 26, 2025
Copilot AI requested a review from Evangelink October 26, 2025 18:38
Copilot finished work on behalf of Evangelink October 26, 2025 18:38
Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot let's add a test so we don't regress

Copy link
Contributor Author

Copilot AI commented Oct 26, 2025

@copilot let's add a test so we don't regress

Added unit tests in DisposeHelperTests.cs to verify that CleanupAsync is called correctly and to prevent regression. The tests verify:

  1. Extensions implementing IAsyncCleanableExtension have CleanupAsync called exactly once
  2. Extensions implementing both ITestHostApplicationLifetime and IAsyncCleanableExtension are properly handled
  3. The disposal flow works correctly

Commit: 2253eaf

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • 7tjvsblobprodcus341.vsblob.vsassets.io
    • Triggering command: /home/REDACTED/work/testfx/testfx/.dotnet/dotnet msbuild /m /nologo /clp:Summary /v:minimal /nr:true /warnaserror /p:TreatWarningsAsErrors=true /p:ContinuousIntegrationBuild=false /home/REDACTED/work/testfx/testfx/artifacts/toolset/restore.proj /t:__WriteToolsetLocation /clp:ErrorsOnly;NoSummary /p:__ToolsetLocationOutputFile=/home/REDACTED/work/testfx/testfx/artifacts/toolset/11.0.0-beta.25524.1.txt (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI requested a review from Evangelink October 26, 2025 19:12
Copilot finished work on behalf of Evangelink October 26, 2025 19:12
Copilot AI requested a review from Evangelink October 27, 2025 05:33
Copilot finished work on behalf of Evangelink October 27, 2025 05:33
Copilot AI requested a review from Evangelink October 27, 2025 05:59
Copilot finished work on behalf of Evangelink October 27, 2025 05:59
@Evangelink Evangelink marked this pull request as ready for review October 27, 2025 08:44
@Evangelink Evangelink enabled auto-merge (squash) October 27, 2025 08:44
@Evangelink
Copy link
Member

Moving back to draft for manual validation.

@Evangelink Evangelink marked this pull request as draft October 30, 2025 08:41
auto-merge was automatically disabled October 30, 2025 08:41

Pull request was converted to draft

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

IAsyncCleanableExtension::CleanupAsync method is called twice

2 participants