homelab/portainer
1
0
Fork 0
mirror of https://github.com/portainer/portainer.git synced 2026-08-07 09:04:48 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

fix(stacks): fix delete/update race condition BE-13171 (#3277)

Browse Source
This commit is contained in:
andres-portainer
2026-07-27 18:41:58 -03:00
committed by GitHub
parent e08d15f221
commit 3e65adb608
5 changed files with 205 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files
api/gitops/workflows/delete.go
+3 -1
View File
@@ -18,7 +18,9 @@ func DeleteIfSingleArtifact(tx workflowDeleteStore, workflowID portainer.Workflo
}
wf, err := tx.Workflow().Read(workflowID)
if err != nil {
if dataservices.IsErrObjectNotFound(err) {
return nil
} else if err != nil {
return err
}
api/gitops/workflows/delete_test.go
+63
View File
@@ -0,0 +1,63 @@
package workflows
import (
"testing"
portainer "github.com/portainer/portainer/api"
"github.com/portainer/portainer/api/datastore"
"github.com/stretchr/testify/require"
)
func TestDeleteIfSingleArtifact_ZeroWorkflowIDIsNoop(t *testing.T) {
t.Parallel()
_, store := datastore.MustNewTestStore(t, false, true)
err := DeleteIfSingleArtifact(store, 0)
require.NoError(t, err)
}
func TestDeleteIfSingleArtifact_NotFoundIsNoop(t *testing.T) {
t.Parallel()
_, store := datastore.MustNewTestStore(t, false, true)
err := DeleteIfSingleArtifact(store, 999)
require.NoError(t, err)
}
func TestDeleteIfSingleArtifact_MultipleArtifactsAreKept(t *testing.T) {
t.Parallel()
_, store := datastore.MustNewTestStore(t, false, true)
wf := &portainer.Workflow{
Name: "multi-artifact",
Artifacts: []portainer.Artifact{
{StackID: 1},
{StackID: 2},
},
}
require.NoError(t, store.Workflow().Create(wf))
err := DeleteIfSingleArtifact(store, wf.ID)
require.NoError(t, err)
_, err = store.Workflow().Read(wf.ID)
require.NoError(t, err)
}
func TestDeleteIfSingleArtifact_SingleArtifactIsDeleted(t *testing.T) {
t.Parallel()
_, store := datastore.MustNewTestStore(t, false, true)
wf := &portainer.Workflow{
Name: "single-artifact",
Artifacts: []portainer.Artifact{{StackID: 1}},
}
require.NoError(t, store.Workflow().Create(wf))
err := DeleteIfSingleArtifact(store, wf.ID)
require.NoError(t, err)
_, err = store.Workflow().Read(wf.ID)
require.Error(t, err)
}
api/http/handler/stacks/create_compose_stack_test.go
+28
View File
@@ -4,9 +4,37 @@ import (
"testing"
portainer "github.com/portainer/portainer/api"
"github.com/portainer/portainer/api/datastore"
"github.com/portainer/portainer/api/filesystem"
"github.com/portainer/portainer/api/internal/testhelpers"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
// TestCheckAndCleanStackDupFromSwarm_WorkflowAlreadyDeleted_StillDeletesStack covers a race where
// the duplicate stack's shared Workflow record was already removed (e.g. by a concurrent
// delete/update on another stack sharing it) before this cleanup runs. The duplicate stack must
// still be removed rather than leaving it stuck blocking stack name uniqueness checks.
func TestCheckAndCleanStackDupFromSwarm_WorkflowAlreadyDeleted_StillDeletesStack(t *testing.T) {
t.Parallel()
_, store := datastore.MustNewTestStore(t, true, false)
fileService, err := filesystem.NewService(t.TempDir(), "")
require.NoError(t, err, "error init file service")
handler := NewHandler(testhelpers.NewTestRequestBouncer(), nil)
handler.DataStore = store
handler.FileService = fileService
stack := &portainer.Stack{ID: 1, Name: "dup-stack", Type: portainer.DockerSwarmStack, WorkflowID: 999}
require.NoError(t, store.Stack().Create(stack))
err = handler.checkAndCleanStackDupFromSwarm(nil, nil, nil, portainer.UserID(0), stack)
require.NoError(t, err, "cleanup should succeed even though the stack's Workflow was already gone")
_, err = store.Stack().Read(stack.ID)
assert.True(t, store.IsErrObjectNotFound(err), "duplicate stack should be deleted")
}
func TestComposeGitPayload_ValidateWithSourceID_URLNotRequired(t *testing.T) {
t.Parallel()
payload := &composeStackFromGitRepositoryPayload{
api/http/handler/stacks/stack_delete_test.go
+46
View File
@@ -71,6 +71,30 @@ func TestStackDelete_WorkflowSingleArtifact_DeletesWorkflowRecord(t *testing.T)
assert.True(t, store.IsErrObjectNotFound(err))
}
// TestStackDelete_WorkflowAlreadyDeleted_StillDeletesStack covers a double-submit race: a
// concurrent request (e.g. a second click before the delete button disables) already deleted the
// shared Workflow record before this request's transaction runs. The stack must still be deleted
// rather than getting permanently stuck with a dangling WorkflowID, which would break every
// endpoint that resolves git config across all stacks.
func TestStackDelete_WorkflowAlreadyDeleted_StillDeletesStack(t *testing.T) {
t.Parallel()
h, store, _ := newStackDeleteHandler(t)
_, err := mockCreateUser(store)
require.NoError(t, err)
endpoint, err := mockCreateEndpoint(store)
require.NoError(t, err)
stack := &portainer.Stack{ID: 1, EndpointID: endpoint.ID, Type: portainer.DockerComposeStack, Name: "test-stack", WorkflowID: 999}
require.NoError(t, store.Stack().Create(stack))
w := httptest.NewRecorder()
h.ServeHTTP(w, stackDeleteRequest(stack.ID, endpoint.ID))
require.Equal(t, http.StatusNoContent, w.Code)
_, err = store.Stack().Read(stack.ID)
assert.True(t, store.IsErrObjectNotFound(err), "stack should be deleted even though its Workflow was already gone")
}
// The workflow deletion and the stack record deletion must commit atomically.
// If record deletion fails, the workflow deletion must roll back too.
func TestStackDelete_RecordDeletionFails_WorkflowDeletionRollsBack(t *testing.T) {
@@ -304,6 +328,28 @@ func TestStackDeleteKubernetesByName_WorkflowSingleArtifact_DeletesWorkflowRecor
assert.True(t, store.IsErrObjectNotFound(err))
}
// TestStackDeleteKubernetesByName_WorkflowAlreadyDeleted_StillDeletesStack mirrors
// TestStackDelete_WorkflowAlreadyDeleted_StillDeletesStack for the Kubernetes-by-name delete path.
func TestStackDeleteKubernetesByName_WorkflowAlreadyDeleted_StillDeletesStack(t *testing.T) {
t.Parallel()
h, store, _ := newStackDeleteHandler(t)
_, err := mockCreateUser(store)
require.NoError(t, err)
endpoint, err := mockCreateEndpoint(store)
require.NoError(t, err)
stack := &portainer.Stack{ID: 1, EndpointID: endpoint.ID, Type: portainer.KubernetesStack, Name: "app", Namespace: "ns1", WorkflowID: 999}
require.NoError(t, store.Stack().Create(stack))
w := httptest.NewRecorder()
h.ServeHTTP(w, stackDeleteKubernetesByNameRequest("app", "ns1", endpoint.ID))
require.Equal(t, http.StatusNoContent, w.Code)
_, err = store.Stack().Read(stack.ID)
assert.True(t, store.IsErrObjectNotFound(err), "stack should be deleted even though its Workflow was already gone")
}
type stubTeardown struct {
teardown.Service
torndownStacks []portainer.StackID
api/http/handler/stacks/stack_update_test.go
+65
View File
@@ -396,6 +396,71 @@ func setupUpdateStackInTxTest[T testUpdateStackPayload](t *testing.T, stack *por
}
}
// Test_updateComposeStack_WorkflowAlreadyDeleted_StillUpdatesStack covers a double-submit race: a
// concurrent request (e.g. a second click before the update button disables) already deleted the
// stack's shared Workflow record before this request's transaction runs. The stack update must
// still succeed rather than failing to detach the stack from its now-gone Workflow.
func Test_updateComposeStack_WorkflowAlreadyDeleted_StillUpdatesStack(t *testing.T) {
t.Parallel()
fips.InitFIPS(false)
payload := &updateComposeStackPayload{
StackFileContent: "version: '3'\nservices:\n web:\n image: nginx:latest",
}
stack := &portainer.Stack{
ID: 1,
Name: "test-stack-workflow-gone",
EntryPoint: "docker-compose.yml",
Type: portainer.DockerComposeStack,
WorkflowID: 999,
}
setup := setupUpdateStackInTxTest(t, stack, payload)
var handlerErr *httperror.HandlerError
err := setup.store.UpdateTx(func(tx dataservices.DataStoreTx) error {
_, _, _, handlerErr = setup.handler.updateStackInTx(tx, setup.req, setup.stack.ID, setup.endpoint.ID)
return nil
})
require.NoError(t, err)
require.Nil(t, handlerErr, "stack update should succeed even though its Workflow was already gone")
stored, err := setup.store.Stack().Read(setup.stack.ID)
require.NoError(t, err)
assert.Zero(t, stored.WorkflowID, "stack should be detached from the now-gone Workflow")
}
// Test_updateSwarmStack_WorkflowAlreadyDeleted_StillUpdatesStack mirrors
// Test_updateComposeStack_WorkflowAlreadyDeleted_StillUpdatesStack for Swarm stacks.
func Test_updateSwarmStack_WorkflowAlreadyDeleted_StillUpdatesStack(t *testing.T) {
t.Parallel()
fips.InitFIPS(false)
payload := &updateSwarmStackPayload{
StackFileContent: "version: '3'\nservices:\n web:\n image: nginx:latest",
}
stack := &portainer.Stack{
ID: 1,
Name: "test-stack-workflow-gone",
EntryPoint: "docker-compose.yml",
Type: portainer.DockerSwarmStack,
WorkflowID: 999,
}
setup := setupUpdateStackInTxTest(t, stack, payload)
setup.handler.SwarmStackManager = swarmStackManager{}
var handlerErr *httperror.HandlerError
err := setup.store.UpdateTx(func(tx dataservices.DataStoreTx) error {
_, _, _, handlerErr = setup.handler.updateStackInTx(tx, setup.req, setup.stack.ID, setup.endpoint.ID)
return nil
})
require.NoError(t, err)
require.Nil(t, handlerErr, "stack update should succeed even though its Workflow was already gone")
stored, err := setup.store.Stack().Read(setup.stack.ID)
require.NoError(t, err)
assert.Zero(t, stored.WorkflowID, "stack should be detached from the now-gone Workflow")
}
type swarmStackManager struct {
portainer.SwarmStackManager
}