mirrors/nomulus
1
0
Fork 0
mirror of https://github.com/google/nomulus synced 2025-12-23 14:25:44 +00:00
Code Issues Releases Wiki Activity

Use transaction time for deletion time cache ticker (#2848)

Browse Source 
Basically, what happened is that the cache's expireAfterWrite was being
called some number of milliseconds (say, 50-100) after the transaction
was started. That method used the transaction time instead of the
current time, so as a result the entries were sticking around 50-100ms
longer in the cache than they should have been.

This fix contains two parts, each of which I believe would be sufficient
on their own to fix the issue:
1. Use the currentTime passed in in Expiry::expireAfterCreate
2. Use the transaction time in the cache's Ticker. This keeps everything
   on the same schedule.
This commit is contained in:
gbrodman
2025-10-16 16:01:17 -04:00
committed by GitHub
parent ddd955e156
commit b144aafb22
2 changed files with 29 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files

21
core/src/main/java/google/registry/flows/domain/DomainDeletionTimeCache.java
View File

@@ -20,9 +20,11 @@ import com.github.benmanes.caffeine.cache.CacheLoader;
import com.github.benmanes.caffeine.cache.Caffeine;
import com.github.benmanes.caffeine.cache.Expiry;
import com.github.benmanes.caffeine.cache.LoadingCache;
import com.github.benmanes.caffeine.cache.Ticker;
import com.google.common.collect.ImmutableSet;
import google.registry.model.ForeignKeyUtils;
import google.registry.model.domain.Domain;
import google.registry.util.DateTimeUtils;
import java.util.Optional;
import org.joda.time.DateTime;
@@ -55,9 +57,9 @@ import org.joda.time.DateTime;
public class DomainDeletionTimeCache {
// Max expiry time is ten minutes
private static final int MAX_EXPIRY_MILLIS = 10 * 60 * 1000;
private static final long NANOS_IN_ONE_MILLISECOND = 100000L;
private static final long MAX_EXPIRY_NANOS = 10L * 60L * 1000L * NANOS_IN_ONE_MILLISECOND;
private static final int MAX_ENTRIES = 500;
private static final int NANOS_IN_ONE_MILLISECOND = 100000;
/**
* Expire after the max duration, or after the domain is set to drop (whichever comes first).
@@ -71,9 +73,13 @@ public class DomainDeletionTimeCache {
new Expiry<>() {
@Override
public long expireAfterCreate(String key, DateTime value, long currentTime) {
long millisUntilDeletion = value.getMillis() - tm().getTransactionTime().getMillis();
return NANOS_IN_ONE_MILLISECOND
* Math.max(0L, Math.min(MAX_EXPIRY_MILLIS, millisUntilDeletion));
// Watch out for Long overflow
long deletionTimeNanos =
value.equals(DateTimeUtils.END_OF_TIME)
? Long.MAX_VALUE
: value.getMillis() * NANOS_IN_ONE_MILLISECOND;
long nanosUntilDeletion = deletionTimeNanos - currentTime;
return Math.max(0L, Math.min(MAX_EXPIRY_NANOS, nanosUntilDeletion));
}
/** Reset the time entirely on update, as if we were creating the entry anew. */
@@ -101,9 +107,14 @@ public class DomainDeletionTimeCache {
return mostRecentResource == null ? null : mostRecentResource.deletionTime();
};
// Unfortunately, maintenance tasks aren't necessarily already in a transaction
private static final Ticker TRANSACTION_TIME_TICKER =
() -> tm().reTransact(() -> tm().getTransactionTime().getMillis() * NANOS_IN_ONE_MILLISECOND);
public static DomainDeletionTimeCache create() {
return new DomainDeletionTimeCache(
Caffeine.newBuilder()
.ticker(TRANSACTION_TIME_TICKER)
.expireAfter(EXPIRY_POLICY)
.maximumSize(MAX_ENTRIES)
.build(CACHE_LOADER));

14
core/src/test/java/google/registry/flows/domain/DomainDeletionTimeCacheTest.java
View File

@@ -26,6 +26,7 @@ import google.registry.testing.DatabaseHelper;
import google.registry.testing.FakeClock;
import java.util.Optional;
import org.joda.time.DateTime;
import org.joda.time.Duration;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;
@@ -67,7 +68,8 @@ public class DomainDeletionTimeCacheTest {
Domain domain = persistActiveDomain("domain.tld");
assertThat(getDeletionTimeFromCache("domain.tld")).hasValue(END_OF_TIME);
persistDomainAsDeleted(domain, clock.nowUtc().plusDays(1));
// Without intervention, the cache should have the old data
// Without intervention, the cache should have the old data, even if a few minutes have passed
clock.advanceBy(Duration.standardMinutes(5));
assertThat(getDeletionTimeFromCache("domain.tld")).hasValue(END_OF_TIME);
}
@@ -79,6 +81,16 @@ public class DomainDeletionTimeCacheTest {
assertThat(getDeletionTimeFromCache("domain.tld")).hasValue(clock.nowUtc().plusDays(1));
}
@Test
void testCache_expires() {
Domain domain = persistActiveDomain("domain.tld");
assertThat(getDeletionTimeFromCache("domain.tld")).hasValue(END_OF_TIME);
DateTime elevenMinutesFromNow = clock.nowUtc().plusMinutes(11);
persistDomainAsDeleted(domain, elevenMinutesFromNow);
clock.advanceBy(Duration.standardMinutes(30));
assertThat(getDeletionTimeFromCache("domain.tld")).hasValue(elevenMinutesFromNow);
}
private Optional<DateTime> getDeletionTimeFromCache(String domainName) {
return tm().transact(() -> cache.getDeletionTimeForDomain(domainName));
}