Retry update on creation failure in dp to cp import#2521
Conversation
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| // | ||
| // Detection is by err.Error() substring, so a violation wrapped with | ||
| // fmt.Errorf("...: %w", err) is still detected without unwrapping. | ||
| func IsUniqueViolation(err error) bool { |
There was a problem hiding this comment.
Mm, so we have to update this func when we introduce a new DB type, neh
There was a problem hiding this comment.
Yes, since we are throwing the raw error from the repository layer and we need to catch this specific error.
2 participants
Summary
When two or more gateways push an artifact with the same
metadata.name(handle) to the control plane at the same time, the importer could fail one of the pushes with a unique-constraint error and, on the gateway's retry, leave a duplicate deployment. This PR makes the importer race-safe by catching the unique violation, re-resolving the artifact by handle, and retrying the push as an update.Root cause
ArtifactImportService.importValidatedresolves an existing artifact withGetByHandleand then lets the per-kind importer take its Create branch when nothing was found. The read and the write are not in one critical section, so two concurrent pushes of the same (org, handle) both observe nil and both attempt an insert. The child table'sUNIQUE(organization_uuid, handle)index lets only one commit; the loser gets a unique-constraint violation that surfaced as a failed import. It affects all artifact kinds (REST, LLM provider, LLM proxy, MCP proxy, LLM provider template) since it lives in the shared import path.Changes
repository/errors.go(new): IsUniqueViolation(err) — a shared, dialect-aware detector for unique/primary-key violations across the three databases the CP supports (SQLite, PostgreSQL, SQL Server). Matches through %w wrapping. The existing unexported isUniqueViolation now delegates to it (and gains SQL Server coverage).service/artifact_import.go: extracted the resolve → decide → import step intoresolveAndImport(...)and wrapped it in a bounded retry loop (importConflictMaxRetries = 3) inimportValidated. On aIsUniqueViolationerror, it re-resolves by handle (the winner's row now exists) and retries — which takes the importer's Update branch and re-runs the last-in-wins decision (DecideMetadataWrite).The retried (losing) push re-enters
DecideMetadataWritewith the now-committed watermark:a newer deployedAt → WriteFullMetadata (it wins the working copy)
a stale one →
SkipWorkingCopy(the winner's copy is untouched)Only the spurious insert failure is removed. Each push still records its own immutable deployment row.
Why catch-and-retry (not ON CONFLICT / advisory locks)
The CP targets three SQL dialects, so ON CONFLICT/MERGE and Postgres advisory locks aren't portable. Catch-and-retry is portable and already idiomatic in this codebase (subscription_repository does the same). Verified that every per-kind importer/repo propagates the raw DB error via %w (no sentinel translation) and that each child table carries the UNIQUE(organization_uuid, handle) constraint, so the violation is reliably detectable for all kinds.
Testing