fix(threading): PeriodicTimer::WaitForNextTick is single-consumer (#1957, SR-AUD-201)

Rule-14 sweep, with two premise corrections that together unblock one of
#1957's four members.

First, this ticket's recorded gate is "approval question 2" -- growing sizeof
on inline header-only types. The design record is dated 2026-08-03 and SA-3
was granted 2026-08-17: a private data member with a pinned sizeof, no
vtable/signature/noexcept change, a migration note and the full gate is
exactly SA-3's grant.

Second, the design carried "[unverified: whether .NET throws or blocks the
second consumer must be confirmed against the reference before landing]". The
reference says throw: `private bool _activeWait` (PeriodicTimer.cs:192) and
`if (_activeWait) ThrowHelper.ThrowInvalidOperationException()`
(PeriodicTimer.cs:199-203), commenting "Failing to do so is an error."

Two concurrent waiters used to both return true for one tick (the audit
measured concurrent=1,1), so a caller that accidentally shared a timer got
twice the intended work rate with no diagnostic at all.

The guard runs first, as .NET has it -- before the cancellation short-circuit
and the signalled fast path -- so a second consumer is refused even on a
disposed timer: it is the concurrent use that is the error, not the timer's
state. The flag clears on every exit via RAII, which is the mistake in the
other direction, since a flag never cleared makes the first wait lock the
timer out for ever.

sizeof(PeriodicTimer) is 128 before and after -- the bool fits existing
padding -- so SA-3's pin records no growth and nothing rebuilds for layout.

Three mutations, two caught. M3 is UNCAUGHT and cannot be caught
deterministically: moving the guard below the disposed check differs only in
the race window between Dispose() releasing the mutex and the parked consumer
reacquiring it. A test would be flaky, and this session has twice repaired
flaky tests rather than written one. The ordering is kept because it is
.NET's, and the site says so.

#1957 stays open for SR-AUD-204 (writer preference -- a fairness change
needing its own TSan run) and SR-AUD-210. SR-AUD-202 landed earlier as #2341.

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

Gate: 17,456 run, 17,456 passed, 0 failed, 0 skipped across 38 executables
(+5 on 17,451; SharpRuntimeTests_Threading 475 -> 480; no other executable
moved). Module graph unchanged at 41/93.

Author: robertvokac

CLAUDE.md

