From 57e60c5595117690b4f3ae8bc1d48ba84044c731 Mon Sep 17 00:00:00 2001 From: Tom Cardoso Date: Fri, 5 Jun 2026 17:55:51 -0400 Subject: [PATCH 1/4] Add sync_tombstones table to propagate sub-table deletions across clients MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #422 Previously, sub-table deletions (tags in particular) were silently resurrected on other clients because the pull-side merge was additive-only. Tombstones give each deletion a durable identity that survives beyond the deleted row itself. Key changes: - schema.ts / shared-schema.ts: add sync_tombstones (table_name, row_id, deleted_at) - contacts:remove-tag: SELECT row id before DELETE, then INSERT tombstone - contacts:update: snapshot row IDs before each sub-table DELETE; insert tombstones for any rows not present in the re-inserted set - engine.ts — pullTombstones: copy shared tombstones to local (phase 2) - engine.ts — pushTombstones: copy local tombstones to shared + GC (phase 3) - engine.ts — loadLocalTombstones: build per-table Set map once per pull - engine.ts — mergeSubTablesFromShared: filter local-only rows against tombstones - engine.ts — pushSubTablesToShared tags: changed to full DELETE+INSERT (was additive-only) so tag deletions actually reach shared before tombstones can filter them - GC: tombstones older than 90 days are pruned from both DBs on each sync Co-Authored-By: Claude Sonnet 4.6 --- src/main/database/index.ts | 6 +- src/main/database/schema.ts | 9 +++ src/main/database/shared-db.ts | 10 --- src/main/database/shared-schema.ts | 9 +++ src/main/ipc/contacts.ts | 54 +++++++++++---- src/main/sync/engine.ts | 85 ++++++++++++++--------- src/test/sync-engine.test.ts | 105 +++++++++++++++++++++++++++++ src/test/tags.test.ts | 17 +++++ 8 files changed, 235 insertions(+), 60 deletions(-) diff --git a/src/main/database/index.ts b/src/main/database/index.ts index 5622389..23f993b 100644 --- a/src/main/database/index.ts +++ b/src/main/database/index.ts @@ -136,11 +136,7 @@ export function runMigrations(db: Database.Database): void { const version = db.pragma('user_version', { simple: true }) as number; if (version >= DB_VERSION) return; - // No migration blocks yet — all schema changes so far are baked into the - // initial schema SQL, so existing pre-production databases can be recreated. - // Wrap in a transaction to establish the correct pattern: when real migration - // DDL blocks are added, each block must stamp user_version atomically with its - // DDL so a mid-migration crash cannot leave the DB partially migrated (fixes #207). + // No migration blocks yet — new DBs receive all schema via initDatabase → LOCAL_SCHEMA_DDL_SQL. db.transaction(() => { db.pragma(`user_version = ${DB_VERSION}`); })(); diff --git a/src/main/database/schema.ts b/src/main/database/schema.ts index 757e20b..28f5120 100644 --- a/src/main/database/schema.ts +++ b/src/main/database/schema.ts @@ -237,7 +237,16 @@ export const LOCAL_SCHEMA_DDL_SQL = ` UNIQUE(contact_id, tag) ); + CREATE TABLE IF NOT EXISTS sync_tombstones ( + id TEXT PRIMARY KEY, + table_name TEXT NOT NULL, + row_id TEXT NOT NULL, + deleted_at INTEGER NOT NULL, + UNIQUE(table_name, row_id) + ); + CREATE INDEX IF NOT EXISTS idx_contact_tags_contact_id ON contact_tags(contact_id); + CREATE INDEX IF NOT EXISTS idx_sync_tombstones_table_row ON sync_tombstones(table_name, row_id); CREATE INDEX IF NOT EXISTS idx_contact_emails_contact_id ON contact_emails(contact_id); CREATE UNIQUE INDEX IF NOT EXISTS idx_contact_emails_contact_email ON contact_emails(contact_id, email); CREATE INDEX IF NOT EXISTS idx_contact_phones_contact_id ON contact_phones(contact_id); diff --git a/src/main/database/shared-db.ts b/src/main/database/shared-db.ts index c2fc8d4..29c2021 100644 --- a/src/main/database/shared-db.ts +++ b/src/main/database/shared-db.ts @@ -85,16 +85,6 @@ function runSharedMigrations(db: Database.Database): void { if (version < 1) { db.pragma('user_version = 1'); } - // Future migration blocks go here. Example: - // if (version < 2) { - // db.prepare('ALTER TABLE contacts ADD COLUMN foo TEXT').run(); - // db.prepare( - // `INSERT OR REPLACE INTO shared_meta (key, value) VALUES ('min_app_version', '0.2.0')` - // ).run(); - // // Note: always use INSERT OR REPLACE here, not ON CONFLICT ... WHERE value > ..., - // // because SQLite compares TEXT lexicographically and '0.9.0' > '0.10.0' is true. - // db.pragma('user_version = 2'); - // } } export function createSharedDb( diff --git a/src/main/database/shared-schema.ts b/src/main/database/shared-schema.ts index 63c081a..9649608 100644 --- a/src/main/database/shared-schema.ts +++ b/src/main/database/shared-schema.ts @@ -64,6 +64,14 @@ export const SHARED_SCHEMA_SQL = ` UNIQUE(contact_id, tag) ); + CREATE TABLE IF NOT EXISTS sync_tombstones ( + id TEXT PRIMARY KEY, + table_name TEXT NOT NULL, + row_id TEXT NOT NULL, + deleted_at INTEGER NOT NULL, + UNIQUE(table_name, row_id) + ); + CREATE TABLE IF NOT EXISTS contact_alert_rss ( id TEXT PRIMARY KEY, contact_id TEXT NOT NULL REFERENCES contacts(id) ON DELETE CASCADE, @@ -118,6 +126,7 @@ export const SHARED_SCHEMA_SQL = ` ); CREATE INDEX IF NOT EXISTS idx_shared_contact_tags_contact_id ON contact_tags(contact_id); + CREATE INDEX IF NOT EXISTS idx_shared_sync_tombstones_table_row ON sync_tombstones(table_name, row_id); CREATE INDEX IF NOT EXISTS idx_shared_contact_emails_contact_id ON contact_emails(contact_id); CREATE UNIQUE INDEX IF NOT EXISTS idx_shared_contact_emails_contact_email ON contact_emails(contact_id, email); CREATE INDEX IF NOT EXISTS idx_shared_contact_phones_contact_id ON contact_phones(contact_id); diff --git a/src/main/ipc/contacts.ts b/src/main/ipc/contacts.ts index 3d4e014..78d4d24 100644 --- a/src/main/ipc/contacts.ts +++ b/src/main/ipc/contacts.ts @@ -446,20 +446,27 @@ export function registerContactHandlers(): void { .all(data.id) as { url: string; wayback_url: string | null }[]; existingWaybacks = new Map(existingWebsites.map((r) => [r.url, r.wayback_url])); - // Snapshot created_at values keyed by normalised value so they survive the re-insert. - const existingEmailCreatedAt = new Map( - (stmt(db, 'SELECT email, created_at FROM contact_emails WHERE contact_id = ?').all(data.id) as { email: string; created_at: number }[]) - .map((r) => [r.email, r.created_at]) - ); - const existingPhoneCreatedAt = new Map( - (stmt(db, 'SELECT phone, created_at FROM contact_phones WHERE contact_id = ?').all(data.id) as { phone: string; created_at: number }[]) - .map((r) => [r.phone, r.created_at]) - ); - const existingLinkCreatedAt = new Map( - (stmt(db, 'SELECT url, created_at FROM contact_links WHERE contact_id = ?').all(data.id) as { url: string; created_at: number }[]) - .map((r) => [r.url.trim(), r.created_at]) + // Snapshot created_at values and row IDs keyed by normalised value so they survive the re-insert. + // Row IDs are used to write tombstones for sub-table rows that get deleted on sync. + const existingEmailRows = stmt(db, 'SELECT id, email, created_at FROM contact_emails WHERE contact_id = ?').all(data.id) as { id: string; email: string; created_at: number }[]; + const existingEmailCreatedAt = new Map(existingEmailRows.map((r) => [r.email, r.created_at])); + const existingEmailIdMap = new Map(existingEmailRows.map((r) => [r.email, r.id])); + + const existingPhoneRows = stmt(db, 'SELECT id, phone, created_at FROM contact_phones WHERE contact_id = ?').all(data.id) as { id: string; phone: string; created_at: number }[]; + const existingPhoneCreatedAt = new Map(existingPhoneRows.map((r) => [r.phone, r.created_at])); + const existingPhoneIdMap = new Map(existingPhoneRows.map((r) => [r.phone, r.id])); + + const existingLinkRows = stmt(db, 'SELECT id, url, created_at FROM contact_links WHERE contact_id = ?').all(data.id) as { id: string; url: string; created_at: number }[]; + const existingLinkCreatedAt = new Map(existingLinkRows.map((r) => [r.url.trim(), r.created_at])); + const existingLinkIdMap = new Map(existingLinkRows.map((r) => [r.url.trim(), r.id])); + + const existingHandleIdMap = new Map( + (stmt(db, 'SELECT id, type, handle FROM contact_handles WHERE contact_id = ?').all(data.id) as { id: string; type: string; handle: string }[]) + .map((r) => [`${r.type}:${r.handle}`, r.id]) ); + const tombstone = stmt(db, 'INSERT OR IGNORE INTO sync_tombstones (id, table_name, row_id, deleted_at) VALUES (?, ?, ?, ?)'); + stmt(db, 'UPDATE contacts SET name = ?, organization = ?, title = ?, dob = ?, notes = ?, updated_at = ? WHERE id = ?', ).run(data.name.trim(), data.organization?.trim() || null, data.title?.trim() || null, /^\d{4}-\d{2}-\d{2}$/.test(data.dob?.trim() ?? '') ? data.dob!.trim() : null, data.notes?.trim() || null, now, data.id); @@ -473,6 +480,10 @@ export function registerContactHandlers(): void { 'INSERT INTO contact_emails (id, contact_id, email, label, sort_order, created_at) VALUES (?, ?, ?, ?, ?, ?)', ).run(uuidv4(), data.id, e.email, e.label, i, existingEmailCreatedAt.get(e.email) ?? now); }); + const keptEmails = new Set(emails.map((e) => e.email)); + for (const [email, id] of existingEmailIdMap) { + if (!keptEmails.has(email)) tombstone.run(uuidv4(), 'contact_emails', id, now); + } stmt(db, 'DELETE FROM contact_phones WHERE contact_id = ?').run(data.id); const phones = (data.phones ?? []) @@ -484,6 +495,10 @@ export function registerContactHandlers(): void { 'INSERT INTO contact_phones (id, contact_id, phone, label, sort_order, created_at) VALUES (?, ?, ?, ?, ?, ?)', ).run(uuidv4(), data.id, p.phone, p.label, i, existingPhoneCreatedAt.get(p.phone) ?? now); }); + const keptPhones = new Set(phones.map((p) => p.phone)); + for (const [phone, id] of existingPhoneIdMap) { + if (!keptPhones.has(phone)) tombstone.run(uuidv4(), 'contact_phones', id, now); + } stmt(db, 'DELETE FROM contact_links WHERE contact_id = ?').run(data.id); const links = (data.links ?? []).filter((l: ContactLinkInput) => l.url.trim() && validateUrl(l.url)); @@ -492,6 +507,10 @@ export function registerContactHandlers(): void { 'INSERT INTO contact_links (id, contact_id, type, label, url, sort_order, created_at) VALUES (?, ?, ?, ?, ?, ?, ?)', ).run(uuidv4(), data.id, link.type, link.label ?? null, link.url.trim(), i, existingLinkCreatedAt.get(link.url.trim()) ?? now); }); + const keptLinks = new Set(links.map((l: ContactLinkInput) => l.url.trim())); + for (const [url, id] of existingLinkIdMap) { + if (!keptLinks.has(url)) tombstone.run(uuidv4(), 'contact_links', id, now); + } stmt(db, 'DELETE FROM contact_handles WHERE contact_id = ?').run(data.id); const handles = (data.handles ?? []).filter((h: ContactHandleInput) => h.handle.trim() && h.type.trim()); @@ -500,6 +519,10 @@ export function registerContactHandlers(): void { 'INSERT INTO contact_handles (id, contact_id, type, handle, sort_order, created_at) VALUES (?, ?, ?, ?, ?, ?)', ).run(uuidv4(), data.id, h.type.trim(), h.handle.trim(), i, now); }); + const keptHandles = new Set(handles.map((h: ContactHandleInput) => `${h.type.trim()}:${h.handle.trim()}`)); + for (const [key, id] of existingHandleIdMap) { + if (!keptHandles.has(key)) tombstone.run(uuidv4(), 'contact_handles', id, now); + } // Restore wayback_urls for previously saved website links for (const [url, waybackUrl] of existingWaybacks) { @@ -775,8 +798,11 @@ export function registerContactHandlers(): void { const db = getDatabase(); const now = Math.floor(Date.now() / 1000); db.transaction(() => { - const { changes } = db.prepare('DELETE FROM contact_tags WHERE contact_id = ? AND tag = ?').run(contactId, normalized); - if (changes > 0) db.prepare('UPDATE contacts SET updated_at = ? WHERE id = ?').run(now, contactId); + const row = db.prepare('SELECT id FROM contact_tags WHERE contact_id = ? AND tag = ?').get(contactId, normalized) as { id: string } | undefined; + if (!row) return; + db.prepare('DELETE FROM contact_tags WHERE contact_id = ? AND tag = ?').run(contactId, normalized); + db.prepare('UPDATE contacts SET updated_at = ? WHERE id = ?').run(now, contactId); + db.prepare('INSERT OR IGNORE INTO sync_tombstones (id, table_name, row_id, deleted_at) VALUES (?, ?, ?, ?)').run(uuidv4(), 'contact_tags', row.id, now); })(); broadcastContactsChanged(); }); diff --git a/src/main/sync/engine.ts b/src/main/sync/engine.ts index e65a767..6d40e4a 100644 --- a/src/main/sync/engine.ts +++ b/src/main/sync/engine.ts @@ -43,6 +43,7 @@ export function syncProject( // Phase 2: pull from shared → local localDb.transaction(() => { + pullTombstones(localDb, sharedDb); pullContacts(localDb, sharedDb); pullMemberships(localDb, sharedDb, projectId, now); pullAppendOnly(localDb, sharedDb); @@ -61,6 +62,7 @@ export function syncProject( let pushedLogEntryIds: string[]; sharedDb.transaction(() => { + pushTombstones(localDb, sharedDb, now); pushedContactIds = pushContacts(localDb, sharedDb, contactIds); pushedMembershipIds = pushMemberships(localDb, sharedDb, projectId); ({ mentionIds: pushedMentionIds, logEntryIds: pushedLogEntryIds } = @@ -73,12 +75,14 @@ export function syncProject( const stmtMembershipSynced = localDb.prepare('UPDATE project_memberships SET synced_at = ? WHERE id = ?'); const stmtMentionSynced = localDb.prepare('UPDATE contact_alert_mentions SET synced_at = ? WHERE id = ?'); const stmtLogEntrySynced = localDb.prepare('UPDATE interaction_log_entries SET synced_at = ? WHERE id = ?'); + const TOMBSTONE_TTL = 90 * 24 * 3600; localDb.transaction(() => { for (const id of pushedContactIds!) stmtContactSynced.run(now, id); for (const id of pushedMembershipIds!) stmtMembershipSynced.run(now, id); for (const id of pushedMentionIds!) stmtMentionSynced.run(now, id); for (const id of pushedLogEntryIds!) stmtLogEntrySynced.run(now, id); localDb.prepare('UPDATE projects SET shared_pending_writes = 0, last_synced_at = ? WHERE id = ?').run(now, projectId); + localDb.prepare('DELETE FROM sync_tombstones WHERE deleted_at < ?').run(now - TOMBSTONE_TTL); })(); return { success: true }; @@ -92,6 +96,37 @@ export function syncProject( } } +// --------------------------------------------------------------------------- +// Tombstone helpers +// --------------------------------------------------------------------------- + +function pullTombstones(local: Database.Database, shared: Database.Database): void { + const rows = shared.prepare('SELECT id, table_name, row_id, deleted_at FROM sync_tombstones').all() as { + id: string; table_name: string; row_id: string; deleted_at: number; + }[]; + const insert = local.prepare('INSERT OR IGNORE INTO sync_tombstones (id, table_name, row_id, deleted_at) VALUES (?, ?, ?, ?)'); + for (const r of rows) insert.run(r.id, r.table_name, r.row_id, r.deleted_at); +} + +function pushTombstones(local: Database.Database, shared: Database.Database, now: number): void { + const rows = local.prepare('SELECT id, table_name, row_id, deleted_at FROM sync_tombstones').all() as { + id: string; table_name: string; row_id: string; deleted_at: number; + }[]; + const insert = shared.prepare('INSERT OR IGNORE INTO sync_tombstones (id, table_name, row_id, deleted_at) VALUES (?, ?, ?, ?)'); + for (const r of rows) insert.run(r.id, r.table_name, r.row_id, r.deleted_at); + shared.prepare('DELETE FROM sync_tombstones WHERE deleted_at < ?').run(now - 90 * 24 * 3600); +} + +function loadLocalTombstones(local: Database.Database): Map> { + const map = new Map>(); + for (const { table_name, row_id } of local.prepare('SELECT table_name, row_id FROM sync_tombstones').all() as { table_name: string; row_id: string }[]) { + let s = map.get(table_name); + if (!s) { s = new Set(); map.set(table_name, s); } + s.add(row_id); + } + return map; +} + // --------------------------------------------------------------------------- // Pull helpers // --------------------------------------------------------------------------- @@ -134,6 +169,7 @@ function adoptSharedUuid(local: Database.Database, fromId: string, toId: string) } function pullContacts(local: Database.Database, shared: Database.Database): void { + const tombstones = loadLocalTombstones(local); const sharedContacts = shared.prepare('SELECT id, name, organization, title, dob, notes, created_at, updated_at FROM contacts').all() as { id: string; name: string; @@ -186,7 +222,7 @@ function pullContacts(local: Database.Database, shared: Database.Database): void ) .run(sc.id, sc.name, sc.organization, sc.title ?? null, sc.dob ?? null, sc.notes, sc.created_at, sc.updated_at, 0); - mergeSubTablesFromShared(local, shared, sc.id); + mergeSubTablesFromShared(local, shared, sc.id, tombstones); } } else { // No local contact with this UUID — check for an identity match by email/phone @@ -228,7 +264,7 @@ function pullContacts(local: Database.Database, shared: Database.Database): void ) .run(sc.name, sc.organization, sc.title ?? null, sc.dob ?? null, sc.notes, sc.updated_at, 0, sc.id); } - mergeSubTablesFromShared(local, shared, sc.id); + mergeSubTablesFromShared(local, shared, sc.id, tombstones); } else { // Genuinely new contact (or ambiguous multi-match / already-adopted local contact) local @@ -242,7 +278,7 @@ function pullContacts(local: Database.Database, shared: Database.Database): void ) .run(sc.id, sc.name, sc.organization, sc.title ?? null, sc.dob ?? null, sc.notes, sc.created_at, sc.updated_at, 0); - mergeSubTablesFromShared(local, shared, sc.id); + mergeSubTablesFromShared(local, shared, sc.id, tombstones); } // Keep identity indexes current so later iterations in this sync see updated state @@ -264,22 +300,11 @@ function mergeSubTablesFromShared( local: Database.Database, shared: Database.Database, contactId: string, + tombstones: Map>, ): void { // Sub-table merge strategy: union of shared rows + local-only rows (rows that - // exist locally but not in shared). This is additive on the pull side. - // - // Deletion behaviour: - // Deletions propagate through the PUSH path: when a client deletes a sub-table - // row and saves (bumping updated_at), they become the newer editor and push the - // trimmed sub-tables to shared on the next sync. Other clients then pull that - // state from shared. - // - // The one failure case: if another client edits the same contact after the - // deletion (making their updated_at newer), the deleting client will pull on - // next sync and the merge will restore the deleted row from shared (resurrection). - // The deleted row then exists in the deleting client's local DB, so there is no - // longer any local deletion intent — the row survives. The concurrent edit wins. - // No data is permanently lost, but the deletion is silently discarded. + // exist locally but not in shared). Local-only rows whose IDs appear in the local + // tombstone table are excluded so that explicit deletions propagate across clients. // // ── Emails ──────────────────────────────────────────────────────────────── // Stored emails are already normalised (lowercased, trimmed) — compare directly. @@ -291,7 +316,8 @@ function mergeSubTablesFromShared( .all(contactId) as { id: string; email: string; label: string | null; sort_order: number; created_at: number }[]; const sharedEmailValues = new Set(sharedEmails.map((e) => e.email)); - const localOnlyEmails = localEmails.filter((e) => !sharedEmailValues.has(e.email)); + const emailTombstones = tombstones.get('contact_emails') ?? new Set(); + const localOnlyEmails = localEmails.filter((e) => !sharedEmailValues.has(e.email) && !emailTombstones.has(e.id)); // Merge and sort by insertion time so rows appear in the order they were added. const mergedEmails = [...sharedEmails, ...localOnlyEmails].sort((a, b) => a.created_at - b.created_at); @@ -313,7 +339,8 @@ function mergeSubTablesFromShared( .all(contactId) as { id: string; phone: string; label: string | null; sort_order: number; created_at: number }[]; const sharedPhoneValues = new Set(sharedPhones.map((p) => p.phone)); - const localOnlyPhones = localPhones.filter((p) => !sharedPhoneValues.has(p.phone)); + const phoneTombstones = tombstones.get('contact_phones') ?? new Set(); + const localOnlyPhones = localPhones.filter((p) => !sharedPhoneValues.has(p.phone) && !phoneTombstones.has(p.id)); const mergedPhones = [...sharedPhones, ...localOnlyPhones].sort((a, b) => a.created_at - b.created_at); @@ -335,7 +362,8 @@ function mergeSubTablesFromShared( const sharedUrlValues = new Set(sharedLinks.map((l) => l.url.trim())); const localWaybacks = new Map(localLinks.filter((l) => l.wayback_url).map((l) => [l.url.trim(), l.wayback_url])); - const localOnlyLinks = localLinks.filter((l) => !sharedUrlValues.has(l.url.trim())); + const linkTombstones = tombstones.get('contact_links') ?? new Set(); + const localOnlyLinks = localLinks.filter((l) => !sharedUrlValues.has(l.url.trim()) && !linkTombstones.has(l.id)); // Build merged set: shared rows (with wayback_url restored) + local-only rows, sorted by created_at. const mergedLinks = [ @@ -359,7 +387,8 @@ function mergeSubTablesFromShared( .all(contactId) as { id: string; type: string; handle: string; sort_order: number; created_at: number }[]; const sharedHandleKeys = new Set(sharedHandles.map((h) => `${h.type}:${h.handle}`)); - const localOnlyHandles = localHandles.filter((h) => !sharedHandleKeys.has(`${h.type}:${h.handle}`)); + const handleTombstones = tombstones.get('contact_handles') ?? new Set(); + const localOnlyHandles = localHandles.filter((h) => !sharedHandleKeys.has(`${h.type}:${h.handle}`) && !handleTombstones.has(h.id)); const mergedHandles = [...sharedHandles, ...localOnlyHandles].sort((a, b) => a.created_at - b.created_at); @@ -394,9 +423,6 @@ function mergeSubTablesFromShared( } // ── Tags ────────────────────────────────────────────────────────────────── - // Additive-only merge: deletions do not propagate across clients. A tag - // removed on one client will be restored on next sync if it still exists on - // shared. Full deletion propagation requires a tombstone table — see #422. const sharedTags = shared .prepare('SELECT * FROM contact_tags WHERE contact_id = ?') .all(contactId) as { id: string; tag: string; created_at: number }[]; @@ -405,7 +431,8 @@ function mergeSubTablesFromShared( .all(contactId) as { id: string; tag: string; created_at: number }[]; const sharedTagValues = new Set(sharedTags.map((t) => t.tag)); - const localOnlyTags = localTags.filter((t) => !sharedTagValues.has(t.tag)); + const tagTombstones = tombstones.get('contact_tags') ?? new Set(); + const localOnlyTags = localTags.filter((t) => !sharedTagValues.has(t.tag) && !tagTombstones.has(t.id)); const mergedTags = [...sharedTags, ...localOnlyTags]; local.prepare('DELETE FROM contact_tags WHERE contact_id = ?').run(contactId); @@ -732,17 +759,13 @@ function pushSubTablesToShared( .run(rss.id, contactId, rss.rss_url, rss.last_polled_at, rss.is_invalid); } - const sharedTagSet = new Set( - (shared.prepare('SELECT tag FROM contact_tags WHERE contact_id = ?').all(contactId) as { tag: string }[]).map((r) => r.tag), - ); + shared.prepare('DELETE FROM contact_tags WHERE contact_id = ?').run(contactId); const tagRows = local .prepare('SELECT * FROM contact_tags WHERE contact_id = ?') .all(contactId) as { id: string; tag: string; created_at: number }[]; const insertSharedTag = shared.prepare('INSERT OR IGNORE INTO contact_tags (id, contact_id, tag, created_at) VALUES (?, ?, ?, ?)'); for (const t of tagRows) { - if (!sharedTagSet.has(t.tag)) { - insertSharedTag.run(t.id, contactId, t.tag, t.created_at); - } + insertSharedTag.run(t.id, contactId, t.tag, t.created_at); } } diff --git a/src/test/sync-engine.test.ts b/src/test/sync-engine.test.ts index a708af4..f1a33fa 100644 --- a/src/test/sync-engine.test.ts +++ b/src/test/sync-engine.test.ts @@ -867,3 +867,108 @@ describe('pushAppendOnly — cross-project membership guard (#410)', () => { expect(pushedMembershipIds).not.toContain(membershipB); }); }); + +// --------------------------------------------------------------------------- +// Tombstone sync (#422) +// --------------------------------------------------------------------------- + +describe('sync_tombstones — propagation and resurrection prevention', () => { + it('pushes local tombstones to shared on sync', () => { + const localDb = createTestDb(); + const sharedDb = createSharedDb(); + const projectId = insertProject(localDb, 'Tombstone Push'); + + const contactId = insertContact(localDb, 'Alice'); + localInsertMembership(localDb, contactId, projectId); + sharedInsertContact(sharedDb, contactId, 'Alice'); + sharedInsertMembership(sharedDb, uuidv4(), contactId); + + const rowId = uuidv4(); + localDb.prepare('INSERT INTO sync_tombstones (id, table_name, row_id, deleted_at) VALUES (?, ?, ?, ?)').run(uuidv4(), 'contact_tags', rowId, NOW); + + syncProject(localDb, sharedDb, projectId); + + expect(sharedDb.prepare('SELECT row_id FROM sync_tombstones WHERE row_id = ?').get(rowId)).toBeDefined(); + }); + + it('pulls tombstones from shared into local on sync', () => { + const localDb = createTestDb(); + const sharedDb = createSharedDb(); + const projectId = insertProject(localDb, 'Tombstone Pull'); + + const contactId = insertContact(localDb, 'Alice'); + localInsertMembership(localDb, contactId, projectId); + sharedInsertContact(sharedDb, contactId, 'Alice'); + sharedInsertMembership(sharedDb, uuidv4(), contactId); + + const rowId = uuidv4(); + sharedDb.prepare('INSERT INTO sync_tombstones (id, table_name, row_id, deleted_at) VALUES (?, ?, ?, ?)').run(uuidv4(), 'contact_tags', rowId, NOW); + + syncProject(localDb, sharedDb, projectId); + + expect(localDb.prepare('SELECT row_id FROM sync_tombstones WHERE row_id = ?').get(rowId)).toBeDefined(); + }); + + it('deleted tag on client A is not resurrected on client B after sync', () => { + const sharedDb = createSharedDb(); + const projectId = uuidv4(); + const contactId = uuidv4(); + const tagId = uuidv4(); + const T0 = NOW - 200; + + // Both clients start with the same contact + tag, already synced + const localA = createTestDb(); + const localB = createTestDb(); + for (const db of [localA, localB]) { + db.prepare('INSERT INTO projects (id, name, is_shared, created_at) VALUES (?, ?, 1, ?)').run(projectId, 'Shared', T0); + db.prepare('INSERT INTO contacts (id, name, created_at, updated_at, synced_at) VALUES (?, ?, ?, ?, ?)').run(contactId, 'Alice', T0, T0, T0); + db.prepare('INSERT INTO project_memberships (id, contact_id, project_id, reporter_email, reporter_name, created_at, updated_at, synced_at) VALUES (?, ?, ?, ?, ?, ?, ?, ?)').run(uuidv4(), contactId, projectId, 'r@r.com', 'Reporter', T0, T0, T0); + db.prepare('INSERT INTO contact_tags (id, contact_id, tag, created_at) VALUES (?, ?, ?, ?)').run(tagId, contactId, 'source', T0); + } + sharedDb.prepare('INSERT INTO contacts (id, name, created_at, updated_at) VALUES (?, ?, ?, ?)').run(contactId, 'Alice', T0, T0); + sharedInsertMembership(sharedDb, uuidv4(), contactId); + sharedDb.prepare('INSERT INTO contact_tags (id, contact_id, tag, created_at) VALUES (?, ?, ?, ?)').run(tagId, contactId, 'source', T0); + + // A removes the tag and writes a tombstone (simulates what contacts:remove-tag does) + localA.prepare('DELETE FROM contact_tags WHERE contact_id = ? AND tag = ?').run(contactId, 'source'); + localA.prepare('UPDATE contacts SET updated_at = ? WHERE id = ?').run(NOW, contactId); + localA.prepare('INSERT OR IGNORE INTO sync_tombstones (id, table_name, row_id, deleted_at) VALUES (?, ?, ?, ?)').run(uuidv4(), 'contact_tags', tagId, NOW); + + // A syncs: pushes tombstone + updated contact to shared + expect(syncProject(localA, sharedDb, projectId).success).toBe(true); + expect((sharedDb.prepare('SELECT tag FROM contact_tags WHERE contact_id = ?').all(contactId) as { tag: string }[]).map((r) => r.tag)).not.toContain('source'); + + // B syncs: pulls A's changes including tombstone; tag must not be resurrected + expect(syncProject(localB, sharedDb, projectId).success).toBe(true); + expect((localB.prepare('SELECT tag FROM contact_tags WHERE contact_id = ?').all(contactId) as { tag: string }[]).map((r) => r.tag)).not.toContain('source'); + }); + + it('tombstoned row does not block a fresh re-add of the same email address', () => { + const localDb = createTestDb(); + const sharedDb = createSharedDb(); + const projectId = insertProject(localDb, 'Re-add Test'); + + const contactId = uuidv4(); + const emailId = uuidv4(); + const T0 = NOW - 100; + + localDb.prepare('INSERT INTO contacts (id, name, created_at, updated_at, synced_at) VALUES (?, ?, ?, ?, ?)').run(contactId, 'Alice', T0, T0, T0); + localInsertMembership(localDb, contactId, projectId); + // Old row with known ID, then tombstoned + localDb.prepare('INSERT INTO contact_emails (id, contact_id, email, sort_order, created_at) VALUES (?, ?, ?, 0, ?)').run(emailId, contactId, 'alice@example.com', T0); + localDb.prepare('INSERT INTO sync_tombstones (id, table_name, row_id, deleted_at) VALUES (?, ?, ?, ?)').run(uuidv4(), 'contact_emails', emailId, NOW - 10); + // Re-added with a new UUID + const newEmailId = uuidv4(); + localDb.prepare('DELETE FROM contact_emails WHERE contact_id = ?').run(contactId); + localDb.prepare('INSERT INTO contact_emails (id, contact_id, email, sort_order, created_at) VALUES (?, ?, ?, 0, ?)').run(newEmailId, contactId, 'alice@example.com', NOW - 5); + + sharedInsertContact(sharedDb, contactId, 'Alice', { updatedAt: T0 }); + sharedInsertMembership(sharedDb, uuidv4(), contactId); + + expect(syncProject(localDb, sharedDb, projectId).success).toBe(true); + + // The re-added email (new UUID, not tombstoned) should survive + const emails = (localDb.prepare('SELECT email FROM contact_emails WHERE contact_id = ?').all(contactId) as { email: string }[]).map((r) => r.email); + expect(emails).toContain('alice@example.com'); + }); +}); diff --git a/src/test/tags.test.ts b/src/test/tags.test.ts index 6e74b51..251cbb1 100644 --- a/src/test/tags.test.ts +++ b/src/test/tags.test.ts @@ -110,6 +110,23 @@ describe('contacts:remove-tag', () => { await handlers.get('contacts:remove-tag')!({}, { contactId, tag: 'ghost' }); expect(getTags(testDb, contactId)).toHaveLength(0); }); + + it('writes a tombstone for the removed tag row', async () => { + const contactId = insertContact(testDb, 'Bob'); + const tagId = uuidv4(); + testDb.prepare('INSERT INTO contact_tags (id, contact_id, tag, created_at) VALUES (?, ?, ?, ?)').run(tagId, contactId, 'source', Math.floor(Date.now() / 1000)); + await handlers.get('contacts:remove-tag')!({}, { contactId, tag: 'source' }); + const tombstone = testDb.prepare('SELECT * FROM sync_tombstones WHERE row_id = ?').get(tagId) as { table_name: string } | undefined; + expect(tombstone).toBeDefined(); + expect(tombstone!.table_name).toBe('contact_tags'); + }); + + it('does not write a tombstone when the tag does not exist', async () => { + const contactId = insertContact(testDb, 'Bob'); + await handlers.get('contacts:remove-tag')!({}, { contactId, tag: 'ghost' }); + const count = (testDb.prepare('SELECT COUNT(*) as n FROM sync_tombstones').get() as { n: number }).n; + expect(count).toBe(0); + }); }); // --------------------------------------------------------------------------- From 42d592d3deabb17226f6205bbded1bb76e6c3dbd Mon Sep 17 00:00:00 2001 From: Tom Cardoso Date: Fri, 5 Jun 2026 23:24:13 -0400 Subject: [PATCH 2/4] Restructure sync_tombstones: composite PK, UPSERT with MAX(deleted_at) Addresses review feedback on PR #424. - Drop the separate `id` (UUID) column; use PRIMARY KEY (table_name, row_id) so the table's natural identity matches its semantic identity. Removes the dual-constraint problem that prevented a clean UPSERT. - Drop the now-redundant UNIQUE index on (table_name, row_id). - All tombstone writes (IPC handlers + sync engine) now use INSERT ... ON CONFLICT(table_name, row_id) DO UPDATE SET deleted_at = MAX(...) instead of INSERT OR IGNORE, so a later deletion from a long-offline client correctly extends the TTL rather than being silently dropped. Co-Authored-By: Claude Sonnet 4.6 --- src/main/database/schema.ts | 4 +--- src/main/database/shared-schema.ts | 4 +--- src/main/ipc/contacts.ts | 12 ++++++------ src/main/sync/engine.ts | 22 ++++++++++++++-------- src/test/sync-engine.test.ts | 8 ++++---- 5 files changed, 26 insertions(+), 24 deletions(-) diff --git a/src/main/database/schema.ts b/src/main/database/schema.ts index 28f5120..7f58ca7 100644 --- a/src/main/database/schema.ts +++ b/src/main/database/schema.ts @@ -238,15 +238,13 @@ export const LOCAL_SCHEMA_DDL_SQL = ` ); CREATE TABLE IF NOT EXISTS sync_tombstones ( - id TEXT PRIMARY KEY, table_name TEXT NOT NULL, row_id TEXT NOT NULL, deleted_at INTEGER NOT NULL, - UNIQUE(table_name, row_id) + PRIMARY KEY (table_name, row_id) ); CREATE INDEX IF NOT EXISTS idx_contact_tags_contact_id ON contact_tags(contact_id); - CREATE INDEX IF NOT EXISTS idx_sync_tombstones_table_row ON sync_tombstones(table_name, row_id); CREATE INDEX IF NOT EXISTS idx_contact_emails_contact_id ON contact_emails(contact_id); CREATE UNIQUE INDEX IF NOT EXISTS idx_contact_emails_contact_email ON contact_emails(contact_id, email); CREATE INDEX IF NOT EXISTS idx_contact_phones_contact_id ON contact_phones(contact_id); diff --git a/src/main/database/shared-schema.ts b/src/main/database/shared-schema.ts index 9649608..4273664 100644 --- a/src/main/database/shared-schema.ts +++ b/src/main/database/shared-schema.ts @@ -65,11 +65,10 @@ export const SHARED_SCHEMA_SQL = ` ); CREATE TABLE IF NOT EXISTS sync_tombstones ( - id TEXT PRIMARY KEY, table_name TEXT NOT NULL, row_id TEXT NOT NULL, deleted_at INTEGER NOT NULL, - UNIQUE(table_name, row_id) + PRIMARY KEY (table_name, row_id) ); CREATE TABLE IF NOT EXISTS contact_alert_rss ( @@ -126,7 +125,6 @@ export const SHARED_SCHEMA_SQL = ` ); CREATE INDEX IF NOT EXISTS idx_shared_contact_tags_contact_id ON contact_tags(contact_id); - CREATE INDEX IF NOT EXISTS idx_shared_sync_tombstones_table_row ON sync_tombstones(table_name, row_id); CREATE INDEX IF NOT EXISTS idx_shared_contact_emails_contact_id ON contact_emails(contact_id); CREATE UNIQUE INDEX IF NOT EXISTS idx_shared_contact_emails_contact_email ON contact_emails(contact_id, email); CREATE INDEX IF NOT EXISTS idx_shared_contact_phones_contact_id ON contact_phones(contact_id); diff --git a/src/main/ipc/contacts.ts b/src/main/ipc/contacts.ts index 78d4d24..5f6ce09 100644 --- a/src/main/ipc/contacts.ts +++ b/src/main/ipc/contacts.ts @@ -465,7 +465,7 @@ export function registerContactHandlers(): void { .map((r) => [`${r.type}:${r.handle}`, r.id]) ); - const tombstone = stmt(db, 'INSERT OR IGNORE INTO sync_tombstones (id, table_name, row_id, deleted_at) VALUES (?, ?, ?, ?)'); + const tombstone = stmt(db, 'INSERT INTO sync_tombstones (table_name, row_id, deleted_at) VALUES (?, ?, ?) ON CONFLICT(table_name, row_id) DO UPDATE SET deleted_at = MAX(deleted_at, excluded.deleted_at)'); stmt(db, 'UPDATE contacts SET name = ?, organization = ?, title = ?, dob = ?, notes = ?, updated_at = ? WHERE id = ?', @@ -482,7 +482,7 @@ export function registerContactHandlers(): void { }); const keptEmails = new Set(emails.map((e) => e.email)); for (const [email, id] of existingEmailIdMap) { - if (!keptEmails.has(email)) tombstone.run(uuidv4(), 'contact_emails', id, now); + if (!keptEmails.has(email)) tombstone.run('contact_emails', id, now); } stmt(db, 'DELETE FROM contact_phones WHERE contact_id = ?').run(data.id); @@ -497,7 +497,7 @@ export function registerContactHandlers(): void { }); const keptPhones = new Set(phones.map((p) => p.phone)); for (const [phone, id] of existingPhoneIdMap) { - if (!keptPhones.has(phone)) tombstone.run(uuidv4(), 'contact_phones', id, now); + if (!keptPhones.has(phone)) tombstone.run('contact_phones', id, now); } stmt(db, 'DELETE FROM contact_links WHERE contact_id = ?').run(data.id); @@ -509,7 +509,7 @@ export function registerContactHandlers(): void { }); const keptLinks = new Set(links.map((l: ContactLinkInput) => l.url.trim())); for (const [url, id] of existingLinkIdMap) { - if (!keptLinks.has(url)) tombstone.run(uuidv4(), 'contact_links', id, now); + if (!keptLinks.has(url)) tombstone.run('contact_links', id, now); } stmt(db, 'DELETE FROM contact_handles WHERE contact_id = ?').run(data.id); @@ -521,7 +521,7 @@ export function registerContactHandlers(): void { }); const keptHandles = new Set(handles.map((h: ContactHandleInput) => `${h.type.trim()}:${h.handle.trim()}`)); for (const [key, id] of existingHandleIdMap) { - if (!keptHandles.has(key)) tombstone.run(uuidv4(), 'contact_handles', id, now); + if (!keptHandles.has(key)) tombstone.run('contact_handles', id, now); } // Restore wayback_urls for previously saved website links @@ -802,7 +802,7 @@ export function registerContactHandlers(): void { if (!row) return; db.prepare('DELETE FROM contact_tags WHERE contact_id = ? AND tag = ?').run(contactId, normalized); db.prepare('UPDATE contacts SET updated_at = ? WHERE id = ?').run(now, contactId); - db.prepare('INSERT OR IGNORE INTO sync_tombstones (id, table_name, row_id, deleted_at) VALUES (?, ?, ?, ?)').run(uuidv4(), 'contact_tags', row.id, now); + db.prepare('INSERT INTO sync_tombstones (table_name, row_id, deleted_at) VALUES (?, ?, ?) ON CONFLICT(table_name, row_id) DO UPDATE SET deleted_at = MAX(deleted_at, excluded.deleted_at)').run('contact_tags', row.id, now); })(); broadcastContactsChanged(); }); diff --git a/src/main/sync/engine.ts b/src/main/sync/engine.ts index 6d40e4a..4ebb506 100644 --- a/src/main/sync/engine.ts +++ b/src/main/sync/engine.ts @@ -101,19 +101,25 @@ export function syncProject( // --------------------------------------------------------------------------- function pullTombstones(local: Database.Database, shared: Database.Database): void { - const rows = shared.prepare('SELECT id, table_name, row_id, deleted_at FROM sync_tombstones').all() as { - id: string; table_name: string; row_id: string; deleted_at: number; + const rows = shared.prepare('SELECT table_name, row_id, deleted_at FROM sync_tombstones').all() as { + table_name: string; row_id: string; deleted_at: number; }[]; - const insert = local.prepare('INSERT OR IGNORE INTO sync_tombstones (id, table_name, row_id, deleted_at) VALUES (?, ?, ?, ?)'); - for (const r of rows) insert.run(r.id, r.table_name, r.row_id, r.deleted_at); + const upsert = local.prepare(` + INSERT INTO sync_tombstones (table_name, row_id, deleted_at) VALUES (?, ?, ?) + ON CONFLICT(table_name, row_id) DO UPDATE SET deleted_at = MAX(deleted_at, excluded.deleted_at) + `); + for (const r of rows) upsert.run(r.table_name, r.row_id, r.deleted_at); } function pushTombstones(local: Database.Database, shared: Database.Database, now: number): void { - const rows = local.prepare('SELECT id, table_name, row_id, deleted_at FROM sync_tombstones').all() as { - id: string; table_name: string; row_id: string; deleted_at: number; + const rows = local.prepare('SELECT table_name, row_id, deleted_at FROM sync_tombstones').all() as { + table_name: string; row_id: string; deleted_at: number; }[]; - const insert = shared.prepare('INSERT OR IGNORE INTO sync_tombstones (id, table_name, row_id, deleted_at) VALUES (?, ?, ?, ?)'); - for (const r of rows) insert.run(r.id, r.table_name, r.row_id, r.deleted_at); + const upsert = shared.prepare(` + INSERT INTO sync_tombstones (table_name, row_id, deleted_at) VALUES (?, ?, ?) + ON CONFLICT(table_name, row_id) DO UPDATE SET deleted_at = MAX(deleted_at, excluded.deleted_at) + `); + for (const r of rows) upsert.run(r.table_name, r.row_id, r.deleted_at); shared.prepare('DELETE FROM sync_tombstones WHERE deleted_at < ?').run(now - 90 * 24 * 3600); } diff --git a/src/test/sync-engine.test.ts b/src/test/sync-engine.test.ts index f1a33fa..99f8dfc 100644 --- a/src/test/sync-engine.test.ts +++ b/src/test/sync-engine.test.ts @@ -884,7 +884,7 @@ describe('sync_tombstones — propagation and resurrection prevention', () => { sharedInsertMembership(sharedDb, uuidv4(), contactId); const rowId = uuidv4(); - localDb.prepare('INSERT INTO sync_tombstones (id, table_name, row_id, deleted_at) VALUES (?, ?, ?, ?)').run(uuidv4(), 'contact_tags', rowId, NOW); + localDb.prepare('INSERT INTO sync_tombstones (table_name, row_id, deleted_at) VALUES (?, ?, ?)').run('contact_tags', rowId, NOW); syncProject(localDb, sharedDb, projectId); @@ -902,7 +902,7 @@ describe('sync_tombstones — propagation and resurrection prevention', () => { sharedInsertMembership(sharedDb, uuidv4(), contactId); const rowId = uuidv4(); - sharedDb.prepare('INSERT INTO sync_tombstones (id, table_name, row_id, deleted_at) VALUES (?, ?, ?, ?)').run(uuidv4(), 'contact_tags', rowId, NOW); + sharedDb.prepare('INSERT INTO sync_tombstones (table_name, row_id, deleted_at) VALUES (?, ?, ?)').run('contact_tags', rowId, NOW); syncProject(localDb, sharedDb, projectId); @@ -932,7 +932,7 @@ describe('sync_tombstones — propagation and resurrection prevention', () => { // A removes the tag and writes a tombstone (simulates what contacts:remove-tag does) localA.prepare('DELETE FROM contact_tags WHERE contact_id = ? AND tag = ?').run(contactId, 'source'); localA.prepare('UPDATE contacts SET updated_at = ? WHERE id = ?').run(NOW, contactId); - localA.prepare('INSERT OR IGNORE INTO sync_tombstones (id, table_name, row_id, deleted_at) VALUES (?, ?, ?, ?)').run(uuidv4(), 'contact_tags', tagId, NOW); + localA.prepare('INSERT INTO sync_tombstones (table_name, row_id, deleted_at) VALUES (?, ?, ?)').run('contact_tags', tagId, NOW); // A syncs: pushes tombstone + updated contact to shared expect(syncProject(localA, sharedDb, projectId).success).toBe(true); @@ -956,7 +956,7 @@ describe('sync_tombstones — propagation and resurrection prevention', () => { localInsertMembership(localDb, contactId, projectId); // Old row with known ID, then tombstoned localDb.prepare('INSERT INTO contact_emails (id, contact_id, email, sort_order, created_at) VALUES (?, ?, ?, 0, ?)').run(emailId, contactId, 'alice@example.com', T0); - localDb.prepare('INSERT INTO sync_tombstones (id, table_name, row_id, deleted_at) VALUES (?, ?, ?, ?)').run(uuidv4(), 'contact_emails', emailId, NOW - 10); + localDb.prepare('INSERT INTO sync_tombstones (table_name, row_id, deleted_at) VALUES (?, ?, ?)').run('contact_emails', emailId, NOW - 10); // Re-added with a new UUID const newEmailId = uuidv4(); localDb.prepare('DELETE FROM contact_emails WHERE contact_id = ?').run(contactId); From a523fde504edb6314f4b4af7bb13331cef0fb563 Mon Sep 17 00:00:00 2001 From: Tom Cardoso Date: Fri, 5 Jun 2026 23:38:32 -0400 Subject: [PATCH 3/4] Address second round of review feedback on sync tombstones MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Filter shared rows against local tombstones before merging, not just local-only rows. Fixes the concurrent-edit resurrection case: if client B pushes a contact that re-includes a tombstoned shared row (same ID), it now gets excluded rather than landing in the merged set. - Extract TOMBSTONE_TTL_SECONDS as a module-level constant; was duplicated between pushTombstones and syncProject's phase-4 GC. - Add idx_sync_tombstones_deleted_at to both schemas so the GC DELETE query doesn't require a full table scan. - Change INSERT OR IGNORE → INSERT in pushSubTablesToShared tag loop; the preceding DELETE already cleared the contact's rows so the guard is redundant. - Add contacts:update tombstone test verifying that deleting emails/phones/ links/handles writes the correct sync_tombstones rows. - Add concurrent-edit resurrection test (tombstoned shared row, not just local-only row, is filtered out). Co-Authored-By: Claude Sonnet 4.6 --- src/main/database/schema.ts | 1 + src/main/database/shared-schema.ts | 1 + src/main/sync/engine.ts | 34 ++++++++++++++---------- src/test/contacts.test.ts | 33 +++++++++++++++++++++++ src/test/sync-engine.test.ts | 42 ++++++++++++++++++++++++++++++ 5 files changed, 97 insertions(+), 14 deletions(-) diff --git a/src/main/database/schema.ts b/src/main/database/schema.ts index 7f58ca7..340b17b 100644 --- a/src/main/database/schema.ts +++ b/src/main/database/schema.ts @@ -245,6 +245,7 @@ export const LOCAL_SCHEMA_DDL_SQL = ` ); CREATE INDEX IF NOT EXISTS idx_contact_tags_contact_id ON contact_tags(contact_id); + CREATE INDEX IF NOT EXISTS idx_sync_tombstones_deleted_at ON sync_tombstones(deleted_at); CREATE INDEX IF NOT EXISTS idx_contact_emails_contact_id ON contact_emails(contact_id); CREATE UNIQUE INDEX IF NOT EXISTS idx_contact_emails_contact_email ON contact_emails(contact_id, email); CREATE INDEX IF NOT EXISTS idx_contact_phones_contact_id ON contact_phones(contact_id); diff --git a/src/main/database/shared-schema.ts b/src/main/database/shared-schema.ts index 4273664..f92960c 100644 --- a/src/main/database/shared-schema.ts +++ b/src/main/database/shared-schema.ts @@ -125,6 +125,7 @@ export const SHARED_SCHEMA_SQL = ` ); CREATE INDEX IF NOT EXISTS idx_shared_contact_tags_contact_id ON contact_tags(contact_id); + CREATE INDEX IF NOT EXISTS idx_shared_sync_tombstones_deleted_at ON sync_tombstones(deleted_at); CREATE INDEX IF NOT EXISTS idx_shared_contact_emails_contact_id ON contact_emails(contact_id); CREATE UNIQUE INDEX IF NOT EXISTS idx_shared_contact_emails_contact_email ON contact_emails(contact_id, email); CREATE INDEX IF NOT EXISTS idx_shared_contact_phones_contact_id ON contact_phones(contact_id); diff --git a/src/main/sync/engine.ts b/src/main/sync/engine.ts index 4ebb506..65ccb4e 100644 --- a/src/main/sync/engine.ts +++ b/src/main/sync/engine.ts @@ -1,6 +1,8 @@ import { v4 as uuidv4 } from 'uuid'; import Database from 'better-sqlite3-multiple-ciphers'; +const TOMBSTONE_TTL_SECONDS = 90 * 24 * 3600; + export interface SyncResult { success: boolean; @@ -75,14 +77,13 @@ export function syncProject( const stmtMembershipSynced = localDb.prepare('UPDATE project_memberships SET synced_at = ? WHERE id = ?'); const stmtMentionSynced = localDb.prepare('UPDATE contact_alert_mentions SET synced_at = ? WHERE id = ?'); const stmtLogEntrySynced = localDb.prepare('UPDATE interaction_log_entries SET synced_at = ? WHERE id = ?'); - const TOMBSTONE_TTL = 90 * 24 * 3600; localDb.transaction(() => { for (const id of pushedContactIds!) stmtContactSynced.run(now, id); for (const id of pushedMembershipIds!) stmtMembershipSynced.run(now, id); for (const id of pushedMentionIds!) stmtMentionSynced.run(now, id); for (const id of pushedLogEntryIds!) stmtLogEntrySynced.run(now, id); localDb.prepare('UPDATE projects SET shared_pending_writes = 0, last_synced_at = ? WHERE id = ?').run(now, projectId); - localDb.prepare('DELETE FROM sync_tombstones WHERE deleted_at < ?').run(now - TOMBSTONE_TTL); + localDb.prepare('DELETE FROM sync_tombstones WHERE deleted_at < ?').run(now - TOMBSTONE_TTL_SECONDS); })(); return { success: true }; @@ -120,7 +121,7 @@ function pushTombstones(local: Database.Database, shared: Database.Database, now ON CONFLICT(table_name, row_id) DO UPDATE SET deleted_at = MAX(deleted_at, excluded.deleted_at) `); for (const r of rows) upsert.run(r.table_name, r.row_id, r.deleted_at); - shared.prepare('DELETE FROM sync_tombstones WHERE deleted_at < ?').run(now - 90 * 24 * 3600); + shared.prepare('DELETE FROM sync_tombstones WHERE deleted_at < ?').run(now - TOMBSTONE_TTL_SECONDS); } function loadLocalTombstones(local: Database.Database): Map> { @@ -321,12 +322,13 @@ function mergeSubTablesFromShared( .prepare('SELECT * FROM contact_emails WHERE contact_id = ? ORDER BY sort_order') .all(contactId) as { id: string; email: string; label: string | null; sort_order: number; created_at: number }[]; - const sharedEmailValues = new Set(sharedEmails.map((e) => e.email)); const emailTombstones = tombstones.get('contact_emails') ?? new Set(); + const filteredSharedEmails = sharedEmails.filter((e) => !emailTombstones.has(e.id)); + const sharedEmailValues = new Set(filteredSharedEmails.map((e) => e.email)); const localOnlyEmails = localEmails.filter((e) => !sharedEmailValues.has(e.email) && !emailTombstones.has(e.id)); // Merge and sort by insertion time so rows appear in the order they were added. - const mergedEmails = [...sharedEmails, ...localOnlyEmails].sort((a, b) => a.created_at - b.created_at); + const mergedEmails = [...filteredSharedEmails, ...localOnlyEmails].sort((a, b) => a.created_at - b.created_at); local.prepare('DELETE FROM contact_emails WHERE contact_id = ?').run(contactId); mergedEmails.forEach((e, i) => { @@ -344,11 +346,12 @@ function mergeSubTablesFromShared( .prepare('SELECT * FROM contact_phones WHERE contact_id = ? ORDER BY sort_order') .all(contactId) as { id: string; phone: string; label: string | null; sort_order: number; created_at: number }[]; - const sharedPhoneValues = new Set(sharedPhones.map((p) => p.phone)); const phoneTombstones = tombstones.get('contact_phones') ?? new Set(); + const filteredSharedPhones = sharedPhones.filter((p) => !phoneTombstones.has(p.id)); + const sharedPhoneValues = new Set(filteredSharedPhones.map((p) => p.phone)); const localOnlyPhones = localPhones.filter((p) => !sharedPhoneValues.has(p.phone) && !phoneTombstones.has(p.id)); - const mergedPhones = [...sharedPhones, ...localOnlyPhones].sort((a, b) => a.created_at - b.created_at); + const mergedPhones = [...filteredSharedPhones, ...localOnlyPhones].sort((a, b) => a.created_at - b.created_at); local.prepare('DELETE FROM contact_phones WHERE contact_id = ?').run(contactId); mergedPhones.forEach((p, i) => { @@ -366,14 +369,15 @@ function mergeSubTablesFromShared( .prepare('SELECT id, type, label, url, wayback_url, sort_order, created_at FROM contact_links WHERE contact_id = ? ORDER BY sort_order') .all(contactId) as { id: string; type: string; label: string | null; url: string; wayback_url: string | null; sort_order: number; created_at: number }[]; - const sharedUrlValues = new Set(sharedLinks.map((l) => l.url.trim())); const localWaybacks = new Map(localLinks.filter((l) => l.wayback_url).map((l) => [l.url.trim(), l.wayback_url])); const linkTombstones = tombstones.get('contact_links') ?? new Set(); + const filteredSharedLinks = sharedLinks.filter((l) => !linkTombstones.has(l.id)); + const sharedUrlValues = new Set(filteredSharedLinks.map((l) => l.url.trim())); const localOnlyLinks = localLinks.filter((l) => !sharedUrlValues.has(l.url.trim()) && !linkTombstones.has(l.id)); // Build merged set: shared rows (with wayback_url restored) + local-only rows, sorted by created_at. const mergedLinks = [ - ...sharedLinks.map((l) => ({ ...l, wayback_url: localWaybacks.get(l.url.trim()) ?? null })), + ...filteredSharedLinks.map((l) => ({ ...l, wayback_url: localWaybacks.get(l.url.trim()) ?? null })), ...localOnlyLinks, ].sort((a, b) => a.created_at - b.created_at); @@ -392,11 +396,12 @@ function mergeSubTablesFromShared( .prepare('SELECT * FROM contact_handles WHERE contact_id = ? ORDER BY sort_order') .all(contactId) as { id: string; type: string; handle: string; sort_order: number; created_at: number }[]; - const sharedHandleKeys = new Set(sharedHandles.map((h) => `${h.type}:${h.handle}`)); const handleTombstones = tombstones.get('contact_handles') ?? new Set(); + const filteredSharedHandles = sharedHandles.filter((h) => !handleTombstones.has(h.id)); + const sharedHandleKeys = new Set(filteredSharedHandles.map((h) => `${h.type}:${h.handle}`)); const localOnlyHandles = localHandles.filter((h) => !sharedHandleKeys.has(`${h.type}:${h.handle}`) && !handleTombstones.has(h.id)); - const mergedHandles = [...sharedHandles, ...localOnlyHandles].sort((a, b) => a.created_at - b.created_at); + const mergedHandles = [...filteredSharedHandles, ...localOnlyHandles].sort((a, b) => a.created_at - b.created_at); local.prepare('DELETE FROM contact_handles WHERE contact_id = ?').run(contactId); mergedHandles.forEach((h, i) => { @@ -436,10 +441,11 @@ function mergeSubTablesFromShared( .prepare('SELECT * FROM contact_tags WHERE contact_id = ?') .all(contactId) as { id: string; tag: string; created_at: number }[]; - const sharedTagValues = new Set(sharedTags.map((t) => t.tag)); const tagTombstones = tombstones.get('contact_tags') ?? new Set(); + const filteredSharedTags = sharedTags.filter((t) => !tagTombstones.has(t.id)); + const sharedTagValues = new Set(filteredSharedTags.map((t) => t.tag)); const localOnlyTags = localTags.filter((t) => !sharedTagValues.has(t.tag) && !tagTombstones.has(t.id)); - const mergedTags = [...sharedTags, ...localOnlyTags]; + const mergedTags = [...filteredSharedTags, ...localOnlyTags]; local.prepare('DELETE FROM contact_tags WHERE contact_id = ?').run(contactId); const insertLocalTag = local.prepare('INSERT OR IGNORE INTO contact_tags (id, contact_id, tag, created_at) VALUES (?, ?, ?, ?)'); @@ -769,7 +775,7 @@ function pushSubTablesToShared( const tagRows = local .prepare('SELECT * FROM contact_tags WHERE contact_id = ?') .all(contactId) as { id: string; tag: string; created_at: number }[]; - const insertSharedTag = shared.prepare('INSERT OR IGNORE INTO contact_tags (id, contact_id, tag, created_at) VALUES (?, ?, ?, ?)'); + const insertSharedTag = shared.prepare('INSERT INTO contact_tags (id, contact_id, tag, created_at) VALUES (?, ?, ?, ?)'); for (const t of tagRows) { insertSharedTag.run(t.id, contactId, t.tag, t.created_at); } diff --git a/src/test/contacts.test.ts b/src/test/contacts.test.ts index 494de71..6f584cb 100644 --- a/src/test/contacts.test.ts +++ b/src/test/contacts.test.ts @@ -196,6 +196,39 @@ describe('contacts:update', () => { const row = testDb.prepare('SELECT updated_at FROM contacts WHERE id = ?').get(id) as { updated_at: number }; expect(row.updated_at).toBeGreaterThan(staleTs); }); + + it('writes tombstones for deleted emails, phones, links, and handles', async () => { + const { v4: uuidv4 } = await import('uuid'); + const now = Math.floor(Date.now() / 1000); + const id = insertContact(testDb, 'Alice Smith'); + + // Pre-insert rows with known IDs so we can verify the tombstone references them + const emailId = uuidv4(); + const phoneId = uuidv4(); + const linkId = uuidv4(); + const handleId = uuidv4(); + testDb.prepare('DELETE FROM contact_emails WHERE contact_id = ?').run(id); + testDb.prepare('INSERT INTO contact_emails (id, contact_id, email, sort_order, created_at) VALUES (?, ?, ?, 0, ?)').run(emailId, id, 'alice@example.com', now); + testDb.prepare('INSERT INTO contact_phones (id, contact_id, phone, sort_order, created_at) VALUES (?, ?, ?, 0, ?)').run(phoneId, id, '+15550001111', now); + testDb.prepare('INSERT INTO contact_links (id, contact_id, type, url, sort_order, created_at) VALUES (?, ?, ?, ?, 0, ?)').run(linkId, id, 'website', 'https://alice.com', now); + testDb.prepare('INSERT INTO contact_handles (id, contact_id, type, handle, sort_order, created_at) VALUES (?, ?, ?, ?, 0, ?)').run(handleId, id, 'twitter', '@alice', now); + + // Update the contact, omitting all sub-table rows + await handlers.get('contacts:update')!({}, { + id, + name: 'Alice Smith', + emails: [], + phones: [], + links: [], + handles: [], + }); + + const tombstoneRowIds = (testDb.prepare('SELECT row_id FROM sync_tombstones').all() as { row_id: string }[]).map((r) => r.row_id); + expect(tombstoneRowIds).toContain(emailId); + expect(tombstoneRowIds).toContain(phoneId); + expect(tombstoneRowIds).toContain(linkId); + expect(tombstoneRowIds).toContain(handleId); + }); }); // --------------------------------------------------------------------------- diff --git a/src/test/sync-engine.test.ts b/src/test/sync-engine.test.ts index 99f8dfc..0d19bd2 100644 --- a/src/test/sync-engine.test.ts +++ b/src/test/sync-engine.test.ts @@ -943,6 +943,48 @@ describe('sync_tombstones — propagation and resurrection prevention', () => { expect((localB.prepare('SELECT tag FROM contact_tags WHERE contact_id = ?').all(contactId) as { tag: string }[]).map((r) => r.tag)).not.toContain('source'); }); + it('shared row with a tombstoned ID is not resurrected (concurrent-edit order)', () => { + // Scenario: A deletes email-1, B edits name (pushing email-1 back to shared with same ID), + // then A syncs — email-1 must not come back from sharedEmails. + const sharedDb = createSharedDb(); + const projectId = uuidv4(); + const contactId = uuidv4(); + const emailId = uuidv4(); + const T0 = NOW - 200; + + const localA = createTestDb(); + const localB = createTestDb(); + for (const db of [localA, localB]) { + db.prepare('INSERT INTO projects (id, name, is_shared, created_at) VALUES (?, ?, 1, ?)').run(projectId, 'Shared', T0); + db.prepare('INSERT INTO contacts (id, name, created_at, updated_at, synced_at) VALUES (?, ?, ?, ?, ?)').run(contactId, 'Alice', T0, T0, T0); + db.prepare('INSERT INTO project_memberships (id, contact_id, project_id, reporter_email, reporter_name, created_at, updated_at, synced_at) VALUES (?, ?, ?, ?, ?, ?, ?, ?)').run(uuidv4(), contactId, projectId, 'r@r.com', 'Reporter', T0, T0, T0); + db.prepare('INSERT INTO contact_emails (id, contact_id, email, sort_order, created_at) VALUES (?, ?, ?, 0, ?)').run(emailId, contactId, 'alice@wapo.com', T0); + } + sharedDb.prepare('INSERT INTO contacts (id, name, created_at, updated_at) VALUES (?, ?, ?, ?)').run(contactId, 'Alice', T0, T0); + sharedInsertMembership(sharedDb, uuidv4(), contactId); + sharedDb.prepare('INSERT INTO contact_emails (id, contact_id, email, sort_order, created_at) VALUES (?, ?, ?, 0, ?)').run(emailId, contactId, 'alice@wapo.com', T0); + + // B edits name at T1 and syncs — B's push uses same emailId (simulating no contacts:update re-generation) + const T1 = NOW - 10; + localB.prepare('UPDATE contacts SET name = ?, updated_at = ? WHERE id = ?').run('Alice B', T1, contactId); + expect(syncProject(localB, sharedDb, projectId).success).toBe(true); + // Shared should still have email-1 + expect(sharedDb.prepare('SELECT id FROM contact_emails WHERE id = ?').get(emailId)).toBeDefined(); + + // A deletes alice@wapo.com at T2 > T1 and writes tombstone + const T2 = NOW; + localA.prepare('DELETE FROM contact_emails WHERE contact_id = ?').run(contactId); + localA.prepare('UPDATE contacts SET updated_at = ? WHERE id = ?').run(T2, contactId); + localA.prepare('INSERT INTO sync_tombstones (table_name, row_id, deleted_at) VALUES (?, ?, ?)').run('contact_emails', emailId, T2); + + // A syncs: A is newer (T2 > T1), so A pushes. B's name update also comes in. + // The tombstoned emailId must not be resurrected from sharedEmails. + expect(syncProject(localA, sharedDb, projectId).success).toBe(true); + + const emails = (localA.prepare('SELECT email FROM contact_emails WHERE contact_id = ?').all(contactId) as { email: string }[]).map((r) => r.email); + expect(emails).not.toContain('alice@wapo.com'); + }); + it('tombstoned row does not block a fresh re-add of the same email address', () => { const localDb = createTestDb(); const sharedDb = createSharedDb(); From 989fc2c9c08135f225a807cb8c0b63efe49a641f Mon Sep 17 00:00:00 2001 From: Tom Cardoso Date: Fri, 5 Jun 2026 23:44:02 -0400 Subject: [PATCH 4/4] Add tests for tombstone GC, kept-row safety, and UPSERT correctness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - contacts:update: kept sub-table rows must not be tombstoned — regression guard for the snapshot-diff logic (only deleted IDs should appear in sync_tombstones, not surviving ones) - GC: tombstones older than 90 days are pruned from both local (phase-4 transaction) and shared (pushTombstones); fresh tombstones survive - UPSERT MAX(deleted_at): when the same row_id arrives from shared with a newer deleted_at, pullTombstones updates the local entry rather than keeping the stale timestamp Co-Authored-By: Claude Sonnet 4.6 --- src/test/contacts.test.ts | 25 +++++++++++++++ src/test/sync-engine.test.ts | 61 ++++++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+) diff --git a/src/test/contacts.test.ts b/src/test/contacts.test.ts index 6f584cb..434dc25 100644 --- a/src/test/contacts.test.ts +++ b/src/test/contacts.test.ts @@ -229,6 +229,31 @@ describe('contacts:update', () => { expect(tombstoneRowIds).toContain(linkId); expect(tombstoneRowIds).toContain(handleId); }); + + it('does not tombstone sub-table rows that are kept on update', async () => { + const { v4: uuidv4 } = await import('uuid'); + const now = Math.floor(Date.now() / 1000); + const id = insertContact(testDb, 'Alice Smith'); + + const keptEmailId = uuidv4(); + const removedEmailId = uuidv4(); + testDb.prepare('DELETE FROM contact_emails WHERE contact_id = ?').run(id); + testDb.prepare('INSERT INTO contact_emails (id, contact_id, email, sort_order, created_at) VALUES (?, ?, ?, 0, ?)').run(keptEmailId, id, 'keep@example.com', now); + testDb.prepare('INSERT INTO contact_emails (id, contact_id, email, sort_order, created_at) VALUES (?, ?, ?, 1, ?)').run(removedEmailId, id, 'remove@example.com', now); + + await handlers.get('contacts:update')!({}, { + id, + name: 'Alice Smith', + emails: [{ email: 'keep@example.com', label: null }], + phones: [], + links: [], + handles: [], + }); + + const tombstoneRowIds = (testDb.prepare('SELECT row_id FROM sync_tombstones').all() as { row_id: string }[]).map((r) => r.row_id); + expect(tombstoneRowIds).toContain(removedEmailId); + expect(tombstoneRowIds).not.toContain(keptEmailId); + }); }); // --------------------------------------------------------------------------- diff --git a/src/test/sync-engine.test.ts b/src/test/sync-engine.test.ts index 0d19bd2..30a673d 100644 --- a/src/test/sync-engine.test.ts +++ b/src/test/sync-engine.test.ts @@ -1013,4 +1013,65 @@ describe('sync_tombstones — propagation and resurrection prevention', () => { const emails = (localDb.prepare('SELECT email FROM contact_emails WHERE contact_id = ?').all(contactId) as { email: string }[]).map((r) => r.email); expect(emails).toContain('alice@example.com'); }); + + it('GC removes tombstones older than 90 days from local and shared', () => { + const localDb = createTestDb(); + const sharedDb = createSharedDb(); + const projectId = insertProject(localDb, 'GC Test'); + + const contactId = insertContact(localDb, 'Alice'); + localInsertMembership(localDb, contactId, projectId); + sharedInsertContact(sharedDb, contactId, 'Alice'); + sharedInsertMembership(sharedDb, uuidv4(), contactId); + + const NINETY_ONE_DAYS = 91 * 24 * 3600; + const staleRowId = uuidv4(); + const freshRowId = uuidv4(); + + // Stale tombstone (> 90 days old) in both DBs + localDb.prepare('INSERT INTO sync_tombstones (table_name, row_id, deleted_at) VALUES (?, ?, ?)').run('contact_tags', staleRowId, NOW - NINETY_ONE_DAYS); + sharedDb.prepare('INSERT INTO sync_tombstones (table_name, row_id, deleted_at) VALUES (?, ?, ?)').run('contact_tags', staleRowId, NOW - NINETY_ONE_DAYS); + + // Fresh tombstone (recent) in local — should survive + localDb.prepare('INSERT INTO sync_tombstones (table_name, row_id, deleted_at) VALUES (?, ?, ?)').run('contact_tags', freshRowId, NOW); + + syncProject(localDb, sharedDb, projectId); + + // Stale tombstone pruned from local (phase 4 GC) + expect(localDb.prepare('SELECT row_id FROM sync_tombstones WHERE row_id = ?').get(staleRowId)).toBeUndefined(); + // Stale tombstone pruned from shared (pushTombstones GC) + expect(sharedDb.prepare('SELECT row_id FROM sync_tombstones WHERE row_id = ?').get(staleRowId)).toBeUndefined(); + // Fresh tombstone survives on local + expect(localDb.prepare('SELECT row_id FROM sync_tombstones WHERE row_id = ?').get(freshRowId)).toBeDefined(); + }); + + it('UPSERT keeps the larger deleted_at when the same row_id arrives with a newer timestamp', () => { + const localDb = createTestDb(); + const sharedDb = createSharedDb(); + const projectId = insertProject(localDb, 'UPSERT Test'); + + const contactId = insertContact(localDb, 'Alice'); + localInsertMembership(localDb, contactId, projectId); + sharedInsertContact(sharedDb, contactId, 'Alice'); + sharedInsertMembership(sharedDb, uuidv4(), contactId); + + const rowId = uuidv4(); + const olderTs = NOW - 100; + const newerTs = NOW - 10; + + // Local has the older timestamp + localDb.prepare('INSERT INTO sync_tombstones (table_name, row_id, deleted_at) VALUES (?, ?, ?)').run('contact_tags', rowId, olderTs); + // Shared has a newer timestamp for the same row_id (simulates a late-syncing client) + sharedDb.prepare('INSERT INTO sync_tombstones (table_name, row_id, deleted_at) VALUES (?, ?, ?)').run('contact_tags', rowId, newerTs); + + syncProject(localDb, sharedDb, projectId); + + // After pullTombstones, local should have the newer deleted_at + const local = localDb.prepare('SELECT deleted_at FROM sync_tombstones WHERE row_id = ?').get(rowId) as { deleted_at: number } | undefined; + expect(local?.deleted_at).toBe(newerTs); + + // After pushTombstones, shared should also have the newer deleted_at (it already did; upsert is idempotent) + const shared = sharedDb.prepare('SELECT deleted_at FROM sync_tombstones WHERE row_id = ?').get(rowId) as { deleted_at: number } | undefined; + expect(shared?.deleted_at).toBe(newerTs); + }); });