From 915303cda576ea9d4578aa6af08b277f24a14a59 Mon Sep 17 00:00:00 2001 From: Nick Fields Date: Tue, 29 Sep 2020 11:05:03 -0400 Subject: [PATCH] fix: surface exit code from spawned process patch: added dotenv sample configuration and command to run locally fix: added timeout_seconds input and handle timeout properly --- .github/workflows/ci_cd.yml | 137 ++++++++++++++++++++++++++---------- action.yml | 7 +- dist/exec.js | 8 ++- dist/index.js | 58 +++++++++++---- package-lock.json | 6 ++ package.json | 2 + sample.env | 6 ++ src/exec.js | 8 ++- src/index.js | 58 +++++++++++---- 9 files changed, 221 insertions(+), 69 deletions(-) create mode 100644 sample.env diff --git a/.github/workflows/ci_cd.yml b/.github/workflows/ci_cd.yml index 8eab28c..d30bbd4 100644 --- a/.github/workflows/ci_cd.yml +++ b/.github/workflows/ci_cd.yml @@ -42,11 +42,15 @@ jobs: command: npm install this-isnt-a-real-package-name-zzz - uses: nick-invision/assert-action@v1 with: - expected: true - actual: ${{ steps.sad_path_wait_sec.outputs.total_attempts == '3' && steps.sad_path_wait_sec.outcome == 'failure' }} + expected: 3 + actual: ${{ steps.sad_path_wait_sec.outputs.total_attempts }} - uses: nick-invision/assert-action@v1 with: - expected: 'Command failed: npm install' + expected: failure + actual: ${{ steps.sad_path_wait_sec.outcome }} + - uses: nick-invision/assert-action@v1 + with: + expected: 'Final attempt failed' actual: ${{ steps.sad_path_wait_sec.outputs.exit_error }} comparison: contains @@ -60,35 +64,12 @@ jobs: command: node -e "process.exit(1)" - uses: nick-invision/assert-action@v1 with: - expected: true - actual: ${{ steps.sad_path_error.outputs.total_attempts == '2' && steps.sad_path_error.outcome == 'failure' }} - - - name: sad-path (timeout) - id: sad_path_timeout - uses: ./ - continue-on-error: true - with: - timeout_minutes: 1 - max_attempts: 2 - command: node -e "(async()=>await new Promise(r => setTimeout(r, 120000)))()" + expected: 2 + actual: ${{ steps.sad_path_error.outputs.total_attempts }} - uses: nick-invision/assert-action@v1 with: - expected: true - actual: ${{ steps.sad_path_timeout.outputs.total_attempts == '2' && steps.sad_path_timeout.outcome == 'failure' }} - - - name: retry_on (timeout) - id: retry_on_timeout - uses: ./ - continue-on-error: true - with: - timeout_minutes: 1 - max_attempts: 2 - retry_on: timeout - command: node -e "(async()=>await new Promise(r => setTimeout(r, 120000)))()" - - uses: nick-invision/assert-action@v1 - with: - expected: true - actual: ${{ steps.retry_on_timeout.outputs.total_attempts == '2' && steps.retry_on_timeout.outcome == 'failure' }} + expected: failure + actual: ${{ steps.sad_path_error.outcome }} - name: retry_on (timeout) fails early if nonzero encountered id: retry_on_timeout_fail @@ -101,8 +82,16 @@ jobs: command: node -e "process.exit(2)" - uses: nick-invision/assert-action@v1 with: - expected: true - actual: ${{ steps.retry_on_timeout_fail.outputs.total_attempts == '1' && steps.retry_on_timeout_fail.outcome == 'failure' && steps.retry_on_timeout_fail.outputs.exit_code == '2' }} + expected: 1 + actual: ${{ steps.retry_on_timeout_fail.outputs.total_attempts }} + - uses: nick-invision/assert-action@v1 + with: + expected: failure + actual: ${{ steps.retry_on_timeout_fail.outcome }} + - uses: nick-invision/assert-action@v1 + with: + expected: 2 + actual: ${{ steps.retry_on_timeout_fail.outputs.exit_code }} - name: retry_on (nonzero) id: retry_on_nonzero @@ -111,26 +100,96 @@ jobs: with: timeout_minutes: 1 max_attempts: 2 - retry_on: timeout + retry_on: nonzero command: node -e "process.exit(2)" - uses: nick-invision/assert-action@v1 with: - expected: true - actual: ${{ steps.retry_on_nonzero.outputs.total_attempts == '2' && steps.retry_on_nonzero.outcome == 'failure' && steps.retry_on_nonzero.outputs.exit_code == '2' }} + expected: 2 + actual: ${{ steps.retry_on_nonzero.outputs.total_attempts }} + - uses: nick-invision/assert-action@v1 + with: + expected: failure + actual: ${{ steps.retry_on_nonzero.outcome }} + - uses: nick-invision/assert-action@v1 + with: + expected: 2 + actual: ${{ steps.retry_on_nonzero.outputs.exit_code }} + + + # timeout tests (takes longer to run so run last) + - name: sad-path (timeout) + id: sad_path_timeout + uses: ./ + continue-on-error: true + with: + timeout_seconds: 15 + max_attempts: 2 + command: node -e "(async()=>await new Promise(r => setTimeout(r, 120000)))()" + - uses: nick-invision/assert-action@v1 + with: + expected: 2 + actual: ${{ steps.sad_path_timeout.outputs.total_attempts }} + - uses: nick-invision/assert-action@v1 + with: + expected: failure + actual: ${{ steps.sad_path_timeout.outcome }} + + - name: retry_on (timeout) + id: retry_on_timeout + uses: ./ + continue-on-error: true + with: + timeout_seconds: 15 + max_attempts: 2 + retry_on: timeout + command: node -e "(async()=>await new Promise(r => setTimeout(r, 120000)))()" + - uses: nick-invision/assert-action@v1 + with: + expected: 2 + actual: ${{ steps.retry_on_timeout.outputs.total_attempts }} + - uses: nick-invision/assert-action@v1 + with: + expected: failure + actual: ${{ steps.retry_on_timeout.outcome }} - name: retry_on (nonzero) fails early if timeout encountered id: retry_on_nonzero_fail uses: ./ continue-on-error: true with: - timeout_minutes: 1 + timeout_seconds: 15 max_attempts: 2 - retry_on: timeout + retry_on: nonzero command: node -e "(async()=>await new Promise(r => setTimeout(r, 120000)))()" - uses: nick-invision/assert-action@v1 with: - expected: true - actual: ${{ steps.retry_on_nonzero_fail.outputs.total_attempts == '2' && steps.retry_on_nonzero_fail.outcome == 'failure' && steps.retry_on_nonzero_fail.outputs.exit_code == '2' }} + expected: 1 + actual: ${{ steps.retry_on_nonzero_fail.outputs.total_attempts }} + - uses: nick-invision/assert-action@v1 + with: + expected: failure + actual: ${{ steps.retry_on_nonzero_fail.outcome }} + - uses: nick-invision/assert-action@v1 + with: + expected: 1 + actual: ${{ steps.retry_on_nonzero_fail.outputs.exit_code }} + + - name: sad-path (timeout minutes) + id: sad_path_timeout_minutes + uses: ./ + continue-on-error: true + with: + timeout_minutes: 1 + max_attempts: 2 + command: node -e "(async()=>await new Promise(r => setTimeout(r, 120000)))()" + - uses: nick-invision/assert-action@v1 + with: + expected: 2 + actual: ${{ steps.sad_path_timeout.outputs.total_attempts }} + - uses: nick-invision/assert-action@v1 + with: + expected: failure + actual: ${{ steps.sad_path_timeout.outcome }} # runs on push to master only cd: diff --git a/action.yml b/action.yml index d03601f..491bbbe 100644 --- a/action.yml +++ b/action.yml @@ -2,8 +2,11 @@ name: Retry Step description: 'Retry a step on failure or timeout' inputs: timeout_minutes: - description: Minutes to wait before attempt times out - required: true + description: Minutes to wait before attempt times out. Must only specify either minutes or seconds + required: false + timeout_seconds: + description: Seconds to wait before attempt times out. Must only specify either minutes or seconds + required: false max_attempts: description: Number of attempts to make before failing the step required: true diff --git a/dist/exec.js b/dist/exec.js index f7f3f46..c5275ca 100644 --- a/dist/exec.js +++ b/dist/exec.js @@ -1,8 +1,12 @@ -const { execSync } = require('child_process'); +const { exec } = require('child_process'); const COMMAND = process.argv.splice(2)[0]; function run() { - execSync(COMMAND, { stdio: 'inherit' }); + exec(COMMAND, { stdio: 'inherit' }, (err) => { + if (err) { + process.exit(err.code); + } + }); } run(); diff --git a/dist/index.js b/dist/index.js index 1f08b60..c952f30 100644 --- a/dist/index.js +++ b/dist/index.js @@ -421,7 +421,8 @@ const ms = __webpack_require__(156); var kill = __webpack_require__(791); // inputs -const TIMEOUT_MINUTES = getInputNumber('timeout_minutes', true); +const TIMEOUT_MINUTES = getInputNumber('timeout_minutes', false); +const TIMEOUT_SECONDS = getInputNumber('timeout_seconds', false); const MAX_ATTEMPTS = getInputNumber('max_attempts', true); const COMMAND = getInput('command', { required: true }); const RETRY_WAIT_SECONDS = getInputNumber('retry_wait_seconds', false); @@ -439,6 +440,11 @@ function getInputNumber(id, required) { const input = getInput(id, { required }); const num = Number.parseInt(input); + // empty is ok + if (!input && !required) { + return; + } + if (!Number.isInteger(num)) { throw `Input ${id} only accepts numbers. Received ${input}`; } @@ -450,18 +456,51 @@ async function wait(ms) { return new Promise((r) => setTimeout(r, ms)); } +async function retryWait() { + const waitStart = Date.now(); + await wait(ms.seconds(RETRY_WAIT_SECONDS)); + debug(`Waited ${Date.now() - waitStart}ms`); + debug(`Configured wait: ${ms.seconds(RETRY_WAIT_SECONDS)}ms`); +} + +async function validateInputs() { + if ((!TIMEOUT_MINUTES && !TIMEOUT_SECONDS) || (TIMEOUT_MINUTES && TIMEOUT_SECONDS)) { + throw new Error('Must specify either timeout_minutes or timeout_seconds inputs'); + } + + if (TIMEOUT_SECONDS && TIMEOUT_SECONDS < RETRY_WAIT_SECONDS) { + throw new Error( + `timeout_seconds ${TIMEOUT_SECONDS}s less than retry_wait_seconds ${RETRY_WAIT_SECONDS}s` + ); + } +} + +function getTimeout() { + if (TIMEOUT_MINUTES) { + return ms.minutes(TIMEOUT_MINUTES); + } + + return ms.seconds(TIMEOUT_SECONDS); +} + async function runCmd() { - const end_time = Date.now() + ms.minutes(TIMEOUT_MINUTES); + const end_time = Date.now() + getTimeout(); exit = 0; done = false; var child = spawn('node', [__webpack_require__.ab + "exec.js", COMMAND], { stdio: 'inherit' }); - child.on('exit', (code) => { + child.on('exit', (code, signal) => { + debug(`Code: ${code}`); + debug(`Signal: ${signal}`); if (code > 0) { exit = code; } + // timeouts are killed manually + if (signal === 'SIGTERM') { + return; + } done = true; }); @@ -472,7 +511,7 @@ async function runCmd() { if (!done) { kill(child.pid); await retryWait(); - throw new Error(`Timeout of ${TIMEOUT_MINUTES}m hit`); + throw new Error(`Timeout of ${getTimeout()}ms hit`); } else if (exit > 0) { await retryWait(); throw new Error(`Child_process exited with error code ${exit}`); @@ -481,14 +520,9 @@ async function runCmd() { } } -async function retryWait() { - const waitStart = Date.now(); - await wait(ms.seconds(RETRY_WAIT_SECONDS)); - debug(`Waited ${Date.now() - waitStart}ms`); - debug(`Configured wait: ${ms.seconds(RETRY_WAIT_SECONDS)}ms`); -} - async function runAction() { + await validateInputs(); + for (let attempt = 1; attempt <= MAX_ATTEMPTS; attempt++) { try { // just keep overwriting attempts output @@ -506,7 +540,7 @@ async function runAction() { // error: nonzero throw error; } else { - warning(`Attempt ${attempt} failed. Reason:`, error.message); + warning(`Attempt ${attempt} failed. Reason: ${error.message}`); } } } diff --git a/package-lock.json b/package-lock.json index 67f4c65..d9c7d50 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1501,6 +1501,12 @@ "is-obj": "^1.0.0" } }, + "dotenv": { + "version": "8.2.0", + "resolved": "https://registry.npmjs.org/dotenv/-/dotenv-8.2.0.tgz", + "integrity": "sha512-8sJ78ElpbDJBHNeBzUbUVLsqKdccaa/BXF1uPTw3GrvQTBgrQrtObr2mUrE38vzYd8cEv+m/JBfDLioYcfXoaw==", + "dev": true + }, "duplexer2": { "version": "0.1.4", "resolved": "https://registry.npmjs.org/duplexer2/-/duplexer2-0.1.4.tgz", diff --git a/package.json b/package.json index 790a2c0..17c6450 100644 --- a/package.json +++ b/package.json @@ -3,6 +3,7 @@ "version": "0.0.0-managed-by-semantic-release", "description": "Retries a GitHub Action step on failure or timeout.", "scripts": { + "local": "node -r dotenv/config ./src/index.js", "prepare": "ncc build src/index.js" }, "repository": { @@ -27,6 +28,7 @@ "@semantic-release/changelog": "^3.0.6", "@semantic-release/git": "^7.0.18", "@zeit/ncc": "^0.20.5", + "dotenv": "8.2.0", "husky": "^3.1.0", "semantic-release": "^17.0.3" }, diff --git a/sample.env b/sample.env new file mode 100644 index 0000000..0dbd99c --- /dev/null +++ b/sample.env @@ -0,0 +1,6 @@ +INPUT_TIMEOUT_MINUTES=1 +INPUT_MAX_ATTEMPTS=3 +INPUT_COMMAND="node -e 'process.exit(99)'" +INPUT_RETRY_WAIT_SECONDS=10 +INPUT_POLLING_INTERVAL_SECONDS=1 +INPUT_RETRY_ON=any diff --git a/src/exec.js b/src/exec.js index f7f3f46..c5275ca 100644 --- a/src/exec.js +++ b/src/exec.js @@ -1,8 +1,12 @@ -const { execSync } = require('child_process'); +const { exec } = require('child_process'); const COMMAND = process.argv.splice(2)[0]; function run() { - execSync(COMMAND, { stdio: 'inherit' }); + exec(COMMAND, { stdio: 'inherit' }, (err) => { + if (err) { + process.exit(err.code); + } + }); } run(); diff --git a/src/index.js b/src/index.js index 68b7586..a370342 100644 --- a/src/index.js +++ b/src/index.js @@ -5,7 +5,8 @@ const ms = require('milliseconds'); var kill = require('tree-kill'); // inputs -const TIMEOUT_MINUTES = getInputNumber('timeout_minutes', true); +const TIMEOUT_MINUTES = getInputNumber('timeout_minutes', false); +const TIMEOUT_SECONDS = getInputNumber('timeout_seconds', false); const MAX_ATTEMPTS = getInputNumber('max_attempts', true); const COMMAND = getInput('command', { required: true }); const RETRY_WAIT_SECONDS = getInputNumber('retry_wait_seconds', false); @@ -23,6 +24,11 @@ function getInputNumber(id, required) { const input = getInput(id, { required }); const num = Number.parseInt(input); + // empty is ok + if (!input && !required) { + return; + } + if (!Number.isInteger(num)) { throw `Input ${id} only accepts numbers. Received ${input}`; } @@ -34,18 +40,51 @@ async function wait(ms) { return new Promise((r) => setTimeout(r, ms)); } +async function retryWait() { + const waitStart = Date.now(); + await wait(ms.seconds(RETRY_WAIT_SECONDS)); + debug(`Waited ${Date.now() - waitStart}ms`); + debug(`Configured wait: ${ms.seconds(RETRY_WAIT_SECONDS)}ms`); +} + +async function validateInputs() { + if ((!TIMEOUT_MINUTES && !TIMEOUT_SECONDS) || (TIMEOUT_MINUTES && TIMEOUT_SECONDS)) { + throw new Error('Must specify either timeout_minutes or timeout_seconds inputs'); + } + + if (TIMEOUT_SECONDS && TIMEOUT_SECONDS < RETRY_WAIT_SECONDS) { + throw new Error( + `timeout_seconds ${TIMEOUT_SECONDS}s less than retry_wait_seconds ${RETRY_WAIT_SECONDS}s` + ); + } +} + +function getTimeout() { + if (TIMEOUT_MINUTES) { + return ms.minutes(TIMEOUT_MINUTES); + } + + return ms.seconds(TIMEOUT_SECONDS); +} + async function runCmd() { - const end_time = Date.now() + ms.minutes(TIMEOUT_MINUTES); + const end_time = Date.now() + getTimeout(); exit = 0; done = false; var child = spawn('node', [join(__dirname, 'exec.js'), COMMAND], { stdio: 'inherit' }); - child.on('exit', (code) => { + child.on('exit', (code, signal) => { + debug(`Code: ${code}`); + debug(`Signal: ${signal}`); if (code > 0) { exit = code; } + // timeouts are killed manually + if (signal === 'SIGTERM') { + return; + } done = true; }); @@ -56,7 +95,7 @@ async function runCmd() { if (!done) { kill(child.pid); await retryWait(); - throw new Error(`Timeout of ${TIMEOUT_MINUTES}m hit`); + throw new Error(`Timeout of ${getTimeout()}ms hit`); } else if (exit > 0) { await retryWait(); throw new Error(`Child_process exited with error code ${exit}`); @@ -65,14 +104,9 @@ async function runCmd() { } } -async function retryWait() { - const waitStart = Date.now(); - await wait(ms.seconds(RETRY_WAIT_SECONDS)); - debug(`Waited ${Date.now() - waitStart}ms`); - debug(`Configured wait: ${ms.seconds(RETRY_WAIT_SECONDS)}ms`); -} - async function runAction() { + await validateInputs(); + for (let attempt = 1; attempt <= MAX_ATTEMPTS; attempt++) { try { // just keep overwriting attempts output @@ -90,7 +124,7 @@ async function runAction() { // error: nonzero throw error; } else { - warning(`Attempt ${attempt} failed. Reason:`, error.message); + warning(`Attempt ${attempt} failed. Reason: ${error.message}`); } } }