Add validation for concurrency deadlock detection (#237)

This adds an error when workflow-level and job-level concurrency groups match, which causes a deadlock at runtime. The job blocks waiting for the workflow to finish, while the workflow is waiting for the job to finish. - Detects both string and mapping forms of concurrency - Only errors on static string matches (expressions are not compared) - Case-sensitive comparison - Errors on both workflow-level and job-level with appropriate messages Fixes #135
2025-12-08 09:20:22 -06:00
parent 4cf3365c68
commit 78ea3ba17f
2 changed files with 317 additions and 1 deletions
@@ -0,0 +1,245 @@
+import {DiagnosticSeverity} from "vscode-languageserver-types";
+import {validate} from "./validate";
+import {createDocument} from "./test-utils/document";
+import {clearCache} from "./utils/workflow-cache";
+
+beforeEach(() => {
+  clearCache();
+});
+
+describe("validate concurrency deadlock", () => {
+  describe("should error on matching concurrency groups", () => {
+    it("simple string match", async () => {
+      const input = `
+on: push
+concurrency: test
+jobs:
+  job1:
+    runs-on: ubuntu-latest
+    concurrency: test
+    steps:
+      - run: echo hi`;
+
+      const result = await validate(createDocument("wf.yaml", input));
+
+      const concurrencyErrors = result.filter(d => d.message.includes("deadlock"));
+      expect(concurrencyErrors).toHaveLength(2);
+
+      // Workflow-level warning
+      expect(concurrencyErrors[0]).toMatchObject({
+        message: "Concurrency group 'test' is also used by job 'job1'. This will cause a deadlock.",
+        severity: DiagnosticSeverity.Error
+      });
+
+      // Job-level warning
+      expect(concurrencyErrors[1]).toMatchObject({
+        message: "Concurrency group 'test' is also defined at the workflow level. This will cause a deadlock.",
+        severity: DiagnosticSeverity.Error
+      });
+    });
+
+    it("workflow mapping form, job string form", async () => {
+      const input = `
+on: push
+concurrency:
+  group: my-group
+  cancel-in-progress: true
+jobs:
+  deploy:
+    runs-on: ubuntu-latest
+    concurrency: my-group
+    steps:
+      - run: echo hi`;
+
+      const result = await validate(createDocument("wf.yaml", input));
+
+      const concurrencyErrors = result.filter(d => d.message.includes("deadlock"));
+      expect(concurrencyErrors).toHaveLength(2);
+      expect(concurrencyErrors[0].message).toContain("my-group");
+      expect(concurrencyErrors[0].message).toContain("deploy");
+    });
+
+    it("workflow string form, job mapping form", async () => {
+      const input = `
+on: push
+concurrency: deploy-group
+jobs:
+  build:
+    runs-on: ubuntu-latest
+    concurrency:
+      group: deploy-group
+      cancel-in-progress: true
+    steps:
+      - run: echo hi`;
+
+      const result = await validate(createDocument("wf.yaml", input));
+
+      const concurrencyErrors = result.filter(d => d.message.includes("deadlock"));
+      expect(concurrencyErrors).toHaveLength(2);
+      expect(concurrencyErrors[0].message).toContain("deploy-group");
+    });
+
+    it("both mapping forms", async () => {
+      const input = `
+on: push
+concurrency:
+  group: shared
+jobs:
+  job1:
+    runs-on: ubuntu-latest
+    concurrency:
+      group: shared
+    steps:
+      - run: echo hi`;
+
+      const result = await validate(createDocument("wf.yaml", input));
+
+      const concurrencyErrors = result.filter(d => d.message.includes("deadlock"));
+      expect(concurrencyErrors).toHaveLength(2);
+    });
+
+    it("multiple jobs with matching concurrency", async () => {
+      const input = `
+on: push
+concurrency: shared
+jobs:
+  job1:
+    runs-on: ubuntu-latest
+    concurrency: shared
+    steps:
+      - run: echo hi
+  job2:
+    runs-on: ubuntu-latest
+    concurrency: shared
+    steps:
+      - run: echo hi`;
+
+      const result = await validate(createDocument("wf.yaml", input));
+
+      // Should have 2 warnings per job (workflow + job) = 4 total, but workflow is only warned once per match
+      // Actually: 1 workflow warning per matching job + 1 job warning per matching job = 4 total
+      const concurrencyErrors = result.filter(d => d.message.includes("deadlock"));
+      expect(concurrencyErrors).toHaveLength(4);
+    });
+  });
+
+  describe("should not warn", () => {
+    it("different concurrency groups", async () => {
+      const input = `
+on: push
+concurrency: workflow-group
+jobs:
+  job1:
+    runs-on: ubuntu-latest
+    concurrency: job-group
+    steps:
+      - run: echo hi`;
+
+      const result = await validate(createDocument("wf.yaml", input));
+
+      const concurrencyErrors = result.filter(d => d.message.includes("deadlock"));
+      expect(concurrencyErrors).toHaveLength(0);
+    });
+
+    it("workflow concurrency is an expression", async () => {
+      const input = `
+on: push
+concurrency: \${{ github.ref }}
+jobs:
+  job1:
+    runs-on: ubuntu-latest
+    concurrency: test
+    steps:
+      - run: echo hi`;
+
+      const result = await validate(createDocument("wf.yaml", input));
+
+      const concurrencyErrors = result.filter(d => d.message.includes("deadlock"));
+      expect(concurrencyErrors).toHaveLength(0);
+    });
+
+    it("job concurrency is an expression", async () => {
+      const input = `
+on: push
+concurrency: test
+jobs:
+  job1:
+    runs-on: ubuntu-latest
+    concurrency: \${{ github.ref }}
+    steps:
+      - run: echo hi`;
+
+      const result = await validate(createDocument("wf.yaml", input));
+
+      const concurrencyErrors = result.filter(d => d.message.includes("deadlock"));
+      expect(concurrencyErrors).toHaveLength(0);
+    });
+
+    it("no workflow-level concurrency", async () => {
+      const input = `
+on: push
+jobs:
+  job1:
+    runs-on: ubuntu-latest
+    concurrency: test
+    steps:
+      - run: echo hi`;
+
+      const result = await validate(createDocument("wf.yaml", input));
+
+      const concurrencyErrors = result.filter(d => d.message.includes("deadlock"));
+      expect(concurrencyErrors).toHaveLength(0);
+    });
+
+    it("no job-level concurrency", async () => {
+      const input = `
+on: push
+concurrency: test
+jobs:
+  job1:
+    runs-on: ubuntu-latest
+    steps:
+      - run: echo hi`;
+
+      const result = await validate(createDocument("wf.yaml", input));
+
+      const concurrencyErrors = result.filter(d => d.message.includes("deadlock"));
+      expect(concurrencyErrors).toHaveLength(0);
+    });
+
+    it("case sensitive - different case is different group", async () => {
+      const input = `
+on: push
+concurrency: Test
+jobs:
+  job1:
+    runs-on: ubuntu-latest
+    concurrency: test
+    steps:
+      - run: echo hi`;
+
+      const result = await validate(createDocument("wf.yaml", input));
+
+      const concurrencyErrors = result.filter(d => d.message.includes("deadlock"));
+      expect(concurrencyErrors).toHaveLength(0);
+    });
+
+    it("workflow concurrency group in mapping is an expression", async () => {
+      const input = `
+on: push
+concurrency:
+  group: \${{ github.ref }}
+jobs:
+  job1:
+    runs-on: ubuntu-latest
+    concurrency: test
+    steps:
+      - run: echo hi`;
+
+      const result = await validate(createDocument("wf.yaml", input));
+
+      const concurrencyErrors = result.filter(d => d.message.includes("deadlock"));
+      expect(concurrencyErrors).toHaveLength(0);
+    });
+  });
+});
@@ -1,6 +1,6 @@
 import {Lexer, Parser, data} from "@actions/expressions";
 import {Expr, FunctionCall, Literal, Logical} from "@actions/expressions/ast";
-import {ParseWorkflowResult, WorkflowTemplate, isBasicExpression, isString} from "@actions/workflow-parser";
+import {ParseWorkflowResult, WorkflowTemplate, isBasicExpression, isMapping, isString} from "@actions/workflow-parser";
 import {ErrorPolicy} from "@actions/workflow-parser/model/convert";
 import {getCronDescription, hasCronIntervalLessThan5Minutes} from "@actions/workflow-parser/model/converter/cron";
 import {ensureStatusFunction} from "@actions/workflow-parser/model/converter/if-condition";
@@ -209,6 +209,9 @@ async function additionalValidations(
      }
    }
  }
