Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Nov 6, 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 6, 2025 5:37am
3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 6, 2025 5:37am
rivet-inspector Ignored Ignored Preview Nov 6, 2025 5:37am
rivet-site Ignored Ignored Preview Nov 6, 2025 5:37am

Copy link
Member Author

NathanFlurry commented Nov 6, 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 6, 2025

PR Review: Add explicit close reason when client runner shuts down

Summary

This PR improves logging clarity by adding a structured close reason (pegboard.runner_shutdown) when the runner initiates a shutdown, and filtering out noisy warnings for expected shutdown events.

Code Quality & Best Practices

Strengths:

  • ✅ Follows the established error code pattern ({group}.{error}) used elsewhere in the codebase (e.g., ws.eviction, ws.downstream_closed)
  • ✅ Improves observability by distinguishing intentional shutdowns from unexpected disconnections
  • ✅ Reduces log noise by converting expected shutdown warnings to info-level logs
  • ✅ Consistent with the structured logging approach using the parseWebSocketCloseReason utility

Suggestions:

  1. Consider adding documentation for error codes - Since this introduces a new error code (pegboard.runner_shutdown), it might be helpful to document all WebSocket close reasons in one place (e.g., in utils.ts or a dedicated constants file). This would help maintainers understand the full set of close reasons.

  2. Structured logging inconsistency - On line 684, the PR changes to this.log?.info("runner websocket evicted") without structured fields, which is good. However, on line 694, it changes to this.log?.info("runner shutdown") also without fields. For consistency with the CLAUDE.md logging guidelines, consider keeping the structured format:

    this.log?.info({
        msg: "runner shutdown",
        code: ev.code,
        reason: ev.reason.toString(),
    });

    This would maintain consistency with the remaining warning case and provide more debugging context.

Potential Issues

Minor concerns:

  1. Server-side handling - I searched for runner_shutdown in the backend Rust code and found no references. This is likely fine since it's a client-initiated close, but verify that the server correctly handles this close reason and doesn't log it as an error. The close code 1000 (Normal) should signal this is expected.

  2. Test coverage - The existing test file (lifecycle.test.ts) is completely commented out. While this PR doesn't require new tests (it's mainly a logging improvement), it would be beneficial to:

    • Uncomment and fix the existing tests as a follow-up
    • Add a test case that verifies the close reason is set correctly during shutdown

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.

Recommendations

Priority: Low

  • Consider the structured logging suggestion above for consistency
  • Document the new error code in the error catalog (if one exists)

Priority: Very Low

  • Add test coverage for shutdown scenarios (can be addressed in a follow-up PR)
  • Verify server-side logging doesn't treat this as an error condition

Overall Assessment

This 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

@pkg-pr-new
Copy link

pkg-pr-new bot commented Nov 6, 2025

More templates

@rivetkit/actor

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

@rivetkit/cloudflare-workers

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

@rivetkit/core

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: 8cf4830

@claude
Copy link

claude bot commented Nov 6, 2025

Code Review

This 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

  1. Improved Observability: Adding the explicit pegboard.runner_shutdown close reason makes it easier to distinguish intentional shutdowns from unexpected disconnections in logs and monitoring.

  2. Consistent Pattern: The close reason follows the established format {group}.{error} that's already used elsewhere (e.g., ws.eviction), making it compatible with the parseWebSocketCloseReason() utility at engine/sdks/typescript/runner/src/utils.ts:50.

  3. Better Logging: The change from structured logging with multiple fields to simpler messages ("runner websocket evicted" and "runner shutdown") is more concise for these info-level events. The warning log for unexpected disconnections still retains the structured format with useful context.

  4. Graceful Shutdown Handling: The close reason is consistently applied in both immediate shutdown (mod.ts:389) and graceful shutdown paths (mod.ts:434).

🔍 Observations

  1. Logging Consistency: The PR reduces structured logging for the eviction case (from engine/sdks/typescript/runner/src/mod.ts:684) but keeps structured logging for unexpected disconnections. This appears intentional - info-level expected events get simple messages, while warnings for unexpected cases get full context.

  2. Missing Fields: In the new "runner shutdown" log at mod.ts:694, the removal of namespace and runnerName fields is reasonable since this is an expected shutdown, though it could make debugging slightly harder if you need to correlate logs across multiple runners.

