fix(persistence): enforce optimistic concurrency in rotatePrincipalSecrets for relational JDBC by aakashofficial-k01 · Pull Request #5504 · apache/polaris

aakashofficial-k01 · 2026-09-12T19:16:16Z

Rationale

In JdbcBasePersistenceImpl.rotatePrincipalSecrets, the SQL update against PRINCIPAL_AUTHENTICATION_DATA omitted main_secret_hash from the WHERE clause and ignored the affected row count from datasourceOperations.executeUpdate(...). Under concurrent rotations for the same principal, a slower transaction could silently overwrite a faster transaction's credentials, causing the faster transaction to return HTTP 200 OK with unpersisted, orphaned secrets and resulting in unexpected 401 Unauthorized authentication failures.

Fixes #5503

Changes

Optimistic Concurrency Control (CAS): Added "main_secret_hash", oldSecretHash to the WHERE clause parameters in JdbcBasePersistenceImpl.rotatePrincipalSecrets.
Conflict Detection: Verified the affected row count from datasourceOperations.executeUpdate(...). If rowsUpdated == 0, throw RetryOnConcurrencyException, matching the optimistic concurrency pattern used in writeEntity.
Unit Test: Added parameterized unit test rotatePrincipalSecrets_concurrentCollision_throwsRetryOnConcurrencyException covering both schema version 1 and 2 to verify that concurrent collisions throw RetryOnConcurrencyException and preserve the committed credential in the database.

Checklist

🛡️ Don't disclose security issues! (contact security@apache.org)
🔗 Clearly explained why the changes are needed, or linked related issues: Fixes [BUG] rotatePrincipalSecrets in JdbcBasePersistenceImpl silently drops credentials on concurrent rotation #5503
🧪 Added/updated tests with good coverage, or manually tested (and explained how): added parameterized unit test in JdbcBasePersistenceImplTest covering schema v1 & v2.
💡 Added comments for complex logic: logic follows existing optimistic locking pattern in writeEntity.
🧾 Updated CHANGELOG.md (if needed): not needed for internal persistence bugfix.
📚 Updated documentation in site/content/in-dev/unreleased (if needed): no API documentation changes required.

…crets for relational JDBC Include main_secret_hash in the WHERE clause of rotatePrincipalSecrets and verify that rowsUpdated == 1. If 0 rows were updated, throw RetryOnConcurrencyException to prevent silent credential overwrites and orphaned ghost secrets under concurrent rotation. Fixes apache#5503

dimas-b

Nice fix 👍 Thanks for your contribution, @aakashofficial-k01 !

dimas-b · 2026-09-14T23:20:16Z

  }
+
+  @ParameterizedTest
+  @ValueSource(ints = {1, 2})


Why test older schema versions?

#5349 proposes using one DDL file and requiring the latest schema.

In any case, we should test the latest schema version in this PR.

Agreed, @dimas-b. Updated the test to target the latest schema version (DatabaseType.H2.getLatestSchemaVersion()). Please take another look when you have a moment.

vigneshio · 2026-09-15T14:28:33Z

