Skip to content

Commit e4dffa0

Browse files
committed
fix(crdt): scope constraint checkpoints and snapshots by database
CRDT constraint checkpoint filenames and tombstone lookups were keyed by tenant and collection only, so a collection with the same name in a different database within the same tenant could collide. Include the database id in checkpoint filenames and replace the crdt_constraints snapshot tuple with a named CrdtConstraintEntry struct carrying the database id alongside tenant, collection, version, and constraints.
1 parent 435f3bb commit e4dffa0

10 files changed

Lines changed: 97 additions & 52 deletions

File tree

nodedb-cluster-tests/tests/cluster_collection_hard_delete.rs

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,14 @@ fn has_tombstone(node: &TestClusterNode, name: &str) -> bool {
3232
let cat = node.shared.credentials.catalog();
3333
cat.load_wal_tombstones()
3434
.map(|set| {
35-
set.iter()
36-
.any(|(tenant, n, lsn)| tenant == 1 && n == name && lsn > 0)
35+
// Tombstones are keyed by database as well as tenant; this helper
36+
// only ever asks about collections in the default database.
37+
set.iter().any(|(database, tenant, n, lsn)| {
38+
database == nodedb_types::DatabaseId::DEFAULT.as_u64()
39+
&& tenant == 1
40+
&& n == name
41+
&& lsn > 0
42+
})
3743
})
3844
.unwrap_or(false)
3945
}

