chore(rivetkit): correctly stringify engine runner request ids for logs #3330
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
Code Review: Correctly stringify engine runner request IDs for logsSummaryThis PR improves logging by replacing the Positive Aspects ✅
Issues & Concerns 🔍1. Logic Error in Acknowledgment Handling (Critical)Location: const pending = this.#pendingTunnelMessages.get(messageIdStr);
if (pending) {
const didDelete = this.#pendingTunnelMessages.delete(messageIdStr);
if (!didDelete) {
logger()?.warn({
msg: "received tunnel ack for nonexistent message",
requestId: requestIdStr,
messageId: messageIdStr,
});
}
}Problem: This code checks if Suggested fix: The logic should be restructured: const didDelete = this.#pendingTunnelMessages.delete(messageIdStr);
if (!didDelete) {
logger()?.warn({
msg: "received tunnel ack for nonexistent message",
requestId: requestIdStr,
messageId: messageIdStr,
});
}Or if you want to keep the check: const pending = this.#pendingTunnelMessages.get(messageIdStr);
if (!pending) {
logger()?.warn({
msg: "received tunnel ack for nonexistent message",
requestId: requestIdStr,
messageId: messageIdStr,
});
} else {
this.#pendingTunnelMessages.delete(messageIdStr);
}2. Missing UUID Package Dependency VerificationThe PR adds
3. Incomplete Import UpdatesLocation: The PR adds imports for import type {
RivetEvent,
RivetMessageEvent,
UniversalWebSocket,
} from "@/common/websocket-interface";However, these types don't appear to be used in the visible diff. Consider:
Minor Suggestions 💡
Testing Recommendations 🧪Per the CLAUDE.md guidelines, when running tests, consider:
Example test verification: cargo test -p rivetkit 2>&1 | tee /tmp/test_output.log
grep "send tunnel msg" /tmp/test_output.log
grep "receive tunnel msg" /tmp/test_output.logSecurity & Performance ✓
Overall AssessmentThis is a valuable improvement for observability and debugging. The main concern is the logic error in the acknowledgment handling code that should be addressed before merging. Once that's fixed, this PR will meaningfully improve the developer experience when debugging tunnel message flows. Recommendation: Request changes to fix the acknowledgment logic, verify the unused imports, and then approve. |
NathanFlurry
force-pushed
the
11-03-chore_rivetkit_correctly_stringify_engine_runner_request_ids_for_logs
branch
from
cad68b3 to
6079f1f
Compare
NathanFlurry
force-pushed
the
11-03-fix_rivetkit_ack_ws_messages
branch
from
f1efe27 to
7b1fd28
Compare
Code ReviewSummaryThis PR improves logging for the engine runner by correctly stringifying request IDs and message IDs using the Positive Changes ✅
Issues & Concerns
|
Code Review for PR #3330OverviewThis PR improves request ID logging in the RivetKit engine runner by correctly stringifying Positive Changes1. Correct UUID Stringification ✅
2. Improved Code Organization ✅
3. Enhanced Logging ✅
Issues Found1. Logic Error in Tunnel Acknowledgment Handling In const pending = this.#pendingTunnelMessages.get(messageIdStr);
if (pending) {
const didDelete = this.#pendingTunnelMessages.delete(messageIdStr);
if (!didDelete) {
logger()?.warn({
msg: "received tunnel ack for nonexistent message",
requestId: requestIdStr,
messageId: messageIdStr,
});
}
}Problem: The Suggested fix: Remove the redundant delete check or move the warning outside the const didDelete = this.#pendingTunnelMessages.delete(messageIdStr);
if (!didDelete) {
logger()?.warn({
msg: "received tunnel ack for nonexistent message",
requestId: requestIdStr,
messageId: messageIdStr,
});
}Minor Code Quality Observations2. Formatting Changes ℹ️ Several formatting changes (like line breaks, spacing) are present throughout the PR. According to
These auto-formatting changes are generally acceptable but add noise to the diff. Consider whether your IDE is auto-formatting on save. 3. Variable Declaration Style ℹ️ In const _unhandled = await this.#handleWebSocketMessage(The Performance & SecurityPerformance: ✅ No concerns. The Security: ✅ No concerns. The changes are purely cosmetic/logging improvements and don't affect security boundaries. Test CoverageObservation: No test files were modified in this PR. Consider adding tests to verify:
SummaryApprove with suggested fix: This PR makes a valuable improvement to logging clarity, but should address the logic issue in the tunnel acknowledgment handling before merging. Action Items:
Overall, this is a solid improvement that will make debugging significantly easier! 🚀 |
NathanFlurry
force-pushed
the
11-03-chore_rivetkit_correctly_stringify_engine_runner_request_ids_for_logs
branch
from
6079f1f to
8d1d88d
Compare
PR Review: Correctly stringify engine runner request IDs for logsSummaryThis PR improves logging consistency by replacing the custom Positive AspectsCode Quality:
Logging Improvements:
Code Consistency:
Issues FoundCritical Issue - Logic Error: const pending = this.#pendingTunnelMessages.get(messageIdStr);
if (pending) {
const didDelete = this.#pendingTunnelMessages.delete(messageIdStr);
if (!didDelete) {
logger()?.warn({
msg: "received tunnel ack for nonexistent message",
requestId: requestIdStr,
messageId: messageIdStr,
});
}
}The logic here is incorrect. If
This code should likely be: const didDelete = this.#pendingTunnelMessages.delete(messageIdStr);
if (!didDelete) {
logger()?.warn({
msg: "received tunnel ack for nonexistent message",
requestId: requestIdStr,
messageId: messageIdStr,
});
}Or keep the existing logic if you want to avoid the warning, but the Minor IssuesUnnecessary Variable Declaration: const _unhandled = await this.#handleWebSocketMessage(
message.requestId,
message.messageKind.val,
);Consider just calling the function without assignment if the return value is truly unused: await this.#handleWebSocketMessage(
message.requestId,
message.messageKind.val,
);Formatting Changes: Performance Considerations✅ No performance concerns:
Security Considerations✅ No security issues:
Test Coverage
Recommendation: Add tests in the rivetkit test suite to verify UUID stringification matches expected format. Dependencies✅ Dependency management:
Recommendations
Overall AssessmentThis is a solid improvement for debugging and log readability. The main refactoring from Verdict: |
NathanFlurry
force-pushed
the
11-03-fix_rivetkit_ack_ws_messages
branch
from
5f837ae to
9d011a4
Compare
NathanFlurry
force-pushed
the
11-03-chore_rivetkit_correctly_stringify_engine_runner_request_ids_for_logs
branch
from
8d1d88d to
35bf113
Compare
|
Deployment failed with the following error: Learn More: https://vercel.com/docs/limits#rate-limits |
Pull Request ReviewSummaryThis PR improves logging for engine runner request IDs by properly stringifying UUID ArrayBuffers using the uuid library's stringify function instead of base64 encoding. This change makes logs more human-readable and consistent with standard UUID formatting. Positive Aspects✅ Correct UUID Handling: The change from bufferToString (base64) to idToStr (UUID stringify) is the right approach for displaying UUIDs in logs. This makes debugging significantly easier. ✅ Consistent Naming: Renaming bufferToString to idToStr better conveys the function's purpose - it's not just converting a buffer to a string, it's specifically formatting UUIDs. ✅ Improved Logging: Added debug logging in key places (e.g., send tunnel msg, receive tunnel msg) which improves observability. ✅ Code Consolidation: Consolidating the utility function in utils.ts and removing duplication is a good practice. ✅ Proper Dependency: Adding uuid to package.json dependencies ensures the library is available. Issues & Concerns1. Logic Bug in Ack Handling (Critical)Location: tunnel.ts:266-278 if (pending) {
const didDelete = this.#pendingTunnelMessages.delete(messageIdStr);
if (!didDelete) {
logger()?.warn({
msg: "received tunnel ack for nonexistent message",
requestId: requestIdStr,
messageId: messageIdStr,
});
}
}Problem: The code checks if pending exists, then attempts to delete, but warns if delete fails. This logic is flawed because:
Recommendation: Simplify to: const pending = this.#pendingTunnelMessages.get(messageIdStr);
if (pending) {
this.#pendingTunnelMessages.delete(messageIdStr);
} else {
logger()?.warn({
msg: "received tunnel ack for nonexistent message",
requestId: requestIdStr,
messageId: messageIdStr,
});
}Or if you want to keep the deletion check: const didDelete = this.#pendingTunnelMessages.delete(messageIdStr);
if (!didDelete) {
logger()?.warn({
msg: "received tunnel ack for nonexistent message",
requestId: requestIdStr,
messageId: messageIdStr,
});
}2. Formatting Changes Mixed with Logic ChangesThe PR mixes formatting changes (prettier/auto-formatting) with the actual logic changes. This makes the diff harder to review. Examples:
Recommendation: According to CLAUDE.md, you should not run formatting commands. These changes suggest auto-formatting was applied. Consider separating formatting from logic changes in future PRs. 3. Unnecessary Variable ReassignmentLocation: tunnel.ts:312 case "ToClientWebSocketMessage": {
this.#sendAck(message.requestId, message.messageId);
const _unhandled = await this.#handleWebSocketMessage(
message.requestId,
message.messageKind.val,
);
break;
}The let _unhandled was changed to const _unhandled which is good, but the variable is never used. The underscore prefix suggests it's intentionally unused. Question: Is this return value important for future use, or can it be removed entirely? 4. Test CoverageNo tests were added or modified in this PR. Recommendation: Given that this changes logging behavior and ID stringification, consider adding a test that:
Minor Observations
Security Considerations✅ No security concerns identified. The change from base64 to UUID string formatting doesn't introduce vulnerabilities. Performance Considerations✅ Minimal performance impact. uuidstringify should have similar performance to base64 encoding. The added logging may have a small impact but only when debug logging is enabled. RecommendationsHigh Priority:
Medium Priority: Low Priority: Overall AssessmentThis is a good improvement for developer experience and debugging. The main issue is the logic bug in the ack handling that should be addressed before merging. Otherwise, the changes properly implement UUID stringification for better log readability. 🤖 Generated by Claude Code |
Merge activity
|
NathanFlurry
force-pushed
the
11-03-chore_rivetkit_correctly_stringify_engine_runner_request_ids_for_logs
branch
from
35bf113 to
2e78d0d
Compare
NathanFlurry
force-pushed
the
11-03-fix_rivetkit_ack_ws_messages
branch
from
9d011a4 to
0298a50
Compare
|
Deployment failed with the following error: Learn More: https://vercel.com/docs/limits#rate-limits |
Code ReviewSummaryThis PR improves logging for engine runner request IDs by replacing base64 encoding with proper UUID string formatting. The change makes logs more readable and debuggable. Positive Changes ✅
Issues & Concerns 🔍1. Logic Error in Tunnel Ack Handler (
|
PR Review: chore(rivetkit): correctly stringify engine runner request ids for logsSummaryThis PR refactors the ID stringification logic across the tunnel and actor driver code, replacing the base64-based ✅ Positives
🔍 Observations & Potential Issues1. Performance Consideration - UUID String AllocationLocation: The new implementation creates UUID strings eagerly even when logging is disabled: const requestIdStr = idToStr(requestId);
const messageIdStr = idToStr(messageId);
logger()?.debug({
msg: "send tunnel msg",
requestId: requestIdStr,
messageId: messageIdStr,
message: messageKind,
});Recommendation: Consider lazy evaluation when logger is optional: if (logger()) {
logger().debug({
msg: "send tunnel msg",
requestId: idToStr(requestId),
messageId: idToStr(messageId),
message: messageKind,
});
}This avoids unnecessary UUID string allocations in production when debug logging is disabled. 2. Inconsistent Error HandlingLocation: The code checks if a message was deleted and logs a warning if it wasn't, but this seems contradictory: const didDelete = this.#pendingTunnelMessages.delete(messageIdStr);
if (!didDelete) {
logger()?.warn({
msg: "received tunnel ack for nonexistent message",
requestId: requestIdStr,
messageId: messageIdStr,
});
}However, this logic is placed inside const pending = this.#pendingTunnelMessages.get(messageIdStr);
if (pending) {
this.#pendingTunnelMessages.delete(messageIdStr);
} else {
logger()?.warn({
msg: "received tunnel ack for nonexistent message",
requestId: requestIdStr,
messageId: messageIdStr,
});
}3. Missing uuid Dependency DocumentationThe PR adds Recommendation: Verify that all packages importing 4. Unrelated Formatting ChangesLocation: Multiple locations throughout tunnel.ts The PR includes many unrelated formatting changes (empty function bodies changed from Per CLAUDE.md:
Recommendation: Keep formatting changes separate from functional changes in future PRs. 🧪 TestingMissing Test Coverage: There are no test files for the tunnel functionality. Given that this changes how IDs are stringified throughout the system, tests would be valuable to ensure:
Recommendation: Consider adding tests for 🔒 SecurityNo security concerns identified. The change from base64 to UUID string representation doesn't introduce security issues. 📊 PerformanceMinor performance considerations:
✨ Suggestions
Overall AssessmentThis is a good quality refactoring that improves log readability and code organization. The functional changes are solid, though there's a minor logic issue with the delete check that should be addressed. The addition of debug logging is helpful. Main suggestions are around performance optimization for logging and fixing the delete check logic. Recommendation: ✅ Approve with minor suggested improvements |
graphite-app
bot
deleted the
11-03-chore_rivetkit_correctly_stringify_engine_runner_request_ids_for_logs
branch
No description provided.