Don't log Ctrl-C/SIGTERM on a foreground session as an error
run_process_foreground() previously collapsed "child killed by a signal" into the exact same exit_code == -1 sentinel as "fork() itself failed", triggering commands.cpp's "failed to run bwrap" spdlog::error for the ordinary case of Ctrl-C (SIGINT, forwarded to bwrap by this project's own forward_signal_to_foreground_child handler) or `kill`/--kill (SIGTERM) ending a foreground -r/--run session -- both ways this project deliberately supports stopping one cleanly, not failures. Now distinguishes WIFSIGNALED from a real fork() failure, returning 128+signal (the same convention a shell itself uses for $? after a signal-killed job) instead of -1. SIGINT/SIGTERM specifically log at debug (invisible at the default log level) rather than warn; any other signal still warns, since that's a genuine, unexpected crash. commands.cpp's own `exit_code < 0` check is now accurate -- it only ever fires on a genuine fork() failure. Verified directly (rootless, this dev machine): SIGINT and SIGTERM against a running foreground session both now exit 130/143 respectively with no error or warning logged at the default level (only debug), unmount/cleanup still ran either way; a genuine failure (nonexistent command inside the sandbox) still warns and exits 1, unaffected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gv3s5jckJKzh6JkMoi2Akz
This commit is contained in:
@@ -809,6 +809,12 @@ int run_container(const std::filesystem::path& image_tar,
|
||||
exit_code = run_bwrap(mounted->merged_path, command, use_nsenter, volume_mounts, resolved_user, hostname,
|
||||
container_name, *resolved_env, namespace_config, inject_dns_resolv_conf,
|
||||
on_bwrap_pid_known);
|
||||
// Only genuinely negative when fork() itself failed inside
|
||||
// run_process_foreground() (process.h) -- a normal Ctrl-C/`kill`
|
||||
// stopping a foreground session (SIGINT/SIGTERM forwarded to the
|
||||
// running bwrap) returns 128+signal instead, not treated as a
|
||||
// failure here, since that's this project's own deliberate,
|
||||
// expected way to end one.
|
||||
if (exit_code < 0) {
|
||||
spdlog::error("failed to run bwrap");
|
||||
}
|
||||
|
||||
+20
-3
@@ -154,9 +154,26 @@ int run_process_foreground(const std::vector<std::string>& argv, const std::func
|
||||
sigaction(SIGTERM, &old_sigterm, nullptr);
|
||||
g_foreground_child_pid = 0;
|
||||
|
||||
int exit_code = WIFEXITED(status) ? WEXITSTATUS(status) : -1;
|
||||
if (exit_code != 0) {
|
||||
spdlog::warn("external command failed (exit code {}): {}", exit_code, fmt::join(argv, " "));
|
||||
int exit_code;
|
||||
if (WIFEXITED(status)) {
|
||||
exit_code = WEXITSTATUS(status);
|
||||
if (exit_code != 0) {
|
||||
spdlog::warn("external command failed (exit code {}): {}", exit_code, fmt::join(argv, " "));
|
||||
}
|
||||
} else if (WIFSIGNALED(status)) {
|
||||
int sig = WTERMSIG(status);
|
||||
exit_code = 128 + sig;
|
||||
if (sig == SIGINT || sig == SIGTERM) {
|
||||
// The normal, expected way to stop a foreground session (Ctrl-C,
|
||||
// or `kill`/--kill) -- not a failure, so debug rather than warn.
|
||||
spdlog::debug("external command terminated by {} (exit code {}): {}", sig == SIGINT ? "SIGINT" : "SIGTERM",
|
||||
exit_code, fmt::join(argv, " "));
|
||||
} else {
|
||||
spdlog::warn("external command terminated by signal {} (exit code {}): {}", sig, exit_code,
|
||||
fmt::join(argv, " "));
|
||||
}
|
||||
} else {
|
||||
exit_code = -1;
|
||||
}
|
||||
return exit_code;
|
||||
}
|
||||
|
||||
+13
-1
@@ -46,7 +46,19 @@ ProcessResult run_process(const std::vector<std::string>& argv);
|
||||
// clearenv()/setenv(), before exec) instead of inheriting this process's own --
|
||||
// e.g. run_bwrap() uses this to give the sandboxed command a minimal, controlled
|
||||
// environment without relying on bwrap's own --clearenv/--setenv. Returns the
|
||||
// exit code, or -1 if fork or exec failed (on_start is not called in that case).
|
||||
// exit code if the child exited normally; -1 only if fork() itself failed
|
||||
// (on_start is not called in that case) -- a failed execvp inside the child
|
||||
// is a normal exit with code 127, not -1. If the child was instead killed by
|
||||
// a signal (including SIGINT/SIGTERM forwarded from this same process's own
|
||||
// signal handler -- see forward_signal_to_foreground_child, process.cpp --
|
||||
// e.g. Ctrl-C or `kill`ing a foreground -r/--run session, both perfectly
|
||||
// ordinary ways to end one), returns 128+signal, the same convention a shell
|
||||
// itself uses for $? after a signal-killed job. SIGINT/SIGTERM specifically
|
||||
// are logged at debug, not warn, since forwarding one there is this
|
||||
// project's own deliberate, expected way to stop a foreground session
|
||||
// cleanly (see run_container()'s unmount/cleanup running afterward either
|
||||
// way) -- any other signal still logs a warning, since that's a genuine,
|
||||
// unexpected crash.
|
||||
int run_process_foreground(const std::vector<std::string>& argv,
|
||||
const std::function<void(pid_t)>& on_start = nullptr,
|
||||
const std::optional<std::vector<std::pair<std::string, std::string>>>& env =
|
||||
|
||||
Reference in New Issue
Block a user