Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Nov 3, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
rivetkit-serverless Ready Ready Preview Comment Nov 4, 2025 8:26pm
3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 4, 2025 8:26pm
rivet-inspector Ignored Ignored Preview Nov 4, 2025 8:26pm
rivet-site Ignored Ignored Preview Nov 4, 2025 8:26pm

Copy link
Member Author

NathanFlurry commented Nov 3, 2025

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.
Learn more


How to use the Graphite Merge Queue

Add 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.

@claude
Copy link

claude bot commented Nov 3, 2025

PR Review: chore(scripts): tee run logs to file

Summary

This PR adds log file output to the engine run scripts using tee to write logs to /tmp/rivet-engine.log. This aligns with the testing guidelines in CLAUDE.md which recommend piping test output to files for later analysis.

Positive Points

  • Simple, focused change that improves debuggability
  • Consistent implementation across both postgres and rocksdb scripts
  • Maintains console output while also writing to file (using tee)
  • Aligns with the project's testing guidelines

Issues & Suggestions

1. Log File Naming Collision

Severity: Medium

Both scripts write to the same log file /tmp/rivet-engine.log. If a developer runs both scripts sequentially or in different terminals, logs will be mixed or overwritten.

Recommendation:

# engine-postgres.sh
cargo run --bin rivet-engine -- start "$@" | tee /tmp/rivet-engine-postgres.log

# engine-rocksdb.sh
cargo run --bin rivet-engine -- start "$@" | tee /tmp/rivet-engine-rocksdb.log

2. Consider Append Mode

Severity: Low

Using tee without -a overwrites the log file on each run. For debugging across multiple runs, append mode might be more useful:

cargo run --bin rivet-engine -- start "$@" | tee -a /tmp/rivet-engine-postgres.log

Though overwrite mode is also valid if you only care about the most recent run.

3. Stderr Not Captured

Severity: Low

Error output (stderr) is not being captured in the log file, only stdout. Consider using:

cargo run --bin rivet-engine -- start "$@" 2>&1 | tee /tmp/rivet-engine-postgres.log

This ensures both stdout and stderr are captured, which is particularly important for debugging errors.

4. Minor: Trailing Newline

Severity: Very Low

The engine-rocksdb.sh file has an extra blank line at the end (line 12). While harmless, it's inconsistent with engine-postgres.sh.

Code Quality: ✅ Good

  • Clean, simple changes
  • No syntax errors
  • Follows bash best practices with set -euo pipefail

Security: ✅ No concerns

  • Writing to /tmp is appropriate for development logs
  • No sensitive data handling changes

Performance: ✅ No impact

  • tee has negligible overhead for logging

Test Coverage: N/A

  • Shell scripts don't have tests in this project

Recommendation

Approve with suggestions. The core change is solid. I recommend addressing issue #1 (naming collision) before merging. Issues #2 and #3 are optional improvements that could be addressed now or in a follow-up.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Nov 3, 2025

More templates

@rivetkit/actor

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/actor@3314

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/cloudflare-workers@3314

@rivetkit/core

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/core@3314

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/db@3314

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/framework-base@3314

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/next-js@3314

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/react@3314

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivet/rivetkit@3314

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sql-loader@3314

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@3314

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@3314

commit: 7549ac0

@vercel
Copy link

vercel bot commented Nov 4, 2025

Deployment failed with the following error:

Too many requests - try again in 5 minutes (more than 120, code: "api-deployments-flood-pro").

Learn More: https://vercel.com/docs/limits#rate-limits

@claude
Copy link

claude bot commented Nov 4, 2025

Pull Request Review

Summary

This PR adds log file output to the engine run scripts by piping output through tee to /tmp/rivet-engine.log. This allows developers to both see logs in the terminal and have them saved to a file for later inspection.

Code Quality - Positive

  • Simple, focused change that addresses a clear need
  • Consistent implementation across both scripts
  • Aligns with the Testing Guidelines in CLAUDE.md which mention piping test output to files in /tmp/
  • Non-breaking change - scripts continue to output to stdout while also saving to file

