Fix ObjectDisposedException race in PublishTestResultsCommand#5516
Open
Fix ObjectDisposedException race in PublishTestResultsCommand#5516
Conversation
avishmsft
force-pushed
the
users/avishjain/fix-publish-test-results-race
branch
2 times, most recently
from
ffe2d2a to
016204d
Compare
Contributor
Author
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
avishmsft
force-pushed
the
users/avishjain/fix-publish-test-results-race
branch
3 times, most recently
from
a7ed563 to
59dcc7a
Compare
Rebased on latest master (includes _isDetectTestRunRetry feature). Changes from original: - ResultsCommandExtension: GetService->CreateService for publishers, capture all instance fields as locals before async boundary (including new _isDetectTestRunRetry), use locals throughout async methods - TestDataPublisher: CreateService for ITestResultsServer and IFeatureFlagService - Parser: new overload accepting IFeatureFlagService from caller, fallback to GetService when not provided - TestDataPublisher passes its FeatureFlagService to parser Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
avishmsft
force-pushed
the
users/avishjain/fix-publish-test-results-race
branch
from
c289d4e to
31021cd
Compare
Contributor
Author
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| private IExecutionContext _executionContext; | ||
|
|
||
| //publish test results inputs | ||
| private List<string> _testResultFiles; |
Contributor
There was a problem hiding this comment.
Can we remove these as class variables and jsut make them local variable in execute method so that multiple execution do not conflict...
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
ObjectDisposedException when multiple ##vso[results.publish] commands are emitted in the same step. The root cause is a concurrency bug:
PublishTestResultsCommand is a singleton whose Execute() method stores inputs in instance fields and then starts an async task. The command serialization lock (_commandSerializeLock) releases when
Execute() returns, but the async task continues in the background. A subsequent Execute() call overwrites the instance fields and reinitializes shared singleton services (via GetService()), causing the
first async task to read corrupted state — disposed VssConnection objects, wrong test runner, wrong file list, etc.
Description
Provide a concise summary of the changes introduced in this PR.
1. GetService → CreateService for per-invocation service isolation:
Changed GetService() to CreateService() for ITestDataPublisher, ILegacyTestRunDataPublisher, ITestResultsServer, IFeatureFlagService, and ITestRunPublisher. GetService returns a cached singleton from
HostContext._serviceInstances — a second Execute() call would reinitialize the same instance (overwriting _connection, _featureFlagService, etc.) while the first async task is still using it.
Capture all: mutable instance state as local variables in Execute() before the async continuation, and pass them as parameters to PublishTestRunDataAsync() and helper methods.
Risk Assessment (Low / Medium / High)
Low
Unit Tests Added or Updated (Yes / No)
No — Existing ResultsCommandTests (3 tests) continue to pass
Additional Testing Performed
List manual or automated tests performed beyond unit tests (e.g., integration, scenario, regression).
Change Behind Feature Flag (Yes / No)
No — This is a concurrency correctness fix
Tech Design / Approach
Per-invocation service instances via CreateService + local variable capture.
Documentation Changes Required (Yes/No)
No
Logging Added/Updated (Yes/No)
No — Existing logging is sufficient. No new code paths were added; only the concurrency model was fixed.
Telemetry Added/Updated (Yes/No)
No — Existing telemetry via PublishEventsAsync is preserved
Rollback Scenario and Process (Yes/No)
Yes — Rollback is a single commit revert
Dependency Impact Assessed and Regression Tested (Yes/No)
Yes — No new dependencies introduced