Skip to content

Commit fd748c3

Browse files
committed
control: cover the edge-recon gate in a non-default database
The fix had no test: `plan_needs_implicit_edge_recon` had zero call sites anywhere in the suite, and the existing non-default-database graph test sends an expression update that the planner rejects at plan time, so it never reaches this gate. Reverting the fix left every test green. Two tests now drive the gate directly: seed a catalog with an edge-bearing collection in a non-default database, hand the gate a `BulkUpdate` carrying the database-qualified collection the planner builds, and assert it fires and returns that qualified key. The second covers `DatabaseId::DEFAULT` as the identity case. Reverting the bare-name lookup fails the first and leaves the second passing, which is the red arm this needed. The restore-path comment also claimed the collection registry qualifies its bare string before the engine is keyed. It does not: the string reaches `from_stored` and the tenant engine's collection map verbatim, which is what makes restore disagree with an ordinary apply. The comment now says that.
1 parent f40dcdb commit fd748c3

2 files changed

Lines changed: 132 additions & 7 deletions

File tree

‎nodedb/src/control/crdt_admission.rs‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -464,13 +464,14 @@ pub(crate) async fn dispatch_crdt_restore_admitted(
464464
collection,
465465
// The restore path builds its own Apply from this same string, so the
466466
// preview and that apply agree with each other. The string is the bare
467-
// caller form, which the collection registry qualifies before the engine
468-
// is keyed, so in a non-default database this path addresses a different
469-
// document than every ordinary apply, which routes the database-qualified
470-
// key (`engine_key` above). Pre-existing and deliberately not changed
471-
// here: canonicalizing it means changing the form the restore caller
472-
// passes, and `CrdtOp::Apply.collection` below is rebuilt from this same
473-
// string.
467+
// caller form, and nothing qualifies it on the way in: it is handed to
468+
// `from_stored` verbatim and the tenant engine keys its collections by
469+
// exactly that string. Every ordinary apply instead routes the
470+
// database-qualified key (`engine_key` above), so in a non-default
471+
// database restore addresses a different document than an apply of the
472+
// same collection. Pre-existing and deliberately not changed here:
473+
// canonicalizing it means changing the form the restore caller passes,
474+
// and `CrdtOp::Apply.collection` below is rebuilt from this same string.
474475
engine_collection: collection,
475476
timeout,
476477
event_source,

‎nodedb/src/control/planner/calvin/dependent_recon.rs‎

Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -440,3 +440,127 @@ async fn dispatch_dependent_edge_recon_inner(
440440
apply_result,
441441
})
442442
}
443+
444+
#[cfg(test)]
445+
mod tests {
446+
use std::sync::Arc;
447+
448+
use super::*;
449+
use crate::control::security::catalog::StoredCollection;
450+
use crate::types::VShardId;
451+
use nodedb_physical::physical_plan::{DocumentOp, PhysicalPlan};
452+
use nodedb_physical::physical_task::{PhysicalTask, PostSetOp};
453+
use nodedb_types::QualifiedCollection;
454+
455+
/// A `SharedState` with a real on-disk catalog and no Data Plane: this test
456+
/// only reads the catalog, so nothing else has to be live.
457+
fn state_with_edge_bearing_collection(
458+
dir: &tempfile::TempDir,
459+
database_id: DatabaseId,
460+
tenant_id: u64,
461+
) -> Arc<SharedState> {
462+
let wal_dir = dir.path().join("wal");
463+
std::fs::create_dir_all(&wal_dir).unwrap();
464+
let wal = Arc::new(crate::wal::WalManager::open_for_testing(&wal_dir).unwrap());
465+
let (dispatcher, _) = crate::bridge::dispatch::Dispatcher::new(1, 16);
466+
let state = SharedState::open(
467+
crate::control::state::DataPlaneHandles {
468+
dispatcher,
469+
quiesce: crate::bridge::quiesce::CollectionQuiesce::new(),
470+
array_catalog: crate::control::array_catalog::ArrayCatalog::handle(),
471+
system_metrics: Arc::new(crate::control::metrics::SystemMetrics::new()),
472+
},
473+
wal,
474+
&dir.path().join("catalog.redb"),
475+
&crate::config::auth::AuthConfig::default(),
476+
Default::default(),
477+
false,
478+
crate::data::executor::core_loop::test_governor(),
479+
)
480+
.unwrap();
481+
482+
let mut coll = StoredCollection::new(tenant_id, "edges_nd", "admin");
483+
coll.collection_type = nodedb_types::CollectionType::document();
484+
coll.has_implicit_edges = true;
485+
state
486+
.credentials
487+
.catalog()
488+
.put_collection(database_id, &coll)
489+
.unwrap();
490+
state
491+
}
492+
493+
/// A `BulkUpdate` on `collection`, the shape the planner lowers a
494+
/// PK-equality `DELETE`/`UPDATE` into so this gate picks it up.
495+
fn bulk_update_task(database_id: DatabaseId, collection: QualifiedCollection) -> PhysicalTask {
496+
PhysicalTask {
497+
tenant_id: TenantId::new(1),
498+
vshard_id: VShardId::new(0),
499+
database_id,
500+
plan: PhysicalPlan::Document(DocumentOp::BulkUpdate {
501+
collection,
502+
filters: vec![],
503+
updates: vec![],
504+
returning: None,
505+
ollp_predicted_surrogates: None,
506+
ollp_predicted_edges: None,
507+
rls_filters: vec![],
508+
rls_write_check: nodedb_types::RlsWriteCheck::pending_injection(),
509+
resolved_sum_targets: Vec::new(),
510+
declared_primary_key: None,
511+
}),
512+
post_set_op: PostSetOp::None,
513+
txn_id: None,
514+
}
515+
}
516+
517+
/// The gate must fire for an edge-bearing collection in a NON-DEFAULT
518+
/// database.
519+
///
520+
/// The plan carries the database-qualified collection, but the catalog is
521+
/// keyed by the bare name, so a lookup with the qualified form misses, this
522+
/// gate returns `None`, and the OLLP/Calvin reconnaissance that cleans up
523+
/// mirrored edges never routes. That failure is silent — the write still
524+
/// succeeds — so only a test that asserts the gate's own answer catches it.
525+
#[test]
526+
fn gate_fires_for_an_edge_bearing_collection_in_a_non_default_database() {
527+
let dir = tempfile::tempdir().unwrap();
528+
let database_id = DatabaseId::new(1024);
529+
let state = state_with_edge_bearing_collection(&dir, database_id, 1);
530+
let task = bulk_update_task(
531+
database_id,
532+
QualifiedCollection::new(database_id, "edges_nd"),
533+
);
534+
535+
let fired = plan_needs_implicit_edge_recon(&state, &[task], TenantId::new(1)).unwrap();
536+
assert!(
537+
fired.is_some(),
538+
"the gate must fire for an edge-bearing collection in a non-default database; \
539+
a qualified catalog lookup misses and the mirrored edges leak"
540+
);
541+
let (collection, db) = fired.unwrap();
542+
assert_eq!(db, database_id);
543+
assert_eq!(
544+
collection, "1024/edges_nd",
545+
"the returned collection is the plan's routing key, not the bare catalog name"
546+
);
547+
}
548+
549+
/// The default database is the identity case and must keep firing.
550+
#[test]
551+
fn gate_fires_for_an_edge_bearing_collection_in_the_default_database() {
552+
let dir = tempfile::tempdir().unwrap();
553+
let state = state_with_edge_bearing_collection(&dir, DatabaseId::DEFAULT, 1);
554+
let task = bulk_update_task(
555+
DatabaseId::DEFAULT,
556+
QualifiedCollection::new(DatabaseId::DEFAULT, "edges_nd"),
557+
);
558+
559+
let fired = plan_needs_implicit_edge_recon(&state, &[task], TenantId::new(1)).unwrap();
560+
assert!(
561+
fired.is_some(),
562+
"the default-database path must be unchanged"
563+
);
564+
assert_eq!(fired.unwrap().0, "edges_nd");
565+
}
566+
}

0 commit comments

Comments
 (0)