From 94c33eba7ed24fd3ee254ac06fb1d96c645cd9f8 Mon Sep 17 00:00:00 2001
From: Ben McIlwain <mcilwain@google.com>
Date: Mon, 14 Feb 2022 11:59:46 -0800
Subject: [PATCH] Make NordnUploadAction resilient to duplicate task queue
 tasks (#1516)

This is necessary because the Cloud Tasks API is not transactionally enrolled,
so it's possible that multiple tasks might end up being enqueued. We need to be
able to handle them.
---
 .../registry/tmch/NordnUploadAction.java      | 20 +++++++++++++++----
 .../registry/tmch/NordnUploadActionTest.java  | 15 +++++++++++++-
 2 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/core/src/main/java/google/registry/tmch/NordnUploadAction.java b/core/src/main/java/google/registry/tmch/NordnUploadAction.java
index df80682b2..b015b3ba9 100644
--- a/core/src/main/java/google/registry/tmch/NordnUploadAction.java
+++ b/core/src/main/java/google/registry/tmch/NordnUploadAction.java
@@ -39,8 +39,11 @@ import com.google.appengine.api.urlfetch.HTTPRequest;
 import com.google.appengine.api.urlfetch.HTTPResponse;
 import com.google.appengine.api.urlfetch.URLFetchService;
 import com.google.apphosting.api.DeadlineExceededException;
+import com.google.common.base.Joiner;
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSortedSet;
+import com.google.common.collect.Ordering;
 import com.google.common.flogger.FluentLogger;
 import google.registry.config.RegistryConfig.Config;
 import google.registry.request.Action;
@@ -125,15 +128,19 @@ public final class NordnUploadAction implements Runnable {
    * delimited String.
    */
   static String convertTasksToCsv(List<TaskHandle> tasks, DateTime now, String columns) {
-    String header = String.format("1,%s,%d\n%s\n", now, tasks.size(), columns);
-    StringBuilder csv = new StringBuilder(header);
+    // Use a Set for deduping purposes so we can be idempotent in case tasks happened to be
+    // enqueued multiple times for a given domain create.
+    ImmutableSortedSet.Builder<String> builder =
+        new ImmutableSortedSet.Builder<>(Ordering.natural());
     for (TaskHandle task : checkNotNull(tasks)) {
       String payload = new String(task.getPayload(), UTF_8);
       if (!Strings.isNullOrEmpty(payload)) {
-        csv.append(payload).append("\n");
+        builder.add(payload + '\n');
       }
     }
-    return csv.toString();
+    ImmutableSortedSet<String> csvLines = builder.build();
+    String header = String.format("1,%s,%d\n%s\n", now, csvLines.size(), columns);
+    return header + Joiner.on("").join(csvLines);
   }
 
   /** Leases and returns all tasks from the queue with the specified tag tld, in batches. */
@@ -168,6 +175,11 @@ public final class NordnUploadAction implements Runnable {
                 : LordnTaskUtils.QUEUE_CLAIMS);
     String columns = phase.equals(PARAM_LORDN_PHASE_SUNRISE) ? COLUMNS_SUNRISE : COLUMNS_CLAIMS;
     List<TaskHandle> tasks = loadAllTasks(queue, tld);
+    // Note: This upload/task deletion isn't done atomically (it's not clear how one would do so
+    // anyway). As a result, it is possible that the upload might succeed yet the deletion of
+    // enqueued tasks might fail. If so, this would result in the same lines being uploaded to NORDN
+    // across mulitple uploads. This is probably OK; all that we really cannot have is a missing
+    // line.
     if (!tasks.isEmpty()) {
       String csvData = convertTasksToCsv(tasks, now, columns);
       uploadCsvToLordn(String.format("/LORDN/%s/%s", tld, phase), csvData);
diff --git a/core/src/test/java/google/registry/tmch/NordnUploadActionTest.java b/core/src/test/java/google/registry/tmch/NordnUploadActionTest.java
index dc74eb286..86e7be307 100644
--- a/core/src/test/java/google/registry/tmch/NordnUploadActionTest.java
+++ b/core/src/test/java/google/registry/tmch/NordnUploadActionTest.java
@@ -138,13 +138,26 @@ class NordnUploadActionTest {
   void test_convertTasksToCsv() {
     List<TaskHandle> tasks =
         ImmutableList.of(
-            makeTaskHandle("task1", "example", "csvLine1", "lordn-sunrise"),
             makeTaskHandle("task2", "example", "csvLine2", "lordn-sunrise"),
+            makeTaskHandle("task1", "example", "csvLine1", "lordn-sunrise"),
             makeTaskHandle("task3", "example", "ending", "lordn-sunrise"));
     assertThat(NordnUploadAction.convertTasksToCsv(tasks, clock.nowUtc(), "col1,col2"))
         .isEqualTo("1,2010-05-01T10:11:12.000Z,3\ncol1,col2\ncsvLine1\ncsvLine2\nending\n");
   }
 
+  @MockitoSettings(strictness = Strictness.LENIENT)
+  @Test
+  void test_convertTasksToCsv_dedupesDuplicates() {
+    List<TaskHandle> tasks =
+        ImmutableList.of(
+            makeTaskHandle("task2", "example", "csvLine2", "lordn-sunrise"),
+            makeTaskHandle("task1", "example", "csvLine1", "lordn-sunrise"),
+            makeTaskHandle("task3", "example", "ending", "lordn-sunrise"),
+            makeTaskHandle("task1", "example", "csvLine1", "lordn-sunrise"));
+    assertThat(NordnUploadAction.convertTasksToCsv(tasks, clock.nowUtc(), "col1,col2"))
+        .isEqualTo("1,2010-05-01T10:11:12.000Z,3\ncol1,col2\ncsvLine1\ncsvLine2\nending\n");
+  }
+
   @MockitoSettings(strictness = Strictness.LENIENT)
   @Test
   void test_convertTasksToCsv_doesntFailOnEmptyTasks() {