Skip to content

feat(storage): answer get-or-closer requests (ADR-0020) - #249

Open
mickvandijke wants to merge 11 commits into
mainfrom
mickvandijke/v2-1358-kad-chunk-get
Open

mickvandijke wants to merge 11 commits into
mainfrom
mickvandijke/v2-1358-kad-chunk-get

Conversation

@mickvandijke

@mickvandijke mickvandijke commented Oct 9, 2026 •

Copy link
Copy Markdown
Member

ADR-0020: a chunk GET returns the chunk, or closer peers, in one request, as Kademlia's FIND_VALUE does. This PR adds the ADR and the node side.

Why. On production, a native chunk read spends 82% of its time finding a holder, because the closest-peers lookup runs to convergence before the GETs start. Asking each queried peer for the chunk as well as for closer peers cuts native p50 from 5.95 s to about 1 s; see the investigation.

Node.

  • A node that holds the chunk answers GetOrCloserRequest with Found.
  • Otherwise it answers Closer, with saorsa-core's encoding of the FIND_NODE answer it would give the authenticated requester.
  • A node with storage enabled announces get-or-closer/1 in its user agent. One with storage off never answers chunk requests, so it does not announce it.
  • Get-or-closer gets its own rows in the chunk traffic summary (group 4). Each request emits the same get_rpc read event a GET does, with queue, storage, encode and send times, and request = get_or_closer to tell them apart. A Closer answer has storage outcome not_found.
  • Devnet nodes announce the token too, so a local devnet exercises the new path.
  • Browsers are unchanged. The WebRTC Direct listener still refuses the request, and HELLO does not list it. A browser variant was built and A/B'd on the dev testnet, and was not faster there (ant-client#221), so it is not part of this stack. The ADR records it and when to revisit it.

Compatibility by design.

  • Old client, new node: the request is never sent; nothing changes.
  • New client, old node: the client sees no token, so it sends FIND_NODE and a GET in the same step instead. That works with every deployed node at the same speed.
  • Lookup finds no holder: the client falls back to today's read path, so the absence rule is unchanged.

Stack, merge in this order:

  1. feat(dht): expose lookup steps for caller-driven lookups saorsa-core#173
  2. feat(chunk): add a get-or-closer request ant-protocol#44
  3. this PR
  4. feat(read): read chunks through a get-or-closer lookup (ADR-0020) ant-client#221

This PR pins saorsa-core (c3c4c81) and ant-protocol (0c6ae27) at those branches by git rev, so it cannot be published as is. Before merging, release saorsa-core#173 and ant-protocol#44, and replace the [patch.crates-io] pins with version bumps.

Review fixes (2026-10-27). Rebased onto main again, plus:

  • Devnet nodes announce get-or-closer/1.
  • Get-or-closer carries the GET read telemetry, and its separate INFO line is gone.
  • The e2e harness answered chunk requests with its own router, which did not pass the sender, so Closer named the requester there. The old test decoded on the requester, which drops itself, so it could not catch that. The harness now answers through the node's own router (RunningNode::answer_chunk_request, test-utils). The test decodes on a third node, with a control showing the same node names the requester to anyone else.
  • The ADR records ant-client#221's read changes: routing at once, chunk requests only to likely holders, at most two at once.

Linear issue

Closes V2-1358

Risk tier

  • T0 — docs / tooling / CI / pure UX-output. Repo CI only.
  • T1 — client-only, no network-facing behavior change. CI + prod compat smoke.
  • T2 — node/client logic with behavioral surface, no protocol/format/economics change. Dev testnet + ADR.
  • T3 — protocol / storage format / payments / routing. T2 evidence + adversarial testing.

Compatibility

  • Wire: the node answers the new GetOrCloserRequest, and its user agent gains the token get-or-closer/1. The node/ prefix is kept. Every existing message is unchanged.
  • Storage: none.
  • API: additive and test-only. RunningNode::answer_chunk_request, behind test-utils, answers a chunk message as a running node's router does. ChunkRequestContext::with_requester and node::user_agent(serves_chunks) are crate-private.
  • Logs: the get_rpc event gains a request field (get or get_or_closer).

Semver impact

  • breaking
  • feature
  • fix

