net: put SocketBackend on the structural registration surface natively

[Yaraslaut]

Closes #569.

Update: driven to CI-green. Since the section below was written, this branch was rebased onto current master and two things were fixed to get here: a stale coverage-allowlist line hint in socket_backend.hpp that this PR's own insertions had shifted, and a genuine ThreadSanitizer data race caught on CI — five TEST_CASEs in tests/net/test_socket_backend.cpp declared their SocketBackend before their MainThreadExecutor, so C++'s reverse local-destruction order tore the executor's condition variable down while SocketBackend's still-running I/O thread could still call post() on it. ~SocketBackend() joins the I/O thread before returning, so reordering the declarations (executor before backend, so it outlives it) closes the window; details in the fix commit. Also fixed: #589, a pre-existing, unrelated master clang-tsan timeout in equation()'s stack-safety test (fixed separately and merged, since it affected every PR based on master, not just this one). All 51 checks are green.

Where I got to

Read AGENTS.md, docs/spec/core/backend.md (the whole IBackend / structural-surface / morph::net / design-decision material), docs/spec/concurrency_and_lifetimes.md's destruction-ordering rules, include/morph/core/backend.hpp (BindRequest/PromoteRequest/bindModel/promoteModel/SynchronousBackendAdapter), include/morph/core/completion.hpp, include/morph/core/detail/reply_router.hpp, include/morph/core/remote.hpp's reply construction, include/morph/core/bridge.hpp's installReconnectHandler, and all of include/morph/net/socket_backend.hpp plus tests/net/test_socket_backend.cpp.

Decided first, then built. Implementation, tests and the spec update are done; everything below marked measured was run.

Measured locally (GCC 16.2.1, Debug, -DMORPH_BUILD_NET=ON):

• morph_net_tests: 1094 assertions in 189 test cases, all passing.
• The mutation check on the decisive test (below), with its real output.
• scripts/check_spec_sync.sh fed the real diff: fails on the code commit alone, passes with the doc commit — so the gate is live on this change, not vacuous.
• scripts/check_spec_citations.sh, scripts/check_catch_test_names.sh: clean.
• clang-tidy-diff.py over origin/master...HEAD with clang-tidy 22, per the CI recipe: clean (one bugprone-empty-catch found and fixed in the last commit).

