feat(threading): ThreadLocal's trackAllValues means something, and Values exists (#1958, SR-AUD-220)

Rule-14 sweep. trackAllValues was accepted and never read, and the type had
no Values property at all -- two halves of one finding, inseparable because
the flag is only observable through the property it gates. A caller who asked
for tracking got a silent no-op with no way to notice.

Values is transcribed from ThreadLocal.cs:421-434, message included, and the
tracking check precedes the disposed check, so a disposed untracked instance
reports InvalidOperationException.

The lifetime question decided the design and rules out the cheap answer:
GetValuesAsList walks the ThreadLocal's OWN LinkedSlot list, so a value
survives its thread exiting. The registry therefore holds strong references
and per-thread storage moved unique_ptr -> shared_ptr; a weak_ptr registry
would silently drop a dead thread's value, which .NET does not do.
Co-ownership rather than copying is also what makes an update reflected
rather than duplicated.

sizeof(ThreadLocal<int>) 56 -> 128; consumers must rebuild.

Six mutations, all caught -- two only after the tests meant to catch them
were found vacuous, and both are recorded:

  * the check-order mutation CANNOT be asserted with EXPECT_THROW, because
    ObjectDisposedException derives from InvalidOperationException (the #2152
    trap), so the derived type must be caught first;
  * Dispose's registry release is unreachable through the public surface --
    after Dispose, Values throws either way -- so its only observable is WHEN
    values are destroyed, tested by letting the owning thread exit and
    counting destructor calls.

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

#1958 stays open for SR-AUD-209 (vtable/base-class change), SR-AUD-194
(signature change, landable under SA-10 but separate work) and SR-AUD-196.

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

Author: robertvokac

CLAUDE.md

