Smg RateLimiter Parameters Are Confusing (Possibly Bugs)

[MOHENOO]

Smg RateLimiter Parameters Are Confusing (Possibly Bugs)

// ==================== Rate Limiting ====================
/// Maximum concurrent requests (-1 to disable)
#[arg(long, default_value_t = -1, help_heading = "Rate Limiting")]
max_concurrent_requests: i32,

/// Queue size for pending requests when limit reached
#[arg(long, default_value_t = 100, help_heading = "Rate Limiting")]
queue_size: usize,

/// Maximum time in seconds a request can wait in queue
#[arg(long, default_value_t = 60, help_heading = "Rate Limiting")]
queue_timeout_secs: u64,

/// Token bucket refill rate (tokens per second)
#[arg(long, help_heading = "Rate Limiting")]
rate_limit_tokens_per_second: Option<i32>,

max_concurrent_requests vs rate_limit_tokens_per_second

max_concurrent_requests appears to control concurrency. Based on the token bucket implementation, it should support two modes:

• Semaphore mode: limits concurrency
• Token bucket mode: limits request rate

impl TokenBucket {
    /// Create a new token bucket
    ///
    /// # Arguments
    /// * `capacity` - Maximum number of tokens (burst capacity)
    /// * `refill_rate` - Tokens added per second (0 for pure concurrency limiting)
    pub fn new(capacity: usize, refill_rate: usize) -> Self {
        let capacity = capacity as f64;
        // Allow refill_rate=0 for pure concurrency limiting (semaphore behavior)
        // When refill_rate=0, tokens are only returned via return_tokens()
        let refill_rate = refill_rate as f64;

        Self {
            inner: Arc::new(Mutex::new(TokenBucketInner {
                tokens: capacity,
                last_refill: Instant::now(),
            })),
            notify: Arc::new(Notify::new()),
            capacity,
            refill_rate,
        }
    }
}

Semaphore behavior

fn maybe_rate_limiter(mut self, config: &RouterConfig) -> Self {
    self.rate_limiter = match config.max_concurrent_requests {
        n if n <= 0 => None,
        n => {
            let rate_limit_tokens = config
                .rate_limit_tokens_per_second
                .filter(|&t| t > 0)
                .unwrap_or(n);
            Some(Arc::new(TokenBucket::new(
                n as usize,
                rate_limit_tokens as usize,
            )))
        }
    };
    self
}

During initialization, if rate_limit_tokens_per_second = 0, it falls back to unwrap_or(n), so:
rate_limit_tokens_per_second = max_concurrent_requests

However, based on the token bucket implementation, setting refill_rate = 0 should enable pure semaphore behavior. This path is effectively unreachable. Is this intentional?

Token bucket behavior

if token_bucket.try_acquire(1.0).is_ok() {
    let response = next.run(request).await;

    let (parts, body) = response.into_parts();
    let guarded_body = TokenGuardBody::new(body, token_bucket, 1.0);
    Response::from_parts(parts, Body::new(guarded_body))
}

In a typical token bucket implementation, tokens limit the request input rate. However, returning tokens after request completion makes rate limiting impossible. If this were a semaphore, returning tokens would make sense—but why does a token bucket need this design? This makes max_concurrent_requests extremely confusing to understand.

---

queue_size

let (queue_tx, queue_rx) = mpsc::channel(size);

This suggests queue_size limits the number of queued requests.

However, in practice:

tokio::spawn(async move {
    if token_bucket
        .acquire_timeout(1.0, remaining_timeout)
        .await
        .is_ok()
    {
        let _ = queued.permit_tx.send(Ok(()));
    } else {
        let _ = queued.permit_tx.send(Err(StatusCode::REQUEST_TIMEOUT));
    }
});

Queued requests are processed via tokio::spawn, which allows unbounded async tasks. This seems to bypass the intended queue size limit.

So queue_size does not actually cap the number of waiting requests. This behavior does not match the parameter's meaning.

---

queue_timeout_secs

queue_timeout_secs is applied via:

token_bucket.acquire_timeout(1.0, remaining_timeout)

However, in the underlying implementation:

let wait_time = {
    let inner = self.inner.lock();
    let tokens_needed = tokens - inner.tokens;
    let wait_secs = (tokens_needed / self.refill_rate).max(0.0);
    Duration::from_secs_f64(wait_secs)
};

tokio::time::timeout(wait_time, async {
    loop {
        if self.try_acquire(tokens).is_ok() {
            return;
        }
        tokio::select! {
            () = self.notify.notified() => {},
            () = tokio::time::sleep(Duration::from_millis(10)) => {},
        }
    }
})
.await?;

When refill_rate > 0 (which is always the case, since it cannot be set to 0), the wait time becomes: wait_time = tokens_needed / refill_rate

This causes acquire() to return quickly, instead of waiting for queue_timeout_secs.

Only when refill_rate = 0 does this logic work as expected:

if self.refill_rate == 0.0 {
    loop {
        self.notify.notified().await;
        if self.try_acquire(tokens).is_ok() {
            return Ok(());
        }
    }
}

But since rate_limit_tokens_per_second cannot be set to 0, this mode is effectively unusable.

---

Token leakage when request is dropped

Currently, tokens are returned via TokenGuardBody:

impl Drop for TokenGuardBody {
    fn drop(&mut self) {
        if let Some(bucket) = self.token_bucket.take() {
            bucket.return_tokens_sync(self.tokens);
        }
    }
}

However, in production we have observed cases where:

• A token is successfully acquired
• The request is abruptly closed by the client
• The token is never returned

This leads to token leakage and eventual throttling issues.

---

Summary

In production (using smg + sglang), the rate limiter behavior is difficult to reason about and does not match expectations:

Issue	Expected Behavior	Actual Behavior
Semaphore mode	refill_rate = 0 should enable semaphore	Unreachable because rate_limit_tokens_per_second cannot be 0
Token bucket	Should limit rate, not concurrency	Tokens returned after request, behaves not like common
queue_size	Should bound queued requests	tokio::spawn allows unbounded async tasks
queue_timeout_secs	Should wait up to timeout	Returns quickly when refill_rate > 0
Token cleanup	All acquired tokens should be returned	Tokens leak when requests are dropped

These issues appear more like bugs than intentional design.

Could you clarify whether this behavior is expected, or if there are known issues or planned fixes?

[MOHENOO]

@slin1237

[slin1237]

sorry for the delay. this makes sense. Will be addressed in up coming release

[MOHENOO]

Is there any progress?