From 6c85cf80d74f467f75f19164685c8e554eb0c469 Mon Sep 17 00:00:00 2001 From: Yuma Endo Date: Thu, 16 Jul 2026 18:18:51 +0900 Subject: [PATCH 1/3] test: add CLI end-to-end suite and real external-metadata provider Add a black-box end-to-end test target (scrap_e2e) that spawns the built scrap binary as a subprocess and asserts on argv -> exit code / stdout / stderr across the built-in, external, and project command paths. Implement MetadataProtocolProvider, a concrete ExternalMetadataProvider that probes ` --scrap-metadata` (falling back to ` --help`) via posix_spawn with a subprocess timeout, extracting a plain first-line description. Wire it into the composition root in place of the previous no-op provider and drop the now-unused NullMetadataProvider. Add direct unit tests for the provider (protocol, help fallback, timeout, missing/empty output). --- README.md | 6 + src/CMakeLists.txt | 2 +- src/command/MetadataProtocolProvider.cpp | 301 ++++++++++++ src/command/MetadataProtocolProvider.h | 49 ++ src/command/NullMetadataProvider.cpp | 20 - src/command/NullMetadataProvider.h | 31 -- src/main.cpp | 4 +- test/CMakeLists.txt | 28 ++ test/e2e/CliE2ETest.cpp | 458 ++++++++++++++++++ .../command/MetadataProtocolProviderTest.cpp | 169 +++++++ 10 files changed, 1014 insertions(+), 54 deletions(-) create mode 100644 src/command/MetadataProtocolProvider.cpp create mode 100644 src/command/MetadataProtocolProvider.h delete mode 100644 src/command/NullMetadataProvider.cpp delete mode 100644 src/command/NullMetadataProvider.h create mode 100644 test/e2e/CliE2ETest.cpp create mode 100644 test/unit/command/MetadataProtocolProviderTest.cpp diff --git a/README.md b/README.md index c06926b..fa0a775 100644 --- a/README.md +++ b/README.md @@ -127,6 +127,10 @@ scrap is in early alpha development (v0.0.1). Currently implemented: ✅ **Core Features** - CLI command framework (help, version, command discovery) +- External command metadata fetching - `scrap-*` executables are probed via + `--scrap-metadata` (falling back to `--help`) for a plain first-line + description shown in `scrap --help`; structured JSON/options metadata is + not yet part of the protocol 🚧 **In Progress** - Project creation (`scrap new`) - currently a placeholder command @@ -245,6 +249,7 @@ ctest --test-dir build/debug --output-on-failure --verbose # Run a specific test executable directly ./build/debug/test/scrap_gtest ./build/debug/test/scrap_gtest_cli11 +./build/debug/test/scrap_e2e # Release build testing cmake -S . -B build/release -DCMAKE_BUILD_TYPE=Release -DBUILD_TESTS=ON @@ -253,6 +258,7 @@ cmake --build build/release --target test **Test Structure:** - `test/unit/command/` - Unit tests for the command layer +- `test/e2e/` - End-to-end tests that spawn the built `scrap` binary as a subprocess ## 📊 Roadmap diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index b7c47c7..e843a57 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -22,6 +22,7 @@ target_sources(${PROJECT_NAME} PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/command/ExternalCommandResolver.cpp ${CMAKE_CURRENT_SOURCE_DIR}/command/ExternalMetadataProvider.cpp ${CMAKE_CURRENT_SOURCE_DIR}/command/HelpRenderer.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/command/MetadataProtocolProvider.cpp ${CMAKE_CURRENT_SOURCE_DIR}/command/ParserAdapter.cpp ${CMAKE_CURRENT_SOURCE_DIR}/command/ProjectCommandResolver.cpp ${CMAKE_CURRENT_SOURCE_DIR}/command/ScriptsReader.cpp @@ -29,7 +30,6 @@ target_sources(${PROJECT_NAME} PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/command/VersionRenderer.cpp ${CMAKE_CURRENT_SOURCE_DIR}/command/driver/CLI11ParserAdapter.cpp # Composition root helpers - ${CMAKE_CURRENT_SOURCE_DIR}/command/NullMetadataProvider.cpp ${CMAKE_CURRENT_SOURCE_DIR}/command/RuntimeEnvironmentFactory.cpp ) diff --git a/src/command/MetadataProtocolProvider.cpp b/src/command/MetadataProtocolProvider.cpp new file mode 100644 index 0000000..ead8225 --- /dev/null +++ b/src/command/MetadataProtocolProvider.cpp @@ -0,0 +1,301 @@ +#include "command/MetadataProtocolProvider.h" + +#include "command/ExternalMetadataProvider.h" + +#include +#include +#include +#include +#include +#include +#include +#include +// does not reliably resolve killpg()/SIGKILL for include-cleaner on +// all platforms; is the POSIX header that actually declares them. +// NOLINTNEXTLINE(hicpp-deprecated-headers,modernize-deprecated-headers) +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace scrap::Command { + +namespace { + +// --- Protocol constants ------------------------------------------------------- + +constexpr const char* ProtocolFlag = "--scrap-metadata"; +constexpr const char* HelpFlag = "--help"; + +// --- Subprocess capture tuning ------------------------------------------------- + +// Upper bound on captured stdout; a metadata description is a single short +// line, so this is generous headroom rather than an expected size. +constexpr std::size_t MaxCaptureBytes = 64UL * 1024UL; +constexpr std::size_t ReadChunkBytes = 4096; + +/** + * Result of running an executable once and capturing its stdout. + */ +struct SubprocessResult { + bool exitedNormally = false; + int exitCode = -1; + std::string capturedStdout; +}; + +/** + * Extract the first non-empty, whitespace-trimmed line from @p text. + * + * Returns an empty string if every line is blank (or @p text is empty). + */ +auto firstNonEmptyLine(std::string_view text) -> std::string +{ + std::size_t pos = 0; + while (pos <= text.size()) { + auto newlinePos = text.find('\n', pos); + auto lineEnd = (newlinePos == std::string_view::npos) ? text.size() : newlinePos; + auto line = text.substr(pos, lineEnd - pos); + + while (! line.empty() && (line.front() == ' ' || line.front() == '\t')) { + line.remove_prefix(1); + } + while (! line.empty() && (line.back() == ' ' || line.back() == '\t' || line.back() == '\r')) { + line.remove_suffix(1); + } + + if (! line.empty()) { + return std::string{line}; + } + if (newlinePos == std::string_view::npos) { + break; + } + pos = newlinePos + 1; + } + return {}; +} + +/** + * Mark @p fd close-on-exec so it never leaks into an unrelated child. + * + * Explicit dup2 targets set up via posix_spawn_file_actions are unaffected: + * dup2 always clears FD_CLOEXEC on the newly created descriptor. + */ +auto setCloseOnExec(int fd) -> bool +{ + auto flags = + ::fcntl(fd, F_GETFD); // NOLINT(hicpp-signed-bitwise) - POSIX fcntl(F_GETFD) result, not a flag combination + if (flags < 0) { + return false; + } + return ::fcntl(fd, F_SETFD, flags | FD_CLOEXEC) == 0; // NOLINT(hicpp-signed-bitwise) - POSIX fcntl flag API +} + +/** + * Drain @p readFd into @p out until EOF, the capture cap is hit, or + * @p deadline passes. Uses poll() so a full pipe can never deadlock the + * caller (the child keeps making progress as we keep reading). + * + * @return true if the deadline was reached before the child's stdout closed. + */ +auto drainUntilEofOrDeadline(int readFd, std::chrono::steady_clock::time_point deadline, std::string& out) -> bool +{ + std::array buffer{}; + + while (true) { + auto remaining = deadline - std::chrono::steady_clock::now(); + if (remaining <= std::chrono::steady_clock::duration::zero()) { + return true; + } + + // NOLINTNEXTLINE(misc-include-cleaner) - std::chrono::ceil is provided by + auto remainingMs = std::chrono::ceil(remaining).count(); + // NOLINTNEXTLINE(misc-include-cleaner) - struct pollfd is provided by + struct pollfd pfd { }; + pfd.fd = readFd; + pfd.events = POLLIN; // NOLINT(misc-include-cleaner) - POLLIN is provided by + + // NOLINTNEXTLINE(misc-include-cleaner) - poll() is provided by + const int pollResult = ::poll(&pfd, 1, static_cast(remainingMs)); + if (pollResult == 0) { + return true; // Timed out waiting for the next chunk. + } + if (pollResult < 0) { + if (errno == EINTR) { + continue; + } + return false; // Unexpected poll() failure: stop reading, not a timeout. + } + + // NOLINTNEXTLINE(hicpp-signed-bitwise, misc-include-cleaner) - POLLHUP is provided by + if ((pfd.revents & (POLLIN | POLLHUP)) == 0) { + // NOLINTNEXTLINE(hicpp-signed-bitwise, misc-include-cleaner) - POLLERR is provided by + if ((pfd.revents & POLLERR) != 0) { + return false; + } + continue; + } + + auto bytesRead = ::read(readFd, buffer.data(), buffer.size()); + if (bytesRead == 0) { + return false; // EOF: child closed stdout (normally because it exited). + } + if (bytesRead < 0) { + if (errno == EINTR || errno == EAGAIN) { + continue; + } + return false; // Unexpected read() failure: stop reading, not a timeout. + } + + if (out.size() >= MaxCaptureBytes) { + return false; // Capture cap reached; stop reading and let the caller reap. + } + auto available = MaxCaptureBytes - out.size(); + auto toAppend = std::min(static_cast(bytesRead), available); + out.append(buffer.data(), toAppend); + } +} + +/** + * Run @p executable with a single @p flag argument (no shell, argv-array + * only), capturing stdout while discarding stdin/stderr. The child runs in + * its own process group so the whole subtree can be killed on timeout. + * + * Every path below closes any fds it opened and, once posix_spawn has + * created a child, reaps it with waitpid — no fd leaks, no zombies. + */ +auto runOnce(const std::filesystem::path& executable, + const char* flag, + std::chrono::milliseconds timeout) -> SubprocessResult +{ + std::array pipeFds{-1, -1}; + if (::pipe(pipeFds.data()) != 0) { + return {}; + } + const int readFd = pipeFds[0]; + const int writeFd = pipeFds[1]; + + if (! setCloseOnExec(readFd) || ! setCloseOnExec(writeFd)) { + ::close(readFd); + ::close(writeFd); + return {}; + } + + posix_spawn_file_actions_t fileActions; + posix_spawn_file_actions_init(&fileActions); + posix_spawn_file_actions_addopen(&fileActions, STDIN_FILENO, "/dev/null", O_RDONLY, 0); + posix_spawn_file_actions_adddup2(&fileActions, writeFd, STDOUT_FILENO); + posix_spawn_file_actions_addopen(&fileActions, STDERR_FILENO, "/dev/null", O_WRONLY, 0); + // Explicit close of the read end for clarity; FD_CLOEXEC already closes + // both pipe fds at exec() time, so this is redundant-but-documented. + posix_spawn_file_actions_addclose(&fileActions, readFd); + + posix_spawnattr_t attr; + posix_spawnattr_init(&attr); + posix_spawnattr_setflags(&attr, POSIX_SPAWN_SETPGROUP); + posix_spawnattr_setpgroup(&attr, 0); // New, independent process group (pgid == child pid). + + const std::string exePath = executable.string(); + std::array argv{exePath.c_str(), flag, nullptr}; + // posix_spawn's argv parameter is char* const[] for historical POSIX + // reasons; the spawned process never mutates argv. + // NOLINTNEXTLINE(cppcoreguidelines-pro-type-const-cast) + auto** spawnArgv = const_cast(argv.data()); + + pid_t childPid = -1; + const int spawnStatus = ::posix_spawn(&childPid, exePath.c_str(), &fileActions, &attr, spawnArgv, environ); + + posix_spawn_file_actions_destroy(&fileActions); + posix_spawnattr_destroy(&attr); + + ::close(writeFd); // Parent's copy: read() below must see EOF once the child is done. + + if (spawnStatus != 0) { + ::close(readFd); + return {}; + } + + SubprocessResult result; + const auto deadline = std::chrono::steady_clock::now() + timeout; + const bool timedOut = drainUntilEofOrDeadline(readFd, deadline, result.capturedStdout); + ::close(readFd); + + if (timedOut) { + // Child is its own process group leader, so pgid == childPid. + // Any write it attempts after we've closed readFd yields SIGPIPE, + // which is harmless: we are about to kill and reap it regardless. + // NOLINTNEXTLINE(misc-include-cleaner) - killpg() and SIGKILL are provided by + ::killpg(childPid, SIGKILL); + } + + int status = 0; + // No do-while: waitpid() must be attempted at least once, then retried on EINTR. + pid_t waited = ::waitpid(childPid, &status, 0); + while (waited < 0 && errno == EINTR) { + waited = ::waitpid(childPid, &status, 0); + } + + // NOLINTNEXTLINE(misc-include-cleaner) - WIFEXITED/WEXITSTATUS are provided by + if (! timedOut && waited == childPid && WIFEXITED(status)) { + result.exitedNormally = true; + result.exitCode = WEXITSTATUS(status); // NOLINT(misc-include-cleaner) - provided by + } + + return result; +} + +/** + * Run @p executable with @p flag and return its first-line description, or + * an empty string if the attempt did not yield a usable result (non-zero + * exit, empty output, spawn failure, or timeout). + */ +auto probe(const std::filesystem::path& executable, const char* flag, std::chrono::milliseconds timeout) -> std::string +{ + auto result = runOnce(executable, flag, timeout); + if (! result.exitedNormally || result.exitCode != 0) { + return {}; + } + return firstNonEmptyLine(result.capturedStdout); +} + +} // namespace + +/** + * Construct with the subprocess timeout used for both probe attempts. + */ +MetadataProtocolProvider::MetadataProtocolProvider(std::chrono::milliseconds timeout) + : timeout_(timeout) +{ +} + +/** + * Fetch metadata via --scrap-metadata, falling back to --help. + * + * The executable path is canonicalized so posix_spawn always receives a + * path containing a slash (no PATH search, no shell, no injection surface). + * Only a plain-text first-line description is extracted; structured + * (name/options) metadata is not yet part of the protocol. + */ +auto MetadataProtocolProvider::fetch(const std::filesystem::path& executable) + -> std::expected +{ + std::error_code ec; + auto canonicalized = std::filesystem::weakly_canonical(executable, ec); + const std::filesystem::path& exe = ec ? executable : canonicalized; + + if (auto description = probe(exe, ProtocolFlag, timeout_); ! description.empty()) { + return ExternalCommandMetadata{.name = "", .description = std::move(description), .options = {}}; + } + + if (auto description = probe(exe, HelpFlag, timeout_); ! description.empty()) { + return ExternalCommandMetadata{.name = "", .description = std::move(description), .options = {}}; + } + + return std::unexpected("no usable metadata from '" + exe.string() + "' via --scrap-metadata or --help"); +} + +} // namespace scrap::Command diff --git a/src/command/MetadataProtocolProvider.h b/src/command/MetadataProtocolProvider.h new file mode 100644 index 0000000..3d594bf --- /dev/null +++ b/src/command/MetadataProtocolProvider.h @@ -0,0 +1,49 @@ +#pragma once + +#include "command/ExternalMetadataProvider.h" + +#include +#include +#include +#include + +namespace scrap::Command { + +/** + * @brief Fetches command metadata from an external executable via the + * `--scrap-metadata` protocol, falling back to `--help`. + * + * The executable is invoked as a subprocess (no shell, no PATH search — the + * path is canonicalized and passed directly to posix_spawn). Only the first + * non-empty line of stdout is used as a plain-text description; structured + * (name/options) metadata is not yet part of the protocol. + */ +class MetadataProtocolProvider final : public ExternalMetadataProvider { +public: + /** + * @brief Construct with an optional subprocess timeout. + * + * @param timeout Maximum time to wait for a probe subprocess before it + * is killed and treated as a failed attempt. Exposed so tests can pass + * a short value and keep runtime bounded. + */ + explicit MetadataProtocolProvider(std::chrono::milliseconds timeout = DefaultTimeout); + + /** + * @brief Fetch metadata by running ` --scrap-metadata`, + * falling back to ` --help` if that attempt fails. + * + * @param executable Path to the scrap-* executable. + * @return Metadata with a plain-text description on success, or an + * error message describing why metadata could not be fetched. + */ + [[nodiscard]] auto + fetch(const std::filesystem::path& executable) -> std::expected override; + +private: + static constexpr std::chrono::milliseconds DefaultTimeout{2000}; + + std::chrono::milliseconds timeout_; +}; + +} // namespace scrap::Command diff --git a/src/command/NullMetadataProvider.cpp b/src/command/NullMetadataProvider.cpp deleted file mode 100644 index 503d7ef..0000000 --- a/src/command/NullMetadataProvider.cpp +++ /dev/null @@ -1,20 +0,0 @@ -#include "command/NullMetadataProvider.h" - -#include "command/ExternalMetadataProvider.h" - -#include -#include - -namespace scrap::Command { - -/** - * Always return an error indicating the metadata protocol is not yet implemented. - */ -// NOLINTNEXTLINE(readability-convert-member-functions-to-static) — virtual override -auto NullMetadataProvider::fetch([[maybe_unused]] const std::filesystem::path& executable) - -> std::expected -{ - return std::unexpected(std::string{"external metadata protocol not yet implemented"}); -} - -} // namespace scrap::Command diff --git a/src/command/NullMetadataProvider.h b/src/command/NullMetadataProvider.h deleted file mode 100644 index 826beed..0000000 --- a/src/command/NullMetadataProvider.h +++ /dev/null @@ -1,31 +0,0 @@ -#pragma once - -#include "command/ExternalMetadataProvider.h" - -#include -#include -#include - -namespace scrap::Command { - -/** - * @brief Metadata provider stub used until the external metadata protocol lands. - * - * External commands are still discovered by ExternalCommandResolver via - * filesystem scanning; this provider simply reports that fetching rich - * metadata (name/description/options) is not yet supported. The real - * --scrap-metadata protocol is implemented in a later phase. - */ -class NullMetadataProvider final : public ExternalMetadataProvider { -public: - /** - * @brief Always report metadata fetching as unimplemented. - * - * @param executable Path to the scrap-* executable (unused). - * @return An error describing that the protocol is not yet implemented. - */ - [[nodiscard]] auto - fetch(const std::filesystem::path& executable) -> std::expected override; -}; - -} // namespace scrap::Command diff --git a/src/main.cpp b/src/main.cpp index 0988d3b..54b0d93 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -4,7 +4,7 @@ #include "command/DefaultVersionRenderer.h" #include "command/ExternalCommandResolver.h" #include "command/HelpRenderer.h" -#include "command/NullMetadataProvider.h" +#include "command/MetadataProtocolProvider.h" #include "command/ProjectCommandResolver.h" #include "command/RuntimeEnvironment.h" #include "command/RuntimeEnvironmentFactory.h" @@ -51,7 +51,7 @@ int main(int argc, char* argv[]) Application app(std::make_unique(), std::move(helpRenderer), std::move(versionRenderer)); app.addResolver(std::make_unique(helpRef, versionRef)); - app.addResolver(std::make_unique(std::make_unique())); + app.addResolver(std::make_unique(std::make_unique())); app.addResolver(std::make_unique(std::make_unique())); return app.run(std::span{argv, static_cast(argc)}, env); diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 007459d..cdb358b 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -8,6 +8,7 @@ add_executable(scrap_gtest unit/command/DefaultVersionRendererTest.cpp unit/command/BuiltinCommandResolverTest.cpp unit/command/ExternalCommandResolverTest.cpp + unit/command/MetadataProtocolProviderTest.cpp unit/command/ProjectCommandResolverTest.cpp unit/command/RuntimeEnvironmentFactoryTest.cpp unit/command/ApplicationTest.cpp @@ -24,6 +25,7 @@ add_executable(scrap_gtest ${CMAKE_SOURCE_DIR}/src/command/BuiltinCommandResolver.cpp ${CMAKE_SOURCE_DIR}/src/command/ExternalCommandResolver.cpp ${CMAKE_SOURCE_DIR}/src/command/ExternalMetadataProvider.cpp + ${CMAKE_SOURCE_DIR}/src/command/MetadataProtocolProvider.cpp ${CMAKE_SOURCE_DIR}/src/command/ProjectCommandResolver.cpp ${CMAKE_SOURCE_DIR}/src/command/ScriptsReader.cpp ${CMAKE_SOURCE_DIR}/src/command/StubScriptsReader.cpp @@ -84,3 +86,29 @@ set_target_properties(scrap_gtest_cli11 PROPERTIES ) gtest_discover_tests(scrap_gtest_cli11) + +# --- CLI end-to-end tests (spawns the built scrap binary as a subprocess) --- +add_executable(scrap_e2e + e2e/CliE2ETest.cpp +) + +target_compile_definitions(scrap_e2e PRIVATE SCRAP_BINARY_PATH="$") + +target_link_libraries(scrap_e2e PRIVATE GTest::gtest_main) +target_compile_features(scrap_e2e PUBLIC cxx_std_23) + +target_compile_options(scrap_e2e PRIVATE + -Wall -Wextra -Wpedantic + $<$:-O0 -g3> + $<$:-O3> +) + +set_target_properties(scrap_e2e PROPERTIES + RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/test + CXX_EXTENSIONS OFF +) + +# The e2e tests spawn the scrap binary itself, so it must be built first. +add_dependencies(scrap_e2e scrap) + +gtest_discover_tests(scrap_e2e) diff --git a/test/e2e/CliE2ETest.cpp b/test/e2e/CliE2ETest.cpp new file mode 100644 index 0000000..28fb302 --- /dev/null +++ b/test/e2e/CliE2ETest.cpp @@ -0,0 +1,458 @@ +// End-to-end tests that spawn the built `scrap` binary as a real subprocess +// and assert on its externally observable behavior (exit code, stdout, +// stderr). These are black-box tests: no scrap:: headers are used here. + +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#ifndef SCRAP_BINARY_PATH +#error "SCRAP_BINARY_PATH must be defined by the build (path to the scrap executable)" +#endif + +namespace { + +constexpr std::chrono::milliseconds HarnessTimeout{5000}; +constexpr std::size_t ReadChunkBytes = 4096; + +/** + * Result of running the scrap binary once. + */ +struct ProcessOutput { + bool exitedNormally = false; + int exitCode = -1; + std::string stdoutText; + std::string stderrText; +}; + +/** + * Remove leading/trailing ASCII whitespace from @p text. + */ +auto trim(std::string_view text) -> std::string +{ + std::size_t begin = 0; + while (begin < text.size() && (std::isspace(static_cast(text[begin])) != 0)) { + ++begin; + } + std::size_t end = text.size(); + while (end > begin && (std::isspace(static_cast(text[end - 1])) != 0)) { + --end; + } + return std::string{text.substr(begin, end - begin)}; +} + +/** + * Build the child's environment: a minimal, fixed base (SCRAP_HOME, PATH, + * LC_ALL) so external-command discovery only ever sees this test's fixture + * directory, plus any caller-supplied "KEY=VALUE" overrides. + */ +auto buildEnv(const std::filesystem::path& scrapHome, + const std::vector& overrides) -> std::vector +{ + std::vector env{ + "SCRAP_HOME=" + scrapHome.string(), + "PATH=/usr/bin:/bin", + "LC_ALL=C", + }; + + for (const auto& entry : overrides) { + auto eq = entry.find('='); + if (eq == std::string::npos) { + continue; + } + auto key = entry.substr(0, eq); + + bool replaced = false; + for (auto& existing : env) { + auto existingEq = existing.find('='); + if (existingEq != std::string::npos && existing.compare(0, existingEq, key) == 0) { + existing = entry; + replaced = true; + break; + } + } + if (! replaced) { + env.push_back(entry); + } + } + + return env; +} + +/** + * Drain both @p fd1 and @p fd2 into @p out1 / @p out2 until each reaches + * EOF or @p deadline passes. Polling both fds together avoids the deadlock + * that reading them sequentially could cause if the child fills one pipe + * while blocked writing to the other. + * + * @return true if @p deadline was reached before both fds closed. + */ +auto drainBoth(int fd1, int fd2, std::chrono::steady_clock::time_point deadline, std::string& out1, std::string& out2) + -> bool +{ + std::array buffer{}; + bool open1 = true; + bool open2 = true; + + while (open1 || open2) { + auto remaining = deadline - std::chrono::steady_clock::now(); + if (remaining <= std::chrono::steady_clock::duration::zero()) { + return true; + } + auto remainingMs = std::chrono::ceil(remaining).count(); + + std::array pfds{}; + int count = 0; + int idx1 = -1; + int idx2 = -1; + if (open1) { + idx1 = count; + pfds[static_cast(count)] = {.fd = fd1, .events = POLLIN, .revents = 0}; + ++count; + } + if (open2) { + idx2 = count; + pfds[static_cast(count)] = {.fd = fd2, .events = POLLIN, .revents = 0}; + ++count; + } + + int pollResult = ::poll(pfds.data(), static_cast(count), static_cast(remainingMs)); + if (pollResult == 0) { + return true; + } + if (pollResult < 0) { + if (errno == EINTR) { + continue; + } + return false; + } + + // NOLINTNEXTLINE(hicpp-signed-bitwise) - POSIX poll() revents flag combination + if (idx1 >= 0 && (pfds[static_cast(idx1)].revents & (POLLIN | POLLHUP)) != 0) { + auto bytesRead = ::read(fd1, buffer.data(), buffer.size()); + if (bytesRead == 0) { + open1 = false; + } else if (bytesRead < 0) { + if (errno != EINTR && errno != EAGAIN) { + open1 = false; + } + } else { + out1.append(buffer.data(), static_cast(bytesRead)); + } + } + // NOLINTNEXTLINE(hicpp-signed-bitwise) - POSIX poll() revents flag combination + if (idx2 >= 0 && (pfds[static_cast(idx2)].revents & (POLLIN | POLLHUP)) != 0) { + auto bytesRead = ::read(fd2, buffer.data(), buffer.size()); + if (bytesRead == 0) { + open2 = false; + } else if (bytesRead < 0) { + if (errno != EINTR && errno != EAGAIN) { + open2 = false; + } + } else { + out2.append(buffer.data(), static_cast(bytesRead)); + } + } + } + + return false; +} + +} // namespace + +/** + * Fixture providing a per-test temp directory (with a bin/ subdirectory for + * dummy external commands) and a helper to run the built scrap binary. + */ +class CliE2ETest : public ::testing::Test { +protected: + /** + * Create a per-test temp directory. Each test case runs as its own + * ctest entry and ctest may run them in parallel, so the directory + * name must be unique per test case (and per process, for reruns). + */ + void SetUp() override + { + const auto* info = ::testing::UnitTest::GetInstance()->current_test_info(); + root_ = std::filesystem::temp_directory_path() / + (std::string("scrap_e2e_") + info->name() + "_" + std::to_string(::getpid())); + std::filesystem::remove_all(root_); + std::filesystem::create_directories(root_ / "bin"); + } + + /** + * Clean up the temp directory. + */ + void TearDown() override + { + std::filesystem::remove_all(root_); + } + + /** + * Write an executable dummy script named @p name into the fixture's + * bin/ directory, with @p body as the shell script content. + */ + void makeDummy(const std::string& name, const std::string& body) const + { + auto path = root_ / "bin" / name; + { + std::ofstream out(path); + out << "#!/bin/sh\n" << body << "\n"; + } + std::filesystem::permissions(path, std::filesystem::perms::owner_exec, std::filesystem::perm_options::add); + } + + /** + * Run the built scrap binary with @p args, a minimal controlled + * environment (SCRAP_HOME=root_, plus any @p envOverrides), and the + * given @p cwd. Captures stdout/stderr separately. + */ + [[nodiscard]] auto runScrap(const std::vector& args, + const std::vector& envOverrides, + const std::filesystem::path& cwd) const -> ProcessOutput + { + std::array outPipe{-1, -1}; + std::array errPipe{-1, -1}; + if (::pipe(outPipe.data()) != 0) { + return {}; + } + if (::pipe(errPipe.data()) != 0) { + ::close(outPipe[0]); + ::close(outPipe[1]); + return {}; + } + + std::vector ownedArgs; + ownedArgs.reserve(args.size() + 1); + ownedArgs.emplace_back(SCRAP_BINARY_PATH); + for (const auto& arg : args) { + ownedArgs.push_back(arg); + } + std::vector argv; + argv.reserve(ownedArgs.size() + 1); + for (auto& arg : ownedArgs) { + argv.push_back(arg.data()); + } + argv.push_back(nullptr); + + auto ownedEnv = buildEnv(root_, envOverrides); + std::vector envp; + envp.reserve(ownedEnv.size() + 1); + for (auto& entry : ownedEnv) { + envp.push_back(entry.data()); + } + envp.push_back(nullptr); + + const std::string cwdStr = cwd.string(); + + pid_t childPid = ::fork(); + if (childPid < 0) { + ::close(outPipe[0]); + ::close(outPipe[1]); + ::close(errPipe[0]); + ::close(errPipe[1]); + return {}; + } + + if (childPid == 0) { + // Child: wire up stdout/stderr, isolate stdin, chdir, then exec. + ::dup2(outPipe[1], STDOUT_FILENO); + ::dup2(errPipe[1], STDERR_FILENO); + ::close(outPipe[0]); + ::close(outPipe[1]); + ::close(errPipe[0]); + ::close(errPipe[1]); + + int devNull = ::open("/dev/null", O_RDONLY); + if (devNull >= 0) { + ::dup2(devNull, STDIN_FILENO); + ::close(devNull); + } + + if (::chdir(cwdStr.c_str()) != 0) { + _exit(127); + } + + ::execve(argv[0], argv.data(), envp.data()); + _exit(127); // execve() only returns on failure. + } + + // Parent: close the write ends so EOF is observed once the child exits. + ::close(outPipe[1]); + ::close(errPipe[1]); + + ProcessOutput result; + auto deadline = std::chrono::steady_clock::now() + HarnessTimeout; + bool timedOut = drainBoth(outPipe[0], errPipe[0], deadline, result.stdoutText, result.stderrText); + ::close(outPipe[0]); + ::close(errPipe[0]); + + if (timedOut) { + ::kill(childPid, SIGKILL); + } + + int status = 0; + pid_t waited = -1; + do { + waited = ::waitpid(childPid, &status, 0); + } while (waited < 0 && errno == EINTR); + + if (! timedOut && waited == childPid && WIFEXITED(status)) { + result.exitedNormally = true; + result.exitCode = WEXITSTATUS(status); + } + + return result; + } + + std::filesystem::path root_; +}; + +// --- TS-01: builtin resolve + execute ------------------------------------------ + +TEST_F(CliE2ETest, BuiltinResolveExecute) +{ + auto result = runScrap({"version"}, {}, root_); + + ASSERT_TRUE(result.exitedNormally); + EXPECT_EQ(result.exitCode, 0); + EXPECT_EQ(trim(result.stdoutText), "scrap 0.0.1"); + EXPECT_TRUE(result.stderrText.empty()); +} + +// --- TS-02: external command discovery ----------------------------------------- + +TEST_F(CliE2ETest, ExternalDiscovery) +{ + makeDummy("scrap-greet", R"(case "$1" in + --scrap-metadata) echo "Greet people" ;; + --help) echo "greet - Greet people" ;; + *) echo "greet called" ;; +esac)"); + + auto result = runScrap({"--help"}, {}, root_); + + ASSERT_TRUE(result.exitedNormally); + EXPECT_EQ(result.exitCode, 0); + EXPECT_NE(result.stdoutText.find("External Commands"), std::string::npos); + EXPECT_NE(result.stdoutText.find("greet"), std::string::npos); +} + +// --- TS-03: project scope with no scrap.toml (honest scope: graceful only) ---- + +TEST_F(CliE2ETest, ProjectScopeNoConfig) +{ + // No scrap.toml is created in root_. StubScriptsReader always returns an + // empty script list regardless of project contents, so this only proves + // a config-less project run does not crash and still lists builtins — + // it does NOT exercise dynamic [scripts] parsing. That is covered by the + // unit-level ProjectCommandResolverTest against a real ScriptsReader. + auto result = runScrap({"--help"}, {}, root_); + + ASSERT_TRUE(result.exitedNormally); + EXPECT_EQ(result.exitCode, 0); + EXPECT_NE(result.stdoutText.find("Built-in Commands"), std::string::npos); + EXPECT_EQ(result.stdoutText.find("totally-fake-project-script"), std::string::npos); +} + +// --- TS-04: builtin vs external name collision priority ------------------------ + +TEST_F(CliE2ETest, Priority) +{ + makeDummy("scrap-greet", "echo \"greet called\""); + makeDummy("scrap-build", "echo \"external build\""); + + auto result = runScrap({"build"}, {}, root_); + + ASSERT_TRUE(result.exitedNormally); + EXPECT_EQ(result.exitCode, 0); + EXPECT_NE(result.stdoutText.find("build: not yet implemented"), std::string::npos); + EXPECT_NE(result.stderrText.find("warning: command 'build' already registered; ignoring duplicate"), + std::string::npos); +} + +// --- TS-05: global help integration --------------------------------------------- + +TEST_F(CliE2ETest, HelpIntegration) +{ + auto result = runScrap({"--help"}, {}, root_); + + ASSERT_TRUE(result.exitedNormally); + EXPECT_EQ(result.exitCode, 0); + for (const auto* expected : {"USAGE: scrap", + "Built-in Commands", + "Project Commands", + "Toolchain Commands", + "Template Commands", + "See 'scrap help '"}) { + EXPECT_NE(result.stdoutText.find(expected), std::string::npos) << "missing: " << expected; + } +} + +// --- TS-06: unknown command ------------------------------------------------------- + +TEST_F(CliE2ETest, UnknownCommand) +{ + auto result = runScrap({"nonexistent"}, {}, root_); + + ASSERT_TRUE(result.exitedNormally); + EXPECT_EQ(result.exitCode, 1); + EXPECT_NE(result.stderrText.find("Run 'scrap --help' for usage information"), std::string::npos); +} + +// --- TS-07: real CLI11 nested subcommand parsing -------------------------------- + +TEST_F(CliE2ETest, RealCli11Nested) +{ + auto result = runScrap({"toolchain", "install"}, {}, root_); + + ASSERT_TRUE(result.exitedNormally); + EXPECT_EQ(result.exitCode, 0); + EXPECT_NE(result.stdoutText.find("install: not yet implemented"), std::string::npos); +} + +// --- TS-08: metadata actually fetched via --scrap-metadata ---------------------- + +TEST_F(CliE2ETest, MetadataFetch) +{ + makeDummy("scrap-greet", R"(case "$1" in + --scrap-metadata) echo "Greet people" ;; + --help) echo "greet - Greet people" ;; + *) echo "greet called" ;; +esac)"); + + auto result = runScrap({"--help"}, {}, root_); + + ASSERT_TRUE(result.exitedNormally); + EXPECT_EQ(result.exitCode, 0); + EXPECT_NE(result.stdoutText.find("greet"), std::string::npos); + EXPECT_NE(result.stdoutText.find("Greet people"), std::string::npos); +} + +// --- TS-09: all version forms agree ---------------------------------------------- + +TEST_F(CliE2ETest, VersionForms) +{ + for (const auto& args : + {std::vector{"version"}, std::vector{"--version"}, std::vector{"-V"}}) { + auto result = runScrap(args, {}, root_); + + ASSERT_TRUE(result.exitedNormally); + EXPECT_EQ(result.exitCode, 0); + EXPECT_EQ(trim(result.stdoutText), "scrap 0.0.1"); + } +} diff --git a/test/unit/command/MetadataProtocolProviderTest.cpp b/test/unit/command/MetadataProtocolProviderTest.cpp new file mode 100644 index 0000000..de8dad8 --- /dev/null +++ b/test/unit/command/MetadataProtocolProviderTest.cpp @@ -0,0 +1,169 @@ +#include + +#include "command/MetadataProtocolProvider.h" + +#include +#include +#include +#include + +using namespace scrap::Command; + +namespace { + +/** + * Timeout used by tests that expect a fast, successful (or cleanly failed) + * probe. Short enough to keep the suite fast, long enough to never be + * mistaken for the deliberately short timeout used by the Timeout test. + */ +constexpr std::chrono::milliseconds FastTimeout{2000}; + +} // namespace + +/** + * Test fixture providing a temp directory for provider tests. + */ +class MetadataProtocolProviderTest : public ::testing::Test { +protected: + /** + * Create a per-test temp directory. + * + * Each test case runs as its own ctest entry and ctest may run them + * in parallel, so the directory name must be unique per test case. + */ + void SetUp() override + { + const auto* info = ::testing::UnitTest::GetInstance()->current_test_info(); + tempDir_ = std::filesystem::temp_directory_path() / (std::string("scrap_provider_test_") + info->name()); + std::filesystem::remove_all(tempDir_); + std::filesystem::create_directories(tempDir_); + } + + /** + * Clean up the temp directory. + */ + void TearDown() override + { + std::filesystem::remove_all(tempDir_); + } + + /** + * Create a shell script with execute permission. + */ + auto createExecutable(const std::string& name, const std::string& body) -> std::filesystem::path + { + auto path = tempDir_ / name; + std::ofstream(path) << "#!/bin/sh\n" << body << "\n"; + std::filesystem::permissions(path, std::filesystem::perms::owner_exec, std::filesystem::perm_options::add); + return path; + } + + std::filesystem::path tempDir_; +}; + +/** + * Verify that a description is fetched from --scrap-metadata output. + */ +TEST_F(MetadataProtocolProviderTest, MetadataViaProtocol) +{ + auto script = createExecutable("scrap-x", R"(if [ "$1" = "--scrap-metadata" ]; then + echo "Hello desc" + exit 0 +fi +exit 1)"); + + MetadataProtocolProvider provider(FastTimeout); + auto result = provider.fetch(script); + + ASSERT_TRUE(result.has_value()); + EXPECT_EQ(result->description, "Hello desc"); + EXPECT_TRUE(result->name.empty()); +} + +/** + * Verify that --help output is used when --scrap-metadata fails. + */ +TEST_F(MetadataProtocolProviderTest, FallbackToHelp) +{ + auto script = createExecutable("scrap-x", R"(if [ "$1" = "--scrap-metadata" ]; then + exit 1 +fi +if [ "$1" = "--help" ]; then + echo "Help line" + exit 0 +fi +exit 1)"); + + MetadataProtocolProvider provider(FastTimeout); + auto result = provider.fetch(script); + + ASSERT_TRUE(result.has_value()); + EXPECT_EQ(result->description, "Help line"); +} + +/** + * Verify that failure of both --scrap-metadata and --help yields an error. + */ +TEST_F(MetadataProtocolProviderTest, BothFailUnexpected) +{ + auto script = createExecutable("scrap-x", "exit 1"); + + MetadataProtocolProvider provider(FastTimeout); + auto result = provider.fetch(script); + + EXPECT_FALSE(result.has_value()); +} + +/** + * Verify that a hung child is killed and fetch() fails within the injected + * timeout, rather than blocking for the child's full runtime. + */ +TEST_F(MetadataProtocolProviderTest, Timeout) +{ + auto script = createExecutable("scrap-x", "sleep 10"); + + constexpr std::chrono::milliseconds shortTimeout{200}; + MetadataProtocolProvider provider(shortTimeout); + + auto start = std::chrono::steady_clock::now(); + auto result = provider.fetch(script); + auto elapsed = std::chrono::steady_clock::now() - start; + + EXPECT_FALSE(result.has_value()); + // Two probe attempts (--scrap-metadata, --help) each bounded by + // shortTimeout; generous upper bound keeps this robust under CI load + // while still proving we did not wait anywhere near the 10s sleep. + EXPECT_LT(elapsed, std::chrono::seconds(5)); +} + +/** + * Verify that a missing or non-executable path fails cleanly (no crash). + */ +TEST_F(MetadataProtocolProviderTest, NonExecutableOrMissing) +{ + MetadataProtocolProvider provider(FastTimeout); + + auto missingResult = provider.fetch(tempDir_ / "does-not-exist"); + EXPECT_FALSE(missingResult.has_value()); + + auto nonExecPath = tempDir_ / "scrap-not-exec"; + std::ofstream(nonExecPath) << "#!/bin/sh\necho unreachable\n"; + auto nonExecResult = provider.fetch(nonExecPath); + EXPECT_FALSE(nonExecResult.has_value()); +} + +/** + * Verify that exit-0-but-empty output from both attempts yields an error. + */ +TEST_F(MetadataProtocolProviderTest, EmptyOutput) +{ + auto script = createExecutable("scrap-x", R"(if [ "$1" = "--scrap-metadata" ]; then + exit 0 +fi +exit 1)"); + + MetadataProtocolProvider provider(FastTimeout); + auto result = provider.fetch(script); + + EXPECT_FALSE(result.has_value()); +} From f1d9a96c9fdafc7108e00b0b71048e1b7350a00b Mon Sep 17 00:00:00 2001 From: Yuma Endo Date: Thu, 16 Jul 2026 18:42:57 +0900 Subject: [PATCH 2/3] fix: bound external-metadata subprocess reap and fix macOS build Guarantee the metadata probe's timeout by unconditionally SIGKILLing the child's process group before the blocking waitpid, instead of only on the drain-timeout path. Capture-cap, EOF-while-still-running (e.g. a child that runs `exec 1>&-; sleep`), and drain errors previously fell through to an unbounded waitpid, letting a single misbehaving scrap-* freeze `scrap --help`. Killing an already-exited child is a harmless no-op on its zombie, so the happy-path exit status is preserved and WIFEXITED alone distinguishes a normal exit from a forced kill. Declare environ via /_NSGetEnviron() on Apple, where does not provide it, so the file builds on macOS. Wrap the pipe descriptors in a small RAII UniqueFd so every exit path closes them exactly once. Add regression tests for the capture-cap and closed-stdout hang variants (both must return within the injected timeout). Isolate the end-to-end metadata test to the --scrap-metadata path with a disjoint marker, create the e2e fixture dir atomically with mkdtemp, and label the e2e tests. --- README.md | 2 +- src/command/MetadataProtocolProvider.cpp | 153 ++++++++++++++---- test/CMakeLists.txt | 2 +- test/e2e/CliE2ETest.cpp | 25 ++- .../command/MetadataProtocolProviderTest.cpp | 57 +++++++ 5 files changed, 196 insertions(+), 43 deletions(-) diff --git a/README.md b/README.md index fa0a775..f8d540a 100644 --- a/README.md +++ b/README.md @@ -237,7 +237,7 @@ ctest --test-dir build/debug --output-on-failure ### Testing -The project uses GoogleTest for unit testing, run through ctest. Tests are automatically built when `BUILD_TESTS=ON`. +The project uses GoogleTest for unit and end-to-end testing, run through ctest. Tests are automatically built when `BUILD_TESTS=ON`. ```bash # Build and run all tests diff --git a/src/command/MetadataProtocolProvider.cpp b/src/command/MetadataProtocolProvider.cpp index ead8225..e8af56b 100644 --- a/src/command/MetadataProtocolProvider.cpp +++ b/src/command/MetadataProtocolProvider.cpp @@ -23,6 +23,12 @@ #include #include +#if defined(__APPLE__) +// NOLINTNEXTLINE(misc-include-cleaner) - only pulled in on Darwin, guarded by __APPLE__ +#include // _NSGetEnviron(): the only correct way to reach environ on Darwin +#define environ (*_NSGetEnviron()) +#endif + namespace scrap::Command { namespace { @@ -96,20 +102,83 @@ auto setCloseOnExec(int fd) -> bool } /** - * Drain @p readFd into @p out until EOF, the capture cap is hit, or - * @p deadline passes. Uses poll() so a full pipe can never deadlock the - * caller (the child keeps making progress as we keep reading). + * RAII wrapper around a POSIX file descriptor. Move-only; closes a valid + * (>= 0) descriptor in its destructor so every exit path out of runOnce() + * — including one taken because of an exception thrown between pipe() + * creation and the descriptor's last explicit use — closes it exactly once. + */ +class UniqueFd { +public: + UniqueFd() = default; + explicit UniqueFd(int fd) + : fd_(fd) + { + } + + UniqueFd(const UniqueFd&) = delete; + auto operator=(const UniqueFd&) -> UniqueFd& = delete; + + UniqueFd(UniqueFd&& other) noexcept + : fd_(other.release()) + { + } + auto operator=(UniqueFd&& other) noexcept -> UniqueFd& + { + if (this != &other) { + reset(other.release()); + } + return *this; + } + + ~UniqueFd() + { + reset(); + } + + [[nodiscard]] auto get() const -> int + { + return fd_; + } + + /** Relinquish ownership, returning the raw descriptor without closing it. */ + [[nodiscard]] auto release() -> int + { + auto fd = fd_; + fd_ = -1; + return fd; + } + + /** Close the current descriptor (if any) and take ownership of @p fd. */ + void reset(int fd = -1) + { + if (fd_ >= 0) { + ::close(fd_); + } + fd_ = fd; + } + +private: + int fd_ = -1; +}; + +/** + * Drain @p readFd into @p out until EOF, the capture cap is hit, the + * deadline passes, or an unexpected poll()/read() error occurs. Uses + * poll() so a full pipe can never deadlock the caller (the child keeps + * making progress as we keep reading). * - * @return true if the deadline was reached before the child's stdout closed. + * The caller (runOnce) always kills and reaps the child once this returns, + * regardless of which of the above reasons stopped the drain, so no return + * value is needed to single out "timed out" from the others. */ -auto drainUntilEofOrDeadline(int readFd, std::chrono::steady_clock::time_point deadline, std::string& out) -> bool +auto drainUntilEofOrDeadline(int readFd, std::chrono::steady_clock::time_point deadline, std::string& out) -> void { std::array buffer{}; while (true) { auto remaining = deadline - std::chrono::steady_clock::now(); if (remaining <= std::chrono::steady_clock::duration::zero()) { - return true; + return; // Deadline reached; the caller kills and reaps the child unconditionally. } // NOLINTNEXTLINE(misc-include-cleaner) - std::chrono::ceil is provided by @@ -122,37 +191,40 @@ auto drainUntilEofOrDeadline(int readFd, std::chrono::steady_clock::time_point d // NOLINTNEXTLINE(misc-include-cleaner) - poll() is provided by const int pollResult = ::poll(&pfd, 1, static_cast(remainingMs)); if (pollResult == 0) { - return true; // Timed out waiting for the next chunk. + return; // Timed out waiting for the next chunk. } if (pollResult < 0) { if (errno == EINTR) { continue; } - return false; // Unexpected poll() failure: stop reading, not a timeout. + return; // Unexpected poll() failure: stop reading. } // NOLINTNEXTLINE(hicpp-signed-bitwise, misc-include-cleaner) - POLLHUP is provided by if ((pfd.revents & (POLLIN | POLLHUP)) == 0) { // NOLINTNEXTLINE(hicpp-signed-bitwise, misc-include-cleaner) - POLLERR is provided by if ((pfd.revents & POLLERR) != 0) { - return false; + return; } continue; } auto bytesRead = ::read(readFd, buffer.data(), buffer.size()); if (bytesRead == 0) { - return false; // EOF: child closed stdout (normally because it exited). + // EOF: the child closed its stdout. It may have exited already, + // or it may still be running (e.g. `exec 1>&-; sleep ...`) — the + // caller's unconditional kill-then-reap handles both uniformly. + return; } if (bytesRead < 0) { if (errno == EINTR || errno == EAGAIN) { continue; } - return false; // Unexpected read() failure: stop reading, not a timeout. + return; // Unexpected read() failure: stop reading. } if (out.size() >= MaxCaptureBytes) { - return false; // Capture cap reached; stop reading and let the caller reap. + return; // Capture cap reached; stop reading and let the caller reap. } auto available = MaxCaptureBytes - out.size(); auto toAppend = std::min(static_cast(bytesRead), available); @@ -163,10 +235,25 @@ auto drainUntilEofOrDeadline(int readFd, std::chrono::steady_clock::time_point d /** * Run @p executable with a single @p flag argument (no shell, argv-array * only), capturing stdout while discarding stdin/stderr. The child runs in - * its own process group so the whole subtree can be killed on timeout. + * its own process group. * - * Every path below closes any fds it opened and, once posix_spawn has - * created a child, reaps it with waitpid — no fd leaks, no zombies. + * The process group is unconditionally SIGKILLed right before the final + * (blocking) waitpid(), regardless of why draining its stdout stopped. If + * the child already exited (the common case: it wrote its output and + * closed stdout), it is a zombie and the kill is a harmless no-op — the + * zombie's exit status is preserved, so waitpid() still returns the real + * WIFEXITED/WEXITSTATUS. If the child is still alive (deadline reached, + * capture cap hit, or a poll/read error while it keeps running), the kill + * bounds the reap so waitpid() cannot block for the child's full lifetime. + * Either way WIFEXITED(status) below naturally distinguishes "exited + * normally" from "we had to kill it" — no separate flag is needed. killpg + * (not kill) also reaps any grandchildren the probe spawned; killing + * before waitpid (not after) avoids a pid/pgid-reuse race, since the + * still-unreaped group leader keeps pgid valid and unique. + * + * Every path below closes any fds it opened (via UniqueFd's RAII) and, once + * posix_spawn has created a child, reaps it with waitpid — no fd leaks, no + * zombies. */ auto runOnce(const std::filesystem::path& executable, const char* flag, @@ -176,23 +263,21 @@ auto runOnce(const std::filesystem::path& executable, if (::pipe(pipeFds.data()) != 0) { return {}; } - const int readFd = pipeFds[0]; - const int writeFd = pipeFds[1]; + UniqueFd readFd{pipeFds[0]}; + UniqueFd writeFd{pipeFds[1]}; - if (! setCloseOnExec(readFd) || ! setCloseOnExec(writeFd)) { - ::close(readFd); - ::close(writeFd); + if (! setCloseOnExec(readFd.get()) || ! setCloseOnExec(writeFd.get())) { return {}; } posix_spawn_file_actions_t fileActions; posix_spawn_file_actions_init(&fileActions); posix_spawn_file_actions_addopen(&fileActions, STDIN_FILENO, "/dev/null", O_RDONLY, 0); - posix_spawn_file_actions_adddup2(&fileActions, writeFd, STDOUT_FILENO); + posix_spawn_file_actions_adddup2(&fileActions, writeFd.get(), STDOUT_FILENO); posix_spawn_file_actions_addopen(&fileActions, STDERR_FILENO, "/dev/null", O_WRONLY, 0); // Explicit close of the read end for clarity; FD_CLOEXEC already closes // both pipe fds at exec() time, so this is redundant-but-documented. - posix_spawn_file_actions_addclose(&fileActions, readFd); + posix_spawn_file_actions_addclose(&fileActions, readFd.get()); posix_spawnattr_t attr; posix_spawnattr_init(&attr); @@ -212,25 +297,23 @@ auto runOnce(const std::filesystem::path& executable, posix_spawn_file_actions_destroy(&fileActions); posix_spawnattr_destroy(&attr); - ::close(writeFd); // Parent's copy: read() below must see EOF once the child is done. + writeFd.reset(); // Parent's copy: read() below must see EOF once the child is done. if (spawnStatus != 0) { - ::close(readFd); return {}; } SubprocessResult result; const auto deadline = std::chrono::steady_clock::now() + timeout; - const bool timedOut = drainUntilEofOrDeadline(readFd, deadline, result.capturedStdout); - ::close(readFd); - - if (timedOut) { - // Child is its own process group leader, so pgid == childPid. - // Any write it attempts after we've closed readFd yields SIGPIPE, - // which is harmless: we are about to kill and reap it regardless. - // NOLINTNEXTLINE(misc-include-cleaner) - killpg() and SIGKILL are provided by - ::killpg(childPid, SIGKILL); - } + drainUntilEofOrDeadline(readFd.get(), deadline, result.capturedStdout); + readFd.reset(); + + // Kill unconditionally (see the doc comment above for why this is safe + // for an already-exited child too). It is its own process group + // leader, so pgid == childPid; any write it attempts after we've + // closed readFd yields a harmless SIGPIPE, since we are about to kill it. + // NOLINTNEXTLINE(misc-include-cleaner) - killpg() and SIGKILL are provided by + ::killpg(childPid, SIGKILL); int status = 0; // No do-while: waitpid() must be attempted at least once, then retried on EINTR. @@ -240,7 +323,7 @@ auto runOnce(const std::filesystem::path& executable, } // NOLINTNEXTLINE(misc-include-cleaner) - WIFEXITED/WEXITSTATUS are provided by - if (! timedOut && waited == childPid && WIFEXITED(status)) { + if (waited == childPid && WIFEXITED(status)) { result.exitedNormally = true; result.exitCode = WEXITSTATUS(status); // NOLINT(misc-include-cleaner) - provided by } diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index cdb358b..8e071b4 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -111,4 +111,4 @@ set_target_properties(scrap_e2e PROPERTIES # The e2e tests spawn the scrap binary itself, so it must be built first. add_dependencies(scrap_e2e scrap) -gtest_discover_tests(scrap_e2e) +gtest_discover_tests(scrap_e2e PROPERTIES LABELS e2e) diff --git a/test/e2e/CliE2ETest.cpp b/test/e2e/CliE2ETest.cpp index 28fb302..0faa58f 100644 --- a/test/e2e/CliE2ETest.cpp +++ b/test/e2e/CliE2ETest.cpp @@ -10,6 +10,8 @@ #include #include #include +#include +#include #include #include #include @@ -188,9 +190,14 @@ class CliE2ETest : public ::testing::Test { void SetUp() override { const auto* info = ::testing::UnitTest::GetInstance()->current_test_info(); - root_ = std::filesystem::temp_directory_path() / - (std::string("scrap_e2e_") + info->name() + "_" + std::to_string(::getpid())); - std::filesystem::remove_all(root_); + auto dirTemplate = + (std::filesystem::temp_directory_path() / (std::string("scrap_e2e_") + info->name() + "_XXXXXX")).string(); + // mkdtemp(3) atomically creates a uniquely-named directory in place + // of the trailing "XXXXXX", removing the /tmp symlink-preplacement + // race inherent in "pick a name, then create_directories(name)". + const char* created = ::mkdtemp(dirTemplate.data()); + ASSERT_NE(created, nullptr) << "mkdtemp failed: " << std::strerror(errno); + root_ = std::filesystem::path(created); std::filesystem::create_directories(root_ / "bin"); } @@ -429,9 +436,15 @@ TEST_F(CliE2ETest, RealCli11Nested) TEST_F(CliE2ETest, MetadataFetch) { + // The --scrap-metadata and --help responses are deliberately disjoint + // (unlike TS-02's dummy, where --help's text is a superstring of + // --scrap-metadata's): if metadata fetching silently fell back to + // --help instead of using --scrap-metadata, this dummy would still make + // "greet" appear in the output, but the --scrap-metadata-only marker + // below would not. makeDummy("scrap-greet", R"(case "$1" in - --scrap-metadata) echo "Greet people" ;; - --help) echo "greet - Greet people" ;; + --scrap-metadata) echo "metadata-greets-you" ;; + --help) echo "greet help text" ;; *) echo "greet called" ;; esac)"); @@ -440,7 +453,7 @@ esac)"); ASSERT_TRUE(result.exitedNormally); EXPECT_EQ(result.exitCode, 0); EXPECT_NE(result.stdoutText.find("greet"), std::string::npos); - EXPECT_NE(result.stdoutText.find("Greet people"), std::string::npos); + EXPECT_NE(result.stdoutText.find("metadata-greets-you"), std::string::npos); } // --- TS-09: all version forms agree ---------------------------------------------- diff --git a/test/unit/command/MetadataProtocolProviderTest.cpp b/test/unit/command/MetadataProtocolProviderTest.cpp index de8dad8..d55c1ad 100644 --- a/test/unit/command/MetadataProtocolProviderTest.cpp +++ b/test/unit/command/MetadataProtocolProviderTest.cpp @@ -136,6 +136,63 @@ TEST_F(MetadataProtocolProviderTest, Timeout) EXPECT_LT(elapsed, std::chrono::seconds(5)); } +/** + * Verify that a child which emits more than the capture cap and then hangs + * is still killed and reaped promptly. Regression for a bug where the kill + * was gated on "the drain timed out", so hitting the capture cap (a + * distinct stop reason) skipped the kill and waitpid() blocked for the + * child's full lifetime. + */ +TEST_F(MetadataProtocolProviderTest, CapThenHang) +{ + // MaxCaptureBytes is 64 * 1024; 70000 bytes safely exceeds it for both + // probe attempts. + auto script = createExecutable("scrap-x", R"(case "$1" in + --scrap-metadata|--help) yes x | head -c 70000; sleep 30 ;; +esac +exit 1)"); + + constexpr std::chrono::milliseconds shortTimeout{200}; + MetadataProtocolProvider provider(shortTimeout); + + auto start = std::chrono::steady_clock::now(); + auto result = provider.fetch(script); + auto elapsed = std::chrono::steady_clock::now() - start; + + EXPECT_FALSE(result.has_value()); + // Two probe attempts each bounded by shortTimeout; generous upper bound + // keeps this robust under CI load while still proving the capture-cap + // path no longer blocks for the child's 30s sleep. + EXPECT_LT(elapsed, std::chrono::seconds(5)); +} + +/** + * Verify that a child which closes stdout (EOF) but keeps running is still + * killed and reaped promptly. Regression for a bug where the kill was + * gated on "the drain timed out"; plain EOF is a distinct stop reason that + * also skipped the kill, so a child that closes stdout and then keeps + * running could hang waitpid() indefinitely — the security-flagged variant + * (a bad `scrap-*` plugin can be probed on every `scrap --help`). + */ +TEST_F(MetadataProtocolProviderTest, ClosesStdoutThenHang) +{ + auto script = createExecutable("scrap-x", "exec 1>&-; sleep 30"); + + constexpr std::chrono::milliseconds shortTimeout{200}; + MetadataProtocolProvider provider(shortTimeout); + + auto start = std::chrono::steady_clock::now(); + auto result = provider.fetch(script); + auto elapsed = std::chrono::steady_clock::now() - start; + + EXPECT_FALSE(result.has_value()); + // EOF is observed immediately (stdout is closed), but the child keeps + // running; fetch() must still return within a small multiple of + // shortTimeout, proving the EOF path no longer blocks for the child's + // 30s sleep. + EXPECT_LT(elapsed, std::chrono::seconds(5)); +} + /** * Verify that a missing or non-executable path fails cleanly (no crash). */ From 128e39ca9869fa5455ab7959e19f5462ab51b02a Mon Sep 17 00:00:00 2001 From: Yuma Endo Date: Thu, 16 Jul 2026 19:15:54 +0900 Subject: [PATCH 3/3] fix: reap the metadata probe with a deadline-bounded loop The unconditional SIGKILL before waitpid could discard a valid child's exit status: EOF on the pipe only proves stdout is closed, not that the child exited, so a probe that writes its line, closes stdout, then does brief work before exiting 0 would be killed mid-flight and reported as WIFSIGNALED, dropping the already-captured output. Reap with a bounded loop instead (extracted into reapBounded): a non-blocking waitpid reaps an already-exited child at once with its real status; a child still running is polled until the deadline so one that closed stdout early but exits shortly after is still reaped normally; only a child still alive at the deadline is SIGKILLed. Add a regression test for the early-stdout-close case. --- src/command/MetadataProtocolProvider.cpp | 123 ++++++++++++------ .../command/MetadataProtocolProviderTest.cpp | 26 ++++ 2 files changed, 110 insertions(+), 39 deletions(-) diff --git a/src/command/MetadataProtocolProvider.cpp b/src/command/MetadataProtocolProvider.cpp index e8af56b..3a37a8a 100644 --- a/src/command/MetadataProtocolProvider.cpp +++ b/src/command/MetadataProtocolProvider.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include // does not reliably resolve killpg()/SIGKILL for include-cleaner on // all platforms; is the POSIX header that actually declares them. @@ -45,6 +46,12 @@ constexpr const char* HelpFlag = "--help"; constexpr std::size_t MaxCaptureBytes = 64UL * 1024UL; constexpr std::size_t ReadChunkBytes = 4096; +// Short sleep between non-blocking reap polls while waiting for a child that +// has already closed stdout to actually exit (see the deadline-bounded reap +// in runOnce()). Small enough not to add perceptible latency, large enough +// to avoid busy-waiting. +constexpr int ReapPollIntervalMs = 5; + /** * Result of running an executable once and capturing its stdout. */ @@ -167,9 +174,9 @@ class UniqueFd { * poll() so a full pipe can never deadlock the caller (the child keeps * making progress as we keep reading). * - * The caller (runOnce) always kills and reaps the child once this returns, - * regardless of which of the above reasons stopped the drain, so no return - * value is needed to single out "timed out" from the others. + * The caller (runOnce) reaps the child (bounded by the same deadline) once + * this returns, regardless of which of the above reasons stopped the drain, + * so no return value is needed to single out "timed out" from the others. */ auto drainUntilEofOrDeadline(int readFd, std::chrono::steady_clock::time_point deadline, std::string& out) -> void { @@ -178,7 +185,7 @@ auto drainUntilEofOrDeadline(int readFd, std::chrono::steady_clock::time_point d while (true) { auto remaining = deadline - std::chrono::steady_clock::now(); if (remaining <= std::chrono::steady_clock::duration::zero()) { - return; // Deadline reached; the caller kills and reaps the child unconditionally. + return; // Deadline reached; the caller reaps the child, bounded by the deadline. } // NOLINTNEXTLINE(misc-include-cleaner) - std::chrono::ceil is provided by @@ -213,7 +220,7 @@ auto drainUntilEofOrDeadline(int readFd, std::chrono::steady_clock::time_point d if (bytesRead == 0) { // EOF: the child closed its stdout. It may have exited already, // or it may still be running (e.g. `exec 1>&-; sleep ...`) — the - // caller's unconditional kill-then-reap handles both uniformly. + // caller's deadline-bounded reap handles both uniformly. return; } if (bytesRead < 0) { @@ -232,28 +239,78 @@ auto drainUntilEofOrDeadline(int readFd, std::chrono::steady_clock::time_point d } } +/** + * Reap @p childPid, bounded by @p deadline, and return its exit code if it + * exited normally in time. + * + * EOF on the child's stdout (where draining stops) proves only that the write + * end is closed, not that the child has exited — a valid probe can legitimately + * `echo ...; exec 1>&-; sleep 0.05; exit 0`, closing stdout while still doing + * brief work. Killing unconditionally at that point would race a + * still-alive-but-about-to-exit child and discard its real (already-captured) + * output for a bogus WIFSIGNALED status. So the reap instead: (1) tries a + * non-blocking waitpid() first — a child that has already exited (the common + * case) is reaped at once with its real status; (2) if the child is still + * running, keeps polling (bounded by @p deadline) so one that closed stdout + * early but exits shortly after is still reaped with its real status; (3) a + * child still alive at the deadline is SIGKILLed (its whole process group) and + * then waited for, bounding the reap so it can never block for the child's full + * lifetime. killpg (not kill) SIGKILLs the whole process group, including + * grandchildren the probe spawned, but waitpid(childPid) only reaps the direct + * child itself — any signalled grandchildren are reparented to init, which + * reaps them. Killing before the post-kill waitpid (not after) avoids a + * pid/pgid-reuse race, since the still-unreaped group leader keeps pgid valid + * and unique. + * + * @return the exit code if the child exited normally within the deadline, or + * std::nullopt if it had to be killed (or could not be reaped). + */ +auto reapBounded(pid_t childPid, std::chrono::steady_clock::time_point deadline) -> std::optional +{ + int status = 0; + pid_t waited = 0; + bool killed = false; + while (true) { + // NOLINTNEXTLINE(misc-include-cleaner) - WNOHANG is provided by + waited = ::waitpid(childPid, &status, WNOHANG); + if (waited == childPid) { + break; // Reaped; status is valid. + } + if (waited < 0) { + if (errno == EINTR) { + continue; + } + break; // e.g. ECHILD; nothing more we can do. + } + // waited == 0: still running. + if (std::chrono::steady_clock::now() >= deadline) { + // NOLINTNEXTLINE(misc-include-cleaner) - killpg() and SIGKILL are provided by + ::killpg(childPid, SIGKILL); + killed = true; + waited = ::waitpid(childPid, &status, 0); + while (waited < 0 && errno == EINTR) { + waited = ::waitpid(childPid, &status, 0); + } + break; + } + ::poll(nullptr, 0, ReapPollIntervalMs); // Portable short sleep; avoid busy-waiting. + } + + // NOLINTNEXTLINE(misc-include-cleaner) - WIFEXITED/WEXITSTATUS are provided by + if (! killed && waited == childPid && WIFEXITED(status)) { + return WEXITSTATUS(status); // NOLINT(misc-include-cleaner) - provided by + } + return std::nullopt; +} + /** * Run @p executable with a single @p flag argument (no shell, argv-array * only), capturing stdout while discarding stdin/stderr. The child runs in - * its own process group. - * - * The process group is unconditionally SIGKILLed right before the final - * (blocking) waitpid(), regardless of why draining its stdout stopped. If - * the child already exited (the common case: it wrote its output and - * closed stdout), it is a zombie and the kill is a harmless no-op — the - * zombie's exit status is preserved, so waitpid() still returns the real - * WIFEXITED/WEXITSTATUS. If the child is still alive (deadline reached, - * capture cap hit, or a poll/read error while it keeps running), the kill - * bounds the reap so waitpid() cannot block for the child's full lifetime. - * Either way WIFEXITED(status) below naturally distinguishes "exited - * normally" from "we had to kill it" — no separate flag is needed. killpg - * (not kill) also reaps any grandchildren the probe spawned; killing - * before waitpid (not after) avoids a pid/pgid-reuse race, since the - * still-unreaped group leader keeps pgid valid and unique. + * its own process group and is reaped via reapBounded(), so a slow or hung + * probe can never block the caller past @p timeout. * * Every path below closes any fds it opened (via UniqueFd's RAII) and, once - * posix_spawn has created a child, reaps it with waitpid — no fd leaks, no - * zombies. + * posix_spawn has created a child, reaps it — no fd leaks, no zombies. */ auto runOnce(const std::filesystem::path& executable, const char* flag, @@ -308,24 +365,12 @@ auto runOnce(const std::filesystem::path& executable, drainUntilEofOrDeadline(readFd.get(), deadline, result.capturedStdout); readFd.reset(); - // Kill unconditionally (see the doc comment above for why this is safe - // for an already-exited child too). It is its own process group - // leader, so pgid == childPid; any write it attempts after we've - // closed readFd yields a harmless SIGPIPE, since we are about to kill it. - // NOLINTNEXTLINE(misc-include-cleaner) - killpg() and SIGKILL are provided by - ::killpg(childPid, SIGKILL); - - int status = 0; - // No do-while: waitpid() must be attempted at least once, then retried on EINTR. - pid_t waited = ::waitpid(childPid, &status, 0); - while (waited < 0 && errno == EINTR) { - waited = ::waitpid(childPid, &status, 0); - } - - // NOLINTNEXTLINE(misc-include-cleaner) - WIFEXITED/WEXITSTATUS are provided by - if (waited == childPid && WIFEXITED(status)) { + // Reap the child, bounded by the deadline (see reapBounded). Its process + // group leader is childPid; any write it attempts after we closed readFd + // yields a harmless SIGPIPE. + if (auto exitCode = reapBounded(childPid, deadline)) { result.exitedNormally = true; - result.exitCode = WEXITSTATUS(status); // NOLINT(misc-include-cleaner) - provided by + result.exitCode = *exitCode; } return result; diff --git a/test/unit/command/MetadataProtocolProviderTest.cpp b/test/unit/command/MetadataProtocolProviderTest.cpp index d55c1ad..0c9faa3 100644 --- a/test/unit/command/MetadataProtocolProviderTest.cpp +++ b/test/unit/command/MetadataProtocolProviderTest.cpp @@ -193,6 +193,32 @@ TEST_F(MetadataProtocolProviderTest, ClosesStdoutThenHang) EXPECT_LT(elapsed, std::chrono::seconds(5)); } +/** + * Verify that a valid child which closes stdout, does a bit of brief work, + * and then exits 0 is NOT force-failed. Regression for a bug where the + * reap unconditionally SIGKILLed the process group right after the drain + * saw EOF: EOF only proves stdout's write end is closed, not that the + * child has exited, so a still-alive-but-about-to-exit child raced the + * kill and its already-captured (valid) output was discarded in favor of + * a bogus killed/WIFSIGNALED status. + */ +TEST_F(MetadataProtocolProviderTest, ValidProviderClosingStdoutEarly) +{ + auto script = createExecutable("scrap-x", R"(if [ "$1" = "--scrap-metadata" ]; then + echo "early-close-desc" + exec 1>&- + sleep 0.1 + exit 0 +fi +exit 1)"); + + MetadataProtocolProvider provider(FastTimeout); + auto result = provider.fetch(script); + + ASSERT_TRUE(result.has_value()); + EXPECT_EQ(result->description, "early-close-desc"); +} + /** * Verify that a missing or non-executable path fails cleanly (no crash). */