Revert index when error occur on issue/PR creation #21362

Gusted · 2022-10-06T20:10:07Z

Currently when a error occur in NewIssueWithIndex, The functions NewIssue and NewPullRequest will not revert the index update, thus a index has been "allocated", but end up not being used. This leaves users confusing, why a index has been skipped.
Add a UpsertDecrResourceIndex function, which will try to decrease the index, only when nobody else tried to allocate a index in the mean time. If it has been the case someone else allocated a new index in the mean time, it's not worth it (complexity in code)to "revert" the index.
Reference: https://codeberg.org/Codeberg/Community/issues/743

- Currently when a error occur in `NewIssueWithIndex`, The functions `NewIssue` and `NewPullRequest` will not revert the index update, thus a index has been "allocated", but end up not being used. - Add a `UpsertDecrResourceIndex` function, which will try to decrease the index, only when nobody else tried to allocate a index in the mean time.

models/db/index.go

wxiaoguang · 2022-10-07T03:12:44Z

Why it is needed since the SQLs are in a transaction and the transaction would be rolled back if error occurs?

fnetX · 2022-10-07T10:09:40Z

At least on Codeberg, we have multiple repos with wrong indices now, most were related to some kind of error.

What could also be an option: If I recall correctly, there is some function to recalculate the correct indices, used by doctor. It shouldn't cost too much to recalculate if the index is still correct after some kind of error occurs, and it would definitely fix every case.

wxiaoguang · 2022-10-07T10:55:11Z

If database transaction should do the rollback work, why the code is needed? If database transaction doesn't work, then there must be some real underlying bug, the real bugs shouldn't be hidden by the patch.

And I recall an old issue about another MariaDB bug for prepared statement:

Linking issue dependencies fails (API returns 500 error) #19368 (comment)

At that time if that bug was bypassed, there would be more problems.

My question is: why the transaction doesn't reset the issue index when error occurs?

lunny · 2022-10-07T12:16:37Z

Except caused by an error, I think an index skipped is a good design than making it always align. Because if you delete last issue, you should not reuse the allocated index for that issue.

zeripath · 2022-10-08T13:26:26Z

