From b217c7a76bcbba9b449218e310a2be76f99479ca Mon Sep 17 00:00:00 2001 From: Eugene Blikh Date: Wed, 22 Jul 2026 14:15:28 +0300 Subject: [PATCH] refactor: move the reconciler's two deletes down to the layers that own them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reconciler reached past its layer twice, because the primitives it needed did not exist: a raw DELETE FROM proposal — the only SQL written outside db/ — and a go-git RemoveReference under gitx's write lock. Both move down, with no change in behaviour. db.Store.DeleteOpenProposal keeps the guard in the statement, as resolveProposal does, and distinguishes the two ways it can bite: a row that is gone is ErrNotFound, a row that has been resolved is the new ErrProposalNotOpen, which tells the reconciler "nothing to repair" apart from "the repair no longer applies". gitx.Repo.DeleteProposalBranch refuses anything outside proposals/* — the only thing between a caller bug and a deleted approved branch — takes the per-space write lock like every other write, and treats an already-absent branch as success: the repair is a postcondition, and the ref may legitimately vanish between the listing that found it and the delete. --- db/proposal.go | 49 ++++++++++++++++++++++++++++++++++++ db/proposal_test.go | 55 ++++++++++++++++++++++++++++++++++++++++ db/store.go | 7 ++++++ gitx/write.go | 41 ++++++++++++++++++++++++++++++ gitx/write_test.go | 46 +++++++++++++++++++++++++++++++++ service/reconcile.go | 60 ++++++-------------------------------------- 6 files changed, 206 insertions(+), 52 deletions(-) diff --git a/db/proposal.go b/db/proposal.go index 7400d46541f51a70af999f35cae554821676b6d7..ff88767e5676d168178727880bcf20a396f70736 100644 --- a/db/proposal.go +++ b/db/proposal.go @@ -258,6 +258,55 @@ WHERE id = $1 AND state = $6` return fmt.Errorf("proposal %d: guarded update matched no row while state is %s", id, from) } +// DeleteOpenProposal removes an open proposal row outright. +// +// It is the only delete in this package and it is deliberately narrow: the +// reconciler's repair for a row whose branch never appeared, where the daemon +// died between the row insert and the branch write. Such a row holds no content +// — the agent still has the document it wanted to write and re-proposes — so +// deleting it loses nothing. A resolved proposal is history and is never +// deleted, which is why this is not a general-purpose delete. +// +// The `state = 'open'` guard is in the statement, not in Go: a check-then-write +// would let a merge land in between and delete the row of a proposal that had +// just succeeded. When the guard bites, the current state is read back only to +// name it in the error — ErrNotFound when the row is gone, ErrProposalNotOpen +// when it has been resolved — exactly as resolveProposal does. +func (s *Store) DeleteOpenProposal(ctx context.Context, id int) error { + const q = `DELETE FROM proposal WHERE id = $1 AND state = $2` + res, err := s.q.ExecContext(ctx, q, id, string(core.StateOpen)) + if err != nil { + return fmt.Errorf("delete proposal %d: %w", id, err) + } + n, err := res.RowsAffected() + if err != nil { + return fmt.Errorf("delete proposal %d: rows affected: %w", id, err) + } + if n == 1 { + return nil + } + + // Nothing was deleted: either the proposal is gone, or it is no longer open. + var current string + err = s.q.QueryRowContext(ctx, `SELECT state FROM proposal WHERE id = $1`, id).Scan(¤t) + if errors.Is(err, sql.ErrNoRows) { + return ErrNotFound + } + if err != nil { + return fmt.Errorf("delete proposal %d: read current state: %w", id, err) + } + from, err := core.ParseProposalState(current) + if err != nil { + return fmt.Errorf("proposal %d: %w", id, err) + } + if from == core.StateOpen { + // The row is open, yet the guarded DELETE matched nothing. That cannot + // happen; refuse rather than report success. + return fmt.Errorf("proposal %d: guarded delete matched no row while state is %s", id, from) + } + return fmt.Errorf("%w: proposal %d is %s", ErrProposalNotOpen, id, from) +} + // MergeProposal records a merge: the proposal's transition to merged and the // new registry location of every document it landed, atomically. // diff --git a/db/proposal_test.go b/db/proposal_test.go index a34e72fe5337adc596f617b33b0a9fcf0f961c2e..7c37ed0b1ff3ca64e6f469ae762e6755b8797ebc 100644 --- a/db/proposal_test.go +++ b/db/proposal_test.go @@ -176,6 +176,61 @@ func TestProposalTransitionsAreGuarded(t *testing.T) { } } +// TestDeleteOpenProposal covers the reconciler's one destructive repair: the +// row of a proposal whose branch never appeared. The guard is the whole point — +// a merge landing between the reconciler's decision and its write must not let +// it delete a proposal that had just succeeded. +func TestDeleteOpenProposal(t *testing.T) { + s, _, cleanup := newTestStore(t) + defer cleanup() + ctx := context.Background() + + space := mkSpace(t, s, ctx, "bigbes", "rfcs") + + p := mkProposal(t, s, ctx, space.ID, "never got a branch") + if err := s.DeleteOpenProposal(ctx, p.ID); err != nil { + t.Fatalf("delete open proposal: %v", err) + } + if _, err := s.GetProposal(ctx, p.ID); !errors.Is(err, ErrNotFound) { + t.Fatalf("get after delete = %v, want ErrNotFound", err) + } + // A row that is already gone is ErrNotFound, distinct from a row that moved + // on: the reconciler has to tell "nothing to repair" from "the repair no + // longer applies". + if err := s.DeleteOpenProposal(ctx, p.ID); !errors.Is(err, ErrNotFound) { + t.Fatalf("second delete = %v, want ErrNotFound", err) + } + if err := s.DeleteOpenProposal(ctx, 99999); !errors.Is(err, ErrNotFound) { + t.Fatalf("delete of a missing proposal = %v, want ErrNotFound", err) + } + + merged := mkProposal(t, s, ctx, space.ID, "merged under the reconciler") + if err := s.MarkProposalMerged(ctx, merged.ID, core.ApprovalHuman, "deadbeef"); err != nil { + t.Fatalf("merge: %v", err) + } + if err := s.DeleteOpenProposal(ctx, merged.ID); !errors.Is(err, ErrProposalNotOpen) { + t.Fatalf("delete of a merged proposal = %v, want ErrProposalNotOpen", err) + } + got, err := s.GetProposal(ctx, merged.ID) + if err != nil { + t.Fatalf("get merged: %v", err) + } + if got.State != core.StateMerged || got.MergedRev != "deadbeef" { + t.Fatalf("the guarded delete modified the row: %+v", got) + } + + rejected := mkProposal(t, s, ctx, space.ID, "rejected") + if err := s.RejectProposal(ctx, rejected.ID); err != nil { + t.Fatalf("reject: %v", err) + } + if err := s.DeleteOpenProposal(ctx, rejected.ID); !errors.Is(err, ErrProposalNotOpen) { + t.Fatalf("delete of a rejected proposal = %v, want ErrProposalNotOpen", err) + } + if _, err := s.GetProposal(ctx, rejected.ID); err != nil { + t.Fatalf("the guarded delete removed a rejected proposal: %v", err) + } +} + // TestMergeProposalIsAtomic is the reason the transaction exists: a merge that // cannot re-point the registry must not leave the proposal reading merged. func TestMergeProposalIsAtomic(t *testing.T) { diff --git a/db/store.go b/db/store.go index 207b641fd9590682869b73128d27f9313d0fce8a..3b55356903e14e17fba9145af08046f4c8315a78 100644 --- a/db/store.go +++ b/db/store.go @@ -144,6 +144,13 @@ var ( // log can say which happened; both map to 401 at the API boundary. ErrTokenRevoked = errors.New("db: agent token revoked") + // ErrProposalNotOpen is returned by DeleteOpenProposal for a proposal that + // exists but has already been resolved. Kept distinct from ErrNotFound so + // the reconciler can tell "the row is gone", which is the state it wanted, + // from "the proposal merged under us", which means the repair it planned no + // longer applies. + ErrProposalNotOpen = errors.New("db: proposal is not open") + // ErrNoTransaction is returned by InTx when the Store is not bound to // something that can begin one (i.e. it is already inside a transaction). ErrNoTransaction = errors.New("db: store cannot begin a transaction") diff --git a/gitx/write.go b/gitx/write.go index c208d8644ac6c99c0bfadc97f24a835c2dcd0ec2..c3aa835830799870f2236fc752c2a93f3169ef0d 100644 --- a/gitx/write.go +++ b/gitx/write.go @@ -342,6 +342,47 @@ func (r *Repo) CreateProposalBranch(ctx context.Context, branch, base string) (p return head, nil } +// DeleteProposalBranch removes a proposal branch, under the space write lock so +// it excludes the merge path and any other in-process write. +// +// It refuses anything outside proposals/*. That check is not defence in depth, +// it is the only thing standing between a caller bug and a deleted approved +// branch — the approved branch is deleted by nobody, ever, which is also what +// CheckRefUpdate tells receive-pack. +// +// An already-absent branch is success rather than ErrNotFound. The caller is +// the reconciler, whose repair is a postcondition ("this unreferenced ref does +// not exist") and not an action, and the ref can legitimately vanish between +// the listing that found it and this call — a native receive-pack push deleting +// it, or an earlier pass that raced this one. Reporting that as a failure would +// fill the reconcile report with failures for repairs that in fact hold. +// +// There is no compare-and-swap here and no lost-race retry: unlike a ref move, +// a delete does not depend on the value it is replacing, so a concurrent writer +// cannot make it do the wrong thing. It can only make it redundant, which is +// the case above. +func (r *Repo) DeleteProposalBranch(ctx context.Context, branch string) error { + ctx, cancel := r.withTimeout(ctx) + defer cancel() + + if !IsProposalBranch(branch) { + return fmt.Errorf("%w: %q is not a %s* branch; only a proposal branch may be deleted", + ErrBadRev, branch, ProposalPrefix) + } + + unlock, err := r.lock(ctx) + if err != nil { + return err + } + defer unlock() + + name := plumbing.NewBranchReferenceName(branch) + if err := r.repo.Storer.RemoveReference(name); err != nil { + return fmt.Errorf("gitx: delete %s in %s: %w", name, r.ref, err) + } + return nil +} + // CommitProposal commits whole-document blobs onto a proposal branch. // // It refuses any branch outside proposals/*: the approved branch moves in diff --git a/gitx/write_test.go b/gitx/write_test.go index 8ca8b9c67192b3471fb5040051d1755daae3306b..8fccb4b0b24f4854ad0de6505cee9254e529578e 100644 --- a/gitx/write_test.go +++ b/gitx/write_test.go @@ -47,6 +47,52 @@ func TestCreateProposalBranch(t *testing.T) { } } +func TestDeleteProposalBranch(t *testing.T) { + repo, _ := newSpace(t) + ctx := context.Background() + + base := pushApproved(t, repo, ownerMeta("seed", 1), + Write{Path: "specs/0007.md", Content: doc("SPEC-0007", "Storage", "v1")}) + openProposal(t, repo, "proposals/1", base.String(), meta("revise 0007", 2), + Write{Path: "specs/0007.md", Content: doc("SPEC-0007", "Storage", "v2")}) + + if err := repo.DeleteProposalBranch(ctx, "proposals/1"); err != nil { + t.Fatalf("DeleteProposalBranch: %v", err) + } + branches, err := repo.ListProposalBranches(ctx) + if err != nil { + t.Fatalf("ListProposalBranches: %v", err) + } + if len(branches) != 0 { + t.Fatalf("branches = %+v, want none", branches) + } + if _, err := repo.BranchHead(ctx, "proposals/1"); !errors.Is(err, ErrNotFound) { + t.Fatalf("BranchHead after the delete = %v, want ErrNotFound", err) + } + + // An already-absent branch is success, not ErrNotFound: the reconciler's + // repair is a postcondition, and the ref may vanish between the listing + // that found it and the delete. Asserted here rather than inherited from + // go-git, so the contract is this package's. + if err := repo.DeleteProposalBranch(ctx, "proposals/1"); err != nil { + t.Fatalf("second DeleteProposalBranch = %v, want success", err) + } + if err := repo.DeleteProposalBranch(ctx, "proposals/404"); err != nil { + t.Fatalf("DeleteProposalBranch of a branch that never existed = %v, want success", err) + } + + // The namespace check is the only thing standing between a caller bug and a + // deleted approved branch. + for _, b := range []string{repo.ApprovedBranch(), "scratch", "proposals", "", "refs/heads/proposals/1"} { + if err := repo.DeleteProposalBranch(ctx, b); !errors.Is(err, ErrBadRev) { + t.Fatalf("DeleteProposalBranch(%q) = %v, want ErrBadRev", b, err) + } + } + if _, err := repo.ApprovedHead(ctx); err != nil { + t.Fatalf("approved branch is gone: %v", err) + } +} + func TestCommitProposalSplicesWholeDocuments(t *testing.T) { repo, _ := newSpace(t) ctx := context.Background() diff --git a/service/reconcile.go b/service/reconcile.go index 4911e84eb3947b4444b9d4b09c6bc1ddae2451f4..ee2a26d4907eea11263a5348e27f4c7f9c0a4185 100644 --- a/service/reconcile.go +++ b/service/reconcile.go @@ -6,7 +6,6 @@ import ( "fmt" "time" - "github.com/go-git/go-git/v5" "github.com/go-git/go-git/v5/plumbing" "sourcecraft.dev/bigbes/sr-ht-spec/core" @@ -473,7 +472,7 @@ func (s *Service) applyRepair(ctx context.Context, sp *Space, r Repair) error { case RepairMarkMerged: return s.store.MarkProposalMerged(ctx, r.ProposalID, r.Approval, r.Rev) case RepairDeleteRow: - return s.deleteOpenProposalRow(ctx, r.ProposalID) + return s.store.DeleteOpenProposal(ctx, r.ProposalID) case RepairDeleteRef: return s.deleteProposalRef(ctx, sp, r.Branch) default: @@ -481,60 +480,17 @@ func (s *Service) applyRepair(ctx context.Context, sp *Space, r Repair) error { } } -// deleteOpenProposalRow removes a proposal row that never got a branch. -// -// The SQL is written here rather than called on db.Store because db/ exposes no -// delete — the corrected repair table needs one and the persistence layer was -// built before the table was corrected. It belongs in db/ as DeleteOpenProposal -// and should move there; it lives in this file so that the repair the design -// specifies actually happens rather than silently not happening. -// -// The `state = 'open'` guard is in the statement, not in Go: a check-then-write -// would let a merge land in between and delete the row of a proposal that had -// just succeeded. -func (s *Service) deleteOpenProposalRow(ctx context.Context, id int) error { - const q = `DELETE FROM proposal WHERE id = $1 AND state = 'open'` - res, err := s.q.ExecContext(ctx, q, id) - if err != nil { - return fmt.Errorf("service: delete proposal %d: %w", id, err) - } - n, err := res.RowsAffected() - if err != nil { - return fmt.Errorf("service: delete proposal %d: rows affected: %w", id, err) - } - if n == 0 { - return fmt.Errorf("service: proposal %d is no longer open; not deleted", id) - } - return nil -} - // deleteProposalRef removes an unreferenced proposals/* branch. // -// Like the row delete above, this primitive is missing from the layer that -// should own it: gitx exposes branch creation and the merge but no delete. It -// is written here against go-git directly, under gitx's own per-space write -// lock, so it still excludes the merge path and a concurrent in-process write — -// but it should move into gitx, where the compare-and-swap retry logic for the -// two-writer problem already lives. -// -// The namespace check is not defence in depth, it is the only thing standing -// between a caller bug and a deleted approved branch. +// The namespace check, the per-space write lock and the choice to treat an +// already-absent branch as success all live in gitx.DeleteProposalBranch, which +// owns refs. What is left here is naming the space the failure belongs to, so +// the reconcile report says which repository could not be repaired. func (s *Service) deleteProposalRef(ctx context.Context, sp *Space, branch string) error { - if !gitx.IsProposalBranch(branch) { - return fmt.Errorf("service: refusing to delete %q: only %s* branches are deletable", - branch, gitx.ProposalPrefix) + if err := sp.Repo.DeleteProposalBranch(ctx, branch); err != nil { + return fmt.Errorf("service: delete branch %q in %s: %w", branch, sp.Ref, err) } - return sp.Repo.WithLock(ctx, func(ctx context.Context) error { - repo, err := git.PlainOpen(sp.Repo.Dir()) - if err != nil { - return fmt.Errorf("service: open %s: %w", sp.Repo.Dir(), err) - } - name := plumbing.NewBranchReferenceName(branch) - if err := repo.Storer.RemoveReference(name); err != nil { - return fmt.Errorf("service: delete %s in %s: %w", name, sp.Ref, err) - } - return nil - }) + return nil } // RunReconciler runs the reconciler at startup and then on a ticker, until ctx