Add [ResourceLock] attribute for fine-grained parallel test isolation by Evangelink · Pull Request #10234 · microsoft/testfx

Evangelink · 2026-07-26T10:37:12Z

Problem

MSTest's only tool for tests that contend on shared state is [DoNotParallelize], and it is all-or-nothing. A test that touches one shared resource is excluded from running with everything, and it is additionally deferred to a sequential phase after the parallel set drains.

test/UnitTests/Microsoft.Testing.Extensions.UnitTests/AzureFoundryChatClientProviderTests.cs is a good illustration: its own comment says the tests "must not run concurrently with each other" because the provider reads exactly three process-wide variables (AZURE_OPENAI_ENDPOINT, AZURE_OPENAI_DEPLOYMENT_NAME, AZURE_OPENAI_API_KEY). The blast radius is three known variables, yet the whole class is serialized against the entire suite. Repos with integration/acceptance tests that share filesystem paths, env vars or the console end up disabling parallelization for whole classes or assemblies.

Solution

[ResourceLock] lets a test declare the named resources it touches, so the scheduler serializes only the conflicting tests. Prior art is JUnit 5's @ResourceLock.

private const string AzureOpenAIEnvironment = "AZURE_OPENAI_*";

[TestMethod]
[ResourceLock(AzureOpenAIEnvironment)]
public void ReadsThoseVariables() { }

[TestMethod]
[ResourceLock(WellKnownResources.EnvironmentVariables)]
public void MutatesPath() { }

[TestMethod]
[ResourceLock("built-asset", Mode = ResourceAccessMode.Read)]   // readers share
public void ReadsSharedAsset() { }

Keys are free-form opaque strings compared ordinally, so well-known and user-invented keys use the identical mechanism. Read locks run concurrently with each other and block only against a ReadWrite holder. Multiple locks are acquired in sorted key order, which makes deadlock impossible. A lock is held across [TestInitialize]/[TestCleanup] (and [ClassInitialize]/[ClassCleanup] where the chunk spans the class), because setup that touches the resource must not race outside the lock.

Design record: RFC 020.

Semantics depend on `ExecutionScope` — please read before reviewing

This is the part most likely to be misread as JUnit-equivalent, so it is stated explicitly:

`ExecutionScope`	Chunk	What `[ResourceLock]` buys
`ClassLevel` (default)	the whole class	Selective coordination between classes. A class's methods are already sequential, so a lock adds nothing within a class; it earns its keep when another class declares the same key.
`MethodLevel`	a single test	Selective coordination between methods and test cases, including within one class.
parallelization disabled	—	No effect; everything is already sequential.

Under ClassLevel the chunk's locks are the union of the class's and every method's keys, each upgraded to the strongest mode declared anywhere in the class. So annotating individual methods does not give finer granularity under ClassLevel.

[DoNotParallelize] is unchanged and takes precedence: such tests run in the sequential phase and never reach the lock scheduler, so their declared locks are inert. [DoNotParallelize] was deliberately not reimplemented on top of this — it defers tests to the end of the run, whereas a lock interleaves them, so unifying them would be a silent behavioural break.

Known limitation

A chunk blocked on a lock occupies a scheduler worker. Workers dequeue a chunk and then wait for its locks, so a worker waiting on a contended key is unavailable for unrelated work queued behind it. [ResourceLock] guarantees correct mutual exclusion, not that unrelated tests always keep running; heavy contention still reduces effective parallelism. Keep locked tests a small fraction of the suite.

Lock-aware dispatch (skip a chunk whose locks are unavailable, take the next) is future work. It is not attempted here because a naive version livelocks without a bounded retry policy.

Two decisions needing explicit sign-off

Both are cheap to change now and impossible after shipping, so they should be ratified rather than pass unnoticed. Both are recorded in the RFC's Unresolved questions.

The exact WellKnownResources values are permanent public API. Proposed: CurrentDirectory = "System.Environment.CurrentDirectory", EnvironmentVariables = "System.Environment.Variables", Console = "System.Console". CurrentDirectory deliberately names Environment.CurrentDirectory (a real BCL property) rather than Directory.CurrentDirectory (which names no actual member). The set is frozen at three keys; culture and time zone are excluded because CultureInfo.CurrentCulture is thread-scoped while DefaultThreadCurrentCulture is process-wide, so one "culture" key would be ambiguous about which it protects. There is no Global key, because equality-based conflict detection cannot make one actually global.
The dynamic resource-locks provider is deferred out of v1. Consequence: a parameterized test uses one constant key across all its variants, which serializes them unnecessarily — coarser but still correct. JUnit shipped @ResourceLock in 5.3 and ResourceLocksProvider only in 5.12; the hook is purely additive.

Note for reviewers

f49bb4210 fixes two pre-existing-shaped cancellation defects that hang the test host and is independently backportable — you may want it reviewed or taken separately:

AsyncReaderWriterLock deadlocked when a waiter was cancelled while being granted: the grant path disposed the waiter's CancellationTokenRegistration while holding the internal gate, and that disposal blocks until the cancel callback completes — a callback that needs the same gate. This wedges a scheduler worker, so Task.WhenAll never completes. Any run cancellation landing during a lock hand-off triggers it.
TestRunCancellationToken.CancellationToken returned a token that Cancel() never signals, so cancellation never reached waiters parked on a lock — precisely when cancellation is most needed, since the stuck test is the one holding the lock.

