From 0e2b18224f26b18bdb26e038c61275fd27683f6a Mon Sep 17 00:00:00 2001 From: Cameron Cordes Date: Sun, 10 May 2026 15:44:38 -0400 Subject: [PATCH] knowledge: pre-delete relational facts so entity delete succeeds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DELETE /knowledge/entities/{id} was 500ing on any entity that was the object of a relational fact. entity_facts.object_entity_id has ON DELETE SET NULL, but the table also has CHECK (object_entity_id IS NOT NULL OR object_value IS NOT NULL) — purely relational facts (subject + predicate + object_entity_id, no object_value, like "Alice is_friend_of Bob") would have both NULL after SET NULL fired, the CHECK would abort, and the whole DELETE would fail with a CHECK violation. The user just saw QueryError because the DAO swallowed the diesel error string. Wrap delete_entity in a transaction that first deletes facts where the entity is the object AND object_value is null, then deletes the entity. Surviving siblings (typed facts about the entity as subject) are CASCADE'd by the FK as before. Also start surfacing the actual diesel error in a warn log before collapsing to DbErrorKind so future similar issues don't masquerade as the opaque QueryError. A schema-level fix (changing object FK to ON DELETE CASCADE via a table-rebuild migration) is the cleaner long-term resolution and is slated for Phase 2; the DAO-side pre-delete is sufficient and less invasive in the meantime. Test pins the contract: a relational fact pointing at the deleted entity is removed, an unrelated typed fact about an unrelated entity survives, and the entity itself is deleted. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/database/knowledge_dao.rs | 110 ++++++++++++++++++++++++++++++++-- 1 file changed, 105 insertions(+), 5 deletions(-) diff --git a/src/database/knowledge_dao.rs b/src/database/knowledge_dao.rs index ffc368f..091cd9a 100644 --- a/src/database/knowledge_dao.rs +++ b/src/database/knowledge_dao.rs @@ -601,12 +601,41 @@ impl KnowledgeDao for SqliteKnowledgeDao { trace_db_call(cx, "delete", "delete_entity", |_span| { use schema::entities::dsl::*; let mut conn = self.connection.lock().expect("KnowledgeDao lock"); - diesel::delete(entities.filter(id.eq(entity_id))) - .execute(conn.deref_mut()) - .map(|_| ()) - .map_err(|e| anyhow::anyhow!("Delete error: {}", e)) + + // entity_facts has a CHECK constraint requiring + // `object_entity_id IS NOT NULL OR object_value IS NOT NULL`. + // The FK on object_entity_id is ON DELETE SET NULL — but + // facts that pointed at the deleted entity *only* via the + // entity reference (the common case for relational facts + // like "Alice is_friend_of Bob") have no object_value, so + // SET NULL would leave them with both NULLs and the CHECK + // aborts the whole DELETE. Pre-delete those facts in a + // transaction so the CASCADE / SET NULL chain on what + // remains can fire cleanly. + // + // Long-term fix is to change the FK to ON DELETE CASCADE + // via a table-rebuild migration, but the DAO-side workaround + // is sufficient and less invasive. + conn.transaction::<(), diesel::result::Error, _>(|conn| { + use schema::entity_facts::dsl as ef; + diesel::delete( + ef::entity_facts + .filter(ef::object_entity_id.eq(entity_id)) + .filter(ef::object_value.is_null()), + ) + .execute(conn)?; + + diesel::delete(entities.filter(id.eq(entity_id))).execute(conn)?; + Ok(()) + }) + .map_err(|e| anyhow::anyhow!("Delete error: {}", e)) + }) + .map_err(|e| { + // Surface the actual diesel error string before collapsing + // to the opaque DbErrorKind::QueryError. + log::warn!("delete_entity({}) failed: {}", entity_id, e); + DbError::new(DbErrorKind::QueryError) }) - .map_err(|_| DbError::new(DbErrorKind::QueryError)) } fn merge_entities( @@ -1334,6 +1363,77 @@ mod tests { ); } + #[test] + fn delete_entity_clears_relational_facts_that_would_violate_check() { + // entity_facts has a CHECK that at least one of object_entity_id / + // object_value is non-null. The FK on object_entity_id is + // ON DELETE SET NULL, which would leave purely-relational facts + // (subject + predicate + object_entity_id, no object_value) + // with both nulls and abort the delete. The DAO pre-deletes + // those rows in a transaction so the parent delete can succeed. + let cx = opentelemetry::Context::new(); + let conn = connection_with_fks_on(); + let alice = create_user(&conn, "alice"); + create_persona_row(&conn, alice, "default"); + + let mut dao = SqliteKnowledgeDao::from_connection(conn.clone()); + let bob = make_entity(&mut dao, "Bob"); + let carol = make_entity(&mut dao, "Carol"); + + // A relational fact where Carol is the object — exactly the + // shape the CHECK + SET NULL combination would otherwise break. + let (rel_fact, _) = dao + .upsert_fact( + &cx, + InsertEntityFact { + subject_entity_id: bob.id, + predicate: "is_friend_of".to_string(), + object_entity_id: Some(carol.id), + object_value: None, + source_photo: None, + source_insight_id: None, + confidence: 0.6, + status: "active".to_string(), + created_at: 0, + persona_id: "default".to_string(), + user_id: alice, + }, + ) + .unwrap(); + + // A typed fact where Bob is the subject — should survive. + add_fact(&mut dao, bob.id, "has_age", "30", alice, "default"); + + // Delete Carol — should succeed (relational fact pre-deleted). + dao.delete_entity(&cx, carol.id).unwrap(); + + assert!( + dao.get_entity_by_id(&cx, carol.id).unwrap().is_none(), + "Carol should be deleted" + ); + // The relational fact about Carol should be gone (pre-deleted by + // the DAO's transaction, not SET NULL'd). + let bob_facts = dao + .get_facts_for_entity( + &cx, + bob.id, + &PersonaFilter::Single { + user_id: alice, + persona_id: "default".to_string(), + }, + ) + .unwrap(); + assert!( + !bob_facts.iter().any(|f| f.id == rel_fact.id), + "relational fact pointing at Carol should be removed" + ); + // The typed fact survives. + assert!( + bob_facts.iter().any(|f| f.predicate == "has_age"), + "typed fact about Bob should survive Carol's deletion" + ); + } + #[test] fn upsert_entity_collapses_near_duplicate_by_embedding() { // The agent's pre-flight check uses FTS5 prefix tokens, which