From def4917447b1d83ea4d7135f9e9241f1b0d30bf6 Mon Sep 17 00:00:00 2001 From: whyour Date: Tue, 29 Sep 2026 00:52:33 +0800 Subject: [PATCH] fix: finalize exited shell tasks and preserve command environments --- docs/development/task-startup.md | 9 +- shell/otask.sh | 4 +- shell/task.sh | 8 +- test/back/task-shell-lifecycle.test.cjs | 119 ++++++++++++++++++++++++ 4 files changed, 137 insertions(+), 3 deletions(-) create mode 100644 test/back/task-shell-lifecycle.test.cjs diff --git a/docs/development/task-startup.md b/docs/development/task-startup.md index 2020c57e..fffc9c74 100644 --- a/docs/development/task-startup.md +++ b/docs/development/task-startup.md @@ -24,7 +24,7 @@ order remain available. This caches the dependency directory, not its contents. | `task script.py` / `.pyc` | Python preload imports generated environment, runs the shell/command before hooks, imports `task_before.py`, then applies account selection | `QLAPI.notify(...)` imports `__ql_notify__` on first call | | `task script.js` / `.mjs` / `.ts` | Node preload imports generated environment, runs shell/command before hooks, requires `task_before.js`, then applies account selection | `QLAPI.notify(...)` requires `__ql_notify__.js` on first call | | `task script.sh` | Shell sources the generated environment and runs shell before hooks | No automatic Python/Node notification module | -| Explicit interpreter or other command, e.g. `task python3 script.py` | Existing generic-command path uses shell environment/before hooks; it does not automatically install the Python/Node preload | No automatic notification module unless configured by the caller | +| Explicit interpreter or other command, e.g. `task python3 script.py` | Retains generated environment and shell before-hook exports; it does not automatically install the Python/Node preload | No automatic notification module unless configured by the caller | TypeScript uses `ts-node-transpile-only`; MJS uses the same Node preload plus the ESM loader. There are no separate built-in Ruby, Go, Java or PHP preload modules. @@ -38,6 +38,13 @@ child shell/Node. Node also still loads its gRPC client eagerly. These are remai startup costs, not work performed by the cron library, and are not removed by this change. +Shell scripts run in a child shell that inherits the wrapper's environment and +before-hook functions. This lets the wrapper run its after hooks and report the +exit status even when a script calls `exit`, replaces itself with `exec`, or fails +under `set -e`. The script's own EXIT trap still runs in the child. Changes made +inside the script (variables, working directory, functions and shell options) +do not propagate back to the wrapper or its after hooks. + ## Lazy notifications Scripts keep calling `QLAPI.notify` with the same arguments. When no notification diff --git a/shell/otask.sh b/shell/otask.sh index 5dcb579d..c11041f3 100755 --- a/shell/otask.sh +++ b/shell/otask.sh @@ -84,7 +84,9 @@ env_str_to_array() { } clear_non_sh_env() { - if [[ $file_param != *.sh ]]; then + # Only automatic Node/Python preloads reconstruct the generated environment. + # Explicit interpreters and other commands must keep the shell-loaded values. + if [[ $isJsOrPythonFile == 'true' ]]; then clear_env fi } diff --git a/shell/task.sh b/shell/task.sh index a98bd827..a4fe900c 100755 --- a/shell/task.sh +++ b/shell/task.sh @@ -18,6 +18,12 @@ single_hanle() { exit 1 } +run_shell_script() { + # Preserve inherited variables/functions, but contain exit, exec and shell + # options so the task wrapper can run after hooks and report completion. + ( . "$@" ) +} + define_program() { local file_param=$1 if [[ $file_param == *.js ]] || [[ $file_param == *.mjs ]]; then @@ -25,7 +31,7 @@ define_program() { elif [[ $file_param == *.py ]] || [[ $file_param == *.pyc ]]; then which_program="python3" elif [[ $file_param == *.sh ]]; then - which_program="." + which_program="run_shell_script" elif [[ $file_param == *.ts ]]; then which_program="ts-node-transpile-only" else diff --git a/test/back/task-shell-lifecycle.test.cjs b/test/back/task-shell-lifecycle.test.cjs new file mode 100644 index 00000000..9aefe072 --- /dev/null +++ b/test/back/task-shell-lifecycle.test.cjs @@ -0,0 +1,119 @@ +const test = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); +const { spawnSync } = require('node:child_process'); + +function extract(file, name) { + const text = fs.readFileSync(file, 'utf8'); + const start = text.indexOf(name + '() {'); + assert.ok(start >= 0, name); + return text.slice(start, text.indexOf('\n}', start) + 2); +} +const helpers = [ + ...['handle_task_start', 'handle_task_end', 'run_task_before', 'run_task_after', 'get_env_array', 'clear_env'].map(n => extract('shell/share.sh', n)), + ...['run_shell_script', 'define_program', 'format_params'].map(n => extract('shell/task.sh', n)), +].join('\n'); +const taskSource = path.resolve('shell/otask.sh'); + +function fixture(t) { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'ql-shell-lifecycle-')); + t.after(() => fs.rmSync(root, { recursive: true, force: true })); + const write = (name, body) => fs.writeFileSync(path.join(root, name), body); + write('env.sh', 'export QA_PANEL="alpha&beta&gamma"\n'); + write('before.sh', 'export QA_BEFORE=ready\nqa_function() { printf "HOOK_FUNCTION\\n"; }\n'); + write('after.sh', 'printf "AFTER:%s:%s\\n" "$QA_PANEL" "$QA_BEFORE" >> "$QA_ROOT/events"\n'); + const run = (args) => { + const r = spawnSync('/bin/bash', ['-c', helpers + ` + dir_scripts=$QA_ROOT; dir_shell=$QA_ROOT; dir_dep=$QA_ROOT + file_env=$QA_ROOT/env.sh; file_task_before=$QA_ROOT/before.sh; file_task_after=$QA_ROOT/after.sh + ID=42; log_path=fixture.log; is_macos=0 + dir_log=$QA_ROOT; log_dir=.; mtime_format='%Y-%m-%d %H:%M:%S' + format_log_time() { printf fixture; } + time_format='%Y-%m-%d %H:%M:%S'; begin_time=start; begin_timestamp=$(date +%s) + real_time=true; no_delay=true; NODE_OPTIONS=''; PYTHONPATH='' + pnpm() { printf '/fixture/global\\n'; } + t() { :; } + update_cron() { printf 'STATUS:%s:%s\\n' "$2" "$7" >> "$QA_ROOT/events"; } + record_cron_stat() { printf 'STAT:%s\\n' "$2" >> "$QA_ROOT/events"; } + format_params "$@"; define_program "${'${task_shell_params[@]}'}" + . "$QA_TASK_SOURCE" + printf 'WRAPPER_FINISHED\\n' >> "$QA_ROOT/events" + `, 'fixture', ...args], { cwd: root, env: { ...process.env, QA_ROOT: root, QA_TASK_SOURCE: taskSource }, encoding: 'utf8', timeout: 10000 }); + assert.equal(r.status, 0, r.stdout + r.stderr); + return { stdout: r.stdout, events: fs.readFileSync(path.join(root, 'events'), 'utf8').trim().split('\n') }; + }; + return { write, run }; +} + +for (const [label, body, code] of [ + ['normal return', ':', 0], + ['exit zero', 'exit 0', 0], + ['exit failure', 'exit 7', 7], + ['exec replacement', "exec bash -c 'exit 9'", 9], + ['errexit failure', 'set -e\nfalse\necho SHOULD_NOT_RUN', 1], + ['script EXIT trap', `trap 'echo SCRIPT_EXIT_TRAP' EXIT\nexit 4`, 4], +]) { + test(`sourced shell ${label} still runs after hook and reports exactly one completion`, t => { + const f = fixture(t); + f.write('probe.sh', `[[ "$QA_PANEL" == 'alpha&beta&gamma' && "$QA_BEFORE" == ready ]] || exit 99\nqa_function\n${body}\n`); + const r = f.run(['probe.sh']); + assert.match(r.stdout, /HOOK_FUNCTION/); + assert.doesNotMatch(r.stdout, /SHOULD_NOT_RUN/); + if (label === 'script EXIT trap') assert.match(r.stdout, /SCRIPT_EXIT_TRAP/); + assert.deepEqual(r.events, ['STATUS:0:', 'AFTER:alpha&beta&gamma:ready', `STATUS:1:${code}`, `STAT:${code}`, 'WRAPPER_FINISHED']); + }); +} + +test('shell arguments and before-hook functions survive isolation while script state stays in the child', t => { + const f = fixture(t); + f.write('probe.sh', '[[ "$1" == "arg with spaces" ]] || exit 99\nqa_function\nexport QA_BEFORE=changed\ncd /\nexit 5\n'); + const r = f.run(['probe.sh', '--', 'arg with spaces']); + assert.match(r.stdout, /HOOK_FUNCTION/); + assert.ok(r.events.includes('AFTER:alpha&beta&gamma:ready')); + assert.ok(r.events.includes('STATUS:1:5')); +}); + +for (const [label, args] of [ + ['explicit shell interpreter', ['bash', 'probe.sh', 'argument']], + ['shell script with positional args', ['probe.sh', 'argument']], +]) { + test(`${label} preserves generated environment and reports failure`, t => { + const f = fixture(t); + f.write('probe.sh', '[[ "$QA_PANEL" == "alpha&beta&gamma" && "$QA_BEFORE" == ready && "$1" == argument ]] || exit 99\nexit 6\n'); + const r = f.run(args); + assert.ok(r.events.includes('STATUS:1:6')); + assert.equal(r.events.filter(x => x.startsWith('STATUS:1:')).length, 1); + }); +} + +test('explicit Python interpreter receives panel and before-hook environment', t => { + const f = fixture(t); + f.write('probe.py', 'import os,sys\nassert os.environ["QA_PANEL"] == "alpha&beta&gamma"\nassert os.environ["QA_BEFORE"] == "ready"\nassert sys.argv[1] == "argument"\nsys.exit(8)\n'); + assert.ok(f.run(['python3', 'probe.py', 'argument']).events.includes('STATUS:1:8')); +}); + +test('explicit Node interpreter retains generated environment', t => { + const f = fixture(t); + f.write('probe.cjs', 'if (process.env.QA_PANEL !== "alpha&beta&gamma" || process.env.QA_BEFORE !== "ready") process.exit(99); process.exit(8);\n'); + assert.ok(f.run(['node', 'probe.cjs']).events.includes('STATUS:1:8')); +}); + +test('plain commands retain generated environment', t => { + const f = fixture(t); + assert.match(f.run(['printenv', 'QA_PANEL']).stdout, /alpha&beta&gamma/); +}); + +for (const mode of ['desi', 'conc']) { + test(`shell ${mode} keeps selected account values and completion reporting`, t => { + const f = fixture(t); + f.write('probe.sh', 'printf "ACCOUNT=%s\\n" "$QA_PANEL"\nexit 0\n'); + const r = f.run(['probe.sh', mode, 'QA_PANEL', '2-3']); + assert.match(r.stdout, mode === 'desi' ? /ACCOUNT=beta&gamma/ : /ACCOUNT=beta\nACCOUNT=gamma/); + assert.doesNotMatch(r.stdout, /ACCOUNT=alpha/); + assert.ok(r.events.includes(`AFTER:${mode === 'desi' ? 'beta&gamma' : 'gamma'}:ready`)); + assert.equal(r.events.filter(x => x === 'STATUS:1:0').length, 1); + assert.equal(r.events.filter(x => x.startsWith('AFTER:')).length, 1); + }); +}