feat(clients): add per-client download/upload speed limits via tc HTB

[HamidRezaSZ]

Store speedDown/speedUp on clients and shape traffic for observed client IPs on the primary interface so operators can cap bandwidth without Fail2ban.

Summary

Adds optional per-client download/upload speed limits (Mbps), stored on the client record and enforced with Linux tc HTB (egress) plus a shared ingress police action per client on the primary interface. Shaping is driven by the existing online-IP scan job and is disabled by default behind a speedLimitEnable setting.

Why

Operators often need to cap a client’s bandwidth without Fail2ban or IP bans. This gives panel-level speed controls that apply to observed client IPs on the host’s primary NIC, without seizing the host qdisc unless an admin explicitly opts in.

Type of change

• Bug fix
• New feature
• Refactoring (no behavior change)
• Documentation
• Tests only
• Build / CI / tooling
• Other

Areas affected

• Frontend (UI / panel pages)
• Backend (API endpoints, login, settings)
• Xray config generation
• Subscription (share links / Clash / JSON)
• Statistics / traffic counters
• Database / migrations
• Install / upgrade script
• Docker image
• Multi-node (sub-nodes)
• Telegram bot

How was this tested?

1. Build/compile the changed packages and regenerate OpenAPI/Zod via make gen.
2. Unit tests: go test ./internal/web/service/ -run 'TestUniqueValidIPs|TestIPMatch|TestTcShaper' (Sync add/update/remove, shared police across multiple IPs).
3. Settings → General: leave Client Speed Limit off; confirm no tc takeover on panel start/restart. Enable it, restart panel, then proceed.
4. Panel → Clients → create/edit: set Download / Upload (Mbps), save, reopen and confirm values persist. Confirm fields are hidden for MTProto-only clients and when the setting is off.
5. Bulk add (with the setting enabled): set speed fields and confirm created clients receive them.
6. On Linux with tc and root (or CAP_NET_ADMIN): connect a limited client, wait for the IP-scan job (~10s), then check tc class show / tc filter show / tc actions for HTB (down) and a shared police index referenced by each IP (up).
7. With limitIp > 1 and a nonzero speedUp, confirm multiple observed IPs share one police action (aggregate upload cap).
8. Set limits to 0 (or disconnect), wait for the next sync, and confirm classes/filters/actions are removed.
9. Confirm Fail2ban IP limiting still runs independently when enabled.

Screenshots / recordings

Breaking changes

None for API consumers beyond optional new fields (speedDown, speedUp; default 0 = unlimited) and optional setting speedLimitEnable (default false).

Ops note: Shaping does not run until Client Speed Limit is enabled and the panel is restarted. When enabled on Linux, the panel takes over the primary interface root qdisc with HTB and may install an ingress qdisc—hosts with an existing custom tc/QoS setup should opt in knowingly. Requires tc and sufficient privileges; unavailable platforms (e.g. Windows) skip shaping. New columns are added via GORM AutoMigrate (speed_down, speed_up).

Checklist

• I tested the change locally and confirmed the described behavior.
• I added or updated tests for the new behavior (when applicable).
• go build ./... and the test suite pass locally.
• For frontend changes: npm run lint, npm run typecheck, and npm run build pass.
• I updated the Wiki / README / API docs if user-facing behavior changed.
• My commits follow the project's existing message style.
• I have no unrelated changes mixed into this PR.

[HamidRezaSZ]

Summary

This PR adds per-client speedDown/speedUp fields (DB, API, frontend forms) and a new tc-based HTB/ingress-policing shaper (internal/web/service/tc_shaper.go, 389 lines) driven by the existing 10s online-IP scan job. The data-model and frontend plumbing is careful and consistent (i18n across all 13 locales, generated OpenAPI/Zod/TS artifacts, AutoMigrate). However, the shaping subsystem itself has two significant gaps: it runs unconditionally with no admin opt-out despite seizing the host's primary-interface qdisc, and its upload enforcement does not actually cap a client's aggregate bandwidth once the client has more than one concurrent IP. No tests were added for either the new shaper or the modified job logic.

