fix: make query_as macros retry #2773
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Claude finished @MasterPtato's task —— View job PR Review: Make query_as macros retryReviewing this PR for code quality, potential bugs, performance, security, and test coverage. Todo List:
Review SummaryThis PR adds retry functionality to SQL query macros, improving reliability when dealing with transient database errors. The implementation adds retry logic to both Code Quality & Best Practices ✅Positive aspects:
Suggestions for improvement:
Potential Bugs & Issues
|
Deploying rivet with
|
| Latest commit: |
f64697c
|
| Status: | 🚫 Build failed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/common/pools/src/utils/sql_query_macros.rs (1)
182-192: Consider additional retryable error types and reducing code duplication.The retry error handling looks good, but consider:
- Should
Configurationerrors be retryable in some cases?- Should
AnyDriverErrorbe examined to determine if the underlying error is retryable?- The retry logic is duplicated across 4 different macro variants. Consider extracting this into a helper macro to reduce duplication and ease maintenance.
Example of extracting retry logic:
+#[macro_export] +macro_rules! __sql_retry_loop { + ($query_builder:expr, $conn:expr, $max_retries:expr, $backoff:expr) => {{ + let mut i = 0; + loop { + match $query_builder.await { + Err(err) => { + i += 1; + if i > $max_retries { + break Err( + sqlx::Error::Io( + std::io::Error::new( + std::io::ErrorKind::Other, + $crate::utils::sql_query_macros::Error::MaxSqlRetries(err), + ) + ) + ); + } + + use sqlx::Error::*; + match &err { + Database(_) | Io(_) | Tls(_) | Protocol(_) | PoolTimedOut | PoolClosed + | WorkerCrashed => { + tracing::warn!(?err, "query retry ({i}/{})", $max_retries); + $backoff.tick().await; + } + _ => break Err(err), + } + } + x => break x, + } + } + }}; +}Also applies to: 239-249, 309-319, 366-376
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
packages/common/chirp-workflow/core/src/db/crdb_nats/debug.rs(2 hunks)packages/common/pools/src/utils/sql_query_macros.rs(2 hunks)packages/core/services/build/ops/get/src/lib.rs(1 hunks)packages/core/services/build/src/ops/patch_tags.rs(2 hunks)packages/core/services/cdn/ops/site-get/src/lib.rs(1 hunks)packages/core/services/cf-custom-hostname/ops/get/src/lib.rs(1 hunks)packages/core/services/cloud/ops/game-config-get/src/lib.rs(1 hunks)packages/core/services/cloud/ops/namespace-get/src/lib.rs(1 hunks)packages/core/services/cloud/ops/version-get/src/lib.rs(1 hunks)packages/core/services/cloud/worker/src/workers/version_name_reserve.rs(1 hunks)packages/core/services/cluster/src/ops/datacenter/get.rs(1 hunks)packages/core/services/cluster/src/ops/server/lost_list.rs(1 hunks)packages/core/services/cluster/src/ops/server/prune_with_filter.rs(1 hunks)packages/core/services/cluster/src/workflows/datacenter/mod.rs(1 hunks)packages/core/services/cluster/src/workflows/server/mod.rs(1 hunks)packages/core/services/game/ops/get/src/lib.rs(1 hunks)packages/core/services/game/ops/namespace-resolve-name-id/src/lib.rs(1 hunks)packages/core/services/game/ops/version-get/src/lib.rs(1 hunks)packages/core/services/linode/standalone/gc/src/lib.rs(1 hunks)packages/core/services/load-test/standalone/sqlx/src/lib.rs(1 hunks)packages/core/services/mm-config/ops/lobby-group-get/src/lib.rs(1 hunks)packages/core/services/mm-config/ops/lobby-group-resolve-version/src/lib.rs(1 hunks)packages/core/services/mm-config/ops/namespace-get/src/lib.rs(1 hunks)packages/core/services/mm/ops/lobby-for-run-id/src/lib.rs(1 hunks)packages/core/services/mm/ops/lobby-get/src/lib.rs(1 hunks)packages/core/services/mm/ops/player-get/src/lib.rs(1 hunks)packages/core/services/route/src/ops/upsert.rs(2 hunks)packages/core/services/team/ops/get/src/lib.rs(1 hunks)packages/core/services/team/ops/member-get/src/lib.rs(1 hunks)packages/core/services/upload/ops/get/src/lib.rs(1 hunks)packages/core/services/user/ops/get/src/lib.rs(1 hunks)packages/core/services/user/ops/profile-validate/src/lib.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/core/services/game/ops/version-get/src/lib.rs (1)
packages/core/services/load-test/standalone/sqlx/src/lib.rs (1)
version_ids(97-101)
packages/core/services/mm-config/ops/namespace-get/src/lib.rs (1)
packages/core/services/load-test/standalone/sqlx/src/lib.rs (1)
namespace_ids(77-81)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cargo-check-main
- GitHub Check: claude-review
🔇 Additional comments (32)
packages/core/services/cloud/worker/src/workers/version_name_reserve.rs (1)
35-35: LGTM: Parameter passing updated to support retry functionality.The change from
date_prefixto&date_prefixis correct and aligns with the systematic refactoring to enable retry functionality for SQL query macros. Passing parameters by reference ensures they remain available for multiple query attempts.packages/core/services/build/ops/get/src/lib.rs (1)
46-46: LGTM: Consistent parameter passing update.The change from
build_idsto&build_idsfollows the consistent pattern across the codebase to support retry functionality. This ensures the vector remains available for multiple query attempts.packages/core/services/mm/ops/lobby-for-run-id/src/lib.rs (1)
36-36: LGTM: Parameter reference update aligns with retry functionality.The modification from
run_idsto&run_idsmaintains consistency with the codebase-wide refactoring to enable retry functionality for SQL query macros.packages/core/services/team/ops/member-get/src/lib.rs (1)
40-40: LGTM: Serialization parameter updated for retry support.The change from
serde_json::to_value(members)toserde_json::to_value(&members)is correct and consistent with the broader refactoring. Passing the vector by reference ensures it remains available for potential retry attempts.packages/core/services/mm-config/ops/namespace-get/src/lib.rs (1)
39-39: LGTM: Consistent parameter passing for retry functionality.The change from
namespace_idsto&namespace_idsfollows the established pattern across the codebase to support retry functionality in SQL query macros.packages/core/services/mm-config/ops/lobby-group-resolve-version/src/lib.rs (1)
45-45: Good optimization: pass vector by referenceThis change improves performance by avoiding unnecessary cloning of the
lobby_group_idsvector when passing it to the SQL query macro. The use of&lobby_group_idsfollows Rust best practices and is consistent with the broader pattern applied across the codebase.packages/core/services/cloud/ops/game-config-get/src/lib.rs (1)
35-35: Consistent optimization: pass vector by referenceThis change mirrors the same optimization pattern seen across the codebase, passing
&game_idsby reference to avoid unnecessary vector cloning. This is a performance improvement that follows Rust best practices.packages/core/services/user/ops/profile-validate/src/lib.rs (1)
79-79: Consistent optimization: pass string by referenceThis change extends the optimization pattern to string parameters, passing
&display_nameby reference to avoid unnecessary string cloning. This is efficient and consistent with the broader refactoring to standardize parameter passing to SQL macros.packages/core/services/cf-custom-hostname/ops/get/src/lib.rs (1)
39-39: Consistent vector optimizationThis change continues the systematic optimization pattern, passing
&identifiersby reference to avoid unnecessary vector cloning. The change is efficient and consistent with the broader refactoring effort.packages/core/services/game/ops/version-get/src/lib.rs (1)
46-46: Final vector optimization in the patternThis change completes the systematic optimization pattern seen across all reviewed files, passing
&version_idsby reference to avoid unnecessary vector cloning. The change is efficient and maintains consistency with the broader refactoring effort.packages/core/services/cdn/ops/site-get/src/lib.rs (1)
30-30: LGTM: Efficient parameter passing.Good change to pass the vector by reference instead of by value, avoiding unnecessary moves while maintaining the same functionality.
packages/core/services/mm-config/ops/lobby-group-get/src/lib.rs (1)
36-36: LGTM: Consistent parameter passing improvement.The change to pass
lobby_group_idsby reference aligns with the broader refactoring pattern and avoids unnecessary vector moves.packages/core/services/cluster/src/ops/server/lost_list.rs (1)
171-171: LGTM: Consistent with refactoring pattern.The change to pass
server_idsby reference is consistent with the broader codebase refactoring and avoids unnecessary vector moves.packages/core/services/mm/ops/player-get/src/lib.rs (1)
46-46: LGTM: Consistent parameter passing optimization.The change to pass
player_idsby reference follows the established pattern and improves efficiency by avoiding unnecessary vector moves.packages/core/services/cloud/ops/namespace-get/src/lib.rs (1)
27-27: LGTM: Completes the consistent refactoring pattern.The change to pass
namespace_idsby reference is consistent with the broader codebase refactoring and provides the same efficiency benefits as the other files.packages/core/services/mm/ops/lobby-get/src/lib.rs (1)
86-86: Good performance optimization by passing vector by referenceThis change avoids unnecessary cloning of the
lobby_idsvector when passing it to the SQL query. TheANY($1)clause works correctly with both owned values and references, making this a safe and beneficial optimization.packages/core/services/team/ops/get/src/lib.rs (1)
41-41: Consistent optimization pattern - passing vector by referenceThis change aligns with the broader pattern in this PR to pass vectors by reference to SQL queries, avoiding unnecessary cloning and improving performance.
packages/core/services/user/ops/get/src/lib.rs (1)
69-69: Performance optimization in cache contextThis change is particularly beneficial since it's within a cache closure where avoiding unnecessary vector cloning can have a more significant performance impact.
packages/core/services/upload/ops/get/src/lib.rs (1)
63-63: Consistent optimization pattern appliedThis change follows the same beneficial pattern of passing vectors by reference to SQL queries, avoiding unnecessary cloning while maintaining the same functionality.
packages/core/services/cluster/src/ops/datacenter/get.rs (1)
105-105: Consistent optimization pattern with function parameterThis change maintains the same beneficial pattern of passing vectors by reference to SQL queries. The optimization works equally well whether the vector is locally created or passed as a function parameter.
packages/core/services/game/ops/get/src/lib.rs (1)
71-71: LGTM: Efficient parameter passing.Passing the vector by reference to the SQL query is more efficient and avoids unnecessary moves, especially since
game_idsis used later in the function.packages/core/services/cloud/ops/version-get/src/lib.rs (1)
27-27: LGTM: Consistent parameter passing improvement.The change to pass the vector by reference aligns with the codebase-wide refactoring and improves efficiency by avoiding unnecessary moves.
packages/core/services/cluster/src/workflows/server/mod.rs (1)
600-600: LGTM: Consistent with codebase refactoring.Passing the vector by reference is the correct approach and aligns with the systematic parameter passing improvements across the codebase.
packages/core/services/cluster/src/ops/server/prune_with_filter.rs (1)
130-130: LGTM: Efficient parameter passing.The change to pass the datacenter IDs vector by reference is consistent with the codebase-wide refactoring and improves efficiency.
packages/core/services/game/ops/namespace-resolve-name-id/src/lib.rs (1)
33-33: LGTM: Consistent parameter handling.The modification to extract and collect name_ids while maintaining the reference passing pattern is consistent with the codebase-wide refactoring for SQL query parameters.
packages/core/services/route/src/ops/upsert.rs (1)
114-114: LGTM! Parameter passing optimization for SQL macros.The changes correctly pass the JSON parameter by reference instead of by value, which improves efficiency by avoiding unnecessary clones. This aligns with the broader pattern across the codebase to prepare for the enhanced SQL macros with retry functionality.
Also applies to: 143-143
packages/core/services/build/src/ops/patch_tags.rs (1)
136-136: LGTM! Consistent parameter passing optimization.Both changes correctly pass JSON parameters by reference to the SQL macros, following the established pattern across the codebase. This improves efficiency and prepares the code for the enhanced SQL macros with retry functionality.
Also applies to: 150-150
packages/core/services/load-test/standalone/sqlx/src/lib.rs (1)
107-107: LGTM! Vector parameter optimization.The change correctly passes the
user_idsvector by reference instead of by value, which is more efficient and follows the consistent pattern across the codebase for SQL macro parameter passing.packages/core/services/linode/standalone/gc/src/lib.rs (1)
109-109: LGTM! Efficient vector parameter passing.The change correctly passes the
image_idsvector by reference, avoiding unnecessary cloning and following the consistent pattern established across the codebase for SQL macro parameter passing.packages/common/chirp-workflow/core/src/db/crdb_nats/debug.rs (1)
52-52: LGTM! Consistent vector parameter optimization.Both changes correctly pass UUID vectors by reference to the SQL macros, following the established pattern across the codebase. This improves efficiency by avoiding unnecessary vector cloning, which is particularly beneficial for debugging operations that may handle large numbers of workflow and signal IDs.
Also applies to: 496-496
packages/core/services/cluster/src/workflows/datacenter/mod.rs (1)
231-237: LGTM! Correct parameter passing for retry mechanism.The change to pass
pools_buf,gph_dns_parent, andgph_staticby reference is appropriate and necessary for the retry mechanism. Since the SQL macros now recreate queries and rebind parameters on each retry attempt, passing by reference prevents moving the values and allows multiple retries.packages/common/pools/src/utils/sql_query_macros.rs (1)
420-424: No Action Needed forsql_fetchMacroThe
sql_fetchmacro (wrapping__sql_query_as_raw) currently has no usages in the repository, so its lack of retry logic does not introduce any inconsistency with other query macros. If in future you expose streaming or cursor-based fetches viasql_fetch, you can revisit adding retry support at that time.
| 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.
💡 Verification agent
🧩 Analysis chain
Verify the Backoff initialization parameters.
The Backoff is initialized with parameters (4, None, QUERY_RETRY_MS, 50) in multiple places. The first parameter 4 is unclear - please verify what this represents in the __rivet_util::Backoff::new() constructor and ensure it's appropriate for SQL query retries.
Also applies to: 280-285, 337-342
🏁 Script executed:
#!/bin/bash
# Search for the Backoff struct definition to understand the parameters
ast-grep --pattern 'impl Backoff {
$$$
fn new($$$) {
$$$
}
$$$
}'
# Also search for any documentation on Backoff
rg -A 10 "struct Backoff" --type rustLength of output: 738
🏁 Script executed:
#!/bin/bash
# Show the Backoff::new signature and docs
rg -n -A5 -B2 "pub fn new" packages/common/util/core/src/lib.rs
# Show the implementation of next() to understand retry behavior
rg -n -A5 -B3 "fn next" packages/common/util/core/src/lib.rsLength of output: 435
Clarify Backoff::new parameters
The call
Backoff::new(4, None, QUERY_RETRY_MS, 50)maps to
pub fn new(
max_exponent: usize, // first arg
max_retries: Option<usize>,
wait: usize, // base wait in ms
randomness: usize, // max jitter in ms
)max_exponent = 4⇒ delays =wait * 2^nup to2⁴→ 100 ms → 1 600 ms (± jitter)max_retries = None⇒ unbounded retry attemptswait = QUERY_RETRY_MS(100 ms) andrandomness = 50ms jitter
Please confirm that:
- A maximum exponent of 4 (1.6 s cap) is sufficient.
- Unlimited retries are desired. Otherwise, pass
Some(MAX_QUERY_RETRIES)instead ofNoneto bound attempts.
Applies at:
- packages/common/pools/src/utils/sql_query_macros.rs:210–215
- packages/common/pools/src/utils/sql_query_macros.rs:280–285
- packages/common/pools/src/utils/sql_query_macros.rs:337–342
🤖 Prompt for AI Agents
In packages/common/pools/src/utils/sql_query_macros.rs around lines 210 to 215,
review the Backoff::new call parameters to ensure they match desired retry
behavior. Confirm if a max_exponent of 4 (capping delay at about 1.6 seconds) is
sufficient for your use case and whether unlimited retries (max_retries set to
None) are intended. If you want to limit retry attempts, replace None with
Some(MAX_QUERY_RETRIES) to bound the number of retries. Apply the same
confirmation and changes as needed at lines 280–285 and 337–342.
MasterPtato
force-pushed
the
07-18-fix_make_query_as_macros_retry
branch
from
6bc330c to
2d84554
Compare
Deploying rivet-studio with
|
| Latest commit: |
f64697c
|
| Status: | 🚫 Build failed. |
|
Claude finished @MasterPtato's task —— View job PR Review: Make query_as macros retryReviewing this PR for code quality, potential bugs, performance, security, and test coverage. Todo List:
Review SummaryThis PR adds retry functionality to SQL query macros, improving reliability when dealing with transient database errors. The implementation adds retry logic to both Code Quality & Best Practices ✅Positive aspects:
Suggestions for improvement:
Potential Bugs & Issues
|
Deploying rivet-hub with
|
| Latest commit: |
f64697c
|
| Status: | 🚫 Deploy failed. |
MasterPtato
force-pushed
the
07-18-fix_make_query_as_macros_retry
branch
from
2d84554 to
f64697c
Compare
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
Changes
Summary by CodeRabbit
Bug Fixes
Chores