Skip to content

Remove ineffective global request limits - #86

Open
behinddwalls wants to merge 1 commit into
tobi:mainfrom
behinddwalls:preetam/issue-78-request-limits
Open

behinddwalls wants to merge 1 commit into
tobi:mainfrom
behinddwalls:preetam/issue-78-request-limits

Conversation

@behinddwalls

@behinddwalls behinddwalls commented Sep 30, 2026 •

Copy link
Copy Markdown

Summary

Why?

The server exposed global concurrency and total request timeout settings without a safe way to apply them. Enforcing them around the whole router would queue health probes behind clones and terminate long streaming Git, LFS, and pack responses mid-transfer.

What?

Remove server.max_concurrent_requests and server.request_timeout, along with the unsafe middleware implementation and misleading documentation. Keep the existing per-repository Git concurrency limit, and reject the removed keys instead of silently accepting unsupported controls.

Test Plan

✅ cargo test -p walgit-config removed_global_request_limit_configuration_is_rejected

✅ cargo check -p walgit-server

✅ cargo clippy -p walgit-server --all-targets -- -D warnings

✅ cargo fmt --all -- --check

✅ git diff --check

Issue

Closes #78

Stack

  1. Remove ineffective global request limits #86 — Remove ineffective global request limits

@0bserver07

Copy link
Copy Markdown
Contributor

I ran this locally. Tests pass but clippy fails on middleware.rs:142 (manual_let_else), so CI would go red.

Two things worried me more though. The limit wraps every route, so /healthz and /readyz have to wait in line behind clones once it's full, which can fail liveness checks under load and keeps the 503 from getting out while draining. And since the permit and the timeout stay on the response body, request_timeout would now cut off long clones and big LFS or packfile downloads halfway through. It was a no-op before, with a 1h default.

Maybe limit only git and LFS work, answer 503 with Retry-After when full, and time out on idle instead of total time? Or just delete the two keys like #78 also suggests. A saturation test would help either way.

## Summary

### Why?

The server exposed global concurrency and total request timeout settings without a safe way to apply them. Enforcing them around the whole router would queue health probes behind clones and terminate long streaming Git, LFS, and pack responses mid-transfer.

### What?

Remove `server.max_concurrent_requests` and `server.request_timeout`, along with the unsafe middleware implementation and misleading documentation. Keep the existing per-repository Git concurrency limit, and reject the removed keys instead of silently accepting unsupported controls.

## Test Plan

✅ `cargo test -p walgit-config removed_global_request_limit_configuration_is_rejected`

✅ `cargo check -p walgit-server`

✅ `cargo clippy -p walgit-server --all-targets -- -D warnings`

✅ `cargo fmt --all -- --check`

✅ `git diff --check`

## Issue

Closes tobi#78
@behinddwalls
behinddwalls force-pushed the preetam/issue-78-request-limits branch from beb9e29 to c7ab81a Compare October 1, 2026 20:17
@behinddwalls behinddwalls changed the title Enforce global request limits Remove ineffective global request limits Oct 1, 2026
@behinddwalls

behinddwalls commented Oct 1, 2026 •

Copy link
Copy Markdown
Author

Removed the unsafe global request controls in c7ab81a while retaining the existing per-repository Git concurrency limit. Applying the controls around the full router would queue health probes behind clones and terminate long streaming Git, LFS, and pack responses, so the unsupported keys are now removed and rejected. The configuration regression test, server check, and clippy pass.

[addressed by agent]

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

server.max_concurrent_requests and request_timeout are not enforced

2 participants