Test evidence

  • cargo test --lib --features test-utils: 1,193 passed. New since the review: a get-or-closer carries the GET read telemetry.

  • cargo test --test e2e --features test-utils -- --test-threads=1: 120 passed, 3 ignored, with every chunk message now answered through the node's own router.

    • test_get_or_closer_returns_chunk_or_closer_peers decodes on a third node, with a control.
    • It failed against the old harness router, which named the requester.
  • cargo clippy --all-targets --all-features -- -D warnings: clean, except lints that main also fails on this toolchain. Those are in src/replication/pointer.rs, src/storage/chunk_store.rs and tests/e2e/pointer_replication.rs, which this PR does not touch.

  • Live. A client from the ant-client PR downloaded a 612 MB production file in 56 s, against 242 s for the current client, with identical bytes. All 150 chunks came through the new lookup, on the route for nodes without the token.

  • Dev testnet comparison against main, 2026-10-27, after the review fixes. Two 60-node DigitalOcean fleets with the same layout:

    • 30% symmetric NAT across 9 regions
    • per fleet, 4 uploaders (50 MiB; 2 merkle, 2 no-merkle), 2 native downloaders and 1 headless-Chromium WASM downloader
    • 60-minute windows
    • both fleets on current main, differing only by the four PRs

    Three runs:

    1. closed loop, as staging runs its clients
    2. the same, with the code swapped between the fleets' droplets
    3. the swapped fleets, with native downloads paced to one every 40 s, so both fleets serve the same reads
    stack vs main run 1 run 2, swapped run 3, swapped, matched load
    native download p50 −52% (19.7 → 9.4 s) −47% (18.5 → 9.9 s) −43% (18.4 → 10.5 s)
    native downloads completed 733 vs 350 707 vs 372 177 vs 176
    merkle upload p50 +7.0% (p < 0.001) +1.7% (p = 0.36) −6.2% (p = 0.001)
    no-merkle upload p50 −0.2% (p = 0.90) +8.6% (p = 0.001) +0.9% (p = 0.72)
    WASM download p50 −2.0% (p = 0.43) −4.2% (p = 0.10) −2.8% (p = 0.13)
    node dial failures, minutes 10–60 (run 3: whole window) 21 vs 0 0 vs 12 0 vs 2
    failed transfers 0 0 0
    • Native downloads are 43–52% faster on whichever droplets run the stack.
    • At equal read load nothing regresses. In a closed loop, the stack's fleet serves about twice the reads, and uploads beside them run about 4% slower. Staging runs its clients closed loop, so it will see that.
    • Node dial and connection failures follow one fleet's droplets, not the code. Every run's steady-state tail sits on the same droplets (B), whichever code they run. saorsa-core#173 changes no node-side DHT path.
    • Duplicate copies: nodes sent 1.45–1.52 bytes per native byte downloaded, against 1.31–1.35 for main. Before the review fixes it was 1.54–1.68. The rest is the 500 ms hedge.
    • Reports: ant-testnet/state/comparisons/v2-1358-review-fixes-{vs-main,swapped,swapped-matched}-60m-1027/README.md.

New dependency

none

ADR

ADR-0020: Kademlia-style chunk GET: return the chunk or closer peers, added in this PR.

Mitigation / rollback

The node is passive until clients send the request. To stop clients using it, drop the user-agent token; they then fall back to FIND_NODE plus GET. Otherwise revert.

🤖 Generated with Claude Code

@dirvine dirvine left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code approval at the reviewed head

No introduced blocking runtime defect found in this PR. Production responses use the size-capped encoder; authenticated requester identity is passed into the handler. Decoder use here is test-only.

Stack coordination remains required: saorsa-core#173 has a P2 decoder-size blocker. Refresh the core patch pin at Cargo.toml:240 after that fix and rerun integration tests; propagate the resulting node head into the client. This code approval is not approval to ship the current linked dependency set or a waiver of merge/release gates.

Reviewed commit: 79385bfddc04a26b5c0747b3355364c12b9a127b.

