Repository navigation
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves the Python driver’s recovery behavior when an EXECUTE receives an UNPREPARED (PreparedQueryNotFound) error, allowing the driver to re-PREPARE and retry using either the returned query id (preferred) or the in-flight prepared statement context (fallback).
Changes:
- Update
ResponseFuture._set_result()to cache in-flightprepared_statement, resolve by returned UNPREPARED id when possible, and fall back to in-flight context when not. - Adjust logging for unknown-prepared and mismatch/fallback scenarios.
- Add unit tests covering fallback to local context and preference for returned-id cache resolution.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
cassandra/cluster.py |
Updates UNPREPARED handling in ResponseFuture._set_result() to improve reprepare/retry correctness across cache-miss and id-mismatch scenarios. |
tests/unit/test_response_future.py |
Adds unit coverage for the new UNPREPARED resolution and fallback behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
0f3737f to
8ed0715
Compare
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
3d3dc24 to
9815e0f
Compare
| @@ -4745,11 +4743,13 @@ def _set_result(self, host, connection, pool, response): | |||
| (current_keyspace, prepared_keyspace))) | |||
| return | |||
|
|
|||
| log.debug("Re-preparing unrecognized prepared statement against host %s: %s", | |||
| host, prepared_statement.query_string) | |||
| prepared_keyspace = prepared_statement.keyspace \ | |||
| log.debug( | |||
| "Re-preparing unrecognized prepared statement against host %s: %s", | |||
| host, self.prepared_statement.query_string | |||
| ) | |||
| prepared_keyspace = self.prepared_statement.keyspace \ | |||
| if ProtocolVersion.uses_keyspace_flag(self.session.cluster.protocol_version) else None | |||
| prepare_message = PrepareMessage(query=prepared_statement.query_string, | |||
| prepare_message = PrepareMessage(query=self.prepared_statement.query_string, | |||
There was a problem hiding this comment.
I'm looking at those changes and I don't see what you are trying to achieve.
In PR description you wrote:
Expected behavior
If the driver still has the original PreparedStatement context, it should:
- issue PREPARE,
- then retry the original EXECUTE, regardless of why the coordinator no longer recognizes the prepared id.
But this is what the driver already does?
- Cache the in-flight self.prepared_statement first (when present).
Why does it need to be done first?
In general the existing logic looks sound to me:
- In first step it determines id of preprared statement, trying both
self.prepared_statementandresponse.info, and also validating protocol assumptions. - Then it resolves prepared statement object, also trying both cluster-scoped cache (
self.session.cluster._prepared_statements) andself.prepared_statement. It also addsself.prepared_statementto cache in that case.
So, what problem is this PR trying to solve? In what scenario does existing logic fail?
There was a problem hiding this comment.
this is one of this scenarios scylladb/scylladb#27657
the driver should retry but it doesn't
There was a problem hiding this comment.
Linked issue describes the need for automatic reprepare. Driver already had mechanism for automatic repreparation - your PR doesn't add it, just modifies its logic. Which is why I'm asking what EXACTLY was wrong with the previous logic, how exactly it not working.
There was a problem hiding this comment.
Ideally we'd have a test that fails before the fix and passes after it. May not be easy to reproduce perhaps.
There was a problem hiding this comment.
Ideally we'd have a test that fails before the fix and passes after it.
Sure, but for now I'd be happy with a textual reasoning, so that I can understand the changes.
There was a problem hiding this comment.
@Lorak-mmk , here is the part of the mentioned discussion: scylladb/scylladb#27657 (comment)
If I understand correctly there was an experiment when Scylla returned UNPREPARED = 0x2500 sc to driver and it didnt repreapre the statement. So if driver already has mechanism for automatic repreparation it didnt work during the experiment - scylladb/scylladb#27657 (comment)
There was a problem hiding this comment.
Yes, I got that. What I don't understand from this PR description is why exactly it didn't work, and how those changes address that.
There was a problem hiding this comment.
Before this PR the logic works roughly like this:
If self.prepared_statement exists, the code forces the query_id to be the local one (self.prepared_statement.query_id).
It then asserts that this id matches the one returned by the server (response.info).
After that it tries to resolve the statement:
It looks it up in the cache using
cluster._prepared_statements[query_id].
If that lookup fails (KeyError), it falls back to the local prepared statement (self.prepared_statement) and stores it in the cache under that query_id.
now this is might be problematic because:
-
It ignores the server’s returned ID when a local statement exists
If we already have a local prepared statement, the code overwrites the ID with the local one before doing the lookup.
That means the ID returned by the server is effectively ignored in this path, even though it might be the correct one to use. -
The assertion can break recovery
The code asserts that the local ID and the server-returned ID must match.
However, in real situations (for example cache invalidation, server restart or crushes), the server might legitimately return a different ID.
When that happens, the assertion triggers and kills the flow before the driver has a chance to recover or re-prepare the statement gracefully. -
Cache updates may reinforce the wrong mapping
If the cache lookup fails, the code stores the local prepared statement back into the cache under the current query_id.
But in this branch the query_id is the local ID, not necessarily the one the server returned.
Because of this, the logic never explicitly tries the clean path of:
First resolve using the ID the server returned, and only fall back to the local statement if needed.
In some LWT tests we actually see this behavior, instead of re-sending its just returns does nothing and waits forever
There was a problem hiding this comment.
Yes, I got that. What I don't understand from this PR description is why exactly it didn't work, and how those changes address that.
why exactly it didn't work
This was actually a question for the drivers team :)
There was a problem hiding this comment.
This was actually a question for the drivers team :)
sadly almost 2 months ago :(
|
Dear @copilot (1) explain what is wrong in the current implementation and how this code change fixes the issue (2) can you come up with a test that fails before the change and passes after? |
@mykaul was my explanation bad ? :) #725 (comment) |
Honestly - #726 (comment) is not bad as an explanation. (What I'm still missing is how we can get to a situation of a different ID. That's not very clear to me, but I suspect it's a server-side issue?) |
yes exactly |
|
@calebxyz - please resolve review conversation with Copilot, and let's see what else remains here. |
done, its all in the code, just had to be resolved in chat. |
|
@mykaul how should we progress with this? |
|
@dkropachev - please schedule reviewing this or assign it for review after private link - we are seeing this hitting us in Scylla CI. If there's any open issues @calebxyz can fix already, please comment. |
|
|
||
| try: | ||
| prepared_statement = self.session.cluster._prepared_statements[query_id] | ||
| self.prepared_statement = self.session.cluster._prepared_statements[query_id] |
There was a problem hiding this comment.
[P2] Keep execution context separate from the statement being reprepared
This replaces execution-wide state with one attempt's missing statement. For an idempotent prepared batch with speculative execution, coordinator 1 can request A and coordinator 2 can request B before PREPARE(A) completes. The future then holds B, so the valid PREPARED(A) response raises an ID-mismatch error and writes A's result metadata into B. I reproduced this callback order: the PR head fails while the base handler does not.
It also affects the accepted unexpected-ID case: EXECUTE(A) with cached returned-ID B prepares B, then retries message A using B's decoding metadata. Preserve the original execution context and associate each PREPARE completion with its corresponding statement while retaining returned-ID-first lookup.
do we still see this problem after i have patched scylladb ? |
… for prepared statements.
### Problem
When the server returned UNPREPARED (PreparedQueryNotFound) during an EXECUTE, the driver could fail to recover in some valid cases (cache miss, race, coordinator restart, id mismatch edge cases), even when the in-flight request still had enough PreparedStatement context to recover.
### Expected behavior
If the driver still has the original PreparedStatement context, it should:
1. issue PREPARE,
2. then retry the original EXECUTE,
regardless of why the coordinator no longer recognizes the prepared id.
### What this PR changes
In ResponseFuture PreparedQueryNotFound handling:
1. Cache the in-flight self.prepared_statement first (when present).
2. Attempt lookup by the returned UNPREPARED id from cluster._prepared_statements.
3. Reprepare using the resolved statement.
4. If returned-id lookup fails:
- fallback to in-flight context if available,
- otherwise fail as unknown prepared statement.
This preserves correctness while improving recovery in real-world coordinator invalidation/race/restart scenarios.
### Tests
Added/updated unit coverage in tests/unit/test_response_future.py:
- validates reprepare path for PreparedQueryNotFound,
- validates fallback to in-flight context,
- validates preference for statement resolved by returned UNPREPARED id when available.
Fixes: scylladb/scylladb#27657
9815e0f to
d9db800
Compare
|
No actionable comments were generated in the recent review.
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 31.25% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 2 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description check The description explains the problem, expected behavior, implementation, tests, and linked issue. The checklist is completed; unchecked docstring and documentation items are consistent with the absenc… Title check The title clearly identifies the main change: fixing UNPREPARED handling in the Python driver execution path. Linked Issues check Issue #27657is closed. It provides historical context only. No active directly linked issue supplies coding requirements for this pull request.Out of Scope Changes check The reported changes are limited to PreparedQueryNotFound recovery in cassandra/cluster.py and focused unit tests in tests/unit/test_response_future.py. These changes match the pull request intent. Full details: Description check
-
Explanation
The description explains the problem, expected behavior, implementation, tests, and linked issue. The checklist is completed; unchecked docstring and documentation items are consistent with the absence of new public entities and documentation changes.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
new version: detached unprepared and execution flow also ID mismatch: after setting the exception it now returns. Before, it went on to write metadata and re-send the query. |
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can type 'qodo, fix this' on a finding and the fix lands right on your PR Powered by Qodo |
|
it was implemented long time ago, good that AI remembers what i have done here |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #725 +/- ##
==========================================
+ Coverage 86.72% 86.78% +0.05%
==========================================
Files 44 44
Lines 19029 19031 +2
Branches 2968 2969 +1
==========================================
+ Hits 16503 16516 +13
+ Misses 1858 1849 -9
+ Partials 668 666 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
test libev (3.15t) fails on master as well so it seems like a flaky |
Problem
When the server returned UNPREPARED (PreparedQueryNotFound) during an EXECUTE, the driver could fail to recover in some valid cases (cache miss, race, coordinator restart, id mismatch edge cases or crushes), even when the in-flight request still had enough PreparedStatement context to recover.
Expected behavior
If the driver still has the original PreparedStatement context, it should:
What this PR changes
In ResponseFuture PreparedQueryNotFound handling:
This preserves correctness while improving recovery in real-world coordinator invalidation/race/restart scenarios.
Tests
Added/updated unit coverage in tests/unit/test_response_future.py:
Fixes: scylladb/scylladb#27657
Pre-review checklist
./docs/source/.Fixes:annotations to PR description.