Correctly rollback in ForkRepository

[zeripath]

The rollback functionality in
services/repository/repository.go:ForkRepository is incorrect and could
lead to a deadlock as it uses DeleteRepository to delete the rolled-back
repository - a function which creates its own transaction.

This PR adjusts the rollback function to only use RemoveAll as any
database changes will be automatically rolled-back. It also handles
panics and adjusts the Close within WithTx to ensure that if there is a
panic the session will always be closed.

Signed-off-by: Andrew Thornton [email protected]

[codecov-commenter]

Codecov Report

Merging #17034 (966439c) into main (8af7a21) will increase coverage by 0.02%.
The diff coverage is 45.45%.

❗ Current head 966439c differs from pull request most recent head 6da7f16. Consider uploading reports for the commit 6da7f16 to get more accurate results

@@            Coverage Diff             @@
##             main   #17034      +/-   ##
==========================================
+ Coverage   45.22%   45.24%   +0.02%     
==========================================
  Files         766      766              
  Lines       86632    86637       +5     
==========================================
+ Hits        39178    39203      +25     
+ Misses      41107    41088      -19     
+ Partials     6347     6346       -1     

Impacted Files	Coverage Δ
modules/repository/fork.go	54.28% <40.00%> (+5.89%)	⬆️
models/context.go	56.00% <100.00%> (-1.15%)	⬇️
modules/queue/queue_bytefifo.go	59.28% <0.00%> (-3.60%)	⬇️
modules/queue/queue_channel.go	95.00% <0.00%> (-1.67%)	⬇️
services/pull/pull.go	41.78% <0.00%> (-0.41%)	⬇️
modules/queue/workerpool.go	46.94% <0.00%> (-0.39%)	⬇️
models/issue_comment.go	52.05% <0.00%> (+0.88%)	⬆️
modules/log/event.go	62.50% <0.00%> (+1.85%)	⬆️
modules/git/utils.go	70.83% <0.00%> (+2.77%)	⬆️
modules/git/repo_base_nogogit.go	85.71% <0.00%> (+2.85%)	⬆️
... and 4 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 8af7a21...6da7f16. Read the comment docs.

[delvh]

While I can completely understand the implementation,
adding a test might be beneficial here so that this potential deadlock does not occur again sometime else.

[zeripath]

While I can completely understand the implementation,
adding a test might be beneficial here so that this potential deadlock does not occur again sometime else.

I don't think it's testable. The deadlock could only happen if there was an error whilst creating the fork, even then it would only occur extremely randomly. We can't force an error, we can't force the deadlock. Even if we had a fully mocked up xorm we'd have to code in what it means to deadlock.

There some very complex solutions that could help reduce the risk of these errors in future but they'd come with their own issues too.

If you can think of a test then I'm happy to discuss.

[delvh]

That is unfortunately correct.
The only thing we could test is whether it behaves correctly under normal circumstances.

[zeripath]

make lgtm work