xCAT/xcat-core
2
0
Fork 0
mirror of https://github.com/xcat2/xcat-core.git synced 2026-09-25 01:04:05 +00:00
Code Issues Releases Wiki Activity

fix(xcat-core): a cancelled Debian build keeps the checkout lock for ever

Browse Source 
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 <path> already holds <dir> (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>
This commit is contained in:
Daniel Hilst
2026-09-24 00:39:10 -03:00
parent 88441f6ec3
commit 7d52506a2d
3 changed files with 284 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files
build-utils/lib/XCAT/BuildUtils.pm
+146 -4
View File
@@ -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
builddebs.pl
+14
View File
@@ -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");
xCAT-test/unit/builddebs_lock_cancellation.t
+124
View File
@@ -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 <path> already holds <dir> (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();