Review method: six substantive seats across the linked set, including a completed z-ai/glm-5.2 API review recovered from a worker that timed out after saving its response; coordinator source checks and local execution. Initial functional/security/test seats recommended approval. The coordinator probe, independent adjudicator, API seat and replacement concurrency/release seats corroborate the missing decoder bound. This is not unanimous approval. The API seat recommended client approval with concerns; the coordinator withholds it because the exact client pin still exposes the defect.

Local verification (macOS, rustc 1.96.0): core closer-peers 3 passed and DHT-manager regression 143 passed; protocol all-features 108 passed; node get-or-closer unit tests 3 passed and real-transport E2E 1 passed; client kad_read unit tests 10 passed and real-transport E2E 1 passed. Formatting passed in all four repositories. No production/testnet deployment or merge performed. Draft git pins were verified with cargo metadata --locked; this is not published dependency closure. ADR-0020 remains Proposed.

@mickvandijke

Copy link
Copy Markdown
Member Author

Thanks.

  • ad88647 moves the saorsa-core pin to fd37775, the bounded closer-peers decoder. The 3 get-or-closer unit tests and the real-transport e2e pass, and CI was green (21/21).
  • 900ff66 updates ADR-0020:
    • the 64 KiB bound on closer peers;
    • the client dropping a peer whose content fails verification (ant-client#221, 7d9a248);
    • that every queried peer can now send content, listed as a trade-off;
    • the validation list.

@dirvine dirvine left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — pin update confirmed, one coordination item

Fix verified

  • saorsa-core pin now fd377754 (the bounded closer-peer decoder) in both Cargo.toml and Cargo.lock — the first-round coordination requirement is met.
  • ADR-0020 docs updated to match: oversized closer-peer payloads rejected before parsing; bad-content peers dropped.

Coordination item (not a code defect)

  • The PR head is 97759205, but ant-client#221 pins ant-node at ad886472 — a commit inside this PR's branch, not the head. The client consumes the get-or-closer handler and the fixed core pin from that mid-branch commit.
  • Please either: (a) pin the head after squash/merge, or (b) confirm ad886472 is the intended node revision. Otherwise the client's node dependency tracks an unreferenced commit once this branch merges.

Local verification

  • Handler + e2e tests at head 97759205: 3/3 and 1/1 ok (local run, macOS).

Verdict

APPROVE at head 97759205, subject to the node-revision pin being reconciled before the client ships. Panel: GLM 5.3 seat billing-failed (402) — 2/3 reviewed, provisional.

@mickvandijke
mickvandijke marked this pull request as ready for review October 13, 2026 13:00
@mickvandijke
mickvandijke force-pushed the mickvandijke/v2-1358-kad-chunk-get branch from 4ab018a to 8b29fc9 Compare October 26, 2026 11:23
mickvandijke and others added 11 commits October 27, 2026 14:25
Kademlia's FIND_VALUE for chunks. A new get-or-closer request is
answered with the chunk by a holder, and otherwise with the closer peers
its FIND_NODE answer would hold, encoded and checked by saorsa-core.
Native clients run their own iterative lookup with it. Peers that do not
announce `get-or-closer/1` in their user agent get FIND_NODE plus a GET
instead, so a new client reads at full speed from today's nodes. When
the lookup finds nothing, the client falls back to today's read path,
so the absence rule is unchanged.

Measured on production: native p50 5.95 s to about 1 s, the whole file
3.7 times faster, at 2.5-3.3 chunk copies per read.

Part of V2-1358.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ADR-0020. A node that holds the requested chunk answers `Found` with it.
Otherwise it answers `Closer` with saorsa-core's encoding of the
FIND_NODE answer it would give the authenticated requester, so clients
check those peers exactly as they check FIND_NODE answers. A node with
storage enabled announces `get-or-closer/1` in its user agent; one with
storage off never answers chunk requests, so it does not. Browsers are
not offered the request yet.

Get-or-closer traffic gets its own rx and tx rows in the chunk traffic
summary (group 4), and each answer is logged as `get_or_closer_rpc`.

Part of V2-1358.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A client built from the design read a 612 MB production file in 56 s,
against 242 s for the current client, through the per-peer fallback that
today's nodes take. In-process networks of upgraded nodes are read with
get-or-closer requests only.

Part of V2-1358.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
From review of the ADR-0020 stack:
- decode_closer_peers rejects a payload over the 64 KiB FIND_NODE limit
  before parsing it (saorsa-core fd37775). The chunk message carrying it
  may be up to 5 MiB.
- A peer whose content fails verification only drops out of the
  client's lookup (ant-client 7d9a248). Every queried peer is asked for
  the chunk, so ending the read on one bad answer would let any
  dishonest node on a lookup path fail reads. Today's read path, which
  runs when the lookup finds nothing, keeps its rule that bad content
  ends the read.
- The trade-offs now list that every queried peer can send content.
- Validation now covers both changes.

Part of V2-1358.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Browsers keep today's read. The browser variant was built and A/B'd on a
60-node testnet, and was not faster: a warm browser already holds sessions
with the chunk's close group there. The review trigger now names when to
revisit it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ADR-0023 made the ordinary read choose its first GET uniformly among a
chunk's close group, so a popular chunk no longer loads one node while
the rest of its close group idles. The get-or-closer read asked the
nearest likely holder first, which put that load back on one node for
every native read. Record that it now chooses the first likely holder
uniformly among those a lookup round hands over, and that the likely
holders are asked one at a time and hedged after 500 ms.

The user-agent token now follows `node/<version>` directly: ADR-0022
removed the migration token it used to follow.

Part of V2-1358.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… branches

saorsa-core c3c4c81 is saorsa-core#173 rebased onto 0.29.2, and
ant-protocol 0c6ae27 is ant-protocol#44 rebased onto 5.0.1, the
rc-2026.10.3 baseline this branch now sits on.

Part of V2-1358.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Devnet nodes built their core config without the node's user agent, so
they never announced the get-or-closer token. A client reading from a
local devnet therefore took the legacy route for every peer, and
ADR-0020's own path went unexercised there. Devnet nodes serve chunks,
so they now announce it, as `NodeBuilder` does for a node with storage.

Part of V2-1358.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A get-or-closer is a chunk read, but it logged only a bare
`get_or_closer_rpc` line, at INFO for every answer, and none of the GET
read event's fields: queue wait, storage read, encode and send time,
and send outcome.

- It now emits the same `get_rpc` event a GET does. A new `request`
  field tells the two apart (`get` or `get_or_closer`). A `Closer`
  answer has storage outcome `not_found`, as a GET for a chunk the node
  does not hold.
- The separate `get_or_closer_rpc` line is gone, so each read logs one
  line, as before.
- `of_get` gets its doc comment back from `of_get_or_closer`, and the
  new response keys are documented.

Part of V2-1358.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The e2e harness answered chunk requests with its own router, which
called `try_handle_request` without the sender. The get-or-closer test
then checked that the answer left out the requester after decoding it
on the requester, which drops itself from any answer it decodes, so the
check could not fail. Decoded on a third node, the harness's answers
named the requester.

- `RunningNode::answer_chunk_request` (test-utils) answers a message as
  a running node's router does: with the authenticated sender, the GET
  telemetry and the traffic counters. The harness now routes every
  chunk message through it, so the e2e suite exercises the production
  path.
- The test decodes on a third node and asks with the requester's own ID
  as the address, so an answer that kept the requester would name it
  first. As a control, the same node's answer to another peer must name
  the requester.

The strengthened test failed against the old harness router and passes
through the node's.

Part of V2-1358.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Records the review fixes in ant-client#221 and this PR:

- Every queried peer answers the lookup at once. Only the likely holder
  chosen to go next is asked for the chunk in its lookup step; other
  likely holders answer with FIND_NODE and wait for a GET, and far peers
  get FIND_NODE only.
- Until the read knows two close groups of peers, every peer counts as
  a likely holder, so the first round is gated too.
- At most two likely holders are asked at once.
- The ordinary read starts from the peers the lookup ended on.
- Node load stays as today's: routing never waits behind PUTs, and
  get-or-closer waits only as a GET does.
- Validation lists the new driver tests, the legacy-node e2e test, the
  requester control, the read telemetry and devnet nodes' token.

Part of V2-1358.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@mickvandijke
mickvandijke force-pushed the mickvandijke/v2-1358-kad-chunk-get branch from 8b29fc9 to 6a14b53 Compare October 27, 2026 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants