Handle closed subscription channel in start method #341
Conversation
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/core/application/subscription.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.go: Follow standard Go conventions: format with gofmt and follow Effective Go patterns
Always check returned errors and provide descriptive error messages
Document exported functions, types, and complex logic with comments
Files:
internal/core/application/subscription.go
internal/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place private application code under internal/
Files:
internal/core/application/subscription.go
internal/core/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Implement core business logic within internal/core/
Files:
internal/core/application/subscription.go
🧠 Learnings (1)
📚 Learning: 2025-08-05T12:57:08.516Z
Learnt from: louisinger
Repo: ArkLabsHQ/fulmine PR: 279
File: internal/core/application/subscription.go:60-65
Timestamp: 2025-08-05T12:57:08.516Z
Learning: In the fulmine codebase's subscription system (internal/core/application/subscription.go), scripts should remain in the database even if the subscription startup fails. The design intentionally keeps scripts persistent to maintain subscription intent and allow retry mechanisms to work properly.
Applied to files:
internal/core/application/subscription.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: integration tests
- GitHub Check: unit tests
| case event, ok := <-subscriptionChannel: | ||
| if !ok { | ||
| log.Warnf("subscription channel closed, retrying...") | ||
| stopped = true | ||
| continue | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Potential resource leak: consider calling h.close() before retrying.
The channel closure handling correctly detects when subscriptionChannel is closed and triggers a retry. However, unlike the error path (line 241) which calls onError(event.Err) → h.close(), this path doesn't clean up the old subscription state before retrying.
When the outer loop retries and calls GetSubscription again (line 218), the new closeFn overwrites h.closeFn (line 225) without invoking the old one. This could leak resources (connections, file descriptors, goroutines, etc.) managed by the previous closeFn.
Consider calling h.close() before continuing, or at minimum clearing the state:
case event, ok := <-subscriptionChannel:
if !ok {
+ h.close()
log.Warnf("subscription channel closed, retrying...")
stopped = true
continue
}If you want to avoid the sleep in onError, call h.close() directly as shown above.
🏁 Script executed:
#!/bin/bash
# Examine the subscription.go file, focusing on the channel handling and error paths
cat internal/core/application/subscription.go | head -300 | tail -150Length of output: 2703
🏁 Script executed:
#!/bin/bash
# Find h.close() definition
rg -n "func.*close\(" internal/core/application/subscription.go -A 10 -B 2Length of output: 255
🏁 Script executed:
#!/bin/bash
# Find onError definition
rg -n "func.*onError" internal/core/application/subscription.go -A 10 -B 2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look at the outer loop and subscription initialization
rg -n "for.*retry\|GetSubscription\|closeFn.*=" internal/core/application/subscription.go -B 2 -A 5Length of output: 0
Resource leak confirmed: h.close() must be called before retrying on channel closure.
The analysis in the original review is correct. The code has a resource leak in the channel closure path:
- Error path (line 234–236): Calls
onError(event.Err)→h.close(), which invokes the oldcloseFn()and clears the state before retrying. - Channel closed path (line 227–231): Does not call
h.close(), leaving the oldcloseFninh.closeFnunexecuted.
When the outer loop retries and assigns a new closeFn at line 222, it overwrites the old one without invoking it. Resources (connections, file descriptors, goroutines) managed by the old closeFn are leaked.
The suggested fix is correct:
case event, ok := <-subscriptionChannel:
if !ok:
+ h.close()
log.Warnf("subscription channel closed, retrying...")
stopped = true
continueThis ensures the old closeFn is called before the loop retries, matching the error handling path.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
internal/core/application/subscription.go around lines 233-238: the
channel-closed branch logs and retries without calling h.close(), leaking the
previous closeFn; before setting stopped and continuing on subscriptionChannel
closure, invoke h.close() (same as the onError path) to run and clear the
previous closeFn so resources are released, then proceed with the retry.
Summary by CodeRabbit