fix(udb): fix rocksdb conflict tracker

[MasterPtato]

No description provided.

[vercel]

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

3 Skipped Deployments

Project	Deployment	Preview	Comments	Updated (UTC)
rivet-cloud	Ignored			Oct 18, 2025 7:14am
rivet-inspector	Ignored	Preview		Oct 18, 2025 7:14am
rivet-site	Ignored	Preview		Oct 18, 2025 7:14am

💡 Enable Vercel Agent with $100 free credit for automated AI reviews

[claude]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[MasterPtato]

• fix(cache): fix recursive cache purges #3221
• fix(udb): fix rocksdb conflict tracker #3225 👈 (View in Graphite)
• feat(engine): add RocksDB engine run script #3220
• 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.

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/rivet-dev/engine/@rivetkit/engine-runner@3225

npm i https://pkg.pr.new/rivet-dev/engine/@rivetkit/engine-runner-protocol@3225

commit: ae7e816

[claude]

Pull Request Review: Fix RocksDB Conflict Tracker

Summary

This PR refactors the RocksDB conflict tracking mechanism from an eager, per-operation conflict checking system to a deferred, commit-time validation system. This is a significant architectural improvement that aligns better with optimistic concurrency control patterns.

Code Quality and Architecture

✅ Strengths

1. Better Concurrency Model: The new TransactionConflictTracker properly implements optimistic concurrency control by deferring conflict detection until commit time, rather than checking eagerly on every read/write operation. This reduces lock contention during transaction execution.

2. Simplified Transaction Lifecycle: Removing the Drop implementation and manual transaction ID tracking simplifies the code and eliminates potential issues with cleanup ordering.

3. Atomic Operations: Good use of AtomicU64 for version tracking and AtomicI32 for retry limits in database.rs:27,65. This is more efficient than using Mutex<i32>.

4. Time-based Pruning: The 10-second TTL for transaction conflicts (transaction_conflict_tracker.rs:15) is reasonable given the 5-second transaction timeout.

5. Comprehensive Test Coverage: The new test in rocksdb.rs:88-138 provides good coverage for concurrent transaction scenarios.

Potential Issues

🔴 Critical: Incorrect Conflict Detection Logic

Location: transaction_conflict_tracker.rs:60-69

The conflict detection logic has a critical bug:

// Check txn versions overlap
if txn1_start_version > txn2.start_version && txn1_start_version < txn2.commit_version {
    for (cr1_start, cr1_end, cr1_type) in &txn1_conflict_ranges {
        for (cr2_start, cr2_end, cr2_type) in &txn2.conflict_ranges {
            // Check conflict ranges overlap
            if cr1_start < cr2_end && cr2_start < cr1_end && cr1_type != cr2_type {
                return true;
            }
        }
    }
}

Issues:

1. Version overlap check is incomplete: You only check if txn1_start_version is between txn2.start_version and txn2.commit_version, but you also need to check the inverse case where txn2_start_version is between txn1's versions. Currently, if txn2 starts during txn1's lifetime, it won't be detected.

2. Read-Read conflicts: The condition cr1_type != cr2_type means you only detect conflicts when types are different (read-write or write-read). However, this incorrectly allows read-read conflicts to pass. While read-read shouldn't conflict in serializable isolation, the logic should be (cr1_type == Write || cr2_type == Write) to be more explicit.

3. Race condition in version assignment: txn1_commit_version is assigned at line 53 BEFORE checking conflicts. If two transactions commit concurrently, they could both pass the conflict check and then both get their commit versions assigned, missing a conflict.

Recommended fix:

