Skip to content

Commit 10befd0

Browse files
authored
fix(vfs): surface real Hub error and map HTTP status to errno on write failure (#198)
## Problem When a streaming (append-only) write fails, two diagnosability problems stack up: 1. The streaming worker captures the real error from `writer.write()`, but the `Finish` handler throws it away and returns a hardcoded `Error::hub("streaming write failed")`. The log only shows a generic message with no Hub status. 2. Every commit failure maps to a blanket `EIO`, so an importing client (e.g. Radarr/Sonarr) cannot tell a quota/storage reject apart from a real I/O error, and retries straight into the quota wall. This surfaced during a real quota incident: the Hub rejected writes (quota exceeded), but the log only said `Hub API error: streaming write failed` with `errno=5`. The actual cause had to be guessed by hand, and the importing app kept looping with file loss. ## Changes - Keep the structured `Error` (not just its string) in the streaming worker so the HTTP status survives, and propagate it on `Finish`. - Preserve the HTTP status from CAS `ClientError` in `From<xet_data::DataError>` instead of flattening it into an opaque string. (Verified the upload error path keeps the structured `DataError::ClientError`, whose `status()` exposes the code.) - Add `Error::status()` and `Error::to_errno()`: map `413`/`507` to `ENOSPC`, `403` to `EACCES`, `429` to `EAGAIN`, everything else stays `EIO`. - Return `e.to_errno()` from the streaming write/commit failure paths. ## Result A quota reject now: - logs the real `Hub API error (413): ...` instead of a generic string, and - surfaces as `ENOSPC` ("No space left on device") to the FUSE client, so the importing app stops retrying into a quota wall instead of looping. ## Design notes - `413`/`507` map to `ENOSPC` rather than `EDQUOT`: `ENOSPC` is more widely recognized by importing apps and clearer to operators. Happy to switch to `EDQUOT` if preferred. - Scope is limited to the streaming/append-only write path (the incident path). The advanced-writes flush path (which already retries) and remote deletes are untouched. ## Tests - `error::tests` covers the status -> errno mapping (including statusless and non-Hub errors staying `EIO`). - `streaming_worker_surfaces_real_hub_error_on_failed_write` is a regression test: a write failing with a `413` must surface the real message, `status() == Some(413)`, and `to_errno() == ENOSPC`. - Full suite green with `--features fuse,nfs` (349 passed); clippy clean across all feature combos with `-D warnings`.
1 parent 91c48f9 commit 10befd0

3 files changed

Lines changed: 122 additions & 11 deletions

File tree

src/error.rs

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,29 @@ impl Error {
2323
status: Some(status),
2424
}
2525
}
26+
27+
/// HTTP status carried by the error, when it originated from a Hub/CAS response.
28+
pub fn status(&self) -> Option<u16> {
29+
match self {
30+
Self::Hub { status, .. } => *status,
31+
_ => None,
32+
}
33+
}
34+
35+
/// Errno to surface to FUSE clients. Maps known Hub/CAS HTTP statuses to a
36+
/// meaningful errno so an importing app (e.g. Radarr/Sonarr) can tell a quota
37+
/// or storage reject apart from a generic I/O failure. Everything else stays EIO.
38+
pub fn to_errno(&self) -> i32 {
39+
match self.status() {
40+
// Payload too large / insufficient storage: surface as "no space left"
41+
// so the client stops retrying into a quota wall instead of looping.
42+
Some(413 | 507) => libc::ENOSPC,
43+
Some(403) => libc::EACCES,
44+
// Rate-limited: transient, signal "try again".
45+
Some(429) => libc::EAGAIN,
46+
_ => libc::EIO,
47+
}
48+
}
2649
}
2750

