[cuebot] Fix channel closing issue#2274
Open
DiegoTavares wants to merge 2 commits into
Open
Conversation
The application os not properly closing the grpc channel at shutdown, leaving this SEVERE warning:
```
Channel ManagedChannelImpl{logId=2787, target=elk0815:8444} was not shutdown properly
```
Explanation:
`RqdClientGrpc` (cuebot/src/main/java/com/imageworks/spcue/rqd/RqdClientGrpc.java:71-89) holds a
Guava `LoadingCache<String, ManagedChannel>`. The `removalListener` calls `conn.shutdown()` **on
cache eviction** — but never on application shutdown. The bean has no `destroy-method`
(applicationContext-service.xml:39, no destroy hook), and `RqdClientGrpc` has no `shutdown()` method
at all. So when cuebot stops:
- The cache is GC'd
- Each `ManagedChannel` is finalized **without** `shutdown()` having been called
- gRPC logs `SEVERE: Channel ... was not shutdown properly!!!` per leaked channel, with the stack
capturing where the channel was first allocated (the `RuntimeException: ManagedChannel allocation
site` — that's a diagnostic stack, not a real exception)
The first truncated stack in your paste is another such allocation-site diagnostic. The actual
runtime call (`launchFrame`) **succeeded** — the frame was dispatched. This is purely a
resource-cleanup hygiene warning, not a functional failure.
Contributor
📝 WalkthroughWalkthroughThis PR adds graceful shutdown of gRPC channels in OpenCue's RqdClientGrpc. A new ChangesgRPC Graceful Shutdown
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
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 `@cuebot/src/main/java/com/imageworks/spcue/rqd/RqdClientGrpc.java`:
- Around line 220-224: The shutdown() method currently only invalidates
channelCache but does not prevent new channels from being created via
channelCache.get(host); add a shutdown guard (e.g., an atomic boolean like
isShutdown) that shutdown() sets before invalidating/invalidationComplete and
update any code that calls channelCache.get(host) to check this guard and refuse
or short-circuit creation if shutdown has begun; ensure shutdown() also prevents
further puts/gets by either making channelCache null after setting the flag or
having creation paths throw/return early when isShutdown is true so no new
channels are recreated during teardown.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 510608ae-c252-4a7a-87d1-47bb051bd437
📒 Files selected for processing (2)
cuebot/src/main/java/com/imageworks/spcue/rqd/RqdClientGrpc.javacuebot/src/main/resources/conf/spring/applicationContext-service.xml
Comment on lines
+220
to
+224
| public void shutdown() { | ||
| if (channelCache != null) { | ||
| logger.info("Shutting down RqdClientGrpc channel cache"); | ||
| channelCache.invalidateAll(); | ||
| } |
Contributor
There was a problem hiding this comment.
Prevent channel recreation after shutdown starts.
shutdown() currently closes existing cached channels, but new channels can still be created afterward via channelCache.get(host) if any teardown-time call races in. That can reintroduce the same leak warning during shutdown.
Suggested fix
+import java.util.concurrent.atomic.AtomicBoolean;
...
+ private final AtomicBoolean shuttingDown = new AtomicBoolean(false);
...
private RqdInterfaceGrpc.RqdInterfaceBlockingStub getStub(String host)
throws ExecutionException {
+ if (shuttingDown.get()) {
+ throw new IllegalStateException("RqdClientGrpc is shutting down");
+ }
if (channelCache == null) {
buildChannelCache();
}
ManagedChannel channel = channelCache.get(host);
return RqdInterfaceGrpc.newBlockingStub(channel).withDeadlineAfter(rqdTaskDeadlineSeconds,
TimeUnit.SECONDS);
}
private RunningFrameGrpc.RunningFrameBlockingStub getRunningFrameStub(String host)
throws ExecutionException {
+ if (shuttingDown.get()) {
+ throw new IllegalStateException("RqdClientGrpc is shutting down");
+ }
if (channelCache == null) {
buildChannelCache();
}
ManagedChannel channel = channelCache.get(host);
return RunningFrameGrpc.newBlockingStub(channel).withDeadlineAfter(rqdTaskDeadlineSeconds,
TimeUnit.SECONDS);
}
...
public void shutdown() {
- if (channelCache != null) {
+ if (!shuttingDown.compareAndSet(false, true)) {
+ return;
+ }
+ if (channelCache != null) {
logger.info("Shutting down RqdClientGrpc channel cache");
channelCache.invalidateAll();
+ channelCache.cleanUp();
}
}🤖 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 `@cuebot/src/main/java/com/imageworks/spcue/rqd/RqdClientGrpc.java` around
lines 220 - 224, The shutdown() method currently only invalidates channelCache
but does not prevent new channels from being created via channelCache.get(host);
add a shutdown guard (e.g., an atomic boolean like isShutdown) that shutdown()
sets before invalidating/invalidationComplete and update any code that calls
channelCache.get(host) to check this guard and refuse or short-circuit creation
if shutdown has begun; ensure shutdown() also prevents further puts/gets by
either making channelCache null after setting the flag or having creation paths
throw/return early when isShutdown is true so no new channels are recreated
during teardown.
ramonfigueiredo
approved these changes
May 12, 2026
Collaborator
ramonfigueiredo
left a comment
ramonfigueiredo
left a comment
There was a problem hiding this comment.
Thanks, @DiegoTavares !
Approved with minor change suggested by CodeRabbitAI
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
2 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.
The application os not properly closing the grpc channel at shutdown, leaving this SEVERE warning:
Explanation:
RqdClientGrpc(cuebot/src/main/java/com/imageworks/spcue/rqd/RqdClientGrpc.java:71-89) holds a GuavaLoadingCache<String, ManagedChannel>. TheremovalListenercallsconn.shutdown()on cache eviction — but never on application shutdown. The bean has nodestroy-method(applicationContext-service.xml:39, no destroy hook), andRqdClientGrpchas noshutdown()method at all. So when cuebot stops:ManagedChannelis finalized withoutshutdown()having been calledSEVERE: Channel ... was not shutdown properly!!!per leaked channel, with the stack capturing where the channel was first allocated (theRuntimeException: ManagedChannel allocation site— that's a diagnostic stack, not a real exception)LLM usage disclosure
Claude Opus was used for investigating the origin of the log and proposing a fix
Summary by CodeRabbit