Services: Continue to show output of stale services during watch mode shutdown

src/execution/service.ts

@@ -243,6 +243,10 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand<ServiceScri
     }
   }
 
+  /**
+   * Take over ownership of this service's running child process, if there is
+   * one.
+   */
   detach(): ScriptChildProcess | undefined {
     switch (this._state.id) {
       case 'started': {
@@ -302,7 +306,7 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand<ServiceScri
           ? Promise.all([this._state.entireExecutionAborted, allConsumersDone])
           : allConsumersDone;
         void abort.then(() => {
-          this._onAbort();
+          void this.abort();
         });
 
         this._state = {
@@ -414,20 +418,16 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand<ServiceScri
           adoptee?.fingerprint !== undefined &&
           !adoptee.fingerprint.equal(fingerprint)
         ) {
-          const child = adoptee.detach();
-          if (child !== undefined) {
-            // There is a previous running version of this service, but the
-            // fingerprint changed, so we need to restart it.
-            this._state = {
-              id: 'stoppingAdoptee',
-              fingerprint,
-              deferredFingerprint: this._state.deferredFingerprint,
-            };
-            child.kill();
-            void child.completed.then(() => {
-              this._onAdopteeStopped();
-            });
-          }
+          // There is a previous running version of this service, but the
+          // fingerprint changed, so we need to restart it.
+          this._state = {
+            id: 'stoppingAdoptee',
+            fingerprint,
+            deferredFingerprint: this._state.deferredFingerprint,
+          };
+          void adoptee.abort().then(() => {
+            this._onAdopteeStopped();
+          });
           return;
         }
         this._state.deferredFingerprint.resolve({
@@ -724,16 +724,20 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand<ServiceScri
     }
   }
 
-  private _onAbort() {
+  /**
+   * Stop this service if it has started, and return a promise that resolves
+   * when it is stopped.
+   */
+  abort(): Promise<void> {
     switch (this._state.id) {
       case 'started': {
         this._state.child.kill();
         this._state = {id: 'stopping'};
-        return;
+        break;
       }
       case 'starting': {
         this._state = {id: 'stopping'};
-        return;
+        break;
       }
       case 'initial':
       case 'executingDeps':
@@ -742,19 +746,20 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand<ServiceScri
       case 'unstarted':
       case 'depsStarting': {
         this._enterStoppedState();
-        return;
+        break;
       }
       case 'stopping':
       case 'stopped':
       case 'failing':
       case 'failed':
       case 'detached': {
-        return;
+        break;
       }
       default: {
         throw unknownState(this._state);
       }
     }
+    return this._terminated.promise.then(() => undefined);
   }
 
   private _enterStoppedState() {

src/executor.ts

@@ -137,18 +137,14 @@ export class Executor {
           currentPersistentServices.add(scriptReferenceToString(script));
         }
       }
-      const stopPromises = [];
+      const abortPromises = [];
       for (const [key, service] of this._previousIterationServices) {
         if (!currentPersistentServices.has(key)) {
-          const child = service.detach();
-          if (child !== undefined) {
-            child.kill();
-            stopPromises.push(child.completed);
-          }
+          abortPromises.push(service.abort());
           this._previousIterationServices.delete(key);
         }
       }
-      await Promise.all(stopPromises);
+      await Promise.all(abortPromises);
     }
 
     const errors: Failure[] = [];

src/test/service.test.ts

@@ -720,6 +720,7 @@ test(
 
     // Iteration 2. We update the config to delete the service. It should get
     // shut down.
+    const serviceSigint = IS_WINDOWS ? undefined : serviceInv.interceptSigint();
     await rig.writeAtomic({
       'package.json': {
         scripts: {
@@ -736,6 +737,18 @@ test(
         },
       },
     });
+    if (!IS_WINDOWS) {
+      // Ensure that we continue to forward stdout/stderr while a stale service
+      // is being stopped. This won't be the case if we naively detach from the
+      // first execution, since then we'd stop listening for the output event
+      // listeners. Note we don't get graceful shutdown in Windows, so just skip
+      // this in Windows.
+      await serviceSigint;
+      serviceInv.stdout('Service shutting down');
+      await wireit.waitForLog(/Service shutting down/);
+      serviceInv.stdout('Service shutting down');
+      serviceInv.exit(0);
+    }
     await serviceInv.closed;
     const standardInv2 = await standard.nextInvocation();
     standardInv2.exit(0);