Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Comment thread
matias-sanchez marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -1756,6 +1756,15 @@ static void push_msg_3p(site_def const *site, pax_machine *p, pax_msg *msg,
STRLIT(pax_op_to_str(msg->op)));
}

/* A reserved synode is only ours while we still hold the node index it was
reserved under. A view change that renumbers us hands that slot to another
node, which may reserve it as well. */
bool_t reservation_is_stale(synode_no msgno) {
site_def const *site = find_site_def(msgno);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to think a bit more about this, because I don't know exactly how find_site_def behaves for synodes that were still in the proposal phase. It works well for synodes that are already in the stream and accepted by the group.

That synode might still happen, but now necessarily with the data you are proposing. That is why we have checks like if (match_my_msg(ep->p->learner.msg, ep->client_msg->p))

node_no me = site ? get_nodeno(site) : VOID_NODE_NO;
return me != VOID_NODE_NO && me != msgno.node;
}

/* Brand client message with unique ID */
static void brand_client_msg(pax_msg *msg, synode_no msgno) {
assert(!synode_eq(msgno, null_synode));
Expand Down Expand Up @@ -2520,6 +2529,13 @@ static int proposer_task(task_arg arg) {

brand_client_msg(ep->client_msg->p, ep->msgno);

/* Only a locally allocated synode carries our own node index; a remote or
global allocation carries the allocating leader's, by design. */
if (ep->synode_allocation == synode_allocation_type::local &&

@jujose-1 jujose-1 Sep 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check covers a view change while reserve_synode_number() is suspended, which matches the reported trace.

However, wait_for_cache() below is another TASK_CALL and may suspend at TIMED_TASK_WAIT. If the member is renumbered during that wait, the reservation can become stale after this check.

Could we revalidate the local reservation immediately after wait_for_cache() succeeds—after the null check and before locking or modifying the Paxos machine?

Keeping both checks would avoid waiting on an already stale reservation while also establishing ownership after the final possible yield.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jujose-1 huge thanks on your review.
Good catch, thanks. Added the revalidation right after wait_for_cache(), after the null check, and kept the first check as you suggested.
I tested this over a 3 hours runs with rolling restarts under load and I didn't see any meaningful impact on throughput, with the GNO mismatch not reproducing. Feels safe.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi!

Just to add a note here... the loop that follows is the main loop of the proposer, where each of the 8 proposers are waiting for the message to be accepted.

In short, I don't think that this check makes sense here, and maybe the one inside the loop is enough, because even with view changes, we keep the proposals active. They will end up being accepted in another configuration, but it will avoid the said problem.

reservation_is_stale(ep->msgno)) {
GOTO(retry_new);
}

for (;;) { /* Loop until the client message has been learned */
/* Get a Paxos instance to send the client message */

Expand All @@ -2530,6 +2546,13 @@ static int proposer_task(task_arg arg) {
goto retry_new;
}

/* Checked again after wait_for_cache: that call can suspend, and a view
change during the wait leaves the reservation stale. */
if (ep->synode_allocation == synode_allocation_type::local &&
reservation_is_stale(ep->msgno)) {
GOTO(retry_new);
}

assert(ep->p);
if (ep->client_msg->p->force_delivery)
ep->p->force_delivery = ep->client_msg->p->force_delivery;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ void *xcom_thread_main(void *cp);

synode_no incr_synode(synode_no synode);

bool_t reservation_is_stale(synode_no msgno);

synode_no decr_synode(synode_no synode);

char *dbg_pax_msg(pax_msg const *p);
Expand Down
1 change: 1 addition & 0 deletions unittest/gunit/libmysqlgcs/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ SET(GCS_XCOM_TESTS
xcom/gcs_xcom_xcom_transport
xcom/gcs_xcom_communication_protocol_changer
xcom/gcs_xcom_xcom_cache
xcom/gcs_xcom_stale_reservation
xcom/gcs_xcom_control_interface
xcom/gcs_xcom_view_identifier
xcom/gcs_message_stage_fragmentation
Expand Down
100 changes: 100 additions & 0 deletions unittest/gunit/libmysqlgcs/xcom/gcs_xcom_stale_reservation-t.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
/* Copyright (c) 2026, Oracle and/or its affiliates.

This program is free software; you can redistribute it and/or modify
it under the terms of the GNU General Public License, version 2.0,
as published by the Free Software Foundation.

This program is designed to work with certain software (including
but not limited to OpenSSL) that is licensed under separate terms,
as designated in a particular file or component or in included license
documentation. The authors of MySQL hereby grant you an additional
permission to link the program and your derivative works with the
separately licensed software that they have either included with
the program or referenced in the documentation.

This program is distributed in the hope that it will be useful,
but WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
GNU General Public License, version 2.0, for more details.

You should have received a copy of the GNU General Public License
along with this program; if not, write to the Free Software
Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA */

#include <xcom/xcom_profile.h>
#include <xcom_vp.h>
#include "gcs_base_test.h"

#include "site_def.h"
#include "xcom_base.h"

namespace xcom_stale_reservation_unittest {

/* Bug#121063: a synode reserved under one node index must be recognised as no
longer ours once a view change gives this member a different index. */
class XcomStaleReservation : public GcsBaseTest {
protected:
void SetUp() override {
m_addr = new std::string("127.0.0.1:12345");
char const *names[]{m_addr->c_str()};
m_na = new_node_address(1, names);

/* The view in force when the synode is reserved: this member is node 1. */
m_before = new_site_def();
init_site_def(1, m_na, m_before);
m_before->start = m_synode_before;
m_before->nodeno = 1;
Comment thread
matias-sanchez marked this conversation as resolved.
Outdated
push_site_def(m_before);

/* The view installed while the reservation is held: the member is now
node 0. */
m_after = new_site_def();
init_site_def(1, m_na, m_after);
m_after->start = m_synode_after;
m_after->nodeno = 0;
push_site_def(m_after);
}

void TearDown() override {
push_site_def(nullptr);
free_site_defs();
delete_node_address(1, m_na);
delete m_addr;
}

std::string *m_addr{nullptr};
node_address *m_na{nullptr};
site_def *m_before{nullptr};
site_def *m_after{nullptr};

/* A view is active from its start synode on, so a slot below 20 falls under
the old view and one at or above it under the new one. */
synode_no const m_synode_before{1, 10, 0};
synode_no const m_synode_after{1, 20, 0};
};

/* Under the view it was taken in, the reservation is ours. */
TEST_F(XcomStaleReservation, reservation_under_the_current_index_is_fresh) {
synode_no const reserved{1, 15, 1};
ASSERT_FALSE(reservation_is_stale(reserved));
}

/* After the renumbering, the same index belongs to another node. */
TEST_F(XcomStaleReservation, reservation_outliving_a_renumbering_is_stale) {
synode_no const reserved{1, 25, 1};
ASSERT_TRUE(reservation_is_stale(reserved));
}

/* A slot that carries the index this member holds now is usable. */
TEST_F(XcomStaleReservation, slot_matching_the_new_index_is_fresh) {
synode_no const reserved{1, 25, 0};
ASSERT_FALSE(reservation_is_stale(reserved));
}

/* Without a site there is nothing to compare against. */
TEST_F(XcomStaleReservation, unknown_site_is_not_reported_stale) {
synode_no const other_group{99, 25, 1};
ASSERT_FALSE(reservation_is_stale(other_group));
}

} // namespace xcom_stale_reservation_unittest
Loading