Skip to content

fix(rivetkit): minor rivetkit debug changes#4676

Open
jog1t wants to merge 1 commit into04-16-feat_frontend_add_enoys_to_the_runner_tablefrom
04-16-fix_rivetkit_minor_rivetkit_debug_changes
Open

fix(rivetkit): minor rivetkit debug changes#4676
jog1t wants to merge 1 commit into04-16-feat_frontend_add_enoys_to_the_runner_tablefrom
04-16-fix_rivetkit_minor_rivetkit_debug_changes

Conversation

@jog1t
Copy link
Copy Markdown
Contributor

@jog1t jog1t commented Apr 16, 2026

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
Contributor Author

jog1t commented Apr 16, 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.

@railway-app
Copy link
Copy Markdown

railway-app bot commented Apr 16, 2026

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

Service Status Web Updated (UTC)
frontend-cloud 😴 Sleeping (View Logs) Web Apr 16, 2026 at 9:59 pm
website 😴 Sleeping (View Logs) Web Apr 16, 2026 at 9:58 pm
frontend-inspector 😴 Sleeping (View Logs) Web Apr 16, 2026 at 9:55 pm
mcp-hub ✅ Success (View Logs) Web Apr 16, 2026 at 9:47 pm
kitchen-sink ❌ Build Failed (View Logs) Web Apr 16, 2026 at 9:47 pm
ladle ❌ Build Failed (View Logs) Web Apr 16, 2026 at 9:47 pm

@claude
Copy link
Copy Markdown

claude bot commented Apr 16, 2026

Code Review

This PR makes several useful debug/reliability improvements to the RivetKit TypeScript runtime. Overall the changes are well-scoped. Below are observations by file.


src/engine-process/mod.ts

Stale-engine detection on manager port (new logic, lines 66-81)

The new stale-engine check is a good guard against the upgrade scenario described in the comment. A few notes:

  1. Hardcoded port number in error messagescheckIfEngineAlreadyRunningOnPort (unchanged code) still has hardcoded strings like "stop the process on port 6420" even when the method is now called with a dynamic managerPort. The new error message correctly uses ${managerPort}, but the inner checkIfEngineAlreadyRunningOnPort function's own error path still has "port 6420" hardcoded when it detects a "rivetkit" runtime or an unknown process. Since this function is now used for both ENGINE_PORT and managerPort, those messages will be misleading if triggered on the manager port check. Consider passing the port through to the error strings inside checkIfEngineAlreadyRunningOnPort.

  2. Double blank line — There is an extra blank line at line 83 (after the closing } of the new if block and before the // Resolve the engine binary comment). Minor style nit.

  3. Return type widening — Changing the return type from Promise<void> to Promise<ChildProcess | undefined> is correct and well-handled. The return undefined on the early-return path is explicit and clear.


runtime/index.ts

Signal handler registration

The new else if branch (lines 200-206) correctly handles the case where an engine child process was spawned but serveManager is false. Good.

One concern: process.on("SIGTERM", ...) and process.on("SIGINT", ...) can accumulate listeners across multiple calls to Runtime.create() if a test suite or hot-reload scenario calls it more than once. Node.js will emit a MaxListenersExceededWarning after the default 10 listeners. The original code had the same shape, so this is a pre-existing issue, but the new else if branch adds a second possible registration path for the same engineChildProcess. Consider using process.once(...) instead of process.on(...), or removing the listener after the first invocation.

engineChildProcess capture in closure

In the existing out.closeServer branch (line 193-199), engineChildProcess is captured via optional chaining (engineChildProcess?.kill). In the new else if branch (line 202), the lambda captures engineChildProcess by reference. At the time the lambda fires, engineChildProcess is const-equivalent in the outer scope (declared as let but never reassigned after the if (shouldSpawnEngine) block), so the value will be stable. This is fine but worth noting for future maintainers.


src/registry/index.ts

start() — replacing biome-ignore with proper .catch()

Replacing the biome-ignore lint/nursery/noFloatingPromises suppression with an explicit .catch() that logs and sets process.exitCode = 1 is a genuine improvement. Using process.exitCode = 1 instead of process.exit(1) to allow logger flush is a nice touch.

startEnvoy() — still uses biome-ignore

The public startEnvoy() method (line 99-102) still uses the biome-ignore suppression for the fire-and-forget call. This pattern is lower-risk since it is invoked only from user-controlled entry points, but if it errors it will be completely silent. Consider applying the same .catch() treatment for consistency:

public startEnvoy() {
    this.#ensureRuntime()
        .then((runtime) => runtime.startEnvoy())
        .catch((err) => {
            logger().error({ msg: "failed to start envoy", err });
        });
}

src/engine-client/ws-proxy.ts

No-op .catch() on connectPromise

The comment is accurate and the fix is correct — attaching a no-op .catch(() => {}) prevents Node.js unhandled rejection warnings when the client disconnects before sending any message. The rejection still propagates to any caller that awaits connectPromise via onMessage. Well done.

The onMessage handler already catches and logs the error (lines 131-140), so the overall error handling chain is complete.


runtime/index.tsstartEnvoy logging

Adding logger().info({ msg: "connecting to engine" }) and logger().info({ msg: "connected to engine" }) with a .catch() that re-throws is good for observability. The log + rethrow pattern is correct.


src/runtime-router/router.ts

The changes here are purely cosmetic: import reordering and reformatting a multi-argument function call. No behavioral change. The blank line removal before } is a minor cleanup.


Summary

Area Severity Note
checkIfEngineAlreadyRunningOnPort hardcoded port 6420 in error strings Minor Misleading when called with managerPort
Double blank line in mod.ts line 83 Trivial
process.on vs process.once for signal handlers Minor Pre-existing, new branch adds second registration path
startEnvoy() still uses biome-ignore suppression Minor Inconsistent with start() improvement in same PR

The core fixes (child process return, stale port detection, unhandled rejection suppression, error logging) are all sound. Resolving the hardcoded port issue in checkIfEngineAlreadyRunningOnPort would be the most impactful follow-up.

@github-actions
Copy link
Copy Markdown
Contributor

Preview packages published to npm

Install with:

npm install rivetkit@pr-4676

All packages published as 0.0.0-pr.4676.118d364 with tag pr-4676.

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-118d364
docker pull rivetdev/engine:full-118d364
Individual packages
npm install rivetkit@pr-4676
npm install @rivetkit/react@pr-4676
npm install @rivetkit/rivetkit-native@pr-4676
npm install @rivetkit/sqlite-wasm@pr-4676
npm install @rivetkit/workflow-engine@pr-4676

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