From ad048ba16040b2642272015cf9bc76560d0197d2 Mon Sep 17 00:00:00 2001 From: Efra Espada Date: Mon, 31 Aug 2026 13:47:40 +0000 Subject: [PATCH 1/6] bugfix-344-throttle-workflow-queue-polling-and-increase-workflow-timeouts --- .github/workflows/copilot_commit.yml | 2 +- .github/workflows/copilot_issue.yml | 2 +- .github/workflows/copilot_issue_comment.yml | 2 +- .github/workflows/copilot_pull_request.yml | 2 +- .../copilot_pull_request_comment.yml | 2 +- .github/workflows/hotfix_workflow.yml | 2 +- .github/workflows/release_workflow.yml | 2 +- README.md | 2 +- build/cli/index.js | 364 +++++++++++++----- .../policies/workflow_queue_policy.d.ts | 10 + .../application/ports/workflow_run_ports.d.ts | 18 +- ...t_for_previous_workflow_runs_use_case.d.ts | 11 +- ...ive_previous_workflow_runs_repository.d.ts | 11 +- .../workflow/workflow_runs_retry.d.ts | 18 +- ...ger_workflow_polling_observer_adapter.d.ts | 6 + ...ystem_workflow_polling_random_adapter.d.ts | 4 + .../system_workflow_queue_clock_adapter.d.ts | 4 + build/github_action/index.js | 364 +++++++++++++----- .../policies/workflow_queue_policy.d.ts | 10 + .../application/ports/workflow_run_ports.d.ts | 18 +- ...t_for_previous_workflow_runs_use_case.d.ts | 11 +- ...ive_previous_workflow_runs_repository.d.ts | 11 +- .../workflow/workflow_runs_retry.d.ts | 18 +- ...ger_workflow_polling_observer_adapter.d.ts | 6 + ...ystem_workflow_polling_random_adapter.d.ts | 4 + .../system_workflow_queue_clock_adapter.d.ts | 4 + docs/development/architecture.mdx | 17 + docs/development/testing.mdx | 8 + docs/features.mdx | 10 +- scripts/validate-workflow-contract.cjs | 138 ++++--- setup/workflows/copilot_commit.yml | 1 + setup/workflows/copilot_issue.yml | 2 +- setup/workflows/copilot_issue_comment.yml | 2 +- setup/workflows/copilot_pull_request.yml | 2 +- .../copilot_pull_request_comment.yml | 2 +- setup/workflows/hotfix_workflow.yml | 2 +- setup/workflows/release_workflow.yml | 2 +- .../policies/workflow_queue_policy.ts | 41 ++ src/application/ports/workflow_run_ports.ts | 22 +- ...or_previous_workflow_runs_use_case.test.ts | 108 +++--- ...ait_for_previous_workflow_runs_use_case.ts | 53 ++- src/cli/commands/__tests__/do_policy.test.ts | 6 +- ..._previous_workflow_runs_repository.test.ts | 214 +++------- .../workflow_runs_retry_policy.test.ts | 101 +++-- ...ctive_previous_workflow_runs_repository.ts | 117 +++--- .../workflow/workflow_runs_retry.ts | 231 +++++++++-- .../workflow_queue_composition_root.test.ts | 6 +- .../workflow_queue_composition_root.ts | 14 +- ...ogger_workflow_polling_observer_adapter.ts | 16 + .../system_workflow_polling_random_adapter.ts | 7 + .../system_workflow_queue_clock_adapter.ts | 7 + .../validate_workflow_contract.test.ts | 54 +++ 52 files changed, 1451 insertions(+), 640 deletions(-) create mode 100644 build/cli/src/infrastructure/time/system_workflow_polling_random_adapter.d.ts create mode 100644 build/cli/src/infrastructure/time/system_workflow_queue_clock_adapter.d.ts create mode 100644 build/github_action/src/infrastructure/time/system_workflow_polling_random_adapter.d.ts create mode 100644 build/github_action/src/infrastructure/time/system_workflow_queue_clock_adapter.d.ts create mode 100644 src/infrastructure/time/system_workflow_polling_random_adapter.ts create mode 100644 src/infrastructure/time/system_workflow_queue_clock_adapter.ts create mode 100644 src/tooling/__tests__/validate_workflow_contract.test.ts diff --git a/.github/workflows/copilot_commit.yml b/.github/workflows/copilot_commit.yml index 812b444f0..7eef745ce 100644 --- a/.github/workflows/copilot_commit.yml +++ b/.github/workflows/copilot_commit.yml @@ -11,7 +11,7 @@ jobs: copilot-commits: name: Copilot - Commit runs-on: [self-hosted, codex] - timeout-minutes: 30 + timeout-minutes: 120 permissions: contents: read steps: diff --git a/.github/workflows/copilot_issue.yml b/.github/workflows/copilot_issue.yml index d4d2b6ab0..0caca4d80 100644 --- a/.github/workflows/copilot_issue.yml +++ b/.github/workflows/copilot_issue.yml @@ -8,7 +8,7 @@ jobs: copilot-issues: name: Copilot - Issue runs-on: [self-hosted, codex] - timeout-minutes: 30 + timeout-minutes: 120 permissions: contents: read steps: diff --git a/.github/workflows/copilot_issue_comment.yml b/.github/workflows/copilot_issue_comment.yml index 894971157..39df20fb9 100644 --- a/.github/workflows/copilot_issue_comment.yml +++ b/.github/workflows/copilot_issue_comment.yml @@ -8,7 +8,7 @@ jobs: copilot-issues: name: Copilot - Issue Comment runs-on: [self-hosted, codex] - timeout-minutes: 30 + timeout-minutes: 120 permissions: contents: write steps: diff --git a/.github/workflows/copilot_pull_request.yml b/.github/workflows/copilot_pull_request.yml index af53edb21..c170eaf26 100644 --- a/.github/workflows/copilot_pull_request.yml +++ b/.github/workflows/copilot_pull_request.yml @@ -8,7 +8,7 @@ jobs: copilot-pull-requests: name: Copilot - Pull Request runs-on: [self-hosted, codex] - timeout-minutes: 30 + timeout-minutes: 120 permissions: contents: read steps: diff --git a/.github/workflows/copilot_pull_request_comment.yml b/.github/workflows/copilot_pull_request_comment.yml index 5cf880f43..280357359 100644 --- a/.github/workflows/copilot_pull_request_comment.yml +++ b/.github/workflows/copilot_pull_request_comment.yml @@ -8,7 +8,7 @@ jobs: copilot-pull-requests: name: Copilot - Pull Request Comment runs-on: [self-hosted, codex] - timeout-minutes: 30 + timeout-minutes: 120 permissions: contents: write steps: diff --git a/.github/workflows/hotfix_workflow.yml b/.github/workflows/hotfix_workflow.yml index 4b2ce38e0..ebd0583d6 100644 --- a/.github/workflows/hotfix_workflow.yml +++ b/.github/workflows/hotfix_workflow.yml @@ -138,7 +138,7 @@ jobs: tag: name: Publish version runs-on: [self-hosted, codex] - timeout-minutes: 30 + timeout-minutes: 120 needs: [ prepare-compiled-files ] env: AGENT_PROVIDER: ${{ vars.AGENT_PROVIDER || 'codex' }} diff --git a/.github/workflows/release_workflow.yml b/.github/workflows/release_workflow.yml index 42bd2b8c0..eab7d6a4d 100644 --- a/.github/workflows/release_workflow.yml +++ b/.github/workflows/release_workflow.yml @@ -138,7 +138,7 @@ jobs: tag: name: Publish version runs-on: [self-hosted, codex] - timeout-minutes: 30 + timeout-minutes: 120 needs: [ prepare-compiled-files ] env: AGENT_PROVIDER: ${{ vars.AGENT_PROVIDER || 'codex' }} diff --git a/README.md b/README.md index cdac0ba52..7f5e1e02d 100644 --- a/README.md +++ b/README.md @@ -47,7 +47,7 @@ Full documentation: **[docs.page/vypdev/copilot](https://docs.page/vypdev/copilo - **Projects** — Link issues and PRs to boards and move them to the right columns. - **Single actions** — On-demand: check progress, think, create release/tag, mark deployed, etc. - **Evidence and safety** — Every run writes a bounded Job Summary; PR reviews expose a `Copilot / Review` Check Run, active findings fail that check, and all agent/comment content remains bounded and treated as untrusted data. -- **Concurrency** — Waits for previous runs of the same workflow so runs can be sequential. See [Features → Workflow concurrency](https://docs.page/vypdev/copilot/features#workflow-concurrency-and-sequential-execution). +- **Concurrency** — Uses a repository-wide application queue across the seven Copilot/Task mutation workflows. Polling is adaptive and rate-limit-aware, with a 90-minute queue deadline and no cancellation or overwrite of intermediate runs. See [Features → Workflow concurrency](https://docs.page/vypdev/copilot/features#workflow-concurrency-and-sequential-execution). AI features use the configured agent runtime and qualified model; see the [Agents](https://docs.page/vypdev/copilot/agents) and [Security & Operations](https://docs.page/vypdev/copilot/security-operations) documentation. You can run progress and Bugbot locally through the [Single actions → Workflow & CLI](https://docs.page/vypdev/copilot/single-actions/workflow-and-cli) path. diff --git a/build/cli/index.js b/build/cli/index.js index 8d0449c23..f495c7aba 100755 --- a/build/cli/index.js +++ b/build/cli/index.js @@ -54553,7 +54553,9 @@ function calculateReviewersStillNeeded(desiredCount, currentCount, confirmedCoun "use strict"; Object.defineProperty(exports, "__esModule", ({ value: true })); -exports.COPILOT_WORKFLOW_NAMES = void 0; +exports.WORKFLOW_QUEUE_POLICY = exports.COPILOT_WORKFLOW_NAMES = void 0; +exports.calculateWorkflowPollingDelay = calculateWorkflowPollingDelay; +exports.calculateJitteredWorkflowDelay = calculateJitteredWorkflowDelay; /** * Workflows that execute the Copilot action and therefore share its * repository mutation queue. Keep these names aligned with workflow `name` @@ -54568,6 +54570,22 @@ exports.COPILOT_WORKFLOW_NAMES = [ 'Task - Hotfix', 'Task - Release', ]; +exports.WORKFLOW_QUEUE_POLICY = { + maximumQueueWaitMilliseconds: 90 * 60 * 1000, + initialDelayMilliseconds: 5 * 1000, + backoffMultiplier: 2, + maximumDelayMilliseconds: 60 * 1000, + jitterRatio: 0.2, +}; +function calculateWorkflowPollingDelay(pollIndex, randomValue, policy = exports.WORKFLOW_QUEUE_POLICY) { + const baseDelay = Math.min(policy.initialDelayMilliseconds * policy.backoffMultiplier ** pollIndex, policy.maximumDelayMilliseconds); + return calculateJitteredWorkflowDelay(baseDelay, randomValue, policy); +} +function calculateJitteredWorkflowDelay(baseDelayMilliseconds, randomValue, policy) { + const boundedRandom = Math.min(1, Math.max(0, randomValue)); + const jitter = (boundedRandom * 2 - 1) * policy.jitterRatio; + return Math.min(policy.maximumDelayMilliseconds, Math.max(0, Math.round(baseDelayMilliseconds * (1 + jitter)))); +} /***/ }), @@ -62632,37 +62650,50 @@ exports.CheckPullRequestCommentLanguageUseCase = CheckPullRequestCommentLanguage /***/ }), /***/ 38301: -/***/ ((__unused_webpack_module, exports) => { +/***/ ((__unused_webpack_module, exports, __nccwpck_require__) => { "use strict"; Object.defineProperty(exports, "__esModule", ({ value: true })); exports.WaitForPreviousWorkflowRunsUseCase = void 0; -const DEFAULT_POLICY = { - maximumAttempts: 2000, - delayMilliseconds: 2000, -}; +const workflow_queue_policy_1 = __nccwpck_require__(43193); +const SYSTEM_CLOCK = { nowMilliseconds: () => Date.now() }; +const SYSTEM_RANDOM = { next: () => Math.random() }; class WaitForPreviousWorkflowRunsUseCase { - constructor(queryPort, delayPort, observerPort, policy = DEFAULT_POLICY) { + constructor(queryPort, delayPort, observerPort, policy = workflow_queue_policy_1.WORKFLOW_QUEUE_POLICY, clock = SYSTEM_CLOCK, random = SYSTEM_RANDOM) { this.queryPort = queryPort; this.delayPort = delayPort; this.observerPort = observerPort; this.policy = policy; + this.clock = clock; + this.random = random; this.taskId = 'WaitForPreviousWorkflowRunsUseCase'; } async invoke(query) { - for (let attempt = 0; attempt < this.policy.maximumAttempts; attempt++) { - const activeRunCount = await this.queryPort.countActivePreviousRuns(query); + const deadlineAtMilliseconds = this.clock.nowMilliseconds() + this.policy.maximumQueueWaitMilliseconds; + let pollIndex = 0; + while (true) { + if (this.clock.nowMilliseconds() >= deadlineAtMilliseconds) { + throw new Error('Timeout waiting for previous runs to finish.'); + } + const activeRunCount = await this.queryPort.countActivePreviousRuns(query, { + deadlineAtMilliseconds, + }); + if (this.clock.nowMilliseconds() >= deadlineAtMilliseconds) { + throw new Error('Timeout waiting for previous runs to finish.'); + } if (activeRunCount === 0) { this.observerPort.noActivePreviousRuns(); return; } - if (attempt === this.policy.maximumAttempts - 1) - break; - this.observerPort.waitingForPreviousRuns(activeRunCount, this.policy.delayMilliseconds); - await this.delayPort.wait(this.policy.delayMilliseconds); + const delayMilliseconds = (0, workflow_queue_policy_1.calculateWorkflowPollingDelay)(pollIndex, this.random.next(), this.policy); + if (this.clock.nowMilliseconds() + delayMilliseconds >= deadlineAtMilliseconds) { + throw new Error('Timeout waiting for previous runs to finish.'); + } + this.observerPort.waitingForPreviousRuns(activeRunCount, delayMilliseconds); + await this.delayPort.wait(delayMilliseconds); + pollIndex += 1; } - throw new Error('Timeout waiting for previous runs to finish.'); } } exports.WaitForPreviousWorkflowRunsUseCase = WaitForPreviousWorkflowRunsUseCase; @@ -69508,76 +69539,70 @@ Object.defineProperty(exports, "__esModule", ({ value: true })); exports.ActivePreviousWorkflowRunsRepository = void 0; const constants_1 = __nccwpck_require__(15415); const workflow_runs_retry_1 = __nccwpck_require__(86434); -const DEFAULT_RETRY_POLICY = { - maximumAttempts: 5, - initialDelayMilliseconds: 1000, - backoffMultiplier: 2, - maximumDelayMilliseconds: 8000, -}; -const NO_OP_DELAY_PORT = { - async wait() { - return Promise.resolve(); - }, -}; +const NO_OP_DELAY_PORT = { wait: async () => undefined }; +const SYSTEM_CLOCK = { nowMilliseconds: () => Date.now() }; +const SYSTEM_RANDOM = { next: () => Math.random() }; class ActivePreviousWorkflowRunsRepository { - constructor(client, retryDelayPort = NO_OP_DELAY_PORT, retryPolicy = DEFAULT_RETRY_POLICY) { + constructor(client, retryDelayPort = NO_OP_DELAY_PORT, retryPolicy = workflow_runs_retry_1.WORKFLOW_RUNS_RETRY_POLICY, clock = SYSTEM_CLOCK, random = SYSTEM_RANDOM, observer) { this.client = client; this.retryDelayPort = retryDelayPort; this.retryPolicy = retryPolicy; + this.clock = clock; + this.random = random; + this.observer = observer; } - async countActivePreviousRuns(query) { - const hasWorkflowScope = query.workflowNames?.some((name) => name.trim().length > 0) || query.workflowName.trim().length > 0; - if (!Number.isFinite(query.currentRunId) || !hasWorkflowScope) { - return 0; + async countActivePreviousRuns(query, context = { deadlineAtMilliseconds: Number.POSITIVE_INFINITY }) { + if (!Number.isSafeInteger(query.currentRunId)) { + throw new Error('GitHub workflow identity is unavailable; refusing to bypass sequential execution.'); } - let activeRunCount = 0; - for (const status of constants_1.WORKFLOW_ACTIVE_STATUSES) { - activeRunCount += await this.countActiveRunsForStatus(query, status); + const workflowNames = query.workflowNames?.filter(name => name.trim().length > 0) ?? []; + if (workflowNames.length === 0 && query.workflowName.trim().length === 0) { + throw new Error('GitHub workflow name is unavailable; refusing to bypass sequential execution.'); } - return activeRunCount; - } - async countActiveRunsForStatus(query, status) { - const useWorkflowEndpoint = Boolean(query.workflowIdentifier - && (!query.workflowNames || query.workflowNames.length === 0) - && this.client.rest.actions.listWorkflowRuns); - const method = useWorkflowEndpoint + const method = query.workflowIdentifier && workflowNames.length === 0 ? this.client.rest.actions.listWorkflowRuns : this.client.rest.actions.listWorkflowRunsForRepo; + if (!method) + throw new Error('GitHub workflow-scoped runs endpoint is unavailable.'); const parameters = { owner: query.owner, repo: query.repository, per_page: 100, - status, - ...(useWorkflowEndpoint ? { workflow_id: query.workflowIdentifier } : {}), + ...(method === this.client.rest.actions.listWorkflowRuns && query.workflowIdentifier + ? { workflow_id: query.workflowIdentifier } + : {}), }; + const names = workflowNames.length > 0 ? workflowNames : [query.workflowName]; return (0, workflow_runs_retry_1.withWorkflowRunsRetry)(async () => { let activeRunCount = 0; for await (const response of this.client.paginate.iterator(method, parameters)) { - activeRunCount += countMatchingRuns(this.extractWorkflowRuns(response), query); + activeRunCount += extractWorkflowRuns(response) + .filter(run => isActivePreviousRun(run, query, names)).length; } return activeRunCount; - }, this.retryDelayPort, this.retryPolicy); - } - extractWorkflowRuns(response) { - const data = response?.data; - if (Array.isArray(data)) { - return data; - } - if (data !== null && typeof data === 'object' && Array.isArray(data.workflow_runs)) { - return data.workflow_runs; - } - throw new Error('GitHub workflow runs response did not contain a workflow_runs array.'); + }, { + delayPort: this.retryDelayPort, + clock: this.clock, + random: this.random, + observer: this.observer, + policy: this.retryPolicy, + deadlineAtMilliseconds: context.deadlineAtMilliseconds, + }); } } exports.ActivePreviousWorkflowRunsRepository = ActivePreviousWorkflowRunsRepository; -function countMatchingRuns(runs, query) { - return runs.filter((run) => isActivePreviousRun(run, query)).length; -} -function isActivePreviousRun(run, query) { - const workflowMatches = query.workflowNames && query.workflowNames.length > 0 - ? query.workflowNames.includes(run.name ?? '') - : run.name === query.workflowName; - return workflowMatches +function extractWorkflowRuns(response) { + const data = response?.data; + if (Array.isArray(data)) + return data; + if (data !== null && typeof data === 'object' && Array.isArray(data.workflow_runs)) { + return data.workflow_runs; + } + throw new Error('GitHub workflow runs response did not contain a workflow_runs array.'); +} +function isActivePreviousRun(run, query, workflowNames) { + return typeof run.name === 'string' + && workflowNames.includes(run.name) && run.id < query.currentRunId && constants_1.WORKFLOW_ACTIVE_STATUSES.includes(run.status ?? 'unknown'); } @@ -69613,12 +69638,75 @@ exports.WorkflowDispatchRepository = WorkflowDispatchRepository; /***/ }), /***/ 86434: -/***/ ((__unused_webpack_module, exports) => { +/***/ ((__unused_webpack_module, exports, __nccwpck_require__) => { "use strict"; Object.defineProperty(exports, "__esModule", ({ value: true })); +exports.WorkflowQueueDeadlineError = exports.WORKFLOW_RUNS_RETRY_POLICY = void 0; exports.withWorkflowRunsRetry = withWorkflowRunsRetry; +const workflow_queue_policy_1 = __nccwpck_require__(43193); +exports.WORKFLOW_RUNS_RETRY_POLICY = { + maximumAttempts: 5, + initialDelayMilliseconds: 1000, + backoffMultiplier: 2, + maximumDelayMilliseconds: 30000, + jitterRatio: 0.2, + rateLimitInitialDelayMilliseconds: 60000, + rateLimitMaximumDelayMilliseconds: 300000, +}; +class WorkflowQueueDeadlineError extends Error { + constructor() { + super('Timeout waiting for previous runs to finish.'); + this.name = 'WorkflowQueueDeadlineError'; + } +} +exports.WorkflowQueueDeadlineError = WorkflowQueueDeadlineError; +function withWorkflowRunsRetry(operation, dependenciesOrDelayPort, legacyPolicy) { + const dependencies = 'clock' in dependenciesOrDelayPort + ? dependenciesOrDelayPort + : { + delayPort: dependenciesOrDelayPort, + clock: { nowMilliseconds: () => Date.now() }, + random: { next: () => 0.5 }, + policy: { + ...exports.WORKFLOW_RUNS_RETRY_POLICY, + ...legacyPolicy, + jitterRatio: 0, + }, + deadlineAtMilliseconds: Number.POSITIVE_INFINITY, + }; + return executeWithRetry(operation, dependencies, 1); +} +async function executeWithRetry(operation, dependencies, attempt) { + if (dependencies.clock.nowMilliseconds() >= dependencies.deadlineAtMilliseconds) { + throw new WorkflowQueueDeadlineError(); + } + try { + return await operation(); + } + catch (error) { + const classification = classifyWorkflowRunsError(error, dependencies.clock); + if (!classification.retryable + || (classification.reason === 'transient' && attempt >= dependencies.policy.maximumAttempts)) { + throw error; + } + const delayMilliseconds = retryDelay(classification, attempt, dependencies); + if (dependencies.clock.nowMilliseconds() + delayMilliseconds >= dependencies.deadlineAtMilliseconds) { + throw new WorkflowQueueDeadlineError(); + } + dependencies.observer?.providerRetry?.({ + reason: classification.reason, + attempt, + delayMilliseconds, + ...(classification.resetEpochSeconds === undefined + ? {} + : { resetEpochSeconds: classification.resetEpochSeconds }), + }); + await dependencies.delayPort.wait(delayMilliseconds); + return executeWithRetry(operation, dependencies, attempt + 1); + } +} const TRANSIENT_NETWORK_ERRORS = new Set([ 'ECONNRESET', 'ETIMEDOUT', @@ -69627,38 +69715,87 @@ const TRANSIENT_NETWORK_ERRORS = new Set([ 'ECONNREFUSED', 'UND_ERR_CONNECT_TIMEOUT', ]); -async function withWorkflowRunsRetry(operation, delayPort, policy) { - return executeWithRetry(operation, delayPort, policy, 1, policy.initialDelayMilliseconds); +function retryDelay(classification, attempt, dependencies) { + if (classification.retryAfterMilliseconds !== undefined) + return classification.retryAfterMilliseconds; + const { policy } = dependencies; + const rateLimit = classification.reason === 'rate_limit'; + const baseDelay = Math.min((rateLimit ? (policy.rateLimitInitialDelayMilliseconds ?? 60000) : policy.initialDelayMilliseconds) + * policy.backoffMultiplier ** (attempt - 1), rateLimit ? (policy.rateLimitMaximumDelayMilliseconds ?? 300000) : policy.maximumDelayMilliseconds); + const jitterPolicy = { + maximumDelayMilliseconds: rateLimit + ? (policy.rateLimitMaximumDelayMilliseconds ?? 300000) + : policy.maximumDelayMilliseconds, + jitterRatio: policy.jitterRatio ?? 0, + }; + return (0, workflow_queue_policy_1.calculateJitteredWorkflowDelay)(baseDelay, dependencies.random.next(), jitterPolicy); } -async function executeWithRetry(operation, delayPort, policy, attempt, delayMilliseconds) { - try { - return await operation(); +function classifyWorkflowRunsError(error, clock) { + if (!error || typeof error !== 'object') + return { retryable: false, reason: 'transient' }; + const candidate = error; + const status = firstNumericValue(candidate.status, candidate.statusCode, candidate.response?.status); + const headers = candidate.response?.headers ?? candidate.headers; + const message = [candidate.response?.data?.message, candidate.data?.message, candidate.message] + .find(value => typeof value === 'string'); + const remaining = header(headers, 'x-ratelimit-remaining'); + const isRateLimited = status === 429 + || (status === 403 && (remaining === '0' + || /(?:rate limit|secondary rate|abuse limit|too many requests)/i.test(message ?? ''))); + if (isRateLimited) { + const retryAfterMilliseconds = parseRetryAfter(header(headers, 'retry-after'), clock); + const resetEpochSeconds = parseEpochSeconds(header(headers, 'x-ratelimit-reset')); + const resetDelay = resetEpochSeconds === undefined + ? undefined + : Math.max(0, resetEpochSeconds * 1000 - clock.nowMilliseconds()); + return { + retryable: true, + reason: 'rate_limit', + retryAfterMilliseconds: retryAfterMilliseconds ?? resetDelay, + resetEpochSeconds, + }; } - catch (error) { - if (!shouldRetry(error, attempt, policy.maximumAttempts)) - throw error; - await delayPort.wait(delayMilliseconds); - return executeWithRetry(operation, delayPort, policy, attempt + 1, nextRetryDelay(delayMilliseconds, policy)); + if (status === 408 || (status !== undefined && status >= 500)) { + return { retryable: true, reason: 'transient' }; + } + if (typeof candidate.code === 'string' && TRANSIENT_NETWORK_ERRORS.has(candidate.code)) { + return { retryable: true, reason: 'transient' }; } + return { + retryable: typeof message === 'string' + && /\b(server error|service unavailable|bad gateway|gateway timeout|temporarily unavailable)\b/i.test(message), + reason: 'transient', + }; } -function shouldRetry(error, attempt, maximumAttempts) { - return attempt < maximumAttempts && isTransientWorkflowRunsError(error); +function header(headers, name) { + if (!headers) + return undefined; + if (typeof headers.get === 'function') { + const value = headers.get(name); + return value === undefined || value === null ? undefined : String(value); + } + if (typeof headers !== 'object') + return undefined; + const entry = Object.entries(headers) + .find(([key]) => key.toLowerCase() === name); + return entry?.[1] === undefined || entry?.[1] === null ? undefined : String(entry[1]); } -function nextRetryDelay(currentDelay, policy) { - return Math.min(currentDelay * policy.backoffMultiplier, policy.maximumDelayMilliseconds); +function parseRetryAfter(value, clock) { + if (!value) + return undefined; + const seconds = Number(value); + if (Number.isFinite(seconds) && seconds >= 0) + return Math.round(seconds * 1000); + const timestamp = Date.parse(value); + return Number.isFinite(timestamp) + ? Math.max(0, timestamp - clock.nowMilliseconds()) + : undefined; } -function isTransientWorkflowRunsError(error) { - if (!error || typeof error !== 'object') - return false; - const candidate = error; - const status = firstNumericValue(candidate.status, candidate.statusCode, candidate.response?.status); - if (status !== undefined) { - return status === 408 || status === 429 || status >= 500; - } - if (typeof candidate.code === 'string' && TRANSIENT_NETWORK_ERRORS.has(candidate.code)) - return true; - return typeof candidate.message === 'string' - && /\b(server error|service unavailable|bad gateway|gateway timeout|temporarily unavailable)\b/i.test(candidate.message); +function parseEpochSeconds(value) { + if (!value) + return undefined; + const epoch = Number(value); + return Number.isFinite(epoch) && epoch >= 0 ? epoch : undefined; } function firstNumericValue(...values) { return values.find((value) => typeof value === 'number' && Number.isFinite(value)); @@ -70823,11 +70960,14 @@ const wait_for_previous_workflow_runs_use_case_1 = __nccwpck_require__(38301); const active_previous_workflow_runs_repository_1 = __nccwpck_require__(40941); const timer_workflow_polling_delay_adapter_1 = __nccwpck_require__(10339); const logger_workflow_polling_observer_adapter_1 = __nccwpck_require__(52883); +const system_workflow_queue_clock_adapter_1 = __nccwpck_require__(49664); +const system_workflow_polling_random_adapter_1 = __nccwpck_require__(32679); const github_workflow_client_factory_1 = __nccwpck_require__(29839); function createWaitForPreviousWorkflowRunsUseCase(token) { const client = (0, github_workflow_client_factory_1.createWorkflowRunsClient)().getClient(token); const delayPort = new timer_workflow_polling_delay_adapter_1.TimerWorkflowPollingDelayAdapter(); - return new wait_for_previous_workflow_runs_use_case_1.WaitForPreviousWorkflowRunsUseCase(new active_previous_workflow_runs_repository_1.ActivePreviousWorkflowRunsRepository(client, delayPort), delayPort, new logger_workflow_polling_observer_adapter_1.LoggerWorkflowPollingObserverAdapter()); + const observerPort = new logger_workflow_polling_observer_adapter_1.LoggerWorkflowPollingObserverAdapter(); + return new wait_for_previous_workflow_runs_use_case_1.WaitForPreviousWorkflowRunsUseCase(new active_previous_workflow_runs_repository_1.ActivePreviousWorkflowRunsRepository(client, delayPort, undefined, new system_workflow_queue_clock_adapter_1.SystemWorkflowQueueClockAdapter(), new system_workflow_polling_random_adapter_1.SystemWorkflowPollingRandomAdapter(), observerPort), delayPort, observerPort); } @@ -71199,6 +71339,16 @@ class LoggerWorkflowPollingObserverAdapter { waitingForPreviousRuns(activeRunCount, delayMilliseconds) { (0, logger_1.logDebugInfo)(`⏳ Found ${activeRunCount} previous run(s) still active. Waiting ${delayMilliseconds / 1000}s...`); } + providerRetry(observation) { + (0, logger_1.logDebugInfo)('GitHub workflow polling retry scheduled.', false, { + reason: observation.reason, + attempt: observation.attempt, + delayMilliseconds: observation.delayMilliseconds, + ...(observation.resetEpochSeconds === undefined + ? {} + : { resetEpochSeconds: observation.resetEpochSeconds }), + }); + } } exports.LoggerWorkflowPollingObserverAdapter = LoggerWorkflowPollingObserverAdapter; @@ -71226,6 +71376,40 @@ class SetupWorkspaceAdapter { exports.SetupWorkspaceAdapter = SetupWorkspaceAdapter; +/***/ }), + +/***/ 32679: +/***/ ((__unused_webpack_module, exports) => { + +"use strict"; + +Object.defineProperty(exports, "__esModule", ({ value: true })); +exports.SystemWorkflowPollingRandomAdapter = void 0; +class SystemWorkflowPollingRandomAdapter { + next() { + return Math.random(); + } +} +exports.SystemWorkflowPollingRandomAdapter = SystemWorkflowPollingRandomAdapter; + + +/***/ }), + +/***/ 49664: +/***/ ((__unused_webpack_module, exports) => { + +"use strict"; + +Object.defineProperty(exports, "__esModule", ({ value: true })); +exports.SystemWorkflowQueueClockAdapter = void 0; +class SystemWorkflowQueueClockAdapter { + nowMilliseconds() { + return Date.now(); + } +} +exports.SystemWorkflowQueueClockAdapter = SystemWorkflowQueueClockAdapter; + + /***/ }), /***/ 20846: diff --git a/build/cli/src/application/policies/workflow_queue_policy.d.ts b/build/cli/src/application/policies/workflow_queue_policy.d.ts index 67cc43f76..f69afed1f 100644 --- a/build/cli/src/application/policies/workflow_queue_policy.d.ts +++ b/build/cli/src/application/policies/workflow_queue_policy.d.ts @@ -4,3 +4,13 @@ * values in `.github/workflows` and the setup templates. */ export declare const COPILOT_WORKFLOW_NAMES: readonly ["Copilot - Issue", "Copilot - Issue Comment", "Copilot - Commit", "Copilot - Pull Request", "Copilot - Pull Request Comment", "Task - Hotfix", "Task - Release"]; +export interface WorkflowPollingPolicy { + maximumQueueWaitMilliseconds: number; + initialDelayMilliseconds: number; + backoffMultiplier: number; + maximumDelayMilliseconds: number; + jitterRatio: number; +} +export declare const WORKFLOW_QUEUE_POLICY: WorkflowPollingPolicy; +export declare function calculateWorkflowPollingDelay(pollIndex: number, randomValue: number, policy?: WorkflowPollingPolicy): number; +export declare function calculateJitteredWorkflowDelay(baseDelayMilliseconds: number, randomValue: number, policy: Pick): number; diff --git a/build/cli/src/application/ports/workflow_run_ports.d.ts b/build/cli/src/application/ports/workflow_run_ports.d.ts index 9946163a8..5f8c26d4a 100644 --- a/build/cli/src/application/ports/workflow_run_ports.d.ts +++ b/build/cli/src/application/ports/workflow_run_ports.d.ts @@ -8,13 +8,29 @@ export interface PreviousWorkflowRunsQuery { /** All Copilot workflow names share one queue so different event workflows cannot overlap. */ workflowNames?: readonly string[]; } +export interface WorkflowQueueRequestContext { + deadlineAtMilliseconds: number; +} export interface PreviousWorkflowRunsQueryPort { - countActivePreviousRuns(query: PreviousWorkflowRunsQuery): Promise; + countActivePreviousRuns(query: PreviousWorkflowRunsQuery, context: WorkflowQueueRequestContext): Promise; } export interface WorkflowPollingDelayPort { wait(milliseconds: number): Promise; } +export interface WorkflowQueueClockPort { + nowMilliseconds(): number; +} +export interface WorkflowPollingRandomPort { + next(): number; +} +export interface WorkflowProviderRetryObservation { + reason: 'rate_limit' | 'transient'; + attempt: number; + delayMilliseconds: number; + resetEpochSeconds?: number; +} export interface WorkflowPollingObserverPort { noActivePreviousRuns(): void; waitingForPreviousRuns(activeRunCount: number, delayMilliseconds: number): void; + providerRetry?(observation: WorkflowProviderRetryObservation): void; } diff --git a/build/cli/src/application/usecases/workflow/wait_for_previous_workflow_runs_use_case.d.ts b/build/cli/src/application/usecases/workflow/wait_for_previous_workflow_runs_use_case.d.ts index 74b8f9f4e..938281f16 100644 --- a/build/cli/src/application/usecases/workflow/wait_for_previous_workflow_runs_use_case.d.ts +++ b/build/cli/src/application/usecases/workflow/wait_for_previous_workflow_runs_use_case.d.ts @@ -1,15 +1,14 @@ -import type { PreviousWorkflowRunsQuery, PreviousWorkflowRunsQueryPort, WorkflowPollingDelayPort, WorkflowPollingObserverPort } from '../../ports/workflow_run_ports'; +import type { PreviousWorkflowRunsQuery, PreviousWorkflowRunsQueryPort, WorkflowPollingDelayPort, WorkflowPollingObserverPort, WorkflowPollingRandomPort, WorkflowQueueClockPort } from '../../ports/workflow_run_ports'; +import { type WorkflowPollingPolicy } from '../../policies/workflow_queue_policy'; import type { ParamUseCase } from '../base/param_usecase'; -export interface WorkflowPollingPolicy { - maximumAttempts: number; - delayMilliseconds: number; -} export declare class WaitForPreviousWorkflowRunsUseCase implements ParamUseCase { private readonly queryPort; private readonly delayPort; private readonly observerPort; private readonly policy; + private readonly clock; + private readonly random; taskId: string; - constructor(queryPort: PreviousWorkflowRunsQueryPort, delayPort: WorkflowPollingDelayPort, observerPort: WorkflowPollingObserverPort, policy?: WorkflowPollingPolicy); + constructor(queryPort: PreviousWorkflowRunsQueryPort, delayPort: WorkflowPollingDelayPort, observerPort: WorkflowPollingObserverPort, policy?: WorkflowPollingPolicy, clock?: WorkflowQueueClockPort, random?: WorkflowPollingRandomPort); invoke(query: PreviousWorkflowRunsQuery): Promise; } diff --git a/build/cli/src/data/repository/workflow/active_previous_workflow_runs_repository.d.ts b/build/cli/src/data/repository/workflow/active_previous_workflow_runs_repository.d.ts index 50b528708..778c1a68c 100644 --- a/build/cli/src/data/repository/workflow/active_previous_workflow_runs_repository.d.ts +++ b/build/cli/src/data/repository/workflow/active_previous_workflow_runs_repository.d.ts @@ -1,12 +1,13 @@ -import type { PreviousWorkflowRunsQuery, PreviousWorkflowRunsQueryPort, WorkflowPollingDelayPort } from '../../../application/ports/workflow_run_ports'; +import type { PreviousWorkflowRunsQuery, PreviousWorkflowRunsQueryPort, WorkflowPollingDelayPort, WorkflowPollingObserverPort, WorkflowPollingRandomPort, WorkflowQueueClockPort, WorkflowQueueRequestContext } from '../../../application/ports/workflow_run_ports'; import type { GithubWorkflowRunsClient } from '../../../infrastructure/github/ports/github_workflow_provider_ports'; import { type WorkflowRunsRetryPolicy } from './workflow_runs_retry'; export declare class ActivePreviousWorkflowRunsRepository implements PreviousWorkflowRunsQueryPort { private readonly client; private readonly retryDelayPort; private readonly retryPolicy; - constructor(client: GithubWorkflowRunsClient, retryDelayPort?: WorkflowPollingDelayPort, retryPolicy?: WorkflowRunsRetryPolicy); - countActivePreviousRuns(query: PreviousWorkflowRunsQuery): Promise; - private countActiveRunsForStatus; - private extractWorkflowRuns; + private readonly clock; + private readonly random; + private readonly observer?; + constructor(client: GithubWorkflowRunsClient, retryDelayPort?: WorkflowPollingDelayPort, retryPolicy?: WorkflowRunsRetryPolicy, clock?: WorkflowQueueClockPort, random?: WorkflowPollingRandomPort, observer?: WorkflowPollingObserverPort | undefined); + countActivePreviousRuns(query: PreviousWorkflowRunsQuery, context?: WorkflowQueueRequestContext): Promise; } diff --git a/build/cli/src/data/repository/workflow/workflow_runs_retry.d.ts b/build/cli/src/data/repository/workflow/workflow_runs_retry.d.ts index 4f5bc5cfa..269efca75 100644 --- a/build/cli/src/data/repository/workflow/workflow_runs_retry.d.ts +++ b/build/cli/src/data/repository/workflow/workflow_runs_retry.d.ts @@ -1,8 +1,24 @@ -import type { WorkflowPollingDelayPort } from '../../../application/ports/workflow_run_ports'; +import type { WorkflowPollingDelayPort, WorkflowPollingObserverPort, WorkflowPollingRandomPort, WorkflowQueueClockPort } from '../../../application/ports/workflow_run_ports'; export interface WorkflowRunsRetryPolicy { maximumAttempts: number; initialDelayMilliseconds: number; backoffMultiplier: number; maximumDelayMilliseconds: number; + jitterRatio?: number; + rateLimitInitialDelayMilliseconds?: number; + rateLimitMaximumDelayMilliseconds?: number; } +export declare const WORKFLOW_RUNS_RETRY_POLICY: WorkflowRunsRetryPolicy; +export declare class WorkflowQueueDeadlineError extends Error { + constructor(); +} +export interface WorkflowRunsRetryDependencies { + delayPort: WorkflowPollingDelayPort; + clock: WorkflowQueueClockPort; + random: WorkflowPollingRandomPort; + observer?: Pick; + policy: WorkflowRunsRetryPolicy; + deadlineAtMilliseconds: number; +} +export declare function withWorkflowRunsRetry(operation: () => Promise, dependencies: WorkflowRunsRetryDependencies): Promise; export declare function withWorkflowRunsRetry(operation: () => Promise, delayPort: WorkflowPollingDelayPort, policy: WorkflowRunsRetryPolicy): Promise; diff --git a/build/cli/src/infrastructure/logging/logger_workflow_polling_observer_adapter.d.ts b/build/cli/src/infrastructure/logging/logger_workflow_polling_observer_adapter.d.ts index de89f64f0..165c856ad 100644 --- a/build/cli/src/infrastructure/logging/logger_workflow_polling_observer_adapter.d.ts +++ b/build/cli/src/infrastructure/logging/logger_workflow_polling_observer_adapter.d.ts @@ -2,4 +2,10 @@ import type { WorkflowPollingObserverPort } from '../../application/ports/workfl export declare class LoggerWorkflowPollingObserverAdapter implements WorkflowPollingObserverPort { noActivePreviousRuns(): void; waitingForPreviousRuns(activeRunCount: number, delayMilliseconds: number): void; + providerRetry(observation: { + reason: 'rate_limit' | 'transient'; + attempt: number; + delayMilliseconds: number; + resetEpochSeconds?: number; + }): void; } diff --git a/build/cli/src/infrastructure/time/system_workflow_polling_random_adapter.d.ts b/build/cli/src/infrastructure/time/system_workflow_polling_random_adapter.d.ts new file mode 100644 index 000000000..6411f7012 --- /dev/null +++ b/build/cli/src/infrastructure/time/system_workflow_polling_random_adapter.d.ts @@ -0,0 +1,4 @@ +import type { WorkflowPollingRandomPort } from '../../application/ports/workflow_run_ports'; +export declare class SystemWorkflowPollingRandomAdapter implements WorkflowPollingRandomPort { + next(): number; +} diff --git a/build/cli/src/infrastructure/time/system_workflow_queue_clock_adapter.d.ts b/build/cli/src/infrastructure/time/system_workflow_queue_clock_adapter.d.ts new file mode 100644 index 000000000..b452ccda2 --- /dev/null +++ b/build/cli/src/infrastructure/time/system_workflow_queue_clock_adapter.d.ts @@ -0,0 +1,4 @@ +import type { WorkflowQueueClockPort } from '../../application/ports/workflow_run_ports'; +export declare class SystemWorkflowQueueClockAdapter implements WorkflowQueueClockPort { + nowMilliseconds(): number; +} diff --git a/build/github_action/index.js b/build/github_action/index.js index ed371aee2..268ec99ee 100644 --- a/build/github_action/index.js +++ b/build/github_action/index.js @@ -50743,7 +50743,9 @@ function calculateReviewersStillNeeded(desiredCount, currentCount, confirmedCoun "use strict"; Object.defineProperty(exports, "__esModule", ({ value: true })); -exports.COPILOT_WORKFLOW_NAMES = void 0; +exports.WORKFLOW_QUEUE_POLICY = exports.COPILOT_WORKFLOW_NAMES = void 0; +exports.calculateWorkflowPollingDelay = calculateWorkflowPollingDelay; +exports.calculateJitteredWorkflowDelay = calculateJitteredWorkflowDelay; /** * Workflows that execute the Copilot action and therefore share its * repository mutation queue. Keep these names aligned with workflow `name` @@ -50758,6 +50760,22 @@ exports.COPILOT_WORKFLOW_NAMES = [ 'Task - Hotfix', 'Task - Release', ]; +exports.WORKFLOW_QUEUE_POLICY = { + maximumQueueWaitMilliseconds: 90 * 60 * 1000, + initialDelayMilliseconds: 5 * 1000, + backoffMultiplier: 2, + maximumDelayMilliseconds: 60 * 1000, + jitterRatio: 0.2, +}; +function calculateWorkflowPollingDelay(pollIndex, randomValue, policy = exports.WORKFLOW_QUEUE_POLICY) { + const baseDelay = Math.min(policy.initialDelayMilliseconds * policy.backoffMultiplier ** pollIndex, policy.maximumDelayMilliseconds); + return calculateJitteredWorkflowDelay(baseDelay, randomValue, policy); +} +function calculateJitteredWorkflowDelay(baseDelayMilliseconds, randomValue, policy) { + const boundedRandom = Math.min(1, Math.max(0, randomValue)); + const jitter = (boundedRandom * 2 - 1) * policy.jitterRatio; + return Math.min(policy.maximumDelayMilliseconds, Math.max(0, Math.round(baseDelayMilliseconds * (1 + jitter)))); +} /***/ }), @@ -59055,37 +59073,50 @@ exports.CheckPullRequestCommentLanguageUseCase = CheckPullRequestCommentLanguage /***/ }), /***/ 38301: -/***/ ((__unused_webpack_module, exports) => { +/***/ ((__unused_webpack_module, exports, __nccwpck_require__) => { "use strict"; Object.defineProperty(exports, "__esModule", ({ value: true })); exports.WaitForPreviousWorkflowRunsUseCase = void 0; -const DEFAULT_POLICY = { - maximumAttempts: 2000, - delayMilliseconds: 2000, -}; +const workflow_queue_policy_1 = __nccwpck_require__(43193); +const SYSTEM_CLOCK = { nowMilliseconds: () => Date.now() }; +const SYSTEM_RANDOM = { next: () => Math.random() }; class WaitForPreviousWorkflowRunsUseCase { - constructor(queryPort, delayPort, observerPort, policy = DEFAULT_POLICY) { + constructor(queryPort, delayPort, observerPort, policy = workflow_queue_policy_1.WORKFLOW_QUEUE_POLICY, clock = SYSTEM_CLOCK, random = SYSTEM_RANDOM) { this.queryPort = queryPort; this.delayPort = delayPort; this.observerPort = observerPort; this.policy = policy; + this.clock = clock; + this.random = random; this.taskId = 'WaitForPreviousWorkflowRunsUseCase'; } async invoke(query) { - for (let attempt = 0; attempt < this.policy.maximumAttempts; attempt++) { - const activeRunCount = await this.queryPort.countActivePreviousRuns(query); + const deadlineAtMilliseconds = this.clock.nowMilliseconds() + this.policy.maximumQueueWaitMilliseconds; + let pollIndex = 0; + while (true) { + if (this.clock.nowMilliseconds() >= deadlineAtMilliseconds) { + throw new Error('Timeout waiting for previous runs to finish.'); + } + const activeRunCount = await this.queryPort.countActivePreviousRuns(query, { + deadlineAtMilliseconds, + }); + if (this.clock.nowMilliseconds() >= deadlineAtMilliseconds) { + throw new Error('Timeout waiting for previous runs to finish.'); + } if (activeRunCount === 0) { this.observerPort.noActivePreviousRuns(); return; } - if (attempt === this.policy.maximumAttempts - 1) - break; - this.observerPort.waitingForPreviousRuns(activeRunCount, this.policy.delayMilliseconds); - await this.delayPort.wait(this.policy.delayMilliseconds); + const delayMilliseconds = (0, workflow_queue_policy_1.calculateWorkflowPollingDelay)(pollIndex, this.random.next(), this.policy); + if (this.clock.nowMilliseconds() + delayMilliseconds >= deadlineAtMilliseconds) { + throw new Error('Timeout waiting for previous runs to finish.'); + } + this.observerPort.waitingForPreviousRuns(activeRunCount, delayMilliseconds); + await this.delayPort.wait(delayMilliseconds); + pollIndex += 1; } - throw new Error('Timeout waiting for previous runs to finish.'); } } exports.WaitForPreviousWorkflowRunsUseCase = WaitForPreviousWorkflowRunsUseCase; @@ -65280,76 +65311,70 @@ Object.defineProperty(exports, "__esModule", ({ value: true })); exports.ActivePreviousWorkflowRunsRepository = void 0; const constants_1 = __nccwpck_require__(15415); const workflow_runs_retry_1 = __nccwpck_require__(86434); -const DEFAULT_RETRY_POLICY = { - maximumAttempts: 5, - initialDelayMilliseconds: 1000, - backoffMultiplier: 2, - maximumDelayMilliseconds: 8000, -}; -const NO_OP_DELAY_PORT = { - async wait() { - return Promise.resolve(); - }, -}; +const NO_OP_DELAY_PORT = { wait: async () => undefined }; +const SYSTEM_CLOCK = { nowMilliseconds: () => Date.now() }; +const SYSTEM_RANDOM = { next: () => Math.random() }; class ActivePreviousWorkflowRunsRepository { - constructor(client, retryDelayPort = NO_OP_DELAY_PORT, retryPolicy = DEFAULT_RETRY_POLICY) { + constructor(client, retryDelayPort = NO_OP_DELAY_PORT, retryPolicy = workflow_runs_retry_1.WORKFLOW_RUNS_RETRY_POLICY, clock = SYSTEM_CLOCK, random = SYSTEM_RANDOM, observer) { this.client = client; this.retryDelayPort = retryDelayPort; this.retryPolicy = retryPolicy; + this.clock = clock; + this.random = random; + this.observer = observer; } - async countActivePreviousRuns(query) { - const hasWorkflowScope = query.workflowNames?.some((name) => name.trim().length > 0) || query.workflowName.trim().length > 0; - if (!Number.isFinite(query.currentRunId) || !hasWorkflowScope) { - return 0; + async countActivePreviousRuns(query, context = { deadlineAtMilliseconds: Number.POSITIVE_INFINITY }) { + if (!Number.isSafeInteger(query.currentRunId)) { + throw new Error('GitHub workflow identity is unavailable; refusing to bypass sequential execution.'); } - let activeRunCount = 0; - for (const status of constants_1.WORKFLOW_ACTIVE_STATUSES) { - activeRunCount += await this.countActiveRunsForStatus(query, status); + const workflowNames = query.workflowNames?.filter(name => name.trim().length > 0) ?? []; + if (workflowNames.length === 0 && query.workflowName.trim().length === 0) { + throw new Error('GitHub workflow name is unavailable; refusing to bypass sequential execution.'); } - return activeRunCount; - } - async countActiveRunsForStatus(query, status) { - const useWorkflowEndpoint = Boolean(query.workflowIdentifier - && (!query.workflowNames || query.workflowNames.length === 0) - && this.client.rest.actions.listWorkflowRuns); - const method = useWorkflowEndpoint + const method = query.workflowIdentifier && workflowNames.length === 0 ? this.client.rest.actions.listWorkflowRuns : this.client.rest.actions.listWorkflowRunsForRepo; + if (!method) + throw new Error('GitHub workflow-scoped runs endpoint is unavailable.'); const parameters = { owner: query.owner, repo: query.repository, per_page: 100, - status, - ...(useWorkflowEndpoint ? { workflow_id: query.workflowIdentifier } : {}), + ...(method === this.client.rest.actions.listWorkflowRuns && query.workflowIdentifier + ? { workflow_id: query.workflowIdentifier } + : {}), }; + const names = workflowNames.length > 0 ? workflowNames : [query.workflowName]; return (0, workflow_runs_retry_1.withWorkflowRunsRetry)(async () => { let activeRunCount = 0; for await (const response of this.client.paginate.iterator(method, parameters)) { - activeRunCount += countMatchingRuns(this.extractWorkflowRuns(response), query); + activeRunCount += extractWorkflowRuns(response) + .filter(run => isActivePreviousRun(run, query, names)).length; } return activeRunCount; - }, this.retryDelayPort, this.retryPolicy); - } - extractWorkflowRuns(response) { - const data = response?.data; - if (Array.isArray(data)) { - return data; - } - if (data !== null && typeof data === 'object' && Array.isArray(data.workflow_runs)) { - return data.workflow_runs; - } - throw new Error('GitHub workflow runs response did not contain a workflow_runs array.'); + }, { + delayPort: this.retryDelayPort, + clock: this.clock, + random: this.random, + observer: this.observer, + policy: this.retryPolicy, + deadlineAtMilliseconds: context.deadlineAtMilliseconds, + }); } } exports.ActivePreviousWorkflowRunsRepository = ActivePreviousWorkflowRunsRepository; -function countMatchingRuns(runs, query) { - return runs.filter((run) => isActivePreviousRun(run, query)).length; -} -function isActivePreviousRun(run, query) { - const workflowMatches = query.workflowNames && query.workflowNames.length > 0 - ? query.workflowNames.includes(run.name ?? '') - : run.name === query.workflowName; - return workflowMatches +function extractWorkflowRuns(response) { + const data = response?.data; + if (Array.isArray(data)) + return data; + if (data !== null && typeof data === 'object' && Array.isArray(data.workflow_runs)) { + return data.workflow_runs; + } + throw new Error('GitHub workflow runs response did not contain a workflow_runs array.'); +} +function isActivePreviousRun(run, query, workflowNames) { + return typeof run.name === 'string' + && workflowNames.includes(run.name) && run.id < query.currentRunId && constants_1.WORKFLOW_ACTIVE_STATUSES.includes(run.status ?? 'unknown'); } @@ -65385,12 +65410,75 @@ exports.WorkflowDispatchRepository = WorkflowDispatchRepository; /***/ }), /***/ 86434: -/***/ ((__unused_webpack_module, exports) => { +/***/ ((__unused_webpack_module, exports, __nccwpck_require__) => { "use strict"; Object.defineProperty(exports, "__esModule", ({ value: true })); +exports.WorkflowQueueDeadlineError = exports.WORKFLOW_RUNS_RETRY_POLICY = void 0; exports.withWorkflowRunsRetry = withWorkflowRunsRetry; +const workflow_queue_policy_1 = __nccwpck_require__(43193); +exports.WORKFLOW_RUNS_RETRY_POLICY = { + maximumAttempts: 5, + initialDelayMilliseconds: 1000, + backoffMultiplier: 2, + maximumDelayMilliseconds: 30000, + jitterRatio: 0.2, + rateLimitInitialDelayMilliseconds: 60000, + rateLimitMaximumDelayMilliseconds: 300000, +}; +class WorkflowQueueDeadlineError extends Error { + constructor() { + super('Timeout waiting for previous runs to finish.'); + this.name = 'WorkflowQueueDeadlineError'; + } +} +exports.WorkflowQueueDeadlineError = WorkflowQueueDeadlineError; +function withWorkflowRunsRetry(operation, dependenciesOrDelayPort, legacyPolicy) { + const dependencies = 'clock' in dependenciesOrDelayPort + ? dependenciesOrDelayPort + : { + delayPort: dependenciesOrDelayPort, + clock: { nowMilliseconds: () => Date.now() }, + random: { next: () => 0.5 }, + policy: { + ...exports.WORKFLOW_RUNS_RETRY_POLICY, + ...legacyPolicy, + jitterRatio: 0, + }, + deadlineAtMilliseconds: Number.POSITIVE_INFINITY, + }; + return executeWithRetry(operation, dependencies, 1); +} +async function executeWithRetry(operation, dependencies, attempt) { + if (dependencies.clock.nowMilliseconds() >= dependencies.deadlineAtMilliseconds) { + throw new WorkflowQueueDeadlineError(); + } + try { + return await operation(); + } + catch (error) { + const classification = classifyWorkflowRunsError(error, dependencies.clock); + if (!classification.retryable + || (classification.reason === 'transient' && attempt >= dependencies.policy.maximumAttempts)) { + throw error; + } + const delayMilliseconds = retryDelay(classification, attempt, dependencies); + if (dependencies.clock.nowMilliseconds() + delayMilliseconds >= dependencies.deadlineAtMilliseconds) { + throw new WorkflowQueueDeadlineError(); + } + dependencies.observer?.providerRetry?.({ + reason: classification.reason, + attempt, + delayMilliseconds, + ...(classification.resetEpochSeconds === undefined + ? {} + : { resetEpochSeconds: classification.resetEpochSeconds }), + }); + await dependencies.delayPort.wait(delayMilliseconds); + return executeWithRetry(operation, dependencies, attempt + 1); + } +} const TRANSIENT_NETWORK_ERRORS = new Set([ 'ECONNRESET', 'ETIMEDOUT', @@ -65399,38 +65487,87 @@ const TRANSIENT_NETWORK_ERRORS = new Set([ 'ECONNREFUSED', 'UND_ERR_CONNECT_TIMEOUT', ]); -async function withWorkflowRunsRetry(operation, delayPort, policy) { - return executeWithRetry(operation, delayPort, policy, 1, policy.initialDelayMilliseconds); +function retryDelay(classification, attempt, dependencies) { + if (classification.retryAfterMilliseconds !== undefined) + return classification.retryAfterMilliseconds; + const { policy } = dependencies; + const rateLimit = classification.reason === 'rate_limit'; + const baseDelay = Math.min((rateLimit ? (policy.rateLimitInitialDelayMilliseconds ?? 60000) : policy.initialDelayMilliseconds) + * policy.backoffMultiplier ** (attempt - 1), rateLimit ? (policy.rateLimitMaximumDelayMilliseconds ?? 300000) : policy.maximumDelayMilliseconds); + const jitterPolicy = { + maximumDelayMilliseconds: rateLimit + ? (policy.rateLimitMaximumDelayMilliseconds ?? 300000) + : policy.maximumDelayMilliseconds, + jitterRatio: policy.jitterRatio ?? 0, + }; + return (0, workflow_queue_policy_1.calculateJitteredWorkflowDelay)(baseDelay, dependencies.random.next(), jitterPolicy); } -async function executeWithRetry(operation, delayPort, policy, attempt, delayMilliseconds) { - try { - return await operation(); +function classifyWorkflowRunsError(error, clock) { + if (!error || typeof error !== 'object') + return { retryable: false, reason: 'transient' }; + const candidate = error; + const status = firstNumericValue(candidate.status, candidate.statusCode, candidate.response?.status); + const headers = candidate.response?.headers ?? candidate.headers; + const message = [candidate.response?.data?.message, candidate.data?.message, candidate.message] + .find(value => typeof value === 'string'); + const remaining = header(headers, 'x-ratelimit-remaining'); + const isRateLimited = status === 429 + || (status === 403 && (remaining === '0' + || /(?:rate limit|secondary rate|abuse limit|too many requests)/i.test(message ?? ''))); + if (isRateLimited) { + const retryAfterMilliseconds = parseRetryAfter(header(headers, 'retry-after'), clock); + const resetEpochSeconds = parseEpochSeconds(header(headers, 'x-ratelimit-reset')); + const resetDelay = resetEpochSeconds === undefined + ? undefined + : Math.max(0, resetEpochSeconds * 1000 - clock.nowMilliseconds()); + return { + retryable: true, + reason: 'rate_limit', + retryAfterMilliseconds: retryAfterMilliseconds ?? resetDelay, + resetEpochSeconds, + }; } - catch (error) { - if (!shouldRetry(error, attempt, policy.maximumAttempts)) - throw error; - await delayPort.wait(delayMilliseconds); - return executeWithRetry(operation, delayPort, policy, attempt + 1, nextRetryDelay(delayMilliseconds, policy)); + if (status === 408 || (status !== undefined && status >= 500)) { + return { retryable: true, reason: 'transient' }; + } + if (typeof candidate.code === 'string' && TRANSIENT_NETWORK_ERRORS.has(candidate.code)) { + return { retryable: true, reason: 'transient' }; } + return { + retryable: typeof message === 'string' + && /\b(server error|service unavailable|bad gateway|gateway timeout|temporarily unavailable)\b/i.test(message), + reason: 'transient', + }; } -function shouldRetry(error, attempt, maximumAttempts) { - return attempt < maximumAttempts && isTransientWorkflowRunsError(error); +function header(headers, name) { + if (!headers) + return undefined; + if (typeof headers.get === 'function') { + const value = headers.get(name); + return value === undefined || value === null ? undefined : String(value); + } + if (typeof headers !== 'object') + return undefined; + const entry = Object.entries(headers) + .find(([key]) => key.toLowerCase() === name); + return entry?.[1] === undefined || entry?.[1] === null ? undefined : String(entry[1]); } -function nextRetryDelay(currentDelay, policy) { - return Math.min(currentDelay * policy.backoffMultiplier, policy.maximumDelayMilliseconds); +function parseRetryAfter(value, clock) { + if (!value) + return undefined; + const seconds = Number(value); + if (Number.isFinite(seconds) && seconds >= 0) + return Math.round(seconds * 1000); + const timestamp = Date.parse(value); + return Number.isFinite(timestamp) + ? Math.max(0, timestamp - clock.nowMilliseconds()) + : undefined; } -function isTransientWorkflowRunsError(error) { - if (!error || typeof error !== 'object') - return false; - const candidate = error; - const status = firstNumericValue(candidate.status, candidate.statusCode, candidate.response?.status); - if (status !== undefined) { - return status === 408 || status === 429 || status >= 500; - } - if (typeof candidate.code === 'string' && TRANSIENT_NETWORK_ERRORS.has(candidate.code)) - return true; - return typeof candidate.message === 'string' - && /\b(server error|service unavailable|bad gateway|gateway timeout|temporarily unavailable)\b/i.test(candidate.message); +function parseEpochSeconds(value) { + if (!value) + return undefined; + const epoch = Number(value); + return Number.isFinite(epoch) && epoch >= 0 ? epoch : undefined; } function firstNumericValue(...values) { return values.find((value) => typeof value === 'number' && Number.isFinite(value)); @@ -66600,11 +66737,14 @@ const wait_for_previous_workflow_runs_use_case_1 = __nccwpck_require__(38301); const active_previous_workflow_runs_repository_1 = __nccwpck_require__(40941); const timer_workflow_polling_delay_adapter_1 = __nccwpck_require__(10339); const logger_workflow_polling_observer_adapter_1 = __nccwpck_require__(52883); +const system_workflow_queue_clock_adapter_1 = __nccwpck_require__(49664); +const system_workflow_polling_random_adapter_1 = __nccwpck_require__(32679); const github_workflow_client_factory_1 = __nccwpck_require__(29839); function createWaitForPreviousWorkflowRunsUseCase(token) { const client = (0, github_workflow_client_factory_1.createWorkflowRunsClient)().getClient(token); const delayPort = new timer_workflow_polling_delay_adapter_1.TimerWorkflowPollingDelayAdapter(); - return new wait_for_previous_workflow_runs_use_case_1.WaitForPreviousWorkflowRunsUseCase(new active_previous_workflow_runs_repository_1.ActivePreviousWorkflowRunsRepository(client, delayPort), delayPort, new logger_workflow_polling_observer_adapter_1.LoggerWorkflowPollingObserverAdapter()); + const observerPort = new logger_workflow_polling_observer_adapter_1.LoggerWorkflowPollingObserverAdapter(); + return new wait_for_previous_workflow_runs_use_case_1.WaitForPreviousWorkflowRunsUseCase(new active_previous_workflow_runs_repository_1.ActivePreviousWorkflowRunsRepository(client, delayPort, undefined, new system_workflow_queue_clock_adapter_1.SystemWorkflowQueueClockAdapter(), new system_workflow_polling_random_adapter_1.SystemWorkflowPollingRandomAdapter(), observerPort), delayPort, observerPort); } @@ -67051,6 +67191,16 @@ class LoggerWorkflowPollingObserverAdapter { waitingForPreviousRuns(activeRunCount, delayMilliseconds) { (0, logger_1.logDebugInfo)(`⏳ Found ${activeRunCount} previous run(s) still active. Waiting ${delayMilliseconds / 1000}s...`); } + providerRetry(observation) { + (0, logger_1.logDebugInfo)('GitHub workflow polling retry scheduled.', false, { + reason: observation.reason, + attempt: observation.attempt, + delayMilliseconds: observation.delayMilliseconds, + ...(observation.resetEpochSeconds === undefined + ? {} + : { resetEpochSeconds: observation.resetEpochSeconds }), + }); + } } exports.LoggerWorkflowPollingObserverAdapter = LoggerWorkflowPollingObserverAdapter; @@ -67078,6 +67228,40 @@ class SetupWorkspaceAdapter { exports.SetupWorkspaceAdapter = SetupWorkspaceAdapter; +/***/ }), + +/***/ 32679: +/***/ ((__unused_webpack_module, exports) => { + +"use strict"; + +Object.defineProperty(exports, "__esModule", ({ value: true })); +exports.SystemWorkflowPollingRandomAdapter = void 0; +class SystemWorkflowPollingRandomAdapter { + next() { + return Math.random(); + } +} +exports.SystemWorkflowPollingRandomAdapter = SystemWorkflowPollingRandomAdapter; + + +/***/ }), + +/***/ 49664: +/***/ ((__unused_webpack_module, exports) => { + +"use strict"; + +Object.defineProperty(exports, "__esModule", ({ value: true })); +exports.SystemWorkflowQueueClockAdapter = void 0; +class SystemWorkflowQueueClockAdapter { + nowMilliseconds() { + return Date.now(); + } +} +exports.SystemWorkflowQueueClockAdapter = SystemWorkflowQueueClockAdapter; + + /***/ }), /***/ 20846: diff --git a/build/github_action/src/application/policies/workflow_queue_policy.d.ts b/build/github_action/src/application/policies/workflow_queue_policy.d.ts index 67cc43f76..f69afed1f 100644 --- a/build/github_action/src/application/policies/workflow_queue_policy.d.ts +++ b/build/github_action/src/application/policies/workflow_queue_policy.d.ts @@ -4,3 +4,13 @@ * values in `.github/workflows` and the setup templates. */ export declare const COPILOT_WORKFLOW_NAMES: readonly ["Copilot - Issue", "Copilot - Issue Comment", "Copilot - Commit", "Copilot - Pull Request", "Copilot - Pull Request Comment", "Task - Hotfix", "Task - Release"]; +export interface WorkflowPollingPolicy { + maximumQueueWaitMilliseconds: number; + initialDelayMilliseconds: number; + backoffMultiplier: number; + maximumDelayMilliseconds: number; + jitterRatio: number; +} +export declare const WORKFLOW_QUEUE_POLICY: WorkflowPollingPolicy; +export declare function calculateWorkflowPollingDelay(pollIndex: number, randomValue: number, policy?: WorkflowPollingPolicy): number; +export declare function calculateJitteredWorkflowDelay(baseDelayMilliseconds: number, randomValue: number, policy: Pick): number; diff --git a/build/github_action/src/application/ports/workflow_run_ports.d.ts b/build/github_action/src/application/ports/workflow_run_ports.d.ts index 9946163a8..5f8c26d4a 100644 --- a/build/github_action/src/application/ports/workflow_run_ports.d.ts +++ b/build/github_action/src/application/ports/workflow_run_ports.d.ts @@ -8,13 +8,29 @@ export interface PreviousWorkflowRunsQuery { /** All Copilot workflow names share one queue so different event workflows cannot overlap. */ workflowNames?: readonly string[]; } +export interface WorkflowQueueRequestContext { + deadlineAtMilliseconds: number; +} export interface PreviousWorkflowRunsQueryPort { - countActivePreviousRuns(query: PreviousWorkflowRunsQuery): Promise; + countActivePreviousRuns(query: PreviousWorkflowRunsQuery, context: WorkflowQueueRequestContext): Promise; } export interface WorkflowPollingDelayPort { wait(milliseconds: number): Promise; } +export interface WorkflowQueueClockPort { + nowMilliseconds(): number; +} +export interface WorkflowPollingRandomPort { + next(): number; +} +export interface WorkflowProviderRetryObservation { + reason: 'rate_limit' | 'transient'; + attempt: number; + delayMilliseconds: number; + resetEpochSeconds?: number; +} export interface WorkflowPollingObserverPort { noActivePreviousRuns(): void; waitingForPreviousRuns(activeRunCount: number, delayMilliseconds: number): void; + providerRetry?(observation: WorkflowProviderRetryObservation): void; } diff --git a/build/github_action/src/application/usecases/workflow/wait_for_previous_workflow_runs_use_case.d.ts b/build/github_action/src/application/usecases/workflow/wait_for_previous_workflow_runs_use_case.d.ts index 74b8f9f4e..938281f16 100644 --- a/build/github_action/src/application/usecases/workflow/wait_for_previous_workflow_runs_use_case.d.ts +++ b/build/github_action/src/application/usecases/workflow/wait_for_previous_workflow_runs_use_case.d.ts @@ -1,15 +1,14 @@ -import type { PreviousWorkflowRunsQuery, PreviousWorkflowRunsQueryPort, WorkflowPollingDelayPort, WorkflowPollingObserverPort } from '../../ports/workflow_run_ports'; +import type { PreviousWorkflowRunsQuery, PreviousWorkflowRunsQueryPort, WorkflowPollingDelayPort, WorkflowPollingObserverPort, WorkflowPollingRandomPort, WorkflowQueueClockPort } from '../../ports/workflow_run_ports'; +import { type WorkflowPollingPolicy } from '../../policies/workflow_queue_policy'; import type { ParamUseCase } from '../base/param_usecase'; -export interface WorkflowPollingPolicy { - maximumAttempts: number; - delayMilliseconds: number; -} export declare class WaitForPreviousWorkflowRunsUseCase implements ParamUseCase { private readonly queryPort; private readonly delayPort; private readonly observerPort; private readonly policy; + private readonly clock; + private readonly random; taskId: string; - constructor(queryPort: PreviousWorkflowRunsQueryPort, delayPort: WorkflowPollingDelayPort, observerPort: WorkflowPollingObserverPort, policy?: WorkflowPollingPolicy); + constructor(queryPort: PreviousWorkflowRunsQueryPort, delayPort: WorkflowPollingDelayPort, observerPort: WorkflowPollingObserverPort, policy?: WorkflowPollingPolicy, clock?: WorkflowQueueClockPort, random?: WorkflowPollingRandomPort); invoke(query: PreviousWorkflowRunsQuery): Promise; } diff --git a/build/github_action/src/data/repository/workflow/active_previous_workflow_runs_repository.d.ts b/build/github_action/src/data/repository/workflow/active_previous_workflow_runs_repository.d.ts index 50b528708..778c1a68c 100644 --- a/build/github_action/src/data/repository/workflow/active_previous_workflow_runs_repository.d.ts +++ b/build/github_action/src/data/repository/workflow/active_previous_workflow_runs_repository.d.ts @@ -1,12 +1,13 @@ -import type { PreviousWorkflowRunsQuery, PreviousWorkflowRunsQueryPort, WorkflowPollingDelayPort } from '../../../application/ports/workflow_run_ports'; +import type { PreviousWorkflowRunsQuery, PreviousWorkflowRunsQueryPort, WorkflowPollingDelayPort, WorkflowPollingObserverPort, WorkflowPollingRandomPort, WorkflowQueueClockPort, WorkflowQueueRequestContext } from '../../../application/ports/workflow_run_ports'; import type { GithubWorkflowRunsClient } from '../../../infrastructure/github/ports/github_workflow_provider_ports'; import { type WorkflowRunsRetryPolicy } from './workflow_runs_retry'; export declare class ActivePreviousWorkflowRunsRepository implements PreviousWorkflowRunsQueryPort { private readonly client; private readonly retryDelayPort; private readonly retryPolicy; - constructor(client: GithubWorkflowRunsClient, retryDelayPort?: WorkflowPollingDelayPort, retryPolicy?: WorkflowRunsRetryPolicy); - countActivePreviousRuns(query: PreviousWorkflowRunsQuery): Promise; - private countActiveRunsForStatus; - private extractWorkflowRuns; + private readonly clock; + private readonly random; + private readonly observer?; + constructor(client: GithubWorkflowRunsClient, retryDelayPort?: WorkflowPollingDelayPort, retryPolicy?: WorkflowRunsRetryPolicy, clock?: WorkflowQueueClockPort, random?: WorkflowPollingRandomPort, observer?: WorkflowPollingObserverPort | undefined); + countActivePreviousRuns(query: PreviousWorkflowRunsQuery, context?: WorkflowQueueRequestContext): Promise; } diff --git a/build/github_action/src/data/repository/workflow/workflow_runs_retry.d.ts b/build/github_action/src/data/repository/workflow/workflow_runs_retry.d.ts index 4f5bc5cfa..269efca75 100644 --- a/build/github_action/src/data/repository/workflow/workflow_runs_retry.d.ts +++ b/build/github_action/src/data/repository/workflow/workflow_runs_retry.d.ts @@ -1,8 +1,24 @@ -import type { WorkflowPollingDelayPort } from '../../../application/ports/workflow_run_ports'; +import type { WorkflowPollingDelayPort, WorkflowPollingObserverPort, WorkflowPollingRandomPort, WorkflowQueueClockPort } from '../../../application/ports/workflow_run_ports'; export interface WorkflowRunsRetryPolicy { maximumAttempts: number; initialDelayMilliseconds: number; backoffMultiplier: number; maximumDelayMilliseconds: number; + jitterRatio?: number; + rateLimitInitialDelayMilliseconds?: number; + rateLimitMaximumDelayMilliseconds?: number; } +export declare const WORKFLOW_RUNS_RETRY_POLICY: WorkflowRunsRetryPolicy; +export declare class WorkflowQueueDeadlineError extends Error { + constructor(); +} +export interface WorkflowRunsRetryDependencies { + delayPort: WorkflowPollingDelayPort; + clock: WorkflowQueueClockPort; + random: WorkflowPollingRandomPort; + observer?: Pick; + policy: WorkflowRunsRetryPolicy; + deadlineAtMilliseconds: number; +} +export declare function withWorkflowRunsRetry(operation: () => Promise, dependencies: WorkflowRunsRetryDependencies): Promise; export declare function withWorkflowRunsRetry(operation: () => Promise, delayPort: WorkflowPollingDelayPort, policy: WorkflowRunsRetryPolicy): Promise; diff --git a/build/github_action/src/infrastructure/logging/logger_workflow_polling_observer_adapter.d.ts b/build/github_action/src/infrastructure/logging/logger_workflow_polling_observer_adapter.d.ts index de89f64f0..165c856ad 100644 --- a/build/github_action/src/infrastructure/logging/logger_workflow_polling_observer_adapter.d.ts +++ b/build/github_action/src/infrastructure/logging/logger_workflow_polling_observer_adapter.d.ts @@ -2,4 +2,10 @@ import type { WorkflowPollingObserverPort } from '../../application/ports/workfl export declare class LoggerWorkflowPollingObserverAdapter implements WorkflowPollingObserverPort { noActivePreviousRuns(): void; waitingForPreviousRuns(activeRunCount: number, delayMilliseconds: number): void; + providerRetry(observation: { + reason: 'rate_limit' | 'transient'; + attempt: number; + delayMilliseconds: number; + resetEpochSeconds?: number; + }): void; } diff --git a/build/github_action/src/infrastructure/time/system_workflow_polling_random_adapter.d.ts b/build/github_action/src/infrastructure/time/system_workflow_polling_random_adapter.d.ts new file mode 100644 index 000000000..6411f7012 --- /dev/null +++ b/build/github_action/src/infrastructure/time/system_workflow_polling_random_adapter.d.ts @@ -0,0 +1,4 @@ +import type { WorkflowPollingRandomPort } from '../../application/ports/workflow_run_ports'; +export declare class SystemWorkflowPollingRandomAdapter implements WorkflowPollingRandomPort { + next(): number; +} diff --git a/build/github_action/src/infrastructure/time/system_workflow_queue_clock_adapter.d.ts b/build/github_action/src/infrastructure/time/system_workflow_queue_clock_adapter.d.ts new file mode 100644 index 000000000..b452ccda2 --- /dev/null +++ b/build/github_action/src/infrastructure/time/system_workflow_queue_clock_adapter.d.ts @@ -0,0 +1,4 @@ +import type { WorkflowQueueClockPort } from '../../application/ports/workflow_run_ports'; +export declare class SystemWorkflowQueueClockAdapter implements WorkflowQueueClockPort { + nowMilliseconds(): number; +} diff --git a/docs/development/architecture.mdx b/docs/development/architecture.mdx index 520310dcb..7f3bbea9e 100644 --- a/docs/development/architecture.mdx +++ b/docs/development/architecture.mdx @@ -16,3 +16,20 @@ The repository separates semantic application ports from provider-specific adapt provider-specific CLI adapters remain behind those ports. The application layer MUST NOT import GitHub SDKs, concrete CLIs, process libraries, or provider-specific protocols. Provider adapters MUST NOT define business policy. Configuration validation belongs at the boundary before execution. Prompt construction, untrusted-content handling, and GitHub publication sanitization are separate policies so no agent capability can bypass the security boundary. + +## Workflow queue boundary + +The repository-wide mutation queue is an application use case backed by semantic +ports. `workflow_queue_policy.ts` owns the seven shared workflow names, the 90-minute +queue budget, adaptive polling schedule, jitter bounds, and retry budgets. The use +case receives a clock and random-value port so deadline and delay behavior remain +deterministic in tests; concrete system clock/random and timer adapters are wired in +`workflow_queue_composition_root.ts`. + +`ActivePreviousWorkflowRunsRepository` performs one paginated repository traversal +per poll and filters workflow names, active statuses, and lower run IDs locally. +Provider errors remain at the repository boundary: rate-limited 429/403 responses, +transient HTTP/network failures, server wait headers, and malformed pages are +classified there and never become a synthetic zero count. Retry logging exposes only +sanitized reason, attempt, delay, and reset metadata. Queue-bearing jobs use a +120-minute workflow timeout, leaving 30 minutes of headroom beyond the queue budget. diff --git a/docs/development/testing.mdx b/docs/development/testing.mdx index 3549b358f..47d6dd2f6 100644 --- a/docs/development/testing.mdx +++ b/docs/development/testing.mdx @@ -19,6 +19,14 @@ git diff --check Tests MUST isolate environment variables such as `AGENT_PROVIDER`, `AGENT_MODEL_PROVIDER`, `AGENT_MODEL`, and `AGENT_COMMAND`. Real provider smoke tests are separate from deterministic unit tests and MUST NOT run without authorization and cost controls. Coverage thresholds are enforced by Jest and must not be reduced to accommodate a new code path; add focused tests instead. +Workflow queue tests inject a clock, random source, and scheduler/delay port. They +cover one repository traversal with mixed statuses and later-page matches, strict +lower-ID/name filtering, malformed responses, fail-closed provider errors, 429 and +rate-limited 403 headers, bounded fallback backoff/jitter, and the absolute deadline. +The workflow contract test keeps the seven workflow names synchronized with the +validator and verifies the `90m queue / 120m job` budget, required runners, action +inputs, missing/short timeouts, and forbidden mutation-workflow concurrency. + Architecture tests additionally verify that production imports remain acyclic, application code does not depend on concrete adapters, and pure model/policy code remains free of runtime, provider, and logging dependencies. Refresh the diff --git a/docs/features.mdx b/docs/features.mdx index decc8f6da..27bcc49fb 100644 --- a/docs/features.mdx +++ b/docs/features.mdx @@ -132,10 +132,10 @@ GitHub's native [concurrency](https://docs.github.com/en/actions/using-workflows ### How it works 1. At the start of each run (except welcome/single-action-only flows), the action resolves the current workflow file from `GITHUB_WORKFLOW_REF`. -2. It asks GitHub for only the active runs in the repository (`in_progress`, `queued`, `requested`, `waiting`, and `pending`) instead of scanning the complete run history. -3. It filters the known Copilot/Task mutation workflow names and keeps only runs with a **lower run ID** (i.e. started earlier). Transient GitHub API failures are retried with exponential backoff. -4. If any such run exists, the action waits 2 seconds and checks again, up to a long timeout (~4000 seconds). -5. When no earlier active run in the mutation queue remains, the action continues. +2. It performs one paginated repository workflow-runs traversal per poll with `per_page: 100`, then locally filters the active statuses (`in_progress`, `queued`, `requested`, `waiting`, and `pending`), the seven known Copilot/Task mutation workflow names, and runs with a **lower run ID** (i.e. started earlier). +3. Provider failures fail closed. Transient 408/5xx/network errors use bounded exponential retry; HTTP 429 and rate-limited 403 responses honor `Retry-After` or `x-ratelimit-reset`, then use a slower bounded fallback. Diagnostics contain only the retry reason, attempt, delay, and safe reset timestamp metadata. +4. If any such run exists, the action polls immediately and then uses adaptive 5s, 10s, 20s, 40s, and 60s maximum delays with bounded ±20% jitter. The absolute queue wait is limited to 90 minutes. +5. When no earlier active run in the mutation queue remains, the action continues. A provider failure or queue deadline never becomes an empty result, so setup and mutation work cannot proceed with an unknown queue state. So you get a **repository-wide mutation queue**: multiple triggers for the same workflow (e.g. many issue edits) and related Copilot workflows run sequentially. This conservative scope prevents two workflows from changing shared branches, issue metadata, or release state at the same time. Read-only CI may keep its own native concurrency policy. @@ -159,7 +159,7 @@ jobs: project-ids: '2,3' ``` -If you prefer to cancel the previous run when a new one is triggered (e.g. for PRs, so only the latest run matters), use `cancel-in-progress: true` and the same `concurrency.group` per PR or issue. +Mutation workflows deliberately do not use a GitHub Actions `concurrency` block: native cancellation retains only one pending run and can discard intermediate issue, branch, or release mutations. Native concurrency remains appropriate for replaceable read-only workflows such as CI and RepoWise. --- diff --git a/scripts/validate-workflow-contract.cjs b/scripts/validate-workflow-contract.cjs index 5ccf9e8a3..c1a52cf6e 100644 --- a/scripts/validate-workflow-contract.cjs +++ b/scripts/validate-workflow-contract.cjs @@ -9,28 +9,31 @@ const workflowDirectories = [ path.join(repositoryRoot, '.github', 'workflows'), path.join(repositoryRoot, 'setup', 'workflows'), ]; +const QUEUE_WAIT_MINUTES = 90; +const MIN_QUEUE_JOB_TIMEOUT_MINUTES = 120; +const QUEUE_WORKFLOW_MANIFEST = Object.freeze([ + ['copilot_commit.yml', 'Copilot - Commit', 'copilot-commits'], + ['copilot_issue.yml', 'Copilot - Issue', 'copilot-issues'], + ['copilot_issue_comment.yml', 'Copilot - Issue Comment', 'copilot-issues'], + ['copilot_pull_request.yml', 'Copilot - Pull Request', 'copilot-pull-requests'], + ['copilot_pull_request_comment.yml', 'Copilot - Pull Request Comment', 'copilot-pull-requests'], + ['hotfix_workflow.yml', 'Task - Hotfix', 'tag'], + ['release_workflow.yml', 'Task - Release', 'tag'], +].map(([file, workflowName, jobId]) => ({ file, workflowName, jobId }))); const requiredAgentInputs = [ - 'agent-provider', - 'agent-model-provider', - 'agent-model', - 'agent-effort', - 'agent-command', - 'findings-provider', - 'findings-model-provider', - 'findings-model', - 'findings-effort', - 'findings-command', - 'fixer-provider', - 'fixer-model-provider', - 'fixer-model', - 'fixer-effort', - 'fixer-command', + 'agent-provider', 'agent-model-provider', 'agent-model', 'agent-effort', 'agent-command', + 'findings-provider', 'findings-model-provider', 'findings-model', 'findings-effort', 'findings-command', + 'fixer-provider', 'fixer-model-provider', 'fixer-model', 'fixer-effort', 'fixer-command', ]; function workflowFiles(directory) { return readdirSync(directory) - .filter((name) => name.endsWith('.yml') || name.endsWith('.yaml')) - .map((name) => path.join(directory, name)); + .filter(name => name.endsWith('.yml') || name.endsWith('.yaml')) + .map(name => path.join(directory, name)); +} + +function relativeWorkflow(file) { + return path.relative(repositoryRoot, file).replaceAll(path.sep, '/'); } function isCopilotAction(step) { @@ -43,35 +46,26 @@ function runnerLabels(value) { } function assertRunner(file, workflow) { - const relativeFile = path.relative(repositoryRoot, file); + const relativeFile = relativeWorkflow(file); const expected = relativeFile.startsWith('setup/workflows/') ? ['ubuntu-latest'] : relativeFile === '.github/workflows/repowise.yml' ? ['self-hosted', 'coolify'] : ['self-hosted', 'codex']; - for (const [jobId, job] of Object.entries(workflow.jobs ?? {})) { const labels = runnerLabels(job['runs-on']); - if (expected.length === 1 ? labels[0] !== expected[0] : expected.some((label) => !labels.includes(label))) { + if (expected.length === 1 ? labels[0] !== expected[0] : expected.some(label => !labels.includes(label))) { throw new Error(`${relativeFile} job ${jobId} must use runs-on ${expected.join(', ')}.`); } } } -function assertSequentialMutationWorkflow(file, workflow) { - const relativeFile = path.relative(repositoryRoot, file); - if (!/\.github\/workflows\/(?:copilot_|hotfix_workflow|release_workflow)/.test(relativeFile)) return; - if (workflow.concurrency !== undefined) { - throw new Error(`${relativeFile} must not define GitHub concurrency: it can discard intermediate mutation runs.`); - } -} - function assertAgentInputs(file, workflow) { - const relativeFile = path.relative(repositoryRoot, file); + const relativeFile = relativeWorkflow(file); for (const [jobId, job] of Object.entries(workflow.jobs ?? {})) { for (const [stepIndex, step] of (job.steps ?? []).entries()) { if (!isCopilotAction(step)) continue; - const missing = requiredAgentInputs.filter((input) => !(input in (step.with ?? {}))); + const missing = requiredAgentInputs.filter(input => !(input in (step.with ?? {}))); if (missing.length > 0) { throw new Error(`${relativeFile} job ${jobId} step ${stepIndex + 1} is missing agent inputs: ${missing.join(', ')}.`); } @@ -79,25 +73,75 @@ function assertAgentInputs(file, workflow) { } } -const files = workflowDirectories.flatMap(workflowFiles); -const errors = []; -for (const file of files) { - try { - const workflow = yaml.load(readFileSync(file, 'utf8')); - if (!workflow || typeof workflow !== 'object') throw new Error('workflow document is empty.'); - assertRunner(file, workflow); - assertSequentialMutationWorkflow(file, workflow); - assertAgentInputs(file, workflow); - } catch (error) { - errors.push(`${path.relative(repositoryRoot, file)}: ${error instanceof Error ? error.message : String(error)}`); +function assertQueueWorkflow(file, workflow) { + const relativeFile = relativeWorkflow(file); + const manifest = QUEUE_WORKFLOW_MANIFEST.find(entry => relativeFile.endsWith(`/${entry.file}`)); + if (!manifest) return; + if (workflow.name !== manifest.workflowName) { + throw new Error(`${relativeFile} must have workflow name ${JSON.stringify(manifest.workflowName)}.`); + } + const queueJob = workflow.jobs?.[manifest.jobId]; + if (!queueJob) throw new Error(`${relativeFile} must define queue job ${manifest.jobId}.`); + if (typeof queueJob['timeout-minutes'] !== 'number' + || queueJob['timeout-minutes'] < MIN_QUEUE_JOB_TIMEOUT_MINUTES) { + throw new Error(`${relativeFile} queue job ${manifest.jobId} must have timeout-minutes >= ${MIN_QUEUE_JOB_TIMEOUT_MINUTES}.`); + } + if (workflow.concurrency !== undefined || queueJob.concurrency !== undefined) { + throw new Error(`${relativeFile} must not define workflow or queue-job concurrency.`); + } + if (!(queueJob.steps ?? []).some(isCopilotAction)) { + throw new Error(`${relativeFile} queue job ${manifest.jobId} must invoke the Copilot action.`); + } + for (const [jobId, job] of Object.entries(workflow.jobs ?? {})) { + if (jobId !== manifest.jobId && (job.steps ?? []).some(isCopilotAction)) { + throw new Error(`${relativeFile} unmanifested job ${jobId} invokes the Copilot action.`); + } + } +} + +function assertSequentialMutationWorkflow(file, workflow) { + const relativeFile = relativeWorkflow(file); + if (!QUEUE_WORKFLOW_MANIFEST.some(entry => relativeFile.endsWith(`/${entry.file}`))) return; + if (workflow.concurrency !== undefined) { + throw new Error(`${relativeFile} must not define GitHub concurrency.`); } } -if (errors.length > 0) { - console.error(errors.join('\n')); - process.exitCode = 1; -} else { - console.log(`workflow contract validation: PASS (${files.length} workflows)`); +function validateWorkflow(file, workflow) { + if (!workflow || typeof workflow !== 'object') throw new Error('workflow document is empty.'); + assertRunner(file, workflow); + assertSequentialMutationWorkflow(file, workflow); + assertAgentInputs(file, workflow); + assertQueueWorkflow(file, workflow); } -module.exports = { assertAgentInputs, assertRunner, assertSequentialMutationWorkflow }; +function main() { + const files = workflowDirectories.flatMap(workflowFiles); + const errors = []; + for (const file of files) { + try { + validateWorkflow(file, yaml.load(readFileSync(file, 'utf8'))); + } catch (error) { + errors.push(`${relativeWorkflow(file)}: ${error instanceof Error ? error.message : String(error)}`); + } + } + if (errors.length > 0) { + console.error(errors.join('\n')); + process.exitCode = 1; + } else { + console.log(`workflow contract validation: PASS (${files.length} workflows, ${QUEUE_WAIT_MINUTES}m queue / ${MIN_QUEUE_JOB_TIMEOUT_MINUTES}m job budget)`); + } +} + +if (require.main === module) main(); + +module.exports = { + QUEUE_WAIT_MINUTES, + MIN_QUEUE_JOB_TIMEOUT_MINUTES, + QUEUE_WORKFLOW_MANIFEST, + assertAgentInputs, + assertQueueWorkflow, + assertRunner, + assertSequentialMutationWorkflow, + validateWorkflow, +}; diff --git a/setup/workflows/copilot_commit.yml b/setup/workflows/copilot_commit.yml index e1914b80a..d795c3e3b 100644 --- a/setup/workflows/copilot_commit.yml +++ b/setup/workflows/copilot_commit.yml @@ -11,6 +11,7 @@ jobs: copilot-commits: name: Copilot - Commit runs-on: ubuntu-latest + timeout-minutes: 120 steps: - name: Checkout Repository uses: actions/checkout@v5 diff --git a/setup/workflows/copilot_issue.yml b/setup/workflows/copilot_issue.yml index ba1d41540..bdd00f71b 100644 --- a/setup/workflows/copilot_issue.yml +++ b/setup/workflows/copilot_issue.yml @@ -8,7 +8,7 @@ jobs: copilot-issues: name: Copilot - Issue runs-on: ubuntu-latest - timeout-minutes: 30 + timeout-minutes: 120 permissions: contents: read steps: diff --git a/setup/workflows/copilot_issue_comment.yml b/setup/workflows/copilot_issue_comment.yml index 18b5026fe..5baabae1b 100644 --- a/setup/workflows/copilot_issue_comment.yml +++ b/setup/workflows/copilot_issue_comment.yml @@ -8,7 +8,7 @@ jobs: copilot-issues: name: Copilot - Issue Comment runs-on: ubuntu-latest - timeout-minutes: 30 + timeout-minutes: 120 permissions: contents: write steps: diff --git a/setup/workflows/copilot_pull_request.yml b/setup/workflows/copilot_pull_request.yml index 76cd491c9..66f7eacee 100644 --- a/setup/workflows/copilot_pull_request.yml +++ b/setup/workflows/copilot_pull_request.yml @@ -8,7 +8,7 @@ jobs: copilot-pull-requests: name: Copilot - Pull Request runs-on: ubuntu-latest - timeout-minutes: 30 + timeout-minutes: 120 steps: - name: Checkout Repository uses: actions/checkout@v5 diff --git a/setup/workflows/copilot_pull_request_comment.yml b/setup/workflows/copilot_pull_request_comment.yml index 6bfbc92a7..383a1ef95 100644 --- a/setup/workflows/copilot_pull_request_comment.yml +++ b/setup/workflows/copilot_pull_request_comment.yml @@ -8,7 +8,7 @@ jobs: copilot-pull-requests: name: Copilot - Pull Request Comment runs-on: ubuntu-latest - timeout-minutes: 30 + timeout-minutes: 120 permissions: contents: write steps: diff --git a/setup/workflows/hotfix_workflow.yml b/setup/workflows/hotfix_workflow.yml index adec993f2..4a7fef997 100644 --- a/setup/workflows/hotfix_workflow.yml +++ b/setup/workflows/hotfix_workflow.yml @@ -86,7 +86,7 @@ jobs: tag: name: Publish version runs-on: ubuntu-latest - timeout-minutes: 30 + timeout-minutes: 120 needs: [ prepare-version-files ] permissions: contents: read diff --git a/setup/workflows/release_workflow.yml b/setup/workflows/release_workflow.yml index 970cd384e..c429a92d2 100644 --- a/setup/workflows/release_workflow.yml +++ b/setup/workflows/release_workflow.yml @@ -86,7 +86,7 @@ jobs: tag: name: Publish version runs-on: ubuntu-latest - timeout-minutes: 30 + timeout-minutes: 120 needs: [ prepare-version-files ] permissions: contents: read diff --git a/src/application/policies/workflow_queue_policy.ts b/src/application/policies/workflow_queue_policy.ts index cb94e2b9f..6cf6d870c 100644 --- a/src/application/policies/workflow_queue_policy.ts +++ b/src/application/policies/workflow_queue_policy.ts @@ -12,3 +12,44 @@ export const COPILOT_WORKFLOW_NAMES = [ 'Task - Hotfix', 'Task - Release', ] as const; + +export interface WorkflowPollingPolicy { + maximumQueueWaitMilliseconds: number; + initialDelayMilliseconds: number; + backoffMultiplier: number; + maximumDelayMilliseconds: number; + jitterRatio: number; +} + +export const WORKFLOW_QUEUE_POLICY: WorkflowPollingPolicy = { + maximumQueueWaitMilliseconds: 90 * 60 * 1000, + initialDelayMilliseconds: 5 * 1000, + backoffMultiplier: 2, + maximumDelayMilliseconds: 60 * 1000, + jitterRatio: 0.2, +}; + +export function calculateWorkflowPollingDelay( + pollIndex: number, + randomValue: number, + policy: WorkflowPollingPolicy = WORKFLOW_QUEUE_POLICY, +): number { + const baseDelay = Math.min( + policy.initialDelayMilliseconds * policy.backoffMultiplier ** pollIndex, + policy.maximumDelayMilliseconds, + ); + return calculateJitteredWorkflowDelay(baseDelay, randomValue, policy); +} + +export function calculateJitteredWorkflowDelay( + baseDelayMilliseconds: number, + randomValue: number, + policy: Pick, +): number { + const boundedRandom = Math.min(1, Math.max(0, randomValue)); + const jitter = (boundedRandom * 2 - 1) * policy.jitterRatio; + return Math.min( + policy.maximumDelayMilliseconds, + Math.max(0, Math.round(baseDelayMilliseconds * (1 + jitter))), + ); +} diff --git a/src/application/ports/workflow_run_ports.ts b/src/application/ports/workflow_run_ports.ts index 9d3b3746b..41b78f22a 100644 --- a/src/application/ports/workflow_run_ports.ts +++ b/src/application/ports/workflow_run_ports.ts @@ -9,15 +9,35 @@ export interface PreviousWorkflowRunsQuery { workflowNames?: readonly string[]; } +export interface WorkflowQueueRequestContext { + deadlineAtMilliseconds: number; +} + export interface PreviousWorkflowRunsQueryPort { - countActivePreviousRuns(query: PreviousWorkflowRunsQuery): Promise; + countActivePreviousRuns(query: PreviousWorkflowRunsQuery, context: WorkflowQueueRequestContext): Promise; } export interface WorkflowPollingDelayPort { wait(milliseconds: number): Promise; } +export interface WorkflowQueueClockPort { + nowMilliseconds(): number; +} + +export interface WorkflowPollingRandomPort { + next(): number; +} + +export interface WorkflowProviderRetryObservation { + reason: 'rate_limit' | 'transient'; + attempt: number; + delayMilliseconds: number; + resetEpochSeconds?: number; +} + export interface WorkflowPollingObserverPort { noActivePreviousRuns(): void; waitingForPreviousRuns(activeRunCount: number, delayMilliseconds: number): void; + providerRetry?(observation: WorkflowProviderRetryObservation): void; } diff --git a/src/application/usecases/workflow/__tests__/wait_for_previous_workflow_runs_use_case.test.ts b/src/application/usecases/workflow/__tests__/wait_for_previous_workflow_runs_use_case.test.ts index d13fb3464..32f4b9617 100644 --- a/src/application/usecases/workflow/__tests__/wait_for_previous_workflow_runs_use_case.test.ts +++ b/src/application/usecases/workflow/__tests__/wait_for_previous_workflow_runs_use_case.test.ts @@ -3,109 +3,113 @@ import type { PreviousWorkflowRunsQueryPort, WorkflowPollingDelayPort, WorkflowPollingObserverPort, + WorkflowPollingRandomPort, + WorkflowQueueClockPort, } from '../../../ports/workflow_run_ports'; +import { WORKFLOW_QUEUE_POLICY, type WorkflowPollingPolicy } from '../../../policies/workflow_queue_policy'; import { WaitForPreviousWorkflowRunsUseCase } from '../wait_for_previous_workflow_runs_use_case'; const query: PreviousWorkflowRunsQuery = { owner: 'org', repository: 'repo', currentRunId: 200, - workflowName: 'CI', + workflowName: 'Copilot - Issue', }; function observer(): jest.Mocked { return { noActivePreviousRuns: jest.fn(), waitingForPreviousRuns: jest.fn(), + providerRetry: jest.fn(), }; } +function policy(overrides: Partial = {}): WorkflowPollingPolicy { + return { ...WORKFLOW_QUEUE_POLICY, ...overrides }; +} + describe('WaitForPreviousWorkflowRunsUseCase', () => { - it('returns immediately and reports when no previous runs are active', async () => { + it('queries immediately and reports when no previous runs are active', async () => { const queryPort: PreviousWorkflowRunsQueryPort = { countActivePreviousRuns: jest.fn().mockResolvedValue(0), }; const delayPort: WorkflowPollingDelayPort = { wait: jest.fn() }; const observerPort = observer(); - const useCase = new WaitForPreviousWorkflowRunsUseCase(queryPort, delayPort, observerPort); + const clock: WorkflowQueueClockPort = { nowMilliseconds: () => 1000 }; + const useCase = new WaitForPreviousWorkflowRunsUseCase(queryPort, delayPort, observerPort, policy(), clock, { next: () => 0.5 }); await useCase.invoke(query); - expect(queryPort.countActivePreviousRuns).toHaveBeenCalledWith(query); + expect(queryPort.countActivePreviousRuns).toHaveBeenCalledWith(query, { deadlineAtMilliseconds: 5401000 }); expect(delayPort.wait).not.toHaveBeenCalled(); expect(observerPort.noActivePreviousRuns).toHaveBeenCalledTimes(1); }); - it('reports and waits between queries until no previous run remains', async () => { + it('uses deterministic exponential polling capped at one minute', async () => { const queryPort: PreviousWorkflowRunsQueryPort = { countActivePreviousRuns: jest.fn() .mockResolvedValueOnce(2) .mockResolvedValueOnce(1) - .mockResolvedValueOnce(0), - }; - const delayPort: WorkflowPollingDelayPort = { wait: jest.fn().mockResolvedValue(undefined) }; - const observerPort = observer(); - const useCase = new WaitForPreviousWorkflowRunsUseCase(queryPort, delayPort, observerPort, { - maximumAttempts: 3, - delayMilliseconds: 25, - }); - - await useCase.invoke(query); - - expect(queryPort.countActivePreviousRuns).toHaveBeenCalledTimes(3); - expect(delayPort.wait).toHaveBeenNthCalledWith(1, 25); - expect(delayPort.wait).toHaveBeenNthCalledWith(2, 25); - expect(observerPort.waitingForPreviousRuns).toHaveBeenNthCalledWith(1, 2, 25); - expect(observerPort.waitingForPreviousRuns).toHaveBeenNthCalledWith(2, 1, 25); - expect(observerPort.noActivePreviousRuns).toHaveBeenCalledTimes(1); - }); - - it('uses the historical two-second polling interval by default', async () => { - const queryPort: PreviousWorkflowRunsQueryPort = { - countActivePreviousRuns: jest.fn() .mockResolvedValueOnce(1) .mockResolvedValueOnce(0), }; - const delayPort: WorkflowPollingDelayPort = { wait: jest.fn().mockResolvedValue(undefined) }; + const delays: number[] = []; + const delayPort: WorkflowPollingDelayPort = { + wait: jest.fn(async milliseconds => { delays.push(milliseconds); }), + }; const observerPort = observer(); + const useCase = new WaitForPreviousWorkflowRunsUseCase( + queryPort, + delayPort, + observerPort, + policy({ maximumQueueWaitMilliseconds: 10 * 60 * 1000 }), + { nowMilliseconds: () => 0 }, + { next: () => 0.5 }, + ); - await new WaitForPreviousWorkflowRunsUseCase(queryPort, delayPort, observerPort).invoke(query); + await useCase.invoke(query); - expect(delayPort.wait).toHaveBeenCalledWith(2000); - expect(observerPort.waitingForPreviousRuns).toHaveBeenCalledWith(1, 2000); + expect(delays).toEqual([5000, 10000, 20000]); + expect(observerPort.waitingForPreviousRuns).toHaveBeenNthCalledWith(1, 2, 5000); + expect(observerPort.waitingForPreviousRuns).toHaveBeenNthCalledWith(2, 1, 10000); }); - it('uses 2000 attempts by default', async () => { + it('applies injected jitter and fails closed at the queue deadline', async () => { const queryPort: PreviousWorkflowRunsQueryPort = { countActivePreviousRuns: jest.fn().mockResolvedValue(1), }; const delayPort: WorkflowPollingDelayPort = { wait: jest.fn().mockResolvedValue(undefined) }; const observerPort = observer(); - const useCase = new WaitForPreviousWorkflowRunsUseCase(queryPort, delayPort, observerPort); + let now = 0; + const clock: WorkflowQueueClockPort = { nowMilliseconds: () => now }; + delayPort.wait = jest.fn(async milliseconds => { now += milliseconds; }); + const useCase = new WaitForPreviousWorkflowRunsUseCase( + queryPort, + delayPort, + observerPort, + policy({ maximumQueueWaitMilliseconds: 10000 }), + clock, + { next: () => 0 }, + ); await expect(useCase.invoke(query)).rejects.toThrow('Timeout waiting for previous runs to finish.'); - - expect(queryPort.countActivePreviousRuns).toHaveBeenCalledTimes(2000); - expect(delayPort.wait).toHaveBeenCalledTimes(1999); - expect(delayPort.wait).toHaveBeenCalledWith(2000); + expect(delayPort.wait).toHaveBeenCalledWith(4000); + expect(observerPort.noActivePreviousRuns).not.toHaveBeenCalled(); }); - it('throws after the configured maximum number of active queries', async () => { + it('does not start a provider query after the deadline', async () => { const queryPort: PreviousWorkflowRunsQueryPort = { - countActivePreviousRuns: jest.fn().mockResolvedValue(1), + countActivePreviousRuns: jest.fn().mockResolvedValue(0), }; - const delayPort: WorkflowPollingDelayPort = { wait: jest.fn().mockResolvedValue(undefined) }; const observerPort = observer(); - const useCase = new WaitForPreviousWorkflowRunsUseCase(queryPort, delayPort, observerPort, { - maximumAttempts: 3, - delayMilliseconds: 25, - }); - - await expect(useCase.invoke(query)).rejects.toThrow( - 'Timeout waiting for previous runs to finish.', - ); - expect(queryPort.countActivePreviousRuns).toHaveBeenCalledTimes(3); - expect(delayPort.wait).toHaveBeenCalledTimes(2); - expect(observerPort.waitingForPreviousRuns).toHaveBeenCalledTimes(2); + await expect(new WaitForPreviousWorkflowRunsUseCase( + queryPort, + { wait: jest.fn() }, + observerPort, + policy({ maximumQueueWaitMilliseconds: 100 }), + { nowMilliseconds: jest.fn().mockReturnValueOnce(0).mockReturnValue(100) }, + { next: () => 0.5 }, + ).invoke(query)).rejects.toThrow('Timeout waiting for previous runs to finish.'); + expect(queryPort.countActivePreviousRuns).not.toHaveBeenCalled(); }); -}); +}); \ No newline at end of file diff --git a/src/application/usecases/workflow/wait_for_previous_workflow_runs_use_case.ts b/src/application/usecases/workflow/wait_for_previous_workflow_runs_use_case.ts index 6aca912ab..3008fe0db 100644 --- a/src/application/usecases/workflow/wait_for_previous_workflow_runs_use_case.ts +++ b/src/application/usecases/workflow/wait_for_previous_workflow_runs_use_case.ts @@ -3,18 +3,18 @@ import type { PreviousWorkflowRunsQueryPort, WorkflowPollingDelayPort, WorkflowPollingObserverPort, + WorkflowPollingRandomPort, + WorkflowQueueClockPort, } from '../../ports/workflow_run_ports'; +import { + calculateWorkflowPollingDelay, + WORKFLOW_QUEUE_POLICY, + type WorkflowPollingPolicy, +} from '../../policies/workflow_queue_policy'; import type { ParamUseCase } from '../base/param_usecase'; -export interface WorkflowPollingPolicy { - maximumAttempts: number; - delayMilliseconds: number; -} - -const DEFAULT_POLICY: WorkflowPollingPolicy = { - maximumAttempts: 2000, - delayMilliseconds: 2000, -}; +const SYSTEM_CLOCK: WorkflowQueueClockPort = { nowMilliseconds: () => Date.now() }; +const SYSTEM_RANDOM: WorkflowPollingRandomPort = { next: () => Math.random() }; export class WaitForPreviousWorkflowRunsUseCase implements ParamUseCase { taskId = 'WaitForPreviousWorkflowRunsUseCase'; @@ -23,22 +23,41 @@ export class WaitForPreviousWorkflowRunsUseCase implements ParamUseCase { - for (let attempt = 0; attempt < this.policy.maximumAttempts; attempt++) { - const activeRunCount = await this.queryPort.countActivePreviousRuns(query); + const deadlineAtMilliseconds = this.clock.nowMilliseconds() + this.policy.maximumQueueWaitMilliseconds; + let pollIndex = 0; + + while (true) { + if (this.clock.nowMilliseconds() >= deadlineAtMilliseconds) { + throw new Error('Timeout waiting for previous runs to finish.'); + } + const activeRunCount = await this.queryPort.countActivePreviousRuns(query, { + deadlineAtMilliseconds, + }); + if (this.clock.nowMilliseconds() >= deadlineAtMilliseconds) { + throw new Error('Timeout waiting for previous runs to finish.'); + } if (activeRunCount === 0) { this.observerPort.noActivePreviousRuns(); return; } - if (attempt === this.policy.maximumAttempts - 1) break; - this.observerPort.waitingForPreviousRuns(activeRunCount, this.policy.delayMilliseconds); - await this.delayPort.wait(this.policy.delayMilliseconds); + const delayMilliseconds = calculateWorkflowPollingDelay( + pollIndex, + this.random.next(), + this.policy, + ); + if (this.clock.nowMilliseconds() + delayMilliseconds >= deadlineAtMilliseconds) { + throw new Error('Timeout waiting for previous runs to finish.'); + } + this.observerPort.waitingForPreviousRuns(activeRunCount, delayMilliseconds); + await this.delayPort.wait(delayMilliseconds); + pollIndex += 1; } - - throw new Error('Timeout waiting for previous runs to finish.'); } } diff --git a/src/cli/commands/__tests__/do_policy.test.ts b/src/cli/commands/__tests__/do_policy.test.ts index df1aa49ef..ea75e943c 100644 --- a/src/cli/commands/__tests__/do_policy.test.ts +++ b/src/cli/commands/__tests__/do_policy.test.ts @@ -54,7 +54,11 @@ describe('do command policy', () => { }); it('collects only actionable authentication notices for each agent task', () => { - const tasks = buildDoAgentTasks({}); + const tasks = buildDoAgentTasks({ + agentCommand: 'codex exec --model gpt-5.6-luna --config model_provider=openai -', + findingsCommand: 'codex exec --model gpt-5.6-luna --config model_provider=openai -', + fixerCommand: 'codex exec --model gpt-5.6-luna --config model_provider=openai -', + }); const runPreflight = jest.fn() .mockReturnValueOnce({ check: { status: 'missing', message: 'findings credentials missing' }, mode: 'warn', shouldFail: false }) .mockReturnValueOnce({ check: { status: 'missing', message: 'fixer credentials missing' }, mode: 'required', shouldFail: true }); diff --git a/src/data/repository/workflow/__tests__/active_previous_workflow_runs_repository.test.ts b/src/data/repository/workflow/__tests__/active_previous_workflow_runs_repository.test.ts index 5cccb9eb5..f0f9bc0ee 100644 --- a/src/data/repository/workflow/__tests__/active_previous_workflow_runs_repository.test.ts +++ b/src/data/repository/workflow/__tests__/active_previous_workflow_runs_repository.test.ts @@ -1,7 +1,7 @@ -import { WORKFLOW_ACTIVE_STATUSES, WORKFLOW_STATUS } from '../../../../utils/constants'; +import { WORKFLOW_STATUS } from '../../../../utils/constants'; import type { - GithubWorkflowRunsClient, GithubWorkflowRun, + GithubWorkflowRunsClient, GithubWorkflowRunsResponse, } from '../../../../infrastructure/github/ports/github_workflow_provider_ports'; import { ActivePreviousWorkflowRunsRepository } from '../active_previous_workflow_runs_repository'; @@ -18,214 +18,122 @@ function workflowRun(run: Pick): Gi return run as GithubWorkflowRun; } +const query = { + owner: 'org', + repository: 'repo', + currentRunId: 200, + workflowName: 'Copilot - Issue', + workflowNames: ['Copilot - Issue', 'Task - Release'], +}; + describe('ActivePreviousWorkflowRunsRepository', () => { beforeEach(() => jest.clearAllMocks()); - it.each([ - { currentRunId: Number.NaN, workflowName: 'CI' }, - { currentRunId: 200, workflowName: '' }, - ])('skips provider pagination for an invalid query identity', async ({ currentRunId, workflowName }) => { + it('fails closed for an invalid query identity', async () => { const repository = new ActivePreviousWorkflowRunsRepository(client); - await expect(repository.countActivePreviousRuns({ - owner: 'org', - repository: 'repo', - currentRunId, - workflowName, - })).resolves.toBe(0); + await expect(repository.countActivePreviousRuns({ ...query, currentRunId: Number.NaN })).rejects.toThrow( + 'refusing to bypass sequential execution', + ); expect(iterator).not.toHaveBeenCalled(); }); - it('counts matching active previous runs across every provider page', async () => { - const pages: GithubWorkflowRunsResponse[] = [ - { - data: { - workflow_runs: [ - workflowRun({ id: 199, name: 'CI', status: WORKFLOW_STATUS.IN_PROGRESS }), - workflowRun({ id: 200, name: 'CI', status: WORKFLOW_STATUS.IN_PROGRESS }), - workflowRun({ id: 197, name: 'Other', status: WORKFLOW_STATUS.IN_PROGRESS }), - ], - }, - }, - { + it('uses one repository traversal and locally filters mixed statuses and workflow names', async () => { + iterator.mockImplementation(async function* () { + yield { data: { workflow_runs: [ - workflowRun({ id: 198, name: 'CI', status: WORKFLOW_STATUS.QUEUED }), - workflowRun({ id: 196, name: 'CI', status: 'completed' }), - workflowRun({ id: 195, name: null, status: null }), + workflowRun({ id: 199, name: 'Copilot - Issue', status: WORKFLOW_STATUS.IN_PROGRESS }), + workflowRun({ id: 198, name: 'Task - Release', status: WORKFLOW_STATUS.QUEUED }), + workflowRun({ id: 200, name: 'Copilot - Issue', status: WORKFLOW_STATUS.IN_PROGRESS }), + workflowRun({ id: 201, name: 'Copilot - Issue', status: WORKFLOW_STATUS.IN_PROGRESS }), + workflowRun({ id: 197, name: 'Unrelated workflow', status: WORKFLOW_STATUS.IN_PROGRESS }), + workflowRun({ id: 196, name: 'Copilot - Issue', status: WORKFLOW_STATUS.COMPLETED }), ], }, - }, - ]; - iterator.mockImplementation(async function* (_method: unknown, parameters: { status?: string }) { - if (parameters.status === WORKFLOW_STATUS.IN_PROGRESS) { - yield pages[0]; - } - if (parameters.status === WORKFLOW_STATUS.QUEUED) { - yield pages[1]; - } + } as GithubWorkflowRunsResponse; }); const repository = new ActivePreviousWorkflowRunsRepository(client); - const count = await repository.countActivePreviousRuns({ - owner: 'org', - repository: 'repo', - currentRunId: 200, - workflowName: 'CI', - }); - + await expect(repository.countActivePreviousRuns(query)).resolves.toBe(2); + expect(iterator).toHaveBeenCalledTimes(1); expect(iterator).toHaveBeenCalledWith(listWorkflowRunsForRepo, { owner: 'org', repo: 'repo', per_page: 100, - status: WORKFLOW_STATUS.IN_PROGRESS, }); - expect(iterator).toHaveBeenCalledTimes(WORKFLOW_ACTIVE_STATUSES.length); - expect(count).toBe(2); }); - it('counts runs when Octokit pagination yields array pages', async () => { - const pages: GithubWorkflowRunsResponse[] = [ - { - data: [ - workflowRun({ id: 199, name: 'CI', status: WORKFLOW_STATUS.IN_PROGRESS }), - workflowRun({ id: 198, name: 'CI', status: WORKFLOW_STATUS.QUEUED }), - ], - }, - ]; - iterator.mockImplementation(async function* (_method: unknown, parameters: { status?: string }) { - if (parameters.status === WORKFLOW_STATUS.IN_PROGRESS) { - yield* pages; - } - }); - const repository = new ActivePreviousWorkflowRunsRepository(client); - - const count = await repository.countActivePreviousRuns({ - owner: 'org', - repository: 'repo', - currentRunId: 200, - workflowName: 'CI', - workflowIdentifier: 'copilot_issue.yml', - }); - - expect(count).toBe(2); - expect(iterator).toHaveBeenCalledWith(listWorkflowRuns, { - owner: 'org', - repo: 'repo', - per_page: 100, - status: WORKFLOW_STATUS.IN_PROGRESS, - workflow_id: 'copilot_issue.yml', - }); - }); - - it('queries the repository endpoint and counts every Copilot workflow in the shared queue', async () => { + it('detects an older active matching run on a later page', async () => { iterator.mockImplementation(async function* () { + yield { data: { workflow_runs: [] } } as GithubWorkflowRunsResponse; yield { data: { workflow_runs: [ - workflowRun({ id: 199, name: 'Copilot - Issue Comment', status: WORKFLOW_STATUS.IN_PROGRESS }), - workflowRun({ id: 198, name: 'Task - Release', status: WORKFLOW_STATUS.QUEUED }), - workflowRun({ id: 197, name: 'Unrelated workflow', status: WORKFLOW_STATUS.IN_PROGRESS }), + workflowRun({ id: 150, name: 'Task - Release', status: WORKFLOW_STATUS.WAITING }), ], }, } as GithubWorkflowRunsResponse; }); const repository = new ActivePreviousWorkflowRunsRepository(client); - const workflowNames = ['Copilot - Issue', 'Copilot - Issue Comment', 'Task - Release']; - - const count = await repository.countActivePreviousRuns({ - owner: 'org', - repository: 'repo', - currentRunId: 200, - workflowName: 'Copilot - Issue', - workflowIdentifier: 'copilot_issue.yml', - workflowNames, - }); - expect(count).toBe(WORKFLOW_ACTIVE_STATUSES.length * 2); - expect(iterator).toHaveBeenCalledWith(listWorkflowRunsForRepo, expect.objectContaining({ - owner: 'org', - repo: 'repo', - per_page: 100, - status: WORKFLOW_STATUS.IN_PROGRESS, - })); - expect(iterator).not.toHaveBeenCalledWith(listWorkflowRuns, expect.anything()); + await expect(repository.countActivePreviousRuns(query)).resolves.toBe(1); + expect(iterator).toHaveBeenCalledTimes(1); }); - it('uses the shared workflow scope even when the current workflow name is unavailable', async () => { + it('supports both Octokit page shapes and the compatibility workflow endpoint', async () => { iterator.mockImplementation(async function* () { - yield { data: [] } as GithubWorkflowRunsResponse; + yield { + data: [workflowRun({ id: 199, name: 'Copilot - Issue', status: WORKFLOW_STATUS.PENDING })], + } as GithubWorkflowRunsResponse; }); const repository = new ActivePreviousWorkflowRunsRepository(client); await expect(repository.countActivePreviousRuns({ + ...query, + workflowNames: undefined, + workflowIdentifier: 'copilot_issue.yml', + })).resolves.toBe(1); + expect(iterator).toHaveBeenCalledWith(listWorkflowRuns, { owner: 'org', - repository: 'repo', - currentRunId: 200, - workflowName: '', - workflowNames: ['Copilot - Issue'], - })).resolves.toBe(0); - expect(iterator).toHaveBeenCalledWith(listWorkflowRunsForRepo, expect.anything()); - }); - - it('reports malformed workflow run pages clearly', async () => { - iterator.mockImplementation(async function* () { - yield { data: {} } as GithubWorkflowRunsResponse; + repo: 'repo', + per_page: 100, + workflow_id: 'copilot_issue.yml', }); - const repository = new ActivePreviousWorkflowRunsRepository(client); - - await expect(repository.countActivePreviousRuns({ - owner: 'org', - repository: 'repo', - currentRunId: 200, - workflowName: 'CI', - })).rejects.toThrow('GitHub workflow runs response did not contain a workflow_runs array.'); }); - it('reports an absent workflow response clearly', async () => { + it('rejects malformed provider pages instead of treating them as empty', async () => { iterator.mockImplementation(async function* () { - yield { data: undefined } as unknown as GithubWorkflowRunsResponse; + yield { data: {} } as GithubWorkflowRunsResponse; }); const repository = new ActivePreviousWorkflowRunsRepository(client); - await expect(repository.countActivePreviousRuns({ - owner: 'org', - repository: 'repo', - currentRunId: 200, - workflowName: 'CI', - })).rejects.toThrow('GitHub workflow runs response did not contain a workflow_runs array.'); + await expect(repository.countActivePreviousRuns(query)).rejects.toThrow( + 'did not contain a workflow_runs array', + ); }); - it('retries transient provider failures before returning active runs', async () => { + it('retries the complete paginated traversal after a transient later-page failure', async () => { const retryDelayPort = { wait: jest.fn().mockResolvedValue(undefined) }; - let attempts = 0; - iterator.mockImplementation(async function* (_method: unknown, parameters: { status?: string }) { - if (parameters.status !== WORKFLOW_STATUS.IN_PROGRESS) { - return; - } - - attempts += 1; - if (attempts === 1) { - throw { status: 500 }; - } - - yield { data: [] } as GithubWorkflowRunsResponse; + let traversals = 0; + iterator.mockImplementation(async function* () { + traversals += 1; + yield { data: { workflow_runs: [] } } as GithubWorkflowRunsResponse; + if (traversals === 1) throw { status: 500 }; + yield { + data: { workflow_runs: [workflowRun({ id: 150, name: 'Task - Release', status: WORKFLOW_STATUS.QUEUED })] }, + } as GithubWorkflowRunsResponse; }); const repository = new ActivePreviousWorkflowRunsRepository(client, retryDelayPort, { maximumAttempts: 3, initialDelayMilliseconds: 10, backoffMultiplier: 2, maximumDelayMilliseconds: 100, + jitterRatio: 0, }); - await expect(repository.countActivePreviousRuns({ - owner: 'org', - repository: 'repo', - currentRunId: 200, - workflowName: 'CI', - })).resolves.toBe(0); - - expect(attempts).toBe(2); + await expect(repository.countActivePreviousRuns(query)).resolves.toBe(1); + expect(traversals).toBe(2); expect(retryDelayPort.wait).toHaveBeenCalledWith(10); }); -}); +}); \ No newline at end of file diff --git a/src/data/repository/workflow/__tests__/workflow_runs_retry_policy.test.ts b/src/data/repository/workflow/__tests__/workflow_runs_retry_policy.test.ts index 4825f1c7a..c04ec5253 100644 --- a/src/data/repository/workflow/__tests__/workflow_runs_retry_policy.test.ts +++ b/src/data/repository/workflow/__tests__/workflow_runs_retry_policy.test.ts @@ -1,38 +1,67 @@ -import { withWorkflowRunsRetry } from '../workflow_runs_retry'; +import { withWorkflowRunsRetry, WORKFLOW_RUNS_RETRY_POLICY, WorkflowQueueDeadlineError } from '../workflow_runs_retry'; + +function dependencies(overrides: Record = {}) { + return { + delayPort: { wait: jest.fn().mockResolvedValue(undefined) }, + clock: { nowMilliseconds: jest.fn().mockReturnValue(0) }, + random: { next: jest.fn().mockReturnValue(0.5) }, + observer: { providerRetry: jest.fn() }, + policy: { ...WORKFLOW_RUNS_RETRY_POLICY, jitterRatio: 0 }, + deadlineAtMilliseconds: 10 * 60 * 1000, + ...overrides, + }; +} describe('workflow runs retry policy', () => { - it.each([ - [{ statusCode: 503 }], - [{ response: { status: 502 } }], - [new Error('Server Error')], - [{ code: 'ECONNREFUSED' }], - ])('retries transient error %p', async (error) => { - const operation = jest.fn() - .mockRejectedValueOnce(error) - .mockResolvedValue('ok'); - const delayPort = { wait: jest.fn().mockResolvedValue(undefined) }; - - await expect(withWorkflowRunsRetry(operation, delayPort, { - maximumAttempts: 2, - initialDelayMilliseconds: 5, - backoffMultiplier: 2, - maximumDelayMilliseconds: 20, - })).resolves.toBe('ok'); - expect(operation).toHaveBeenCalledTimes(2); - expect(delayPort.wait).toHaveBeenCalledWith(5); - }); - - it('does not retry a non-transient not-found error', async () => { - const operation = jest.fn().mockRejectedValue({ status: 404 }); - const delayPort = { wait: jest.fn().mockResolvedValue(undefined) }; - - await expect(withWorkflowRunsRetry(operation, delayPort, { - maximumAttempts: 3, - initialDelayMilliseconds: 5, - backoffMultiplier: 2, - maximumDelayMilliseconds: 20, - })).rejects.toMatchObject({ status: 404 }); - expect(operation).toHaveBeenCalledTimes(1); - expect(delayPort.wait).not.toHaveBeenCalled(); - }); -}); + it.each([ + [{ statusCode: 503 }], + [{ response: { status: 502 } }], + [new Error('Server Error')], + [{ code: 'ECONNREFUSED' }], + ])('retries transient error %p with bounded backoff', async error => { + const operation = jest.fn().mockRejectedValueOnce(error).mockResolvedValue('ok'); + const deps = dependencies(); + + await expect(withWorkflowRunsRetry(operation, deps)).resolves.toBe('ok'); + expect(operation).toHaveBeenCalledTimes(2); + expect(deps.delayPort.wait).toHaveBeenCalledWith(1000); + }); + + it('honors numeric Retry-After for 429 without jitter', async () => { + const operation = jest.fn().mockRejectedValueOnce({ status: 429, response: { headers: { 'retry-after': '7' } } }).mockResolvedValue('ok'); + const deps = dependencies(); + + await expect(withWorkflowRunsRetry(operation, deps)).resolves.toBe('ok'); + expect(deps.delayPort.wait).toHaveBeenCalledWith(7000); + expect(deps.observer.providerRetry).toHaveBeenCalledWith(expect.objectContaining({ reason: 'rate_limit' })); + }); + + it('honors x-ratelimit-reset for a rate-limited 403', async () => { + const operation = jest.fn().mockRejectedValueOnce({ + status: 403, + response: { headers: { 'x-ratelimit-remaining': '0', 'x-ratelimit-reset': '12' } }, + }).mockResolvedValue('ok'); + const deps = dependencies(); + + await expect(withWorkflowRunsRetry(operation, deps)).resolves.toBe('ok'); + expect(deps.delayPort.wait).toHaveBeenCalledWith(12000); + expect(deps.observer.providerRetry).toHaveBeenCalledWith(expect.objectContaining({ resetEpochSeconds: 12 })); + }); + + it('does not retry an unrelated 403 and never converts failures to zero', async () => { + const operation = jest.fn().mockRejectedValue({ status: 403, message: 'forbidden' }); + const deps = dependencies(); + + await expect(withWorkflowRunsRetry(operation, deps)).rejects.toMatchObject({ status: 403 }); + expect(operation).toHaveBeenCalledTimes(1); + expect(deps.delayPort.wait).not.toHaveBeenCalled(); + }); + + it('stops a retry whose delay would cross the absolute queue deadline', async () => { + const operation = jest.fn().mockRejectedValue({ status: 429, response: { headers: { 'retry-after': '60' } } }); + const deps = dependencies({ deadlineAtMilliseconds: 1000 }); + + await expect(withWorkflowRunsRetry(operation, deps)).rejects.toBeInstanceOf(WorkflowQueueDeadlineError); + expect(deps.delayPort.wait).not.toHaveBeenCalled(); + }); +}); \ No newline at end of file diff --git a/src/data/repository/workflow/active_previous_workflow_runs_repository.ts b/src/data/repository/workflow/active_previous_workflow_runs_repository.ts index a850d6ea0..a1f09342d 100644 --- a/src/data/repository/workflow/active_previous_workflow_runs_repository.ts +++ b/src/data/repository/workflow/active_previous_workflow_runs_repository.ts @@ -2,103 +2,92 @@ import type { PreviousWorkflowRunsQuery, PreviousWorkflowRunsQueryPort, WorkflowPollingDelayPort, + WorkflowPollingObserverPort, + WorkflowPollingRandomPort, + WorkflowQueueClockPort, + WorkflowQueueRequestContext, } from '../../../application/ports/workflow_run_ports'; import type { GithubWorkflowRunsClient, GithubWorkflowRun, GithubWorkflowRunsResponse, - GithubWorkflowRunsMethod, } from '../../../infrastructure/github/ports/github_workflow_provider_ports'; import { WORKFLOW_ACTIVE_STATUSES } from '../../../utils/constants'; -import { withWorkflowRunsRetry, type WorkflowRunsRetryPolicy } from './workflow_runs_retry'; +import { withWorkflowRunsRetry, WORKFLOW_RUNS_RETRY_POLICY, type WorkflowRunsRetryPolicy } from './workflow_runs_retry'; -const DEFAULT_RETRY_POLICY: WorkflowRunsRetryPolicy = { - maximumAttempts: 5, - initialDelayMilliseconds: 1000, - backoffMultiplier: 2, - maximumDelayMilliseconds: 8000, -}; - -const NO_OP_DELAY_PORT: WorkflowPollingDelayPort = { - async wait(): Promise { - return Promise.resolve(); - }, -}; +const NO_OP_DELAY_PORT: WorkflowPollingDelayPort = { wait: async () => undefined }; +const SYSTEM_CLOCK: WorkflowQueueClockPort = { nowMilliseconds: () => Date.now() }; +const SYSTEM_RANDOM: WorkflowPollingRandomPort = { next: () => Math.random() }; export class ActivePreviousWorkflowRunsRepository implements PreviousWorkflowRunsQueryPort { constructor( private readonly client: GithubWorkflowRunsClient, private readonly retryDelayPort: WorkflowPollingDelayPort = NO_OP_DELAY_PORT, - private readonly retryPolicy: WorkflowRunsRetryPolicy = DEFAULT_RETRY_POLICY, + private readonly retryPolicy: WorkflowRunsRetryPolicy = WORKFLOW_RUNS_RETRY_POLICY, + private readonly clock: WorkflowQueueClockPort = SYSTEM_CLOCK, + private readonly random: WorkflowPollingRandomPort = SYSTEM_RANDOM, + private readonly observer?: WorkflowPollingObserverPort, ) {} - async countActivePreviousRuns(query: PreviousWorkflowRunsQuery): Promise { - const hasWorkflowScope = query.workflowNames?.some((name) => name.trim().length > 0) || query.workflowName.trim().length > 0; - if (!Number.isFinite(query.currentRunId) || !hasWorkflowScope) { - return 0; - } - - let activeRunCount = 0; - for (const status of WORKFLOW_ACTIVE_STATUSES) { - activeRunCount += await this.countActiveRunsForStatus(query, status); - } - - return activeRunCount; - } - - private async countActiveRunsForStatus( + async countActivePreviousRuns( query: PreviousWorkflowRunsQuery, - status: string, + context: WorkflowQueueRequestContext = { deadlineAtMilliseconds: Number.POSITIVE_INFINITY }, ): Promise { - const useWorkflowEndpoint = Boolean( - query.workflowIdentifier - && (!query.workflowNames || query.workflowNames.length === 0) - && this.client.rest.actions.listWorkflowRuns, - ); - const method: GithubWorkflowRunsMethod = useWorkflowEndpoint - ? this.client.rest.actions.listWorkflowRuns! + if (!Number.isSafeInteger(query.currentRunId)) { + throw new Error('GitHub workflow identity is unavailable; refusing to bypass sequential execution.'); + } + const workflowNames = query.workflowNames?.filter(name => name.trim().length > 0) ?? []; + if (workflowNames.length === 0 && query.workflowName.trim().length === 0) { + throw new Error('GitHub workflow name is unavailable; refusing to bypass sequential execution.'); + } + const method = query.workflowIdentifier && workflowNames.length === 0 + ? this.client.rest.actions.listWorkflowRuns : this.client.rest.actions.listWorkflowRunsForRepo; + if (!method) throw new Error('GitHub workflow-scoped runs endpoint is unavailable.'); const parameters = { owner: query.owner, repo: query.repository, per_page: 100, - status, - ...(useWorkflowEndpoint ? { workflow_id: query.workflowIdentifier } : {}), + ...(method === this.client.rest.actions.listWorkflowRuns && query.workflowIdentifier + ? { workflow_id: query.workflowIdentifier } + : {}), }; + const names = workflowNames.length > 0 ? workflowNames : [query.workflowName]; return withWorkflowRunsRetry(async () => { let activeRunCount = 0; for await (const response of this.client.paginate.iterator(method, parameters)) { - activeRunCount += countMatchingRuns(this.extractWorkflowRuns(response), query); + activeRunCount += extractWorkflowRuns(response) + .filter(run => isActivePreviousRun(run, query, names)).length; } return activeRunCount; - }, this.retryDelayPort, this.retryPolicy); + }, { + delayPort: this.retryDelayPort, + clock: this.clock, + random: this.random, + observer: this.observer, + policy: this.retryPolicy, + deadlineAtMilliseconds: context.deadlineAtMilliseconds, + }); } - - private extractWorkflowRuns(response: GithubWorkflowRunsResponse): GithubWorkflowRun[] { - const data = response?.data; - if (Array.isArray(data)) { - return data; - } - - if (data !== null && typeof data === 'object' && Array.isArray(data.workflow_runs)) { - return data.workflow_runs; - } - - throw new Error('GitHub workflow runs response did not contain a workflow_runs array.'); - } - } -function countMatchingRuns(runs: ReadonlyArray, query: PreviousWorkflowRunsQuery): number { - return runs.filter((run) => isActivePreviousRun(run, query)).length; +function extractWorkflowRuns(response: GithubWorkflowRunsResponse): GithubWorkflowRun[] { + const data = response?.data; + if (Array.isArray(data)) return data; + if (data !== null && typeof data === 'object' && Array.isArray(data.workflow_runs)) { + return data.workflow_runs; + } + throw new Error('GitHub workflow runs response did not contain a workflow_runs array.'); } -function isActivePreviousRun(run: GithubWorkflowRun, query: PreviousWorkflowRunsQuery): boolean { - const workflowMatches = query.workflowNames && query.workflowNames.length > 0 - ? query.workflowNames.includes(run.name ?? '') - : run.name === query.workflowName; - return workflowMatches +function isActivePreviousRun( + run: GithubWorkflowRun, + query: PreviousWorkflowRunsQuery, + workflowNames: readonly string[], +): boolean { + return typeof run.name === 'string' + && workflowNames.includes(run.name) && run.id < query.currentRunId && WORKFLOW_ACTIVE_STATUSES.includes(run.status ?? 'unknown'); -} +} \ No newline at end of file diff --git a/src/data/repository/workflow/workflow_runs_retry.ts b/src/data/repository/workflow/workflow_runs_retry.ts index eda18db3b..1bd6f5d23 100644 --- a/src/data/repository/workflow/workflow_runs_retry.ts +++ b/src/data/repository/workflow/workflow_runs_retry.ts @@ -1,80 +1,235 @@ -import type { WorkflowPollingDelayPort } from '../../../application/ports/workflow_run_ports'; +import type { + WorkflowPollingDelayPort, + WorkflowPollingObserverPort, + WorkflowPollingRandomPort, + WorkflowQueueClockPort, +} from '../../../application/ports/workflow_run_ports'; +import { + calculateJitteredWorkflowDelay, + type WorkflowPollingPolicy, +} from '../../../application/policies/workflow_queue_policy'; export interface WorkflowRunsRetryPolicy { maximumAttempts: number; initialDelayMilliseconds: number; backoffMultiplier: number; maximumDelayMilliseconds: number; + jitterRatio?: number; + rateLimitInitialDelayMilliseconds?: number; + rateLimitMaximumDelayMilliseconds?: number; } -const TRANSIENT_NETWORK_ERRORS = new Set([ - 'ECONNRESET', - 'ETIMEDOUT', - 'EAI_AGAIN', - 'ENETUNREACH', - 'ECONNREFUSED', - 'UND_ERR_CONNECT_TIMEOUT', -]); +export const WORKFLOW_RUNS_RETRY_POLICY: WorkflowRunsRetryPolicy = { + maximumAttempts: 5, + initialDelayMilliseconds: 1000, + backoffMultiplier: 2, + maximumDelayMilliseconds: 30000, + jitterRatio: 0.2, + rateLimitInitialDelayMilliseconds: 60000, + rateLimitMaximumDelayMilliseconds: 300000, +}; + +export class WorkflowQueueDeadlineError extends Error { + constructor() { + super('Timeout waiting for previous runs to finish.'); + this.name = 'WorkflowQueueDeadlineError'; + } +} -export async function withWorkflowRunsRetry( +export interface WorkflowRunsRetryDependencies { + delayPort: WorkflowPollingDelayPort; + clock: WorkflowQueueClockPort; + random: WorkflowPollingRandomPort; + observer?: Pick; + policy: WorkflowRunsRetryPolicy; + deadlineAtMilliseconds: number; +} + +export function withWorkflowRunsRetry( + operation: () => Promise, + dependencies: WorkflowRunsRetryDependencies, +): Promise; +export function withWorkflowRunsRetry( operation: () => Promise, delayPort: WorkflowPollingDelayPort, policy: WorkflowRunsRetryPolicy, +): Promise; +export function withWorkflowRunsRetry( + operation: () => Promise, + dependenciesOrDelayPort: WorkflowRunsRetryDependencies | WorkflowPollingDelayPort, + legacyPolicy?: WorkflowRunsRetryPolicy, ): Promise { - return executeWithRetry(operation, delayPort, policy, 1, policy.initialDelayMilliseconds); + const dependencies: WorkflowRunsRetryDependencies = 'clock' in dependenciesOrDelayPort + ? dependenciesOrDelayPort + : { + delayPort: dependenciesOrDelayPort, + clock: { nowMilliseconds: () => Date.now() }, + random: { next: () => 0.5 }, + policy: { + ...WORKFLOW_RUNS_RETRY_POLICY, + ...legacyPolicy, + jitterRatio: 0, + }, + deadlineAtMilliseconds: Number.POSITIVE_INFINITY, + }; + return executeWithRetry(operation, dependencies, 1); } async function executeWithRetry( operation: () => Promise, - delayPort: WorkflowPollingDelayPort, - policy: WorkflowRunsRetryPolicy, + dependencies: WorkflowRunsRetryDependencies, attempt: number, - delayMilliseconds: number, ): Promise { + if (dependencies.clock.nowMilliseconds() >= dependencies.deadlineAtMilliseconds) { + throw new WorkflowQueueDeadlineError(); + } + try { return await operation(); } catch (error: unknown) { - if (!shouldRetry(error, attempt, policy.maximumAttempts)) throw error; - await delayPort.wait(delayMilliseconds); - return executeWithRetry( - operation, - delayPort, - policy, - attempt + 1, - nextRetryDelay(delayMilliseconds, policy), - ); + const classification = classifyWorkflowRunsError(error, dependencies.clock); + if (!classification.retryable + || (classification.reason === 'transient' && attempt >= dependencies.policy.maximumAttempts)) { + throw error; + } + + const delayMilliseconds = retryDelay(classification, attempt, dependencies); + if (dependencies.clock.nowMilliseconds() + delayMilliseconds >= dependencies.deadlineAtMilliseconds) { + throw new WorkflowQueueDeadlineError(); + } + dependencies.observer?.providerRetry?.({ + reason: classification.reason, + attempt, + delayMilliseconds, + ...(classification.resetEpochSeconds === undefined + ? {} + : { resetEpochSeconds: classification.resetEpochSeconds }), + }); + await dependencies.delayPort.wait(delayMilliseconds); + return executeWithRetry(operation, dependencies, attempt + 1); } } -function shouldRetry(error: unknown, attempt: number, maximumAttempts: number): boolean { - return attempt < maximumAttempts && isTransientWorkflowRunsError(error); +type RetryReason = 'rate_limit' | 'transient'; + +interface ErrorClassification { + retryable: boolean; + reason: RetryReason; + retryAfterMilliseconds?: number; + resetEpochSeconds?: number; } -function nextRetryDelay(currentDelay: number, policy: WorkflowRunsRetryPolicy): number { - return Math.min( - currentDelay * policy.backoffMultiplier, - policy.maximumDelayMilliseconds, +const TRANSIENT_NETWORK_ERRORS = new Set([ + 'ECONNRESET', + 'ETIMEDOUT', + 'EAI_AGAIN', + 'ENETUNREACH', + 'ECONNREFUSED', + 'UND_ERR_CONNECT_TIMEOUT', +]); + +function retryDelay( + classification: ErrorClassification, + attempt: number, + dependencies: WorkflowRunsRetryDependencies, +): number { + if (classification.retryAfterMilliseconds !== undefined) return classification.retryAfterMilliseconds; + const { policy } = dependencies; + const rateLimit = classification.reason === 'rate_limit'; + const baseDelay = Math.min( + (rateLimit ? (policy.rateLimitInitialDelayMilliseconds ?? 60000) : policy.initialDelayMilliseconds) + * policy.backoffMultiplier ** (attempt - 1), + rateLimit ? (policy.rateLimitMaximumDelayMilliseconds ?? 300000) : policy.maximumDelayMilliseconds, ); + const jitterPolicy: Pick = { + maximumDelayMilliseconds: rateLimit + ? (policy.rateLimitMaximumDelayMilliseconds ?? 300000) + : policy.maximumDelayMilliseconds, + jitterRatio: policy.jitterRatio ?? 0, + }; + return calculateJitteredWorkflowDelay(baseDelay, dependencies.random.next(), jitterPolicy); } -function isTransientWorkflowRunsError(error: unknown): boolean { - if (!error || typeof error !== 'object') return false; +function classifyWorkflowRunsError( + error: unknown, + clock: WorkflowQueueClockPort, +): ErrorClassification { + if (!error || typeof error !== 'object') return { retryable: false, reason: 'transient' }; const candidate = error as { status?: unknown; statusCode?: unknown; code?: unknown; - response?: { status?: unknown }; + headers?: unknown; + data?: { message?: unknown }; + response?: { + status?: unknown; + headers?: unknown; + data?: { message?: unknown }; + }; message?: unknown; }; const status = firstNumericValue(candidate.status, candidate.statusCode, candidate.response?.status); - if (status !== undefined) { - return status === 408 || status === 429 || status >= 500; + const headers = candidate.response?.headers ?? candidate.headers; + const message = [candidate.response?.data?.message, candidate.data?.message, candidate.message] + .find(value => typeof value === 'string') as string | undefined; + const remaining = header(headers, 'x-ratelimit-remaining'); + const isRateLimited = status === 429 + || (status === 403 && (remaining === '0' + || /(?:rate limit|secondary rate|abuse limit|too many requests)/i.test(message ?? ''))); + if (isRateLimited) { + const retryAfterMilliseconds = parseRetryAfter(header(headers, 'retry-after'), clock); + const resetEpochSeconds = parseEpochSeconds(header(headers, 'x-ratelimit-reset')); + const resetDelay = resetEpochSeconds === undefined + ? undefined + : Math.max(0, resetEpochSeconds * 1000 - clock.nowMilliseconds()); + return { + retryable: true, + reason: 'rate_limit', + retryAfterMilliseconds: retryAfterMilliseconds ?? resetDelay, + resetEpochSeconds, + }; + } + if (status === 408 || (status !== undefined && status >= 500)) { + return { retryable: true, reason: 'transient' }; } - if (typeof candidate.code === 'string' && TRANSIENT_NETWORK_ERRORS.has(candidate.code)) return true; - return typeof candidate.message === 'string' - && /\b(server error|service unavailable|bad gateway|gateway timeout|temporarily unavailable)\b/i.test(candidate.message); + if (typeof candidate.code === 'string' && TRANSIENT_NETWORK_ERRORS.has(candidate.code)) { + return { retryable: true, reason: 'transient' }; + } + return { + retryable: typeof message === 'string' + && /\b(server error|service unavailable|bad gateway|gateway timeout|temporarily unavailable)\b/i.test(message), + reason: 'transient', + }; +} + +function header(headers: unknown, name: string): string | undefined { + if (!headers) return undefined; + if (typeof (headers as { get?: unknown }).get === 'function') { + const value = (headers as { get(key: string): unknown }).get(name); + return value === undefined || value === null ? undefined : String(value); + } + if (typeof headers !== 'object') return undefined; + const entry = Object.entries(headers as Record) + .find(([key]) => key.toLowerCase() === name); + return entry?.[1] === undefined || entry?.[1] === null ? undefined : String(entry[1]); +} + +function parseRetryAfter(value: string | undefined, clock: WorkflowQueueClockPort): number | undefined { + if (!value) return undefined; + const seconds = Number(value); + if (Number.isFinite(seconds) && seconds >= 0) return Math.round(seconds * 1000); + const timestamp = Date.parse(value); + return Number.isFinite(timestamp) + ? Math.max(0, timestamp - clock.nowMilliseconds()) + : undefined; +} + +function parseEpochSeconds(value: string | undefined): number | undefined { + if (!value) return undefined; + const epoch = Number(value); + return Number.isFinite(epoch) && epoch >= 0 ? epoch : undefined; } function firstNumericValue(...values: unknown[]): number | undefined { return values.find((value): value is number => typeof value === 'number' && Number.isFinite(value)); -} +} \ No newline at end of file diff --git a/src/infrastructure/composition/__tests__/workflow_queue_composition_root.test.ts b/src/infrastructure/composition/__tests__/workflow_queue_composition_root.test.ts index 617f297a6..6294d7f7c 100644 --- a/src/infrastructure/composition/__tests__/workflow_queue_composition_root.test.ts +++ b/src/infrastructure/composition/__tests__/workflow_queue_composition_root.test.ts @@ -1,6 +1,6 @@ import * as github from '@actions/github'; import { createWaitForPreviousWorkflowRunsUseCase } from '../workflow_queue_composition_root'; -import { WORKFLOW_ACTIVE_STATUSES, WORKFLOW_STATUS } from '../../../utils/constants'; + jest.mock('@actions/github'); @@ -31,9 +31,8 @@ describe('workflow queue composition root', () => { owner: 'org', repo: 'repo', per_page: 100, - status: WORKFLOW_STATUS.IN_PROGRESS, }); - expect(iterator).toHaveBeenCalledTimes(WORKFLOW_ACTIVE_STATUSES.length); + expect(iterator).toHaveBeenCalledTimes(1); }); it('uses the workflow-scoped endpoint when the workflow identifier is available', async () => { @@ -60,7 +59,6 @@ describe('workflow queue composition root', () => { owner: 'org', repo: 'repo', per_page: 100, - status: WORKFLOW_STATUS.IN_PROGRESS, workflow_id: 'copilot_issue.yml', }); expect(iterator).not.toHaveBeenCalledWith(listWorkflowRunsForRepo, expect.anything()); diff --git a/src/infrastructure/composition/workflow_queue_composition_root.ts b/src/infrastructure/composition/workflow_queue_composition_root.ts index 4009652f5..f1f1ad9d5 100644 --- a/src/infrastructure/composition/workflow_queue_composition_root.ts +++ b/src/infrastructure/composition/workflow_queue_composition_root.ts @@ -2,15 +2,25 @@ import { WaitForPreviousWorkflowRunsUseCase } from '../../application/usecases/w import { ActivePreviousWorkflowRunsRepository } from '../../data/repository/workflow/active_previous_workflow_runs_repository'; import { TimerWorkflowPollingDelayAdapter } from '../time/timer_workflow_polling_delay_adapter'; import { LoggerWorkflowPollingObserverAdapter } from '../logging/logger_workflow_polling_observer_adapter'; +import { SystemWorkflowQueueClockAdapter } from '../time/system_workflow_queue_clock_adapter'; +import { SystemWorkflowPollingRandomAdapter } from '../time/system_workflow_polling_random_adapter'; import { createWorkflowRunsClient } from './github_workflow_client_factory'; export function createWaitForPreviousWorkflowRunsUseCase(token: string): WaitForPreviousWorkflowRunsUseCase { const client = createWorkflowRunsClient().getClient(token); const delayPort = new TimerWorkflowPollingDelayAdapter(); + const observerPort = new LoggerWorkflowPollingObserverAdapter(); return new WaitForPreviousWorkflowRunsUseCase( - new ActivePreviousWorkflowRunsRepository(client, delayPort), + new ActivePreviousWorkflowRunsRepository( + client, + delayPort, + undefined, + new SystemWorkflowQueueClockAdapter(), + new SystemWorkflowPollingRandomAdapter(), + observerPort, + ), delayPort, - new LoggerWorkflowPollingObserverAdapter(), + observerPort, ); } diff --git a/src/infrastructure/logging/logger_workflow_polling_observer_adapter.ts b/src/infrastructure/logging/logger_workflow_polling_observer_adapter.ts index c03de21aa..e07952972 100644 --- a/src/infrastructure/logging/logger_workflow_polling_observer_adapter.ts +++ b/src/infrastructure/logging/logger_workflow_polling_observer_adapter.ts @@ -11,4 +11,20 @@ export class LoggerWorkflowPollingObserverAdapter implements WorkflowPollingObse `⏳ Found ${activeRunCount} previous run(s) still active. Waiting ${delayMilliseconds / 1000}s...`, ); } + + providerRetry(observation: { + reason: 'rate_limit' | 'transient'; + attempt: number; + delayMilliseconds: number; + resetEpochSeconds?: number; + }): void { + logDebugInfo('GitHub workflow polling retry scheduled.', false, { + reason: observation.reason, + attempt: observation.attempt, + delayMilliseconds: observation.delayMilliseconds, + ...(observation.resetEpochSeconds === undefined + ? {} + : { resetEpochSeconds: observation.resetEpochSeconds }), + }); + } } diff --git a/src/infrastructure/time/system_workflow_polling_random_adapter.ts b/src/infrastructure/time/system_workflow_polling_random_adapter.ts new file mode 100644 index 000000000..9c7c9517f --- /dev/null +++ b/src/infrastructure/time/system_workflow_polling_random_adapter.ts @@ -0,0 +1,7 @@ +import type { WorkflowPollingRandomPort } from '../../application/ports/workflow_run_ports'; + +export class SystemWorkflowPollingRandomAdapter implements WorkflowPollingRandomPort { + next(): number { + return Math.random(); + } +} \ No newline at end of file diff --git a/src/infrastructure/time/system_workflow_queue_clock_adapter.ts b/src/infrastructure/time/system_workflow_queue_clock_adapter.ts new file mode 100644 index 000000000..155e9a810 --- /dev/null +++ b/src/infrastructure/time/system_workflow_queue_clock_adapter.ts @@ -0,0 +1,7 @@ +import type { WorkflowQueueClockPort } from '../../application/ports/workflow_run_ports'; + +export class SystemWorkflowQueueClockAdapter implements WorkflowQueueClockPort { + nowMilliseconds(): number { + return Date.now(); + } +} \ No newline at end of file diff --git a/src/tooling/__tests__/validate_workflow_contract.test.ts b/src/tooling/__tests__/validate_workflow_contract.test.ts new file mode 100644 index 000000000..fb980937c --- /dev/null +++ b/src/tooling/__tests__/validate_workflow_contract.test.ts @@ -0,0 +1,54 @@ +import path from 'node:path'; +import { COPILOT_WORKFLOW_NAMES, WORKFLOW_QUEUE_POLICY } from '../../application/policies/workflow_queue_policy'; + +interface ContractModule { + assertQueueWorkflow(file: string, workflow: Record): void; + assertRunner(file: string, workflow: Record): void; + MIN_QUEUE_JOB_TIMEOUT_MINUTES: number; + QUEUE_WORKFLOW_MANIFEST: readonly { workflowName: string }[]; +} + +const { + assertQueueWorkflow, + assertRunner, + MIN_QUEUE_JOB_TIMEOUT_MINUTES, + QUEUE_WORKFLOW_MANIFEST, +} = require('../../../scripts/validate-workflow-contract.cjs') as ContractModule; + +const queueFile = path.join(process.cwd(), '.github', 'workflows', 'copilot_issue.yml'); +const validWorkflow = { + name: 'Copilot - Issue', + jobs: { + 'copilot-issues': { + 'runs-on': ['self-hosted', 'codex'], + 'timeout-minutes': MIN_QUEUE_JOB_TIMEOUT_MINUTES, + steps: [{ uses: './', with: {} }], + }, + }, +}; + +describe('workflow contract validator', () => { + it('keeps the manifest, workflow names, and queue budget synchronized', () => { + expect(QUEUE_WORKFLOW_MANIFEST.map(entry => entry.workflowName)).toEqual(expect.arrayContaining(COPILOT_WORKFLOW_NAMES)); + expect(COPILOT_WORKFLOW_NAMES).toEqual(expect.arrayContaining(QUEUE_WORKFLOW_MANIFEST.map(entry => entry.workflowName))); + expect(WORKFLOW_QUEUE_POLICY.maximumQueueWaitMilliseconds).toBe(90 * 60 * 1000); + }); + + it.each([ + ['wrong workflow name', { ...validWorkflow, name: 'Wrong' }], + ['missing timeout', { ...validWorkflow, jobs: { 'copilot-issues': { ...validWorkflow.jobs['copilot-issues'], 'timeout-minutes': undefined } } }], + ['short timeout', { ...validWorkflow, jobs: { 'copilot-issues': { ...validWorkflow.jobs['copilot-issues'], 'timeout-minutes': 119 } } }], + ['workflow concurrency', { ...validWorkflow, concurrency: { group: 'x' } }], + ['job concurrency', { ...validWorkflow, jobs: { 'copilot-issues': { ...validWorkflow.jobs['copilot-issues'], concurrency: { group: 'x' } } } }], + ['missing queue job', { ...validWorkflow, jobs: {} }], + ['unmanifested action job', { ...validWorkflow, jobs: { ...validWorkflow.jobs, other: { steps: [{ uses: './' }] } } }], + ])('rejects %s', (_reason, workflow) => { + expect(() => assertQueueWorkflow(queueFile, workflow)).toThrow(); + }); + + it('rejects a queue job with the wrong runner', () => { + expect(() => assertRunner(queueFile, { + jobs: { 'copilot-issues': { 'runs-on': 'ubuntu-latest' } }, + })).toThrow('runs-on self-hosted, codex'); + }); +}); \ No newline at end of file From 1cc15f9a8d79757f6c546b3f24c97304fc116261 Mon Sep 17 00:00:00 2001 From: Efra Espada Date: Mon, 31 Aug 2026 17:57:15 +0000 Subject: [PATCH 2/6] bugfix-344-throttle-workflow-queue-polling-and-increase-workflow-timeouts --- build/cli/index.js | 21 +++++++-- build/cli/src/actions/main_run_lifecycle.d.ts | 8 ++++ build/github_action/index.js | 21 +++++++-- .../src/actions/main_run_lifecycle.d.ts | 8 ++++ scripts/validate-workflow-contract.cjs | 9 ++++ src/actions/__tests__/common_action.test.ts | 26 +++++++++-- src/actions/main_run_lifecycle.ts | 22 +++++++-- ...or_previous_workflow_runs_use_case.test.ts | 33 +++++++++++++ ..._previous_workflow_runs_repository.test.ts | 28 +++++++++++ .../workflow_runs_retry_policy.test.ts | 46 +++++++++++++++++++ .../validate_workflow_contract.test.ts | 30 +++++++++++- 11 files changed, 238 insertions(+), 14 deletions(-) diff --git a/build/cli/index.js b/build/cli/index.js index f495c7aba..e67ae2f92 100755 --- a/build/cli/index.js +++ b/build/cli/index.js @@ -53303,6 +53303,7 @@ var __importDefault = (this && this.__importDefault) || function (mod) { return (mod && mod.__esModule) ? mod : { "default": mod }; }; Object.defineProperty(exports, "__esModule", ({ value: true })); +exports.WorkflowQueueFailureError = exports.WORKFLOW_QUEUE_FAILURE_MESSAGE = void 0; exports.buildPreviousWorkflowRunsQuery = buildPreviousWorkflowRunsQuery; exports.waitForPreviousWorkflowRuns = waitForPreviousWorkflowRuns; exports.logWelcomeMessage = logWelcomeMessage; @@ -53318,6 +53319,18 @@ const main_run_dispatcher_1 = __nccwpck_require__(28586); const workflow_context_1 = __nccwpck_require__(55224); const workflow_queue_composition_root_1 = __nccwpck_require__(21598); const workflow_queue_policy_1 = __nccwpck_require__(43193); +exports.WORKFLOW_QUEUE_FAILURE_MESSAGE = 'Workflow queue check failed; sequential execution was not bypassed.'; +/** + * Keeps provider diagnostics out of the action's externally visible failure + * channel while preserving fail-closed queue behavior. + */ +class WorkflowQueueFailureError extends Error { + constructor() { + super(exports.WORKFLOW_QUEUE_FAILURE_MESSAGE); + this.name = 'WorkflowQueueFailureError'; + } +} +exports.WorkflowQueueFailureError = WorkflowQueueFailureError; function buildPreviousWorkflowRunsQuery(repository) { const query = { owner: repository.owner, @@ -53339,9 +53352,11 @@ async function waitForPreviousWorkflowRuns(execution, repository) { } await (0, workflow_queue_composition_root_1.createWaitForPreviousWorkflowRunsUseCase)(execution.tokens.token) .invoke(query) - .catch((error) => { - (0, logger_1.logError)(`Error waiting for previous runs: ${error}`); - throw error; + .catch(() => { + // Provider/Octokit errors can contain response bodies, URLs, + // headers, and credentials. Never interpolate or forward them. + (0, logger_1.logError)(exports.WORKFLOW_QUEUE_FAILURE_MESSAGE); + throw new WorkflowQueueFailureError(); }); } function logWelcomeMessage(execution) { diff --git a/build/cli/src/actions/main_run_lifecycle.d.ts b/build/cli/src/actions/main_run_lifecycle.d.ts index 271b656e3..07e065a6c 100644 --- a/build/cli/src/actions/main_run_lifecycle.d.ts +++ b/build/cli/src/actions/main_run_lifecycle.d.ts @@ -3,6 +3,14 @@ import type { Result } from '../data/model/result'; import type { ExecutableMainRunRoute, MainRunRouteHandlers } from './main_run_route_handlers'; import type { RepositoryCoordinates } from './repository_context'; import type { PreviousWorkflowRunsQuery } from '../application/ports/workflow_run_ports'; +export declare const WORKFLOW_QUEUE_FAILURE_MESSAGE = "Workflow queue check failed; sequential execution was not bypassed."; +/** + * Keeps provider diagnostics out of the action's externally visible failure + * channel while preserving fail-closed queue behavior. + */ +export declare class WorkflowQueueFailureError extends Error { + constructor(); +} export declare function buildPreviousWorkflowRunsQuery(repository: RepositoryCoordinates): PreviousWorkflowRunsQuery; export declare function waitForPreviousWorkflowRuns(execution: Execution, repository: RepositoryCoordinates): Promise; export declare function logWelcomeMessage(execution: Execution): void; diff --git a/build/github_action/index.js b/build/github_action/index.js index 268ec99ee..3d133731a 100644 --- a/build/github_action/index.js +++ b/build/github_action/index.js @@ -49102,6 +49102,7 @@ var __importDefault = (this && this.__importDefault) || function (mod) { return (mod && mod.__esModule) ? mod : { "default": mod }; }; Object.defineProperty(exports, "__esModule", ({ value: true })); +exports.WorkflowQueueFailureError = exports.WORKFLOW_QUEUE_FAILURE_MESSAGE = void 0; exports.buildPreviousWorkflowRunsQuery = buildPreviousWorkflowRunsQuery; exports.waitForPreviousWorkflowRuns = waitForPreviousWorkflowRuns; exports.logWelcomeMessage = logWelcomeMessage; @@ -49117,6 +49118,18 @@ const main_run_dispatcher_1 = __nccwpck_require__(28586); const workflow_context_1 = __nccwpck_require__(55224); const workflow_queue_composition_root_1 = __nccwpck_require__(21598); const workflow_queue_policy_1 = __nccwpck_require__(43193); +exports.WORKFLOW_QUEUE_FAILURE_MESSAGE = 'Workflow queue check failed; sequential execution was not bypassed.'; +/** + * Keeps provider diagnostics out of the action's externally visible failure + * channel while preserving fail-closed queue behavior. + */ +class WorkflowQueueFailureError extends Error { + constructor() { + super(exports.WORKFLOW_QUEUE_FAILURE_MESSAGE); + this.name = 'WorkflowQueueFailureError'; + } +} +exports.WorkflowQueueFailureError = WorkflowQueueFailureError; function buildPreviousWorkflowRunsQuery(repository) { const query = { owner: repository.owner, @@ -49138,9 +49151,11 @@ async function waitForPreviousWorkflowRuns(execution, repository) { } await (0, workflow_queue_composition_root_1.createWaitForPreviousWorkflowRunsUseCase)(execution.tokens.token) .invoke(query) - .catch((error) => { - (0, logger_1.logError)(`Error waiting for previous runs: ${error}`); - throw error; + .catch(() => { + // Provider/Octokit errors can contain response bodies, URLs, + // headers, and credentials. Never interpolate or forward them. + (0, logger_1.logError)(exports.WORKFLOW_QUEUE_FAILURE_MESSAGE); + throw new WorkflowQueueFailureError(); }); } function logWelcomeMessage(execution) { diff --git a/build/github_action/src/actions/main_run_lifecycle.d.ts b/build/github_action/src/actions/main_run_lifecycle.d.ts index 271b656e3..07e065a6c 100644 --- a/build/github_action/src/actions/main_run_lifecycle.d.ts +++ b/build/github_action/src/actions/main_run_lifecycle.d.ts @@ -3,6 +3,14 @@ import type { Result } from '../data/model/result'; import type { ExecutableMainRunRoute, MainRunRouteHandlers } from './main_run_route_handlers'; import type { RepositoryCoordinates } from './repository_context'; import type { PreviousWorkflowRunsQuery } from '../application/ports/workflow_run_ports'; +export declare const WORKFLOW_QUEUE_FAILURE_MESSAGE = "Workflow queue check failed; sequential execution was not bypassed."; +/** + * Keeps provider diagnostics out of the action's externally visible failure + * channel while preserving fail-closed queue behavior. + */ +export declare class WorkflowQueueFailureError extends Error { + constructor(); +} export declare function buildPreviousWorkflowRunsQuery(repository: RepositoryCoordinates): PreviousWorkflowRunsQuery; export declare function waitForPreviousWorkflowRuns(execution: Execution, repository: RepositoryCoordinates): Promise; export declare function logWelcomeMessage(execution: Execution): void; diff --git a/scripts/validate-workflow-contract.cjs b/scripts/validate-workflow-contract.cjs index c1a52cf6e..9d5e5df97 100644 --- a/scripts/validate-workflow-contract.cjs +++ b/scripts/validate-workflow-contract.cjs @@ -11,6 +11,14 @@ const workflowDirectories = [ ]; const QUEUE_WAIT_MINUTES = 90; const MIN_QUEUE_JOB_TIMEOUT_MINUTES = 120; +function assertQueueBudget(queueWaitMinutes, minimumJobTimeoutMinutes) { + if (!Number.isFinite(queueWaitMinutes) + || !Number.isFinite(minimumJobTimeoutMinutes) + || minimumJobTimeoutMinutes < queueWaitMinutes) { + throw new Error(`minimum job timeout must be >= queue wait (${queueWaitMinutes}m).`); + } +} +assertQueueBudget(QUEUE_WAIT_MINUTES, MIN_QUEUE_JOB_TIMEOUT_MINUTES); const QUEUE_WORKFLOW_MANIFEST = Object.freeze([ ['copilot_commit.yml', 'Copilot - Commit', 'copilot-commits'], ['copilot_issue.yml', 'Copilot - Issue', 'copilot-issues'], @@ -140,6 +148,7 @@ module.exports = { MIN_QUEUE_JOB_TIMEOUT_MINUTES, QUEUE_WORKFLOW_MANIFEST, assertAgentInputs, + assertQueueBudget, assertQueueWorkflow, assertRunner, assertSequentialMutationWorkflow, diff --git a/src/actions/__tests__/common_action.test.ts b/src/actions/__tests__/common_action.test.ts index 850085cf8..07b87d0c9 100644 --- a/src/actions/__tests__/common_action.test.ts +++ b/src/actions/__tests__/common_action.test.ts @@ -425,12 +425,30 @@ describe('mainRun', () => { expect(results).toEqual([]); }); - it('propagates workflow queue errors when polling rejects and welcome is false', async () => { - mockWaitForPreviousWorkflowRunsInvoke.mockRejectedValue(new Error('Queue error')); + it('propagates a canonical queue failure without exposing provider diagnostics', async () => { + const markers = [ + 'provider-message-marker', + 'response-body-marker', + 'https://api.example.test/repos/org/repo?token=url-marker', + 'authorization-header-marker', + 'credential-marker', + ]; + const providerError = Object.assign(new Error(markers.join(' ')), { + response: { + data: { message: markers[1] }, + headers: { authorization: markers[3], location: markers[2] }, + }, + request: { headers: { authorization: markers[3] } }, + }); + mockWaitForPreviousWorkflowRunsInvoke.mockRejectedValue(providerError); const execution = mockExecution({ welcome: undefined }); - await expect(runMain(execution)).rejects.toThrow('Queue error'); + await expect(runMain(execution)).rejects.toThrow( + 'Workflow queue check failed; sequential execution was not bypassed.', + ); - expect(logger.logError).toHaveBeenCalledWith('Error waiting for previous runs: Error: Queue error'); + const logCalls = logger.logError.mock.calls.flat().map(String).join('\n'); + expect(logCalls).toContain('Workflow queue check failed; sequential execution was not bypassed.'); + for (const marker of markers) expect(logCalls).not.toContain(marker); }); }); diff --git a/src/actions/main_run_lifecycle.ts b/src/actions/main_run_lifecycle.ts index dc99369b7..031abfe49 100644 --- a/src/actions/main_run_lifecycle.ts +++ b/src/actions/main_run_lifecycle.ts @@ -13,6 +13,20 @@ import { createWaitForPreviousWorkflowRunsUseCase } from '../infrastructure/comp import { COPILOT_WORKFLOW_NAMES } from '../application/policies/workflow_queue_policy'; import type { PreviousWorkflowRunsQuery } from '../application/ports/workflow_run_ports'; +export const WORKFLOW_QUEUE_FAILURE_MESSAGE = + 'Workflow queue check failed; sequential execution was not bypassed.'; + +/** + * Keeps provider diagnostics out of the action's externally visible failure + * channel while preserving fail-closed queue behavior. + */ +export class WorkflowQueueFailureError extends Error { + constructor() { + super(WORKFLOW_QUEUE_FAILURE_MESSAGE); + this.name = 'WorkflowQueueFailureError'; + } +} + export function buildPreviousWorkflowRunsQuery( repository: RepositoryCoordinates, ): PreviousWorkflowRunsQuery { @@ -41,9 +55,11 @@ export async function waitForPreviousWorkflowRuns( } await createWaitForPreviousWorkflowRunsUseCase(execution.tokens.token) .invoke(query) - .catch((error: unknown) => { - logError(`Error waiting for previous runs: ${error}`); - throw error; + .catch(() => { + // Provider/Octokit errors can contain response bodies, URLs, + // headers, and credentials. Never interpolate or forward them. + logError(WORKFLOW_QUEUE_FAILURE_MESSAGE); + throw new WorkflowQueueFailureError(); }); } diff --git a/src/application/usecases/workflow/__tests__/wait_for_previous_workflow_runs_use_case.test.ts b/src/application/usecases/workflow/__tests__/wait_for_previous_workflow_runs_use_case.test.ts index 32f4b9617..e74d6da3d 100644 --- a/src/application/usecases/workflow/__tests__/wait_for_previous_workflow_runs_use_case.test.ts +++ b/src/application/usecases/workflow/__tests__/wait_for_previous_workflow_runs_use_case.test.ts @@ -74,6 +74,39 @@ describe('WaitForPreviousWorkflowRunsUseCase', () => { expect(observerPort.waitingForPreviousRuns).toHaveBeenNthCalledWith(2, 1, 10000); }); + it('continues polling through the configured delay cap before observing an empty queue', async () => { + const queryPort: PreviousWorkflowRunsQueryPort = { + countActivePreviousRuns: jest.fn() + .mockResolvedValueOnce(1) + .mockResolvedValueOnce(1) + .mockResolvedValueOnce(1) + .mockResolvedValueOnce(1) + .mockResolvedValueOnce(1) + .mockResolvedValueOnce(1) + .mockResolvedValueOnce(1) + .mockResolvedValueOnce(0), + }; + const delays: number[] = []; + const delayPort: WorkflowPollingDelayPort = { + wait: jest.fn(async milliseconds => { delays.push(milliseconds); }), + }; + const observerPort = observer(); + const useCase = new WaitForPreviousWorkflowRunsUseCase( + queryPort, + delayPort, + observerPort, + policy({ maximumQueueWaitMilliseconds: 10 * 60 * 1000 }), + { nowMilliseconds: () => 0 }, + { next: () => 0.5 }, + ); + + await useCase.invoke(query); + + expect(delays).toEqual([5000, 10000, 20000, 40000, 60000, 60000, 60000]); + expect(queryPort.countActivePreviousRuns).toHaveBeenCalledTimes(8); + expect(observerPort.noActivePreviousRuns).toHaveBeenCalledTimes(1); + }); + it('applies injected jitter and fails closed at the queue deadline', async () => { const queryPort: PreviousWorkflowRunsQueryPort = { countActivePreviousRuns: jest.fn().mockResolvedValue(1), diff --git a/src/data/repository/workflow/__tests__/active_previous_workflow_runs_repository.test.ts b/src/data/repository/workflow/__tests__/active_previous_workflow_runs_repository.test.ts index f0f9bc0ee..9d37c77e6 100644 --- a/src/data/repository/workflow/__tests__/active_previous_workflow_runs_repository.test.ts +++ b/src/data/repository/workflow/__tests__/active_previous_workflow_runs_repository.test.ts @@ -81,6 +81,34 @@ describe('ActivePreviousWorkflowRunsRepository', () => { expect(iterator).toHaveBeenCalledTimes(1); }); + it('counts multiple earlier queue runs while excluding cancelled and skipped terminals', async () => { + iterator.mockImplementation(async function* () { + yield { + data: { + workflow_runs: [ + workflowRun({ id: 199, name: 'Copilot - Issue', status: WORKFLOW_STATUS.IN_PROGRESS }), + workflowRun({ id: 198, name: 'Task - Release', status: WORKFLOW_STATUS.QUEUED }), + workflowRun({ id: 197, name: 'Copilot - Commit', status: WORKFLOW_STATUS.CANCELLED }), + workflowRun({ id: 196, name: 'Copilot - Pull Request', status: WORKFLOW_STATUS.SKIPPED }), + workflowRun({ id: 200, name: 'Copilot - Issue Comment', status: WORKFLOW_STATUS.IN_PROGRESS }), + ], + }, + } as GithubWorkflowRunsResponse; + }); + const repository = new ActivePreviousWorkflowRunsRepository(client); + + await expect(repository.countActivePreviousRuns({ + ...query, + workflowNames: [ + 'Copilot - Issue', + 'Copilot - Issue Comment', + 'Task - Release', + 'Copilot - Commit', + 'Copilot - Pull Request', + ], + })).resolves.toBe(2); + }); + it('supports both Octokit page shapes and the compatibility workflow endpoint', async () => { iterator.mockImplementation(async function* () { yield { diff --git a/src/data/repository/workflow/__tests__/workflow_runs_retry_policy.test.ts b/src/data/repository/workflow/__tests__/workflow_runs_retry_policy.test.ts index c04ec5253..ed45c65aa 100644 --- a/src/data/repository/workflow/__tests__/workflow_runs_retry_policy.test.ts +++ b/src/data/repository/workflow/__tests__/workflow_runs_retry_policy.test.ts @@ -48,6 +48,52 @@ describe('workflow runs retry policy', () => { expect(deps.observer.providerRetry).toHaveBeenCalledWith(expect.objectContaining({ resetEpochSeconds: 12 })); }); + it('classifies a message-based rate-limited 403 and retries it', async () => { + const operation = jest.fn() + .mockRejectedValueOnce({ status: 403, response: { data: { message: 'You have exceeded the secondary rate limit.' } } }) + .mockResolvedValue('ok'); + const deps = dependencies({ + policy: { ...WORKFLOW_RUNS_RETRY_POLICY, jitterRatio: 0 }, + }); + + await expect(withWorkflowRunsRetry(operation, deps)).resolves.toBe('ok'); + expect(operation).toHaveBeenCalledTimes(2); + expect(deps.delayPort.wait).toHaveBeenCalledWith(60000); + expect(deps.observer.providerRetry).toHaveBeenCalledWith(expect.objectContaining({ reason: 'rate_limit' })); + }); + + it('uses bounded rate-limit fallback backoff with jitter and a cap', async () => { + const operation = jest.fn() + .mockRejectedValueOnce({ status: 429 }) + .mockRejectedValueOnce({ status: 429 }) + .mockRejectedValueOnce({ status: 429 }) + .mockResolvedValue('ok'); + const deps = dependencies({ + random: { next: jest.fn().mockReturnValueOnce(0).mockReturnValueOnce(1).mockReturnValueOnce(1) }, + policy: { + ...WORKFLOW_RUNS_RETRY_POLICY, + rateLimitInitialDelayMilliseconds: 100, + rateLimitMaximumDelayMilliseconds: 250, + jitterRatio: 0.5, + }, + }); + + await expect(withWorkflowRunsRetry(operation, deps)).resolves.toBe('ok'); + expect(deps.delayPort.wait.mock.calls.map(([delay]: [number]) => delay)).toEqual([50, 250, 250]); + }); + + it('exhausts transient retries without converting the provider failure to success', async () => { + const providerError = { statusCode: 503, message: 'transient provider failure' }; + const operation = jest.fn().mockRejectedValue(providerError); + const deps = dependencies({ + policy: { ...WORKFLOW_RUNS_RETRY_POLICY, maximumAttempts: 3, jitterRatio: 0 }, + }); + + await expect(withWorkflowRunsRetry(operation, deps)).rejects.toBe(providerError); + expect(operation).toHaveBeenCalledTimes(3); + expect(deps.delayPort.wait.mock.calls.map(([delay]: [number]) => delay)).toEqual([1000, 2000]); + }); + it('does not retry an unrelated 403 and never converts failures to zero', async () => { const operation = jest.fn().mockRejectedValue({ status: 403, message: 'forbidden' }); const deps = dependencies(); diff --git a/src/tooling/__tests__/validate_workflow_contract.test.ts b/src/tooling/__tests__/validate_workflow_contract.test.ts index fb980937c..b6be625f4 100644 --- a/src/tooling/__tests__/validate_workflow_contract.test.ts +++ b/src/tooling/__tests__/validate_workflow_contract.test.ts @@ -1,18 +1,26 @@ import path from 'node:path'; +import { readFileSync } from 'node:fs'; +import * as yaml from 'js-yaml'; import { COPILOT_WORKFLOW_NAMES, WORKFLOW_QUEUE_POLICY } from '../../application/policies/workflow_queue_policy'; interface ContractModule { assertQueueWorkflow(file: string, workflow: Record): void; assertRunner(file: string, workflow: Record): void; MIN_QUEUE_JOB_TIMEOUT_MINUTES: number; - QUEUE_WORKFLOW_MANIFEST: readonly { workflowName: string }[]; + QUEUE_WAIT_MINUTES: number; + QUEUE_WORKFLOW_MANIFEST: readonly { file: string; workflowName: string; jobId: string }[]; + assertQueueBudget(queueWaitMinutes: number, minimumJobTimeoutMinutes: number): void; + validateWorkflow(file: string, workflow: Record): void; } const { assertQueueWorkflow, assertRunner, MIN_QUEUE_JOB_TIMEOUT_MINUTES, + QUEUE_WAIT_MINUTES, QUEUE_WORKFLOW_MANIFEST, + assertQueueBudget, + validateWorkflow, } = require('../../../scripts/validate-workflow-contract.cjs') as ContractModule; const queueFile = path.join(process.cwd(), '.github', 'workflows', 'copilot_issue.yml'); @@ -32,6 +40,26 @@ describe('workflow contract validator', () => { expect(QUEUE_WORKFLOW_MANIFEST.map(entry => entry.workflowName)).toEqual(expect.arrayContaining(COPILOT_WORKFLOW_NAMES)); expect(COPILOT_WORKFLOW_NAMES).toEqual(expect.arrayContaining(QUEUE_WORKFLOW_MANIFEST.map(entry => entry.workflowName))); expect(WORKFLOW_QUEUE_POLICY.maximumQueueWaitMilliseconds).toBe(90 * 60 * 1000); + expect(MIN_QUEUE_JOB_TIMEOUT_MINUTES).toBeGreaterThanOrEqual(QUEUE_WAIT_MINUTES); + }); + + it('enforces the minimum job timeout to be at least the queue wait budget', () => { + expect(() => assertQueueBudget(90, 89)).toThrow('must be >= queue wait'); + expect(() => assertQueueBudget(90, 90)).not.toThrow(); + }); + + it('keeps every queue workflow sequential across repeated runs without cancellation or overwrite', () => { + for (const directory of ['.github/workflows', 'setup/workflows']) { + for (const manifest of QUEUE_WORKFLOW_MANIFEST) { + const file = path.join(process.cwd(), directory, manifest.file); + const workflow = yaml.load(readFileSync(file, 'utf8')) as Record & { + jobs: Record; + }; + expect(workflow.concurrency).toBeUndefined(); + expect(workflow.jobs[manifest.jobId].concurrency).toBeUndefined(); + expect(() => validateWorkflow(file, workflow)).not.toThrow(); + } + } }); it.each([ From 3b4a61d556999e9e5461846464edc732dc6c4fd2 Mon Sep 17 00:00:00 2001 From: Efra Espada Date: Mon, 31 Aug 2026 21:08:15 +0000 Subject: [PATCH 3/6] bugfix-344-throttle-workflow-queue-polling-and-increase-workflow-timeouts: bound rate-limit retries --- build/cli/index.js | 40 +++++++++++----- .../workflow/workflow_runs_retry.d.ts | 1 + build/github_action/index.js | 40 +++++++++++----- .../workflow/workflow_runs_retry.d.ts | 1 + ..._previous_workflow_runs_repository.test.ts | 22 +++++++++ .../workflow_runs_retry_policy.test.ts | 46 +++++++++++++++++++ ...ctive_previous_workflow_runs_repository.ts | 6 +++ .../workflow/workflow_runs_retry.ts | 40 +++++++++++----- 8 files changed, 161 insertions(+), 35 deletions(-) diff --git a/build/cli/index.js b/build/cli/index.js index e67ae2f92..d03bade54 100755 --- a/build/cli/index.js +++ b/build/cli/index.js @@ -69590,6 +69590,12 @@ class ActivePreviousWorkflowRunsRepository { const names = workflowNames.length > 0 ? workflowNames : [query.workflowName]; return (0, workflow_runs_retry_1.withWorkflowRunsRetry)(async () => { let activeRunCount = 0; + // Keep one complete sequential traversal: GitHub cannot safely express + // the seven shared workflow names, five active statuses, or the strict + // lower-ID predicate in this endpoint. Do not add provider filters or + // early-stop on page order; a matching run may occur on a later page. + // The residual cost is deep-history pagination, with retries restarting + // from page one, in exchange for an exact fail-closed count. for await (const response of this.client.paginate.iterator(method, parameters)) { activeRunCount += extractWorkflowRuns(response) .filter(run => isActivePreviousRun(run, query, names)).length; @@ -69663,6 +69669,7 @@ exports.withWorkflowRunsRetry = withWorkflowRunsRetry; const workflow_queue_policy_1 = __nccwpck_require__(43193); exports.WORKFLOW_RUNS_RETRY_POLICY = { maximumAttempts: 5, + rateLimitMaximumAttempts: 5, initialDelayMilliseconds: 1000, backoffMultiplier: 2, maximumDelayMilliseconds: 30000, @@ -69691,9 +69698,9 @@ function withWorkflowRunsRetry(operation, dependenciesOrDelayPort, legacyPolicy) }, deadlineAtMilliseconds: Number.POSITIVE_INFINITY, }; - return executeWithRetry(operation, dependencies, 1); + return executeWithRetry(operation, dependencies, 0, 0); } -async function executeWithRetry(operation, dependencies, attempt) { +async function executeWithRetry(operation, dependencies, transientFailures, rateLimitFailures) { if (dependencies.clock.nowMilliseconds() >= dependencies.deadlineAtMilliseconds) { throw new WorkflowQueueDeadlineError(); } @@ -69702,24 +69709,29 @@ async function executeWithRetry(operation, dependencies, attempt) { } catch (error) { const classification = classifyWorkflowRunsError(error, dependencies.clock); - if (!classification.retryable - || (classification.reason === 'transient' && attempt >= dependencies.policy.maximumAttempts)) { + const failureCount = classification.reason === 'rate_limit' + ? rateLimitFailures + 1 + : transientFailures + 1; + const maximumAttempts = classification.reason === 'rate_limit' + ? dependencies.policy.rateLimitMaximumAttempts + : dependencies.policy.maximumAttempts; + if (!classification.retryable || failureCount >= maximumAttempts) { throw error; } - const delayMilliseconds = retryDelay(classification, attempt, dependencies); + const delayMilliseconds = retryDelay(classification, failureCount, dependencies); if (dependencies.clock.nowMilliseconds() + delayMilliseconds >= dependencies.deadlineAtMilliseconds) { throw new WorkflowQueueDeadlineError(); } dependencies.observer?.providerRetry?.({ reason: classification.reason, - attempt, + attempt: failureCount, delayMilliseconds, ...(classification.resetEpochSeconds === undefined ? {} : { resetEpochSeconds: classification.resetEpochSeconds }), }); await dependencies.delayPort.wait(delayMilliseconds); - return executeWithRetry(operation, dependencies, attempt + 1); + return executeWithRetry(operation, dependencies, classification.reason === 'transient' ? failureCount : transientFailures, classification.reason === 'rate_limit' ? failureCount : rateLimitFailures); } } const TRANSIENT_NETWORK_ERRORS = new Set([ @@ -69762,7 +69774,9 @@ function classifyWorkflowRunsError(error, clock) { const resetEpochSeconds = parseEpochSeconds(header(headers, 'x-ratelimit-reset')); const resetDelay = resetEpochSeconds === undefined ? undefined - : Math.max(0, resetEpochSeconds * 1000 - clock.nowMilliseconds()); + : resetEpochSeconds * 1000 > clock.nowMilliseconds() + ? resetEpochSeconds * 1000 - clock.nowMilliseconds() + : undefined; return { retryable: true, reason: 'rate_limit', @@ -69799,11 +69813,13 @@ function parseRetryAfter(value, clock) { if (!value) return undefined; const seconds = Number(value); - if (Number.isFinite(seconds) && seconds >= 0) - return Math.round(seconds * 1000); + if (Number.isFinite(seconds)) { + const milliseconds = Math.round(seconds * 1000); + return milliseconds > 0 ? milliseconds : undefined; + } const timestamp = Date.parse(value); - return Number.isFinite(timestamp) - ? Math.max(0, timestamp - clock.nowMilliseconds()) + return Number.isFinite(timestamp) && timestamp > clock.nowMilliseconds() + ? timestamp - clock.nowMilliseconds() : undefined; } function parseEpochSeconds(value) { diff --git a/build/cli/src/data/repository/workflow/workflow_runs_retry.d.ts b/build/cli/src/data/repository/workflow/workflow_runs_retry.d.ts index 269efca75..9e089f492 100644 --- a/build/cli/src/data/repository/workflow/workflow_runs_retry.d.ts +++ b/build/cli/src/data/repository/workflow/workflow_runs_retry.d.ts @@ -1,6 +1,7 @@ import type { WorkflowPollingDelayPort, WorkflowPollingObserverPort, WorkflowPollingRandomPort, WorkflowQueueClockPort } from '../../../application/ports/workflow_run_ports'; export interface WorkflowRunsRetryPolicy { maximumAttempts: number; + rateLimitMaximumAttempts: number; initialDelayMilliseconds: number; backoffMultiplier: number; maximumDelayMilliseconds: number; diff --git a/build/github_action/index.js b/build/github_action/index.js index 3d133731a..b97bbdc24 100644 --- a/build/github_action/index.js +++ b/build/github_action/index.js @@ -65362,6 +65362,12 @@ class ActivePreviousWorkflowRunsRepository { const names = workflowNames.length > 0 ? workflowNames : [query.workflowName]; return (0, workflow_runs_retry_1.withWorkflowRunsRetry)(async () => { let activeRunCount = 0; + // Keep one complete sequential traversal: GitHub cannot safely express + // the seven shared workflow names, five active statuses, or the strict + // lower-ID predicate in this endpoint. Do not add provider filters or + // early-stop on page order; a matching run may occur on a later page. + // The residual cost is deep-history pagination, with retries restarting + // from page one, in exchange for an exact fail-closed count. for await (const response of this.client.paginate.iterator(method, parameters)) { activeRunCount += extractWorkflowRuns(response) .filter(run => isActivePreviousRun(run, query, names)).length; @@ -65435,6 +65441,7 @@ exports.withWorkflowRunsRetry = withWorkflowRunsRetry; const workflow_queue_policy_1 = __nccwpck_require__(43193); exports.WORKFLOW_RUNS_RETRY_POLICY = { maximumAttempts: 5, + rateLimitMaximumAttempts: 5, initialDelayMilliseconds: 1000, backoffMultiplier: 2, maximumDelayMilliseconds: 30000, @@ -65463,9 +65470,9 @@ function withWorkflowRunsRetry(operation, dependenciesOrDelayPort, legacyPolicy) }, deadlineAtMilliseconds: Number.POSITIVE_INFINITY, }; - return executeWithRetry(operation, dependencies, 1); + return executeWithRetry(operation, dependencies, 0, 0); } -async function executeWithRetry(operation, dependencies, attempt) { +async function executeWithRetry(operation, dependencies, transientFailures, rateLimitFailures) { if (dependencies.clock.nowMilliseconds() >= dependencies.deadlineAtMilliseconds) { throw new WorkflowQueueDeadlineError(); } @@ -65474,24 +65481,29 @@ async function executeWithRetry(operation, dependencies, attempt) { } catch (error) { const classification = classifyWorkflowRunsError(error, dependencies.clock); - if (!classification.retryable - || (classification.reason === 'transient' && attempt >= dependencies.policy.maximumAttempts)) { + const failureCount = classification.reason === 'rate_limit' + ? rateLimitFailures + 1 + : transientFailures + 1; + const maximumAttempts = classification.reason === 'rate_limit' + ? dependencies.policy.rateLimitMaximumAttempts + : dependencies.policy.maximumAttempts; + if (!classification.retryable || failureCount >= maximumAttempts) { throw error; } - const delayMilliseconds = retryDelay(classification, attempt, dependencies); + const delayMilliseconds = retryDelay(classification, failureCount, dependencies); if (dependencies.clock.nowMilliseconds() + delayMilliseconds >= dependencies.deadlineAtMilliseconds) { throw new WorkflowQueueDeadlineError(); } dependencies.observer?.providerRetry?.({ reason: classification.reason, - attempt, + attempt: failureCount, delayMilliseconds, ...(classification.resetEpochSeconds === undefined ? {} : { resetEpochSeconds: classification.resetEpochSeconds }), }); await dependencies.delayPort.wait(delayMilliseconds); - return executeWithRetry(operation, dependencies, attempt + 1); + return executeWithRetry(operation, dependencies, classification.reason === 'transient' ? failureCount : transientFailures, classification.reason === 'rate_limit' ? failureCount : rateLimitFailures); } } const TRANSIENT_NETWORK_ERRORS = new Set([ @@ -65534,7 +65546,9 @@ function classifyWorkflowRunsError(error, clock) { const resetEpochSeconds = parseEpochSeconds(header(headers, 'x-ratelimit-reset')); const resetDelay = resetEpochSeconds === undefined ? undefined - : Math.max(0, resetEpochSeconds * 1000 - clock.nowMilliseconds()); + : resetEpochSeconds * 1000 > clock.nowMilliseconds() + ? resetEpochSeconds * 1000 - clock.nowMilliseconds() + : undefined; return { retryable: true, reason: 'rate_limit', @@ -65571,11 +65585,13 @@ function parseRetryAfter(value, clock) { if (!value) return undefined; const seconds = Number(value); - if (Number.isFinite(seconds) && seconds >= 0) - return Math.round(seconds * 1000); + if (Number.isFinite(seconds)) { + const milliseconds = Math.round(seconds * 1000); + return milliseconds > 0 ? milliseconds : undefined; + } const timestamp = Date.parse(value); - return Number.isFinite(timestamp) - ? Math.max(0, timestamp - clock.nowMilliseconds()) + return Number.isFinite(timestamp) && timestamp > clock.nowMilliseconds() + ? timestamp - clock.nowMilliseconds() : undefined; } function parseEpochSeconds(value) { diff --git a/build/github_action/src/data/repository/workflow/workflow_runs_retry.d.ts b/build/github_action/src/data/repository/workflow/workflow_runs_retry.d.ts index 269efca75..9e089f492 100644 --- a/build/github_action/src/data/repository/workflow/workflow_runs_retry.d.ts +++ b/build/github_action/src/data/repository/workflow/workflow_runs_retry.d.ts @@ -1,6 +1,7 @@ import type { WorkflowPollingDelayPort, WorkflowPollingObserverPort, WorkflowPollingRandomPort, WorkflowQueueClockPort } from '../../../application/ports/workflow_run_ports'; export interface WorkflowRunsRetryPolicy { maximumAttempts: number; + rateLimitMaximumAttempts: number; initialDelayMilliseconds: number; backoffMultiplier: number; maximumDelayMilliseconds: number; diff --git a/src/data/repository/workflow/__tests__/active_previous_workflow_runs_repository.test.ts b/src/data/repository/workflow/__tests__/active_previous_workflow_runs_repository.test.ts index 9d37c77e6..6a6ad7421 100644 --- a/src/data/repository/workflow/__tests__/active_previous_workflow_runs_repository.test.ts +++ b/src/data/repository/workflow/__tests__/active_previous_workflow_runs_repository.test.ts @@ -5,6 +5,8 @@ import type { GithubWorkflowRunsResponse, } from '../../../../infrastructure/github/ports/github_workflow_provider_ports'; import { ActivePreviousWorkflowRunsRepository } from '../active_previous_workflow_runs_repository'; +import { COPILOT_WORKFLOW_NAMES } from '../../../../application/policies/workflow_queue_policy'; +import { WORKFLOW_ACTIVE_STATUSES } from '../../../../utils/constants'; const listWorkflowRunsForRepo = jest.fn(); const listWorkflowRuns = jest.fn(); @@ -81,6 +83,25 @@ describe('ActivePreviousWorkflowRunsRepository', () => { expect(iterator).toHaveBeenCalledTimes(1); }); + it('counts all seven shared workflow names and five active statuses across every page', async () => { + const runs = COPILOT_WORKFLOW_NAMES.flatMap((name, nameIndex) => WORKFLOW_ACTIVE_STATUSES.map((status, statusIndex) => workflowRun({ + id: 1 + nameIndex * WORKFLOW_ACTIVE_STATUSES.length + statusIndex, + name, + status, + }))); + iterator.mockImplementation(async function* () { + yield { data: { workflow_runs: runs.slice(0, 17) } } as GithubWorkflowRunsResponse; + yield { data: { workflow_runs: runs.slice(17) } } as GithubWorkflowRunsResponse; + }); + const repository = new ActivePreviousWorkflowRunsRepository(client); + + await expect(repository.countActivePreviousRuns({ + ...query, + workflowNames: [...COPILOT_WORKFLOW_NAMES], + })).resolves.toBe(COPILOT_WORKFLOW_NAMES.length * WORKFLOW_ACTIVE_STATUSES.length); + expect(iterator).toHaveBeenCalledTimes(1); + }); + it('counts multiple earlier queue runs while excluding cancelled and skipped terminals', async () => { iterator.mockImplementation(async function* () { yield { @@ -154,6 +175,7 @@ describe('ActivePreviousWorkflowRunsRepository', () => { }); const repository = new ActivePreviousWorkflowRunsRepository(client, retryDelayPort, { maximumAttempts: 3, + rateLimitMaximumAttempts: 5, initialDelayMilliseconds: 10, backoffMultiplier: 2, maximumDelayMilliseconds: 100, diff --git a/src/data/repository/workflow/__tests__/workflow_runs_retry_policy.test.ts b/src/data/repository/workflow/__tests__/workflow_runs_retry_policy.test.ts index ed45c65aa..f528db6fa 100644 --- a/src/data/repository/workflow/__tests__/workflow_runs_retry_policy.test.ts +++ b/src/data/repository/workflow/__tests__/workflow_runs_retry_policy.test.ts @@ -36,6 +36,14 @@ describe('workflow runs retry policy', () => { expect(deps.observer.providerRetry).toHaveBeenCalledWith(expect.objectContaining({ reason: 'rate_limit' })); }); + it('uses the bounded fallback when Retry-After is non-positive', async () => { + const operation = jest.fn().mockRejectedValueOnce({ status: 429, response: { headers: { 'retry-after': '0' } } }).mockResolvedValue('ok'); + const deps = dependencies(); + + await expect(withWorkflowRunsRetry(operation, deps)).resolves.toBe('ok'); + expect(deps.delayPort.wait).toHaveBeenCalledWith(60000); + }); + it('honors x-ratelimit-reset for a rate-limited 403', async () => { const operation = jest.fn().mockRejectedValueOnce({ status: 403, @@ -48,6 +56,17 @@ describe('workflow runs retry policy', () => { expect(deps.observer.providerRetry).toHaveBeenCalledWith(expect.objectContaining({ resetEpochSeconds: 12 })); }); + it('uses the bounded fallback when x-ratelimit-reset is expired', async () => { + const operation = jest.fn().mockRejectedValueOnce({ + status: 403, + response: { headers: { 'x-ratelimit-remaining': '0', 'x-ratelimit-reset': '12' } }, + }).mockResolvedValue('ok'); + const deps = dependencies({ clock: { nowMilliseconds: jest.fn().mockReturnValue(13_000) } }); + + await expect(withWorkflowRunsRetry(operation, deps)).resolves.toBe('ok'); + expect(deps.delayPort.wait).toHaveBeenCalledWith(60000); + }); + it('classifies a message-based rate-limited 403 and retries it', async () => { const operation = jest.fn() .mockRejectedValueOnce({ status: 403, response: { data: { message: 'You have exceeded the secondary rate limit.' } } }) @@ -82,6 +101,33 @@ describe('workflow runs retry policy', () => { expect(deps.delayPort.wait.mock.calls.map(([delay]: [number]) => delay)).toEqual([50, 250, 250]); }); + it('exhausts rate-limit retries independently and preserves the original provider error', async () => { + const providerError = { status: 429, message: 'rate-limited provider failure' }; + let nowMilliseconds = 0; + const operation = jest.fn().mockRejectedValue(providerError); + const deps = dependencies({ + clock: { nowMilliseconds: jest.fn(() => nowMilliseconds) }, + delayPort: { wait: jest.fn(async (delay: number) => { nowMilliseconds += delay; }) }, + deadlineAtMilliseconds: 1_000_000, + }); + + await expect(withWorkflowRunsRetry(operation, deps)).rejects.toBe(providerError); + expect(operation).toHaveBeenCalledTimes(5); + expect(deps.delayPort.wait).toHaveBeenCalledTimes(4); + }); + + it('keeps transient and rate-limit backoff counters independent', async () => { + const operation = jest.fn() + .mockRejectedValueOnce({ status: 503 }) + .mockRejectedValueOnce({ status: 429 }) + .mockResolvedValue('ok'); + const deps = dependencies(); + + await expect(withWorkflowRunsRetry(operation, deps)).resolves.toBe('ok'); + expect(deps.delayPort.wait.mock.calls.map(([delay]: [number]) => delay)).toEqual([1000, 60000]); + expect(deps.observer.providerRetry.mock.calls.map(([observation]) => observation.attempt)).toEqual([1, 1]); + }); + it('exhausts transient retries without converting the provider failure to success', async () => { const providerError = { statusCode: 503, message: 'transient provider failure' }; const operation = jest.fn().mockRejectedValue(providerError); diff --git a/src/data/repository/workflow/active_previous_workflow_runs_repository.ts b/src/data/repository/workflow/active_previous_workflow_runs_repository.ts index a1f09342d..1de95e404 100644 --- a/src/data/repository/workflow/active_previous_workflow_runs_repository.ts +++ b/src/data/repository/workflow/active_previous_workflow_runs_repository.ts @@ -56,6 +56,12 @@ export class ActivePreviousWorkflowRunsRepository implements PreviousWorkflowRun return withWorkflowRunsRetry(async () => { let activeRunCount = 0; + // Keep one complete sequential traversal: GitHub cannot safely express + // the seven shared workflow names, five active statuses, or the strict + // lower-ID predicate in this endpoint. Do not add provider filters or + // early-stop on page order; a matching run may occur on a later page. + // The residual cost is deep-history pagination, with retries restarting + // from page one, in exchange for an exact fail-closed count. for await (const response of this.client.paginate.iterator(method, parameters)) { activeRunCount += extractWorkflowRuns(response) .filter(run => isActivePreviousRun(run, query, names)).length; diff --git a/src/data/repository/workflow/workflow_runs_retry.ts b/src/data/repository/workflow/workflow_runs_retry.ts index 1bd6f5d23..3c1df7117 100644 --- a/src/data/repository/workflow/workflow_runs_retry.ts +++ b/src/data/repository/workflow/workflow_runs_retry.ts @@ -11,6 +11,7 @@ import { export interface WorkflowRunsRetryPolicy { maximumAttempts: number; + rateLimitMaximumAttempts: number; initialDelayMilliseconds: number; backoffMultiplier: number; maximumDelayMilliseconds: number; @@ -21,6 +22,7 @@ export interface WorkflowRunsRetryPolicy { export const WORKFLOW_RUNS_RETRY_POLICY: WorkflowRunsRetryPolicy = { maximumAttempts: 5, + rateLimitMaximumAttempts: 5, initialDelayMilliseconds: 1000, backoffMultiplier: 2, maximumDelayMilliseconds: 30000, @@ -72,13 +74,14 @@ export function withWorkflowRunsRetry( }, deadlineAtMilliseconds: Number.POSITIVE_INFINITY, }; - return executeWithRetry(operation, dependencies, 1); + return executeWithRetry(operation, dependencies, 0, 0); } async function executeWithRetry( operation: () => Promise, dependencies: WorkflowRunsRetryDependencies, - attempt: number, + transientFailures: number, + rateLimitFailures: number, ): Promise { if (dependencies.clock.nowMilliseconds() >= dependencies.deadlineAtMilliseconds) { throw new WorkflowQueueDeadlineError(); @@ -88,25 +91,35 @@ async function executeWithRetry( return await operation(); } catch (error: unknown) { const classification = classifyWorkflowRunsError(error, dependencies.clock); - if (!classification.retryable - || (classification.reason === 'transient' && attempt >= dependencies.policy.maximumAttempts)) { + const failureCount = classification.reason === 'rate_limit' + ? rateLimitFailures + 1 + : transientFailures + 1; + const maximumAttempts = classification.reason === 'rate_limit' + ? dependencies.policy.rateLimitMaximumAttempts + : dependencies.policy.maximumAttempts; + if (!classification.retryable || failureCount >= maximumAttempts) { throw error; } - const delayMilliseconds = retryDelay(classification, attempt, dependencies); + const delayMilliseconds = retryDelay(classification, failureCount, dependencies); if (dependencies.clock.nowMilliseconds() + delayMilliseconds >= dependencies.deadlineAtMilliseconds) { throw new WorkflowQueueDeadlineError(); } dependencies.observer?.providerRetry?.({ reason: classification.reason, - attempt, + attempt: failureCount, delayMilliseconds, ...(classification.resetEpochSeconds === undefined ? {} : { resetEpochSeconds: classification.resetEpochSeconds }), }); await dependencies.delayPort.wait(delayMilliseconds); - return executeWithRetry(operation, dependencies, attempt + 1); + return executeWithRetry( + operation, + dependencies, + classification.reason === 'transient' ? failureCount : transientFailures, + classification.reason === 'rate_limit' ? failureCount : rateLimitFailures, + ); } } @@ -181,7 +194,9 @@ function classifyWorkflowRunsError( const resetEpochSeconds = parseEpochSeconds(header(headers, 'x-ratelimit-reset')); const resetDelay = resetEpochSeconds === undefined ? undefined - : Math.max(0, resetEpochSeconds * 1000 - clock.nowMilliseconds()); + : resetEpochSeconds * 1000 > clock.nowMilliseconds() + ? resetEpochSeconds * 1000 - clock.nowMilliseconds() + : undefined; return { retryable: true, reason: 'rate_limit', @@ -217,10 +232,13 @@ function header(headers: unknown, name: string): string | undefined { function parseRetryAfter(value: string | undefined, clock: WorkflowQueueClockPort): number | undefined { if (!value) return undefined; const seconds = Number(value); - if (Number.isFinite(seconds) && seconds >= 0) return Math.round(seconds * 1000); + if (Number.isFinite(seconds)) { + const milliseconds = Math.round(seconds * 1000); + return milliseconds > 0 ? milliseconds : undefined; + } const timestamp = Date.parse(value); - return Number.isFinite(timestamp) - ? Math.max(0, timestamp - clock.nowMilliseconds()) + return Number.isFinite(timestamp) && timestamp > clock.nowMilliseconds() + ? timestamp - clock.nowMilliseconds() : undefined; } From fde72dee827c3914f25025e37294a2f6d742e819 Mon Sep 17 00:00:00 2001 From: Efra Espada Date: Tue, 1 Sep 2026 08:43:34 +0000 Subject: [PATCH 4/6] fix workflow run endpoint compatibility fallback --- build/cli/index.js | 11 +++-- build/github_action/index.js | 11 +++-- docs/development/architecture.mdx | 9 ++++ docs/features.mdx | 9 +++- ..._previous_workflow_runs_repository.test.ts | 48 +++++++++++++++++++ ...ctive_previous_workflow_runs_repository.ts | 11 +++-- 6 files changed, 83 insertions(+), 16 deletions(-) diff --git a/build/cli/index.js b/build/cli/index.js index d03bade54..7634d9124 100755 --- a/build/cli/index.js +++ b/build/cli/index.js @@ -69574,17 +69574,18 @@ class ActivePreviousWorkflowRunsRepository { if (workflowNames.length === 0 && query.workflowName.trim().length === 0) { throw new Error('GitHub workflow name is unavailable; refusing to bypass sequential execution.'); } - const method = query.workflowIdentifier && workflowNames.length === 0 - ? this.client.rest.actions.listWorkflowRuns - : this.client.rest.actions.listWorkflowRunsForRepo; + const actions = this.client.rest.actions; + const workflowIdentifier = workflowNames.length === 0 ? query.workflowIdentifier : undefined; + const workflowMethod = workflowIdentifier ? actions.listWorkflowRuns : undefined; + const method = workflowMethod ?? actions.listWorkflowRunsForRepo; if (!method) throw new Error('GitHub workflow-scoped runs endpoint is unavailable.'); const parameters = { owner: query.owner, repo: query.repository, per_page: 100, - ...(method === this.client.rest.actions.listWorkflowRuns && query.workflowIdentifier - ? { workflow_id: query.workflowIdentifier } + ...(workflowMethod && workflowIdentifier + ? { workflow_id: workflowIdentifier } : {}), }; const names = workflowNames.length > 0 ? workflowNames : [query.workflowName]; diff --git a/build/github_action/index.js b/build/github_action/index.js index b97bbdc24..9dd1a23cf 100644 --- a/build/github_action/index.js +++ b/build/github_action/index.js @@ -65346,17 +65346,18 @@ class ActivePreviousWorkflowRunsRepository { if (workflowNames.length === 0 && query.workflowName.trim().length === 0) { throw new Error('GitHub workflow name is unavailable; refusing to bypass sequential execution.'); } - const method = query.workflowIdentifier && workflowNames.length === 0 - ? this.client.rest.actions.listWorkflowRuns - : this.client.rest.actions.listWorkflowRunsForRepo; + const actions = this.client.rest.actions; + const workflowIdentifier = workflowNames.length === 0 ? query.workflowIdentifier : undefined; + const workflowMethod = workflowIdentifier ? actions.listWorkflowRuns : undefined; + const method = workflowMethod ?? actions.listWorkflowRunsForRepo; if (!method) throw new Error('GitHub workflow-scoped runs endpoint is unavailable.'); const parameters = { owner: query.owner, repo: query.repository, per_page: 100, - ...(method === this.client.rest.actions.listWorkflowRuns && query.workflowIdentifier - ? { workflow_id: query.workflowIdentifier } + ...(workflowMethod && workflowIdentifier + ? { workflow_id: workflowIdentifier } : {}), }; const names = workflowNames.length > 0 ? workflowNames : [query.workflowName]; diff --git a/docs/development/architecture.mdx b/docs/development/architecture.mdx index 7f3bbea9e..0581ae383 100644 --- a/docs/development/architecture.mdx +++ b/docs/development/architecture.mdx @@ -33,3 +33,12 @@ transient HTTP/network failures, server wait headers, and malformed pages are classified there and never become a synthetic zero count. Retry logging exposes only sanitized reason, attempt, delay, and reset metadata. Queue-bearing jobs use a 120-minute workflow timeout, leaving 30 minutes of headroom beyond the queue budget. + +When a query has a workflow identifier but no workflow-name list, the repository uses +the optional workflow-scoped provider endpoint when available. Repository-only clients +fall back to `listWorkflowRunsForRepo` without sending `workflow_id`; an error from an +invoked provider endpoint is not converted into a capability fallback. Exact counting +still requires exhaustive pagination: the current provider contract cannot express the +seven workflow names, five active statuses, and strict lower-ID predicate as one safe +server-side request. `per_page: 100` and one traversal minimize fan-out, but deep-history +API pressure remains a bounded-retry residual risk rather than a correctness shortcut. diff --git a/docs/features.mdx b/docs/features.mdx index 27bcc49fb..839f8c920 100644 --- a/docs/features.mdx +++ b/docs/features.mdx @@ -132,11 +132,18 @@ GitHub's native [concurrency](https://docs.github.com/en/actions/using-workflows ### How it works 1. At the start of each run (except welcome/single-action-only flows), the action resolves the current workflow file from `GITHUB_WORKFLOW_REF`. -2. It performs one paginated repository workflow-runs traversal per poll with `per_page: 100`, then locally filters the active statuses (`in_progress`, `queued`, `requested`, `waiting`, and `pending`), the seven known Copilot/Task mutation workflow names, and runs with a **lower run ID** (i.e. started earlier). +2. It performs one paginated repository workflow-runs traversal per poll with `per_page: 100`, then locally filters the active statuses (`in_progress`, `queued`, `requested`, `waiting`, and `pending`), the seven known Copilot/Task mutation workflow names, and runs with a **lower run ID** (i.e. started earlier). For compatibility queries that provide a workflow identifier without names, it uses the workflow-scoped endpoint when the provider exposes it; otherwise it uses the repository endpoint without `workflow_id`. 3. Provider failures fail closed. Transient 408/5xx/network errors use bounded exponential retry; HTTP 429 and rate-limited 403 responses honor `Retry-After` or `x-ratelimit-reset`, then use a slower bounded fallback. Diagnostics contain only the retry reason, attempt, delay, and safe reset timestamp metadata. 4. If any such run exists, the action polls immediately and then uses adaptive 5s, 10s, 20s, 40s, and 60s maximum delays with bounded ±20% jitter. The absolute queue wait is limited to 90 minutes. 5. When no earlier active run in the mutation queue remains, the action continues. A provider failure or queue deadline never becomes an empty result, so setup and mutation work cannot proceed with an unknown queue state. +The queue deliberately traverses every provider page because exact counting must detect +matching runs on later pages. GitHub's current adapter contract cannot safely combine +all seven workflow names, all five active statuses, and the strict lower-ID predicate +in one server-side filter. A full traversal with `per_page: 100` and one sequential +request path reduces fan-out while preserving correctness; deep-history pagination +therefore remains an explicit API-pressure risk, not an early-stop optimization. + So you get a **repository-wide mutation queue**: multiple triggers for the same workflow (e.g. many issue edits) and related Copilot workflows run sequentially. This conservative scope prevents two workflows from changing shared branches, issue metadata, or release state at the same time. Read-only CI may keep its own native concurrency policy. ### Example diff --git a/src/data/repository/workflow/__tests__/active_previous_workflow_runs_repository.test.ts b/src/data/repository/workflow/__tests__/active_previous_workflow_runs_repository.test.ts index 6a6ad7421..15f685e82 100644 --- a/src/data/repository/workflow/__tests__/active_previous_workflow_runs_repository.test.ts +++ b/src/data/repository/workflow/__tests__/active_previous_workflow_runs_repository.test.ts @@ -151,8 +151,56 @@ describe('ActivePreviousWorkflowRunsRepository', () => { }); }); + it('falls back to repository traversal when the workflow endpoint is unavailable', async () => { + iterator.mockImplementation(async function* () { + yield { + data: { + workflow_runs: [workflowRun({ id: 199, name: 'Copilot - Issue', status: WORKFLOW_STATUS.PENDING })], + }, + } as GithubWorkflowRunsResponse; + }); + const fallbackClient = { + rest: { actions: { listWorkflowRunsForRepo } }, + paginate: { iterator }, + } as unknown as GithubWorkflowRunsClient; + const repository = new ActivePreviousWorkflowRunsRepository(fallbackClient); + + await expect(repository.countActivePreviousRuns({ + ...query, + workflowNames: undefined, + workflowIdentifier: 'copilot_issue.yml', + })).resolves.toBe(1); + expect(iterator).toHaveBeenCalledWith(listWorkflowRunsForRepo, { + owner: 'org', + repo: 'repo', + per_page: 100, + }); + }); + + it('does not switch endpoints after a scoped provider failure', async () => { + iterator.mockImplementation(async function* () { + yield* []; + throw { status: 404 }; + }); + const repository = new ActivePreviousWorkflowRunsRepository(client); + + await expect(repository.countActivePreviousRuns({ + ...query, + workflowNames: undefined, + workflowIdentifier: 'copilot_issue.yml', + })).rejects.toMatchObject({ status: 404 }); + expect(iterator).toHaveBeenCalledWith(listWorkflowRuns, { + owner: 'org', + repo: 'repo', + per_page: 100, + workflow_id: 'copilot_issue.yml', + }); + expect(iterator).not.toHaveBeenCalledWith(listWorkflowRunsForRepo, expect.anything()); + }); + it('rejects malformed provider pages instead of treating them as empty', async () => { iterator.mockImplementation(async function* () { + yield { data: { workflow_runs: [] } } as GithubWorkflowRunsResponse; yield { data: {} } as GithubWorkflowRunsResponse; }); const repository = new ActivePreviousWorkflowRunsRepository(client); diff --git a/src/data/repository/workflow/active_previous_workflow_runs_repository.ts b/src/data/repository/workflow/active_previous_workflow_runs_repository.ts index 1de95e404..ca7024be0 100644 --- a/src/data/repository/workflow/active_previous_workflow_runs_repository.ts +++ b/src/data/repository/workflow/active_previous_workflow_runs_repository.ts @@ -40,16 +40,17 @@ export class ActivePreviousWorkflowRunsRepository implements PreviousWorkflowRun if (workflowNames.length === 0 && query.workflowName.trim().length === 0) { throw new Error('GitHub workflow name is unavailable; refusing to bypass sequential execution.'); } - const method = query.workflowIdentifier && workflowNames.length === 0 - ? this.client.rest.actions.listWorkflowRuns - : this.client.rest.actions.listWorkflowRunsForRepo; + const actions = this.client.rest.actions; + const workflowIdentifier = workflowNames.length === 0 ? query.workflowIdentifier : undefined; + const workflowMethod = workflowIdentifier ? actions.listWorkflowRuns : undefined; + const method = workflowMethod ?? actions.listWorkflowRunsForRepo; if (!method) throw new Error('GitHub workflow-scoped runs endpoint is unavailable.'); const parameters = { owner: query.owner, repo: query.repository, per_page: 100, - ...(method === this.client.rest.actions.listWorkflowRuns && query.workflowIdentifier - ? { workflow_id: query.workflowIdentifier } + ...(workflowMethod && workflowIdentifier + ? { workflow_id: workflowIdentifier } : {}), }; const names = workflowNames.length > 0 ? workflowNames : [query.workflowName]; From cb30b759e387b43a74aac5a6c661af6de4bfc75c Mon Sep 17 00:00:00 2001 From: Efra Espada Date: Tue, 1 Sep 2026 10:19:30 +0000 Subject: [PATCH 5/6] feat: gate release and hotfix workflows before mutations --- .github/workflows/hotfix_workflow.yml | 18 ++ .github/workflows/release_workflow.yml | 18 ++ action.yml | 3 + build/cli/index.js | 7 +- build/cli/src/actions/main_run_lifecycle.d.ts | 2 +- build/cli/src/utils/constants.d.ts | 1 + build/github_action/index.js | 30 +++- .../src/actions/main_run_lifecycle.d.ts | 2 +- build/github_action/src/utils/constants.d.ts | 1 + docs/features.mdx | 14 ++ scripts/validate-workflow-contract.cjs | 161 +++++++++++++++++- setup/workflows/hotfix_workflow.yml | 18 ++ setup/workflows/release_workflow.yml | 18 ++ src/actions/__tests__/github_action.test.ts | 48 ++++++ src/actions/common_action.ts | 2 +- src/actions/github_action.ts | 26 ++- src/actions/main_run_lifecycle.ts | 4 +- .../validate_workflow_contract.test.ts | 60 +++++++ src/utils/constants.ts | 1 + 19 files changed, 413 insertions(+), 21 deletions(-) diff --git a/.github/workflows/hotfix_workflow.yml b/.github/workflows/hotfix_workflow.yml index ebd0583d6..3a79ddc41 100644 --- a/.github/workflows/hotfix_workflow.yml +++ b/.github/workflows/hotfix_workflow.yml @@ -21,9 +21,27 @@ on: default: '-1' jobs: + queue-gate: + name: Wait for previous mutation workflows + runs-on: [self-hosted, codex] + timeout-minutes: 120 + permissions: + actions: read + contents: read + steps: + - uses: actions/checkout@v5 + with: + persist-credentials: false + - name: Admit workflow to mutation queue + uses: ./ + with: + queue-gate-only: 'true' + token: ${{ github.token }} + prepare-version-files: name: Prepare files for hotfix runs-on: [self-hosted, codex] + needs: queue-gate timeout-minutes: 15 permissions: contents: write diff --git a/.github/workflows/release_workflow.yml b/.github/workflows/release_workflow.yml index eab7d6a4d..addab8919 100644 --- a/.github/workflows/release_workflow.yml +++ b/.github/workflows/release_workflow.yml @@ -21,9 +21,27 @@ on: default: '-1' jobs: + queue-gate: + name: Wait for previous mutation workflows + runs-on: [self-hosted, codex] + timeout-minutes: 120 + permissions: + actions: read + contents: read + steps: + - uses: actions/checkout@v5 + with: + persist-credentials: false + - name: Admit workflow to mutation queue + uses: ./ + with: + queue-gate-only: 'true' + token: ${{ github.token }} + prepare-version-files: name: Prepare files for release runs-on: [self-hosted, codex] + needs: queue-gate timeout-minutes: 15 permissions: contents: write diff --git a/action.yml b/action.yml index 9d440e65b..cc4b88891 100644 --- a/action.yml +++ b/action.yml @@ -414,6 +414,9 @@ inputs: token: description: "Fine-grained personal access token for branch and project operations" required: true + queue-gate-only: + description: "Admit this workflow to the sequential queue and return before constructing or executing a project action." + default: "false" agent-provider: description: "Single agent provider used by findings and fixer tasks: codex, opencode, or cursor. A provider failure is terminal; no fallback provider is attempted." default: "codex" diff --git a/build/cli/index.js b/build/cli/index.js index 7634d9124..c7611b0ae 100755 --- a/build/cli/index.js +++ b/build/cli/index.js @@ -52564,7 +52564,7 @@ async function mainRun(execution, projectBoardCommandPort, latestTagQueryPort, l (0, logger_1.logDebugInfo)(`Event: ${execution.eventName}, actor: ${execution.actor}, repo: ${repository.owner}/${repository.repo}, debug: ${execution.debug}`); if (!execution.welcome) { // Queue before setup or route work so executions cannot overlap mutations. - await (0, main_run_lifecycle_1.waitForPreviousWorkflowRuns)(execution, repository); + await (0, main_run_lifecycle_1.waitForPreviousWorkflowRuns)(execution.tokens.token, repository); } await (0, execution_setup_composition_root_1.createSetupExecutionUseCase)(latestTagQueryPort).invoke(execution); (0, logger_1.clearAccumulatedLogs)(); @@ -53345,12 +53345,12 @@ function buildPreviousWorkflowRunsQuery(repository) { } return query; } -async function waitForPreviousWorkflowRuns(execution, repository) { +async function waitForPreviousWorkflowRuns(token, repository) { const query = buildPreviousWorkflowRunsQuery(repository); if (process.env.GITHUB_ACTIONS === 'true' && !Number.isSafeInteger(query.currentRunId)) { throw new Error('GitHub workflow identity is unavailable; refusing to bypass sequential execution.'); } - await (0, workflow_queue_composition_root_1.createWaitForPreviousWorkflowRunsUseCase)(execution.tokens.token) + await (0, workflow_queue_composition_root_1.createWaitForPreviousWorkflowRunsUseCase)(token) .invoke(query) .catch(() => { // Provider/Octokit errors can contain response bodies, URLs, @@ -72577,6 +72577,7 @@ exports.INPUT_KEYS = { SINGLE_ACTION_CHANGELOG: 'single-action-changelog', // Tokens TOKEN: 'token', + QUEUE_GATE_ONLY: 'queue-gate-only', // Agent selection AGENT_PROVIDER: 'agent-provider', AGENT_MODEL_PROVIDER: 'agent-model-provider', diff --git a/build/cli/src/actions/main_run_lifecycle.d.ts b/build/cli/src/actions/main_run_lifecycle.d.ts index 07e065a6c..3982bcace 100644 --- a/build/cli/src/actions/main_run_lifecycle.d.ts +++ b/build/cli/src/actions/main_run_lifecycle.d.ts @@ -12,7 +12,7 @@ export declare class WorkflowQueueFailureError extends Error { constructor(); } export declare function buildPreviousWorkflowRunsQuery(repository: RepositoryCoordinates): PreviousWorkflowRunsQuery; -export declare function waitForPreviousWorkflowRuns(execution: Execution, repository: RepositoryCoordinates): Promise; +export declare function waitForPreviousWorkflowRuns(token: string, repository: RepositoryCoordinates): Promise; export declare function logWelcomeMessage(execution: Execution): void; export declare function runTokenExecution(execution: Execution, routeHandlers: MainRunRouteHandlers): Promise; export declare function runNoIssueExecution(execution: Execution, routeHandlers: MainRunRouteHandlers): Promise; diff --git a/build/cli/src/utils/constants.d.ts b/build/cli/src/utils/constants.d.ts index c43f22fb6..8512680be 100644 --- a/build/cli/src/utils/constants.d.ts +++ b/build/cli/src/utils/constants.d.ts @@ -53,6 +53,7 @@ export declare const INPUT_KEYS: { readonly SINGLE_ACTION_TITLE: "single-action-title"; readonly SINGLE_ACTION_CHANGELOG: "single-action-changelog"; readonly TOKEN: "token"; + readonly QUEUE_GATE_ONLY: "queue-gate-only"; readonly AGENT_PROVIDER: "agent-provider"; readonly AGENT_MODEL_PROVIDER: "agent-model-provider"; readonly AGENT_EFFORT: "agent-effort"; diff --git a/build/github_action/index.js b/build/github_action/index.js index 9dd1a23cf..108727e0f 100644 --- a/build/github_action/index.js +++ b/build/github_action/index.js @@ -48086,7 +48086,7 @@ async function mainRun(execution, projectBoardCommandPort, latestTagQueryPort, l (0, logger_1.logDebugInfo)(`Event: ${execution.eventName}, actor: ${execution.actor}, repo: ${repository.owner}/${repository.repo}, debug: ${execution.debug}`); if (!execution.welcome) { // Queue before setup or route work so executions cannot overlap mutations. - await (0, main_run_lifecycle_1.waitForPreviousWorkflowRuns)(execution, repository); + await (0, main_run_lifecycle_1.waitForPreviousWorkflowRuns)(execution.tokens.token, repository); } await (0, execution_setup_composition_root_1.createSetupExecutionUseCase)(latestTagQueryPort).invoke(execution); (0, logger_1.clearAccumulatedLogs)(); @@ -48244,24 +48244,29 @@ const input_boolean_policy_1 = __nccwpck_require__(18330); const github_action_execution_1 = __nccwpck_require__(39691); const github_event_inputs_1 = __nccwpck_require__(63452); const common_action_1 = __nccwpck_require__(42238); +const main_run_lifecycle_1 = __nccwpck_require__(916); const constants_1 = __nccwpck_require__(15415); const logger_1 = __nccwpck_require__(91151); const lifecycle_state_composition_root_1 = __nccwpck_require__(4673); const copilot_evidence_composition_root_1 = __nccwpck_require__(64686); const github_action_summary_composition_root_1 = __nccwpck_require__(75305); async function runGitHubAction() { + if ((0, input_boolean_policy_1.isEnabledInput)((0, github_action_input_1.getGithubActionInput)(constants_1.INPUT_KEYS.QUEUE_GATE_ONLY))) { + await runQueueGateOnly(); + return; + } const eventInputs = (0, github_event_inputs_1.buildGithubActionEventInputs)({ payload: github.context.payload, eventName: github.context.eventName, actor: github.context.actor, repo: github.context.repo, }); - const projectBoard = (0, project_board_composition_root_1.createProjectBoardCompositionRoot)(); (0, logger_1.logInfo)('GitHub Action: runGitHubAction started.'); const debug = (0, input_boolean_policy_1.isEnabledInput)((0, github_action_input_1.getGithubActionInput)(constants_1.INPUT_KEYS.DEBUG)); if (debug) { (0, logger_1.logInfo)('Debug mode is enabled. Full logs will be included in the report.'); } + const projectBoard = (0, project_board_composition_root_1.createProjectBoardCompositionRoot)(); const execution = await (0, github_action_execution_1.buildGithubActionExecution)({ debug, eventInputs, @@ -48274,6 +48279,22 @@ async function runGitHubAction() { const issueContentPort = (0, issue_content_composition_root_1.createIssueContentCompositionRoot)(); await (0, github_action_completion_1.finishGithubAction)(execution, results, (0, issue_interaction_composition_root_1.createIssueNotificationRepository)(), new configuration_handler_1.ConfigurationHandler(issueContentPort), (0, copilot_evidence_composition_root_1.createCopilotEvidenceCompositionRoot)(), (0, github_action_summary_composition_root_1.createGithubActionSummaryCompositionRoot)()); } +async function runQueueGateOnly() { + try { + const eventInputs = (0, github_event_inputs_1.buildGithubActionEventInputs)({ + payload: github.context.payload, + eventName: github.context.eventName, + actor: github.context.actor, + repo: github.context.repo, + }); + const token = (0, github_action_input_1.getGithubActionInput)(constants_1.INPUT_KEYS.TOKEN, { required: true }); + await (0, main_run_lifecycle_1.waitForPreviousWorkflowRuns)(token, eventInputs.repo); + } + catch { + (0, logger_1.logError)(main_run_lifecycle_1.WORKFLOW_QUEUE_FAILURE_MESSAGE); + throw new main_run_lifecycle_1.WorkflowQueueFailureError(); + } +} // Only auto-run when executed as the action entry (not when imported by tests) if (typeof process.env.JEST_WORKER_ID === 'undefined') { runGitHubAction() @@ -49144,12 +49165,12 @@ function buildPreviousWorkflowRunsQuery(repository) { } return query; } -async function waitForPreviousWorkflowRuns(execution, repository) { +async function waitForPreviousWorkflowRuns(token, repository) { const query = buildPreviousWorkflowRunsQuery(repository); if (process.env.GITHUB_ACTIONS === 'true' && !Number.isSafeInteger(query.currentRunId)) { throw new Error('GitHub workflow identity is unavailable; refusing to bypass sequential execution.'); } - await (0, workflow_queue_composition_root_1.createWaitForPreviousWorkflowRunsUseCase)(execution.tokens.token) + await (0, workflow_queue_composition_root_1.createWaitForPreviousWorkflowRunsUseCase)(token) .invoke(query) .catch(() => { // Provider/Octokit errors can contain response bodies, URLs, @@ -68429,6 +68450,7 @@ exports.INPUT_KEYS = { SINGLE_ACTION_CHANGELOG: 'single-action-changelog', // Tokens TOKEN: 'token', + QUEUE_GATE_ONLY: 'queue-gate-only', // Agent selection AGENT_PROVIDER: 'agent-provider', AGENT_MODEL_PROVIDER: 'agent-model-provider', diff --git a/build/github_action/src/actions/main_run_lifecycle.d.ts b/build/github_action/src/actions/main_run_lifecycle.d.ts index 07e065a6c..3982bcace 100644 --- a/build/github_action/src/actions/main_run_lifecycle.d.ts +++ b/build/github_action/src/actions/main_run_lifecycle.d.ts @@ -12,7 +12,7 @@ export declare class WorkflowQueueFailureError extends Error { constructor(); } export declare function buildPreviousWorkflowRunsQuery(repository: RepositoryCoordinates): PreviousWorkflowRunsQuery; -export declare function waitForPreviousWorkflowRuns(execution: Execution, repository: RepositoryCoordinates): Promise; +export declare function waitForPreviousWorkflowRuns(token: string, repository: RepositoryCoordinates): Promise; export declare function logWelcomeMessage(execution: Execution): void; export declare function runTokenExecution(execution: Execution, routeHandlers: MainRunRouteHandlers): Promise; export declare function runNoIssueExecution(execution: Execution, routeHandlers: MainRunRouteHandlers): Promise; diff --git a/build/github_action/src/utils/constants.d.ts b/build/github_action/src/utils/constants.d.ts index c43f22fb6..8512680be 100644 --- a/build/github_action/src/utils/constants.d.ts +++ b/build/github_action/src/utils/constants.d.ts @@ -53,6 +53,7 @@ export declare const INPUT_KEYS: { readonly SINGLE_ACTION_TITLE: "single-action-title"; readonly SINGLE_ACTION_CHANGELOG: "single-action-changelog"; readonly TOKEN: "token"; + readonly QUEUE_GATE_ONLY: "queue-gate-only"; readonly AGENT_PROVIDER: "agent-provider"; readonly AGENT_MODEL_PROVIDER: "agent-model-provider"; readonly AGENT_EFFORT: "agent-effort"; diff --git a/docs/features.mdx b/docs/features.mdx index 839f8c920..7781abb3c 100644 --- a/docs/features.mdx +++ b/docs/features.mdx @@ -129,6 +129,20 @@ Codex is the default runtime for the repository's AI feature paths. OpenCode rem GitHub's native [concurrency](https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#concurrency) can cancel in-progress runs when a new one starts (`cancel-in-progress: true`) and can retain only one pending run. Copilot adds an application-level queue: every started Copilot/Task mutation run waits for earlier active runs, including runs from the other Copilot event workflows, so intermediate issue changes are not discarded by a native concurrency group. +Release and hotfix workflows use a separate first `queue-gate` job. That job invokes +the Action with the internal `queue-gate-only` mode and only the ephemeral +`${{ github.token }}` read permissions; it admits the run and exits before project +composition, agent provisioning, setup, or mutation work. The version-preparation and +publication jobs depend on that gate transitively, so a failed or skipped admission +cannot start a write-capable descendant. The setup templates use `vypdev/copilot@v2` +for this gate and therefore require a published v2 bundle that supports +`queue-gate-only`. + +This workflow-level gate is distinct from the ordinary Action-level queue wait: the +latter runs inside a fully constructed Action execution for general Copilot mutation +triggers, while the former is intentionally control-plane-only and must not construct +an execution or publish results. + ### How it works 1. At the start of each run (except welcome/single-action-only flows), the action resolves the current workflow file from `GITHUB_WORKFLOW_REF`. diff --git a/scripts/validate-workflow-contract.cjs b/scripts/validate-workflow-contract.cjs index 9d5e5df97..1c7fbb376 100644 --- a/scripts/validate-workflow-contract.cjs +++ b/scripts/validate-workflow-contract.cjs @@ -11,6 +11,7 @@ const workflowDirectories = [ ]; const QUEUE_WAIT_MINUTES = 90; const MIN_QUEUE_JOB_TIMEOUT_MINUTES = 120; + function assertQueueBudget(queueWaitMinutes, minimumJobTimeoutMinutes) { if (!Number.isFinite(queueWaitMinutes) || !Number.isFinite(minimumJobTimeoutMinutes) @@ -19,6 +20,7 @@ function assertQueueBudget(queueWaitMinutes, minimumJobTimeoutMinutes) { } } assertQueueBudget(QUEUE_WAIT_MINUTES, MIN_QUEUE_JOB_TIMEOUT_MINUTES); + const QUEUE_WORKFLOW_MANIFEST = Object.freeze([ ['copilot_commit.yml', 'Copilot - Commit', 'copilot-commits'], ['copilot_issue.yml', 'Copilot - Issue', 'copilot-issues'], @@ -28,6 +30,12 @@ const QUEUE_WORKFLOW_MANIFEST = Object.freeze([ ['hotfix_workflow.yml', 'Task - Hotfix', 'tag'], ['release_workflow.yml', 'Task - Release', 'tag'], ].map(([file, workflowName, jobId]) => ({ file, workflowName, jobId }))); + +const MUTATION_WORKFLOW_MANIFEST = Object.freeze([ + { file: 'release_workflow.yml', workflowName: 'Task - Release' }, + { file: 'hotfix_workflow.yml', workflowName: 'Task - Hotfix' }, +]); + const requiredAgentInputs = [ 'agent-provider', 'agent-model-provider', 'agent-model', 'agent-effort', 'agent-command', 'findings-provider', 'findings-model-provider', 'findings-model', 'findings-effort', 'findings-command', @@ -49,6 +57,10 @@ function isCopilotAction(step) { && (step.uses === './' || /(?:^|\/)copilot@/.test(step.uses)); } +function isQueueGateAction(step) { + return isCopilotAction(step) && step.with?.['queue-gate-only'] === 'true'; +} + function runnerLabels(value) { return Array.isArray(value) ? value.map(String) : [String(value)]; } @@ -72,7 +84,7 @@ function assertAgentInputs(file, workflow) { const relativeFile = relativeWorkflow(file); for (const [jobId, job] of Object.entries(workflow.jobs ?? {})) { for (const [stepIndex, step] of (job.steps ?? []).entries()) { - if (!isCopilotAction(step)) continue; + if (!isCopilotAction(step) || isQueueGateAction(step)) continue; const missing = requiredAgentInputs.filter(input => !(input in (step.with ?? {}))); if (missing.length > 0) { throw new Error(`${relativeFile} job ${jobId} step ${stepIndex + 1} is missing agent inputs: ${missing.join(', ')}.`); @@ -81,10 +93,145 @@ function assertAgentInputs(file, workflow) { } } +function needsFor(job) { + if (job?.needs === undefined) return []; + return (Array.isArray(job.needs) ? job.needs : [job.needs]).map(String); +} + +function assertNoUnsafeCondition(relativeFile, jobId, job) { + if (typeof job.if === 'string' && /\b(always|failure|cancelled)\s*\(/i.test(job.if)) { + throw new Error(`${relativeFile} job ${jobId} has a bypass-capable if condition.`); + } + for (const [stepIndex, step] of (job.steps ?? []).entries()) { + if (typeof step.if === 'string' && /\b(always|failure|cancelled)\s*\(/i.test(step.if)) { + throw new Error(`${relativeFile} job ${jobId} step ${stepIndex + 1} has a bypass-capable if condition.`); + } + } +} + +function assertNoConcurrency(relativeFile, workflow) { + if (workflow.concurrency !== undefined) { + throw new Error(`${relativeFile} must not define GitHub concurrency.`); + } + for (const [jobId, job] of Object.entries(workflow.jobs ?? {})) { + if (job.concurrency !== undefined) { + throw new Error(`${relativeFile} job ${jobId} must not define GitHub concurrency.`); + } + } +} + +function assertQueueGateJob(file, workflow, expectedUses) { + const relativeFile = relativeWorkflow(file); + const queueGate = workflow.jobs?.['queue-gate']; + if (!queueGate) throw new Error(`${relativeFile} must define queue-gate.`); + if (queueGate['timeout-minutes'] !== MIN_QUEUE_JOB_TIMEOUT_MINUTES) { + throw new Error(`${relativeFile} queue-gate must have timeout-minutes ${MIN_QUEUE_JOB_TIMEOUT_MINUTES}.`); + } + const permissions = queueGate.permissions ?? {}; + const permissionKeys = Object.keys(permissions).sort(); + if (permissionKeys.join(',') !== 'actions,contents' + || permissions.actions !== 'read' + || permissions.contents !== 'read') { + throw new Error(`${relativeFile} queue-gate must have only actions: read and contents: read permissions.`); + } + if (queueGate.env !== undefined || queueGate['continue-on-error'] !== undefined && queueGate['continue-on-error'] !== false) { + throw new Error(`${relativeFile} queue-gate must not define bypass or agent environment.`); + } + assertNoUnsafeCondition(relativeFile, 'queue-gate', queueGate); + + const steps = queueGate.steps ?? []; + if (steps.length !== 2 || steps.some(step => step.run !== undefined)) { + throw new Error(`${relativeFile} queue-gate may contain only a safe checkout and one gate action.`); + } + const checkout = steps[0]; + if (typeof checkout?.uses !== 'string' || !/^actions\/checkout@/.test(checkout.uses) + || checkout.with?.['persist-credentials'] !== false + || Object.keys(checkout.with ?? {}).some(key => key !== 'persist-credentials')) { + throw new Error(`${relativeFile} queue-gate checkout must set persist-credentials: false.`); + } + const action = steps[1]; + if (action?.uses !== expectedUses || !isQueueGateAction(action)) { + throw new Error(`${relativeFile} queue-gate must invoke ${expectedUses} with queue-gate-only: 'true'.`); + } + const actionInputs = action.with ?? {}; + if (actionInputs.token !== '${{ github.token }}' + || Object.keys(actionInputs).some(key => key !== 'queue-gate-only' && key !== 'token')) { + throw new Error(`${relativeFile} queue-gate must pass only queue-gate-only and github.token.`); + } + const gateText = JSON.stringify(queueGate); + if (/\b(secrets\.|PAT|API_KEY|AGENT_|CODEX_|OPENCODE_|CURSOR_|ANTHROPIC_|OPENROUTER_)/i.test(gateText)) { + throw new Error(`${relativeFile} queue-gate must not contain PAT, provider secrets, or agent environment.`); + } +} + +function assertExactNeeds(relativeFile, jobId, job, expected) { + const actual = needsFor(job); + if (actual.length !== expected.length || actual.some((value, index) => value !== expected[index])) { + throw new Error(`${relativeFile} job ${jobId} must need exactly ${expected.join(', ') || 'no jobs'}.`); + } +} + +function assertTransitiveQueueGateAncestry(file, workflow, gateJobId) { + const relativeFile = relativeWorkflow(file); + const jobs = workflow.jobs ?? {}; + const ancestry = new Map(); + const visiting = new Set(); + const reachesGate = (jobId) => { + if (jobId === gateJobId) return true; + if (ancestry.has(jobId)) return ancestry.get(jobId); + if (visiting.has(jobId)) throw new Error(`${relativeFile} has a cycle in job needs.`); + const job = jobs[jobId]; + if (!job) throw new Error(`${relativeFile} references missing job ${jobId}.`); + visiting.add(jobId); + const result = needsFor(job).some(parent => reachesGate(parent)); + visiting.delete(jobId); + ancestry.set(jobId, result); + return result; + }; + + for (const [jobId, job] of Object.entries(jobs)) { + assertNoUnsafeCondition(relativeFile, jobId, job); + if (jobId !== gateJobId && !reachesGate(jobId)) { + throw new Error(`${relativeFile} job ${jobId} is not a transitive descendant of ${gateJobId}.`); + } + } +} + +function assertMutationWorkflow(file, workflow) { + const relativeFile = relativeWorkflow(file); + const manifest = MUTATION_WORKFLOW_MANIFEST.find(entry => relativeFile.endsWith(`/${entry.file}`)); + if (!manifest) return false; + if (workflow.name !== manifest.workflowName) { + throw new Error(`${relativeFile} must have workflow name ${JSON.stringify(manifest.workflowName)}.`); + } + const setup = relativeFile.startsWith('setup/workflows/'); + const expectedJobs = setup + ? ['queue-gate', 'prepare-version-files', 'tag'] + : ['queue-gate', 'prepare-version-files', 'prepare-compiled-files', 'tag']; + const actualJobs = Object.keys(workflow.jobs ?? {}); + if (actualJobs.length !== expectedJobs.length || expectedJobs.some(jobId => !actualJobs.includes(jobId))) { + throw new Error(`${relativeFile} must define the exact gate-first job graph.`); + } + assertNoConcurrency(relativeFile, workflow); + assertQueueGateJob(file, workflow, setup ? 'vypdev/copilot@v2' : './'); + assertExactNeeds(relativeFile, 'queue-gate', workflow.jobs['queue-gate'], []); + assertExactNeeds(relativeFile, 'prepare-version-files', workflow.jobs['prepare-version-files'], ['queue-gate']); + if (setup) { + assertExactNeeds(relativeFile, 'tag', workflow.jobs.tag, ['prepare-version-files']); + } else { + assertExactNeeds(relativeFile, 'prepare-compiled-files', workflow.jobs['prepare-compiled-files'], ['prepare-version-files']); + assertExactNeeds(relativeFile, 'tag', workflow.jobs.tag, ['prepare-compiled-files']); + } + assertTransitiveQueueGateAncestry(file, workflow, 'queue-gate'); + return true; +} + function assertQueueWorkflow(file, workflow) { const relativeFile = relativeWorkflow(file); const manifest = QUEUE_WORKFLOW_MANIFEST.find(entry => relativeFile.endsWith(`/${entry.file}`)); if (!manifest) return; + assertNoConcurrency(relativeFile, workflow); + if (assertMutationWorkflow(file, workflow)) return; if (workflow.name !== manifest.workflowName) { throw new Error(`${relativeFile} must have workflow name ${JSON.stringify(manifest.workflowName)}.`); } @@ -94,9 +241,6 @@ function assertQueueWorkflow(file, workflow) { || queueJob['timeout-minutes'] < MIN_QUEUE_JOB_TIMEOUT_MINUTES) { throw new Error(`${relativeFile} queue job ${manifest.jobId} must have timeout-minutes >= ${MIN_QUEUE_JOB_TIMEOUT_MINUTES}.`); } - if (workflow.concurrency !== undefined || queueJob.concurrency !== undefined) { - throw new Error(`${relativeFile} must not define workflow or queue-job concurrency.`); - } if (!(queueJob.steps ?? []).some(isCopilotAction)) { throw new Error(`${relativeFile} queue job ${manifest.jobId} must invoke the Copilot action.`); } @@ -110,9 +254,7 @@ function assertQueueWorkflow(file, workflow) { function assertSequentialMutationWorkflow(file, workflow) { const relativeFile = relativeWorkflow(file); if (!QUEUE_WORKFLOW_MANIFEST.some(entry => relativeFile.endsWith(`/${entry.file}`))) return; - if (workflow.concurrency !== undefined) { - throw new Error(`${relativeFile} must not define GitHub concurrency.`); - } + assertNoConcurrency(relativeFile, workflow); } function validateWorkflow(file, workflow) { @@ -147,10 +289,15 @@ module.exports = { QUEUE_WAIT_MINUTES, MIN_QUEUE_JOB_TIMEOUT_MINUTES, QUEUE_WORKFLOW_MANIFEST, + MUTATION_WORKFLOW_MANIFEST, assertAgentInputs, + assertMutationWorkflow, + assertNoConcurrency, assertQueueBudget, + assertQueueGateJob, assertQueueWorkflow, assertRunner, assertSequentialMutationWorkflow, + assertTransitiveQueueGateAncestry, validateWorkflow, }; diff --git a/setup/workflows/hotfix_workflow.yml b/setup/workflows/hotfix_workflow.yml index 4a7fef997..2034d0186 100644 --- a/setup/workflows/hotfix_workflow.yml +++ b/setup/workflows/hotfix_workflow.yml @@ -21,9 +21,27 @@ on: default: '-1' jobs: + queue-gate: + name: Wait for previous mutation workflows + runs-on: ubuntu-latest + timeout-minutes: 120 + permissions: + actions: read + contents: read + steps: + - uses: actions/checkout@v5 + with: + persist-credentials: false + - name: Admit workflow to mutation queue + uses: vypdev/copilot@v2 + with: + queue-gate-only: 'true' + token: ${{ github.token }} + prepare-version-files: name: Prepare files for hotfix runs-on: ubuntu-latest + needs: queue-gate timeout-minutes: 15 permissions: contents: write diff --git a/setup/workflows/release_workflow.yml b/setup/workflows/release_workflow.yml index c429a92d2..0e3921d37 100644 --- a/setup/workflows/release_workflow.yml +++ b/setup/workflows/release_workflow.yml @@ -21,9 +21,27 @@ on: default: '-1' jobs: + queue-gate: + name: Wait for previous mutation workflows + runs-on: ubuntu-latest + timeout-minutes: 120 + permissions: + actions: read + contents: read + steps: + - uses: actions/checkout@v5 + with: + persist-credentials: false + - name: Admit workflow to mutation queue + uses: vypdev/copilot@v2 + with: + queue-gate-only: 'true' + token: ${{ github.token }} + prepare-version-files: name: Prepare files for release runs-on: ubuntu-latest + needs: queue-gate timeout-minutes: 15 permissions: contents: write diff --git a/src/actions/__tests__/github_action.test.ts b/src/actions/__tests__/github_action.test.ts index adb135668..56b304c78 100644 --- a/src/actions/__tests__/github_action.test.ts +++ b/src/actions/__tests__/github_action.test.ts @@ -33,6 +33,17 @@ jest.mock('../common_action', () => ({ mainRun: (...args: unknown[]) => mockMainRun(...args), })); +const mockWaitForPreviousWorkflowRuns = jest.fn(); +jest.mock('../main_run_lifecycle', () => ({ + WORKFLOW_QUEUE_FAILURE_MESSAGE: 'Workflow queue check failed; sequential execution was not bypassed.', + WorkflowQueueFailureError: class WorkflowQueueFailureError extends Error { + constructor() { + super('Workflow queue check failed; sequential execution was not bypassed.'); + } + }, + waitForPreviousWorkflowRuns: (...args: unknown[]) => mockWaitForPreviousWorkflowRuns(...args), +})); + const mockProvision = jest.fn(); jest.mock('../../data/repository/agent_cli_provisioner', () => ({ AgentCliProvisioner: jest.fn().mockImplementation(() => ({ provision: mockProvision })), @@ -65,6 +76,7 @@ describe('runGitHubAction', () => { mockMainRun.mockResolvedValue([]); mockPublishInvoke.mockResolvedValue([]); mockStoreInvoke.mockResolvedValue([]); + mockWaitForPreviousWorkflowRuns.mockResolvedValue(undefined); }); it('builds Execution and calls mainRun', async () => { @@ -82,6 +94,42 @@ describe('runGitHubAction', () => { expect(execution.actor).toBe('test-actor'); }); + it('admits a queue-gate-only run before project composition or execution construction', async () => { + (core.getInput as jest.Mock).mockImplementation((key: string, opts?: { required?: boolean }) => { + if (key === INPUT_KEYS.QUEUE_GATE_ONLY) return 'true'; + if (opts?.required && key === INPUT_KEYS.TOKEN) return 'github-token'; + return ''; + }); + + await runGitHubAction(); + + expect(mockWaitForPreviousWorkflowRuns).toHaveBeenCalledWith( + 'github-token', + { owner: 'test-owner', repo: 'test-repo' }, + ); + expect(mockMainRun).not.toHaveBeenCalled(); + expect(mockGetProjectDetail).not.toHaveBeenCalled(); + expect(mockPublishInvoke).not.toHaveBeenCalled(); + expect(mockStoreInvoke).not.toHaveBeenCalled(); + }); + + it('fails a queue-gate-only run closed with the canonical sanitized error', async () => { + mockWaitForPreviousWorkflowRuns.mockRejectedValue(new Error('provider response body and token should not escape')); + (core.getInput as jest.Mock).mockImplementation((key: string, opts?: { required?: boolean }) => { + if (key === INPUT_KEYS.QUEUE_GATE_ONLY) return 'true'; + if (opts?.required && key === INPUT_KEYS.TOKEN) return 'github-token'; + return ''; + }); + + await expect(runGitHubAction()).rejects.toThrow( + 'Workflow queue check failed; sequential execution was not bypassed.', + ); + expect(mockMainRun).not.toHaveBeenCalled(); + expect(mockGetProjectDetail).not.toHaveBeenCalled(); + expect(mockPublishInvoke).not.toHaveBeenCalled(); + expect(mockStoreInvoke).not.toHaveBeenCalled(); + }); + it('calls finishWithResults (PublishResult and StoreConfiguration) after mainRun', async () => { await runGitHubAction(); diff --git a/src/actions/common_action.ts b/src/actions/common_action.ts index ef355bbdb..e79d78ea6 100644 --- a/src/actions/common_action.ts +++ b/src/actions/common_action.ts @@ -35,7 +35,7 @@ export async function mainRun( if (!execution.welcome) { // Queue before setup or route work so executions cannot overlap mutations. - await waitForPreviousWorkflowRuns(execution, repository); + await waitForPreviousWorkflowRuns(execution.tokens.token, repository); } await createSetupExecutionUseCase(latestTagQueryPort).invoke(execution); diff --git a/src/actions/github_action.ts b/src/actions/github_action.ts index 2f39236ed..0642e44a8 100644 --- a/src/actions/github_action.ts +++ b/src/actions/github_action.ts @@ -11,6 +11,7 @@ import { isEnabledInput } from './input_boolean_policy'; import { buildGithubActionExecution } from './github_action_execution'; import { buildGithubActionEventInputs } from './github_event_inputs'; import { mainRun } from './common_action'; +import { waitForPreviousWorkflowRuns, WorkflowQueueFailureError, WORKFLOW_QUEUE_FAILURE_MESSAGE } from './main_run_lifecycle'; import { INPUT_KEYS } from '../utils/constants'; import { logDebugInfo, logError, logInfo } from '../utils/logger'; import { createSynchronizeLifecycleStateUseCase } from '../infrastructure/composition/lifecycle_state_composition_root'; @@ -18,20 +19,25 @@ import { createCopilotEvidenceCompositionRoot } from '../infrastructure/composit import { createGithubActionSummaryCompositionRoot } from '../infrastructure/composition/github_action_summary_composition_root'; export async function runGitHubAction(): Promise { + if (isEnabledInput(getGithubActionInput(INPUT_KEYS.QUEUE_GATE_ONLY))) { + await runQueueGateOnly(); + return; + } + const eventInputs = buildGithubActionEventInputs({ payload: github.context.payload as Record, eventName: github.context.eventName, actor: github.context.actor, repo: github.context.repo, }); - const projectBoard = createProjectBoardCompositionRoot(); - logInfo('GitHub Action: runGitHubAction started.'); const debug = isEnabledInput(getGithubActionInput(INPUT_KEYS.DEBUG)); if (debug) { logInfo('Debug mode is enabled. Full logs will be included in the report.'); } + const projectBoard = createProjectBoardCompositionRoot(); + const execution = await buildGithubActionExecution({ debug, eventInputs, @@ -60,6 +66,22 @@ export async function runGitHubAction(): Promise { ); } +async function runQueueGateOnly(): Promise { + try { + const eventInputs = buildGithubActionEventInputs({ + payload: github.context.payload as Record, + eventName: github.context.eventName, + actor: github.context.actor, + repo: github.context.repo, + }); + const token = getGithubActionInput(INPUT_KEYS.TOKEN, { required: true }); + await waitForPreviousWorkflowRuns(token, eventInputs.repo); + } catch { + logError(WORKFLOW_QUEUE_FAILURE_MESSAGE); + throw new WorkflowQueueFailureError(); + } +} + // Only auto-run when executed as the action entry (not when imported by tests) if (typeof process.env.JEST_WORKER_ID === 'undefined') { runGitHubAction() diff --git a/src/actions/main_run_lifecycle.ts b/src/actions/main_run_lifecycle.ts index 031abfe49..f049cc739 100644 --- a/src/actions/main_run_lifecycle.ts +++ b/src/actions/main_run_lifecycle.ts @@ -46,14 +46,14 @@ export function buildPreviousWorkflowRunsQuery( } export async function waitForPreviousWorkflowRuns( - execution: Execution, + token: string, repository: RepositoryCoordinates, ): Promise { const query = buildPreviousWorkflowRunsQuery(repository); if (process.env.GITHUB_ACTIONS === 'true' && !Number.isSafeInteger(query.currentRunId)) { throw new Error('GitHub workflow identity is unavailable; refusing to bypass sequential execution.'); } - await createWaitForPreviousWorkflowRunsUseCase(execution.tokens.token) + await createWaitForPreviousWorkflowRunsUseCase(token) .invoke(query) .catch(() => { // Provider/Octokit errors can contain response bodies, URLs, diff --git a/src/tooling/__tests__/validate_workflow_contract.test.ts b/src/tooling/__tests__/validate_workflow_contract.test.ts index b6be625f4..177fe816b 100644 --- a/src/tooling/__tests__/validate_workflow_contract.test.ts +++ b/src/tooling/__tests__/validate_workflow_contract.test.ts @@ -62,6 +62,66 @@ describe('workflow contract validator', () => { } }); + it('validates the exact gate-first DAG for active and setup release/hotfix workflows', () => { + for (const directory of ['.github/workflows', 'setup/workflows']) { + for (const fileName of ['release_workflow.yml', 'hotfix_workflow.yml']) { + const file = path.join(process.cwd(), directory, fileName); + const workflow = yaml.load(readFileSync(file, 'utf8')) as Record; + expect(() => validateWorkflow(file, workflow)).not.toThrow(); + } + } + }); + + it('rejects a release workflow when a mutation job bypasses the queue gate', () => { + const workflow = { + name: 'Task - Release', + jobs: { + 'queue-gate': { + 'runs-on': 'ubuntu-latest', + 'timeout-minutes': 120, + permissions: { actions: 'read', contents: 'read' }, + steps: [ + { uses: 'actions/checkout@v5', with: { 'persist-credentials': false } }, + { uses: 'vypdev/copilot@v2', with: { 'queue-gate-only': 'true', token: '${{ github.token }}' } }, + ], + }, + 'prepare-version-files': { + 'runs-on': 'ubuntu-latest', + 'timeout-minutes': 15, + permissions: { contents: 'write' }, + needs: 'unrelated', + steps: [], + }, + unrelated: { 'runs-on': 'ubuntu-latest', steps: [] }, + 'prepare-compiled-files': { 'runs-on': 'ubuntu-latest', needs: 'prepare-version-files', steps: [] }, + tag: { 'runs-on': 'ubuntu-latest', needs: 'prepare-compiled-files', steps: [] }, + }, + }; + + expect(() => validateWorkflow(path.join(process.cwd(), 'setup/workflows/release_workflow.yml'), workflow)).toThrow(); + }); + + it.each([ + ['gate write permission', (gate: Record) => { gate.permissions = { actions: 'write', contents: 'read' }; }], + ['gate provider environment', (gate: Record) => { gate.env = { OPENAI_API_KEY: '${{ secrets.OPENAI_API_KEY }}' }; }], + ['gate false mode', (gate: Record) => { + const steps = gate.steps as Record[]; + (steps[1].with as Record)['queue-gate-only'] = 'false'; + }], + ['gate unsafe condition', (gate: Record) => { gate.if = '${{ always() }}'; }], + ['persistent gate checkout', (gate: Record) => { + const steps = gate.steps as Record[]; + (steps[0].with as Record)['persist-credentials'] = true; + }], + ])('rejects %s in a release gate', (_reason, mutate) => { + const file = path.join(process.cwd(), 'setup/workflows/release_workflow.yml'); + const workflow = JSON.parse(JSON.stringify(yaml.load(readFileSync(file, 'utf8')))) as { + jobs: Record>; + }; + mutate(workflow.jobs['queue-gate']); + expect(() => validateWorkflow(file, workflow)).toThrow(); + }); + it.each([ ['wrong workflow name', { ...validWorkflow, name: 'Wrong' }], ['missing timeout', { ...validWorkflow, jobs: { 'copilot-issues': { ...validWorkflow.jobs['copilot-issues'], 'timeout-minutes': undefined } } }], diff --git a/src/utils/constants.ts b/src/utils/constants.ts index 5bb213afa..cedc328c6 100644 --- a/src/utils/constants.ts +++ b/src/utils/constants.ts @@ -216,6 +216,7 @@ export const INPUT_KEYS = { // Tokens TOKEN: 'token', + QUEUE_GATE_ONLY: 'queue-gate-only', // Agent selection AGENT_PROVIDER: 'agent-provider', From 75dad46e2cf80b30210a61c23edfccd114e4c532 Mon Sep 17 00:00:00 2001 From: Efra Espada Date: Tue, 1 Sep 2026 10:53:53 +0000 Subject: [PATCH 6/6] [verified] close queue gate validator findings --- .github/workflows/hotfix_workflow.yml | 2 + .github/workflows/release_workflow.yml | 2 + scripts/validate-workflow-contract.cjs | 44 ++++++- src/actions/__tests__/github_action.test.ts | 17 +++ .../validate_workflow_contract.test.ts | 113 ++++++++++++++++++ 5 files changed, 172 insertions(+), 6 deletions(-) diff --git a/.github/workflows/hotfix_workflow.yml b/.github/workflows/hotfix_workflow.yml index 3a79ddc41..89262f33a 100644 --- a/.github/workflows/hotfix_workflow.yml +++ b/.github/workflows/hotfix_workflow.yml @@ -158,6 +158,8 @@ jobs: runs-on: [self-hosted, codex] timeout-minutes: 120 needs: [ prepare-compiled-files ] + permissions: + contents: read env: AGENT_PROVIDER: ${{ vars.AGENT_PROVIDER || 'codex' }} AGENT_MODEL_PROVIDER: ${{ vars.AGENT_MODEL_PROVIDER || 'openai' }} diff --git a/.github/workflows/release_workflow.yml b/.github/workflows/release_workflow.yml index addab8919..9da3b548f 100644 --- a/.github/workflows/release_workflow.yml +++ b/.github/workflows/release_workflow.yml @@ -158,6 +158,8 @@ jobs: runs-on: [self-hosted, codex] timeout-minutes: 120 needs: [ prepare-compiled-files ] + permissions: + contents: read env: AGENT_PROVIDER: ${{ vars.AGENT_PROVIDER || 'codex' }} AGENT_MODEL_PROVIDER: ${{ vars.AGENT_MODEL_PROVIDER || 'openai' }} diff --git a/scripts/validate-workflow-contract.cjs b/scripts/validate-workflow-contract.cjs index 1c7fbb376..21be8532e 100644 --- a/scripts/validate-workflow-contract.cjs +++ b/scripts/validate-workflow-contract.cjs @@ -10,7 +10,11 @@ const workflowDirectories = [ path.join(repositoryRoot, 'setup', 'workflows'), ]; const QUEUE_WAIT_MINUTES = 90; -const MIN_QUEUE_JOB_TIMEOUT_MINUTES = 120; +const QUEUE_GATE_TIMEOUT_MINUTES = 120; +const PREPARE_VERSION_TIMEOUT_MINUTES = 15; +const PREPARE_COMPILED_TIMEOUT_MINUTES = 20; +const TAG_TIMEOUT_MINUTES = 120; +const MIN_QUEUE_JOB_TIMEOUT_MINUTES = QUEUE_GATE_TIMEOUT_MINUTES; function assertQueueBudget(queueWaitMinutes, minimumJobTimeoutMinutes) { if (!Number.isFinite(queueWaitMinutes) @@ -84,7 +88,7 @@ function assertAgentInputs(file, workflow) { const relativeFile = relativeWorkflow(file); for (const [jobId, job] of Object.entries(workflow.jobs ?? {})) { for (const [stepIndex, step] of (job.steps ?? []).entries()) { - if (!isCopilotAction(step) || isQueueGateAction(step)) continue; + if (jobId === 'queue-gate' || !isCopilotAction(step) || isQueueGateAction(step)) continue; const missing = requiredAgentInputs.filter(input => !(input in (step.with ?? {}))); if (missing.length > 0) { throw new Error(`${relativeFile} job ${jobId} step ${stepIndex + 1} is missing agent inputs: ${missing.join(', ')}.`); @@ -124,9 +128,7 @@ function assertQueueGateJob(file, workflow, expectedUses) { const relativeFile = relativeWorkflow(file); const queueGate = workflow.jobs?.['queue-gate']; if (!queueGate) throw new Error(`${relativeFile} must define queue-gate.`); - if (queueGate['timeout-minutes'] !== MIN_QUEUE_JOB_TIMEOUT_MINUTES) { - throw new Error(`${relativeFile} queue-gate must have timeout-minutes ${MIN_QUEUE_JOB_TIMEOUT_MINUTES}.`); - } + assertExactTimeout(relativeFile, 'queue-gate', queueGate, QUEUE_GATE_TIMEOUT_MINUTES); const permissions = queueGate.permissions ?? {}; const permissionKeys = Object.keys(permissions).sort(); if (permissionKeys.join(',') !== 'actions,contents' @@ -134,7 +136,7 @@ function assertQueueGateJob(file, workflow, expectedUses) { || permissions.contents !== 'read') { throw new Error(`${relativeFile} queue-gate must have only actions: read and contents: read permissions.`); } - if (queueGate.env !== undefined || queueGate['continue-on-error'] !== undefined && queueGate['continue-on-error'] !== false) { + if (queueGate.env !== undefined || Object.prototype.hasOwnProperty.call(queueGate, 'continue-on-error')) { throw new Error(`${relativeFile} queue-gate must not define bypass or agent environment.`); } assertNoUnsafeCondition(relativeFile, 'queue-gate', queueGate); @@ -153,6 +155,9 @@ function assertQueueGateJob(file, workflow, expectedUses) { if (action?.uses !== expectedUses || !isQueueGateAction(action)) { throw new Error(`${relativeFile} queue-gate must invoke ${expectedUses} with queue-gate-only: 'true'.`); } + if (Object.prototype.hasOwnProperty.call(action, 'continue-on-error')) { + throw new Error(`${relativeFile} queue-gate action must not define continue-on-error.`); + } const actionInputs = action.with ?? {}; if (actionInputs.token !== '${{ github.token }}' || Object.keys(actionInputs).some(key => key !== 'queue-gate-only' && key !== 'token')) { @@ -171,6 +176,20 @@ function assertExactNeeds(relativeFile, jobId, job, expected) { } } +function assertExactTimeout(relativeFile, jobId, job, expected) { + if (job?.['timeout-minutes'] !== expected) { + throw new Error(`${relativeFile} job ${jobId} must have timeout-minutes ${expected}.`); + } +} + +function assertTagPermissions(relativeFile, job) { + const permissions = job?.permissions ?? {}; + const permissionKeys = Object.keys(permissions).sort(); + if (permissionKeys.length !== 1 || permissionKeys[0] !== 'contents' || permissions.contents !== 'read') { + throw new Error(`${relativeFile} tag must have only contents: read permissions.`); + } +} + function assertTransitiveQueueGateAncestry(file, workflow, gateJobId) { const relativeFile = relativeWorkflow(file); const jobs = workflow.jobs ?? {}; @@ -214,11 +233,18 @@ function assertMutationWorkflow(file, workflow) { } assertNoConcurrency(relativeFile, workflow); assertQueueGateJob(file, workflow, setup ? 'vypdev/copilot@v2' : './'); + assertExactTimeout(relativeFile, 'queue-gate', workflow.jobs['queue-gate'], QUEUE_GATE_TIMEOUT_MINUTES); + assertExactTimeout(relativeFile, 'prepare-version-files', workflow.jobs['prepare-version-files'], PREPARE_VERSION_TIMEOUT_MINUTES); assertExactNeeds(relativeFile, 'queue-gate', workflow.jobs['queue-gate'], []); assertExactNeeds(relativeFile, 'prepare-version-files', workflow.jobs['prepare-version-files'], ['queue-gate']); if (setup) { + assertExactTimeout(relativeFile, 'tag', workflow.jobs.tag, TAG_TIMEOUT_MINUTES); + assertTagPermissions(relativeFile, workflow.jobs.tag); assertExactNeeds(relativeFile, 'tag', workflow.jobs.tag, ['prepare-version-files']); } else { + assertExactTimeout(relativeFile, 'prepare-compiled-files', workflow.jobs['prepare-compiled-files'], PREPARE_COMPILED_TIMEOUT_MINUTES); + assertExactTimeout(relativeFile, 'tag', workflow.jobs.tag, TAG_TIMEOUT_MINUTES); + assertTagPermissions(relativeFile, workflow.jobs.tag); assertExactNeeds(relativeFile, 'prepare-compiled-files', workflow.jobs['prepare-compiled-files'], ['prepare-version-files']); assertExactNeeds(relativeFile, 'tag', workflow.jobs.tag, ['prepare-compiled-files']); } @@ -288,12 +314,18 @@ if (require.main === module) main(); module.exports = { QUEUE_WAIT_MINUTES, MIN_QUEUE_JOB_TIMEOUT_MINUTES, + QUEUE_GATE_TIMEOUT_MINUTES, + PREPARE_VERSION_TIMEOUT_MINUTES, + PREPARE_COMPILED_TIMEOUT_MINUTES, + TAG_TIMEOUT_MINUTES, QUEUE_WORKFLOW_MANIFEST, MUTATION_WORKFLOW_MANIFEST, assertAgentInputs, assertMutationWorkflow, assertNoConcurrency, assertQueueBudget, + assertExactTimeout, + assertTagPermissions, assertQueueGateJob, assertQueueWorkflow, assertRunner, diff --git a/src/actions/__tests__/github_action.test.ts b/src/actions/__tests__/github_action.test.ts index 56b304c78..05383d1e5 100644 --- a/src/actions/__tests__/github_action.test.ts +++ b/src/actions/__tests__/github_action.test.ts @@ -4,6 +4,10 @@ */ import * as core from '@actions/core'; +import * as projectBoardCompositionRoot from '../../infrastructure/composition/project_board_composition_root'; +import * as executionBuilder from '../github_action_execution'; +import * as agentRuntime from '../github_action_runtime'; +import * as actionCompletion from '../github_action_completion'; import { runGitHubAction } from '../github_action'; import { ACTIONS, INPUT_KEYS } from '../../utils/constants'; @@ -65,6 +69,11 @@ jest.mock('../../data/repository/project/project_board_query_repository', () => })), })); +const projectCompositionSpy = jest.spyOn(projectBoardCompositionRoot, 'createProjectBoardCompositionRoot'); +const executionBuilderSpy = jest.spyOn(executionBuilder, 'buildGithubActionExecution'); +const agentProvisioningSpy = jest.spyOn(agentRuntime, 'prepareGithubAgentRuntime'); +const finishActionSpy = jest.spyOn(actionCompletion, 'finishGithubAction'); + describe('runGitHubAction', () => { beforeEach(() => { jest.clearAllMocks(); @@ -108,6 +117,10 @@ describe('runGitHubAction', () => { { owner: 'test-owner', repo: 'test-repo' }, ); expect(mockMainRun).not.toHaveBeenCalled(); + expect(projectCompositionSpy).not.toHaveBeenCalled(); + expect(executionBuilderSpy).not.toHaveBeenCalled(); + expect(agentProvisioningSpy).not.toHaveBeenCalled(); + expect(finishActionSpy).not.toHaveBeenCalled(); expect(mockGetProjectDetail).not.toHaveBeenCalled(); expect(mockPublishInvoke).not.toHaveBeenCalled(); expect(mockStoreInvoke).not.toHaveBeenCalled(); @@ -125,6 +138,10 @@ describe('runGitHubAction', () => { 'Workflow queue check failed; sequential execution was not bypassed.', ); expect(mockMainRun).not.toHaveBeenCalled(); + expect(projectCompositionSpy).not.toHaveBeenCalled(); + expect(executionBuilderSpy).not.toHaveBeenCalled(); + expect(agentProvisioningSpy).not.toHaveBeenCalled(); + expect(finishActionSpy).not.toHaveBeenCalled(); expect(mockGetProjectDetail).not.toHaveBeenCalled(); expect(mockPublishInvoke).not.toHaveBeenCalled(); expect(mockStoreInvoke).not.toHaveBeenCalled(); diff --git a/src/tooling/__tests__/validate_workflow_contract.test.ts b/src/tooling/__tests__/validate_workflow_contract.test.ts index 177fe816b..58347cd9f 100644 --- a/src/tooling/__tests__/validate_workflow_contract.test.ts +++ b/src/tooling/__tests__/validate_workflow_contract.test.ts @@ -7,6 +7,10 @@ interface ContractModule { assertQueueWorkflow(file: string, workflow: Record): void; assertRunner(file: string, workflow: Record): void; MIN_QUEUE_JOB_TIMEOUT_MINUTES: number; + QUEUE_GATE_TIMEOUT_MINUTES: number; + PREPARE_VERSION_TIMEOUT_MINUTES: number; + PREPARE_COMPILED_TIMEOUT_MINUTES: number; + TAG_TIMEOUT_MINUTES: number; QUEUE_WAIT_MINUTES: number; QUEUE_WORKFLOW_MANIFEST: readonly { file: string; workflowName: string; jobId: string }[]; assertQueueBudget(queueWaitMinutes: number, minimumJobTimeoutMinutes: number): void; @@ -17,6 +21,10 @@ const { assertQueueWorkflow, assertRunner, MIN_QUEUE_JOB_TIMEOUT_MINUTES, + QUEUE_GATE_TIMEOUT_MINUTES, + PREPARE_VERSION_TIMEOUT_MINUTES, + PREPARE_COMPILED_TIMEOUT_MINUTES, + TAG_TIMEOUT_MINUTES, QUEUE_WAIT_MINUTES, QUEUE_WORKFLOW_MANIFEST, assertQueueBudget, @@ -24,6 +32,29 @@ const { } = require('../../../scripts/validate-workflow-contract.cjs') as ContractModule; const queueFile = path.join(process.cwd(), '.github', 'workflows', 'copilot_issue.yml'); +const mutationDirectories = ['.github/workflows', 'setup/workflows'] as const; +const mutationWorkflowNames = ['release_workflow.yml', 'hotfix_workflow.yml'] as const; +type MutationWorkflow = { jobs: Record>; [key: string]: any }; + +function loadMutationWorkflow(directory: string, fileName: string): { file: string; workflow: MutationWorkflow } { + const file = path.join(process.cwd(), directory, fileName); + return { + file, + workflow: JSON.parse(JSON.stringify(yaml.load(readFileSync(file, 'utf8')))), + }; +} + +function expectMutationRejected( + directory: string, + fileName: string, + mutate: (workflow: MutationWorkflow) => void, + expectedMessage: string, +): void { + const { file, workflow } = loadMutationWorkflow(directory, fileName); + mutate(workflow); + expect(() => validateWorkflow(file, workflow)).toThrow(expectedMessage); +} + const validWorkflow = { name: 'Copilot - Issue', jobs: { @@ -72,6 +103,88 @@ describe('workflow contract validator', () => { } }); + it.each(mutationDirectories.flatMap((directory) => mutationWorkflowNames.map((fileName) => [directory, fileName] as const)))('enforces the exact queue, preparation, and tag budgets for %s/%s', (directory, fileName) => { + expectMutationRejected(directory, fileName, (workflow) => { + workflow.jobs['queue-gate']['timeout-minutes'] = QUEUE_GATE_TIMEOUT_MINUTES - 1; + }, 'queue-gate must have timeout-minutes 120'); + expectMutationRejected(directory, fileName, (workflow) => { + workflow.jobs['prepare-version-files']['timeout-minutes'] = PREPARE_VERSION_TIMEOUT_MINUTES - 1; + }, 'job prepare-version-files must have timeout-minutes 15'); + expectMutationRejected(directory, fileName, (workflow) => { + workflow.jobs.tag['timeout-minutes'] = TAG_TIMEOUT_MINUTES - 1; + }, 'job tag must have timeout-minutes 120'); + }); + + it.each(mutationWorkflowNames)('enforces the active compiled-files budget for %s', (fileName) => { + expectMutationRejected('.github/workflows', fileName, (workflow) => { + workflow.jobs['prepare-compiled-files']['timeout-minutes'] = PREPARE_COMPILED_TIMEOUT_MINUTES - 1; + }, 'job prepare-compiled-files must have timeout-minutes 20'); + }); + + it('requires explicit least-privilege read permissions for active tag jobs', () => { + for (const fileName of mutationWorkflowNames) { + expectMutationRejected('.github/workflows', fileName, (workflow) => { + delete workflow.jobs.tag.permissions; + }, 'tag must have only contents: read permissions'); + expectMutationRejected('.github/workflows', fileName, (workflow) => { + workflow.jobs.tag.permissions = { contents: 'write' }; + }, 'tag must have only contents: read permissions'); + } + }); + + it.each([ + ['direct pre-gate mutation', (workflow: MutationWorkflow) => { + workflow.jobs['prepare-version-files'].needs = []; + }, 'job prepare-version-files must need exactly queue-gate'], + ['broken transitive edge', (workflow: MutationWorkflow) => { + workflow.jobs.tag.needs = ['queue-gate']; + }, 'job tag must need exactly prepare-compiled-files'], + ['always bypass', (workflow: MutationWorkflow) => { + workflow.jobs['prepare-version-files'].if = '${{ always() }}'; + }, 'bypass-capable if condition'], + ['failure bypass', (workflow: MutationWorkflow) => { + workflow.jobs['prepare-version-files'].if = '${{ failure() }}'; + }, 'bypass-capable if condition'], + ['cancelled bypass', (workflow: MutationWorkflow) => { + workflow.jobs['prepare-version-files'].if = '${{ cancelled() }}'; + }, 'bypass-capable if condition'], + ['queue-gate continue-on-error', (workflow: MutationWorkflow) => { + workflow.jobs['queue-gate']['continue-on-error'] = false; + }, 'queue-gate must not define bypass'], + ['gate-step continue-on-error', (workflow: MutationWorkflow) => { + workflow.jobs['queue-gate'].steps[1]['continue-on-error'] = true; + }, 'queue-gate action must not define continue-on-error'], + ['gate write permission', (workflow: MutationWorkflow) => { + workflow.jobs['queue-gate'].permissions.actions = 'write'; + }, 'queue-gate must have only actions: read'], + ['missing actions read', (workflow: MutationWorkflow) => { + delete workflow.jobs['queue-gate'].permissions.actions; + }, 'queue-gate must have only actions: read'], + ['PAT token', (workflow: MutationWorkflow) => { + workflow.jobs['queue-gate'].steps[1].with.token = '${{ secrets.PAT }}'; + }, 'queue-gate must pass only queue-gate-only and github.token'], + ['credential persistence', (workflow: MutationWorkflow) => { + workflow.jobs['queue-gate'].steps[0].with['persist-credentials'] = true; + }, 'queue-gate checkout must set persist-credentials: false'], + ['pre-gate run', (workflow: MutationWorkflow) => { + workflow.jobs['queue-gate'].steps[0].run = 'printf unsafe'; + }, 'queue-gate may contain only a safe checkout'], + ['missing gate-only', (workflow: MutationWorkflow) => { + delete workflow.jobs['queue-gate'].steps[1].with['queue-gate-only']; + }, 'queue-gate must invoke'], + ['false gate-only', (workflow: MutationWorkflow) => { + workflow.jobs['queue-gate'].steps[1].with['queue-gate-only'] = 'false'; + }, 'queue-gate must invoke'], + ['workflow concurrency', (workflow: MutationWorkflow) => { + workflow.concurrency = { group: 'unsafe' }; + }, 'must not define GitHub concurrency'], + ['job concurrency', (workflow: MutationWorkflow) => { + workflow.jobs.tag.concurrency = { group: 'unsafe' }; + }, 'job tag must not define GitHub concurrency'], + ])('rejects isolated %s mutation in a real release fixture', (_reason, mutate, message) => { + expectMutationRejected('.github/workflows', 'release_workflow.yml', mutate, message); + }); + it('rejects a release workflow when a mutation job bypasses the queue gate', () => { const workflow = { name: 'Task - Release',