Make ICANN reporting not fail on success upload (#791)

* Make ICANN reporting not fail on success upload

According to the spec
(https://tools.ietf.org/html/draft-lozano-icann-registry-interfaces-13#page-16),
when an upload succeeds (HTTP response code 200), the result code
contained in the response message is always 1000 (success). So there is
no need to parse the response content and check the result code. Given
that we are having a problem parsing the response content due to encoding,
it is best that we don't check it so as to not get false negative
alerts when the upload is successful.

The current logic also has a bug: HttpRequest.execute() will by default
throw when the response code is non-20X. Therefore for a 400 response,
our parsing logic never runs on it. Coincidentally, this month when we
uploaded the July activity report (due to stale cursors), we get 400
responses (due to existing reports on the ICANN servers). The stack
trace printed for the thrown exceptions from the 400 responses contained
correctly parsed response contents. This lead us to believe that the issue with
encoding was transient last month. However when we tried again to upload this
month's report, our parser failed again (because the response code was 200 this
time, and our parser actually ran on the response contents).

This seems to suggest that ICANN is sending back readable response
contents, but our parser somehow failed to understand it, assuming that
ICANN is using the same encoding for 200 (which we tried and failed to
parse) and 400 response contents (which caused an exception and was printed
corrected in the stack trace).

This PR changed the transport behavior so that it doesn't throw
automatically for non-20X responses. We will print the content for both
200 and 400 responses, but only try to parse 400 response content. We
put the 400 response in an HttpResponseException and print stack trace
from it, which should display the content correctly so that we can
compare it with the result of our own parsing.

* Add tests

core/build.gradle

@@ -63,8 +63,6 @@ def dockerIncompatibleTestPatterns = [
     // methods, so we exclude the whole test class.
     "google/registry/tools/params/PathParameterTest.*",
     "google/registry/persistence/PersistenceModuleTest.*",
-    // This test is failing in docker when using Java 11. The cause is unclear.
-    "google/registry/tools/DomainLockUtilsTest.*",
 ]
 
 // Tests that conflict with members of both the main test suite and the

core/gradle/dependency-locks/testCompile.lockfile

@@ -6,9 +6,9 @@ aopalliance:aopalliance:1.0
 args4j:args4j:2.33
 cglib:cglib-nodep:2.2
 com.beust:jcommander:1.60
-com.fasterxml.jackson.core:jackson-annotations:2.10.2
-com.fasterxml.jackson.core:jackson-core:2.10.2
-com.fasterxml.jackson.core:jackson-databind:2.10.2
+com.fasterxml.jackson.core:jackson-annotations:2.11.2
+com.fasterxml.jackson.core:jackson-core:2.11.2
+com.fasterxml.jackson.core:jackson-databind:2.11.2
 com.fasterxml:classmate:1.5.1
 com.github.jnr:jffi:1.2.23
 com.github.jnr:jnr-a64asm:1.0.0

core/gradle/dependency-locks/testCompileClasspath.lockfile

@@ -6,9 +6,9 @@ aopalliance:aopalliance:1.0
 args4j:args4j:2.33
 cglib:cglib-nodep:2.2
 com.beust:jcommander:1.60
-com.fasterxml.jackson.core:jackson-annotations:2.10.2
-com.fasterxml.jackson.core:jackson-core:2.10.2
-com.fasterxml.jackson.core:jackson-databind:2.10.2
+com.fasterxml.jackson.core:jackson-annotations:2.11.2
+com.fasterxml.jackson.core:jackson-core:2.11.2
+com.fasterxml.jackson.core:jackson-databind:2.11.2
 com.fasterxml:classmate:1.5.1
 com.github.jnr:jffi:1.2.23
 com.github.jnr:jnr-a64asm:1.0.0

core/gradle/dependency-locks/testRuntime.lockfile

@@ -6,9 +6,9 @@ aopalliance:aopalliance:1.0
 args4j:args4j:2.33
 cglib:cglib-nodep:2.2
 com.beust:jcommander:1.60
-com.fasterxml.jackson.core:jackson-annotations:2.10.2
-com.fasterxml.jackson.core:jackson-core:2.10.2
-com.fasterxml.jackson.core:jackson-databind:2.10.2
+com.fasterxml.jackson.core:jackson-annotations:2.11.2
+com.fasterxml.jackson.core:jackson-core:2.11.2
+com.fasterxml.jackson.core:jackson-databind:2.11.2
 com.fasterxml:classmate:1.5.1
 com.github.jnr:jffi:1.2.23
 com.github.jnr:jnr-a64asm:1.0.0

core/gradle/dependency-locks/testRuntimeClasspath.lockfile

@@ -6,9 +6,9 @@ aopalliance:aopalliance:1.0
 args4j:args4j:2.33
 cglib:cglib-nodep:2.2
 com.beust:jcommander:1.60
-com.fasterxml.jackson.core:jackson-annotations:2.10.2
-com.fasterxml.jackson.core:jackson-core:2.10.2
-com.fasterxml.jackson.core:jackson-databind:2.10.2
+com.fasterxml.jackson.core:jackson-annotations:2.11.2
+com.fasterxml.jackson.core:jackson-core:2.11.2
+com.fasterxml.jackson.core:jackson-databind:2.11.2
 com.fasterxml:classmate:1.5.1
 com.github.jnr:jffi:1.2.23
 com.github.jnr:jnr-a64asm:1.0.0

core/src/main/java/google/registry/batch/AsyncTaskEnqueuer.java

@@ -169,16 +169,20 @@ public final class AsyncTaskEnqueuer {
         lock.getRelockDuration().isPresent(),
         "Lock with ID %s not configured for relock",
         lock.getRevisionId());
+    enqueueDomainRelock(lock.getRelockDuration().get(), lock.getRevisionId(), 0);
+  }
+
+  /** Enqueues a task to asynchronously re-lock a registry-locked domain after it was unlocked. */
+  void enqueueDomainRelock(Duration countdown, long lockRevisionId, int previousAttempts) {
     String backendHostname = appEngineServiceUtils.getServiceHostname("backend");
     addTaskToQueueWithRetry(
         asyncActionsPushQueue,
         TaskOptions.Builder.withUrl(RelockDomainAction.PATH)
             .method(Method.POST)
             .header("Host", backendHostname)
-            .param(
-                RelockDomainAction.OLD_UNLOCK_REVISION_ID_PARAM,
-                String.valueOf(lock.getRevisionId()))
-            .countdownMillis(lock.getRelockDuration().get().getMillis()));
+            .param(RelockDomainAction.OLD_UNLOCK_REVISION_ID_PARAM, String.valueOf(lockRevisionId))
+            .param(RelockDomainAction.PREVIOUS_ATTEMPTS_PARAM, String.valueOf(previousAttempts))
+            .countdownMillis(countdown.getMillis()));
   }
 
   /**

core/src/main/java/google/registry/batch/BatchModule.java

@@ -21,6 +21,7 @@ import static google.registry.batch.AsyncTaskEnqueuer.PARAM_RESOURCE_KEY;
 import static google.registry.batch.AsyncTaskEnqueuer.QUEUE_ASYNC_ACTIONS;
 import static google.registry.batch.AsyncTaskEnqueuer.QUEUE_ASYNC_DELETE;
 import static google.registry.batch.AsyncTaskEnqueuer.QUEUE_ASYNC_HOST_RENAME;
+import static google.registry.request.RequestParameters.extractIntParameter;
 import static google.registry.request.RequestParameters.extractLongParameter;
 import static google.registry.request.RequestParameters.extractOptionalBooleanParameter;
 import static google.registry.request.RequestParameters.extractOptionalIntParameter;
@@ -94,9 +95,15 @@ public class BatchModule {
   }
 
   @Provides
-  @Parameter("oldUnlockRevisionId")
+  @Parameter(RelockDomainAction.OLD_UNLOCK_REVISION_ID_PARAM)
   static long provideOldUnlockRevisionId(HttpServletRequest req) {
-    return extractLongParameter(req, "oldUnlockRevisionId");
+    return extractLongParameter(req, RelockDomainAction.OLD_UNLOCK_REVISION_ID_PARAM);
+  }
+
+  @Provides
+  @Parameter(RelockDomainAction.PREVIOUS_ATTEMPTS_PARAM)
+  static int providePreviousAttempts(HttpServletRequest req) {
+    return extractIntParameter(req, RelockDomainAction.PREVIOUS_ATTEMPTS_PARAM);
   }
 
   @Provides

core/src/main/java/google/registry/batch/RelockDomainAction.java

@@ -15,19 +15,23 @@
 package google.registry.batch;
 
 import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static google.registry.model.ofy.ObjectifyService.ofy;
 import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
+import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
 import static google.registry.request.Action.Method.POST;
 import static google.registry.tools.LockOrUnlockDomainCommand.REGISTRY_LOCK_STATUSES;
-import static javax.servlet.http.HttpServletResponse.SC_INTERNAL_SERVER_ERROR;
 import static javax.servlet.http.HttpServletResponse.SC_NO_CONTENT;
 import static javax.servlet.http.HttpServletResponse.SC_OK;
 
 import com.google.common.collect.ImmutableSet;
 import com.google.common.flogger.FluentLogger;
 import com.google.common.net.MediaType;
+import google.registry.config.RegistryConfig.Config;
 import google.registry.model.domain.DomainBase;
 import google.registry.model.eppcommon.StatusValue;
+import google.registry.model.registrar.Registrar;
+import google.registry.model.registrar.RegistrarContact;
 import google.registry.model.registry.RegistryLockDao;
 import google.registry.request.Action;
 import google.registry.request.Parameter;
@@ -36,11 +40,15 @@ import google.registry.request.auth.Auth;
 import google.registry.schema.domain.RegistryLock;
 import google.registry.tools.DomainLockUtils;
 import google.registry.util.DateTimeUtils;
+import google.registry.util.EmailMessage;
+import google.registry.util.SendEmailService;
+import java.util.Optional;
 import javax.inject.Inject;
+import javax.mail.internet.AddressException;
+import javax.mail.internet.InternetAddress;
+import org.joda.time.Duration;
 
-/**
- * Task that relocks a previously-Registry-Locked domain after some predetermined period of time.
- */
+/** Task that re-locks a previously-Registry-Locked domain after a predetermined period of time. */
 @Action(
     service = Action.Service.BACKEND,
     path = RelockDomainAction.PATH,
@@ -51,30 +59,78 @@ public class RelockDomainAction implements Runnable {
 
   public static final String PATH = "/_dr/task/relockDomain";
   public static final String OLD_UNLOCK_REVISION_ID_PARAM = "oldUnlockRevisionId";
+  public static final String PREVIOUS_ATTEMPTS_PARAM = "previousAttempts";
+
+  static final int ATTEMPTS_BEFORE_SLOWDOWN = 36; // every ten minutes for six hours then every hour
+  static final int FAILURES_BEFORE_EMAIL = 2; // email after three failures, one half hour
+  private static final Duration TEN_MINUTES = Duration.standardMinutes(10);
+  private static final Duration ONE_HOUR = Duration.standardHours(1);
+
+  private static final String RELOCK_SUCCESS_EMAIL_TEMPLATE =
+      "The domain %s was successfully re-locked.\n\nPlease contact support at %s if you have any "
+          + "questions.";
+  private static final String RELOCK_NON_RETRYABLE_FAILURE_EMAIL_TEMPLATE =
+      "There was an error when automatically re-locking %s. Error message: %s\n\nPlease contact "
+          + "support at %s if you have any questions.";
+  private static final String RELOCK_TRANSIENT_FAILURE_EMAIL_TEMPLATE =
+      "There was an unexpected error when automatically re-locking %s. We will continue retrying "
+          + "the lock for five hours. Please contact support at %s if you have any questions";
+  private static final String RELOCK_UNKNOWN_ID_FAILURE_EMAIL_TEMPLATE =
+      "The old lock with revision ID %d is not present or is not accessible";
 
   private static final FluentLogger logger = FluentLogger.forEnclosingClass();
 
   private final long oldUnlockRevisionId;
+  private final int previousAttempts;
+  private final InternetAddress alertRecipientAddress;
+  private final InternetAddress gSuiteOutgoingEmailAddress;
+  private final String supportEmail;
+  private final SendEmailService sendEmailService;
   private final DomainLockUtils domainLockUtils;
   private final Response response;
+  private final AsyncTaskEnqueuer asyncTaskEnqueuer;
 
   @Inject
   public RelockDomainAction(
       @Parameter(OLD_UNLOCK_REVISION_ID_PARAM) long oldUnlockRevisionId,
+      @Parameter(PREVIOUS_ATTEMPTS_PARAM) int previousAttempts,
+      @Config("alertRecipientEmailAddress") InternetAddress alertRecipientAddress,
+      @Config("gSuiteOutgoingEmailAddress") InternetAddress gSuiteOutgoingEmailAddress,
+      @Config("supportEmail") String supportEmail,
+      SendEmailService sendEmailService,
       DomainLockUtils domainLockUtils,
-      Response response) {
+      Response response,
+      AsyncTaskEnqueuer asyncTaskEnqueuer) {
     this.oldUnlockRevisionId = oldUnlockRevisionId;
+    this.previousAttempts = previousAttempts;
+    this.alertRecipientAddress = alertRecipientAddress;
+    this.gSuiteOutgoingEmailAddress = gSuiteOutgoingEmailAddress;
+    this.supportEmail = supportEmail;
+    this.sendEmailService = sendEmailService;
     this.domainLockUtils = domainLockUtils;
     this.response = response;
+    this.asyncTaskEnqueuer = asyncTaskEnqueuer;
   }
 
   @Override
   public void run() {
-    jpaTm().transact(this::relockDomain);
+    /* We wish to manually control our retry behavior, in order to limit the number of retries
+     * and/or notify registrars / support only after a certain number of retries, or only
+     * with a certain type of failure. AppEngine will automatically retry on any non-2xx status
+     * code, so return SC_NO_CONTENT (204) by default to avoid this auto-retry.
+     *
+     * See https://cloud.google.com/appengine/docs/standard/java/taskqueue/push/retrying-tasks
+     * for more details on retry behavior. */
+    response.setStatus(SC_NO_CONTENT);
+    response.setContentType(MediaType.PLAIN_TEXT_UTF_8);
+
+    // nb: DomainLockUtils relies on the JPA transaction being the outermost transaction
+    jpaTm().transact(() -> tm().transact(this::relockDomain));
   }
 
   private void relockDomain() {
-    RegistryLock oldLock;
+    RegistryLock oldLock = null;
+    DomainBase domain;
     try {
       oldLock =
           RegistryLockDao.getByRevisionId(oldUnlockRevisionId)
@@ -82,87 +138,187 @@ public class RelockDomainAction implements Runnable {
                   () ->
                       new IllegalArgumentException(
                           String.format("Unknown revision ID %d", oldUnlockRevisionId)));
-      DomainBase domain =
+      domain =
           ofy()
               .load()
               .type(DomainBase.class)
               .id(oldLock.getRepoId())
               .now()
               .cloneProjectedAtTime(jpaTm().getTransactionTime());
-
-      if (domain.getStatusValues().containsAll(REGISTRY_LOCK_STATUSES)
-          || oldLock.getRelock() != null) {
-        // The domain was manually locked, so we shouldn't worry about relocking
-        String message =
-            String.format(
-                "Domain %s is already manually relocked, skipping automated relock.",
-                domain.getDomainName());
-        logger.atInfo().log(message);
-        // SC_NO_CONTENT (204) skips retry -- see the comment below
-        response.setStatus(SC_NO_CONTENT);
-        response.setContentType(MediaType.PLAIN_TEXT_UTF_8);
-        response.setPayload(message);
-        return;
-      }
-      verifyDomainAndLockState(oldLock, domain);
     } catch (Throwable t) {
-      /* If there's a bad verification code or the domain is in a bad state, we won't want to retry.
-       * AppEngine will retry on non-2xx error codes, so we return SC_NO_CONTENT (204) to avoid it.
-       *
-       * See https://cloud.google.com/appengine/docs/standard/java/taskqueue/push/retrying-tasks
-       * for more details on retry behavior. */
-      logger.atWarning().withCause(t).log(
-          "Exception when attempting to relock domain with old revision ID %d.",
-          oldUnlockRevisionId);
-      response.setStatus(SC_NO_CONTENT);
-      response.setContentType(MediaType.PLAIN_TEXT_UTF_8);
-      response.setPayload(String.format("Relock failed: %s", t.getMessage()));
+      handleTransientFailure(Optional.ofNullable(oldLock), t);
       return;
     }
-    applyRelock(oldLock);
+
+    if (domain.getStatusValues().containsAll(REGISTRY_LOCK_STATUSES)
+        || oldLock.getRelock() != null) {
+      // The domain was manually locked, so we shouldn't worry about re-locking
+      String message =
+          String.format(
+              "Domain %s is already manually re-locked, skipping automated re-lock.",
+              domain.getDomainName());
+      logger.atInfo().log(message);
+      response.setPayload(message);
+      return;
+    }
+    try {
+      verifyDomainAndLockState(oldLock, domain);
+    } catch (Throwable t) {
+      // If the domain was, for example, transferred, then notify the old registrar and don't retry.
+      handleNonRetryableFailure(oldLock, t);
+      return;
+    }
+    try {
+      applyRelock(oldLock);
+    } catch (Throwable t) {
+      handleTransientFailure(Optional.of(oldLock), t);
+    }
   }
 
   private void applyRelock(RegistryLock oldLock) {
-    try {
-      domainLockUtils.administrativelyApplyLock(
-          oldLock.getDomainName(),
-          oldLock.getRegistrarId(),
-          oldLock.getRegistrarPocId(),
-          oldLock.isSuperuser());
-      logger.atInfo().log("Relocked domain %s.", oldLock.getDomainName());
-      response.setStatus(SC_OK);
-    } catch (Throwable t) {
-      // Any errors that occur here are unexpected, so we should retry. Return a non-2xx
-      // error code to get AppEngine to retry
-      logger.atSevere().withCause(t).log(
-          "Exception when attempting to relock domain %s.", oldLock.getDomainName());
-      response.setStatus(SC_INTERNAL_SERVER_ERROR);
-      response.setContentType(MediaType.PLAIN_TEXT_UTF_8);
-      response.setPayload(String.format("Relock failed: %s", t.getMessage()));
+    domainLockUtils.administrativelyApplyLock(
+        oldLock.getDomainName(),
+        oldLock.getRegistrarId(),
+        oldLock.getRegistrarPocId(),
+        oldLock.isSuperuser());
+    logger.atInfo().log("Re-locked domain %s.", oldLock.getDomainName());
+    response.setStatus(SC_OK);
+    // Only send a success email if we previously sent a failure email
+    if (previousAttempts > FAILURES_BEFORE_EMAIL) {
+      sendSuccessEmail(oldLock);
     }
   }
 
   private void verifyDomainAndLockState(RegistryLock oldLock, DomainBase domain) {
     // Domain shouldn't be deleted or have a pending transfer/delete
     String domainName = domain.getDomainName();
-    checkArgument(
-        !DateTimeUtils.isAtOrAfter(jpaTm().getTransactionTime(), domain.getDeletionTime()),
-        "Domain %s has been deleted",
-        domainName);
     ImmutableSet<StatusValue> statusValues = domain.getStatusValues();
     checkArgument(
         !statusValues.contains(StatusValue.PENDING_DELETE),
-        "Domain %s has a pending delete",
+        "Domain %s has a pending delete.",
+        domainName);
+    checkArgument(
+        !DateTimeUtils.isAtOrAfter(jpaTm().getTransactionTime(), domain.getDeletionTime()),
+        "Domain %s has been deleted.",
         domainName);
     checkArgument(
         !statusValues.contains(StatusValue.PENDING_TRANSFER),
-        "Domain %s has a pending transfer",
+        "Domain %s has a pending transfer.",
         domainName);
     checkArgument(
         domain.getCurrentSponsorClientId().equals(oldLock.getRegistrarId()),
-        "Domain %s has been transferred from registrar %s to registrar %s since the unlock",
+        "Domain %s has been transferred from registrar %s to registrar %s since the unlock.",
         domainName,
         oldLock.getRegistrarId(),
         domain.getCurrentSponsorClientId());
   }
+
+  private void handleNonRetryableFailure(RegistryLock oldLock, Throwable t) {
+    logger.atWarning().withCause(t).log(
+        "Exception thrown when attempting to re-lock domain with old revision ID %d.",
+        oldUnlockRevisionId);
+    response.setPayload(String.format("Re-lock failed: %s", t.getMessage()));
+
+    String body =
+        String.format(
+            RELOCK_NON_RETRYABLE_FAILURE_EMAIL_TEMPLATE,
+            oldLock.getDomainName(),
+            t.getMessage(),
+            supportEmail);
+    sendEmailService.sendEmail(
+        EmailMessage.newBuilder()
+            .setFrom(gSuiteOutgoingEmailAddress)
+            .setBody(body)
+            .setSubject(String.format("Error re-locking domain %s", oldLock.getDomainName()))
+            .setRecipients(getEmailRecipients(oldLock.getRegistrarId()))
+            .build());
+  }
+
+  private void handleTransientFailure(Optional<RegistryLock> oldLock, Throwable t) {
+    String message = String.format("Re-lock failed: %s", t.getMessage());
+    logger.atSevere().withCause(t).log(message);
+    response.setPayload(message);
+
+    if (previousAttempts == FAILURES_BEFORE_EMAIL) {
+      if (oldLock.isPresent()) {
+        sendGenericTransientFailureEmail(oldLock.get());
+      } else {
+        // if the old lock isn't present, something has gone horribly wrong
+        sendUnknownRevisionIdAlertEmail();
+      }
+    }
+    Duration timeBeforeRetry = previousAttempts < ATTEMPTS_BEFORE_SLOWDOWN ? TEN_MINUTES : ONE_HOUR;
+    asyncTaskEnqueuer.enqueueDomainRelock(
+        timeBeforeRetry, oldUnlockRevisionId, previousAttempts + 1);
+  }
+
+  private void sendSuccessEmail(RegistryLock oldLock) {
+    String body =
+        String.format(RELOCK_SUCCESS_EMAIL_TEMPLATE, oldLock.getDomainName(), supportEmail);
+
+    sendEmailService.sendEmail(
+        EmailMessage.newBuilder()
+            .setFrom(gSuiteOutgoingEmailAddress)
+            .setBody(body)
+            .setSubject(String.format("Successful re-lock of domain %s", oldLock.getDomainName()))
+            .setRecipients(getEmailRecipients(oldLock.getRegistrarId()))
+            .build());
+  }
+
+  private void sendGenericTransientFailureEmail(RegistryLock oldLock) {
+    String body =
+        String.format(
+            RELOCK_TRANSIENT_FAILURE_EMAIL_TEMPLATE, oldLock.getDomainName(), supportEmail);
+    // For an unexpected failure, notify both the lock-enabled contacts and our alerting email
+    ImmutableSet<InternetAddress> allRecipients =
+        new ImmutableSet.Builder<InternetAddress>()
+            .addAll(getEmailRecipients(oldLock.getRegistrarId()))
+            .add(alertRecipientAddress)
+            .build();
+    sendEmailService.sendEmail(
+        EmailMessage.newBuilder()
+            .setFrom(gSuiteOutgoingEmailAddress)
+            .setBody(body)
+            .setSubject(String.format("Error re-locking domain %s", oldLock.getDomainName()))
+            .setRecipients(allRecipients)
+            .build());
+  }
+
+  private void sendUnknownRevisionIdAlertEmail() {
+    sendEmailService.sendEmail(
+        EmailMessage.newBuilder()
+            .setFrom(gSuiteOutgoingEmailAddress)
+            .setBody(String.format(RELOCK_UNKNOWN_ID_FAILURE_EMAIL_TEMPLATE, oldUnlockRevisionId))
+            .setSubject("Error re-locking domain")
+            .setRecipients(ImmutableSet.of(alertRecipientAddress))
+            .build());
+  }
+
+  private ImmutableSet<InternetAddress> getEmailRecipients(String registrarId) {
+    Registrar registrar =
+        Registrar.loadByClientIdCached(registrarId)
+            .orElseThrow(
+                () ->
+                    new IllegalStateException(String.format("Unknown registrar %s", registrarId)));
+
+    ImmutableSet<String> registryLockEmailAddresses =
+        registrar.getContacts().stream()
+            .filter(RegistrarContact::isRegistryLockAllowed)
+            .map(RegistrarContact::getRegistryLockEmailAddress)
+            .filter(Optional::isPresent)
+            .map(Optional::get)
+            .collect(toImmutableSet());
+
+    ImmutableSet.Builder<InternetAddress> builder = new ImmutableSet.Builder<>();
+    // can't use streams due to the 'throws' in the InternetAddress constructor
+    for (String registryLockEmailAddress : registryLockEmailAddresses) {
+      try {
+        builder.add(new InternetAddress(registryLockEmailAddress));
+      } catch (AddressException e) {
+        // This shouldn't stop any other emails going out, so swallow it
+        logger.atWarning().log("Invalid email address %s", registryLockEmailAddress);
+      }
+    }
+    return builder.build();
+  }
 }

core/src/main/java/google/registry/model/contact/ContactHistory.java

@@ -19,8 +19,13 @@ import com.googlecode.objectify.annotation.EntitySubclass;
 import google.registry.model.EppResource;
 import google.registry.model.reporting.HistoryEntry;
 import google.registry.persistence.VKey;
+import javax.persistence.Access;
+import javax.persistence.AccessType;
 import javax.persistence.Column;
 import javax.persistence.Entity;
+import javax.persistence.GeneratedValue;
+import javax.persistence.GenerationType;
+import javax.persistence.Id;
 
 /**
  * A persisted history entry representing an EPP modification to a contact.
@@ -38,6 +43,7 @@ import javax.persistence.Entity;
       @javax.persistence.Index(columnList = "historyModificationTime")
     })
 @EntitySubclass
+@Access(AccessType.FIELD)
 public class ContactHistory extends HistoryEntry {
   // Store ContactBase instead of ContactResource so we don't pick up its @Id
   ContactBase contactBase;
@@ -45,6 +51,15 @@ public class ContactHistory extends HistoryEntry {
   @Column(nullable = false)
   VKey<ContactResource> contactRepoId;
 
+  @Id
+  @GeneratedValue(strategy = GenerationType.SEQUENCE, generator = "HistorySequenceGenerator")
+  @Column(name = "historyRevisionId")
+  @Access(AccessType.PROPERTY)
+  @Override
+  public long getId() {
+    return super.getId();
+  }
+
   /** The state of the {@link ContactBase} object at this point in time. */
   public ContactBase getContactBase() {
     return contactBase;

core/src/main/java/google/registry/model/domain/DomainBase.java

@@ -14,7 +14,6 @@
 
 package google.registry.model.domain;
 
-
 import com.googlecode.objectify.Key;
 import google.registry.model.EppResource;
 import google.registry.model.EppResource.ForeignKeyedEppResource;
@@ -82,12 +81,26 @@ public class DomainBase extends DomainContent
     return super.nsHosts;
   }
 
+  /**
+   * Returns the set of {@link GracePeriod} associated with the domain.
+   *
+   * <p>This is the getter method specific for Hibernate to access the field so it is set to
+   * private. The caller can use the public {@link #getGracePeriods()} to get the grace periods.
+   *
+   * <p>Note that we need to set `insertable = false, updatable = false` for @JoinColumn, otherwise
+   * Hibernate would try to set the foreign key to null(through an UPDATE TABLE sql) instead of
+   * deleting the whole entry from the table when the {@link GracePeriod} is removed from the set.
+   */
   @Access(AccessType.PROPERTY)
   @OneToMany(
       cascade = {CascadeType.ALL},
       fetch = FetchType.EAGER,
       orphanRemoval = true)
-  @JoinColumn(name = "domainRepoId", referencedColumnName = "repoId")
+  @JoinColumn(
+      name = "domainRepoId",
+      referencedColumnName = "repoId",
+      insertable = false,
+      updatable = false)
   @SuppressWarnings("UnusedMethod")
   private Set<GracePeriod> getInternalGracePeriods() {
     return gracePeriods;

core/src/main/java/google/registry/model/domain/DomainContent.java

@@ -280,12 +280,10 @@ public class DomainContent extends EppResource
     // object will have a null hashcode so that it can get a recalculated hashcode
     // when its hashCode() is invoked.
     // TODO(b/162739503): Remove this after fully migrating to Cloud SQL.
-    if (gracePeriods != null) {
-      gracePeriods =
-          gracePeriods.stream()
-              .map(gracePeriod -> gracePeriod.cloneWithDomainRepoId(getRepoId()))
-              .collect(toImmutableSet());
-    }
+    gracePeriods =
+        nullToEmptyImmutableCopy(gracePeriods).stream()
+            .map(gracePeriod -> gracePeriod.cloneWithDomainRepoId(getRepoId()))
+            .collect(toImmutableSet());
   }
 
   @PostLoad
@@ -698,7 +696,13 @@ public class DomainContent extends EppResource
       }
       checkArgumentNotNull(instance.getRegistrant(), "Missing registrant");
       instance.tld = getTldFromDomainName(instance.fullyQualifiedDomainName);
-      return super.build();
+
+      T newDomain = super.build();
+      // Hibernate throws exception if gracePeriods is null because we enabled all cascadable
+      // operations and orphan removal.
+      newDomain.gracePeriods =
+          newDomain.gracePeriods == null ? ImmutableSet.of() : newDomain.gracePeriods;
+      return newDomain;
     }
 
     public B setDomainName(String domainName) {

core/src/main/java/google/registry/model/domain/DomainHistory.java

@@ -14,20 +14,32 @@
 
 package google.registry.model.domain;
 
+import static google.registry.util.CollectionUtils.nullToEmptyImmutableCopy;
+
 import com.googlecode.objectify.Key;
 import com.googlecode.objectify.annotation.EntitySubclass;
+import com.googlecode.objectify.annotation.Ignore;
 import google.registry.model.EppResource;
-import google.registry.model.contact.ContactResource;
+import google.registry.model.ImmutableObject;
+import google.registry.model.domain.DomainHistory.DomainHistoryId;
 import google.registry.model.host.HostResource;
 import google.registry.model.reporting.HistoryEntry;
 import google.registry.persistence.VKey;
+import java.io.Serializable;
 import java.util.Set;
 import javax.persistence.Access;
 import javax.persistence.AccessType;
 import javax.persistence.Column;
 import javax.persistence.ElementCollection;
 import javax.persistence.Entity;
+import javax.persistence.GeneratedValue;
+import javax.persistence.GenerationType;
+import javax.persistence.Id;
+import javax.persistence.IdClass;
+import javax.persistence.Index;
 import javax.persistence.JoinTable;
+import javax.persistence.PostLoad;
+import javax.persistence.Table;
 
 /**
  * A persisted history entry representing an EPP modification to a domain.
@@ -37,27 +49,43 @@ import javax.persistence.JoinTable;
  * the foreign-keyed fields in that class can refer to this object.
  */
 @Entity
-@javax.persistence.Table(
+@Table(
     indexes = {
-      @javax.persistence.Index(columnList = "creationTime"),
-      @javax.persistence.Index(columnList = "historyRegistrarId"),
-      @javax.persistence.Index(columnList = "historyType"),
-      @javax.persistence.Index(columnList = "historyModificationTime")
+      @Index(columnList = "creationTime"),
+      @Index(columnList = "historyRegistrarId"),
+      @Index(columnList = "historyType"),
+      @Index(columnList = "historyModificationTime")
     })
 @EntitySubclass
+@Access(AccessType.FIELD)
+@IdClass(DomainHistoryId.class)
 public class DomainHistory extends HistoryEntry {
   // Store DomainContent instead of DomainBase so we don't pick up its @Id
   DomainContent domainContent;
 
-  @Column(nullable = false)
-  VKey<DomainBase> domainRepoId;
+  @Id String domainRepoId;
 
+  // We could have reused domainContent.nsHosts here, but Hibernate throws a weird exception after
+  // we change to use a composite primary key.
+  // TODO(b/166776754): Investigate if we can reuse domainContent.nsHosts for storing host keys.
+  @Ignore
   @ElementCollection
   @JoinTable(name = "DomainHistoryHost")
-  @Access(AccessType.PROPERTY)
   @Column(name = "host_repo_id")
+  Set<VKey<HostResource>> nsHosts;
+
+  @Id
+  @GeneratedValue(strategy = GenerationType.SEQUENCE, generator = "HistorySequenceGenerator")
+  @Column(name = "historyRevisionId")
+  @Access(AccessType.PROPERTY)
+  @Override
+  public long getId() {
+    return super.getId();
+  }
+
+  /** Returns keys to the {@link HostResource} that are the nameservers for the domain. */
   public Set<VKey<HostResource>> getNsHosts() {
-    return domainContent.nsHosts;
+    return nsHosts;
   }
 
   /** The state of the {@link DomainContent} object at this point in time. */
@@ -65,16 +93,51 @@ public class DomainHistory extends HistoryEntry {
     return domainContent;
   }
 
-  /** The key to the {@link ContactResource} this is based off of. */
+  /** The key to the {@link DomainBase} this is based off of. */
   public VKey<DomainBase> getDomainRepoId() {
-    return domainRepoId;
+    return VKey.create(DomainBase.class, domainRepoId, Key.create(DomainBase.class, domainRepoId));
   }
 
-  // Hibernate needs this in order to populate nsHosts but no one else should ever use it
-  @SuppressWarnings("UnusedMethod")
-  private void setNsHosts(Set<VKey<HostResource>> nsHosts) {
+  public VKey<DomainHistory> createVKey() {
+    return VKey.createSql(DomainHistory.class, new DomainHistoryId(domainRepoId, getId()));
+  }
+
+  @PostLoad
+  void postLoad() {
     if (domainContent != null) {
-      domainContent.nsHosts = nsHosts;
+      domainContent.nsHosts = nullToEmptyImmutableCopy(nsHosts);
+    }
+  }
+
+  /** Class to represent the composite primary key of {@link DomainHistory} entity. */
+  static class DomainHistoryId extends ImmutableObject implements Serializable {
+
+    private String domainRepoId;
+
+    private Long id;
+
+    /** Hibernate requires this default constructor. */
+    private DomainHistoryId() {}
+
+    DomainHistoryId(String domainRepoId, long id) {
+      this.domainRepoId = domainRepoId;
+      this.id = id;
+    }
+
+    String getDomainRepoId() {
+      return domainRepoId;
+    }
+
+    void setDomainRepoId(String domainRepoId) {
+      this.domainRepoId = domainRepoId;
+    }
+
+    long getId() {
+      return id;
+    }
+
+    void setId(long id) {
+      this.id = id;
     }
   }
 
@@ -93,12 +156,15 @@ public class DomainHistory extends HistoryEntry {
 
     public Builder setDomainContent(DomainContent domainContent) {
       getInstance().domainContent = domainContent;
+      if (domainContent != null) {
+        getInstance().nsHosts = nullToEmptyImmutableCopy(domainContent.nsHosts);
+      }
       return this;
     }
 
-    public Builder setDomainRepoId(VKey<DomainBase> domainRepoId) {
+    public Builder setDomainRepoId(String domainRepoId) {
       getInstance().domainRepoId = domainRepoId;
-      domainRepoId.maybeGetOfyKey().ifPresent(parent -> getInstance().parent = parent);
+      getInstance().parent = Key.create(DomainBase.class, domainRepoId);
       return this;
     }
 
@@ -106,8 +172,7 @@ public class DomainHistory extends HistoryEntry {
     @Override
     public Builder setParent(Key<? extends EppResource> parent) {
       super.setParent(parent);
-      getInstance().domainRepoId =
-          VKey.create(DomainBase.class, parent.getName(), (Key<DomainBase>) parent);
+      getInstance().domainRepoId = parent.getName();
       return this;
     }
   }

core/src/main/java/google/registry/model/host/HostHistory.java

@@ -19,8 +19,13 @@ import com.googlecode.objectify.annotation.EntitySubclass;
 import google.registry.model.EppResource;
 import google.registry.model.reporting.HistoryEntry;
 import google.registry.persistence.VKey;
+import javax.persistence.Access;
+import javax.persistence.AccessType;
 import javax.persistence.Column;
 import javax.persistence.Entity;
+import javax.persistence.GeneratedValue;
+import javax.persistence.GenerationType;
+import javax.persistence.Id;
 
 /**
  * A persisted history entry representing an EPP modification to a host.
@@ -39,6 +44,7 @@ import javax.persistence.Entity;
       @javax.persistence.Index(columnList = "historyModificationTime")
     })
 @EntitySubclass
+@Access(AccessType.FIELD)
 public class HostHistory extends HistoryEntry {
 
   // Store HostBase instead of HostResource so we don't pick up its @Id
@@ -47,6 +53,15 @@ public class HostHistory extends HistoryEntry {
   @Column(nullable = false)
   VKey<HostResource> hostRepoId;
 
+  @Id
+  @GeneratedValue(strategy = GenerationType.SEQUENCE, generator = "HistorySequenceGenerator")
+  @Column(name = "historyRevisionId")
+  @Access(AccessType.PROPERTY)
+  @Override
+  public long getId() {
+    return super.getId();
+  }
+
   /** The state of the {@link HostBase} object at this point in time. */
   public HostBase getHostBase() {
     return hostBase;

core/src/main/java/google/registry/model/reporting/HistoryEntry.java

@@ -42,15 +42,14 @@ import google.registry.persistence.VKey;
 import google.registry.persistence.WithStringVKey;
 import java.util.Set;
 import javax.annotation.Nullable;
+import javax.persistence.Access;
+import javax.persistence.AccessType;
 import javax.persistence.AttributeOverride;
 import javax.persistence.AttributeOverrides;
 import javax.persistence.Column;
 import javax.persistence.EnumType;
 import javax.persistence.Enumerated;
-import javax.persistence.GeneratedValue;
-import javax.persistence.GenerationType;
 import javax.persistence.MappedSuperclass;
-import javax.persistence.SequenceGenerator;
 import javax.persistence.Transient;
 import org.joda.time.DateTime;
 
@@ -59,6 +58,7 @@ import org.joda.time.DateTime;
 @Entity
 @MappedSuperclass
 @WithStringVKey // TODO(b/162229294): This should be resolved during the course of that bug
+@Access(AccessType.FIELD)
 public class HistoryEntry extends ImmutableObject implements Buildable {
 
   /** Represents the type of history entry. */
@@ -110,17 +110,13 @@ public class HistoryEntry extends ImmutableObject implements Buildable {
     SYNTHETIC
   }
 
-  /** The autogenerated id of this event. */
-  @GeneratedValue(strategy = GenerationType.SEQUENCE, generator = "HistorySequenceGenerator")
-  @SequenceGenerator(
-      name = "HistorySequenceGenerator",
-      sequenceName = "history_id_sequence",
-      allocationSize = 1)
-  @Id
-  @javax.persistence.Id
-  @Column(name = "historyRevisionId")
-  @VisibleForTesting
-  public Long id;
+  /**
+   * The autogenerated id of this event. Note that, this field is marked as {@link Transient} in the
+   * SQL schema, this is because the child class of {@link HistoryEntry}, e.g. {@link
+   * DomainHistory}, uses a composite primary key which the id is part of, and Hibernate requires
+   * that all the {@link javax.persistence.Id} fields must be put in the exact same class.
+   */
+  @Id @Transient @VisibleForTesting public Long id;
 
   /** The resource this event mutated. */
   @Parent @Transient protected Key<? extends EppResource> parent;
@@ -196,8 +192,17 @@ public class HistoryEntry extends ImmutableObject implements Buildable {
   @Transient // domain-specific
   Set<DomainTransactionRecord> domainTransactionRecords;
 
-  public Long getId() {
-    return id;
+  public long getId() {
+    // For some reason, Hibernate throws NPE during some initialization phase if we don't deal with
+    // the null case. Setting the id to 0L when it is null should be fine because 0L for primitive
+    // type is considered as null for wrapper class in the Hibernate context.
+    return id == null ? 0L : id;
+  }
+
+  // This method is required by Hibernate.
+  @SuppressWarnings("UnusedMethod")
+  private void setId(long id) {
+    this.id = id;
   }
 
   public Key<? extends EppResource> getParent() {
@@ -279,11 +284,7 @@ public class HistoryEntry extends ImmutableObject implements Buildable {
     // can't use a switch statement since we're calling getKind()
     if (parentKind.equals(getKind(DomainBase.class))) {
       resultEntity =
-          new DomainHistory.Builder()
-              .copyFrom(this)
-              .setDomainRepoId(
-                  VKey.create(DomainBase.class, parent.getName(), (Key<DomainBase>) parent))
-              .build();
+          new DomainHistory.Builder().copyFrom(this).setDomainRepoId(parent.getName()).build();
     } else if (parentKind.equals(getKind(HostResource.class))) {
       resultEntity =
           new HostHistory.Builder()
@@ -349,6 +350,7 @@ public class HistoryEntry extends ImmutableObject implements Buildable {
       return thisCastToDerived();
     }
 
+    // Until we move completely to SQL, override this in subclasses (e.g. HostHistory) to set VKeys
     public B setParent(Key<? extends EppResource> parent) {
       getInstance().parent = parent;
       return thisCastToDerived();

core/src/main/java/google/registry/model/tmch/ClaimsListDao.java

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package google.registry.schema.tmch;
+package google.registry.model.tmch;
 
 import static google.registry.config.RegistryConfig.getDomainLabelListCacheDuration;
 import static google.registry.model.CacheUtils.tryMemoizeWithExpiration;
@@ -24,28 +24,28 @@ import google.registry.util.NonFinalForTesting;
 import java.util.Optional;
 import javax.persistence.EntityManager;
 
-/** Data access object for {@link ClaimsList}. */
+/** Data access object for {@link ClaimsListShard}. */
 public class ClaimsListDao {
 
   private static final FluentLogger logger = FluentLogger.forEnclosingClass();
 
   /** In-memory cache for claims list. */
   @NonFinalForTesting
-  private static Supplier<Optional<ClaimsList>> cacheClaimsList =
+  private static Supplier<Optional<ClaimsListShard>> cacheClaimsList =
       tryMemoizeWithExpiration(getDomainLabelListCacheDuration(), ClaimsListDao::getLatestRevision);
 
-  private static void save(ClaimsList claimsList) {
+  private static void save(ClaimsListShard claimsList) {
     jpaTm().transact(() -> jpaTm().getEntityManager().persist(claimsList));
   }
 
   /**
-   * Try to save the given {@link ClaimsList} into Cloud SQL. If the save fails, the error will be
-   * logged but no exception will be thrown.
+   * Try to save the given {@link ClaimsListShard} into Cloud SQL. If the save fails, the error will
+   * be logged but no exception will be thrown.
    *
    * <p>This method is used during the dual-write phase of database migration as Datastore is still
    * the authoritative database.
    */
-  public static void trySave(ClaimsList claimsList) {
+  static void trySave(ClaimsListShard claimsList) {
     try {
       ClaimsListDao.save(claimsList);
       logger.atInfo().log(
@@ -57,12 +57,12 @@ public class ClaimsListDao {
   }
 
   /**
-   * Returns the most recent revision of the {@link ClaimsList} in Cloud SQL, if it exists.
+   * Returns the most recent revision of the {@link ClaimsListShard} in Cloud SQL, if it exists.
    * TODO(shicong): Change this method to package level access after dual-read phase.
    * ClaimsListShard uses this method to retrieve claims list in Cloud SQL for the comparison, and
    * ClaimsListShard is not in this package.
    */
-  public static Optional<ClaimsList> getLatestRevision() {
+  public static Optional<ClaimsListShard> getLatestRevision() {
     return jpaTm()
         .transact(
             () -> {
@@ -73,15 +73,15 @@ public class ClaimsListDao {
               return em.createQuery(
                       "FROM ClaimsList cl LEFT JOIN FETCH cl.labelsToKeys WHERE cl.revisionId ="
                           + " :revisionId",
-                      ClaimsList.class)
+                      ClaimsListShard.class)
                   .setParameter("revisionId", revisionId)
                   .getResultStream()
                   .findFirst();
             });
   }
 
-  /** Returns the most recent revision of the {@link ClaimsList}, from cache. */
-  public static Optional<ClaimsList> getLatestRevisionCached() {
+  /** Returns the most recent revision of the {@link ClaimsListShard}, from cache. */
+  public static Optional<ClaimsListShard> getLatestRevisionCached() {
     return cacheClaimsList.get();
   }
 

core/src/main/java/google/registry/model/tmch/ClaimsListShard.java

@@ -40,6 +40,7 @@ import com.googlecode.objectify.annotation.Id;
 import com.googlecode.objectify.annotation.Ignore;
 import com.googlecode.objectify.annotation.OnSave;
 import com.googlecode.objectify.annotation.Parent;
+import google.registry.model.CreateAutoTimestamp;
 import google.registry.model.ImmutableObject;
 import google.registry.model.annotations.NotBackedUp;
 import google.registry.model.annotations.NotBackedUp.Reason;
@@ -47,8 +48,6 @@ import google.registry.model.annotations.VirtualEntity;
 import google.registry.model.common.CrossTldSingleton;
 import google.registry.schema.replay.DatastoreEntity;
 import google.registry.schema.replay.SqlEntity;
-import google.registry.schema.tmch.ClaimsList;
-import google.registry.schema.tmch.ClaimsListDao;
 import google.registry.util.CollectionUtils;
 import google.registry.util.Concurrent;
 import google.registry.util.Retrier;
@@ -59,6 +58,15 @@ import java.util.Map;
 import java.util.Optional;
 import java.util.concurrent.Callable;
 import javax.annotation.Nullable;
+import javax.persistence.CollectionTable;
+import javax.persistence.Column;
+import javax.persistence.ElementCollection;
+import javax.persistence.GeneratedValue;
+import javax.persistence.GenerationType;
+import javax.persistence.JoinColumn;
+import javax.persistence.MapKeyColumn;
+import javax.persistence.Table;
+import javax.persistence.Transient;
 import org.joda.time.DateTime;
 
 /**
@@ -74,10 +82,21 @@ import org.joda.time.DateTime;
  * 10MB per transaction limit.
  *
  * <p>Therefore, it is never OK to save an instance of this class directly to Datastore. Instead you
- * must use the {@link #save} method to do it for you.
+ * must use the {@link #saveToDatastore} method to do it for you.
+ *
+ * <p>Note that the primary key of this entity is {@link #revisionId}, which is auto-generated by
+ * the database. So, if a retry of insertion happens after the previous attempt unexpectedly
+ * succeeds, we will end up with having two exact same claims list with only different {@link
+ * #revisionId}. However, this is not an actual problem because we only use the claims list with
+ * highest {@link #revisionId}.
+ *
+ * <p>TODO(b/162007765): Rename the class to ClaimsList and remove Datastore related fields and
+ * methods.
  */
 @Entity
 @NotBackedUp(reason = Reason.EXTERNALLY_SOURCED)
+@javax.persistence.Entity(name = "ClaimsList")
+@Table
 public class ClaimsListShard extends ImmutableObject implements DatastoreEntity {
 
   private static final FluentLogger logger = FluentLogger.forEnclosingClass();
@@ -85,22 +104,44 @@ public class ClaimsListShard extends ImmutableObject implements DatastoreEntity
   /** The number of claims list entries to store per shard. */
   private static final int SHARD_SIZE = 10000;
 
-  @Id
-  long id;
+  @Transient @Id long id;
 
-  @Parent
-  Key<ClaimsListRevision> parent;
+  @Transient @Parent Key<ClaimsListRevision> parent;
 
-  /** When the claims list was last updated. */
+  @Ignore
+  @javax.persistence.Id
+  @GeneratedValue(strategy = GenerationType.IDENTITY)
+  Long revisionId;
+
+  @Ignore
+  @Column(nullable = false)
+  CreateAutoTimestamp creationTimestamp = CreateAutoTimestamp.create(null);
+
+  /**
+   * When the claims list was last updated.
+   *
+   * <p>Note that the value of this field is parsed from the claims list file(See this <a
+   * href="https://tools.ietf.org/html/draft-lozano-tmch-func-spec-08#section-6.1">RFC</>), it is
+   * the DNL List creation datetime from the rfc. Since this field has been used by Datastore, we
+   * cannot change its name until we finish the migration.
+   *
+   * <p>TODO(b/166784536): Rename this field to tmdbGenerationTime.
+   */
+  @Column(name = "tmdb_generation_time", nullable = false)
   DateTime creationTime;
 
   /** A map from labels to claims keys. */
   @EmbedMap
+  @ElementCollection
+  @CollectionTable(
+      name = "ClaimsEntry",
+      joinColumns = @JoinColumn(name = "revisionId", referencedColumnName = "revisionId"))
+  @MapKeyColumn(name = "domainLabel", nullable = false)
+  @Column(name = "claimKey", nullable = false)
   Map<String, String> labelsToKeys;
 
   /** Indicates that this is a shard rather than a "full" list. */
-  @Ignore
-  boolean isShard = false;
+  @Ignore @Transient boolean isShard = false;
 
   private static final Retrier LOADER_RETRIER = new Retrier(new SystemSleeper(), 2);
 
@@ -164,10 +205,10 @@ public class ClaimsListShard extends ImmutableObject implements DatastoreEntity
         return datastoreList;
       };
 
-  private static final void loadAndCompareCloudSqlList(ClaimsListShard datastoreList) {
-    Optional<ClaimsList> maybeCloudSqlList = ClaimsListDao.getLatestRevision();
+  private static void loadAndCompareCloudSqlList(ClaimsListShard datastoreList) {
+    Optional<ClaimsListShard> maybeCloudSqlList = ClaimsListDao.getLatestRevision();
     if (maybeCloudSqlList.isPresent()) {
-      ClaimsList cloudSqlList = maybeCloudSqlList.get();
+      ClaimsListShard cloudSqlList = maybeCloudSqlList.get();
       MapDifference<String, String> diff =
           Maps.difference(datastoreList.labelsToKeys, cloudSqlList.getLabelsToKeys());
       if (!diff.areEqual()) {
@@ -206,15 +247,34 @@ public class ClaimsListShard extends ImmutableObject implements DatastoreEntity
       memoizeWithShortExpiration(
           () -> LOADER_RETRIER.callWithRetry(LOADER_CALLABLE, IllegalStateException.class));
 
-  public DateTime getCreationTime() {
+  /** Returns the revision id of this claims list, or throws exception if it is null. */
+  public Long getRevisionId() {
+    checkState(
+        revisionId != null, "revisionId is null because it is not persisted in the database");
+    return revisionId;
+  }
+
+  /**
+   * Returns the time when the external TMDB service generated this revision of the claims list.
+   *
+   * @see <a href="https://tools.ietf.org/html/draft-lozano-tmch-func-spec-08#section-6.1">DNL List
+   *     creation datetime</a>
+   */
+  public DateTime getTmdbGenerationTime() {
     return creationTime;
   }
 
+  /** Returns the creation time of this claims list. */
+  public DateTime getCreationTimestamp() {
+    return creationTimestamp.getTimestamp();
+  }
+
   /** Returns the claim key for a given domain if there is one, empty otherwise. */
   public Optional<String> getClaimKey(String label) {
     return Optional.ofNullable(labelsToKeys.get(label));
   }
 
+  /** Returns an {@link Map} mapping domain label to its lookup key. */
   public ImmutableMap<String, String> getLabelsToKeys() {
     return ImmutableMap.copyOf(labelsToKeys);
   }
@@ -229,11 +289,12 @@ public class ClaimsListShard extends ImmutableObject implements DatastoreEntity
    * switching over to using them atomically, then deleting the old ones.
    */
   public void save() {
-    save(SHARD_SIZE);
+    saveToDatastore(SHARD_SIZE);
+    ClaimsListDao.trySave(this);
   }
 
   @VisibleForTesting
-  void save(int shardSize) {
+  void saveToDatastore(int shardSize) {
     // Figure out what the next versionId should be based on which ones already exist.
     final Key<ClaimsListRevision> oldRevision = getCurrentRevision();
     final Key<ClaimsListRevision> parentKey = ClaimsListRevision.createKey();
@@ -270,10 +331,11 @@ public class ClaimsListShard extends ImmutableObject implements DatastoreEntity
             });
   }
 
-  public static ClaimsListShard create(DateTime creationTime, Map<String, String> labelsToKeys) {
+  public static ClaimsListShard create(
+      DateTime tmdbGenerationTime, Map<String, String> labelsToKeys) {
     ClaimsListShard instance = new ClaimsListShard();
     instance.id = allocateId();
-    instance.creationTime = checkNotNull(creationTime);
+    instance.creationTime = checkNotNull(tmdbGenerationTime);
     instance.labelsToKeys = checkNotNull(labelsToKeys);
     return instance;
   }

core/src/main/java/google/registry/persistence/converter/DurationConverter.java

@@ -40,11 +40,17 @@ public class DurationConverter implements AttributeConverter<Duration, PGInterva
     if (duration == null) {
       return new PGInterval();
     }
+    // When the period is created from duration by calling duration.toPeriod(), only precise fields
+    // in the period type will be used. Thus, only the hour, minute, second and millisecond fields
+    // on the period will be used. The year, month, week and day fields will not be populated:
+    //   1. If the duration is small, less than one day, then this method will just set
+    //      hours/minutes/seconds correctly.
+    //   2. If the duration is larger than one day then all the remaining duration will
+    //      be stored in the largest available field, hours in this case.
+    // So, when we convert the period to a PGInterval instance, we set the days field by extracting
+    // it from period's hours field.
+    Period period = duration.toPeriod();
     PGInterval interval = new PGInterval();
-    Period period = new Period(duration);
-    // For some reason when the period is created from the duration, it does not set days, but
-    // instead just a total number of hours. Years and months are not created because those can
-    // differ in length of milliseconds.
     interval.setDays(period.getHours() / 24);
     interval.setHours(period.getHours() % 24);
     interval.setMinutes(period.getMinutes());

core/src/main/java/google/registry/reporting/icann/IcannHttpReporter.java

@@ -24,6 +24,8 @@ import com.google.api.client.http.GenericUrl;
 import com.google.api.client.http.HttpHeaders;
 import com.google.api.client.http.HttpRequest;
 import com.google.api.client.http.HttpResponse;
+import com.google.api.client.http.HttpResponseException;
+import com.google.api.client.http.HttpStatusCodes;
 import com.google.api.client.http.HttpTransport;
 import com.google.common.base.Ascii;
 import com.google.common.base.Splitter;
@@ -80,6 +82,7 @@ public class IcannHttpReporter {
     headers.setContentType(CSV_UTF_8.toString());
     request.setHeaders(headers);
     request.setFollowRedirects(false);
+    request.setThrowExceptionOnExecuteError(false);
 
     HttpResponse response = null;
     logger.atInfo().log(
@@ -87,6 +90,12 @@ public class IcannHttpReporter {
     boolean success = true;
     try {
       response = request.execute();
+      // Only responses with a 200 or 400 status have a body. For everything else, throw so that
+      // the caller catches it and prints the stack trace.
+      if (response.getStatusCode() != HttpStatusCodes.STATUS_CODE_OK
+          && response.getStatusCode() != HttpStatusCodes.STATUS_CODE_BAD_REQUEST) {
+        throw new HttpResponseException(response);
+      }
       byte[] content;
       try {
         content = ByteStreams.toByteArray(response.getContent());
@@ -94,13 +103,24 @@ public class IcannHttpReporter {
         response.getContent().close();
       }
       logger.atInfo().log(
-          "Received response code %d with content: %s\n\nResponse content in hex: %s",
+          "Received response code %d\n\n"
+              + "Response headers: %s\n\n"
+              + "Response content in UTF-8: %s\n\n"
+              + "Response content in HEX: %s",
           response.getStatusCode(),
+          response.getHeaders(),
           new String(content, UTF_8),
           BaseEncoding.base16().encode(content));
-      XjcIirdeaResult result = parseResult(content);
-      if (result.getCode().getValue() != 1000) {
+      // For reasons unclear at the moment, when we parse the response content using UTF-8 we get
+      // garbled texts. Since we know that an HTTP 200 response can only contain a result code of
+      // 1000 (i. e. success), there is no need to parse it.
+      if (response.getStatusCode() == HttpStatusCodes.STATUS_CODE_BAD_REQUEST) {
         success = false;
+        // To debug if there is a problem with our parsing, we wrap the response and print the stack
+        // trace of it. As far as we can tell, the stack trace for such an exception contains the
+        // response content that is decoded correctly using the expected charset.
+        new HttpResponseException(response).printStackTrace();
+        XjcIirdeaResult result = parseResult(content);
         logger.atWarning().log(
             "PUT rejected, status code %s:\n%s\n%s",
             result.getCode(), result.getMsg(), result.getDescription());

core/src/main/java/google/registry/schema/tmch/ClaimsList.java

@@ -1,116 +0,0 @@
-// Copyright 2019 The Nomulus Authors. All Rights Reserved.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package google.registry.schema.tmch;
-
-import static com.google.common.base.Preconditions.checkState;
-import static google.registry.util.DateTimeUtils.toJodaDateTime;
-import static google.registry.util.DateTimeUtils.toZonedDateTime;
-
-import com.google.common.collect.ImmutableList;
-import google.registry.model.CreateAutoTimestamp;
-import google.registry.model.ImmutableObject;
-import google.registry.schema.replay.DatastoreEntity;
-import google.registry.schema.replay.SqlEntity;
-import java.time.ZonedDateTime;
-import java.util.Map;
-import java.util.Optional;
-import javax.persistence.CollectionTable;
-import javax.persistence.Column;
-import javax.persistence.ElementCollection;
-import javax.persistence.Entity;
-import javax.persistence.GeneratedValue;
-import javax.persistence.GenerationType;
-import javax.persistence.Id;
-import javax.persistence.JoinColumn;
-import javax.persistence.MapKeyColumn;
-import javax.persistence.Table;
-import org.joda.time.DateTime;
-
-/**
- * A list of TMCH claims labels and their associated claims keys.
- *
- * <p>Note that the primary key of this entity is {@link #revisionId}, which is auto-generated by
- * the database. So, if a retry of insertion happens after the previous attempt unexpectedly
- * succeeds, we will end up with having two exact same claims list with only different {@link
- * #revisionId}. However, this is not an actual problem because we only use the claims list with
- * highest {@link #revisionId}.
- */
-@Entity
-@Table
-public class ClaimsList extends ImmutableObject implements SqlEntity {
-  @Id
-  @GeneratedValue(strategy = GenerationType.IDENTITY)
-  @Column
-  private Long revisionId;
-
-  @Column(nullable = false)
-  private CreateAutoTimestamp creationTimestamp = CreateAutoTimestamp.create(null);
-
-  @Column(nullable = false)
-  private ZonedDateTime tmdbGenerationTime;
-
-  @ElementCollection
-  @CollectionTable(
-      name = "ClaimsEntry",
-      joinColumns = @JoinColumn(name = "revisionId", referencedColumnName = "revisionId"))
-  @MapKeyColumn(name = "domainLabel", nullable = false)
-  @Column(name = "claimKey", nullable = false)
-  private Map<String, String> labelsToKeys;
-
-  private ClaimsList(ZonedDateTime tmdbGenerationTime, Map<String, String> labelsToKeys) {
-    this.tmdbGenerationTime = tmdbGenerationTime;
-    this.labelsToKeys = labelsToKeys;
-  }
-
-  // Hibernate requires this default constructor.
-  private ClaimsList() {}
-
-  /** Constructs a {@link ClaimsList} object. */
-  public static ClaimsList create(DateTime creationTimestamp, Map<String, String> labelsToKeys) {
-    return new ClaimsList(toZonedDateTime(creationTimestamp), labelsToKeys);
-  }
-
-  /** Returns the revision id of this claims list, or throws exception if it is null. */
-  public Long getRevisionId() {
-    checkState(
-        revisionId != null, "revisionId is null because it is not persisted in the database");
-    return revisionId;
-  }
-
-  /** Returns the TMDB generation time of this claims list. */
-  public DateTime getTmdbGenerationTime() {
-    return toJodaDateTime(tmdbGenerationTime);
-  }
-
-  /** Returns the creation time of this claims list. */
-  public DateTime getCreationTimestamp() {
-    return creationTimestamp.getTimestamp();
-  }
-
-  /** Returns an {@link Map} mapping domain label to its lookup key. */
-  public Map<String, String> getLabelsToKeys() {
-    return labelsToKeys;
-  }
-
-  /** Returns the claim key for a given domain if there is one, empty otherwise. */
-  public Optional<String> getClaimKey(String label) {
-    return Optional.ofNullable(labelsToKeys.get(label));
-  }
-
-  @Override
-  public ImmutableList<DatastoreEntity> toDatastoreEntities() {
-    return ImmutableList.of(); // ClaimsList is dual-written
-  }
-}

core/src/main/java/google/registry/tmch/ClaimsListParser.java

@@ -18,7 +18,7 @@ import static com.google.common.base.Preconditions.checkArgument;
 
 import com.google.common.base.Splitter;
 import com.google.common.collect.ImmutableMap;
-import google.registry.schema.tmch.ClaimsList;
+import google.registry.model.tmch.ClaimsListShard;
 import java.util.List;
 import org.joda.time.DateTime;
 
@@ -34,11 +34,11 @@ import org.joda.time.DateTime;
 public class ClaimsListParser {
 
   /**
-   * Converts the lines from the DNL CSV file into a {@link ClaimsList} object.
+   * Converts the lines from the DNL CSV file into a {@link ClaimsListShard} object.
    *
    * <p>Please note that this does <b>not</b> insert the object into Datastore.
    */
-  public static ClaimsList parse(List<String> lines) {
+  public static ClaimsListShard parse(List<String> lines) {
     ImmutableMap.Builder<String, String> builder = new ImmutableMap.Builder<>();
 
     // First line: <version>,<DNL List creation datetime>
@@ -74,6 +74,6 @@ public class ClaimsListParser {
       builder.put(label, lookupKey);
     }
 
-    return ClaimsList.create(creationTime, builder.build());
+    return ClaimsListShard.create(creationTime, builder.build());
   }
 }

core/src/main/java/google/registry/tmch/TmchDnlAction.java

@@ -21,8 +21,6 @@ import google.registry.keyring.api.KeyModule.Key;
 import google.registry.model.tmch.ClaimsListShard;
 import google.registry.request.Action;
 import google.registry.request.auth.Auth;
-import google.registry.schema.tmch.ClaimsList;
-import google.registry.schema.tmch.ClaimsListDao;
 import java.io.IOException;
 import java.security.SignatureException;
 import java.util.List;
@@ -56,14 +54,10 @@ public final class TmchDnlAction implements Runnable {
     } catch (SignatureException | IOException | PGPException e) {
       throw new RuntimeException(e);
     }
-    ClaimsList claims = ClaimsListParser.parse(lines);
-    ClaimsListShard claimsListShard =
-        ClaimsListShard.create(claims.getTmdbGenerationTime(), claims.getLabelsToKeys());
-    claimsListShard.save();
+    ClaimsListShard claims = ClaimsListParser.parse(lines);
+    claims.save();
     logger.atInfo().log(
         "Inserted %,d claims into Datastore, created at %s",
-        claimsListShard.size(), claimsListShard.getCreationTime());
-
-    ClaimsListDao.trySave(claims);
+        claims.size(), claims.getTmdbGenerationTime());
   }
 }

core/src/main/java/google/registry/tools/UploadClaimsListCommand.java

@@ -22,8 +22,6 @@ import com.beust.jcommander.Parameters;
 import com.google.common.base.Joiner;
 import com.google.common.io.Files;
 import google.registry.model.tmch.ClaimsListShard;
-import google.registry.schema.tmch.ClaimsList;
-import google.registry.schema.tmch.ClaimsListDao;
 import google.registry.tmch.ClaimsListParser;
 import java.io.File;
 import java.io.IOException;
@@ -39,7 +37,7 @@ final class UploadClaimsListCommand extends ConfirmingCommand implements Command
 
   private String claimsListFilename;
 
-  private ClaimsList claimsList;
+  private ClaimsListShard claimsList;
 
   @Override
   protected void init() throws IOException {
@@ -58,8 +56,7 @@ final class UploadClaimsListCommand extends ConfirmingCommand implements Command
 
   @Override
   public String execute() {
-    ClaimsListShard.create(claimsList.getTmdbGenerationTime(), claimsList.getLabelsToKeys()).save();
-    ClaimsListDao.trySave(claimsList);
+    claimsList.save();
     return String.format("Successfully uploaded claims list %s", claimsListFilename);
   }
 }

core/src/main/javascript/google/registry/ui/js/registrar/registry_lock.js

@@ -56,7 +56,8 @@ registry.registrar.RegistryLock.prototype.runAfterRender = function(objArgs) {
   } else {
     goog.soy.renderElement(
         goog.dom.getRequiredElement('locks-content'),
-        registry.soy.registrar.registrylock.lockNotAllowedOnRegistrar);
+        registry.soy.registrar.registrylock.lockNotAllowedOnRegistrar,
+        {supportEmail: objArgs.supportEmail});
   }
 };
 

core/src/main/resources/META-INF/orm.xml

@@ -10,6 +10,9 @@
       <basic name="amount" access="FIELD"/>
     </attributes>
   </embeddable>
+
+  <sequence-generator name="HistorySequenceGenerator" sequence-name="history_id_sequence" />
+
   <persistence-unit-metadata>
     <persistence-unit-defaults>
       <entity-listeners>

core/src/main/resources/META-INF/persistence.xml

@@ -53,8 +53,8 @@
     <class>google.registry.model.registry.label.PremiumList</class>
     <class>google.registry.model.reporting.Spec11ThreatMatch</class>
     <class>google.registry.persistence.transaction.TransactionEntity</class>
+    <class>google.registry.model.tmch.ClaimsListShard</class>
     <class>google.registry.schema.domain.RegistryLock</class>
-    <class>google.registry.schema.tmch.ClaimsList</class>
     <class>google.registry.schema.cursor.Cursor</class>
     <class>google.registry.schema.server.Lock</class>
     <class>google.registry.schema.tld.PremiumEntry</class>

core/src/main/resources/google/registry/ui/soy/registrar/RegistryLock.soy

@@ -163,5 +163,7 @@
 
 /** Content if the registrar is not allowed to use registry lock. */
 {template .lockNotAllowedOnRegistrar}
-  <h2>Registry Lock is coming soon; please stay tuned for updates.</h2>
+  {@param supportEmail: string}
+  <h2>Sorry, your registrar hasn't enrolled in registry lock yet. To do so, please
+    contact {$supportEmail}.</h2>
 {/template}

core/src/test/java/google/registry/batch/AsyncTaskEnqueuerTest.java

@@ -31,7 +31,7 @@ import static google.registry.testing.TestLogHandlerUtils.assertLogMessage;
 import static org.joda.time.Duration.standardDays;
 import static org.joda.time.Duration.standardHours;
 import static org.joda.time.Duration.standardSeconds;
-import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.Assert.assertThrows;
 import static org.mockito.Mockito.when;
 
 import com.google.common.collect.ImmutableSortedSet;
@@ -159,7 +159,7 @@ public class AsyncTaskEnqueuerTest {
                 .setRegistrarPocId("someone@example.com")
                 .setVerificationCode("hi")
                 .build());
-    asyncTaskEnqueuer.enqueueDomainRelock(lock);
+    asyncTaskEnqueuer.enqueueDomainRelock(lock.getRelockDuration().get(), lock.getRevisionId(), 0);
     assertTasksEnqueued(
         QUEUE_ASYNC_ACTIONS,
         new TaskMatcher()
@@ -169,6 +169,7 @@ public class AsyncTaskEnqueuerTest {
             .param(
                 RelockDomainAction.OLD_UNLOCK_REVISION_ID_PARAM,
                 String.valueOf(lock.getRevisionId()))
+            .param(RelockDomainAction.PREVIOUS_ATTEMPTS_PARAM, "0")
             .etaDelta(
                 standardHours(6).minus(standardSeconds(30)),
                 standardHours(6).plus(standardSeconds(30))));
@@ -188,9 +189,9 @@ public class AsyncTaskEnqueuerTest {
                 .setVerificationCode("hi")
                 .build());
     assertThat(
-            assertThrows(
-                IllegalArgumentException.class,
-                () -> asyncTaskEnqueuer.enqueueDomainRelock(lockWithoutDuration)))
+        assertThrows(
+            IllegalArgumentException.class,
+            () -> asyncTaskEnqueuer.enqueueDomainRelock(lockWithoutDuration)))
         .hasMessageThat()
         .isEqualTo(
             String.format(

core/src/test/java/google/registry/batch/RelockDomainActionTest.java

@@ -15,10 +15,12 @@
 package google.registry.batch;
 
 import static com.google.common.truth.Truth.assertThat;
+import static google.registry.batch.AsyncTaskEnqueuer.QUEUE_ASYNC_ACTIONS;
 import static google.registry.model.eppcommon.StatusValue.PENDING_DELETE;
 import static google.registry.model.eppcommon.StatusValue.PENDING_TRANSFER;
 import static google.registry.model.ofy.ObjectifyService.ofy;
 import static google.registry.testing.DatastoreHelper.createTlds;
+import static google.registry.testing.DatastoreHelper.deleteResource;
 import static google.registry.testing.DatastoreHelper.newDomainBase;
 import static google.registry.testing.DatastoreHelper.persistActiveHost;
 import static google.registry.testing.DatastoreHelper.persistDomainAsDeleted;
@@ -26,9 +28,16 @@ import static google.registry.testing.DatastoreHelper.persistResource;
 import static google.registry.testing.SqlHelper.getMostRecentVerifiedRegistryLockByRepoId;
 import static google.registry.testing.SqlHelper.getRegistryLockByVerificationCode;
 import static google.registry.testing.SqlHelper.saveRegistryLock;
+import static google.registry.testing.TaskQueueHelper.assertNoTasksEnqueued;
+import static google.registry.testing.TaskQueueHelper.assertTasksEnqueued;
 import static google.registry.tools.LockOrUnlockDomainCommand.REGISTRY_LOCK_STATUSES;
 import static javax.servlet.http.HttpServletResponse.SC_NO_CONTENT;
+import static javax.servlet.http.HttpServletResponse.SC_OK;
+import static org.joda.time.Duration.standardSeconds;
+import static org.mockito.Mockito.lenient;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
 
 import com.google.common.collect.ImmutableSet;
 import google.registry.model.domain.DomainBase;
@@ -38,17 +47,27 @@ import google.registry.testing.AppEngineExtension;
 import google.registry.testing.DeterministicStringGenerator;
 import google.registry.testing.FakeClock;
 import google.registry.testing.FakeResponse;
+import google.registry.testing.TaskQueueHelper.TaskMatcher;
 import google.registry.testing.UserInfo;
 import google.registry.tools.DomainLockUtils;
 import google.registry.util.AppEngineServiceUtils;
+import google.registry.util.EmailMessage;
+import google.registry.util.SendEmailService;
 import google.registry.util.StringGenerator.Alphabets;
 import java.util.Optional;
+import javax.mail.internet.InternetAddress;
+import org.joda.time.DateTime;
 import org.joda.time.Duration;
+import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
 import org.junit.jupiter.api.extension.RegisterExtension;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
 
 /** Unit tests for {@link RelockDomainAction}. */
+@ExtendWith(MockitoExtension.class)
 public class RelockDomainActionTest {
 
   private static final String DOMAIN_NAME = "example.tld";
@@ -56,7 +75,7 @@ public class RelockDomainActionTest {
   private static final String POC_ID = "marla.singer@example.com";
 
   private final FakeResponse response = new FakeResponse();
-  private final FakeClock clock = new FakeClock();
+  private final FakeClock clock = new FakeClock(DateTime.parse("2015-05-18T12:34:56Z"));
   private final DomainLockUtils domainLockUtils =
       new DomainLockUtils(
           new DeterministicStringGenerator(Alphabets.BASE_58),
@@ -68,15 +87,18 @@ public class RelockDomainActionTest {
   public final AppEngineExtension appEngineRule =
       AppEngineExtension.builder()
           .withDatastoreAndCloudSql()
+          .withTaskQueue()
           .withUserService(UserInfo.create(POC_ID, "12345"))
           .build();
 
   private DomainBase domain;
   private RegistryLock oldLock;
+  @Mock private SendEmailService sendEmailService;
+  private AsyncTaskEnqueuer asyncTaskEnqueuer;
   private RelockDomainAction action;
 
   @BeforeEach
-  void beforeEach() {
+  void beforeEach() throws Exception {
     createTlds("tld", "net");
     HostResource host = persistActiveHost("ns1.example.net");
     domain = persistResource(newDomainBase(DOMAIN_NAME, host));
@@ -88,9 +110,22 @@ public class RelockDomainActionTest {
         domainLockUtils.administrativelyApplyUnlock(
             DOMAIN_NAME, CLIENT_ID, false, Optional.empty());
     assertThat(reloadDomain(domain).getStatusValues()).containsNoneIn(REGISTRY_LOCK_STATUSES);
+
+    AppEngineServiceUtils appEngineServiceUtils = mock(AppEngineServiceUtils.class);
+    lenient()
+        .when(appEngineServiceUtils.getServiceHostname("backend"))
+        .thenReturn("backend.hostname.fake");
+
+    asyncTaskEnqueuer =
+        AsyncTaskEnqueuerTest.createForTesting(appEngineServiceUtils, clock, Duration.ZERO);
     action = createAction(oldLock.getRevisionId());
   }
 
+  @AfterEach
+  void afterEach() {
+    verifyNoMoreInteractions(sendEmailService);
+  }
+
   @Test
   void testLock() {
     action.run();
@@ -104,29 +139,36 @@ public class RelockDomainActionTest {
   }
 
   @Test
-  void testFailure_unknownCode() {
+  void testFailure_unknownCode() throws Exception {
     action = createAction(12128675309L);
     action.run();
     assertThat(response.getStatus()).isEqualTo(SC_NO_CONTENT);
-    assertThat(response.getPayload()).isEqualTo("Relock failed: Unknown revision ID 12128675309");
+    assertThat(response.getPayload()).isEqualTo("Re-lock failed: Unknown revision ID 12128675309");
+    assertTaskEnqueued(1, 12128675309L, Duration.standardMinutes(10)); // should retry, transient
   }
 
   @Test
-  void testFailure_pendingDelete() {
+  void testFailure_pendingDelete() throws Exception {
     persistResource(domain.asBuilder().setStatusValues(ImmutableSet.of(PENDING_DELETE)).build());
     action.run();
+    String expectedFailureMessage = "Domain example.tld has a pending delete.";
     assertThat(response.getStatus()).isEqualTo(SC_NO_CONTENT);
     assertThat(response.getPayload())
-        .isEqualTo(String.format("Relock failed: Domain %s has a pending delete", DOMAIN_NAME));
+        .isEqualTo(String.format("Re-lock failed: %s", expectedFailureMessage));
+    assertNonTransientFailureEmail(expectedFailureMessage);
+    assertNoTasksEnqueued(QUEUE_ASYNC_ACTIONS);
   }
 
   @Test
-  void testFailure_pendingTransfer() {
+  void testFailure_pendingTransfer() throws Exception {
     persistResource(domain.asBuilder().setStatusValues(ImmutableSet.of(PENDING_TRANSFER)).build());
     action.run();
+    String expectedFailureMessage = "Domain example.tld has a pending transfer.";
     assertThat(response.getStatus()).isEqualTo(SC_NO_CONTENT);
     assertThat(response.getPayload())
-        .isEqualTo(String.format("Relock failed: Domain %s has a pending transfer", DOMAIN_NAME));
+        .isEqualTo(String.format("Re-lock failed: %s", expectedFailureMessage));
+    assertNonTransientFailureEmail(expectedFailureMessage);
+    assertNoTasksEnqueued(QUEUE_ASYNC_ACTIONS);
   }
 
   @Test
@@ -135,29 +177,64 @@ public class RelockDomainActionTest {
     action.run();
     assertThat(response.getStatus()).isEqualTo(SC_NO_CONTENT);
     assertThat(response.getPayload())
-        .isEqualTo("Domain example.tld is already manually relocked, skipping automated relock.");
+        .isEqualTo("Domain example.tld is already manually re-locked, skipping automated re-lock.");
+    assertNoTasksEnqueued(QUEUE_ASYNC_ACTIONS);
   }
 
   @Test
-  void testFailure_domainDeleted() {
+  void testFailure_domainDeleted() throws Exception {
     persistDomainAsDeleted(domain, clock.nowUtc());
     action.run();
+    String expectedFailureMessage = "Domain example.tld has been deleted.";
     assertThat(response.getStatus()).isEqualTo(SC_NO_CONTENT);
     assertThat(response.getPayload())
-        .isEqualTo(String.format("Relock failed: Domain %s has been deleted", DOMAIN_NAME));
+        .isEqualTo(String.format("Re-lock failed: %s", expectedFailureMessage));
+    assertNonTransientFailureEmail(expectedFailureMessage);
+    assertNoTasksEnqueued(QUEUE_ASYNC_ACTIONS);
   }
 
   @Test
-  void testFailure_domainTransferred() {
+  void testFailure_domainTransferred() throws Exception {
     persistResource(domain.asBuilder().setPersistedCurrentSponsorClientId("NewRegistrar").build());
     action.run();
+    String expectedFailureMessage =
+        "Domain example.tld has been transferred from registrar TheRegistrar to registrar "
+            + "NewRegistrar since the unlock.";
     assertThat(response.getStatus()).isEqualTo(SC_NO_CONTENT);
     assertThat(response.getPayload())
-        .isEqualTo(
-            String.format(
-                "Relock failed: Domain %s has been transferred from registrar %s to registrar "
-                    + "%s since the unlock",
-                DOMAIN_NAME, CLIENT_ID, "NewRegistrar"));
+        .isEqualTo(String.format("Re-lock failed: %s", expectedFailureMessage));
+    assertNonTransientFailureEmail(expectedFailureMessage);
+    assertNoTasksEnqueued(QUEUE_ASYNC_ACTIONS);
+  }
+
+  @Test
+  public void testFailure_transientFailure_enqueuesTask() {
+    // Hard-delete the domain to simulate a DB failure
+    deleteResource(domain);
+    action.run();
+    assertThat(response.getStatus()).isEqualTo(SC_NO_CONTENT);
+    assertThat(response.getPayload()).isEqualTo("Re-lock failed: null");
+    assertTaskEnqueued(1);
+  }
+
+  @Test
+  void testFailure_sufficientTransientFailures_sendsEmail() throws Exception {
+    // Hard-delete the domain to simulate a DB failure
+    deleteResource(domain);
+    action = createAction(oldLock.getRevisionId(), RelockDomainAction.FAILURES_BEFORE_EMAIL);
+    action.run();
+    assertTaskEnqueued(RelockDomainAction.FAILURES_BEFORE_EMAIL + 1);
+    assertTransientFailureEmail();
+    assertThat(response.getStatus()).isEqualTo(SC_NO_CONTENT);
+    assertThat(response.getPayload()).isEqualTo("Re-lock failed: null");
+  }
+
+  @Test
+  void testSuccess_afterSufficientFailures_sendsEmail() throws Exception {
+    action = createAction(oldLock.getRevisionId(), RelockDomainAction.FAILURES_BEFORE_EMAIL + 1);
+    action.run();
+    assertThat(response.getStatus()).isEqualTo(SC_OK);
+    assertSuccessEmailSent();
   }
 
   @Test
@@ -170,14 +247,108 @@ public class RelockDomainActionTest {
     action.run();
     assertThat(response.getStatus()).isEqualTo(SC_NO_CONTENT);
     assertThat(response.getPayload())
-        .isEqualTo("Domain example.tld is already manually relocked, skipping automated relock.");
+        .isEqualTo("Domain example.tld is already manually re-locked, skipping automated re-lock.");
+    assertNoTasksEnqueued(QUEUE_ASYNC_ACTIONS);
+  }
+
+  @Test
+  void testFailure_slowsDown() throws Exception {
+    deleteResource(domain);
+    action = createAction(oldLock.getRevisionId(), RelockDomainAction.ATTEMPTS_BEFORE_SLOWDOWN);
+    action.run();
+    assertTaskEnqueued(
+        RelockDomainAction.ATTEMPTS_BEFORE_SLOWDOWN + 1,
+        oldLock.getRevisionId(),
+        Duration.standardHours(1));
+  }
+
+  private void assertSuccessEmailSent() throws Exception {
+    EmailMessage expectedEmail =
+        EmailMessage.newBuilder()
+            .setSubject("Successful re-lock of domain example.tld")
+            .setBody(
+                "The domain example.tld was successfully re-locked.\n\nPlease "
+                    + "contact support at support@example.com if you have any questions.")
+            .setRecipients(
+                ImmutableSet.of(new InternetAddress("Marla.Singer.RegistryLock@crr.com")))
+            .setFrom(new InternetAddress("outgoing@example.com"))
+            .build();
+    verify(sendEmailService).sendEmail(expectedEmail);
+  }
+
+  private void assertNonTransientFailureEmail(String exceptionMessage) throws Exception {
+    String expectedBody =
+        String.format(
+            "There was an error when automatically re-locking example.tld. Error message: %s\n\n"
+                + "Please contact support at support@example.com if you have any questions.",
+            exceptionMessage);
+    assertFailureEmailWithBody(
+        expectedBody, ImmutableSet.of(new InternetAddress("Marla.Singer.RegistryLock@crr.com")));
+  }
+
+  private void assertTransientFailureEmail() throws Exception {
+    String expectedBody =
+        "There was an unexpected error when automatically re-locking example.tld. We will continue "
+            + "retrying the lock for five hours. Please contact support at support@example.com if "
+            + "you have any questions";
+    assertFailureEmailWithBody(
+        expectedBody,
+        ImmutableSet.of(
+            new InternetAddress("Marla.Singer.RegistryLock@crr.com"),
+            new InternetAddress("alerts@example.com")));
+  }
+
+  private void assertFailureEmailWithBody(String body, ImmutableSet<InternetAddress> recipients)
+      throws Exception {
+    EmailMessage expectedEmail =
+        EmailMessage.newBuilder()
+            .setSubject("Error re-locking domain example.tld")
+            .setBody(body)
+            .setRecipients(recipients)
+            .setFrom(new InternetAddress("outgoing@example.com"))
+            .build();
+    verify(sendEmailService).sendEmail(expectedEmail);
+  }
+
+  private void assertTaskEnqueued(int numAttempts) {
+    assertTaskEnqueued(numAttempts, oldLock.getRevisionId(), Duration.standardMinutes(10));
+  }
+
+  private void assertTaskEnqueued(int numAttempts, long oldUnlockRevisionId, Duration duration) {
+    assertTasksEnqueued(
+        QUEUE_ASYNC_ACTIONS,
+        new TaskMatcher()
+            .url(RelockDomainAction.PATH)
+            .method("POST")
+            .header("Host", "backend.hostname.fake")
+            .param(
+                RelockDomainAction.OLD_UNLOCK_REVISION_ID_PARAM,
+                String.valueOf(oldUnlockRevisionId))
+            .param(RelockDomainAction.PREVIOUS_ATTEMPTS_PARAM, String.valueOf(numAttempts))
+            .etaDelta(duration.minus(standardSeconds(30)), duration.plus(standardSeconds(30))));
   }
 
   private DomainBase reloadDomain(DomainBase domain) {
     return ofy().load().entity(domain).now();
   }
 
-  private RelockDomainAction createAction(Long oldUnlockRevisionId) {
-    return new RelockDomainAction(oldUnlockRevisionId, domainLockUtils, response);
+  private RelockDomainAction createAction(Long oldUnlockRevisionId) throws Exception {
+    return createAction(oldUnlockRevisionId, 0);
+  }
+
+  private RelockDomainAction createAction(Long oldUnlockRevisionId, int previousAttempts)
+      throws Exception {
+    InternetAddress alertRecipientAddress = new InternetAddress("alerts@example.com");
+    InternetAddress gSuiteOutgoingAddress = new InternetAddress("outgoing@example.com");
+    return new RelockDomainAction(
+        oldUnlockRevisionId,
+        previousAttempts,
+        alertRecipientAddress,
+        gSuiteOutgoingAddress,
+        "support@example.com",
+        sendEmailService,
+        domainLockUtils,
+        response,
+        asyncTaskEnqueuer);
   }
 }

core/src/test/java/google/registry/model/domain/DomainBaseSqlTest.java

@@ -14,6 +14,7 @@
 
 package google.registry.model.domain;
 
+import static com.google.common.truth.Truth.assertThat;
 import static google.registry.model.ImmutableObjectSubject.assertAboutImmutableObjects;
 import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
 import static google.registry.testing.SqlHelper.assertThrowForeignKeyViolation;
@@ -21,6 +22,7 @@ import static google.registry.testing.SqlHelper.saveRegistrar;
 import static google.registry.util.DateTimeUtils.END_OF_TIME;
 import static google.registry.util.DateTimeUtils.START_OF_TIME;
 import static org.joda.time.DateTimeZone.UTC;
+import static org.junit.jupiter.api.Assertions.fail;
 
 import com.google.common.collect.ImmutableSet;
 import google.registry.model.contact.ContactResource;
@@ -37,7 +39,7 @@ import google.registry.persistence.transaction.JpaTestRules;
 import google.registry.persistence.transaction.JpaTestRules.JpaIntegrationWithCoverageExtension;
 import google.registry.testing.DatastoreEntityExtension;
 import google.registry.testing.FakeClock;
-import javax.persistence.EntityManager;
+import java.util.Arrays;
 import org.joda.time.DateTime;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Order;
@@ -119,77 +121,217 @@ public class DomainBaseSqlTest {
 
   @Test
   void testDomainBasePersistence() {
-    jpaTm()
-        .transact(
-            () -> {
-              // Persist the contacts.  Note that these need to be persisted before the domain
-              // otherwise we get a foreign key constraint error.  If we ever decide to defer the
-              // relevant foreign key checks to commit time, then the order would not matter.
-              jpaTm().saveNew(contact);
-              jpaTm().saveNew(contact2);
-
-              // Persist the domain.
-              jpaTm().saveNew(domain);
-
-              // Persist the host.  This does _not_ need to be persisted before the domain,
-              // because only the row in the join table (DomainHost) is subject to foreign key
-              // constraints, and Hibernate knows to insert it after domain and host.
-              jpaTm().saveNew(host);
-            });
+    persistDomain();
 
     jpaTm()
         .transact(
             () -> {
-              // Load the domain in its entirety.
-              EntityManager em = jpaTm().getEntityManager();
-              DomainBase result = em.find(DomainBase.class, "4-COM");
-
-              // Fix DS data, since we can't persist it yet.
-              result =
-                  result
-                      .asBuilder()
-                      .setDsData(
-                          ImmutableSet.of(
-                              DelegationSignerData.create(1, 2, 3, new byte[] {0, 1, 2})))
-                      .build();
-
-              // Fix the original creation timestamp (this gets initialized on first write)
-              DomainBase org = domain.asBuilder().setCreationTime(result.getCreationTime()).build();
-
-              // Note that the equality comparison forces a lazy load of all fields.
-              assertAboutImmutableObjects()
-                  .that(result)
-                  .isEqualExceptFields(org, "updateTimestamp");
+              DomainBase result = jpaTm().load(domain.createVKey());
+              assertEqualDomainExcept(result);
             });
   }
 
   @Test
   void testHostForeignKeyConstraints() {
     assertThrowForeignKeyViolation(
-        () -> {
-          jpaTm()
-              .transact(
-                  () -> {
-                    // Persist the domain without the associated host object.
-                    jpaTm().saveNew(contact);
-                    jpaTm().saveNew(contact2);
-                    jpaTm().saveNew(domain);
-                  });
-        });
+        () ->
+            jpaTm()
+                .transact(
+                    () -> {
+                      // Persist the domain without the associated host object.
+                      jpaTm().saveNew(contact);
+                      jpaTm().saveNew(contact2);
+                      jpaTm().saveNew(domain);
+                    }));
   }
 
   @Test
   void testContactForeignKeyConstraints() {
     assertThrowForeignKeyViolation(
-        () -> {
-          jpaTm()
-              .transact(
-                  () -> {
-                    // Persist the domain without the associated contact objects.
-                    jpaTm().saveNew(domain);
-                    jpaTm().saveNew(host);
-                  });
-        });
+        () ->
+            jpaTm()
+                .transact(
+                    () -> {
+                      // Persist the domain without the associated contact objects.
+                      jpaTm().saveNew(domain);
+                      jpaTm().saveNew(host);
+                    }));
+  }
+
+  @Test
+  void testResaveDomain_succeeds() {
+    persistDomain();
+    jpaTm()
+        .transact(
+            () -> {
+              DomainBase persisted = jpaTm().load(domain.createVKey());
+              jpaTm().saveNewOrUpdate(persisted.asBuilder().build());
+            });
+    jpaTm()
+        .transact(
+            () -> {
+              // Load the domain in its entirety.
+              DomainBase result = jpaTm().load(domain.createVKey());
+              assertEqualDomainExcept(result);
+            });
+  }
+
+  @Test
+  void testModifyGracePeriod_setEmptyCollectionSuccessfully() {
+    persistDomain();
+    jpaTm()
+        .transact(
+            () -> {
+              DomainBase persisted = jpaTm().load(domain.createVKey());
+              DomainBase modified =
+                  persisted.asBuilder().setGracePeriods(ImmutableSet.of()).build();
+              jpaTm().saveNewOrUpdate(modified);
+            });
+
+    jpaTm()
+        .transact(
+            () -> {
+              DomainBase persisted = jpaTm().load(domain.createVKey());
+              assertThat(persisted.getGracePeriods()).isEmpty();
+            });
+  }
+
+  @Test
+  void testModifyGracePeriod_setNullCollectionSuccessfully() {
+    persistDomain();
+    jpaTm()
+        .transact(
+            () -> {
+              DomainBase persisted = jpaTm().load(domain.createVKey());
+              DomainBase modified = persisted.asBuilder().setGracePeriods(null).build();
+              jpaTm().saveNewOrUpdate(modified);
+            });
+
+    jpaTm()
+        .transact(
+            () -> {
+              DomainBase persisted = jpaTm().load(domain.createVKey());
+              assertThat(persisted.getGracePeriods()).isEmpty();
+            });
+  }
+
+  @Test
+  void testModifyGracePeriod_addThenRemoveSuccessfully() {
+    persistDomain();
+    jpaTm()
+        .transact(
+            () -> {
+              DomainBase persisted = jpaTm().load(domain.createVKey());
+              DomainBase modified =
+                  persisted
+                      .asBuilder()
+                      .addGracePeriod(
+                          GracePeriod.create(
+                              GracePeriodStatus.RENEW, "4-COM", END_OF_TIME, "registrar1", null))
+                      .build();
+              jpaTm().saveNewOrUpdate(modified);
+            });
+
+    jpaTm()
+        .transact(
+            () -> {
+              DomainBase persisted = jpaTm().load(domain.createVKey());
+              assertThat(persisted.getGracePeriods().size()).isEqualTo(2);
+              persisted
+                  .getGracePeriods()
+                  .forEach(
+                      gracePeriod -> {
+                        assertThat(gracePeriod.id).isNotNull();
+                        if (gracePeriod.getType() == GracePeriodStatus.ADD) {
+                          assertAboutImmutableObjects()
+                              .that(gracePeriod)
+                              .isEqualExceptFields(
+                                  GracePeriod.create(
+                                      GracePeriodStatus.ADD,
+                                      "4-COM",
+                                      END_OF_TIME,
+                                      "registrar1",
+                                      null),
+                                  "id");
+                        } else if (gracePeriod.getType() == GracePeriodStatus.RENEW) {
+                          assertAboutImmutableObjects()
+                              .that(gracePeriod)
+                              .isEqualExceptFields(
+                                  GracePeriod.create(
+                                      GracePeriodStatus.RENEW,
+                                      "4-COM",
+                                      END_OF_TIME,
+                                      "registrar1",
+                                      null),
+                                  "id");
+                        } else {
+                          fail("Unexpected GracePeriod: " + gracePeriod);
+                        }
+                      });
+              assertEqualDomainExcept(persisted, "gracePeriods");
+            });
+
+    jpaTm()
+        .transact(
+            () -> {
+              DomainBase persisted = jpaTm().load(domain.createVKey());
+              DomainBase.Builder builder = persisted.asBuilder();
+              for (GracePeriod gracePeriod : persisted.getGracePeriods()) {
+                if (gracePeriod.getType() == GracePeriodStatus.RENEW) {
+                  builder.removeGracePeriod(gracePeriod);
+                }
+              }
+              jpaTm().saveNewOrUpdate(builder.build());
+            });
+
+    jpaTm()
+        .transact(
+            () -> {
+              DomainBase persisted = jpaTm().load(domain.createVKey());
+              assertEqualDomainExcept(persisted);
+            });
+  }
+
+  @Test
+  void testModifyGracePeriod_removeThenAddSuccessfully() {
+    persistDomain();
+    jpaTm()
+        .transact(
+            () -> {
+              DomainBase persisted = jpaTm().load(domain.createVKey());
+              DomainBase modified =
+                  persisted.asBuilder().setGracePeriods(ImmutableSet.of()).build();
+              jpaTm().saveNewOrUpdate(modified);
+            });
+
+    jpaTm()
+        .transact(
+            () -> {
+              DomainBase persisted = jpaTm().load(domain.createVKey());
+              assertThat(persisted.getGracePeriods()).isEmpty();
+              DomainBase modified =
+                  persisted
+                      .asBuilder()
+                      .addGracePeriod(
+                          GracePeriod.create(
+                              GracePeriodStatus.ADD, "4-COM", END_OF_TIME, "registrar1", null))
+                      .build();
+              jpaTm().saveNewOrUpdate(modified);
+            });
+
+    jpaTm()
+        .transact(
+            () -> {
+              DomainBase persisted = jpaTm().load(domain.createVKey());
+              assertThat(persisted.getGracePeriods().size()).isEqualTo(1);
+              assertAboutImmutableObjects()
+                  .that(persisted.getGracePeriods().iterator().next())
+                  .isEqualExceptFields(
+                      GracePeriod.create(
+                          GracePeriodStatus.ADD, "4-COM", END_OF_TIME, "registrar1", null),
+                      "id");
+              assertEqualDomainExcept(persisted, "gracePeriods");
+            });
   }
 
   @Test
@@ -232,4 +374,42 @@ public class DomainBaseSqlTest {
         .setPersistedCurrentSponsorClientId("registrar1")
         .build();
   }
+
+  private void persistDomain() {
+    jpaTm()
+        .transact(
+            () -> {
+              // Persist the contacts.  Note that these need to be persisted before the domain
+              // otherwise we get a foreign key constraint error.  If we ever decide to defer the
+              // relevant foreign key checks to commit time, then the order would not matter.
+              jpaTm().saveNew(contact);
+              jpaTm().saveNew(contact2);
+
+              // Persist the domain.
+              jpaTm().saveNew(domain);
+
+              // Persist the host.  This does _not_ need to be persisted before the domain,
+              // because only the row in the join table (DomainHost) is subject to foreign key
+              // constraints, and Hibernate knows to insert it after domain and host.
+              jpaTm().saveNew(host);
+            });
+  }
+
+  private void assertEqualDomainExcept(DomainBase thatDomain, String... excepts) {
+    // Fix DS data, since we can't persist it yet.
+    thatDomain =
+        thatDomain
+            .asBuilder()
+            .setDsData(ImmutableSet.of(DelegationSignerData.create(1, 2, 3, new byte[] {0, 1, 2})))
+            .build();
+
+    // Fix the original creation timestamp (this gets initialized on first write)
+    DomainBase org = domain.asBuilder().setCreationTime(thatDomain.getCreationTime()).build();
+
+    String[] moreExcepts = Arrays.copyOf(excepts, excepts.length + 1);
+    moreExcepts[moreExcepts.length - 1] = "updateTimestamp";
+
+    // Note that the equality comparison forces a lazy load of all fields.
+    assertAboutImmutableObjects().that(thatDomain).isEqualExceptFields(org, moreExcepts);
+  }
 }

core/src/test/java/google/registry/model/history/DomainHistoryTest.java

@@ -14,6 +14,7 @@
 
 package google.registry.model.history;
 
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.common.truth.Truth.assertThat;
 import static google.registry.model.ImmutableObjectSubject.assertAboutImmutableObjects;
 import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
@@ -71,11 +72,15 @@ public class DomainHistoryTest extends EntityTestCase {
     jpaTm()
         .transact(
             () -> {
-              DomainHistory fromDatabase =
-                  jpaTm().load(VKey.createSql(DomainHistory.class, domainHistory.getId()));
+              DomainHistory fromDatabase = jpaTm().load(domainHistory.createVKey());
               assertDomainHistoriesEqual(fromDatabase, domainHistory);
               assertThat(fromDatabase.getDomainRepoId().getSqlKey())
                   .isEqualTo(domainHistory.getDomainRepoId().getSqlKey());
+              assertThat(fromDatabase.getNsHosts())
+                  .containsExactlyElementsIn(
+                      domainHistory.getNsHosts().stream()
+                          .map(key -> VKey.createSql(HostResource.class, key.getSqlKey()))
+                          .collect(toImmutableSet()));
             });
   }
 
@@ -120,7 +125,7 @@ public class DomainHistoryTest extends EntityTestCase {
   static void assertDomainHistoriesEqual(DomainHistory one, DomainHistory two) {
     assertAboutImmutableObjects()
         .that(one)
-        .isEqualExceptFields(two, "domainContent", "domainRepoId", "parent");
+        .isEqualExceptFields(two, "domainContent", "domainRepoId", "parent", "nsHosts");
   }
 
   private DomainHistory createDomainHistory(DomainContent domain) {
@@ -134,7 +139,7 @@ public class DomainHistoryTest extends EntityTestCase {
         .setReason("reason")
         .setRequestedByRegistrar(true)
         .setDomainContent(domain)
-        .setDomainRepoId(domain.createVKey())
+        .setDomainRepoId(domain.getRepoId())
         .build();
   }
 }

core/src/test/java/google/registry/model/tmch/ClaimsListDaoTest.java

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package google.registry.schema.tmch;
+package google.registry.model.tmch;
 
 import static com.google.common.truth.Truth.assertThat;
 
@@ -40,20 +40,22 @@ public class ClaimsListDaoTest {
 
   @Test
   void trySave_insertsClaimsListSuccessfully() {
-    ClaimsList claimsList =
-        ClaimsList.create(fakeClock.nowUtc(), ImmutableMap.of("label1", "key1", "label2", "key2"));
+    ClaimsListShard claimsList =
+        ClaimsListShard.create(
+            fakeClock.nowUtc(), ImmutableMap.of("label1", "key1", "label2", "key2"));
     ClaimsListDao.trySave(claimsList);
-    ClaimsList insertedClaimsList = ClaimsListDao.getLatestRevision().get();
+    ClaimsListShard insertedClaimsList = ClaimsListDao.getLatestRevision().get();
     assertClaimsListEquals(claimsList, insertedClaimsList);
     assertThat(insertedClaimsList.getCreationTimestamp()).isEqualTo(fakeClock.nowUtc());
   }
 
   @Test
   void trySave_noExceptionThrownWhenSaveFail() {
-    ClaimsList claimsList =
-        ClaimsList.create(fakeClock.nowUtc(), ImmutableMap.of("label1", "key1", "label2", "key2"));
+    ClaimsListShard claimsList =
+        ClaimsListShard.create(
+            fakeClock.nowUtc(), ImmutableMap.of("label1", "key1", "label2", "key2"));
     ClaimsListDao.trySave(claimsList);
-    ClaimsList insertedClaimsList = ClaimsListDao.getLatestRevision().get();
+    ClaimsListShard insertedClaimsList = ClaimsListDao.getLatestRevision().get();
     assertClaimsListEquals(claimsList, insertedClaimsList);
     // Save ClaimsList with existing revisionId should fail because revisionId is the primary key.
     ClaimsListDao.trySave(insertedClaimsList);
@@ -61,9 +63,9 @@ public class ClaimsListDaoTest {
 
   @Test
   void trySave_claimsListWithNoEntries() {
-    ClaimsList claimsList = ClaimsList.create(fakeClock.nowUtc(), ImmutableMap.of());
+    ClaimsListShard claimsList = ClaimsListShard.create(fakeClock.nowUtc(), ImmutableMap.of());
     ClaimsListDao.trySave(claimsList);
-    ClaimsList insertedClaimsList = ClaimsListDao.getLatestRevision().get();
+    ClaimsListShard insertedClaimsList = ClaimsListDao.getLatestRevision().get();
     assertClaimsListEquals(claimsList, insertedClaimsList);
     assertThat(insertedClaimsList.getLabelsToKeys()).isEmpty();
   }
@@ -75,16 +77,18 @@ public class ClaimsListDaoTest {
 
   @Test
   void getCurrent_returnsLatestClaims() {
-    ClaimsList oldClaimsList =
-        ClaimsList.create(fakeClock.nowUtc(), ImmutableMap.of("label1", "key1", "label2", "key2"));
-    ClaimsList newClaimsList =
-        ClaimsList.create(fakeClock.nowUtc(), ImmutableMap.of("label3", "key3", "label4", "key4"));
+    ClaimsListShard oldClaimsList =
+        ClaimsListShard.create(
+            fakeClock.nowUtc(), ImmutableMap.of("label1", "key1", "label2", "key2"));
+    ClaimsListShard newClaimsList =
+        ClaimsListShard.create(
+            fakeClock.nowUtc(), ImmutableMap.of("label3", "key3", "label4", "key4"));
     ClaimsListDao.trySave(oldClaimsList);
     ClaimsListDao.trySave(newClaimsList);
     assertClaimsListEquals(newClaimsList, ClaimsListDao.getLatestRevision().get());
   }
 
-  private void assertClaimsListEquals(ClaimsList left, ClaimsList right) {
+  private void assertClaimsListEquals(ClaimsListShard left, ClaimsListShard right) {
     assertThat(left.getRevisionId()).isEqualTo(right.getRevisionId());
     assertThat(left.getTmdbGenerationTime()).isEqualTo(right.getTmdbGenerationTime());
     assertThat(left.getLabelsToKeys()).isEqualTo(right.getLabelsToKeys());

core/src/test/java/google/registry/model/tmch/ClaimsListShardTest.java

@@ -76,7 +76,7 @@ public class ClaimsListShardTest {
     DateTime now = DateTime.now(UTC);
     // Save it with sharding, and make sure that reloading it works.
     ClaimsListShard unsharded = ClaimsListShard.create(now, ImmutableMap.copyOf(labelsToKeys));
-    unsharded.save(shardSize);
+    unsharded.saveToDatastore(shardSize);
     assertThat(ClaimsListShard.get().labelsToKeys).isEqualTo(unsharded.labelsToKeys);
     List<ClaimsListShard> shards1 = ofy().load().type(ClaimsListShard.class).list();
     assertThat(shards1).hasSize(4);
@@ -90,7 +90,7 @@ public class ClaimsListShardTest {
       labelsToKeys.put(Integer.toString(i), Integer.toString(i));
     }
     unsharded = ClaimsListShard.create(now.plusDays(1), ImmutableMap.copyOf(labelsToKeys));
-    unsharded.save(shardSize);
+    unsharded.saveToDatastore(shardSize);
     ofy().clearSessionCache();
     assertThat(ClaimsListShard.get().labelsToKeys).hasSize(unsharded.labelsToKeys.size());
     assertThat(ClaimsListShard.get().labelsToKeys).isEqualTo(unsharded.labelsToKeys);

core/src/test/java/google/registry/persistence/converter/DurationConverterTest.java

@@ -51,22 +51,37 @@ public class DurationConverterTest {
             .plus(Duration.standardMinutes(30))
             .plus(Duration.standardSeconds(15))
             .plus(Duration.millis(7));
-    DurationTestEntity entity = new DurationTestEntity(testDuration);
-    jpaTm().transact(() -> jpaTm().getEntityManager().persist(entity));
-    DurationTestEntity persisted =
-        jpaTm().transact(() -> jpaTm().getEntityManager().find(DurationTestEntity.class, "id"));
-    assertThat(persisted.duration.getMillis()).isEqualTo(testDuration.getMillis());
+    assertPersistedEntityHasSameDuration(testDuration);
   }
 
   @Test
   void testRoundTripLargeNumberOfDays() {
     Duration testDuration =
         Duration.standardDays(10001).plus(Duration.standardHours(100)).plus(Duration.millis(790));
-    DurationTestEntity entity = new DurationTestEntity(testDuration);
-    jpaTm().transact(() -> jpaTm().getEntityManager().persist(entity));
+    assertPersistedEntityHasSameDuration(testDuration);
+  }
+
+  @Test
+  void testRoundTripLessThanOneDay() {
+    Duration testDuration =
+        Duration.standardHours(15)
+            .plus(Duration.standardMinutes(40))
+            .plus(Duration.standardSeconds(50));
+    assertPersistedEntityHasSameDuration(testDuration);
+  }
+
+  @Test
+  void testRoundTripExactOneDay() {
+    Duration testDuration = Duration.standardDays(1);
+    assertPersistedEntityHasSameDuration(testDuration);
+  }
+
+  private void assertPersistedEntityHasSameDuration(Duration duration) {
+    DurationTestEntity entity = new DurationTestEntity(duration);
+    jpaTm().transact(() -> jpaTm().saveNew(entity));
     DurationTestEntity persisted =
         jpaTm().transact(() -> jpaTm().getEntityManager().find(DurationTestEntity.class, "id"));
-    assertThat(persisted.duration.getMillis()).isEqualTo(testDuration.getMillis());
+    assertThat(persisted.duration.getMillis()).isEqualTo(duration.getMillis());
   }
 
   @Entity(name = "TestEntity") // Override entity name to avoid the nested class reference.

core/src/test/java/google/registry/persistence/transaction/JpaTransactionManagerRuleTest.java

@@ -20,7 +20,6 @@ import static org.junit.jupiter.api.Assertions.assertThrows;
 
 import google.registry.model.ImmutableObject;
 import google.registry.persistence.transaction.JpaTestRules.JpaUnitTestExtension;
-import google.registry.schema.tmch.ClaimsList;
 import java.util.List;
 import javax.persistence.Entity;
 import javax.persistence.Id;
@@ -36,9 +35,7 @@ public class JpaTransactionManagerRuleTest {
 
   @RegisterExtension
   public final JpaUnitTestExtension jpaExtension =
-      new JpaTestRules.Builder()
-          .withEntityClass(ClaimsList.class, TestEntity.class)
-          .buildUnitTestRule();
+      new JpaTestRules.Builder().withEntityClass(TestEntity.class).buildUnitTestRule();
 
   @Test
   void verifiesRuleWorks() {
@@ -58,7 +55,7 @@ public class JpaTransactionManagerRuleTest {
               List results =
                   jpaTm()
                       .getEntityManager()
-                      .createNativeQuery("SELECT * FROM \"ClaimsList\"")
+                      .createNativeQuery("SELECT * FROM \"TestEntity\"")
                       .getResultList();
               assertThat(results).isEmpty();
             });

core/src/test/java/google/registry/reporting/icann/IcannHttpReporterTest.java

@@ -21,6 +21,8 @@ import static google.registry.testing.DatastoreHelper.createTld;
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 
+import com.google.api.client.http.HttpResponseException;
+import com.google.api.client.http.HttpStatusCodes;
 import com.google.api.client.http.LowLevelHttpRequest;
 import com.google.api.client.http.LowLevelHttpResponse;
 import com.google.api.client.testing.http.MockHttpTransport;
@@ -50,7 +52,8 @@ class IcannHttpReporterTest {
   AppEngineExtension appEngineRule =
       new AppEngineExtension.Builder().withDatastoreAndCloudSql().build();
 
-  private MockHttpTransport createMockTransport(final ByteSource iirdeaResponse) {
+  private MockHttpTransport createMockTransport(
+      int statusCode, final ByteSource iirdeaResponse) {
     return new MockHttpTransport() {
       @Override
       public LowLevelHttpRequest buildRequest(String method, String url) {
@@ -59,7 +62,7 @@ class IcannHttpReporterTest {
               @Override
               public LowLevelHttpResponse execute() throws IOException {
                 MockLowLevelHttpResponse response = new MockLowLevelHttpResponse();
-                response.setStatusCode(200);
+                response.setStatusCode(statusCode);
                 response.setContentType(PLAIN_TEXT_UTF_8.toString());
                 response.setContent(iirdeaResponse.read());
                 return response;
@@ -71,6 +74,10 @@ class IcannHttpReporterTest {
     };
   }
 
+  private MockHttpTransport createMockTransport(final ByteSource iirdeaResponse) {
+    return createMockTransport(HttpStatusCodes.STATUS_CODE_OK, iirdeaResponse);
+  }
+
   @BeforeEach
   void beforeEach() {
     createTld("test");
@@ -117,10 +124,21 @@ class IcannHttpReporterTest {
   @Test
   void testFail_BadIirdeaResponse() throws Exception {
     IcannHttpReporter reporter = createReporter();
-    reporter.httpTransport = createMockTransport(IIRDEA_BAD_XML);
+    reporter.httpTransport =
+        createMockTransport(HttpStatusCodes.STATUS_CODE_BAD_REQUEST, IIRDEA_BAD_XML);
     assertThat(reporter.send(FAKE_PAYLOAD, "test-transactions-201706.csv")).isFalse();
   }
 
+  @Test
+  void testFail_transportException() throws Exception {
+    IcannHttpReporter reporter = createReporter();
+    reporter.httpTransport =
+        createMockTransport(HttpStatusCodes.STATUS_CODE_FORBIDDEN, ByteSource.empty());
+    assertThrows(
+        HttpResponseException.class,
+        () -> reporter.send(FAKE_PAYLOAD, "test-transactions-201706.csv"));
+  }
+
   @Test
   void testFail_invalidFilename_nonSixDigitYearMonth() {
     IcannHttpReporter reporter = createReporter();

core/src/test/java/google/registry/schema/integration/SqlIntegrationTestSuite.java

@@ -27,6 +27,7 @@ import google.registry.model.poll.PollMessageTest;
 import google.registry.model.registry.RegistryLockDaoTest;
 import google.registry.model.registry.label.ReservedListSqlDaoTest;
 import google.registry.model.reporting.Spec11ThreatMatchTest;
+import google.registry.model.tmch.ClaimsListDaoTest;
 import google.registry.persistence.transaction.JpaEntityCoverageExtension;
 import google.registry.persistence.transaction.JpaTestRules.JpaIntegrationWithCoverageExtension;
 import google.registry.schema.cursor.CursorDaoTest;
@@ -35,7 +36,6 @@ import google.registry.schema.integration.SqlIntegrationTestSuite.BeforeSuiteTes
 import google.registry.schema.registrar.RegistrarDaoTest;
 import google.registry.schema.server.LockDaoTest;
 import google.registry.schema.tld.PremiumListDaoTest;
-import google.registry.schema.tmch.ClaimsListDaoTest;
 import google.registry.testing.AppEngineExtension;
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeAll;

core/src/test/java/google/registry/tmch/TmchDnlActionTest.java

@@ -50,7 +50,8 @@ class TmchDnlActionTest extends TmchActionTestCase {
 
     // Make sure the contents of testdata/dnl-latest.csv got inserted into the database.
     ClaimsListShard claimsList = ClaimsListShard.get();
-    assertThat(claimsList.getCreationTime()).isEqualTo(DateTime.parse("2013-11-24T23:15:37.4Z"));
+    assertThat(claimsList.getTmdbGenerationTime())
+        .isEqualTo(DateTime.parse("2013-11-24T23:15:37.4Z"));
     assertThat(claimsList.getClaimKey("xn----7sbejwbn3axu3d"))
         .hasValue("2013112500/7/4/8/dIHW0DiuybvhdP8kIz");
     assertThat(claimsList.getClaimKey("lolcat")).isEmpty();

core/src/test/java/google/registry/tools/DomainLockUtilsTest.java

@@ -271,6 +271,7 @@ public final class DomainLockUtilsTest {
             .param(
                 RelockDomainAction.OLD_UNLOCK_REVISION_ID_PARAM,
                 String.valueOf(lock.getRevisionId()))
+            .param(RelockDomainAction.PREVIOUS_ATTEMPTS_PARAM, "0")
             .etaDelta(
                 standardHours(6).minus(standardSeconds(30)),
                 standardDays(6).plus(standardSeconds(30))));

core/src/test/java/google/registry/tools/UploadClaimsListCommandTest.java

@@ -37,7 +37,8 @@ class UploadClaimsListCommandTest extends CommandTestCase<UploadClaimsListComman
     runCommand("--force", filename);
 
     ClaimsListShard claimsList = ClaimsListShard.get();
-    assertThat(claimsList.getCreationTime()).isEqualTo(DateTime.parse("2012-08-16T00:00:00.0Z"));
+    assertThat(claimsList.getTmdbGenerationTime())
+        .isEqualTo(DateTime.parse("2012-08-16T00:00:00.0Z"));
     assertThat(claimsList.getClaimKey("example"))
         .hasValue("2013041500/2/6/9/rJ1NrDO92vDsAzf7EQzgjX4R0000000001");
     assertThat(claimsList.getClaimKey("another-example"))

core/src/test/java/google/registry/webdriver/OteSetupConsoleScreenshotTest.java

@@ -41,7 +41,7 @@ public class OteSetupConsoleScreenshotTest extends WebDriverTestCase {
   @RetryingTest(3)
   void get_owner_fails() throws Throwable {
     driver.get(server.getUrl("/registrar-ote-setup"));
-    driver.waitForElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.tagName("h1"));
     driver.diffPage("unauthorized");
   }
 
@@ -49,14 +49,14 @@ public class OteSetupConsoleScreenshotTest extends WebDriverTestCase {
   void get_admin_succeeds() throws Throwable {
     server.setIsAdmin(true);
     driver.get(server.getUrl("/registrar-ote-setup"));
-    driver.waitForElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.tagName("h1"));
     driver.diffPage("formEmpty");
     driver.findElement(By.id("clientId")).sendKeys("acmereg");
     driver.findElement(By.id("email")).sendKeys("acmereg@registry.example");
     driver.findElement(By.id("password")).sendKeys("StRoNgPaSsWoRd");
     driver.diffPage("formFilled");
     driver.findElement(By.id("submit-button")).click();
-    driver.waitForElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.tagName("h1"));
     driver.diffPage("oteResult");
   }
 
@@ -64,11 +64,11 @@ public class OteSetupConsoleScreenshotTest extends WebDriverTestCase {
   void get_admin_fails_badEmail() throws Throwable {
     server.setIsAdmin(true);
     driver.get(server.getUrl("/registrar-ote-setup"));
-    driver.waitForElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.tagName("h1"));
     driver.findElement(By.id("clientId")).sendKeys("acmereg");
     driver.findElement(By.id("email")).sendKeys("bad email");
     driver.findElement(By.id("submit-button")).click();
-    driver.waitForElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.tagName("h1"));
     driver.diffPage("oteResultFailed");
   }
 }

core/src/test/java/google/registry/webdriver/RegistrarConsoleScreenshotTest.java

@@ -68,7 +68,7 @@ class RegistrarConsoleScreenshotTest extends WebDriverTestCase {
   @RetryingTest(3)
   void index_owner() throws Throwable {
     driver.get(server.getUrl("/registrar"));
-    driver.waitForElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.tagName("h1"));
     driver.diffPage("page");
   }
 
@@ -77,7 +77,7 @@ class RegistrarConsoleScreenshotTest extends WebDriverTestCase {
   void index_adminAndOwner() throws Throwable {
     server.setIsAdmin(true);
     driver.get(server.getUrl("/registrar"));
-    driver.waitForElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.tagName("h1"));
     driver.diffPage("page");
   }
 
@@ -88,22 +88,21 @@ class RegistrarConsoleScreenshotTest extends WebDriverTestCase {
     // To make sure we're only ADMIN (and not also "OWNER"), we switch to the NewRegistrar we
     // aren't in the contacts of
     driver.get(server.getUrl("/registrar?clientId=NewRegistrar"));
-    driver.waitForElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.tagName("h1"));
     driver.diffPage("page");
   }
 
   @RetryingTest(3)
   void contactUs() throws Throwable {
     driver.get(server.getUrl("/registrar#contact-us"));
-    driver.waitForElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.tagName("h1"));
     driver.diffPage("page");
   }
 
   @RetryingTest(3)
   void settingsContact() throws Throwable {
     driver.get(server.getUrl("/registrar#contact-settings"));
-    Thread.sleep(1000);
-    driver.waitForElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.tagName("h1"));
     driver.diffPage("page");
   }
 
@@ -112,16 +111,14 @@ class RegistrarConsoleScreenshotTest extends WebDriverTestCase {
   void settingsContact_asAdmin() throws Throwable {
     server.setIsAdmin(true);
     driver.get(server.getUrl("/registrar?clientId=NewRegistrar#contact-settings"));
-    Thread.sleep(1000);
-    driver.waitForElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.tagName("h1"));
     driver.diffPage("page");
   }
 
   @RetryingTest(3)
   void settingsContactItem() throws Throwable {
     driver.get(server.getUrl("/registrar#contact-settings/johndoe@theregistrar.com"));
-    Thread.sleep(1000);
-    driver.waitForElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.tagName("h1"));
     driver.diffPage("page");
   }
 
@@ -132,8 +129,7 @@ class RegistrarConsoleScreenshotTest extends WebDriverTestCase {
     driver.get(
         server.getUrl(
             "/registrar?clientId=NewRegistrar#contact-settings/janedoe@theregistrar.com"));
-    Thread.sleep(1000);
-    driver.waitForElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.tagName("h1"));
     driver.diffPage("page");
   }
 
@@ -141,9 +137,8 @@ class RegistrarConsoleScreenshotTest extends WebDriverTestCase {
   void settingsContactEdit() throws Throwable {
     driver.manage().window().setSize(new Dimension(1050, 2000));
     driver.get(server.getUrl("/registrar#contact-settings/johndoe@theregistrar.com"));
-    Thread.sleep(1000);
-    driver.waitForElement(By.tagName("h1"));
-    driver.waitForElement(By.id("reg-app-btn-edit")).click();
+    driver.waitForDisplayedElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.id("reg-app-btn-edit")).click();
     driver.diffPage("page");
   }
 
@@ -162,9 +157,8 @@ class RegistrarConsoleScreenshotTest extends WebDriverTestCase {
         });
     driver.manage().window().setSize(new Dimension(1050, 2000));
     driver.get(server.getUrl("/registrar#contact-settings/johndoe@theregistrar.com"));
-    Thread.sleep(1000);
-    driver.waitForElement(By.tagName("h1"));
-    driver.waitForElement(By.id("reg-app-btn-edit")).click();
+    driver.waitForDisplayedElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.id("reg-app-btn-edit")).click();
     // The password should show as dots when the user types it in
     driver.findElement(By.id("contacts[1].registryLockPassword")).sendKeys("password");
     driver.diffPage("page_with_password");
@@ -179,9 +173,10 @@ class RegistrarConsoleScreenshotTest extends WebDriverTestCase {
     Thread.sleep(5);
     driver.diffPage("page_with_password_after_hide");
 
-    // now actually set the password
-    driver.waitForElement(By.id("reg-app-btn-save")).click();
-    Thread.sleep(500);
+    // Now click the Save button and wait for another Edit button to show up
+    driver.waitForRefreshedElementAfterAction(
+        () -> driver.waitForDisplayedElement(By.id("reg-app-btn-save")).click(),
+        By.id("reg-app-btn-edit"));
     driver.diffPage("contact_view");
 
     server.runInAppEngineEnvironment(
@@ -213,9 +208,8 @@ class RegistrarConsoleScreenshotTest extends WebDriverTestCase {
         });
     driver.manage().window().setSize(new Dimension(1050, 2000));
     driver.get(server.getUrl("/registrar#contact-settings/johndoe@theregistrar.com"));
-    Thread.sleep(1000);
-    driver.waitForElement(By.tagName("h1"));
-    driver.waitForElement(By.id("reg-app-btn-edit")).click();
+    driver.waitForDisplayedElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.id("reg-app-btn-edit")).click();
     driver.diffPage("page");
   }
 
@@ -225,9 +219,8 @@ class RegistrarConsoleScreenshotTest extends WebDriverTestCase {
         () -> persistResource(makeRegistrar2().asBuilder().setRegistryLockAllowed(true).build()));
     driver.manage().window().setSize(new Dimension(1050, 2000));
     driver.get(server.getUrl("/registrar#contact-settings/johndoe@theregistrar.com"));
-    Thread.sleep(1000);
-    driver.waitForElement(By.tagName("h1"));
-    driver.waitForElement(By.id("reg-app-btn-edit")).click();
+    driver.waitForDisplayedElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.id("reg-app-btn-edit")).click();
     driver.diffPage("page");
   }
 
@@ -235,9 +228,8 @@ class RegistrarConsoleScreenshotTest extends WebDriverTestCase {
   void settingsContactAdd() throws Throwable {
     driver.manage().window().setSize(new Dimension(1050, 2000));
     driver.get(server.getUrl("/registrar#contact-settings"));
-    Thread.sleep(1000);
-    driver.waitForElement(By.tagName("h1"));
-    driver.waitForElement(By.id("reg-app-btn-add")).click();
+    driver.waitForDisplayedElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.id("reg-app-btn-add")).click();
     // Attempt to fix flaky tests. The going theory is that the click button CSS animation needs to
     // finish before the screenshot is captured.
     Thread.sleep(250);
@@ -251,10 +243,10 @@ class RegistrarConsoleScreenshotTest extends WebDriverTestCase {
     // To make sure we're only ADMIN (and not also "OWNER"), we switch to the NewRegistrar we
     // aren't in the contacts of
     driver.get(server.getUrl("/registrar?clientId=NewRegistrar#admin-settings"));
-    driver.waitForElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.tagName("h1"));
     driver.diffPage("view");
-    driver.waitForElement(By.id("reg-app-btn-edit")).click();
-    driver.waitForElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.id("reg-app-btn-edit")).click();
+    driver.waitForDisplayedElement(By.tagName("h1"));
     driver.diffPage("edit");
   }
 
@@ -272,7 +264,7 @@ class RegistrarConsoleScreenshotTest extends WebDriverTestCase {
   void settingsAdmin_whenNotAdmin_showsHome() throws Throwable {
     driver.manage().window().setSize(new Dimension(1050, 2000));
     driver.get(server.getUrl("/registrar#admin-settings"));
-    driver.waitForElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.tagName("h1"));
     driver.diffPage("view");
   }
 
@@ -280,7 +272,7 @@ class RegistrarConsoleScreenshotTest extends WebDriverTestCase {
   void getOteStatus_noButtonWhenReal() throws Exception {
     server.setIsAdmin(true);
     driver.get(server.getUrl("/registrar#admin-settings"));
-    driver.waitForElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.tagName("h1"));
     driver.diffPage("result");
   }
 
@@ -299,7 +291,7 @@ class RegistrarConsoleScreenshotTest extends WebDriverTestCase {
   void getOteStatus_completed() throws Exception {
     server.setIsAdmin(true);
     driver.get(server.getUrl("/registrar?clientId=otefinished-1#admin-settings"));
-    driver.waitForElement(By.id("btn-ote-status"));
+    driver.waitForDisplayedElement(By.id("btn-ote-status"));
     driver.diffPage("before_click");
     driver.findElement(By.id("btn-ote-status")).click();
     driver.findElement(By.id("ote-results-table")).click();
@@ -312,10 +304,10 @@ class RegistrarConsoleScreenshotTest extends WebDriverTestCase {
   void settingsSecurity() throws Throwable {
     driver.manage().window().setSize(new Dimension(1050, 2000));
     driver.get(server.getUrl("/registrar#security-settings"));
-    driver.waitForElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.tagName("h1"));
     driver.diffPage("view");
-    driver.waitForElement(By.id("reg-app-btn-edit")).click();
-    driver.waitForElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.id("reg-app-btn-edit")).click();
+    driver.waitForDisplayedElement(By.tagName("h1"));
     driver.diffPage("edit");
   }
 
@@ -325,7 +317,7 @@ class RegistrarConsoleScreenshotTest extends WebDriverTestCase {
     server.setIsAdmin(true);
     driver.manage().window().setSize(new Dimension(1050, 2000));
     driver.get(server.getUrl("/registrar?clientId=NewRegistrar#security-settings"));
-    driver.waitForElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.tagName("h1"));
     driver.diffPage("view");
   }
 
@@ -343,10 +335,10 @@ class RegistrarConsoleScreenshotTest extends WebDriverTestCase {
         });
     driver.manage().window().setSize(new Dimension(1050, 2000));
     driver.get(server.getUrl("/registrar#security-settings"));
-    driver.waitForElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.tagName("h1"));
     driver.diffPage("view");
-    driver.waitForElement(By.id("reg-app-btn-edit")).click();
-    driver.waitForElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.id("reg-app-btn-edit")).click();
+    driver.waitForDisplayedElement(By.tagName("h1"));
     driver.diffPage("edit");
   }
 
@@ -363,10 +355,10 @@ class RegistrarConsoleScreenshotTest extends WebDriverTestCase {
         });
     driver.manage().window().setSize(new Dimension(1050, 2000));
     driver.get(server.getUrl("/registrar#security-settings"));
-    driver.waitForElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.tagName("h1"));
     driver.diffPage("view");
-    driver.waitForElement(By.id("reg-app-btn-edit")).click();
-    driver.waitForElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.id("reg-app-btn-edit")).click();
+    driver.waitForDisplayedElement(By.tagName("h1"));
     driver.diffPage("edit");
   }
 
@@ -377,14 +369,14 @@ class RegistrarConsoleScreenshotTest extends WebDriverTestCase {
             persistResource(
                 loadRegistrar("TheRegistrar").asBuilder().setState(State.DISABLED).build()));
     driver.get(server.getUrl("/registrar"));
-    driver.waitForElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.tagName("h1"));
     driver.diffPage("view");
   }
 
   @RetryingTest(3)
   void settingsWhois() throws Throwable {
     driver.get(server.getUrl("/registrar#whois-settings"));
-    driver.waitForElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.tagName("h1"));
     driver.diffPage("page");
   }
 
@@ -392,8 +384,8 @@ class RegistrarConsoleScreenshotTest extends WebDriverTestCase {
   void settingsWhoisEdit() throws Throwable {
     driver.manage().window().setSize(new Dimension(1050, 2000));
     driver.get(server.getUrl("/registrar#whois-settings"));
-    driver.waitForElement(By.id("reg-app-btn-edit")).click();
-    Thread.sleep(1000);
+    driver.waitForDisplayedElement(By.id("reg-app-btn-edit")).click();
+    driver.waitForDisplayedElement(By.id("reg-app-btn-save"));
     driver.diffPage("page");
   }
 
@@ -401,10 +393,12 @@ class RegistrarConsoleScreenshotTest extends WebDriverTestCase {
   void settingsWhoisEditError() throws Throwable {
     driver.manage().window().setSize(new Dimension(1050, 2000));
     driver.get(server.getUrl("/registrar#whois-settings"));
-    driver.waitForElement(By.id("reg-app-btn-edit")).click();
+    driver.waitForDisplayedElement(By.id("reg-app-btn-edit")).click();
     driver.setFormFieldsById(ImmutableMap.of("faxNumber", "cat"));
-    driver.waitForElement(By.id("reg-app-btn-save")).click();
-    Thread.sleep(1000);
+    driver.waitForDisplayedElement(By.id("reg-app-btn-save")).click();
+    // After the click, a div element without id would show up with an error message.
+    driver.waitForElementWithCondition(
+        By.tagName("div"), e -> e.getText().startsWith("Must be a valid +E.164 phone number,"));
     driver.diffPage("page");
   }
 
@@ -412,7 +406,7 @@ class RegistrarConsoleScreenshotTest extends WebDriverTestCase {
   void indexPage_smallScrolledDown() throws Throwable {
     driver.manage().window().setSize(new Dimension(600, 300));
     driver.get(server.getUrl("/registrar"));
-    driver.waitForElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.tagName("h1"));
     driver.executeScript("document.getElementById('reg-content-and-footer').scrollTop = 200");
     Thread.sleep(500);
     driver.diffPage("page");
@@ -439,21 +433,21 @@ class RegistrarConsoleScreenshotTest extends WebDriverTestCase {
     driver.get(
         server.getUrl(
             "/registry-lock-verify?isLock=true&lockVerificationCode=" + lockVerificationCode));
-    driver.waitForElement(By.id("reg-content"));
+    driver.waitForDisplayedElement(By.id("reg-content"));
     driver.diffPage("page");
   }
 
   @RetryingTest(3)
   void registryLockVerify_unknownLock() throws Throwable {
     driver.get(server.getUrl("/registry-lock-verify?isLock=true&lockVerificationCode=asdfasdf"));
-    driver.waitForElement(By.id("reg-content"));
+    driver.waitForDisplayedElement(By.id("reg-content"));
     driver.diffPage("page");
   }
 
   @RetryingTest(3)
   void registryLock_empty() throws Throwable {
     driver.get(server.getUrl("/registrar?clientId=TheRegistrar#registry-lock"));
-    driver.waitForElement(By.tagName("h2"));
+    driver.waitForDisplayedElement(By.tagName("h2"));
     driver.diffPage("page");
   }
 
@@ -465,7 +459,7 @@ class RegistrarConsoleScreenshotTest extends WebDriverTestCase {
           return null;
         });
     driver.get(server.getUrl("/registrar?clientId=TheRegistrar#registry-lock"));
-    driver.waitForElement(By.tagName("h2"));
+    driver.waitForDisplayedElement(By.tagName("h2"));
     driver.diffPage("page");
   }
 
@@ -477,7 +471,7 @@ class RegistrarConsoleScreenshotTest extends WebDriverTestCase {
           return null;
         });
     driver.get(server.getUrl("/registrar#registry-lock"));
-    driver.waitForElement(By.tagName("h2"));
+    driver.waitForDisplayedElement(By.tagName("h2"));
     driver.diffPage("page");
   }
 
@@ -530,7 +524,7 @@ class RegistrarConsoleScreenshotTest extends WebDriverTestCase {
           return null;
         });
     driver.get(server.getUrl("/registrar#registry-lock"));
-    driver.waitForElement(By.tagName("h2"));
+    driver.waitForDisplayedElement(By.tagName("h2"));
     driver.diffPage("page");
   }
 
@@ -542,9 +536,9 @@ class RegistrarConsoleScreenshotTest extends WebDriverTestCase {
           return null;
         });
     driver.get(server.getUrl("/registrar#registry-lock"));
-    driver.waitForElement(By.tagName("h2"));
+    driver.waitForDisplayedElement(By.tagName("h2"));
     driver.findElement(By.id("button-unlock-example.tld")).click();
-    driver.waitForElement(By.className("modal-content"));
+    driver.waitForDisplayedElement(By.className("modal-content"));
     driver.findElement(By.id("domain-lock-password")).sendKeys("password");
     driver.diffPage("page");
   }
@@ -559,9 +553,9 @@ class RegistrarConsoleScreenshotTest extends WebDriverTestCase {
           return null;
         });
     driver.get(server.getUrl("/registrar#registry-lock"));
-    driver.waitForElement(By.tagName("h2"));
+    driver.waitForDisplayedElement(By.tagName("h2"));
     driver.findElement(By.id("button-lock-domain")).click();
-    driver.waitForElement(By.className("modal-content"));
+    driver.waitForDisplayedElement(By.className("modal-content"));
     driver.findElement(By.id("domain-lock-input-value")).sendKeys("somedomain.tld");
     driver.diffPage("page");
   }
@@ -578,7 +572,7 @@ class RegistrarConsoleScreenshotTest extends WebDriverTestCase {
           return null;
         });
     driver.get(server.getUrl("/registrar?clientId=TheRegistrar#registry-lock"));
-    driver.waitForElement(By.tagName("h2"));
+    driver.waitForDisplayedElement(By.tagName("h2"));
     driver.diffPage("page");
   }
 

core/src/test/java/google/registry/webdriver/RegistrarConsoleWebTest.java

@@ -55,7 +55,7 @@ public class RegistrarConsoleWebTest extends WebDriverTestCase {
 
   /** Checks that an element is visible. */
   void assertEltVisible(String eltId) throws Throwable {
-    assertThat(driver.waitForElement(By.id(eltId)).isDisplayed()).isTrue();
+    assertThat(driver.waitForDisplayedElement(By.id(eltId)).isDisplayed()).isTrue();
   }
 
   /** Checks that an element is invisible. */
@@ -141,7 +141,7 @@ public class RegistrarConsoleWebTest extends WebDriverTestCase {
   @RetryingTest(3)
   void testWhoisSettingsEdit() throws Throwable {
     driver.get(server.getUrl("/registrar#whois-settings"));
-    driver.waitForElement(By.id("reg-app-btn-edit")).click();
+    driver.waitForDisplayedElement(By.id("reg-app-btn-edit")).click();
     driver.setFormFieldsById(
         new ImmutableMap.Builder<String, String>()
             .put("emailAddress", "test1@example.com")
@@ -178,7 +178,7 @@ public class RegistrarConsoleWebTest extends WebDriverTestCase {
   @RetryingTest(3)
   void testContactSettingsView() throws Throwable {
     driver.get(server.getUrl("/registrar#contact-settings"));
-    driver.waitForElement(By.id("reg-app-btn-add"));
+    driver.waitForDisplayedElement(By.id("reg-app-btn-add"));
     ImmutableList<RegistrarContact> contacts =
         server.runInAppEngineEnvironment(
             () -> loadRegistrar("TheRegistrar").getContacts().asList());
@@ -192,7 +192,7 @@ public class RegistrarConsoleWebTest extends WebDriverTestCase {
   @RetryingTest(3)
   void testSecuritySettingsView() throws Throwable {
     driver.get(server.getUrl("/registrar#security-settings"));
-    driver.waitForElement(By.id("reg-app-btn-edit"));
+    driver.waitForDisplayedElement(By.id("reg-app-btn-edit"));
     Registrar registrar = server.runInAppEngineEnvironment(() -> loadRegistrar("TheRegistrar"));
     assertThat(driver.findElement(By.id("phonePasscode")).getAttribute("value"))
         .isEqualTo(registrar.getPhonePasscode());

core/src/test/java/google/registry/webdriver/RegistrarCreateConsoleScreenshotTest.java

@@ -41,7 +41,7 @@ class RegistrarCreateConsoleScreenshotTest extends WebDriverTestCase {
   @RetryingTest(3)
   void get_owner_fails() throws Throwable {
     driver.get(server.getUrl("/registrar-create"));
-    driver.waitForElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.tagName("h1"));
     driver.diffPage("unauthorized");
   }
 
@@ -49,7 +49,7 @@ class RegistrarCreateConsoleScreenshotTest extends WebDriverTestCase {
   void get_admin_succeeds() throws Throwable {
     server.setIsAdmin(true);
     driver.get(server.getUrl("/registrar-create"));
-    driver.waitForElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.tagName("h1"));
     driver.diffPage("formEmpty");
     driver.findElement(By.id("clientId")).sendKeys("my-name");
     driver.findElement(By.id("name")).sendKeys("registrar name");
@@ -69,7 +69,7 @@ class RegistrarCreateConsoleScreenshotTest extends WebDriverTestCase {
     driver.findElement(By.id("passcode")).sendKeys("01234");
     driver.diffPage("formFilled");
     driver.findElement(By.id("submit-button")).click();
-    driver.waitForElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.tagName("h1"));
     driver.diffPage("createResult");
   }
 
@@ -77,7 +77,7 @@ class RegistrarCreateConsoleScreenshotTest extends WebDriverTestCase {
   void get_admin_fails_badEmail() throws Throwable {
     server.setIsAdmin(true);
     driver.get(server.getUrl("/registrar-create"));
-    driver.waitForElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.tagName("h1"));
     driver.findElement(By.id("clientId")).sendKeys("my-name");
     driver.findElement(By.id("name")).sendKeys("registrar name");
     driver
@@ -93,7 +93,7 @@ class RegistrarCreateConsoleScreenshotTest extends WebDriverTestCase {
     driver.findElement(By.id("city")).sendKeys("Citysville");
     driver.findElement(By.id("countryCode")).sendKeys("fr");
     driver.findElement(By.id("submit-button")).click();
-    driver.waitForElement(By.tagName("h1"));
+    driver.waitForDisplayedElement(By.tagName("h1"));
     driver.diffPage("createResultFailed");
   }
 }

core/src/test/java/google/registry/webdriver/WebDriverPlusScreenDifferExtension.java

@@ -19,9 +19,11 @@ import static java.util.stream.Collectors.joining;
 import static org.apache.commons.text.StringEscapeUtils.escapeEcmaScript;
 
 import com.google.common.base.Preconditions;
+import com.google.common.base.Predicates;
 import java.net.URL;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.TimeUnit;
 import java.util.function.Predicate;
@@ -34,6 +36,7 @@ import org.openqa.selenium.Dimension;
 import org.openqa.selenium.HasCapabilities;
 import org.openqa.selenium.JavascriptExecutor;
 import org.openqa.selenium.OutputType;
+import org.openqa.selenium.StaleElementReferenceException;
 import org.openqa.selenium.TakesScreenshot;
 import org.openqa.selenium.WebDriver;
 import org.openqa.selenium.WebDriverException;
@@ -55,7 +58,6 @@ public final class WebDriverPlusScreenDifferExtension
         HasCapabilities {
 
   private static final int WAIT_FOR_ELEMENTS_POLLING_INTERVAL_MS = 10;
-  private static final int WAIT_FOR_ELEMENTS_BONUS_DELAY_MS = 150;
 
   // The maximum difference between pixels that would be considered as "identical". Calculated as
   // the sum of the absolute difference between the values of the RGB channels. So a 120,30,200
@@ -121,28 +123,40 @@ public final class WebDriverPlusScreenDifferExtension
     driver.get(url.toString());
   }
 
-  /** Waits indefinitely for an element to appear on the page, then returns it. */
-  WebElement waitForElement(By by) throws InterruptedException {
-    while (true) {
-      List<WebElement> elements = findElements(by);
-      if (!elements.isEmpty()) {
-        Thread.sleep(WAIT_FOR_ELEMENTS_BONUS_DELAY_MS);
-        return elements.get(0);
-      }
-      Thread.sleep(WAIT_FOR_ELEMENTS_POLLING_INTERVAL_MS);
-    }
+  /** Waits indefinitely for a <em>visible</em> element matching {@code by}, then returns it. */
+  WebElement waitForDisplayedElement(By by) throws InterruptedException {
+    return waitForElementWithCondition(by, WebElement::isDisplayed);
   }
 
-  /** Waits for element matching {@code by} whose {@code attribute} satisfies {@code predicate}. */
-  public WebElement waitForAttribute(
-      By by, String attribute, Predicate</*@Nullable*/ ? super CharSequence> predicate)
+  /** Waits indefinitely for an element matching {@code by}, then returns it. */
+  WebElement waitForElement(By by) throws InterruptedException {
+    return waitForElementWithCondition(by, Predicates.alwaysTrue());
+  }
+
+  /**
+   * Executes an action and waits indefinitely for the replacement of an <em>existing</em> element.
+   */
+  WebElement waitForRefreshedElementAfterAction(ThrowingRunnable action, By by) throws Exception {
+    WebElement element = findElement(by);
+    action.run();
+    while (true) {
+      try {
+        element.isDisplayed(); // Eventually triggers StaleElementReferenceException
+        Thread.sleep(WAIT_FOR_ELEMENTS_POLLING_INTERVAL_MS);
+      } catch (StaleElementReferenceException e) {
+        break;
+      }
+    }
+    return waitForDisplayedElement(by);
+  }
+
+  /** Waits for an element matching {@code by} and satisfying {@code predicate}, then returns it. */
+  WebElement waitForElementWithCondition(By by, Predicate<WebElement> predicate)
       throws InterruptedException {
     while (true) {
-      for (WebElement element : findElements(by)) {
-        if (predicate.test(element.getAttribute(attribute))) {
-          Thread.sleep(WAIT_FOR_ELEMENTS_BONUS_DELAY_MS);
-          return element;
-        }
+      Optional<WebElement> firstMatch = findElements(by).stream().filter(predicate).findFirst();
+      if (firstMatch.isPresent()) {
+        return firstMatch.get();
       }
       Thread.sleep(WAIT_FOR_ELEMENTS_POLLING_INTERVAL_MS);
     }
@@ -303,4 +317,9 @@ public final class WebDriverPlusScreenDifferExtension
     Preconditions.checkNotNull(imageNamePrefix);
     return imageNamePrefix + "_" + imageKey;
   }
+
+  public interface ThrowingRunnable {
+
+    void run() throws Exception;
+  }
 }

core/src/test/resources/google/registry/model/schema.txt

@@ -270,9 +270,9 @@ class google.registry.model.domain.DomainHistory {
   google.registry.model.domain.Period period;
   google.registry.model.eppcommon.Trid trid;
   google.registry.model.reporting.HistoryEntry$Type type;
-  google.registry.persistence.VKey<google.registry.model.domain.DomainBase> domainRepoId;
   java.lang.Boolean requestedByRegistrar;
   java.lang.String clientId;
+  java.lang.String domainRepoId;
   java.lang.String otherClientId;
   java.lang.String reason;
   java.util.Set<google.registry.model.reporting.DomainTransactionRecord> domainTransactionRecords;

db/src/main/resources/sql/flyway/V51__use_composite_primary_key_for_domain_history_table.sql

@@ -0,0 +1,34 @@
+-- Copyright 2020 The Nomulus Authors. All Rights Reserved.
+--
+-- Licensed under the Apache License, Version 2.0 (the "License");
+-- you may not use this file except in compliance with the License.
+-- You may obtain a copy of the License at
+--
+--     http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing, software
+-- distributed under the License is distributed on an "AS IS" BASIS,
+-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+-- See the License for the specific language governing permissions and
+-- limitations under the License.
+
+alter sequence "history_id_sequence" increment 50;
+
+alter table "DomainHistory" alter column history_revision_id drop default;
+alter table "ContactHistory" alter column history_revision_id drop default;
+alter table "HostHistory" alter column history_revision_id drop default;
+
+alter table if exists "DomainHistoryHost"
+    drop constraint fk6b8eqdxwe3guc56tgpm89atx;
+
+alter table "DomainHistory" drop constraint "DomainHistory_pkey";
+
+alter table "DomainHistory"
+    add constraint "DomainHistory_pkey" primary key (domain_repo_id, history_revision_id);
+
+alter table "DomainHistoryHost" add column domain_history_domain_repo_id text not null;
+
+alter table if exists "DomainHistoryHost"
+    add constraint FKa9woh3hu8gx5x0vly6bai327n
+    foreign key (domain_history_domain_repo_id, domain_history_history_revision_id)
+    references "DomainHistory";

db/src/main/resources/sql/schema/db-schema.sql.generated

@@ -11,7 +11,7 @@
 -- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 -- See the License for the specific language governing permissions and
 -- limitations under the License.
-create sequence history_id_sequence start 1 increment 1;
+create sequence history_id_sequence start 1 increment 50;
 
     create table "AllocationToken" (
        token text not null,
@@ -86,8 +86,8 @@ create sequence history_id_sequence start 1 increment 1;
 
     create table "ClaimsList" (
        revision_id  bigserial not null,
-        creation_timestamp timestamptz not null,
         tmdb_generation_time timestamptz not null,
+        creation_timestamp timestamptz not null,
         primary key (revision_id)
     );
 
@@ -286,7 +286,8 @@ create sequence history_id_sequence start 1 increment 1;
     );
 
     create table "DomainHistory" (
-       history_revision_id int8 not null,
+       domain_repo_id text not null,
+        history_revision_id int8 not null,
         history_by_superuser boolean not null,
         history_registrar_id text,
         history_modification_time timestamptz not null,
@@ -341,12 +342,12 @@ create sequence history_id_sequence start 1 increment 1;
         last_epp_update_time timestamptz,
         statuses text[],
         update_timestamp timestamptz,
-        domain_repo_id text not null,
-        primary key (history_revision_id)
+        primary key (domain_repo_id, history_revision_id)
     );
 
     create table "DomainHistoryHost" (
-       domain_history_history_revision_id int8 not null,
+       domain_history_domain_repo_id text not null,
+        domain_history_history_revision_id int8 not null,
         host_repo_id text
     );
 
@@ -644,8 +645,8 @@ create index spec11threatmatch_check_date_idx on "Spec11ThreatMatch" (check_date
        references "ClaimsList";
 
     alter table if exists "DomainHistoryHost" 
-       add constraint FK378h8v3j8qd8xtjn2e0bcmrtj 
-       foreign key (domain_history_history_revision_id) 
+       add constraint FKa9woh3hu8gx5x0vly6bai327n 
+       foreign key (domain_history_domain_repo_id, domain_history_history_revision_id) 
        references "DomainHistory";
 
     alter table if exists "DomainHost" 

db/src/main/resources/sql/schema/nomulus.golden.sql

@@ -275,24 +275,12 @@ CREATE TABLE public."Contact" (
 );
 
 
---
--- Name: history_id_sequence; Type: SEQUENCE; Schema: public; Owner: -
---
-
-CREATE SEQUENCE public.history_id_sequence
-    START WITH 1
-    INCREMENT BY 1
-    NO MINVALUE
-    NO MAXVALUE
-    CACHE 1;
-
-
 --
 -- Name: ContactHistory; Type: TABLE; Schema: public; Owner: -
 --
 
 CREATE TABLE public."ContactHistory" (
-    history_revision_id bigint DEFAULT nextval('public.history_id_sequence'::regclass) NOT NULL,
+    history_revision_id bigint NOT NULL,
     history_by_superuser boolean NOT NULL,
     history_registrar_id text,
     history_modification_time timestamp with time zone NOT NULL,
@@ -431,7 +419,7 @@ CREATE TABLE public."Domain" (
 --
 
 CREATE TABLE public."DomainHistory" (
-    history_revision_id bigint DEFAULT nextval('public.history_id_sequence'::regclass) NOT NULL,
+    history_revision_id bigint NOT NULL,
     history_by_superuser boolean NOT NULL,
     history_registrar_id text,
     history_modification_time timestamp with time zone NOT NULL,
@@ -496,7 +484,8 @@ CREATE TABLE public."DomainHistory" (
 
 CREATE TABLE public."DomainHistoryHost" (
     domain_history_history_revision_id bigint NOT NULL,
-    host_repo_id text
+    host_repo_id text,
+    domain_history_domain_repo_id text NOT NULL
 );
 
 
@@ -549,7 +538,7 @@ ALTER SEQUENCE public."GracePeriod_id_seq" OWNED BY public."GracePeriod".id;
 --
 
 CREATE TABLE public."HostHistory" (
-    history_revision_id bigint DEFAULT nextval('public.history_id_sequence'::regclass) NOT NULL,
+    history_revision_id bigint NOT NULL,
     history_by_superuser boolean NOT NULL,
     history_registrar_id text NOT NULL,
     history_modification_time timestamp with time zone NOT NULL,
@@ -929,6 +918,18 @@ CREATE SEQUENCE public."Transaction_id_seq"
 ALTER SEQUENCE public."Transaction_id_seq" OWNED BY public."Transaction".id;
 
 
+--
+-- Name: history_id_sequence; Type: SEQUENCE; Schema: public; Owner: -
+--
+
+CREATE SEQUENCE public.history_id_sequence
+    START WITH 1
+    INCREMENT BY 50
+    NO MINVALUE
+    NO MAXVALUE
+    CACHE 1;
+
+
 --
 -- Name: BillingCancellation billing_cancellation_id; Type: DEFAULT; Schema: public; Owner: -
 --
@@ -1083,7 +1084,7 @@ ALTER TABLE ONLY public."Cursor"
 --
 
 ALTER TABLE ONLY public."DomainHistory"
-    ADD CONSTRAINT "DomainHistory_pkey" PRIMARY KEY (history_revision_id);
+    ADD CONSTRAINT "DomainHistory_pkey" PRIMARY KEY (domain_repo_id, history_revision_id);
 
 
 --
@@ -1612,14 +1613,6 @@ ALTER TABLE ONLY public."HostHistory"
     ADD CONSTRAINT fk3d09knnmxrt6iniwnp8j2ykga FOREIGN KEY (history_registrar_id) REFERENCES public."Registrar"(registrar_id);
 
 
---
--- Name: DomainHistoryHost fk6b8eqdxwe3guc56tgpm89atx; Type: FK CONSTRAINT; Schema: public; Owner: -
---
-
-ALTER TABLE ONLY public."DomainHistoryHost"
-    ADD CONSTRAINT fk6b8eqdxwe3guc56tgpm89atx FOREIGN KEY (domain_history_history_revision_id) REFERENCES public."DomainHistory"(history_revision_id);
-
-
 --
 -- Name: ClaimsEntry fk6sc6at5hedffc0nhdcab6ivuq; Type: FK CONSTRAINT; Schema: public; Owner: -
 --
@@ -1916,6 +1909,14 @@ ALTER TABLE ONLY public."PollMessage"
     ADD CONSTRAINT fk_poll_message_transfer_response_losing_registrar_id FOREIGN KEY (transfer_response_losing_registrar_id) REFERENCES public."Registrar"(registrar_id);
 
 
+--
+-- Name: DomainHistoryHost fka9woh3hu8gx5x0vly6bai327n; Type: FK CONSTRAINT; Schema: public; Owner: -
+--
+
+ALTER TABLE ONLY public."DomainHistoryHost"
+    ADD CONSTRAINT fka9woh3hu8gx5x0vly6bai327n FOREIGN KEY (domain_history_domain_repo_id, domain_history_history_revision_id) REFERENCES public."DomainHistory"(domain_repo_id, history_revision_id);
+
+
 --
 -- Name: DomainHost fkfmi7bdink53swivs390m2btxg; Type: FK CONSTRAINT; Schema: public; Owner: -
 --

dependencies.gradle

@@ -131,7 +131,7 @@ ext {
       'org.bouncycastle:bcpg-jdk15on:1.61',
       'org.bouncycastle:bcpkix-jdk15on:1.61',
       'org.bouncycastle:bcprov-jdk15on:1.61',
-      'com.fasterxml.jackson.core:jackson-databind:2.9.10',
+      'com.fasterxml.jackson.core:jackson-databind:2.11.2',
       'org.flywaydb:flyway-core:5.2.4',
       'org.glassfish.jaxb:jaxb-runtime:2.3.0',
       'org.hamcrest:hamcrest-all:1.3',

gradle/wrapper/gradle-wrapper.properties

@@ -1,5 +1,5 @@
 distributionBase=GRADLE_USER_HOME
 distributionPath=wrapper/dists
-distributionUrl=https\://services.gradle.org/distributions/gradle-6.6-all.zip
+distributionUrl=https\://services.gradle.org/distributions/gradle-6.6.1-all.zip
 zipStoreBase=GRADLE_USER_HOME
 zipStorePath=wrapper/dists

gradlew

@@ -130,7 +130,7 @@ fi
 if [ "$cygwin" = "true" -o "$msys" = "true" ] ; then
     APP_HOME=`cygpath --path --mixed "$APP_HOME"`
     CLASSPATH=`cygpath --path --mixed "$CLASSPATH"`
-    
+
     JAVACMD=`cygpath --unix "$JAVACMD"`
 
     # We build the pattern for arguments to be converted via cygpath

gradlew.bat

@@ -40,7 +40,7 @@ if defined JAVA_HOME goto findJavaFromJavaHome
 
 set JAVA_EXE=java.exe
 %JAVA_EXE% -version >NUL 2>&1
-if "%ERRORLEVEL%" == "0" goto init
+if "%ERRORLEVEL%" == "0" goto execute
 
 echo.
 echo ERROR: JAVA_HOME is not set and no 'java' command could be found in your PATH.
@@ -54,7 +54,7 @@ goto fail
 set JAVA_HOME=%JAVA_HOME:"=%
 set JAVA_EXE=%JAVA_HOME%/bin/java.exe
 
-if exist "%JAVA_EXE%" goto init
+if exist "%JAVA_EXE%" goto execute
 
 echo.
 echo ERROR: JAVA_HOME is set to an invalid directory: %JAVA_HOME%
@@ -64,21 +64,6 @@ echo location of your Java installation.
 
 goto fail
 
-:init
-@rem Get command-line arguments, handling Windows variants
-
-if not "%OS%" == "Windows_NT" goto win9xME_args
-
-:win9xME_args
-@rem Slurp the command line arguments.
-set CMD_LINE_ARGS=
-set _SKIP=2
-
-:win9xME_args_slurp
-if "x%~1" == "x" goto execute
-
-set CMD_LINE_ARGS=%*
-
 :execute
 @rem Setup the command line
 
@@ -86,7 +71,7 @@ set CLASSPATH=%APP_HOME%\gradle\wrapper\gradle-wrapper.jar
 
 
 @rem Execute Gradle
-"%JAVA_EXE%" %DEFAULT_JVM_OPTS% %JAVA_OPTS% %GRADLE_OPTS% "-Dorg.gradle.appname=%APP_BASE_NAME%" -classpath "%CLASSPATH%" org.gradle.wrapper.GradleWrapperMain %CMD_LINE_ARGS%
+"%JAVA_EXE%" %DEFAULT_JVM_OPTS% %JAVA_OPTS% %GRADLE_OPTS% "-Dorg.gradle.appname=%APP_BASE_NAME%" -classpath "%CLASSPATH%" org.gradle.wrapper.GradleWrapperMain %*
 
 :end
 @rem End local scope for the variables with windows NT shell

integration/run_schema_check.sh

@@ -26,9 +26,14 @@ Checks for post-deployment change to Flyway scripts.
 
 With Flyway, once an incremental change script is deployed, it must not be
 changed. Even changes to comments or whitespaces would cause validation
-failures during future deployment. This script checks for changes to scripts
-that have already been deployed to Sandbox. The assumption is that the schema
-in Sandbox is always newer than that in production.
+failures during future deployment. This script checks for changes (including
+removal and renaming which may happen due to incorrect merge conflict
+resolution) to scripts that have already been deployed to Sandbox. The
+assumption is that the schema in Sandbox is always newer than that in
+production.
+
+A side-effect of this check is that old branches missing recently deployed
+scripts must update first.
 
 Options:
     -h, --help  show this help text
@@ -61,7 +66,7 @@ fi
 sandbox_tag=$(fetchVersion sql sandbox ${DEV_PROJECT})
 echo "Checking Flyway scripts against schema in Sandbox (${sandbox_tag})."
 modified_sqls=$(git diff --name-status ${sandbox_tag} \
-    db/src/main/resources/sql/flyway | grep ^M | grep \.sql$ | wc -l)
+    db/src/main/resources/sql/flyway | grep "^M\|^D\|^R" | grep \.sql$ | wc -l)
 
 if [[ ${modified_sqls} = 0 ]]; then
   echo "No illegal change to deployed schema scripts."
@@ -69,6 +74,7 @@ if [[ ${modified_sqls} = 0 ]]; then
 else
   echo "Changes to the following files are not allowed:"
   echo $(git diff --name-status ${sandbox_tag} \
-      db/src/main/resources/sql/flyway | grep ^M | grep \.sql$)
+      db/src/main/resources/sql/flyway | grep "^M\|^D\|^R" | grep \.sql$)
+  echo "Make sure your branch is up to date with HEAD of master."
   exit 1
 fi

integration/testutils_bashrc

@@ -15,7 +15,8 @@
 # This file declares shell functions used by other scripts in this folder.
 
 # Fetch the tag of the currently deployed release of Nomulus server
-# or SQL schema.
+# or SQL schema. The caller is responsible for fetching relevant git
+# tags to the local repo.
 function fetchVersion() {
   local deployed_system=${1}
   local env=${2}

util/src/main/java/google/registry/util/EmailMessage.java

@@ -16,6 +16,7 @@ package google.registry.util;
 
 import com.google.auto.value.AutoValue;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.net.MediaType;
 import java.util.Collection;
 import java.util.Optional;
@@ -27,10 +28,10 @@ public abstract class EmailMessage {
 
   public abstract String subject();
   public abstract String body();
-  public abstract ImmutableList<InternetAddress> recipients();
+  public abstract ImmutableSet<InternetAddress> recipients();
   public abstract InternetAddress from();
 
-  public abstract ImmutableList<InternetAddress> bccs();
+  public abstract ImmutableSet<InternetAddress> bccs();
 
   public abstract Optional<MediaType> contentType();
   public abstract Optional<Attachment> attachment();
@@ -63,9 +64,9 @@ public abstract class EmailMessage {
     public abstract Builder setContentType(MediaType contentType);
     public abstract Builder setAttachment(Attachment attachment);
 
-    abstract ImmutableList.Builder<InternetAddress> recipientsBuilder();
+    abstract ImmutableSet.Builder<InternetAddress> recipientsBuilder();
 
-    abstract ImmutableList.Builder<InternetAddress> bccsBuilder();
+    abstract ImmutableSet.Builder<InternetAddress> bccsBuilder();
 
     public Builder addRecipient(InternetAddress value) {
       recipientsBuilder().add(value);