Potential Issues and Recommendations

1. File Path Collision
Both scripts write to the same file path /tmp/rivet-engine.log. If a developer runs both scripts (or the same script multiple times), the log file will be overwritten without warning.

Recommendation: Consider using distinct filenames like /tmp/rivet-engine-postgres.log and /tmp/rivet-engine-rocksdb.log, or include a timestamp for each run to preserve history.

2. stderr Not Captured
The current implementation only captures stdout. Error messages written to stderr will appear in the terminal but won't be saved to the log file.

Recommendation: Use 2>&1 to capture both stdout and stderr.

3. Log File Growth
Since the log file is in /tmp, it will be cleaned up on reboot. However, for long-running development sessions, the file could grow large. This is likely acceptable for a development script.

4. Documentation
Consider adding a comment in the scripts or updating documentation to inform users that logs are being saved to /tmp/rivet-engine.log.

Security Considerations

No security concerns. Using /tmp for development logs is appropriate.

Performance Considerations

Minimal impact. tee is efficient and the I/O overhead is negligible for log output.

Overall Assessment

This is a useful improvement for debugging and development workflows. The implementation is clean and simple. Addressing the file collision and stderr capture issues would make it even more robust.

Recommendation: Approve with minor suggestions for improvement.

@claude
Copy link

claude bot commented Nov 4, 2025

Code Review

Summary

This PR adds log file output via tee to two engine run scripts, allowing logs to be saved to /tmp/rivet-engine.log while still displaying on stdout. This is a useful improvement for debugging and log persistence.

Positive Aspects

✅ Simple, focused change that adds value
✅ Consistent implementation across both scripts
✅ Aligns with the testing guidelines in CLAUDE.md which mention piping test output to files in /tmp/

Concerns & Recommendations

1. Log File Conflict ⚠️

Both scripts write to the same file path /tmp/rivet-engine.log. If a developer runs both scripts (or the same script multiple times), logs will be overwritten without warning.

Recommendation: Use distinct log filenames:

# engine-postgres.sh
cargo run --bin rivet-engine -- start "$@" | tee /tmp/rivet-engine-postgres.log

# engine-rocksdb.sh  
cargo run --bin rivet-engine -- start "$@" | tee /tmp/rivet-engine-rocksdb.log

Or use timestamps for uniqueness:

LOG_FILE="/tmp/rivet-engine-$(date +%Y%m%d-%H%M%S).log"
cargo run --bin rivet-engine -- start "$@" | tee "$LOG_FILE"
echo "Logs saved to: $LOG_FILE"

2. stderr Not Captured ⚠️

The current implementation only captures stdout. Error messages and stack traces written to stderr will not be captured in the log file.

Recommendation: Capture both stdout and stderr:

cargo run --bin rivet-engine -- start "$@" 2>&1 | tee /tmp/rivet-engine.log

3. Signal Handling

When the script is interrupted (Ctrl+C), tee should properly handle the signal. This is generally fine with the current implementation, but worth testing.

4. Minor: Whitespace Consistency

The engine-rocksdb.sh has an extra blank line added at the end (line 11-12). While not harmful, it is inconsistent with engine-postgres.sh.

Performance Considerations

✅ No performance concerns - tee has minimal overhead for log output.

Security Considerations

✅ Using /tmp/ is acceptable for development scripts
✅ No sensitive data exposure concerns
ℹ️ Note that /tmp/ files may be readable by other users on multi-user systems, but this is expected for development environments

Test Coverage

Not applicable for script changes. However, I recommend testing:

  • Running both scripts simultaneously
  • Interrupting the scripts with Ctrl+C
  • Verifying both stdout and stderr are properly captured

Overall Assessment

Good incremental improvement. The main issue is the log file conflict between the two scripts. Otherwise, this is a straightforward and useful change. Consider capturing stderr as well for complete log coverage.

