chore(runner): improve runner connection logs

[NathanFlurry]

No description provided.

[vercel]

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

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Ready	Preview	Comment	Nov 5, 2025 4:06am

3 Skipped Deployments

Project	Deployment	Preview	Comments	Updated (UTC)
rivet-cloud	Ignored			Nov 5, 2025 4:06am
rivet-inspector	Ignored	Preview		Nov 5, 2025 4:06am
rivet-site	Ignored	Preview		Nov 5, 2025 4:06am

[NathanFlurry]

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

• chore(pegboard): allow configuring reschedule_backoff_max_exponent #3370
• fix(next-js): auto shut down runners when source code updates #3369
• fix(rivetkit): fix duplicate runner key in serverless #3366
• chore(runner): improve runner connection logs #3365 👈 (View in Graphite)
• chore(rivetkit): add runner sse ping interval #3362
• chore(rivetkit): log close code when client ws closes #3361
• chore(rivetkit): log runner id with actor logs #3360
• chore(scripts): add endian converter script #3359
• fix(guard): fix url decoding actor id + token #3357
• chore(rivetkit): remove unneeded headers & protocols in requests #3356
• fix(rivetkit): reduce pegboard actor retry for spawned engines #3355
• chore(pegboard): allow configuring base_retry_timeout, actor_start_threshold, actor_stop_threshold, and retry_reset_duration #3354
• feat(pb): add reschedule_ts to actors #3363
• fix: fix all dashboards #3358
• chore: change log levels #3333
• chore: misc bug fixes, add logs for outbound req #3332
• fix: use correct vbare functions for runner tunnel #3300 : 1 other dependent PR (#3301 )
• feat(grafana): fix api dashboards #3193
• fix: docker compose build #3273
• chore: fix remaining warnings for all targets #3305
• fix(gas): fix from_workflow when creating standalone ctx from op ctx #3342
• ci: udpate ci filter to succeed if no relevant changes #3343
• main

---

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]

Pull Request Review: chore(runner): improve runner connection logs

Summary

This PR improves observability in the TypeScript runner SDK by adding detailed connection logging and refactoring the endpoint getter.

Positive Aspects ✅

1. Clean refactoring: Extracting pegboardEndpoint into a getter (lines 452-454) follows DRY principles and improves code reusability
2. Better observability: The new connection log (lines 477-483) provides valuable debugging information including endpoint, namespace, runnerKey, and token presence
3. Style consistency: Changing "Connected" → "connected" (line 486) correctly follows CLAUDE.md guidelines for lowercase log messages
4. Structured logging: Properly uses structured logging with field parameters instead of string interpolation ✨

Security Consideration ⚠️

Logging sensitive data (line 481):

runnerKey: this.#config.runnerKey,

The runnerKey is being logged in plain text. Consider whether this should be truncated/masked in production:

runnerKey: this.#config.runnerKey.substring(0, 8) + '...',
// or
runnerKey: '***redacted***',

While helpful for debugging, if this key grants access to runner operations and logs are accessible to unauthorized users, this could be a security concern.

Good practice (line 482):

hasToken: !!this.config.token,

Correctly logs token presence without exposing the actual value.

Performance & Code Quality

• No performance concerns - getter is lightweight
• Code is clean and maintainable
• Follows existing patterns in the codebase

Test Coverage

No tests included, which is acceptable for a logging-only change. If desired, you could add tests to verify:

• The connection log is emitted with expected fields
• The getter returns the correct endpoint value

Recommendation

Approve with suggestion: This is a solid improvement to observability. Consider masking/truncating the runnerKey in logs if it's sensitive in your threat model.

---

Review generated by Claude Code

[claude]

Code Review

Summary

This PR improves the logging for runner connections by adding more detailed connection information and extracting the endpoint logic into a reusable getter. Overall, this is a good quality-of-life improvement for debugging connection issues.

Positive Changes ✅

