Repository navigation
Add legacy database migration to the normal SDK packages - #4455
Conversation
|
Macroscope skipped reviewing this pull request. Per-review cost limit exceeded (workspace setting). This review would cost an estimated $15.09, which exceeds your per-review limit of $10.00. The top 3 files driving up this estimate:
Tip To get this pull request reviewed, you can:
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## self-hosted #4455 +/- ##
===============================================
+ Coverage 87.87% 87.94% +0.07%
===============================================
Files 543 549 +6
Lines 102946 104073 +1127
===============================================
+ Hits 90461 91526 +1065
- Misses 12485 12547 +62 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| mls::message_contents::{GroupMetadataV1, GroupMutableMetadataV1}, | ||
| }; | ||
|
|
||
| const MUTABLE_METADATA_EXTENSION: u16 = 0xff00; |
There was a problem hiding this comment.
Major: These numeric metadata IDs duplicate the canonical definitions in xmtp_configuration::MUTABLE_METADATA_EXTENSION_ID and xmtp_mls_common::app_data::component_id::ComponentId::{COMPONENT_REGISTRY, CREATOR_INBOX_ID}. If the shared definitions change, this decoder can silently stop recognizing legacy metadata and omit it from migration archives. Please reference those existing constants instead of copying their wire values.
Suggested fix: No diff — use the shared constants and add the required crate dependency/imports.
Posted via Macroscope — Code Quality
There was a problem hiding this comment.
🤖 These values describe the frozen legacy storage format. They must remain tied to the supported source releases even if current protocol constants change. Both pinned legacy sources define mutable metadata as 0xff00; the pinned AppData source defines COMPONENT_REGISTRY as 0x8000 and CREATOR_INBOX_ID as 0xbffe. The current definitions match these values, but importing future current constants would make historical decoding depend on a later protocol.
The source-pin comment and checked-in legacy fixtures document and verify the values. This was validated before the requested rebase at ab0d2bf; the decoder values did not change. MIG-001 requires support for the published legacy range. No dependency or wire-value change is needed. Leaving the thread open with this disposition.
a88b981 to
a05364d
Compare
|
🤖 Response to Spec check 112959025458 at 3c094a5: this report repeats a settled migration policy. The migration owner approved excluding every stored non-NULL per-message deadline and retaining otherwise eligible NULL-expiry history, including unknown legacy expiry. MIG-006, the guide, and the approved plan record this rule. Macroscope already acknowledged the accepted policy and resolved the prior thread. Current group settings do not establish each historical row's deadline. On this head, stable_history_metadata_and_exclusions, earlier_schema_exports_history_without_recorded_deadlines, and legacy_migration_timestamps_do_not_remove_null_expiry_history pass in the restored 35-test suite. The reported NULL-expiry fixture is retained by that accepted policy. The source and output preservation proofs also pass. Please assess the Spec result against the accepted migration policy. No new owner decision is pending, and no check conclusion has been overridden. |
8b5c7c8 to
6811d27
Compare
6811d27 to
61b6bd7
Compare
6835a48 to
51c05a6
Compare
| .await | ||
| .map_err(output)?; | ||
| pool.unpause_vfs().await.map_err(output)?; | ||
| pool.clear_all().await.map_err(output)?; |
There was a problem hiding this comment.
Major: If clear_all() fails after unpause_vfs() succeeds, the OpfsWorkingCopy cleanup guard has not yet been constructed. The function returns with the private VFS still unpaused and any partial files left behind, which can contaminate or block a retry. Construct the RAII owner immediately after installation and ensure setup failures also pause/clean the VFS; add a fault-injection test for this initialization path.
Suggested fix: No diff — the guard lifetime and initialization-failure regression need coordinated changes.
Posted via Macroscope — Code Quality
There was a problem hiding this comment.
🤖 Fixed in 0072850. The private working-copy owner is constructed immediately after installation, before unpause and clear_all. Cleanup also attempts pause after a deletion failure, and source cleanup runs on every ordinary return. Real handle controls fail the old package (private7opens/6closes; source6opens/0closes; delete fault12opens/6closes). The repaired package closes all handles before the reply and retries successfully. Full Chromium migration and all43native tests pass. Current-head CI and independent scoped closure reviews are pending, so this thread stays open.
ApprovabilityVerdict: Skipped Macroscope did not run approvability analysis for this PR. This PR modifies 4 files matched by one or more of your Not approved because:
Review your spending limits in Billing settings, or comment |
|
🤖 Current-head Security check 115005150023 repeats the settled migration expiry policy. The owner explicitly accepted retaining otherwise eligible NULL-expiry history, including legacy imports whose deadlines were lost, to preserve available history. This is not inferred acceptance from an ARCH gap waiver. MIG-legacy-database-migration.md records the owner's 2026-10-06 decision and MIG-006 requires exclusion for every stored non-NULL deadline while NULL alone does not cause exclusion. The approved plan P4 records the same decision: https://plan.ref.tools/1hYl5TruynqXmFkp. The earlier current-policy explanation is #4455 (comment). Commit 0072850 does not change expiry selection or either requirement. Its records.rs diff adds required deletion/message identity validation only; ELIGIBLE_MESSAGES and the stored-expiry rule are unchanged from parent 06c38a8. Every stored deadline, past or future, remains excluded. Unknown NULL-expiry remains the accepted limitation. Current 43 native tests and full Chromium migration tests pass this selected policy. The Security check therefore records a product-policy disagreement rather than a new expiry implementation defect in this repair. Please account for the explicit owner decision when assessing this check. The implementation keeps that decision and does not claim unknown-expiry history was always non-disappearing. |
|
8.0.0 capability tracking: #4691 — Offline/history access and migration. This merged PR is included in the tracking issue's implementation checklist. |
Legacy SDK databases use a schema that the self-hosted SDK cannot open. This adds an offline converter to the normal SDK packages. It applies frozen legacy migrations to a temporary copy, then prepares a standard encrypted archive for the existing importer.
Validation on repair source committed as 0072850:
Runtime evidence uses Chromium and host-native targets. No Windows runtime or physical-device execution is claimed.
Approved plan.
Note
Add
xmtp_legacy_migrationcrate and exposeprepareMigrationArchivein all SDKsxmtp_legacy_migrationcrate that converts a closed legacy SQLite database into an encrypted archive. It applies the full pinned legacy migration history in migrations.rs and exports groups, messages, and consent records through a typed error taxonomy (InvalidInput,SourceBusy,UnsupportedSchema,Migration,RecordRead,Output). OnlySourceBusyis retryable.prepareMigrationArchiveandreadMigrationArchive(WASM-only) through UniFFI in the Swift, Kotlin, Node, and Browser SDKs, with migration error variants added toXmtpErrorand keys redacted from debug output.ElementWriterinxmtp_archivethat returns an error instead of truncating oversized frame lengths.prepareMigrationArchiveruns in an exclusive worker session — concurrent clients or storage calls are rejected withStorageBusy. Browser migration rejects database keys because browser legacy storage is unencrypted.Macroscope summarized 0072850.