Each regression test was verified to fail against the unfixed code, not merely to pass against the fix.

Also included

MSTEST0073 analyzer encouraging const keys over bare string literals, since a typo in a free-form key fails open (a silent race rather than an error).
Null/empty/whitespace keys are rejected at attribute construction; an empty key would otherwise become a shared key that silently serializes unrelated tests.
ReadWrite is the zero value so default(ResourceAccessMode) is the exclusive, safe mode.

Testing

Unit tests for the attribute, the async reader/writer lock, chunk lock merging and the analyzer.
Acceptance tests over two generated assets — one MethodLevel, one ClassLevel — covering writer serialization, reader concurrency, reader/writer exclusion, lock lifetime across TestInitialize/TestCleanup, class-level locks, and chunk mode merging.
Full suites green: TestFramework.UnitTests 1481/1481, MSTestAdapter.PlatformServices.UnitTests 956/956, analyzer 7/7, acceptance 6/6 across net8.0/net462/net10.0.

Adoption is intentionally out of scope: no existing [DoNotParallelize] usage is migrated here. The RFC carries an inventory of candidates for follow-up.

Copilot

Pull request overview

Adds named read/write resource locking for fine-grained MSTest parallel-test isolation.

Changes:

Introduces public resource-lock APIs and MSTEST0073.
Integrates lock discovery, transport, scheduling, cancellation, and synchronization.
Adds unit/acceptance tests, documentation, localization, and API tracking.

Show a summary per file

File	Description
`test/UnitTests/TestFramework.UnitTests/Attributes/ResourceLockAttributeTests.cs`	Tests public locking APIs.
`test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/ResourceLockManagerTests.cs`	Tests lock merging and execution.
`test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/AsyncReaderWriterLockTests.cs`	Tests synchronization and cancellation.
`test/UnitTests/MSTest.Analyzers.UnitTests/PreferConstantForResourceLockAnalyzerTests.cs`	Tests MSTEST0073 behavior.
`test/IntegrationTests/MSTest.Acceptance.IntegrationTests/ResourceLockExecutionTests.cs`	Adds end-to-end scheduling tests.
`src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.zh-Hant.xlf`	Adds localized-resource placeholder.
`src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.zh-Hans.xlf`	Adds localized-resource placeholder.
`src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.tr.xlf`	Adds localized-resource placeholder.
`src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.ru.xlf`	Adds localized-resource placeholder.
`src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.pt-BR.xlf`	Adds localized-resource placeholder.
`src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.pl.xlf`	Adds localized-resource placeholder.
`src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.ko.xlf`	Adds localized-resource placeholder.
`src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.ja.xlf`	Adds localized-resource placeholder.
`src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.it.xlf`	Adds localized-resource placeholder.
`src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.fr.xlf`	Adds localized-resource placeholder.
`src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.es.xlf`	Adds localized-resource placeholder.
`src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.de.xlf`	Adds localized-resource placeholder.
`src/TestFramework/TestFramework/Resources/xlf/FrameworkMessages.cs.xlf`	Adds localized-resource placeholder.
`src/TestFramework/TestFramework/Resources/FrameworkMessages.resx`	Defines invalid-key message.
`src/TestFramework/TestFramework/PublicAPI/PublicAPI.Unshipped.txt`	Tracks new public APIs.
`src/TestFramework/TestFramework/Attributes/Lifecycle/WellKnownResources.cs`	Defines standard resource keys.
`src/TestFramework/TestFramework/Attributes/Lifecycle/ResourceLockAttribute.cs`	Defines resource-lock attribute.
`src/TestFramework/TestFramework/Attributes/Lifecycle/ResourceAccessMode.cs`	Defines read/write modes.
`src/Analyzers/MSTest.Analyzers/xlf/Resources.zh-Hant.xlf`	Adds analyzer localization entries.
`src/Analyzers/MSTest.Analyzers/xlf/Resources.zh-Hans.xlf`	Adds analyzer localization entries.
`src/Analyzers/MSTest.Analyzers/xlf/Resources.tr.xlf`	Adds analyzer localization entries.
`src/Analyzers/MSTest.Analyzers/xlf/Resources.ru.xlf`	Adds analyzer localization entries.
`src/Analyzers/MSTest.Analyzers/xlf/Resources.pt-BR.xlf`	Adds analyzer localization entries.
`src/Analyzers/MSTest.Analyzers/xlf/Resources.pl.xlf`	Adds analyzer localization entries.
`src/Analyzers/MSTest.Analyzers/xlf/Resources.ko.xlf`	Adds analyzer localization entries.
`src/Analyzers/MSTest.Analyzers/xlf/Resources.ja.xlf`	Adds analyzer localization entries.
`src/Analyzers/MSTest.Analyzers/xlf/Resources.it.xlf`	Adds analyzer localization entries.
`src/Analyzers/MSTest.Analyzers/xlf/Resources.fr.xlf`	Adds analyzer localization entries.
`src/Analyzers/MSTest.Analyzers/xlf/Resources.es.xlf`	Adds analyzer localization entries.
`src/Analyzers/MSTest.Analyzers/xlf/Resources.de.xlf`	Adds analyzer localization entries.
`src/Analyzers/MSTest.Analyzers/xlf/Resources.cs.xlf`	Adds analyzer localization entries.
`src/Analyzers/MSTest.Analyzers/Resources.resx`	Defines MSTEST0073 messages.
`src/Analyzers/MSTest.Analyzers/PreferConstantForResourceLockAnalyzer.cs`	Implements MSTEST0073.
`src/Analyzers/MSTest.Analyzers/Helpers/WellKnownTypeNames.cs`	Registers attribute metadata name.
`src/Analyzers/MSTest.Analyzers/Helpers/DiagnosticIds.cs`	Assigns MSTEST0073 ID.
`src/Analyzers/MSTest.Analyzers/AnalyzerReleases.Unshipped.md`	Records analyzer release.
`src/Adapter/MSTestAdapter.PlatformServices/ObjectModel/UnitTestElement.cs`	Carries discovered locks.
`src/Adapter/MSTestAdapter.PlatformServices/ObjectModel/ResourceLockInfo.cs`	Models and serializes locks.
`src/Adapter/MSTestAdapter.PlatformServices/InternalAPI/InternalAPI.Unshipped.txt`	Tracks internal adapter APIs.
`src/Adapter/MSTestAdapter.PlatformServices/Execution/TestRunCancellationToken.cs`	Exposes scheduler cancellation token.
`src/Adapter/MSTestAdapter.PlatformServices/Execution/TestExecutionManager.Parallelization.cs`	Integrates locking into scheduling.
`src/Adapter/MSTestAdapter.PlatformServices/Execution/ResourceLockManager.cs`	Acquires per-resource locks.
`src/Adapter/MSTestAdapter.PlatformServices/Execution/AsyncReaderWriterLock.cs`	Implements asynchronous locking.
`src/Adapter/MSTestAdapter.PlatformServices/Discovery/TypeEnumerator.cs`	Discovers and merges attributes.
`src/Adapter/MSTest.TestAdapter/InternalAPI/uwp/InternalAPI.Unshipped.txt`	Tracks UWP transport property.
`src/Adapter/MSTest.TestAdapter/InternalAPI/InternalAPI.Unshipped.txt`	Tracks transport property.
`src/Adapter/MSTest.TestAdapter/Extensions/UnitTestElementExtensions.cs`	Serializes locks into test cases.
`src/Adapter/MSTest.TestAdapter/Extensions/TestCaseExtensions.cs`	Restores locks during execution.
`src/Adapter/MSTest.TestAdapter/AdapterTestProperties.cs`	Defines hidden lock property.
`docs/RFCs/020-Resource-Lock-Attribute.md`	Documents design and semantics.

