Reinstall continuity: auto-revoke dead installation + restore conversation consent#1151
Conversation
The iOS keychain outlives the app, so a reinstall resumes the identity but must mint a new XMTP installation - the previous one's keys died with the deleted app container, yet it stays registered on the inbox as a ghost "Device <hex>" row that reappears after every reinstall cycle (revoking it only orphans the next one). A device-local keychain marker now records which installation this device last ran. On session ready, any marker installation that isn't the current one is provably this device's own dead installation - not a paired device - so it is revoked automatically. Failed revokes stay in the marker and retry next launch; an inbox change (pairing adoption, delete-all) resets the marker without revoking. Skipped in the notification service extension. Verified on-sim: seed launch wrote the marker, an uninstall/reinstall cycle logged stale_own_installations_revoked count=1 and left only the live installation on the inbox. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @yewreeka's task —— View job Code Review: Reinstall ContinuitySummaryThis PR successfully addresses two critical reinstall bugs through well-architected solutions. The implementation is solid, the test coverage is comprehensive, and all Macroscope blocking issues have been properly resolved. The code quality is high with clear separation of concerns and thorough edge case handling. ✅ Recommendation: Approve with minor suggestions Verified FixesAll four Macroscope blocking issues have been correctly addressed:
The fix for the mirror race (ConsentBackupMirror.swift:138) is also confirmed - the Code Quality HighlightsArchitecture
Implementation Details
Test CoverageExcellent coverage across both pure logic and integration scenarios: StaleInstallationReconcilerTests (6 tests):
ConsentBackupTests (7 tests):
The QA test 45 provides end-to-end validation with real on-simulator reinstall cycles. Minor Suggestions (Non-blocking)1. Cancellation Check PlacementFile: SessionStateMachine.swift:731-732 The let liveIds = try await client.listInstallations(refreshFromNetwork: true).map(\.id)
try Task.checkCancellation() // Add this
let idsToRevoke = plan.candidateStaleIds.filter(Set(liveIds).contains)This would allow quicker teardown when stop/delete is called during the network fetch. 2. ConsentBackupMirror Carry Logic DocumentationFile: ConsentBackupMirror.swift:140-148 The carry logic is subtle (especially // Carried IDs = backup IDs that haven't appeared in DB yet and aren't currently allowed.
// These are either: (a) conversations still refilling, or (b) denied from another device.
// The window expiry flush eventually drops (b) while (a) surface naturally.
let pending = mirrorState.carryWindowExpired
? []
: (mirrorState.carriedBackupIds ?? []).subtracting(mirrorState.everObservedIds)
return (pending.subtracting(allowedConversationIds), mirrorState.lastSaveFailed)3. Test DocumentationFile: ConsentBackupTests.swift:83-134 The reinstall refill test ( @Test("Mirror carries unseen backup ids through a reinstall refill, never shrinking the backup")
func mirrorCarriesUnseenBackupIdsThroughRefill() async throws {
// Timeline:
// 1. Previous install wrote backup with [a, b]
// 2. Reinstall: DB starts empty
// 3. Initial emission: [] (must not shrink backup)
// 4. Partial refill: [a] (must not shrink backup - b still pending)
// 5. New conversation: [a, new] (must carry b)
// 6. Full refill: [a, b] observed
// 7. Deny b: [a] (now safe to shrink - b was observed)
...
}4. QA Test DocumentationFile: qa/tests/45-reinstall-message-continuity.md Excellent test! Consider adding a "Known Limitations" section noting:
Security Considerations✅ No security issues found
Performance Considerations✅ Performance looks good
Edge Cases HandledThe implementation correctly handles:
Test ResultsAccording to the PR description:
Review of Author's ClaimsThe author states "the exact fix it prescribes is already at HEAD" for the ConsentBackupMirror race. Verified: correct. The cancellation check at ConsentBackupMirror.swift:138 is indeed present and properly placed after the All four Macroscope blocking issues have been addressed as claimed in commits:
Final VerdictThis is excellent work. The solution is architecturally sound, thoroughly tested, and all blocking issues have been resolved. The code demonstrates strong understanding of Swift concurrency, keychain lifecycles, and the XMTP installation model. The minor suggestions above are all non-blocking polish items that could be addressed in a follow-up if desired. ✅ Approve |
ApprovabilityVerdict: Needs human review This PR introduces a new feature (reinstall continuity with auto-revocation and consent restoration) that integrates into the auth flow and adds significant new runtime behavior. The complexity of the consent backup mirror's carry-window logic and the security-sensitive nature of installation revocation warrant human review. You can customize Macroscope's approvability policy. Learn more. |
Review finding: deferring the marker write to the ready-state reconcile meant a launch killed between minting the installation and reaching .ready never recorded it, and a later reinstall could not prove the orphan was this device's own. The marker is now written inline on the authorize and register paths as soon as the client exists; the revocation half of the reconcile stays deferred to .ready. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Delete + reinstall while actively chatting with another member: the same inbox must resume from the keychain, the previous installation must be auto-revoked, and messages must flow both directions with the CLI in every prior conversation. Runs on an erased disposable clone; first run (e7ee334257db) validated the mechanics and found a major bug: recovered conversations are stored with consent=unknown and never surface in the UI, so incoming messages are stored but invisible. Also folds the runner's findings into the docs: RULES.md home-shell tabs (Convos/Things/Contacts, agent-marketing empty state) and the join-request watch-ordering and auto-capitalization notes in the YAML. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QA test 45 found that a reinstall left every recovered conversation invisible: consent records died with the wiped app container, the re-welcomed conversations landed as consent=unknown, and the list filters those with no requests surface - messages were received and stored but the user saw the first-run empty state forever. The network cannot restore this: libxmtp propagates consent records as messages inside the device-sync MLS group, which a new installation cannot decrypt retroactively, and the only installation that could re-send them died with the app container. Instead, the allowed conversation-id set is continuously mirrored into the device-local keychain (ConsentBackupMirror, a session-scoped GRDB observer owned by SyncingManager), where it survives app deletion alongside the identity. The stale-installation reconcile - the reliable reinstall signal - restores it through the preferences layer, so consent records cover welcomes that have not arrived yet, then flips any already-stored unknown rows. Denied rows are never resurrected and a replaced identity's backup is ignored. Verified end-to-end by re-running QA test 45 (run c4ba3927cf16): all six criteria pass, consent_backup_restored count=3 on the reinstall launch, and both conversations resurfaced as allowed with bidirectional delivery through the UI. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review findings on the consent-restore commit, all addressed: - A consent-restore failure now aborts the reconcile before the revoke instead of being swallowed: letting the revoke clear the marker after a failed restore ended the retry chain and left conversations hidden permanently. - The mirror no longer writes a fresh database's initial empty snapshot over a non-empty backup for the same inbox - on a reinstall launch the empty first emission raced the reconcile's restore and could clobber the backup it was about to read. Genuine transitions to empty (the user deleted everything) still mirror once the session has seen a non-empty set. - The reconcile task is tracked and cancelled on stop and delete, so a teardown racing it can't issue revocations or rewrite keychain state for a session being removed. - KeychainIdentityStore.delete() sweeps the installation marker and consent backup alongside the identity, so "delete all data" no longer leaves state a later sign-in could restore against the user's intent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review recommendation: note that unbounded growth needs a full reinstall cycle per entry with every revoke failing, and one online launch drains the whole list. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QA re-run 905e8c3bd5d5 caught a gap in the empty-snapshot guard: during the post-restore refill the database emits partial allowed sets, and mirroring one shrank the 3-entry backup to 1 for about a minute - a second uninstall inside that window would lose the missing conversations. The mirror now carries ids from the session's initial backup until the database has actually shown them once: partial refill snapshots can only grow the backup, an id observed and later gone was denied by the user and drops out, and deleting everything still mirrors an empty set once everything has been seen. The previous first-empty-emission special case falls out of the same rule. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review finding: cancellation is cooperative and the observe loop only checks between emissions, so a mirror task cancelled while suspended on the backup load could still write - and if start() had already launched a replacement observer, the stale snapshot could land after its successor's. Re-check cancellation after the load, before the write. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Re the review's blocking item (ConsentBackupMirror race): the exact fix it prescribes is already at HEAD — commit 13449e4 added |
The job's 15-minute timeout only fits a warm package cache; on a cache miss the cold dependency build alone exceeds it and the job dies mid-compile without running a single test - it failed this way three times across PR #1151's pushes and passed only when the cache hit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Seven confirmed defects from an eight-angle adversarial review: - Restoring consent could resurrect conversations denied from a paired device while this one was uninstalled: restored records carry fresh timestamps and win last-writer-wins against the older deny. The restore now only uses the backup when this device is the inbox's sole surviving installation (where "denied elsewhere" is impossible); with live peers it requests preference sync instead, which replays consent with original timestamps. - The mirror's carry rule never expired, so an id denied elsewhere (or whose conversation died network-side) was carried in the backup forever and re-allowed on every future reinstall. The carry is now bounded by a settling window with an expiry flush that drops ids the session never observed. - The notification service extension can mint an installation when it authorizes against a wiped database, and marker seeding was gated behind the app-only syncingManager check - producing a permanently unrevokable ghost if the user uninstalled again before opening the app. Seeding (one keychain write, no network) now runs in every session mode; only revocation stays app-only. - Two teardown races could re-create wiped keychain state after Delete All Data (a queued mirror save; the reconcile's marker save behind a revoke call that ignores Swift cancellation). The store now carries a swept flag set by delete() and cleared by the next identity save, so device-local slot writes serialize against the wipe at their only choke point; the reconcile also re-checks cancellation before its final save. - The mirror did not converge after transient keychain failures: observed-set bookkeeping lived in the fallible body (a later shrink was misread as refill, undoing a deny in the backup) and a failed save was only retried on the next database change. Bookkeeping moved to the observe loop and failed writes retry on the next emission or the window flush. - QA test 45's a_to_b note still documented the invite-deep-link workaround for the bug this PR fixes, which would let that step pass during a consent-restore regression; it now instructs list-only navigation and fail-fast. Three new unit tests cover the carry-window expiry, the failed-write retry, and the swept-slot no-op semantics. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The documented form (positional conversation id, --timeout flag) is not a valid convos CLI command. Documents the correct plural/flag form, external timeout bounding, and the watcher stop condition (wait for Sent ProfileSnapshot; killing at the Adding line consumes the request without completing the add-member commit). Surfaced by the test 45 regression run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two reinstall fixes, driven by Jarod's on-device testing and the new QA test 45 (included). The iOS keychain outlives the app, so a reinstall resumes the identity but wipes the app container - which orphaned the old XMTP installation as a ghost device row, and silently hid every conversation.
1. Ghost "Device " row after every reinstall
A reinstall mints a new installation; the previous one's keys died with the deleted container, but it stays registered on the inbox, and revoking it from the Devices list just orphans the next one. A device-local keychain marker (
InstallationMarker) now records which installation this device last ran - any marker installation that isn't the current one is provably this device's own dead installation (never a paired device), so it is auto-revoked on session ready. Seeded as soon as the client exists (survives startup failures); failed revokes retry next launch; inbox changes reset without revoking; skipped in the NSE.2. Conversations invisible after reinstall (major, found by QA test 45)
Consent records died with the container, re-welcomed conversations landed
consent=unknown, and the list filters those with no requests surface - messages were received and stored but the user saw the first-run empty state forever, relaunch included.The network cannot restore consent: libxmtp propagates consent records as messages inside the device-sync MLS group, which a new installation cannot decrypt retroactively (and the only peer that could re-send them is the dead installation). Instead,
ConsentBackupMirror(session-scoped GRDB observer in SyncingManager) continuously mirrors the allowed-conversation set into the device-local keychain, and the stale-installation reconcile restores it through XMTP's preferences layer - records written before a welcome arrives still apply to it - then flips already-stored unknown rows. Denied conversations are never resurrected; a replaced identity's backup is ignored.QA test 45 (new): reinstall message continuity
Fresh account on an erased disposable clone, two conversations with the CLI, bidirectional baseline chat,
simctl uninstall+ reinstall, then bidirectional delivery checks in both conversations plus identity/revocation/history criteria.e7ee334257db: found bug 2 (FAIL on b_to_a_delivery, all reinstall mechanics passing).c4ba3927cf16with both fixes: all six criteria pass -stale_own_installations_revoked count=1,consent_backup_restored count=3, conversations resurfaced as allowed, delivery both ways through the UI.Tests
Also folds in QA-doc fixes surfaced by the test runs (RULES.md home-shell staleness, join-request watch ordering, case-insensitive greps).
🤖 Generated with Claude Code
Note
Auto-revoke dead installations and restore conversation consent after reinstall
StaleInstallationReconcilerandInstallationMarkerto track orphaned installations across launches, and integrates the reconcile flow intoSessionStateMachineso stale own installations are automatically revoked after reinstall.ConsentBackupandConsentBackupMirrorto continuously mirror allowed conversation ids into a device-local keychain slot with a 5-minute carry window, so consent can be restored after reinstall without relying on peers.KeychainIdentityStorewith device-local slots for the installation marker and consent backup; writes to these slots are blocked afterdelete()and re-enabled only after the nextsave().Macroscope summarized 7ff5adf.