models/issue.go NewIssue() could fail due to duplicate key #3796

New Issue

GiteaMirror · 2025-11-02T05:25:30-06:00

GiteaMirror commented

2025-11-02 05:25:30 -06:00

Originally created by @guillep2k on GitHub (Aug 16, 2019).

The current code for models/issue.go roughly does:

func getMaxIndexOfIssue(e Engine, repoID int64) (int64, error) {
    var (
        maxIndex int64
        has      bool
        err      error
    )

    has, err = e.SQL("SELECT COALESCE((SELECT MAX(`index`) FROM issue WHERE repo_id = ?),0)", repoID).Get(&maxIndex)

   [...]

    return maxIndex, nil
}

func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) {
	opts.Issue.Title = strings.TrimSpace(opts.Issue.Title)

	maxIndex, err := getMaxIndexOfIssue(e, opts.Issue.RepoID)
	if err != nil {
		return err
	}
	opts.Issue.Index = maxIndex + 1

   [...]

	if _, err = e.Insert(opts.Issue); err != nil {
		return err
	}

   [...]
}

It's possible that under heavy loads (e.g. a small server) two sessions may attempt to use the same index number to create issues; one of the two will fail due to a race condition:

User A submits the form creating an issue for repo abc.
User B submits the form creating another issue for repo abc.
At issue.go, in the context of user A's request, getMaxIndexOfIssue() returns 99. It will be used at step 5.
At issue.go, in the context of user B's request, getMaxIndexOfIssue() also returns 99. because at the moment that's the latest issue for the repo. It will be used at step 6.
At issue.go, in the context of user A's request, newIssue() inserts A's issue with Index: 100.
At issue.go, in the context of user B's request, newIssue() attempts to insert B's issue with Index: 100 and fails with duplicate key error, because 100 was used at step 5.

This will of course depend on the database locking model.

I think the best way to handle this is to use a mutex, thus covering most scenarios.

Originally created by @guillep2k on GitHub (Aug 16, 2019). The current code for `models/issue.go` roughly does: ``` func getMaxIndexOfIssue(e Engine, repoID int64) (int64, error) { var ( maxIndex int64 has bool err error ) has, err = e.SQL("SELECT COALESCE((SELECT MAX(`index`) FROM issue WHERE repo_id = ?),0)", repoID).Get(&maxIndex) [...] return maxIndex, nil } func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) { opts.Issue.Title = strings.TrimSpace(opts.Issue.Title) maxIndex, err := getMaxIndexOfIssue(e, opts.Issue.RepoID) if err != nil { return err } opts.Issue.Index = maxIndex + 1 [...] if _, err = e.Insert(opts.Issue); err != nil { return err } [...] } ``` It's possible that under heavy loads (e.g. a small server) two sessions may attempt to use the same index number to create issues; one of the two will fail due to a race condition: 1. User **A** submits the form creating an issue for repo abc. 2. User **B** submits the form creating another issue for repo abc. 3. At `issue.go`, in the context of user **A**'s request, `getMaxIndexOfIssue()` returns `99`. It will be used at step *5*. 4. At `issue.go`, in the context of user **B**'s request, `getMaxIndexOfIssue()` also returns `99`. because at the moment that's the latest issue for the repo. It will be used at step *6*. 5. At `issue.go`, in the context of user **A**'s request, `newIssue()` inserts **A**'s issue with `Index: 100`. 6. At `issue.go`, in the context of user **B**'s request, `newIssue()` attempts to insert **B**'s issue with `Index: 100` and fails with `duplicate key error`, because `100` was used at step *5*. This will of course depend on the database locking model. I think the best way to handle this is to use a mutex, thus covering most scenarios.

GiteaMirror closed this issue

2025-11-02 05:25:30 -06:00

GiteaMirror commented

2025-11-02 05:25:30 -06:00

@zeripath commented on GitHub (Aug 17, 2019):

So a mutex would actually be at the wrong level - it needs to be at the DB level (Consider clusters). There's only really three options to fix it:

