feat: add retries to sql macros#2529
Closed
MasterPtato wants to merge 1 commit into06-03-fix_add_create_ts_to_server_listfrom
Closed
feat: add retries to sql macros#2529MasterPtato wants to merge 1 commit into06-03-fix_add_create_ts_to_server_listfrom
MasterPtato wants to merge 1 commit into06-03-fix_add_create_ts_to_server_listfrom
Conversation
This was referenced Jun 3, 2025
Closed
Contributor
Author
There was a problem hiding this comment.
PR Summary
Adds SQL query retry mechanism with exponential backoff and improves memory efficiency across the codebase.
- Added retry logic in
sql_query_macros.rswith 100ms base retry and max 16 retries - Removed
MaxSqlRetrieserror variant fromWorkflowErrorin favor of macro-level retry handling - Renamed
querytotxnin database drivers for better semantic clarity - Added reference operators (
&) to SQL query parameters across multiple services to prevent unnecessary cloning - Simplified transaction retry logic to use consistent backoff for all database errors
12 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile
Comment on lines
2001
to
2012
| if is_retrying.load(Ordering::Relaxed) { | ||
| self.query(|| async { | ||
| sql_execute!( | ||
| [self, &pool] | ||
| " | ||
| DELETE FROM workflow_signal_events | ||
| WHERE location = jsonb(?1) | ||
| ", | ||
| location, | ||
| ) | ||
| .await | ||
| }) | ||
| sql_execute!( | ||
| [self, &pool] | ||
| " | ||
| DELETE FROM workflow_signal_events | ||
| WHERE location = jsonb(?1) | ||
| ", | ||
| location, | ||
| ) | ||
| .await | ||
| .map_err(|x| fdb::FdbBindingError::CustomError(x.into()))?; | ||
| } |
There was a problem hiding this comment.
logic: Potential race condition: the DELETE operation should be in the same transaction as the INSERT to maintain atomicity
Comment on lines
+153
to
+158
| let mut backoff = $crate::__rivet_util::Backoff::new( | ||
| 4, | ||
| None, | ||
| $crate::utils::sql_query_macros::QUERY_RETRY_MS, | ||
| 50 | ||
| ); |
There was a problem hiding this comment.
logic: Backoff parameters are inconsistent - base=4 and factor=50 here vs QUERY_RETRY_MS=100 defined at the top. Consider aligning these values or documenting why they differ.
Comment on lines
+185
to
+186
| Database(_) | Io(_) | Tls(_) | Protocol(_) | PoolTimedOut | PoolClosed | ||
| | WorkerCrashed => { |
There was a problem hiding this comment.
style: Consider adding RowNotFound to non-retryable errors since retrying won't help if the row doesn't exist
Suggested change
| Database(_) | Io(_) | Tls(_) | Protocol(_) | PoolTimedOut | PoolClosed | |
| | WorkerCrashed => { | |
| Database(_) | Io(_) | Tls(_) | Protocol(_) | PoolTimedOut | PoolClosed | |
| | WorkerCrashed | RowNotFound => { |
Deploying rivet with
|
| Latest commit: |
c307626
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://758a1d78.rivet.pages.dev |
| Branch Preview URL: | https://06-03-feat-add-retries-to-sq.rivet.pages.dev |
200c987 to
b3eae4a
Compare
Deploying rivet-studio with
|
| Latest commit: |
c307626
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://bb342d1e.rivet-studio.pages.dev |
| Branch Preview URL: | https://06-03-feat-add-retries-to-sq.rivet-studio.pages.dev |
Deploying rivet-hub with
|
| Latest commit: |
c307626
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6498ef2a.rivet-hub-7jb.pages.dev |
| Branch Preview URL: | https://06-03-feat-add-retries-to-sq.rivet-hub-7jb.pages.dev |
This was referenced Jun 4, 2025
5e33d41 to
7666132
Compare
b3eae4a to
4fc5f51
Compare
This was referenced Jun 4, 2025
4fc5f51 to
406d6f9
Compare
7666132 to
20eace2
Compare
20eace2 to
b77a826
Compare
406d6f9 to
d44c0b9
Compare
This was referenced Jun 5, 2025
Closed
d44c0b9 to
c307626
Compare
b77a826 to
863f139
Compare
This was referenced Jun 6, 2025
Contributor
Merge activity
|
graphite-app Bot
pushed a commit
that referenced
this pull request
Jun 9, 2025
<!-- Please make sure there is an issue that this PR is correlated to. --> ## Changes <!-- If there are frontend changes, please include screenshots. -->
This was referenced Jun 9, 2025
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.

Changes