From c7ab81ace98ce8a30d2c2546b2e0573c17a6b944 Mon Sep 17 00:00:00 2001
From: Preetam Dwivedi <preetam@uber.com>
Date: Wed, 30 Sep 2026 11:08:09 -0700
Subject: [PATCH] Remove ineffective global request limits
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

## 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
---
 crates/walgit-config/src/lib.rs        | 16 +++++++++++-----
 crates/walgit-server/src/middleware.rs |  6 ++----
 crates/walgit-server/tests/harness.rs  |  1 -
 walgit.example.toml                    |  2 --
 4 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/crates/walgit-config/src/lib.rs b/crates/walgit-config/src/lib.rs
index e3cdaf15..318ae801 100644
--- a/crates/walgit-config/src/lib.rs
+++ b/crates/walgit-config/src/lib.rs
@@ -41,11 +41,8 @@ pub struct Config {
 pub struct ServerConfig {
     pub listen: SocketAddr,
     pub http2: bool,
-    pub max_concurrent_requests: usize,
     /// Per-repo cap on concurrent upload-pack/receive-pack processes.
     pub max_concurrent_per_repo: usize,
-    #[serde(with = "humantime_serde")]
-    pub request_timeout: Duration,
     /// Graceful drain after SIGTERM: new object work (fetch/push/LFS) is
     /// refused with 503 + Retry-After and `/readyz` turns 503 at once; in-flight
     /// requests and the running maintenance unit get this long to finish
@@ -797,9 +794,7 @@ impl Default for ServerConfig {
         ServerConfig {
             listen: std::net::SocketAddr::from(([127, 0, 0, 1], 8080)),
             http2: true,
-            max_concurrent_requests: 512,
             max_concurrent_per_repo: 64,
-            request_timeout: Duration::from_hours(1),
             drain_timeout: Duration::from_secs(20),
             max_push_bytes: ByteSize::gib(64),
             roles: vec![],
@@ -1529,6 +1524,17 @@ mod tests {
         assert!(!base.public_settings_toml().unwrap().contains("[bundles]"));
     }
 
+    #[test]
+    fn removed_global_request_limit_configuration_is_rejected() {
+        for input in [
+            "[server]\nmax_concurrent_requests = 512\n",
+            "[server]\nrequest_timeout = \"1h\"\n",
+        ] {
+            assert!(Config::parse(input).is_err(), "{input}");
+            assert!(Config::default().with_settings(input).is_err(), "{input}");
+        }
+    }
+
     #[test]
     fn removed_compaction_configuration_is_rejected() {
         for input in [
diff --git a/crates/walgit-server/src/middleware.rs b/crates/walgit-server/src/middleware.rs
index 78de0d76..f7b0fb57 100644
--- a/crates/walgit-server/src/middleware.rs
+++ b/crates/walgit-server/src/middleware.rs
@@ -1,7 +1,5 @@
-//! HTTP middleware: request id + tracing span. Request timeout, body limit and
-//! tracing layers are applied in [`crate::router`] via `tower-http`. Per-repo
-//! concurrency limiting lives in the handlers (they hold a repo-keyed semaphore
-//! for the duration of the git operation).
+//! HTTP middleware: request id + tracing span and per-repository concurrency
+//! limits. Per-repository permits live in handlers for the full Git operation.
 
 use std::sync::Arc;
 use std::sync::atomic::{AtomicU64, Ordering};
diff --git a/crates/walgit-server/tests/harness.rs b/crates/walgit-server/tests/harness.rs
index da9ea20f..66273bee 100644
--- a/crates/walgit-server/tests/harness.rs
+++ b/crates/walgit-server/tests/harness.rs
@@ -77,7 +77,6 @@ impl Server {
         cfg.cache.max_bytes = ByteSize::gib(2);
         cfg.server.listen = "127.0.0.1:0".parse().unwrap();
         cfg.server.max_concurrent_per_repo = 8;
-        cfg.server.request_timeout = std::time::Duration::from_mins(10);
         cfg.server.max_push_bytes = ByteSize::gib(2);
         cfg.wal.fsck_objects = true;
         cfg.wal.check_connectivity = true;
diff --git a/walgit.example.toml b/walgit.example.toml
index 79584aa9..87e0a294 100644
--- a/walgit.example.toml
+++ b/walgit.example.toml
@@ -7,9 +7,7 @@
 [server]
 listen = "127.0.0.1:8080"        # default; `mode = none` is refused unless this is loopback. Public bind: 0.0.0.0 with token/oidc.
 http2 = true                      # HTTP/2 (h2c when TLS is off, ALPN when on)
-max_concurrent_requests = 512     # global cap on in-flight git requests
 max_concurrent_per_repo = 64      # per-repo cap (upload-pack / receive-pack)
-request_timeout = "1h"
 drain_timeout = "20s"             # after SIGTERM, phase 2: how long in-flight requests may finish once /readyz is 503
                                   # and new fetch/push/LFS are refused (503 + Retry-After). The running maintenance unit
                                   # is interrupted at once in phase 1 while serving continues.