diff --git a/apps/figcraft-server/src/codes_tests.rs b/apps/figcraft-server/src/codes_tests.rs index cbc7f70a7..42b15961e 100644 --- a/apps/figcraft-server/src/codes_tests.rs +++ b/apps/figcraft-server/src/codes_tests.rs @@ -118,7 +118,7 @@ fn a_file_shared_online_is_joined_by_its_codes() { }); for guest in [&b, &c] { assert!(guest.message.as_deref().unwrap_or_default().contains("Sharing was stopped"), "{:?}", guest.message); - assert_eq!(guest.live().status, Status::Off, "over, not failed: nothing retries"); + assert!(guest.live().rejoinable() && !guest.live().status.active(), "over: nothing retries, and the tab says so"); } assert_eq!(layers(&b), before + 2, "Bo keeps the copy"); let folder = dir.path().join("files").join(&p.file); diff --git a/apps/figcraft-server/src/server_tests.rs b/apps/figcraft-server/src/server_tests.rs index f60150da9..f3d084499 100644 --- a/apps/figcraft-server/src/server_tests.rs +++ b/apps/figcraft-server/src/server_tests.rs @@ -160,7 +160,7 @@ fn revoked_links_stop_working_at_once() { assert_eq!((files.len(), files[0].people), (1, 3)); // Revoked: Cy is out at once, the others stay. assert_eq!(ask(Admin::Revoke { file: file.clone(), link: review.link.id.clone() }), AdminReply::Done); - pump(&mut [&mut a, &mut b, &mut c], "Cy out", |s| s[2].live().status == Status::Off && s[0].live().peers.len() == 1); + pump(&mut [&mut a, &mut b, &mut c], "Cy out", |s| s[2].live().rejoinable() && s[0].live().peers.len() == 1); // Told as when a host stops: the session is over for Cy, whose copy stays open (no retries). assert!(c.message.as_deref().unwrap_or_default().contains("Sharing was stopped"), "{:?}", c.message); assert!(live(&b)); diff --git a/crates/collab-net/src/driver.rs b/crates/collab-net/src/driver.rs index 001497c6d..e55eba957 100644 --- a/crates/collab-net/src/driver.rs +++ b/crates/collab-net/src/driver.rs @@ -28,7 +28,7 @@ use crate::invite::Endpoint; use crate::join::{GuestConfig, Next, Outcome, Resuming, Seen, Taken, after, ask, closed, hello, resolved, take, too_large, unresolved}; use crate::limits::NetLimits; use crate::requests::Publish; -use crate::throttle::{PRESENCE_INTERVAL, Throttle, earliest}; +use crate::throttle::{PRESENCE_INTERVAL, Paced, Throttle, earliest}; /// A WebSocket the driver opens, writes and closes; what it receives comes back through the /// driver's event methods. @@ -79,6 +79,8 @@ pub(crate) struct Driver { /// Your latest presence while not welcomed (sent with the welcome). pending: Option, throttle: Throttle, + /// Sync messages waiting their turn (a long queue from while offline). + paced: Paced, reconnect_at: Option, /// What this connection asks a server (a managing one), and what follows its answers. asking: Option<(Box, Then)>, @@ -86,7 +88,7 @@ pub(crate) struct Driver { } impl Driver { - fn new(sock: S, cfg: GuestConfig, inbox: Inbox) -> Self { + fn new(sock: S, cfg: GuestConfig, inbox: Inbox, now: Instant) -> Self { Self { sock, cfg, @@ -97,6 +99,7 @@ impl Driver { resume: Resuming::default(), pending: None, throttle: Throttle::new(PRESENCE_INTERVAL), + paced: Paced::new(now), reconnect_at: None, asking: None, done: false, @@ -105,7 +108,7 @@ impl Driver { /// Start joining (progress arrives in `inbox`). pub(crate) fn start(sock: S, cfg: GuestConfig, inbox: Inbox, now: Instant) -> Self { - let mut d = Self::new(sock, cfg, inbox); + let mut d = Self::new(sock, cfg, inbox, now); d.inbox.push(Event::Status(Status::Connecting)); d.connect(now); d @@ -114,7 +117,7 @@ impl Driver { /// Start with what `next` asks a server (a background request): the events it ends in go to /// `inbox`, then the join it ends in, if any, as `cfg` says. pub(crate) fn asking(sock: S, cfg: GuestConfig, next: exchange::Next, inbox: Inbox, now: Instant) -> Self { - let mut d = Self::new(sock, cfg, inbox); + let mut d = Self::new(sock, cfg, inbox, now); d.follow(next, now); d } @@ -159,6 +162,8 @@ impl Driver { fn connect(&mut self, now: Instant) { self.reconnect_at = None; + // A server counts per connection, and these were made for the last one. + self.paced = Paced::new(now); self.conn = Some(Conn::new(now)); if let Err(e) = self.sock.open(&self.server().url()) { self.dropped(e, now); @@ -303,13 +308,35 @@ impl Driver { if !self.conn.as_ref().is_some_and(|c| c.welcomed) { return; } - if !self.write(&msg) { + self.paced.push(msg); + if !self.send_paced(now) { self.end(Outcome::Retry("the connection dropped".into()), now); } } + /// Send the sync messages whose turn has come; false when one couldn't go. + fn send_paced(&mut self, now: Instant) -> bool { + while let Some(msg) = self.paced.take_due(now) { + if !self.write(&msg) { + return false; + } + } + true + } + + /// Reconnect now, if it is waiting to: the pauses grow from the beginning again. + pub(crate) fn retry(&mut self, now: Instant) { + if !self.done && self.reconnect_at.is_some() { + self.attempt = 0; + self.connect(now); + } + } + /// Leave the session. pub(crate) fn leave(&mut self, now: Instant) { + // What may go now still reaches the others. The rest would get the connection refused, and + // a page can't wait: the sync client says so when you leave. + let _ = self.send_paced(now); self.end(Outcome::Left, now); } @@ -327,7 +354,7 @@ impl Driver { [Some(c.last_ping + l.keepalive), Some(c.last_heard + c.silence_limit(l)), if c.welcomed { self.throttle.due() } else { None }] } }); - earliest(conn.into_iter().flatten().chain([self.reconnect_at])) + earliest(conn.into_iter().flatten().chain([self.reconnect_at, self.paced.due()])) } /// Do what's due at `now`: reconnect, send held presence or a ping, give up on a connection @@ -357,7 +384,7 @@ impl Driver { if now >= last_heard + silence { return self.end(Outcome::Retry("the host stopped answering".into()), now); } - let ok = self.throttle.take_due(now).is_none_or(|p| self.write(&ClientMsg::Presence(p))); + let ok = self.send_paced(now) && self.throttle.take_due(now).is_none_or(|p| self.write(&ClientMsg::Presence(p))); let ok = ok && (now < last_ping + l.keepalive || self.ping(now)); if !ok { self.end(Outcome::Retry("the connection dropped".into()), now); @@ -388,6 +415,8 @@ impl Driver { self.sock.close(); } self.throttle = Throttle::new(PRESENCE_INTERVAL); + // Made for this connection: the sync client sends again what the server didn't get. + self.paced = Paced::new(now); match after(outcome, &self.cfg, self.was_live, &mut self.attempt, &self.inbox) { Next::Stop(e) => { if let Some(e) = e { diff --git a/crates/collab-net/src/driver_tests.rs b/crates/collab-net/src/driver_tests.rs index 465bf4ff6..746c84bf6 100644 --- a/crates/collab-net/src/driver_tests.rs +++ b/crates/collab-net/src/driver_tests.rs @@ -252,7 +252,7 @@ fn a_silent_host_counts_as_dropped_and_the_client_reconnects() { } #[test] -fn reconnects_back_off_then_give_up() { +fn a_live_connection_reconnects_in_rounds_and_never_gives_up() { let t0 = Instant::now(); let (mut d, inbox) = start(Script::default(), t0); d.opened(t0); @@ -260,7 +260,7 @@ fn reconnects_back_off_then_give_up() { let mut now = t0; let mut pauses = Vec::new(); d.closed(1006, "", now); - while !d.done() { + for _ in 0..26 { let next = d.deadline().unwrap(); pauses.push(next - now); now = next; @@ -269,11 +269,183 @@ fn reconnects_back_off_then_give_up() { d.closed(1006, "", now); } let secs: Vec = pauses.iter().map(Duration::as_secs_f64).collect(); - assert_eq!(secs, [0.5, 1.0, 2.0, 4.0, 8.0, 10.0, 10.0, 10.0, 10.0, 10.0, 10.0, 10.0]); + let round = [1.0, 2.0, 4.0, 8.0, 10.0, 10.0, 10.0, 10.0, 10.0, 10.0, 10.0]; + assert_eq!(secs[0], 0.5); + assert_eq!(secs[1..12], round); + assert_eq!(secs[12], 60.0, "a longer pause after a round"); + assert_eq!(secs[13..24], round, "then as many again"); + assert_eq!(secs[24], 60.0); + assert!(!d.done()); let st = statuses(&inbox.drain()); - assert_eq!(st.len(), 14, "connecting, 12 reconnects, then failed"); - assert!(matches!(st.last(), Some(Status::Failed { reason }) if reason.starts_with("Lost the connection to the host")), "{st:?}"); - assert_eq!(d.deadline(), None); + assert!(!st.iter().any(|s| matches!(s, Status::Failed { .. })), "{st:?}"); + assert!(matches!(st.last(), Some(Status::Reconnecting { attempt: 27 })), "{st:?}"); +} + +#[test] +fn retry_now_skips_the_pause_and_starts_the_round_over() { + let t0 = Instant::now(); + let (mut d, inbox) = start(Script::default(), t0); + d.opened(t0); + d.message(&welcome(), t0); + d.closed(1006, "", t0); + let mut now = t0; + for _ in 0..12 { + now = d.deadline().unwrap(); + d.tick(now); + d.closed(1006, "", now); + } + assert_eq!(d.deadline().unwrap() - now, Duration::from_secs(60)); + let opened = d.sock.opened.len(); + d.retry(now); + assert_eq!(d.sock.opened.len(), opened + 1, "connects at once"); + inbox.drain(); + d.closed(1006, "", now); + assert_eq!(statuses(&inbox.drain()), vec![Status::Reconnecting { attempt: 1 }]); + assert_eq!(d.deadline().unwrap() - now, ms(500), "the pauses grow from the beginning again"); +} + +#[test] +fn a_session_that_was_live_only_stops_for_a_refusal_that_is_for_good() { + let t0 = Instant::now(); + // The words servers turn a session away with for now. + for reason in [ + "too many messages", + "the server is busy: try again in a moment", + "the session is full", + "no hello in time", + "this file can't be opened on the server (its data is damaged); tell the server's owner", + "too many connections from your address lately: wait a minute and try again", + ] { + for frame in [false, true] { + let (mut d, inbox) = start(Script::default(), t0); + d.opened(t0); + d.message(&welcome(), t0); + inbox.drain(); + if frame { + d.closed(1008, reason, t0); + } else { + d.message(&encode(&ServerMsg::Refused { reason: reason.into() }), t0); + } + assert!(!d.done(), "{reason}"); + assert_eq!(statuses(&inbox.drain()), vec![Status::Reconnecting { attempt: 1 }], "{reason}"); + } + } + // And the words for good. + for reason in [ + "this link isn't valid (it may have been revoked)", + "this file isn't on the server (it may have been removed)", + "this file was removed from the server", + "you were removed from this session", + "this server no longer accepts connections from your address", + "this server doesn't accept connections from your address", + "wrong session token", + "protocol 9 isn't supported", + "message too large: this server takes at most 64 MiB at once (a change that big can't be kept here)", + ] { + let (mut d, inbox) = start(Script::default(), t0); + d.opened(t0); + d.message(&welcome(), t0); + d.closed(1008, reason, t0); + assert!(d.done(), "{reason}"); + assert!(matches!(statuses(&inbox.drain()).last(), Some(Status::Failed { reason: r }) if r.contains(reason)), "{reason}"); + } + // What this end finds it can't talk to: a text frame, or a message it can't read. + for text in [true, false] { + let (mut d, inbox) = start(Script::default(), t0); + d.opened(t0); + d.message(&welcome(), t0); + if text { + d.text(t0); + } else { + d.message(&[0xFF, 1, 2, 3], t0); + } + assert!(d.done(), "text: {text}"); + assert!(matches!(statuses(&inbox.drain()).last(), Some(Status::Failed { .. }))); + } +} + +#[test] +fn a_long_queue_goes_out_paced_and_in_order() { + let t0 = Instant::now(); + let (mut d, _inbox) = start(Script::default(), t0); + d.opened(t0); + d.message(&welcome(), t0); + let before = d.sock.sent.len(); + for seq in 0..500 { + d.send(ClientMsg::Ack { seq }, t0); + } + let at_once = d.sock.sent.len() - before; + assert!((1..=200).contains(&at_once), "{at_once} at once: a server drops a connection past 400"); + let mut now = t0; + while d.sock.sent.iter().filter(|m| matches!(m, ClientMsg::Ack { .. })).count() < 500 { + now = d.deadline().unwrap(); + d.tick(now); + } + let seqs: Vec = d.sock.sent.iter().filter_map(|m| if let ClientMsg::Ack { seq } = m { Some(*seq) } else { None }).collect(); + assert_eq!(seqs, (0..500).collect::>()); + assert!(now - t0 >= Duration::from_secs(4), "the rest take their time: {:?}", now - t0); + // Unsent ones were made for this connection: the sync client sends them again. + for seq in 0..500 { + d.send(ClientMsg::Ack { seq }, now); + } + d.closed(1006, "", now); + assert_eq!(d.deadline().unwrap() - now, ms(500), "nothing left to send but the reconnect"); +} + +#[test] +fn messages_held_up_by_a_stall_dont_leave_together() { + let t0 = Instant::now(); + let (mut d, _inbox) = start(Script::default(), t0); + d.opened(t0); + d.message(&welcome(), t0); + for seq in 0..700 { + d.send(ClientMsg::Ack { seq }, t0); + } + let acks = |d: &Driver