Review details

Files reviewed: 55/55 changed files
Comments generated: 18
Review effort level: Medium

Copilot

Review details

Comments suppressed due to low confidence (15)

test/IntegrationTests/MSTest.Acceptance.IntegrationTests/ResourceLockExecutionTests.cs:1

This new C# file is missing the UTF-8 BOM required by .editorconfig:65-67. Please preserve the repository encoding so the charset check does not fail.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestRunCancellationToken.cs:1
This edit removes the UTF-8 BOM required for C# files by .editorconfig:65-67. Please restore it to avoid failing the repository charset check.

// Copyright (c) Microsoft Corporation. All rights reserved.

src/Analyzers/MSTest.Analyzers/xlf/Resources.zh-Hant.xlf:52

This resets an existing Traditional Chinese translation to English and marks it new. The change is unrelated to MSTEST0073 and regresses the localized MSTEST0072 description; preserve the translated target while adding the new resource strings.
src/Analyzers/MSTest.Analyzers/xlf/Resources.zh-Hans.xlf:52
This resets an existing Simplified Chinese translation to English and marks it new. That regresses the unrelated localized MSTEST0072 description; retain the translated target.
src/Analyzers/MSTest.Analyzers/xlf/Resources.tr.xlf:52
This replaces an existing Turkish translation for MSTEST0072 with English and marks it new. Preserve the translated target; only the new MSTEST0073 units need to be added here.

        <target state="new">'[AssemblyFixtureProvider]' discovery relies on walking the runtime assembly reference graph, which is not supported when the runtime cannot generate dynamic code (for example under Native AOT or Blazor WebAssembly AOT). The attribute is ignored in such builds, so the provided assembly initialize and cleanup methods will not run. Declare the '[AssemblyInitialize]' and '[AssemblyCleanup]' methods directly in the test assembly instead.</target>

src/Analyzers/MSTest.Analyzers/xlf/Resources.ru.xlf:54

This replaces an existing Russian MSTEST0072 translation with English and marks it new. Restore the translated target to avoid a localization regression unrelated to this feature.

        <target state="new">'[AssemblyFixtureProvider]' discovery relies on walking the runtime assembly reference graph, which is not supported when the runtime cannot generate dynamic code (for example under Native AOT or Blazor WebAssembly AOT). The attribute is ignored in such builds, so the provided assembly initialize and cleanup methods will not run. Declare the '[AssemblyInitialize]' and '[AssemblyCleanup]' methods directly in the test assembly instead.</target>

