fix(node): bound federated repository aggregation - #466
cairn-intern wants to merge 6 commits into
Conversation
Register the bounded peer-query fixture in the writer ledger, name federation bounds, and share the state-owned limiter across routers and the cleanup sweep. Cover exhausted-bucket rejection before database access.
|
Warning Review limit reachedNext included review available in 6 minutes. View limit detailsLimit details: You’ve used the included review currently available. Only developers with an assigned seat can use this organization's usage-based review budget, and seats here are assigned manually. Ask an admin to assign a seat, or change the review continuation mode in Billing. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
beardthelion
added
crate:node
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Verified on this head: the peers query is LIMIT-bound, the per-peer body cap fires mid-stream without Content-Length, the 200-row page bound truncates rather than rejects, the aggregate deadline and concurrency cap hold, the per-IP brake rejects before any database access, and the peers-table write guard is registered. Removing each of those guards individually turns the new tests red, so the protections are real. One gap in the coverage, not the logic.
Findings
- [P2] Pin the federated response contract end to end
crates/gitlawb-node/src/api/repos.rs:3206
The new envelope is exercised only at unit level: tests assert budget internals and a self-constructed JSON body, but nothing readstruncated,nodes_queried, or thenode_url/node_did/localannotations from a real handler response. On this head I hardcoded"truncated": falsein the handler, deleted the failed-peeraggregate.truncated = true, and droppednode_didfromenrich_federated_repo; all nine federated tests stayed green in each case. Assert the fields on an actual 200 body (the route tests already drivebuild_router), and add one case that observestruncated == true; seedingMAX_FEDERATED_PEERS + 2peer rows with the fixture INSERT makes that reachable without a live peer.
One process note, not a finding: the handler still scans all local repos (list_all_repos_with_stars plus the visibility-rules load) outside the aggregate deadline, so per-request work stays proportional to local repo count. That matches the standing exposure on GET /api/v1/repos, so it is not an ask here, but bounding the local page with the same LIMIT-plus-sentinel shape used for peers would finish the bound this PR starts.
Not an ask, recorded only: nodes_queried counts peers that returned a usable page, not peers contacted; an exactly-full budget reports truncated: true even when nothing was omitted; a peers-table error now fails the whole request instead of degrading to local-only results; and a compliant peer whose 200-row page exceeds 512 KiB is dropped whole rather than partially used.
…eview) Address beardthelion's P2: the federated envelope was only exercised at unit level, so hardcoding "truncated": false, deleting the failed-peer truncated flag, or dropping node_did from enrich_federated_repo each left all nine federated tests green. Add four route tests driving build_router and asserting on the real HTTP 200 body: - envelope fields (count, nodes_queried, truncated) and local annotations (node_url, node_did, local) for a seeded public repo - truncated == true when the peer table overflows MAX_FEDERATED_PEERS (unreachable peer URLs, no live peer needed) - truncated == true on the failed-fetch path with the table under the bound - peer annotations (node_url, node_did, local: false) via a mockito peer
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Verified on this head against main: the new route tests do pin the envelope
end to end. Deleting the failed-fetch truncated assignment, hardcoding
"truncated": false, or dropping the node_did enrichment each turns a route
test red, and the deadline, peers-query clamp, streaming byte cap, and
route-layer ordering all re-verified the same way. One suite-level failure
and two pins that still are not load-bearing.
Findings
-
[P1] Disposition the three new peers-table writers in the writer ledger
crates/gitlawb-node/src/db/mod.rs:8387
cargo test -p gitlawb-node peers_table_writer_guard fails on this head: the
scan finds 12 writers against a 9-entry LEDGER. Each new route test issues a
raw INSERT INTO peers (repos.rs:3946, 3972, 4008) with no ledger row and no
entry in the disposition table above it. Raw SQL is the right fixture shape
here since upsert_peer refuses the loopback URLs these tests need; add the
three ("name", 1) rows and matching table entries, same as the
federated_peer_query_is_bounded precedent. -
[P2] Give the peer-overflow flag its own witness
crates/gitlawb-node/src/api/repos.rs:3946
The overflow test seeds 202 peers at an unreachable address, so its
truncated == true is satisfied by the failed-fetch arm even with the count
check gone: deleting aggregate.truncated |= peers.len() > MAX_FEDERATED_PEERS
keeps it green. Point the seeds at one reachable stub returning [] and
assert nodes_queried, so only the bound can set the flag. Same shape of gap
in the envelope test at 3911: the seeded repo is owned by the node DID, so
stamping owner_did in place of node_did at 3149 also stays green. Seed a
repo owned by a different DID. -
[P3] Pin the outbound peer page request on the mock
crates/gitlawb-node/src/api/repos.rs:3999
match_query(Matcher::Any) accepts any query string, so the
?limit=200&offset=0 request the bounded fan-out depends on is unpinned;
dropping the params stays green. Match or capture the query in
federated_route_annotates_peer_repos_on_real_200_body.
Not an ask, recorded only: the fetcher dials whatever http_url a peers row
carries with no fetch-time re-validation beyond last_ping_ok; identical on
main, and this change narrows the reachable set, so it is worth a separate
fix-up rather than an ask here. nodes_queried counts peers that returned a
usable page rather than attempts, and truncated also fires on an exact-boundary
aggregate fill; both match the docstring's reading.
- db/mod.rs: add writer-ledger entries and disposition docs for the three federated route tests touching the peers table - api/repos.rs: seed the envelope test repo under a non-node owner DID so node_did must equal the actual node DID; pin the outbound peer query to exact limit=200&offset=0; make the peer-table-overflow test use a reachable stub for every peer so the count bound is the only possible reason for truncated=true, and assert nodes_queried
euxaristia
left a comment
euxaristia
left a comment
There was a problem hiding this comment.
Re-submission of the closed #409 (federated aggregation bounds). Same note as the closed round: the per-peer byte budget and row caps need tests, and the rebase should account for the current federation collector shape. Coordinating the landing order with #465 avoids a double-rebase of the shared collector.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-checked head a7f931d against origin/main at bfc44f9. The prior round ledger, overflow witness, envelope pins, and mock query assertions are all on this head. I ran cargo test -p gitlawb-node federated_, peers_table_writer_guard, and cargo clippy -p gitlawb-node --all-targets -- -D warnings on the PR branch locally; all passed. Removing aggregate.truncated |= peers.len() > MAX_FEDERATED_PEERS turns federated_route_reports_truncated_when_peer_table_overflows red, so the peer-table bound is load-bearing, not only the failed-fetch path.
Not an ask, recorded only: the handler still loads the full local repo inventory before the aggregate budget applies, same shape as on main before this PR. nodes_queried counts peers that returned a usable page, and truncated also fires on exact-boundary aggregate fill, matching the docstring.
One process note, not a finding: rebasing onto recent main will likely conflict with other open work on repos.rs and state.rs; coordinate with #465 if you are both touching federation-adjacent code.
euxaristia
left a comment
euxaristia
left a comment
There was a problem hiding this comment.
LGTM. Re-review of head a7f931d confirms federated query bounds, peer ledger registrations, and response truncation behavior are fully verified.
3 participants
Summary
Bound federated repository aggregation so peer responses cannot cause unbounded
allocation, fan-out, or request duration. Return partial results with an explicit
truncatedflag when a peer or aggregate budget is exceeded.Closes #427
Changes
Prior reviewer feedback addressed
Test plan
cargo fmt --all -- --checkcargo check --workspace --all-targetscargo clippy --workspace --all-targets -- -D warningscargo test -p gitlawb-node federatedcancellation, deadline-driven partial results, and shared rate-limit admission.
Recreated from closed PR #409 by @euxaristia (approved but unmerged). Original branch: euxaristia/node:codex/fix-federated-response-bounds