fix: republish delete events for resources stuck deleting (HYPERFLEET-1497)

[ciaranRoche]

What

Restores healing for resources stuck soft-deleted, with a hard cap. When MarkAsDeleting is called for a resource whose deletion is already in flight, we now re-enqueue the delete event, throttled to at most one per --delete-event-republish-interval (default 60s, 0 disables) per resource. The check-then-create runs under the existing per-resource advisory lock, so the cap holds across replicas.

The stale-delete detector is adjusted to match: it now only retires delete events that are themselves older than the threshold, so a fresh healing event for a long soft-deleted resource gets a full threshold of delivery attempts instead of being retired within one detector tick.

Why

An upstream race in the OCM SDK agent store (sdk-go#232) can erase the DeletionTimestamp from the maestro-agent's in-memory store when a delete event lands while a status patch is in flight. The agent then recreates the deleted ManifestWork on the spoke and never reports Deleted=true, so the server's hard-delete never fires and cluster deletion hangs until an agent reconnect happens to trigger a spec resync. We observed 16+ hours across four e2e CI runs (ARO-28958, HYPERFLEET-1497).

Before #562 (AROSLSRE-1547) every retried delete request enqueued a fresh delete event, and each re-published delete_request re-stamped the DeletionTimestamp in the agent store, accidentally healing the race within one retry cycle. #562 made retries silent no-ops for a good reason (the event storm was starving the spec-event worker), but that also removed the healing. This change brings the healing back deliberately: the throttle guarantees retries can never accumulate events without bound, so the AROSLSRE-1547 fix is preserved. Worst case accumulation per stuck resource is threshold/interval unreconciled rows (60 at defaults) while its agent is disconnected.

This is a bounded mitigation while the upstream fix (sdk-go#233) is in review, and it also heals any future agent-side loss of deletion state. One caveat to be aware of: healing is driven by the client retrying deletes, which CS destructors do. Once the agent confirms the delete the resource row is hard-deleted and republishing stops naturally.

Testing

• Unit: TestMarkAsDeletingRepublishesStaleDeleteEvent, TestMarkAsDeletingRepublishDisabled, and the existing TestMarkAsDeletingIsIdempotent (unchanged assertions, now also validates the throttle window). The mock event DAO now stamps CreatedAt on create, mirroring gorm's autoCreateTime.
• Integration (real Postgres): TestMarkAsDeletingRepublishThrottle (throttle + republish after backdating the event row) and TestReconcileStaleDeleteEventsSparesFreshHealingEvents (detector retires a 2h-old event but spares the fresh healing event on a 2h soft-deleted resource). Existing TestReconcileStaleDeleteEvents passes unchanged.
• Full unit suite and mqtt integration suite green locally.

Related: HYPERFLEET-1497, ARO-28958, upstream root cause sdk-go#232 / fix sdk-go#233, same symptoms in the wild ocm#1404.

Note for reviewers: this touches the same MarkAsDeleting early-returns as the pending HYPERFLEET-1496 logging PR (#575), whichever lands second is a trivial rebase.

🤖 Generated with Claude Code

https://claude.ai/code/session_01USPzrSgidrmV9YrxRRf5tQ

Summary by CodeRabbit

• New Features
  • Added configurable republishing of delete events for resources that remain in a deleting state.
  • Delete-event republishing defaults to every 60 seconds and can be adjusted with a command-line setting.
  • Set the interval to zero to disable republishing.
• Bug Fixes
  • Prevented duplicate delete events during repeated deletion requests.
  • Improved stale-event reconciliation so recent republished events are preserved.

[coderabbitai]

Walkthrough

The change adds a configurable delete-event republish interval. Resource deletion retries now republish stale delete events, and reconciliation preserves fresh events. DAO, service, unit, and integration tests cover the behavior.

Changes

Delete event republish flow

Layer / File(s)	Summary
Configuration and service wiring pkg/config/event_server.go, pkg/config/event_server_test.go, cmd/maestro/environments/service_types.go, pkg/controllers/undelivered_detector_test.go	Adds a 60-second default, command-line flag, configuration expectations, and constructor wiring for the republish interval.
Event lookup and stale reconciliation pkg/dao/event.go, pkg/dao/mocks/event.go, pkg/services/event.go	Adds latest delete-event lookup and limits stale reconciliation to events created before the cutoff.
Throttled resource republishing pkg/services/resource.go, pkg/services/resource_test.go	Republishes delete events after the configured interval, skips recent events, supports disabling with a non-positive interval, and propagates creation failures.
Republish and reconciliation integration validation test/integration/resource_test.go	Validates republish throttling and preservation of fresh healing events during stale reconciliation.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ResourceService
  participant EventService
  participant EventDao
  ResourceService->>EventService: FindLatestDeleteEvent(sourceID)
  EventService->>EventDao: FindLatestDeleteEvent(sourceID)
  EventDao-->>EventService: Latest delete event or none
  EventService-->>ResourceService: Lookup result
  ResourceService->>EventService: Create delete event when interval elapsed

Loading

🚥 Pre-merge checks | ✅ 9 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Ai-Attribution	⚠️ Warning	AI use is stated and the PR commit has Co-Authored-By: Claude Fable 5, but no Assisted-by or Generated-by trailer.	Amend the commit with the required Red Hat Assisted-by or Generated-by trailer, and remove the AI Co-Authored-By trailer.

✅ Passed checks (9 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes republishing delete events for resources stuck in the deleting state.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
No-Weak-Crypto	✅ Passed	Parent-to-HEAD diff adds no MD5, SHA1, DES, RC4, Blowfish, ECB, crypto APIs, or secret comparisons; changes only event timing and database logic.
Container-Privileges	✅ Passed	The PR changes only Go files. The diff adds no container or Kubernetes manifests and no privilege-related settings such as privileged, hostPID, hostNetwork, hostIPC, SYS_ADMIN, or allowPrivilegeEsc...
No-Sensitive-Data-In-Logs	✅ Passed	The diff adds only resource UUID and delete-event age fields plus a lookup error; no passwords, tokens, API keys, PII, hostnames, or customer payloads are logged.
No-Hardcoded-Secrets	✅ Passed	The HEAD diff adds interval settings, event logic, and test UUID/JSON data only; scans found no API keys, tokens, passwords, private keys, credential URLs, or base64 config strings over 32 characters.
No-Injection-Vectors	✅ Passed	PR additions use parameterized GORM predicates and SQL placeholders; scans found no new SQL concatenation, shell/eval, unsafe deserialization, or dangerouslySetInnerHTML usage.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (2)

test/integration/resource_test.go (1)

680-684: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Assert that the fresh event remains unreconciled.

The final count does not identify which event remains. A regression that reconciles the fresh healing event and keeps the old event can still produce a count of one.

Load both delete events after reconciliation. Assert that the old event has ReconciledDate set and the fresh event has ReconciledDate == nil.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/integration/resource_test.go` around lines 680 - 684, Update the
reconciliation assertions after ReconcileStaleDeleteEvents in the integration
test to load both delete events and verify their individual states: the
2-hour-old event must have ReconciledDate set, while the fresh healing event
must retain a nil ReconciledDate. Keep the existing count and pending-delete
assertions as appropriate.

pkg/config/event_server_test.go (1)

28-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Test flag values that change behavior.

The cases only validate constructor defaults. They never set delete-event-republish-interval. A broken flag binding or an incorrect zero-disable value can pass.

Add cases for a positive override and 0, then assert DeleteEventRepublishInterval.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/config/event_server_test.go` around lines 28 - 65, Extend the
EventServerConfig constructor tests to cover the delete-event-republish-interval
input: add one case with a positive override and another with 0, and assert the
corresponding DeleteEventRepublishInterval values in each expected
EventServerConfig. Keep the existing default cases unchanged.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@pkg/config/event_server_test.go`:
- Around line 28-65: Extend the EventServerConfig constructor tests to cover the
delete-event-republish-interval input: add one case with a positive override and
another with 0, and assert the corresponding DeleteEventRepublishInterval values
in each expected EventServerConfig. Keep the existing default cases unchanged.

In `@test/integration/resource_test.go`:
- Around line 680-684: Update the reconciliation assertions after
ReconcileStaleDeleteEvents in the integration test to load both delete events
and verify their individual states: the 2-hour-old event must have
ReconciledDate set, while the fresh healing event must retain a nil
ReconciledDate. Keep the existing count and pending-delete assertions as
appropriate.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: openshift-online/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: c4cfc8f8-e79d-4568-8fb9-b4533fc3182c

📥 Commits

Reviewing files that changed from the base of the PR and between ab480c6 and c206ca4.

📒 Files selected for processing (10)

• cmd/maestro/environments/service_types.go
• pkg/config/event_server.go
• pkg/config/event_server_test.go
• pkg/controllers/undelivered_detector_test.go
• pkg/dao/event.go
• pkg/dao/mocks/event.go
• pkg/services/event.go
• pkg/services/resource.go
• pkg/services/resource_test.go
• test/integration/resource_test.go