Skip to content

sdk/python: a peer that is gone costs connect one wait, not one per router - #546

Merged
aojea merged 2 commits into
google:mainfrom
aojea:sdk-python-dead-paths
Sep 29, 2026
Merged

aojea merged 2 commits into
google:mainfrom
aojea:sdk-python-dead-paths

Conversation

@aojea

@aojea aojea commented Sep 29, 2026

Copy link
Copy Markdown
Collaborator

The sam-sdk-python cold-path probe failed on the bananas deploy of c916264 (and in four scheduled runs before it, on the previous image), always at the node-mcp stage as hung for 120s with 1 attempts, while the JS probe passed. This is what the Python SDK did with the time.

After a rollout the routers' tables still name the pods it replaced. connect() walked the relayed paths in turn: a relay whose destination is gone answers a CONNECT only after its own 30s timeout (ConnectTimeout in go-libp2p's relay), and a dead provider was tried through every router twice, once as an admitted router and once more as the relay the DHT named for it. py-libp2p's own limits on a relayed connection's upgrade add another half minute or more when the relay accepted the circuit and the far end never speaks. The first stale provider alone used up the probe's budget.

  • session.py: the routers of each step are dialed at once, as join() and the DHT walk already do, and the first circuit that opens ends the others. The DHT step skips relays that are admitted routers, which the first step dialed. The JS SDK dials every address of a peer at once already.
  • relay.py: a relayed dial is held to DIAL_TIMEOUT as a direct one is, and a circuit given up on (timed out, failed, or cancelled because another path won) is reset so the relay drops it too.
  • mesh.py (first commit): the refresh loop and the control plane pull run in separate worker threads and both persist the credential with the same temporary file name; one probe run logged [Errno 2] No such file or directory: 'identity.key.tmp' -> 'identity.key'. The control plane also redeems only the last biscuit it issued, so two refreshes at once leave the loser with a spent one. A lock on the member serializes refresh, the pull and save.

Tests:

  • test_session.py: three routers that all wait on a dead destination cost connect() one wait; one that opens ends the others' attempts.
  • test_relay.py (new): a relay that answers CONNECT with OK and forwards nothing; the dial ends at DIAL_TIMEOUT and the relay sees the circuit let go. Hangs past 20s on the previous code.
  • test_mesh.py: the fake control plane redeems only its last biscuit, as the real one does; eight concurrent refreshes all land. Seven fail with biscuit already redeemed on the previous code.
  • TestNativeSDKsMesh gains a stalled-relay step: a Go relay that accepts every circuit and forwards nothing, dialed by every SDK member at once. Each gives up within its dial timeout and the relay counts every circuit let go. The Python member exceeds the 20s budget on the previous code.

make sdk-python (96 passed), TestNativeSDKsMesh and TestNativeSDKsAcrossRouters pass with both SDK toolchains present.

The session refreshes the credential in one worker thread and pulls from
the control plane, which may refresh as well, in another; both persist the
result with the same temporary file name. On the testnet the loser found
its temporary file already renamed by the winner:

  credential refresh failed, retrying in 30s: [Errno 2] No such file or
  directory: 'identity.key.tmp' -> 'identity.key'

The other outcome is worse: the control plane redeems only the last
biscuit it issued, so the loser presents a spent one and the refresh
fails until the next attempt. A lock on the member serializes refresh,
the pull and save; the fake control plane in the tests now redeems only
its last biscuit, as the real one does, and eight refreshes at once all
land.
…outer

After a rollout the routers' tables still name the pods it replaced, and
the sam-sdk-python probe on the testnet ran out its 120s budget on the
first provider it tried, printing nothing. connect() walked the relayed
paths in turn: a relay whose destination is gone answers a CONNECT only
after its own 30s timeout, and a dead provider was tried through every
router twice, once as an admitted router and once more as the relay the
DHT named for it. py-libp2p's limits on a relayed connection's upgrade
add another half minute or more when the relay accepted the circuit and
the far end never speaks.

The routers of each step are now dialed at once, as join() and the DHT
walk already do, and the first circuit that opens ends the others; the
DHT step skips relays that are admitted routers, which the first step
dialed. A relayed dial is held to DIAL_TIMEOUT as a direct one is, and a
circuit given up on is reset so the relay drops it too. The JS SDK dials
every address of a peer at once already.

A unit test pins the race with three routers that all wait on a dead
destination, and one that opens. The SDK mesh integration test gains a
relay that accepts every circuit and forwards nothing: every SDK member
gives up within its dial timeout and the relay sees each circuit let go.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces concurrency improvements and timeout boundaries to the Python SDK and integration tests. Specifically, it adds thread-safety locks to credential refresh and control plane synchronization in mesh.py, implements concurrent dialing of routers in session.py to prevent sequential timeouts, and bounds relayed dials with a timeout in relay.py. It also adds corresponding unit and integration tests to verify these changes under concurrent and stalled conditions. There are no review comments to address, and the changes look solid.

@aojea
aojea merged commit fe0813f into google:main Sep 29, 2026
19 checks passed
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.

1 participant