Skip to content

feat(envoy-client): fully flesh out tunnel impl#4547

Merged
NathanFlurry merged 1 commit intomainfrom
04-01-feat_envoy-client_fully_flesh_out_tunnel_impl
Apr 5, 2026
Merged

feat(envoy-client): fully flesh out tunnel impl#4547
NathanFlurry merged 1 commit intomainfrom
04-01-feat_envoy-client_fully_flesh_out_tunnel_impl

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 2, 2026

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

Service Status Web Updated (UTC)
kitchen-sink ❌ Build Failed (View Logs) Web Apr 5, 2026 at 11:35 am
frontend-inspector 🕒 Building (View Logs) Web Apr 5, 2026 at 11:35 am
website ❌ Build Failed (View Logs) Web Apr 3, 2026 at 1:25 am
frontend-cloud ❌ Build Failed (View Logs) Web Apr 3, 2026 at 1:25 am
mcp-hub ✅ Success (View Logs) Web Apr 2, 2026 at 2:48 am
ladle ❌ Build Failed (View Logs) Web Apr 2, 2026 at 2:48 am

@railway-app railway-app bot temporarily deployed to rivet-frontend / rivet-pr-4547 April 2, 2026 02:47 Destroyed
@MasterPtato MasterPtato force-pushed the 04-01-feat_envoy-client_fully_flesh_out_tunnel_impl branch from f9d0c82 to 2d95295 Compare April 2, 2026 22:04
@railway-app railway-app bot temporarily deployed to rivet-frontend / rivet-pr-4547 April 2, 2026 22:04 Destroyed
@MasterPtato MasterPtato force-pushed the 04-01-feat_envoy-client_fully_flesh_out_tunnel_impl branch from 2d95295 to 56e899b Compare April 3, 2026 01:24
@MasterPtato MasterPtato force-pushed the 03-31-feat_basic_rivetkit_impl branch from 7e0e9b7 to 516d91e Compare April 3, 2026 01:24
@railway-app railway-app bot temporarily deployed to rivet-frontend / rivet-pr-4547 April 3, 2026 01:24 Destroyed
@NathanFlurry NathanFlurry mentioned this pull request Apr 4, 2026
11 tasks
@NathanFlurry NathanFlurry marked this pull request as ready for review April 5, 2026 10:58
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:35 AM UTC: Graphite rebased this pull request as part of a merge.
  • Apr 5, 11:35 AM UTC: @NathanFlurry merged this pull request with Graphite.

@NathanFlurry NathanFlurry changed the base branch from 03-31-feat_basic_rivetkit_impl to graphite-base/4547 April 5, 2026 11:32
@NathanFlurry NathanFlurry changed the base branch from graphite-base/4547 to main April 5, 2026 11:33
@NathanFlurry NathanFlurry force-pushed the 04-01-feat_envoy-client_fully_flesh_out_tunnel_impl branch from 56e899b to e0b34ec Compare April 5, 2026 11:34
@railway-app railway-app bot temporarily deployed to rivet-frontend / rivet-pr-4547 April 5, 2026 11:34 Destroyed
@NathanFlurry NathanFlurry merged commit 2f3c056 into main Apr 5, 2026
10 of 20 checks passed
@NathanFlurry NathanFlurry deleted the 04-01-feat_envoy-client_fully_flesh_out_tunnel_impl branch April 5, 2026 11:35
@claude
Copy link
Copy Markdown

claude bot commented Apr 5, 2026

PR Review: feat(envoy-client): fully flesh out tunnel impl

Critical Issues

1. v1.bare protocol schema modified in-place (breaking wire format)

Per CLAUDE.md: "Never modify an existing published *.bare runner protocol version unless explicitly asked to do so. Add a new versioned schema instead."

ToEnvoyConnClose is inserted into the ToEnvoyConn union in engine/sdks/schemas/envoy-protocol/v1.bare. Inserting at index 1 shifts the ordinals of ToEnvoyCommands, ToEnvoyAckEvents, and ToEnvoyTunnelMessage — a breaking wire-format change for existing v1 clients. A new v2.bare schema should be created instead, and compatibility code should bridge the versions.

2. Generation check inversion in actor2/mod.rs — needs verification and comment

The comment says "Ignore signals for previous generations." The new code:

if sig.generation != state.generation {
    return Ok(Loop::Continue);
}

This skips signals whose generation doesn't match the current state — which is correct for the stated intent. But the logic before this change returned Continue on ==, which would have processed stale signals. Please add a comment confirming this is intentional and was a pre-existing bug fix, since the inversion is non-obvious.


Moderate Issues

3. Six subscriptions always created regardless of actor_v2 branch

engine/packages/pegboard/src/ops/actor/create.rs: All six subscriptions (create_sub, fail_sub, destroy_sub, create_sub2, fail_sub2, destroy_sub2) are created eagerly via tokio::try_join!. When actor_v2 == true, the v1 set is unused; when actor_v2 == false, the v2 set is unused. These subscriptions should be created lazily based on the runtime flag to avoid unnecessary resource allocation.

4. Pagination cursor not stable across DC fanout

engine/packages/api-public/src/envoys.rs: The cursor is create_ts. After the first page is fetched, the same created_before timestamp is sent to all DCs on subsequent calls. DCs that already exhausted their entries return nothing, while others may still have data, and entries near the page boundary could be duplicated or skipped across pages. Consider a more robust cursor design for cross-DC pagination.


Minor Issues

5. from_v1: bool lacks doc comment

engine/packages/pegboard/src/workflows/actor2/mod.rs: This flag skips several behaviors (key reservation, create_ts write, metrics increment, CreateComplete message). A brief doc comment explaining why migrated-from-v1 actors need these skipped would help future maintainers.

6. Redundant field initialization

engine/packages/pegboard/src/ops/envoy/get.rs:

namespace_id: namespace_id,  // should be just `namespace_id,`

7. Typo in TypeScript comment

engine/sdks/typescript/envoy-client/src/tasks/actor.ts:

// - Content-Type: tbackgroundOperationsext/event-stream
// should be:
// - Content-Type: text/event-stream

8. input.crash_policy indentation copied from v1 branch

engine/packages/pegboard/src/ops/actor/create.rs: The crash_policy argument in the new v2 forward_to_datacenter(...) call has incorrect indentation (same pre-existing issue in the v1 branch that was copied without correction).

9. runner_version no longer traced alongside envoy_version

engine/packages/pegboard/src/ops/serverless_metadata/fetch.rs: The debug log previously included ?runner_version; the new version only logs ?envoy_version. Including both would help when debugging v1 (runner-based) deployments.


Positive Changes

  • Service manager fix for duplicate force-shutdown task spawning is clean and correct.
  • Graceful outbound shutdown loop mirrors the worker shutdown pattern well.
  • get_inner extracted to pub(crate) for reuse in list.rs — good refactor.
  • BufferMap (renamed from BatchMap) fixing composite key handling is a real correctness fix.
  • protocol_version-based v1/v2 dispatch at runtime is a sensible design.
  • Envoy drain on version upgrade with config flag and metrics counter is well-structured.
  • Structured logging consistently follows CLAUDE.md conventions throughout new Rust code.
  • No rivet.gg domain references introduced.

Severity Issue
Critical v1.bare modified, breaking wire format
Critical Generation check inversion needs verification/comment
Moderate Six subscriptions created eagerly regardless of branch taken
Moderate Cross-DC pagination cursor not stable
Minor from_v1 missing doc comment
Minor namespace_id: namespace_id redundant field init
Minor Typo in TypeScript comment
Minor crash_policy indentation copied incorrectly
Minor runner_version dropped from debug log

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