Skip to content

fix(sp): unify agent-routed instance delete to enroll-before-publish - #63

Open
gabriel-farache wants to merge 1 commit into
dcm-project:mainfrom
gabriel-farache:fix/sp_deletion_unify
Open

fix(sp): unify agent-routed instance delete to enroll-before-publish#63
gabriel-farache wants to merge 1 commit into
dcm-project:mainfrom
gabriel-farache:fix/sp_deletion_unify

Conversation

@gabriel-farache

@gabriel-farache gabriel-farache commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Unify agent-routed instance delete to enroll-before-publish

Refects changes from dcm-project/enhancements#106

@gabriel-farache

Copy link
Copy Markdown
Contributor Author

/agentic_review

@qodo-code-review

qodo-code-review Bot commented Sep 10, 2026

Copy link
Copy Markdown

Code Review by Qodo

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

Grey Divider


Action required

1. Late agent acknowledgements are ignored 🐞 Bug ≡ Correctness ⭐ New
Description
handleDeletionAcknowledged treats an acknowledgement as pending only while deletion_status is
SCHEDULED (or the status is pending_deletion), so it ignores an acknowledgement after the
cleanup scheduler changes a hard-delete row to FAILED. A non-deferred request whose initial
publish fails deliberately remains in its original running status, and a delayed acknowledgement
after scheduler retries are exhausted therefore leaves a physically deleted resource represented by
a failed record instead of purging it.
Code

internal/sp/consumer/response_consumer.go[R296-298]

+	stillPending := instance.Status == model.StatusPendingDeletion ||
+		(instance.DeletionStatus != nil && *instance.DeletionStatus == rmstore.DeletionStatusScheduled)
+
Evidence
The new service behavior explicitly returns success while retaining a scheduled hard-delete row in
its original status when publication fails. The scheduler subsequently transitions scheduled rows at
the retry limit to FAILED, while the new consumer gate accepts neither that deletion status nor
the retained running status.

internal/sp/service/resource_manager/service_type_instance.go[363-371]
internal/sp/cleanup/scheduler.go[136-141]
internal/sp/consumer/response_consumer.go[296-317]
internal/sp/service/resource_manager/service_type_instance_test.go[455-480]

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

Issue description
`handleDeletionAcknowledged` ignores a valid deletion acknowledgement when the scheduler has already changed `deletion_status` from `SCHEDULED` to `FAILED`. This newly reachable path occurs when the synchronous non-deferred publish fails: the instance remains `running`, scheduler retries can later publish successfully, and an acknowledgement arriving after retry exhaustion cannot finalize the deletion.

Fix Focus Areas
- internal/sp/consumer/response_consumer.go[296-317]
- internal/sp/cleanup/scheduler.go[136-141]
- internal/sp/service/resource_manager/service_type_instance.go[363-371]

Recommended Fix
Allow an acknowledgement from the currently assigned agent to finalize the same deletion lifecycle after retry exhaustion, rather than requiring only `SCHEDULED` or `pending_deletion`. Preserve the protections against duplicate acknowledgements and acknowledgements from superseded agents; if necessary, make the terminal transition conditional on the current deletion lifecycle state.

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


2. Upgrades retain unwanted tombstones 🐞 Bug ≡ Correctness ⭐ New
Description
The new HardDelete column defaults every pre-existing row to false without backfilling in-flight
non-deferred enrollments from their prior status=deleting signal. During an upgrade with scheduled
hard deletes, acknowledgements and audit give-up therefore take the soft-completion path and retain
DELETED rows that should be purged.
Code

internal/sp/store/model/service_type_instance.go[53]

+	HardDelete bool `gorm:"column:hard_delete;not null;default:false"`
Evidence
The application automatically migrates ServiceTypeInstance, and the added field is non-null with a
false default. Both newly changed finalizers treat that field as the sole outcome selector, so
legacy scheduled rows receiving the default are completed as tombstones.

