feat(node): per-IP write-surface rate brake + purge-spam admin tool#196
feat(node): per-IP write-surface rate brake + purge-spam admin tool#196beardthelion wants to merge 25 commits into
Conversation
…utes Introduces a dedicated write_rate_limiter bucket (GITLAWB_WRITE_RATE_LIMIT, default 600/hr, 0 disables), separate from the creation brake so a write flood cannot drain the creation budget (KTD-1). Attaches rate_limit_by_ip to write_routes, mirroring creation_routes. Per-DID deliberately not paired (a DID farm never trips it). Tests (route-level, execution-verified against Postgres): a write_routes sink (PUT /star) is 429'd before auth; an exhausted write bucket does not throttle repo creation (separate-bucket property).
Attaches rate_limit_by_ip (write bucket) to task_write_routes, issue_write_routes, bounty_write_routes, profile_write_routes, and the /graphql POST/GET surface (KTD-5 — an HTTP-layer brake counts every /graphql request; /graphql/ws subscriptions stay unbraked). write_ip_limiter is now built once and shared. Every non-git, non-creation authenticated write group is braked. Tests (execution-verified against Postgres): /graphql POST is 429'd, and a representative REST write sink (issue comment) is 429'd, by the write brake.
…rake Middleware-level tests complementing the client_key unit coverage: a spoofer varying the client-controlled leftmost X-Forwarded-For hop cannot rotate its bucket key (keyed on the trusted rightmost hop); distinct trusted client hops get distinct buckets (no cross-starvation); and a request with no trusted header in a proxy mode falls back to the socket-peer key — the documented collapse, asserted by name so any change is deliberate. Operator-warning on missing-header deferred deliberately: a per-request warning is a log-noise risk and the security-load-bearing fallback (peer key, never skip) is what these tests lock down.
Adds a purge-spam subcommand (clap #[command(subcommand)] split; no-subcommand path still runs the node unchanged). Candidate selection is signature-based, not per-DID: a repo qualifies only if owned by the named burst DID AND verified empty (zero git refs) per repo. A hard exclusion gate runs BEFORE the empty check, so an empty repo owned by the content-bearing user or the mirror-bot DID is never a candidate. Dry-run is the default; deletion needs an explicit --execute flag and operates per-repo, never 'all repos of DID X'. Tests (RED->GREEN, exclusion gate confirmed load-bearing by targeting an excluded DID directly): empty target listed; target-with-refs spared; empty excluded/ intern repos spared (must-assert); dry-run deletes nothing; execute removes only the empty target on disk.
P1 from code review: on_disk_ref_count shelled 'git for-each-ref' with only the repo path as cwd and no --git-dir, so a path that exists but is not a bare repo let git discover a parent .git (repos_dir may sit inside the operator's checkout) and read a DIFFERENT repo's refs — possibly 0, deleting a real repo. Now require the bare-repo markers (HEAD file + objects/ dir) before trusting any count; anything else fails closed (skipped). Tests: a non-git dir under a git ancestor (zero-ref) now fails closed — verified RED without the guard (read the ancestor's 0 refs); target-DID-never-excluded invariant pinned; write_flood test anchored to a genuinely-drained bucket so its assert_ne can't pass vacuously.
…rage Addresses the remaining code-review findings: - purge-spam DID matching now uses did:key normalization on both sides (list_repos_by_owner_did via OWNER_KEY_CASE_SQL, is_excluded and the target scope via normalize_owner_key), so short/full form is consistent and an excluded identity is protected in either form. Adds a short-form exclusion test. - Extract the per-IP write brake into a chainable WriteBraked helper so a future write group can't silently ship unbraked (kept separate from add_auth_layers by design; documented why). - Mark --repos-dir / --database-url global so admin subcommands accept them after the subcommand (was an 'unexpected argument' parse error); verified. - Document GITLAWB_WRITE_RATE_LIMIT in .env.example incl. the per-IP NAT-aggregate caveat so operators can size it. - Coverage: adoption-floor (under-limit write passes), write-limit=0 disables, task/bounty/profile route-level 429s, /graphql/ws stays unbraked. Full suite 927 pass / 0 fail; fmt + clippy -D warnings clean.
- purge-spam execute now re-verifies emptiness immediately before delete (TOCTOU: a push between selection and delete no longer risks deleting a now-non-empty repo) and removes the on-disk bare repo dir so DB and disk stay consistent; a per-repo delete failure warns and continues rather than aborting the batch. - Tests (RED-verified where they cover new behavior): partition_for_delete skips a repo no longer empty; execute removes the on-disk dir (RED without the removal); list_repos_by_owner_did finds a short-form burst row (RED without the SQL normalization); every braked write group passes under-limit (per-group grant path, not just the star representative).
📝 WalkthroughWalkthroughChangesThe pull request adds a Spam purge administration
Authenticated write rate limiting
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant AdminCLI
participant PurgeRunner
participant Db
participant RepoStore
AdminCLI->>PurgeRunner: purge-spam
PurgeRunner->>Db: list target repositories
PurgeRunner->>RepoStore: acquire lock and refresh archive
PurgeRunner->>Db: delete repository and descendants
PurgeRunner->>RepoStore: remove local directory and archive
sequenceDiagram
participant Client
participant Router
participant WriteLimiter
participant WriteRoute
Client->>Router: authenticated write request
Router->>WriteLimiter: resolve IP and check write bucket
WriteLimiter->>WriteRoute: forward permitted request
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
beardthelion
added
crate:node
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/main.rs (1)
440-455: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
write_rate_limiteris missing from the periodic cleanup loop.Every other limiter (
rate_limiter,create_ip_rate_limiter,push_rate_limiter,sync_trigger_rate_limiter,peer_write_rate_limiter) is swept here, but the newly addedwrite_rate_limiteris not. It is bounded, so no unbounded leak, yet stale keys are only evicted lazily on a capacity miss instead of proactively like its peers — an inconsistency worth closing.🧹 Proposed fix
let peer_write_rl = state.peer_write_rate_limiter.clone(); + let write_rl = state.write_rate_limiter.clone(); let db = state.db.clone();peer_write_rl.cleanup().await; + write_rl.cleanup().await;🤖 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/main.rs` around lines 440 - 455, Include the state.write_rate_limiter in the periodic cleanup task alongside the other cloned rate limiters. Clone it before spawning the task and invoke its cleanup method in the five-minute tokio::select! interval, preserving the existing cleanup behavior for all other limiters.
🧹 Nitpick comments (1)
crates/gitlawb-node/src/api/repos.rs (1)
2629-2966: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting a shared test-request helper to reduce duplication.
All 9 new tests repeat the same
Request::builder()+ConnectInfoinsertion +oneshot()+ status-assert boilerplate.rate_limit.rsin this same PR already factored this pattern intopost_from/post_withhelpers — a similar helper here (e.g.,send_from(&router, method, uri, body, peer)) would cut most of the repetition acrosswrite_route_is_rate_limited_by_ip,write_flood_does_not_drain_creation_budget,graphql_post_is_rate_limited_by_ip,issue_comment_is_rate_limited_by_ip,under_limit_write_is_not_throttled,write_rate_limit_zero_disables_the_brake,task_bounty_profile_writes_are_rate_limited,graphql_ws_is_not_braked, andevery_write_group_passes_under_limit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gitlawb-node/src/api/repos.rs` around lines 2629 - 2966, Reduce duplicated request setup across the listed rate-limit tests by adding a shared test helper such as send_from that accepts the router, HTTP method, URI, body, and peer, inserts ConnectInfo, sends via oneshot, and returns the response or status. Update each named test to use this helper while preserving its existing request bodies, methods, URIs, and status assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/gitlawb-node/src/main.rs`:
- Around line 440-455: Include the state.write_rate_limiter in the periodic
cleanup task alongside the other cloned rate limiters. Clone it before spawning
the task and invoke its cleanup method in the five-minute tokio::select!
interval, preserving the existing cleanup behavior for all other limiters.
---
Nitpick comments:
In `@crates/gitlawb-node/src/api/repos.rs`:
- Around line 2629-2966: Reduce duplicated request setup across the listed
rate-limit tests by adding a shared test helper such as send_from that accepts
the router, HTTP method, URI, body, and peer, inserts ConnectInfo, sends via
oneshot, and returns the response or status. Update each named test to use this
helper while preserving its existing request bodies, methods, URIs, and status
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e186f361-729a-419c-b633-a90e12b77a44
📒 Files selected for processing (11)
.env.examplecrates/gitlawb-node/src/admin.rscrates/gitlawb-node/src/api/repos.rscrates/gitlawb-node/src/auth/mod.rscrates/gitlawb-node/src/config.rscrates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/main.rscrates/gitlawb-node/src/rate_limit.rscrates/gitlawb-node/src/server.rscrates/gitlawb-node/src/state.rscrates/gitlawb-node/src/test_support.rs
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
High — fix before merge
H1. Read/fork handlers run blocking git subprocesses directly on the tokio runtime (no spawn_blocking, no timeout)
fork_repo—crates/gitlawb-node/src/api/repos.rs:1567-1575invokesstd::process::Command::new("git").args(["clone","--mirror",…]).output()inside the async handler.output()blocks the calling worker thread;git clone --mirrorof a non-trivial repo can take minutes and there is no timeout, unlike every other git walk in this file which correctly usestokio::task::spawn_blocking(repos.rs:67, 110, 661, 1143) ortokio::process::Command(info/refs, upload-pack, receive-pack).- Read metadata handlers —
list_commits/get_tree/get_blobcallstore::{resolve_head, log, ls_tree, read_file}(api/repos.rs:360-361, 397, 443, 481) which are synchronousstd::process::Command::output()calls (crates/gitlawb-node/src/git/store.rs:92-231). A slow disk or large repo blocks a worker thread for seconds; with a finite worker pool a handful of concurrent/commits,/tree,/blob, or a single/forkcan starve the whole runtime, including/healthand/ready(same runtime inbuild_router). - Impact: availability/reliability regression, not just latency.
git_service_timeout_secsis bypassed on these paths. - Fix: wrap the blocking
storecalls (and the fork clone) intokio::task::spawn_blockingand bound withtokio::time::timeout(git_service_timeout_secs, …), returningAppError::Timeout.
H2. write_rate_limiter is never reaped by the background cleanup loop
crates/gitlawb-node/src/main.rs:440-455— the periodic task clones exactly five limiters (rate_limiter,create_ip_rate_limiter,push_rate_limiter,sync_trigger_rate_limiter,peer_write_rate_limiter) and calls.cleanup()on each.state.write_rate_limiter(declared atmain.rs:329-333, wired intoAppStateat:404) is omitted, even though it protects the entire authenticated non-creation write surface +/graphql(server.rs:84-98, 110, 154+).- The
RateLimiterdesign makes the inline capacity sweep amortized and explicitly relies on the background loop for full reclamation (rate_limit.rs:36-39, 94-114). With no background sweep,write_rate_limiteronly reclaims expired keys once its map hits the 200,000-key cap. Until then every distinct client IP persists as a staleWindowfor the full 1-hour window and beyond → steady memory growth (bounded at the cap) and a latent correctness cliff: once at cap, the amortized inline sweep becomes the only reclaimer and a distinct-IP write flood serializes behind O(max_keys) scans while legitimate new IPs get spuriously 429'd between sweeps. - Fix: add
let write_rl = state.write_rate_limiter.clone();andwrite_rl.cleanup().await;inside the cleanup tick beside the other five.
Medium — should fix
M1. fork_repo / create_repo orphan on-disk repos (and a spent proof / uploaded object) on late DB failure
fork_repoordering (api/repos.rs:1555 → 1567 → 1585 → 1605): spend proof →git clone --mirrortodisk_path→release_after_write(Tigris upload) →db.create_repo. Ifcreate_repofails, the on-disk mirror and the already-uploaded Tigris object are left with no DB row, and the single-use iCaptcha proof is already consumed.create_repo(api/repos.rs:208 → 210-219 → 236):proof.consumethenrepo_store.init(blockinggit init) happen beforedb.create_repo. A DB failure orphans the freshly-initialized directory.inititself is also a blocking git call on the async runtime (same class as H1).- Fix: make disk materialization and proof spend adjacent to a guaranteed-successful DB write, and on
db.create_repofailure best-effortremove_dir_all(disk_path)(and prefer deferring the Tigris upload until after the row commits).
M2. TrustedProxy IP trust is config-asserted, not network-enforced — header rotation defeats every per-IP brake
crates/gitlawb-node/src/rate_limit.rs:198-218(client_key): inFly/XForwardedFormodes the limiter key is taken entirely fromFly-Client-IPor the rightmostX-Forwarded-Forhop, and only falls back to the socket peer when the header is absent. There is no check that the socket peer (ConnectInfo) is actually the operator's proxy. IfGITLAWB_TRUSTED_PROXYis set but the node's HTTP port is reachable directly (misconfigured firewall, exposed Fly port, second ingress, internal attacker), a client can setFly-Client-IP: <random>/ append any rightmostX-Forwarded-Forhop and get a fresh bucket per request, evading the creation/write/push/sync brakes (the only thing stopping a single-source spam/push flood). The unit tests cover header parsing but never assert "peer not the proxy → key on peer."- Fix: honor the forwarded header only when
ConnectInfopeer is in a configured proxy CIDR/IP allowlist; otherwise key on the socket peer (fail closed). Add a test for "trust mode set, request from a non-proxy IP." - Note (validation): exploitability is deployment-dependent (Fly's edge does overwrite
Fly-Client-IP), so rated Medium not Blocker.
M3. purge-spam --execute deletes via remove_dir_all on an unsanitized repo_name (path traversal → arbitrary repo deletion)
crates/gitlawb-node/src/git/store.rs:450-453—repo_disk_pathsanitizesowner_did(replace([':', '/'], "_")) but joinsrepo_nameverbatim viaformat!("{repo_name}.git")with no../slash/dot validation.crates/gitlawb-node/src/admin.rs:243-244callsstd::fs::remove_dir_all(store::repo_disk_path(&repos_dir, &c.owner_did, &c.name))on that path. The fail-closed marker guard (admin.rs:128) only requires aHEADfile +objects/dir — i.e. any existing legitimate bare repo satisfies it. API names are validated (api/repos.rs:189-197), butsmart_http_repo_nameexplicitly notes repos are "creatable via the peer mirror path, which skips API name validation" (api/repos.rs:490-494), so a burst-DID row with a../name can be planted, then deleted outsiderepos_dir.- Fix: validate
repo_name(and re-validateowner_did) the same way the serving paths do (repo_store::validate_repo_name/validate_path_components), and assert the resolved path is canonically insiderepos_dirbefore anyremove_dir_all. Ideally makerepo_disk_pathitself reject unsafe names. - Note: a destructive
remove_dir_allshould never run on a DB-sourced, unsanitized name. End-to-end exploitability is narrow (requires the burst DID + marker check at the traversed path), so rated Low/Medium — but it is a clear defense-in-depth defect.
M4. purge-spam TOCTOU: emptiness re-checked once as a batch snapshot, not immediately before each delete — races a live push
crates/gitlawb-node/src/admin.rs:221-235—partition_for_delete(:224-226) re-verifies emptiness for all candidates up front, producingto_delete; the actualdelete_repo_by_idruns later in a loop (:235). Between the snapshot and a given delete, a concurrentgit-receive-packon the running node can add a ref, making the repo non-empty — yet it is still deleted (silent data loss). The comment claims it "re-verify[s] emptiness immediately before deleting," but the recheck is a single batch pass, not a per-candidate check immediately beforedelete_repo_by_id.- The execute path also does not acquire the node's
acquire_writeadvisory lock (api/repos.rs:927), so it races a live push: a push can leave orphaned Tigris objects / ref-certificate rows referencing a now-deletedrepos.id, andremove_dir_allcan race an in-flightreceive-packwritingobjects/. - Fix: re-verify
ref_count_on_diskimmediately before eachdelete_repo_by_id(or wrap recheck+delete of each repo in arepo_store.acquire_writeguard / DB transaction re-checking a ref signal). Document that the tool must be run against a quiesced node for the target DID.
M5. iCaptcha gate fails open in enforce mode when the public key cannot load
crates/gitlawb-node/src/icaptcha.rs:302-304— insidedecide,if v.key.is_none() { return Decision::Allow; }runs before the mode match, so inMode::Enforcea missing key (iCaptcha unreachable at startup, DNS/BGP block, badICAPTCHA_PUBKEY) allows every create/registration request with no proof.init()(icaptcha.rs:160-166) leaveskey = Noneand only warns. Since the gate defaults tooff, the default deployment already has no captcha; enforce-mode is now trivially bypassable by denying the key fetch, directly weakening the abuse protection this PR leans on after PoW removal.- Fix: in enforce mode fail closed (reject) when
key.is_none(); or refuse to start in enforce mode without a key. At minimum document that enforce+no-key == fail-open.
M6. purge-spam deletes the DB row, then removes the dir; a disk failure is swallowed while the deleted counter is already incremented
crates/gitlawb-node/src/admin.rs:235-248— onOk(n)the row is counted asdeletedbeforeremove_dir_all, which on error onlywarn!s. The final "deleted N repo row(s)" summary therefore reports success even when the on-disk repo survives → DB/disk drift and a misleading operator-facing count. No retry / non-zero exit signal.- Fix: remove the on-disk dir first (or track disk-failure separately from the DB-deleted count) and surface "N deleted, M failed."
M7. delete_repo_by_id (and purge-spam) leave dependent rows orphaned
crates/gitlawb-node/src/db/mod.rs:1288-1294—DELETE FROM repos WHERE id = $1with no cascade. Child tables keyed onrepo_id(branch_cids,ref_certificates,visibility_rules,repo_stars,encrypted_blobs,received_ref_updates,repo_replicas,arweave_anchors,webhooks,pull_requests) keep orphan rows that accumulate and can be re-joined if a repo with the same id is later created.- Fix: add
ON DELETE CASCADE(or explicit child deletes) for therepo_idFKs, ideally within the same transaction as the row delete.
Low / Info — nice to have
- L1. Rate-limiter bounded-map DoS.
rate_limit.rs:96-114rejects new keys when at the 200k cap; a distinct-key flood (IPv6 variation or the M2 header rotation) then 429s legitimate new clients for up to a full window. Consider evicting expired entries to admit new keys, or a metric on cap-full rejections. - L2. Limiter state is per-process / in-memory (
rate_limit.rs:24-41). In multi-machine Fly deployments the effective per-IP/per-DID limit isN × configuredand a restart wipes buckets. Known tradeoff — document explicitly; consider shared counters for the spam-relevant paths. - L3. Non-atomic
create_repo.api/repos.rs:203-236check-then-insert with noUNIQUE(owner_did, name); theidx_repos_owner_key_nameis a plain index (db/mod.rs:474-490). A concurrent create race surfaces a rawUNIQUE(disk_path)violation mapped to a generic 500 instead ofRepoExists(409). - L4. Read handlers mask git errors as empty
200 OK.api/repos.rs:361(store::log(...).unwrap_or_default())and theget_treesiblings coerce real backend failures into empty lists, unlikeget_blob(api/repos.rs:398-410) which classifies errors. MapErrtoAppError::Git/NotFound. - L5.
parse_ref_updatessilently tolerates malformed/truncated bodies (api/repos.rs:1651-1699) → emptyVec<RefUpdate>; receive-pack then proceeds with zero ref updates (skipping branch-protection/owner-push checks). Return400on a non-empty body that parses short. - L6. Push bookkeeping errors swallowed (
api/repos.rs:959, 977, 982, 999-1005let _ =/ only-warn!) — transient DB blips silently loserecord_push/update_trust_score. At least raise log severity / add a durable outbox. - L7.
git_info_refsreceive-pack brake skipped whenclient_keyreturnsNone(api/repos.rs:556/rate_limit.rs:198): if no trusted header and noConnectInfopeer, theif let Some(key)guard is skipped and the request proceeds unthrottled. Unreachable in prod (socket peer always present) but a divergent contract fromrate_limit_by_ip; document or reject instead of skip. - L8. Malformed rate-limit env vars silently fall back to defaults.
main.rs:304-307, 325-328, 343-346parseGITLAWB_CREATE/WRITE/PUSH_RATE_LIMITwith.ok().and_then(parse).unwrap_or(default); a fat-fingered value silently disables the intended (stricter) brake with no warning, unlike the clap-validated siblings. Warn (or fail fast) on present-but-unparseable values. - L9.
.env.exampledocs drift. MissingGITLAWB_IPFS_API,GITLAWB_TIGRIS_BUCKET,GITLAWB_METRICS_ADDR,GITLAWB_SHUTDOWN_GRACE_SECS(parsed inconfig.rs:88,161,188,194). Reverse direction is clean. Add with defaults. - L10. Weak dry-run safety test.
admin.rs:471-492(dry_run_deletes_nothing) seeds repos with an emptyrepos_dir, sorun_purge_spamhits the no-candidate early return before any delete logic — it does not exercise theif !execute { return }guard when candidates exist. Add a dry-run test with a real empty on-disk target repo so a regression that deletes in dry-run is caught. - L11. Unauthenticated
/metrics(main.rs:828-875,config.rs:187warns it is unauthenticated). If bound to a non-loopback addr it leaks peer/repo/push counts. Operator-controlled; note in hardening guidance. - L12. Git error strings returned to clients (
AppError::Git(e.to_string())sites, e.g.api/repos.rs:398-410). Can surface internal paths; return opaque messages, keep detail in server-side logs.
…ve_dir_all A peer-mirror row skips API name validation, so a burst-owned repo can carry a '../' name; repo_disk_path joined it verbatim and the emptiness marker check passed on the traversed target, so purge-spam selected and remove_dir_all'd a repo OUTSIDE repos_dir. Validate the name via repo_store::validate_repo_name at selection (fail closed -> non-candidate) and assert canonical containment inside repos_dir immediately before the destructive remove. Adversarial RED->GREEN: a victim bare repo reachable via '../../victim' is deleted before, survives after; symlink-escape covered by path_within.
delete_repo_by_id was a bare DELETE FROM repos with no cascade and the schema has no FKs, so every child row (ref_certificates, pull_requests + pr_reviews/ pr_comments, webhooks, push_events, protected_branches, repo_stars, repo_replicas, repo_labels, visibility_rules, encrypted_blobs, repo_icaptcha_proofs, agent_tasks, and the slug-keyed branch_cids/sync_queue/received_ref_updates/arweave_anchors) was orphaned and could re-join a later repo reusing the id. Wrap parent+child deletes in one transaction, resolving the owner_short/name slug for the repo-TEXT children. bounties (financial) and issue_comments (unmappable) are left intact by design. RED->GREEN test seeds a repo_id child, a slug child, and a PR grandchild.
The PR's new write_rate_limiter was the one limiter omitted from main()'s hand-maintained cleanup list, so its key map never shed expired entries until the 200k cap. Move the sweep into AppState::cleanup_rate_limiters (co-located with the limiter fields, single source of truth) and call it from the loop, so a newly-added limiter is reaped rather than silently dropped. Guard test seeds a reclaimable entry into write_rate_limiter and asserts the sweep reaps it — RED if write_rate_limiter is dropped from cleanup_rate_limiters (verified).
purge-spam took no write lock, so a concurrent receive-pack could land a ref between the emptiness recheck and the delete, silently losing a live push. Add RepoStore::try_lock_repo — a lock-only, non-blocking counterpart to acquire_write (no Tigris I/O) whose guard holds a dedicated pooled connection so the advisory lock and its unlock run on the same session. purge now locks each candidate, rechecks emptiness UNDER the lock, deletes, and releases; a repo held by a live writer is skipped, not force-deleted. RED->GREEN: a locked empty repo survives purge, then deletes once the writer releases (neuter-check confirms the lock is load-bearing).
…etes The Ok(n) arm incremented the deleted total before remove_dir_all, whose failure only warn!'d, so the summary reported N deleted even when the on-disk dir survived (DB/disk drift shown as clean success). run_purge_spam now returns a PurgeSummary and counts a failed (or containment-refused) on-disk removal in disk_failed, never folding it into deleted; the summary line surfaces 'M on-disk removal(s) failed'. RED->GREEN: a read-only parent dir forces the removal to fail; deleted=1 AND disk_failed=1 with the dir surviving (neuter-check confirms the counter is load-bearing).
…y defaulting GITLAWB_CREATE/WRITE/PUSH_RATE_LIMIT parsed with .ok().and_then(parse).unwrap_or, so a typo (e.g. 5O with a letter O) silently reverted to the default and left the operator believing a stricter cap was in force. Route all three through rate_limit_from_env, which distinguishes absent/empty (default, silent) from present-but-unparseable (default, WARN). Pure resolve_rate_limit is unit-tested across absent/empty/valid/zero/unparseable — 0 stays valid (its own ==0 warn handles the disable case).
…DOWN_GRACE_SECS in .env.example Four config knobs read in config.rs were absent from .env.example, so operators had no template for them. Add each in its matching section with the config default (INV-24: operator docs match the config the code reads, in the same PR that touches the file).
…present
dry_run_deletes_nothing ran against an empty repos_dir, so select_spam_candidates
found nothing and run_purge_spam hit the no-candidate early return — the
if !execute { return } guard was never exercised with candidates present (a
vacuous guard, INV-21). Materialize a real empty bare repo so a genuine candidate
exists, then assert dry-run leaves both the row and the on-disk dir. Verified
load-bearing: removing the !execute guard turns the test RED (dry-run deletes).
|
Pushed the fixes at Fixed in this PR
Deferred to follow-up issues (pre-existing, outside this PR's diff)
Declined
@jatmn ready for another look. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.env.example (1)
143-152: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDocument all GraphQL HTTP requests covered by this limiter.
The route wiring in
crates/gitlawb-node/src/server.rsapplies the brake to the whole/graphqlroute, so queries and the playground GET also consume this bucket—not only mutations. Update the text to say “GraphQL HTTP requests” or narrow the middleware.🤖 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 @.env.example around lines 143 - 152, Update the write rate-limiting documentation near GITLAWB_WRITE_RATE_LIMIT to state that the limiter covers all GraphQL HTTP requests on the /graphql route, including queries and the playground GET, rather than describing only GraphQL mutations. Keep the existing rate-limit scope and configuration details unchanged.
🧹 Nitpick comments (3)
.env.example (1)
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the dotenv keys in linter order.
dotenv-linterreports thatGITLAWB_METRICS_ADDRshould appear beforeGITLAWB_PORT; moving it will keep configuration checks clean.🤖 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 @.env.example at line 19, Move the GITLAWB_METRICS_ADDR entry in .env.example so it appears before GITLAWB_PORT, preserving the existing key definitions and values.Source: Linters/SAST tools
crates/gitlawb-node/src/main.rs (1)
292-345: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated per-IP limiter setup (create/write/push) into a helper.
The create/write/push blocks each repeat "resolve env limit → build
RateLimiter::new_bounded→ warn if zero" verbatim, just with different var names/messages. A small helper collapses the duplication and makes adding a fourth limiter trivial.♻️ Proposed refactor
+fn build_ip_limiter(limit: usize, var: &str, label: &str) -> rate_limit::RateLimiter { + let limiter = rate_limit::RateLimiter::new_bounded( + limit, + std::time::Duration::from_secs(3600), + 200_000, + ); + if limit == 0 { + tracing::warn!("{var}=0 — per-IP {label} rate limiting disabled"); + } + limiter +} + let create_limit = rate_limit_from_env("GITLAWB_CREATE_RATE_LIMIT", 120); - let create_ip_rate_limiter = rate_limit::RateLimiter::new_bounded( - create_limit, - std::time::Duration::from_secs(3600), - 200_000, - ); - if create_limit == 0 { - tracing::warn!("GITLAWB_CREATE_RATE_LIMIT=0 — per-IP creation rate limiting disabled"); - } + let create_ip_rate_limiter = + build_ip_limiter(create_limit, "GITLAWB_CREATE_RATE_LIMIT", "creation"); let write_limit = rate_limit_from_env("GITLAWB_WRITE_RATE_LIMIT", 600); - let write_rate_limiter = rate_limit::RateLimiter::new_bounded( - write_limit, - std::time::Duration::from_secs(3600), - 200_000, - ); - if write_limit == 0 { - tracing::warn!("GITLAWB_WRITE_RATE_LIMIT=0 — per-IP write rate limiting disabled"); - } + let write_rate_limiter = build_ip_limiter(write_limit, "GITLAWB_WRITE_RATE_LIMIT", "write"); let push_limit = rate_limit_from_env("GITLAWB_PUSH_RATE_LIMIT", 600); - let push_rate_limiter = rate_limit::RateLimiter::new_bounded( - push_limit, - std::time::Duration::from_secs(3600), - 200_000, - ); - if push_limit == 0 { - tracing::warn!("GITLAWB_PUSH_RATE_LIMIT=0 — per-IP push rate limiting disabled"); - } + let push_rate_limiter = build_ip_limiter(push_limit, "GITLAWB_PUSH_RATE_LIMIT", "push");🤖 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/main.rs` around lines 292 - 345, Extract the repeated create, write, and push per-IP limiter initialization into a shared helper that accepts the environment variable name, default limit, and disabled-warning context, then returns the bounded RateLimiter. Replace the three inline rate_limit_from_env/new_bounded/warn sequences with helper calls while preserving their existing limits, environment overrides, warning behavior, and bucket duration/capacity.crates/gitlawb-node/src/git/repo_store.rs (1)
428-446: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake repo lock release drop-safe
RepoLockGuard::release()dropspg_advisory_unlockerrors, and the manual guard still depends on every caller remembering.release(). Switching tosqlx::postgres::PgAdvisoryLock/PgAdvisoryLockGuard(or adding a logged, drop-safe fallback) would make leaked locks much harder to miss.🤖 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/repo_store.rs` around lines 428 - 446, Update RepoLockGuard and RepoLockGuard::release so advisory locks are drop-safe: use sqlx’s PgAdvisoryLock/PgAdvisoryLockGuard where compatible, or add a Drop-based fallback that attempts cleanup and logs unlock failures. Ensure callers no longer depend solely on remembering release(), while preserving unlocking on the same connection.
🤖 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.
Outside diff comments:
In @.env.example:
- Around line 143-152: Update the write rate-limiting documentation near
GITLAWB_WRITE_RATE_LIMIT to state that the limiter covers all GraphQL HTTP
requests on the /graphql route, including queries and the playground GET, rather
than describing only GraphQL mutations. Keep the existing rate-limit scope and
configuration details unchanged.
---
Nitpick comments:
In @.env.example:
- Line 19: Move the GITLAWB_METRICS_ADDR entry in .env.example so it appears
before GITLAWB_PORT, preserving the existing key definitions and values.
In `@crates/gitlawb-node/src/git/repo_store.rs`:
- Around line 428-446: Update RepoLockGuard and RepoLockGuard::release so
advisory locks are drop-safe: use sqlx’s PgAdvisoryLock/PgAdvisoryLockGuard
where compatible, or add a Drop-based fallback that attempts cleanup and logs
unlock failures. Ensure callers no longer depend solely on remembering
release(), while preserving unlocking on the same connection.
In `@crates/gitlawb-node/src/main.rs`:
- Around line 292-345: Extract the repeated create, write, and push per-IP
limiter initialization into a shared helper that accepts the environment
variable name, default limit, and disabled-warning context, then returns the
bounded RateLimiter. Replace the three inline
rate_limit_from_env/new_bounded/warn sequences with helper calls while
preserving their existing limits, environment overrides, warning behavior, and
bucket duration/capacity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0ed69896-8eea-4a8f-802d-9ceb9db33022
📒 Files selected for processing (7)
.env.examplecrates/gitlawb-node/src/admin.rscrates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/git/repo_store.rscrates/gitlawb-node/src/main.rscrates/gitlawb-node/src/rate_limit.rscrates/gitlawb-node/src/state.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/gitlawb-node/src/admin.rs
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 the writer's advisory-lock session in its guard
crates/gitlawb-node/src/git/repo_store.rs:156
acquire_writeobtains a session-scoped advisory lock through&self.pool, then immediately returns that connection to the pool;RepoWriteGuard::releaselater also uses an arbitrary pooled connection. The new purge guard can therefore receive the writer's idle lock-owning session and re-acquire the lock reentrantly, pass its final empty check, and remove the DB row and bare directory while a receive-pack/merge/issue write is still using its guard. Retain the acquiredPoolConnectionforRepoWriteGuard(asRepoLockGuarddoes) and add contention coverage that holds a realacquire_writeguard while purge runs. -
[P1] Make purge-spam verify and remove the configured Tigris object
crates/gitlawb-node/src/main.rs:625
The admin command deliberately constructs itsRepoStorewithNone, so selection and both rechecks inspect only the local cache and deletion removes only that cache plus the DB row. On a Tigris deployment, a locally empty but remotely updated repo can be deleted despite current remote refs; the archive also remains after a successful purge and can be downloaded into a later repo with the same owner/name. Initialize the configured Tigris client for this command, refresh/fail closed under the per-repo lock, and delete or otherwise tombstone the archive with explicit failure handling.
acquire_write took the session-scoped advisory lock through the shared pool and returned the lock-owning connection to the pool, so release unlocked on a different connection (silent no-op) and a concurrent try_lock_repo could be handed the idle lock-owning connection and reentrantly re-grab the lock, defeating the purge tool's mutual exclusion. Pin a dedicated PoolConnection for the guard's lifetime and unlock on it, mirroring RepoLockGuard. Load-bearing contention test (single-connection pool to force the reentrant grab deterministic): RED (Ok(Some)) before, GREEN after.
Extract an async ObjectStore trait (exists/upload/download/delete) that TigrisClient implements, and hold Option<Arc<dyn ObjectStore>> in RepoStore instead of Option<TigrisClient>. A dyn async trait needs async-trait (added as a direct dep; already transitively present), which E0038 would otherwise reject. Pure refactor — no behavior change; full suite stays green. Enables an in-test fake for the purge Tigris work (U3). Cargo.lock also picks up the pre-existing 0.5.0->0.5.1 version sync (#185: the committed lock was stale vs the manifests) that any cargo run regenerates.
purge-spam built its RepoStore with no object store, so on a Tigris deployment it rechecked emptiness against possibly-stale local disk (deleting a repo with live remote refs) and left the archive behind (to be re-downloaded into a later same-owner/name repo). Give the admin command the configured object store, refresh the local copy from the authoritative archive under the per-repo lock before the recheck (fail-closed on a store error via a new skipped_store_error), and delete the archive on a successful purge (failures counted in a new archive_failed, never folded into success). When no bucket is configured the None path is byte-for-byte unchanged. Three load-bearing guards verified RED->GREEN on the Tigris-blind purge: remote-refs-not-deleted, archive-deleted, fail-closed-on-unreachable.
Two regressions the connection-pinning fix (010c625) introduced, caught in pre-push review: - Pool pressure: acquire_write held its pinned connection through the whole 60s lock-retry backoff; pre-change the retry probed via the pool and returned the connection between attempts. Under a same-repo write burst every spinner now held a pool connection through its sleeps and could starve the shared pool. Restructure the loop to return the connection between FAILED attempts and keep it only on the winning one. - Lock leak: on the object-store download-error early-return the pinned connection dropped back to the pool with the advisory lock still held (no guard Drop runs pre-construction, and sqlx does not release session locks on connection return), blocking every future write to that repo until the pool reaped the connection. Unlock on the pinned connection before bailing. Add acquire_write_guard_excludes_try_lock_across_connections: a multi-connection test proving the exclusion holds across sessions (the production topology), which also catches a pin-but-don't-lock regression the single-connection test cannot.
|
Both fixed at P1a — advisory-lock session pinned. Two things surfaced while pinning the connection, both fixed in the same push. The retry loop now returns the connection to the pool between failed attempts rather than holding it through the 60s backoff (otherwise a same-repo write burst starves the pool), and the object-store-download error path unlocks before bailing (otherwise the lock orphaned on the returned connection until the pool reaped it, since sqlx doesn't release session locks on connection return). Tradeoff worth flagging: a held write now pins one pool connection for the operation's duration, so P1b — purge is object-store-authoritative. The admin command now builds the configured object store (and refuses to run local-only when a bucket is set), refreshes the local copy from the archive under the per-repo lock before the emptiness recheck, and deletes the archive on a successful purge. It fails closed on any store error (a new The archive delete needed a seam, so Full workspace suite green, fmt + clippy clean. |
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 (2)
crates/gitlawb-node/src/admin.rs (1)
236-238: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not require a local cache hit before the authoritative recheck.
on_disk_ref_counttreats a missing local repository as non-empty, so remote-only empty archives are filtered out beforerefresh_from_archivecan download and verify them. Separate owner/exclusion scoping from emptiness selection, then refresh and check under the lock. Add a missing-local/empty-archive test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gitlawb-node/src/admin.rs` around lines 236 - 238, Update the spam-candidate flow around select_spam_candidates so candidate selection does not require an on-disk repository or use on_disk_ref_count as the emptiness filter; retain only owner/exclusion scoping, then call refresh_from_archive and perform the authoritative empty check while holding the lock. Add a test covering a remote-only empty archive with no local repository, ensuring it is selected, refreshed, and verified.crates/gitlawb-node/src/git/repo_store.rs (1)
79-99: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftObject-store uploads can resurrect an archive after purge.
All archive PUT paths must participate in the same repository-lock protocol as
purge-spamarchive deletion.
crates/gitlawb-node/src/git/repo_store.rs#L79-L99: acquire and retain the repository lock through the lazy migration HEAD/PUT.crates/gitlawb-node/src/git/repo_store.rs#L286-L295: initialize and upload under the repository lock instead of spawning an unlocked PUT.crates/gitlawb-node/src/git/repo_store.rs#L301-L315: require a lock guard/token or acquire the lock internally before uploading.🤖 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/repo_store.rs` around lines 79 - 99, Ensure every archive upload uses the repository-lock protocol shared with purge-spam deletion: in crates/gitlawb-node/src/git/repo_store.rs lines 79-99, retain the lock across the lazy migration existence check and upload; in lines 286-295, initialize and upload while holding that lock rather than spawning an unlocked PUT; and in lines 301-315, require a lock guard/token or acquire the repository lock internally before performing the upload.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/gitlawb-node/src/git/repo_store.rs`:
- Around line 177-205: Update RepoWriteGuard and RepoLockGuard so dropping
either guard automatically releases its advisory lock instead of relying solely
on release(). Ensure the unlock runs before the pooled connection is returned,
using an appropriate Drop-safe mechanism or transaction-scoped advisory locks,
while preserving explicit release behavior.
---
Outside diff comments:
In `@crates/gitlawb-node/src/admin.rs`:
- Around line 236-238: Update the spam-candidate flow around
select_spam_candidates so candidate selection does not require an on-disk
repository or use on_disk_ref_count as the emptiness filter; retain only
owner/exclusion scoping, then call refresh_from_archive and perform the
authoritative empty check while holding the lock. Add a test covering a
remote-only empty archive with no local repository, ensuring it is selected,
refreshed, and verified.
In `@crates/gitlawb-node/src/git/repo_store.rs`:
- Around line 79-99: Ensure every archive upload uses the repository-lock
protocol shared with purge-spam deletion: in
crates/gitlawb-node/src/git/repo_store.rs lines 79-99, retain the lock across
the lazy migration existence check and upload; in lines 286-295, initialize and
upload while holding that lock rather than spawning an unlocked PUT; and in
lines 301-315, require a lock guard/token or acquire the repository lock
internally before performing the upload.
🪄 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: e562d9be-a41b-488d-b6ef-e2576c26346e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/gitlawb-node/Cargo.tomlcrates/gitlawb-node/src/admin.rscrates/gitlawb-node/src/git/repo_store.rscrates/gitlawb-node/src/git/tigris.rscrates/gitlawb-node/src/main.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/gitlawb-node/src/main.rs
RepoWriteGuard and RepoLockGuard now hold their pooled connection as an Option: release() takes it (returns it to the pool) while a Drop impl closes the connection when release() was never reached — an early ?, a panic, or the handler future cancelled on client disconnect. Closing the connection ends the Postgres session so the server frees the session-scoped advisory lock, instead of returning a lock-holding connection to the pool where it would block every later write to that repo until the connection is reaped (up to ~30min at the pool's default max_lifetime). acquire_write now wraps the winning connection in the guard BEFORE the freshness download, so a cancellation during that await is covered by the guard's Drop rather than orphaning the lock on a bare connection. Tests observe the leak by disabling the pool's ambient reaping (which otherwise masks it ~1.8s after drop), so the lock stays held until the fix's own unlock. RED before / GREEN after for both guards and the acquire-side cancel; each protection line proven load-bearing by revert (INV-21).
git_receive_pack now runs the acquire -> receive-pack -> release core in a spawned task the handler awaits but whose cancellation it does not propagate. The pack body is fully buffered before the handler runs, so the task is self-contained: a client that disconnects mid-apply drops the handler future without cancelling the task, and the push still applies, uploads, and releases its lock in order. Drop-safety (the guard's close-on-drop) remains the backstop; this closes the dominant window where a disconnect both bricked the lock and abandoned a received push. The post-push bookkeeping tail stays in the cancellable handler (a disconnect still forgoes it, as today). Settled OQ1 by execution: a guard abandoned inside a detached task when the tokio runtime tears down does not panic in Drop (5/5 runs), so detached-task shutdown tracking stays deferred. Kept the check as a regression guard. Disconnect regression drives a real push with a sleeping pre-receive hook and drops the handler mid-hook: RED on the inline handler (the git group is killed, ref absent), GREEN once detached.
The lazy-migration upload in acquire(), init()'s background upload, and release_after_write() (fork's upload) previously PUT to the object store with no lock, so an in-flight upload could resurrect an archive purge-spam had just deleted under the same lock, and init's background upload could clobber a freshly-pushed archive. All three now go through upload_locked(): take the per-repo lock, re-check the local dir exists under it (a purge removes the dir under its lock, so a post-purge uploader finds it gone and skips), upload, then release. The background migration/init uploads skip on contention and self-heal; fork's foreground upload waits (bounded) instead, since a skipped fork upload has no later retry. Tests (recording object-store double): init upload skips under a held lock, fork upload waits then PUTs once after release, a late upload after a purge-style delete does not resurrect the archive, and an uncontended upload PUTs once. Load-bearing per INV-21 — neutralizing the lock+dir-check turns the three contention/resurrect tests RED.
On a multi-machine (object-store) deployment a spam repo can exist only as a Tigris archive with no local copy. Selection filtered on a ref count that returns non-empty for a missing local path, so such a repo was never a candidate and the tool it was hardened for could not reach it. Selection now distinguishes missing-local (Option::None) from a one-ref repo and admits a missing-local row as a *remote-unverified* candidate only when an object store is configured (via RepoStore::has_object_store); partition_for_delete passes those through rather than re-dropping them on the local ref count. The execute loop is unchanged — it already refreshes from the archive and rechecks emptiness under the per-repo lock — so a remote-only empty archive is deleted, one with refs is refreshed and skipped, and missing-both fails closed (refresh no-ops, the local recheck reports non-empty). Storeless deployments keep the old fail-closed behavior. Dry-run marks remote-only candidates distinctly and counts them in a new PurgeSummary field. Tests (recording object-store double): remote-only empty archive deleted, remote-only archive-with-refs skipped, missing-both fail-closed, storeless missing-local not a candidate. Load-bearing per INV-21 — reverting the store-configured admission turns the three remote-reach tests RED.
…nv wording - .env.example: the write brake wraps the whole /graphql route, so its docs now say it covers all GraphQL HTTP requests (queries and the playground GET), not only mutations, with WebSocket subscriptions called out as excluded. - main.rs: collapse the identical create/write/push per-IP limiter setup into build_ip_limiter (resolve-env, build bounded, warn-on-zero), preserving the exact limits/defaults and the present-but-unparseable warning in rate_limit_from_env; each limiter now also info-logs its configured value. - api/repos.rs: the nine per-IP write-brake tests share a send_from helper mirroring rate_limit.rs's post_from/post_with, replacing the repeated request-build + ConnectInfo + oneshot + status boilerplate. Behavior identical.
|
Pushed at Guards are Drop-safe; receive-pack survives a client disconnect ( Every archive upload holds the per-repo lock ( purge-spam reaches repos that exist only as an archive ( Each new guard, lock, and selection test is proven load-bearing by reverting the exact production line and watching it go RED. Also in this push: the One item I'll file separately rather than fold in: the per-repo advisory-lock key is hashed with @jatmn ready for another look. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/gitlawb-node/src/admin.rs`:
- Around line 174-192: Update local_refs_on_disk to reserve None for paths that
do not exist; treat an existing path lacking a valid bare-repository HEAD or
objects directory as non-empty (for example, Some(1)) so it cannot be admitted
as remote-unverified or replaced by refresh_from_archive. Preserve the existing
unsafe-name fail-closed behavior and update the related test assertion
accordingly, including the analogous logic near the second referenced call site.
In `@crates/gitlawb-node/src/api/repos.rs`:
- Around line 2737-2747: Update the git_receive_pack disconnect test to
synchronize with hook entry before aborting the handler: provide a signal the
detached hook emits when it starts, await that signal, then cancel or drop the
handler and retain the assertion that the push completes afterward. Remove
reliance on the fixed 800ms timeout as the disconnect trigger while preserving
the existing post-hook completion wait.
- Around line 930-966: Move the post-push tail currently following the
receive-pack task— including touch_repo, push records, certificates, webhooks,
and replication—inside the detached tokio::spawn task after a successful
receive_pack and lock release, so it continues after client disconnects. Update
the task result flow to perform this bookkeeping there, and leave the handler
responsible only for awaiting the task outcome and constructing the response.
🪄 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: cb675107-30e9-4c35-8906-71aeb518aecf
📒 Files selected for processing (5)
.env.examplecrates/gitlawb-node/src/admin.rscrates/gitlawb-node/src/api/repos.rscrates/gitlawb-node/src/git/repo_store.rscrates/gitlawb-node/src/main.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- .env.example
- crates/gitlawb-node/src/main.rs
- crates/gitlawb-node/src/git/repo_store.rs
| /// Local ref state for selection: `Some(n)` when a local bare repo exists (n | ||
| /// refs), `None` when there is no local copy. `None` is what lets selection | ||
| /// distinguish a missing-local repo (a remote-unverified candidate when a store | ||
| /// is configured) from a one-ref repo — both of which `ref_count_on_disk` | ||
| /// collapses to a non-zero count. An unsafe name or an unreadable repo fails | ||
| /// closed to `Some(1)` so it is skipped, never admitted as remote-unverified. | ||
| fn local_refs_on_disk(repos_dir: &Path, owner_did: &str, name: &str) -> Option<usize> { | ||
| // Fail closed on an unsafe repo name BEFORE building any on-disk path (a | ||
| // peer-mirror row can carry a `../` name). Report it as non-empty so it is | ||
| // never a candidate — never as missing (which could admit it remote-unverified). | ||
| if let Err(e) = crate::git::repo_store::validate_repo_name(name) { | ||
| warn!(name = %name, err = %e, | ||
| "purge-spam: unsafe repo name — treating as non-empty (skipped)"); | ||
| return Some(1); | ||
| } | ||
| let path = store::repo_disk_path(repos_dir, owner_did, name); | ||
| if !path.join("HEAD").is_file() || !path.join("objects").is_dir() { | ||
| // No local bare repo at the expected path. | ||
| return None; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Treat existing invalid paths as unsafe, not remote-only.
Lines 190-192 return None for an existing non-bare directory. With object storage configured, that admits it as remote-unverified and allows refresh_from_archive to replace its contents. Reserve None for genuinely absent paths; fail closed for existing, unreadable, or escaping paths. Update the test currently asserting None.
Proposed fix
let path = store::repo_disk_path(repos_dir, owner_did, name);
- if !path.join("HEAD").is_file() || !path.join("objects").is_dir() {
- // No local bare repo at the expected path.
- return None;
+ match std::fs::symlink_metadata(&path) {
+ Err(e) if e.kind() == std::io::ErrorKind::NotFound => return None,
+ Err(_) => return Some(1),
+ Ok(_) => {}
+ }
+ if !path_within(&path, repos_dir)
+ || !path.join("HEAD").is_file()
+ || !path.join("objects").is_dir()
+ {
+ return Some(1);
}- None,
- "a non-git dir under a git ancestor must report no local repo..."
+ Some(1),
+ "an existing non-git path must fail closed..."Also applies to: 1017-1033
🤖 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/admin.rs` around lines 174 - 192, Update
local_refs_on_disk to reserve None for paths that do not exist; treat an
existing path lacking a valid bare-repository HEAD or objects directory as
non-empty (for example, Some(1)) so it cannot be admitted as remote-unverified
or replaced by refresh_from_archive. Preserve the existing unsafe-name
fail-closed behavior and update the related test assertion accordingly,
including the analogous logic near the second referenced call site.
| // Detach the acquire → receive-pack → release core from THIS handler future. | ||
| // The pack `body` is already fully buffered, so the spawned task is | ||
| // self-contained: a client disconnect drops the handler future but does NOT | ||
| // cancel the task, so a fully-received push still completes server-side — | ||
| // applies the pack, uploads, and releases the lock in order. What a | ||
| // disconnect forgoes is the cancellable post-push bookkeeping below, not the | ||
| // push itself. (KTD-3, R3.) | ||
| let repo_store = state.repo_store.clone(); | ||
| let owner_did = record.owner_did.clone(); | ||
| let repo_name = record.name.clone(); | ||
| let receive_result = tokio::spawn(async move { | ||
| let guard = repo_store | ||
| .acquire_write(&owner_did, &repo_name) | ||
| .await | ||
| .map_err(|e| { | ||
| tracing::error!(repo = %repo_name, err = %e, "acquire_write failed"); | ||
| AppError::Git(e.to_string()) | ||
| })?; | ||
| let disk_path = guard.path().to_path_buf(); | ||
| tracing::debug!(repo = %repo_name, path = %disk_path.display(), "running git receive-pack"); | ||
| let result = smart_http::receive_pack(&disk_path, body, git_timeout).await; | ||
| // Always release the advisory lock — even on error — to prevent stale | ||
| // locks from blocking subsequent pushes. Only upload to Tigris when the | ||
| // push succeeded; uploading a half-applied repo would propagate corruption. | ||
| guard.release(result.is_ok()).await; | ||
| Ok::<_, AppError>(result) | ||
| }) | ||
| .await | ||
| .map_err(|e| { | ||
| tracing::error!(repo = %name, err = %e, "receive-pack task panicked"); | ||
| AppError::Internal(anyhow::anyhow!("receive-pack task failed: {e}")) | ||
| })??; | ||
|
|
||
| // Recompute the on-disk path for the post-push tail below — the guard that | ||
| // exposed it now lives inside the detached task. Identical to the path | ||
| // acquire_write resolved (repos_dir/owner_slug/name.git). | ||
| let disk_path = store::repo_disk_path(&state.config.repos_dir, &record.owner_did, &record.name); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant ranges with line numbers
sed -n '840,990p' crates/gitlawb-node/src/api/repos.rs | cat -n
printf '\n====\n'
# Show the later post-push section referenced by the comment
sed -n '980,1420p' crates/gitlawb-node/src/api/repos.rs | cat -nRepository: Gitlawb/node
Length of output: 30066
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the tests mentioned in the scratchpad and the surrounding helper usage.
sed -n '2580,3185p' crates/gitlawb-node/src/api/repos.rs | cat -nRepository: Gitlawb/node
Length of output: 27406
Detach the accepted-push tail from the request future. receive-pack is already offloaded, but touch_repo, push records, certificates, webhooks, and replication still run in the handler path. A disconnect can therefore leave the repo contents updated while the post-push metadata and fan-out never happen. Move that tail into the detached task (or a durable job) and keep the handler to response construction only.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/gitlawb-node/src/api/repos.rs` around lines 930 - 966, Move the
post-push tail currently following the receive-pack task— including touch_repo,
push records, certificates, webhooks, and replication—inside the detached
tokio::spawn task after a successful receive_pack and lock release, so it
continues after client disconnects. Update the task result flow to perform this
bookkeeping there, and leave the handler responsible only for awaiting the task
outcome and constructing the response.
| // Drive the handler, then DROP it mid-hook (the "client disconnect"). | ||
| let handler = super::git_receive_pack( | ||
| State(state.clone()), | ||
| AxPath((owner.to_string(), format!("{name}.git"))), | ||
| Extension(crate::auth::AuthenticatedDid(owner.to_string())), | ||
| body, | ||
| ); | ||
| let _ = tokio::time::timeout(Duration::from_millis(800), handler).await; | ||
|
|
||
| // Wait past the hook's sleep so a surviving (detached) push can finish. | ||
| tokio::time::sleep(Duration::from_millis(3000)).await; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the test and relevant hook logic.
rg -n "fully_received_push_completes_after_client_disconnect|git_receive_pack|tokio::spawn|timeout\\(Duration::from_millis\\(800\\)|sleep\\(Duration::from_millis\\(3000\\)" crates/gitlawb-node/src/api/repos.rs
# Show the surrounding region for the test.
sed -n '2680,2795p' crates/gitlawb-node/src/api/repos.rsRepository: Gitlawb/node
Length of output: 5233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the receive-pack handler implementation around its control flow.
sed -n '850,1025p' crates/gitlawb-node/src/api/repos.rs
# Look for any hook-entry markers or synchronization primitives in the test file.
rg -n "marker|notify|watch|channel|oneshot|broadcast|hook.*start|pre-receive" crates/gitlawb-node/src/api/repos.rsRepository: Gitlawb/node
Length of output: 11070
Synchronize the disconnect with hook start. The 800ms timeout can still fire before git_receive_pack reaches the detached tokio::spawn, so the test may drop the handler before the hook even begins. Wait on a hook-entry signal first, then abort the handler and assert the push still completes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/gitlawb-node/src/api/repos.rs` around lines 2737 - 2747, Update the
git_receive_pack disconnect test to synchronize with hook entry before aborting
the handler: provide a signal the detached hook emits when it starts, await that
signal, then cancel or drop the handler and retain the assertion that the push
completes afterward. Remove reliance on the fixed 800ms timeout as the
disconnect trigger while preserving the existing post-hook completion wait.
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] Preserve the post-push completion work after a disconnect
crates/gitlawb-node/src/api/repos.rs:930
The spawned task only coversacquire_write → receive_pack → release; all of the successful-push work remains in the request future after the join (timestamp, push/trust records, ref certificates, webhooks, pinning/replication, and ref-update broadcast). When a client disconnects while a fully buffered pack is applying, the task can commit and upload the ref but cancellation drops that tail. That leaves the committed Git state without the metadata and fan-out that every other accepted push produces. Move that completion work into the detached/durable path once receive-pack succeeds. -
[P1] Do not overwrite an existing non-repository directory during purge
crates/gitlawb-node/src/admin.rs:189
local_refs_on_diskreturnsNoneboth for a missing path and for an existing path without bare-repo markers. With Tigris configured,Nonepromotes the latter to aremote_unverifiedcandidate; execution callsrefresh_from_archive, whose extraction replaces the existing path, and can then delete it. ReserveNonefor a verifiedNotFound; existing, unreadable, non-bare, or escaping paths must fail closed so purge cannot replace operator data or a corrupted local repo. -
[P1] Serialize repository recreation with the purge delete
crates/gitlawb-node/src/admin.rs:406
The purge lock excludes writers, butcreate_reponever takes that lock. Once this code deletes the old row, another request for the same owner/name can pass its existence check, initialize the same directory, and insert a new row while purge still removes the directory and archive. The newly created repo is then left pointing at missing or stale storage. Make creation/recreation participate in the same namespace lock or retain a transactional tombstone until the destructive tail has finished. -
[P1] Do not leave bounties attached to a deleted repository name
crates/gitlawb-node/src/db/mod.rs:1349
The new deletion path deliberately leavesbountiesintact even though they are keyed only by(repo_owner, repo_name), not a repository id. If the owner recreates that name, the new repository immediately exposes the old open/claimed bounty and its readers can claim it through the normal bounty endpoint. Preserve the financial record without making it live on a distinct repo—for example by associating it with an immutable repo id or tombstoning it during purge. -
[P1] Avoid deleting slug-scoped records belonging to a surviving mirror row
crates/gitlawb-node/src/db/mod.rs:1337
list_repos_by_owner_diddeliberately returns every physical row, while canonicaldid:key:and bare-owner mirror rows share the normalizedowner/nameslug. Deleting one empty physical row therefore deletesbranch_cids, sync queue entries, received updates, and anchors for a non-empty mirror row that survives the purge. Scope this cascade to an unambiguous repository identity, or only remove slug-scoped data after confirming no other row still owns that slug. -
[P1] Keep database capacity available while holding advisory locks
crates/gitlawb-node/src/git/repo_store.rs:167
Every successfulacquire_writenow pins a connection from the shared application pool for the entire receive-pack and archive upload—up to the 600-second git timeout. Twenty concurrent pushes to distinct repos consume the default 20-connection pool, so unrelated DB-backed requests fail their acquire timeout. Use a dedicated/reserved lock pool or enforce push concurrency that leaves capacity for normal handlers. -
[P2] Make purge work with the supported one-connection pool
crates/gitlawb-node/src/admin.rs:372
try_lock_repoholds a pool connection for the advisory lock, thendelete_repo_by_idstarts a transaction through the same pool before the guard is released.GITLAWB_DB_MAX_CONNECTIONS=1is accepted by configuration, so the admin command deterministically waits for its own held connection and skips every deletion. Reserve a second connection by configuration or restructure the lock/transaction ownership so this supported setting can execute a purge. -
[P2] Either enforce the shutdown grace or stop documenting that it is enforced
.env.example:20
This newly documented setting promises that in-flight requests are cut off after the grace period, butmain.rsonly constructs the duration, drains Axum with an unbounded await, then explicitly discards it. A stuck git or storage request can prevent shutdown indefinitely. Bound the drain and force termination/503 handling at expiry, or document the actual behavior. -
[P2] Return a retry delay compatible with the new one-hour write window
crates/gitlawb-node/src/rate_limit.rs:274
The new write bucket retains timestamps for an hour, yet every rejection advertisesRetry-After: 60. A compliant client retries after one minute and will commonly receive repeated 429s for almost another hour, increasing rejected load on the very write surface this change protects. Compute the delay from the oldest live timestamp (or omitRetry-Afterrather than provide an incorrect value).
2 participants
What
Two related pieces of the creation-surface abuse-resistance work:
A per-IP write-surface rate brake. Every authenticated non-creation write sink now sits behind a per-client-IP flood brake on its own bucket: pull/issue comments, labels, stars, merges, protect/unprotect, replicas, visibility, agent deregister, tasks, bounties, profile, and GraphQL mutations. Previously these route groups mounted with no rate limit at all, so a single-source flood on any of them was uncounted (only repo/agent creation, git-push, and peer-sync were braked). The bucket is configured by
GITLAWB_WRITE_RATE_LIMIT(default 600/hr,0disables), keyed on the resolved client IP via the existingGITLAWB_TRUSTED_PROXYboundary.A
purge-spamadmin subcommand for cleaning up a specific empty spam-burst that predates the mirror bot. Dry-run by default;--executedeletes.Key decisions
sync_triggervspeer_writesplit). A shared bucket would also self-throttle a legitimate session doing repo + issues + comments.did:keyfarm never trips a per-DID limit (the same reasongit-receive-packdeliberately omits it), and a per-DID brake would false-positive busy legitimate agents./graphqlrequest;/graphql/wssubscriptions stay unbraked.MutationRootwas the largest previously-uncounted write vector.Verification
cargo test --workspacegreen;cargo fmt --all --checkandcargo clippy --workspace --all-targets -D warningsclean.TrustedProxycoverage: a spoofed leftmostX-Forwarded-Forhop cannot rotate the bucket key, distinct trusted hops get distinct buckets, and an absent header falls back to the socket peer.Scope
This is the write-surface brake coverage plus the burst cleanup. iCaptcha enforcement, storage/size quotas, and moving the gate toward enforce-by-default are separate follow-ups, not in this PR.
Summary by CodeRabbit
purge-spamadmin command with dry-run and optional execution.GITLAWB_WRITE_RATE_LIMIT(0disables), applied to key REST and GraphQL mutation routes while keeping/graphql/wsunaffected..env.examplewith new runtime knobs (GITLAWB_METRICS_ADDR,GITLAWB_SHUTDOWN_GRACE_SECS,GITLAWB_TIGRIS_BUCKET,GITLAWB_IPFS_API) and the write-rate policy description.