future: Add regression test for double callback#470
Merged
wprzytula merged 1 commit intoJun 2, 2026
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds a regression test Sequence Diagram(s)sequenceDiagram
participant TestThread as TestThread
participant Tokio as TokioRuntime
participant Worker as spawn_blocking
participant CassFuture as CassFuture
participant Callback as counting_cb
TestThread->>Tokio: create multi-threaded runtime
TestThread->>Tokio: submit iteration task
Tokio->>Worker: spawn_blocking (create & resolve future with Barrier)
Worker->>CassFuture: resolve future (after Barrier)
TestThread->>CassFuture: cass_future_set_callback(counting_cb)
CassFuture->>Callback: invoke counting_cb (once)
TestThread->>CassFuture: cass_future_wait()
Possibly related PRs
Suggested labelsP1, area/Driver_-_cpp-rs-driver 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
coderabbitai
Bot
added
P1
There was a problem hiding this comment.
Pull request overview
Adds a Rust unit regression test to guard against the previously fixed race where a CassFuture callback could be invoked twice when cass_future_set_callback races with future resolution (refs #468 / issue #468).
Changes:
- Introduces a new multi-threaded, contention-based test that attempts to race
set_callbackagainst resolution and asserts the callback counter never exceeds 1. - Uses a barrier + blocking task to widen the race window and repeats the scenario many times to increase the chance of exercising the interleaving.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// (but not yet acquired the state lock to store it) by the time | ||
| /// `set_callback` is called, creating lock contention. | ||
| #[test] | ||
| #[ntest::timeout(30000)] |
| .into(), | ||
| ); | ||
|
|
||
| for _ in 0..10_000 { |
Comment on lines
+911
to
+913
| /// To trigger this, we ensure the async task has already produced its result | ||
| /// (but not yet acquired the state lock to store it) by the time | ||
| /// `set_callback` is called, creating lock contention. |
| unsafe { cass_future_wait(cass_fut.borrow()) }; | ||
|
|
||
| // Spin-wait for the callback to be invoked at least once. | ||
| let deadline = std::time::Instant::now() + Duration::from_millis(100); |
Add a probabilistic test that verifies the callback is invoked exactly once when set_callback races with future resolution. The test creates a future whose resolution is synchronized via a barrier, then calls set_callback at the same time, checking that the atomic callback counter never exceeds 1. The natural race window is unfortunately too narrow to hit reliably without instrumentation. Manual testing on my machine showed the bug in ~20-40% runs, so this is enough to be hit in the CI from time to time.
wprzytula
force-pushed
the
double-callback-bug-regression-test
branch
from
June 2, 2026 10:29
872285d to
86331f3
Compare
Lorak-mmk
approved these changes
Jun 2, 2026
Merged
wprzytula
added a commit
that referenced
this pull request
Jun 2, 2026
Refs: #468 Add a probabilistic test (authored by Claude Opus 4.6) that verifies the callback is invoked exactly once when set_callback races with future resolution. The test creates a future whose resolution is synchronized via a barrier, then calls set_callback at the same time, checking that the atomic callback counter never exceeds 1. The natural race window is unfortunately too narrow to hit reliably without instrumentation. Manual testing on my machine showed the bug in ~20-40% runs, so this is enough to be hit in the CI from time to time. (cherry picked from commit ab2ce69)
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.
Refs: #468
Add a probabilistic test (authored by Claude Opus 4.6) that verifies the callback is invoked exactly once when set_callback races with future resolution. The test creates a future whose resolution is synchronized via a barrier, then calls set_callback at the same time, checking that the atomic callback counter never exceeds 1.
The natural race window is unfortunately too narrow to hit reliably without instrumentation. Manual testing on my machine showed the bug in ~20-40% runs, so this is enough to be hit in the CI from time to time.
Pre-review checklist
[ ] I have enabled appropriate tests inMakefilein{SCYLLA,CASSANDRA}_(NO_VALGRIND_)TEST_FILTER.[ ] I added appropriateFixes:annotations to PR description.