chore(engine): tweak platform limits #3824
Conversation
|
🚅 Deployed to the rivet-pr-3824 environment in rivet-frontend
|
railway-app
bot
temporarily deployed
to
rivet-frontend / rivet-pr-3824
How to use the Graphite Merge QueueAdd the label merge-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
PR Review: Platform Limits ConfigurationSummaryThis PR introduces comprehensive configuration for platform limits, making previously hardcoded timeouts, message sizes, and rate limits configurable. The changes improve flexibility for self-hosted deployments and properly document all limits. ✅ Strengths1. Excellent Documentation Sync
2. Comprehensive Configuration Structure
3. Good Error Handling
4. Clean Code Organization
🔍 Issues & RecommendationsCritical Issues1. Inconsistent Default Value: // packages/config/src/config/pegboard.rs:219
pub fn gateway_response_start_timeout_ms(&self) -> u64 {
self.gateway_response_start_timeout_ms
.unwrap_or(5 * 60 * 1000) // 5 minutes
}// website/src/content/docs/self-hosting/configuration.mdx:100
gateway_response_start_timeout_ms?: number; // Default: 15000 (ms)Issue: The code defaults to 5 minutes (300,000ms) but documentation says 15 seconds (15,000ms). This is a 20x difference! Impact: Users relying on documentation will be surprised by actual behavior. Recommendation: Update documentation to reflect the actual 5 minute default, or change the code if 15 seconds was intended. 2. Missing Fallback for WebSocket Message Size in Error Proxy Path // packages/guard-core/src/proxy_service.rs:329
websocket_max_message_size: usize,The comment says "Fallback WebSocket max message size (used for error proxy path)" but this fallback is stored but never actually used in the codebase. The error proxy path doesn't appear to use this value. Recommendation: Either use this value in the error proxy path or remove it if not needed. 3. Configuration Documentation Incomplete The
Recommendation: Add all configurable options to the documentation for completeness. Minor Issues4. Duplicate Error Definition // engine/packages/guard-core/src/errors.rs:114
#[derive(RivetError, Serialize, Deserialize)]
#[error("guard", "request_body_too_large", ...)]
pub struct RequestBodyTooLarge { ... }
// engine/packages/pegboard-gateway/src/lib.rs:47
#[derive(RivetError, Serialize, Deserialize)]
#[error("guard", "request_body_too_large", ...)]
pub struct RequestBodyTooLarge { ... }Issue: Same error is defined in two places. This could lead to inconsistencies if one is updated but not the other. Recommendation: Keep only one definition (in guard-core/src/errors.rs) and import it where needed. 5. Comment Style Issues Per CLAUDE.md guidelines, comments should be complete sentences without fragmented structures: // packages/config/src/config/guard.rs:14
/// Route cache TTL in milliseconds.
Some doc comments are sentence fragments. While these are acceptable for field documentation, ensure consistency. 6. Response Body Streaming Comment Inconsistency // packages/guard-core/src/proxy_service.rs:1003-1004
// Stream response through without buffering
// Response body size limits are enforced in pegboard-runnerThe code removed the buffering logic entirely, which is good. However, it would be helpful to have a comment explaining why we always stream (to support large responses, SSE, etc.) rather than just stating the fact. Recommendation: Expand the comment to explain the rationale: "Always stream responses to support large payloads, SSE, and chunked transfer encoding. Response body size limits are enforced in pegboard-runner.". 7. Middleware Timeout Handling // packages/guard-core/src/proxy_service.rs:501-503
Err(_) => {
// Fallback to config-based defaults if middleware times out
tracing::warn!("middleware timed out, using fallback config");Issue: The timeout error is discarded. This could hide issues with middleware performance. Recommendation: Log more details about the timeout, including the actor_id: 8. WebSocket Config Creation // packages/guard-core/src/proxy_service.rs:372-374
fn client_websocket_config(max_message_size: usize) -> WebSocketConfig {
WebSocketConfig::default().max_message_size(Some(max_message_size))
}Issue: The method name suggests it's only for client connections, but WebSocketConfig has separate settings for incoming vs outgoing messages. This method only sets Recommendation: Either rename to 9. Type Conversion Safety Multiple places use self.gateway_hws_max_pending_size.unwrap_or(128 * 1024 * 1024) // 128 MiBThese are fine, but consider using constants for common sizes to avoid magic numbers: const MIB: u64 = 1024 * 1024;
const GIB: u64 = 1024 * MIB;
self.gateway_hws_max_pending_size.unwrap_or(128 * MIB)🎯 Testing Recommendations
📊 Performance Considerations✅ Good: Using Moka cache with proper TTLs for route and proxy state caching 🔒 Security Considerations✅ Good: Rate limiting is configurable and has sensible defaults 📝 SummaryThis is a well-structured PR that makes the platform more flexible and better documented. The main issues are:
The code follows Rivet conventions well and the changes are backward compatible (all new config is optional with defaults). Checklist
Recommendation: Request changes to fix the critical default value mismatch and complete the documentation before merging. |
More templates
@rivetkit/cloudflare-workers
@rivetkit/db
@rivetkit/framework-base
@rivetkit/next-js
@rivetkit/react
rivetkit
@rivetkit/sql-loader
@rivetkit/virtual-websocket
@rivetkit/engine-runner
@rivetkit/engine-runner-protocol
commit: |
NathanFlurry
force-pushed
the
01-10-chore_engine_tweak_platform_limits
branch
from
f2db255 to
caa66ae
Compare
No description provided.