Fix inconsistent ownership handling in SelectiveChannel::AddChannel on failure (#1408)

[two-headBoy]

What problem does this PR solve?

Issue Number: resolve #1408

Problem Summary:

• SelectiveChannel::AddChannel 各失败路径对用户 sub channel 的所有权处理不一致：有的泄漏（OWNS 时未删用户 channel / 已创建的 fake Socket 未回收），有的行为不统一，调用方无法预判，容易泄漏或 double free。

What is changed and the side effects?

Changed:

• 统一所有权语义为「OWNS_CHANNEL 一律接管、DOESNT_OWN_CHANNEL 一律不接管」，以首次成功注册为唯一事实源。
• Socket::Create 失败时按 ownership 手动回收；其余失败路径对 fake Socket 调SetFailed() +ReleaseHCRelatedReference() ，由SubChannel::BeforeRecycle 按 ownership 清理；重复 channel 返回 -1 且不改变任何东西。补充注释并新增单测。

注：Socket::Create 失败、address 返回 < 0、健康检查未启用、AddServer 因ServerId 重复失败等内部路径依赖 Socket 内部状态，单测环境难以稳定复刻，正确性主要靠代码审查保证；单测覆盖外部可触发的失败（nullptr、重复）及成功路径，间接验证规则一致性。

Side effects:

• Performance effects: 无，仅修正失败路径回收，不影响成功热路径。
• Breaking backward compatibility: 无破坏性变更，仅把失败路径从「可能泄漏」修正为「按 ownership 正确回收」。

---

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]

🟡 Changes recommended

Socket creation failures can synchronously recycle the subchannel, making the new cleanup a use-after-free and double-free.

2 open findings

• Creation failure can double-free an already-recycled SubChannel · New
• Tests omit creation and AddServer failure cleanup paths · New

What changed in this PR

Standardizes SelectiveChannel::AddChannel ownership behavior across registration failures.

Changes:

• Documents ownership and duplicate-registration semantics.
• Adds cleanup for fake-socket failure paths.
• Adds ownership lifecycle tests.

File	Description
src/​brpc/​selective_channel.cpp	Implements failure cleanup and duplicate handling.
src/​brpc/​selective_channel.h	Documents ownership semantics.
test/​brpc_channel_unittest.cpp	Tests duplicate and successful ownership behavior.

🧠 Review effort: Balanced

---

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

[Copilot]

🟡 Changes recommended

Socket creation and pre-initialization failures still violate the documented ownership contract.

1 open finding

• Socket creation failure leaks channels before ownership handoff · New

2 resolved since last review

• Creation failure can double-free an already-recycled SubChannel
• Tests omit creation and AddServer failure cleanup paths

Previously missed (1)

In code that hasn't changed since last review

AddChannel failure violates OWNS_CHANNEL ownership contract

src/​brpc/​selective_channel.h:80

This failure contract is not honored when AddChannel is called before SelectiveChannel::Init: SelectiveChannel::AddChannel returns at selective_channel.cpp:582-584 without deleting an OWNS_CHANNEL argument. That externally reachable failure still leaves ownership with the caller, contrary to the new API guarantee and PR's “OWNS always takes over” rule. Handle ownership on this path (and cover it in the test), or explicitly narrow the contract.

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

An existing PartitionChannel failure handler can double-delete an owned channel under the new semantics.

1 open finding

• Prevent double-free after AddChannel failure ownership change · New

1 resolved since last review

• Socket creation failure leaks channels before ownership handoff

🧠 Review effort: Balanced

[Copilot]

🟡 Changes recommended

Socket::Create may double-transition and recycle or reuse a socket before cleanup completes.

1 open finding

• SetFailed may recycle socket before Create regains control · New

1 resolved since last review

• Prevent double-free after AddChannel failure ownership change

🧠 Review effort: Lite

[wwbmmm]

[replied by brpc-oncall robot] Both of chenBright's findings are valid, and there is a deeper reason they cannot be fixed only in the callers: this path now invokes BeforeRecycle on a Socket that is still half-constructed, so Socket::BeforeRecycled() itself will dereference the not-yet-created _transport (and destroy the not-yet-initialized _id_wait_list) after running the callback — the _user attachment should be dropped and the PR kept scoped to schan (or BeforeRecycledmade safe for a partially constructed Socket first). Also note that Socket::Create can fail before OnCreated runs, in which case no callback runs, so "delete sub_chan or not" cannot be decided at the AddChannel call site with a single unconditional choice; the failure handling needs a mechanism that covers both.

[wwbmmm]

[replied by brpc-oncall robot] The set_user handoff resolves the previous issues cleanly: creation failures never run a recycle callback so AddChannel disposes the wrapper itself, while post-creation failures rely on SubChannel::BeforeRecycle, and the dead re-address block is gone. One inconsistency remains between the newly documented failure contract and the not-initialized path.