internal/sp/store/model/service_type_instance.go[43-53]
internal/sp/store/db.go[65-71]
internal/sp/consumer/response_consumer.go[296-317]
internal/sp/cleanup/scheduler.go[175-187]

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

## Issue description
Adding the non-null `hard_delete` column with a false default classifies every deletion already in progress during an upgrade as deferred, even when the previous implementation recorded a non-deferred delete through `status=deleting`.

## Fix Focus Areas
- internal/sp/store/model/service_type_instance.go[43-53]
- internal/sp/store/db.go[65-71]
- internal/app/db.go[72-83]

## Recommended Fix
Add an idempotent data migration that backfills `hard_delete=true` for legacy scheduled deletion rows whose prior durable state identifies them as non-deferred, before acknowledgement consumers or cleanup processing can run. Add an upgrade test that creates a row using the old schema/state, migrates it, and verifies that acknowledgement purges it.

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


3. Re-deletes can finalize the wrong mode 🐞 Bug ≡ Correctness ⭐ New
Description
handleDeletionAcknowledged and auditGiveUp choose purge versus tombstone from an unversioned
in-memory HardDelete snapshot, while their final store operations do not verify that the database
still has that mode or lifecycle state. If a concurrent re-delete changes the persisted mode through
ResetRetryCount after the worker read, the stale acknowledgement or scheduler cycle can preserve a
requested hard delete or purge a row after the latest deferred request selected tombstone retention.
Code

internal/sp/consumer/response_consumer.go[R296-300]

