Skip to content

FLPATH-4769 | Fix status consumer race by persisting instance before dispatch #46

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jordigilh wants to merge 2 commits into from
Closed

FLPATH-4769 | Fix status consumer race by persisting instance before dispatch #46

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
279c395
Fix status consumer race by persisting instance before dispatch
jordigilh Aug 6, 2026
2bee540
fix(sp): drop synchronous provider status write, superseded by Status…
jordigilh Aug 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 22 additions & 11 deletions internal/sp/service/resource_manager/service_type_instance.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,32 +82,43 @@ func (s *InstanceService) CreateInstance(ctx context.Context, request *resource_
return nil, service.NewValidationError("spec.service_type must not be empty")
}

// Send request to provider endpoint with the resolved ID
providerResponse, err := s.createInstanceWithProvider(ctx, provider.Endpoint, request, instanceID)
if err != nil {
log.Error("Provider provisioning failed", "instance_id", *instanceID, "provider_name", providerName, "error", err)
return nil, service.NewProviderError(fmt.Sprintf("Error from Provider (%s): %v", providerName, err))
}

// Create instance in database
// Persist a placeholder row *before* dispatching to the provider. This closes
// the race where a status event for this instance (e.g. from StatusConsumer)
// arrives while the create request is still in flight: the row already
// exists, so the update applies instead of being silently and permanently
// discarded as ErrInstanceNotFound (see enhancements/sp-resource-manager and
// sp-resource-status-reader design docs).
instance := model.ServiceTypeInstance{
ID: *instanceID,
ProviderName: providerName,
ServiceType: serviceType,
Status: providerResponse.Status,
Status: rmstore.StatusPending,
Spec: request.Spec,
}

created, err := s.store.ServiceTypeInstance().Create(ctx, instance)
if err != nil {
log.Error("Failed to create instance in store", "instance_id", *instanceID, "error", err)
return nil, service.NewInternalError(fmt.Sprintf("failed to create database record for instance %s: %v", providerResponse.ID, err))
return nil, service.NewInternalError(fmt.Sprintf("failed to create database record for instance %s: %v", *instanceID, err))
Comment on lines 99 to +102

@qodo-code-review qodo-code-review Bot Aug 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Delete/create orphan race 🐞 Bug ≡ Correctness

Because CreateInstance now persists the row before calling the provider, a caller-supplied instance
ID can be deleted while the provider create request is still in flight. DeleteInstance treats
provider 404 as success and then HardDelete removes the DB row, so the provider can still finish
creating the resource and subsequent status events are Ack()’d as not-found, leaving an orphaned
provider instance with no DB record.
Agent Prompt 
### Issue description
`CreateInstance` now persists an instance row before dispatching to the provider. This makes the (potentially caller-supplied) instance ID visible to other API calls while the provider request is still in flight. In that window, `DeleteInstance` can:
1) successfully `Get()` the placeholder row,
2) call provider DELETE,
3) treat provider `404` as success,
4) `HardDelete` the DB record,
while the in-flight provider create may still succeed afterward, leaving an orphaned provider resource with no DB row and future status events being discarded.

### Issue Context
- The API supports a caller-supplied instance ID (`CreateInstanceParams.Id`), so a client can know the ID before `CreateInstance` returns.
- Provider deletes treat `404` as non-error.

### Fix Focus Areas
- internal/sp/service/resource_manager/service_type_instance.go[85-113]
- internal/sp/service/resource_manager/service_type_instance.go[217-371]

### Suggested fix direction
Implement a state-aware lifecycle guard so delete cannot “win” against an in-flight create:
- If the instance is still in the placeholder status (`StatusPending`), either:
  - reject non-deferred deletes with a conflict/try-later error, **or**
  - force the delete into deferred/scheduled deletion (do not hard-delete immediately), **or**
  - treat provider `404` as retryable (not success) when deleting a `StatusPending` instance, and schedule cleanup instead of hard-deleting.
- Ensure the DB record remains until you can conclusively determine the create outcome and/or guarantee the provider resource is gone.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

}

// Send request to provider endpoint with the resolved ID
if _, err := s.createInstanceWithProvider(ctx, provider.Endpoint, request, instanceID); err != nil {
log.Error("Provider provisioning failed", "instance_id", *instanceID, "provider_name", providerName, "error", err)
if delErr := s.store.ServiceTypeInstance().HardDelete(ctx, *instanceID); delErr != nil {
log.Error("Failed to roll back placeholder instance after provider failure", "instance_id", *instanceID, "error", delErr)
}
return nil, service.NewProviderError(fmt.Sprintf("Error from Provider (%s): %v", providerName, err))
}

