feat: make admission and shutdown safe (Phase 2)

[heynemann]

What

Phase 2 of the batcher performance plan: make admission and shutdown safe. All
four milestones are complete.

Commit	Milestone
test: characterize batching engine before replacing rill	2.1
refactor: replace rill with an owned aggregation pipeline	2.1
test: enforce the per-batcher goroutine budget	2.1
feat: own the intake queue and add an admission gate	2.2
feat: add resumable Shutdown and forward the fx stop context	2.3
feat: bound diagnostics and recover processor panics per batch	2.4
docs: record Phase 2 measured outcomes	docs
docs: document the Phase 2 lifecycle and admission APIs	docs

The defects this fixes

Each was reproducible on main and is now verified fixed by direct measurement,
not just by the tests added here:

Defect	Before	After
Partial batch, interval > close grace	50 accepted, 0 processed, Len() = 50	50 accepted, 50 processed, Len() = 0
Add after Close	panics the process	counted rejection
Sustained overload	unbounded, ~4.3GB in 2s	queue capped, rejections reported
isClosed	data race under -race	atomic lifecycle state
Start called twice	double-close panic	idempotent
Goroutines per batcher	6	2, none leaked

Design decisions worth reviewing

The intake queue is never closed. chann's relay goroutine can only exit by
closing its own ingress channel — the channel publishers send to — so "never close
the input" and "leak no goroutines" were mutually exclusive. Owning the queue
resolves that, and removes the entire class of send-on-closed-channel panic.

Reserve before publishing, roll back before leaving the gate. Reserving after
publication would let the drain conclude while a publisher was mid-publish; rolling
back after leaving the gate would leave a phantom obligation the drain waits on
forever.

Two lost-wakeup hazards, both with sabotage-verified tests. Every gate
decrement routes through leave(), including enter()'s own rejection path, and
the coordinator performs its own post-seal gate check. Removing either mechanism
makes its test hang — I checked, because a concurrency test that cannot fail is
worthless.

Shutdown reports, never discards. Shutdown(ctx) is resumable: a later call
waits on the same drain rather than restarting it. A caller's deadline is per-caller
and never stored as the terminal result, so one impatient caller cannot poison
another's answer.

Panics are recovered per batch, not per consumer. Batcher owns goroutines the
caller cannot reach, so an unrecovered panic crashed the process and destroyed every
other queued item. Recovery is scoped to the batch so one poison batch cannot stall
everything behind it.

Performance

Removing the relay layers made the hot path substantially faster, measured with
benchstat over -count=6 against the stored baseline:

• -54% geomean sec/op on enqueue (Add 229.8ns → 63.4ns at small batch sizes)
• -49% to -90% bytes/op
• Goroutines per batcher 6 → 2

This is the one place in the plan where a safety change also made things faster,
because each relay cost a channel hop and a goroutine handoff per item.

Two plan predictions were wrong

Both caught by measurement, both corrected in the plan with reasoning:

1. Merging aggregation into the processing loop changes latency. It inverted
   the documented baseline (5ms window becoming faster than 100ms with a 50ms
   processor). I hit this twice — once in 2.1 and again in 2.2 — and reverted both
   times. Aggregation and processing stay separate until Phase 3 decides the worker
   model deliberately.
2. Goroutines could not reach the predicted counts. Predicted 3, then 4;
   measured 5 after 2.1, then 2 after 2.2. Reaching fewer required behaviour
   changes that Phase 2 is not allowed to make.

Bug found while testing

Start racing Shutdown deadlocked: Start declined to launch the consumer when
it saw a sealed gate, but shared startOnce with the drain path, so whichever call
won left the drain waiting on a consumer that would never exist. Start now always
launches the consumer, because sealed does not mean empty. Covered across 200
trials.

Validation

• go test -race ./... clean; go vet ./... clean.
• All 8 characterization tests pass unchanged from before the rewrite.
• The Phase 1 inversion baseline still holds, confirming Phase 2 did not alter the
  window/processor coupling.
• Sabotage-verified: coordinator gate check, leave() routing, and panic recovery
  each fail their test when removed.
• chann and rill both removed from go.mod.

Dependency context

