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 44 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.
The authenticated git-receive-pack POST acquired only the global write semaphore — no per-source sub-cap, unlike the read and anon-advert pools. Owner enforcement defaults off, so one host minting disposable did:key identities could open max_concurrent_git_pushes slow POSTs from a single source IP and 503 every other source's push; the 600/hour push limiter bounds arrival rate, not in-flight concurrency (P1-d). Add git_write_per_caller (a PerCallerConcurrency sized like the advert cap, max_concurrent_git_pushes/8), and acquire it before the global write permit, keyed on the resolved source IP via read_caller_key — never the signed DID (a DID farm defeats a DID key). This required adding the PeerAddr + HeaderMap extractors to git_receive_pack, which it lacked (without them the key is None and the cap is inert). Neutralized the shared acquire helper's log wording now that it serves both read and write paths. Regression: receive_pack_per_source_write_cap_sheds_capped_source_not_others — a source at its write sub-cap sheds Overloaded/503 (proving the extractors resolve a key), while a different source is not shed. RED before (the capped source proceeds past admission to a git error instead of shedding), GREEN after; the existing receive-pack advert cap test still passes.
…ion pool (#174) After a successful path-scoped push the handler released its write permit and then ran a DETACHED tokio task whose spawn_blocking(withheld_blob_recipients_bounded) performed another full-history git walk under no admission. N fast completed pushes spawned N concurrent full-history walks past GITLAWB_MAX_CONCURRENT_GIT_PUSHES (which bounds only the in-handler phase), and the per-child bounding from 71389e6 caps each walk's duration but not their count (P1-e). Add git_encrypt_semaphore, a pool of its own (sized from max_concurrent_git_pushes, no new knob — Q1) so a long background walk never holds a foreground write slot and a handler holding a write permit can't self-deadlock. Route the walk through a new withheld_recipients_gated helper that acquires the pool before the walk and DEFERS (blocks) when full rather than shedding — dropping the walk would lose the withheld-blob recovery copy, so durability stays fail-closed. Regression: encrypt_walk_defers_when_pool_exhausted drives the gated helper with the pool exhausted and asserts the walk blocks and does not run its rev-list, then runs once a permit frees. RED before (the walk runs regardless of the pool), GREEN after. The detached push task calls this exact helper.
…surface (#174) A source-scan tripwire that fails when a new site reintroduces the concurrency-cap class PR #174 fixed, or when one of the five gates (U1-U5) is removed. INV-22: a permit held per op recovers only if every path is duration-bounded and reaps the group before releasing admission, and every detached git task carries admission. Lives in tests/ (a separate crate) on purpose — a guard scanning the file it lives in would match its own identifier literals and pass vacuously; scanning src/ from here keeps every check load-bearing. Checks: run_bounded_git confirms child exit via child_terminated_without_reaping (U1); KillGroupOnDrop launches the reaper via Handle::try_current (U2); git_receive_pack acquires state.git_write_per_caller (U4); the encryption walk runs through withheld_recipients_gated over git_encrypt_semaphore (U5); and the bounded recipients walk is invoked exactly once — only inside the gated helper — so a new spawn_blocking bypass (count > 1) fails (P1-e non-bypass). Proven load-bearing: a synthetic second call site turns the count tripwire RED, and each named gate maps to a fix whose own regression already goes RED on revert.
|
All five addressed. Each carries a regression that fails on the exact pre-fix behavior and passes after (checked RED then GREEN by execution, not just left green), and the whole workspace suite is green with Keep the bounded visibility-walk watchdog armed until the child has exited ( Make direct smart-HTTP disconnect teardown as strong as the timeout teardown ( Do not release admission when a cancelled filtered fetch leaves its blocking walk alive ( Apply the per-source in-flight limit to receive-pack POSTs ( Bound the detached encrypt-then-pin history walks ( Plus a completeness guard ( @jatmn re-requesting review. |
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] Keep admission permits until disconnect teardown has completed
crates/gitlawb-node/src/git/smart_http.rs:142
On client cancellation, the handler-local global and per-caller permits ingit_info_refs,git_upload_pack, andgit_receive_packare dropped immediately, whileKillGroupOnDroptransfers only theChildinto a detached TERM/grace/KILL reaper. A caller can repeatedly start a slow smart-HTTP operation and disconnect, admitting replacements while each prior process group remains alive for the teardown window. That defeats both the configured process cap and the per-source cap precisely when disconnects are used to evade them. Retain the corresponding permits in the reaper (and add a disconnect test that a replacement sheds until the group has exited). -
[P1] Put repository acquisition under a finite operation deadline
crates/gitlawb-node/src/api/repos.rs:632
The new read, advert, and write permits are acquired beforeRepoStore::{acquire,acquire_fresh,acquire_write}, butGITLAWB_GIT_SERVICE_TIMEOUT_SECSstarts only once the git child is spawned. Those acquisition paths await Tigris HEAD/GET/body collection without a local deadline; the write path can also spend 60 seconds retrying the advisory lock. A stalled storage backend can therefore hold every permit indefinitely (and make the corresponding service return 503 forever) without ever reaching the bounded git runner. Bound this phase with the same operation deadline, or use a separately bounded acquisition admission policy. -
[P1] Apply admission control to the
/ipfsvisibility walk
crates/gitlawb-node/src/api/ipfs.rs:150
The new bounded helper limits oneallowed_blob_set_for_caller_boundedwalk to the timeout, butget_by_cidlaunches it inspawn_blockingfor every matching path-scoped blob request without a global/per-source permit or route rate limit. Public callers can fan out requests for a known CID and create arbitrary concurrent full-history git walks for up to 600 seconds, exhausting blocking workers and PIDs outside all of the new served-Git pools. Gate this consumer with bounded admission as well (and retain it through cancellation), rather than only timing out each individual walk. -
[P2] Reject unsupported services before acquiring a read slot
crates/gitlawb-node/src/api/repos.rs:515
git_info_refstreats every service other thangit-receive-packas a read operation, then performs the DB/visibility/Tigris work under a read permit; the service is not validated untilsmart_http::info_refsafter acquisition. Thus unauthenticated?service=anythingrequests to a public repository can consume the read pool and storage work without spawning git or passing the receive-advert rate limit. Validate that the query is exactlygit-upload-packorgit-receive-packimmediately after parsing it. -
[P2] Bound queued post-push encryption jobs, not only active walks
crates/gitlawb-node/src/api/repos.rs:1364
Every successful path-scoped push creates a detached task, which performspin_new_objectsand then waits indefinitely forgit_encrypt_semaphore. The semaphore caps active recipient walks but leaves the waiting-task queue unbounded; accepted pushes can accumulate arbitrary tasks (and their object lists, rules, paths, and cloned service state) while a slow walk occupies the pool. Add a bounded/coalesced background-work queue or acquire bounded admission before spawning, and include the pre-walk pin work in that resource policy. -
[P2] Update the PR description to match the dedicated advertisement pool
crates/gitlawb-node/src/main.rs:389
The description still says both advertisements usegit_read_semaphoreand later says receive-pack advertisement uses the write pool. The current implementation instead puts receive-pack advertisements in the separategit_push_advert_semaphore, leaving the write pool for POSTs. This gives reviewers and operators the wrong capacity/isolation model; update the PR description to the current routing.
…d on the plain spawn paths (#174) On the plain (non-path-scoped) info_refs / upload-pack / receive-pack paths the global + per-source admission permits were handler-locals that dropped the instant the handler future dropped on a client disconnect, while KillGroupOnDrop reaps the git process group in a DETACHED task (SIGTERM -> grace -> SIGKILL -> reap). So a disconnect-spammer admitted replacements while prior groups were still alive, defeating the process cap and the per-source cap during teardown. be0cdd6 already fixed this for the path-scoped upload-pack walk; this closes the residual plain paths (P1-a). Introduce AdmissionGuard, a move-only holder of the permits, threaded through info_refs/upload_pack/receive_pack/run_git_service into drive_git_child's KillGroupOnDrop. On disconnect the guard moves into the detached reaper and drops only after the group is ESRCH-confirmed reaped; on success/timeout disarm() returns it for the earliest provably-free drop. The rev-list/pack-objects visibility-walk callers hold no admission and pass None. Thread git_bin through upload_pack/receive_pack (they hardcoded "git") so the plain POST path is drivable by an injected fake git, matching the existing injectable seam on info_refs/run_git_service/upload_pack_excluding. Handler-layer regression upload_pack_plain_permit_held_through_group_reap_after_disconnect: isolates the global read pool (size 1, per-source + rate limiter permissive), a SIGTERM-ignoring descendant keeps the group alive across the reap window, and asserts available_permits()==0 held on disconnect + a cross-source replacement sheds 503, then frees after ESRCH. Reverting the guard-into-reaper move (release immediately) turns it RED at the held-through-reap assertion. Plus a None-key arm.
…ase-on-expiry deadline (#174) The served-git admission permit is acquired, then RepoStore::acquire*/advisory-lock runs before the bounded git runner starts — with no local deadline. acquire/acquire_fresh await Tigris HEAD/GET, and acquire_write's advisory-lock loop awaits a per-iteration pg_try_advisory_lock that can block indefinitely on a hung Postgres pool. Since GITLAWB_GIT_SERVICE_TIMEOUT_SECS only starts once git spawns, a stalled backend pins the permit and drains the pool until every later request 503s (P1-2). Wrap each git-path acquire (git_info_refs read/advert, git_upload_pack, git_receive_pack) and the /ipfs per-repo acquire loop in tokio::time::timeout(git_acquire_timeout_secs, ..). On expiry the git paths return AppError::Overloaded (503 + Retry-After); the permit is a handler-local at that point (moved into the AdmissionGuard only after acquire) so the early return frees the slot. The /ipfs per-repo acquire keeps its fail-closed `continue` on timeout (never serves an un-acquired repo). No repo_store.rs signature change: the outer timeout cancels a mid-sleep/mid-fetch_one future, so no in-loop deadline is needed. New knob GITLAWB_GIT_ACQUIRE_TIMEOUT_SECS (default 30, range 1..), modeled on git_service_timeout_secs, kept separate because acquisition and git execution are distinct cost centers. Handler-layer regression receive_pack_acquire_deadline_sheds_and_releases_permit: holds the same pg advisory lock acquire_write derives on a second connection so the real loop must retry, drives git-receive-pack with the deadline at 2s and write pool size 1, and asserts 503 at ~2s + the permit recovers + a follow-up admits once the lock frees. Removing the timeout wrapper turns it RED (blocks ~31s to the test ceiling on the ~59s advisory loop).
…er-source cap + route rate limit (#174) GET /ipfs/{cid} (get_by_cid, publicly reachable via optional_signature) ran allowed_blob_set_for_caller_bounded in spawn_blocking inside a per-repo loop with NO global/per-source admission and NO route rate limit — the surface had zero concurrency or rate control. A permissionless caller could fan out concurrent full-history git walks (each up to the walk timeout) exhausting blocking-pool threads and PIDs outside every served-git pool (P1-3). Acquire a global git_ipfs_walk permit (try_acquire_owned, shed 503) plus a per-source sub-cap (with_default_max_keys, reject-before-insert, keyed via client_key/TrustedProxy so XFF can't spoof it) ONCE after CID validation, held as handler locals across the whole request — including every spawn_blocking walk, so the slot reflects real blocking-thread occupancy (a tokio timeout cannot cancel spawn_blocking). None key -> global pool only. Cap repos walked per request so one request can't serialize N full-history walks. Attach rate_limit_by_ip + IpRateLimiter to the /ipfs route (mirrors the git/create/peer routers; the extension is required or the middleware is a silent no-op). Visibility/authz semantics unchanged. New 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, 0 disables) — all range-validated and enforced on-path. Handler-layer regressions (all mutation-verified RED->GREEN): shed-at-capacity 503 (delete acquire -> falls through), per-source cap sheds same source / admits another, None-key arm sheds on the global pool, map self-bounds reject-before-insert, the walk permit is held while the spawn_blocking walk runs (available_permits()==0; dropping it before the loop -> RED), repos-walked cap (remove break -> 2 walks), and the route IP rate limit actually fires 429 (drop the Extension -> RED no-op).
…cing without dropping recovery copies (#174) After a successful path-scoped push the handler spawns a DETACHED task that runs pin_new_objects then withheld_recipients_gated, which acquire_owned().await's on git_encrypt_semaphore — it DEFERS (blocks) when the pool is full. The semaphore caps active walks but nothing capped how many detached tasks were spawned and PARKED on that await: N rapid path-scoped pushes spawned N tasks, each holding cloned object lists/rules/paths/keys — an unbounded parked-waiter set (P2-2). 2a54c15 deliberately chose defer-not-shed because dropping the walk loses the withheld-blob recovery copy and there is no reconciliation sweep to rebuild it. So bound the OUTSTANDING-TASK set by per-repo coalescing rather than shedding: EncryptInflight (a bounded Arc<Mutex<HashSet<repo_id>>>) + try_begin — before the spawn, if a task for the repo is already in-flight skip the duplicate (the pending walk covers the newer objects); otherwise spawn and move an RAII EncryptInflightGuard into the task that removes the key on drop (completion, error, or panic-unwind). This bounds the set to <=1 pending task per repo and never drops work; withheld_recipients_gated's defer is unchanged. The second detached post-push spawn (Pinata replication, also runs pin_new_objects) is scoped out with rationale: it parks on no semaphore (no unbounded waiter set) and does per-push per-ref work (branch->CID, gossip, subscription, Arweave, peer-notify) keyed to this push's ref_updates — coalescing it would DROP a later push's announcements, a correctness regression. Regressions (mutation-verified RED->GREEN): the outstanding set is bounded to 1 per repo under saturation (never-coalesce -> 32 tasks RED); a coalesced repo's key is released when its task ends so it is reprocessed, never permanently skipped (guard-drop no-op -> repo locked out, recovery copy lost forever, RED — the exact durability regression the fix prevents); distinct repos each admit; cold-set first push admits; plus an inv22_gates structural tripwire that fails if the handler gate is removed.
…ent the new admission knobs (#174) P2-1: git_info_refs treated every service other than git-receive-pack as a read op and did the DB/visibility/Tigris work under a read permit, validating the service string only downstream in smart_http. So an unauthenticated `?service=anything` to a public repo consumed a read slot + storage work before rejection. Validate the service is exactly git-upload-pack or git-receive-pack immediately after parsing, returning 400 before the pre-DB shed and the permit. Handler-layer regression: with the read pool exhausted, `?service=git-explode` returns 400 (validated first), not 503 — removing the validation makes it 503 (RED). U5/INV-24 docs: document the new knobs from U2/U3 in .env.example and the README env-var table — GITLAWB_GIT_ACQUIRE_TIMEOUT_SECS, GITLAWB_MAX_CONCURRENT_IPFS_WALKS, GITLAWB_IPFS_WALK_PER_SOURCE, GITLAWB_IPFS_MAX_REPOS_WALKED, GITLAWB_IPFS_RATE_LIMIT. Each is already enforced on the path it names (U2/U3 tests), so the docs match the implemented routing.
|
@jatmn all six addressed on 413d6cf (5 commits on top of 5749df6), each mutation-verified at the handler layer. P1-1 — keep admission until disconnect teardown completes. The plain (non-path-scoped) info_refs/upload-pack/receive-pack permits now move into P1-2 — finite acquisition deadline. P1-3 — admission-gate the /ipfs walk. One correction worth flagging: the route had no rate limiter (I re-grepped — there was no existing P2-1 — reject unsupported service before the read slot. P2-2 — bound the queued encryption jobs. The residual was the unbounded outer P2-3 — PR description. Corrected to the real four-way routing (receive-pack advert → dedicated Full workspace green, fmt + workspace clippy clean, pre-push gates passed. |
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] Keep filtered-pack admission through disconnect teardown
crates/gitlawb-node/src/api/repos.rs:943
The path-scoped upload-pack branch retains its global and per-source permits only in handler-local_hold, butupload_pack_excludingstarts bothrev-listandpack-objectswith noAdmissionGuard. If the client disconnects after the filtered pack begins,_holddrops immediately whileKillGroupOnDropis still asynchronously terminating and reaping the process group. Repeated filtered fetches can therefore admit replacements during each reap window and exceed both configured read caps. Thread the admission guard through the filtered stages (including their handoff) so it is released only after the active child group is reaped. -
[P1] Do not turn existing IPFS content into a false 404 after 64 unrelated repos
crates/gitlawb-node/src/api/ipfs.rs:157
repos_walkedis consumed beforeacquireand theobject_typecheck, so it counts every readable repository rather than repositories that actually contain the requested CID or need a history walk. Sincelist_all_repos()is ordered byupdated_at, a node with more than the default 64 public repos returns an opaque 404 for a CID whose only copy is in an older repo, despite that object being retrievable before this change. Apply the expensive-walk cap after identifying a CID candidate, or return an explicit incomplete/retryable result instead of not-found. -
[P1] Bound the total lifetime of an admitted IPFS request
crates/gitlawb-node/src/api/ipfs.rs:170
The request holds onegit_ipfs_walk_semaphorepermit while giving every one of up to 64 loop iterations a new acquire timeout and a new fullgit_service_timeout_secswalk budget. With the defaults, a request whose CID is present in 64 path-scoped repos can hold a scarce slot for roughly 64 * (30s + 600s), over eleven hours. A small number of such requests exhausts the 32-slot pool despite the new admission control. Use an absolute request deadline/budget shared by all acquisition and walk iterations. -
[P1] Admit the post-receive Git scans to a bounded pool
crates/gitlawb-node/src/api/repos.rs:1375
The write admission is moved intosmart_http::receive_packand dropped when that RPC is reaped, before this handler runsreplication_withheld_set,resolve_candidates_for_push, and the possible full scan. Those paths spawn bounded Git children, but none are covered by the write pool or the encrypt-walk semaphore. A stream of successful pushes can consequently accumulate unboundedls-tree,rev-list, andcat-filescans after each receive-pack slot is released, defeating the process/worker exhaustion protection this PR adds. Keep suitable admission through these scans or route them through a dedicated bounded queue. -
[P1] Requeue coalesced post-push recovery work
crates/gitlawb-node/src/api/repos.rs:1441
Skip-only coalescing loses a later push once the in-flight task has already taken its recipients/object snapshot. For example, task A can finishwithheld_recipients_gatedand then be pinning, encrypting, or anchoring when push B completes; B seestry_beginreturnNone, and no work is recorded for it. A only processes its earlierobject_listand recipients map, then releases the guard with no rerun, leaving B's normal pins and newly withheld recovery copies absent until another push happens. Track a dirty generation and rerun, or otherwise schedule work for pushes that arrive after the task snapshot.
…concurrency base Rebased onto #174 (fix/served-git-concurrency-cap). #173's incremental history cannot replay commit-by-commit because #174 rewrote the same handler files in parallel; this single commit carries the fully-integrated tree (identical to the verified merge result), with the follow-up fixes as separate commits on top. Brings in #173: GET /ipfs/{cid} CID->oid resolution via pinned_cids, per-caller path-scoped blob/tree/commit-tag gating (#135/#173 F1-F6), and the pin-source provenance table. Adapts #173's tree/commit-tag walks to #174's run_bounded_git so the held /ipfs walk-concurrency permit is duration-safe (F5).
P1-A: gate_and_serve held the /ipfs walk permit across a bare repo_store.acquire; wrap it in git_acquire_timeout_secs (mirroring #174's own handler) so a cold/hung Tigris acquire cannot pin the global walk slot. On expiry skip the repo and mark the search truncated (retryable 503, not a false 404). P1-B: the local-IPFS pin path recorded Kubo's provider Hash as pinned_cids.cid; for objects above the block size that is a dag-pb root that does not hash the raw content, so GET /ipfs/{cid} listed then 404'd them under the F2 integrity check. Record the locally-computed raw-content CID, mirroring the pinata twin.
…174) drive_git_child hands the AdmissionGuard back on success so upload_pack_excluding can thread it across the rev-list and pack-objects stages; on timeout or error it is dropped post-reap, and on future-drop it rides the detached reaper as before. The upload-pack handler builds the guard once ahead of the withheld/plain branch, so a client disconnect mid-filtered-serve no longer frees the read-pool and per-source permits while the git process group is still being reaped.
…iling, 503 on truncation (#174) A request-scoped taint list replaces the silent folds: any repo skipped without a verdict (acquire timeout, probe or read failure, walk failure, cap or ceiling exhaustion) turns the terminal into a retryable 503 with Retry-After, and 404 is reserved for a scan in which every candidate reached a verdict. The walk cap now counts only expensive allowed-set walks and skip-continues on exhaustion so a plain public copy past the cap still serves; a new GITLAWB_IPFS_MAX_REPO_VISITS knob (default 1024) bounds the acquire+probe cost class and stops the scan, documented with its worst-case object-store fetch count.
…ared budget (#174) GITLAWB_IPFS_REQUEST_BUDGET_SECS (default 600) is captured as one deadline at handler entry: every acquire and expensive walk is clamped to the remaining budget, every stage checks remaining before starting, and exhaustion taints the scan into the retryable 503. A walk in flight is never aborted at the tokio layer (that would free the walk permit while the blocking thread still runs); its clamped git deadline plus group teardown ends it. Docs state the residual overshoot: the in-flight stage's kill/reap slack plus the unclamped probe subprocesses.
…r-not-shed (#174) replication_withheld_set's walk arm, resolve_candidates_for_push's git stages, and fail_closed_full_scan_objects each take a git_encrypt_semaphore permit before their spawn_blocking and carry it inside the closure, so a push burst holds at most pool-many concurrent scans and a started scan finishes with its permit. Deletion-only pushes and repos without path-scoped rules never park. Contention defers the landed push's tail (logged with queue_wait_ms), never errors it; the acquire sites document the accepted disconnect-during-park residual.
…ot and loop-drain (#174) try_begin now merges a losing push's tip pairs into the in-flight key's pending slot in the same critical section as the presence check, bounded at 1024 pairs with a full-scan marker on overflow (force_full_scan threads to the resolver; empty tips never encode the marker). The task loop-drains: finish_or_take_pending either hands back the batch with the key retained, or removes the key and disarms the guard atomically, so a successor task's key survives the guard drop. Drains re-fetch rules fresh and replay the tail pipeline under the U4 helper gates only; the loop holds no task-level pool permit, so a hot repo cannot deadlock the pool at size 1.
object_type now distinguishes repo-level git failures (not-a-repository, error:-prefixed corruption) from genuine object absence, so a corrupt repo taints the /ipfs scan into the retryable 503 instead of vouching for a definitive 404; the drain reads visibility rules by the freshly fetched record id, closing a latent fail-open on id divergence. The four budget gates collapse into one stage-labelled helper, the three scan sites share state::acquire_scan_permit (inv22 tripwire repointed and sever-verified), and the Tigris stall tests use a local accept-and-park listener instead of a non-routable address.
…estly; net the acquire-taint found path The budget-knob docs claimed the overshoot was bounded by one unclamped probe; an unclamped subprocess is not a bound, so the README, .env.example, config help, and module comments now say plainly that a hung git probe holds the walk slot for the hang's duration. New regression: an acquire-tainting ghost row ahead of a healthy public copy must not stop the scan; the later found short-circuit still serves 200.
|
All five addressed on [P1] Keep filtered-pack admission through disconnect teardown ( [P1] Do not turn existing IPFS content into a false 404 after 64 unrelated repos ( [P1] Bound the total lifetime of an admitted IPFS request ( [P1] Admit the post-receive Git scans to a bounded pool ( [P1] Requeue coalesced post-push recovery work ( The |
|
Settling the F4 design call, since a few of you flagged it: the post-receive scans stay inline in the The direction is to detach the scans onto the F5 coalescer's drain task so the push response returns without waiting on the scan pool: I kept it out of #174 deliberately: the branch is deep with review pending and #173 stacked, and a push-tail behavioral restructure here is pure rebase churn for a change that reuses machinery this PR already lands. Worth stating plainly while we are here: the detach shrinks the disconnect window but does not reach zero, because a node crash or shutdown mid-drain still loses in-flight work. |
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] Make the new write-acquire timeout cancellation-safe
crates/gitlawb-node/src/api/repos.rs:1508
acquire_write()obtains the session-level Postgres advisory lock before awaiting Tigris, but the new outertokio::time::timeoutcan drop that future duringexists/download, before aRepoWriteGuardis returned. That skips the only unlock path, leaving the lock on a pooled session and causing later pushes for the repo to keep retrying/failing until that connection disappears. Keep a cancellation-safe lock guard inside acquisition (or explicitly unlock on cancellation) instead of externally cancelling a future that may already own the lock. -
[P1] Do not drop post-receive replication work while waiting for scan capacity
crates/gitlawb-node/src/api/repos.rs:1655
The successful push waits on the newgit_encrypt_semaphorethroughreplication_withheld_setand candidate/full-scan resolution before it creates the coalesced detached task at line 1734. If that pool is saturated and the client/proxy disconnects, Axum cancels this request future and there is no queued/coalesced record to retry the already-landed push. Its pins, recovery copy, and announcements can therefore be lost permanently;state.rseven documents this exact residual. Record durable/coalesced work before an interruptible permit wait, or move this tail into an independently owned task. -
[P1] Actually bound the
/ipfscat-file operations
crates/gitlawb-node/src/api/ipfs.rs:313
The request budget is checked only beforestore::object_typeandread_object_content; both helpers use synchronousCommand::output()without a deadline or process-group cleanup. A blocked filesystem or hung/corrupt object database therefore holds the new global/per-source IPFS permits indefinitely (and blocks a Tokio worker), so enough requests can exhaust the route despiteGITLAWB_IPFS_REQUEST_BUDGET_SECS. Run these probes through the bounded/reaped Git runner off the async runtime, using the remaining request budget while retaining admission until reap. -
[P2] Share one deadline across the full-scan filtering phases
crates/gitlawb-node/src/api/repos.rs:140
fail_closed_full_scan_objectsfirst runsreplicable_blob_set_bounded(..., timeout)and then startsall_blob_oidswith a freshInstant::now() + timeout, while retaining one scan-pool permit. A fallback can consequently occupy the pool for nearly two full service-timeout windows rather than the configured whole-scan budget, multiplying queued post-push work. Create a deadline before the first phase and pass its remaining duration to the second phase. -
[P2] Treat only a confirmed missing object as an absence verdict
crates/gitlawb-node/src/git/store.rs:283
object_typemaps nearly every nonzerogit cat-file -texit toOk(None): it only promotes “not a git repository” and lines beginningerror:. Other probe failures such as a permission failure or anotherfatal:diagnostic become an absence verdict, so the new/ipfsscan can return a definitive 404 for an existing object instead of tainting the scan and returning its intended retryable 503. Recognize an actual missing-object diagnostic explicitly and surface all other nonzero exits as errors. -
[P2] Bound outstanding encryption tasks globally, not only per repository
crates/gitlawb-node/src/state.rs:319
EncryptInflightlimits a repo to one task but has no global key/task bound. Once the scan semaphore is full, one first push to each distinct public path-scoped repo can leave another detached task parked with its context, object list, rules, and map entry. An actor can repeat that across repositories, so the claimed outstanding-task bound does not prevent unbounded memory/task growth. Add a global bounded queue/permit with durable overflow or retry behavior.
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