Skip to content

Fix issue where commenting with a status change could partially fail. #35317

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.

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

lunny
Copy link
Member

@lunny lunny commented Aug 19, 2025

When commenting on an issue while changing its status (close or reopen), the comment may sometimes not be created even though the status has changed. This happens because the two operations are executed in separate database transactions.

This PR ensures both operations are performed within the same database transaction, preventing such inconsistencies.

@lunny lunny added type/bug backport/v1.24 This PR should be backported to Gitea 1.24 labels Aug 19, 2025
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 19, 2025
@github-actions github-actions bot added modifies/translation modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code labels Aug 19, 2025
@lunny lunny removed the backport/v1.24 This PR should be backported to Gitea 1.24 label Aug 19, 2025
@lunny lunny added this to the 1.25.0 milestone Aug 19, 2025
@lunny lunny changed the title Fix bug partially failure when commenting with status change in an issue Fix issue where commenting with a status change could partially fail. Aug 19, 2025
@wxiaoguang
Copy link
Contributor

When commenting on an issue while changing its status (close or reopen), the comment may sometimes not be created even though the status has changed. This happens because the two operations are executed in separate database transactions.

Are you sure it is really the problem? In the defer func(), the previous transaction should have been committed, so where is the bug?

@lunny
Copy link
Member Author

lunny commented Aug 19, 2025

When commenting on an issue while changing its status (close or reopen), the comment may sometimes not be created even though the status has changed. This happens because the two operations are executed in separate database transactions.

Are you sure it is really the problem? In the defer func(), the previous transaction should have been committed, so where is the bug?

I’m certain there’s a bug here—I’ve encountered it multiple times myself. If CreateComment fails, the defer functions are still executed, but there’s no err check in place.

@wxiaoguang
Copy link
Contributor

I believe this change is wrong. The defer func() itself is already an abuse, you didn't figure the root problem but make the logic more messy.

@lunny
Copy link
Member Author

lunny commented Aug 19, 2025

I believe this change is wrong. The defer func() itself is already an abuse, you didn't figure the root problem but make the logic more messy.

The issue occurs because createComment fails, but the close/reopen action still continues executing. I have mentioned this in my previous comment. I think you are right, the defer function looks not right. I have sent 0434311 to remove it.

@wxiaoguang
Copy link
Contributor

Then it comes to another question: why you need to change issue_service.CloseIssue/ReopenIssue (add more arguments like "", nil)?

I think the only thing you need to do is to rewrite the abused defer func()?

@lunny
Copy link
Member Author

lunny commented Aug 20, 2025

Then it comes to another question: why you need to change issue_service.CloseIssue/ReopenIssue (add more arguments like "", nil)?

I think the only thing you need to do is to rewrite the abused defer func()?

Because Comment and Close should be in the same database transaction to ensure it will not fail partially. Or I can create a new method named ComemntAndClose and ComentAndReopen. I don't think rewrite the defer func() could address the target.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code modifies/translation type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants