From 74d2612eac643b9e6e038e6dd1a3dda9b7940519 Mon Sep 17 00:00:00 2001 From: Eugene Blikh Date: Fri, 24 Jul 2026 20:48:25 +0300 Subject: [PATCH] fix(db): map repository_path_key to ErrNameTaken CreateRepo only recognized uq_repo_owner_id_name as a name collision, but path is derived from (owner, name) by RepoDiskPath, so a duplicate always violates both indexes -- and repository_path_key, declared inline on the column, has the lower OID and is the one Postgres reports. ErrNameTaken was therefore unreachable in practice and every duplicate surfaced as a raw 23505, breaking all three callers that branch on it: - /internal/repos returned 500 "create database" instead of an idempotent 200, so git.sr.ht's post-update hook printed "companion provisioning failed (500)" on every push to a repo whose companion already existed - the web create form returned 500 instead of 409 "You already have a database with that name." - a lost remotesapi auto-create race failed with codes.Unavailable instead of adopting the winner's row TestCreateRepoDuplicateName missed it by re-creating under a different path, which only the name index catches; it is now table-driven over both. --- db/repos.go | 18 ++++++++++++++---- db/repos_test.go | 42 +++++++++++++++++++++++++++++------------- db/store.go | 3 ++- 3 files changed, 45 insertions(+), 18 deletions(-) diff --git a/db/repos.go b/db/repos.go index fd8b8231b1e8d7cc0469197cf13fae82159339b0..46c075c9d6fcaf62c08cf6820a931f94deae0a65 100644 --- a/db/repos.go +++ b/db/repos.go @@ -39,9 +39,18 @@ func scanRepo(sc rowScanner) (*core.Repo, error) { // unit of work and roll both back on failure. r.Name, r.OwnerID, r.Path and // r.Visibility must be set; ID, created and updated are assigned here and the // populated repo is returned (with OwnerName carried through from the input, -// which the caller already knows). A uq_repo_owner_id_name violation is mapped -// to ErrNameTaken; any other unique violation (e.g. duplicate path) is returned -// unwrapped so the caller sees the true cause. +// which the caller already knows). +// +// Both unique violations that a same-name re-create can raise are mapped to +// ErrNameTaken: uq_repo_owner_id_name and repository_path_key. The path index +// is redundant with (owner_id, name) — Path is always derived from the pair by +// RepoDiskPath — but it is declared inline on the column, so its index has the +// lower OID and Postgres reports *it* first. Matching only the named constraint +// therefore never fired in practice and every duplicate surfaced as a raw +// 23505: a 500 on /internal/repos for each push of an existing companion, a 500 +// instead of 409 on the web create form, and a failed adopt on a lost +// remotesapi auto-create race. Any other unique violation is returned unwrapped +// so the caller still sees the true cause. func (s *Store) CreateRepo(ctx context.Context, r *core.Repo) (*core.Repo, error) { now := time.Now().UTC() const q = ` @@ -58,7 +67,8 @@ RETURNING id` if err != nil { var pqErr *pq.Error if errors.As(err, &pqErr) && pqErr.Code == "23505" && - pqErr.Constraint == "uq_repo_owner_id_name" { + (pqErr.Constraint == "uq_repo_owner_id_name" || + pqErr.Constraint == "repository_path_key") { return nil, ErrNameTaken } return nil, fmt.Errorf("insert repository: %w", err) diff --git a/db/repos_test.go b/db/repos_test.go index 4499e7f7c2cae64fb6ca7deaa3438dda64d6b6d8..9c78b8c3a73e73b2a4d52088f65d3ecf7cf57b81 100644 --- a/db/repos_test.go +++ b/db/repos_test.go @@ -36,20 +36,36 @@ func TestCreateAndGetRepo(t *testing.T) { } } +// TestCreateRepoDuplicateName covers both unique indexes a same-name re-create +// can trip. The "same path" case is the one production actually takes — Path is +// derived from (owner, name), so a real duplicate violates both indexes and +// Postgres reports the lower-OID repository_path_key. The "different path" case +// only reaches uq_repo_owner_id_name; it needs the repos root to have moved +// after the first row was written. func TestCreateRepoDuplicateName(t *testing.T) { - s, db, cleanup := newTestStore(t) - defer cleanup() - ctx := context.Background() - - insertUser(t, db, 1, "alice", core.UserTypeUser) - mkRepo(t, s, ctx, 1, "alice", "widgets", core.VisibilityPublic) - - _, err := s.CreateRepo(ctx, &core.Repo{ - Name: "widgets", OwnerID: 1, OwnerName: "alice", - Path: "/var/lib/dolt/~alice/widgets2", Visibility: core.VisibilityPrivate, - }) - if !errors.Is(err, ErrNameTaken) { - t.Fatalf("expected ErrNameTaken, got %v", err) + for _, tc := range []struct { + name string + path string + }{ + {"same path (both indexes, path reported)", "/var/lib/dolt/~alice/widgets"}, + {"different path (name index only)", "/srv/dolt/~alice/widgets"}, + } { + t.Run(tc.name, func(t *testing.T) { + s, db, cleanup := newTestStore(t) + defer cleanup() + ctx := context.Background() + + insertUser(t, db, 1, "alice", core.UserTypeUser) + mkRepo(t, s, ctx, 1, "alice", "widgets", core.VisibilityPublic) + + _, err := s.CreateRepo(ctx, &core.Repo{ + Name: "widgets", OwnerID: 1, OwnerName: "alice", + Path: tc.path, Visibility: core.VisibilityPrivate, + }) + if !errors.Is(err, ErrNameTaken) { + t.Fatalf("expected ErrNameTaken, got %v", err) + } + }) } } diff --git a/db/store.go b/db/store.go index 6a16f82929cbfe50bef0bc99657adaf105428284..5082e24854050130bc45174c2bb7ff8214e06c6b 100644 --- a/db/store.go +++ b/db/store.go @@ -71,7 +71,8 @@ var ( // ErrNotFound is returned when a lookup, update or delete matched no row. ErrNotFound = errors.New("db: not found") // ErrNameTaken is returned by CreateRepo when the owner already has a - // repository with the requested name (uq_repo_owner_id_name violation). + // repository with the requested name (uq_repo_owner_id_name or the + // equivalent repository_path_key violation). ErrNameTaken = errors.New("db: repository name already taken") // ErrKeyExists is returned by InsertKey when the key id (kid) is already // registered (dolt_key.kid UNIQUE violation).