From fea9e46dc860ca67a94e77dbf43b46b21d55ca62 Mon Sep 17 00:00:00 2001 From: Atefeh Mohseni-Ejiyeh Date: Mon, 8 Apr 2024 19:14:10 +0000 Subject: [PATCH] Mitigate racing condition between create repos and delete space (#1178) --- app/api/controller/check/check_list.go | 2 +- app/api/controller/check/check_recent.go | 2 +- app/api/controller/check/check_report.go | 2 +- app/api/controller/pullreq/check_list.go | 2 +- app/api/controller/pullreq/pr_create.go | 4 +-- app/api/controller/repo/create.go | 6 +++++ app/api/controller/repo/import.go | 6 +++++ .../controller/space/import_repositories.go | 6 +++++ app/api/controller/space/soft_delete.go | 9 +++++++ app/store/database.go | 3 +++ app/store/database/space.go | 26 +++++++++++++++++++ 11 files changed, 62 insertions(+), 6 deletions(-) diff --git a/app/api/controller/check/check_list.go b/app/api/controller/check/check_list.go index 8f5a163f8..2826c4eab 100644 --- a/app/api/controller/check/check_list.go +++ b/app/api/controller/check/check_list.go @@ -33,7 +33,7 @@ func (c *Controller) ListChecks( ) ([]types.Check, int, error) { repo, err := c.getRepoCheckAccess(ctx, session, repoRef, enum.PermissionRepoView) if err != nil { - return nil, 0, fmt.Errorf("failed to acquire access access to repo: %w", err) + return nil, 0, fmt.Errorf("failed to acquire access to repo: %w", err) } var checks []types.Check diff --git a/app/api/controller/check/check_recent.go b/app/api/controller/check/check_recent.go index 27248fc61..66b7554a9 100644 --- a/app/api/controller/check/check_recent.go +++ b/app/api/controller/check/check_recent.go @@ -33,7 +33,7 @@ func (c *Controller) ListRecentChecks( ) ([]string, error) { repo, err := c.getRepoCheckAccess(ctx, session, repoRef, enum.PermissionRepoView) if err != nil { - return nil, fmt.Errorf("failed to acquire access access to repo: %w", err) + return nil, fmt.Errorf("failed to acquire access to repo: %w", err) } if opts.Since == 0 { diff --git a/app/api/controller/check/check_report.go b/app/api/controller/check/check_report.go index 9f47234b8..f33680782 100644 --- a/app/api/controller/check/check_report.go +++ b/app/api/controller/check/check_report.go @@ -128,7 +128,7 @@ func (c *Controller) Report( ) (*types.Check, error) { repo, err := c.getRepoCheckAccess(ctx, session, repoRef, enum.PermissionRepoReportCommitCheck) if err != nil { - return nil, fmt.Errorf("failed to acquire access access to repo: %w", err) + return nil, fmt.Errorf("failed to acquire access to repo: %w", err) } if errValidate := in.Sanitize(c.sanitizers, session); errValidate != nil { diff --git a/app/api/controller/pullreq/check_list.go b/app/api/controller/pullreq/check_list.go index 251af7e71..ac6c6ffd5 100644 --- a/app/api/controller/pullreq/check_list.go +++ b/app/api/controller/pullreq/check_list.go @@ -36,7 +36,7 @@ func (c *Controller) ListChecks( ) (types.PullReqChecks, error) { repo, err := c.getRepoCheckAccess(ctx, session, repoRef, enum.PermissionRepoView) if err != nil { - return types.PullReqChecks{}, fmt.Errorf("failed to acquire access access to repo: %w", err) + return types.PullReqChecks{}, fmt.Errorf("failed to acquire access to repo: %w", err) } pr, err := c.pullreqStore.FindByNumber(ctx, repo.ID, prNum) diff --git a/app/api/controller/pullreq/pr_create.go b/app/api/controller/pullreq/pr_create.go index 7458aacdb..af8f271a0 100644 --- a/app/api/controller/pullreq/pr_create.go +++ b/app/api/controller/pullreq/pr_create.go @@ -55,14 +55,14 @@ func (c *Controller) Create( targetRepo, err := c.getRepoCheckAccess(ctx, session, repoRef, enum.PermissionRepoPush) if err != nil { - return nil, fmt.Errorf("failed to acquire access access to target repo: %w", err) + return nil, fmt.Errorf("failed to acquire access to target repo: %w", err) } sourceRepo := targetRepo if in.SourceRepoRef != "" { sourceRepo, err = c.getRepoCheckAccess(ctx, session, in.SourceRepoRef, enum.PermissionRepoView) if err != nil { - return nil, fmt.Errorf("failed to acquire access access to source repo: %w", err) + return nil, fmt.Errorf("failed to acquire access to source repo: %w", err) } } diff --git a/app/api/controller/repo/create.go b/app/api/controller/repo/create.go index 9e0c5dd16..079cee915 100644 --- a/app/api/controller/repo/create.go +++ b/app/api/controller/repo/create.go @@ -81,6 +81,12 @@ func (c *Controller) Create(ctx context.Context, session *auth.Session, in *Crea return fmt.Errorf("resource limit exceeded: %w", limiter.ErrMaxNumReposReached) } + // lock the space for update during repo creation to prevent racing conditions with space soft delete. + parentSpace, err = c.spaceStore.FindForUpdate(ctx, parentSpace.ID) + if err != nil { + return fmt.Errorf("failed to find the parent space: %w", err) + } + gitResp, isEmpty, err := c.createGitRepository(ctx, session, in) if err != nil { return fmt.Errorf("error creating repository on git: %w", err) diff --git a/app/api/controller/repo/import.go b/app/api/controller/repo/import.go index a93bea0da..03c64a214 100644 --- a/app/api/controller/repo/import.go +++ b/app/api/controller/repo/import.go @@ -66,6 +66,12 @@ func (c *Controller) Import(ctx context.Context, session *auth.Session, in *Impo c.publicResourceCreationEnabled, ) + // lock the space for update during repo creation to prevent racing conditions with space soft delete. + parentSpace, err = c.spaceStore.FindForUpdate(ctx, parentSpace.ID) + if err != nil { + return fmt.Errorf("failed to find the parent space: %w", err) + } + err = c.repoStore.Create(ctx, repo) if err != nil { return fmt.Errorf("failed to create repository in storage: %w", err) diff --git a/app/api/controller/space/import_repositories.go b/app/api/controller/space/import_repositories.go index eaf353b9f..608c0a470 100644 --- a/app/api/controller/space/import_repositories.go +++ b/app/api/controller/space/import_repositories.go @@ -96,6 +96,12 @@ func (c *Controller) ImportRepositories( duplicateRepos := make([]*types.Repository, 0, len(remoteRepositories)) err = c.tx.WithTx(ctx, func(ctx context.Context) error { + // lock the space for update during repo creation to prevent racing conditions with space soft delete. + space, err = c.spaceStore.FindForUpdate(ctx, space.ID) + if err != nil { + return fmt.Errorf("failed to find the parent space: %w", err) + } + if err := c.resourceLimiter.RepoCount( ctx, space.ID, len(remoteRepositories)); err != nil { return fmt.Errorf("resource limit exceeded: %w", limiter.ErrMaxNumReposReached) diff --git a/app/api/controller/space/soft_delete.go b/app/api/controller/space/soft_delete.go index f2271f66a..4b33b03f6 100644 --- a/app/api/controller/space/soft_delete.go +++ b/app/api/controller/space/soft_delete.go @@ -81,6 +81,10 @@ func (c *Controller) softDeleteInnerInTx( session *auth.Session, space *types.Space, ) (*SoftDeleteResponse, error) { + _, err := c.spaceStore.FindForUpdate(ctx, space.ID) + if err != nil { + return nil, fmt.Errorf("failed to lock the space for update: %w", err) + } filter := &types.SpaceFilter{ Page: 1, Size: math.MaxInt, @@ -98,6 +102,11 @@ func (c *Controller) softDeleteInnerInTx( now := time.Now().UnixMilli() for _, space := range subSpaces { + _, err := c.spaceStore.FindForUpdate(ctx, space.ID) + if err != nil { + return nil, fmt.Errorf("failed to lock the space for update: %w", err) + } + if err := c.spaceStore.SoftDelete(ctx, space, now); err != nil { return nil, fmt.Errorf("failed to soft delete subspace: %w", err) } diff --git a/app/store/database.go b/app/store/database.go index fac1cd63e..2c4415c0f 100644 --- a/app/store/database.go +++ b/app/store/database.go @@ -176,6 +176,9 @@ type ( UpdateOptLock(ctx context.Context, space *types.Space, mutateFn func(space *types.Space) error) (*types.Space, error) + // FindForUpdate finds the space and locks it for an update. + FindForUpdate(ctx context.Context, id int64) (*types.Space, error) + // SoftDelete deletes the space. SoftDelete(ctx context.Context, space *types.Space, deletedAt int64) error diff --git a/app/store/database/space.go b/app/store/database/space.go index 4d539231e..01f311763 100644 --- a/app/store/database/space.go +++ b/app/store/database/space.go @@ -386,6 +386,32 @@ func (s *SpaceStore) updateDeletedOptLock( ) } +// FindForUpdate finds the space and locks it for an update (should be called in a tx). +func (s *SpaceStore) FindForUpdate(ctx context.Context, id int64) (*types.Space, error) { + // sqlite allows at most one write to proceed (no need to lock) + if strings.HasPrefix(s.db.DriverName(), "sqlite") { + return s.find(ctx, id, nil) + } + + stmt := database.Builder.Select("space_id"). + From("spaces"). + Where("space_id = ? AND space_deleted IS NULL", id). + Suffix("FOR UPDATE") + + sqlQuery, params, err := stmt.ToSql() + if err != nil { + return nil, database.ProcessSQLErrorf(ctx, err, "failed to generate lock on spaces") + } + + dst := new(space) + db := dbtx.GetAccessor(ctx, s.db) + if err = db.GetContext(ctx, dst, sqlQuery, params...); err != nil { + return nil, database.ProcessSQLErrorf(ctx, err, "Failed to find space") + } + + return mapToSpace(ctx, s.db, s.spacePathStore, dst) +} + // SoftDelete deletes a space softly. func (s *SpaceStore) SoftDelete( ctx context.Context,