iframe-proxy

BGQ99 · 2026-10-19T09:38:57Z

What problem does this PR solve?

Issue Number: Related to #3428

Problem Summary:

URMA send completion handling publishes the reclaimed SQ credit before clearing the corresponding IOBuf. A concurrent writer can observe the available credit and reuse the ring slot while the completion handler is still clearing it, potentially causing concurrent IOBuf modification and a crash during buffer release.

What is changed and the side effects?

Changed:

Clear the completed send buffer and advance the completion cursor before publishing the SQ credit, following the reclamation order used by the RDMA endpoint.
Move the existing memory barrier before the credit update.
Check window capacity before reclamation so an invalid completion cannot clear a buffer or advance the cursor.
Add regression tests for reclamation order, cursor wraparound, and rejection of a completion when the SQ window is already full.

Validation:

CI passed in the LinQuickDev/brpc repository.
Full CMake build with WITH_URMA=ON and URMA_USE_MOCK=ON passed.
All 17 URMA unit tests passed, including the 3 new tests.
The reclamation-order test fails as expected with the original upstream implementation.
git diff --check passed.
Real URMA hardware validation of this upstream-based revision and ASAN were not run locally.

Side effects:

Performance effects: adds a capacity check before reclamation. No performance improvement is claimed.
Breaking backward compatibility: none intended. No public API or wire protocol changes. Receive processing and ACK-only completions are unchanged.

Check List:

Please make sure your changes are compilable.
When providing us with a new feature, it is best to add related tests.
Please follow Contributor Covenant Code of Conduct.

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

2 open issues

