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 19 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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 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 docstrings
🧪 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.
beardthelion
force-pushed
the
fix/served-git-concurrency-cap
branch
from
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.
|
Addressed all four findings on the new head ( P1 (reserve the receive-pack advertisement). P1 (keep the filtered-upload git work inside the lifecycle). The rev-list stage moved under P1 (non-farmable per-source read cap). P2. README:346 corrected: the timeout now bounds upload-pack, receive-pack, One thing beyond the findings, worth flagging: bounding the walk covers the receive-pack replication path, not only the serve path. |
There was a problem hiding this comment.
Actionable comments posted: 2
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/api/repos.rs (1)
111-120: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the fallback object scan too.
all_blob_oidsshells out togit cat-file --batch-all-objectswith no timeout, so this fallback can still hang and hold the write permit indefinitely. Add the same deadline handling here.🤖 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/api/repos.rs` around lines 111 - 120, The fallback scan in the `spawn_blocking` closure must use the existing timeout instead of calling `all_blob_oids` without a deadline. Update the `all_blob_oids` path in this function to apply the same timeout handling, preserving the fail-closed behavior while ensuring the write permit cannot be held indefinitely.
🧹 Nitpick comments (1)
crates/gitlawb-node/src/api/repos.rs (1)
2293-2319: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the production cap calculation instead of duplicating it.
This test reimplements the expression from
main.rs, so changing the production formula—including accidentally removing.max(1)—would not fail it. Extract a shared sizing helper and call it from both production wiring and this test.🤖 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/api/repos.rs` around lines 2293 - 2319, The test advert_per_caller_cap_sizing_is_never_zero currently duplicates the production cap formula, so it cannot detect production changes. Extract the receive-pack per-source cap calculation into a shared sizing helper, use that helper in main.rs production wiring, and update the test to call it while preserving the existing minimum and default-cap assertions.
🤖 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 `@crates/gitlawb-node/src/api/repos.rs`:
- Around line 518-528: In both affected handlers, including the blocks around
the receive-pack advertisement and the referenced ranges, acquire the per-source
permit via git_permit immediately after _caller_permit. Keep this acquisition
before the global pool permit, repository acquisition, and Git execution, while
preserving the existing read/write semaphore selection and permit lifetimes.
In `@crates/gitlawb-node/src/git/visibility_pack.rs`:
- Around line 135-142: The teardown in the process-group execution flow must not
set reaped or disarm the watchdog immediately after child.wait; keep escalation
active until the process group is confirmed gone via ESRCH, then complete the
joins and notification. Update the watchdog/reaped coordination around
child.wait and watchdog.join so descendants that ignore SIGTERM still receive
SIGKILL and cannot block pipe-thread joins. Add a regression test covering a
leader that exits on SIGTERM while a background group member ignores it and
selectively closes or retains inherited pipes.
---
Outside diff comments:
In `@crates/gitlawb-node/src/api/repos.rs`:
- Around line 111-120: The fallback scan in the `spawn_blocking` closure must
use the existing timeout instead of calling `all_blob_oids` without a deadline.
Update the `all_blob_oids` path in this function to apply the same timeout
handling, preserving the fail-closed behavior while ensuring the write permit
cannot be held indefinitely.
---
Nitpick comments:
In `@crates/gitlawb-node/src/api/repos.rs`:
- Around line 2293-2319: The test advert_per_caller_cap_sizing_is_never_zero
currently duplicates the production cap formula, so it cannot detect production
changes. Extract the receive-pack per-source cap calculation into a shared
sizing helper, use that helper in main.rs production wiring, and update the test
to call it while preserving the existing minimum and default-cap assertions.
🪄 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: 4e82d225-b106-467e-bd80-78b29f742f7e
📒 Files selected for processing (9)
README.mdcrates/gitlawb-node/src/api/ipfs.rscrates/gitlawb-node/src/api/repos.rscrates/gitlawb-node/src/auth/mod.rscrates/gitlawb-node/src/git/smart_http.rscrates/gitlawb-node/src/git/visibility_pack.rscrates/gitlawb-node/src/main.rscrates/gitlawb-node/src/state.rscrates/gitlawb-node/src/test_support.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/gitlawb-node/src/test_support.rs
- crates/gitlawb-node/src/state.rs
- crates/gitlawb-node/src/auth/mod.rs
- crates/gitlawb-node/src/git/smart_http.rs
| let status = child.wait().context("git wait failed")?; | ||
| // Reaped: bar the watchdog from signalling the now-reaped (possibly recycled) | ||
| // pgid before it can fire, then stand it down. | ||
| reaped.store(true, std::sync::atomic::Ordering::SeqCst); | ||
| let err = err_reader.join().unwrap_or_default(); | ||
| let _ = writer.join(); | ||
| let _ = done_tx.send(()); | ||
| let killed = watchdog.join().unwrap_or(false); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Keep the watchdog armed until the entire process group is gone.
Line 138 marks teardown complete after reaping only the leader. If that leader exits on SIGTERM while a descendant ignores it, the watchdog stops before SIGKILL escalation; the descendant can remain alive or block the subsequent pipe-thread joins indefinitely.
Once the deadline fires, continue teardown until the process group returns ESRCH. Add a regression test where the leader exits on SIGTERM while a background group member ignores SIGTERM and closes or selectively retains inherited pipes.
Also applies to: 605-738
🤖 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/visibility_pack.rs` around lines 135 - 142, The
teardown in the process-group execution flow must not set reaped or disarm the
watchdog immediately after child.wait; keep escalation active until the process
group is confirmed gone via ESRCH, then complete the joins and notification.
Update the watchdog/reaped coordination around child.wait and watchdog.join so
descendants that ignore SIGTERM still receive SIGKILL and cannot block
pipe-thread joins. Add a regression test covering a leader that exits on SIGTERM
while a background group member ignores it and selectively closes or retains
inherited pipes.
There was a problem hiding this comment.
Not changing this one, and here's the reasoning. The withheld-walk drives only read-only plumbing — for-each-ref, cat-file, rev-list, ls-tree, rev-parse — none of which fork a descendant that could ignore SIGTERM and hold the inherited pipes; the leader-ignores-SIGTERM case is already covered and escalates to SIGKILL (run_bounded_git_reaps_a_sigterm_ignoring_child_via_sigkill). The reaped short-circuit that stands the watchdog down once the leader is reaped is deliberate: it's the reused-pgid guard from 87a7e40 — after the leader's pid is reaped the kernel can recycle it as a new group leader, so continuing to kill(-pgid) past that point risks SIGKILLing an unrelated recycled group. "Continue until ESRCH" reintroduces exactly that race. Since these plumbing commands can't produce the orphan and the guard is the intended resolution of orphan-reaping vs recycled-pgid, I'm leaving it as-is; glad to revisit if we ever run a forking child under this walk.
There was a problem hiding this comment.
@beardthelion, that's a solid rebuttal — thanks for walking through it. You're right that the commands driven here (for-each-ref, cat-file, rev-list, ls-tree, rev-parse) are all read-only plumbing that don't fork long-lived descendants holding onto inherited pipes, so the orphan scenario I raised doesn't apply to this call site. And the reused-pgid guard is a legitimate reason to stand the watchdog down once the leader is reaped rather than pushing kill(-pgid) calls past that point. I'll leave this as-is; appreciate the detailed explanation and pointer to the existing SIGKILL-escalation test.
(ᵘ‿ᵘ)⑅*
…before it (#174) CodeRabbit review: the held git_permit was acquired at the top of git_info_refs and git_upload_pack, before the per-source cap, so one source could occupy the global pool during the DB/visibility window before its sub-cap rejected the excess. Move the held git_permit to after acquire_read_caller_permit (still before acquire_fresh/git, so INV-10's bound on the fresh Tigris acquire holds), and add a cheap pre-DB early shed (available_permits() == 0 -> 503, holds no permit) so the #62 shed-before-DB property is preserved without holding a permit across the DB work. The receive-pack POST is unchanged (owner-only; its top-of-handler git_permit is deliberate for acquire_write bounding). Tests: the two #62 shed-before-DB tests now exercise the explicit early check (load-bearing: disable it and they fall through to the DB); three new *_per_source_cap_sheds_with_global_capacity tests prove the per-source sub-cap sheds a capped source even with global capacity free (load-bearing on the caller cap). Full workspace green.
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.
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. Reads (upload-pack and bothinfo/refsadvertisements) draw fromgit_read_semaphore(GITLAWB_MAX_CONCURRENT_GIT_OPS, default 128); pushes draw from a separategit_write_semaphore(GITLAWB_MAX_CONCURRENT_GIT_PUSHES, default 32), so a read flood can never shed an authenticated push. Config ranges are clap-bounded, so0(which would shed everything) and an oversized value that would panic tokio'sSemaphoreat boot are clean CLI errors, not a bad runtime state.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(sized to a fraction of the write pool, so filling it takes many distinct source IPs). Keys resolve through the trusted-proxy-awareclient_key(socket-peer fallback), and the read cap keys on that source IP rather than the signed DID, so a disposable-did:keyfarm cannot multiply its budget.The push handshake is reserved end to end. The receive-pack
info/refsadvertisement (phase one of a push) draws from the write pool, so an anonymous clone flood cannot 503 a push before it reaches the POST; the per-source advert cap keeps the advertisement itself from monopolizing that pool.Every served git child is duration-bounded and reaped. The pack path (
run_git_service) already tears its process group down on drop; this extends the same discipline toinfo/refsand to the withheld-blob classification walk (blob_paths: for-each-ref, cat-file, rev-parse HEAD, rev-list, per-commit ls-tree), which now runs under one shared deadline (GITLAWB_GIT_SERVICE_TIMEOUT_SECS) withprocess_group(0)+ SIGTERM/SIGKILL reap. The walk is bounded on every consumer that funnels through it: the upload-pack serve path, the receive-pack replication, full-scan, and encrypt-then-pin walks (which hold the write permit), and the/ipfsgate.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 (the granted 2xx and the shed/deny/hung 503/504) by the lowest-privilege anonymous caller:did:keys minted from one source IP both shed once the IP budget is full (the farm is defeated);client_key's trusted-proxy and forged/empty-header handling keeps its existing coverage.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