Stacked on #28 (Phase 1); targets phase-1-performance-baseline. Review #28 first.

Phase 3 is next: wiring WithConcurrency to real workers, which is what finally
removes the effective window = max(window, processor duration) coupling and makes
5-10ms windows worthwhile.

_{Stack created with GitHub Stacks CLI • Give Feedback 💬}

Summary by CodeRabbit

• New Features
  • Added bounded queues with context-aware Enqueue and configurable diagnostic error buffers.
  • Added resumable shutdown with configurable grace periods, shutdown status checks, and detailed incomplete-drain errors.
  • Added Stats() snapshots for tracking pending, queued, completed, failed, rejected, and dropped work.
  • Added processor panic reporting and continued processing where possible.
• Bug Fixes
  • Prevented work loss, deadlocks, empty batches, and late-admission panics during shutdown.
• Documentation
  • Expanded shutdown, queue capacity, error handling, and statistics guidance.
• Chores
  • Updated the required Go version to 1.25.0.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ce1ad663-7132-474a-8884-2646c9b331ab

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

The batcher replaces its external pipeline with internal bounded admission, queueing, aggregation, processing, diagnostics, statistics, and resumable shutdown. Tests cover concurrency, draining, backpressure, panic recovery, goroutine budgets, and lifecycle behavior. Documentation and tooling now use Go 1.25.0.

Changes

Batcher lifecycle and processing

