fix: recreate application in managed mode upon deletion from spoke

[Rizwana777]

What does this PR do / why we need it:
Application was not recreating when deleted from managed mode even though Principal is not in deletion state, In this fix application is recreating immediately once it is deleted from managed mode

Which issue(s) this PR fixes:

Fixes #?
https://issues.redhat.com/browse/GITOPS-7836

How to test changes / Special notes to the reviewer:

Checklist

• Documentation update is required by this PR (and has been updated) OR no documentation update is required.

Summary by CodeRabbit

Release Notes

• New Features

  • Applications from the principal cluster now automatically recreate if deleted from the managed cluster.

• Tests

  • Added test cases validating app recreation scenarios and handling expected deletions.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 62.50000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 45.44%. Comparing base (6235cf1) to head (a347ec0).

Files with missing lines	Patch %	Lines
agent/outbound.go	62.50%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #605      +/-   ##
==========================================
- Coverage   45.45%   45.44%   -0.01%     
==========================================
  Files          90       90              
  Lines        9907     9915       +8     
==========================================
+ Hits         4503     4506       +3     
- Misses       4944     4949       +5     
  Partials      460      460              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[chetan-rns]

@Rizwana777 Can you please pull the latest updates from main? Hopefully, that should fix the e2e tests.

[chetan-rns]

We maintain a cache of the desired state app spec on the agent. Can we use that to revert the deletion?

Ref: #567

Also, some of these methods are being refactored to avoid duplication here #596

[Rizwana777]

I will update this logic

[chetan-rns]

@Rizwana777 #596 has been merged

[Rizwana777]

Thanks @chetan-rns I will modify my PR accordingly

[chetan-rns]

This check may not always pass. What if the app gets recreated before we reach this check?

[chetan-rns]

Can we move this test to appcache_test.go? We have all the revert-based tests in that file. For example, here's a similar test for the autonomous agent https://github.com/argoproj-labs/argocd-agent/blob/main/test/e2e/appcache_test.go#L235

[coderabbitai]

Walkthrough

Changes add app deletion reversion logic when apps originate from the principal cluster, with conditional abort of the deletion queue path if revert succeeds. Unit and e2e tests validate the new pre-delete check and resulting app recreation behavior.

Changes

Cohort / File(s)	Summary
Core reversion logic agent/outbound.go	Added pre-delete check in addAppDeletionToQueue: calls manager.RevertUserInitiatedDeletion for apps with principal origin; aborts deletion queue path if revert succeeds (reverted=true). Existing resource removal and managed checks now gated behind revert logic.
Unit tests agent/outbound_test.go	Added two test cases to Test_addAppDeletionToQueue: "Recreation of application from principal when deleted" validates app recreation when deletion is initiated; "No recreation when deletion is expected from principal" confirms deletion queues normally when marked expected. New imports: manager package and k8stypes alias for UID handling.
E2E tests test/e2e/appcache_test.go	Added Test_RevertManagedAppDeletion to CacheTestSuite: creates managed app in principal cluster, deploys to managed cluster, deletes from managed agent, verifies recreation, and validates recreated app matches principal spec.

Sequence Diagram

sequenceDiagram
    participant Queue as Deletion Queue
    participant Agent as addAppDeletionToQueue
    participant Manager as RevertUserInitiatedDeletion
    participant Cache as App Cache

    Agent->>Manager: Check if app from principal + attempt revert
    alt App reverted successfully
        Manager-->>Agent: reverted=true
        Agent->>Cache: App remains in cache
        Note over Agent: Deletion aborted
    else No revert or deletion expected
        Manager-->>Agent: reverted=false or error
        Agent->>Queue: Enqueue deletion
        Queue->>Cache: Process deletion
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• agent/outbound.go: Requires careful review of the conditional control flow and error handling path for revert logic
• agent/outbound_test.go: Verify test coverage of both revert success and expected deletion paths; confirm SourceUIDAnnotation usage is correct
• test/e2e/appcache_test.go: Validate e2e test timing assumptions and eventual consistency checks for app recreation

Poem

🐰 A hop toward restoration!
When apps must dance away and back again,
The rabbit guards the principal's plan—
Deletion? Nay! Revert and mend,
Let managed clusters know: we resurrect, my friend! 🌱

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The PR title "fix: recreate application in managed mode upon deletion from spoke" directly and fully aligns with the main objective of the changeset. The changes implement logic to detect when an Application is deleted from the managed cluster (spoke) and recreate it if the principal cluster has not initiated the deletion. The title accurately captures this core functionality: it specifies the action (recreate application), the context (in managed mode), and the trigger (upon deletion from spoke). The phrasing is concise, specific, and sufficiently clear that a developer reviewing the commit history would understand the primary change without additional context.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9aa7536 and a347ec0.

📒 Files selected for processing (3)

• agent/outbound.go (1 hunks)
• agent/outbound_test.go (3 hunks)
• test/e2e/appcache_test.go (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• test/e2e/appcache_test.go

🧰 Additional context used

🧬 Code graph analysis (2)

agent/outbound_test.go (3)

internal/event/event.go (2)

• NewEventSource (127-131)
• Delete (56-56)

pkg/types/types.go (1)

• AgentModeManaged (30-30)

internal/manager/manager.go (1)

• SourceUIDAnnotation (46-46)

agent/outbound.go (1)

internal/manager/manager.go (1)

• RevertUserInitiatedDeletion (208-245)

🔇 Additional comments (5)

agent/outbound_test.go (3)

10-10: LGTM!

The new imports are necessary for the test cases and are used appropriately.

Also applies to: 21-21

---

169-199: Verify actual recreation in integration tests.

The test correctly validates that the deletion is aborted when the app originates from the principal and deletion is not expected. However, it only verifies the side effect (app remains managed and no queue event) without confirming the app was actually recreated via mgr.Create.

Since the actual recreation happens inside RevertUserInitiatedDeletion using the fake client, consider adding integration or E2E tests to verify the full recreation flow.

---

200-231: LGTM!

The test correctly validates that when a deletion is expected from the principal, the normal deletion flow proceeds (delete event is queued) rather than triggering recreation.

agent/outbound.go (2)

121-131: LGTM! Pre-delete revert check correctly gates the deletion flow.

The new logic appropriately checks if the app originated from the principal and attempts to revert unauthorized deletions. The early return when reverted=true ensures the app remains managed and no delete event is queued, which aligns with the PR objective of recreating apps deleted in managed mode.

The pattern is consistent with existing appProject and repository deletion handling.

---

123-126: Verify error recovery for failed recreation.

When RevertUserInitiatedDeletion returns an error (likely from a failed Create call), the function returns early, leaving the app in a managed state even though it may no longer exist. This could lead to state inconsistencies if the error is not transient.

Is there a reconciliation mechanism that would retry the recreation, or should the error handling include fallback logic to clean up the managed state?

---

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

[chetan-rns]

Thanks @Rizwana777!

LGTM!

[jannfis]

LGTM!