Skip to content

Ref-update feed loads the whole repo table per request, above its own scan cap #423

Description

@PierrunoYT

collect_visible_ref_updates_inner (api/events.rs:37) loads three whole tables before it filters a single row:

let deduped = db.list_all_repos_deduped().await?;
let quarantined = db.list_quarantined_repos().await?;
let ids: Vec<String> = deduped.iter().map(|r| r.id.clone()).collect();
let rules = db.list_visibility_rules_for_repos(&ids).await?;

That runs per request, on the anonymously reachable GET /api/v1/events/ref-updates and on the GraphQL ref_updates resolver that shares the collector.

What is not the problem

The gate itself is correct and this issue does not dispute it. Loading the inputs once and failing closed on a DB error is deliberate; the keyset paging past withheld rows is the fix from #114 and #112 and is the right shape. The max_scan cap bounds the row walk.

What is unbounded is the preload, which sits above max_scan and grows with the repo table rather than with the request.

Cost

O(repos) rows plus O(repos) String clones for the id vector, plus the visibility-rule fetch keyed on all of them, per request. On a node with a large repo table this is the dominant cost of a feed request, and the caller pays nothing for it — the route is anonymous and outside every rate_limit_by_ip layer in build_router.

Related in kind to #147, which capped an unbounded per-repo fetch on a permissionless read path, and to the GraphQL repository-enumeration and /api/v1/stats paths that load repo records to compute visibility-aware answers. This is the same pattern on the cheapest route to reach.

Fix direction

Two options, probably in this order:

  1. Cache the three gate inputs behind a short TTL keyed on the repo table's last update. Cheap, no behavior change, and the inputs are already loaded as a unit.
  2. Push the visibility predicate into SQL so the rows are filtered by the database rather than fetched and dropped in Rust. Larger, and it has to preserve the quarantine-before-visibility ordering that events.rs:98-107 is careful about — quarantine denies before the owner short-circuit in visibility_check can run, and a naive SQL translation would lose that.

Option 2 is the real fix; option 1 is worth having regardless because the inputs change rarely relative to how often the feed is read.

Validation status

Verified by reading api/events.rs and the db methods it calls. No measurement against a populated repo table is offered, so the point at which this matters in practice is unquantified.


Found during an external audit pass. Duplicate-checked against open and closed issues; no existing coverage found, but the search used a limited keyword set.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    crate:nodegitlawb-node — the serving node and REST APIkind:securityVulnerability fix or hardeningsev:highMajor break or real security/trust risk, no easy workaroundsubsystem:apiNode REST API request/response surfacesubsystem:visibilityPath-scoped visibility and content withholding

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions