fix: CI flakes from leaked hook contexts, ActivityCollector race and Repro5700 rendezvous

[thomhurst]

Fixes three intermittent CI failures found by classifying recent failed modularpipeline runs across unrelated branches.

1. FilterByDynamicAddedPropertyTestsModule (Windows, file in use)

Run 36354476720: BeforeAssembly hook failed: The process cannot access the file '...TUnit_3882_Assembly_Before.txt' because it is being used by another process.

AssemblyLevelCancellationTests only writes its marker when its class appears in context.TestClasses. But in a subprocess that ran only the dynamic-property filter, it still appeared there, so it raced with the CancellationAfterHooksTests subprocess writing the same file. There were two engine bugs behind this:

• Filtered-out tests stayed in hook contexts. Each test is added to its ClassHookContext when its TestContext is created. Filters that can only be evaluated after building, such as property filters, left every built test registered. TestClasses, AllTests and TestCount then reported classes that were not part of the run. TestDiscoveryService now removes excluded tests once the final list is known, via the new internal ClassHookContext.RetainTests. It only does this for execution and only when the filter excluded something.
• Orphaned contexts from a GetOrAdd race. AssemblyHookContext and ClassHookContext register with their parent in the constructor, but ContextProvider created them in a ConcurrentDictionary.GetOrAdd factory. Contexts that lost the race stayed registered as duplicates with no tests. With the dynamic-property filter, about 40 of these showed up. Creation is now double-checked under a lock. That runs once per class or assembly, so lookups stay lock-free.

Both paths go through TestDiscoveryService and ContextProvider, so the fix applies to source-gen and reflection modes alike.

2. IndexOutOfRangeException from Activity.Stop() (SessionActivityLifecycleTests, macOS/ubuntu)

Runs 36537750479 and 36325109303: IndexOutOfRangeException at SynchronizedList.EnumWithAction / Activity.Stop().

ActivityCollector.OnActivityStopped counts tags, events and links, then fills exactly-sized arrays in a second pass. If another thread adds a tag between the two passes, the array overflows. The exception then escapes through Activity.Stop() into whatever code stopped the activity. The fill is now bounded and the array trimmed to what it actually saw.

The trigger was the test itself. Its ActivityListenerScope tracked every activity on the TUnit sources, including engine spans for tests running in parallel, and stopped them on dispose while the engine was still writing to them. The scope now tracks only activities started from its own async flow.

3. Repro5700.Test2_KeyedSerializes timeout (Windows)

Run 36466695267: TaskCanceledException after 1m. Test2 also waited for Test1 to start. Test1 sits in the shared parallel queue behind hundreds of tests in the engine-test subprocess, so it was sometimes dispatched more than 60s later. Now only Test1 waits for Test2. That still catches the #5700 regression (keyed tests deferred until all unconstrained tests finish), because Test1 would time out.

Other failures, classified but not changed here

• STAThreadTests.Without_STA failures (6 runs, Sep 26–27): fixed by fix: don't run DedicatedThreadExecutor continuations inline on the dedicated thread #6898.
• ObjectInitializerTests.Waiting_Continuations_Do_Not_Run_On_The_Initializing_Thread: handled separately.
• renovate/major-nunit: compile errors from the dependency bump.
• The [slow] still running ... InstanceMethodDataSource_DoesNotReuseEnumerationInstance lines are the deferred-enumeration placeholder. By design it stays in progress until the session ends. They are not a failure.

Tests

• New ContextProviderTests.GetOrCreateClassContext_ConcurrentCallers_RegisterASingleContext. It fails on main (5 contexts registered) and passes with the fix.
• FilterByDynamicAddedPropertyTests.Test1 now asserts that session and assembly contexts contain only its own class. It failed before the fix (40 classes) and passes in source-gen and --reflection modes.
• TUnit.UnitTests: 399/399, run 3 times.
• TUnit.Engine.Tests (net10.0, reflection): 337 passed. The only 2 failures are FSharp.Test and VB.Test, because those projects are not built locally. AOT variants were skipped.
• TestProject [EngineTest=Pass] in source-gen mode: 5724 passed, 0 failed.
• Repro5700: 3 runs in each mode, all passed.

Summary by CodeRabbit

• Bug Fixes
  • Filtered test runs now keep session, assembly, and class contexts aligned with the tests that will run.
  • Adding a dynamic test after filtering removes all tests from a class now restores that class and its assembly to the session.
  • Improved consistency when contexts are created concurrently, preventing duplicate context registrations.
  • Activity reporting handles concurrent changes to event data more safely.
  • Activity listener cleanup now stops only activities started in its own async flow.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-29T14:55:21.354898Z	91295cd	New commits

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: a0b9330b-2f7b-494e-81eb-5a11873df876

