Commit `5e9732fb1d`

5e9732fb1d5142142fdae2985f07125cf852647c

parent: f686b22ad7

Verified · cmc ci/build: success ci/test: success ci/vuln: success

cmc <hello@cleberg.net> · 2026-09-04 14:55 UTC

store: one admin's settings change no longer overwrites another's

settings_json is one blob, so changing one field means writing all of
them. Every caller read the struct off a Repo it had loaded earlier,
changed a field and wrote the whole blob back. Two admins at once and the
later write puts back what it read for the other's field. A test against
the old path loses an update in 39 of 40 races.

UpdateRepoSettings takes a mutate function and does the read and the
write itself, inside one transaction. BEGIN IMMEDIATE takes the write
lock up front, so a second updater waits at the start rather than
discovering the conflict after reading a stale blob; the whole exchange
runs on one connection so BEGIN brackets it.

json_set in the UPDATE was the other option in the issue. It cannot
express turning a flag off: the JSON tags are omitempty, so false is an
absent key, and a merge patch built from the struct is empty exactly when
the change is "set this to its zero value". Passing paths as strings
instead would put JSON-path knowledge in twelve call sites and make a
typo a silent write nothing reads.

Protected branches read and rewrite the list inside the update, so two
admins protecting different branches both land.

SetForkOf, which the issue names alongside this, has no callers — #108's
CreateFork replaced it. Left in place; it is not a read-modify-write and
deleting it is not this change's business.

Closes #123

Layout: unified · split

