Add CAS and callback retry for DAG progression

[jenniferubah]

Summary

• Add UpdateStatusFrom to the placement store for atomic status transitions
• Harden create/delete callbacks with CAS guards and a PROVISIONING claim state to prevent duplicate SPRM creates and races on redelivered events
• NAK status/deletion consumers on retryable placement callback failures so transient errors redeliver

Assisted-By: Cursor AI

[qodo-code-review]

PR Summary by Qodo

Add CAS-guarded DAG progression and callback retries

🐞 Bug fix ✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Add atomic CAS lifecycle transitions and a PROVISIONING claim for idempotent DAG progression.
• Redeliver status and deletion events when placement callbacks fail transiently.
• Cover duplicate callbacks and conditional store transitions with regression tests.

Diagram

sequenceDiagram
    participant J as JetStream
    participant C as SP Consumer
    participant P as Placement Service
    participant S as Resource Store
    participant D as Database
    participant R as SPRM
    J->>C: Status event
    C->>P: Lifecycle callback
    P->>S: CAS transition
    S->>D: Conditional update
    D-->>S: Rows affected
    S-->>P: Applied flag
    alt CAS accepted
        P->>R: Create or delete
        R-->>P: Result
    else Duplicate callback
        P-->>C: Safe no-op
    end
    opt Retryable callback error
        P-->>C: Service error
        C-->>J: Delayed NAK
    end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Recoverable provisioning lease

• ➕ Allows another delivery to reclaim resources stranded after a post-CAS transient failure.
• ➕ Preserves the lightweight database-claim model used by this PR.
• ➖ Requires lease expiry, ownership metadata, and recovery rules.
• ➖ Introduces timing considerations for slow SPRM operations.

2. Transactional callback inbox and outbox

• ➕ Separates message acknowledgement from durable callback and SPRM dispatch progress.
• ➕ Supports reliable retries after local state has already been finalized.
• ➖ Requires additional tables, workers, and operational monitoring.
• ➖ Substantially increases implementation and migration complexity.

Recommendation: Keep the atomic UpdateStatusFrom primitive and explicit retry classification, but pair them with recoverable or idempotent dispatch. A callback can fail after PENDING becomes PROVISIONING, causing redelivery to skip the claimed resource; similarly, a deletion acknowledgement may finalize the SP record before its placement callback fails, preventing redelivery from invoking that callback again. A durable inbox/outbox is the strongest guarantee, while a lease plus idempotent SPRM operations is the smaller incremental option.

Files changed (8) +296 / -8

Enhancement (3) +34 / -0

errors.goClassify placement callback errors for redelivery +16/-0

Classify placement callback errors for redelivery

• Adds IsCallbackRetryable to distinguish terminal domain and client errors from transient callback failures that should trigger message redelivery.

internal/placement/service/errors.go

resource.goAdd atomic conditional resource status updates +17/-0

Add atomic conditional resource status updates

• Extends the resource store interface with UpdateStatusFrom and implements it as a single conditional database update. The method reports whether the expected source state matched.

internal/placement/store/resource.go

status.goDefine the PROVISIONING resource lifecycle state +1/-0

Define the PROVISIONING resource lifecycle state

• Adds the intermediate PROVISIONING status used to claim resources before dispatching an SPRM create operation.

internal/placement/types/status.go

Bug fix (3) +149 / -8

callbacks.goGuard lifecycle callbacks and DAG dispatch with CAS transitions +120/-8

Guard lifecycle callbacks and DAG dispatch with CAS transitions

• Replaces unconditional lifecycle status writes with allowed-source conditional transitions. Adds a PROVISIONING claim before SPRM creation and makes running, failed, deleted, and reverse-deletion callbacks tolerate duplicate or stale events.

internal/placement/service/callbacks.go

consumer.goRedeliver retryable placement status callback failures +19/-0

Redeliver retryable placement status callback failures

• Classifies running, deleted, and failed callback errors and issues a five-second delayed NAK for retryable failures. Non-retryable failures continue to be logged before the message is acknowledged.

internal/sp/consumer/consumer.go

response_consumer.goRetry placement failures from deletion acknowledgements +10/-0

Retry placement failures from deletion acknowledgements

• Applies the shared retryability classification to finalized deletion callbacks and negatively acknowledges transient failures with a five-second delay.

internal/sp/consumer/response_consumer.go

Tests (2) +113 / -0

placement_test.goVerify duplicate lifecycle callbacks remain idempotent +75/-0

Verify duplicate lifecycle callbacks remain idempotent

