fix: add version rollback check

[MasterPtato]

No description provided.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Error			Oct 31, 2025 1:17am

3 Skipped Deployments

Project	Deployment	Preview	Comments	Updated (UTC)
rivet-cloud	Ignored			Oct 31, 2025 1:17am
rivet-inspector	Ignored	Preview		Oct 31, 2025 1:17am
rivet-site	Ignored	Preview		Oct 31, 2025 1:17am

[MasterPtato]

• fix: add version rollback check #3291 👈 (View in Graphite)
• fix: integrate sentry with tracing and axum #3290
• chore: remove headers through guard #3288
• feat: add ws msg ack to runner protocol #3275 : 1 other dependent PR (#3282 )
• fix: improve reconnection with sleeping and tunnel #3261
• main

---

How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

[claude]

PR Review: Version Rollback Protection

This PR adds a safeguard to prevent the Rivet Engine from being rolled back to a previous version, which could cause data corruption. Overall, the implementation is solid and addresses an important production safety concern.

✅ Strengths

1. Critical Safety Feature: Prevents accidental version rollbacks that could corrupt data
2. Good UX: Clear error messages with actionable information showing both versions
3. Escape Hatch: Provides allow_version_rollback config option for intentional downgrades
4. Proper Dependencies: Uses workspace dependencies correctly (semver.workspace = true, indoc.workspace = true)
5. Follows Conventions: Matches existing patterns for database keys and error handling

🔍 Issues & Recommendations

1. Critical: Missing imports in keys.rs ❗

The keys.rs file references RIVET and VERSION constants without importing them:

// Line 31-32
let t = (RIVET, VERSION);

These constants come from universaldb::prelude::*. The file needs:

use universaldb::prelude::*;

Without this import, the code will not compile.

2. Unclear Error Handling Pattern 🤔

The double Result wrapping in verify_engine_version is confusing:

return Ok(Err(anyhow!(...)))  // Line 125

This pattern makes the code harder to read. Consider refactoring to:

pools
    .udb()?
    .run(|tx| async move {
        let current_version = semver::Version::parse(env!("CARGO_PKG_VERSION"))
            .context("failed to parse cargo pkg version as semver")?;

        if let Some(existing_version) = tx.read_opt(&keys::EngineVersionKey {}, Serializable).await? {
            if current_version < existing_version {
                bail!("{}", formatdoc!(...));
            }
        }

        tx.write(&keys::EngineVersionKey {}, current_version)?;
        Ok(())
    })
    .await

Using bail! from anyhow is more idiomatic for early error returns.

3. Isolation Level Import Style 📝

Line 7 in start.rs:

use universaldb::utils::IsolationLevel::*;

This wildcard import only for Serializable is inconsistent. Per CLAUDE.md conventions, either:

• Import explicitly: use universaldb::utils::IsolationLevel::Serializable;
• Or use the qualified path: IsolationLevel::Serializable

4. Missing Test Coverage 🧪

No tests were added for this critical safety feature. Recommended test cases:

• ✅ First startup (no existing version) → should succeed and write version
• ✅ Same version restart → should succeed
• ✅ Upgrade (newer version) → should succeed and update version
• ❌ Downgrade without flag → should fail with clear error
• ✅ Downgrade with allow_version_rollback: true → should succeed with warning

Consider adding integration tests in engine/packages/engine/tests/.

5. Constructor Not Used 🧹

The EngineVersionKey::new() method (lines 7-10 in keys.rs) is defined but never used. Since the struct is a unit struct with no fields, direct construction EngineVersionKey {} is clearer. Consider removing the constructor or using it consistently.

6. Logging Recommendations 📊

Per CLAUDE.md logging patterns, consider adding structured logs:

tracing::info!(
    current = %current_version,
    existing = ?existing_version,
    "verifying engine version"
);

This would help with debugging version-related issues in production.

7. Minor: Unused anyhow Import

Line 3 in start.rs imports anyhow directly when the pattern should be use anyhow::*; per CLAUDE.md guidelines. However, the current selective import (Context, Result, anyhow) is actually fine and more explicit.

🔒 Security Considerations

✅ No security issues identified. The version check happens early in startup before any critical operations.

⚡ Performance Considerations

✅ Minimal performance impact - single database read/write during startup only.

📋 Suggested Action Items

Must Fix Before Merge:

1. Add missing import in keys.rs: use universaldb::prelude::*;

Recommended:
2. Simplify error handling pattern (use bail!)
3. Add integration tests for version verification logic
4. Add structured logging for version checks
5. Fix import style for IsolationLevel

📊 Code Quality: 7.5/10

The implementation demonstrates good understanding of the codebase and follows most conventions, but the missing import is a blocker and the lack of tests for a safety-critical feature is concerning.