core: stop a cancelled control call reaching the wrapped backend, and pin parkIfInFrame's unreachable double-claim arm (fixes #636, fixes #648)

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
@@ -634,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
@@ -2217,7 +2250,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

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<std::weak_ptr<BindPromise>> snapshot;
+        std::vector<std::weak_ptr<PendingControl>> 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 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.
+        ///
+        /// 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<Settled::Promise>(std::move(promise));
+        auto pending = std::make_shared<PendingControl>(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<BindPromise>& promise) {
+    /// @param pending Record to cancel and reject if `cancelPending` runs before
+    ///                 its task settles it.
+    void trackPending(const std::shared_ptr<PendingControl>& 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<detail::IBackend> _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<std::weak_ptr<BindPromise>> _pending;
+    std::vector<std::weak_ptr<PendingControl>> _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;

include/morph/core/bridge.hpp

@@ -335,14 +335,31 @@ 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;
     {
         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;