From b62585475a9e8d7362993b702bb6365fcda599f7 Mon Sep 17 00:00:00 2001 From: Daniel Hilst <392820+dhilst@users.noreply.github.com> Date: Thu, 24 Sep 2026 10:44:10 -0300 Subject: [PATCH] fix(xcat-core): build locks the shared tree refuses to grant A cancelled build left its lock directory behind on the shared tree, so every later build of the same checkout waited for a lock whose owner was gone. The build helper ran each command with a plain fork and killed only the immediate child, so the shell's own workers survived the cancellation and kept the tree in use. XCAT::BuildUtils now runs each command in its own process group, blocks the cancellation signals across the fork so neither side can miss the group, and signals the group rather than the pid. It escalates to KILL, confirms the group is gone, and retains the locks when it is not, so a lock is never released while a worker can still write. xCAT-test/unit/builddebs_lock_cancellation.t captures the defect. It fails without the change. Backport of #7867 to 2.19. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com> --- build-utils/lib/XCAT/BuildUtils.pm | 215 ++++++++++++++++++- builddebs.pl | 14 ++ buildrpms.pl | 30 ++- xCAT-test/unit/builddebs_lock.t | 8 +- xCAT-test/unit/builddebs_lock_cancellation.t | 187 ++++++++++++++++ 5 files changed, 433 insertions(+), 21 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 54df11487..5eb897799 100644 --- a/build-utils/lib/XCAT/BuildUtils.pm +++ b/build-utils/lib/XCAT/BuildUtils.pm @@ -107,11 +107,62 @@ 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) = @_; + require POSIX; say "Running: $cmd" if $VERBOSE; - system($cmd); - return $? >> 8; + + # 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) { + 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) { + $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; # 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}); + 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 @@ -463,12 +514,160 @@ 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 = $$; + 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; + # 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; +} + +{ package XCAT::BuildUtils::_BuildLock; + 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/buildrpms.pl b/buildrpms.pl index df22aaea2..42b87bb7a 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; @@ -876,9 +876,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. @@ -935,6 +943,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 } @@ -964,9 +975,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" @@ -974,9 +986,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(); diff --git a/xCAT-test/unit/builddebs_lock_cancellation.t b/xCAT-test/unit/builddebs_lock_cancellation.t new file mode 100644 index 000000000..37d9de525 --- /dev/null +++ b/xCAT-test/unit/builddebs_lock_cancellation.t @@ -0,0 +1,187 @@ +#!/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'") }; +} + +# --------------------------------------------------------------------------- +# 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();