From fc583cd1b38ebb0e54dc88edc9ab4c77341b41df Mon Sep 17 00:00:00 2001 From: thedancingdeveloper Date: Mon, 26 Oct 2026 13:32:38 +0000 Subject: [PATCH 1/2] mirror: pr-batch never adopts a PR it did not open; skip taken batch numbers (WI-2585) open() found the batch PR by head name across all states, so a fresh mirror DB reusing graft/batch-N adopted a leftover merged PR and settled new lands as merged. Batch numbers now skip any branch or PR already on GitHub, and only an open PR at the batch's own head is adopted. Adds regression tests and a repair runbook. Co-Authored-By: Claude Opus 5.5 --- changes/WI-2585.md | 1 + crates/graft-mirror/src/batch/batcher.rs | 53 ++++++++++- crates/graft-mirror/tests/batch.rs | 112 +++++++++++++++++++++++ docs/runbooks/batch-adoption-repair.md | 95 +++++++++++++++++++ 4 files changed, 257 insertions(+), 4 deletions(-) create mode 100644 changes/WI-2585.md create mode 100644 docs/runbooks/batch-adoption-repair.md diff --git a/changes/WI-2585.md b/changes/WI-2585.md new file mode 100644 index 00000000..186f6536 --- /dev/null +++ b/changes/WI-2585.md @@ -0,0 +1 @@ +- `graft-mirror pr-batch` no longer adopts a PR it did not open. A new batch skips every number whose `graft/batch-` branch or PR already exists on GitHub, and a PR is adopted only when it is open at the batch's own head; any other is refused and the batch rejected, so its lands stay pending. Previously a reset mirror DB could settle new lands as merged in an old merged PR. Repair procedure: `docs/runbooks/batch-adoption-repair.md`. diff --git a/crates/graft-mirror/src/batch/batcher.rs b/crates/graft-mirror/src/batch/batcher.rs index 86865f2b..658ba0e2 100644 --- a/crates/graft-mirror/src/batch/batcher.rs +++ b/crates/graft-mirror/src/batch/batcher.rs @@ -34,6 +34,9 @@ use super::types::{ /// An open batch's detail when the line has nothing past its tip. const NOTHING_NEW: &str = "waiting for review; no new lands to append"; +/// How many taken batch numbers a new batch skips before it gives up. +const MAX_TAKEN_PROBES: u64 = 32; + /// Batches one integration line into squashed GitHub PRs. pub struct Batcher<'a, A, F> { api: &'a A, @@ -991,7 +994,7 @@ where } } - let number = self.db.next_batch_number()?; + let number = self.free_number().await?; let created_ms = (self.clock)(); let branch = format!("{BRANCH_PREFIX}{number}"); let line = clean_text(&self.config.line, FIELD_CHARS, false); @@ -1595,11 +1598,53 @@ where Ok(()) } - /// Opens the PR, or adopts the one already on the branch. Never merges - /// or queues it. + /// The next batch number whose branch graft is free to use: past the + /// database's last batch, and past any `graft/batch-` that already + /// has a branch or a PR on GitHub (a reset database, trial leftovers). + /// Such a branch or PR is never reused or adopted (WI-2585). + async fn free_number(&mut self) -> Result { + let first = self.db.next_batch_number()?; + for number in first..first + MAX_TAKEN_PROBES { + let branch = format!("{BRANCH_PREFIX}{number}"); + let there = self.mirror.ls_remote(&[branch.as_str()]).await?; + if there.iter().any(|(_, sha)| sha.is_some()) { + tracing::warn!(branch = %branch, "mirror batch: branch exists; skipping its number"); + continue; + } + if let Some(pull) = self.forge.find_pull(&branch).await? { + tracing::warn!( + branch = %branch, pr = pull.number, + "mirror batch: branch has a PR graft did not open; skipping its number" + ); + continue; + } + return Ok(number); + } + Err(config_err(format!( + "batch numbers {first} to {} all have a branch or PR on GitHub; \ + remove stale {BRANCH_PREFIX}* branches or PRs", + first + MAX_TAKEN_PROBES - 1 + ))) + } + + /// Opens the PR, or adopts the one this batch opened before a crash: open, + /// and at the batch's own head. Any other PR on the branch (merged, + /// closed, or at other commits) was not opened by this batch; it is never + /// adopted, and the batch is rejected so its lands stay pending. async fn open(&mut self, rec: &mut BatchRecord) -> Result<(), MirrorError> { let pull = match self.forge.find_pull(&rec.branch).await? { - Some(pull) => pull, + Some(pull) if pull.open && !pull.merged && pull.head_sha == rec.squash_sha.to_hex() => { + pull + } + Some(pull) => { + return self.reject( + rec, + format!( + "branch {} already has PR #{} that this batch did not open; not adopted", + rec.branch, pull.number + ), + ); + } None => { let new = crate::github::NewPull { title: rec.title.clone(), diff --git a/crates/graft-mirror/tests/batch.rs b/crates/graft-mirror/tests/batch.rs index 61e1e3b8..23a8d602 100644 --- a/crates/graft-mirror/tests/batch.rs +++ b/crates/graft-mirror/tests/batch.rs @@ -2036,3 +2036,115 @@ async fn an_append_whose_lease_fails_is_an_error() { ); assert_eq!(support::branch(&w.upstream, &first.branch).unwrap(), main); } + +/// A PR left on GitHub for `head` that this mirror DB never opened (a trial, +/// or a DB that was reset): merged into `main` as `merged`, its branch gone. +fn leftover_merged_pr(w: &World, head: &str, merged: &str) -> u64 { + let mut prs = w.github.prs_mut(); + let number = prs.len() as u64 + 1; + prs.push(FakePr { + number, + new: NewPull { + title: "an earlier batch".to_owned(), + body: String::new(), + head: head.to_owned(), + base: "main".to_owned(), + }, + open: false, + merged: Some(merged.to_owned()), + }); + number +} + +/// WI-2585: a fresh DB on a GitHub repo that already has `graft/batch-1` +/// merged (branch deleted) never adopts that PR; the lands go in a new PR. +#[tokio::test] +async fn a_leftover_merged_pr_is_never_adopted() { + let w = World::new().await; + w.start().await; + let main = support::branch(&w.upstream, "main").unwrap(); + let old = leftover_merged_pr(&w, "graft/batch-1", &main); + let a = w.land(&[("a.txt", "a1\n")], "add a", 1).await; + let b = w.land(&[("b.txt", "b1\n")], "add b", 2).await; + + let run = w.batcher().run().await.unwrap(); + assert!(run.settled.is_none()); + let rec = opened(run.outcome); + assert_eq!(rec.state, BatchState::Open, "{}", rec.detail); + assert_eq!((rec.number, rec.lands), (2, 2)); + assert_eq!(rec.branch, "graft/batch-2"); + assert!(rec.merged.is_none() && rec.merge_sha.is_none()); + let new = rec.pr_number.unwrap(); + assert_ne!(new, old); + + let prs = w.github.prs(); + let pr = prs.iter().find(|p| p.number == new).unwrap(); + assert!(pr.open && pr.merged.is_none()); + assert_eq!(pr.new.head, "graft/batch-2"); + for id in [a, b] { + assert!( + pr.new.body.contains(&id.to_string()[..13]), + "{}", + pr.new.body + ); + } + // Nothing reached main; the old PR is as it was. + assert_eq!(support::branch(&w.upstream, "main").unwrap(), main); + assert!(support::branch(&w.upstream, "graft/batch-1").is_none()); + let old_pr = prs.iter().find(|p| p.number == old).unwrap(); + assert!(!old_pr.open && old_pr.new.head == "graft/batch-1"); + + // The next run waits on the new PR; it does not settle anything. + match w.batcher().run().await.unwrap().outcome { + BatchOutcome::Waiting(r) => assert_eq!(r.pr_number, Some(new)), + other => panic!("expected to wait, got {other:?}"), + } +} + +/// WI-2585: a leftover branch with other commits and a leftover merged PR +/// (the trial's batch-1 and batch-2) are both skipped; the lands go in a +/// fresh PR on a free number, and neither leftover is changed. +#[tokio::test] +async fn leftover_batch_branches_and_prs_are_skipped_for_a_fresh_number() { + let w = World::new().await; + w.start().await; + let main = support::branch(&w.upstream, "main").unwrap(); + let tree = git(&w.upstream, &["rev-parse", &format!("{main}^{{tree}}")]); + let stray = git( + &w.upstream, + &["commit-tree", &tree, "-p", &main, "-m", "trial leftover"], + ); + git( + &w.upstream, + &["update-ref", "refs/heads/graft/batch-1", &stray], + ); + let old = leftover_merged_pr(&w, "graft/batch-2", &main); + w.land(&[("a.txt", "a1\n")], "add a", 1).await; + w.land(&[("b.txt", "b1\n")], "add b", 2).await; + + let rec = opened(w.batcher().run().await.unwrap().outcome); + assert_eq!(rec.state, BatchState::Open, "{}", rec.detail); + assert_eq!((rec.number, rec.lands), (3, 2)); + assert_eq!(rec.branch, "graft/batch-3"); + assert!(rec.merged.is_none()); + assert_ne!(rec.pr_number, Some(old)); + let prs = w.github.prs(); + let pr = prs + .iter() + .find(|p| Some(p.number) == rec.pr_number) + .unwrap(); + assert!(pr.open && pr.merged.is_none()); + assert_eq!(pr.new.head, "graft/batch-3"); + assert_eq!( + w.changed( + &main, + &support::branch(&w.upstream, "graft/batch-3").unwrap() + ), + ["a.txt", "b.txt"] + ); + assert_eq!( + support::branch(&w.upstream, "graft/batch-1").unwrap(), + stray + ); + assert_eq!(support::branch(&w.upstream, "main").unwrap(), main); +} diff --git a/docs/runbooks/batch-adoption-repair.md b/docs/runbooks/batch-adoption-repair.md new file mode 100644 index 00000000..28dd80ba --- /dev/null +++ b/docs/runbooks/batch-adoption-repair.md @@ -0,0 +1,95 @@ +# Repair a batch that adopted a leftover PR (WI-2585) + +Before WI-2585, `graft-mirror pr-batch` looked up a batch's PR by head +branch name across all PR states. On a repo whose GitHub side still had a +`graft/batch-` PR from earlier (a trial, a re-onboarded repo, a reset +mirror DB), a new batch with the same number adopted that PR. When the old PR +was merged, the batch was settled as `merged` against the old merge commit: +its lands were recorded as delivered but never reached GitHub `main`. + +The fix makes `pr-batch` skip every number whose branch or PR already exists +on GitHub, and never adopt a PR it did not open at its own head. The DB rows +written by the bad run still need this one-off repair. **Deploy the fixed +image first**, or the next run repeats the fault. + +## The graft-dev case + +Repo `graft`, line `backlog-1`, mirror container `graft-dev-mirror-graft`, +DB `/var/lib/graft/mirror/graft.sqlite` in volume `graft-dev-mirror-graft`. +Batch 2 adopted PR #143 (merged 2026-10-24 as `98f6abb`) and was settled as +merged. Batch 1 is `rejected` and changed nothing. Batch 2 was the line's +first batch that was not rejected on a fresh DB, so before it settled the line +record held: + +- `start` = batch 2's `start` (the fork point; nothing had merged since); +- `start_base` = the same changeset (a fresh line starts with both at the fork); +- `sync_from` = `synced` (every sync sets both to the base tip; only a merged + batch moves `sync_from` away from it); +- `sync_batch` = NULL. + +`synced` itself is untouched by the bad settle and stays. + +## Procedure + +1. Stop the mirror so no `pr-batch` or follow runs during the repair: + `docker stop graft-dev-mirror-graft`. +2. Back up the DB: + ```sh + docker run --rm -v graft-dev-mirror-graft:/m alpine \ + cp /m/graft.sqlite /m/graft.sqlite.pre-wi-2585 + ``` +3. Open it with any SQLite 3 shell on the volume (for example + `docker run --rm -it -v graft-dev-mirror-graft:/m keinos/sqlite3 sqlite3 /m/graft.sqlite`) + and check you are repairing what you expect: + ```sql + SELECT number, line, state, pr_number, lands, hex(start), hex(tip), hex(base) + FROM batches ORDER BY number; + -- expect: 1 backlog-1 rejected ...; 2 backlog-1 merged 143 2 ... + SELECT line, hex(synced), hex(sync_from), hex(start), hex(start_base), sync_batch + FROM batch_lines WHERE line = 'backlog-1'; + -- expect: start = sync_from = batch 2's tip (the bad settle wrote both) + ``` + Stop if batch 2 is not `merged` with `pr_number` 143, or if a batch newer + than 2 exists: the state has moved on and this procedure no longer fits. +4. Undo the settle, in one transaction: + ```sql + BEGIN; + UPDATE batches + SET state = 'rejected', + detail = 'WI-2585: adopted leftover PR #143 by head name; not delivered', + merge_sha = NULL, + merged = NULL + WHERE number = 2 AND line = 'backlog-1' AND state = 'merged'; + UPDATE batch_commits SET merged_sha = NULL WHERE batch = 2; + UPDATE batch_lines + SET start = (SELECT start FROM batches WHERE number = 2), + start_base = (SELECT start FROM batches WHERE number = 2), + sync_from = synced, + sync_batch = NULL + WHERE line = 'backlog-1'; + COMMIT; + ``` + Re-run the two queries from step 3: batch 2 is `rejected`, and the line's + `start` equals batch 2's `start`. +5. Start the mirror (`docker start graft-dev-mirror-graft`) and run the batch + with the fixed image: + ```sh + docker exec graft-dev-mirror-graft graft-mirror pr-batch backlog-1 \ + --base main --config /etc/graft/deploy/mirror.toml + ``` + Expected: `batch 3 (open): 2 land(s)` (or a higher number: `graft/batch-2` + now has both a branch and PR #143, so it is skipped) with a **new** PR + listing both lands (c5277958105f2 for WI-2548, and the WI-2542 land). + PR #143 is not touched. +6. Optional tidy-up on GitHub: the branch `graft/batch-2` now holds the bad + run's commits under the merged PR #143. Delete it once the new PR is open; + nothing references it. + +## Other repos + +The same shape applies to any line whose batch settled against a PR it did +not open: check `batches.pr_number` against the PRs the mirror created (the PR +body carries `Graft-Batch: ` and the land ids). If the line had a merged +batch before the bad one, `start`, `start_base` and `sync_from` must come from +that earlier batch's settle (`start` = its `tip`, `start_base` = its `base`), +not from the fork; work it out from `batches` before writing anything. From 70be54b7f5255111c9cdeb14fe13b157de67254f Mon Sep 17 00:00:00 2001 From: thedancingdeveloper Date: Mon, 26 Oct 2026 13:40:42 +0000 Subject: [PATCH 2/2] mirror: runbook STOP check for a post-settle sync; test open() refusing a foreign PR (WI-2585) Review of #261: the repair runbook now stops unless synced is unchanged and no sync changeset follows batch 2, and step 5 checks the new PR carries both lands' files. Adds a test for open()'s reject branch and documents the crash-after-create_pull window. Co-Authored-By: Claude Opus 5.5 --- crates/graft-mirror/src/batch/batcher.rs | 5 ++ crates/graft-mirror/tests/batch.rs | 66 +++++++++++++++++++++++- docs/runbooks/batch-adoption-repair.md | 43 +++++++++++++++ 3 files changed, 113 insertions(+), 1 deletion(-) diff --git a/crates/graft-mirror/src/batch/batcher.rs b/crates/graft-mirror/src/batch/batcher.rs index 658ba0e2..0e6a7acd 100644 --- a/crates/graft-mirror/src/batch/batcher.rs +++ b/crates/graft-mirror/src/batch/batcher.rs @@ -1636,6 +1636,11 @@ where Some(pull) if pull.open && !pull.merged && pull.head_sha == rec.squash_sha.to_hex() => { pull } + // This includes the batch's own PR when a run crashed after + // create_pull and the PR was merged or closed before the retry: + // the batch is rejected and its lands are batched again. If that + // PR was merged, the next batch repeats changes main already has + // and the operator closes it; lands are never silently lost. Some(pull) => { return self.reject( rec, diff --git a/crates/graft-mirror/tests/batch.rs b/crates/graft-mirror/tests/batch.rs index 23a8d602..2fb909bd 100644 --- a/crates/graft-mirror/tests/batch.rs +++ b/crates/graft-mirror/tests/batch.rs @@ -47,6 +47,9 @@ struct FakeGitHub { merging: Mutex>, /// A branch update made immediately after the next PR read. race_push: Mutex>, + /// `(branch, skip)`: after `skip` PR lookups for `branch` answer as + /// usual, a merged PR graft did not open appears on it. + foreign_pr: Mutex>, } impl FakeGitHub { @@ -58,6 +61,7 @@ impl FakeGitHub { comments: Mutex::new(Vec::new()), merging: Mutex::new(Vec::new()), race_push: Mutex::new(None), + foreign_pr: Mutex::new(None), } } @@ -187,7 +191,30 @@ impl FakeGitHub { impl Forge for FakeGitHub { async fn find_pull(&self, branch: &str) -> Result, ForgeError> { - let prs = self.prs.lock().unwrap(); + let mut prs = self.prs.lock().unwrap(); + let mut foreign = self.foreign_pr.lock().unwrap(); + if let Some((b, skip)) = foreign.as_mut() + && b == branch + { + if *skip == 0 { + let main = support::branch(&self.upstream, "main").unwrap(); + let number = prs.len() as u64 + 1; + prs.push(FakePr { + number, + new: NewPull { + title: "someone else's PR".to_owned(), + body: String::new(), + head: branch.to_owned(), + base: "main".to_owned(), + }, + open: false, + merged: Some(main), + }); + *foreign = None; + } else { + *skip -= 1; + } + } Ok(prs .iter() .find(|p| p.new.head == branch) @@ -2148,3 +2175,40 @@ async fn leftover_batch_branches_and_prs_are_skipped_for_a_fresh_number() { ); assert_eq!(support::branch(&w.upstream, "main").unwrap(), main); } + +/// WI-2585: a PR graft did not open that appears on the batch branch after +/// the number was chosen and the branch pushed is not adopted. The batch is +/// rejected unmerged, and the next run carries its lands in a fresh PR. +#[tokio::test] +async fn a_foreign_pr_on_the_pushed_branch_is_rejected_not_adopted() { + let w = World::new().await; + w.start().await; + let main = support::branch(&w.upstream, "main").unwrap(); + let a = w.land(&[("a.txt", "a1\n")], "add a", 1).await; + // The number check reads graft/batch-1 once; the PR appears for open(). + *w.github.foreign_pr.lock().unwrap() = Some(("graft/batch-1".to_owned(), 1)); + + let run = w.batcher().run().await.unwrap(); + let rec = opened(run.outcome); + assert_eq!(rec.number, 1); + assert_eq!(rec.state, BatchState::Rejected); + assert!(rec.detail.contains("did not open"), "{}", rec.detail); + assert!(rec.merged.is_none() && rec.merge_sha.is_none()); + assert_eq!(rec.pr_number, None, "the foreign PR is not recorded"); + assert_eq!(w.github.prs().len(), 1, "no PR was opened over it"); + assert_eq!(support::branch(&w.upstream, "main").unwrap(), main); + + // The lands are still pending: the next run puts them in a new PR on a + // free number, past the taken branch. + let rec = opened(w.batcher().run().await.unwrap().outcome); + assert_eq!(rec.state, BatchState::Open, "{}", rec.detail); + assert_eq!((rec.number, rec.lands), (2, 1)); + let prs = w.github.prs(); + let pr = prs + .iter() + .find(|p| Some(p.number) == rec.pr_number) + .unwrap(); + assert!(pr.open && pr.merged.is_none()); + assert_eq!(pr.new.head, "graft/batch-2"); + assert!(pr.new.body.contains(&a.to_string()[..13])); +} diff --git a/docs/runbooks/batch-adoption-repair.md b/docs/runbooks/batch-adoption-repair.md index 28dd80ba..6f52cbda 100644 --- a/docs/runbooks/batch-adoption-repair.md +++ b/docs/runbooks/batch-adoption-repair.md @@ -51,6 +51,33 @@ record held: ``` Stop if batch 2 is not `merged` with `pr_number` 143, or if a batch newer than 2 exists: the state has moved on and this procedure no longer fits. + + **STOP check: no sync ran after the bad settle.** A `pr-batch` run after + the settle syncs the line by diffing from batch 2's tip. That sync + snapshot leaves out the two lands' content, so a DB-only reset would then + give a PR that lists 2 lands but carries none of their files. Both checks + must pass: + + a. `synced` is unchanged: `hex(synced)` from `batch_lines` equals + `hex(base)` of batch 2. (Batch 2 was cut on the line's newest sync. A + later sync replaces `synced` with the newer base tip.) + b. No sync changeset sits after batch 2 on the line. From a graft client + pointed at graft-dev, repo `graft`, list the line's first-parent history + down to batch 2's tip: + ```sh + graft log --at backlog-1 --limit 50 --json + ``` + Look at every entry above batch 2's tip (its id is `hex(tip)` from + step 3, lower case). None may have a subject starting + `graft: sync line backlog-1 onto`. If the tip is not in the first 50, + page on with `--cursor` from the output until it is. + + **If either check fails, do not run step 4.** Restart the mirror + (`docker start graft-dev-mirror-graft`), leave the DB as it is, and hand + the work item back to oversight. That state needs the line repaired in + graft first: the two lands re-applied onto the line, for example with + `graft pick`, so the next batch carries them as new lands. That is a + separate, operator-approved fix. 4. Undo the settle, in one transaction: ```sql BEGIN; @@ -81,10 +108,26 @@ record held: now has both a branch and PR #143, so it is skipped) with a **new** PR listing both lands (c5277958105f2 for WI-2548, and the WI-2542 land). PR #143 is not touched. + + **Verify the content, not just the list.** Gather the files each land + changed (`graft show ` for each of the two), then the new PR's + files (`gh pr diff --name-only -R TheDancingDeveloper-org/graft`). + Every file of both lands must appear in the PR diff, with the lands' + changes. If a file is missing, the PR lists lands it does not deliver: + close it unmerged, mark its batch rejected the same way as step 4, and + hand back to oversight as in step 3. 6. Optional tidy-up on GitHub: the branch `graft/batch-2` now holds the bad run's commits under the merged PR #143. Delete it once the new PR is open; nothing references it. +## Crash window (known, safe) + +If `pr-batch` crashes after GitHub creates the batch's PR but before the DB +records it, and that PR is merged or closed before the next run, the retry +does not adopt it. The batch is rejected and its lands go in the next batch. +If the PR had merged, close the follow-up PR, whose changes main already has. +Lands are never marked delivered without a PR the batch opened. + ## Other repos The same shape applies to any line whose batch settled against a PR it did