internal/control/migrate.go +3 −1

		@@ -215,8 +215,10 @@ func runAccountImportBundle(c *Ctx, args []string) int {
215	215	if len(cmds) > 0 {
216	216	deferrals = append(deferrals, deferred{path, cmds})
217	217	}
	218	// An import writes the whole blob: the repository was created a
	219	// few lines up and nobody else holds settings on it yet.
218	220	s.GitDaemon = false // instance-dependent; opt back in explicitly
219		c.Store.SetRepoSettings(repo.ID, s)
	221	c.Store.UpdateRepoSettings(repo.ID, func(cur store.RepoSettings) { cur = s })
220	222
221	223	for _, bi := range br.Issues {
222	224	key := fmt.Sprintf("mig-issue:%d", bi.Number)

internal/control/mr.go +10 −15

		@@ -133,9 +133,8 @@ func runRequireApprovals(c *Ctx, args []string) int {
133	133	if code >= 0 {
134	134	return code
135	135	}
136		s := repo.Settings
137		s.RequireApprovals = n
138		if err := c.Store.SetRepoSettings(repo.ID, s); err != nil {
	136	s, err := c.Store.UpdateRepoSettings(repo.ID, func(s *store.RepoSettings) { s.RequireApprovals = n })
	137	if err != nil {
139	138	return c.fail(protocol.ExitFailure, "%v", err)
140	139	}
141	140	return c.emit(s, func(w io.Writer) {
		@@ -151,9 +150,8 @@ func runRequireResolved(c *Ctx, args []string) int {
151	150	if code >= 0 {
152	151	return code
153	152	}
154		s := repo.Settings
155		s.RequireResolved = args[1] == "on"
156		if err := c.Store.SetRepoSettings(repo.ID, s); err != nil {
	153	s, err := c.Store.UpdateRepoSettings(repo.ID, func(s *store.RepoSettings) { s.RequireResolved = args[1] == "on" })
	154	if err != nil {
157	155	return c.fail(protocol.ExitFailure, "%v", err)
158	156	}
159	157	return c.emit(s, func(w io.Writer) {
		@@ -169,9 +167,8 @@ func runRequireCodeowners(c *Ctx, args []string) int {
169	167	if code >= 0 {
170	168	return code
171	169	}
172		s := repo.Settings
173		s.RequireCodeowners = args[1] == "on"
174		if err := c.Store.SetRepoSettings(repo.ID, s); err != nil {
	170	s, err := c.Store.UpdateRepoSettings(repo.ID, func(s *store.RepoSettings) { s.RequireCodeowners = args[1] == "on" })
	171	if err != nil {
175	172	return c.fail(protocol.ExitFailure, "%v", err)
176	173	}
177	174	return c.emit(s, func(w io.Writer) {
		@@ -187,9 +184,8 @@ func runRequireChecks(c *Ctx, args []string) int {
187	184	if code >= 0 {
188	185	return code
189	186	}
190		s := repo.Settings
191		s.RequireChecks = args[1] == "on"
192		if err := c.Store.SetRepoSettings(repo.ID, s); err != nil {
	187	s, err := c.Store.UpdateRepoSettings(repo.ID, func(s *store.RepoSettings) { s.RequireChecks = args[1] == "on" })
	188	if err != nil {
193	189	return c.fail(protocol.ExitFailure, "%v", err)
194	190	}
195	191	return c.emit(s, func(w io.Writer) {
		@@ -205,9 +201,8 @@ func runRequireSigned(c *Ctx, args []string) int {
205	201	if code >= 0 {
206	202	return code
207	203	}
208		s := repo.Settings
209		s.RequireSignedCommits = args[1] == "on"
210		if err := c.Store.SetRepoSettings(repo.ID, s); err != nil {
	204	s, err := c.Store.UpdateRepoSettings(repo.ID, func(s *store.RepoSettings) { s.RequireSignedCommits = args[1] == "on" })
	205	if err != nil {
211	206	return c.fail(protocol.ExitFailure, "%v", err)
212	207	}
213	208	return c.emit(s, func(w io.Writer) {

internal/control/repo.go +19 −22

		@@ -562,9 +562,7 @@ func runSetWebsite(c *Ctx, args []string) int {
562	562	if code >= 0 {
563	563	return code
564	564	}
565		s := repo.Settings
566		s.Website = site
567		if err := c.Store.SetRepoSettings(repo.ID, s); err != nil {
	565	if _, err := c.Store.UpdateRepoSettings(repo.ID, func(s *store.RepoSettings) { s.Website = site }); err != nil {
568	566	return c.fail(protocol.ExitFailure, "%v", err)
569	567	}
570	568	return c.emit(map[string]string{"website": site}, func(w io.Writer) {
		@@ -601,9 +599,7 @@ func setRepoVisibility(c *Ctx, repo store.Repo, visibility string) int {
601	599	// Going private takes the repository off every anonymous surface, so
602	600	// git:// exposure cannot outlive the change.
603	601	if visibility == "private" && repo.Settings.GitDaemon {
604		s := repo.Settings
605		s.GitDaemon = false
606		c.Store.SetRepoSettings(repo.ID, s)
	602	c.Store.UpdateRepoSettings(repo.ID, func(s *store.RepoSettings) { s.GitDaemon = false })
607	603	}
608	604	c.Store.Audit(c.User.ID, "repo.visibility", map[string]any{"repo": repo.ID, "visibility": visibility})
609	605	return c.emit(map[string]string{"visibility": visibility}, func(w io.Writer) {
		@@ -626,9 +622,8 @@ func runGitDaemon(c *Ctx, args []string) int {
626	622	if on && !c.Cfg.GitDaemon.Enabled {
627	623	return c.fail(protocol.ExitUsage, "this instance does not run the git:// daemon ([git_daemon] enabled = false)")
628	624	}
629		s := repo.Settings
630		s.GitDaemon = on
631		if err := c.Store.SetRepoSettings(repo.ID, s); err != nil {
	625	s, err := c.Store.UpdateRepoSettings(repo.ID, func(s *store.RepoSettings) { s.GitDaemon = on })
	626	if err != nil {
632	627	return c.fail(protocol.ExitFailure, "%v", err)
633	628	}
634	629	return c.emit(s, func(w io.Writer) { fmt.Fprintf(w, "git-daemon %s on %s\n", args[1], repo.Path()) })
		@@ -662,9 +657,8 @@ func archiveRepo(c *Ctx, repo store.Repo, archived bool) int {
662	657	if repo.Settings.Archived == archived {
663	658	return c.fail(protocol.ExitUsage, "%s is already %sd", repo.Path(), verb)
664	659	}
665		s := repo.Settings
666		s.Archived = archived
667		if err := c.Store.SetRepoSettings(repo.ID, s); err != nil {
	660	s, err := c.Store.UpdateRepoSettings(repo.ID, func(s *store.RepoSettings) { s.Archived = archived })
	661	if err != nil {
668	662	return c.fail(protocol.ExitFailure, "%v", err)
669	663	}
670	664	c.Store.RecordEvent(repo.ID, c.User.ID, "repo."+verb+"d", "{}")
		@@ -898,16 +892,19 @@ func setProtect(c *Ctx, args []string, protect bool) int {
898	892	return code
899	893	}
900	894	branch := args[1]
901		s := repo.Settings
902		has := slices.Contains(s.ProtectedBranches, branch)
903		if protect && !has {
904		s.ProtectedBranches = append(s.ProtectedBranches, branch)
905		slices.Sort(s.ProtectedBranches)
906		}
907		if !protect && has {
908		s.ProtectedBranches = slices.DeleteFunc(s.ProtectedBranches, func(b string) bool { return b == branch })
909		}
910		if err := c.Store.SetRepoSettings(repo.ID, s); err != nil {
	895	// The list is read and rewritten inside the update, so two admins
	896	// protecting different branches at once both land.
	897	s, err := c.Store.UpdateRepoSettings(repo.ID, func(s *store.RepoSettings) {
	898	has := slices.Contains(s.ProtectedBranches, branch)
	899	if protect && !has {
	900	s.ProtectedBranches = append(s.ProtectedBranches, branch)
	901	slices.Sort(s.ProtectedBranches)
	902	}
	903	if !protect && has {
	904	s.ProtectedBranches = slices.DeleteFunc(s.ProtectedBranches, func(b string) bool { return b == branch })
	905	}
	906	})
	907	if err != nil {
911	908	return c.fail(protocol.ExitFailure, "%v", err)
912	909	}
913	910	verb := "protected"

internal/store/repos.go +57 −5

		@@ -1,6 +1,7 @@
1	1	package store
2	2
3	3	import (
	4	"context"
4	5	"database/sql"
5	6	"encoding/json"
6	7	"errors"
		@@ -92,13 +93,64 @@ func (s *Store) SetRepoVisibility(repoID int64, visibility string) error {
92	93	return err
93	94	}
94	95
95		func (s *Store) SetRepoSettings(repoID int64, settings RepoSettings) error {
96		raw, err := json.Marshal(settings)
	96	// UpdateRepoSettings applies mutate to the repository's settings and
	97	// stores the result, returning what was stored.
	98	//
	99	// settings_json is one blob, so changing one field means writing all of
	100	// them. Callers used to read the struct off a Repo they had loaded
	101	// earlier, change a field and write the whole blob back, which loses the
	102	// other admin's change whenever two ran at once — last write wins over a
	103	// value it never read. The read and the write happen here instead, inside
	104	// one transaction, and BEGIN IMMEDIATE takes the write lock up front: a
	105	// second updater waits at the start rather than discovering the conflict
	106	// after it has already read a stale blob.
	107	func (s Store) UpdateRepoSettings(repoID int64, mutate func(RepoSettings)) (RepoSettings, error) {
	108	ctx := context.Background()
	109	var out RepoSettings
	110	// The whole exchange must run on one connection for BEGIN to bracket
	111	// it; the pool would otherwise be free to hand the statements out
	112	// separately.
	113	conn, err := s.DB.Conn(ctx)
97	114	if err != nil {
98		return err
	115	return out, err
99	116	}
100		_, err = s.DB.Exec("UPDATE repos SET settings_json = ? WHERE id = ?", string(raw), repoID)
101		return err
	117	defer conn.Close()
	118	if _, err := conn.ExecContext(ctx, "BEGIN IMMEDIATE"); err != nil {
	119	return out, err
	120	}
	121	committed := false
	122	defer func() {
	123	if !committed {
	124	conn.ExecContext(ctx, "ROLLBACK")
	125	}
	126	}()
	127	var raw string
	128	if err := conn.QueryRowContext(ctx,
	129	"SELECT settings_json FROM repos WHERE id = ?", repoID).Scan(&raw); err != nil {
	130	if errors.Is(err, sql.ErrNoRows) {
	131	return out, ErrNotFound
	132	}
	133	return out, err
	134	}
	135	if raw != "" {
	136	if err := json.Unmarshal([]byte(raw), &out); err != nil {
	137	return out, err
	138	}
	139	}
	140	mutate(&out)
	141	next, err := json.Marshal(out)
	142	if err != nil {
	143	return out, err
	144	}
	145	if _, err := conn.ExecContext(ctx,
	146	"UPDATE repos SET settings_json = ? WHERE id = ?", string(next), repoID); err != nil {
	147	return out, err
	148	}
	149	if _, err := conn.ExecContext(ctx, "COMMIT"); err != nil {
	150	return out, err
	151	}
	152	committed = true
	153	return out, nil
102	154	}
103	155
104	156	// CreateFork is CreateRepo with fork_of set in the same insert, so a fork

internal/store/reposettings_test.go added +120

		@@ -0,0 +1,120 @@
	1	package store
	2
	3	import (
	4	"sync"
	5	"testing"
	6	)
	7
	8	func settingsFixture(t testing.T) (Store, int64) {
	9	t.Helper()
	10	s := open(t)
	11	if err := s.MigrateUp(); err != nil {
	12	t.Fatal(err)
	13	}
	14	uid, err := s.CreateUser("cmc", true)
	15	if err != nil {
	16	t.Fatal(err)
	17	}
	18	repoID, err := s.CreateRepo("user", uid, "lib", "public")
	19	if err != nil {
	20	t.Fatal(err)
	21	}
	22	return s, repoID
	23	}
	24
	25	func TestUpdateRepoSettingsRoundTrip(t *testing.T) {
	26	s, repoID := settingsFixture(t)
	27	got, err := s.UpdateRepoSettings(repoID, func(set *RepoSettings) { set.RequireApprovals = 2 })
	28	if err != nil {
	29	t.Fatal(err)
	30	}
	31	if got.RequireApprovals != 2 {
	32	t.Fatalf("returned %+v", got)
	33	}
	34	repo, err := s.RepoByID(repoID)
	35	if err != nil {
	36	t.Fatal(err)
	37	}
	38	if repo.Settings.RequireApprovals != 2 {
	39	t.Fatalf("stored %+v", repo.Settings)
	40	}
	41
	42	// Turning a flag back off must persist: the JSON tags are omitempty,
	43	// so "false" is an absent key, and a patch-style write would drop it.
	44	if _, err := s.UpdateRepoSettings(repoID, func(set *RepoSettings) { set.RequireChecks = true }); err != nil {
	45	t.Fatal(err)
	46	}
	47	got, err = s.UpdateRepoSettings(repoID, func(set *RepoSettings) { set.RequireChecks = false })
	48	if err != nil {
	49	t.Fatal(err)
	50	}
	51	if got.RequireChecks {
	52	t.Fatal("require_checks stayed on")
	53	}
	54	repo, _ = s.RepoByID(repoID)
	55	if repo.Settings.RequireChecks \|\| repo.Settings.RequireApprovals != 2 {
	56	t.Fatalf("stored %+v", repo.Settings)
	57	}
	58	}
	59
	60	func TestUpdateRepoSettingsMissingRepo(t *testing.T) {
	61	s, _ := settingsFixture(t)
	62	if _, err := s.UpdateRepoSettings(9999, func(*RepoSettings) {}); err != ErrNotFound {
	63	t.Fatalf("err = %v, want ErrNotFound", err)
	64	}
	65	}
	66
	67	// Concurrent updates to different fields both survive. Read-modify-write
	68	// through the caller lost one of them: each read the same blob and the
	69	// later write put back what it had read for the other's field.
	70	func TestUpdateRepoSettingsConcurrent(t *testing.T) {
	71	s, repoID := settingsFixture(t)
	72	var wg sync.WaitGroup
	73	errs := make(chan error, 2)
	74	wg.Add(2)
	75	go func() {
	76	defer wg.Done()
	77	_, err := s.UpdateRepoSettings(repoID, func(set *RepoSettings) { set.RequireApprovals = 3 })
	78	errs <- err
	79	}()
	80	go func() {
	81	defer wg.Done()
	82	_, err := s.UpdateRepoSettings(repoID, func(set *RepoSettings) { set.RequireResolved = true })
	83	errs <- err
	84	}()
	85	wg.Wait()
	86	close(errs)
	87	for err := range errs {
	88	if err != nil {
	89	t.Fatal(err)
	90	}
	91	}
	92	repo, err := s.RepoByID(repoID)
	93	if err != nil {
	94	t.Fatal(err)
	95	}
	96	if repo.Settings.RequireApprovals != 3 \|\| !repo.Settings.RequireResolved {
	97	t.Fatalf("one update was lost: %+v", repo.Settings)
	98	}
	99	}
	100
	101	// The protected-branch list is read and rewritten inside the update, so
	102	// two admins protecting different branches at once both land.
	103	func TestProtectedBranchesConcurrent(t *testing.T) {
	104	s, repoID := settingsFixture(t)
	105	var wg sync.WaitGroup
	106	for _, branch := range []string{"main", "release"} {
	107	wg.Add(1)
	108	go func(b string) {
	109	defer wg.Done()
	110	s.UpdateRepoSettings(repoID, func(set *RepoSettings) {
	111	set.ProtectedBranches = append(set.ProtectedBranches, b)
	112	})
	113	}(branch)
	114	}
	115	wg.Wait()
	116	repo, _ := s.RepoByID(repoID)
	117	if len(repo.Settings.ProtectedBranches) != 2 {
	118	t.Fatalf("protected branches = %v, want both", repo.Settings.ProtectedBranches)
	119	}
	120	}

Commit 5e9732fb1d

Commit `5e9732fb1d`