Separate QUIC idle-timeout test setup from inactivity

[rzikm]

IdleTimeout_ThrowsQuicException can expire its one-second connection idle timeout before the initial stream exchange completes, failing during setup rather than reaching its intended assertions. Controlled three-second managed pauses reproduce this premature failure. This change separates successful setup from deliberate inactivity; it does not establish where the delay occurred in the natural CI failures.

Approach

Enable server-only keep-alive during setup, then disable it through the existing MsQuic API table after establishing a pending server read and inbound accept. Keep the original one-second idle setting and all three exact ConnectionIdle assertions. Require the setup byte to be received and bound the initial stream operations with cancellation.

The test-only helper protects the native handle with DangerousAddRef/DangerousRelease, preserves the exact private _handle field for NativeAOT, and checks native status plus settings readback. Pending operation faults are observed after resource disposal if setup fails. No product API, exception mapping, shared connection helper, global setting or test skip changes.

Validation

• Windows x64 CoreCLR, Schannel, MsQuic 2.5.10: 100 ordinary gated iterations and 12 iterations with three-second pauses across four setup phases passed. All three ungated delayed controls reproduced premature idle failure.
• Omitting disable kept operations pending beyond five seconds; disabling keep-alive then allowed all idle assertions to pass. Across 113 gated completions, time from disable/readback to completed assertions was 963-1107 ms. These are managed observation times, not exact native timer measurements.
• Native setter failure, cancellation cleanup and closed-handle controls passed. The closed-handle control disposes the stream first because a live stream retains the native connection handle.
• Final complete QUIC innerloop plus outerloop suite: 478 passed, 0 failed, 1 existing platform skip. Final focused independent-process repetitions: 20/20 passed.
• Final Windows x64 NativeAOT executable: 1/1 passed with each of OpenSSL and Schannel. Native code generation succeeded, but the standard test wrapper failed to resolve the bare executable name (exit 9009); these passes used the generated executable by explicit path. No new broad rooting or warning suppression was added.

Review considerations

This hardens the test against managed setup pauses while native workers remain responsive. The natural CI cause remains unproven, and the gate does not protect against native-worker starvation or sustained packet loss. Setup now uses keep-alive, while the intentional idle phase retains the original timeout semantics.

Private-field reflection and mutable native settings add maintenance cost. Linux, Windows x86 and the minimum supported MsQuic 2.2.2 have not been tested; Windows OpenSSL coverage is not Linux coverage. No native leak audit was performed.

Note

This PR description and implementation were generated with GitHub Copilot.

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

Tagging subscribers to this area: @karelz, @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

[Copilot]

🔵 Needs a closer look

A moderate unresolved cleanup issue remains, and the native-settings test changes warrant human review.

Pull request overview

Hardens the QUIC idle-timeout test by separating connection setup from intentional inactivity.

Changes:

• Adds a test-only MsQuic settings helper with handle-lifetime protection.
• Uses keep-alive during setup, then disables it before idle-timeout assertions.
• Adds cancellation-bounded setup operations.

File summaries

File	Summary	Review findings
src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestCollection.cs	Adds native settings manipulation and handle protection.	None noted.
src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs	Separates setup from idle-timeout validation.	Moderate (1 vote): Pending operation faults may become unobserved during cleanup. Nit (1 vote): Validate the pre-update keep-alive setting.

Review details

Suppressed comments (2)

src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs:1408

• These checks only observe a fault if it has already completed. On a setup or assertion failure, disposing the connections completes the pending accept/read sources, but their async continuations can run after this finally; a later QuicException can therefore become unobserved (the collection tracks unobserved task exceptions). Attach an exception-observing continuation or await both tasks during cleanup while preserving the original failure.

[!NOTE]
This review comment was generated by GitHub Copilot.

                if (readTask?.IsFaulted == true)
                {
                    _ = readTask.Exception;
                }

src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs:1395

• This only verifies the value after the helper disables keep-alive. If the server configuration stops applying the 100 ms keep-alive, the setter/readback still produces 0 and the test can pass without protecting setup from the one-second timeout, leaving the original pause sensitivity undetected. Read and validate the pre-update setting (or expose that precondition from the helper) before asserting the disabled value.

[!NOTE]
This review comment was generated by GitHub Copilot.

                    Microsoft.Quic.QUIC_SETTINGS settings = QuicTestCollection.DisableConnectionKeepAlive(serverConnection);
                    Assert.Equal(0u, settings.KeepAliveIntervalMs);
                    Assert.Equal(1000ul, settings.IdleTimeoutMs);

• Files reviewed: 2/2 changed files
• Comments generated: 0
• Review effort level: Lite

[rzikm]

Pending operation faults may become unobserved during cleanup.

Addressed in 323e6ac: cleanup now awaits both retained tasks with SuppressThrowing, so delayed faults are observed without replacing the original setup/assertion failure. A temporary control delayed both real operation completions and injected a setup failure; the old cleanup failed the drain assertion, while the updated cleanup passed and preserved the original failure.

Validate the pre-update keep-alive setting.

