From 84e6ed0784c679c03ec6380a4aed6aa78eab2c24 Mon Sep 17 00:00:00 2001
From: Danny Tuppeny <danny@tuppeny.com>
Date: Wed, 19 Feb 2025 08:23:28 -0800
Subject: [PATCH] [dds/dap] Fix a race in DAP startup code that caused flaky
 tests

We start isolates paused so that we can send breakpoints before any code runs. This means we need to resume after initialization is complete.

It's important we don't try to resume multiple times during initialization (regardless of the order of isolate events or whether the isolate was found when we queried for isolates during connection).. This is done with the flag `startupHandled`.

One code path was not taking this flag into account, which meant multiple resumes were still possible. This seemed to occur on Linux during test runs (I've noticed the order of events being different on Linux in the past).

This change extracts the checking of `startupHandled` before sending `readyToResume` and uses it in all places that handle this kind of startup resume.

Fixes https://github.com/dart-lang/sdk/issues/60128

Change-Id: Ie2679fc806ab3edf007259298da82dbc8b802a6f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/410760
Reviewed-by: Ben Konyi <bkonyi@google.com>
Commit-Queue: Derek Xu <derekx@google.com>
Commit-Queue: Ben Konyi <bkonyi@google.com>
Reviewed-by: Derek Xu <derekx@google.com>
---
 pkg/dds/lib/src/dap/adapters/dart.dart   |  6 ++--
 pkg/dds/lib/src/dap/isolate_manager.dart | 36 ++++++++++++++++--------
 2 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/pkg/dds/lib/src/dap/adapters/dart.dart b/pkg/dds/lib/src/dap/adapters/dart.dart
index b838cb05203..908e2e598ee 100644
--- a/pkg/dds/lib/src/dap/adapters/dart.dart
+++ b/pkg/dds/lib/src/dap/adapters/dart.dart
@@ -832,7 +832,8 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
       final pauseEventKind = isolate.runnable ?? false
           ? vm.EventKind.kIsolateRunnable
           : vm.EventKind.kIsolateStart;
-      await isolateManager.registerIsolate(isolate, pauseEventKind);
+      final thread =
+          await isolateManager.registerIsolate(isolate, pauseEventKind);
 
       // If the Isolate already has a Pause event we can give it to the
       // IsolateManager to handle (if it's PausePostStart it will re-configure
@@ -844,7 +845,8 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
           isolate.pauseEvent!,
         );
       } else if (isolate.runnable == true) {
-        await isolateManager.readyToResumeIsolate(isolate);
+        await isolateManager.handleThreadStartup(thread,
+            sendStoppedOnEntry: false);
       }
     }));
   }
diff --git a/pkg/dds/lib/src/dap/isolate_manager.dart b/pkg/dds/lib/src/dap/isolate_manager.dart
index a38cd1303c7..d34f1c359da 100644
--- a/pkg/dds/lib/src/dap/isolate_manager.dart
+++ b/pkg/dds/lib/src/dap/isolate_manager.dart
@@ -640,18 +640,9 @@ class IsolateManager {
     // after a hot restart.
     if (eventKind == vm.EventKind.kPausePostRequest) {
       await _configureIsolate(thread);
-      await readyToResumeThread(thread.threadId);
+      await handleThreadStartup(thread, sendStoppedOnEntry: false);
     } else if (eventKind == vm.EventKind.kPauseStart) {
-      // Don't resume from a PauseStart if this has already happened (see
-      // comments on [thread.hasBeenStarted]).
-      if (!thread.startupHandled) {
-        thread.startupHandled = true;
-        // Send a Stopped event to inform the client UI the thread is paused and
-        // declare that we are ready to resume (which might result in an
-        // immediate resume).
-        sendStoppedOnEntryEvent(thread);
-        await readyToResumeThread(thread.threadId);
-      }
+      handleThreadStartup(thread, sendStoppedOnEntry: true);
     } else {
       // PauseExit, PauseBreakpoint, PauseInterrupted, PauseException
       var reason = 'pause';
@@ -719,6 +710,29 @@ class IsolateManager {
     }
   }
 
+  /// Handles thread startup if it has not already been handled.
+  ///
+  /// This includes sending Stopped-on-Entry and sending a readyToResume.
+  Future<void> handleThreadStartup(
+    ThreadInfo thread, {
+    required bool sendStoppedOnEntry,
+  }) async {
+    // Don't resume from a PauseStart if this has already happened (see
+    // comments on [thread.startupHandled]).
+    if (thread.startupHandled) {
+      return;
+    }
+
+    thread.startupHandled = true;
+    // Send a Stopped event to inform the client UI the thread is paused and
+    // declare that we are ready to resume (which might result in an
+    // immediate resume).
+    if (sendStoppedOnEntry) {
+      sendStoppedOnEntryEvent(thread);
+    }
+    await readyToResumeThread(thread.threadId);
+  }
+
   /// Handles a resume event from the VM, updating our local state.
   void _handleResumed(vm.Event event) {
     final isolate = event.isolate!;