fix(threading): Dispose also refuses a ReaderWriterLockSlim with waiters (#2389)

Completes what #1956 half-landed. .NET's Dispose performs TWO checks
(ReaderWriterLockSlim.cs:1250-1258) and #1956's design record named only the
held-mode one, so Dispose accepted a lock other threads were waiting for.
waitingReaders_ and waitingUpgraders_ join SR-AUD-204's waitingWriters_, and
both checks now run in .NET's order.

The two new counters feed no admission predicate -- only writer-waiting does
-- so their guards take notifyOnLast=false and cannot perturb wake-up
ordering.

sizeof 120 -> 128: SR-AUD-204's single counter fit existing padding and these
two did not, so consumers must rebuild. The pre-existing layout gate was
updated from 120 to 128.

Four mutations, all caught -- but only after two defects were found, and
those are the substance of this ticket:

  1. The first run reported all four "caught" by a test that was ALREADY
     FAILING: the layout gate asserts 120 and was failing on the unmutated
     build too, so no verdict meant anything. A mutation verdict is only
     evidence against a baseline that passes.
  2. Re-run green, M1 and M2 came back NOT CAUGHT -- a real defect in my
     tests, which disposed from a thread that also held a mode, so the
     held-mode check fired and the waiter check was never exercised. The
     disposer must hold nothing.
  3. The corrected tests then flaked inside the gate: a fixed 150 ms settle
     does not guarantee the waiter has been counted. Tuning the sleep would
     have been the wrong repair. They now rebuild the scenario over up to six
     attempts and pass the moment Dispose refuses -- sound rather than
     tolerant, since only a genuinely firing check can pass, and every
     mutation still fails all six.

That layout gate doing its job is also the evidence 128 is a real change
rather than drift: it stayed green through SR-AUD-204's earlier counter.

Downstream measured: 0 sites in cna, 0 in mobile-eggbert.

Gate: 17,482 run, 17,482 passed, 0 failed, 0 skipped across 38 executables
(+6 on 17,476; SharpRuntimeTests_Threading 500 -> 506; no other executable
moved). Module graph unchanged at 41/93.

Author: robertvokac

CLAUDE.md

@@ -0,0 +1,126 @@
+<!-- SPDX-License-Identifier: MIT -->
+<!-- Copyright (c) Robert Vokac and contributors -->
+
+# Migration — `ReaderWriterLockSlim::Dispose` also refuses a lock with waiters (ticket #2389)
+
+*2026-08-19.* `Dispose()` now throws `SynchronizationLockException` when any thread is **waiting**
+to acquire the lock, not only when the calling thread **holds** a mode. Both checks are .NET's,
+in .NET's order.
+
+**`sizeof(ReaderWriterLockSlim)` grows 120 → 128, so consumers must be recompiled.** Landed under
+**SA-3** (private data members, `sizeof` pinned) and **SA-5** (behaviour derived from the
+reference).
+
+---
+
+## 1. What this completes
+
+.NET's `Dispose(bool)` performs **two** checks (`ReaderWriterLockSlim.cs:1250-1258`):
+
+```csharp
+if (WaitingReadCount > 0 || WaitingUpgradeCount > 0 || WaitingWriteCount > 0)
+    throw new SynchronizationLockException(SR.SynchronizationLockException_IncorrectDispose);
+
+if (IsReadLockHeld || IsUpgradeableReadLockHeld || IsWriteLockHeld)
+    throw new SynchronizationLockException(SR.SynchronizationLockException_IncorrectDispose);
+```
+
+Ticket #1956 landed the **second** — its design record named only that one, so the omission was a
+gap in the record rather than a shortcut taken while implementing it. The first needed per-mode
+waiter counts. #1957/SR-AUD-204 added `waitingWriters_` for a different purpose (writer
+preference); this ticket adds `waitingReaders_` and `waitingUpgraders_` and wires all three into
+`Dispose`.
+
+## 2. What changes
+
+| `Dispose()` called while… | Was | Is |
+|---|---|---|
+| nothing held, nobody waiting | succeeds | succeeds — unchanged, still idempotent |
+| the calling thread holds a mode | throws (#1956) | throws — unchanged |
+| **another thread is waiting for a read lock** | **succeeded** | `SynchronizationLockException` |
+| **another thread is waiting for the write lock** | **succeeded** | `SynchronizationLockException` |
+| **another thread is waiting to upgrade** | **succeeded** | `SynchronizationLockException` |
+| a waiter that has since **timed out** | — | succeeds; the counters come back down |
+
+## 3. The new counters affect no admission decision
+
+Only *writer*-waiting influences who may enter, which is SR-AUD-204's rule and .NET's.
+`waitingReaders_` and `waitingUpgraders_` are consulted by `Dispose` **and nothing else** — their
+guards are constructed with `notifyOnLast = false` precisely so they cannot perturb wake-up
+ordering. That is stated at the site.
+
+All three use the same RAII guard, so a waiter that **times out or throws** stops being counted.
+Without that, a lock that ever had a waiter could never be disposed again — a permanent failure,
+not a transient one, and a test pins it.
+
+## 4. The order is transcribed, not chosen
+
+.NET tests **waiters first**. Both arms raise the same message in this port, so which one fires
+is currently *unobservable* — the ordering is nonetheless .NET's, and a test constructs the case
+where both conditions hold so that if the messages ever diverge the ordering is already covered
+rather than newly at risk.
+
+## 5. Layout
+
+| | Was | Is |
+|---|---|---|
+| `sizeof(ReaderWriterLockSlim)` | **120** | **128** |
+| `alignof` | 8 | 8 |
+
+SR-AUD-204's single counter was layout-neutral — it landed in padding the type already had. These
+two did not fit, so this is a real object-layout change: **every consumer must be recompiled.**
+No source change is needed. The pin was updated in place rather than duplicated, and it now names
+both tickets so a third counter cannot arrive unnoticed.
+
+## 6. Evidence
+
+Four mutations, **all caught**:
+
+| Mutation | Caught by |
+|---|---|
+| M1 — the waiter check is removed | all three waiting-kind tests |
+| M2 — only writers are counted | `Fix2389_DisposingWithAWaitingReaderThrows`, `Fix2389_DisposingWithAWaitingUpgraderThrows` |
+| M3 — the reader guard never decrements | `Fix2389_OnceTheWaiterGivesUpDisposalSucceeds`, `Decl2389_TheHeldModeCheckStillFiresOnItsOwn`, and #1956's held-mode test |
+| M4 — the upgrader is not counted | `Fix2389_DisposingWithAWaitingUpgraderThrows` |
+
+**A second process note, and it is the more useful one.** With the layout gate fixed and the
+baseline green, M1 and M2 came back **NOT CAUGHT** — a real defect in the tests, not in the
+repair. Every case had the **disposing thread also holding a mode**, so the *held-mode* check
+fired and the waiter check was never the one that threw. The cases now use a third thread to hold
+the lock, a second to wait for it, and dispose from a thread holding **nothing** — which is the
+only shape where the waiter check can be observed at all. All four mutations are caught against
+that.
+
+**One process note, and it is the reason these verdicts are trustworthy.** The first mutation run
+reported all four as "caught" — by `ThreadingSharedStateTests.RepairedTypes_LayoutUnchanged`,
+a **pre-existing** layout gate that asserts `sizeof(ReaderWriterLockSlim) == 120`. That gate was
+failing on the **unmutated** build too, because of the 120 → 128 growth, so every run had a
+failure regardless of the mutation and none of the four results meant anything. The gate was
+updated to 128 first, a green baseline confirmed (506 cases), and only then were the mutations
+re-run. A mutation verdict is only evidence against a baseline that passes.
+
+That gate doing its job is also the evidence that 128 is a **real** change rather than drift: it
+stayed green through SR-AUD-204's earlier counter, which landed in existing padding.
+
+**A third process note: the first robust-looking version still flaked.** With the tests
+restructured so the disposer holds nothing, they passed in isolation and then **failed inside the
+full gate** — a fixed 150 ms settle is not a guarantee that the waiting thread has reached the
+wait and been counted, and under gate load it had not. Tuning the sleep upward would have been
+the wrong repair; this repository has twice *repaired* such a test rather than tuned it (#2352,
+#2166).
+
+The cases now rebuild the whole scenario on each of up to six attempts, with a growing settle,
+and pass the moment `Dispose` refuses. That is **sound rather than merely tolerant**: the case can
+only pass if the waiter check genuinely fires, so every mutation that removes or narrows it fails
+all six attempts and is still caught by name. Verified green three consecutive times in isolation
+and once through the full gate.
+
+Gate: **17,482 run, 17,482 passed, 0 failed, 0 skipped** across 38 executables — `+6` on 17,476,
+exactly the six new cases (`SharpRuntimeTests_Threading` 500 → 506). No other executable moved.
+Module graph unchanged at 41/93.
+
+## 7. Downstream, measured
+
+`ReaderWriterLockSlim` appears in **zero** places in `cna` and **zero** in `mobile-eggbert`, so
+the rebuild requirement is recorded here for future consumers rather than acted on. Neither
+repository was modified.

docs/Migration-ReaderWriterDisposeWaiterCheck.md

@@ -106,6 +106,25 @@ namespace System::Threading {
         // (ReaderWriterLockSlim.cs:1039-1042), so a writer that TIMES OUT stops blocking readers;
         // the RAII guard in TryEnterWriteLock does the same here.
         intcs waitingWriters_ = 0;
+        // Ticket #2389. .NET's Dispose refuses a lock that has WAITERS as well as one with a
+        // held mode, and it counts all three kinds (ReaderWriterLockSlim.cs:1254). #1956
+        // implemented the held-mode check only, because these two counters did not exist; they
+        // do now, on the same RAII pattern waitingWriters_ uses.
+        //
+        // These are NOT consulted by any admission predicate -- only writer-waiting affects
+        // admission, which is #1957/SR-AUD-204's rule and .NET's. They exist for Dispose.
+        intcs waitingReaders_ = 0;
+        intcs waitingUpgraders_ = 0;
+
+        /// Increments a waiter count for the duration of a wait and restores it on every exit.
+        struct WaiterCountGuard {
+            intcs&                   count;
+            std::condition_variable& cv;
+            bool                     notifyOnLast;
+            ~WaiterCountGuard() {
+                if (--count == 0 && notifyOnLast) cv.notify_all();
+            }
+        };
         bool writerActive_ = false;
         bool upgradeableActive_ = false;
         // Ticket #1955 / cause T-A of docs/ThreadingNamespaceReviewPlan.md. This was an
@@ -236,6 +255,9 @@ namespace System::Threading {
             // holds the read, write or upgrade lock never reaches here -- every one of those
             // cases returned above -- so writer preference can only delay a genuinely NEW
             // reader, which is precisely .NET's contract and cannot deadlock a recursive one.
+            // #2389: counted for Dispose's benefit only -- a waiting reader blocks nothing.
+            ++waitingReaders_;
+            WaiterCountGuard readerWaitGuard{waitingReaders_, cv_, /*notifyOnLast=*/false};
             if (!waitFor(lk, millisecondsTimeout,
                          [&] { return !writerActive_ && waitingWriters_ == 0; }))
                 return false;
@@ -304,13 +326,7 @@ namespace System::Threading {
             // Declared after `lk` so it is destroyed BEFORE the lock is released: the decrement
             // and the notify both happen under the mutex.
             ++waitingWriters_;
-            struct WaitingWriterGuard {
-                intcs&                   count;
-                std::condition_variable& cv;
-                ~WaitingWriterGuard() {
-                    if (--count == 0) cv.notify_all();
-                }
-            } waitingWriterGuard{waitingWriters_, cv_};
+            WaiterCountGuard waitingWriterGuard{waitingWriters_, cv_, /*notifyOnLast=*/true};
 
             bool acquired = upgradingToWrite
                 ? waitFor(lk, millisecondsTimeout, [&] { return readers_ == 0; })
@@ -384,6 +400,9 @@ namespace System::Threading {
                 throw LockRecursionException("Upgradeable lock may not be acquired with read lock held.");
 
             std::unique_lock<std::mutex> lk(stateMtx_);
+            // #2389: counted for Dispose's benefit only.
+            ++waitingUpgraders_;
+            WaiterCountGuard upgraderWaitGuard{waitingUpgraders_, cv_, /*notifyOnLast=*/false};
             if (!waitFor(lk, millisecondsTimeout, [&] { return !writerActive_ && !upgradeableActive_; })) return false;
             upgradeableActive_ = true;
             ++counts.upgrade;
@@ -409,19 +428,29 @@ namespace System::Threading {
          * owning a mode on a disposed object. .NET refuses
          * (`ReaderWriterLockSlim.cs:1250-1258`).
          *
-         * @note **.NET performs TWO checks here and this port performs one**, which is stated
-         * rather than glossed. The reference tests, in this order:
+         * @throws System::Threading::SynchronizationLockException if any thread is WAITING to
+         *         acquire the lock.
+         *
+         * Both checks are .NET's, in .NET's order (`ReaderWriterLockSlim.cs:1250-1258`):
          * @code
          * if (WaitingReadCount > 0 || WaitingUpgradeCount > 0 || WaitingWriteCount > 0) throw ...;
          * if (IsReadLockHeld || IsUpgradeableReadLockHeld || IsWriteLockHeld)          throw ...;
          * @endcode
-         * The second is implemented here. The first needs per-mode WAITER counts, of which this
-         * port has only `waitingWriters_` (added by SR-AUD-204); counting waiting readers and
-         * upgraders is additional state on three more paths and is ticket **#2389**. The
-         * narrowing is therefore a strict subset of .NET's -- this port never refuses a disposal
-         * .NET would accept.
+         * #1956 landed the second; ticket **#2389** added the waiter counts and the first.
+         *
+         * @note The order is transcribed rather than chosen. Both arms raise the same message
+         * here, so which one fires is currently unobservable -- but the ordering is .NET's and a
+         * test pins it, so it cannot be inverted casually if the messages ever diverge.
          */
         void Dispose() override {
+            std::unique_lock<std::mutex> lk(stateMtx_);
+            if (waitingReaders_ > 0 || waitingUpgraders_ > 0 || waitingWriters_ > 0) {
+                throw System::Threading::SynchronizationLockException(
+                    "The lock is being disposed while still being used. It either is being held "
+                    "by a thread and/or has active waiters waiting to acquire the lock.");
+            }
+            lk.unlock();
+
             auto& map = threadCounts();
             auto it = map.find(id_);
             if (it != map.end() &&