📥 Commits

Reviewing files that changed from the base of the PR and between 2f969fb and 91295cd.

📒 Files selected for processing (4)

• src/TUnit.Core/ContextProvider.cs
• src/TUnit.Core/Models/AssemblyHookContext.cs
• src/TUnit.Core/Models/ClassHookContext.cs
• tests/TUnit.UnitTests/ContextProviderTests.cs

🚧 Files skipped from review as they are similar to previous changes (1)

• src/TUnit.Core/ContextProvider.cs

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 4 remain after this review.

---

📝 Walkthrough

Walkthrough

The changes coordinate context creation and cache updates during filtered test discovery. They also bound activity report collection, scope activity tracking to its async flow, and adjust a keyed scheduling regression test.

Changes

Test Context Consistency

Layer / File(s)	Summary
Context creation and concurrency test src/TUnit.Core/ContextProvider.cs, tests/TUnit.UnitTests/ContextProviderTests.cs	Assembly and class context creation use a shared lock and cache checks before registering a context. A regression test checks concurrent requests and session registration.
Filtered tests and cache invalidation src/TUnit.Core/Models/*HookContext.cs, src/TUnit.Core/Models/TestSessionContext.cs, src/TUnit.Engine/TestDiscoveryService.cs, tests/TUnit.TestProject/FilterByDynamicAddedPropertyTests.cs, tests/TUnit.UnitTests/ContextProviderTests.cs	Filtered discovery retains matching tests in class contexts. Class, assembly, and session contexts update membership and invalidate test caches as needed. Tests check filtered contexts and reattachment after adding a dynamic test.

Activity Handling

Layer / File(s)	Summary
Bound activity report data src/TUnit.Engine/Reporters/Html/ActivityCollector.cs	Event, link, and tag enumeration stops at the initial array length. Returned arrays are trimmed to the number of entries copied.
Scope activity tracking to the async flow tests/TUnit.UnitTests/SessionActivityLifecycleTests.cs	The activity listener test scope records activities started while its instance is current in the async flow. Disposal restores the prior scope and retains cleanup of tracked activities.

Keyed Scheduling Regression Test

Layer / File(s)	Summary
Adjust the keyed scheduling reproduction tests/TUnit.TestProject/Bugs/5700/Repro.cs	Test2 no longer waits for Test1’s signal. Test1 still waits for Test2, and the comments describe the targeted scheduling behavior.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant TestDiscoveryService
  participant ClassHookContext
  participant AssemblyHookContext
  participant TestSessionContext
  TestDiscoveryService->>ClassHookContext: Retain matching test contexts
  ClassHookContext->>AssemblyHookContext: Remove class if no tests remain
  ClassHookContext->>AssemblyHookContext: Invalidate caches if tests remain
  AssemblyHookContext->>TestSessionContext: Invalidate session test caches

Loading

Merge Risk: ⚪ Minimal · up to 91295

No actionable PR-specific issue remains from this review; the change is mergeable after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 91295

The changes appear confined to test-runner context membership. Review found no new privilege boundary or external exposure, but concurrent lifecycle behavior and incomplete security coverage leave some uncertainty.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The observed effects remain within test-context membership and aggregate caches for the owning session; no cross-service or privileged sink is established by the inspected paths.

Trust Boundaries and Controls

• inferred — Code already running in a test process can access public context methods, but the changed assembly path does not add a new entrypoint or authority transition. This does not establish safety for uninspected uses of those contexts.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 17.24% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 29 functions across 10 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the three main fixes: leaked hook contexts, the ActivityCollector race, and the Repro5700 rendezvous.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit checks each context’s place,
Then keeps the tests that pass the case.
The tags fit safely in their rows,
The async scope tracks what it knows.
Test2 starts; the queue runs clear,
The rabbit bounds away with cheer.}

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Review of #6933

Overall this is a solid, well-scoped fix, and the root-cause analysis in the description matches the code.

What I checked

• ContextProvider double-checked creation: the lock-free fast path is preserved. The assembly context is resolved before taking _creationLock, so there is no nested locking or re-entrancy. Orphaned contexts from lost GetOrAdd races can no longer be constructed, since the constructor's parent-registration side effect now only runs once. The new concurrency test targets exactly this.
• RetainTests and RemoveExcludedTestsFromHookContexts: the class lock is released before calling into AssemblyContext, so lock ordering is safe. Cache invalidation is propagated up to assembly and session on both the empty and non-empty paths. Gating on isForExecution && finalTests.Count != allTests.Count keeps the common path free of extra work. Because this sits in TestDiscoveryService, it applies to source-gen and reflection modes alike.
• ActivityCollector: the bounded fill plus TrimToFilled is a reasonable defensive fix, and it stops exceptions escaping through Activity.Stop().
• Test fixes: the AsyncLocal-scoped listener and the one-directional rendezvous in Repro5700 both address the root cause of the flake rather than just raising timeouts. The Repro5700 test still catches the Running TestsCases produced by Arguments not in parallel with each other but parallel with any other test #5700 regression.

Minor suggestions (non-blocking)

1. TrimToFilled returns null when nothing was filled, and ToReportKeyValues may now return null where it previously returned an empty array. Please confirm all callers and serializers treat null and empty the same way.
2. ContextProvider now uses one lock shared by assembly and class creation. That's fine at once-per-type frequency, but a comment noting that the class path deliberately resolves the assembly context outside the lock would protect against a future refactor introducing lock nesting.
3. The AsyncLocal-based listener filter in SessionActivityLifecycleTests relies on activities being started within the test's async flow. If any test starts activities from a thread-pool callback that doesn't flow ExecutionContext, they'd be silently untracked. That's acceptable for this test, but worth keeping in mind.

No blocking issues found. LGTM.

[greptile-apps]

Confidence Score: 5/5

[High risk] Fixes race conditions in test context creation and filtering.

The PR appears safe to merge; no new actionable issue or outstanding previous finding was identified.

Summary

The PR removes filtered-out tests from execution hook contexts, prevents duplicate context registration during concurrent creation, bounds activity collection during concurrent mutation, narrows test activity cleanup, and simplifies the keyed-test rendezvous. The changes since the previous review add safe context reattachment and bound the concurrency test’s worker count.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Built tests] --> B[Execution filter]
  B --> C[Retain selected tests in hook contexts]
  C --> D[Detach empty class and assembly contexts]
  D --> E[Dynamic test added later]
  E --> F[Reattach class and assembly; invalidate caches]

Loading

_{Reviews (2) · Last reviewed commit: "fix(core): reattach a filtered-out class..."}

[thomhurst]

Replies to the github-actions review. Changes are in 36acab4 and 91295cd.

1. TrimToFilled returning null. No behaviour change. ToReportKeyValues already returned null when an activity had no tags, and events/links were already null when empty, so null was the existing "empty" value for every consumer. TrimToFilled only returns null when the concurrent mutation leaves nothing, which is the same case.
2. Lock nesting comment. Added in 36acab4. The class path resolves the assembly context before it takes _creationLock, so the two creation paths never nest the lock.
3. AsyncLocal listener scope. Acknowledged. That trade-off is acceptable for this test.

Also fixed Greptile's P1 (91295cd). A dynamic test added to a class whose built tests were all filtered out now reattaches the class, and the assembly if needed, to the hook contexts.

[github-actions]

Review of #6933

I read the diff and didn't run the tests. I found no blocking issues.

Hook-context fixes

• ContextProvider now creates contexts with double-checked locking. This removes the orphaned-context race, because constructors register with their parent and the old GetOrAdd factory could run on the losing side of a race. The assembly context is resolved before the lock is taken, so the lock never nests. The concurrent-creation unit test covers this.
• RetainTests / Detach / _detached handle filtered-out tests, and AddTest reattaches a class after filtering emptied it. The lock order is class → assembly → session. I checked that the assembly's TestClasses copies its list under the lock and enumerates outside it, so there is no path back into a class while an assembly lock is held. I see no deadlock risk.
• RemoveExcludedTestsFromHookContexts only runs for execution and only when the filter excluded something, so the common path costs nothing.

Non-blocking suggestions

1. _cachedAllTests ??= is not synchronised with invalidation. A reader that computed the array just before an invalidation can store it afterwards and leave a stale cache. This was already the case, but the PR adds more invalidation points. A version counter, or computing and assigning under the lock, would close the gap. It only matters if hooks read AllTests while dynamic tests are being added.
2. ActivityCollector now bounds the fill loops and trims the arrays. That is the right defensive fix. TrimToFilled returning null for an empty result also matches the existing null-when-empty convention.
3. The test-side changes (Repro5700 waiting on one side only, and ActivityListenerScope tracking only activities from its own async flow) address the cause of each flake rather than adding timeouts.

Overall this is a well-reasoned fix with regression tests, and I'd approve it.

[thomhurst]

Thanks. On the non-blocking points: the unsynchronised _cachedAllTests ??= race existed before this PR. It only matters when a hook reads AllTests while dynamic tests are being added concurrently, so I'm leaving it for a follow-up and not widening this flake-fix PR.