Fix: remove close(l.buffer) to prevent send-on-closed-channel panic i…#92
Fix: remove close(l.buffer) to prevent send-on-closed-channel panic i…#92SantiagoDePolonia merged 4 commits intomainfrom
Conversation
…n async loggers A race existed between Write() and flushLoop() during shutdown: a goroutine could pass the l.closed check, get preempted, and resume after close(l.buffer), causing a panic. Closing the buffer channel is unnecessary since flushLoop exits via l.done; the GC reclaims the channel. Drain now uses a non-blocking select loop instead of range. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This pull request fixes a race condition in async loggers that could cause a "send on closed channel" panic during shutdown. The issue occurred when Write() goroutines passed the l.closed check but then got preempted before sending on the buffer channel, allowing flushLoop to close the channel. The fix removes the channel close operation and replaces the range-based drain with a non-blocking select loop.
Changes:
- Removed
close(l.buffer)calls to prevent send-on-closed-channel panics - Replaced blocking range drain with non-blocking select loop in shutdown path
- Updated comments to explain the synchronization strategy
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/usage/logger.go | Fixed race condition by removing channel close and using non-blocking drain loop; maintains existing writes.Wait() synchronization |
| internal/auditlog/logger.go | Applied similar fix but missing the writes WaitGroup synchronization pattern that usage/logger.go has |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/auditlog/logger.go
Outdated
| // Shutdown: mark as closed so Write() stops sending | ||
| l.closed.Store(true) | ||
| close(l.buffer) | ||
| // Drain remaining entries from buffer | ||
| for entry := range l.buffer { | ||
| batch = append(batch, entry) | ||
| // Drain remaining entries from buffer using non-blocking loop. | ||
| // We do NOT close(l.buffer) — closing is unnecessary since flushLoop | ||
| // exits via l.done, and closing creates a race with concurrent Write() calls. | ||
| for { | ||
| select { | ||
| case entry := <-l.buffer: | ||
| batch = append(batch, entry) | ||
| default: | ||
| goto drainComplete | ||
| } | ||
| } |
There was a problem hiding this comment.
The auditlog/logger.go implementation is missing the synchronization pattern that usage/logger.go uses to prevent race conditions during shutdown. Specifically: (1) auditlog/logger.go lacks a 'writes sync.WaitGroup' field to track in-flight Write() calls, (2) the Close() method doesn't set l.closed before closing l.done, and (3) Close() doesn't wait for in-flight writes to complete. This creates a critical race where Write() can send to l.buffer after the drain starts but before it exits via the default case, potentially losing entries. Setting l.closed here (line 123) is ineffective because it happens after Close() has already closed l.done without synchronization. To fix this properly: add a writes WaitGroup field, modify Write() to track calls like usage/logger.go does (lines 63-64, 102), update Close() to set l.closed and call writes.Wait() before closing l.done, and remove the Store call from flushLoop.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
✨ 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 |
The single-connection pool (MaxOpenConns=1) serialized all DB access, negating WAL mode's native support for concurrent readers. Dashboard read queries blocked on background flush writes and vice versa. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Mirrors the synchronization pattern already in usage/logger.go: - writes WaitGroup tracks in-flight Write() calls so Close() waits for them before signaling flushLoop to stop - Double-check pattern in Write() closes the race window between the first closed.Load() and writes.Add(1) - Close() uses closed.Swap(true) for idempotency — safe to call multiple times during shutdown Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Restore sqliteDateRangeConditions(), pgDateRangeConditions(), and mongoDateRangeFilter() helper functions. The inlining was an unrelated refactoring regression that duplicated identical date-range logic across 12 call sites with no behavioral change. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…n async loggers
A race existed between Write() and flushLoop() during shutdown: a goroutine could pass the l.closed check, get preempted, and resume after close(l.buffer), causing a panic. Closing the buffer channel is unnecessary since flushLoop exits via l.done; the GC reclaims the channel. Drain now uses a non-blocking select loop instead of range.