These new tests reach deeply into UrmaEndpoint internals (_sbuf, _sq_size, _sq_sent,… · New
These new tests reach deeply into UrmaEndpoint internals (_sbuf, _sq_size, _sq_sent,… · New

What changed in this PR

Fixes a race in URMA send completion handling by reclaiming/clearing the completed send buffer before publishing SQ credits, preventing a concurrent writer from reusing a slot that is still being cleared.

Changes:

Reorders send-buffer reclamation vs. SQ-credit publication (and moves the memory barrier accordingly).
Adds a capacity guard to reject invalid completions when the SQ window is already full.
Adds regression tests covering reclamation order, cursor wraparound, and full-window rejection.

_{Review effort: Lite}

BGQ99 · 2026-10-21T07:22:17Z

+    endpoint._sbuf.resize(endpoint._sq_size -
+                          urma::UrmaEndpoint::RESERVED_WR_NUM);
+    endpoint._local_window_capacity = endpoint._sbuf.size();
+    endpoint._sq_window_size.store(0, butil::memory_order_relaxed);
+    ASSERT_EQ(0, endpoint._sbuf[0].append_user_data(
+        &payload, sizeof(payload), [&](void*) {
+            released = true;
+            // Observe the credit at the exact point the old buffer is freed.
+            // Publishing first lets the writer reuse a slot still being cleared.
+            EXPECT_EQ(0, endpoint._sq_window_size.load(
+                             butil::memory_order_relaxed));
+        }));


These tests use the existing test build’s -fno-access-control flag, without changing production member visibility. Direct access lets the ordering test observe the SQ credit from the buffer deleter at the exact point of reclamation. Adding FRIEND_TEST would make access explicit, but would not remove the tests’ dependence on these internals. I’ve kept the existing test convention for this focused regression fix.

BGQ99 · 2026-10-21T07:22:30Z

+    ASSERT_EQ(0, endpoint.HandleCompletion(SendCompletion()));
+    EXPECT_TRUE(released);
+    EXPECT_TRUE(endpoint._sbuf[0].empty());
+    EXPECT_EQ(1, endpoint._sq_sent);


Same rationale as above; this duplicates the other comment.

wwbmmm

[replied by brpc-oncall robot] The reclamation-order change in HandleCompletion looks correct and consistent with the RDMA endpoint, but the new unit tests cannot compile when BRPC_WITH_URMA is enabled because they access private UrmaEndpoint members; also the capacity check is now duplicated.

wwbmmm · 2026-10-20T07:49:09Z

+    char payload = 'x';
+    bool released = false;
+    urma::UrmaEndpoint endpoint(_socket.get());
+    endpoint._sbuf.resize(endpoint._sq_size -


[replied by brpc-oncall robot] These tests cannot compile when the tree is built with WITH_URMA=ON: _sbuf, _sq_size, _sq_sent, _sq_window_size, _local_window_capacity and HandleCompletion() are all private in UrmaEndpoint (see src/brpc/urma/urma_endpoint.h: everything after the private: section that starts at the State enum), and the fixture is not declared as a friend. CI does not catch this because test/brpc_urma_unittest.cpp compiles to an empty translation unit unless BRPC_WITH_URMA is 1. Please grant access explicitly, e.g. add #include <gtest/gtest_prod.h> plus FRIEND_TEST(UrmaSendCompletionTest, reclaim_buffer_before_publishing_credit); (and the other two test names) in UrmaEndpoint, or expose a small test-only accessor, so that the URMA unit-test build actually compiles.

Thanks for checking. The existing test build disables C++ access checks with -fno-access-control. In CMake, this is set on brpc_test_config, which the URMA unit-test target links against; test/Makefile includes the same flag in CXXFLAGS. Therefore, these private-member accesses are permitted by the existing CMake and Make test configurations, including when URMA is enabled.

wwbmmm · 2026-10-20T07:49:09Z

        uint16_t wnd = 1;  // We signal every WR (complete_enable=1).
        uint16_t old =
            _sq_window_size.load(butil::memory_order_relaxed);
+        if (old >= _local_window_capacity) {


[replied by brpc-oncall robot] This capacity check plus its warning is now duplicated: the same check is still inside the while (true) CAS loop below. The in-loop branch is now only reachable after the buffer has already been cleared and _sq_sent advanced, in which case the credit is dropped while the state was already mutated. Since _sq_window_size is only incremented here (writers only decrement it), consider doing the single pre-check and then publishing with _sq_window_size.fetch_add(wnd, butil::memory_order_relaxed) after the barrier, removing the now-redundant retry loop so reclamation and credit publication stay in one place.

Updated in 45a7cfb. Kept the capacity check before buffer reclamation and replaced the subsequent CAS loop with fetch_add, preserving the memory barrier before credit publication.

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

6 open issues

Changing the SQ credit update from a CAS loop to load + fetch_add removes the atomic capacity… · New
The capacity guard compares old against capacity, but the log message includes an increment and… · New
These tests directly read/write UrmaEndpoint internals (e.g., _sbuf, _sq_size,… · New
These new tests reach deeply into UrmaEndpoint internals (_sbuf, _sq_size, _sq_sent,…
These new tests reach deeply into UrmaEndpoint internals (_sbuf, _sq_size, _sq_sent,…
To make the errno assertion more robust, explicitly set errno = 0 immediately before calling… · New

_{Review effort: Lite}

        uint16_t wnd = 1;  // We signal every WR (complete_enable=1).
        uint16_t old =
            _sq_window_size.load(butil::memory_order_relaxed);
-        while (true) {
-            if (old >= _local_window_capacity) {
-                LOG(WARNING)
-                    << "URMA send completion exceeds SQ window: old=" << old
-                    << " increment=" << wnd
-                    << " capacity=" << _local_window_capacity
-                    << " user_ctx=" << cr.user_ctx
-                    << " on " << _socket->description();
-                errno = EPROTO;
-                return -1;
-            }
-            if (_sq_window_size.compare_exchange_weak(
-                    old, static_cast<uint16_t>(old + wnd),
-                    butil::memory_order_relaxed)) {
-                break;
-            }
+        if (old >= _local_window_capacity) {
+            LOG(WARNING)
+                << "URMA send completion exceeds SQ window: old=" << old
+                << " increment=" << wnd
+                << " capacity=" << _local_window_capacity
+                << " user_ctx=" << cr.user_ctx
+                << " on " << _socket->description();
+            errno = EPROTO;
+            return -1;
        }
        for (uint16_t i = 0; i < wnd; ++i) {
            _sbuf[_sq_sent].clear();
            _sq_sent = (_sq_sent + 1) % (_sq_size - RESERVED_WR_NUM);
        }
        butil::subtle::MemoryBarrier();
+        // Reclaim the buffer before publishing the SQ credit. Otherwise the
+        // writer may reuse this slot while the completion thread clears it.
+        _sq_window_size.fetch_add(wnd, butil::memory_order_relaxed);


        uint16_t wnd = 1;  // We signal every WR (complete_enable=1).
        uint16_t old =
            _sq_window_size.load(butil::memory_order_relaxed);
-        while (true) {
-            if (old >= _local_window_capacity) {
-                LOG(WARNING)
-                    << "URMA send completion exceeds SQ window: old=" << old
-                    << " increment=" << wnd
-                    << " capacity=" << _local_window_capacity
-                    << " user_ctx=" << cr.user_ctx
-                    << " on " << _socket->description();
-                errno = EPROTO;
-                return -1;
-            }
-            if (_sq_window_size.compare_exchange_weak(
-                    old, static_cast<uint16_t>(old + wnd),
-                    butil::memory_order_relaxed)) {
-                break;
-            }
+        if (old >= _local_window_capacity) {


+    urma::UrmaEndpoint endpoint(_socket.get());
+    endpoint._sbuf.resize(endpoint._sq_size -
+                          urma::UrmaEndpoint::RESERVED_WR_NUM);
+    endpoint._local_window_capacity = endpoint._sbuf.size();
+    endpoint._sq_window_size.store(0, butil::memory_order_relaxed);


+    EXPECT_EQ(-1, endpoint.HandleCompletion(SendCompletion()));
+    EXPECT_EQ(EPROTO, errno);


Fix URMA send buffer reclamation order

6c05373

wwbmmm requested a balanced review from Copilot October 19, 2026 12:50

Copilot AI reviewed Oct 19, 2026

View reviewed changes

Copilot stopped reviewing on behalf of wwbmmm due to an error October 19, 2026 14:02
The job was not acquired by a runner even after multiple attempts.

wwbmmm reviewed Oct 20, 2026

View reviewed changes

Simplify URMA send completion credit update

45a7cfb

wwbmmm requested a balanced review from Copilot October 21, 2026 09:08

Copilot AI reviewed Oct 21, 2026

View reviewed changes

Copilot stopped reviewing on behalf of wwbmmm due to an error October 21, 2026 11:45
Workflow completed but conclusion is not available. This may indicate rate limiting or API issues.

File	Description
test/brpc_urma_unittest.cpp	Adds regression tests that validate the new reclamation order and edge cases (wraparound, full window).
src/brpc/urma/urma_endpoint.cpp	Reorders buffer clearing and SQ credit publication and adds a capacity check to reject overflow completions.

Sunbelt Computer Software

PL/B Language Development and Support

Navigation Menu

Repository navigation

Fix URMA send buffer reclamation order - #3626

Fix URMA send buffer reclamation order#3626
BGQ99 wants to merge 2 commits into
apache:masterfrom
LinQuickDev:fix/urma-send-buffer-reclaim

BGQ99 commented Oct 19, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

BGQ99 Oct 21, 2026

Uh oh!

BGQ99 Oct 21, 2026

Uh oh!

wwbmmm left a comment

Uh oh!

wwbmmm Oct 20, 2026

Uh oh!

BGQ99 Oct 20, 2026

Uh oh!

wwbmmm Oct 20, 2026

Uh oh!

BGQ99 Oct 21, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		EXPECT_EQ(-1, endpoint.HandleCompletion(SendCompletion()));
		EXPECT_EQ(EPROTO, errno);

Sunbelt Computer Software

PL/B Language Development and Support

Conversation

BGQ99 commented Oct 19, 2026

What problem does this PR solve?

What is changed and the side effects?

Check List:

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

BGQ99 Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

BGQ99 Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

wwbmmm left a comment

Choose a reason for hiding this comment

Uh oh!

wwbmmm Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

BGQ99 Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

wwbmmm Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

BGQ99 Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants