Add CAS and callback retry for DAG progression - #62
Open
jenniferubah wants to merge 1 commit into
Open
Conversation
Assisted-By: Cursor AI Signed-off-by: Jennifer Ubah <cju.cipher@gmail.com>
jenniferubah
requested review from
NoamNakash,
gabriel-farache,
gciavarrini,
machacekondra and
tkiss28
September 4, 2026 21:16
PR Summary by QodoAdd CAS-guarded DAG progression and callback retries
AI Description
Diagram
High-Level Assessment
Files changed (8)
|
Code Review by Qodo
1. Provisioning claim blocks retries
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+90
to
+93
| claimed, err := s.store.Resource().UpdateStatusFrom(ctx, r.ID, | ||
| []string{types.ResourceStatusPending}, | ||
| types.ResourceStatusProvisioning, | ||
| ) |
There was a problem hiding this comment.
1. Provisioning claim blocks retries 🐞 Bug ☼ Reliability
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.
Agent Prompt
## 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
Comment on lines
+235
to
+238
| applied, err := s.store.Resource().UpdateStatusFrom(ctx, r.ID, | ||
| []string{types.ResourceStatusPendingDeletion}, | ||
| types.ResourceStatusDeleting, | ||
| ) |
There was a problem hiding this comment.
2. Delete claim blocks redispatch 🐞 Bug ☼ Reliability
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.
Agent Prompt
## 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
Comment on lines
+318
to
+323
| switch resource.Status { | ||
| case types.ResourceStatusFailed, | ||
| types.ResourceStatusPendingDeletion, | ||
| types.ResourceStatusDeleting, | ||
| types.ResourceStatusDeleted: | ||
| return nil |
There was a problem hiding this comment.
3. Failed callback skips teardown retry 🐞 Bug ☼ Reliability
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.
Agent Prompt
## 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
Comment on lines
+333
to
+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 |
There was a problem hiding this comment.
4. Deletion callback cannot retry 🐞 Bug ☼ Reliability
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.
Agent Prompt
## 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
Comment on lines
+238
to
+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) |
There was a problem hiding this comment.
5. Callback retries are unbounded 🐞 Bug ☼ Reliability
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.
Agent Prompt
## 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Assisted-By: Cursor AI