Verdict: ✅ Approve with suggestions for improvement

@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 4, 2025

Merge activity

  • Nov 4, 8:09 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Nov 4, 8:11 PM UTC: CI is running for this pull request on a draft pull request (#3349) due to your merge queue CI optimization settings.
  • Nov 4, 8:41 PM UTC: The Graphite merge queue removed this pull request due to removal of a downstack PR #3342.
  • Nov 4, 8:49 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Nov 4, 8:50 PM UTC: CI is running for this pull request on a draft pull request (#3352) due to your merge queue CI optimization settings.
  • Nov 4, 8:52 PM UTC: Merged by the Graphite merge queue via draft PR: #3352.

graphite-app bot pushed a commit that referenced this pull request Nov 4, 2025
@claude
Copy link

claude bot commented Nov 4, 2025

PR Review: Tee run logs to file

This PR adds log file output to the engine startup scripts using tee, allowing logs to be both displayed in the terminal and written to /tmp/rivet-engine.log.

Positive Aspects:

  • Aligns with CLAUDE.md testing guidelines about piping to /tmp files
  • Non-invasive change preserving existing terminal output
  • Consistent implementation across both scripts
  • Enables easier debugging and log analysis

Suggestions:

  1. Stderr Handling - Only stdout is captured. Consider: 2>&1 | tee to capture both streams
  2. Unique Log Files - Both scripts use same file path. Running simultaneously will overwrite logs
  3. Minor inconsistency - Extra blank line at end of engine-rocksdb.sh

Verdict: Useful quality-of-life improvement. Approve with optional enhancements for future iteration.

@claude
Copy link

claude bot commented Nov 4, 2025

PR Review: chore(scripts): tee run logs to file

Summary

This PR adds log file output to the engine run scripts by piping the output through tee to save logs to /tmp/rivet-engine.log. This aligns well with the testing guidelines in CLAUDE.md that recommend piping test output to files for easier analysis.

Code Quality

  • Clean, minimal change that follows the principle of doing one thing well
  • Consistent application across both postgres and rocksdb engine scripts
  • Follows the existing script patterns and conventions

Potential Issues & Considerations

1. Log File Collision

Both scripts write to the same log file (/tmp/rivet-engine.log). If a user runs both scripts (or runs one after another), the second invocation will overwrite the first log file. Consider:

  • Using different log filenames: /tmp/rivet-engine-postgres.log and /tmp/rivet-engine-rocksdb.log
  • Or appending timestamps to avoid overwrites
  • Or using append mode with tee -a

2. Error Stream Handling

The current implementation only captures stdout. stderr is not being captured to the log file. Consider adding 2>&1 before the pipe to capture both stdout and stderr, which is crucial for debugging issues.

3. Exit Code Preservation

When using tee in a pipeline, the exit code of cargo run might be masked. However, this is already handled correctly by the existing set -euo pipefail at the top of both scripts.

4. Disk Space Considerations

Long-running engines with verbose logging could generate large log files in /tmp/. This is generally fine for development scripts, but consider documenting this behavior.

5. Whitespace Consistency

Minor: engine-rocksdb.sh adds an extra blank line at the end. This is cosmetic but could be cleaned up for consistency.

Recommendations

High Priority:

  1. Capture stderr in addition to stdout by adding 2>&1 before the pipe
  2. Use distinct log filenames for postgres vs rocksdb variants to prevent overwrites

Low Priority:
3. Remove extra blank line in engine-rocksdb.sh
4. Consider adding a comment in the script explaining where logs are saved

Verdict

This is a useful quality-of-life improvement for debugging. With the suggested stderr capture and unique filenames, this will be even more robust. The change is functional as-is for basic use cases.

graphite-app bot pushed a commit that referenced this pull request Nov 4, 2025
@graphite-app graphite-app bot closed this Nov 4, 2025
@graphite-app graphite-app bot deleted the 11-02-chore_scripts_tee_run_logs_to_file branch November 4, 2025 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants