message_fetch: Fix get_frontfill_anchor fallback.

This fixes a very rare exception seen in production, which the
previous assertion allowed us to understand was possible in a rare
race, now documented in comments.
This commit is contained in:
Tim Abbott 2023-11-08 11:46:12 -08:00
parent 8c8dbb3d66
commit 39fb1a0f70
2 changed files with 84 additions and 55 deletions

View File

@ -408,12 +408,30 @@ export function get_frontfill_anchor(msg_list) {
return last_msg.id;
}
// Although it is impossible that we reach here since we
// are already checking `msg_list.fetch_status.can_load_newer_messages`
// and user cannot be scrolling down on an empty message_list to
// fetch more data, and if user is, then the available data is wrong
// and we raise a fatal error.
throw new Error("There are no message available to frontfill.");
// This fallthrough only occurs in a rare race, where the user
// navigates to a currently empty narrow, and the `GET /messages`
// request sees 0 matching messages, but loses the race with a
// simultaneous `GET /events` request returning a just-sent
// message matching this narrow. In that case,
// get_messages_success will see no matching messages, even though
// we know via `FetchStatus._expected_max_message_id` that we are
// expecting to see a new message here, and thus
// `FetchStatus.has_found_newest` remains false.
//
// In this situation, we know there are no messages older than the
// ones we're looking for, so returning "oldest" should correctly
// allow the follow-up request to find all messages that raced in
// this way.
//
// Can be manually reproduced as follows:
// * Add a long sleep at the end of `GET /messages` API requests
// in the server.
// * Open two browser windows.
// * Narrow to an empty topic in the first. You'll see a loading indicator.
// * In the second window, send a message to the empty topic.
// * When the first browser window's `GET /messages` request finishes,
// this code path will be reached.
return "oldest";
}
export function maybe_load_older_messages(opts) {

View File

@ -324,44 +324,52 @@ run_test("loading_newer", () => {
can_call_again: true,
});
// The msg_list is empty and we are calling frontfill, which should
// raise fatal error.
if (opts.empty_msg_list) {
assert.throws(
() => {
message_fetch.maybe_load_newer_messages({
msg_list,
show_loading: noop,
hide_loading: noop,
});
},
{
name: "Error",
message: "There are no message available to frontfill.",
},
);
} else {
message_fetch.maybe_load_newer_messages({
msg_list,
show_loading: noop,
hide_loading: noop,
});
message_fetch.maybe_load_newer_messages({
msg_list,
show_loading: noop,
hide_loading: noop,
});
test_dup_new_fetch(msg_list);
test_dup_new_fetch(msg_list);
test_fetch_success({
fetch,
response: data.resp,
});
}
test_fetch_success({
fetch,
response: data.resp,
});
}
(function test_narrow() {
const msg_list = simulate_narrow();
let msg_list = simulate_narrow();
page_params.unread_msgs = {
old_unreads_missing: true,
};
// Test what happens when an empty list is returned with found_newest false.
const empty_list_data = {
req: {
anchor: "oldest",
num_before: 0,
num_after: 100,
narrow: `[{"negated":false,"operator":"dm","operand":[${alice.user_id}]}]`,
client_gravatar: true,
},
resp: {
messages: message_range(500, 600),
found_newest: false,
},
};
test_happy_path({
msg_list,
data: empty_list_data,
});
msg_list = simulate_narrow();
msg_list.append_to_view = () => {};
// Instead of using 444 as page_param.pointer, we
// should have a message with that id in the message_list.
msg_list.append(message_range(444, 445), false);
const data = {
req: {
anchor: "444",
@ -379,18 +387,6 @@ run_test("loading_newer", () => {
test_happy_path({
msg_list,
data,
empty_msg_list: true,
});
msg_list.append_to_view = () => {};
// Instead of using 444 as page_param.pointer, we
// should have a message with that id in the message_list.
msg_list.append(message_range(444, 445), false);
test_happy_path({
msg_list,
data,
empty_msg_list: false,
});
assert.equal(msg_list.data.fetch_status.can_load_newer_messages(), true);
@ -416,7 +412,28 @@ run_test("loading_newer", () => {
(function test_home() {
reset_lists();
const msg_list = message_lists.home;
let msg_list = message_lists.home;
// Test what happens when an empty list is returned with found_newest false.
const empty_list_data = {
req: {
anchor: "oldest",
num_before: 0,
num_after: 100,
client_gravatar: true,
},
resp: {
messages: message_range(500, 600),
found_newest: false,
},
};
test_happy_path({
msg_list,
data: empty_list_data,
});
reset_lists();
const data = [
{
@ -445,18 +462,12 @@ run_test("loading_newer", () => {
},
];
test_happy_path({
msg_list,
data: data[0],
empty_msg_list: true,
});
msg_list = message_lists.home;
all_messages_data.append(message_range(444, 445), false);
test_happy_path({
msg_list,
data: data[0],
empty_msg_list: false,
});
assert.equal(msg_list.data.fetch_status.can_load_newer_messages(), true);