Skip to content

insipx/db integrity 8 mobile bindings - #4040

Closed
insipx wants to merge 1 commit into
insipx/db-integrity-7-browser-sdkfrom
insipx/db-integrity-8-mobile-bindings
Closed

insipx wants to merge 1 commit into
insipx/db-integrity-7-browser-sdkfrom
insipx/db-integrity-8-mobile-bindings

Conversation

@insipx

@insipx insipx commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Stacked on #4039. uniffi mobile bindings: FfiIntegrityCheckLevel, FfiIntegrityCheckOutcome, FfiXmtpClient.db_integrity_check(level?) and free check_database_integrity(db_path, encryption_key?, level?) — both async via spawn_blocking around the sync Rust calls; key conversion mirrors create_client ("Malformed 32 byte encryption key"). 3 backend-connected tests (ok / by-path ok on encrypted DB / wrong-key unreadable). clippy -D warnings + fmt clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_01V1AXHNsnQmHW174i5rRUEb

Note

Add mobile FFI bindings for database integrity checks

  • Adds FfiIntegrityCheckLevel enum and FfiIntegrityCheckOutcome record with conversions to xmtp_db types
  • Adds check_database_integrity free function for read-only path-based checks, validating optional 32-byte encryption keys
  • Adds FfiXmtpClient.db_integrity_check method to run checks against the client's database
  • Both APIs offload work to tokio::task::spawn_blocking and return structured outcomes with standardized strings (ok, corrupt, unreadable, saltMissing, locked, failed)
  • Adds tests in integrity.rs covering the client method and the free function with correct and wrong keys

Macroscope summarized dc5b3d0.

…rityCheck

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Aug 25, 2026

Copy link
Copy Markdown

Claude finished @insipx's task —— View job


Code Review - August 25, 2026 14:14 UTC

  • Read changed files and understand implementation
  • Analyze code quality and best practices
  • Check for potential bugs and issues
  • Review performance considerations
  • Assess security concerns
  • Evaluate test coverage

Summary

This PR successfully adds mobile bindings for database integrity checking, matching the patterns established in Node and WASM bindings. The implementation is clean and well-tested. Overall: LGTM with minor observations below.

Code Quality ✅

The implementation follows established patterns:

  • Consistent with Node bindings (bindings/node/src/integrity.rs:52-73)
  • Proper use of uniffi attributes for FFI exposure
  • Good use of spawn_blocking for synchronous DB operations (matching Node approach)
  • Error handling with proper JoinError propagation from spawn_blocking

Potential Issues ⚠️

1. Inconsistent spawn_blocking error handling (bindings/mobile/src/mls.rs:831)

The free function check_database_integrity uses .await? which will surface JoinError as-is:

let result = tokio::task::spawn_blocking(move || {
    xmtp_db::prelude::check_database_integrity(&db_path, key.as_ref(), level)
})
.await?;  // JoinError exposed directly

The client method uses ?? which properly unwraps both the join and inner result:

let result = tokio::task::spawn_blocking(move || client.db_integrity_check(level)).await??;

Recommendation: Change line 831 to .await?? for consistency and to properly surface the inner ClientError. The Node bindings handle this by explicitly converting the JoinError to a domain error (line 71 in bindings/node/src/integrity.rs).

2. Missing tracing on free function (bindings/mobile/src/mls.rs:563)

The client method has #[tracing::instrument(skip_all)] but the free function check_database_integrity does not. For consistency and debuggability, consider adding the attribute.

Performance ✅

  • Proper use of spawn_blocking to avoid blocking the async runtime
  • Read-only operations won't contend with client DB connections
  • Matches the performance characteristics of Node and WASM implementations

Security ✅

  • Proper 32-byte encryption key validation with consistent error message
  • No key material logged or exposed in error paths
  • Database path validation delegated to core implementation

Test Coverage ✅

Strong test coverage in bindings/mobile/src/mls/tests/integrity.rs:

  • test_db_integrity_check_ok - Basic client method happy path
  • test_check_database_integrity_free_fn_ok - Free function with proper shutdown
  • test_check_database_integrity_free_fn_wrong_key_is_unreadable - Encryption key validation

Suggestions:

  1. Consider adding a test for the "corrupt" case (if feasible to simulate)
  2. Consider testing the level parameter (Quick vs Full) to ensure it's passed through correctly
  3. Test None vs Some(key) paths for the free function

Minor Notes

  • Line 844: Good attention to detail calling client.shutdown().await before checking the DB file externally
  • The FfiIntegrityCheckLevel enum properly defaults via unwrap_or_default() which is idiomatic
  • Documentation comments are clear and consistent with other bindings

@codecov

codecov Bot commented Aug 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 78.57143% with 9 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (insipx/db-integrity-7-browser-sdk@17532ba). Learn more about missing BASE report.

Files with missing lines Patch % Lines
bindings/mobile/src/mls.rs 78.57% 9 Missing ⚠️
Additional details and impacted files
@@                         Coverage Diff                          @@
##             insipx/db-integrity-7-browser-sdk    #4040   +/-   ##
====================================================================
  Coverage                                     ?   85.94%           
====================================================================
  Files                                        ?      420           
  Lines                                        ?    68885           
  Branches                                     ?        0           
====================================================================
  Hits                                         ?    59201           
  Misses                                       ?     9684           
  Partials                                     ?        0           

☔ View full report in Codecov by Harness.
📢 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.

@insipx
insipx deleted the branch insipx/db-integrity-7-browser-sdk August 25, 2026 14:29
@insipx insipx closed this Aug 25, 2026
@insipx
insipx deleted the insipx/db-integrity-8-mobile-bindings branch August 25, 2026 14:29
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