future: Fix double callback execution

[Lorak-mmk]

ResolvableFuture::set_callback first set a callback under lock, released the lock, and then checked if result is set. If it is, then it invoked the callback.
The other path for callback calling is in future execution: when setting the result (under lock) new_from_future also reads a callback. If it exsits, then it is called.
the following race is possible:

• set_callback sets callback, releases lock
• new_from_future produces result, sees the callback, calls it
• set_callback checks for the result, sees it exists, calls the callback.

This results in double callback execution, potentially causing UB and other issues.
This commit fixes that: in set_callback, while still under lock, we check if callback should be called. Then outside of lock we call it if needed. It is important to call the callback outside the lock, to avoid various deadlocks.

Fixes: https://scylladb.atlassian.net/browse/CUSTOMER-409

Pre-review checklist

• I have split my patch into logically separate commits.
• All commit messages clearly explain what they change and why.
• PR description sums up the changes and reasons why they should be introduced.
• I have implemented Rust unit tests for the features/changes introduced. - no clue how to test that
• I have enabled appropriate tests in Makefile in {SCYLLA,CASSANDRA}_(NO_VALGRIND_)TEST_FILTER.
• I added appropriate Fixes: annotations to PR description.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR refactors ResolvableFuture::set_callback to move the result-availability check into the state-lock critical section. Previously, the method checked whether self.result.get().is_some() after releasing the lock to decide on immediate callback invocation. Now, the availability check is captured as should_invoke_callback while the lock is held, and that flag is used to make the invocation decision after the lock is released. This ensures the callback invocation decision is atomic with any concurrent state updates.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely summarizes the main change: fixing a double callback execution bug in the future implementation.
Description check	✅ Passed	The description thoroughly explains the race condition, the fix, and includes appropriate Fixes annotation, with pre-review checklist mostly completed.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

scylla-rust-wrapper/src/future.rs (1)

427-443: ⚡ Quick win

Add a regression test for exactly-once callback delivery.

This closes the race in the implementation, but the current callback tests only prove “called at least once” with a bool flag. A future regression to double execution would still pass. Please add a test that races cass_future_set_callback() against resolution and asserts the callback count stays at 1.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scylla-rust-wrapper/src/future.rs` around lines 427 - 443, Add a regression
test that races cass_future_set_callback() against resolving the future and
asserts the callback is invoked exactly once: create a Future (the type
exercised by cass_future_set_callback and the code paths around
self.state.lock() and bound_cb.invoke), spawn one thread that repeatedly calls
cass_future_set_callback() (or equivalent wrapper) while another thread
resolves/completes the future, use an atomic counter inside the callback rather
than a bool, run the race loop many times (or use joins/barriers to increase
collision likelihood), and assert the atomic counter equals 1 after each
iteration to catch any double-invocation regressions.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@scylla-rust-wrapper/src/future.rs`:
- Around line 427-443: Add a regression test that races
cass_future_set_callback() against resolving the future and asserts the callback
is invoked exactly once: create a Future (the type exercised by
cass_future_set_callback and the code paths around self.state.lock() and
bound_cb.invoke), spawn one thread that repeatedly calls
cass_future_set_callback() (or equivalent wrapper) while another thread
resolves/completes the future, use an atomic counter inside the callback rather
than a bool, run the race loop many times (or use joins/barriers to increase
collision likelihood), and assert the atomic counter equals 1 after each
iteration to catch any double-invocation regressions.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: f500f532-9134-4749-a476-da187fc54254

📥 Commits

Reviewing files that changed from the base of the PR and between 530d334 and f50548a.

📒 Files selected for processing (1)

• scylla-rust-wrapper/src/future.rs

[wprzytula]

Can you add a test that with high probability causes a race with the unfixed code?

[Lorak-mmk]

I would love to, but I don't really have any idea how to write such test.