Repository navigation
core: LocalBackend::cancelPending settles its calls before stopping their handlers - #883
Merged
Merged
Conversation
…heir 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WeDnphcJ7EZBLwVfE6pqZu
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #876.
What the nightly failure actually was
The Valgrind job reported no memory errors on any of the failing nights:
ERROR SUMMARY: 0 errorsin every binary. It failed because Catch2 tests failed intests/test_coroutine_model.cpp:Each of these is a 2 s wait for a backend switch, or a
cancelPending, to give the caller a specific error type (BackendChangedErrororDisconnectedError).That error sometimes never arrived, because of an ordering race in
LocalBackend::cancelPending:A stopped handler resumes on its strand thread and settles its own call with
OperationCancelled. When that settle landed first, the caller gotOperationCancelled, and the later settle was a no-op.docs/spec/concurrency_and_lifetimes.mdsays these verbs settle the call with the given error, so this is a library defect, not a test-timing problem.The fix
docs/spec/concurrency_and_lifetimes.mdgains a sentence saying why the order matters.Evidence
Measured locally with clang 20.1.2 (Debug) and valgrind 3.22.0. Each run executed the three nightly-failing tests together, 40 runs, 4 at a time:
OperationCancelled(temporary instrumentation inSwitchedFlag, not committed)--fair-sched=yesNew regression test: "cancelPending answers a running Task handler's caller with its error and not with the stop". It runs the backend's strands on
morph::testing::InlineExecutor, so the stopped handler settles insiderequest_stop()itself. That makes the race deterministic:backend.hppreverted tomaster: fails 50/50,holds<DisconnectedError>(answered)is false and the caller gotOperationCancelledFull
morph_tests, natively, with the nightly job's filters (~[oom-injector] ~[issue108]): 1810 test cases, all pass. The one "failed as expected" is the existing[!shouldfail]replay-ledger control.Natively, without Valgrind, the old order never failed in 200 single-CPU runs. So this showed up only on the Valgrind leg, where threads are serialised.
Not verified locally:
🤖 Generated with Claude Code
https://claude.ai/code/session_01WeDnphcJ7EZBLwVfE6pqZu
Generated by Claude Code