Skip to content

fix: unified profile#1126

Merged
cameronvoell merged 43 commits into
devfrom
cv/replace-conversation-profiles-with-unified-profile-table
Jul 7, 2026
Merged

fix: unified profile#1126
cameronvoell merged 43 commits into
devfrom
cv/replace-conversation-profiles-with-unified-profile-table

Conversation

@cameronvoell

@cameronvoell cameronvoell commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

View with Codesmith Autofix with Codesmith
Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.

Note

Replace per-conversation DBMemberProfile with canonical DBProfile for unified identity storage

  • Introduces canonical DBProfile, DBProfileAvatar, and related tables to store per-inbox identity and per-conversation avatar slots, replacing scattered DBMemberProfile lookups across messaging, streaming, and contact sync.
  • Adds ProfilesRepository, ProfilePublisher, ProfileInboundApplier, and ProfileMerge to manage reactive profile reads, durable publish jobs, and precedence-based identity merges.
  • Profile updates/snapshots in MessagingService and StreamProcessor now delegate to ProfileInboundApplier.apply instead of inline DBMemberProfile writes.
  • InboxProfileAvatarView is introduced so contact rows and conversation avatars observe the unified profile stream by inboxId and update live.
  • Asset renewal, expired asset recovery, agent verification, and contact sync are all rerouted from DBMemberProfile to DBProfileAvatar/DBProfile.
  • ProfileBackfill migrates existing DBMemberProfile rows into the new canonical tables on service start.
  • Risk: wide schema migration (new tables, dropped selfProfile table, new conversationLocalState column); existing databases are upgraded in place via SharedDatabaseMigrator.

Macroscope summarized 14ea335.

@claude

claude Bot commented Jun 30, 2026

Copy link
Copy Markdown

Claude finished @cameronvoell's task —— View job


PR Review: Unified Profile Architecture

This is a well-architected refactor that introduces a canonical profile system. The implementation is solid with strong test coverage and thoughtful design patterns.

✅ Strengths

Architecture & Design:

  • Clean separation of concerns with ProfilesRepository as the canonical source
  • Actor-based concurrency patterns prevent race conditions
  • Proper precedence rules (contact < appData < profileSnapshot < profileUpdate)
  • Dual caching strategy (in-memory cache + reactive GRDB ValueObservation)
  • Non-clobbering migration via ProfileBackfill at lowest precedence
  • Pure merge logic in ProfileMerge (no I/O, easily testable)

