Fix: wrap status update with RetryOnConflict to prevent linkedSkills loss

[pdettori]

Summary

• Wraps the reconciler's final status update (step 8) with retry.RetryOnConflict to handle optimistic concurrency conflicts
• Moves linkedSkills and SkillsDiscovered condition assignment inside the retry loop so they survive a re-fetch
• Adds a unit test simulating a concurrent status modification to verify linkedSkills persists through a conflict

Root Cause

When another reconciliation loop (e.g., applying labels/config-hash) modifies the same AgentRuntime object concurrently, the status update fails with a conflict error. Without retry, the linkedSkills field is never populated, causing the Skill Discovery E2E test to time out after 180s.

Approach

Uses the same retry.RetryOnConflict(retry.DefaultRetry, ...) pattern already established throughout this controller (see applyWorkloadConfig, removeLabelsOnDelete, etc.). Inside the retry closure:

1. Re-fetches the AgentRuntime to get the latest resourceVersion
2. Re-applies all status fields (configuredPods, phase, Ready condition, linkedSkills, SkillsDiscovered condition)
3. Calls Status().Update()

Closes #422

Test plan

• Unit test: new "should persist linkedSkills even after a concurrent status modification" test passes
• Unit test: existing "should read linked skills into status" test still passes
• CI: E2E skill discovery tests pass (previously failing on every main run)

Assisted-By: Claude Code

[huang195]

Review Summary

The diagnosis and the RetryOnConflict approach are spot-on, Paolo — bellissimo reuse of the established controller pattern, and the root-cause writeup is chef's-kiss. The catch: re-fetching rt inside the retry closure throws away the conditions set earlier in the same reconcile that weren't persisted yet, so linkedSkills is saved but ConfigResolved and IstioMeshEnrolled are lost. The unit-test suite caught it (2 failing specs in a file this PR doesn't even touch — that's the tell).

Two CI gates to clear:

• Unit Tests (fail) — the condition-drop regression (see inline). The real blocker.
• verify-pr-title (fail) — title must use a capitalized prefix. fix: → Fix: (the check is case-sensitive: Build, Chore, CI, Docs, Feat, Fix, ...).
• E2E Tests (fail) — this is the suite the PR targets; worth confirming it goes green once the condition-drop is fixed (the test-plan checkbox for it is still unchecked).

Areas reviewed: Go (controller + test) · Commits: 1, signed-off ✅ · CI: failing (Unit, E2E, pr-title)

Fix the re-fetch to preserve all conditions and that title prefix, and this is good to go. 🍝

Assisted-By: Claude Code

[huang195]

must-fix — This r.Get(ctx, req.NamespacedName, rt) overwrites the entire in-memory rt (including rt.Status.Conditions) with server state. But ConfigResolved (set at step 5) and IstioMeshEnrolled (step 4.6) are set in-memory and not yet persisted when we reach step 8 — the re-fetch discards them, and the closure only re-applies Ready, SkillsDiscovered, ConfiguredPods, LinkedSkills. So those conditions are silently dropped on every reconcile.

This is exactly what breaks the two agentruntime_config_test.go specs (lines 348 & 386 — ConfigResolved comes back nil).

Suggest snapshotting the desired status before the loop and restoring it after the fresh Get:

desired := rt.Status.DeepCopy()
if err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
    if err := r.Get(ctx, req.NamespacedName, rt); err != nil {
        return err
    }
    rt.Status = *desired // keeps refreshed resourceVersion, preserves all computed conditions
    rt.Status.ConfiguredPods = configuredPods
    // ... existing Ready / LinkedSkills / SkillsDiscovered logic ...
    return r.Status().Update(ctx, rt)
}); err != nil { ... }

[huang195]

suggestion — This new test only asserts LinkedSkills survives the conflict, which is why it stayed green while the ConfigResolved / IstioMeshEnrolled regression slipped through. Adding an assertion that ConfigResolved (and ideally IstioMeshEnrolled) is still present after the conflicting reconcile would lock down the whole status-preservation contract, not just the one field.

[huang195]

Review Summary (re-review)

Perfetto, Paolo — this is exactly the fix. Snapshotting rt.Status.DeepCopy() before the retry loop and restoring it after the fresh Get preserves ConfigResolved, IstioMeshEnrolled, and the rest while still getting the refreshed resourceVersion for a conflict-free update. And bellissimo touch adding the ConfigResolved-survives-the-retry assertion to the new test — that locks down the whole status-preservation contract, not just linkedSkills. Unit Tests, pr-title, and lint are all green now.

Only open item: E2E is still running (pending) — since that's the suite this PR ultimately targets (skill discovery timeout), worth a glance once it lands before merge.

Areas reviewed: Go (controller + test) · Commits: 2, all signed-off ✅ · CI: all green except E2E (pending)

No blockers — approving. 🍝

Assisted-By: Claude Code

[huang195]

nit — rt.Status = *desired is last-writer-wins on the entire status: any field a concurrent actor wrote that this reconcile didn't recompute gets overwritten. That's correct here since this controller is the sole owner of AgentRuntime status, but a one-line comment noting that intent would save a future reader from mistaking it for an oversight.

[kevincogan]

LGTM and E2E is green. Nice work!