+	stillPending := instance.Status == model.StatusPendingDeletion ||
+		(instance.DeletionStatus != nil && *instance.DeletionStatus == rmstore.DeletionStatusScheduled)
+
	switch {
-	case instance.Status == model.StatusDeleting:
-		// Checked ahead of DeletionStatus: a non-deferred delete must
-		// always be fully removed once acknowledged, regardless of whether
-		// its best-effort MarkForDeletion enrollment also set SCHEDULED.
+	case stillPending && instance.HardDelete:
Evidence
The enrollment helper establishes that a re-delete with a different deferred value may update the
persisted deletion mode through ResetRetryCount. Both the consumer and scheduler then choose a
finalizer from an earlier snapshot—the scheduler iterates value copies returned by
ListPendingDeletions—while HardDeleteFromAgent, MarkDeletionCompleteFromAgent, HardDelete,
and MarkDeletionComplete do not predicate their writes on the mode that was read; the hard-delete
operation checks only the ID, and the acknowledgement variant additionally checks the agent name.

internal/sp/consumer/response_consumer.go[284-317]
internal/sp/cleanup/scheduler.go[82-95]
internal/sp/cleanup/scheduler.go[175-187]
internal/sp/store/resource_manager/service_instance.go[404-472]
internal/sp/store/resource_manager/service_instance.go[475-484]
internal/sp/service/resource_manager/service_type_instance.go[389-411]
internal/sp/cleanup/scheduler.go[175-184]
internal/sp/store/resource_manager/service_instance.go[439-472]
internal/sp/consumer/response_consumer.go[296-316]

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

## Issue description
Deletion finalization reads `HardDelete` from an unversioned snapshot, but a later `DeleteInstance` call may change the persisted deletion mode before the purge or tombstone operation runs. A stale scheduler or acknowledgement worker can therefore finalize according to the prior mode instead of the latest request.

## Fix Focus Areas
- internal/sp/consumer/response_consumer.go[284-317]
- internal/sp/cleanup/scheduler.go[175-187]
- internal/sp/service/resource_manager/service_type_instance.go[389-411]
- internal/sp/store/resource_manager/service_instance.go[418-484]

## Recommended Fix
Make mode selection and finalization atomic and conditional on the current deletion mode and lifecycle state in the database rather than on the worker's loaded model alone. Add mode-aware guarded store operations for hard deletion and soft completion, or use a transaction that locks and rereads the currently scheduled row before choosing the operation; alternatively, introduce a deletion generation or token. Use the same atomic mechanism from acknowledgement handling and scheduler give-up, treat a failed conditional update as a stale worker result rather than finalizing the newer request, and add concurrency tests where `ResetRetryCount` changes the mode after the initial read but before finalization.

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


View high (1)
4. Fast acknowledgements leave broken state ✓ Resolved 🐞 Bug ☼ Reliability
Description
DeleteInstance enrolls a non-deferred instance with only deletion_status=SCHEDULED and does not
durably set status=deleting until after synchronous publication, with publish failures and
post-publish UpdateStatus failures leaving the deletion mode unrecorded. A fast acknowledgement,
or one following a scheduler retry, then takes the deferred soft-completion branch and creates a
DELETED tombstone, which a racing ID-only status update can overwrite to deleting, while retries
may continue until acknowledgement.
Code

internal/sp/service/resource_manager/service_type_instance.go[R340-342]

+	if err := s.enrollForDeletion(ctx, instance); err != nil {
+		return err
+	}
Evidence
Enrollment sets deletion_status=SCHEDULED before publication, while the non-deferred
status=deleting update occurs afterward without a compare-and-swap guard; both a
non-agent-not-found publish failure and an UpdateStatus database failure can therefore leave the
row scheduled without its intended mode. The cleanup scheduler republishes scheduled rows, and the
acknowledgement consumer independently reads the row, hard-deleting only those already marked
deleting while soft-completing the others as tombstones; because UpdateStatus targets the row
solely by ID, it can also overwrite a tombstone produced by a fast acknowledgement.

internal/sp/service/resource_manager/service_type_instance.go[340-369]
internal/sp/messaging/publisher.go[93-103]
internal/sp/consumer/response_consumer.go[278-325]
internal/sp/store/resource_manager/service_instance.go[194-211]
internal/sp/store/resource_manager/service_instance.go[416-427]
internal/sp/cleanup/scheduler.go[100-156]
internal/sp/consumer/response_consumer.go[290-325]
internal/sp/store/resource_manager/service_instance.go[323-351]
internal/sp/cleanup/scheduler.go[90-156]

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 enroll-before-publish sequence exposes a scheduled non-deferred deletion before its hard-deletion mode is durably recorded. A fast acknowledgement, an initial publish failure followed by scheduler retry, or a successful publish followed by an `UpdateStatus` failure can leave the request classified as deferred, causing acknowledgement processing to preserve a tombstone; a racing post-publish update can also overwrite the completed lifecycle state.

## Fix Focus Areas
- internal/sp/service/resource_manager/service_type_instance.go[340-369]
- internal/sp/consumer/response_consumer.go[278-325]
- internal/sp/store/resource_manager/service_instance.go[194-211]
- internal/sp/store/resource_manager/service_instance.go[323-351]

## Recommended Fix
Atomically persist enrollment and the non-deferred deletion marker before publishing, then remove the required unconditional post-publish status transition. Alternatively, introduce a dedicated durable deletion-mode field and make acknowledgement handling use it to select hard deletion. Add coverage for acknowledgement arriving before publish returns, initial publish failure followed by a successful scheduler retry, and an injected database failure after successful external dispatch, verifying that every accepted non-deferred deletion remains classified for hard deletion and that the row is removed completely.

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



Remediation recommended

5. Delete docs misstate no-agent behavior ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The OpenAPI description says instances are enrolled and published and that enrollment success
guarantees a 204 response. Non-deferred instances without an agent or publisher bypass enrollment
and are hard-deleted immediately, while deferred instances are enrolled and completed later by the
scheduler rather than being completed immediately.
Code

api/sp/v1alpha1/resource_manager/openapi.yaml[R214-217]

+        Remove an instance. The instance is enrolled for deletion
+        (deletion_status=SCHEDULED) and a deletion request is published to
+        the agent, best-effort; the call always returns 204 once enrollment
+        succeeds, regardless of whether the publish succeeded — a publish
Evidence
The updated specification universally describes enrollment and best-effort publication, but the
service hard-deletes non-deferred no-agent/no-publisher instances before enrollment. Deferred
instances take the enrollment path, and the scheduler later marks no-agent records complete.

api/sp/v1alpha1/resource_manager/openapi.yaml[214-231]
internal/sp/service/resource_manager/service_type_instance.go[317-337]
internal/sp/cleanup/scheduler.go[100-119]

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 public API description presents enrollment, publication, and completion behavior as universal even though no-agent and no-publisher paths differ by deletion mode. Consumers therefore receive an inaccurate contract for when enrollment occurs and when tombstones are created.

## Fix Focus Areas
- api/sp/v1alpha1/resource_manager/openapi.yaml[214-231]
- internal/sp/service/resource_manager/service_type_instance.go[317-337]
- internal/sp/cleanup/scheduler.go[100-119]

## Recommended Fix
Describe agent-routed and no-agent behavior separately: non-deferred no-agent deletes hard-delete immediately without enrollment, while deferred no-agent deletes enroll and are later tombstoned by the scheduler. Regenerate the embedded specification and generated comments after updating OpenAPI.

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


Grey Divider

Context sources
Review mode: 🧠 Deep: This push changes deletion semantics across API, persistence, service, scheduler, and asynchronous consumer paths, creating multiple independent race, retry, and hard-delete edge cases that materially benefit from redundant review passes.

Grey Divider

Tip of the day
💡 Did you know, you can turn these tips off under Display preferences

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Previous reviews

Review updated until commit a7f8ea4

Results up to commit 00f32ad ⚖️ Balanced


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Action required
1. Fast acknowledgements leave broken state ✓ Resolved 🐞 Bug ☼ Reliability
Description
DeleteInstance enrolls a non-deferred instance with only deletion_status=SCHEDULED and does not
durably set status=deleting until after synchronous publication, with publish failures and
post-publish UpdateStatus failures leaving the deletion mode unrecorded. A fast acknowledgement,
or one following a scheduler retry, then takes the deferred soft-completion branch and creates a
DELETED tombstone, which a racing ID-only status update can overwrite to deleting, while retries
may continue until acknowledgement.
Code

internal/sp/service/resource_manager/service_type_instance.go[R340-342]

+	if err := s.enrollForDeletion(ctx, instance); err != nil {
+		return err
+	}
Evidence
Enrollment sets deletion_status=SCHEDULED before publication, while the non-deferred
status=deleting update occurs afterward without a compare-and-swap guard; both a
non-agent-not-found publish failure and an UpdateStatus database failure can therefore leave the
row scheduled without its intended mode. The cleanup scheduler republishes scheduled rows, and the
acknowledgement consumer independently reads the row, hard-deleting only those already marked
deleting while soft-completing the others as tombstones; because UpdateStatus targets the row
solely by ID, it can also overwrite a tombstone produced by a fast acknowledgement.

internal/sp/service/resource_manager/service_type_instance.go[340-369]
internal/sp/messaging/publisher.go[93-103]
internal/sp/consumer/response_consumer.go[278-325]
internal/sp/store/resource_manager/service_instance.go[194-211]
internal/sp/store/resource_manager/service_instance.go[416-427]
internal/sp/cleanup/scheduler.go[100-156]
internal/sp/consumer/response_consumer.go[290-325]
internal/sp/store/resource_manager/service_instance.go[323-351]
internal/sp/cleanup/scheduler.go[90-156]

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 enroll-before-publish sequence exposes a scheduled non-deferred deletion before its hard-deletion mode is durably recorded. A fast acknowledgement, an initial publish failure followed by scheduler retry, or a successful publish followed by an `UpdateStatus` failure can leave the request classified as deferred, causing acknowledgement processing to preserve a tombstone; a racing post-publish update can also overwrite the completed lifecycle state.

## Fix Focus Areas
- internal/sp/service/resource_manager/service_type_instance.go[340-369]
- internal/sp/consumer/response_consumer.go[278-325]
- internal/sp/store/resource_manager/service_instance.go[194-211]
- internal/sp/store/resource_manager/service_instance.go[323-351]

## Recommended Fix
Atomically persist enrollment and the non-deferred deletion marker before publishing, then remove the required unconditional post-publish status transition. Alternatively, introduce a dedicated durable deletion-mode field and make acknowledgement handling use it to select hard deletion. Add coverage for acknowledgement arriving before publish returns, initial publish failure followed by a successful scheduler retry, and an injected database failure after successful external dispatch, verifying that every accepted non-deferred deletion remains classified for hard deletion and that the row is removed completely.

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



Remediation recommended
2. Delete docs misstate no-agent behavior ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The OpenAPI description says instances are enrolled and published and that enrollment success
guarantees a 204 response. Non-deferred instances without an agent or publisher bypass enrollment
and are hard-deleted immediately, while deferred instances are enrolled and completed later by the
scheduler rather than being completed immediately.
Code

api/sp/v1alpha1/resource_manager/openapi.yaml[R214-217]

+        Remove an instance. The instance is enrolled for deletion
+        (deletion_status=SCHEDULED) and a deletion request is published to
+        the agent, best-effort; the call always returns 204 once enrollment
+        succeeds, regardless of whether the publish succeeded — a publish
Evidence
The updated specification universally describes enrollment and best-effort publication, but the
service hard-deletes non-deferred no-agent/no-publisher instances before enrollment. Deferred
instances take the enrollment path, and the scheduler later marks no-agent records complete.

api/sp/v1alpha1/resource_manager/openapi.yaml[214-231]
internal/sp/service/resource_manager/service_type_instance.go[317-337]
internal/sp/cleanup/scheduler.go[100-119]

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 public API description presents enrollment, publication, and completion behavior as universal even though no-agent and no-publisher paths differ by deletion mode. Consumers therefore receive an inaccurate contract for when enrollment occurs and when tombstones are created.

## Fix Focus Areas
- api/sp/v1alpha1/resource_manager/openapi.yaml[214-231]
- internal/sp/service/resource_manager/service_type_instance.go[317-337]
- internal/sp/cleanup/scheduler.go[100-119]

## Recommended Fix
Describe agent-routed and no-agent behavior separately: non-deferred no-agent deletes hard-delete immediately without enrollment, while deferred no-agent deletes enroll and are later tombstoned by the scheduler. Regenerate the embedded specification and generated comments after updating OpenAPI.

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


Grey Divider

Qodo Logo

Comment thread internal/sp/service/resource_manager/service_type_instance.go Outdated
Comment thread api/sp/v1alpha1/resource_manager/openapi.yaml Outdated
gabriel-farache added a commit to gabriel-farache/control-plane that referenced this pull request Sep 10, 2026
…review)

