fix(client): make register publisher startup idempotent - #7082

🟡 Changes recommended

Failed startup can leak heartbeat schedulers and cause duplicate traffic on retry.

Get a fresh assessment by requesting another Copilot review.

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

REQUEST_CHANGES (PMC Aias00): the main build job is failing — this PR does not compile/pass tests, so it cannot be merged. Please fix the compilation/test failures and re-run CI, then request a re-review.

Review (re-review, PMC Aias00) — fix(client): make register publisher startup idempotent

Decision: APPROVE

Correction to my earlier review

My previous REQUEST_CHANGES said "this PR does not compile". That was wrong, and I apologise for the noise: I read the red CI at face value. Pulling the actual job log for build (21, ubuntu-latest) shows it ends with ##[error]The operation was canceled. — every build/build-docker-images/k8s-examples-http job in that run was cancelled, not failed, and no module reported a compilation or test failure (check-license-header and changes in the same run are green). I have re-triggered the cancelled jobs so we get a clean signal. The code is fine.

Why the change is correct

• The redundant Tars startup is genuinely redundant. TarsServiceBeanEventListener extends AbstractContextRefreshedEventListener, and AbstractContextRefreshedEventListener's constructor already calls publisher.start(shenyuClientRegisterRepository) (line 139). The extra call in the Tars subclass constructor invoked start() a second time on the same singleton, which built a second DisruptorProviderManage and overwrote providerManage — leaking the first ring buffer and its shenyu_disruptor_consumer_* / shenyu_disruptor_provider_* threads. That is exactly #6541. Removing it is right.
• Idempotency. start() is now synchronized and returns early when providerManage != null, so any number of client listeners (tars + http + dubbo in one app, etc.) share one disruptor. providerManage is volatile, so publishEvent()/getProviderManage() readers see a safely published, fully started instance.
• Publish-after-startup + rollback. providerManage is assigned only after manage.startup() and uriSubscriber.start() succeed. On a RuntimeException the URI subscriber and the DisruptorProvider are shut down and the exception is rethrown, leaving providerManage null so a retry can actually succeed. This closes the "failed startup leaks heartbeat schedulers" gap.
• Heartbeat moved out of the constructor. Scheduling the 30s/10s heartbeat from ShenyuClientURIExecutorSubscriber's constructor meant a scheduler thread was created even when startup subsequently failed. Moving it to start() behind an AtomicBoolean guard fixes that, with no behaviour change on the happy path. The existing ShenyuClientURIExecutorSubscriberTest constructs the subscriber directly and only exercises getType()/executor(...), so it is unaffected.
• executor() now calls shutdown() instead of if (!executor.isTerminated()) { executor.shutdown(); } — ScheduledThreadPoolExecutor#shutdown() is idempotent, so this is equivalent.

Non-blocking suggestions

1. createUriSubscriber(...) / createProviderManage(...) are test-only seams in production code. They widen the API of a public singleton purely so the test can inject mocks. Consider keeping them package-private with a @VisibleForTesting-style comment, or use Mockito's mockConstruction(...), so the extension point does not become part of the supported API surface.
2. Document the silent no-op. After the first successful start(), a later start() with a different ShenyuClientRegisterRepository is silently ignored. That is the intended idempotency, but a line of Javadoc on start() would save the next reader the same question.
3. start() catches RuntimeException only; an Error from startup() would leave the disruptor running with providerManage null. Practically never happens — mentioning it only for completeness.

CI status

I re-triggered the cancelled jobs; they are still sitting in queued (Apache runner pool is saturated), so the status may stay yellow/red for a while through no fault of this PR. From the log evidence and the symbol checks above I am satisfied the code compiles and the tests are sound, so I am approving — but please make sure a clean run lands before merge.

Thanks for the fix, and sorry again for the earlier mis-diagnosis.

Re-reviewed: making register publisher startup idempotent (and dropping the redundant Tars listener call) removes a double-start hazard. Approving.

Reviewed the current head. The publisher startup lifecycle is idempotent and recovers cleanly after partial startup failures. No blocking issues found.