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

feat(serverless): make runner 0 config#1335

Closed
abcxff wants to merge 1 commit into10-03-fix_core_fix_sec-websocket-protocol_in_denofrom
10-02-feat_serverless_make_runner_0_config
Closed

feat(serverless): make runner 0 config#1335
abcxff wants to merge 1 commit into10-03-fix_core_fix_sec-websocket-protocol_in_denofrom
10-02-feat_serverless_make_runner_0_config

Conversation

@abcxff
Copy link
Collaborator

@abcxff abcxff commented Oct 2, 2025

Fixes KIT-332 (merge with rivet-dev/rivet#3056)

@vercel
Copy link

vercel bot commented Oct 2, 2025

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

Project Deployment Preview Comments Updated (UTC)
rivetkit-serverless Error Error Oct 5, 2025 0:43am

@abcxff abcxff mentioned this pull request Oct 2, 2025
@claude
Copy link

claude bot commented Oct 2, 2025

Claude finished @abcxff's task —— View job


PR Review: feat(serverless): make runner 0 config

I've completed my review of this pull request that consolidates serverless configuration into a unified runner 0 config approach.

Todo List:

  • Read and analyze all 6 changed files
  • Understand the purpose of "runner 0 config" changes
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Assess performance considerations
  • Evaluate security concerns
  • Check test coverage implications
  • Provide comprehensive feedback

Summary

This PR removes the separate startServerless() method and consolidates serverless functionality into the main start() method using a runnerKind configuration parameter. The changes simplify the API by eliminating duplication between the two startup methods.

Code Quality & Best Practices ✅

Positive aspects:

  • API Simplification: Removing startServerless() reduces API surface area and eliminates code duplication
  • Consistent Configuration: Using runnerKind provides a clear way to distinguish between normal and serverless operation
  • Type Safety: The ServerlessActorDriverBuilder type is properly defined and used consistently
  • Error Messages: Good validation with clear error messages in packages/rivetkit/src/drivers/default.ts:17-21

Minor concerns:

  • Commented Code: packages/next-js/src/mod.ts:10 has commented out code that should be removed
  • Magic Numbers: Hard-coded limits like 32 actor IDs in packages/rivetkit/src/manager/router.ts:213 could be constants

Potential Bugs & Issues ⚠️

  1. Inconsistent Inspector Configuration (packages/rivetkit/src/registry/mod.ts:72-74):

    if (driver.name === "engine") {
        config.inspector.enabled = { manager: false, actor: true };  // Object
    }
    // vs the old startServerless code had:
    config.inspector.enabled = false;  // Boolean

    This type inconsistency could cause runtime issues if the inspector config expects a boolean in some contexts.

  2. Header Name Inconsistency (packages/rivetkit/src/manager/router.ts:123):

    • Changed from x-rivetkit-total-slots to x-rivet-total-slots
    • Ensure this aligns with client expectations and doesn't break existing integrations
  3. Missing Validation (packages/rivetkit/src/manager/router.ts:124-125):

    if (!Number.isFinite(totalSlots)) totalSlots = undefined;

    While this handles NaN, it doesn't validate reasonable ranges (e.g., negative numbers, zero)

Performance Considerations 🚀

Good:

  • Eliminates code duplication (~110 lines removed)
  • Single initialization path reduces complexity
  • Lazy actor driver creation for serverless scenarios

Concerns:

  • None significant - the changes are primarily structural

Security Concerns 🔒

Low Risk:

  • Token handling appears consistent with existing patterns
  • No new attack vectors introduced
  • Proper validation exists for endpoint requirements with serverless mode

Considerations:

  • Header-based configuration in serverless routes should be validated against trusted sources
  • Consider rate limiting for the /start endpoint if not already handled upstream

Test Coverage 📋

Areas needing test coverage:

  1. New runnerKind validation in chooseDefaultDriver()
  2. Serverless route parameter handling (token, totalSlots, runnerName, namespace)
  3. Migration path from startServerless() to start() with runnerKind: "serverless"
  4. Inspector configuration with different driver types and serverless mode
  5. Error handling for malformed headers in serverless endpoints

Recommendations

  1. Remove commented code in packages/next-js/src/mod.ts:10
  2. Add validation for totalSlots range (positive integers only)
  3. Verify inspector config types are consistent across all driver configurations
  4. Add tests for the new runnerKind functionality and serverless parameter handling
  5. Document the migration from startServerless() to start() with appropriate configuration
  6. Consider constants for magic numbers like the 32 actor ID limit

Overall Assessment

This is a well-executed refactoring that simplifies the API while maintaining functionality. The consolidation approach is sound, but there are some type consistency issues and missing test coverage that should be addressed before merging.

Recommendation: Approve with minor changes - Fix the inspector config type inconsistency and add validation for the totalSlots parameter.


Copy link
Collaborator Author

abcxff commented Oct 2, 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 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.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 2, 2025

More templates

@rivetkit/actor

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

@rivetkit/cloudflare-workers

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

@rivetkit/core

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

commit: f2bd09f

@claude
Copy link

claude bot commented Oct 2, 2025

Claude finished @abcxff's task —— View job


PR Review: feat(serverless): make runner 0 config

I've completed my review of this pull request that consolidates serverless configuration into a unified runner 0 config approach.

Todo List:

  • Read and analyze all 7 changed files
  • Understand the purpose of "runner 0 config" changes
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Assess performance considerations
  • Evaluate security concerns
  • Check test coverage implications
  • Provide comprehensive feedback

Summary

This PR removes the separate startServerless() method and consolidates serverless functionality into the main start() method using a runnerKind configuration parameter. The changes simplify the API by eliminating duplication between the two startup methods.

Code Quality & Best Practices ✅

Positive aspects:

  • API Simplification: Removing startServerless() reduces API surface area and eliminates code duplication
  • Consistent Configuration: Using runnerKind provides a clear way to distinguish between normal and serverless operation
  • Type Safety: The ServerlessActorDriverBuilder type is properly defined and used consistently
  • Error Messages: Good validation with clear error messages in packages/rivetkit/src/drivers/default.ts:17-21

Minor concerns:

  • Commented Code: packages/next-js/src/mod.ts:10 has commented out code that should be removed
  • Magic Numbers: Hard-coded limits like 32 actor IDs in packages/rivetkit/src/manager/router.ts:213 could be constants

Potential Bugs & Issues ⚠️

  1. Inconsistent Inspector Configuration (packages/rivetkit/src/registry/mod.ts:72-74):

    if (driver.name === "engine") {
        config.inspector.enabled = { manager: false, actor: true };  // Object
    }
    // vs the old startServerless code had:
    config.inspector.enabled = false;  // Boolean

    This type inconsistency could cause runtime issues if the inspector config expects a boolean in some contexts.

  2. Header Name Inconsistency (packages/rivetkit/src/manager/router.ts:123):

    • Changed from x-rivetkit-total-slots to x-rivet-total-slots
    • Ensure this aligns with client expectations and doesn't break existing integrations
  3. Missing Validation (packages/rivetkit/src/manager/router.ts:124-125):

    if (!Number.isFinite(totalSlots)) totalSlots = undefined;

    While this handles NaN, it doesn't validate reasonable ranges (e.g., negative numbers, zero)

Performance Considerations 🚀

Good:

  • Eliminates code duplication (~110 lines removed)
  • Single initialization path reduces complexity
  • Lazy actor driver creation for serverless scenarios

Concerns:

  • None significant - the changes are primarily structural

Security Concerns 🔒

Low Risk:

  • Token handling appears consistent with existing patterns
  • No new attack vectors introduced
  • Proper validation exists for endpoint requirements with serverless mode

Considerations:

  • Header-based configuration in serverless routes should be validated against trusted sources
  • Consider rate limiting for the /start endpoint if not already handled upstream

Test Coverage 📋

Areas needing test coverage:

  1. New runnerKind validation in chooseDefaultDriver()
  2. Serverless route parameter handling (token, totalSlots, runnerName, namespace)
  3. Migration path from startServerless() to start() with runnerKind: "serverless"
  4. Inspector configuration with different driver types and serverless mode
  5. Error handling for malformed headers in serverless endpoints

Recommendations

  1. Remove commented code in packages/next-js/src/mod.ts:10
  2. Add validation for totalSlots range (positive integers only)
  3. Verify inspector config types are consistent across all driver configurations
  4. Add tests for the new runnerKind functionality and serverless parameter handling
  5. Document the migration from startServerless() to start() with appropriate configuration
  6. Consider constants for magic numbers like the 32 actor ID limit

Overall Assessment

This is a well-executed refactoring that simplifies the API while maintaining functionality. The consolidation approach is sound, but there are some type consistency issues and missing test coverage that should be addressed before merging.

Recommendation: Approve with minor changes - Fix the inspector config type inconsistency and add validation for the totalSlots parameter.


@linear
Copy link

linear bot commented Oct 2, 2025

@abcxff abcxff requested a review from NathanFlurry October 3, 2025 17:40
@abcxff abcxff force-pushed the 10-02-feat_serverless_make_runner_0_config branch from 6f15f4a to 257915e Compare October 3, 2025 18:39
@claude
Copy link

claude bot commented Oct 3, 2025

Claude finished @abcxff's task —— View job


PR Review: feat(serverless): make runner 0 config

I'm reviewing this PR that consolidates serverless configuration into a unified runner 0 config approach.

Todo List:

  • Read and analyze all 7 changed files
  • Understand the "runner 0 config" consolidation approach
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Assess performance considerations
  • Evaluate security concerns
  • Check test coverage implications
  • Address reviewer comments from NathanFlurry
  • Provide comprehensive feedback

Summary

This PR successfully consolidates serverless functionality by removing the separate startServerless() method and integrating serverless configuration through the main start() method using a runnerKind parameter. The changes simplify the API by eliminating ~110 lines of duplicate code while maintaining full functionality.

Code Quality & Best Practices ✅

Positive aspects:

  • API Simplification: Removing startServerless() reduces API surface area and eliminates code duplication between serverless and normal startup paths
  • Consistent Configuration: The runnerKind enum provides a clear, type-safe way to distinguish between normal and serverless operation modes
  • Type Safety: The ServerlessActorDriverBuilder type is properly defined and used consistently across the codebase
  • Error Handling: Good validation with clear error messages in packages/rivetkit/src/drivers/default.ts:17-21
  • Clean Integration: The serverless logic is well-integrated into the existing router structure

Areas for improvement:

  • Commented Code: packages/next-js/src/mod.ts:10 has commented out code (// inputConfig.runnerKind = "serverless";) that should be either removed or uncommented with proper justification
  • Magic Numbers: Hard-coded limit of 32 actor IDs in packages/rivetkit/src/manager/router.ts:213 could be extracted to a named constant

Addressing Reviewer Comments

NathanFlurry's Comments:

  1. Line 10 (packages/next-js/src/mod.ts:10): The commented code should indeed be addressed - either remove it or provide justification for keeping it commented
  2. Line 19 (packages/rivetkit/src/drivers/default.ts:19): The error message correctly explains this scenario shouldn't occur since the serverless endpoint should be configured by the engine when connecting to serverless mode

Potential Bugs & Issues ⚠️

  1. Header Name Change (packages/rivetkit/src/manager/router.ts:123):

    c.req.header("x-rivet-total-slots") // Changed from "x-rivetkit-total-slots"

    Ensure this header name change aligns with the engine/client expectations and doesn't break existing integrations.

  2. Numeric Validation (packages/rivetkit/src/manager/router.ts:124-125):

    if (!Number.isFinite(totalSlots)) totalSlots = undefined;

    While this handles NaN/Infinity, it doesn't validate against negative numbers or zero, which might be invalid configurations.

  3. Configuration Override Pattern (packages/rivetkit/src/registry/mod.ts:146-149):

    if (token) config.token = token;
    if (totalSlots) config.totalSlots = totalSlots;

    This mutates the shared config object which could cause issues if multiple serverless instances are created from the same config.

Performance Considerations 🚀

Improvements:

  • Code Reduction: Eliminates ~110 lines of duplicate code, reducing bundle size
  • Single Initialization Path: Consolidating into one startup method reduces complexity and potential bugs
  • Lazy Driver Creation: Serverless actor drivers are created on-demand only when needed

No significant performance concerns identified.

Security Concerns 🔒

Low Risk Profile:

  • Token handling remains consistent with existing patterns
  • No new attack vectors introduced
  • Proper validation exists for serverless endpoint requirements
  • Header-based configuration follows established patterns

Recommendations:

  • Consider validating totalSlots range (positive integers only)
  • Ensure header values are properly sanitized if they come from untrusted sources

Test Coverage 📋

Areas needing test coverage:

  1. New runnerKind validation in chooseDefaultDriver()
  2. Serverless route parameter handling (token, totalSlots, runnerName, namespace)
  3. Configuration override behavior in the serverless actor driver builder
  4. Error handling for malformed headers in serverless endpoints
  5. Migration scenarios from startServerless() to start() with appropriate configuration

Recommendations

High Priority:

  1. Address commented code in packages/next-js/src/mod.ts:10 - either remove or uncomment with justification
  2. Fix config mutation in packages/rivetkit/src/registry/mod.ts:146-149 to avoid side effects on shared config objects
  3. Add input validation for totalSlots to ensure positive integers only

Medium Priority:

  1. Extract magic number (32) to a named constant for actor ID limits
  2. Verify header naming consistency with engine expectations for x-rivet-total-slots
  3. Add comprehensive tests for the new runnerKind functionality

Low Priority:

  1. Document migration path from startServerless() to start() with examples

Overall Assessment

This is a well-executed refactoring that successfully simplifies the API while maintaining all existing functionality. The consolidation approach is architecturally sound and the implementation is clean. The main concerns are around addressing the commented code and preventing config mutation side effects.

Recommendation: Approve with minor changes - Address the commented code and fix the config mutation issue, then this is ready to merge.


@NathanFlurry NathanFlurry changed the base branch from main to graphite-base/1335 October 3, 2025 19:04
@NathanFlurry NathanFlurry force-pushed the 10-02-feat_serverless_make_runner_0_config branch from 257915e to f2bd09f Compare October 3, 2025 19:04
@NathanFlurry NathanFlurry changed the base branch from graphite-base/1335 to 10-03-fix_core_fix_sec-websocket-protocol_in_deno October 3, 2025 19:04
@claude
Copy link

claude bot commented Oct 3, 2025

Claude finished @abcxff's task —— View job


Claude is analyzing the PR

PR Review: feat(serverless): make runner 0 config

I'm reviewing this PR that consolidates serverless configuration into a unified runner 0 config approach.

Todo List:

  • Read and analyze all 7 changed files
  • Understand the "runner 0 config" consolidation approach
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Assess performance considerations
  • Evaluate security concerns
  • Check test coverage implications
  • Address reviewer comments from NathanFlurry
  • Provide comprehensive feedback

Summary

This PR successfully consolidates serverless functionality by removing the separate startServerless() method and integrating serverless configuration through the main start() method using a runnerKind parameter. The changes simplify the API by eliminating ~110 lines of duplicate code while maintaining full functionality.

Code Quality & Best Practices ✅

Positive aspects:

  • API Simplification: Removing startServerless() reduces API surface area and eliminates code duplication between serverless and normal startup paths
  • Consistent Configuration: The runnerKind enum provides a clear, type-safe way to distinguish between normal and serverless operation modes
  • Type Safety: The ServerlessActorDriverBuilder type is properly defined and used consistently across the codebase
  • Error Handling: Good validation with clear error messages in packages/rivetkit/src/drivers/default.ts:17-21
  • Clean Integration: The serverless logic is well-integrated into the existing router structure

Areas for improvement:

  • Commented Code: packages/next-js/src/mod.ts:10 has commented out code (// inputConfig.runnerKind = "serverless";) that should be either removed or uncommented with proper justification
  • Magic Numbers: Hard-coded limit of 32 actor IDs in packages/rivetkit/src/manager/router.ts:213 could be extracted to a named constant

Addressing Reviewer Comments

NathanFlurry's Comments:

  1. Line 10 (packages/next-js/src/mod.ts:10): The commented code should indeed be addressed - either remove it or provide justification for keeping it commented
  2. Line 19 (packages/rivetkit/src/drivers/default.ts:19): The error message correctly explains this scenario shouldn't occur since the serverless endpoint should be configured by the engine when connecting to serverless mode

Potential Bugs & Issues ⚠️

  1. Header Name Change (packages/rivetkit/src/manager/router.ts:123):

    c.req.header("x-rivet-total-slots") // Changed from "x-rivetkit-total-slots"

    Ensure this header name change aligns with the engine/client expectations and doesn't break existing integrations.

  2. Numeric Validation (packages/rivetkit/src/manager/router.ts:124-125):

    if (!Number.isFinite(totalSlots)) totalSlots = undefined;

    While this handles NaN/Infinity, it doesn't validate against negative numbers or zero, which might be invalid configurations.

  3. Configuration Override Pattern (packages/rivetkit/src/registry/mod.ts:146-149):

    if (token) config.token = token;
    if (totalSlots) config.totalSlots = totalSlots;

    This mutates the shared config object which could cause issues if multiple serverless instances are created from the same config.

Performance Considerations 🚀

Improvements:

  • Code Reduction: Eliminates ~110 lines of duplicate code, reducing bundle size
  • Single Initialization Path: Consolidating into one startup method reduces complexity and potential bugs
  • Lazy Driver Creation: Serverless actor drivers are created on-demand only when needed

No significant performance concerns identified.

Security Concerns 🔒

Low Risk Profile:

  • Token handling remains consistent with existing patterns
  • No new attack vectors introduced
  • Proper validation exists for serverless endpoint requirements
  • Header-based configuration follows established patterns

Recommendations:

  • Consider validating totalSlots range (positive integers only)
  • Ensure header values are properly sanitized if they come from untrusted sources

Test Coverage 📋

Areas needing test coverage:

  1. New runnerKind validation in chooseDefaultDriver()
  2. Serverless route parameter handling (token, totalSlots, runnerName, namespace)
  3. Configuration override behavior in the serverless actor driver builder
  4. Error handling for malformed headers in serverless endpoints
  5. Migration scenarios from startServerless() to start() with appropriate configuration

Recommendations

High Priority:

  1. Address commented code in packages/next-js/src/mod.ts:10 - either remove or uncomment with justification
  2. Fix config mutation in packages/rivetkit/src/registry/mod.ts:146-149 to avoid side effects on shared config objects
  3. Add input validation for totalSlots to ensure positive integers only

Medium Priority:

  1. Extract magic number (32) to a named constant for actor ID limits
  2. Verify header naming consistency with engine expectations for x-rivet-total-slots
  3. Add comprehensive tests for the new runnerKind functionality

Low Priority:

  1. Document migration path from startServerless() to start() with examples

Overall Assessment

This is a well-executed refactoring that successfully simplifies the API while maintaining all existing functionality. The consolidation approach is architecturally sound and the implementation is clean. The main concerns are around addressing the commented code and preventing config mutation side effects.

Recommendation: Approve with minor changes - Address the commented code and fix the config mutation issue, then this is ready to merge.


@claude
Copy link

claude bot commented Oct 3, 2025

Claude finished @abcxff's task —— View job


PR Review: feat(serverless): make runner 0 config

I'm reviewing this PR that consolidates serverless configuration into a unified runner 0 config approach.

Todo List:

  • Read and analyze all 7 changed files
  • Understand the "runner 0 config" consolidation approach
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Assess performance considerations
  • Evaluate security concerns
  • Check test coverage implications
  • Address reviewer comments from NathanFlurry
  • Provide comprehensive feedback

Summary

This PR successfully consolidates serverless functionality by removing the separate startServerless() method and integrating serverless configuration through the main start() method using a runnerKind parameter. The changes simplify the API by eliminating ~110 lines of duplicate code while maintaining full functionality.

Code Quality & Best Practices ✅

Positive aspects:

  • API Simplification: Removing startServerless() reduces API surface area and eliminates code duplication between serverless and normal startup paths
  • Consistent Configuration: The runnerKind enum provides a clear, type-safe way to distinguish between normal and serverless operation modes
  • Type Safety: The ServerlessActorDriverBuilder type is properly defined and used consistently across the codebase
  • Error Handling: Good validation with clear error messages in packages/rivetkit/src/drivers/default.ts:17-21
  • Clean Integration: The serverless logic is well-integrated into the existing router structure

Areas for improvement:

  • Commented Code: packages/next-js/src/mod.ts:10 has commented out code (// inputConfig.runnerKind = "serverless";) that should be either removed or uncommented with proper justification
  • Magic Numbers: Hard-coded limit of 32 actor IDs in packages/rivetkit/src/manager/router.ts:213 could be extracted to a named constant

Addressing Reviewer Comments

NathanFlurry's Comments:

  1. Line 10 (packages/next-js/src/mod.ts:10): The commented code should indeed be addressed - either remove it or provide justification for keeping it commented
  2. Line 19 (packages/rivetkit/src/drivers/default.ts:19): The error message correctly explains this scenario shouldn't occur since the serverless endpoint should be configured by the engine when connecting to serverless mode

Potential Bugs & Issues ⚠️

  1. Header Name Change (packages/rivetkit/src/manager/router.ts:123):

    c.req.header("x-rivet-total-slots") // Changed from "x-rivetkit-total-slots"

    Ensure this header name change aligns with the engine/client expectations and doesn't break existing integrations.

  2. Numeric Validation (packages/rivetkit/src/manager/router.ts:124-125):

    if (!Number.isFinite(totalSlots)) totalSlots = undefined;

    While this handles NaN/Infinity, it doesn't validate against negative numbers or zero, which might be invalid configurations.

  3. Configuration Override Pattern (packages/rivetkit/src/registry/mod.ts:146-149):

    if (token) config.token = token;
    if (totalSlots) config.totalSlots = totalSlots;

    This mutates the shared config object which could cause issues if multiple serverless instances are created from the same config.

Performance Considerations 🚀

Improvements:

  • Code Reduction: Eliminates ~110 lines of duplicate code, reducing bundle size
  • Single Initialization Path: Consolidating into one startup method reduces complexity and potential bugs
  • Lazy Driver Creation: Serverless actor drivers are created on-demand only when needed

No significant performance concerns identified.

Security Concerns 🔒

Low Risk Profile:

  • Token handling remains consistent with existing patterns
  • No new attack vectors introduced
  • Proper validation exists for serverless endpoint requirements
  • Header-based configuration follows established patterns

Recommendations:

  • Consider validating totalSlots range (positive integers only)
  • Ensure header values are properly sanitized if they come from untrusted sources

Test Coverage 📋

Areas needing test coverage:

  1. New runnerKind validation in chooseDefaultDriver()
  2. Serverless route parameter handling (token, totalSlots, runnerName, namespace)
  3. Configuration override behavior in the serverless actor driver builder
  4. Error handling for malformed headers in serverless endpoints
  5. Migration scenarios from startServerless() to start() with appropriate configuration

Recommendations

High Priority:

  1. Address commented code in packages/next-js/src/mod.ts:10 - either remove or uncomment with justification
  2. Fix config mutation in packages/rivetkit/src/registry/mod.ts:146-149 to avoid side effects on shared config objects
  3. Add input validation for totalSlots to ensure positive integers only

Medium Priority:

  1. Extract magic number (32) to a named constant for actor ID limits
  2. Verify header naming consistency with engine expectations for x-rivet-total-slots
  3. Add comprehensive tests for the new runnerKind functionality

Low Priority:

  1. Document migration path from startServerless() to start() with examples

Overall Assessment

This is a well-executed refactoring that successfully simplifies the API while maintaining all existing functionality. The consolidation approach is architecturally sound and the implementation is clean. The main concerns are around addressing the commented code and preventing config mutation side effects.

Recommendation: Approve with minor changes - Address the commented code and fix the config mutation issue, then this is ready to merge.


@NathanFlurry NathanFlurry force-pushed the 10-02-feat_serverless_make_runner_0_config branch from f2bd09f to effd562 Compare October 5, 2025 00:43
@NathanFlurry NathanFlurry force-pushed the 10-03-fix_core_fix_sec-websocket-protocol_in_deno branch from 9cd6530 to 89f1f51 Compare October 5, 2025 00:43
@claude
Copy link

claude bot commented Oct 5, 2025

Claude finished @abcxff's task —— View job


Claude is analyzing the PR

PR Review: feat(serverless): make runner 0 config

I'm reviewing this PR that consolidates serverless configuration into a unified runner 0 config approach.

Todo List:

  • Read CLAUDE.md for repo-specific guidelines
  • Read and analyze all 7 changed files
  • Understand the "runner 0 config" consolidation approach
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Assess performance considerations
  • Evaluate security concerns
  • Check test coverage implications
  • Address previous reviewer feedback
  • Provide comprehensive feedback

Summary

This PR successfully consolidates serverless functionality by removing the separate startServerless() method and integrating serverless configuration through the main start() method using a runnerKind parameter. The changes simplify the API by eliminating ~110 lines of duplicate code while maintaining full functionality.

Code Quality & Best Practices ✅

Positive aspects:

  • API Simplification: Removing startServerless() reduces API surface area and eliminates code duplication between serverless and normal startup paths
  • Consistent Configuration: The runnerKind enum provides a clear, type-safe way to distinguish between normal and serverless operation modes
  • Type Safety: The ServerlessActorDriverBuilder type is properly defined and used consistently across the codebase
  • Error Handling: Good validation with clear error messages in packages/rivetkit/src/drivers/default.ts:17-21
  • Clean Integration: The serverless logic is well-integrated into the existing router structure

Areas for improvement:

  • Magic Numbers: Hard-coded limit of 32 actor IDs in packages/rivetkit/src/manager/router.ts:232 could be extracted to a named constant

Addressing Previous Reviewer Comments

NathanFlurry's feedback has been addressed:

  1. Line 10 concern: The commented code in packages/next-js/src/mod.ts appears to have been properly removed - the file now shows clean implementation without commented serverless configuration
  2. Error message clarification: The error message in packages/rivetkit/src/drivers/default.ts:19 correctly explains that this scenario shouldn't occur since the serverless endpoint should be configured by the engine when connecting to serverless mode

Potential Bugs & Issues ⚠️

  1. Header Name Change (packages/rivetkit/src/manager/router.ts:142):

    c.req.header("x-rivet-total-slots") // Changed from "x-rivetkit-total-slots"

    Ensure this header name change aligns with the engine/client expectations and doesn't break existing integrations.

  2. Numeric Validation (packages/rivetkit/src/manager/router.ts:143-144):

    if (!Number.isFinite(totalSlots)) totalSlots = undefined;

    While this handles NaN/Infinity, it doesn't validate against negative numbers or zero, which might be invalid configurations.

  3. Configuration Override Pattern (packages/rivetkit/src/registry/mod.ts:150-153):

    if (token) config.token = token;
    if (totalSlots) config.totalSlots = totalSlots;

    This mutates the shared config object which could cause issues if multiple serverless instances are created from the same config.

Performance Considerations 🚀

Improvements:

  • Code Reduction: Eliminates ~110 lines of duplicate code, reducing bundle size
  • Single Initialization Path: Consolidating into one startup method reduces complexity and potential bugs
  • Lazy Driver Creation: Serverless actor drivers are created on-demand only when needed

No significant performance concerns identified.

Security Concerns 🔒

Low Risk Profile:

  • Token handling remains consistent with existing patterns
  • No new attack vectors introduced
  • Proper validation exists for serverless endpoint requirements
  • Header-based configuration follows established patterns

Recommendations:

  • Consider validating totalSlots range (positive integers only)
  • Ensure header values are properly sanitized if they come from untrusted sources

Test Coverage 📋

Areas needing test coverage:

  1. New runnerKind validation in chooseDefaultDriver()
  2. Serverless route parameter handling (token, totalSlots, runnerName, namespace)
  3. Configuration override behavior in the serverless actor driver builder
  4. Error handling for malformed headers in serverless endpoints
  5. Migration scenarios from startServerless() to start() with appropriate configuration

Recommendations

High Priority:

  1. Fix config mutation in packages/rivetkit/src/registry/mod.ts:150-153 to avoid side effects on shared config objects
  2. Add input validation for totalSlots to ensure positive integers only

Medium Priority:

  1. Extract magic number (32) to a named constant for actor ID limits
  2. Verify header naming consistency with engine expectations for x-rivet-total-slots
  3. Add comprehensive tests for the new runnerKind functionality

Low Priority:

  1. Document migration path from startServerless() to start() with examples

Overall Assessment

This is a well-executed refactoring that successfully simplifies the API while maintaining all existing functionality. The consolidation approach is architecturally sound and the implementation is clean. The previous reviewer concerns have been properly addressed.

Recommendation: Approve with minor changes - Fix the config mutation issue and add input validation for totalSlots, then this is ready to merge.


@claude
Copy link

claude bot commented Oct 5, 2025

Claude finished @abcxff's task —— View job


PR Review: feat(serverless): make runner 0 config

I'm reviewing this PR that consolidates serverless configuration into a unified runner 0 config approach.

Todo List:

  • Read and analyze all 7 changed files
  • Understand the "runner 0 config" consolidation approach
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Assess performance considerations
  • Evaluate security concerns
  • Check test coverage implications
  • Address reviewer comments from NathanFlurry
  • Provide comprehensive feedback

Summary

This PR successfully consolidates serverless functionality by removing the separate startServerless() method and integrating serverless configuration through the main start() method using a runnerKind parameter. The changes simplify the API by eliminating ~110 lines of duplicate code while maintaining full functionality.

Code Quality & Best Practices ✅

Positive aspects:

  • API Simplification: Removing startServerless() reduces API surface area and eliminates code duplication between serverless and normal startup paths
  • Consistent Configuration: The runnerKind enum provides a clear, type-safe way to distinguish between normal and serverless operation modes
  • Type Safety: The ServerlessActorDriverBuilder type is properly defined and used consistently across the codebase
  • Error Handling: Good validation with clear error messages in packages/rivetkit/src/drivers/default.ts:17-21
  • Clean Integration: The serverless logic is well-integrated into the existing router structure

Areas for improvement:

  • Commented Code: packages/next-js/src/mod.ts:10 has commented out code (// inputConfig.runnerKind = "serverless";) that should be either removed or uncommented with proper justification
  • Magic Numbers: Hard-coded limit of 32 actor IDs in packages/rivetkit/src/manager/router.ts:213 could be extracted to a named constant

Addressing Reviewer Comments

NathanFlurry's Comments:

  1. Line 10 (packages/next-js/src/mod.ts:10): The commented code should indeed be addressed - either remove it or provide justification for keeping it commented
  2. Line 19 (packages/rivetkit/src/drivers/default.ts:19): The error message correctly explains this scenario shouldn't occur since the serverless endpoint should be configured by the engine when connecting to serverless mode

Potential Bugs & Issues ⚠️

  1. Header Name Change (packages/rivetkit/src/manager/router.ts:123):

    c.req.header("x-rivet-total-slots") // Changed from "x-rivetkit-total-slots"

    Ensure this header name change aligns with the engine/client expectations and doesn't break existing integrations.

  2. Numeric Validation (packages/rivetkit/src/manager/router.ts:124-125):

    if (!Number.isFinite(totalSlots)) totalSlots = undefined;

    While this handles NaN/Infinity, it doesn't validate against negative numbers or zero, which might be invalid configurations.

  3. Configuration Override Pattern (packages/rivetkit/src/registry/mod.ts:146-149):

    if (token) config.token = token;
    if (totalSlots) config.totalSlots = totalSlots;

    This mutates the shared config object which could cause issues if multiple serverless instances are created from the same config.

Performance Considerations 🚀

Improvements:

  • Code Reduction: Eliminates ~110 lines of duplicate code, reducing bundle size
  • Single Initialization Path: Consolidating into one startup method reduces complexity and potential bugs
  • Lazy Driver Creation: Serverless actor drivers are created on-demand only when needed

No significant performance concerns identified.

Security Concerns 🔒

Low Risk Profile:

  • Token handling remains consistent with existing patterns
  • No new attack vectors introduced
  • Proper validation exists for serverless endpoint requirements
  • Header-based configuration follows established patterns

Recommendations:

  • Consider validating totalSlots range (positive integers only)
  • Ensure header values are properly sanitized if they come from untrusted sources

Test Coverage 📋

Areas needing test coverage:

  1. New runnerKind validation in chooseDefaultDriver()
  2. Serverless route parameter handling (token, totalSlots, runnerName, namespace)
  3. Configuration override behavior in the serverless actor driver builder
  4. Error handling for malformed headers in serverless endpoints
  5. Migration scenarios from startServerless() to start() with appropriate configuration

Recommendations

High Priority:

  1. Address commented code in packages/next-js/src/mod.ts:10 - either remove or uncomment with justification
  2. Fix config mutation in packages/rivetkit/src/registry/mod.ts:146-149 to avoid side effects on shared config objects
  3. Add input validation for totalSlots to ensure positive integers only

Medium Priority:

  1. Extract magic number (32) to a named constant for actor ID limits
  2. Verify header naming consistency with engine expectations for x-rivet-total-slots
  3. Add comprehensive tests for the new runnerKind functionality

Low Priority:

  1. Document migration path from startServerless() to start() with examples

Overall Assessment

This is a well-executed refactoring that successfully simplifies the API while maintaining all existing functionality. The consolidation approach is architecturally sound and the implementation is clean. The main concerns are around addressing the commented code and preventing config mutation side effects.

Recommendation: Approve with minor changes - Address the commented code and fix the config mutation issue, then this is ready to merge.


@graphite-app
Copy link

graphite-app bot commented Oct 5, 2025

Merge activity

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

graphite-app bot pushed a commit that referenced this pull request Oct 5, 2025
@graphite-app graphite-app bot closed this Oct 5, 2025
@graphite-app graphite-app bot deleted the 10-02-feat_serverless_make_runner_0_config branch October 5, 2025 00:46
graphite-app bot pushed a commit to rivet-dev/rivet that referenced this pull request Oct 7, 2025
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.

2 participants