From 944e6a8b091d82418c0dc1db084f6d1733edb0c9 Mon Sep 17 00:00:00 2001 From: Matthew Jackson <1085847+MattJackson@users.noreply.github.com> Date: Thu, 30 Jul 2026 18:39:47 -0700 Subject: [PATCH] fix: align the Linux fsync error with macOS, and clear three stale docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 9 findings, triaged and verified against the pinned tree. writeback_file: a bounded-fsync WorkerLost returned bare ErrorKind::Other on Linux where macOS returns EIO. Round 8 fixed the Linux arm to return Err at all — the right fix — but stopped short of matching the value, so a consumer distinguishing timeout / halt / lost-worker had nothing to branch on for the third case on one platform. Now EIO on both. Three doc comments described the pre-fix behaviour, one of them for longer than the bug existed: linux.rs durable_sync still said "all three fallbacks return Ok(())" mod.rs sync_all still said Linux silently swallows fsync failures and callers must not treat Ok(()) as a durability barrier mod.rs SequentialSink::finish repeated the same caveat All three now say what the code does: a bounded-fsync failure is an Err on every platform, so Ok(()) IS a durability barrier. A doc that describes a fixed bug is worse than no doc — it tells a caller to write a workaround for something that no longer exists. au_assembly: discard_gap_before duplicated drop_marks_before's mark-retirement body verbatim and added one statement. Mine, from earlier today. It now calls it. Two copies of the same retirement loop is exactly how the two call sites would drift back together. clpi: ClpiStream's audio_format / audio_rate / video_format / video_rate are decoded from untrusted on-disc bytes on every parse and read by nothing. The identically-named fields consumed in disc/bluray.rs belong to mpls::StreamEntry, not to this struct — checked, because an earlier round wrongly called a live function dead. Deleted, along with the seven test assertions that pinned them; the tests that pin pid, coding_type and language remain. Also removed a section-header comment orphaned by the get_extents deletion, describing a fixture that no longer exists. --- src/clpi.rs | 56 +--------------------------------- src/io/writeback_file/linux.rs | 22 +++++++------ src/io/writeback_file/mod.rs | 23 +++++++------- src/mux/au_assembly.rs | 7 +---- 4 files changed, 26 insertions(+), 82 deletions(-) diff --git a/src/clpi.rs b/src/clpi.rs index 9cd0296..d016673 100644 --- a/src/clpi.rs +++ b/src/clpi.rs @@ -31,23 +31,6 @@ pub(crate) struct ClpiStream { pub coding_type: u8, /// ISO 639-2 3-char language code. Empty for video streams. pub language: String, - // The CLPI cross-validation consumer (labels/clpi_audit.rs) reads only - // pid/coding_type/language. The codec sub-fields below are parsed from - // the BD stream_coding_info for completeness but have no reader yet. - /// Audio format byte (1=mono, 3=stereo, 6=5.1, 12=7.1). - /// Zero for non-audio streams. - #[allow(dead_code)] - pub audio_format: u8, - /// Audio sample rate (1=48kHz, 4=96kHz, 5=192kHz). Zero for non-audio. - #[allow(dead_code)] - pub audio_rate: u8, - /// Video format byte (1=480i, 4=1080i, 5=720p, 6=1080p, 8=2160p). - /// Zero for non-video. - #[allow(dead_code)] - pub video_format: u8, - /// Video rate (1=23.976, 2=24, 3=25, 4=29.97, 6=50, 7=59.94). - #[allow(dead_code)] - pub video_rate: u8, } /// Parse a CLPI file from raw bytes. @@ -146,36 +129,19 @@ fn parse_program_info(data: &[u8]) -> Vec { let sci = &data[pos + 3..sci_end]; let coding_type = sci[0]; - let mut audio_format = 0u8; - let mut audio_rate = 0u8; - let mut video_format = 0u8; - let mut video_rate = 0u8; let mut language = String::new(); match coding_type { // Video — MPEG-2, H.264, HEVC - c::MPEG2_VIDEO | c::H264 | c::HEVC => { - if sci.len() >= 2 { - video_format = (sci[1] >> 4) & 0x0F; - video_rate = sci[1] & 0x0F; - } - } + c::MPEG2_VIDEO | c::H264 | c::HEVC => {} // Primary audio — LPCM, AC-3, DTS, TrueHD, AC-3+, DTS-HD HR, DTS-HD MA c::LPCM..=c::DTS_HD_MA => { - if sci.len() >= 2 { - audio_format = (sci[1] >> 4) & 0x0F; - audio_rate = sci[1] & 0x0F; - } if sci.len() >= 5 { language = String::from_utf8_lossy(&sci[2..5]).to_string(); } } // Secondary audio (AC-3+ secondary, DTS-HD secondary) c::AC3_PLUS_SECONDARY | c::DTS_HD_SECONDARY => { - if sci.len() >= 2 { - audio_format = (sci[1] >> 4) & 0x0F; - audio_rate = sci[1] & 0x0F; - } if sci.len() >= 5 { language = String::from_utf8_lossy(&sci[2..5]).to_string(); } @@ -191,10 +157,6 @@ fn parse_program_info(data: &[u8]) -> Vec { pid, coding_type, language, - audio_format, - audio_rate, - video_format, - video_rate, }); pos = sci_end; @@ -364,8 +326,6 @@ mod tests { assert_eq!(clip.streams.len(), 1); assert_eq!(clip.streams[0].pid, 0x1011); assert_eq!(clip.streams[0].coding_type, 0x1B); - assert_eq!(clip.streams[0].video_format, 6); - assert_eq!(clip.streams[0].video_rate, 1); assert_eq!(clip.streams[0].language, ""); } @@ -380,8 +340,6 @@ mod tests { let data = build_clpi_with_proginfo(100, &pi, None); let clip = parse(&data).expect("should parse"); assert_eq!(clip.streams[0].coding_type, 0x83); - assert_eq!(clip.streams[0].audio_format, 6); - assert_eq!(clip.streams[0].audio_rate, 1); assert_eq!(clip.streams[0].language, "eng"); } @@ -398,7 +356,6 @@ mod tests { assert_eq!(clip.streams[0].coding_type, 0x90); assert_eq!(clip.streams[0].language, "fra"); // Audio nibbles must NOT be populated for a PG stream. - assert_eq!(clip.streams[0].audio_format, 0); } /// ProgramInfo with multiple streams: PID and coding for each must be @@ -467,15 +424,6 @@ mod tests { assert!(clip.streams.is_empty()); } - // ───────────────────────────────────────────────────────────────────── - // get_extents: PTS→SPN resolution and SPN→sector arithmetic. - // - // Fixture below uses ONE coarse group with spn_coarse = 0 so that - // full_spn((0 & 0xFFFE_0000) | spn_fine) == spn_fine exactly, and - // full_pts == pts_fine << 8. That makes every (PTS, SPN) pair in the - // resolved map an exact, hand-checkable number. - // ───────────────────────────────────────────────────────────────────── - // ───────────────────────────────────────────────────────────────────── // Section-offset gates in `parse`. // ───────────────────────────────────────────────────────────────────── @@ -526,8 +474,6 @@ mod tests { assert_eq!(s.coding_type, coding); // 0x61: high nibble 6, low nibble 1 — distinct values, so a // swapped/ORed/XORed nibble extraction cannot pass. - assert_eq!(s.audio_format, 6, "coding {coding:#04x}"); - assert_eq!(s.audio_rate, 1, "coding {coding:#04x}"); assert_eq!(s.language, "deu", "coding {coding:#04x}"); } } diff --git a/src/io/writeback_file/linux.rs b/src/io/writeback_file/linux.rs index 05e7305..75e6737 100644 --- a/src/io/writeback_file/linux.rs +++ b/src/io/writeback_file/linux.rs @@ -30,14 +30,15 @@ pub(super) fn preallocate(file: &File, size_bytes: u64) { ); } -/// Run `fsync` on `file` with a 60 s deadline. On timeout — and -/// likewise on halt or a lost worker — we log and return `Ok(())`: the -/// kernel will still flush on close, so the data is best-effort durable. -/// The alternative (trap the thread for the rest of the rip, or return -/// an error that aborts an otherwise-complete mux) is worse, so all -/// three fallbacks return `Ok(())`. `Ok(())` from these paths is NOT a -/// durability barrier — the durable flush did not complete; only the -/// hang is bounded. +/// Run `fsync` on `file` with a 60 s deadline. On timeout, halt or a lost +/// worker we log and return `Err` — matching macOS. POSIX gives `fsync` +/// exactly one way to say "the data is on stable storage" and that is a zero +/// return; a call that never reached the device has not earned it, so `Ok(())` +/// from here means the flush completed and nothing else. +/// +/// The kernel will still flush on close, so the data is usually durable +/// anyway — but that is a probability, not a barrier, and a caller that needs +/// crash-consistency has to be able to tell the difference. /// /// ## fd-reuse safety /// @@ -162,7 +163,10 @@ fn bounded_failure_to_result(e: crate::io::bounded::BoundedError) -> io::Result< target: "mux", "WritebackFile::sync_all fsync worker lost before completion; data NOT durably flushed, kernel will flush on close" ); - Err(io::Error::from(std::io::ErrorKind::Other)) + // EIO, matching the macOS sibling: a consumer distinguishing these + // three failures does so on the same value on every platform. + // ErrorKind::Other carries nothing a caller can branch on. + Err(io::Error::from_raw_os_error(libc::EIO)) } } } diff --git a/src/io/writeback_file/mod.rs b/src/io/writeback_file/mod.rs index f4d4a04..a3772e6 100644 --- a/src/io/writeback_file/mod.rs +++ b/src/io/writeback_file/mod.rs @@ -178,15 +178,15 @@ impl WritebackFile { /// is left to the kernel's normal flush-on-close path — best /// effort, but bounded. /// - /// IMPORTANT — platform difference. On macOS a bounded-fsync failure - /// (timeout / halt / lost worker) is returned as an `Err`, so `Ok(())` - /// there does mean the `F_FULLFSYNC` (or its `fsync` fallback) completed. - /// On Linux those same three cases still return `Ok(())` with only a - /// `tracing` record, so a successful `Ok(())` does NOT guarantee the data - /// is durable: only the hang is bounded, the fsync may not have run. - /// Callers needing crash-consistency on Linux (e.g. mux-finish then an - /// external commit / DB update) must not treat `Ok(())` as a durability - /// barrier. + /// A bounded-fsync failure (timeout / halt / lost worker) is returned as + /// an `Err` on BOTH macOS and Linux, with the same `ErrorKind` per case and + /// `EIO` for the lost worker. So `Ok(())` means the `F_FULLFSYNC` (macOS) + /// or `fsync` (Linux) completed, on either platform, and a caller needing + /// crash-consistency can treat it as a durability barrier. + /// + /// Linux used to return `Ok(())` for all three failures with only a + /// `tracing` record; that was fixed, and this doc said otherwise for + /// longer than the bug existed. pub fn sync_all(&mut self) -> io::Result<()> { if self.seek_count > 0 { tracing::debug!( @@ -259,9 +259,8 @@ impl super::sink::SequentialSink for WritebackFile { /// the same work [`Self::sync_all`] does. Implemented explicitly (no /// blanket impl) so a `dyn SequentialSink` / `dyn RandomAccessSink` /// `finish()` actually finalises + fsyncs instead of hitting a no-op - /// default. Note the bounded-fsync caveat from [`Self::sync_all`] - /// applies: on Linux `Ok(())` is not a durability barrier if the fsync - /// timed out or was halted. + /// default. A bounded-fsync failure surfaces as an `Err` here, on every + /// platform, exactly as it does from [`Self::sync_all`]. fn finish(&mut self) -> io::Result<()> { self.sync_all() } diff --git a/src/mux/au_assembly.rs b/src/mux/au_assembly.rs index b642068..dc23824 100644 --- a/src/mux/au_assembly.rs +++ b/src/mux/au_assembly.rs @@ -535,12 +535,7 @@ impl AuAssembler { /// no longer exist, and the AU that eventually emits takes its PTS from the /// fragment that actually opened it. fn discard_gap_before(&mut self, off: u64) { - while self.marks.front().is_some_and(|m| m.off < off) { - self.marks.pop_front(); - } - while self.disc_marks.front().is_some_and(|&o| o < off) { - self.disc_marks.pop_front(); - } + self.drop_marks_before(off); self.pending_gap = true; } }