Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: webhook race condition on hook creation #1175

Merged
merged 2 commits into from
Sep 6, 2024
Merged

fix: webhook race condition on hook creation #1175

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7664f2a
fix: webhook race condition on hook creation
jbrockopp Sep 4, 2024
b3d6ea7
Merge branch 'main' into fix/webhook/race_condition
wass3rw3rk Sep 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 62 additions & 23 deletions api/webhook/post.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@

defer func() {
// send API call to update the webhook
_, err = database.FromContext(c).UpdateHook(ctx, h)

Check failure on line 193 in api/webhook/post.go

View workflow job for this annotation

GitHub Actions / golangci

[golangci] api/webhook/post.go#L193 

Non-inherited new context, use function like `context.WithXXX` instead (contextcheck)
Raw output 
api/webhook/post.go:193:32: Non-inherited new context, use function like `context.WithXXX` instead (contextcheck)
		_, err = database.FromContext(c).UpdateHook(ctx, h)
		                             ^
if err != nil {
l.Errorf("unable to update webhook %s/%d: %v", r.GetFullName(), h.GetNumber(), err)
}
Expand Down Expand Up @@ -239,35 +239,74 @@
b.SetRepo(repo)
h.SetRepoID(repo.GetID())

// send API call to capture the last hook for the repo
lastHook, err := database.FromContext(c).LastHookForRepo(ctx, repo)
if err != nil {
retErr := fmt.Errorf("unable to get last hook for repo %s: %w", repo.GetFullName(), err)
util.HandleError(c, http.StatusInternalServerError, retErr)
// number of times to retry
retryLimit := 3
// implement a loop to process asynchronous operations with a retry limit
//
// Some operations taken during the webhook workflow can lead to race conditions
// failing to successfully process the request. This logic ensures we attempt our
// best efforts to handle these cases gracefully.
for i := 0; i < retryLimit; i++ {
// check if we're on the first iteration of the loop
if i > 0 {
// incrementally sleep in between retries
time.Sleep(time.Duration(i) * time.Second)
}

h.SetStatus(constants.StatusFailure)
h.SetError(retErr.Error())
// send API call to capture the last hook for the repo
lastHook, err := database.FromContext(c).LastHookForRepo(ctx, repo)
if err != nil {
// format the error message with extra information
err = fmt.Errorf("unable to get last hook for repo %s: %w", r.GetFullName(), err)

return
}
// log the error for traceability
logrus.Error(err.Error())

// set the Number field
if lastHook != nil {
h.SetNumber(
lastHook.GetNumber() + 1,
)
}
// check if the retry limit has been exceeded
if i < retryLimit {
// continue to the next iteration of the loop
continue
}

// send API call to create the webhook
h, err = database.FromContext(c).CreateHook(ctx, h)
if err != nil {
retErr := fmt.Errorf("unable to create webhook %s/%d: %w", repo.GetFullName(), h.GetNumber(), err)
util.HandleError(c, http.StatusInternalServerError, retErr)
retErr := fmt.Errorf("%s: %w", baseErr, err)
util.HandleError(c, http.StatusInternalServerError, retErr)

h.SetStatus(constants.StatusFailure)
h.SetError(retErr.Error())
h.SetStatus(constants.StatusFailure)
h.SetError(retErr.Error())

return
return
}

// set the Number field
if lastHook != nil {
h.SetNumber(
lastHook.GetNumber() + 1,
)
}

// send API call to create the webhook
h, err = database.FromContext(c).CreateHook(ctx, h)
if err != nil {
// format the error message with extra information
err = fmt.Errorf("unable to create webhook %s/%d: %w", r.GetFullName(), h.GetNumber(), err)

// log the error for traceability
logrus.Error(err.Error())

// check if the retry limit has been exceeded
if i < retryLimit {
// continue to the next iteration of the loop
continue
}

retErr := fmt.Errorf("%s: %w", baseErr, err)
util.HandleError(c, http.StatusInternalServerError, retErr)

h.SetStatus(constants.StatusFailure)
h.SetError(retErr.Error())

return
}
}

l.WithFields(logrus.Fields{
Expand Down Expand Up @@ -385,7 +424,7 @@
deployment := webhook.Deployment

deployment.SetRepoID(repo.GetID())
deployment.SetBuilds([]*library.Build{b.ToLibrary()})

Check failure on line 427 in api/webhook/post.go

View workflow job for this annotation

GitHub Actions / golangci

[golangci] api/webhook/post.go#L427 

SA1019: library.Build is deprecated: use Build from github.com/go-vela/server/api/types instead. (staticcheck)
Raw output 
api/webhook/post.go:427:29: SA1019: library.Build is deprecated: use Build from github.com/go-vela/server/api/types instead. (staticcheck)
				deployment.SetBuilds([]*library.Build{b.ToLibrary()})
				                        ^

dr, err := database.FromContext(c).CreateDeployment(c, deployment)
if err != nil {
Expand Down Expand Up @@ -595,7 +634,7 @@
case "archived", "unarchived", constants.ActionEdited:
l.Debugf("repository action %s for %s", h.GetEventAction(), r.GetFullName())
// send call to get repository from database
dbRepo, err := database.FromContext(c).GetRepoForOrg(ctx, r.GetOrg(), r.GetName())

Check failure on line 637 in api/webhook/post.go

View workflow job for this annotation

GitHub Actions / golangci

[golangci] api/webhook/post.go#L637 

Non-inherited new context, use function like `context.WithXXX` instead (contextcheck)
Raw output 
api/webhook/post.go:637:38: Non-inherited new context, use function like `context.WithXXX` instead (contextcheck)
		dbRepo, err := database.FromContext(c).GetRepoForOrg(ctx, r.GetOrg(), r.GetName())
		                                   ^
if err != nil {
retErr := fmt.Errorf("%s: failed to get repo %s: %w", baseErr, r.GetFullName(), err)

Expand All @@ -606,7 +645,7 @@
}

// send API call to capture the last hook for the repo
lastHook, err := database.FromContext(c).LastHookForRepo(ctx, dbRepo)

Check failure on line 648 in api/webhook/post.go

View workflow job for this annotation

GitHub Actions / golangci

[golangci] api/webhook/post.go#L648 

Non-inherited new context, use function like `context.WithXXX` instead (contextcheck)
Raw output 
api/webhook/post.go:648:40: Non-inherited new context, use function like `context.WithXXX` instead (contextcheck)
		lastHook, err := database.FromContext(c).LastHookForRepo(ctx, dbRepo)
		                                     ^
if err != nil {
retErr := fmt.Errorf("unable to get last hook for repo %s: %w", r.GetFullName(), err)

Expand Down
Loading