From 2a8c061a49dda7699c386bca93ec8d644c3851f1 Mon Sep 17 00:00:00 2001 From: Tiago Farto Date: Fri, 12 Jun 2026 16:41:05 +0000 Subject: [PATCH 1/4] feat: serialize tenant-owned writes against tenant data purges Tenant data purges now take an exclusive transaction-scoped advisory lock per tenant while every tenant-owned mutation (feedback records, webhooks, embeddings, taxonomy) holds the same key in shared try-lock mode inside a single repository-level helper. Writes for a tenant under purge fail fast with a retryable 409 (code tenant_write_conflict); the purge waits up to TENANT_PURGE_LOCK_TIMEOUT_SECONDS (default 5s) for in-flight writes to drain and then returns the same retryable conflict. Other tenants are unaffected. ID-based mutations now resolve the row's tenant inside the transaction and mutate with tenant-scoped WHERE clauses; the GDPR delete-by-user locks every spanned tenant; tenant-changing webhook updates lock both tenants. Workers skip or retry cleanly on conflicts. Closes ENG-1013 Co-Authored-By: Claude Fable 5 --- .env.example | 5 + AGENTS.md | 1 + cmd/api/app.go | 2 +- internal/api/response/errors.go | 9 + internal/api/response/problem.go | 2 + internal/api/response/response_test.go | 9 + internal/config/config.go | 14 + internal/config/config_test.go | 31 + internal/huberrors/errors.go | 34 + internal/repository/embeddings_repository.go | 63 +- .../repository/feedback_records_repository.go | 247 ++++++-- internal/repository/taxonomy_repository.go | 330 +++++----- internal/repository/tenant_data_repository.go | 17 +- .../repository/tenant_data_repository_test.go | 78 ++- internal/repository/tenant_write_lock.go | 178 ++++++ internal/repository/tenant_write_lock_test.go | 320 ++++++++++ internal/repository/webhooks_repository.go | 123 +++- internal/workers/feedback_embedding.go | 94 ++- internal/workers/feedback_embedding_test.go | 155 +++++ internal/workers/webhook_dispatch.go | 13 +- internal/workers/webhook_dispatch_test.go | 38 +- openapi.yaml | 86 ++- tests/integration_test.go | 2 +- tests/tenant_write_lock_test.go | 591 ++++++++++++++++++ 24 files changed, 2132 insertions(+), 310 deletions(-) create mode 100644 internal/repository/tenant_write_lock.go create mode 100644 internal/repository/tenant_write_lock_test.go create mode 100644 internal/workers/feedback_embedding_test.go create mode 100644 tests/tenant_write_lock_test.go diff --git a/.env.example b/.env.example index d20d7113..d7068713 100644 --- a/.env.example +++ b/.env.example @@ -108,6 +108,11 @@ WEBHOOK_MAX_COUNT=500 # GOOGLE_APPLICATION_CREDENTIALS= (optional; for google-gemini when outside Google Cloud: path to service account key JSON) # EMBEDDING_MODEL= (required to enable embeddings; no default) +# Tenant data purge (optional). How long DELETE /v1/tenants/{tenant_id}/data waits for in-flight +# tenant-owned writes to drain before returning a retryable 409 (code tenant_write_conflict). +# Must be a positive integer (seconds); non-positive values fall back to the default. Default: 5 +# TENANT_PURGE_LOCK_TIMEOUT_SECONDS=5 + # Local River UI basic auth (optional, used by docker compose). Change these for your local setup as needed. # compose.yml defaults to admin / changeme if these are unset. RIVER_BASIC_AUTH_USER=admin diff --git a/AGENTS.md b/AGENTS.md index 4b7949d2..a3651014 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -36,6 +36,7 @@ - Async jobs must carry the tenant boundary when the source data has one, and workers must re-check tenant scope before doing side effects. Do not rely only on enqueue-time filtering. - Global resources may intentionally have `tenant_id = NULL` only when the domain explicitly documents them as non-tenant-owned. Webhooks are tenant-owned and must require a non-empty `tenant_id`; a missing tenant on event data must not match any webhook. - When changing access rules for a tenant-owned model, search for all alternate access paths by resource name and by derived side effects: list, get, update, delete, bulk delete, webhook fan-out, River jobs, workers, embeddings, search, exports, cache invalidation, logs, and metrics. +- Every tenant-owned mutation must run through the tenant write lock helper (`internal/repository/tenant_write_lock.go`): a transaction that first try-acquires the shared advisory lock on `tenant_write|:` (resolve the row's tenant inside the transaction for ID-based mutations, then mutate with a tenant-scoped WHERE). The tenant data purge holds the same key exclusively. Lock order is fixed: tenant shared lock → scope-specific advisory locks (e.g. taxonomy run scope) → row locks; never block on an advisory lock while holding row locks. A path that skips the helper silently reopens the purge/write race. - For any tenant-scoping change, include verification at the boundary where the leak could happen: database query behavior, service fan-out, worker execution, and API behavior when relevant. Include at least one alternate-path regression test proving that data allowed through the primary path cannot leak through async dispatch, bulk operations, derived indexes, exports, or background workers. Tests are the evidence; the invariant belongs in the architecture. ## Testing Guidelines diff --git a/cmd/api/app.go b/cmd/api/app.go index 81521f37..25da1046 100644 --- a/cmd/api/app.go +++ b/cmd/api/app.go @@ -229,7 +229,7 @@ func NewApp(cfg *config.Config, db *pgxpool.Pool) (*App, error) { feedbackRecordsRepo := repository.NewFeedbackRecordsRepository(db) embeddingsRepo := repository.NewEmbeddingsRepository(db) - tenantDataRepo := repository.NewTenantDataRepository(db) + tenantDataRepo := repository.NewTenantDataRepository(db, cfg.TenantData.PurgeLockTimeout.Duration()) embeddingProviderName, embeddingModel := embeddingProviderAndModel(cfg) embeddingModelForDB := embeddingModel diff --git a/internal/api/response/errors.go b/internal/api/response/errors.go index dac8ce66..5a3846a2 100644 --- a/internal/api/response/errors.go +++ b/internal/api/response/errors.go @@ -83,6 +83,15 @@ func problemFromError(err error) ProblemDetails { return problem } + var tenantWriteConflictErr *huberrors.TenantWriteConflictError + if errors.As(err, &tenantWriteConflictErr) { + problem := newProblem(http.StatusConflict, tenantWriteConflictErr.Error()) + problem.Type = ProblemTypeTenantWriteConflict + problem.Code = CodeTenantWriteConflict + + return problem + } + var conflictErr *huberrors.ConflictError if errors.As(err, &conflictErr) { return newProblem(http.StatusConflict, conflictErr.Error()) diff --git a/internal/api/response/problem.go b/internal/api/response/problem.go index 75f145c5..b557c3a2 100644 --- a/internal/api/response/problem.go +++ b/internal/api/response/problem.go @@ -14,6 +14,7 @@ const ( ProblemTypeForbidden = "https://hub.formbricks.com/problems/forbidden" ProblemTypeNotFound = "https://hub.formbricks.com/problems/not-found" ProblemTypeConflict = "https://hub.formbricks.com/problems/conflict" + ProblemTypeTenantWriteConflict = "https://hub.formbricks.com/problems/tenant-write-conflict" ProblemTypeMethodNotAllowed = "https://hub.formbricks.com/problems/method-not-allowed" ProblemTypeServiceUnavailable = "https://hub.formbricks.com/problems/service-unavailable" ProblemTypeInternalServerError = "https://hub.formbricks.com/problems/internal-server-error" @@ -31,6 +32,7 @@ const ( CodeForbidden = "forbidden" CodeNotFound = "not_found" CodeConflict = "conflict" + CodeTenantWriteConflict = "tenant_write_conflict" CodeMethodNotAllowed = "method_not_allowed" CodeServiceUnavailable = "service_unavailable" CodeInternalServerError = "internal_server_error" diff --git a/internal/api/response/response_test.go b/internal/api/response/response_test.go index d698523d..bbef35b6 100644 --- a/internal/api/response/response_test.go +++ b/internal/api/response/response_test.go @@ -66,6 +66,15 @@ func TestRespondErrorMapping(t *testing.T) { name: "conflict", err: huberrors.NewConflictError("already exists"), wantStatus: http.StatusConflict, wantCode: CodeConflict, wantType: ProblemTypeConflict, }, + { + name: "tenant write conflict", err: huberrors.NewTenantWriteConflictError("tenant data purge in progress; retry later"), + wantStatus: http.StatusConflict, wantCode: CodeTenantWriteConflict, wantType: ProblemTypeTenantWriteConflict, + }, + { + name: "wrapped tenant write conflict keeps dedicated code", + err: fmt.Errorf("delete feedback record: %w", huberrors.NewTenantWriteConflictError("")), + wantStatus: http.StatusConflict, wantCode: CodeTenantWriteConflict, wantType: ProblemTypeTenantWriteConflict, + }, { name: "limit exceeded", err: huberrors.NewLimitExceededError("webhook limit reached"), wantStatus: http.StatusForbidden, wantCode: CodeForbidden, wantType: ProblemTypeForbidden, diff --git a/internal/config/config.go b/internal/config/config.go index 43b07e3b..54e287a5 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -46,6 +46,7 @@ type Config struct { MessagePublisher MessagePublisherConfig Embedding EmbeddingConfig Taxonomy TaxonomyConfig + TenantData TenantDataConfig Observability ObservabilityConfig } @@ -134,6 +135,14 @@ type TaxonomyConfig struct { MinimumEmbeddedRecords int `env:"TAXONOMY_MIN_EMBEDDED_RECORDS" env-default:"20"` } +// TenantDataConfig holds tenant data purge settings. +type TenantDataConfig struct { + // PurgeLockTimeout bounds how long a tenant data purge waits for in-flight + // tenant-owned writes to drain before giving up with a retryable conflict. + // Must be positive: 0 would mean "wait forever" at the database level. + PurgeLockTimeout DurationSec `env:"TENANT_PURGE_LOCK_TIMEOUT_SECONDS" env-default:"5"` +} + // ObservabilityConfig holds OpenTelemetry settings. type ObservabilityConfig struct { MetricsExporter string `env:"OTEL_METRICS_EXPORTER"` @@ -283,6 +292,11 @@ func applyDefaults(cfg *Config) { if cfg.Taxonomy.MinimumEmbeddedRecords <= 0 { cfg.Taxonomy.MinimumEmbeddedRecords = 20 } + + const defaultPurgeLockTimeoutSec = 5 + if cfg.TenantData.PurgeLockTimeout.Duration() <= 0 { + cfg.TenantData.PurgeLockTimeout = DurationSec(time.Duration(defaultPurgeLockTimeoutSec) * time.Second) + } } func isNotExist(err error) bool { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 17765636..ba5cee63 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -346,6 +346,37 @@ func TestLoad_TaxonomyConfig(t *testing.T) { } } +func TestLoad_TenantDataPurgeLockTimeout(t *testing.T) { + tests := []struct { + name string + value string + set bool + want time.Duration + }{ + {name: "defaults to 5 seconds", set: false, want: 5 * time.Second}, + {name: "honors env override", value: "9", set: true, want: 9 * time.Second}, + {name: "coerces zero to default", value: "0", set: true, want: 5 * time.Second}, + {name: "coerces negative to default", value: "-3", set: true, want: 5 * time.Second}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.set { + t.Setenv("TENANT_PURGE_LOCK_TIMEOUT_SECONDS", tt.value) + } + + cfg, err := Load() + if err != nil { + t.Fatalf("Load() error = %v", err) + } + + if got := cfg.TenantData.PurgeLockTimeout.Duration(); got != tt.want { + t.Errorf("TenantData.PurgeLockTimeout = %v, want %v", got, tt.want) + } + }) + } +} + func TestLoad_TaxonomyServiceURLValidation(t *testing.T) { tests := []struct { name string diff --git a/internal/huberrors/errors.go b/internal/huberrors/errors.go index bac2e08e..d33acb49 100644 --- a/internal/huberrors/errors.go +++ b/internal/huberrors/errors.go @@ -135,3 +135,37 @@ func (e *ConflictError) Is(target error) bool { return ok } + +// ErrTenantWriteConflict is the sentinel for tenant write coordination conflicts: +// a tenant-owned write rejected because a tenant data purge is in progress, or a +// purge that could not acquire its lock while tenant-owned writes were in flight. +// Both directions are retryable. +var ErrTenantWriteConflict = &TenantWriteConflictError{} + +// TenantWriteConflictError is a sentinel error for tenant write coordination conflicts. +// It is deliberately distinct from ConflictError so retryable lock conflicts are never +// confused with terminal resource conflicts (e.g. duplicate submissions). +type TenantWriteConflictError struct { + Message string +} + +// NewTenantWriteConflictError creates a TenantWriteConflictError with a custom message. +func NewTenantWriteConflictError(message string) *TenantWriteConflictError { + return &TenantWriteConflictError{Message: message} +} + +// Error implements the error interface. +func (e *TenantWriteConflictError) Error() string { + if e.Message != "" { + return e.Message + } + + return "tenant write conflict" +} + +// Is implements the error interface for error comparison. +func (e *TenantWriteConflictError) Is(target error) bool { + _, ok := target.(*TenantWriteConflictError) + + return ok +} diff --git a/internal/repository/embeddings_repository.go b/internal/repository/embeddings_repository.go index cf2263c7..58f2c479 100644 --- a/internal/repository/embeddings_repository.go +++ b/internal/repository/embeddings_repository.go @@ -51,33 +51,58 @@ func (r *EmbeddingsRepository) Upsert( vec := pgvector.NewHalfVector(embedding) now := time.Now() - _, err := r.db.Exec(ctx, ` - INSERT INTO embeddings (feedback_record_id, embedding, model, created_at, updated_at) - VALUES ($1, $2, $3, $4, $5) - ON CONFLICT (feedback_record_id, model) - DO UPDATE SET embedding = EXCLUDED.embedding, updated_at = $5`, - feedbackRecordID, vec, model, now, now, - ) - if err != nil { - return fmt.Errorf("embeddings upsert: %w", err) - } + return withTenantWriteTx(ctx, tenantWritePool{db: r.db}, nil, func(dbTx tenantWriteTx) error { + // Embeddings derive their tenant boundary from the parent feedback + // record; resolve and lock it so embedding writes cannot race a tenant + // data purge. A missing parent means the record was deleted or purged. + tenantID, err := resolveFeedbackRecordTenant(ctx, dbTx, feedbackRecordID) + if err != nil { + return err + } + + if err := tryLockTenantsShared(ctx, dbTx, []string{tenantID}); err != nil { + return err + } - return nil + _, err = dbTx.Exec(ctx, ` + INSERT INTO embeddings (feedback_record_id, embedding, model, created_at, updated_at) + VALUES ($1, $2, $3, $4, $5) + ON CONFLICT (feedback_record_id, model) + DO UPDATE SET embedding = EXCLUDED.embedding, updated_at = $5`, + feedbackRecordID, vec, model, now, now, + ) + if err != nil { + return fmt.Errorf("embeddings upsert: %w", err) + } + + return nil + }) } // DeleteByFeedbackRecordAndModel removes the embedding row for the given feedback record and model. func (r *EmbeddingsRepository) DeleteByFeedbackRecordAndModel( ctx context.Context, feedbackRecordID uuid.UUID, model string, ) error { - _, err := r.db.Exec(ctx, - `DELETE FROM embeddings WHERE feedback_record_id = $1 AND model = $2`, - feedbackRecordID, model, - ) - if err != nil { - return fmt.Errorf("embeddings delete: %w", err) - } + return withTenantWriteTx(ctx, tenantWritePool{db: r.db}, nil, func(dbTx tenantWriteTx) error { + tenantID, err := resolveFeedbackRecordTenant(ctx, dbTx, feedbackRecordID) + if err != nil { + return err + } + + if err := tryLockTenantsShared(ctx, dbTx, []string{tenantID}); err != nil { + return err + } + + _, err = dbTx.Exec(ctx, + `DELETE FROM embeddings WHERE feedback_record_id = $1 AND model = $2`, + feedbackRecordID, model, + ) + if err != nil { + return fmt.Errorf("embeddings delete: %w", err) + } - return nil + return nil + }) } // ListFeedbackRecordIDsForBackfill returns IDs of feedback records that have non-empty value_text diff --git a/internal/repository/feedback_records_repository.go b/internal/repository/feedback_records_repository.go index 4f1cf54a..26c098d5 100644 --- a/internal/repository/feedback_records_repository.go +++ b/internal/repository/feedback_records_repository.go @@ -17,6 +17,10 @@ import ( "github.com/formbricks/hub/internal/models" ) +// uniqueViolationSQLState is the SQLSTATE Postgres reports for unique +// constraint violations (23505 unique_violation). +const uniqueViolationSQLState = "23505" + // FeedbackRecordsRepository handles data access for feedback records. type FeedbackRecordsRepository struct { db *pgxpool.Pool @@ -51,30 +55,56 @@ func (r *FeedbackRecordsRepository) Create(ctx context.Context, req *models.Crea var record models.FeedbackRecord - err := r.db.QueryRow(ctx, query, - collectedAt, req.SourceType, req.SourceID, req.SourceName, - req.FieldID, req.FieldLabel, req.FieldType, req.FieldGroupID, req.FieldGroupLabel, - req.ValueText, req.ValueNumber, req.ValueBoolean, req.ValueDate, - req.Metadata, req.Language, req.UserID, req.TenantID, req.SubmissionID, - ).Scan( - &record.ID, &record.CollectedAt, &record.CreatedAt, &record.UpdatedAt, - &record.SourceType, &record.SourceID, &record.SourceName, - &record.FieldID, &record.FieldLabel, &record.FieldType, &record.FieldGroupID, &record.FieldGroupLabel, - &record.ValueText, &record.ValueNumber, &record.ValueBoolean, &record.ValueDate, - &record.Metadata, &record.Language, &record.UserID, &record.TenantID, &record.SubmissionID, - ) - if err != nil { - var pgErr *pgconn.PgError - if errors.As(err, &pgErr) && pgErr.Code == "23505" { - return nil, huberrors.NewConflictError("a feedback record with this tenant_id, submission_id, and field_id already exists") + err := withTenantWriteTx(ctx, tenantWritePool{db: r.db}, []string{req.TenantID}, func(dbTx tenantWriteTx) error { + err := dbTx.QueryRow(ctx, query, + collectedAt, req.SourceType, req.SourceID, req.SourceName, + req.FieldID, req.FieldLabel, req.FieldType, req.FieldGroupID, req.FieldGroupLabel, + req.ValueText, req.ValueNumber, req.ValueBoolean, req.ValueDate, + req.Metadata, req.Language, req.UserID, req.TenantID, req.SubmissionID, + ).Scan( + &record.ID, &record.CollectedAt, &record.CreatedAt, &record.UpdatedAt, + &record.SourceType, &record.SourceID, &record.SourceName, + &record.FieldID, &record.FieldLabel, &record.FieldType, &record.FieldGroupID, &record.FieldGroupLabel, + &record.ValueText, &record.ValueNumber, &record.ValueBoolean, &record.ValueDate, + &record.Metadata, &record.Language, &record.UserID, &record.TenantID, &record.SubmissionID, + ) + if err != nil { + var pgErr *pgconn.PgError + if errors.As(err, &pgErr) && pgErr.Code == uniqueViolationSQLState { + return huberrors.NewConflictError("a feedback record with this tenant_id, submission_id, and field_id already exists") + } + + return fmt.Errorf("failed to create feedback record: %w", err) } - return nil, fmt.Errorf("failed to create feedback record: %w", err) + return nil + }) + if err != nil { + return nil, err } return &record, nil } +// resolveFeedbackRecordTenant reads the tenant boundary of a feedback record +// inside the current transaction so the caller can acquire the tenant write +// lock before mutating. tenant_id is immutable on feedback records, so a +// plain read is race-free once the lock is held. +func resolveFeedbackRecordTenant(ctx context.Context, querier queryer, id uuid.UUID) (string, error) { + var tenantID string + + err := querier.QueryRow(ctx, `SELECT tenant_id FROM feedback_records WHERE id = $1`, id).Scan(&tenantID) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return "", huberrors.NewNotFoundError("feedback record", "feedback record not found") + } + + return "", fmt.Errorf("resolve feedback record tenant: %w", err) + } + + return tenantID, nil +} + // GetByID retrieves a single feedback record by ID. Embedding is not selected (API/worker reads stay lean). func (r *FeedbackRecordsRepository) GetByID(ctx context.Context, id uuid.UUID) (*models.FeedbackRecord, error) { query := ` @@ -334,16 +364,18 @@ func buildUpdateQuery( args = append(args, id) + // tenant_id is appended by the caller after resolving the row's tenant + // boundary inside the tenant write transaction. query = fmt.Sprintf(` UPDATE feedback_records SET %s - WHERE id = $%d + WHERE id = $%d AND tenant_id = $%d RETURNING id, collected_at, created_at, updated_at, source_type, source_id, source_name, field_id, field_label, field_type, field_group_id, field_group_label, value_text, value_number, value_boolean, value_date, metadata, language, user_id, tenant_id, submission_id - `, strings.Join(updates, ", "), argCount) + `, strings.Join(updates, ", "), argCount, argCount+1) return query, args, true } @@ -355,24 +387,41 @@ func (r *FeedbackRecordsRepository) Update( ) (*models.FeedbackRecord, error) { query, args, hasUpdates := buildUpdateQuery(req, id, time.Now()) if !hasUpdates { + // No write happens, so no tenant write lock is needed. return r.GetByID(ctx, id) } var record models.FeedbackRecord - err := r.db.QueryRow(ctx, query, args...).Scan( - &record.ID, &record.CollectedAt, &record.CreatedAt, &record.UpdatedAt, - &record.SourceType, &record.SourceID, &record.SourceName, - &record.FieldID, &record.FieldLabel, &record.FieldType, &record.FieldGroupID, &record.FieldGroupLabel, - &record.ValueText, &record.ValueNumber, &record.ValueBoolean, &record.ValueDate, - &record.Metadata, &record.Language, &record.UserID, &record.TenantID, &record.SubmissionID, - ) - if err != nil { - if errors.Is(err, pgx.ErrNoRows) { - return nil, huberrors.NewNotFoundError("feedback record", "feedback record not found") + err := withTenantWriteTx(ctx, tenantWritePool{db: r.db}, nil, func(dbTx tenantWriteTx) error { + tenantID, err := resolveFeedbackRecordTenant(ctx, dbTx, id) + if err != nil { + return err } - return nil, fmt.Errorf("failed to update feedback record: %w", err) + if err := tryLockTenantsShared(ctx, dbTx, []string{tenantID}); err != nil { + return err + } + + err = dbTx.QueryRow(ctx, query, append(args, tenantID)...).Scan( + &record.ID, &record.CollectedAt, &record.CreatedAt, &record.UpdatedAt, + &record.SourceType, &record.SourceID, &record.SourceName, + &record.FieldID, &record.FieldLabel, &record.FieldType, &record.FieldGroupID, &record.FieldGroupLabel, + &record.ValueText, &record.ValueNumber, &record.ValueBoolean, &record.ValueDate, + &record.Metadata, &record.Language, &record.UserID, &record.TenantID, &record.SubmissionID, + ) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return huberrors.NewNotFoundError("feedback record", "feedback record not found") + } + + return fmt.Errorf("failed to update feedback record: %w", err) + } + + return nil + }) + if err != nil { + return nil, err } return &record, nil @@ -380,29 +429,108 @@ func (r *FeedbackRecordsRepository) Update( // Delete removes a feedback record. func (r *FeedbackRecordsRepository) Delete(ctx context.Context, id uuid.UUID) error { - query := `DELETE FROM feedback_records WHERE id = $1` + return withTenantWriteTx(ctx, tenantWritePool{db: r.db}, nil, func(dbTx tenantWriteTx) error { + tenantID, err := resolveFeedbackRecordTenant(ctx, dbTx, id) + if err != nil { + return err + } - result, err := r.db.Exec(ctx, query, id) - if err != nil { - return fmt.Errorf("failed to delete feedback record: %w", err) - } + if err := tryLockTenantsShared(ctx, dbTx, []string{tenantID}); err != nil { + return err + } - if result.RowsAffected() == 0 { - return huberrors.NewNotFoundError("feedback record", "feedback record not found") - } + result, err := dbTx.Exec(ctx, `DELETE FROM feedback_records WHERE id = $1 AND tenant_id = $2`, id, tenantID) + if err != nil { + return fmt.Errorf("failed to delete feedback record: %w", err) + } + + if result.RowsAffected() == 0 { + return huberrors.NewNotFoundError("feedback record", "feedback record not found") + } - return nil + return nil + }) } // DeleteByUser deletes all feedback records matching user_id. -// When tenant_id is provided, deletion is restricted to that tenant; otherwise all user records are deleted. +// When tenant_id is provided, deletion is restricted to that tenant; otherwise all user records +// are deleted across tenants (documented GDPR/right-to-erasure exception). +// Every spanned tenant's write lock is acquired before deleting; if any tenant is under purge +// the whole request fails with a retryable conflict. The delete is scoped to the locked tenants, +// so records appearing in new tenants mid-transaction are never touched without a lock (a retry +// catches them). // It returns deleted IDs grouped by tenant so callers can publish tenant-scoped side effects. func (r *FeedbackRecordsRepository) DeleteByUser( ctx context.Context, filters *models.DeleteFeedbackRecordsByUserFilters, ) ([]models.DeletedFeedbackRecordsByTenant, error) { - query := ` - DELETE FROM feedback_records - WHERE user_id = $1` + groups := make([]models.DeletedFeedbackRecordsByTenant, 0) + + err := withTenantWriteTx(ctx, tenantWritePool{db: r.db}, nil, func(dbTx tenantWriteTx) error { + tenantIDs, err := listUserFeedbackTenants(ctx, dbTx, filters) + if err != nil { + return err + } + + if len(tenantIDs) == 0 { + // Nothing to delete; keep the endpoint idempotent without taking locks. + return nil + } + + if err := tryLockTenantsShared(ctx, dbTx, tenantIDs); err != nil { + return err + } + + rows, err := dbTx.Query(ctx, ` + DELETE FROM feedback_records + WHERE user_id = $1 AND tenant_id = ANY($2) + RETURNING id, tenant_id`, filters.UserID, tenantIDs) + if err != nil { + return fmt.Errorf("failed to delete feedback records by user: %w", err) + } + defer rows.Close() + + groupIndexByTenant := make(map[string]int) + + for rows.Next() { + var ( + id uuid.UUID + tenantID string + ) + + if err := rows.Scan(&id, &tenantID); err != nil { + return fmt.Errorf("failed to scan deleted feedback record id: %w", err) + } + + groupIndex, ok := groupIndexByTenant[tenantID] + if !ok { + groupIndex = len(groups) + groupIndexByTenant[tenantID] = groupIndex + groups = append(groups, models.DeletedFeedbackRecordsByTenant{TenantID: tenantID}) + } + + groups[groupIndex].IDs = append(groups[groupIndex].IDs, id) + } + + if err := rows.Err(); err != nil { + return fmt.Errorf("error iterating delete feedback records by user result: %w", err) + } + + return nil + }) + if err != nil { + return nil, err + } + + return groups, nil +} + +// listUserFeedbackTenants returns the distinct tenant boundaries holding feedback +// records for the user (optionally restricted to one tenant), so DeleteByUser can +// lock each one before deleting. +func listUserFeedbackTenants( + ctx context.Context, dbTx tenantWriteTx, filters *models.DeleteFeedbackRecordsByUserFilters, +) ([]string, error) { + query := `SELECT DISTINCT tenant_id FROM feedback_records WHERE user_id = $1` args := []any{filters.UserID} if filters.TenantID != nil { @@ -411,43 +539,30 @@ func (r *FeedbackRecordsRepository) DeleteByUser( args = append(args, *filters.TenantID) } - query += ` - RETURNING id, tenant_id` + query += ` ORDER BY tenant_id` - rows, err := r.db.Query(ctx, query, args...) + rows, err := dbTx.Query(ctx, query, args...) if err != nil { - return nil, fmt.Errorf("failed to delete feedback records by user: %w", err) + return nil, fmt.Errorf("failed to list tenants for user feedback records: %w", err) } defer rows.Close() - groups := make([]models.DeletedFeedbackRecordsByTenant, 0) - groupIndexByTenant := make(map[string]int) + var tenantIDs []string for rows.Next() { - var ( - id uuid.UUID - tenantID string - ) - - if err := rows.Scan(&id, &tenantID); err != nil { - return nil, fmt.Errorf("failed to scan deleted feedback record id: %w", err) + var tenantID string + if err := rows.Scan(&tenantID); err != nil { + return nil, fmt.Errorf("failed to scan tenant id: %w", err) } - groupIndex, ok := groupIndexByTenant[tenantID] - if !ok { - groupIndex = len(groups) - groupIndexByTenant[tenantID] = groupIndex - groups = append(groups, models.DeletedFeedbackRecordsByTenant{TenantID: tenantID}) - } - - groups[groupIndex].IDs = append(groups[groupIndex].IDs, id) + tenantIDs = append(tenantIDs, tenantID) } if err := rows.Err(); err != nil { - return nil, fmt.Errorf("error iterating delete feedback records by user result: %w", err) + return nil, fmt.Errorf("error iterating tenants for user feedback records: %w", err) } - return groups, nil + return tenantIDs, nil } // fetchFeedbackRecords executes the given query and scans rows into FeedbackRecord slices. diff --git a/internal/repository/taxonomy_repository.go b/internal/repository/taxonomy_repository.go index de4af7a2..a3a56190 100644 --- a/internal/repository/taxonomy_repository.go +++ b/internal/repository/taxonomy_repository.go @@ -145,70 +145,74 @@ func (r *TaxonomyRepository) CreateRunIfAvailable( ctx context.Context, params CreateTaxonomyRunParams, ) (*models.TaxonomyRun, bool, error) { - transaction, err := r.db.BeginTx(ctx, pgx.TxOptions{}) - if err != nil { - return nil, false, fmt.Errorf("begin taxonomy run create tx: %w", err) - } + var ( + run *models.TaxonomyRun + created bool + ) - defer func() { - _ = transaction.Rollback(ctx) - }() + err := withTenantWriteTx(ctx, tenantWritePool{db: r.db}, []string{params.TenantID}, func(dbTx tenantWriteTx) error { + // Scope lock comes second, after the shared tenant write lock, per the + // tenant write lock-order convention (see tenant_write_lock.go). Scope + // waiters already hold the tenant lock in shared mode, so they never + // block a same-tenant writer and always yield to a queued purge. + lockKey := taxonomyScopeLockKey(params.TaxonomyScope) + if _, err := dbTx.Exec(ctx, `SELECT pg_advisory_xact_lock(hashtextextended($1, 0))`, lockKey); err != nil { + return fmt.Errorf("lock taxonomy run scope: %w", err) + } - lockKey := taxonomyScopeLockKey(params.TaxonomyScope) - if _, err := transaction.Exec(ctx, `SELECT pg_advisory_xact_lock(hashtextextended($1, 0))`, lockKey); err != nil { - return nil, false, fmt.Errorf("lock taxonomy run scope: %w", err) - } + existing, err := queryTaxonomyRun(ctx, dbTx, taxonomyRunSelect+` + FROM taxonomy_runs + WHERE tenant_id = $1 + AND source_type = $2 + AND source_id = $3 + AND field_id = $4 + AND status IN ('pending', 'running') + ORDER BY created_at DESC, id DESC + LIMIT 1`, + params.TenantID, params.SourceType, params.SourceID, params.FieldID, + ) + if err != nil && !errors.Is(err, pgx.ErrNoRows) { + return fmt.Errorf("find in-progress taxonomy run: %w", err) + } - existing, err := queryTaxonomyRun(ctx, transaction, taxonomyRunSelect+` - FROM taxonomy_runs - WHERE tenant_id = $1 - AND source_type = $2 - AND source_id = $3 - AND field_id = $4 - AND status IN ('pending', 'running') - ORDER BY created_at DESC, id DESC - LIMIT 1`, - params.TenantID, params.SourceType, params.SourceID, params.FieldID, - ) - if err != nil && !errors.Is(err, pgx.ErrNoRows) { - return nil, false, fmt.Errorf("find in-progress taxonomy run: %w", err) - } + if existing != nil { + run = existing - if existing != nil { - if err := transaction.Commit(ctx); err != nil { - return nil, false, fmt.Errorf("commit existing taxonomy run tx: %w", err) + return nil } - return existing, false, nil - } + inserted, err := queryTaxonomyRun(ctx, dbTx, ` + WITH taxonomy_runs AS ( + INSERT INTO taxonomy_runs ( + tenant_id, source_type, source_id, field_id, field_label, + status, params, record_count, embedding_count + ) + VALUES ($1, $2, $3, $4, $5, 'pending', $6, $7, $8) + RETURNING * + )`+taxonomyRunSelect+` FROM taxonomy_runs`, + params.TenantID, + params.SourceType, + params.SourceID, + params.FieldID, + params.FieldLabel, + rawOrDefault(params.Params, defaultJSONObj), + params.RecordCount, + params.EmbeddingCount, + ) + if err != nil { + return fmt.Errorf("insert taxonomy run: %w", err) + } - run, err := queryTaxonomyRun(ctx, transaction, ` - WITH taxonomy_runs AS ( - INSERT INTO taxonomy_runs ( - tenant_id, source_type, source_id, field_id, field_label, - status, params, record_count, embedding_count - ) - VALUES ($1, $2, $3, $4, $5, 'pending', $6, $7, $8) - RETURNING * - )`+taxonomyRunSelect+` FROM taxonomy_runs`, - params.TenantID, - params.SourceType, - params.SourceID, - params.FieldID, - params.FieldLabel, - rawOrDefault(params.Params, defaultJSONObj), - params.RecordCount, - params.EmbeddingCount, - ) - if err != nil { - return nil, false, fmt.Errorf("insert taxonomy run: %w", err) - } + run = inserted + created = true - if err := transaction.Commit(ctx); err != nil { - return nil, false, fmt.Errorf("commit taxonomy run create tx: %w", err) + return nil + }) + if err != nil { + return nil, false, err } - return run, true, nil + return run, created, nil } func taxonomyScopeLockKey(scope models.TaxonomyScope) string { @@ -227,21 +231,32 @@ func (r *TaxonomyRepository) MarkRunRunning( runID uuid.UUID, tenantID string, ) (*models.TaxonomyRun, error) { - run, err := queryTaxonomyRun(ctx, r.db, ` - WITH taxonomy_runs AS ( - UPDATE taxonomy_runs - SET status = 'running', started_at = COALESCE(started_at, NOW()), updated_at = NOW() - WHERE id = $1 AND tenant_id = $2 AND status = 'pending' - RETURNING * - )`+taxonomyRunSelect+` FROM taxonomy_runs`, - runID, tenantID, - ) - if err != nil { - if errors.Is(err, pgx.ErrNoRows) { - return nil, r.transitionError(ctx, runID, tenantID, models.TaxonomyRunStatusRunning) + var run *models.TaxonomyRun + + err := withTenantWriteTx(ctx, tenantWritePool{db: r.db}, []string{tenantID}, func(dbTx tenantWriteTx) error { + updated, err := queryTaxonomyRun(ctx, dbTx, ` + WITH taxonomy_runs AS ( + UPDATE taxonomy_runs + SET status = 'running', started_at = COALESCE(started_at, NOW()), updated_at = NOW() + WHERE id = $1 AND tenant_id = $2 AND status = 'pending' + RETURNING * + )`+taxonomyRunSelect+` FROM taxonomy_runs`, + runID, tenantID, + ) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return r.transitionError(ctx, runID, tenantID, models.TaxonomyRunStatusRunning) + } + + return fmt.Errorf("mark taxonomy run running: %w", err) } - return nil, fmt.Errorf("mark taxonomy run running: %w", err) + run = updated + + return nil + }) + if err != nil { + return nil, err } return run, nil @@ -255,21 +270,32 @@ func (r *TaxonomyRepository) MarkRunFailed( message string, errorCode models.TaxonomyRunFailureCode, ) (*models.TaxonomyRun, error) { - run, err := queryTaxonomyRun(ctx, r.db, ` - WITH taxonomy_runs AS ( - UPDATE taxonomy_runs - SET status = 'failed', error = $2, error_code = $3, finished_at = NOW(), updated_at = NOW() - WHERE id = $1 AND tenant_id = $4 AND status IN ('pending', 'running') - RETURNING * - )`+taxonomyRunSelect+` FROM taxonomy_runs`, - runID, message, nullableFailureCode(errorCode), tenantID, - ) - if err != nil { - if errors.Is(err, pgx.ErrNoRows) { - return nil, r.transitionError(ctx, runID, tenantID, models.TaxonomyRunStatusFailed) + var run *models.TaxonomyRun + + err := withTenantWriteTx(ctx, tenantWritePool{db: r.db}, []string{tenantID}, func(dbTx tenantWriteTx) error { + updated, err := queryTaxonomyRun(ctx, dbTx, ` + WITH taxonomy_runs AS ( + UPDATE taxonomy_runs + SET status = 'failed', error = $2, error_code = $3, finished_at = NOW(), updated_at = NOW() + WHERE id = $1 AND tenant_id = $4 AND status IN ('pending', 'running') + RETURNING * + )`+taxonomyRunSelect+` FROM taxonomy_runs`, + runID, message, nullableFailureCode(errorCode), tenantID, + ) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return r.transitionError(ctx, runID, tenantID, models.TaxonomyRunStatusFailed) + } + + return fmt.Errorf("mark taxonomy run failed: %w", err) } - return nil, fmt.Errorf("mark taxonomy run failed: %w", err) + run = updated + + return nil + }) + if err != nil { + return nil, err } return run, nil @@ -434,15 +460,29 @@ func (r *TaxonomyRepository) StoreResultAndActivate( tenantID string, req models.TaxonomyRunResultRequest, ) (*models.TaxonomyRun, error) { - transaction, err := r.db.BeginTx(ctx, pgx.TxOptions{}) + var updated *models.TaxonomyRun + + err := withTenantWriteTx(ctx, tenantWritePool{db: r.db}, []string{tenantID}, func(dbTx tenantWriteTx) error { + var err error + + updated, err = storeResultAndActivateInTx(ctx, dbTx, runID, tenantID, req) + + return err + }) if err != nil { - return nil, fmt.Errorf("begin taxonomy result tx: %w", err) + return nil, err } - defer func() { - _ = transaction.Rollback(ctx) - }() + return updated, nil +} +func storeResultAndActivateInTx( + ctx context.Context, + transaction tenantWriteTx, + runID uuid.UUID, + tenantID string, + req models.TaxonomyRunResultRequest, +) (*models.TaxonomyRun, error) { run, err := queryTaxonomyRun(ctx, transaction, taxonomyRunSelect+` FROM taxonomy_runs WHERE id = $1 AND tenant_id = $2 @@ -609,10 +649,6 @@ func (r *TaxonomyRepository) StoreResultAndActivate( return nil, fmt.Errorf("mark taxonomy run succeeded: %w", err) } - if err := transaction.Commit(ctx); err != nil { - return nil, fmt.Errorf("commit taxonomy result tx: %w", err) - } - return updated, nil } @@ -645,43 +681,35 @@ func (r *TaxonomyRepository) RenameNode( actorID string, label string, ) (*models.TaxonomyNode, error) { - transaction, err := r.db.BeginTx(ctx, pgx.TxOptions{}) - if err != nil { - return nil, fmt.Errorf("begin taxonomy node rename tx: %w", err) - } + var updated *models.TaxonomyNode - defer func() { - _ = transaction.Rollback(ctx) - }() + err := withTenantWriteTx(ctx, tenantWritePool{db: r.db}, []string{tenantID}, func(dbTx tenantWriteTx) error { + node, run, err := getNodeForUpdate(ctx, dbTx, nodeID, tenantID) + if err != nil { + return err + } - node, run, err := getNodeForUpdate(ctx, transaction, nodeID, tenantID) - if err != nil { - return nil, err - } + updated, err = queryTaxonomyNode(ctx, dbTx, ` + WITH taxonomy_nodes AS ( + UPDATE taxonomy_nodes + SET label = $2, updated_at = NOW() + WHERE id = $1 + RETURNING * + )`+taxonomyNodeSelect+` FROM taxonomy_nodes`, + nodeID, label, + ) + if err != nil { + return fmt.Errorf("rename taxonomy node: %w", err) + } - updated, err := queryTaxonomyNode(ctx, transaction, ` - WITH taxonomy_nodes AS ( - UPDATE taxonomy_nodes - SET label = $2, updated_at = NOW() - WHERE id = $1 - RETURNING * - )`+taxonomyNodeSelect+` FROM taxonomy_nodes`, - nodeID, label, - ) + return insertNodeEvent(ctx, dbTx, run, nodeID, "rename", actorID, + map[string]string{"label": node.Label}, + map[string]string{"label": label}) + }) if err != nil { - return nil, fmt.Errorf("rename taxonomy node: %w", err) - } - - if err := insertNodeEvent(ctx, transaction, run, nodeID, "rename", actorID, - map[string]string{"label": node.Label}, - map[string]string{"label": label}); err != nil { return nil, err } - if err := transaction.Commit(ctx); err != nil { - return nil, fmt.Errorf("commit taxonomy node rename tx: %w", err) - } - return updated, nil } @@ -692,43 +720,35 @@ func (r *TaxonomyRepository) RemoveNode( tenantID string, actorID string, ) (*models.TaxonomyNode, error) { - transaction, err := r.db.BeginTx(ctx, pgx.TxOptions{}) - if err != nil { - return nil, fmt.Errorf("begin taxonomy node remove tx: %w", err) - } + var updated *models.TaxonomyNode - defer func() { - _ = transaction.Rollback(ctx) - }() + err := withTenantWriteTx(ctx, tenantWritePool{db: r.db}, []string{tenantID}, func(dbTx tenantWriteTx) error { + _, run, err := getNodeForUpdate(ctx, dbTx, nodeID, tenantID) + if err != nil { + return err + } - _, run, err := getNodeForUpdate(ctx, transaction, nodeID, tenantID) - if err != nil { - return nil, err - } + updated, err = queryTaxonomyNode(ctx, dbTx, ` + WITH taxonomy_nodes AS ( + UPDATE taxonomy_nodes + SET removed_at = NOW(), removed_by = $2, updated_at = NOW() + WHERE id = $1 + RETURNING * + )`+taxonomyNodeSelect+` FROM taxonomy_nodes`, + nodeID, actorID, + ) + if err != nil { + return fmt.Errorf("remove taxonomy node: %w", err) + } - updated, err := queryTaxonomyNode(ctx, transaction, ` - WITH taxonomy_nodes AS ( - UPDATE taxonomy_nodes - SET removed_at = NOW(), removed_by = $2, updated_at = NOW() - WHERE id = $1 - RETURNING * - )`+taxonomyNodeSelect+` FROM taxonomy_nodes`, - nodeID, actorID, - ) + return insertNodeEvent(ctx, dbTx, run, nodeID, "soft_remove", actorID, + map[string]any{"removed_at": nil}, + map[string]string{"removed_by": actorID}) + }) if err != nil { - return nil, fmt.Errorf("remove taxonomy node: %w", err) - } - - if err := insertNodeEvent(ctx, transaction, run, nodeID, "soft_remove", actorID, - map[string]any{"removed_at": nil}, - map[string]string{"removed_by": actorID}); err != nil { return nil, err } - if err := transaction.Commit(ctx); err != nil { - return nil, fmt.Errorf("commit taxonomy node remove tx: %w", err) - } - return updated, nil } @@ -964,15 +984,21 @@ func scanTaxonomyNode(row scanner) (*models.TaxonomyNode, error) { func getNodeForUpdate( ctx context.Context, - transaction pgx.Tx, + transaction queryer, nodeID uuid.UUID, tenantID string, ) (*models.TaxonomyNode, *models.TaxonomyRun, error) { + // The tenant predicate keeps the row lock tenant-scoped: a caller can never + // lock another tenant's node row, even transiently. node, err := queryTaxonomyNode(ctx, transaction, taxonomyNodeSelect+` FROM taxonomy_nodes WHERE id = $1 AND removed_at IS NULL + AND EXISTS ( + SELECT 1 FROM taxonomy_runs + WHERE taxonomy_runs.id = taxonomy_nodes.run_id AND taxonomy_runs.tenant_id = $2 + ) FOR UPDATE`, - nodeID, + nodeID, tenantID, ) if err != nil { if errors.Is(err, pgx.ErrNoRows) { @@ -1000,7 +1026,7 @@ func getNodeForUpdate( func insertNodeEvent( ctx context.Context, - transaction pgx.Tx, + transaction tenantWriteTx, run *models.TaxonomyRun, nodeID uuid.UUID, eventType string, diff --git a/internal/repository/tenant_data_repository.go b/internal/repository/tenant_data_repository.go index 08a8fa3d..3e6ef71b 100644 --- a/internal/repository/tenant_data_repository.go +++ b/internal/repository/tenant_data_repository.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "log/slog" + "time" "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgconn" @@ -43,11 +44,15 @@ func (p tenantDataPool) BeginTx(ctx context.Context, txOptions pgx.TxOptions) (t // TenantDataRepository handles tenant-scoped data purge operations. type TenantDataRepository struct { db tenantDataTxBeginner + // purgeLockTimeout bounds how long a purge waits for in-flight tenant-owned + // writes (shared tenant write lock holders) to drain before returning a + // retryable conflict. + purgeLockTimeout time.Duration } // NewTenantDataRepository creates a new tenant data repository. -func NewTenantDataRepository(db *pgxpool.Pool) *TenantDataRepository { - return &TenantDataRepository{db: tenantDataPool{db: db}} +func NewTenantDataRepository(db *pgxpool.Pool, purgeLockTimeout time.Duration) *TenantDataRepository { + return &TenantDataRepository{db: tenantDataPool{db: db}, purgeLockTimeout: purgeLockTimeout} } // DeleteByTenant deletes all Hub-owned data for a tenant and returns per-resource counts. @@ -68,6 +73,14 @@ func (r *TenantDataRepository) DeleteByTenant(ctx context.Context, tenantID stri } }() + // Serialize against tenant-owned writes: writers hold the tenant write lock + // in shared mode, so the exclusive acquisition waits for in-flight writes to + // drain (bounded by purgeLockTimeout) and rejects new ones the moment it is + // queued. + if err := acquireTenantPurgeLock(ctx, dbTx, tenantID, r.purgeLockTimeout); err != nil { + return nil, err + } + counts, err := deleteTenantDataInTx(ctx, dbTx, tenantID) if err != nil { return nil, err diff --git a/internal/repository/tenant_data_repository_test.go b/internal/repository/tenant_data_repository_test.go index 86dbdcf7..3e954f1b 100644 --- a/internal/repository/tenant_data_repository_test.go +++ b/internal/repository/tenant_data_repository_test.go @@ -5,9 +5,12 @@ import ( "errors" "strings" "testing" + "time" "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgconn" + + "github.com/formbricks/hub/internal/huberrors" ) type fakeTenantDataDB struct { @@ -74,20 +77,30 @@ func (f *fakeTenantDataTx) Rollback(context.Context) error { return f.rollbackErr } +// purgeLockTags are the command tags for the three lock-related statements +// (set_config, advisory lock, set_config reset) that precede the deletes. +func purgeLockTags() []pgconn.CommandTag { + return []pgconn.CommandTag{ + pgconn.NewCommandTag("SELECT 1"), + pgconn.NewCommandTag("SELECT 1"), + pgconn.NewCommandTag("SELECT 1"), + } +} + func TestTenantDataRepository_DeleteByTenant(t *testing.T) { - t.Run("commits transaction and returns counts", func(t *testing.T) { + t.Run("locks tenant exclusively, commits transaction, and returns counts", func(t *testing.T) { transaction := &fakeTenantDataTx{ fakeTenantDataExecutor: fakeTenantDataExecutor{ - tags: []pgconn.CommandTag{ + tags: append(purgeLockTags(), pgconn.NewCommandTag("DELETE 2"), pgconn.NewCommandTag("DELETE 4"), pgconn.NewCommandTag("DELETE 3"), pgconn.NewCommandTag("DELETE 1"), - }, + ), }, rollbackErr: pgx.ErrTxClosed, } - repo := &TenantDataRepository{db: &fakeTenantDataDB{tx: transaction}} + repo := &TenantDataRepository{db: &fakeTenantDataDB{tx: transaction}, purgeLockTimeout: 5 * time.Second} counts, err := repo.DeleteByTenant(context.Background(), "org-123") if err != nil { @@ -105,13 +118,62 @@ func TestTenantDataRepository_DeleteByTenant(t *testing.T) { if !transaction.rolledBack { t.Fatal("deferred rollback was not called") } + + if len(transaction.queries) != 7 { + t.Fatalf("queries = %d, want 7 (3 lock statements + 4 deletes)", len(transaction.queries)) + } + + assertQueryContains(t, transaction.queries[0], "set_config('lock_timeout', $1, true)") + assertQueryContains(t, transaction.queries[1], "pg_advisory_xact_lock(hashtextextended($1, 0))") + assertQueryContains(t, transaction.queries[2], "set_config('lock_timeout', '0', true)") + assertQueryContains(t, transaction.queries[3], "DELETE FROM embeddings") + + if len(transaction.args[1]) != 1 || transaction.args[1][0] != TenantWriteLockKey("org-123") { + t.Fatalf("lock args = %#v, want tenant write lock key", transaction.args[1]) + } + }) + + t.Run("lock timeout returns tenant write conflict without deletes", func(t *testing.T) { + transaction := &fakeTenantDataTx{ + fakeTenantDataExecutor: fakeTenantDataExecutor{ + errAtQuery: 2, + err: &pgconn.PgError{Code: lockNotAvailableSQLState}, + }, + } + repo := &TenantDataRepository{db: &fakeTenantDataDB{tx: transaction}, purgeLockTimeout: time.Second} + + counts, err := repo.DeleteByTenant(context.Background(), "org-123") + if !errors.Is(err, huberrors.ErrTenantWriteConflict) { + t.Fatalf("DeleteByTenant() error = %v, want tenant write conflict", err) + } + + if counts != nil { + t.Fatalf("counts = %+v, want nil", counts) + } + + if transaction.committed { + t.Fatal("transaction was committed after lock timeout") + } + + if !transaction.rolledBack { + t.Fatal("transaction was not rolled back") + } + + for _, query := range transaction.queries { + if strings.Contains(query, "DELETE FROM") { + t.Fatalf("delete executed despite lock timeout: %q", query) + } + } }) t.Run("rolls back and returns delete error", func(t *testing.T) { rollbackErr := errors.New("rollback failed") transaction := &fakeTenantDataTx{ - fakeTenantDataExecutor: fakeTenantDataExecutor{errAtQuery: 2}, - rollbackErr: rollbackErr, + fakeTenantDataExecutor: fakeTenantDataExecutor{ + tags: purgeLockTags(), + errAtQuery: 5, + }, + rollbackErr: rollbackErr, } repo := &TenantDataRepository{db: &fakeTenantDataDB{tx: transaction}} @@ -151,12 +213,12 @@ func TestTenantDataRepository_DeleteByTenant(t *testing.T) { commitErr := errors.New("commit failed") transaction := &fakeTenantDataTx{ fakeTenantDataExecutor: fakeTenantDataExecutor{ - tags: []pgconn.CommandTag{ + tags: append(purgeLockTags(), pgconn.NewCommandTag("DELETE 2"), pgconn.NewCommandTag("DELETE 4"), pgconn.NewCommandTag("DELETE 3"), pgconn.NewCommandTag("DELETE 1"), - }, + ), }, commitErr: commitErr, rollbackErr: pgx.ErrTxClosed, diff --git a/internal/repository/tenant_write_lock.go b/internal/repository/tenant_write_lock.go new file mode 100644 index 00000000..a4436d01 --- /dev/null +++ b/internal/repository/tenant_write_lock.go @@ -0,0 +1,178 @@ +package repository + +import ( + "context" + "errors" + "fmt" + "log/slog" + "slices" + "strconv" + "time" + + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgconn" + "github.com/jackc/pgx/v5/pgxpool" + + "github.com/formbricks/hub/internal/huberrors" +) + +// Tenant write serialization. +// +// Every tenant-owned mutation runs inside a transaction that first acquires a +// SHARED transaction-scoped advisory lock on the tenant's key, so concurrent +// writes for the same tenant proceed in parallel. The tenant data purge +// acquires the same key EXCLUSIVELY, so a purge and tenant-owned writes for +// the same tenant are mutually exclusive while other tenants are unaffected. +// Writers use try-locks (fail fast with a retryable conflict, never wait); +// the purge blocks for a bounded time. Postgres queues no new shared +// try-locks behind a waiting exclusive request, so writers are rejected from +// the moment a purge arrives while in-flight writers drain. +// +// Lock-order convention (deadlock-free by construction): tenant shared +// try-lock first, then any scope-specific advisory lock (e.g. taxonomy run +// scope), then row locks. The purge takes only the tenant exclusive lock and +// holds no row locks while waiting. No code path may block on an advisory +// lock while holding row locks. + +// tenantWriteLockSharedSQL acquires the tenant key in shared mode without waiting. +const tenantWriteLockSharedSQL = `SELECT pg_try_advisory_xact_lock_shared(hashtextextended($1, 0))` + +// tenantWriteLockExclusiveSQL acquires the tenant key exclusively, waiting up +// to the transaction-local lock_timeout. +const tenantWriteLockExclusiveSQL = `SELECT pg_advisory_xact_lock(hashtextextended($1, 0))` + +// lockNotAvailableSQLState is the SQLSTATE Postgres reports when lock_timeout +// expires while waiting for a lock (55P03 lock_not_available). +const lockNotAvailableSQLState = "55P03" + +// TenantWriteLockKey returns the advisory lock key string that serializes +// tenant-owned writes against tenant data purges for the given tenant. +// Format: "tenant_write|:" (length-prefixed to keep distinct +// tenant IDs from aliasing); the key is hashed in SQL via +// hashtextextended(key, 0). Exported because the format is a cross-process +// contract: the API, the worker, and integration tests must compute the +// identical key to coordinate on the same lock. +func TenantWriteLockKey(tenantID string) string { + return "tenant_write|" + strconv.Itoa(len(tenantID)) + ":" + tenantID +} + +// tenantWriteTx is the transaction surface tenant-owned mutations run on. +// pgx.Tx satisfies it. +type tenantWriteTx interface { + Exec(ctx context.Context, sql string, arguments ...any) (pgconn.CommandTag, error) + Query(ctx context.Context, sql string, args ...any) (pgx.Rows, error) + QueryRow(ctx context.Context, sql string, args ...any) pgx.Row + Commit(ctx context.Context) error + Rollback(ctx context.Context) error +} + +// tenantWriteTxBeginner abstracts transaction creation so the helper is +// testable without a database. +type tenantWriteTxBeginner interface { + BeginTx(ctx context.Context, txOptions pgx.TxOptions) (tenantWriteTx, error) +} + +// tenantWritePool adapts *pgxpool.Pool to tenantWriteTxBeginner. +type tenantWritePool struct { + db *pgxpool.Pool +} + +func (p tenantWritePool) BeginTx(ctx context.Context, txOptions pgx.TxOptions) (tenantWriteTx, error) { + dbTx, err := p.db.BeginTx(ctx, txOptions) + if err != nil { + return nil, fmt.Errorf("begin tenant write transaction: %w", err) + } + + return dbTx, nil +} + +// withTenantWriteTx runs mutate inside a transaction that holds shared tenant +// write locks for the given tenant IDs (sorted and deduplicated). Pass no +// tenant IDs when the tenant boundary can only be resolved inside the +// transaction; mutate must then call tryLockTenantsShared itself before +// writing. A lock that cannot be acquired immediately aborts with +// *huberrors.TenantWriteConflictError without calling mutate. +func withTenantWriteTx( + ctx context.Context, db tenantWriteTxBeginner, tenantIDs []string, mutate func(dbTx tenantWriteTx) error, +) error { + dbTx, err := db.BeginTx(ctx, pgx.TxOptions{}) + if err != nil { + return fmt.Errorf("begin tenant write transaction: %w", err) + } + + defer func() { + if err := dbTx.Rollback(ctx); err != nil && !errors.Is(err, pgx.ErrTxClosed) { + slog.Error("tenant write tx: rollback failed", "error", err) + } + }() + + if err := tryLockTenantsShared(ctx, dbTx, tenantIDs); err != nil { + return err + } + + if err := mutate(dbTx); err != nil { + return err + } + + if err := dbTx.Commit(ctx); err != nil { + return fmt.Errorf("commit tenant write transaction: %w", err) + } + + return nil +} + +// tryLockTenantsShared acquires the shared tenant write lock for every given +// tenant ID (sorted, deduplicated) without waiting. It returns +// *huberrors.TenantWriteConflictError when any lock is unavailable, which +// means a tenant data purge for that tenant is running or waiting to run. +func tryLockTenantsShared(ctx context.Context, querier queryer, tenantIDs []string) error { + if len(tenantIDs) == 0 { + return nil + } + + sorted := slices.Clone(tenantIDs) + slices.Sort(sorted) + sorted = slices.Compact(sorted) + + for _, tenantID := range sorted { + var locked bool + if err := querier.QueryRow(ctx, tenantWriteLockSharedSQL, TenantWriteLockKey(tenantID)).Scan(&locked); err != nil { + return fmt.Errorf("acquire shared tenant write lock: %w", err) + } + + if !locked { + return huberrors.NewTenantWriteConflictError("tenant data purge in progress for this tenant; retry later") + } + } + + return nil +} + +// acquireTenantPurgeLock acquires the tenant write lock exclusively, waiting +// up to timeout for in-flight tenant-owned writes to drain. On success the +// transaction-local lock_timeout is reset to 0 so it does not apply to the +// purge's subsequent row and FK lock acquisitions. When the timeout expires +// it returns *huberrors.TenantWriteConflictError (retryable). +func acquireTenantPurgeLock( + ctx context.Context, exec tenantDataExecutor, tenantID string, timeout time.Duration, +) error { + timeoutMs := strconv.FormatInt(timeout.Milliseconds(), 10) + if _, err := exec.Exec(ctx, `SELECT set_config('lock_timeout', $1, true)`, timeoutMs); err != nil { + return fmt.Errorf("set tenant purge lock timeout: %w", err) + } + + if _, err := exec.Exec(ctx, tenantWriteLockExclusiveSQL, TenantWriteLockKey(tenantID)); err != nil { + var pgErr *pgconn.PgError + if errors.As(err, &pgErr) && pgErr.Code == lockNotAvailableSQLState { + return huberrors.NewTenantWriteConflictError("tenant-owned writes in progress; retry purge later") + } + + return fmt.Errorf("acquire exclusive tenant purge lock: %w", err) + } + + if _, err := exec.Exec(ctx, `SELECT set_config('lock_timeout', '0', true)`); err != nil { + return fmt.Errorf("reset tenant purge lock timeout: %w", err) + } + + return nil +} diff --git a/internal/repository/tenant_write_lock_test.go b/internal/repository/tenant_write_lock_test.go new file mode 100644 index 00000000..20a13708 --- /dev/null +++ b/internal/repository/tenant_write_lock_test.go @@ -0,0 +1,320 @@ +package repository + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgconn" + + "github.com/formbricks/hub/internal/huberrors" +) + +type fakeTenantWriteRow struct { + locked bool + scanErr error +} + +func (r fakeTenantWriteRow) Scan(dest ...any) error { + if r.scanErr != nil { + return r.scanErr + } + + if len(dest) == 1 { + if target, ok := dest[0].(*bool); ok { + *target = r.locked + } + } + + return nil +} + +type fakeTenantWriteTx struct { + fakeTenantDataExecutor + + lockResults []bool + lockScanErr error + lockErrAt int + lockKeys []string + commitErr error + rollbackErr error + committed bool + rolledBack bool +} + +func (f *fakeTenantWriteTx) Query(context.Context, string, ...any) (pgx.Rows, error) { + return nil, errors.New("query not implemented in fake") +} + +func (f *fakeTenantWriteTx) QueryRow(_ context.Context, _ string, args ...any) pgx.Row { + if len(args) == 1 { + if key, ok := args[0].(string); ok { + f.lockKeys = append(f.lockKeys, key) + } + } + + if f.lockErrAt == len(f.lockKeys) && f.lockScanErr != nil { + return fakeTenantWriteRow{scanErr: f.lockScanErr} + } + + lockIndex := len(f.lockKeys) - 1 + if lockIndex < len(f.lockResults) { + return fakeTenantWriteRow{locked: f.lockResults[lockIndex]} + } + + return fakeTenantWriteRow{locked: true} +} + +func (f *fakeTenantWriteTx) Commit(context.Context) error { + f.committed = true + + return f.commitErr +} + +func (f *fakeTenantWriteTx) Rollback(context.Context) error { + f.rolledBack = true + + return f.rollbackErr +} + +type fakeTenantWriteDB struct { + tx *fakeTenantWriteTx + beginErr error +} + +func (f *fakeTenantWriteDB) BeginTx(context.Context, pgx.TxOptions) (tenantWriteTx, error) { + if f.beginErr != nil { + return nil, f.beginErr + } + + return f.tx, nil +} + +func TestTenantWriteLockKey(t *testing.T) { + tests := []struct { + tenantID string + want string + }{ + {tenantID: "org-123", want: "tenant_write|7:org-123"}, + {tenantID: "", want: "tenant_write|0:"}, + {tenantID: "a|b", want: "tenant_write|3:a|b"}, + } + + for _, tt := range tests { + if got := TenantWriteLockKey(tt.tenantID); got != tt.want { + t.Errorf("TenantWriteLockKey(%q) = %q, want %q", tt.tenantID, got, tt.want) + } + } +} + +func TestWithTenantWriteTx(t *testing.T) { + t.Run("locks sorted deduplicated tenants, runs fn, commits", func(t *testing.T) { + transaction := &fakeTenantWriteTx{rollbackErr: pgx.ErrTxClosed} + fnCalled := false + + err := withTenantWriteTx( + context.Background(), &fakeTenantWriteDB{tx: transaction}, + []string{"org-b", "org-a", "org-b"}, + func(tenantWriteTx) error { + fnCalled = true + + return nil + }, + ) + if err != nil { + t.Fatalf("withTenantWriteTx() error = %v", err) + } + + if !fnCalled { + t.Fatal("fn was not called") + } + + if !transaction.committed { + t.Fatal("transaction was not committed") + } + + wantKeys := []string{TenantWriteLockKey("org-a"), TenantWriteLockKey("org-b")} + if len(transaction.lockKeys) != len(wantKeys) { + t.Fatalf("lock keys = %v, want %v", transaction.lockKeys, wantKeys) + } + + for keyIndex, want := range wantKeys { + if transaction.lockKeys[keyIndex] != want { + t.Fatalf("lock keys = %v, want %v", transaction.lockKeys, wantKeys) + } + } + }) + + t.Run("skips locking when no tenants given", func(t *testing.T) { + transaction := &fakeTenantWriteTx{rollbackErr: pgx.ErrTxClosed} + + err := withTenantWriteTx( + context.Background(), &fakeTenantWriteDB{tx: transaction}, nil, + func(tenantWriteTx) error { return nil }, + ) + if err != nil { + t.Fatalf("withTenantWriteTx() error = %v", err) + } + + if len(transaction.lockKeys) != 0 { + t.Fatalf("lock keys = %v, want none", transaction.lockKeys) + } + }) + + t.Run("unavailable lock returns conflict without calling fn", func(t *testing.T) { + transaction := &fakeTenantWriteTx{lockResults: []bool{false}} + fnCalled := false + + err := withTenantWriteTx( + context.Background(), &fakeTenantWriteDB{tx: transaction}, []string{"org-a"}, + func(tenantWriteTx) error { + fnCalled = true + + return nil + }, + ) + if !errors.Is(err, huberrors.ErrTenantWriteConflict) { + t.Fatalf("withTenantWriteTx() error = %v, want tenant write conflict", err) + } + + if fnCalled { + t.Fatal("fn was called despite lock conflict") + } + + if transaction.committed { + t.Fatal("transaction was committed despite lock conflict") + } + + if !transaction.rolledBack { + t.Fatal("transaction was not rolled back") + } + }) + + t.Run("lock scan error is wrapped", func(t *testing.T) { + scanErr := errors.New("scan failed") + transaction := &fakeTenantWriteTx{lockScanErr: scanErr, lockErrAt: 1} + + err := withTenantWriteTx( + context.Background(), &fakeTenantWriteDB{tx: transaction}, []string{"org-a"}, + func(tenantWriteTx) error { return nil }, + ) + if !errors.Is(err, scanErr) { + t.Fatalf("withTenantWriteTx() error = %v, want scan error", err) + } + + if errors.Is(err, huberrors.ErrTenantWriteConflict) { + t.Fatal("scan error must not be a tenant write conflict") + } + }) + + t.Run("fn error rolls back without commit", func(t *testing.T) { + fnErr := errors.New("fn failed") + transaction := &fakeTenantWriteTx{} + + err := withTenantWriteTx( + context.Background(), &fakeTenantWriteDB{tx: transaction}, []string{"org-a"}, + func(tenantWriteTx) error { return fnErr }, + ) + if !errors.Is(err, fnErr) { + t.Fatalf("withTenantWriteTx() error = %v, want fn error", err) + } + + if transaction.committed { + t.Fatal("transaction was committed after fn error") + } + + if !transaction.rolledBack { + t.Fatal("transaction was not rolled back") + } + }) + + t.Run("begin error is wrapped", func(t *testing.T) { + beginErr := errors.New("begin failed") + + err := withTenantWriteTx( + context.Background(), &fakeTenantWriteDB{beginErr: beginErr}, []string{"org-a"}, + func(tenantWriteTx) error { return nil }, + ) + if !errors.Is(err, beginErr) { + t.Fatalf("withTenantWriteTx() error = %v, want begin error", err) + } + }) + + t.Run("commit error is wrapped", func(t *testing.T) { + commitErr := errors.New("commit failed") + transaction := &fakeTenantWriteTx{commitErr: commitErr, rollbackErr: pgx.ErrTxClosed} + + err := withTenantWriteTx( + context.Background(), &fakeTenantWriteDB{tx: transaction}, []string{"org-a"}, + func(tenantWriteTx) error { return nil }, + ) + if !errors.Is(err, commitErr) { + t.Fatalf("withTenantWriteTx() error = %v, want commit error", err) + } + }) +} + +func TestAcquireTenantPurgeLock(t *testing.T) { + t.Run("sets timeout, locks exclusively, resets timeout", func(t *testing.T) { + exec := &fakeTenantDataExecutor{} + + err := acquireTenantPurgeLock(context.Background(), exec, "org-123", 5*time.Second) + if err != nil { + t.Fatalf("acquireTenantPurgeLock() error = %v", err) + } + + if len(exec.queries) != 3 { + t.Fatalf("queries = %d, want 3", len(exec.queries)) + } + + assertQueryContains(t, exec.queries[0], "set_config('lock_timeout', $1, true)") + assertQueryContains(t, exec.queries[1], "pg_advisory_xact_lock(hashtextextended($1, 0))") + assertQueryContains(t, exec.queries[2], "set_config('lock_timeout', '0', true)") + + if len(exec.args[0]) != 1 || exec.args[0][0] != "5000" { + t.Fatalf("set_config args = %#v, want timeout in milliseconds", exec.args[0]) + } + + if len(exec.args[1]) != 1 || exec.args[1][0] != TenantWriteLockKey("org-123") { + t.Fatalf("lock args = %#v, want tenant write lock key", exec.args[1]) + } + }) + + t.Run("lock timeout maps to tenant write conflict", func(t *testing.T) { + exec := &fakeTenantDataExecutor{ + errAtQuery: 2, + err: &pgconn.PgError{Code: lockNotAvailableSQLState}, + } + + err := acquireTenantPurgeLock(context.Background(), exec, "org-123", time.Second) + if !errors.Is(err, huberrors.ErrTenantWriteConflict) { + t.Fatalf("acquireTenantPurgeLock() error = %v, want tenant write conflict", err) + } + }) + + t.Run("other lock error is wrapped, not conflict", func(t *testing.T) { + lockErr := errors.New("connection lost") + exec := &fakeTenantDataExecutor{errAtQuery: 2, err: lockErr} + + err := acquireTenantPurgeLock(context.Background(), exec, "org-123", time.Second) + if !errors.Is(err, lockErr) { + t.Fatalf("acquireTenantPurgeLock() error = %v, want lock error", err) + } + + if errors.Is(err, huberrors.ErrTenantWriteConflict) { + t.Fatal("generic lock error must not be a tenant write conflict") + } + }) + + t.Run("set_config error is wrapped", func(t *testing.T) { + setErr := errors.New("set_config failed") + exec := &fakeTenantDataExecutor{errAtQuery: 1, err: setErr} + + err := acquireTenantPurgeLock(context.Background(), exec, "org-123", time.Second) + if !errors.Is(err, setErr) { + t.Fatalf("acquireTenantPurgeLock() error = %v, want set_config error", err) + } + }) +} diff --git a/internal/repository/webhooks_repository.go b/internal/repository/webhooks_repository.go index bd6fc975..2291825c 100644 --- a/internal/repository/webhooks_repository.go +++ b/internal/repository/webhooks_repository.go @@ -62,14 +62,21 @@ func (r *WebhooksRepository) Create(ctx context.Context, req *models.CreateWebho dbEventTypes []string ) - err := r.db.QueryRow(ctx, query, - req.URL, req.SigningKey, enabled, req.TenantID, eventTypes, - ).Scan( - &webhook.ID, &webhook.URL, &webhook.SigningKey, &webhook.Enabled, - &webhook.TenantID, &webhook.CreatedAt, &webhook.UpdatedAt, &dbEventTypes, - ) + err := withTenantWriteTx(ctx, tenantWritePool{db: r.db}, []string{*req.TenantID}, func(dbTx tenantWriteTx) error { + err := dbTx.QueryRow(ctx, query, + req.URL, req.SigningKey, enabled, req.TenantID, eventTypes, + ).Scan( + &webhook.ID, &webhook.URL, &webhook.SigningKey, &webhook.Enabled, + &webhook.TenantID, &webhook.CreatedAt, &webhook.UpdatedAt, &dbEventTypes, + ) + if err != nil { + return fmt.Errorf("failed to create webhook: %w", err) + } + + return nil + }) if err != nil { - return nil, fmt.Errorf("failed to create webhook: %w", err) + return nil, err } webhook.EventTypes, err = parseDBEventTypes(dbEventTypes) @@ -80,6 +87,26 @@ func (r *WebhooksRepository) Create(ctx context.Context, req *models.CreateWebho return &webhook, nil } +// resolveWebhookTenant reads the tenant boundary of a webhook inside the +// current transaction so the caller can acquire the tenant write lock before +// mutating. Webhook tenant_id is mutable (and NULL on legacy rows), so the +// subsequent mutation must stay scoped to this resolved value; zero affected +// rows then means the webhook moved tenants concurrently. +func resolveWebhookTenant(ctx context.Context, querier queryer, id uuid.UUID) (*string, error) { + var tenantID *string + + err := querier.QueryRow(ctx, `SELECT tenant_id FROM webhooks WHERE id = $1`, id).Scan(&tenantID) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return nil, huberrors.NewNotFoundError("webhook", "webhook not found") + } + + return nil, fmt.Errorf("resolve webhook tenant: %w", err) + } + + return tenantID, nil +} + // GetByID retrieves a single webhook by ID. func (r *WebhooksRepository) GetByID(ctx context.Context, id uuid.UUID) (*models.Webhook, error) { query := ` @@ -308,29 +335,59 @@ func (r *WebhooksRepository) Update(ctx context.Context, id uuid.UUID, req *mode args = append(args, id) + // The resolved tenant boundary is appended inside the transaction below. + // IS NOT DISTINCT FROM keeps the predicate correct for legacy NULL-tenant rows. query := fmt.Sprintf(` UPDATE webhooks SET %s - WHERE id = $%d + WHERE id = $%d AND tenant_id IS NOT DISTINCT FROM $%d RETURNING id, url, signing_key, enabled, tenant_id, created_at, updated_at, event_types, disabled_reason, disabled_at - `, strings.Join(updates, ", "), argCount) + `, strings.Join(updates, ", "), argCount, argCount+1) var ( webhook models.Webhook dbEventTypes []string ) - err := r.db.QueryRow(ctx, query, args...).Scan( - &webhook.ID, &webhook.URL, &webhook.SigningKey, &webhook.Enabled, - &webhook.TenantID, &webhook.CreatedAt, &webhook.UpdatedAt, &dbEventTypes, - &webhook.DisabledReason, &webhook.DisabledAt, - ) - if err != nil { - if errors.Is(err, pgx.ErrNoRows) { - return nil, huberrors.NewNotFoundError("webhook", "webhook not found") + err := withTenantWriteTx(ctx, tenantWritePool{db: r.db}, nil, func(dbTx tenantWriteTx) error { + currentTenantID, err := resolveWebhookTenant(ctx, dbTx, id) + if err != nil { + return err } - return nil, fmt.Errorf("failed to update webhook: %w", err) + // Lock the current tenant and, for tenant-changing updates, the target + // tenant as well, so the webhook cannot move into or out of a tenant + // that is being purged. + var lockTenants []string + if currentTenantID != nil { + lockTenants = append(lockTenants, *currentTenantID) + } + + if req.TenantID != nil { + lockTenants = append(lockTenants, *req.TenantID) + } + + if err := tryLockTenantsShared(ctx, dbTx, lockTenants); err != nil { + return err + } + + err = dbTx.QueryRow(ctx, query, append(args, currentTenantID)...).Scan( + &webhook.ID, &webhook.URL, &webhook.SigningKey, &webhook.Enabled, + &webhook.TenantID, &webhook.CreatedAt, &webhook.UpdatedAt, &dbEventTypes, + &webhook.DisabledReason, &webhook.DisabledAt, + ) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return huberrors.NewTenantWriteConflictError("webhook was modified concurrently; retry later") + } + + return fmt.Errorf("failed to update webhook: %w", err) + } + + return nil + }) + if err != nil { + return nil, err } webhook.EventTypes, err = parseDBEventTypes(dbEventTypes) @@ -345,19 +402,37 @@ func (r *WebhooksRepository) Update(ctx context.Context, id uuid.UUID, req *mode func (r *WebhooksRepository) Delete(ctx context.Context, id uuid.UUID) (*models.DeletedWebhook, error) { query := ` DELETE FROM webhooks - WHERE id = $1 + WHERE id = $1 AND tenant_id IS NOT DISTINCT FROM $2 RETURNING id, tenant_id ` var webhook models.DeletedWebhook - err := r.db.QueryRow(ctx, query, id).Scan(&webhook.ID, &webhook.TenantID) - if err != nil { - if errors.Is(err, pgx.ErrNoRows) { - return nil, huberrors.NewNotFoundError("webhook", "webhook not found") + err := withTenantWriteTx(ctx, tenantWritePool{db: r.db}, nil, func(dbTx tenantWriteTx) error { + currentTenantID, err := resolveWebhookTenant(ctx, dbTx, id) + if err != nil { + return err + } + + if currentTenantID != nil { + if err := tryLockTenantsShared(ctx, dbTx, []string{*currentTenantID}); err != nil { + return err + } } - return nil, fmt.Errorf("failed to delete webhook: %w", err) + err = dbTx.QueryRow(ctx, query, id, currentTenantID).Scan(&webhook.ID, &webhook.TenantID) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return huberrors.NewTenantWriteConflictError("webhook was modified concurrently; retry later") + } + + return fmt.Errorf("failed to delete webhook: %w", err) + } + + return nil + }) + if err != nil { + return nil, err } return &webhook, nil diff --git a/internal/workers/feedback_embedding.go b/internal/workers/feedback_embedding.go index 651d12d5..f0d4e42b 100644 --- a/internal/workers/feedback_embedding.go +++ b/internal/workers/feedback_embedding.go @@ -86,7 +86,7 @@ func (w *FeedbackEmbeddingWorker) Work(ctx context.Context, job *river.Job[servi text := service.BuildEmbeddingInput(record.FieldLabel, record.ValueText, w.docPrefix) if text == "" { - return w.handleEmptyText(ctx, args.FeedbackRecordID, args.Model, record, start) + return w.handleEmptyText(ctx, job, record, start) } embedding, err := w.embeddingClient.CreateEmbedding(ctx, text) @@ -119,18 +119,9 @@ func (w *FeedbackEmbeddingWorker) Work(ctx context.Context, job *river.Job[servi err = w.embeddingService.SetEmbedding(ctx, args.FeedbackRecordID, args.Model, embedding) if err != nil { - if w.metrics != nil { - w.metrics.RecordWorkerError(ctx, "update_failed") - w.metrics.RecordEmbeddingOutcome(ctx, "failed_final") - w.metrics.RecordEmbeddingDuration(ctx, time.Since(start), "failed_final") - } - - slog.Error("embedding: set embedding failed", - "feedback_record_id", args.FeedbackRecordID, - "error", err, - ) + isLastAttempt := job.Attempt >= job.MaxAttempts - return fmt.Errorf("set feedback record embedding: %w", err) + return w.handleSetEmbeddingError(ctx, err, args.FeedbackRecordID, start, isLastAttempt, "set feedback record embedding") } slog.Info("embedding: stored", @@ -145,29 +136,80 @@ func (w *FeedbackEmbeddingWorker) Work(ctx context.Context, job *river.Job[servi return nil } +// handleSetEmbeddingError maps embedding write failures to worker outcomes. +// A missing record means it was deleted or its tenant purged between fetch and +// write: the job completes (nothing left to embed). A tenant write conflict +// means a tenant data purge is in progress: the job retries via River, and the +// post-purge attempt finds the record gone and completes. Anything else fails +// the job as before. +func (w *FeedbackEmbeddingWorker) handleSetEmbeddingError( + ctx context.Context, + err error, + feedbackRecordID uuid.UUID, + start time.Time, + isLastAttempt bool, + action string, +) error { + switch { + case errors.Is(err, huberrors.ErrNotFound): + if w.metrics != nil { + w.metrics.RecordEmbeddingOutcome(ctx, "skipped") + w.metrics.RecordEmbeddingDuration(ctx, time.Since(start), "skipped") + } + + slog.Info("embedding: record gone before write, skipping", + "feedback_record_id", feedbackRecordID, + ) + + return nil + case errors.Is(err, huberrors.ErrTenantWriteConflict): + outcome := "retry" + if isLastAttempt { + outcome = "failed_final" + } + + if w.metrics != nil { + w.metrics.RecordWorkerError(ctx, "tenant_write_conflict") + w.metrics.RecordEmbeddingOutcome(ctx, outcome) + w.metrics.RecordEmbeddingDuration(ctx, time.Since(start), outcome) + } + + slog.Warn("embedding: tenant data purge in progress, deferring write", + "feedback_record_id", feedbackRecordID, + ) + + return fmt.Errorf("%s: %w", action, err) + default: + if w.metrics != nil { + w.metrics.RecordWorkerError(ctx, "update_failed") + w.metrics.RecordEmbeddingOutcome(ctx, "failed_final") + w.metrics.RecordEmbeddingDuration(ctx, time.Since(start), "failed_final") + } + + slog.Error("embedding: "+action+" failed", + "feedback_record_id", feedbackRecordID, + "error", err, + ) + + return fmt.Errorf("%s: %w", action, err) + } +} + // handleEmptyText clears the embedding for text fields when value_text is empty, or records skip for non-text. func (w *FeedbackEmbeddingWorker) handleEmptyText( ctx context.Context, - feedbackRecordID uuid.UUID, - model string, + job *river.Job[service.FeedbackEmbeddingArgs], record *models.FeedbackRecord, start time.Time, ) error { + feedbackRecordID := job.Args.FeedbackRecordID + if record.FieldType == models.FieldTypeText { - err := w.embeddingService.SetEmbedding(ctx, feedbackRecordID, model, nil) + err := w.embeddingService.SetEmbedding(ctx, feedbackRecordID, job.Args.Model, nil) if err != nil { - if w.metrics != nil { - w.metrics.RecordWorkerError(ctx, "update_failed") - w.metrics.RecordEmbeddingOutcome(ctx, "failed_final") - w.metrics.RecordEmbeddingDuration(ctx, time.Since(start), "failed_final") - } - - slog.Error("embedding: clear failed", - "feedback_record_id", feedbackRecordID, - "error", err, - ) + isLastAttempt := job.Attempt >= job.MaxAttempts - return fmt.Errorf("clear feedback record embedding: %w", err) + return w.handleSetEmbeddingError(ctx, err, feedbackRecordID, start, isLastAttempt, "clear feedback record embedding") } if w.metrics != nil { diff --git a/internal/workers/feedback_embedding_test.go b/internal/workers/feedback_embedding_test.go new file mode 100644 index 00000000..e8f2a403 --- /dev/null +++ b/internal/workers/feedback_embedding_test.go @@ -0,0 +1,155 @@ +package workers + +import ( + "context" + "errors" + "testing" + + "github.com/google/uuid" + "github.com/riverqueue/river" + "github.com/riverqueue/river/rivertype" + + "github.com/formbricks/hub/internal/huberrors" + "github.com/formbricks/hub/internal/models" + "github.com/formbricks/hub/internal/service" +) + +type mockEmbeddingService struct { + record *models.FeedbackRecord + getErr error + setErr error + setCalls int + setEmbeddingNil bool +} + +func (m *mockEmbeddingService) GetFeedbackRecord(_ context.Context, _ uuid.UUID) (*models.FeedbackRecord, error) { + return m.record, m.getErr +} + +func (m *mockEmbeddingService) SetEmbedding( + _ context.Context, _ uuid.UUID, _ string, embedding []float32, +) error { + m.setCalls++ + m.setEmbeddingNil = embedding == nil + + return m.setErr +} + +type mockEmbeddingClient struct { + embedding []float32 + err error +} + +func (m *mockEmbeddingClient) CreateEmbedding(_ context.Context, _ string) ([]float32, error) { + return m.embedding, m.err +} + +func (m *mockEmbeddingClient) CreateEmbeddingForQuery(_ context.Context, _ string) ([]float32, error) { + return m.embedding, m.err +} + +func embeddingJob() *river.Job[service.FeedbackEmbeddingArgs] { + return &river.Job[service.FeedbackEmbeddingArgs]{ + JobRow: &rivertype.JobRow{Attempt: 1, MaxAttempts: 3}, + Args: service.FeedbackEmbeddingArgs{ + FeedbackRecordID: uuid.Must(uuid.NewV7()), + Model: "test-model", + }, + } +} + +func textRecord(valueText string) *models.FeedbackRecord { + label := "How was it?" + + record := &models.FeedbackRecord{FieldType: models.FieldTypeText, FieldLabel: &label} + if valueText != "" { + record.ValueText = &valueText + } + + return record +} + +func TestFeedbackEmbeddingWorker_Work_SetEmbeddingConflict(t *testing.T) { + ctx := context.Background() + + t.Run("tenant write conflict returns error so River retries", func(t *testing.T) { + svc := &mockEmbeddingService{ + record: textRecord("great product"), + setErr: huberrors.NewTenantWriteConflictError(""), + } + worker := NewFeedbackEmbeddingWorker(svc, &mockEmbeddingClient{embedding: []float32{0.1}}, "", nil) + + err := worker.Work(ctx, embeddingJob()) + if !errors.Is(err, huberrors.ErrTenantWriteConflict) { + t.Fatalf("Work() error = %v, want tenant write conflict for retry", err) + } + }) + + t.Run("record gone before write completes the job", func(t *testing.T) { + svc := &mockEmbeddingService{ + record: textRecord("great product"), + setErr: huberrors.NewNotFoundError("feedback record", ""), + } + worker := NewFeedbackEmbeddingWorker(svc, &mockEmbeddingClient{embedding: []float32{0.1}}, "", nil) + + err := worker.Work(ctx, embeddingJob()) + if err != nil { + t.Fatalf("Work() error = %v, want nil (record purged, nothing to embed)", err) + } + + if svc.setCalls != 1 { + t.Fatalf("SetEmbedding called %d times, want 1", svc.setCalls) + } + }) + + t.Run("other set errors still fail the job", func(t *testing.T) { + svc := &mockEmbeddingService{ + record: textRecord("great product"), + setErr: errors.New("connection lost"), + } + worker := NewFeedbackEmbeddingWorker(svc, &mockEmbeddingClient{embedding: []float32{0.1}}, "", nil) + + err := worker.Work(ctx, embeddingJob()) + if err == nil { + t.Fatal("Work() error = nil, want error") + } + + if errors.Is(err, huberrors.ErrTenantWriteConflict) { + t.Fatalf("Work() error = %v, must not be a tenant write conflict", err) + } + }) +} + +func TestFeedbackEmbeddingWorker_Work_EmptyTextConflict(t *testing.T) { + ctx := context.Background() + + t.Run("clear during purge returns error so River retries", func(t *testing.T) { + svc := &mockEmbeddingService{ + record: textRecord(""), + setErr: huberrors.NewTenantWriteConflictError(""), + } + worker := NewFeedbackEmbeddingWorker(svc, &mockEmbeddingClient{}, "", nil) + + err := worker.Work(ctx, embeddingJob()) + if !errors.Is(err, huberrors.ErrTenantWriteConflict) { + t.Fatalf("Work() error = %v, want tenant write conflict for retry", err) + } + + if !svc.setEmbeddingNil { + t.Fatal("SetEmbedding should have been called with nil to clear the embedding") + } + }) + + t.Run("record gone before clear completes the job", func(t *testing.T) { + svc := &mockEmbeddingService{ + record: textRecord(""), + setErr: huberrors.NewNotFoundError("feedback record", ""), + } + worker := NewFeedbackEmbeddingWorker(svc, &mockEmbeddingClient{}, "", nil) + + err := worker.Work(ctx, embeddingJob()) + if err != nil { + t.Fatalf("Work() error = %v, want nil (record purged, nothing to clear)", err) + } + }) +} diff --git a/internal/workers/webhook_dispatch.go b/internal/workers/webhook_dispatch.go index d45ac639..38e473f7 100644 --- a/internal/workers/webhook_dispatch.go +++ b/internal/workers/webhook_dispatch.go @@ -2,6 +2,7 @@ package workers import ( "context" + "errors" "fmt" "log/slog" "time" @@ -9,6 +10,7 @@ import ( "github.com/google/uuid" "github.com/riverqueue/river" + "github.com/formbricks/hub/internal/huberrors" "github.com/formbricks/hub/internal/models" "github.com/formbricks/hub/internal/observability" "github.com/formbricks/hub/internal/service" @@ -175,7 +177,16 @@ func (w *WebhookDispatchWorker) Work(ctx context.Context, job *river.Job[service DisabledReason: &reason, DisabledAt: &now, }) - if updateErr != nil { + + switch { + case updateErr == nil: + case errors.Is(updateErr, huberrors.ErrTenantWriteConflict): + // A tenant data purge is deleting this webhook; disabling it is moot. + slog.Warn("webhook dispatch: disable skipped, tenant purge in progress", + "webhook_id", webhook.ID, + "event_id", args.EventID, + ) + default: slog.Error("webhook dispatch: failed to disable webhook after max attempts", "webhook_id", webhook.ID, "event_id", args.EventID, diff --git a/internal/workers/webhook_dispatch_test.go b/internal/workers/webhook_dispatch_test.go index 72fc72ff..51da4a8e 100644 --- a/internal/workers/webhook_dispatch_test.go +++ b/internal/workers/webhook_dispatch_test.go @@ -10,14 +10,16 @@ import ( "github.com/riverqueue/river" "github.com/riverqueue/river/rivertype" + "github.com/formbricks/hub/internal/huberrors" "github.com/formbricks/hub/internal/models" "github.com/formbricks/hub/internal/service" ) type mockDispatchRepo struct { - webhook *models.Webhook - err error - update *models.UpdateWebhookRequest + webhook *models.Webhook + err error + update *models.UpdateWebhookRequest + updateErr error } func (m *mockDispatchRepo) GetByID(_ context.Context, _ uuid.UUID) (*models.Webhook, error) { @@ -27,6 +29,10 @@ func (m *mockDispatchRepo) GetByID(_ context.Context, _ uuid.UUID) (*models.Webh func (m *mockDispatchRepo) Update(_ context.Context, _ uuid.UUID, req *models.UpdateWebhookRequest) (*models.Webhook, error) { m.update = req + if m.updateErr != nil { + return nil, m.updateErr + } + return nil, nil } @@ -329,6 +335,32 @@ func TestWebhookDispatchWorker_Work(t *testing.T) { t.Error("DisabledAt should be set") } }) + + t.Run("swallows disable conflict during tenant purge and still returns send error", func(t *testing.T) { + repo := &mockDispatchRepo{ + webhook: &models.Webhook{ID: webhookID, Enabled: true, URL: "http://x", SigningKey: "sk", TenantID: &tenantID}, + updateErr: huberrors.NewTenantWriteConflictError(""), + } + sender := &mockSender{err: errors.New("final failure")} + worker := NewWebhookDispatchWorker(repo, sender, 15*time.Second, nil) + job := &river.Job[service.WebhookDispatchArgs]{ + JobRow: &rivertype.JobRow{Attempt: 3, MaxAttempts: 3}, + Args: args, + } + + err := worker.Work(ctx, job) + if err == nil { + t.Error("Work() error = nil, want the send error") + } + + if errors.Is(err, huberrors.ErrTenantWriteConflict) { + t.Errorf("Work() error = %v, want the send error, not the disable conflict", err) + } + + if repo.update == nil { + t.Error("disable update should have been attempted") + } + }) } func TestWebhookDispatchWorker_Timeout(t *testing.T) { diff --git a/openapi.yaml b/openapi.yaml index 4b12fbdc..5db57972 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -406,7 +406,10 @@ paths: - name: "field_type" reason: "has invalid value \"textt\"; must be one of: text, categorical, nps, csat, ces, rating, number, boolean, date" "409": - description: Conflict – duplicate tenant_id/submission_id/field_id. + description: | + Conflict. Either a duplicate tenant_id/submission_id/field_id (code `conflict`, terminal), + or a tenant data purge is in progress for this tenant (code `tenant_write_conflict`, + retryable – retry after the purge completes). content: application/problem+json: schema: @@ -469,6 +472,14 @@ paths: application/problem+json: schema: $ref: '#/components/schemas/ErrorModel' + "409": + description: | + Conflict – a tenant data purge is in progress for a tenant holding records for this user + (code `tenant_write_conflict`). No records were deleted; retry after the purge completes. + content: + application/problem+json: + schema: + $ref: '#/components/schemas/ErrorModel' default: description: Error content: @@ -546,6 +557,14 @@ paths: application/problem+json: schema: $ref: '#/components/schemas/ErrorModel' + "409": + description: | + Conflict – a tenant data purge is in progress for this record's tenant + (code `tenant_write_conflict`); retry after the purge completes. + content: + application/problem+json: + schema: + $ref: '#/components/schemas/ErrorModel' default: description: Error content: @@ -636,6 +655,14 @@ paths: application/problem+json: schema: $ref: '#/components/schemas/ErrorModel' + "409": + description: | + Conflict – a tenant data purge is in progress for this record's tenant + (code `tenant_write_conflict`); retry after the purge completes. + content: + application/problem+json: + schema: + $ref: '#/components/schemas/ErrorModel' default: description: Error content: @@ -943,6 +970,14 @@ paths: application/problem+json: schema: $ref: '#/components/schemas/ErrorModel' + "409": + description: | + Conflict – a tenant data purge is in progress for this tenant + (code `tenant_write_conflict`); retry after the purge completes. + content: + application/problem+json: + schema: + $ref: '#/components/schemas/ErrorModel' default: description: Error content: @@ -1030,6 +1065,15 @@ paths: application/problem+json: schema: $ref: '#/components/schemas/ErrorModel' + "409": + description: | + Conflict – a tenant data purge is in progress for this webhook's current (or target) + tenant, or the webhook was modified concurrently (code `tenant_write_conflict`); + retry after the purge completes. + content: + application/problem+json: + schema: + $ref: '#/components/schemas/ErrorModel' default: description: Error content: @@ -1066,6 +1110,15 @@ paths: application/problem+json: schema: $ref: '#/components/schemas/ErrorModel' + "409": + description: | + Conflict – a tenant data purge is in progress for this webhook's tenant, or the + webhook was modified concurrently (code `tenant_write_conflict`); retry after the + purge completes. + content: + application/problem+json: + schema: + $ref: '#/components/schemas/ErrorModel' default: description: Error content: @@ -1080,11 +1133,19 @@ paths: description: | Permanently deletes Hub-owned data for the specified tenant_id. This endpoint is intended for tenant/account offboarding after the tenant has been deprovisioned and upstream writes for that - tenant have stopped. This includes feedback records, derived embeddings, and webhooks for the - tenant. This operation is synchronous and idempotent; repeated calls return zero counts after the - tenant data has already been deleted. Concurrent writes for the same tenant_id are not serialized - by Hub in this version. - No webhook events are published as part of this purge operation. + tenant have stopped. This includes feedback records, derived embeddings, taxonomy data, and + webhooks for the tenant. This operation is synchronous and idempotent; repeated calls return zero + counts after the tenant data has already been deleted. + + The purge is serialized against tenant-owned writes: while it runs, Hub-owned writes for the same + tenant_id are rejected with HTTP 409 (code `tenant_write_conflict`); writes for other tenants are + unaffected. If tenant-owned writes are in flight when the purge starts, the purge waits up to a + configured lock timeout for them to drain and then returns a retryable 409. + + No webhook events are published as part of this purge operation, and writes rejected during the + purge publish no events. Webhook deliveries already in flight when the purge commits complete + normally, and previously enqueued delivery jobs no-op after the purge (their queued payloads are + removed by the job queue's retention pruning rather than by this endpoint). operationId: delete-tenant-data parameters: - name: tenant_id @@ -1119,6 +1180,14 @@ paths: application/problem+json: schema: $ref: '#/components/schemas/ErrorModel' + "409": + description: | + Conflict – tenant-owned writes were in flight and did not drain within the purge + lock timeout (code `tenant_write_conflict`). No data was deleted; retry the purge. + content: + application/problem+json: + schema: + $ref: '#/components/schemas/ErrorModel' default: description: Error content: @@ -1405,7 +1474,9 @@ components: description: >- Stable, machine-readable error code; the primary value to branch on. service_unavailable indicates a feature or dependency is temporarily - unavailable and the request may be retried later; all other codes are + unavailable and the request may be retried later. tenant_write_conflict + indicates the write conflicted with an in-progress tenant data purge + (or vice versa) and may be retried later unchanged. All other codes are terminal until the request itself is changed. enum: - validation @@ -1414,6 +1485,7 @@ components: - forbidden - not_found - conflict + - tenant_write_conflict - method_not_allowed - service_unavailable - internal_server_error diff --git a/tests/integration_test.go b/tests/integration_test.go index cd9153da..45014473 100644 --- a/tests/integration_test.go +++ b/tests/integration_test.go @@ -98,7 +98,7 @@ func setupTestServerWithEventProviders( // Initialize repository, service, and handler layers feedbackRecordsRepo := repository.NewFeedbackRecordsRepository(db) embeddingsRepo := repository.NewEmbeddingsRepository(db) - tenantDataRepo := repository.NewTenantDataRepository(db) + tenantDataRepo := repository.NewTenantDataRepository(db, cfg.TenantData.PurgeLockTimeout.Duration()) feedbackRecordsService := service.NewFeedbackRecordsService( feedbackRecordsRepo, embeddingsRepo, diff --git a/tests/tenant_write_lock_test.go b/tests/tenant_write_lock_test.go new file mode 100644 index 00000000..7b5be1f8 --- /dev/null +++ b/tests/tenant_write_lock_test.go @@ -0,0 +1,591 @@ +package tests + +import ( + "bytes" + "context" + "encoding/json" + "io" + "net/http" + "net/url" + "testing" + "time" + + "github.com/google/uuid" + "github.com/jackc/pgx/v5/pgxpool" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/formbricks/hub/internal/config" + "github.com/formbricks/hub/internal/huberrors" + "github.com/formbricks/hub/internal/models" + "github.com/formbricks/hub/internal/repository" + "github.com/formbricks/hub/pkg/database" +) + +// Tenant write serialization tests (ENG-1013). +// +// Determinism: instead of racing goroutines, these tests hold the tenant write +// advisory lock directly on a dedicated connection — exclusively to simulate a +// purge in progress, or shared to simulate an in-flight tenant-owned write — +// and then exercise the real code paths against it. + +func newTenantLockDB(ctx context.Context, t *testing.T) *pgxpool.Pool { + t.Helper() + + cfg, err := config.Load() + require.NoError(t, err) + + db, err := database.NewPostgresPool(ctx, cfg.Database.URL, + database.WithPoolConfig(cfg.Database.PoolConfig()), + ) + require.NoError(t, err) + + t.Cleanup(db.Close) + + return db +} + +// holdTenantWriteLock acquires the tenant write advisory lock on its own +// connection and returns an idempotent release func. +func holdTenantWriteLock( + ctx context.Context, t *testing.T, db *pgxpool.Pool, tenantID string, shared bool, +) func() { + t.Helper() + + conn, err := db.Acquire(ctx) + require.NoError(t, err) + + lockTx, err := conn.Begin(ctx) + require.NoError(t, err) + + lockSQL := `SELECT pg_advisory_xact_lock(hashtextextended($1, 0))` + if shared { + lockSQL = `SELECT pg_advisory_xact_lock_shared(hashtextextended($1, 0))` + } + + _, err = lockTx.Exec(ctx, lockSQL, repository.TenantWriteLockKey(tenantID)) + require.NoError(t, err) + + released := false + release := func() { + if released { + return + } + + released = true + + _ = lockTx.Rollback(ctx) + + conn.Release() + } + + t.Cleanup(release) + + return release +} + +// doTenantLockRequest performs an authenticated request and returns the +// status code and the fully read (and closed) response body. +func doTenantLockRequest( + ctx context.Context, t *testing.T, client *http.Client, method, requestURL string, payload any, +) (int, []byte) { + t.Helper() + + var reqBody io.Reader = http.NoBody + + if payload != nil { + raw, err := json.Marshal(payload) + require.NoError(t, err) + + reqBody = bytes.NewBuffer(raw) + } + + req, err := http.NewRequestWithContext(ctx, method, requestURL, reqBody) + require.NoError(t, err) + req.Header.Set("Authorization", "Bearer "+testAPIKey) + + if payload != nil { + req.Header.Set("Content-Type", "application/json") + } + + resp, err := client.Do(req) + require.NoError(t, err) + + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + + return resp.StatusCode, body +} + +// requireTenantWriteConflict asserts a 409 carrying the dedicated retryable +// problem code. +func requireTenantWriteConflict(t *testing.T, status int, body []byte) { + t.Helper() + + var problem struct { + Code string `json:"code"` + Type string `json:"type"` + } + + require.Equal(t, http.StatusConflict, status, "body: %s", string(body)) + require.NoError(t, json.Unmarshal(body, &problem)) + assert.Equal(t, "tenant_write_conflict", problem.Code) + assert.Equal(t, "https://hub.formbricks.com/problems/tenant-write-conflict", problem.Type) +} + +func feedbackRecordBody(tenantID string) map[string]any { + return map[string]any{ + "source_type": "formbricks", + "submission_id": uuid.NewString(), + "tenant_id": tenantID, + "field_id": "tenant-lock-field", + "field_type": "text", + "value_text": "tenant write lock test", + } +} + +func (r *tenantDataEventRecorder) totalEventCount() int { + r.mu.Lock() + defer r.mu.Unlock() + + return len(r.events) +} + +func waitForEventCount(t *testing.T, recorder *tenantDataEventRecorder, want int) { + t.Helper() + + deadline := time.Now().Add(2 * time.Second) + for time.Now().Before(deadline) { + if recorder.totalEventCount() >= want { + return + } + + time.Sleep(10 * time.Millisecond) + } + + t.Fatalf("event count = %d, want at least %d", recorder.totalEventCount(), want) +} + +// requireEventCountStays asserts no further events are published for a short +// observation window (rejected writes must publish nothing). +func requireEventCountStays(t *testing.T, recorder *tenantDataEventRecorder, want int) { + t.Helper() + + deadline := time.Now().Add(150 * time.Millisecond) + for time.Now().Before(deadline) { + if count := recorder.totalEventCount(); count != want { + t.Fatalf("event count = %d, want %d (rejected writes must publish no events)", count, want) + } + + time.Sleep(10 * time.Millisecond) + } +} + +// TestTenantPurgeWaitsForInFlightWritesThenConflicts covers the purge side: +// an in-flight tenant-owned write (shared lock holder) makes the purge wait up +// to its lock timeout and return a retryable 409; after the writer finishes +// the purge succeeds. +func TestTenantPurgeWaitsForInFlightWritesThenConflicts(t *testing.T) { + t.Setenv("TENANT_PURGE_LOCK_TIMEOUT_SECONDS", "1") + + server, cleanup := setupTestServer(t) + defer cleanup() + + ctx := context.Background() + db := newTenantLockDB(ctx, t) + client := &http.Client{} + tenantA := "tenant-lock-purge-" + uuid.NewString() + + createTenantDataFeedbackRecord(ctx, t, client, server.URL, tenantA, uuid.NewString(), "tenant-lock-field") + + release := holdTenantWriteLock(ctx, t, db, tenantA, true) + + purgeURL := server.URL + "/v1/tenants/" + url.PathEscape(tenantA) + "/data" + + started := time.Now() + status, body := doTenantLockRequest(ctx, t, client, http.MethodDelete, purgeURL, nil) + requireTenantWriteConflict(t, status, body) + assert.GreaterOrEqual(t, time.Since(started), 900*time.Millisecond, + "purge should have waited for the lock timeout before conflicting") + + // The failed purge attempt deleted nothing. + recordsURL := server.URL + "/v1/feedback-records?tenant_id=" + url.QueryEscape(tenantA) + status, body = doTenantLockRequest(ctx, t, client, http.MethodGet, recordsURL, nil) + + var listed struct { + Data []models.FeedbackRecord `json:"data"` + } + + require.Equal(t, http.StatusOK, status) + require.NoError(t, json.Unmarshal(body, &listed)) + require.Len(t, listed.Data, 1, "record must survive the conflicted purge") + + // After the in-flight writer drains, the purge succeeds. + release() + + purgeResp := deleteTenantData(ctx, t, client, server.URL, tenantA) + assert.Equal(t, int64(1), purgeResp.DeletedFeedbackRecords) +} + +// TestTenantWritesRejectedDuringPurge covers the writer side: while the purge +// holds the exclusive tenant lock, every same-tenant mutation returns the +// retryable 409 and publishes no events, while other tenants are unaffected. +func TestTenantWritesRejectedDuringPurge(t *testing.T) { + eventRecorder := &tenantDataEventRecorder{} + + server, cleanup := setupTestServerWithEventProviders(t, eventRecorder) + defer cleanup() + + ctx := context.Background() + db := newTenantLockDB(ctx, t) + client := &http.Client{} + tenantA := "tenant-lock-writes-a-" + uuid.NewString() + tenantB := "tenant-lock-writes-b-" + uuid.NewString() + + // Seed tenant A with a record and a webhook before the purge starts. + record := createTenantDataFeedbackRecord(ctx, t, client, server.URL, tenantA, uuid.NewString(), "tenant-lock-field") + webhook := createTenantDataWebhook(ctx, t, client, server.URL, tenantA, "tenant-lock-writes") + + // Both seed writes publish events; wait for them so the no-new-events + // assertion below is anchored at a stable count. + waitForEventCount(t, eventRecorder, 2) + + baseline := eventRecorder.totalEventCount() + + release := holdTenantWriteLock(ctx, t, db, tenantA, false) + defer release() + + recordURL := server.URL + "/v1/feedback-records/" + record.ID.String() + webhookURL := server.URL + "/v1/webhooks/" + webhook.ID.String() + + t.Run("feedback record create rejected", func(t *testing.T) { + status, body := doTenantLockRequest( + ctx, t, client, http.MethodPost, server.URL+"/v1/feedback-records", feedbackRecordBody(tenantA)) + requireTenantWriteConflict(t, status, body) + }) + + t.Run("feedback record update rejected", func(t *testing.T) { + status, body := doTenantLockRequest(ctx, t, client, http.MethodPatch, recordURL, map[string]any{"value_text": "blocked"}) + requireTenantWriteConflict(t, status, body) + }) + + t.Run("feedback record delete rejected", func(t *testing.T) { + status, body := doTenantLockRequest(ctx, t, client, http.MethodDelete, recordURL, nil) + requireTenantWriteConflict(t, status, body) + }) + + t.Run("webhook create rejected", func(t *testing.T) { + status, body := doTenantLockRequest(ctx, t, client, http.MethodPost, server.URL+"/v1/webhooks", + map[string]any{"url": testWebhookURL + "/tenant-lock-blocked", "tenant_id": tenantA}) + requireTenantWriteConflict(t, status, body) + }) + + t.Run("webhook update rejected", func(t *testing.T) { + status, body := doTenantLockRequest(ctx, t, client, http.MethodPatch, webhookURL, map[string]any{"enabled": false}) + requireTenantWriteConflict(t, status, body) + }) + + t.Run("webhook delete rejected", func(t *testing.T) { + status, body := doTenantLockRequest(ctx, t, client, http.MethodDelete, webhookURL, nil) + requireTenantWriteConflict(t, status, body) + }) + + t.Run("no events published for rejected writes", func(t *testing.T) { + requireEventCountStays(t, eventRecorder, baseline) + }) + + t.Run("other tenants keep writing", func(t *testing.T) { + status, body := doTenantLockRequest( + ctx, t, client, http.MethodPost, server.URL+"/v1/feedback-records", feedbackRecordBody(tenantB)) + require.Equal(t, http.StatusCreated, status, "body: %s", string(body)) + + status, body = doTenantLockRequest(ctx, t, client, http.MethodPost, server.URL+"/v1/webhooks", + map[string]any{"url": testWebhookURL + "/tenant-lock-other", "tenant_id": tenantB}) + require.Equal(t, http.StatusCreated, status, "body: %s", string(body)) + }) + + t.Run("writes succeed again after purge releases", func(t *testing.T) { + release() + + status, body := doTenantLockRequest(ctx, t, client, http.MethodPatch, recordURL, map[string]any{"value_text": "unblocked"}) + require.Equal(t, http.StatusOK, status, "body: %s", string(body)) + }) + + cleanupTenantDataBestEffort(ctx, client, server.URL, tenantA) + cleanupTenantDataBestEffort(ctx, client, server.URL, tenantB) +} + +// TestTenantWritesFailFastWhilePurgeQueued covers the load-bearing queueing +// property: the moment a purge is WAITING for the exclusive lock (an in-flight +// writer still holds shared), brand-new same-tenant writes are rejected +// immediately rather than being granted ahead of the queued purge. +func TestTenantWritesFailFastWhilePurgeQueued(t *testing.T) { + t.Setenv("TENANT_PURGE_LOCK_TIMEOUT_SECONDS", "10") + + server, cleanup := setupTestServer(t) + defer cleanup() + + ctx := context.Background() + db := newTenantLockDB(ctx, t) + client := &http.Client{} + tenantA := "tenant-lock-queued-" + uuid.NewString() + + createTenantDataFeedbackRecord(ctx, t, client, server.URL, tenantA, uuid.NewString(), "tenant-lock-field") + + release := holdTenantWriteLock(ctx, t, db, tenantA, true) + defer release() + + purgeURL := server.URL + "/v1/tenants/" + url.PathEscape(tenantA) + "/data" + purgeStatus := make(chan int, 1) + + go func() { + req, err := http.NewRequestWithContext(ctx, http.MethodDelete, purgeURL, http.NoBody) + if err != nil { + purgeStatus <- -1 + + return + } + + req.Header.Set("Authorization", "Bearer "+testAPIKey) + + resp, err := (&http.Client{}).Do(req) + if err != nil { + purgeStatus <- -1 + + return + } + + _, _ = io.Copy(io.Discard, resp.Body) + _ = resp.Body.Close() + + purgeStatus <- resp.StatusCode + }() + + // Wait until the purge's exclusive advisory lock request is queued. + require.Eventually(t, func() bool { + var waiting int + + err := db.QueryRow(ctx, + `SELECT count(*) FROM pg_locks WHERE locktype = 'advisory' AND NOT granted`, + ).Scan(&waiting) + + return err == nil && waiting > 0 + }, 5*time.Second, 20*time.Millisecond, "purge never queued for the advisory lock") + + // A new same-tenant write must fail fast while the purge is queued. + status, body := doTenantLockRequest( + ctx, t, client, http.MethodPost, server.URL+"/v1/feedback-records", feedbackRecordBody(tenantA)) + requireTenantWriteConflict(t, status, body) + + // Release the in-flight writer; the queued purge must now complete. + release() + + select { + case status := <-purgeStatus: + require.Equal(t, http.StatusOK, status, "queued purge should succeed once writers drain") + case <-time.After(10 * time.Second): + t.Fatal("queued purge did not finish after writer released") + } +} + +// TestGDPRDeleteByUserDuringPurge covers the cross-tenant erasure flow: the +// unfiltered delete spans tenants A and B; with A under purge the whole call +// conflicts and deletes nothing, while the tenant-B-scoped call proceeds. +func TestGDPRDeleteByUserDuringPurge(t *testing.T) { + server, cleanup := setupTestServer(t) + defer cleanup() + + ctx := context.Background() + db := newTenantLockDB(ctx, t) + client := &http.Client{} + tenantA := "tenant-lock-gdpr-a-" + uuid.NewString() + tenantB := "tenant-lock-gdpr-b-" + uuid.NewString() + userID := "tenant-lock-gdpr-user-" + uuid.NewString() + + createUserRecord := func(tenantID string) models.FeedbackRecord { + requestBody := feedbackRecordBody(tenantID) + requestBody["user_id"] = userID + + status, body := doTenantLockRequest(ctx, t, client, http.MethodPost, server.URL+"/v1/feedback-records", requestBody) + + var record models.FeedbackRecord + + require.Equal(t, http.StatusCreated, status, "body: %s", string(body)) + require.NoError(t, json.Unmarshal(body, &record)) + + return record + } + + recordA := createUserRecord(tenantA) + recordB := createUserRecord(tenantB) + + release := holdTenantWriteLock(ctx, t, db, tenantA, false) + defer release() + + deleteByUserURL := server.URL + "/v1/feedback-records?user_id=" + url.QueryEscape(userID) + + t.Run("unfiltered delete conflicts and deletes nothing", func(t *testing.T) { + status, body := doTenantLockRequest(ctx, t, client, http.MethodDelete, deleteByUserURL, nil) + requireTenantWriteConflict(t, status, body) + + for _, id := range []uuid.UUID{recordA.ID, recordB.ID} { + getStatus, getBody := doTenantLockRequest( + ctx, t, client, http.MethodGet, server.URL+"/v1/feedback-records/"+id.String(), nil) + require.Equal(t, http.StatusOK, getStatus, "body: %s", string(getBody)) + } + }) + + t.Run("tenant-scoped delete for unlocked tenant proceeds", func(t *testing.T) { + status, body := doTenantLockRequest(ctx, t, client, http.MethodDelete, + deleteByUserURL+"&tenant_id="+url.QueryEscape(tenantB), nil) + + var deleted models.DeleteFeedbackRecordsByUserResponse + + require.Equal(t, http.StatusOK, status, "body: %s", string(body)) + require.NoError(t, json.Unmarshal(body, &deleted)) + assert.Equal(t, int64(1), deleted.DeletedCount) + + getStatus, getBody := doTenantLockRequest( + ctx, t, client, http.MethodGet, server.URL+"/v1/feedback-records/"+recordA.ID.String(), nil) + require.Equal(t, http.StatusOK, getStatus, "body: %s", string(getBody)) + }) + + t.Run("unfiltered delete succeeds after purge releases", func(t *testing.T) { + release() + + status, body := doTenantLockRequest(ctx, t, client, http.MethodDelete, deleteByUserURL, nil) + + var deleted models.DeleteFeedbackRecordsByUserResponse + + require.Equal(t, http.StatusOK, status, "body: %s", string(body)) + require.NoError(t, json.Unmarshal(body, &deleted)) + assert.Equal(t, int64(1), deleted.DeletedCount) + }) + + cleanupTenantDataBestEffort(ctx, client, server.URL, tenantA) +} + +// TestWebhookTenantMoveLocksTargetTenant covers tenant-changing webhook +// updates: moving a webhook into a tenant under purge must conflict even +// though the webhook's current tenant is unlocked. +func TestWebhookTenantMoveLocksTargetTenant(t *testing.T) { + server, cleanup := setupTestServer(t) + defer cleanup() + + ctx := context.Background() + db := newTenantLockDB(ctx, t) + client := &http.Client{} + tenantA := "tenant-lock-move-a-" + uuid.NewString() + tenantB := "tenant-lock-move-b-" + uuid.NewString() + + webhook := createTenantDataWebhook(ctx, t, client, server.URL, tenantA, "tenant-lock-move") + webhookURL := server.URL + "/v1/webhooks/" + webhook.ID.String() + + release := holdTenantWriteLock(ctx, t, db, tenantB, false) + defer release() + + status, body := doTenantLockRequest(ctx, t, client, http.MethodPatch, webhookURL, map[string]any{"tenant_id": tenantB}) + requireTenantWriteConflict(t, status, body) + + release() + + status, body = doTenantLockRequest(ctx, t, client, http.MethodPatch, webhookURL, map[string]any{"tenant_id": tenantB}) + require.Equal(t, http.StatusOK, status, "body: %s", string(body)) + + cleanupTenantDataBestEffort(ctx, client, server.URL, tenantB) +} + +// TestRepositoryWritesConflictDuringPurge covers the non-HTTP write surfaces +// (worker-driven webhook disable, embedding writes, taxonomy writes) directly +// at the repository layer, which every caller shares. +func TestRepositoryWritesConflictDuringPurge(t *testing.T) { + server, cleanup := setupTestServer(t) + defer cleanup() + + ctx := context.Background() + db := newTenantLockDB(ctx, t) + client := &http.Client{} + tenantA := "tenant-lock-repo-" + uuid.NewString() + + webhooksRepo := repository.NewWebhooksRepository(db) + embeddingsRepo := repository.NewEmbeddingsRepository(db) + taxonomyRepo := repository.NewTaxonomyRepository(db) + + record := createTenantDataFeedbackRecord(ctx, t, client, server.URL, tenantA, uuid.NewString(), "tenant-lock-field") + webhook := createTenantDataWebhook(ctx, t, client, server.URL, tenantA, "tenant-lock-repo") + + pendingRun, created, err := taxonomyRepo.CreateRunIfAvailable(ctx, repository.CreateTaxonomyRunParams{ + TaxonomyScope: models.TaxonomyScope{ + TenantID: tenantA, + SourceType: "formbricks", + SourceID: "tenant-lock-source", + FieldID: record.FieldID, + }, + RecordCount: 1, + EmbeddingCount: 1, + }) + require.NoError(t, err) + require.True(t, created) + + release := holdTenantWriteLock(ctx, t, db, tenantA, false) + defer release() + + t.Run("webhook delivery disable conflicts", func(t *testing.T) { + enabled := false + reason := "max attempts" + now := time.Now() + + _, err := webhooksRepo.Update(ctx, webhook.ID, &models.UpdateWebhookRequest{ + Enabled: &enabled, + DisabledReason: &reason, + DisabledAt: &now, + }) + require.ErrorIs(t, err, huberrors.ErrTenantWriteConflict) + + current, err := webhooksRepo.GetByID(ctx, webhook.ID) + require.NoError(t, err) + assert.True(t, current.Enabled, "webhook must stay enabled after rejected disable") + }) + + t.Run("embedding upsert and delete conflict", func(t *testing.T) { + embedding := make([]float32, models.EmbeddingVectorDimensions) + embedding[0] = 0.5 + + err := embeddingsRepo.Upsert(ctx, record.ID, "model-name", embedding) + require.ErrorIs(t, err, huberrors.ErrTenantWriteConflict) + + err = embeddingsRepo.DeleteByFeedbackRecordAndModel(ctx, record.ID, "model-name") + require.ErrorIs(t, err, huberrors.ErrTenantWriteConflict) + }) + + t.Run("taxonomy run create and transitions conflict", func(t *testing.T) { + _, _, err := taxonomyRepo.CreateRunIfAvailable(ctx, repository.CreateTaxonomyRunParams{ + TaxonomyScope: models.TaxonomyScope{ + TenantID: tenantA, + SourceType: "formbricks", + SourceID: "tenant-lock-source-2", + FieldID: record.FieldID, + }, + RecordCount: 1, + EmbeddingCount: 1, + }) + require.ErrorIs(t, err, huberrors.ErrTenantWriteConflict) + + _, err = taxonomyRepo.MarkRunRunning(ctx, pendingRun.ID, tenantA) + require.ErrorIs(t, err, huberrors.ErrTenantWriteConflict) + }) + + t.Run("writes succeed after release", func(t *testing.T) { + release() + + embedding := make([]float32, models.EmbeddingVectorDimensions) + embedding[0] = 0.5 + require.NoError(t, embeddingsRepo.Upsert(ctx, record.ID, "model-name", embedding)) + + _, err := taxonomyRepo.MarkRunRunning(ctx, pendingRun.ID, tenantA) + require.NoError(t, err) + }) + + cleanupTenantDataBestEffort(ctx, client, server.URL, tenantA) +} From b923d1f12ab2b3e480864055c06195511097b23b Mon Sep 17 00:00:00 2001 From: Tiago Farto Date: Mon, 15 Jun 2026 10:42:40 +0000 Subject: [PATCH 2/4] chore: consolidate tenant write lock helpers and interfaces --- internal/repository/embeddings_repository.go | 22 ++--- .../repository/feedback_records_repository.go | 90 +++++++++++-------- internal/repository/taxonomy_repository.go | 12 +-- internal/repository/tenant_data_repository.go | 33 ++----- .../repository/tenant_data_repository_test.go | 55 +++--------- internal/repository/tenant_write_lock.go | 78 +++++++++++++--- internal/repository/webhooks_repository.go | 36 ++++---- 7 files changed, 170 insertions(+), 156 deletions(-) diff --git a/internal/repository/embeddings_repository.go b/internal/repository/embeddings_repository.go index 58f2c479..5524daf2 100644 --- a/internal/repository/embeddings_repository.go +++ b/internal/repository/embeddings_repository.go @@ -51,20 +51,15 @@ func (r *EmbeddingsRepository) Upsert( vec := pgvector.NewHalfVector(embedding) now := time.Now() - return withTenantWriteTx(ctx, tenantWritePool{db: r.db}, nil, func(dbTx tenantWriteTx) error { + return withTenantWritePoolTx(ctx, r.db, nil, func(dbTx tenantWriteTx) error { // Embeddings derive their tenant boundary from the parent feedback // record; resolve and lock it so embedding writes cannot race a tenant // data purge. A missing parent means the record was deleted or purged. - tenantID, err := resolveFeedbackRecordTenant(ctx, dbTx, feedbackRecordID) - if err != nil { - return err - } - - if err := tryLockTenantsShared(ctx, dbTx, []string{tenantID}); err != nil { + if _, err := lockFeedbackRecordTenantShared(ctx, dbTx, feedbackRecordID); err != nil { return err } - _, err = dbTx.Exec(ctx, ` + _, err := dbTx.Exec(ctx, ` INSERT INTO embeddings (feedback_record_id, embedding, model, created_at, updated_at) VALUES ($1, $2, $3, $4, $5) ON CONFLICT (feedback_record_id, model) @@ -83,17 +78,12 @@ func (r *EmbeddingsRepository) Upsert( func (r *EmbeddingsRepository) DeleteByFeedbackRecordAndModel( ctx context.Context, feedbackRecordID uuid.UUID, model string, ) error { - return withTenantWriteTx(ctx, tenantWritePool{db: r.db}, nil, func(dbTx tenantWriteTx) error { - tenantID, err := resolveFeedbackRecordTenant(ctx, dbTx, feedbackRecordID) - if err != nil { - return err - } - - if err := tryLockTenantsShared(ctx, dbTx, []string{tenantID}); err != nil { + return withTenantWritePoolTx(ctx, r.db, nil, func(dbTx tenantWriteTx) error { + if _, err := lockFeedbackRecordTenantShared(ctx, dbTx, feedbackRecordID); err != nil { return err } - _, err = dbTx.Exec(ctx, + _, err := dbTx.Exec(ctx, `DELETE FROM embeddings WHERE feedback_record_id = $1 AND model = $2`, feedbackRecordID, model, ) diff --git a/internal/repository/feedback_records_repository.go b/internal/repository/feedback_records_repository.go index 26c098d5..e440c37c 100644 --- a/internal/repository/feedback_records_repository.go +++ b/internal/repository/feedback_records_repository.go @@ -38,6 +38,12 @@ func (r *FeedbackRecordsRepository) Create(ctx context.Context, req *models.Crea collectedAt = *req.CollectedAt } + // The tenant is known up front, so gate the insert on the shared tenant + // write lock in a single statement (held for this statement's implicit + // transaction): one round trip, same isolation against a tenant data purge. + // Zero rows means the lock was refused (purge in progress). + const lockKeyParam = 19 // $19, after the 18 inserted columns + query := ` INSERT INTO feedback_records ( collected_at, source_type, source_id, source_name, @@ -45,7 +51,8 @@ func (r *FeedbackRecordsRepository) Create(ctx context.Context, req *models.Crea value_text, value_number, value_boolean, value_date, metadata, language, user_id, tenant_id, submission_id ) - VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18) + SELECT $1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18 + WHERE ` + tenantWriteLockGate(lockKeyParam) + ` RETURNING id, collected_at, created_at, updated_at, source_type, source_id, source_name, field_id, field_label, field_type, field_group_id, field_group_label, @@ -55,32 +62,30 @@ func (r *FeedbackRecordsRepository) Create(ctx context.Context, req *models.Crea var record models.FeedbackRecord - err := withTenantWriteTx(ctx, tenantWritePool{db: r.db}, []string{req.TenantID}, func(dbTx tenantWriteTx) error { - err := dbTx.QueryRow(ctx, query, - collectedAt, req.SourceType, req.SourceID, req.SourceName, - req.FieldID, req.FieldLabel, req.FieldType, req.FieldGroupID, req.FieldGroupLabel, - req.ValueText, req.ValueNumber, req.ValueBoolean, req.ValueDate, - req.Metadata, req.Language, req.UserID, req.TenantID, req.SubmissionID, - ).Scan( - &record.ID, &record.CollectedAt, &record.CreatedAt, &record.UpdatedAt, - &record.SourceType, &record.SourceID, &record.SourceName, - &record.FieldID, &record.FieldLabel, &record.FieldType, &record.FieldGroupID, &record.FieldGroupLabel, - &record.ValueText, &record.ValueNumber, &record.ValueBoolean, &record.ValueDate, - &record.Metadata, &record.Language, &record.UserID, &record.TenantID, &record.SubmissionID, - ) - if err != nil { - var pgErr *pgconn.PgError - if errors.As(err, &pgErr) && pgErr.Code == uniqueViolationSQLState { - return huberrors.NewConflictError("a feedback record with this tenant_id, submission_id, and field_id already exists") - } + err := r.db.QueryRow(ctx, query, + collectedAt, req.SourceType, req.SourceID, req.SourceName, + req.FieldID, req.FieldLabel, req.FieldType, req.FieldGroupID, req.FieldGroupLabel, + req.ValueText, req.ValueNumber, req.ValueBoolean, req.ValueDate, + req.Metadata, req.Language, req.UserID, req.TenantID, req.SubmissionID, + TenantWriteLockKey(req.TenantID), + ).Scan( + &record.ID, &record.CollectedAt, &record.CreatedAt, &record.UpdatedAt, + &record.SourceType, &record.SourceID, &record.SourceName, + &record.FieldID, &record.FieldLabel, &record.FieldType, &record.FieldGroupID, &record.FieldGroupLabel, + &record.ValueText, &record.ValueNumber, &record.ValueBoolean, &record.ValueDate, + &record.Metadata, &record.Language, &record.UserID, &record.TenantID, &record.SubmissionID, + ) + if err != nil { + var pgErr *pgconn.PgError + if errors.As(err, &pgErr) && pgErr.Code == uniqueViolationSQLState { + return nil, huberrors.NewConflictError("a feedback record with this tenant_id, submission_id, and field_id already exists") + } - return fmt.Errorf("failed to create feedback record: %w", err) + if errors.Is(err, pgx.ErrNoRows) { + return nil, huberrors.NewTenantWriteConflictError("tenant data purge in progress for this tenant; retry later") } - return nil - }) - if err != nil { - return nil, err + return nil, fmt.Errorf("failed to create feedback record: %w", err) } return &record, nil @@ -105,6 +110,25 @@ func resolveFeedbackRecordTenant(ctx context.Context, querier queryer, id uuid.U return tenantID, nil } +// lockFeedbackRecordTenantShared resolves a feedback record's tenant boundary +// and acquires the shared tenant write lock for it, in that order, inside the +// current transaction. It is the single entry point for mutating a feedback +// record (or its derived rows, e.g. embeddings) by ID without racing a tenant +// data purge. Returns NotFound if the record is gone and a tenant write +// conflict if the tenant is under purge. +func lockFeedbackRecordTenantShared(ctx context.Context, dbTx tenantWriteTx, id uuid.UUID) (string, error) { + tenantID, err := resolveFeedbackRecordTenant(ctx, dbTx, id) + if err != nil { + return "", err + } + + if err := tryLockTenantsShared(ctx, dbTx, []string{tenantID}); err != nil { + return "", err + } + + return tenantID, nil +} + // GetByID retrieves a single feedback record by ID. Embedding is not selected (API/worker reads stay lean). func (r *FeedbackRecordsRepository) GetByID(ctx context.Context, id uuid.UUID) (*models.FeedbackRecord, error) { query := ` @@ -393,16 +417,12 @@ func (r *FeedbackRecordsRepository) Update( var record models.FeedbackRecord - err := withTenantWriteTx(ctx, tenantWritePool{db: r.db}, nil, func(dbTx tenantWriteTx) error { - tenantID, err := resolveFeedbackRecordTenant(ctx, dbTx, id) + err := withTenantWritePoolTx(ctx, r.db, nil, func(dbTx tenantWriteTx) error { + tenantID, err := lockFeedbackRecordTenantShared(ctx, dbTx, id) if err != nil { return err } - if err := tryLockTenantsShared(ctx, dbTx, []string{tenantID}); err != nil { - return err - } - err = dbTx.QueryRow(ctx, query, append(args, tenantID)...).Scan( &record.ID, &record.CollectedAt, &record.CreatedAt, &record.UpdatedAt, &record.SourceType, &record.SourceID, &record.SourceName, @@ -429,16 +449,12 @@ func (r *FeedbackRecordsRepository) Update( // Delete removes a feedback record. func (r *FeedbackRecordsRepository) Delete(ctx context.Context, id uuid.UUID) error { - return withTenantWriteTx(ctx, tenantWritePool{db: r.db}, nil, func(dbTx tenantWriteTx) error { - tenantID, err := resolveFeedbackRecordTenant(ctx, dbTx, id) + return withTenantWritePoolTx(ctx, r.db, nil, func(dbTx tenantWriteTx) error { + tenantID, err := lockFeedbackRecordTenantShared(ctx, dbTx, id) if err != nil { return err } - if err := tryLockTenantsShared(ctx, dbTx, []string{tenantID}); err != nil { - return err - } - result, err := dbTx.Exec(ctx, `DELETE FROM feedback_records WHERE id = $1 AND tenant_id = $2`, id, tenantID) if err != nil { return fmt.Errorf("failed to delete feedback record: %w", err) @@ -465,7 +481,7 @@ func (r *FeedbackRecordsRepository) DeleteByUser( ) ([]models.DeletedFeedbackRecordsByTenant, error) { groups := make([]models.DeletedFeedbackRecordsByTenant, 0) - err := withTenantWriteTx(ctx, tenantWritePool{db: r.db}, nil, func(dbTx tenantWriteTx) error { + err := withTenantWritePoolTx(ctx, r.db, nil, func(dbTx tenantWriteTx) error { tenantIDs, err := listUserFeedbackTenants(ctx, dbTx, filters) if err != nil { return err diff --git a/internal/repository/taxonomy_repository.go b/internal/repository/taxonomy_repository.go index a3a56190..ab39c058 100644 --- a/internal/repository/taxonomy_repository.go +++ b/internal/repository/taxonomy_repository.go @@ -150,7 +150,7 @@ func (r *TaxonomyRepository) CreateRunIfAvailable( created bool ) - err := withTenantWriteTx(ctx, tenantWritePool{db: r.db}, []string{params.TenantID}, func(dbTx tenantWriteTx) error { + err := withTenantWritePoolTx(ctx, r.db, []string{params.TenantID}, func(dbTx tenantWriteTx) error { // Scope lock comes second, after the shared tenant write lock, per the // tenant write lock-order convention (see tenant_write_lock.go). Scope // waiters already hold the tenant lock in shared mode, so they never @@ -233,7 +233,7 @@ func (r *TaxonomyRepository) MarkRunRunning( ) (*models.TaxonomyRun, error) { var run *models.TaxonomyRun - err := withTenantWriteTx(ctx, tenantWritePool{db: r.db}, []string{tenantID}, func(dbTx tenantWriteTx) error { + err := withTenantWritePoolTx(ctx, r.db, []string{tenantID}, func(dbTx tenantWriteTx) error { updated, err := queryTaxonomyRun(ctx, dbTx, ` WITH taxonomy_runs AS ( UPDATE taxonomy_runs @@ -272,7 +272,7 @@ func (r *TaxonomyRepository) MarkRunFailed( ) (*models.TaxonomyRun, error) { var run *models.TaxonomyRun - err := withTenantWriteTx(ctx, tenantWritePool{db: r.db}, []string{tenantID}, func(dbTx tenantWriteTx) error { + err := withTenantWritePoolTx(ctx, r.db, []string{tenantID}, func(dbTx tenantWriteTx) error { updated, err := queryTaxonomyRun(ctx, dbTx, ` WITH taxonomy_runs AS ( UPDATE taxonomy_runs @@ -462,7 +462,7 @@ func (r *TaxonomyRepository) StoreResultAndActivate( ) (*models.TaxonomyRun, error) { var updated *models.TaxonomyRun - err := withTenantWriteTx(ctx, tenantWritePool{db: r.db}, []string{tenantID}, func(dbTx tenantWriteTx) error { + err := withTenantWritePoolTx(ctx, r.db, []string{tenantID}, func(dbTx tenantWriteTx) error { var err error updated, err = storeResultAndActivateInTx(ctx, dbTx, runID, tenantID, req) @@ -683,7 +683,7 @@ func (r *TaxonomyRepository) RenameNode( ) (*models.TaxonomyNode, error) { var updated *models.TaxonomyNode - err := withTenantWriteTx(ctx, tenantWritePool{db: r.db}, []string{tenantID}, func(dbTx tenantWriteTx) error { + err := withTenantWritePoolTx(ctx, r.db, []string{tenantID}, func(dbTx tenantWriteTx) error { node, run, err := getNodeForUpdate(ctx, dbTx, nodeID, tenantID) if err != nil { return err @@ -722,7 +722,7 @@ func (r *TaxonomyRepository) RemoveNode( ) (*models.TaxonomyNode, error) { var updated *models.TaxonomyNode - err := withTenantWriteTx(ctx, tenantWritePool{db: r.db}, []string{tenantID}, func(dbTx tenantWriteTx) error { + err := withTenantWritePoolTx(ctx, r.db, []string{tenantID}, func(dbTx tenantWriteTx) error { _, run, err := getNodeForUpdate(ctx, dbTx, nodeID, tenantID) if err != nil { return err diff --git a/internal/repository/tenant_data_repository.go b/internal/repository/tenant_data_repository.go index 3e6ef71b..ffa42ed3 100644 --- a/internal/repository/tenant_data_repository.go +++ b/internal/repository/tenant_data_repository.go @@ -14,36 +14,19 @@ import ( "github.com/formbricks/hub/internal/models" ) +// tenantDataExecutor is the Exec-only statement surface the purge runs on +// (DELETE statements and the advisory-lock SQL). The tenant write transaction +// (tenantWriteTx) satisfies it. type tenantDataExecutor interface { Exec(ctx context.Context, sql string, arguments ...any) (pgconn.CommandTag, error) } -type tenantDataTx interface { - tenantDataExecutor - Commit(ctx context.Context) error - Rollback(ctx context.Context) error -} - -type tenantDataTxBeginner interface { - BeginTx(ctx context.Context, txOptions pgx.TxOptions) (tenantDataTx, error) -} - -type tenantDataPool struct { - db *pgxpool.Pool -} - -func (p tenantDataPool) BeginTx(ctx context.Context, txOptions pgx.TxOptions) (tenantDataTx, error) { - dbTx, err := p.db.BeginTx(ctx, txOptions) - if err != nil { - return nil, fmt.Errorf("begin tenant data transaction: %w", err) - } - - return dbTx, nil -} - // TenantDataRepository handles tenant-scoped data purge operations. type TenantDataRepository struct { - db tenantDataTxBeginner + // db opens the purge transaction. It shares tenantWriteTxBeginner with every + // tenant-owned write path so the purge's exclusive lock and writers' shared + // locks coordinate on the same transaction machinery. + db tenantWriteTxBeginner // purgeLockTimeout bounds how long a purge waits for in-flight tenant-owned // writes (shared tenant write lock holders) to drain before returning a // retryable conflict. @@ -52,7 +35,7 @@ type TenantDataRepository struct { // NewTenantDataRepository creates a new tenant data repository. func NewTenantDataRepository(db *pgxpool.Pool, purgeLockTimeout time.Duration) *TenantDataRepository { - return &TenantDataRepository{db: tenantDataPool{db: db}, purgeLockTimeout: purgeLockTimeout} + return &TenantDataRepository{db: tenantWritePool{db: db}, purgeLockTimeout: purgeLockTimeout} } // DeleteByTenant deletes all Hub-owned data for a tenant and returns per-resource counts. diff --git a/internal/repository/tenant_data_repository_test.go b/internal/repository/tenant_data_repository_test.go index 3e954f1b..c6502524 100644 --- a/internal/repository/tenant_data_repository_test.go +++ b/internal/repository/tenant_data_repository_test.go @@ -13,19 +13,9 @@ import ( "github.com/formbricks/hub/internal/huberrors" ) -type fakeTenantDataDB struct { - tx *fakeTenantDataTx - beginErr error -} - -func (f *fakeTenantDataDB) BeginTx(context.Context, pgx.TxOptions) (tenantDataTx, error) { - if f.beginErr != nil { - return nil, f.beginErr - } - - return f.tx, nil -} - +// fakeTenantDataExecutor is the shared Exec recorder embedded by +// fakeTenantWriteTx (see tenant_write_lock_test.go); the purge tests below +// drive that same fake transaction, which satisfies tenantWriteTxBeginner. type fakeTenantDataExecutor struct { tags []pgconn.CommandTag errAtQuery int @@ -56,27 +46,6 @@ func (f *fakeTenantDataExecutor) Exec( return f.tags[tagIndex], nil } -type fakeTenantDataTx struct { - fakeTenantDataExecutor - - commitErr error - rollbackErr error - committed bool - rolledBack bool -} - -func (f *fakeTenantDataTx) Commit(context.Context) error { - f.committed = true - - return f.commitErr -} - -func (f *fakeTenantDataTx) Rollback(context.Context) error { - f.rolledBack = true - - return f.rollbackErr -} - // purgeLockTags are the command tags for the three lock-related statements // (set_config, advisory lock, set_config reset) that precede the deletes. func purgeLockTags() []pgconn.CommandTag { @@ -89,7 +58,7 @@ func purgeLockTags() []pgconn.CommandTag { func TestTenantDataRepository_DeleteByTenant(t *testing.T) { t.Run("locks tenant exclusively, commits transaction, and returns counts", func(t *testing.T) { - transaction := &fakeTenantDataTx{ + transaction := &fakeTenantWriteTx{ fakeTenantDataExecutor: fakeTenantDataExecutor{ tags: append(purgeLockTags(), pgconn.NewCommandTag("DELETE 2"), @@ -100,7 +69,7 @@ func TestTenantDataRepository_DeleteByTenant(t *testing.T) { }, rollbackErr: pgx.ErrTxClosed, } - repo := &TenantDataRepository{db: &fakeTenantDataDB{tx: transaction}, purgeLockTimeout: 5 * time.Second} + repo := &TenantDataRepository{db: &fakeTenantWriteDB{tx: transaction}, purgeLockTimeout: 5 * time.Second} counts, err := repo.DeleteByTenant(context.Background(), "org-123") if err != nil { @@ -134,13 +103,13 @@ func TestTenantDataRepository_DeleteByTenant(t *testing.T) { }) t.Run("lock timeout returns tenant write conflict without deletes", func(t *testing.T) { - transaction := &fakeTenantDataTx{ + transaction := &fakeTenantWriteTx{ fakeTenantDataExecutor: fakeTenantDataExecutor{ errAtQuery: 2, err: &pgconn.PgError{Code: lockNotAvailableSQLState}, }, } - repo := &TenantDataRepository{db: &fakeTenantDataDB{tx: transaction}, purgeLockTimeout: time.Second} + repo := &TenantDataRepository{db: &fakeTenantWriteDB{tx: transaction}, purgeLockTimeout: time.Second} counts, err := repo.DeleteByTenant(context.Background(), "org-123") if !errors.Is(err, huberrors.ErrTenantWriteConflict) { @@ -168,14 +137,14 @@ func TestTenantDataRepository_DeleteByTenant(t *testing.T) { t.Run("rolls back and returns delete error", func(t *testing.T) { rollbackErr := errors.New("rollback failed") - transaction := &fakeTenantDataTx{ + transaction := &fakeTenantWriteTx{ fakeTenantDataExecutor: fakeTenantDataExecutor{ tags: purgeLockTags(), errAtQuery: 5, }, rollbackErr: rollbackErr, } - repo := &TenantDataRepository{db: &fakeTenantDataDB{tx: transaction}} + repo := &TenantDataRepository{db: &fakeTenantWriteDB{tx: transaction}} counts, err := repo.DeleteByTenant(context.Background(), "org-123") if err == nil { @@ -197,7 +166,7 @@ func TestTenantDataRepository_DeleteByTenant(t *testing.T) { t.Run("returns begin transaction error", func(t *testing.T) { beginErr := errors.New("begin failed") - repo := &TenantDataRepository{db: &fakeTenantDataDB{beginErr: beginErr}} + repo := &TenantDataRepository{db: &fakeTenantWriteDB{beginErr: beginErr}} counts, err := repo.DeleteByTenant(context.Background(), "org-123") if !errors.Is(err, beginErr) { @@ -211,7 +180,7 @@ func TestTenantDataRepository_DeleteByTenant(t *testing.T) { t.Run("returns commit error", func(t *testing.T) { commitErr := errors.New("commit failed") - transaction := &fakeTenantDataTx{ + transaction := &fakeTenantWriteTx{ fakeTenantDataExecutor: fakeTenantDataExecutor{ tags: append(purgeLockTags(), pgconn.NewCommandTag("DELETE 2"), @@ -223,7 +192,7 @@ func TestTenantDataRepository_DeleteByTenant(t *testing.T) { commitErr: commitErr, rollbackErr: pgx.ErrTxClosed, } - repo := &TenantDataRepository{db: &fakeTenantDataDB{tx: transaction}} + repo := &TenantDataRepository{db: &fakeTenantWriteDB{tx: transaction}} counts, err := repo.DeleteByTenant(context.Background(), "org-123") if !errors.Is(err, commitErr) { diff --git a/internal/repository/tenant_write_lock.go b/internal/repository/tenant_write_lock.go index a4436d01..1c1eb1f8 100644 --- a/internal/repository/tenant_write_lock.go +++ b/internal/repository/tenant_write_lock.go @@ -33,6 +33,14 @@ import ( // scope), then row locks. The purge takes only the tenant exclusive lock and // holds no row locks while waiting. No code path may block on an advisory // lock while holding row locks. +// +// Two write shapes: +// - Tenant known before the write (inserts): gate the write on the lock in a +// single autocommit statement via tenantWriteLockGate — one round trip, no +// explicit transaction. +// - Tenant resolved from the row being mutated (updates/deletes): use +// withTenantWriteTx / withTenantWritePoolTx, resolve the tenant inside the +// transaction, then tryLockTenantsShared before mutating. // tenantWriteLockSharedSQL acquires the tenant key in shared mode without waiting. const tenantWriteLockSharedSQL = `SELECT pg_try_advisory_xact_lock_shared(hashtextextended($1, 0))` @@ -45,6 +53,20 @@ const tenantWriteLockExclusiveSQL = `SELECT pg_advisory_xact_lock(hashtextextend // expires while waiting for a lock (55P03 lock_not_available). const lockNotAvailableSQLState = "55P03" +// tenantWriteLockGate returns a SQL boolean expression that try-acquires the +// shared tenant write lock, for gating a single-statement write +// (INSERT ... SELECT ... WHERE ) without an explicit transaction. Use it +// only when the tenant is known before the write (no in-transaction resolve): +// the advisory lock is transaction-scoped, and in autocommit the transaction is +// the single statement, so the lock is held for exactly the duration of the +// write — the same isolation as the explicit-transaction path, in one round +// trip. The statement must pass TenantWriteLockKey(tenantID) as the $paramIndex +// parameter, and the caller must treat zero affected rows (pgx.ErrNoRows) as a +// tenant write conflict (the lock was refused: a purge is in progress). +func tenantWriteLockGate(paramIndex int) string { + return fmt.Sprintf("pg_try_advisory_xact_lock_shared(hashtextextended($%d, 0))", paramIndex) +} + // TenantWriteLockKey returns the advisory lock key string that serializes // tenant-owned writes against tenant data purges for the given tenant. // Format: "tenant_write|:" (length-prefixed to keep distinct @@ -72,7 +94,9 @@ type tenantWriteTxBeginner interface { BeginTx(ctx context.Context, txOptions pgx.TxOptions) (tenantWriteTx, error) } -// tenantWritePool adapts *pgxpool.Pool to tenantWriteTxBeginner. +// tenantWritePool adapts *pgxpool.Pool to tenantWriteTxBeginner. It is the +// transaction source for every tenant-owned write path, including the tenant +// data purge. type tenantWritePool struct { db *pgxpool.Pool } @@ -86,6 +110,14 @@ func (p tenantWritePool) BeginTx(ctx context.Context, txOptions pgx.TxOptions) ( return dbTx, nil } +// withTenantWritePoolTx is withTenantWriteTx for the common case of a +// *pgxpool.Pool, so call sites do not repeat the tenantWritePool adapter. +func withTenantWritePoolTx( + ctx context.Context, pool *pgxpool.Pool, tenantIDs []string, mutate func(dbTx tenantWriteTx) error, +) error { + return withTenantWriteTx(ctx, tenantWritePool{db: pool}, tenantIDs, mutate) +} + // withTenantWriteTx runs mutate inside a transaction that holds shared tenant // write locks for the given tenant IDs (sorted and deduplicated). Pass no // tenant IDs when the tenant boundary can only be resolved inside the @@ -122,27 +154,44 @@ func withTenantWriteTx( } // tryLockTenantsShared acquires the shared tenant write lock for every given -// tenant ID (sorted, deduplicated) without waiting. It returns -// *huberrors.TenantWriteConflictError when any lock is unavailable, which -// means a tenant data purge for that tenant is running or waiting to run. +// tenant ID without waiting. It returns *huberrors.TenantWriteConflictError +// when any lock is unavailable, which means a tenant data purge for that tenant +// is running or waiting to run. func tryLockTenantsShared(ctx context.Context, querier queryer, tenantIDs []string) error { - if len(tenantIDs) == 0 { + // Fast path for the common single-tenant write: skip the clone/sort/dedup + // allocation entirely. + switch len(tenantIDs) { + case 0: return nil + case 1: + return tryLockTenantShared(ctx, querier, tenantIDs[0]) } + // Multiple tenants (GDPR erasure across tenants, webhook tenant-move): + // deduplicate so a repeated tenant_id does not cost a redundant lock round + // trip. Try-locks never wait, so the order is not needed for deadlock + // avoidance; sorting just keeps behavior deterministic. sorted := slices.Clone(tenantIDs) slices.Sort(sorted) sorted = slices.Compact(sorted) for _, tenantID := range sorted { - var locked bool - if err := querier.QueryRow(ctx, tenantWriteLockSharedSQL, TenantWriteLockKey(tenantID)).Scan(&locked); err != nil { - return fmt.Errorf("acquire shared tenant write lock: %w", err) + if err := tryLockTenantShared(ctx, querier, tenantID); err != nil { + return err } + } - if !locked { - return huberrors.NewTenantWriteConflictError("tenant data purge in progress for this tenant; retry later") - } + return nil +} + +func tryLockTenantShared(ctx context.Context, querier queryer, tenantID string) error { + var locked bool + if err := querier.QueryRow(ctx, tenantWriteLockSharedSQL, TenantWriteLockKey(tenantID)).Scan(&locked); err != nil { + return fmt.Errorf("acquire shared tenant write lock: %w", err) + } + + if !locked { + return huberrors.NewTenantWriteConflictError("tenant data purge in progress for this tenant; retry later") } return nil @@ -156,8 +205,11 @@ func tryLockTenantsShared(ctx context.Context, querier queryer, tenantIDs []stri func acquireTenantPurgeLock( ctx context.Context, exec tenantDataExecutor, tenantID string, timeout time.Duration, ) error { - timeoutMs := strconv.FormatInt(timeout.Milliseconds(), 10) - if _, err := exec.Exec(ctx, `SELECT set_config('lock_timeout', $1, true)`, timeoutMs); err != nil { + // Postgres treats lock_timeout = 0 as "wait forever"; floor a zero or + // sub-millisecond duration to 1ms so the purge always fails fast rather + // than blocking indefinitely on in-flight writers. + timeoutMs := max(timeout.Milliseconds(), 1) + if _, err := exec.Exec(ctx, `SELECT set_config('lock_timeout', $1, true)`, strconv.FormatInt(timeoutMs, 10)); err != nil { return fmt.Errorf("set tenant purge lock timeout: %w", err) } diff --git a/internal/repository/webhooks_repository.go b/internal/repository/webhooks_repository.go index 2291825c..ffbf31f6 100644 --- a/internal/repository/webhooks_repository.go +++ b/internal/repository/webhooks_repository.go @@ -49,11 +49,18 @@ func (r *WebhooksRepository) Create(ctx context.Context, req *models.CreateWebho } } + // The tenant is known up front, so gate the insert on the shared tenant + // write lock in a single statement (held for this statement's implicit + // transaction): one round trip, same isolation against a tenant data purge. + // Zero rows means the lock was refused (purge in progress). + const lockKeyParam = 6 // $6, after the 5 inserted columns + query := ` INSERT INTO webhooks ( url, signing_key, enabled, tenant_id, event_types ) - VALUES ($1, $2, $3, $4, $5) + SELECT $1, $2, $3, $4, $5 + WHERE ` + tenantWriteLockGate(lockKeyParam) + ` RETURNING id, url, signing_key, enabled, tenant_id, created_at, updated_at, event_types ` @@ -62,21 +69,18 @@ func (r *WebhooksRepository) Create(ctx context.Context, req *models.CreateWebho dbEventTypes []string ) - err := withTenantWriteTx(ctx, tenantWritePool{db: r.db}, []string{*req.TenantID}, func(dbTx tenantWriteTx) error { - err := dbTx.QueryRow(ctx, query, - req.URL, req.SigningKey, enabled, req.TenantID, eventTypes, - ).Scan( - &webhook.ID, &webhook.URL, &webhook.SigningKey, &webhook.Enabled, - &webhook.TenantID, &webhook.CreatedAt, &webhook.UpdatedAt, &dbEventTypes, - ) - if err != nil { - return fmt.Errorf("failed to create webhook: %w", err) + err := r.db.QueryRow(ctx, query, + req.URL, req.SigningKey, enabled, req.TenantID, eventTypes, TenantWriteLockKey(*req.TenantID), + ).Scan( + &webhook.ID, &webhook.URL, &webhook.SigningKey, &webhook.Enabled, + &webhook.TenantID, &webhook.CreatedAt, &webhook.UpdatedAt, &dbEventTypes, + ) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return nil, huberrors.NewTenantWriteConflictError("tenant data purge in progress for this tenant; retry later") } - return nil - }) - if err != nil { - return nil, err + return nil, fmt.Errorf("failed to create webhook: %w", err) } webhook.EventTypes, err = parseDBEventTypes(dbEventTypes) @@ -349,7 +353,7 @@ func (r *WebhooksRepository) Update(ctx context.Context, id uuid.UUID, req *mode dbEventTypes []string ) - err := withTenantWriteTx(ctx, tenantWritePool{db: r.db}, nil, func(dbTx tenantWriteTx) error { + err := withTenantWritePoolTx(ctx, r.db, nil, func(dbTx tenantWriteTx) error { currentTenantID, err := resolveWebhookTenant(ctx, dbTx, id) if err != nil { return err @@ -408,7 +412,7 @@ func (r *WebhooksRepository) Delete(ctx context.Context, id uuid.UUID) (*models. var webhook models.DeletedWebhook - err := withTenantWriteTx(ctx, tenantWritePool{db: r.db}, nil, func(dbTx tenantWriteTx) error { + err := withTenantWritePoolTx(ctx, r.db, nil, func(dbTx tenantWriteTx) error { currentTenantID, err := resolveWebhookTenant(ctx, dbTx, id) if err != nil { return err From e777b59d83f575751312e3de502316a8ff6eaa30 Mon Sep 17 00:00:00 2001 From: Tiago Farto Date: Mon, 15 Jun 2026 11:37:29 +0000 Subject: [PATCH 3/4] chore: address PR comments --- .../repository/feedback_records_repository.go | 46 +++++++++++- internal/repository/taxonomy_repository.go | 26 +++++-- internal/repository/tenant_data_repository.go | 6 +- internal/repository/tenant_write_lock.go | 5 +- internal/repository/tenant_write_lock_test.go | 16 +++++ internal/workers/webhook_dispatch.go | 18 +++-- internal/workers/webhook_dispatch_test.go | 72 +++++++++++++++++++ openapi.yaml | 12 ++-- tests/tenant_write_lock_test.go | 68 ++++++++++++++++-- 9 files changed, 243 insertions(+), 26 deletions(-) diff --git a/internal/repository/feedback_records_repository.go b/internal/repository/feedback_records_repository.go index e440c37c..aabf565b 100644 --- a/internal/repository/feedback_records_repository.go +++ b/internal/repository/feedback_records_repository.go @@ -473,8 +473,12 @@ func (r *FeedbackRecordsRepository) Delete(ctx context.Context, id uuid.UUID) er // are deleted across tenants (documented GDPR/right-to-erasure exception). // Every spanned tenant's write lock is acquired before deleting; if any tenant is under purge // the whole request fails with a retryable conflict. The delete is scoped to the locked tenants, -// so records appearing in new tenants mid-transaction are never touched without a lock (a retry -// catches them). +// so records appearing in new tenants mid-transaction are never touched without a lock. +// Because the tenant set is snapshotted before locking, a record could be written into a new +// (unlocked) tenant for the same user after the snapshot; after deleting, the same transaction +// re-checks for any in-scope record still present and, if found, returns a retryable conflict +// (rolling the whole delete back) rather than reporting an incomplete erasure as success. Erasure +// is idempotent, so the caller's retry converges once writes for the subject have stopped. // It returns deleted IDs grouped by tenant so callers can publish tenant-scoped side effects. func (r *FeedbackRecordsRepository) DeleteByUser( ctx context.Context, filters *models.DeleteFeedbackRecordsByUserFilters, @@ -531,6 +535,16 @@ func (r *FeedbackRecordsRepository) DeleteByUser( return fmt.Errorf("error iterating delete feedback records by user result: %w", err) } + // Drift guard: a record for this user may have been written into a tenant + // not in the locked snapshot (a new tenant for the all-tenant erase, or a + // concurrent insert into an already-locked tenant). Re-check the in-scope + // set; if anything survives, fail with a retryable conflict so the whole + // erase rolls back and the caller retries, rather than reporting a + // partial erasure as complete. + if err := ensureNoResidualUserFeedback(ctx, dbTx, filters); err != nil { + return err + } + return nil }) if err != nil { @@ -540,6 +554,34 @@ func (r *FeedbackRecordsRepository) DeleteByUser( return groups, nil } +// ensureNoResidualUserFeedback returns a retryable tenant write conflict if any +// in-scope feedback record for the user still exists after DeleteByUser's delete. +func ensureNoResidualUserFeedback( + ctx context.Context, dbTx tenantWriteTx, filters *models.DeleteFeedbackRecordsByUserFilters, +) error { + query := `SELECT EXISTS (SELECT 1 FROM feedback_records WHERE user_id = $1` + args := []any{filters.UserID} + + if filters.TenantID != nil { + query += ` AND tenant_id = $2` + + args = append(args, *filters.TenantID) + } + + query += `)` + + var remaining bool + if err := dbTx.QueryRow(ctx, query, args...).Scan(&remaining); err != nil { + return fmt.Errorf("check residual feedback records for user: %w", err) + } + + if remaining { + return huberrors.NewTenantWriteConflictError("feedback records for this user changed during deletion; retry") + } + + return nil +} + // listUserFeedbackTenants returns the distinct tenant boundaries holding feedback // records for the user (optionally restricted to one tenant), so DeleteByUser can // lock each one before deleting. diff --git a/internal/repository/taxonomy_repository.go b/internal/repository/taxonomy_repository.go index ab39c058..7367b78c 100644 --- a/internal/repository/taxonomy_repository.go +++ b/internal/repository/taxonomy_repository.go @@ -245,7 +245,7 @@ func (r *TaxonomyRepository) MarkRunRunning( ) if err != nil { if errors.Is(err, pgx.ErrNoRows) { - return r.transitionError(ctx, runID, tenantID, models.TaxonomyRunStatusRunning) + return r.transitionError(ctx, dbTx, runID, tenantID, models.TaxonomyRunStatusRunning) } return fmt.Errorf("mark taxonomy run running: %w", err) @@ -284,7 +284,7 @@ func (r *TaxonomyRepository) MarkRunFailed( ) if err != nil { if errors.Is(err, pgx.ErrNoRows) { - return r.transitionError(ctx, runID, tenantID, models.TaxonomyRunStatusFailed) + return r.transitionError(ctx, dbTx, runID, tenantID, models.TaxonomyRunStatusFailed) } return fmt.Errorf("mark taxonomy run failed: %w", err) @@ -841,15 +841,28 @@ func (r *TaxonomyRepository) listVisibleNodes(ctx context.Context, runID uuid.UU return nodes, nil } +// transitionError builds the conflict (or not-found) error for a run that could +// not be transitioned. It reads through the caller's query handle (q) — which is +// the open transaction for the in-tx callers — so it never checks out a second +// pooled connection while a transaction connection is held. func (r *TaxonomyRepository) transitionError( ctx context.Context, + q queryer, runID uuid.UUID, tenantID string, target models.TaxonomyRunStatus, ) error { - run, err := r.GetRunForTenant(ctx, runID, tenantID) + run, err := queryTaxonomyRun(ctx, q, taxonomyRunSelect+` + FROM taxonomy_runs + WHERE id = $1 AND tenant_id = $2`, + runID, tenantID, + ) if err != nil { - return err + if errors.Is(err, pgx.ErrNoRows) { + return huberrors.NewNotFoundError("taxonomy_run", "taxonomy run not found") + } + + return fmt.Errorf("get taxonomy run: %w", err) } return taxonomyTransitionConflict(run, target) @@ -982,9 +995,12 @@ func scanTaxonomyNode(row scanner) (*models.TaxonomyNode, error) { return &node, nil } +// getNodeForUpdate takes a tenantWriteTx (not the narrower queryer) so the +// compiler enforces that the SELECT ... FOR UPDATE row lock is held for the +// life of a transaction; outside one, the lock would release at statement end. func getNodeForUpdate( ctx context.Context, - transaction queryer, + transaction tenantWriteTx, nodeID uuid.UUID, tenantID string, ) (*models.TaxonomyNode, *models.TaxonomyRun, error) { diff --git a/internal/repository/tenant_data_repository.go b/internal/repository/tenant_data_repository.go index ffa42ed3..239f73f6 100644 --- a/internal/repository/tenant_data_repository.go +++ b/internal/repository/tenant_data_repository.go @@ -46,7 +46,11 @@ func (r *TenantDataRepository) DeleteByTenant(ctx context.Context, tenantID stri } defer func() { - if err := dbTx.Rollback(ctx); err != nil && !errors.Is(err, pgx.ErrTxClosed) { + // A canceled request ctx (e.g. while waiting for the purge lock) can make + // pgx close the connection so Rollback can't send ROLLBACK; Postgres still + // aborts the tx and releases the advisory lock on session end. Skip logging + // when the ctx is already done — that rollback error is expected, not a fault. + if err := dbTx.Rollback(ctx); err != nil && !errors.Is(err, pgx.ErrTxClosed) && ctx.Err() == nil { slog.Error( "tenant data delete: rollback failed", "tenant_id_present", tenantID != "", diff --git a/internal/repository/tenant_write_lock.go b/internal/repository/tenant_write_lock.go index 1c1eb1f8..ca57374c 100644 --- a/internal/repository/tenant_write_lock.go +++ b/internal/repository/tenant_write_lock.go @@ -133,7 +133,10 @@ func withTenantWriteTx( } defer func() { - if err := dbTx.Rollback(ctx); err != nil && !errors.Is(err, pgx.ErrTxClosed) { + // A canceled ctx can make pgx close the connection before Rollback runs; + // Postgres aborts the tx and releases the advisory lock on session end + // regardless, so skip logging an expected rollback error when ctx is done. + if err := dbTx.Rollback(ctx); err != nil && !errors.Is(err, pgx.ErrTxClosed) && ctx.Err() == nil { slog.Error("tenant write tx: rollback failed", "error", err) } }() diff --git a/internal/repository/tenant_write_lock_test.go b/internal/repository/tenant_write_lock_test.go index 20a13708..94b12141 100644 --- a/internal/repository/tenant_write_lock_test.go +++ b/internal/repository/tenant_write_lock_test.go @@ -282,6 +282,22 @@ func TestAcquireTenantPurgeLock(t *testing.T) { } }) + t.Run("floors non-positive timeout to 1ms so lock_timeout is never disabled", func(t *testing.T) { + // lock_timeout = 0 means "wait forever" in Postgres; a zero or negative + // duration must never reach set_config as "0". + for _, timeout := range []time.Duration{0, -5 * time.Second, time.Microsecond} { + exec := &fakeTenantDataExecutor{} + + if err := acquireTenantPurgeLock(context.Background(), exec, "org-123", timeout); err != nil { + t.Fatalf("acquireTenantPurgeLock(%v) error = %v", timeout, err) + } + + if len(exec.args[0]) != 1 || exec.args[0][0] != "1" { + t.Fatalf("set_config args for timeout %v = %#v, want floored to \"1\"", timeout, exec.args[0]) + } + } + }) + t.Run("lock timeout maps to tenant write conflict", func(t *testing.T) { exec := &fakeTenantDataExecutor{ errAtQuery: 2, diff --git a/internal/workers/webhook_dispatch.go b/internal/workers/webhook_dispatch.go index 38e473f7..7499da1c 100644 --- a/internal/workers/webhook_dispatch.go +++ b/internal/workers/webhook_dispatch.go @@ -162,8 +162,8 @@ func (w *WebhookDispatchWorker) Work(ctx context.Context, job *river.Job[service // Send failed isLastAttempt := job.Attempt >= job.MaxAttempts if isLastAttempt { + // The delivery failed regardless of what happens to the webhook row. if w.metrics != nil { - w.metrics.RecordWebhookDisabled(ctx, "max_attempts") w.metrics.RecordDelivery(ctx, args.EventType, "failed_final") w.metrics.RecordWebhookDeliveryDuration(ctx, time.Since(start), args.EventType, "failed_final") } @@ -178,8 +178,18 @@ func (w *WebhookDispatchWorker) Work(ctx context.Context, job *river.Job[service DisabledAt: &now, }) + // Only signal "disabled" when the webhook was actually disabled. switch { case updateErr == nil: + if w.metrics != nil { + w.metrics.RecordWebhookDisabled(ctx, "max_attempts") + } + + slog.Error("webhook disabled after max delivery attempts", + "webhook_id", webhook.ID, + "event_id", args.EventID, + "error", err, + ) case errors.Is(updateErr, huberrors.ErrTenantWriteConflict): // A tenant data purge is deleting this webhook; disabling it is moot. slog.Warn("webhook dispatch: disable skipped, tenant purge in progress", @@ -194,12 +204,6 @@ func (w *WebhookDispatchWorker) Work(ctx context.Context, job *river.Job[service ) } - slog.Error("webhook disabled after max delivery attempts", - "webhook_id", webhook.ID, - "event_id", args.EventID, - "error", err, - ) - return fmt.Errorf("webhook send (final attempt): %w", err) } diff --git a/internal/workers/webhook_dispatch_test.go b/internal/workers/webhook_dispatch_test.go index 51da4a8e..ae1c6558 100644 --- a/internal/workers/webhook_dispatch_test.go +++ b/internal/workers/webhook_dispatch_test.go @@ -12,9 +12,35 @@ import ( "github.com/formbricks/hub/internal/huberrors" "github.com/formbricks/hub/internal/models" + "github.com/formbricks/hub/internal/observability" "github.com/formbricks/hub/internal/service" ) +// countingWebhookMetrics records how often each webhook metric fired, for +// asserting that "disabled" is signaled only when the disable write succeeds. +type countingWebhookMetrics struct { + disabled int + delivered map[string]int +} + +func newCountingWebhookMetrics() *countingWebhookMetrics { + return &countingWebhookMetrics{delivered: map[string]int{}} +} + +func (m *countingWebhookMetrics) RecordJobsEnqueued(context.Context, string, int64) {} +func (m *countingWebhookMetrics) RecordProviderError(context.Context, string) {} +func (m *countingWebhookMetrics) RecordDelivery(_ context.Context, _, status string) { + m.delivered[status]++ +} +func (m *countingWebhookMetrics) RecordWebhookDisabled(context.Context, string) { m.disabled++ } +func (m *countingWebhookMetrics) RecordDispatchError(context.Context, string) {} +func (m *countingWebhookMetrics) RecordWebhookDeliveryDuration( + context.Context, time.Duration, string, string, +) { +} + +var _ observability.WebhookMetrics = (*countingWebhookMetrics)(nil) + type mockDispatchRepo struct { webhook *models.Webhook err error @@ -361,6 +387,52 @@ func TestWebhookDispatchWorker_Work(t *testing.T) { t.Error("disable update should have been attempted") } }) + + t.Run("records disabled only when the disable write succeeds", func(t *testing.T) { + repo := &mockDispatchRepo{ + webhook: &models.Webhook{ID: webhookID, Enabled: true, URL: "http://x", SigningKey: "sk", TenantID: &tenantID}, + } + metrics := newCountingWebhookMetrics() + worker := NewWebhookDispatchWorker(repo, &mockSender{err: errors.New("final failure")}, 15*time.Second, metrics) + job := &river.Job[service.WebhookDispatchArgs]{ + JobRow: &rivertype.JobRow{Attempt: 3, MaxAttempts: 3}, + Args: args, + } + + _ = worker.Work(ctx, job) + + if metrics.disabled != 1 { + t.Errorf("RecordWebhookDisabled called %d times, want 1 (disable succeeded)", metrics.disabled) + } + + if metrics.delivered["failed_final"] != 1 { + t.Errorf("failed_final delivery recorded %d times, want 1", metrics.delivered["failed_final"]) + } + }) + + t.Run("does not record disabled when disable is skipped during purge", func(t *testing.T) { + repo := &mockDispatchRepo{ + webhook: &models.Webhook{ID: webhookID, Enabled: true, URL: "http://x", SigningKey: "sk", TenantID: &tenantID}, + updateErr: huberrors.NewTenantWriteConflictError(""), + } + metrics := newCountingWebhookMetrics() + worker := NewWebhookDispatchWorker(repo, &mockSender{err: errors.New("final failure")}, 15*time.Second, metrics) + job := &river.Job[service.WebhookDispatchArgs]{ + JobRow: &rivertype.JobRow{Attempt: 3, MaxAttempts: 3}, + Args: args, + } + + _ = worker.Work(ctx, job) + + if metrics.disabled != 0 { + t.Errorf("RecordWebhookDisabled called %d times, want 0 (disable skipped during purge)", metrics.disabled) + } + + // The delivery still failed and must still be recorded. + if metrics.delivered["failed_final"] != 1 { + t.Errorf("failed_final delivery recorded %d times, want 1", metrics.delivered["failed_final"]) + } + }) } func TestWebhookDispatchWorker_Timeout(t *testing.T) { diff --git a/openapi.yaml b/openapi.yaml index 5db57972..886ac088 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -1067,9 +1067,9 @@ paths: $ref: '#/components/schemas/ErrorModel' "409": description: | - Conflict – a tenant data purge is in progress for this webhook's current (or target) - tenant, or the webhook was modified concurrently (code `tenant_write_conflict`); - retry after the purge completes. + Conflict (code `tenant_write_conflict`) – the update could not be serialized against + a tenant data purge in progress for this webhook's current (or target) tenant, or the + webhook was modified concurrently. The conflict is transient; retry the request. content: application/problem+json: schema: @@ -1112,9 +1112,9 @@ paths: $ref: '#/components/schemas/ErrorModel' "409": description: | - Conflict – a tenant data purge is in progress for this webhook's tenant, or the - webhook was modified concurrently (code `tenant_write_conflict`); retry after the - purge completes. + Conflict (code `tenant_write_conflict`) – the delete could not be serialized against + a tenant data purge in progress for this webhook's tenant, or the webhook was modified + concurrently. The conflict is transient; retry the request. content: application/problem+json: schema: diff --git a/tests/tenant_write_lock_test.go b/tests/tenant_write_lock_test.go index 7b5be1f8..e7d9bbfa 100644 --- a/tests/tenant_write_lock_test.go +++ b/tests/tenant_write_lock_test.go @@ -362,16 +362,23 @@ func TestTenantWritesFailFastWhilePurgeQueued(t *testing.T) { purgeStatus <- resp.StatusCode }() - // Wait until the purge's exclusive advisory lock request is queued. + // Wait until the purge's exclusive advisory lock request is queued, scoped to + // tenantA's key so an unrelated waiter can't satisfy this. For the single-arg + // advisory form, Postgres stores the int8 key as classid = high 32 bits and + // objid = low 32 bits, with objsubid = 1. require.Eventually(t, func() bool { var waiting int - err := db.QueryRow(ctx, - `SELECT count(*) FROM pg_locks WHERE locktype = 'advisory' AND NOT granted`, + err := db.QueryRow(ctx, ` + SELECT count(*) FROM pg_locks + WHERE locktype = 'advisory' AND NOT granted AND objsubid = 1 + AND classid::bigint = ((hashtextextended($1, 0) >> 32) & 4294967295) + AND objid::bigint = (hashtextextended($1, 0) & 4294967295)`, + repository.TenantWriteLockKey(tenantA), ).Scan(&waiting) return err == nil && waiting > 0 - }, 5*time.Second, 20*time.Millisecond, "purge never queued for the advisory lock") + }, 5*time.Second, 20*time.Millisecond, "purge never queued for tenantA's advisory lock") // A new same-tenant write must fail fast while the purge is queued. status, body := doTenantLockRequest( @@ -587,5 +594,58 @@ func TestRepositoryWritesConflictDuringPurge(t *testing.T) { require.NoError(t, err) }) + t.Run("transition on a non-pending run reports conflict without a second connection", func(t *testing.T) { + // The run is now 'running'; marking it running again misses the + // status='pending' filter and routes through transitionError, which must + // read the run state through the open transaction (not the pool) to build + // the conflict — exercising the no-second-connection fix. + _, err := taxonomyRepo.MarkRunRunning(ctx, pendingRun.ID, tenantA) + require.ErrorIs(t, err, huberrors.ErrConflict) + }) + + cleanupTenantDataBestEffort(ctx, client, server.URL, tenantA) +} + +// TestTaxonomyNodeMutationsAreTenantScoped verifies getNodeForUpdate's tenant +// predicate: a node may only be renamed/removed by its owning tenant, so a +// caller can never lock or mutate another tenant's node row. +func TestTaxonomyNodeMutationsAreTenantScoped(t *testing.T) { + server, cleanup := setupTestServer(t) + defer cleanup() + + ctx := context.Background() + db := newTenantLockDB(ctx, t) + client := &http.Client{} + tenantA := "tenant-node-a-" + uuid.NewString() + tenantB := "tenant-node-b-" + uuid.NewString() + + taxonomyRepo := repository.NewTaxonomyRepository(db) + + record := createTenantDataFeedbackRecord(ctx, t, client, server.URL, tenantA, uuid.NewString(), "tenant-node-field") + runID := createTenantDataTaxonomyGraph( + ctx, t, db, tenantA, record.ID, "tenant-node-source-"+uuid.NewString(), record.FieldID, + ) + + var nodeID uuid.UUID + require.NoError(t, db.QueryRow(ctx, + `SELECT id FROM taxonomy_nodes WHERE run_id = $1 AND removed_at IS NULL ORDER BY level LIMIT 1`, runID, + ).Scan(&nodeID)) + + t.Run("rename from another tenant is not found", func(t *testing.T) { + _, err := taxonomyRepo.RenameNode(ctx, nodeID, tenantB, "actor", "renamed-by-b") + require.ErrorIs(t, err, huberrors.ErrNotFound) + }) + + t.Run("remove from another tenant is not found", func(t *testing.T) { + _, err := taxonomyRepo.RemoveNode(ctx, nodeID, tenantB, "actor") + require.ErrorIs(t, err, huberrors.ErrNotFound) + }) + + t.Run("rename from the owning tenant succeeds", func(t *testing.T) { + node, err := taxonomyRepo.RenameNode(ctx, nodeID, tenantA, "actor", "renamed-by-a") + require.NoError(t, err) + assert.Equal(t, "renamed-by-a", node.Label) + }) + cleanupTenantDataBestEffort(ctx, client, server.URL, tenantA) } From e15124e358c4fe0a011c4697da85873815197ea9 Mon Sep 17 00:00:00 2001 From: Tiago Farto Date: Tue, 16 Jun 2026 09:28:17 +0000 Subject: [PATCH 4/4] chore: address pr issues --- internal/service/feedback_records_service.go | 8 +++-- internal/service/webhooks_service.go | 8 ++++- openapi.yaml | 6 ++-- tests/tenant_write_lock_test.go | 38 ++++++++++++++++++++ 4 files changed, 55 insertions(+), 5 deletions(-) diff --git a/internal/service/feedback_records_service.go b/internal/service/feedback_records_service.go index 0a3a34c3..766ce98f 100644 --- a/internal/service/feedback_records_service.go +++ b/internal/service/feedback_records_service.go @@ -184,8 +184,12 @@ func (s *FeedbackRecordsService) UpdateFeedbackRecord( return nil, fmt.Errorf("update feedback record: %w", err) } - if s.publisher != nil { - s.publisher.PublishEventWithChangedFields(ctx, datatypes.FeedbackRecordUpdated, record, req.ChangedFields()) + // A no-op update (no fields set) writes nothing — the repository returns the + // current row without taking the tenant write lock — so it must not publish + // an "updated" event either. Otherwise an empty PATCH would fire tenant-owned + // side effects, including while the tenant is under a data purge. + if changed := req.ChangedFields(); s.publisher != nil && len(changed) > 0 { + s.publisher.PublishEventWithChangedFields(ctx, datatypes.FeedbackRecordUpdated, record, changed) } return record, nil diff --git a/internal/service/webhooks_service.go b/internal/service/webhooks_service.go index a2493412..74288de5 100644 --- a/internal/service/webhooks_service.go +++ b/internal/service/webhooks_service.go @@ -371,7 +371,13 @@ func (s *WebhooksService) UpdateWebhook(ctx context.Context, id uuid.UUID, req * return nil, fmt.Errorf("update webhook: %w", err) } - s.publisher.PublishEventWithChangedFields(ctx, datatypes.WebhookUpdated, *webhook, req.ChangedFields()) + // A no-op update (no fields set) writes nothing — the repository returns the + // current row without taking the tenant write lock — so it must not publish + // an "updated" event either. Otherwise an empty PATCH would fire tenant-owned + // side effects, including while the tenant is under a data purge. + if changed := req.ChangedFields(); len(changed) > 0 { + s.publisher.PublishEventWithChangedFields(ctx, datatypes.WebhookUpdated, *webhook, changed) + } return webhook, nil } diff --git a/openapi.yaml b/openapi.yaml index 886ac088..93168012 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -474,8 +474,10 @@ paths: $ref: '#/components/schemas/ErrorModel' "409": description: | - Conflict – a tenant data purge is in progress for a tenant holding records for this user - (code `tenant_write_conflict`). No records were deleted; retry after the purge completes. + Conflict (code `tenant_write_conflict`) – the erasure could not be serialized: either a + tenant data purge is in progress for a tenant holding records for this user, or the user's + records changed mid-deletion (drift guard). No records were deleted. The conflict is + transient; retry the request. content: application/problem+json: schema: diff --git a/tests/tenant_write_lock_test.go b/tests/tenant_write_lock_test.go index e7d9bbfa..e3a31eec 100644 --- a/tests/tenant_write_lock_test.go +++ b/tests/tenant_write_lock_test.go @@ -649,3 +649,41 @@ func TestTaxonomyNodeMutationsAreTenantScoped(t *testing.T) { cleanupTenantDataBestEffort(ctx, client, server.URL, tenantA) } + +// TestNoOpUpdatesDoNotPublishEvents verifies that an empty PATCH (no fields) +// writes nothing and therefore publishes no "updated" event — so it cannot fire +// tenant-owned side effects, including while the tenant is under a data purge +// (the no-op path intentionally returns the current row without locking). +func TestNoOpUpdatesDoNotPublishEvents(t *testing.T) { + eventRecorder := &tenantDataEventRecorder{} + + server, cleanup := setupTestServerWithEventProviders(t, eventRecorder) + defer cleanup() + + ctx := context.Background() + client := &http.Client{} + tenantA := "tenant-noop-" + uuid.NewString() + + record := createTenantDataFeedbackRecord(ctx, t, client, server.URL, tenantA, uuid.NewString(), "tenant-noop-field") + webhook := createTenantDataWebhook(ctx, t, client, server.URL, tenantA, "tenant-noop") + + // The two creates publish one event each; anchor the baseline once both land. + waitForEventCount(t, eventRecorder, 2) + baseline := eventRecorder.totalEventCount() + + t.Run("empty feedback PATCH returns 200 and publishes no event", func(t *testing.T) { + status, body := doTenantLockRequest( + ctx, t, client, http.MethodPatch, server.URL+"/v1/feedback-records/"+record.ID.String(), map[string]any{}) + require.Equal(t, http.StatusOK, status, "body: %s", string(body)) + requireEventCountStays(t, eventRecorder, baseline) + }) + + t.Run("empty webhook PATCH returns 200 and publishes no event", func(t *testing.T) { + status, body := doTenantLockRequest( + ctx, t, client, http.MethodPatch, server.URL+"/v1/webhooks/"+webhook.ID.String(), map[string]any{}) + require.Equal(t, http.StatusOK, status, "body: %s", string(body)) + requireEventCountStays(t, eventRecorder, baseline) + }) + + cleanupTenantDataBestEffort(ctx, client, server.URL, tenantA) +}