common.sh: Make die in a subshell actually stop everything immediately - #4211
common.sh: Make die in a subshell actually stop everything immediately#4211chewi wants to merge 2 commits into
die in a subshell actually stop everything immediately#4211Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates common.sh’s fatal error handling so that when die is triggered inside a subshell (e.g., $(...)), it terminates the overall script run immediately rather than allowing the parent shell to continue executing with invalid state.
Changes:
- Extend
die_notraceto detect subshell execution and send a terminating signal to the current process group. - Add a
SIGTERMtrap so the top-level script exits with status code1(instead of143) when terminated viaSIGTERM.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
a90f825 to
a020d1f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
common.sh:186
- Using SIGTERM as the “abort whole run” signal is fragile because many scripts legitimately install/override TERM traps for cleanup after sourcing common.sh (e.g. core_sign_update:42
trap cleanup INT TERM EXIT). In that case,kill -s TERM 0may not actually stop the top-level script (the TERM trap runs and execution resumes), reintroducing the repeateddiebehavior this PR is trying to eliminate. Consider switching this internal mechanism to a dedicated signal (e.g. USR1) that is unlikely to be trapped elsewhere, and trap that signal in the top-level shell to exit 1.
# `exit` only leaves the current shell. When die is reached inside a $(...)
# command substitution or other subshell, the parent keeps running with bad
# data and usually hits another die, so the same failure gets reported several
# times. BASHPID (unlike $$, which stays the top-level PID even in subshells)
# lets us detect that case and signal the main script so the whole run stops
core_sign_update was the only case we could find, and that case isn't important here. I think USR1 could cause strange behaviour from other processes in the process group. |
a020d1f to
c6e3881
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
common.sh:170
- The comment says "Passing 0 to kill terminates the whole process group", but
kill ... 0only sends the signal to every process in the current process group; processes can still ignore/trap SIGTERM. Tweaking the wording avoids implying a guarantee that isn’t true (and is relevant here since SIGTERM is commonly trapped).
# lets us detect that case and signal the main script so the whole run stops
# immediately. Passing 0 to kill terminates the whole process group.
Currently, it is more like a slow motion car crash where parent processes continue to execute, usually with bad data that triggers yet more `die` calls. Signed-off-by: James Le Cuirot <jlecuirot@microsoft.com>
…rocess/audit The package installed it with a 640 mode, which breaks our tmpfiles post-processing. Signed-off-by: Krzesimir Nowak <knowak@microsoft.com>
eabab00 to
5574248
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (4)
common.sh:186
- The new subshell-fast-fail relies on SIGTERM to stop the top-level shell, but the current logic explicitly avoids touching an existing TERM trap. If a caller already has a TERM trap that does not exit (e.g.
core_sign_updatetraps TERM for cleanup only),kill -s TERM 0won’t actually stop the script, so the “stop everything immediately” goal is not guaranteed. Consider chaining any existing TERM handler with anexit 1(and update the comment that claims the process group is “terminated” bykill 0).
# Only arm this in the real top-level shell (BASHPID == $$), and never clobber
# an existing TERM trap. The latter keeps re-sourcing common.sh idempotent and
# defers to any caller that installed its own handler first. Note that `trap -p`
# reports the parent's traps even from a command substitution.
[[ ${BASHPID:-$$} == $$ && -z $(trap -p TERM) ]] && trap 'exit 1' TERM
sdk_container/src/third_party/coreos-overlay/coreos/config/env/sys-process/audit:4
- This used to append the mask to both INSTALL_MASK and PKG_INSTALL_MASK. Dropping PKG_INSTALL_MASK means the masked paths can still be installed when emerging from a binary package (the profile explicitly uses PKG_INSTALL_MASK to block files from binpkgs). Unless the intent is to only affect source builds, keep PKG_INSTALL_MASK in sync with INSTALL_MASK here.
INSTALL_MASK+=" /etc/audit/audit.rules* /usr/libexec "
sdk_container/src/third_party/coreos-overlay/coreos/config/env/sys-process/audit:7
- Typo in comment: “unnecessarilly” → “unnecessarily”.
# Upstream installs its tmpfiles config file with unnecessarilly
sdk_container/src/third_party/coreos-overlay/coreos/config/env/sys-fs/lvm2:2
- Typo in comment: “unnecessarilly” → “unnecessarily”.
# Upstream installs its tmpfiles config file with unnecessarilly
5574248 to
cadc674
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
sdk_container/src/third_party/coreos-overlay/coreos/config/env/sys-process/audit:5
PKG_INSTALL_MASKis no longer updated for audit, so the masked files can still be installed into the rootfs when installing from binary packages. Other env overrides update bothINSTALL_MASKandPKG_INSTALL_MASKfor the same paths, andprofiles/coreos/base/make.defaultsdescribesPKG_INSTALL_MASKas controlling binpkg installs.
INSTALL_MASK+=" /etc/audit/audit.rules* /usr/libexec "
sdk_container/src/third_party/coreos-overlay/coreos/config/env/sys-process/audit:7
- Spelling: “unnecessarilly” -> “unnecessarily”.
# Upstream installs its tmpfiles config file with unnecessarilly
Currently, it is more like a slow motion car crash where parent processes continue to execute, usually with bad data that triggers yet more
diecalls.How to use
Run some image build command that fails.
Testing done
Before:
After:
I've also run an SDK build under Jenkins...
changelog/directory (user-facing change, bug fix, security fix, update) -- N/A/bootand/usrsize, packages, list files for any missing binaries, kernel modules, config files, kernel modules, etc.