From 5df59ee7d30373f0002a568c45cfa972ee899385 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 7 Oct 2026 13:55:36 +0000 Subject: [PATCH] core: LocalBackend::cancelPending settles its calls before stopping their handlers cancelPending requested stop on every running Task handler and only then settled the pending calls with the error it was given. A stopped handler resumes on its strand and settles its own call with OperationCancelled; when that settle won the race, the caller was answered with the stop instead of BackendChangedError / DisconnectedError, which concurrency_and_lifetimes.md says it gets. Settle first, then stop: the handler's own settle is then the ignored second one. This is what the nightly Valgrind job has been failing on. Three tests in test_coroutine_model.cpp wait for that error type and timed out when the other one arrived. Measured locally under valgrind 3.22, the three tests together, 40 runs each: before: 23/40 failed; every failing run, and no passing one, logged a completion settled with OperationCancelled after: 0/40 failed Raising the wait budget to 60 s did not help (runs still gave up at 60 s) and neither did --fair-sched=yes (16/40), so it was neither slowness nor valgrind's scheduler. The new test runs the backend's strands on an inline executor, so the stopped handler settles inside request_stop() itself. With the old order it fails 50/50; with the fix it passes 50/50. Fixes #876 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WeDnphcJ7EZBLwVfE6pqZu --- docs/spec/concurrency_and_lifetimes.md | 6 +++++- include/morph/core/backend.hpp | 15 ++++++++----- tests/test_coroutine_model.cpp | 30 ++++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 6 deletions(-) diff --git a/docs/spec/concurrency_and_lifetimes.md b/docs/spec/concurrency_and_lifetimes.md index 290b42eff..272497090 100644 --- a/docs/spec/concurrency_and_lifetimes.md +++ b/docs/spec/concurrency_and_lifetimes.md @@ -350,7 +350,11 @@ returns a `core::async::Task` on `LocalBackend`, which carries a | The client-side execute deadline | `ClientTimeoutError` | That call | A stopped handler suspended in a stop-aware await resumes with -`OperationCancelled` on its model's strand. A synchronous handler has nothing +`OperationCancelled` on its model's strand. `LocalBackend::cancelPending` +settles every pending call before it requests any stop, because the stopped +handler settles its own call too, from another thread: were the stop +requested first, that `OperationCancelled` could reach the caller ahead of the +error the verb names. A synchronous handler has nothing to stop and runs to completion; its result is discarded by the settled state. A call on a remote backend is settled locally only — the wire carries no cancellation. `tests/test_coroutine_model.cpp` measures the scope's diff --git a/include/morph/core/backend.hpp b/include/morph/core/backend.hpp index 5d442904f..f0c89faca 100644 --- a/include/morph/core/backend.hpp +++ b/include/morph/core/backend.hpp @@ -1356,11 +1356,11 @@ class LocalBackend : public detail::IBackend { // With the swap: a run admitted before this point is in `snapshot` // and is failed below, and one admitted after it is not. _cancels->record(exc); - for (auto const& weak : _taskRuns) { - if (auto const run = weak.lock()) { - static_cast(run->stopSource->request_stop()); - } - } + // Settle before stopping. A stopped Task handler resumes on its strand + // and settles its own sink with `OperationCancelled`; requested first, + // that settle can land before this one, and the caller would be + // answered with the stop instead of @p exc. Settled first, the + // handler's later settle is the ignored one. for (auto& weak : snapshot) { if (auto sink = weak.lock()) { // May race a reply settling the same sink; `ISettleSink`'s @@ -1370,6 +1370,11 @@ class LocalBackend : public detail::IBackend { sink->settleException(exc); } } + for (auto const& weak : _taskRuns) { + if (auto const run = weak.lock()) { + static_cast(run->stopSource->request_stop()); + } + } } /// @brief Number of entries currently held in the pending list. diff --git a/tests/test_coroutine_model.cpp b/tests/test_coroutine_model.cpp index ee172a80f..d87233d10 100644 --- a/tests/test_coroutine_model.cpp +++ b/tests/test_coroutine_model.cpp @@ -856,6 +856,36 @@ TEST_CASE("cancelPending on a LocalBackend that stays alive stops its running Ta REQUIRE(probe().order == std::vector{"sleep-start", "sleep-cancelled"}); } +// The stopped handler settles its own call with `OperationCancelled`, from its +// strand. With the strand inline, that settle happens inside `request_stop()` +// itself, so this pins the order `cancelPending` must keep: the caller is +// answered with the error it was given, never with the stop it caused. +TEST_CASE("cancelPending answers a running Task handler's caller with its error and not with the stop", + "[coroutine][model][cancel]") { + coro_test::SchedulerScope const timers; + morph::testing::InlineExecutor pool; + morph::exec::MainThreadExecutor exec; + auto backend = std::make_unique(pool); + auto* local = backend.get(); + morph::bridge::Bridge bridge{std::move(backend), exec}; + morph::bridge::BridgeHandler handler{bridge, &exec}; + armHold(&exec); + + std::exception_ptr answered; + handler.execute(CoroSleep{.ms = 60'000}).then([](int) {}).onError([&](const std::exception_ptr& error) { + answered = error; + }); + REQUIRE(pumpUntil(exec, [&] { + std::scoped_lock const lock{probe().mtx}; + return !probe().order.empty(); + })); + + local->cancelPending(std::make_exception_ptr(morph::backend::DisconnectedError{})); + REQUIRE(pumpUntil(exec, [&] { return answered != nullptr && probe().holdFinished.load(); })); + CHECK(holds(answered)); + CHECK_FALSE(holds(answered)); +} + TEST_CASE("an action queued behind a suspended Task handler does not run once a backend switch failed it", "[coroutine][model][lifetime]") { coro_test::SchedulerScope const timers;