Services: Shut down services that were deleted between watch iterations

src/execution/service.ts

@@ -254,8 +254,13 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand<ServiceScri
         // fact that the promises remain unresolved will prevent GC of old
         // executions in watch mode. Those promises should probably be
         // Promise.race'd to prevent that.
-        child.stdout.removeAllListeners();
-        child.stderr.removeAllListeners();
+
+        // Note that for some reason, removing all listeners from stdout/stderr
+        // without specifying the "data" event will also remove the listeners
+        // directly on "child" inside the ScriptChildProceess for noticing when
+        // e.g. the process has exited.
+        child.stdout.removeAllListeners('data');
+        child.stderr.removeAllListeners('data');
         return child;
       }
       case 'stopping':

src/executor.ts

@@ -125,10 +125,32 @@ export class Executor {
    * Execute the root script.
    */
   async execute(): Promise<Result<ServiceMap, Failure[]>> {
-    // TOOD(aomarks) If we have any running services from a previous watch
-    // iteration, we should at this point shut down any of the ones that have
-    // since been deleted from the build graph entirely, or which have become
-    // non-directly-invoked.
+    if (
+      this._previousIterationServices !== undefined &&
+      this._previousIterationServices.size > 0
+    ) {
+      // If any services were removed from the graph entirely, or used to be
+      // directly invoked but are no longer, then stop them now.
+      const currentDirectlyInvokedServices = new Set<ScriptReferenceString>();
+      for (const script of findAllScripts(this._rootConfig)) {
+        if (script.service && script.isDirectlyInvoked) {
+          currentDirectlyInvokedServices.add(scriptReferenceToString(script));
+        }
+      }
+      const stopPromises = [];
+      for (const [key, service] of this._previousIterationServices) {
+        if (!currentDirectlyInvokedServices.has(key)) {
+          const child = service.detach();
+          if (child !== undefined) {
+            child.kill();
+            stopPromises.push(child.completed);
+          }
+          this._previousIterationServices.delete(key);
+        }
+      }
+      await Promise.all(stopPromises);
+    }
+
     const errors: Failure[] = [];
     const rootExecutionResult = await this.getExecution(
       this._rootConfig
@@ -215,3 +237,22 @@ export class Executor {
     return execution as ConfigToExecution<T>;
   }
 }
+
+/**
+ * Walk the dependencies of the given root script and return all scripts in the
+ * graph (including the root itself).
+ */
+function findAllScripts(root: ScriptConfig): Set<ScriptConfig> {
+  const visited = new Set<ScriptConfig>();
+  const stack = [root];
+  while (stack.length > 0) {
+    const next = stack.pop()!;
+    visited.add(next);
+    for (const dep of next.dependencies) {
+      if (!visited.has(dep.config)) {
+        stack.push(dep.config);
+      }
+    }
+  }
+  return visited;
+}

src/test/service.test.ts

@@ -665,4 +665,73 @@ test(
   })
 );
 
+test(
+  'deleted service shuts down between watch iterations',
+  timeout(async ({rig}) => {
+    //      entrypoint
+    //        /   \
+    //       v     v
+    // standard   service (gets deleted)
+
+    const standard = await rig.newCommand();
+    const service = await rig.newCommand();
+    await rig.writeAtomic({
+      'package.json': {
+        scripts: {
+          entrypoint: 'wireit',
+          standard: 'wireit',
+          service: 'wireit',
+        },
+        wireit: {
+          entrypoint: {
+            dependencies: ['standard', 'service'],
+          },
+          standard: {
+            command: standard.command,
+          },
+          service: {
+            command: service.command,
+            service: true,
+          },
+        },
+      },
+    });
+
+    // Iteration 1. Both scripts start.
+    const wireit = rig.exec('npm run entrypoint --watch');
+    const serviceInv = await service.nextInvocation();
+    const standardInv1 = await standard.nextInvocation();
+    standardInv1.exit(0);
+    await wireit.waitForLog(/Watching for file changes/);
+
+    // Iteration 2. We update the config to delete the service. It should get
+    // shut down.
+    await rig.writeAtomic({
+      'package.json': {
+        scripts: {
+          entrypoint: 'wireit',
+          standard: 'wireit',
+        },
+        wireit: {
+          entrypoint: {
+            dependencies: ['standard'],
+          },
+          standard: {
+            command: standard.command,
+          },
+        },
+      },
+    });
+    await serviceInv.closed;
+    const standardInv2 = await standard.nextInvocation();
+    standardInv2.exit(0);
+    await wireit.waitForLog(/Watching for file changes/);
+
+    wireit.kill();
+    await wireit.exit;
+    assert.equal(service.numInvocations, 1);
+    assert.equal(standard.numInvocations, 2);
+  })
+);
+
 test.run();