From a05c5ba99e1e1a184dee376d6d6465895e47cfcb Mon Sep 17 00:00:00 2001 From: Mura Li <2606021+typeless@users.noreply.github.com> Date: Wed, 12 Aug 2026 12:43:56 +0800 Subject: [PATCH 1/2] Announce record damage that is not a layout failure A record too small to hold a header, and one whose magic says it is not a putup index at all, are damage by any reading -- but both carried InvalidFormat, the same code as "this record's version is one I do not read", which is the expected benign outcome on every upgrade. load_old_index announced only checksum and layout failures, so a build over a truncated or overwritten record printed nothing and read as a first build: the failure IndexDamaged was introduced to end (#381), still live at three of its five sites. Give each open-time rejection a code that says which of the two happened: damage by any reading takes IndexDamaged, and a version outside the readable window takes IndexVersionMismatch, a code that was declared and never produced. The too-small and magic checks stay ahead of the version check because no version of the format is shorter than header plus footer and none lacks the magic, so neither can be a record of a version this binary merely does not read. Then invert the announcement: silence is now the enumerated case (IndexVersionMismatch alone) rather than the announced set, so a rejection the reader gains later is loud by default instead of silent by default -- the property whose absence is why #381 fixed one row and left three. The remaining arm covers IoError, where the record may be intact and the path or its permissions is the problem, and deliberately does not call it damage. InvalidFormat and IndexTruncated have no producers after this and never had a consumer or a test; ErrorCode is in-process only, so deleting them costs nothing. InvalidState moves to the general block, where it belongs. REQ-READ-ANNOUNCE-DAMAGE is widened by complement rather than by enumeration: it names the benign exception and lets damage be everything else, so it stays true as damage modes are added. Closes #383. --- DESIGN.md | 2 +- include/pup/core/result.hpp | 6 +-- spec/requirements/record-read.ears.md | 7 ++- src/cli/context.cpp | 14 +++++- src/index/reader.cpp | 7 +-- test/unit/test_e2e.cpp | 59 ++++++++++++++++++++++++ test/unit/test_index.cpp | 66 +++++++++++++++++++++++++++ 7 files changed, 148 insertions(+), 13 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index 7b75c503..1aa110c4 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -200,7 +200,7 @@ Error codes are categorized: | Category | Examples | |----------|----------| | General | InvalidArgument, NotFound, IoError | -| Index | IndexCorrupted, ChecksumMismatch | +| Index | IndexCorrupted, IndexChecksumMismatch, IndexDamaged, IndexVersionMismatch | | Parser | ParseError, UnterminatedString, CircularInclude | | Graph | CyclicDependency, UnknownMacro | | Exec | CommandFailed, MissingInput | diff --git a/include/pup/core/result.hpp b/include/pup/core/result.hpp index c39c801f..223ef7c4 100644 --- a/include/pup/core/result.hpp +++ b/include/pup/core/result.hpp @@ -20,12 +20,12 @@ enum class ErrorCode { AlreadyExists, PermissionDenied, IoError, + InvalidState, // Index errors IndexCorrupted, IndexVersionMismatch, IndexChecksumMismatch, - IndexTruncated, // A code that conflates damage with an expected-benign outcome forces every caller to choose between noise and silence. IndexDamaged, @@ -46,10 +46,6 @@ enum class ErrorCode { InvalidEdge, UnknownMacro, - // Index errors - InvalidFormat, - InvalidState, - // Execution errors CommandFailed, MissingInput, diff --git a/spec/requirements/record-read.ears.md b/spec/requirements/record-read.ears.md index d7022b7b..c4fe8dda 100644 --- a/spec/requirements/record-read.ears.md +++ b/spec/requirements/record-read.ears.md @@ -78,7 +78,10 @@ What the build says about a record it could not load. - conformance: putup-only - discharge: test "Scenario: A damaged record says so instead of rebuilding in silence" +- discharge: test "Scenario: A record too short to hold a header says so instead of rebuilding in silence" +- discharge: test "Scenario: A record that cannot be opened is announced without being called damage" - discharge: test "Scenario: An index from an unsupported version rebuilds without calling it damage" -If the record a build would load is damaged, then putup shall announce the damage; if its version is -merely outside the readable window, putup shall not call it damage. +If the record a build would load cannot be loaded, then putup shall announce that and shall call it +damage wherever the record itself is at fault; if its version is merely outside the readable window, +putup shall neither announce it nor call it damage. diff --git a/src/cli/context.cpp b/src/cli/context.cpp index e0535924..5c25a101 100644 --- a/src/cli/context.cpp +++ b/src/cli/context.cpp @@ -660,10 +660,20 @@ auto load_old_index(std::string_view output_root, bool verbose) -> IndexLoadResu // Not fatal here -- what this build can still do depends on what is on disk, and the // guard that knows decides (#291). Said out loud because the alternative is a silent // full rebuild, which reads as a first build (#294). - if (index_result.error().code == pup::ErrorCode::IndexChecksumMismatch) { + // Silence is the enumerated case, so a rejection added later announces unless deliberately excused (#383). + switch (index_result.error().code) { + case pup::ErrorCode::IndexVersionMismatch: + break; + case pup::ErrorCode::IndexChecksumMismatch: eprint("Warning: the build record at {} failed its checksum, so this build cannot use it.\n", index_path_sv); - } else if (index_result.error().code == pup::ErrorCode::IndexDamaged) { + break; + case pup::ErrorCode::IndexDamaged: eprint("Warning: the build record at {} is damaged, so this build cannot use it: {}\n", index_path_sv, index_result.error().msg()); + break; + default: + // Not damage: the bytes may be intact and the path or its permissions the problem. + eprint("Warning: the build record at {} could not be read, so this build cannot use it: {}\n", index_path_sv, index_result.error().msg()); + break; } return result; } diff --git a/src/index/reader.cpp b/src/index/reader.cpp index 76f65591..c52af723 100644 --- a/src/index/reader.cpp +++ b/src/index/reader.cpp @@ -92,16 +92,17 @@ auto open_index_in_window(std::string_view path, std::uint32_t min_version) -> R result.file = std::move(*file_result); + // Damage before version: neither a sub-header file nor foreign magic is a record of any version (#383). if (result.file.size() < sizeof(RawHeader) + sizeof(RawFooter)) { - return make_error(ErrorCode::InvalidFormat, "Index file too small"); + return make_error(ErrorCode::IndexDamaged, "Index file too small"); } auto const* hdr = index_header(result); if (!hdr || std::memcmp(hdr->magic.data(), INDEX_MAGIC.data(), 4) != 0) { - return make_error(ErrorCode::InvalidFormat, "Invalid index file magic"); + return make_error(ErrorCode::IndexDamaged, "Invalid index file magic"); } if (hdr->version < min_version || hdr->version > INDEX_VERSION) { - return make_error(ErrorCode::InvalidFormat, "Unsupported index version"); + return make_error(ErrorCode::IndexVersionMismatch, "Unsupported index version"); } if (!declared_layout_fits(result.file.size(), *hdr)) { return make_error(ErrorCode::IndexDamaged, "Index sections do not fit the file"); diff --git a/test/unit/test_e2e.cpp b/test/unit/test_e2e.cpp index e2654361..e4614ffd 100644 --- a/test/unit/test_e2e.cpp +++ b/test/unit/test_e2e.cpp @@ -7980,6 +7980,65 @@ SCENARIO("A damaged record says so instead of rebuilding in silence", "[e2e][inc } } +SCENARIO("A record too short to hold a header says so instead of rebuilding in silence", "[e2e][incremental]") +{ + GIVEN("an in-tree project built once, whose record is then cut below header and footer") + { + auto f = E2EFixture { "glob_mixed_space" }; + f.write_file("Tupfile", ": src.txt |> cp %f %o |> out.txt\n"); + f.write_file("src.txt", "ORIGINAL\n"); + REQUIRE(f.build().success()); + // Below sizeof(RawHeader) + sizeof(RawFooter): above it the declared-layout row answers + // first and this would pin the wrong rejection. + truncate_index(f, 40); + // Without this the shadow guard speaks first, as in the layout scenario above. + f.remove_file("out.txt"); + + WHEN("the next build loads that record") + { + auto const result = f.build(); + + THEN("it names the damage rather than reading as a first build") + { + INFO("stdout: " << result.stdout_output); + INFO("stderr: " << result.stderr_output); + REQUIRE(result.stderr_output.find("is damaged") != std::string::npos); + REQUIRE(result.success()); + REQUIRE(f.exists("out.txt")); + } + } + } +} + +SCENARIO("A record that cannot be opened is announced without being called damage", "[e2e][incremental]") +{ + GIVEN("an in-tree project built once, with a directory standing where its record belongs") + { + auto f = E2EFixture { "glob_mixed_space" }; + f.write_file("Tupfile", ": src.txt |> cp %f %o |> out.txt\n"); + f.write_file("src.txt", "ORIGINAL\n"); + REQUIRE(f.build().success()); + + auto const index_path = f.workdir() / ".pup" / "index"; + std::filesystem::remove(index_path); + std::filesystem::create_directory(index_path); + f.remove_file("out.txt"); + + WHEN("the next build tries to load it") + { + auto const result = f.build(); + + THEN("it says the record could not be read, and does not claim the record is damaged") + { + INFO("stdout: " << result.stdout_output); + INFO("stderr: " << result.stderr_output); + REQUIRE(result.stderr_output.find("could not be read") != std::string::npos); + REQUIRE(result.stderr_output.find("damaged") == std::string::npos); + } + } + } +} + SCENARIO("A rule writing above the build root fails the build instead of overwriting the file there", "[e2e][build][hierarchy]") { GIVEN("an out-of-tree project whose rule writes a path that climbs out of the build directory") diff --git a/test/unit/test_index.cpp b/test/unit/test_index.cpp index 9e72abf9..12927710 100644 --- a/test/unit/test_index.cpp +++ b/test/unit/test_index.cpp @@ -1599,6 +1599,72 @@ auto sorted_paths_of_type(Index const& index, NodeType type) -> Vec } // namespace +TEST_CASE("A record too short to hold a header is damage rather than a format this reader skips", "[index]") +{ + auto index = Index {}; + index.add_file(FileEntry { .id = 1, .parent_id = 0, .name = intern("main.c") }); + + auto data = serialize_index(index); + REQUIRE(data.has_value()); + + auto bytes = *data; + bytes.resize(sizeof(RawHeader) + sizeof(RawFooter) - 1); + + auto const path = temp_index_path("pup_record_too_short"); + write_bytes(path, bytes); + + auto const opened = open_index(path); + REQUIRE_FALSE(opened.has_value()); + REQUIRE(opened.error().code == ErrorCode::IndexDamaged); + + std::filesystem::remove(path); +} + +TEST_CASE("A file that does not carry the index magic is damage", "[index]") +{ + auto index = Index {}; + index.add_file(FileEntry { .id = 1, .parent_id = 0, .name = intern("main.c") }); + + auto data = serialize_index(index); + REQUIRE(data.has_value()); + + auto bytes = *data; + // No re-signing: the magic check runs before the checksum, so this is the arm under test. + for (auto i = std::size_t { 0 }; i < INDEX_MAGIC.size(); ++i) { + bytes[i] = std::byte { 'X' }; + } + + auto const path = temp_index_path("pup_record_foreign_magic"); + write_bytes(path, bytes); + + auto const opened = open_index(path); + REQUIRE_FALSE(opened.has_value()); + REQUIRE(opened.error().code == ErrorCode::IndexDamaged); + + std::filesystem::remove(path); +} + +TEST_CASE("A record from a version outside the window is a version mismatch rather than damage", "[index]") +{ + auto index = Index {}; + index.add_file(FileEntry { .id = 1, .parent_id = 0, .name = intern("main.c") }); + + auto data = serialize_index(index); + REQUIRE(data.has_value()); + + auto bytes = *data; + stamp_version(bytes, INDEX_VERSION + 1); + + auto const path = temp_index_path("pup_record_future_version"); + write_bytes(path, bytes); + + auto const opened = open_index(path); + REQUIRE_FALSE(opened.has_value()); + REQUIRE(opened.error().code == ErrorCode::IndexVersionMismatch); + + std::filesystem::remove(path); +} + TEST_CASE("An operand count larger than the record makes it unreadable", "[index]") { auto index = Index {}; From 8c578d2455635f90a76d4894e748a430b7d6b999 Mon Sep 17 00:00:00 2001 From: Mura Li <2606021+typeless@users.noreply.github.com> Date: Wed, 12 Aug 2026 12:44:04 +0800 Subject: [PATCH 2/2] Delete is_valid_index, a third encoding of the readable window No production caller; its only use was its own test. It answered a different question from open_index in two ways -- an exact version match rather than the [INDEX_LAYOUT_FLOOR, INDEX_VERSION] window, and a size floor of the header alone without the footer -- so keeping it meant maintaining a third definition of "a record this putup can read" that no build consults. --- include/pup/index/reader.hpp | 4 ---- src/index/reader.cpp | 13 ------------- test/unit/test_index.cpp | 15 --------------- 3 files changed, 32 deletions(-) diff --git a/include/pup/index/reader.hpp b/include/pup/index/reader.hpp index df052048..94c104cd 100644 --- a/include/pup/index/reader.hpp +++ b/include/pup/index/reader.hpp @@ -24,10 +24,6 @@ struct IndexFile { [[nodiscard]] auto open_index(std::string_view path) -> Result; -/// Check if a file is a valid index file (checks magic and version) -[[nodiscard]] -auto is_valid_index(std::string_view path) -> bool; - /// Read the entire index into memory [[nodiscard]] auto read_index(IndexFile const& f) -> Result; diff --git a/src/index/reader.cpp b/src/index/reader.cpp index c52af723..05e00670 100644 --- a/src/index/reader.cpp +++ b/src/index/reader.cpp @@ -123,19 +123,6 @@ auto open_index(std::string_view path) -> Result return open_index_in_window(path, INDEX_VERSION); } -auto is_valid_index(std::string_view path) -> bool -{ - auto file = pup::platform::MappedFile::open(path); - if (!file || file->size() < sizeof(RawHeader)) { - return false; - } - - // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast) - auto const* header = reinterpret_cast(file->data()); - return std::memcmp(header->magic.data(), INDEX_MAGIC.data(), 4) == 0 - && header->version == INDEX_VERSION; -} - auto read_index(IndexFile const& f) -> Result { if (!index_is_open(f)) { diff --git a/test/unit/test_index.cpp b/test/unit/test_index.cpp index 12927710..fa35c6a5 100644 --- a/test/unit/test_index.cpp +++ b/test/unit/test_index.cpp @@ -648,21 +648,6 @@ TEST_CASE("Index reader validation", "[e2e][index]") REQUIRE_FALSE(result.has_value()); } - SECTION("is_valid_index") - { - REQUIRE_FALSE(is_valid_index("/nonexistent")); - - // Create a valid index - auto index = Index {}; - index.add_file(FileEntry { .id = 1, .name = intern("test.c") }); - - auto temp_path = (std::filesystem::temp_directory_path() / "pup_valid_test").string(); - (void)write_index(temp_path, index); - - REQUIRE(is_valid_index(temp_path)); - - std::filesystem::remove(temp_path); - } } TEST_CASE("A record whose declared layout does not fit the file is refused", "[e2e][index]")