Skip to content

fix(envoy): use global instance, add signal handlers#4565

Open
MasterPtato wants to merge 1 commit into04-05-chore_misc_fixes_add_pb_snapshot_testfrom
04-06-fix_envoy_use_global_instance_add_signal_handlers
Open

fix(envoy): use global instance, add signal handlers#4565
MasterPtato wants to merge 1 commit into04-05-chore_misc_fixes_add_pb_snapshot_testfrom
04-06-fix_envoy_use_global_instance_add_signal_handlers

Conversation

@MasterPtato
Copy link
Copy Markdown
Contributor

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

MasterPtato commented Apr 6, 2026

@railway-app
Copy link
Copy Markdown

railway-app bot commented Apr 6, 2026

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

Service Status Web Updated (UTC)
kitchen-sink ❌ Build Failed (View Logs) Web Apr 7, 2026 at 1:31 am
mcp-hub ✅ Success (View Logs) Web Apr 6, 2026 at 8:22 pm
frontend-inspector ❌ Build Failed (View Logs) Web Apr 6, 2026 at 8:08 pm
website ❌ Build Failed (View Logs) Web Apr 6, 2026 at 8:07 pm
ladle ❌ Build Failed (View Logs) Web Apr 6, 2026 at 8:07 pm
frontend-cloud ❌ Build Failed (View Logs) Web Apr 6, 2026 at 8:07 pm

@claude
Copy link
Copy Markdown

claude bot commented Apr 6, 2026

PR Review: fix(envoy): use global instance, add signal handlers

The refactor to a global singleton is the right approach, preventing multiple EngineActorDriver instances from each spawning their own envoy WebSocket connection. However, there are several issues worth addressing before merge.

Critical

1. Compilation break in test-envoy

engine/sdks/typescript/test-envoy/src/index.ts still uses the old onShutdown(reason: ShutdownReason) signature and serverlessStartPayload config option, both of which were removed in this PR. This will fail to compile. The test-envoy usage needs to be updated to the new onShutdown: () => void signature and serverlessStartPayload removed from that call site.

2. GLOBAL_ENVOY never cleared after shutdown

engine/sdks/typescript/envoy-client/src/tasks/envoy/index.ts: once the global envoy shuts down and the spawn loop exits, GLOBAL_ENVOY still holds the stopped handle. Any subsequent call to startEnvoySync (e.g., in tests or after a reconnect) returns the dead handle instead of creating a new one. GLOBAL_ENVOY should be set to undefined before calling onShutdown() in the cleanup path.

Medium

3. Signal handlers registered even for notGlobal: true envoys

When config.notGlobal is true, the code still registers process.once("SIGINT") and process.once("SIGTERM") handlers. Since each call creates a new closure, process.once will not deduplicate them, so calling startEnvoySync with notGlobal: true multiple times (e.g., in tests) accumulates handlers and triggers Node.js MaxListenersExceededWarning. Signal handler registration should be guarded by !config.notGlobal.

4. SSE abort handler silently became a no-op

rivetkit-typescript/packages/rivetkit/src/drivers/engine/actor-driver.ts: the previous code called this.shutdown(false) on SSE abort (which fires when the platform drops the connection). The new code only logs. If the assumption is that the platform always sends SIGTERM before dropping the SSE, that should be documented with a comment. As written it looks like dead code and leaves a potential shutdown-path gap if SIGTERM is not reliably delivered.

Minor

5. Dead config field serverlessStartPayload

engine/sdks/typescript/envoy-client/src/config.ts: serverlessStartPayload?: ArrayBuffer is still present in EnvoyConfig but is now silently ignored. The JSDoc comment also still references EnvoyHandle.startServerless (now startServerlessActor). This should either be removed or clearly marked deprecated.

6. ShutdownReason type has a dead variant

engine/sdks/typescript/envoy-client/src/utils.ts: "serverless-early-exit" is no longer reachable since that code path was removed. The exported type and the onShutdown callback no longer accept a reason at all. Clean up the export to avoid confusing consumers.

7. immediate parameter in shutdown() is ignored

The signal handler calls handle.shutdown(false) but false has no effect since the parameter is never used in the implementation. Worth fixing since the signal handler now relies on this method.

Positive Changes

  • Global singleton correctly prevents duplicate envoy connections from multiple driver instances.
  • Removing the serverless-specific early-exit logic simplifies the event loop.
  • PromiseWithResolvers<undefined> to PromiseWithResolvers<void> is a correct type fix.
  • "Runtime already started as runner" to "Runtime already started as serverful" is a more accurate invariant message.
  • The indentation fix in actor-driver.ts is correct.

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