src/Analyzers/MSTest.Analyzers/xlf/Resources.pt-BR.xlf:52

This replaces an existing Portuguese MSTEST0072 translation with English and marks it new. Preserve the translated target while adding the new analyzer resources.

        <target state="new">'[AssemblyFixtureProvider]' discovery relies on walking the runtime assembly reference graph, which is not supported when the runtime cannot generate dynamic code (for example under Native AOT or Blazor WebAssembly AOT). The attribute is ignored in such builds, so the provided assembly initialize and cleanup methods will not run. Declare the '[AssemblyInitialize]' and '[AssemblyCleanup]' methods directly in the test assembly instead.</target>

src/Analyzers/MSTest.Analyzers/xlf/Resources.pl.xlf:52

This replaces an existing Polish MSTEST0072 translation with English and marks it new. Restore the translated target to avoid regressing an unrelated diagnostic.

        <target state="new">'[AssemblyFixtureProvider]' discovery relies on walking the runtime assembly reference graph, which is not supported when the runtime cannot generate dynamic code (for example under Native AOT or Blazor WebAssembly AOT). The attribute is ignored in such builds, so the provided assembly initialize and cleanup methods will not run. Declare the '[AssemblyInitialize]' and '[AssemblyCleanup]' methods directly in the test assembly instead.</target>

src/Analyzers/MSTest.Analyzers/xlf/Resources.ko.xlf:52

This replaces an existing Korean MSTEST0072 translation with English and marks it new. Keep the translated target; this resource was not changed by the new analyzer.

        <target state="new">'[AssemblyFixtureProvider]' discovery relies on walking the runtime assembly reference graph, which is not supported when the runtime cannot generate dynamic code (for example under Native AOT or Blazor WebAssembly AOT). The attribute is ignored in such builds, so the provided assembly initialize and cleanup methods will not run. Declare the '[AssemblyInitialize]' and '[AssemblyCleanup]' methods directly in the test assembly instead.</target>

src/Analyzers/MSTest.Analyzers/xlf/Resources.it.xlf:52

This replaces an existing Italian MSTEST0072 translation with English and marks it new. Restore the translated target so adding MSTEST0073 does not regress another diagnostic's localization.

        <target state="new">'[AssemblyFixtureProvider]' discovery relies on walking the runtime assembly reference graph, which is not supported when the runtime cannot generate dynamic code (for example under Native AOT or Blazor WebAssembly AOT). The attribute is ignored in such builds, so the provided assembly initialize and cleanup methods will not run. Declare the '[AssemblyInitialize]' and '[AssemblyCleanup]' methods directly in the test assembly instead.</target>

src/Analyzers/MSTest.Analyzers/xlf/Resources.fr.xlf:52

This replaces an existing French MSTEST0072 translation with English and marks it new. Preserve the translated target; only the new resource-lock strings should remain untranslated.

        <target state="new">'[AssemblyFixtureProvider]' discovery relies on walking the runtime assembly reference graph, which is not supported when the runtime cannot generate dynamic code (for example under Native AOT or Blazor WebAssembly AOT). The attribute is ignored in such builds, so the provided assembly initialize and cleanup methods will not run. Declare the '[AssemblyInitialize]' and '[AssemblyCleanup]' methods directly in the test assembly instead.</target>

src/Analyzers/MSTest.Analyzers/xlf/Resources.es.xlf:52

This replaces an existing Spanish MSTEST0072 translation with English and marks it new. Restore the translated target to prevent an unrelated localization regression.

        <target state="new">'[AssemblyFixtureProvider]' discovery relies on walking the runtime assembly reference graph, which is not supported when the runtime cannot generate dynamic code (for example under Native AOT or Blazor WebAssembly AOT). The attribute is ignored in such builds, so the provided assembly initialize and cleanup methods will not run. Declare the '[AssemblyInitialize]' and '[AssemblyCleanup]' methods directly in the test assembly instead.</target>

src/Analyzers/MSTest.Analyzers/xlf/Resources.de.xlf:52

This replaces an existing German MSTEST0072 translation with English and marks it new. Preserve the translated target while introducing the MSTEST0073 resource entries.

        <target state="new">'[AssemblyFixtureProvider]' discovery relies on walking the runtime assembly reference graph, which is not supported when the runtime cannot generate dynamic code (for example under Native AOT or Blazor WebAssembly AOT). The attribute is ignored in such builds, so the provided assembly initialize and cleanup methods will not run. Declare the '[AssemblyInitialize]' and '[AssemblyCleanup]' methods directly in the test assembly instead.</target>

src/Analyzers/MSTest.Analyzers/xlf/Resources.cs.xlf:52

This replaces an existing Czech MSTEST0072 translation with English and marks it new. Restore the translated target so this PR does not regress localization for an unrelated rule.

        <target state="new">'[AssemblyFixtureProvider]' discovery relies on walking the runtime assembly reference graph, which is not supported when the runtime cannot generate dynamic code (for example under Native AOT or Blazor WebAssembly AOT). The attribute is ignored in such builds, so the provided assembly initialize and cleanup methods will not run. Declare the '[AssemblyInitialize]' and '[AssemblyCleanup]' methods directly in the test assembly instead.</target>

