Skip to content

txgovernor: per-channel rate limits are flattened to a single global limit at boot #583

Description

@kotfu

Found while investigating #564. Not a cause of that issue.

Summary

TxTiming stores rate_1min and rate_5min per channel, and the REST API exposes them per channel. The governor applies them globally. At boot, the per-channel values are flattened into a single pair of limits, so a channel configured as unlimited can silently inherit another channel's limit.

Where

pkg/app/wiring.go:435-456:

var rate1, rate5 int
if timings, err := a.store.ListTxTimings(ctx); err == nil {
    for _, t := range timings {
        channelTimings[t.Channel] = txgovernor.ChannelTiming{...}
        if t.Rate1Min > 0 && rate1 == 0 {
            rate1 = int(t.Rate1Min)
        }
        if t.Rate5Min > 0 && rate5 == 0 {
            rate5 = int(t.Rate5Min)
        }
    }
}

ListTxTimings orders by channel (pkg/configstore/store.go:1132), so the effective behavior is: the lowest-numbered channel with a non-zero limit imposes that limit on every channel.

This is not purely a wiring slip. txgovernor.Config has no way to express a per-channel rate limit — Rate1MinLimit and Rate5MinLimit are flat ints, and ChannelTiming, which is per-channel, has no rate fields. isRateLimitedLocked takes a channel and counts per channel (g.rates is keyed by channel), but compares those counts against the global values. So the counters are per-channel and the thresholds are not.

Meanwhile pkg/configstore/models.go:275-287 is explicit about the intent:

// TxTiming holds per-channel CSMA parameters.
...
Channel uint32 `gorm:"not null;uniqueIndex"`
// Rate limits; 0 = unlimited.
Rate1Min uint32 `gorm:"not null;default:0"`

Reproduction

Channel 1 with rate_1min=2, channel 2 with rate_1min=0 (unlimited per the schema comment). Submitting four distinct frames on channel 2:

channel 2 (rate_1min=0, unlimited): submitted 4, transmitted 2, still queued 2

Channel 2 was throttled to channel 1's limit. The excess frames are not dropped, they sit in the queue until the window drains.

Impact

An operator who sets a conservative rate limit on a busy shared channel silently imposes it on every other channel, including ones they deliberately left unlimited. Combined with #580, the effect is worse than throttling one channel: the queued frames sit at the head of the shared queue and block traffic for channels that have no limit configured at all.

A closely related defect in the same lines

While confirming the above I found that no tx-timing change reaches the running governor at all. Governor.SetChannelTiming exists and is safe for live reconfiguration, but it has zero production callers — only pkg/txgovernor/hooks_test.go. POST/PUT /api/tx-timing calls notifyBridgeForChannel → notifyBridgeReload, which kicks the TX backend snapshot and bridge.ReconfigureAudioDevice, neither of which touches the governor (pkg/webapi/server.go:505-520).

Splitting the fields by who owns them:

Field Owner Hot-reloads?
tx_delay_ms, tx_tail_ms Rust modem via ConfigurePtt Yes, via the bridge reload
slot_ms, persist, full_dup governor cfg.Channels No — boot only
rate_1min, rate_5min governor cfg.Rate*MinLimit No — boot only, and flattened

The governor's own comment on ChannelTiming notes that TX delay and tail "live in ConfigurePtt (hot-reloaded by the bridge)", which reads as though the rest of the struct is hot too. It isn't.

This is arguably its own issue rather than part of this one — happy to split it out if you'd prefer it tracked separately. I have kept it here because a fix for the flattening will almost certainly need to decide the reload question at the same time.

Possible direction

Move the rate limits onto ChannelTiming next to the CSMA parameters, since that struct is already per-channel and already has a live setter. That makes isRateLimitedLocked consult the same per-channel record it already looks up, and gives the reload path a single place to write.

Related, all found in the same transmit-path read during #564: #580, #581, #582.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions