Fix SelectiveChannel handling of late subcall callbacks

[altman08]

What problem does this PR solve?

Issue Number: N/A

Problem Summary:

Late SelectiveChannel subcall callbacks can continue processing results after the main RPC has finished, as long as other subcalls remain outstanding. This can modify the completed RPC result and, in the older version identified by the core dump, trigger a retry that dereferences a cleared load balancer.

The issue was identified from core-mtable-stablete-270416-1790555285, generated by mtable-stabletest while calling mtable.tableserver.Sdk.ReadTable.

The main RPC used a 1000ms timeout, a 996ms backup request delay, and a maximum of three retries. Its custom MTableRetryPolicy allowed retries for error 5013 (PARTITION_NOT_READY).

The failure sequence was:

1. The request reached the backup request timeout, resulting in two allocated subcall resources. A subcall returned E5013 and triggered a retry, still subject to the main RPC’s original 1000ms deadline.
2. The main RPC timed out with ERPCTIMEDOUT (1008). In the affected older version, Controller::EndRPC() cleared the main controller’s _lb before invoking Sender::Run().
3. Sender::Run() set _finished=true. Since subcalls were still outstanding, it released the main call-id lock, signaled cancellation to the subcalls, and waited for their resources to be returned.
4. A late subcall callback carrying E5013 acquired the main call-id lock. Its saved_error was 1008, confirming that the main RPC had already timed out.
5. PushFree() returned the subcall resource, leaving _nfree=1 and _nalloc=2. The previous implementation returned false only when _finished && _nfree == _nalloc. Consequently, this callback was allowed to continue processing its result even though _finished was already true.
6. SubDone::Run() wrote E5013 into the main controller and called OnVersionedRPCReturned(). The subcall’s correlation ID matched the current call, so the stale-call filtering did not reject it.
7. MTableRetryPolicy::DoRetry() accepted E5013. The retry count increased from two to three, and execution entered Sender::IssueRPC().
8. In the affected older version, Sender::IssueRPC() accessed the main controller’s already-cleared _lb. The faulting instruction was mov 0x18(%rbx), %rdi, with RBX=0, causing a SIGSEGV at address 0x18.

The core contained the following state:

Field	Value
Sender _finished	true
Sender _nfree / _nalloc	1 / 2
Main controller _lb	nullptr
Main error saved by the subcall callback	1008 (ERPCTIMEDOUT)
Late subcall error	5013 (PARTITION_NOT_READY)
Retry count at the crash	3

The main controller’s error text also recorded E5013 after the timeout:

[E1007]Reached backup timeout=996ms
[R1][E5013][10.233.187.5:8000][E5013]failed
[R2][E1008]Reached timeout=1000ms
[R2][E5013][10.233.187.5:8000][E5013]failed

The retry start timestamp was 3μs after the main RPC’s end timestamp and 75μs after its deadline. These timestamps support the failure sequence, although they do not constitute a complete thread execution trace. The main controller’s _error_code=0 at the crash resulted from clearing the error for the new retry; it did not indicate a successful RPC.

The current repository already has an ending-state guard and a separate load balancer reference in Sender, which protect against the same retry/crash path. However, late callbacks can still modify the main controller’s error text, response, and attachment before reaching that guard. This PR fixes the underlying callback handling issue.

What is changed and the side effects?

Changed:

• Make Sender::PushFree() stop result processing whenever _finished is true, regardless of whether all subcall resources have been returned.
• Continue returning resources from late callbacks. Release the main call-id lock when callbacks remain outstanding, and let the final callback execute Clear().
• Return immediately after unlocking or cleanup, since either operation may allow the Sender, SubDone, or main controller to be destroyed.
• Preserve normal subcall error propagation and retry behavior while the main RPC remains active.
• Add three deterministic regression tests covering:
  • Late callbacks after timeout or cancellation, with retryable errors, non-retryable errors, or successful results.
  • Both callback orders and controller destruction inside the user completion callback.
  • Retryable and non-retryable subcall errors before the main RPC finishes.

Validation:

• All 87 tests in brpc_channel_unittest passed.
• The late-callback regression test failed against the original implementation, confirming that it detects the existing issue.
• The before-completion error propagation and retry tests passed with both implementations.

Side effects:

• Performance effects: No significant impact expected. Late callbacks skip unnecessary result processing.
• Breaking backward compatibility: None. No public API changes; active RPC error propagation and retry behavior are preserved.

---

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 findings

• This test asserts an internal invariant (Controller::_lb being cleared) which is not part of the… · New
• RetryPolicy::DoRetry may be called concurrently in real usage, and mutating calls inside a… · New

What changed in this PR

Fixes SelectiveChannel behavior so late subcall callbacks cannot continue modifying the main RPC result after the main RPC has already finished, while still allowing late callbacks to safely return subcall resources.

Changes:

• Update Sender::PushFree() to immediately stop result processing once _finished is true (but still allow resource return/cleanup).
• Add deterministic unit tests covering late callback ordering, retryable/non-retryable/success results, and controller destruction inside user done callbacks.
• Add tests ensuring retries and error propagation still behave normally before the main RPC completes.

File	Description
test/​brpc_channel_unittest.cpp	Adds regression and behavior-preservation tests for late subcall callbacks and pre-completion retry/error handling.
src/​brpc/​selective_channel.cpp	Changes Sender::PushFree() logic to stop processing late results after finish while still draining subcall resources safely.

🧠 Review effort: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[yanglimingcn]

LGTM