fix: entry deletion tombstone creation for cross-device sync by jamiepine · Pull Request #3014 · spacedriveapp/spacedrive

jamiepine · 2026-02-06T09:49:57Z

Summary

Entry deletions were not creating tombstones, so deletions never synced to peer devices. The tombstone infrastructure (protocol, querying, receiving) was fully implemented but the entry sending side was broken.
Adds create_tombstone() to the Syncable trait as a unified API, with a generic ConnectionTrait parameter so it works inside transactions.
Adds tombstone creation to the entry deletion path in DatabaseAdapter, with device_db_id cached at construction to avoid per-deletion queries.
Refactors location/volume managers to use the unified API instead of manual tombstone insertion.
Adds sync_deletion_test.rs with 6 test cases covering location, volume, and entry deletion sync, cascading folder deletion, and tombstone race condition protection.

Test plan

Run cargo test -p sd-core --test sync_deletion_test -- --test-threads=1 --nocapture to verify all 6 deletion sync tests pass
Run cargo test -p sd-core to verify no regressions in existing tests
Run cargo clippy to verify no new warnings
Manual test: index a location on device A, sync to device B, delete a file on device A, verify it disappears on device B after incremental sync

Made with Cursor

Entry deletions were not creating tombstones in device_state_tombstones, so deletions never synced to peer devices. Location and volume deletions worked because their managers manually created tombstones, but entries (the most common case) were missing this entirely. Changes: - Add create_tombstone() to Syncable trait with generic ConnectionTrait parameter so it works inside transactions - Add tombstone creation to the entry deletion path in DatabaseAdapter, with device_db_id cached at construction to avoid per-deletion queries - Refactor location/volume managers to use the unified create_tombstone API instead of manual tombstone insertion - Add deletion-specific test helpers (wait_for_deletion_sync, count_tombstones, create_entry_tombstone) - Add sync_deletion_test.rs with 6 test cases: location deletion sync, volume deletion sync, entry tombstone creation, entry deletion sync, cascading folder deletion, and tombstone race condition protection Co-authored-by: Cursor <[email protected]>

cursor · 2026-02-06T09:50:03Z

PR Summary

Medium Risk
Touches deletion/sync semantics and introduces new DB writes during indexing-driven deletes, which could affect data correctness if tombstones are created with the wrong device/model identifiers. Changes are localized and covered by new end-to-end deletion sync tests, reducing regression risk.

Overview
Fixes cross-device deletion sync by ensuring local entry deletions emit a deletion tombstone (previously missing, so peers never applied deletes).

Adds a unified Syncable::create_tombstone helper that works with both connections and transactions (ConnectionTrait), then refactors location/volume deletion paths to use it so record deletion and tombstone creation are atomic.

Updates the persistent indexing DatabaseAdapter delete path to create a tombstone for the root entry UUID (cascading deletion model) and caches device_db_id during adapter construction to avoid per-delete lookups, plus adds a new sync_deletion_test.rs suite and helper utilities to validate location/volume/entry deletion sync, cascade behavior, and tombstone race-condition protection.

^{Written by Cursor Bugbot for commit 8f4daab. Configure here.}

tembo · 2026-02-06T09:54:45Z

core/src/infra/sync/syncable.rs

+			)
+			.exec(db)
+			.await
+			.ok(); // Ignore duplicate conflict (idempotent)


Swallowing all DB errors here makes it hard to detect real failures (disk full, schema mismatch, etc). With on_conflict(...).do_nothing(), duplicates are already idempotent, so it’s safe to just propagate errors.

Suggested change

.ok(); // Ignore duplicate conflict (idempotent)

.exec(db)

.await?;

Ok(())

tembo · 2026-02-06T09:54:52Z

core/src/ops/indexing/change_detection/persistent.rs

 		};

+		// Create tombstone for root entry only (cascading design)
+		// This must happen before database deletion for atomicity


Minor nit: this comment says “atomicity”, but create_tombstone(..., &self.db) runs outside the transaction that actually deletes the rows (let txn = self.db.begin() below). If the intent is “tombstone insert + deletes commit together”, consider moving the tombstone insert to after let txn = ... and passing &txn instead (you already made create_tombstone accept ConnectionTrait for this).

tembo · 2026-02-06T09:54:58Z

core/src/location/manager.rs

+		use crate::infra::sync::Syncable;
+		entities::location::Model::create_tombstone(location_id, location.device_id, &txn)
+			.await
+			.map_err(|e| LocationError::Other(format!("Failed to create tombstone: {}", e)))?;


Since this is a DB error, it might be nicer to keep it in a DB-flavored variant (so callers can pattern-match / aggregate) while still preserving context.

Suggested change

.map_err(|e| LocationError::Other(format!("Failed to create tombstone: {}", e)))?;

.map_err(|e| LocationError::DatabaseError(format!("Failed to create tombstone: {e}")))?;

tembo · 2026-02-06T09:55:05Z

core/tests/sync_deletion_test.rs

+		.parent()
+		.unwrap()
+		.join("docs");
+	let device_record = entities::device::Entity::find()


