test(kad_dht): cover the per-query FIND_NODE timeout against a silent peer

[yashksaini-coder]

What

Reshaped per review: the unbounded-read hang from #1434 was already fixed on main by 96151e1 (move_on_after(QUERY_TIMEOUT) inside _query_peer_for_closest). This PR now only:

• keeps a silent-peer regression test, pointed at _query_peer_for_closest (the real I/O choke point) — a peer that opens the stream and never replies yields [] after QUERY_TIMEOUT;
• adds one accurate changelog entry for the bound (96151e1 never got a user-facing note).

The redundant outer wrapper and its stale comments are gone (peer_routing.py is unchanged vs main). #1434 closed as fixed by 96151e1.

Refs #1434

[acul71]

Recommended path

The hang in #1434 is already fixed on main by 96151e12 (inner move_on_after(QUERY_TIMEOUT) in _query_peer_for_closest). Please reshape this PR as follows:

1. Drop the redundant outer move_on_after wrapper in _query_single_peer_for_closest (and the incorrect comments that say the inner path has no timeout).
2. Keep a silent-peer regression test, but point it at _query_peer_for_closest (the real I/O choke point).
3. Reword or drop newsfragments/1434.bugfix.rst so the changelog does not claim this PR introduced the only timeout. If 96151e12 never got a user-facing note, a single accurate bugfix fragment is fine; otherwise drop the duplicate.
4. Close #1434 as already fixed by 96151e12 (or retarget it only if you want a follow-up for bounding stream.close() / stream.reset() in finally — that is a separate, smaller change).

Optional follow-up (not required for this PR): bound await stream.close() with its own short timeout (or reset() on query timeout) instead of stacking two identical QUERY_TIMEOUT scopes.

---

Review: #1435 — fix(kad_dht): bound network lookups with a per-query timeout

• PR: #1435
• Issue: #1434
• Author: yashksaini-coder
• Review date: 2026-08-30

---

1. Summary of Changes

This PR intends to fix #1434: a silent/malicious peer that opens a Kademlia query stream but never replies would hang find_peer, provide, find_providers, and routing-table refresh because peer_routing._query_peer_for_closest allegedly read the response with no timeout.

What the PR actually changes:

• Wraps _query_single_peer_for_closest in trio.move_on_after(QUERY_TIMEOUT) and logs when the cancel scope is caught.
• Adds newsfragments/1434.bugfix.rst.
• Adds a unit test that a silent peer (stream.read → trio.sleep_forever) does not hang _query_single_peer_for_closest.

Modules: libp2p/kad_dht/peer_routing.py (Kademlia iterative FIND_NODE lookup), plus tests and a newsfragment. No public API, deprecations, or breaking changes.

Critical context: the hang described in #1434 is already fixed on main. Commit 96151e12 (2026-08-03, 10 days before the issue) wrapped _query_peer_for_closest itself in trio.move_on_after(QUERY_TIMEOUT). The issue text still talks about unbounded stream.read() around line 338; current main reads via read_varint_prefixed_bytes_limited inside that inner timeout. This PR’s comments, description, and newsfragment do not reflect that.

---

2. Branch Sync Status and Merge Conflicts

Branch Sync Status

• Status: Ahead of origin/main (not behind).
• Details: 0 behind, 6 ahead (one functional commit plus five merges of main).

Merge Conflict Analysis

✅ No merge conflicts detected. The PR branch can be merged cleanly into origin/main.

---

3. Strengths

