Repository navigation
feat: publish compatibility, tuning guidance, and the 10ms default (Phase 5) - #33
Merged
heynemann merged 8 commits intoAug 8, 2026
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe change documents v0.3.0 compatibility, keeps the default batch interval at one second, adds configurable Fx options, guards construction-time startup, and expands queue, shutdown, lifecycle, and performance validation. ChangesBatching and measurement
Fx lifecycle
Compatibility
Lifecycle and queue validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Fx
participant provideBatcherModule
participant Batcher
participant Processor
Fx->>provideBatcherModule: provide options and processor factory
provideBatcherModule->>Batcher: construct with WithSkipAutoStart
Fx->>Batcher: start through lifecycle hook
Batcher->>Processor: process submitted batches
Fx->>Batcher: shutdown with hook context
Batcher-->>Fx: close or return incomplete-drain error
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
heynemann
changed the title
heynemann
marked this pull request as ready for review
August 6, 2026 22:43
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
pkg/batcher/compatibility_test.go (1)
147-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert legacy Fx option forwarding.
This test only proves that the positional call compiles and that work eventually processes. It also passes if a future provider ignores
batchSizeandbatchIntervaland uses defaults. Afterapp.RequireStart(), assertb.Config().BatchSizeis2andb.Config().BatchIntervalis50*time.Millisecond.Proposed test addition
app.RequireStart() +cfg := b.Config() +require.Equal(t, 2, cfg.BatchSize) +require.Equal(t, 50*time.Millisecond, cfg.BatchInterval) + b.Add(&legacyItem{ID: 1})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/batcher/compatibility_test.go` around lines 147 - 154, Extend the legacy Fx compatibility test after app.RequireStart() to assert that b.Config().BatchSize equals 2 and b.Config().BatchInterval equals 50*time.Millisecond. Keep the existing processing assertions intact so the test verifies both option forwarding and runtime behavior through ProvideBatcherInFX.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/improvements/default-window.md`:
- Around line 135-138: Update the 5ms row in the default-window comparison table
to state that its 13.1ms p99.9 latency is lower than 10ms’s 21.7ms, while noting
the difference is within measurement noise; retain the coalescing advantage as
the rationale for preferring 10ms.
In `@pkg/batcher/constants.go`:
- Around line 11-14: Update the comment describing the 10ms default near the
batcher latency/coalescing constants so its percentile is consistent with the
documented measurements: report approximately 12ms as p99 at 1,000 items/s, or
explicitly label the approximately 22ms figure as p99.9.
In `@pkg/batcher/fx.go`:
- Around line 56-59: Update the option ordering in the processor factory flow
around WithProcessor so caller options are applied first and the injected
processorFunc is appended afterward, preventing any competing caller-supplied
WithProcessor from replacing it. Add a regression test that supplies both a
competing WithProcessor and the injected processor, then verifies the injected
processor is used.
In `@pkg/batcher/options.go`:
- Around line 31-35: The batch-size estimate documentation near the batch
interval option currently describes only arrival rate × interval. Update it to
state that, under steady traffic, expected size is approximately min(BatchSize,
arrival rate × interval), reflecting the size-based flush bound while preserving
the existing sparse-traffic and non-positive interval explanations.
---
Nitpick comments:
In `@pkg/batcher/compatibility_test.go`:
- Around line 147-154: Extend the legacy Fx compatibility test after
app.RequireStart() to assert that b.Config().BatchSize equals 2 and
b.Config().BatchInterval equals 50*time.Millisecond. Keep the existing
processing assertions intact so the test verifies both option forwarding and
runtime behavior through ProvideBatcherInFX.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1854bc96-d1ea-4613-a147-cc2badd4be6e
📒 Files selected for processing (10)
README.mddocs/improvements/compatibility.mddocs/improvements/default-window.mddocs/improvements/plan-perf.mdpkg/batcher/compatibility_test.gopkg/batcher/constants.gopkg/batcher/fx.gopkg/batcher/fx_options_test.gopkg/batcher/options.gotest/scenario/decision_test.go
heynemann
force-pushed
the
phase-5-guidance-and-default
branch
from
August 6, 2026 23:25
bd859c7 to
dd4c2f3
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/improvements/compatibility.md`:
- Around line 53-54: Clarify the migration comment accompanying the Config()
usage to state that Config() returns a value whose fields callers can read
directly without a pointer.
- Around line 3-5: Update the migration guide’s behavior-change list to document
that v0.3.0 changes the default BatchInterval from 1 second to 10 milliseconds,
including the resulting lower latency and increased processor calls when batch
size is not reached. State how callers can override the interval and how to
bound the queue.
- Around line 179-186: Update the “Repeated stop” row in the Fx stop-hook
behaviour table to document that later Shutdown calls share the existing drain
and return their own wait result, rather than observing a cached
ShutdownIncompleteError. Preserve the idempotent shared-drain behavior while
distinguishing per-call outcomes.
In `@docs/improvements/default-window.md`:
- Around line 174-178: Update the migration table row for WithBatchInterval to
distinguish callers that set a positive interval from those passing zero or a
negative duration. Document that non-positive values use DefaultBatchInterval
and therefore retain the 1s fallback, while preserving the existing behavior
described for explicit positive intervals.
- Around line 25-27: Add the text language tag to the environment metadata code
fence in default-window.md, changing the opening fence to use text while
preserving the block contents.
- Around line 181-187: Update the meanBatch calculation in the Stats example to
use Completed + Failed + Panicked as the total processed outcomes, and guard the
division when BatchesFlushed is zero so the example cannot divide by zero.
In `@README.md`:
- Line 387: Update the measured-latency sentence in README.md to read “With 8
workers, the same scenario measured a p50 latency of 4ms.”
- Around line 353-355: Update the batch-size explanation near the documented
examples to state the general bound as min(arrival rate × interval, batch size),
and clarify that the arrival-rate formula applies only when WithBatchSize does
not trigger first. Revise the example to reflect the default batch size of
1,000: at 50,000 items/s and a 100ms interval, the batch reaches 1,000 items and
flushes after about 20ms.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d344cdce-6399-42e6-a2cb-a7b799b8e7b6
📒 Files selected for processing (10)
README.mddocs/improvements/compatibility.mddocs/improvements/default-window.mddocs/improvements/plan-perf.mdpkg/batcher/compatibility_test.gopkg/batcher/constants.gopkg/batcher/fx.gopkg/batcher/fx_options_test.gopkg/batcher/options.gotest/scenario/decision_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- docs/improvements/plan-perf.md
- pkg/batcher/options.go
- pkg/batcher/constants.go
- test/scenario/decision_test.go
- pkg/batcher/fx_options_test.go
- pkg/batcher/fx.go
- pkg/batcher/compatibility_test.go
heynemann
force-pushed
the
phase-5-guidance-and-default
branch
from
August 7, 2026 02:26
811c053 to
3393926
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
209-216: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse one batcher variable and preserve the retry result.
This snippet calls
b, but the surrounding example usesbatcherand does not declareb. The secondShutdownresult is assigned toerrand then discarded. Usebatcherconsistently and return or log the final error.Proposed fix
-if err := b.Shutdown(ctx); err != nil { +if err := batcher.Shutdown(ctx); err != nil { ... - err = b.Shutdown(context.Background()) + if err = batcher.Shutdown(context.Background()); err != nil { + log.Printf("batcher shutdown incomplete: %v", err) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 209 - 216, Update the shutdown example to use the declared batcher variable consistently instead of b, and preserve the result of the retry call to batcher.Shutdown(context.Background()) by returning it or logging the final error.
🧹 Nitpick comments (4)
docs/improvements/thresholds.md (1)
156-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the documented coverage scope with CI
CI runs
go test -coverprofile=coverage.out ./...and excludes/test/, so it enforces aggregate shipped-code coverage, notpkg/batchercoverage. Define this denominator and replace96.2%with the CI total (93.5%), or change CI to measure only./pkg/batcher.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/improvements/thresholds.md` around lines 156 - 158, Update the coverage statement in the thresholds documentation to match CI’s aggregate shipped-code scope from go test -coverprofile=coverage.out ./..., excluding /test/. Replace the pkg/batcher-specific 96.2% figure with the CI total of 93.5%, or instead revise CI to measure only ./pkg/batcher and retain the narrower scope.pkg/batcher/stability_test.go (1)
32-34: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winGate the high trial counts behind
testing.Short().The file runs 700 full batcher lifecycles across five tests: 100 + 200 + 150 + 100 + 150 trials. Each trial constructs a batcher, spawns goroutines, and drains a shutdown. Under
-racethis dominates package test time, and the trials inside each test run serially.Race coverage needs repetition, so do not remove it. Reduce the count in short mode instead, and keep the full count for CI.
♻️ Proposed pattern, applied per test
- const trials = 100 + trials := 100 + if testing.Short() { + trials = 5 + }Also applies to: 101-105, 147-149, 200-205, 301-303
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/batcher/stability_test.go` around lines 32 - 34, Update the trial-count setup in each affected test in stability_test.go to use reduced counts when testing.Short() is true, while preserving the existing 100/200/150/100/150 full-mode counts for CI and race repetition. Apply the conditional count before each corresponding trial loop.pkg/batcher/queue_edge_test.go (1)
199-203: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep the clamp cases tied to
capacityFloor. The small-ceiling case stops coveringbatchSize < capacityFloorwhen the floor becomes8. Derive its values fromcapacityFloor, and keep the inside-bounds case above the floor.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/batcher/queue_edge_test.go` around lines 199 - 203, Update the table cases in the queue edge tests so clamp scenarios remain tied to capacityFloor: derive the small-ceiling case from capacityFloor while preserving batchSize below the floor, and adjust the inside-bounds case to use a value above capacityFloor. Keep the existing ceiling and expected-result behavior intact.pkg/batcher/options_freeze_test.go (1)
82-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
batcher.DefaultConcurrencyfor the concurrency assertion.MaxQueueSizehas no default constant, so keep its0assertion. This keeps the test focused on option freezing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/batcher/options_freeze_test.go` around lines 82 - 85, Update the concurrency assertion in the option-freezing test to compare against batcher.DefaultConcurrency instead of the literal 1. Keep the MaxQueueSize assertion at 0 and leave the other option assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/improvements/default-window.md`:
- Line 178: Update the migration guidance table in default-window.md to
accurately reflect the measured 10k/s and 50k/s behavior: replace the claim that
no change is measurable with the documented mean batch sizes (about 101 and
500), acknowledge the increased calls per second, and split the row by the
BatchSize threshold if needed.
In `@pkg/batcher/fx.go`:
- Around line 79-80: Update Shutdown so all callers share both the single drain
operation and its final result, rather than recomputing the outcome after
waiting. Persist the terminal error or nil when the drain completes, and have
every repeated Shutdown call return that same stored result, including
*ShutdownIncompleteError.
In `@README.md`:
- Around line 383-395: The slow-processor documentation examples are
inconsistent with TestReproducesInlineSlowProcessorInversion and contain
unpinned latency values. In README.md lines 383-395, use the test’s 50ms
processor with 5ms and 100ms windows while removing the 120ms and 100ms p50
claims unless accompanied by reproducible environment details; apply the same
correction to docs/improvements/default-window.md lines 164-168.
---
Outside diff comments:
In `@README.md`:
- Around line 209-216: Update the shutdown example to use the declared batcher
variable consistently instead of b, and preserve the result of the retry call to
batcher.Shutdown(context.Background()) by returning it or logging the final
error.
---
Nitpick comments:
In `@docs/improvements/thresholds.md`:
- Around line 156-158: Update the coverage statement in the thresholds
documentation to match CI’s aggregate shipped-code scope from go test
-coverprofile=coverage.out ./..., excluding /test/. Replace the
pkg/batcher-specific 96.2% figure with the CI total of 93.5%, or instead revise
CI to measure only ./pkg/batcher and retain the narrower scope.
In `@pkg/batcher/options_freeze_test.go`:
- Around line 82-85: Update the concurrency assertion in the option-freezing
test to compare against batcher.DefaultConcurrency instead of the literal 1.
Keep the MaxQueueSize assertion at 0 and leave the other option assertions
unchanged.
In `@pkg/batcher/queue_edge_test.go`:
- Around line 199-203: Update the table cases in the queue edge tests so clamp
scenarios remain tied to capacityFloor: derive the small-ceiling case from
capacityFloor while preserving batchSize below the floor, and adjust the
inside-bounds case to use a value above capacityFloor. Keep the existing ceiling
and expected-result behavior intact.
In `@pkg/batcher/stability_test.go`:
- Around line 32-34: Update the trial-count setup in each affected test in
stability_test.go to use reduced counts when testing.Short() is true, while
preserving the existing 100/200/150/100/150 full-mode counts for CI and race
repetition. Apply the conditional count before each corresponding trial loop.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e9ac956b-7126-4616-bcef-b333b4fc81f8
📒 Files selected for processing (12)
README.mddocs/improvements/compatibility.mddocs/improvements/default-window.mddocs/improvements/plan-perf.mddocs/improvements/thresholds.mdpkg/batcher/fx.gopkg/batcher/fx_options_test.gopkg/batcher/options.gopkg/batcher/options_freeze_test.gopkg/batcher/queue_edge_test.gopkg/batcher/shutdown_test.gopkg/batcher/stability_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/improvements/plan-perf.md
- pkg/batcher/options.go
heynemann
force-pushed
the
phase-5-guidance-and-default
branch
from
August 7, 2026 13:53
2adc8dc to
632b185
Compare
heynemann
commented
Aug 7, 2026
heynemann
commented
Aug 7, 2026
heynemann
added a commit
that referenced
this pull request
Aug 7, 2026
Two review decisions from PR #33. Keep DefaultBatchInterval at 1s. The latency evidence for 10ms stands and is unchanged, but it was only half the trade: at 1,000 items/s the measured downstream call rate moves from ~1/s to ~94/s, and that cost falls on every caller who never set an interval and never asked for lower latency. Latency is visible to whoever measures it and is fixed with one option; a silent ~90x increase in downstream calls is not visible until something else saturates. So 10ms becomes a published recommendation and the default stays where existing deployments already are. The decision record now records that outcome rather than a default change, with an "Adopting 10ms" section carrying the call-rate cost per arrival rate so the trade is visible at the point of choosing. compatibility.md drops the breaking-change entry -- there is nothing to migrate -- and plan-perf.md records why its own gate ("change the default only if latency, batching efficiency, allocation and overload behaviour all support it") was not met: downstream call rate is the axis that fails. TestDefaultBatchIntervalStaysOneSecond pins the literal value through both the constant and Config. Nothing asserted the concrete default before, only that Config agreed with whatever the constant said, so the constant could drift back with a green suite. Sabotage-verified. Start now panics when called before construction finishes, instead of returning silently. Reachable only from inside an Option, since Option is an arbitrary func(*Batcher[T]). The previous no-op avoided the data race but left a caller who wrote Start believing a batcher was running when it was not; a panic names the cause, which the stack alone does not since it points into New. Also corrected the slow-processor examples flagged in review. Both documents cited a 10ms window against 100ms while TestReproducesInlineSlowProcessorInversion measures 5ms against 100ms, and quoted 120ms/100ms as if pinned when the test asserts only the ordering. Re-measured on this host (5ms p50 120ms, 100ms p50 100ms), corrected the window, labelled the environment, and stated what the test actually guarantees. The unpinned "4ms with 8 workers" claim is replaced by a reference to TestConcurrencyRemovesSlowProcessorCoupling, which pins the improvement without quoting a number no test defends.
heynemann
added a commit
that referenced
this pull request
Aug 7, 2026
Two review decisions from PR #33. Keep DefaultBatchInterval at 1s. The latency evidence for 10ms stands and is unchanged, but it was only half the trade: at 1,000 items/s the measured downstream call rate moves from ~1/s to ~94/s, and that cost falls on every caller who never set an interval and never asked for lower latency. Latency is visible to whoever measures it and is fixed with one option; a silent ~90x increase in downstream calls is not visible until something else saturates. So 10ms becomes a published recommendation and the default stays where existing deployments already are. The decision record now records that outcome rather than a default change, with an "Adopting 10ms" section carrying the call-rate cost per arrival rate so the trade is visible at the point of choosing. compatibility.md drops the breaking-change entry -- there is nothing to migrate -- and plan-perf.md records why its own gate ("change the default only if latency, batching efficiency, allocation and overload behaviour all support it") was not met: downstream call rate is the axis that fails. TestDefaultBatchIntervalStaysOneSecond pins the literal value through both the constant and Config. Nothing asserted the concrete default before, only that Config agreed with whatever the constant said, so the constant could drift back with a green suite. Sabotage-verified. Start now panics when called before construction finishes, instead of returning silently. Reachable only from inside an Option, since Option is an arbitrary func(*Batcher[T]). The previous no-op avoided the data race but left a caller who wrote Start believing a batcher was running when it was not; a panic names the cause, which the stack alone does not since it points into New. Also corrected the slow-processor examples flagged in review. Both documents cited a 10ms window against 100ms while TestReproducesInlineSlowProcessorInversion measures 5ms against 100ms, and quoted 120ms/100ms as if pinned when the test asserts only the ordering. Re-measured on this host (5ms p50 120ms, 100ms p50 100ms), corrected the window, labelled the environment, and stated what the test actually guarantees. The unpinned "4ms with 8 workers" claim is replaced by a reference to TestConcurrencyRemovesSlowProcessorCoupling, which pins the improvement without quoting a number no test defends.
heynemann
force-pushed
the
phase-5-guidance-and-default
branch
from
August 7, 2026 15:38
c4f2ad6 to
045ce97
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
docs/improvements/thresholds.md (1)
198-199: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the baseline command shell-safe.
At Line 199,
<env>is parsed as shell redirection syntax. A copied command will not create the documented output file. Replace the placeholder with a concrete value or a quoted shell variable.Proposed fix
- make bench-enqueue > docs/improvements/baselines/enqueue-<env>.txt + env_name=ubuntu-latest-amd64 + make bench-enqueue > "docs/improvements/baselines/enqueue-${env_name}.txt"This is based on the shell command in
docs/improvements/thresholds.md.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/improvements/thresholds.md` around lines 198 - 199, Update the baseline command in the thresholds documentation to use a shell-safe concrete environment value or a quoted shell variable instead of the angle-bracket placeholder, while preserving the documented enqueue baseline output path.docs/improvements/plan-perf.md (3)
1126-1127: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBound the sizing rule by
BatchSize.Lines 1126-1127 state
arrival rate × windowwithout the batch-size cap. This conflicts with the bounded formula inREADME.mdLines 362-369 anddocs/improvements/default-window.mdLines 203-206. Usemin(arrival rate × window, BatchSize)so high-rate callers do not infer batches larger than the configured limit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/improvements/plan-perf.md` around lines 1126 - 1127, Update the “Sizing guidance” statement in docs/improvements/plan-perf.md to bound the expected batch calculation by BatchSize, using the established min(arrival rate × window, BatchSize) rule and preserving the existing 5–10ms guidance.
1119-1122: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake the request-handler example observe admission errors.
Lines 1119-1122 require the README request-handler example to use
Enqueue, butREADME.mdLines 459-465 still callAddand always returnnil. That example cannot reportErrClosingor queue admission failure and can silently drop work after shutdown. Update the example, or mark this migration as deferred.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/improvements/plan-perf.md` around lines 1119 - 1122, Update the README request-handler example referenced by the migration plan to use Enqueue and return or otherwise propagate its admission error instead of always returning nil. Preserve a separate Add example only for explicitly best-effort behavior, or mark this migration as deferred if the example cannot be updated.
17-30: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winUpdate the slow-processor baseline and implementation status.
Lines 17–30 describe inline processing, but
pkg/batcher/batcher.gonow separates aggregation and processing. Mark these values as a pre-Phase-3 baseline, record the environment, align the 100 ms result with the documented ~100 ms value, and remove the unqualified 2.4x claim because tests assert ordering only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/improvements/plan-perf.md` around lines 17 - 30, Update the slow-processor baseline section to label the inline-processing measurements as pre-Phase-3, document the test environment, and change the 100ms result to the documented approximately 100ms value. Remove the unqualified 2.4x latency claim, and reflect that current tests verify ordering only.README.md (1)
299-301: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument
Pendingas a conservative drain obligation.Line 300 describes
Pendingas only accepted work. The admission contract indocs/improvements/plan-perf.mdLines 189-220 also includes publishers inside the gate before publication, soPendingcan exceedAccepteduntilPublishersInGatereaches zero. State this qualification next to the field.Proposed documentation fix
-// s.Pending -> accepted work not yet finished, including in-flight batches +// s.Pending -> drain obligations, including accepted unfinished work and +// publishers still inside the admission gate; it becomes +// exact for accepted work after PublishersInGate reaches zero🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 299 - 301, Update the README totals documentation for s.Pending to describe it as a conservative drain obligation: include accepted work not yet finished and publishers still inside the admission gate before publication, so it may exceed s.Accepted until PublishersInGate reaches zero. Leave the s.Accepted description unchanged.
🧹 Nitpick comments (1)
docs/improvements/thresholds.md (1)
156-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the coverage scope and command.
The CI job measures all packages with
go test -coverprofile=coverage.out ./..., then excludes/test/; it does not measurepkg/batcheralone. Update96.2%with its package scope and command, and clarify thatcoverage-report-cialso excludesmain.gobut is not used by the workflow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/improvements/thresholds.md` around lines 156 - 158, Update the coverage statement to identify 96.2% as the package-specific pkg/batcher figure, while documenting that CI runs go test -coverprofile=coverage.out ./... across all packages and excludes /test/. Clarify that coverage-report-ci also excludes main.go but is not used by the workflow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/improvements/default-window.md`:
- Around line 102-105: The 10k/s, 10ms documentation uses inconsistent measured
batch-size values. Update docs/improvements/default-window.md lines 102-105 and
README.md lines 362-364 to distinguish predicted 100 from measured approximately
101, or apply the same rounding convention consistently across both locations.
- Around line 171-172: Correct the sentence in the default-window documentation
to state that the default remains 1 second and does not avoid the per-hop
latency; only the 10ms opt-in avoids that latency. Keep the surrounding
explanation and configuration guidance unchanged.
- Around line 215-227: Update the example around the Stats() snapshot to
calculate and log the mean only after terminal draining completes: either take
the snapshot after Shutdown returns or require s.Pending == 0 alongside
BatchesFlushed > 0. Preserve the existing terminal-counter numerator and avoid
logging an in-flight, undercounted ratio.
---
Outside diff comments:
In `@docs/improvements/plan-perf.md`:
- Around line 1126-1127: Update the “Sizing guidance” statement in
docs/improvements/plan-perf.md to bound the expected batch calculation by
BatchSize, using the established min(arrival rate × window, BatchSize) rule and
preserving the existing 5–10ms guidance.
- Around line 1119-1122: Update the README request-handler example referenced by
the migration plan to use Enqueue and return or otherwise propagate its
admission error instead of always returning nil. Preserve a separate Add example
only for explicitly best-effort behavior, or mark this migration as deferred if
the example cannot be updated.
- Around line 17-30: Update the slow-processor baseline section to label the
inline-processing measurements as pre-Phase-3, document the test environment,
and change the 100ms result to the documented approximately 100ms value. Remove
the unqualified 2.4x latency claim, and reflect that current tests verify
ordering only.
In `@docs/improvements/thresholds.md`:
- Around line 198-199: Update the baseline command in the thresholds
documentation to use a shell-safe concrete environment value or a quoted shell
variable instead of the angle-bracket placeholder, while preserving the
documented enqueue baseline output path.
In `@README.md`:
- Around line 299-301: Update the README totals documentation for s.Pending to
describe it as a conservative drain obligation: include accepted work not yet
finished and publishers still inside the admission gate before publication, so
it may exceed s.Accepted until PublishersInGate reaches zero. Leave the
s.Accepted description unchanged.
---
Nitpick comments:
In `@docs/improvements/thresholds.md`:
- Around line 156-158: Update the coverage statement to identify 96.2% as the
package-specific pkg/batcher figure, while documenting that CI runs go test
-coverprofile=coverage.out ./... across all packages and excludes /test/.
Clarify that coverage-report-ci also excludes main.go but is not used by the
workflow.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1ca68cf8-9284-445f-a495-c7faf3730279
📒 Files selected for processing (12)
README.mddocs/improvements/compatibility.mddocs/improvements/default-window.mddocs/improvements/plan-perf.mddocs/improvements/thresholds.mdpkg/batcher/batcher.gopkg/batcher/compatibility_test.gopkg/batcher/constants.gopkg/batcher/fx.gopkg/batcher/options.gopkg/batcher/options_freeze_test.gotest/scenario/decision_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/batcher/compatibility_test.go
- test/scenario/decision_test.go
- pkg/batcher/fx.go
- pkg/batcher/options.go
…entory Milestone 5.1. ProvideBatcherInFXWithOptions gives Fx applications the full option set. Bounded queues, worker concurrency, close grace and the diagnostics buffer were previously unreachable from Fx without hand-constructing the batcher and losing lifecycle integration. The existing positional ProvideBatcherInFX signature is deliberately untouched, so no current call site breaks; adding parameters there for options most callers do not use would have been the wrong trade. Both variants share one module implementation, so the lifecycle contract cannot drift between them. Fx owns the lifecycle, so WithSkipAutoStart is applied last and cannot be overridden by a caller-supplied option: the batcher must not process anything before the start hook runs. docs/improvements/compatibility.md is the migration guide the plan requires. Every behaviour change is listed with before, after, and what a caller has to do: Config() returning a value, DefaultConcurrency 3 to 1 with a functioning worker pool, Close no longer abandoning the drain, Add no longer panicking after shutdown, Len counting in-flight work, the Go floor moving to 1.25.0, and the rill and chann removals. Target version is v0.3.0. It also states the performance picture honestly rather than only the flattering half: removing the relay layers made enqueue 54% faster, while the admission gate that makes bounded queues and panic-free shutdown possible costs roughly 25-30% on the admission path itself. The net is faster, but someone benchmarking admission alone should expect the gate rather than be surprised by it. compatibility_test.go executes the guide's compatibility claims instead of asserting them in prose: legacy construction, read-only Config() access, manual start, the Errors() range loop, and the original Fx signature all still compile and pass. The one intentional break, assignment through Config(), is deliberately not tested, because it no longer compiles by design. TestConfigSnapshotCannotMutateRunningBatcher is the race proof for that break. Four goroutines hammer Config() and mutate every returned copy while the batcher aggregates and processes 400 items; the live configuration is unaffected and every item is processed. Verified separately that the old pointer form does propagate such a mutation, which is exactly the race this removes.
Milestones 5.2 and 5.3. DefaultBatchInterval changes 1s -> 10ms, with the full matrix and reasoning published in docs/improvements/default-window.md. The decision rests on four measurements, not on the intuition that smaller is better: - Latency and coalescing across 1ms..1s at 1k, 10k and 50k items/s. At 1k/s a 1s default measured p99 981ms; 10ms measured p99 12ms while still coalescing ~11 items per batch. At 10k/s, p99 99ms -> 10ms with ~101 items per batch. - The sizing rule rate x interval predicts batch size: predicted 100, measured 100 at 10k/s with a 10ms interval. Asserted by a test, because the tuning guidance is only trustworthy if that relationship is real. - Sustained overload. Every interval from 10ms to 1s behaved identically under a saturated downstream: same queue peak, same heap, same ~199 downstream calls/s, because BatchSize binds before the timer does. This is the check that stopped the decision resting on latency alone. - Bursts. 10ms halved p99 against 100ms while only doubling downstream calls. Two findings shaped the choice against going lower. 1ms *increases* downstream load under saturation (495 calls/s versus 199) because it flushes before batches fill, and at 1k/s it coalesces only 2 items. 10ms is where latency is bounded by the window while coalescing stays meaningful at every rate tested. Notably, the old 1s default was already inert above ~10k/s: 1s and 100ms produced identical results because BatchSize=1000 filled first. Its only real effect was on sparse traffic, where it did the most latency damage and bought the least. The record states plainly where 10ms is the wrong choice: a low-rate service with an expensive downstream should configure a larger interval, no interval provides overload protection, and a slow processor at Concurrency=1 bounds the effective interval, so a 5ms window behind a 50ms processor measured p50 120ms versus 100ms for a 100ms window. Also fixes two option fallbacks that hardcoded literals instead of the constants, so WithBatchInterval(0) no longer silently reverts to 1s now that the default has moved. README gains a "Choosing a batch interval" section with the coalescing table, the overload disclaimer, the slow-processor inversion, the complete option list, and the new Fx options variant. Corrects the stale coverage claim to what CI actually enforces.
Ten review findings on Phase 5. The substantive bug: ProvideBatcherInFXWithOptions documented that callers must not pass WithProcessor, but applied the injected processor first and caller options afterwards. A caller-supplied WithProcessor therefore silently replaced the injected factory result. Reproduced: injected=0, rogue=1. The injected processor is now applied last, so dependency injection wins (injected=1, rogue=0), and a regression test passes a competing processor and asserts it never runs. The interval documentation now states the actual steady-state formula: min(BatchSize, arrival rate x interval). The old rate-times-interval form overstates at high rates because size flushing happens first; at the default size of 1000, 50k/s fills in about 20ms, so a 100ms interval does not create a 5000-item batch. The README and WithBatchInterval godoc both say this now. The compatibility guide claimed every public behaviour change was listed while omitting the largest one: DefaultBatchInterval 1s -> 10ms. It now has a complete before/after/migration section, including callers with positive explicit intervals, callers relying on zero/negative fallback, sparse traffic call-rate changes, and the fact that neither a small nor large interval is overload protection. It also fixes the Config wording and correctly documents per-call rather than cached Shutdown results. Measurement correctness fixes: - scenario concurrency runs use runtime.NumCPU producers at 10k/s, so a single producer's timer precision does not collapse offered load toward service rate; - report sweeps reject timed-out runs instead of including invalid output; - procRNG is mutex-protected around the RNG read only, not the simulated sleep. This fixes a real race from JitteredProcessor/SlowOutlierProcessor combined with WithConcurrency, reproduced in math/rand.(*rngSource).Uint64; - allocation evidence now rejects TimedOut before deriving mean batch size or waste; - the +2% capacity rule is correctly labelled measured evidence, not CI enforcement. Finally, worker tests release blocked processors via t.Cleanup and sync.Once, so an assertion failure cannot turn into a package timeout, and all auto-started batchers in batcher_test.go are closed so they cannot perturb goroutine-budget baselines. All suites pass under -race, vet, gofmt, and actionlint.
Adds the tests that were missing rather than the ones that were easy. Each covers behaviour that is currently correct but pinned by nothing, so a refactor could break it silently. Stability, all written as repeated trials because every one is an ordering race and a single pass proves little: - Blocking Add released by shutdown. Enqueue's parked case was already covered, but Add is the compatibility fast path with no error return, so if shutdown failed to release it the caller would hang with no way to observe why. Reachable only in bounded mode, which is why it was missed. - Add racing Close, 200 trials, asserting every accepted item reaches exactly one terminal outcome. This is the interleaving a request handler produces accidentally during process shutdown. - Panic in the final shutdown batch with a concurrent Errors() consumer, 150 trials. This is the narrowest window in the protocol: a diagnostic published while the coordinator closes the diagnostics channel. Get the single-closer rule wrong and it panics inside the library's own goroutine, where the caller cannot recover it. - Eight concurrent Shutdown callers, asserting one drain, no deadlock, and no false error for work that completed. - A processor that always errors with a 2-entry diagnostics buffer, proving a failed batch still releases its drain obligation. If it did not, Pending would never reach zero and shutdown would wait forever on work that already ran. - Start racing Shutdown from five goroutines, asserting exactly one lifecycle. Correctness, covering paths only reachable at a boundary: - tryPush rejection at exactly capacity, and that the rejection is transient. - Unbounded mode never rejecting, since that is the documented default. - Negative capacity normalising to unbounded rather than rejecting everything. - The full-drain cursor reset, distinct from the partial-drain compaction test. - push honouring a cancelled context, and being released by seal. - Capacity clamping when BatchSize is below the floor, which a caller reaches with WithBatchSize(1), and non-positive BatchSize not yielding a zero-capacity slice. - clampCapacity as a direct table, because it is the retention bound itself. - ShutdownIncompleteError.Error(), which lifecycle tools surface even though callers inspect it with errors.As. - Every option individually respecting the freeze, so adding an option without the guard fails a test rather than surfacing later as a data race. Three of these are sabotage-verified: removing the queue capacity bound, the seal release, or Add's blocking path makes the corresponding test fail or hang. The thresholds doc now records that verification as a table, and states the reasoning — a concurrency test that cannot fail manufactures confidence. Coverage 93.0% -> 96.2%, with queue.go push/tryPush/newQueue and clampCapacity now fully covered.
Five findings from the Phase 5 adversarial review.
The substantive one is a live data race the review reproduced and I confirmed.
Option[T] is func(*Batcher[T]) -- an arbitrary closure, not a declarative value -- so
a caller can pass an option that calls Start during New's option loop. The aggregator
then launched while New was still assigning fields:
WARNING: DATA RACE
Read at ... batcher.(*Batcher[int]).run() batcher.go:445
Previous write at ... batcher.New[int]() batcher.go:108
That is a race inside the library, triggered by caller code. Fixed structurally
rather than by narrowing the doc claim: Start is now inert until New sets
constructed, which happens after every field the pipeline touches is assigned. It
degrades to a no-op rather than panicking, because New starts the batcher itself
unless WithSkipAutoStart was given, and an explicit Start afterwards still works.
Two regression tests cover the hostile option and the skip-auto-start path.
The fx.go comment claiming auto-start "cannot be re-enabled by a caller-supplied
option" was true only for declarative options. It now says why the flag ordering
works and points at Start for the closure case, instead of asserting something the
type system does not guarantee.
Three documentation corrections:
- default-window.md's 5ms row said p99.9 measured "worse" than 10ms while its own
numbers show 13.1ms versus 21.7ms -- better, not worse. The row now states the
comparison correctly and makes the real argument the coalescing one: 6 items per
batch versus 11, for a latency gain the measurement cannot distinguish.
- constants.go cited ~22ms as p99 at 1k/s. That is the p99.9 figure; p99 is ~12ms,
which is what the README, PR body and decision record all say.
- fx.go said repeated stop observes "the same terminal result", implying a sticky
error. shutdown.go and compatibility.md already document the opposite, and
TestShutdownDeadlineDoesNotPoisonLaterCallers proves it: later callers wait on the
same drain and return their own result.
Phase 1 renamed PendingPeak to PendingWorkPeak because Len() is sampled in-system work, not queue depth. The Phase 5 decision test was added after that layer and still referenced the old field, making the fully-rebased stack fail vet and build. Use the renamed field and label its diagnostic column work_peak so the test does not revive the discarded queue-depth claim.
Two review decisions from PR #33. Keep DefaultBatchInterval at 1s. The latency evidence for 10ms stands and is unchanged, but it was only half the trade: at 1,000 items/s the measured downstream call rate moves from ~1/s to ~94/s, and that cost falls on every caller who never set an interval and never asked for lower latency. Latency is visible to whoever measures it and is fixed with one option; a silent ~90x increase in downstream calls is not visible until something else saturates. So 10ms becomes a published recommendation and the default stays where existing deployments already are. The decision record now records that outcome rather than a default change, with an "Adopting 10ms" section carrying the call-rate cost per arrival rate so the trade is visible at the point of choosing. compatibility.md drops the breaking-change entry -- there is nothing to migrate -- and plan-perf.md records why its own gate ("change the default only if latency, batching efficiency, allocation and overload behaviour all support it") was not met: downstream call rate is the axis that fails. TestDefaultBatchIntervalStaysOneSecond pins the literal value through both the constant and Config. Nothing asserted the concrete default before, only that Config agreed with whatever the constant said, so the constant could drift back with a green suite. Sabotage-verified. Start now panics when called before construction finishes, instead of returning silently. Reachable only from inside an Option, since Option is an arbitrary func(*Batcher[T]). The previous no-op avoided the data race but left a caller who wrote Start believing a batcher was running when it was not; a panic names the cause, which the stack alone does not since it points into New. Also corrected the slow-processor examples flagged in review. Both documents cited a 10ms window against 100ms while TestReproducesInlineSlowProcessorInversion measures 5ms against 100ms, and quoted 120ms/100ms as if pinned when the test asserts only the ordering. Re-measured on this host (5ms p50 120ms, 100ms p50 100ms), corrected the window, labelled the environment, and stated what the test actually guarantees. The unpinned "4ms with 8 workers" claim is replaced by a reference to TestConcurrencyRemovesSlowProcessorCoupling, which pins the improvement without quoting a number no test defends.
Three verified Phase 5 review findings. The 10k/s, 10ms sizing text said predicted 100, measured 100, while its own matrix says ~101 at 99 calls/s. The docs now distinguish the predicted 100 from measured ~101; the test deliberately asserts a factor-of-two range rather than a host-specific exact value. The default-window record contradicted its own decision: it keeps the default at 1s but said the default avoids up to one second per-hop latency. Corrected: explicit 10ms avoids it; an unchanged 1s default retains it. The Stats example claimed to calculate a post-drain mean but only checked BatchesFlushed. That counter advances on dispatch while terminal counters advance when the processor returns, so a live snapshot undercounts. The example now takes Stats after Shutdown completes and additionally guards Pending == 0.
heynemann
force-pushed
the
phase-5-guidance-and-default
branch
from
August 8, 2026 00:50
045ce97 to
3f42106
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Phase 5 completes the plan: compatibility/release inventory, operational guidance,
and the evidence-backed default interval decision.
feat: add Fx options variant and publish the v0.3.0 compatibility inventoryfeat: set the default batch interval to 10ms on measured evidenceDecision: default interval 1s → 10ms
The full decision record is
docs/improvements/default-window.md. This is not an intuition change; it is the outcome of the Phase 1 open-loop matrix, rerun after Phases 2-4.The old 1s default was already inert above ~10k/s because
BatchSize=1000filled first. Its only real effect was on sparse traffic, where it did the most latency damage and bought the least.Controls that kept this decision honest
Sizing rule:
rate × windowpredicts batch size. At 10k/s with 10ms: predicted 100, measured 100. Asserted in a test.Overload: 10ms did not trade latency for stability. At 200k/s into a saturated 2ms processor, 10ms, 100ms and 1s had effectively identical accepted/completed rates, queue peaks, heap and downstream call rates.
BatchSizebound before the timer did.Burst: 50k/s for 100ms / idle 100ms x5: 10ms p99 10ms vs 100ms p99 20ms, with 56 vs 28 calls/s.
Why not 1ms: it increased downstream load under saturation (495 calls/s vs 199), and coalesced only two items at 1k/s. It optimises latency by almost removing batching.
Caveats
10ms is a default, not a mandate:
WithMaxQueueSizeandEnqueue, or upstream flow control.Concurrency=1, a slow processor bounds the effective interval. With a 50ms processor at 10k/s, a 5ms window measured p50 120ms vs 100ms for a 100ms window. RaiseWithConcurrencywithWithoutOrderedProcessingbefore lowering the interval.Compatibility: v0.2.1 → v0.3.0
docs/improvements/compatibility.mdis the migration guide. It inventories every public behaviour change, before/after/migration: Go 1.25 floor,Config()value snapshot,DefaultConcurrency3→1 becoming live,Addafter shutdown becoming a rejection not a panic, non-destructive close,Lensemantics, Fx options, and removed dependencies.The guide's compatibility claims are executable in
compatibility_test.go: legacy construction, read-only Config access, manual start, Errors range loop and the existing Fx signature all compile and run. The one intentional break — assigning throughConfig()— is declared rather than hidden.Fx
Adds
ProvideBatcherInFXWithOptions, giving Fx applications access to bounded queues, concurrency, close grace and diagnostics configuration without breaking the original positionalProvideBatcherInFXsignature. Both variants forward the Fx stop context toShutdown.Validation
Dependency context
Top of the stack: #33 → #32 → #31 → #29 → #28. Review and merge bottom-up.
This is the final planned phase. No Phase 6 is proposed: the work now has an evidence-backed default, a compatibility story, operational docs, CI gates, and a five-layer reviewable stack.
Stack created with GitHub Stacks CLI • Give Feedback 💬
Summary by CodeRabbit
New Features
Bug Fixes
Documentation