Add sync tombstones to propagate sub-table deletions across clients#424
Conversation
…ents 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 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces deletion tombstones for contact sub-tables (emails/phones/links/handles/tags) so that explicit deletions propagate across clients and aren’t resurrected by the additive pull-merge strategy. It also fixes tag syncing to use full replace semantics (DELETE + INSERT) like the other sub-tables, and adds tests covering tombstone push/pull and resurrection prevention.
Changes:
- Add
sync_tombstonestable to both local and shared schemas, plus basic TTL garbage collection. - Write tombstones on sub-table deletions (notably
contacts:updatefor sub-tables andcontacts:remove-tagfor tags). - Sync engine now pulls/pushes tombstones and uses them to filter tombstoned local-only rows during sub-table merges; tag push is changed to DELETE+INSERT.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/tags.test.ts | Adds tests verifying tombstones are written (or not) for tag removals. |
| src/test/sync-engine.test.ts | Adds sync tests for tombstone push/pull and resurrection prevention scenarios. |
| src/main/sync/engine.ts | Adds tombstone pull/push + GC and filters local-only merge rows by tombstones; fixes tag push semantics. |
| src/main/ipc/contacts.ts | Writes tombstones for deleted sub-table rows in contacts:update and contacts:remove-tag. |
| src/main/database/shared-schema.ts | Adds sync_tombstones table + index to shared schema SQL. |
| src/main/database/shared-db.ts | Shared DB migration scaffolding area touched; needs tombstone migration behavior for existing DBs. |
| src/main/database/schema.ts | Adds sync_tombstones table + index to local schema SQL. |
| src/main/database/index.ts | Local migrations comment updated; needs an actual migration for existing DBs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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); | ||
| } |
| .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 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); |
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 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // 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. |
There was a problem hiding this comment.
Missing migration block — this will break existing databases.
sync_tombstones is added to LOCAL_SCHEMA_DDL_SQL, but that SQL only runs for brand-new databases via initDatabase. Existing users whose user_version is already at DB_VERSION will hit the return guard above and skip this function entirely, so their databases will never get the table. Any call to INSERT INTO sync_tombstones or SELECT FROM sync_tombstones will throw no such table: sync_tombstones at runtime.
DB_VERSION needs to be bumped and a migration block needs to be added here:
| // No migration blocks yet — new DBs receive all schema via initDatabase → LOCAL_SCHEMA_DDL_SQL. | |
| // Migration to DB_VERSION N: add sync_tombstones for deletion propagation. | |
| if (version < /* new DB_VERSION */) { | |
| db.transaction(() => { | |
| db.prepare(` | |
| CREATE TABLE IF NOT EXISTS sync_tombstones ( | |
| table_name TEXT NOT NULL, | |
| row_id TEXT NOT NULL, | |
| deleted_at INTEGER NOT NULL, | |
| PRIMARY KEY (table_name, row_id) | |
| ) | |
| `).run(); | |
| db.pragma(`user_version = /* new DB_VERSION */`); | |
| })(); | |
| } |
The DB_VERSION constant (wherever it's defined) must also be incremented to match.
| @@ -85,16 +85,6 @@ function runSharedMigrations(db: Database.Database): void { | |||
| if (version < 1) { | |||
| db.pragma('user_version = 1'); | |||
| } | |||
There was a problem hiding this comment.
Missing migration for the shared DB. Same issue as the local DB: sync_tombstones was added to SHARED_SCHEMA_SQL, but that only runs when a new shared DB is first created. Existing shared databases will never get the table, causing runtime crashes for any client that tries to sync tombstones.
A new if (version < 2) block is needed here (and the shared user_version must be bumped to match):
| } | |
| if (version < 2) { | |
| db.transaction(() => { | |
| db.prepare(` | |
| CREATE TABLE IF NOT EXISTS sync_tombstones ( | |
| table_name TEXT NOT NULL, | |
| row_id TEXT NOT NULL, | |
| deleted_at INTEGER NOT NULL, | |
| PRIMARY KEY (table_name, row_id) | |
| ) | |
| `).run(); | |
| db.pragma('user_version = 2'); | |
| })(); | |
| } |
Note: the example migration template that was deleted from this file in this PR contained an important warning about using INSERT OR REPLACE instead of ON CONFLICT … WHERE value > due to SQLite's lexicographic comparison for min_app_version. Worth retaining that comment somewhere if it's no longer inline here.
| 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<string>(); | ||
| const localOnlyEmails = localEmails.filter((e) => !sharedEmailValues.has(e.email) && !emailTombstones.has(e.id)); |
There was a problem hiding this comment.
Tombstone filtering only covers local-only rows — shared rows aren't checked.
This filter correctly prevents a local deletion from being resurrected by a local-only row, but it doesn't protect against the concurrent-edit resurrection case:
- Client A and B both have
alice@example.com(IDemail-1), synced. - A deletes it and writes a tombstone for
email-1. - Before A syncs, B edits the contact (name change) and syncs — B's push re-inserts
alice@example.cominto shared. - A syncs:
pullTombstonesbrings in nothing new (no shared tombstone), thenmergeSubTablesFromSharedfindsalice@example.cominsharedEmails— not inlocalOnlyEmails. The tombstone filter never fires. Email resurrected.
The fix is to also filter sharedEmails against local tombstones:
| const localOnlyEmails = localEmails.filter((e) => !sharedEmailValues.has(e.email) && !emailTombstones.has(e.id)); | |
| const emailTombstones = tombstones.get('contact_emails') ?? new Set<string>(); | |
| const filteredSharedEmails = sharedEmails.filter((e) => !emailTombstones.has(e.id)); | |
| const localOnlyEmails = localEmails.filter((e) => !sharedEmailValues.has(e.email) && !emailTombstones.has(e.id)); |
and use filteredSharedEmails in place of sharedEmails in mergedEmails. The same pattern applies to phones, links, handles, and tags below. The same fix is needed in the test — the resurrection test only exercises the case where A syncs before B, not the concurrent-edit order.
| 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); |
There was a problem hiding this comment.
TTL magic number duplicated. 90 * 24 * 3600 also appears as TOMBSTONE_TTL in syncProject (for the local GC). Extract a module-level constant so both GC sites stay in sync:
| for (const r of rows) upsert.run(r.table_name, r.row_id, r.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 - TOMBSTONE_TTL_SECONDS); |
With const TOMBSTONE_TTL_SECONDS = 90 * 24 * 3600; at the top of the file, replacing the inline constant in syncProject as well.
| row_id TEXT NOT NULL, | ||
| deleted_at INTEGER NOT NULL, | ||
| PRIMARY KEY (table_name, row_id) | ||
| ); |
There was a problem hiding this comment.
Consider adding an index on deleted_at to keep the GC query fast as tombstones accumulate:
| ); | |
| ); | |
| CREATE INDEX IF NOT EXISTS idx_sync_tombstones_deleted_at ON sync_tombstones(deleted_at); |
DELETE FROM sync_tombstones WHERE deleted_at < ? is a table scan without it. The same index should be added to SHARED_SCHEMA_SQL in shared-schema.ts.
| 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 (?, ?, ?, ?)'); |
There was a problem hiding this comment.
INSERT OR IGNORE is now redundant. The DELETE two lines above just wiped all tags for this contact, so there's nothing to conflict with. Can be simplified to plain INSERT:
| 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 (?, ?, ?, ?)'); |
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Tombstone sync (#422) | ||
| // --------------------------------------------------------------------------- |
There was a problem hiding this comment.
The tombstone tests for contacts:remove-tag are covered in tags.test.ts, but there are no tests for the tombstone writes in contacts:update for emails, phones, links, and handles. That path writes tombstones via the snapshot-before-delete approach in contacts.ts and is the primary deletion mechanism for those sub-tables. Worth adding at least one test (e.g. delete an email via contacts:update and verify sync_tombstones receives a row with table_name = 'contact_emails' and the correct row_id).
- 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 <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
Summary
sync_tombstonestable (both local and shared schemas) that records the row ID and table name whenever a sub-table row is explicitly deletedcontacts:remove-tagandcontacts:updatewrite tombstones for every deleted email/phone/link/handle/tag rowmergeSubTablesFromSharednow filters local-only rows against local tombstones so deletions from other clients aren't resurrectedWhat's new
Sync now respects explicit deletions. Previously, if you removed a tag (or phone, email, link, or handle) on one client and synced, the deleted item could silently reappear on your next sync from another client. This is fixed with a tombstone table: each deletion is recorded by row ID, and the sync engine uses those records to filter out resurrected rows during merges. Tombstones are shared across all clients and automatically cleaned up after 90 days.
Test plan
contacts:remove-tagCloses #422
🤖 Generated with Claude Code