Repository navigation
Conversation
dirvine
left a comment
dirvine
left a comment
There was a problem hiding this comment.
Provisional review — no severe introduced defect confirmed
Reviewed WithAutonomi/ant-node at e1c7b2e6fbc990037df1a19fb905a019ab6cc832 against merge base f8008ef4e75f4b95a9cbb18442af0718a20171f5. This is a review comment, not an approval or merge/release sign-off.
Review participation
2/3 models reviewed; 4/6 independent role reviews completed. GLM 5.3 through OpenRouter and local Qwen 3.8 completed separate correctness and adversarial reviews. Their returned model identifiers matched the requested models and all four bounded responses ended with finish_reason=stop. Both found no severe introduced defect in the evidence supplied. Local DS4 Flash did not complete: the original native run exhausted its overall deadline; separate bounded replacement requests for both scopes timed out. DS4 endpoint discovery worked, but endpoint availability is not a completed review. No fallback was counted in its place. There is no full-team consensus; formal approval is withheld.
What the patch does
The cap is two fetches per answering holder, not an unconditional limit of two. The dispatcher and retry path select another startable holder or park the key instead of sending a third request to a holder still regarded as answering. Parked keys remain in the queued payload/capacity budget. Source departure, retries, verified-repair requeue, cancellation and panic handling were checked against the worker lifecycle and local tests.
The silent-holder exception is deliberate in ADR-0024 (WithAutonomi/ant-node). A holder that does not answer can be fetched from above its share, within the existing global worker concurrency limit. Contact ordering, expiry and bounded health-table overflow determine that exception. Actual wire contact is recorded separately from payload validity and local persistence, so a local write failure is not itself evidence of a silent holder.
This fits the reported V2-1514 failure mechanism: answering holders drop the third concurrent request without a reply, which turns admission shedding into timeout work. However, shedding itself can produce silence, and health-table overflow treats untracked holders as silent. This is a mitigation, not proof that all admission drops or retries disappear. It also changes upgraded requesters; it cannot retroactively throttle the old external 0.22.0 cohort. The cohort figures in the thread are author-reported evidence, not measurements freshly reproduced during this review.
Executed verification
cargo test --locked --features test-utils --lib replication::: 673 passed, 0 failed.cargo test --locked --features test-utils --lib: passed, exit 0.cargo test --locked --features test-utils --test e2e fetch_per_source_share:: -- --nocapture: both new tests passed.cargo fmt --all -- --check: passed.scripts/adr-governance.py: passed, 20 ADR files checked.
The new e2e tests cover an answering holder staying within its share and a silent holder draining through the exception. They do not replace a mixed-holder join-wave test or independently prove production admission-drop rates. No source edits, branch pushes, staging provisioning or live-network writes were made.
Remaining gates and watch-outs
- T2 dev/staging testnet is unchecked in the PR body and was not run here. Before release, supply the ADR's join-wave evidence. Check admission drops, completed fetches/bytes, bootstrap drain,
oldest_parked, source-health overflow, queue-lock wait/hold and memory. The documented worst-case parked index is large; a bounded allocation is not automatically a suitable fleet allocation. - Security Audit is red. I independently ran cargo-audit against head and base lockfiles using the same advisory database. Both have byte-identical lockfiles and fail with the same
quinn-proto 0.11.17advisories:RUSTSEC-2026-0343,RUSTSEC-2026-0338,RUSTSEC-2026-0340. This is an existing dependency problem, not introduced by this patch; it still needs tracking. The other exact-head GitHub checks passed. - DS4's two review scopes remain missing. Complete those before treating this as a full-panel approval.
I rejected unsupported reviewer hypotheses rather than copying them into findings. In particular, the proposed stale FuturesUnordered concurrency count is not a reachable oversubscription case: pushing the retry future updates the collection before the next fill pass. Reviewer-invented split filenames were not accepted as source citations. Table-overflow bypass is an explicit design trade-off, not a newly discovered universal cap violation. No responding reviewer supplied a confirmed severe blocker; that does not turn the missing reviewer into agreement.
e1c7b2e to
4eba869
Compare
…on share A fetch responder admits two outstanding requests per requesting peer and drops later arrivals without replying while that share is full. The fetch worker ran one fetch per hardware thread and sent each one to the first verified holder of its key, and the nearest-first queue often groups keys that share holders. When more than two fetch slots were free and several queued keys shared a holder, the worker sent more than that holder's share. Each resulting drop cost the full 30 s fetch timeout, a connection-timeout trust event against a holder that did nothing wrong, and a retry that ignored how busy the next holder was. The fetch worker now keeps at most FETCH_MAX_IN_FLIGHT_PER_SOURCE (the responder's share) of its fetches in flight to any one source that is answering: - Each free slot goes to the nearest queued key that can start now, on the first untried source in its preference order that is answering and below the limit, or else on its first silent source. - A key whose untried sources are all answering and full is parked: it leaves the fetch heap, is indexed exactly under each of those sources, and starts again as soon as one frees a share or falls silent, competing with the heap on distance. - A failed fetch moves on by the same rule. If every remaining source is answering and full, it is parked instead of being sent to a full holder, keeping its tried sources and its verification retry reservation. With no untried source left, or a full fetch queue, it returns to verification as before. - The sources a queued or in-flight key is associated with, tried ones included, are capped at close_group_size + paid_list_close_group_size + MAX_HINT_SOURCES_PER_KEY from the running configuration. A re-advertisement merges into such a key's verification retry entry under MAX_HINT_SOURCES_PER_KEY, as into a pending entry, instead of without a bound. A holder that stops answering has no share to keep within, and holding it to two requests per fetch timeout would drain the keys only it holds four times more slowly than before. Each attempt now records whether its source answered. Requests and answers take places in one process-wide sequence, so they are ordered exactly whatever order their outcomes are applied in. An answer reaches the scheduler as soon as it arrives, before its chunk is checked and stored, so a timeout it outdates cannot mark its source while that chunk is still being written. A source is silent while a request sent to it after its latest answer has gone unanswered, and a silent source is fetched from past the limit, as before this change, until it answers again, or until no newer unanswered request to it has been seen for two fetch timeouts (at least one second), after which it is probed under the limit. The table is bounded at 8,192 sources and drops only entries that hold no mark and guard no in-flight fetch; if that frees nothing, untracked sources count as silent for one mark lifetime, which again is the behaviour before this change. A "replication fetch dispatch summary" line every 5 minutes and at shutdown reports parked keys, index size, oldest parked age, queued and in-flight fetches, sources at the limit, parks, attempts by outcome, stored bytes, attempt latency, cancelled attempts, fill-pass lock wait and hold, parked-index maintenance time, and the silent-source counts and events. The limit removes over-share bursts but cannot rule out every refusal: a request this node has already timed out on may still hold its share at the responder until a worker sheds it. Responder admission, the fetch timeout, fetch concurrency and the wire protocol are unchanged. ADR-0024 records the decision, a local risk simulation and a scale benchmark at the fetch queue's hard bound, which runs when ANT_FETCH_SCALE_BENCH names a sources-per-key count. Test-only hooks, compiled only under cfg(test) or the test-utils feature and scoped to one engine: the fetch concurrency, a gate that holds the engine's admitted fetch requests before serving them and that engine shutdown releases, and a fetch-pipeline snapshot. With concurrency eight and twelve keys directed to one holder, the requester records at most two in-flight fetches to it. With a 5 s fetch timeout and sixteen keys directed to a holder that answers nothing, the requester sends past the share once the first requests time out and drains the keys in three timeouts.
4eba869 to
31f028a
Compare
`SourceBusyTracker::record_busy` never shortens a cooldown that is already running, but it returned the newly computed cooldown, so when an earlier, longer one was still running the requester's debug line reported less than the holder was really put last for. It now returns what is left of the cooldown in force. ant-node only logs the return value, so scheduling and wire behaviour do not change; callers of the public method now get the remaining cooldown in force. The never-shortened test now checks the value too. The same debug line said the holder was passed over "for new keys", but the cooldown applies both when a key starts and when it moves on to another holder. It now says the holder is put last. ADR-0025: - the context adds the refusals WithAutonomi#252 leaves at holders it currently counts as silent, and says WithAutonomi#252's ADR defers a busy reply rather than naming it as the remaining piece; - the trade-off on holders that answer busy to everything says they are put behind every holder that is not cooling, on retries too, and names the `fetch_busy_reply_send_pool_full` counter in full; - the requester section states the cooldown as 1, 2, 4 and 8 times the jittered wait drawn for each busy answer, capped at 60 s, says that a later busy answer never shortens a running cooldown, and gives the table's eviction order when every holder in it is cooling; - the request-rate trade-off is given for any fetch timeout, not only the default 30 s; - the requester end-to-end test is described as filling that requester's own two-request share at the holder; - `fetch_busy_sources_tracked` is listed as a gauge, apart from the cumulative counters; - the positive consequence notes that the busy wait can be cut short by the attempt's own fetch timeout; - the rejected retry-rotation option states its cost as the mechanism (one request per round trip instead of one per fetch timeout) rather than an estimate; - the order section records that this change and WithAutonomi#252 conflict and have not been built or measured together yet; - the real-node measurements are rerun on upstream main a6e3388, and the local simulation, which is not published, is named only as design input rather than offered as validation.
2 participants
Requesters now keep their own in-flight fetches to each answering holder within that holder's admission share. This removes the ordinary over-share bursts that the holder dropped unanswered, each of which cost the requester a 30 s timeout and charged the holder a trust penalty it had not earned.
The problem. A fetch responder admits two outstanding requests per requesting peer and drops later arrivals without a reply while that share is full. The fetch worker did not respect that:
When more than two fetch slots were available and several queued keys shared a holder, the worker could send more than that holder's share. Each resulting drop:
On the production fleet, the responders' own cumulative counters showed:
In that window, 68.5% of drops happened with only 2 of the responder's 12 admission slots in use, both held by the refused peer.
The change (node only; no wire, storage or production public API change). The worker keeps at most two fetches in flight to any one answering source, the same share the responder grants.
close_group_size + paid_list_close_group_size + MAX_HINT_SOURCES_PER_KEYfrom the running configuration. This never truncates a verification round's own sources, and it bounds the new index. A re-advertisement of such a key merges into its verification retry entry underMAX_HINT_SOURCES_PER_KEY, as into a pending entry, instead of without a bound.Holders that stop answering. A holder that answers nothing has no share to keep within. Holding it to two requests per timeout would drain the keys only it holds four times more slowly than before, which a local simulation confirmed (40.1 s against 10.2 s). So a source counts as silent while a request sent to it after its latest answer has gone without any response. Requests and answers are ordered by one process-wide sequence, so the verdict does not depend on the order outcomes arrive in. An answer reaches the scheduler as soon as it arrives, before its chunk is checked and stored, so a timeout it outdates cannot mark its source while that chunk is still being written. A silent source is fetched from past the limit, as before this change, until it answers again, or until two fetch timeouts pass with no newer unanswered request to it, after which it is probed under the limit. The cost is one fetch timeout to notice the silence.
Observability. A
replication fetch dispatch summaryline every 5 minutes and at shutdown reports the parked backlog and its oldest age, sources at the limit, attempts by outcome, stored bytes, attempt latency, lock wait and hold times, and the silent-source counts.What it does not do:
Linear issue
Closes V2-1514
Risk tier
Compatibility
pub(crate)); publicReplicationQueuesfetch methods keep their signatures and now apply the per-source scheduling and source limit. Additive and test-only, compiled undercfg(test)or thetest-utilsfeature:ReplicationEngine::{set_fetch_concurrency_for_test, close_fetch_serve_gate_for_test, open_fetch_serve_gate_for_test, fetch_pipeline_snapshot_for_test},replication::FetchPipelineSnapshotandReplicationQueues::fetch_pipeline_snapshot.Semver impact
Test evidence
Rebased onto main
a6e3388e(after #239 and the 0.23.0 release), with the change itself unchanged. The one conflict was textual, intests/e2e/testnet.rs: the harness now sets the fetch concurrency override and then wires pointers with #239'swith_pointer_service. The tests, gates, simulation and benchmark below were re-run on this head; the mutation checks and the "before" builds are from the previous head.Local risk simulation (12-node local network, 8 fetch slots, 5 s fetch timeout). "Before" is a build with the limit removed and no silent-source handling, which schedules exactly as before this change.
The last row is the documented one-timeout penalty: with
C ≥ 2fetch slots,T × (1 + ceil((N − 2) / C))againstT × ceil(N / C). These are the figures recorded in ADR-0024. Re-run on this head while other builds shared the machine, the three scenarios took 0.72 s, 5.2 s and 15.1 s.Unit tests:
cargo test --features test-utils --libpasses: 1,236 tests, 48 of them new (one is the opt-in scale benchmark), inscheduling.rsandmod.rs. They cover:dequeue_fetchreturning the global nearest key;New e2e tests (
tests/e2e/fetch_per_source_share.rs):Mutation checks: removing silent-source handling fails 16 unit tests and the e2e test. Ignoring silence when waking parked keys, marking a source despite a later answer, clearing a mark on any answer, not refreshing a mark on newer failures, or re-marking on older failures each fail at least one unit test. Ignoring answers reported before their fetch ends fails the early-answer test, an unbounded retry-entry merge fails the hint-bound test, and not releasing the serve gate at shutdown fails the e2e teardown.
CI test suites, run locally on this head (each CI test command, with CI's
RUSTFLAGS="-D warnings"):cargo test --lib --features test-utils: 1,236 pass;cargo test --lib --no-default-features: 1,191 pass;--test e2e --features test-utils -- --test-threads=1: 121 pass, 3 ignored (main'slive_testnettests, which this change does not touch);chunk_store_crash_safety2 (its 1 ignored test is the crash test's child process),storage_scale2,webrtc_direct_devnet3, its ignored five-node devnet test 1,poc_commitment_audit_attacks19,poc_audit_handler_live16,poc_bootstrap_stall3: all pass;Outside CI's list,
poc_d1_bounded_queues(7) andpointer_convergence(17) pass.poc_price_floor_livefailsenforced_floor_rejects_cheapest_of_k_real_settlementin exactly the same way on maina6e3388e; this change touches no payment code.Scale benchmark at the fetch queue's hard bound (131,072 parked keys, release build): one park pass 89 to 99 ms / 207 to 216 ms / 754 to 960 ms over repeated runs, and peak RSS 197 MiB / 342 MiB / 1.13 GiB, at 1 / 8 / 35 sources per key, within the thresholds declared before it ran. The exception is dropping the queues at 35 sources per key: 274 to 315 ms over repeated runs, against the 250 ms first declared. The scheduler before the silent-source handling took 226 to 295 ms, and the same time in alternating runs on one machine. It happens once at shutdown, outside any lock, so its limit is now 1 s. Re-run on this head, once through cargo and twice as the bare test binary for peak RSS: one park pass 77 to 94 ms / 184 to 239 ms / 693 to 767 ms, peak RSS 195 MiB / 342 to 348 MiB / 1.13 GiB, departures at most 103 ms, and dropping the queues at 35 sources per key 224 to 300 ms, all within the thresholds.
Gates:
cargo clippy --all-targets --all-features -- -D warningson stable and on 1.99.0, andcargo clippy -- -D warnings;cargo fmt --check;cargo docandcargo doc --all-featureswith-D warnings;cargo +1.95.0 check --all-targets --all-features --locked(MSRV);RUSTFLAGS="-D warnings" cargo build --release --no-default-featuresandcargo check --lib --no-default-features --locked;scripts/adr-governance.py.cargo audit, and with it CI's Security Audit job, fails on four rustls 0.23.45 advisories (RUSTSEC-2026-0346 to 0349). Main's own latest CI run fails the same way, and this change does not touchCargo.lock.Dev testnet: not run yet. T2 needs it before merge, ideally with a batch of fresh nodes joining a populated network, comparing drain time, stored bytes, fetch admission drops, trust events and host load against the release.
Release: this should be tested on a dev testnet before it ships, so it is not proposed for the next release unless Chris decides otherwise.
New dependency
none
ADR
https://github.com/grumbach/ant-node/blob/fix/fetch-per-holder-cap/docs/adr/ADR-0024-fetch-requesters-stay-within-the-responder-share.md
Mitigation / rollback
Revert the commit. The change is confined to requester-side fetch scheduling, with no stored state and no wire change, so a node on either side of it interoperates with every other.