Add CAS and callback retry for DAG progression - #62
Conversation
Assisted-By: Cursor AI Signed-off-by: Jennifer Ubah <cju.cipher@gmail.com>
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.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Assisted-By: Cursor AI Signed-off-by: Jennifer Ubah <cju.cipher@gmail.com>
|
|
||
| refs, err := cel.CollectReferences(r.Spec) | ||
| if err != nil { | ||
| return NewValidationError(fmt.Sprintf("resource %s: %v", r.Name, err)) |
There was a problem hiding this comment.
Before this PR, CEL validation ran while the resource was still PENDING. A validation error returned early but the resource stayed eligible for the next DAG progression attempt.
This PR adds the PROVISIONING claim first, then runs CollectReferences.
On ValidationError the claim is not released, so the resource stays PROVISIONING. Readiness only picks PENDING resources, so the run is stuck in a state that did not exist before.
Not sure if it's better moving CEL validation before the claim, or call releaseProvisioningClaim on terminal errors here too.
There was a problem hiding this comment.
Reverted it back to Pre-PR behavior, CEL validation before claim so that failed validation leave resource in Pending state. See c299ba8
| "error", err, | ||
| ) | ||
| return handleSPRMError(err) | ||
| svcErr := handleSPRMError(err) |
There was a problem hiding this comment.
Similar to my previous comment. releaseProvisioningClaim runs only for retryable SPRM errors. A terminal 4xx leaves the resource in PROVISIONING with no redelivery path.
| } | ||
| } else { | ||
| return handleSPRMError(err) | ||
| svcErr := handleSPRMError(err) |
There was a problem hiding this comment.
Same pattern on delete. A non retryable SPRM delete error keeps the resource in DELETING, and progressRunDeletion waits on any DELETING resource before advancing.
Should we rolling back to PENDING_DELETION or surfacing FAILED for terminal delete errors?
There was a problem hiding this comment.
I think we want to rollback to PENDING_DELETION for this as well. Updated, c299ba8
| // releaseProvisioningClaim rolls a failed create progression back to PENDING so | ||
| // a redelivered RUNNING callback can retry SPRM dispatch. | ||
| func releaseProvisioningClaim(ctx context.Context, resources store.Resource, resourceID string) { | ||
| _, _ = resources.UpdateStatusFrom(ctx, resourceID, |
There was a problem hiding this comment.
releaseProvisioningClaim and releaseDeletionDispatch ignore the CAS result.
If the rollback update does not apply, the consumer may still NAK while the resource stays in the claim state. I think at least a log is necessary or propagate a failed rollback
There was a problem hiding this comment.
Added a logger for rollback falilure: c299ba8
| return false | ||
| } | ||
| switch svcErr.Code { | ||
| case ErrCodeInternal, ErrCodeUnavailable, ErrCodeSPRMError, ErrCodePolicyInternalError: |
There was a problem hiding this comment.
handlePolicyError maps client communication failures to ErrCodePolicyError, which is outside the retryable set. That path also skips claim release in OnResourceRunning.
Worth treating policy transport errors like internal errors for callback retry, or releasing the claim when policy is unreachable. WDYT?
| Expect(deleteCalls).To(BeNumerically(">=", firstDeleteCalls)) | ||
| }) | ||
|
|
||
| It("releases PROVISIONING and retries SPRM create after a retryable failure", func() { |
There was a problem hiding this comment.
Please add a test that a retryable SPRM delete failure rolls DELETING back to PENDING_DELETION and retries on redelivery
Assisted-By: Cursor AI Signed-off-by: Jennifer Ubah <cju.cipher@gmail.com>
2 participants
Summary
Assisted-By: Cursor AI