fix(core): improved circular dependency detection

We now detect circular dependencies in `ConfigGraph` after the
graph has been augmented, possibly catching more circular
dependencies.

Also added circular task dependency detection to `TaskGraph`.

We also use the new `DependencyValidationGraph` class everywhere
we need to detect circular dependencies, e.g. when detecting
circular plugin/plugin module dependencies.

garden-service/src/config-graph.ts

@@ -18,7 +18,11 @@ import { TestConfig } from "./config/test"
 import { uniqByName, pickKeys } from "./util/util"
 import { ConfigurationError } from "./exceptions"
 import { deline } from "./util/string"
-import { validateDependencies } from "./util/validate-dependencies"
+import {
+  detectMissingDependencies,
+  handleDependencyErrors,
+  DependencyValidationGraph,
+} from "./util/validate-dependencies"
 import { ServiceConfig } from "./config/service"
 import { TaskConfig } from "./config/task"
 import { makeTestTaskName } from "./tasks/helpers"
@@ -69,14 +73,16 @@ interface EntityConfigEntry<T extends string, C extends EntityConfig> {
   config: C
 }
 
+export type DependencyGraph = { [key: string]: DependencyGraphNode }
+
 /**
  * A graph data structure that facilitates querying (recursive or non-recursive) of the project's dependency and
  * dependant relationships.
  *
  * This should be initialized with fully resolved and validated ModuleConfigs.
  */
 export class ConfigGraph {
-  private dependencyGraph: { [key: string]: DependencyGraphNode }
+  private dependencyGraph: DependencyGraph
   private moduleConfigs: { [key: string]: ModuleConfig }
 
   private serviceConfigs: {
@@ -164,7 +170,11 @@ export class ConfigGraph {
       }
     }
 
-    this.validateDependencies()
+    const missingDepsError = detectMissingDependencies(
+      Object.values(this.moduleConfigs),
+      Object.keys(this.serviceConfigs),
+      Object.keys(this.taskConfigs)
+    )
 
     // Add relations between nodes
     for (const moduleConfig of moduleConfigs) {
@@ -260,21 +270,28 @@ export class ConfigGraph {
         }
       }
     }
+
+    const validationGraph = DependencyValidationGraph.fromDependencyGraph(this.dependencyGraph)
+    const cycles = validationGraph.detectCircularDependencies()
+
+    let circularDepsError
+    if (cycles.length > 0) {
+      const description = validationGraph.cyclesToString(cycles)
+      const errMsg = `\nCircular dependencies detected: \n\n${description}\n`
+      circularDepsError = new ConfigurationError(errMsg, { "circular-dependencies": description })
+    } else {
+      circularDepsError = null
+    }
+
+    // Throw an error if one or both of these errors is non-null.
+    handleDependencyErrors(missingDepsError, circularDepsError)
   }
 
   // Convenience method used in the constructor above.
   keyForModule(config: ModuleConfig | BuildDependencyConfig) {
     return getModuleKey(config.name, config.plugin)
   }
 