Layer / File(s)	Summary
Queue, admission, and observability primitives pkg/batcher/queue.go, pkg/batcher/gate.go, pkg/batcher/stats.go, pkg/batcher/options.go, pkg/batcher/constants.go, pkg/batcher/shutdown.go	Adds bounded queueing, admission sealing, statistics snapshots, configuration options, defaults, and typed shutdown and panic errors.
Processing and resumable shutdown pkg/batcher/batcher.go	Adds internal aggregation and processing, context-aware admission, panic recovery, nonblocking diagnostics, lifecycle states, and resumable draining.
Batching, admission, and shutdown validation pkg/batcher/*_test.go	Adds characterization and concurrency tests for batching, admission races, queue bounds, shutdown draining, idempotency, errors, and statistics.
Diagnostics, panic, and goroutine validation pkg/batcher/diagnostics_test.go, pkg/batcher/goroutine_*_test.go, pkg/batcher/batcher_test.go, pkg/batcher/fx_test.go	Validates diagnostic buffering, panic recovery, allocation behavior, goroutine budgets, leak handling, and standard-library atomic counters.
Lifecycle integration and project documentation pkg/batcher/fx.go, README.md, docs/improvements/*, .github/workflows/bench.yml, go.mod	Wires Fx shutdown to Shutdown(ctx), documents the new APIs, updates performance measurements, selects the module Go version in benchmarks, and raises the minimum Go version to 1.25.0.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Producer
  participant Batcher
  participant Queue
  participant Processor
  participant ShutdownCaller
  Producer->>Batcher: Enqueue(ctx, item)
  Batcher->>Queue: publish accepted item
  Queue-->>Batcher: item-ready signal
  Batcher->>Processor: flush and process batch
  Processor-->>Batcher: result or diagnostic
  ShutdownCaller->>Batcher: Shutdown(ctx)
  Batcher->>Queue: drain accepted work
  Batcher->>Processor: process final partial batch
  Batcher-->>ShutdownCaller: completion or ShutdownIncompleteError

Loading

Possibly related PRs

• NSXBet/batcher#28: Shares performance-planning, threshold, and benchmark workflow changes.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the pull request's main changes to admission and shutdown safety in Phase 2.
Docstring Coverage	✅ Passed	Docstring coverage is 84.75% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch phase-2-lifecycle-safety

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 10

🧹 Nitpick comments (9)

pkg/batcher/characterization_test.go (2)

387-404: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Replace the hand-rolled itoa with strconv.Itoa.

itoa duplicates standard-library behaviour, allocates on every digit through the prepend pattern, and returns wrong output for negative input. Test helpers gain nothing from this.

♻️ Proposed refactor

 func keyFor(i int) string {
-	return "item-" + itoa(i)
-}
-
-func itoa(i int) string {
-	if i == 0 {
-		return "0"
-	}
-
-	var digits []byte
-
-	for i > 0 {
-		digits = append([]byte{byte('0' + i%10)}, digits...)
-		i /= 10
-	}
-
-	return string(digits)
+	return "item-" + strconv.Itoa(i)
 }

Add "strconv" to the import block.

🤖 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/characterization_test.go` around lines 387 - 404, Remove the
hand-rolled itoa helper and update keyFor to use strconv.Itoa directly for
integer conversion. Add the strconv import and preserve keyFor’s existing
“item-” prefix and output behavior.

---

105-117: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

The assertion under-pins the documented behaviour.

The doc comment claims the interval is measured from the first item, not from an earlier tick. elapsed >= interval/2 only proves the flush was not immediate. A periodic-ticker implementation aligned to interval boundaries would still pass roughly half the time. Assert a tighter lower bound, for example elapsed >= interval minus a small tolerance, so a ticker-based reimplementation fails.

🤖 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/characterization_test.go` around lines 105 - 117, The timing
assertion in the first-item flush test should enforce that the batch does not
flush before approximately the full interval measured from Add of the first
item. Update the require.GreaterOrEqual check around start, b.Add, and elapsed
to use interval minus a small, explicit tolerance, while preserving the existing
batch-count and item-count assertions.

pkg/batcher/admission_test.go (2)

226-259: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Close the batcher when the test ends.

This test never closes b. The aggregator and processing goroutines stay alive for the rest of the package run, and the queue still holds accepted items. Other tests in this package assert goroutine budgets, so a leaked lifecycle can make them flaky.

🧹 Proposed fix

 	release := make(chan struct{})
 	defer close(release)
 
 	b := batcher.New(
 		batcher.WithBatchSize[int](1),
 		batcher.WithBatchInterval[int](time.Hour),
 		batcher.WithMaxQueueSize[int](2),
 		batcher.WithProcessor(func([]int) error {
 			<-release
 
 			return nil
 		}),
 	)
+
+	t.Cleanup(func() { _ = b.Close() })

Note: register the cleanup after defer close(release) so the processor unblocks before Close waits on the drain. Same applies to TestBlockedEnqueueIsReleasedByShutdown, which starts Close in a goroutine and never waits for it.

🤖 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/admission_test.go` around lines 226 - 259, Register test cleanup
for the batcher instance after defer close(release) so the processor is
unblocked before b.Close waits for draining; apply the same lifecycle cleanup
and ensure the Close goroutine is awaited in
TestBlockedEnqueueIsReleasedByShutdown.

---

31-35: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Unconditional high trial counts across the race tests. Five tests repeat full batcher lifecycles hundreds of times with millisecond intervals. The shared root cause is a fixed repetition count with no testing.Short() path, which makes every local go test ./... pay the full cost and multiplies under -race.

• pkg/batcher/admission_test.go#L31-L35: make trials a variable and lower it to a small value when testing.Short() is set; apply the same to the 300-trial loops at Lines 110 and 145 in this file.
• pkg/batcher/shutdown_test.go#L190-L190: apply the same testing.Short() reduction to this 300-trial loop.
• pkg/batcher/shutdown_test.go#L284-L284: apply the same testing.Short() reduction to this 200-trial loop.

🤖 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/admission_test.go` around lines 31 - 35, Reduce repeated
race-test cost by making the shared trials value in
pkg/batcher/admission_test.go lines 31-35 short-test aware, using a small count
when testing.Short() is enabled while preserving the current default. Apply the
same testing.Short() reduction to the 300-trial loop at
pkg/batcher/admission_test.go lines 110 and 145, the 300-trial loop at
pkg/batcher/shutdown_test.go line 190, and the 200-trial loop at
pkg/batcher/shutdown_test.go line 284; leave normal trial counts unchanged.

pkg/batcher/batcher.go (2)

166-171: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Rename the nonBlockingWhenFull parameter; it never applies when the queue can be full.

The flag only selects the fast path when MaxQueueSize <= 0, where the queue cannot be full. In bounded mode Add passes true and still blocks in push. The name states the opposite of the behaviour. A name such as unboundedFastPath matches the condition at Line 166.

♻️ Proposed rename

-func (b *Batcher[T]) publish(ctx context.Context, item T, nonBlockingWhenFull bool) error {
+func (b *Batcher[T]) publish(ctx context.Context, item T, allowUnboundedFastPath bool) error {

-	if nonBlockingWhenFull && b.config.MaxQueueSize <= 0 {
+	if allowUnboundedFastPath && b.config.MaxQueueSize <= 0 {
 		// Unbounded fast path: publication cannot fail.
 		_ = b.input.tryPush(item)

🤖 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/batcher.go` around lines 166 - 171, Rename the
nonBlockingWhenFull parameter and all references in the surrounding Add flow to
unboundedFastPath, preserving the existing condition and behavior so the fast
path is selected only when MaxQueueSize <= 0 while bounded queues continue using
push.

---

553-563: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

The finish fallback is unreachable, and its failure mode is silent.

coordinateDrain runs once under shutdownOnce, and it CASes stateSealing→stateDraining at Line 511 before calling finish. beginSealing never lowers the state below stateSealing. So the state is always stateDraining here and the first CAS always succeeds. The comment "Another caller may already be finishing" does not match the code.

If the second branch ever did run, errorsChan would stay open and every consumer of Errors() would block forever. Make that path loud instead of silent, or drop it.

♻️ Proposed simplification

 func (b *Batcher[T]) finish() {
-	if b.gate.state.CompareAndSwap(stateDraining, stateClosed) {
-		close(b.errorsChan)
-
-		return
-	}
-
-	// Another caller may already be finishing; make sure the terminal state is
-	// visible before returning.
-	b.gate.state.CompareAndSwap(stateSealing, stateClosed)
+	// coordinateDrain runs exactly once and has already moved the state to
+	// stateDraining, so this transition cannot lose a race.
+	b.gate.state.Store(stateClosed)
+
+	close(b.errorsChan)
 }

🤖 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/batcher.go` around lines 553 - 563, Update Batcher.finish to
remove the unreachable stateSealing fallback and its misleading comment, or make
any unexpected state transition fail loudly instead of returning silently with
errorsChan open. Preserve the successful stateDraining-to-stateClosed transition
and errorsChan closure.

pkg/batcher/gate.go (1)

124-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove the unused isSealed method, or use it.

golangci-lint reports func (*admissionGate).isSealed is unused. That fails the lint stage. The doc comment also names the field, not the method.

♻️ Proposed removal

-// sealed reports whether admission is closed.
-func (g *admissionGate) isSealed() bool {
-	return g.sealed.Load()
-}
-

🤖 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/gate.go` around lines 124 - 127, Remove the unused
admissionGate.isSealed method from pkg/batcher/gate.go, including its inaccurate
doc comment, rather than leaving an unreferenced helper that fails lint.

Source: Linters/SAST tools

pkg/batcher/queue.go (1)

60-92: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Consider re-arming notFull after a successful push while space remains.

notFull holds one pending signal. One pop therefore releases exactly one parked publisher, even when the bounded queue has many free slots. The remaining publishers stay parked until the next pop. Progress still happens, because the aggregator pops on every wakeup, but admission latency under a full bounded queue is higher than the free space justifies.

Signal notFull again after a successful append when len(q.items)-q.head < q.capacity still holds.

🤖 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.go` around lines 60 - 92, Update queue.push so that after a
successful append to a bounded queue, it re-signals notFull when remaining
queued items are still below capacity. Preserve the existing unbounded-queue
behavior and only re-arm the signal after the item is appended and the mutex is
released.

pkg/batcher/shutdown.go (1)

51-55: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Use a shallow sentinel check in Is.

errors.Is(target, ErrTimeout) checks whether target wraps ErrTimeout, so *ShutdownIncompleteError also matches wrapped timeout errors. Use target == ErrTimeout, then remove the errors import if unused.

🤖 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/shutdown.go` around lines 51 - 55, Update
ShutdownIncompleteError.Is to use a direct target == ErrTimeout comparison,
limiting equivalence to the exact sentinel; remove the errors import if it
becomes unused.

🤖 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/plan-perf.md`:
- Around line 1044-1046: Update the Go version in thresholds.md’s keyed
reference environment baseline from 1.22.4 to 1.25.0 to match go.mod, and ensure
any newly introduced CI tooling remains gated for Go 1.25.0 or newer.

In `@docs/improvements/thresholds.md`:
- Around line 30-38: Make the Phase 3 goroutine requirements in the thresholds
document consistent with the table’s “exactly 2 goroutines per running batcher”
gate. Update the Phase 3 table heading and rows around the existing goroutine
requirements, or explicitly scope them as Phase 3-only without contradicting the
global gate; remove the conflicting 1 and 1+n requirements.

In `@pkg/batcher/batcher.go`:
- Around line 477-486: Update the shutdown wait logic in Shutdown to check
b.shutdownDone before entering the select, then retain the existing select for
cases where completion has not yet occurred. Return nil whenever the drain is
already complete, including when ctx.Done() is simultaneously ready, while
preserving the existing ShutdownIncompleteError behavior for an incomplete
drain.

In `@pkg/batcher/goroutine_leak_test.go`:
- Around line 347-350: Replace the Stats().Pending polling in the in-flight
timeout test with a processor-owned entry signal, and wait on that signal before
calling Close(). Update the test’s processor setup to signal immediately when
processor execution begins, while preserving the existing timeout and
assertions.

In `@pkg/batcher/options.go`:
- Around line 76-77: Update the godoc comment for WithErrorBufferSize to clearly
state the diagnostic buffer capacity and separately explain that, once full, the
oldest errors are retained while newer errors are dropped.

In `@pkg/batcher/queue.go`:
- Around line 113-148: Update queue.pop to compact q.items when q.head becomes
sufficiently large: move the unconsumed suffix to the front, reslice to the
remaining count, and reset q.head to zero while preserving item order and
capacity. Keep the existing fully-drained reset and signaling behavior, and
ensure push/tryPush continue enforcing the queue bound using the compacted
state.

In `@pkg/batcher/shutdown_test.go`:
- Around line 237-241: Update the shutdown completion select around shutdownDone
to receive and assert the returned error rather than discarding it. Fail the
trial when Shutdown reports a ShutdownIncompleteError, while preserving the
existing timeout failure for a shutdown that does not complete.

In `@pkg/batcher/stats.go`:
- Around line 5-20: Correct the Stats documentation to remove the claim that
snapshots never take a lock or are entirely allocation-free, since Queued is
populated via queue.length(). State that reading Queued acquires the queue mutex
and may contend with publishers and the aggregator, while preserving the
existing O(1), eventually consistent snapshot description.

In `@README.md`:
- Around line 174-187: Update the README shutdown example around batcher.Close
so it handles the returned incomplete-drain error instead of discarding it
through defer. Show explicit Close error handling or propagate the deferred
result, while preserving the documented behavior that remaining work continues
in the background after the grace period.
- Around line 220-223: Update the batcher.New example to pass the declared
processor’s Process method to batcher.WithProcessor instead of passing the
processor struct, matching the processor function contract and the earlier
usage.

---

Nitpick comments:
In `@pkg/batcher/admission_test.go`:
- Around line 226-259: Register test cleanup for the batcher instance after
defer close(release) so the processor is unblocked before b.Close waits for
draining; apply the same lifecycle cleanup and ensure the Close goroutine is
awaited in TestBlockedEnqueueIsReleasedByShutdown.
- Around line 31-35: Reduce repeated race-test cost by making the shared trials
value in pkg/batcher/admission_test.go lines 31-35 short-test aware, using a
small count when testing.Short() is enabled while preserving the current
default. Apply the same testing.Short() reduction to the 300-trial loop at
pkg/batcher/admission_test.go lines 110 and 145, the 300-trial loop at
pkg/batcher/shutdown_test.go line 190, and the 200-trial loop at
pkg/batcher/shutdown_test.go line 284; leave normal trial counts unchanged.

In `@pkg/batcher/batcher.go`:
- Around line 166-171: Rename the nonBlockingWhenFull parameter and all
references in the surrounding Add flow to unboundedFastPath, preserving the
existing condition and behavior so the fast path is selected only when
MaxQueueSize <= 0 while bounded queues continue using push.
- Around line 553-563: Update Batcher.finish to remove the unreachable
stateSealing fallback and its misleading comment, or make any unexpected state
transition fail loudly instead of returning silently with errorsChan open.
Preserve the successful stateDraining-to-stateClosed transition and errorsChan
closure.

In `@pkg/batcher/characterization_test.go`:
- Around line 387-404: Remove the hand-rolled itoa helper and update keyFor to
use strconv.Itoa directly for integer conversion. Add the strconv import and
preserve keyFor’s existing “item-” prefix and output behavior.
- Around line 105-117: The timing assertion in the first-item flush test should
enforce that the batch does not flush before approximately the full interval
measured from Add of the first item. Update the require.GreaterOrEqual check
around start, b.Add, and elapsed to use interval minus a small, explicit
tolerance, while preserving the existing batch-count and item-count assertions.

In `@pkg/batcher/gate.go`:
- Around line 124-127: Remove the unused admissionGate.isSealed method from
pkg/batcher/gate.go, including its inaccurate doc comment, rather than leaving
an unreferenced helper that fails lint.

In `@pkg/batcher/queue.go`:
- Around line 60-92: Update queue.push so that after a successful append to a
bounded queue, it re-signals notFull when remaining queued items are still below
capacity. Preserve the existing unbounded-queue behavior and only re-arm the
signal after the item is appended and the mutex is released.

In `@pkg/batcher/shutdown.go`:
- Around line 51-55: Update ShutdownIncompleteError.Is to use a direct target ==
ErrTimeout comparison, limiting equivalence to the exact sentinel; remove the
errors import if it becomes unused.

🪄 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 Plus

Run ID: 2e7f2a56-4e28-44fe-accd-fae3e81e47ed

📥 Commits

Reviewing files that changed from the base of the PR and between 32da402 and 8f1dfeb.

⛔ Files ignored due to path filters (1)

• go.sum is excluded by !**/*.sum

📒 Files selected for processing (21)

• README.md
• docs/improvements/plan-perf.md
• docs/improvements/thresholds.md
• go.mod
• pkg/batcher/admission_test.go
• pkg/batcher/batcher.go
• pkg/batcher/batcher_test.go
• pkg/batcher/characterization_test.go
• pkg/batcher/constants.go
• pkg/batcher/diagnostics_test.go
• pkg/batcher/fx.go
• pkg/batcher/fx_test.go
• pkg/batcher/gate.go
• pkg/batcher/gate_internal_test.go
• pkg/batcher/goroutine_budget_test.go
• pkg/batcher/goroutine_leak_test.go
• pkg/batcher/options.go
• pkg/batcher/queue.go
• pkg/batcher/shutdown.go
• pkg/batcher/shutdown_test.go
• pkg/batcher/stats.go

[coderabbitai]

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.

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (1)

docs/improvements/thresholds.md (1)

54-59: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Document the queue capacity contract.

admissionGate tracks publishers; it does not limit queued items. State that MaxQueueSize <= 0 selects the unbounded path used by this allocation gate, while positive values bound the queue. In bounded mode, Add blocks when full and Enqueue returns ctx.Err() or ErrClosing when it cannot accept an item.

🤖 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 54 - 59, Update the queue
capacity documentation near admissionGate to state that it tracks publishers
rather than limiting queued items. Document that MaxQueueSize <= 0 selects the
unbounded allocation-gate path, while positive values bound the queue; specify
that bounded Add blocks when full and Enqueue returns ctx.Err() or ErrClosing
when it cannot accept an item.

🤖 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 @.github/workflows/go.yml:
- Around line 110-112: The comment above go-version-file must accurately
describe the change: replace the claim about a hardcoded floor or pinned Go
version with wording that explains removing version drift and aligning CI with
the Go version declared in go.mod. Keep the go-version-file setting unchanged.
- Around line 110-112: Update the setup-go configuration in the build, test, and
coverage jobs to use go-version-file: go.mod, ensuring each job derives its Go
version from the module directive instead of the runner’s pre-installed version.

In `@pkg/batcher/admission_test.go`:
- Around line 257-258: The assertion for a cancelled Enqueue in the relevant
admission test incorrectly expects Rejected to increase. Update it to match the
accounting contract: a full-queue deadline rolls back the reservation without
incrementing Rejected, while preserving the surrounding deadline behavior
assertions.

---

Nitpick comments:
In `@docs/improvements/thresholds.md`:
- Around line 54-59: Update the queue capacity documentation near admissionGate
to state that it tracks publishers rather than limiting queued items. Document
that MaxQueueSize <= 0 selects the unbounded allocation-gate path, while
positive values bound the queue; specify that bounded Add blocks when full and
Enqueue returns ctx.Err() or ErrClosing when it cannot accept an item.

🪄 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: 7e3e9fb2-d1dd-410f-847a-02c48669d4cf

