[chenBright]

What problem does this PR solve?

Issue Number: resolve

Problem Summary:

Removing a failed socket can race with health-check revival and leak an additional reference.

The leak occurs because removal assumes the additional reference is already gone, but health
checking restores it afterward, leaving nobody responsible for releasing it.

Let:

- A = additional reference.
- H = the owner’s health-check reference.
- T = the health-check task’s reference.

Initially, the socket is already failed. SetFailed() has released A, leaving H + T = 2.

Removal thread                      Health-check thread              References
      |                                     |
      | ReleaseAdditionalReference()        |
      | A was already released; return      |                        2 = H + T
      |                                     |
      | H has not been released yet         | Revive(2)
      |                                     | Reference count is 2
      |                                     | Revive and restore A   3 = H + T + A
      |                                     |
      | ReleaseHCRelatedReference()         |
      | Release H                           |                        2 = T + A
      |                                     |
      |                                     | Health-check task ends
      |                                     | Release T              1 = A
      v                                     v

The owner has removed the socket, and the health-check task has exited.
Nobody releases A, so the reference count never reaches zero.

The original state only recorded that A had been released. It did not distinguish temporary 
failure from permanent removal. A reference count of 2 does not establish that the owner 
still wants the socket.

What is changed and the side effects?

Changed:

Distinguish references released by SetFailed() from permanent owner releases. Coordinate
removal with revival and update SelectiveChannel cleanup accordingly.

The fix introduces separate states: ADDITIONAL_REF_RELEASED_ON_FAILURE allows revival, 
while ADDITIONAL_REF_RELEASED permanently prevents it.

Removal thread                      Health-check thread              References
      |                                     |
      | ReleaseAdditionalReference()        |
      | RELEASED_ON_FAILURE -> RELEASED     |
      | Mark permanent removal              |
      | No second reference decrement       |                        2 = H + T
      |                                     |
      |                                     | Revive(2)
      |                                     | CAS to REVIVING fails
      |                                     | Observe RELEASED
      |                                     | Return without adding A
      |                                     |                        2 = H + T
      | Release H                           |                        1 = T
      |                                     | Release T              0
      v                                     v

The reference count reaches zero, and the socket is recycled.

If revival acquires REVIVING first, removal waits for it to finish, then marks permanent removal 
and releases the restored A. Both orderings avoid the leak.

Side effects:

• Performance effects:

• Breaking backward compatibility:

---

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

3 open issues

• Revive() assumes the only possible CAS-failure state is ADDITIONAL_REF_RELEASED, but… · New
• This test configures crash_on_fatal_log (and the comment references fatal logs), but the PR also… · New
• The test directly mutates an internal socket field (_is_hc_related_ref_held) to simulate HC… · New

What changed in this PR

Fixes a socket reference leak when socket removal races with health-check revival by distinguishing “additional ref released due to failure” vs “additional ref permanently released,” and ensures SelectiveChannel cleanup prevents revival after removal.

Changes:

• Introduce new additional-ref states to prevent Revive() from restoring the additional ref after permanent removal.
• Update SelectiveChannel cleanup/removal paths to always mark permanent removal before releasing the HC-related reference.
• Add unit tests covering removal↔revival race orderings, invalid SelectiveChannel LB initialization, and removal preventing revival.

File	Description
test/​brpc_socket_unittest.cpp	Adds tests for removal/revival ordering and concurrent racing.
test/​brpc_channel_unittest.cpp	Adds tests for invalid SelectiveChannel LB init and removal preventing revival.
src/​brpc/​versioned_ref_with_id.h	Adds permanent vs failure-only additional-ref states and ref-release coordination with Revive().
src/​brpc/​selective_channel.cpp	Ensures permanent removal is recorded before releasing HC-related refs; adds unified cleanup guard.
src/​brpc/​load_balancer.cpp	Changes invalid LB protocol handling from LOG(FATAL) to LOG(ERROR) + return error.

_{Review effort: Lite}

[chenBright]

@wwbmmm @wasphin None of these three comments identifies a blocking issue in the current PR. We do not recommend adopting the first two suggestions; the third is, at most, a low-priority test encapsulation improvement. The PR is ready to merge.

[wwbmmm]

please resolve merge conflict

[chenBright]

please resolve merge conflict

@wwbmmm done.