diff --git a/docs/spec/core/backend.md b/docs/spec/core/backend.md index 1543959d1..723381cca 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 @@ -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 diff --git a/include/morph/core/backend.hpp b/include/morph/core/backend.hpp index 6523d06f4..5bc3fd34a 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 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(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/include/morph/core/bridge.hpp b/include/morph/core/bridge.hpp index 6ae75aa74..70839555e 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/scripts/branch_partial_allowlist.json b/scripts/branch_partial_allowlist.json index 4b58f03c8..c4912ee36 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 bf79e936c..50add0d8f 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." } diff --git a/tests/test_async_registration.cpp b/tests/test_async_registration.cpp index ed1ddffc2..b3e0dc1fc 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,37 @@ 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 diff --git a/tests/test_backend_registration_surface.cpp b/tests/test_backend_registration_surface.cpp index 8e88d5c13..6d760152a 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); +}