From a91fb1731094f9408a5543ea09040dfd69dbe2ad Mon Sep 17 00:00:00 2001 From: Alexander Wang <3120367+alixander@users.noreply.github.com> Date: Mon, 7 Sep 2026 08:42:02 -0700 Subject: [PATCH] Fix background failure reporting and D2 formatting --- README.md | 9 ++++ bin/fmt.sh | 14 ++--- ci/ci.sh | 4 +- ci/test.sh | 15 ++++-- lib.sh | 73 +++++++++++++++++-------- lib/ci.sh | 3 +- lib/ci_test.sh | 64 ++++++++++++++++++++++ lib/flag_test.sh | 20 +++---- lib/fmt_test.sh | 39 ++++++++++++++ lib/job.sh | 67 +++++++++++++++++------ lib/job_test.sh | 137 +++++++++++++++++++++++++++++++++++++++++++++++ lib/log_test.sh | 12 +++-- lib/make_test.sh | 4 +- lib/notify.sh | 3 -- 14 files changed, 393 insertions(+), 71 deletions(-) create mode 100755 lib/ci_test.sh create mode 100755 lib/fmt_test.sh create mode 100755 lib/job_test.sh diff --git a/README.md b/README.md index ad5e4df..fab2387 100644 --- a/README.md +++ b/README.md @@ -21,3 +21,12 @@ Currently used by: And in our internal monorepo. For robust example usage of the flag parser see [./examples/date.sh](./examples/date.sh). + +Use `runjob name command` for foreground work. For parallel work, replace +`runjob name command &` with `runjob_bg name command`, then call `waitjobs`. +The launcher records each process ID immediately so completed failures cannot be +lost from the shell's job list. `waitjobs` rejects unregistered background jobs +that it can still detect; migrate callers before updating this shared library. + +`ci_waitjobs` waits for jobs and checks generated files. Commit-message policy is +opt-in through `bin/nofixups.sh`; cleanup and notifications do not enforce it. diff --git a/bin/fmt.sh b/bin/fmt.sh index 6732b7b..036188d 100755 --- a/bin/fmt.sh +++ b/bin/fmt.sh @@ -55,7 +55,7 @@ d2fmt() { curl -fsSL https://d2lang.com/install.sh | sh -s -- ) fi - sh_c XARGS_N=1 hide xargsd --null "'\.\(d2\)$'" d2 fmt + sh_c XARGS_N=1 hide xargsd "'\.\(d2\)$'" d2 fmt } main() { @@ -66,22 +66,22 @@ main() { # runjob trailing-whitespace trailing_whitespace # fi if <"$CHANGED_FILES" grep -q '\.\(md\)$'; then - runjob mdtocsubst mdtocsubst_xargsd & + runjob_bg mdtocsubst mdtocsubst_xargsd fi if search_up go.mod >/dev/null; then - runjob go.mod gomodtidy & + runjob_bg go.mod gomodtidy fi if <"$CHANGED_FILES" grep -q '\.\(go\)$'; then - runjob gofmt & + runjob_bg gofmt fi if search_up package.json >/dev/null; then - runjob package.json pkgjson & + runjob_bg package.json pkgjson fi if <"$CHANGED_FILES" grep -q '\.\(js\|jsx\|ts\|tsx\|scss\|css\|html\)$'; then - runjob prettier & + runjob_bg prettier fi if <"$CHANGED_FILES" grep -qm1 '\.\(d2\)$'; then - runjob d2fmt & + runjob_bg d2fmt fi waitjobs } diff --git a/ci/ci.sh b/ci/ci.sh index e012444..f5d7469 100755 --- a/ci/ci.sh +++ b/ci/ci.sh @@ -13,8 +13,8 @@ fmtgen() { job_parseflags "$@" ensure_git_base -fmtgen & +_job_bg fmtgen if is_changed lib; then - runjob test ./ci/test.sh & + runjob_bg test ./ci/test.sh fi ci_waitjobs diff --git a/ci/test.sh b/ci/test.sh index 7cb9bfe..db88833 100755 --- a/ci/test.sh +++ b/ci/test.sh @@ -9,16 +9,25 @@ cd - >/dev/null job_parseflags "$@" if is_changed ./lib/test.sh ./lib/rand.sh ./lib/log.sh \ ./lib/temp.sh; then - runjob log ./lib/log_test.sh & + runjob_bg log ./lib/log_test.sh fi if is_changed ./lib/test.sh ./lib/rand.sh ./lib/log.sh \ ./lib/flag.sh ./lib/temp.sh; then - runjob flag ./lib/flag_test.sh & + runjob_bg flag ./lib/flag_test.sh fi if is_changed \ ./lib/test.sh ./lib/rand.sh ./lib/log.sh ./lib/git.sh \ ./lib/flag.sh ./lib/ci.sh ./lib/job.sh ./lib/notify.sh \ ./lib/temp.sh ./lib/release.sh; then - runjob make ./lib/make_test.sh & + runjob_bg make ./lib/make_test.sh +fi +if is_changed ./lib/job.sh ./lib/job_test.sh ./lib/ci.sh ./lib/ci_test.sh \ + ./lib/notify.sh ./lib/log.sh ./lib/test.sh ./lib/temp.sh ./ci/test.sh; then + runjob_bg job ./lib/job_test.sh + runjob_bg ci ./lib/ci_test.sh +fi +if is_changed ./bin/fmt.sh ./lib/fmt_test.sh ./lib/git.sh ./lib/job.sh \ + ./lib/log.sh ./lib/test.sh ./lib/temp.sh ./ci/test.sh; then + runjob_bg formatter ./lib/fmt_test.sh fi waitjobs diff --git a/lib.sh b/lib.sh index da5d74f..ffa8b6e 100644 --- a/lib.sh +++ b/lib.sh @@ -11,7 +11,7 @@ ci_go_lint() { ci_waitjobs() { if [ -z "${CI-}" ]; then waitjobs - return 0 + return "$?" fi capcode waitjobs @@ -24,7 +24,6 @@ ci_waitjobs() { notify return "$code" fi - capcode nofixups notify return "$code" } @@ -435,6 +434,8 @@ LIB_JOB=1 # and propogating of signals. Not sure how to debug even without something like gdb and # going through the source code of the shell too. runjob() {( + # A child must never wait on its parent's other jobs. + JOB_PIDS= jobname=$1 export JOBNAME=${JOBNAME+$JOBNAME/}$jobname shift @@ -462,16 +463,29 @@ runjob() {( # We add the prefix to all lines and remove any warning lines about recursive make. # We cannot silence these with -s which is unfortunate. (sed -e "s#^#$(echop "$jobname"): #" -e "/make\[.\]: warning: -j/d" "$stdout" || true) & + RUNJOB_READERS="$!" # This intentionally does not output to our stderr, it becomes our stdout. (sed -e "s#^#$(echop "$jobname"): #" -e "/make\[.\]: warning: -j/d" "$stderr" || true) & + RUNJOB_READERS="$RUNJOB_READERS $!" start="$(awk 'BEGIN{srand(); print srand()}')" trap runjob_exittrap EXIT # For some reason without wrapping this in a subshell, the waitjobs in subjob # case_notequal_sign of ./lib/flags_test.sh freezes. - ( eval "$*" >"$stdout" 2>"$stderr" ) + ( JOB_PIDS=; eval "$*" >"$stdout" 2>"$stderr" ) )} +# Use this instead of `runjob ... &` so the shell retains the child's exit status. +runjob_bg() { + _job_bg runjob "$@" +} + +# Register an unprefixed helper without adding a job-filter level. +_job_bg() { + JOB_PIDS= "$@" & + JOB_PIDS="${JOB_PIDS-} $!" +} + _runjob_filter() { if [ -z "${JOBFILTER-}" ]; then return 0 @@ -511,42 +525,62 @@ runjob_filter() { runjob_exittrap() { code="$?" + trap - EXIT end="$(awk 'BEGIN{srand(); print srand()}')" dur="$((end - start))" - waitjobs_sigtrap + # Preserve the existing output-reader shutdown behavior. Their status must + # never replace the command's exit status. + for reader_pid in $RUNJOB_READERS; do + kill "$reader_pid" 2>/dev/null || true + done + for reader_pid in $RUNJOB_READERS; do + wait "$reader_pid" 2>/dev/null || true + done if [ "$code" -eq 0 ]; then echop "$jobname\$" "$(setaf 2 success)" "($(echo_dur "$dur"))" else echop "$jobname\$" "$(setaf 1 failure)" "($(echo_dur "$dur"))" fi + exit "$code" } waitjobs() { + waitjobs_status=0 + # Fail clearly for legacy callers rather than silently dropping their failures. + # `jobs` must run in this shell: dash has an empty job table in substitutions. wait_tmpdir="$(mktempd)" - jobs -l > "$wait_tmpdir/jobsl" - trap waitjobs_sigtrap INT TERM - - jobs -p > "$wait_tmpdir/jobsp" + jobs -p >"$wait_tmpdir/jobsp" for pid in $(cat "$wait_tmpdir/jobsp"); do + case " ${JOB_PIDS-} " in + *" $pid "*) ;; + *) + echoerr "unregistered background job $pid: use runjob_bg instead of runjob ... &" + JOB_PIDS="${JOB_PIDS-} $pid" + waitjobs_status=1 + ;; + esac + done + trap waitjobs_sigtrap INT TERM + set -- ${JOB_PIDS-} + for pid do if ! wait "$pid"; then - caterr < /dev/null || true done - waitjobs } job_parseflags() { @@ -1060,9 +1094,6 @@ EOF return 1 fi - if [ "$code" -eq 0 ]; then - capcode nofixups - fi if [ "$code" -eq 0 ]; then status=success emoji=🟢 diff --git a/lib/ci.sh b/lib/ci.sh index 7b74f9a..af963f5 100644 --- a/lib/ci.sh +++ b/lib/ci.sh @@ -15,7 +15,7 @@ ci_go_lint() { ci_waitjobs() { if [ -z "${CI-}" ]; then waitjobs - return 0 + return "$?" fi capcode waitjobs @@ -28,7 +28,6 @@ ci_waitjobs() { notify return "$code" fi - capcode nofixups notify return "$code" } diff --git a/lib/ci_test.sh b/lib/ci_test.sh new file mode 100755 index 0000000..56e090c --- /dev/null +++ b/lib/ci_test.sh @@ -0,0 +1,64 @@ +#!/bin/sh +set -eu +cd -- "$(dirname "$0")" +. ./test.sh +. ./ci.sh +cd - >/dev/null + +case_commit_wording() { + CI=1 + CI_MAKE_ROOT=0 + GIT_BASE= + cd "$(mktempd)" + git_pure init -q + printf 'tracked\n' >file + git_pure add file + git_pure -c commit.gpgsign=false commit -qm 'fixup! permitted by caller' + ci_waitjobs + # The opt-in standalone check still works. + if nofixups; then + echoerr "standalone nofixups check should reject the fixture" + return 1 + fi + printf 'dirty\n' >>file + if ci_waitjobs; then + echoerr "CI cleanup accepted a dirty tracked file" + return 1 + fi +} + +case_local_failure() { + CI= + runjob_bg failing false + if ci_waitjobs; then + echoerr "local cleanup accepted a failed background job" + return 1 + fi +} + +case_notification_status() { + CI=1 + CI_MAKE_ROOT=1 + GITHUB_REF_PROTECTED=true + GITHUB_RUN_ID=1 + GITHUB_JOB=test + GITHUB_REPOSITORY=test/repo + GITHUB_WORKFLOW=test + GITHUB_TOKEN=test + DISCORD_WEBHOOK_URL=https://example.invalid/webhook + nofixups() { echoerr "notification invoked nofixups"; return 1; } + curl() { + case "$*" in + *' -X POST '*) return 0 ;; + *) printf '%s\n' '{"jobs":[{"name":"test","html_url":"https://example.invalid/job"}]}' ;; + esac + } + code=0 + notify + assert code 0 +} + +job_parseflags "$@" +runjob case_commit_wording +runjob case_local_failure +runjob case_notification_status diff --git a/lib/flag_test.sh b/lib/flag_test.sh index 6e2f6d4..f738471 100755 --- a/lib/flag_test.sh +++ b/lib/flag_test.sh @@ -153,9 +153,9 @@ case_notequal_sign() { assert_term '' "$@" } - runjob case_with_args & - runjob case_without_args & - runjob case_term & + runjob_bg case_with_args + runjob_bg case_without_args + runjob_bg case_term waitjobs } @@ -253,11 +253,11 @@ case_flag_fmt() { } job_parseflags "$@" -runjob case_term & -runjob case_equal_sign & -runjob case_notequal_sign & -runjob case_reqarg & -runjob case_nonemptyarg & -runjob case_noarg & -runjob case_flag_fmt & +runjob_bg case_term +runjob_bg case_equal_sign +runjob_bg case_notequal_sign +runjob_bg case_reqarg +runjob_bg case_nonemptyarg +runjob_bg case_noarg +runjob_bg case_flag_fmt waitjobs diff --git a/lib/fmt_test.sh b/lib/fmt_test.sh new file mode 100755 index 0000000..888f687 --- /dev/null +++ b/lib/fmt_test.sh @@ -0,0 +1,39 @@ +#!/bin/sh +set -eu +cd -- "$(dirname "$0")" +. ./test.sh +formatter=$(cd ../bin && pwd)/fmt.sh +cd - >/dev/null + +case_d2_formatter() { + directory=$(mktempd) + mkdir "$directory/bin" "$directory/repo" + cat >"$directory/bin/d2" <<'EOF' +#!/bin/sh +printf '%s\n' "$*" >>"$FORMATTER_CALLS" +exit "${FORMATTER_STATUS:-0}" +EOF + chmod +x "$directory/bin/d2" + PATH="$directory/bin:$PATH" + export PATH FORMATTER_CALLS="$directory/calls" + # Match git's physical root path (macOS temporary directories use a symlink). + cd -P "$directory/repo" + git_pure init -q + printf 'a -> b\n' >fixture.d2 + printf 'unchanged\n' >unchanged.d2 + git_pure add . + git_pure -c commit.gpgsign=false commit -qm fixture + export CI=1 GIT_BASE=HEAD OS=linux + printf 'a->b\n' >fixture.d2 + + "$formatter" + assert calls "$(cat "$FORMATTER_CALLS")" 'fmt fixture.d2' + export FORMATTER_STATUS=23 + if "$formatter"; then + echoerr "formatter failure was ignored" + return 1 + fi +} + +job_parseflags "$@" +runjob case_d2_formatter diff --git a/lib/job.sh b/lib/job.sh index 337cc49..a78ad1e 100644 --- a/lib/job.sh +++ b/lib/job.sh @@ -16,6 +16,8 @@ LIB_JOB=1 # and propogating of signals. Not sure how to debug even without something like gdb and # going through the source code of the shell too. runjob() {( + # A child must never wait on its parent's other jobs. + JOB_PIDS= jobname=$1 export JOBNAME=${JOBNAME+$JOBNAME/}$jobname shift @@ -43,16 +45,29 @@ runjob() {( # We add the prefix to all lines and remove any warning lines about recursive make. # We cannot silence these with -s which is unfortunate. (sed -e "s#^#$(echop "$jobname"): #" -e "/make\[.\]: warning: -j/d" "$stdout" || true) & + RUNJOB_READERS="$!" # This intentionally does not output to our stderr, it becomes our stdout. (sed -e "s#^#$(echop "$jobname"): #" -e "/make\[.\]: warning: -j/d" "$stderr" || true) & + RUNJOB_READERS="$RUNJOB_READERS $!" start="$(awk 'BEGIN{srand(); print srand()}')" trap runjob_exittrap EXIT # For some reason without wrapping this in a subshell, the waitjobs in subjob # case_notequal_sign of ./lib/flags_test.sh freezes. - ( eval "$*" >"$stdout" 2>"$stderr" ) + ( JOB_PIDS=; eval "$*" >"$stdout" 2>"$stderr" ) )} +# Use this instead of `runjob ... &` so the shell retains the child's exit status. +runjob_bg() { + _job_bg runjob "$@" +} + +# Register an unprefixed helper without adding a job-filter level. +_job_bg() { + JOB_PIDS= "$@" & + JOB_PIDS="${JOB_PIDS-} $!" +} + _runjob_filter() { if [ -z "${JOBFILTER-}" ]; then return 0 @@ -92,42 +107,62 @@ runjob_filter() { runjob_exittrap() { code="$?" + trap - EXIT end="$(awk 'BEGIN{srand(); print srand()}')" dur="$((end - start))" - waitjobs_sigtrap + # Preserve the existing output-reader shutdown behavior. Their status must + # never replace the command's exit status. + for reader_pid in $RUNJOB_READERS; do + kill "$reader_pid" 2>/dev/null || true + done + for reader_pid in $RUNJOB_READERS; do + wait "$reader_pid" 2>/dev/null || true + done if [ "$code" -eq 0 ]; then echop "$jobname\$" "$(setaf 2 success)" "($(echo_dur "$dur"))" else echop "$jobname\$" "$(setaf 1 failure)" "($(echo_dur "$dur"))" fi + exit "$code" } waitjobs() { + waitjobs_status=0 + # Fail clearly for legacy callers rather than silently dropping their failures. + # `jobs` must run in this shell: dash has an empty job table in substitutions. wait_tmpdir="$(mktempd)" - jobs -l > "$wait_tmpdir/jobsl" - trap waitjobs_sigtrap INT TERM - - jobs -p > "$wait_tmpdir/jobsp" + jobs -p >"$wait_tmpdir/jobsp" for pid in $(cat "$wait_tmpdir/jobsp"); do + case " ${JOB_PIDS-} " in + *" $pid "*) ;; + *) + echoerr "unregistered background job $pid: use runjob_bg instead of runjob ... &" + JOB_PIDS="${JOB_PIDS-} $pid" + waitjobs_status=1 + ;; + esac + done + trap waitjobs_sigtrap INT TERM + set -- ${JOB_PIDS-} + for pid do if ! wait "$pid"; then - caterr < /dev/null || true done - waitjobs } job_parseflags() { diff --git a/lib/job_test.sh b/lib/job_test.sh new file mode 100755 index 0000000..a9f9ec2 --- /dev/null +++ b/lib/job_test.sh @@ -0,0 +1,137 @@ +#!/bin/sh +set -eu +cd -- "$(dirname "$0")" +. ./test.sh +cd - >/dev/null + +# Synchronize on completion without consuming the status that waitjobs must keep. +await_completed() { + test_attempt=0 + while kill -0 "$1" 2>/dev/null; do + test_attempt=$((test_attempt + 1)) + if [ "$test_attempt" -ge 500 ]; then + echoerr "job $1 did not finish within five seconds" + return 1 + fi + sleep 0.01 + done +} + +await_file() { + test_attempt=0 + while [ ! -f "$1" ]; do + test_attempt=$((test_attempt + 1)) + if [ "$test_attempt" -ge 500 ]; then + echoerr "job did not produce $1 within five seconds" + return 1 + fi + sleep 0.01 + done +} + +expect_failed_batch() { + if waitjobs; then + echoerr "waitjobs accepted a failed job" + return 1 + fi +} + +case_completed_success() { + runjob_bg success true + await_completed "$!" + waitjobs +} + +case_completed_failure() { + runjob_bg failure false + await_completed "$!" + expect_failed_batch +} + +case_mixed_after_jobs() { + runjob_bg success true + success_pid=$! + runjob_bg failure false + failure_pid=$! + await_completed "$success_pid" + await_completed "$failure_pid" + # Listing completed jobs can discard the shell's jobs-table entries. + jobs -l >/dev/null + expect_failed_batch +} + +case_batch_reset() { + runjob_bg failure false + expect_failed_batch + waitjobs + runjob_bg success true + waitjobs + waitjobs +} + +case_nested() { + nested_dir=$(mktempd) + unrelated() { + await_file "$nested_dir/release" + touch "$nested_dir/unrelated-done" + } + nested_success() { + runjob_bg leaf true + waitjobs + touch "$nested_dir/nested-success" + } + nested_failure() { + runjob_bg leaf false + waitjobs + } + + runjob_bg unrelated unrelated + runjob_bg nested-success nested_success + success_pid=$! + runjob_bg nested-failure nested_failure + failure_pid=$! + await_file "$nested_dir/nested-success" + await_completed "$success_pid" + await_completed "$failure_pid" + test ! -f "$nested_dir/unrelated-done" + touch "$nested_dir/release" + expect_failed_batch + test -f "$nested_dir/unrelated-done" +} + +case_unregistered() { + legacy_output=$(mktempd)/error + # Reject visible legacy launches even when their command succeeds. + runjob legacy 'sleep 0.1; true' >/dev/null 2>&1 & + if waitjobs 2>"$legacy_output"; then + echoerr "waitjobs accepted an unregistered background job" + return 1 + fi + grep -q 'unregistered background job .*use runjob_bg' "$legacy_output" +} + +case_terminated() { + term_dir=$(mktempd) + stoppable() { + touch "$term_dir/ready" + # Remain bounded even if the wrapper leaves its command running on TERM. + sleep 0.2 + } + runjob_bg stopped stoppable + stopped_pid=$! + await_file "$term_dir/ready" + kill -TERM "$stopped_pid" + expect_failed_batch + await_completed "$stopped_pid" + # The existing wrapper can leave its command alive briefly after cancellation. + sleep 0.3 +} + +job_parseflags "$@" +runjob case_completed_success +runjob case_completed_failure +runjob case_mixed_after_jobs +runjob case_batch_reset +runjob case_nested +runjob case_unregistered +runjob case_terminated diff --git a/lib/log_test.sh b/lib/log_test.sh index 2bc6e7f..8e9b2b5 100755 --- a/lib/log_test.sh +++ b/lib/log_test.sh @@ -48,17 +48,19 @@ two" 2>&1) } case4() { + TERM=xterm + export TERM got=$(COLOR=1 FGCOLOR=1 bigheader "one two" 2>&1) - assert got "$(COLOR=1 tput setaf 1)/****************************************************************$(tput sgr0) + assert got "$(COLOR=1 tput setaf 1)/****************************************************************$(COLOR=1 tput sgr0) $(COLOR=1 tput setaf 1) * one$(COLOR=1 tput sgr0) $(COLOR=1 tput setaf 1) * two$(COLOR=1 tput sgr0) $(COLOR=1 tput setaf 1) ****************************************************************/$(COLOR=1 tput sgr0)" } job_parseflags "$@" -runjob case1 & -runjob case2 & -runjob case3 & -runjob case4 & +runjob_bg case1 +runjob_bg case2 +runjob_bg case3 +runjob_bg case4 waitjobs diff --git a/lib/make_test.sh b/lib/make_test.sh index 4d1ec1b..08d3237 100755 --- a/lib/make_test.sh +++ b/lib/make_test.sh @@ -64,6 +64,6 @@ EOF } job_parseflags "$@" -runjob case1 & -runjob case2 & +runjob_bg case1 +runjob_bg case2 waitjobs diff --git a/lib/notify.sh b/lib/notify.sh index 9882075..1f93b89 100644 --- a/lib/notify.sh +++ b/lib/notify.sh @@ -33,9 +33,6 @@ EOF return 1 fi - if [ "$code" -eq 0 ]; then - capcode nofixups - fi if [ "$code" -eq 0 ]; then status=success emoji=🟢