Not run (this is the CI risk on the draft): the non-net test suites, the Qt legs, tests/net_qt_interop, the sanitizer legs (ASan/UBSan/TSan — relevant, since #576 now instruments the net suites), the Doxygen WARN_AS_ERROR docs build, and a clang-20 build (local clang is 22).

The decision: (2), native — and the evidence that decided it

SocketBackend now overrides bindModel/promoteModel itself. Three findings decided it, in descending order of weight:

1. The adapter's one reconnect-related property does not apply to this backend. This is the load-bearing one, and it is a fact about the code rather than a preference. SynchronousBackendAdapter forwards setReconnectHandler straight to the wrapped backend (backend.hpp:1041), and Bridge::installReconnectHandler (bridge.hpp:1910) re-registers through the blocking registerModelShared/registerModelWithContext, which the adapter also forwards unchanged. So the spec's bullet "a control call issued from a reconnect handler runs on the strand" holds only for a handler that issues it through bindModel, and no handler in the tree does. A wrapped SocketBackend would run its reconnect control calls exactly where it runs them today. Choosing the wrapper would have meant adopting it for a benefit that is, here, vacuous.

2. A wrapper would keep every bind inside sendSync's one-call token. sendSync admits exactly one synchronous control call across the whole backend and throws "a synchronous call is already in flight (reentrant use)" on a second. The adapter's strand serialises binds against each other, but listInstances and the legacy registerModel are not on that strand — and SocketBackend is explicitly documented as safe to drive from several threads at once, unlike QtWebSocketBackend. The wrapper would therefore hide the restriction for binds while leaving the cross-path collision, and would park a thread per bind for a round trip. The native path takes no token at all.

3. The machinery already exists, and the protocol already supports it. The I/O thread demultiplexes replies by callId for execute, and RemoteServer echoes callId on every control reply — makeOk(env.callId, {}, mid.v) at remote.hpp:751 (registerShared), :846 (attach), :1144 (register), :1175 (assign). So a native control call is the same shape as an execute: a second PendingCallTable sharing _pending's counter, and a branch in the reply router. No protocol change, no server change, no new thread, and interop with QtWebSocketServer is untouched because both sides go through the same RemoteServer::handle.

Against native: it is more code in a transport than a wrapper would have been. That is real, and it is the whole of the case for (1) once finding 1 removes the other one. It did not outweigh three properties that are all about correctness rather than effort.

Every legacy verb is untouched and still uses sendSync with callId == 0, so callers #570/#571 have yet to migrate see no behaviour change and no wire change.

The reconnect-handler deadlock hazard — the explicit answer

The hazard: a control call parks on _syncCv awaiting a reply only the I/O thread's read loop can deliver, so running one on that thread blocks the thread that would satisfy it. SocketBackend prevents it today by invoking reconnect handlers on a dedicated _handlerThread (onConnected()).

Three parts, because one word would be wrong:

• Through the adapter it would be untouched — not relocated, untouched. See finding 1: neither setReconnectHandler nor the blocking verbs would have gone near the strand.
• Natively, a bind cannot have the hazard at all. bindModel never enters sendSync, never waits on _syncCv, and returns before its reply exists, so there is no wait for any thread to block — including the I/O thread itself. That is structural: it follows from the signature returning a Completion rather than a ModelId, and it holds whichever thread issues the call.
• The hazard is nevertheless still in the backend, and _handlerThread is still load-bearing. The blocking verbs still park on _syncCv and Bridge's reconnect handler still calls them. A hazard-free route now exists; the hazardous one has not been removed or moved. It becomes unreachable from the reconnect path only when ladder: move the example GUIs and the WASM spike onto the structural registration surface #570 puts installReconnectHandler on bindModel — and only then is dropping the handler thread a question worth asking. I did not drop it, and nothing here should be read as saying it can be.

None of this touches morph#486: no lock is added and nothing here knows about a caller's teardown. #567 moved the choice of delivery thread from the implementor to the caller, and this change does exactly that much for this backend and no more.

The test, and the mutation proving it is not vacuous

The claim above rests on "a native control call never parks", so the test pins that, not just the functional result. SocketBackend: a bind settles while the synchronous control channel is still parked: a legacy registerModel is parked on _syncCv against a FakeWsServer that deliberately withholds its callId == 0 reply; with that token held, a bindModel is issued, reaches the wire with a non-zero callId, is answered alone, and settles — while the synchronous call is still parked (CHECK_FALSE(syncReturned.load())).

Mutation — rename SocketBackend::bindModel so the blocking default is used instead, rebuild, run:

tests/net/test_socket_backend.cpp:1923: FAILED:
  REQUIRE( accepted )
with message:
  the bind was rejected instead of accepted: register failed: sendSync: a
  synchronous call is already in flight (reentrant use)

A second test, several binds are in flight at once and are matched by callId with replies out of order, fails by hanging under the same mutation (the first bind would block for a reply sent only after all four are issued) — the same failure convention the existing a reconnect handler that re-registers does not deadlock the transport test uses, which ctest's per-test TIMEOUT 120 turns into a failure.

Also added: the BindRequest shape table over a real SocketServer plus promoteModel and its two local guards; a delivery-thread test (the continuation does not run until the caller's MainThreadExecutor is pumped, and then runs on the pumping thread); disconnect rejection both before and during flight; the server's err message surfacing with the same "<verb> failed: ..." wording the blocking verbs use; and a reconnect handler re-binding through the new surface without waiting — that last one is explicitly commented as a parity test, not evidence about the hazard, since the handler thread makes it pass either way.

Gaps in #567's surface that #568 / #570 / #571 should know about

None that needs a change to core/backend.hpp — the surface was sufficient as-is, and I did not touch that header. Three things worth carrying forward:

1. The adapter's reconnect bullet needs its proviso. As shipped it reads as an unconditional property; it holds only for handlers that issue control calls through bindModel. I corrected that bullet in the spec. This matters most to ladder: move the example GUIs and the WASM spike onto the structural registration surface #570: moving Bridge::installReconnectHandler onto bindModel is what actually makes the property true anywhere.
2. cancelPending is a second obligation the surface does not name. A backend overriding bindModel natively must also sweep its new pending state in cancelPending, or an in-flight bind hangs forever on a disconnect/teardown instead of rejecting. Nothing in IBackend's docs says so. qt: move QtWebSocketBackend onto the structural registration surface and delete the WASM special case #568 needs the same for QtWebSocketBackend.
3. promoteModel's "resolve, do not reject" no-op cases are a server-side property, not a client-side one. For this transport they hold because RemoteServer answers assign with ok in those cases; a backend whose peer answers err would have to translate. Worth a sentence in IBackend's docs if core: remove IBackend's four async twins and retire the prose threading contract #571 is editing there anyway.

Filed / to file

One finding, not folded in per AGENTS.md and not yet filed (the wind-down came first): SocketBackend drops contextKey on a private registration. It does not override registerModelWithContext, so the default forwards to registerModel and discards it — while wire::makeRegister accepts a contextKey and SimulatedRemoteBackend does override the verb to carry it across. The consequence is that a server-side action log has no entity key for instances registered privately over morph::net. Status: inferred from reading the code, not reproduced. I deliberately preserved the drop in bindModel so the native path stays bit-for-bit identical to the blocking one, with a comment saying why.

Two stale statements corrected in passing, because the argument above depends on them: the SocketBackend API table and setReconnectHandler's own doc comment both said the reconnect handler runs on the I/O thread, which is precisely what the dedicated thread exists to prevent.

Not done — ordered

1. Drive CI to green; rebase onto master first (Give Completion<T> a value-handling contract, and stop a benchmark racing on a dead frame #579 on completion.hpp and qt: move QtWebSocketBackend onto the structural registration surface and delete the WASM special case #568 on the Qt backend are both in flight; qt: move QtWebSocketBackend onto the structural registration surface and delete the WASM special case #568 edits the same spec file, so expect a conflict in docs/spec/core/backend.md).
2. Confirm the sanitizer legs, which Four gates that could not fail: UBSan modes, sanitizer instrumentation, spec-sync, and the forms key vocabulary #576 newly points at the net suites — especially TSan against the second PendingCallTable and the reply-router branch.
3. Confirm the Doxygen WARN_AS_ERROR build accepts the new public overloads' @param/@return blocks.
4. Confirm tests/net_qt_interop still passes (not built locally — no Qt in this environment).
5. File the contextKey issue above.
6. Flip to non-draft and change Refs #569 to Closes #569.

Traps

• A Catch2 test name containing a comma splits into two filters when you pass it on the command line. scripts/check_catch_test_names.sh passes anyway (it escapes), so nothing catches it — renamed.
• A failing REQUIRE with a live std::thread in scope turns a readable assertion failure into terminate() + SIGABRT. The decisive test now releases and joins the parked thread before the failing assertion, which is the difference between the mutation output quoted above and a core dump.
• scripts/check_spec_sync.sh run bare reports "the change touches no files" — it reads a path list on stdin. It has to be fed git diff --name-only <base> HEAD to mean anything, which is exactly the "a control that reports success while measuring nothing" shape AGENTS.md warns about.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DbGrZGkr2WqcAs2DJyMym6

[codecov]

Codecov Report

❌ Patch coverage is 81.03448% with 11 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
include/morph/net/socket_backend.hpp	81.03%	7 Missing and 4 partials ⚠️

📢 Thoughts on this report? Let us know!