bugfix: use service port for naming server control metadata - #8128
bugfix: use service port for naming server control metadata#8128skt-shinyruo wants to merge 9 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes how Seata server instances publish their control endpoint port to the naming server by using the Seata service port (XID.getPort()) instead of Spring’s server.port, preventing naming server request forwarding to a non-listening port (Fixes #8110).
Changes:
- Switch control endpoint port selection to
XID.getPort()in both General and Raft instance strategies. - Align Raft leader metadata publishing and current-node metadata sync to publish the service port for
control. - Add/adjust regression tests to validate the control port behavior and restore shared singleton/environment state between tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/src/main/java/org/apache/seata/server/instance/GeneralInstanceStrategy.java | Publish control endpoint using XID.getPort() instead of server.port. |
| server/src/main/java/org/apache/seata/server/instance/RaftServerInstanceStrategy.java | Publish control endpoint using XID.getPort() instead of server.port. |
| server/src/main/java/org/apache/seata/server/cluster/raft/RaftStateMachine.java | Use service port for control metadata in leader metadata and current-node sync paths. |
| server/src/test/java/org/apache/seata/server/instance/GeneralInstanceStrategyTest.java | New regression test ensuring control endpoint uses service port; restores shared state. |
| server/src/test/java/org/apache/seata/server/instance/RaftServerInstanceStrategyTest.java | Update regression to assert control endpoint uses service port; restores shared state. |
| server/src/test/java/org/apache/seata/server/cluster/raft/RaftStateMachineTest.java | Add regression tests validating metadata uses service port for control; cleans up RouteTable and environment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 2.x #8128 +/- ##
============================================
+ Coverage 72.79% 72.81% +0.01%
Complexity 1141 1141
============================================
Files 1151 1151
Lines 42272 42272
Branches 5045 5045
============================================
+ Hits 30773 30780 +7
+ Misses 9023 9019 -4
+ Partials 2476 2473 -3 🚀 New features to boost your workflow:
|
slievrly
left a comment
There was a problem hiding this comment.
中文 / Chinese
总体判断
看懂 issue #8110 后,这个修复方向是对的。根因是端口模型混淆:
- seata-server 的实际服务端口(Netty listen port)由
-p>-Dserver.servicePort>SEATA_PORT>server.service-port>server.port这条优先级解析得到,最终XID.setPort(nettyRemotingServer.getListenPort()),所以XID.getPort()就是真正的服务端口。 - console/control 的 HTTP 请求是走 Netty 端口处理的(尤其
web-application-type: none时根本没有 Spring Boot HTTP server,server.port毫无意义)。 - 老代码把
server.port当成 control endpoint 发布,导致 namingServer 把/api/v1/console/*转发到错误端口 —— 正是 issue 描述的现象。
改成 XID.getPort() 让 control endpoint 指向真正 listen 的端口,符合 seata-server 的实际行为。
时序也没问题:Server.start() 里 XID.setPort()(L108)先于 seataInstanceStrategy.init()(L125),所以 serverInstanceInit() 读到的 XID.getPort() 已经是有效值。三处(GeneralInstanceStrategy、RaftServerInstanceStrategy、RaftStateMachine 的 leader metadata + syncCurrentNodeInfo)改得一致,没有漏。
建议确认/改进
1. 确认没有"console 独立部署在 server.port"的场景
这个修复的前提是"control HTTP 永远走 Netty 端口"。如果存在某种部署把 console 用一个独立 Spring Boot embedded web server 挂在 server.port 上单独提供服务,这个改动会把它们指到 Netty 端口。请在 PR 里明确一句"已确认 console/control API 始终由 Netty HTTP 处理,不存在 server.port 独立提供 console 的部署"。
2. serverProperties 是否还有用途
改完之后,GeneralInstanceStrategy / RaftServerInstanceStrategy 里 serverProperties.getPort() 的唯一调用点被替换掉了。请扫一眼 serverProperties 是否变成完全未使用的字段 —— 如果是,可以顺手清理(连同 AbstractSeataInstanceStrategy 里的注入),避免留下误导性的死字段。
3. 测试脆弱性(非阻塞)
RaftStateMachineTest 里的新测试:
- 用
MockedStatic<XID>+MockedStatic<RaftServerManager>+ 反射调私有syncCurrentNodeInfo removeObject直接反射清ObjectHolder.OBJECT_MAP内部字段
这些能 work,但和实现细节耦合较深。私有方法反射调用尤其脆 —— 方法签名一变测试就挂且报错难懂。如果 syncCurrentNodeInfo 值得直接测,考虑提取成 package-private,比反射更稳。不阻塞合并。
4. server.port 兜底行为的文档
既然澄清了端口优先级,建议在这个 PR 或 follow-up 里更新一下文档:说明 server.port 现在只是 service-port 的最低优先级兜底,且不再作为 control endpoint 发布。issue #8110 的报告者显然是被这套优先级绕晕了。
小结
真实 bug、修复正确、时序安全、覆盖一致,值得合并。上面 1/2 建议在合并前确认一下,3/4 可 follow-up。
English
Overall
Once I read issue #8110, this fix is heading the right direction. The root cause is port-model confusion:
- seata-server's actual service port (the Netty listen port) is resolved via the priority chain
-p>-Dserver.servicePort>SEATA_PORT>server.service-port>server.port, and ends up inXID.setPort(nettyRemotingServer.getListenPort())— soXID.getPort()is the real service port. - console/control HTTP requests are served over the Netty port (especially with
web-application-type: none, where there's no Spring Boot HTTP server at all andserver.portis meaningless). - The old code published
server.portas the control endpoint, so namingServer forwarded/api/v1/console/*to the wrong port — exactly the reported symptom.
Switching to XID.getPort() points the control endpoint at the port actually being listened on, matching seata-server's real behavior.
Ordering is also safe: XID.setPort() (Server.java L108) runs before seataInstanceStrategy.init() (L125), so serverInstanceInit() reads a populated XID.getPort(). The three sites (GeneralInstanceStrategy, RaftServerInstanceStrategy, and both RaftStateMachine paths — leader metadata + syncCurrentNodeInfo) are changed consistently, nothing missed.
Things to confirm / improve
1. Confirm no "console served separately on server.port" deployment
This fix assumes control HTTP is always served over the Netty port. If some deployment runs the console on a separate Spring Boot embedded web server bound to server.port, this change would misdirect it. Please note in the PR: "confirmed the console/control API is always served by Netty HTTP; there is no deployment where server.port serves the console independently."
2. Is serverProperties still used?
After this change, the only caller of serverProperties.getPort() in GeneralInstanceStrategy / RaftServerInstanceStrategy is replaced. Please check whether serverProperties becomes a fully unused field — if so, clean it up (along with its injection in AbstractSeataInstanceStrategy) rather than leaving a misleading dead field.
3. Test fragility (non-blocking)
The new RaftStateMachineTest cases:
- Use
MockedStatic<XID>+MockedStatic<RaftServerManager>+ reflection to invoke the privatesyncCurrentNodeInfo removeObjectreflects intoObjectHolder.OBJECT_MAP's internal field
These work but couple tightly to implementation details. Reflective invocation of a private method is especially brittle — a signature change breaks the test with an opaque failure. If syncCurrentNodeInfo is worth testing directly, consider making it package-private instead of reflecting. Not a merge blocker.
4. Document the server.port fallback behavior
Since the port priority is now clarified, consider updating the docs (in this PR or a follow-up) to state that server.port is only the lowest-priority fallback for the service port and is no longer published as the control endpoint. The #8110 reporter was clearly tripped up by this chain.
Summary
Real bug, correct fix, safe ordering, consistent coverage — worth merging. Please confirm items 1/2 before merge; 3/4 can be follow-up.
Summary
XID.getPort()instead ofserver.portwhen registering naming server control metadata in the general and raft instance strategiesRaftStateMachinemetadata sync paths so leader metadata and current node sync both publish the service port forcontrolFixes #8110.
Test Plan
./mvnw -pl server -DskipITs -Dtest=GeneralInstanceStrategyTest,RaftServerInstanceStrategyTest,RaftStateMachineTest test