bridge: registerHandlerImpl reads binding->contextKey under neither lock, violating the invariant _attachMtx's own comment states

[Yaraslaut]

Summary

Bridge's _attachMtx member comment states a hard invariant:

HandlerBinding::primary/contextKey are therefore mutated (and must be read) only under _attachMtx — never under _mtx alone.

registerHandlerImpl reads binding->contextKey twice while holding neither _attachMtx nor _mtx.

Verification status

Verified by reading; not reproduced. I enumerated every access and every lock acquisition in the function rather than sampling. Revision: branch framework-comment-audit (= origin/master adfe8e5f plus doc-only commits); line numbers as of that revision.

The two reads:

• bridge.hpp:1760 — binding->contextKey passed as the third argument to backend->registerModelAsync(...).
• bridge.hpp:1814 — backend->registerModelWithContext(binding->typeId, binding->modelFactory, binding->contextKey), the synchronous fallback.

Every lock acquisition in the enclosing range, by line:

1742:  std::scoped_lock const lock{_mtx};                  <- block ends before 1760
1753:  std::scoped_lock const lock{binding->registrationMtx};  <- separate scope, not _attachMtx
1774:  std::shared_lock const gate{lifetime->mtx};         <- inside the callback, not this frame
1776:  std::scoped_lock const lock{_mtx};                  <- inside the callback

Neither read is covered. By contrast every other attach/assign site is: :507-509, :692-694, :716, :750, :826, :888, :927-931, :943-945, :955, :1298-1307, :1906-1913 all hold _attachMtx.

Why it is reachable rather than theoretical

The registerHandler() overload that takes a pre-built binding hands the caller the shared_ptr before registerHandlerImpl is ever called — registerHandlerImpl's own fallback comment says so, in the course of explaining a different race it does defend against:

"a whenBound() call from another thread that already holds this same binding (the pre-built-binding registerHandler() overload hands the caller the shared_ptr before this function is even called) could have raced in during the window above"

So a second thread can hold the binding and call attachHandler/assignHandlerPrimary on it — both of which write contextKey under _attachMtx — concurrently with these unlocked reads. std::string is not atomic; this is a data race by the invariant's own reasoning ("publishing them without it would be a data race, not just a stale read").

Which is wrong: the code or the invariant?

Both readings are defensible and the fix differs, which is why this is filed rather than patched:

1. The invariant is right and the code is wrong — take _attachMtx around both reads (or copy the string out under it first, as attachHandlerAsync does with primaryCopy).
2. The invariant is overstated — if initial registration is defined as happening-before any other thread can reach the binding, then these reads are safe and the member comment needs the carve-out. But the pre-built-binding overload appears to defeat that argument.

I did not resolve which, because it is a design question about the pre-built-binding overload's contract, not a mechanical fix.

What would change the verdict

• Close it by showing the pre-built-binding overload cannot deliver a binding to another thread before registerHandlerImpl returns — then correct the _attachMtx comment to say so.
• Raise it with a TSan test: construct a binding, hand it to a second thread that calls attachHandler on it, and call registerHandler(binding) on the first.

Context

Found while correcting a stale comment at bridge.hpp:615 (PR #503), which claimed "five other sites read under _attachMtx". The count was wrong; my first correction said "every other site", which is also wrong because of exactly these two reads. The comment now names them as the exception and points here.

[Yaraslaut]

Triage: triage: rescope, area: core — and removing bug

Correcting my own filing, on the same rule as #500: this is a documented-invariant/code disagreement, and the repository's guidance puts that in rescope, not bug. The worked precedent is #159 — spec and code disagreeing about x-decimalPlaces, filed with resolutions rather than a unilateral fix.

Premise re-checked, and it strengthens. The two unlocked reads are at bridge.hpp:1760 and :1814; every lock acquisition in the enclosing frame is :1742 (_mtx, block closes before 1760) and :1753 (binding->registrationMtx). Neither read is covered, while every other attach/assign site is.

More than that, I checked the reachability argument rather than asserting it. The pre-built-binding overload is bridge.hpp:452, and its own doc says it exists "when binding->contextKey needs to be set before registration" — so the caller constructs the binding, sets contextKey, and holds the shared_ptr across the call. BridgeHandler's constructor uses it (:2063), as do several tests. The binding is caller-visible for the whole of registerHandlerImpl.

Why rescope and not valid. Invariant 5: "Do not silently pick a side. Which one is wrong is a decision with consequences, and it belongs to whoever owns the contract." Two coherent readings survive verification, and they have different fixes:

1. The invariant holds and the code breaks it — take _attachMtx around both reads, or copy the string out under it first, as attachHandlerAsync already does with primaryCopy.
2. The invariant is overstated — if initial registration is defined as happening-before any other thread can touch the binding, the reads are safe and the _attachMtx member comment needs an explicit carve-out. But that definition has to survive the pre-built-binding overload, which hands the binding out first by design.

Reading 2 also has a cost the issue should say out loud: it weakens a currently-absolute rule ("must be read only under _attachMtx") into one with an exception, and absolute rules are what make the other eleven sites checkable at a glance.

Invariant 7 for whichever is chosen: a regression test must actually race — construct a binding, hand it to a second thread calling attachHandler on it, and call registerHandler(binding) on the first, under TSan. A single-threaded test would be green under both readings and would measure nothing.

What would make this invalid: showing the pre-built-binding overload cannot deliver a binding to another thread before registerHandlerImpl returns. I could not construct that argument — the overload's documented purpose is the opposite.