Skip to content

Fix flaky TestCatFileBatch/QueryTerminated test#37159

Merged
silverwind merged 6 commits intogo-gitea:mainfrom
silverwind:fix-catfile-flaky-test
Apr 10, 2026
Merged

Fix flaky TestCatFileBatch/QueryTerminated test#37159
silverwind merged 6 commits intogo-gitea:mainfrom
silverwind:fix-catfile-flaky-test

Conversation

@silverwind
Copy link
Copy Markdown
Member

@silverwind silverwind commented Apr 9, 2026

TestCatFileBatch/QueryTerminated relied on timing to distinguish os.ErrClosed vs io.EOF error paths. Replace time.Sleep-based synchronization with a channel-based hook on pipe close, making both error paths fully deterministic regardless of CI runner speed.

Ref: https://github.com/go-gitea/gitea/actions/runs/24193070536/job/70615366804


This PR was written with the help of Claude Opus 4.6

Widen timing margins from 20-40ms to 500ms. The test asserts on
specific error types (`os.ErrClosed` vs `io.EOF`) depending on
whether pipe close or pipe read wins the race. On slow CI runners the
original 20ms margin was too tight, causing the close path to lose the
race and return EOF instead of ErrClosed.

Co-Authored-By: silverwind <me@silverwind.io>
Co-Authored-By: Claude (Opus 4.6) <noreply@anthropic.com>
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 9, 2026
@silverwind silverwind added backport/v1.26 This PR should be backported to Gitea 1.26 type/testing labels Apr 9, 2026
@silverwind silverwind requested a review from Copilot April 9, 2026 14:07
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR reduces flakiness in TestCatFileBatch/QueryTerminated by widening timing margins in a race-sensitive test so CI scheduling variance is less likely to flip the observed error (os.ErrClosed vs io.EOF).

Changes:

  • Increased the “reader slower than close” delay to more reliably produce os.ErrClosed.
  • Increased the “close slower than reader” delay to more reliably produce io.EOF.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread modules/git/catfile_batch_test.go Outdated
silverwind and others added 2 commits April 9, 2026 16:35
Use a function hook and channels instead of time.Sleep delays for
deterministic test synchronization. Eliminates all timing dependencies.

Co-Authored-By: silverwind <me@silverwind.io>
Co-Authored-By: Claude (Opus 4.6) <noreply@anthropic.com>
Co-Authored-By: silverwind <me@silverwind.io>
Co-Authored-By: Claude (Opus 4.6) <noreply@anthropic.com>
@gerliczkowalczuk
Copy link
Copy Markdown

gerliczkowalczuk commented Apr 9, 2026

@silverwind This change makes sense to me, especially the idea of controlling the race between Read and pipe closing in tests.

I just have a question about the implementation:

  • Regarding the global debug hook:

    var catFileBatchDebugPipeClose atomic.Pointer[func(stdPipeClose func())]

    This is used from tests to control timing, which is really helpful, but is there any concern about this being global state if tests run in parallel?

    I see defer ...Store(nil) is used, but I’m wondering if this would still be safe if t.Parallel() is ever introduced in this package, since tests could override this pointer concurrently.

  • Also, just to confirm my understanding of the two test cases:

    • PipeClosedBeforeReados.ErrClosed
    • ReadBeforePipeCloseio.EOF

    The difference here is purely the ordering enforced by the hook and channels, right?

Overall, replacing the previous timing-based approach with hooks + channels makes the test much clearer and more deterministic. Nice improvement!

@wxiaoguang wxiaoguang force-pushed the fix-catfile-flaky-test branch 2 times, most recently from 527dc3a to c2a340a Compare April 10, 2026 08:09
@wxiaoguang wxiaoguang force-pushed the fix-catfile-flaky-test branch 2 times, most recently from c07f823 to f9d8bab Compare April 10, 2026 09:39
@wxiaoguang wxiaoguang force-pushed the fix-catfile-flaky-test branch from f9d8bab to f60e52a Compare April 10, 2026 09:40
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 10, 2026
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 10, 2026
@silverwind
Copy link
Copy Markdown
Member Author

Above comment seems like AI spam, but I still checked it and there's nothing addressable in it.

@silverwind silverwind added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 10, 2026
@silverwind silverwind enabled auto-merge (squash) April 10, 2026 17:03
@silverwind silverwind merged commit 09c2677 into go-gitea:main Apr 10, 2026
26 checks passed
@silverwind silverwind deleted the fix-catfile-flaky-test branch April 10, 2026 17:34
@GiteaBot GiteaBot added this to the 1.27.0 milestone Apr 10, 2026
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 10, 2026
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Apr 11, 2026
`TestCatFileBatch/QueryTerminated` relied on timing to distinguish
`os.ErrClosed` vs `io.EOF` error paths. Replace `time.Sleep`-based
synchronization with a channel-based hook on pipe close, making both
error paths fully deterministic regardless of CI runner speed.

Ref: https://github.com/go-gitea/gitea/actions/runs/24193070536/job/70615366804
Co-authored-by: Claude (Opus 4.6) <noreply@anthropic.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@GiteaBot GiteaBot added the backport/done All backports for this PR have been created label Apr 11, 2026
silverwind added a commit that referenced this pull request Apr 11, 2026
Backport #37159 by @silverwind

`TestCatFileBatch/QueryTerminated` relied on timing to distinguish
`os.ErrClosed` vs `io.EOF` error paths. Replace `time.Sleep`-based
synchronization with a channel-based hook on pipe close, making both
error paths fully deterministic regardless of CI runner speed.

Ref:
https://github.com/go-gitea/gitea/actions/runs/24193070536/job/70615366804

---
This PR was written with the help of Claude Opus 4.6

Co-authored-by: silverwind <me@silverwind.io>
Co-authored-by: Claude (Opus 4.6) <noreply@anthropic.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 14, 2026
* main:
  Add comment for the design of "user activity time" (go-gitea#37195)
  fix(api): handle missing base branch in PR commits API (go-gitea#37193)
  Refactor htmx and fetch-action related code (go-gitea#37186)
  Fix encoding for Matrix Webhooks (go-gitea#37190)
  Always show owner/repo name in compare page dropdowns (go-gitea#37172)
  fix(api): handle fork-only commits in compare API (go-gitea#37185)
  Improve Contributing docs and set a release schedule (go-gitea#37109)
  Update Nix flake (go-gitea#37183)
  Remove outdated RunUser logic (go-gitea#37180)
  Refactor flash message and remove SanitizeHTML template func (go-gitea#37179)
  Indicate form field readonly via background (go-gitea#37175)
  Remove dead CSS rules (go-gitea#37173)
  Fix flaky `TestCatFileBatch/QueryTerminated` test (go-gitea#37159)
  Implement logout redirection for reverse proxy auth setups (go-gitea#36085)
  Add missing `//nolint:depguard` (go-gitea#37162)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport/done All backports for this PR have been created backport/v1.26 This PR should be backported to Gitea 1.26 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants