-
Notifications
You must be signed in to change notification settings - Fork 129
chore(runner): add explicit close reason when client runner shuts down #3382
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
c322940 to
9283d04
Compare
500649f to
4078059
Compare
PR Review: Add explicit close reason when client runner shuts downSummaryThis PR improves logging clarity by adding a structured close reason ( Code Quality & Best PracticesStrengths:
Suggestions:
Potential IssuesMinor concerns:
Performance Considerations✅ No performance impact - this only affects logging and close reason strings. Security Concerns✅ No security issues identified. The close reason is a benign informational string. RecommendationsPriority: Low
Priority: Very Low
Overall AssessmentThis is a clean, focused improvement that enhances debugging experience. The changes follow existing patterns and conventions. The PR is ready to merge with the optional improvements noted above. Approval: ✅ Approved with minor suggestions |
More templates
@rivetkit/actor
@rivetkit/cloudflare-workers
@rivetkit/core
@rivetkit/db
@rivetkit/framework-base
@rivetkit/next-js
@rivetkit/react
rivetkit
@rivetkit/sql-loader
@rivetkit/engine-runner
@rivetkit/engine-runner-protocol
commit: |
Code ReviewThis PR improves the clarity of WebSocket close handling by adding explicit close reasons and improving logging. Overall, the changes look good and follow the established patterns in the codebase. ✅ Strengths
🔍 Observations
💡 Minor Suggestions
🎯 VerdictApproved - This is a clean change that improves observability without introducing any bugs or breaking changes. The close reason format is compatible with existing parsing logic, and the logging changes are appropriate for the different severity levels. |
PR ReviewSummaryThis PR improves logging clarity by adding an explicit close reason Positive Aspects✅ Improved observability: The change makes it easier to distinguish intentional shutdowns from unexpected disconnections in logs Code QualityGood:
Observation:
This is readable, but could potentially be flattened using early returns or a switch-like pattern if more close reasons are added in the future. Not a blocker for this PR. Logging PatternsThe logging changes follow the project's conventions from CLAUDE.md:
Potential IssuesMinor concerns:
Security & Performance
Recommendations
VerdictApproved ✅ This is a clean, well-focused change that improves observability without introducing bugs or breaking changes. The implementation correctly follows established patterns in the codebase. |
9283d04 to
8b6c64b
Compare
4078059 to
04f4430
Compare
PR Review: Add explicit close reason when client runner shuts downSummaryThis PR improves observability by adding an explicit close reason ( Code Quality ✅Strengths:
Best Practices:
Potential Issues 🔍Minor Concerns:
Performance ✅No performance concerns. The changes are minimal and only affect shutdown paths. Security ✅No security concerns identified. Test Coverage
|
PR ReviewSummaryThis PR improves logging clarity by adding an explicit close reason ( Code Quality ✅Positive aspects:
Code changes:
Potential Issues & Suggestions1. Missing namespace and runnerName fields 🔍In the eviction case (line 684), the simplified logging lost the Suggestion: Consider keeping these fields for the eviction case: this.log?.info({
msg: "runner websocket evicted",
namespace: this.#config.namespace,
runnerName: this.#config.runnerName,
});2. Inconsistent structured logging pattern 📝Line 684 uses string-only logging ( Current: this.log?.info("runner shutdown");Suggested (following structured logging pattern): this.log?.info({ msg: "runner shutdown" });This maintains consistency with other log statements in the file (e.g., lines 393-396, 431-433). 3. Close code documentation 📚The WebSocket close code Performance Considerations ✅No performance concerns. The changes are purely logging-related and don't introduce any new computational overhead. Security Concerns ✅No security issues identified. The change improves observability which is beneficial for security monitoring. Test Coverage
|
04f4430 to
d93280c
Compare
8b6c64b to
8cf4830
Compare
PR Review: Add explicit close reason when client runner shuts downSummaryThis PR adds a more explicit close reason ( Code Quality ✅Strengths:
Style:
Functionality ✅What the change does:
Correctness:
Potential Issues 🤔Minor concern:
Question:
Testing
|
Merge activity
|

No description provided.