pub async fn check_and_insert(
    &self,
    txn1_start_version: u64,
    txn1_conflict_ranges: Vec<(Vec<u8>, Vec<u8>, ConflictRangeType)>,
) -> bool {
    let mut txns = self.txns.lock().await;
    
    // Prune old entries
    txns.retain(|txn| txn.insert_instant.elapsed() < TXN_CONFLICT_TTL);

    for txn2 in &*txns {
        // Check if transactions overlap in time
        // txn1 conflicts with txn2 if txn1 started before txn2 committed
        if txn1_start_version < txn2.commit_version {
            for (cr1_start, cr1_end, cr1_type) in &txn1_conflict_ranges {
                for (cr2_start, cr2_end, cr2_type) in &txn2.conflict_ranges {
                    // Check if ranges overlap
                    let ranges_overlap = cr1_start < cr2_end && cr2_start < cr1_end;
                    // Conflict if at least one is a write
                    let has_write = matches!(cr1_type, ConflictRangeType::Write) 
                                 || matches!(cr2_type, ConflictRangeType::Write);
                    
                    if ranges_overlap && has_write {
                        return true;
                    }
                }
            }
        }
    }

    // Only assign commit version after successfully checking conflicts
    let txn1_commit_version = self.next_global_version();
    
    // If no conflicts were detected, save txn data
    txns.push(PreviousTransaction {
        insert_instant: Instant::now(),
        start_version: txn1_start_version,
        commit_version: txn1_commit_version,
        conflict_ranges: txn1_conflict_ranges,
    });

    false
}

🟡 Medium: Missing Cleanup on Transaction Failure

Location: transaction_task.rs:389-390

When a RocksDB commit fails, you call self.txn_conflict_tracker.remove(start_version). However, this only removes by start_version, but the transaction was already inserted into the tracker at line 379. If the commit fails due to RocksDB-level conflicts, you're removing it correctly, but:

1. The remove function searches by start_version (line 86-91), but multiple transactions could theoretically have operations that start at similar times.
2. If the RocksDB commit succeeds but returns Ok(_), the transaction remains in the tracker until TTL expires. This is correct, but worth documenting.

🟡 Medium: Transaction Timeout Handling

Location: database.rs:25,78-81

The transaction timeout is set to 5 seconds:

const TXN_TIMEOUT: Duration = Duration::from_secs(5);

But when a timeout occurs, you return DatabaseError::TransactionTooOld, which is marked as retryable in error.rs:22. However, if a transaction times out, retrying it with the same logic will likely timeout again. Consider:

1. Logging a warning when timeouts occur to help debug slow transactions
2. Potentially using a different error type or tracking timeout retries separately

🟡 Medium: SeqCst Ordering May Be Overkill

Location: transaction_conflict_tracker.rs:44, database.rs:73,118

You're using Ordering::SeqCst for atomic operations. While correct, this is the strongest (and slowest) memory ordering. For the version counter, Ordering::AcqRel or even Ordering::Relaxed for fetch_add would likely suffice since the mutex in check_and_insert provides the necessary synchronization.

🟢 Minor: Unused Variable

Location: metrics/providers.rs:75

The mut keyword was removed from resource, which is correct since it's not mutated. Good cleanup.

🟢 Minor: Documentation

The comment at transaction_conflict_tracker.rs:13-14 mentions "Transactions cannot live longer than 5 seconds" but the TTL is 10 seconds. Consider updating:

// Transactions timeout after 5 seconds, but we keep conflict data for 10 seconds 
// to ensure proper conflict detection for any in-flight commits
const TXN_CONFLICT_TTL: Duration = Duration::from_secs(10);

Performance Considerations

✅ Improvements

• Removing per-operation conflict tracking reduces lock contention significantly
• Using AtomicI32 instead of Mutex<i32> for retry limits is more efficient
• Time-based pruning prevents unbounded memory growth

⚠️ Potential Concerns

• The O(n*m*k) conflict checking (transactions × conflict_ranges × conflict_ranges) could be slow with many concurrent transactions. Consider using interval trees or spatial indexing if this becomes a bottleneck.
• The async Mutex in check_and_insert serializes all commits. This is correct for safety but limits commit throughput.

Security Considerations

No security issues identified. The conflict tracking mechanism properly prevents race conditions in transaction commits.

Test Coverage

✅ Good Coverage

• rocksdb.rs:88-138 tests concurrent transaction scenarios with 64 parallel tasks
• rocksdb.rs:11-85 tests native RocksDB behavior for comparison

🔴 Missing Tests

1. No test for conflict detection: You should add a test that verifies conflicts are actually detected:

   #[tokio::test]
   async fn test_conflict_detection() {
       // Two transactions that read and write the same key
       // One should fail with NotCommitted error
   }

2. No test for timeout handling: Add a test that verifies the 5-second timeout works correctly.

