Properly record a new task for a new dart_internal build ID. (#4513)

I think this has been a bug for a while, but could have also been caused by the Firestore migration.

After this PR, `dart_internal` should show re-runs and previous builds like expected.

Closes flutter/flutter#166935.

/cc @reidbaker

Author: matanlurey

app_dart/lib/src/model/firestore/task.dart

@@ -373,9 +373,18 @@ final class Task extends AppDocument<Task> {
     _setStatusFromLuciStatus(build);
   }
 
-  void resetAsRetry({int attempt = 1}) {
-    name =
-        '$kDatabase/documents/$kTaskCollectionId/${commitSha}_${taskName}_$attempt';
+  void resetAsRetry({int? attempt}) {
+    attempt ??= currentAttempt + 1;
+    name = p.posix.join(
+      kDatabase,
+      'documents',
+      kTaskCollectionId,
+      Task.documentIdFor(
+        commitSha: commitSha,
+        currentAttempt: attempt,
+        taskName: taskName,
+      ).documentId,
+    );
     fields = <String, Value>{
       fieldCreateTimestamp: DateTime.now().millisecondsSinceEpoch.toValue(),
       fieldEndTimestamp: 0.toValue(),
@@ -389,6 +398,10 @@ final class Task extends AppDocument<Task> {
     };
   }
 
+  void setBuildNumber(int buildNumber) {
+    fields[fieldBuildNumber] = buildNumber.toValue();
+  }
+
   String _setStatusFromLuciStatus(bbv2.Build build) {
     // Updates can come out of order. Ensure completed statuses are kept.
     if (_isStatusCompleted()) {

app_dart/lib/src/request_handlers/dart_internal_subscription.dart

@@ -53,6 +53,10 @@ final class DartInternalSubscription extends SubscriptionHandler {
       );
       if (existing != null) {
         fsTask = existing;
+        if (build.number != fsTask.buildNumber) {
+          fsTask.resetAsRetry();
+          fsTask.setBuildNumber(build.number);
+        }
         fsTask.updateFromBuild(build);
       } else {
         fsTask = fs.Task(

app_dart/lib/src/service/luci_build_service.dart

@@ -1108,8 +1108,7 @@ class LuciBuildService {
     fs.Task task,
   ) async {
     // Update task status in Firestore.
-    final newAttempt = task.currentAttempt + 1;
-    task.resetAsRetry(attempt: newAttempt);
+    task.resetAsRetry();
     task.setStatus(fs.Task.statusInProgress);
 
     final firestore = await _config.createFirestoreService();
@@ -1132,11 +1131,11 @@ class LuciBuildService {
       name: task.taskName,
     );
     dsExistingTask
-      ..attempts = newAttempt
+      ..attempts = task.currentAttempt
       ..status = Task.statusInProgress;
     await datastore.insert([dsExistingTask]);
 
-    return newAttempt;
+    return task.currentAttempt;
   }
 
   /// Check if a builder should be rerun.

app_dart/test/model/firestore/task_test.dart

@@ -117,7 +117,7 @@ void main() {
         status: Task.statusFailed,
         testFlaky: true,
       );
-      task.resetAsRetry(attempt: 2);
+      task.resetAsRetry();
 
       expect(int.parse(task.name!.split('_').last), 2);
       expect(task.status, Task.statusNew);

app_dart/test/request_handlers/dart_internal_subscription_test.dart

@@ -187,4 +187,62 @@ void main() {
           .having((t) => t.status, 'status', 'Succeeded'),
     ]);
   });
+
+  test('records a retry of an existing task', () async {
+    // Insert into Datastore:
+    await config.db.commit(
+      inserts: [
+        generateTask(
+          1,
+          attempts: 1,
+          buildNumber: buildNumber - 1,
+          builderName: builder,
+          name: builder,
+          status: fs.Task.statusFailed,
+          parent: dsCommit,
+        ),
+      ],
+    );
+
+    // Insert into Firestore:
+    firestoreService.putDocument(
+      generateFirestoreTask(
+        1,
+        attempts: 1,
+        buildNumber: buildNumber - 1,
+        name: builder,
+        status: fs.Task.statusFailed,
+        commitSha: dsCommit.sha,
+      ),
+    );
+
+    tester.message = const PushMessage(data: buildMessageJson);
+    await tester.post(handler);
+
+    // Check Firestore:
+    expect(
+      firestoreService,
+      existsInStorage(fs.Task.metadata, [
+        isTask
+            .hasTaskName(builder)
+            .hasCurrentAttempt(1)
+            .hasBuildNumber(buildNumber - 1)
+            .hasStatus(fs.Task.statusFailed),
+        isTask
+            .hasTaskName(builder)
+            .hasCurrentAttempt(2)
+            .hasBuildNumber(buildNumber)
+            .hasStatus(fs.Task.statusSucceeded),
+      ]),
+    );
+
+    // Check Datastore:
+    expect(config.db.values.values.whereType<ds.Task>(), [
+      isA<ds.Task>()
+          .having((t) => t.builderName, 'builderName', builder)
+          .having((t) => t.attempts, 'attempts', 2)
+          .having((t) => t.buildNumber, 'buildNumber', buildNumber)
+          .having((t) => t.status, 'status', 'Succeeded'),
+    ]);
+  });
 }