lore-revision, lore-server: Bound concurrent store lookups during branch push verification - #186
lore-revision, lore-server: Bound concurrent store lookups during branch push verification#186bclarke123 wants to merge 2 commits into
Conversation
github-actions
Bot
added
area:server
|
Looks like the linter failed as the latest version of |
|
|
||
| fn store_lookup_limiter() -> &'static Semaphore { | ||
| STORE_LOOKUP_LIMITER.get_or_init(|| { | ||
| let cpus = std::thread::available_parallelism().map_or(1, |n| n.get()); |
There was a problem hiding this comment.
use the cpu counter helper in lore-base
| const QUERY_BATCHES_IN_FLIGHT_PER_CPU: usize = 8; | ||
| const MIN_QUERY_BATCHES_IN_FLIGHT: usize = 16; | ||
| const MAX_QUERY_BATCHES_IN_FLIGHT: usize = 128; | ||
| let max_batches_in_flight = (std::thread::available_parallelism().map_or(1, |n| n.get()) |
There was a problem hiding this comment.
Same here, use core count helper from lore-base
|
Updated, switched both to processor_count(), thanks again |
7f00d46 to
90298fa
Compare
|
Smoke failure is |
|
Update: it's deterministic, not flaky — the test fails on main itself (3/3 locally on 9a9c24b; the earlier passing run predated the test). The client reports AddressNotFound as "Address not found: … peer is missing a fragment" and never echoes the server's "Missing fragment" text, so the assertion can't match. Fix in #195 . |
…nch push verification collect_new_addresses spawned one immutable-store metadata lookup per fragment address, capped per JoinSet but multiplied by the recursion into fragmented files, and verify_fragments spawned every query batch at once. On a remote store that is one request per fragment with thousands in flight, so server memory, CPU and handler time grew with the push size: a 20 GB push OOM-killed a 4 GB server or timed out. Share one lookup budget across the recursion, released before recursing so parents never hold it while waiting on children, and cap the query batches in flight. SlowDown batches still wait for the retry back-off together. Same results and errors, no new configuration. Signed-off-by: Ben Clarke <ben@arrayofstars.com>
…kup budgets Per review: size the branch push lookup and query batch budgets with lore_base::runtime::processor_count(), which also accounts for Windows processor groups, rather than std::thread::available_parallelism. Signed-off-by: Ben Clarke <ben@arrayofstars.com>
90298fa to
8c14968
Compare
2 participants
Closes #185
Problem
Branch push verification fans out one immutable-store metadata lookup per fragment address with effectively unbounded concurrency, so server memory, CPU and handler time all scale with the size of the push.
verify_fragments(lore-server/src/grpc/handlers/branch_push.rs) callsstate::collect_new_fragments, whosecollect_new_addresses(lore-revision/src/state.rs) spawns a task per address callingimmutable_store().get_metadata(). On the AWS store that is a DynamoDBget_itemplus an S3HEADper fragment. EachJoinSetcaps only its own tasks (MAX_TASKS = 1000), and the recursion nests one set per fragmented file, so the budgets multiply.verify_fragmentsthen spawns every query batch at once.Observed on a 2-vCPU / 4 GB server (S3/DynamoDB-backed stores):
Request handler timeout exceeded(50 s default) while pinned at 200 % CPU.LORE_ALLOCATOR=tracking) showed the live growth to be AWS SDK request machinery — hyper read buffers, TLS connector churn, ~148k HTTP header maps, ~70k SigV4 signings in 70 s — pluscollect_new_addressestask state, i.e. roughly one AWS request per fragment, thousands in flight.Change
collect_new_addresses: a process-wideSemaphore(same pattern aslore-storage/src/concurrency.rs) bounds the store lookups across every level of the recursion. The permit covers only the store reads and is released before recursing, so a parent never holds a permit while waiting on children.verify_fragments: cap the query batches in flight instead of spawning all of them. Batches that answeredSlowDownare collected and reissued together after the existing retry back-off, so the retry semantics are unchanged.Both budgets scale with the host, since what an in-flight lookup costs is CPU (TLS, signing) and memory: 64 lookups and 8 batches per CPU, clamped to [128, 1024] and [16, 128]. A 2-vCPU host gets the floor (the values validated below); 16+ cores get 1024 lookups in flight, about what the previous per-set caps allowed in practice, so capable hardware keeps its throughput.
No behavior change beyond concurrency: same results, same errors, no new configuration.
Effect
Same test host, same build otherwise:
Notes
ImmutableStore::query(100 addresses per request) instead of per-addressget_metadata, which would cut requests by ~100×. That needs the tree to know which addresses are fragment lists (nested lists for large files), so it is left as a separate discussion.Checks
cargo +nightly fmt --allno changes ·cargo clippy --all-targets -- -D warnings --no-depsclean ·cargo test -p lore-revisionandcargo test -p lore-servergreen on this branch.