// The row stays PENDING here - StatusConsumer owns every status transition
// from this point on. The provider's synchronous response isn't persisted:
// it's immediately superseded by StatusConsumer's async update in practice,
// and nothing downstream reads it off the create response.
log.Info("Instance created successfully",
"instance_id", created.ID,
"provider_name", providerName,
"status", providerResponse.Status,
"status", created.Status,
)
return ModelToAPI(created), nil
}
Expand Down
92 changes: 72 additions & 20 deletions internal/sp/service/resource_manager/service_type_instance_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
rmsvc "github.com/dcm-project/control-plane/internal/sp/service/resource_manager"
"github.com/dcm-project/control-plane/internal/sp/store"
"github.com/dcm-project/control-plane/internal/sp/store/model"
"github.com/dcm-project/control-plane/internal/sp/testutil"
"github.com/go-resty/resty/v2"
"github.com/google/uuid"
. "github.com/onsi/ginkgo/v2"
Expand Down Expand Up @@ -69,7 +70,7 @@ var _ = Describe("InstanceService", func() {
}
Expect(db.Create(&provider).Error).NotTo(HaveOccurred())

dataStore = store.NewStore(db)
dataStore = store.NewStore(db, store.WithServiceTypeInstanceRetry(testutil.FastServiceTypeInstanceRetry()...))
instanceService = rmsvc.NewInstanceService(dataStore, resty.New().
SetTimeout(5*time.Second).
SetRetryCount(0))
Expand Down Expand Up @@ -342,38 +343,88 @@ var _ = Describe("InstanceService", func() {
Expect(providerCalled).To(BeFalse())
})

It("returns internal error with instance ID when DB insert fails", func() {
var instanceID string
var providerCallCount int
mockProviderWithID := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
providerCallCount++
instanceID = uuid.New().String()

if providerCallCount == 1 {
sqlDB, _ := db.DB()
_ = sqlDB.Close()
}

It("applies a status update that arrives while the create request is still in flight to the provider (BAC-1)", func() {
// The mock provider's handler is the exact dispatch point: while the
// synchronous create call to the provider is still outstanding, simulate
// a status event for this same instance arriving via the real production
// code path (StatusConsumer.handleMessage calls this same store method).
// The handler runs on httptest's own goroutine, so the result must cross
// back to the test over a channel rather than a shared variable.
statusUpdateErrCh := make(chan error, 1)
mockProviderMidDispatch := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
instanceID := r.URL.Query().Get("id")
statusUpdateErrCh <- dataStore.ServiceTypeInstance().UpdateStatus(ctx, instanceID, "RUNNING", "provisioning started")
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(http.StatusOK)
_ = json.NewEncoder(w).Encode(map[string]string{
"id": instanceID,
"status": "PROVISIONING",
})
}))
defer mockProviderWithID.Close()
defer mockProviderMidDispatch.Close()

providerMidDispatch := model.Provider{
ID: uuid.New().String(),
Name: "provider-mid-dispatch",
ServiceType: "vm",
Endpoint: mockProviderMidDispatch.URL,
HealthStatus: model.HealthStatusReady,
}
Expect(db.Create(&providerMidDispatch).Error).NotTo(HaveOccurred())

req := &resource_manager.ServiceTypeInstance{
ProviderName: "provider-mid-dispatch",
Spec: map[string]interface{}{"cpu": 1, "service_type": "vm"},
}

_, err := instanceService.CreateInstance(ctx, req, nil)
Expect(err).NotTo(HaveOccurred())

statusUpdateErr := <-statusUpdateErrCh
Expect(statusUpdateErr).NotTo(HaveOccurred())
})

It("does not leave an orphaned instance visible after a provider failure (BAC-2)", func() {
mockProviderFailing := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
w.WriteHeader(http.StatusInternalServerError)
_, _ = w.Write([]byte(`{"error": "boom"}`))
}))
defer mockProviderFailing.Close()

providerWithID := model.Provider{
providerFailing := model.Provider{
ID: uuid.New().String(),
Name: "provider-db-fail",
Name: "provider-failing",
ServiceType: "vm",
Endpoint: mockProviderWithID.URL,
Endpoint: mockProviderFailing.URL,
HealthStatus: model.HealthStatusReady,
}
Expect(db.Create(&providerWithID).Error).NotTo(HaveOccurred())
Expect(db.Create(&providerFailing).Error).NotTo(HaveOccurred())

specifiedID := uuid.New().String()
req := &resource_manager.ServiceTypeInstance{
ProviderName: "provider-db-fail",
ProviderName: "provider-failing",
Spec: map[string]interface{}{"cpu": 1, "service_type": "vm"},
}

_, err := instanceService.CreateInstance(ctx, req, &specifiedID)
Expect(err).To(HaveOccurred())

_, err = instanceService.GetInstance(ctx, specifiedID, false)
Expect(err).To(HaveOccurred())
var svcErr *service.ServiceError
Expect(err).To(BeAssignableToTypeOf(svcErr))
errors.As(err, &svcErr)
Expect(svcErr.Code).To(Equal(service.ErrCodeNotFound))
})

It("returns internal error naming the instance id and never contacts the provider when the DB persist fails (BAC-4)", func() {
// Drop only the service_type_instances table so the provider lookup
// (a separate table) still succeeds, and the failure is isolated to the
// initial persist step - before the provider is ever dispatched to.
Expect(db.Migrator().DropTable(&model.ServiceTypeInstance{})).To(Succeed())

req := &resource_manager.ServiceTypeInstance{
ProviderName: "test-provider",
Spec: map[string]interface{}{"cpu": 2, "service_type": "vm"},
}

Expand All @@ -385,7 +436,8 @@ var _ = Describe("InstanceService", func() {
errors.As(err, &svcErr)
Expect(svcErr.Code).To(Equal(service.ErrCodeInternal))
Expect(svcErr.Message).To(ContainSubstring("failed to create database record"))
Expect(svcErr.Message).To(ContainSubstring(instanceID))
Expect(svcErr.Message).To(MatchRegexp(`[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}`))
Expect(providerCalled).To(BeFalse())
})
})

Expand Down
8 changes: 8 additions & 0 deletions internal/sp/store/resource_manager/service_instance.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,14 @@ const (
DeletionStatusPendingProvider = "PENDING_PROVIDER"
)

// StatusPending is the placeholder status an instance record is created with
// before its create request is dispatched to the provider. Persisting this
// row first (rather than after a successful dispatch) closes the window
// where a status event for the instance could arrive before any record of
// it exists - see enhancements/sp-resource-manager and
// sp-resource-status-reader design docs.
const StatusPending = "PENDING"

func (s *ServiceTypeInstanceStore) MarkForDeletion(ctx context.Context, id string) error {
now := time.Now()
result := s.db.WithContext(ctx).
Expand Down
Loading