📥 Commits

Reviewing files that changed from the base of the PR and between 3f48a40 and c58d01c.

⛔ Files ignored due to path filters (1)

• go.sum is excluded by !**/*.sum

📒 Files selected for processing (24)

• .github/workflows/bench.yml
• .github/workflows/go.yml
• README.md
• docs/improvements/plan-perf.md
• docs/improvements/thresholds.md
• go.mod
• pkg/batcher/admission_test.go
• pkg/batcher/batcher.go
• pkg/batcher/batcher_test.go
• pkg/batcher/characterization_test.go
• pkg/batcher/constants.go
• pkg/batcher/diagnostics_test.go
• pkg/batcher/fx.go
• pkg/batcher/fx_test.go
• pkg/batcher/gate.go
• pkg/batcher/gate_internal_test.go
• pkg/batcher/goroutine_budget_test.go
• pkg/batcher/goroutine_leak_test.go
• pkg/batcher/options.go
• pkg/batcher/queue.go
• pkg/batcher/queue_test.go
• pkg/batcher/shutdown.go
• pkg/batcher/shutdown_test.go
• pkg/batcher/stats.go

🚧 Files skipped from review as they are similar to previous changes (14)

• pkg/batcher/batcher_test.go
• pkg/batcher/goroutine_leak_test.go
• pkg/batcher/options.go
• pkg/batcher/shutdown.go
• pkg/batcher/fx_test.go
• pkg/batcher/gate_internal_test.go
• pkg/batcher/stats.go
• docs/improvements/plan-perf.md
• pkg/batcher/fx.go
• pkg/batcher/constants.go
• go.mod
• pkg/batcher/shutdown_test.go
• pkg/batcher/characterization_test.go
• pkg/batcher/batcher.go

[coderabbitai]

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.

[coderabbitai]

Actionable comments posted: 2

🤖 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 `@pkg/batcher/gate.go`:
- Around line 124-127: Resolve the unused admissionGate.isSealed method by
removing it if no current code requires sealed-state checks; otherwise, add its
intended caller and ensure the returned state is used. Do not leave the method
unused.

In `@README.md`:
- Around line 205-218: Rename the batcher instance used in the README example
from batcher to a distinct identifier such as b, updating both Shutdown calls
while retaining batcher.ShutdownIncompleteError for the imported package type.

🪄 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: 63d9123a-fc66-40cb-9c60-871cc13810c3

📥 Commits

Reviewing files that changed from the base of the PR and between 8b6d33c and e0353a5.

⛔ Files ignored due to path filters (1)

• go.sum is excluded by !**/*.sum