3. No test for the pruning logic: Verify that old transactions are properly removed after 10 seconds.

Recommendations

Must Fix (Blocking)

1. Fix the conflict detection logic in transaction_conflict_tracker.rs:60-69 as detailed above

Should Fix (Before Merge)

1. Add test coverage for conflict detection
2. Add test coverage for transaction timeout
3. Update the comment about transaction TTL for clarity
4. Consider reducing memory ordering from SeqCst to AcqRel where appropriate

Nice to Have (Follow-up PR)

1. Add metrics/logging for transaction conflicts and timeouts
2. Consider optimizing conflict range checking with spatial indexing if performance becomes an issue
3. Document the transaction lifecycle and conflict detection algorithm

Conclusion

This is a solid architectural improvement that simplifies the codebase and improves concurrency. However, the critical bug in conflict detection logic must be fixed before merging as it could allow phantom reads and lost updates in concurrent scenarios.

Great work overall on the refactoring! The direction is definitely correct.

---

🤖 Generated with Claude Code

[claude]

Code Review: Fix RocksDB Conflict Tracker

Overview

This PR refactors the RocksDB conflict tracking mechanism from an eager, pre-operation conflict detection approach to a lazy, commit-time validation approach. This is a significant architectural improvement that aligns the RocksDB driver more closely with how optimistic concurrency control typically works.

Positive Changes

1. Improved Conflict Detection Architecture ✅

• The new TransactionConflictTracker using version-based conflict detection (MVCC-style) is much more appropriate for optimistic transactions
• Moving conflict checking to commit time (check_and_insert) rather than during operations is the correct approach for optimistic concurrency
• The version numbering system (start_version, commit_version) provides a clean way to detect overlapping transactions

2. Simplified Transaction Logic ✅

• Removing the old ConflictRangeTracker that eagerly checked conflicts on every operation is a good simplification
• Transaction operations no longer need to track conflicts during reads/writes, reducing overhead
• The reset() method is now cleaner without manual transaction ID management

3. Better Concurrency Primitives ✅

• Using AtomicI32 instead of Arc<Mutex<i32>> for max_retries in both Postgres and RocksDB drivers (lines in database.rs)
• Properly using AtomicU64 with fetch_add for version generation

4. Transaction Timeout ✅

• Adding TXN_TIMEOUT (5 seconds) with tokio::time::timeout in database.rs:73-81 is excellent for preventing stuck transactions
• Returns DatabaseError::TransactionTooOld on timeout

5. Test Coverage ✅

• New rocksdb.rs test file with both native RocksDB and UDB wrapper tests
• Concurrent test with 64 parallel transactions is good for stress testing

Issues and Concerns

1. Critical Bug: Incorrect Conflict Detection Logic 🔴
In transaction_conflict_tracker.rs:60-66, the conflict detection has a logical error:

if txn1_start_version > txn2.start_version && txn1_start_version < txn2.commit_version {
    // Check ranges...
    if cr1_start < cr2_end && cr2_start < cr1_end && cr1_type != cr2_type {
        return true;
    }
}

Issues:

• Line 60: The version overlap check is incorrect. It should be checking if the transactions overlap in time, which means: txn1_start_version < txn2.commit_version && txn2.start_version < txn1.commit_version
• Line 64: The condition cr1_type != cr2_type only detects read-write conflicts but misses write-write conflicts! It should be cr1_type == ConflictRangeType::Write || cr2_type == ConflictRangeType::Write

Correct logic should be:

// Check if transactions overlap in time
if txn1_start_version < txn2.commit_version && txn2.start_version < txn1_commit_version {
    for (cr1_start, cr1_end, cr1_type) in &txn1_conflict_ranges {
        for (cr2_start, cr2_end, cr2_type) in &txn2.conflict_ranges {
            // Check if ranges overlap
            if cr1_start < cr2_end && cr2_start < cr1_end {
                // Conflict if at least one is a write
                if *cr1_type == ConflictRangeType::Write || *cr2_type == ConflictRangeType::Write {
                    return true;
                }
            }
        }
    }
}

2. Transaction Cleanup Issue ⚠️
In transaction_task.rs:389-390, when RocksDB commit fails, you call:

self.txn_conflict_tracker.remove(start_version).await;