Qodo's automated review of PR dcm-project#63
(dcm-project#63 (comment))
found that the only durable signal separating a non-deferred
(hard-delete) scheduled deletion from a deferred one was a
post-publish status=deleting write: a fast acknowledgement, a publish
failure followed by a scheduler retry, or a DB failure on that write
after a successful publish could each misclassify the deletion as
deferred and leave a DELETED tombstone instead of hard-deleting, and
an unguarded late UpdateStatus could resurrect status=deleting on an
already-finalized row.

Add a durable ServiceTypeInstance.HardDelete column, persisted in the
same update as deletion_status=SCHEDULED at enrollment
(MarkForDeletion/ResetRetryCount), before any publish attempt.
handleDeletionAcknowledged and the cleanup scheduler's auditGiveUp now
key off this field instead of status, so classification survives fast
acks, publish failures/retries, and best-effort status-write failures.
The post-publish status=deleting write becomes observability-only
(GetInstance surfacing "deleting"); its failure no longer fails
DeleteInstance.

Also fix the OpenAPI deleteInstance description, which stated
enrollment/publish/204 behavior universally even though non-deferred
no-agent/no-publisher deletes bypass enrollment and hard-delete
immediately, while deferred ones enroll and are completed later by the
cleanup scheduler rather than immediately. Regenerated spec.gen.go via
`make generate-sp-rm-api` (types.gen.go/server.gen.go/client.gen.go are
byte-identical, since they don't embed the operation description text).

Assisted by: Cursor Agent - Claude Sonnet 4.5

Signed-off-by: gabriel-farache <gfarache@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@gabriel-farache
gabriel-farache marked this pull request as ready for review September 10, 2026 10:03
@gabriel-farache
gabriel-farache requested a review from a team as a code owner September 10, 2026 10:03
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Unify agent deletion with durable enroll-before-publish

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 20-40 Minutes

Grey Divider

AI Description

• Enrolls all agent-routed deletions before best-effort publication and scheduler retries.
• Persists hard-delete intent independently from transient instance status.
• Aligns acknowledgements, audit outcomes, API documentation, and regression coverage.
Diagram

sequenceDiagram
  actor Caller
  participant Service as Delete Service
  participant Store as Instance Store
  participant Agent
  participant Scheduler
  participant Consumer as Ack Consumer
  Caller->>Service: Delete instance
  Service->>Store: Enroll SCHEDULED and mode
  Store-->>Service: Durable state
  Service->>Agent: Publish delete
  alt Publish fails
    Agent--xService: Publish error
    Service-->>Caller: 204 enrolled
    Scheduler->>Store: Load SCHEDULED
    Store-->>Scheduler: Instance and mode
    Scheduler->>Agent: Retry delete
  else Publish succeeds
    Agent-->>Service: Accepted
    Service-->>Caller: 204 enrolled
  end
  Agent->>Consumer: Deletion acknowledged
  Consumer->>Store: Read pending mode
  alt Hard delete
    Consumer->>Store: Purge row
  else Deferred delete
    Consumer->>Store: Keep tombstone
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Deletion mode enum
  • ➕ Makes completion intent explicit and supports additional future deletion policies.
  • ➕ Avoids boolean ambiguity at call sites and in persisted records.
  • ➖ Requires broader schema and generated-code changes for only two current outcomes.
  • ➖ Adds migration and compatibility complexity without changing delivery guarantees.
2. Transactional outbox
  • ➕ Atomically couples deletion enrollment with eventual message publication.
  • ➕ Provides a generalized durable messaging mechanism beyond deletion retries.
  • ➖ Requires a new outbox schema, dispatcher, retention policy, and operational monitoring.
  • ➖ Duplicates guarantees already supplied here by the existing cleanup scheduler.

Recommendation: Keep the PR's durable HardDelete marker and enroll-before-publish ordering. It is the smallest change that closes fast-ack, publish-failure, retry, and status-write races while reusing the existing scheduler; an enum is only warranted if more completion modes emerge, and a transactional outbox would be disproportionate for the current architecture.

Files changed (16) +584 / -181

Bug fix (8) +150 / -87
server.gen.goRemove generated delete 422 response support +5/-16

Remove generated delete 422 response support

• Updates generated parameter documentation and removes the generated typed 422 deletion response.

internal/sp/api/resource_manager/server.gen.go

scheduler.goHonor durable deletion mode during cleanup give-up +21/-2

Honor durable deletion mode during cleanup give-up

• Makes audited give-up purge hard-delete enrollments while preserving tombstones for deferred enrollments. It also documents why no-agent scheduled rows are always deferred.

internal/sp/cleanup/scheduler.go

response_consumer.goFinalize acknowledgements using durable deletion mode +23/-16

Finalize acknowledgements using durable deletion mode

• Uses 'HardDelete' instead of transient status to select purge versus tombstone completion while retaining the existing pending-deletion gate. Internal cancel-rejected enrollment explicitly preserves tombstones.

internal/sp/consumer/response_consumer.go

errors.goRemove obsolete delete provisioning-error mapping +0/-4

Remove obsolete delete provisioning-error mapping

• Drops the dedicated 422 mapping because agent publish failures are now retried internally rather than returned by 'DeleteInstance'.

internal/sp/handlers/resource_manager/errors.go

service_type_instance.goEnroll agent deletions durably before publishing +75/-37

Enroll agent deletions durably before publishing

• Unifies deferred and non-deferred agent deletion around a shared enrollment step that atomically stores scheduling state and completion mode. Publish and post-publish status failures become non-fatal after enrollment, while enrollment failures remain caller-visible and agent-not-found retains mode-specific handling.

internal/sp/service/resource_manager/service_type_instance.go

service_type_instance.goAdd durable hard-delete mode to instances +12/-0

Add durable hard-delete mode to instances

• Adds a non-null 'hard_delete' field that records whether completion purges the row or retains a tombstone.

internal/sp/store/model/service_type_instance.go

service_instance.goPersist deletion mode during enrollment and reset +14/-4

Persist deletion mode during enrollment and reset

• Extends 'MarkForDeletion' and 'ResetRetryCount' to accept and atomically persist the hard-delete mode alongside scheduling and retry state.

internal/sp/store/resource_manager/service_instance.go

client.gen.goRemove generated client handling for delete 422 responses +0/-8

Remove generated client handling for delete 422 responses

• Drops the obsolete 422 response field and parsing branch from the generated resource manager client.

pkg/sp/client/resource_manager/client.gen.go

Tests (5) +354 / -37
scheduler_test.goCover hard-delete cleanup give-up behavior +30/-6

Cover hard-delete cleanup give-up behavior

• Updates enrollment calls for the new mode argument and verifies that an unregistered agent causes a hard-delete enrollment to be purged.

internal/sp/cleanup/scheduler_test.go

response_consumer_test.goTest race-safe acknowledgement finalization +66/-4

Test race-safe acknowledgement finalization

• Reworks non-deferred acknowledgement tests around 'SCHEDULED' plus 'HardDelete=true'. Adds coverage for fast acknowledgements and acknowledgements after simulated publish retries without relying on 'status=deleting'.

internal/sp/consumer/response_consumer_test.go

errors_test.goVerify removed delete 422 handling +9/-6

Verify removed delete 422 handling

• Updates the provisioning-error test to expect the generic 500 fallback now that deletion has no dedicated 422 response.

internal/sp/handlers/resource_manager/errors_test.go

service_type_instance_test.goExercise durable deletion enrollment failure scenarios +230/-2

Exercise durable deletion enrollment failure scenarios

• Adds failing publisher and decorated-store fixtures to test publish failures, status-write failures, re-delete retry resets, mode changes, and reset failures. Verifies both deferred and non-deferred modes are persisted before publication.

internal/sp/service/resource_manager/service_type_instance_test.go

service_instance_test.goAdapt store tests to explicit deletion mode +19/-19

Adapt store tests to explicit deletion mode

• Updates deletion enrollment and retry-reset tests to pass the new 'hardDelete' argument throughout the store suite.

internal/sp/store/resource_manager/service_instance_test.go

Documentation (3) +80 / -57
openapi.yamlDocument unified instance deletion semantics +25/-11

Document unified instance deletion semantics

• Clarifies agent-routed and no-agent deletion behavior, redefines 'deferred' as tombstone retention, and removes the obsolete 422 publish-failure response.

api/sp/v1alpha1/resource_manager/openapi.yaml

spec.gen.goRegenerate the embedded OpenAPI specification +50/-44

Regenerate the embedded OpenAPI specification

• Refreshes the compressed embedded specification to include the revised deletion contract and response set.

api/sp/v1alpha1/resource_manager/spec.gen.go

types.gen.goRegenerate deferred parameter documentation +5/-2

Regenerate deferred parameter documentation

• Updates generated 'DeleteInstanceParams' comments to describe tombstone retention rather than publish-failure deferral.

api/sp/v1alpha1/resource_manager/types.gen.go

Comment thread internal/sp/store/model/service_type_instance.go
Comment thread internal/sp/consumer/response_consumer.go Outdated
Comment thread internal/sp/consumer/response_consumer.go Outdated
@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 0e9359a

Assisted by: Claude Code - claude-4.6-sonnet

Signed-off-by: gabriel-farache <gfarache@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant