From d4c0ef2bb801c864cb507cc3a3fd611203c93824 Mon Sep 17 00:00:00 2001 From: Takashi Yamashina Date: Wed, 12 Aug 2026 12:42:15 +0900 Subject: [PATCH 1/2] test: cover wal.rs's canonical_bytes_of_line and non-NotFound error arms MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #587 (#533 persistence audit): canonical_bytes_of_line's three call sites (shippable_records, parse_log's interior loop, tail_record_is_complete) had zero coverage of its None return — issue text confirmed by grep, no test in the repo ever exercised it. Also covers reset/replay/replay_readonly's non-NotFound error arms (previously only the NotFound-tolerant path was tested), strengthens the two existing heal-failure tests to check the outer "could not heal" wrapper message rather than only the injected cause, and adds a characterization test pinning that shippable_records does not dedupe a repeated seq (unlike parse_log's later-wins resolution) — intentional per its own doc comment, not a gap. Refs #587 --- src/wal.rs | 151 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 151 insertions(+) diff --git a/src/wal.rs b/src/wal.rs index 6896c328..d2c0e4f4 100644 --- a/src/wal.rs +++ b/src/wal.rs @@ -1100,6 +1100,44 @@ mod tests { assert!(error.to_string().contains("checksum mismatch"), "{error}"); } + /// `canonical_bytes_of_line`'s `,"crc":` marker is a byte-exact + /// match; a crc field that exists but isn't preceded by that exact + /// marker (here, because it's the FIRST field, preceded by `{` not + /// `,`) cannot be canonicalized — issue #587, this call site of + /// `canonical_bytes_of_line` had never been exercised at all. + #[test] + fn shippable_records_reports_an_uncanonicalizable_crc_field() { + let bytes = b"{\"crc\":7,\"seq\":1}\n".to_vec(); + let error = shippable_records(&bytes) + .expect_err("a crc field the marker search cannot locate must refuse, not trust"); + assert_eq!(error.kind(), io::ErrorKind::InvalidData); + assert!( + error + .to_string() + .contains("could not find it in the raw line"), + "{error}" + ); + } + + /// A duplicate seq is `parse_log`'s later-wins problem, not this + /// function's: shipping is a byte transport, not the trust + /// boundary (see the doc comment on `shippable_records`), so both + /// copies ride straight through. + #[test] + fn shippable_records_does_not_dedupe_a_repeated_seq() { + let path = scratch_wal("shippable-dupseq"); + append_batch(&path, 1, &[associate("ghost")]).unwrap(); + append_batch(&path, 1, &[associate("real")]).unwrap(); + + let bytes = fs::read(&path).unwrap(); + let records = shippable_records(&bytes).unwrap(); + assert_eq!( + records.iter().map(|r| r.seq).collect::>(), + vec![1, 1], + "unlike replay, both copies of a reused seq ship" + ); + } + /// An interior empty line is the same fatal corruption `parse_log` /// (replay) already refuses — never a shape `shippable_records` /// may skip past. Before the fix, this function silently skipped @@ -1207,6 +1245,35 @@ mod tests { assert!(error.to_string().contains("line 1"), "{error}"); } + /// A THIRD distinct corruption shape from the two siblings above: + /// the field name and its data are both intact, but the exact + /// `,"crc":` marker `canonical_bytes_of_line` scans for isn't + /// there (here, because whitespace crept in after the comma — JSON + /// itself does not care, but the byte-exact marker search does). + /// The record still parses with `crc: Some(_)`, so this is neither + /// the pre-checksum path nor the corrupted-field-name path: it is + /// `canonical_bytes_of_line` returning `None`, exercised for the + /// first time in this repository (issue #587). + #[test] + fn an_interior_crc_field_whose_marker_cannot_be_recovered_is_fatal() { + let path = scratch_wal("crc-marker-gone-interior"); + append_batch(&path, 1, &[associate("a")]).unwrap(); + + let text = String::from_utf8(fs::read(&path).unwrap()).unwrap(); + assert_eq!(text.matches(",\"crc\":").count(), 1); + fs::write(&path, text.replace(",\"crc\":", ", \"crc\":")).unwrap(); + + let error = replay::(&path, 0).unwrap_err(); + assert_eq!(error.kind(), io::ErrorKind::InvalidData); + assert!( + error + .to_string() + .contains("could not find it in the raw line"), + "{error}" + ); + assert!(error.to_string().contains("line 1"), "{error}"); + } + #[test] fn a_weight_serde_json_cannot_parse_bit_exact_still_replays_clean() { // serde_json (1.0.150, pinned) mis-rounds this exact literal by @@ -1525,6 +1592,32 @@ mod tests { ); } + /// The tail-specific counterpart to + /// `an_interior_crc_field_whose_marker_cannot_be_recovered_is_fatal`: + /// the record parses complete with `crc: Some(_)`, but the marker + /// `canonical_bytes_of_line` scans for isn't present byte-exact, so + /// this must be reported through `tail_record_is_complete`'s own + /// error rather than silently discarded as a torn write. + #[test] + fn a_tail_crc_field_whose_marker_cannot_be_recovered_is_fatal_not_torn() { + let path = scratch_wal("crc-marker-gone-tail"); + append_batch(&path, 1, &[associate("a")]).unwrap(); + let mut bytes = fs::read(&path).unwrap(); + assert_eq!(bytes.pop(), Some(b'\n')); + let text = String::from_utf8(bytes).unwrap(); + assert_eq!(text.matches(",\"crc\":").count(), 1); + fs::write(&path, text.replace(",\"crc\":", ", \"crc\":")).unwrap(); + + let error = replay::(&path, 0).unwrap_err(); + assert_eq!(error.kind(), io::ErrorKind::InvalidData); + assert!( + error + .to_string() + .contains("could not find it in the raw line"), + "{error}" + ); + } + #[test] fn a_legacy_tail_missing_its_newline_is_recovered_and_counted_unchecked() { // A pre-checksum record (no crc field) missing only its @@ -1578,6 +1671,15 @@ mod tests { error.to_string().contains("injected truncate failure"), "{error}" ); + // `replay` rewraps the injected failure with its own "could not + // heal" message — the inner assertion above only pinned the + // cause, not that this specific heal site is the one reporting it. + assert!( + error + .to_string() + .contains("could not heal torn WAL tail at"), + "{error}" + ); // The failed heal must not leave the file in some in-between // state: nothing was truncated, so the torn bytes are untouched. assert_eq!(fs::metadata(&path).unwrap().len(), torn_len); @@ -1599,11 +1701,35 @@ mod tests { error.to_string().contains("injected newline-heal failure"), "{error}" ); + // Same rewrap check as the truncate-heal sibling above: pin the + // outer "could not heal" message, not just the injected cause. + assert!( + error + .to_string() + .contains("could not heal a missing WAL newline at"), + "{error}" + ); // The failed heal must not leave the file in some in-between // state: the missing newline was never appended. assert_eq!(fs::metadata(&path).unwrap().len(), stripped_len); } + /// `reset`'s non-`NotFound` arm: any other truncate failure must + /// propagate, not be swallowed the way a missing file is. + #[test] + fn reset_propagates_a_non_missing_truncate_failure() { + let path = scratch_wal("reset-non-notfound-failure"); + append_batch(&path, 1, &[associate("a")]).unwrap(); + + fail_truncates_after(0); + let error = reset(&path).unwrap_err(); + assert_eq!(error.kind(), io::ErrorKind::Other); + assert!( + error.to_string().contains("injected truncate failure"), + "{error}" + ); + } + #[test] fn a_duplicate_seq_keeps_the_later_record_not_both() { // A complete batch whose sync then failed, with a rollback that @@ -1658,6 +1784,31 @@ mod tests { assert_eq!(unchecked, 0); } + /// `replay`'s non-`NotFound` read-failure arm. `fs::read` sits + /// outside the fault-injection choke point (storage.rs), so this + /// can only be reached with a real OS error — a directory sitting + /// where the WAL file should be turns the read into "not NotFound", + /// exactly the aspect this guards, without pinning a + /// platform-specific error kind (Linux and macOS disagree on it). + #[test] + fn replay_propagates_a_non_missing_read_failure() { + let path = scratch_wal("replay-non-notfound-failure"); + fs::create_dir_all(&path).unwrap(); + + let error = replay::(&path, 0).unwrap_err(); + assert_ne!(error.kind(), io::ErrorKind::NotFound, "{error}"); + } + + /// The read-only sibling of the test above. + #[test] + fn replay_readonly_propagates_a_non_missing_read_failure() { + let path = scratch_wal("replay-readonly-non-notfound-failure"); + fs::create_dir_all(&path).unwrap(); + + let error = replay_readonly::(&path, 0).unwrap_err(); + assert_ne!(error.kind(), io::ErrorKind::NotFound, "{error}"); + } + #[test] fn an_empty_batch_creates_no_file_and_appends_nothing() { let path = scratch_wal("empty"); From a2773d9a26b523148bf38da541b054b662837c29 Mon Sep 17 00:00:00 2001 From: Takashi Yamashina Date: Wed, 12 Aug 2026 13:41:45 +0900 Subject: [PATCH 2/2] fix: pin the tail's own line-number computation, not just the interior loop's MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pre-PR mutation gate found one missed mutant: parse_log's tail branch computes its 1-indexed line number as segments.len() + 1 (distinct from the interior loop's index + 1) — replacing + with * was unobserved because the new tail-position marker test never asserted on the reported line number, only the message body. Refs #587 --- src/wal.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/wal.rs b/src/wal.rs index d2c0e4f4..981d61fa 100644 --- a/src/wal.rs +++ b/src/wal.rs @@ -1616,6 +1616,10 @@ mod tests { .contains("could not find it in the raw line"), "{error}" ); + // The tail's line number is computed separately from the + // interior loop's (`segments.len() + 1`, not `index + 1`) — + // pin it here too, not just in the interior sibling test. + assert!(error.to_string().contains("line 1"), "{error}"); } #[test]