Skip to content

fix(pegboard): decode envoy actor eviction keys#4803

Closed
NathanFlurry wants to merge 1 commit intoengine-stabilize/envoy-subscribe-before-registerfrom
engine-stabilize/envoy-evict-actor-keys
Closed

fix(pegboard): decode envoy actor eviction keys#4803
NathanFlurry wants to merge 1 commit intoengine-stabilize/envoy-subscribe-before-registerfrom
engine-stabilize/envoy-evict-actor-keys

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 27, 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.

@claude
Copy link
Copy Markdown

claude Bot commented Apr 27, 2026

Code Review: fix(pegboard): decode envoy actor eviction keys

Overview

A one-line bug fix in engine/packages/pegboard/src/ops/envoy/evict_actors.rs that corrects the key type used to decode actor entries when evicting actors from a disconnecting envoy.


The Bug

pegboard_envoy_evict_actors queries the envoy actor subspace (built from keys::envoy::ActorKey::subspace(...)) but was decoding each entry with keys::runner::ActorKey — a completely different key schema.

The two key types have incompatible tuple layouts:

Key type TuplePack layout
runner::ActorKey (RUNNER, ACTOR, runner_id, actor_id) — 4 elements
envoy::ActorKey (NAMESPACE, ENVOY, ACTOR, namespace_id, envoy_key, actor_id) — 6 elements

Attempting to unpack a 6-element envoy key through the 4-element runner schema would either panic on deserialization or silently produce a wrong actor_id, meaning the subsequent GoingAway signals would target the wrong actor workflows (or fail entirely). The fix is correct and minimal.

Code Quality

  • Fix is appropriately scoped — a single type parameter change.
  • The corrected type (keys::envoy::ActorKey) matches both the subspace being iterated and the actor_id field that is then used to route the GoingAway signal, so the full call chain is now self-consistent.
  • Consistent with other envoy ops: expire.rs, drain.rs, and list.rs all use only keys::envoy::* types for their range reads.

Potential Issues

  • No regression test. The function has no test coverage. It is only called from ws_to_tunnel_task.rs during envoy teardown, but given that this was a silent misuse that would not have been caught by the compiler (both types implement the same trait), a test exercising the eviction path would prevent this class of bug from regressing. Even a basic integration test that registers an actor under an envoy key and confirms eviction sends the right signal would add meaningful protection.
  • PR description is unfilled. Minor nit — the checklist and test sections were left as the template defaults. For a hotfix this small it is not a blocker, but filling in "Bug fix" and a brief test note keeps the audit trail clean.

Summary

The fix is correct, low-risk, and high-impact. The mismatch would have caused runtime deserialization errors or incorrect actor eviction on every envoy disconnection. Recommend merging, with a follow-up to add a regression test for the eviction path.

@NathanFlurry NathanFlurry force-pushed the engine-stabilize/envoy-subscribe-before-register branch from bdd460a to d5733bb Compare April 27, 2026 17:35
@NathanFlurry NathanFlurry force-pushed the engine-stabilize/envoy-evict-actor-keys branch 2 times, most recently from 297bda5 to 9b65473 Compare April 27, 2026 19:30
@NathanFlurry NathanFlurry force-pushed the engine-stabilize/envoy-subscribe-before-register branch 2 times, most recently from 80a124a to c491f97 Compare April 27, 2026 19:40
@NathanFlurry NathanFlurry force-pushed the engine-stabilize/envoy-evict-actor-keys branch 2 times, most recently from 23aa102 to fc994ce Compare April 27, 2026 20:48
@NathanFlurry NathanFlurry force-pushed the engine-stabilize/envoy-subscribe-before-register branch from 9b2fdc5 to e8f7ae6 Compare April 27, 2026 21:29
@NathanFlurry NathanFlurry force-pushed the engine-stabilize/envoy-evict-actor-keys branch from fc994ce to 3b94d07 Compare April 27, 2026 21:29
@NathanFlurry NathanFlurry force-pushed the engine-stabilize/envoy-evict-actor-keys branch from 3b94d07 to 7339874 Compare April 27, 2026 21:38
@NathanFlurry NathanFlurry force-pushed the engine-stabilize/envoy-subscribe-before-register branch from e8f7ae6 to 51ba630 Compare April 27, 2026 21:38
@NathanFlurry NathanFlurry force-pushed the engine-stabilize/envoy-subscribe-before-register branch from 51ba630 to f8eb8a6 Compare April 27, 2026 22:03
@NathanFlurry NathanFlurry force-pushed the engine-stabilize/envoy-evict-actor-keys branch from 7339874 to 095d57c Compare April 27, 2026 22:03
@NathanFlurry
Copy link
Copy Markdown
Member Author

Landed in main via stack-merge fast-forward push. Commits are in main; closing to match.

@github-actions
Copy link
Copy Markdown
Contributor

Preview packages published to npm

Install with:

npm install rivetkit@pr-4803

All packages published as 0.0.0-pr.4803.f672d48 with tag pr-4803.

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-f672d48
docker pull rivetdev/engine:full-f672d48
Individual packages
npm install rivetkit@pr-4803
npm install @rivetkit/react@pr-4803
npm install @rivetkit/rivetkit-napi@pr-4803
npm install @rivetkit/workflow-engine@pr-4803

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