nodedb-cluster-tests/tests/cluster_post_apply_follower_dispatch.rs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,13 +45,18 @@ fn pick_follower_index(cluster: &TestCluster) -> usize {
4545

4646
/// Tombstone tuples `(tenant_id, collection, purge_lsn)` currently
4747
/// persisted in this node's `_system.wal_tombstones`.
48+
///
49+
/// Tombstones are keyed by database as well as tenant; this test only
50+
/// creates and purges collections in the default database, so entries from
51+
/// any other database are filtered out rather than flattened away.
4852
fn follower_tombstones(node: &common::cluster_harness::TestClusterNode) -> Vec<(u64, String, u64)> {
4953
let catalog = node.shared.credentials.catalog();
5054
catalog
5155
.load_wal_tombstones()
5256
.expect("load_wal_tombstones")
5357
.iter()
54-
.map(|(t, n, l)| (t, n.to_string(), l))
58+
.filter(|(database, _, _, _)| *database == nodedb_types::DatabaseId::DEFAULT.as_u64())
59+
.map(|(_, t, n, l)| (t, n.to_string(), l))
5560
.collect()
5661
}
5762

nodedb-cluster-tests/tests/install_snapshot_crdt_constraints_cluster.rs

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -128,11 +128,12 @@ async fn crdt_constraint_survives_snapshot_capture_and_restore() {
128128
validator's installed constraint set"
129129
);
130130
assert!(
131-
snap.crdt_constraints
132-
.iter()
133-
.any(|(tid, coll, version, encoded)| {
134-
*tid == TENANT && coll == COLLECTION && *version == 1 && !encoded.is_empty()
135-
}),
131+
snap.crdt_constraints.iter().any(|entry| {
132+
entry.tenant_id == TENANT
133+
&& entry.collection == COLLECTION
134+
&& entry.version == 1
135+
&& !entry.constraints.is_empty()
136+
}),
136137
"captured snapshot did not contain an entry for (tenant={TENANT}, collection={COLLECTION}, \
137138
version=1) with at least one constraint blob; observed: {:?}",
138139
snap.crdt_constraints

nodedb/src/control/cluster/snapshot_builder.rs

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -225,11 +225,9 @@ impl DataPlaneSnapshotBuilder {
225225

226226
// CRDT constraints: same per-collection vshard filter as `crdt_state`
227227
// — each entry is routed by its single collection's vshard.
228-
for (database_id, tid, collection, version, encoded) in snap.crdt_constraints {
229-
if group_vshards.contains(&Self::vshard_of(&collection)) {
230-
merged
231-
.crdt_constraints
232-
.push((database_id, tid, collection, version, encoded));
228+
for entry in snap.crdt_constraints {
229+
if group_vshards.contains(&Self::vshard_of(&entry.collection)) {
230+
merged.crdt_constraints.push(entry);
233231
}
234232
}
235233

nodedb/src/data/executor/crdt_checkpoint.rs

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
//!
55
//! The matching write path lives in `handlers/control/checkpoint_crdt.rs`
66
//! (`checkpoint_crdt_engines`). Checkpoints are written per-core to
7-
//! `{data_dir}/crdt-ckpt/core-{core_id}/tenant-{tid}-coll-{hex(collection)}.ckpt` because
7+
//! `{data_dir}/crdt-ckpt/core-{core_id}/db-{dbid}-tenant-{tid}-coll-{hex(collection)}.ckpt` because
88
//! `data_dir` is shared across cores and each core only owns the CRDT
99
//! fragments routed to its vShards.
1010
@@ -21,7 +21,8 @@ pub(crate) fn crdt_ckpt_dir(data_dir: &std::path::Path, core_id: usize) -> std::
2121
data_dir.join("crdt-ckpt").join(format!("core-{core_id}"))
2222
}
2323

24-
/// Per-collection checkpoint filename: `tenant-{tid}-coll-{hex(collection)}.ckpt`.
24+
/// Per-collection checkpoint filename:
25+
/// `db-{dbid}-tenant-{tid}-coll-{hex(collection)}.ckpt`.
2526
///
2627
/// The collection is hex-encoded so the filename is filesystem-safe (collection
2728
/// names may contain `/`, `:` or `-`) and unambiguously parseable: hex contains
@@ -101,7 +102,8 @@ impl CoreLoop {
101102
continue;
102103
}
103104

104-
// Checkpoint filenames are `"tenant-{tid}-coll-{hex(collection)}.ckpt"`.
105+
// Checkpoint filenames are
106+
// `"db-{dbid}-tenant-{tid}-coll-{hex(collection)}.ckpt"`.
105107
let stem = path
106108
.file_stem()
107109
.and_then(|s| s.to_str())
@@ -199,7 +201,7 @@ mod tests {
199201
.expect("an absent checkpoint dir must not be treated as corruption");
200202
}
201203

202-
/// A `.ckpt` file with a valid, parseable `tenant-{tid}-coll-{hex}` stem
204+
/// A `.ckpt` file with a valid, parseable `db-{dbid}-tenant-{tid}-coll-{hex}` stem
203205
/// but bytes that are not a real Loro snapshot must fail the load, not be
204206
/// silently skipped: once the WAL below this checkpoint's LSN is
205207
/// truncated, the checkpoint is the only durable copy of the CRDT state,

nodedb/src/data/executor/handlers/control/checkpoint_crdt.rs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ impl CoreLoop {
1616
/// durable through, plus the number of checkpoint files published.
1717
///
1818
/// Each tenant's Loro state is exported per collection and written to
19-
/// `{data_dir}/crdt-ckpt/core-{core_id}/tenant-{tid}-coll-{hex(collection)}.ckpt`
19+
/// `{data_dir}/crdt-ckpt/core-{core_id}/db-{dbid}-tenant-{tid}-coll-{hex(collection)}.ckpt`
2020
/// with atomic temp+rename. The per-core subdir is required because `data_dir` is
2121
/// shared across all cores and a tenant's CRDT state is fragmented across
2222
/// cores by collection — without the subdir, cores would race-overwrite
@@ -62,10 +62,10 @@ impl CoreLoop {
6262
for ((database_id, tenant_id), engine) in &self.crdt_engines {
6363
let database_id = database_id.as_u64();
6464
let tid = tenant_id.as_u64();
65-
// One checkpoint file per (tenant, collection) — each collection
66-
// owns its own LoroDoc. Filenames are
67-
// `tenant-{id}-coll-{hex(collection)}.ckpt`, matching the loader's
68-
// parse and the cluster-restore writer.
65+
// One checkpoint file per (database, tenant, collection) — each
66+
// collection owns its own LoroDoc. Filenames are
67+
// `db-{dbid}-tenant-{id}-coll-{hex(collection)}.ckpt`, matching the
68+
// loader's parse and the cluster-restore writer.
6969
let snapshots = engine
7070
.export_all_snapshots()
7171
.map_err(|e| crate::Error::Storage {

nodedb/src/data/executor/handlers/snapshot/create.rs

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -173,13 +173,15 @@ impl CoreLoop {
173173
if failed || encoded.is_empty() {
174174
continue;
175175
}
176-
snapshot.crdt_constraints.push((
177-
task.request.database_id.as_u64(),
178-
tenant_id,
179-
collection,
180-
version,
181-
encoded,
182-
));
176+
snapshot
177+
.crdt_constraints
178+
.push(crate::types::snapshot::CrdtConstraintEntry {
179+
database_id: task.request.database_id.as_u64(),
180+
tenant_id,
181+
collection,
182+
version,
183+
constraints: encoded,
184+
});
183185
}
184186
}
185187

nodedb/src/data/executor/handlers/snapshot/restore/tenant_snapshot.rs

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -276,14 +276,15 @@ impl CoreLoop {
276276
// on error — warn and continue, matching the `crdt_state` loop, since
277277
// a failed reconstruction only reverts to the pre-fix (over-rejecting)
278278
// behavior rather than corrupting state.
279-
for (database_raw, tid_raw, collection, version, encoded) in &snap.crdt_constraints {
279+
for entry in &snap.crdt_constraints {
280280
if let Err(e) = self.restore_crdt_constraints(
281-
*database_raw,
282-
*tid_raw,
283-
collection,
284-
*version,
285-
encoded,
281+
entry.database_id,
282+
entry.tenant_id,
283+
&entry.collection,
284+
entry.version,
285+
&entry.constraints,
286286
) {
287+
let (tid_raw, collection) = (entry.tenant_id, &entry.collection);
287288
warn!(tid_raw, %collection, error = %e, "failed to restore crdt constraints");
288289
} else {
289290
crdt_constraints_written += 1;

nodedb/src/types/snapshot.rs

Lines changed: 42 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,35 @@ pub struct TenantDataSnapshot {
156156
/// pre-fix (fail-safe, over-rejecting) behavior rather than a hard error.
157157
#[msgpack(default)]
158158
#[serde(default)]
159-
pub crdt_constraints: Vec<(u64, u64, String, u64, Vec<Vec<u8>>)>,
159+
pub crdt_constraints: Vec<CrdtConstraintEntry>,
160+
}
161+
162+
/// One collection's CRDT constraint set plus its installed version, carried
163+
/// through a snapshot so `restore_crdt_constraints` can reconstruct the
164+
/// validator and the version together.
165+
#[derive(
166+
Debug,
167+
Clone,
168+
PartialEq,
169+
Eq,
170+
serde::Serialize,
171+
serde::Deserialize,
172+
zerompk::ToMessagePack,
173+
zerompk::FromMessagePack,
174+
Default,
175+
)]
176+
pub struct CrdtConstraintEntry {
177+
/// Database owning the collection.
178+
pub database_id: u64,
179+
/// Tenant owning the collection.
180+
pub tenant_id: u64,
181+
/// Collection the constraint set applies to.
182+
pub collection: String,
183+
/// Installed constraint version. The write gate rejects peer deltas whose
184+
/// `constraint_version_required` exceeds this.
185+
pub version: u64,
186+
/// Encoded constraint definitions.
187+
pub constraints: Vec<Vec<u8>>,
160188
}
161189

162190
/// A single PK → surrogate identity binding carried in a snapshot/backup.
@@ -529,13 +557,13 @@ mod tests {
529557
let not_null_bytes = zerompk::to_msgpack_vec(&not_null).expect("encode NotNull constraint");
530558

531559
let snap = TenantDataSnapshot {
532-
crdt_constraints: vec![(
533-
0,
534-
7,
535-
"users".to_string(),
536-
3,
537-
vec![unique_bytes, not_null_bytes],
538-
)],
560+
crdt_constraints: vec![CrdtConstraintEntry {
561+
database_id: 0,
562+
tenant_id: 7,
563+
collection: "users".to_string(),
564+
version: 3,
565+
constraints: vec![unique_bytes, not_null_bytes],
566+
}],
539567
..Default::default()
540568
};
541569

@@ -544,11 +572,12 @@ mod tests {
544572
zerompk::from_msgpack(&bytes).expect("decode snapshot with crdt_constraints");
545573

546574
assert_eq!(decoded.crdt_constraints.len(), 1);
547-
let (database_id, tid, collection, version, encoded) = &decoded.crdt_constraints[0];
548-
assert_eq!(*database_id, 0);
549-
assert_eq!(*tid, 7);
550-
assert_eq!(collection, "users");
551-
assert_eq!(*version, 3);
575+
let entry = &decoded.crdt_constraints[0];
576+
let encoded = &entry.constraints;
577+
assert_eq!(entry.database_id, 0);
578+
assert_eq!(entry.tenant_id, 7);
579+
assert_eq!(entry.collection, "users");
580+
assert_eq!(entry.version, 3);
552581
assert_eq!(encoded.len(), 2);
553582

554583
let decoded_unique: nodedb_crdt::Constraint =

nodedb/tests/warm_storage_object_store.rs

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -292,14 +292,15 @@ async fn snapshot_bytes_roundtrip_write_and_restore() {
292292
assert_eq!(on_disk, hnsw_bytes, "HNSW checkpoint bytes must match");
293293

294294
// ── Verify CRDT checkpoint file ──────────────────────────────────────────
295-
// CRDT checkpoints are per-collection and per-core:
296-
// `crdt-ckpt/core-{id}/tenant-{tid}-coll-{hex(collection)}.ckpt`. The hex
297-
// encoding mirrors the engine's filename scheme (collection bytes, lowercase).
295+
// CRDT checkpoints are per-collection, per-database and per-core:
296+
// `crdt-ckpt/core-{id}/db-{dbid}-tenant-{tid}-coll-{hex(collection)}.ckpt`.
297+
// The hex encoding mirrors the engine's filename scheme (collection bytes,
298+
// lowercase).
298299
let crdt_coll_hex: String = "testcoll".bytes().map(|b| format!("{b:02x}")).collect();
299300
let crdt_ckpt = data_dir
300301
.join("crdt-ckpt")
301302
.join("core-0")
302-
.join(format!("tenant-1-coll-{crdt_coll_hex}.ckpt"));
303+
.join(format!("db-0-tenant-1-coll-{crdt_coll_hex}.ckpt"));
303304
assert!(
304305
crdt_ckpt.exists(),
305306
"CRDT checkpoint file must exist after restore"

0 commit comments

Comments
 (0)