From ec7a7df5a7b6887c182e15508eb2b1918eb1e2eb Mon Sep 17 00:00:00 2001 From: Arthur van de Vondervoort Date: Mon, 28 Sep 2026 13:35:51 +0200 Subject: [PATCH 1/2] test(CM0001): observe a cancelled request's disconnect off the thread pool The cancellation test raced the fake server's ReadAsync-based disconnect observation against a two-second Task.Delay. The client closes its socket synchronously inside Cancel(), but observing that through task continuations needs more thread-pool hops than the timer, so a saturated CI runner let the timer win and reported a false "not prompt" while the later assertions still proved cancellation had propagated. Observe the disconnect with a blocking read on a dedicated thread that sets a ManualResetEventSlim, and wait on that signal. The window and every assertion are unchanged; the blocking read is bounded by the server's shutdown budget because the shutdown token cannot interrupt it. Co-Authored-By: Claude Fable 5.1 --- ...m0001-configuration-could-not-be-loaded.md | 1 + .claude/rules/testing.md | 2 ++ .../ALCopsSettingsRemoteRecoveryTests.cs | 34 ++++++++++++++++--- 3 files changed, 32 insertions(+), 5 deletions(-) diff --git a/.claude/rules/diagnostics/cm0001-configuration-could-not-be-loaded.md b/.claude/rules/diagnostics/cm0001-configuration-could-not-be-loaded.md index 371379d0..ff775c9c 100644 --- a/.claude/rules/diagnostics/cm0001-configuration-could-not-be-loaded.md +++ b/.claude/rules/diagnostics/cm0001-configuration-could-not-be-loaded.md @@ -58,3 +58,4 @@ Registers `RegisterCompilationAction` (no node or symbol kinds; reports at `Loca - Tests use a manual `Compilation.Create` + `CompilationWithAnalyzers` harness in `ALCops.Common.Test/Analyzers/` because RoslynTestKit's marker-based assertions cannot match `Location.None`. - `ThrowingFileSystem` (Helpers) simulates exists-but-unreadable deterministically; real file locks are advisory-only on Linux. - `{1}` may contain OS-localized exception text, so tests assert only substrings the code controls. +- The cancellation test measures the disconnect of the stalled HTTP request off the thread pool (dedicated blocking read plus a synchronous signal); `testing.md` (Concurrent synchronous HTTP tests) explains why a task-based race against `Task.Delay` reports a false "not prompt" on a saturated runner. diff --git a/.claude/rules/testing.md b/.claude/rules/testing.md index a46333d8..6055acb1 100644 --- a/.claude/rules/testing.md +++ b/.claude/rules/testing.md @@ -363,6 +363,8 @@ Tests run in parallel across assemblies (`[assembly: Parallelizable(ParallelScop When a test deliberately starts many synchronous settings lookups, use dedicated callers (`TaskCreationOptions.LongRunning` with `TaskScheduler.Default`) and a bounded start barrier. Scheduling blocking callers with `Task.Run` can occupy the same worker pool needed by the loopback server and HTTP continuations, producing artificial five-second timeouts on small CI runners. `[NonParallelizable]` only controls NUnit scheduling; it does not isolate those workers. Keep server continuations queued in the regression fixture so inline loopback I/O cannot hide this dependency. Preserve the real timeout and assertions; do not mask starvation with retries, higher thread-pool minimums or longer production timeouts. A looping fake server must stop its `TcpListener` only after the serve loop has exited (cancel the token, await the loop, then `Stop()`): on a stopped listener `AcceptTcpClientAsync` throws `InvalidOperationException` before it checks the cancellation token, which surfaces as a teardown failure in the last test that used the server. +A test that asserts how promptly a cancelled request closes must observe the disconnect on a dedicated thread with a synchronous signal (a blocking `NetworkStream.Read` on a `LongRunning` task that sets a `ManualResetEventSlim`) and wait on that signal, not on a task. The client side of a cancellation is synchronous: `Cancel()` runs the linked-token registrations inline down to `HttpConnection.Dispose`, so the socket is closed before `Cancel()` returns. A task-based observation (`ReadAsync` continuation, then a `RunContinuationsAsynchronously` completion) needs more thread-pool hops than the `Task.Delay` it races, so on a saturated runner the timer wins even though the close happened immediately. Give the blocking read a `ReadTimeout` equal to the server's shutdown budget, because the shutdown token cannot interrupt it. + ## Common Mistakes to Avoid - **Forgetting markers in NoDiagnostic files.** Both HasDiagnostic and NoDiagnostic .al files need `[|...|]` markers. The difference is whether a diagnostic is expected at those locations. diff --git a/src/ALCops.Common.Test/Settings/ALCopsSettingsRemoteRecoveryTests.cs b/src/ALCops.Common.Test/Settings/ALCopsSettingsRemoteRecoveryTests.cs index 09e0b676..ffdc69f1 100644 --- a/src/ALCops.Common.Test/Settings/ALCopsSettingsRemoteRecoveryTests.cs +++ b/src/ALCops.Common.Test/Settings/ALCopsSettingsRemoteRecoveryTests.cs @@ -91,8 +91,7 @@ public async Task CancelledCompilation_ClosesHttpRequestPromptly_AndNextCompilat await server.FirstRequest.WaitAsync(TimeSpan.FromSeconds(10)).ConfigureAwait(false); cancellation.Cancel(); - Task completed = await Task.WhenAny(server.FirstDisconnected, Task.Delay(TimeSpan.FromSeconds(2))).ConfigureAwait(false); - bool closedPromptly = completed == server.FirstDisconnected; + bool closedPromptly = server.WaitForFirstDisconnect(TimeSpan.FromSeconds(2)); try { await analysis.ConfigureAwait(false); } catch (OperationCanceledException) { } await server.FirstDisconnected.WaitAsync(TimeSpan.FromSeconds(10)).ConfigureAwait(false); @@ -231,10 +230,12 @@ private RelativeFileSystem CreateFileSystem(string source, bool invalidLocalValu private sealed class SettingsServer : IAsyncDisposable { + private static readonly TimeSpan ShutdownBudget = TimeSpan.FromSeconds(20); private readonly TcpListener _listener = new(IPAddress.Loopback, 0); - private readonly CancellationTokenSource _shutdown = new(TimeSpan.FromSeconds(20)); + private readonly CancellationTokenSource _shutdown = new(ShutdownBudget); private readonly TaskCompletionSource _firstRequest = new(TaskCreationOptions.RunContinuationsAsynchronously); private readonly TaskCompletionSource _firstDisconnected = new(TaskCreationOptions.RunContinuationsAsynchronously); + private readonly ManualResetEventSlim _firstDisconnectedSignal = new(); private readonly Task _serve; private int _requestCount; @@ -250,6 +251,9 @@ public SettingsServer(bool failFirst = false, bool stallFirst = false) public Task FirstRequest => _firstRequest.Task; public Task FirstDisconnected => _firstDisconnected.Task; + /// Blocks the caller; the signal is set on the observing thread without a thread-pool hop. + public bool WaitForFirstDisconnect(TimeSpan timeout) => _firstDisconnectedSignal.Wait(timeout); + private async Task ServeAsync(bool failFirst, bool stallFirst) { try @@ -271,8 +275,27 @@ private async Task ServeAsync(bool failFirst, bool stallFirst) _firstRequest.TrySetResult(); if (stallFirst && request == 1) { - await stream.ReadAsync(new byte[1], _shutdown.Token).ConfigureAwait(false); - _firstDisconnected.TrySetResult(); + // The client closes its socket synchronously inside Cancel(), but observing + // that with ReadAsync needs more thread-pool hops than the timer the test + // races it against. A blocking read on a dedicated thread and a synchronous + // signal keep the promptness measurement independent of pool saturation. + // The shutdown token cannot interrupt a blocking read, so bound it by time. + await Task.Factory.StartNew(() => + { + stream.ReadTimeout = (int)ShutdownBudget.TotalMilliseconds; + try + { + if (stream.Read(new byte[1], 0, 1) == 0) + _firstDisconnectedSignal.Set(); + } + catch (IOException ex) when (ex.InnerException is not SocketException { SocketErrorCode: SocketError.TimedOut }) + { + // A reset from the closing peer is a disconnect too. + _firstDisconnectedSignal.Set(); + } + }, CancellationToken.None, TaskCreationOptions.LongRunning, TaskScheduler.Default).ConfigureAwait(false); + if (_firstDisconnectedSignal.IsSet) + _firstDisconnected.TrySetResult(); } else { @@ -291,6 +314,7 @@ public async ValueTask DisposeAsync() await _serve.ConfigureAwait(false); _listener.Stop(); _shutdown.Dispose(); + _firstDisconnectedSignal.Dispose(); } } } From efdcead3db3294c45824930d8f1e2243624ee724 Mon Sep 17 00:00:00 2001 From: Arthur van de Vondervoort Date: Mon, 28 Sep 2026 13:35:51 +0200 Subject: [PATCH 2/2] chore(ci): overwrite test-result artifacts on re-run Artifacts are stored per run attempt. Re-running a single test leg left two test-results- artifacts and two unified test-results artifacts, so the merge job picked up the stale attempt-1 trx and the Test Report check listed every trx twice with the already-fixed failure still red. Set overwrite: true on both uploads so a re-run replaces the same-named artifact and the report reflects the latest attempt. Co-Authored-By: Claude Fable 5.1 --- .claude/rules/release-strategy.md | 2 +- .github/workflows/build-test.yml | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/.claude/rules/release-strategy.md b/.claude/rules/release-strategy.md index 90446765..824878dd 100644 --- a/.claude/rules/release-strategy.md +++ b/.claude/rules/release-strategy.md @@ -123,7 +123,7 @@ Beta tags are created inside the workflow using `GITHUB_TOKEN`, which doesn't tr ## Test report gate -Every release channel is gated on the unified dorny test report. `build-test.yml` merges the per-AL-version `.trx` artifacts into one `test-results` artifact; for push/scheduled runs (`publish-report: true`) a `report` job publishes the "Test results" check inline, and because the release job `needs` the workflow call, a failed test on any AL version blocks alpha, beta, and stable alike — before anything is published. Individual `dotnet test` steps stay `continue-on-error` so all cops × all AL versions always run and report together; the dorny check is the failure signal, not the test job. Pull requests get the same check via the `workflow_run` Test Report workflow (`test-report.yml`, 'Pull Request' only) — the fork-safe path, since fork PR tokens lack `checks: write`. That workflow uses dorny's `artifact:` input instead of checking out the PR head (`actions/checkout` refuses fork-PR SHAs in a `workflow_run` context). +Every release channel is gated on the unified dorny test report. `build-test.yml` merges the per-AL-version `.trx` artifacts into one `test-results` artifact; for push/scheduled runs (`publish-report: true`) a `report` job publishes the "Test results" check inline, and because the release job `needs` the workflow call, a failed test on any AL version blocks alpha, beta, and stable alike — before anything is published. Individual `dotnet test` steps stay `continue-on-error` so all cops × all AL versions always run and report together; the dorny check is the failure signal, not the test job. Pull requests get the same check via the `workflow_run` Test Report workflow (`test-report.yml`, 'Pull Request' only) — the fork-safe path, since fork PR tokens lack `checks: write`. That workflow uses dorny's `artifact:` input instead of checking out the PR head (`actions/checkout` refuses fork-PR SHAs in a `workflow_run` context). Artifacts are stored per run attempt, so re-running one test leg would otherwise leave two `test-results-` artifacts and two `test-results` artifacts: the merge job would pick up the stale attempt-1 result and dorny would list every trx twice. Both uploads therefore set `overwrite: true`, which deletes the same-named artifact before uploading. ## What external contributors should expect diff --git a/.github/workflows/build-test.yml b/.github/workflows/build-test.yml index a543c74b..e90d241b 100644 --- a/.github/workflows/build-test.yml +++ b/.github/workflows/build-test.yml @@ -437,6 +437,9 @@ jobs: uses: actions/upload-artifact@v7 with: name: test-results-${{ matrix.version }} + # Artifacts are per run attempt. Replacing the same-named artifact on a re-run keeps + # the merge job from picking up the stale attempt-1 result for this version. + overwrite: true path: | **/TestResults_${{ matrix.version }}.trx @@ -461,6 +464,9 @@ jobs: uses: actions/upload-artifact@v7 with: name: test-results + # The Test Report workflow downloads every artifact with this name; without + # overwrite a re-run would leave two and report every trx twice. + overwrite: true path: TestResults/**/*.trx if-no-files-found: ignore