📒 Files selected for processing (23)

• .github/workflows/bench.yml
• README.md
• docs/improvements/plan-perf.md
• docs/improvements/thresholds.md
• go.mod
• pkg/batcher/admission_test.go
• pkg/batcher/batcher.go
• pkg/batcher/batcher_test.go
• pkg/batcher/characterization_test.go
• pkg/batcher/constants.go
• pkg/batcher/diagnostics_test.go
• pkg/batcher/fx.go
• pkg/batcher/fx_test.go
• pkg/batcher/gate.go
• pkg/batcher/gate_internal_test.go
• pkg/batcher/goroutine_budget_test.go
• pkg/batcher/goroutine_leak_test.go
• pkg/batcher/options.go
• pkg/batcher/queue.go
• pkg/batcher/queue_test.go
• pkg/batcher/shutdown.go
• pkg/batcher/shutdown_test.go
• pkg/batcher/stats.go

🚧 Files skipped from review as they are similar to previous changes (21)

• pkg/batcher/fx_test.go
• pkg/batcher/goroutine_leak_test.go
• pkg/batcher/queue.go
• .github/workflows/bench.yml
• pkg/batcher/shutdown.go
• pkg/batcher/constants.go
• docs/improvements/thresholds.md
• pkg/batcher/stats.go
• pkg/batcher/diagnostics_test.go
• pkg/batcher/goroutine_budget_test.go
• go.mod
• pkg/batcher/options.go
• pkg/batcher/fx.go
• docs/improvements/plan-perf.md
• pkg/batcher/queue_test.go
• pkg/batcher/shutdown_test.go
• pkg/batcher/gate_internal_test.go
• pkg/batcher/characterization_test.go
• pkg/batcher/admission_test.go
• pkg/batcher/batcher.go
• pkg/batcher/batcher_test.go

