jarredhj0214 commented on PR #13606:
URL: https://github.com/apache/gravitino/pull/13606#issuecomment-5906542565
I pulled the current PR head (`b88c4cf44`) and found three issues that seem
worth clarifying before merge.
### 1. The new concurrent-eviction test fails consistently
Using the project's Gradle build with JDK 17, I ran:
```bash
./gradlew :server-common:test \
--tests
'org.apache.gravitino.server.authorization.jcasbin.TestJcasbinAuthorizer.testConcurrentEvictionNeverFlipsCompositeDecisions'
\
-PskipITs
```
I ran the test four times and it failed four times at
`TestJcasbinAuthorizer.java:1334`:
```text
an eviction must not deny a granted check
expected: <true> but was: <false>
```
The other 12 newly added JCasbin scenarios passed, as did
`TestAuthorizationRequestContext`.
The concurrent test invalidates both roles every 1 ms, while
`evaluateWithLoadedRolePolicies` allows at most three reloads and then
deliberately fails closed. Therefore, repeated eviction can legitimately
exhaust the reload budget and return `false` for a granted request. There is
also a race after `versionCheckAndLoadRoles()` returns: the subsequent
`loadedRoles.getIfPresent()` checks are outside `rolePolicyLock`, so another
eviction in that window can make the check fail closed immediately. This is
secure in the sense that it does not grant excess access, but it is still a
false denial and contradicts the test's stated “no wrong deny” guarantee.
### 2. `roleClearGenerations` grows without a bound
```java
private final Map<Long, Long> roleClearGenerations = new
ConcurrentHashMap<>();
```
Entries are intentionally retained forever. Consequently, the map grows with
every distinct role whose policies have ever been cleared during the lifetime
of the process, including roles that are later deleted. The comment describes
the cost as “two longs per role”, but the actual representation includes boxed
`Long` objects, `ConcurrentHashMap` nodes/table entries, object headers, and
alignment overhead. On a long-running node with substantial role churn, this
becomes unbounded retained heap rather than a small fixed cache.
### 3. A single eviction can amplify concurrent reload work
After one role is cleared, every concurrent request whose recorded
generation is stale can independently enter `versionCheckAndLoadRoles()`. The
read/write lock protects JCasbin state mutation, but role/entity/resource
resolution occurs outside the write lock. There is no per-role
single-flight/coalescing mechanism, so many requests may perform the same
metadata/DB resolution concurrently before one completed reload becomes
visible. Under load, one cache eviction can therefore produce a burst of
duplicate metadata reads and increase the chance of hitting the bounded
reload/fail-closed path.
These observations are against the current two-commit PR head, not an
earlier revision.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
