Skip to content

cutover queries#3119

Open
insipx wants to merge 1 commit intopush-rmpzuuomqxrsfrom
push-zwqsrxnyywwx
Open

cutover queries#3119
insipx wants to merge 1 commit intopush-rmpzuuomqxrsfrom
push-zwqsrxnyywwx

Conversation

@insipx
Copy link
Contributor

@insipx insipx commented Feb 4, 2026

cutover table & queries #3029

@insipx insipx requested a review from a team as a code owner February 4, 2026 21:10
@insipx
Copy link
Contributor Author

insipx commented Feb 4, 2026

This PR is part of a stack of 4 bookmarks:

  1. trunk()
  2. push-qqtpmktputny
  3. push-rmpzuuomqxrs
  4. push-zwqsrxnyywwx ← this PR
  5. push-tlyurqorvnrq

Created with jj-stack

@insipx insipx mentioned this pull request Feb 4, 2026
@claude
Copy link

claude bot commented Feb 4, 2026

Claude finished @insipx's task —— View job


PR Review Complete

Review Date: February 4, 2026, 21:19 UTC

Summary

This PR introduces infrastructure for managing decentralized migration cutover state via a new d14n_migration_cutover table and associated query trait. The implementation is solid overall with good adherence to the project's patterns. Below are key findings:


✅ Strengths

  1. Excellent test coverage - All four mutation methods have dedicated tests (lines 101-145 in d14n_migration_cutover.rs)
  2. Follows project patterns - Correctly implements trait-based query interface, Diesel integration, and trait blanket implementations
  3. Proper singleton enforcement - SQL constraint CHECK (id = 1) ensures single row
  4. Good defaults - i64::MAX for cutover_ns and 0 for last_checked_ns are sensible

🔍 Issues & Recommendations

1. Missing Test Coverage (20 lines uncovered)

The Codecov report shows 76.47% patch coverage with 20 lines missing. Looking at the implementation, the likely uncovered lines are:

  • Error path handling in setter methods (lines 65-90)
  • The reference implementation for &T (lines 40-56)

Recommendation: Add error scenario tests:

#[xmtp_common::test]
fn test_concurrent_updates() {
    with_connection(|conn| {
        // Test concurrent writes don't cause corruption
    })
}
2. Race Condition Potential

The set_* methods use find(1) which assumes the row exists. If called before database initialization (or if row deleted), the update would silently succeed with 0 rows affected (line 69, 77, 87).

Recommendation: Either:

  • Add a check that execute() returns 1 row affected
  • Document that these methods assume initialization has occurred
  • Make get_migration_cutover() ensure row exists on first call
3. Silent Failure on Missing Row

get_migration_cutover() returns Default if row doesn't exist (line 62). While convenient, this masks potential database corruption issues.

Recommendation: Consider logging a warning when falling back to default:

let result = self.raw_query_read(|conn| dsl::d14n_migration_cutover.first(conn).optional())?;
match result {
    Some(r) => Ok(r),
    None => {
        tracing::warn!("d14n_migration_cutover row missing, using defaults");
        Ok(StoredMigrationCutover::default())
    }
}
4. Dropped Execute Results

Lines 69, 77, 87: The execute() results are not checked. If the row doesn't exist, updates silently do nothing.

Recommendation: Verify row count:

let rows_affected = diesel::update(dsl::d14n_migration_cutover.find(1))
    .set(d14n_migration_cutover::cutover_ns.eq(cutover_ns))
    .execute(conn)?;
if rows_affected != 1 {
    return Err(diesel::result::Error::NotFound);
}
5. Missing Documentation

The module and trait lack documentation explaining:

  • Purpose of the cutover mechanism
  • What cutover_ns and last_checked_ns represent
  • Expected usage patterns
  • Why it's a singleton

Recommendation: Add module-level docs:

//! Manages decentralized (d14n) migration cutover state.
//!
//! This module tracks when the system should switch from legacy to d14n infrastructure.
//! It maintains a singleton row with timestamp thresholds for migration decisions.
6. Type Safety Opportunity

Using raw i64 for timestamps makes it easy to mix up cutover_ns and last_checked_ns.

Recommendation (optional, lower priority): Consider newtype wrappers:

pub struct CutoverTimestamp(i64);
pub struct LastCheckedTimestamp(i64);

📊 Architecture Assessment

  • ✅ Proper integration with DbQuery trait (traits.rs:94, 126)
  • ✅ Exported in prelude (lib.rs:39)
  • ✅ Mock implementation provided (mock.rs:828-836)
  • ✅ Migration up/down scripts are correct
  • ✅ Schema correctly generated

🎯 Priority Recommendations

  1. HIGH: Add test coverage for error paths to reach >90% coverage
  2. MEDIUM: Verify execute() returns 1 row or log/error appropriately
  3. MEDIUM: Add module and trait documentation
  4. LOW: Add warning log in get_migration_cutover() when row missing

@macroscopeapp
Copy link
Contributor

macroscopeapp bot commented Feb 4, 2026

Add d14n cutover queries by creating d14n_migration_cutover table with a singleton row (id=1) and exposing QueryMigrationCutover on DbConnection and DbQuery

Introduce a new Diesel-backed table and model for d14n_migration_cutover, add up/down migrations, implement QueryMigrationCutover with setters for cutover_ns, last_checked_ns, and has_migrated, export the trait in the crate prelude, and extend DbQuery to include it.

📍Where to Start

Start with the QueryMigrationCutover trait and its DbConnection<C> impl in d14n_migration_cutover.rs.


Macroscope summarized d6c3c30.

@codecov
Copy link

codecov bot commented Feb 4, 2026

Codecov Report

❌ Patch coverage is 76.47059% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.36%. Comparing base (26d963d) to head (d6c3c30).

Files with missing lines Patch % Lines
...p_db/src/encrypted_store/d14n_migration_cutover.rs 76.19% 20 Missing ⚠️
Additional details and impacted files
@@                  Coverage Diff                  @@
##           push-rmpzuuomqxrs    #3119      +/-   ##
=====================================================
+ Coverage              74.21%   74.36%   +0.14%     
=====================================================
  Files                    449      450       +1     
  Lines                  55741    55826      +85     
=====================================================
+ Hits                   41367    41513     +146     
+ Misses                 14374    14313      -61     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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