models/issues/issue.go

 	if err = NewIssueWithIndex(ctx, issue.Poster, NewIssueOptions{
 		Repo:        repo,
 		Issue:       issue,
 		LabelIDs:    labelIDs,
 		Attachments: uuids,
 	}); err != nil {
+		if err := onFail(); err != nil {
+			return fmt.Errorf("couldn't gracefully handle error: %v", err)


Suggested change

return fmt.Errorf("couldn't gracefully handle error: %v", err)

return fmt.Errorf("couldn't gracefully handle error: %w", err)

Is this a new code-style I haven't been aware of? Since when are we going to use the wrapping errors verb?

Well, as I've learned: go seems to do some magic if you use %w instead of %v:
https://stackoverflow.com/a/61287626

if we can wrap stuff my pr which tries to create a common hierarchy of errors will be a lot easier

zeripath · 2022-10-08T13:26:34Z

models/issues/issue.go

+		committer.Close()
+		// Try to revert the increase in resource index.
+		if err := db.UpsertDecrResourceIndex(db.DefaultContext, "issue_index", repo.ID, idx); err != nil {
+			return fmt.Errorf("UpsertDecrResourceIndex: %v", err)


Suggested change

return fmt.Errorf("UpsertDecrResourceIndex: %v", err)

return fmt.Errorf("UpsertDecrResourceIndex: %w", err)

zeripath · 2022-10-08T13:26:47Z

models/issues/issue.go

 		if repo_model.IsErrUserDoesNotHaveAccessToRepo(err) || IsErrNewIssueInsert(err) {
 			return err
 		}
 		return fmt.Errorf("newIssue: %v", err)
 	}

 	if err = committer.Commit(); err != nil {
+		if err := onFail(); err != nil {
+			return fmt.Errorf("couldn't gracefully handle error: %v", err)


Suggested change

return fmt.Errorf("couldn't gracefully handle error: %v", err)

return fmt.Errorf("couldn't gracefully handle error: %w", err)

And wrap the rest of them too if possible please

zeripath · 2022-10-08T13:27:02Z

models/issues/issue.go

+		if err := onFail(); err != nil {
+			return fmt.Errorf("couldn't gracefully handle error: %v", err)
+		}
+
 		if repo_model.IsErrUserDoesNotHaveAccessToRepo(err) || IsErrNewIssueInsert(err) {
 			return err
 		}
 		return fmt.Errorf("newIssue: %v", err)


Wrap errors!

Suggested change

return fmt.Errorf("newIssue: %v", err)

return fmt.Errorf("newIssue: %w", err)

zeripath · 2022-10-08T13:28:23Z

models/issues/pull.go

+		// Close commiter.
+		committer.Close()
+		// Try to revert the increase in resource index.
+		if err := db.UpsertDecrResourceIndex(outerCtx, "issue_index", repo.ID, idx); err != nil {


I suspect that outerCtx isn't the correct thing here. In fact I'm certain it isn't.

db.DefaultContext is needed here.

zeripath · 2022-10-08T13:32:47Z

I suspect that the times where this would actually work are actually infrequent. The decrement can only work if the next ID hasn't been requested and I suspect that most failures occur when there is a double click new Issue/Pull and a double ID has been added.

Gusted · 2022-10-10T19:12:49Z

Hi all,

Sorry for the late response. So a small background-story of why the code is currently as-is, the call to db.GetNextResourceIndex should be done separately, as it has it's own transaction(and feels unsafe to let the caller of the function do that), likely this can be simplified but I have zero clue. Therefor, the "rollback" code is not done via the transaction, but instead the "goFail" function.

Except caused by an error, I think an index skipped is a good design than making it always align. Because if you delete last issue, you should not reuse the allocated index for that issue.

Why align? The issue is never inserted into the database, so it's actually not-aligned if we don't try to rollback the update in issue_index table.
Could you elaborate on the "delete the last issue"? That shouldn't have any impact on this code.

My question is: why the transaction doesn't reset the issue index when error occurs?

The transaction doesn't cover the call to db.GetNextResourceIndex.

I suspect that the times where this would actually work are actually infrequent. The decrement can only work if the next ID hasn't been requested and I suspect that most failures occur when there is a double click new Issue/Pull and a double ID has been added.

It's a bug non-the-less that should be fixed. It was only observed due to a custom patch in Codeberg's Gitea fork, which would make the error "more frequent". It's a best-try effort to decrement the issue index, if it's not possible then nothing we can do.

Gusted

wxiaoguang · 2022-10-10T19:23:42Z

My question is: why the transaction doesn't reset the issue index when error occurs?

The transaction doesn't cover the call to db.GetNextResourceIndex.

Then the question is: why not make GetNextResourceIndex respect the transaction? It should be clear and correct.

the call to db.GetNextResourceIndex should be done separately

Why the call to GetNextResourceIndex should be done separately, couldn't the code be refactored to use transaction correctly?

Gusted · 2022-10-10T19:26:50Z

Then the question is: why not make GetNextResourceIndex respect the transaction? It should be clear and correct.

Please see:

So a small background-story of why the code is currently as-is, the call to db.GetNextResourceIndex should be done separately, as it has it's own transaction(and feels unsafe to let the caller of the function do that), likely this can be simplified but I have zero clue.

But if everyone feels fine with adding the appropriate comment to that function we can try and move the transaction handling to the caller of the function.

wxiaoguang · 2022-10-10T19:36:19Z

Then the question is: why not make GetNextResourceIndex respect the transaction? It should be clear and correct.

Please see:

So a small background-story of why the code is currently as-is, the call to db.GetNextResourceIndex should be done separately, as it has it's own transaction(and feels unsafe to let the caller of the function do that), likely this can be simplified but I have zero clue.

But if everyone feels fine with adding the appropriate comment to that function we can try and move the transaction handling to the caller of the function.

Yup, I have read the comment but at that time I just couldn't understand why the code couldn't be refactored to use correct transaction.

IMO database transaction is the proper approach for this problem, no need to re-invent wheels to reset the data.

There are a lot of bugs in Gitea, I believe this is not the only one which misuses database transaction. Making good use of transaction everywhere will resolve problem fundamentally.

Gusted · 2022-10-10T19:41:49Z

Alright @wxiaoguang, I will refactor the code to let the caller take care of the transaction.

Gusted · 2022-10-10T19:59:35Z

Okay, seems like this doesn't seem as easy as expected, the code has a good reason to implement the transaction itself:

gitea/models/db/index.go

Line 64 in 94037ad

for i := 0; i < MaxDupIndexAttempts; i++ {

It will retry multiple times if needed, unfortunately you can't cover that with one general transaction, it has to rollback immediately if the resource is outdated and try again.

So looking around I found savepoints which can be used as replacements, such that it can be used in the transaction and rollback just that portion of the actions. But Xorm currently doesn't builtin function for Savepoint and I'd prefer to not mess around with raw SQL on something I don't understand that great.

Another option would be if the GetNextResourceIndex creates the transaction for each iteration and returns the "correct" transaction if it succeeds.

@wxiaoguang What are your thoughts?

wxiaoguang · 2022-10-11T03:12:17Z

I think the old code is wrong.

A simple if (UPDATE value=value +1).rows == 0 { insert() } is enough for a transaction.

The first UPDATE will acquire the write lock, it's safe to do other operations on it then, no concurrency problem.

Gusted · 2022-10-11T18:08:55Z

I think the old code is wrong.

A simple if (UPDATE value=value +1).rows == 0 { insert() } is enough for a transaction.

I have no idea what that means. The PR that added this, seems to have the same discussion if the code is good.

CC @zeripath @lunny

Gusted · 2022-10-14T21:07:19Z

I think the old code is wrong.

A simple if (UPDATE value=value +1).rows == 0 { insert() } is enough for a transaction.

@wxiaoguang I'm still not necessary sure of what you mean(and which exact SQL actions needs to be done), would you like to create a PR for this? To simplify that function. Happy to review such PR, so this PR can also be simplified 😅.

wxiaoguang · 2022-10-15T13:33:17Z

I will take a try

wxiaoguang · 2022-10-15T14:18:39Z

Refactor GetNextResourceIndex to make it work properly with transaction #21469

You can ignore the diff and only take a look at the new code.

If the code looks good to you, I can add some tests.

…on (#21469) Related: * #21362 This PR uses a general and stable method to generate resource index (eg: Issue Index, PR Index) If the code looks good, I can add more tests ps: please skip the diff, only have a look at the new code. It's entirely re-written. Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>

lunny · 2022-10-16T10:44:55Z

replaced by #21469

Gusted added this to the 1.18.0 milestone Oct 6, 2022

Gusted added type/bug backport/v1.17 labels Oct 6, 2022

Remove testing code

8e8f781

Gusted commented Oct 6, 2022

View reviewed changes

models/db/index.go Show resolved Hide resolved

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 6, 2022

Merge branch 'main' into gracefully-revert-idx

13cf733

zeripath reviewed Oct 8, 2022

View reviewed changes

Merge branch 'main' into gracefully-revert-idx

0bb3bf5

Merge branch 'main' into gracefully-revert-idx

cce1089

Gusted added 2 commits October 11, 2022 18:09

Merge branch 'main' into gracefully-revert-idx

9951025

Merge branch 'main' into gracefully-revert-idx

7ea3b9c

wxiaoguang mentioned this pull request Oct 15, 2022

Refactor GetNextResourceIndex to make it work properly with transaction #21469

Merged

lunny closed this Oct 16, 2022

lunny removed this from the 1.18.0 milestone Oct 16, 2022

Gusted deleted the gracefully-revert-idx branch October 16, 2022 19:18

go-gitea locked and limited conversation to collaborators May 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert index when error occur on issue/PR creation #21362

Revert index when error occur on issue/PR creation #21362

Gusted commented Oct 6, 2022 •

edited

Loading

wxiaoguang commented Oct 7, 2022

fnetX commented Oct 7, 2022

wxiaoguang commented Oct 7, 2022

lunny commented Oct 7, 2022 •

edited

Loading

zeripath Oct 8, 2022

Gusted Oct 10, 2022

delvh Oct 10, 2022

zeripath Oct 10, 2022

zeripath Oct 8, 2022

zeripath Oct 8, 2022

zeripath Oct 8, 2022

zeripath Oct 8, 2022

zeripath Oct 8, 2022

zeripath commented Oct 8, 2022

Gusted commented Oct 10, 2022

wxiaoguang commented Oct 10, 2022 •

edited

Loading

Gusted commented Oct 10, 2022

wxiaoguang commented Oct 10, 2022

Gusted commented Oct 10, 2022

Gusted commented Oct 10, 2022

wxiaoguang commented Oct 11, 2022 •

edited

Loading

Gusted commented Oct 11, 2022

Gusted commented Oct 14, 2022

wxiaoguang commented Oct 15, 2022

wxiaoguang commented Oct 15, 2022

lunny commented Oct 16, 2022

	return fmt.Errorf("couldn't gracefully handle error: %v", err)
	return fmt.Errorf("couldn't gracefully handle error: %w", err)

	return fmt.Errorf("UpsertDecrResourceIndex: %v", err)
	return fmt.Errorf("UpsertDecrResourceIndex: %w", err)

	return fmt.Errorf("newIssue: %v", err)
	return fmt.Errorf("newIssue: %w", err)

Revert index when error occur on issue/PR creation #21362

Revert index when error occur on issue/PR creation #21362

Conversation

Gusted commented Oct 6, 2022 • edited Loading

wxiaoguang commented Oct 7, 2022

fnetX commented Oct 7, 2022

wxiaoguang commented Oct 7, 2022

lunny commented Oct 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeripath commented Oct 8, 2022

Gusted commented Oct 10, 2022

wxiaoguang commented Oct 10, 2022 • edited Loading

Gusted commented Oct 10, 2022

wxiaoguang commented Oct 10, 2022

Gusted commented Oct 10, 2022

Gusted commented Oct 10, 2022

wxiaoguang commented Oct 11, 2022 • edited Loading

Gusted commented Oct 11, 2022

Gusted commented Oct 14, 2022

wxiaoguang commented Oct 15, 2022

wxiaoguang commented Oct 15, 2022

lunny commented Oct 16, 2022

Gusted commented Oct 6, 2022 •

edited

Loading

lunny commented Oct 7, 2022 •

edited

Loading

wxiaoguang commented Oct 10, 2022 •

edited

Loading

wxiaoguang commented Oct 11, 2022 •

edited

Loading