This query is a bit fragile once you register Bob (Alice’s DB now has multiple devices). Filtering by UUID makes it deterministic and keeps the test future-proof.

Suggested change

let device_record = entities::device::Entity::find()

let device_record = entities::device::Entity::find()

.filter(entities::device::Column::Uuid.eq(device_alice_id))

.one(library_alice.db().conn())

.await?

.ok_or_else(|| anyhow::anyhow!("Device not found"))?;

tembo · 2026-02-06T09:55:16Z

core/tests/sync_deletion_test.rs

+		.unwrap()
+		.join("crates")
+		.join("sdk");
+	let device_record = entities::device::Entity::find()


Same thing here: .find().one() can become nondeterministic if the test ever ends up with multiple devices in the DB. Filtering by device_id keeps it stable.

Suggested change

let device_record = entities::device::Entity::find()

let device_record = entities::device::Entity::find()

.filter(entities::device::Column::Uuid.eq(device_id))

.one(library.db().conn())

.await?

.ok_or_else(|| anyhow::anyhow!("Device not found"))?;

cursor

Cursor Bugbot has reviewed your changes and found 2 potential issues.

^{Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.}

cursor · 2026-02-06T09:56:24Z

core/src/infra/sync/syncable.rs

+			.exec(db)
+			.await
+			.ok(); // Ignore duplicate conflict (idempotent)
+		Ok(())


Tombstone creation silently swallows all database errors

High Severity

The create_tombstone method uses .ok() to discard the Result from exec(), then unconditionally returns Ok(()). This swallows all database errors, not just the expected conflict case. All three callers (location/manager.rs, volume/manager.rs, persistent.rs) have error handling with .map_err()? that is now dead code. If tombstone creation fails due to a connection error or other database issue, the deletion will proceed without a tombstone, causing deletions to not sync to peer devices—the exact problem this PR intends to fix. The original code properly propagated errors with ?.

Additional Locations (2)

core/src/location/manager.rs#L586-L589

core/src/volume/manager.rs#L1589-L1592

cursor · 2026-02-06T09:56:24Z

core/src/ops/indexing/change_detection/persistent.rs

+			entities::entry::Model::create_tombstone(root_uuid, self.device_db_id, &self.db)
+				.await
+				.map_err(|e| anyhow::anyhow!("Failed to create tombstone: {}", e))?;
+		}


Entry tombstone creation lacks transactional atomicity with deletion

Medium Severity

The tombstone creation uses &self.db (a raw DatabaseConnection) while the comment claims "for atomicity". The deletion transaction starts on line 401 with self.db.begin() - a separate transaction. If the tombstone commits but the deletion transaction fails/rolls back, peer devices will delete their copies (via the tombstone) while the local device retains the entry, causing cross-device data inconsistency. The location/manager.rs and volume/manager.rs implementations correctly pass &txn to create_tombstone to achieve actual atomicity.

Added a new step to the setup-system action for Windows that configures the environment for LLVM. This includes setting the LIBCLANG_PATH, CC, and CXX variables, and modifying the PATH to prevent CMake from using Clang instead of MSVC's cl.exe. This change enhances compatibility for builds on Windows systems.

tembo bot reviewed Feb 6, 2026

View reviewed changes

cursor bot reviewed Feb 6, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: entry deletion tombstone creation for cross-device sync#3014

fix: entry deletion tombstone creation for cross-device sync#3014
jamiepine wants to merge 2 commits intomainfrom
fix/deletion-sync-tombstones

jamiepine commented Feb 6, 2026

Uh oh!

cursor bot commented Feb 6, 2026 •

edited

Loading

Uh oh!

tembo bot Feb 6, 2026

Uh oh!

tembo bot Feb 6, 2026

Uh oh!

tembo bot Feb 6, 2026

Uh oh!

tembo bot Feb 6, 2026

Uh oh!

tembo bot Feb 6, 2026

Uh oh!

cursor bot left a comment

Uh oh!

cursor bot Feb 6, 2026

Uh oh!

cursor bot Feb 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

	.map_err(\|e\| LocationError::Other(format!("Failed to create tombstone: {}", e)))?;
	.map_err(\|e\| LocationError::DatabaseError(format!("Failed to create tombstone: {e}")))?;

-	let device_record = entities::device::Entity::find()
+	let device_record = entities::device::Entity::find()
+		.filter(entities::device::Column::Uuid.eq(device_alice_id))
+		.one(library_alice.db().conn())
+		.await?
+		.ok_or_else(|| anyhow::anyhow!("Device not found"))?;

Uh oh!

Conversation

jamiepine commented Feb 6, 2026

Summary

Test plan

Uh oh!

cursor bot commented Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Uh oh!

tembo bot Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

tembo bot Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

tembo bot Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

tembo bot Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

tembo bot Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

cursor bot left a comment

Choose a reason for hiding this comment

Uh oh!

cursor bot Feb 6, 2026

Choose a reason for hiding this comment

Tombstone creation silently swallows all database errors

Uh oh!

cursor bot Feb 6, 2026

Choose a reason for hiding this comment

Entry tombstone creation lacks transactional atomicity with deletion

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

cursor bot commented Feb 6, 2026 •

edited

Loading