feat: optional bounded concurrency for batch channel testing - #6004
feat: optional bounded concurrency for batch channel testing#6004opastorello wants to merge 5 commits into
Conversation
Batch channel testing ran every channel strictly sequentially, so a run over many channels waited on each slow/dead upstream one after another. Add MonitorSetting.ChannelTestConcurrency (json test_concurrency, default 1) plus an optional CHANNEL_TEST_CONCURRENCY env override. performChannelTests now dispatches tests through a bounded worker pool of that size: the shared summary is mutex guarded, progress uses an atomic counter, ctx cancellation is honored while acquiring a slot, and RequestInterval still throttles dispatch. Default 1 preserves the original sequential behavior.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a ChangesConcurrent Channel Testing
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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
🤖 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 `@controller/channel-test.go`:
- Around line 988-990: The progress callback invocation in testOne is now
concurrent, which can make report calls arrive out of order and regress
downstream progress updates. Serialize all report(...) calls by introducing a
shared synchronization point around the callback path in the
controller/channel-test.go flow, and apply the same protection to the other
report usage near the second mentioned location so every callback is emitted
sequentially with monotonically increasing processed counts.
In `@setting/operation_setting/monitor_setting_test.go`:
- Around line 45-56: Test
GetMonitorSetting_ChannelTestConcurrencyNormalizedToAtLeastOne currently depends
on CHANNEL_TEST_CONCURRENCY from the environment, so make the fixture fully
explicit by setting or clearing that env var inside the test before calling
GetMonitorSetting(). Keep the existing monitorSetting override in place, but
ensure the test controls the env-backed input as well so CI or local settings
cannot change the result. Use the GetMonitorSetting function and the
monitorSetting test fixture as the main points to update.
In `@setting/operation_setting/monitor_setting.go`:
- Around line 55-62: The CHANNEL_TEST_CONCURRENCY override handling in
monitorSetting should normalize any non-positive env value to 1 instead of
skipping the assignment and leaving an older positive value intact. Update the
logic around os.LookupEnv and strconv.Atoi so that when the env var is present
and parses successfully, monitorSetting.ChannelTestConcurrency is set from the
parsed value and then clamped to at least 1. Keep the fallback normalization in
the monitor setting initialization path so the final value is always valid.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7a6079f3-1a06-4ec0-bd23-f11f2ba5abc1
📒 Files selected for processing (3)
controller/channel-test.gosetting/operation_setting/monitor_setting.gosetting/operation_setting/monitor_setting_test.go
…ency env Address review feedback: - Serialize the progress report callback: concurrent testOne goroutines could call report out of order. Route all reports through advanceProgress, guarded by a dedicated mutex and a monotonic counter, so persisted progress never regresses. - Honor a non-positive CHANNEL_TEST_CONCURRENCY override and let the clamp normalize it to 1 instead of silently ignoring it. - Isolate the normalization test from ambient CHANNEL_TEST_CONCURRENCY and add a case covering the env=0 -> 1 normalization.
|
Thanks for the review 🙏 Addressed all three in the latest commit:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
controller/channel-test.go (1)
1004-1046: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueOptional: skip the dispatch throttle after the final channel.
The
RequestIntervalthrottle at Lines 1035-1045 runs after every dispatched channel, including the last one, adding an unnecessaryRequestIntervaldelay beforewg.Wait(). Additionally, on cancellation thetime.Aftertimer at Line 1042 leaks until it fires. Both are minor, but you can avoid the trailing wait by only throttling between dispatches.The core semaphore/wait-group coordination and cancellation handling look correct.
🤖 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 `@controller/channel-test.go` around lines 1004 - 1046, The dispatch loop in channel-test.go applies the RequestInterval throttle after the final channel and also leaves a time.After timer pending on cancellation. Update the dispatch logic in the dispatch loop around testOne, sem, wg, and RequestInterval so throttling only happens between channels (not after the last dispatch), and avoid using a timer that can linger on ctx.Done() by restructuring the sleep/cancel handling accordingly.
🤖 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.
Nitpick comments:
In `@controller/channel-test.go`:
- Around line 1004-1046: The dispatch loop in channel-test.go applies the
RequestInterval throttle after the final channel and also leaves a time.After
timer pending on cancellation. Update the dispatch logic in the dispatch loop
around testOne, sem, wg, and RequestInterval so throttling only happens between
channels (not after the last dispatch), and avoid using a timer that can linger
on ctx.Done() by restructuring the sleep/cancel handling accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 703dc17d-bf1e-4520-aafc-aa3e11c85d85
📒 Files selected for processing (3)
controller/channel-test.gosetting/operation_setting/monitor_setting.gosetting/operation_setting/monitor_setting_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- setting/operation_setting/monitor_setting.go
Move the RequestInterval throttle to the top of the dispatch loop, gated on a dispatched flag, so it runs strictly between channels and not after the final one. Use a stoppable time.NewTimer instead of time.After so the timer does not linger when ctx is cancelled during the throttle.
|
Applied the nitpick as well: the |
…tings Add a 'Test concurrency' number field to the default-theme monitor settings so the new monitor_setting.test_concurrency backend option is configurable from the admin UI (mirrors the existing test-interval field). Includes en/zh i18n.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web/default/src/features/system-settings/models/routing-reliability-section.tsx (1)
78-89: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider capping
test_concurrencyto a sane maximum.Unlike
RetryTimes(.max(10)),test_concurrencyonly has a lower bound (.min(1)). Per the PR objectives, the backend normalizes non-positive values to 1 but doesn't appear to clamp an upper bound either, so an admin could set an arbitrarily large value that's forwarded directly to the channel-test worker pool, risking resource exhaustion during batch tests.♻️ Suggested bound
test_concurrency: z.coerce .number() .int() - .min(1, 'Concurrency must be at least 1'), + .min(1, 'Concurrency must be at least 1') + .max(50, 'Concurrency must be at most 50'),<Input type='number' min={1} + max={50} step={1} {...safeNumberFieldProps(field)} />Worth confirming what (if any) upper bound the backend enforces, so the UI limit stays consistent.
Also applies to: 466-489
🤖 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 `@web/default/src/features/system-settings/models/routing-reliability-section.tsx` around lines 78 - 89, The `monitor_setting` schema in `routing-reliability-section.tsx` only enforces a minimum for `test_concurrency`, so add an explicit upper cap in the `z.coerce.number().int()` chain similar to `RetryTimes` to prevent overly large worker-pool values. Update the `test_concurrency` validation in `monitor_setting` (and any matching schema referenced by the same settings form) to clamp to a sane maximum that aligns with backend behavior, and ensure the user-facing error message explains the allowed range.
🤖 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.
Nitpick comments:
In
`@web/default/src/features/system-settings/models/routing-reliability-section.tsx`:
- Around line 78-89: The `monitor_setting` schema in
`routing-reliability-section.tsx` only enforces a minimum for
`test_concurrency`, so add an explicit upper cap in the
`z.coerce.number().int()` chain similar to `RetryTimes` to prevent overly large
worker-pool values. Update the `test_concurrency` validation in
`monitor_setting` (and any matching schema referenced by the same settings form)
to clamp to a sane maximum that aligns with backend behavior, and ensure the
user-facing error message explains the allowed range.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b6a26c16-2b86-4180-b42d-7d560532ec54
📒 Files selected for processing (6)
web/default/src/features/system-settings/models/index.tsxweb/default/src/features/system-settings/models/routing-reliability-section.tsxweb/default/src/features/system-settings/models/section-registry.tsxweb/default/src/features/system-settings/types.tsweb/default/src/i18n/locales/en.jsonweb/default/src/i18n/locales/zh.json
✅ Files skipped from review due to trivial changes (1)
- web/default/src/i18n/locales/en.json
The ModelSettings type now requires monitor_setting.test_concurrency, so the default object in model-mutate-drawer must include it. Also run i18n:sync to propagate the two new Routing Reliability strings to all locale files.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web/default/src/i18n/locales/zh-TW.json (1)
4355-4356: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew keys added with English placeholder values instead of Traditional Chinese translations.
Both
"Test concurrency"and"Number of channels tested in parallel during a batch test (1 = one at a time)."are added with English text as thetranslationvalue, identical to the source key. Every other entry in this file has a proper Traditional Chinese translation. Note the flat English-key convention itself is fine per prior guidance; the issue here is the untranslated value.🌐 Suggested translation
- "Test concurrency": "Test concurrency", - "Number of channels tested in parallel during a batch test (1 = one at a time).": "Number of channels tested in parallel during a batch test (1 = one at a time).", + "Test concurrency": "測試並行數", + "Number of channels tested in parallel during a batch test (1 = one at a time).": "大量測試時同時測試的渠道數量（1 = 逐個測試）。",Based on learnings, flat English-key i18n convention in
web/default/src/i18n/locales/is correct and not itself a violation; the concern here is the untranslated value for the zh-TW locale.🤖 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 `@web/default/src/i18n/locales/zh-TW.json` around lines 4355 - 4356, The zh-TW locale entries for the new i18n keys still use English placeholder text instead of Traditional Chinese. Update the translation values for “Test concurrency” and “Number of channels tested in parallel during a batch test (1 = one at a time).” in the zh-TW JSON file so they match the locale, keeping the existing flat English-key convention intact and changing only the untranslated values.Source: Learnings
🤖 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.
Nitpick comments:
In `@web/default/src/i18n/locales/zh-TW.json`:
- Around line 4355-4356: The zh-TW locale entries for the new i18n keys still
use English placeholder text instead of Traditional Chinese. Update the
translation values for “Test concurrency” and “Number of channels tested in
parallel during a batch test (1 = one at a time).” in the zh-TW JSON file so
they match the locale, keeping the existing flat English-key convention intact
and changing only the untranslated values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8321ab49-7939-49c8-bd78-73e81406ffdb
📒 Files selected for processing (9)
web/default/src/features/models/components/drawers/model-mutate-drawer.tsxweb/default/src/i18n/locales/_reports/_sync-report.jsonweb/default/src/i18n/locales/_reports/ja.untranslated.jsonweb/default/src/i18n/locales/_reports/ru.untranslated.jsonweb/default/src/i18n/locales/fr.jsonweb/default/src/i18n/locales/ja.jsonweb/default/src/i18n/locales/ru.jsonweb/default/src/i18n/locales/vi.jsonweb/default/src/i18n/locales/zh-TW.json
✅ Files skipped from review due to trivial changes (4)
- web/default/src/i18n/locales/_reports/ja.untranslated.json
- web/default/src/i18n/locales/_reports/ru.untranslated.json
- web/default/src/i18n/locales/ru.json
- web/default/src/i18n/locales/vi.json
1 participant
📝 变更描述 / Description
Batch channel testing (
performChannelTestsincontroller/channel-test.go) tested every channel strictly sequentially — onetestChannelcall at a time. On instances with many channels a run takes very long because each slow/dead upstream is waited on one after another.This PR makes the batch test run channels through a bounded worker pool whose size is configurable, while keeping the default behavior identical to today.
Backend
MonitorSetting.ChannelTestConcurrency(json:"test_concurrency"), default1→ existing deployments are unchanged unless they opt in.CHANNEL_TEST_CONCURRENCY(mirrorsCHANNEL_TEST_FREQUENCY/CHANNEL_TEST_ENABLED); non-positive values normalize to1.performChannelTestsdispatches to at mostNconcurrent goroutines: sharedchannelTestSummaryis mutex-guarded, progress is reported through a single serializedadvanceProgress(monotonic),ctxcancellation is honored while acquiring a slot, andRequestIntervalthrottles strictly between dispatches (no trailing wait, no leaked timer).Frontend (default theme)
monitor_setting.test_concurrency(mirrors the existing test-interval field), with en/zh strings andi18n:syncapplied to the other locales.Why it works
Per-channel
testChannelcalls each build their own gin context and recorder, so they are independent. The only shared state (summary counters, progress) is synchronized. Per-channel side effects (processChannelError,EnableChannel,UpdateResponseTime, consume-log writes) operate on independent rows through GORM, which is safe for concurrent use.🚀 变更类型 / Type of change
1)🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
go build/go vet/包测试通过；前端bun run typecheck通过，改动文件oxlint0 error。1保持原有行为。📸 运行证明 / Proof of Work