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 73 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.
…174 F5 review) The deterministic-fault terminal 500 was checked before the transient truncated_by 503, unconditionally. When one repo is corrupt (deterministic) while a DIFFERENT repo is transiently skipped in the same scan, the requested CID may live in the transient repo -- a retryable 503 is correct, but the handler returned a terminal 500 and a conformant client won't retry, hiding retrievable content until the unrelated corrupt repo is repaired. Gate the 500 on truncated_by.is_empty(): it fires only when a deterministic fault is the SOLE reason nothing served; a co-occurring transient taint falls through to the retryable 503. A pure deterministic fault still terminally 500s an absent lookup by design. Adds a co-occurrence regression test.
…#174) - steal_after: the reclaim is NOT a guarantee that only a leaked lease is reclaimed. A waiter's timeout starts at acquire(), not at the FIFO head, so a same-repo backlog whose cumulative wait exceeds steal_after can steal while an earlier waiter still writes; correctness rests on the retained pg advisory lock (which serializes the stealer at acquire_write), not on the bound. - pinata_object_list_for_refs: recomputes from the tail-start rules snapshot, not a fresh read at pin-worker time, so a rule tightened AFTER tail-start is not reflected (matches the old retained-list behavior; reconciliation sweep is the backstop). Removed the overstated 'rule tightened since the push is honored'. - RepoWriteGuard::Drop: note that on runtime shutdown the detached unlock task may be dropped before it polls, bounded by pool teardown / connection close.
|
All six addressed on Bound queued Pinata replication ( Keep the write lock until a disconnected receive-pack is reaped ( Cancellation-safe advisory unlock ( Positively-identified missing object ( IPFS request deadline on the initial metadata queries ( Coalesce the post-receive tail before its scans. Keeping this as an accepted residual (documented at |
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] Do not leave per-repository lease waiters outside write admission
crates/gitlawb-node/src/api/repos.rs:1640
The per-repository lease is awaited before the per-source and global write permits. Its waiter set is unbounded (state.rs:517-557) and waits for up to2 * git_service_timeout + 60seconds, so many requests for one busy repository retain their already-buffered pack bodies while neithergit_write_per_callernorgit_write_semaphoreaccounts for them. This lets a push flood exhaust task/memory capacity despite the new write cap. Give lease waiters a bounded admission/queue (or otherwise bound the body and waiter count) before they can park. -
[P1] Coalesce post-push work before the first admission-gated scan
crates/gitlawb-node/src/api/repos.rs:1843
A successful push always spawns an outer replication task, which first awaitsreplication_withheld_setand candidate resolution and materializesobject_list; only afterwards does it callencrypt_inflight.try_beginaround line 1930. When the encrypt scan pool is saturated, rapid pushes to the same path-scoped repository accumulate parked tasks and later repeat scans/object-list allocation before the per-repo coalescer ever runs. The separate Pinata/announcement task at line 1970 also queues before acquiringpin_semaphore, retaining its per-push ref data while the backend is slow. Move bounded, durable queueing/coalescing ahead of these awaits and use compact ref-update inputs for the drain worker. -
[P1] Keep the advisory-lock backstop armed when unlock fails
crates/gitlawb-node/src/git/repo_store.rs:431
releaseignores an error frompg_advisory_unlockand unconditionally setsreleased = true. If that query is cancelled or errors while the pinned session remains alive,Dropno longer runs its same-connection unlock backstop and thePoolConnectionreturns to the pool still holding the session-level advisory lock. Later writers can then spin until the retry limit. Only mark the guard released after a successful unlock (and preserve an error path that disposes/unlocks the pinned connection).
…o lease A push buffers its whole pack before the handler runs, then parks on the per-repo write lease for up to git_service_timeout_secs * 2 + 60, which is 1260s at defaults. Both admission permits were taken after that park, so nothing accounted for waiters: any number of requests could sit on one contended repo, each holding a fully buffered body, invisible to both semaphores. The per-source sub-cap now runs before the park. It already exists, is sized at a quarter of the push pool, and is keyed on the resolved source IP, so parked bodies are bounded at four per source without a new knob or a new cap to tune. The global permit deliberately stays below the lease. Moving it up is what f02c5e1 fixed: a lease-blocked waiter holding a global slot sheds pushes to every other repo node-wide. A per-source key does not have that shape, since exhausting it denies only the source doing the exhausting, so the two permits belong on opposite sides of the park and the comment there now says why. The load-bearing test is not that a capped source sheds, which any cap satisfies, including the global one this rejects. It is that a different source still parks and is served while the first is at its cap. Keying the permit on a constant leaves the shed test green and turns only that one red. Moving the global permit above the lease turns the f02c5e1 guard red. Bounded per source, not globally. A distributed set of sources can still each hold their four, and the unbounded-total term is the pre-auth buffering in #263 rather than the lease.
…f pooling a locked connection `release` discarded the unlock's result and set `released` regardless, so an unlock that errored while its session stayed alive (a statement timeout, an admin cancel) returned a connection to the pool still holding the repo's advisory lock, with the Drop backstop already disarmed by that flag. The error is now captured. On failure the connection is taken and closed, which ends the session and makes Postgres drop the lock server-side, and only then is the guard marked released, because the lock went with the session rather than with the unlock. `close()` rather than `detach()` since we are already in an async fn and can wait for the teardown deterministically. Disposal is the only mechanism on this path. It cannot coexist with an armed Drop backstop: disposal needs the connection taken, and Drop returns early when there is none, while leaving the connection in place so Drop can use it just puts the locked connection back in the pool when that spawned task ends. The Drop doc now says so rather than claiming to cover a case it cannot. The cancellation path is untouched. It was already correct, since `released` is set after the await, and its test still passes unmodified. The new tests assert against `pg_locks` from a connection held out of the pool before the acquire, never by calling `acquire_write` a second time. That shortcut would pass whether or not the fix is present, because the pool can hand back the same connection and session advisory locks are re-entrant. The unlock error is injected by leaving the pinned connection in an aborted transaction, which errors the next statement while keeping the session and its lock alive, and the test asserts that precondition before releasing. Not covered: that the failure path still skips the Tigris upload. The test store builds with no Tigris client, so the upload site is unobservable here without a new seam, and that block is untouched by this change.
…, not after The post-push tail resolved the withheld set and materialized the object list before it ever consulted the per-repo coalescer, so rapid pushes to one repo each parked on the encrypt semaphore and repeated the scan before discovering they should have coalesced. The gate now runs first, keyed on the cheap listable-at-root predicate that `replication_withheld_set` already computes from rules alone before it touches the semaphore. Three things made this less mechanical than it looks, all of them ways a naive move would have shipped a regression. The withheld set had a second consumer. It also gated the Pinata pin and the branch-to-CID upsert, so a coalesced push that skipped the walk would have pinned nothing and recorded no CID, while a test asserting merely that the announce spawn ran would have stayed green. That gate moves to the announce predicate too. Coalescing must not return from the tail. The announce spawn sits below the coalescer in the same task, and returning early would drop gossip, the GraphQL broadcast, Arweave anchoring and peer notify, which are per-push and non-idempotent. A flag skips the encrypt section and falls through. The publish paths are gated on announce and iterate the ref updates, not the object list, so handing the tail the cheap predicate would have made a repo whose walk is failing start gossiping and anchoring, losing the fail-closed property. `pinata_object_list_for_refs` already recomputes the real set inside the pin permit, so it now returns that verdict alongside the list and the tail uses it. No additional walk is introduced. Moving the gate earlier also opens a window where a push can coalesce while the admitted one is still walking. If that walk then fails, the admitted task now still spawns the drain with an empty snapshot so those tips are consumed, rather than dropping the guard and discarding them. Note a released guard and a dropped guard are indistinguishable by the in-flight map, since Drop clears the key either way, so that case is asserted on drained work rather than on the map being empty. Each of the four is pinned by a mutation: walking anyway, returning early, leaving the Pinata gate on the withheld set, and dropping instead of draining each turn exactly one test red.
…deadline The two F1 tests asserted "the capped source shed rather than parked" by racing a ten second timeout, and drove the holding push in ten millisecond slices. Under `cargo test --workspace` that is a race against the scheduler rather than a test: a reviewer saw two of five runs go red, and under load the pair stretched from 1.8s to 19.2s while the fake git holding the lease only lived for 10s, past which every assertion in both tests is meaningless. Shed versus park is a question of which path ran, so both are now read as state. A shed is `Err(Overloaded)`. A park is a second reference on the lease entry, exposed by a test-only `refs_for`; `acquire` takes that reference synchronously before the cancellable wait, so a waiter is visible the instant it reaches the lease, and it stays visible because the steal bound is 1260s. The tests spawn their pushes and poll until one of the two mutually exclusive states appears, so a starved machine iterates more rather than failing. Every wall-clock assertion is gone. The worst of them was a sleep followed by `!is_finished()`: a runtime that had not yet polled that task satisfied it vacuously, which means the constant-key mutation could have gone green under load, and the discriminator is the whole reason that test exists. All three mutations still go red, now in seconds rather than by timeout expiry: the per-caller acquire below the lease, the global permit above it, and the per-caller key replaced with a constant. Also fixes a third flake found on the way: the acquire-deadline test's follow-up push inherited the deliberately short two second acquire timeout, which has to cover a real advisory-lock round trip, so its `Overloaded` was indistinguishable from the drained-pool `Overloaded` the assertion was actually testing for. The sqlx `databases_pkey` collisions seen alongside this are machine residue, not a leak from these tests: sqlx derives the test database name from a hash of the test path and does cleanup-then-insert without a lock, so any two concurrent test binaries running the same path collide. The orphan count here held at 53 across every run.
… budget on a per-repo wait The previous attempt moved the per-source write permit above the lease park to bound buffered pack bodies. Two reviewers demonstrated that it denies far more than the source doing the parking. A source parked on one repo's lease was shed on repos with no contention at all, and because the trusted-proxy setting defaults to trusting nothing and keying on the socket peer, every pusher behind a proxy, NAT, or CI runner pool resolves to one key: four parked pushes shed every push from every pusher to every repo for up to twenty-one minutes. That is the node-wide shed the permit was moved below the lease to prevent in the first place, reintroduced at an eighth of the threshold. The permit goes back below the lease. What is actually unbounded is the waiter set, so that is what is bounded now, per repo, with a shed past the cap. The cap counts live waiters, never the entry refcount. The refcount includes the holder, so capping it would let a lease whose owner died without unwinding pin a slot forever, which is the permanent wedge the steal timeout exists to survive. The count is claimed in the same critical section as the refcount and held by a guard scoped to the cancellable wait, so a cancelled or shed waiter gives it back. An uncontended acquire takes the fast path and spends no waiter budget; that path does not barge a queued waiter, which was checked over two thousand rounds rather than assumed. Two tests now guard the defect that was just removed: a source parked on one repo is still served on another, and a parked push does not shed an unrelated pusher sharing its resolved key. Restoring the old ordering turns both red. Capping on the refcount instead of live waiters turns the steal-backstop test red. Holding the waiter slot past the wait turns the fast-path test red. What this bounds is the parked count, not the parked bytes. At a two gigabyte pack limit the byte figure stays large, and the total across repos, whose dominant term is the buffering the signature middleware does before any admission point at all, is tracked in #263. Past the cap, same-repo concurrency sheds instead of queueing; that is the trade the knob buys.
… close on an errored unlock Three findings from the review round. Moving the Pinata gate to the rules-only predicate was necessary, because a coalesced push has no walk of its own to consult. But it also meant a push whose OWN walk failed now acquired a global pin permit and ran a second full re-derivation that failed the same way. Measured on a repo whose rev-list always fails: twelve git children and three walk attempts, against four and one before. With eight pin slots that defer rather than shed, eight such pushes stall pins for every repo on the node. The gate now excludes a non-coalesced push whose own walk failed, leaving the coalesced path on the rules-only predicate it needs, and the pin helper returns before taking a permit for an empty object list. The connection close on the failing-unlock path had no deadline, and release is awaited inline while the write lease and both admission permits are still held. The condition that produces that branch is a connection whose last statement errored, and a blackholed socket to Postgres is one way to get there, so the await could pin three admission resources for a TCP timeout. It now runs under a five second bound; on expiry the future is dropped, which closes the socket and ends the session anyway, which is what frees the lock. What is tested is the deadline. That sqlx's close is the thing that stalls in production is reasoned from its source, not reproduced, and the test says so. The two tests for that path waited a fixed four hundred milliseconds before asserting. Replacing the sleep with polling was not enough on its own: the test pool sets a one second idle timeout, so a connection returned to the pool is reaped shortly after and the session ends without any help from the code under test. Both tests passed under mutation once the sleep was long enough to reach that reaper, which means a straightforward flake fix would have quietly gutted them. They now run against a pool with no idle reaper, so release is the only thing that can end the session, and both mutations go red again. Adds tokio's test-util as a dev-dependency for paused-time assertions. Production dependencies are unchanged and the build stays --locked clean.
|
All three addressed on Do not leave per-repository lease waiters outside write admissionFixed, but not the way I first tried, and the first attempt is worth describing because it was worse than the bug. I initially moved the per-source write permit above the lease park, reasoning that a per-source key is not global so exhausting it denies only the exhausting source. That is false in the default deployment. The permit is back below the lease. What was actually unbounded is the waiter set, so that is what is bounded now: a per-repository cap inside The cap counts live waiters, never the entry refcount. The refcount includes the holder, so capping it would let a lease whose owner died without unwinding pin a slot permanently, which is the wedge the steal timeout exists to survive. The count is claimed in the same critical section as the refcount and held by a guard scoped to the cancellable wait, so a cancelled or shed waiter returns it. An uncontended acquire takes a Two tests guard the defect I introduced and then removed: a source parked on one repository is still served on another, and a parked push does not shed an unrelated pusher sharing its resolved key. Restoring the old ordering turns both red. What this bounds is the parked count, not the parked bytes. A lease holder has to win a non-blocking global write permit, so at most Coalesce post-push work before the first admission-gated scanFixed. Three things made that less mechanical than it looks. The withheld set had a second consumer. It also gated the Pinata pin and the branch-to-CID upsert, so a coalesced push skipping the walk would have pinned nothing and recorded no CID, while a test asserting merely that the announce spawn ran would have stayed green. That gate moved too. Coalescing must not return from the tail. The announce spawn sits below the coalescer in the same task, so an early return would drop gossip, the subscription broadcast, anchoring and peer notify, which are per-push and non-idempotent. The publish paths are gated on announce and iterate the ref updates rather than the object list, so an empty list does not suppress them. Handing the tail the cheap predicate would have made a repository whose walk is failing start gossiping and anchoring. Moving the gate earlier also opened a window where a push can coalesce while the admitted one is still walking. If that walk then fails, the admitted task now still spawns the drain with an empty snapshot so those tips are consumed rather than discarded. One consequence I caught reviewing my own fix and corrected in the same series: with the gate on the rules-only predicate, a push whose own walk failed was acquiring a global pin permit and running a second re-derivation that failed identically, twelve git children and three walk attempts against four and one before. The gate now excludes a non-coalesced push whose own walk failed, and the pin helper returns before taking a permit for an empty list. On the drain worker's inputs, declining
Already the case, from an earlier round. The Pinata task captures Keep the advisory-lock backstop armed when unlock failsHalf of this was real and is fixed. The cancellation half was already handled. The real half: the unlock's result was discarded and Disposal is the only mechanism on that path. It cannot coexist with an armed The close is bounded at five seconds, because On cancellation, declining
Already handled. TestsThe new lock tests originally waited a fixed four hundred milliseconds before asserting. Replacing that with polling was not enough on its own: the test pool sets a one second idle timeout, so a connection returned to the pool is reaped shortly after and the session ends without any help from the code under test. Both tests passed under mutation once the poll reached that reaper, which means the obvious flake fix would have made them stable and worthless. They now run against a pool with no idle reaper, and both mutations go red again. Two earlier tests also asserted timing under a ten second deadline while driving the holding push in ten millisecond slices, and went red two runs in five under Merge and verificationMerged Adds tokio's |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the detailed d753198 response — I'm taking it as authoritative for this round.
My prior findings look addressed on current head as you described: the per-repo lease waiter cap with permits kept below the lease park (and the reverted above-lease ordering that would have shed cross-tenant behind a proxy), try_begin before replication_withheld_set with the Pinata/announce gating and failed-walk pin skip, advisory-unlock error disposal with bounded close, and compact ref-tuple retention in the Pinata worker (already in place at repos.rs:2103). I'm not re-raising those.
On the design choices you documented rather than changed: dropping the write lease before the detached post-receive tail so the next same-repo push can proceed once the write is durable (repos.rs:1767) matches the accepted-residual pattern you've used elsewhere — bounded parked-task cost, fail-closed replication, reconciliation backstop. I'm not treating that overlap window as an open defect this round.
I have two small items left before I'd call this done.
Findings
-
[P3] Add
/ipfsrate-limiter cleanup to the periodic sweeper
crates/gitlawb-node/src/main.rs:494
You addedipfs_rate_limiteron the/ipfs/{cid}route in the P1-3 round (concurrency cap plus per-IP rate limit). The 300s cleanup task still sweeps push/create/sync/peer limiters only.RateLimiter::checkalready does an inline capacity sweep when the map is full, so I don't think this is a permanent false-429 trap, but stale keys under the 200k cap are never evicted on the same cadence as the other limiters. Please callcleanup()onipfs_rate_limiteralongside the rest for consistency with how you operate the other limiters. -
[P3] Document
GITLAWB_REPO_LEASE_MAX_WAITERSin.env.example
crates/gitlawb-node/src/config.rs:340
The knob you added for the per-repo lease waiter cap is load-bearing for the shed path you described (acquirereturnsNonepast the cap → 503). It's absent from.env.exampleand the README settings table while the other concurrency knobs from this PR are documented. Operators copying the template can't tune or discover why pushes to one hot repo returnrepo is busy with another push. Please add it beside the other admission settings.
… cap The 300s cleanup task swept the DID, create-IP, push, sync-trigger and peer-write limiters but not ipfs_rate_limiter, so its expired keys sat until the map hit max_keys and the inline capacity sweep reclaimed them. Drive the sweep off AppState in a named sweep_rate_limiters so a limiter added to the state and forgotten here fails a test instead of leaking keys, and cover all six with a regression that goes red when any one is dropped from the sweep. GITLAWB_REPO_LEASE_MAX_WAITERS was also missing from .env.example and the README settings table, leaving operators without a way to discover or tune the cap behind the "repo is busy with another push" 503.
|
Both addressed on [P3] Rather than add a sixth line to an inline The observable this needed was a key count, which [P3] Document The half I want to flag: the README table did not document the other concurrency knobs from this PR.
|
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the b9566b7 follow-up — the /ipfs rate-limiter sweep and GITLAWB_REPO_LEASE_MAX_WAITERS in .env.example address my prior P3 items, and I'm not re-raising those.
Rechecked current head after reconciling against main and the stated residuals. A few items below; I'm not re-raising the transient-DB fail-closed drain path (resolve_drain_object_list returning None on get_repo Err is intentional) or the advert/POST Retry-After mismatch (that predates this PR).
Findings
-
[P2] Hold
/ipfswalk admission until blocking git work finishes, not just the handler future
crates/gitlawb-node/src/api/ipfs.rs:134-149
The global and per-source walk permits are handler locals. On client disconnect the future drops and both permits release immediately, while the uncancellablespawn_blockingprobe/walk/read keeps running —get_by_cid_walk_permit_held_through_blocking_walkdocuments this at lines 2334-2339. I understand this as a knownspawn_blockingtradeoff, but it still lets a disconnect flood admit replacement requests while prior blocking git children occupy threads/PIDs, weakeningGITLAWB_MAX_CONCURRENT_IPFS_WALKSandGITLAWB_IPFS_WALK_PER_SOURCE. Same admission class you closed on upload-pack: move the permits into eachspawn_blockingclosure (asreplication_withheld_setdoes atrepos.rs:80-85) and release them only when the blocking work completes. -
[P2] Key per-repo lease and encrypt coalescing on the disk path, not
record.id
crates/gitlawb-node/src/api/repos.rs:1671+1937+crates/gitlawb-node/src/state.rs:555
repo_write_leasesandencrypt_inflightkey onrecord.id, butRepoStore::local_pathand the pg advisory lock key onowner_slug/repo_name. After delete+recreate with the same slug, a new row gets a new id while reusing the same on-disk bare repo: a disconnected push's reaper can still be tearing down the old group while a new push acquires a different lease/coalesce key and proceeds concurrently on the sharedobjects/directory (F3), and two detached encrypt tasks can run for the same path.resolve_drain_object_listalready documents this id-rotation case for rules (repos.rs:918-921); please key these serializers on the same stable identity the store uses (owner/name or normalized path). -
[P2] Write encrypted-pin metadata under the re-fetched repo id on coalesced drains
crates/gitlawb-node/src/api/repos.rs:1162-1166
Coalesced drains re-fetch the repo row by owner/name and load fresh rules (resolve_drain_object_list, lines 900-922), butencrypt_and_pinstill receives&ctx.repo_idfrozen at task spawn. On delete+recreate (or any id rotation with the same slug), recovery metadata is written against the deleted row while the live repo row is different — authorized readers on the new row cannot discover those pins. Please pass the re-fetchedrecord.id(or otherwise updateEncryptTaskCtx) for encrypt/anchor writes in the drain path. -
[P3] Move the encrypt-walk permit into the blocking closure in
withheld_recipients_gated
crates/gitlawb-node/src/api/repos.rs:823-832
withheld_recipients_gatedacquiresgit_encrypt_semaphorein the async frame, then.awaitsspawn_blockingwithout moving_permitinto the closure. Every other post-receive scan helper and the F4acquire_scan_permitcontract move the permit into the blocking closure. Low practical risk inside the detached encrypt task, but please align this site so the F4 pattern cannot drift. -
[P3] Spawn the post-receive replication tail before the certificate/webhook window
crates/gitlawb-node/src/api/repos.rs:1784-1888
F2 is a real improvement overmain(the tail used to run inline in the handler), buttokio::spawn(post_receive_replication_tail(...))still happens only aftertouch_repo, trust-score updates, per-ref certificate issuance, and webhook fan-out. If the handler future is cancelled in that window, the push is already durable on disk yet the tail never spawns. The window is narrow in practice (the client is usually still waiting for the HTTP response), but please spawn (or at leasttry_begin+ enqueue) immediately after a successfulreceive_pack/guard.release()if you want F2 to be complete. -
[P3] Honor
state.git_binon theinfo/refsadvertisement path
crates/gitlawb-node/src/api/repos.rs:731
git_info_refscallssmart_http::info_refs("git", …)while upload-pack and receive-pack use&state.git_bin.info/refsnever had an injectable binary onmain; this is partial wiring of the newgit_binfield rather than a regression, but tests and any fake-git harness still exercise pack paths without the advertisement.
…po id resolve_drain_object_list already re-fetches the repo row by owner/name and deliberately reads visibility rules under record.id, because a delete+recreate under the same slug gives the row a new id while the bare repo on disk is reused. The encrypt write a few lines later still took ctx.repo_id, frozen when the task was spawned, so recovery-pin metadata for a coalesced drain landed against the deleted row and was unreachable from the live one. Return the re-fetched id from the resolver and pass it explicitly to pin_and_encrypt_objects. The parameter is explicit rather than read from ctx so the two callers stay honest about which id they mean: the snapshot iteration passes its own spawn-time id, the drain passes the fresh one. The Arweave anchor is unaffected - EncryptedManifest carries no repo id and builds its slug from owner and name, both stable across the rotation.
…isk identity repo_write_leases and encrypt_inflight keyed on record.id, but RepoStore::local_path and the pg advisory lock key on the sanitized owner slug plus repo name, and the bare repo on disk is reused when a repo is deleted and recreated under the same slug. The row id rotates across that recreate, so both in-process serializers stopped serializing exactly where the shared objects/ directory still needed them: a disconnected push's reaper could still be tearing down while a new push took a different key and proceeded. Add repo_identity_key(owner_did, repo_name) and use it at both sites. Three details are load-bearing and carry tests: the sanitization is repo_disk_path's replace([':','/'],"_") rather than normalize_owner_key, which folds a did:key: prefix and would disagree with the disk path; the separator is a NUL, without which owner 'a' + name 'b_c' and owner 'a_b' + name 'c' collide; and both call sites pass record.owner_did / record.name rather than the request path segments, since get_repo normalizes DID aliases and the raw segments would mint two keys for one directory. The sanitization is deliberately not injective. Two did:web spellings can fold to one slug, which would misroute encrypt coalescing if it were reachable, but repos.disk_path is NOT NULL UNIQUE and holds exactly this derivation, so two such rows cannot coexist. A test pins that fold in place: making the key injective is what would let two owners sharing one objects/ directory push concurrently.
…he handler future The global and per-source walk permits were handler locals, so a client disconnect released them the moment the future dropped while the uncancellable spawn_blocking probe/walk/read kept running against a slot the pool now considered free. A disconnect flood could admit replacements while prior blocking git children still occupied threads and PIDs, weakening GITLAWB_MAX_CONCURRENT_IPFS_WALKS and GITLAWB_IPFS_WALK_PER_SOURCE on a surface any unauthenticated caller can reach. Hold the pair in an Arc and clone it into each of the three blocking closures. The permits release when the last clone drops, which collapses both failure directions into one case: a disconnect leaves the abandoned closure holding the slot, and a panicking closure leaves the handler holding it. No loop arm changes and the JoinError arms keep today's taint-and-continue. A move-and-return shuttle was implemented and rejected. It is correct only if every arm that continues the loop re-binds the returned value, which the compiler cannot enforce - the absent-object arm Ok(Ok(None)) => continue, taken on nearly every iteration of a random-CID scan, drops it - and it forces a second decision about the panic arms whose least-bad answer would let one broken repo shed every other owner's /ipfs lookups. The regression strengthens the existing admission test: it now asserts the slot is still held after the future is dropped and before the child is killed, with a child-still-alive precondition so it cannot pass vacuously. Only the walk site runs under state.git_bin and can be pinned that way; the probe and read deliberately shell to real git, so a source-scan gate in inv22_gates.rs binds all three clone sites instead. Removing any one clone turns that gate red.
…efs under git_bin Two single-site wiring fixes from the same review round. withheld_recipients_gated held its git_encrypt_semaphore permit in the async frame and then awaited spawn_blocking, so dropping the future released the permit while the uncancellable walk kept running. Every other post-receive scan helper moves the permit into the closure - replication_withheld_set says so in as many words - so this aligns the last site with the F4 contract. The regression drops the future mid-walk and asserts the pool stays at zero while the git child is still alive. git_info_refs passed a literal "git" to smart_http::info_refs while upload-pack and receive-pack both pass state.git_bin, so no fake-git harness could drive the advertisement path. The regression points git_bin at a fake emitting a distinctive advertisement and asserts it reaches the response body; before the fix the body came back from real system git (agent=git/2.43.0).
The post-receive replication tail was spawned at the end of git_receive_pack, after touch_repo, the trust-score update, per-ref certificate issuance and webhook fan-out. A client or proxy disconnect anywhere in that window dropped the tail, and with it this push's pins, recovery copy, and announcements - even though the push was already durable on disk. Spawn it immediately after the receive_result ? instead. That is the first point where the push is known to have landed: guard.release() a few lines up runs on failure too, so anchoring there would spawn a tail for a push git rejected, pinning and announcing a half-applied repo - the state release() deliberately refuses to upload to Tigris, and one a hostile pusher can produce on demand by aborting a pack mid-transfer. RefUpdate gains Clone so the tail takes the parsed updates while the certificate and webhook loops keep iterating their own copy. This makes the tail run concurrently with certificate issuance rather than after it, so a ref can be announced before its signed certificate exists. Accepted, and stated here rather than left for a reviewer to find: cert issuance already fails open (errors are logged and skipped) and the gossip event carries cert_id: None regardless, so no announce consumer reads a certificate out of it. Evidence is an ordering gate in inv22_gates.rs rather than a cancellation-race test, matching the F3 gate's instrument: forcing a disconnect to land inside the cert/webhook window is timing-dependent and would be flaky, while the ordering is precisely what changed. The gate is mutation-verified in both directions - moving the spawn back below the webhooks fails the upper bound, hoisting it above the ? fails the lower one.
The key is logged as the `repo` field on the lease waiter-cap shed and the steal-bound warning (state.rs), and it was joined with a NUL. Those are the two messages an operator reads to find a contended repo, and a NUL-hostile log sink truncates the field at the separator, rendering two different repos' warnings identically. Before this PR that field was a UUID or a mirror id, both printable, so the NUL was a regression this change introduced. Join with '/' instead. It cannot occur in the owner slug by construction - the slug is owner_did.replace([':','/'], "_") - and it mirrors the shape of the disk path the key exists to reproduce. A test asserts the key carries no control characters so this cannot regress silently. Rotating the U2 lease test's row to a UUID id is not incidental: the mirror path that seeds it uses 'owner_short/name' as the row id, which now coincides with the identity key and would leave the test unable to tell the two keys apart. The rotated id is also the real shape of the bug. Re-verified RED against the reverted call site after the change.
|
All six addressed on [P2] Hold I tried the move-and-return shuttle you'd expect first and backed it out. It is only correct if every arm that continues the loop re-binds the returned value, and the compiler cannot enforce that. My first cut of the rule said "every error arm" and missed
[P2] Key the lease and encrypt coalescing on the disk path. Fixed. Added The sanitization is not injective. [P2] Write encrypted-pin metadata under the re-fetched repo id. Fixed for the pin write, and I'm declining the anchor half. [P3] Move the encrypt-walk permit into the blocking closure. Fixed, matching [P3] Spawn the post-receive replication tail before the certificate window. Fixed, and this one has a deliberate design call in it worth stating rather than leaving to be found. The spawn is anchored on The tail now runs concurrently with certificate issuance rather than after it, so a ref can be announced before its certificate exists. I'm accepting that window: cert issuance already fails open (errors are logged and skipped at Evidence here is an ordering check rather than a cancellation-race test, and I'd rather say so than imply otherwise. Forcing a disconnect to land inside the cert window is timing-dependent and would be flaky; the ordering is what actually changed. It's pinned in both directions: moving the spawn below the webhooks fails the upper bound, hoisting it above the [P3] Honor Accepted residual. The Full suite green (662 plus the structural checks), clippy clean. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the detailed 62ec882 follow-up — I took your writeup as authoritative for this round and re-verified each item on current head against main (111cff7e..62ec882).
Rechecked the six items from my prior review. Five are fully addressed on this head as you described; one adjacent gap remains (below). I am not re-raising the other five.
Findings
- [P2] Spawn the replication tail before
guard.release(), not after it
crates/gitlawb-node/src/api/repos.rs:1806-1852
The tail is now correctly beforetouch_repo, certificates, and webhooks, but it still sits afterguard.release(receive_result.is_ok()).awaitanddrop(lease). On a successfulreceive-pack,release(true)can await a Tigris upload (and advisory unlock) while the handler future is still tied to the client connection. If that future is cancelled/disconnected in that window, the push has already landed locally yettokio::spawn(post_receive_replication_tail(...))never runs — the same dropped-tail class F2 was meant to close, just shifted earlier in the handler.
You already ruled out anchoring the spawn onrelease()becauserelease()also runs on failure; the fix is not to move the spawn belowrelease(), but to move it above it while still gating on success: checkreceive_result.is_ok()(or take the?on a cloned/inspected result), spawn the tail, then awaitguard.release(...). That preserves the failed-push no-spawn invariant and removes the cancellable gap between a landed push and tail ownership.
Prior round — verified on 62ec882
/ipfs walk admission through blocking work. WalkAdmission is shared via Arc and cloned into all three spawn_blocking closures (ipfs.rs:164-179, 412-416, 493-496, 567-571). get_by_cid_walk_permit_held_through_blocking_walk passes locally. inv22_ipfs_walk_admission_reaches_every_blocking_site binds all three sites.
Stable disk identity for lease and encrypt coalescing. repo_identity_key(&record.owner_did, &record.name) keys both repo_write_leases.acquire (repos.rs:1711-1714) and encrypt_inflight.try_begin (repos.rs:1992-1993). u2_lease_registers_under_the_disk_identity_not_the_row_id proves the lease registers under the identity key after an id rotation.
Coalesced-drain pin metadata under the re-fetched repo id. resolve_drain_object_list returns record.id from the owner/name re-fetch (repos.rs:930-1021), and pin_and_encrypt_objects takes repo_id explicitly (repos.rs:1157-1204). u3_drain_resolves_the_refetched_repo_id_after_an_id_rotation covers the drain path. I agree the anchor half does not carry a repo id in EncryptedManifest.
Encrypt-walk permit inside the blocking closure. withheld_recipients_gated moves the acquire_owned permit into spawn_blocking (repos.rs:829-837).
state.git_bin on info/refs. git_info_refs calls smart_http::info_refs(&state.git_bin, …) (repos.rs:731-732). u6_info_refs_runs_the_configured_git_binary fails if the literal "git" is restored.
Accepted residuals (not re-raised)
- Post-receive scan work remains inline with F5 coalescer offload tracked separately;
acquire_scan_permit's unbounded defer queue is documented instate.rs:842-848. drop(lease)before the detached tail/Pinata workers is intentional so a second push is not blocked for the full tail duration; concurrent local git reads during a subsequent write are a known tradeoff called out in the handler comments./ipfsprobe/read use realgitwhile the walk usesstate.git_bin;inv22structure-only coverage does not exercise customGITLAWB_GIT_BINon probe/read (author-accepted residual).- Brief announce-before-certificate window; cert issuance fails open and gossip carries
cert_id: None. - Coalesced encrypt drain drops pending work on repo/DB miss or abnormal task exit (fail-closed, logged, no sweep) — matches the stated F5 semantics.
- README,
config.rs, and.env.examplestill say/ipfsprobe/read subprocesses have no per-subprocess duration bound; code on this head usesobject_type_bounded/read_object_content_boundedwithrun_bounded_git_rawdeadlines (get_by_cid_hung_probe_is_reaped_and_sheds_503proves reaping). Doc drift only — production behavior is bounded. git_read_semaphore's field comment still attributes receive-packinfo/refsto the read pool; routing usesgit_push_advert_semaphore(repos.rs:581,685). Comment drift only.- README settings table omits four served-git concurrency knobs present in
.env.example(GITLAWB_MAX_CONCURRENT_GIT_OPS,GITLAWB_MAX_CONCURRENT_GIT_PUSHES,GITLAWB_MAX_CONCURRENT_READS_PER_CALLER,GITLAWB_MAX_CONCURRENT_PIN_TASKS);.env.exampleremains the authoritative operator template. pin_new_objects_gatedholds a global pin permit over pre-existing unboundedstore::read_objectcalls (ipfs_pin.rs:121); F6 caps concurrent pin lists but not per-object git duration. Out of scope for this PR's served-git/smart-http hardening unless you want parity with/ipfs.- Initial replication tail keys rules and
EncryptTaskCtx.repo_idoff the handler's push-timerecordwhile U3 re-fetches on coalesced drain; a delete+recreate under the same slug during a longreceive-packis a narrow theoretical window and likely fail-closed on empty rules. - Several
u5encrypt-coalescing tests calltry_begin(&rec.id, …)on mirror-seeded repos whererec.id == repo_identity_key(owner, name), so a production regression back to row-id coalescing for canonical UUID repos would not be caught;f2a_coalesced_push_still_pins_and_announcesandu2cover the identity-key paths but there is no coalesce test with a rotated UUID id.
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