fix: slotsfull is occupancy, not free slots, and cover it end to end
Running the e2e suite against a real soulfind showed the per-member slots field was implemented backwards. The wire field is `slotsfull` -- a boolean that is true when the member's upload slots are all occupied -- not a count of free ones. `slots_free: u32` named it, documented it and printed it as the opposite of what the server means, so a caller reading it would pick the busiest peer to download from. - types: `slots_full: bool`, with the wire name in the doc comment - the DTO, the `room users` output and SKILL.md follow - a new e2e test joins a room with a member sharing a known three files and asserts the counts, the roster alignment and the slot flag against soulfind, so the parsing is checked against a real server rather than only against frames this repo wrote itself
This commit is contained in:
@@ -632,7 +632,7 @@ fn room_member_stats_are_returned_sorted_and_scoped_to_their_room() {
|
||||
average_speed: 1,
|
||||
shared_files: 2,
|
||||
shared_folders: 3,
|
||||
slots_free: 1,
|
||||
slots_full: true,
|
||||
country: None,
|
||||
};
|
||||
|
||||
@@ -660,7 +660,7 @@ fn rejoining_replaces_the_previous_member_stats() {
|
||||
average_speed: 1,
|
||||
shared_files: 2,
|
||||
shared_folders: 3,
|
||||
slots_free: 0,
|
||||
slots_full: false,
|
||||
country: None,
|
||||
};
|
||||
|
||||
|
||||
@@ -59,7 +59,8 @@ fn read_int32s(message: &mut Message) -> Vec<u32> {
|
||||
|
||||
/// Parse the per-member statistics that follow the membership list of a
|
||||
/// `JoinRoom` (code 14) reply: statuses, then a record of share statistics per
|
||||
/// member, then free upload slots, then country codes.
|
||||
/// member, then whether each member's upload slots are full, then country
|
||||
/// codes.
|
||||
///
|
||||
/// Each vector is independently length-prefixed and a server may send fewer
|
||||
/// entries than there are members (or, on an older server, omit the country
|
||||
@@ -87,7 +88,7 @@ fn parse_member_stats(
|
||||
shares.push((average_speed, shared_files, shared_folders));
|
||||
}
|
||||
|
||||
let slots_free = read_int32s(message);
|
||||
let slots_full = read_int32s(message);
|
||||
let countries = read_strings(message);
|
||||
|
||||
users
|
||||
@@ -104,7 +105,7 @@ fn parse_member_stats(
|
||||
average_speed,
|
||||
shared_files,
|
||||
shared_folders,
|
||||
slots_free: slots_free.get(i).copied().unwrap_or(0),
|
||||
slots_full: slots_full.get(i).copied().unwrap_or(0) != 0,
|
||||
country: countries.get(i).cloned().filter(|c| !c.is_empty()),
|
||||
}
|
||||
})
|
||||
@@ -156,7 +157,7 @@ mod tests {
|
||||
.write_int64(0)
|
||||
.write_int32(10)
|
||||
.write_int32(2)
|
||||
// free slots
|
||||
// slots full
|
||||
.write_int32(2)
|
||||
.write_int32(1)
|
||||
.write_int32(0)
|
||||
@@ -188,7 +189,7 @@ mod tests {
|
||||
average_speed: 1024,
|
||||
shared_files: 4321,
|
||||
shared_folders: 77,
|
||||
slots_free: 1,
|
||||
slots_full: true,
|
||||
country: Some("NL".to_string()),
|
||||
},
|
||||
RoomUserStats {
|
||||
@@ -197,7 +198,7 @@ mod tests {
|
||||
average_speed: 50,
|
||||
shared_files: 10,
|
||||
shared_folders: 2,
|
||||
slots_free: 0,
|
||||
slots_full: false,
|
||||
country: None,
|
||||
},
|
||||
]
|
||||
|
||||
@@ -341,8 +341,10 @@ pub struct RoomUserStats {
|
||||
pub average_speed: u32,
|
||||
pub shared_files: u32,
|
||||
pub shared_folders: u32,
|
||||
/// Upload slots the member has free; zero means they are busy.
|
||||
pub slots_free: u32,
|
||||
/// True when the member's upload slots are all occupied, so a download
|
||||
/// from them will queue. The wire field is `slotsfull`, a boolean — not a
|
||||
/// count, and not a count of *free* slots.
|
||||
pub slots_full: bool,
|
||||
/// Two-letter country code, when the server reports one.
|
||||
pub country: Option<String>,
|
||||
}
|
||||
|
||||
@@ -2622,3 +2622,84 @@ fn a_peer_that_hangs_up_its_control_connection_still_delivers_the_file() {
|
||||
|
||||
let _ = std::fs::remove_dir_all(&download_dir);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_joined_room_reports_its_members_statistics() {
|
||||
let server = server_or_skip!();
|
||||
|
||||
let room = "e2e_room_stats";
|
||||
|
||||
// A member with a known share, so the statistics are checkable rather
|
||||
// than merely present.
|
||||
let share_dir = unique_download_dir().join("stats_share");
|
||||
std::fs::create_dir_all(share_dir.join("album")).unwrap();
|
||||
for name in ["one.flac", "two.flac", "three.flac"] {
|
||||
std::fs::write(share_dir.join("album").join(name), b"xxxx").unwrap();
|
||||
}
|
||||
let mut sharer = Client::with_settings(ClientSettings {
|
||||
shared_directories: vec![share_dir.display().to_string()],
|
||||
..server.settings("e2e_stats_sharer", "pw")
|
||||
});
|
||||
sharer.connect().expect("sharer connect");
|
||||
assert!(sharer.login().expect("sharer login"));
|
||||
sharer.join_room(room).expect("sharer joins room");
|
||||
|
||||
// Let the server register the sharer's membership and share counts, so
|
||||
// the observer's own join reply describes a room that already has them.
|
||||
std::thread::sleep(Duration::from_millis(750));
|
||||
|
||||
let mut observer =
|
||||
Client::with_settings(server.settings("e2e_stats_observer", "pw"));
|
||||
observer.connect().expect("observer connect");
|
||||
assert!(observer.login().expect("observer login"));
|
||||
observer.join_room(room).expect("observer joins room");
|
||||
|
||||
// The stats ride along with the membership list, so poll for the member
|
||||
// rather than for a fixed delay.
|
||||
let deadline = Instant::now() + Duration::from_secs(15);
|
||||
let mut sharer_stats = None;
|
||||
while Instant::now() < deadline {
|
||||
if let Some(found) = observer
|
||||
.room_member_stats(room)
|
||||
.into_iter()
|
||||
.find(|s| s.username == "e2e_stats_sharer")
|
||||
{
|
||||
sharer_stats = Some(found);
|
||||
break;
|
||||
}
|
||||
std::thread::sleep(Duration::from_millis(100));
|
||||
}
|
||||
let stats =
|
||||
sharer_stats.expect("the join reply should describe the members");
|
||||
|
||||
assert!(
|
||||
stats.status.is_reachable(),
|
||||
"a member who is in the room cannot read as offline, got {}",
|
||||
stats.status
|
||||
);
|
||||
assert_eq!(
|
||||
stats.shared_files, 3,
|
||||
"the member's file count must survive the parallel stat vectors"
|
||||
);
|
||||
assert_eq!(stats.shared_folders, 1);
|
||||
assert!(
|
||||
!stats.slots_full,
|
||||
"a client that never filled its slots must not read as full"
|
||||
);
|
||||
|
||||
// Every member is described, not just the one we looked up: a short or
|
||||
// misread vector would drop or shift the rest.
|
||||
let members = observer.room_member_stats(room);
|
||||
let names: Vec<&str> =
|
||||
members.iter().map(|s| s.username.as_str()).collect();
|
||||
assert!(
|
||||
names.contains(&"e2e_stats_sharer")
|
||||
&& names.contains(&"e2e_stats_observer"),
|
||||
"both members should be described, got {names:?}"
|
||||
);
|
||||
assert_eq!(
|
||||
members.len(),
|
||||
observer.room_members(room).len(),
|
||||
"the stats must line up one-to-one with the roster"
|
||||
);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user