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.