fix(rds): run one teardown of a DB instance delete at a time (#1647)

* fix(rds): run one teardown of a DB instance delete at a time

The handler that accepted a delete now owns its teardown for its request
budget, a repeat of an accepted delete answers deleting without a second
teardown, and the reconciler resumes only a delete older than that budget.
Two teardowns racing let the handler see the VM mid-shutdown as gone and
answer 500 on the still-attached data volume.

* fix(rds): end the delete handler's teardown exactly when its claim does

The handler's deadline now derives from the persisted transition stamp the
reconciler measures from, and a teardown past that deadline starts no
terminate, since the graceful engine stop swallows the cancellation.

spinifex/domains/rds/instance/delete.go

@@ -66,6 +66,12 @@ func (s *Service) DeleteDBInstance(ctx context.Context, input *rds.DeleteDBInsta
 			"DB instance %s is already being deleted with %s; a retry must repeat that choice", id, snapshotChoice(rec.FinalSnapshotIdentifier))
 	}
 
+	// The handler that accepted the delete tears it down, and the reconciler
+	// resumes one whose handler is gone, so a repeat only reports the state.
+	if rec.Status == StatusDeleting {
+		return &rds.DeleteDBInstanceOutput{DBInstance: s.projectDBInstance(rec)}, nil
+	}
+
 	// Reserve the final snapshot before the instance enters deleting. A collision
 	// therefore fails while the VM and data volume are still intact, and a crash
 	// afterwards leaves a creating record the reconciler can resolve.
@@ -74,29 +80,29 @@ func (s *Service) DeleteDBInstance(ctx context.Context, input *rds.DeleteDBInsta
 		return nil, err
 	}
 