@@ -0,0 +1,128 @@
+<!-- SPDX-License-Identifier: MIT -->
+<!-- Copyright (c) Robert Vokac and contributors -->
+
+# Migration — `ThreadLocal`'s `trackAllValues` now means something, and `Values` exists (ticket #1958, SR-AUD-220)
+
+*2026-08-19.* `System::Threading::ThreadLocal<T>` gains `getValuesProperty()`, and the
+`trackAllValues` constructor flag — previously accepted and **never read** — now controls it.
+
+**`sizeof(ThreadLocal<int>)` grows 56 → 128, so consumers must be recompiled.** Landed under
+**SA-5** (the behaviour is derived; the property is additive) with **SA-3**'s layout condition
+discharged.
+
+---
+
+## 1. What was wrong
+
+Two halves of one finding, and they are inseparable: the flag was **accepted and never read**, and
+the type exposed **no `Values` property at all**. A caller who asked for tracking got a silent
+no-op, and had no way to notice — the flag is only observable through the property it gates.
+
+## 2. What .NET does
+
+```csharp
+public IList<T> Values
+{
+    get
+    {
+        if (!_trackAllValues)
+        {
+            throw new InvalidOperationException(SR.ThreadLocal_ValuesNotAvailable);
+        }
+
+        List<T>? list = GetValuesAsList();          // returns null if disposed
+        ObjectDisposedException.ThrowIf(list is null, this);
+        return list;
+    }
+}                                                    // ThreadLocal.cs:421-434
+```
+
+The message is transcribed verbatim from `Strings.resx`.
+
+**The tracking check comes first, and that is observable**: a disposed instance built *without*
+tracking reports `InvalidOperationException`, not `ObjectDisposedException`. The order is .NET's
+and a test pins both directions.
+
+## 3. Values outlive their threads — and that decided the design
+
+`GetValuesAsList` walks the `ThreadLocal`'s **own** linked list of `LinkedSlot`s
+(`ThreadLocal.cs:437-456, 584-598`), so a value survives the thread that created it and is
+released when the `ThreadLocal` is disposed.
+
+That rules out the plausible cheap design. The registry holds **strong** references, not
+`weak_ptr` — a weak registry would silently drop a dead thread's value, which .NET does not do.
+The per-thread storage therefore moved from `unique_ptr<T>` to `shared_ptr<T>`, so one value is
+co-owned by the owning thread's map and the instance-wide registry.
+
+**Co-ownership, not copying**, is also why an update is reflected rather than duplicated: the
+setter writes through the shared object, so `Values` sees the new value without a second entry. A
+registry that stored a copy at creation time would fail both of those — and that is mutation M5.
+
+## 4. What changes
+
+| | Was | Is |
+|---|---|---|
+| `getValuesProperty()` | **absent** | returns `std::vector<T>`, a snapshot |
+| …without `trackAllValues` | — | `InvalidOperationException` |
+| …when disposed **and** tracking | — | `ObjectDisposedException` |
+| …when disposed **and not** tracking | — | `InvalidOperationException` — tracking is checked first |
+| `trackAllValues` | accepted, ignored | controls the above |
+| per-thread storage | `unique_ptr<T>` | `shared_ptr<T>` — internal, no API effect |
+| `Dispose()` | cleared the thread's slot | also releases the registry, as .NET unlinks its slots |
+| `sizeof(ThreadLocal<int>)` | **56** | **128** |
+
+An instance built **without** tracking pays a mutex it never locks and nothing else — the
+registry is only populated when the flag is set.
+
+`std::vector<T>` by value is the return shape because .NET's `IList<T>` has no counterpart here,
+and `GetValuesAsList` builds a fresh list on every call anyway; mutating the result cannot affect
+the instance, which is true of .NET's copy too.
+
+## 5. Evidence
+
+Six mutations, **all caught** — but two only after the tests that were supposed to catch them
+were found to be vacuous, and both defects are worth recording.
+
+| Mutation | Caught by |
+|---|---|
+| M1 — the tracking check is removed | `Fix1958_ValuesThrowsWhenNotTracking` |
+| M2 — the check order is inverted | `Decl1958_TheTrackingCheckPrecedesTheDisposedCheck` — **after repair**, below |
+| M3 — the factory path is not tracked | `Fix1958_TheFactoryPathIsTrackedToo` |
+| M4 — the setter path is not tracked | four cases |
+| M5 — the registry stores a copy, not the pointer | `Fix1958_AnUpdatedValueIsReflectedNotDuplicated` |
+| M6 — `Dispose` does not release the registry | `Fix1958_DisposeReleasesTheTrackedValues` — **after repair**, below |
+
+**M2 could not be asserted with `EXPECT_THROW` at all.** `ObjectDisposedException` **derives
+from** `InvalidOperationException`, here as in .NET, so `EXPECT_THROW(…, InvalidOperationException)`
+passes whichever check fired and the inverted order went uncaught. The test now catches the
+derived type first and requires the base one — the same trap #2152 recorded for the compression
+streams.
+
+**M6's release is unreachable through the public surface.** After `Dispose`, `Values` throws
+whether or not the registry was cleared, so asserting that it throws proves nothing. The only
+observable is *when* the values are destroyed — so the test creates the value on a worker thread
+which then **exits**, leaving the registry as the sole owner, and counts destructor calls across
+`Dispose`.
+
+The factory and setter paths are tested separately because they are **two different creation
+sites** — tracking one and not the other is the easy half-repair.
+
+Gate: **17,490 run, 17,490 passed, 0 failed, 0 skipped** across 38 executables — `+8` on 17,482,
+exactly the eight new cases (`SharpRuntimeTests_Threading` 506 → 514). No other executable moved.
+All 506 pre-existing cases passed unchanged before the new ones were added. Module graph
+unchanged at 41/93.
+
+## 6. Downstream, measured
+
+`ThreadLocal` 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.
+
+## 7. Scope
+
+This is one of #1958's eight findings. SR-AUD-193 (`ManagedThreadId` uniqueness) landed earlier
+today; SR-AUD-189 and SR-AUD-214 landed as #1971; SR-AUD-215 was excluded there with a measured
+reason. **SR-AUD-209** (making the two events derive from `WaitHandle` — a vtable and base-class
+change SA-3 excludes), **SR-AUD-194** (`Thread::Start(void*)` discards its parameter — a public
+signature change) and **SR-AUD-196** (`ThreadStartException` publishes constructors .NET makes
+internal) remain, and #1958 stays open for them.

docs/Migration-ThreadLocalTrackAllValues.md

@@ -6,6 +6,8 @@
 #include <cstdint>
 #include <functional>
 #include <memory>
+#include <mutex>
+#include <vector>
 #include <unordered_map>
 #include <unordered_set>
 #include "System/ArgumentNullException.hpp"
@@ -41,8 +43,15 @@ namespace System::Threading {
         }
         std::uint64_t id_ = nextId().fetch_add(1, std::memory_order_relaxed);
 