2851
impl fmt::Display for Error {
@@ -63,6 +86,14 @@ impl From<reqwest::Error> for Error {
6386

6487
impl From<xet_data::DataError> for Error {
6588
fn from(err: xet_data::DataError) -> Self {
89+
// Preserve the HTTP status from CAS client errors (e.g. 413/507 on a quota
90+
// reject during upload) so it can be surfaced in logs and mapped to a
91+
// meaningful errno. Without this the status is flattened into an opaque string.
92+
if let xet_data::DataError::ClientError(client_error) = &err
93+
&& let Some(status) = client_error.status()
94+
{
95+
return Self::hub_status(status.as_u16(), err.to_string());
96+
}
6697
Self::Xet(err.to_string())
6798
}
6899
}
@@ -78,3 +109,27 @@ pub fn is_retryable_status(status: u16) -> bool {
78109
}
79110

80111
pub type Result<T> = std::result::Result<T, Error>;
112+
113+
#[cfg(test)]
114+
mod tests {
115+
use super::*;
116+
117+
#[test]
118+
fn to_errno_maps_known_statuses() {
119+
assert_eq!(Error::hub_status(413, "payload too large").to_errno(), libc::ENOSPC);
120+
assert_eq!(Error::hub_status(507, "insufficient storage").to_errno(), libc::ENOSPC);
121+
assert_eq!(Error::hub_status(403, "forbidden").to_errno(), libc::EACCES);
122+
assert_eq!(Error::hub_status(429, "rate limited").to_errno(), libc::EAGAIN);
123+
// Unknown status and statusless errors stay generic.
124+
assert_eq!(Error::hub_status(500, "server error").to_errno(), libc::EIO);
125+
assert_eq!(Error::hub("no status").to_errno(), libc::EIO);
126+
assert_eq!(Error::Xet("opaque".into()).to_errno(), libc::EIO);
127+
}
128+
129+
#[test]
130+
fn status_only_set_for_hub_with_status() {
131+
assert_eq!(Error::hub_status(413, "x").status(), Some(413));
132+
assert_eq!(Error::hub("x").status(), None);
133+
assert_eq!(Error::Xet("x".into()).status(), None);
134+
}
135+
}

src/virtual_fs/mod.rs

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1120,7 +1120,7 @@ impl VirtualFs {
11201120
// unbounded memory. 32 slots × ~128KB FUSE write = ~4MB max in-flight.
11211121
// blocking_send is safe here: FUSE threads are not tokio workers.
11221122
let (tx, rx) = tokio::sync::mpsc::channel::<WriteMsg>(32);
1123-
let error: Arc<std::sync::Mutex<Option<String>>> = Arc::new(std::sync::Mutex::new(None));
1123+
let error: Arc<std::sync::Mutex<Option<crate::error::Error>>> = Arc::new(std::sync::Mutex::new(None));
11241124
self.runtime
11251125
.spawn(streaming_worker(streaming_writer, rx, error.clone()));
11261126

@@ -2316,8 +2316,8 @@ impl VirtualFs {
23162316
channel,
23172317
} => {
23182318
// Check for previous worker error
2319-
if channel.error.lock().expect("error poisoned").is_some() {
2320-
return Err(libc::EIO);
2319+
if let Some(err) = channel.error.lock().expect("error poisoned").as_ref() {
2320+
return Err(err.to_errno());
23212321
}
23222322

23232323
// Enforce append-only: offset must match bytes written so far
@@ -2362,8 +2362,8 @@ impl VirtualFs {
23622362

23632363
if let Some(channel) = streaming_channel {
23642364
// Check for worker errors first.
2365-
if channel.error.lock().expect("error poisoned").is_some() {
2366-
return Err(libc::EIO);
2365+
if let Some(err) = channel.error.lock().expect("error poisoned").as_ref() {
2366+
return Err(err.to_errno());
23672367
}
23682368

23692369
// Check current commit state.
@@ -2629,7 +2629,7 @@ impl VirtualFs {
26292629
Ok(Ok(info)) => info,
26302630
Ok(Err(e)) => {
26312631
error!("Streaming upload failed for ino={}: {}", ino, e);
2632-
return Err(libc::EIO);
2632+
return Err(e.to_errno());
26332633
}
26342634
Err(_) => {
26352635
error!("Streaming worker dropped for ino={}", ino);
@@ -2665,7 +2665,7 @@ impl VirtualFs {
26652665
error!("Failed to commit file {}: {}", full_path, e);
26662666
// CAS upload succeeded — preserve file_info for retry in release()
26672667
*channel.pending_info.lock().expect("pending_info poisoned") = Some(file_info);
2668-
return Err(libc::EIO);
2668+
return Err(e.to_errno());
26692669
}
26702670

26712671
let mut inodes = self.inode_table.write().expect("inodes poisoned");
@@ -3837,7 +3837,8 @@ struct StreamingChannel {
38373837
tx: tokio::sync::mpsc::Sender<WriteMsg>,
38383838
bytes_written: AtomicU64,
38393839
/// Set by the background worker if add_data() fails. Shared with worker via Arc.
3840-
error: Arc<std::sync::Mutex<Option<String>>>,
3840+
/// Holds the structured error so its HTTP status survives to errno mapping.
3841+
error: Arc<std::sync::Mutex<Option<crate::error::Error>>>,
38413842
/// Commit lifecycle state machine.
38423843
state: std::sync::Mutex<CommitState>,
38433844
/// CAS upload succeeded but Hub commit failed — stored for retry.
@@ -3904,7 +3905,7 @@ fn same_process(pid_a: u32, pid_b: u32) -> bool {
39043905
async fn streaming_worker(
39053906
mut writer: Box<dyn StreamingWriterOps>,
39063907
mut rx: tokio::sync::mpsc::Receiver<WriteMsg>,
3907-
error: Arc<std::sync::Mutex<Option<String>>>,
3908+
error: Arc<std::sync::Mutex<Option<crate::error::Error>>>,
39083909
) {
39093910
let mut failed = false;
39103911
while let Some(msg) = rx.recv().await {
@@ -3914,13 +3915,21 @@ async fn streaming_worker(
39143915
continue; // drain remaining messages
39153916
}
39163917
if let Err(e) = writer.write(&data).await {
3917-
*error.lock().unwrap() = Some(e.to_string());
3918+
*error.lock().expect("error poisoned") = Some(e);
39183919
failed = true;
39193920
}
39203921
}
39213922
WriteMsg::Finish(reply) => {
39223923
let result = if failed {
3923-
Err(crate::error::Error::hub("streaming write failed"))
3924+
// Surface the real error captured on the failing write() instead of a
3925+
// hardcoded generic. Keeping the structured Error preserves its HTTP
3926+
// status (e.g. 413/507 on a quota reject), which both logs the real
3927+
// cause and lets streaming_commit map it to a meaningful errno.
3928+
Err(error
3929+
.lock()
3930+
.expect("error poisoned")
3931+
.take()
3932+
.unwrap_or_else(|| crate::error::Error::hub("streaming write failed: unknown error")))
39243933
} else {
39253934
writer.finish_boxed().await
39263935
};

src/virtual_fs/tests.rs

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5418,3 +5418,50 @@ fn overlay_rmdir_remote_dir_eperm() {
54185418
assert_eq!(err, libc::EPERM);
54195419
});
54205420
}
5421+
5422+
/// Regression: when a streaming write fails, the worker must surface the real Hub
5423+
/// error (including its HTTP status) on Finish, not a hardcoded generic. A quota
5424+
/// reject (HTTP 413) was previously logged as "Hub API error: streaming write failed"
5425+
/// and mapped to a blanket EIO, hiding the actual cause and making the importing app
5426+
/// (Radarr/Sonarr) retry into a quota wall instead of stopping.
5427+
#[test]
5428+
fn streaming_worker_surfaces_real_hub_error_on_failed_write() {
5429+
struct FailingWriter;
5430+
5431+
#[async_trait::async_trait]
5432+
impl StreamingWriterOps for FailingWriter {
5433+
async fn write(&mut self, _data: &[u8]) -> crate::error::Result<()> {
5434+
Err(crate::error::Error::hub_status(413, "storage quota exceeded"))
5435+
}
5436+
async fn finish_boxed(self: Box<Self>) -> crate::error::Result<XetFileInfo> {
5437+
unreachable!("finish must not be called after a failed write");
5438+
}
5439+
fn len(&self) -> u64 {
5440+
0
5441+
}
5442+
fn is_empty(&self) -> bool {
5443+
true
5444+
}
5445+
}
5446+
5447+
new_runtime().block_on(async {
5448+
let (tx, rx) = tokio::sync::mpsc::channel::<WriteMsg>(4);
5449+
let error = Arc::new(std::sync::Mutex::new(None));
5450+
let worker = tokio::spawn(streaming_worker(Box::new(FailingWriter), rx, error.clone()));
5451+
5452+
tx.send(WriteMsg::Data(vec![0u8; 8])).await.unwrap();
5453+
let (result_tx, result_rx) = tokio::sync::oneshot::channel();
5454+
tx.send(WriteMsg::Finish(result_tx)).await.unwrap();
5455+
5456+
let result = result_rx.await.expect("worker dropped reply");
5457+
let err = result.expect_err("failed write must produce an error");
5458+
let msg = err.to_string();
5459+
assert!(msg.contains("413"), "lost HTTP status: {msg}");
5460+
assert!(msg.contains("storage quota exceeded"), "lost real detail: {msg}");
5461+
// Structured status must survive so it maps to a meaningful errno, not blanket EIO.
5462+
assert_eq!(err.status(), Some(413));
5463+
assert_eq!(err.to_errno(), libc::ENOSPC);
5464+
5465+
worker.await.unwrap();
5466+
});
5467+
}

0 commit comments

Comments
 (0)