fix: snapshotter's failure is not propogated (backport #16588) (#16603)

Co-authored-by: yihuang <huang@crypto.com>
cosmos · Jun 19, 2023 · 40c4daf · 40c4daf
1 parent de4e55d
commit 40c4daf
Show file tree

Hide file tree

Showing 4 changed files with 50 additions and 3 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -44,6 +44,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 ### Bug Fixes
 
 * (x/auth) [#16554](https://github.com/cosmos/cosmos-sdk/pull/16554) `ModuleAccount.Validate` now reports a nil `.BaseAccount` instead of panicking.
+* [#16588](https://github.com/cosmos/cosmos-sdk/pull/16588) Propogate the Snapshotter's failure to the caller, (it will create a empty snapshot silently before).
 
 ## [v0.47.3](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.47.3) - 2023-06-08
 

diff --git a/snapshots/chunk.go b/snapshots/chunk.go
@@ -58,11 +58,13 @@ func (w *ChunkWriter) Close() error {
 // CloseWithError closes the writer and sends an error to the reader.
 func (w *ChunkWriter) CloseWithError(err error) {
 	if !w.closed {
+		if w.pipe == nil {
+			// create a dummy pipe just to propagate the error to the reader, it always returns nil
+			_ = w.chunk()
+		}
 		w.closed = true
 		close(w.ch)
-		if w.pipe != nil {
-			_ = w.pipe.CloseWithError(err) // CloseWithError always returns nil
-		}
+		_ = w.pipe.CloseWithError(err) // CloseWithError always returns nil
 	}
 }
 

diff --git a/snapshots/helpers_test.go b/snapshots/helpers_test.go
@@ -171,6 +171,38 @@ func (m *mockSnapshotter) SetSnapshotInterval(snapshotInterval uint64) {
 	m.snapshotInterval = snapshotInterval
 }
 
+type mockErrorSnapshotter struct{}
+
+var _ snapshottypes.Snapshotter = (*mockErrorSnapshotter)(nil)
+
+func (m *mockErrorSnapshotter) Snapshot(height uint64, protoWriter protoio.Writer) error {
+	return errors.New("mock snapshot error")
+}
+
+func (m *mockErrorSnapshotter) Restore(
+	height uint64, format uint32, protoReader protoio.Reader,
+) (snapshottypes.SnapshotItem, error) {
+	return snapshottypes.SnapshotItem{}, errors.New("mock restore error")
+}
+
+func (m *mockErrorSnapshotter) SnapshotFormat() uint32 {
+	return snapshottypes.CurrentFormat
+}
+
+func (m *mockErrorSnapshotter) SupportedFormats() []uint32 {
+	return []uint32{snapshottypes.CurrentFormat}
+}
+
+func (m *mockErrorSnapshotter) PruneSnapshotHeight(height int64) {
+}
+
+func (m *mockErrorSnapshotter) GetSnapshotInterval() uint64 {
+	return 0
+}
+
+func (m *mockErrorSnapshotter) SetSnapshotInterval(snapshotInterval uint64) {
+}
+
 // setupBusyManager creates a manager with an empty store that is busy creating a snapshot at height 1.
 // The snapshot will complete when the returned closer is called.
 func setupBusyManager(t *testing.T) *snapshots.Manager {

diff --git a/snapshots/manager_test.go b/snapshots/manager_test.go
@@ -4,12 +4,14 @@ import (
 	"errors"
 	"testing"
 
+	db "github.com/cometbft/cometbft-db"
 	"github.com/cometbft/cometbft/libs/log"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"github.com/cosmos/cosmos-sdk/snapshots"
 	"github.com/cosmos/cosmos-sdk/snapshots/types"
+	"github.com/cosmos/cosmos-sdk/testutil"
 )
 
 var opts = types.NewSnapshotOptions(1500, 2)
@@ -243,3 +245,13 @@ func TestManager_Restore(t *testing.T) {
 	})
 	require.NoError(t, err)
 }
+
+func TestManager_TakeError(t *testing.T) {
+	snapshotter := &mockErrorSnapshotter{}
+	store, err := snapshots.NewStore(db.NewMemDB(), testutil.GetTempDir(t))
+	require.NoError(t, err)
+	manager := snapshots.NewManager(store, opts, snapshotter, nil, log.NewNopLogger())
+
+	_, err = manager.Create(1)
+	require.Error(t, err)
+}