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

[jordigilh]

Summary

InstanceService.CreateInstance dispatched the create request to the provider's REST endpoint before persisting the instance's own DB row. Any status event for that instance (e.g. from StatusConsumer) arriving while the create request was still in flight found no record, hit ErrInstanceNotFound, and was unconditionally Ack()'d — silently and permanently discarded, with no MaxDeliver/backoff configured to self-heal it.

Fixes #44. Closes FLPATH-4769.

Why this approach (reorder, not bounded JetStream retry)

The issue proposed two options. This isn't a coin-flip between two valid architectures — the already-approved (7-reviewer) design docs for this exact subsystem mandate persist-before-dispatch, so today's code is a bug relative to spec, not an open design question:

• enhancements/sp-resource-manager/sp-resource-manager.md — the creation-flow sequence diagram and steps both place "Creates an instance record in the database with status PENDING" before "Publishes a creation request CloudEvent to the agent's topic".
• enhancements/sp-resource-status-reader/sp-resource-status-reader.md states the consumer "Updates existing instance records created during POST /api/v1/service-type-instances", and that a not-found is only expected "when an instance has been deleted before the status event was processed or when events arrive out of order" — i.e. it assumes the record already exists, which today's ordering violates.
• Same-repo precedent: the deletion path (MarkForDeletion → persist SCHEDULED → dispatch via background Scheduler) already follows persist-then-act.

Rejected alternative (bounded JetStream Nak/MaxDeliver retry): technically feasible, but no design doc anywhere prescribes bus-level redelivery for this consumer, and UpdateStatus has no sequencing guard — a redelivered stale event could regress status backward if a newer legitimate event already landed in the meantime. Reordering closes the race at its structural source instead of papering over it with retries.

What changed

internal/sp/service/resource_manager/service_type_instance.go CreateInstance:

1. Persist a PENDING placeholder row before dispatching to the provider.
2. Dispatch to the provider (unchanged).
3. On provider failure: HardDelete the placeholder (preserves the existing "no artifact on failed create" contract) before returning the error.
4. On provider success: the row stays PENDING. StatusConsumer owns every subsequent transition — the provider's synchronous response status is no longer persisted (see review discussion: per-type status vocabularies are well-defined and providers follow them correctly, but the value is designed to lose to a real async status event anyway, which lands within ~100ms-2s in practice, and nothing downstream reads it off the create response).

Tests (TDD, BAC-driven)

All new/changed tests stay at the same tier as the existing suite (Ginkgo component tests, in-memory SQLite, httptest mock provider) — the race is closed structurally inside CreateInstance, so no integration/E2E coverage is needed and the test pyramid shape is preserved.

• BAC-1 (the fix): a status update arriving while the create request is still in flight is applied, not lost — reproduces the bug against unmodified code (fails with ErrInstanceNotFound), passes after the fix.
• BAC-2 (existing contract, preserved): a provider failure leaves no orphaned instance visible via GetInstance.
• BAC-4 (existing contract, relocated): a DB persist failure means the provider is never contacted, and the error names the instance id — rewritten because the failure point moved from post-dispatch to pre-dispatch.
• BAC-3 (existing "creates a new instance" test) and all other existing CreateInstance tests are unchanged and remain green.

No changes needed to internal/sp/consumer/consumer.go or its tests — the fix closes the race at the source, so the consumer's existing "unconditional Ack on not-found" behavior becomes correct as originally designed.

Full internal/sp suite passes with -race.

---

Assisted-By: Claude (Anthropic)

Made with Cursor

[qodo-code-review]

PR Summary by Qodo

Fix create/status race by persisting instance before provider dispatch

🐞 Bug fix 🧪 Tests ⚙️ Configuration changes 🕐 20-40 Minutes

AI Description

• Persist a PENDING instance row before provider create dispatch to prevent dropped status events.
• Apply provider response status only if the instance is still PENDING.
• Add regression tests for mid-dispatch updates, provider failure rollback, and DB-persist failures.

Diagram

graph TD
A["API create request"] --> B["InstanceService.CreateInstance"] --> C[("ServiceTypeInstance DB")]
B --> D["Provider REST"] --> B
B --> E["UpdateStatusIfPending"] --> C
F[("NATS JetStream")] --> G["StatusConsumer"] --> C
subgraph Legend
  direction LR
  _svc["Service"] ~~~ _db[("Database")] ~~~ _bus[("Message bus")] ~~~ _ext["External API"]
end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. JetStream bounded retry (Nak/MaxDeliver + backoff) on not-found

• ➕ Could self-heal transient ordering races without changing create flow
• ➕ Keeps producer-side logic unchanged
• ➖ Design docs mandate persist-before-dispatch, so this treats a spec violation as normal
• ➖ Redelivery can apply stale events and regress status without additional sequencing/monotonic guards
• ➖ Still fails if not-found is permanent (e.g., delete) unless differentiated robustly

