[Code Quality] Establish channel lifecycle guidelines and add explicit close patterns

[github-actions]

Description

The codebase lacks explicit channel close operations and documented ownership semantics, leading to potential goroutine leaks from goroutines waiting on channels that are never closed. Zero explicit close(channel) calls were found in the pkg/ directory, with multiple instances of channels created but never closed.

Problem

Current State:

• 0 explicit close(channel) calls in pkg/ directory
• Multiple channels created without documented lifecycle
• Goroutines waiting on channels may never unblock
• Unclear ownership semantics (who creates, who closes)

Impact:

• Goroutine leaks from blocked channel receivers
• Resource exhaustion in long-running processes
• Unclear code patterns for channel usage
• Difficult to reason about concurrent code

Specific Examples Found

1. pkg/cli/compile_integration_test.go:262 - done := make(chan struct{}) never closed
2. pkg/cli/mcp_inspect_inspector.go:180 - done := make(chan struct{}) never closed
3. pkg/cli/logs_json_clean_test.go:107 - done := make(chan bool) never closed
4. Multiple test files with similar patterns

Suggested Changes

1. Add Explicit Channel Close Operations

Pattern to implement:

// ❌ BEFORE - Channel never closed
done := make(chan struct{})
go func() {
    // do work...
    done <- struct{}{}
}()
<-done  // Blocks until work completes

// ✅ AFTER - Explicit close by sender
done := make(chan struct{})
go func() {
    defer close(done)  // Sender closes after work
    // do work...
}()
<-done  // Blocks until channel closes

2. Document Channel Ownership

Add comments documenting lifecycle:

// done is closed by the worker goroutine after completion
done := make(chan struct{})

// results is closed by the producer after all items are sent
results := make(chan Item)

3. Use Proper Signal Channel Types

Replace chan bool with chan struct{} for signaling:

// ❌ AVOID - Uses memory for bool value
done := make(chan bool)

// ✅ PREFER - Zero memory overhead
done := make(chan struct{})

4. Add Timeout Patterns

Prevent indefinite blocking:

select {
case <-done:
    // Success
case <-time.After(5 * time.Second):
    // Timeout
}

Channel Lifecycle Guidelines

Ownership Rules

1. Document ownership - Add comment stating who closes the channel (required)
2. Sender closes - The goroutine sending on the channel closes it after the last send
3. Use defer close - defer close(ch) for safety
4. Never close on receiver side - Risk of panic if sender still writing
5. Buffered for send-forget - Use buffer size 1 when sender shouldn't block

Best Practices

// 1. Signal channels - use chan struct{}
done := make(chan struct{})

// 2. Buffered for single result (sender doesn't block)
result := make(chan error, 1)

// 3. Sender closes with defer
go func() {
    defer close(done)
    // work...
}()

// 4. Broadcast by closing
start := make(chan struct{})
for i := 0; i < 10; i++ {
    go func() {
        <-start  // All wait
        // work...
    }()
}
close(start)  // Broadcast to all

// 5. Timeout safety
select {
case <-done:
case <-time.After(timeout):
}

Exception: os.Signal Channels

// ✅ CORRECT - Signal channels use signal.Stop(), not close()
sigChan := make(chan os.Signal, 1)
signal.Notify(sigChan, os.Interrupt, syscall.SIGTERM)
defer signal.Stop(sigChan)  // Cleanup, not close()

Files to Update

Priority 1: Test Files

• pkg/cli/compile_integration_test.go:262
• pkg/cli/mcp_inspect_inspector.go:180
• pkg/cli/logs_json_clean_test.go:107
• Other test files with similar patterns

Priority 2: Production Code

• Audit all make(chan calls in pkg/
• Add explicit close operations where missing
• Document ownership in comments

Testing Strategy

Goroutine Leak Detection

func TestNoGoroutineLeaks(t *testing.T) {
    before := runtime.NumGoroutine()
    
    // Run operation that spawns goroutines
    done := make(chan struct{})
    go func() {
        defer close(done)
        // work...
    }()
    <-done
    
    // Wait briefly for cleanup
    time.Sleep(100 * time.Millisecond)
    
    after := runtime.NumGoroutine()
    assert.Equal(t, before, after, "goroutine leak detected")
}

Race Detection

go test -race ./pkg/...

Timeout Verification

# Ensure tests don't hang
go test -timeout 30s ./pkg/...

Success Criteria

• All channel creation sites audited in pkg/
• defer close(ch) added where sender owns channel
• Ownership documented in comments
• Timeout patterns added for blocking receives
• chan struct{} used for signaling (not chan bool)
• Channel lifecycle section added to code style guide
• All tests pass with -race flag
• No goroutine leaks detected in test suite
• No tests hang (run with -timeout)

Documentation

Add to code style guide:

Channel Lifecycle Guidelines

1. Ownership: Creator documents who closes channel
2. Defer close: Use defer close(ch) for safety
3. Signal channels: Prefer chan struct{} over chan bool
4. Buffered for send-forget: Buffer size 1 when sender shouldn't block
5. Close after last send: Sender closes after final value
6. Never close receiver side: Panic risk if sender still writing

Benefits

• Prevents goroutine leaks from blocked channel receivers
• Clarifies channel ownership and lifecycle
• Standardizes channel usage patterns across codebase
• Improves code readability and maintainability
• Reduces resource leaks in long-running processes

Priority

Medium - Prevents resource leaks but mostly affects test code

Source

Extracted from Goroutine Lifecycle & API Consistency Analysis discussion #12225

Analysis Context:

• Zero explicit close(channel) calls found in pkg/ directory
• Multiple test files create channels without closing them
• Missing channel lifecycle guidelines in coding standards

AI generated by Discussion Task Miner - Code Quality Improvement Agent

• expires on Feb 18, 2026, 5:18 PM UTC