-	// A repeat of a delete already under way re-runs the teardown rather than
-	// being rejected: the first call may have died partway through it.
-	if rec.Status != StatusDeleting {
-		now := time.Now().UTC()
-		rec.Status = StatusDeleting
-		rec.FailureReason = ""
-		rec.FinalSnapshotIdentifier = finalSnapshot
-		rec.TransitionStartedAt = &now
-		rec.UpdatedAt = now
-		if err := updateJSON(ctx, kv, DBInstanceKey(id), rev, rec); err != nil {
-			if reservation.creator {
-				s.rollbackFinalSnapshotReservation(ctx, kv, finalSnapshot, reservation.revision)
-			}
-			if errors.Is(err, kvstore.ErrConflict) {
-				return nil, awserrors.Errorf(awserrors.ErrorDBInstanceInvalidState,
-					"DB instance %s changed state concurrently; retry the delete", id)
-			}
-			return nil, err
+	now := time.Now().UTC()
+	rec.Status = StatusDeleting
+	rec.FailureReason = ""
+	rec.FinalSnapshotIdentifier = finalSnapshot
+	rec.TransitionStartedAt = &now
+	rec.UpdatedAt = now
+	if err := updateJSON(ctx, kv, DBInstanceKey(id), rev, rec); err != nil {
+		if reservation.creator {
+			s.rollbackFinalSnapshotReservation(ctx, kv, finalSnapshot, reservation.revision)
 		}
-		s.RecordEvent(ctx, accountID, EventSourceTypeDBInstance, id, "DB instance deleted.", EventCategoryDeletion)
+		if errors.Is(err, kvstore.ErrConflict) {
+			return nil, awserrors.Errorf(awserrors.ErrorDBInstanceInvalidState,
+				"DB instance %s changed state concurrently; retry the delete", id)
+		}
+		return nil, err
 	}
+	s.RecordEvent(ctx, accountID, EventSourceTypeDBInstance, id, "DB instance deleted.", EventCategoryDeletion)
 
-	if err := s.teardownDBInstance(ctx, kv, accountID, rec, reservation.creator); err != nil {
+	// Ends exactly when the reconciler's guard does, so the teardown is this
+	// handler's alone until the reconciler may resume it.
+	teardownCtx, cancel := context.WithDeadline(ctx, deleteClaimEnds(rec))
+	defer cancel()
+	if err := s.teardownDBInstance(teardownCtx, kv, accountID, rec, reservation.creator); err != nil {
 		// The reconciler finishes the delete once the modify lets go.
 		if errors.Is(err, errModifyInProgress) {
 			return &rds.DeleteDBInstanceOutput{DBInstance: s.projectDBInstance(rec)}, nil
@@ -202,6 +208,11 @@ func (s *Service) teardownDBInstance(ctx context.Context, kv *kvstore.Bucket, ac
 		s.stopEngineOrRecordFallback(ctx, accountID, rec, "deleting")
 	}
 
+	// The engine stop swallows a deadline, and a handler past its claim must not
+	// start a terminate the reconciler may already be running.
+	if err := ctx.Err(); err != nil {
+		return err
+	}
 	if err := s.terminateInstanceVM(ctx, rec.InstanceID, rec.DataVolumeID); err != nil {
 		return fmt.Errorf("rds: terminate the VM behind %s: %w", rec.DBInstanceIdentifier, err)
 	}

spinifex/domains/rds/instance/delete_test.go

@@ -1,9 +1,11 @@
 package instance
 
 import (
+	"context"
 	"errors"
 	"fmt"
 	"testing"
+	"time"
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/rds"
@@ -176,13 +178,12 @@ func TestDeleteDBInstance_ReservesTheFinalSnapshotBeforeCuttingIt(t *testing.T)
 }
 
 // A dead worker can leave the EC2 snapshot cut while its RDS record still says
-// creating. A retry adopts that exact account-scoped snapshot instead of
-// cutting another one and permanently pinning the source volume twice.
-func TestDeleteDBInstance_AdoptsAnInterruptedFinalSnapshot(t *testing.T) {
+// creating. The resumed teardown adopts that exact account-scoped snapshot
+// instead of cutting another one and permanently pinning the source volume twice.
+func TestReconciler_AdoptsAnInterruptedFinalSnapshot(t *testing.T) {
 	t.Parallel()
 	h := newLifecycleHarness(t, false)
-	rec := availableRecord()
-	rec.Status = StatusDeleting
+	rec := abandonedDelete(availableRecord())
 	rec.FinalSnapshotIdentifier = "orders-db-final"
 	seedInstance(t, h.svc, rec)
 
@@ -201,13 +202,9 @@ func TestDeleteDBInstance_AdoptsAnInterruptedFinalSnapshot(t *testing.T) {
 		},
 	}
 
-	_, err = h.svc.DeleteDBInstance(t.Context(), &rds.DeleteDBInstanceInput{
-		DBInstanceIdentifier:      aws.String(testDBID),
-		FinalDBSnapshotIdentifier: aws.String(rec.FinalSnapshotIdentifier),
-	}, testAccountID)
-	require.NoError(t, err)
+	require.NoError(t, onePass(t, NewReconciler(h.svc, "node-a")))
 
-	assert.Empty(t, h.snaps.created, "the retry must adopt the cut snapshot, not create another")
+	assert.Empty(t, h.snaps.created, "the resumed teardown must adopt the cut snapshot, not create another")
 	stored, found := h.snapshotRecord(t, rec.FinalSnapshotIdentifier)
 	require.True(t, found)
 	assert.Equal(t, SnapshotStatusAvailable, stored.Status)
@@ -325,26 +322,18 @@ func TestDeleteDBInstance_RetainsTheVolumeWhenTheVolumeStoreReportsItInUse(t *te
 
 // A teardown that died partway through is replayed, so every step has to treat
 // a resource that is already gone as work it has done.
-func TestDeleteDBInstance_IsIdempotentAcrossARetriedTeardown(t *testing.T) {
+func TestReconciler_IsIdempotentAcrossARetriedTeardown(t *testing.T) {
 	t.Parallel()
 	h := newLifecycleHarness(t, false)
-	rec := availableRecord()
-	// The shape a delete leaves behind when its caller died mid-teardown.
-	rec.Status = StatusDeleting
+	rec := abandonedDelete(availableRecord())
 	rec.FinalSnapshotIdentifier = "orders-db-final"
 	seedInstance(t, h.svc, rec)
-
-	input := &rds.DeleteDBInstanceInput{
-		DBInstanceIdentifier:      aws.String(testDBID),
-		FinalDBSnapshotIdentifier: aws.String("orders-db-final"),
-	}
-	_, err := h.svc.DeleteDBInstance(t.Context(), input, testAccountID)
-	require.NoError(t, err)
+	reconciler := NewReconciler(h.svc, "node-a")
+	require.NoError(t, onePass(t, reconciler))
 
 	// The record left behind by the interrupted attempt, replayed.
 	seedInstance(t, h.svc, rec)
-	_, err = h.svc.DeleteDBInstance(t.Context(), input, testAccountID)
-	require.NoError(t, err)
+	require.NoError(t, onePass(t, reconciler))
 
 	assert.Len(t, h.snaps.created, 1, "a replayed teardown must not take a second final snapshot")
 	assert.False(t, h.recordExists(t, testDBID))
@@ -355,9 +344,7 @@ func TestDeleteDBInstance_IsIdempotentAcrossARetriedTeardown(t *testing.T) {
 func TestReconciler_ResumesAnInterruptedDelete(t *testing.T) {
 	t.Parallel()
 	h := newLifecycleHarness(t, false)
-	rec := availableRecord()
-	rec.Status = StatusDeleting
-	seedInstance(t, h.svc, rec)
+	seedInstance(t, h.svc, abandonedDelete(availableRecord()))
 
 	reconciler := NewReconciler(h.svc, "node-a")
 	require.NoError(t, onePass(t, reconciler))
@@ -366,6 +353,87 @@ func TestReconciler_ResumesAnInterruptedDelete(t *testing.T) {
 	assert.False(t, h.recordExists(t, testDBID))
 }
 
+// A second teardown racing the handler's sees the VM mid-shutdown as gone and
+// fails on the still-attached volume, so the handler's delete is left to it.
+func TestReconciler_LeavesADeleteItsHandlerMayStillBeRunning(t *testing.T) {
+	t.Parallel()
+	h := newLifecycleHarness(t, false)
+	rec := availableRecord()
+	started := time.Now().UTC()
+	rec.Status = StatusDeleting
+	rec.TransitionStartedAt = &started
+	seedInstance(t, h.svc, rec)
+
+	require.NoError(t, onePass(t, NewReconciler(h.svc, "node-a")))
+
+	assert.Empty(t, h.launcher.terminated)
+	assert.True(t, h.recordExists(t, testDBID))
+}
+
+// A repeat must not start a teardown of its own alongside the one under way;
+// the reconciler is what resumes an abandoned one.
+func TestDeleteDBInstance_ARepeatStartsNoTeardown(t *testing.T) {
+	t.Parallel()
+	h := newLifecycleHarness(t, false)
+	seedInstance(t, h.svc, abandonedDelete(availableRecord()))
+
+	out, err := h.svc.DeleteDBInstance(t.Context(), skipFinalSnapshot(), testAccountID)
+	require.NoError(t, err)
+
+	assert.Equal(t, string(StatusDeleting), aws.StringValue(out.DBInstance.DBInstanceStatus))
+	assert.Empty(t, h.launcher.terminated)
+	assert.True(t, h.recordExists(t, testDBID))
+}
+
+// The reconciler resumes a delete from the persisted stamp, so the handler's
+// teardown must stop at that same instant, however long the acceptance took.
+func TestDeleteDBInstance_TeardownEndsWhenTheClaimDoes(t *testing.T) {
+	t.Parallel()
+	h := newLifecycleHarness(t, false)
+	seedInstance(t, h.svc, availableRecord())
+	var claimEnds time.Time
+	h.snaps.beforeCreate = func() { stored := h.record(t); claimEnds = deleteClaimEnds(&stored) }
+
+	_, err := h.svc.DeleteDBInstance(t.Context(), &rds.DeleteDBInstanceInput{
+		DBInstanceIdentifier:      aws.String(testDBID),
+		FinalDBSnapshotIdentifier: aws.String("orders-db-final"),
+	}, testAccountID)
+	require.NoError(t, err)
+
+	require.False(t, claimEnds.IsZero())
+	assert.True(t, h.snaps.deadline.Equal(claimEnds), "deadline %s, claim ends %s", h.snaps.deadline, claimEnds)
+}
+
+// A graceful stop that runs into the claim's end is swallowed, so the teardown
+// itself has to notice and leave the terminate to the reconciler.
+func TestTeardownDBInstance_StartsNoTerminatePastItsDeadline(t *testing.T) {
+	t.Parallel()
+	h := newLifecycleHarness(t, false)
+	h.agent.silenceType(CommandStopEngine)
+	rec := availableRecord()
+	rec.Status = StatusDeleting
+	rec.FinalSnapshotIdentifier = "orders-db-final"
+	seedInstance(t, h.svc, rec)
+	kv, err := h.svc.bucket(t.Context(), testAccountID)
+	require.NoError(t, err)
+
+	ctx, cancel := context.WithTimeout(t.Context(), 200*time.Millisecond)
+	defer cancel()
+	err = h.svc.teardownDBInstance(ctx, kv, testAccountID, &rec, true)
+
+	require.ErrorIs(t, err, context.DeadlineExceeded)
+	assert.Empty(t, h.launcher.terminated)
+	assert.True(t, h.recordExists(t, testDBID))
+}
+
+// The shape a delete leaves once its handler's request budget has run out.
+func abandonedDelete(rec DBInstanceRecord) DBInstanceRecord {
+	started := time.Now().UTC().Add(-deleteTimeout - time.Minute)
+	rec.Status = StatusDeleting
+	rec.TransitionStartedAt = &started
+	return rec
+}
+
 // A stop whose caller died leaves the VM possibly still running, so the stop is
 // re-issued rather than assumed.
 func TestReconciler_ResumesAnInterruptedStop(t *testing.T) {

spinifex/domains/rds/instance/lifecycle_test.go

@@ -117,7 +117,9 @@ func (f *fakeInstanceCommander) DetachVolumeFromStopped(_ context.Context, insta
 // DescribeSnapshots reports against the data volume, which is what decides
 // between deleting the volume and retaining it.
 type fakeSnapshots struct {
-	created       []*ec2.CreateSnapshotInput
+	created []*ec2.CreateSnapshotInput
+	// The context deadline the last CreateSnapshot ran under.
+	deadline      time.Time
 	holding       []string
 	deleted       []string
 	createErr     error
@@ -132,7 +134,8 @@ type fakeSnapshots struct {
 
 var _ snapshotProvider = (*fakeSnapshots)(nil)
 
-func (f *fakeSnapshots) CreateSnapshot(_ context.Context, input *ec2.CreateSnapshotInput, _ string) (*ec2.Snapshot, error) {
+func (f *fakeSnapshots) CreateSnapshot(ctx context.Context, input *ec2.CreateSnapshotInput, _ string) (*ec2.Snapshot, error) {
+	f.deadline, _ = ctx.Deadline()
 	if f.beforeCreate != nil {
 		f.beforeCreate()
 	}

spinifex/domains/rds/instance/reconciler.go

@@ -610,6 +610,11 @@ func (r *Reconciler) reconcileStopping(ctx context.Context, kv *kvstore.Bucket,
 // did; only a teardown still stuck past the bound is called failed, which is
 // what lets the customer retry the delete.
 func (r *Reconciler) reconcileDeleting(ctx context.Context, kv *kvstore.Bucket, rev uint64, accountID string, rec *DBInstanceRecord) error {
+	// The handler that accepted the delete tears it down until its claim ends; a
+	// second teardown alongside it sees the VM mid-shutdown as gone.
+	if time.Now().Before(deleteClaimEnds(rec)) {
+		return nil
+	}
 	err := r.svc.teardownDBInstance(ctx, kv, accountID, rec, false)
 	if err == nil || errors.Is(err, errFinalSnapshotInProgress) || errors.Is(err, errModifyInProgress) {
 		return nil
@@ -795,6 +800,12 @@ func (s *Service) findEC2SnapshotFor(ctx context.Context, accountID, dbSnapshotI
 // When the transition began. A record written by an older control plane carries
 // no stamp, so its last write stands in — it is never earlier than the
 // transition, so the bound cannot be under-counted.
+// When the handler that accepted a delete gives up its teardown, measured from
+// the persisted stamp so the handler and the reconciler agree on it.
+func deleteClaimEnds(rec *DBInstanceRecord) time.Time {
+	return transitionStarted(rec).Add(deleteTimeout)
+}
+
 func transitionStarted(rec *DBInstanceRecord) time.Time {
 	if rec.TransitionStartedAt != nil {
 		return *rec.TransitionStartedAt