fix: align the Linux fsync error with macOS, and clear three stale docs

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.
This commit is contained in:
Matthew Jackson
2026-07-30 18:39:47 -07:00
parent 5360f8d309
commit 944e6a8b09
4 changed files with 26 additions and 82 deletions
+1 -55
View File
@@ -31,23 +31,6 @@ pub(crate) struct ClpiStream {
pub coding_type: u8, pub coding_type: u8,
/// ISO 639-2 3-char language code. Empty for video streams. /// ISO 639-2 3-char language code. Empty for video streams.
pub language: String, 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. /// Parse a CLPI file from raw bytes.
@@ -146,36 +129,19 @@ fn parse_program_info(data: &[u8]) -> Vec<ClpiStream> {
let sci = &data[pos + 3..sci_end]; let sci = &data[pos + 3..sci_end];
let coding_type = sci[0]; 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(); let mut language = String::new();
match coding_type { match coding_type {
// Video — MPEG-2, H.264, HEVC // Video — MPEG-2, H.264, HEVC
c::MPEG2_VIDEO | c::H264 | c::HEVC => { c::MPEG2_VIDEO | c::H264 | c::HEVC => {}
if sci.len() >= 2 {
video_format = (sci[1] >> 4) & 0x0F;
video_rate = sci[1] & 0x0F;
}
}
// Primary audio — LPCM, AC-3, DTS, TrueHD, AC-3+, DTS-HD HR, DTS-HD MA // Primary audio — LPCM, AC-3, DTS, TrueHD, AC-3+, DTS-HD HR, DTS-HD MA
c::LPCM..=c::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 { if sci.len() >= 5 {
language = String::from_utf8_lossy(&sci[2..5]).to_string(); language = String::from_utf8_lossy(&sci[2..5]).to_string();
} }
} }
// Secondary audio (AC-3+ secondary, DTS-HD secondary) // Secondary audio (AC-3+ secondary, DTS-HD secondary)
c::AC3_PLUS_SECONDARY | c::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 { if sci.len() >= 5 {
language = String::from_utf8_lossy(&sci[2..5]).to_string(); language = String::from_utf8_lossy(&sci[2..5]).to_string();
} }
@@ -191,10 +157,6 @@ fn parse_program_info(data: &[u8]) -> Vec<ClpiStream> {
pid, pid,
coding_type, coding_type,
language, language,
audio_format,
audio_rate,
video_format,
video_rate,
}); });
pos = sci_end; pos = sci_end;
@@ -364,8 +326,6 @@ mod tests {
assert_eq!(clip.streams.len(), 1); assert_eq!(clip.streams.len(), 1);
assert_eq!(clip.streams[0].pid, 0x1011); assert_eq!(clip.streams[0].pid, 0x1011);
assert_eq!(clip.streams[0].coding_type, 0x1B); 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, ""); assert_eq!(clip.streams[0].language, "");
} }
@@ -380,8 +340,6 @@ mod tests {
let data = build_clpi_with_proginfo(100, &pi, None); let data = build_clpi_with_proginfo(100, &pi, None);
let clip = parse(&data).expect("should parse"); let clip = parse(&data).expect("should parse");
assert_eq!(clip.streams[0].coding_type, 0x83); 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"); 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].coding_type, 0x90);
assert_eq!(clip.streams[0].language, "fra"); assert_eq!(clip.streams[0].language, "fra");
// Audio nibbles must NOT be populated for a PG stream. // 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 /// ProgramInfo with multiple streams: PID and coding for each must be
@@ -467,15 +424,6 @@ mod tests {
assert!(clip.streams.is_empty()); 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`. // Section-offset gates in `parse`.
// ───────────────────────────────────────────────────────────────────── // ─────────────────────────────────────────────────────────────────────
@@ -526,8 +474,6 @@ mod tests {
assert_eq!(s.coding_type, coding); assert_eq!(s.coding_type, coding);
// 0x61: high nibble 6, low nibble 1 — distinct values, so a // 0x61: high nibble 6, low nibble 1 — distinct values, so a
// swapped/ORed/XORed nibble extraction cannot pass. // 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}"); assert_eq!(s.language, "deu", "coding {coding:#04x}");
} }
} }
+13 -9
View File
@@ -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 /// Run `fsync` on `file` with a 60 s deadline. On timeout, halt or a lost
/// likewise on halt or a lost worker we log and return `Ok(())`: the /// worker we log and return `Err` — matching macOS. POSIX gives `fsync`
/// kernel will still flush on close, so the data is best-effort durable. /// exactly one way to say "the data is on stable storage" and that is a zero
/// The alternative (trap the thread for the rest of the rip, or return /// return; a call that never reached the device has not earned it, so `Ok(())`
/// an error that aborts an otherwise-complete mux) is worse, so all /// from here means the flush completed and nothing else.
/// three fallbacks return `Ok(())`. `Ok(())` from these paths is NOT a ///
/// durability barrier — the durable flush did not complete; only the /// The kernel will still flush on close, so the data is usually durable
/// hang is bounded. /// 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 /// ## fd-reuse safety
/// ///
@@ -162,7 +163,10 @@ fn bounded_failure_to_result(e: crate::io::bounded::BoundedError) -> io::Result<
target: "mux", target: "mux",
"WritebackFile::sync_all fsync worker lost before completion; data NOT durably flushed, kernel will flush on close" "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))
} }
} }
} }
+11 -12
View File
@@ -178,15 +178,15 @@ impl WritebackFile {
/// is left to the kernel's normal flush-on-close path — best /// is left to the kernel's normal flush-on-close path — best
/// effort, but bounded. /// effort, but bounded.
/// ///
/// IMPORTANT — platform difference. On macOS a bounded-fsync failure /// A bounded-fsync failure (timeout / halt / lost worker) is returned as
/// (timeout / halt / lost worker) is returned as an `Err`, so `Ok(())` /// an `Err` on BOTH macOS and Linux, with the same `ErrorKind` per case and
/// there does mean the `F_FULLFSYNC` (or its `fsync` fallback) completed. /// `EIO` for the lost worker. So `Ok(())` means the `F_FULLFSYNC` (macOS)
/// On Linux those same three cases still return `Ok(())` with only a /// or `fsync` (Linux) completed, on either platform, and a caller needing
/// `tracing` record, so a successful `Ok(())` does NOT guarantee the data /// crash-consistency can treat it as a durability barrier.
/// 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 /// Linux used to return `Ok(())` for all three failures with only a
/// external commit / DB update) must not treat `Ok(())` as a durability /// `tracing` record; that was fixed, and this doc said otherwise for
/// barrier. /// longer than the bug existed.
pub fn sync_all(&mut self) -> io::Result<()> { pub fn sync_all(&mut self) -> io::Result<()> {
if self.seek_count > 0 { if self.seek_count > 0 {
tracing::debug!( tracing::debug!(
@@ -259,9 +259,8 @@ impl super::sink::SequentialSink for WritebackFile {
/// the same work [`Self::sync_all`] does. Implemented explicitly (no /// the same work [`Self::sync_all`] does. Implemented explicitly (no
/// blanket impl) so a `dyn SequentialSink` / `dyn RandomAccessSink` /// blanket impl) so a `dyn SequentialSink` / `dyn RandomAccessSink`
/// `finish()` actually finalises + fsyncs instead of hitting a no-op /// `finish()` actually finalises + fsyncs instead of hitting a no-op
/// default. Note the bounded-fsync caveat from [`Self::sync_all`] /// default. A bounded-fsync failure surfaces as an `Err` here, on every
/// applies: on Linux `Ok(())` is not a durability barrier if the fsync /// platform, exactly as it does from [`Self::sync_all`].
/// timed out or was halted.
fn finish(&mut self) -> io::Result<()> { fn finish(&mut self) -> io::Result<()> {
self.sync_all() self.sync_all()
} }
+1 -6
View File
@@ -535,12 +535,7 @@ impl AuAssembler {
/// no longer exist, and the AU that eventually emits takes its PTS from the /// no longer exist, and the AU that eventually emits takes its PTS from the
/// fragment that actually opened it. /// fragment that actually opened it.
fn discard_gap_before(&mut self, off: u64) { fn discard_gap_before(&mut self, off: u64) {
while self.marks.front().is_some_and(|m| m.off < off) { self.drop_marks_before(off);
self.marks.pop_front();
}
while self.disc_marks.front().is_some_and(|&o| o < off) {
self.disc_marks.pop_front();
}
self.pending_gap = true; self.pending_gap = true;
} }
} }