+                      .stream()
+                      .toList(),
+                  params));
+      if (rowsUpdated == 0) {


createPrincipal still rotates twice with the same hash (PolarisTestMetaStoreManager ~515). The second call is stale, which causes this exception during catalog setup. The CAS can remain as-is; the fixture should use the hash returned by the first rotation. WDYT ??

Thanks for checking, @vigneshio. I too Spotted this after the CI run failure. Updated PolarisTestMetaStoreManager to pass the updated hash on the second rotation.

…xture

dimas-b

LGTM 👍

vigneshio

LGTM.. Thanks @aakashofficial-k01

flyingImer

The CAS fix itself looks right. One error-handling gap: expected contention is surfaced as a server failure. Details inline.

flyingImer · 2026-09-16T20:14:22Z

+                      .stream()
+                      .toList(),
+                  params));
+      if (rowsUpdated == 0) {


The CAS check looks right, but the new conflict exception needs handling before it reaches the API caller. Neither AtomicOperationMetaStoreManager nor the admin rotation path catches or retries RetryOnConcurrencyException, so it falls through IcebergExceptionMapper to HTTP 500 for an ordinary concurrent rotation. Could we handle this conflict at the rotation boundary, for example with the existing CommitConflictException (409), and add an API-level regression test? The new JDBC test verifies that the winning credentials survive, but does not cover the losing caller's response.

+1 to using 409 on the losing caller's response.

I do not think adding server-side retries justifies the extra complexity. I do not think this failure mode is common.

Thanks for the feedback @dimas-b and @flyingImer!

I have updated the PR in commit 4e1870d:

Metastore mapping: In AtomicOperationMetaStoreManager.rotatePrincipalSecrets, caught RetryOnConcurrencyException from ms.rotatePrincipalSecrets(...) and mapped it to BaseResult.ReturnStatus.TARGET_ENTITY_CONCURRENTLY_MODIFIED.

HTTP 409 translation: In PolarisAdminService.rotateOrResetCredentialsHelper, mapped TARGET_ENTITY_CONCURRENTLY_MODIFIED to CommitConflictException (which Polaris translates to HTTP 409 Conflict rather than an unhandled 500).

Regression test: Added testRotateCredentialsConcurrentModificationThrowsCommitConflictException in PolarisAdminServiceTest to verify that a concurrent modification during credential rotation throws CommitConflictException.

…onflictException Catch RetryOnConcurrencyException in AtomicOperationMetaStoreManager during rotatePrincipalSecrets and return TARGET_ENTITY_CONCURRENTLY_MODIFIED. Handle TARGET_ENTITY_CONCURRENTLY_MODIFIED and RetryOnConcurrencyException in PolarisAdminService to throw CommitConflictException, returning HTTP 409 Conflict instead of unhandled 500. Add regression test in PolarisAdminServiceTest.

dimas-b · 2026-09-17T18:01:19Z

+              currentPrincipalEntity.getId(),
+              shouldReset,
+              currentSecrets.getMainSecretHash());
+    } catch (RetryOnConcurrencyException e) {


Is this exception possible at this level now? Why do we have to catch it in two places? Here and in AtomicOperationMetaStoreManager

Acknowledged @dimas-b. I was extra defensive across both layers after the earlier feedback.

…ialsHelper Since AtomicOperationMetaStoreManager already converts RetryOnConcurrencyException to TARGET_ENTITY_CONCURRENTLY_MODIFIED, remove the redundant try-catch and unused import in PolarisAdminService.

dimas-b · 2026-09-22T18:46:27Z

@aakashofficial-k01 : Please fix CI errors. It looks like they are related to code format (spotless).

aakashofficial-k01 · 2026-09-24T21:18:49Z

@dimas-b Fixed the spotless formatting issue in PolarisAdminServiceTest. Could you please approve and trigger the CI run when you have a chance? Thank you.

dimas-b · 2026-09-28T15:10:19Z

As far as I can tell, previous review comments have been addressed... merging. If someone was missed, let's address in follow-up PRs.

Fix NPE on `AttributeMap` in `testRotateCredentialsConcurrentModificationThrowsCommitConflictException()` caused by overlapped merges of #5504 and #5119

github-project-automation Bot added this to Basic Kanban Board Sep 12, 2026

github-project-automation Bot moved this to PRs In Progress in Basic Kanban Board Sep 12, 2026

dimas-b reviewed Sep 14, 2026

View reviewed changes

vigneshio reviewed Sep 15, 2026

View reviewed changes

test: test latest schema in rotate collision and fix stale hash in fi…

cc538b4

…xture

dimas-b previously approved these changes Sep 15, 2026

View reviewed changes

github-project-automation Bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Sep 15, 2026

vigneshio approved these changes Sep 16, 2026

View reviewed changes

flyingImer reviewed Sep 16, 2026

View reviewed changes

aakashofficial-k01 dismissed dimas-b’s stale review via 4e1870d September 17, 2026 16:37

dimas-b reviewed Sep 17, 2026

View reviewed changes

refactor(service): remove redundant try-catch in rotateOrResetCredent…

207798c

…ialsHelper Since AtomicOperationMetaStoreManager already converts RetryOnConcurrencyException to TARGET_ENTITY_CONCURRENTLY_MODIFIED, remove the redundant try-catch and unused import in PolarisAdminService.

dimas-b previously approved these changes Sep 18, 2026

View reviewed changes

style(test): fix spotless formatting in PolarisAdminServiceTest

7ceae5c

aakashofficial-k01 dismissed dimas-b’s stale review via 7ceae5c September 24, 2026 16:59

dimas-b approved these changes Sep 25, 2026

View reviewed changes

dimas-b requested a review from flyingImer September 25, 2026 19:02

dimas-b merged commit 52ab269 into apache:main Sep 28, 2026
24 checks passed

dimas-b mentioned this pull request Sep 28, 2026

Fix PolarisAdminServiceTest #5637

Merged

6 tasks

dimas-b added a commit that referenced this pull request Sep 28, 2026

Fix PolarisAdminServiceTest (#5637)

ac7d1d3

Fix NPE on `AttributeMap` in `testRotateCredentialsConcurrentModificationThrowsCommitConflictException()` caused by overlapped merges of #5504 and #5119

jbonofre mentioned this pull request Sep 29, 2026

fix: omit null LOAD_TABLE_RESPONSE on conditional loadTable after-event #5638

Open

Conversation

aakashofficial-k01 commented Sep 12, 2026

Rationale

Changes

Checklist

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

vigneshio left a comment

Choose a reason for hiding this comment

Uh oh!

flyingImer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b commented Sep 22, 2026

Uh oh!

aakashofficial-k01 commented Sep 24, 2026

Uh oh!

dimas-b commented Sep 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants