Skip to content

A write guard holds a lock-pool slot for up to 600s, so ~32 concurrent pushes deny every write on the node #282

Description

@beardthelion

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.

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 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.
  • 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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    crate:nodegitlawb-node — the serving node and REST APIkind:bugDefect fix — wrong or unsafe behaviorsev:highMajor break or real security/trust risk, no easy workaroundsubsystem:storageBlob/object store, Arweave, IPFS, archives

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions