Skip to content

chore: engine npm package#4581

Draft
NathanFlurry wants to merge 1 commit into04-06-chore_remove_non-engine_driversfrom
04-07-chore_engine_npm_package
Draft

chore: engine npm package#4581
NathanFlurry wants to merge 1 commit into04-06-chore_remove_non-engine_driversfrom
04-07-chore_engine_npm_package

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

Copy link
Copy Markdown
Member Author

NathanFlurry commented Apr 7, 2026

@claude
Copy link
Copy Markdown

claude bot commented Apr 7, 2026

PR #4581 Review: chore: engine npm package

Summary

This PR refactors how the Rivet engine binary is distributed and resolved. Instead of downloading the engine binary at runtime from a remote URL, the binary is now shipped as platform-specific npm packages (@rivetkit/engine-{platform}-{arch}) and resolved at startup via a new @rivetkit/engine meta-package. The PR also renames several config fields and splits engine-spawning config out of the serverless sub-object into top-level fields.

What's Good

  • Removing the binary download-at-runtime path simplifies a large surface area of code (removes ~150 lines including temp file management, checksums, slow-download warnings, and platform triplet resolution).
  • The dynamic import pattern (["@rivetkit", "engine"].join("/")) correctly follows the codebase convention for preventing static bundler analysis.
  • The update_version.ts script is updated to keep the new npm/*/package.json versions in sync during releases.
  • The release script handles idempotency correctly: it checks if a version already exists before publishing and skips gracefully.
  • Tests are updated to match the new API surface.

Issues

1. Potential Race Condition in ensureHttpServer

The ensureHttpServer method uses this.httpPort as a guard to prevent double initialization. If two callers await ensureHttpServer() concurrently, both will pass the if (this.httpPort) check before either sets the field, resulting in two servers binding to the same port. The old code was gated behind serveManager which was controlled up-front. Now that the HTTP server is lazily started on-demand, this is a real risk in tests. Consider using a promise-based mutex or an #httpServerPromise field to deduplicate concurrent calls (per CLAUDE.md, antiox provides concurrency primitives for this).

2. resolveEngineBinary Reports Version from the Meta-Package, Not the Platform Package

getInstalledVersion() reads the version from the meta-package's package.json, not from the platform-specific package that was resolved. After npm publish, these are independently versioned packages. If they diverge (e.g., a partially failed publish), the mismatch check would compare against the wrong version. Consider reading the version from the resolved platform package's package.json instead.

3. Breaking Config Renames Without Migration Path

Several fields are renamed without any deprecation shim:

  • managerPort -> httpPort
  • managerBasePath -> httpBasePath
  • publicDir -> staticDir
  • serveManager removed
  • serverless.spawnEngine -> startEngine (top-level)
  • serverless.engineVersion -> engineVersion (top-level)
  • serverless.configureRunnerPool / serverless.configurePool -> configurePool (top-level)

Any user currently setting these fields will silently get the wrong behavior. At minimum, the PR description should flag these as breaking changes.

4. Default Port Changed from 6420 to 6421

Any existing .env, docker-compose configs, or firewall rules pointing at port 6420 would silently break. Should be explicitly called out as a breaking change.

5. Hardcoded Magic Number for Default Port

In src/registry/index.ts, the port default 6421 is duplicated inline (this.#config.httpPort ?? 6421) in addition to the Zod schema default and the serve.ts constant. Use the schema default or import from serve.ts to avoid drift.

6. #printWelcome Called Before ensureHttpServer Completes

In the startServerless path, #printWelcome is called while this.httpPort may still be undefined, causing the inspector URL in the welcome message to display incorrectly. Either call ensureHttpServer before the welcome print or document the intent.

7. Missing Test Coverage for darwin-x64 and win32-x64 Platforms

The new tests only cover linux/x64 and darwin/arm64, dropping coverage for darwin/x64 and win32-x64. Given the mapping is hand-coded in PLATFORM_PACKAGES, a typo would go undetected. Consider restoring tests for all four supported platforms.


Minor Notes

  • Fire-and-forget errors in start(): The biome-ignore lint/nursery/noFloatingPromises comment silently swallows errors from ensureHttpServer() and startEnvoy(). Consider attaching a .catch that logs the error so failures are visible.
  • Sequential platform package publishing: Platform packages are published in a for...of loop with await. Could be parallelized in CI for speed.
  • Fragile npm error string matching: npmVersionExists checks for specific npm CLI error strings. These could break silently with npm CLI version changes.

Checklist Gaps

  • PR description is empty — type of change, test plan, and checklist are unchecked.
  • The breaking config renames and default port change are not documented.
  • No integration test verifying that the @rivetkit/engine package resolution path works end-to-end (existing tests cover getEnginePackageNameForPlatform in isolation, not the full ensureEngineProcess path with the new package-based resolution).

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