loadgen: large-room bot scaling scenario (max-room-size)#275
Conversation
|
Warning Review limit reached
More reviews will be available in 44 minutes and 52 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (19)
📝 WalkthroughWalkthroughAdds a max-room-size loadgen scenario: deterministic botroom fixtures, paced bot sender and optional reader, per-step PASS/TRIP/INCONCLUSIVE verdicts, console/CSV reporting, Prometheus metrics, CLI seed/teardown and run subcommands, deploy Makefile/docker-compose updates, and unit/integration tests. ChangesLarge-room bot scaling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
tools/loadgen/botroom_integration_test.go (1)
34-34: ⚡ Quick winRoleOwner string literal matches the BSON value.
model.RoleOwneris defined asRole = "owner"andSubscription.Rolesis stored in Mongo asbson:"roles"(a[]Rolearray). So the filter{"roles": "owner"}correctly matches when"owner"is present in the roles array. Optional: usemodel.RoleOwnerinstead of the duplicated"owner"literal for maintainability.🤖 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 `@tools/loadgen/botroom_integration_test.go` at line 34, The filter uses the string literal "owner" for the roles field; replace that literal with the constant model.RoleOwner to avoid duplication and improve maintainability (the filter {"roles": "owner"} is valid for matching the Subscription.Roles array, so simply change the value to model.RoleOwner); ensure the file imports the package that defines RoleOwner if not already present.docs/superpowers/plans/2026-06-04-loadgen-large-room-bot-scaling.md (1)
1377-1384: 💤 Low valueConsider structured error logging in E1 gatekeeper handler.
The E1 subscription handler (lines 1377-1384) increments a metric when
payload.Error != ""but doesn't log the actual error message. While this is acceptable for a metrics-only handler, consider whether gatekeeper rejection details should be logged at DEBUG level for troubleshooting (the first few at least, similar to the design doc's "log first 5 verbatim" pattern on line 232).💡 Optional enhancement
If gatekeeper rejection details would help troubleshooting:
e1Sub, err := nc.NatsConn().Subscribe(subject.UserResponseWildcard(), func(msg *nats.Msg) { var payload struct { Error string `json:"error"` } if json.Unmarshal(msg.Data, &payload) == nil && payload.Error != "" { + // Log first few gatekeeper rejections for troubleshooting + if gkFailed.Load() < 5 { + slog.Debug("gatekeeper rejected send", "error", payload.Error) + } metrics.BotRoomPublishErrors.WithLabelValues("gatekeeper").Inc() gkFailed.Add(1) } })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-06-04-loadgen-large-room-bot-scaling.md` around lines 1377 - 1384, The E1 gatekeeper subscription handler (created via nc.NatsConn().Subscribe and assigned to e1Sub) increments metrics.BotRoomPublishErrors and gkFailed when payload.Error != "" but never logs the error; update the handler to log the gatekeeper rejection details (payload.Error) at DEBUG level—using the existing “log first 5 verbatim” pattern (e.g., a small counter or sampler) so only the first few rejections are logged verbatim while subsequent ones remain metric-only; ensure the log includes context (subject.UserResponseWildcard, any request identifiers) and use the existing logger used elsewhere in this file for consistency.tools/loadgen/README.md (1)
622-626: 💤 Low valueMinor: Add language identifier to fenced code block.
The quick start code block should specify
bashorshellfor syntax highlighting, consistent with other code blocks in this README.📝 Proposed fix
-``` +```bash make -C tools/loadgen/deploy up make -C tools/loadgen/deploy seed-botroom PRESET=botroom-medium make -C tools/loadgen/deploy run-max-room-size PRESET=botroom-medium RATE=200</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@tools/loadgen/README.mdaround lines 622 - 626, The fenced code block in the
README lacks a language identifier; update the three-line snippet (the block
containing the make commands: "make -C tools/loadgen/deploy up", "make -C
tools/loadgen/deploy seed-botroom PRESET=botroom-medium", "make -C
tools/loadgen/deploy run-max-room-size PRESET=botroom-medium RATE=200") to use a
bash/shell language tag by changing the openingtobash so it matches
other examples and enables syntax highlighting.</details> </blockquote></details> <details> <summary>docs/superpowers/specs/2026-06-04-loadgen-large-room-bot-scaling-design.md (2)</summary><blockquote> `262-277`: _💤 Low value_ **Minor: Add language identifier to fenced code block.** The file layout code block should specify a language (e.g., `text` or leave the language tag for plain formatting). <details> <summary>📝 Proposed fix</summary> ```diff -``` +```text tools/loadgen/ botroom.go # BotRoomPreset, size-ramp driver, step protocol ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>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/superpowers/specs/2026-06-04-loadgen-large-room-bot-scaling-design.md
around lines 262 - 277, The fenced file-layout code block in the docs (the block
showing the tools/loadgen/ tree) lacks a language tag; update that fenced block
to include a language identifier such as "text" (e.g., changetotext) so
the block is treated as plain formatted text—ensure the opening fence for the
file tree in the specs/2026-06-04-loadgen-large-room-bot-scaling-design.md
document is updated accordingly.</details> --- `77-82`: _💤 Low value_ **Minor: Add language identifier to fenced code block.** The fenced code block should specify `bash` or `shell` as the language for syntax highlighting. <details> <summary>📝 Proposed fix</summary> ```diff -``` +```bash loadgen max-room-size --preset=<name> --rate=<rps> \ [--sizes=100,500,1000,2000,5000] [--rooms-per-size=4] \ [--reads=<rps>] [--warmup=60s] [--hold=180s] [--cooldown=30s] \ [--stop-on-trip=true] [--seed=42] [--csv=<path>] ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>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/superpowers/specs/2026-06-04-loadgen-large-room-bot-scaling-design.md
around lines 77 - 82, The fenced code block showing the CLI example for "loadgen
max-room-size --preset= --rate= ..." should include a language
identifier for syntax highlighting; edit the triple-backtick fence that precedes
that command to bebash (orshell) so the block becomes a bash/shell code
block.</details> </blockquote></details> <details> <summary>tools/loadgen/botroom_verdict.go (1)</summary><blockquote> `137-147`: _💤 Low value_ **Percentile calculation is clear and correct.** The simple linear interpolation approach is appropriate for loadgen verdict evaluation. <details> <summary>Optional: Add comment for clarity</summary> ```diff // percentileMs returns the q-quantile of samples (already in ms). Empty → 0. +// Uses nearest-rank (simple linear interpolation) for speed. func percentileMs(samples []float64, q float64) float64 { ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/loadgen/botroom_verdict.go` around lines 137 - 147, The percentileMs function is correct but would benefit from an inline comment explaining the chosen quantile method; update the percentileMs function to document that it returns the q-quantile using a nearest-rank style approach (empty → 0), that samples are copied and sorted into sorted, and that the index computation idx := int(float64(len(sorted)-1) * q) picks the nearest element (no linear interpolation). ``` </details> </blockquote></details> <details> <summary>tools/loadgen/botroom.go (1)</summary><blockquote> `223-223`: **Clarify `time.Sleep` usage in `tools/loadgen/botroom.go` (runBotRoomStep)** `time.Sleep(time.Second)` at `tools/loadgen/botroom.go:223` is used as an explicit “drain/settle trailing broadcasts” window before snapshotting metrics, not for goroutine synchronization (sender/reader goroutine lifecycle is controlled by `cancel()` + `wg.Wait()`). The same loadgen pattern exists elsewhere in `tools/loadgen/` (e.g., “drain trailing replies/broadcasts” sleeps). Optional: replace the fixed sleep with a more explicit stabilization signal (e.g., wait until pending/collector state converges) to reduce timing flakiness. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/loadgen/botroom.go` at line 223, The one-second sleep in runBotRoomStep (time.Sleep(time.Second)) is intended as a short "drain/settle trailing broadcasts" window before snapshotting metrics, not for goroutine synchronization (those use cancel() + wg.Wait()); update tools/loadgen/botroom.go by replacing the bare sleep with an explicit comment explaining this purpose and either (a) keep the sleep but document it clearly near runBotRoomStep, or (b) implement a stabilization wait that polls the pending/broadcast/collector state until it converges with a small timeout (fall back to a short sleep) — reference runBotRoomStep, cancel(), and wg.Wait() when making the change so future readers know why goroutine lifecycle isn't controlled by this sleep. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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/superpowers/plans/2026-06-04-loadgen-large-room-bot-scaling.md:
- Around line 1567-1572: The stub newBotRoomReader has an invalid parameter list
with two consecutive unnamed string parameters; update its signature to give
each parameter a name (or a distinct blank identifier) so the parameter types
are separated correctly—e.g., ensure newBotRoomReader declares names for the two
string parameters and for the other args (or use named blanks like conn
*nats.Conn, a string, b string, ids []string, n int, sender *BotRoomSender) so
the compiler sees valid parameter declarations and the rest of the stub methods
(botRoomReader.Run and SamplesMs) remain unchanged.In
@tools/loadgen/botroom_integration_test.go:
- Around line 1-37: Add a TestMain to this integration test file that invokes
testutil.RunTests(m) so testcontainers and other resources are cleaned up at
process exit; specifically, in the same file containing TestBotRoom_SeedShape
add a TestMain(m *testing.M) that calls testutil.RunTests(m) (no additional
imports needed since testing and testutil are already used).In
@tools/loadgen/botroom.go:
- Around line 346-350: The handler currently increments BotRoomPublishErrors on
read failures (rerr) which is wrong; change it to increment a dedicated read
metric (e.g., Metrics.BotRoomReadErrors) and label by the actual failure kind
instead of always "timeout". In the rerr != nil branch (same place referencing
rerr, ctx, and r.sender.cfg.Metrics.BotRoomPublishErrors) compute an errorKind
string by inspecting ctx.Err() and rerr (e.g., "timeout" when
context.DeadlineExceeded, "cancelled" when context.Canceled, or a generic
"nats_error"/rerr type otherwise), skip counting only when ctx.Err() ==
context.Canceled if you intentionally want to ignore user cancellations, and
call BotRoomReadErrors.WithLabelValues(errorKind).Inc() instead of using
BotRoomPublishErrors.WithLabelValues("timeout"). Ensure you use the
Metrics.BotRoomReadErrors symbol (or add it if missing) so read errors are
tracked separately from publish metrics.
Nitpick comments:
In@docs/superpowers/plans/2026-06-04-loadgen-large-room-bot-scaling.md:
- Around line 1377-1384: The E1 gatekeeper subscription handler (created via
nc.NatsConn().Subscribe and assigned to e1Sub) increments
metrics.BotRoomPublishErrors and gkFailed when payload.Error != "" but never
logs the error; update the handler to log the gatekeeper rejection details
(payload.Error) at DEBUG level—using the existing “log first 5 verbatim” pattern
(e.g., a small counter or sampler) so only the first few rejections are logged
verbatim while subsequent ones remain metric-only; ensure the log includes
context (subject.UserResponseWildcard, any request identifiers) and use the
existing logger used elsewhere in this file for consistency.In
@docs/superpowers/specs/2026-06-04-loadgen-large-room-bot-scaling-design.md:
- Around line 262-277: The fenced file-layout code block in the docs (the block
showing the tools/loadgen/ tree) lacks a language tag; update that fenced block
to include a language identifier such as "text" (e.g., changetotext) so
the block is treated as plain formatted text—ensure the opening fence for the
file tree in the specs/2026-06-04-loadgen-large-room-bot-scaling-design.md
document is updated accordingly.- Around line 77-82: The fenced code block showing the CLI example for "loadgen
max-room-size --preset= --rate= ..." should include a language
identifier for syntax highlighting; edit the triple-backtick fence that precedes
that command to bebash (orshell) so the block becomes a bash/shell code
block.In
@tools/loadgen/botroom_integration_test.go:
- Line 34: The filter uses the string literal "owner" for the roles field;
replace that literal with the constant model.RoleOwner to avoid duplication and
improve maintainability (the filter {"roles": "owner"} is valid for matching the
Subscription.Roles array, so simply change the value to model.RoleOwner); ensure
the file imports the package that defines RoleOwner if not already present.In
@tools/loadgen/botroom_verdict.go:
- Around line 137-147: The percentileMs function is correct but would benefit
from an inline comment explaining the chosen quantile method; update the
percentileMs function to document that it returns the q-quantile using a
nearest-rank style approach (empty → 0), that samples are copied and sorted into
sorted, and that the index computation idx := int(float64(len(sorted)-1) * q)
picks the nearest element (no linear interpolation).In
@tools/loadgen/botroom.go:
- Line 223: The one-second sleep in runBotRoomStep (time.Sleep(time.Second)) is
intended as a short "drain/settle trailing broadcasts" window before
snapshotting metrics, not for goroutine synchronization (those use cancel() +
wg.Wait()); update tools/loadgen/botroom.go by replacing the bare sleep with an
explicit comment explaining this purpose and either (a) keep the sleep but
document it clearly near runBotRoomStep, or (b) implement a stabilization wait
that polls the pending/broadcast/collector state until it converges with a small
timeout (fall back to a short sleep) — reference runBotRoomStep, cancel(), and
wg.Wait() when making the change so future readers know why goroutine lifecycle
isn't controlled by this sleep.In
@tools/loadgen/README.md:
- Around line 622-626: The fenced code block in the README lacks a language
identifier; update the three-line snippet (the block containing the make
commands: "make -C tools/loadgen/deploy up", "make -C tools/loadgen/deploy
seed-botroom PRESET=botroom-medium", "make -C tools/loadgen/deploy
run-max-room-size PRESET=botroom-medium RATE=200") to use a bash/shell language
tag by changing the openingtobash so it matches other examples and
enables syntax highlighting.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `e02c454f-2dc8-4a5a-a6e6-c8a1fcdefd7e` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between de844af684d7cb8c4ee7e429de3c33e35aac894d and 6bed4ac54974b50086b812bd83b1fade2303750d. </details> <details> <summary>📒 Files selected for processing (19)</summary> * `docs/superpowers/plans/2026-06-04-loadgen-large-room-bot-scaling.md` * `docs/superpowers/specs/2026-06-04-loadgen-large-room-bot-scaling-design.md` * `tools/loadgen/README.md` * `tools/loadgen/botroom.go` * `tools/loadgen/botroom_integration_test.go` * `tools/loadgen/botroom_report.go` * `tools/loadgen/botroom_report_test.go` * `tools/loadgen/botroom_test.go` * `tools/loadgen/botroom_verdict.go` * `tools/loadgen/botroom_verdict_test.go` * `tools/loadgen/deploy/Makefile` * `tools/loadgen/deploy/docker-compose.yml` * `tools/loadgen/main.go` * `tools/loadgen/members.go` * `tools/loadgen/members_test.go` * `tools/loadgen/metrics.go` * `tools/loadgen/metrics_test.go` * `tools/loadgen/seed_botroom.go` * `tools/loadgen/seed_botroom_test.go` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| //go:build integration | ||
|
|
||
| package main | ||
|
|
||
| import ( | ||
| "context" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
|
|
||
| "github.com/hmchangw/chat/pkg/testutil" | ||
| ) | ||
|
|
||
| func TestBotRoom_SeedShape(t *testing.T) { | ||
| ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) | ||
| defer cancel() | ||
|
|
||
| db := testutil.MongoDB(t, "botroom") | ||
|
|
||
| p, _ := BuiltinBotRoomPreset("botroom-small") | ||
| fixtures, layout := BuildBotRoomFixtures(&p, 1, "site-local") | ||
| require.NoError(t, Seed(ctx, db, &fixtures)) | ||
|
|
||
| // Each size-50 room must have exactly 50 subscriptions in Mongo. | ||
| count, err := db.Collection("subscriptions").CountDocuments(ctx, | ||
| map[string]any{"roomId": layout.RoomsBySize[50][0]}) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, int64(50), count) | ||
|
|
||
| // The bot must be the owner of that room. | ||
| ownerCount, err := db.Collection("subscriptions").CountDocuments(ctx, | ||
| map[string]any{"roomId": layout.RoomsBySize[50][0], "roles": "owner", "u.account": layout.BotAccount}) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, int64(1), ownerCount) | ||
| } |
There was a problem hiding this comment.
Missing required TestMain for integration test cleanup.
Per coding guidelines, all *_integration_test.go files must include a TestMain that calls testutil.RunTests(m) to drive cleanup at process exit. Without it, testcontainers won't be properly terminated.
🔧 Proposed fix
Add this function to the file:
func TestMain(m *testing.M) {
testutil.RunTests(m)
}As per coding guidelines: "Always include a TestMain that calls testutil.RunTests(m) to drive cleanup at process exit."
🤖 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 `@tools/loadgen/botroom_integration_test.go` around lines 1 - 37, Add a
TestMain to this integration test file that invokes testutil.RunTests(m) so
testcontainers and other resources are cleaned up at process exit; specifically,
in the same file containing TestBotRoom_SeedShape add a TestMain(m *testing.M)
that calls testutil.RunTests(m) (no additional imports needed since testing and
testutil are already used).
| if rerr != nil { | ||
| if ctx.Err() == nil { | ||
| r.sender.cfg.Metrics.BotRoomPublishErrors.WithLabelValues("timeout").Inc() | ||
| } | ||
| return |
There was a problem hiding this comment.
Incorrect metric name and label for read operation errors.
The code increments BotRoomPublishErrors.WithLabelValues("timeout") for read request failures. This is incorrect because:
- The metric name is
BotRoomPublishErrors, but this is a read operation, not a publish. - The label
"timeout"assumes the error is a timeout, butrerrcould be a connection error, deserialization error, or other NATS failure. - The condition
ctx.Err() == nilfilters out step-cancelled errors, but this still conflates timeouts with other failures.
Consider either:
- Using a separate
BotRoomReadErrorsmetric, or - Changing the label to distinguish read vs. publish errors and using a more accurate label (e.g.,
"read_failed"or inspectrerrtype).
🔧 Suggested fix: use a dedicated read-error label or metric
If metrics.go already has a BotRoomReadErrors metric:
if rerr != nil {
if ctx.Err() == nil {
- r.sender.cfg.Metrics.BotRoomPublishErrors.WithLabelValues("timeout").Inc()
+ r.sender.cfg.Metrics.BotRoomReadErrors.WithLabelValues("failed").Inc()
}
return
}Alternatively, if reusing BotRoomPublishErrors, use a distinct label:
- r.sender.cfg.Metrics.BotRoomPublishErrors.WithLabelValues("timeout").Inc()
+ r.sender.cfg.Metrics.BotRoomPublishErrors.WithLabelValues("read_failed").Inc()🤖 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 `@tools/loadgen/botroom.go` around lines 346 - 350, The handler currently
increments BotRoomPublishErrors on read failures (rerr) which is wrong; change
it to increment a dedicated read metric (e.g., Metrics.BotRoomReadErrors) and
label by the actual failure kind instead of always "timeout". In the rerr != nil
branch (same place referencing rerr, ctx, and
r.sender.cfg.Metrics.BotRoomPublishErrors) compute an errorKind string by
inspecting ctx.Err() and rerr (e.g., "timeout" when context.DeadlineExceeded,
"cancelled" when context.Canceled, or a generic "nats_error"/rerr type
otherwise), skip counting only when ctx.Err() == context.Canceled if you
intentionally want to ignore user cancellations, and call
BotRoomReadErrors.WithLabelValues(errorKind).Inc() instead of using
BotRoomPublishErrors.WithLabelValues("timeout"). Ensure you use the
Metrics.BotRoomReadErrors symbol (or add it if missing) so read errors are
tracked separately from publish metrics.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tools/loadgen/botroom.go (2)
226-260:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop generating traffic before sampling the measured latencies.
Lines 228-257 snapshot
a1/f1andendPendingat hold end, butsender.Runandreader.Runkeep producing new work until Line 260. That means the 1s settle plusLatencySamples()/SamplesMs()include post-hold traffic, while rate/error/pending are computed from the earlier cutoff.Suggested fix
- endPending, _ := pollPending(ctx, c.JSZURL) - a1, f1 := sender.Attempted(), sender.Failed()+c.GKFailed.Load() - - // Settle briefly so trailing broadcasts land before the snapshot. - // Intentional: mirrors the existing runRun/runMembers* 2s drain pattern. - time.Sleep(time.Second) + endPending, _ := pollPending(ctx, c.JSZURL) + a1, f1 := sender.Attempted(), sender.Failed()+c.GKFailed.Load() + cancel() // freeze the measured window before collecting trailing samples + wg.Wait() + + // Settle briefly so in-flight broadcasts land before the snapshot. + time.Sleep(time.Second) @@ - cancel() // stop sender/reader before cooldown - wg.Wait() _ = waitOrCancel(ctx, c.Cooldown)🤖 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 `@tools/loadgen/botroom.go` around lines 226 - 260, The latency/reading samples are taken after the sender/reader may still be producing traffic; move the cancel() call so traffic is stopped before sampling: call cancel() immediately after the time.Sleep and before collecting a1/f1, c.Collector.LatencySamples(), reader.SamplesMs(), snapshotSelfMetrics(), and diffPending(startPending,endPending); then compute Attempted/Failed and read pending (a1/f1, endPending) and observe E2Samples/ReadSamples from the stopped collector/reader to ensure samples reflect only the measured hold period (references: cancel(), sender, reader, c.Collector.LatencySamples(), reader.SamplesMs(), snapshotSelfMetrics(), diffPending).
223-248:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't let pending-poll failures silently bypass the backlog gate.
Lines 224 and 228 discard
pollPendingerrors, and Lines 246-248 only populateConsumerPendingwhen both polls succeed. In that failure modeevaluateBotRoomStepskips the durable backlog checks entirely, so a step can PASS without its backlog gate ever running.🤖 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 `@tools/loadgen/botroom.go` around lines 223 - 248, pollPending errors are being discarded causing evaluateBotRoomStep to skip the durable backlog gate when startPending or endPending fails; update the call sites that set startPending and endPending (calls to pollPending) to capture and check their returned errors, and if either poll fails either (a) set botRoomStepInputs.ConsumerPending to a sentinel value (e.g. -1) or populate it with best-effort data (use diffPending when one snapshot exists) and add a logged error, or (b) return/mark the step as failed so evaluateBotRoomStep still enforces the backlog gate; ensure code around startPending, endPending, botRoomStepInputs.ConsumerPending, and evaluateBotRoomStep handles the sentinel/error case instead of silently omitting the backlog check.
🤖 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.
Outside diff comments:
In `@tools/loadgen/botroom.go`:
- Around line 226-260: The latency/reading samples are taken after the
sender/reader may still be producing traffic; move the cancel() call so traffic
is stopped before sampling: call cancel() immediately after the time.Sleep and
before collecting a1/f1, c.Collector.LatencySamples(), reader.SamplesMs(),
snapshotSelfMetrics(), and diffPending(startPending,endPending); then compute
Attempted/Failed and read pending (a1/f1, endPending) and observe
E2Samples/ReadSamples from the stopped collector/reader to ensure samples
reflect only the measured hold period (references: cancel(), sender, reader,
c.Collector.LatencySamples(), reader.SamplesMs(), snapshotSelfMetrics(),
diffPending).
- Around line 223-248: pollPending errors are being discarded causing
evaluateBotRoomStep to skip the durable backlog gate when startPending or
endPending fails; update the call sites that set startPending and endPending
(calls to pollPending) to capture and check their returned errors, and if either
poll fails either (a) set botRoomStepInputs.ConsumerPending to a sentinel value
(e.g. -1) or populate it with best-effort data (use diffPending when one
snapshot exists) and add a logged error, or (b) return/mark the step as failed
so evaluateBotRoomStep still enforces the backlog gate; ensure code around
startPending, endPending, botRoomStepInputs.ConsumerPending, and
evaluateBotRoomStep handles the sentinel/error case instead of silently omitting
the backlog check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 739c3d87-7d8d-4238-8063-d170f6c7ec1b
📒 Files selected for processing (3)
tools/loadgen/botroom.gotools/loadgen/botroom_verdict.gotools/loadgen/main.go
hmchangw
force-pushed
the
claude/loadgen-room-member-scaling-QBDNA
branch
from
da8f182 to
45f3175
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
tools/loadgen/deploy/Makefile (1)
116-127: 💤 Low valueTarget body is readable but exceeds style guideline.
The
run-max-room-sizetarget body (10 lines) exceeds the checkmake recommended maximum of 5 lines. While the target is functionally correct and follows the established patterns in this Makefile (e.g.,run-dailyat 6 lines,run-max-rpsat 7 lines), consider extracting common validation logic or argument construction if the target grows further.The current form is acceptable given the number of required validations (PRESET, RATE) and configurable parameters (SIZES, ROOMS_PER_SIZE, READS, HOLD).
🤖 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 `@tools/loadgen/deploy/Makefile` around lines 116 - 127, The run-max-room-size target body is too long; extract the validation/argument construction into a shared make variable or helper target (e.g., MAX_ROOM_ARGS or a helper target invoked by run-max-room-size) so PRESET and RATE checks and optional flags (SIZES, ROOMS_PER_SIZE, READS, HOLD) are built separately, then collapse the run-max-room-size recipe to a single COMPOSE exec invocation using that variable; update references to PRESET, RATE, SIZES, ROOMS_PER_SIZE, READS, HOLD and COMPOSE in the new helper so the final run-max-room-size target body is under the style limit.
🤖 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/superpowers/plans/2026-06-04-loadgen-large-room-bot-scaling.md`:
- Around line 1954-1958: The fenced code block containing the three make
commands (make -C tools/loadgen/deploy up, make -C tools/loadgen/deploy
seed-botroom PRESET=botroom-medium, make -C tools/loadgen/deploy
run-max-room-size PRESET=botroom-medium RATE=200) should include a language tag
to satisfy markdownlint MD040; update the triple-backtick fence from ``` to
```bash so the snippet is marked as bash shell commands.
In `@docs/superpowers/specs/2026-06-04-loadgen-large-room-bot-scaling-design.md`:
- Around line 77-82: Add missing language identifiers to the two fenced code
blocks shown in the spec so markdownlint MD040 stops failing: change the first
backtick block containing the CLI example to use ```bash and change the second
backtick block that lists tools/loadgen files to use ```text (also update the
other occurrence referenced at lines 262-277 similarly). Ensure the opening
fence includes the language token and do not alter the block contents.
In `@tools/loadgen/botroom_report.go`:
- Line 65: Add a brief explanatory comment above the defer that ignores the
Close error in botroom_report.go so it's clear why the error is intentionally
discarded; reference the defer func() { _ = f.Close() }() and the file handle
variable f, and state that Close errors are ignored because file writes/flush
are already handled (or the program is exiting/cleanup where Close failures are
non-actionable), satisfying the guideline "never ignore errors silently."
- Around line 52-58: The current worstBotRoomPending calls worstDurable and
always formats wdelta with a leading "+" which yields confusing strings like
"+0" or "+-50" when deltas are non-positive; update worstBotRoomPending to check
wdelta and if wdelta <= 0 return "-" (or keep the existing "-" for empty wd),
otherwise format and return fmt.Sprintf("%s +%d", wd, wdelta); reference the
worstBotRoomPending function and the wd/wdelta variables returned from
worstDurable to locate and implement the guard.
In `@tools/loadgen/README.md`:
- Around line 622-626: Update the fenced code block containing the quick-start
commands to use a language tag so markdownlint MD040 passes: replace the opening
triple backticks (```) with a bash-tagged fence (```bash) surrounding the three
make commands; ensure the closing fence stays as ``` and that the commands
themselves (the make lines) remain unchanged.
---
Nitpick comments:
In `@tools/loadgen/deploy/Makefile`:
- Around line 116-127: The run-max-room-size target body is too long; extract
the validation/argument construction into a shared make variable or helper
target (e.g., MAX_ROOM_ARGS or a helper target invoked by run-max-room-size) so
PRESET and RATE checks and optional flags (SIZES, ROOMS_PER_SIZE, READS, HOLD)
are built separately, then collapse the run-max-room-size recipe to a single
COMPOSE exec invocation using that variable; update references to PRESET, RATE,
SIZES, ROOMS_PER_SIZE, READS, HOLD and COMPOSE in the new helper so the final
run-max-room-size target body is under the style limit.
🪄 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
Run ID: 8b367518-7603-48db-9f17-8ed2308379a1
📒 Files selected for processing (19)
docs/superpowers/plans/2026-06-04-loadgen-large-room-bot-scaling.mddocs/superpowers/specs/2026-06-04-loadgen-large-room-bot-scaling-design.mdtools/loadgen/README.mdtools/loadgen/botroom.gotools/loadgen/botroom_integration_test.gotools/loadgen/botroom_report.gotools/loadgen/botroom_report_test.gotools/loadgen/botroom_test.gotools/loadgen/botroom_verdict.gotools/loadgen/botroom_verdict_test.gotools/loadgen/deploy/Makefiletools/loadgen/deploy/docker-compose.ymltools/loadgen/main.gotools/loadgen/members.gotools/loadgen/members_test.gotools/loadgen/metrics.gotools/loadgen/metrics_test.gotools/loadgen/seed_botroom.gotools/loadgen/seed_botroom_test.go
🚧 Files skipped from review as they are similar to previous changes (13)
- tools/loadgen/members.go
- tools/loadgen/botroom_integration_test.go
- tools/loadgen/botroom_verdict_test.go
- tools/loadgen/metrics_test.go
- tools/loadgen/botroom_report_test.go
- tools/loadgen/deploy/docker-compose.yml
- tools/loadgen/metrics.go
- tools/loadgen/seed_botroom_test.go
- tools/loadgen/botroom_verdict.go
- tools/loadgen/seed_botroom.go
- tools/loadgen/botroom_test.go
- tools/loadgen/main.go
- tools/loadgen/botroom.go
| ``` | ||
| make -C tools/loadgen/deploy up | ||
| make -C tools/loadgen/deploy seed-botroom PRESET=botroom-medium | ||
| make -C tools/loadgen/deploy run-max-room-size PRESET=botroom-medium RATE=200 | ||
| ``` |
There was a problem hiding this comment.
Specify the fenced block language in the quick-start snippet.
This fence should include a language tag (likely bash) to satisfy markdownlint MD040.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 1958-1958: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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/superpowers/plans/2026-06-04-loadgen-large-room-bot-scaling.md` around
lines 1954 - 1958, The fenced code block containing the three make commands
(make -C tools/loadgen/deploy up, make -C tools/loadgen/deploy seed-botroom
PRESET=botroom-medium, make -C tools/loadgen/deploy run-max-room-size
PRESET=botroom-medium RATE=200) should include a language tag to satisfy
markdownlint MD040; update the triple-backtick fence from ``` to ```bash so the
snippet is marked as bash shell commands.
| ``` | ||
| loadgen max-room-size --preset=<name> --rate=<rps> \ | ||
| [--sizes=100,500,1000,2000,5000] [--rooms-per-size=4] \ | ||
| [--reads=<rps>] [--warmup=60s] [--hold=180s] [--cooldown=30s] \ | ||
| [--stop-on-trip=true] [--seed=42] [--csv=<path>] | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks.
Both code fences are missing a language tag (e.g., bash/text), which will keep triggering markdownlint MD040 and can break docs lint gates.
Suggested doc fix
-```
+```bash
loadgen max-room-size --preset=<name> --rate=<rps> \
[--sizes=100,500,1000,2000,5000] [--rooms-per-size=4] \
[--reads=<rps>] [--warmup=60s] [--hold=180s] [--cooldown=30s] \
[--stop-on-trip=true] [--seed=42] [--csv=<path>]- +text
tools/loadgen/
botroom.go # BotRoomPreset, size-ramp driver, step protocol
...
Also applies to: 262-277
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 77-77: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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/superpowers/specs/2026-06-04-loadgen-large-room-bot-scaling-design.md`
around lines 77 - 82, Add missing language identifiers to the two fenced code
blocks shown in the spec so markdownlint MD040 stops failing: change the first
backtick block containing the CLI example to use ```bash and change the second
backtick block that lists tools/loadgen files to use ```text (also update the
other occurrence referenced at lines 262-277 similarly). Ensure the opening
fence includes the language token and do not alter the block contents.
| func worstBotRoomPending(m map[string]ConsumerPendingDelta) string { | ||
| wd, wdelta := worstDurable(m) | ||
| if wd == "" { | ||
| return "-" | ||
| } | ||
| return fmt.Sprintf("%s +%d", wd, wdelta) | ||
| } |
There was a problem hiding this comment.
Guard against non-positive delta to avoid confusing formatting.
Line 57 formats wdelta with a hardcoded + prefix, assuming the delta is positive. However, worstDurable returns the max delta even if all deltas are ≤ 0 (e.g., when all consumer backlogs are shrinking). This edge case would produce output like "consumer +0" or "consumer +-50", which is confusing.
🛡️ Proposed fix to return "-" for non-positive deltas
func worstBotRoomPending(m map[string]ConsumerPendingDelta) string {
wd, wdelta := worstDurable(m)
- if wd == "" {
+ if wd == "" || wdelta <= 0 {
return "-"
}
return fmt.Sprintf("%s +%d", wd, wdelta)
}🤖 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 `@tools/loadgen/botroom_report.go` around lines 52 - 58, The current
worstBotRoomPending calls worstDurable and always formats wdelta with a leading
"+" which yields confusing strings like "+0" or "+-50" when deltas are
non-positive; update worstBotRoomPending to check wdelta and if wdelta <= 0
return "-" (or keep the existing "-" for empty wd), otherwise format and return
fmt.Sprintf("%s +%d", wd, wdelta); reference the worstBotRoomPending function
and the wd/wdelta variables returned from worstDurable to locate and implement
the guard.
| if err != nil { | ||
| return fmt.Errorf("create csv: %w", err) | ||
| } | ||
| defer func() { _ = f.Close() }() |
There was a problem hiding this comment.
Add a comment explaining why the Close error is ignored.
Line 65 ignores the Close() error without explanation. As per coding guidelines, "never ignore errors silently without a comment." While this is a common Go cleanup pattern and the risk is low (close rarely fails if write/flush succeeded), adding a brief comment improves clarity and compliance.
📝 Suggested comment
- defer func() { _ = f.Close() }()
+ defer func() { _ = f.Close() }() // Close error ignored; write/flush errors already handled📝 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.
| defer func() { _ = f.Close() }() | |
| defer func() { _ = f.Close() }() // Close error ignored; write/flush errors already handled |
🤖 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 `@tools/loadgen/botroom_report.go` at line 65, Add a brief explanatory comment
above the defer that ignores the Close error in botroom_report.go so it's clear
why the error is intentionally discarded; reference the defer func() { _ =
f.Close() }() and the file handle variable f, and state that Close errors are
ignored because file writes/flush are already handled (or the program is
exiting/cleanup where Close failures are non-actionable), satisfying the
guideline "never ignore errors silently."
hmchangw
force-pushed
the
claude/loadgen-room-member-scaling-QBDNA
branch
from
45f3175 to
2d7155d
Compare
3 participants
Summary
Adds a
max-room-sizeload-test scenario totools/loadgenfor the question: "with a bot sending at a fixed RPS, what's the largest room it can blast before a real signal breaks — and what breaks first?"Motivated by a production pattern where bots frequently send messages into rooms with hundreds–thousands of members. The ramp variable is room size; the bot send rate is an operator flag.
What it does
botroomworkload: seeds large channel rooms (up to ~5000 members) directly into Mongo (bypassingMAX_ROOM_SIZEfor creation), with a bot seeded as room owner so it clears the gatekeeper's large-room post restriction.max-room-sizesubcommand: ramps over--sizes, holds at each size while a paced bot sender publishes--ratemsgs/sec round-robin across--rooms-per-sizerooms, polls JetStream consumer backlog at hold start/end, and reports the largest size that held SLO plus the first signal that tripped.RoomEvent.LastMsgID), error rate, and loadgen self-saturation (GC/rate-shortfall → INCONCLUSIVE) round out the verdict.--rooms-per-sizeknob (default 4):=1concentrates load to probe the Cassandra hot-partition / Mongo hot-doc limit; the default spreads it to measure aggregate fan-out + cache churn.--readsdriver issuesmessage.readRPCs to exercise room-service's O(N) read-floor recompute on large rooms (off by default).members-capacity-xlpreset +MAX_ROOM_SIZE=6000in the loadgen deploy so the existing members add-path workload can grow rooms past the old 1000 cap.Design + plan:
docs/superpowers/specs/2026-06-04-loadgen-large-room-bot-scaling-design.md,docs/superpowers/plans/2026-06-04-loadgen-large-room-bot-scaling.md.Deferred to v2 (documented): the create-and-blast pattern (cold-cache) and a live N-connection pool to measure NATS core delivery fan-out.
Test Plan
make test SERVICE=tools/loadgen— full unit suite passes with-race(~207s)make lint— 0 issuesmake fmt— cleango vet -tags=integration ./tools/loadgen/— integration test compiles (not executed here: Docker unavailable)make -C tools/loadgen/deploy up && make -C tools/loadgen/deploy seed-botroom PRESET=botroom-medium && make -C tools/loadgen/deploy run-max-room-size PRESET=botroom-medium RATE=200https://claude.ai/code/session_01S1ad2YqGAEFoJ2TQfixXAe
Generated by Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Chores