docs/RFCs/020-Resource-Lock-Attribute.md:300

The implementation in AsyncReaderWriterLock.cs is not SemaphoreSlim-based or merely writer-preference; it uses a monitor-protected linked-list queue with FIFO fairness. Update the RFC so the design record matches the code being shipped.

There is no async reader-writer lock in the BCL, so a small internal one is added — a
`SemaphoreSlim`-based writer-preference implementation — correct under cancellation, since the
scheduler honors `_testRunCancellationToken`.

Files reviewed: 55/55 changed files
Comments generated: 1
Review effort level: Medium

Adds a named-resource locking mechanism so tests can declare the shared resources they touch, letting the parallel scheduler serialize only the conflicting tests instead of disabling parallelization wholesale with [DoNotParallelize]. - RFC 020 documenting the design, granularity guidance, migration inventory and future work. - Public API: ResourceLockAttribute, ResourceAccessMode (ReadWrite = 0 so the default fails closed) and WellKnownResources constants. - Adapter: AsyncReaderWriterLock, ResourceLockManager and scheduler wiring so locks are acquired in sorted order (deadlock-free) and span TestInitialize/TestCleanup. - Analyzer MSTEST0073 encouraging const keys over bare string literals. - Unit tests plus an acceptance test proving writer serialization, reader concurrency, reader/writer exclusion and lifecycle span. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d11a47de-a31c-4d9e-8612-811f252b3d7e

Both remaining open questions now carry a proposed answer with rationale, recorded as proposals pending ratification rather than settled decisions, since each is cheap to reverse before merge and impossible after ship. - Dynamic resource-locks provider is explicitly deferred out of v1. The v1 consequence is that parameterized tests share one constant key across their variants: coarser but still correct, matching the RFC's own over-lock-rather-than-under-lock guidance. JUnit shipped @ResourceLock in 5.3 and ResourceLocksProvider only in 5.12, and the hook is purely additive, so nothing is foreclosed. - WellKnownResources.CurrentDirectory changes from "System.IO.Directory.CurrentDirectory" to "System.Environment.CurrentDirectory". The former named no actual member (Directory exposes GetCurrentDirectory()/SetCurrentDirectory(), not a property); the latter is a real BCL property and makes every key rooted at the type that owns the state. - Freeze the set at three keys for v1 and document why culture and time zone are excluded: CultureInfo.CurrentCulture is thread-scoped while DefaultThreadCurrentCulture is process-wide, so one "culture" key would be ambiguous about which it protects. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d11a47de-a31c-4d9e-8612-811f252b3d7e

C1: AsyncReaderWriterLock deadlocked when a waiter was cancelled while being granted. ProcessQueue ran under the internal gate and completed granted waiters inline, and completing disposes the waiter's CancellationTokenRegistration. That disposal blocks until any concurrently executing callback for the registration finishes - and the callback takes the same gate, so the two threads wedge permanently. This stalls a scheduler worker, so Task.WhenAll never completes and the test host hangs. Any run cancellation landing during a lock hand-off (Ctrl-C, "Cancel run") triggers it. ProcessQueue now only collects granted waiters; callers complete them after leaving the gate. This is safe because Node is cleared under the gate, so a cancel callback can no longer complete an already-granted waiter. Registration also moves out of the gate, since Register invokes the callback synchronously when the token is already cancelled, which would otherwise re-enter the gate and dispose registrations while holding it. CancellationTokenRegistration.Unregister is deliberately not used: it is .NET 8+ only and this must build on net462/netstandard2.0. C2: TestRunCancellationToken.CancellationToken returned _originalCancellationToken, which Cancel() never signals, so cancellation arriving through Cancel() never reached waiters parked on a resource lock. Cancellation is typically requested because something is stuck, and the stuck test holds the lock, so every contending worker would stay parked. It now returns _cancellationTokenSource.Token, a strict superset signaled both by Cancel() and by the host token via the registration in MSTestEngine, matching what Register() on the same type already used. Both regressions are covered by time-bounded tests, each verified to fail against the unfixed code. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d11a47de-a31c-4d9e-8612-811f252b3d7e

