feat(node,git): cap concurrent served git ops with a 503 load-shed (#62) - #174
feat(node,git): cap concurrent served git ops with a 503 load-shed (#62)#174beardthelion wants to merge 92 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds configurable global and per-caller concurrency limits for served-Git operations, sheds saturated requests with HTTP 503 responses, and applies timeout-controlled process-group teardown to smart HTTP Git subprocesses and visibility walks. ChangesGit operation hardening
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant GitClient
participant GitHandler
participant AdmissionControls
participant VisibilityWalk
participant SmartHttp
participant GitProcess
GitClient->>GitHandler: Submit smart HTTP request
GitHandler->>AdmissionControls: Acquire global and caller permits
AdmissionControls-->>GitHandler: Permit or overload rejection
GitHandler->>VisibilityWalk: Compute bounded visibility data
VisibilityWalk->>GitProcess: Run Git walk with deadline
GitHandler->>SmartHttp: Run bounded Git service
SmartHttp->>GitProcess: Stream process-group I/O
GitProcess-->>SmartHttp: Output or timeout
SmartHttp-->>GitHandler: Git response or mapped error
GitHandler-->>GitClient: Response or 503 Retry-After
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/gitlawb-node/src/auth/mod.rs (1)
488-524: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider consolidating the duplicated
AppStatetest-builders.This PR had to add
git_semaphorein two near-identical places:make_test_statehere andbuild_stateintest_support.rs. Extracting a single shared constructor (parameterized bynode_did/pool where they differ) would prevent future field additions from needing to be mirrored by hand in both files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gitlawb-node/src/auth/mod.rs` around lines 488 - 524, Consolidate the duplicated AppState test builders by extracting a shared constructor for the common initialization currently duplicated in make_test_state and build_state. Parameterize the helper with differing values such as node_did and the database pool, then update both callers to use it so future AppState fields are maintained in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/gitlawb-node/src/auth/mod.rs`:
- Around line 488-524: Consolidate the duplicated AppState test builders by
extracting a shared constructor for the common initialization currently
duplicated in make_test_state and build_state. Parameterize the helper with
differing values such as node_did and the database pool, then update both
callers to use it so future AppState fields are maintained in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e92738ad-b9fb-4527-926d-a47ad4a19781
📒 Files selected for processing (7)
crates/gitlawb-node/src/api/repos.rscrates/gitlawb-node/src/auth/mod.rscrates/gitlawb-node/src/config.rscrates/gitlawb-node/src/error.rscrates/gitlawb-node/src/main.rscrates/gitlawb-node/src/state.rscrates/gitlawb-node/src/test_support.rs
beardthelion
added
crate:node
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Reserve capacity for authenticated pushes
crates/gitlawb-node/src/api/repos.rs:512
git_info_refsandgit_upload_packare anonymous-reachable, but both consume the samestate.git_semaphorethatgit_receive_packconsumes at line 881. An anonymous client can keep every read slot busy (the normal upload-pack timeout is 600 seconds), which makes a legitimate authenticated push fail at admission with this new 503 before it reaches its auth or owner checks. Please reserve write capacity or split the read and write pools, and add a regression test that holds anonymous-read capacity while verifying that a receive-pack request can still enter. -
[P1] Do not release the cap while its Git process is still running
crates/gitlawb-node/src/api/repos.rs:512
The owned permit is dropped with the handler future, butinfo_refsuses a bareCommand::output()and the filtered upload path reaches uncancellablespawn_blockingwork. A client can start either operation and disconnect repeatedly: each request returns its permit while its Git work continues, so the number of live Git processes can exceed the configured cap and still exhaust PID/CPU resources. The new config documentation explicitly describes this escape hatch. Please keep a slot accounted for until those children are reaped, or give both paths the same cancellation-safe process-group teardown asrun_git_service, with an abort/disconnect regression test.
PR3 of the #62 served-git hardening stack (timeout #165 and teardown wiring #150 are merged). A bounded semaphore caps how many upload-pack / receive-pack / info-refs operations run at once; past the cap a request is shed with a clean 503 + Retry-After before spawning another git subprocess, instead of exhausting the PID/thread table. A permit is acquired at the top of each of the three handlers and held for the whole op, releasing on return. The cap is a portable backstop: the compose pids_limit is absent on Fly, whose 500-connection cap is a different axis. Size --max-concurrent-git-ops (GITLAWB_MAX_CONCURRENT_GIT_OPS, default 128) below the process budget. Range 1..=1_048_576 so 0 (shed everything) and an oversized value that would panic tokio's Semaphore at boot are clean CLI errors. Known gap, tracked separately: info/refs and the withheld-blob (upload_pack_excluding) path are not duration-bounded and do not reap their git child on client disconnect, so a hung git on those two paths holds its slot until it exits and live git can briefly exceed the cap. The main pack path (run_git_service) tears its group down on drop. Tests: Overloaded maps to 503 + Retry-After; the config knob defaults and rejects out-of-range; git_permit sheds at capacity and releases; and each of the three endpoints sheds with 503 when the semaphore is exhausted (load-bearing: drop the permit line and the endpoint test goes red).
Add max_concurrent_git_pushes (default 32) and max_concurrent_reads_per_caller (default 16), both clap range(1..=1_048_576) so an oversized value is a clean CLI error, not a Semaphore::new boot panic. The per-caller knob documents that per-source-IP keying is only as granular as GITLAWB_TRUSTED_PROXY. Wiring lands in the following commits; these are the config surface for the #174 concurrency-fairness fix. Resolves jatmn P1a/P1b groundwork on #174.
git-receive-pack now draws from a separate git_write_semaphore (max_concurrent_git_pushes) instead of the shared pool, so a flood of anonymous reads can no longer shed an authenticated push at admission (jatmn P1a). The shared field is renamed git_read_semaphore and continues to gate upload-pack and both info/refs advertisements. The write permit stays above acquire_write so it precedes the Tigris fresh-acquire (INV-10). Handler-layer tests: write-pool shed (503), and a cross-boundary proof that an exhausted read pool does NOT shed a push; both mutation-checked (routing receive-pack back to the read pool flips each RED). 497 tests pass. Part of #174.
Adds PerCallerConcurrency, a bounded-keyed in-flight limiter (distinct from the request-rate RateLimiter) so no single caller monopolizes the served-git read pool. Each caller (per-DID when signed via optional_signature, else per-source-IP via client_key) may hold at most max_concurrent_reads_per_caller concurrent reads; over that it sheds 503. The key map is self-bounding (a key is dropped when its in-flight count hits zero) with a reject-before-insert max_keys backstop so a key farm can't grow it (INV-15). Applied in git_upload_pack and both info/refs advertisements, acquired after the visibility gate so a denied request never consumes a slot (KTD7). Primitive unit-tested (cap + self-bounding + reject-before-insert) and mutation-checked. Handler-layer SC2: same-caller sheds while a different caller passes, proven on BOTH git_info_refs and git_upload_pack with independent mutation probes; plus a None-key bypass test. Per-source-IP keying is trust-config dependent, documented on the config knob. 502 tests pass. Part of #174.
info_refs ran a bare Command::output() with no timeout and no process-group teardown, so a hung git pinned its concurrency slot indefinitely and a client disconnect orphaned the child (jatmn P1b). Extract the timeout + process_group(0) + KillGroupOnDrop core from run_git_service into a shared drive_git_child, and route info_refs through it with an injectable git_bin. A hung advertisement now aborts with GitServiceTimeout (mapped to 504); disconnect reaps the group. run_git_service's teardown tests all pass through the shared core (proving the group teardown info_refs inherits), the real-git filter tests cover the advertisement happy path, and a new watchdog-bounded test proves a hung advertisement times out. 503 tests pass. Part of #174.
The filtered-pack path ran the whole rev-list + pack-objects build inside a spawn_blocking, so an outer tokio timeout could not cancel the blocking thread and a client disconnect orphaned the git child while the permit freed (jatmn P1b, the second gap path). Split it: rev-list enumeration stays blocking off the runtime (rev_list_keep), but the streaming pack-objects stage now runs under the shared drive_git_child on the async side, so it is duration-bounded (GitServiceTimeout -> 504) and its process group is reaped on disconnect. build_filtered_pack becomes async and takes a git_bin seam + timeout; upload_pack_excluding threads the git_service_timeout through. A watchdog-bounded test proves a hung pack-objects times out (rev-list fast, pack-objects hangs). The refactor's happy path is covered by the existing filtered-pack correctness and real-git partial-clone/fetch tests, all still green; disconnect/group-teardown is the shared drive_git_child code proven by the run_git_service tests. 504 tests pass. Part of #174.
#62) The max_concurrent_git_ops and git_service_timeout_secs doc-comments (and .env.example) described the info/refs and withheld-blob paths as unbounded follow-up gaps. Both are now closed (#174): the comments reflect the read/write pool split, the per-caller sub-cap, and that every capped path is duration-bounded with process-group teardown. Verified the pattern-doc pre-ship checklist: every git_permit / write-permit / per-caller site holds only a timeout+teardown git path, and all three size knobs are range(1..=1_048_576). Closes the #174 work. No behavior change.
Review of the served-git concurrency cap found no P0/P1; these are the verified P2 follow-ups. config: the max_concurrent_git_ops doc overclaimed that "every capped path is duration-bounded." The rev-list object enumeration in the withheld-blob path still runs in an uncancellable spawn_blocking, so a stuck rev-list can hold its slot until git exits. Scope the guarantee to the streaming stages and name the residual. Also tighten the fairness claim: the receive-pack advertisement shares the read pool (a shed advertisement is a cheap retryable GET); only the push POST is on the isolated write pool. api/repos: add info_refs_per_caller_cap_keys_on_did_not_ip, the missing handler proof that a signed caller is keyed by its DID, not its source IP. Filling the DID slot sheds a request from a free IP; collapsing read_caller_key to its IP arm turns the assertion green-not-503 (mutation-verified RED). api/repos: extract acquire_read_caller_permit so both read handlers share one shed path instead of a duplicated match block. rate_limit: recover from a poisoned PerCallerConcurrency mutex instead of panicking. The critical section is pure counter arithmetic and cannot poison the lock, but a panic there would brick the limiter for every caller. 505 tests pass; clippy -D warnings and fmt clean. Part of #174.
88b8870 to
5069cd1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/gitlawb-node/src/git/smart_http.rs (1)
352-412: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winApply the timeout to
rev-listas well.Line 362 still uses blocking
Command::output(), so a hungrev-listsurvives cancellation and holds the endpoint’s concurrency permit indefinitely. The new test only exercises a fastrev-list, leaving this failure mode uncovered.Run both stages through
drive_git_childusing one deadline and add a hung-rev-listregression test.Also applies to: 438-448, 1292-1329
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gitlawb-node/src/git/smart_http.rs` around lines 352 - 412, Apply the same timeout deadline to both rev-list and pack-objects: replace rev_list_keep’s blocking Command::output path with drive_git_child, preserving injectable git_bin and filtering withheld OIDs from rev-list output before packing. Compute one deadline or remaining timeout and ensure cancellation reaps either child process, including when rev-list hangs. Update build_filtered_pack and related callers/tests accordingly, and add a regression test using a hung rev-list fixture to verify timeout and permit release.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.env.example:
- Around line 112-130: Add a GITLAWB_MAX_CONCURRENT_GIT_OPS example entry to
.env.example near GITLAWB_MAX_CONCURRENT_GIT_PUSHES, including a concise
description and the intended default value, so the general Git operation
concurrency setting is discoverable alongside the related push and read limits.
---
Outside diff comments:
In `@crates/gitlawb-node/src/git/smart_http.rs`:
- Around line 352-412: Apply the same timeout deadline to both rev-list and
pack-objects: replace rev_list_keep’s blocking Command::output path with
drive_git_child, preserving injectable git_bin and filtering withheld OIDs from
rev-list output before packing. Compute one deadline or remaining timeout and
ensure cancellation reaps either child process, including when rev-list hangs.
Update build_filtered_pack and related callers/tests accordingly, and add a
regression test using a hung rev-list fixture to verify timeout and permit
release.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9efa7936-8b5f-4746-923b-095bfef2df03
📒 Files selected for processing (10)
.env.examplecrates/gitlawb-node/src/api/repos.rscrates/gitlawb-node/src/auth/mod.rscrates/gitlawb-node/src/config.rscrates/gitlawb-node/src/error.rscrates/gitlawb-node/src/git/smart_http.rscrates/gitlawb-node/src/main.rscrates/gitlawb-node/src/rate_limit.rscrates/gitlawb-node/src/state.rscrates/gitlawb-node/src/test_support.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/gitlawb-node/src/main.rs
- crates/gitlawb-node/src/error.rs
- crates/gitlawb-node/src/test_support.rs
|
Both P1s are resolved on the new head (5069cd1). P1a (reserve push capacity). Split the pool. P1b (don't free the slot while its git runs). Extracted the One residual I'd rather name than bury: the 505 tests pass; clippy |
The read-pool knob was referenced by the push and per-caller entries' comments but had no example line of its own, so operators couldn't discover it from the template. Add it with the config default (128).
…ng enumeration can't pin a git permit (#174)
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Reserve capacity for the receive-pack advertisement as well
crates/gitlawb-node/src/api/repos.rs:512
A push starts with the signedGET /info/refs?service=git-receive-packbefore itsgit-receive-packPOST, but this handler always acquiresgit_read_semaphore. Consequently, an anonymous clone/read flood can exhaust the read pool and return 503 to the push during its required advertisement phase, before it can reach the new write semaphore. The existing isolation test exercises only the POST, so it misses the protocol-level path. Put receive-pack advertisements behind capacity that reads cannot consume (or otherwise reserve an end-to-end push path) and add a full-handshake regression. -
[P1] Keep filtered-upload Git work inside the timeout and concurrency lifecycle
crates/gitlawb-node/src/git/smart_http.rs:402
rev_list_keepis launched throughspawn_blockingand uses a bareCommand::output()without either the configured deadline or process-group teardown. The preceding withheld-blob classification walk has the same pattern inapi/repos.rs:762. If either stage stalls, it can hold a read slot indefinitely; if the client disconnects, the handler drops its permits while Tokio continues the blocking task and its Git child. Repeating that path-scoped fetch can therefore exceed the configured live-Git cap and exhaust processes/threads. Run all of these children under cancellation-safe, deadline-bounded management (or retain admission until they are reaped), and cover hung/disconnect cases for both enumeration stages. -
[P1] Do not let disposable signed DIDs bypass the per-source read cap
crates/gitlawb-node/src/api/repos.rs:647
read_caller_keydiscards the source-IP key whenever an optional signature is present, even though public read routes accept any validdid:keysignature without an admission/registration step. A single host can mint eight DIDs and hold 16 slots under each at the defaults, filling the 128-slot read pool while the same host would be capped at 16 when unsigned. Enforce a non-farmable source budget alongside (or instead of) the DID budget, and add a multi-DID/same-peer regression. -
[P2] Update the operator timeout documentation
README.md:346
The README still saysGITLAWB_GIT_SERVICE_TIMEOUT_SECSdoes not boundinfo/refs, but this PR routes that operation throughdrive_git_childwith the configured timeout. This contradicts the updated.env.exampleand config help, so operators are left with inaccurate deployment guidance. Update the table entry to describe the current coverage and the remaining filtered-enumeration limitation precisely.
…ID (#174) read_caller_key returned the authenticated DID when a caller signed, dropping the source-IP key. Public read routes accept any valid did:key via optional_signature with no admission step, so one host could mint N disposable DIDs and hold max_concurrent_reads_per_caller slots under each, multiplying its budget N-fold and filling the global read pool, while the same host unsigned was capped on its IP. Key the read sub-cap on the resolved source IP for every caller, signed or not, mirroring the push path's IpRateLimiter which already throttles on source IP for this exact DID-farm reason. Drops the now-unused caller_did parameter at both call sites (git_info_refs, git_upload_pack). Inverts info_refs_per_caller_cap_keys_on_did_not_ip into info_refs_per_caller_cap_keys_on_ip_not_did: fill one source IP's slot, then two requests signed under different DIDs from that same IP both shed 503 (farm defeated), while a signed request from a different IP keeps its own budget. RED on the DID-keyed tree, GREEN after.
) git_info_refs acquired git_read_semaphore for BOTH services, so the push handshake (GET /info/refs?service=git-receive-pack) competed in the global read pool. An anonymous clone flood could exhaust that pool and shed a legitimate push with 503 during its required advertisement phase, before it ever reached git_write_semaphore on the POST. The write pool exists precisely so anonymous reads cannot shed an authenticated push, but only the POST drew from it. Select the pool by service: the receive-pack advertisement (phase one of a push) now draws from git_write_semaphore, like the git-receive-pack POST, so a saturated read pool cannot starve it. The per-IP push_rate_limiter that already brakes the advertisement stays as the anti-flood control, and the advertisement stays reader-visible with no new auth requirement. Because the receive-pack branch is now a write-path op, it no longer consumes a read per-caller slot. Handler-layer proofs: with the read pool at zero the receive-pack advertisement survives while the upload-pack advertisement sheds; with the write pool at zero the receive-pack advertisement sheds while upload-pack is unaffected; and a receive-pack advertisement from an IP whose read per-caller budget is full still gets through (mutation-checked, RED when the skip is neutralized).
…#174) The withheld-blob classification walk (blob_paths) fanned out blocking git children with no deadline and no process-group teardown: git for-each-ref, git cat-file, git rev-list, a git ls-tree per commit, and an uncounted git rev-parse (via store::head_commit). A hung or pathologically slow child pinned the caller's served-git permit for the whole hang, and on client disconnect the spawn_blocking task and its git children ran on, orphaned. blob_paths is the shared core of five callers: the upload-pack serve path (holds a read permit) AND, inside git_receive_pack, the post-push replication and encrypt-then-pin walks (hold the write permit U2 reserves for pushes). So the same unbounded walk could pin either pool, and leaving the write-side twin unbounded would have made U2's reservation a claim that does not match behavior. Bound every git child at the blob_paths spawn seam on the blocking side: each child runs in its own process group with a watchdog thread that SIGTERMs (then SIGKILLs) the group on one shared deadline spanning the whole walk, and retains admission until the group is reaped. This is the blocking-side counterpart of smart_http::drive_git_child (spawn_blocking cannot be cancelled by an async timeout). blob_paths stays sync, so all five callers keep their signatures and the 32 classification tests are unchanged; because every caller funnels through blob_paths, one seam bounds both the serve and replication paths. The previously unbounded store::head_commit child becomes a bounded git rev-parse inside the walk. A walk that hits its deadline carries GitServiceTimeout, which the serve handler now maps to 504 rather than a generic 500. Proof: a fake git that hangs on rev-list makes blob_paths return GitServiceTimeout within the watchdog budget (not block on the child) and the recorded process-group leader is reaped, not orphaned; neutralizing the watchdog kill makes it hang past the budget (RED). The 32 real-git classification tests stay green through the refactor, including detached-HEAD, non-standard-ref, and deleted-in-history cases.
GITLAWB_GIT_SERVICE_TIMEOUT_SECS bounds the info/refs advertisement too: smart_http::info_refs drives it through drive_git_child under this timeout, with a passing test proving the 504. The old note claimed it does not. It also claimed the withheld-blob path is unbounded; after the blob_paths seam bound (this PR) the walk is bounded and reaped, by a fixed internal deadline rather than this env var, so the line now states that precisely instead of overclaiming this setting covers it.
Code review found run_bounded_git's watchdog could return a spurious 504 and signal a recycled process group. The watchdog runs off a wall clock on its own thread; done_tx.send() only fires after child.wait() reaps the leader, so a walk that finished within microseconds of the deadline took the watchdog's Timeout branch, discarded a fully-captured successful result, and returned GitServiceTimeout (a 504 for a walk that actually completed). Worse, the Timeout branch SIGTERMed -pgid unconditionally after the leader was reaped, so a recycled pgid could be signalled, the exact hazard smart_http guards via disarm-after-wait. Set a reaped AtomicBool the instant the main thread reaps the child; the watchdog checks it before every kill and stands down if the leader is already reaped. Gate the timeout verdict on !status.success(), so a child that exited on its own is never reported as a timeout even if the watchdog fired late. Add the survived-SIGKILL warn smart_http's reap already emits, for operator visibility on a wedged (D-state) git. The hung-walk test stays green (a killed child exits by signal, not success, so it still surfaces GitServiceTimeout and reaps the group) and the 32 real-git classification tests stay green (a fast walk is never spuriously killed).
…nnot starve the write pool (#174) U2 moved the receive-pack info/refs advertisement onto git_write_semaphore to keep an anonymous read flood from starving the push handshake. But the advertisement is anon-reachable on public repos and holds its write permit across the slow acquire_fresh Tigris download, and the only per-source brake on it was the push RATE limiter, not a concurrency cap. So a multi-source flood of receive-pack advertisements could hold the write pool's slots across those downloads and shed authenticated pushes (both the advertisement and the owner-gated git-receive-pack POST draw from the same pool). U2 thus introduced the first anonymous consumer of the write pool the state doc promised anon could never reach; the plan's residual note (no worse than the POST) was wrong, because the POST is owner-gated and the advertisement is not. Add git_push_advert_per_caller, a per-source concurrency sub-cap on the receive-pack advertisement keyed on the resolved source IP (the same PerCallerConcurrency mechanism U1 uses for reads), sized to an eighth of the write pool so a single source holds at most that share and saturating the pool takes many distinct source IPs, each also braked by the per-IP push rate limiter. The upload-pack advertisement keeps its read-pool per-caller cap; the owner-gated POST is unchanged. Correct the state doc for git_write_semaphore accordingly. Handler-layer proof: a source at its receive-pack advertisement cap sheds 503 (RED before the acquisition, 500-not-503), while a different source and the upload-pack advertisement are unaffected. Full suite 510 green.
…d timeout (#174) Close the reasoned-not-run gaps from the code review by making the walk's git binary and timeout injectable, then driving the missing branches with a real handler and a fake git instead of reasoning about them. - Add state.git_bin and *_bounded variants of the walk entry points taking (git_bin, timeout); the served handlers (upload-pack serve, receive-pack replication and full-scan and encrypt-pin, and the ipfs gate) now pass the operator-configured GITLAWB_GIT_SERVICE_TIMEOUT_SECS, so the whole walk is bounded by the same budget as the other served-git ops rather than a fixed constant. The git_bin-less wrappers stay for the real-git classification tests. Newly vetted by execution (not reasoning): - receive-pack replication path is bounded: replication_withheld_set with an injected hung git returns within the budget and fails closed, so it cannot pin the write permit git_receive_pack holds across it. - a hung withheld-blob walk on the upload-pack POST returns 504 (real handler, real repo on disk, injected hung git), proving the GitServiceTimeout -> git_service_app_error wiring end to end. - the watchdog status-gate: a child that exits successfully is not reported as a timeout even when the watchdog fired (mutation-checked: drop the guard -> RED). - SIGKILL escalation: a SIGTERM-ignoring child is still reaped via SIGKILL and the group is gone; a truly uninterruptible D-state child (unreapable by any signal) is the documented residual, matching the async teardown. - the advertisement per-source cap sizing never derives 0. Full gitlawb-node suite 515 green.
…a fixed const (#174) Follow-up to threading the configured timeout into the walk: the walk now honors GITLAWB_GIT_SERVICE_TIMEOUT_SECS on both the serve and replication paths, so the README no longer says a fixed internal deadline.
…erve (#62) The path-scoped git-upload-pack branch spent git_service_timeout_secs twice: once on the withheld-blob classification walk, then a second full budget on the serve. One clone could hold a read-pool permit for ~2x the configured budget, which is the occupancy hole this PR otherwise exists to close. Thread one Instant deadline through both phases, mirroring fail_closed_full_scan_objects and build_filtered_pack: the walk runs against the remainder and the serve gets what the walk left, computed after the blocking task's await so it cannot pick up a fresh budget. A walk that consumes the whole budget saturates the serve to zero, which surfaces as GitServiceTimeout -> 504 rather than running unbounded. Under-serving one slow clone is the safe direction versus over-holding the pool. Both serve arms are covered because a fix that reached only the plain arm would leave the ~2x hold reachable by any clone of a repo that actually withholds a blob. Each test was observed RED before the change (status 200, the serve completing on a fresh budget) and GREEN after, and reverting the filtered arm alone flips only its own test, so neither guard is a proxy for the other. The knob's meaning changed for this path, so .env.example now says the value bounds the two phases combined rather than each separately.
…ed caps (#62) Four prose corrections on this PR's surfaces, no behavior change. The receive-pack advertisement no longer draws from the write pool, it draws from a dedicated advert pool, but five comments still described the old shape: the info/refs per-source block, the cap construction in main.rs, the State field doc, and a test doc plus its assertion message. The state.rs and main.rs ones mattered most because they contradicted the operator text being written in this same commit. The /ipfs probe and content-read subprocesses each run under their own deadline now (the lesser of git_service_timeout_secs and the remaining budget, reaped by process-group teardown), but four surfaces still claimed they had no duration bound past the pre-start budget check. The correction stays scoped rather than declaring the route safe: the probe's object_store_readable check is a synchronous filesystem sweep with nothing to reap, so a wedged filesystem can still hold the walk slot past the deadline, and that residual is now stated where the old warning used to be. A grep for the full phrase misses two of these surfaces, since one wraps across doc-comment lines and one says "duration clamp" instead, so the sweep uses the fragment. Both push-side per-source concurrency caps are derived as max_concurrent_git_pushes / 8 with a floor of 1 and have no environment variable, so neither was documented anywhere an operator reads. The write-side one is load-bearing for push availability: it is acquired before the global write permit, and owner enforcement defaults off, so without it one host minting disposable did:key identities could monopolize the write pool. They are described in .env.example beside the read cap rather than in README's table, which is keyed by variable name. The hung-walk assertion in visibility_pack printed {err}, showing only the top anyhow context and dropping the underlying io error. A beta-lane failure surfaced as "failed to spawn git for-each-ref" with no errno, which made it undiagnosable. {err:#} prints the chain. The underlying flake is still undiagnosed: it did not reproduce in 123 local runs and the stable lane passed on the same commit, so this only improves the next occurrence.
…hanges (#62) Two table rows needed correcting for the changes in this PR: the GITLAWB_GIT_SERVICE_TIMEOUT_SECS row now says the classification walk and the pack serve share one deadline on the path-scoped upload-pack path, so the value bounds them combined; and the GITLAWB_IPFS_REQUEST_BUDGET_SECS row no longer claims the probe and content-read subprocesses are unbounded, while keeping the object-store-readability sweep named as a live residual. The other thirteen changed lines are punctuation only, no wording changes: em dashes replaced with colons, commas, or sentence breaks, and one pair of curly quotes straightened. The repo's prose-style hook validates a whole file rather than a diff, so these pre-existing characters were rejecting every edit to README.md, including the two rows above.
… deadline (#62) The shared deadline computed the walk's budget on the async side, before the task was queued on the blocking pool. withheld_blob_oids_bounded starts its own clock when it actually runs, so a budget measured at queue time handed the walk a full budget from whenever the pool got to it: the read permit was held for queue delay PLUS the budget, and only the serve phase saw the outer deadline already spent. Computing the remainder inside the closure charges that delay against the shared deadline, which is what makes the ~1x bound in the docs true rather than approximate. Not a regression from the previous commit (the old code passed a constant git_timeout, so the delay went uncharged there too, and the combined hold was queue delay plus two full budgets), but the claim this PR now prints in README and .env.example should be exact. The two shared-deadline tests still pass and still fail on revert; they cover the budget arithmetic, not the queue-saturation path. Reproducing that needs a runtime with the blocking pool pinned and parked, which the sqlx::test harness does not give a seam for, so the queue-delay path is reasoned, not executed.
|
All four addressed, plus one item I added and one a follow-up pass turned up. New head [P3] Shared deadline across the path-scoped walk and serve. Done in One correction on the baseline, since it changes whether this belonged in this PR. Your note said [P3] Stale [P3] [P3] Derived authenticated-push per-source cap. Documented in Added by me: the hung-walk assertion in
The README diff also normalizes punctuation on thirteen unrelated lines (long dashes to colons or commas, one quote pair straightened). No wording changes. |
) The absent-CID path disambiguates a clean `missing` by re-running the probe, but the re-probe took the same deadline with no check that any budget was left. A first probe that nearly exhausted the budget still spawned a second child that could only be reaped, and the watchdog's SIGTERM grace plus SIGKILL settle carried the whole call to roughly twice the budget. Measured on object_type_bounded with a 1s budget and a probe that burns 0.9s of it: two spawns and 2021ms elapsed. That is the same ~2x occupancy shape this PR exists to remove, on a different handler, and it is worse here than on the served-git path: /ipfs/{cid} carries only optional_signature, and an absent CID drives this branch once per repo, so an unauthenticated caller spraying well-formed absent CIDs pays the overshoot per repo. It also degraded an honest absence into a retryable 503. Re-probe only when the remaining budget covers what the first probe actually took, since the re-probe runs the same command. Below that, taint to Transient rather than spawn: an inconclusive disambiguation is not an absence verdict, so it must not become a false 404 either. The same 1s case is now one spawn and finishes inside the budget. Both introduced by this PR (d39fc8d, the F5 disambiguation); neither object_type_bounded nor the re-probe exists at the merge base, so this is not a pre-existing condition. The companion test pins that an ample budget still re-probes, so the check cannot silently disable the disambiguation it guards.
|
One more commit,
So roughly twice the budget on the last row, and an honest absence degraded into a retryable 503. Worse here than on the served-git path you flagged: Mine, and recent: neither The fix re-probes only when the remaining budget covers what the first probe actually took, since the re-probe runs the same command. Below that it taints to Two things worth separating out. I also chased a claim that turned out to be wrong, and I am recording it so it does not get raised later as though it were open. I thought Separately, the absent path spawns two |
Brings the branch onto post-#243 main so cargo metadata --locked passes against the committed tree.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
PR #174 review draft (Gitlawb/node)
Head: 4b69c450070c20d30244f25871a0c1c6632e6947 · Merge-base with main: 111cff7e203c698c06a1f9f22a2ffe7ecc474f4d · main tip: c83cbc56e05324b314b5a83082b4fc756190a1b2 · PR: #174
Thanks for the 4f3278f / 28a6ca4 / 4b69c45 follow-up — I rechecked current head against current main and reran the full review campaign.
Your prior open P3s on a9d8906 look addressed on this head: the path-scoped upload-pack walk and serve share one deadline (with blocking-pool queue delay charged inside the walk closure), the receive-pack advert comment and /ipfs probe/read duration docs match the bounded implementation, and the derived push per-source caps are documented in .env.example. I am not re-raising those.
I am also not re-raising the previously accepted residuals (D-state release-without-ESRCH, object_store_readable unbounded FS sweep, Tigris-upload vs detached-tail ordering, coalesce/panic loss with no sweep, non-Unix best-effort teardown, /ipfs probe/read hardcoding "git").
Findings
-
[P1] Rebase onto current
mainbefore further review —repo_store.rsconflicts with #272 path-containment fixes
Merge-base is still111cff7e; currentmainisc83cbc56.git merge-treereportscrates/gitlawb-node/src/git/repo_store.rschanged in both. Onmain, #272 addedpath_within_root/Containment, the shared slug validator, and the NAME_MAX owner bound. This head rewrites the same file for connection-affine advisory locks and cancellation-safeRepoWriteGuardand does not contain those #272 surfaces. Merging as-is will conflict or risk dropping the path-escape gates. Please rebase, preserve both the #272 containment/slug work and this PR's lock/Drop/timeout wiring, then re-request review of the resolved diff (includingCargo.lock/ CI--lockedsync that landed onmainafter the merge-base). -
[P2] Dispose the write-guard connection when the Drop-path unlock errors
crates/gitlawb-node/src/git/repo_store.rs:568-576
RepoWriteGuard::releasecloses the connection on an erroredpg_advisory_unlockso a locked session cannot return to the pool (F3b). The cancellationDropbackstop still only logs on unlockErrand then drops thePoolConnection, which returns it to the pool while the session-level advisory lock may still be held — the same failure modereleasedocuments (statement timeout / aborted transaction / admin cancel with a live session). Please mirrorclose_conn_boundedon the Drop unlock error path (and on the off-runtimeErrarm that currently drops the connection without unlocking), with a RED/GREEN test parallel to thereleaseF3b cases.
Prior round — verified on 4b69c450
Shared upload-pack deadline. Path-scoped branch uses one deadline for withheld_blob_oids_bounded and the serve; walk budget is derived inside the spawn_blocking closure (repos.rs:1374-1420).
/ipfs absence re-probe budget gate. object_type_bounded skips an unaffordable confirming re-probe and taints Transient (store.rs:457-472). absent_probe_skips_a_reprobe_it_cannot_afford and absent_probe_still_reprobes_when_the_budget_allows both pass locally.
Docs for prior P3s. Advert-pool comment, /ipfs probe/read duration text, and derived push per-source caps in .env.example match the implementation.
Accepted residuals (not re-raised)
- D-state / ~4s hard-cap release without confirmed group reap;
object_store_readableunbounded FS sweep; Tigris upload vs detached-tail ordering; coalesce/panic pending loss with no reconciliation sweep; non-Unix best-effort teardown;/ipfsprobe/read using"git"while the walk usesstate.git_bin; deliberate per-push Pinata tasks with pin-pool defer. - Overlap with open #276 (quarantine on
/ipfs) and #285/#279 (advisory-lock session affinity onmain): this head already carries the connection-affine unlock design, but must be rebased through #272'srepo_storechanges rather than merged over them.
…ish (#174) pin_new_objects runs under a pin_semaphore permit and that pool defers rather than sheds. Both sinks in ipfs_pin.rs built a bare reqwest::Client::new(), which carries no timeout, so a silent endpoint parked the pool indefinitely. Both now use the shared no-redirect client from build_http_client() through a module-local OnceLock, rather than the hand-rolled twin its docstring forbids. The client alone does not bound the hold: without a ceiling the loop is O(N) with N chosen by the pusher. A 120s batch budget is taken once at loop start and gated at the top of every iteration, so no object's work begins with zero budget left. The remainder is also passed to each add as a per-request timeout, which is what lets one large healthy upload run past the client's 10s default without letting the batch run forever. RequestBuilder::timeout replaces the client value for that request only and leaks nothing to other callers, verified by execution against the pinned reqwest. An earlier version of this commit classified a dead endpoint by reqwest::Error::is_timeout() || is_connect() and broke the batch. That was wrong in both directions and is gone. It missed the ordinary dead-peer shapes, since a peer closing mid-request yields IncompleteMessage and a peer RST mid-upload yields ConnectionReset, both observed with is_timeout and is_connect false, so a load-balancer idle timeout evaded it entirely. And it fired on a slow but alive endpoint, because .timeout() is a total deadline covering body upload, so a healthy multi-megabyte add was read as a dead endpoint and abandoned the rest of the push. A wall-clock bound needs neither judgement. The docstring now states what is bounded and what is not, rather than implying the hold is closed. Not bounded: the git read, since store::read_object shells two bare Command::output() calls with no timeout or process-group reaping and this loop does not run it under spawn_blocking; the DB round-trips; and the pool itself, since the Pinata replication task holds the same semaphore across a full git re-derivation with no deadline. Truncation is stated plainly: a batch stopped at the deadline leaves the rest unpinned and nothing sweeps them up, so recovery is opportunistic through a later full-scan push. Tests written first, both RED by assertion rather than by a harness timeout: pin_new_objects_stops_the_batch_at_its_deadline ... FAILED the batch must stop partway, not pin all five and not stall on the first: pinned 5 pin_new_objects_does_not_abandon_the_batch_on_a_slow_but_alive_endpoint ... FAILED a slow but progressing endpoint must pin both objects: an upload past the client's 10s default is not a dead endpoint left: 1, right: 2 Both guards mutation-proved load-bearing against present-but-wrong mutants, each red on its own message: the loop-top break widened to continue, and the per-request timeout replaced with None. Also bounds encrypted_pin.rs's pin_git_object call, where a hang wedged a repo's EncryptInflight key. Its Drop backstop covers unwind, not hang. Full suite 730 passed, 0 failed. fmt and clippy clean.
The walk's deadline clamp was computed on the async side, before spawn_blocking was called, then used inside the closure. Time spent queued for a blocking thread went uncharged, so the walk ran with a budget larger than the request's true remainder. The clamp is now derived inside the closure at task start. This is the sibling of the upload-pack site fixed in 28a6ca4, and unlike that one it sits on the anonymously reachable route. A queue delay that eats the whole remainder saturates the clamp to zero and the bounded walk fails closed through the existing taint arm, no verdict, which is the safe direction. Testing gap, recorded at the site rather than papered over: the queue-delay path is reasoned, not executed. Observing it needs a runtime with the blocking pool pinned and parked, and the sqlx::test harness gives no seam for that. 28a6ca4 shipped with the same gap for the same reason, verified against its diff and message rather than assumed. The tests in this file saturate the admission semaphores, never the blocking pool, so there was nothing to reuse. An assertion on the arithmetic would only restate the implementation. cargo test ipfs::: 24 passed, 0 failed.
…#174) separator_prevents_the_owner_name_boundary_collision asserted that ("a","b_c") and ("a_b","c") produce different keys. Those differ under any join at all, bare included (ab_c against a_bc), so deleting the separator from repo_identity_key left the test green. It could not fail for the reason it named. The inputs are now ("a","bc") against ("ab","c"), both abc under a bare join. The test docstring said the separator was \0; the code joins with /. Corrected. The docstring on repo_identity_key itself carried the same wrong collision example, so it is corrected too. Proven by mutation rather than argued, joining with no separator: old inputs, injected, re-running ... still green VACUOUS the guard's own defect was injected and nothing failed 0/1 load-bearing new inputs, injected, re-running ... red, on the expected message LOAD-BEARING matched 'owner/name boundary must not be ambiguous' 1/1 load-bearing Separator restored, no mutation residue. All 6 repo_identity_key_tests pass.
…#174) advert_per_caller_cap_sizing_is_never_zero opened with a local closure re-implementing (pushes / 8).max(1), so it bound nothing. Production derives that expression twice, and the second site was added after the test was written. per_source_push_cap now lives in rate_limit.rs next to PerCallerConcurrency, the type the value feeds, and both main.rs sites and the test call it. The finding was confirmed by execution before the fix. Deleting .max(1) from both production sites with the closure still in place: injected (2 sites), re-running ... still green VACUOUS the guard's own defect was injected and nothing failed 0/1 load-bearing And after, deleting .max(1) from the helper: injected (1 site), re-running ... red, on the expected message LOAD-BEARING matched 'advert cap must be >= 1 for pushes=1' 1/1 load-bearing The first attempt at that second proof came back RED-WRONG-REASON against 'minimum write pool must derive cap 1, not 0': the loop's assertion for pushes=1 fires first and aborts before that line is reached. Same property, earlier assertion. No mutation residue. Test green, fmt and clippy clean.
The pinata_object_list_for_refs docstring said a visibility tightening that lands after tail-start is covered by the reconciliation sweep. No such sweep exists; state.rs says so directly, that a dropped job would be lost forever precisely because there is none. The claim is dropped and nothing replaces it. An earlier draft wanted to substitute a next-push claim, which no site establishes either, and swapping one unbacked claim for another is the same defect. Four pre-existing sites make the same claim (repos.rs:30, push_delta.rs at 8, 189 and 281 with warns at 349 and 358). They predate this PR and are left for a follow-up rather than widening a nine-round diff.
…ing lacks (#174) Both new comments said saturating the pool takes ~8 distinct source IPs, each also rate-limited. Neither half holds for an IPv6 caller with a routed /64: the sub-cap and the rate limiter both key on rate_limit::client_key, which with TrustedProxy::None falls through to the full peer address with no prefix folding, so one caller has 2^64 keys. max_keys does not compensate, since keys are freed on permit drop and live keys never approach the 100,000 default. The comments now say what the cap actually bounds, concurrent slots per resolved client key, and name the residual. The keying fix stays deferred as a design call, and that deferral is defensible: full-IP keying predates this PR, so these caps help IPv4 and single-address callers without regressing anything. Landing a comment that overstates the guarantee is not defensible, in a round that corrects a weaker overclaim one screen away. The per_source_push_cap docstring added earlier in this round repeated the same claim, so it is corrected here rather than shipping fresh. Comments only, no behaviour change. check, fmt clean.
…rors (#174) Review finding from jatmn, round nine. RepoWriteGuard::release closes the connection when pg_advisory_unlock returns Err (F3b): the await resolved, so the session is alive and still holds the lock, and returning that connection to the pool hands the next caller a lock nobody tracks. The Drop backstop carried the same hazard and only logged. Both arms are fixed. The spawned detached unlock now closes the connection through close_conn_bounded on the error path, instead of letting the async block end and return it to the pool. The off-runtime arm was worse than the finding described. It dropped an already-taken connection with no unlock attempted at all, and this turned out to panic rather than leak: PoolConnection::drop calls rt::spawn for its return-to-pool task, whose no-runtime fallback panics with "this functionality requires a Tokio context". So any off-runtime guard drop panicked in a destructor today. It now detaches, which gives up the pool slot and yields a plain PgConnection whose drop closes the socket, ending the session that holds the lock. Drop cannot await, so detach is the whole disposal available here. Confirmed against the vendored sqlx 0.8.6 source rather than assumed, and min_connections is 0 everywhere in this repo. Three tests, modeled on the release F3b cases, on a pool with the idle reaper disabled so a reaped session cannot pass the assertion for us. Observed RED first: write_guard_dropped_off_runtime_disposes_the_connection ... FAILED panicked at sqlx-core-0.8.6/src/pool/connection.rs:208: this functionality requires a Tokio context then: dropping a write guard off a Tokio runtime must not panic write_guard_drop_with_failing_unlock_does_not_return_the_connection ... FAILED timed out waiting for the connection whose detached unlock errored to be closed rather than returned to the pool still holding the session lock write_guard_drop_with_successful_unlock_keeps_the_connection ... ok The success case is green on both sides on purpose: it is the must-not case, so it only ever catches a widening of disposal onto the healthy path. GREEN after, and all 12 guard tests pass, so the release coverage did not regress. Both new guards mutation-proved load-bearing, each red on its own message, 2/2. Full suite 729 passed, 0 failed. fmt and clippy clean.
|
Both findings addressed. New head [P1] Base. Resolved, but by merge rather than rebase, and the difference is worth stating. Replaying the branch onto current Checked against the merged tree rather than assumed: The push was a fast forward rather than a force push, since the merge keeps [P2] Drop-path disposal. Fixed in The off-runtime arm was worse than the finding described. It was not only returning a lock-holding connection: Three tests, on a pool with the idle reaper disabled so a reaped session cannot satisfy the assertion for us: poisoned unlock (the connection must not return, and the lock must end free), healthy unlock (the connection must still return, which is the case that catches this being widened into always-close), and the off-runtime drop. Both new guards go red when removed. One change you did not ask for, since it rewrote a commit you had already reviewed. The IPFS pin commit at the base of this range previously classified a dead endpoint as It is now a wall-clock batch budget, with the remainder passed as each request's timeout, which needs no judgement about why a request failed. The docstring states what the change does not bound, including that the Pinata replication task holds the same semaphore with no deadline of its own. Local gates on this head: 730 tests pass, |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
- [P2] Validate the derived lease timeout before multiplying it
crates/gitlawb-node/src/api/repos.rs:1732
GITLAWB_GIT_SERVICE_TIMEOUT_SECSaccepts every positiveu64and its help text explicitly permits setting it very large, but this calculation does unchecked* 2 + 60. A valid high value therefore panics every push in debug builds or wraps to a shortDurationin release builds, letting a waiter steal the same-repo lease well before the configured service timeout. Reject values that cannot support the derived duration (or use checked/saturating arithmetic with a safe maximum) before this path is reachable.
…ing it (#174) GITLAWB_GIT_SERVICE_TIMEOUT_SECS accepts every positive u64, so deriving the per-repo write-lease steal bound as `* 2 + 60` panics every push in a debug build across the top of the accepted range, and in release wraps to a short Duration that would let a waiter steal a live push's lease. Saturating keeps the knob's documented meaning: a timeout that large means the service bound is effectively off, so the steal bound should be effectively infinite rather than tiny. The regression drives git_receive_pack rather than the arithmetic, so the call-site wiring is what is under test. RED before at repos.rs:1733 with "attempt to multiply with overflow", GREEN after; re-confirmed load-bearing by reinstating the unchecked expression.
…n represent (#174) Tracing the lease-bound overflow to its other consumers turned up a worse instance of the same defect. This PR routed git_service_timeout_secs into build_filtered_pack and blob_paths, and both build a deadline as `Instant::now() + Duration::from_secs(..)`. That addition panics on overflow in RELEASE as well as debug, so a large-but-accepted value crashes the serve path rather than merely disabling a timeout. Hardening each arithmetic site as it is found would leave the next one open, so the bound goes where the value enters: clap now rejects anything above 365 days at parse time, which keeps every derived duration in range at once and fails fast at boot instead of at the first path-scoped clone. A year is many orders of magnitude above any real clone or push, so it remains the practical way to disable the bound. The test pins both derivations at the maximum and rejects the value one past it and u64::MAX. RED when the upper bound is dropped from the value_parser.
…ot tidiness (#174) A second-model pass on the previous two commits caught an upgrade footgun in the first and stale prose in the second. The 365-day ceiling was tighter than the defect required. The help text has always told operators to set this very large to disable the bound, and clap accepted every positive u64, so values like 999999999 (~31 years) are working production "off" settings that neither overflow `* 2 + 60` nor exceed an Instant deadline. A node running one of those would have failed to start after this upgrade, over a value that was never the defect. The ceiling moves to 100 years, which clears every such setting and stays an order of magnitude under the ~584-year limit of a u64-nanosecond Instant, the tightest representation on any platform we build for. Every value that worked before still parses; only the ones that would have panicked are rejected. The test now pins the pre-existing disable values alongside the rejections so the distinction cannot quietly regress. Clamp-and-warn was the suggested remedy and is not what landed: it would keep accepting a value the node cannot honor and silently substitute a different one, where the range says what is representable and fails fast, matching how every other bound in this file behaves. The lease-bound comment and its test still described the pre-ceiling world. Both now say what is true, and the test deliberately keeps u64::MAX rather than moving to the ceiling as suggested: Config is reachable by direct construction, and at the ceiling the arithmetic does not overflow, so a test pinned there would pass with the saturation removed and prove nothing. Re-confirmed load-bearing after the rewording.
|
Addressed on [P2] Validate the derived lease timeout before multiplying it. Confirmed and fixed. The sibling, which is worse than the original. This PR routed Patching each arithmetic site would have left the next one open, so the bound went where the value enters the system: clap now rejects anything the derived deadlines cannot represent. One check covers every consumer, and it fails at boot rather than at the first path-scoped clone. On the ceiling, since my first choice was wrong. I set it to 365 days initially. The help text has always told operators to set this very large to disable the bound, so values like Clamping an oversized value down to the maximum was the other option and I did not take it: it keeps accepting a value the node cannot honor and silently substitutes a different one, where a range says what is representable and fails fast, which is how the other bounds in README and |
|
One interaction worth recording here rather than acting on. The replication gate this PR moves into the receive-pack tail, and the second instance it adds on the coalescing path, both decide announceability from a repo row's own Nothing to change in this PR. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Apply a real admission limit before the database visibility work
crates/gitlawb-node/src/api/repos.rs:570
available_permits()is only a snapshot: while one read permit remains, an attacker can start an arbitrary number ofinfo/refsorgit-upload-packrequests, and each proceeds throughget_repo, quarantine, and visibility-rule queries before the per-source and global permits are acquired at lines 646/685 (or 1314/1332). If Postgres is slow, that defeats the new pre-DB load-shed guarantee and leaves an unbounded unauthenticated DB queue. Acquire a bounded admission token before those awaits, or describe this as best-effort rather than protection for the database. -
[P1] Acquire the pin budget before retaining the local-IPFS object list
crates/gitlawb-node/src/api/repos.rs:2118
The replication tail constructs the fullobject_listand moves it into a detached per-repository task, butpin_new_objects_gateddoes not acquirepin_semaphoreuntil after it receives that vector at line 1146. With a slow IPFS backend, pushes to distinct repositories can leave arbitrarily many parked tasks each retaining an MB-scale list, defeating the new global pin-memory cap and allowing memory exhaustion. Defer/re-derive the list after pin admission (as the Pinata path does), or bound admission before materializing and retaining it. -
[P2] Reject IPFS request budgets that cannot be represented by
Instant
crates/gitlawb-node/src/config.rs:505
GITLAWB_IPFS_REQUEST_BUDGET_SECSaccepts every positiveu64, thenget_by_cidadds it directly toInstantatapi/ipfs.rs:139. A valid value such asu64::MAXoverflows that addition and panics every/ipfs/{cid}handler instead of producing a bounded failure. Give this option the same representability bound as the related git-service timeout, or use a checked deadline construction and reject invalid startup configuration.
2 participants
Served-git hardening for #62, plus the follow-ups raised in review. The total-duration timeout (#165) and the teardown-wiring test (#150) are already merged; this adds the concurrency cap and the per-source / duration-bound / anti-farm hardening on top, and closes the admission/permit-lifetime holes jatmn raised.
What this does
Concurrency cap. A bounded semaphore limits how many served git operations run at once; past the cap a request is shed with a clean 503 +
Retry-Afterbefore spawning git, instead of exhausting the PID/thread table. The routing is four-way and disjoint: the upload-pack POST and the upload-packinfo/refsadvertisement draw fromgit_read_semaphore(GITLAWB_MAX_CONCURRENT_GIT_OPS, default 128); authenticatedgit-receive-packPOSTs draw fromgit_write_semaphore(GITLAWB_MAX_CONCURRENT_GIT_PUSHES, default 32); and the anon-reachable receive-packinfo/refsadvertisement draws from its own dedicatedgit_push_advert_semaphore(sized like, but disjoint from, the write pool), so an advertisement flood can shed neither a read nor an authenticated push. Config ranges are clap-bounded, so0and an oversized value that would panic tokio'sSemaphoreat boot are clean CLI errors.Per-source sub-caps. Each caller is bounded per source IP so one caller cannot monopolize a pool: upload-pack and its advertisement via
git_read_per_caller(GITLAWB_MAX_CONCURRENT_READS_PER_CALLER), and the anon-reachable receive-pack advertisement viagit_push_advert_per_caller. Keys resolve through the trusted-proxy-awareclient_key(socket-peer fallback), and every cap keys on the source IP rather than the signed DID, so a disposable-did:keyfarm cannot multiply its budget.Admission is held until the work it admitted actually completes. On the plain (non-path-scoped)
info/refs/ upload-pack / receive-pack spawn paths, the global + per-source permits are now moved into the process-group reaper and released only after the group is ESRCH-confirmed reaped, on complete, timeout, or client-disconnect. Previously the permits dropped the instant the handler future dropped on a disconnect, while the detached reaper kept tearing the group down, so a disconnect-spammer could admit replacements past both caps during the teardown window. (be0cdd6already did this for the path-scoped upload-pack walk; this closes the residual plain paths.)The acquisition phase is bounded too. The permit is taken before
RepoStore::{acquire,acquire_fresh,acquire_write}, which awaits Tigris HEAD/GET (and, on push, a per-iterationpg_try_advisory_lockthat can block on a hung Postgres pool). That phase now runs underGITLAWB_GIT_ACQUIRE_TIMEOUT_SECS(default 30, separate from the git-run timeout); on expiry the permit is released and the request sheds 503, so a stalled storage backend can no longer pin every permit and 503 the pool until restart. The/ipfsper-repo acquire loop shares this deadline.The
/ipfs/{cid}visibility walk is admission-gated. This public route ran a per-repo full-history git walk inspawn_blockingwith no concurrency cap and no rate limit. It now takes a dedicatedgit_ipfs_walkglobal permit + a per-source sub-cap (bounded, reject-before-insert map) held through thespawn_blocking— since a tokio timeout cannot cancel a blocking thread, the slot reflects real thread occupancy — plus a per-request cap on repos walked, and an IP rate limit on the route. Knobs:GITLAWB_MAX_CONCURRENT_IPFS_WALKS(32),GITLAWB_IPFS_WALK_PER_SOURCE(4),GITLAWB_IPFS_MAX_REPOS_WALKED(64),GITLAWB_IPFS_RATE_LIMIT(600/hr).Post-push encryption work is bounded without dropping durable work. Every path-scoped push spawned a detached task that parked on
git_encrypt_semaphore.acquire_owned().await; the semaphore caps active walks but the parked-waiter set was unbounded. It is now bounded by per-repo coalescing (a bounded in-flight set): a repo with a task already pending does not spawn a duplicate, and the guard releases the repo key on task completion, error, or panic. Theacquire_owneddefer stays — dropping the walk would lose the withheld-blob recovery copy and there is no reconciliation sweep to rebuild it. The Pinata replication spawn is deliberately not coalesced (it does per-push per-ref work; coalescing would drop a later push's announcements).Unsupported services are rejected before the read slot.
git_info_refsnow validates the?service=is exactlygit-upload-packorgit-receive-packimmediately after parsing, returning 400 before any read permit or DB/Tigris work, so an unauthenticated?service=anythingcan no longer consume a read slot.Every served git child is duration-bounded and reaped. The pack path already tears its process group down on drop; this discipline extends to
info/refsand the withheld-blob classification walk under one shared deadline (GITLAWB_GIT_SERVICE_TIMEOUT_SECS) withprocess_group(0)+ SIGTERM/SIGKILL reap, on every consumer (upload-pack serve, receive-pack replication, full-scan, encrypt-then-pin, and the/ipfsgate).Capacity note (operators). Holding admission through teardown means each op's effective occupancy includes the reap window (up to the ~4s SIGKILL cap; ~ms on the happy path). Size pools with teardown in mind; the per-source sub-cap, acquired before spawn and released at ESRCH, is what keeps disconnect-spam bounded per source.
Testing
Sheds are proven at the handler layer, not helper-only: each pool sheds the exact 503/504 at the router with
Semaphore::new(0), and dropping the wiring line turns the test RED. Cases are driven both ways (granted 2xx and shed/deny/hung 503/504/400) by the lowest-privilege anonymous caller. Every fix in this round is mutation-verified (revert the exact production line → RED):available_permits()==0) while a SIGTERM-ignoring group is reaped and a cross-source replacement sheds 503; releasing the permit immediately turns it RED.acquire_writeretry; the request sheds 503 at the deadline and the permit recovers; removing the timeout wrapper hangs to the test ceiling (RED)./ipfswalk: shed-at-capacity 503, per-source cap, None-key arm, bounded map, repos-walked cap, and the walk permit held through thespawn_blocking(RED when dropped before the loop); the route IP rate limit fires 429 (RED when the extension is dropped).?service=: 400 before the read pool even when the pool is exhausted (RED → 503 without the validation).did:keyfarm, hung withheld-blob walk 504, SIGTERM-ignoring child SIGKILLed) still pass.Full workspace suite green; fmt and clippy
--workspace --all-targetsclean.Closes #62.
Summary by CodeRabbit
New Features
GITLAWB_MAX_CONCURRENT_GIT_OPS,GITLAWB_MAX_CONCURRENT_GIT_PUSHES, and per-callerGITLAWB_MAX_CONCURRENT_READS_PER_CALLER.info/refsand upload-pack paths (including trusted-proxy-aware keying and bypass semantics).Bug Fixes
503andRetry-After: 1.504.Documentation / Tests