Return a retryable 409 instead of 500 when a policy update loses a concurrent write

[ayushtkn]

PolicyCatalog.updatePolicy calls updateEntityPropertiesIfNotChanged, which returns no entity when another writer has already modified the policy since it was read. That outcome is currently turned into an IllegalStateException("Failed to update policy …"), which reaches the client as HTTP 500.
Two clients updating the same policy at the same time is ordinary optimistic-concurrency behaviour rather than a server fault: the losing request is safe to retry, and a 500 tells the client the opposite — that something went wrong server-side and retrying is inadvisable.
It also leaves no way to distinguish a lost race from a genuine failure. Every comparable path in the codebase already treats this as a conflict and throws CommitConflictException, which maps to HTTP 409: PolarisAdminService.updateCatalog, updatePrincipal and updatePrincipalRole for the same compare-and-swap outcome, and LocalIcebergCatalog.setProperties and removeProperties for concurrent namespace modification.
This change makes policy updates behave the same way, so a client that loses the race receives a retryable 409 naming the policy.

example ref:

polaris/runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java

Lines 1047 to 1055 in ffa6977

	Optional.ofNullable(
	CatalogEntity.of(
	PolarisEntity.of(
	metaStoreManager.updateEntityPropertiesIfNotChanged(
	getCurrentPolarisContext(), null, updatedEntity))))
	.orElseThrow(
	() ->
	new CommitConflictException(
	"Concurrent modification on Catalog '%s'; retry later", name));

Checklist

• 🛡️ Don't disclose security issues! (contact security@apache.org)
• 🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
• 🧪 Added/updated tests with good coverage, or manually tested (and explained how)
• 💡 Added comments for complex logic
• 🧾 Updated CHANGELOG.md (if needed)
• 📚 Updated documentation in site/content/in-dev/unreleased (if needed)

[Copilot]

Pull request overview

This pull request adjusts policy-update optimistic concurrency handling so that losing a concurrent write results in a retryable conflict (CommitConflictException → HTTP 409) instead of an internal server error (HTTP 500), matching the behavior used in other compare-and-swap update paths.

Changes:

• Throw CommitConflictException from PolicyCatalog.updatePolicy when the compare-and-swap update returns no updated entity.
• Add a unit test that simulates a concurrent modification return status from the metastore and asserts updatePolicy raises CommitConflictException.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
runtime/service/src/main/java/org/apache/polaris/service/catalog/policy/PolicyCatalog.java	Converts a null CAS update result into CommitConflictException (409) for retryable concurrent-update failures.
runtime/service/src/test/java/org/apache/polaris/service/catalog/policy/AbstractPolicyCatalogTest.java	Adds coverage for the concurrent-update-loss case using a spied metastore manager.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[nandorKollar]

nit: might worth a simple refactor into orElseThrow (we follow this pattern elsewhere already when throwing CommitConflictException)

I think we follow this pattern in other updates in admin service too, but is null result of an entity update the right indicator of commit conflict.

[ayushtkn]

Thanx @nandorKollar for the review. I have changed to match the existing pattern.

Regarding the null

updateEntityPropertiesIfNotChanged catches RetryOnConcurrencyException and returns an EntityResult with status TARGET_ENTITY_CONCURRENTLY_MODIFIED and no entity (null).

-> On success it returns the updated entity.
-> Other failures typically throw, not return null.

But maybe it can drift tmrw or there is some corner case for some impl. I have currently kept the pattern to match the existing methods, but maybe we can update everywhere to check ReturnStatus.TARGET_ENTITY_CONCURRENTLY_MODIFIED like this (rough idea)

EntityResult updateResult =
    metaStoreManager.updateEntityPropertiesIfNotChanged(
        callContext.getPolarisCallContext(),
        PolarisEntity.toCoreList(catalogPath),
        newPolicyEntity);
if (!updateResult.isSuccess()) {
  if (updateResult.getReturnStatus()
      == BaseResult.ReturnStatus.TARGET_ENTITY_CONCURRENTLY_MODIFIED) {
    throw new CommitConflictException(
        "Concurrent modification on policy '%s'; retry later", policyIdentifier);
  }
  throw new IllegalStateException(
      String.format(
          "Failed to update policy %s: %s with extraInfo: %s",
          policyIdentifier,
          updateResult.getReturnStatus(),
          updateResult.getExtraInformation()));
}
newPolicyEntity = PolicyEntity.of(updateResult.getEntity());

But I believe if the current way doesn't look good, and we like the above maybe we should do it for every other place as a followup to maintain consistency. Let me know wdyt