H1 A chunk blocked on a lock occupies a scheduler worker, so unrelated queued chunks wait behind it. Accepted for v1 and documented rather than mitigated: the RFC no longer claims "everything else keeps running concurrently", a new "Scheduling and throughput" section states the limitation plainly, and lock-aware dispatch is filed as future work with a note that it needs a bounded retry policy to avoid livelock. H2 Removed the false claim that annotating methods substitutes for JUnit's CHILDREN under ClassLevel. The scheduler unions every method's keys into the class chunk and upgrades each to the strongest mode, so method placement only adds granularity under MethodLevel. H3 The public XML doc asserted class-level locks span ClassInitialize and ClassCleanup unconditionally, which holds only under ClassLevel. It is now qualified per scope, since under MethodLevel the lock is acquired and released per test and other classes may interleave. H4 Added a ClassLevel acceptance asset. It covers class-level [ResourceLock] and cross-element chunk mode merging, neither of which the MethodLevel asset could reach even though ClassLevel is the default. The asset uses more workers than there are contending classes: with too few, the contending classes occupy every worker and the merge assertion passes vacuously - verified by injecting a broken merge, which the test now catches. M1 Documented that DoNotParallelize takes precedence and resource locks are inert on such tests, replacing the misleading "either or both". M2 Kept Inherited=true, matching DoNotParallelizeAttribute, the closest analogue. Inheriting over-locks (slow) while not inheriting under-locks (racy), so this is the fail-closed direction. Documented that a derived class can neither remove nor weaken an inherited lock, and added a test pinning the attribute usage. M3 Documented that lock lifetime follows the scheduling chunk, including the bimodal data-driven case: unfolded rows are separate chunks, folded rows share one acquisition. M4 Reframed the motivating example. AzureFoundryChatClientProviderTests is a single class, so at the default ClassLevel its methods are already sequential and merely removing DoNotParallelize would preserve exclusion while un-deferring it. Value is now framed per scope. M5 Corrected the Global future-work design: every parallel chunk, including chunks declaring no locks, must implicitly take the key in Read mode, otherwise an explicit global writer would not exclude unlocked tests. Noted that v1 cannot express interleaved global exclusivity. M6 Reject null, empty and whitespace resource keys in the attribute constructor with a localized message. An empty key silently became a shared key that serialized unrelated tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d11a47de-a31c-4d9e-8612-811f252b3d7e

The rationale for Workers exceeding the number of contending classes sat in a comment several lines below the [assembly: Parallelize] line, so a reader "tidying" the worker count down would not see it. Move the explanation to the point of edit and record the evidence: with Workers equal to the contending-class count those classes occupy every worker, the merge classes never overlap, and the key:MK assertion passes even against a deliberately broken merge. Also note at the contending classes why worker starvation is deliberately not asserted: it would be timing-dependent, and it would fail once lock-aware dispatch is implemented, penalising a fix for removing a limitation. Comment-only change to the generated asset; rebuilt and re-ran the acceptance tests to confirm the asset still compiles and passes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d11a47de-a31c-4d9e-8612-811f252b3d7e

Observable concurrency in the ClassLevel asset is bounded by runnable thread-pool threads, not only by Workers: a chunk blocked on a lock awaits and releases its pool thread, while a running test body pins one for its Thread.Sleep. Min threads defaults to Environment.ProcessorCount and injection past that is throttled, so on a low-core agent the merge assertion could in principle observe less overlap than intended. Raise the pool minimum from an [AssemblyInitialize] so the asset does not depend on injection heuristics, and extend the load-bearing comment to name thread availability alongside the worker count, since the worker count argument alone is incomplete. Verified by mutation: with mode merging deliberately broken the test fails, with it restored the test passes, and both hold under a simulated two-processor host. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d11a47de-a31c-4d9e-8612-811f252b3d7e

- ResourceLockInfo.Decode stripped the first character even when the prefix was not one it wrote, so a malformed payload like "key-without-prefix" decoded to "ey-without-prefix" - a different key that no longer conflicts with the one it was meant to name. Only consume a recognized R/W prefix. The existing test asserted the decoded mode but not the key, which is why it missed this. - Mode is publicly settable, so (ResourceAccessMode)42 is valid attribute syntax and was carried through unchanged. It then encoded as "R" and failed to promote an existing Read during strongest-mode merging, both of which fail open. Normalize once in the ResourceLockInfo constructor: Read is the only value granting shared access, everything else is exclusive. - Restore the UTF-8 BOM on three files that lost it. Two were flagged in review; TestExecutionManager.Parallelization.cs had also lost it. - Regenerate the analyzer XLF from the current base. The previous regeneration predated the MSTEST0072 translations, so the diff reverted thirteen translated targets to English with state="new". The XLF diff is now purely additive and only adds the MSTEST0073 units. - Add round-trip coverage for the hidden VSTest TestProperty transport, which the acceptance assets never exercise because they run on the native MSTest runner, including keys that begin with the prefix characters. - Correct the RFC's description of the lock: it is a monitor-guarded FIFO waiter queue, not a SemaphoreSlim writer-preference implementation. - Assert the cancelled outcome in the previously empty catch block. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d11a47de-a31c-4d9e-8612-811f252b3d7e

Copilot

Review details

Files reviewed: 56/56 changed files
Comments generated: 0 new
Review effort level: Medium

The existing test verified only that the keys are distinct and non-empty, so a swap between two constants or a changed value survived. These strings are permanent public API and conflict detection is plain string equality, so changing one after shipping would silently stop matching every user key spelling the old value, and tests intending to share a resource would quietly stop being serialized. Asserting the exact values turns that into a build failure here rather than a silent loss of synchronization downstream. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d11a47de-a31c-4d9e-8612-811f252b3d7e

Copilot

Review details

Comments suppressed due to low confidence (1)

docs/RFCs/020-Resource-Lock-Attribute.md:157

This usage example is flagged by the MSTEST0073 analyzer introduced by this RFC, so users copying the documented pattern immediately receive a diagnostic. Define a named const for "db-fixture" and reference it here, consistent with the preceding example and the analyzer guidance.

