Refactor diminished returns tracking into DiminishedReturnsCache/State

Extracts per-player DR data out of PlayerProfile into two new classes:
- DiminishedReturnsState: thread-safe XP gain queue and rolling totals
- DiminishedReturnsCache: UUID-keyed server-side cache that keeps DR
  state alive across disconnect/reconnect to prevent window bypass

Fixes a bug where evictExpired() would remove fresh states that had
never registered any XP, orphaning the PlayerProfile reference and
allowing reconnects to bypass the DR window.

DR tracking is now skipped entirely (no state mutation) when the
diminished returns config option is disabled.

Fixes #3475. Fixes #3814.
This commit is contained in:
nossr50
2026-05-30 21:12:30 -07:00
parent 8e1b6deaa5
commit 3fb1369444
6 changed files with 501 additions and 20 deletions

View File

@ -0,0 +1,84 @@
package com.gmail.nossr50.datatypes.experience;
import com.gmail.nossr50.config.experience.ExperienceConfig;
import java.util.UUID;
import java.util.concurrent.ConcurrentHashMap;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
/**
* Server-side cache that keeps {@link DiminishedReturnsState} alive across a player's
* disconnect/reconnect cycle, preventing players from resetting their DR window by logging out.
*
* <p>Entries are keyed by player UUID. When the DR time window elapses the entry becomes
* eligible for removal; call {@link #evictExpired()} periodically (e.g. from
* {@link com.gmail.nossr50.runnables.player.ClearRegisteredXPGainTask}) to release stale entries
* and prevent unbounded cache growth.</p>
*
* <p>All public methods are safe to call from multiple threads.</p>
*/
public final class DiminishedReturnsCache {
private static final ConcurrentHashMap<UUID, DiminishedReturnsState> cache =
new ConcurrentHashMap<>();
private DiminishedReturnsCache() {}
/**
* Returns the existing {@link DiminishedReturnsState} for {@code uuid} if it still has active
* DR entries, or atomically replaces/creates a fresh one.
*
* <p>Passing {@code null} (offline or legacy profiles without a UUID) always returns a fresh,
* uncached state — those profiles never participate in the disconnect/reconnect bypass.</p>
*
* @param uuid the player's unique identifier, or {@code null} for uncached profiles
* @return a non-null {@link DiminishedReturnsState} for this player
*/
public static @NotNull DiminishedReturnsState getOrCreate(@Nullable final UUID uuid) {
if (uuid == null || !ExperienceConfig.getInstance().getDiminishedReturnsEnabled()) {
return new DiminishedReturnsState();
}
return cache.compute(uuid, (k, existing) -> {
// Keep the existing instance if it is either still active OR fresh (never used).
// A fresh existing state means the player's PlayerProfile (which holds a strong
// reference to it) has not yet gained any XP — replacing it now would orphan
// that reference and break the disconnect/reconnect bypass protection on the
// next XP gain.
if (existing != null && !existing.isEvictable()) {
return existing;
}
return new DiminishedReturnsState();
});
}
/**
* Removes all cache entries whose DR time window has fully elapsed. Fresh entries that
* have never registered XP are preserved — see {@link DiminishedReturnsState#isEvictable()}.
* Safe to call from any thread; intended for use in the periodic cleanup task.
*/
public static void evictExpired() {
cache.values().removeIf(DiminishedReturnsState::isEvictable);
}
/** Removes the cache entry for {@code uuid}. For test teardown only. */
static void remove(@Nullable final UUID uuid) {
if (uuid != null) {
cache.remove(uuid);
}
}
/** Clears every entry from the cache. For test teardown only. */
static void clearAll() {
cache.clear();
}
/** Returns the number of entries currently held in the cache. For test assertions only. */
static int size() {
return cache.size();
}
/** Returns {@code true} if the cache contains an entry for {@code uuid}. For test assertions only. */
static boolean contains(@NotNull final UUID uuid) {
return cache.containsKey(uuid);
}
}

View File