[coderabbitai]

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)

docs/improvements/plan-perf.md (1)

508-510: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Align the Phase 2.2 goroutine budget.

The plan defines incompatible budgets. Line 509 and Line 706 require one goroutine for a running n=1 batcher. Lines 1163-1165 report two goroutines, which also matches the PR objective. Update the Phase 2.2 threshold and acceptance criteria to two, and clarify whether the single-goroutine target applies only after Phase 3.

This conflict can make lifecycle tests reject the intended implementation or accept the wrong budget.

Also applies to: 589-593, 705-707, 1163-1167

🤖 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 508 - 510, Update the Phase 2.2
goroutine budget references in the plan so a running n=1 batcher consistently
requires two goroutines, including the threshold and acceptance criteria at the
referenced sections. Explicitly state that the one-goroutine target applies only
after Phase 3, while preserving the zero-idle and post-closed baseline
requirements.

🧹 Nitpick comments (1)

docs/improvements/plan-perf.md (1)

164-164: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add language identifiers to fenced blocks.

markdownlint-cli2 reports MD040 for these fences. Add text to pseudocode and ASCII blocks, and go where the block contains Go code.

Also applies to: 208-208, 229-229, 284-284, 321-321, 375-375, 1137-1137

🤖 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` at line 164, Add explicit language
identifiers to every fenced code block called out in plan-perf.md: use text for
pseudocode and ASCII diagrams, and go for blocks containing Go code. Update all
listed fence locations so markdownlint MD040 passes without changing the block
contents.

Source: Linters/SAST tools

🤖 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 `@README.md`:
- Around line 238-241: Update the error-handling documentation around b.Enqueue
to describe both possible context errors: context.Canceled and
context.DeadlineExceeded, while retaining batcher.ErrClosing as the shutdown
case.

---

Outside diff comments:
In `@docs/improvements/plan-perf.md`:
- Around line 508-510: Update the Phase 2.2 goroutine budget references in the
plan so a running n=1 batcher consistently requires two goroutines, including
the threshold and acceptance criteria at the referenced sections. Explicitly
state that the one-goroutine target applies only after Phase 3, while preserving
the zero-idle and post-closed baseline requirements.

---

Nitpick comments:
In `@docs/improvements/plan-perf.md`:
- Line 164: Add explicit language identifiers to every fenced code block called
out in plan-perf.md: use text for pseudocode and ASCII diagrams, and go for
blocks containing Go code. Update all listed fence locations so markdownlint
MD040 passes without changing the block contents.

🪄 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: 3716646d-393b-4d68-96fe-b336c7568872

📥 Commits

Reviewing files that changed from the base of the PR and between e0353a5 and c578f61.

📒 Files selected for processing (5)

• README.md
• docs/improvements/plan-perf.md
• pkg/batcher/admission_test.go
• pkg/batcher/gate.go
• pkg/batcher/options.go

🚧 Files skipped from review as they are similar to previous changes (3)

• pkg/batcher/gate.go
• pkg/batcher/options.go
• pkg/batcher/admission_test.go