2. Transactional outbox / publish-after-commit

• ➕ Strongest ordering guarantee between DB state and dispatched events
• ➕ General pattern that scales to more workflows
• ➖ Significantly more infrastructure and moving parts for a targeted race
• ➖ Requires background publisher semantics and operational monitoring

Recommendation: Keep the PR’s approach: persist-before-dispatch aligns with the approved design docs and structurally removes the race. The added UpdateStatusIfPending guard is a pragmatic complement that prevents provider synchronous responses from overwriting newer async status events, avoiding the need for bus-level retries or more heavyweight outbox infrastructure.

Files changed (5) +336 / -29

Bug fix (2) +68 / -11

service_type_instance.goFix CreateInstance race by persisting placeholder before provider dispatch +40/-11

Fix CreateInstance race by persisting placeholder before provider dispatch

• Reorders CreateInstance to create a PENDING instance row before calling the provider, preventing status events from being dropped due to missing rows. On provider failure, hard-deletes the placeholder to preserve the no-artifact-on-failed-create behavior. Adds conditional post-dispatch status application via UpdateStatusIfPending and refreshes from DB when pre-empted by newer async status.

internal/sp/service/resource_manager/service_type_instance.go

service_instance.goAdd conditional status update helper and PENDING placeholder constant +28/-0

Add conditional status update helper and PENDING placeholder constant

• Extends the ServiceTypeInstance store interface with UpdateStatusIfPending and implements it as an atomic conditional update (WHERE id AND status). Introduces a StatusPending constant to standardize the placeholder status used during create-before-dispatch.

internal/sp/store/resource_manager/service_instance.go

Tests (2) +116 / -18

monitor_test.goUpdate healthcheck test mock to satisfy extended instance store interface +4/-0

Update healthcheck test mock to satisfy extended instance store interface

• Adds a stub UpdateStatusIfPending method to the mockInstanceStore used by Monitor tests, keeping the test suite compiling after the store interface change.

internal/sp/healthcheck/monitor_test.go

service_type_instance_test.goAdd regression tests for mid-dispatch status handling and rollback behavior +112/-18

Add regression tests for mid-dispatch status handling and rollback behavior

• Extends CreateInstance tests to simulate a status update arriving during the provider call and asserts it is applied. Verifies provider response cannot regress a newer status, placeholder rows are removed on provider failure, and DB persist failures return an internal error without contacting the provider.

internal/sp/service/resource_manager/service_type_instance_test.go

Other (1) +152 / -0

hindsight-memory.mdcAdd Cursor rule to enforce DCM-tagged memory recall gating +152/-0

Add Cursor rule to enforce DCM-tagged memory recall gating

• Introduces a project-specific Cursor rule describing when/how to recall Hindsight memory with strict DCM tags. Documents mandatory recall gates and tool usage guidance to prevent cross-project memory contamination.

.cursor/rules/hindsight-memory.mdc

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📜 Skill insights (0)

1. Delete/create orphan race 🐞 Bug ≡ Correctness

Description

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.

Code

internal/sp/service/resource_manager/service_type_instance.go[R99-102]

	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))

Evidence

The PR moved DB persistence ahead of provider dispatch, which makes the instance row visible
earlier. The service supports caller-supplied IDs, and the delete path hard-deletes the DB record
even when provider DELETE returns 404; afterward, StatusConsumer will Ack() not-found updates, so
any later status events for the provider-created resource are permanently dropped.

internal/sp/service/resource_manager/service_type_instance.go[48-113]
internal/sp/service/resource_manager/service_type_instance.go[303-325]
internal/sp/service/resource_manager/service_type_instance.go[217-300]
internal/sp/service/resource_manager/service_type_instance.go[353-371]
api/sp/v1alpha1/resource_manager/types.gen.go[118-123]
internal/sp/consumer/consumer.go[145-164]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### 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

---

2. Rollback deletes updated row 🐞 Bug ☼ Reliability

Description

On provider create errors, CreateInstance unconditionally HardDelete’s the placeholder row even
though the new flow explicitly allows real status events to update that row while the provider call
is in flight. For ambiguous failures (e.g., timeouts/connection errors or provider-side acceptance
with client-side error), this can erase the only tracking record for a potentially-created resource
and causes later status events to be permanently discarded as ErrInstanceNotFound.

Code

internal/sp/service/resource_manager/service_type_instance.go[R106-110]

+	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)
+		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)

Evidence

The PR’s new flow persists the row first and then calls the provider; it also explicitly handles the
case where a newer status event landed before the provider response. Despite that, the provider
error branch still hard-deletes the instance row, and StatusConsumer will Ack() updates for missing
rows, permanently discarding later events.

