perf: consolidate test dedup + hook register guards (#5528 A)

[thomhurst]

Part A of the perf work catalogued in #5528. Tackles the two P0 architectural items.

Changes

1. Remove duplicate test-execution dedup layer

`TestRunner` already holds a `ConcurrentDictionary<string, TaskCompletionSource>` to prevent double-execution, and it's the sole entry point for both the scheduler and dependency recursion. `TestExecutionGuard` wrapped every call with a second identical mechanism — every test paid for two TCS allocations and two dictionary ops when one suffices.

• Deleted `TestExecutionGuard`
• `TestCoordinator.ExecuteTestAsync` now calls `ExecuteTestInternalAsync` directly
• Removed the DI registration

2. Guard per-test assembly/class hook registrations

`RegisterAfterAssemblyHook` and `RegisterAfterClassHook` were called for every test but only need to register once per assembly / per class. Each redundant call heap-allocated a `CancellationTokenRegistration` into an unbounded `ConcurrentBag`.

Added `ConcurrentHashSet` and `ConcurrentHashSet` guards mirroring the existing `_sessionHookRegistered` pattern. For 10k tests across 5 assemblies / 200 classes: ~19,795 fewer registrations.

Test plan

• `dotnet build TUnit.Engine` — 0 errors
• `TUnit.Engine.Tests` — 148/148 pass (99 env-gated skips)
• `TUnit.UnitTests` — 180/180 pass

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric	Results
Complexity	0

View in Codacy

_{TIP This summary will be updated as you push new changes. Give us feedback}

[claude]

Code Review

Overall this is a solid, well-reasoned performance PR. The analysis in the description is accurate and the implementation is clean. A couple of things worth flagging.

---

✅ TestExecutionGuard removal — correct

The claim holds up. All callers of ITestCoordinator.ExecuteTestAsync go exclusively through TestRunner.ExecuteTestAsync (confirmed: TestScheduler, ConstraintKeyScheduler, and recursive dependency calls in TestRunner.ExecuteTestInternalAsync all route through TestRunner). The ConcurrentDictionary-based dedup in TestRunner covers every path, making the second guard genuinely redundant. Good catch.

---

✅ ConcurrentHashSet guard for assembly/class hooks — correct and actually more correct than the session hook pattern

The new ConcurrentHashSet<T>.Add() guard is atomically safe. The existing _sessionHookRegistered pattern it's modeled after has a subtle TOCTOU race:

// AfterHookPairTracker.cs:46-51
if (_sessionHookRegistered)   // Thread A reads false
    return;
_sessionHookRegistered = true; // Thread B also reads false, both proceed

Two threads racing on RegisterAfterTestSessionHook could both pass the if check before either writes true, resulting in double registration of the session hook callback. This doesn't affect the hook execution itself (which uses GetOrCreateAfterTestSessionTask with its lock guard), but it wastes a CancellationTokenRegistration. The new ConcurrentHashSet.Add() approach is strictly better. Consider updating the session hook to use the same pattern for consistency:

private readonly ConcurrentHashSet<int> _sessionHookRegistered = new(); // or Interlocked.CompareExchange on an int flag

---

⚠️ Single cancellation token registration per assembly/class

With the guard in place, the After Assembly/Class hook cancellation callback is registered only on the first test's per-test cancellation token. If that test completes before a mid-run session cancellation, whether the hook fires depends on whether the first test's CT is linked to the session CT. If per-test CancellationTokenSources are derived via CreateLinkedTokenSource from the session token, cancellation cascades correctly. If they're independent (e.g. per-test timeout tokens), a session cancellation after the first test completes would bypass the cancellation path for that assembly.

The normal-completion path (GetOrCreateAfterAssemblyTask direct call) handles the non-cancellation case, so this is only a risk when execution is aborted abnormally. Worth verifying that per-test tokens are always linked to the session token, and if so, adding a brief comment here to explain why the single registration is safe.

---

Minor: TestRunner._executingTests never removes entries

This isn't introduced by this PR, but the PR now relies on it as the correctness foundation: the dictionary grows O(test count) across the run and is never pruned. For very large suites (100k+ tests), this is meaningful resident memory. Not a blocker, but worth a // intentional: dedup across re-entrancy comment or a future TryRemove after tasks complete (once the re-entry window closes).

---

Summary: Both core changes are correct and achieve their stated goals. The main actionable item is the _sessionHookRegistered TOCTOU inconsistency — the new code is more correct than the pattern it claims to mirror, so either fix the session hook too or document the known difference.

[claude]

Code Review

Good, well-scoped performance PR with clear reasoning. Two genuine concerns worth discussing.

---

1. The CT-is-session-scoped assumption is load-bearing and unenforced

In AfterHookPairTracker, the assembly/class guards now ensure only the first registered cancellation token fires the After hook. The comment explains why this is safe:

"The CancellationToken passed in is always the session-scoped token..."

This is correct today — TestExecutor.cs never surfaces per-test CTs to these methods. But the type signature accepts any CancellationToken, so there's nothing preventing a future caller from passing a narrower token. If that happens, subsequent tests in the same assembly/class would silently skip registration, and their After hooks would not fire when that CT cancels.

Suggestion: Either encode the invariant in the API (e.g., rename the parameter to sessionCancellationToken) or add a debug-only assertion at the call site in TestExecutor.cs verifying the token is session-scoped. The comment is good, but the invariant is silently broken by callers who don't read it.

---

2. Removing TestExecutionGuard makes dedup an implicit TestRunner invariant

The old guard was a named, DI-registered abstraction with a clear SRP contract. Now the dedup guarantee is an implicit consequence of TestRunner being the sole ITestCoordinator caller. This is accurate today, but:

• ITestCoordinator is an interface — any future caller (retry coordinator, test harness in tests) that calls ExecuteTestAsync directly would silently lose dedup protection.
• The invariant is only discoverable by reading the comment on TestCoordinator.ExecuteTestAsync.

Suggestion: Consider adding an XML doc note on ITestCoordinator.ExecuteTestAsync stating callers must go through TestRunner to preserve dedup semantics. This is lower priority than concern 1 — the performance gain is real and the current call graph is clear.

---

What's good

• Interlocked fix for _sessionHookRegistered is a genuine race condition fix, not just a perf improvement. The old volatile bool check-then-set was a TOCTOU bug; CompareExchange is the correct pattern here.
• The ConcurrentHashSet guards are clean and mirror the existing session pattern well. The Add-returns-bool idiom avoids a separate lookup.
• The TestRunner _executingTests comment clearly explains the intentional retention-for-session design.
• PR description is excellent — quantified impact, clear reasoning per change, solid test plan.

The changes are correct and the performance improvement is real. The concerns above are about future maintenance fragility, not current correctness.

[claude]

Code Review — Follow-up

All items raised in the previous two reviews have been addressed. Here's the checklist:

Previous concerns resolved

Concern	Status
TOCTOU race on _sessionHookRegistered (volatile bool check-then-set)	✅ Fixed — Interlocked.CompareExchange on int flag
CT-is-session-scoped assumption unenforced (Review 2, concern 1)	✅ Fixed — parameter renamed to sessionCancellationToken on all three methods
ITestCoordinator.ExecuteTestAsync callers must go through TestRunner (Review 2, concern 2)	✅ Fixed — <remarks> block added to the interface documenting the invariant
_executingTests retention unexplained minor note (Review 1)	✅ Fixed — comment added explaining intentional session-scoped growth and the O(test count) bound
CT invariant needs call-site documentation	✅ Fixed — <param> XML docs added to RegisterAfterAssemblyHook and RegisterAfterClassHook

Remaining observations (informational, not blocking)

ConcurrentHashSet<Assembly> / ConcurrentHashSet<Type> sizing: Both sets are sized at default capacity and will hold one entry per distinct assembly/class for the session lifetime. For typical suites this is negligible. Worth being aware of if a suite has thousands of distinct test classes, though the per-entry overhead is far smaller than the CancellationTokenRegistration allocations it replaces.

_registrations bag: Now populated far less (one entry per assembly/class + one for session, vs. one per test). The cleanup path downstream is unchanged — this is purely a win.

Verdict

The changes are correct, well-documented, and the PR description is exemplary. The dedup consolidation is architecturally sound (single entry-point owns the ledger), and the Interlocked fix for the session hook is a genuine race correction that improves on the original pattern.

Approving.