1. Better observability: The new log before connection provides valuable debugging information (endpoint, namespace, runnerKey, token presence)
2. Code reuse: Extracting pegboardEndpoint as a getter eliminates duplication and makes the endpoint accessible elsewhere if needed
3. Consistent casing: Changing "Connected" to "connected" aligns with the lowercase convention used throughout the file (e.g., "stopping all actors", "starting runner", "connection closed")

Security Concern 🔒

Issue: The PR logs runnerKey in plain text (line 481):

runnerKey: this.#config.runnerKey,

Risk: The runnerKey appears to be a credential/secret used for authentication (it's passed as a query parameter in the WebSocket URL and is part of the authentication flow). Logging it in plain text could expose it in log aggregation systems, making it accessible to unauthorized parties.

Recommendation: Either:

1. Omit the runnerKey from logs entirely
2. Log only a prefix or hash (e.g., runnerKey: this.#config.runnerKey.substring(0, 8) + '...',)
3. If it's truly not sensitive, document why it's safe to log

Note that hasToken is correctly implemented - it only logs the presence of a token, not its value.

Minor Suggestions

1. Consider adding more context: You might also want to log the WebSocket URL (with credentials redacted) to help debug endpoint configuration issues
2. Error logging: Consider adding a log in the error event listener (if not already present) to capture connection failures with the same context

Code Quality: Good ✨

• Clean refactoring with the new getter
• Follows existing patterns in the codebase
• Improves maintainability

Verdict: Approve pending security fix for runnerKey logging.

[claude]

Code Review for PR #3365: chore(runner): improve runner connection logs

Summary

This PR improves the runner connection logs by:

1. Extracting pegboardEndpoint into a getter method for reusability
2. Adding detailed logging before WebSocket connection attempt
3. Changing the "Connected" log message to lowercase "connected" for consistency

Overall Assessment

This is a solid improvement to observability. The changes are focused, well-structured, and follow good logging practices. ✅

---

Detailed Feedback

Code Quality & Best Practices ✅

1. Good refactoring of pegboardEndpoint

get pegboardEndpoint() {
    return this.#config.pegboardEndpoint || this.#config.endpoint;
}

• Extracting this into a getter eliminates code duplication
• Makes the code more maintainable
• Follows DRY principle

2. Structured logging with Pino ✅
The new logging follows Pino's structured logging pattern correctly:

logger()?.info({
    msg: "connecting",
    endpoint: this.pegboardEndpoint,
    namespace: this.#config.namespace,
    runnerKey: this.#config.runnerKey,
    hasToken: !!this.config.token,
});

This is good practice for log aggregation and searching.

3. Log message consistency ✅
Changing "Connected" → "connected" follows the existing convention in the codebase where log messages are lowercase (verified by checking other logger calls).

---

Potential Issues & Concerns

🔴 SECURITY CONCERN: runnerKey in logs

Issue: The runnerKey is being logged in plain text at line 481.

runnerKey: this.#config.runnerKey,  // ⚠️ Potential credential exposure

Risk Assessment:

• runnerKey appears to be a credential used for authentication (line 464: passed in URL query params)
• Logging credentials can lead to exposure in log files, log aggregation systems, or monitoring tools
• This violates security best practices for credential handling

Recommendation:

// Option 1: Hash or redact the key
runnerKey: this.#config.runnerKey.slice(0, 8) + "...",

// Option 2: Only log presence
hasRunnerKey: !!this.#config.runnerKey,

// Option 3: Remove entirely if not needed for debugging

Context check needed: If runnerKey is not actually sensitive (e.g., it's just a namespace identifier like "default" from the test file at line 129), then this concern can be disregarded. However, the name suggests it's a key/credential.

---

🟡 Minor: Redundant getter access

Location: Line 482

hasToken: !!this.config.token,

Issue: Uses this.config (public getter) instead of this.#config (private field) like the other properties in the same log statement.

Impact: Minimal - just a minor inconsistency. Both work correctly.

Suggestion: For consistency, use this.#config.token to match lines 479-481.

---

Performance Considerations ✅

• The new getter pegboardEndpoint is lightweight and called only when needed
• No performance concerns with the additional logging
• Logging happens before connection attempt, so no impact on connection latency

---

Test Coverage

Current state:

• The main test file (tests/lifecycle.test.ts) is completely commented out
• No active tests verify this logging behavior
• No tests verify the pegboardEndpoint getter

Recommendation:
While this is a simple change, consider:

1. Enabling the lifecycle tests to catch regressions
2. Adding a unit test for the pegboardEndpoint getter to verify the fallback logic

Note: This is not blocking for this PR, but something to consider for the overall project health.

---

Summary of Recommendations

1. 🔴 HIGH PRIORITY: Review whether runnerKey should be logged. If it's a credential, redact or hash it.
2. 🟡 LOW PRIORITY: Use this.#config.token instead of this.config.token for consistency (line 482)
3. Future work: Consider re-enabling the lifecycle tests

---

Verdict

Approve with minor security concern to address. ✅

The code quality is good, follows best practices for structured logging, and improves observability. The only blocking concern is the potential credential logging issue with runnerKey - please verify whether this is sensitive information before merging.

Nice work on improving the debugging experience! 👍

[claude]

Code Review

Summary

This PR improves logging for the runner's connection process by adding structured diagnostic information before the WebSocket connection attempt and standardizing log message casing.

Positive Aspects ✅

1. Better Diagnostics: The new logging before connection provides valuable debugging information including endpoint, namespace, runner key, and token presence.

2. Code Organization: Extracting pegboardEndpoint into a getter improves code reusability and readability.

3. Consistent Logging Style: Changing "Connected" to "connected" aligns with the lowercase convention used elsewhere in the file (e.g., "starting runner" at line 266, "received commands" at line 708).

4. Structured Logging: Using structured fields instead of string interpolation follows best practices and makes logs easier to parse and query.

Suggestions for Improvement 💡

Security Consideration

The PR logs runnerKey in plaintext. While this may be intentional for debugging, consider:

logger()?.info({
    msg: "connecting",
    endpoint: this.pegboardEndpoint,
    namespace: this.#config.namespace,
    runnerKey: this.#config.runnerKey.substring(0, 8) + '...',  // Log prefix only
    hasToken: !!this.config.token,
});

Question: Is it safe to log the full runnerKey in production environments? If these logs are aggregated or stored, this could be a security risk.

Minor: Property Access Inconsistency

Line 482 uses this.config.token (public getter) while line 481 uses this.#config.runnerKey (private field). For consistency, consider:

hasToken: !!this.#config.token,  // Use private field like other properties

Or use the public getter for all:

hasToken: !!this.config.token,

Test Coverage 📋

No tests were found for this module. While logging changes are low-risk, consider:

• Integration tests that verify connection flow
• Tests ensuring sensitive data isn't logged inappropriately

Verdict

Approve with minor suggestions ✓

The changes improve observability and follow good logging practices. The security consideration around logging runnerKey should be addressed, but the overall change is beneficial and well-implemented.

[claude]

PR Review: chore(runner): improve runner connection logs

Summary

This PR improves connection logging by adding more detailed information when the runner connects to pegboard, and extracts a getter method for the pegboard endpoint. The changes are focused and align with the stated goal of improving observability.

Code Quality ✅

Positive aspects:

• Clean refactoring of pegboardEndpoint into a getter method (lines 452-454), which improves code reusability
• The new logging follows structured logging patterns correctly using tracing-style object notation
• Log message capitalization follows the codebase convention (lowercase "connecting" and "connected")
• The refactoring maintains backward compatibility

Best Practices ✅

Logging conventions:

• ✅ Uses structured logging properly: logger()?.info({ msg: "connecting", endpoint: ..., namespace: ..., ... })
• ✅ Follows the convention of lowercase log messages ("connecting", "connected")
• ✅ Includes useful context: endpoint, namespace, runnerKey, and hasToken flag
• Note: The hasToken field uses a boolean flag rather than exposing the actual token value, which is good for security

Potential Issues 🔍

Minor considerations:

1. Sensitive data in logs (lines 477-483): While the actual token value is not logged (good!), the runnerKey is logged in plaintext. Consider whether this should be redacted or hashed in production environments, depending on its sensitivity.

2. Getter performance: The new pegboardEndpoint getter (line 452-454) is called once in pegboardUrl getter. This is fine, but be aware that getters are recalculated on each access. Since the config is immutable, this isn't a concern here.

3. Consistency opportunity: The "Connected" → "connected" change on line 486 is good for consistency. Consider doing a broader audit of log messages in this file to ensure all follow the lowercase convention.

Test Coverage ⚠️

• No test files were found for the runner module
• Recommendation: Consider adding tests for:
  • The new pegboardEndpoint getter
  • Logging behavior (verify expected log output)
  • Connection flow with various configurations (with/without token, custom pegboard endpoint)

Performance ✅

• No performance concerns
• The refactoring is performance-neutral (same number of string operations)
• Logging overhead is minimal and only occurs during connection establishment

Security ✅

Good practices:

• Token value is not logged (only hasToken: !!this.config.token)
• This prevents accidental token exposure in logs

Consider:

• As mentioned above, evaluate whether runnerKey should be redacted in logs

Suggestions

1. Optional: Add JSDoc comments for the new getter to improve code documentation

2. Optional: Centralize connection info logging - Consider creating a helper method if similar diagnostic logs are needed elsewhere

Conclusion ✅

This is a solid, focused improvement to the codebase. The changes are minimal, well-scoped, and improve observability without introducing bugs or breaking changes. The code follows existing conventions and best practices.

Recommendation: Approve with minor suggestions for future improvements.

---

Review completed by Claude Code

[claude]

PR Review: chore(runner): improve runner connection logs

Summary

This PR improves logging during the runner WebSocket connection process by adding more detailed structured logging before the connection attempt and adjusting log message casing for consistency.

Code Quality ✅

Positive aspects:

• Follows the repository's structured logging conventions from CLAUDE.md (using tracing-style structured logging with separate parameters)
• Good refactoring by extracting pegboardEndpoint as a getter property, reducing code duplication
• Log message casing change from "Connected" to "connected" aligns with the codebase convention of lowercase log messages

Structured Logging Pattern:

logger()?.info({
    msg: "connecting",
    endpoint: this.pegboardEndpoint,
    namespace: this.#config.namespace,
    runnerKey: this.#config.runnerKey,
    hasToken: !!this.config.token,
});

This follows best practices by:

• Using lowercase message
• Separating data into structured fields rather than string interpolation
• Including relevant diagnostic information

Observations & Suggestions

1. Information Disclosure Consideration ⚠️

The new log includes runnerKey in plain text. While this is helpful for debugging, consider:

• Is the runnerKey sensitive? Should it be redacted/hashed in logs?
• The log already includes hasToken as a boolean (good practice), but runnerKey is logged in full

Recommendation: If runnerKey is sensitive, consider logging only a prefix or hash:

runnerKey: this.#config.runnerKey?.substring(0, 8) + '...',
// or
runnerKeyHash: someHashFunction(this.#config.runnerKey),

2. Consistency: Property Access Pattern 🔍

Line 482 uses this.config.token while line 481 uses this.#config.runnerKey. Since both are from the same config:

hasToken: !!this.config.token,  // Uses public getter
runnerKey: this.#config.runnerKey,  // Uses private field

Recommendation: Use consistent access pattern - either both via the public getter or both via private field:

hasToken: !!this.#config.token,

3. Log Timing 💭

The "connecting" log is placed after creating the WebSocket but before attaching event listeners. This is fine, but be aware:

• If WebSocket constructor throws, this log won't appear
• Consider if logging before new WS() would be more helpful for debugging connection issues

Test Coverage

This is a logging-only change with no functional impact, so additional tests aren't necessary. However, if you have integration tests that validate log output, consider adding assertions for the new log structure.

Performance Considerations ✅

• Minimal performance impact
• The getter pegboardEndpoint is a simple ternary operation, negligible overhead
• No additional blocking operations introduced

Security Concerns

See point #1 above regarding runnerKey disclosure in logs. Otherwise, no security issues identified.

Final Verdict

Approval recommended with minor consideration for the runnerKey logging sensitivity.

The changes improve observability and follow the codebase conventions well. The refactoring to extract pegboardEndpoint as a getter is a nice touch that improves code maintainability.

---

Suggested improvements (optional):

1. Review whether runnerKey should be redacted/hashed in logs
2. Use consistent config access pattern (this.#config vs this.config)

Great work on improving the logging infrastructure! 👍

[claude]

PR Review: chore(runner): improve runner connection logs

Summary

This PR improves logging for runner connection lifecycle by:

1. Extracting pegboardEndpoint as a getter for reusability
2. Adding structured logging before connection attempt with key diagnostic information
3. Making log message casing consistent with codebase conventions

Code Quality & Best Practices ✅

Strengths:

• DRY Principle: Extracting pegboardEndpoint as a getter eliminates code duplication and improves maintainability
• Structured Logging: Follows the existing pattern of using structured logging with logger()?.<level>({ msg: "...", ... }) consistently throughout the file
• Lowercase Messages: The change from "Connected" to "connected" aligns with the codebase convention (see line 266: "starting runner", line 654: "connection closed", etc.)
• Useful Diagnostic Info: The new log includes essential connection parameters that would be valuable for debugging connection issues

Suggested Improvements:

1. Potential Sensitive Data Logging ⚠️

   runnerKey: this.#config.runnerKey,
   hasToken: !!this.config.token,

   • Logging runnerKey directly could be a security concern if this is sensitive credential material
   • Consider logging only a prefix/hash: runnerKey: this.#config.runnerKey.substring(0, 8) + '...'
   • Or redact it entirely depending on sensitivity

2. Inconsistent Property Access

   hasToken: !!this.config.token,  // Uses this.config

   vs.

   namespace: this.#config.namespace,  // Uses this.#config
   runnerKey: this.#config.runnerKey,

   • Line 482 uses this.config.token (public) while others use this.#config (private)
   • Should be consistent - likely this.#config.token for consistency
   • This works because there's a public config getter (line 149), but mixing access patterns reduces clarity

Potential Bugs 🔍

No bugs detected. The refactoring is straightforward and maintains existing behavior.

Performance Considerations ⚡

• Adding one extra log call has negligible performance impact
• The getter adds minimal overhead and is only called during connection setup (infrequent operation)

Security Concerns 🔒

1. Runner Key Logging: As mentioned above, verify that runnerKey is safe to log in plaintext. If it's a sensitive credential:

   • Redact or hash it
   • Or ensure logs are properly secured/access-controlled

2. Namespace Logging: Appears safe as this is typically non-sensitive configuration

Test Coverage 📋

• No test files found for the runner SDK (**/*runner*test*.ts)
• This is a logging-only change with minimal risk, but consider:
  • Integration tests that verify connection logging occurs
  • Tests that validate sensitive data is properly redacted from logs

Minor Observations

• The change is well-scoped and focused on improving observability
• The connection flow now logs:
  1. "connecting" with diagnostic details (NEW - helpful!)
  2. "connected" on successful WebSocket open (existing, improved casing)
  3. "connection closed" on WebSocket close (existing)

Recommendation

Approve with minor suggestions:

• ✅ Code structure improvement is good
• ⚠️ Review whether runnerKey should be redacted from logs
• ⚠️ Fix inconsistent property access pattern (this.config.token → this.#config.token)

The logging improvements will make connection debugging much easier. Nice work! 🎉

[graphite-app]

Merge activity

• Nov 5, 5:07 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
• Nov 5, 5:08 AM UTC: CI is running for this pull request on a draft pull request (#3371) due to your merge queue CI optimization settings.
• Nov 5, 5:11 AM UTC: Merged by the Graphite merge queue via draft PR: #3371.