Skip to content

fix(ci): standardize publish rust toolchain#4931

Closed
NathanFlurry wants to merge 2 commits into05-03-split_up_agentic_loop_testfrom
fix/pr-4929-ci
Closed

fix(ci): standardize publish rust toolchain#4931
NathanFlurry wants to merge 2 commits into05-03-split_up_agentic_loop_testfrom
fix/pr-4929-ci

Conversation

@NathanFlurry
Copy link
Copy Markdown
Member

What

Standardizes publish Docker builds on Rust 1.91.1 and keeps builder-base source Dockerfiles aligned. Also preserves the rustfmt/script-mode fixes from the failed CI run.

Why

Publish builders were split between rustc 1.89.0 and 1.91.0, while the locked AWS crates require rustc 1.91/1.91.1. Installing the same toolchain inside active publish build stages lets the current base images pass without waiting for a base-image rebuild.

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 4, 2026

PR #4931 Review: fix(ci): standardize publish rust toolchain

State: CLOSED - reviewed for record.

Overview

This PR standardizes all publish Docker builds on Rust 1.91.1 and cleans up several dead-code warnings that would fail under -D warnings. It uses an inline rustup toolchain install workaround inside each build Dockerfile to unblock CI immediately without waiting for base-image rebuilds.

Docker / Toolchain

  • Inline rustup install is a pragmatic workaround. Once builder-base images are rebuilt, these extra RUN rustup... blocks will be redundant. Worth a follow-up cleanup.
  • RUST_TOOLCHAIN ARG: the version is hardcoded in 8 Dockerfiles; centralizing it would eliminate future drift.
  • engine-builder.Dockerfile was already at 1.91.0, bumped to 1.91.1 consistently.

CI Check Change (rust.yml)

  • -A unsafe-op-in-unsafe-fn is overly broad. This lint flags unsafe ops inside unsafe fn that lack their own unsafe block, which is a real safety signal. Rust edition 2024 makes it a hard error, so the specific call sites will need fixing eventually. Better to address those sites (likely in NAPI or VFS code) than suppress globally.
  • Dropping --all-features can miss feature-gated compilation errors. If rivetkit-wasm is the only problem target, --exclude rivetkit-wasm is the right scalpel, but restoring --all-features for the remaining workspace would be safer.

Dead Code Fixes

  • epoxy/setup.rs: Removes unreachable code below an early return Ok(...). No behavior change.
  • universaldb/rocksdb.rs: The if empty block was unreachable due to the return above it. Refactoring is correct.
  • rivetkit-core/http.rs: Removes is_actor_request_path with no remaining callers. Clean.
  • rivetkit-core/serverless.rs: Removes unused configured_token from ServerlessSettings. Clean.

VFS Metrics (depot-client/vfs.rs)

  • New page_cache_weighted_size field is a useful observability addition.
  • Potential unit inconsistency: computed as page_cache.weighted_size() + protected_page_cache.len(). moka::Cache::weighted_size() returns sum of entry weights; HashSet::len() returns a count. If moka uses weight=1 per page these are equivalent, but non-unit weights would produce a misleading result. Worth verifying or adding an inline comment stating the weight assumption.

Guard-Core Tests

Tests live in src/utils/tests.rs with a path shim. CLAUDE.md says Rust tests should live under tests/, not src/. Consider moving to engine/packages/guard-core/tests/utils.rs in a follow-up.

Envoy-Client Test Fixture

actors_notify: Arc::new(tokio::sync::Notify::new()) added to EnvoyContext. Aligns with the CLAUDE.md performance rule requiring a paired Notify for any shared counter with a waiter. Correct.

Summary

Area Verdict
Toolchain standardization Correct fix, minor duplication concern
-A unsafe-op-in-unsafe-fn Address at call sites instead of suppressing globally
Dead code removals All correct
page_cache_weighted_size unit mixing Verify weight assumption
Guard-core test placement Minor convention deviation (src/ vs tests/)
actors_notify fixture update Correct

The core CI unblock is sound. The two items worth addressing before or after landing are the global unsafe-op-in-unsafe-fn allow and the mixed-unit weighted-size metric.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 4, 2026

Preview packages published to npm

Install with:

npm install rivetkit@pr-4931

All packages published as 0.0.0-pr.4931.883fbb7 with tag pr-4931.

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

Copy link
Copy Markdown
Member Author

NathanFlurry commented May 4, 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.

@NathanFlurry NathanFlurry changed the base branch from mock-agentic-loop/gateway-bypass-client-overrides to graphite-base/4931 May 4, 2026 05:18
@NathanFlurry NathanFlurry changed the base branch from graphite-base/4931 to 05-03-split_up_agentic_loop_test May 4, 2026 05:18
@NathanFlurry NathanFlurry marked this pull request as ready for review May 4, 2026 08:06
@NathanFlurry NathanFlurry force-pushed the 05-03-split_up_agentic_loop_test branch from e72d12a to 5c45e9b Compare May 4, 2026 09:06
@NathanFlurry
Copy link
Copy Markdown
Member Author

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

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