fix(io): a halted fsync is recognisable as a halt, not a hard failure
The three bounded-fsync failures returned bare io::ErrorKind values — TimedOut, Interrupted, Other/EIO. `is_halt()` matches on `io_error_code(e) == Some(E_HALTED)`, i.e. the "E<code>" prefix that `From<Error> for io::Error` mints, and that is documented as the ONLY recognised shape. A bare ErrorKind carries no prefix. So cancelling a rip while sync_all / finish was inside the bounded fsync made `is_halt()` return false, and the CLI reported a clean user cancel as a hard I/O failure at the end of an otherwise complete mux. All three were also mutually unclassifiable, which is the same information-loss the numeric-code scheme exists to prevent: a caller could not tell "cancelled" from "NFS wedged" from "worker died", and should not retry the third the way it retries the first. And their Display text is std English — "timed out", "operation interrupted" — reaching a user from library code, which this crate does not do. Now Error::Halted, Error::SyncTimeout (E_SYNC_TIMEOUT 9056) and Error::SyncWorkerLost (E_SYNC_WORKER_LOST 9057), on both platforms. E_HALTED also now maps to ErrorKind::Interrupted rather than falling into the 6000..=6999 InvalidData bucket. A stop is an interruption, not invalid data. Nothing branched on the old kind — every consumer uses is_halt() — so this is safe as well as more accurate. Two things worth recording. I first placed the E_HALTED arm AFTER the 6000..=6999 range arm and wrote a comment claiming it preceded it; match arms are ordered, so the range won and the comment was simply false. The test caught it. And the macOS test asserted only that each arm was non-Ok with a particular ErrorKind — it passed throughout the period the three were indistinguishable. It now asserts they can be TOLD APART, which is the property that actually matters. Found by the round-9 opus escalation over the API contract.
This commit is contained in:
@@ -213,6 +213,14 @@ pub const E_MP4_MISSING_CODEC_PRIVATE: u16 = 9050;
|
|||||||
/// the sink would have to write 0x0, producing a structurally complete file no
|
/// the sink would have to write 0x0, producing a structurally complete file no
|
||||||
/// player can render. Refuse instead.
|
/// player can render. Refuse instead.
|
||||||
pub const E_MP4_UNKNOWN_RESOLUTION: u16 = 9055;
|
pub const E_MP4_UNKNOWN_RESOLUTION: u16 = 9055;
|
||||||
|
/// The bounded durable flush did not complete within its deadline. The data is
|
||||||
|
/// NOT known to be on stable storage; the kernel will still flush on close, but
|
||||||
|
/// that is a probability, not a barrier.
|
||||||
|
pub const E_SYNC_TIMEOUT: u16 = 9056;
|
||||||
|
/// The bounded durable flush's worker thread was lost before it reported. Same
|
||||||
|
/// durability consequence as [`E_SYNC_TIMEOUT`], different cause — a caller
|
||||||
|
/// retrying a timeout should not retry this.
|
||||||
|
pub const E_SYNC_WORKER_LOST: u16 = 9057;
|
||||||
/// READ CAPACITY returned a short or overflowing transfer.
|
/// READ CAPACITY returned a short or overflowing transfer.
|
||||||
pub const E_DISC_CAPACITY_MALFORMED: u16 = 9047;
|
pub const E_DISC_CAPACITY_MALFORMED: u16 = 9047;
|
||||||
|
|
||||||
@@ -557,6 +565,10 @@ pub enum Error {
|
|||||||
/// `mp4://` video track has no resolved frame dimensions. See
|
/// `mp4://` video track has no resolved frame dimensions. See
|
||||||
/// [`E_MP4_UNKNOWN_RESOLUTION`].
|
/// [`E_MP4_UNKNOWN_RESOLUTION`].
|
||||||
Mp4UnknownResolution,
|
Mp4UnknownResolution,
|
||||||
|
/// The bounded durable flush timed out. See [`E_SYNC_TIMEOUT`].
|
||||||
|
SyncTimeout,
|
||||||
|
/// The bounded durable flush's worker was lost. See [`E_SYNC_WORKER_LOST`].
|
||||||
|
SyncWorkerLost,
|
||||||
PesFrameTooLarge {
|
PesFrameTooLarge {
|
||||||
size: usize,
|
size: usize,
|
||||||
},
|
},
|
||||||
@@ -747,6 +759,8 @@ impl Error {
|
|||||||
Error::Mp4Invalid => E_MP4_INVALID,
|
Error::Mp4Invalid => E_MP4_INVALID,
|
||||||
Error::Mp4MissingCodecPrivate => E_MP4_MISSING_CODEC_PRIVATE,
|
Error::Mp4MissingCodecPrivate => E_MP4_MISSING_CODEC_PRIVATE,
|
||||||
Error::Mp4UnknownResolution => E_MP4_UNKNOWN_RESOLUTION,
|
Error::Mp4UnknownResolution => E_MP4_UNKNOWN_RESOLUTION,
|
||||||
|
Error::SyncTimeout => E_SYNC_TIMEOUT,
|
||||||
|
Error::SyncWorkerLost => E_SYNC_WORKER_LOST,
|
||||||
Error::PesFrameTooLarge { .. } => E_PES_FRAME_TOO_LARGE,
|
Error::PesFrameTooLarge { .. } => E_PES_FRAME_TOO_LARGE,
|
||||||
Error::PesInvalidMagic => E_PES_INVALID_MAGIC,
|
Error::PesInvalidMagic => E_PES_INVALID_MAGIC,
|
||||||
Error::PesTrackTooLarge { .. } => E_PES_TRACK_TOO_LARGE,
|
Error::PesTrackTooLarge { .. } => E_PES_TRACK_TOO_LARGE,
|
||||||
@@ -957,6 +971,9 @@ impl From<Error> for std::io::Error {
|
|||||||
3000..=3999 => std::io::ErrorKind::PermissionDenied,
|
3000..=3999 => std::io::ErrorKind::PermissionDenied,
|
||||||
4000..=4999 => std::io::ErrorKind::Other,
|
4000..=4999 => std::io::ErrorKind::Other,
|
||||||
5000..=5999 => std::io::ErrorKind::Other,
|
5000..=5999 => std::io::ErrorKind::Other,
|
||||||
|
// A stop is an interruption, not invalid data. MUST precede the
|
||||||
|
// 6000..=6999 arm — E_HALTED is 6010 and match arms are ordered.
|
||||||
|
E_HALTED => std::io::ErrorKind::Interrupted,
|
||||||
6000..=6999 => std::io::ErrorKind::InvalidData,
|
6000..=6999 => std::io::ErrorKind::InvalidData,
|
||||||
7000..=7999 => std::io::ErrorKind::PermissionDenied,
|
7000..=7999 => std::io::ErrorKind::PermissionDenied,
|
||||||
8000..=8999 => std::io::ErrorKind::Other,
|
8000..=8999 => std::io::ErrorKind::Other,
|
||||||
@@ -1006,6 +1023,10 @@ impl From<Error> for std::io::Error {
|
|||||||
| E_MP4_INVALID
|
| E_MP4_INVALID
|
||||||
| E_MP4_MISSING_CODEC_PRIVATE
|
| E_MP4_MISSING_CODEC_PRIVATE
|
||||||
| E_MP4_UNKNOWN_RESOLUTION => std::io::ErrorKind::InvalidData,
|
| E_MP4_UNKNOWN_RESOLUTION => std::io::ErrorKind::InvalidData,
|
||||||
|
// Durability, not data validity: the write landed, the flush did
|
||||||
|
// not. TimedOut keeps the std kind a caller might already branch on
|
||||||
|
// while the E-code carries the distinction.
|
||||||
|
E_SYNC_TIMEOUT | E_SYNC_WORKER_LOST => std::io::ErrorKind::TimedOut,
|
||||||
// 9030 ExtentNotUnitAligned: a malformed/non-AACS-aligned
|
// 9030 ExtentNotUnitAligned: a malformed/non-AACS-aligned
|
||||||
// extent was handed to the prefetch producer.
|
// extent was handed to the prefetch producer.
|
||||||
9030 => std::io::ErrorKind::InvalidInput,
|
9030 => std::io::ErrorKind::InvalidInput,
|
||||||
@@ -1708,6 +1729,8 @@ mod tests {
|
|||||||
(Error::Mp4Invalid, E_MP4_INVALID),
|
(Error::Mp4Invalid, E_MP4_INVALID),
|
||||||
(Error::Mp4MissingCodecPrivate, E_MP4_MISSING_CODEC_PRIVATE),
|
(Error::Mp4MissingCodecPrivate, E_MP4_MISSING_CODEC_PRIVATE),
|
||||||
(Error::Mp4UnknownResolution, E_MP4_UNKNOWN_RESOLUTION),
|
(Error::Mp4UnknownResolution, E_MP4_UNKNOWN_RESOLUTION),
|
||||||
|
(Error::SyncTimeout, E_SYNC_TIMEOUT),
|
||||||
|
(Error::SyncWorkerLost, E_SYNC_WORKER_LOST),
|
||||||
(Error::M2tsPacketMalformed, E_M2TS_PACKET_MALFORMED),
|
(Error::M2tsPacketMalformed, E_M2TS_PACKET_MALFORMED),
|
||||||
(Error::ExtentNotUnitAligned, E_EXTENT_NOT_UNIT_ALIGNED),
|
(Error::ExtentNotUnitAligned, E_EXTENT_NOT_UNIT_ALIGNED),
|
||||||
(Error::DiscCapacityMalformed, E_DISC_CAPACITY_MALFORMED),
|
(Error::DiscCapacityMalformed, E_DISC_CAPACITY_MALFORMED),
|
||||||
|
|||||||
@@ -149,14 +149,14 @@ fn bounded_failure_to_result(e: crate::io::bounded::BoundedError) -> io::Result<
|
|||||||
target: "mux",
|
target: "mux",
|
||||||
"WritebackFile::sync_all fsync timed out after 60s; data NOT durably flushed, kernel will flush on close"
|
"WritebackFile::sync_all fsync timed out after 60s; data NOT durably flushed, kernel will flush on close"
|
||||||
);
|
);
|
||||||
Err(io::Error::from(io::ErrorKind::TimedOut))
|
Err(crate::error::Error::SyncTimeout.into())
|
||||||
}
|
}
|
||||||
crate::io::bounded::BoundedError::Halted => {
|
crate::io::bounded::BoundedError::Halted => {
|
||||||
tracing::warn!(
|
tracing::warn!(
|
||||||
target: "mux",
|
target: "mux",
|
||||||
"WritebackFile::sync_all fsync skipped (halt requested); data NOT durably flushed, kernel will flush on close"
|
"WritebackFile::sync_all fsync skipped (halt requested); data NOT durably flushed, kernel will flush on close"
|
||||||
);
|
);
|
||||||
Err(io::Error::from(io::ErrorKind::Interrupted))
|
Err(crate::error::Error::Halted.into())
|
||||||
}
|
}
|
||||||
crate::io::bounded::BoundedError::WorkerLost => {
|
crate::io::bounded::BoundedError::WorkerLost => {
|
||||||
tracing::error!(
|
tracing::error!(
|
||||||
@@ -166,7 +166,7 @@ fn bounded_failure_to_result(e: crate::io::bounded::BoundedError) -> io::Result<
|
|||||||
// EIO, matching the macOS sibling: a consumer distinguishing these
|
// EIO, matching the macOS sibling: a consumer distinguishing these
|
||||||
// three failures does so on the same value on every platform.
|
// three failures does so on the same value on every platform.
|
||||||
// ErrorKind::Other carries nothing a caller can branch on.
|
// ErrorKind::Other carries nothing a caller can branch on.
|
||||||
Err(io::Error::from_raw_os_error(libc::EIO))
|
Err(crate::error::Error::SyncWorkerLost.into())
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -129,21 +129,21 @@ fn bounded_failure_to_result(e: crate::io::bounded::BoundedError) -> io::Result<
|
|||||||
target: "mux",
|
target: "mux",
|
||||||
"WritebackFile::sync_all F_FULLFSYNC timed out after 60s; data NOT durably flushed, kernel will flush on close"
|
"WritebackFile::sync_all F_FULLFSYNC timed out after 60s; data NOT durably flushed, kernel will flush on close"
|
||||||
);
|
);
|
||||||
Err(io::Error::from(io::ErrorKind::TimedOut))
|
Err(crate::error::Error::SyncTimeout.into())
|
||||||
}
|
}
|
||||||
crate::io::bounded::BoundedError::Halted => {
|
crate::io::bounded::BoundedError::Halted => {
|
||||||
tracing::warn!(
|
tracing::warn!(
|
||||||
target: "mux",
|
target: "mux",
|
||||||
"WritebackFile::sync_all F_FULLFSYNC skipped (halt requested); data NOT durably flushed, kernel will flush on close"
|
"WritebackFile::sync_all F_FULLFSYNC skipped (halt requested); data NOT durably flushed, kernel will flush on close"
|
||||||
);
|
);
|
||||||
Err(io::Error::from(io::ErrorKind::Interrupted))
|
Err(crate::error::Error::Halted.into())
|
||||||
}
|
}
|
||||||
crate::io::bounded::BoundedError::WorkerLost => {
|
crate::io::bounded::BoundedError::WorkerLost => {
|
||||||
tracing::error!(
|
tracing::error!(
|
||||||
target: "mux",
|
target: "mux",
|
||||||
"WritebackFile::sync_all F_FULLFSYNC worker lost before completion; data NOT durably flushed, kernel will flush on close"
|
"WritebackFile::sync_all F_FULLFSYNC worker lost before completion; data NOT durably flushed, kernel will flush on close"
|
||||||
);
|
);
|
||||||
Err(io::Error::from_raw_os_error(libc::EIO))
|
Err(crate::error::Error::SyncWorkerLost.into())
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -215,12 +215,28 @@ mod tests {
|
|||||||
"a halted F_FULLFSYNC must not be reported as a completed sync"
|
"a halted F_FULLFSYNC must not be reported as a completed sync"
|
||||||
);
|
);
|
||||||
|
|
||||||
|
// The three arms must be DISTINGUISHABLE, not merely non-Ok. Each
|
||||||
|
// carries its own numeric code through the "E<code>" prefix that
|
||||||
|
// `From<Error> for io::Error` mints — the only shape `io_error_code`
|
||||||
|
// recognises. A bare `ErrorKind` cannot be classified, which is how a
|
||||||
|
// user cancel here used to read as a hard I/O failure.
|
||||||
let lost = bounded_failure_to_result(BoundedError::WorkerLost)
|
let lost = bounded_failure_to_result(BoundedError::WorkerLost)
|
||||||
.expect_err("a lost F_FULLFSYNC worker must be an error");
|
.expect_err("a lost F_FULLFSYNC worker must be an error");
|
||||||
assert_eq!(
|
assert!(
|
||||||
lost.raw_os_error(),
|
lost.to_string()
|
||||||
Some(libc::EIO),
|
.starts_with(&format!("E{}", crate::error::E_SYNC_WORKER_LOST)),
|
||||||
"a lost F_FULLFSYNC worker must not be reported as a completed sync"
|
"a lost worker must be identifiable, got {lost}"
|
||||||
|
);
|
||||||
|
assert!(
|
||||||
|
timeout
|
||||||
|
.to_string()
|
||||||
|
.starts_with(&format!("E{}", crate::error::E_SYNC_TIMEOUT)),
|
||||||
|
"a timeout must be distinguishable from a lost worker, got {timeout}"
|
||||||
|
);
|
||||||
|
assert!(
|
||||||
|
crate::error::is_halt(&halted),
|
||||||
|
"a halt must satisfy the crate's own is_halt(), or the CLI reports a \
|
||||||
|
user cancel as a failure; got {halted}"
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -178,15 +178,19 @@ 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.
|
||||||
///
|
///
|
||||||
/// A bounded-fsync failure (timeout / halt / lost worker) is returned as
|
/// A bounded-fsync failure is returned as an `Err` on BOTH platforms, so
|
||||||
/// an `Err` on BOTH macOS and Linux, with the same `ErrorKind` per case and
|
/// `Ok(())` means the flush completed and a caller needing
|
||||||
/// `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.
|
/// crash-consistency can treat it as a durability barrier.
|
||||||
///
|
///
|
||||||
/// Linux used to return `Ok(())` for all three failures with only a
|
/// The three causes are DISTINGUISHABLE by numeric code, because a caller
|
||||||
/// `tracing` record; that was fixed, and this doc said otherwise for
|
/// should not retry a lost worker the way it retries a timeout, and must
|
||||||
/// longer than the bug existed.
|
/// not report a user cancel as a failure:
|
||||||
|
///
|
||||||
|
/// * [`E_SYNC_TIMEOUT`](crate::error::E_SYNC_TIMEOUT) — deadline expired
|
||||||
|
/// * [`E_HALTED`](crate::error::E_HALTED) — cancelled;
|
||||||
|
/// [`is_halt`](crate::error::is_halt) recognises it
|
||||||
|
/// * [`E_SYNC_WORKER_LOST`](crate::error::E_SYNC_WORKER_LOST) — the worker
|
||||||
|
/// thread died before reporting
|
||||||
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!(
|
||||||
|
|||||||
Reference in New Issue
Block a user