-  private validateDependencies() {
-    validateDependencies(
-      Object.values(this.moduleConfigs),
-      Object.keys(this.serviceConfigs),
-      Object.keys(this.taskConfigs)
-    )
-  }
-
   private addRuntimeRelation(node: DependencyGraphNode, depName: string) {
     const dep = this.serviceConfigs[depName] || this.taskConfigs[depName]
 
@@ -729,7 +746,7 @@ export class DependencyGraphNode {
  * Note: If type === "build", name should be a prefix-qualified module name, as
  * returned by keyForModule or getModuleKey.
  */
-function nodeKey(type: DependencyGraphNodeType, name: string) {
+export function nodeKey(type: DependencyGraphNodeType, name: string) {
   return `${type}.${name}`
 }
 

garden-service/src/garden.ts

@@ -7,6 +7,10 @@
  */
 
 import Bluebird from "bluebird"
+import chalk from "chalk"
+import { ensureDir } from "fs-extra"
+import dedent from "dedent"
+import { platform, arch } from "os"
 import { parse, relative, resolve, dirname } from "path"
 import { flatten, isString, sortBy, fromPairs, keyBy } from "lodash"
 const AsyncLock = require("async-lock")
@@ -37,7 +41,6 @@ import { ModuleConfigContext, OutputConfigContext } from "./config/config-contex
 import { createPluginContext, CommandInfo } from "./plugin-context"
 import { ModuleAndRuntimeActionHandlers, RegisterPluginParam } from "./types/plugin/plugin"
 import { SUPPORTED_PLATFORMS, SupportedPlatform, DEFAULT_GARDEN_DIR_NAME } from "./constants"
-import { platform, arch } from "os"
 import { LogEntry } from "./logger/log-entry"
 import { EventBus } from "./events"
 import { Watcher } from "./watch"
@@ -52,14 +55,11 @@ import {
 import { Provider, ProviderConfig, getAllProviderDependencyNames, defaultProvider } from "./config/provider"
 import { ResolveProviderTask } from "./tasks/resolve-provider"
 import { ActionRouter } from "./actions"
-import { detectCycles, cyclesToString, Dependency } from "./util/validate-dependencies"
-import chalk from "chalk"
 import { RuntimeContext } from "./runtime-context"
-import { ensureDir } from "fs-extra"
 import { loadPlugins, getDependencyOrder, getModuleTypes } from "./plugins"
 import { deline, naturalList } from "./util/string"
-import dedent from "dedent"
 import { ensureConnected } from "./db/connection"
+import { DependencyValidationGraph } from "./util/validate-dependencies"
 
 export interface ActionHandlerMap<T extends keyof PluginActionHandlers> {
   [actionName: string]: PluginActionHandlers[T]
@@ -475,25 +475,26 @@ export class Garden {
 
       const plugins = keyBy(await this.getPlugins(), "name")
 
-      // Detect circular deps here
-      const pluginDeps: Dependency[] = []
+      // Detect circular dependencies here
+      const validationGraph = new DependencyValidationGraph()
 
       await Bluebird.map(rawConfigs, async (config) => {
         const plugin = plugins[config.name]
+        validationGraph.addNode(plugin.name)
 
         for (const dep of await getAllProviderDependencyNames(plugin!, config!)) {
-          pluginDeps.push({ from: config!.name, to: dep })
+          validationGraph.addNode(dep)
+          validationGraph.addDependency(plugin.name, dep)
         }
       })
 
-      const cycles = detectCycles(pluginDeps)
+      const cycles = validationGraph.detectCircularDependencies()
 
       if (cycles.length > 0) {
-        const cyclesStr = cyclesToString(cycles)
-
+        const description = validationGraph.cyclesToString(cycles)
         throw new PluginError(
-          "One or more circular dependencies found between providers or their configurations: " + cyclesStr,
-          { cycles }
+          `One or more circular dependencies found between providers or their configurations:\n\n${description}`,
+          { "circular-dependencies": description }
         )
       }
 

garden-service/src/plugins.ts

@@ -6,7 +6,6 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/.
  */
 
-import { DepGraph } from "dependency-graph"
 import {
   PluginMap,
   GardenPlugin,
@@ -19,9 +18,10 @@ import { ProviderConfig } from "./config/provider"
 import { ConfigurationError, PluginError, RuntimeError } from "./exceptions"
 import { uniq, mapValues, fromPairs, flatten, keyBy, some } from "lodash"
 import { findByName, pushToKey, getNames } from "./util/util"
-import { deline, naturalList } from "./util/string"
+import { deline } from "./util/string"
 import { validateSchema } from "./config/validation"
 import { LogEntry } from "./logger/log-entry"
+import { DependencyValidationGraph } from "./util/validate-dependencies"
 
 export function loadPlugins(log: LogEntry, registeredPlugins: PluginMap, configs: ProviderConfig[]) {
   const loadedPlugins: PluginMap = {}
@@ -108,7 +108,7 @@ export function loadPlugins(log: LogEntry, registeredPlugins: PluginMap, configs
  * Returns the given provider configs in dependency order.
  */
 export function getDependencyOrder<T extends ProviderConfig>(configs: T[], registeredPlugins: PluginMap): T[] {
-  let graph = new DepGraph()
+  const graph = new DependencyValidationGraph()
 
   for (const plugin of Object.values(registeredPlugins)) {
     graph.addNode(plugin.name)
@@ -124,20 +124,16 @@ export function getDependencyOrder<T extends ProviderConfig>(configs: T[], regis
     }
   }
 
-  let ordered: string[]
+  const cycles = graph.detectCircularDependencies()
 
-  try {
-    ordered = graph.overallOrder()
-  } catch (err) {
-    if (err.cyclePath) {
-      throw new PluginError(`Found a circular dependency between registered plugins: ${err.cyclePath.join(" -> ")}`, {
-        cyclePath: err.cyclePath,
-      })
-    } else {
-      throw err
-    }
+  if (cycles.length > 0) {
+    const description = graph.cyclesToString(cycles)
+    const detail = { "circular-dependencies": description }
+    throw new PluginError(`Found a circular dependency between registered plugins:\n\n${description}`, detail)
   }
 
+  const ordered = graph.overallOrder()
+
   // Note: concat() makes sure we're not mutating the original array, because JS...
   return configs.concat().sort((a, b) => {
     return ordered.indexOf(a.name) - ordered.indexOf(b.name)
@@ -316,18 +312,18 @@ interface ModuleDefinitionMap {
 
 function resolveModuleDefinitions(resolvedPlugins: PluginMap, configs: ProviderConfig[]): PluginMap {
   // Collect module type declarations
-  const graph = new DepGraph()
+  const graph = new DependencyValidationGraph()
   const moduleDefinitionMap: { [moduleType: string]: { plugin: GardenPlugin; spec: ModuleTypeDefinition }[] } = {}
   const moduleExtensionMap: { [moduleType: string]: { plugin: GardenPlugin; spec: ModuleTypeExtension }[] } = {}
 
   for (const plugin of Object.values(resolvedPlugins)) {
     for (const spec of plugin.createModuleTypes) {
       pushToKey(moduleDefinitionMap, spec.name, { plugin, spec })
 
-      graph.addNode(spec.name)
+      graph.addNode(spec.name, `${spec.name} (from plugin ${plugin.name})`)
 
       if (spec.base) {
-        graph.addNode(spec.base)
+        graph.addNode(spec.base, `${spec.base} (from plugin ${plugin.name})`)
         graph.addDependency(spec.name, spec.base)
       }
     }
@@ -352,27 +348,16 @@ function resolveModuleDefinitions(resolvedPlugins: PluginMap, configs: ProviderC
   }
 
   // Make sure we don't have circular dependencies in module type bases
-  let ordered: string[]
-
-  try {
-    ordered = graph.overallOrder()
-  } catch (err) {
-    if (err.cyclePath) {
-      const plugins: string[] = err.cyclePath.map(
-        (name: string) => "'" + moduleDefinitionMap[name][0].plugin.name + "'"
-      )
+  const cycles = graph.detectCircularDependencies()
 
-      throw new PluginError(
-        `Found circular dependency between module type bases (defined in plugin(s) ${naturalList(uniq(plugins))}): ` +
-          err.cyclePath.join(" -> "),
-        { cyclePath: err.cyclePath }
-      )
-    } else {
-      throw err
-    }
+  if (cycles.length > 0) {
+    const description = graph.cyclesToString(cycles)
+    const detail = { "circular-dependencies": description }
+    const msg = `Found circular dependency between module type bases:\n\n${description}`
+    throw new PluginError(msg, detail)
   }
 
-  ordered = ordered.filter((name) => name in moduleDefinitionMap)
+  const ordered = graph.overallOrder().filter((name) => name in moduleDefinitionMap)
 
   const resolvedDefinitions: ModuleDefinitionMap = {}
 

garden-service/src/task-graph.ts

@@ -14,11 +14,12 @@ import { every, flatten, intersection, merge, padEnd, union, uniqWith, without }
 import { BaseTask, TaskDefinitionError, TaskType } from "./tasks/base"
 
 import { LogEntry, LogEntryMetadata, TaskLogStatus } from "./logger/log-entry"
-import { toGardenError, GardenBaseError } from "./exceptions"
+import { toGardenError, GardenBaseError, InternalError } from "./exceptions"
 import { Garden } from "./garden"
 import { dedent } from "./util/string"
 import { defer, relationshipClasses, uuidv4 } from "./util/util"
 import { renderError } from "./logger/renderers"
+import { DependencyValidationGraph } from "./util/validate-dependencies"
 
 class TaskGraphError extends GardenBaseError {
   type = "task-graph"
@@ -84,7 +85,15 @@ export class TaskGraph {
 
   async process(tasks: BaseTask[], opts?: ProcessTasksOpts): Promise<TaskResults> {
     const unlimitedConcurrency = opts ? !!opts.unlimitedConcurrency : false
-    const nodes = await this.nodesWithDependencies(tasks, unlimitedConcurrency)
+
+    const validationGraph = new DependencyValidationGraph()
+
+    let nodes: TaskNode[]
+    try {
+      nodes = await this.nodesWithDependencies(tasks, validationGraph, unlimitedConcurrency)
+    } catch (circularDepsErr) {
+      throw circularDepsErr
+    }
 
     const batches = this.partition(nodes, { unlimitedConcurrency })
     for (const batch of batches) {
@@ -121,9 +130,28 @@ export class TaskGraph {
     return results
   }
 
-  async nodesWithDependencies(tasks: BaseTask[], unlimitedConcurrency = false): Promise<TaskNode[]> {
+  async nodesWithDependencies(
+    tasks: BaseTask[],
+    validationGraph: DependencyValidationGraph,
+    unlimitedConcurrency = false
+  ): Promise<TaskNode[]> {
     return Bluebird.map(tasks, async (task) => {
-      const depNodes = await this.nodesWithDependencies(await task.getDependencies(), unlimitedConcurrency)
+      const depTasks = await task.getDependencies()
+
+      // Detect circular dependencies
+      validationGraph.addNode(task.getKey())
+      for (const depTask of depTasks) {
+        validationGraph.addNode(depTask.getKey())
+        validationGraph.addDependency(task.getKey(), depTask.getKey())
+      }
+      const cycles = validationGraph.detectCircularDependencies()
+      if (cycles.length > 0) {
+        const description = validationGraph.cyclesToString(cycles)
+        const msg = `\nCircular task dependencies detected:\n\n${description}\n`
+        throw new InternalError(msg, { "circular-dependencies": description })
+      }
+
+      const depNodes = await this.nodesWithDependencies(depTasks, validationGraph, unlimitedConcurrency)
       return new TaskNode(task, depNodes, unlimitedConcurrency)
     })
   }