fix(gitlab): prevent duplicate webhook creation via lock+double-check#117347
Draft
wedamija wants to merge 2 commits into
Draft
fix(gitlab): prevent duplicate webhook creation via lock+double-check#117347wedamija wants to merge 2 commits into
wedamija wants to merge 2 commits into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When a manual "Sync now" and the autosync beat (
scm_repo_sync_beat, fires every 1 min) both fire for the same org integration beforecreate_repos_batchhas written the new repo to DB, both tasks compute the repo as new and each dispatches acreate_repos_batch. Both tasks then callon_create_repositoryconcurrently. The in-memory guard (repo.config.get('webhook_id')) uses the stale object passed by the caller — neither task has savedwebhook_idyet — so both proceed to callcreate_project_webhook, producing a duplicate webhook in GitLab.The existing lock on
sync_repos_for_orgserialises the diff+dispatch phase correctly, butcreate_repos_batchruns async after the lock is released, so a subsequent beat tick firing in that window still sees the repo as new.Fix
In
on_create_repository, acquire a per-(integration_id, project_id)distributed lock, then re-read the repository from DB inside the lock (double-checked locking). The losing concurrent caller findswebhook_idalready present in the fresh read and returns without touching GitLab.blocking_acquire(initial_delay=1, timeout=30)lets the loser wait briefly rather than fail immediately.UnableToAcquireLockpropagates so the Celery task (retry=Retry(times=3, delay=120)) can recover.What was verified
test_create_repositoryandtest_create_repository_verify_payloadstill cover the normal happy path (unchanged).test_on_create_repository_skips_if_webhook_id_already_in_db: stale in-memory object with nowebhook_idbut DB row has it → no GitLab API call.test_on_create_repository_propagates_lock_timeout:UnableToAcquireLockfrom lock propagates so Celery can retry.What remains unverified
Full test suite not run (sandbox limitation). The two new tests exercise the core correctness invariants.
View Session in Sentry