Announce record damage that is not a layout failure

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 |

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,

include/pup/index/reader.hpp

@@ -24,10 +24,6 @@ struct IndexFile {
 [[nodiscard]]
 auto open_index(std::string_view path) -> Result<IndexFile>;
 
-/// 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<Index>;

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.

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;
     }

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<IndexFile>(ErrorCode::InvalidFormat, "Index file too small");
+        return make_error<IndexFile>(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<IndexFile>(ErrorCode::InvalidFormat, "Invalid index file magic");
+        return make_error<IndexFile>(ErrorCode::IndexDamaged, "Invalid index file magic");
     }
     if (hdr->version < min_version || hdr->version > INDEX_VERSION) {
-        return make_error<IndexFile>(ErrorCode::InvalidFormat, "Unsupported index version");
+        return make_error<IndexFile>(ErrorCode::IndexVersionMismatch, "Unsupported index version");
     }
     if (!declared_layout_fits(result.file.size(), *hdr)) {
         return make_error<IndexFile>(ErrorCode::IndexDamaged, "Index sections do not fit the file");
@@ -122,19 +123,6 @@ auto open_index(std::string_view path) -> Result<IndexFile>
     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<RawHeader const*>(file->data());
-    return std::memcmp(header->magic.data(), INDEX_MAGIC.data(), 4) == 0
-        && header->version == INDEX_VERSION;
-}
-
 auto read_index(IndexFile const& f) -> Result<Index>
 {
     if (!index_is_open(f)) {

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")

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]")
@@ -1599,6 +1584,72 @@ auto sorted_paths_of_type(Index const& index, NodeType type) -> Vec<StringId>
 
 } // 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 {};