Findings

Severity: High Confidence: High Category: feature-gating Location: internal/web/web.go:664 Problem: service.DetectPrimaryInterface() + TcShaper.Init() (internal/web/service/tc_shaper.go:67) run unconditionally on every panel start and every panel restart whenever tc is on PATH and a default route is detected — there is no Setting, env var, or other admin-facing toggle. Init() unconditionally runs tc qdisc del dev <iface> root (tc_shaper.go:74) and tc qdisc del dev <iface> ingress (tc_shaper.go:84) before installing its own HTB/ingress qdiscs, destroying any pre-existing qdisc configuration on that interface. Because StopPanelOnly/StartPanelOnly (used by the routine SIGHUP restart path in main.go for ordinary settings changes) map to stop(false, true)/start(false, true), and stop() calls TcShaper.Cleanup() unconditionally (web.go:701-703) while start() calls Init() again unconditionally, this teardown/recreate cycle runs on every panel restart, not just once at boot. Why it matters: every other optional or invasive background behavior in this codebase is gated behind an explicit, default-off control: ldapEnable and tgBotEnable both default to "false" (internal/web/service/setting.go:70,130), and the tunnel-health monitor — which only restarts xray — ships disabled by default specifically because the action is disruptive ("disabled-by-default monitor settings", internal/tunnelmonitor/monitor.go:53-63). Even Fail2ban-based IP limiting, the closest sibling feature and one living in this exact job, never performs a host-level action itself — it only appends a log line for a separately, manually installed fail2ban jail to act on (check_client_ip_job.go:519-533) — and the codebase carries a dedicated migration to zero out stray settings when enforcement isn't possible (resetIpLimitsWithoutFail2ban, internal/database/db.go:1236). This PR's feature has a strictly larger blast radius than any of those (it reprograms kernel queueing for the whole interface, not just x-ui's own traffic) yet ships with no opt-out at all. An operator with an existing custom tc/QoS setup (acknowledged in the PR's own "Ops note") silently loses it on upgrade, and again on every future panel restart. Recommendation: add a default-off Setting (e.g. speedLimitEnable), following the ldapEnable pattern, and only call DetectPrimaryInterface/Init() when explicitly enabled by the admin.

