Repository navigation
Conversation
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
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.
dirvine
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — T2 testnet evidence is still missing
Reviewed WithAutonomi/ant-node at head 31f028a2b84013841504783b32ae750842f9c83d, against base a6e3388e79d0907ef678bcb6ec0fcaa12822e148.
Blocking request: complete the stated before-merge gate
The PR description still says:
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.
This changes live replication scheduling, including the deliberate cap bypass for a source classified as silent. The local tests check the mechanics; they do not measure the join-wave behaviour or load on a populated network. The requested T2 evidence is therefore still absent. ADR-0024 also calls for this comparison before release (lines 233–237); the stricter before-merge requirement comes from the PR description, not from relabelling the ADR.
Minimum change: attach the approved dev-testnet run and baseline comparison to this PR. Identify the revisions, topology and join wave; provide drain time, stored bytes/completeness, fetch admission drops, trust events and host load. Show that the run completes without a fetch/parking stall or unacceptable load regression. The local e2e tests and scheduler simulation are not substitutes for this run. If the team intends to move this gate to release-only, record the authorised human decision explicitly; I have not waived it or started a testnet.
No severe introduced code defect was confirmed in this re-review. This verdict is for the unmet, explicitly documented gate, not a claim that a missing reviewer is a code defect.
Checks I ran on this exact head
Runtime: macOS 26.4, rustc/cargo 1.96.0. Detached checkout remained clean.
cargo test --locked --features test-utils --lib: 1,236 passed, none failed.cargo test --locked --features test-utils --test e2e fetch_per_source_share:: -- --nocapture --test-threads=1: 2 passed.- Answering-holder case: maximum in flight to the holder 2, with global concurrency 8 and 12 keys.
- Silent-holder case: maximum 8, drained after 15.123578667 s, with a 5 s timeout. This verifies the intentional exception, not a zero-admission-drop guarantee.
cargo fmt --all -- --check,git diff --check a6e3388e...HEAD, andpython3 scripts/adr-governance.py: passed.
The current GitHub checks pass except Security Audit. Cargo.lock is byte-identical to the base revision; local cargo audit --no-fetch --json reports seven existing advisories. This is an inherited dependency/audit issue, not a dependency change introduced here. It remains red; I am not describing CI as fully green. The additional runs listed by the author are not counted as runs I performed.
Review coverage and dissent
Partial panel: 2/3 named models contributed — GLM 5.3 and Qwen 3.8. No full-team consensus. Both correctness/evidence and adversarial roles have substantive responses. The source was split into scheduler and engine scopes: 6/12 original scoped passes completed. Focused GLM/Qwen follow-ups connected the caller/callee seams; those are not extra independent votes.
- GLM 5.3 (
z-ai/glm-5.3): substantive engine reviews and a focused adversarial scheduler/engine retry. Full scheduler correctness still failed with incomplete/length termination after retry. Its focused review found no confirmed defect in the supplied seams and explicitly marked the T2 gate CHANGES_REQUIRED. - Qwen 3.8 (
qwen38-27b): completed both scopes and both roles. Its correctness follow-up withdrew the proposed infinite re-parking loop and unbounded silent-source dispatch concerns:nearest_parkedfilters out blocked sources, and engine global concurrency bounds the deliberate bypass. Its adversarial follow-up also withdrew the proposed lost-wakeup defect: the notified future is enabled before reading the gate flag. - DS4 (
deepseek-v4-flash): no substantive completed review. Scheduler/other attempts and the narrowed retry hit the model-lock queue timeout; the original correctness-engine request exceeded its response deadline. These are missing coverage, not agreement.
Residual dissent: Qwen's adversarial follow-up still returned CHANGES_REQUIRED, but its remaining counter concern was described in its own explanation as a design choice, not a demonstrated fault. I do not accept that as an additional blocker: unreachable counts unanswered attempts, not new marks; silent_peak is a per-report-window peak seeded from the current count. GLM independently confirmed those semantics. I have not converted Qwen's verdict into an approval or counted omitted source in an individual scope as proof of a defect.
The intended limits remain important: silent sources may receive more than two concurrent fetches, subject to global concurrency; older requesters can still overshoot; and the ADR documents substantial worst-case parked-index memory. The missing join-wave run must test the network effect. No merge, deployment or release approval is implied.
31f028a to
447c000
Compare
Bring in WithAutonomi#252 (447c000, on main b0e29c3) so that this branch is the combined build WithAutonomi#252 and WithAutonomi#253 will run as. The busy tracker now feeds WithAutonomi#252's scheduler. ReplicationQueues holds the same SourceBusyTracker that the fetches record busy answers in. Its dispatch and its retry put a holder that recently answered busy after every other free holder, and still before a silent one. The tracker's own source choice is removed, and retry_fetch is back to main's form. A busy answer is a response, so it already counts as an answer for WithAutonomi#252's silent-source tracking. A busy wait keeps its attempt in flight, so it counts against the holder's per-source share. Both branches added the same fetch serve gate hooks; one copy is kept.
With WithAutonomi#252's per-source scheduler, one requester no longer sends more than a holder's two-request share, so the over-share measurement can no longer produce a busy reply. Keep the measurement of the refusals that WithAutonomi#252 leaves, a holder whose admission pool is full of other peers' requests, and say so in the file's description.
…hat refusals answer busy WithAutonomi#252's a_holder_that_stops_answering_is_fetched_from_past_its_share closes the holder's fetch serve gate so that the holder answers nothing. With busy replies, that holder still answers busy to the requests it refuses. It is then not silent, and WithAutonomi#252's per-source cap rightly applies to it. Add a test-only switch, turn_off_fetch_busy_replies_for_test, that makes a holder refuse silently. Use it there, so that the test again models a holder that has stopped answering. Also correct the A/B file's description of the test hooks it needs.
…ld measurements Record how the busy reply works with WithAutonomi#252's per-source scheduler: a cooling holder goes after every other free holder, a busy answer counts as an answer, and a busy wait counts against the holder's share. Replace the measurements with runs on the combined build and on WithAutonomi#252 alone. Keep the earlier runs without WithAutonomi#252 as history.
Under WithAutonomi#252's per-source share, a holder that just answered busy is passed over only for another free holder of the same key.
…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.
447c000 to
33ec231
Compare
WithAutonomi#252 was rebased onto main 2598bcf (WithAutonomi#250) as 33ec231, which is 447c000 plus exactly WithAutonomi#250's changes. Bring it in; the only real conflict is the ADR index, which now lists ADR-0023, ADR-0024 and ADR-0025. The result is the previous head plus WithAutonomi#250's changes.
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
2598bcf4(after #250, ADR-0023), with the change itself unchanged. The only conflict was in the ADR index, where ADR-0023 and this change's ADR-0024 each add a line after ADR-0022; both lines are kept. The rebase before that, ontob0e29c32(after #256, which moves zip to 9 and rustls to 0.23.46), had no conflicts. The one ontoa6e3388e(after #239 and the 0.23.0 release) had one conflict, a textual one 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 earlier heads.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.37 s, 5.1 s and 15.2 s.Unit tests:
cargo test --features test-utils --libpasses: 1,237 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,237 pass;cargo test --lib --no-default-features: 1,192 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 main2598bcf4; 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 74 to 89 ms / 181 to 193 ms / 652 to 676 ms, peak RSS 189 to 195 MiB / 342 to 348 MiB / 1.13 GiB, departures at most 89 ms, and dropping the queues at 35 sources per key 211 to 220 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: reports two advisories added to the RustSec database after the previous head was audited:rustls-webpki0.103.15 (RUSTSEC-2026-0382) andwebpki-root-certs1.0.9 (RUSTSEC-2026-0383). This change does not touchCargo.lock, which is identical to main's, so main fails the same way; chore(deps): move rustls-webpki to 0.103.16 and webpki-root-certs to 1.0.10 for two RustSec advisories #258 moves both crates to fixed versions.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.