core: CallbackScope::reset() races every other member, under a doc stating all members are concurrency-safe

[Yaraslaut]

Summary

CallbackScope documents every member as safe to call concurrently. reset() writes the _state member that four other members read, with no synchronisation on the pointer object itself.

Verification status

Inferred from reading the code, not reproduced. No test exercises it. Revision: origin/master adfe8e5f plus this branch's doc-only commits.

The claim

include/morph/core/callback_scope.hpp:194-198:

"@Par Thread safety All members are safe to call concurrently from any thread. requestStop() and stopRequested() are lock-free atomic operations; reset() publishes a fresh generation and retires the previous one…"

docs/spec/core/callback_scope.md:192 makes it normative: "Every member is safe to call from any thread, concurrently with any other."

What is actually true

_state is a plain shared_ptr, not an atomic one:

270:    std::shared_ptr<detail::CallbackScopeState> _state;

reset() writes it (:240, _state = std::move(fresh);). Concurrently:

• token() (:251) reads it — CallbackToken{_state}
• guard() (:266) reads it via token()
• requestStop() (:225-226) reads it twice
• stopRequested() (:246) reads it twice

Concurrent read/write of the same shared_ptr object is a data race. The control block's refcount atomicity protects the pointee, not the pointer object — and the spec states exactly that wrong reason at :194-196 ("whose reference counting is itself atomic").

CallbackToken's own claim (:54-57) is correct: a token holds its own weak_ptr copy. The defect is scope-member vs. scope-member only.

Test coverage

None. tests/test_callback_scope.cpp:561's scope.reset() is std::unique_ptr::reset(), not this method; the two [thread] cases (:500, :538) race requestStop() and destruction, never reset().

Fix shapes

Either std::atomic<std::shared_ptr<detail::CallbackScopeState>> (C++20, available here), or narrow the documented contract to "reset() is not safe against concurrent calls to other members" — in the header and the spec.

Adjacent, minor

_state != nullptr at :225 and :246 are unreachable branches: the only constructor make_shareds it, the class is non-copyable and non-movable (:214-217), and reset() always assigns a freshly-make_shared'd value. In a repo that now runs a mutation gate (morph#408) these are two permanently-surviving mutants and two uncoverable branches.

What would change the verdict

• Close it if reset() is documented and enforced as owner-thread-only — that is a legitimate resolution, but it needs the doc change, since the current text promises the opposite.
• Raise it with a TSan test racing reset() against token() on the same scope.

[Yaraslaut]

Triage: triage: valid, bug, area: core

Premise re-checked by reading every access. _state is a plain std::shared_ptr (callback_scope.hpp:270), not std::atomic<std::shared_ptr<...>>. reset() writes it (:240); token() (:251), guard() (:266), requestStop() (:225-226) and stopRequested() (:246) read it. Concurrent read/write of the same shared_ptr object is a race — the control block's atomic refcount protects the pointee, not the pointer.

Kept valid rather than rescope, deliberately. My first instinct was that a doc/code disagreement belongs in rescope per invariant 5. It does not apply here: invariant 5 is about not silently picking a side when spec and code disagree, and this issue already presents both resolutions (make _state atomic, or narrow the documented contract) without choosing. The framing is right; only the decision is open. That is a well-framed issue, not a mis-framed one.

Invariant 2 is worth noting in passing and supports the current design: CallbackToken holding its own weak_ptr copy — a member, not a base — is exactly what makes the token side genuinely safe (:54-57 is correct as written). The defect is confined to scope-member vs. scope-member.

Invariant 7 governs the fix. tests/test_callback_scope.cpp has two [thread] cases (:500, :538) and neither races reset(); the scope.reset() at :561 is std::unique_ptr::reset(), not this method. A TSan test must race reset() against token() on the same scope, or it proves nothing either way.

Secondary, recorded rather than folded in: the _state != nullptr guards at :225/:246 are unreachable — the sole constructor make_shareds, the class is non-copyable and non-movable, and reset() always assigns a fresh value. Now that the mutation gate is live (morph#408), those are two permanently-surviving mutants.