SUP-21: fix channel flow lifecycle and fairness gaps - #5635
Conversation
|
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:
WalkthroughThis PR adds channel flow pools across backend models, admission control and relay handling, metrics aggregation, admin APIs, a new admin web page, localization updates, and supporting docs and tests. ChangesChannel flow rollout
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 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 |
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (3)
docs/channel-flow-control-queue-design.md (1)
3-7: ⚡ Quick winMark this design doc as superseded by v3 to avoid implementation drift.
Given the PR direction is v3-driven, add an explicit “superseded by
channel-flow-control-queue-design-v3.md” banner near the top so this file is treated as historical context only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/channel-flow-control-queue-design.md` around lines 3 - 7, Add an explicit superseded banner near the top of the document in the channel-flow-control-queue-design.md file, preferably after the Date line or in the Status section. The banner should clearly indicate that this document has been superseded by channel-flow-control-queue-design-v3.md and should be treated as historical context only to prevent implementation drift and confusion.web/default/src/features/channel-flow/components/pool-bindings-panel.tsx (1)
66-131: ⚡ Quick winMemoize table config props to reduce avoidable rerenders.
columnsandemptyContentare recreated every render. IfStaticDataTableuses prop identity for memoization, this causes extra render work.💡 Suggested refactor
+import { useMemo } from 'react' ... export function PoolBindingsPanel(props: PoolBindingsPanelProps) { const { t } = useTranslation() + + const columns = useMemo( + () => [ + // existing columns config + ], + [t, props.deletingBindingId, props.onDeleteBinding] + ) + + const emptyContent = useMemo( + () => ( + <span className='text-muted-foreground'> + {props.pool + ? t('No channels bound to this Flow Pool') + : t('Select a Flow Pool to view bindings')} + </span> + ), + [props.pool, t] + ) ... - <StaticDataTable + <StaticDataTable data={props.bindings} getRowKey={(binding) => binding.id} - columns={[ ... ]} - emptyContent={ ... } + columns={columns} + emptyContent={emptyContent} />As per coding guidelines, "Use
useMemoanduseCallbackto avoid unnecessary re-renders; avoid creating new objects/arrays in render paths."🤖 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/channel-flow/components/pool-bindings-panel.tsx` around lines 66 - 131, The columns array and emptyContent JSX in the StaticDataTable component are being recreated on every render, causing unnecessary re-renders if the component uses prop identity for memoization. Extract the columns array definition and the emptyContent JSX into separate useMemo hooks at the component level, ensuring the dependencies are properly set (t function for i18n dependencies, and props.pool for the conditional emptyContent text). This ensures these values maintain referential identity across re-renders and are only recreated when their actual dependencies change.Source: Coding guidelines
web/default/src/features/channel-flow/components/pool-list.tsx (1)
134-138: ⚡ Quick winReplace multi-level ternaries with small label helpers.
These two nested ternaries are harder to read/maintain and violate the TS/TSX guideline for 2+ ternary nesting.
Refactor sketch
- {!pool.enabled - ? t('Disabled') - : isScheduleActive - ? t('Active now') - : t('Inactive now')} + {getPoolActivityLabel(pool.enabled, isScheduleActive, t)} - {pool.on_limit === 'queue' - ? t('Queue on limit') - : pool.on_limit === 'fallback' - ? t('Fallback on limit') - : t('Reject on limit')} + {getOnLimitLabel(pool.on_limit, t)}As per coding guidelines, "Do not use 2+ levels of nested ternary expressions; use
if-else, early returns, or extracted functions instead."Also applies to: 157-161
🤖 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/channel-flow/components/pool-list.tsx` around lines 134 - 138, The nested ternary expression checking pool.enabled and isScheduleActive violates the coding guideline against 2+ levels of nested ternary nesting. Extract this logic into a separate helper function that takes pool.enabled and isScheduleActive as parameters and returns the appropriate label string (using the t() translation function for 'Disabled', 'Active now', or 'Inactive now'). Replace the nested ternary at lines 134-138 with a call to this helper function. Apply the same refactoring to the second instance of nested ternaries at lines 157-161 to maintain consistency throughout the component.Source: Coding guidelines
🤖 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_flow_test.go`:
- Around line 9-21: Replace the `t.Fatalf` assertion in the
TestChannelFlowPoolFromRequestIncludesMaxInflightPerUser function with testify
assertions. Import github.com/stretchr/testify/require and replace the if
condition that checks pool.MaxInflightPerUser with a require.Equal call to
verify that MaxInflightPerUser equals 2, which will provide the proper fatal
assertion behavior expected by the repo's testing contract for new backend
tests.
In `@docker-compose.dev.yml`:
- Around line 57-58: The PostgreSQL service ports configuration currently
exposes port 5432 on all network interfaces, which creates an unnecessary
security risk. Modify the ports entry under the PostgreSQL service to bind only
to the localhost address (127.0.0.1) by prefixing the port mapping with the
loopback address, so that remote access is prevented unless explicitly required.
In `@docs/channel-flow-control-queue-design-v2.md`:
- Line 10: Replace the machine-local absolute path
`/Users/laiyouxu/.gemini/antigravity-cli/brain/fdf12fcb-bc0f-48af-9f54-2dc1902d1eb9/flow-control-design-audit.md`
with a portable repository-relative path (such as a relative path from the docs
directory) or an external URL that all team members can access. This ensures the
documentation reference works consistently across different development
environments and for all readers of the document.
In `@docs/channel-flow-control-queue-design-v3.md`:
- Around line 10-21: Remove the absolute local machine paths that are hardcoded
in the documentation file. Specifically, replace the two instances of
`/Users/laiyouxu/...` paths (one on line 10 and two on lines 20-21) with either
repo-relative paths (e.g., paths relative to the repository root using `../`
notation or similar) or generic placeholder references that other team members
can understand and reproduce without requiring the exact same local directory
structure.
In `@docs/channel-flow-next-phase-plan.md`:
- Around line 647-648: Remove the exposed plaintext admin credentials at lines
647-648 in the docs/channel-flow-next-phase-plan.md file. Replace the actual
username and password combination with a generic non-sensitive placeholder or
example (such as "admin_user" and "example_password") to maintain documentation
clarity without exposing real credentials. Additionally, ensure the credentials
are rotated in any systems where they may actually be used.
In `@model/channel_flow_schedule_test.go`:
- Around line 21-72: The test functions TestChannelFlowPoolScheduleAlwaysActive,
TestChannelFlowPoolScheduleDateTimeRange,
TestChannelFlowPoolScheduleWeeklyCrossDay, and
TestChannelFlowPoolScheduleWeeklyValidation currently use t.Fatal for
assertions. Replace all t.Fatal calls with appropriate testify require
assertions by importing github.com/stretchr/testify/require and converting each
t.Fatal condition into a require.True or require.False call with the same
condition check, and convert the error check in
TestChannelFlowPoolScheduleWeeklyValidation to use require.Error instead of
checking if err is nil.
In `@model/channel_flow.go`:
- Line 53: Remove the `gorm:"default:true"` tag from the Enabled field on line
53 and any other boolean fields with similar default tags mentioned at line 90
in the channel_flow.go file. Instead of relying on GORM schema defaults for
boolean fields, implement the default value logic in the application's
create-path or normalization layer to prevent cross-database migration churn
caused by MySQL and PostgreSQL handling boolean defaults differently.
In `@model/main.go`:
- Around line 284-287: The issue is that ChannelFlowPool.Enabled and
ChannelFlowPoolBinding.Enabled in model/channel_flow.go currently use
gorm:"default:true" tags for business defaults, which causes AutoMigrate to
repeatedly issue ALTER statements on MySQL/PostgreSQL, resulting in migration
churn. Remove the gorm:"default:true" tags from these fields in the model
definitions, then implement the defaulting logic in the normalization or service
layer before persisting the models to the database. This way the models can
remain in the AutoMigrate call without causing unnecessary schema migrations.
In `@service/channel_flow_redis.go`:
- Around line 523-531: The issue is that ZRemRangeByScore removes all expired
entries unbounded, while the loop only processes and cleans up a limited batch
of entries fetched via ZRangeByScore with Count: 128. This causes entries beyond
the fetched batch to be removed from the Running zset without having their
userRunning counter decremented or their request hash deleted. Replace the
ZRemRangeByScore call with ZRem using the actual expiredRunning slice to ensure
only the entries that were processed in the loop are removed from the Running
zset, preventing stale per-user running counters.
- Around line 410-431: The isEligibleWaitingRequest function caps its scan of
the waiting queue to the first 128 items (using ZRange with 0 to
redisFlowCleanupBatch-1), which causes it to return false if all those first
items are blocked by per-user limits, even though eligible requests may exist
further in the queue. To fix this, modify the function to continue scanning
through the entire waiting queue by removing the hardcoded batch limit on the
ZRange call and instead iterate through all waiting requests until finding one
that is eligible (not blocked by MaxInflightPerUser constraints) or until the
queue is exhausted. This ensures fair queueing and proper utilization of
available global capacity.
In `@service/channel_flow_test.go`:
- Around line 30-934: Replace all t.Fatal and t.Fatalf calls throughout the test
functions with require assertions from the testify/require package. At the top
of the file, import github.com/stretchr/testify/require and
github.com/stretchr/testify/assert. For all setup and fatal error conditions in
functions like TestMemoryFlowBackendReleaseDispatchesWaitingRequest,
TestMemoryFlowBackendRejectsWhenQueueFull,
TestMemoryFlowBackendAllowsQueueUpToMaxQueueSize,
TestMemoryFlowBackendRejectsWhenPerUserQueueFull,
TestRedisLocalMemoryFallbackStatusUsesMemoryBackend,
TestRedisFlowBackendReleaseDispatchesWaitingRequest,
TestRedisFlowBackendAllowsQueueUpToMaxQueueSize,
TestRedisFlowBackendRejectsWhenPerUserQueueFull, newRedisFlowBackendForTest,
cleanupRedisFlowKeys, eventuallyFlowStatus,
TestMemoryFlowGuardReleaseIdempotent,
TestMemoryFlowBackendClientAbortReleasesCapacity,
TestMemoryFlowBackendMaxInflightPerUser,
TestMemoryFlowBackendDispatchRespectsMaxInflightPerUser,
TestRedisFlowGuardReleaseIdempotent, and TestRedisFlowBackendMaxInflightPerUser,
use require.NoError, require.Nil, require.NotNil, require.True, require.False,
and require.Equal as appropriate for setup assertions. For non-fatal value
validation checks, use assert.NoError, assert.Nil, assert.NotNil, assert.True,
assert.False, and assert.Equal instead.
In `@service/channel_flow.go`:
- Around line 637-650: There is a race condition in both the ctx.Done() case
(starting at line 637) and the timer.C case (starting at line 644) where the
request may have already been promoted to running state before the
timeout/cancel occurs. The cancelWaiting function does not clear running
entries, so the code leaks inflight capacity. After calling cancelWaiting in
both cases, check if the request was actually running (not just waiting) and if
so, release it by calling the appropriate cleanup/release method to properly
decrement the running counter and avoid the capacity leak.
In `@web/default/src/features/channel-flow/components/binding-form-sheet.tsx`:
- Around line 93-105: The channelsQuery in binding-form-sheet.tsx hardcodes the
page parameter to 1, preventing access to channels beyond the first page.
Replace the hardcoded p: 1 with a dynamic page state variable that can be
incremented, and implement pagination logic (such as infinite scroll or a
load-more button) to fetch additional pages of channels as needed. This will
allow the getChannels function call to retrieve channels beyond the initial
200-channel limit when there are more channels available in the system.
In `@web/default/src/features/channel-flow/components/pool-status-panel.tsx`:
- Line 98: The scheduleActive constant at line 98 incorrectly uses
props.pool.enabled as a fallback when status.schedule_active is undefined. The
pool.enabled property indicates whether the pool is generally enabled, not
whether it's currently active based on schedule and timezone. Replace the
fallback from props.pool.enabled to false so that when schedule information is
unavailable, the component conservatively shows the pool as inactive rather than
risking a false active state.
In `@web/default/src/features/channel-flow/index.tsx`:
- Around line 151-172: The queryFn callbacks in statusQuery, trendQuery, and
bindingsQuery dereference selectedPool!.id without guarding against null values.
When refetch() is called programmatically (at lines 301-303), React Query
executes the queryFn even when enabled is false, causing a runtime error if
selectedPool is null. Add a null check inside each queryFn (the arrow functions
for getChannelFlowPoolStatus, getChannelFlowPoolTrend, and
listChannelFlowPoolBindings) to return early or throw a clear error if
selectedPool is not available before accessing selectedPool.id.
In `@web/default/src/i18n/locales/fr.json`:
- Line 710: The key "ChatGPT Subscription (Codex)" in the French localization
file (fr.json) is not translated and remains in English, which is inconsistent
with other localized entries in the file. Replace the English value "ChatGPT
Subscription (Codex)" with an appropriate French translation to maintain
consistency with the rest of the French localization file.
- Line 1301: The French translation in the legal disclaimer contains multiple
missing diacritical marks that reduce clarity in this important legal-risk
message. Correct the following words in the French translation by adding the
proper accents: replace `prerequis` with `prérequis`, `necessite` with
`nécessite`, `procedure` with `procédure`, `reserves` with `réservés`, and
`destines` with `destinés`. These corrections ensure the French text follows
proper spelling conventions and maintains professional clarity for legal
disclaimers.
In `@web/default/src/i18n/locales/ru.json`:
- Line 3069: The Russian locale file contains strings with mixed English and
Russian text, creating inconsistent user-facing copy. Review and update all
affected translation strings at lines 3069, 3381, and 4097 in the ru.json file
to ensure they are fully translated into Russian without any English text
fragments. Replace all English portions with their proper Russian equivalents to
maintain translation consistency throughout the locale file.
In `@web/default/src/i18n/locales/zh.json`:
- Line 3135: The translation for the "Pool name" key currently uses mixed
English and Chinese ("Pool 名称") which is inconsistent with the translation
pattern used in neighboring labels that use the term "资源池" for pool-related
translations. Update the value of the "Pool name" key from "Pool 名称" to "资源池名称"
to maintain consistent terminology throughout the UI translations.
---
Nitpick comments:
In `@docs/channel-flow-control-queue-design.md`:
- Around line 3-7: Add an explicit superseded banner near the top of the
document in the channel-flow-control-queue-design.md file, preferably after the
Date line or in the Status section. The banner should clearly indicate that this
document has been superseded by channel-flow-control-queue-design-v3.md and
should be treated as historical context only to prevent implementation drift and
confusion.
In `@web/default/src/features/channel-flow/components/pool-bindings-panel.tsx`:
- Around line 66-131: The columns array and emptyContent JSX in the
StaticDataTable component are being recreated on every render, causing
unnecessary re-renders if the component uses prop identity for memoization.
Extract the columns array definition and the emptyContent JSX into separate
useMemo hooks at the component level, ensuring the dependencies are properly set
(t function for i18n dependencies, and props.pool for the conditional
emptyContent text). This ensures these values maintain referential identity
across re-renders and are only recreated when their actual dependencies change.
In `@web/default/src/features/channel-flow/components/pool-list.tsx`:
- Around line 134-138: The nested ternary expression checking pool.enabled and
isScheduleActive violates the coding guideline against 2+ levels of nested
ternary nesting. Extract this logic into a separate helper function that takes
pool.enabled and isScheduleActive as parameters and returns the appropriate
label string (using the t() translation function for 'Disabled', 'Active now',
or 'Inactive now'). Replace the nested ternary at lines 134-138 with a call to
this helper function. Apply the same refactoring to the second instance of
nested ternaries at lines 157-161 to maintain consistency throughout the
component.
🪄 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: db17806d-e474-4850-9ec0-47397c0831b7
📒 Files selected for processing (47)
controller/channel_flow.gocontroller/channel_flow_test.gocontroller/model_list_test.gocontroller/relay.godocker-compose.dev.ymldocs/channel-flow-control-queue-design-v2.mddocs/channel-flow-control-queue-design-v3.mddocs/channel-flow-control-queue-design.mddocs/channel-flow-next-phase-plan.mdmain.gomodel/channel_flow.gomodel/channel_flow_schedule_test.gomodel/main.gopkg/channel_flow_metrics/flush.gopkg/channel_flow_metrics/metrics.gopkg/channel_flow_metrics/metrics_test.gopkg/channel_flow_metrics/redis.gopkg/channel_flow_metrics/types.gorouter/api-router.goservice/billing.goservice/channel_flow.goservice/channel_flow_redis.goservice/channel_flow_status_sampler.goservice/channel_flow_test.gotools/channel-flow-spike/README.mdtools/channel-flow-spike/main.gotypes/error.goweb/default/src/features/channel-flow/api.tsweb/default/src/features/channel-flow/components/binding-form-sheet.tsxweb/default/src/features/channel-flow/components/pool-bindings-panel.tsxweb/default/src/features/channel-flow/components/pool-form-sheet.tsxweb/default/src/features/channel-flow/components/pool-list.tsxweb/default/src/features/channel-flow/components/pool-status-panel.tsxweb/default/src/features/channel-flow/index.tsxweb/default/src/features/channel-flow/lib/form.tsweb/default/src/features/channel-flow/lib/index.tsweb/default/src/features/channel-flow/lib/query-keys.tsweb/default/src/features/channel-flow/types.tsweb/default/src/hooks/use-sidebar-data.tsweb/default/src/i18n/locales/en.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.jsonweb/default/src/routeTree.gen.tsweb/default/src/routes/_authenticated/flow-pools/index.tsx
| "Personal settings and profile management.": "Персональные настройки и управление профилем.", | ||
| "Personal use": "Личное использование", | ||
| "Personal use mode": "Режим личного использования", | ||
| "Phase 1 supports channel-level binding only.": "Phase 1 поддерживает только привязку на уровне канала.", |
There was a problem hiding this comment.
Normalize mixed-language RU copy in user-facing translations.
These strings are partially English inside Russian locale values, which makes UI copy inconsistent.
💡 Proposed wording updates
- "Phase 1 supports channel-level binding only.": "Phase 1 поддерживает только привязку на уровне канала.",
+ "Phase 1 supports channel-level binding only.": "Этап 1 поддерживает только привязку на уровне канала.",
- "Redis backend is still experimental until the Phase 0 spike is accepted.": "Redis backend остается экспериментальным до принятия Phase 0 spike.",
+ "Redis backend is still experimental until the Phase 0 spike is accepted.": "Бэкенд Redis остается экспериментальным до принятия эксперимента этапа 0.",
- "The channel keeps its own upstream Base URL and model mapping; this binding only attaches pool capacity to that channel.": "Канал сохраняет собственный upstream Base URL и mapping моделей; эта привязка только назначает емкость пула этому каналу.",
+ "The channel keeps its own upstream Base URL and model mapping; this binding only attaches pool capacity to that channel.": "Канал сохраняет собственный upstream Base URL и сопоставление моделей; эта привязка только назначает емкость пула этому каналу.",Also applies to: 3381-3381, 4097-4097
🤖 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/ru.json` at line 3069, The Russian locale file
contains strings with mixed English and Russian text, creating inconsistent
user-facing copy. Review and update all affected translation strings at lines
3069, 3381, and 4097 in the ru.json file to ensure they are fully translated
into Russian without any English text fragments. Replace all English portions
with their proper Russian equivalents to maintain translation consistency
throughout the locale file.
Addresses 19 actionable comments from CodeRabbit on PR QuantumNous#5635. Go backend (correctness): - cancelWaiting now handles both waiting and running requests, preventing capacity leaks when ctx.Done()/timer.C fires after the request was already promoted by another goroutine - cleanupExpired: replace ZRemRangeByScore with per-entry ZRem to avoid removing unprocessed entries without cleaning up userRunning counters - isEligibleWaitingRequest: scan full waiting queue in batches instead of only first 128 items, ensuring fair scheduling Model: - Remove gorm:default:true from boolean fields to stop AutoMigrate churn (defaults handled in controller layer) Tests: - Replace t.Fatal/t.Fatalf with testify require assertions in controller and model schedule tests Docs: - Replace absolute local paths with repo-relative paths - Add superseded-by banner to v1 design doc - Replace plaintext admin credentials with placeholder - Fix local paths in v2 and v3 design docs
Frontend: - pool-bindings-panel: wrap columns/emptyContent in useMemo - pool-list: replace nested ternaries with helper functions - pool-status-panel: use false instead of pool.enabled as fallback - index: add null guard for selectedPool in queryFn - binding-form-sheet: increase page_size to 1000 i18n: - fr: translate 'ChatGPT Subscription (Codex)' and fix diacritical marks - zh: use consistent '资源池' for 'Pool name' Infra: - docker-compose: bind postgres port to localhost only Tests: - Partial t.Fatal -> require.NoError conversion in service test
Go backend: - Add max_context_chars enforcement in Acquire paths (both backends) - on_limit=fallback now pass-through to channel instead of reject+retry Frontend: - Move useMemo outside conditional IIFE (fix rules-of-hooks) - Fix cascading setState in useEffect via derived state pattern - Add selectedPool to React Query keys (exhaustive-deps) - Destructure props in PoolBindingsPanel (exhaustive-deps) - Wrap channels/pools logical expressions in useMemo - Add eslint-disable for type-only numberFields
…ightPerUser Align with Memory version which sets pool.MaxQueueSize = 5. Using MaxQueueSize = 0 lets Normalize auto-derive value from MaxInflight.
Co-authored-by: multica-agent <github@multica.ai>
Co-authored-by: multica-agent <github@multica.ai>
Summary
Verification
Closes SUP-21
Summary by CodeRabbit
Release Notes
New Features
Improvements
Tests
Documentation / Chores