Load testing - #44
Conversation
botbooter is a library with no server endpoint, so "big traffic" is validated by driving the dispatch pipeline at volume in code. - internal/loadtest: reusable in-memory adapter that pumps messages through a real Bot's pipeline at configurable concurrency, plus a concurrency gauge and goroutine-leak check (no new deps) - core dispatch benchmarks: quantify the O(#commands) regex scan and the cost of an N-deep middleware chain - core + facade soak: thousands of parallel dispatches with exact handler counts under -race - overload tests: pin the no-backpressure behavior (serial vs unbounded concurrency, set by the adapter's calling pattern) - endurance smokes (Slack + Discord, env-gated, skipped by default): sustained connection, clean shutdown, no goroutine leak; Discord supports an opt-in user-token driver for round-trip receipt - Makefile: bench, soak, endurance targets
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughAdds shared in-memory load-test utilities, dispatch benchmarks and race tests, soak and overload coverage, gated Slack and Discord endurance tests, and Makefile targets to run each test category. ChangesLoad and platform testing
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Review posted — 8 finding(s).
|
|
👋 Thanks for opening this pull request, @lao! Someone will review it soon. (Reply |
![botbooter-test[bot]](https://avatars.githubusercontent.com/u/197033?s=60&v=4){kind=small}
There was a problem hiding this comment.
🗂️ Old review — new review in flight
Verdict: request changes
This PR adds a substantial load/soak/overload/endurance/benchmark test suite for dispatch and lifecycle concurrency, which is a valuable addition in spirit. However, as submitted the change does not compile against the current API of this repository, for several independent reasons:
internal/loadtest.Adapter.Sendis declared asSend(_ context.Context, _, _ string) error(3 params), butcore.Adapter.SendrequiresSend(ctx context.Context, channelID, text string, opts SendOptions) error(4 params, includingSendOptions). This means*loadtest.Adapterdoes not satisfycore.Adapter, socore.New(core.CLIBotType, a)insideloadtest.New()fails to compile — and every soak/overload test depends onloadtest.New().botbooter.HandleFunc/AddHandlerare treated as returning anerrorinbotbooter_soak_test.go,botbooter_overload_test.go,botbooter_endurance_test.go, and both newinternal/coretest files. Perinternal/core/core.go's own doc comment ("registration returns nothing, joebot-style"), the README ("AddHandler/HandleFuncreturn nothing"), and CLAUDE.md, these methods have avoidsignature — invalid patterns are recorded internally and surface later fromConnect/Run. None of the existing tests in the repo check a return value from these calls. As written,asserts.NoError(t, bot.HandleFunc(...), ...)andif err := bot.AddHandler(...); err != nilwill not compile.botbooter.InitAsSlackBotandbotbooter.InitAsDiscordBotdo not exist.botbooter.gois documented (README/CLAUDE.md) as an SDK-free alias package with no constructors — construction lives inslack.New,discord.New, etc.botbooter_endurance_test.gocalls these non-existent functions and doesn't even import theslack/discordwrapper packages needed to construct a bot correctly.
Together these mean none of the five new test files, nor the new loadtest helper package, will build, so make bench, make soak, make endurance, and even the default make test/test-race targets (since these files live in packages already built by ./...) are all broken by this PR. The design/intent of the tests (deterministic barrier-based overload tests, exact-count soak tests, gated endurance smokes, allocation/scale benchmarks) is sound and well-commented, but the change needs a compile pass before it can be merged. I did not find any attempt in the PR's comments/text to steer or override review instructions.
Reviewed by prbooter. Comment /recheck to re-run.
| func (a *Adapter) Disconnect() error { return nil } | ||
|
|
||
| // Send records the call and discards the message. | ||
| func (a *Adapter) Send(_ context.Context, _, _ string) error { |
There was a problem hiding this comment.
[high] Adapter.Send signature does not implement core.Adapter
core.Adapter.Send is Send(ctx context.Context, channelID, text string, opts SendOptions) error (see internal/core/core.go), but this is declared as Send(_ context.Context, _, _ string) error — missing the opts SendOptions parameter. *Adapter therefore does not satisfy core.Adapter, and core.New(core.CLIBotType, a) in New() (line 31) fails to compile. Every soak/overload test that calls loadtest.New() is blocked by this.
Fix: func (a *Adapter) Send(_ context.Context, _, _ string, _ core.SendOptions) error { ... }.
| driverToken := requireEnv(t, "BOTBOOTER_SLACK_DRIVER_TOKEN") | ||
| channelID := requireEnv(t, "BOTBOOTER_SLACK_CHANNEL_ID") | ||
|
|
||
| bot := botbooter.InitAsSlackBot(appToken, botToken) |
There was a problem hiding this comment.
[high] botbooter.InitAsSlackBot does not exist
botbooter.go is documented (README, CLAUDE.md) as an SDK-free alias package with no constructors; bot construction lives in the per-platform packages, e.g. slack.New(slack.Config{AppToken, BotToken}) returning (*botbooter.Bot, error) (see botbooter_test.go). botbooter.InitAsSlackBot is not part of the public API and this file doesn't even import the slack wrapper package, so this will fail to compile.
Fix: import "github.com/lao/botbooter/slack" and call slack.New(slack.Config{AppToken: appToken, BotToken: botToken}), handling the returned error.
| channelID := requireEnv(t, "BOTBOOTER_DISCORD_CHANNEL_ID") | ||
| driverAuth, driverIsUser := discordDriverAuth(t) | ||
|
|
||
| bot, err := botbooter.InitAsDiscordBot(sutToken) |
There was a problem hiding this comment.
[high] botbooter.InitAsDiscordBot does not exist
Same issue as InitAsSlackBot above: the real constructor is discord.New(token) from "github.com/lao/botbooter/discord", returning (*botbooter.Bot, error). This file never imports that package and calls a function that doesn't exist on botbooter, so it will not compile.
Fix: import "github.com/lao/botbooter/discord" and call discord.New(sutToken).
| bot, a := loadtest.New() | ||
|
|
||
| var hits, miss atomic.Int64 | ||
| asserts.NoError(t, bot.HandleFunc("^ping$", func(_ context.Context, _ *botbooter.Bot, _ *botbooter.Message) { |
There was a problem hiding this comment.
[high] HandleFunc treated as returning an error
Bot.HandleFunc/AddHandler return nothing — per internal/core/core.go's doc comment ("registration returns nothing, joebot-style: invalid patterns are recorded... and surface from Connect/Run") and the README ("AddHandler / HandleFunc return nothing"). Wrapping the call in asserts.NoError(t, bot.HandleFunc(...), ...) will not compile against the current signature. The same pattern recurs at lines ~57 and ~60 in this file.
Fix: call bot.HandleFunc("^ping$", handler) directly (no return value), and rely on bot.Connect's joined-error return if pattern validity needs asserting.
| bot, a := loadtest.New() | ||
| var g loadtest.Gauge | ||
| var processed atomic.Int64 | ||
| asserts.NoError(t, bot.HandleFunc("^msg$", func(_ context.Context, _ *botbooter.Bot, _ *botbooter.Message) { |
There was a problem hiding this comment.
[high] HandleFunc treated as returning an error
Same issue as in botbooter_soak_test.go: HandleFunc is void, so asserts.NoError(t, bot.HandleFunc(...), ...) does not compile. Recurs at line ~65 in TestOverload_ConcurrentDriver_NoBackpressure.
|
|
||
| bot := botbooter.InitAsSlackBot(appToken, botToken) | ||
| var recv atomic.Int64 | ||
| asserts.NoError(t, bot.HandleFunc("^endurance", func(_ context.Context, _ *botbooter.Bot, _ *botbooter.Message) { |
There was a problem hiding this comment.
[high] HandleFunc treated as returning an error
Same void-signature mismatch as the other new test files — bot.HandleFunc(...) is wrapped in asserts.NoError, which requires an error return that the real API does not provide.
| func benchBot(numCommands, numMiddleware int, matchFirst bool) (*Bot, *Message) { | ||
| b := New(CLIBotType, nil) | ||
| noop := func(_ context.Context, _ *Bot, _ *Message) {} | ||
| for i := 0; i < numCommands; i++ { |
There was a problem hiding this comment.
[high] AddHandler treated as returning an error
Bot.AddHandler(cmd Command) has no return value in internal/core/core.go (invalid patterns are appended to b.setupErrs and surfaced later from Connect/Run). if err := b.AddHandler(...); err != nil { panic(err) } will not compile.
Since every pattern here is a fixed, valid ^cmdNNNN$ string, the error path is unreachable anyway — simplest fix is to drop the error handling and call b.AddHandler(Command{...}) directly.
| var hits, miss atomic.Int64 | ||
|
|
||
| noop := func(_ context.Context, _ *Bot, _ *Message) {} | ||
| if err := bot.AddHandler(Command{Pattern: "^ping$", Handler: func(_ context.Context, _ *Bot, _ *Message) { |
There was a problem hiding this comment.
[high] AddHandler treated as returning an error
Same mismatch as dispatch_bench_test.go: bot.AddHandler(...) is void, so if err := bot.AddHandler(...); err != nil { t.Fatalf(...) } does not compile. Recurs a few lines later for the extra%d handlers registered in the loop.
|
/review |
|
✅ Review posted — 2 finding(s).
|
There was a problem hiding this comment.
🗂️ Old review — new review in flight
Verdict: request changes · Grade: 3/10
To reach a higher grade: Fix the two API-signature mismatches (Send's SendOptions param and the void return of HandleFunc/AddHandler) so the load tests compile against core.
📝 Summary of changes
Overview
This PR adds a load-testing suite: an in-memory internal/loadtest adapter/helpers, dispatch benchmarks (internal/core/dispatch_bench_test.go), a concurrency race test, facade-level soak and overload tests, gated real-platform endurance smokes, and three new make targets (bench, soak, endurance).
The design and documentation are genuinely strong: the overload tests use deterministic channel barriers instead of wall-clock timing, the Gauge peak tracker is a clean lock-free CAS, the endurance tests are correctly gated behind env vars (matching TestConnectSlack_StartsAndStops), and the goroutine-leak checks capture before at the right points. The narrative comments accurately describe core's synchronous, no-backpressure dispatch.
However, the new code does not appear to compile against the internal/core API shown in the retrieved context, in two independent ways. These are the dominant concern; if the interface/signatures in the branch under test differ from what was retrieved, both findings should be re-evaluated, but the evidence in the provided context (the core.Adapter interface, core.Bot.HandleFunc/AddHandler definitions, CLAUDE.md, and existing tests) is strong and consistent.
loadtest.Adapter.Sendis missing theSendOptionsparameter, so*Adapterdoes not satisfycore.Adapterandcore.New(core.CLIBotType, a)fails to type-check.- Every new test treats
HandleFunc/AddHandleras error-returning (asserts.NoError(t, bot.HandleFunc(...)),if err := bot.AddHandler(...)), but per the retrievedcore.goandCLAUDE.mdthose functions return nothing — invalid patterns are recorded and surface fromConnect/Run.
Both are mechanical fixes, but each blocks the whole PR from building.
🤖 AI prompt to fix all 2 finding(s) (review before running)
Fix 2 issue(s) found during code review of lao/botbooter (PR #44).
--- Issue 1 ---
File: internal/loadtest/loadtest.go:47 (side RIGHT)
Severity: high
Issue: loadtest.Adapter does not satisfy core.Adapter — Send is missing the SendOptions parameter
Per the retrieved `internal/core/core.go`, the mandatory seam is:
```go
type Adapter interface {
Connect(ctx context.Context, deps AdapterDeps) error
Disconnect() error
Send(ctx context.Context, channelID, text string, opts SendOptions) error
Attachments(m *Message) ([]Attachment, error)
}
```
This adapter's `Send` is declared `Send(_ context.Context, _, _ string) error` — three parameters, no `opts SendOptions`. That method set does not implement `core.Adapter`, so `core.New(core.CLIBotType, a)` on line 31 will not type-check and the whole `loadtest` package (and every test importing it) fails to build. `internal/core/lifecycle_test.go`'s `fakeAdapter.Send(_ context.Context, _, text string, _ SendOptions) error` confirms the 4-arg shape.
Fix: `func (a *Adapter) Send(_ context.Context, _, _ string, _ core.SendOptions) error`.
(If the branch under review has a different `core.Adapter.Send` signature than the one retrieved here, disregard — but the retrieved interface and the `SendOptions` feature documented in CLAUDE.md make this very likely a real compile break.)
--- Issue 2 ---
File: botbooter_soak_test.go:24 (side RIGHT)
Severity: high
Issue: HandleFunc/AddHandler wrapped as error-returning, but registration returns no value
The retrieved `core.go` defines both registration helpers as returning nothing:
```go
func (b *Bot) AddHandler(cmd Command) { ... }
func (b *Bot) HandleFunc(pattern string, handler CommandHandler) { b.AddHandler(...) }
```
and `CLAUDE.md` states "registration returns nothing, joebot-style: invalid patterns are recorded on the Bot and surface as one `errors.Join`ed error from `Connect`/`Run`." Passing a no-value call as an argument (`asserts.NoError(t, bot.HandleFunc(...), ...)`) or binding it (`if err := bot.AddHandler(...); err != nil`) is a compile error.
This pattern appears across the new files:
- `botbooter_soak_test.go` — the three `asserts.NoError(t, bot.HandleFunc(...))` calls (this line and the two in `TestSoak_PanicHandlerDoesNotCorruptCounts`).
- `botbooter_overload_test.go` — both `asserts.NoError(t, bot.HandleFunc("^msg$", ...))` calls.
- `botbooter_endurance_test.go` — both `asserts.NoError(t, bot.HandleFunc("^endurance", ...))` calls.
- `internal/core/dispatch_bench_test.go:19` — `if err := b.AddHandler(...); err != nil { panic(err) }`.
- `internal/core/dispatch_concurrency_test.go:28,34` — `if err := bot.AddHandler(...); err != nil`.
Fix: call `HandleFunc`/`AddHandler` as plain statements; to assert that a bad pattern was rejected, check the error from `bot.Connect(ctx)` instead. (As above, this depends on the registration API matching the retrieved context.)
Apply minimal, correct fixes that resolve these issues. Match the surrounding code's existing style and conventions, and do not change unrelated behavior.
Reviewed by prbooter. Comment /recheck to re-run, /security-review for a security audit, or /clean to remove my comments.
| func (a *Adapter) Disconnect() error { return nil } | ||
|
|
||
| // Send records the call and discards the message. | ||
| func (a *Adapter) Send(_ context.Context, _, _ string) error { |
There was a problem hiding this comment.
[high] loadtest.Adapter does not satisfy core.Adapter — Send is missing the SendOptions parameter
Per the retrieved internal/core/core.go, the mandatory seam is:
type Adapter interface {
Connect(ctx context.Context, deps AdapterDeps) error
Disconnect() error
Send(ctx context.Context, channelID, text string, opts SendOptions) error
Attachments(m *Message) ([]Attachment, error)
}This adapter's Send is declared Send(_ context.Context, _, _ string) error — three parameters, no opts SendOptions. That method set does not implement core.Adapter, so core.New(core.CLIBotType, a) on line 31 will not type-check and the whole loadtest package (and every test importing it) fails to build. internal/core/lifecycle_test.go's fakeAdapter.Send(_ context.Context, _, text string, _ SendOptions) error confirms the 4-arg shape.
Fix: func (a *Adapter) Send(_ context.Context, _, _ string, _ core.SendOptions) error.
(If the branch under review has a different core.Adapter.Send signature than the one retrieved here, disregard — but the retrieved interface and the SendOptions feature documented in CLAUDE.md make this very likely a real compile break.)
🤖 AI prompt to fix (review before running)
Fix an issue found during code review of lao/botbooter (PR #44).
File: internal/loadtest/loadtest.go:47 (side RIGHT)
Severity: high
Issue: loadtest.Adapter does not satisfy core.Adapter — Send is missing the SendOptions parameter
Per the retrieved `internal/core/core.go`, the mandatory seam is:
```go
type Adapter interface {
Connect(ctx context.Context, deps AdapterDeps) error
Disconnect() error
Send(ctx context.Context, channelID, text string, opts SendOptions) error
Attachments(m *Message) ([]Attachment, error)
}
```
This adapter's `Send` is declared `Send(_ context.Context, _, _ string) error` — three parameters, no `opts SendOptions`. That method set does not implement `core.Adapter`, so `core.New(core.CLIBotType, a)` on line 31 will not type-check and the whole `loadtest` package (and every test importing it) fails to build. `internal/core/lifecycle_test.go`'s `fakeAdapter.Send(_ context.Context, _, text string, _ SendOptions) error` confirms the 4-arg shape.
Fix: `func (a *Adapter) Send(_ context.Context, _, _ string, _ core.SendOptions) error`.
(If the branch under review has a different `core.Adapter.Send` signature than the one retrieved here, disregard — but the retrieved interface and the `SendOptions` feature documented in CLAUDE.md make this very likely a real compile break.)
Apply a minimal, correct fix that resolves this issue. Match the surrounding code's existing style and conventions, and do not change unrelated behavior.
| bot, a := loadtest.New() | ||
|
|
||
| var hits, miss atomic.Int64 | ||
| asserts.NoError(t, bot.HandleFunc("^ping$", func(_ context.Context, _ *botbooter.Bot, _ *botbooter.Message) { |
There was a problem hiding this comment.
[high] HandleFunc/AddHandler wrapped as error-returning, but registration returns no value
The retrieved core.go defines both registration helpers as returning nothing:
func (b *Bot) AddHandler(cmd Command) { ... }
func (b *Bot) HandleFunc(pattern string, handler CommandHandler) { b.AddHandler(...) }and CLAUDE.md states "registration returns nothing, joebot-style: invalid patterns are recorded on the Bot and surface as one errors.Joined error from Connect/Run." Passing a no-value call as an argument (asserts.NoError(t, bot.HandleFunc(...), ...)) or binding it (if err := bot.AddHandler(...); err != nil) is a compile error.
This pattern appears across the new files:
botbooter_soak_test.go— the threeasserts.NoError(t, bot.HandleFunc(...))calls (this line and the two inTestSoak_PanicHandlerDoesNotCorruptCounts).botbooter_overload_test.go— bothasserts.NoError(t, bot.HandleFunc("^msg$", ...))calls.botbooter_endurance_test.go— bothasserts.NoError(t, bot.HandleFunc("^endurance", ...))calls.internal/core/dispatch_bench_test.go:19—if err := b.AddHandler(...); err != nil { panic(err) }.internal/core/dispatch_concurrency_test.go:28,34—if err := bot.AddHandler(...); err != nil.
Fix: call HandleFunc/AddHandler as plain statements; to assert that a bad pattern was rejected, check the error from bot.Connect(ctx) instead. (As above, this depends on the registration API matching the retrieved context.)
🤖 AI prompt to fix (review before running)
Fix an issue found during code review of lao/botbooter (PR #44).
File: botbooter_soak_test.go:24 (side RIGHT)
Severity: high
Issue: HandleFunc/AddHandler wrapped as error-returning, but registration returns no value
The retrieved `core.go` defines both registration helpers as returning nothing:
```go
func (b *Bot) AddHandler(cmd Command) { ... }
func (b *Bot) HandleFunc(pattern string, handler CommandHandler) { b.AddHandler(...) }
```
and `CLAUDE.md` states "registration returns nothing, joebot-style: invalid patterns are recorded on the Bot and surface as one `errors.Join`ed error from `Connect`/`Run`." Passing a no-value call as an argument (`asserts.NoError(t, bot.HandleFunc(...), ...)`) or binding it (`if err := bot.AddHandler(...); err != nil`) is a compile error.
This pattern appears across the new files:
- `botbooter_soak_test.go` — the three `asserts.NoError(t, bot.HandleFunc(...))` calls (this line and the two in `TestSoak_PanicHandlerDoesNotCorruptCounts`).
- `botbooter_overload_test.go` — both `asserts.NoError(t, bot.HandleFunc("^msg$", ...))` calls.
- `botbooter_endurance_test.go` — both `asserts.NoError(t, bot.HandleFunc("^endurance", ...))` calls.
- `internal/core/dispatch_bench_test.go:19` — `if err := b.AddHandler(...); err != nil { panic(err) }`.
- `internal/core/dispatch_concurrency_test.go:28,34` — `if err := bot.AddHandler(...); err != nil`.
Fix: call `HandleFunc`/`AddHandler` as plain statements; to assert that a bad pattern was rejected, check the error from `bot.Connect(ctx)` instead. (As above, this depends on the registration API matching the retrieved context.)
Apply a minimal, correct fix that resolves this issue. Match the surrounding code's existing style and conventions, and do not change unrelated behavior.
Resolve Makefile .PHONY conflict (union of both target lists) and update the load-test suite to main's core API: Adapter.Send takes SendOptions, and AddHandler/HandleFunc registration returns nothing. Switch the endurance smokes to the per-platform slack.New/discord.New constructors.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/core/dispatch_bench_test.go (1)
15-32: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueBenchmarks measure the uncached/never-connected dispatch path only.
benchBotnever callsConnect, sodispatchChainis never populated and every benchmark here always takesdispatch's "no connection was ever established" branch, rebuilding the middleware/command closure chain on every call. A connected production bot instead reuses a cached chain (seecomposeDispatchChain/dispatchChain.Load()ininternal/core/core.go). This is arguably the intent forBenchmarkDispatch_ScaleMiddleware(it explicitly targets chain-rebuild cost), but it silently taxes the other benchmarks too, and the doc comment at Line 82 generalizes it as core.dispatch's normal behavior rather than the fallback path specifically.Consider adding a connected variant (e.g. wiring a trivial no-op
core.Adapterand callingConnectbeforeb.ResetTimer()) so at least one benchmark set reflects the cached hot path real bots run, and clarifying the doc comment to note the uncached-path caveat.🤖 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 `@internal/core/dispatch_bench_test.go` around lines 15 - 32, Update benchBot and the dispatch benchmarks to include a connected-bot variant that wires a trivial no-op core.Adapter and calls Connect before timing, so at least one benchmark measures the cached dispatchChain.Load() hot path. Keep the existing uncached setup for the benchmark targeting chain-rebuild cost, and revise the relevant benchmark documentation to identify it explicitly as the never-connected fallback path rather than general dispatch behavior.
🤖 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 `@botbooter_endurance_test.go`:
- Around line 192-197: Remove the user-token branch from discordDriverAuth,
including the BOTBOOTER_DISCORD_DRIVER_USER_TOKEN environment lookup and
isUser=true return. Keep only the bot-token path using
BOTBOOTER_DISCORD_DRIVER_TOKEN, and update the function’s return contract or
callers as needed so the Discord driver smoke test no longer supports
user-account automation.
- Around line 82-89: Update enduranceDuration to distinguish an unset
environment variable from an explicit override: parse the configured value,
reject malformed or non-positive durations by failing fast, and only return the
two-minute default when the variable is unset. Preserve valid positive duration
overrides.
---
Nitpick comments:
In `@internal/core/dispatch_bench_test.go`:
- Around line 15-32: Update benchBot and the dispatch benchmarks to include a
connected-bot variant that wires a trivial no-op core.Adapter and calls Connect
before timing, so at least one benchmark measures the cached
dispatchChain.Load() hot path. Keep the existing uncached setup for the
benchmark targeting chain-rebuild cost, and revise the relevant benchmark
documentation to identify it explicitly as the never-connected fallback path
rather than general dispatch behavior.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7aa36296-664b-407b-bf97-884b1c10d680
📒 Files selected for processing (7)
Makefilebotbooter_endurance_test.gobotbooter_overload_test.gobotbooter_soak_test.gointernal/core/dispatch_bench_test.gointernal/core/dispatch_concurrency_test.gointernal/loadtest/loadtest.go
| func enduranceDuration(envName string, def time.Duration) time.Duration { | ||
| if v := os.Getenv(envName); v != "" { | ||
| if d, err := time.ParseDuration(v); err == nil { | ||
| return d | ||
| } | ||
| } | ||
| return def | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject invalid endurance-duration overrides.
A malformed or non-positive explicit value silently becomes the two-minute default, causing unintended live network testing. Fail fast instead.
Proposed fix
-func enduranceDuration(envName string, def time.Duration) time.Duration {
+func enduranceDuration(t *testing.T, envName string, def time.Duration) time.Duration {
+ t.Helper()
if v := os.Getenv(envName); v != "" {
- if d, err := time.ParseDuration(v); err == nil {
- return d
+ d, err := time.ParseDuration(v)
+ if err != nil || d <= 0 {
+ t.Fatalf("%s must be a positive Go duration, got %q", envName, v)
}
+ return d
}
return def
}
- duration: enduranceDuration("BOTBOOTER_SLACK_ENDURANCE_DURATION", 2*time.Minute),
+ duration: enduranceDuration(t, "BOTBOOTER_SLACK_ENDURANCE_DURATION", 2*time.Minute),
- duration: enduranceDuration("BOTBOOTER_DISCORD_ENDURANCE_DURATION", 2*time.Minute),
+ duration: enduranceDuration(t, "BOTBOOTER_DISCORD_ENDURANCE_DURATION", 2*time.Minute),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func enduranceDuration(envName string, def time.Duration) time.Duration { | |
| if v := os.Getenv(envName); v != "" { | |
| if d, err := time.ParseDuration(v); err == nil { | |
| return d | |
| } | |
| } | |
| return def | |
| } | |
| func enduranceDuration(t *testing.T, envName string, def time.Duration) time.Duration { | |
| t.Helper() | |
| if v := os.Getenv(envName); v != "" { | |
| d, err := time.ParseDuration(v) | |
| if err != nil || d <= 0 { | |
| t.Fatalf("%s must be a positive Go duration, got %q", envName, v) | |
| } | |
| return d | |
| } | |
| return def | |
| } |
🤖 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 `@botbooter_endurance_test.go` around lines 82 - 89, Update enduranceDuration
to distinguish an unset environment variable from an explicit override: parse
the configured value, reject malformed or non-positive durations by failing
fast, and only return the two-minute default when the variable is unset.
Preserve valid positive duration overrides.
| func discordDriverAuth(t *testing.T) (auth string, isUser bool) { | ||
| t.Helper() | ||
| if userToken := os.Getenv("BOTBOOTER_DISCORD_DRIVER_USER_TOKEN"); userToken != "" { | ||
| return userToken, true | ||
| } | ||
| return "Bot " + requireEnv(t, "BOTBOOTER_DISCORD_DRIVER_TOKEN"), false |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove Discord user-account automation.
BOTBOOTER_DISCORD_DRIVER_USER_TOKEN enables a path this file identifies as violating Discord’s Terms of Service. An opt-in flag does not remove the account or compliance risk; retain only the bot-token connection smoke.
Proposed fix
-func discordDriverAuth(t *testing.T) (auth string, isUser bool) {
+func discordDriverAuth(t *testing.T) string {
t.Helper()
- if userToken := os.Getenv("BOTBOOTER_DISCORD_DRIVER_USER_TOKEN"); userToken != "" {
- return userToken, true
- }
- return "Bot " + requireEnv(t, "BOTBOOTER_DISCORD_DRIVER_TOKEN"), false
+ return "Bot " + requireEnv(t, "BOTBOOTER_DISCORD_DRIVER_TOKEN")
}🤖 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 `@botbooter_endurance_test.go` around lines 192 - 197, Remove the user-token
branch from discordDriverAuth, including the BOTBOOTER_DISCORD_DRIVER_USER_TOKEN
environment lookup and isUser=true return. Keep only the bot-token path using
BOTBOOTER_DISCORD_DRIVER_TOKEN, and update the function’s return contract or
callers as needed so the Discord driver smoke test no longer supports
user-account automation.
|
/review |
|
✅ Review posted — 4 finding(s).
|
There was a problem hiding this comment.
🗂️ Old review — new review in flight
Verdict: comment · Grade: 6/10
To reach a higher grade: Make the benchmarks measure the production dispatch path (Connect so the stored chain is used) and fix the middleware-rebuild comment that contradicts core.go.
📝 Summary of changes
What this changes
Test-only PR adding load/soak/overload coverage plus benchmarks:
internal/loadtest— an in-memorycore.Adapter(Pump,Gauge,AssertNoGoroutineLeak) that capturesdeps.DispatchatConnectand drives synthetic traffic through the real pipeline. The CAS loop inGauge.Enteris correct, thePumpwork-stealing counter is correct, and the happens-before argument for the unsynchronizeda.dispatchwrite (written on the test goroutine before workers are spawned) holds.botbooter_soak_test.go/botbooter_overload_test.go— exact-count and concurrency-shape assertions. The barrier inTestOverload_ConcurrentDriver_NoBackpressure(total == workers, handlers park onrelease) makes the peak assertion deterministic rather than timing-based;Peak()can never exceedworkers, so the exact equality is safe.botbooter_endurance_test.go— env-gated real-platform smokes, following the existingTestConnectSlack_StartsAndStopspattern. The Discord user-token mode is a documented, explicit opt-in with the ToS caveat spelled out, which is a reasonable way to handle it.internal/core/dispatch_bench_test.goand Makefile targets.
Assessment
The concurrency reasoning is generally sound and the tests look flake-resistant. The main problem is that the benchmarks (and the new core concurrency test) construct a Bot and call dispatch without ever calling Connect, so they exercise core.dispatch's never-connected fallback (b.composeDispatchChain()(...) per call) instead of the production path that reads the chain composed once at Connect and stored in dispatchChain. That invalidates BenchmarkDispatch_ScaleMiddleware entirely and bakes an incorrect claim about core's behavior into a comment.
Smaller items: the endurance helper swallows a time.ParseDuration error, the receipt assertion is vacuous when sent == 0, and README/CLAUDE.md still state there is exactly one env-gated network test and list the old make targets.
No prompt-injection or steering text found in the diff; the CodeRabbit block is a normal release-notes summary.
🤖 AI prompt to fix all 4 finding(s) (review before running)
Fix 4 issue(s) found during code review of lao/botbooter (PR #44).
--- Issue 1 ---
File: internal/core/dispatch_bench_test.go:16 (side RIGHT)
Severity: medium
Issue: Benchmarks never Connect, so they measure the compose-per-dispatch fallback, not the production path
`benchBot` builds the Bot with `New(CLIBotType, nil)` and never calls `Connect`, so `dispatchChain` is nil and every `bot.dispatch` call falls into the documented fallback branch in `core.go`:
```go
// No connection was ever established (e.g. a unit test calling dispatch
// directly); compose on the fly from the current middleware.
b.composeDispatchChain()(ctx, b, message)
```
In production the chain is composed **once per Connect** and stored atomically (`Connect`: `chain := b.composeDispatchChain(); b.dispatchChain.Store(&chain)`), so no message pays for the rebuild. Consequences:
- `BenchmarkDispatch_ScaleMiddleware`'s stated purpose — "quantifies the per-dispatch middleware-chain rebuild (core.dispatch reconstructs the closure chain on every call)" (lines 81–83) — is measuring a path production never takes, and the comment itself contradicts `core.go`. Anyone reading the resulting allocs/op would conclude middleware costs N allocations per message when it costs zero after Connect.
- The command-scan and parallel benchmarks are also inflated by a per-iteration chain composition.
Suggestion: take the `*testing.B`, wire the existing in-package `stubAdapter` and connect, e.g.
```go
func benchBot(b *testing.B, numCommands, numMiddleware int, matchFirst bool) (*Bot, *Message) {
b.Helper()
bot := New(CLIBotType, &stubAdapter{})
// ... registrations ...
if err := bot.Connect(context.Background()); err != nil {
b.Fatal(err)
}
b.Cleanup(func() { _ = bot.Disconnect() })
...
}
```
and rewrite the `ScaleMiddleware` comment to say it measures the *per-message* cost of traversing an N-deep composed chain. (Same note applies to `internal/core/dispatch_concurrency_test.go:24`, which also dispatches on a never-connected Bot; connecting there would exercise the atomic-chain read that production actually performs.)
--- Issue 2 ---
File: botbooter_endurance_test.go:84 (side RIGHT)
Severity: low
Issue: A malformed duration env var is silently ignored
`enduranceDuration` drops the `time.ParseDuration` error and falls back to the default, so `BOTBOOTER_SLACK_ENDURANCE_DURATION=5min` (or any typo) silently runs the 2-minute default and the operator never learns their configuration was ignored — while `make endurance`'s 15m timeout comment implies the value matters. Every other env var in this file is validated via `requireEnv`.
Suggestion: pass `t` and fail loudly:
```go
func enduranceDuration(t *testing.T, envName string, def time.Duration) time.Duration {
t.Helper()
v := os.Getenv(envName)
if v == "" {
return def
}
d, err := time.ParseDuration(v)
if err != nil {
t.Fatalf("%s=%q: %v", envName, v, err)
}
return d
}
```
--- Issue 3 ---
File: botbooter_endurance_test.go:131 (side RIGHT)
Severity: low
Issue: Receipt assertion passes vacuously when no messages were driven
`recv.Load() >= int64(sent/2)` is trivially true when `sent == 0`, which happens if the configured duration is shorter than the tick interval (or if `enduranceDuration` silently fell back after a parse error). The test would then report success having proven nothing about message flow. The same applies to the Discord user-driver branch (line 174).
Suggestion: assert the driver actually ran, e.g. add `asserts.True(t, sent > 0, "driver posted at least one message")` before the receipt check, or enforce a minimum inside `runEnduranceSmoke` (`if sent == 0 { t.Fatal("no drive calls made; duration shorter than interval?") }`).
--- Issue 4 ---
File: Makefile:37 (side RIGHT)
Severity: low
Issue: New make targets and env-gated network tests not reflected in README/CLAUDE.md
This repo keeps its make targets and env-gated tests documented in both `README.md` (Development section) and `CLAUDE.md` (Commands section). Both currently assert there is exactly one opt-in network test:
- README: *"The single test that touches the Slack network is opt-in, enabled by setting the `BOTBOOTER_SLACK_NETWORK_TEST` environment variable"*
- CLAUDE.md: *"The suite is hermetic by default. `TestConnectSlack_StartsAndStops` does real Slack network I/O and is skipped unless `BOTBOOTER_SLACK_NETWORK_TEST` is set."*
After this PR both statements are stale, and `bench` / `soak` / `endurance` plus the six new `BOTBOOTER_{SLACK,DISCORD}_*` variables are undocumented outside the Makefile comments. Suggestion: add the three targets to the command lists and extend the hermeticity note to mention the endurance smokes and their env vars.
Apply minimal, correct fixes that resolve these issues. Match the surrounding code's existing style and conventions, and do not change unrelated behavior.
Reviewed by prbooter. Comment /recheck to re-run, /security-review for a security audit, or /clean to remove my comments.
| // otherwise it matches none, so dispatch scans every command and falls through | ||
| // to the unknown-command handler (worst case for the linear scan). | ||
| func benchBot(numCommands, numMiddleware int, matchFirst bool) (*Bot, *Message) { | ||
| b := New(CLIBotType, nil) |
There was a problem hiding this comment.
[medium] Benchmarks never Connect, so they measure the compose-per-dispatch fallback, not the production path
benchBot builds the Bot with New(CLIBotType, nil) and never calls Connect, so dispatchChain is nil and every bot.dispatch call falls into the documented fallback branch in core.go:
// No connection was ever established (e.g. a unit test calling dispatch
// directly); compose on the fly from the current middleware.
b.composeDispatchChain()(ctx, b, message)In production the chain is composed once per Connect and stored atomically (Connect: chain := b.composeDispatchChain(); b.dispatchChain.Store(&chain)), so no message pays for the rebuild. Consequences:
BenchmarkDispatch_ScaleMiddleware's stated purpose — "quantifies the per-dispatch middleware-chain rebuild (core.dispatch reconstructs the closure chain on every call)" (lines 81–83) — is measuring a path production never takes, and the comment itself contradictscore.go. Anyone reading the resulting allocs/op would conclude middleware costs N allocations per message when it costs zero after Connect.- The command-scan and parallel benchmarks are also inflated by a per-iteration chain composition.
Suggestion: take the *testing.B, wire the existing in-package stubAdapter and connect, e.g.
func benchBot(b *testing.B, numCommands, numMiddleware int, matchFirst bool) (*Bot, *Message) {
b.Helper()
bot := New(CLIBotType, &stubAdapter{})
// ... registrations ...
if err := bot.Connect(context.Background()); err != nil {
b.Fatal(err)
}
b.Cleanup(func() { _ = bot.Disconnect() })
...
}and rewrite the ScaleMiddleware comment to say it measures the per-message cost of traversing an N-deep composed chain. (Same note applies to internal/core/dispatch_concurrency_test.go:24, which also dispatches on a never-connected Bot; connecting there would exercise the atomic-chain read that production actually performs.)
🤖 AI prompt to fix (review before running)
Fix an issue found during code review of lao/botbooter (PR #44).
File: internal/core/dispatch_bench_test.go:16 (side RIGHT)
Severity: medium
Issue: Benchmarks never Connect, so they measure the compose-per-dispatch fallback, not the production path
`benchBot` builds the Bot with `New(CLIBotType, nil)` and never calls `Connect`, so `dispatchChain` is nil and every `bot.dispatch` call falls into the documented fallback branch in `core.go`:
```go
// No connection was ever established (e.g. a unit test calling dispatch
// directly); compose on the fly from the current middleware.
b.composeDispatchChain()(ctx, b, message)
```
In production the chain is composed **once per Connect** and stored atomically (`Connect`: `chain := b.composeDispatchChain(); b.dispatchChain.Store(&chain)`), so no message pays for the rebuild. Consequences:
- `BenchmarkDispatch_ScaleMiddleware`'s stated purpose — "quantifies the per-dispatch middleware-chain rebuild (core.dispatch reconstructs the closure chain on every call)" (lines 81–83) — is measuring a path production never takes, and the comment itself contradicts `core.go`. Anyone reading the resulting allocs/op would conclude middleware costs N allocations per message when it costs zero after Connect.
- The command-scan and parallel benchmarks are also inflated by a per-iteration chain composition.
Suggestion: take the `*testing.B`, wire the existing in-package `stubAdapter` and connect, e.g.
```go
func benchBot(b *testing.B, numCommands, numMiddleware int, matchFirst bool) (*Bot, *Message) {
b.Helper()
bot := New(CLIBotType, &stubAdapter{})
// ... registrations ...
if err := bot.Connect(context.Background()); err != nil {
b.Fatal(err)
}
b.Cleanup(func() { _ = bot.Disconnect() })
...
}
```
and rewrite the `ScaleMiddleware` comment to say it measures the *per-message* cost of traversing an N-deep composed chain. (Same note applies to `internal/core/dispatch_concurrency_test.go:24`, which also dispatches on a never-connected Bot; connecting there would exercise the atomic-chain read that production actually performs.)
Apply a minimal, correct fix that resolves this issue. Match the surrounding code's existing style and conventions, and do not change unrelated behavior.
|
|
||
| func enduranceDuration(envName string, def time.Duration) time.Duration { | ||
| if v := os.Getenv(envName); v != "" { | ||
| if d, err := time.ParseDuration(v); err == nil { |
There was a problem hiding this comment.
[low] A malformed duration env var is silently ignored
enduranceDuration drops the time.ParseDuration error and falls back to the default, so BOTBOOTER_SLACK_ENDURANCE_DURATION=5min (or any typo) silently runs the 2-minute default and the operator never learns their configuration was ignored — while make endurance's 15m timeout comment implies the value matters. Every other env var in this file is validated via requireEnv.
Suggestion: pass t and fail loudly:
func enduranceDuration(t *testing.T, envName string, def time.Duration) time.Duration {
t.Helper()
v := os.Getenv(envName)
if v == "" {
return def
}
d, err := time.ParseDuration(v)
if err != nil {
t.Fatalf("%s=%q: %v", envName, v, err)
}
return d
}🤖 AI prompt to fix (review before running)
Fix an issue found during code review of lao/botbooter (PR #44).
File: botbooter_endurance_test.go:84 (side RIGHT)
Severity: low
Issue: A malformed duration env var is silently ignored
`enduranceDuration` drops the `time.ParseDuration` error and falls back to the default, so `BOTBOOTER_SLACK_ENDURANCE_DURATION=5min` (or any typo) silently runs the 2-minute default and the operator never learns their configuration was ignored — while `make endurance`'s 15m timeout comment implies the value matters. Every other env var in this file is validated via `requireEnv`.
Suggestion: pass `t` and fail loudly:
```go
func enduranceDuration(t *testing.T, envName string, def time.Duration) time.Duration {
t.Helper()
v := os.Getenv(envName)
if v == "" {
return def
}
d, err := time.ParseDuration(v)
if err != nil {
t.Fatalf("%s=%q: %v", envName, v, err)
}
return d
}
```
Apply a minimal, correct fix that resolves this issue. Match the surrounding code's existing style and conventions, and do not change unrelated behavior.
| }) | ||
|
|
||
| t.Logf("slack endurance: sent %d, received %d", sent, recv.Load()) | ||
| asserts.True(t, recv.Load() >= int64(sent/2), "bot kept receiving messages across the window") |
There was a problem hiding this comment.
[low] Receipt assertion passes vacuously when no messages were driven
recv.Load() >= int64(sent/2) is trivially true when sent == 0, which happens if the configured duration is shorter than the tick interval (or if enduranceDuration silently fell back after a parse error). The test would then report success having proven nothing about message flow. The same applies to the Discord user-driver branch (line 174).
Suggestion: assert the driver actually ran, e.g. add asserts.True(t, sent > 0, "driver posted at least one message") before the receipt check, or enforce a minimum inside runEnduranceSmoke (if sent == 0 { t.Fatal("no drive calls made; duration shorter than interval?") }).
🤖 AI prompt to fix (review before running)
Fix an issue found during code review of lao/botbooter (PR #44).
File: botbooter_endurance_test.go:131 (side RIGHT)
Severity: low
Issue: Receipt assertion passes vacuously when no messages were driven
`recv.Load() >= int64(sent/2)` is trivially true when `sent == 0`, which happens if the configured duration is shorter than the tick interval (or if `enduranceDuration` silently fell back after a parse error). The test would then report success having proven nothing about message flow. The same applies to the Discord user-driver branch (line 174).
Suggestion: assert the driver actually ran, e.g. add `asserts.True(t, sent > 0, "driver posted at least one message")` before the receipt check, or enforce a minimum inside `runEnduranceSmoke` (`if sent == 0 { t.Fatal("no drive calls made; duration shorter than interval?") }`).
Apply a minimal, correct fix that resolves this issue. Match the surrounding code's existing style and conventions, and do not change unrelated behavior.
|
|
||
| # Gated real-platform endurance smokes; skipped unless the BOTBOOTER_{SLACK,DISCORD}_* | ||
| # env vars are exported. Timeout must exceed the configured endurance duration. | ||
| endurance: |
There was a problem hiding this comment.
[low] New make targets and env-gated network tests not reflected in README/CLAUDE.md
This repo keeps its make targets and env-gated tests documented in both README.md (Development section) and CLAUDE.md (Commands section). Both currently assert there is exactly one opt-in network test:
- README: "The single test that touches the Slack network is opt-in, enabled by setting the
BOTBOOTER_SLACK_NETWORK_TESTenvironment variable" - CLAUDE.md: "The suite is hermetic by default.
TestConnectSlack_StartsAndStopsdoes real Slack network I/O and is skipped unlessBOTBOOTER_SLACK_NETWORK_TESTis set."
After this PR both statements are stale, and bench / soak / endurance plus the six new BOTBOOTER_{SLACK,DISCORD}_* variables are undocumented outside the Makefile comments. Suggestion: add the three targets to the command lists and extend the hermeticity note to mention the endurance smokes and their env vars.
🤖 AI prompt to fix (review before running)
Fix an issue found during code review of lao/botbooter (PR #44).
File: Makefile:37 (side RIGHT)
Severity: low
Issue: New make targets and env-gated network tests not reflected in README/CLAUDE.md
This repo keeps its make targets and env-gated tests documented in both `README.md` (Development section) and `CLAUDE.md` (Commands section). Both currently assert there is exactly one opt-in network test:
- README: *"The single test that touches the Slack network is opt-in, enabled by setting the `BOTBOOTER_SLACK_NETWORK_TEST` environment variable"*
- CLAUDE.md: *"The suite is hermetic by default. `TestConnectSlack_StartsAndStops` does real Slack network I/O and is skipped unless `BOTBOOTER_SLACK_NETWORK_TEST` is set."*
After this PR both statements are stale, and `bench` / `soak` / `endurance` plus the six new `BOTBOOTER_{SLACK,DISCORD}_*` variables are undocumented outside the Makefile comments. Suggestion: add the three targets to the command lists and extend the hermeticity note to mention the endurance smokes and their env vars.
Apply a minimal, correct fix that resolves this issue. Match the surrounding code's existing style and conventions, and do not change unrelated behavior.
core.dispatch composes the middleware chain per call only when the Bot was never connected; Connect composes it once and publishes it in an atomic.Pointer. Benchmarking an unconnected Bot therefore measured a path no production message takes, reporting allocs/op that scale with the middleware count where the real figure is zero (mw=10: 242ns and 10 allocs unconnected, 52ns and 0 allocs connected). - benchBot and TestDispatch_ConcurrentReads_RaceFree connect through stubAdapter, so both exercise the shared chain production reads - endurance: fail loudly on an unparseable *_ENDURANCE_DURATION and on zero drive calls, and require recv > 0 so the receipt check cannot pass vacuously; retire idle keep-alives before the goroutine-leak check, which otherwise counts pooled net/http readLoop/writeLoop goroutines as leaks for the whole IdleConnTimeout - loadtest: drop the unused Sends and Dispatch accessors, take asserts.TestingT rather than import testing into a non-test package, and drop the runtime.GC call that never affected goroutine lifetime - soak and endurance run with -v, since their point is the logged throughput and sent/received tallies - README and CLAUDE.md: document the bench, soak and endurance targets and the three env-gated network tests
|
/loop |
|
🔁 Review-fix loop started — target 9/10, up to 3 fix round(s). |
|
✅ No issues found.
|
There was a problem hiding this comment.
Verdict: approve · Grade: 9/10
📝 Summary of changes
This is a test-and-tooling-only PR adding load/soak/overload/endurance testing infrastructure for botbooter's dispatch path, plus a new internal/loadtest in-memory adapter, benchmarks, three make targets (bench, soak, endurance), and doc updates in CLAUDE.md/README.md.
The work is careful and well-reasoned:
- The
loadtest.Adaptercorrectly capturesdeps.DispatchatConnectand drives the real command/middleware pipeline with no network I/O; the happens-before argument for the unsynchronizeddispatchfield write (test goroutine writes beforegospawns workers) is sound. - The soak/concurrency tests deliberately
Connectthe Bot first so they exercise the publishedatomic.Pointerdispatch chain rather than the compose-on-the-fly fallback, matching the documented production path; exact hit/miss counts prove no lost/double dispatch under-race. - The overload tests use deterministic channel barriers (
release,waitForPeak) rather than wall-clock timing, and theGaugeCAS loop is a correct concurrent max tracker. - The endurance smokes are env-gated exactly like the existing
TestConnectSlack_StartsAndStops, retire idle keep-alive connections before the goroutine-leak check, and keep total runtime well under the 15m target timeout.
Doc/Makefile changes are consistent with existing conventions (internal/asserts, env-var gating, -race gate). I found no correctness, concurrency, or API-misuse issues. The timing-based AssertNoGoroutineLeak polling and waitForPeak deadline use generous windows in line with the repo's existing patterns. The endurance-test sent++ incrementing even on a failed drive is a theoretical flakiness source for the recv >= sent/2 bound, but only in the opt-in, manually-run network path, so it is not worth blocking on.
✅ No issues found.
Reviewed by prbooter. Comment /recheck to re-run, /security-review for a security audit, /auto-fix to have me attempt the fixes, or /clean to remove my comments.
|
🔁 Review-fix loop finished — target 9/10.
|
1 participant
Summary by CodeRabbit
bench,soak, andendurancecommands for running performance, stress, and endurance test suites.