perf: skip TimeoutHelper wrap when no explicit [Timeout] is set (#5711)

[thomhurst]

Closes #5711

Summary

Every test was running through TimeoutHelper.ExecuteWithTimeoutAsync using the built-in 30-minute default, paying a linked CancellationTokenSource + TaskCompletionSource + Task.WhenAny cost per test for a ceiling almost no one relies on.

• TestBuilder and TestBuilderPipeline no longer seed TestDetails.Timeout from the default.
• TestCoordinator only feeds TestExecutor a non-null timeout when either a [Timeout] attribute set TestDetails.Timeout, or the user opted into a session-wide default via TUnitSettings.Timeouts.DefaultTestTimeout.
• TestExecutor's existing null-timeout fast path then invokes the test directly with no wrapping overhead.

User-visible behavior is preserved:

• [Timeout(...)] still enforces its duration (verified against TimeoutCancellationTokenTests).
• Programmatic DefaultTestTimeout overrides are tracked via a new internal DefaultTestTimeoutExplicitlySet flag so they continue to apply to every test.

Test plan

• dotnet build TUnit.Engine/TUnit.Engine.csproj -c Release -f net10.0 — clean
• TUnit.Core.SourceGenerator.Tests — 116 passed, 0 failed (1 pre-existing skip)
• TUnit.TestProject BasicTests filter — 3 passed
• TimeoutDoesNotFireTests (positive path) — passes
• TimeoutCancellationTokenTests/BasicTest with explicit [Timeout(5000)] — still correctly times out via TimeoutHelper

[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

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[claude]

Code Review

Summary

This is a well-targeted performance improvement. The core idea — removing per-test + + overhead for the vast majority of tests that have no explicit timeout — is sound, and the implementation is clean. No previous review comments to address.

---

Concern 1: Static singleton access in (architectural)

In TestCoordinator.ExecuteTestLifecycleAsync:

var timeouts = TUnitSettings.Default.Timeouts;
TimeSpan? testTimeout = test.Context.Metadata.TestDetails.Timeout;
if (testTimeout is null && timeouts.DefaultTestTimeoutExplicitlySet)
{
    testTimeout = timeouts.DefaultTestTimeout;
}

TUnitSettings.Default is pulled from the static singleton inside an instance method, which is inconsistent with how TestCoordinator receives everything else (via constructor DI). This makes unit testing this path awkward — you can't substitute a different TimeoutSettings without mutating the global singleton.

Better approach: Inject TimeoutSettings (or just TUnitSettings) in the constructor alongside the other dependencies, and store it as a field:

// constructor
private readonly TimeoutSettings _timeoutSettings;

public TestCoordinator(..., TimeoutSettings timeoutSettings)
{
    ...
    _timeoutSettings = timeoutSettings;
}

// usage
TimeSpan? testTimeout = test.Context.Metadata.TestDetails.Timeout;
if (testTimeout is null && _timeoutSettings.DefaultTestTimeoutExplicitlySet)
{
    testTimeout = _timeoutSettings.DefaultTestTimeout;
}

Why it matters: The static singleton call embeds a hidden dependency that won't show up in the constructor signature. It also means any future test that exercises this code path without the real singleton in place will silently use the wrong settings.

---

Concern 2: Public API doc gap

The XML doc on DefaultTestTimeout still says only:

Default: 30 minutes. Overridden per-test by TimeoutAttribute.

But the behavior has meaningfully changed: leaving DefaultTestTimeout unset now means tests run with no timeout wrapping at all, not with a 30-minute ceiling. A user who discovers a stuck test and reads this doc will expect a 30-minute backstop exists — it no longer does.

Suggested addition to the existing summary:

If this property is never set, tests without a [Timeout] attribute run without any timeout wrapper (no overhead). The 30-minute fallback only applies when this property is explicitly assigned.

This is especially important because DefaultTestTimeoutExplicitlySet is internal — users have no programmatic way to discover whether the opt-in is active.

---

Minor: Issue references in code comments

Two comments reference #5711:

• TimeoutSettings.cs: // When false, tests without [Timeout] bypass TimeoutHelper entirely (#5711)
• TestCoordinator.cs: // The built-in 30-minute default is intentionally not applied per-test (#5711)

Per project style, issue numbers belong in commit/PR messages, not in source comments — they rot and become dead links as the repo moves. The why (applying the 30-minute built-in default to every test would be pure overhead) is the valuable part; that can stand alone without the ticket reference.

---

What's good

• The DefaultTestTimeoutExplicitlySet flag is the right mechanism: a simple bool on the setter, no extra API surface, no change to the public DefaultTestTimeout type.
• Removing the seeding from four TestDetails construction sites consistently is clean — no partial states possible.
• The TestExecutor null-timeout fast path was already in place, so the engine-side change (in TestCoordinator) is the only new logic.
• The test plan in the PR description is thorough and covers both the positive path (no-op for unset) and the timeout-fires path (TimeoutCancellationTokenTests).

[claude]

Code Review

Summary

This is a well-targeted performance improvement. The core idea — removing per-test CancellationTokenSource + TaskCompletionSource + Task.WhenAny overhead for the vast majority of tests that have no explicit timeout — is sound, and the implementation is clean. No previous review comments to address.

---

Concern 1: Static singleton access in TestCoordinator (architectural)

In TestCoordinator.ExecuteTestLifecycleAsync:

var timeouts = TUnitSettings.Default.Timeouts;
TimeSpan? testTimeout = test.Context.Metadata.TestDetails.Timeout;
if (testTimeout is null && timeouts.DefaultTestTimeoutExplicitlySet)
{
    testTimeout = timeouts.DefaultTestTimeout;
}

TUnitSettings.Default is pulled from the static singleton inside an instance method, which is inconsistent with how TestCoordinator receives everything else (via constructor DI). This makes unit testing this path awkward — you cannot substitute a different TimeoutSettings without mutating the global singleton.

Better approach: Inject TimeoutSettings (or TUnitSettings) in the constructor alongside the other dependencies and store it as a field:

// constructor
private readonly TimeoutSettings _timeoutSettings;

public TestCoordinator(..., TimeoutSettings timeoutSettings)
{
    _timeoutSettings = timeoutSettings;
}

// usage
TimeSpan? testTimeout = test.Context.Metadata.TestDetails.Timeout;
if (testTimeout is null && _timeoutSettings.DefaultTestTimeoutExplicitlySet)
{
    testTimeout = _timeoutSettings.DefaultTestTimeout;
}

Why it matters: The static singleton call embeds a hidden dependency that won't show up in the constructor signature. Any future test exercising this code path without the real singleton in place will silently use the wrong settings.

---

Concern 2: Public API doc gap

The XML doc on DefaultTestTimeout still says only:

Default: 30 minutes. Overridden per-test by TimeoutAttribute.

But the behavior has meaningfully changed: leaving DefaultTestTimeout unset now means tests run with no timeout wrapping at all, not with a 30-minute ceiling. A user who discovers a hung test and reads this doc will expect a 30-minute backstop — it no longer exists.

Suggested addition to the existing summary:

If this property is never explicitly set, tests without a [Timeout] attribute run without any timeout wrapper. The 30-minute fallback only applies when this property is explicitly assigned.

This is especially important because DefaultTestTimeoutExplicitlySet is internal — users have no programmatic way to discover whether the opt-in is active.

---

Minor: Issue references in code comments

Two comments reference #5711:

• TimeoutSettings.cs: // When false, tests without [Timeout] bypass TimeoutHelper entirely (#5711)
• TestCoordinator.cs: // The built-in 30-minute default is intentionally not applied per-test (#5711)

Issue numbers belong in commit/PR messages, not in source comments — they become dead references as the repo evolves. The why (applying the 30-minute built-in default to every test would be pure overhead) is the valuable part and can stand alone without the ticket reference.

---

What's good

• The DefaultTestTimeoutExplicitlySet flag is the right mechanism: a simple bool set in the property setter, no extra public API surface, no change to the DefaultTestTimeout type.
• Removing the timeout seeding from all four TestDetails construction sites consistently is clean — no partial states possible.
• The TestExecutor null-timeout fast path was already in place, so the engine-side change in TestCoordinator is the only new logic needed.
• The test plan in the PR description is thorough and covers both the positive path (no overhead for unset) and the timeout-fires path (TimeoutCancellationTokenTests).

[thomhurst]

Pushed 8ef26c9b0 addressing review feedback:

• Doc gap (Concern 2): DefaultTestTimeout XML doc now explicitly states that leaving the property unset means tests without [Timeout] run with no timeout wrapper at all, and the 30-minute fallback only kicks in when the property is explicitly assigned.
• Minor (issue refs in comments): Dropped (#5711) suffixes from both TimeoutSettings.cs and TestCoordinator.cs; kept the WHY text.
• Micro-tidy in TestCoordinator: Moved the TUnitSettings.Default.Timeouts lookup inside the testTimeout is null branch so the static singleton access is skipped on the common [Timeout]-set path.

Concern 1 (DI of TimeoutSettings) deferred as a follow-up. TUnitSettings.Default is the established access pattern used consistently by HookTimeoutHelper, TestScheduler, TestRunner, and TUnitMessageBus on the engine side — threading TimeoutSettings through constructor DI just for TestCoordinator would leave the architecture half-converted. Converting the whole engine to inject TUnitSettings is worth doing, but it's a larger DI-cleanup task separate from the perf fix. I'll open a follow-up for it if you agree.

Verified locally:

• dotnet build TUnit.slnx -c Release — 0 errors
• TUnit.Core.SourceGenerator.Tests (net10.0) — 116 passed, 1 pre-existing skip
• TUnit.TestProject --treenode-filter "/*/*/BasicTests/*" (net10.0) — 3/3 passed

Codacy reported 1 minor CodeStyle issue; annotations aren't exposed through GitHub or public API so I couldn't pinpoint it exactly. The revised block replaces the previous mixed var / explicit-type variable pair with a single var and drops the timeouts local from the fast path, which should address the most likely candidates.

[claude]

Code Review

This is a clean, well-motivated performance optimization. The removal of the per-test CTS + TCS + Task.WhenAny overhead for the common no-timeout case is straightforward and correct. The previous review feedback has been addressed: the XML doc now clearly states the behavioral change, issue-reference comments are gone from source, and the static singleton lookup is deferred until it's actually needed.

---

What works well

• The null-first check in TestCoordinator short-circuits cleanly: [Timeout] still wins, and the static singleton is only touched when testTimeout is null.
• TimeoutAttribute.OnTestStart still writes to TestDetails.Timeout directly, so the attribute path is completely unaffected.
• The fast path in TestExecutor (lines 213-216) is already in place; this PR correctly feeds it null when no timeout is wanted, so no changes to TestExecutor were needed.
• Removing the 5× repeated Timeout = TUnitSettings.Default.Timeouts.DefaultTestTimeout from the object initialisers is an unambiguous improvement.

---

Concern: dual-flag pattern vs. nullable backing field

The _defaultTestTimeoutExplicitlySet boolean exists solely to distinguish "never assigned" from "assigned to 30 minutes". An alternative that eliminates the flag entirely:

// TimeoutSettings.cs
private TimeSpan? _defaultTestTimeout;   // null = unset; non-null = user-assigned

public TimeSpan DefaultTestTimeout
{
    get => _defaultTestTimeout ?? TimeSpan.FromMinutes(30);
    set { ValidatePositive(value); _defaultTestTimeout = value; }
}

internal TimeSpan? ExplicitDefaultTestTimeout => _defaultTestTimeout;

// TestCoordinator.cs
testTimeout ??= TUnitSettings.Default.Timeouts.ExplicitDefaultTestTimeout;

This removes the boolean flag and makes the intent self-evident: null means "not configured", which is a well-understood .NET idiom. The public get still returns 30 minutes for any user code that reads it programmatically (so it's not a breaking change for consumers), and the internal property surfaces the raw optional.

That said, the current dual-flag approach is not wrong — it's just carrying slightly more complexity than necessary.

---

Minor: memory visibility of _defaultTestTimeoutExplicitlySet

_defaultTestTimeoutExplicitlySet is a plain bool field with no volatile keyword. In the normal TUnit flow, settings are configured before tests run on separate threads, so in practice there will be a happens-before boundary (likely the thread-pool task handoff). But if a user ever sets DefaultTestTimeout concurrently with test execution starting, the write might not be visible to the reading thread under the ECMA memory model.

Marking it volatile would be the safest fix and has zero runtime cost for a single bool:

private volatile bool _defaultTestTimeoutExplicitlySet;

---

Test coverage gap

The existing tests cover:

• No timeout path (TimeoutDoesNotFireTests)
• Explicit [Timeout(...)] attribute path

The new code path — TUnitSettings.Timeouts.DefaultTestTimeout explicitly set, no [Timeout] on the test — does not appear to have a dedicated test. The DefaultTestTimeoutExplicitlySet branch in TestCoordinator (lines 324-330) is the novel logic in this PR and is worth covering with a test that:

1. Sets TUnitSettings.Timeouts.DefaultTestTimeout = TimeSpan.FromSeconds(1)
2. Runs a test that takes longer than 1 second
3. Asserts it times out

Without this, a regression in that branch would be invisible.

---

DI deferral acknowledged

The TUnitSettings.Default static access in TestCoordinator is consistent with the rest of the engine and the deferral rationale (half-converted DI would be worse) is sound. +1 for a follow-up issue.

---

Overall this is a good change — the optimization is real, the correctness reasoning is solid, and the documentation is now accurate. The nullable-backing-field approach and volatile annotation are worth considering, and a test for the DefaultTestTimeout-explicit path would close the coverage gap.

[thomhurst]

Pushed 7195d82f2 addressing the remaining feedback:

Non-blocking review suggestion (nullable backing field). Replaced the TimeSpan _defaultTestTimeout + bool _defaultTestTimeoutExplicitlySet pair with a single TimeSpan? _defaultTestTimeout:

• Public DefaultTestTimeout getter now returns _defaultTestTimeout ?? TimeSpan.FromMinutes(30) — same observable default for any user code reading the property, no breaking change.
• Setter just assigns; no more secondary flag to keep in sync.
• New internal ExplicitDefaultTestTimeout surfaces the raw TimeSpan? for the engine path.
• TestCoordinator collapses to a single null-coalescing chain:

  var testTimeout = test.Context.Metadata.TestDetails.Timeout
      ?? TUnitSettings.Default.Timeouts.ExplicitDefaultTestTimeout;

  Still skips the static-singleton lookup on the common [Timeout]-set path, and the wrap-skip semantics are unchanged (null flows through to TestExecutor's existing null-timeout fast path).

Codacy ACTION_REQUIRED (1 minor CodeStyle). Same commit. The specific rule isn't exposed through the GitHub annotations pipeline or the public Codacy API for this account, but the rewrite removes the most plausible candidates: the mixed-intent dual-field declaration, the nested if + intermediate timeouts local, and the redundant assignment in the setter. If a separate issue remains, it'll retrigger on this push and I'll handle it specifically.

Verified locally on net10.0:

• dotnet build TUnit.slnx -c Release — 0 errors
• TUnit.Core.SourceGenerator.Tests — 116 passed, 1 pre-existing skip
• TUnit.TestProject --treenode-filter "/*/*/BasicTests/*" — 3/3 passed
• TUnit.TestProject --treenode-filter "/*/*/TimeoutCancellationTokenTests/*" — exercises both branches: DefaultTest (no [Timeout], no default set → no wrap, completes in 5s) passes; the 7 ExpectedResult.Failure tests with [Timeout(5_000)] fire and fail as designed.

[claude]

Code Review — Round 4

This is a clean, focused performance optimization. After three rounds of review the implementation is solid. Here's the current state:

✅ Previous Concerns Addressed

• Nullable backing field (TimeSpan? _defaultTestTimeout) cleanly replaced the dual TimeSpan + bool pair — idiomatic .NET and self-documenting.
• XML doc on DefaultTestTimeout now clearly explains that leaving the property unset means no timeout wrapper is applied.
• Issue references removed from source comments (proper Git-history discipline).
• The TUnitSettings.Default singleton lookup was moved inside the testTimeout is null branch, so tests with an explicit [Timeout] attribute skip the static read entirely.
• DI deferral rationale accepted — consistent with HookTimeoutHelper, TestScheduler, TestRunner, and TUnitMessageBus which all use TUnitSettings.Default directly.

---

⚠️ Remaining Concern: Test Coverage Gap

The new logic branch in TestCoordinator.ExecuteTestLifecycleAsync —

var testTimeout = test.Context.Metadata.TestDetails.Timeout
    ?? TUnitSettings.Default.Timeouts.ExplicitDefaultTestTimeout;

— still has no dedicated test exercising the right-hand side of the ??. That is: ExplicitDefaultTestTimeout is non-null (user assigned TUnitSettings.Timeouts.DefaultTestTimeout) but no [Timeout] attribute is on the test.

Without this test, a future refactor that accidentally short-circuits ExplicitDefaultTestTimeout won't be caught. Since the whole performance benefit is contingent on getting the precedence right, a test that asserts the explicit default is honoured would make this change more robust.

---

Observation: Behavioral Change Visibility

This is a silent removal of the implicit 30-minute safety net. Tests that currently rely on that ceiling (e.g. runaway integration tests or accidental infinite loops) will now spin indefinitely unless DefaultTestTimeout is explicitly configured. The change is intentional and well-documented in the DefaultTestTimeout XML doc — worth a prominent entry in the changelog/release notes so adopters aren't surprised when a stuck test no longer self-terminates.

---

Everything Else Looks Good

• All five TestDetails initialisers consistently drop the Timeout = ... assignment — no stray seeding left behind.
• ExplicitDefaultTestTimeout exposes only what's needed (internal, read-only) without leaking TimeSpan? into the public surface.
• The comment in TestCoordinator correctly explains the why (opt-in vs. implicit overhead).
• Dual-mode compatibility: TestDetails.Timeout is populated by the [Timeout] attribute processor in both source-gen and reflection paths, so null-as-default works correctly in both modes.

Summary: Solid PR with one outstanding item (test for the explicit DefaultTestTimeout path) and one doc recommendation (changelog note). Approve once the test gap is addressed or explicitly deferred with a tracking issue.

[thomhurst]

Added test coverage for the ExplicitDefaultTestTimeout fallback in TUnit.UnitTests/TUnitSettingsTests.cs (commit b43f149):

• ExplicitDefaultTestTimeout_Is_Null_When_Unset — a freshly-constructed TimeoutSettings returns null from the internal getter while the public DefaultTestTimeout still reports the 30-minute fallback. Confirms the right-hand side of the coalesce is null when the user never assigns DefaultTestTimeout (the no-wrap fast path).
• ExplicitDefaultTestTimeout_Returns_Assigned_Value — once the setter is called, the internal getter surfaces the assignment.
• Configured_Default_Timeout_Fires_On_Hanging_Test — mirrors TestCoordinator's call site by feeding ExplicitDefaultTestTimeout into the same TimeoutHelper.ExecuteWithTimeoutAsync. A non-cooperative 30s hang against a 200ms configured default throws TimeoutException as expected. Uses a private CTS in finally to terminate the hang task rather than leaking it to process exit.

All three tests reuse the existing SnapshotSettings / RestoreSettings hooks in TUnitSettingsTests, so no new global-state leakage. Full TUnit.UnitTests (183 tests) green on net10.0.

[claude]

Code Review — Round 5

Previous Concerns Status

All concerns from Rounds 3–4 have been addressed:

• ✅ Nullable backing field () replaced the pair
• ✅ internal property is clean and self-documenting
• ✅ XML doc on accurately describes the new behavior
• ✅ Issue references removed from source comments
• ✅ Test coverage added via TUnitSettingsTests

---

🐛 Bug: No-retry fast path does NOT apply ExplicitDefaultTestTimeout

TestCoordinator has two separate execution paths, and they handle the timeout inconsistently:

No-retry fast path (line 154, the common case for most tests):

var testTimeout = test.Context.Metadata.TestDetails.Timeout;
// ^ ExplicitDefaultTestTimeout is never consulted here
await _testExecutor.ExecuteAsync(test, _testInitializer, cancellationToken, testTimeout);

Retry path via ExecuteTestLifecycleAsync (line 379–380):

var testTimeout = test.Context.Metadata.TestDetails.Timeout
    ?? TUnitSettings.Default.Timeouts.ExplicitDefaultTestTimeout;  // ✅ correct

Because the no-retry path was inlined as a hot-path optimisation (introduced in #5714), it needs to be kept in sync with the logic in ExecuteTestLifecycleAsync. Right now it isn't. The effect:

• A user sets TUnitSettings.Timeouts.DefaultTestTimeout = TimeSpan.FromSeconds(30) to catch runaway tests.
• Their tests have no [Retry] attribute (the common case — retryLimit == 0).
• They go through the no-retry fast path, which passes null to TestExecutor, and the timeout is silently ignored.
• Only tests that also have [RetryN] actually honour the configured default.

Fix — apply the same coalesce in the inlined fast path:

// line 154 area — no-retry fast path
var testTimeout = test.Context.Metadata.TestDetails.Timeout
    ?? TUnitSettings.Default.Timeouts.ExplicitDefaultTestTimeout;
await _testExecutor.ExecuteAsync(test, _testInitializer, cancellationToken, testTimeout);

This is the only structural issue remaining.

---

⚠️ RestoreSettings() cannot fully restore the null state

SnapshotSettings() reads through the public getter:

_savedTestTimeout = TUnitSettings.Default.Timeouts.DefaultTestTimeout;
// returns _defaultTestTimeout ?? TimeSpan.FromMinutes(30) — always non-null

RestoreSettings() then writes back that value through the public setter, which sets _defaultTestTimeout to TimeSpan.FromMinutes(30) (non-null) rather than restoring it to null. After TUnitSettingsTests runs, TUnitSettings.Default.Timeouts.ExplicitDefaultTestTimeout is permanently TimeSpan.FromMinutes(30) for the rest of the process.

The test comment acknowledges this and works around it by using new TimeoutSettings() — that's correct for ExplicitDefaultTestTimeout_Is_Null_When_Unset. But once the bug above (no-retry path) is fixed, every subsequent test without [Timeout] running in the same process will see a 30-minute timeout applied via ExplicitDefaultTestTimeout.

A clean fix would be an internal reset escape hatch:

// TimeoutSettings.cs
internal void ResetDefaultTestTimeout() => _defaultTestTimeout = null;

// TUnitSettingsTests.cs — RestoreSettings()
if (_savedExplicitTestTimeout.HasValue)
    TUnitSettings.Default.Timeouts.DefaultTestTimeout = _savedExplicitTestTimeout.Value;
else
    TUnitSettings.Default.Timeouts.ResetDefaultTestTimeout();

---

Changelog note (carried from Round 4)

The silent removal of the implicit 30-minute safety net is still worth a release-notes entry. Users who relied on that backstop to surface stuck tests will find them spinning indefinitely unless they opt in via DefaultTestTimeout.

---

What's good

• The new tests are well-structured: ExplicitDefaultTestTimeout_Is_Null_When_Unset correctly uses a fresh instance to sidestep the singleton-restore problem.
• hangCts.Cancel() in the finally block of Configured_Default_Timeout_Fires_On_Hanging_Test prevents a 30-second leak — good hygiene.
• The [NotInParallel] class attribute prevents concurrent singleton mutation. Correct.
• The internal-only ExplicitDefaultTestTimeout property keeps the nullable state from leaking into the public API.

Summary: One real bug (no-retry fast path skips ExplicitDefaultTestTimeout) and one test-isolation issue (restore cannot recover null). The no-retry bug should be fixed before merge; the isolation issue becomes important only once that fix lands.

[claude]

Code Review — Round 5

Previous Concerns Status

All concerns from Rounds 3–4 have been addressed:

• Nullable backing field (TimeSpan? _defaultTestTimeout) replaced the TimeSpan + bool pair
• ExplicitDefaultTestTimeout internal property is clean and self-documenting
• XML doc on DefaultTestTimeout accurately describes the new behavior
• Issue references removed from source comments
• Test coverage added via TUnitSettingsTests

---

Bug: No-retry fast path does NOT apply ExplicitDefaultTestTimeout

TestCoordinator has two separate execution paths, and they handle the timeout inconsistently.

No-retry fast path (line 154, the common case for most tests):

var testTimeout = test.Context.Metadata.TestDetails.Timeout;
// ExplicitDefaultTestTimeout is never consulted here
await _testExecutor.ExecuteAsync(test, _testInitializer, cancellationToken, testTimeout);

Retry path via ExecuteTestLifecycleAsync (line 379-380):

var testTimeout = test.Context.Metadata.TestDetails.Timeout
    ?? TUnitSettings.Default.Timeouts.ExplicitDefaultTestTimeout;  // correct

Because the no-retry path was inlined as a hot-path optimization (introduced in #5714), it needs to be kept in sync with the logic in ExecuteTestLifecycleAsync. Right now it is not. The effect:

• A user sets TUnitSettings.Timeouts.DefaultTestTimeout = TimeSpan.FromSeconds(30) to catch runaway tests.
• Their tests have no [Retry] attribute (the common case - retryLimit == 0).
• They go through the no-retry fast path, which passes null to TestExecutor, and the configured timeout is silently ignored.
• Only tests that also carry [RetryN] actually honour the configured default.

Fix - apply the same coalesce in the inlined fast path (around line 154):

var testTimeout = test.Context.Metadata.TestDetails.Timeout
    ?? TUnitSettings.Default.Timeouts.ExplicitDefaultTestTimeout;
await _testExecutor.ExecuteAsync(test, _testInitializer, cancellationToken, testTimeout);

This is the only structural issue remaining.

---

RestoreSettings() cannot fully restore the null state

SnapshotSettings() reads through the public getter:

_savedTestTimeout = TUnitSettings.Default.Timeouts.DefaultTestTimeout;
// returns _defaultTestTimeout ?? TimeSpan.FromMinutes(30) — always non-null

RestoreSettings() then writes that value back through the public setter, setting _defaultTestTimeout to TimeSpan.FromMinutes(30) (non-null) rather than restoring it to null. After TUnitSettingsTests runs, TUnitSettings.Default.Timeouts.ExplicitDefaultTestTimeout is permanently TimeSpan.FromMinutes(30) for the rest of the process lifetime.

The test comment acknowledges this and works around it by using new TimeoutSettings() - that is the right move for ExplicitDefaultTestTimeout_Is_Null_When_Unset. But once the bug above (no-retry path) is fixed, every subsequent test without [Timeout] running in the same process after this class will silently see a 30-minute timeout applied via ExplicitDefaultTestTimeout, defeating the optimization.

A clean fix would be an internal reset escape hatch:

// TimeoutSettings.cs
internal void ResetDefaultTestTimeout() => _defaultTestTimeout = null;

Then in SnapshotSettings() capture ExplicitDefaultTestTimeout (nullable) and in RestoreSettings() call ResetDefaultTestTimeout() when the snapshot was null, or assign the saved value when it was not.

---

Changelog note (carried from Round 4, still not addressed)

The silent removal of the implicit 30-minute safety net is worth a release-notes entry. Users who relied on that backstop to surface stuck integration tests will find them spinning indefinitely unless they opt in via DefaultTestTimeout.

---

What's good

• The new tests are well-structured: ExplicitDefaultTestTimeout_Is_Null_When_Unset correctly uses a fresh instance to sidestep the singleton-restore limitation.
• hangCts.Cancel() in the finally block of Configured_Default_Timeout_Fires_On_Hanging_Test prevents a 30-second task leak.
• The [NotInParallel] class attribute prevents concurrent singleton mutation. Correct.
• ExplicitDefaultTestTimeout being internal keeps the nullable state from leaking into the public API.

Summary: One real bug (no-retry fast path skips ExplicitDefaultTestTimeout) and one test-isolation issue (restore cannot recover null state). The no-retry bug should be fixed before merge; the isolation issue becomes important to fix once that bug is corrected.

[thomhurst]

Round 5 review follow-up — 7cc64e7.

Primary finding (inlined no-retry fast path in TestCoordinator) — not reproducible.
Checked TUnit.Engine/Services/TestExecution/TestCoordinator.cs on this branch: there is only one _testExecutor.ExecuteAsync call site, at line 325 inside ExecuteTestLifecycleAsync, and it already does

var testTimeout = test.Context.Metadata.TestDetails.Timeout
    ?? TUnitSettings.Default.Timeouts.ExplicitDefaultTestTimeout;

Both branches of the retryLimit == 0 check (lines 109–120) route through ExecuteTestLifecycleAsync; there is no inlined copy at line ~154. grep -n 'TestDetails\.Timeout\|ExecuteAsync(test, _testInitializer' TUnit.Engine/Services/TestExecution/TestCoordinator.cs confirms the single occurrence. So the observed behaviour (a user-set DefaultTestTimeout only applying to [Retry] tests) can't happen on this branch — the coalesce is on the shared path.

Secondary finding (Snapshot/Restore leaks state) — real, fixed in 7cc64e7.
SnapshotSettings() was reading through the public DefaultTestTimeout getter, which coalesces the null backing field to 30 minutes. RestoreSettings() then wrote that value back through the validating setter, permanently pinning _defaultTestTimeout after the first test ran. Fix:

• TimeoutSettings.SetExplicitDefaultTestTimeout(TimeSpan?) — internal, accepts null, reuses ValidatePositive for non-null values.
• Snapshot now captures the internal ExplicitDefaultTestTimeout (nullable); restore uses the new setter.
• New SnapshotRestore_Preserves_Unset_DefaultTestTimeout test exercises the full round-trip on TUnitSettings.Default to guard the regression.

Verified

• dotnet build TUnit.slnx -c Release — 0 errors.
• TUnit.UnitTests net10.0 — 184/184 passed (including the existing tests from b43f149 and the new round-trip guard).
• TUnit.Core.SourceGenerator.Tests net10.0 — 116/116 passed (1 pre-existing skip).
• TUnit.TestProject BasicTests filter — 3/3 passed.

If the "line ~154 inlined fast path" finding was based on a local branch or a different PR, please point me at the exact commit/file and I'll take another look.

[claude]

Code Review: PR #5728 — perf: skip TimeoutHelper wrap when no explicit [Timeout] is set

Summary

The goal of this PR is sound and the performance motivation is real: eliminating a LinkedCancellationTokenSource + TaskCompletionSource + Task.WhenAny triplet per test for a 30-minute backstop that almost nobody triggers is a worthwhile optimization. The overall shape of the solution — nullable backing field, ExplicitDefaultTestTimeout internal getter, public getter that coalesces to the documented 30-minute default, and SetExplicitDefaultTestTimeout for test-harness restore — is clean and well-reasoned. The snapshot/restore regression fix and the new unit tests add genuine value.

That said, there is one confirmed bug that silently breaks a user-visible contract, and a secondary reporting gap that will cause confusing output in tooling.

---

Bug 1 (Blocker): No-retry fast path ignores DefaultTestTimeout entirely

File: TUnit.Engine/Services/TestExecution/TestCoordinator.cs

The two execution paths in ExecuteTestAsync are not equivalent:

// No-retry fast path (retryLimit == 0) — THE HOT PATH, used by the vast majority of tests
var testTimeout = test.Context.Metadata.TestDetails.Timeout;   // ← only [Timeout] attribute
await _testExecutor.ExecuteAsync(test, _testInitializer, cancellationToken, testTimeout);

// Retry path — dispatches through ExecuteTestLifecycleAsync
var testTimeout = test.Context.Metadata.TestDetails.Timeout
    ?? TUnitSettings.Default.Timeouts.ExplicitDefaultTestTimeout;   // ← correct coalesce
await _testExecutor.ExecuteAsync(test, _testInitializer, cancellationToken, testTimeout);

Impact: A user who writes:

// In a [Before(HookType.TestDiscovery)] hook:
context.Settings.Timeouts.DefaultTestTimeout = TimeSpan.FromMinutes(5);

...gets the timeout only on tests that also carry [Retry]. Plain tests without [Retry] — the overwhelming majority — get no timeout at all, despite the user's explicit opt-in. retryLimit == 0 is literally the "everyone by default" path.

Fix — one-line change to the no-retry path:

var testTimeout = test.Context.Metadata.TestDetails.Timeout
    ?? TUnitSettings.Default.Timeouts.ExplicitDefaultTestTimeout;

Note: The new Configured_Default_Timeout_Fires_On_Hanging_Test test calls TimeoutHelper.ExecuteWithTimeoutAsync directly and validates the ExplicitDefaultTestTimeout property value — it does not route through TestCoordinator and therefore cannot catch this regression. A meaningful integration test would set DefaultTestTimeout, register a hanging test body, and verify it times out via the coordinator's no-retry path.

---

Bug 2 (Minor): Timeout failures via DefaultTestTimeout are misreported as generic errors

File: TUnit.Engine/TUnitMessageBus.cs

if (category == FailureCategory.Timeout
    && testContext.Metadata.TestDetails.Timeout != null       // ← null when timeout came from DefaultTestTimeout
    && duration >= testContext.Metadata.TestDetails.Timeout.Value)
{
    return new TimeoutTestNodeStateProperty(...);
}

When a test times out via DefaultTestTimeout (rather than a [Timeout] attribute), TestDetails.Timeout is null because the timeout value is resolved at TestCoordinator level and never written back to TestDetails. The condition fails and the test is reported as ErrorTestNodeStateProperty rather than TimeoutTestNodeStateProperty. Downstream consumers — JUnit XML writer, GitHub reporter, HTML report — key off TimeoutTestNodeStateProperty for categorisation and will silently misclassify the failure as a generic error.

Recommended fix (Option A — preferred): Write the effective timeout back to TestDetails at the coordinator, right after resolving it:

// After resolving testTimeout in both paths:
test.Context.Metadata.TestDetails.Timeout ??= testTimeout;  // [Timeout] attribute wins; fills the gap for DefaultTestTimeout

This is self-contained — the timeout value is already known at coordination time and belongs on the TestDetails it describes, avoiding a second static-singleton access inside the message bus.

---

Design Note: SetExplicitDefaultTestTimeout naming

File: TUnit.Core/Settings/TimeoutSettings.cs

The method is internal and commented // Test-only seam, which is appropriate. A cleaner name would make the intent unambiguous and avoid the question of whether callers should ever pass a non-null value:

internal void ResetDefaultTestTimeout() => _defaultTestTimeout = null;

Non-blocking — just a readability/discoverability note.

---

What is good about this PR

• The nullable backing field (TimeSpan? _defaultTestTimeout) is the right primitive for modelling "explicitly unset". The previous bool-flag pattern was redundant state.
• ExplicitDefaultTestTimeout cleanly separates the "engine needs to know if it was set" concern from the "user reads the documented default" concern — no magic numbers duplicated.
• The snapshot/restore fix in TUnitSettingsTests is a real bug fix (the public getter was permanently pinning the backing field after the first test ran), and SnapshotRestore_Preserves_Unset_DefaultTestTimeout is a solid regression guard.
• The XML-doc update on DefaultTestTimeout now correctly describes the "no wrap unless explicitly set" semantic.
• Removing the Timeout = Core.Settings.TUnitSettings.Default.Timeouts.DefaultTestTimeout initialisation from all five TestDetails construction sites is the right layering decision — pre-seeding at build time was wrong; the coordinator is the correct place to apply a project-level default.
• No AOT or source-gen concerns: the new accessors are pure data with no reflection.

---

Recommended actions before merge

1. Fix TestCoordinator.cs no-retry path (Bug 1) — one-line change, hard blocker.
2. Fix TUnitMessageBus.cs timeout categorisation (Bug 2) — Option A recommended.
3. Add an integration test that goes through the coordinator's no-retry path with DefaultTestTimeout set, to prevent Bug 1 from regressing silently.

[claude]

Code Review — Round 13

Changes Since Last Approval

The last approval was at the round-12 comment (commit area). One additional commit landed after that:

• ** — chore: trim verbose comments on GlobalContext + TUnitTestFramework**

This is a pure comment-trimming commit — no logic, no types, no control flow changed. The shorter comments in and still convey the critical invariants (AsyncLocal-poisoning hazard, AOT factory requirement, context-before-init ordering). The trimming is appropriate; the previous comments were on the verbose side.

Status of All Previous Concerns

All concerns from rounds 1–12 remain addressed:

• ✅ Nullable backing field () correctly models "unset" vs "explicitly 30 minutes"
• ✅ + internal seam for snapshot/restore
• ✅ Timeout seeding removed from all / initialization sites
• ✅ write-back correctly classifies -triggered timeouts as timeout failures
• ✅ is process-wide static (not ) with correct + visibility semantics
• ✅ Contexts installed before so hooks observe the real
• ✅ handles null filter; uses it instead of
• ✅ Integration test + 4 unit tests directly cover the two bugs fixed

Verdict

No new issues introduced by the comment-trimming commit. The PR remains ready to merge.

[claude]

Code Review — Round 13

Changes Since Last Approval

The last approval was at the round-12 comment. One additional commit landed after that:

• 28b247d — chore: trim verbose comments on GlobalContext + TUnitTestFramework

This is a pure comment-trimming commit — no logic, no types, no control flow changed. The shorter comments in GlobalContext.cs and TUnitTestFramework.cs still convey the critical invariants (AsyncLocal-poisoning hazard, AOT factory requirement, context-before-init ordering). The trimming is appropriate; the previous comments were on the verbose side.

Status of All Previous Concerns

All concerns from rounds 1–12 remain addressed:

• Nullable backing field (TimeSpan? _defaultTestTimeout) correctly models "unset" vs "explicitly 30 minutes"
• ExplicitDefaultTestTimeout + SetExplicitDefaultTestTimeout internal seam for snapshot/restore
• Timeout seeding removed from all TestBuilder / TestBuilderPipeline initialization sites
• TestCoordinator write-back correctly classifies DefaultTestTimeout-triggered timeouts as timeout failures
• GlobalContext.Current is process-wide static (not AsyncLocal) with correct Volatile.Write + LazyInitializer visibility semantics
• Contexts installed before Initializer.Initialize() so hooks observe the real GlobalContext.Current
• FilterParser.StringifyFilter handles null filter; TUnitServiceProvider uses it instead of Filter?.ToString()
• Integration test + 4 unit tests directly cover the two bugs fixed

Verdict

No new issues introduced by the comment-trimming commit. The PR remains ready to merge.

[claude]

Code Review: perf: skip TimeoutHelper wrap when no explicit [Timeout] is set (#5711)

Overall Assessment: The goal — eliminating the unconditional CancellationTokenSource + TaskCompletionSource + Task.WhenAny overhead per test — is sound and the performance win is real. The design is mostly correct but there is one genuine correctness bug (the no-retry fast path silently ignores ExplicitDefaultTestTimeout), a subtle thread-safety concern in the GlobalContext change, and a handful of design and documentation issues worth addressing.

---

Bug: No-retry fast path does not apply ExplicitDefaultTestTimeout

The DefaultTestTimeout propagation is only implemented in ExecuteTestLifecycleAsync (the retry branch). The inlined no-retry fast path in ExecuteTestAsync still reads test.Context.Metadata.TestDetails.Timeout raw and passes it directly to _testExecutor.ExecuteAsync:

// TestCoordinator.cs — no-retry fast path (retryLimit == 0)
var testTimeout = test.Context.Metadata.TestDetails.Timeout;   // ← null when no [Timeout]
await _testExecutor.ExecuteAsync(test, _testInitializer, cancellationToken, testTimeout);

The retry path correctly does:

// ExecuteTestLifecycleAsync (retry branch)
var testTimeout = test.Context.Metadata.TestDetails.Timeout
    ?? TUnitSettings.Default.Timeouts.ExplicitDefaultTestTimeout;

The no-retry path is the hot path — it applies to all tests with RetryLimit == 0, which is the default for every test that does not have a [Retry] attribute. This means: if a user sets context.Settings.Timeouts.DefaultTestTimeout, their timeout silently has no effect on any test that does not also use [Retry]. Only retried tests honour the programmatic default.

The fix is to mirror the coalescing logic in the fast path as well, or extract it to a shared helper so both paths stay in sync. The Timeout write-back (TestDetails.Timeout = testTimeout) for correct TimeoutTestNodeStateProperty classification also needs to be mirrored.

---

Thread-safety concern in GlobalContext.Current

The PR changes GlobalContext from AsyncLocal<T> to a plain static field protected by LazyInitializer.EnsureInitialized (getter) and Volatile.Write (setter):

private static GlobalContext? _current;
get => LazyInitializer.EnsureInitialized(ref _current, static () => new GlobalContext())!;
internal set => Volatile.Write(ref _current, value);

LazyInitializer.EnsureInitialized(ref T, Func<T>) is documented as a "best effort" initialiser — under contention it may invoke the factory more than once and discard the extra instances. The asymmetry between LazyInitializer (compare-exchange-based) on the getter and Volatile.Write (plain store) on the setter creates a potential race: if early startup code triggers the lazy-create, it stores an empty GlobalContext via CompareExchange. If the framework init then calls Volatile.Write with the real populated GlobalContext, it races with any concurrent lazy reads.

More importantly, switching from AsyncLocal to a static field changes the isolation model. AsyncLocal kept the context per execution-context flow; the static field is process-global. This is fine today (TUnit runs one session per process), but it changes the contract for any future multi-session scenario.

The comment justifying the switch ("a lazy-creating AsyncLocal getter poisons the first reading branch") is right about the original ??= bug, but the correct fix would have been to remove the lazy-create from the AsyncLocal getter (null is a valid initial state when the framework sets it before test code runs — which the ordering fix in this same PR ensures). Consider reverting to AsyncLocal without the lazy init, or at minimum using Interlocked.CompareExchange in the setter to be consistent with the getter's semantics.

---

Design: ExplicitDefaultTestTimeout leaks internal state as a public API surface

TimeoutSettings.ExplicitDefaultTestTimeout is internal, but together with the public DefaultTestTimeout { get; set; } it creates an asymmetric API where the getter returns a value (30 minutes) that does not drive test execution unless it was explicitly set. Having separate public and internal setters with different capabilities (internal void SetExplicitDefaultTestTimeout(TimeSpan? value)) is a code smell.

A cleaner design: expose a single nullable TimeSpan? DefaultTestTimeout publicly. Document the 30-minute recommended value separately. This eliminates the internal seam, makes the "unset" vs "set" distinction explicit in the type system, and removes the need for the test-only SetExplicitDefaultTestTimeout shim entirely.

---

Docs not updated

docs/docs/reference/programmatic-configuration.md still says:

DefaultTestTimeout | TimeSpan | 30 minutes | Maximum duration for a single test before it is cancelled.

This is now inaccurate — tests without [Timeout] run without any timeout unless DefaultTestTimeout is explicitly set. The 30-minute value in the public getter is a display-only default, not an enforcement value. Users relying on the documented 30-minute safety net will be surprised.

The docs should clarify:

• Default is now "none" (no timeout wrapper) unless the property is explicitly assigned.
• The 30-minute value shown by the getter is illustrative, not enforced.

---

GlobalContext.SetAsyncLocalContext semantics changed silently

GlobalContext.SetAsyncLocalContext still calls Current = this, which with the new design sets the global singleton rather than propagating context into the current execution flow. Previously (with AsyncLocal) this call was meaningful for restoring context in forked tasks. With the static-field design it becomes either a no-op (the singleton is already set) or, worse, a confusing re-assignment. It deserves a comment or should be made a no-op with an explanation.

---

FilterParser null-safety fix

The ITestExecutionFilter? null-safety fix (null => null arm added, parameter made nullable) is correct and has no concerns. Good catch.

---

Context initialisation ordering fix

Moving GlobalContext.Current = ... before serviceProvider.Initializer.Initialize(context) is the correct fix for the ordering bug. The comment explains the motivation clearly. No concerns.

---

Test coverage gap

There is no integration-level test verifying ExplicitDefaultTestTimeout fires on the no-retry fast path (the bug identified above). Given the no-retry path is the default for all non-[Retry] tests, an integration test covering: no [Retry], DefaultTestTimeout set, hanging test body → test is cancelled after the configured timeout, would prevent a regression.

---

Summary of issues by severity

#	Severity	Issue
1	Bug	ExplicitDefaultTestTimeout is never applied on the no-retry fast path (retryLimit == 0) — the predominant path for all tests without [Retry].
2	Medium	GlobalContext switch from AsyncLocal to static field changes isolation semantics; Volatile.Write/LazyInitializer asymmetry is fragile under concurrency.
3	Medium	Docs still document a 30-minute implicit default that no longer applies.
4	Low	ExplicitDefaultTestTimeout + SetExplicitDefaultTestTimeout internal seam is a leaky abstraction — a nullable public property would be cleaner.
5	Low	GlobalContext.SetAsyncLocalContext semantics changed silently with the static-field switch.
6	Low	Missing integration test for no-retry + DefaultTestTimeout combination.

[claude]

Code Review — Round 14 (Current State)

Changes Since Last Review

Three commits landed since the last round-13 review:

• f53549c — Reverts the "drop flaky" commit, restoring the engine test and TestProject fixture
• 9a0aeb4 — Applies DefaultTestTimeout on the no-retry fast path (this was the actual blocker)
• 75c88a9 — Centralises the [Timeout] ?? DefaultTestTimeout coalesce into GetEffectiveTestTimeout on TimeoutSettings

---

✅ All Previous Concerns Addressed

No-retry fast path (the only genuine blocker): Commit 9a0aeb44 confirms the reviewer was right — the no-retry path in ExecuteTestAsync was not applying DefaultTestTimeout. The ResolveAndPropagateTestTimeout extraction fixes both call sites uniformly, and the integration test (DefaultTimeoutClassificationTests) now exercises the corrected fast path. This was the right fix.

Refactored coalesce (75c88a9d): Promoting attributeTimeout ?? _defaultTestTimeout into TimeoutSettings.GetEffectiveTestTimeout is a clean improvement — TestCoordinator no longer reaches through the ExplicitDefaultTestTimeout internal seam, and the hook-side resolution now has a parallel structure.

AOT factory (round-10/11 concern): static () => new GlobalContext() explicit factory is present.

Nullable backing field + snapshot/restore: ExplicitDefaultTestTimeout + SetExplicitDefaultTestTimeout seam is correct. The unit tests prove the round-trip preserves null.

---

Remaining Observation (non-blocking)

GlobalContext — LazyInitializer.EnsureInitialized + Volatile.Write asymmetry

The combination is technically correct: EnsureInitialized uses Interlocked.CompareExchange (CAS), so a setter write always wins — CAS will see the non-null setter value and return it rather than overwriting it with a lazy-created empty instance. The context hoisting before Initializer.Initialize (in TUnitTestFramework.ExecuteRequestAsync) eliminates the only practical race window.

That said, the getter's CAS semantics and the setter's plain volatile write are slightly asymmetric idioms. An alternative that's more self-explanatory is a simple Volatile.Read/Volatile.Write pair with an explicit null-init check:

This makes the race-resolution contract explicit in both getter and setter. Worth considering if LazyInitializer ever needs to be dropped for a reason (e.g. trimming), but not a blocker — the current form is correct.

---

Summary

The PR is in good shape. The no-retry fast path was a real bug and is now fixed. The coalesce refactor is a genuine improvement. Tests cover both the classification fix and the snapshot/restore regression. All previous concerns are resolved.

✅ Approved.

[claude]

Round 14: All previous concerns addressed. No-retry fast path bug confirmed fixed in 9a0aeb4, coalesce refactored cleanly in 75c88a9. One non-blocking observation on GlobalContext LazyInitializer + Volatile.Write asymmetry noted in the companion comment — the current form is correct. Approved.