• Adds regression coverage ensuring repeated running callbacks do not duplicate SPRM creates and repeated failure callbacks do not restart deletion.

internal/placement/service/placement_test.go

resource_test.goTest conditional status transition semantics +38/-0

Test conditional status transition semantics

• Verifies matching transitions apply, mismatched transitions remain unchanged, and empty source-status sets return false without error.

internal/placement/store/resource_test.go

[qodo-code-review]

Code Review by Qodo

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

1. Provisioning claim blocks retries ✓ Resolved 🐞 Bug ☼ Reliability

Description

After claiming a ready resource as PROVISIONING, OnResourceRunning leaves it there when policy,
store, or SPRM creation returns a retryable error. Redelivery cannot retry creation because DAG
readiness only selects PENDING resources, permanently stranding the run.

Code

internal/placement/service/callbacks.go[R90-93]

+		claimed, err := s.store.Resource().UpdateStatusFrom(ctx, r.ID,
+			[]string{types.ResourceStatusPending},
+			types.ResourceStatusProvisioning,
+		)

Evidence

The new CAS changes the resource to PROVISIONING before multiple operations that can return
errors, including CreateResource. Those paths do not restore a retryable state, while readiness
explicitly excludes every status except PENDING.

internal/placement/service/callbacks.go[88-160]
internal/placement/service/orchestration.go[14-58]
internal/placement/service/errors.go[164-196]

Agent prompt

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

## Issue description
A resource is changed from `PENDING` to `PROVISIONING` before fallible policy, persistence, and SPRM operations. Retryable failures leave it excluded from future DAG readiness, so redelivery cannot retry creation.

## Issue Context
The retry design must still prevent concurrent callbacks from issuing duplicate creates. Use a recoverable claim/lease or an idempotent dispatch mechanism; safely release the claim on retryable failures where appropriate.

## Fix Focus Areas
- internal/placement/service/callbacks.go[88-160]
- internal/placement/service/orchestration.go[14-58]

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

---

2. Deletion callback cannot retry ✓ Resolved 🐞 Bug ☼ Reliability

Description

handleDeletionAcknowledged finalizes or hard-deletes the SP instance before calling placement,
then NAKs when placement fails. On redelivery the finalized instance makes deletionFinalized
false—or is no longer found—so onDeleted is skipped and the message is ACKed without retrying DAG
progression.

Code

internal/sp/consumer/response_consumer.go[R333-340]