[ResourceLock("db-fixture", Mode = ResourceAccessMode.Read)]  // readers run concurrently

Files reviewed: 56/56 changed files
Comments generated: 2
Review effort level: Medium

The analyzer declares LanguageNames.VisualBasic and deliberately inspects syntax token text rather than C#-specific syntax nodes, so it is language-neutral by construction - but nothing exercised the VB path, so the support was advertised and untested. A later change to C#-specific nodes would have dropped VB silently instead of failing a test. Adds a VB literal diagnostic case, a VB constant no-diagnostic case, and a case where several attributes share one VB angle-bracket list, which checks the analyzer reports per attribute application rather than per list. All three pass against the current implementation. Also marks the RFC status Implementation as complete, matching how other implemented-but-unshipped RFCs are recorded (docs/RFCs/017). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d11a47de-a31c-4d9e-8612-811f252b3d7e

Copilot

Review details

Comments suppressed due to low confidence (1)

src/Adapter/MSTestAdapter.PlatformServices/ObjectModel/ResourceLockInfo.cs:63

A prefix-only payload such as "R" cannot be produced from a valid [ResourceLock] because empty keys are rejected, so it represents truncated/corrupt transport data. This branch currently decodes it as an empty shared lock, which fails open and contradicts the documented guarantee that truncated data remains exclusive. Require at least one key character before consuming either prefix; otherwise preserve the whole payload as an exclusive key.

        if (encoded.Length > 0 && encoded[0] == 'R')
        {
            return new ResourceLockInfo(encoded.Substring(1), ResourceAccessMode.Read);
        }

        // Strip the prefix only when it is one we actually wrote. Stripping unconditionally would rewrite an
        // unrecognized payload into a *different* key, which would stop it conflicting with the intended one.
        string resource = encoded.Length > 0 && encoded[0] == 'W' ? encoded.Substring(1) : encoded;

Files reviewed: 56/56 changed files
Comments generated: 0 new
Review effort level: Medium

Decode consumed an R/W prefix even when nothing followed it, so the truncated payload "R" decoded to an empty *shared* lock. That fails open and manufactures an empty resource key, which [ResourceLock] explicitly rejects at construction, contradicting the documented guarantee that truncated transport data stays exclusive. Require at least one key character before consuming a prefix; otherwise keep the whole payload as an exclusive key. Encode cannot emit a bare prefix, so this is only reachable from corrupt or truncated data - but the whole point of the branch is to handle exactly that case, and it was the one input it got wrong. Also updates the RFC's third usage example to reference a named constant. It previously used a bare string literal, which the MSTEST0073 analyzer shipped by this same PR flags, so anyone copying the documented pattern got an immediate diagnostic. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d11a47de-a31c-4d9e-8612-811f252b3d7e

Copilot

Review details

Files reviewed: 56/56 changed files
Comments generated: 1
Review effort level: Medium

The class-level asset already pinned its thread-pool minimum, but the method-level asset did not, leaving its concurrency assertions dependent on injection timing. Its assertions are lower bounds - Read locks must be seen running concurrently with each other, and unlocked tests concurrently with the rest - so a starved pool makes them fail rather than pass vacuously, which is a flake on a low-core agent rather than a silent gap. A chunk blocked on a lock awaits and releases its pool thread, but a running body pins one for its Thread.Sleep, so worker count alone does not guarantee the concurrency these assertions require. Verified on a simulated two-processor host as well as normally. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d11a47de-a31c-4d9e-8612-811f252b3d7e

The test verified the resource strings for keys beginning with R and W but not the modes, so a mode-corruption mutation survived it. That is the mirror of the gap this suite already caught once, where a decode test asserted the mode but not the key and missed the key being rewritten. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d11a47de-a31c-4d9e-8612-811f252b3d7e

Copilot

Review details

Comments suppressed due to low confidence (1)

src/Adapter/MSTestAdapter.PlatformServices/ObjectModel/ResourceLockInfo.cs:60

The malformed-payload fallback still fails open when the unprefixed key starts with a valid prefix character. For example, if WRegistry loses its transport prefix, Decode("Registry") returns a shared lock for egistry, both weakening the mode and changing the protected key. Because arbitrary keys may begin with R or W, a one-character marker cannot distinguish this case; use an unambiguous marker/version such as R:/W: and decode only that form.

        if (encoded.Length > 1 && encoded[0] == 'R')

Files reviewed: 56/56 changed files
Comments generated: 0 new
Review effort level: Medium

Copilot

Review details

Files reviewed: 56/56 changed files
Comments generated: 0 new
Review effort level: Medium

The Encode doc claimed the fixed-width prefix "avoids any delimiter ambiguity with arbitrary resource strings", which overclaims. The encoding is not self-describing: a string never produced by Encode that happens to begin with R or W is indistinguishable from a real payload. Record what is actually true and why it is accepted. Encode and Decode are a matched pair over a private adapter property, so Decode only ever sees Encode's output, and keys beginning with the marker characters round-trip correctly, which is covered by tests. A delimited marker such as R: narrows the window but does not close it, since it must still decide what to do with a key that starts with the delimiter; only length-prefixing would, which is not warranted for an internal in-process property. Documentation only, no behavior change. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d11a47de-a31c-4d9e-8612-811f252b3d7e

