-
Notifications
You must be signed in to change notification settings - Fork 45
Description
Description
Establish channel lifecycle guidelines and add explicit close() operations throughout the codebase. Currently, zero explicit channel close calls were found in pkg/ directory, leading to potential goroutine leaks where goroutines wait indefinitely on channels.
Current Issues
Missing Channel Close Patterns:
- Search for
close(.*chanreturned zero results in pkg/ directory - 10+ files with channels that are never explicitly closed
- Goroutines waiting on channels may never unblock
Specific Examples:
pkg/cli/compile_integration_test.go:262-done := make(chan struct{})never closedpkg/cli/mcp_inspect_inspector.go:180-done := make(chan struct{})never closedpkg/cli/logs_json_clean_test.go:107-done := make(chan bool)never closed
Suggested Changes
1. Audit and Fix Channel Usage
- Audit all channel creation sites in pkg/ directory
- Add
defer close(ch)where sender owns the channel - Add timeout patterns:
select { case <-done: case <-time.After(...): } - Use
chan struct{}for signaling (notchan bool)
2. Document Channel Lifecycle Guidelines
Add channel lifecycle section to code style guide with these principles:
Ownership Rules:
- Creator documents who closes the channel (comment required)
- Sender closes after last send (use
defer close(ch)) - Never close receiver side (panic risk if sender still writing)
Best Practices:
- Prefer
chan struct{}overchan boolfor signaling - Use buffered channel (size 1) when sender shouldn't block
- Always close channels in
deferfor safety - Add timeouts to prevent indefinite blocking
Anti-Patterns to Avoid:
- ❌ Creating channels without documented ownership
- ❌ Not closing channels when done
- ❌ Closing channels on receiver side
- ❌ Using
chan boolfor signaling (wastes memory)
3. Update Existing Code
Fix identified files:
// BEFORE (leak risk)
done := make(chan struct{})
go func() {
// ... work ...
done <- struct{}{} // Channel never closed
}()
<-done
// AFTER (correct)
done := make(chan struct{})
go func() {
defer close(done) // Explicit close
// ... work ...
}()
<-doneFiles Affected
Primary fixes (10+ files):
pkg/cli/compile_integration_test.go:262pkg/cli/mcp_inspect_inspector.go:180pkg/cli/logs_json_clean_test.go:107- All test files with similar patterns (audit needed)
Documentation:
- Add channel lifecycle section to
AGENTS.mdor code style guide - Update existing channel usage examples
Success Criteria
- ✅ All channels have documented ownership (who closes)
- ✅ All channels are explicitly closed with
defer close() - ✅ Signal channels use
chan struct{}instead ofchan bool - ✅ Channel lifecycle guidelines documented in style guide
- ✅ All tests pass with
-raceflag - ✅ No blocked goroutines detected in test suite
- ✅ Timeout patterns added where appropriate
Benefits
- Prevents goroutine leaks waiting on channels
- Clarifies channel ownership and lifecycle
- Standardizes channel usage patterns across codebase
- Improves code readability with clear ownership semantics
- Educational value for team members
Testing Strategy
- Run all tests with
-raceflag to detect races - Add goroutine leak detection tests
- Verify timeouts work correctly
- Check for blocked goroutines in test suite
- Integration tests for graceful shutdown scenarios
Source
Extracted from Sergo Report discussion #12225 - "Finding 4: Missing Channel Close Patterns" and "Task 3: Channel Lifecycle Guidelines" sections.
The report identifies this as MEDIUM severity: "Goroutines waiting on channels may never unblock, causing goroutine leaks. Missing channel lifecycle guidelines, unclear ownership semantics."
Priority
Medium - Prevents leaks, combines documentation and code fixes, medium effort (3-4 days)
AI generated by Discussion Task Miner - Code Quality Improvement Agent
- expires on Feb 11, 2026, 1:22 PM UTC