From d4c0e8f0a107dfcb9b42358fabb3f26e781f704f Mon Sep 17 00:00:00 2001 From: Viorel Munteanu Date: Sat, 22 Aug 2026 09:16:57 +0000 Subject: [PATCH] Add --clean-processes to remove stale session pid files Scans the same $XDG_STATE_HOME/slocker-lite/run/ directory as --list-processes and removes every pid file that's genuinely stale. The liveness check and the removal happen atomically per file (the same non-blocking flock() used to test it is held across the remove() call itself), rather than reusing a separate earlier scan, closing the race window where a new session could start in between. Only files actually removed are reported, one line each; sessions still running are left untouched and silently skipped. Refactored list_sessions() and the new clean_stale_sessions() to share a private open_session_file() helper for the open/read-pid/ recover-name step they both need. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Gv3s5jckJKzh6JkMoi2Akz --- CLAUDE.md | 31 ++++++++++---- README.md | 10 ++++- src/main.cpp | 37 ++++++++++++---- src/pid_file.cpp | 109 ++++++++++++++++++++++++++++++++++++----------- src/pid_file.h | 12 ++++++ 5 files changed, 157 insertions(+), 42 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index b7a1f54..105bec3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -18,12 +18,18 @@ Source layout (all under `src/`): - `main.cpp` — CLI entry point, dependency checks, orchestration (`mount_image()`, `run_container()`, `cleanup_image()`, `unmount_image()`, `list_images_command()`, `inspect_image_command()`, `create_volume_command()`, `list_volumes_command()`, - `delete_volume_command()`, `list_processes_command()`). `list_processes_command()` - implements `--list-processes` (long-option only): calls `list_sessions()` - (`pid_file.{h,cpp}`, see below) and prints one tab-aligned `pid`, `container - name`, `running`/`exited` row per entry (same two-column tab-alignment scheme - as `list_images_command()`/`list_volumes_command()`, extended to a third - column), no header row, silent success on an empty list. + `delete_volume_command()`, `list_processes_command()`, `clean_processes_command()`). + `list_processes_command()` implements `--list-processes` (long-option only): + calls `list_sessions()` (`pid_file.{h,cpp}`, see below) and prints one + tab-aligned `pid`, `container name`, `running`/`exited` row per entry (same + two-column tab-alignment scheme as `list_images_command()`/ + `list_volumes_command()`, extended to a third column), no header row, silent + success on an empty list. `clean_processes_command()` implements + `--clean-processes` (also long-option only): calls `clean_stale_sessions()` + (`pid_file.{h,cpp}`) and prints one `removed stale pid file for '' (pid + )` line per file actually removed — nothing is printed for sessions still + running, and an empty result (nothing stale) is silent success, same + convention as the rest of this file's list/delete commands. `inspect_image_command()` implements `-i/--inspect `: prints every `OciImageConfig` field (user/group, exposed ports, env, volumes, default command) without mounting or running the image — extend it @@ -206,7 +212,16 @@ Source layout (all under `src/`): released again immediately either way, never left held by the check itself. A file that can't be opened or doesn't parse as a pid (e.g. removed mid-scan) is silently skipped, not reported as an error — scanning a live directory is - inherently racy. + inherently racy. Both `list_sessions()` and `clean_stale_sessions()` (the + latter implements `--clean-processes`) share a private `open_session_file()` + helper for the open/read-pid/recover-name step. `clean_stale_sessions()` + doesn't just remove whatever a separate `list_sessions()` call reported as not + running — it re-takes the same non-blocking `flock()` used to test liveness + and holds it across the `remove()` call itself, per file, so the stale check + and the removal stay atomic against a new session starting in the gap between + a check and a later removal. Only files it actually removes are reported back + (as `SessionInfo`s with `running=false`); still-locked (running) files are + left untouched and not reported. - `config_file.{h,cpp}` — `load_config_file()` reads and parses (via libyaml's document API, ``) the `global` and `volumes` sections of the local YAML config file located by `config_file_path()` (`$XDG_CONFIG_HOME/slocker-lite/config.yaml`, @@ -285,7 +300,7 @@ Build directory is `buildDir/` (already configured). full flag list: `-m/--mount`, `-r/--run`, `-u/--umount`, `-c/--cleanup`, `-l/--list-images`, `-i/--inspect`, `-n/--no-nsenter`, `--user`, `--group`, `--hostname`, `-v/--volume`, `--list-volumes`, `--delete-volume`, - `--delete-volume-full`, `--list-processes`, `-t/--test`, `--log-level`, + `--delete-volume-full`, `--list-processes`, `--clean-processes`, `-t/--test`, `--log-level`, `-h/--help`, `-V/--version`) - Run tests: `meson test -C buildDir` diff --git a/README.md b/README.md index f896098..a692085 100644 --- a/README.md +++ b/README.md @@ -63,6 +63,7 @@ slocker-lite --list-volumes slocker-lite --delete-volume slocker-lite --delete-volume-full slocker-lite --list-processes +slocker-lite --clean-processes slocker-lite -t|--test slocker-lite -h|--help slocker-lite -V|--version @@ -85,6 +86,7 @@ slocker-lite -V|--version | `--delete-volume ` | Remove a named volume from the config. The host directory is left untouched. | | `--delete-volume-full ` | Like `--delete-volume`, but also recursively deletes the volume's host directory. | | `--list-processes` | List running `--run` sessions found by their pid files under `$XDG_STATE_HOME/slocker-lite/run/`, with their pid, container name, and status (`running` or `exited`). | +| `--clean-processes` | Remove stale pid files (see `--list-processes`) left behind by sessions that are no longer running. | | `-t, --test` | Print which `bwrap --unshare-xxx` namespaces the running kernel supports. | | `--log-level ` | Set log verbosity (`trace`, `debug`, `info`, `warn`, `error`, `critical`, `off`). | | `-h, --help` | Print usage and exit. | @@ -131,6 +133,9 @@ sudo ./buildDir/slocker-lite -r myimage.tar --user git # List currently running (and any leftover, exited) --run sessions ./buildDir/slocker-lite --list-processes + +# Remove any leftover, stale pid files +./buildDir/slocker-lite --clean-processes ``` ## Configuration @@ -177,7 +182,10 @@ be run concurrently without collisions. The file is removed automatically once t run ends; any tool can check whether a session is still alive by attempting the same exclusive, non-blocking `flock()` on its file. `--list-processes` does exactly that for every pid file it finds, reporting each one's pid, container -name, and `running`/`exited` status. +name, and `running`/`exited` status. Normally the file is removed automatically +when its own session ends, but `--clean-processes` removes any stale ones left +behind (e.g. after a crash) using that same check, atomically per file, so it +never removes one that's still genuinely running. See `CLAUDE.md` for the full architecture writeup (file-by-file breakdown, the reasoning behind each of the above, and known gaps). diff --git a/src/main.cpp b/src/main.cpp index c1a0d89..3804679 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -57,14 +57,15 @@ enum class Mode { kDeleteVolume, kDeleteVolumeFull, kInspect, - kListProcesses + kListProcesses, + kCleanProcesses }; // --log-level/--user/--group/--list-volumes/--delete-volume[-full]/--hostname/ -// --list-processes have no short form (--log-level's was freed up so -l could -// become --list-images; -u is already --umount; the rest have no natural free -// letter left, or don't need one), so they need long-option vals outside the -// printable-char range short options use. +// --list-processes/--clean-processes have no short form (--log-level's was freed +// up so -l could become --list-images; -u is already --umount; the rest have no +// natural free letter left, or don't need one), so they need long-option vals +// outside the printable-char range short options use. constexpr int kLogLevelOpt = 256; constexpr int kUserOpt = 257; constexpr int kGroupOpt = 258; @@ -73,8 +74,9 @@ constexpr int kDeleteVolumeOpt = 260; constexpr int kDeleteVolumeFullOpt = 261; constexpr int kHostnameOpt = 262; constexpr int kListProcessesOpt = 263; +constexpr int kCleanProcessesOpt = 264; -constexpr std::array kLongOptions = {{ +constexpr std::array kLongOptions = {{ {"help", no_argument, nullptr, 'h'}, {"version", no_argument, nullptr, 'V'}, {"test", no_argument, nullptr, 't'}, @@ -94,6 +96,7 @@ constexpr std::array kLongOptions = {{ {"inspect", required_argument, nullptr, 'i'}, {"hostname", required_argument, nullptr, kHostnameOpt}, {"list-processes", no_argument, nullptr, kListProcessesOpt}, + {"clean-processes", no_argument, nullptr, kCleanProcessesOpt}, {nullptr, 0, nullptr, 0}, }}; @@ -110,6 +113,7 @@ void print_usage(const char* prog) { " {0} --delete-volume \n" " {0} --delete-volume-full \n" " {0} --list-processes\n" + " {0} --clean-processes\n" " {0} -t|--test\n" " {0} -h|--help\n" " {0} -V|--version\n" @@ -170,6 +174,9 @@ void print_usage(const char* prog) { " files under $XDG_STATE_HOME/slocker-lite/run/,\n" " with their pid, container name, and status\n" " (running or exited)\n" + " --clean-processes remove stale pid files (see --list-processes)\n" + " left behind by sessions that are no longer\n" + " running\n" " -t, --test run the test suite\n" " --log-level set log verbosity (trace, debug, info, warn,\n" " error, critical, off)\n" @@ -469,6 +476,13 @@ int list_processes_command() { return 0; } +int clean_processes_command() { + for (const auto& session : clean_stale_sessions()) { + fmt::print("removed stale pid file for '{}' (pid {})\n", session.container_name, session.pid); + } + return 0; +} + int delete_volume_command(const std::string& name, const std::filesystem::path& config_path, AppConfig& config, bool delete_directory) { auto it = std::find_if(config.volumes.begin(), config.volumes.end(), @@ -628,7 +642,8 @@ int main(int argc, char* argv[]) { case kListVolumesOpt: case kDeleteVolumeOpt: case kDeleteVolumeFullOpt: - case kListProcessesOpt: { + case kListProcessesOpt: + case kCleanProcessesOpt: { Mode requested; switch (opt) { case 't': @@ -661,9 +676,12 @@ int main(int argc, char* argv[]) { case kDeleteVolumeFullOpt: requested = Mode::kDeleteVolumeFull; break; - default: + case kListProcessesOpt: requested = Mode::kListProcesses; break; + default: + requested = Mode::kCleanProcesses; + break; } if (mode != Mode::kNone && mode != requested) { spdlog::error("multiple actions specified"); @@ -776,6 +794,9 @@ int main(int argc, char* argv[]) { if (mode == Mode::kListProcesses) { return list_processes_command(); } + if (mode == Mode::kCleanProcesses) { + return clean_processes_command(); + } if (mode == Mode::kRun) { std::vector command(argv + optind, argv + argc); // As root, containers-storage mount doesn't need to reexec into a private diff --git a/src/pid_file.cpp b/src/pid_file.cpp index e7fc3d6..8654d54 100644 --- a/src/pid_file.cpp +++ b/src/pid_file.cpp @@ -56,6 +56,46 @@ std::filesystem::path session_run_dir() { return state_home / "slocker-lite" / "run"; } +struct ParsedSessionFile { + pid_t pid; + std::string container_name; + int fd; +}; + +// Opens `path` and reads back its pid (from the file's own contents, not parsed +// from the filename -- ambiguous for names that themselves contain '-') and +// recovers container_name by stripping the exact "-" suffix that pid implies +// back off the filename. Returns nullopt (logged at debug level, never an error -- +// scanning a live directory is inherently racy) if the file can't be opened or its +// contents don't parse as a pid. The caller owns the returned fd and is +// responsible for closing it. +std::optional open_session_file(const std::filesystem::path& path) { + int fd = open(path.c_str(), O_RDWR); + if (fd < 0) { + spdlog::debug("failed to open session file {}: {}", path.string(), strerror(errno)); + return std::nullopt; + } + + char buf[32] = {}; + ssize_t n = read(fd, buf, sizeof(buf) - 1); + pid_t pid = n > 0 ? static_cast(std::atoi(buf)) : 0; + if (pid <= 0) { + spdlog::debug("skipping malformed session pid file {}", path.string()); + close(fd); + return std::nullopt; + } + + std::string filename = path.filename().string(); + std::string suffix = fmt::format("-{}", pid); + std::string container_name = + filename.size() > suffix.size() && + filename.compare(filename.size() - suffix.size(), suffix.size(), suffix) == 0 + ? filename.substr(0, filename.size() - suffix.size()) + : filename; + + return ParsedSessionFile{pid, container_name, fd}; +} + } // namespace std::filesystem::path session_pid_file_path(std::string_view container_name, pid_t pid) { @@ -117,39 +157,58 @@ std::vector list_sessions() { for (auto it = std::filesystem::directory_iterator(dir, it_ec); !it_ec && it != std::filesystem::directory_iterator(); it.increment(it_ec)) { const auto& path = it->path(); - - int fd = open(path.c_str(), O_RDWR); - if (fd < 0) { - spdlog::debug("failed to open session file {}: {}", path.string(), strerror(errno)); + auto parsed = open_session_file(path); + if (!parsed) { continue; } - char buf[32] = {}; - ssize_t n = read(fd, buf, sizeof(buf) - 1); - pid_t pid = n > 0 ? static_cast(std::atoi(buf)) : 0; - bool running = true; - if (flock(fd, LOCK_EX | LOCK_NB) == 0) { + if (flock(parsed->fd, LOCK_EX | LOCK_NB) == 0) { running = false; - flock(fd, LOCK_UN); + flock(parsed->fd, LOCK_UN); } - close(fd); + close(parsed->fd); - if (pid <= 0) { - spdlog::debug("skipping malformed session pid file {}", path.string()); - continue; - } - - std::string filename = path.filename().string(); - std::string suffix = fmt::format("-{}", pid); - std::string container_name = - filename.size() > suffix.size() && - filename.compare(filename.size() - suffix.size(), suffix.size(), suffix) == 0 - ? filename.substr(0, filename.size() - suffix.size()) - : filename; - - sessions.push_back(SessionInfo{pid, container_name, running}); + sessions.push_back(SessionInfo{parsed->pid, parsed->container_name, running, path}); } return sessions; } + +std::vector clean_stale_sessions() { + std::vector removed; + + auto dir = session_run_dir(); + std::error_code dir_ec; + if (!std::filesystem::is_directory(dir, dir_ec)) { + return removed; + } + + std::error_code it_ec; + for (auto it = std::filesystem::directory_iterator(dir, it_ec); + !it_ec && it != std::filesystem::directory_iterator(); it.increment(it_ec)) { + const auto& path = it->path(); + auto parsed = open_session_file(path); + if (!parsed) { + continue; + } + + // Holding the same flock() used to test liveness across the removal + // itself (rather than reusing a separate, earlier list_sessions() result) + // keeps the check-then-remove atomic against a new session starting in + // between. + if (flock(parsed->fd, LOCK_EX | LOCK_NB) == 0) { + std::error_code rm_ec; + std::filesystem::remove(path, rm_ec); + if (rm_ec) { + spdlog::warn("failed to remove stale pid file {}: {}", path.string(), rm_ec.message()); + } else { + removed.push_back(SessionInfo{parsed->pid, parsed->container_name, false, path}); + } + flock(parsed->fd, LOCK_UN); + } + close(parsed->fd); + } + + return removed; +} diff --git a/src/pid_file.h b/src/pid_file.h index 50f8bee..1073b01 100644 --- a/src/pid_file.h +++ b/src/pid_file.h @@ -61,6 +61,7 @@ struct SessionInfo { pid_t pid; std::string container_name; // as recovered from the pid file's own name (sanitized) bool running; + std::filesystem::path path; }; // Scans $XDG_STATE_HOME/slocker-lite/run/ (see session_pid_file_path()) for pid @@ -75,3 +76,14 @@ struct SessionInfo { // that's inherent to scanning a live directory) is skipped, not reported as an // error. An empty or missing run directory yields an empty result, not an error. std::vector list_sessions(); + +// Scans the same run directory as list_sessions() and removes every pid file +// that's genuinely stale, returning the ones actually removed (running=false in +// each). Unlike calling list_sessions() and removing what it reports as not +// running, the stale check and the removal happen atomically per file (the same +// flock() acquired to test liveness is held across the removal itself), closing +// the race window where a new session could start between a separate check and a +// later removal. A file that can't be opened, doesn't parse as a pid, is still +// locked (still running), or fails to remove is left alone/skipped -- logged at +// most as a warning, never fatal; this is best-effort cleanup. +std::vector clean_stale_sessions();