github-actions · 2026-07-26T14:02:52Z

🧪 Test quality grade — PR #10234

No new or modified test methods were identified in the changed regions
of this PR. Nothing to grade.

_{Re-run with /grade-tests.}

🤖 Automated content by GitHub Copilot. Generated by the Grade Tests on PR (on open / sync) workflow. · sonnet46 36.1 AIC · ⌖ 6.82 AIC · ⊞ 10.3K · [◷]( · ◷)

Copilot

Review details

Files reviewed: 56/56 changed files
Comments generated: 0 new
Review effort level: Medium

Copilot AI review requested due to automatic review settings July 26, 2026 10:37

Copilot started reviewing on behalf of Evangelink July 26, 2026 10:37 View session

Copilot AI reviewed Jul 26, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 26, 2026 10:43

Evangelink force-pushed the dev/amauryleve/resource-lock-attribute branch from 1c4e41e to 0a07a15 Compare July 26, 2026 10:43

Copilot started reviewing on behalf of Evangelink July 26, 2026 10:44 View session

This comment has been minimized.

Sign in to view

Copilot AI reviewed Jul 26, 2026

View reviewed changes

Comment thread src/Adapter/MSTestAdapter.PlatformServices/ObjectModel/ResourceLockInfo.cs Outdated

github-code-quality Bot found potential problems Jul 26, 2026

View reviewed changes

Comment thread src/Analyzers/MSTest.Analyzers/PreferConstantForResourceLockAnalyzer.cs

Comment thread test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/AsyncReaderWriterLockTests.cs Fixed

Evangelink added the state/needs-review Awaiting review from the team. label Jul 26, 2026

Evangelink enabled auto-merge (squash) July 26, 2026 12:48

Evangelink and others added 7 commits July 26, 2026 14:51

Copilot AI review requested due to automatic review settings July 26, 2026 12:58

Evangelink force-pushed the dev/amauryleve/resource-lock-attribute branch from 0a07a15 to e55553d Compare July 26, 2026 12:58

Copilot started reviewing on behalf of Evangelink July 26, 2026 12:59 View session

Copilot AI reviewed Jul 26, 2026

View reviewed changes

0101 approved these changes Jul 26, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

Copilot AI review requested due to automatic review settings July 26, 2026 13:14

Copilot started reviewing on behalf of Evangelink July 26, 2026 13:15 View session

Copilot AI reviewed Jul 26, 2026

View reviewed changes

Comment thread src/Analyzers/MSTest.Analyzers/PreferConstantForResourceLockAnalyzer.cs

Comment thread docs/RFCs/020-Resource-Lock-Attribute.md Outdated

This comment has been minimized.

Sign in to view

Copilot AI review requested due to automatic review settings July 26, 2026 13:25

Copilot started reviewing on behalf of Evangelink July 26, 2026 13:26 View session

Copilot AI reviewed Jul 26, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

Copilot AI review requested due to automatic review settings July 26, 2026 13:38

Copilot started reviewing on behalf of Evangelink July 26, 2026 13:38 View session

Copilot AI reviewed Jul 26, 2026

View reviewed changes

Comment thread test/IntegrationTests/MSTest.Acceptance.IntegrationTests/ResourceLockExecutionTests.cs

Copilot AI review requested due to automatic review settings July 26, 2026 13:45

Copilot started reviewing on behalf of Evangelink July 26, 2026 13:45 View session

Copilot AI reviewed Jul 26, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 26, 2026 13:51

Copilot started reviewing on behalf of Evangelink July 26, 2026 13:51 View session

Copilot AI reviewed Jul 26, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

Copilot AI review requested due to automatic review settings July 26, 2026 13:58

Copilot started reviewing on behalf of Evangelink July 26, 2026 13:58 View session

0101 approved these changes Jul 26, 2026

View reviewed changes

Copilot AI reviewed Jul 26, 2026

View reviewed changes

Evangelink merged commit 0865e2c into main Jul 26, 2026
33 checks passed

Evangelink deleted the dev/amauryleve/resource-lock-attribute branch July 26, 2026 15:11

Evangelink mentioned this pull request Jul 27, 2026

Add parallel-safety analyzers (MSTEST0074-MSTEST0077) #10248

Merged

Uh oh!

Conversation

Evangelink commented Jul 26, 2026

Problem

Solution

Semantics depend on ExecutionScope — please read before reviewing

Known limitation

Two decisions needing explicit sign-off

Note for reviewers

Also included

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Review details

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Copilot AI left a comment

Choose a reason for hiding this comment

Review details

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Review details

Uh oh!

This comment has been minimized.

Copilot AI left a comment

Choose a reason for hiding this comment

Review details

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Copilot AI left a comment

Choose a reason for hiding this comment

Review details

Uh oh!

This comment has been minimized.

Copilot AI left a comment

Choose a reason for hiding this comment

Review details

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Review details

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Review details

Uh oh!

This comment has been minimized.

github-actions Bot commented Jul 26, 2026

🧪 Test quality grade — PR #10234

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Review details

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Semantics depend on `ExecutionScope` — please read before reviewing