fix(kvstore): make merge request writes depend on the repository key so a delete cannot leave stray keys

commit: 50c6964b8620331af124216870564118861e985f
author: brice · 11d ago · 2026-10-10 17:40 UTC
parent: c7d4ad1
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

4 files · +183 -32

M internal/adapters/kvstore/mergerequests.go +53 -14


@@ -56,10 +56,9 @@ func readKey(repo domain.RepoName, n int64) []byte {
56	56		}
57	57		func seqKey(repo domain.RepoName) []byte { return []byte(seqPrefix + repo.String()) }
58	58
59		-	// mergeRequestKeys lists every key that belongs to the repository's merge
60		-	// requests.
61		-	func (s *Store) mergeRequestKeys(ctx context.Context, repo domain.RepoName) [][]byte {
62		-	snap := s.db.Snapshot(ctx)
	59	+	// mergeRequestKeys lists every key of the snapshot that belongs to the
	60	+	// repository's merge requests.
	61	+	func mergeRequestKeys(ctx context.Context, snap *kv.Snapshot, repo domain.RepoName) [][]byte {
63	62		var keys [][]byte
64	63		for _, p := range []string{mrRepoPrefix(repo), commentPrefix + repo.String() + "/", readPrefix + repo.String() + "/"} {
65	64		for k := range snap.Prefix(ctx, []byte(p)) {

@@ -72,12 +71,34 @@ func (s *Store) mergeRequestKeys(ctx context.Context, repo domain.RepoName) [][]
72	71		return keys
73	72		}
74	73
	74	+	// guardRepo makes the batch depend on the repository: it requires the
	75	+	// repository key to be at the version just read and rewrites it, which gives it
	76	+	// a new version. Every write that adds a key under the repository's merge
	77	+	// requests (a request, a comment, a read position) goes through it, so that
	78	+	//
	79	+	// - such a write fails with domain.ErrNotFound once the repository is deleted,
	80	+	// and leaves no key that a repository created again under the name would
	81	+	// inherit;
	82	+	// - Store.Delete, which requires the version of the repository key it listed
	83	+	// the keys at, notices a write that got in after the listing and lists again.
	84	+	//
	85	+	// It returns domain.ErrNotFound when the repository does not exist.
	86	+	func (m MergeRequests) guardRepo(ctx context.Context, b kv.Batch, repo domain.RepoName) error {
	87	+	item, ok := m.db.Get(ctx, repoKey(repo))
	88	+	if !ok {
	89	+	return domain.ErrNotFound
	90	+	}
	91	+	b.RequireVersion(repoKey(repo), item.Version)
	92	+	b.Put(repoKey(repo), item.Value)
	93	+	return nil
	94	+	}
	95	+
75	96		// Create takes the next number of the repository from mrseq/<repo> and stores
76	97		// the merge request under it, in one batch that requires the sequence to be
77	98		// unchanged since it was read: two concurrent creations never share a number.
78		-	// The batch also requires the repository to be unchanged since it was read, so
79		-	// that a repository deleted meanwhile is not left with a stray merge request
80		-	// (a push recording pushed_at makes it retry). A missing repository gives
	99	+	// The batch also requires the repository to be unchanged since it was read and
	100	+	// rewrites it (see guardRepo), so that a repository deleted meanwhile is not
	101	+	// left with a stray merge request (a push recording pushed_at makes it retry). A missing repository gives
81	102		// domain.ErrNotFound.
82	103		//
83	104		//hexcheck:ignore suspicious-business-logic-in-adapter takes the next number under a version precondition: a storage concern, no policy

@@ -103,6 +124,7 @@ func (m *MergeRequests) Create(ctx context.Context, mr domain.MergeRequest) (dom
103	124		}
104	125		var b kv.Batch
105	126		b.RequireVersion(repoKey(mr.Repo), repoItem.Version)
	127	+	b.Put(repoKey(mr.Repo), repoItem.Value) // see guardRepo
106	128		if ok {
107	129		b.RequireVersion(seqKey(mr.Repo), item.Version)
108	130		} else {

@@ -202,7 +224,8 @@ func (m *MergeRequests) GetComment(ctx context.Context, repo domain.RepoName, nu
202	224		}
203	225
204	226		// SaveComment writes the comment and the merge request in one batch, each with
205		-	// its version precondition (absent for a new comment).
	227	+	// its version precondition (absent for a new comment), and requires the
	228	+	// repository to exist (see guardRepo): domain.ErrNotFound otherwise.
206	229		func (m *MergeRequests) SaveComment(ctx context.Context, mr domain.MergeRequest, c domain.Comment) error {
207	230		mrVal, err := encodeMergeRequest(mr)
208	231		if err != nil {

@@ -216,6 +239,9 @@ func (m *MergeRequests) SaveComment(ctx context.Context, mr domain.MergeRequest,
216	239		return fmt.Errorf("save comment on %d of %q: read the merge request first: %w", mr.Number, mr.Repo, domain.ErrConflict)
217	240		}
218	241		var b kv.Batch
	242	+	if err := m.guardRepo(ctx, &b, mr.Repo); err != nil {
	243	+	return err
	244	+	}
219	245		b.RequireVersion(mrKey(mr.Repo, mr.Number), mr.Version)
220	246		b.Put(mrKey(mr.Repo, mr.Number), mrVal)
221	247		if c.Version == 0 {

@@ -246,17 +272,30 @@ func (m *MergeRequests) GetRead(ctx context.Context, repo domain.RepoName, numbe
246	272		return id, nil
247	273		}
248	274
249		-	// SetRead writes mrread/<repo>/<number>.
	275	+	// SetRead writes mrread/<repo>/<number>. The repository must exist (see
	276	+	// guardRepo): domain.ErrNotFound otherwise.
	277	+	//
	278	+	//hexcheck:ignore suspicious-business-logic-in-adapter retries a write that lost a race with the repository key: a storage concern, no policy
250	279		func (m *MergeRequests) SetRead(ctx context.Context, repo domain.RepoName, number int64, head domain.ObjectID) error {
251	280		if head.IsZero() {
252	281		return fmt.Errorf("%w: empty read position", domain.ErrInvalidObjectID)
253	282		}
254		-	var b kv.Batch
255		-	b.Put(readKey(repo, number), []byte(head.String()))
256		-	if _, err := m.db.Apply(ctx, &b); err != nil {
257		-	return fmt.Errorf("record read position of %d in %q: %w", number, repo, err)
	283	+	for range createTries {
	284	+	var b kv.Batch
	285	+	if err := m.guardRepo(ctx, &b, repo); err != nil {
	286	+	return err
	287	+	}
	288	+	b.Put(readKey(repo, number), []byte(head.String()))
	289	+	_, err := m.db.Apply(ctx, &b)
	290	+	if errors.Is(err, kv.ErrConflict) {
	291	+	continue
	292	+	}
	293	+	if err != nil {
	294	+	return fmt.Errorf("record read position of %d in %q: %w", number, repo, err)
	295	+	}
	296	+	return nil
258	297		}
259		-	return nil
	298	+	return fmt.Errorf("record read position of %d in %q: %w", number, repo, domain.ErrConflict)
260	299		}
261	300
262	301		func encodeMergeRequest(mr domain.MergeRequest) ([]byte, error) {

 }
 func seqKey(repo domain.RepoName) []byte { return []byte(seqPrefix + repo.String()) }
-// mergeRequestKeys lists every key that belongs to the repository's merge
+// mergeRequestKeys lists every key of the snapshot that belongs to the
-// requests.
+// repository's merge requests.
-func (s *Store) mergeRequestKeys(ctx context.Context, repo domain.RepoName) [][]byte {
+func mergeRequestKeys(ctx context.Context, snap *kv.Snapshot, repo domain.RepoName) [][]byte {
-	snap := s.db.Snapshot(ctx)
 	var keys [][]byte
 	for _, p := range []string{mrRepoPrefix(repo), commentPrefix + repo.String() + "/", readPrefix + repo.String() + "/"} {
 		for k := range snap.Prefix(ctx, []byte(p)) {
 	return keys
 }
+// guardRepo makes the batch depend on the repository: it requires the
+// repository key to be at the version just read and rewrites it, which gives it
+// a new version. Every write that adds a key under the repository's merge
+// requests (a request, a comment, a read position) goes through it, so that
+//
+//   - such a write fails with domain.ErrNotFound once the repository is deleted,
+//     and leaves no key that a repository created again under the name would
+//     inherit;
+//   - Store.Delete, which requires the version of the repository key it listed
+//     the keys at, notices a write that got in after the listing and lists again.
+//
+// It returns domain.ErrNotFound when the repository does not exist.
+func (m *MergeRequests) guardRepo(ctx context.Context, b *kv.Batch, repo domain.RepoName) error {
+	item, ok := m.db.Get(ctx, repoKey(repo))
+	if !ok {
+		return domain.ErrNotFound
+	}
+	b.RequireVersion(repoKey(repo), item.Version)
+	b.Put(repoKey(repo), item.Value)
+	return nil
+}
 // Create takes the next number of the repository from mrseq/<repo> and stores
 // the merge request under it, in one batch that requires the sequence to be
 // unchanged since it was read: two concurrent creations never share a number.
-// The batch also requires the repository to be unchanged since it was read, so
+// The batch also requires the repository to be unchanged since it was read and
-// that a repository deleted meanwhile is not left with a stray merge request
+// rewrites it (see guardRepo), so that a repository deleted meanwhile is not
-// (a push recording pushed_at makes it retry). A missing repository gives
+// left with a stray merge request (a push recording pushed_at makes it retry). A missing repository gives
 // domain.ErrNotFound.
 //
 //hexcheck:ignore suspicious-business-logic-in-adapter takes the next number under a version precondition: a storage concern, no policy
 		}
 		var b kv.Batch
 		b.RequireVersion(repoKey(mr.Repo), repoItem.Version)
+		b.Put(repoKey(mr.Repo), repoItem.Value) // see guardRepo
 		if ok {
 			b.RequireVersion(seqKey(mr.Repo), item.Version)
 		} else {
 }
 // SaveComment writes the comment and the merge request in one batch, each with
-// its version precondition (absent for a new comment).
+// its version precondition (absent for a new comment), and requires the
+// repository to exist (see guardRepo): domain.ErrNotFound otherwise.
 func (m *MergeRequests) SaveComment(ctx context.Context, mr domain.MergeRequest, c domain.Comment) error {
 	mrVal, err := encodeMergeRequest(mr)
 	if err != nil {
 		return fmt.Errorf("save comment on %d of %q: read the merge request first: %w", mr.Number, mr.Repo, domain.ErrConflict)
 	}
 	var b kv.Batch
+	if err := m.guardRepo(ctx, &b, mr.Repo); err != nil {
+		return err
+	}
 	b.RequireVersion(mrKey(mr.Repo, mr.Number), mr.Version)
 	b.Put(mrKey(mr.Repo, mr.Number), mrVal)
 	if c.Version == 0 {
 	return id, nil
 }
-// SetRead writes mrread/<repo>/<number>.
+// SetRead writes mrread/<repo>/<number>. The repository must exist (see
+// guardRepo): domain.ErrNotFound otherwise.
+//
+//hexcheck:ignore suspicious-business-logic-in-adapter retries a write that lost a race with the repository key: a storage concern, no policy
 func (m *MergeRequests) SetRead(ctx context.Context, repo domain.RepoName, number int64, head domain.ObjectID) error {
 	if head.IsZero() {
 		return fmt.Errorf("%w: empty read position", domain.ErrInvalidObjectID)
 	}
-	var b kv.Batch
+	for range createTries {
-	b.Put(readKey(repo, number), []byte(head.String()))
+		var b kv.Batch
-	if _, err := m.db.Apply(ctx, &b); err != nil {
+		if err := m.guardRepo(ctx, &b, repo); err != nil {
-		return fmt.Errorf("record read position of %d in %q: %w", number, repo, err)
+			return err
+		}
+		b.Put(readKey(repo, number), []byte(head.String()))
+		_, err := m.db.Apply(ctx, &b)
+		if errors.Is(err, kv.ErrConflict) {
+			continue
+		}
+		if err != nil {
+			return fmt.Errorf("record read position of %d in %q: %w", number, repo, err)
+		}
+		return nil
 	}
-	return nil
+	return fmt.Errorf("record read position of %d in %q: %w", number, repo, domain.ErrConflict)
 }
 func encodeMergeRequest(mr domain.MergeRequest) ([]byte, error) {

M internal/adapters/kvstore/mergerequests_test.go +84 -0


@@ -4,6 +4,7 @@ package kvstore_test
4	4
5	5		import (
6	6		"context"
	7	+	"errors"
7	8		"strings"
8	9		"sync"
9	10		"testing"

@@ -321,6 +322,89 @@ func TestDeleteDropsTheMergeRequestsOfTheRepository(t *testing.T) {
321	322		assert.Equal(t, int64(1), again.Number)
322	323		}
323	324
	325	+	// TestWritesAfterDeleteLeaveNothing: a comment or a read position written for
	326	+	// a repository that was deleted meanwhile is refused, so that a repository
	327	+	// created again under the name does not inherit it (GO-002).
	328	+	func TestWritesAfterDeleteLeaveNothing(t *testing.T) {
	329	+	ctx := context.Background()
	330	+	s, mrs, n := repoWithMRs(t)
	331	+	mr, err := mrs.Create(ctx, newMR(t, n, "t"))
	332	+	require.NoError(t, err)
	333	+
	334	+	require.NoError(t, s.Delete(ctx, n))
	335	+
	336	+	mr.CommentSeq = 1
	337	+	assert.ErrorIs(t, mrs.SaveComment(ctx, mr, newComment(t, 1, "late")), domain.ErrNotFound)
	338	+	assert.ErrorIs(t, mrs.SetRead(ctx, n, mr.Number, oid('a')), domain.ErrNotFound)
	339	+	_, err = mrs.Create(ctx, newMR(t, n, "late"))
	340	+	assert.ErrorIs(t, err, domain.ErrNotFound)
	341	+
	342	+	require.NoError(t, s.Create(ctx, domain.Repo{Name: n, CreatedAt: created, Visibility: domain.VisibilityPublic}))
	343	+	assertNoMergeRequestKeys(t, mrs, n)
	344	+	}
	345	+
	346	+	func assertNoMergeRequestKeys(t testing.T, mrs kvstore.MergeRequests, n domain.RepoName) {
	347	+	t.Helper()
	348	+	ctx := context.Background()
	349	+	list, err := mrs.List(ctx, n)
	350	+	require.NoError(t, err)
	351	+	assert.Empty(t, list, "merge requests")
	352	+	comments, err := mrs.ListComments(ctx, n, 1)
	353	+	require.NoError(t, err)
	354	+	assert.Empty(t, comments, "comments")
	355	+	read, err := mrs.GetRead(ctx, n, 1)
	356	+	require.NoError(t, err)
	357	+	assert.True(t, read.IsZero(), "read position")
	358	+	}
	359	+
	360	+	// TestDeleteRacingWithWrites: writes racing with Delete either land before it
	361	+	// and are deleted, or are refused after it; nothing is left for a repository
	362	+	// created again under the name.
	363	+	func TestDeleteRacingWithWrites(t *testing.T) {
	364	+	ctx := context.Background()
	365	+	for round := range 20 {
	366	+	s, mrs, n := repoWithMRs(t)
	367	+	mr, err := mrs.Create(ctx, newMR(t, n, "t"))
	368	+	require.NoError(t, err)
	369	+
	370	+	var wg sync.WaitGroup
	371	+	for w := range 4 {
	372	+	wg.Add(1)
	373	+	go func() {
	374	+	defer wg.Done()
	375	+	for i := range 30 {
	376	+	if w%2 == 0 {
	377	+	err := mrs.SetRead(ctx, n, mr.Number, oid('a'))
	378	+	if err != nil {
	379	+	assert.ErrorIs(t, err, domain.ErrNotFound)
	380	+	return
	381	+	}
	382	+	continue
	383	+	}
	384	+	cur, err := mrs.Get(ctx, n, mr.Number)
	385	+	if err != nil {
	386	+	return
	387	+	}
	388	+	cur.CommentSeq = int64(w*1000 + i + 1)
	389	+	err = mrs.SaveComment(ctx, cur, newComment(t, cur.CommentSeq, "note"))
	390	+	if err != nil && !assert.True(t, errors.Is(err, domain.ErrConflict) \|\| errors.Is(err, domain.ErrNotFound), "round %d: %v", round, err) {
	391	+	return
	392	+	}
	393	+	}
	394	+	}()
	395	+	}
	396	+	delErr := s.Delete(ctx, n)
	397	+	wg.Wait()
	398	+	require.NoError(t, delErr)
	399	+
	400	+	require.NoError(t, s.Create(ctx, domain.Repo{Name: n, CreatedAt: created, Visibility: domain.VisibilityPublic}))
	401	+	assertNoMergeRequestKeys(t, mrs, n)
	402	+	again, err := mrs.Create(ctx, newMR(t, n, "fresh"))
	403	+	require.NoError(t, err)
	404	+	assert.Equal(t, int64(1), again.Number)
	405	+	}
	406	+	}
	407	+
324	408		func TestDeleteManyMergeRequests(t *testing.T) {
325	409		ctx := context.Background()
326	410		s, mrs, n := repoWithMRs(t)


@@ -4,6 +4,7 @@ package kvstore_test
4		4
5	import (	5		import (
6	"context"	6		"context"
		7	+	"errors"
7	"strings"	8		"strings"
8	"sync"	9		"sync"
9	"testing"	10		"testing"

@@ -321,6 +322,89 @@ func TestDeleteDropsTheMergeRequestsOfTheRepository(t *testing.T) {
321	assert.Equal(t, int64(1), again.Number)	322		assert.Equal(t, int64(1), again.Number)
322	}	323		}
323		324
		325	+	// TestWritesAfterDeleteLeaveNothing: a comment or a read position written for
		326	+	// a repository that was deleted meanwhile is refused, so that a repository
		327	+	// created again under the name does not inherit it (GO-002).
		328	+	func TestWritesAfterDeleteLeaveNothing(t *testing.T) {
		329	+	ctx := context.Background()
		330	+	s, mrs, n := repoWithMRs(t)
		331	+	mr, err := mrs.Create(ctx, newMR(t, n, "t"))
		332	+	require.NoError(t, err)
		333	+
		334	+	require.NoError(t, s.Delete(ctx, n))
		335	+
		336	+	mr.CommentSeq = 1
		337	+	assert.ErrorIs(t, mrs.SaveComment(ctx, mr, newComment(t, 1, "late")), domain.ErrNotFound)
		338	+	assert.ErrorIs(t, mrs.SetRead(ctx, n, mr.Number, oid('a')), domain.ErrNotFound)
		339	+	_, err = mrs.Create(ctx, newMR(t, n, "late"))
		340	+	assert.ErrorIs(t, err, domain.ErrNotFound)
		341	+
		342	+	require.NoError(t, s.Create(ctx, domain.Repo{Name: n, CreatedAt: created, Visibility: domain.VisibilityPublic}))
		343	+	assertNoMergeRequestKeys(t, mrs, n)
		344	+	}
		345	+
		346	+	func assertNoMergeRequestKeys(t testing.T, mrs kvstore.MergeRequests, n domain.RepoName) {
		347	+	t.Helper()
		348	+	ctx := context.Background()
		349	+	list, err := mrs.List(ctx, n)
		350	+	require.NoError(t, err)
		351	+	assert.Empty(t, list, "merge requests")
		352	+	comments, err := mrs.ListComments(ctx, n, 1)
		353	+	require.NoError(t, err)
		354	+	assert.Empty(t, comments, "comments")
		355	+	read, err := mrs.GetRead(ctx, n, 1)
		356	+	require.NoError(t, err)
		357	+	assert.True(t, read.IsZero(), "read position")
		358	+	}
		359	+
		360	+	// TestDeleteRacingWithWrites: writes racing with Delete either land before it
		361	+	// and are deleted, or are refused after it; nothing is left for a repository
		362	+	// created again under the name.
		363	+	func TestDeleteRacingWithWrites(t *testing.T) {
		364	+	ctx := context.Background()
		365	+	for round := range 20 {
		366	+	s, mrs, n := repoWithMRs(t)
		367	+	mr, err := mrs.Create(ctx, newMR(t, n, "t"))
		368	+	require.NoError(t, err)
		369	+
		370	+	var wg sync.WaitGroup
		371	+	for w := range 4 {
		372	+	wg.Add(1)
		373	+	go func() {
		374	+	defer wg.Done()
		375	+	for i := range 30 {
		376	+	if w%2 == 0 {
		377	+	err := mrs.SetRead(ctx, n, mr.Number, oid('a'))
		378	+	if err != nil {
		379	+	assert.ErrorIs(t, err, domain.ErrNotFound)
		380	+	return
		381	+	}
		382	+	continue
		383	+	}
		384	+	cur, err := mrs.Get(ctx, n, mr.Number)
		385	+	if err != nil {
		386	+	return
		387	+	}
		388	+	cur.CommentSeq = int64(w*1000 + i + 1)
		389	+	err = mrs.SaveComment(ctx, cur, newComment(t, cur.CommentSeq, "note"))
		390	+	if err != nil && !assert.True(t, errors.Is(err, domain.ErrConflict) \|\| errors.Is(err, domain.ErrNotFound), "round %d: %v", round, err) {
		391	+	return
		392	+	}
		393	+	}
		394	+	}()
		395	+	}
		396	+	delErr := s.Delete(ctx, n)
		397	+	wg.Wait()
		398	+	require.NoError(t, delErr)
		399	+
		400	+	require.NoError(t, s.Create(ctx, domain.Repo{Name: n, CreatedAt: created, Visibility: domain.VisibilityPublic}))
		401	+	assertNoMergeRequestKeys(t, mrs, n)
		402	+	again, err := mrs.Create(ctx, newMR(t, n, "fresh"))
		403	+	require.NoError(t, err)
		404	+	assert.Equal(t, int64(1), again.Number)
		405	+	}
		406	+	}
		407	+
324	func TestDeleteManyMergeRequests(t *testing.T) {	408		func TestDeleteManyMergeRequests(t *testing.T) {
325	ctx := context.Background()	409		ctx := context.Background()
326	s, mrs, n := repoWithMRs(t)	410		s, mrs, n := repoWithMRs(t)

M internal/adapters/kvstore/repos.go +41 -16


@@ -94,6 +94,10 @@ func (s *Store) Update(ctx context.Context, repo domain.Repo) error {
94	94		// last one, which also removes the repository.
95	95		const deleteChunk = 1000
96	96
	97	+	// deleteTries bounds the listings of Delete: a listing is repeated when a merge
	98	+	// request write got in after it.
	99	+	const deleteTries = 32
	100	+
97	101		// Delete removes the repository and everything keyed to it: its merge
98	102		// requests, their comments and read positions, and its number sequence. The
99	103		// Git data is moved to the trash by the caller; the merge requests are not

@@ -103,28 +107,49 @@ const deleteChunk = 1000
103	107		//
104	108		// The merge request keys go first, in chunks, and the repository key goes with
105	109		// the last chunk: a delete that fails half way leaves the repository, which
106		-	// the caller can delete again.
	110	+	// the caller can delete again. The last batch requires the repository key to
	111	+	// be at the version the keys were listed at. Every write of a merge request key
	112	+	// rewrites the repository key (see MergeRequests.guardRepo), so a write that
	113	+	// got in after the listing makes the batch fail and the keys are listed again,
	114	+	// and a write after the batch finds no repository: no key outlives the
	115	+	// repository to attach itself to a repository created again.
	116	+	//
	117	+	//hexcheck:ignore suspicious-business-logic-in-adapter chunked delete under a version precondition: a storage concern, no policy
107	118		func (s *Store) Delete(ctx context.Context, name domain.RepoName) error {
108		-	keys := s.mergeRequestKeys(ctx, name)
109		-	for len(keys) > deleteChunk {
	119	+	for range deleteTries {
	120	+	snap := s.db.Snapshot(ctx)
	121	+	repoItem, repoOK := snap.Get(ctx, repoKey(name))
	122	+	keys := mergeRequestKeys(ctx, snap, name)
	123	+	for len(keys) > deleteChunk {
	124	+	var b kv.Batch
	125	+	for _, k := range keys[:deleteChunk] {
	126	+	b.Delete(k)
	127	+	}
	128	+	if _, err := s.db.Apply(ctx, &b); err != nil {
	129	+	return fmt.Errorf("delete merge requests of %q: %w", name, err)
	130	+	}
	131	+	keys = keys[deleteChunk:]
	132	+	}
110	133		var b kv.Batch
111		-	for _, k := range keys[:deleteChunk] {
	134	+	if repoOK {
	135	+	b.RequireVersion(repoKey(name), repoItem.Version)
	136	+	} else {
	137	+	b.RequireAbsent(repoKey(name))
	138	+	}
	139	+	for _, k := range keys {
112	140		b.Delete(k)
113	141		}
114		-	if _, err := s.db.Apply(ctx, &b); err != nil {
115		-	return fmt.Errorf("delete merge requests of %q: %w", name, err)
	142	+	b.Delete(repoKey(name))
	143	+	_, err := s.db.Apply(ctx, &b)
	144	+	if errors.Is(err, kv.ErrConflict) {
	145	+	continue
116	146		}
117		-	keys = keys[deleteChunk:]
118		-	}
119		-	var b kv.Batch
120		-	for _, k := range keys {
121		-	b.Delete(k)
122		-	}
123		-	b.Delete(repoKey(name))
124		-	if _, err := s.db.Apply(ctx, &b); err != nil {
125		-	return fmt.Errorf("delete repository %q: %w", name, err)
	147	+	if err != nil {
	148	+	return fmt.Errorf("delete repository %q: %w", name, err)
	149	+	}
	150	+	return nil
126	151		}
127		-	return nil
	152	+	return fmt.Errorf("delete repository %q: %w", name, domain.ErrConflict)
128	153		}
129	154
130	155		func encodeRepo(r domain.Repo) ([]byte, error) {

 // last one, which also removes the repository.
 const deleteChunk = 1000
+// deleteTries bounds the listings of Delete: a listing is repeated when a merge
+// request write got in after it.
+const deleteTries = 32
 // Delete removes the repository and everything keyed to it: its merge
 // requests, their comments and read positions, and its number sequence. The
 // Git data is moved to the trash by the caller; the merge requests are not
 //
 // The merge request keys go first, in chunks, and the repository key goes with
 // the last chunk: a delete that fails half way leaves the repository, which
-// the caller can delete again.
+// the caller can delete again. The last batch requires the repository key to
+// be at the version the keys were listed at. Every write of a merge request key
+// rewrites the repository key (see MergeRequests.guardRepo), so a write that
+// got in after the listing makes the batch fail and the keys are listed again,
+// and a write after the batch finds no repository: no key outlives the
+// repository to attach itself to a repository created again.
+//
+//hexcheck:ignore suspicious-business-logic-in-adapter chunked delete under a version precondition: a storage concern, no policy
 func (s *Store) Delete(ctx context.Context, name domain.RepoName) error {
-	keys := s.mergeRequestKeys(ctx, name)
+	for range deleteTries {
-	for len(keys) > deleteChunk {
+		snap := s.db.Snapshot(ctx)
+		repoItem, repoOK := snap.Get(ctx, repoKey(name))
+		keys := mergeRequestKeys(ctx, snap, name)
+		for len(keys) > deleteChunk {
+			var b kv.Batch
+			for _, k := range keys[:deleteChunk] {
+				b.Delete(k)
+			}
+			if _, err := s.db.Apply(ctx, &b); err != nil {
+				return fmt.Errorf("delete merge requests of %q: %w", name, err)
+			}
+			keys = keys[deleteChunk:]
+		}
 		var b kv.Batch
-		for _, k := range keys[:deleteChunk] {
+		if repoOK {
+			b.RequireVersion(repoKey(name), repoItem.Version)
+		} else {
+			b.RequireAbsent(repoKey(name))
+		}
+		for _, k := range keys {
 			b.Delete(k)
 		}
-		if _, err := s.db.Apply(ctx, &b); err != nil {
+		b.Delete(repoKey(name))
-			return fmt.Errorf("delete merge requests of %q: %w", name, err)
+		_, err := s.db.Apply(ctx, &b)
+		if errors.Is(err, kv.ErrConflict) {
+			continue
 		}
-		keys = keys[deleteChunk:]
+		if err != nil {
-	}
+			return fmt.Errorf("delete repository %q: %w", name, err)
-	var b kv.Batch
+		}
-	for _, k := range keys {
+		return nil
-		b.Delete(k)
-	}
-	b.Delete(repoKey(name))
-	if _, err := s.db.Apply(ctx, &b); err != nil {
-		return fmt.Errorf("delete repository %q: %w", name, err)
 	}
-	return nil
+	return fmt.Errorf("delete repository %q: %w", name, domain.ErrConflict)
 }
 func encodeRepo(r domain.Repo) ([]byte, error) {

M internal/ports/ports.go +5 -2


@@ -190,7 +190,9 @@ type TokenStore interface {
190	190
191	191		// MergeRequestStore keeps the merge requests of the repositories, their
192	192		// comments and the read position of the reviewer. A repository has no entry
193		-	// until its first merge request; the store does not check that it exists.
	193	+	// until its first merge request. Create, SaveComment and SetRead return
	194	+	// domain.ErrNotFound when the repository does not exist, so that deleting a
	195	+	// repository leaves nothing for one created again under its name.
194	196		type MergeRequestStore interface {
195	197		// Create stores the merge request under the next number of its
196	198		// repository, which it sets, and returns it. Numbers start at 1 and are

@@ -219,7 +221,8 @@ type MergeRequestStore interface {
219	221		// GetRead returns the head the reviewer last marked read, or the zero
220	222		// ObjectID when there is none.
221	223		GetRead(ctx context.Context, repo domain.RepoName, number int64) (domain.ObjectID, error)
222		-	// SetRead records the head the reviewer has read.
	224	+	// SetRead records the head the reviewer has read. It returns
	225	+	// domain.ErrNotFound when the repository does not exist.
223	226		SetRead(ctx context.Context, repo domain.RepoName, number int64, head domain.ObjectID) error
224	227		}
225	228

 // MergeRequestStore keeps the merge requests of the repositories, their
 // comments and the read position of the reviewer. A repository has no entry
-// until its first merge request; the store does not check that it exists.
+// until its first merge request. Create, SaveComment and SetRead return
+// domain.ErrNotFound when the repository does not exist, so that deleting a
+// repository leaves nothing for one created again under its name.
 type MergeRequestStore interface {
 	// Create stores the merge request under the next number of its
 	// repository, which it sets, and returns it. Numbers start at 1 and are
 	// GetRead returns the head the reviewer last marked read, or the zero
 	// ObjectID when there is none.
 	GetRead(ctx context.Context, repo domain.RepoName, number int64) (domain.ObjectID, error)
-	// SetRead records the head the reviewer has read.
+	// SetRead records the head the reviewer has read. It returns
+	// domain.ErrNotFound when the repository does not exist.
 	SetRead(ctx context.Context, repo domain.RepoName, number int64, head domain.ObjectID) error
 }