fix(#29 review r1): stack-delete leak, rollback/retention/version tests, UX trap

F3: deleting a file-based stack now removes the stack ROOT (compose/{id}) via a new removeStackProjectDir helper, not stack.ProjectPath (which the PR repointed to compose/{id}/v{N}) — old version dirs + parent no longer leak. Git stacks unchanged. F1: tests for validateRollbackTarget (rejects 0/neg/>current/hole) and the rollback snapshot (client content ignored, target read from disk, monotonic new version, note). F2: tests for pruneStackFileVersionDirs (deletes given dirs, swallows errors) + the post-commit gate contract + a monotonic-version regression guard. F4: handler tests for ?version= (negative/out-of-range -> 400, valid version served, legacy fallback). F5: swagger @param version on GET file; @version 2.44.0 (handler.go) + package.json 2.44.0, matching APIVersion. F6: the version selector no longer sets rollbackTo for the current/top version and clears it on a manual buffer edit (so edits are honored, not silently discarded); returning to the current version restores the current content. Distinguishes real user edits from the programmatic version-load (CodeMirror ExternalChange). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-02 17:28:52 +03:00
parent d0d3c068ba
commit bb68acfbf6
8 changed files with 616 additions and 11 deletions
@@ -1,11 +1,16 @@
 package stacks

 import (
+	"errors"
 	"strconv"
 	"testing"

 	portainer "github.com/portainer/portainer/api"
+	"github.com/portainer/portainer/api/dataservices"
+	"github.com/portainer/portainer/api/datastore"
 	"github.com/portainer/portainer/api/filesystem"
+	"github.com/portainer/portainer/api/internal/testhelpers"
+	httperror "github.com/portainer/portainer/pkg/libhttp/error"

 	"github.com/stretchr/testify/require"
 )
@@ -92,6 +97,250 @@ func versionNumbers(versions []portainer.StackFileVersionInfo) []int {
 	return out
 }

+// TestValidateRollbackTarget checks the rollback-version guard: it rejects non-positive versions,
+// versions above the current file version, and versions that are not present in the append-only
+// history (a "hole"), while accepting an in-range version that exists in Versions[].
+func TestValidateRollbackTarget(t *testing.T) {
+	t.Parallel()
+
+	// StackFileVersion is 5, but v4 was never recorded (a hole in the history).
+	stack := &portainer.Stack{
+		StackFileVersion: 5,
+		Versions: []portainer.StackFileVersionInfo{
+			{Version: 1}, {Version: 2}, {Version: 3}, {Version: 5},
+		},
+	}
+
+	require.Error(t, validateRollbackTarget(stack, 0), "zero is not a valid version")
+	require.Error(t, validateRollbackTarget(stack, -1), "negative is not a valid version")
+	require.Error(t, validateRollbackTarget(stack, 6), "version above StackFileVersion is out of range")
+	require.Error(t, validateRollbackTarget(stack, 4), "a version missing from history (hole) is rejected")
+
+	require.NoError(t, validateRollbackTarget(stack, 3), "in-range version present in history is accepted")
+	require.NoError(t, validateRollbackTarget(stack, 5), "current version present in history is accepted")
+}
+
+// newVersioningTestHandler builds a Handler wired with a real filesystem service and datastore,
+// plus a test user, for exercising the version snapshot/prune helpers.
+func newVersioningTestHandler(t *testing.T) (*Handler, *datastore.Store, portainer.FileService, *portainer.User) {
+	t.Helper()
+
+	_, store := datastore.MustNewTestStore(t, false, true)
+
+	fs, err := filesystem.NewService(t.TempDir(), "")
+	require.NoError(t, err)
+
+	user, err := mockCreateUser(store)
+	require.NoError(t, err)
+
+	handler := NewHandler(testhelpers.NewTestRequestBouncer())
+	handler.DataStore = store
+	handler.FileService = fs
+
+	return handler, store, fs, user
+}
+
+// TestSnapshotFileBasedStackVersion_Rollback verifies the rollback branch: it reads the TARGET
+// version's content from disk (ignoring the client-supplied content), writes a NEW monotonic
+// version whose note is "rollback from v{N}", and repoints ProjectPath to the new version dir.
+func TestSnapshotFileBasedStackVersion_Rollback(t *testing.T) {
+	t.Parallel()
+
+	handler, store, fs, user := newVersioningTestHandler(t)
+
+	stackFolder := "1"
+	entryPoint := "docker-compose.yml"
+
+	// Seed three on-disk versions with distinct content.
+	for v, content := range map[int]string{1: "V1-CONTENT", 2: "V2-CONTENT", 3: "V3-CONTENT"} {
+		_, err := fs.StoreStackFileFromBytesByVersion(stackFolder, entryPoint, v, []byte(content))
+		require.NoError(t, err)
+	}
+
+	stack := &portainer.Stack{
+		ID:               1,
+		EntryPoint:       entryPoint,
+		StackFileVersion: 3,
+		ProjectPath:      fs.GetStackProjectPathByVersion(stackFolder, 3, ""),
+		Versions: []portainer.StackFileVersionInfo{
+			{Version: 1}, {Version: 2}, {Version: 3},
+		},
+	}
+
+	target := 1
+	var (
+		pruned  []int
+		httpErr *httperror.HandlerError
+	)
+	err := store.UpdateTx(func(tx dataservices.DataStoreTx) error {
+		// Client content is deliberately non-empty to prove it is IGNORED for a rollback.
+		pruned, httpErr = handler.snapshotFileBasedStackVersion(tx, stack, []byte("CLIENT-CONTENT-IGNORED"), &target, user.ID)
+		return nil
+	})
+	require.NoError(t, err)
+	require.Nil(t, httpErr)
+	require.Empty(t, pruned, "no retention pruning expected below the cap")
+
+	// A new monotonic version (v4) was created.
+	require.Equal(t, 4, stack.StackFileVersion)
+	require.Equal(t, fs.GetStackProjectPathByVersion(stackFolder, 4, ""), stack.ProjectPath)
+
+	last := stack.Versions[len(stack.Versions)-1]
+	require.Equal(t, 4, last.Version)
+	require.Equal(t, "rollback from v1", last.Note)
+
+	// The new version's content is the TARGET (v1) content read from disk, not the client payload.
+	got, err := fs.GetFileContent(stack.ProjectPath, entryPoint)
+	require.NoError(t, err)
+	require.Equal(t, "V1-CONTENT", string(got))
+}
+
+// TestSnapshotFileBasedStackVersion_MonotonicVersion guards against a len-based next-version bug:
+// the new version must be StackFileVersion+1, strictly greater than any previously trimmed version,
+// even when the history slice is shorter than StackFileVersion (older entries already pruned).
+func TestSnapshotFileBasedStackVersion_MonotonicVersion(t *testing.T) {
+	t.Parallel()
+
+	handler, store, fs, user := newVersioningTestHandler(t)
+
+	stackFolder := "1"
+	entryPoint := "docker-compose.yml"
+
+	// StackFileVersion is 24 but only two history entries remain (older ones were pruned):
+	// a len-based scheme would wrongly compute the next version as len+1 = 3.
+	stack := &portainer.Stack{
+		ID:               1,
+		EntryPoint:       entryPoint,
+		StackFileVersion: 24,
+		ProjectPath:      fs.GetStackProjectPathByVersion(stackFolder, 24, ""),
+		Versions: []portainer.StackFileVersionInfo{
+			{Version: 23}, {Version: 24},
+		},
+	}
+
+	var httpErr *httperror.HandlerError
+	err := store.UpdateTx(func(tx dataservices.DataStoreTx) error {
+		_, httpErr = handler.snapshotFileBasedStackVersion(tx, stack, []byte("NEW-CONTENT"), nil, user.ID)
+		return nil
+	})
+	require.NoError(t, err)
+	require.Nil(t, httpErr)
+
+	require.Equal(t, 25, stack.StackFileVersion, "next version must be StackFileVersion+1, not len-based")
+	last := stack.Versions[len(stack.Versions)-1]
+	require.Equal(t, 25, last.Version)
+	require.Greater(t, last.Version, 24, "new version must be strictly greater than any prior version")
+
+	got, err := fs.GetFileContent(stack.ProjectPath, entryPoint)
+	require.NoError(t, err)
+	require.Equal(t, "NEW-CONTENT", string(got))
+}
+
+// countingFileService wraps a FileService to count and optionally fail RemoveDirectory calls,
+// so tests can assert prune ordering and error-swallowing without touching real disk.
+type countingFileService struct {
+	portainer.FileService
+	removeDirErr   error
+	removeDirCalls int
+	removedDirs    []string
+}
+
+func (s *countingFileService) GetStackProjectPathByVersion(stackID string, version int, commitHash string) string {
+	return "compose/" + stackID + "/v" + strconv.Itoa(version)
+}
+
+func (s *countingFileService) RemoveDirectory(dir string) error {
+	s.removeDirCalls++
+	s.removedDirs = append(s.removedDirs, dir)
+	return s.removeDirErr
+}
+
+// TestPruneStackFileVersionDirs_RemovesGivenDirs verifies the prune helper physically deletes
+// exactly the requested version directories and leaves the others untouched.
+func TestPruneStackFileVersionDirs_RemovesGivenDirs(t *testing.T) {
+	t.Parallel()
+
+	fs, err := filesystem.NewService(t.TempDir(), "")
+	require.NoError(t, err)
+
+	handler := NewHandler(testhelpers.NewTestRequestBouncer())
+	handler.FileService = fs
+
+	const stackID = portainer.StackID(9)
+	stackFolder := strconv.Itoa(int(stackID))
+
+	for v := 1; v <= 3; v++ {
+		_, err := fs.StoreStackFileFromBytesByVersion(stackFolder, "docker-compose.yml", v, []byte("v"+strconv.Itoa(v)))
+		require.NoError(t, err)
+	}
+
+	handler.pruneStackFileVersionDirs(stackID, []int{1, 3})
+
+	for v, wantExists := range map[int]bool{1: false, 2: true, 3: false} {
+		exists, err := fs.FileExists(fs.GetStackProjectPathByVersion(stackFolder, v, ""))
+		require.NoError(t, err)
+		require.Equal(t, wantExists, exists, "version %d directory existence mismatch", v)
+	}
+}
+
+// TestPruneStackFileVersionDirs_SwallowsRemoveError verifies a RemoveDirectory failure is
+// best-effort: it is logged and swallowed (no panic), and every requested version is attempted.
+func TestPruneStackFileVersionDirs_SwallowsRemoveError(t *testing.T) {
+	t.Parallel()
+
+	fs := &countingFileService{removeDirErr: errors.New("disk error")}
+	handler := NewHandler(testhelpers.NewTestRequestBouncer())
+	handler.FileService = fs
+
+	require.NotPanics(t, func() {
+		handler.pruneStackFileVersionDirs(7, []int{1, 2})
+	})
+	require.Equal(t, 2, fs.removeDirCalls, "every requested version directory must be attempted")
+}
+
+// TestPruneGateContract_Illustrative documents the post-commit gate shape used by stackUpdate:
+// the pruned version directories are physically removed only when the transaction committed
+// (err == nil); on a failed transaction the trimmed Versions[] was never persisted, so the
+// directories must be kept on disk to stay consistent with the database.
+//
+// NOTE: this reproduces the gate condition inline — it illustrates the intended contract rather
+// than exercising the real handler wiring (forcing a mid-commit UpdateTx failure while
+// pruneVersions is already populated is not injectable in a unit test). The actual gate lives in
+// stackUpdate (`if err == nil && len(pruneVersions) > 0`); pruneStackFileVersionDirs's real
+// behaviour (deletes the given dirs, swallows RemoveDirectory errors) is covered non-vacuously by
+// TestPruneStackFileVersionDirs_RemovesGivenDirs and _SwallowsRemoveError.
+func TestPruneGateContract_Illustrative(t *testing.T) {
+	t.Parallel()
+
+	pruneVersions := []int{1, 2}
+
+	t.Run("transaction failed - prune skipped", func(t *testing.T) {
+		fs := &countingFileService{}
+		handler := NewHandler(testhelpers.NewTestRequestBouncer())
+		handler.FileService = fs
+
+		var err error = errors.New("commit failed")
+		if err == nil && len(pruneVersions) > 0 {
+			handler.pruneStackFileVersionDirs(7, pruneVersions)
+		}
+
+		require.Zero(t, fs.removeDirCalls, "no directory may be deleted when the transaction failed")
+	})
+
+	t.Run("transaction committed - prune runs", func(t *testing.T) {
+		fs := &countingFileService{}
+		handler := NewHandler(testhelpers.NewTestRequestBouncer())
+		handler.FileService = fs
+
+		var err error
+		if err == nil && len(pruneVersions) > 0 {
+			handler.pruneStackFileVersionDirs(7, pruneVersions)
+		}
+
+		require.Equal(t, len(pruneVersions), fs.removeDirCalls, "all pruned directories deleted after commit")
+	})
+}
+
 // TestSnapshotStackFilesToVersion verifies a multi-file snapshot writes every file into the
 // v{N} folder and returns the version project path (not the base path).
 func TestSnapshotStackFilesToVersion(t *testing.T) {