From 78ea3ba17fe9576945282193ea02d7da3db6e605 Mon Sep 17 00:00:00 2001 From: eric sciple Date: Mon, 8 Dec 2025 09:20:22 -0600 Subject: [PATCH] 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 --- .../src/validate.concurrency.test.ts | 245 ++++++++++++++++++ languageservice/src/validate.ts | 73 +++++- 2 files changed, 317 insertions(+), 1 deletion(-) create mode 100644 languageservice/src/validate.concurrency.test.ts diff --git a/languageservice/src/validate.concurrency.test.ts b/languageservice/src/validate.concurrency.test.ts new file mode 100644 index 0000000..9bbccec --- /dev/null +++ b/languageservice/src/validate.concurrency.test.ts @@ -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); + }); + }); +}); diff --git a/languageservice/src/validate.ts b/languageservice/src/validate.ts index f81c836..9f24976 100644 --- a/languageservice/src/validate.ts +++ b/languageservice/src/validate.ts @@ -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; +}