The helper now checks that native keep-alive is nonzero before disabling it, reusing the checked settings reader for both reads. A temporary zero-keep-alive control failed this precondition as expected. All diagnostic edits were removed before the commit.

The final full QUIC suite passed 478 tests with one existing platform skip and no failures. The final focused CoreCLR test and the rebuilt Windows x64 NativeAOT executable passed; NativeAOT was executed directly with both OpenSSL and Schannel. The previously documented natural-CI and cross-platform coverage limitations remain.

Note

This response and the changes were generated with GitHub Copilot.

[Copilot]

🔵 Needs a closer look

Unresolved concerns remain around task observation, deterministic setup-delay coverage, and cross-platform/version validation or gating.

Review details

Suppressed comments (3)

src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs:1398

• If the WaitAsync timeout fires here, it detaches from the task returned by AssertThrowsQuicExceptionAsync, and the write operation is created inside that lambda with no retained task. The outer finally only observes readTask and acceptTask, so a delayed write fault (or a later assertion fault in the wrapper) can become unobserved and pollute the shared QUIC test run. Retain the assertion task and observe it with SuppressThrowing in a cleanup path, as is done for the other pending operations.

                    await AssertThrowsQuicExceptionAsync(QuicError.ConnectionIdle, async () => await serverStream.WriteAsync(new byte[10])).WaitAsync(TimeSpan.FromSeconds(10));

src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs:1368

• The regression this setting is intended to prevent is a setup pause longer than the one-second idle timeout, but this test never pauses setup before disabling keep-alive. As written, a future regression that stops keep-alive from protecting setup can still pass whenever scheduling is normal; add a deterministic pause longer than the idle timeout (or a separate gated regression case) before disabling it so the protection is actually exercised.

                    serverOptions.KeepAliveInterval = TimeSpan.FromMilliseconds(100);

src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestCollection.cs:148

• This new helper calls the raw QUIC_PARAM_CONN_SETTINGS entry point with the generated native QUIC_SETTINGS layout, so its behavior depends on MsQuic version, OS/backend, and architecture. The validation described for this change covers Windows x64 only, while this functional-test class runs on supported Unix/OpenSSL configurations and MsQuicApi accepts the minimum supported 2.2.2 version. Please validate this path on at least Linux/OpenSSL and the minimum supported MsQuic, or add an explicit capability/version gate, before relying on it across the shared suite.

            int status = apiTable->SetParam(nativeHandle, QUIC_PARAM_CONN_SETTINGS, (uint)sizeof(QUIC_SETTINGS), &settings);

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite

[rzikm]

Retain the assertion task and observe it with SuppressThrowing in a cleanup path.

Addressed in 0139a9c. The test retains the current assertion task for all three sequential assertions and drains it after disposal, including a write/assertion fault arriving after WaitAsync times out. A temporary delayed-write control failed with the prior cleanup and passed with the fix while preserving the original timeout.

Add a deterministic pause longer than the idle timeout.

The test now includes a three-second pre-open setup pause within its existing cancellation budget. With keep-alive disabled, this reproduces ConnectionIdle at the initial stream open; with keep-alive enabled, the setup exchange and all three idle assertions pass. This deliberately adds three seconds to this outerloop test.

Validate Linux/OpenSSL and minimum supported MsQuic, or add a capability/version gate.

Linux and minimum-version execution remain deferred coverage, not claimed successes. I checked the v2.2.2 source: connection settings updates, cancellation of the running keep-alive timer, and settings-size handling support this path already. That is source evidence, not execution validation. There is no demonstrated unsupported version or platform on which to base a new gate, so I have not disabled existing coverage. The local Linux environment lacks this checkout's compatible Linux runtime/baseline; provisioning and running that additional configuration is deferred.

The final clean full QUIC suite passed 478 tests with one existing platform skip and no failures. Focused CoreCLR and rebuilt Windows x64 NativeAOT tests passed, with NativeAOT executed directly against both OpenSSL and Schannel. All temporary failure controls were removed. The natural CI delay location remains unproven.

Note

This response and the changes were generated with GitHub Copilot.

[Copilot]

🔵 Needs a closer look

The direct MsQuic ABI path needs supported native-combination coverage or a narrowly justified capability/version guard.

Review details

Suppressed comments (1)

src/libraries/System.Net.Quic/tests/FunctionalTests/QuicTestCollection.cs:149

• This adds an unconditional direct QUIC_PARAM_CONN_SETTINGS/QUIC_SETTINGS ABI path to a test project targeting Windows, Linux, and macOS, but the stated validation covers only Windows x64 and not the minimum supported MsQuic version. A platform- or version-specific status/layout mismatch will fail this test before it reaches the idle-timeout assertions. Please add coverage for the supported native combinations or guard the test behind a narrowly justified capability/version check.

            int status = apiTable->SetParam(nativeHandle, QUIC_PARAM_CONN_SETTINGS, (uint)sizeof(QUIC_SETTINGS), &settings);
            Assert.False(StatusFailed(status), $"Disabling connection keep-alive failed: 0x{status:X8}");

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite

[rzikm]

/backport to release/11.0

[github-actions]

Started backporting to release/11.0 (link to workflow run)