@ -0,0 +1,103 @@
package com.gmail.nossr50.datatypes.experience;
import com.gmail.nossr50.datatypes.skills.PrimarySkillType;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.DelayQueue;
import org.jetbrains.annotations.NotNull;
/**
* Thread-safe container for a player's Diminished Returns tracking data.
*
* <p>Stored in {@link DiminishedReturnsCache} keyed by player UUID so that the DR window
* survives a disconnect/reconnect cycle. Each skill's rolling XP total and the expiry
* queue are held here rather than directly in {@code PlayerProfile}.</p>
*/
public final class DiminishedReturnsState {
private final DelayQueue<SkillXpGain> gainedSkillsXp = new DelayQueue<>();
private final ConcurrentHashMap<PrimarySkillType, Float> rollingSkillsXp = new ConcurrentHashMap<>();
/**
* The epoch-millisecond timestamp of the last-expiring {@link SkillXpGain} that has been
* registered. When the current time exceeds this value every entry in the queue has expired
* and this state can safely be evicted from the cache without losing any DR data.
*
* <p>{@code volatile} ensures the write from one thread (e.g. the main thread registering XP)
* is visible to the scheduler thread that runs {@link DiminishedReturnsCache#evictExpired()}.
*/
private volatile long latestExpiryTimeMillis = 0L;
// Package-private: only DiminishedReturnsCache (same package) and PlayerProfile
// (via DiminishedReturnsCache.getOrCreate) should instantiate this class.
DiminishedReturnsState() {}
/**
* Returns {@code true} if at least one DR entry is still within its time window.
*
* <p>This is intentionally distinct from {@link #isEvictable()}: a freshly constructed state
* that has never registered any XP has no active entries but is also <em>not</em> evictable,
* because its owning {@code PlayerProfile} still holds a reference and may yet register XP
* on it. Removing it from the cache prematurely would break the disconnect/reconnect
* bypass protection.</p>
*/
public boolean hasActiveEntries() {
return latestExpiryTimeMillis != 0L && System.currentTimeMillis() < latestExpiryTimeMillis;
}
/**
* Returns {@code true} if this state has held DR entries and they have all expired.
* Fresh states (no XP ever registered) are never evictable.
*/
public boolean isEvictable() {
return latestExpiryTimeMillis != 0L && System.currentTimeMillis() >= latestExpiryTimeMillis;
}
/**
* Returns the rolling XP total recorded for the given skill within the current DR window.
* Returns {@code 0} if no XP has been registered or all entries have expired.
*
* @param skill the skill to query
* @return total registered XP for the skill, or {@code 0}
*/
public float getRegisteredXpGain(@NotNull final PrimarySkillType skill) {
return rollingSkillsXp.getOrDefault(skill, 0F);
}
/**
* Records an XP gain for DR tracking and advances the latest-expiry ceiling accordingly.
*
* @param skill the skill that gained XP
* @param xp the amount of XP gained
*/
public void registerXpGain(@NotNull final PrimarySkillType skill, final float xp) {
final SkillXpGain gain = new SkillXpGain(skill, xp);
gainedSkillsXp.add(gain);
rollingSkillsXp.merge(skill, xp, Float::sum);
// Advance the expiry ceiling so we know the exact moment all entries will have expired.
final long entryExpiry = gain.getExpiryTimeMillis();
if (entryExpiry > latestExpiryTimeMillis) {
latestExpiryTimeMillis = entryExpiry;
}
}
/**
* Polls the delay queue for expired entries and decrements the rolling skill totals.
* Called periodically by
* {@link com.gmail.nossr50.runnables.player.ClearRegisteredXPGainTask}.
*/
public void purgeExpiredXpGains() {
SkillXpGain gain;
while ((gain = gainedSkillsXp.poll()) != null) {
final PrimarySkillType skill = gain.getSkill();
final float expiredXp = gain.getXp();
rollingSkillsXp.compute(skill, (k, existing) -> {
if (existing == null) {
return null;
}
final float updated = existing - expiredXp;
return updated <= 0F ? null : updated;
});
}
}
}

