From 88441f6ec354d1534fd01653d98c030b11981612 Mon Sep 17 00:00:00 2001 From: Daniel Hilst <392820+dhilst@users.noreply.github.com> Date: Wed, 23 Sep 2026 10:39:36 -0300 Subject: [PATCH 1/3] fix(xcat-core): build locks the shared tree refuses to grant A build tree can live on an NFS re-export. The kernel refuses locks on one -- "Clients are not allowed to get file locks or delegations from a reexport server" -- so every flock() there answers errno 524, and a build that takes one dies before it starts. buildrpms.pl's per-target lock and BuildUtils.pm's take_build_lock, which builddebs.pl calls for the Ubuntu core build, are both atomic mkdir claims now. Each records its owner and names it when it refuses. A directory is not released by a filehandle closing, which is how both locks were freed before. buildrpms.pl releases from END, and again in abort_builds because that handler re-raises the signal with DEFAULT and END blocks do not run then -- a killed build would otherwise strand the lock for every later one. BuildUtils returns a small object whose DESTROY releases it, preserving the caller's "hold the returned value" contract. Both releases are guarded by owning pid: both scripts fork, and the flock they replace could not be released by a child. builddebs_lock.t closed the returned value to prove the lock is released, which is "Not a GLOB reference" against the new contract. It now lets the value go out of scope. What it asserts is unchanged: a second build of the same checkout is refused, and the next one succeeds once the first releases. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com> --- build-utils/lib/XCAT/BuildUtils.pm | 27 +++++++++++++++++++++------ buildrpms.pl | 30 ++++++++++++++++++++---------- xCAT-test/unit/builddebs_lock.t | 8 +++++--- 3 files changed, 46 insertions(+), 19 deletions(-) diff --git a/build-utils/lib/XCAT/BuildUtils.pm b/build-utils/lib/XCAT/BuildUtils.pm index 54df11487..3ca224908 100644 --- a/build-utils/lib/XCAT/BuildUtils.pm +++ b/build-utils/lib/XCAT/BuildUtils.pm @@ -463,12 +463,27 @@ sub lock_path_for { # Returns the open handle -- the lock is held for as long as the caller keeps it. sub take_build_lock { my ($path, $dir) = @_; - require Fcntl; - my $lockfile = lock_path_for($path, $dir); - open my $fh, '>', $lockfile or die "FATAL: cannot open $lockfile: $!\n"; - flock($fh, Fcntl::LOCK_EX() | Fcntl::LOCK_NB()) - or die "FATAL: another build of $path already holds $lockfile\n"; - return $fh; + require POSIX; + # A directory, not an flock. A build tree can live on an NFS re-export, where the kernel + # refuses locks outright: every attempt answers errno 524. mkdir(2) is arbitrated by the + # server and needs no lock daemon. + my $lockdir = lock_path_for($path, $dir) . '.d'; + unless (mkdir $lockdir) { + die "FATAL: cannot take $lockdir: $!\n" unless $! == POSIX::EEXIST(); + my $who = ''; if (open(my $h, '<', "$lockdir/owner")) { local $/; $who = <$h> // ''; close $h } + chomp $who; + die "FATAL: another build of $path already holds $lockdir" + . ($who ? " (held by [$who])" : "") . "\n"; + } + if (open(my $ow, '>', "$lockdir/owner")) { print {$ow} "pid=$$\n"; close $ow } + # The caller keeps the returned value; release is by pid so a fork cannot free the parent's. + my $owner = $$; + return XCAT::BuildUtils::_BuildLock->new($lockdir, $owner); +} + +{ package XCAT::BuildUtils::_BuildLock; + sub new { my ($c,$d,$p)=@_; return bless { dir=>$d, pid=>$p }, $c } + sub DESTROY { my $s=shift; return unless $$ == $s->{pid}; unlink "$s->{dir}/owner"; rmdir $s->{dir} } } # The rpm architecture a mock target builds for. A target carries the arch as its diff --git a/buildrpms.pl b/buildrpms.pl index e14bdd436..fe2768d0f 100755 --- a/buildrpms.pl +++ b/buildrpms.pl @@ -45,7 +45,7 @@ use FindBin qw($Bin); use lib "$Bin/build-utils/lib"; use XCAT::BuildUtils qw(git_revision source_date_epoch sh sh_or_die usage buildinfo_text write_script read_line targetarch_from_target); -use Fcntl qw(:flock); # per-target build lock (concurrency guard; see main()) +use POSIX (); # EEXIST, for the directory build lock (see main()) use Getopt::Long qw(GetOptions); use POSIX qw(strftime); use Parallel::ForkManager; @@ -870,9 +870,17 @@ sub merge_core_repos { # file and the cached chroot dirs left behind are normal mock state, not leaks.) my %MOCK_INFLIGHT; # ForkManager child pid => mock chroot (-r) name it is building my $ABORTING = 0; -my $BUILD_LOCK_FH; # per-target build flock; MUST stay file-scoped so the fd (and thus the - # lock) lives for the whole process. A lexical inside main()'s block would - # be DESTROYED at block exit -> lock released before any worker forks. +my $BUILD_LOCK_DIR; # per-target build lock, an atomic mkdir rather than an flock. The build + # tree can live on an NFS re-export, where the kernel refuses locks + # outright ("Clients are not allowed to get file locks or delegations from + # a reexport server"), so every flock there fails with errno 524. mkdir(2) + # is arbitrated by the server and needs no lock daemon. File-scoped so the + # release runs once, from END, for the whole process. +my $BUILD_LOCK_PID; # the pid that took it. ForkManager children inherit $BUILD_LOCK_DIR, and + # their END would release the PARENT's lock -- the flock this replaces + # could not be released by a child, and neither may this. +END { rmdir $BUILD_LOCK_DIR if defined $BUILD_LOCK_DIR and defined $BUILD_LOCK_PID and $$ == $BUILD_LOCK_PID } + my @CHILD_FAILURES; # idents (chroot names) of ForkManager children that exited non-zero # PIDs of running mock processes whose `-r ` matches one of @chroots. @@ -929,6 +937,9 @@ sub abort_builds { sweep_mock_mounts(@chroots); } warn "[buildrpms] abort cleanup done\n"; + # The re-raise below kills this process by signal, and END blocks do not run then. Release the + # build lock here or a killed build strands it for every later run. + rmdir $BUILD_LOCK_DIR if defined $BUILD_LOCK_DIR and defined $BUILD_LOCK_PID and $$ == $BUILD_LOCK_PID; $SIG{$sig} = 'DEFAULT'; kill $sig, $$; # re-raise for the correct exit status } @@ -958,9 +969,10 @@ sub main { my $key = join('-', $opts{targets}->@*) . ($opts{mock_uniqueext} ? "-$opts{mock_uniqueext}" : ""); $key =~ s/[^A-Za-z0-9._-]/-/g; - my $lock = "/var/lock/buildrpms.$key.lock"; - if (open($BUILD_LOCK_FH, '>', $lock)) { - unless (flock($BUILD_LOCK_FH, LOCK_EX | LOCK_NB)) { + my $lock = "/var/lock/buildrpms.$key.lock.d"; + { + unless (mkdir $lock) { + die "FATAL: cannot take the build lock $lock: $!\n" unless $! == POSIX::EEXIST(); die "FATAL: another buildrpms.pl is already building target '@{$opts{targets}}'" . ($opts{mock_uniqueext} ? " (uniqueext=$opts{mock_uniqueext})" : "") . ".\n" . " ($lock is held). Concurrent builds of the same target collide on the shared\n" @@ -968,9 +980,7 @@ sub main { . " cleanup unmounts this build's chroot). Serialize them, or pass a distinct\n" . " --mock-uniqueext per build.\n"; } - # $BUILD_LOCK_FH is file-scoped, so the fd stays open (lock held) until this process - # exits. Child forks inherit the fd but their exits never release it (the parent's - # still-open fd keeps the lock), which is exactly what we want. + $BUILD_LOCK_DIR = $lock; $BUILD_LOCK_PID = $$; } } diff --git a/xCAT-test/unit/builddebs_lock.t b/xCAT-test/unit/builddebs_lock.t index cba190133..f022fc08c 100644 --- a/xCAT-test/unit/builddebs_lock.t +++ b/xCAT-test/unit/builddebs_lock.t @@ -44,9 +44,11 @@ like( $@, qr/already holds/, 'and says which checkout is already building' ); my $stable = eval { take_build_lock('/opt/builds/stable/xcat-core', $lockdir) }; ok( $stable, 'a build of a DIFFERENT checkout runs concurrently' ); -# Releasing lets the next build in. -close $first; +# Releasing lets the next build in. The lock is a directory now, not an flock on a filehandle -- +# an NFS re-export refuses locks outright (errno 524) -- so it is freed when the returned object +# goes out of scope, not when a handle is closed. +undef $first; my $again = eval { take_build_lock($devel, $lockdir) }; -ok( $again, 'the lock is released when the handle is closed' ); +ok( $again, 'the lock is released when the returned value goes out of scope' ); done_testing(); From 7d52506a2d79bb02551cd430c4ecc5c3aebde19d Mon Sep 17 00:00:00 2001 From: Daniel Hilst <392820+dhilst@users.noreply.github.com> Date: Thu, 24 Sep 2026 00:39:10 -0300 Subject: [PATCH 2/3] fix(xcat-core): a cancelled Debian build keeps the checkout lock for ever The build lock is released in DESTROY, and perl does not run DESTROY when a signal ends the process. A build stopped with SIGTERM or SIGINT therefore left its lock directory behind, and the next build of that checkout died on FATAL: another build of already holds (held by [pid=NNNN]) naming a pid that had already exited. Nothing clears it but a person. One such directory blocked an openSUSE target across three consecutive CI runs before anyone looked at what the lock actually said. buildrpms.pl has released its lock on cancellation for some time, through an END block and an abort handler. This is the Debian builder catching up. The order matters, and is the reason this is not simply an END block. The command in flight is stopped BEFORE the lock is released: handing the checkout to a second build while dpkg-buildpackage is still rewriting debian/changelog and debian/control in it is worse than holding the lock a moment longer. The wait for that command is bounded, so a subprocess that ignores the signal cannot hold the lock for ever either. sh() now forks and execs rather than calling system(), because system() gives no pid and a handler cannot stop what it cannot name. The child _exits rather than exits, so it never runs the parent's END block and releases a lock the parent still holds. The handler is installed by XCAT::BuildUtils::install_build_cancellation rather than written inline in the builder, so a test can use the same wiring the builder uses. A test that installs an equivalent handler of its own proves the helper works while saying nothing about whether anything calls it -- the first version of this test did exactly that, and passed with the wiring removed. Release is idempotent: a signal handler and then DESTROY both reach it, and the second must not remove a directory a LATER build has since taken. builddebs_lock_cancellation.t terminates the holder, then takes the lock again, and checks no build subprocess was orphaned. Verified by removing the wiring: assertions 9 through 12 fail, naming the leaked lock, the refused build and the stray process. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com> --- build-utils/lib/XCAT/BuildUtils.pm | 150 ++++++++++++++++++- builddebs.pl | 14 ++ xCAT-test/unit/builddebs_lock_cancellation.t | 124 +++++++++++++++ 3 files changed, 284 insertions(+), 4 deletions(-) create mode 100644 xCAT-test/unit/builddebs_lock_cancellation.t diff --git a/build-utils/lib/XCAT/BuildUtils.pm b/build-utils/lib/XCAT/BuildUtils.pm index 3ca224908..4eacd6abb 100644 --- a/build-utils/lib/XCAT/BuildUtils.pm +++ b/build-utils/lib/XCAT/BuildUtils.pm @@ -107,10 +107,33 @@ sub rewrite_file { # status, which is the exit code times 256, so it is shifted here: a caller # comparing the result against a specific code gets the code it expects, not a # multiple of it. +# The pid of the command sh() is running, so a cancellation handler can stop it before the +# build lock is released. system() gives no pid, which is why this forks explicitly. +our $CURRENT_CHILD; + +# Locks this process holds, weakly. Released by a signal handler or at exit, because +# _BuildLock::DESTROY does not run when a signal ends the process. +our @LIVE_LOCKS; +END { release_build_locks() } + sub sh { my ($cmd) = @_; say "Running: $cmd" if $VERBOSE; - system($cmd); + my $pid = fork(); + unless (defined $pid) { + warn "FATAL: cannot fork to run: $cmd\n"; + return 127; + } + unless ($pid) { + # _exit, not exit: the child must not run the parent's END block and release a lock + # the parent still holds. + require POSIX; + exec('/bin/sh', '-c', $cmd) or POSIX::_exit(127); + } + local $CURRENT_CHILD = $pid; + # waitpid returns -1 with EINTR when a signal arrives, and a build takes signals. + my $got; + do { $got = waitpid($pid, 0) } while ($got == -1 && $!{EINTR}); return $? >> 8; } @@ -478,12 +501,131 @@ sub take_build_lock { if (open(my $ow, '>', "$lockdir/owner")) { print {$ow} "pid=$$\n"; close $ow } # The caller keeps the returned value; release is by pid so a fork cannot free the parent's. my $owner = $$; - return XCAT::BuildUtils::_BuildLock->new($lockdir, $owner); + my $lock = XCAT::BuildUtils::_BuildLock->new($lockdir, $owner); + # Registered weakly, so holding it here does not keep the lock alive past its caller's + # scope. The registry exists only so a signal can release what DESTROY will not. + require Scalar::Util; + push @LIVE_LOCKS, $lock; + Scalar::Util::weaken($LIVE_LOCKS[-1]); + return $lock; +} + +#------------------------------------------------------------------------------- + +=head3 release_build_locks + +Descriptions: + Release every build lock this process still holds. + + DESTROY does not run when a signal terminates the process, so a cancelled build left its + lock directory behind and the next build of that checkout died on "another build already + holds" naming a pid that had long exited. One such directory blocked an openSUSE target + across three consecutive runs before anyone looked. + +Arguments: + None. +Returns: + Nothing. + +=cut + +#------------------------------------------------------------------------------- +sub release_build_locks { + for my $l (@LIVE_LOCKS) { $l->release if defined $l } + return; +} + +#------------------------------------------------------------------------------- + +=head3 install_build_cancellation + +Descriptions: + Install the INT and TERM handlers that stop the build and release its locks. + + It lives here rather than in the builder so the behaviour can be tested. A builder that + wired its own handler inline could only be covered by reading its source, and a test that + installs an equivalent handler of its own proves the helper works while saying nothing + about whether anything calls it. + + The signal is re-raised with the default disposition afterwards, so the exit status still + tells a caller the build was cancelled rather than that it failed. + +Arguments: + $announce - optional coderef called with the signal name before the build is stopped +Returns: + Nothing. + +=cut + +#------------------------------------------------------------------------------- +sub install_build_cancellation { + my ($announce) = @_; + for my $sig (qw(INT TERM)) { + $SIG{$sig} = sub { + my ($caught) = @_; + $announce->($caught) if $announce; + cancel_build($caught); + $SIG{$caught} = 'DEFAULT'; + kill $caught => $$; + }; + } + return; +} + +#------------------------------------------------------------------------------- + +=head3 cancel_build + +Descriptions: + Stop the command in flight, then release the build locks. + + The order matters. Releasing first would hand the checkout to a second build while + dpkg-buildpackage is still rewriting debian/changelog and debian/control in it. + + The wait is bounded: a child that ignores the signal must not keep the lock for ever, so + it is given a few seconds and then killed outright. + +Arguments: + $sig - the signal name that started the cancellation +Returns: + Nothing. + +=cut + +#------------------------------------------------------------------------------- +sub cancel_build { + my ($sig) = @_; + require POSIX; + if ($CURRENT_CHILD) { + kill $sig => $CURRENT_CHILD; + my $gone = 0; + for (1 .. 50) { + if (waitpid($CURRENT_CHILD, POSIX::WNOHANG()) > 0) { $gone = 1; last } + select undef, undef, undef, 0.1; + } + unless ($gone) { + kill 'KILL' => $CURRENT_CHILD; + waitpid($CURRENT_CHILD, 0); + } + } + release_build_locks(); + return; } { package XCAT::BuildUtils::_BuildLock; - sub new { my ($c,$d,$p)=@_; return bless { dir=>$d, pid=>$p }, $c } - sub DESTROY { my $s=shift; return unless $$ == $s->{pid}; unlink "$s->{dir}/owner"; rmdir $s->{dir} } + sub new { my ($c,$d,$p)=@_; return bless { dir=>$d, pid=>$p, released=>0 }, $c } + # Idempotent: a signal handler and then DESTROY both reach here, and the second must not + # remove a directory a LATER build has since taken. + sub release { + my $s = shift; + return if $s->{released}; + $s->{released} = 1; + return unless $$ == $s->{pid}; + unlink "$s->{dir}/owner"; + rmdir $s->{dir}; + return; + } + sub DESTROY { shift->release } } # The rpm architecture a mock target builds for. A target carries the arch as its diff --git a/builddebs.pl b/builddebs.pl index def0fd89b..4c53ca722 100755 --- a/builddebs.pl +++ b/builddebs.pl @@ -335,6 +335,20 @@ SCRIPT } # ----------------------------------------------------------------- main ------ +# A cancelled build must not keep the checkout. The lock releases in DESTROY, which perl does +# not run when a signal ends the process, so a killed build left its directory behind and the +# next build of that checkout died on "another build already holds" naming a pid that had +# already exited. buildrpms.pl has released its lock on cancellation for some time; this is the +# Debian builder catching up. +# +# cancel_build stops the command in flight BEFORE releasing: handing the checkout to a second +# build while dpkg-buildpackage is still rewriting debian/changelog in it is worse than holding +# the lock a moment longer. +XCAT::BuildUtils::install_build_cancellation(sub { + my ($caught) = @_; + print STDERR "\n[builddebs] SIG$caught: stopping the build and releasing the lock\n"; +}); + my $lock = take_build_lock($ROOT); my $dest = resolve_dest($opts{dest}, "$ROOT/dist/debs"); diff --git a/xCAT-test/unit/builddebs_lock_cancellation.t b/xCAT-test/unit/builddebs_lock_cancellation.t new file mode 100644 index 000000000..b2258306c --- /dev/null +++ b/xCAT-test/unit/builddebs_lock_cancellation.t @@ -0,0 +1,124 @@ +#!/usr/bin/env perl +# A KILLED BUILD MUST NOT KEEP THE CHECKOUT. +# +# The build lock is released in DESTROY, and perl does not run DESTROY when a signal ends the +# process. So a cancelled build left its lock directory behind, and the next build of that +# checkout died on +# FATAL: another build of already holds (held by [pid=NNNN]) +# naming a pid that had already exited. One such directory blocked an openSUSE target across +# three consecutive runs before anyone looked at it. +# +# buildrpms.pl has released its lock on cancellation for some time. This covers the Debian +# builder doing the same, and the ORDER it must do it in: the command in flight is stopped +# before the lock is released, because handing the checkout to a second build while +# dpkg-buildpackage is still rewriting debian/changelog in it is worse than holding the lock a +# moment longer. +use strict; +use warnings; +use Test::More; +use File::Temp qw(tempdir); +use POSIX qw(WNOHANG); +use FindBin; +use lib "$FindBin::Bin/../../build-utils/lib"; +use XCAT::BuildUtils (); + +my $lockdir = tempdir(CLEANUP => 1); +my $ckout = tempdir(CLEANUP => 1); + +# --------------------------------------------------------------------------- +# 1. The lock is taken, and a second taker is refused. Without this the test below could pass +# by the lock never having worked at all. +# --------------------------------------------------------------------------- +{ + my $l = XCAT::BuildUtils::take_build_lock($ckout, $lockdir); + ok($l, 'a build takes the checkout lock'); + my $path = XCAT::BuildUtils::lock_path_for($ckout, $lockdir) . '.d'; + ok(-d $path, 'the lock directory exists while it is held'); + my $second = eval { XCAT::BuildUtils::take_build_lock($ckout, $lockdir) }; + ok(!$second, 'a second build of the same checkout is refused'); + like($@, qr/already holds/, 'and told which lock is held'); + undef $l; + ok(!-d $path, 'releasing it removes the directory'); +} + +# --------------------------------------------------------------------------- +# 2. THE REGRESSION. Terminate the holder with SIGTERM, then take the lock again. +# --------------------------------------------------------------------------- +{ + my $path = XCAT::BuildUtils::lock_path_for($ckout, $lockdir) . '.d'; + my $ready = "$lockdir/ready"; + + my $pid = fork(); + die "cannot fork\n" unless defined $pid; + unless ($pid) { + # the child is the build: it takes the lock, says so, and waits to be killed + # exactly what builddebs.pl does -- not a hand-rolled equivalent, or this would pass + # with the wiring removed and prove nothing. + XCAT::BuildUtils::install_build_cancellation(); + my $l = XCAT::BuildUtils::take_build_lock($ckout, $lockdir); + if (open(my $r, '>', $ready)) { close $r } + sleep 30; + POSIX::_exit(0); + } + + # wait for the child to actually hold it, rather than guessing with a sleep + my $held = 0; + for (1 .. 100) { if (-e $ready) { $held = 1; last } select undef, undef, undef, 0.1 } + ok($held, 'the build says it holds the lock'); + ok(-d $path, 'and the directory is there while it runs'); + + kill 'TERM' => $pid; + my $reaped = 0; + for (1 .. 100) { if (waitpid($pid, WNOHANG) > 0) { $reaped = 1; last } select undef, undef, undef, 0.1 } + ok($reaped, 'the build stops when it is terminated'); + + ok(!-d $path, 'a TERMINATED build leaves no lock behind'); + + # the point of all of it: the next build can run + my $next = eval { XCAT::BuildUtils::take_build_lock($ckout, $lockdir) }; + ok($next, 'the next build of that checkout takes the lock') + or diag("still refused: $@"); + undef $next; +} + +# --------------------------------------------------------------------------- +# 3. The command in flight is STOPPED, not orphaned. +# Checked from the parent, because the cancelled build re-raises the signal with the default +# disposition and so never reaches an END block of its own. A distinctive sleep makes the +# build subprocess findable; the pattern is bracketed so the search cannot match itself. +# --------------------------------------------------------------------------- +{ + my $started = "$lockdir/started3"; + my $marker = '778349'; # nothing else on this host sleeps for this long + my $pid = fork(); + die "cannot fork\n" unless defined $pid; + unless ($pid) { + XCAT::BuildUtils::install_build_cancellation(); + my $l = XCAT::BuildUtils::take_build_lock($ckout, $lockdir); + if (open(my $st, '>', $started)) { close $st } + XCAT::BuildUtils::sh("sleep $marker"); + POSIX::_exit(0); + } + my $up = 0; + for (1 .. 100) { if (-e $started) { $up = 1; last } select undef, undef, undef, 0.1 } + ok($up, 'the build subprocess is running'); + my $live = `pgrep -f "[s]leep $marker" 2>/dev/null | wc -l`; chomp $live; + cmp_ok($live, '>', 0, 'and the test can see it -- the control for the check below'); + + kill 'TERM' => $pid; + my $reaped = 0; + for (1 .. 100) { if (waitpid($pid, WNOHANG) > 0) { $reaped = 1; last } select undef, undef, undef, 0.1 } + ok($reaped, 'the cancelled build exits'); + + my $left = 1; + for (1 .. 50) { + $left = `pgrep -f "[s]leep $marker" 2>/dev/null | wc -l`; chomp $left; + last if $left == 0; + select undef, undef, undef, 0.1; + } + is($left, 0, 'the build subprocess was stopped, not left running without its lock') + or do { diag('orphaned build subprocess still holds the checkout'); + system("pkill -f '[s]leep $marker'") }; +} + +done_testing(); From c8a57880d6af6994b59e20e5435104da02cd5e05 Mon Sep 17 00:00:00 2001 From: Daniel Hilst <392820+dhilst@users.noreply.github.com> Date: Thu, 24 Sep 2026 06:02:12 -0300 Subject: [PATCH 3/3] fix(xcat-core): cancelling a build left its workers writing the checkout Killing the command is not killing the build. sh() ran the command through /bin/sh, and cancellation signalled that shell alone -- but dpkg-buildpackage starts workers of its own, and those survive their shell. The lock was then released while they were still writing debian/changelog and debian/control, which is the state the lock exists to prevent: the next build takes the checkout and the two rewrite it together. The command now runs in its own process group, so cancellation can take all of it. Both sides call setpgid, so neither depends on which runs first, and INT and TERM are blocked across the fork so cancellation cannot land in the window before the group exists. Cancellation escalates from the caught signal to KILL, and then CHECKS: a shell that has exited is not a build that has stopped, so it waits for the whole group to disappear rather than for the leader to be reaped. If the group is still there after that, the locks are RETAINED and the process exits non-zero. Releasing a lock while a worker may still be writing is worse than leaving a lock behind for a person to clear -- the first corrupts a build, the second stops one. cancel_build ignores INT and TERM while it runs, so a second Ctrl-C cannot interrupt the cleanup half way and release the lock early. sh() also reports a signalled command as 128+signal instead of 0. $? >> 8 is zero for a child killed by a signal, so a build stopped mid-way looked to its caller like one that had succeeded. Two cases added to builddebs_lock_cancellation.t: a build whose worker is a grandchild, and a command killed by a signal. Verified by signalling the pid instead of the group, which leaves the worker running and turns the first red. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com> --- build-utils/lib/XCAT/BuildUtils.pm | 76 +++++++++++++++----- xCAT-test/unit/builddebs_lock_cancellation.t | 63 ++++++++++++++++ 2 files changed, 122 insertions(+), 17 deletions(-) diff --git a/build-utils/lib/XCAT/BuildUtils.pm b/build-utils/lib/XCAT/BuildUtils.pm index 4eacd6abb..5eb897799 100644 --- a/build-utils/lib/XCAT/BuildUtils.pm +++ b/build-utils/lib/XCAT/BuildUtils.pm @@ -118,23 +118,51 @@ END { release_build_locks() } sub sh { my ($cmd) = @_; + require POSIX; say "Running: $cmd" if $VERBOSE; + + # Do not let cancellation run between fork and publishing the process group. + my $blocked = POSIX::SigSet->new(POSIX::SIGINT(), POSIX::SIGTERM()); + my $oldmask = POSIX::SigSet->new(); + POSIX::sigprocmask(POSIX::SIG_BLOCK(), $blocked, $oldmask) + or die "Cannot block build cancellation signals: $!\n"; + my $pid = fork(); unless (defined $pid) { - warn "FATAL: cannot fork to run: $cmd\n"; + my $error = "$!"; + POSIX::sigprocmask(POSIX::SIG_SETMASK(), $oldmask) + or POSIX::_exit(127); + warn "FATAL: cannot fork to run $cmd: $error\n"; return 127; } unless ($pid) { - # _exit, not exit: the child must not run the parent's END block and release a lock - # the parent still holds. - require POSIX; + $SIG{INT} = $SIG{TERM} = 'DEFAULT'; + POSIX::setpgid(0, 0) or POSIX::_exit(127); + POSIX::sigprocmask(POSIX::SIG_SETMASK(), $oldmask) + or POSIX::_exit(127); exec('/bin/sh', '-c', $cmd) or POSIX::_exit(127); } - local $CURRENT_CHILD = $pid; - # waitpid returns -1 with EINTR when a signal arrives, and a build takes signals. + + local $CURRENT_CHILD = $pid; # also the command's process-group ID + # Both sides set the group, so neither depends on which side runs first. + # EACCES means the child already exec'd, after setting its group; ESRCH + # means it has already gone away. + unless (POSIX::setpgid($pid, $pid) || $!{EACCES} || $!{ESRCH}) { + warn "FATAL: cannot create build process group $pid: $!; retaining locks\n"; + kill 'KILL', $pid; + POSIX::_exit(127); + } + POSIX::sigprocmask(POSIX::SIG_SETMASK(), $oldmask) or do { + warn "FATAL: cannot restore signal mask: $!\n"; + cancel_build('TERM'); + POSIX::_exit(127); + }; + my $got; do { $got = waitpid($pid, 0) } while ($got == -1 && $!{EINTR}); - return $? >> 8; + my $status = $?; + return 127 if $got == -1; + return ($status & 127) ? 128 + ($status & 127) : $status >> 8; } # pod2usage reads the POD of the running program, so each builder keeps its own @@ -596,17 +624,31 @@ Returns: sub cancel_build { my ($sig) = @_; require POSIX; - if ($CURRENT_CHILD) { - kill $sig => $CURRENT_CHILD; - my $gone = 0; - for (1 .. 50) { - if (waitpid($CURRENT_CHILD, POSIX::WNOHANG()) > 0) { $gone = 1; last } - select undef, undef, undef, 0.1; - } - unless ($gone) { - kill 'KILL' => $CURRENT_CHILD; - waitpid($CURRENT_CHILD, 0); + # A second Ctrl-C must not interrupt cleanup and release the lock early. + local $SIG{INT} = 'IGNORE'; + local $SIG{TERM} = 'IGNORE'; + + if (my $pgid = $CURRENT_CHILD) { + my $reaped = 0; + for my $stop_signal ($sig, 'KILL') { + kill $stop_signal, -$pgid; + for (1 .. 50) { + unless ($reaped) { + my $got = waitpid($pgid, POSIX::WNOHANG()); + $reaped = 1 if $got == $pgid || ($got == -1 && $!{ECHILD}); + } + # The shell exiting is not enough: its workers may still exist. + if (!kill(0, -$pgid) && $!{ESRCH}) { + $CURRENT_CHILD = undef; + release_build_locks(); + return; + } + select undef, undef, undef, 0.1; + } } + # Never let END/DESTROY unlock a checkout whose workers may still run. + warn "FATAL: build process group $pgid has not disappeared; retaining locks\n"; + POSIX::_exit(1); } release_build_locks(); return; diff --git a/xCAT-test/unit/builddebs_lock_cancellation.t b/xCAT-test/unit/builddebs_lock_cancellation.t index b2258306c..37d9de525 100644 --- a/xCAT-test/unit/builddebs_lock_cancellation.t +++ b/xCAT-test/unit/builddebs_lock_cancellation.t @@ -121,4 +121,67 @@ my $ckout = tempdir(CLEANUP => 1); system("pkill -f '[s]leep $marker'") }; } +# --------------------------------------------------------------------------- +# 4. THE WHOLE PROCESS GROUP GOES, not just the shell. +# Killing /bin/sh does not kill what it started: dpkg-buildpackage leaves workers behind, and +# those keep writing the checkout after the lock would otherwise have been handed to the next +# build. The command runs in its own process group so cancellation can take all of it. +# --------------------------------------------------------------------------- +{ + my $tag = '661277'; # the worker, a grandchild of the build + my $started = "$lockdir/started4"; + my $pid = fork(); + die "cannot fork\n" unless defined $pid; + unless ($pid) { + XCAT::BuildUtils::install_build_cancellation(); + my $l = XCAT::BuildUtils::take_build_lock($ckout, $lockdir); + if (open(my $st, '>', $started)) { close $st } + # a shell that spawns a worker and waits: the worker is a GRANDCHILD of this process + XCAT::BuildUtils::sh("sleep $tag & sleep $tag"); + POSIX::_exit(0); + } + my $up = 0; + for (1 .. 100) { if (-e $started) { $up = 1; last } select undef, undef, undef, 0.1 } + ok($up, 'the build started'); + + my $workers = 0; + for (1 .. 100) { + $workers = `pgrep -f "[s]leep $tag" 2>/dev/null | wc -l`; chomp $workers; + last if $workers >= 2; + select undef, undef, undef, 0.1; + } + cmp_ok($workers, '>=', 2, 'the build has a worker of its own -- the control for the check below'); + + kill 'TERM' => $pid; + for (1 .. 100) { last if waitpid($pid, WNOHANG) > 0; select undef, undef, undef, 0.1 } + + my $left = 1; + for (1 .. 100) { + $left = `pgrep -f "[s]leep $tag" 2>/dev/null | wc -l`; chomp $left; + last if $left == 0; + select undef, undef, undef, 0.1; + } + is($left, 0, 'cancelling the build takes its workers with it, not just its shell') + or do { diag('a build worker outlived the cancellation and can still write the checkout'); + system("pkill -f '[s]leep $tag'") }; +} + +# --------------------------------------------------------------------------- +# 5. A command killed by a signal reports as killed, not as success. +# $? >> 8 is 0 for a signalled child, so a build stopped mid-way looked like it had worked. +# --------------------------------------------------------------------------- +{ + my $rc = -1; + my $pid = fork(); + die "cannot fork\n" unless defined $pid; + unless ($pid) { + my $got = XCAT::BuildUtils::sh("kill -TERM \$\$"); + POSIX::_exit($got); + } + waitpid($pid, 0); + $rc = $? >> 8; + is($rc, 128 + 15, 'a command killed by SIGTERM reports 128+15, not 0'); +} + + done_testing();