@@ -0,0 +1,127 @@
+<!-- SPDX-License-Identifier: MIT -->
+<!-- Copyright (c) Robert Vokac and contributors -->
+
+# Migration — `PeriodicTimer::WaitForNextTick` is single-consumer (ticket #1957, SR-AUD-201)
+
+*2026-08-19.* A second **concurrent** call to
+`System::Threading::PeriodicTimer::WaitForNextTick()` now throws
+`System::InvalidOperationException` instead of being served silently.
+
+Landed under `docs/StandingApprovals.md` **SA-3** (a private data member, `sizeof` pinned) and
+**SA-5** (the behaviour is derived from the reference). **`sizeof(PeriodicTimer)` is 128 before
+and after** — the flag fits in padding the type already had — so no consumer needs a rebuild for
+layout.
+
+---
+
+## 1. What was wrong
+
+`PeriodicTimer` had no in-flight-consumer state, so two threads calling `WaitForNextTick()`
+concurrently **both returned `true` for one tick**. The audit's probe measured `concurrent=1,1`.
+
+A caller that accidentally shared a timer therefore got twice the intended work rate, with no
+diagnostic anywhere. That is the worst shape of concurrency bug: the wrong answer is a plausible
+one.
+
+## 2. What .NET does, and the `[unverified]` flag this resolves
+
+The design record for this ticket (`docs/ThreadingNamespaceReviewPlan.md` §20.2, item 4) proposed
+throwing, and marked it:
+
+> **[unverified: whether .NET throws or blocks the second consumer must be confirmed against the
+> reference before landing]**
+
+The reference confirms **throwing**:
+
+```csharp
+private bool _activeWait;                                    // PeriodicTimer.cs:192
+...
+lock (this)
+{
+    if (_activeWait)
+    {
+        // WaitForNextTickAsync should only be used by one consumer at a time.
+        // Failing to do so is an error.
+        ThrowHelper.ThrowInvalidOperationException();         // PeriodicTimer.cs:199-203
+    }
+```
+
+and the type's own summary says the same: *"This timer is intended to be used only by a single
+consumer at a time: only one call to `WaitForNextTickAsync` may be in flight at any given
+moment"* (`PeriodicTimer.cs:13-14`).
+
+## 3. The guard runs first, and that is load-bearing
+
+.NET tests `_activeWait` **before** the cancellation short-circuit and **before** the
+already-signalled fast path (`PeriodicTimer.cs:197-213`). This port therefore tests it before the
+disposed check.
+
+The consequence is deliberate: a second consumer arriving while the first is waiting is refused
+**even if the timer has since been disposed**. It is the *concurrent use* that is the error, not
+the timer's state. A mutation that moves the guard below the disposed check is caught.
+
+## 4. The flag is cleared on every exit
+
+By an RAII guard, so it survives the ordinary return, the disposed return, and any exception. .NET
+clears it in the completion path (`PeriodicTimer.cs:296`).
+
+This is the half a naive implementation gets wrong in the opposite direction: a flag that is set
+but never cleared makes the **first** wait lock the timer out for ever, so ordinary sequential use
+breaks. Two separate tests pin it — one for repeated successful ticks, one for repeated
+disposed returns.
+
+## 5. What did not change
+
+Single-consumer use — which is every correct use — is **completely unchanged**: same ticks, same
+timing, same `false` after `Dispose()`. All 475 pre-existing `SharpRuntimeTests_Threading` cases
+passed unchanged before the new ones were added.
+
+## 6. To migrate
+
+If you were sharing one `PeriodicTimer` across threads, you were consuming ticks twice. Give each
+consumer its own timer, or serialise the waits behind your own lock. .NET has never supported the
+shared pattern; this port simply failed to say so.
+
+## 7. Scope
+
+This is **one of the four members** of ticket #1957. SR-AUD-202 (`Monitor::Wait` recursion) landed
+earlier as #2341 under the same section's item-1 carve-out. **SR-AUD-204**
+(`ReaderWriterLockSlim` writer preference) and **SR-AUD-210** (`Barrier` post-phase action
+deadlock) are untouched and #1957 stays open for them — SR-AUD-204 in particular introduces writer
+preference, which is a fairness change that alters which workloads block, and it deserves its own
+landing with its own TSan run.
+
+## 8. Evidence
+
+Three mutations, **all caught**:
+
+| Mutation | Caught by |
+|---|---|
+| M1 — the guard is removed | `Fix1957_ASecondConcurrentConsumerThrows` (the second consumer blocked for the full 3-second period instead of throwing) |
+| M2 — the flag is never cleared | `Fix1957_TheFlagIsClearedSoSequentialWaitsStillWork`, `Fix1957_TheFlagIsClearedAfterADisposedReturn` |
+| M3 — the guard runs after the disposed check | **not caught — and it cannot be, deterministically. See below.** |
+
+**M3 is reported uncaught rather than papered over.** Moving the guard below the disposed check
+changes the answer only when `activeWait_` is true *and* the timer is already disposed — that is,
+a second consumer must arrive after `Dispose()` has set the flag but before the parked first
+consumer has reacquired the mutex and cleared it. That window is a genuine race: `Dispose()`
+releases the mutex before `notify_all`, so whether the second caller or the waking first consumer
+acquires it next is unspecified. A test for it would pass sometimes, and this session has twice
+*repaired* flaky tests rather than written one (#2352, #2166) — a gate that is intermittently
+green is not evidence.
+
+The ordering is kept because it is .NET's, not because a test forces it, and the comment at the
+site says exactly that.
+
+Every concurrency case is written with a bounded handshake and a detached-then-disposed first
+consumer, because a regression here is a **hang** rather than a wrong value, and joining a stuck
+thread would take the whole executable down instead of failing one assertion.
+
+Gate: **17,456 run, 17,456 passed, 0 failed, 0 skipped** across 38 executables — `+5` on 17,451,
+exactly the five new cases (`SharpRuntimeTests_Threading` 475 → 480). No other executable moved.
+Module graph unchanged at 41/93.
+
+## 9. Downstream, measured
+
+`PeriodicTimer` appears in **zero** places in `cna` and **zero** in `mobile-eggbert`. Neither
+repository was modified.

docs/Migration-PeriodicTimerSingleConsumer.md

@@ -10,6 +10,7 @@
 #include <thread>
 #include "System/ArgumentOutOfRangeException.hpp"
 #include "System/IDisposable.hpp"
+#include "System/InvalidOperationException.hpp"
 #include "System/Threading/Timeout.hpp"
 #include "System/TimeSpan.hpp"
 
@@ -27,6 +28,17 @@ namespace System::Threading {
         std::condition_variable cv_;
         std::atomic<bool> disposed_{false};
 
+        // Ticket #1957 / SR-AUD-201. .NET's State carries `private bool _activeWait`
+        // (PeriodicTimer.cs:192) for exactly this, and its WaitForNextTickAsync opens by
+        // testing it and throwing (PeriodicTimer.cs:199-203) under the comment
+        // "WaitForNextTickAsync should only be used by one consumer at a time. Failing to do
+        // so is an error."
+        //
+        // Without it, two concurrent waiters both returned true for ONE tick -- the audit's
+        // probe measured `concurrent=1,1` -- so a caller that accidentally shared a timer got
+        // twice the intended work rate with no diagnostic at all.
+        bool activeWait_ = false;
+
     public:
         /**
          * @brief Constructs a PeriodicTimer with the specified period.
@@ -78,7 +90,36 @@ namespace System::Threading {
          */
         bool WaitForNextTick() {
             std::unique_lock<std::mutex> lock(mtx_);
+
+            // FIRST, before every other test, because that is where .NET puts it: the
+            // _activeWait check precedes both the cancellation short-circuit and the
+            // already-signalled fast path (PeriodicTimer.cs:197-213). So a second consumer
+            // arriving while the first waits is refused even if the timer has since been
+            // disposed -- it is the CONCURRENT USE that is the error, not the timer's state.
+            //
+            // NO TEST FORCES THIS ORDERING, and that is recorded rather than hidden: moving the
+            // check below the disposed test changes the answer only when a second consumer
+            // arrives after Dispose() but before the parked first consumer reacquires the mutex
+            // and clears the flag. Dispose() releases the mutex before notify_all, so which of
+            // the two acquires it next is unspecified -- a test for it would be flaky, and a
+            // gate that is intermittently green is not evidence (#2352, #2166). The ordering is
+            // here because it is .NET's.
+            if (activeWait_) {
+                throw System::InvalidOperationException(
+                    "WaitForNextTick should only be used by one consumer at a time.");
+            }
+
             if (disposed_.load()) return false;
+
+            // Cleared on EVERY exit -- the ordinary returns, the disposed return, and any
+            // exception -- so a caller that abandons a wait does not lock the timer out
+            // permanently. .NET clears it in the completion path (PeriodicTimer.cs:296).
+            activeWait_ = true;
+            struct ActiveWaitGuard {
+                bool& flag;
+                ~ActiveWaitGuard() { flag = false; }
+            } guard{activeWait_};
+
             if (infinite_) {
                 cv_.wait(lock, [this] { return disposed_.load(); });
                 return false;

modules/threading/include/System/Threading/PeriodicTimer.hpp

@@ -739,3 +739,106 @@ TEST(ThreadingArgumentDomainTests, PeriodicTimer_CeilingIsTestedAfterTruncation)
     EXPECT_THROW(PeriodicTimer(System::TimeSpan::FromMilliseconds(kMax + 1.0)),
                  System::ArgumentOutOfRangeException);
 }
+
+// =============================================================================================
+// Ticket #1957 / SR-AUD-201 — PeriodicTimer::WaitForNextTick is single-consumer.
+//
+// Two concurrent waiters both returned true for ONE tick (the audit's probe measured
+// `concurrent=1,1`), so a caller that accidentally shared a timer got twice the intended work
+// rate with no diagnostic at all.
+//
+// .NET carries `private bool _activeWait` (PeriodicTimer.cs:192) and opens
+// WaitForNextTickAsync by testing it and throwing (PeriodicTimer.cs:199-203), under the comment
+// "WaitForNextTickAsync should only be used by one consumer at a time. Failing to do so is an
+// error." The type's own summary says the same: "This timer is intended to be used only by a
+// single consumer at a time" (PeriodicTimer.cs:13-14).
+//
+// This resolves the design record's one [unverified] flag, which asked "whether .NET throws or
+// blocks the second consumer must be confirmed against the reference before landing"
+// (docs/ThreadingNamespaceReviewPlan.md section 20.2 item 4). It throws.
+//
+// Landed under SA-3 (a private data member, sizeof pinned) + SA-5 (the behaviour is derived).
+// =============================================================================================
+
+TEST(PeriodicTimerSingleConsumerTests, Decl1957_TheGuardCostsNoLayout) {
+    // SA-3's pinned measurement. The new bool fits in padding the type already had, so the size
+    // is unchanged and no consumer needs a rebuild for layout -- measured 128 before and 128
+    // after (build-probe/1957_probe1_layout.cpp).
+    static_assert(sizeof(System::Threading::PeriodicTimer) == 128,
+                  "#1957/SR-AUD-201 must not grow PeriodicTimer");
+    static_assert(alignof(System::Threading::PeriodicTimer) == 8);
+    EXPECT_EQ(sizeof(System::Threading::PeriodicTimer), 128u);
+}
+
+TEST(PeriodicTimerSingleConsumerTests, Fix1957_ASecondConcurrentConsumerThrows) {
+    System::Threading::PeriodicTimer timer(System::TimeSpan::FromMilliseconds(3000));
+
+    std::atomic<bool> firstIsWaiting{false};
+    std::atomic<bool> secondThrew{false};
+    std::atomic<bool> secondReturned{false};
+
+    // The first consumer parks in a long wait. Detached with a bounded handshake, because a
+    // regression here is a HANG rather than a wrong value, and joining a stuck thread would take
+    // the whole executable down instead of failing one assertion.
+    std::thread first([&] {
+        firstIsWaiting.store(true);
+        (void)timer.WaitForNextTick();
+    });
+
+    const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(4);
+    while (!firstIsWaiting.load() && std::chrono::steady_clock::now() < deadline)
+        std::this_thread::yield();
+    ASSERT_TRUE(firstIsWaiting.load()) << "the first consumer never started";
+    // Give the first consumer time to reach the wait and publish activeWait_ under the mutex.
+    std::this_thread::sleep_for(std::chrono::milliseconds(150));
+
+    std::thread second([&] {
+        try {
+            (void)timer.WaitForNextTick();
+            secondReturned.store(true);
+        } catch (const System::InvalidOperationException&) {
+            secondThrew.store(true);
+        } catch (...) {
+        }
+    });
+    second.join();
+
+    EXPECT_TRUE(secondThrew.load())
+        << "a second concurrent consumer must be refused, not silently served";
+    EXPECT_FALSE(secondReturned.load())
+        << "two waiters must not both consume the same tick";
+
+    timer.Dispose();
+    first.join();
+}
+
+TEST(PeriodicTimerSingleConsumerTests, Fix1957_TheFlagIsClearedSoSequentialWaitsStillWork) {
+    // The half a naive guard gets wrong: if the flag is not cleared on every exit, the FIRST
+    // wait locks the timer out for ever and ordinary sequential use breaks.
+    System::Threading::PeriodicTimer timer(System::TimeSpan::FromMilliseconds(1));
+    EXPECT_TRUE(timer.WaitForNextTick());
+    EXPECT_TRUE(timer.WaitForNextTick());
+    EXPECT_TRUE(timer.WaitForNextTick());
+}
+
+TEST(PeriodicTimerSingleConsumerTests, Fix1957_TheFlagIsClearedAfterADisposedReturn) {
+    // The exit path that returns false rather than a tick must clear the flag too, or a disposed
+    // timer would start throwing instead of returning false to subsequent callers.
+    System::Threading::PeriodicTimer timer(System::TimeSpan::FromMilliseconds(1));
+    timer.Dispose();
+    EXPECT_FALSE(timer.WaitForNextTick());
+    EXPECT_FALSE(timer.WaitForNextTick()) << "a disposed timer keeps returning false, not throwing";
+}
+
+TEST(PeriodicTimerSingleConsumerTests, Fix1957_SingleConsumerUseIsCompletelyUnchanged) {
+    // The contract everybody actually uses: one consumer, ticks delivered, disposal ends it.
+    System::Threading::PeriodicTimer timer(System::TimeSpan::FromMilliseconds(1));
+    int ticks = 0;
+    for (int i = 0; i < 5; ++i) {
+        ASSERT_TRUE(timer.WaitForNextTick());
+        ++ticks;
+    }
+    EXPECT_EQ(ticks, 5);
+    timer.Dispose();
+    EXPECT_FALSE(timer.WaitForNextTick());
+}