+
+  // Validate concurrency deadlock between workflow and job levels
+  validateConcurrencyDeadlock(diagnostics, template);
 }

 function invalidValue(diagnostics: Diagnostic[], token: StringToken, kind: ValueProviderKind) {
@@ -712,3 +715,71 @@ async function validateExpression(
    );
  }
 }
+
+/**
+ * Validates that workflow-level and job-level concurrency groups don't match,
+ * which would cause a deadlock at runtime.
+ */
+function validateConcurrencyDeadlock(diagnostics: Diagnostic[], template: WorkflowTemplate): void {
+  const workflowGroup = getStaticConcurrencyGroup(template.concurrency);
+  if (!workflowGroup) {
+    return; // No workflow-level concurrency or it's an expression
+  }
+
+  for (const job of template.jobs || []) {
+    if (!job.concurrency) {
+      continue;
+    }
+
+    const jobGroup = getStaticConcurrencyGroup(job.concurrency);
+    if (!jobGroup) {
+      continue; // Job concurrency is an expression
+    }
+
+    if (workflowGroup.value === jobGroup.value) {
+      // Error on workflow-level concurrency
+      if (template.concurrency.range) {
+        diagnostics.push({
+          message: `Concurrency group '${workflowGroup.value}' is also used by job '${job.id.value}'. This will cause a deadlock.`,
+          range: mapRange(template.concurrency.range),
+          severity: DiagnosticSeverity.Error
+        });
+      }
+
+      // Error on job-level concurrency
+      if (job.concurrency.range) {
+        diagnostics.push({
+          message: `Concurrency group '${jobGroup.value}' is also defined at the workflow level. This will cause a deadlock.`,
+          range: mapRange(job.concurrency.range),
+          severity: DiagnosticSeverity.Error
+        });
+      }
+    }
+  }
+}
+
+/**
+ * Extracts the static concurrency group name from a concurrency token.
+ * Returns undefined if the token is an expression or doesn't have a static group.
+ */
+function getStaticConcurrencyGroup(token: TemplateToken | undefined): StringToken | undefined {
+  if (!token || token.isExpression) {
+    return undefined;
+  }
+
+  // Simple string form: concurrency: "test"
+  if (isString(token)) {
+    return token;
+  }
+
+  // Mapping form: concurrency: { group: "test", cancel-in-progress: true }
+  if (isMapping(token)) {
+    for (const pair of token) {
+      if (isString(pair.key) && pair.key.value === "group" && isString(pair.value) && !pair.value.isExpression) {
+        return pair.value;
+      }
+    }
+  }
+
+  return undefined;
+}