feat(server): cap concurrent transfers with a 503 backstop - #288
Conversation
There was a problem hiding this comment.
Solid design overall — permit lifetime tied to the transfer stream (not the handler) is correctly implemented for both upload (held across store.write) and download (moved into the inspect closure so it survives until the body is dropped), and error paths release the permit via early ? returns. Good test coverage of the saturation/streaming/zero-disables-cap behavior in saturation.rs.
One blocking issue found in the Helm chart (see inline comment): limits.maxConcurrentTransfers: 0 — the documented way to disable the cap — is swallowed by {{- with }}'s falsy check, so it silently falls back to the server default of 128 instead of disabling it.
Nit: max_concurrent_transfers parsing in config.rs silently defaults to 128 on an unparseable env value with no tracing::warn!, unlike the bytes()/compression()/lookup_budget() helpers in the same file which all log on a bad value. Not a big deal, but worth matching the established pattern for consistency.
SonarQube — aucune nouvelle issueComparaison entre le projet bac à sable de cette PR et la branche par défaut : SonarQube Community n'analyse pas les PR, ce delta est calculé côté CI. Détail |
There was a problem hiding this comment.
Follow-up: both items from my prior review are fixed in 4753892 — the chart now renders LFSX_MAX_CONCURRENT_TRANSFERS=0 correctly instead of swallowing it, and transfer_cap() now warns on an unparseable value with matching tests. No new issues in the changed code. Approving.
Closes #275.
The decision lands as both halves the issue weighed.
The proxy owns request-rate limiting. It sees the client address before anything rewrites it and sheds abuse without spending a connection on the server.
docs/reverse-proxy.mdnow carries a "Rate limiting belongs here" section with nginx (limit_req+limit_conn) and Traefik (rateLimit+inFlightReq) examples, sized around git-lfs opening up to 8 parallel transfers per client (lfs.concurrenttransfers), so a per-address limit below that breaks one legitimate user.The server keeps one backstop: a cap on transfers served at once. The expensive thing on a bare deployment is not a request counted, it is a transfer held open against disk and network.
LFSX_MAX_CONCURRENT_TRANSFERS(default 128,0disables) is a semaphore over uploads and downloads only; batch and locks are never gated. There is no queueing: past the cap the answer is503withRetry-After: 5and rejection causetransfers_saturated, which git-lfs honours by retrying.The permit's lifetime matches the transfer's, not the handler's: an upload holds its slot while the body is still arriving, and a download's permit rides inside the response body stream, released when the client finishes reading or disconnects. Without that, every large download would free its slot at the first byte.
Also in the diff:
lfsx_transfers_in_flightgauge, documented indocs/observability.mdlimits.maxConcurrentTransfers(values, schema, env wiring, README)docs/configuration.mdrow for the new variableserver/tests/saturation.rs: the transfer past the cap gets503+Retry-Afterwhile an upload body is still streaming, then succeeds once the slot frees; a download's unread body keeps holding its slot until dropped; cap0never refuses