• Chooses the existing QUERY_TIMEOUT constant and Trio move_on_after / cancelled_caught APIs already used in provider_store and kad_dht.
• Regression test uses autojump_clock plus an outer fail_after, so it will not stall CI if the query is unbounded.
• Newsfragment is present, named 1434.bugfix.rst, ReST-formatted, user-facing, and ends with a newline.
• PR body links the issue (Closes #1434).
• GitHub Actions for this PR are fully green (Linux tox matrix, Windows, docs).

---

4. Issues Found

Critical

None. The added wrapper is not a logic regression; it is largely redundant with code already on main.

Major

• File: libp2p/kad_dht/peer_routing.py
• Line(s): 158–180
• Issue: The stated bug is already fixed. _query_peer_for_closest already bounds the stream open/write/read in trio.move_on_after(QUERY_TIMEOUT) (lines 375–416). The new outer wrapper does not add a second, independent timeout budget for the silent-peer hang, and the comments claiming _query_peer_for_closest has no timeout are false.
• Suggestion: Follow the recommended path above (drop wrapper; keep test on the inner method).

            # Bound the whole per-peer exchange: _query_peer_for_closest reads
            # the response with stream.read() and no timeout, so a peer that
            # opens the stream but never replies would otherwise block this
            # query — and the surrounding lookup nursery — forever. Mirror the
            # move_on_after(QUERY_TIMEOUT) guard already used in provider_store
            # and value_store.
            with trio.move_on_after(QUERY_TIMEOUT) as cancel_scope:
                result = await self._query_peer_for_closest(peer, target_key)
                # ...
            if cancel_scope.cancelled_caught:
                logger.debug(
                    "Query to peer %s timed out after %ss", peer, QUERY_TIMEOUT
                )

Already on main (lines 361–416):

    async def _query_peer_for_closest(self, peer: ID, target_key: bytes) -> list[ID]:
        """
        Query a peer for their closest peers to the target key using varint
        length prefix. Each operation has a timeout to prevent hanging on
        unresponsive peers.
        """
        # ...
        try:
            with trio.move_on_after(QUERY_TIMEOUT):
                # ...
                stream = await self.host.new_stream(peer, [PROTOCOL_ID])
                # ...
                response_bytes = await read_varint_prefixed_bytes_limited(
                    stream, MAX_DHT_MESSAGE_SIZE
                )

• File: newsfragments/1434.bugfix.rst

• Line(s): 1

• Issue: Changelog text tells users that lookups could previously block indefinitely and that this PR bounds them. On current main they are already bounded. Shipping this fragment as-is over-claims the user-visible change.

• Suggestion: Reword to a single accurate user-facing note if 96151e12 never got changelog credit; otherwise drop the duplicate.

• File: tests/core/kad_dht/test_unit_peer_routing.py

• Line(s): 462–491

• Issue: The new test would pass on main without this PR, because the inner move_on_after already cancels stream.read. It does not prove the new wrapper is what prevents the hang.

• Suggestion: Keep a silent-peer regression test, but target _query_peer_for_closest directly.

Minor

• File: libp2p/kad_dht/peer_routing.py

• Line(s): 165–166 vs 483–485

• Issue: Nested identical deadlines. The outer scope starts first, so it expires slightly before the inner one. Cancellation then hits finally: await stream.close(), which previously ran outside the inner cancel scope and could complete. Same-duration nesting is a fragile way to bound cleanup and can interrupt close.

• Suggestion: If the residual risk is a hanging close(), wrap that call in its own short move_on_after (or reset()), rather than stacking two QUERY_TIMEOUT scopes.

• File: libp2p/kad_dht/peer_routing.py

• Line(s): 177–180

• Issue: Timeout is logged at debug, same as generic query failures. A silent peer is the DoS scenario the issue cares about; warning would be easier to spot in production.

• Suggestion: Use logger.warning (or info) for timeout, keep debug for expected empty results.

---

5. Security Review

The intent is correct: unbounded DHT RPC reads are a liveness/DoS issue (one silent peer stalling iterative lookup). That class of bug is already mitigated on main by the inner timeout.

• Risk: Residual hang if stream.close() never returns after the inner timeout. Nested QUERY_TIMEOUT may cancel close, but is not a dedicated cleanup bound.
• Impact: low
• Mitigation: Timeout stream.close() separately; consider stream.reset() on query timeout so the muxer does not wait for a graceful close.

No new subprocess, file, key-handling, or logging of secrets. Payload path still uses read_varint_prefixed_bytes_limited and signed-record checks.

---

6. Documentation and Examples

• _query_peer_for_closest already documents a per-operation timeout; the new comments on _query_single_peer_for_closest contradict that docstring.
• No public API change; no README / tutorial / example updates required.
• Local Sphinx HTML + doctest (make linux-docs, -W) succeeded with no warnings.

---

7. Newsfragment Requirement

• Issue reference: Present (Closes #1434). Mandatory issue requirement is met.
• File: newsfragments/1434.bugfix.rst — correct name, .bugfix type, trailing newline.
• Content: Format is valid; the claim is stale relative to main. Reword or drop before merge so the changelog is truthful.

---

8. Tests and Validation

New test TestPeerRouting.test_query_single_peer_times_out_on_silent_peer PASSED locally. GitHub Actions for this PR are fully green.

Local make lint / make typecheck / make linux-docs: all passed.

Local make test: 3 failed, 3427 passed, 15 skipped, 4 warnings, 3 rerun. The three failures (test_put_and_get_value, test_provide_and_find_providers, test_reissue_when_listen_addrs_change) assert get_peer_record(...) is an Envelope but get None during dht_pair setup — a pre-existing signed-peer-record race, not caused by this diff. CI green supports treating them as local flakes.

---

9. Recommendations for Improvement

1. Re-read _query_peer_for_closest on current main and update #1434: the unbounded stream.read hang is already gone as of 96151e12.
2. Prefer one timeout at the real I/O choke point (_query_peer_for_closest), plus an explicit bound on stream.close() / stream.reset() in finally if needed later.
3. Keep a silent-peer test; point it at the method that actually reads the stream.
4. Reword or drop 1434.bugfix.rst so the next changelog does not claim this PR introduced the only timeout.
5. Log query timeouts at warning level (optional).

---

10. Questions for the Author

1. Did you branch from a tree where _query_peer_for_closest still used unbounded stream.read, and miss 96151e12 after merging main?
2. Is the outer wrapper meant to cover stream.close() hanging after the inner timeout? If so, why not timeout close directly?

---

11. Overall Assessment

• Quality Rating: Needs Work
• Security Impact: Low (intent is DoS liveness; inner timeout already provides the real bound)
• Merge Readiness: Needs fixes
• Confidence: High

Process checks (issue link, newsfragment filename, lint, types, docs, GitHub CI, clean merge) are fine. Please follow the Recommended path at the top before this can be approved.

[yashksaini-coder]

@acul71 Thanks — you're right, I'd branched before 96151e1 and the merges hid it. Reshaped as recommended in 3d5cefb7: wrapper + stale comments dropped (peer_routing.py now identical to main), the silent-peer test targets _query_peer_for_closest, and the newsfragment credits the existing bound. Closed #1434 as fixed by 96151e1. Left the stream.close() bound as a separate follow-up, as you suggested.