From 15ccbec9ef76911d990a48890bb33bf51d47a27d Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sun, 20 Sep 2026 00:50:17 +0200 Subject: [PATCH 1/3] net: carry contextKey on SocketBackend's private registration (fixes #587) `SocketBackend` left `IBackend::registerModelWithContext` unoverridden, so its default dropped `contextKey`, and the native `bindModel` path added in #586 dropped it again by omission at the `wire::makeRegister` call site. The consequence is stronger than "a log missing its entity key". `RemoteServer::attachLogIfConfigured` returns *without consulting its `LogProvider` at all* when the envelope's `contextKey` is empty, so an instance registered privately over `morph::net` was not journalled -- no audit record -- while the same registration over `SimulatedRemoteBackend` was. It failed open. `backend.hpp`'s own doc comment on the default already states the rule: backends whose instances live behind a wire protocol override this to carry the key across. `SocketBackend` is such a backend and did not. Both edges now do: - `bindModel`'s private branch passes `request.contextKey` to `makeRegister`; - `registerModelWithContext` is overridden, mirroring `SimulatedRemoteBackend::registerModelWithContext`, and `registerModel` forwards to it with an empty key. `registerModelShared` and `attachModel` degrade to `registerModelWithContext` when `primary` is empty, so their private paths are fixed with it. The shared and attach shapes already carried the key and are untouched, as is `registerModel`, which has no key to send. The obsolete comment at the `bindModel` call site explaining the drop is removed. Verification: reproduced end-to-end over a real socket, not inferred. The new test in `tests/net/test_socket_backend.cpp` stands up a `SocketServer` over a `RemoteServer` with a `LogProvider` installed and asserts the provider was consulted with the key -- and that the log it returns records the executed action under that `entityKey`. With the fix reverted it fails on exactly those assertions (`{ } == { "SbEchoModel:acct-587" }`, `0 == 1` entries, and `{ } == { "SbEchoModel:acct-blocking" }`); registration itself succeeded both before and after, which is why nothing asserts on that. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GS5K2vqZtC4xbRiGJHT7jH --- CHANGELOG.md | 19 +++++ include/morph/net/socket_backend.hpp | 51 ++++++++----- tests/net/test_socket_backend.cpp | 110 +++++++++++++++++++++++++++ 3 files changed, 161 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index faf2d12e2..32c670980 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -182,6 +182,25 @@ API surface). ### Fixed +- **A model registered privately over `morph::net` was not journalled at all.** + `morph::net::SocketBackend` left `IBackend::registerModelWithContext` + unoverridden, so its default dropped the `contextKey`, and the native + `bindModel` path added in morph#586 dropped it again by omission at the + `wire::makeRegister` call site. Because `RemoteServer::attachLogIfConfigured` + returns *without consulting its `LogProvider`* when the envelope's + `contextKey` is empty, the effect was not a log entry missing its entity + key — no log was attached, so the instance produced no audit record at all, + while the same registration over `SimulatedRemoteBackend` produced one. It + failed open. `SocketBackend` now overrides `registerModelWithContext` and + passes `request.contextKey` to `makeRegister` on the `bindModel` private + branch, so both edges carry the key; the empty-`primary` degrades of + `registerModelShared`/`attachModel`, which route through + `registerModelWithContext`, are fixed with them. The shared and attach shapes + already carried it and are unchanged. Reproduced end-to-end over a real + socket in `tests/net/test_socket_backend.cpp`, which asserts the provider was + consulted with the key and that the attached log records the action under it. + See morph#587. + - **A locale-formatted entry could submit ten times what the user typed.** `morph::render::normalizeLocaleNumber` dropped every occurrence of the group separator unconditionally, with no check on placement, so a de-DE user typing diff --git a/include/morph/net/socket_backend.hpp b/include/morph/net/socket_backend.hpp index c8c1d8f25..c57661c68 100644 --- a/include/morph/net/socket_backend.hpp +++ b/include/morph/net/socket_backend.hpp @@ -183,25 +183,42 @@ class SocketBackend : public ::morph::backend::detail::IBackend { /// is outstanding throws immediately rather than queuing. The factory /// argument is ignored — model construction is delegated to the server. /// @param typeId String type-id of the model to register. + /// @param factory Ignored — the server constructs via its own registry. /// @return `ModelId` assigned by the server. /// @throws std::runtime_error if the server replies with an error, the /// socket is not connected, or a synchronous call is already in flight. ::morph::exec::detail::ModelId registerModel( const std::string& typeId, - std::function()> /*factory*/) override { - auto env = ::morph::wire::makeRegister(typeId); + std::function()> factory) override { + return registerModelWithContext(typeId, std::move(factory), {}); + } + + /// @brief Sends a `register` message carrying @p contextKey and blocks for the reply. + /// + /// `IBackend::registerModelWithContext`'s default drops @p contextKey, which + /// is right for `LocalBackend` — the caller's own factory closure already + /// captures the identity — but wrong for a backend whose instances live on + /// the far side of a wire protocol: the server constructs the holder itself, + /// so `contextKey` is the *only* channel by which the instance's identity + /// reaches it. `RemoteServer::attachLogIfConfigured` returns without + /// consulting its `LogProvider` at all when the envelope's `contextKey` is + /// empty, so dropping it here does not merely lose an entity key — it leaves + /// the instance unjournalled (morph#587). `SimulatedRemoteBackend` overrides + /// this for the same reason; the two must not disagree. + /// + /// Same synchronous-call constraint as `registerModel`. The factory argument + /// is ignored — model construction is delegated to the server. + /// @param typeId String type-id of the model to register. + /// @param contextKey Stable identity of the new instance; empty if none. + /// @return `ModelId` assigned by the server. + /// @throws std::runtime_error if the server replies with an error, the + /// socket is not connected, or a synchronous call is already in flight. + ::morph::exec::detail::ModelId registerModelWithContext( + const std::string& typeId, std::function()> /*factory*/, + std::string_view contextKey) override { + auto env = ::morph::wire::makeRegister(typeId, std::string{contextKey}); env.session = currentSession(); - std::string replyJson; - try { - replyJson = sendSync(::morph::wire::encode(env)); - } catch (const std::exception& exc) { - throw std::runtime_error(std::string{"register failed: "} + exc.what()); - } - auto reply = ::morph::wire::decode(replyJson); - if (reply.kind == "ok") { - return ::morph::exec::detail::ModelId{reply.modelId}; - } - throw std::runtime_error("register failed: " + reply.message); + return sendControlForId(env, "register"); } /// @brief Sends a shared (register-or-attach) `register` and blocks for the reply. @@ -310,12 +327,8 @@ class SocketBackend : public ::morph::backend::detail::IBackend { deregisterModel(request.current); } if (request.primary.empty()) { - // `contextKey` is dropped here because the blocking path drops it: - // `IBackend::registerModelWithContext`'s default forwards to - // `registerModel` and discards it, and this backend does not - // override it. Keeping the native path bit-for-bit identical - // matters more than changing that here; it is filed separately. - return sendControlAsync(::morph::wire::makeRegister(request.typeId), "register", std::nullopt, cbExec); + return sendControlAsync(::morph::wire::makeRegister(request.typeId, request.contextKey), "register", + std::nullopt, cbExec); } if (request.current.v != 0U) { return sendControlAsync( diff --git a/tests/net/test_socket_backend.cpp b/tests/net/test_socket_backend.cpp index aacd57497..c6c6046c2 100644 --- a/tests/net/test_socket_backend.cpp +++ b/tests/net/test_socket_backend.cpp @@ -16,12 +16,14 @@ #include #include #include +#include #include #include #include #include #include #include +#include #include #include #include @@ -2124,3 +2126,111 @@ TEST_CASE("SocketBackend: a reconnect handler can re-bind through the structural // The transport is unharmed: the synchronous channel was never involved. CHECK(fixture.backend->registerModel("SbEchoModel", nullptr).v != 0U); } + +// ── contextKey on a private registration (morph#587) ───────────────────────── +// +// `RemoteServer::attachLogIfConfigured` (core/remote.hpp) returns *without +// consulting its `LogProvider` at all* when the envelope's `contextKey` is +// empty, so dropping the key on the way out is not "a log missing its entity +// key" -- the instance is never journalled. Both of this backend's +// private-registration edges therefore have to put the key on the wire: the +// native `bindModel` path and the blocking `registerModelWithContext`. The +// shared/attach shapes always carried it. +// +// Mutation check (AGENTS.md, "would this check still pass if the feature did +// nothing"): every assertion below is on the provider having been consulted +// with the key, or on the resulting journal entry's `entityKey` -- never on the +// registration merely succeeding, which it did before the fix too. Measured: +// with `makeRegister(request.typeId)` restored at the `bindModel` call site the +// first section fails on `requestedFor`, and with +// `registerModelWithContext`'s override removed the second fails the same way. +TEST_CASE("SocketBackend: a private registration carries contextKey to the server's log provider", + "[net][socket_backend][registration-surface][action_log]") { + morph::exec::ThreadPoolExecutor serverPool{2}; + auto server = std::make_shared(serverPool); + + // The provider runs on the server's own strand, the assertions on this + // thread; the mutex is what makes that handoff a data race TSan will not + // flag rather than one it will. + std::mutex providerMtx; + std::vector requestedFor; + auto log = std::make_shared(); + server->setLogProvider([&](std::string_view modelType, std::string_view contextKey) { + std::scoped_lock const lock{providerMtx}; + requestedFor.emplace_back(std::string{modelType} + ":" + std::string{contextKey}); + return log; + }); + + morph::net::SocketServer wsServer{*server, 0}; + REQUIRE(wsServer.listen()); + + // Declared before `backend` so it outlives it -- see the ordering note on + // the first registration-surface test above. + morph::exec::MainThreadExecutor callerExec; + morph::net::SocketBackend backend{"ws://127.0.0.1:" + std::to_string(static_cast(wsServer.port()))}; + REQUIRE(backend.waitForConnected()); + + SECTION("through the native bindModel path, and the attached log journals under that key") { + morph::exec::detail::ModelId bound{}; + std::atomic done{false}; + backend + .bindModel(morph::backend::detail::BindRequest{.typeId = "SbEchoModel", + .factory = nullptr, + .contextKey = "acct-587", + .primary = {}, + .current = {}}, + callerExec) + .thenDetached([&](morph::exec::detail::ModelId mid) { + bound = mid; + done.store(true); + }); + REQUIRE(drainUntil(callerExec, done)); + REQUIRE(bound.v != 0U); + { + std::scoped_lock const lock{providerMtx}; + CHECK(requestedFor == std::vector{"SbEchoModel:acct-587"}); + } + + // …and the log the provider handed over is really attached to that + // instance: an action executed against it lands in the journal under + // the same key. Hand-built `ActionCall` for the reason the + // attachModel test above gives. + morph::backend::detail::ActionCall call{ + .modelTypeId = "SbEchoModel", + .actionTypeId = "SbEchoAction", + .serializeAction = [] { return std::string{R"({"value":7})"}; }, + .deserializeResult = + [](std::string_view body) { + return std::static_pointer_cast(std::make_shared(body)); + }, + .localOp = nullptr, + .session = {}, + }; + morph::exec::ThreadPoolExecutor cbPool{1}; + std::atomic settled{false}; + backend.execute(bound, std::move(call), &cbPool) + .then([&](const std::shared_ptr&) { settled.store(true); }) + .onError([&](const std::exception_ptr&) { settled.store(true); }); + spinUntil([&] { return settled.load(); }); + REQUIRE(settled.load()); + + auto entries = log->entries(); + REQUIRE(entries.size() == 1); + CHECK(entries[0].entityKey == "acct-587"); + CHECK(entries[0].actionType == "SbEchoAction"); + } + + SECTION("through the blocking registerModelWithContext path") { + auto const mid = backend.registerModelWithContext("SbEchoModel", nullptr, "acct-blocking"); + REQUIRE(mid.v != 0U); + std::scoped_lock const lock{providerMtx}; + CHECK(requestedFor == std::vector{"SbEchoModel:acct-blocking"}); + } + + SECTION("while plain registerModel still sends no key, so the provider is not consulted") { + auto const mid = backend.registerModel("SbEchoModel", nullptr); + REQUIRE(mid.v != 0U); + std::scoped_lock const lock{providerMtx}; + CHECK(requestedFor.empty()); + } +} From f89c2f77c4126d2b9f1961d80953c81aa9a3365a Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sun, 20 Sep 2026 01:07:29 +0200 Subject: [PATCH 2/3] coverage: refresh the socket_backend.hpp line hint the header edit shifted The `default:` disposition in `scripts/branch_partial_allowlist.json` pins a line number as a hint; the fix above moved that label from 813 to 826. The gate's own message says the disposition itself is still sound -- "The text still matches, so nothing is wrong with the disposition -- update the `line` hint" -- and the pinned `source` text (`default:`) is unchanged, so this is a hint refresh, not a new or widened suppression. No entry is added, removed or reworded. Verified: `sed -n '826p' include/morph/net/socket_backend.hpp` prints `default:`, and the file still parses as JSON. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GS5K2vqZtC4xbRiGJHT7jH --- scripts/branch_partial_allowlist.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/branch_partial_allowlist.json b/scripts/branch_partial_allowlist.json index bb8c9734f..fd448a51b 100644 --- a/scripts/branch_partial_allowlist.json +++ b/scripts/branch_partial_allowlist.json @@ -146,7 +146,7 @@ }, { "file": "include/morph/net/socket_backend.hpp", - "line": 813, + "line": 826, "source": "default:", "reason": "Unreachable except via the adjacent `Error` case it deliberately shares a body with (net audit, `socket_backend.hpp` extra finding #7). `detail::ExecuteReplyKind` is a closed 3-value enum (`Value`/`Timeout`/`Error`), all three handled explicitly above this label; `default:` exists only to satisfy this project's `-Wswitch-default`, per the source's own inline comment directly below this line. Reaching it via any value other than through the `Error` case falling through would require an out-of-range `static_cast` producing a value outside the enum's domain -- undefined behavior, not a legitimate test target." } From 2890caac07c6e081df1e632de77740530901391b Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sun, 20 Sep 2026 01:11:11 +0200 Subject: [PATCH 3/3] docs: record that SocketBackend now carries contextKey on a private bind Three spots in docs/spec/core/backend.md stated, correctly before this branch and incorrectly after it, that `SimulatedRemoteBackend` is the only backend overriding `registerModelWithContext`. - The `IBackend` method table now names both wire backends, and says why the override is not cosmetic: `attachLogIfConfigured` skips the `LogProvider` lookup entirely on an empty key, so dropping it leaves the instance with no action log rather than a log missing a field. - `SocketBackend`'s API reference gains a `registerModelWithContext` row, `registerModel` becomes the empty-key forwarder it now is, and `bindModel`'s row records that every shape carries `request.contextKey`, the private one included. The empty-`primary` degrades of `registerModelShared`/`attachModel` are named where they land. - The design-decisions row for the permissive default says what the permissiveness costs, since it is what let this ship unnoticed. Scope held to what this branch establishes. Nothing here touches `bindModel`'s blocking/non-blocking question (morph#593) or `QtWebSocketBackend`, whose own drop is real, unchanged, and filed as morph#594 -- `:1215` still describes it accurately. Verified: `scripts/check_spec_sync.sh` over this branch's file list reports "Spec sync OK: 10 sub-domain(s) classified", and `scripts/check_spec_citations.sh` reports "Prose lint OK" with 837 references and 73 cited sections scanned. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GS5K2vqZtC4xbRiGJHT7jH --- docs/spec/core/backend.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/docs/spec/core/backend.md b/docs/spec/core/backend.md index 9c625a909..4c7ccbdcd 100644 --- a/docs/spec/core/backend.md +++ b/docs/spec/core/backend.md @@ -75,7 +75,7 @@ holds a `unique_ptr` and delegates all model operations to it. | Method | Purpose | |---|---| | `registerModel(typeId, factory)` | Registers a new model instance, returns its opaque `ModelId`. | -| `registerModelWithContext(typeId, factory, contextKey)` | Same as `registerModel`, additionally passes a stable identity (e.g. account id). Default implementation drops `contextKey` and forwards to `registerModel` — correct for `LocalBackend` where the factory closure already captures identity. `SimulatedRemoteBackend` overrides to carry `contextKey` across the wire. | +| `registerModelWithContext(typeId, factory, contextKey)` | Same as `registerModel`, additionally passes a stable identity (e.g. account id). Default implementation drops `contextKey` and forwards to `registerModel` — correct for `LocalBackend` where the factory closure already captures identity. Every backend whose instances live behind a wire protocol overrides it to carry `contextKey` across: `SimulatedRemoteBackend` and `SocketBackend` both do. Not cosmetic — `RemoteServer::attachLogIfConfigured` skips the `LogProvider` lookup entirely on an empty `contextKey`, so a wire backend that drops the key leaves the instance with **no** action log rather than a log missing a field (morph#587). | | `registerModelAsync(typeId, factory, contextKey, onRegistered, onError)` | Optional non-blocking counterpart to `registerModelWithContext`. Returns `false` by default (no async path); `Bridge::registerHandler()` prefers this when it returns `true` and falls back to the synchronous call otherwise. See [Asynchronous registration](#asynchronous-registration--registermodelasync). | | `bindModel(request, cbExec)` | Acquires a model instance and returns a `Completion` delivered on `cbExec`. One verb covering `registerModelWithContext`, `registerModelShared` and `attachModel`, selected by the request's shape. The preferred surface — see [The structural registration surface](#the-structural-registration-surface--bindmodel-and-promotemodel). | | `promoteModel(request, cbExec)` | Files an already-live instance under a key and returns a `Completion` delivered on `cbExec`. The structural counterpart of `assignPrimary`. | @@ -2062,8 +2062,9 @@ not a behavior change to the existing loopback-only default. |---|---| | `explicit SocketBackend(serverUrl, cfg = Config{})` | Parses `serverUrl` (`ws://` only — throws immediately on `wss://`) and starts the I/O thread, which connects asynchronously. | | `waitForConnected(timeout = 5000ms)` | Blocks the calling thread on a condition variable until connected or the timeout elapses; returns the current connected state. The backend must outlive the call — destroying it while a thread is parked here is undefined, and there is no cancel (see Lifetime & ownership). | -| `registerModel(typeId, factory)` | Synchronous via a parked condition variable; `factory` ignored. Throws on `err` reply or disconnect. Thread-safe, but only one such call may be in flight at a time. | -| `bindModel(request, cbExec)` | Native override of the structural surface. Sends the envelope `request`'s shape names with a non-zero `callId` and returns immediately; the I/O thread settles the `Completion` when the reply arrives, delivered on `cbExec`. Never enters `sendSync`, so it takes no synchronous-call token and any number may be in flight. Rejects with `DisconnectedError` when the socket is down or drops first, or with `std::runtime_error{" failed: "}`. | +| `registerModel(typeId, factory)` | Forwards to `registerModelWithContext` with an empty `contextKey`; `factory` ignored. | +| `registerModelWithContext(typeId, factory, contextKey)` | Synchronous via a parked condition variable; sends `register` carrying `contextKey`, so the server's `LogProvider` is consulted for a private registration exactly as it is for a shared one (morph#587). `factory` ignored. Throws on `err` reply or disconnect. Thread-safe, but only one such call may be in flight at a time. `registerModelShared` and `attachModel` degrade here when `primary` is empty, so their private paths carry the key too. | +| `bindModel(request, cbExec)` | Native override of the structural surface. Sends the envelope `request`'s shape names with a non-zero `callId` and returns immediately; every shape carries `request.contextKey`, the private one included; the I/O thread settles the `Completion` when the reply arrives, delivered on `cbExec`. Never enters `sendSync`, so it takes no synchronous-call token and any number may be in flight. Rejects with `DisconnectedError` when the socket is down or drops first, or with `std::runtime_error{" failed: "}`. | | `promoteModel(request, cbExec)` | The `assign` counterpart of `bindModel`, on the same path; resolves with `request.mid` echoed back. An empty `primary` or a zero `mid` resolves without sending, matching `assignPrimary`'s guards. | | `deregisterModel(mid)` | **Fire-and-forget** — sends only if connected, does not wait for the ack. Carries a non-zero `callId` from the same counter `execute` uses so its unawaited `ok` cannot be handed to a parked synchronous control call (issue #454; the `QtWebSocketBackend` precedent is issue #65). Needs no pending-id bookkeeping of its own: `dispatchIncomingEnvelope` already drops a non-zero `callId` that is absent from `_pending`. | | `execute(mid, call, cbExec)` | Assigns a `callId`, sends `execute`, returns a `Completion`. Immediate `DisconnectedError` if not connected. Thread-safe; supports concurrent in-flight calls from multiple threads. | @@ -2091,7 +2092,7 @@ not a behavior change to the existing loopback-only default. | Decision | Choice | Why | |---|---|---| | Dual-path `ActionCall` | Three callables: `localOp`, `serializeAction`, `deserializeResult` | The same `ActionCall` struct works for both local and remote execution without an `if (isRemote)` branch at the call site — each backend uses the field(s) it needs. | -| `registerModelWithContext` | Virtual with a default that drops `contextKey` | `LocalBackend`'s factory closure already captures identity, so there is nothing to forward. `SimulatedRemoteBackend` overrides to carry `contextKey` across the wire so the server's `LogProvider` can attach an action log. | +| `registerModelWithContext` | Virtual with a default that drops `contextKey` | `LocalBackend`'s factory closure already captures identity, so there is nothing to forward — which is why the default drops the key rather than being pure virtual. A backend whose instances are constructed on the far side of a wire protocol has no such closure, so the envelope is the only channel the identity has: `SimulatedRemoteBackend` and `SocketBackend` both override it so the server's `LogProvider` can attach an action log. The default being *permissive* is what let `SocketBackend` ship without an override and silently stop journalling private registrations (morph#587); the price of that permissiveness is that "is this a wire backend?" has to be answered by hand for each new transport. | | `RemoteServer` heap requirement | `std::enable_shared_from_this` | `handle()` posts to the worker pool capturing `shared_from_this()` — the server must outlive any in-flight message. | | `handleInline` | Synchronous; caller-restricted to control messages | Safe to call from a worker-pool thread (e.g. from a `BridgeHandler` constructor). It is meant for `register`/`deregister` only; an `execute` envelope is rejected with an `err` reply, because `dispatchExecute` posts to the strand and would reply after `handleInline` returns (writing into an already-destroyed reply buffer). The rejection is now enforced by the code, matching the documented intent. | | `SimulatedRemoteBackend` factory ignored | Model construction delegated to `RemoteServer`'s `ModelRegistryFactory` | The factory closure lives on the client side; the server owns the actual instances. |