From 75d9e47ed31e4e19aaf60d8080de271f6215a211 Mon Sep 17 00:00:00 2001 From: silverwind Date: Thu, 9 Apr 2026 16:04:06 +0200 Subject: [PATCH 1/5] Fix flaky TestCatFileBatch/QueryTerminated test 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 Co-Authored-By: Claude (Opus 4.6) --- modules/git/catfile_batch_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/git/catfile_batch_test.go b/modules/git/catfile_batch_test.go index 69662ffc1a7c0..d2ea54a6969a2 100644 --- a/modules/git/catfile_batch_test.go +++ b/modules/git/catfile_batch_test.go @@ -72,9 +72,9 @@ func testCatFileBatch(t *testing.T) { } t.Run("QueryTerminated", func(t *testing.T) { - err := simulateQueryTerminated(0, 20*time.Millisecond) + err := simulateQueryTerminated(0, 500*time.Millisecond) assert.ErrorIs(t, err, os.ErrClosed) // pipes are closed faster - err = simulateQueryTerminated(40*time.Millisecond, 20*time.Millisecond) + err = simulateQueryTerminated(500*time.Millisecond, 20*time.Millisecond) assert.ErrorIs(t, err, io.EOF) // reader is faster }) From dabb06ef4f9c5e0cc963644d72e650a7387f9134 Mon Sep 17 00:00:00 2001 From: silverwind Date: Thu, 9 Apr 2026 16:35:09 +0200 Subject: [PATCH 2/5] Replace timing-based sync with channel-based sync Use a function hook and channels instead of time.Sleep delays for deterministic test synchronization. Eliminates all timing dependencies. Co-Authored-By: silverwind Co-Authored-By: Claude (Opus 4.6) --- modules/git/catfile_batch_reader.go | 10 +++--- modules/git/catfile_batch_test.go | 53 +++++++++++++++++------------ 2 files changed, 36 insertions(+), 27 deletions(-) diff --git a/modules/git/catfile_batch_reader.go b/modules/git/catfile_batch_reader.go index 0c8fc740bee29..92b5e6b1c09a5 100644 --- a/modules/git/catfile_batch_reader.go +++ b/modules/git/catfile_batch_reader.go @@ -13,13 +13,12 @@ import ( "strconv" "strings" "sync/atomic" - "time" "code.gitea.io/gitea/modules/git/gitcmd" "code.gitea.io/gitea/modules/log" ) -var catFileBatchDebugWaitClose atomic.Int64 +var catFileBatchDebugPipeClose atomic.Pointer[func(stdPipeClose func())] type catFileBatchCommunicator struct { closeFunc func(err error) @@ -42,10 +41,11 @@ func newCatFileBatch(ctx context.Context, repoPath string, cmdCatFile *gitcmd.Co // We often want to feed the commits in order into cat-file --batch, followed by their trees and subtrees as necessary. stdinWriter, stdoutReader, stdPipeClose := cmdCatFile.MakeStdinStdoutPipe() pipeClose := func() { - if delay := catFileBatchDebugWaitClose.Load(); delay > 0 { - time.Sleep(time.Duration(delay)) // for testing purpose only + if fn := catFileBatchDebugPipeClose.Load(); fn != nil { + (*fn)(stdPipeClose) // for testing purpose only + } else { + stdPipeClose() } - stdPipeClose() } closeFunc := func(err error) { ctxCancel(err) diff --git a/modules/git/catfile_batch_test.go b/modules/git/catfile_batch_test.go index d2ea54a6969a2..8ad260a8a76c2 100644 --- a/modules/git/catfile_batch_test.go +++ b/modules/git/catfile_batch_test.go @@ -7,9 +7,7 @@ import ( "io" "os" "path/filepath" - "sync" "testing" - "time" "code.gitea.io/gitea/modules/test" @@ -39,12 +37,10 @@ func testCatFileBatch(t *testing.T) { require.Error(t, err) }) - simulateQueryTerminated := func(pipeCloseDelay, pipeReadDelay time.Duration) (errRead error) { - catFileBatchDebugWaitClose.Store(int64(pipeCloseDelay)) - defer catFileBatchDebugWaitClose.Store(0) + setupTerminatedBatch := func(t *testing.T) (*catFileBatchCommunicator, func()) { + t.Helper() batch, err := NewBatch(t.Context(), filepath.Join(testReposDir, "repo1_bare")) require.NoError(t, err) - defer batch.Close() _, _ = batch.QueryInfo("e2129701f1a4d54dc44f03c93bca0a2aec7c5449") var c *catFileBatchCommunicator switch b := batch.(type) { @@ -57,25 +53,38 @@ func testCatFileBatch(t *testing.T) { default: t.FailNow() } - - wg := sync.WaitGroup{} - wg.Go(func() { - time.Sleep(pipeReadDelay) - var n int - n, errRead = c.respReader.Read(make([]byte, 100)) - assert.Zero(t, n) - }) - time.Sleep(10 * time.Millisecond) - c.debugGitCmd.DebugKill() - wg.Wait() - return errRead + return c, func() { batch.Close() } } t.Run("QueryTerminated", func(t *testing.T) { - err := simulateQueryTerminated(0, 500*time.Millisecond) - assert.ErrorIs(t, err, os.ErrClosed) // pipes are closed faster - err = simulateQueryTerminated(500*time.Millisecond, 20*time.Millisecond) - assert.ErrorIs(t, err, io.EOF) // reader is faster + t.Run("PipeClosedBeforeRead", func(t *testing.T) { + closed := make(chan struct{}) + hook := func(doClose func()) { doClose(); close(closed) } + c, cleanup := setupTerminatedBatch(t) + defer cleanup() + catFileBatchDebugPipeClose.Store(&hook) + defer catFileBatchDebugPipeClose.Store(nil) + c.debugGitCmd.DebugKill() + <-closed + n, err := c.respReader.Read(make([]byte, 100)) + assert.Zero(t, n) + assert.ErrorIs(t, err, os.ErrClosed) + }) + t.Run("ReadBeforePipeClose", func(t *testing.T) { + ready := make(chan struct{}) + proceed := make(chan struct{}) + hook := func(doClose func()) { close(ready); <-proceed; doClose() } + c, cleanup := setupTerminatedBatch(t) + defer cleanup() + catFileBatchDebugPipeClose.Store(&hook) + defer catFileBatchDebugPipeClose.Store(nil) + c.debugGitCmd.DebugKill() + <-ready + n, err := c.respReader.Read(make([]byte, 100)) + assert.Zero(t, n) + assert.ErrorIs(t, err, io.EOF) + close(proceed) + }) }) batch, err := NewBatch(t.Context(), filepath.Join(testReposDir, "repo1_bare")) From 2912174cb65c15bc2673f8d2c9df6616ce74d6d6 Mon Sep 17 00:00:00 2001 From: silverwind Date: Thu, 9 Apr 2026 16:37:58 +0200 Subject: [PATCH 3/5] Remove redundant comment Co-Authored-By: silverwind Co-Authored-By: Claude (Opus 4.6) --- modules/git/catfile_batch_reader.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/git/catfile_batch_reader.go b/modules/git/catfile_batch_reader.go index 92b5e6b1c09a5..d772ac05d79d5 100644 --- a/modules/git/catfile_batch_reader.go +++ b/modules/git/catfile_batch_reader.go @@ -42,7 +42,7 @@ func newCatFileBatch(ctx context.Context, repoPath string, cmdCatFile *gitcmd.Co stdinWriter, stdoutReader, stdPipeClose := cmdCatFile.MakeStdinStdoutPipe() pipeClose := func() { if fn := catFileBatchDebugPipeClose.Load(); fn != nil { - (*fn)(stdPipeClose) // for testing purpose only + (*fn)(stdPipeClose) } else { stdPipeClose() } From 8d4453ed8455c86aa9df345d67475179ff949878 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 10 Apr 2026 15:50:53 +0800 Subject: [PATCH 4/5] fix --- modules/git/catfile_batch_reader.go | 48 +++++++++++++++--------- modules/git/catfile_batch_test.go | 58 +++++++++++++---------------- 2 files changed, 55 insertions(+), 51 deletions(-) diff --git a/modules/git/catfile_batch_reader.go b/modules/git/catfile_batch_reader.go index d772ac05d79d5..85807b8cf9110 100644 --- a/modules/git/catfile_batch_reader.go +++ b/modules/git/catfile_batch_reader.go @@ -16,40 +16,52 @@ import ( "code.gitea.io/gitea/modules/git/gitcmd" "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/util" ) -var catFileBatchDebugPipeClose atomic.Pointer[func(stdPipeClose func())] - type catFileBatchCommunicator struct { - closeFunc func(err error) + closeFunc atomic.Pointer[func(err error)] reqWriter io.Writer respReader *bufio.Reader debugGitCmd *gitcmd.Command } -func (b *catFileBatchCommunicator) Close() { - if b.closeFunc != nil { - b.closeFunc(nil) - b.closeFunc = nil +func (b *catFileBatchCommunicator) Close(err ...error) { + if fn := b.closeFunc.Load(); fn != nil { + (*fn)(util.OptionalArg(err)) + b.closeFunc.Store(nil) } } +func (b *catFileBatchCommunicator) debugKill() (ret struct { + beforeClose chan struct{} + blockClose chan struct{} + afterClose chan struct{} +}) { + ret.beforeClose = make(chan struct{}) + ret.blockClose = make(chan struct{}) + ret.afterClose = make(chan struct{}) + oldCloseFunc := b.closeFunc.Load() + b.closeFunc.Store(new(func(err error) { + b.closeFunc.Store(oldCloseFunc) + close(ret.beforeClose) + <-ret.blockClose + (*oldCloseFunc)(err) + close(ret.afterClose) + })) + b.debugGitCmd.DebugKill() + return ret +} + // newCatFileBatch opens git cat-file --batch in the provided repo and returns a stdin pipe, a stdout reader and cancel function func newCatFileBatch(ctx context.Context, repoPath string, cmdCatFile *gitcmd.Command) (ret *catFileBatchCommunicator) { ctx, ctxCancel := context.WithCancelCause(ctx) // We often want to feed the commits in order into cat-file --batch, followed by their trees and subtrees as necessary. stdinWriter, stdoutReader, stdPipeClose := cmdCatFile.MakeStdinStdoutPipe() - pipeClose := func() { - if fn := catFileBatchDebugPipeClose.Load(); fn != nil { - (*fn)(stdPipeClose) - } else { - stdPipeClose() - } - } closeFunc := func(err error) { ctxCancel(err) - pipeClose() + stdPipeClose() } return newCatFileBatchWithCloseFunc(ctx, repoPath, cmdCatFile, stdinWriter, stdoutReader, closeFunc) } @@ -59,17 +71,17 @@ func newCatFileBatchWithCloseFunc(ctx context.Context, repoPath string, cmdCatFi ) *catFileBatchCommunicator { ret := &catFileBatchCommunicator{ debugGitCmd: cmdCatFile, - closeFunc: closeFunc, reqWriter: stdinWriter, respReader: bufio.NewReaderSize(stdoutReader, 32*1024), // use a buffered reader for rich operations } + ret.closeFunc.Store(&closeFunc) err := cmdCatFile.WithDir(repoPath).StartWithStderr(ctx) if err != nil { log.Error("Unable to start git command %v: %v", cmdCatFile.LogString(), err) // ideally here it should return the error, but it would require refactoring all callers // so just return a dummy communicator that does nothing, almost the same behavior as before, not bad - closeFunc(err) + ret.Close(err) return ret } @@ -78,7 +90,7 @@ func newCatFileBatchWithCloseFunc(ctx context.Context, repoPath string, cmdCatFi if err != nil && !errors.Is(err, context.Canceled) { log.Error("cat-file --batch command failed in repo %s, error: %v", repoPath, err) } - closeFunc(err) + ret.Close(err) }() return ret diff --git a/modules/git/catfile_batch_test.go b/modules/git/catfile_batch_test.go index 8ad260a8a76c2..228c949c64cee 100644 --- a/modules/git/catfile_batch_test.go +++ b/modules/git/catfile_batch_test.go @@ -37,11 +37,21 @@ func testCatFileBatch(t *testing.T) { require.Error(t, err) }) - setupTerminatedBatch := func(t *testing.T) (*catFileBatchCommunicator, func()) { - t.Helper() + simulateQueryTerminated := func(t *testing.T, errBeforePipeClose, errAfterPipeClose error) { + readError := func(t *testing.T, r io.Reader, expectedErr error) { + if expectedErr == nil { + return // expectedErr == nil means this read should be skipped + } + n, err := r.Read(make([]byte, 100)) + assert.Zero(t, n) + assert.ErrorIs(t, err, expectedErr) + } + batch, err := NewBatch(t.Context(), filepath.Join(testReposDir, "repo1_bare")) require.NoError(t, err) - _, _ = batch.QueryInfo("e2129701f1a4d54dc44f03c93bca0a2aec7c5449") + _, err = batch.QueryInfo("e2129701f1a4d54dc44f03c93bca0a2aec7c5449") + require.NoError(t, err) + var c *catFileBatchCommunicator switch b := batch.(type) { case *catFileBatchLegacy: @@ -53,38 +63,20 @@ func testCatFileBatch(t *testing.T) { default: t.FailNow() } - return c, func() { batch.Close() } - } + defer c.Close() + + require.True(t, (errBeforePipeClose != nil) != (errAfterPipeClose != nil), "must set exactly one of the expected errors") + inceptor := c.debugKill() + <-inceptor.beforeClose // wait for the command's Close to be called, the pipe is not closed yet + readError(t, c.respReader, errBeforePipeClose) // then caller will read on an open pipe which will be closed soon + close(inceptor.blockClose) // continue to close the pipe + <-inceptor.afterClose // wait for the pipe to be closed + readError(t, c.respReader, errAfterPipeClose) // then caller will read on a closed pipe + } t.Run("QueryTerminated", func(t *testing.T) { - t.Run("PipeClosedBeforeRead", func(t *testing.T) { - closed := make(chan struct{}) - hook := func(doClose func()) { doClose(); close(closed) } - c, cleanup := setupTerminatedBatch(t) - defer cleanup() - catFileBatchDebugPipeClose.Store(&hook) - defer catFileBatchDebugPipeClose.Store(nil) - c.debugGitCmd.DebugKill() - <-closed - n, err := c.respReader.Read(make([]byte, 100)) - assert.Zero(t, n) - assert.ErrorIs(t, err, os.ErrClosed) - }) - t.Run("ReadBeforePipeClose", func(t *testing.T) { - ready := make(chan struct{}) - proceed := make(chan struct{}) - hook := func(doClose func()) { close(ready); <-proceed; doClose() } - c, cleanup := setupTerminatedBatch(t) - defer cleanup() - catFileBatchDebugPipeClose.Store(&hook) - defer catFileBatchDebugPipeClose.Store(nil) - c.debugGitCmd.DebugKill() - <-ready - n, err := c.respReader.Read(make([]byte, 100)) - assert.Zero(t, n) - assert.ErrorIs(t, err, io.EOF) - close(proceed) - }) + simulateQueryTerminated(t, io.EOF, nil) // reader is faster + simulateQueryTerminated(t, nil, os.ErrClosed) // pipes are closed faster }) batch, err := NewBatch(t.Context(), filepath.Join(testReposDir, "repo1_bare")) From f60e52a6464987e7b9d5e19d20ce9be3bd993968 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 10 Apr 2026 17:36:52 +0800 Subject: [PATCH 5/5] fix lint --- modules/git/catfile_batch_reader.go | 65 +++++++++++++---------------- modules/git/catfile_batch_test.go | 4 +- 2 files changed, 30 insertions(+), 39 deletions(-) diff --git a/modules/git/catfile_batch_reader.go b/modules/git/catfile_batch_reader.go index 85807b8cf9110..5727c4a8ac3d4 100644 --- a/modules/git/catfile_batch_reader.go +++ b/modules/git/catfile_batch_reader.go @@ -27,54 +27,24 @@ type catFileBatchCommunicator struct { } func (b *catFileBatchCommunicator) Close(err ...error) { - if fn := b.closeFunc.Load(); fn != nil { + if fn := b.closeFunc.Swap(nil); fn != nil { (*fn)(util.OptionalArg(err)) - b.closeFunc.Store(nil) } } -func (b *catFileBatchCommunicator) debugKill() (ret struct { - beforeClose chan struct{} - blockClose chan struct{} - afterClose chan struct{} -}) { - ret.beforeClose = make(chan struct{}) - ret.blockClose = make(chan struct{}) - ret.afterClose = make(chan struct{}) - oldCloseFunc := b.closeFunc.Load() - b.closeFunc.Store(new(func(err error) { - b.closeFunc.Store(oldCloseFunc) - close(ret.beforeClose) - <-ret.blockClose - (*oldCloseFunc)(err) - close(ret.afterClose) - })) - b.debugGitCmd.DebugKill() - return ret -} - -// newCatFileBatch opens git cat-file --batch in the provided repo and returns a stdin pipe, a stdout reader and cancel function -func newCatFileBatch(ctx context.Context, repoPath string, cmdCatFile *gitcmd.Command) (ret *catFileBatchCommunicator) { +// newCatFileBatch opens git cat-file --batch/--batch-check/--batch-command command and prepares the stdin/stdout pipes for communication. +func newCatFileBatch(ctx context.Context, repoPath string, cmdCatFile *gitcmd.Command) *catFileBatchCommunicator { ctx, ctxCancel := context.WithCancelCause(ctx) - - // We often want to feed the commits in order into cat-file --batch, followed by their trees and subtrees as necessary. stdinWriter, stdoutReader, stdPipeClose := cmdCatFile.MakeStdinStdoutPipe() - closeFunc := func(err error) { - ctxCancel(err) - stdPipeClose() - } - return newCatFileBatchWithCloseFunc(ctx, repoPath, cmdCatFile, stdinWriter, stdoutReader, closeFunc) -} - -func newCatFileBatchWithCloseFunc(ctx context.Context, repoPath string, cmdCatFile *gitcmd.Command, - stdinWriter gitcmd.PipeWriter, stdoutReader gitcmd.PipeReader, closeFunc func(err error), -) *catFileBatchCommunicator { ret := &catFileBatchCommunicator{ debugGitCmd: cmdCatFile, reqWriter: stdinWriter, respReader: bufio.NewReaderSize(stdoutReader, 32*1024), // use a buffered reader for rich operations } - ret.closeFunc.Store(&closeFunc) + ret.closeFunc.Store(new(func(err error) { + ctxCancel(err) + stdPipeClose() + })) err := cmdCatFile.WithDir(repoPath).StartWithStderr(ctx) if err != nil { @@ -96,6 +66,27 @@ func newCatFileBatchWithCloseFunc(ctx context.Context, repoPath string, cmdCatFi return ret } +func (b *catFileBatchCommunicator) debugKill() (ret struct { + beforeClose chan struct{} + blockClose chan struct{} + afterClose chan struct{} +}, +) { + ret.beforeClose = make(chan struct{}) + ret.blockClose = make(chan struct{}) + ret.afterClose = make(chan struct{}) + oldCloseFunc := b.closeFunc.Load() + b.closeFunc.Store(new(func(err error) { + b.closeFunc.Store(nil) + close(ret.beforeClose) + <-ret.blockClose + (*oldCloseFunc)(err) + close(ret.afterClose) + })) + b.debugGitCmd.DebugKill() + return ret +} + // catFileBatchParseInfoLine reads the header line from cat-file --batch // We expect: SP SP LF // then leaving the rest of the stream " LF" to be read diff --git a/modules/git/catfile_batch_test.go b/modules/git/catfile_batch_test.go index 228c949c64cee..782d34d2493ca 100644 --- a/modules/git/catfile_batch_test.go +++ b/modules/git/catfile_batch_test.go @@ -49,6 +49,7 @@ func testCatFileBatch(t *testing.T) { batch, err := NewBatch(t.Context(), filepath.Join(testReposDir, "repo1_bare")) require.NoError(t, err) + defer batch.Close() _, err = batch.QueryInfo("e2129701f1a4d54dc44f03c93bca0a2aec7c5449") require.NoError(t, err) @@ -63,9 +64,8 @@ func testCatFileBatch(t *testing.T) { default: t.FailNow() } - defer c.Close() - require.True(t, (errBeforePipeClose != nil) != (errAfterPipeClose != nil), "must set exactly one of the expected errors") + require.NotEqual(t, errBeforePipeClose == nil, errAfterPipeClose == nil, "must set exactly one of the expected errors") inceptor := c.debugKill() <-inceptor.beforeClose // wait for the command's Close to be called, the pipe is not closed yet