💡 Minor Suggestions

  1. Test Coverage: Consider adding a test case that verifies the close reason is correctly set during shutdown. The test file at engine/sdks/typescript/runner/tests/lifecycle.test.ts is currently commented out, but when active, it would be a good place to verify the close reason.

  2. Documentation: While not strictly necessary, a comment near the close reason string explaining why this specific format was chosen could help future maintainers understand the pattern.

🎯 Verdict

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

@claude
Copy link

claude bot commented Nov 6, 2025

PR Review

Summary

This PR improves logging clarity by adding an explicit close reason pegboard.runner_shutdown when the client runner intentionally shuts down, distinguishing it from unexpected disconnections.

Positive Aspects

Improved observability: The change makes it easier to distinguish intentional shutdowns from unexpected disconnections in logs
Consistent error format: Follows the existing group.error pattern used elsewhere (e.g., ws.eviction)
Clean implementation: The change is minimal and focused, affecting only the necessary touch points

Code Quality

Good:

  • The close reason format pegboard.runner_shutdown correctly follows the established pattern that parseWebSocketCloseReason expects (group.error#rayId)
  • The new handling path at lines 690-694 properly logs runner shutdown at info level, which is appropriate for an expected shutdown
  • The change is applied consistently to both immediate and graceful shutdown paths

Observation:

  • The nested if-else structure (lines 690-702) creates three branches:
    1. eviction (lines 690-688)
    2. runner_shutdown (lines 690-694)
    3. everything else (lines 695-702)

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 Patterns

The logging changes follow the project's conventions from CLAUDE.md:

  • ✅ Uses lowercase log messages (runner shutdown, runner websocket evicted)
  • ✅ Uses structured logging with tracing where appropriate
  • ⚠️ Line 684 uses a string parameter ("runner websocket evicted") which is fine for simple cases, but line 696-701 properly uses structured logging with field parameters

Potential Issues

Minor concerns:

  1. No tests for shutdown behavior: The test file lifecycle.test.ts is entirely commented out, so there's no coverage for the shutdown behavior. While this isn't introduced by this PR, it would be valuable to have tests that verify:

    • The correct close reason is sent during shutdown
    • The appropriate log message is generated
    • No warnings are logged for intentional shutdowns
  2. Line 684 inconsistency: The eviction path uses a simple string log message while the other paths use structured logging. Consider:

    this.log?.info({ msg: "runner websocket evicted" });

    for consistency, though the current approach is acceptable.

Security & Performance

  • ✅ No security concerns
  • ✅ No performance impact
  • ✅ WebSocket close code 1000 (normal closure) is appropriate

Recommendations

  1. Consider adding tests (future work, not blocking): When the test suite is re-enabled, add coverage for:

    • Graceful shutdown logging behavior
    • Immediate shutdown logging behavior
    • Verification that pegboard.runner_shutdown is the close reason
  2. Optional consistency improvement: Consider using structured logging for the eviction case at line 684 for consistency with other log calls.

Verdict

Approved

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.

@NathanFlurry NathanFlurry force-pushed the 11-05-chore_runner_add_explicit_close_reason_when_client_runner_shuts_down branch from 9283d04 to 8b6c64b Compare November 6, 2025 02:16
@NathanFlurry NathanFlurry force-pushed the 11-05-fix_guard_fix_1006_when_client_closes_ws_connection branch from 4078059 to 04f4430 Compare November 6, 2025 02:16
@claude
Copy link

claude bot commented Nov 6, 2025

PR Review: Add explicit close reason when client runner shuts down

Summary

This PR improves observability by adding an explicit close reason (pegboard.runner_shutdown) when the runner shuts down, replacing the generic "Stopping" message. This allows the close event handler to distinguish between intentional shutdowns and unexpected disconnections.

Code Quality ✅

Strengths:

  • Improved observability: The structured close reason (pegboard.runner_shutdown) follows the existing pattern of {group}.{error} format used by parseWebSocketCloseReason
  • Consistent logging: The change properly distinguishes between intentional shutdown (info level) and unexpected disconnections (warn level)
  • Clean implementation: The logic correctly checks for the shutdown reason before logging warnings about disconnections

Best Practices:

  • Follows the existing error format convention (group.error pattern)
  • Uses appropriate log levels (info for expected shutdown, warn for unexpected disconnect)
  • Maintains consistency across both immediate and graceful shutdown paths

Potential Issues 🔍

Minor Concerns:

  1. Logging consistency (lines 684, 694):

    • Line 684: this.log?.info("runner websocket evicted") - simplified format
    • Line 694: this.log?.info("runner shutdown") - simplified format
    • Line 696-701: Still using structured logging with object format

    Consider keeping consistent structured logging format across all cases for better log parsing:

    this.log?.info({ msg: "runner shutdown" });
  2. Missing context in shutdown log (line 694):
    The shutdown log could include the namespace/runner name for better debugging in production:

    this.log?.info({
        msg: "runner shutdown",
        namespace: this.#config.namespace,
        runnerName: this.#config.runnerName,
    });
  3. Code duplication:
    The close reason "pegboard.runner_shutdown" is duplicated in two places (lines 389, 434). Consider extracting to a constant:

    const RUNNER_SHUTDOWN_REASON = "pegboard.runner_shutdown";

Performance ✅

No performance concerns. The changes are minimal and only affect shutdown paths.

Security ✅

No security concerns identified.

Test Coverage ⚠️

Observation:
The test file tests/lifecycle.test.ts is currently commented out entirely. While this PR is a small logging improvement that doesn't require immediate test coverage, consider:

  1. Re-enabling and updating the lifecycle tests to verify the shutdown behavior
  2. Adding a test case that verifies the close reason is set correctly during shutdown
  3. Testing that the log message distinguishes between eviction, shutdown, and unexpected disconnect

Suggested test case:

it("should use correct close reason on shutdown", async () => {
  const runner = new Runner(config);
  runner.start();
  await runnerStarted.promise;
  
  // Capture close event
  const closePromise = new Promise((resolve) => {
    runner.onDisconnected = (code, reason) => {
      resolve({ code, reason });
    };
  });
  
  await runner.shutdown(true);
  const { reason } = await closePromise;
  expect(reason).toBe("pegboard.runner_shutdown");
});

Recommendations

Optional improvements:

  1. Extract the close reason to a constant to avoid duplication
  2. Use structured logging consistently across all log statements
  3. Add namespace/runner context to shutdown log for better debugging
  4. Consider re-enabling tests to verify shutdown behavior

Overall Assessment:Approve with minor suggestions

This is a clean, focused improvement that enhances observability during runner shutdown. The code follows existing patterns and conventions. The suggested improvements are minor and optional.

@claude
Copy link

claude bot commented Nov 6, 2025

PR Review

Summary

This PR improves logging clarity by adding an explicit close reason (pegboard.runner_shutdown) when the client runner shuts down, and adjusts the logging behavior to reduce noise during normal shutdown operations.

Code Quality ✅

Positive aspects:

  • Consistent error code format: The new pegboard.runner_shutdown reason follows the established {group}.{error} pattern used by parseWebSocketCloseReason (as seen in utils.ts:42-66)
  • Improved logging clarity: Differentiates between normal shutdown (info level) and unexpected disconnections (warn level)
  • Follows CLAUDE.md conventions: Lowercase log messages with structured logging parameters

Code changes:

  1. Lines 389, 434: Changed WebSocket close reason from generic "Stopping" to structured "pegboard.runner_shutdown"
  2. Lines 684, 694: Simplified logging for eviction case (removed redundant structured fields)
  3. Lines 690-702: Added conditional logic to log normal shutdowns at info level instead of warning

Potential Issues & Suggestions

1. Missing namespace and runnerName fields 🔍

In the eviction case (line 684), the simplified logging lost the namespace and runnerName fields that were present in the warn case. While this reduces noise, it might make debugging harder in production.

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 (this.log?.info("runner websocket evicted")) while line 694 also uses string-only format. However, the CLAUDE.md guidelines suggest using structured logging with objects.

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 1000 indicates a normal closure. This is correct, but it might be helpful to add a comment explaining why both immediate and graceful shutdowns use the same 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 ⚠️

Missing: No test changes included. Consider adding tests to verify:

  1. The close reason is correctly set to "pegboard.runner_shutdown"
  2. The parseWebSocketCloseReason function correctly parses the new reason
  3. Appropriate log levels are used for different shutdown scenarios

Suggested test locations:

  • Unit tests for the shutdown logic with different scenarios (immediate vs graceful)
  • Integration tests verifying the WebSocket close reason is propagated correctly

Minor Observations

  1. Line 684: "websocket" could be "WebSocket" for consistency with other messages (e.g., line 432: "closing WebSocket")
  2. Line 694: Consider adding structured fields here too if you want to track shutdown frequency/patterns

Verdict: ✅ Approve with minor suggestions

This is a solid improvement that makes the logging more informative and reduces log noise. The main suggestions are:

  1. Maintain consistent structured logging format
  2. Consider retaining namespace and runnerName in eviction logs
  3. Add test coverage for the new close reason handling

The changes are backward compatible and follow the existing error code patterns in the codebase.

@NathanFlurry NathanFlurry force-pushed the 11-05-fix_guard_fix_1006_when_client_closes_ws_connection branch from 04f4430 to d93280c Compare November 6, 2025 05:36
@NathanFlurry NathanFlurry force-pushed the 11-05-chore_runner_add_explicit_close_reason_when_client_runner_shuts_down branch from 8b6c64b to 8cf4830 Compare November 6, 2025 05:36
@claude
Copy link

claude bot commented Nov 6, 2025

PR Review: Add explicit close reason when client runner shuts down

Summary

This PR adds a more explicit close reason (pegboard.runner_shutdown) when the runner intentionally shuts down, allowing better distinction between intentional shutdowns and unexpected disconnections in the logs.

Code Quality ✅

Strengths:

  • Clear, focused change that improves observability
  • Consistent use of the new close reason in both immediate and graceful shutdown paths
  • Follows existing error code format (group.error) used by parseWebSocketCloseReason
  • Appropriate log level changes (info for intentional shutdown vs warn for unexpected disconnection)

Style:

  • Code follows existing patterns in the file
  • Log messages are appropriately lowercase per project conventions

Functionality ✅

What the change does:

  1. Replaces generic "Stopping" string with structured pegboard.runner_shutdown close reason
  2. Adds special handling in the close event listener to log intentional shutdowns at info level instead of warning level
  3. Reduces log noise by avoiding structured logging for the eviction case (line 684)

Correctness:

  • The close reason format matches the expected {group}.{error} pattern parsed by parseWebSocketCloseReason
  • Logic correctly identifies intentional shutdowns and handles them appropriately
  • The close code 1000 (normal closure) is correct for this scenario

Potential Issues 🤔

Minor concern:

  • The removal of namespace and runnerName fields from the warning log (lines 697-701) reduces debugging context. While this simplifies the log for the common case, it may make troubleshooting unexpected disconnections slightly harder. Consider whether these fields might be useful for debugging production issues.

Question:

  • Is there a corresponding server-side handler that expects this specific close reason format? It would be good to ensure the server recognizes pegboard.runner_shutdown as expected.

Testing ⚠️

Test coverage:

  • No test files found for the runner module
  • This change modifies shutdown behavior which is critical for system reliability
  • Recommendation: Consider adding integration tests for:
    • Immediate shutdown flow
    • Graceful shutdown flow
    • Verification that close reasons are properly set and parsed
    • Log output validation for different shutdown scenarios

Performance ✅

  • No performance concerns
  • Change is purely related to logging and close reason messaging

Security ✅

  • No security concerns identified
  • Close reasons are internal operational messages

Overall Assessment

Status: ✅ Approved with minor suggestions

This is a good quality change that improves observability. The code is clean and follows project conventions. The main areas for improvement are:

  1. Consider retaining namespace and runnerName in the warning log for unexpected disconnections
  2. Add tests for shutdown behavior if feasible

The change is safe to merge as-is, but the testing gap should be addressed in a follow-up if integration tests are within scope for this codebase.

@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 6, 2025

Merge activity

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

@graphite-app graphite-app bot closed this Nov 6, 2025
@graphite-app graphite-app bot deleted the 11-05-chore_runner_add_explicit_close_reason_when_client_runner_shuts_down branch November 6, 2025 05:50
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.

2 participants