Skip to content
This repository was archived by the owner on Oct 22, 2025. It is now read-only.

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@graphite-app
Copy link

graphite-app bot commented Oct 20, 2025

Merge activity

  • Oct 20, 3:11 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Oct 20, 3:11 AM UTC: CI is running for this pull request on a draft pull request (#1413) due to your merge queue CI optimization settings.
  • Oct 20, 3:12 AM UTC: Merged by the Graphite merge queue via draft PR: #1413.

@vercel
Copy link

vercel bot commented Oct 20, 2025

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

Project Deployment Preview Comments Updated (UTC)
rivetkit-next-js-max Ready Ready Preview Comment Oct 20, 2025 3:21am
rivetkit-serverless Ready Ready Preview Comment Oct 20, 2025 3:21am

💡 Enable Vercel Agent with $100 free credit for automated AI reviews

Copy link
Member Author

NathanFlurry commented Oct 20, 2025


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • merge queue - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

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 pushed a commit that referenced this pull request Oct 20, 2025
@graphite-app graphite-app bot closed this Oct 20, 2025
@graphite-app graphite-app bot deleted the 10-19-chore_core_auto-start_engine_in_development_for_serverless_runners_disable_health_check_in_serverless branch October 20, 2025 03:12
@claude
Copy link

claude bot commented Oct 20, 2025

Pull Request Review

Summary

This PR improves the developer experience for serverless runners by automatically enabling the engine and auto-configuration in development mode, and by disabling health checks for serverless runners (since the endpoint isn't configured until runtime).

Code Quality & Best Practices ✅

Strengths:

  • Clear separation of concerns with logical grouping of serverless-specific configuration
  • Good use of guard conditions to prevent misuse (lines 78-81 in registry/mod.ts)
  • Follows the project's logging conventions (lowercase messages, structured logging)
  • Proper error handling that doesn't crash the process when serverless configuration fails

Suggestions:

  1. Type Safety Improvement (packages/rivetkit/src/registry/mod.ts:88-90)

    • The condition checks inputConfig?.runEngine === undefined but then sets config.runEngine = true. Since inputConfig might be undefined, consider being more explicit:
    if (inputConfig?.runEngine === undefined) config.runEngine = true;
    // Could be:
    if (!inputConfig || inputConfig.runEngine === undefined) config.runEngine = true;

    However, the current approach works because of optional chaining. Just note that this sets config properties even when inputConfig is undefined.

  2. Configuration Logic (packages/rivetkit/src/registry/mod.ts:88-90)

    • There's a subtle issue: you're checking inputConfig?.runEngine but setting config.runEngine. If inputConfig.runEngine is explicitly set to false, this code will not override it (which is correct). However, the check happens after RunnerConfigSchema.parse(inputConfig) which may have already set defaults. Consider whether you want to check against the parsed config or the raw input.

Potential Bugs 🐛

  1. Auto-configuration Race Condition (packages/rivetkit/src/registry/mod.ts:213-217)

    • The configureServerlessRunner function is called after Promise.all(readyPromises), but there's no guarantee the endpoint is configured before the first request arrives. The error message suggests restarting the process, but this could be jarring in development.
    • Recommendation: Consider adding a retry mechanism or queueing requests until configuration completes.
  2. Error Swallowing (packages/rivetkit/src/registry/mod.ts:313-320)

    • The error in configureServerlessRunner is caught and logged but not thrown. While this prevents crashes, it means the application continues running in a potentially broken state.
    • Recommendation: Add a state flag to track configuration status and return meaningful errors to requests that arrive before configuration succeeds.

Performance Considerations ⚡

  1. Promise Chaining (packages/rivetkit/src/registry/mod.ts:206-209, 213-217)
    • Both actor driver initialization and serverless configuration wait for readyPromises, which is good. However, they run sequentially when they could potentially run in parallel:
    Promise.all(readyPromises).then(async () => {
      await Promise.all([
        !config.disableActorDriver && driver.actor(...),
        config.runnerKind === "serverless" && config.autoConfigureServerless && configureServerlessRunner(config)
      ].filter(Boolean));
    });
    • This is a minor optimization since serverless configuration is typically fast.

Security Concerns 🔒

No major security issues identified. The changes are mostly configuration-related and don't introduce new attack vectors.

Minor observations:

  • The health check bypass for serverless is appropriate since the endpoint isn't known upfront
  • Error messages don't leak sensitive information

Test Coverage 🧪

Missing test coverage for new functionality:

  1. Serverless Auto-configuration

    • No tests verify that disableHealthCheck is automatically set when runnerKind === "serverless"
    • No tests verify that runEngine and autoConfigureServerless are enabled in non-production environments
  2. Error Handling

    • No tests verify behavior when configureServerlessRunner fails
    • No tests verify that the application continues running after configuration failure

Recommendations:

  • Add unit tests for the auto-configuration logic in registry/mod.ts:74-91
  • Add integration tests to verify serverless runner behavior in development vs production
  • Add tests for the error handling path in configureServerlessRunner

Additional Observations 📝

  1. Consistency (packages/rivetkit/src/registry/mod.ts:88-90)

    • The auto-configuration logic checks process.env.NODE_ENV !== "production", which is consistent with Next.js patterns but may not work in all environments (e.g., some use NODE_ENV=prod or development). Consider documenting this assumption.
  2. Documentation

    • The inline comments are helpful, but consider adding a note in the README or migration guide about this breaking change (removal of explicit endpoint and disableHealthCheck configuration for serverless runners).
  3. Example Simplification (examples/counter-serverless/src/server.ts)

    • The removal of endpoint: "http://localhost:6420" is a good simplification. Consider documenting in the example's README that the engine is auto-started in development.

Verdict

This is a solid improvement that simplifies the developer experience for serverless runners. The main concerns are:

  1. Missing test coverage
  2. Error handling that swallows failures silently
  3. Minor type safety considerations

Recommendation: Approve with suggestions to add tests and improve error handling in a follow-up PR.


🤖 Generated with Claude Code

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant