From 75836ffb99a34f8796ea423f80e5662f45d4b525 Mon Sep 17 00:00:00 2001 From: Ian Rogers Date: Tue, 28 Apr 2026 08:15:20 -0700 Subject: [PATCH 1/2] db: prevent merging distinct versions or colliding patch indices Add index collision and version compatibility checks to the cover letter message ID candidate matching loop in create_patchset. This prevents a v2 patchset from incorrectly merging into a v1 patchset when a cover letter ID or similar is matched. The candidate matching loop in create_patchset resolves patchsets by cover_letter_message_id. Without strict validation, this bypassed both version compatibility checks and index collision prevention, causing incoming v2 patches to overwrite and corrupt matching v1 series. Fix this by explicitly validating candidates in the matching loop: 1. Verify that the incoming patch's version matches the candidate's version. 2. Ensure the candidate does not already contain a patch for the same part index (index collision check). I expierenced this issue in this v2 patch series: https://sashiko.dev/#/patchset/20260425020529.3246331-1-irogers%40google.com where the patch series is complete but the 1 of 2 patch was matched as a cover letter. Signed-off-by: Ian Rogers --- src/db.rs | 25 +++- tests/cover_letter_version_merge_test.rs | 169 +++++++++++++++++++++++ 2 files changed, 193 insertions(+), 1 deletion(-) create mode 100644 tests/cover_letter_version_merge_test.rs diff --git a/src/db.rs b/src/db.rs index 60af07cbb..682fcbc12 100644 --- a/src/db.rs +++ b/src/db.rs @@ -1637,11 +1637,34 @@ impl Database { ) .await?; if let Ok(Some(row)) = rows.next().await { - // Found it! Use this ID. We'll update its fields below. let id: i64 = row.get(0)?; + let existing_subject: String = row.get(3)?; let subject_index: u32 = row.get(4).unwrap_or(9999); let existing_total: u32 = row.get(5).unwrap_or(1); + let v_new = version.unwrap_or(1); + let v_old = crate::patch::parse_subject_version(&existing_subject).unwrap_or(1); + + // Check for index collision + let index_collision = if part_index == 0 { + subject_index == 0 + } else { + let mut p_rows = self + .conn + .query( + "SELECT 1 FROM patches WHERE patchset_id = ? AND part_index = ? AND message_id != ?", + libsql::params![id, part_index, message_id], + ) + .await?; + p_rows.next().await.ok().flatten().is_some() + }; + + if index_collision || v_new != v_old { + continue; + } + + // Found it! Use this ID. We'll update its fields below. + // Prevent downgrading a series to a singleton if we already have multiple parts. // This handles cases where a singleton root (1/1) overwrites a series (N/N) inferred from replies. let final_total = if total_parts == 1 && existing_total > 1 { diff --git a/tests/cover_letter_version_merge_test.rs b/tests/cover_letter_version_merge_test.rs new file mode 100644 index 000000000..e5196aedb --- /dev/null +++ b/tests/cover_letter_version_merge_test.rs @@ -0,0 +1,169 @@ +// Copyright 2026 The Sashiko Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use sashiko::db::Database; +use sashiko::settings::DatabaseSettings; +use std::sync::Arc; + +async fn setup_db() -> Arc { + let settings = DatabaseSettings { + url: ":memory:".to_string(), + token: String::new(), + }; + let db = Database::new(&settings).await.unwrap(); + db.migrate().await.unwrap(); + Arc::new(db) +} + +#[tokio::test] +async fn test_cover_letter_matching_with_different_versions_should_not_merge() { + let db = setup_db().await; + + // Create Thread + let t1 = db + .create_thread("root1", "Test Series", 1000) + .await + .unwrap(); + + // v1 Series: Part 1/2 + db.create_message( + /* message_id: */ "v1_patch1", + /* thread_id: */ t1, + /* in_reply_to: */ None, + /* author: */ "Author", + /* subject: */ "[PATCH 1/2] Fix bug", + /* date: */ 1000, + /* body: */ "body", + /* to: */ "", + /* cc: */ "", + /* git_blob_hash: */ None, + /* mailing_list: */ None, + ) + .await + .unwrap(); + + let ps1 = db + .create_patchset( + /* thread_id: */ t1, + /* cover_letter_message_id: */ None, // No cover letter yet + /* message_id: */ "v1_patch1", + /* subject: */ "[PATCH 1/2] Fix bug", + /* author: */ "Author", + /* date: */ 1000, + /* total_parts: */ 2, + /* parser_version: */ 0, + /* to: */ "", + /* cc: */ "", + /* version: */ None, + /* part_index: */ 1, + /* baseline_id: */ None, + /* strict_author: */ true, + /* skip_filters: */ None, + /* only_filters: */ None, + ) + .await + .unwrap() + .unwrap(); + + db.create_patch(ps1, "v1_patch1", 1, "diff").await.unwrap(); + + // v1 Series: Part 2/2 (reply to v1 1/2) + db.create_message( + /* message_id: */ "v1_patch2", + /* thread_id: */ t1, + /* in_reply_to: */ Some("v1_patch1"), + /* author: */ "Author", + /* subject: */ "[PATCH 2/2] Fix bug", + /* date: */ 1010, + /* body: */ "body", + /* to: */ "", + /* cc: */ "", + /* git_blob_hash: */ None, + /* mailing_list: */ None, + ) + .await + .unwrap(); + + let ps1_b = db + .create_patchset( + /* thread_id: */ t1, + /* cover_letter_message_id: */ + Some("v1_patch1"), // Treated as cover letter by in_reply_to logic + /* message_id: */ "v1_patch2", + /* subject: */ "[PATCH 2/2] Fix bug", + /* author: */ "Author", + /* date: */ 1010, + /* total_parts: */ 2, + /* parser_version: */ 0, + /* to: */ "", + /* cc: */ "", + /* version: */ None, + /* part_index: */ 2, + /* baseline_id: */ None, + /* strict_author: */ true, + /* skip_filters: */ None, + /* only_filters: */ None, + ) + .await + .unwrap() + .unwrap(); + + assert_eq!(ps1, ps1_b); // They merge fine + + // v2 Series: Part 1/2 (in reply to v1 1/2) + db.create_message( + /* message_id: */ "v2_patch1", + /* thread_id: */ t1, + /* in_reply_to: */ Some("v1_patch1"), + /* author: */ "Author", + /* subject: */ "[PATCH v2 1/2] Fix bug", + /* date: */ 1100, + /* body: */ "body", + /* to: */ "", + /* cc: */ "", + /* git_blob_hash: */ None, + /* mailing_list: */ None, + ) + .await + .unwrap(); + + let ps2 = db + .create_patchset( + /* thread_id: */ t1, + /* cover_letter_message_id: */ + Some("v1_patch1"), // Incoming patch has in_reply_to pointing to v1 + /* message_id: */ "v2_patch1", + /* subject: */ "[PATCH v2 1/2] Fix bug", + /* author: */ "Author", + /* date: */ 1100, + /* total_parts: */ 2, + /* parser_version: */ 0, + /* to: */ "", + /* cc: */ "", + /* version: */ Some(2), + /* part_index: */ 1, + /* baseline_id: */ None, + /* strict_author: */ true, + /* skip_filters: */ None, + /* only_filters: */ None, + ) + .await + .unwrap() + .unwrap(); + + assert_ne!( + ps1, ps2, + "v2 patch 1/2 should NOT merge into v1 patchset via cover_letter_message_id matching" + ); +} From 269865f393b7baf83111fc958145d19c43d16b6d Mon Sep 17 00:00:00 2001 From: Ian Rogers Date: Tue, 28 Apr 2026 08:42:56 -0700 Subject: [PATCH 2/2] db: allow cross-thread merging of complementary patchsets Relax the thread isolation rule in create_patchset to allow merging cross-thread patchsets when the existing patchset is incomplete and the two are complementary (no index collision). This fixes an issue when a partial patch series is resent (e.g., due to a git send-email failure halfway through), where the resent patches arrive in a new thread. Previously, strict thread isolation prevented merging these patches into the original incomplete series, resulting in two distinct incomplete series. To support cross-thread complementary merging safely without accidental patch stealing, this commit introduces the following: 1. Relax thread compatibility: Allow different threads to be matched if the candidate series is incomplete (existing_received < existing_total) and there is no index collision. 2. Prioritize same-thread candidates: When sorting matched candidates, prioritize matches in the same thread over cross-thread matches. 3. Prevent cross-thread stealing: In the multiple matches merge loop, only merge existing patchsets if they belong to the same thread. This issue was seen when sending a large patch series that failed half way through, and sending the remaining patches left two incomplete series: https://sashiko.dev/#/patchset/20260425224125.160890-1-irogers%40google.com https://sashiko.dev/#/patchset/20260425224503.170337-1-irogers%40google.com Signed-off-by: Ian Rogers --- src/db.rs | 38 +++-- .../cross_thread_complementary_merge_test.rs | 131 ++++++++++++++++++ tests/merge_bug_prefixes.rs | 6 +- 3 files changed, 158 insertions(+), 17 deletions(-) create mode 100644 tests/cross_thread_complementary_merge_test.rs diff --git a/src/db.rs b/src/db.rs index 682fcbc12..9fce2d177 100644 --- a/src/db.rs +++ b/src/db.rs @@ -1862,8 +1862,10 @@ impl Database { }; // Thread Enforcement: To prevent cross-thread "stealing" of patches for resends of the same series, - // we strictly require multi-part series patches to belong to the same thread. - let thread_compatible = same_thread || is_singleton; + // we strictly require multi-part series patches to belong to the same thread, UNLESS + // the candidate series is incomplete, in which case we allow merging complementary parts. + let thread_compatible = + same_thread || is_singleton || (existing_received < existing_total); if author_or_series_match && (!strict_author || (date - existing_date).abs() < 86400) @@ -1874,20 +1876,28 @@ impl Database { && thread_compatible && !index_collision { - matches.push((id, existing_subject_index)); + matches.push((id, existing_subject_index, existing_thread_id)); } } if !matches.is_empty() { - // Sort matches to pick the "best" one to keep (e.g. oldest ID or one with lowest subject index) - // Let's keep the one with the lowest ID (created first) - matches.sort_by_key(|k| k.0); + // Sort matches: Prioritize matches in the same thread over cross-thread matches, then pick by oldest ID. + matches.sort_by_key(|k| { + let same = k.2 == Some(thread_id); + (!same, k.0) + }); let target_id = matches[0].0; + let target_thread_id = matches[0].2; let mut current_subject_index = matches[0].1; - // If we have multiple matches, merge others into target_id - for (merge_from_id, merge_subject_index) in matches.iter().skip(1) { + // If we have multiple matches, merge others into target_id. + // Cross-thread merging of existing distinct patchsets is prevented to avoid accidental stealing. + for (merge_from_id, merge_subject_index, merge_from_thread_id) in matches.iter().skip(1) + { + if *merge_from_thread_id != target_thread_id { + continue; + } let merge_from_id = *merge_from_id; info!("Merging patchset {} into {}", merge_from_id, target_id); @@ -4978,7 +4988,7 @@ mod tests { } #[tokio::test] - async fn test_cross_thread_no_merge() { + async fn test_cross_thread_complementary_merge_in_db() { let db = setup_db().await; // 1. Create Thread A and Patchset A (1/2) @@ -5068,10 +5078,10 @@ mod tests { .unwrap() .unwrap(); - // 3. Assert they DID NOT merge (ps2 should NOT equal ps1) - assert_ne!( + // 3. Assert they DID merge (ps2 should equal ps1) + assert_eq!( ps1, ps2, - "Patchsets from different threads should NOT merge even if author/time match" + "Patchsets from different threads SHOULD merge when they are complementary and incomplete" ); // 4. Verify total patches count or received parts @@ -5083,13 +5093,13 @@ mod tests { .await .unwrap() .unwrap(); - assert_eq!(details1["received_parts"], 1); + assert_eq!(details1["received_parts"], 2); let details2 = db .get_patchset_details(ps2, None, None) .await .unwrap() .unwrap(); - assert_eq!(details2["received_parts"], 1); + assert_eq!(details2["received_parts"], 2); } #[tokio::test] diff --git a/tests/cross_thread_complementary_merge_test.rs b/tests/cross_thread_complementary_merge_test.rs new file mode 100644 index 000000000..ca6fc5f2d --- /dev/null +++ b/tests/cross_thread_complementary_merge_test.rs @@ -0,0 +1,131 @@ +// Copyright 2026 The Sashiko Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use sashiko::db::Database; +use sashiko::settings::DatabaseSettings; +use std::sync::Arc; + +async fn setup_db() -> Arc { + let settings = DatabaseSettings { + url: ":memory:".to_string(), + token: String::new(), + }; + let db = Database::new(&settings).await.unwrap(); + db.migrate().await.unwrap(); + Arc::new(db) +} + +#[tokio::test] +async fn test_cross_thread_complementary_merge() { + let db = setup_db().await; + + // 1. Create Thread A + let t_a = db + .create_thread("root_a", "Series Subject", 1000) + .await + .unwrap(); + + // First batch: patches 1 to 40 out of 60 + db.create_message( + /* message_id: */ "patch_1", + /* thread_id: */ t_a, + /* in_reply_to: */ None, + /* author: */ "Author", + /* subject: */ "[PATCH 1/60] Fix bug part 1", + /* date: */ 1000, + /* body: */ "body", + /* to: */ "", + /* cc: */ "", + /* git_blob_hash: */ None, + /* mailing_list: */ None, + ) + .await + .unwrap(); + + let ps1 = db + .create_patchset( + /* thread_id: */ t_a, + /* cover_letter_message_id: */ None, + /* message_id: */ "patch_1", + /* subject: */ "[PATCH 1/60] Fix bug part 1", + /* author: */ "Author", + /* date: */ 1000, + /* total_parts: */ 60, + /* parser_version: */ 0, + /* to: */ "", + /* cc: */ "", + /* version: */ None, + /* part_index: */ 1, + /* baseline_id: */ None, + /* strict_author: */ true, + /* skip_filters: */ None, + /* only_filters: */ None, + ) + .await + .unwrap() + .unwrap(); + + db.create_patch(ps1, "patch_1", 1, "diff").await.unwrap(); + + // 2. Resend second batch (41 to 60 out of 60) in Thread B + let t_b = db + .create_thread("root_b", "Series Subject", 1010) + .await + .unwrap(); + + db.create_message( + /* message_id: */ "patch_41", + /* thread_id: */ t_b, + /* in_reply_to: */ None, // No connection to Thread A + /* author: */ "Author", + /* subject: */ "[PATCH 41/60] Fix bug part 41", + /* date: */ 1010, + /* body: */ "body", + /* to: */ "", + /* cc: */ "", + /* git_blob_hash: */ None, + /* mailing_list: */ None, + ) + .await + .unwrap(); + + let ps2 = db + .create_patchset( + /* thread_id: */ t_b, + /* cover_letter_message_id: */ None, + /* message_id: */ "patch_41", + /* subject: */ "[PATCH 41/60] Fix bug part 41", + /* author: */ "Author", + /* date: */ 1010, + /* total_parts: */ 60, + /* parser_version: */ 0, + /* to: */ "", + /* cc: */ "", + /* version: */ None, + /* part_index: */ 41, + /* baseline_id: */ None, + /* strict_author: */ true, + /* skip_filters: */ None, + /* only_filters: */ None, + ) + .await + .unwrap() + .unwrap(); + + // 3. Assert that they DO merge (ps1 == ps2) + assert_eq!( + ps1, ps2, + "Patchsets from different threads should merge when they are complementary parts of the same series." + ); +} diff --git a/tests/merge_bug_prefixes.rs b/tests/merge_bug_prefixes.rs index d2f79d83d..60f6fbfc5 100644 --- a/tests/merge_bug_prefixes.rs +++ b/tests/merge_bug_prefixes.rs @@ -99,7 +99,7 @@ async fn test_merge_prefixes_mismatch_should_split() { } #[tokio::test] -async fn test_merge_prefixes_match_should_not_merge() { +async fn test_merge_prefixes_match_should_merge_if_complementary() { let db = setup_db().await; let t1 = db @@ -161,8 +161,8 @@ async fn test_merge_prefixes_match_should_not_merge() { .unwrap() .unwrap(); - assert_ne!( + assert_eq!( ps1, ps2, - "Matching prefixes (net-next) should NOT merge across threads" + "Matching prefixes (net-next) SHOULD merge across threads when they are complementary" ); }