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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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