internal/sp/service/resource_manager/service_type_instance.go[85-134]
internal/sp/consumer/consumer.go[150-160]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`CreateInstance` now persists a placeholder instance record and allows async status updates to land during the provider create call. However, on provider error it unconditionally `HardDelete`s the row. If the provider error is ambiguous (network/timeout) and the provider actually proceeded (or if a real status event already updated the row), this rollback deletes meaningful lifecycle state and makes subsequent status events get Ack()'d as not-found.

### Issue Context
- The PR already introduced `UpdateStatusIfPending` to prevent the synchronous provider response from overwriting newer async state, acknowledging mid-flight status arrival.
- The same mid-flight arrival can happen before the provider call returns an error.

### Fix Focus Areas
- internal/sp/service/resource_manager/service_type_instance.go[105-134]
- internal/sp/consumer/consumer.go[150-160]

### Suggested fix direction
Make rollback state-aware:
- Before deleting on provider error, re-check the DB row:
 - If it is still `StatusPending` (and not otherwise marked), then rollback-delete as today.
 - If it has moved past `StatusPending` (or has other indicators that async processing has begun), do **not** hard-delete; instead leave the record for reconciliation and/or mark a failure state that can be healed by later status events.
- Optionally introduce a dedicated “create_in_flight/dispatch_started” marker to distinguish placeholder from real provider-emitted statuses that might coincidentally equal `PENDING`.

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

---

Context used

⚠️ Tickets: not configured — ticket URL found in PR but could not be fetched — check ticket provider credentials

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[qodo-code-review]

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

[qodo-code-review]

2. Rollback deletes updated row 🐞 Bug ☼ Reliability

On provider create errors, CreateInstance unconditionally HardDelete’s the placeholder row even
though the new flow explicitly allows real status events to update that row while the provider call
is in flight. For ambiguous failures (e.g., timeouts/connection errors or provider-side acceptance
with client-side error), this can erase the only tracking record for a potentially-created resource
and causes later status events to be permanently discarded as ErrInstanceNotFound.

Agent Prompt

### Issue description
`CreateInstance` now persists a placeholder instance record and allows async status updates to land during the provider create call. However, on provider error it unconditionally `HardDelete`s the row. If the provider error is ambiguous (network/timeout) and the provider actually proceeded (or if a real status event already updated the row), this rollback deletes meaningful lifecycle state and makes subsequent status events get Ack()'d as not-found.

### Issue Context
- The PR already introduced `UpdateStatusIfPending` to prevent the synchronous provider response from overwriting newer async state, acknowledging mid-flight status arrival.
- The same mid-flight arrival can happen before the provider call returns an error.

### Fix Focus Areas
- internal/sp/service/resource_manager/service_type_instance.go[105-134]
- internal/sp/consumer/consumer.go[150-160]

### Suggested fix direction
Make rollback state-aware:
- Before deleting on provider error, re-check the DB row:
  - If it is still `StatusPending` (and not otherwise marked), then rollback-delete as today.
  - If it has moved past `StatusPending` (or has other indicators that async processing has begun), do **not** hard-delete; instead leave the record for reconciliation and/or mark a failure state that can be healed by later status events.
- Optionally introduce a dedicated “create_in_flight/dispatch_started” marker to distinguish placeholder from real provider-emitted statuses that might coincidentally equal `PENDING`.

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

[jordigilh]

Draft, not seeking review yet.

This PR fixes the original status-consumer race (closes #44), but reviewing this diff surfaced two additional correctness gaps: a delete/create orphan race, and a rollback that can discard a status update that already landed. Closing those requires a new correctness contract that isn't in the design doc yet, so the fix is gated on documenting it first: dcm-project/enhancements#102.

Holding this PR in draft until that enhancement PR is approved/merged. Once it lands, I'll push the additional commits here, update the description, and mark this ready for review.

Tracking: FLPATH-4769

[jenniferubah]

Not sure why we need to update it from Pending to the provider's status. Initially, we agreed providers will send PROVISINONING as response status on acceptance but since we are moving the setting it wihin the create function, I believe we don't need to persist the provider's status here. Also, we will be moving to publishing for CloudEvents for the create and delete.

[jordigilh]

Agreed. The per-type status vocabularies are actually well-defined and each SP follows theirs correctly, so the value itself isn't the issue — but UpdateStatusIfPending exists specifically to lose to a real status event, and nothing downstream reads it. Not worth the complexity, especially given the CloudEvent move.

Dropping the post-dispatch UpdateStatusIfPending call/branching and BAC-5 (it only existed to guard that write). Rollback-on-failure stays.

[jordigilh]

Closing as this gap will be covered this milestone.