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..0e6a7acd 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,58 @@ 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 + } + // 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, + 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..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) @@ -2036,3 +2063,152 @@ 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); +} + +/// 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 new file mode 100644 index 00000000..6f52cbda --- /dev/null +++ b/docs/runbooks/batch-adoption-repair.md @@ -0,0 +1,138 @@ +# 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. + + **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; + 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. + + **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 +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.