mirror of
https://github.com/rustfs/rustfs.git
synced 2026-08-22 04:16:38 +00:00
fix(ecstore): bound remote shard writers with a progress deadline so one black-hole peer cannot pin write quorum (#4925)
A PUT that fans out erasure shards to remote peers awaited every shard writer to completion on both the per-block write and the final shutdown, and the remote HttpWriter had no progress deadline. A peer that accepts the TCP connection but never drains the request body (or never sends a response) therefore wedges the writer forever once the bounded buffers fill, pinning an otherwise-healthy write quorum indefinitely — a cluster-level write-availability hazard triggered by a single bad peer (rustfs/backlog#1319, https://github.com/rustfs/backlog/issues/1319). MultiWriter now wraps each shard write and each shard-writer shutdown in a forward-progress deadline. The budget is re-armed on every block, so it bounds a stall rather than the total transfer time of a large object: a slow-but-honest writer that keeps completing shards is never killed, while a writer that makes no progress within the budget is failed and its disk dropped before commit. An optional absolute per-object cap (disabled by default) backstops a slow-drip peer that dribbles just enough progress to reset the per-block timer without ever converging; it is off by default so a legitimate large upload over a slow link is not killed on total time alone. Both knobs come from RUSTFS_OBJECT_DISK_WRITE_STALL_TIMEOUT (default 30s) and RUSTFS_OBJECT_DISK_WRITE_ABSOLUTE_CAP (default 0 = disabled); setting the stall timeout to 0 restores the previous wait-forever behavior for a conservative rollback. The deadline enforcement lives in MultiWriter (writer-agnostic), so it covers local and remote writers alike and keeps the existing control-flow shape: a timed-out shard is marked failed (Error::Timeout, which is not an ignored error) and excluded from the write quorum exactly like any other shard write failure, and the unchanged nil_count/quorum check then continues on quorum or fails cleanly. This deliberately stays out of the MultiWriter lifecycle / commit-coordinator territory owned by rustfs/backlog#1312. When a stalled writer is dropped to fail its shard, the remote HttpWriter must stop holding the connection and its buffered body. HttpWriter previously left its spawned request task running on drop; it now aborts that background task in Drop (it is no longer pin-projected, since every field is Unpin and the AsyncWrite impl already used get_mut). Bytes already handed to the transport cannot be unsent, but they land only in this upload's unique tmp path and are reclaimed by tmp GC — they never touch a committed object. Tests, all on a paused virtual clock so they are deterministic and non-flaky: - one black-hole writer still meets a 3/4 write quorum without hanging; two black holes fail the quorum cleanly (both for the per-block write and the shutdown paths). - a slow-but-honest writer that keeps making progress within the stall budget is never failed across many blocks. - the absolute cap bounds a slow-drip writer within a finite budget while the healthy writers keep quorum. - the default policy is armed by default and honors 0 as disabled. - HttpWriter aborts its background request task on drop against a hanging peer. The toxiproxy/black-hole 4x4 end-to-end acceptance depends on black-box test facilities from rustfs/backlog#1325, which are not built yet; that acceptance is deferred to #1325 and intentionally not faked here.
This commit is contained in:
@@ -976,21 +976,21 @@ impl Stream for ReceiverStream {
|
||||
}
|
||||
}
|
||||
|
||||
pin_project! {
|
||||
pub struct HttpWriter {
|
||||
url:String,
|
||||
method: Method,
|
||||
headers: HeaderMap,
|
||||
err_rx: tokio::sync::oneshot::Receiver<std::io::Error>,
|
||||
start_tx: Option<tokio::sync::oneshot::Sender<()>>,
|
||||
sender: PollSender<Option<Bytes>>,
|
||||
handle: tokio::task::JoinHandle<std::io::Result<()>>,
|
||||
pending_chunk: BytesMut,
|
||||
finish:bool,
|
||||
track_internode_metrics: bool,
|
||||
internode_operation: Option<&'static str>,
|
||||
|
||||
}
|
||||
// Not pin-projected: every field is `Unpin` and the `AsyncWrite` impl accesses
|
||||
// them through `get_mut()`, so a plain struct lets us add a manual `Drop`
|
||||
// (pin-project forbids one) to abort the background HTTP task — see below.
|
||||
pub struct HttpWriter {
|
||||
url: String,
|
||||
method: Method,
|
||||
headers: HeaderMap,
|
||||
err_rx: tokio::sync::oneshot::Receiver<std::io::Error>,
|
||||
start_tx: Option<tokio::sync::oneshot::Sender<()>>,
|
||||
sender: PollSender<Option<Bytes>>,
|
||||
handle: tokio::task::JoinHandle<std::io::Result<()>>,
|
||||
pending_chunk: BytesMut,
|
||||
finish: bool,
|
||||
track_internode_metrics: bool,
|
||||
internode_operation: Option<&'static str>,
|
||||
}
|
||||
|
||||
const HTTP_WRITER_CHANNEL_CAPACITY: usize = 8;
|
||||
@@ -1476,6 +1476,20 @@ impl AsyncWrite for HttpWriter {
|
||||
}
|
||||
}
|
||||
|
||||
impl Drop for HttpWriter {
|
||||
/// Abort the background HTTP request when the writer is dropped without a
|
||||
/// clean shutdown. On a stall timeout the caller drops this writer to fail
|
||||
/// its shard (rustfs/backlog#1319); a black-hole peer would otherwise leave
|
||||
/// the spawned task holding the connection and its buffered body alive. A
|
||||
/// cleanly shut-down writer has already joined the task, so aborting a
|
||||
/// finished handle is a no-op. This does not — and cannot — unsend bytes
|
||||
/// already handed to the transport: those land only in the upload's unique
|
||||
/// tmp path and are reclaimed by tmp GC, never touching a committed object.
|
||||
fn drop(&mut self) {
|
||||
self.handle.abort();
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
@@ -1877,18 +1891,10 @@ mod tests {
|
||||
};
|
||||
|
||||
let writer = HttpWriter::new(url, Method::PUT, HeaderMap::new()).await.unwrap();
|
||||
let HttpWriter {
|
||||
handle,
|
||||
sender,
|
||||
start_tx,
|
||||
..
|
||||
} = writer;
|
||||
drop(start_tx);
|
||||
drop(sender);
|
||||
handle
|
||||
.await
|
||||
.expect("HttpWriter background task should not panic")
|
||||
.expect("an unstarted HttpWriter should stop cleanly");
|
||||
// Dropping an unstarted writer aborts its parked background task
|
||||
// (rustfs/backlog#1319) before it can ever send a PUT.
|
||||
drop(writer);
|
||||
tokio::time::sleep(Duration::from_millis(50)).await;
|
||||
|
||||
assert_eq!(state.put_count.load(Ordering::SeqCst), 0);
|
||||
assert!(state.put_bodies.lock().await.is_empty());
|
||||
@@ -1896,6 +1902,70 @@ mod tests {
|
||||
server_handle.abort();
|
||||
}
|
||||
|
||||
/// A PUT handler that registers the request, then parks forever without ever
|
||||
/// sending a response — modeling a black-hole peer that accepts the
|
||||
/// connection but never completes the request, so the writer's background
|
||||
/// task stays parked at `request.send().await` until it is aborted.
|
||||
async fn hanging_put(State(state): State<TestState>, _body: Body) -> impl IntoResponse {
|
||||
state.put_count.fetch_add(1, Ordering::SeqCst);
|
||||
std::future::pending::<()>().await;
|
||||
StatusCode::OK
|
||||
}
|
||||
|
||||
// rustfs/backlog#1319: when a stalled remote writer is dropped (the encode
|
||||
// path drops it to fail its shard), the background HTTP task must be aborted
|
||||
// so it stops holding the connection and buffered body — it must not linger.
|
||||
#[tokio::test]
|
||||
async fn http_writer_drop_aborts_background_request_to_hanging_peer() {
|
||||
let state = TestState::default();
|
||||
let listener = match TcpListener::bind("127.0.0.1:0").await {
|
||||
Ok(listener) => listener,
|
||||
Err(err) if err.kind() == std::io::ErrorKind::PermissionDenied => return,
|
||||
Err(err) => panic!("test listener should bind: {err}"),
|
||||
};
|
||||
let addr = listener.local_addr().expect("listener local address should be available");
|
||||
let app = Router::new()
|
||||
.route("/hang", axum::routing::put(hanging_put))
|
||||
.with_state(state.clone());
|
||||
let server_handle = tokio::spawn(async move {
|
||||
axum::serve(listener, app).await.unwrap();
|
||||
});
|
||||
|
||||
let url = format!("http://{addr}/hang");
|
||||
let mut writer = HttpWriter::new(url, Method::PUT, HeaderMap::new()).await.unwrap();
|
||||
// A >1MiB write starts the request and hands the body to the background
|
||||
// task, which then parks in the handler that never responds.
|
||||
writer.write_all(&vec![0xa5u8; HTTP_WRITER_BUFFER_SIZE + 1]).await.unwrap();
|
||||
// Let the server register the request, so the background task is parked
|
||||
// (alive) at send() before we drop the writer.
|
||||
for _ in 0..100 {
|
||||
if state.put_count.load(Ordering::SeqCst) >= 1 {
|
||||
break;
|
||||
}
|
||||
tokio::time::sleep(Duration::from_millis(10)).await;
|
||||
}
|
||||
assert_eq!(state.put_count.load(Ordering::SeqCst), 1, "server should have accepted the request");
|
||||
|
||||
// Observe the background task via its abort handle; it must still be
|
||||
// running before the drop, then be aborted (finished) after it.
|
||||
let task = writer.handle.abort_handle();
|
||||
assert!(!task.is_finished(), "background task should still be running before drop");
|
||||
|
||||
drop(writer);
|
||||
|
||||
let mut aborted = false;
|
||||
for _ in 0..500 {
|
||||
if task.is_finished() {
|
||||
aborted = true;
|
||||
break;
|
||||
}
|
||||
tokio::time::sleep(Duration::from_millis(10)).await;
|
||||
}
|
||||
assert!(aborted, "dropping the writer must abort the background HTTP task");
|
||||
|
||||
server_handle.abort();
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn http_writer_shutdown_without_write_sends_empty_put() {
|
||||
let state = TestState::default();
|
||||
|
||||
Reference in New Issue
Block a user