WIP: Fix incorrect open issue count after commit close

[lynxplay]

This commit fixes the bug that a repositories open issue value is not
updated when an issue is closed through a commit that references such
issue.

More specifically, creating an issue on a repository with currently 0
open bugs lead to the repository showing one open issue.
Closing such issue through a commit onto the branch this issue was
targetting would close the issue correctly but failed to update the
issue counter in the repositories header. Hence the repository would
show one open issue while the open issue list remains empty.

As this commit is a response to #10536, more information about the
bug can be aquired there.

Fixes #10536.

Problem:
The issue comes from the fact that the the commit issue hooks are executed before this line, which updates the entire repository into the database.
This overwrites the calls further up in the method which updates the number of closed issues correctly.

Solution:
To solve this (and to run the solving logic as few times as possible) this commit implemented a simple flag in the CommitRepoAction method that tracks if any commit actually modifies the closed issue number.

If that is the case, we select the updated value from the database and assign it to the local repository instance so the updated value is saved to the database in line 277.

[codecov-io]

Codecov Report

Merging #10541 into master will increase coverage by 0.06%.
The diff coverage is 36%.

@@            Coverage Diff             @@
##           master   #10541      +/-   ##
==========================================
+ Coverage   43.66%   43.72%   +0.06%     
==========================================
  Files         586      586              
  Lines       81553    81595      +42     
==========================================
+ Hits        35607    35680      +73     
+ Misses      41539    41503      -36     
- Partials     4407     4412       +5

Impacted Files	Coverage Δ
models/repo.go	51.08% <0%> (-0.45%)	⬇️
modules/repofiles/action.go	64.11% <75%> (+0.66%)	⬆️
modules/indexer/code/indexer.go	38.02% <0%> (-4.6%)	⬇️
modules/indexer/issues/indexer.go	57.52% <0%> (-1.39%)	⬇️
services/pull/pull.go	34.89% <0%> (-1.18%)	⬇️
models/gpg_key.go	53.95% <0%> (+0.52%)	⬆️
models/pull.go	42.37% <0%> (+0.55%)	⬆️
modules/log/event.go	65.64% <0%> (+1.02%)	⬆️
models/unit.go	39.5% <0%> (+2.46%)	⬆️
services/pull/patch.go	62.89% <0%> (+2.51%)	⬆️
... and 5 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 154b137...c7fbede. Read the comment docs.

[guillep2k]

This should be handled at changeIssueStatus(). If it doesn't, we should look into it and find the reason. Otherwise other counters would fail to update, like milestone's, label's, etc.

IMHO the problem is ...... wait for it ..... a locking issue (this section also relevant). 😁

[jolheiser]

imo what's biting us here is the models.UpdateRepository, as it updates all columns without any refresh check first.
Perhaps a better solution would be to only update the columns that this function intended to change, rather than all.

I'm not sure if the comment is still accurate (probably not quite), but // Change repository empty status and update last updated time. implies that only a few things would have changed, so updating all columns is overkill and can lead to bugs like this in the future.

To clarify, changeIssueStatus() is working as intended, however as mentioned in the OP it is then overridden because of a later update with old data.

[guillep2k]

imo what's biting us here is the models.UpdateRepository, as it updates all columns without any refresh check first.

That's exactly my point: we read the repository.... changes occur in the middle.... we write stale information. Even if we restrict the writing to the specific columns of interest, we could still be writing stale info; for example when we update counters, statuses or info that depends on those statuses we've read before. Getting collisions on those updates is just a matter of chance and server load. Specificity would only reduce our chances of collisions. That's why statements like SELECT ... FOR UPDATE exist. 😁

[lynxplay]

Well SELECT ... FOR UPDATE works nicely for parallel transactions, running on the same table. The models.UpdateRepository function creates a new session and therefore a new transaction tho, invalidating this concept right ?

As this is my first time working on this project, I am not quite sure how parallel the repository fetching from the database can be but why is it even necessary to completely recalculate the number of closed issues.

Simply updating the repository structs value by either removing or adding 1 to the count would suffice as that updated value would then be stored correctly.
Even though closing the issue manually (executed here) does not seem to update the repository (which is why the bug does not occur when manually closing the issue)

[guillep2k]

Well SELECT ... FOR UPDATE works nicely for parallel transactions, running on the same table. The models.UpdateRepository function creates a new session and therefore a new transaction tho, invalidating this concept right ?

Well... we should fix that. 😑

[lynxplay]

Well... we should fix that. 😑

Concerning that the repository is provided by the context and is read in its own session, we would have to forward the initial session/transaction that is used to create the repository all the way through the entire endpoint logic.

Seems like a pretty big fix for such a problem.

[jackzhum]

This bug has not been fixed in version 1.11.4

[CatoLynx]

Is there any update on this? I'm on 1.11.3, the issue is present there, I checked the releases since then and did not see any mention of this issue.

[lynxplay]

Closed in favour of #11310
Any further motion on this issue will hopefully be linked in that PR or the issues linked there.