Code Quality:

  • Comprehensive test coverage (24 test files covering core flows)
  • Integration tests verify end-to-end profile persistence
  • Clear documentation in implementation design doc
  • Error handling with fallbacks (transient errors don't terminate streams)
  • Idempotent operations (backfill, apply)

Security:

  • Avatar encryption enforcement (rejects unencrypted avatars)
  • Proper scoping (self profile queries filtered by current inbox)
  • No plaintext avatar storage

⚠️ Issues to Address

High Priority

1. Profile table cleanup on account deletion (ProfilesRepository.swift:380)
The purgeConversationAvatars method only deletes avatars for a specific conversation, but there's no cleanup when an account is deleted. Legacy profile data (DBProfile, DBProfileAvatar, DBMyProfile, DBProfilePublishJob) will accumulate.

Recommendation: Add a purgeAccount(inboxId:) method called during account deletion that clears all profile tables for that inbox.

2. Task cancellation during teardown (ProfilePublisher.swift:69, ProfilesRepository.swift:288)
The ProfilePublisher.drain() task and other long-running operations aren't explicitly cancelled when the session tears down. This could leak background work.

Recommendation: Track the drain task and cancel it in unbind():

private var drainTask: Task<Void, Never>?

func attach(session: any ProfilePublishSession) async {
    // ...
    drainTask = Task { await drain() }
}

func detach() async {
    drainTask?.cancel()
    drainTask = nil
    // ...
}

3. Self-profile handling in associations (Automated review comment)
The repository applies a "skip self events" guard in apply(), but it's unclear if all entry points respect this. Need to verify that ProfileInboundApplier and other callers never attempt to store the current user's identity in DBProfile.

Recommendation: Add a runtime assertion or logging to catch violations during development.

Medium Priority

4. Cache invalidation race condition (ProfilesRepository.swift:151-166)
warmUp() loads the cache, but concurrent apply() calls during warmup could result in partial cache state. The warmedUp flag prevents re-warmup but doesn't block concurrent writes.

Recommendation: Consider using a continuation or task group to ensure warmup completes before processing events, or document that callers must await warmUp() before calling apply().

5. Error recovery in reactive publishers (ProfilesRepository.swift:68-86)
The profilePublisher() catches errors and returns empty profiles, but transient database errors (e.g., disk full, corruption) will silently surface as missing names/avatars to the UI with no user-facing feedback or retry.

Recommendation: Log errors to Sentry before returning .empty() so transient failures are visible.

6. Merge precedence documentation (ProfileMerge.swift:1-11)
The precedence hierarchy is documented in the file header but not in the ProfileSource enum definition itself, making it easy to misuse.

Recommendation: Add doc comments to ProfileSource with explicit ordering.

Low Priority

7. Test coverage gaps

  • No tests for purgeConversationAvatars interaction with cache
  • No tests for concurrent apply() calls during warmUp()
  • No tests for ProfilePublisher.drain() retry/backoff behavior
  • Missing test for leftover DBMyProfile from previous account (covered in code but not tested end-to-end)

Recommendation: Add tests for these edge cases.

8. Performance consideration
avatarsByInbox is a nested dictionary ([String: [String: DBProfileAvatar]]) that could grow large in accounts with many conversations. Consider measuring memory usage or adding eviction for inactive conversations.

Security

✅ No security concerns identified:

  • All avatars are properly encrypted before storage
  • Self-profile queries are scoped to current inbox
  • No secrets in code or logs

Performance

✅ Generally efficient:

  • In-memory cache reduces database reads
  • ValueObservation provides reactive updates without polling
  • ProfileMerge is pure (no I/O overhead)

⚠️ Potential concern:

  • warmUp() loads all profiles into memory. For accounts with hundreds of contacts, this could be significant. Consider lazy loading or pagination if memory usage becomes an issue.

Test Coverage

✅ Strong coverage:

  • 24 test files with unit, integration, and persistence tests
  • ProfileMerge has comprehensive edge case coverage
  • ProfileBackfill idempotency and non-clobbering verified
  • End-to-end message-sourced profile persistence tested

Recommendations Summary

Before merge:

  1. Add account deletion cleanup (purgeAccount)
  2. Add task cancellation in unbind()
  3. Verify/document self-profile handling in all entry points

Follow-up PRs:
4. Add error logging in reactive publishers
5. Document ProfileSource precedence ordering
6. Add test coverage for identified gaps
7. Monitor memory usage of avatarsByInbox in production

Overall, this is a high-quality refactor with solid foundations. The issues identified are addressable and don't block the core architecture.

cameronvoell commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

How to use the Graphite Merge Queue

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

Comment thread docs/plans/2026-06-29-profile-table-implementation.md
Comment thread docs/plans/2026-06-29-profile-table-implementation.md
Comment thread docs/plans/2026-06-29-profile-table-implementation.md Outdated
Comment thread docs/plans/2026-06-29-profile-table-implementation.md Outdated
@cameronvoell cameronvoell force-pushed the cv/replace-conversation-profiles-with-unified-profile-table branch from cf617a0 to 56c615a Compare June 30, 2026 19:22
@cameronvoell cameronvoell changed the title new profile database tables fix: shared profile database - new tables Jun 30, 2026
@cameronvoell cameronvoell changed the title fix: shared profile database - new tables fix: unified profile - new database tables Jun 30, 2026
Comment thread docs/plans/2026-06-29-profile-table-implementation.md
Comment thread ConvosCore/Sources/ConvosCore/Profiles/Repository/Stores/SelfProfileStore.swift Outdated
@cameronvoell cameronvoell changed the title fix: unified profile - new database tables fix: unified profile - new database tables/persistence Jun 30, 2026
@cameronvoell cameronvoell force-pushed the cv/fix-image-flickering-loop branch from 4337333 to f800fdd Compare July 1, 2026 17:49
@cameronvoell cameronvoell force-pushed the cv/replace-conversation-profiles-with-unified-profile-table branch from 6e23b2f to d592e9c Compare July 1, 2026 17:49
Comment thread ConvosCore/Sources/ConvosCore/Storage/SharedDatabaseMigrator.swift
Comment thread docs/plans/2026-06-29-profile-table-implementation.md
@graphite-app graphite-app Bot changed the base branch from cv/fix-image-flickering-loop to graphite-base/1126 July 1, 2026 20:01
@graphite-app graphite-app Bot force-pushed the cv/replace-conversation-profiles-with-unified-profile-table branch from d592e9c to 0934417 Compare July 1, 2026 20:06
@graphite-app graphite-app Bot force-pushed the graphite-base/1126 branch from f800fdd to 8be2d61 Compare July 1, 2026 20:06
@graphite-app graphite-app Bot changed the base branch from graphite-base/1126 to dev July 1, 2026 20:07
@graphite-app graphite-app Bot force-pushed the cv/replace-conversation-profiles-with-unified-profile-table branch from 0934417 to 7d284bf Compare July 1, 2026 20:07
Comment thread docs/plans/2026-06-29-profile-table-implementation.md Outdated
@cameronvoell cameronvoell force-pushed the cv/replace-conversation-profiles-with-unified-profile-table branch from 7d284bf to 7444f90 Compare July 2, 2026 18:47
@cameronvoell cameronvoell changed the title fix: unified profile - new database tables/persistence fix: unified profile Jul 2, 2026
Comment thread ConvosCore/Sources/ConvosCore/Sessions/SessionManager.swift
Comment thread ConvosCore/Sources/ConvosCore/Sessions/SessionManager.swift
Comment thread ConvosCore/Sources/ConvosCore/Sessions/SessionManager.swift
@cameronvoell cameronvoell merged commit 0dc31f4 into dev Jul 7, 2026
11 checks passed
@cameronvoell cameronvoell deleted the cv/replace-conversation-profiles-with-unified-profile-table branch July 7, 2026 04:58
yewreeka added a commit that referenced this pull request Jul 8, 2026
…data (#1144)

* fix: restore per-conversation scoping for grant/timezone profile metadata

The unified-profile cutover (#1126) rerouted ProfileMetadataWriter through
the global DBMyProfile.metadata map: conversation-scoped keys (cloud
connection grants, agent timezone) were merged into one global map and
broadcast to every conversation on each lazy publish. Granting a
connection in one conversation overwrote another conversation's grants,
grant details and timezone leaked to every conversation the user
touched, and the grant writer's send-failure contract (decline to
persist the local grant on a failed publish) was silently voided because
the global path performs no network send.

Restore the scoping with a new selfConversationMetadata table keyed by
(inboxId, conversationId):

- ProfileMetadataWriter reads and merges the scoped map for one
  conversation and publishes it immediately through the new
  ProfilesRepository.publishMyProfileMetadata(_:toConversation:), which
  throws on failure so grant callers can decline to persist.
- ProfilePublisher merges each conversation's scoped keys over the
  global map at send time (scoped wins), so a later name or avatar
  publish can never wipe a conversation's grants at the receiver. The
  scoped map is persisted only after a successful send.
- The createSelfConversationMetadata migration also strips connections/
  timezone out of any DBMyProfile.metadata written by the interim global
  routing, so polluted dev installs stop broadcasting them.
- Scoped rows cascade on conversation delete and are cleared by
  wipeAccountScopedRows.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix: address PR review - clear scoped rows in clear(), document persist-after-send ordering

GRDBSelfProfileStore.clear() now deletes the inbox's scoped-metadata rows
alongside the profile row, matching the in-memory implementation so the
store contract holds (no production caller today, but a future one must
not resurface a cleared account's grants). Contract test extended.

publishScopedMetadata keeps its send-then-persist ordering; the doc
comment now spells out why both alternatives (persist-before-send,
non-fatal persist failure) leave worse end states.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix: propagate metadata clears to receivers; thread inbox id through scoped store

Macroscope re-review round two. The High: clearing the last scoped key
produced a ProfileUpdate with no metadata field, which post-cutover
receivers treated as "no change" - a revoked grant survived forever on
the receiver. The wire cannot distinguish an empty map from an absent
one (proto3 map), but every wire client re-sends its full map on each
update - the legacy receivers replaced the stored map wholesale and the
CLI/agents were built against that (serve.ts merges existing metadata
into every send). The unified-profile cutover silently changed this to
keep-on-absent. Restore replace-including-clear semantics for the
addressed ProfileUpdate paths (stream, NSE, history replay): a winning
update's non-nil map replaces the stored one and an empty map clears it,
while nil (snapshot/appData fill paths) still says nothing. The recency
guard keeps an older replay from clearing newer data.

The Medium: the scoped-metadata store resolved the self inbox id through
its own provider, so a transiently nil answer mid-drain could silently
drop a conversation's scoped keys from a queued send. The scoped store
methods now take the caller's already-resolved inbox id explicitly,
matching the ProfileStore style.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix: scope empty-map metadata clears to scoped keys; tombstone the clear

Macroscope round three, both findings valid against the previous
replace-including-clear change:

An omitted metadata map decodes identically to an empty one (proto3), so
treating an empty map as a wholesale clear let a name-only update from
any client that does not re-send its map wipe unrelated stored metadata,
including the agent attestation. A winning update's empty map now clears
only the conversation-scoped keys (new shared ConversationScopedMetadataKey:
connections, timezone) - the keys whose senders always publish their full
current state - and leaves everything else untouched.

A clear stored as nil was indistinguishable from never-known, so the
fill-blank path could resurrect revoked grants from a stale snapshot.
The clear now leaves an empty-map tombstone (mirroring the avatar-slot
tombstone), which fill paths treat as known-and-empty.

Covered by new merge- and applier-level tests: scoped-only clearing
preserves an attestation through a rename, and the tombstone blocks
snapshot resurrection.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant