CASSGO-133 Cap RequestErrUnprepared retry recursion on query and batch paths - #1952
CASSGO-133 Cap RequestErrUnprepared retry recursion on query and batch paths#1952arloliu wants to merge 1 commit into
Conversation
Conn.executeQuery and Conn.executeBatch both responded to a
*RequestErrUnprepared by evicting the prepared-statement cache entry
and recursing on themselves with no upper bound. When the server
persistently re-reports the same statement as unprepared after
re-prepare (a coordinator thrashing its prepared-statement cache,
or a misbehaving proxy/fork), the recursion never terminates. The
goroutine stack eventually exceeds runtime.SetMaxStack (1 GiB by
default), at which point Go runtime.throw crashes the entire process
with an unrecoverable stack-overflow — no recover() can intercept it.
The failure mode is reachable from any prepared-statement workload;
the batch path is on the hot write path.
Cap recursion on both paths at maxUnprepRetries = 5 by threading an
unprepAttempt counter through unexported helpers
executeQueryWithUnprepRetries and executeBatchWithUnprepRetries. When
the cap fires, return an Iter whose err is
fmt.Errorf("...after N re-prepare attempts: %w", serverErr)
The %w wrap means callers can:
- Detect cap-driven failures via error message pattern.
- Recover the underlying *RequestErrUnprepared with errors.As to
inspect the StatementId the server kept rejecting.
Behavior is a strict superset of the prior code: queries and batches
that succeed within 5 attempts behave exactly as before. Only the
pathological no-progress case is changed.
Test fake-server (conn_test.go) gains an always-unprep opPrepare
case returning id=99, an opBatch arm that replies ErrCodeUnprepared
when any statement carries id=99, and case-insensitive verb-trimming
in the opPrepare query-name parser (select/insert/update/delete) so
DML in batches can reach the always-unprep case. Two new tests in
unprep_retry_test.go drive the always-unprep path through Query.Exec
and Session.ExecuteBatch respectively, assert no infinite recursion,
verify the wrap is recoverable via errors.As, and check that the
server received exactly maxUnprepRetries+1 prepare/execute (resp.
prepare/batch) pairs.
Patch by Arlo Liu
worryg0d
left a comment
worryg0d
left a comment
There was a problem hiding this comment.
Hi @arloliu, thanks for the patch.
I agree that this type of error should not cause a client application crash, so it's better to return an error instead of recursively re-executing the query on the conn. Also, returning an error not only prevents the process from crashing due to stack overflow but also enables RetryPolicy to decide whether it's worth retrying on this host or choosing another.
What I'm thinking of if recursive calls are even needed since it is already governed by retry policy.
| // Pathological re-prepare loop (server-side cache evicting | ||
| // our prepared statement between every attempt). Bail with | ||
| // the underlying server error rather than recursing | ||
| // indefinitely. | ||
| iter.err = fmt.Errorf("gocql: failed to execute prepared statement after %d re-prepare attempts: %w", | ||
| unprepAttempt+1, x) | ||
| return iter |
There was a problem hiding this comment.
Previously, RequestErrUnprepared was never returned to the users, but this patch changes that, so I'm concerned about inconsistencies in error wrapping here. Similar error types are just returned as is without any wrapping, so it is possible to type-assert for the desired error type:
err := gocql.Query("...").ExecContext(ctx)
if err != nil {
if syntaxErr, ok := err.(*RequestErrSyntax); ok {
...
}
}However, having this error wrapped makes type-assertion for *RequestErrUnprepared impossible, but it still would work with errors.As, so I'm unsure if this is problematic or not for the users
|
I created CASSGO-133 for this |
worryg0d
changed the title
|
Is this something that you actually ran into in the real world? No other CQL driver has a cap on the number of prepare retries and I don't think I've ever seen someone report an issue with this. Even if the application is using prepared statements in the worst way possible the prepare should eventually succeed. |
|
There is one thing that is different about gocql and it's the fact that the unprepared retries happen in a recursive manner which can lead to the stack growing slowly over time until that request eventually succeeds. I think that surfacing the unprepared error to the retry policy is not the right approach. The prepare flow is something that should be internal to the driver, it's mostly a protocol thing and the user in theory doesn't even know or care about prepare semantics. I'm not opposed to capping the number of prepare retries even though this has never been needed on other drivers but this should be an internal driver thing and the retry policy should not be involved in my opinion. |
You're right, gocql implicitly prepares queries, so it makes sense. Interesting that RequestErrUnprepared is never returned to the users, but the type is exposed. Then what we can do is handle it in the preparing goroutine in Conn.prepareStatement method, and return this error when it hits the cap |
3 participants
Summary
Conn.executeQueryandConn.executeBatchboth respond to a*RequestErrUnpreparedby evicting the prepared-statement cache entry and recursing on themselves with no upper bound.When the server persistently re-reports the same statement as unprepared after re-prepare — for example a coordinator thrashing its prepared-statement cache under high statement cardinality, or a misbehaving proxy/fork — the recursion never terminates. The goroutine stack eventually exceeds
runtime.SetMaxStack(1 GiB by default), at which point Go's runtime crashes the entire process with an unrecoverable stack-overflow throw. Norecover()can intercept it.The failure mode is reachable from any prepared-statement workload; the batch path is on the hot write path. No attacker is required — organic Cassandra prep-cache thrash is sufficient.
This PR caps re-prepare retries on both paths at 5 and surfaces a descriptive error wrapping the underlying
*RequestErrUnpreparedsoerrors.Ascontinues to work.Approach
executeQueryWithUnprepRetries/executeBatchWithUnprepRetriestake anunprepAttempt intand increment it on each retry.executeQuery,executeBatch) become thin wrappers that start the counter at 0.maxUnprepRetries = 5, the iter is returned witherr = fmt.Errorf("…after N re-prepare attempts: %w", serverErr).Tests
Test fake-server (
conn_test.go) gains:always-unprepopPreparecase returningid=99,opBatcharm that repliesErrCodeUnpreparedwhen any statement carriesid=99,opPreparequery-name parser (select|insert|update|delete) so DML in batches can reach thealways-unprepcase.unprep_retry_test.go(new):TestExecuteQuery_UnprepRetryIsCappeddrives the always-unprep path throughQuery.Exec.TestExecuteBatch_UnprepRetryIsCappeddrives it throughSession.ExecuteBatch."re-prepare attempts",errors.As(err, &*RequestErrUnprepared)succeeds, and the server received exactlymaxUnprepRetries + 1prepare/execute (resp. prepare/batch) pairs.Test plan
make checkcleanmake test-unitgreen (both new tests pass)Notes
No CASSGO ticket attached; happy to file one and amend the commit / CHANGELOG entry if the committer prefers.