Database Tools

[codabrink]

Creates a tool to allow database management of xmtp instances.

[graphite-app]

How to use the Graphite Merge Queue

Add the label mergequeue 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.

[claude]

Claude finished @codabrink's task —— View job

---

PR Review - Database Tools

Review completed: December 8, 2025 at 21:58 UTC

---

Architecture & Design

Positive:

• Clean separation of concerns with task-based modules
• Good use of Rust's type system with generic Manager<Db> and DbBencher<Db>
• Transaction-based benchmarking with rollback is clever and safe (db_bench.rs:91-115)

Issues:

1. Missing binary target in Cargo.toml (db_tools/Cargo.toml)

   • The package has a main.rs but no [[bin]] section. While Rust will infer this, it's better to be explicit for a CLI tool.

2. Workspace inconsistency (Cargo.toml:29-47)

   • db_tools is in members but not default-members, which means cargo test from workspace root won't include it. This could lead to broken tests going unnoticed.

---

Security & Input Validation

3. Unsafe key conversion (main.rs:48)

   • key_bytes.try_into().unwrap() will panic if not exactly 32 bytes, despite the check on line 45-47. However, the validation makes this safe in practice.

4. Group ID trimming (main.rs:112)

   • Good addition of .filter(|id| !id.trim().is_empty()) to handle empty segments from commas.

---

Performance

5. Benchmark measurement key collision (db_bench.rs:22)

   • The bench! macro uses stringify!($fn($($args),*)) as key. If the same function is called multiple times with different runtime args, they'll share the same key and overwrite measurements. This is already flagged by macroscopeapp.

6. Division by zero guard missing (db_bench.rs:158-161)

   • When measurements.len() == 0, the average calculation will panic. Consider early return or N/A message.

---

Error Handling

7. Silenced benchmark errors (db_bench.rs:119-123)

   • Errors are logged but execution continues, making it hard to know if benchmarks failed. Consider failing fast or at least returning error summary.

8. Migration error handling (migrations.rs:30-34)

   • Logs errors but continues loop, which could lead to partial rollback state. The continue-on-error behavior should be documented or changed.

---

Test Coverage

Good coverage for critical paths:

• ✅ Migration rollback/reapply (migrations.rs:100-120)
• ✅ Message clearing (clear_messages.rs:53-93)
• ✅ Group enable/disable (group_management.rs:47-61)
• ✅ Benchmark execution (db_bench.rs:499-510)

Missing coverage:

• ❌ CLI argument parsing and validation
• ❌ Encryption key handling from environment
• ❌ db_vacuum task (snapshot_to_persistent.rs has no tests)
• ❌ Error cases (invalid hex, missing target, etc.)

Codecov shows 0.82% patch coverage with 240 lines missing. This is extremely low and concerning for production tooling that performs destructive operations.

---

Code Quality

9. Unused confirm_destructive function (main.rs:120-134)

   • This function exists but is never called from main(). Tasks call their own confirmation. Either use it consistently or remove it.

10. Inconsistent error messages

    • Some errors use bail!(), others panic!(), creating inconsistent UX.

11. CLI arg naming (main.rs:170)

    • .target() method name doesn't clearly indicate it can panic. Consider require_target() or return Result.

---

Suggestions (Not Critical)

• Consider adding --dry-run flag for destructive operations
• Add progress indicators for long-running benchmarks
• Consider structured output (JSON) for benchmark results to enable scripting/automation
• Document the difference between DbRollback (reverts multiple) vs DbRevertMigration (reverts one)

---

Summary

This is a useful internal tool with solid architecture. The main concerns are:

1. Test coverage is critically low (0.82%) for destructive database operations
2. Workspace configuration excludes this from default test runs
3. Error handling in benchmarks and migrations could be clearer

The existing bot reviews have caught most of the detailed implementation issues. Focus on improving test coverage and ensuring this runs in CI.

---

[codecov]

Codecov Report

❌ Patch coverage is 0.82645% with 240 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.28%. Comparing base (fd468b5) to head (6d038ff).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
manager/src/tasks/query_bench.rs	0.00%	164 Missing ⚠️
manager/src/main.rs	0.00%	40 Missing ⚠️
manager/src/tasks/revert_migrations.rs	0.00%	28 Missing ⚠️
manager/src/tasks/snapshot_to_persistent.rs	0.00%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2806      +/-   ##
==========================================
- Coverage   74.36%   74.28%   -0.09%     
==========================================
  Files         376      388      +12     
  Lines       48819    49834    +1015     
==========================================
+ Hits        36305    37019     +714     
- Misses      12514    12815     +301     

☔ 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.

[macroscopeapp]

Add a db_tools CLI to run XMTP database maintenance and benchmarks and trigger workspace CI for db_tools/** changes

Introduce a new binary crate xmtp-db-tools that provides CLI tasks for migrations, message clearing with optional retention days, group enable/disable, DB vacuum to file, and a read/write-safe benchmark suite that runs 10 iterations per measurement. Wire the crate into the workspace and CI path filters, re-export xmtp_proto from xmtp_db, and add explicit down migrations across recent schema changes.

📍Where to Start

Start with the CLI entrypoint and task dispatch in main.rs, then review task implementations in mod.rs and the specific tasks such as db_bench.rs and migrations.rs.

Changes since #2806 opened

• Replaced find_groups_by_id_paged calls with find_groups calls in DbBencher::new constructor [a151a2f]
• Reordered conversation setup and import statements in test_bench_works test [a151a2f]
• Modified the test_run_and_revert_specific_migration test in the db_tools migrations module to use a persistent database and rollback to the 2025-10-07-180046_create_tasks migration [fb38d9f]
• Modified test initialization in test_rollback_and_run_pending_migrations to use persistent database configuration [77de2b7]

---

📊 Macroscope summarized 77de2b7. 9 files reviewed, 11 issues evaluated, 10 issues filtered, 1 comment posted. View details

[macroscopeapp]

The rollback loop can spin forever on a revert failure. We log the error and continue, so applied_migrations(conn)?.first() keeps returning the same version and we retry endlessly. Consider breaking (or returning the error) when revert_last_migration fails to avoid an infinite loop.

-            if let Err(err) = result {
-                tracing::warn!("{err:?}");
-            } else {
+            if let Err(err) = result {
+                tracing::warn!("{err:?}");
+                break;
+            } else {
                 info!("Reverted {version}");
-            }
+            }

🚀 Reply to ask Macroscope to explain or update this suggestion.

👍 Helpful? React to give us feedback.