Skip to content

chore: fix serverless in rivetkit-core#4719

Closed
NathanFlurry wants to merge 1 commit into04-23-chore_rivetkit_impl_follow_up_reviewfrom
04-23-chore_fix_serverless
Closed

chore: fix serverless in rivetkit-core#4719
NathanFlurry wants to merge 1 commit into04-23-chore_rivetkit_impl_follow_up_reviewfrom
04-23-chore_fix_serverless

Conversation

@NathanFlurry
Copy link
Copy Markdown
Member

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@railway-app
Copy link
Copy Markdown

railway-app Bot commented Apr 24, 2026

🚅 Deployed to the rivet-pr-4719 environment in rivet-frontend

Service Status Web Updated (UTC)
kitchen-sink ❌ Build Failed (View Logs) Web Apr 24, 2026 at 12:32 pm
website ✅ Success (View Logs) Web Apr 24, 2026 at 12:29 pm
frontend-cloud ❌ Build Failed (View Logs) Web Apr 24, 2026 at 1:42 am
ladle ✅ Success (View Logs) Web Apr 24, 2026 at 1:42 am
frontend-inspector ❌ Build Failed (View Logs) Web Apr 24, 2026 at 1:42 am
mcp-hub ✅ Success (View Logs) Web Apr 24, 2026 at 1:41 am

Copy link
Copy Markdown
Member Author

NathanFlurry commented Apr 24, 2026

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

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link
Copy Markdown

claude Bot commented Apr 24, 2026

PR Review: chore: fix serverless in rivetkit-core

Overview

This PR refactors the serverless metadata types by extracting shared Serde structs into a new rivetkit-shared-types crate, eliminating duplication between pegboard (engine) and rivetkit-core. It also improves error diagnostics, simplifies the NAPI build script, and broadens Turbo cache invalidation inputs.


Issues

Silent failure in serialization (rivetkit-rust/packages/rivetkit-core/src/serverless.rs)

The unwrap_or_else(|_| json!({})) on serialization silently returns {} on failure, which violates the fail-by-default principle from CLAUDE.md. This should either propagate as an error or log with tracing::error! before the fallback:

let mut response = match serde_json::to_value(payload) {
    Ok(v) => v,
    Err(err) => {
        tracing::error!(?err, "failed to serialize serverless metadata");
        return Err(anyhow::anyhow!("failed to serialize serverless metadata: {err}"));
    }
};

Manual null removal for runner (rivetkit-rust/packages/rivetkit-core/src/serverless.rs)

The post-serialization null check and removal for runner works around the default Option::None to null behavior. The idiomatic fix is #[serde(skip_serializing_if = "Option::is_none")] on the field in the shared types struct - safe since Option deserializes a missing field as None. This eliminates the need to patch the serialized map at all.


Typed struct approach is incomplete (rivetkit-rust/packages/rivetkit-core/src/serverless.rs)

The envoy.kind and clientEndpoint fields are still manually patched after serialization. Since the PR is moving to typed structs, consider adding a kind enum to ServerlessMetadataEnvoy and client_endpoint: Option<String> to ServerlessMetadataPayload. If this is deferred intentionally, a comment noting why would prevent future confusion.


build:force script silently ignores its flag (rivetkit-typescript/packages/rivetkit-napi/)

package.json still has "build:force": "node scripts/build.mjs --force", but the new build.mjs no longer parses --force. Since the prebuilt detection was removed entirely, build and build:force are now equivalent. CLAUDE.md still documents using build:force for native changes. Either remove build:force or keep it as an explicit alias and update the CLAUDE.md note.


Positive Changes

  • Better error diagnostics: Adding parse_error: String to InvalidResponseJson and introducing a dedicated InvalidEnvoyProtocolVersion variant instead of reusing InvalidResponseJson for a version mismatch is a clear improvement.
  • Deduplication via rivetkit-shared-types: Extracting the shared types into a minimal crate with only serde deps is the right approach. The crate follows workspace conventions correctly.
  • Turbo cache inputs: Expanding inputs in rivetkit-napi/turbo.json to cover all dependent Rust crates prevents stale cache hits. The broader glob is appropriate given the new shared-types dependency.
  • Build script simplification: Removing the prebuilt detection logic and relying on Turbos own caching is cleaner than the previous filesystem scanning approach.
  • execFileSync over execSync: Safer approach that avoids shell interpretation of the command.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 24, 2026

Preview packages published to npm

Install with:

npm install rivetkit@pr-4719

All packages published as 0.0.0-pr.4719.23ea795 with tag pr-4719.

Engine binary is shipped via @rivetkit/engine-cli on linux-x64-musl, linux-arm64-musl, darwin-x64, and darwin-arm64. Windows users should use the release installer or set RIVET_ENGINE_BINARY.

Docker images:

docker pull rivetdev/engine:slim-23ea795
docker pull rivetdev/engine:full-23ea795
Individual packages
npm install rivetkit@pr-4719
npm install @rivetkit/react@pr-4719
npm install @rivetkit/rivetkit-napi@pr-4719
npm install @rivetkit/workflow-engine@pr-4719

@NathanFlurry NathanFlurry force-pushed the 04-23-chore_rivetkit_impl_follow_up_review branch from 853e804 to e5493f1 Compare April 24, 2026 09:52
@NathanFlurry NathanFlurry force-pushed the 04-23-chore_fix_serverless branch from b22666f to 7f1df3a Compare April 24, 2026 09:52
@railway-app railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-4719 April 24, 2026 09:52 Destroyed
@NathanFlurry NathanFlurry mentioned this pull request Apr 24, 2026
11 tasks
@NathanFlurry NathanFlurry force-pushed the 04-23-chore_fix_serverless branch from 7f1df3a to f431f98 Compare April 24, 2026 10:19
@NathanFlurry NathanFlurry force-pushed the 04-23-chore_rivetkit_impl_follow_up_review branch from e5493f1 to be662e1 Compare April 24, 2026 10:19
@railway-app railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-4719 April 24, 2026 10:19 Destroyed
@NathanFlurry NathanFlurry force-pushed the 04-23-chore_fix_serverless branch from f431f98 to a4cf129 Compare April 24, 2026 10:32
@railway-app railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-4719 April 24, 2026 10:32 Destroyed
@NathanFlurry NathanFlurry force-pushed the 04-23-chore_rivetkit_impl_follow_up_review branch from 9ba105f to be3c2df Compare April 24, 2026 11:48
@NathanFlurry NathanFlurry force-pushed the 04-23-chore_fix_serverless branch from a4cf129 to 94df783 Compare April 24, 2026 11:48
@railway-app railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-4719 April 24, 2026 11:49 Destroyed
@NathanFlurry NathanFlurry force-pushed the 04-23-chore_fix_serverless branch from 94df783 to a4dd922 Compare April 24, 2026 12:14
@NathanFlurry NathanFlurry force-pushed the 04-23-chore_rivetkit_impl_follow_up_review branch from be3c2df to a19835c Compare April 24, 2026 12:14
@railway-app railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-4719 April 24, 2026 12:14 Destroyed
@NathanFlurry NathanFlurry force-pushed the 04-23-chore_rivetkit_impl_follow_up_review branch from a19835c to c947310 Compare April 24, 2026 12:32
@NathanFlurry NathanFlurry force-pushed the 04-23-chore_fix_serverless branch from a4dd922 to 24a5eb0 Compare April 24, 2026 12:32
@railway-app railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-4719 April 24, 2026 12:32 Destroyed
@NathanFlurry
Copy link
Copy Markdown
Member Author

Landed in main via stack-merge fast-forward push. Commits are in main; closing to match.

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.

1 participant