From cb0666a6190a2b76fd55bb88c6eef61b71ccb78d Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 6 Mar 2026 15:19:03 +0800 Subject: [PATCH 1/4] fix-dbfs --- models/dbfs/dbfile.go | 20 +++++++++----------- models/dbfs/dbfs_test.go | 31 +++++++++++++++++++------------ modules/actions/log.go | 6 +++--- 3 files changed, 31 insertions(+), 26 deletions(-) diff --git a/models/dbfs/dbfile.go b/models/dbfs/dbfile.go index ccb13583e1377..582579f9824ab 100644 --- a/models/dbfs/dbfile.go +++ b/models/dbfs/dbfile.go @@ -75,7 +75,7 @@ func (f *file) readAt(fileMeta *dbfsMeta, offset int64, p []byte) (n int, err er } func (f *file) Read(p []byte) (n int, err error) { - if f.metaID == 0 || !f.allowRead { + if !f.allowRead { return 0, os.ErrInvalid } @@ -89,7 +89,7 @@ func (f *file) Read(p []byte) (n int, err error) { } func (f *file) Write(p []byte) (n int, err error) { - if f.metaID == 0 || !f.allowWrite { + if !f.allowWrite { return 0, os.ErrInvalid } @@ -184,10 +184,6 @@ func (f *file) Close() error { } func (f *file) Stat() (os.FileInfo, error) { - if f.metaID == 0 { - return nil, os.ErrInvalid - } - fileMeta, err := findFileMetaByID(f.ctx, f.metaID) if err != nil { return nil, err @@ -240,6 +236,11 @@ func (f *file) open(flag int) (err error) { } } } + } else /* no O_CREATE flag */ { + // file must exist. + if f.metaID == 0 { + return os.ErrNotExist + } } if flag&os.O_TRUNC != 0 { if err = f.truncate(); err != nil { @@ -252,7 +253,7 @@ func (f *file) open(flag int) (err error) { } } return nil - } + } // end if: allowWrite // read only mode if f.metaID == 0 { @@ -322,9 +323,6 @@ func (f *file) delete() error { } func (f *file) size() (int64, error) { - if f.metaID == 0 { - return 0, os.ErrNotExist - } fileMeta, err := findFileMetaByID(f.ctx, f.metaID) if err != nil { return 0, err @@ -339,7 +337,7 @@ func findFileMetaByID(ctx context.Context, metaID int64) (*dbfsMeta, error) { } else if ok { return &fileMeta, nil } - return nil, nil //nolint:nilnil // return nil to indicate that the object does not exist + return nil, os.ErrNotExist } func buildPath(path string) string { diff --git a/models/dbfs/dbfs_test.go b/models/dbfs/dbfs_test.go index e1ecd871e4d71..b4deb9ccaf1bb 100644 --- a/models/dbfs/dbfs_test.go +++ b/models/dbfs/dbfs_test.go @@ -9,22 +9,29 @@ import ( "os" "testing" + "code.gitea.io/gitea/modules/test" + "github.com/stretchr/testify/assert" ) -func changeDefaultFileBlockSize(n int64) (restore func()) { - old := defaultFileBlockSize - defaultFileBlockSize = n - return func() { - defaultFileBlockSize = old - } -} - func TestDbfsBasic(t *testing.T) { - defer changeDefaultFileBlockSize(4)() + defer test.MockVariableValue(&defaultFileBlockSize, 4)() + + // test non-existing + f, err := OpenFile(t.Context(), "test.txt", os.O_RDONLY) + assert.ErrorIs(t, err, os.ErrNotExist) + assert.Nil(t, f) + + f, err = OpenFile(t.Context(), "test.txt", os.O_WRONLY) + assert.ErrorIs(t, err, os.ErrNotExist) + assert.Nil(t, f) + + f, err = OpenFile(t.Context(), "test.txt", os.O_WRONLY|os.O_APPEND|os.O_TRUNC) + assert.ErrorIs(t, err, os.ErrNotExist) + assert.Nil(t, f) // test basic write/read - f, err := OpenFile(t.Context(), "test.txt", os.O_RDWR|os.O_CREATE) + f, err = OpenFile(t.Context(), "test.txt", os.O_RDWR|os.O_CREATE) assert.NoError(t, err) n, err := f.Write([]byte("0123456789")) // blocks: 0123 4567 89 @@ -125,7 +132,7 @@ func TestDbfsBasic(t *testing.T) { } func TestDbfsReadWrite(t *testing.T) { - defer changeDefaultFileBlockSize(4)() + defer test.MockVariableValue(&defaultFileBlockSize, 4)() f1, err := OpenFile(t.Context(), "test.log", os.O_RDWR|os.O_CREATE) assert.NoError(t, err) @@ -157,7 +164,7 @@ func TestDbfsReadWrite(t *testing.T) { } func TestDbfsSeekWrite(t *testing.T) { - defer changeDefaultFileBlockSize(4)() + defer test.MockVariableValue(&defaultFileBlockSize, 4)() f, err := OpenFile(t.Context(), "test2.log", os.O_RDWR|os.O_CREATE) assert.NoError(t, err) diff --git a/modules/actions/log.go b/modules/actions/log.go index 5a1425e031750..fff9eb074a818 100644 --- a/modules/actions/log.go +++ b/modules/actions/log.go @@ -41,13 +41,13 @@ func WriteLogs(ctx context.Context, filename string, offset int64, rows []*runne name := DBFSPrefix + filename f, err := dbfs.OpenFile(ctx, name, flag) if err != nil { - return nil, fmt.Errorf("dbfs OpenFile %q: %w", name, err) + return nil, fmt.Errorf("dbfs.OpenFile %q (flag:0x%x,offset:%d): %w", name, flag, offset, err) } defer f.Close() stat, err := f.Stat() if err != nil { - return nil, fmt.Errorf("dbfs Stat %q: %w", name, err) + return nil, fmt.Errorf("dbfs.Stat %q: %w", name, err) } if stat.Size() < offset { // If the size is less than offset, refuse to write, or it could result in content holes. @@ -56,7 +56,7 @@ func WriteLogs(ctx context.Context, filename string, offset int64, rows []*runne } if _, err := f.Seek(offset, io.SeekStart); err != nil { - return nil, fmt.Errorf("dbfs Seek %q: %w", name, err) + return nil, fmt.Errorf("dbfs.Seek %q: %w", name, err) } writer := bufio.NewWriterSize(f, defaultBufSize) From e34aa72662f26d88734f95b0aff898e88230443c Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 6 Mar 2026 16:27:13 +0800 Subject: [PATCH 2/4] fine tune comment and error message --- modules/actions/log.go | 18 ++++++++++-------- routers/api/actions/runner/runner.go | 2 +- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/modules/actions/log.go b/modules/actions/log.go index fff9eb074a818..3fb56b402abaf 100644 --- a/modules/actions/log.go +++ b/modules/actions/log.go @@ -33,15 +33,16 @@ const ( // It doesn't respect the file format in the filename like ".zst", since it's difficult to reopen a closed compressed file and append new content. // Why doesn't it store logs in object storage directly? Because it's not efficient to append content to object storage. func WriteLogs(ctx context.Context, filename string, offset int64, rows []*runnerv1.LogRow) ([]int, error) { - flag := os.O_WRONLY + flag, openFileFor := os.O_WRONLY, "write-only" if offset == 0 { - // Create file only if offset is 0, or it could result in content holes if the file doesn't exist. - flag |= os.O_CREATE + // Only allow to create file if offset is 0 (the first write), see #25560. + // Otherwise, it might result in content holes if the file has been deleted after transferred (actions.TransferLogs). + flag, openFileFor = os.O_WRONLY|os.O_CREATE, "write-create" } name := DBFSPrefix + filename f, err := dbfs.OpenFile(ctx, name, flag) if err != nil { - return nil, fmt.Errorf("dbfs.OpenFile %q (flag:0x%x,offset:%d): %w", name, flag, offset, err) + return nil, fmt.Errorf("dbfs.OpenFile %q for %s: %w", name, openFileFor, err) } defer f.Close() @@ -121,16 +122,17 @@ const ( // TransferLogs transfers logs from DBFS to object storage. // It happens when the file is complete and no more logs will be appended. // It respects the file format in the filename like ".zst", and compresses the content if needed. +// The task log file must be marked as "log_in_storage=true" after the transfer. func TransferLogs(ctx context.Context, filename string) (func(), error) { name := DBFSPrefix + filename remove := func() { if err := dbfs.Remove(ctx, name); err != nil { - log.Warn("dbfs remove %q: %v", name, err) + log.Warn("dbfs.Remove %q: %v", name, err) } } f, err := dbfs.Open(ctx, name) if err != nil { - return nil, fmt.Errorf("dbfs open %q: %w", name, err) + return nil, fmt.Errorf("dbfs.Open %q: %w", name, err) } defer f.Close() @@ -164,7 +166,7 @@ func RemoveLogs(ctx context.Context, inStorage bool, filename string) error { name := DBFSPrefix + filename err := dbfs.Remove(ctx, name) if err != nil { - return fmt.Errorf("dbfs remove %q: %w", name, err) + return fmt.Errorf("dbfs.Remove %q: %w", name, err) } return nil } @@ -180,7 +182,7 @@ func OpenLogs(ctx context.Context, inStorage bool, filename string) (io.ReadSeek name := DBFSPrefix + filename f, err := dbfs.Open(ctx, name) if err != nil { - return nil, fmt.Errorf("dbfs open %q: %w", name, err) + return nil, fmt.Errorf("dbfs.Open %q: %w", name, err) } return f, nil } diff --git a/routers/api/actions/runner/runner.go b/routers/api/actions/runner/runner.go index 86bab4b340c27..49d1b13262209 100644 --- a/routers/api/actions/runner/runner.go +++ b/routers/api/actions/runner/runner.go @@ -270,7 +270,7 @@ func (s *Service) UpdateLog( rows := req.Msg.Rows[ack-req.Msg.Index:] ns, err := actions.WriteLogs(ctx, task.LogFilename, task.LogSize, rows) if err != nil { - return nil, status.Errorf(codes.Internal, "write logs: %v", err) + return nil, status.Errorf(codes.Internal, "unable to append logs to dbfs file: %v", err) } task.LogLength += int64(len(rows)) for _, n := range ns { From 6218e0d99d6a514746fd11eb5098cea641684a9a Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 6 Mar 2026 17:47:27 +0800 Subject: [PATCH 3/4] cover more edge cases, add more tests --- models/dbfs/dbfile.go | 17 +++++------- models/dbfs/dbfs.go | 3 ++ models/dbfs/dbfs_test.go | 59 ++++++++++++++++++++++++++++++---------- 3 files changed, 55 insertions(+), 24 deletions(-) diff --git a/models/dbfs/dbfile.go b/models/dbfs/dbfile.go index 582579f9824ab..a6981cb7d6a33 100644 --- a/models/dbfs/dbfile.go +++ b/models/dbfs/dbfile.go @@ -228,20 +228,17 @@ func (f *file) open(flag int) (err error) { if f.metaID != 0 { return os.ErrExist } - } else { - // create a new file if none exists. - if f.metaID == 0 { - if err = f.createEmpty(); err != nil { - return err - } - } } - } else /* no O_CREATE flag */ { - // file must exist. + // create a new file if not exists. if f.metaID == 0 { - return os.ErrNotExist + if err = f.createEmpty(); err != nil { + return err + } } } + if f.metaID == 0 { + return os.ErrNotExist + } if flag&os.O_TRUNC != 0 { if err = f.truncate(); err != nil { return err diff --git a/models/dbfs/dbfs.go b/models/dbfs/dbfs.go index f68b4a2b70b48..3f768b5339d50 100644 --- a/models/dbfs/dbfs.go +++ b/models/dbfs/dbfs.go @@ -40,6 +40,9 @@ The DBFS solution: * In the future, when Gitea action needs to limit the log size (other CI/CD services also do so), it's easier to calculate the log file size. * Even sometimes the UI needs to render the tailing lines, the tailing lines can be found be counting the "\n" from the end of the file by seek. The seeking and finding is not the fastest way, but it's still acceptable and won't affect the performance too much. + +Limitations of the DBFS solution: +* Not fully POSIX-compliant, some behaviors may be different from the real filesystem, especially for concurrent read/write */ type dbfsMeta struct { diff --git a/models/dbfs/dbfs_test.go b/models/dbfs/dbfs_test.go index b4deb9ccaf1bb..f70fe70ee050d 100644 --- a/models/dbfs/dbfs_test.go +++ b/models/dbfs/dbfs_test.go @@ -10,6 +10,7 @@ import ( "testing" "code.gitea.io/gitea/modules/test" + "github.com/stretchr/testify/require" "github.com/stretchr/testify/assert" ) @@ -17,21 +18,8 @@ import ( func TestDbfsBasic(t *testing.T) { defer test.MockVariableValue(&defaultFileBlockSize, 4)() - // test non-existing - f, err := OpenFile(t.Context(), "test.txt", os.O_RDONLY) - assert.ErrorIs(t, err, os.ErrNotExist) - assert.Nil(t, f) - - f, err = OpenFile(t.Context(), "test.txt", os.O_WRONLY) - assert.ErrorIs(t, err, os.ErrNotExist) - assert.Nil(t, f) - - f, err = OpenFile(t.Context(), "test.txt", os.O_WRONLY|os.O_APPEND|os.O_TRUNC) - assert.ErrorIs(t, err, os.ErrNotExist) - assert.Nil(t, f) - // test basic write/read - f, err = OpenFile(t.Context(), "test.txt", os.O_RDWR|os.O_CREATE) + f, err := OpenFile(t.Context(), "test.txt", os.O_RDWR|os.O_CREATE) assert.NoError(t, err) n, err := f.Write([]byte("0123456789")) // blocks: 0123 4567 89 @@ -129,6 +117,49 @@ func TestDbfsBasic(t *testing.T) { stat, err = f.Stat() assert.NoError(t, err) assert.EqualValues(t, 10, stat.Size()) + + t.Run("NonExisting", func(t *testing.T) { + f, err := OpenFile(t.Context(), "non-existing.txt", os.O_RDONLY) + assert.ErrorIs(t, err, os.ErrNotExist) + assert.Nil(t, f) + + f, err = OpenFile(t.Context(), "non-existing.txt", os.O_WRONLY) + assert.ErrorIs(t, err, os.ErrNotExist) + assert.Nil(t, f) + + f, err = OpenFile(t.Context(), "non-existing.txt", os.O_WRONLY|os.O_APPEND|os.O_TRUNC) + assert.ErrorIs(t, err, os.ErrNotExist) + assert.Nil(t, f) + }) + + t.Run("Existing", func(t *testing.T) { + assertFileContent := func(f File, expected string) { + _, _ = f.Seek(0, io.SeekStart) + buf, _ := io.ReadAll(f) + assert.Equal(t, expected, string(buf)) + } + + f, err := OpenFile(t.Context(), "existing.txt", os.O_RDWR|os.O_CREATE) + require.NoError(t, err) + _, _ = f.Write([]byte("test")) + assertFileContent(f, "test") + assert.NoError(t, f.Close()) + + f, err = OpenFile(t.Context(), "existing.txt", os.O_RDWR|os.O_CREATE|os.O_APPEND) + require.NoError(t, err) + _, _ = f.Write([]byte("\nnew")) + assertFileContent(f, "test\nnew") + assert.NoError(t, f.Close()) + + f, err = OpenFile(t.Context(), "existing.txt", os.O_RDWR|os.O_TRUNC) + require.NoError(t, err) + assertFileContent(f, "") + assert.NoError(t, f.Close()) + + f, err = OpenFile(t.Context(), "existing.txt", os.O_RDWR|os.O_CREATE|os.O_EXCL) + assert.ErrorIs(t, err, os.ErrExist) + assert.Nil(t, f) + }) } func TestDbfsReadWrite(t *testing.T) { From 7cece4f75e489648201bc58d4bae7cf6ce412eb8 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 6 Mar 2026 17:55:00 +0800 Subject: [PATCH 4/4] fix test --- models/dbfs/dbfs_test.go | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/models/dbfs/dbfs_test.go b/models/dbfs/dbfs_test.go index f70fe70ee050d..ca57ebe171a23 100644 --- a/models/dbfs/dbfs_test.go +++ b/models/dbfs/dbfs_test.go @@ -10,9 +10,9 @@ import ( "testing" "code.gitea.io/gitea/modules/test" - "github.com/stretchr/testify/require" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestDbfsBasic(t *testing.T) { @@ -134,8 +134,10 @@ func TestDbfsBasic(t *testing.T) { t.Run("Existing", func(t *testing.T) { assertFileContent := func(f File, expected string) { - _, _ = f.Seek(0, io.SeekStart) - buf, _ := io.ReadAll(f) + _, err := f.Seek(0, io.SeekStart) + require.NoError(t, err) + buf, err := io.ReadAll(f) + require.NoError(t, err) assert.Equal(t, expected, string(buf)) } @@ -197,28 +199,30 @@ func TestDbfsReadWrite(t *testing.T) { func TestDbfsSeekWrite(t *testing.T) { defer test.MockVariableValue(&defaultFileBlockSize, 4)() - f, err := OpenFile(t.Context(), "test2.log", os.O_RDWR|os.O_CREATE) - assert.NoError(t, err) - defer f.Close() + // write something + fw, err := OpenFile(t.Context(), "test2.log", os.O_RDWR|os.O_CREATE) + require.NoError(t, err) + defer fw.Close() - n, err := f.Write([]byte("111")) + n, err := fw.Write([]byte("111")) assert.NoError(t, err) - _, err = f.Seek(int64(n), io.SeekStart) + _, err = fw.Seek(int64(n), io.SeekStart) assert.NoError(t, err) - _, err = f.Write([]byte("222")) + _, err = fw.Write([]byte("222")) assert.NoError(t, err) - _, err = f.Seek(int64(n), io.SeekStart) + _, err = fw.Seek(int64(n), io.SeekStart) assert.NoError(t, err) - _, err = f.Write([]byte("333")) + _, err = fw.Write([]byte("333")) assert.NoError(t, err) + // then read it fr, err := OpenFile(t.Context(), "test2.log", os.O_RDONLY) - assert.NoError(t, err) - defer f.Close() + require.NoError(t, err) + defer fr.Close() buf, err := io.ReadAll(fr) assert.NoError(t, err)