fix(core): add more logging around downloading engine #1412
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
💡 Enable Vercel Agent with $100 free credit for automated AI reviews |
Merge activity
|
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
How to use the Graphite Merge QueueAdd either label to this PR to merge it via 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. |
graphite-app
bot
changed the base branch from
10-19-chore_core_auto-start_engine_in_development_for_serverless_runners_disable_health_check_in_serverless
to
graphite-base/1412
graphite-app
bot
deleted the
10-19-fix_core_add_more_logging_around_downloading_engine
branch
Pull Request ReviewSummaryThis PR adds enhanced logging around the engine binary download process to improve observability and user experience during download operations. ✅ Strengths
🔍 Observations & Suggestions1. Race Condition Still Possible (Minor)While using Suggestion: Consider using atomic file operations or a lock file mechanism: // Optional improvement for the future
const lockPath = `${binaryPath}.lock`;
// Implement file-based locking or check if temp file already exists before downloadingHowever, this is likely a rare edge case and may not be worth the added complexity. 2. Timeout Duration Hardcoded (Minor)The 5000ms timeout is hardcoded. For very large binaries or slow networks, this might trigger too early. Suggestion: Consider making this configurable or calculating it based on content-length: const contentLength = response.headers.get("content-length");
const warningTimeout = contentLength
? Math.max(5000, parseInt(contentLength) / 100000) // Adjust based on size
: 5000;This is a minor point and the current implementation is reasonable for most cases. 3. Download Size Not Validated (Minor)The code gets the file size after download but doesn't validate it against the Suggestion: Add validation: const contentLength = response.headers.get("content-length");
if (contentLength && stats.size !== parseInt(contentLength)) {
logger().warn({
msg: "downloaded file size mismatch",
expected: contentLength,
actual: stats.size,
});
// Consider retrying or throwing an error
}4. Error Object in LogsLines 227-228 log the raw error object. Depending on the error type, this might not serialize well in structured logs. Suggestion: Consider extracting error properties: logger().warn({
msg: "engine download failed, please report this error",
tempPath,
error: error instanceof Error ? error.message : String(error),
stack: error instanceof Error ? error.stack : undefined,
issues: "https://github.com/rivet-dev/rivetkit/issues",
support: "https://rivet.dev/discord",
});🔒 SecurityNo security concerns identified. The changes are limited to logging and error handling improvements. ⚡ PerformanceThe changes have minimal performance impact:
🧪 Test CoverageThe existing test file ( Recommendation: Consider adding integration tests for:
However, testing download functionality can be challenging and may require mocking. The current manual testing might be sufficient given the relatively straightforward nature of the changes. 📝 Overall AssessmentRecommendation: ✅ Approve This is a solid PR that improves the user experience and error handling around engine downloads. The changes are well-implemented, follow the project's coding standards, and add valuable observability. The suggestions above are minor improvements that could be addressed in future PRs if needed. They don't block this PR from being merged. Code Quality: ⭐⭐⭐⭐ (4/5) Great work on improving the download experience! 🚀 |
1 participant
No description provided.