chore(pegboard-gateway): add new message id format & add deprecated tunnel ack #3492
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
NathanFlurry
force-pushed
the
11-19-chore_pegboard-gateway_add_new_message_id_format_add_deprecated_tunnel_ack
branch
from
c020fe1 to
e47241f
Compare
Code ReviewThis PR introduces a significant architectural improvement to the tunnel messaging system by implementing a more efficient message ID format and removing the need for explicit tunnel acknowledgments. Overall, the changes are well-structured and maintain backward compatibility. Here are my findings: ✅ Strengths
|
Code ReviewI've reviewed this PR which adds a new message ID format and deprecates the old tunnel ack mechanism. Here's my feedback: Positive Aspects✅ Well-structured message ID system: The new ✅ Backwards compatibility: The PR thoughtfully handles protocol versioning with ✅ Clean architecture: The new ✅ Improved state management: Removing the ✅ Request ID collision handling: Good defensive programming in Issues & Concerns🔴 Critical: Request ID Not Released After CompletionLocation: engine/packages/guard-core/src/proxy_service.rs:1889 The // At line 1889, request_id is generated
let request_id = state.generate_unique_request_id().await?;
// ... but I don't see state.release_request_id(request_id) anywhere in the taskRecommendation: Add a 🟡 Medium: Missing Request ID Release in HTTP PathLocation: engine/packages/guard-core/src/proxy_service.rs:1086 Similar to above, the HTTP request path generates a unique request ID but doesn't appear to release it when the request completes. The request ID should be released after the response is sent. Recommendation: Wrap the request handling in a scope guard or defer pattern to ensure 🟡 Medium: Serialization Size Not Verified at Compile TimeLocation: engine/packages/pegboard/src/tunnel/id.rs:34-39 The code uses a runtime assertion to verify the serialized size is exactly 12 bytes: ensure!(
written == 12,
"message id serialization produced wrong size: expected 12 bytes, got {}",
written
);While this is good defensive programming, if the BARE schema changes, this will only be caught at runtime. Recommendation: Consider adding a compile-time test or const assertion to verify the size. At minimum, add a unit test that exercises this code path. 🟡 Medium: Wrapping Arithmetic Without Overflow DocumentationLocation: engine/packages/pegboard-gateway/src/shared_state.rs:136 The message index uses wrapping arithmetic: req.message_index = req.message_index.wrapping_add(1);This is correct for a u16 counter, but there's no documentation about what happens when the index wraps from 65535 to 0, especially for WebSocket message acknowledgments. Recommendation: Add a comment explaining the wrapping behavior and confirming that the 🟢 Minor: Inconsistent Error Message FormattingLocation: engine/packages/pegboard/src/tunnel/id.rs:80-81 vs TypeScript implementation The Rust error messages use lowercase ("invalid request id length") while some TypeScript errors might use different casing. While minor, consistency helps with debugging. Recommendation: Standardize error message formats across Rust and TypeScript implementations. 🟢 Minor: Missing Documentation on MessageId FormatLocation: engine/sdks/schemas/runner-protocol/v3.bare:217 The BARE schema defines Recommendation: Add a comment in the BARE schema: Performance Considerations✅ The new format is actually more efficient - the old approach with UUID-based IDs used 16 bytes per message ID, while the new format uses 12 bytes. Over millions of messages, this saves bandwidth. ✅ Removing the
Security Considerations✅ Using random u32 for both gateway_id and request_id provides sufficient entropy for non-cryptographic uniqueness.
Test Coverage❌ Missing tests: I found no test files for the new tunnel ID functionality. Given this is a protocol-level change, unit tests are strongly recommended. Recommended tests:
SummaryThis is a solid refactoring that simplifies the protocol and improves efficiency. The main concern is the request ID memory leak which must be fixed before merging. The other issues are lower priority but should still be addressed. Required before merge:
Recommended before merge:
Nice to have:
Great work on the backwards compatibility handling! The deprecated tunnel ack approach is clean and will make rolling this out much safer. Review generated with Claude Code |
MasterPtato
force-pushed
the
11-19-chore_pegboard-gateway_remove_tunnelack_
branch
from
4adef6f to
759df3e
Compare
MasterPtato
force-pushed
the
11-19-chore_pegboard-gateway_add_new_message_id_format_add_deprecated_tunnel_ack
branch
from
e47241f to
6521b98
Compare
PR Review: New Message ID Format & Deprecated Tunnel AckI have completed a thorough review of this PR. Overall, this is a solid refactoring that improves the message ID system and maintains backwards compatibility. Here are my findings: Strengths
Critical Issues1. Memory Leak: Request IDs Never Released The release_request_id() method is defined in proxy_service.rs:687-689 but never called. This means the inflight_requests HashSet will grow indefinitely, memory usage will increase over time, and eventually could lead to performance degradation. Fix needed: Add cleanup calls after custom serve request completes (both success and error paths) and after WebSocket task completion. 2. Unsafe .expect() Calls in Database Code In engine/packages/pegboard/src/keys/actor.rs there are .expect() calls that will panic if the database contains invalid data. Per CLAUDE.md error handling guidelines, these should return proper Result<> with context instead of panicking in production code paths. Moderate Issues3. Missing Test Coverage - No tests added for the new message ID format, collision detection logic, or backwards compatibility handling. 4. Message Index Overflow - proxy_service.rs uses wrapping_add(1) for message_index (u16). After 65,535 messages it wraps to 0. Consider adding a comment or warning log. 5. Import Organization - Some files have imports that should be moved to the top of the file per CLAUDE.md style. Performance & Security
SummaryThis is a strong refactoring with good architectural decisions, but has two critical issues that must be fixed before merge:
Once these are addressed, this PR will be ready to merge. The new message ID format is well-designed and the backwards compatibility approach is solid. |
PR Review: New Message ID Format & Deprecated Tunnel AckThis is a well-structured refactoring that modernizes the tunnel message ID system. The changes introduce a more efficient message identification scheme while maintaining backward compatibility. Here's my detailed review: ✅ Strengths1. Improved Message ID Design
2. Strong Backward Compatibility
3. Good Type Safety
4. Hibernation Request Improvements
|
NathanFlurry
force-pushed
the
11-19-chore_pegboard-gateway_add_new_message_id_format_add_deprecated_tunnel_ack
branch
from
6521b98 to
d6a2e79
Compare
NathanFlurry
force-pushed
the
11-19-chore_pegboard-gateway_remove_tunnelack_
branch
from
759df3e to
4adef6f
Compare
PR Review: Add New Message ID Format & Deprecated Tunnel AckOverviewThis PR introduces a significant refactor to the message ID system in the Pegboard gateway, moving from UUID-based IDs to a more compact and structured format. The changes touch critical paths in the proxy service and gateway communication. Code Quality & Best Practices ✅Strengths:
Areas for Improvement: 1. Inconsistent Whitespace (engine/packages/guard-core/src/proxy_service.rs:641)// Generate unique request ID
let request_id = Some(self.generate_unique_request_id().await?);Mixed tabs and spaces. The leading whitespace appears to be spaces instead of tabs. Per CLAUDE.md, this project uses hard tabs for Rust formatting. 2. Missing Import Consolidation (engine/packages/guard-core/src/proxy_service.rs:16)The import for use pegboard::tunnel::id::{RequestId, generate_request_id};Consider grouping this with other pegboard imports if any exist nearby. Potential Bugs & Issues
|
Pull Request ReviewSummaryThis PR introduces a new message ID format for the tunnel protocol and adds backward compatibility for deprecated tunnel acknowledgments. The changes migrate from a 16-byte UUID-based message ID to a more efficient 12-byte structured format. Code Quality - Strengths
CRITICAL BUG - Double In-Flight AcquisitionLocation: engine/packages/guard-core/src/proxy_service.rs:1097-1114 The CustomServe path acquires in-flight limits TWICE:
Impact: CustomServe HTTP requests count twice against the in-flight limit, reducing effective capacity by 50%. Fix: Remove the second acquire_in_flight call in the CustomServe branch. Other Issues1. Request ID Collision Detection (proxy_service.rs:465-495)
2. Message Index Wrap-Around (pegboard-gateway/src/shared_state.rs:136-151)
3. Lock Contention (proxy_service.rs:381, 465)
4. Test Coverage
RecommendationsHigh Priority:
Medium Priority: Low Priority: ConclusionSolid refactoring with good protocol versioning. However, the double in-flight acquisition bug should be fixed before merging. The code follows Rivet conventions well. Great work on backward compatibility! |
MasterPtato
force-pushed
the
11-19-chore_pegboard-gateway_add_new_message_id_format_add_deprecated_tunnel_ack
branch
from
d6a2e79 to
adeb05d
Compare
PR Review: New Message ID Format & Deprecated Tunnel AckI've reviewed this PR and have several observations about code quality, potential issues, and areas for improvement. SummaryThis PR introduces a new compact message ID format (12 bytes instead of 16) and refactors in-flight request management. The changes touch critical path code in the gateway and proxy services. Positive Aspects1. Well-Designed Message ID StructureThe new message ID format (
2. Proper Version CompatibilityThe backward compatibility handling in
3. Collision DetectionThe Issues & Concerns🔴 Critical: Potential Resource LeakLocation: There's a problematic pattern where
Problem: The second acquisition will succeed and generate a different request ID, but the first request ID is never released. This creates:
Affected code: // First acquire - line 812
let request_id = match self.state.acquire_in_flight(client_ip, &actor_id, req.headers()).await? {
Some(id) => id,
None => { return Err(errors::RateLimit{...}.build()); }
};
// ... later in CustomServe path - line 1100
let request_id = match self.state.acquire_in_flight(client_ip, &actor_id, &req_headers).await? {
Some(id) => id, // NEW ID generated, old one leaked\!
None => { return Err(errors::RateLimit{...}.build()); }
};Recommendation:
🟡 Medium: Race Condition RiskLocation: async fn generate_unique_request_id(&self) -> anyhow::Result<RequestId> {
const MAX_TRIES: u32 = 100;
let mut requests = self.inflight_requests.lock().await; // Lock held during loop
for attempt in 0..MAX_TRIES {
let request_id = generate_request_id();
if \!requests.contains(&request_id) {
requests.insert(request_id);
return Ok(request_id);
}
// Lock still held while logging collision\!
tracing::warn\!(?request_id, attempt, "request id collision, generating new id");
}
}Issues:
Recommendation:
🟡 Medium: Indentation IssueLocation: // Generate unique request ID
let request_id = Some(self.generate_unique_request_id().await?); // Mixed spaces/tabs
Ok(request_id)This line uses spaces instead of hard tabs (violates 🟢 Minor: Unnecessary Code DuplicationLocation: The error logging pattern is duplicated: tracing::warn\!(
request_id=?tunnel_id::request_id_to_string(&request_id),
"received no message response during request init",
);Recommendation: Extract to a helper function or use a macro. 🟢 Minor: Missing Error ContextLocation: pub fn parse_message_id(message_id: MessageId) -> Result<protocol::MessageIdParts> {
serde_bare::from_slice(&message_id).context("failed to deserialize message id")
}Could include the actual message_id bytes in the error context for better debugging: serde_bare::from_slice(&message_id)
.with_context(|| format\!("failed to deserialize message id: {:?}", message_id))Security Considerations✅ Randomness SourceUses ✅ Collision Resistance4 bytes (2^32 = ~4 billion IDs) provides good collision resistance given:
|
Code Review: New Message ID Format & Deprecated Tunnel AckI've reviewed this PR and found the implementation to be well-structured with good attention to backward compatibility. Here are my findings: ✅ Strengths
🔍 Issues & Concerns1. Potential Double In-Flight Acquisition in
|
NathanFlurry
force-pushed
the
11-19-chore_pegboard-gateway_add_new_message_id_format_add_deprecated_tunnel_ack
branch
from
9d2a169 to
e98d4d0
Compare
NathanFlurry
force-pushed
the
11-19-chore_pegboard-gateway_remove_tunnelack_
branch
from
bdf8c0e to
bb070a3
Compare
Code ReviewI've reviewed this PR which adds a new message ID format and deprecated tunnel ack functionality to the pegboard-gateway. Overall, the changes are well-structured and improve the tunnel protocol architecture. Here are my findings: ✅ Strengths1. Improved Message ID Architecture
2. Request ID Uniqueness
3. Better Resource Management
4. Protocol Versioning
|
|
Pull Request Review: This PR introduces a significant refactoring of the tunnel messaging system. Key findings: 1) Improved 12-byte message ID format vs previous 16-byte UUIDs. 2) Good backward compatibility. 3) CRITICAL BUG: Double in-flight acquisition in CustomServe path (proxy_service.rs:815 and 1100) causing incorrect rate limiting. 4) Formatting issue at line 641 (mixed indentation). 5) Consider adding tests for ID collision, message wraparound, and protocol conversion. Recommendation: Fix double acquisition bug before merge. |
|
Detailed Review Points: POSITIVES:
ISSUES FOUND:
|
MasterPtato
force-pushed
the
11-19-chore_pegboard-gateway_remove_tunnelack_
branch
from
bb070a3 to
bdf8c0e
Compare
MasterPtato
force-pushed
the
11-19-chore_pegboard-gateway_add_new_message_id_format_add_deprecated_tunnel_ack
branch
from
e98d4d0 to
9d2a169
Compare
Pull Request ReviewI've reviewed PR #3492 which introduces a new message ID format and adds deprecated tunnel acknowledgment functionality. Here's my detailed feedback: OverviewThis PR refactors the tunnel message identification system from UUID-based IDs to a composite ID structure (gateway_id + request_id + message_index) and simplifies message acknowledgment by removing the deprecated ✅ Strengths
🔴 Critical Issues1. Race Condition in In-Flight Request Management (HIGH PRIORITY)Location: There's a serious bug where in-flight limits are acquired twice for the same request in some code paths: // First acquisition at line 809
let request_id = match self.state.acquire_in_flight(...).await? { ... };
// Then later for CustomServe routes, it acquires AGAIN at line 1100
let request_id = match self.state.acquire_in_flight(...).await? { ... };Problem:
Fix: Remove the duplicate 2. Request ID Not Released on Early Returns (HIGH PRIORITY)Location: The code spawns a task to release in-flight counters, but this only happens on normal returns: let state_clone = self.state.clone();
tokio::spawn(async move {
state_clone.release_in_flight(client_ip, &actor_id, request_id).await;
});Problem: If Fix: Use RAII pattern with a guard struct that releases on drop, or move the release logic into a 3. Integer Overflow Vulnerability (MEDIUM PRIORITY)Location: pub type MessageIndex = u16;
// ...
req.message_index = req.message_index.wrapping_add(1);Problem: While wrapping arithmetic is intentional, there's no guarantee that Recommendation:
|
Code ReviewThis PR introduces a new message ID format for the pegboard gateway tunnel system, transitioning from UUID-based IDs to a more compact format. Overall, the implementation is well-structured with good backwards compatibility handling. Here are my findings: Positive Aspects✅ Excellent backwards compatibility: The protocol version conversion logic (v1→v2→v3) is comprehensive and maintains compatibility with older clients. ✅ Good error handling: Proper error handling throughout, especially in message ID parsing and serialization. ✅ Structured logging: Uses tracing with structured fields instead of string interpolation (following CLAUDE.md guidelines). ✅ Memory efficiency: The new 12-byte message ID format (4 bytes gateway + 4 bytes request + 2 bytes index + 2 bytes padding) is more compact than the previous UUID approach. Issues and Concerns🔴 Critical: Indentation in proxy_service.rsLocation: // Generate unique request ID
let request_id = Some(self.generate_unique_request_id().await?);This line has incorrect indentation (spaces instead of tabs). Per CLAUDE.md, "Hard tabs for Rust formatting". This should be: // Generate unique request ID
let request_id = Some(self.generate_unique_request_id().await?);🟡 Medium: Request ID Collision HandlingLocation: The collision detection uses only 4 bytes (2^32 possibilities) for request IDs. While the code handles collisions with retry logic, there are concerns:
Suggestion: Consider using 8 bytes for request IDs to drastically reduce collision probability, or implement a more sophisticated algorithm (e.g., atomically incrementing counter + timestamp + random bits). 🟡 Medium: Message Index WrappingLocation: message_index: req.message_index.wrapping_add(1);The message index is a Potential Issue: If a connection sends >65k messages and wraps around while some messages are still pending ack, the comparison logic may incorrectly retain/discard messages. Suggestion: Add a comment explaining the wrapping behavior and its limitations, or consider using a larger type (u32) for message indices. 🟡 Medium: Duplicate In-Flight AcquisitionLocation: In the
This means:
Suggestion: Remove the duplicate acquisition inside the CustomServe handler since it's already handled at the top level. Or clearly document why CustomServe needs different rate limiting. 🟢 Minor: BARE Serialization AssumptionLocation: The code assumes BARE serialization of ensure!(
written == 12,
"message id serialization produced wrong size: expected 12 bytes, got {}",
written
);While this is currently correct (4 + 4 + 2 = 10, with BARE encoding adding minimal overhead), it's fragile if the struct changes. Consider adding a compile-time assertion or unit test to verify this invariant. 🟢 Minor: Missing Import OrganizationLocation: Per CLAUDE.md: "Always add imports at the top of the file inside of inline within the function." The new import for 🟢 Minor: Inconsistent Error MessagesLocation: Various logging statements Some log messages use base64 representation ( Security Considerations✅ No SQL injection: Uses parameterized queries throughout. Performance Considerations✅ Reduced memory footprint: 12-byte message IDs vs 16-byte UUIDs saves memory at scale. Test CoverageThe PR doesn't include new tests for:
Recommendation: Add unit tests for these scenarios. Recommendations
SummaryThis is a solid refactoring that improves the tunnel message format. The main concerns are around the collision handling for 4-byte request IDs and duplicate resource acquisition. The indentation issue needs to be fixed before merge. Overall Assessment: ✅ Approve with changes requested 🤖 Generated with Claude Code |
NathanFlurry
force-pushed
the
11-19-chore_pegboard-gateway_add_new_message_id_format_add_deprecated_tunnel_ack
branch
from
9d2a169 to
e98d4d0
Compare
NathanFlurry
force-pushed
the
11-19-chore_pegboard-gateway_remove_tunnelack_
branch
from
bdf8c0e to
bb070a3
Compare
Pull Request Review: Message ID Format & Deprecated Tunnel AckI've reviewed this PR which implements a new message ID format and adds backwards compatibility for deprecated tunnel acknowledgments. Here's my comprehensive feedback: ✅ Strengths1. Well-Designed ID System
2. Request Lifecycle Management
3. Protocol Versioning
4. Database Key Updates
|
PR Review: Add New Message ID FormatThis PR introduces a new message ID format for improved tunnel message tracking. Changes span 50 files with significant refactoring in gateway and protocol handling. Critical Issues (Fix before merge)1. Indentation Error (proxy_service.rs:641) - Line uses spaces instead of tabs, violates CLAUDE.md hard tabs requirement 2. Double In-Flight Acquisition Bug - handle_http_request calls acquire_in_flight again for custom serve routes, but request ID was already acquired in handle_request. This causes double-counting and potential resource leaks. Pass request_id from outer scope instead. 3. Resource Leak Risk - In handle_websocket_upgrade, spawned task only releases in-flight at task end. If task panics or cancels, request ID leaks from HashSet. Implement RAII guard pattern for cleanup. Performance Concerns
Security
Test CoverageMissing tests for:
VerdictSolid architectural improvement but has critical bugs that must be fixed before merge. Test coverage needed for such significant changes. |
Merge activity
|
graphite-app
bot
deleted the
11-19-chore_pegboard-gateway_add_new_message_id_format_add_deprecated_tunnel_ack
branch
No description provided.