However, the transaction was already inserted into the tracker at line 379. If a RocksDB conflict occurs (line 395), you correctly return DatabaseError::NotCommitted, but the transaction has been removed from the tracker. This could allow subsequent transactions to miss conflicts with this partially-committed transaction.

Suggestion: Only remove from tracker on non-conflict errors, or reconsider whether removal is needed at all since TTL-based cleanup handles this.

3. Missing Conflict Ranges ⚠️
In transaction.rs, all the methods (set, clear, clear_range) no longer add conflict ranges during operations. While operations are recorded in self.operations, I don't see where conflict ranges are being populated before commit.

Looking at commit_ref() at line 237, it consumes conflict_ranges, but these appear to only come from explicit add_conflict_range() calls. Are conflict ranges being generated from operations somewhere? If not, the conflict tracker will have empty conflict ranges and won't detect any conflicts.

4. Postgres Transaction Reset Issue 🔴
In postgres/transaction.rs:252, you're resetting the transaction by creating a new OnceCell:

self.tx_sender = OnceCell::new();

However, this leaves the old transaction task running! The old task will continue to hold database resources. You should either:

• Send a cancel command before replacing the sender
• Track and properly clean up the old task
• Document that this is intentional behavior

5. Potential Race Condition ⚠️
In transaction_conflict_tracker.rs:52-56, there's a potential race:

let mut txns = self.txns.lock().await;
let txn1_commit_version = self.next_global_version();  // Line 53

The next_global_version() is called while holding the lock, which is good. However, if another transaction gets its commit version between line 53 and when this transaction is inserted at line 73, the ordering could be wrong.

Recommendation: This is likely fine since the lock is held throughout, but add a comment explaining the locking guarantees.

6. Time-Based Cleanup Concern ⚠️
The TXN_CONFLICT_TTL (10 seconds) in line 15 is twice the transaction timeout (5 seconds). While conservative, this means:

• Memory usage grows linearly with transaction throughput for 10 seconds
• Under high load (1000s of transactions/sec), this could accumulate significant memory

Suggestion: Consider making the TTL configurable or add a max size limit with LRU eviction.

7. Test Coverage Gaps ⚠️
The tests in rocksdb.rs are good stress tests but missing:

• Unit tests for the conflict detection logic itself
• Tests for edge cases (same key conflicts, range overlaps, read-write vs write-write)
• Tests verifying that conflicts are actually detected (the current test just checks for success)
• Tests for transaction timeout behavior
• Tests for cleanup/TTL behavior

8. Minor: Unused Variable ℹ️
In metrics/providers.rs:75, the mut is removed from resource. This is correct but unrelated to the PR's main purpose.

Performance Considerations

Positive:

• Lazy conflict detection reduces overhead during transaction execution
• Atomic operations instead of mutexes for simple counters
• TTL-based cleanup is simple and effective

Concerns:

• The nested loop in conflict checking (transaction_conflict_tracker.rs:58-69) is O(n*m) where n = number of active transactions and m = average conflict ranges per transaction
• With high concurrency, this could become a bottleneck
• Consider using an interval tree or R-tree for efficient range overlap queries

Security Considerations

No security concerns identified. The changes are internal to the database driver and don't expose new attack surfaces.

Recommendations

High Priority:

1. Fix the conflict detection logic in transaction_conflict_tracker.rs:60-66
2. Verify that conflict ranges are being populated from operations
3. Fix or document the transaction cleanup in Postgres driver

Medium Priority:
4. Add comprehensive unit tests for conflict detection
5. Review the transaction removal logic on RocksDB commit failure
6. Consider optimization for conflict range checking at scale

Low Priority:
7. Add configuration for TTL and consider max size limits
8. Add documentation explaining the new conflict detection approach
9. Consider adding metrics for conflict detection (conflicts detected, tracker size, etc.)

Conclusion

This is a solid refactoring that moves the RocksDB conflict detection to a more appropriate architecture. However, there are critical bugs in the conflict detection logic that must be fixed before merging. The test coverage should also be expanded to verify correctness of the conflict detection itself.

The PR demonstrates good understanding of optimistic concurrency control principles, but needs careful review of the conflict detection implementation to ensure correctness.