feat: fix errors.Is chain, bounded NotifyAsync, Newf/Wrapf#11
Conversation
- Fix Unwrap() to return immediate parent instead of root cause, so errors.Is/errors.As traverses the full wrap chain. Cause() still returns root for backward compatibility. - Cap stack depth at 64 frames (configurable via SetMaxStackDepth) to reduce GC pressure under high error rates. - Add NotifyAsync with bounded semaphore (default 1000) to prevent goroutine explosion under sustained error bursts. - Add SetMaxAsyncNotifications for tuning the async pool size. - Add Newf and Wrapf convenience functions for formatted errors.
|
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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds configurable stack-depth and formatted helpers to the error package, changes Unwrap semantics to follow immediate wrappers, and introduces a bounded, drop-on-full async notification mechanism with a semaphore and helper to configure its capacity. Changes
Sequence DiagramsequenceDiagram
participant Caller
participant NotifyAsync
participant Semaphore as Semaphore<br/>(capacity: 1000)
participant Notify
rect rgba(100, 200, 100, 0.5)
Note over Caller,Notify: Success Path — slot available
Caller->>NotifyAsync: NotifyAsync(err, rawData...)
NotifyAsync->>Semaphore: try acquire slot (non-blocking)
Semaphore-->>NotifyAsync: slot granted
NotifyAsync->>Notify: call Notify(err, rawData...)
Notify-->>NotifyAsync: completes
NotifyAsync->>Semaphore: release slot
NotifyAsync-->>Caller: return err
end
rect rgba(200, 100, 100, 0.5)
Note over Caller,Semaphore: Failure Path — semaphore full
Caller->>NotifyAsync: NotifyAsync(err, rawData...)
NotifyAsync->>Semaphore: try acquire slot (non-blocking)
Semaphore-->>NotifyAsync: no slot available
NotifyAsync-->>Caller: silently drop, return err
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
errors_test.go (1)
107-113: Test doesn't stress the stack depth cap.This test verifies the invariant (
<= 64) but doesn't actually exercise the capping logic since a typical call stack is far shallower. Consider adding a test with recursive calls to exceed the default limit and confirm truncation occurs.💡 Optional: Add a stress test for deep stacks
func TestStackDepthCappedDeep(t *testing.T) { var createDeepError func(depth int) ErrorExt createDeepError = func(depth int) ErrorExt { if depth == 0 { return New("deep error") } return createDeepError(depth - 1) } // Create error from a stack deeper than 64 err := createDeepError(100) if len(err.StackFrame()) > 64 { t.Errorf("stack depth %d exceeds max 64", len(err.StackFrame())) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@errors_test.go` around lines 107 - 113, The current TestStackDepthCapped only checks the <=64 invariant but doesn't trigger the capping logic; modify or add a new test (e.g., TestStackDepthCappedDeep) that uses a recursive helper to call New("...") from a stack deeper than 64 and then assert that len(err.StackFrame()) <= 64 (and optionally that frames were truncated rather than omitted) so the capping behavior in New and StackFrame is exercised; reference the existing TestStackDepthCapped, New, and StackFrame symbols when locating where to add the recursive test.notifier/notifier.go (2)
55-69: Consider logging or emitting a metric when notifications are dropped.When the semaphore is full (lines 65-67), notifications are silently dropped. This could make it difficult to diagnose why certain errors weren't reported during sustained bursts. A debug-level log or a counter metric would aid observability without impacting performance.
💡 Suggested improvement
select { case asyncSem <- struct{}{}: go func() { defer func() { <-asyncSem }() _ = Notify(err, rawData...) }() default: // drop notification to prevent goroutine explosion + log.Debug(context.Background(), "msg", "async notification dropped due to capacity", "err", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notifier/notifier.go` around lines 55 - 69, NotifyAsync currently drops notifications silently when the semaphore asyncSem is full; modify the default branch in NotifyAsync to record the drop (e.g., emit a non-blocking metric counter and/or write a debug-level log) so lost notifications are observable without blocking or changing behavior of asyncSem. Locate the NotifyAsync function and update the default case to increment a drop counter (e.g., notifyDropCounter.Inc()) and/or call a debug logger (e.g., logger.Debugf or processLogger.Debug) in a non-blocking way, ensuring the added instrumentation cannot block or cause errors if metrics/logging systems are unavailable.
41-49: Potential race if called after initialization.Replacing
asyncSemwhile goroutines may be operating on the old channel is unsafe. The docstring warns "Must be called during initialization," but there's no enforcement. If a caller ignores this, goroutines holding slots on the old channel will never release them to the new one.Consider documenting more explicitly or using
sync.Onceto prevent re-initialization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notifier/notifier.go` around lines 41 - 49, The SetMaxAsyncNotifications function currently replaces the package-level asyncSem channel which is unsafe if called after goroutines have started; change it so the semaphore is only initialized once (e.g., use a package-level sync.Once like asyncSemInit.Do(...) or return/panic if already initialized) and document that behavior; reference the SetMaxAsyncNotifications function and asyncSem symbol when making the change so callers cannot reinitialize asyncSem at runtime and running goroutines won’t be left referencing a replaced channel.errors.go (1)
231-237: Same init-time-only concern asSetMaxAsyncNotifications.Modifying
maxStackDepthis not thread-safe. The docstring warning is present, but concurrent error creation during reconfiguration could read partially updated values or cause inconsistent behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@errors.go` around lines 231 - 237, SetMaxStackDepth currently mutates maxStackDepth unsafely; make the update thread-safe by either (A) changing maxStackDepth to an int32/int64 and using atomic.StoreInt32/StoreInt64 in SetMaxStackDepth (and corresponding atomic loads where it's read), or (B) enforce init-only semantics by adding an initialization guard (e.g., a sync.Once or an "initialized" boolean set during startup) and have SetMaxStackDepth panic or return an error if called after initialization. Update references to maxStackDepth accordingly and adjust reads to use atomic loads if you choose the atomic approach; locate the function SetMaxStackDepth and the global maxStackDepth variable to apply the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@errors_test.go`:
- Around line 107-113: The current TestStackDepthCapped only checks the <=64
invariant but doesn't trigger the capping logic; modify or add a new test (e.g.,
TestStackDepthCappedDeep) that uses a recursive helper to call New("...") from a
stack deeper than 64 and then assert that len(err.StackFrame()) <= 64 (and
optionally that frames were truncated rather than omitted) so the capping
behavior in New and StackFrame is exercised; reference the existing
TestStackDepthCapped, New, and StackFrame symbols when locating where to add the
recursive test.
In `@errors.go`:
- Around line 231-237: SetMaxStackDepth currently mutates maxStackDepth
unsafely; make the update thread-safe by either (A) changing maxStackDepth to an
int32/int64 and using atomic.StoreInt32/StoreInt64 in SetMaxStackDepth (and
corresponding atomic loads where it's read), or (B) enforce init-only semantics
by adding an initialization guard (e.g., a sync.Once or an "initialized" boolean
set during startup) and have SetMaxStackDepth panic or return an error if called
after initialization. Update references to maxStackDepth accordingly and adjust
reads to use atomic loads if you choose the atomic approach; locate the function
SetMaxStackDepth and the global maxStackDepth variable to apply the change.
In `@notifier/notifier.go`:
- Around line 55-69: NotifyAsync currently drops notifications silently when the
semaphore asyncSem is full; modify the default branch in NotifyAsync to record
the drop (e.g., emit a non-blocking metric counter and/or write a debug-level
log) so lost notifications are observable without blocking or changing behavior
of asyncSem. Locate the NotifyAsync function and update the default case to
increment a drop counter (e.g., notifyDropCounter.Inc()) and/or call a debug
logger (e.g., logger.Debugf or processLogger.Debug) in a non-blocking way,
ensuring the added instrumentation cannot block or cause errors if
metrics/logging systems are unavailable.
- Around line 41-49: The SetMaxAsyncNotifications function currently replaces
the package-level asyncSem channel which is unsafe if called after goroutines
have started; change it so the semaphore is only initialized once (e.g., use a
package-level sync.Once like asyncSemInit.Do(...) or return/panic if already
initialized) and document that behavior; reference the SetMaxAsyncNotifications
function and asyncSem symbol when making the change so callers cannot
reinitialize asyncSem at runtime and running goroutines won’t be left
referencing a replaced channel.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 440c9ea7-8c1a-40d0-8466-7d8729a9c598
📒 Files selected for processing (3)
errors.goerrors_test.gonotifier/notifier.go
There was a problem hiding this comment.
Pull request overview
This PR updates the library’s error wrapping and notification behavior to better integrate with Go’s standard errors.Is/errors.As semantics while improving operational safety under high error volume.
Changes:
- Fix
errors.Is/errors.Astraversal by makingUnwrap()return the immediate wrapped error while keepingCause()as the root error. - Add bounded async error notification via
NotifyAsync()with configurable concurrency (SetMaxAsyncNotifications). - Cap captured stack depth (default 64) via
SetMaxStackDepth, and add formatted helpersNewf/Wrapf.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| notifier/notifier.go | Adds bounded async notification API and routes NotifyWithExclude through it. |
| errors.go | Adjusts unwrap chain semantics, adds stack depth cap + setters, introduces Newf/Wrapf. |
| errors_test.go | Adds regression tests for errors.Is chain, Cause() behavior, formatting helpers, and stack cap. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
If SetMaxAsyncNotifications reassigns asyncSem while goroutines are in-flight, the deferred release would target the new channel. Snapshot the semaphore into a local variable so acquire and release always use the same channel instance.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
When wrapping an ErrorExt that doesn't implement NotifyExt, shouldNotify was left at zero value (false), silently skipping error notifications. Default to true and override from NotifyExt if present.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
notifier/notifier.go (1)
66-68: Surface dropped notifications somehow.Once the semaphore fills, notifications vanish silently. A counter or hook here would make saturation visible and let operators tune the cap before incidents start disappearing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notifier/notifier.go` around lines 66 - 68, The select default branch in notifier.go currently drops notifications silently; modify the notifier's drop path (the select default in the send/notify routine) to surface dropped notifications by incrementing a concurrent-safe counter/metric and/or invoking a configurable hook (e.g., an OnDrop callback or emit to an existing metrics client) so operators can detect saturation; update the notifier struct (add a dropCounter or OnDrop func) and use it inside the default branch so every dropped notification either increments the metric or calls the hook (ensure the counter is safe for concurrent access or use the project's metrics API).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@notifier/notifier.go`:
- Around line 45-48: SetMaxAsyncNotifications currently assigns to asyncSem
unsafely while NotifyAsync reads it; prevent races by making
SetMaxAsyncNotifications idempotent and write-once: add a package-level
sync.Once (e.g., setMaxAsyncOnce) and wrap the asyncSem = make(chan struct{}, n)
assignment inside setMaxAsyncOnce.Do(func(){...}), returning immediately if Do
has already run; keep NotifyAsync's read unchanged (it will then never race with
subsequent writes), and ensure any tests or callers treat
SetMaxAsyncNotifications as an init-only operation.
- Around line 60-65: The goroutine launched in NotifyAsync captures the rawData
slice by reference which can cause races if the caller mutates the backing array
after returning; fix by making a shallow copy of rawData in the successful
acquire path (e.g., before starting the goroutine in the select branch) and pass
that copied slice into the closure that calls Notify so the goroutine reads an
immutable snapshot; keep the semaphore handling (sem) as-is but ensure the
goroutine parameter list receives the copied slice and the semaphore channel
(e.g., pass sem and copiedRawData into the anonymous func) so no shared mutable
slice is accessed concurrently.
---
Nitpick comments:
In `@notifier/notifier.go`:
- Around line 66-68: The select default branch in notifier.go currently drops
notifications silently; modify the notifier's drop path (the select default in
the send/notify routine) to surface dropped notifications by incrementing a
concurrent-safe counter/metric and/or invoking a configurable hook (e.g., an
OnDrop callback or emit to an existing metrics client) so operators can detect
saturation; update the notifier struct (add a dropCounter or OnDrop func) and
use it inside the default branch so every dropped notification either increments
the metric or calls the hook (ensure the counter is safe for concurrent access
or use the project's metrics API).
…tifications - Add docstrings to all customError methods for coverage threshold - Add TestStackDepthCappedDeep with recursive calls exceeding 64 frames to exercise the capping logic - Log at debug level when async notifications are dropped due to semaphore capacity, aiding observability during sustained bursts
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…wData slice - Use sync.Once to enforce write-once semantics for asyncSem, preventing races if SetMaxAsyncNotifications is called post-init. - Copy rawData slice before launching goroutine in NotifyAsync to prevent data races if the caller mutates the backing array after NotifyAsync returns.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
errors.Is/errors.Aschain:Unwrap()now returns the immediate parent error instead of jumping to root cause.Cause()still returns root for backward compatibility. This fixeserrors.Is(doubleWrapped, singleWrapped)returningfalse.NotifyAsync()with a semaphore (default cap 1000) to prevent goroutine explosion under sustained error bursts.SetMaxAsyncNotifications(n)for tuning.generateStacknow caps at 64 frames (configurable viaSetMaxStackDepth) to reduce GC pressure.Newf(format, args...)andWrapf(err, format, args...).Test plan
make test -race)TestErrorsIs,TestCauseStillReturnsRoot,TestNewf,TestWrapf,TestStackDepthCapped🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Behavior Changes