From 75f09c2fdf98e50e2370e3199f4d4817f7e96a42 Mon Sep 17 00:00:00 2001
From: Ben McIlwain <mcilwain@google.com>
Date: Wed, 10 Jul 2024 15:03:42 -0400
Subject: [PATCH] Fail permamently in re-save entity action when entity doesn't
 exist (#2492)

Our logs are getting gummed up with an indefinitely failing and retrying task to
re-save a prober domain that doesn't exist (likely because it was hard-deleted
by delete prober data action), so this makes the re-save action resilient to
that failure case so that it stops assuming every enqueued re-save actually
corresponds to an entity that exists, thus allowing it to fail permanently if
the entity doesn't exist.  Failing permanently is the right thing to do as if
the entity doesn't exist now there's no reason to think it will in the future,
plus all re-saves are optimistic rather than guaranteed anyway.

This should fix http://b/350530720
---
 .../registry/batch/ResaveEntityAction.java       | 12 ++++++++++--
 .../registry/batch/ResaveEntityActionTest.java   | 16 ++++++++++++++--
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/core/src/main/java/google/registry/batch/ResaveEntityAction.java b/core/src/main/java/google/registry/batch/ResaveEntityAction.java
index f27307800..507004e99 100644
--- a/core/src/main/java/google/registry/batch/ResaveEntityAction.java
+++ b/core/src/main/java/google/registry/batch/ResaveEntityAction.java
@@ -29,6 +29,7 @@ import google.registry.request.Action.Method;
 import google.registry.request.Parameter;
 import google.registry.request.Response;
 import google.registry.request.auth.Auth;
+import java.util.Optional;
 import javax.inject.Inject;
 import org.joda.time.DateTime;
 
@@ -74,8 +75,15 @@ public class ResaveEntityAction implements Runnable {
         "Re-saving entity %s which was enqueued at %s.", resourceKey, requestedTime);
     tm().transact(
             () -> {
-              EppResource entity = tm().loadByKey(VKey.createEppVKeyFromString(resourceKey));
-              tm().put(entity.cloneProjectedAtTime(tm().getTransactionTime()));
+              Optional<EppResource> entity =
+                  tm().loadByKeyIfPresent(VKey.createEppVKeyFromString(resourceKey));
+              if (entity.isEmpty()) {
+                logger.atSevere().log(
+                    "Could not re-save entity %s because it does not exist; failing permanently.",
+                    resourceKey);
+                return;
+              }
+              tm().put(entity.get().cloneProjectedAtTime(tm().getTransactionTime()));
               if (!resaveTimes.isEmpty()) {
                 asyncTaskEnqueuer.enqueueAsyncResave(
                     VKey.createEppVKeyFromString(resourceKey), requestedTime, resaveTimes);
diff --git a/core/src/test/java/google/registry/batch/ResaveEntityActionTest.java b/core/src/test/java/google/registry/batch/ResaveEntityActionTest.java
index 48b2d5b2c..f2fbfd09d 100644
--- a/core/src/test/java/google/registry/batch/ResaveEntityActionTest.java
+++ b/core/src/test/java/google/registry/batch/ResaveEntityActionTest.java
@@ -20,6 +20,7 @@ import static google.registry.batch.AsyncTaskEnqueuer.PARAM_RESOURCE_KEY;
 import static google.registry.batch.AsyncTaskEnqueuer.QUEUE_ASYNC_ACTIONS;
 import static google.registry.testing.DatabaseHelper.createTld;
 import static google.registry.testing.DatabaseHelper.loadByEntity;
+import static google.registry.testing.DatabaseHelper.newDomain;
 import static google.registry.testing.DatabaseHelper.persistActiveContact;
 import static google.registry.testing.DatabaseHelper.persistDomainWithDependentResources;
 import static google.registry.testing.DatabaseHelper.persistDomainWithPendingTransfer;
@@ -39,7 +40,6 @@ import google.registry.persistence.transaction.JpaTestExtensions.JpaIntegrationT
 import google.registry.request.Response;
 import google.registry.testing.CloudTasksHelper;
 import google.registry.testing.CloudTasksHelper.TaskMatcher;
-import google.registry.testing.DatabaseHelper;
 import google.registry.testing.FakeClock;
 import org.joda.time.DateTime;
 import org.junit.jupiter.api.BeforeEach;
@@ -107,7 +107,7 @@ public class ResaveEntityActionTest {
 
   @Test
   void test_domainPendingDeletion_isResavedAndReenqueued() {
-    Domain newDomain = DatabaseHelper.newDomain("domain.tld");
+    Domain newDomain = newDomain("domain.tld");
     Domain domain =
         persistResource(
             newDomain
@@ -144,4 +144,16 @@ public class ResaveEntityActionTest {
             .param(PARAM_REQUESTED_TIME, requestedTime.toString())
             .scheduleTime(clock.nowUtc().plus(standardDays(5))));
   }
+
+  @Test
+  void test_queuedTaskForNonExistentDomain_failsPermanently() {
+    DateTime requestedTime = clock.nowUtc();
+    // It should complete its run without throwing an exception (that would cause a retry) ...
+    runAction(
+        newDomain("nonexistent.tld").createVKey().stringify(),
+        requestedTime,
+        ImmutableSortedSet.of(requestedTime.plusDays(5)));
+    // ... and it shouldn't enqueue the subsequent re-save 5 days later.
+    cloudTasksHelper.assertNoTasksEnqueued(QUEUE_ASYNC_ACTIONS);
+  }
 }