feat: improve data-builder performance and resilience#25
Conversation
- Cache reflect.ValueOf(builder) at registration time to avoid repeated reflection on every Run(). - Check context cancellation before each execution stage so cancelled plans short-circuit instead of running all stages. - Aggregate multiple builder errors with errors.Join instead of returning only the first error.
|
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. 📝 WalkthroughWalkthroughReplaced stored exported function with a cached reflect.Value for builders, added explicit rejection of typed-nil builders, centralized error-joining behavior, changed plan to return joined errors, and added tests for nil-builder rejection, context cancellation, and joinErrors behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
There was a problem hiding this comment.
Pull request overview
This PR improves the execution runtime of databuilder plans by reducing per-builder reflection overhead, short-circuiting plan execution on context cancellation, and aggregating errors from builder execution (especially in parallel stages).
Changes:
- Cache each builder’s
reflect.Valueat registration time and reuse it during execution. - Stop executing additional plan stages when
ctxis canceled. - Aggregate errors using
errors.Joininstead of returning only the first encountered error.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| plan.go | Uses cached reflection value during execution, adds ctx cancellation checks per stage, and joins errors instead of returning the first. |
| databuilder.go | Extends builder to store a cached reflect.Value and initializes it during builder registration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… ctx cancel - Remove unused Builder field from builder struct since fnValue replaced all usages - Join accumulated errors with ctx.Err() on context cancellation so prior stage failures are not lost
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plan.go (1)
169-173:⚠️ Potential issue | 🟠 MajorAggregate
o.errinstead of returning the first one.Line 172 still short-circuits on the first internal failure, so parallel panics and
ErrWTFcases remain lossy and order-dependent even though builder-returned errors are now joined.🧩 Proposed fix
for o := range outChan { if o.err != nil { - // error occured, return it back and stop processing - return o.err + errs = append(errs, o.err) + continue } outputs := o.outputs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plan.go` around lines 169 - 173, The loop over outChan currently returns the first o.err, which short-circuits and loses other errors; instead, collect all non-nil o.err values as you drain outChan (e.g., append to a slice of error or use errors.Join) and continue processing every item; after the for-range completes, if you have any collected errors return errors.Join(collected...) (or a wrapped multi-error), otherwise return nil — apply this change to the loop that reads from outChan and any related logic handling ErrWTF to ensure all builder errors are aggregated instead of returning immediately.databuilder.go (1)
137-149:⚠️ Potential issue | 🔴 CriticalReject typed-nil builder funcs — check value in IsValidBuilder.
A typed-nil func stored in
anypasses the type checks inIsValidBuilderbutreflect.ValueOf(bldr).Pointer()ingetFuncNamepanics on a nil value. The safest fix is to checkfnValue.IsNil()immediately after callingreflect.ValueOf(bldr)ingetBuilderand returnErrInvalidBuilderif true. This also eliminates redundant reflection calls.Proposed fix
func getBuilder(bldr any) (*builder, error) { if err := IsValidBuilder(bldr); err != nil { return nil, err } - t := reflect.TypeOf(bldr) + fnValue := reflect.ValueOf(bldr) + if fnValue.IsNil() { + return nil, ErrInvalidBuilder + } + + t := fnValue.Type() out := getStructName(t.Out(0)) - name := getFuncName(bldr) + name := runtime.FuncForPC(fnValue.Pointer()).Name() b := &builder{ Out: out, - fnValue: reflect.ValueOf(bldr), + fnValue: fnValue, Name: name, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@databuilder.go` around lines 137 - 149, getBuilder currently calls IsValidBuilder but then uses reflect.ValueOf(bldr) later which can be a typed nil and cause getFuncName to panic; after creating fnValue := reflect.ValueOf(bldr) inside getBuilder, immediately check fnValue.IsNil() and return ErrInvalidBuilder if true, and then reuse fnValue (and fnValue.Type() / fnValue.Type().Out(0) or pass fnValue to getFuncName) instead of re-calling reflect.TypeOf or reflect.ValueOf to eliminate redundant reflection and prevent the nil-pointer panic; update references to use the builder.fnValue you create.
🧹 Nitpick comments (1)
plan.go (1)
186-187: Keep single-error paths unwrapped.
errors.Joinwraps even one error. That changes the concrete error returned byRun/RunParallel, so direct checks likeerr == context.Canceledor sentinel equality stop working in the single-failure case.♻️ Proposed fix
- if len(errs) > 0 { - return errors.Join(errs...) - } - return nil + switch len(errs) { + case 0: + return nil + case 1: + return errs[0] + default: + return errors.Join(errs...) + }- if ctx.Err() != nil { - return errors.Join(append(errs, ctx.Err())...) + if err := ctx.Err(); err != nil { + if len(errs) == 0 { + return err + } + return errors.Join(append(errs, err)...) }Also applies to: 206-208, 214-215
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plan.go` around lines 186 - 187, The code currently always uses errors.Join on the errs slice (e.g., in the return inside Run/RunParallel), which wraps even a single error and breaks sentinel equality checks; change each occurrence so that if len(errs) == 1 you return errs[0] directly, otherwise return errors.Join(errs...), updating the branches where errors are aggregated (the spots shown around the errors.Join calls and the other occurrences referenced) so single-error paths are not wrapped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@databuilder.go`:
- Around line 137-149: getBuilder currently calls IsValidBuilder but then uses
reflect.ValueOf(bldr) later which can be a typed nil and cause getFuncName to
panic; after creating fnValue := reflect.ValueOf(bldr) inside getBuilder,
immediately check fnValue.IsNil() and return ErrInvalidBuilder if true, and then
reuse fnValue (and fnValue.Type() / fnValue.Type().Out(0) or pass fnValue to
getFuncName) instead of re-calling reflect.TypeOf or reflect.ValueOf to
eliminate redundant reflection and prevent the nil-pointer panic; update
references to use the builder.fnValue you create.
In `@plan.go`:
- Around line 169-173: The loop over outChan currently returns the first o.err,
which short-circuits and loses other errors; instead, collect all non-nil o.err
values as you drain outChan (e.g., append to a slice of error or use
errors.Join) and continue processing every item; after the for-range completes,
if you have any collected errors return errors.Join(collected...) (or a wrapped
multi-error), otherwise return nil — apply this change to the loop that reads
from outChan and any related logic handling ErrWTF to ensure all builder errors
are aggregated instead of returning immediately.
---
Nitpick comments:
In `@plan.go`:
- Around line 186-187: The code currently always uses errors.Join on the errs
slice (e.g., in the return inside Run/RunParallel), which wraps even a single
error and breaks sentinel equality checks; change each occurrence so that if
len(errs) == 1 you return errs[0] directly, otherwise return
errors.Join(errs...), updating the branches where errors are aggregated (the
spots shown around the errors.Join calls and the other occurrences referenced)
so single-error paths are not wrapped.
…ping - Guard against typed-nil func in getBuilder to prevent panic in reflect.ValueOf().Pointer(). Reuse fnValue to eliminate redundant reflection calls. - Remove unused getFuncName (inlined into getBuilder). - Add joinErrors helper that returns single errors unwrapped to preserve sentinel equality (e.g. err == context.Canceled). errors.Join is only used for 2+ errors.
- TestTypedNilBuilderRejected: typed-nil func returns ErrInvalidBuilder - TestContextCancellation: cancelled context short-circuits and returns context.Canceled (verifies sentinel equality is preserved) - TestJoinErrorsSingle: single error returned unwrapped, nil returns nil, multiple errors joined and individually matchable via errors.Is
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.
- Add nil check in IsValidBuilder to prevent reflect.TypeOf(nil) panic when called via Plan.Replace or directly - Rename TestJoinErrorsSingle to TestJoinErrors to reflect full coverage
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.
Comments suppressed due to low confidence (1)
databuilder.go:99
IsValidBuilderstill returns nil for a typed-nil function value (e.g.var f func(...) ...; IsValidBuilder(f)), even thoughAddBuilders/getBuilderwill reject it viafnValue.IsNil(). SinceIsValidBuilderis public API documented as validating builders, it should also detect and reject typed-nil funcs (e.g., checkreflect.ValueOf(builder).IsNil()oncet.Kind()==reflect.Func).
// IsValidBuilder checks if the given function is valid or not
func IsValidBuilder(builder any) error {
if builder == nil {
return ErrInvalidBuilder
}
t := reflect.TypeOf(builder)
if t.Kind() != reflect.Func {
// Input can only be a function
return ErrInvalidBuilderKind
}
if t.NumOut() != 2 {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plan.go (1)
168-186:⚠️ Potential issue | 🟠 Major
o.errstill bypasses the new aggregation path.Line 172 returns on the first internal worker error, so panics and
ErrWTFfrom the rest of the same parallel batch are still dropped. That undercuts the new “preserve parallel failures” behavior.Suggested fix
errs := make([]error, 0) for o := range outChan { if o.err != nil { - // error occured, return it back and stop processing - return o.err + errs = append(errs, o.err) + continue } outputs := o.outputs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plan.go` around lines 168 - 186, The loop currently returns immediately on any internal worker error (o.err), which bypasses aggregation; change the handling in the outChan loop to treat o.err like other per-item errors: append o.err to the errs slice (or wrap it consistently with joinErrors/ErrWTF semantics), skip processing outputs for that item, and continue iterating so all parallel failures are collected; after the loop keep the final return as return joinErrors(errs). Ensure you update the block that references o.err and do not short-circuit the function there.
🧹 Nitpick comments (1)
databuilder_test.go (1)
95-99: Use an identity assertion for the single-sentinel case.
assert.Equalonly checks deep equality here. A fresherrors.New(ErrWTF.Error())would still pass, so this does not actually provejoinErrorsreturned the original error unchanged.Suggested fix
- assert.Equal(t, sentinel, err, "single error should be returned as-is, not wrapped") + assert.Same(t, sentinel, err, "single error should be returned as-is, not wrapped")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@databuilder_test.go` around lines 95 - 99, The test currently uses assert.Equal to check the single-sentinel case which only checks deep equality; change this to an identity assertion so we verify joinErrors returns the exact sentinel object. Replace the assert.Equal call in the test for joinErrors with an identity check such as assert.Same(t, sentinel, err, "single error should be returned as-is, not wrapped") (or require.Same if you prefer a hard fail), referencing the sentinel variable ErrWTF and the joinErrors result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plan.go`:
- Around line 217-225: The loop currently appends ctx.Err() unconditionally
which can duplicate sentinel errors like context.Canceled and force joinErrors
to return a composite error; change the check so if ctx.Err() != nil you first
detect whether that exact error value is already present in errs (e.g., compare
to each element) and if so return joinErrors(errs) immediately, otherwise return
joinErrors(append(errs, ctx.Err())). Update the block around ctx.Err() handling
(where ctx.Err() is checked before calling doWorkAndGetResult) so you avoid
re-adding the same sentinel error to errs and preserve direct err ==
context.Canceled behavior.
---
Outside diff comments:
In `@plan.go`:
- Around line 168-186: The loop currently returns immediately on any internal
worker error (o.err), which bypasses aggregation; change the handling in the
outChan loop to treat o.err like other per-item errors: append o.err to the errs
slice (or wrap it consistently with joinErrors/ErrWTF semantics), skip
processing outputs for that item, and continue iterating so all parallel
failures are collected; after the loop keep the final return as return
joinErrors(errs). Ensure you update the block that references o.err and do not
short-circuit the function there.
---
Nitpick comments:
In `@databuilder_test.go`:
- Around line 95-99: The test currently uses assert.Equal to check the
single-sentinel case which only checks deep equality; change this to an identity
assertion so we verify joinErrors returns the exact sentinel object. Replace the
assert.Equal call in the test for joinErrors with an identity check such as
assert.Same(t, sentinel, err, "single error should be returned as-is, not
wrapped") (or require.Same if you prefer a hard fail), referencing the sentinel
variable ErrWTF and the joinErrors result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa05e7e9-a4d7-423d-8b3d-4955d37c0d0f
📒 Files selected for processing (3)
databuilder.godatabuilder_test.goplan.go
🚧 Files skipped from review as they are similar to previous changes (1)
- databuilder.go
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.
Summary
reflect.ValueOf(builder)in thebuilderstruct at registration time, avoiding repeated reflection on everyRun()/RunParallel()ctx.Err()before each execution stage so cancelled plans short-circuit instead of running all remaining stageserrors.Join(errs...)instead of returning only the first error, preserving all failure information from parallel buildersTest plan
make test -race), including extensive dependency, plan, and builder tests🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests