fix: add check to Observe the last sync status for the resource

[SatabdiG]

Fixes #491

ServiceInstance shows Ready: True, Synced: True when async update operations fail

Bug Description

When a ServiceInstance update fails asynchronously in BTP (e.g., due to invalid parameters), the Crossplane resource incorrectly shows Ready: True and Synced: True even though the operation failed. The failure is only visible in the LastAsyncOperation condition.

Root Cause

The controller was not checking the LastAsyncOperation condition status in the Observe() method. When terraform detected drift and triggered an Update that failed asynchronously, the subsequent Observe would see Drift status and return ResourceUpToDate: false, but Crossplane would still set Synced: True because no error was returned.

Fix

Added checkAsyncOperationFailure() helper that checks LastAsyncOperation condition before the terraform status switch, ensuring failed async operations are caught regardless of whether terraform reports Drift or UpToDate.

Affected Resources

• ServiceInstance (account.btp.sap.crossplane.io/v1alpha1)

Note
Potentially other resources using upjet with async operations

[enrico-kaack-comp]

we are checking the cr. But the CR has the status from the last Observe. Wouldnt it be better to check the status of the current Observe?

[SatabdiG]

We could but the catch is we have to use the last observe because:
The synced condition should reflect whether our last reconciliation attempt succeeded, not just whether there's drift. The LastAsyncOperation condition (set by upjet's saveCallback during Update) tells us if the previous update succeeded or failed. Without this check, resources would show Synced=True even when updates fail, because terraform would only report "Drift" without exposing the failure reason

[SatabdiG]

According to what I found This pattern follows upjet's native controller implementation, where conditions from async operations are checked in subsequent Observe cycles. The conditions are "fresh" because
upjet's saveCallback updates them immediately when the async terraform operation completes.

Any other alternative would require refactoring Upjets saveCallback which is more refactoring work and not sure we even want to go in that direction.

[enrico-kaack-comp]

I got it, the CR contains the status of the current observe cycle because it is immediately saved by tf. Than this is good :)
Maybe add this as a comment in case someone is wondering the next time :D

[sdischer-sap]

Just wondering, is the status condition Unavailable() commonly used for purposes like that?
Missed potentially the last discussions, just wondering if we should have this discussion and formalize it a bit more in an ADR or so, because I think reflected failed updates is generally a topic we (and for all a know the whole crossplane community) has totally ignored up until now.

[SatabdiG]

@sdischer-sap, good point, seems like a dev sync topic.

[sdischer-sap]

@SatabdiG deal ;)
-> https://github.tools.sap/cloud-orchestration/handbook/issues/388

[sdischer-sap]

@SatabdiG can we merge that one? I think we reached an agreement right?

[SatabdiG]

As decided we will update to reflect synced:false in such cases as opposed to ready:false