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): cancelling a build left its workers writing the checkout

Browse Source 
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>
This commit is contained in:
Daniel Hilst
2026-09-24 06:02:12 -03:00
parent 7d52506a2d
commit c8a57880d6
2 changed files with 122 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files
build-utils/lib/XCAT/BuildUtils.pm
+59 -17
View File
@@ -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;
xCAT-test/unit/builddebs_lock_cancellation.t
+63
View File
@@ -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();