From 6351f6570b3d12e2f75c9eb998b2b24abd90152d Mon Sep 17 00:00:00 2001 From: Mohammod Al Amin Ashik Date: Thu, 25 Jun 2026 11:55:33 +0800 Subject: [PATCH] fix(storage): drop a deleted FeatureSet from bindings (no "not found") MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FeatureSets are soft-deleted (`is_deleted = 1`), so the junction's FK ON DELETE CASCADE never fires — a workspace binding kept a junction row pointing at a FeatureSet that `get()` now reports as missing, surfacing "Feature set not found" and a mapping that didn't reflect the deletion. - Read side: `load_fs_for_bindings` joins `feature_sets` and skips `is_deleted = 1`, so a binding never reports a deleted FS. This also heals bindings orphaned before this fix — no migration needed. - Write side: `FeatureSetRepository::delete` now prunes the junction and `client_grants` rows for the FS in the same transaction. Tests: storage unit (binding drops a soft-deleted FS across get/list/ find_exact; empty binding is valid, not an error) and an integration test (delete a bound FS via the repo → binding reflects it and the resolver routes to the survivor instead of erroring). Claude-Session: https://claude.ai/code/session_01Baan9JmzR43uxxRUh7CAMF Signed-off-by: Mohammod Al Amin Ashik --- .../repositories/feature_set_repository.rs | 20 +++++- .../workspace_binding_repository.rs | 71 +++++++++++++++++-- .../tests/integration/feature_set_resolver.rs | 28 ++++++++ 3 files changed, 113 insertions(+), 6 deletions(-) diff --git a/crates/mcpmux-storage/src/repositories/feature_set_repository.rs b/crates/mcpmux-storage/src/repositories/feature_set_repository.rs index 580a0f54..fed4cb93 100644 --- a/crates/mcpmux-storage/src/repositories/feature_set_repository.rs +++ b/crates/mcpmux-storage/src/repositories/feature_set_repository.rs @@ -300,11 +300,27 @@ impl FeatureSetRepository for SqliteFeatureSetRepository { anyhow::bail!("Cannot delete builtin FeatureSet: {}", id); } - // Soft delete - conn.execute( + // Soft delete, and drop every reference to it in the same transaction. + // FeatureSets are soft-deleted (`is_deleted = 1`), so the FK + // `ON DELETE CASCADE` on the junction / grants never fires — a + // workspace binding or client grant would keep pointing at a + // FeatureSet that `get()` now reports as missing ("Feature set not + // found"). Prune those references explicitly so the deletion is fully + // reflected. + let tx = conn.unchecked_transaction()?; + tx.execute( "UPDATE feature_sets SET is_deleted = 1, updated_at = datetime('now') WHERE id = ?", params![id], )?; + tx.execute( + "DELETE FROM workspace_binding_feature_sets WHERE feature_set_id = ?", + params![id], + )?; + tx.execute( + "DELETE FROM client_grants WHERE feature_set_id = ?", + params![id], + )?; + tx.commit()?; Ok(()) } diff --git a/crates/mcpmux-storage/src/repositories/workspace_binding_repository.rs b/crates/mcpmux-storage/src/repositories/workspace_binding_repository.rs index 132dbe1a..25f07268 100644 --- a/crates/mcpmux-storage/src/repositories/workspace_binding_repository.rs +++ b/crates/mcpmux-storage/src/repositories/workspace_binding_repository.rs @@ -94,11 +94,20 @@ impl SqliteWorkspaceBindingRepository { let placeholders = std::iter::repeat_n("?", binding_ids.len()) .collect::>() .join(", "); + // Join `feature_sets` and skip soft-deleted ones: deleting a FeatureSet + // is a soft delete (`is_deleted = 1`), so the FK `ON DELETE CASCADE` on + // this junction never fires and the binding would otherwise keep + // pointing at a FeatureSet that `get()` now reports as missing + // ("Feature set not found"). Filtering here keeps a binding's + // `feature_set_ids` consistent with what actually resolves — and fixes + // bindings that were orphaned before delete-time cleanup existed. let sql = format!( - "SELECT binding_id, feature_set_id - FROM workspace_binding_feature_sets - WHERE binding_id IN ({placeholders}) - ORDER BY binding_id, sort_order, feature_set_id" + "SELECT wbfs.binding_id, wbfs.feature_set_id + FROM workspace_binding_feature_sets wbfs + JOIN feature_sets fs ON fs.id = wbfs.feature_set_id + WHERE wbfs.binding_id IN ({placeholders}) + AND fs.is_deleted = 0 + ORDER BY wbfs.binding_id, wbfs.sort_order, wbfs.feature_set_id" ); let mut stmt = conn.prepare(&sql)?; let params_dyn: Vec<&dyn rusqlite::ToSql> = binding_ids @@ -376,6 +385,60 @@ mod tests { assert_eq!(after.feature_set_ids, vec![fs_id2]); } + async fn soft_delete_fs(db: &Arc>, fs_id: &str) { + let guard = db.lock().await; + guard + .connection() + .execute( + "UPDATE feature_sets SET is_deleted = 1 WHERE id = ?", + params![fs_id], + ) + .unwrap(); + } + + #[tokio::test] + async fn test_soft_deleted_feature_set_is_dropped_from_binding() { + // Regression: a FeatureSet is soft-deleted, so the junction's FK + // ON DELETE CASCADE never fires. The binding must NOT keep reporting + // the deleted FS (which `get()` now treats as missing → "Feature set + // not found"); only the live one(s) come back. + let (repo, space_id, fs_id1) = fixture().await; + let db = repo.db.clone(); + let fs_id2 = add_fs(&db, space_id, "second").await; + + let root = if cfg!(windows) { "d:\\del" } else { "/del" }; + let binding = + WorkspaceBinding::new_multi(root, space_id, vec![fs_id1.clone(), fs_id2.clone()]); + repo.create(&binding).await.unwrap(); + assert_eq!( + repo.get(&binding.id) + .await + .unwrap() + .unwrap() + .feature_set_ids, + vec![fs_id1.clone(), fs_id2.clone()] + ); + + soft_delete_fs(&db, &fs_id1).await; + + // get(), list() and find_exact_for_roots() all drop the deleted FS. + let got = repo.get(&binding.id).await.unwrap().unwrap(); + assert_eq!(got.feature_set_ids, vec![fs_id2.clone()]); + let listed = repo.list().await.unwrap(); + assert_eq!(listed[0].feature_set_ids, vec![fs_id2.clone()]); + let matched = repo + .find_exact_for_roots(&[root.to_string()]) + .await + .unwrap() + .unwrap(); + assert_eq!(matched.feature_set_ids, vec![fs_id2.clone()]); + + // Deleting the remaining FS leaves an (empty) binding, not an error. + soft_delete_fs(&db, &fs_id2).await; + let empty = repo.get(&binding.id).await.unwrap().unwrap(); + assert!(empty.feature_set_ids.is_empty()); + } + #[tokio::test] async fn test_create_allows_empty_fs_list() { // An empty feature_set_ids is a valid "no Space tools" mapping — the diff --git a/tests/rust/tests/integration/feature_set_resolver.rs b/tests/rust/tests/integration/feature_set_resolver.rs index c5328b49..b9bf33e5 100644 --- a/tests/rust/tests/integration/feature_set_resolver.rs +++ b/tests/rust/tests/integration/feature_set_resolver.rs @@ -280,6 +280,34 @@ async fn binding_routes_to_its_target_space_and_fs() { assert_eq!(r.feature_set_ids, vec![f.fs_a_id]); } +#[tokio::test] +async fn deleting_a_bound_feature_set_drops_it_and_resolution_survives() { + // Repro of the "Feature set not found" report: a folder mapped to two + // FeatureSets; deleting one must drop it from the binding (FeatureSets are + // soft-deleted, so the FK ON DELETE CASCADE can't fire) and resolution must + // keep working — routing to the survivor, not erroring on the missing one. + let f = Fixture::new().await; + let binding = WorkspaceBinding::new_multi( + normalize_workspace_root(test_root()), + f.space_id, + vec![f.fs_a_id.clone(), f.fs_b_id.clone()], + ); + f.binding_repo.create(&binding).await.unwrap(); + + f.fs_repo.delete(&f.fs_a_id).await.unwrap(); + + // The binding no longer references the deleted FS... + let reloaded = f.binding_repo.get(&binding.id).await.unwrap().unwrap(); + assert_eq!(reloaded.feature_set_ids, vec![f.fs_b_id.clone()]); + + // ...and the resolver routes via the binding to just the survivor. + f.session_roots.set("s", [test_root()]); + f.session_roots.set_roots_capable("s", true); + let r = f.resolver.resolve(Some("s"), None).await.unwrap(); + assert_eq!(r.source, ResolutionSource::WorkspaceBinding); + assert_eq!(r.feature_set_ids, vec![f.fs_b_id.clone()]); +} + #[tokio::test] async fn no_inheritance_child_of_bound_parent_falls_back_to_default() { // Inheritance is intentionally NOT supported: a session whose reported root