View File

@ -25,6 +25,14 @@ public class SkillXpGain implements Delayed {
return xp;
}
/**
* Returns the absolute epoch-millisecond timestamp at which this entry expires.
* Used by {@link DiminishedReturnsState} to track the latest expiry across all entries.
*/
public long getExpiryTimeMillis() {
return expiryTime;
}
private static long getDuration() {
return TimeUnit.MINUTES.toMillis(
ExperienceConfig.getInstance().getDiminishedReturnsTimeInterval());

View File

@ -1,8 +1,9 @@
package com.gmail.nossr50.datatypes.player;
import com.gmail.nossr50.config.experience.ExperienceConfig;
import com.gmail.nossr50.datatypes.experience.DiminishedReturnsCache;
import com.gmail.nossr50.datatypes.experience.DiminishedReturnsState;
import com.gmail.nossr50.datatypes.experience.FormulaType;
import com.gmail.nossr50.datatypes.experience.SkillXpGain;
import com.gmail.nossr50.datatypes.skills.PrimarySkillType;
import com.gmail.nossr50.datatypes.skills.SuperAbilityType;
import com.gmail.nossr50.mcMMO;
@ -15,7 +16,6 @@ import java.util.EnumMap;
import java.util.Map;
import java.util.Objects;
import java.util.UUID;
import java.util.concurrent.DelayQueue;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
@ -41,10 +41,8 @@ public class PlayerProfile {
private final Map<UniqueDataType, Integer> uniquePlayerData = new EnumMap<>(
UniqueDataType.class); //Misc data that doesn't fit into other categories (chimaera wing, etc..)
// Store previous XP gains for diminished returns
private final DelayQueue<SkillXpGain> gainedSkillsXp = new DelayQueue<>();
private final Map<PrimarySkillType, Float> rollingSkillsXp = new EnumMap<>(
PrimarySkillType.class);
// Store previous XP gains for diminished returns (persisted across reconnects via cache)
private final DiminishedReturnsState diminishedReturnsState;
@Deprecated
public PlayerProfile(String playerName) {
@ -64,6 +62,7 @@ public class PlayerProfile {
public PlayerProfile(String playerName, @Nullable UUID uuid, int startingLevel) {
this.uuid = uuid;
this.playerName = playerName;
this.diminishedReturnsState = DiminishedReturnsCache.getOrCreate(uuid);
scoreboardTipsShown = 0;
@ -99,6 +98,8 @@ public class PlayerProfile {
this.playerName = playerName;
this.uuid = uuid;
this.scoreboardTipsShown = scoreboardTipsShown;
// This constructor is used for save copies only — do not pull DR state from cache.
this.diminishedReturnsState = DiminishedReturnsCache.getOrCreate(null);
skills.putAll(levelData);
skillsXp.putAll(xpData);
@ -404,13 +405,7 @@ public class PlayerProfile {
* @return xp Experience amount registered
*/
public float getRegisteredXpGain(PrimarySkillType primarySkillType) {
float xp = 0F;
if (rollingSkillsXp.get(primarySkillType) != null) {
xp = rollingSkillsXp.get(primarySkillType);
}
return xp;
return diminishedReturnsState.getRegisteredXpGain(primarySkillType);
}
/**
@ -420,19 +415,16 @@ public class PlayerProfile {
* @param xp Experience amount to add
*/
public void registerXpGain(PrimarySkillType primarySkillType, float xp) {
gainedSkillsXp.add(new SkillXpGain(primarySkillType, xp));
rollingSkillsXp.put(primarySkillType, getRegisteredXpGain(primarySkillType) + xp);
if (ExperienceConfig.getInstance().getDiminishedReturnsEnabled()) {
diminishedReturnsState.registerXpGain(primarySkillType, xp);
}
}
/**
* Remove experience gains older than a given time This is used for diminished XP returns
*/
public void purgeExpiredXpGains() {
SkillXpGain gain;
while ((gain = gainedSkillsXp.poll()) != null) {
rollingSkillsXp.put(gain.getSkill(),
getRegisteredXpGain(gain.getSkill()) - gain.getXp());
}
diminishedReturnsState.purgeExpiredXpGains();
}
/**

View File

@ -1,5 +1,6 @@
package com.gmail.nossr50.runnables.player;
import com.gmail.nossr50.datatypes.experience.DiminishedReturnsCache;
import com.gmail.nossr50.datatypes.player.McMMOPlayer;
import com.gmail.nossr50.util.CancellableRunnable;
import com.gmail.nossr50.util.player.UserManager;
@ -10,5 +11,7 @@ public class ClearRegisteredXPGainTask extends CancellableRunnable {
for (McMMOPlayer mmoPlayer : UserManager.getPlayers()) {
mmoPlayer.getProfile().purgeExpiredXpGains();
}
// Evict cache entries whose DR window has fully elapsed to prevent unbounded growth.
DiminishedReturnsCache.evictExpired();
}
}

View File

@ -0,0 +1,291 @@
package com.gmail.nossr50.datatypes.experience;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNotSame;
import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.mockStatic;
import static org.mockito.Mockito.when;
import com.gmail.nossr50.config.experience.ExperienceConfig;
import java.lang.reflect.Field;
import java.util.UUID;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.mockito.MockedStatic;
/**
* Unit tests for {@link DiminishedReturnsCache}.
*
* <p>These tests verify the cache's UUID-keyed persistence semantics and eviction logic.
* {@link ExperienceConfig} is mocked so tests do not require a running Minecraft server.
* By default DR is enabled; individual tests that need DR disabled override that mock.
*/
class DiminishedReturnsCacheTest {
private MockedStatic<ExperienceConfig> mockedExperienceConfig;
private ExperienceConfig experienceConfigMock;
@BeforeEach
void setupMocks() {
experienceConfigMock = mock(ExperienceConfig.class);
mockedExperienceConfig = mockStatic(ExperienceConfig.class);
mockedExperienceConfig.when(ExperienceConfig::getInstance).thenReturn(experienceConfigMock);
// Default: DR is enabled so UUID-based caching is exercised
when(experienceConfigMock.getDiminishedReturnsEnabled()).thenReturn(true);
}
@AfterEach
void teardown() {
mockedExperienceConfig.close();
DiminishedReturnsCache.clearAll();
}
/**
* Uses reflection to force the {@code latestExpiryTimeMillis} field to an arbitrary value.
* This lets tests drive the {@code hasActiveEntries()} return value without waiting for real
* time to elapse or calling into ExperienceConfig.
*/
private static void setLatestExpiry(final DiminishedReturnsState state, final long epochMillis)
throws Exception {
final Field field = DiminishedReturnsState.class.getDeclaredField("latestExpiryTimeMillis");
field.setAccessible(true);
field.setLong(state, epochMillis);
}
@Nested
class DisabledBehavior {
@Test
void returnsUncachedStateWhenDrDisabled() {
// Given - DR is disabled in config
when(experienceConfigMock.getDiminishedReturnsEnabled()).thenReturn(false);
final UUID playerUuid = UUID.randomUUID();
// When - two getOrCreate calls for the same UUID
final DiminishedReturnsState firstState = DiminishedReturnsCache.getOrCreate(playerUuid);
final DiminishedReturnsState secondState = DiminishedReturnsCache.getOrCreate(playerUuid);
// Then - a fresh uncached state is returned each time; no bypass protection needed
assertNotSame(firstState, secondState,
"DR disabled: states must not be cached across calls");
assertEquals(0, DiminishedReturnsCache.size(),
"DR disabled: cache must remain empty");
}
@Test
void cacheRemainsEmptyWhenDrDisabled() {
// Given - DR is disabled
when(experienceConfigMock.getDiminishedReturnsEnabled()).thenReturn(false);
// When - several players look up their DR state
DiminishedReturnsCache.getOrCreate(UUID.randomUUID());
DiminishedReturnsCache.getOrCreate(UUID.randomUUID());
DiminishedReturnsCache.getOrCreate(UUID.randomUUID());
// Then - nothing is stored
assertEquals(0, DiminishedReturnsCache.size(),
"DR disabled: cache must never grow");
}
}
@Nested
class NullUuidBehavior {
@Test
void alwaysReturnsFreshInstance() {
// Given - two calls with null (offline / legacy profile)
// When
final DiminishedReturnsState firstState = DiminishedReturnsCache.getOrCreate(null);
final DiminishedReturnsState secondState = DiminishedReturnsCache.getOrCreate(null);
// Then - every call returns a distinct, uncached object
assertNotNull(firstState);
assertNotNull(secondState);
assertNotSame(firstState, secondState,
"null-UUID calls must never share a cached state");
}
@Test
void doesNotGrowCache() {
// Given - several calls with null
// When
DiminishedReturnsCache.getOrCreate(null);
DiminishedReturnsCache.getOrCreate(null);
DiminishedReturnsCache.getOrCreate(null);
// Then - cache should be empty (null profiles are never stored)
assertEquals(0, DiminishedReturnsCache.size(),
"null-UUID states must not be stored in the cache");
}
}
@Nested
class SameUuidPersistence {
@Test
void returnsSameInstanceForSameUuidWhileActive() throws Exception {
// Given - a UUID with an active DR state (expiry set far in the future)
final UUID playerUuid = UUID.randomUUID();
final DiminishedReturnsState firstState = DiminishedReturnsCache.getOrCreate(playerUuid);
setLatestExpiry(firstState, System.currentTimeMillis() + 600_000L); // 10 min ahead
// When - a second lookup with the same UUID (simulates reconnect while DR is active)
final DiminishedReturnsState secondState = DiminishedReturnsCache.getOrCreate(playerUuid);
// Then - identical instance returned, so DR window is preserved
assertSame(firstState, secondState,
"active DR state must be returned as-is on reconnect to prevent bypass");
}
@Test
void returnsFreshInstanceWhenPreviousStateHasExpired() throws Exception {
// Given - a UUID whose cached state has fully elapsed (latestExpiry in the past, non-zero)
final UUID playerUuid = UUID.randomUUID();
final DiminishedReturnsState expiredState = DiminishedReturnsCache.getOrCreate(playerUuid);
// 1L = had XP at some point AND that XP has now expired
setLatestExpiry(expiredState, 1L);
// When - player reconnects after their DR window has elapsed
final DiminishedReturnsState freshState = DiminishedReturnsCache.getOrCreate(playerUuid);
// Then - a new state is created; no stale XP data carries over
assertNotSame(expiredState, freshState,
"expired state should be replaced with a fresh one on next getOrCreate");
}
}
@Nested
class UuidIsolation {
@Test
void differentUuidsGetIndependentStates() throws Exception {
// Given - two distinct players
final UUID uuidAlpha = UUID.randomUUID();
final UUID uuidBeta = UUID.randomUUID();
// When - both look up their DR state (mark both active so cache keeps them)
final DiminishedReturnsState stateAlpha = DiminishedReturnsCache.getOrCreate(uuidAlpha);
final DiminishedReturnsState stateBeta = DiminishedReturnsCache.getOrCreate(uuidBeta);
setLatestExpiry(stateAlpha, System.currentTimeMillis() + 600_000L);
setLatestExpiry(stateBeta, System.currentTimeMillis() + 600_000L);
// Then - they receive independent state containers
assertNotSame(stateAlpha, stateBeta,
"each player UUID must have its own isolated DR state");
}
}
@Nested
class Eviction {
@Test
void evictExpiredRemovesStateWithExpiredEntries() throws Exception {
// Given - a player whose DR state has expired (latestExpiry in the past, non-zero)
final UUID playerUuid = UUID.randomUUID();
final DiminishedReturnsState expiredState = DiminishedReturnsCache.getOrCreate(playerUuid);
// 1L = "had XP registered" (non-zero) AND "already expired" (< currentTimeMillis)
setLatestExpiry(expiredState, 1L);
assertTrue(DiminishedReturnsCache.contains(playerUuid),
"state should be in cache before eviction");
// When
DiminishedReturnsCache.evictExpired();
// Then
assertFalse(DiminishedReturnsCache.contains(playerUuid),
"expired state should be removed by evictExpired");
assertEquals(0, DiminishedReturnsCache.size());
}
@Test
void evictExpiredPreservesFreshStateThatNeverRegisteredXp() {
// Regression: a player who joined but has not yet gained any XP must keep their cached
// state. Otherwise the cache entry is evicted, then once XP is registered on the
// PlayerProfile's orphaned reference, a disconnect/reconnect creates a new state and
// bypasses the DR window.
// Given - a fresh state (latestExpiryTimeMillis defaults to 0)
final UUID playerUuid = UUID.randomUUID();
DiminishedReturnsCache.getOrCreate(playerUuid);
assertTrue(DiminishedReturnsCache.contains(playerUuid));
// When
DiminishedReturnsCache.evictExpired();
// Then - fresh state must NOT be evicted
assertTrue(DiminishedReturnsCache.contains(playerUuid),
"fresh state with no registered XP must survive eviction "
+ "to preserve reconnect bypass protection");
}
@Test
void evictExpiredPreservesStateWithActiveEntries() throws Exception {
// Given - a player with an active DR state
final UUID playerUuid = UUID.randomUUID();
final DiminishedReturnsState activeState = DiminishedReturnsCache.getOrCreate(playerUuid);
setLatestExpiry(activeState, System.currentTimeMillis() + 600_000L); // active
assertTrue(activeState.hasActiveEntries());
// When
DiminishedReturnsCache.evictExpired();
// Then - active entry must survive the sweep
assertTrue(DiminishedReturnsCache.contains(playerUuid),
"active DR state must not be evicted");
assertEquals(1, DiminishedReturnsCache.size());
}
@Test
void evictExpiredOnlyRemovesExpiredEntries() throws Exception {
// Given - one active player and one whose DR window has elapsed
final UUID activePlayerUuid = UUID.randomUUID();
final UUID expiredPlayerUuid = UUID.randomUUID();
final DiminishedReturnsState activeState = DiminishedReturnsCache.getOrCreate(activePlayerUuid);
final DiminishedReturnsState expiredState = DiminishedReturnsCache.getOrCreate(expiredPlayerUuid);
setLatestExpiry(activeState, System.currentTimeMillis() + 600_000L);
setLatestExpiry(expiredState, 1L);
// When
DiminishedReturnsCache.evictExpired();
// Then - only the expired entry is removed
assertTrue(DiminishedReturnsCache.contains(activePlayerUuid),
"active player state must survive eviction");
assertFalse(DiminishedReturnsCache.contains(expiredPlayerUuid),
"expired player state must be removed");
assertEquals(1, DiminishedReturnsCache.size());
}
}
@Nested
class HasActiveEntries {
@Test
void freshStateHasNoActiveEntries() {
// Given - a freshly constructed state (latestExpiryTimeMillis defaults to 0)
final DiminishedReturnsState freshState = new DiminishedReturnsState();
// When / Then
assertFalse(freshState.hasActiveEntries(),
"a newly created state with no XP registrations should report no active entries");
}
@Test
void stateWithFutureExpiryHasActiveEntries() throws Exception {
// Given - a state whose expiry ceiling has been pushed into the future
final DiminishedReturnsState state = new DiminishedReturnsState();
setLatestExpiry(state, System.currentTimeMillis() + 600_000L);
// When / Then
assertTrue(state.hasActiveEntries(),
"state with a future latestExpiryTimeMillis should report active entries");
}
}
}