Skip to content

fix(rivetkit): restore local engine development thresholds - #5610

Open
NathanFlurry wants to merge 3 commits into
mainfrom
cargo-watch-issue
Open

fix(rivetkit): restore local engine development thresholds#5610
NathanFlurry wants to merge 3 commits into
mainfrom
cargo-watch-issue

Conversation

@NathanFlurry

@NathanFlurry NathanFlurry commented Aug 27, 2026

Copy link
Copy Markdown
Member
  • Restore fast local actor recovery and shutdown thresholds for active Envoy and legacy runner paths.
  • Publish Engine checksum manifests required by Rust automatic downloads.
  • Add regression coverage for development thresholds.

@railway-app

railway-app Bot commented Aug 27, 2026

Copy link
Copy Markdown

🚅 Deployed to the actors-pr-5610 environment in rivet-frontend

Service Status Web Updated (UTC)
frontend-cloud 😴 Sleeping (View Logs) Web Aug 28, 2026 at 5:09 pm
kitchen-sink 😴 Sleeping (View Logs) Web Aug 28, 2026 at 1:48 pm
frontend-inspector 😴 Sleeping (View Logs) Web Aug 28, 2026 at 12:49 pm
website ❌ Build Failed (View Logs) Web Aug 28, 2026 at 8:05 am
ladle ✅ Success (View Logs) Web Aug 27, 2026 at 8:36 pm
mcp-hub ✅ Success (View Logs) Web Aug 27, 2026 at 8:34 pm

@claude

claude Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Review

Reviewed the diff (dev-env threshold sync between scripts/run/dev-env.sh and rivetkit-rust/packages/engine-process/src/lib.rs::engine_env, plus the CI checksum step). No correctness bugs found. Units and invariants check out (ms vs. seconds fields are set correctly, force_shutdown_duration >= worker/guard holds, the new ENVOY_ELIGIBLE_THRESHOLD/ENVOY_LOST_THRESHOLD mirror the existing RUNNER_* pair correctly), and engine_env has only the two expected callers (rivet dev and rivet engine, both local-dev-only per docs/content/docs/cli.mdx).

A few lower-severity notes for consideration:

  1. engine_env now permanently bakes 11 dev-only threshold overrides into a function shared by two callers (rivetkit-rust/packages/engine-process/src/lib.rs:373-449). Both current callers are dev-only today, so this is safe now, but the function doc comment says it is shared by the spawn path and by callers that exec the engine binary directly. Nothing stops a future non-dev caller from silently inheriting shortened recovery/shutdown thresholds. Consider an explicit is_dev: bool parameter or override list rather than hardcoded constants, so the dev-only intent is enforced by the type signature rather than a comment.

  2. Verbose literal, easy to typo or unbalance (lib.rs:390-449): the 11 .to_owned() key/value pairs are exactly the kind of hand-maintained list whose drift from dev-env.sh this PR is fixing. A const DEV_THRESHOLDS table merged in via chain/extend would reduce boilerplate and make the single source of truth easier to keep in sync.

  3. The dev-env.sh to engine_env sync is still manual and textual (scripts/run/dev-env.sh:4-5): the comment now correctly points at engine_env (the old target, a nonexistent TS path, was stale), but there is still no shared generation step or test that cross-checks the two lists, so the same silent-drift failure mode this PR fixes could recur. Not a blocker, but worth a follow-up if this list grows further.

  4. Minor convention note: the new test engine_env_uses_development_recovery_thresholds is added inline in src/, whereas CLAUDE.md asks for Rust tests to live under tests/ with justified exceptions. This matches the file pre-existing all-inline-test convention, so it is not a regression introduced by this PR. Flagging only for awareness, not as something to fix here.

Nothing blocking; comments and shell script changes follow repo conventions (no em dashes, complete-sentence comments).

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