You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Once #279's fix lands, every in-flight repo write pins one connection from a
dedicated pool (GITLAWB_DB_LOCK_POOL_MAX_CONNECTIONS, default 32) for the write's
whole duration. That duration is bounded, but at GITLAWB_GIT_SERVICE_TIMEOUT_SECS,
default 600s. So a caller who can start 32 concurrent pushes holds the node's
entire write capacity for ten minutes at a time, and can sustain it.
This is not a regression in #279's branch. Before that change the lock ran on the
shared 20-connection app pool, so the same pressure starved ordinary reads as
well; the dedicated pool narrows the blast radius to writes. What the change does is
make the ceiling explicit and worth bounding properly.
The arithmetic
Verified against the branch and main:
Pool default 32 (crates/gitlawb-node/src/config.rs, GITLAWB_DB_LOCK_POOL_MAX_CONNECTIONS).
A guard is taken in git_receive_pack (crates/gitlawb-node/src/api/repos.rs), then smart_http::receive_pack runs under git_timeout = git_service_timeout_secs
(default 600), then guard.release(...). So worst-case hold is about 600s.
The only brake on that route is a per-IP rate limit, default 600/hour
(crates/gitlawb-node/src/main.rs, unwrap_or(600)), and GITLAWB_ENFORCE_OWNER_PUSH defaults to false (config.rs:57), so a signed
non-owner reaches it.
Sustaining total write denial therefore costs 32 requests per 600s window, six
windows an hour: 192 requests/hour against a 600/hour budget, about 32%.
Why a rate limit cannot fix this
Little's Law: L = lambda * W. A rate limit pins the arrival rate. It says nothing
about W, the time in system. With W up to 600s, concurrency is unbounded by any rate
cap that permits 32 arrivals, which 600/hour comfortably does. Netflix states the
systems form directly as Limit = Average RPS * Average Latency.
So #196 does not close this. Its brake is a per-IP rate limit, and no rate limit
bounds concurrency on a long-held resource. Please do not close this as covered by
that PR.
Why a per-IP concurrency limit is also the wrong instrument here
The obvious counter-move is a per-IP concurrency cap rather than a rate cap. That
fails on this project specifically, for a reason the codebase already documents: TrustedProxy defaults to None and main.rs calls its own limiter key "a
client-influenced IP". RFC 6269 is explicit that an IP no longer identifies a
subscriber (and that per-IP penalties hit bystanders); MDN is explicit that untrusted X-Forwarded-For yields rate-limiter avoidance. A per-IP concurrency cap would
inherit exactly the rotatable key that already weakens the rate cap, and would read
as a bound while not being one.
#174 introduces GITLAWB_MAX_CONCURRENT_GIT_OPS (default 128) as a global
semaphore that git_receive_pack draws from. That is a genuine concurrency bound and
it is the right shape. It does not close this issue, because 128 is greater than
32: after #174 merges, the 32-slot lock pool is still the binding constraint, and 32
concurrent pushes fit inside the 128-op cap with room to spare.
#175 is the fairness gap inside #174's cap (a single global pool, so anonymous reads
can shed authenticated pushes). Same family, different axis: that one is about who
gets a permit, this one is about how long a permit-holder occupies a database slot.
Direction, not a prescription
The lever is W, the hold time. Options worth weighing, in rough order of how much
they change:
Bound the lock-held portion separately from the whole git op. The guard does not
need to span the entire receive_pack; only the phase that must be exclusive does.
Holding the lock across ref-update and releasing before the slower tail would cut W
by most of its range without capping legitimate large pushes.
Give the lock hold its own timeout, shorter than git_service_timeout_secs. The
simplest change. The tradeoff is that a genuinely slow large push starts failing at
the new bound, so the number needs evidence about real push durations.
Whatever lands, the acceptance test is a hold-time assertion (a guard's lifetime
never exceeds its bound), not a concurrency count. A count test passes while the hold
is unbounded, which is the shape that let this through.
Not in scope here
The refs/gitlawb/** pushability that makes the issue-author fallback untrustworthy,
and the GITLAWB_ENFORCE_OWNER_PUSH=false default itself. Both are larger than this
and want their own issues.
Once #279's fix lands, every in-flight repo write pins one connection from a
dedicated pool (
GITLAWB_DB_LOCK_POOL_MAX_CONNECTIONS, default 32) for the write'swhole duration. That duration is bounded, but at
GITLAWB_GIT_SERVICE_TIMEOUT_SECS,default 600s. So a caller who can start 32 concurrent pushes holds the node's
entire write capacity for ten minutes at a time, and can sustain it.
This is not a regression in #279's branch. Before that change the lock ran on the
shared 20-connection app pool, so the same pressure starved ordinary reads as
well; the dedicated pool narrows the blast radius to writes. What the change does is
make the ceiling explicit and worth bounding properly.
The arithmetic
Verified against the branch and
main:crates/gitlawb-node/src/config.rs,GITLAWB_DB_LOCK_POOL_MAX_CONNECTIONS).git_receive_pack(crates/gitlawb-node/src/api/repos.rs), thensmart_http::receive_packruns undergit_timeout=git_service_timeout_secs(default 600), then
guard.release(...). So worst-case hold is about 600s.(
crates/gitlawb-node/src/main.rs,unwrap_or(600)), andGITLAWB_ENFORCE_OWNER_PUSHdefaults tofalse(config.rs:57), so a signednon-owner reaches it.
windows an hour: 192 requests/hour against a 600/hour budget, about 32%.
Why a rate limit cannot fix this
Little's Law:
L = lambda * W. A rate limit pins the arrival rate. It says nothingabout W, the time in system. With W up to 600s, concurrency is unbounded by any rate
cap that permits 32 arrivals, which 600/hour comfortably does. Netflix states the
systems form directly as
Limit = Average RPS * Average Latency.So #196 does not close this. Its brake is a per-IP rate limit, and no rate limit
bounds concurrency on a long-held resource. Please do not close this as covered by
that PR.
Why a per-IP concurrency limit is also the wrong instrument here
The obvious counter-move is a per-IP concurrency cap rather than a rate cap. That
fails on this project specifically, for a reason the codebase already documents:
TrustedProxydefaults toNoneandmain.rscalls its own limiter key "aclient-influenced IP". RFC 6269 is explicit that an IP no longer identifies a
subscriber (and that per-IP penalties hit bystanders); MDN is explicit that untrusted
X-Forwarded-Foryields rate-limiter avoidance. A per-IP concurrency cap wouldinherit exactly the rotatable key that already weakens the rate cap, and would read
as a bound while not being one.
Relationship to #174 and #175
#174 introduces
GITLAWB_MAX_CONCURRENT_GIT_OPS(default 128) as a globalsemaphore that
git_receive_packdraws from. That is a genuine concurrency bound andit is the right shape. It does not close this issue, because 128 is greater than
32: after #174 merges, the 32-slot lock pool is still the binding constraint, and 32
concurrent pushes fit inside the 128-op cap with room to spare.
#175 is the fairness gap inside #174's cap (a single global pool, so anonymous reads
can shed authenticated pushes). Same family, different axis: that one is about who
gets a permit, this one is about how long a permit-holder occupies a database slot.
Direction, not a prescription
The lever is W, the hold time. Options worth weighing, in rough order of how much
they change:
need to span the entire
receive_pack; only the phase that must be exclusive does.Holding the lock across ref-update and releasing before the slower tail would cut W
by most of its range without capping legitimate large pushes.
git_service_timeout_secs. Thesimplest change. The tradeoff is that a genuinely slow large push starts failing at
the new bound, so the number needs evidence about real push durations.
rather than one silently shadowing the other, and validate the relationship at boot
rather than in prose.
Whatever lands, the acceptance test is a hold-time assertion (a guard's lifetime
never exceeds its bound), not a concurrency count. A count test passes while the hold
is unbounded, which is the shape that let this through.
Not in scope here
The
refs/gitlawb/**pushability that makes the issue-author fallback untrustworthy,and the
GITLAWB_ENFORCE_OWNER_PUSH=falsedefault itself. Both are larger than thisand want their own issues.