+			if placementservice.IsCallbackRetryable(err) {
+				slog.Warn("deletion-acknowledged: placement OnResourceDeleted callback failed with retryable error, nacking",
+					"instance_id", data.ResourceID,
+					"event_type", messaging.CETypeDeletionAcknowledged,
+					"error", err,
+				)
+				_ = msg.NakWithDelay(5 * time.Second)
+				return

Evidence

The instance is hard-deleted or marked complete before onDeleted runs. The callback is guarded by
the delivery-local deletionFinalized flag; on hard-delete redelivery, not-found is immediately
ACKed, while completed tombstones fall through without setting the flag.

internal/sp/consumer/response_consumer.go[275-330]
internal/sp/consumer/response_consumer.go[331-349]
internal/placement/service/callbacks.go[166-200]

Agent prompt

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

## Issue description
The handler NAKs a placement callback failure only after irreversibly finalizing the local deletion. Redelivery cannot reconstruct that the placement callback still needs to run.

## Issue Context
Persist callback-delivery state or use a transactional outbox so local deletion finalization and placement notification can be retried independently. Do not infer callback completion solely from the current instance state.

## Fix Focus Areas
- internal/sp/consumer/response_consumer.go[275-349]
- internal/placement/service/callbacks.go[166-200]

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

---

3. Delete claim blocks redispatch ✓ Resolved 🐞 Bug ☼ Reliability

Description

progressRunDeletion persists DELETING before calling SPRM but leaves that status unchanged when
DeleteResource fails. Redelivery reaches progression again, sees the existing DELETING resource
as an in-flight barrier, and returns without redispatching the failed delete.

Code

internal/placement/service/callbacks.go[R235-238]

+			applied, err := s.store.Resource().UpdateStatusFrom(ctx, r.ID,
+				[]string{types.ResourceStatusPendingDeletion},
+				types.ResourceStatusDeleting,
+			)

Evidence

The new CAS commits PENDING_DELETION → DELETING before the SPRM call. Non-404 errors return
without changing the status, and the next progression call immediately exits whenever any resource
remains DELETING.

internal/placement/service/callbacks.go[202-218]
internal/placement/service/callbacks.go[233-285]
internal/placement/service/orchestration.go[79-102]

Agent prompt

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

## Issue description
A transient SPRM deletion failure leaves the resource in `DELETING`. Subsequent callback deliveries cannot retry the request because deletion progression treats every `DELETING` resource as still in flight.

## Issue Context
Preserve duplicate-dispatch protection while making failed claims recoverable, for example through a lease/attempt state or an idempotent deletion redispatch mechanism.

## Fix Focus Areas
- internal/placement/service/callbacks.go[202-218]
- internal/placement/service/callbacks.go[233-285]

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

---

View high (1)

4. Failed callback skips teardown retry ✓ Resolved 🐞 Bug ☼ Reliability

Description

When the first OnResourceFailed invocation changes the resource to FAILED but DeleteRun then
fails, redelivery returns successfully for FAILED or deletion states without retrying teardown.
The consumer therefore ACKs the redelivery while the run remains only partially deleted.

Code

internal/placement/service/callbacks.go[R318-323]

+		switch resource.Status {
+		case types.ResourceStatusFailed,
+			types.ResourceStatusPendingDeletion,
+			types.ResourceStatusDeleting,
+			types.ResourceStatusDeleted:
+			return nil

Evidence

The first successful CAS is followed by DeleteRun, which can fail while listing, updating, or
dispatching deletion. On redelivery the added switch returns nil for FAILED, PENDING_DELETION,
and DELETING, so DeleteRun is never retried and the consumer ACKs.

internal/placement/service/callbacks.go[291-328]
internal/placement/service/placement.go[236-256]
internal/sp/consumer/consumer.go[259-272]

Agent prompt

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

## Issue description
Redelivered failed-resource callbacks return early once the initial status transition has happened, even when the prior teardown attempt returned a retryable error. This defeats the newly added NAK behavior.

## Issue Context
Make failure handling idempotently resume the appropriate teardown phase for `FAILED`, `PENDING_DELETION`, and recoverable deletion states rather than treating all of them as completed.

## Fix Focus Areas
- internal/placement/service/callbacks.go[291-328]
- internal/placement/service/placement.go[236-256]
- internal/sp/consumer/consumer.go[259-270]

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

---

5. Callback retries are unbounded ✓ Resolved 🐞 Bug ☼ Reliability

Description

The new NAK paths retry every error classified as internal, policy, SPRM, unavailable, or
non-ServiceError, but the JetStream consumer has no delivery limit. Permanently failing cases such
as an unrecognized policy HTTP error can therefore redeliver every five seconds indefinitely.

Code

internal/sp/consumer/consumer.go[R238-241]

+				if placementservice.IsCallbackRetryable(err) {
+					slog.Warn("Placement OnResourceRunning failed with retryable error, nacking",
+						"instance_id", payload.Id, "error", err)
+					_ = msg.NakWithDelay(5 * time.Second)

Evidence

The added callback branches explicitly NAK with delay, while the durable consumer configuration only
sets its name and explicit acknowledgement policy. IsCallbackRetryable defaults to true for
non-service errors and all service codes outside its narrow denylist; policy HTTP statuses outside
the explicit mappings become retryable ErrCodePolicyError.

internal/sp/consumer/consumer.go[110-130]
internal/sp/consumer/consumer.go[230-272]
internal/placement/service/errors.go[110-123]
internal/placement/service/errors.go[140-161]

Agent prompt

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

## Issue description
Retryable callback errors are NAKed indefinitely because the consumer has no maximum delivery count. Permanent failures in broadly retryable error classes become poison messages.

## Issue Context
Configure a bounded retry policy and define terminal handling such as logging, metrics, or a dead-letter subject. Apply the same policy consistently to status and response consumers.

## Fix Focus Areas
- internal/sp/consumer/consumer.go[110-130]
- internal/sp/consumer/consumer.go[230-270]
- internal/sp/consumer/response_consumer.go[331-349]
- internal/placement/service/errors.go[110-123]

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

---

Context sources

Review mode: ⚖️ Balanced: Downgraded extended -> standard: change is below the extended eligibility bar (hunks 16/18, lines 304/200; both must reach the floor). Router rationale: This is a concurrency- and retry-sensitive behavioral change spanning placement state transitions, DAG progression, callback idempotency, and multiple consumers, with many independent logic sites where subtle race and redelivery defects could be missed in one pass.

Tip of the day

💡 Did you know, you can commit Qodo's fix in one click with committable suggestions (GitHub & GitLab)

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[gciavarrini]

lgtm