deploy: gate config merges on ancestry, CAS the applied/main move
A config deploy could silently discard committed agent config. sock's icon commit carried a full proposal/approved/building/deployed tag set yet was not an ancestor of `main` — genuinely deployed, then dropped. Two gaps compounded. `prepare_applied_target` is documented as "fast-forward applied/main to target", but `git_update_ref` is `git update-ref <ref> <target>` with no old-value guard: an unconditional force move. Anything reachable from the old `main` but not from `target` leaves the branch without a word. And nothing checked that it *was* a fast-forward. `run_deploy_merge_verify` asserts exactly one thing about history — that the live PR head still equals the reviewed sha. That is a drift gate on the *head*; it says nothing about the *base*. A PR opened from a stale base passes it unchanged and then rewinds `main` when it lands. Adds, in the order they run: - an ancestry gate as step 3 of MergeVerify — the reviewed head must descend from `applied/main`, else bail before the irreversible merge. It sits after the fetch (the commit has to be local to check reachability) and before the eval, so it stays inside the region where nothing is mutated and the node is still safely retryable. - `git_update_ref_cas`, used for the `applied/main` move. `git update-ref <ref> <new> <old>` refuses, and leaves the ref alone, when the current value is not `old`. The ancestry gate only proves the target is safe against the `main` observed *then*; the CAS is what keeps that proof true *now*. `run_deploy_apply` already reads `prev_main` to park the rollback ref, so that value is threaded in — re-reading it inside the callee would reintroduce the race. `git_is_ancestor` returns `Ok(false)` for exit 1 rather than treating "not an ancestor" as a failure. Its doc comment notes this is not the "did this branch land upstream" question: a squash-merge rewrites the commit, so `--is-ancestor` correctly answers false for a branch whose contents were merged. Different question, same command. Tests cover both directions of the ancestry check, and that a stale CAS both errors *and* leaves the ref where it was — a guard that fails while still moving the ref would be worse than none. Not covered here, deliberately: the non-PR apply path also writes `main` and wants the same treatment. Kept separate to stay reviewable.
This commit is contained in:
parent
1db3cc32a1
commit
6973610d39
4 changed files with 185 additions and 10 deletions
|
|
@ -198,7 +198,14 @@ fn deploy_ctx(coord: &Coordinator, approval_id: i64) -> Result<DeployCtx> {
|
|||
/// (the operator must re-review the new head);
|
||||
/// 2. fetch the reviewed head into the applied repo so later git ops resolve
|
||||
/// it locally;
|
||||
/// 3. eval-verify the reviewed commit against the meta flake.
|
||||
/// 3. ancestry gate — the reviewed head must descend from `applied/main`;
|
||||
/// 4. eval-verify the reviewed commit against the meta flake.
|
||||
///
|
||||
/// Steps 1 and 3 ask different questions and both are load-bearing. The drift
|
||||
/// gate asks whether the *head* is still what was reviewed; the ancestry gate
|
||||
/// asks whether the *base* is still underneath it. A PR opened from a stale base
|
||||
/// passes the drift gate untouched and then rewinds `main` when it lands,
|
||||
/// silently dropping every commit made in between.
|
||||
///
|
||||
/// Nothing here needs undoing on failure: the fetch only adds objects, and
|
||||
/// `main` doesn't move. That's the whole reason this is its own node — a
|
||||
|
|
@ -208,9 +215,9 @@ fn deploy_ctx(coord: &Coordinator, approval_id: i64) -> Result<DeployCtx> {
|
|||
///
|
||||
/// Returns an error if the approval can't be loaded, if the live PR head has
|
||||
/// drifted from the reviewed sha, if fetching that head into the applied repo
|
||||
/// fails, or if the eval-verify of the reviewed commit fails. Every one of
|
||||
/// these leaves the forge and `main` untouched, so the node is safely
|
||||
/// retryable.
|
||||
/// fails, if the reviewed head does not descend from `applied/main`, or if the
|
||||
/// eval-verify of the reviewed commit fails. Every one of these leaves the forge
|
||||
/// and `main` untouched, so the node is safely retryable.
|
||||
pub async fn run_deploy_merge_verify(coord: &Arc<Coordinator>, approval_id: i64) -> Result<()> {
|
||||
let ctx = deploy_ctx(coord, approval_id)?;
|
||||
let pr = ctx.pr;
|
||||
|
|
@ -231,7 +238,23 @@ pub async fn run_deploy_merge_verify(coord: &Arc<Coordinator>, approval_id: i64)
|
|||
.await
|
||||
.map_err(|e| anyhow::anyhow!("fetch PR #{pr} head into applied: {e}"))?;
|
||||
|
||||
// 3. Eval-verify BEFORE the irreversible merge (bad nix fails fast here).
|
||||
// 3. Ancestry gate: `main` must be reachable from the reviewed head, or the
|
||||
// "fast-forward" in prepare_applied_target is really a rewind that drops
|
||||
// every commit between the PR's base and where `main` actually is now.
|
||||
let current_main = lifecycle::git_rev_parse(&ctx.applied_dir, "refs/heads/main")
|
||||
.await
|
||||
.map_err(|e| anyhow::anyhow!("read applied/main: {e:#}"))?;
|
||||
if !lifecycle::git_is_ancestor(&ctx.applied_dir, ¤t_main, reviewed)
|
||||
.await
|
||||
.map_err(|e| anyhow::anyhow!("ancestry check {current_main}..{reviewed}: {e:#}"))?
|
||||
{
|
||||
bail!(
|
||||
"PR #{pr} does not descend from applied/main (main {current_main}, reviewed {reviewed}); \
|
||||
merging it would discard commits — rebase the PR onto main and re-review"
|
||||
);
|
||||
}
|
||||
|
||||
// 4. Eval-verify BEFORE the irreversible merge (bad nix fails fast here).
|
||||
crate::meta::verify_commit(ctx.approval.agent.as_str(), &ctx.applied_dir, reviewed)
|
||||
.await
|
||||
.map_err(|e| anyhow::anyhow!("verify merge head {reviewed}: {e:#}"))?;
|
||||
|
|
@ -282,7 +305,7 @@ pub async fn run_deploy_apply(coord: &Arc<Coordinator>, approval_id: i64) -> Res
|
|||
Err(e) => bail!("ff-merge PR #{pr}: {e}"),
|
||||
}
|
||||
|
||||
prepare_applied_target(agent, &ctx.applied_dir, &ctx.reviewed).await
|
||||
prepare_applied_target(agent, &ctx.applied_dir, &ctx.reviewed, &prev_main).await
|
||||
}
|
||||
|
||||
/// `DeployTail` node body — compensation + bookkeeping, `AfterAny` the apply
|
||||
|
|
@ -771,13 +794,23 @@ async fn prepare_applied_target(
|
|||
agent: &str,
|
||||
applied_dir: &std::path::Path,
|
||||
target: &str,
|
||||
expected_main: &str,
|
||||
) -> Result<()> {
|
||||
// Fast-forward applied/main to target + sync the working tree. Meta input
|
||||
// pins `?ref=main`, so this is what makes nix re-lock to the target commit
|
||||
// on the prepare_deploy step below.
|
||||
lifecycle::git_update_ref(applied_dir, "refs/heads/main", target)
|
||||
//
|
||||
// Compare-and-swap, not a bare set: `main` must still be the sha the caller
|
||||
// read before the merge. A plain `update-ref` here moves the branch to
|
||||
// `target` whatever it currently points at, which turns "fast-forward" into
|
||||
// "discard anything that landed in the meantime" — the ancestry gate in
|
||||
// run_deploy_merge_verify only proves the target is safe against the `main`
|
||||
// observed *then*, so this is what makes that proof still true *now*.
|
||||
lifecycle::git_update_ref_cas(applied_dir, "refs/heads/main", target, expected_main)
|
||||
.await
|
||||
.map_err(|e| anyhow::anyhow!("ff main to {target}: {e:#}"))?;
|
||||
.map_err(|e| {
|
||||
anyhow::anyhow!("ff main {expected_main} -> {target} (concurrent move?): {e:#}")
|
||||
})?;
|
||||
lifecycle::git_read_tree_reset(applied_dir, "refs/heads/main")
|
||||
.await
|
||||
.map_err(|e| anyhow::anyhow!("read-tree to main: {e:#}"))?;
|
||||
|
|
|
|||
Loading…
Reference in a new issue