Severity: High Confidence: High Category: correctness Location: internal/web/service/tc_shaper.go:302 Problem: addUpFilterLocked installs an independent, unshared tc filter ... police rate <upMbps>mbit ... drop per observed IP. Unlike the download path, where every IP of a client is routed into the same shared HTB class (ensureDownClassLocked, tc_shaper.go:228, reused for all of a client's IPs in addClientLocked/updateClientLocked), there is no shared or indexed police action tying a client's multiple IPs to one combined upload rate. Why it matters: limitIp, this same panel's own concurrent-IP-per-client control, explicitly allows more than one simultaneous IP per client. For a client with, say, limitIp=3 and speedUp=10, each of the three IPs gets its own independent 10 Mbps policer, so the client can achieve roughly 30 Mbps aggregate upload. The configured cap is not enforced as a per-client limit once more than one IP is active, silently defeating the feature's stated purpose for a normal, explicitly supported configuration. Recommendation: create one shared/indexed police action per client (tc actions add action police rate ... burst ... index N) and reference it from each per-IP filter (... action police index N), mirroring the aggregation the HTB class already gives the download path.

Severity: Medium Confidence: Medium Category: correctness Location: frontend/src/pages/clients/ClientFormModal.tsx:677 Problem: the new Download/Upload speed fields are added to the client form's protocol-agnostic "basic" tab, shown for every protocol including MTProto (neither the tab nor this row carries a protocol condition). Per this repository's own architecture, MTProto inbounds run entirely outside Xray as a separate mtg-multi child process; syncTcRules (internal/web/job/check_client_ip_job.go:756) only ever sees IPs returned by xrayService.GetOnlineUsers(), which wraps Xray's own gRPC StatsServiceClient.GetUsersStats (internal/xray/api.go:731-742) and has no path to mtg-multi's connections. Why it matters: an admin can set a Download/Upload speed on an MTProto client, save it successfully, and it will never be enforced. The UI presents a control that is silently a no-op for that protocol, with no error or indication. Recommendation: hide or disable the speed fields when the client's protocol is mtproto (and double-check wireguard, whose peer model may not surface per-connection IPs the same way vmess/vless/trojan/shadowsocks do), or note the limitation in the field tooltip.

Severity: Medium Confidence: High Category: test-coverage Location: internal/web/service/tc_shaper.go (whole file); internal/web/job/check_client_ip_job.go:756-798 Problem: no tests accompany either the new tc_shaper.go or the modified job logic (syncTcRules, loadClientSpeeds, the changed Run() control flow), and the PR's own checklist leaves "I added or updated tests" unchecked. Why it matters: CheckClientIpJob, the exact file this PR modifies, already has four dedicated test files (check_client_ip_job_test.go, check_client_ip_job_integration_test.go, check_client_ip_scale_test.go, check_client_ip_frozen_ban_test.go), showing this job is normally held to a real test bar in this codebase. The pure logic in tc_shaper.go (IP validation, class/filter id allocation, add/update/remove diffing in Sync) is unit-testable without a real tc binary by injecting a fake runner; none of that was exercised here. Recommendation: add unit tests for TcShaper.Sync's add/update/remove diffing (with runTC replaced by a fake/recording function) and for uniqueValidIPs/ipMatch, plus a table-driven test for the new Run() branching (fail2ban-only, tc-only, both, neither).

Severity: Low Confidence: High Category: performance Location: internal/web/job/check_client_ip_job.go:756 Problem: syncTcRules unconditionally calls loadClientSpeeds, which runs a chunked speed_down, speed_up query for every observed email on every 10-second scan whenever tcShaper != nil (effectively any Linux host with tc installed) — even if no client has ever set a nonzero speed. The sibling LimitIP path in the same file guards its equivalent per-email work behind a single cheap probe, hasLimitIp() (check_client_ip_job.go:122), before doing any per-email querying. Recommendation: add an analogous hasSpeedLimit() probe (Where("speed_down > 0 OR speed_up > 0").Limit(1).Count(...)) and skip syncTcRules when it reports false.

Positive observations

The data-model plumbing is careful: all 13 locale files receive the new speedDown/speedUp keys (a commonly missed step), the generated OpenAPI/Zod/TypeScript artifacts are internally consistent with the new fields, and the new columns ride the existing GORM AutoMigrate convention correctly, including on Postgres (postgresModelSettled will correctly detect the missing columns on existing installs and trigger the migration).

Verdict

Request changes. The two High-severity findings — an invasive, ungated host network change, and an upload cap that doesn't actually hold for multi-IP clients — mean the feature does not yet deliver on its stated purpose and can cause real operational harm on hosts with pre-existing tc configuration. @MHSanaei please weigh in on both before merge.

This review was generated automatically; a maintainer may follow up.

The findings are addressed in the latest commits:

High — feature gating
Added a default-off speedLimitEnable setting (Settings → General). DetectPrimaryInterface / TcShaper.Init() only run when that flag is enabled, so upgrades and panel restarts no longer seize the primary iface qdisc unless an admin opts in.

High — multi-IP upload cap
Upload limiting now creates one shared tc actions … police index N per client; each observed IP’s ingress filter references that index, so the configured speedUp is enforced as a single aggregate (same idea as the shared HTB class for download).

Medium — MTProto UI
Speed fields are hidden when speed limiting is disabled, and when the client’s attached inbounds are MTProto-only (those clients never appear in Xray online-stats). WireGuard stays available since peers go through Xray.

Medium — tests
Added tc_shaper_test.go covering IP helpers, Sync add/update/remove diffing (via an injected tc runner), and shared police across multiple IPs.

Low — unnecessary DB work
hasSpeedLimit() probes speed_down > 0 OR speed_up > 0 before syncTcRules / loadClientSpeeds, matching the existing hasLimitIp() pattern.

[xAlokyx]

@MHSanaei, this pull request is working perfectly. Why hasn’t it been merged yet? :/

[github-actions]

Summary

This adds per-client download/upload caps stored on ClientRecord and enforced with Linux tc (HTB on egress, a shared ingress police action per client), driven from the existing 10s online-IP scan job and gated behind a new speedLimitEnable setting that defaults to off. The plumbing is careful and complete for a change of this size: all 13 locale files are updated and the keys are referenced, the generated OpenAPI/Zod/types artifacts are regenerated, applyClientRecordMerge and the no-inbound update column map are both extended so the value survives SyncInbound, a NULL-backfill migration is added, and the new shaper ships with real unit tests (including a regression test for u32 handle exhaustion). The risk is concentrated in internal/web/service/tc_shaper.go: it seizes the host's root and ingress qdiscs, hardcodes a u32 hash-table id in a way that looks like it breaks dual-stack hosts, and allocates tc police-action indices from a namespace it shares with the rest of the system.

Reviewed head: 5162cc8

Note for context: three earlier PRs proposing the same feature were closed without discussion (#5629, #4141, #3507). It may be worth confirming with @MHSanaei that there is appetite for panel-managed tc at all before iterating further on the details below.

---

Findings

Severity: High / Confidence: Medium / Category: correctness
Location: internal/web/service/tc_shaper.go:404
Problem: Both filter builders hardcode the u32 hash-table id as 800:: (handle := fmt.Sprintf("800::%x", h) at :404 for the egress filter and :427 for the ingress filter), while adding IPv4 and IPv6 filters at the same parent/prio 1. In cls_u32, each (parent, protocol, prio) tuple is a separate tcf_proto; the first one created on a qdisc gets root hash table 800:, and every subsequent one on the same qdisc gets 801:, 802:, … iproute2 does not send TCA_U32_HASH unless the ht/link keyword is used, so the kernel compares the supplied handle's htid against that tcf_proto's own root ht and rejects a mismatch with -EINVAL ("Handle specified hash table address mismatch").
Why it matters: On a dual-stack panel — the common case — whichever address family gets its first filter installed second will have every tc filter add rejected. The failure is swallowed into a logger.Warningf (:411, :437) and st.downH/st.upH are simply left unset, so those clients are silently unshaped with no UI signal that the limit is not in force. Half the configured limits would quietly not apply.
Recommendation: Do not hardcode the htid. Either drop the explicit handle and record the handle tc assigns (read back from tc filter show), or resolve the per-family root htid once at Init() and use it in both add and delete. Please verify with a live test that installs an IPv4 client filter and an IPv6 client filter on the same interface and then checks tc filter show dev <iface> for both.

Severity: Medium / Confidence: High / Category: reliability
Location: internal/web/service/tc_shaper.go:91
Problem: Init() unconditionally runs tc qdisc del dev <iface> root (:91) and tc qdisc del dev <iface> ingress (:101) before installing its own. Nothing inspects what was there first, nothing is restored on failure (the class-add failure path at :96 only deletes HTB again, dropping back to the kernel default rather than the operator's qdisc), and Cleanup() (:479) leaves the interface qdisc-less rather than restoring the prior configuration.
Why it matters: On a modern host the ffff: slot is frequently a clsact qdisc carrying eBPF programs (Docker network plugins, Cilium, systemd), and the root may be mq on a multiqueue NIC or fq/cake tuned by the operator. Deleting them can break container networking or measurably degrade throughput and latency for all host traffic, not just shaped clients. Separately, Cleanup() only runs from Server.stop(); a SIGKILL, OOM kill or panic leaves the HTB root, the ingress qdisc and the police actions installed, so the IPs observed at that moment stay capped (and policed with conform-exceed drop) with no panel left to remove them.
Recommendation: Probe the existing qdisc first and refuse to take over anything other than the kernel default unless the operator explicitly confirms; log exactly what was replaced. Consider a defer/recover-independent cleanup path or a startup reconciliation that flushes panel-owned classes, filters and actions.

Severity: Medium / Confidence: Medium / Category: reliability
Location: internal/web/service/tc_shaper.go:339
Problem: allocPoliceLocked picks the lowest index (starting at 1) not present in this shaper's own s.applied map — it never consults what actually exists in the kernel. The action is then installed with tc actions replace ... index N (:285) and later removed with tc actions delete action police index N (:312).
Why it matters: tc police indices are a host-global namespace. If any other tool on the box (or a leftover from a previous panel run that did not reach Cleanup()) owns index 1, actions replace silently overwrites its rate/burst, and delPoliceLocked later deletes it outright. Init() flushes the qdiscs but not the action table, so orphaned police actions from a crashed run also survive.
Recommendation: Enumerate tc actions ls action police at Init() and allocate above the highest index in use (or from a high panel-specific base), and flush this shaper's own leftovers there.

Severity: Medium / Confidence: High / Category: correctness
Location: internal/web/job/check_client_ip_job.go:60
Problem: Run() returns at :60 whenever collectFromOnlineAPI reports the online-stats API unavailable — Xray stopped, restarting, or a transient gRPC failure — without reaching the shaper block at :82. Sync is therefore never called with an empty set on that path.
Why it matters: Every class, filter and police action installed for the last-seen IPs stays in the kernel for as long as Xray is down. Those rules apply to all host traffic to/from those addresses, so an operator stopping Xray leaves the box shaping and (on ingress) dropping traffic for IPs that are no longer connected, indefinitely.
Recommendation: Call j.tcShaper.Sync(nil) on that early-return path too, or age entries out of the shaper by last-seen timestamp.

Severity: Medium / Confidence: High / Category: correctness
Location: internal/web/job/check_client_ip_job.go:147
Problem: hasSpeedLimit() returns err == nil && id > 0, so a DB error is indistinguishable from "no client has a limit", and the caller at :83 then takes the else branch and calls Sync(nil).
Why it matters: A transient database error tears down every installed class, filter and police action for that tick and rebuilds them on the next one. Contrast hasLimitIp() (:129), where the same pattern only means "do not enforce this tick" and has no destructive side effect. Here it produces real churn plus a window where every limit is off.
Recommendation: Return a (bool, error) (or keep the previous decision on error) and skip the sync entirely rather than treating a failed probe as "no limits configured".

Severity: Medium / Confidence: High / Category: performance
Location: internal/web/service/tc_shaper.go:116
Problem: Sync holds s.mu and forks one tc process per class, filter and action (runTC, :504, 5s timeout each) — roughly 2 × (total observed IPs) + (clients) execs on the first sync after Init(). It runs inline in CheckClientIpJob.Run(), which is scheduled @every 10s.
Why it matters: The cron chain uses cron.SkipIfStillRunning (see the cron.New chain in internal/web/web.go), so a shaper run that overruns 10s silently causes the fail2ban IP-limit scan to be skipped for those ticks — a speed-limit feature degrading an unrelated security feature, with no log line saying so. On a panel with a few hundred limited clients the initial sync is a few hundred forks.
Recommendation: Batch the commands through tc -batch (one process per sync), or move the shaper onto its own goroutine/schedule so it cannot starve the IP-limit scan.

Severity: Medium / Confidence: High / Category: design
Location: internal/web/service/tc_shaper.go:394
Problem: Shaping is keyed purely on the observed source/destination IP on the host's primary NIC, with no way to distinguish proxied traffic from anything else, and no handling for an IP that maps to more than one client.
Why it matters: Two panel clients behind the same NAT/CGNAT address produce two u32 filters with identical matches at the same prio on the same parent — only one can win, so one client's cap is silently applied to the other's traffic, and the second class receives nothing. The ingress side is worse: conform-exceed drop applies to every packet from that source address, so if an admin's IP happens to also be an observed client IP, their SSH and panel sessions are policed and dropped. Policing (rather than shaping via an ifb device) is also unusually harsh on TCP, since excess packets are dropped rather than queued.
Recommendation: At minimum document these limits in the settings description and the docs page. Better: skip shaping for any IP observed for more than one client, and consider ifb + HTB for the upload direction instead of police ... drop.

Severity: Medium / Confidence: High / Category: documentation
Location: frontend/public/openapi.json:181
Problem: frontend/public/openapi.json gains speedLimitEnable, speedDown and speedUp, but docs/public/openapi.json is untouched and the generated MDX under docs/content/docs/en/reference/api/ was not regenerated.
Why it matters: CLAUDE.md calls this out explicitly, and docs-ci.yml only fires on docs/**, so no CI job catches the omission — the published API reference will be missing the three new fields. docs/public/openapi.json already tracks this schema (it contains restartXrayOnClientDisable), so it is genuinely stale, not out of scope.
Recommendation: Copy frontend/public/openapi.json to docs/public/openapi.json and run cd docs && pnpm gen:api.

Severity: Low / Confidence: High / Category: consistency
Location: internal/database/model/model.go:1317
Problem: The new SpeedDown/SpeedUp blocks in MergeClientRecord pick the smaller non-zero value (incoming.SpeedDown < existing.SpeedDown), whereas the LimitIP block immediately above and the LimitHwid block immediately below both pick the larger. There is no comment explaining the different tie-break.
Why it matters: MergeClientRecord resolves node-sync/import conflicts, and a reader comparing the three adjacent blocks cannot tell whether "most restrictive wins" is intentional here or a copy-paste slip. (The incoming != 0 guard, which means a sync can never clear a limit back to unlimited, is consistent with the existing fields, so that part is fine.)
Recommendation: Add a one-line comment stating the intended rule, e.g. that for a rate cap the lower value is the more restrictive one.

Severity: Low / Confidence: High / Category: validation
Location: frontend/src/schemas/client.ts:227
Problem: speedDown/speedUp have a lower bound (z.number().int().min(0)) but no upper bound, and there is no server-side validation at all. A value tc cannot express produces only logger.Warningf in ensureDownClassLocked/ensureUpPoliceLocked and no shaping. The same silence applies when the ingress qdisc is unavailable (tc_shaper.go:103) — upload limits are simply never applied.
Why it matters: The operator sets a limit, the panel accepts it and shows it saved, and nothing is enforced. There is no signal anywhere in the UI that shaping is unavailable or that a value was rejected.
Recommendation: Add a sane maximum (and matching server-side check), and surface a "speed shaping unavailable on this host" state in the settings/client UI when Init() failed or the ingress qdisc could not be created.

Severity: Low / Confidence: High / Category: convention
Location: internal/web/service/tc_shaper.go:19
Problem: Three comment blocks run to three lines, over the 2-line maximum in CLAUDE.md: tc_shaper.go:19-21, tc_shaper.go:24-26, and internal/database/db.go:346-348.
Why it matters: It is a stated hard rule in the repo guide and no linter enforces it, so review is the only place it gets caught.
Recommendation: Trim each to the two lines that carry the why (the hex-parsing constraint and the 12-bit node-id limit are the load-bearing halves).

Severity: Low / Confidence: High / Category: scope
Location: frontend/src/components/clients/ConfigBlock.css:42
Problem: Two changes are unrelated to speed limits: the color: var(--ant-color-text); addition here, and the deletion of the // Subscribe Telegram notifications for event bus comment in internal/web/web.go.
Why it matters: The PR checklist states "I have no unrelated changes mixed into this PR", and the repo's definition of done asks for a focused diff.
Recommendation: Drop both, or split the CSS fix into its own PR if it is a real bug fix.

Severity: Low / Confidence: Medium / Category: multi-node
Location: internal/web/web.go:670
Problem: speedLimitEnable is read from the local panel's own settings, and shaping is driven by the local Xray online-stats API (XrayService.GetOnlineUsers, internal/web/service/xray.go:905) against the local host's primary NIC.
Why it matters: In a master/sub-node deployment the setting is not synced, so an operator must enable it on every node individually, and the master will not shape traffic that terminates on a sub-node. The PR marks "Multi-node (sub-nodes)" as unaffected, which is true only in the sense that nothing breaks — the feature simply does not apply there unless configured per node.
Recommendation: Add a line to the docs page stating that the setting is per-panel and shaping is local to each node's own interface.

Severity: Suggestion / Confidence: High / Category: testing
Location: internal/web/service/tc_shaper_test.go:36
Problem: The tests are genuinely useful (they drive real diff behaviour and assert exact values, and the handle-recycling test is a good regression guard), but they set s.ready/s.ownIngress directly rather than going through Init(), they are not table-driven with t.Run subtests as CLAUDE.md asks, and they never assert a full emitted argument vector — a wrong flowid, parent or prio would pass every assertion. Nothing exercises syncTcRules/loadClientSpeeds against a throwaway DB (the initSubDB(t) pattern) or migrateClientSpeedLimitColumns.
Why it matters: The argument vectors are the entire contract with tc; the High finding above is exactly the class of bug a golden-args assertion would have caught.
Recommendation: Add one table-driven test asserting the complete tc argv for a down filter, an up filter and a class, including an IPv6 case, plus a DB-backed test for loadClientSpeeds.

Severity: Suggestion / Confidence: High / Category: maintainability
Location: internal/web/service/setting.go:966
Problem: SetSpeedLimitEnable has no caller — settings are persisted through the reflection loop in UpdateAllSetting.
Why it matters: Minor dead code. Noting it only for completeness: SetRestartXrayOnClientDisable is equally unused, so this matches existing precedent and may be deliberate symmetry.
Recommendation: Drop it, or keep it if the paired getter/setter convention is intentional.

---

Positive observations

• The cross-cutting plumbing is unusually thorough for a feature PR: all 13 locale files, both AllSetting and the derived AllSettingView, applyClientRecordMerge (so SyncInbound does not drop the value), the no-inbound update column map, ToRecord/ToClient, the defaults payload, and the regenerated frontend/src/generated/* all line up. The edit modal correctly picks the value up because onEdit hydrates the full record via /get/:email rather than the trimmed ClientSlim row.
• defaultClassMinor = 0x9999 with the hex-vs-decimal comment is a real trap correctly avoided — tc parses both htb default and class minors as base 16.
• TestTcShaperRecyclesFilterHandles drives 3000 sync rounds and asserts every emitted handle stays inside the 12-bit u32 node-id range; that is a genuine regression guard, not a restatement of the code.
• Rolling the shaper into the existing IP-scan job instead of adding a new cron entry keeps the change small, and Sync(nil) when no client carries a limit means an enabled-but-unused feature costs one indexed query per tick.

---

Verdict

Request changes. The hardcoded 800:: u32 hash-table id looks like it will make every filter for the second address family fail on any dual-stack host, silently leaving those clients unshaped, and the unconditional takeover of the root and ingress/clsact qdiscs plus host-global police-index allocation can disturb unrelated system configuration; those three plus the missing Sync(nil) on the Xray-down path are worth resolving before merge. @MHSanaei — the blocking item is internal/web/service/tc_shaper.go:404: IPv4 and IPv6 u32 filters are installed at the same parent and priority with an explicit handle 800::N, which the kernel rejects for whichever family is created second, so on a dual-stack panel roughly half the configured speed limits would silently not apply.

This review was generated automatically; a maintainer may follow up.