Repository navigation
A write guard holds a lock-pool slot for up to 600s, so ~32 concurrent pushes deny every write on the node #282
Description
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_packruns undergit_timeout=git_service_timeout_secs
(default 600), thenguard.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_PUSHdefaults tofalse(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.
Relationship to #174 and #175
#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 entirereceive_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. - Size the pool against the cap that actually binds (see feat(node,git): cap concurrent served git ops with a 503 load-shed (#62) #174) so the two agree
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=false default itself. Both are larger than this
and want their own issues.
Activity