Just retry and hope you can get it right once - Adding a mutex to the web server level will help prevent hitting the problem if you're not in a cluster but should also reduce the likelihood of hitting the problem if you are.
Use a DB stored procedure to generate the issue number. (Not guaranteed to completely solve this issue on all DBs IIRC.)
Change the transactional isolation mode on your DB. (Also not guaranteed to completely solve the issue on all DBs IIRC.)

I think option 1 is probably a reasonable thing to do, but someone with more specific DB knowledge could look into how much work it would be to add 2 or 3 - but I think you still need retries when I last looked into this.

@zeripath commented on GitHub (Aug 17, 2019): So a mutex would actually be at the wrong level - it needs to be at the DB level (Consider clusters). There's only really three options to fix it: 1. Just retry and hope you can get it right once - Adding a mutex to the web server level will help prevent hitting the problem if you're not in a cluster but should also reduce the likelihood of hitting the problem if you are. 2. Use a DB stored procedure to generate the issue number. (Not guaranteed to completely solve this issue on all DBs IIRC.) 3. Change the transactional isolation mode on your DB. (Also not guaranteed to completely solve the issue on all DBs IIRC.) I think option 1 is probably a reasonable thing to do, but someone with more specific DB knowledge could look into how much work it would be to add 2 or 3 - but I think you still need retries when I last looked into this.

GiteaMirror commented

2025-11-02 05:25:31 -06:00

@lunny commented on GitHub (Aug 17, 2019):

If we want finish that on DB level, it's a complex SQL and specific for different databases. So I think the simple and dirty method is just retry it 3 times if it failed because of exist issue number.

@lunny commented on GitHub (Aug 17, 2019): If we want finish that on DB level, it's a complex SQL and specific for different databases. So I think the simple and dirty method is just retry it 3 times if it failed because of exist issue number.

GiteaMirror commented

2025-11-02 05:25:31 -06:00

@typeless commented on GitHub (Aug 28, 2019):

Ultimately, I would like to consider dropping the index column from the table. It should be something deducible from the other columns.

@typeless commented on GitHub (Aug 28, 2019): Ultimately, I would like to consider dropping the `index` column from the table. It should be something deducible from the other columns.

GiteaMirror commented

2025-11-02 05:25:31 -06:00

@lunny commented on GitHub (Oct 1, 2019):

~~This should be resolved by #8309~~

@lunny commented on GitHub (Oct 1, 2019): ~This should be resolved by #8309~

GiteaMirror commented

2025-11-02 05:25:31 -06:00

@guillep2k commented on GitHub (Oct 1, 2019):

This should be resolved by #8309

Unfortunately, it won't. Consider the following scenario:

By the time T2 inserts the row in issue using MAX(index), T1 already picked the value 3 for that, but T2 can't possible know about it because T1 didn't commit the changes yet. So T2 also picks the value 3 and fails at the time of the commit.

I realized this when I created the stress test in #8005. It will always fail with this kind of method, unless we use some method of serialization like a semaphore or a lock in the table. But the semaphore will not scale in a multi-server setup as you've pointed out, and a database lock (either at row or table level) is dangerous because it's very easy to end up with a deadlock.

That's why I gave up and went for the retry route.

@guillep2k commented on GitHub (Oct 1, 2019): > > > This should be resolved by #8309 Unfortunately, it won't. Consider the following scenario: ![image](https://user-images.githubusercontent.com/18600385/65973674-1a327680-e442-11e9-84cc-143209a15b15.png) By the time T2 inserts the row in `issue` using `MAX(index)`, T1 already picked the value `3` for that, but T2 can't possible know about it because T1 didn't commit the changes yet. So T2 also picks the value `3` and fails at the time of the commit. I realized this when I created the stress test in #8005. It will always fail with this kind of method, unless we use some method of serialization like a semaphore or a lock in the table. But the semaphore will not scale in a multi-server setup as you've pointed out, and a database lock (either at row or table level) is dangerous because it's very easy to end up with a deadlock. That's why I gave up and went for the retry route.

Sign in to join this conversation.

Branches Tags

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: github-starred/gitea#3796