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 63 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).
|
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:
📝 WalkthroughWalkthroughThe pull request adds a safe ChangesSpam purge administration
Repository storage and write lifecycle
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 | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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.
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 full iCaptcha origin in the advertised-URL trust check
crates/icaptcha-client/src/lib.rs:91
resolve_solver_urlnow compares only the hostname when accepting a node-advertised URL and deciding whether to attachGITLAWB_ICAPTCHA_API_KEY. A hostile node can advertisehttps://<configured-iCaptcha-host>:8443/...; that is a different origin but passes both checks, so the client posts the operator's bearer token to the alternate service. The base implementation deliberately included the effective port in this comparison. Restore normalized scheme/host/effective-port matching and retain a cross-port regression test. -
[P1] Do not drop the iCaptcha PoW protocol fields
crates/icaptcha-client/src/lib.rs:197
This removesChallenge.pow, the PoW solver, and thepowNoncesent to/v1/answer. A PoW-enabled service therefore receives only{ token, answer }and rejects the proof attempt, leavingglunable to complete any protected create, fork, or registration flow. This is a rollback of the base client protocol without a corresponding service migration; preserve the optional PoW solve-and-submit path. -
[P1] Ensure a newly created repository is uploaded after releasing its creation lock
crates/gitlawb-node/src/git/repo_store.rs:334
create_repoholds the per-repository lock while callinginit, whileinitstarts a non-waitingupload_locked(..., false)task. When that task runs before the request releases the lock, it sees the lock as contended and exits permanently. The API still returns 201, but the new empty repository has no object-store archive, so another node (or the creator after local loss) cannot hydrate it until a later write happens. Schedule a retry/upload after releasing the lock, or upload through the already-held guard. -
[P1] Propagate failed fork archive uploads instead of publishing an unavailable fork
crates/gitlawb-node/src/api/repos.rs:1699
release_after_writediscardsupload_locked's boolean outcome, andfork_repoalways inserts the DB row and returns 201. With Tigris enabled, an upload failure or bounded lock-acquire failure leaves a fork only on the local node; cache misses on other nodes then resolve the DB record to no repository. Return the upload result and fail the fork before publishing its row when durability was not achieved. -
[P2] Serialize forks with purge-spam just as repository creation is serialized
crates/gitlawb-node/src/api/repos.rs:1661
The new create-vs-purge lock is not used byfork_repo. If the burst DID forks into the owner/name being purged after its row is removed but before the purge finishes removing disk/archive state, the purge can remove the newly cloned fork or its archive and the fork can subsequently insert a dangling row. Hold the same namespace lock across the conflict check, clone/upload, and row insertion. -
[P1] Prevent an in-flight cache download from restoring a purged repository
crates/gitlawb-node/src/git/repo_store.rs:157
acquire_freshperforms the object-store download anddecompress_repopublishes it to disk without the advisory lock used by purge. A request that begins a GET before purge deletes the archive can complete after purge releases its lock and swap the deleted repository back into its old path. That leaves stale data on disk and can block or contaminate a same-name recreation. Make the cache publish participate in the repository lock, or revalidate after download before publishing it. -
[P2] Do not present a locally-applied issue create as a retry-safe failure
crates/gitlawb-node/src/api/issues.rs:48
On an archive-upload failure the handler writes the issue ref locally, returns a retryable 5xx, and instructs the client to retry. Each retry creates a new UUID and ref; if the first local state survives (for example while the store remains unavailable), the retry uploads both issues. Add an idempotency/deduplication mechanism for this recovery path, or use a recovery response that does not imply a safe replay.
…lock (#196) create_repo holds the per-repo lock across init, so the wait=false spawn try-locked against its creator and died permanently, leaving a new repo with no object-store archive until its first later access. RED observed: init_upload_waits_out_creators_lock_then_uploads failed (PUT count 0) against the old code; the bounded-skip negative is retained as init_upload_gives_up_after_bounded_lock_wait.
…le upload fails (#196) The failed-upload 500 previously left refs/gitlawb/issues/{id} on disk, so a client retry minted a second issue once the store recovered. The delete now runs via RepoWriteGuard::release_with_failure_cleanup while the advisory lock is still held, closing the unlock-to-delete window a concurrent write could have archived the orphan ref through. RED observed on both new tests (orphan ref survived; retry yielded 2 issues); the retry test pins the store double's exists()=false contract so it cannot pass via a stale-archive revert.
…the target-namespace lock (#196) fork_repo now takes the per-repo advisory lock before its conflict check and holds it through clone, upload, and row insert, 503ing on contention exactly as create_repo does. The upload runs under the held guard via upload_under_guard, PUT bounded by release_upload_timeout, returning false only for an attempted upload that failed or timed out (no-store nodes fork unchanged); on failure the mirror is removed and the fork 500s before any row is built. release_after_write is retired; its tests migrated onto the new method. RED observed on the serialization, durability, and contention tests; defect injection (row insert reordered above the upload) turned the durability test RED and was reverted.
…pinning it across the network (#196) acquire's cache-miss and every acquire_fresh refresh now coalesce on a per-repo download mutex, download to a temp sibling with no advisory lock held, then try-lock, re-check the archive exists under the lock, and swap - so an in-flight read can no longer resurrect a repository purge just deleted (RED observed on both the cache-miss and refresh-over-existing repros), a post-purge reader degrades instead of publishing, and concurrent cold reads download once. The hit path stays lock-free (deterministic held-lock fence) and no lock-pool connection is held while a download is parked.
…oad path (#196) Round-two review found the durability fixes reintroduced the unbounded-work-under-lock class they were meant to close: upload_locked's PUT (now reachable under the lock via init's wait=true), the read-path download, and the under-lock exists() re-check all ran with no timeout while pinning the advisory lock and its lock-pool connection. All three now wrap the call in timeout(release_upload_timeout) and fail closed on elapse, matching RepoWriteGuard::release's existing INV-22 pattern. upload_under_guard and upload_locked's duplicated resolve/recheck/upload body is extracted into upload_dir_locked, threading the load-bearing dir-gone signal (fork needs success, lazy migration needs skip-and-retry) through a parameter rather than aligning it. RED observed: upload_locked_put_stall_is_bounded failed (lock never freed) against the un-timeout'd PUT.
…swaps, and lock contention (#196) Four read-path gaps the review surfaced. A TempDownloadDir RAII guard now cleans the temp download dir if the handler future is cancelled on client disconnect (the read handlers have no spawn shield, and a post-download disconnect leaked the full populated dir), plus an entry-time sweep of stale siblings. A cold or refresh reader that downloaded while a concurrent writer published fresher on-disk state now discards its stale temp and serves the writer's copy instead of swapping over it (detected by presence for the cold path, mtime for the refresh path). The takeover path re-registers its download-mutex entry so late readers coalesce instead of starting a duplicate download. And the try_lock Ok(None) degrade arm - the KTD-3 invariant that a reader degrades rather than resurrecting under a live writer/purge - is now tested by holding the advisory lock via an out-of-pool observer. RED observed on both stale-swap tests.
…#196) The fork tail held the target-namespace advisory lock across two unbounded operations. The source acquire, which on a cold source triggers a full object-store download plus a nested lock-pool acquire on the same writer-sized pool, now runs before the target lock is taken (it is read-only on a different namespace and nothing under the lock depends on it) so a cold-source fork can no longer demand two connections from a one-per-writer pool at once. The git clone --mirror now runs via tokio::process with kill_on_drop under a timeout(git_service_timeout_secs) ceiling, cleaning the partial mirror and releasing the lock on elapse. RED observed by executing the pool-size-1 cold-source fork with the source acquire moved back under the lock: PoolTimedOut, source never publishes, clone fails.
…e submodule (#196) repo_store.rs had grown past 2800 lines. The download-coordination code (DownloadOutcome, TempDownloadDir, download_published, download_and_publish, remove_download_entry) moves to a private repo_store/download.rs child module, which reaches the parent's private fields and methods without widening visibility beyond pub(super) on the two items the parent names. Pure move: no behavior change, the same 44 git::repo_store tests pass unchanged.
…ntry on cancel (#196) Two gaps a second-model adversarial pass caught that the in-family review missed. acquire_write's under-lock exists()/download() ran with no timeout, so a stalled Tigris GET pinned the write lock and its lock-pool connection forever (the local-copy fallback only fires on an error, never a hang) - both are now wrapped in timeout(release_upload_timeout), a timeout mapping to the existing download-error fallback, which also makes the prior 'bound every under-lock call' claim true. The download_locks map entry was removed only on normal return, not on a cancelled cold read, so a disconnecting caller could grow the map across public repos; a DownloadEntryGuard now prunes it in Drop (outer map moved to std::sync::Mutex, held only for map ops, never across an await). RED observed on both new tests, including a real mid-download cancellation.
… failures (#196) The bounded-clone rework cleaned up the mirror on the timeout and spawn arms but not on a clone that creates the destination then exits nonzero, so a retry hit 'destination already exists' and could never complete that fork name. And because upload_under_guard is now required and runs before the row insert, a create_repo failure reliably orphaned the durable archive too, not just the local dir. Both failure arms now remove the mirror, delete the fork's archive (no-op on a store-less node), release the lock, and return. RED observed on both new tests (a non-empty-dest clone-128 repro and a decoy-row insert-failure repro).
|
Pushed the fixes (head 8d2fec1). Against your findings: iCaptcha origin check and PoW fields — these don't apply to this branch. It never touched create_repo starves its own init upload — fixed. fork publishes despite a failed upload, and isn't serialized with purge — both fixed by one change: the fork tail now holds the target-namespace advisory lock across the conflict check, clone, upload, and row insert. in-flight cache download resurrects a purged repo — fixed. Cache-miss and refresh downloads extract to a temp dir with no lock held, then take the per-repo lock, re-check the archive still exists, and swap; a download racing a purge discards instead of republishing. issue-create retry duplicates the issue — fixed. The failed-upload arm deletes the local ref before the advisory unlock, so a retry yields exactly one issue. While in here I also closed the lock-lifetime gap the durability work exposed: no advisory lock is now held across a network transfer on the read path, and every under-lock object-store call (upload, download, exists, write-acquire) is bounded by |
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] Serialize mirror ingestion with the purge child-record cascade
crates/gitlawb-node/src/db/mod.rs:1386
delete_repo_by_iddecides that no normalized owner/name sibling exists, then deletes every slug-scoped record and tombstones the slug's bounties.upsert_mirror_repodoes not participate in the purge advisory lock or otherwise lock that namespace, so peer sync can insert a surviving bare-owner mirror (and its sync/branch/anchor state) after this point-in-time check but before those deletes execute. The purge then removes live state from that mirror and can make its active bounty unclaimable. Serialize the mirror upsert with this deletion, or use a transaction/constraint that prevents this phantom before cascading slug-scoped records. -
[P1] Roll back every locally applied write when its durable upload fails
crates/gitlawb-node/src/api/issues.rs:296
close_issuechanges the issue ref and then uses plainguard.release; when the Tigris upload times out or fails, it returns a retryable 5xx but leaves the changed ref in the local cache. Normal issue reads useRepoStore::acquire's local fast path, so this node continues to report the issue closed even though the client was told the operation failed; the stale state can persist across restart until a later write refreshes the archive.create_issuealready usesrelease_with_failure_cleanupfor this exact reason. Restore the previous ref (and apply equivalent rollback to the merge and receive-pack paths) before releasing the write lock on a failed durable upload. -
[P2] Do not let cancelled cold reads recreate abandoned download directories
crates/gitlawb-node/src/git/repo_store/download.rs:226
The timeout/cancellation path dropsTempDownloadDir, butTigrisClient::downloadhas already starteddecompress_repoinspawn_blocking. Dropping the await detaches that blocking task; it can finish afterward and rename its extraction back into the just-removed.tmp-download.*path. That directory is only swept on a later request for the same repository, so cancelled cold reads across distinct repositories accumulate full extracted repositories on disk. Tie temp cleanup to completion of the blocking extraction, or add bounded, reliable cleanup for these abandoned directories.
…ry lock delete_repo_by_id ran a point-in-time sibling check then cascaded slug-scoped deletes; upsert_mirror_repo (peer sync) did not serialize against that window, so a colliding mirror could land mid-purge and get half-wiped. Both now take a transaction-scoped pg_advisory_xact_lock keyed on the normalized slug. (F1, #196)
…receive-pack) close_issue, merge_pr, and the receive-pack task applied a local ref write then released with an empty failure cleanup, so a failed Tigris upload left stale state that the local-fast-path read served despite the client getting a 5xx. Each now captures the prior ref and restores it via release_with_failure_cleanup when the upload fails, matching create_issue. (F2, #196)
On a download timeout the TempDownloadDir guard dropped while the detached spawn_blocking extraction kept running and could rename its output back into the just-removed temp path; that dir was only reclaimed on a later same-repo request, so cold reads across distinct repos accumulated extracted repos on disk. The timeout arm now hands the owned download future to a detached janitor that awaits extraction completion before removing the temp dir, off the critical path so waiters are still freed immediately. (F3, #196)
Auto-deref (&mut tx) on the lock_repo_slug calls and rustfmt of the new test lines. No behavior change.
…ck; cover merge and push rollbacks
…st in the purge-race test
…cellation cannot resurrect temp dirs
…push ref snapshot fails
…flight task, compensate a timed-out upload with the rolled-back tree, aggregate partial ref restores, race-test the anchor lock, fail-closed snapshot test
|
All three addressed at Purge vs mirror ingest. Rollback on failed durable upload. Cancelled cold reads. The download task now owns the temp dir, the per-repo mutex, and the locks-map entry, releasing all three only when the task settles. That covers timeout, error, and client disconnect with one path, and late readers coalesce onto the in-flight download instead of starting duplicates (the pre-fix code could stack N full downloads of the same object under timeout waves). Resurrection and coalescing tests exist in timeout and cancel variants; all fail against the pre-fix file. One pre-existing gap I am disclosing rather than fixing here: peer-supplied repo identifiers stored in full-DID form write rows the purge cascade's normalized-slug The merge from main had two conflicts ( |
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] Rebase without rolling the released version back
.release-please-manifest.json:2
This is rebase drift, not part of the feature: the base commit is already taggedv0.7.0, but this head resets every package and release-please manifest to0.6.0and removes the0.7.0changelog entry. Merging it would make binaries report an older, already-published version and make the next release calculation start from invalid release state. Rebase onto the current base and retain the 0.7.0 release artifacts. -
[P1] Keep the gitlawb-core dependency-purity gate
.github/workflows/pr-checks.yml:249
This is also rebase drift: the PR removes the only required job that enforces the exhaustive normal-dependency allowlist forgitlawb-core, and deletes both the checker and its policy file with no replacement. Future heavyweight or supply-chain dependencies added to the core crate will then pass normal CI and silently propagate to every shipped CLI/node consumer. Restore the gate when rebasing. -
[P1] Do not use the repository root mtime as the fresh-read write generation
crates/gitlawb-node/src/git/repo_store/download.rs:395
acquire_freshdownloads an archive without the advisory lock and then assumes a changed bare-repository root directory mtime proves a writer published during that window. Ordinary pushes update nested refs such asrefs/heads/mainand leave the bare repo root mtime unchanged, so the reader can take the lock after a successful writer, remove that writer's local checkout, and rename its older archive over it. Subsequent local reads serve stale refs until another refresh/write. Use a reliable version/generation check or keep the download/publish sequence ordered with writers. -
[P1] Abort close and merge when their rollback snapshot cannot be captured
crates/gitlawb-node/src/api/issues.rs:296
issue_ref_oid(...).ok().flatten()(and the equivalent target-ref snapshot inapi/pulls.rs) suppresses a snapshot error and proceeds with the mutation. If the following durable upload fails, the cleanup closure has no prior ref to restore, yet the handler returns 5xx while the local fast path keeps serving the closed issue or merged branch; a later write can upload that state. This is the same durable-write split thatgit_receive_packnow correctly fails closed for, so capture the snapshot or reject before mutating. -
[P1] Do not treat an unordered compensating PUT as rollback convergence
crates/gitlawb-node/src/git/repo_store.rs:855
After an upload timeout, this rolls the local refs back and sends one compensating PUT, but the timed-out S3 request may have reached the service and can commit after that compensation. These unversioned writes have no ordering or conditional precondition, so the first PUT can become the final archive even though the client received a failure and local disk was restored. A cold node then resurrects the rejected write. Establish completion/ordering with the original PUT, or use a versioned/conditional storage protocol before claiming the rollback is safe. -
[P2] Bound object-store refresh while purge owns the repository lock
crates/gitlawb-node/src/git/repo_store.rs:678
run_purge_spamcallsrefresh_from_archivewhile it holds the per-repository advisory lock, but that helper awaits bothexistsanddownloadwithout a timeout. A stalled S3 request therefore pins the repo lock and its dedicated pool connection indefinitely, blocking writes for that repository and eventually exhausting the lock pool under several stalls. Apply the existing bounded timeout and fail closed/skip the candidate on expiry. -
[P2] Cascade slug-scoped children using the same normalized representation writers persist
crates/gitlawb-node/src/db/mod.rs:1463
The purge cascade deletes only the normalized bare slug, whileenqueue_syncandinsert_ref_updateuse that normalization only for their advisory-lock key and persist the peer-suppliedrepostring verbatim. A known peer can enqueuedid:key:<purge-target>/name; the purge of that target deletes<bare>/namebut leaves the pending queue entry, and the sync worker later clones and upserts the repository again. Normalize before storing these child keys or delete both equivalent forms as part of the cascade.
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 leaving GraphQL WebSocket traffic unaffected.Retry-Afterwindow on applicable endpoints..env.examplewith metrics, storage, IPFS, lock-pool sizing, and the write-rate throttling policy.