Fix background failure reporting and D2 formatting

Author: alixander

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.

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
 }

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

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

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 <<EOF
-failed to wait on $pid:
-$(<"$wait_tmpdir/jobsl" grep "$pid")
-EOF
-      FAILURE=1
+      echoerr "background job $pid failed"
+      waitjobs_status=1
     fi
+    shift
+    JOB_PIDS="$*"
   done
-  if [ -n "${FAILURE-}" ]; then
-    return 1
-  fi
+  JOB_PIDS=
+  trap - INT TERM
+  return "$waitjobs_status"
 }
 
 waitjobs_sigtrap() {
-  for pid in $(jobs -p); do
+  waitjobs_status=1
+  for pid in ${JOB_PIDS-}; do
     kill "$pid" 2> /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=🟢

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"
 }

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

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

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