From 36949804e88e28dd21114a359061166684e2a094 Mon Sep 17 00:00:00 2001 From: Josh Siviour Date: Tue, 20 Oct 2026 09:41:28 +1000 Subject: [PATCH] fix: unreadable volume records are never NotFound in DeleteVolume or ForceDetachVolume (#1226) * fix: report an unreadable volume document as InternalError, not NotFound * fix: ForceDetachVolume refuses an unreadable volume record ForceDetachVolume mapped every metadata read error to InvalidVolume.NotFound. An unreadable (present but undecodable or unreconstructable) document now returns InternalError and leaves the attachment untouched; a genuinely absent document or another account's volume still returns NotFound. This is the call spx admin account delete --force uses, so an unreadable volume no longer reads as already detached. --- .../volume/delete_volume_unreadable_test.go | 211 ++++++++++++++++++ spinifex/domains/ec2/volume/service_impl.go | 14 +- 2 files changed, 222 insertions(+), 3 deletions(-) create mode 100644 spinifex/domains/ec2/volume/delete_volume_unreadable_test.go diff --git a/spinifex/domains/ec2/volume/delete_volume_unreadable_test.go b/spinifex/domains/ec2/volume/delete_volume_unreadable_test.go new file mode 100644 index 000000000..29833b7a4 --- /dev/null +++ b/spinifex/domains/ec2/volume/delete_volume_unreadable_test.go @@ -0,0 +1,211 @@ +package volume + +import ( + "context" + "errors" + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/aws/aws-sdk-go/service/s3" + "github.com/mulgadc/spinifex/spinifex/domains/ec2/ebs/metadata" + awserrors "github.com/mulgadc/spinifex/spinifex/foundation/aws/errors" + ebsprovider "github.com/mulgadc/spinifex/spinifex/providers/ebs" + "github.com/mulgadc/spinifex/spinifex/providers/objectstore" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// getFailingStore fails the read of one key with an error that is not a +// missing key, standing in for a document that is present but unreadable +// (e.g. a storage-layer reconstruction failure). +type getFailingStore struct { + *objectstore.MemoryObjectStore + + failKey string +} + +func (s *getFailingStore) GetObject(ctx context.Context, input *s3.GetObjectInput) (*s3.GetObjectOutput, error) { + if aws.StringValue(input.Key) == s.failKey { + return nil, errors.New("reconstruction failed: shard sizes do not match") + } + return s.MemoryObjectStore.GetObject(ctx, input) +} + +// countingFailAfterStore fails every read of one key from its (succeedCalls+1)th +// call onward, deterministically reproducing a document that answers one read +// and then goes unreadable on the next, without timing. +type countingFailAfterStore struct { + *objectstore.MemoryObjectStore + + failKey string + succeedCalls int + calls int +} + +func (s *countingFailAfterStore) GetObject(ctx context.Context, input *s3.GetObjectInput) (*s3.GetObjectOutput, error) { + if aws.StringValue(input.Key) == s.failKey { + s.calls++ + if s.calls > s.succeedCalls { + return nil, errors.New("reconstruction failed: shard sizes do not match") + } + } + return s.MemoryObjectStore.GetObject(ctx, input) +} + +// TestDeleteVolume_UnreadableDocumentIsInternalError pins the fix: a volume +// document that fails to read for a reason other than absence must answer +// InternalError, never InvalidVolume.NotFound. Reporting NotFound here is +// what let terminate cleanup and the legacy reaper count a volume whose +// state is unknown as already deleted. +func TestDeleteVolume_UnreadableDocumentIsInternalError(t *testing.T) { + memory := objectstore.NewMemoryObjectStore() + svc := newTestVolumeServiceWithStore("ap-southeast-2a", memory) + + volumeID := "vol-unreadable0000" + createVolumeInStoreWithMeta(t, svc, memory, volumeID, ebsmetadata.Volume{ + VolumeID: volumeID, CapacityGiB: 10, State: "available", + }) + + failing := &getFailingStore{MemoryObjectStore: memory, failKey: metadataKey(t, volumeID)} + svc.store = failing + svc.metadata = ebsmetadata.NewStore(failing, "test-bucket") + + _, err := svc.DeleteVolume(context.Background(), &ec2.DeleteVolumeInput{ + VolumeId: aws.String(volumeID), + }, testVolAccountID) + + require.Error(t, err) + assert.Equal(t, awserrors.ErrorServerInternal, awserrors.ValidErrorCodeFromError(err), + "an unreadable document must answer InternalError, not NotFound") + assert.NotContains(t, err.Error(), awserrors.ErrorInvalidVolumeNotFound) + + // Nothing was deleted: the document is still there (read through the + // underlying store, bypassing the fault), and so is the provider volume. + _, getErr := ebsmetadata.NewStore(memory, "test-bucket").GetVolume(context.Background(), testVolAccountID, volumeID) + require.NoError(t, getErr, "the control-plane document must survive a failed delete") + + provVol, provErr := svc.provider.GetVolume(context.Background(), ebsprovider.GetVolumeRequest{ + Versioned: ebsprovider.NewVersioned(), VolumeID: volumeID, + }) + require.NoError(t, provErr) + require.NotNil(t, provVol, "the provider's data must survive a failed delete") +} + +// TestDeleteVolume_AbsentDocumentIsStillNotFound guards the other side of the +// fix: absence (NoSuchKey) must keep returning InvalidVolume.NotFound, not be +// swept up into the new InternalError branch. +func TestDeleteVolume_AbsentDocumentIsStillNotFound(t *testing.T) { + svc := newTestVolumeService("ap-southeast-2a") + + _, err := svc.DeleteVolume(context.Background(), &ec2.DeleteVolumeInput{ + VolumeId: aws.String("vol-absent00000001"), + }, testVolAccountID) + + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidVolumeNotFound, err.Error()) +} + +// TestDeleteVolume_CrossAccountDocumentIsStillNotFound guards the cross-tenant +// branch: a document that exists but belongs to another account must keep +// returning InvalidVolume.NotFound, exactly as before the fix. +func TestDeleteVolume_CrossAccountDocumentIsStillNotFound(t *testing.T) { + store := objectstore.NewMemoryObjectStore() + svc := newTestVolumeServiceWithStore("ap-southeast-2a", store) + + createVolumeInStoreWithMeta(t, svc, store, "vol-othertenant000", ebsmetadata.Volume{ + VolumeID: "vol-othertenant000", CapacityGiB: 10, State: "available", TenantID: "222222222222", + }) + + _, err := svc.DeleteVolume(context.Background(), &ec2.DeleteVolumeInput{ + VolumeId: aws.String("vol-othertenant000"), + }, testVolAccountID) + + require.Error(t, err) + assert.Equal(t, awserrors.ErrorInvalidVolumeNotFound, err.Error()) +} + +// TestDeleteVolumeOnTerminate_DeleteReadFailureIsNotSuccess reproduces the +// production shape from mulga-salhr: the attachment-clear read inside +// UpdateVolumeState succeeds, but the document goes unreadable before +// DeleteVolume's own read of the same key. That read failure must come back +// as a non-NotFound error, which is exactly the signal +// daemon/vm_adapters.go's DeleteVolumes and instance/service_impl.go's +// deleteInstanceVolumes use to tell a real failure from an already-gone +// volume (!awserrors.IsNotFound(err) && !objectstore.IsNoSuchKeyError(err)). +func TestDeleteVolumeOnTerminate_DeleteReadFailureIsNotSuccess(t *testing.T) { + memory := objectstore.NewMemoryObjectStore() + svc := newTestVolumeServiceWithStore("ap-southeast-2a", memory) + + volumeID := "vol-stopped-root2" + createVolumeInStoreWithMeta(t, svc, memory, volumeID, ebsmetadata.Volume{ + VolumeID: volumeID, + CapacityGiB: 10, + State: "available", + AttachedInstance: "i-stopped", + }) + + // The first read of the document (UpdateVolumeState's attachment clear) + // succeeds; every read from the second onward (DeleteVolume's own read) + // fails, deterministically, with no reliance on timing. + failing := &countingFailAfterStore{MemoryObjectStore: memory, failKey: metadataKey(t, volumeID), succeedCalls: 1} + svc.store = failing + svc.metadata = ebsmetadata.NewStore(failing, "test-bucket") + + err := svc.DeleteVolumeOnTerminate(context.Background(), volumeID, testVolAccountID) + + require.Error(t, err, "a delete-read failure after a successful attachment clear must not be reported as success") + assert.False(t, awserrors.IsNotFound(err), + "terminate cleanup treats IsNotFound as already-gone; this must not qualify") + assert.False(t, objectstore.IsNoSuchKeyError(err), + "terminate cleanup treats IsNoSuchKeyError as already-gone; this must not qualify") + + // The attachment was cleared (terminate implies detach), but the document + // and the provider volume must both still exist: nothing was deleted. + _, getErr := ebsmetadata.NewStore(memory, "test-bucket").GetVolume(context.Background(), testVolAccountID, volumeID) + require.NoError(t, getErr, "the control-plane document must survive a failed delete") + + provVol, provErr := svc.provider.GetVolume(context.Background(), ebsprovider.GetVolumeRequest{ + Versioned: ebsprovider.NewVersioned(), VolumeID: volumeID, + }) + require.NoError(t, provErr) + require.NotNil(t, provVol, "the provider's data must survive a failed delete") +} + +// TestForceDetachVolume_UnreadableDocumentIsInternalError: an attached volume +// whose document is present but unreadable must answer InternalError and keep +// its attachment; absence and another account's volume stay NotFound. +func TestForceDetachVolume_UnreadableDocumentIsInternalError(t *testing.T) { + memory := objectstore.NewMemoryObjectStore() + svc := newTestVolumeServiceWithStore("ap-southeast-2a", memory) + + volumeID := "vol-forcedetach000" + createVolumeInStoreWithMeta(t, svc, memory, volumeID, ebsmetadata.Volume{ + VolumeID: volumeID, CapacityGiB: 10, State: "in-use", AttachedInstance: "i-0123456789abcdef0", + }) + + failing := &getFailingStore{MemoryObjectStore: memory, failKey: metadataKey(t, volumeID)} + svc.store = failing + svc.metadata = ebsmetadata.NewStore(failing, "test-bucket") + + _, err := svc.ForceDetachVolume(context.Background(), &ec2.DetachVolumeInput{ + VolumeId: aws.String(volumeID), + }, testVolAccountID) + require.Error(t, err) + assert.Equal(t, awserrors.ErrorServerInternal, awserrors.ValidErrorCodeFromError(err)) + + meta, getErr := ebsmetadata.NewStore(memory, "test-bucket").GetVolume(context.Background(), testVolAccountID, volumeID) + require.NoError(t, getErr) + assert.Equal(t, "in-use", meta.State, "a failed force-detach must not clear the attachment") + assert.Equal(t, "i-0123456789abcdef0", meta.AttachedInstance) + + _, err = svc.ForceDetachVolume(context.Background(), &ec2.DetachVolumeInput{ + VolumeId: aws.String("vol-doesnotexist00"), + }, testVolAccountID) + assert.Equal(t, awserrors.ErrorInvalidVolumeNotFound, awserrors.ValidErrorCodeFromError(err)) + + _, err = svc.ForceDetachVolume(context.Background(), &ec2.DetachVolumeInput{ + VolumeId: aws.String(volumeID), + }, "999999999999") + assert.Equal(t, awserrors.ErrorInvalidVolumeNotFound, awserrors.ValidErrorCodeFromError(err)) +} diff --git a/spinifex/domains/ec2/volume/service_impl.go b/spinifex/domains/ec2/volume/service_impl.go index a560daaab..e40ca82b9 100644 --- a/spinifex/domains/ec2/volume/service_impl.go +++ b/spinifex/domains/ec2/volume/service_impl.go @@ -1056,7 +1056,10 @@ func (s *VolumeServiceImpl) ForceDetachVolume(ctx context.Context, input *ec2.De meta, err := s.metadata.GetVolume(ctx, accountID, volumeID) if err != nil { - return nil, errors.New(awserrors.ErrorInvalidVolumeNotFound) + if objectstore.IsNoSuchKeyError(err) { + return nil, errors.New(awserrors.ErrorInvalidVolumeNotFound) + } + return nil, awserrors.Errorf(awserrors.ErrorServerInternal, "get volume metadata %s: %v", volumeID, err) } if meta.TenantID != accountID { return nil, errors.New(awserrors.ErrorInvalidVolumeNotFound) @@ -1091,10 +1094,15 @@ func (s *VolumeServiceImpl) DeleteVolume(ctx context.Context, input *ec2.DeleteV return nil, err } - // AWS-faithful: an absent volume returns InvalidVolume.NotFound. + // AWS-faithful: an absent volume returns InvalidVolume.NotFound. Any other + // read failure means the document's state is unknown, so it must not be + // reported or treated as already deleted. meta, err := s.metadata.GetVolume(ctx, accountID, volumeID) if err != nil { - return nil, errors.New(awserrors.ErrorInvalidVolumeNotFound) + if objectstore.IsNoSuchKeyError(err) { + return nil, errors.New(awserrors.ErrorInvalidVolumeNotFound) + } + return nil, awserrors.Errorf(awserrors.ErrorServerInternal, "get volume metadata %s: %v", volumeID, err) } if meta.TenantID != accountID { return nil, errors.New(awserrors.ErrorInvalidVolumeNotFound)