-        static std::unordered_map<std::uint64_t, std::unique_ptr<T>>& storageMap() {
-            static thread_local std::unordered_map<std::uint64_t, std::unique_ptr<T>> map;
+        // Ticket #1958 / SR-AUD-220. This was unique_ptr<T>. A tracked value must be reachable
+        // from BOTH the owning thread's map and the instance-wide registry, and it must OUTLIVE
+        // the owning thread -- .NET's LinkedSlot hangs off the ThreadLocal's own linked list and
+        // GetValuesAsList walks that list (ThreadLocal.cs:437-456, 584-598), so a value survives
+        // its thread exiting and is released when the ThreadLocal is. shared_ptr is the direct
+        // counterpart; a weak_ptr registry would silently drop dead threads' values, which .NET
+        // does not do.
+        static std::unordered_map<std::uint64_t, std::shared_ptr<T>>& storageMap() {
+            static thread_local std::unordered_map<std::uint64_t, std::shared_ptr<T>> map;
             return map;
         }
 
@@ -60,6 +69,20 @@ namespace System::Threading {
 
         std::function<T()> factory_;
         bool trackAllValues_ = false;
+        // Ticket #1958 / SR-AUD-220. trackAllValues_ was ACCEPTED AND NEVER READ, and the type
+        // exposed no Values property at all -- so a caller who asked for tracking got a silent
+        // no-op and had no way to notice. These two members are what makes the flag mean
+        // something. They are populated ONLY when trackAllValues_ is set, so an untracking
+        // instance pays a mutex it never locks and nothing else.
+        mutable std::mutex          trackedMutex_;
+        std::vector<std::shared_ptr<T>> trackedValues_;
+
+        /// Registers a newly created value with the instance-wide registry, when tracking.
+        void trackIfRequested(const std::shared_ptr<T>& value) {
+            if (!trackAllValues_) return;
+            std::lock_guard<std::mutex> lk(trackedMutex_);
+            trackedValues_.push_back(value);
+        }
         // Ticket #1955 / cause T-A of docs/ThreadingNamespaceReviewPlan.md. This was an
         // ordinary `bool`, written by Dispose() and read by the guard below with no
         // synchronisation between them. Mixing synchronised and unsynchronised access to the
@@ -148,14 +171,15 @@ namespace System::Threading {
                 if (!active.insert(id_).second)
                     throw System::InvalidOperationException(
                         "ValueFactory attempted to access the Value property of this instance.");
-                std::unique_ptr<T> value;
+                std::shared_ptr<T> value;
                 try {
-                    value = std::make_unique<T>(factory_ ? factory_() : T{});
+                    value = std::make_shared<T>(factory_ ? factory_() : T{});
                 } catch (...) {
                     active.erase(id_);
                     throw;
                 }
                 active.erase(id_);
+                trackIfRequested(value);
                 it = map.emplace(id_, std::move(value)).first;
             }
             return *it->second;
@@ -169,17 +193,67 @@ namespace System::Threading {
             ThrowIfDisposed();
             auto& map = storageMap();
             auto it = map.find(id_);
-            if (it == map.end()) map.emplace(id_, std::make_unique<T>(v));
-            else *it->second = v;
+            if (it == map.end()) {
+                auto value = std::make_shared<T>(v);
+                trackIfRequested(value);
+                map.emplace(id_, std::move(value));
+            } else {
+                // An existing value is UPDATED IN PLACE, so the registry -- which co-owns the
+                // same object -- sees the new value without a second entry. That is why the
+                // registry holds the pointer rather than a copy.
+                *it->second = v;
+            }
         }
 
         /** Returns the value for the current thread (alias for getValueProperty). */
         [[nodiscard]] T& Value() { return getValueProperty(); }
 
+        /**
+         * @brief Returns the values held for every thread that has one, as a snapshot.
+         * @throws System::InvalidOperationException if this instance was not constructed with
+         *         `trackAllValues = true`.
+         * @throws System::ObjectDisposedException if this instance has been disposed.
+         *
+         * Ticket #1958 / SR-AUD-220. Transcribed from `ThreadLocal<T>.Values`
+         * (`ThreadLocal.cs:421-434`):
+         * @code
+         * if (!_trackAllValues) throw new InvalidOperationException(SR.ThreadLocal_ValuesNotAvailable);
+         * List<T>? list = GetValuesAsList();          // returns null if disposed
+         * ObjectDisposedException.ThrowIf(list is null, this);
+         * @endcode
+         *
+         * @note **The tracking check comes FIRST, before the disposed check, and that is
+         * observable**: a disposed instance built WITHOUT tracking reports
+         * `InvalidOperationException`, not `ObjectDisposedException`. The order is .NET's and a
+         * test pins it.
+         *
+         * @note Returns `std::vector<T>` by value -- a snapshot, as .NET's `GetValuesAsList`
+         * builds a fresh `List<T>` on every call. .NET's declared return type is `IList<T>`,
+         * which this port has no counterpart for; the vector is the closest faithful shape and
+         * mutating it cannot affect the instance, which is also true of .NET's copy.
+         */
+        [[nodiscard]] std::vector<T> getValuesProperty() const {
+            if (!trackAllValues_) {
+                throw System::InvalidOperationException(
+                    "The ThreadLocal object is not tracking values. To use the Values property, "
+                    "use a ThreadLocal constructor that accepts the trackAllValues parameter and "
+                    "set the parameter to true.");
+            }
+            ThrowIfDisposed();
+            std::lock_guard<std::mutex> lk(trackedMutex_);
+            std::vector<T> snapshot;
+            snapshot.reserve(trackedValues_.size());
+            for (const auto& v : trackedValues_) snapshot.push_back(*v);
+            return snapshot;
+        }
+
         /** Releases resources for the current thread's value. */
         void Dispose() override {
             disposed_.store(true, std::memory_order_release);
             storageMap().erase(id_);
+            // .NET's Dispose unlinks every LinkedSlot, releasing the tracked values with it.
+            std::lock_guard<std::mutex> lk(trackedMutex_);
+            trackedValues_.clear();
         }
     };