fix(xmtp_mls): forward db-disconnect signal through SyncSummary and sibling errors#3716
fix(xmtp_mls): forward db-disconnect signal through SyncSummary and sibling errors#3716insipx wants to merge 1 commit into
Conversation
|
Claude finished @insipx's task —— View job Code Review Completed ✓Review Date: June 10, 2026 SummaryThis PR correctly fixes a critical bug where database disconnects were being permanently buried when wrapped in Strengths
Minor Observations (Not Blocking)
Code Quality
VerdictApprove — This is a well-crafted fix for a critical runtime bug. The implementation is correct, thorough, and well-tested. |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. You can customize Macroscope's approvability policy. Learn more. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3716 +/- ##
==========================================
+ Coverage 84.45% 84.49% +0.03%
==========================================
Files 408 408
Lines 59772 59878 +106
==========================================
+ Hits 50483 50594 +111
+ Misses 9289 9284 -5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ibling error variants A dropped DB pool (PoolNeedsConnection) surfacing during a sync was misclassified as not-a-disconnect: SyncSummary had no NeedsDbReconnect impl, so any disconnect landing in its publish/post-commit/other/per-message errors got wrapped into GroupError::Sync / SyncFailedToWait / DeviceSyncError::Sync / CommitLogError::SyncError and hit `_ => false`. The worker supervisor never dropped the pool to reconnect — it retried forever on a dead pool (GroupError::SyncFailedToWait is also is_retryable, a true hot-loop). Fix — forward the signal everywhere it was dropped: - SqlKeyStoreError gains db_needs_connection() (mirrors StorageError / ConnectionError) so the OpenMLS key-store disconnect check lives in one place. - SyncSummary: NeedsDbReconnect scans all four error sources, including the per-message process.errored failures that is_retryable deliberately skips. - GroupMessageProcessingError: NeedsDbReconnect forwards the directly-wrapped DB variants and the OpenMLS state-I/O wrappers (process/merge), which carry a drop as StorageError(SqlKeyStoreError::Connection(_)). - GroupError: Sync / SyncFailedToWait / SqlKeyStore forwarded. - TaskWorkerError: Group / LoadGroup / DeviceSync forward the full classification instead of structurally matching only ::Storage. - CommitLogError: SyncError / KeystoreError forwarded. - DeviceSyncError: Sync forwarded. Every new arm bottoms out at db_needs_connection(), which matches only PoolNeedsConnection / Pool(_) — benign connection errors (e.g. DisconnectInTransaction) stay false, pinned by negative test cases. Rebased on main (post #3744 TaskRunner rework; tasks.rs merged with the new LoadGroup variant). Adds 3 disconnect-propagation tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
insipx
force-pushed
the
insipx/disconnect-classification
branch
from
fae72a0 to
0f3d67a
Compare
macroscopeapp
Bot
dismissed
their stale review
Dismissing prior approval to re-evaluate 0f3d67a
| impl crate::worker::NeedsDbReconnect for GroupMessageProcessingError { | ||
| /// Forwards a dropped-pool signal from storage-bearing variants so a | ||
| /// per-message disconnect (landing in `SyncSummary::process.errored`) isn't lost. | ||
| fn needs_db_reconnect(&self) -> bool { | ||
| use super::app_data::ProcessMessageWithAppDataError; | ||
| match self { | ||
| Self::Storage(s) => s.db_needs_connection(), | ||
| Self::Db(c) => c.db_needs_connection(), | ||
| Self::Identity(i) => i.needs_db_reconnect(), | ||
| Self::Client(c) => c.db_needs_connection(), | ||
| // OpenMLS state-I/O carries a pool drop as | ||
| // `StorageError(SqlKeyStoreError::Connection(_))`; forward it. | ||
| Self::ClearPendingCommit(e) => e.db_needs_connection(), | ||
| Self::OpenMlsProcessMessage(ProcessMessageError::StorageError(e)) => { | ||
| e.db_needs_connection() | ||
| } | ||
| Self::OpenMlsProcessMessageWithAppData(ProcessMessageWithAppDataError::OpenMls( | ||
| ProcessMessageError::StorageError(e), | ||
| )) => e.db_needs_connection(), | ||
| Self::MergeStagedCommit(openmls::group::MergeCommitError::StorageError(e)) => { | ||
| e.db_needs_connection() | ||
| } | ||
| _ => false, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Medium groups/mls_sync.rs:276
GroupMessageProcessingError::needs_db_reconnect() returns false for Self::Intent(i), hiding database disconnects that surface through IntentError::Storage(StorageError::Connection(PoolNeedsConnection)). The worker will retry on a dead pool because the disconnect signal is lost. Consider matching on Self::Intent and delegating to IntentError::needs_db_reconnect() if available, or add a variant check for the Storage wrapper.
Self::MergeStagedCommit(openmls::group::MergeCommitError::StorageError(e)) => {
e.db_needs_connection()
}
+ Self::Intent(i) => i.needs_db_reconnect(),
_ => false,🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/xmtp_mls/src/groups/mls_sync.rs around lines 276-301:
`GroupMessageProcessingError::needs_db_reconnect()` returns `false` for `Self::Intent(i)`, hiding database disconnects that surface through `IntentError::Storage(StorageError::Connection(PoolNeedsConnection))`. The worker will retry on a dead pool because the disconnect signal is lost. Consider matching on `Self::Intent` and delegating to `IntentError::needs_db_reconnect()` if available, or add a variant check for the `Storage` wrapper.
1 participant
Summary
A dropped DB pool (
PoolNeedsConnection) that surfaced during a sync was being misclassified as not-a-disconnect, so the worker supervisor never dropped the pool to reconnect — it retried forever on a dead pool. The worst path (GroupError::SyncFailedToWait) is alsois_retryable == true, making it a true hot-loop.Root cause:
SyncSummary— the aggregate sync error — had noNeedsDbReconnectimpl at all. AnyPoolNeedsConnectionlanding in itspublish_errors/post_commit_errors/other/ per-messageprocess.erroredgot wrapped intoGroupError::Sync/SyncFailedToWait/DeviceSyncError::Sync/CommitLogError::SyncError, all of which hit_ => falseinneeds_db_reconnect. The disconnect signal was permanently buried.This is the same bug class the existing
disconnect_propagation_testswere written to prevent — the SyncSummary path just wasn't covered.Fix — forward the disconnect signal everywhere it was being dropped
SqlKeyStoreError::db_needs_connection()(new, inxmtp_db) — mirrorsStorageError/ConnectionError, so the OpenMLS key-store disconnect check lives in one place instead of three nested-variant matches.SyncSummary: NeedsDbReconnect(new) — scans all four error sources, including the per-messageprocess.erroredfailures (whichis_retryabledeliberately skips, but which can still carry a pool drop).GroupMessageProcessingError: NeedsDbReconnect(new) — forwards the directly-wrapped DB variants (Storage/Db/Identity/Client/ClearPendingCommit) and the OpenMLS state-I/O wrappers (OpenMlsProcessMessage,OpenMlsProcessMessageWithAppData,MergeStagedCommit), which carry a drop asStorageError(SqlKeyStoreError::Connection(_))— the most likely per-message disconnect carriers.GroupError::needs_db_reconnect—Sync/SyncFailedToWaitnow forward toSyncSummary;SqlKeyStoreforwarded.TaskWorkerError::needs_db_reconnect—Group/LoadGroup/DeviceSyncforward the full classification instead of structurally matching only the innerStoragevariant.CommitLogError—SyncError/KeystoreErrorforwarded.DeviceSyncError—Syncforwarded.Safety: no false positives
Every new arm bottoms out at
db_needs_connection(), which matches onlyPoolNeedsConnection(and the pre-existingPool(_)). A non-pool-drop connection error likeDisconnectInTransactionstays classifiedfalse, so a healthy pool is never torn down needlessly. Each new test includes a benign-error negative case to pin this.Rebase
Rebased onto latest
main(post-#3744 TaskRunner rework);TaskWorkerErrormerge keeps the newLoadGroupvariant forwarding.Test Plan
cargo check -p xmtp_mls --tests— compiles cleanjust lint-rust— clippy / fmt / hakari cleancargo nextest run -p xmtp_mls disconnect_propagation_tests— 10/10 pass (3 new:sync_summary_forwards_disconnect,group_message_processing_error_forwards_openmls_disconnect,task_worker_error_forwards_disconnect)cargo nextest run -p xmtp_mls -E 'test(worker)'— 33/33 pass (no regressions)🤖 Generated with Claude Code
Note
Forward db-disconnect signals through
SyncSummaryand sibling error types inxmtp_mlsSeveral
NeedsDbReconnecttrait implementations previously dropped pool-disconnect signals when errors were wrapped inSyncSummary,SyncError,KeystoreError, or OpenMLS storage error paths, causing reconnect logic to silently miss these conditions.SyncSummarygets a newNeedsDbReconnectimpl that checks all contained error collections (publish_errors,post_commit_errors,other,process.errored).GroupError,CommitLogError, andDeviceSyncErrornow delegate to innerSyncSummary::needs_db_reconnect()instead of returningfalse.GroupMessageProcessingErrorgets a newNeedsDbReconnectimpl covering OpenMLSStorageErrorandMergeCommitErrorpaths.TaskWorkerErrornow fully delegatesDeviceSync(e)toe.needs_db_reconnect()rather than only forwarding theStoragesub-variant.📊 Macroscope summarized 0f3d67a. 8 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.