Skip to content

fix: optimize e2e actor path#4551

Merged
NathanFlurry merged 1 commit intomainfrom
04-03-fix_optimize_e2e_actor_path
Apr 5, 2026
Merged

fix: optimize e2e actor path#4551
NathanFlurry merged 1 commit intomainfrom
04-03-fix_optimize_e2e_actor_path

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

@railway-app
Copy link
Copy Markdown

railway-app bot commented Apr 4, 2026

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

Service Status Web Updated (UTC)
kitchen-sink ❌ Build Failed (View Logs) Web Apr 5, 2026 at 11:39 am
frontend-cloud ❌ Build Failed (View Logs) Web Apr 4, 2026 at 4:00 am
frontend-inspector ❌ Build Failed (View Logs) Web Apr 4, 2026 at 4:00 am
mcp-hub ✅ Success (View Logs) Web Apr 4, 2026 at 3:59 am
website ❌ Build Failed (View Logs) Web Apr 4, 2026 at 3:59 am
ladle ❌ Build Failed (View Logs) Web Apr 4, 2026 at 3:59 am

@railway-app railway-app bot temporarily deployed to rivet-frontend / rivet-pr-4551 April 4, 2026 03:58 Destroyed
@claude
Copy link
Copy Markdown

claude bot commented Apr 4, 2026

PR Review: fix: optimize e2e actor path

Summary

This PR reduces end-to-end actor path latency through namespace operation caching, reqwest client pre-warming, tracing improvements, an envoy client shutdown race fix, and new Sleep/Reallocate actor signals.


Potential Bugs

1. Debug /foo endpoint left in test-envoy
engine/sdks/typescript/test-envoy/src/index.ts contains a new app.post route that looks like debug scaffolding. If intentional, rename it to something meaningful.

2. Race condition: abort handler fires before envoy is assigned
envoy is declared inside a setTimeout callback, but the SSE abort handler references envoy with a non-null assertion. If the client disconnects before the 0ms timeout fires, envoy will be undefined at abort time. The assertion silently skips the shutdown call, meaning the underlying process may not be cleaned up on early disconnect. The same pattern exists in /api/rivet/start.

3. LOG_TIMESTAMP default flip is a silent behavioral change
Previously timestamps were opt-in to enable. Now they are opt-out to disable. Any existing environment relying on the old default will silently start emitting timestamps. Since this is test-envoy the impact is limited, but it should be noted in the PR description.


Code Quality

4. TODO without issue reference in sleep signal handler
packages/core/pegboard/src/protocol/mod.rs has a TODO comment with no tracking issue. When an actor is signaled to sleep during Allocating/Starting/GoingAway, the sleep intent is silently dropped. Link a tracking issue if this is a known limitation.

5. Main::Reallocate silent drop while Reallocating
A second Reallocate signal during an in-progress reallocation is silently dropped. Consider adding a tracing::debug! to make the drop observable, and clarify that Reallocating is distinct from Allocating.

6. Leftover console.log in test script
scripts/tests/actor_e2e.ts has two bare console.log(new Date().toISOString()) calls that duplicate the surrounding console.time/console.timeEnd instrumentation. These look like debug artifacts.


Performance

Namespace caching (good): Both namespace_get_local and namespace_resolve_for_name_local are now cached via fetch_all_json/fetch_one_json. Cache misses when namespace is not found correctly avoid polluting the cache with negative results.

Reqwest pre-warming (good): Eagerly initializing clients in Pools::create() matches the CLAUDE.md guidance to reuse rivet_pools::reqwest::client().


Minor

7. RUST_LOG=warn removal from engine-rocksdb-release.sh
The release script no longer sets RUST_LOG=warn, which may make release builds noisier. Confirm this is intentional and not a debugging leftover.

8. pnpm-lock.yaml specifier mismatch for @types/bun
Lockfile records specifier: latest but package.json has a pinned version. The resolved version matches but the specifier mismatch could cause pnpm install to silently update on the next run. Regenerate the lockfile.


Convention Checks (CLAUDE.md)

  • rivet.dev domain: no violations
  • Structured logging: all new tracing:: calls use structured fields correctly
  • No new Arc<Mutex> instances introduced
  • Workspace dependencies: no new Cargo deps added outside workspace

Overall this is a solid performance improvement. Main items to address before merge: remove the /foo debug endpoint, clean up the console.log timestamp artifacts, and confirm the RUST_LOG removal is intentional. The sleep/reallocate signal edge cases and abort handler race are worth tracking issues even if not merge-blocking.

Copy link
Copy Markdown
Member

NathanFlurry commented Apr 5, 2026

Merge activity

  • Apr 5, 11:11 AM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Apr 5, 11:39 AM UTC: Graphite rebased this pull request as part of a merge.
  • Apr 5, 11:40 AM UTC: @NathanFlurry merged this pull request with Graphite.

@NathanFlurry NathanFlurry changed the base branch from 04-02-feat_pb_actor_v1_to_v2_migration to graphite-base/4551 April 5, 2026 11:36
@NathanFlurry NathanFlurry changed the base branch from graphite-base/4551 to main April 5, 2026 11:37
@NathanFlurry NathanFlurry force-pushed the 04-03-fix_optimize_e2e_actor_path branch from f52ab07 to b453037 Compare April 5, 2026 11:38
@railway-app railway-app bot temporarily deployed to rivet-frontend / rivet-pr-4551 April 5, 2026 11:38 Destroyed
@NathanFlurry NathanFlurry merged commit 3d61f45 into main Apr 5, 2026
10 of 19 checks passed
@NathanFlurry NathanFlurry deleted the 04-03-fix_optimize_e2e_actor_path branch April 5, 2026 11:40
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.

3 participants