Make Redis dispose-vs-connect guard reach the seam deterministically (#1332)

[Chris0Jeky]

Summary

RedisCacheServiceTests.Dispose_IsNotSerialized_BehindAnInFlightConnect flaked under full-API-suite load on Windows with Expected connectEntered.Wait(TimeSpan.FromSeconds(5)) to be True ... but found False. The connecting actor was launched with Task.Run(...), which queues a thread-pool work item. Under a saturated pool the injection throttle delayed running that item, so the worker never reached OnBeforeConnect before the wait window elapsed — a scheduling artifact, not a real regression.

This is a test-only change. No production RedisCacheService behavior is touched (the production file has an empty diff).

Mechanism (deterministic, not a bigger timeout)

Replaced Task.Run(...) with a dedicated, named background Thread (redis-connector, IsBackground = true) that is created and scheduled by the OS immediately, independent of thread-pool saturation. Coordination is unchanged and explicit:

• GetConnection invokes OnBeforeConnect synchronously on the connecting thread before any await, so the existing connectEntered / releaseConnect handshake is a real rendezvous.
• The connecting thread is now Joined (bounded, 5s) instead of awaited, and its result/exception are captured on fields so the post-dispose degrade assertions still hold.

The 5s wait was not increased; determinism comes from the dedicated OS thread, not from a wider margin. The disposeElapsed < 500ms concurrency assertion is preserved verbatim.

Negative proof (still fails against pre-#1189 semantics)

Temporarily reverted the production GetConnection to hold _connectionLock across the blocking connect (the pre-#1189 lock-around-connect bug), rebuilt, and ran the test:

Expected disposeElapsed to be less than 500ms because Dispose must not be serialized
behind another thread's in-flight connect (#1189), but found 7s, 75ms and 86.9µs.
Failed!  - Failed: 1, Passed: 0

With the fixed production code the dedicated connecting thread releases _connectionLock before the connect, so Dispose() acquires it in microseconds. The revert was restored before committing (production diff is empty); the new mechanism did not weaken the guard.

Verification

• dotnet build backend/Taskdeck.sln -c Release — clean (0 errors).
• Targeted repeat: 20/20 runs green of --filter "FullyQualifiedName~RedisCacheServiceTests" (8/8 tests each run).
• Negative proof: pre-Thread.Sleep inside lock in RedisCacheService.GetConnection starves thread pool #1189 revert → Dispose test fails (disposeElapsed ~7s), as required.
• Full project run: Taskdeck.Api.Tests — 2060 passed, 0 failed, 0 skipped.

Closes #1332

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Chris0Jeky]

Adversarial Code Review

Scope: single changed file backend/tests/Taskdeck.Api.Tests/RedisCacheServiceTests.cs (test-only). Production RedisCacheService diff is empty.

CRITICAL

• None

HIGH

• None

MEDIUM

• None

LOW

• None

Analysis notes (verified correct, no action)

• Memory visibility: connectorResult / connectorError are written on the redis-connector thread and read on the test thread. Thread.Join establishes a happens-before edge, so the post-join reads are safe. On a Join timeout the .Should().BeTrue() assertion throws before any read, so stale values are never observed.
• Determinism claim holds: a dedicated Thread is OS-scheduled and bypasses the thread-pool injection throttle that was the actual root cause of the flake. GetConnection invokes OnBeforeConnect synchronously on that thread before any await, so the connectEntered/releaseConnect handshake is a genuine rendezvous — not a probabilistic sleep or a widened timeout (the 5s wait is unchanged).
• No hang risk: background thread + bounded Join(5s) + bounded releaseConnect.Wait(5s).
• Guard not weakened: disposeElapsed < 500ms assertion preserved verbatim. Negative proof: reverting production to pre-Thread.Sleep inside lock in RedisCacheService.GetConnection starves thread pool #1189 lock-around-connect makes the test fail (disposeElapsed ~7s).

Bot Comments Addressed

• None present at review time.

Summary

0 CRITICAL / 0 HIGH / 0 MEDIUM / 0 LOW. Not merge-blocking. Test-only change; 20/20 targeted runs green, full Taskdeck.Api.Tests 2060/2060 pass, negative proof confirmed against pre-#1189 semantics.

[gemini-code-assist]

Code Review

This pull request refactors the Dispose_IsNotSerialized_BehindAnInFlightConnect test in RedisCacheServiceTests.cs by replacing Task.Run with a dedicated background thread. This ensures the connecting worker reaches the connection seam deterministically, resolving test flakiness caused by thread pool saturation under heavy load. There are no review comments, so I have no additional feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[Chris0Jeky]

Adversarial Review — Round 2 (consolidated)

Two independent adversarial reviews were run against this PR with distinct lenses:

1. Concurrency-correctness lens — 0 substantive findings.
2. Guard-strength / regression-sensitivity lens — 0 substantive findings.

Attack surface traced (all refuted against RedisCacheService source)

• Seam synchronicity: GetConnection invokes OnBeforeConnect synchronously on the connecting thread before any await, so the connectEntered / releaseConnect handshake is a genuine rendezvous — the dedicated thread cannot "miss" the seam.
• Join happens-before: connectorResult / connectorError are written on the redis-connector thread and read only after Thread.Join succeeds; Join establishes the happens-before edge, and the Join(...).Should().BeTrue() assertion throws before any read on the timeout path — stale values are never observed.
• Revert sensitivity: the test still fails hard against pre-Thread.Sleep inside lock in RedisCacheService.GetConnection starves thread pool #1189 lock-around-connect semantics (disposeElapsed ~7s vs. the unchanged 500ms bound) — the mechanism change did not weaken the regression guard.
• Join(5s) flake surface: post-releaseConnect.Set() the connecting thread only has to finish a failed connect against a refused loopback port (connectRetry=0, 3s cap) and the degrade path; the 5s bounded Join is not a new starvation-sensitive wait because the thread is already running (OS-scheduled, not pool-queued).
• Cross-test bleed: the seam (OnBeforeConnect) and cache instance are per-test-instance (xUnit creates a fresh fixture per test), so no state leaks between the two seam-using tests.

Actionable items

• INFO-1 (informational, folded into this PR): the ManualResetEventSlim instances in the test file are never disposed — releaseConnect + connectEntered in Dispose_IsNotSerialized_BehindAnInFlightConnect, and the sibling events in ConnectionLock_IsNotHeld_WhileBlockingConnectRuns. Fix: using var declarations in BOTH tests for file consistency. No assertion, timing, or mechanism change.

Summary

0 CRITICAL / 0 HIGH / 0 MEDIUM / 0 LOW substantive findings across both lenses. One informational disposal-hygiene item (INFO-1) to be fixed in this PR. Not merge-blocking.

[Chris0Jeky]

Adversarial Review — Fixes Applied (Round 2)

Finding	Severity	Fix Commit	Verified
INFO-1: ManualResetEventSlim instances never disposed (releaseConnect + connectEntered in Dispose_IsNotSerialized_BehindAnInFlightConnect; probeCompleted + seamFired in ConnectionLock_IsNotHeld_WhileBlockingConnectRuns)	INFO	c9119919	Build clean (0 errors); 5/5 consecutive targeted runs green (--filter FullyQualifiedName~RedisCacheServiceTests, 8/8 tests each run)

Fix detail: all four events converted to using var declarations — disposal-only, no assertion, timing, or mechanism change. Disposal at method exit cannot race a live waiter: in the Dispose test both events are last touched before the bounded Join succeeds, and in the ConnectionLock test the probe thread sets probeCompleted before the seam returns.

All findings addressed. CI status: PENDING (fresh push, checks re-running).