fix: unified profile - activate backfill and populate live data#1129
fix: unified profile - activate backfill and populate live data#1129cameronvoell wants to merge 8 commits into
Conversation
|
Claude finished @cameronvoell's task —— View job Code Review Complete ✅I've reviewed this PR and identified several critical issues that need to be addressed before merging. The macroscopeapp bot has already identified most of these, but I'm providing additional context and prioritization. 🔴 Critical Issues (Must Fix)1. Unstructured Task Creates Race Condition on TeardownLocation: The Task that calls Fix: Store the task handle and cancel it in private var profileMirroringTask: Task<Void, Never>?
// In init:
profileMirroringTask = Task { [weak self] in
guard let self, !Task.isCancelled else { return }
await self.loadOrCreateService().startProfileMirroring()
}
// In deinit:
profileMirroringTask?.cancel()2. Canonical Profile Tables Not Cleared on Account DeletionLocation: The delete-all-inboxes path wipes legacy Fix: Add to try DBProfile.deleteAll(db)
try DBProfileAvatar.deleteAll(db)
try DBSelfProfile.deleteAll(db)3. Inbox ID Resolution Blocks Event ProcessingLocation: Both Impact: On session start, if Fix: Make resolution non-blocking: // In apply(_:)
let selfId = cachedSelfInboxId ?? await selfInboxIdProvider()
if selfId == nil {
// Cache miss, fire-and-forget resolution for next time
Task { _ = await resolveSelfInboxId() }
}
if let selfId, event.inboxId == selfId {
return // Skip self-echo
}
// Continue processing event...🟡 High Priority Issues4. DM Conversations Included in Profile PublisherLocation:
Fix: Filter to groups only: try String.fetchAll(db, sql: "SELECT id FROM conversation WHERE kind = 'group'")5. Profile Updates Clobber memberKind and MetadataLocation:
Fix: Read and preserve existing profile data: // Before building ProfileUpdate, read existing profile from DB
let existing = try await selfProfileStore.load()
update.memberKind = existing?.memberKind ?? .unspecified
update.metadata = existing?.metadata ?? [:]
if let name {
update.name = name
}
// ...6. Mirror Never Deletes Stale Canonical DataLocation:
Fix Options:
7. Self Profile Never Re-Syncs After Initial BackfillLocation:
Fix: Merge against existing self profile: if sawSelf {
let existing = try await selfProfileStore.load() ?? DBSelfProfile(...)
let merged = ProfileMerge.mergeSelfProfile(existing: existing, incoming: ...)
if merged != existing {
try await selfProfileStore.save(merged)
}
}🟢 Code Quality & Best PracticesStrengths:
Suggestions:
🧪 Test CoverageGood:
Gaps:
Recommended additions: // Test that mirror processes events when inbox ID isn't ready yet
func testMirrorProcessesEventsBeforeInboxReady()
// Test that canonical tables are cleared on account deletion
func testDeleteAllInboxesClearsCanonicaProfileTables()
// Test that self profile updates after initial backfill
func testMirrorUpdatesSelfProfileOnSubsequentRuns()⚡ Performance Considerations
🔒 Security ConcernsNo security issues identified. Profile data is already user-controlled and properly scoped by inbox/conversation IDs. SummaryThis PR has good architectural foundations but 7 significant issues that need addressing:
The most severe is the unstructured task (#1) which can cause cross-account data pollution. I recommend fixing all critical issues before merging, and at minimum documenting the high-priority issues if they're deferred to follow-up PRs.
|
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
How to use the Graphite Merge QueueAdd the label merge-queue 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. This stack of pull requests is managed by Graphite. Learn more about stacking. |
cameronvoell
changed the title
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| /// Canonical identity source. Dormant until the cutover - nothing renders | ||
| /// from it yet. | ||
| func profilesRepository() -> ProfilesRepository { | ||
| sharedProfilesRepository |
There was a problem hiding this comment.
🟡 Medium Messaging/MessagingService.swift:197
startProfileMirroring() writes the new canonical profile tables (DBProfile, DBProfileAvatar, DBSelfProfile), but the inbox delete path never clears them — wipeResidualInboxRows() only wipes legacy tables. After deleteAllInboxes(), the deleted account's profile names and avatars remain on disk and resurface for the next session that reads from those stores. The delete path needs to wipe the new canonical tables alongside the legacy ones.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @ConvosCore/Sources/ConvosCore/Messaging/MessagingService.swift around line 197:
`startProfileMirroring()` writes the new canonical profile tables (`DBProfile`, `DBProfileAvatar`, `DBSelfProfile`), but the inbox delete path never clears them — `wipeResidualInboxRows()` only wipes legacy tables. After `deleteAllInboxes()`, the deleted account's profile names and avatars remain on disk and resurface for the next session that reads from those stores. The delete path needs to wipe the new canonical tables alongside the legacy ones.
There was a problem hiding this comment.
🟠 High
The Task at line 169 that calls loadOrCreateService().startProfileMirroring() is unstructured — its handle is never stored, so it is never cancelled during teardown. When deleteAllInboxes() runs while this task is still pending, tearDownInbox() calls existing.stopProfileMirroring() and then clears the cache, but this orphaned task can fire afterwards, call startProfileMirroring() on the same MessagingService, and restart the ProfileMemberMirror. That mirror then stays alive with the deleted session's cached selfInboxId, so later DBMemberProfile writes from a new account are mirrored into the canonical profile tables using the old account's identity. Store the task handle and cancel it in deinit alongside the other teardown cancellations.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @ConvosCore/Sources/ConvosCore/Sessions/SessionManager.swift around line 169:
The `Task` at line 169 that calls `loadOrCreateService().startProfileMirroring()` is unstructured — its handle is never stored, so it is never cancelled during teardown. When `deleteAllInboxes()` runs while this task is still pending, `tearDownInbox()` calls `existing.stopProfileMirroring()` and then clears the cache, but this orphaned task can fire afterwards, call `startProfileMirroring()` on the same `MessagingService`, and restart the `ProfileMemberMirror`. That mirror then stays alive with the deleted session's cached `selfInboxId`, so later `DBMemberProfile` writes from a new account are mirrored into the canonical profile tables using the old account's identity. Store the task handle and cancel it in `deinit` alongside the other teardown cancellations.
| return | ||
| } | ||
| var changed = false | ||
|
|
There was a problem hiding this comment.
🟡 Medium Repository/ProfilesRepository.swift:111
apply(_:) awaits resolveSelfInboxId() before processing any inbound event. When the inbox id provider suspends (e.g. the production waitForInboxReadyResult() call), a ProfileDomainEvent for another member blocks at the self-echo check instead of being merged into DBProfile/DBProfileAvatar, contradicting the comment that unresolved self id should not stop processing. Consider resolving the self inbox id without blocking the apply path — e.g. fire-and-forget the resolution and use the cached value if available, otherwise proceed to merge the event.
Also found in 1 other location(s)
ConvosCore/Sources/ConvosCore/Profiles/Repository/ProfileMemberMirror.swift:58
ProfileMemberMirror.mirror(_:)returns immediately whenresolveSelfInboxId()is stillnil.startProfileMirroring()is explicitly launched early fromSessionManagerbefore inbox-ready, and GRDBValueObservation.values(in:)only gives the initial snapshot plus latermemberProfilechanges. If that initial snapshot arrives before the inbox is ready and nomemberProfilerow changes afterward, the canonicalDBProfile/DBSelfProfiletables are never backfilled at all, so this PR's new profile tables remain permanently empty/stale for that session.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @ConvosCore/Sources/ConvosCore/Profiles/Repository/ProfilesRepository.swift around line 111:
`apply(_:)` awaits `resolveSelfInboxId()` before processing any inbound event. When the inbox id provider suspends (e.g. the production `waitForInboxReadyResult()` call), a `ProfileDomainEvent` for another member blocks at the self-echo check instead of being merged into `DBProfile`/`DBProfileAvatar`, contradicting the comment that unresolved self id should not stop processing. Consider resolving the self inbox id without blocking the apply path — e.g. fire-and-forget the resolution and use the cached value if available, otherwise proceed to merge the event.
Also found in 1 other location(s):
- ConvosCore/Sources/ConvosCore/Profiles/Repository/ProfileMemberMirror.swift:58 -- `ProfileMemberMirror.mirror(_:)` returns immediately when `resolveSelfInboxId()` is still `nil`. `startProfileMirroring()` is explicitly launched early from `SessionManager` before inbox-ready, and GRDB `ValueObservation.values(in:)` only gives the initial snapshot plus later `memberProfile` changes. If that initial snapshot arrives before the inbox is ready and no `memberProfile` row changes afterward, the canonical `DBProfile`/`DBSelfProfile` tables are never backfilled at all, so this PR's new profile tables remain permanently empty/stale for that session.
| struct MessagingProfilePublishSession: ProfilePublishSession { | ||
| private let sessionStateManager: any SessionStateManagerProtocol | ||
| private let databaseReader: any DatabaseReader | ||
|
|
||
| init(sessionStateManager: any SessionStateManagerProtocol, databaseReader: any DatabaseReader) { |
There was a problem hiding this comment.
🟡 Medium Repository/MessagingProfilePublishSession.swift:14
conversationIds() returns ids for every row in the conversation table, including DMs, but imageKey returns nil for non-group conversations and sendProfileUpdate throws ProfilePublishSessionError.conversationNotFound for them. Because the publisher enqueues jobs for DM conversations, avatar jobs are dropped as conversationGone and name-only jobs throw and retry forever, so the publisher keeps retrying work that can never succeed for every DM. Consider filtering conversationIds() to only group conversations (e.g., SELECT id FROM conversation WHERE kind = 'group').
func conversationIds() async throws -> [String] {
try await databaseReader.read { db in
- try String.fetchAll(db, sql: "SELECT id FROM conversation")
+ try String.fetchAll(db, sql: "SELECT id FROM conversation WHERE kind = 'group'")
}
}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @ConvosCore/Sources/ConvosCore/Profiles/Repository/MessagingProfilePublishSession.swift around lines 14-18:
`conversationIds()` returns ids for every row in the `conversation` table, including DMs, but `imageKey` returns `nil` for non-group conversations and `sendProfileUpdate` throws `ProfilePublishSessionError.conversationNotFound` for them. Because the publisher enqueues jobs for DM conversations, avatar jobs are dropped as `conversationGone` and name-only jobs throw and retry forever, so the publisher keeps retrying work that can never succeed for every DM. Consider filtering `conversationIds()` to only group conversations (e.g., `SELECT id FROM conversation WHERE kind = 'group'`).
f736df6 to
a6907f7
Compare
cameronvoell
force-pushed
the
cv/profile-table-activate
branch
from
170950b to
69b95b5
Compare
| contentType: "application/octet-stream", | ||
| acl: "public-read" | ||
| ) | ||
| } |
There was a problem hiding this comment.
🟡 Medium Repository/MessagingProfilePublishSession.swift:51
sendProfileUpdate builds a ProfileUpdate with only name and encryptedImage, leaving memberKind at .unspecified and metadata empty. Inbound apply paths treat .unspecified as a real overwrite and empty metadata as nil, so every publish from this session silently clears the sender's agent/template identity and per-sender metadata like connections/timezone for peers. Populate memberKind and metadata from the sender's existing profile before sending, or default memberKind to a no-op value the apply path skips.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @ConvosCore/Sources/ConvosCore/Profiles/Repository/MessagingProfilePublishSession.swift around line 51:
`sendProfileUpdate` builds a `ProfileUpdate` with only `name` and `encryptedImage`, leaving `memberKind` at `.unspecified` and `metadata` empty. Inbound apply paths treat `.unspecified` as a real overwrite and empty metadata as `nil`, so every publish from this session silently clears the sender's agent/template identity and per-sender metadata like `connections`/`timezone` for peers. Populate `memberKind` and `metadata` from the sender's existing profile before sending, or default `memberKind` to a no-op value the apply path skips.
graphite-app
Bot
force-pushed
the
cv/profile-table-merge-repo-publisher
branch
from
a6907f7 to
7969469
Compare
graphite-app
Bot
force-pushed
the
cv/profile-table-activate
branch
from
69b95b5 to
a9adc0d
Compare
There was a problem hiding this comment.
🟡 Medium
ProfileBackfill.mirror(_:) only upserts identities and avatars from the current DBMemberProfile snapshot and never deletes canonical rows for members or conversations that were removed from that table. When a conversation is deleted (e.g., via DBMemberProfile.filter(...).deleteAll(db) in SessionStateMachine/UnusedConversationCache) or the legacy table is cleared on logout, the corresponding DBProfile and DBProfileAvatar rows remain in the canonical stores indefinitely. Because this PR re-runs mirror(_:) on every memberProfile change to keep the canonical stores in sync, they will return stale identities and avatars after deletions instead of matching the live legacy data. Consider tracking removed legacy rows and deleting their canonical counterparts in mirror(_:), or documenting why stale canonical rows are acceptable during the transition.
Also found in 1 other location(s)
ConvosCore/Sources/ConvosCore/Messaging/MessagingService.swift:213
Starting
profileMemberMirrorhere exposes a data-sync bug: the mirror only replays existingDBMemberProfilerows throughProfileBackfill.mirror(rows), but that code never deletes canonicalDBProfileAvatar/DBSelfProfiledata when source rows disappear. If a conversation is deleted or a legacy profile row is removed, the canonical profile tables keep the old avatar/self data forever, so the new profile store diverges from the livememberProfilestate instead of mirroring it.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @ConvosCore/Sources/ConvosCore/Profiles/Repository/ProfileBackfill.swift around line 1:
`ProfileBackfill.mirror(_:)` only upserts identities and avatars from the current `DBMemberProfile` snapshot and never deletes canonical rows for members or conversations that were removed from that table. When a conversation is deleted (e.g., via `DBMemberProfile.filter(...).deleteAll(db)` in `SessionStateMachine`/`UnusedConversationCache`) or the legacy table is cleared on logout, the corresponding `DBProfile` and `DBProfileAvatar` rows remain in the canonical stores indefinitely. Because this PR re-runs `mirror(_:)` on every `memberProfile` change to keep the canonical stores in sync, they will return stale identities and avatars after deletions instead of matching the live legacy data. Consider tracking removed legacy rows and deleting their canonical counterparts in `mirror(_:)`, or documenting why stale canonical rows are acceptable during the transition.
Also found in 1 other location(s):
- ConvosCore/Sources/ConvosCore/Messaging/MessagingService.swift:213 -- Starting `profileMemberMirror` here exposes a data-sync bug: the mirror only replays existing `DBMemberProfile` rows through `ProfileBackfill.mirror(rows)`, but that code never deletes canonical `DBProfileAvatar`/`DBSelfProfile` data when source rows disappear. If a conversation is deleted or a legacy profile row is removed, the canonical profile tables keep the old avatar/self data forever, so the new profile store diverges from the live `memberProfile` state instead of mirroring it.
There was a problem hiding this comment.
🟡 Medium
mirror(_:) only seeds DBSelfProfile when selfProfileStore.load() == nil, so the canonical self profile is never refreshed after the first backfill even when mirror is re-run on later memberProfile changes. Ongoing MyProfileWriter writes still update per-conversation DBMemberProfile rows, not DBSelfProfile, so the canonical self profile stays stale while ProfileMemberMirror is supposed to keep the canonical stores in sync. Consider merging the self row against the existing DBSelfProfile (at the same .contact/floor precedence used for other identities) so later legacy writes fill blanks and real events still supersede, instead of skipping the write once the slot exists.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @ConvosCore/Sources/ConvosCore/Profiles/Repository/ProfileBackfill.swift around line 71:
`mirror(_:)` only seeds `DBSelfProfile` when `selfProfileStore.load() == nil`, so the canonical self profile is never refreshed after the first backfill even when `mirror` is re-run on later `memberProfile` changes. Ongoing `MyProfileWriter` writes still update per-conversation `DBMemberProfile` rows, not `DBSelfProfile`, so the canonical self profile stays stale while `ProfileMemberMirror` is supposed to keep the canonical stores in sync. Consider merging the self row against the existing `DBSelfProfile` (at the same `.contact`/floor precedence used for other identities) so later legacy writes fill blanks and real events still supersede, instead of skipping the write once the slot exists.
cameronvoell
force-pushed
the
cv/profile-table-merge-repo-publisher
branch
from
7969469 to
7517772
Compare
cameronvoell
force-pushed
the
cv/profile-table-activate
branch
from
a9adc0d to
1cc4b0f
Compare
|
folded into #1126 |
1 participant
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Start writing legacy member profiles to the canonical profile tables
ProfileMemberMirror, an actor that observes the legacymemberProfiletable via GRDBValueObservationand mirrors changes into canonical profile stores usingProfileBackfill.mirror(_:).ProfileBackfill.mirror(_:), a public method that mirrors an in-memory set ofDBMemberProfilerows into profile stores, writing only when values differ from what is already stored.ProfilesRepositoryto accept an asyncselfInboxIdProviderclosure instead of a concrete inbox ID string, resolving and caching it lazily on first use.MessagingServiceandSessionManager: mirroring starts (with an initial backfill and repository warm-up) on session start and stops on inbox teardown.📊 Macroscope summarized 5f261c7. 5 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.