Fix UDP socket leak in CentralDogma-to-CentralDogma mirroring - #1328
Merged
Conversation
Motivation: CentralDogma-to-CentralDogma mirrors were creating and closing a new CentralDogmaClient on every mirror run which is wasteful. Modifications: - Add CentralDogmaMirror.getOrCreateRemoteClient() that lazily creates and caches a single CentralDogmaClient per mirror instance, reusing it across all subsequent runs. - Disable health checks (healthCheckIntervalMillis(0)) in the remote client: mirrors run on a fixed schedule and a failed run retries on the next tick, so persistent health-check requests serve no purpose. - Add MirrorSchedulingService.activeMirrors (ConcurrentHashMap) to cache Mirror instances across scheduler ticks Result: - Each CentralDogmaMirror holds at most one remote client for its lifetime, eliminating per-run DNS/TLS/connection overhead and preventing UDP socket accumulation.
📝 WalkthroughWalkthroughThe PR adds derived-client access-token support, introduces shared client-pool injection in mirror scheduling, and updates ChangesMirror and client flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/mirror/MirrorSchedulingService.java`:
- Around line 290-302: The mirror replacement logic in MirrorSchedulingService
currently closes the previous mirror immediately inside activeMirrors.compute,
which can interrupt an in-flight sync in the worker. Update the active mirror
swap flow so replaced or deleted mirrors are not closed until any active runs
using them have finished, or introduce reference counting for the shared client
in CentralDogmaMirror to avoid shutting down the cached CentralDogma client
mid-run. Keep the existing identity check in the compute block, but defer the
existing.close() call to the point where the mirror is no longer in use.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 992e2c01-e94a-4945-961e-11a3ff3b0cc8
📒 Files selected for processing (6)
it/mirror-listener/src/test/java/com/linecorp/centraldogma/it/mirror/listener/CustomMirrorListenerTest.javaserver-mirror-dogma/src/main/java/com/linecorp/centraldogma/server/internal/mirror/CentralDogmaMirror.javaserver-mirror-git/src/test/java/com/linecorp/centraldogma/server/internal/mirror/MirrorSchedulingServiceTest.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/mirror/DefaultMirroringServicePlugin.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/mirror/MirrorSchedulingService.javaserver/src/main/java/com/linecorp/centraldogma/server/mirror/Mirror.java
ikhoon
reviewed
Jul 8, 2026
| } | ||
|
|
||
| final String key = mirrorCacheKey(freshMirror); | ||
| currentMirrorKeys.add(key); |
Contributor
There was a problem hiding this comment.
What do you think of adding a new API that derives a Central Dogma with a new access token, and caching CentralDogma clients per endpoint URL?
Map<String, CentralDogma> clients = ...;
String key = dogmaUrl;
CentralDogma defaultClient = clients.get(key);
CentralDogma derivedClient = defaultClient.withAccessToken(credential.accessToken());
// Perform CD-to-CD mirroring with the derivedClientEven if we cache clients for active mirrors, I am concerned that the number of DnsEndpointGroups will increase as CD-to-CD mirroring increases. It could lead to leaks.
Contributor
Author
There was a problem hiding this comment.
Thanks, addressed!
Please note that there are some limitations though:
MirrorSchedulingService, which manages the resources, do not have the client dependency so we need to useObject.- This won't work for mLTS auth so I might refactor later when I implement it:
https://github.com/line/centraldogma/blob/main/server-mirror-dogma/src/main/java/com/linecorp/centraldogma/server/internal/mirror/CentralDogmaMirror.java#L112 - Might use the cache for mirror again When I need to reuse the SSH client.
jrhee17
approved these changes
Jul 9, 2026
| * Closes the resources held by this {@link Mirror}, such as cached connections to the remote. | ||
| * The default implementation is a no-op. | ||
| */ | ||
| default void close() {} |
Contributor
There was a problem hiding this comment.
Question) Will mirrors created by MirrorServiceV1 also be closed/cleaned up?
Contributor
Author
There was a problem hiding this comment.
It should be closed. 😓 Thanks!
By the way, close method is gone now.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
client/java-armeria/src/main/java/com/linecorp/centraldogma/internal/client/armeria/ArmeriaCentralDogma.java (1)
180-186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding unit tests for the new
withAccessTokenderived-client behavior.No test changes are included for this override (or the
ReplicationLagTolerantCentralDogmacounterpart). Given the derived client changes close()/resource-ownership semantics, a test asserting the derived client shares the underlyingWebClient/connection pool and thatclose()is a no-op would guard against regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/java-armeria/src/main/java/com/linecorp/centraldogma/internal/client/armeria/ArmeriaCentralDogma.java` around lines 180 - 186, Add unit tests for the new derived-client behavior in ArmeriaCentralDogma.withAccessToken and the ReplicationLagTolerantCentralDogma counterpart. Verify that the returned client reuses the same underlying WebClient/connection pool as the base instance and that calling close() on the derived client does not release shared resources or affect the original client. Add coverage for the no-op SafeCloseable ownership semantics so future changes to derived-client construction or close handling do not regress.server/src/main/java/com/linecorp/centraldogma/server/internal/mirror/MirrorSchedulingService.java (1)
108-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePool key comment says "maxBytes" but the actual key excludes it.
The field comment states keys are "host:port:tls:maxBytes," but
CentralDogmaMirror.baseClientPoolKeyproduceshost:port:tlsonly. This is functionally safe becausemaxNumBytesPerMirroris a single global value, but the comment is inaccurate and could mislead future maintainers. Align the comment with the actual key format.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/main/java/com/linecorp/centraldogma/server/internal/mirror/MirrorSchedulingService.java` around lines 108 - 110, The shared base-client pool comment is inaccurate because it says the key includes maxBytes, while CentralDogmaMirror.baseClientPoolKey actually uses only host:port:tls. Update the comment near baseClientPool to match the real key format and keep the explanation aligned with how MirrorSchedulingService and CentralDogmaMirror share the pool.server/src/main/java/com/linecorp/centraldogma/server/mirror/Mirror.java (1)
136-142: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueJavadoc mentions
maxResponseLengthin the key but the actual key doesn't include it.The Javadoc on lines 137-139 states the pool key encodes "host, port, TLS flag, and maxResponseLength," but
CentralDogmaMirror.baseClientPoolKeyonly includes host, port, and TLS flag. SincemaxNumBytesPerMirroris a global setting inMirrorSchedulingService, this doesn't cause a functional issue today, but the documentation is misleading. Consider correcting the Javadoc to match the actual key composition, or addingmaxResponseLengthto the key if per-mirror values are planned.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/main/java/com/linecorp/centraldogma/server/mirror/Mirror.java` around lines 136 - 142, The Javadoc for setBaseClientPool in Mirror is inconsistent with the actual cache key composition used by CentralDogmaMirror.baseClientPoolKey. Update the documentation to describe only the fields actually included in the shared base-client pool key (host, port, and TLS flag), or, if per-mirror sizing is intended, adjust the key generation logic accordingly so the documented and implemented behavior match.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@server-mirror-dogma/src/main/java/com/linecorp/centraldogma/server/internal/mirror/CentralDogmaMirror.java`:
- Around line 141-144: The no-pool branch in CentralDogmaMirror creates a base
client via createBaseClient(uri, maxNumBytes) but never retains or closes it, so
the close() path only releases remoteClient and leaks resources. Update
CentralDogmaMirror to store the base client created in the no-pool path,
distinguish whether it is pool-owned, and make close() explicitly close that
retained base client when no scheduler pool was injected.
---
Nitpick comments:
In
`@client/java-armeria/src/main/java/com/linecorp/centraldogma/internal/client/armeria/ArmeriaCentralDogma.java`:
- Around line 180-186: Add unit tests for the new derived-client behavior in
ArmeriaCentralDogma.withAccessToken and the ReplicationLagTolerantCentralDogma
counterpart. Verify that the returned client reuses the same underlying
WebClient/connection pool as the base instance and that calling close() on the
derived client does not release shared resources or affect the original client.
Add coverage for the no-op SafeCloseable ownership semantics so future changes
to derived-client construction or close handling do not regress.
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/mirror/MirrorSchedulingService.java`:
- Around line 108-110: The shared base-client pool comment is inaccurate because
it says the key includes maxBytes, while CentralDogmaMirror.baseClientPoolKey
actually uses only host:port:tls. Update the comment near baseClientPool to
match the real key format and keep the explanation aligned with how
MirrorSchedulingService and CentralDogmaMirror share the pool.
In `@server/src/main/java/com/linecorp/centraldogma/server/mirror/Mirror.java`:
- Around line 136-142: The Javadoc for setBaseClientPool in Mirror is
inconsistent with the actual cache key composition used by
CentralDogmaMirror.baseClientPoolKey. Update the documentation to describe only
the fields actually included in the shared base-client pool key (host, port, and
TLS flag), or, if per-mirror sizing is intended, adjust the key generation logic
accordingly so the documented and implemented behavior match.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b30a1736-766e-4a43-95a6-1234be3bb7e7
📒 Files selected for processing (7)
client/java-armeria/src/main/java/com/linecorp/centraldogma/internal/client/armeria/ArmeriaCentralDogma.javaclient/java/src/main/java/com/linecorp/centraldogma/client/AbstractCentralDogma.javaclient/java/src/main/java/com/linecorp/centraldogma/client/CentralDogma.javaclient/java/src/main/java/com/linecorp/centraldogma/internal/client/ReplicationLagTolerantCentralDogma.javaserver-mirror-dogma/src/main/java/com/linecorp/centraldogma/server/internal/mirror/CentralDogmaMirror.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/mirror/MirrorSchedulingService.javaserver/src/main/java/com/linecorp/centraldogma/server/mirror/Mirror.java
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/src/main/java/com/linecorp/centraldogma/server/internal/mirror/MirrorSchedulingService.java (1)
224-241: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAlways attempt to terminate the worker before closing pooled clients.
terminate(scheduler) || terminate(worker)short-circuits. If scheduler termination reports interruption, the worker is not terminated, but the newly addedbaseClientPoolclose/clear still runs and can close clients under an active worker.Proposed fix
- final boolean interrupted = terminate(scheduler) || terminate(worker); + final boolean schedulerInterrupted = terminate(scheduler); + final boolean workerInterrupted = terminate(worker); + final boolean interrupted = schedulerInterrupted || workerInterrupted;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/main/java/com/linecorp/centraldogma/server/internal/mirror/MirrorSchedulingService.java` around lines 224 - 241, The shutdown logic in MirrorSchedulingService currently short-circuits on terminate(scheduler) || terminate(worker), so the worker may never be terminated before baseClientPool is closed. Update the termination flow in the shutdown/finally path to always call terminate on both scheduler and worker first, then combine their interruption results afterward, before closing and clearing baseClientPool. Keep the change localized around terminate, scheduler, worker, and the pooled-client cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/mirror/MirrorSchedulingService.java`:
- Around line 224-241: The shutdown logic in MirrorSchedulingService currently
short-circuits on terminate(scheduler) || terminate(worker), so the worker may
never be terminated before baseClientPool is closed. Update the termination flow
in the shutdown/finally path to always call terminate on both scheduler and
worker first, then combine their interruption results afterward, before closing
and clearing baseClientPool. Keep the change localized around terminate,
scheduler, worker, and the pooled-client cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 664514ec-dc2f-4565-9e6a-163e2c3dc8ab
📒 Files selected for processing (3)
server-mirror-dogma/src/main/java/com/linecorp/centraldogma/server/internal/mirror/CentralDogmaMirror.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/mirror/MirrorSchedulingService.javaserver/src/main/java/com/linecorp/centraldogma/server/mirror/Mirror.java
💤 Files with no reviewable changes (1)
- server/src/main/java/com/linecorp/centraldogma/server/mirror/Mirror.java
minwoox
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation:
CentralDogma-to-CentralDogma mirrors were creating a new
ArmeriaCentralDogmaClient on every scheduler tick, allocating a fresh
Armeria WebClient, connection pool, and UDP socket each time. These
sockets were not reclaimed promptly, leading to too many open
files.
Modifications:
that returns a derived client sharing the same underlying WebClient.
Calling close() on the derived client is a no-op; the base client owns
the connection resources.
default no-op method so MirrorSchedulingService can inject the pool
without coupling the server module to client types.
Result:
eliminating per-run DNS/TLS/connection overhead and stopping UDP socket
accumulation.