fix(threading): a Barrier post-phase action can read the phase number (#1957, SR-AUD-210)

Rule-14 sweep. FinishPhase() invokes the post-phase action while HOLDING
mutex_, and getCurrentPhaseNumberProperty() took that same non-recursive
mutex, so a legal call from inside the action self-deadlocked. Not a new
discovery: #1955 fixed the sibling property the same way and its comment
names this one as the remaining case.

.NET's CurrentPhaseNumber is Volatile.Read(ref _currentPhase)
(Barrier.cs:184-188), a lock-free read of a plain field, so the reference
settles the design. phaseCount_ becomes std::atomic<longcs> and is read
without the lock. sizeof(Barrier) is 160 before and after, so nothing
rebuilds for layout.

The design record named only the deadlock, and the reference exposes a second
half: .NET increments the phase in SetResetEvents, called from FinishPhase's
finally -- after the action, and on the throwing path too -- while this port
incremented first. That was unobservable only because the property that would
have seen it hung. Fixing the deadlock alone would have shipped a newly
reachable wrong answer in place of a hang, so both land together: inside the
action the phase is now the one ENDING, as .NET reports. Nothing outside the
action moves, since mutex_ is held throughout.

The design record instead proposed releasing mutex_ around the action and
reacquiring it. That is not what the reference does and is materially riskier
-- it would let other participants observe a half-finished transition.

The boundary is unchanged and pinned: only the two read-only properties are
callable from the action; the four mutating members still guard before
locking and throw.

Three mutations, all caught. Two process notes: M2 was invalid as first
written (adding the increment without removing the later one is a double
increment, not a move) and was reformulated rather than counted; and M3 was
first reported "not caught", which was a defect in my mutation harness found
by checking rather than believing it -- M3 hangs a pre-existing
multi-participant case, so the suite run hit its timeout and emitted no
FAILED lines, which a line-grepping harness cannot tell from a pass. Run
alone, the new pin fails in 0 ms.

#1957 now stays open for SR-AUD-204 alone.

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

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

Author: robertvokac

CLAUDE.md

@@ -0,0 +1,122 @@
+<!-- SPDX-License-Identifier: MIT -->
+<!-- Copyright (c) Robert Vokac and contributors -->
+
+# Migration — a `Barrier` post-phase action can read the phase number (ticket #1957, SR-AUD-210)
+
+*2026-08-19.* `System::Threading::Barrier::getCurrentPhaseNumberProperty()` no longer takes the
+barrier's lock, so calling it from inside a post-phase action no longer self-deadlocks. The phase
+increment moved to **after** the action, which is where .NET performs it.
+
+Landed under **SA-5**, with SA-3's layout condition discharged as **layout-neutral**:
+`sizeof(Barrier)` is **160 before and after**.
+
+---
+
+## 1. What was wrong
+
+`FinishPhase()` invokes the post-phase action while **holding** `mutex_`, and
+`getCurrentPhaseNumberProperty()` took that same non-recursive `std::mutex`. So:
+
+```cpp
+Barrier barrier(1, [](Barrier& b) {
+    auto phase = b.getCurrentPhaseNumberProperty();   // deadlocked, permanently
+});
+barrier.SignalAndWait();
+```
+
+This is not a new discovery. Ticket #1955 fixed the sibling property
+`getParticipantCountProperty()` in exactly this way and **named this one as the remaining case**,
+in a comment that is still in the header.
+
+## 2. What .NET does
+
+`CurrentPhaseNumber` is a lock-free read of a plain field:
+
+```csharp
+public long CurrentPhaseNumber
+{
+    // use the new Volatile.Read/Write method because it is cheaper than Interlocked.Read
+    get { return Volatile.Read(ref _currentPhase); }      // Barrier.cs:184-188
+```
+
+so the reference settles the design: the property must not take the barrier's lock. `phaseCount_`
+becomes a `std::atomic<longcs>` and is read with `memory_order_acquire`, which is the same repair
+#1955 applied to `participantCount_`.
+
+## 3. The half the design record did not name
+
+.NET increments the phase in `SetResetEvents`, which `FinishPhase` calls from its `finally` —
+**after** the action has run, and on the throwing path too (`Barrier.cs:804-812, 834-836`).
+
+This port incremented **first**. That was unobservable only because the property that would have
+seen it deadlocked. **Fixing the deadlock alone would have shipped a newly reachable wrong answer
+in place of a hang** — the action would have read the phase about to begin, where .NET reports the
+one that is ending.
+
+So both changes land together:
+
+| Read from | Was | Is |
+|---|---|---|
+| inside the post-phase action | **deadlock** | the phase that is **ending** (0, 1, 2, …) |
+| after `SignalAndWait()` returns | 1, 2, 3, … | 1, 2, 3, … — **unchanged** |
+| after a **throwing** action | advanced | advanced — unchanged |
+
+Nothing outside the action can see the difference: `mutex_` is held for the whole of
+`FinishPhase`, so no waiter can run until it is released. The increment is still inside the
+critical section, before `notify_all`.
+
+## 4. The boundary that did not move
+
+Only the two **read-only** properties are callable from the action.
+`AddParticipant`, `RemoveParticipant`, `SignalAndWait` and `Dispose` all call
+`ThrowIfCalledFromPostPhaseAction()` **before** taking the lock, so they throw
+`InvalidOperationException` rather than deadlocking — matching .NET's `_actionCallerID` guard.
+That is pinned by a test asserting all four still refuse.
+
+## 5. Evidence
+
+Mutations, **all caught**:
+
+| Mutation | Caught by |
+|---|---|
+| M1 — the property takes the lock again | `Fix1957_ThePostPhaseActionCanReadThePhaseNumber` |
+| M2 — the increment moves back before the action | `Fix1957_ThePostPhaseActionCanReadThePhaseNumber`, `Fix1957_ThePhaseAdvancesAfterTheActionNotBefore`, `Fix1957_TheOtherMembersStillRefuseReentrancy` |
+| M3 — the phase advances only on the success path | `Fix1957_ThePhaseStillAdvancesWhenTheActionThrows` |
+
+**M2 was invalid as first written and was reformulated rather than counted**: adding the
+increment before the action without removing it afterwards is a *double* increment, not a move,
+and it broke two pre-existing tests for the wrong reason.
+
+**M3 was first reported "not caught", and that was a defect in the mutation harness, found by
+checking rather than by believing it.** M3 makes a *pre-existing* multi-participant `BarrierTests`
+case **hang** — waiters block on `phaseCount_ > myPhase` and the phase never advances — so the
+whole-suite run hit its timeout and produced no `[  FAILED  ]` lines at all, which the harness
+read as "no failures". Run on its own, `Fix1957_ThePhaseStillAdvancesWhenTheActionThrows` fails
+in 0 ms. The mutation is therefore caught **by name**, and it is also caught as a hang; a harness
+that only greps for failure lines cannot tell those two apart from a pass.
+
+Every case that exercises the action runs its work on a **detached** thread behind a bounded
+four-second handshake, because the pre-repair behaviour is a **hang**: joining a stuck thread
+would take the whole executable down instead of failing one assertion. That is the same shape
+#2341 used for `Monitor::Wait`.
+
+`Fix1957_ThePhaseAdvancesAfterTheActionNotBefore` asserts the whole sequence across three phases
+(`{0,1,2}` inside, `{1,2,3}` outside) rather than a single value, so an off-by-one cannot pass by
+accident.
+
+Gate: **17,462 run, 17,462 passed, 0 failed, 0 skipped** across 38 executables — `+6` on 17,456,
+exactly the six new cases (`SharpRuntimeTests_Threading` 480 → 486). No other executable moved.
+Module graph unchanged at 41/93.
+
+## 6. Scope
+
+This is the **third** of ticket #1957's four members. SR-AUD-202 (`Monitor::Wait` recursion)
+landed as #2341; SR-AUD-201 (`PeriodicTimer` single-consumer) landed earlier today. **SR-AUD-204**
+(`ReaderWriterLockSlim` writer preference) remains, and #1957 stays open for it — it introduces
+writer preference, a fairness change that alters which workloads block, and the plan's §12
+requires a ThreadSanitizer run for it specifically.
+
+## 7. Downstream, measured
+
+`System::Threading::Barrier` appears in **zero** places in `cna` and **zero** in
+`mobile-eggbert`. Neither repository was modified.

docs/Migration-BarrierPostPhasePhaseNumber.md

@@ -57,7 +57,22 @@ namespace System::Threading {
         // so the field is layout-neutral (build-probe/1955_probe1_layout_{before,after}.log).
         std::atomic<intcs> participantCount_;
         intcs remainingCount_;
-        longcs phaseCount_ = 0;
+        // Ticket #1957 / SR-AUD-210, cause T-E/2. This was a plain `longcs` read under mutex_,
+        // and FinishPhase() runs the post-phase action while HOLDING mutex_, so a legal
+        // `barrier.getCurrentPhaseNumberProperty()` inside that action deadlocked on a
+        // non-recursive std::mutex -- exactly the hazard #1955 recorded here when it fixed the
+        // sibling property, and it named this one as the remaining case.
+        //
+        // .NET's own CurrentPhaseNumber is `Volatile.Read(ref _currentPhase)` (Barrier.cs:184-188)
+        // -- a lock-free read of a plain field -- so the reference already answers this: the
+        // property must not take the barrier's lock. std::atomic<longcs> is 8 bytes and 8-aligned,
+        // the same as longcs, so the change is layout-neutral (sizeof(Barrier) is 160 before and
+        // after, build-probe/1957_probe2_barrier.cpp) -- the same argument #1955 made for
+        // participantCount_ and disposed_.
+        //
+        // Writers keep writing under mutex_, so every compound invariant with remainingCount_ is
+        // unaffected.
+        std::atomic<longcs> phaseCount_{0};
         std::function<void(Barrier&)> postPhaseAction_;
         mutable std::mutex mutex_;
         std::condition_variable cv_;
@@ -106,8 +121,22 @@ namespace System::Threading {
         [[nodiscard]] intcs  getParticipantCountProperty() const {
             return participantCount_.load(std::memory_order_acquire);
         }
-        /** Returns the current phase number. */
-        [[nodiscard]] longcs getCurrentPhaseNumberProperty() const { std::unique_lock lock(mutex_); return phaseCount_; }
+        /**
+         * @brief Returns the current phase number.
+         *
+         * Read without taking the barrier's lock, matching .NET's
+         * `Volatile.Read(ref _currentPhase)` (`Barrier.cs:184-188`) and keeping the property
+         * callable from inside a post-phase action (#1957, SR-AUD-210). Taking `mutex_` here
+         * deadlocked, because `FinishPhase()` invokes that action while holding it.
+         *
+         * @note **Inside the post-phase action this returns the phase that is ENDING, not the
+         * one about to begin.** That is .NET's value, not an accident of ordering: .NET
+         * increments in `SetResetEvents`, which its `FinishPhase` calls from the `finally`
+         * *after* the action has run (`Barrier.cs:781-816, 834-836`).
+         */
+        [[nodiscard]] longcs getCurrentPhaseNumberProperty() const {
+            return phaseCount_.load(std::memory_order_acquire);
+        }
 
         /**
          * @brief Signals that a participant has reached the barrier and blocks until all participants have arrived.
@@ -187,7 +216,9 @@ namespace System::Threading {
          * lastPostPhaseException_ once they wake.
          */
         void FinishPhase(std::unique_lock<std::mutex>& lock) {
-            ++phaseCount_;
+            // The participant count is reset BEFORE the action, matching .NET, which zeroes the
+            // arrival count in SetCurrentTotal and only then calls FinishPhase
+            // (Barrier.cs:454-458).
             remainingCount_ = participantCount_;
             if (postPhaseAction_) {
                 actionCallerId_.store(std::this_thread::get_id());
@@ -201,6 +232,19 @@ namespace System::Threading {
             } else {
                 lastPostPhaseException_ = nullptr;
             }
+
+            // #1957 MOVED THIS, and the move is what makes the now-reachable property answer
+            // what .NET answers. .NET increments the phase in SetResetEvents, which FinishPhase
+            // calls from its `finally` -- AFTER the action, and on the throwing path too
+            // (Barrier.cs:804-812, 834-836). So inside the action the phase is the one ENDING.
+            //
+            // The increment used to run first, which was unobservable only because the property
+            // that would have seen it deadlocked. Fixing the deadlock without moving it would
+            // have shipped a newly reachable wrong answer instead of a hang.
+            //
+            // Nothing outside the action can see the difference: mutex_ is held for this whole
+            // function, so no waiter can run until it is released.
+            ++phaseCount_;
             cv_.notify_all();
             (void)lock;
             if (lastPostPhaseException_)

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

@@ -23,15 +23,18 @@
 #include <chrono>
 #include <functional>
 #include <memory>
+#include <mutex>
 #include <thread>
 #include <utility>
 #include <vector>
 
 #include "System/ArgumentNullException.hpp"
 #include "System/ArgumentOutOfRangeException.hpp"
 #include "System/Exception.hpp"
+#include "System/InvalidOperationException.hpp"
 #include "System/NullReferenceException.hpp"
 #include "System/OperationCanceledException.hpp"
+#include "System/Threading/Barrier.hpp"
 #include "System/Threading/CancellationToken.hpp"
 #include "System/Threading/CancellationTokenRegistration.hpp"
 #include "System/Threading/CancellationTokenSource.hpp"
@@ -842,3 +845,151 @@ TEST(PeriodicTimerSingleConsumerTests, Fix1957_SingleConsumerUseIsCompletelyUnch
     timer.Dispose();
     EXPECT_FALSE(timer.WaitForNextTick());
 }
+
+// =============================================================================================
+// Ticket #1957 / SR-AUD-210 — the Barrier's post-phase action can read the barrier.
+//
+// FinishPhase() invokes the post-phase action while HOLDING mutex_, and
+// getCurrentPhaseNumberProperty() took that same non-recursive mutex, so a legal call from
+// inside the action self-deadlocked. #1955 fixed the sibling property
+// (getParticipantCountProperty) the same way and named this one as the remaining case.
+//
+// .NET's CurrentPhaseNumber is `Volatile.Read(ref _currentPhase)` (Barrier.cs:184-188) -- a
+// lock-free read of a plain field -- so the reference settles the design: the property must not
+// take the lock.
+//
+// THE SECOND HALF, which the design record did not name: .NET increments the phase in
+// SetResetEvents, called from FinishPhase's `finally` AFTER the action runs
+// (Barrier.cs:804-812, 834-836). This port incremented FIRST, which was unobservable only
+// because the property that would have seen it deadlocked. Fixing the deadlock alone would have
+// shipped a newly reachable WRONG ANSWER in place of a hang.
+//
+// Landed under SA-5, with SA-3's layout condition discharged as layout-neutral: sizeof(Barrier)
+// is 160 before and after (build-probe/1957_probe2_barrier.cpp).
+// =============================================================================================
+
+TEST(BarrierPostPhaseReadabilityTests, Decl1957_TheAtomicPhaseIsLayoutNeutral) {
+    static_assert(sizeof(System::Threading::Barrier) == 160,
+                  "#1957/SR-AUD-210 must not change Barrier's layout");
+    static_assert(alignof(System::Threading::Barrier) == 8);
+    EXPECT_EQ(sizeof(System::Threading::Barrier), 160u);
+}
+
+TEST(BarrierPostPhaseReadabilityTests, Fix1957_ThePostPhaseActionCanReadThePhaseNumber) {
+    // The deadlock itself. Before the repair this test HANGS rather than fails, which is why the
+    // work runs on a detached thread behind a bounded handshake: a stuck join would take the whole
+    // executable down instead of failing one assertion.
+    std::atomic<bool> done{false};
+    std::atomic<long long> seenInsideAction{-1};
+
+    std::thread worker([&] {
+        System::Threading::Barrier barrier(1, [&](System::Threading::Barrier& b) {
+            seenInsideAction.store(b.getCurrentPhaseNumberProperty());
+        });
+        barrier.SignalAndWait();
+        done.store(true);
+    });
+    worker.detach();
+
+    const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(4);
+    while (!done.load() && std::chrono::steady_clock::now() < deadline)
+        std::this_thread::sleep_for(std::chrono::milliseconds(5));
+
+    ASSERT_TRUE(done.load())
+        << "reading the phase number from the post-phase action deadlocked";
+    EXPECT_EQ(seenInsideAction.load(), 0)
+        << "inside the action the phase is the one ENDING, matching .NET";
+}
+
+TEST(BarrierPostPhaseReadabilityTests, Fix1957_ThePhaseAdvancesAfterTheActionNotBefore) {
+    // The ordering half, asserted across three phases so an off-by-one cannot pass by accident.
+    std::atomic<bool> done{false};
+    std::vector<long long> insideAction;
+    std::vector<long long> afterSignal;
+    std::mutex recordMutex;
+
+    std::thread worker([&] {
+        System::Threading::Barrier barrier(1, [&](System::Threading::Barrier& b) {
+            std::lock_guard<std::mutex> g(recordMutex);
+            insideAction.push_back(b.getCurrentPhaseNumberProperty());
+        });
+        for (int i = 0; i < 3; ++i) {
+            barrier.SignalAndWait();
+            std::lock_guard<std::mutex> g(recordMutex);
+            afterSignal.push_back(barrier.getCurrentPhaseNumberProperty());
+        }
+        done.store(true);
+    });
+    worker.detach();
+
+    const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(4);
+    while (!done.load() && std::chrono::steady_clock::now() < deadline)
+        std::this_thread::sleep_for(std::chrono::milliseconds(5));
+    ASSERT_TRUE(done.load()) << "the barrier deadlocked";
+
+    std::lock_guard<std::mutex> g(recordMutex);
+    ASSERT_EQ(insideAction.size(), 3u);
+    ASSERT_EQ(afterSignal.size(), 3u);
+    EXPECT_EQ(insideAction, (std::vector<long long>{0, 1, 2})) << "the phase that is ending";
+    EXPECT_EQ(afterSignal,  (std::vector<long long>{1, 2, 3})) << "the phase that has begun";
+}
+
+TEST(BarrierPostPhaseReadabilityTests, Fix1957_ThePhaseStillAdvancesWhenTheActionThrows) {
+    // .NET increments in the `finally`, so a throwing action still advances the phase. Easy to
+    // break by moving the increment into the success path only.
+    System::Threading::Barrier barrier(1, [](System::Threading::Barrier&) {
+        throw System::InvalidOperationException("boom");
+    });
+    EXPECT_EQ(barrier.getCurrentPhaseNumberProperty(), 0);
+    EXPECT_THROW(barrier.SignalAndWait(), System::Threading::BarrierPostPhaseException);
+    EXPECT_EQ(barrier.getCurrentPhaseNumberProperty(), 1)
+        << "a throwing post-phase action must still advance the phase";
+}
+
+TEST(BarrierPostPhaseReadabilityTests, Fix1957_ThePhaseIsUnchangedForOutsideObservers) {
+    // Nothing a caller outside the action can see moved: the increment is still inside the
+    // critical section, before notify_all and before the lock is released.
+    System::Threading::Barrier barrier(1);
+    EXPECT_EQ(barrier.getCurrentPhaseNumberProperty(), 0);
+    barrier.SignalAndWait();
+    EXPECT_EQ(barrier.getCurrentPhaseNumberProperty(), 1);
+    barrier.SignalAndWait();
+    EXPECT_EQ(barrier.getCurrentPhaseNumberProperty(), 2);
+}
+
+TEST(BarrierPostPhaseReadabilityTests, Fix1957_TheOtherMembersStillRefuseReentrancy) {
+    // The boundary this ticket must NOT move: the members that mutate still throw rather than
+    // deadlock, because each guards before taking the lock. Only the two READ-ONLY properties are
+    // callable from the action.
+    std::atomic<bool> done{false};
+    std::atomic<int> threwCount{0};
+    std::atomic<long long> phase{-1};
+    std::atomic<int> participants{-1};
+
+    std::thread worker([&] {
+        System::Threading::Barrier barrier(1, [&](System::Threading::Barrier& b) {
+            phase.store(b.getCurrentPhaseNumberProperty());
+            participants.store(static_cast<int>(b.getParticipantCountProperty()));
+            for (auto call : std::vector<std::function<void()>>{
+                     [&] { (void)b.AddParticipant(); },
+                     [&] { b.RemoveParticipant(); },
+                     [&] { b.SignalAndWait(); },
+                     [&] { b.Dispose(); }}) {
+                try { call(); } catch (const System::InvalidOperationException&) { ++threwCount; }
+                catch (...) {}
+            }
+        });
+        barrier.SignalAndWait();
+        done.store(true);
+    });
+    worker.detach();
+
+    const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(4);
+    while (!done.load() && std::chrono::steady_clock::now() < deadline)
+        std::this_thread::sleep_for(std::chrono::milliseconds(5));
+
+    ASSERT_TRUE(done.load()) << "a mutating member deadlocked instead of throwing";
+    EXPECT_EQ(threwCount.load(), 4) << "all four mutating members must refuse reentrancy";
+    EXPECT_EQ(phase.load(), 0);
+    EXPECT_EQ(participants.load(), 1);
+}