From b3a58506aa1fd852c1deecd638af52ddac671b37 Mon Sep 17 00:00:00 2001 From: anant Date: Sun, 15 Nov 2026 00:44:54 +0530 Subject: [PATCH] filer: report a SQL listing that fails partway through (#12654) * filer: report a SQL listing that fails partway through ListDirectoryPrefixedEntries never checked rows.Err() after its rows.Next() loop. When the result set failed partway, for example on a lost connection, rows.Next() returned false and the listing returned the entries read so far with a nil error, so callers saw a shorter directory instead of an error. Check rows.Err() once the loop has run to the end, as the scan in abstract_sql_store_scan.go already does. A listing the callback stopped on purpose is left alone: rows.Err() would return a canceled request context's error there. This covers every store built on AbstractSqlStore (mysql, mysql2, postgres, postgres2, sqlite). Signed-off-by: anant <170962632+antcybersec@users.noreply.github.com> * filer: wait for the canceled SQL listing to release its rows in the test A fixed sleep let the callback return before database/sql closed the rows, so a listing that always consulted rows.Err could still pass. Wait, bounded, until the connection is back in the pool instead. Co-Authored-By: Claude Opus 5.5 --------- Signed-off-by: anant <170962632+antcybersec@users.noreply.github.com> Co-authored-by: anant <170962632+antcybersec@users.noreply.github.com> Co-authored-by: Chris Lu Co-authored-by: Claude Opus 5.5 --- weed/filer/abstract_sql/abstract_sql_store.go | 11 +++ .../abstract_sql/abstract_sql_store_test.go | 74 +++++++++++++++++++ 2 files changed, 85 insertions(+) diff --git a/weed/filer/abstract_sql/abstract_sql_store.go b/weed/filer/abstract_sql/abstract_sql_store.go index 14a552315d..fdbb7a8adb 100644 --- a/weed/filer/abstract_sql/abstract_sql_store.go +++ b/weed/filer/abstract_sql/abstract_sql_store.go @@ -428,6 +428,7 @@ func (store *AbstractSqlStore) ListDirectoryPrefixedEntries(ctx context.Context, } defer rows.Close() + stoppedEarly := false for rows.Next() { var name string var data []byte @@ -452,10 +453,20 @@ func (store *AbstractSqlStore) ListDirectoryPrefixedEntries(ctx context.Context, } if !resEachEntryFunc { + stoppedEarly = true break } } + // rows.Next also returns false when the result set fails partway, for + // example on a lost connection; that must not read as the end of the + // directory. A listing the callback stopped on purpose has no such error + // to report (rows.Err would return a canceled context's error instead). + if err == nil && !stoppedEarly { + if rowsErr := rows.Err(); rowsErr != nil { + err = fmt.Errorf("list %s: %w", dirPath, rowsErr) + } + } return lastFileName, err } diff --git a/weed/filer/abstract_sql/abstract_sql_store_test.go b/weed/filer/abstract_sql/abstract_sql_store_test.go index 027c5b308e..7c2f1ad3d3 100644 --- a/weed/filer/abstract_sql/abstract_sql_store_test.go +++ b/weed/filer/abstract_sql/abstract_sql_store_test.go @@ -2,10 +2,13 @@ package abstract_sql import ( "context" + "errors" "reflect" "testing" + "time" "github.com/DATA-DOG/go-sqlmock" + "github.com/seaweedfs/seaweedfs/weed/filer" "github.com/seaweedfs/seaweedfs/weed/util" ) @@ -140,3 +143,74 @@ func TestBucketHooksSkipInvalidBucketNames(t *testing.T) { t.Error(err) } } + +type listGen struct{ SqlGenerator } + +func (listGen) GetSqlListExclusive(string) string { return "SELECT name, meta" } +func (listGen) GetSqlListInclusive(string) string { return "SELECT name, meta" } + +// A listing that loses its connection partway through must report the error, +// not return the rows read so far as if the directory ended there. +func TestListDirectoryEntriesReportsRowIterationError(t *testing.T) { + db, mock, err := sqlmock.New() + if err != nil { + t.Fatal(err) + } + defer db.Close() + + meta, err := (&filer.Entry{}).EncodeAttributesAndChunks() + if err != nil { + t.Fatal(err) + } + connLost := errors.New("connection reset by peer") + mock.ExpectQuery("SELECT name, meta").WillReturnRows( + sqlmock.NewRows([]string{"name", "meta"}). + AddRow("a.txt", meta). + AddRow("b.txt", meta). + RowError(1, connLost)) + + store := &AbstractSqlStore{SqlGenerator: listGen{}, DB: db} + var listed []string + _, err = store.ListDirectoryEntries(context.Background(), "/dir", "", true, 100, func(entry *filer.Entry) (bool, error) { + listed = append(listed, entry.Name()) + return true, nil + }) + if !errors.Is(err, connLost) { + t.Fatalf("ListDirectoryEntries error = %v, want %v (listed %v)", err, connLost, listed) + } +} + +// A listing the callback stops on purpose is not a failure, even if the +// request context is canceled before the rows are closed. +func TestListDirectoryEntriesStoppedByCallbackWithCanceledContext(t *testing.T) { + db, mock, err := sqlmock.New() + if err != nil { + t.Fatal(err) + } + defer db.Close() + + meta, err := (&filer.Entry{}).EncodeAttributesAndChunks() + if err != nil { + t.Fatal(err) + } + mock.ExpectQuery("SELECT name, meta").WillReturnRows( + sqlmock.NewRows([]string{"name", "meta"}).AddRow("a.txt", meta).AddRow("b.txt", meta)) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + store := &AbstractSqlStore{SqlGenerator: listGen{}, DB: db} + _, err = store.ListDirectoryEntries(ctx, "/dir", "", true, 100, func(entry *filer.Entry) (bool, error) { + cancel() + // database/sql closes the rows on a background goroutine; once the + // connection is back in the pool, rows.Err reports the cancellation. + for deadline := time.Now().Add(5 * time.Second); db.Stats().InUse > 0; time.Sleep(time.Millisecond) { + if time.Now().After(deadline) { + t.Fatal("the canceled listing still holds its connection") + } + } + return false, nil + }) + if err != nil { + t.Fatalf("ListDirectoryEntries error = %v, want nil", err) + } +}