diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a1e0ea..85475d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,27 @@ EXPERIMENTAL, so on-disk formats and the CLI may change without notice. ## [Unreleased] +### Fixed documentation + +- **The detached replay buffer is capped, and the retention docs said it was + not.** They claimed the buffer had no cap of its own, that a runaway remote + shell would retain roughly 17 MB across a 15 minute window, and that the + detached TTL was therefore the only backstop against such a process. Measured + directly, all three were wrong. + + A detached session is never acknowledged, so its reader waits for buffer space + that never frees and stops at `MAX_BUFFERED_BYTES`, currently 4 MiB. A runaway + producer pins at exactly 4 MiB and stays there while the remote process blocks + writing to its own PTY. Retention is bounded per session no matter how long + the window is, and in aggregate by the server session cap — 256 MiB at the + default of 64 sessions. + + Backpressure is the backstop against a runaway remote process; the TTL bounds + how long a session lives, not how much it holds. This matters beyond accuracy: + the "no cap" claim was the stated reason not to raise the detached TTL + further, and that reason does not hold. A test now pins the bound so the claim + cannot drift back. + ### Added - **Durable connection telemetry.** `fabric status` now reports, per peer, how @@ -84,14 +105,11 @@ EXPERIMENTAL, so on-disk formats and the CLI may change without notice. shell now survives a closed lid over lunch; previously it did not survive a coffee break. The number comes from measured cost rather than taste: an idle detached session buffers 0 bytes across a full window, so holding one costs a - session struct and a PTY process and nothing that grows with time, while a - session still producing output grows at whatever the remote writes — about - 19 KB/s for a pathological loop, roughly 17 MB over this window. Past the - window the behaviour is unchanged and already proven: the client reports - `remote shell could not resume`, names the expired session, and exits - non-zero. This TTL remains the only backstop against a runaway remote process, - because the replay buffer has no cap of its own, so raising it much further - wants that cap first. + session struct and a PTY process and nothing that grows with time. A session + still producing output is bounded too: the replay buffer stops at 4 MiB, so + retention does not grow with the length of the window. Past the window the + behaviour is unchanged and already proven: the client reports `remote shell + could not resume`, names the expired session, and exits non-zero. - **Release archives have an enforced one-file contract.** Each platform tarball must contain exactly one member named literal `fabric` (not `./fabric`), and diff --git a/README.md b/README.md index 55d2d81..0e4d691 100644 --- a/README.md +++ b/README.md @@ -1348,12 +1348,23 @@ the PTY. The window is 15 minutes because that is what the cost measures out to, not as a round guess. An idle detached shell buffers nothing — 0 bytes across a full detached window — so holding one costs a session struct and a PTY process and -nothing that grows with time. A session still producing output is the expensive -case, growing at whatever the remote writes; a pathological output loop measured -about 19 KB/s, so roughly 17 MB over this window for one runaway shell. Note -that this TTL is currently the only backstop against such a process, because the -replay buffer has no cap of its own, which is why raising it much further wants -that cap first. +nothing that grows with time. + +A session still producing output is the expensive case, and it is bounded too. +The replay buffer stops at 4 MiB. Nothing acknowledges a detached session, so +the reader waits for buffer space that never frees and the remote process then +blocks writing to its own PTY. Measured directly, a runaway producer pins at +exactly 4 MiB and stays there. Retention is therefore bounded per session +regardless of how long this window is, and in aggregate by the server session +cap — 256 MiB at the default of 64 sessions. + +Backpressure, not this TTL, is what bounds a runaway remote process. The TTL +bounds how long a session lives, not how much it holds. + +> An earlier version of this section said the replay buffer had no cap, that a +> runaway shell would reach roughly 17 MB across this window, and that the TTL +> was the only backstop. All three were wrong. The "no cap" claim was also the +> stated reason not to raise the TTL further, so that reason no longer applies. Past the window, the client does not retry a session the server has already refused: it reports `remote shell could not resume`, names the expired session, diff --git a/src/config.rs b/src/config.rs index 60fe21e..9c2e57b 100644 --- a/src/config.rs +++ b/src/config.rs @@ -20,16 +20,25 @@ pub const DEFAULT_SERVER_SESSION_MAX_PER_PEER: usize = 16; /// longer window is bounded by what a detached session actually retains: an idle /// shell buffers nothing at all, measured at 0 bytes across a full detached /// window, so holding it costs a session struct and a PTY process and nothing -/// that grows with time. A session still producing output is the expensive case -/// and it grows at whatever the remote writes, measured at about 19 KB/s for a -/// pathological loop, so roughly 17 MB over this window for one runaway shell. +/// that grows with time. /// /// The benefit is the case that actually happens: a closed laptop lid over lunch /// keeps its shell. Sixty seconds did not survive a coffee break. /// -/// This is the only backstop against a runaway remote process, because the -/// server's replay buffer has no cap of its own, so it should not be raised much -/// further without capping that buffer first. +/// A session still producing output is the expensive case, and it is bounded +/// too. The tunnel replay buffer stops at its own cap, currently 4 MiB: +/// nothing ACKs a detached session, the reader waits for buffer space +/// that never frees, and the remote process then blocks on its own PTY write. +/// Measured directly, a runaway producer pins at exactly 4 MiB and stays there. +/// So retention is bounded per session no matter how long this window is, and in +/// aggregate by `DEFAULT_SERVER_SESSION_MAX_TOTAL`, which is 256 MiB at the +/// defaults. +/// +/// This doc previously said the buffer had no cap, that a runaway shell would +/// reach roughly 17 MB across this window, and that this TTL was the only +/// backstop. All three were wrong, and the first was the stated reason not to +/// raise this value further. Backpressure is the backstop against a runaway +/// process; this TTL bounds how long a session lives, not how much it holds. pub const DEFAULT_SERVER_SESSION_DETACHED_TTL_SECS: u64 = 15 * 60; #[derive(Debug, Clone)] diff --git a/src/tunnel.rs b/src/tunnel.rs index 6f2bfe2..1bdfb8f 100644 --- a/src/tunnel.rs +++ b/src/tunnel.rs @@ -2084,6 +2084,72 @@ mod tests { assert!(kill.is_cancelled()); } + /// A producer that never stops, standing in for a runaway remote process. + struct EndlessProducer; + + impl AsyncRead for EndlessProducer { + fn poll_read( + self: std::pin::Pin<&mut Self>, + _cx: &mut std::task::Context<'_>, + buf: &mut tokio::io::ReadBuf<'_>, + ) -> std::task::Poll> { + let n = buf.remaining().min(8192); + buf.put_slice(&vec![b'x'; n]); + std::task::Poll::Ready(Ok(())) + } + } + + /// A detached session's replay buffer is bounded, and the bound is the real + /// backstop against a runaway remote process. + /// + /// The retention docs used to say this buffer had no cap of its own, that a + /// runaway shell would reach roughly 17 MB across a 15 minute window, and + /// that the detached TTL was therefore the only backstop. Measured, all + /// three are wrong. The reader waits for buffer space, nothing ACKs a + /// detached session, so it stops at exactly MAX_BUFFERED_BYTES and the + /// remote process blocks on its own PTY write instead. Backpressure is the + /// backstop; the TTL bounds how long a session lives, not how much it holds. + /// + /// This matters beyond tidiness: "no cap" was the stated reason not to raise + /// the detached TTL further. + #[tokio::test] + async fn a_detached_replay_buffer_stops_at_the_cap_instead_of_growing() { + let (_write_peer, write) = duplex(64); + let (session, local_read) = TunnelSession::new_parts( + session_id(77), + peer_id(), + Box::new(EndlessProducer), + Box::new(write), + ); + // Never attached, so nothing ever ACKs. That is the worst case for + // retention and exactly the detached-session case being bounded here. + tokio::spawn(session.clone().run_local_reader(local_read)); + + let mut samples = Vec::new(); + for _ in 0..4 { + tokio::time::sleep(Duration::from_millis(150)).await; + samples.push(session.state.lock().await.buffered_bytes); + } + + // POSITIVE CONTROL. A producer that silently produced nothing would + // satisfy every bound below while proving nothing at all. + assert!( + samples[0] > 0, + "the producer never produced, so this test measured nothing" + ); + for (index, bytes) in samples.iter().enumerate() { + assert!( + *bytes <= MAX_BUFFERED_BYTES, + "sample {index} held {bytes} bytes, past the {MAX_BUFFERED_BYTES} byte cap" + ); + } + assert_eq!( + samples.last(), + samples.first(), + "the buffer must settle at the cap rather than keep growing: {samples:?}" + ); + } + /// A local reader that hands over `bytes`, then ends the way the test asks. /// /// This is the whole platform difference in issue 32, made explicit. Dropping