From 9ef6385d8b212cf01819bd3513f940830d7dcdee Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Mon, 21 Sep 2026 05:20:02 +0200 Subject: [PATCH 1/5] core: stop a cancelled control call from reaching the wrapped backend (fixes #636) `SynchronousBackendAdapter::cancelPending` rejected the completions the adapter itself produced (morph#619) and did nothing about the work behind them. A task still queued on `_control` reached the head of the strand afterwards and made its blocking control call anyway -- a real `registerModelWithContext`/`registerModelShared`/`attachModel` on the wrapped backend -- and its `resolve` then found the state already rejected and did nothing. The caller was told the bind was cancelled while the registration went through: a live instance on a backend whose `Bridge` is gone, or which `switchBackend` has just replaced, that nothing will ever `deregisterModel` because no caller ever learned its id. Settling a promise cannot stop a task, so the check has to be inside the task. Each dispatch now allocates a `PendingControl` -- the promise plus an `atomic_bool cancelled` -- which the strand task reads (acquire) before calling `op()` and `cancelPending` sets (release) before rejecting. `_pending` tracks those records weakly, exactly as it tracked the bare promises, so the expiry-means-settled bookkeeping and the amortised compaction are unchanged. Only the queued-but-not-started window closes, which is all this adapter can close: it has no way to interrupt a blocking verb it does not implement, so a task already inside `op()` still completes. That limit is now written into the header and the spec as a limit rather than as a defect. The test is the point. Asserting that the completion was rejected passes on the pre-fix code and proves nothing, so the new case observes the *wrapped backend* instead: `GatedBackend::entered` counts entries to a control call, call 1 holds the strand open so call 2 is provably queued-and-not-started, and a third call dispatched after the cancellation acts as a FIFO probe -- its arrival proves call 2's task has already run and declined. Two entries, not three. Verified by mutation, both sections: with the `cancelled` check disabled the case fails on `CHECK(inner->entered.load() == 2)` with `3 == 2`. GCC 16.2.1, Debug, Linux: 1557 passed, 1 failed as expected (22955 assertions). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- docs/spec/core/backend.md | 29 ++++-- include/morph/core/backend.hpp | 100 +++++++++++++++----- tests/test_backend_registration_surface.cpp | 86 +++++++++++++++++ 3 files changed, 186 insertions(+), 29 deletions(-) diff --git a/docs/spec/core/backend.md b/docs/spec/core/backend.md index 1543959d..0c044983 100644 --- a/docs/spec/core/backend.md +++ b/docs/spec/core/backend.md @@ -392,12 +392,27 @@ natively](#the-structural-registration-surface-natively). Two limits are deliberate rather than overlooked. A task that settles first wins, because its completion was not still pending — the same race - `LocalBackend::cancelPending` has always had. And a task already **queued** - on `_control` still runs its blocking control call against the wrapped - backend after `cancelPending` returns: the caller is told the bind was - cancelled while the registration may still go through. Stopping that is a - different change — it needs the task to check before calling `op()`, not the - promise to be settled after it — and is tracked as morph#636. + `LocalBackend::cancelPending` has always had. And a task already *inside* + `op()` cannot be recalled: the adapter has no way to interrupt a blocking + verb it does not implement. +- **It stops a control call the strand has not started yet** (morph#636). + Settling the promise is only half of the cancellation, because it says + nothing about the *work* behind it: a task still queued on `_control` used to + reach the head of the strand after `cancelPending` returned and make its + blocking control call anyway — an actual `registerModelWithContext` / + `registerModelShared` / `attachModel` on the wrapped backend, whose `resolve` + then found the state already rejected and did nothing. The caller was told + the bind was cancelled while the registration went through, leaving a live + instance on a backend whose `Bridge` is gone (`~Bridge`) or which + `switchBackend` has just replaced — one nothing will ever `deregisterModel`, + because no caller ever learned its id. So each dispatched task carries a + `PendingControl` record — its promise plus an `atomic_bool cancelled` — and + checks that flag before calling `op()`; `cancelPending` sets it (release) + before rejecting. What this closes is exactly the queued-but-not-started + window, which is all this adapter *can* close; the preceding bullet's + already-running case is unchanged, and a task that reads the flag a few + instructions before the store registers exactly as one already inside `op()` + would. - **Control calls are serialised** onto one strand, so the wrapped backend sees them one at a time, as it did when the blocking call itself serialised callers. `~SynchronousBackendAdapter` waits for any in-flight control call, so @@ -2217,7 +2232,7 @@ inside the class calls `close()` — no thread it joins can be waiting on it. | `bindModel(request, cbExec)` | Posts `inner->bindModelBlocking(request)` onto the control strand; settles the returned `Completion` on `cbExec`. Never blocks the caller. | | `bindWaitPolicy()` | `BindWait::kCallerMustNotBlock`, always. Not forwarded: it describes the two verbs the adapter reshapes. | | `promoteModel(request, cbExec)` | Posts `inner->assignPrimary(...)` onto the control strand; resolves with `request.mid`. | -| `cancelPending(exc)` | Rejects the adapter's own still-unsettled `bindModel`/`promoteModel` promises with `exc`, **then** forwards to `inner`. Not a plain forward: those promises are settled from `_control` tasks the wrapped backend has never heard of (morph#619). | +| `cancelPending(exc)` | Sets each still-unsettled `bindModel`/`promoteModel` record's `cancelled` flag and rejects its promise with `exc`, **then** forwards to `inner`. Not a plain forward: those promises are settled from `_control` tasks the wrapped backend has never heard of (morph#619). The flag is what stops a task still *queued* on `_control` from making its blocking control call after the caller was told the bind was cancelled (morph#636); a task already inside that call is unaffected. | | every other `IBackend` verb | Forwarded to `inner` unchanged. Since morph#571 those are the synchronous verbs only: the one verb that could carry a non-blocking path is `bindModel`, which this adapter reshapes. | ### Error types diff --git a/include/morph/core/backend.hpp b/include/morph/core/backend.hpp index 6523d06f..2b9923e5 100644 --- a/include/morph/core/backend.hpp +++ b/include/morph/core/backend.hpp @@ -884,23 +884,39 @@ class SynchronousBackendAdapter : public detail::IBackend { /// after finds the promise settled, which `CompletionState::setValue`'s /// `if (ready) return;` makes a no-op. /// - /// **What this does not do:** a task already queued on `_control` still - /// runs its blocking control call against the wrapped backend after this - /// returns. The caller is told the bind was cancelled, but the registration - /// may still happen — morph#636, since stopping it needs the task to check - /// before calling `op()`, not the promise to be settled after it. + /// Settling the promise is only half of it, because it cannot stop work the + /// strand has already been handed. Each dispatched task therefore carries a + /// cancellation flag next to its promise, and this verb **sets that flag + /// before rejecting**: a task still queued on `_control` sees it when it + /// reaches the head of the strand and returns without calling `op()`, so + /// the blocking control call never reaches the wrapped backend at all + /// (morph#636). Without it the caller was told the bind was cancelled while + /// the registration went through anyway — a live instance on a backend + /// whose `Bridge` is gone, which nothing will ever `deregisterModel`. + /// + /// **What this still does not do:** a task already *inside* `op()` cannot + /// be recalled. Only the queued-but-not-started window is closed, which is + /// all this adapter can close — it has no way to interrupt a blocking verb + /// it does not implement. A task that wins the race by a few instructions + /// (flag read, then this store) registers exactly as one that had already + /// entered `op()`, and its completion stays rejected either way. /// @param exc Exception delivered to every still-pending completion, this /// adapter's own and then the wrapped backend's. void cancelPending(const std::exception_ptr& exc) override { - std::vector> snapshot; + std::vector> snapshot; { std::scoped_lock const lock{_pendingMtx}; snapshot.swap(_pending); _compactAt = kPendingCompactFloor; } - for (auto& weak : snapshot) { - if (auto promise = weak.lock()) { - promise->reject(exc); + for (auto& pendingWeak : snapshot) { + if (auto pending = pendingWeak.lock()) { + // Flag first, promise second. A task that reads the flag after + // this store declines to run; one that read it just before + // finds its promise already rejected by the line below, which + // is the pre-morph#636 outcome and the narrowest window left. + pending->cancelled.store(true, std::memory_order_release); + pending->promise.reject(exc); } } _inner->cancelPending(exc); @@ -923,6 +939,35 @@ class SynchronousBackendAdapter : public detail::IBackend { void setSession(::morph::session::Context session) override { _inner->setSession(std::move(session)); } private: + /// @brief One dispatched control call: its promise and its cancellation flag. + /// + /// The two travel together because `cancelPending` has to act on both, and + /// acting on only the promise is the defect morph#636 recorded — the queued + /// task went on to make the blocking control call the caller had just been + /// told was cancelled. The strand task holds the only `shared_ptr` to this + /// record; `_pending` holds `weak_ptr`s, so an entry expires by itself when + /// the task is destroyed. + struct PendingControl { + /// @brief Takes ownership of the dispatched call's promise. + /// @param p Producer side of the `Completion` handed to the caller. + explicit PendingControl(BindPromise p) : promise{std::move(p)} {} + + /// @brief Producer side of the completion this call settles. + /// + /// Touched by the strand task (resolve/reject) and by `cancelPending` + /// (reject); `Promise`'s own `CompletionState` is internally + /// synchronised, so no further lock is needed here. + BindPromise promise; + + /// @brief Set by `cancelPending` before it rejects; read by the task + /// before it calls `op()`. + /// + /// Atomic rather than guarded by `_pendingMtx`, so the strand task + /// never has to take a lock the caller's thread also takes just to + /// learn whether it should run. + std::atomic_bool cancelled{false}; + }; + /// @brief Posts @p op to the control strand and settles a `Completion` with its outcome. /// /// The posted task captures the wrapped backend's `shared_ptr` and the @@ -936,26 +981,36 @@ class SynchronousBackendAdapter : public detail::IBackend { ::morph::async::Completion<::morph::exec::detail::ModelId> dispatch(::morph::exec::IExecutor& cbExec, Op op) { using Settled = ::morph::async::Completion<::morph::exec::detail::ModelId>; auto [completion, promise] = Settled::makeSettleable(&cbExec); - auto shared = std::make_shared(std::move(promise)); + auto pending = std::make_shared(std::move(promise)); // Tracked *before* the post, not after: a `cancelPending` that lands in // between would otherwise find an empty list and leave a completion // that is genuinely pending uncancelled. Rejecting a promise whose task // has not started yet is safe — the task's own `resolve` then finds the // state ready and returns (morph#619). - trackPending(shared); - _control.post(kControlStrand, [shared, op = std::move(op)]() mutable { + trackPending(pending); + _control.post(kControlStrand, [pending, op = std::move(op)]() mutable { + // Checked *before* `op()`, which is the whole of morph#636: a + // promise settled by `cancelPending` makes the reply a no-op but + // says nothing about the call, and this task is the last place that + // can decline to make it. Read with acquire against + // `cancelPending`'s release store, so a task that observes the flag + // also observes everything the cancelling thread did before setting + // it. + if (pending->cancelled.load(std::memory_order_acquire)) { + return; + } try { - shared->resolve(op()); + pending->promise.resolve(op()); } catch (...) { - shared->reject(std::current_exception()); + pending->promise.reject(std::current_exception()); } }); return std::move(completion); } - /// @brief Records @p promise as cancellable until its task settles it. + /// @brief Records @p pending as cancellable until its task settles it. /// - /// The strand task holds the only `shared_ptr` to the promise, so an entry + /// The strand task holds the only `shared_ptr` to the record, so an entry /// here expires exactly when that task is destroyed — "still pending" needs /// no separate bookkeeping and no erase on the success path. /// @@ -967,14 +1022,15 @@ class SynchronousBackendAdapter : public detail::IBackend { /// so in practice the live count is one and the floor is never reached; /// without the sweep the list would still grow without bound on an adapter /// whose `cancelPending` is never called. - /// @param promise Promise to reject if `cancelPending` runs before its task settles it. - void trackPending(const std::shared_ptr& promise) { + /// @param pending Record to cancel and reject if `cancelPending` runs before + /// its task settles it. + void trackPending(const std::shared_ptr& pending) { std::scoped_lock const lock{_pendingMtx}; if (_pending.size() >= _compactAt) { std::erase_if(_pending, [](const auto& weak) { return weak.expired(); }); _compactAt = std::max(kPendingCompactFloor, _pending.size() * 2); } - _pending.emplace_back(promise); + _pending.emplace_back(pending); } /// @brief The single strand key every control call shares, so they run one @@ -988,11 +1044,11 @@ class SynchronousBackendAdapter : public detail::IBackend { std::shared_ptr _inner; ::morph::exec::detail::StrandExecutor _control; mutable std::mutex _pendingMtx; - // Every `bindModel`/`promoteModel` promise handed to a `_control` task and - // not yet settled by it. Weak, so a settled task's promise drops out on its + // Every `bindModel`/`promoteModel` record handed to a `_control` task and + // not yet settled by it. Weak, so a settled task's record drops out on its // own; guarded by `_pendingMtx`, because `cancelPending` is called from // `Bridge`'s thread while `dispatch` runs on whichever thread called it. - std::vector> _pending; + std::vector> _pending; // Size at which `trackPending` next sweeps `_pending`; re-armed at twice // the surviving count. Guarded by `_pendingMtx` with `_pending` itself. std::size_t _compactAt = kPendingCompactFloor; diff --git a/tests/test_backend_registration_surface.cpp b/tests/test_backend_registration_surface.cpp index 8e88d5c1..02b916f8 100644 --- a/tests/test_backend_registration_surface.cpp +++ b/tests/test_backend_registration_surface.cpp @@ -752,3 +752,89 @@ TEST_CASE("morph::backend::SynchronousBackendAdapter: cancelPending rejects the REQUIRE(okRan.load() == 0); REQUIRE(errRan.load() == 1); } + +// ── cancelPending and the control call the strand has not started yet (#636) ─ +// +// #619's case above is about the *completion*: it must be rejected. This one is +// about the *work behind it*: a task still queued on `_control` when +// `cancelPending` runs must never make its blocking control call at all. +// Settling the promise cannot achieve that -- the task's own `resolve` being a +// no-op afterwards says nothing about the `registerModelWithContext` it made on +// the way there -- so the check has to be inside the task, before `op()`. +// +// The observation this case rests on is `GatedBackend::entered`, which the +// wrapped backend increments on *entry* to a control call. Asserting only that +// the completion was rejected would pass on the pre-#636 code and prove +// nothing; asserting that the wrapped backend was never entered is what the +// fix changes. Verified by mutation: with the `cancelled` check removed from +// `SynchronousBackendAdapter::dispatch`'s task, this case fails on +// `entered == 2` (it sees 3). + +TEST_CASE("morph::backend::SynchronousBackendAdapter: cancelPending stops a queued control call from ever reaching " + "the wrapped backend", + "[backend][registration-surface][threading]") { + morph::exec::ThreadPoolExecutor pool{1}; + morph::exec::MainThreadExecutor callerExec; + auto inner = std::make_shared(); + SynchronousBackendAdapter adapter{inner, pool}; + + std::atomic okRan{0}; + std::atomic errRan{0}; + + auto attach = [&](ModelCompletion completion) { + completion.then([&](ModelId /*mid*/) { okRan.fetch_add(1); }).onError([&](const std::exception_ptr& /*exc*/) { + errRan.fetch_add(1); + }); + }; + + std::function dispatchOne; + SECTION("bind") { + dispatchOne = [&] { + return adapter.bindModel( + BindRequest{.typeId = std::string{kTypeId}, .factory = makeHolder, .contextKey = "ctx", .primary = {}}, + callerExec); + }; + } + SECTION("promote") { + dispatchOne = [&] { + return adapter.promoteModel(PromoteRequest{.mid = ModelId{7}, .typeId = std::string{kTypeId}, + .primary = "key"}, + callerExec); + }; + } + + // Call 1 occupies the strand and blocks inside the wrapped backend, so + // call 2 is provably *queued and not started* -- the window this fix + // closes, rather than one the scheduler happened to give us. + attach(dispatchOne()); + REQUIRE(morph::testing::waitUntil([&] { return inner->entered.load() == 1; })); + attach(dispatchOne()); + REQUIRE(inner->entered.load() == 1); + + adapter.cancelPending(std::make_exception_ptr(morph::backend::BridgeDestroyedError{})); + REQUIRE(morph::testing::waitUntil([&] { + callerExec.runOnce(); + return errRan.load() == 2; + })); + + // A third call, dispatched *after* the cancellation, is not one of the + // promises `cancelPending` snapshotted, so it runs. It is the probe: the + // strand is FIFO, so its arrival at the wrapped backend proves call 2's + // task has already run to completion and can no longer call anything. + std::atomic probeOk{0}; + dispatchOne().then([&](ModelId /*mid*/) { probeOk.fetch_add(1); }); + inner->letGo(); + REQUIRE(morph::testing::waitUntil([&] { + callerExec.runOnce(); + return probeOk.load() == 1; + })); + + // Two entries, not three: call 1 (already inside `op()` when the + // cancellation landed) and the probe. Call 2 never reached the wrapped + // backend, so nothing registered behind the caller's back. + CHECK(inner->entered.load() == 2); + CHECK(inner->finished.load() == 2); + CHECK(okRan.load() == 0); + CHECK(errRan.load() == 2); + CHECK(inner->cancels.load() == 1); +} From b3a6d5887d1ee36be995a8b6361a1f2aa26cebb2 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Mon, 21 Sep 2026 05:20:13 +0200 Subject: [PATCH 2/5] core/bridge: pin parkIfInFrame's double-claim arm and correct the three places that misdescribe it (fixes #648) The ticket asked whether the arm should be deleted or allowlisted. Neither: it is kept and it is now *tested*, which is strictly better than both -- deletion removes a caller-invariant safety net to satisfy a coverage gate, and an allowlist entry records "nothing reaches this" forever without ever detecting the day something does. What was measured, on this branch, GCC 16.2.1, Debug, Linux: * Replacing `return true` in the arm with `std::abort()` and running the whole suite (1556 cases, 22932 assertions) plus `morph_net_tests` (191 cases, 1112 assertions) fires it **zero** times. So the ticket's premise holds, and by direct reachability rather than by reading: `handoff.fired` is never true on entry, because all eight call sites are `.then`/`.onError` on one `Completion` and a `CompletionState` settles once. * Deleting the arm outright leaves the whole suite green, `DoubleFiringBackend` included (`completions == 1`). The ticket is therefore not wrong, and nothing in the tree would have noticed the deletion. That second measurement is the actual problem: dead defensive code whose removal no test detects. `parkIfInFrame` is a free function in `detail`, and the invariant that makes the arm unreachable belongs to every *current* caller, not to the function -- a ninth site that does not park one `Completion`'s outcome would need it again. So the arm stays and a new case calls `parkIfInFrame` directly, twice on one handoff, asserting the second claim returns `true` and leaves the first outcome intact. With the arm deleted that case, and only that case, fails (1556 passed, 1 failed). Three statements in the tree disagreed about this and one of them was simply false: * `tests/test_async_registration.cpp:2160` said the guard "must swallow the second, already-claimed callback". Stale since morph#571 -- what makes that test pass is `CompletionState`, which drops the second settle before any `Bridge` code sees it. Rewritten to say so, and to point at the new case. * `docs/spec/core/backend.md:638` said the guard is kept "because `parkIfInFrame` is also called from the dispatching frame". That is false: the dispatching frame calls `claimHandoff`/`awaitHandoff`, and `git grep` finds no other caller. Replaced with the measured reason. * `bridge.hpp`'s own comment said only that a backend gets one callback. It now carries the reachability argument and what it rests on. Decided separately, as the ticket asked: the `try`/`catch (...)` around the dispatch stays untouched. Unlike the double-claim arm it is not dead -- any out-of-tree `IBackend` override that throws out of `bindModel` reaches it, `IBackend` is a public extension point, and `ThrowingDispatchBackend` already exercises it in-tree. Covered defensive code, not dead code. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- docs/spec/core/backend.md | 22 ++++++++++++-- include/morph/core/bridge.hpp | 19 +++++++++++- tests/test_async_registration.cpp | 50 +++++++++++++++++++++++++++---- 3 files changed, 83 insertions(+), 8 deletions(-) diff --git a/docs/spec/core/backend.md b/docs/spec/core/backend.md index 0c044983..723381cc 100644 --- a/docs/spec/core/backend.md +++ b/docs/spec/core/backend.md @@ -649,8 +649,26 @@ double-claim guard. A `Completion` cannot be settled twice — `CompletionState` drops the second settle before any `Bridge` code sees it — so `tests/test_async_registration.cpp`'s `DoubleFiringBackend` now pins the observable contract ("exactly one `onDone`") while that guard inside -`parkIfInFrame` is no longer reachable from a backend at all. The guard is kept -because `parkIfInFrame` is also called from the dispatching frame. +`parkIfInFrame` is no longer reachable from a backend at all. + +morph#648 settled what to do about the now-unreachable arm, and corrected the +reason recorded here: this section used to say the guard was kept "because +`parkIfInFrame` is also called from the dispatching frame", which is false — +the dispatching frame calls `claimHandoff`/`awaitHandoff`, and all eight +`parkIfInFrame` call sites are completion callbacks. That the arm is +unreachable is *measured*, not read: replacing its `return true` with an +`abort()` and running `morph_tests` (1556 cases) and `morph_net_tests` (191) +fires it zero times. It is kept anyway, for a different reason than the one +that was written down — the invariant that makes it dead is a property of every +current *caller*, not of the function, so a ninth site that does not park a +single `Completion`'s outcome would resurrect it — and it is now pinned by a +test that calls `parkIfInFrame` directly, twice on one handoff, rather than +left as an arm whose deletion nothing would detect. The neighbouring +`try`/`catch (...)` around the dispatch was decided separately and left alone: +it is reachable by any out-of-tree `IBackend` override that throws out of +`bindModel`, `IBackend` is a public extension point, and +`ThrowingDispatchBackend` already exercises it — so it is covered defensive +code, not dead code. `Bridge::installReconnectHandler` and `Bridge::switchBackend`'s phase 1 were the two dispatch sites morph#568 did **not** move: both still called the diff --git a/include/morph/core/bridge.hpp b/include/morph/core/bridge.hpp index 6ae75aa7..70839555 100644 --- a/include/morph/core/bridge.hpp +++ b/include/morph/core/bridge.hpp @@ -335,6 +335,20 @@ struct AsyncDispatchHandoff { /// call is still on the dispatcher's stack (which owns the outcome from /// here on) or because another callback already claimed this dispatch. /// `false` if the caller owns the outcome and should deliver it itself. +/// +/// @par Reachability of the double-claim arm +/// No backend can reach it today, and that is a property of the callers rather +/// than of this function (morph#648). Every one of the eight call sites below +/// is a `.then`/`.onError` on one `Completion`, and a `CompletionState` settles +/// once — the second `resolve`/`reject` is a documented no-op — so exactly one +/// of the two lambdas runs, exactly once, and `fired` is always `false` on +/// entry. Measured, not assumed: replacing the arm's `return true` with an +/// `abort()` runs the whole suite (1556 cases) plus `morph_net_tests` (191) +/// without firing. The arm is kept because the invariant that makes it dead is +/// every *current* caller's, and a ninth site that does not park a single +/// `Completion`'s outcome would need it again; `tests/test_async_registration.cpp` +/// calls this function directly so the arm is pinned by a test rather than +/// merely unreached. inline bool parkIfInFrame(AsyncDispatchHandoff& handoff, bool succeeded, ::morph::exec::detail::ModelId modelId, std::exception_ptr failure) { bool inFrame = false; @@ -342,7 +356,10 @@ inline bool parkIfInFrame(AsyncDispatchHandoff& handoff, bool succeeded, ::morph std::scoped_lock const guard{handoff.mtx}; if (handoff.fired) { // A backend is contractually allowed exactly one callback per dispatch; - // swallow a second one rather than reporting twice. + // swallow a second one rather than reporting twice. Unreachable from + // a backend since morph#571 put every dispatch behind one + // `Completion` -- see @par Reachability above for what that rests on + // and why the arm stays. return true; } handoff.fired = true; diff --git a/tests/test_async_registration.cpp b/tests/test_async_registration.cpp index ed1ddffc..fd5f00d4 100644 --- a/tests/test_async_registration.cpp +++ b/tests/test_async_registration.cpp @@ -394,6 +394,8 @@ class ThrowingDispatchBackend : public AsyncRegisterBackend { // settle before any Bridge code sees it -- so this double now pins the // *observable* contract ("exactly one onDone") while the guard inside // parkIfInFrame is no longer reachable from a backend. See the PR for morph#571. +// The guard itself is pinned by a direct call to parkIfInFrame instead +// (morph#648), next to the test case this double drives. class DoubleFiringBackend : public AsyncRegisterBackend { public: ModelCompletion bindModel(morph::backend::detail::BindRequest request, morph::exec::IExecutor& cbExec) override { @@ -2156,11 +2158,17 @@ TEST_CASE("execute() surfaces a throwing ActionKeyTraits::key() through onError TEST_CASE("attachHandlerAsync reports exactly once even when the backend fires its callback twice inline", "[bridge][registration][shared-instances][issue26]") { - // DoubleFiringBackend violates bindModel's documented one-settle - // contract on purpose: detail::parkIfInFrame's `handoff.fired` guard must - // swallow the second, already-claimed callback rather than letting - // attachHandlerAsync invoke onDone (and, downstream, publish the binding) - // twice for a single dispatch. + // DoubleFiringBackend violates bindModel's documented one-settle contract + // on purpose: attachHandlerAsync must still invoke onDone (and, downstream, + // publish the binding) exactly once for a single dispatch. + // + // What makes that hold is CompletionState, not detail::parkIfInFrame's + // `handoff.fired` guard -- this comment used to name the guard, and was + // wrong from morph#571 onwards (morph#648). The second promise.resolve() + // below is dropped by the already-settled state before any Bridge code + // sees it, so parkIfInFrame is entered once and its double-claim arm is + // never taken. That arm is pinned separately, by the direct-call case + // below this one; what this case pins is the observable contract. SyncExec cbExec; morph::bridge::Bridge bridge{std::make_unique()}; morph::bridge::BridgeHandler handler{bridge, &cbExec}; @@ -2179,6 +2187,38 @@ TEST_CASE("attachHandlerAsync reports exactly once even when the backend fires i CHECK(handler.primary().value_or(-1) == 21); } +TEST_CASE("parkIfInFrame swallows a second claim on the same handoff and keeps the first outcome", + "[bridge][registration][shared-instances][issue26]") { + // The arm the case above used to claim to exercise, driven where it can + // actually be reached: directly (morph#648). + // + // No backend reaches it any more -- every dispatch site parks one + // Completion's outcome, and a CompletionState settles once -- so without + // this case the arm is dead code whose deletion nothing would detect. + // Verified by mutation: with `if (handoff.fired) return true;` deleted from + // detail::parkIfInFrame, the whole suite still passes and only this case + // fails. parkIfInFrame is a free function in `detail`, and the invariant + // that makes the arm unreachable belongs to its callers, so its contract is + // pinned here rather than inferred from the callers that happen to exist. + morph::bridge::detail::AsyncDispatchHandoff handoff; + handoff.inFrame = false; // The dispatching frame has already returned. + + auto const first = + morph::bridge::detail::parkIfInFrame(handoff, true, morph::exec::detail::ModelId{41}, nullptr); + CHECK_FALSE(first); // Out of frame: the first claimant owns the outcome. + CHECK(handoff.fired); + + auto const second = morph::bridge::detail::parkIfInFrame( + handoff, false, morph::exec::detail::ModelId{}, std::make_exception_ptr(std::runtime_error("second"))); + CHECK(second); // Already claimed: the caller must not report it. + + // ...and the second claim left the first outcome untouched, so a frame + // that had not yet called claimHandoff still picks up the real one. + CHECK(handoff.succeeded); + CHECK(handoff.modelId.v == 41U); + CHECK(handoff.failure == nullptr); +} + TEST_CASE("attachHandlerAsync's out-of-frame success callback is a genuine no-op once the binding itself is gone", "[bridge][registration][shared-instances][issue26]") { // The other attachHandlerAsync/ensureBoundAsync "binding is gone" tests From 4ef26fd8c5f771284b3f0a1e70b6d68b93988eea Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Mon, 21 Sep 2026 05:20:52 +0200 Subject: [PATCH 3/5] chore(gates): repoint the six line hints the two fixes moved in backend.hpp and bridge.hpp Mechanical, no disposition changed. `scripts/check_mutation_survivors.py` reported three and the shared resolver behind `check_branch_coverage.py` reported three more. Two of the six were reported as ambiguous rather than drifted, because the cited text appears more than once. Both were resolved by reading the candidates, not by taking the first: * backend.hpp `registerCount` emission: 1087 (`LocalBackend::registerModel`, which the entry's own reason names) vs 1106 (`registerModelShared`). * bridge.hpp `if (deadlineHandle && schedulerRef)`: 1679 is the `catch` block that decrements `_pendingCalls` and cancels the deadline before rethrowing, which is what the entry says it covers; 1703 and 1792 are the `.then()`/`.onError()` continuations the same entry explicitly excludes. The third mutation-survivors entry (`executeInFlight`) was likewise ambiguous between the `fetch_add` at 1251 and the `fetch_sub` at 1296; the reason says "the increment side", so 1251. The two prose line numbers inside the bridge.hpp:1679 entry's own reason are refreshed with it (1686/1775 -> 1703/1792, and its cross-reference to the sibling entry, 1540 -> 1557). Those are free text the gate does not audit, which is exactly why they rot. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- scripts/branch_partial_allowlist.json | 8 ++++---- scripts/mutation_survivors.json | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/scripts/branch_partial_allowlist.json b/scripts/branch_partial_allowlist.json index 4b58f03c..c4912ee3 100644 --- a/scripts/branch_partial_allowlist.json +++ b/scripts/branch_partial_allowlist.json @@ -80,7 +80,7 @@ }, { "file": "include/morph/core/backend.hpp", - "line": 1140, + "line": 1196, "source": "if (const auto* inst = _instances.find(modelId)) {", "reason": "Unreachable by construction given the `_changeAware`/`_instances` invariant (core audit finding BK2). `_changeAware` is an index over the instance directory: an id enters it in `createHolder` (this file, when the holder answers `isBackendChangeAware()`) in the same `_regMtx`-held critical section that files the instance, and leaves it in `deregisterModel` only when `InstanceDirectory::release` reports the instance actually destroyed. `notifyBackendChanged()` (this function) holds the same `_regMtx` while walking `_changeAware` and looking each id up at this line, so every id it walks is still live -- the null arm cannot occur without a code change that breaks that subset invariant. Formerly keyed on `_models`, the map morph#523 replaced with the directory; the invariant and its reason are unchanged." }, @@ -98,15 +98,15 @@ }, { "file": "include/morph/core/bridge.hpp", - "line": 1540, + "line": 1557, "source": "if (_executeDeadline.count() > 0 && _timeoutScheduler) {", "reason": "Unreachable by construction (core audit finding B6). `setExecuteDeadline` (this file) is the only writer of both `_executeDeadline` and `_timeoutScheduler`, and always creates `_timeoutScheduler` in the same call that sets `_executeDeadline` positive (`_executeDeadline = deadline; if (_executeDeadline.count() > 0 && !_timeoutScheduler) { _timeoutScheduler = std::make_shared<...>(); }`, both under `_executeDeadlineMtx`); nothing anywhere resets `_timeoutScheduler` back to null -- the class's own doc comment on `setExecuteDeadline` says so explicitly (\"setting the deadline back to 0 stops new calls from arming it but does not tear the thread down\"). So `_executeDeadline > 0 && !_timeoutScheduler` cannot happen at this line once any positive deadline has ever been set." }, { "file": "include/morph/core/bridge.hpp", - "line": 1662, + "line": 1679, "source": "if (deadlineHandle && schedulerRef) {", - "reason": "Unreachable by construction, same joint-assignment shape as B6 above (core audit finding B11, reclassified (a)->(b) on review). `deadlineHandle` and `schedulerRef` are assigned together, a few lines above this one in `executeVia`, only inside `if (_executeDeadline.count() > 0 && _timeoutScheduler) { schedulerRef = _timeoutScheduler; ... }` (see the bridge.hpp:1540 entry above) -- there is no path that sets `deadlineHandle` without also having set `schedulerRef` from the same non-null `_timeoutScheduler` in the same conditional. So `schedulerRef` null while `deadlineHandle` is non-null cannot occur; the only theoretically-open arm this compound condition has is structurally impossible. This entry is specifically the exception-path use of the guard (the `catch` block that undoes `_pendingCalls` and cancels the deadline before rethrowing). Two more textually-identical `if (deadlineHandle && schedulerRef)` guards exist further down, in the `.then()`/`.onError()` continuations (lines 1686, 1775) -- present on master too, this PR only shifted their line numbers -- which are a different guard on a different, reachable arm (schedulerRef going null between capture and a callback that can run after `~Bridge()`) and are not covered by this disposition." + "reason": "Unreachable by construction, same joint-assignment shape as B6 above (core audit finding B11, reclassified (a)->(b) on review). `deadlineHandle` and `schedulerRef` are assigned together, a few lines above this one in `executeVia`, only inside `if (_executeDeadline.count() > 0 && _timeoutScheduler) { schedulerRef = _timeoutScheduler; ... }` (see the bridge.hpp:1557 entry above) -- there is no path that sets `deadlineHandle` without also having set `schedulerRef` from the same non-null `_timeoutScheduler` in the same conditional. So `schedulerRef` null while `deadlineHandle` is non-null cannot occur; the only theoretically-open arm this compound condition has is structurally impossible. This entry is specifically the exception-path use of the guard (the `catch` block that undoes `_pendingCalls` and cancels the deadline before rethrowing). Two more textually-identical `if (deadlineHandle && schedulerRef)` guards exist further down, in the `.then()`/`.onError()` continuations (lines 1703, 1792) -- present on master too, this PR only shifted their line numbers -- which are a different guard on a different, reachable arm (schedulerRef going null between capture and a callback that can run after `~Bridge()`) and are not covered by this disposition." }, { "file": "include/morph/core/remote.hpp", diff --git a/scripts/mutation_survivors.json b/scripts/mutation_survivors.json index bf79e936..50add0d8 100644 --- a/scripts/mutation_survivors.json +++ b/scripts/mutation_survivors.json @@ -81,7 +81,7 @@ }, { "file": "include/morph/core/backend.hpp", - "line": 1138, + "line": 1194, "mutants": 1, "mutator": "cxx_replace_scalar_call", "source": "aware.reserve(_changeAware.size());", @@ -147,13 +147,13 @@ "representative_sites": [ { "file": "include/morph/core/backend.hpp", - "line": 1031, + "line": 1087, "source": "::morph::observe::detail::emitMetric(::morph::observe::Metric::registerCount, 1.0);", "reason": "The registerCount emission on LocalBackend::registerModel. Note for whoever refreshes this hint: the same statement appears character-for-character on the registerModelShared arm as well, so if this `line` ever drifts the gate will report the citation as ambiguous rather than printing a corrected line. That is the right outcome -- which of the two arms is meant is a question for a reader, not for a resolver -- and it is recorded here so the message is not a surprise." }, { "file": "include/morph/core/backend.hpp", - "line": 1195, + "line": 1251, "source": "::morph::observe::detail::emitMetric(::morph::observe::Metric::executeInFlight,", "reason": "The executeInFlight emission on the increment side of an execute. Its decrement twin inside the posted task is the same text after stripping, so the ambiguity note on the registerCount entry above applies here too." } From 0644d6cfe15a95e5c98ebf7fff50e54d1019fc8e Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Mon, 21 Sep 2026 05:27:27 +0200 Subject: [PATCH 4/5] style: clang-format the two test files this branch edits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `clang-format` leg was red on 9 violations across the lines this branch added: tests/test_async_registration.cpp:2206:23 auto const first = tests/test_async_registration.cpp:2211:62 parkIfInFrame( tests/test_backend_registration_surface.cpp:773 TEST_CASE("… cancelPending … tests/test_backend_registration_surface.cpp:800 adapter.promoteModel(… (and four more) Not a version skew: `ci.yml` pins `CLANG_VERSION: "22"` and the local binary is clang-format 22.1.8, which reproduces all nine. Running it over the two files leaves none. Whitespace only. Verified by comparing both files before and after with all whitespace stripped -- the token streams are identical, so no test, assertion or string literal changed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- tests/test_async_registration.cpp | 7 +++---- tests/test_backend_registration_surface.cpp | 12 ++++++------ 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/tests/test_async_registration.cpp b/tests/test_async_registration.cpp index fd5f00d4..b3e0dc1f 100644 --- a/tests/test_async_registration.cpp +++ b/tests/test_async_registration.cpp @@ -2203,13 +2203,12 @@ TEST_CASE("parkIfInFrame swallows a second claim on the same handoff and keeps t morph::bridge::detail::AsyncDispatchHandoff handoff; handoff.inFrame = false; // The dispatching frame has already returned. - auto const first = - morph::bridge::detail::parkIfInFrame(handoff, true, morph::exec::detail::ModelId{41}, nullptr); + auto const first = morph::bridge::detail::parkIfInFrame(handoff, true, morph::exec::detail::ModelId{41}, nullptr); CHECK_FALSE(first); // Out of frame: the first claimant owns the outcome. CHECK(handoff.fired); - auto const second = morph::bridge::detail::parkIfInFrame( - handoff, false, morph::exec::detail::ModelId{}, std::make_exception_ptr(std::runtime_error("second"))); + auto const second = morph::bridge::detail::parkIfInFrame(handoff, false, morph::exec::detail::ModelId{}, + std::make_exception_ptr(std::runtime_error("second"))); CHECK(second); // Already claimed: the caller must not report it. // ...and the second claim left the first outcome untouched, so a frame diff --git a/tests/test_backend_registration_surface.cpp b/tests/test_backend_registration_surface.cpp index 02b916f8..6d760152 100644 --- a/tests/test_backend_registration_surface.cpp +++ b/tests/test_backend_registration_surface.cpp @@ -770,9 +770,10 @@ TEST_CASE("morph::backend::SynchronousBackendAdapter: cancelPending rejects the // `SynchronousBackendAdapter::dispatch`'s task, this case fails on // `entered == 2` (it sees 3). -TEST_CASE("morph::backend::SynchronousBackendAdapter: cancelPending stops a queued control call from ever reaching " - "the wrapped backend", - "[backend][registration-surface][threading]") { +TEST_CASE( + "morph::backend::SynchronousBackendAdapter: cancelPending stops a queued control call from ever reaching " + "the wrapped backend", + "[backend][registration-surface][threading]") { morph::exec::ThreadPoolExecutor pool{1}; morph::exec::MainThreadExecutor callerExec; auto inner = std::make_shared(); @@ -797,9 +798,8 @@ TEST_CASE("morph::backend::SynchronousBackendAdapter: cancelPending stops a queu } SECTION("promote") { dispatchOne = [&] { - return adapter.promoteModel(PromoteRequest{.mid = ModelId{7}, .typeId = std::string{kTypeId}, - .primary = "key"}, - callerExec); + return adapter.promoteModel( + PromoteRequest{.mid = ModelId{7}, .typeId = std::string{kTypeId}, .primary = "key"}, callerExec); }; } From 180edc9969d54ef33ac94682f2bff9b368a84240 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Mon, 21 Sep 2026 07:14:25 +0200 Subject: [PATCH 5/5] core: name PendingControl's parameter for what it is `clang-tidy-diff` was red on the one finding this branch introduced: include/morph/core/backend.hpp:953:45: error: parameter name 'p' is too short, expected at least 3 characters [readability-identifier-length] Renamed `p` to `dispatched`, and the `@param` line with it -- the Docs workflow runs Doxygen with WARN_AS_ERROR=FAIL_ON_WARNINGS, so a `@param` that no longer names a parameter is its own failure. The file's line count is unchanged, so no citation in scripts/mutation_survivors.json or scripts/branch_partial_allowlist.json moves; both resolvers still pass. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- include/morph/core/backend.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/morph/core/backend.hpp b/include/morph/core/backend.hpp index 2b9923e5..5bc3fd34 100644 --- a/include/morph/core/backend.hpp +++ b/include/morph/core/backend.hpp @@ -949,8 +949,8 @@ class SynchronousBackendAdapter : public detail::IBackend { /// the task is destroyed. struct PendingControl { /// @brief Takes ownership of the dispatched call's promise. - /// @param p Producer side of the `Completion` handed to the caller. - explicit PendingControl(BindPromise p) : promise{std::move(p)} {} + /// @param dispatched Producer side of the `Completion` handed to the caller. + explicit PendingControl(BindPromise dispatched) : promise{std::move(dispatched)} {} /// @brief Producer side of the completion this call settles. ///