Terminate non-detached test devices on flutter run completion (#159170)

See https://github.com/flutter/flutter/issues/159154.
See https://github.com/flutter/flutter/pull/159169.

Before this PR, it appeared we were accidentally leaking (keeping
active) `flutter_tester` instances (or any test device) after `flutter
run` completion, even if the runner was not explicitly detached. I
_think_ this is a bug, but I'll check with the tools team and possibly
@jonahwilliams before finalizing this.

/cc @jason-simmons

---------

Co-authored-by: Andrew Kolos <andrewrkolos@gmail.com>

packages/flutter_tools/lib/src/resident_runner.dart

@@ -1118,6 +1118,16 @@ abstract class ResidentRunner extends ResidentHandlers {
     return 'main.dart${swap ? '.swap' : ''}.dill';
   }
 
+  /// Whether the app being instrumented by the runner should be stopped during
+  /// cleanup.
+  ///
+  /// A detached app can happen one of two ways:
+  /// - [run] is used, and then the created application is manually [detach]ed;
+  /// - [attach] is used to explicitly connect to an already running app.
+  @protected
+  @visibleForTesting
+  bool stopAppDuringCleanup = true;
+
   bool get debuggingEnabled => debuggingOptions.debuggingEnabled;
 
   @override
@@ -1254,7 +1264,10 @@ abstract class ResidentRunner extends ResidentHandlers {
   }
 
   @override
+  @mustCallSuper
   Future<void> detach() async {
+    stopAppDuringCleanup = false;
+
     // TODO(bkonyi): remove when ready to serve DevTools from DDS.
     await residentDevtoolsHandler!.shutdown();
     await stopEchoingDeviceLog();
@@ -1398,6 +1411,7 @@ abstract class ResidentRunner extends ResidentHandlers {
     }
   }
 
+  @protected
   void appFinished() {
     if (_finished.isCompleted) {
       return;

packages/flutter_tools/lib/src/run_hot.dart

@@ -125,9 +125,6 @@ class HotRunner extends ResidentRunner {
   /// reload process do not have this issue.
   bool _swap = false;
 
-  /// Whether the resident runner has correctly attached to the running application.
-  bool _didAttach = false;
-
   final Map<String, List<int>> benchmarkData = <String, List<int>>{};
 
   String? _targetPlatform;
@@ -220,15 +217,29 @@ class HotRunner extends ResidentRunner {
     throw Exception('Failed to compile $expression');
   }
 
-  // Returns the exit code of the flutter tool process, like [run].
   @override
+  @nonVirtual
   Future<int> attach({
     Completer<DebugConnectionInfo>? connectionInfoCompleter,
     Completer<void>? appStartedCompleter,
     bool allowExistingDdsInstance = false,
     bool needsFullRestart = true,
   }) async {
-    _didAttach = true;
+    stopAppDuringCleanup = false;
+    return _attach(
+      connectionInfoCompleter: connectionInfoCompleter,
+      appStartedCompleter: appStartedCompleter,
+      allowExistingDdsInstance: allowExistingDdsInstance,
+      needsFullRestart: needsFullRestart,
+    );
+  }
+
+  Future<int> _attach({
+    Completer<DebugConnectionInfo>? connectionInfoCompleter,
+    Completer<void>? appStartedCompleter,
+    bool allowExistingDdsInstance = false,
+    bool needsFullRestart = true,
+  }) async {
     try {
       await connectToServiceProtocol(
         reloadSources: _reloadSourcesService,
@@ -464,7 +475,7 @@ class HotRunner extends ResidentRunner {
       return 1;
     }
 
-    return attach(
+    return _attach(
       connectionInfoCompleter: connectionInfoCompleter,
       appStartedCompleter: appStartedCompleter,
       needsFullRestart: false,
@@ -1142,7 +1153,7 @@ class HotRunner extends ResidentRunner {
     } else {
       commandHelp.hWithoutDetails.print();
     }
-    if (_didAttach) {
+    if (stopAppDuringCleanup) {
       commandHelp.d.print();
     }
     commandHelp.c.print();
@@ -1239,11 +1250,10 @@ class HotRunner extends ResidentRunner {
   Future<void> cleanupAfterSignal() async {
     await stopEchoingDeviceLog();
     await hotRunnerConfig!.runPreShutdownOperations();
-    if (_didAttach) {
-      appFinished();
-    } else {
-      await exitApp();
+    if (stopAppDuringCleanup) {
+      return exitApp();
     }
+    appFinished();
   }
 
   @override

packages/flutter_tools/test/general.shard/hot_shared.dart

@@ -4,6 +4,7 @@
 
 import 'package:flutter_tools/src/application_package.dart';
 import 'package:flutter_tools/src/asset.dart';
+import 'package:flutter_tools/src/base/dds.dart';
 import 'package:flutter_tools/src/build_info.dart';
 import 'package:flutter_tools/src/build_system/tools/shader_compiler.dart';
 import 'package:flutter_tools/src/compile.dart';
@@ -52,6 +53,9 @@ class FakeDevice extends Fake implements Device {
 
   bool disposed = false;
 
+  @override
+  final DartDevelopmentService dds = _FakeDartDevelopmentService();
+
   @override
   bool isSupported() => true;
 
@@ -90,6 +94,11 @@ class FakeDevice extends Fake implements Device {
   }
 }
 
+class _FakeDartDevelopmentService extends Fake implements DartDevelopmentService {
+  @override
+  void shutdown() {}
+}
+
 class FakeFlutterDevice extends Fake implements FlutterDevice {
   FakeFlutterDevice(this.device);
 

packages/flutter_tools/test/general.shard/resident_runner_helpers.dart

@@ -146,6 +146,9 @@ class FakeDartDevelopmentService extends Fake with DartDevelopmentServiceLocalOp
 
   @override
   Uri? get uri => null;
+
+  @override
+  void shutdown() {}
 }
 
 class FakeDartDevelopmentServiceException implements DartDevelopmentServiceException {

packages/flutter_tools/test/general.shard/resident_runner_test.dart

@@ -96,6 +96,12 @@ void main() {
     expect(fakeVmServiceHost?.hasRemainingExpectations, false);
   }));
 
+  testUsingContext('ResidentRunner reports whether detach() was used', () => testbed.run(() async {
+    expect(residentRunner.stopAppDuringCleanup, true);
+    await residentRunner.detach();
+    expect(residentRunner.stopAppDuringCleanup, false);
+  }));
+
   testUsingContext('ResidentRunner suppresses errors for the initial compilation', () => testbed.run(() async {
     globals.fs.file(globals.fs.path.join('lib', 'main.dart'))
       .createSync(recursive: true);
@@ -1302,6 +1308,7 @@ flutter:
           commandHelp.M,
           commandHelp.g,
           commandHelp.hWithDetails,
+          commandHelp.d,
           commandHelp.c,
           commandHelp.q,
           '',
@@ -1331,6 +1338,7 @@ flutter:
           commandHelp.r,
           commandHelp.R,
           commandHelp.hWithoutDetails,
+          commandHelp.d,
           commandHelp.c,
           commandHelp.q,
           '',

packages/flutter_tools/test/general.shard/run_hot_test.dart

@@ -2,7 +2,12 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+import 'package:file/memory.dart';
+import 'package:flutter_tools/src/base/file_system.dart';
+import 'package:flutter_tools/src/build_info.dart';
+import 'package:flutter_tools/src/compile.dart';
 import 'package:flutter_tools/src/devfs.dart';
+import 'package:flutter_tools/src/device.dart';
 import 'package:flutter_tools/src/reporting/reporting.dart';
 import 'package:flutter_tools/src/resident_runner.dart';
 import 'package:flutter_tools/src/run_hot.dart';
@@ -11,11 +16,13 @@ import 'package:test/fake.dart';
 import 'package:unified_analytics/unified_analytics.dart';
 import 'package:vm_service/vm_service.dart' as vm_service;
 
-//import '../src/context.dart';
 import '../src/common.dart';
+import '../src/context.dart';
+import 'hot_shared.dart';
 
 void main() {
-  testWithoutContext('defaultReloadSourcesHelper() handles empty DeviceReloadReports)', () {
+  testWithoutContext(
+      'defaultReloadSourcesHelper() handles empty DeviceReloadReports)', () {
     defaultReloadSourcesHelper(
       _FakeHotRunner(),
       <FlutterDevice?>[_FakeFlutterDevice()],
@@ -29,6 +36,88 @@ void main() {
       const NoOpAnalytics(),
     );
   });
+
+  group('signal handling', () {
+    late _FakeHotCompatibleFlutterDevice flutterDevice;
+    late MemoryFileSystem fileSystem;
+
+    setUp(() {
+      flutterDevice = _FakeHotCompatibleFlutterDevice(FakeDevice());
+      fileSystem = MemoryFileSystem.test();
+    });
+
+    testUsingContext(
+      'kills the test device',
+      () async {
+        final HotRunner runner = HotRunner(
+          <FlutterDevice>[
+            flutterDevice,
+          ],
+          target: 'main.dart',
+          debuggingOptions: DebuggingOptions.disabled(BuildInfo.debug),
+          analytics: _FakeAnalytics(),
+        );
+
+        await runner.run();
+        await runner.cleanupAfterSignal();
+        expect(flutterDevice.wasExited, true);
+      },
+      overrides: <Type, Generator>{
+        FileSystem: () => fileSystem,
+        ProcessManager: FakeProcessManager.empty,
+      },
+    );
+
+    testUsingContext(
+      'kill with a detach keeps the test device running',
+      () async {
+        final HotRunner runner = HotRunner(
+          <FlutterDevice>[
+            flutterDevice,
+          ],
+          target: 'main.dart',
+          debuggingOptions: DebuggingOptions.disabled(BuildInfo.debug),
+          analytics: _FakeAnalytics(),
+        );
+
+        await runner.run();
+        await runner.detach();
+        await runner.cleanupAfterSignal();
+        expect(flutterDevice.wasExited, false);
+      },
+      overrides: <Type, Generator>{
+        FileSystem: () => fileSystem,
+        ProcessManager: FakeProcessManager.empty,
+      },
+    );
+
+    testUsingContext(
+      'kill on an attached device keeps the test device running',
+      () async {
+        final HotRunner runner = HotRunner(
+          <FlutterDevice>[
+            flutterDevice,
+          ],
+          target: 'main.dart',
+          debuggingOptions: DebuggingOptions.disabled(BuildInfo.debug),
+          analytics: _FakeAnalytics(),
+        );
+
+        await runner.attach();
+        await runner.cleanupAfterSignal();
+        expect(flutterDevice.wasExited, false);
+      },
+      overrides: <Type, Generator>{
+        FileSystem: () => fileSystem,
+        ProcessManager: FakeProcessManager.empty,
+      },
+    );
+  });
+}
+
+class _FakeAnalytics extends Fake implements Analytics {
+  @override
+  void send(Event event) {}
 }
 
 class _FakeHotRunner extends Fake implements HotRunner {}
@@ -37,6 +126,9 @@ class _FakeDevFS extends Fake implements DevFS {
   @override
   final Uri? baseUri = Uri();
 
+  @override
+  Future<void> destroy() async {}
+
   @override
   void resetLastCompiled() {}
 }
@@ -49,6 +141,36 @@ class _FakeFlutterDevice extends Fake implements FlutterDevice {
   final FlutterVmService? vmService = _FakeFlutterVmService();
 }
 
+class _FakeHotCompatibleFlutterDevice extends Fake implements FlutterDevice {
+  _FakeHotCompatibleFlutterDevice(this.device);
+
+  @override
+  final Device device;
+
+  @override
+  DevFS? devFS = _FakeDevFS();
+
+  @override
+  ResidentCompiler? get generator => null;
+
+  @override
+  Future<int> runHot({required HotRunner hotRunner, String? route}) async {
+    return 0;
+  }
+
+  @override
+  Future<void> stopEchoingDeviceLog() async {}
+
+  @override
+  Future<void> exitApps({
+    Duration timeoutDelay = const Duration(seconds: 10),
+  }) async {
+    wasExited = true;
+  }
+
+  bool wasExited = false;
+}
+
 class _FakeFlutterVmService extends Fake implements FlutterVmService {
   @override
   final vm_service.VmService service = _FakeVmService();